23 KiB
issue, issue_title
| issue | issue_title |
|---|---|
| 282 | Extract ToolPreviewFormatter from tool-input-preview.ts |
Extract ToolPreviewFormatter from tool-input-preview.ts
Problem Statement
src/tool-input-preview.ts is a flat module of 15 exports (3 constants + 12 functions) that mix prompt formatting, log formatting, and text utilities.
The config-dependent formatting functions (formatToolInputForPrompt, sanitizeInlineText, etc.) read module-level constants (TOOL_INPUT_PREVIEW_MAX_LENGTH = 200, TOOL_TEXT_SUMMARY_MAX_LENGTH = 80) with no way to receive runtime configuration.
The call chain from PermissionGateHandler.handleToolCall -> describeToolGate -> formatAskPrompt -> formatToolInputForPrompt -> formatJsonInputForPrompt spans 5 pure-function layers.
Adding configurable limits (#266) would require threading two numbers through every layer.
This is prerequisite work for #266 (configurable preview limits).
Phase 1 of the improvement roadmap in packages/pi-permission-system/docs/architecture/architecture.md defines this extraction as Step 1 and Step 2.
Goals
- Extract a
ToolPreviewFormatterclass fromtool-input-preview.tsthat accepts limits in its constructor. - Thread the formatter through the gate descriptor chain:
describeToolGateandformatAskPromptgain a formatter parameter. PermissionGateHandler.handleToolCallconstructs the formatter with default values and passes it to the tool gate.- Eliminate the module-level
vi.mock("../src/tool-input-preview.js")inpermission-prompts.test.ts— tests inject the formatter directly. - All existing tests pass without behavioral changes.
Non-Goals
- No configuration fields added to
PermissionSystemExtensionConfig— that is #266 Step 3. - No
register()formatter extension seam — that is #283. - No change to
tool.tsdescribeToolGate's return shape,GateDescriptor,GateResult, orGateRunnerDeps. - No change to the gate pipeline in
permission-gate-handler.ts(already decomposed by #285). - No renaming of pure utility functions that stay in
tool-input-preview.ts.
Background
Current module boundaries
src/tool-input-preview.ts exports 15 items:
| Export | Category | Config-dependent? |
|---|---|---|
TOOL_INPUT_PREVIEW_MAX_LENGTH (200) |
constant | default value |
TOOL_INPUT_LOG_PREVIEW_MAX_LENGTH (1000) |
constant | default value |
TOOL_TEXT_SUMMARY_MAX_LENGTH (80) |
constant | default value |
truncateInlineText(value, maxLength) |
pure utility | no — maxLength is a parameter |
sanitizeInlineText(value, maxLength?) |
formatter | yes — default uses TOOL_TEXT_SUMMARY_MAX_LENGTH |
countTextLines(value) |
pure utility | no |
formatCount(value, singular, plural) |
pure utility | no |
getPromptPath(input) |
pure utility | no |
formatEditInputForPrompt(input) |
pure utility | no |
formatWriteInputForPrompt(input) |
pure utility | no |
formatReadInputForPrompt(input) |
pure utility | no |
formatSearchInputForPrompt(toolName, input) |
formatter | yes — calls sanitizeInlineText |
serializeToolInputPreview(input) |
pure utility | no |
formatJsonInputForPrompt(input) |
formatter | yes — uses TOOL_INPUT_PREVIEW_MAX_LENGTH |
formatToolInputForPrompt(toolName, input) |
formatter | yes — dispatches to formatJsonInputForPrompt |
formatGenericToolInputForLog(input) |
log formatter | yes — uses TOOL_INPUT_LOG_PREVIEW_MAX_LENGTH |
getToolInputPreviewForLog(result, input, pathBearingTools) |
log formatter | yes — calls config-dependent functions |
getPermissionLogContext(result, input, pathBearingTools) |
log formatter | yes — calls config-dependent functions |
Current call chain
PermissionGateHandler.handleToolCall()
└─ gate producer thunk
├─ describeToolGate(tcc, check)
│ ├─ getPermissionLogContext(check, tcc.input, PATH_BEARING_TOOLS)
│ │ └─ getToolInputPreviewForLog(result, input, pathBearingTools)
│ │ ├─ formatToolInputForPrompt(toolName, input)
│ │ │ └─ formatJsonInputForPrompt(input)
│ │ │ └─ truncateInlineText(inline, TOOL_INPUT_PREVIEW_MAX_LENGTH)
│ │ └─ formatGenericToolInputForLog(input)
│ │ └─ truncateInlineText(inline, TOOL_INPUT_LOG_PREVIEW_MAX_LENGTH)
│ └─ formatAskPrompt(check, agentName, input)
│ └─ formatToolInputForPrompt(toolName, input)
│ └─ formatJsonInputForPrompt(input)
│ └─ truncateInlineText(inline, TOOL_INPUT_PREVIEW_MAX_LENGTH)
└─ toolDescriptor.preCheck = toolCheck
Prior work
#285 (Phase 2 Step 1) has already decomposed handleToolCall into a gate pipeline with a runGate helper and validateRequestedTool prelude.
The tool gate is now a producer thunk in a gateProducers array, making the formatter injection point clean and isolated.
References
- Architecture roadmap:
packages/pi-permission-system/docs/architecture/architecture.mdlines 543–650. - Constants and config-dependent functions:
src/tool-input-preview.ts. - Gate descriptor:
src/handlers/gates/tool.ts. - Ask-prompt formatting:
src/permission-prompts.ts. - Handler orchestrator:
src/handlers/permission-gate-handler.ts. - Existing tests:
test/tool-input-preview.test.ts,test/permission-prompts.test.ts,test/handlers/gates/tool.test.ts.
Design Overview
ToolPreviewFormatter class (new file: src/tool-preview-formatter.ts)
export interface ToolPreviewFormatterOptions {
toolInputPreviewMaxLength: number;
toolTextSummaryMaxLength: number;
toolInputLogPreviewMaxLength: number;
}
export class ToolPreviewFormatter {
constructor(private readonly options: ToolPreviewFormatterOptions) {}
// Prompt formatting — config-dependent
formatToolInputForPrompt(toolName: string, input: unknown): string;
formatJsonInputForPrompt(input: unknown): string;
formatSearchInputForPrompt(toolName: string, input: Record<string, unknown>): string;
sanitizeInlineText(value: string, maxLength?: number): string;
// Log formatting — config-dependent
formatGenericToolInputForLog(input: unknown): string | undefined;
getToolInputPreviewForLog(
result: PermissionCheckResult,
input: unknown,
pathBearingTools: ReadonlySet<string>,
): string | undefined;
getPermissionLogContext(
result: PermissionCheckResult,
input: unknown,
pathBearingTools: ReadonlySet<string>,
): { command?: string; target?: string; toolInputPreview?: string; origin?: string };
}
The constructor is the single config injection point.
All config-dependent methods use this.options instead of module-level constants.
sanitizeInlineText preserves its optional maxLength override parameter — when provided, it bypasses the constructor default.
The class imports pure utilities from tool-input-preview.ts as-is (truncateInlineText, serializeToolInputPreview, getPromptPath).
tool-input-preview.ts after extraction
Keeps only: constants, pure utilities, and the tool-specific formatters that are purely structural (no config dependency):
- Module-level constants (
TOOL_INPUT_PREVIEW_MAX_LENGTH,TOOL_INPUT_LOG_PREVIEW_MAX_LENGTH,TOOL_TEXT_SUMMARY_MAX_LENGTH) truncateInlineText,countTextLines,formatCount,getPromptPathserializeToolInputPreviewformatEditInputForPrompt,formatWriteInputForPrompt,formatReadInputForPrompt
These functions have no config dependency and no reason to be instance methods.
Threading through the call chain
describeToolGate in src/handlers/gates/tool.ts:
export function describeToolGate(
tcc: ToolCallContext,
check: PermissionCheckResult,
formatter: ToolPreviewFormatter,
): GateDescriptor {
const permissionLogContext = formatter.getPermissionLogContext(
check,
tcc.input,
PATH_BEARING_TOOLS,
);
// ...
const askMessage = formatAskPrompt(
check,
tcc.agentName ?? undefined,
tcc.input,
formatter,
);
// ...
}
formatAskPrompt in src/permission-prompts.ts:
export function formatAskPrompt(
result: PermissionCheckResult,
agentName?: string,
input?: unknown,
formatter?: ToolPreviewFormatter,
): string {
// ... bash and MCP branches unchanged ...
const inputPreview = formatter
? formatter.formatToolInputForPrompt(result.toolName, input)
: "";
// ...
}
The formatter parameter is optional for backward compatibility — formatAskPrompt is not called from other entry points today, but keeping it optional avoids a forced change on consumers that don't have a formatter.
PermissionGateHandler.handleToolCall in src/handlers/permission-gate-handler.ts constructs the formatter once and passes it into the tool gate producer thunk:
const formatter = new ToolPreviewFormatter({
toolInputPreviewMaxLength: TOOL_INPUT_PREVIEW_MAX_LENGTH,
toolTextSummaryMaxLength: TOOL_TEXT_SUMMARY_MAX_LENGTH,
toolInputLogPreviewMaxLength: TOOL_INPUT_LOG_PREVIEW_MAX_LENGTH,
});
// Inside the gateProducers array, the last producer:
() => {
const toolCheck = checkPermission(
tcc.toolName,
tcc.input,
tcc.agentName ?? undefined,
getSessionRuleset(),
);
const toolDescriptor = describeToolGate(tcc, toolCheck, formatter);
toolDescriptor.preCheck = toolCheck;
return toolDescriptor;
},
When #266 adds configurable limits, the constructor call changes — the config values replace the defaults.
Design verification (Tell-Don't-Ask / Law of Demeter)
The formatter is a pure collaborator — it is called from describeToolGate to produce formatted strings, and from formatAskPrompt to produce input previews.
Neither consumer reaches through the formatter to access its options.
The consumer's call site reads naturally:
// tool.ts
const permissionLogContext = formatter.getPermissionLogContext(
check,
tcc.input,
PATH_BEARING_TOOLS,
);
The formatter encapsulates its own state (the options) and exposes only the methods that consumers need. No output arguments, no reverse searches from the original code.
Edge cases
formatAskPromptcalled without a formatter (if a future code path forgets to pass one): the optional parameter defaults toundefined, input preview is empty string, prompt still works.sanitizeInlineTextwith explicitmaxLengthoverride: still works because the override parameter bypassesthis.options.toolTextSummaryMaxLength.getToolInputPreviewForLogwith bash/mcp: returnsundefinedas before — the guard logic is unchanged, just moved to the class method.
Module-Level Changes
| File | Change |
|---|---|
src/tool-preview-formatter.ts |
New — ToolPreviewFormatter class + ToolPreviewFormatterOptions interface |
src/tool-input-preview.ts |
Remove 7 config-dependent exports (formatToolInputForPrompt, formatJsonInputForPrompt, formatSearchInputForPrompt, sanitizeInlineText, formatGenericToolInputForLog, getToolInputPreviewForLog, getPermissionLogContext); keep 8 pure utilities + 3 constants |
src/permission-prompts.ts |
Replace import { formatToolInputForPrompt } with import type { ToolPreviewFormatter }; add formatter?: ToolPreviewFormatter parameter to formatAskPrompt |
src/handlers/gates/tool.ts |
Replace import { getPermissionLogContext } with import type { ToolPreviewFormatter }; add formatter: ToolPreviewFormatter parameter to describeToolGate |
src/handlers/permission-gate-handler.ts |
Import ToolPreviewFormatter and constants; construct formatter in handleToolCall with default values; pass to tool gate producer |
test/tool-preview-formatter.test.ts |
New — unit tests for ToolPreviewFormatter methods |
test/tool-input-preview.test.ts |
Remove imports of the 7 moved functions; remove their test blocks; keep constant and pure-utility tests |
test/permission-prompts.test.ts |
Remove vi.mock("../src/tool-input-preview.js"); import ToolPreviewFormatter; construct real instance or spy; pass as parameter |
test/handlers/gates/tool.test.ts |
Import ToolPreviewFormatter; construct a formatter with defaults; pass to describeToolGate in each call |
docs/architecture/architecture.md |
Update module listing at line ~527: add tool-preview-formatter.ts, update tool-input-preview.ts description |
No symbol documented in .pi/skills/package-pi-permission-system/SKILL.md is removed — no skill update needed.
No barrel (src/index.ts) export changes — the formatter is an internal collaborator, not a public API.
Test Impact Analysis
-
New unit tests enabled (
test/tool-preview-formatter.test.ts):- Constructor stores options correctly.
formatToolInputForPromptdispatches tool names correctly (edit/write/read/find/grep/ls/fallback).formatJsonInputForPrompttruncates at the configuredtoolInputPreviewMaxLength.sanitizeInlineTextrespects constructor default and explicit override.formatSearchInputForPromptcallssanitizeInlineTextwith config-dependent truncation.formatGenericToolInputForLogtruncates attoolInputLogPreviewMaxLength.getToolInputPreviewForLogreturns undefined for bash/mcp, path preview for path-bearing tools, JSON preview for others — all with config-dependent truncation.getPermissionLogContextreturns the correct shape with config-dependent preview. These tests were previously impossible without mocking because the constants were module-level.
-
Tests that become redundant: The
formatToolInputForPrompttests intool-input-preview.test.ts(thedescribe("formatToolInputForPrompt")block) become redundant with the new class-level tests that cover the same dispatch logic. They should be removed as part of the extraction to avoid duplication. -
Tests that stay as-is:
- The constant-value tests in
tool-input-preview.test.ts(thedescribe("constants")block) stay — the constants remain as default values. - The
describeToolGateintegration tests intool.test.tsstay — they exercise the gate descriptor layer that the formatter is threaded through, and pass the formatter as a construction dependency. - The
formatAskPromptbehavioral tests inpermission-prompts.test.tsstay — they exercise the prompt message logic that is unchanged. - The end-to-end handler tests in
tool-call.test.tsandtool-call-events.test.tsstay — they exercise the full pipeline with the formatter injected.
- The constant-value tests in
TDD Order
-
test:AddToolPreviewFormattertests covering all 7 config-dependent methods. New filetest/tool-preview-formatter.test.tswith constructor options, dispatch logic, config-dependent truncation at each of the three limits, and log formatting. Commit:test: add ToolPreviewFormatter tests -
refactor:Createsrc/tool-preview-formatter.tswith the class; remove the 7 config-dependent exports fromtool-input-preview.ts. Updatetest/tool-input-preview.test.ts— removeformatToolInputForPromptdescribe block and the 6 other moved-function imports/test blocks; keep constant and pure-utility tests. The class is not yet used by any consumer — this step is purely the extraction. Commit:refactor: extract ToolPreviewFormatter class from tool-input-preview -
test:Updatetest/handlers/gates/tool.test.ts— importToolPreviewFormatter, construct with defaults, pass as third argument todescribeToolGate. The test file is the first consumer; this step proves the type signature works before modifying production code. Commit:test: update tool gate tests with formatter parameter -
refactor:Thread the formatter through the gate descriptor chain.src/handlers/gates/tool.ts: addformatterparameter todescribeToolGate, replacegetPermissionLogContext(...)import withformatter.getPermissionLogContext(...), pass formatter toformatAskPrompt.src/permission-prompts.ts: addformatter?: ToolPreviewFormatterparameter toformatAskPrompt, useformatter.formatToolInputForPrompt(...)when present.src/handlers/permission-gate-handler.ts: importToolPreviewFormatterand the three constants; construct the formatter insidehandleToolCallwith default values; pass it to the tool gate producer thunk. All existing handler and gate tests remain green. Commit:refactor: thread ToolPreviewFormatter through gate descriptor chain
-
test:Updatetest/permission-prompts.test.ts— removevi.mock("../src/tool-input-preview.js")and theimport { formatToolInputForPrompt }/vi.mocked(formatToolInputForPrompt)lines. ImportToolPreviewFormatter, construct a real instance with defaults, and pass it as the 4th argument toformatAskPrompt. The mock-based assertions (mockedFormatToolInput.mockReturnValue/toHaveBeenCalledWith) become direct assertions on the formatter instance or on the resulting string. Commit:test: replace vi.mock with direct formatter injection in permission-prompts tests -
docs:Updatedocs/architecture/architecture.md— addtool-preview-formatter.tsto the module listing undersrc/; updatetool-input-preview.tsdescription from "Loggable context from tool inputs" to "Pure tool-input text utilities (truncation, counting, path extraction) + default constants". Mark Phase 1 Steps 1 and 2 as completed in the roadmap steps section. Commit:docs: mark Phase 1 steps 1-2 complete in permission-system architecture
Steps 3 and 4 follow the lift-and-shift pattern: introduce the class (step 2), then consume it in tests (step 3), then wire it in production (step 4). This keeps each commit individually reviewable and the type checker satisfied at each boundary.
Risks and Mitigations
- Risk: Forgetting to thread the formatter to all call sites of the moved functions.
Mitigation: Move the 7 exports from
tool-input-preview.ts, so TypeScript immediately flags any remaining import at compile time. Only two modules import moved functions:tool.tsandpermission-prompts.ts, both updated in step 4. - Risk: Backward compatibility — a third-party extension imports these functions.
Mitigation: These are internal symbols with no barrel export from
src/index.ts. No external consumer exists. - Risk:
formatAskPromptcalled without a formatter produces different behavior (empty input preview instead of the previously guaranteed preview). Mitigation:formatAskPromptis only called fromdescribeToolGate(updated) and from tests (updated). The optional parameter ensures TypeScript does not force a change on hypothetical future callers, and the empty-string fallback is the safe default (no preview is better than a wrong preview). - Risk: Module-level
vi.mockremoval inpermission-prompts.test.tsaccidentally changes test semantics. Mitigation: The mock currently returns"mocked preview"unconditionally. The injected real formatter will return real previews — tests that check the string content need adjustment to match actual formatting output rather than the constant mock value. This is surfaced explicitly in step 5's scope.
Open Questions
- Whether
TOOL_INPUT_LOG_PREVIEW_MAX_LENGTHshould also become configurable in #266. It is included inToolPreviewFormatterOptionsfor consistency since the log-formatting methods that use it live on the class. If #266 decides not to expose it inPermissionSystemExtensionConfig, the field defaults to 1000 and remains internal. - Whether the constructor should also be used as a factory for a future
extension seam (
register()for custom tool formatters) — deferred to #283.