mirror of
https://bitbucket.org/siakitem/my-pi.git
synced 2026-08-28 16:45:22 +00:00
369 lines
34 KiB
Markdown
369 lines
34 KiB
Markdown
---
|
||
issue: 21
|
||
issue_title: "Split src/index.ts (1,983 lines) into focused modules"
|
||
---
|
||
|
||
# Split src/index.ts into focused modules
|
||
|
||
## Problem Statement
|
||
|
||
`src/index.ts` is ~1,973 lines and houses at least seven distinct concerns beyond the extension factory that is its actual responsibility.
|
||
AGENTS.md requires "one concern per file in `src/`" — `index.ts` is the last major outlier.
|
||
|
||
## Goals
|
||
|
||
- Mechanically extract cohesive groups of functions into new focused modules.
|
||
- Reduce `src/index.ts` line count by moving all extractable module-scope functions out.
|
||
- Preserve all existing behavior — no observable change from the test suite.
|
||
- Add unit tests for the newly extracted modules and the pre-existing focused modules that lack them, using dependency injection and vitest mocks to test each module in isolation.
|
||
|
||
## Non-Goals
|
||
|
||
- Behavior changes of any kind.
|
||
- Restructuring the `piPermissionSystemExtension` factory or its closure-scoped helpers (they are inherently coupled to extension lifecycle state).
|
||
|
||
## Background
|
||
|
||
### Dependency status
|
||
|
||
| Issue | Title | Status | Relevance |
|
||
| ----- | --------------------------------------- | ------------------ | ------------------------------------------------------------------------------------- |
|
||
| #10 | Consolidate config layout | Closed/implemented | Was a prerequisite — no longer blocks. |
|
||
| #20 | Delete permission-request event channel | Closed/implemented | Removed `emitPermissionRequestEvent` and related types, reducing `index.ts` slightly. |
|
||
|
||
### What was in src/index.ts (~1,973 lines)
|
||
|
||
The file contained these function/constant groups beyond the extension factory:
|
||
|
||
1. **Active-agent detection** (~50 lines): `ACTIVE_AGENT_TAG_REGEX`, `normalizeAgentName`, `getActiveAgentName`, `getActiveAgentNameFromSystemPrompt`.
|
||
2. **External-directory / path utilities** (~70 lines): `PATH_BEARING_TOOLS`, `normalizePathForComparison`, `isPathWithinDirectory`, `getPathBearingToolPath`, `isPathOutsideWorkingDirectory`.
|
||
3. **Permission prompt formatting** (~250 lines): `formatMissingToolNameReason`, `formatUnknownToolReason`, `formatPermissionHardStopHint`, `formatDenyReason`, `formatUserDeniedReason`, `formatAskPrompt`, `formatSkillAskPrompt`, `formatSkillPathAskPrompt`, `formatSkillPathDenyReason`, `formatExternalDirectoryHardStopHint`, `formatExternalDirectoryAskPrompt`, `formatExternalDirectoryDenyReason`, `formatExternalDirectoryUserDeniedReason`.
|
||
4. **Tool-input preview / text utilities** (~120 lines): `TOOL_INPUT_PREVIEW_MAX_LENGTH`, `TOOL_INPUT_LOG_PREVIEW_MAX_LENGTH`, `TOOL_TEXT_SUMMARY_MAX_LENGTH`, `truncateInlineText`, `sanitizeInlineText`, `countTextLines`, `formatCount`, `getPromptPath`, `formatEditInputForPrompt`, `formatWriteInputForPrompt`, `formatReadInputForPrompt`, `formatSearchInputForPrompt`, `serializeToolInputPreview`, `formatJsonInputForPrompt`, `formatToolInputForPrompt`, `formatGenericToolInputForLog`, `getToolInputPreviewForLog`, `getPermissionLogContext`.
|
||
5. **Subagent context** (~25 lines): `normalizeFilesystemPath`, `isSubagentExecutionContext`.
|
||
6. **Forwarded-permission file IO** (~180 lines): `sleep`, `formatUnknownErrorMessage`, `isErrnoCode`, `logPermissionForwardingWarning`, `logPermissionForwardingError`, `ensureDirectoryExists`, `getPermissionForwardingLocationForSession`, `ensurePermissionForwardingLocation`, `getExistingPermissionForwardingLocation`, `tryRemoveDirectoryIfEmpty`, `cleanupPermissionForwardingLocationIfEmpty`, `safeDeleteFile`, `writeJsonFileAtomic`, `readForwardedPermissionRequest`, `readForwardedPermissionResponse`.
|
||
7. **Forwarded-permission polling + confirmation** (~180 lines): `formatForwardedPermissionPrompt`, `waitForForwardedPermissionApproval`, `processForwardedPermissionRequests`, `confirmPermission`.
|
||
8. **Misc helpers** (~30 lines): `extractSkillNameFromInput`, `getEventToolName`, `getEventInput`, `getContextSystemPrompt`, `getSessionId`, `canRequestPermissionConfirmation`, `derivePiProjectPaths`, `createPermissionManagerForCwd`.
|
||
|
||
### Permission surfaces affected
|
||
|
||
None — pure refactor.
|
||
|
||
### Test coverage gap
|
||
|
||
Before this issue, the entire `src/index.ts` — and every pre-existing focused module that was *already* extracted (`bash-filter.ts`, `wildcard-matcher.ts`, `system-prompt-sanitizer.ts`, `skill-prompt-sanitizer.ts`, `permission-manager.ts`, etc.) — was tested exclusively through 2 integration test files:
|
||
|
||
| File | Tests | Lines |
|
||
| --------------------------------- | ----- | ----- |
|
||
| `tests/permission-system.test.ts` | 68 | 2,490 |
|
||
| `tests/session-start.test.ts` | 2 | 114 |
|
||
|
||
These are flat lists of `test()` calls (no `describe()` grouping) that exercise the full `piPermissionSystemExtension` factory end-to-end via a mock `ExtensionAPI`.
|
||
The modules listed below have **no dedicated unit test file** at all:
|
||
|
||
- `src/active-agent.ts` (58 lines) — newly extracted
|
||
- `src/bash-filter.ts` (51 lines) — pre-existing
|
||
- `src/before-agent-start-cache.ts` (44 lines) — pre-existing
|
||
- `src/common.ts` (88 lines) — pre-existing
|
||
- `src/external-directory.ts` (113 lines) — newly extracted
|
||
- `src/logging.ts` (118 lines) — pre-existing
|
||
- `src/permission-dialog.ts` (89 lines) — pre-existing
|
||
- `src/permission-forwarding.ts` (126 lines) — pre-existing
|
||
- `src/permission-manager.ts` (941 lines) — pre-existing
|
||
- `src/permission-prompts.ts` (131 lines) — newly extracted
|
||
- `src/skill-prompt-sanitizer.ts` (344 lines) — pre-existing
|
||
- `src/status.ts` (35 lines) — pre-existing
|
||
- `src/subagent-context.ts` (52 lines) — newly extracted
|
||
- `src/system-prompt-sanitizer.ts` (210 lines) — pre-existing
|
||
- `src/tool-input-preview.ts` (206 lines) — newly extracted
|
||
- `src/tool-registry.ts` (139 lines) — pre-existing
|
||
- `src/wildcard-matcher.ts` (84 lines) — pre-existing
|
||
- `src/yolo-mode.ts` (29 lines) — pre-existing
|
||
- `src/forwarded-permissions/io.ts` (328 lines) — newly extracted
|
||
- `src/forwarded-permissions/polling.ts` (334 lines) — newly extracted
|
||
|
||
### Design challenges still in index.ts
|
||
|
||
After extraction, `src/index.ts` is ~970 lines.
|
||
The factory function itself is ~740 lines because it owns:
|
||
|
||
1. **Module-scope mutable state** — `extensionConfig`, `extensionLogger`, `loggingWarningReporter`, and `reportedLoggingWarnings` live outside the factory.
|
||
The factory writes to them via `setExtensionConfig` / `setLoggingWarningReporter` and every extracted module that needs logging receives it via setter injection (`setForwardedPermissionLogger`).
|
||
This works but creates hidden temporal coupling: callers must call the setter before any logging function is invoked.
|
||
|
||
2. **Module-scope constants derived from `getAgentDir()`** — `PI_AGENT_DIR`, `SESSIONS_DIR`, `SUBAGENT_SESSIONS_DIR`, `PERMISSION_FORWARDING_DIR`, and `GLOBAL_LOGS_DIR` are all computed at import time, which violates the AGENTS.md rule *"Do not cache `getAgentDir()` at module scope."*
|
||
They happen to work because `getAgentDir()` returns a stable value in production, but they make the module difficult to test in isolation (tests set `PI_CODING_AGENT_DIR` after import).
|
||
|
||
3. **Closure-scoped helpers that could be pure** — `refreshExtensionConfig`, `saveExtensionConfig`, `resolveAgentName`, `shouldExposeTool`, `logResolvedConfigPaths`, `reviewPermissionDecision`, `promptPermission`, `startForwardedPermissionPolling`, and `stopForwardedPermissionPolling` are all closures over `permissionManager`, `runtimeContext`, `extensionConfig`, and the forwarding timer.
|
||
Some of these (e.g., `resolveAgentName`, `shouldExposeTool`) could be pure functions if given their dependencies as parameters; others (e.g., `startForwardedPermissionPolling`) genuinely need mutable timer state.
|
||
|
||
4. **Six event handlers inline** — `session_start`, `resources_discover`, `session_shutdown`, `before_agent_start`, `input`, and `tool_call` are defined inline as lambdas inside the factory.
|
||
The `tool_call` handler alone is ~250 lines.
|
||
These could be separate named functions that receive a context object with the shared state.
|
||
|
||
Addressing these is out of scope for this issue (pure refactor) but would be the next step toward a testable, sub-300-line `index.ts`.
|
||
|
||
## Design Overview
|
||
|
||
### New module layout (implemented)
|
||
|
||
| New file | Concern | Actual lines |
|
||
| -------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------ |
|
||
| `src/active-agent.ts` | Agent name extraction from session metadata and system prompt | 58 |
|
||
| `src/external-directory.ts` | Path normalization, outside-cwd detection, `PATH_BEARING_TOOLS`, external-directory format helpers | 113 |
|
||
| `src/permission-prompts.ts` | All `format*` helpers for ask/deny/user-denied prompts | 131 |
|
||
| `src/tool-input-preview.ts` | Text utilities and tool-input formatting for prompts and logs | 206 |
|
||
| `src/subagent-context.ts` | `isSubagentExecutionContext`, `normalizeFilesystemPath` | 52 |
|
||
| `src/forwarded-permissions/io.ts` | Atomic JSON write, request/response read, directory ensure/cleanup, error helpers, logger setter | 328 |
|
||
| `src/forwarded-permissions/polling.ts` | `waitForForwardedPermissionApproval`, `processForwardedPermissionRequests`, `confirmPermission`, `getSessionId`, `getContextSystemPrompt`, `formatForwardedPermissionPrompt`, `PermissionForwardingDeps` | 334 |
|
||
|
||
`src/index.ts` retains (~970 lines):
|
||
|
||
- Imports from the new modules and existing ones.
|
||
- Module-scope logging state and helpers (`extensionConfig`, `extensionLogger`, `writeDebugLog`, `writeReviewLog`, etc.).
|
||
- Module-scope constants (`PI_AGENT_DIR`, `SESSIONS_DIR`, etc.).
|
||
- Small helpers tightly coupled to module state (`extractSkillNameFromInput`, `getEventToolName`, `getEventInput`, `canRequestPermissionConfirmation`, `derivePiProjectPaths`, `createPermissionManagerForCwd`).
|
||
- `piPermissionSystemExtension` factory (the default export) with closure-scoped helpers and six event handlers.
|
||
|
||
### Module dependency direction
|
||
|
||
```text
|
||
index.ts
|
||
├── active-agent.ts
|
||
├── external-directory.ts
|
||
├── permission-prompts.ts
|
||
│ └── tool-input-preview.ts
|
||
├── subagent-context.ts
|
||
└── forwarded-permissions/
|
||
├── io.ts
|
||
└── polling.ts (imports io.ts, active-agent.ts, subagent-context.ts)
|
||
```
|
||
|
||
No new module imports from `index.ts` — dependency flows one way (index → modules).
|
||
|
||
### Dependency injection patterns used
|
||
|
||
- `isSubagentExecutionContext(ctx, subagentSessionsDir)` — takes the directory as a parameter instead of reading a module-scope constant.
|
||
- `getToolInputPreviewForLog(result, input, pathBearingTools)` / `getPermissionLogContext(result, input, pathBearingTools)` — receive the `PATH_BEARING_TOOLS` set as a parameter.
|
||
- `ensurePermissionForwardingLocation(forwardingDir, sessionId)` / `getExistingPermissionForwardingLocation(forwardingDir, sessionId)` — receive the forwarding directory as a parameter.
|
||
- `setForwardedPermissionLogger({ writeReviewLog, writeDebugLog })` — setter injection for the IO module's logger.
|
||
- `PermissionForwardingDeps` — context object passed to `confirmPermission`, `processForwardedPermissionRequests`, and `waitForForwardedPermissionApproval` carrying `forwardingDir`, `subagentSessionsDir`, `writeReviewLog`, `requestPermissionDecisionFromUi`, and `shouldAutoApprove`.
|
||
|
||
### Testing strategy: dependency injection and vitest mocks
|
||
|
||
Every module has collaborators — other modules it imports and calls.
|
||
Unit tests should verify each module in isolation by mocking its collaborators with `vi.mock()` and, where needed, `vi.fn()` / `vi.spyOn()`.
|
||
This ensures tests exercise the module's own logic and boundary conditions without coupling to the real behavior of dependencies.
|
||
|
||
**Guiding principles:**
|
||
|
||
1. **Mock collaborators, not the module under test.**
|
||
If `permission-prompts.ts` imports `formatToolInputForPrompt` from `tool-input-preview.ts`, the permission-prompts tests mock `tool-input-preview.ts` and verify that the prompts module calls it with the right arguments and uses its return value correctly.
|
||
2. **Use `vi.mock()` for module-level imports.**
|
||
Vitest hoists `vi.mock()` calls so the module under test receives mocked versions of its dependencies at import time.
|
||
3. **Use `vi.fn()` for injected function dependencies.**
|
||
When a function takes a callback or deps object (e.g., `PermissionForwardingDeps`), pass `vi.fn()` stubs and assert they were called correctly.
|
||
4. **Mock `ExtensionContext` as a plain object.**
|
||
The Pi `ExtensionContext` is an interface — tests construct minimal objects satisfying only the properties the module actually reads (e.g., `{ sessionManager: { getEntries: vi.fn() } }`).
|
||
5. **Mock filesystem operations.**
|
||
Modules that use `node:fs` (`forwarded-permissions/io.ts`) should have `node:fs` mocked via `vi.mock("node:fs")` so tests never touch the real filesystem.
|
||
6. **Restore mocks between tests.**
|
||
Use `afterEach(() => { vi.restoreAllMocks(); })` to prevent test pollution.
|
||
|
||
**Example pattern for a module with collaborators:**
|
||
|
||
```typescript
|
||
import { describe, expect, test, vi, afterEach } from "vitest";
|
||
|
||
// Mock the collaborator module before importing the module under test.
|
||
vi.mock("../tool-input-preview.js", () => ({
|
||
formatToolInputForPrompt: vi.fn(() => "mocked preview"),
|
||
}));
|
||
|
||
import { formatAskPrompt } from "../src/permission-prompts.js";
|
||
import { formatToolInputForPrompt } from "../src/tool-input-preview.js";
|
||
|
||
afterEach(() => {
|
||
vi.restoreAllMocks();
|
||
});
|
||
|
||
describe("formatAskPrompt", () => {
|
||
test("includes tool input preview for non-bash tools", () => {
|
||
const result = formatAskPrompt(
|
||
{ toolName: "read", state: "ask", source: "tool" },
|
||
"my-agent",
|
||
{ path: "/foo" },
|
||
);
|
||
expect(formatToolInputForPrompt).toHaveBeenCalledWith("read", { path: "/foo" });
|
||
expect(result).toContain("mocked preview");
|
||
});
|
||
});
|
||
```
|
||
|
||
### Export strategy
|
||
|
||
Each new module exports only the functions and constants that `index.ts` (or sibling modules) actually reference.
|
||
`PermissionReviewSource` stays in `index.ts` as it is only used there.
|
||
|
||
### Module-scope constant rule
|
||
|
||
Per AGENTS.md, `getAgentDir()` must not be cached at module scope.
|
||
The extracted modules receive directory values as parameters; `index.ts` calls `getAgentDir()` at invocation time inside closures (no change from current behavior).
|
||
Constants like `ACTIVE_AGENT_TAG_REGEX`, `PATH_BEARING_TOOLS`, and length limits are safe at module scope since they do not depend on the environment.
|
||
|
||
## Module-Level Changes
|
||
|
||
### Added
|
||
|
||
| File | Content |
|
||
| -------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||
| `src/active-agent.ts` | `ACTIVE_AGENT_TAG_REGEX`, `normalizeAgentName`, `getActiveAgentName`, `getActiveAgentNameFromSystemPrompt` |
|
||
| `src/external-directory.ts` | `PATH_BEARING_TOOLS`, `normalizePathForComparison`, `isPathWithinDirectory`, `getPathBearingToolPath`, `isPathOutsideWorkingDirectory`, `formatExternalDirectoryHardStopHint`, `formatExternalDirectoryAskPrompt`, `formatExternalDirectoryDenyReason`, `formatExternalDirectoryUserDeniedReason` |
|
||
| `src/permission-prompts.ts` | `formatMissingToolNameReason`, `formatUnknownToolReason`, `formatPermissionHardStopHint`, `formatDenyReason`, `formatUserDeniedReason`, `formatAskPrompt`, `formatSkillAskPrompt`, `formatSkillPathAskPrompt`, `formatSkillPathDenyReason` |
|
||
| `src/tool-input-preview.ts` | `truncateInlineText`, `sanitizeInlineText`, `countTextLines`, `formatCount`, `getPromptPath`, `formatEditInputForPrompt`, `formatWriteInputForPrompt`, `formatReadInputForPrompt`, `formatSearchInputForPrompt`, `serializeToolInputPreview`, `formatJsonInputForPrompt`, `formatToolInputForPrompt`, `formatGenericToolInputForLog`, `getToolInputPreviewForLog`, `getPermissionLogContext`, length constants |
|
||
| `src/subagent-context.ts` | `normalizeFilesystemPath`, `isSubagentExecutionContext` |
|
||
| `src/forwarded-permissions/io.ts` | `sleep`, `formatUnknownErrorMessage`, `isErrnoCode`, `logPermissionForwardingWarning`, `logPermissionForwardingError`, `ensureDirectoryExists`, `getPermissionForwardingLocationForSession`, `ensurePermissionForwardingLocation`, `getExistingPermissionForwardingLocation`, `tryRemoveDirectoryIfEmpty`, `cleanupPermissionForwardingLocationIfEmpty`, `safeDeleteFile`, `writeJsonFileAtomic`, `readForwardedPermissionRequest`, `readForwardedPermissionResponse`, `listRequestFiles`, `setForwardedPermissionLogger`, `ForwardedPermissionLogger` |
|
||
| `src/forwarded-permissions/polling.ts` | `getSessionId`, `getContextSystemPrompt`, `formatForwardedPermissionPrompt`, `waitForForwardedPermissionApproval`, `processForwardedPermissionRequests`, `confirmPermission`, `PermissionForwardingDeps` |
|
||
|
||
### Changed
|
||
|
||
| File | Change |
|
||
| -------------- | -------------------------------------------------------------------------------------------------------- |
|
||
| `src/index.ts` | Removed extracted functions/constants; added imports from new modules; reduced from ~1,973 to ~970 lines |
|
||
|
||
### Unchanged
|
||
|
||
All pre-existing modules (`src/permission-manager.ts`, `src/bash-filter.ts`, `src/wildcard-matcher.ts`, `src/system-prompt-sanitizer.ts`, `src/skill-prompt-sanitizer.ts`, `src/extension-config.ts`, etc.) and test files are not modified.
|
||
|
||
## TDD Order
|
||
|
||
### Phase 1: Extract modules (steps 1–8, completed)
|
||
|
||
Since this was a pure mechanical refactor with no behavior change, the cycle was extract → verify → commit.
|
||
Existing tests passed after each step without modification.
|
||
|
||
1. ✅ **Extract `src/active-agent.ts`** — move agent-name detection functions.
|
||
Commit: `refactor: extract active-agent detection into src/active-agent.ts (#21)`
|
||
|
||
2. ✅ **Extract `src/subagent-context.ts`** — move subagent detection helpers.
|
||
Commit: `refactor: extract subagent context into src/subagent-context.ts (#21)`
|
||
|
||
3. ✅ **Extract `src/tool-input-preview.ts`** — move text utilities and tool-input formatters.
|
||
Commit: `refactor: extract tool-input preview into src/tool-input-preview.ts (#21)`
|
||
|
||
4. ✅ **Extract `src/external-directory.ts`** — move path utilities and external-directory format helpers.
|
||
Commit: `refactor: extract external-directory logic into src/external-directory.ts (#21)`
|
||
|
||
5. ✅ **Extract `src/permission-prompts.ts`** — move ask/deny/user-denied prompt formatters (imports `tool-input-preview.ts`).
|
||
Commit: `refactor: extract permission prompts into src/permission-prompts.ts (#21)`
|
||
|
||
6. ✅ **Extract `src/forwarded-permissions/io.ts`** — move file IO, directory management, and error helpers.
|
||
Commit: `refactor: extract forwarded-permission IO into src/forwarded-permissions/io.ts (#21)`
|
||
|
||
7. ✅ **Extract `src/forwarded-permissions/polling.ts`** — move polling loop and `confirmPermission`.
|
||
Commit: `refactor: extract forwarded-permission polling into src/forwarded-permissions/polling.ts (#21)`
|
||
|
||
8. ✅ **Final cleanup** — remove dead imports, fix lint warnings.
|
||
Commit: `refactor: finalize index.ts split (#21)`
|
||
|
||
### Phase 2: Add unit tests (steps 9–20)
|
||
|
||
Each step adds a dedicated test file for a module that currently has no unit tests.
|
||
The goal is direct coverage of each module's exported functions in isolation — using `vi.mock()` to replace collaborator modules and `vi.fn()` for injected dependencies.
|
||
Tests exercise edge cases and boundary conditions that are hard to reach through the end-to-end factory tests.
|
||
|
||
Every test file should use `describe()` blocks to group tests by exported function, and `afterEach(() => { vi.restoreAllMocks(); })` to prevent cross-test pollution.
|
||
|
||
1. **`tests/wildcard-matcher.test.ts`** — test `compileWildcardPatternEntries`, `findCompiledWildcardMatch`, `findCompiledWildcardMatchForNames`.
|
||
No collaborators to mock (pure algorithm).
|
||
Cover: empty patterns, exact match, glob `*` matching, last-match-wins precedence, multi-name lookup, no-match returns null.
|
||
Commit: `test: add unit tests for wildcard-matcher (#21)`
|
||
|
||
2. **`tests/common.test.ts`** — test `toRecord`, `getNonEmptyString`, `isPermissionState`, `extractFrontmatter`, `parseSimpleYamlMap`.
|
||
No collaborators to mock (pure functions).
|
||
Cover: non-object inputs to `toRecord`, whitespace-only strings, all three permission states, malformed frontmatter delimiters, empty YAML map, multi-line values.
|
||
Commit: `test: add unit tests for common (#21)`
|
||
|
||
3. **`tests/bash-filter.test.ts`** — test `BashFilter.check`.
|
||
Mock: `vi.mock("./wildcard-matcher.js")` to verify `BashFilter` delegates pattern matching to the wildcard-matcher and applies the default fallback correctly.
|
||
Cover: exact match, glob patterns, last-match-wins, default fallback for unmatched commands, empty command, whitespace normalization.
|
||
Commit: `test: add unit tests for bash-filter (#21)`
|
||
|
||
4. **`tests/yolo-mode.test.ts`** — test `shouldAutoApprovePermissionState`, `canResolveAskPermissionRequest`.
|
||
No collaborators to mock (pure functions taking config/flags).
|
||
Cover: yolo on/off × ask/allow/deny, subagent with no UI and yolo off, subagent with no UI and yolo on.
|
||
Commit: `test: add unit tests for yolo-mode (#21)`
|
||
|
||
5. **`tests/tool-input-preview.test.ts`** — test all exported formatters and `getPermissionLogContext`.
|
||
Mock: `vi.mock("./logging.js")` so `safeJsonStringify` returns controlled output — verifies the module delegates serialization to its collaborator and handles the result.
|
||
Cover: truncation at exact boundary, multi-line content, empty input, edit with multiple replacements, path-bearing vs non-path-bearing tools in `getPermissionLogContext`.
|
||
Commit: `test: add unit tests for tool-input-preview (#21)`
|
||
|
||
6. **`tests/external-directory.test.ts`** — test `normalizePathForComparison`, `isPathWithinDirectory`, `isPathOutsideWorkingDirectory`, `getPathBearingToolPath`, and format helpers.
|
||
Mock: `vi.mock("node:os", () => ({ homedir: vi.fn(() => "/mock/home") }))` so tilde-expansion tests are deterministic and platform-independent.
|
||
Cover: tilde expansion, relative paths, path-bearing vs non-path-bearing tools, empty strings, quoted paths, `@`-prefixed paths, format helpers with/without agent name.
|
||
Commit: `test: add unit tests for external-directory (#21)`
|
||
|
||
7. **`tests/permission-prompts.test.ts`** — test all `format*` exported functions.
|
||
Mock: `vi.mock("./tool-input-preview.js")` so `formatToolInputForPrompt` returns controlled strings — verifies `formatAskPrompt` calls the collaborator with the right tool name and input and incorporates the preview into the prompt string.
|
||
Cover: with/without agent name, MCP target, bash command with/without matched pattern, denial reason, skill path deny/ask.
|
||
Commit: `test: add unit tests for permission-prompts (#21)`
|
||
|
||
8. **`tests/active-agent.test.ts`** — test `normalizeAgentName`, `getActiveAgentName`, `getActiveAgentNameFromSystemPrompt`.
|
||
Mock `ExtensionContext` as a plain object: `{ sessionManager: { getEntries: vi.fn(() => [...]) } }`.
|
||
Cover: whitespace-only name, null, tag variations in system prompt, missing tag, session entries with `active_agent` custom type, last-entry-wins when multiple entries exist, entry with `name: null` resets.
|
||
Commit: `test: add unit tests for active-agent (#21)`
|
||
|
||
9. **`tests/subagent-context.test.ts`** — test `isSubagentExecutionContext`, `normalizeFilesystemPath`.
|
||
Mock `ExtensionContext` as a plain object: `{ sessionManager: { getSessionDir: vi.fn() } }`.
|
||
Use `vi.stubEnv()` / `vi.unstubAllEnvs()` to control `SUBAGENT_ENV_HINT_KEYS` without leaking across tests.
|
||
Cover: env variable detection (each of the 3 hint keys), session dir within/outside subagent root, missing session dir, empty env values.
|
||
Commit: `test: add unit tests for subagent-context (#21)`
|
||
|
||
10. **`tests/tool-registry.test.ts`** — test `checkRequestedToolRegistration`, `getToolNameFromValue`.
|
||
Mock: `vi.mock("./common.js")` to control `getNonEmptyString` / `toRecord` return values — verifies the registry delegates input parsing to its collaborators.
|
||
Cover: registered tool, unregistered tool, missing tool name, event with `input` vs `arguments`, empty tool list.
|
||
Commit: `test: add unit tests for tool-registry (#21)`
|
||
|
||
11. **`tests/system-prompt-sanitizer.test.ts`** — test `sanitizeAvailableToolsSection`.
|
||
No collaborators to mock (pure string transformation).
|
||
Cover: removing denied tools, preserving allowed tools, multi-section prompts, missing Available tools section, tool guidance blocks for inactive tools, empty allowed-tools list.
|
||
Commit: `test: add unit tests for system-prompt-sanitizer (#21)`
|
||
|
||
12. **`tests/skill-prompt-sanitizer.test.ts`** — test `resolveSkillPromptEntries`, `findSkillPathMatch`.
|
||
Mock `PermissionManager` as a plain object: `{ checkPermission: vi.fn() }` — verifies the sanitizer delegates permission checks to the manager and uses the returned state to decide whether to strip, keep, or mark skill blocks.
|
||
Cover: skill allow/deny/ask, path matching within/outside skill directories, multi-skill prompts, no skill blocks in prompt.
|
||
Commit: `test: add unit tests for skill-prompt-sanitizer (#21)`
|
||
|
||
## Risks and Mitigations
|
||
|
||
| Risk | Mitigation |
|
||
| -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||
| Could this silently weaken a permission? | No — pure refactor moves functions without changing logic. Every step verifies `npm test` passes, and the test suite covers tool/bash/mcp/skill/special/external-directory permission decisions. |
|
||
| Circular dependency between new modules | Dependency flows one way (index → modules → shared types). No module imports from `index.ts`. `tool-input-preview.ts` is imported by `permission-prompts.ts` only. |
|
||
| Module-scope caching of `getAgentDir()` | Extracted modules receive directory paths as parameters. `getAgentDir()` is called only in `index.ts` closures at invocation time, matching the existing pattern and the AGENTS.md rule. Note: `index.ts` itself still caches `PI_AGENT_DIR` at module scope — this is a pre-existing violation, not introduced by this change. |
|
||
| Import path breaks in tests | Only two test files import from `index.ts` (`tests/permission-system.test.ts`, `tests/session-start.test.ts`), both importing only the default export `piPermissionSystemExtension`, which remains in `index.ts`. |
|
||
| Forwarded-permission closures depend on logger state | `forwarded-permissions/io.ts` uses a setter-injected logger (`setForwardedPermissionLogger`). The setter must be called before any IO function that logs. This is wired up in the factory before `refreshExtensionConfig()`. |
|
||
| Mocked unit tests could drift from real module contracts | Unit tests verify each module's contract with its collaborators (correct arguments passed, return values used). Integration tests in `permission-system.test.ts` continue to verify end-to-end wiring with real collaborators. Both must pass — mocks catch contract violations early, integration tests catch wiring mistakes. |
|
||
| Over-mocking hides real bugs | Mock only direct collaborators (one level deep). Never mock the module under test. If a test needs to mock more than 2–3 collaborators, that is a signal the module has too many responsibilities and should be split further. |
|
||
|
||
## Open Questions
|
||
|
||
- **Should `PermissionReviewSource` move to `src/types.ts`?**
|
||
Currently only used in `index.ts`.
|
||
Defer until a second module needs it.
|
||
- **Should `extractSkillNameFromInput` move to `src/skill-prompt-sanitizer.ts`?**
|
||
It's closely related but currently only called in the `input` event handler.
|
||
Defer to keep this change mechanical.
|
||
- **Should `permission-manager.ts` (941 lines) get its own unit test file?**
|
||
It has complex logic (MCP target resolution, policy merge, caching) that would benefit from direct tests with `vi.mock("node:fs")` to control config file reads.
|
||
Deferred — it would be a large effort and the integration tests cover the main paths.
|
||
Consider as a separate issue.
|
||
- **Should the module-scope `PI_AGENT_DIR` constants be moved inside the factory?**
|
||
This would fix the AGENTS.md rule violation but requires threading the values through more call sites.
|
||
Consider as part of a future factory restructuring issue.
|
||
- **Should `forwarded-permissions/io.ts` replace setter injection with parameter injection?**
|
||
The current `setForwardedPermissionLogger` pattern creates temporal coupling.
|
||
An alternative is to pass the logger as a parameter to each function that logs (matching the `PermissionForwardingDeps` pattern in `polling.ts`).
|
||
This would make the module fully stateless and easier to test without calling a setter first.
|
||
Consider for a future cleanup.
|