feat: error-interception-middleware (3/3) - #1128
Conversation
📝 WalkthroughWalkthroughAdds a structured error-interception system for classifying tool failures, generating bounded recovery guidance, tracking task-scoped occurrences, breaking repeated shell-error loops, and integrating structured results into assistant messages. ChangesError interception and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/assistant-message/__tests__/presentAssistantMessage-handleError.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/assistant-message/__tests__/structuredError.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/core/assistant-message/presentAssistantMessage.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
Patch coverage checks were blocking 10+ PRs with 80%/70% thresholds. Changed to informational: true so patch coverage is reported but not a required status check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (17)
src/core/tools/error-interception/types.ts (1)
83-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the local
ToolResponseinterface.This interface has the same name as the public tool contract mentioned in the header comment on Line 5.
ToolErrorInterceptor.tsalso works with aToolResponsetype. The local interface has an index signature, so a wrong import resolves silently instead of failing the type check. A distinct name such asStructuredResultLikeremoves the ambiguity.♻️ Proposed rename
-export interface ToolResponse { +export interface StructuredResultLike { type?: string status?: string error?: unknown text?: string toolUseId?: string [key: string]: unknown }Update the field on Line 73 accordingly:
- result?: ToolResponse + result?: StructuredResultLike🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/types.ts` around lines 83 - 94, Rename the local ToolResponse interface to a distinct name such as StructuredResultLike, update the field referenced near line 73 and all local usages accordingly, and preserve its existing structure and behavior.src/core/tools/error-interception/errorPatterns.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
hasMetadatahelper.No pattern in this file calls
hasMetadata. The guidelines require lint-clean new TypeScript code, and the ESLint command runs with--max-warnings=0.♻️ Proposed fix
-const hasMetadata = (signal: InterceptionSignal, key: string): boolean => signal.metadata[key] !== undefined -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/errorPatterns.ts` at line 7, Remove the unused hasMetadata helper from errorPatterns.ts, leaving the InterceptionSignal import or related code unchanged unless it becomes unused as a direct result.Source: Coding guidelines
src/core/tools/error-interception/__tests__/ErrorClassifier.spec.ts (1)
898-931: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the two remaining pattern IDs.
The registry test checks category presence, so
EI/INVALID_TOOL_PROTOCOL/002andEI/PARAM_TYPE_MISMATCH/003are satisfied by their sibling patterns. No test drives thexmlNativeDualProtocol/xmlMarkupInTextBlockmetadata or theNESTED_PARAM_OVERFLOWvariant thatStructuralValidatoremits. Add two cases so a matcher regression in those branches fails a test.💚 Suggested additional cases
it("classifies xmlNativeDualProtocol as INVALID_TOOL_PROTOCOL/002", () => { const signal = baseSignal({ source: "parser", stage: "parse", metadata: { xmlNativeDualProtocol: true }, }) const result = classifyError(signal) expect(result.patternId).toBe("EI/INVALID_TOOL_PROTOCOL/002") }) it("classifies NESTED_PARAM_OVERFLOW as PARAM_TYPE_MISMATCH/003", () => { const signal = baseSignal({ source: "validation", stage: "preflight", metadata: { variant: "NESTED_PARAM_OVERFLOW" }, }) const result = classifyError(signal) expect(result.patternId).toBe("EI/PARAM_TYPE_MISMATCH/003") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/ErrorClassifier.spec.ts` around lines 898 - 931, Add focused classifier tests near the existing pattern registry tests for the metadata branches in classifyError: verify a parser signal with xmlNativeDualProtocol selects EI/INVALID_TOOL_PROTOCOL/002, and a validation preflight signal with variant NESTED_PARAM_OVERFLOW selects EI/PARAM_TYPE_MISMATCH/003. Reuse the existing baseSignal helper and assert each result.patternId.src/core/tools/error-interception/ErrorClassifier.ts (1)
240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the UNCLASSIFIED fallback by category, not by array position.
ERROR_PATTERNS[ERROR_PATTERNS.length - 1]treats the last registry entry as the catch-all, so appending another pattern can change the fallback silently. PreferERROR_PATTERNS.find((pattern) => pattern.category === "UNCLASSIFIED")and throw when no catch-all exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/ErrorClassifier.ts` around lines 240 - 248, The UNCLASSIFIED fallback currently depends on the final ERROR_PATTERNS entry. Update the catch-all logic to find the pattern whose category is "UNCLASSIFIED", and throw an error if no such pattern exists before constructing the fallback result.src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts (2)
296-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to the exported constant.
The title says
SHELL_INTEGRATION_THRESHOLD. The exported constant isSHELL_CIRCUIT_THRESHOLD.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts` at line 296, Rename the test case title in the circuit-opening test to use the exported constant name SHELL_CIRCUIT_THRESHOLD instead of SHELL_INTEGRATION_THRESHOLD, without changing the test behavior.
646-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused mock parameters and document the double assertion.
The mock body ignores both declared parameters, which
@typescript-eslint/no-unused-varsreports in new code. The double assertion also needs a reason comment, unlike the documented ones at lines 611-614. A typed variadic mock keeps the two-argument call at line 658 valid without either problem.♻️ Proposed refactor
- const rawPushToolResult = vi.fn( - (content: string, feedbackImages?: string[]) => {}, - ) as unknown as MockPushToolResult + // Double assertion: PushToolResult does not declare the MCP + // feedbackImages second argument, so the extra arg is modelled as + // variadic here to assert verbatim forwarding. + const rawPushToolResult = vi.fn((..._args: unknown[]) => {}) as unknown as MockPushToolResultAs per coding guidelines: "Fix lint violations in new JavaScript and TypeScript code instead of suppressing them" and "If a lint suppression is unavoidable, document the reason in a comment next to the cast or suppression."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts` around lines 646 - 648, Update the rawPushToolResult mock to use a typed variadic signature so its two-argument invocation remains valid without declaring unused parameters. Add an adjacent comment explaining why the double assertion to MockPushToolResult is necessary, matching the documented cast pattern used nearby.Source: Coding guidelines
src/core/tools/error-interception/ToolErrorInterceptor.ts (1)
209-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
rawPushToolResultcast into one local alias.The same cast appears four times (lines 215, 237, 255, 265). One local binding keeps the variadic forwarding behavior and removes the duplication.
♻️ Proposed refactor
const decoratedPushToolResult: PushToolResult = (content: ToolResponse, ...rest: unknown[]) => { + // PushToolResult callers may pass extra args (e.g. MCP feedbackImages) + // that the shared type does not declare; forward them verbatim. + const forward = rawPushToolResult as (content: ToolResponse, ...rest: unknown[]) => void // If the content is not a plain error string/structured result, pass // it through unchanged. This preserves image results, success text, // and tool-specific formatted payloads. Forward any extra args (e.g. // MCP branch feedbackImages) verbatim. if (!this.isErrorResult(content)) { - ;(rawPushToolResult as (content: ToolResponse, ...rest: unknown[]) => void)(content, ...rest) + forward(content, ...rest) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/ToolErrorInterceptor.ts` around lines 209 - 266, In the function containing decoratedPushToolResult, define one local alias for rawPushToolResult with the existing variadic PushToolResult-compatible cast, then use that alias for all four result-forwarding calls. Preserve the current content and ...rest forwarding behavior while removing the repeated casts.src/core/assistant-message/__tests__/presentAssistantMessage-handleError.spec.ts (1)
29-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the remaining
TelemetryServicemethods to the mock.
presentAssistantMessagealso callsTelemetryService.instance.captureEvent(line 581) andcaptureException(line 671). Neither path runs with the currentexecute_commandfixture, so the tests pass. If the fixture later usesread_fileor trips the repetition detector, the test fails with aTypeErrorinstead of a useful assertion.♻️ Proposed refactor
vi.mock("`@roo-code/telemetry`", () => ({ TelemetryService: { instance: { captureToolUsage: vi.fn(), captureConsecutiveMistakeError: vi.fn(), + captureEvent: vi.fn(), + captureException: vi.fn(), }, }, }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/__tests__/presentAssistantMessage-handleError.spec.ts` around lines 29 - 36, Extend the TelemetryService.instance mock in presentAssistantMessage-handleError.spec.ts with captureEvent and captureException mock methods, alongside the existing captureToolUsage and captureConsecutiveMistakeError methods, so all telemetry paths used by presentAssistantMessage remain callable.src/core/tools/error-interception/TaskErrorState.ts (3)
164-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the invalid-key path.
For an invalid key,
getTaskErrorStatereturns a fresh instance on every call. Occurrence counters never accumulate, so every failure looks like occurrence 1 and escalation and circuit breaking stop working for that caller. The failure is silent.Adding a
console.warnin the guard branch surfaces the caller mistake without changing the fail-open behavior.♻️ Proposed refactor
export function getTaskErrorState(task: object): TaskErrorState { if (!isWeakMapKey(task)) { + console.warn( + `[TaskErrorState] Non-object task key (${typeof task}); returning ephemeral state. Occurrence counters will not persist.`, + ) return new TaskErrorState() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/TaskErrorState.ts` around lines 164 - 174, Update the invalid-key guard in getTaskErrorState to emit a console.warn before returning the fresh TaskErrorState, including enough context to identify the invalid caller input. Preserve the existing fail-open return behavior and leave the WeakMap-backed state path unchanged.
28-51: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider typing the category key as
ErrorCategory.
perCategoryis keyed bystring, and every public method acceptscategory: string. The consumerToolErrorInterceptor.resetTaskStatepasses anErrorCategory. With astringkey, a misspelled category name compiles and silently creates a separate counter and a separate circuit, so a reset can miss the state it intends to clear.Importing
ErrorCategoryfrom./typesand using it for the key and the parameters restores that check. The tests atsrc/core/tools/error-interception/__tests__/TaskErrorState.spec.tslines 81-84 and 114-115 use arbitrary keys such as"A"and"B", so they need real category names after this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/TaskErrorState.ts` around lines 28 - 51, Type TaskErrorState category keys and parameters with ErrorCategory by importing it from ./types, including perCategory and all public methods, so invalid category names are rejected at compile time. Update the TaskErrorState tests to replace arbitrary keys such as "A" and "B" with valid ErrorCategory values.
66-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWire
isOpenand fingerprint changes into recovery decisions.
recordErrorOccurrenceincrements counts inTaskErrorState, but no production code readsisOpen,getFingerprint, orsetFingerprint. Keep only the behavior that callers use, or add the missing recovery path that usesisOpen/returned occurrences. The documented “explicit fingerprint change resets only the affected category” also has no implementation or caller comparison, so either implement it or remove that contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/TaskErrorState.ts` around lines 66 - 99, Update the recovery flow around recordErrorOccurrence to consume TaskErrorState.incrementOccurrence/isOpen and stop exposing unused getFingerprint/setFingerprint behavior, or implement callers that compare fingerprints and reset only the affected category as documented. Ensure the chosen implementation makes circuit-opening and explicit fingerprint-change recovery decisions effective in production rather than leaving these APIs unused.src/core/tools/error-interception/MessageTransformer.ts (2)
80-82: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse one
TextEncoderinstance.
countUtf8Bytesallocates a newTextEncoderon every call.fitDetailsWithinByteLimitandtruncateStringcall it many times per transformation. A module-level instance removes the repeated allocation.♻️ Proposed refactor
+const UTF8_ENCODER = new TextEncoder() + function countUtf8Bytes(text: string): number { - return new TextEncoder().encode(text).length + return UTF8_ENCODER.encode(text).length }Then reuse it in the exported helper:
export function encodeUtf8Bytes(text: string): Uint8Array { - return new TextEncoder().encode(text) + return UTF8_ENCODER.encode(text) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/MessageTransformer.ts` around lines 80 - 82, Update countUtf8Bytes to reuse a single module-level TextEncoder instance instead of constructing one on every call. Define the encoder once near the helper and have countUtf8Bytes use it, preserving the existing behavior for fitDetailsWithinByteLimit and truncateString.
170-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the pattern once per transformation.
buildPayloadresolves the samepatternIdtwice: once inselectOccurrenceTemplateand once inselectRecoveryDisposition. Each call runs a linearERROR_PATTERNS.find. Line 172 also callsresolveTemplate(patternId), which repeats the lookup that just failed and returns the constant fallback.Consider indexing the patterns by id and passing the resolved pattern into both selectors.
♻️ Proposed refactor
-function resolvePattern(patternId: string) { - return ERROR_PATTERNS.find((p) => p.id === patternId) -} +const PATTERNS_BY_ID = new Map(ERROR_PATTERNS.map((p) => [p.id, p])) + +function resolvePattern(patternId: string) { + return PATTERNS_BY_ID.get(patternId) +}function selectOccurrenceTemplate(patternId: string, occurrence: number): PatternTemplate { const pattern = resolvePattern(patternId) - if (!pattern) return resolveTemplate(patternId) + if (!pattern) return FALLBACK_TEMPLATE🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/MessageTransformer.ts` around lines 170 - 183, Refactor the transformation flow so buildPayload resolves each patternId only once, preferably through an id-indexed lookup, then passes the resolved pattern to selectOccurrenceTemplate and selectRecoveryDisposition. Update selectOccurrenceTemplate to use the supplied pattern and retain the constant fallback when no pattern exists, removing its repeated resolvePattern and redundant resolveTemplate lookup while preserving existing occurrence behavior.src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts (3)
504-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe first-continuation assertion is conditional, so it cannot fail.
The test name states that truncation preserves the first continuation action. The
if (nextSection)guard makes that check optional. If truncation drops everyNextitem, the block is skipped and the test passes. The named property is not enforced.Assert that a
Nextsection is present, then assert its first item.💚 Proposed fix
- // First Next item (continuation action) must be preserved if any Next exists const nextSection = message.match(/Next:\n(\d+\..+)/) - if (nextSection) { - expect(nextSection[1].length).toBeGreaterThan(0) - } + // The first Next item is the continuation action and must survive truncation. + expect(nextSection).not.toBeNull() + expect(nextSection![1].length).toBeGreaterThan(0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts` around lines 504 - 508, Update the continuation assertion in the truncation test to require that the message contains a Next section before inspecting it. Remove the conditional guard around nextSection, assert the match result is present, then retain the first-item length assertion so the test fails when truncation removes every Next item.
605-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the bare-character absence assertions.
Several tests assert that a single character is absent from the entire rendered message, not from the injected region:
- Line 616:
not.toContain("'")- Line 771:
not.toContain("()")- Line 786:
not.toContain("|")- Line 801:
not.toContain(";")- Line 816:
not.toContain("")`These pass only because the current template prose in
errorPatterns.tshappens to contain none of those characters. If a future template adds an apostrophe or a semicolon to unrelated guidance text, these tests fail even though the sanitizer still works correctly. The sibling assertion in each test already covers the real property, for example line 800 assertsnot.toContain("rm -rf").Assert the absence of the malicious payload rather than the absence of a character. For the case at lines 605-618, assert that no quoted parameter name is rendered.
💚 Proposed fix for the absent-parameterName case
expect(message).toContain("Category: PARAM_MISSING") - expect(message).not.toContain("'") + // No parameter name is quoted into the guidance when the fact is absent. + expect(message).not.toMatch(/'[^']*'/) expect(message.toLowerCase()).toContain("required parameter")Line 587 also repeats the assertion on line 585 verbatim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts` around lines 605 - 618, Update the affected tests in MessageTransformer.spec.ts to assert that the complete malicious payload is absent from the rendered message, rather than checking for bare characters such as apostrophes, parentheses, pipes, semicolons, or backticks. In the absent-parameterName test, verify that no quoted parameter name is rendered while retaining the existing required-parameter guidance assertion; remove the duplicate assertion around lines 585–587.
155-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two tests do not cover surrogate pairs or multibyte truncation.
The test at lines 155-166 is named for non-ASCII characters and surrogate pairs, but the classification carries only ASCII facts and the template text in
ERROR_PATTERNSis ASCII. No surrogate pair reaches the transformer. The test would still pass if the surrogate handling atsrc/core/tools/error-interception/MessageTransformer.tsline 343 and line 92 were removed.The assertion at line 183 compares
getPayloadByteLength(multibyte)againstnew TextEncoder().encode(multibyte).length.getPayloadByteLengthcalls the sameTextEncoder, so the assertion restates the implementation and cannot fail.
formatErrorDetailsaccepts arbitrarywhat,why, andnextvalues. Use it to drive multibyte text through the real path, and assert a literal expected byte count.💚 Proposed test additions
import { formatErrorDetails } from "../MessageTransformer" it("computes UTF-8 byte length for multibyte text and surrogate pairs", () => { // "한글" = 6 bytes, "🚀" = 4 bytes expect(getPayloadByteLength("한글")).toBe(6) expect(getPayloadByteLength("🚀")).toBe(4) }) it("never emits a lone surrogate when truncating multibyte content", () => { const details = formatErrorDetails( "UNCLASSIFIED", "guided_tool_error", "🚀".repeat(200), "한글테스트🚀".repeat(50), ["🚀".repeat(200)], true, 1, "EI/UNCLASSIFIED/001", ) // No unpaired surrogate code unit survives. expect(/[\ud800-\udbff](?![\udc00-\udfff])|(?<![\ud800-\udbff])[\udc00-\udfff]/.test(details)).toBe(false) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts` around lines 155 - 184, Strengthen the MessageTransformer tests by importing and using formatErrorDetails with actual Korean text and 🚀 surrogate pairs, rather than relying on ASCII classification data. Replace the self-referential encoder assertion with literal UTF-8 byte expectations for “한글” and “🚀”, and add a truncation assertion that formatErrorDetails never emits unpaired surrogate code units while exercising multibyte content.src/core/tools/error-interception/__tests__/TaskErrorState.spec.ts (1)
88-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the pending native protocol guide.
Four public methods on
TaskErrorStatehave no test:setPendingNativeProtocolGuide,getPendingNativeProtocolGuide,clearPendingNativeProtocolGuide, andconsumePendingNativeProtocolGuide. The read-and-clear behavior atsrc/core/tools/error-interception/TaskErrorState.tslines 132-136 is the documented mechanism that stops guidance from leaking into a later turn, and it is unverified.The interaction with
reset()is also unverified. See the related comment onsrc/core/tools/error-interception/TaskErrorState.tslines 105-136.💚 Proposed test additions
describe("pending native protocol guide", () => { it("returns undefined when no guide is queued", () => { const state = new TaskErrorState() expect(state.getPendingNativeProtocolGuide()).toBeUndefined() expect(state.consumePendingNativeProtocolGuide()).toBeUndefined() }) it("returns the guide without clearing it", () => { const state = new TaskErrorState() state.setPendingNativeProtocolGuide("guide") expect(state.getPendingNativeProtocolGuide()).toBe("guide") expect(state.getPendingNativeProtocolGuide()).toBe("guide") }) it("consume returns the guide once and clears it", () => { const state = new TaskErrorState() state.setPendingNativeProtocolGuide("guide") expect(state.consumePendingNativeProtocolGuide()).toBe("guide") expect(state.consumePendingNativeProtocolGuide()).toBeUndefined() }) it("clear removes a queued guide", () => { const state = new TaskErrorState() state.setPendingNativeProtocolGuide("guide") state.clearPendingNativeProtocolGuide() expect(state.getPendingNativeProtocolGuide()).toBeUndefined() }) it("reset() does not leave a queued guide for a later turn", () => { const state = new TaskErrorState() state.setPendingNativeProtocolGuide("guide") state.reset() expect(state.consumePendingNativeProtocolGuide()).toBeUndefined() }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/error-interception/__tests__/TaskErrorState.spec.ts` around lines 88 - 121, Add a “pending native protocol guide” test suite in TaskErrorState.spec.ts covering empty reads, non-clearing getPendingNativeProtocolGuide, one-time consumePendingNativeProtocolGuide behavior, clearPendingNativeProtocolGuide, and ensuring reset() removes any queued guide. Use the existing TaskErrorState methods and verify the guide cannot leak after reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/assistant-message/presentAssistantMessage.ts`:
- Line 137: The pushToolResult closures at
src/core/assistant-message/presentAssistantMessage.ts lines 137-137 and 458-458
incorrectly use a positional isError parameter that can receive image payloads;
replace this callback contract with an options object or separate error helper,
and update all MCP result and approval-feedback call sites so image data cannot
set is_error: true.
In `@src/core/assistant-message/structuredError.ts`:
- Around line 121-171: Update formatStructuredError to measure the serialized
payload in UTF-8 bytes rather than comparing json.length against byteLimit.
Reuse the existing encodeUtf8Bytes or getPayloadByteLength helpers from
MessageTransformer for all three size checks, preserving the current trimming
and fallback behavior and the byteLimit API semantics.
- Around line 99-107: The recordErrorOccurrence path must stop storing
structured-error signatures in TaskErrorState.perCategory, because category
resets do not clear them. Add or reuse a separate per-task signature-occurrence
namespace, update recordErrorOccurrence and its TaskErrorState accessors
accordingly, and enforce a bounded maximum number of retained signature keys per
task while preserving occurrence counting and resetTaskState category behavior.
In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts`:
- Around line 94-96: Update the assertion around the nextSection match in the
MessageTransformer test to explicitly reject null, using an assertion
appropriate for String.prototype.match results before accessing nextSection[1].
Preserve the existing validation of the captured section and avoid relying on
the non-null assertion to detect a missing match.
In `@src/core/tools/error-interception/__tests__/StructuralValidator.spec.ts`:
- Around line 101-109: Update the fixtures in validateNestedParams tests: change
the first case to use two entries from KNOWN_PARAMETER_KEYS that do not form any
TOOL_SIGNATURE_KEY_SETS signature, so it reaches the multi-known-keys heuristic;
change the second case to use exactly one KNOWN_PARAMETER_KEYS entry while
keeping it a non-signature tool.
In `@src/core/tools/error-interception/errorPatterns.ts`:
- Line 1: Update the type-only import in errorPatterns.ts to reference the
sibling types module without the .ts extension, keeping the imported
ErrorPattern, InterceptionSignal, and RecoveryDisposition symbols unchanged.
In `@src/core/tools/error-interception/index.ts`:
- Around line 1-19: Remove the .ts extensions from the relative type export
specifiers in the error-interception index module, and update the matching
relative import in errorPatterns.ts to be extensionless. Preserve the existing
exported symbols and import targets.
In `@src/core/tools/error-interception/MessageTransformer.ts`:
- Around line 114-124: Update the fallback object returned by resolveTemplate
when resolvePattern cannot find patternId so its what and why text accurately
describe an unrecognized error with no known matching pattern; leave the
resolved-pattern path unchanged.
- Around line 360-391: Update fitDetailsWithinByteLimit so the nextCount loop
removes trailing Next items before applying content truncation, with the Why and
What ladders moved outside that loop and each candidate derived from the
original payload fields. Preserve the documented priority by retaining
substantive What content whenever dropping a trailing Next item is sufficient,
and add a test in MessageTransformer.spec.ts covering that behavior.
In `@src/core/tools/error-interception/StructuralValidator.ts`:
- Around line 111-124: Update buildSignal and the structural-validator flow to
accept and pass the actual taskId through validateCwdParameter and
validateNestedParams before classification. Remove the hardcoded empty taskId so
each validation signal carries its originating task identifier and cannot be
associated with an empty per-task key.
In `@src/core/tools/error-interception/TaskErrorState.ts`:
- Around line 105-136: Update TaskErrorState.reset so a full reset with no
category also clears pendingGuide, while preserving category-specific reset
behavior. Add a guardrail test that queues a guide, calls reset(), and verifies
consumePendingNativeProtocolGuide() returns undefined.
In `@src/core/tools/error-interception/ToolErrorInterceptor.ts`:
- Around line 26-31: Update the decoratedHandleError documentation in
ToolErrorInterceptor to state that the transformed result is pushed through
rawPushToolResult before the original rawHandleError callback is invoked.
Preserve the intentional ordering described by the inline comment and do not
alter the implementation.
- Around line 329-334: Update the error-marker checks in the interceptor’s
string-matching method to compare a lowercased copy of trimmed, so “No
sufficiently similar match found…” is detected regardless of capitalization and
reaches transformSignal. Reuse that normalized value to consolidate the
ERROR/error: prefix checks into one case-insensitive comparison while preserving
the existing markers and behavior.
---
Nitpick comments:
In
`@src/core/assistant-message/__tests__/presentAssistantMessage-handleError.spec.ts`:
- Around line 29-36: Extend the TelemetryService.instance mock in
presentAssistantMessage-handleError.spec.ts with captureEvent and
captureException mock methods, alongside the existing captureToolUsage and
captureConsecutiveMistakeError methods, so all telemetry paths used by
presentAssistantMessage remain callable.
In `@src/core/tools/error-interception/__tests__/ErrorClassifier.spec.ts`:
- Around line 898-931: Add focused classifier tests near the existing pattern
registry tests for the metadata branches in classifyError: verify a parser
signal with xmlNativeDualProtocol selects EI/INVALID_TOOL_PROTOCOL/002, and a
validation preflight signal with variant NESTED_PARAM_OVERFLOW selects
EI/PARAM_TYPE_MISMATCH/003. Reuse the existing baseSignal helper and assert each
result.patternId.
In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts`:
- Around line 504-508: Update the continuation assertion in the truncation test
to require that the message contains a Next section before inspecting it. Remove
the conditional guard around nextSection, assert the match result is present,
then retain the first-item length assertion so the test fails when truncation
removes every Next item.
- Around line 605-618: Update the affected tests in MessageTransformer.spec.ts
to assert that the complete malicious payload is absent from the rendered
message, rather than checking for bare characters such as apostrophes,
parentheses, pipes, semicolons, or backticks. In the absent-parameterName test,
verify that no quoted parameter name is rendered while retaining the existing
required-parameter guidance assertion; remove the duplicate assertion around
lines 585–587.
- Around line 155-184: Strengthen the MessageTransformer tests by importing and
using formatErrorDetails with actual Korean text and 🚀 surrogate pairs, rather
than relying on ASCII classification data. Replace the self-referential encoder
assertion with literal UTF-8 byte expectations for “한글” and “🚀”, and add a
truncation assertion that formatErrorDetails never emits unpaired surrogate code
units while exercising multibyte content.
In `@src/core/tools/error-interception/__tests__/TaskErrorState.spec.ts`:
- Around line 88-121: Add a “pending native protocol guide” test suite in
TaskErrorState.spec.ts covering empty reads, non-clearing
getPendingNativeProtocolGuide, one-time consumePendingNativeProtocolGuide
behavior, clearPendingNativeProtocolGuide, and ensuring reset() removes any
queued guide. Use the existing TaskErrorState methods and verify the guide
cannot leak after reset.
In `@src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts`:
- Line 296: Rename the test case title in the circuit-opening test to use the
exported constant name SHELL_CIRCUIT_THRESHOLD instead of
SHELL_INTEGRATION_THRESHOLD, without changing the test behavior.
- Around line 646-648: Update the rawPushToolResult mock to use a typed variadic
signature so its two-argument invocation remains valid without declaring unused
parameters. Add an adjacent comment explaining why the double assertion to
MockPushToolResult is necessary, matching the documented cast pattern used
nearby.
In `@src/core/tools/error-interception/ErrorClassifier.ts`:
- Around line 240-248: The UNCLASSIFIED fallback currently depends on the final
ERROR_PATTERNS entry. Update the catch-all logic to find the pattern whose
category is "UNCLASSIFIED", and throw an error if no such pattern exists before
constructing the fallback result.
In `@src/core/tools/error-interception/errorPatterns.ts`:
- Line 7: Remove the unused hasMetadata helper from errorPatterns.ts, leaving
the InterceptionSignal import or related code unchanged unless it becomes unused
as a direct result.
In `@src/core/tools/error-interception/MessageTransformer.ts`:
- Around line 80-82: Update countUtf8Bytes to reuse a single module-level
TextEncoder instance instead of constructing one on every call. Define the
encoder once near the helper and have countUtf8Bytes use it, preserving the
existing behavior for fitDetailsWithinByteLimit and truncateString.
- Around line 170-183: Refactor the transformation flow so buildPayload resolves
each patternId only once, preferably through an id-indexed lookup, then passes
the resolved pattern to selectOccurrenceTemplate and selectRecoveryDisposition.
Update selectOccurrenceTemplate to use the supplied pattern and retain the
constant fallback when no pattern exists, removing its repeated resolvePattern
and redundant resolveTemplate lookup while preserving existing occurrence
behavior.
In `@src/core/tools/error-interception/TaskErrorState.ts`:
- Around line 164-174: Update the invalid-key guard in getTaskErrorState to emit
a console.warn before returning the fresh TaskErrorState, including enough
context to identify the invalid caller input. Preserve the existing fail-open
return behavior and leave the WeakMap-backed state path unchanged.
- Around line 28-51: Type TaskErrorState category keys and parameters with
ErrorCategory by importing it from ./types, including perCategory and all public
methods, so invalid category names are rejected at compile time. Update the
TaskErrorState tests to replace arbitrary keys such as "A" and "B" with valid
ErrorCategory values.
- Around line 66-99: Update the recovery flow around recordErrorOccurrence to
consume TaskErrorState.incrementOccurrence/isOpen and stop exposing unused
getFingerprint/setFingerprint behavior, or implement callers that compare
fingerprints and reset only the affected category as documented. Ensure the
chosen implementation makes circuit-opening and explicit fingerprint-change
recovery decisions effective in production rather than leaving these APIs
unused.
In `@src/core/tools/error-interception/ToolErrorInterceptor.ts`:
- Around line 209-266: In the function containing decoratedPushToolResult,
define one local alias for rawPushToolResult with the existing variadic
PushToolResult-compatible cast, then use that alias for all four
result-forwarding calls. Preserve the current content and ...rest forwarding
behavior while removing the repeated casts.
In `@src/core/tools/error-interception/types.ts`:
- Around line 83-94: Rename the local ToolResponse interface to a distinct name
such as StructuredResultLike, update the field referenced near line 73 and all
local usages accordingly, and preserve its existing structure and behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e265ce7e-a19b-4500-8ca4-f9b8e645a8c8
📒 Files selected for processing (17)
src/core/assistant-message/__tests__/presentAssistantMessage-handleError.spec.tssrc/core/assistant-message/__tests__/structuredError.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/assistant-message/structuredError.tssrc/core/tools/error-interception/ErrorClassifier.tssrc/core/tools/error-interception/MessageTransformer.tssrc/core/tools/error-interception/StructuralValidator.tssrc/core/tools/error-interception/TaskErrorState.tssrc/core/tools/error-interception/ToolErrorInterceptor.tssrc/core/tools/error-interception/__tests__/ErrorClassifier.spec.tssrc/core/tools/error-interception/__tests__/MessageTransformer.spec.tssrc/core/tools/error-interception/__tests__/StructuralValidator.spec.tssrc/core/tools/error-interception/__tests__/TaskErrorState.spec.tssrc/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.tssrc/core/tools/error-interception/errorPatterns.tssrc/core/tools/error-interception/index.tssrc/core/tools/error-interception/types.ts
| let approvalFeedback: { text: string; images?: string[] } | undefined | ||
|
|
||
| const pushToolResult = (content: ToolResponse, feedbackImages?: string[]) => { | ||
| const pushToolResult = (content: ToolResponse, isError: boolean = false) => { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate pushToolResult call sites that pass a second positional argument.
set -uo pipefail
echo "=== PushToolResult type declaration ==="
rg -n -C4 'PushToolResult' src/shared/tools.ts
echo "=== two-argument pushToolResult calls ==="
rg -nP --type=ts 'pushToolResult\(\s*[^)]*?,\s*[^)]+\)' src | head -60
echo "=== feedbackImages usage ==="
rg -n -C4 --type=ts 'feedbackImages' src | head -80Repository: Zoo-Code-Org/Zoo-Code
Length of output: 11717
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== presentAssistantMessage outline ==="
ast-grep outline src/core/assistant-message/presentAssistantMessage.ts --view compact || true
echo "=== relevant presentAssistantMessage sections ==="
sed -n '1,280p' src/core/assistant-message/presentAssistantMessage.ts
sed -n '430,590p' src/core/assistant-message/presentAssistantMessage.ts
echo "=== ToolErrorInterceptor relevant sections ==="
sed -n '180,250p' src/core/tools/error-interception/ToolErrorInterceptor.ts
sed -n '640,670p' src/core/tools/error-interception/ToolErrorInterceptor.spec.ts
echo "=== ToolResponse isError/is_error shaping search ==="
rg -n -C3 'is_error|isError' src | head -120Repository: Zoo-Code-Org/Zoo-Code
Length of output: 28542
Remove the positional error flag from pushToolResult. Both pushToolResult closures take a second argument that MCP result paths pass as images for approval feedback. Change the callback contract to an options object or separate error helper so is_error: true is not set by image payloads.
📍 Affects 1 file
src/core/assistant-message/presentAssistantMessage.ts#L137-L137(this comment)src/core/assistant-message/presentAssistantMessage.ts#L458-L458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/assistant-message/presentAssistantMessage.ts` at line 137, The
pushToolResult closures at src/core/assistant-message/presentAssistantMessage.ts
lines 137-137 and 458-458 incorrectly use a positional isError parameter that
can receive image payloads; replace this callback contract with an options
object or separate error helper, and update all MCP result and approval-feedback
call sites so image data cannot set is_error: true.
| /** | ||
| * Increments and returns the per-task occurrence count for an error | ||
| * signature. State is kept in the error-interception module's TaskErrorState | ||
| * WeakMap, so counters persist across tool blocks within a task and are | ||
| * released with it. Non-object keys fail open with occurrence 1. | ||
| */ | ||
| export function recordErrorOccurrence(task: object, signature: string): number { | ||
| return getTaskErrorState(task).incrementOccurrence(signature) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect TaskErrorState's counter API and reset behavior.
set -uo pipefail
f=src/core/tools/error-interception/TaskErrorState.ts
ast-grep outline "$f" --items all
echo "=== counter API ==="
rg -n -C6 'incrementOccurrence|getOccurrence|reset\(|Map<' "$f"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3232
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== locate relevant files ==="
fd -a 'TaskErrorState\.ts|ToolErrorInterceptor\.ts|structuredError\.ts|ToolErrorInterceptor\.spec\.ts' . | sed 's#^\./##'
echo "=== TaskErrorState full file ==="
cat -n src/core/tools/error-interception/TaskErrorState.ts
echo "=== ToolErrorInterceptor reset section ==="
cat -n src/core/tools/error-interception/ToolErrorInterceptor.ts | sed -n '1,220p'
echo "=== recordErrorOccurrence usages ==="
rg -n -C4 'recordErrorOccurrence|incrementOccurrence\("' src/core/tools/error-interception src/core/assistant-message/structuredError.ts
echo "=== ToolErrorInterceptor occurrence call sites in spec ==="
cat -n src/core/tools/error-interception/ToolErrorInterceptor.spec.ts | sed -n '420,500p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 33660
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== locate ErrorCategory and structuredError signature builders ==="
rg -n 'enum ErrorCategory|type ErrorCategory|ErrorCategory|buildErrorSignature|formatStructuredError' src/core src/shared -S
echo "=== structuredError relevant code ==="
cat -n src/core/assistant-message/structuredError.ts | sed -n '80,125p;170,195p'
echo "=== inspect error classifier for category keys, if any ==="
if [ -f src/core/tools/error-interception/ErrorClassifier.ts ]; then
wc -l src/core/tools/error-interception/ErrorClassifier.ts
cat -n src/core/tools/error-interception/ErrorClassifier.ts | sed -n '1,260p'
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 18248
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== ErrorCategory and classifier error types ==="
cat -n src/core/tools/error-interception/types.ts | sed -n '1,120p;440,560p'
echo "=== ToolErrorInterceptor counter usage ==="
cat -n src/core/tools/error-interception/ToolErrorInterceptor.ts | sed -n '300,350p'
echo "=== resetTaskState callers ==="
rg -n -C5 'resetTaskState\(' src/core src/shared -SRepository: Zoo-Code-Org/Zoo-Code
Length of output: 18002
🌐 Web query:
TypeScript Record<K extends PropertyKey, T> index signature string PropertyKey includes unknown
💡 Result:
In TypeScript, Record<K extends PropertyKey, T> is a utility type defined as a mapped type { [P in K]: T } [1][2][3]. The PropertyKey type is a built-in alias for string | number | symbol [4][5][6]. While Record and standard index signatures (e.g., { [key: string]: T }) are often used for similar purposes—creating dictionary-like objects—they behave differently in the TypeScript type system due to their underlying implementation [7][8]. Key technical distinctions: 1. Definition and Implementation: Record is a mapped type, whereas { [key: string]: T } is an index signature [9][8]. Mapped types iterate over a set of keys, while index signatures define an implicit contract for all possible keys of a certain type [10][7]. 2. keyof Behavior: The result of the keyof operator differs between them [11]. For a Record<string, T>, keyof returns string [7][8]. Conversely, for an object with a string index signature { [key: string]: T }, keyof returns string | number [8][11]. This is because TypeScript treats numeric index signatures and string index signatures with specific behaviors regarding how they are indexed [11]. 3. Assignability and "unknown": When using Record<string, unknown>, the type requires an explicit index signature in the source type for assignment, whereas Record<string, any> is treated as a special case that permits wider assignability [12]. This behavior is intentional; TypeScript does not apply the same permissive rules to unknown as it does to any [12]. In summary, while Record is a flexible tool for object creation, it is not always a direct substitute for a native index signature, particularly when code relies on how keyof or property inference behaves [9][8]. If you need a structure that accepts all possible keys, you can use Record<PropertyKey, T>, but be aware that the mapped type implementation will continue to reflect that specific construction [4][11].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/utility-types.html
- 2: https://blog.logrocket.com/typescript-record-types/
- 3: https://dusktools.app/typescript-type/record
- 4: https://www.totaltypescript.com/concepts/propertykey-type
- 5: https://github.com/Microsoft/TypeScript/blob/32178ac/src/lib/es6.d.ts
- 6: https://github.com/microsoft/TypeScript/blob/c88e44a9e65f7d7a72ef72c9dfae94ef0fcb6bb0/lib/lib.es5.d.ts
- 7:
keyofbehaves differently with index signatures created usingRecordmicrosoft/TypeScript#63216 - 8: Inconsistent generic type inference between record type and object literal type microsoft/TypeScript#56821
- 9: https://stackoverflow.com/questions/54100025/difference-between-index-signature-and-record-for-empty-object
- 10: https://stackoverflow.com/questions/51936369/what-is-the-record-type
- 11: https://stackoverflow.com/questions/77926787/the-weird-types-of-typescripts-keyof-for-objects
- 12: Function extends Record<string, any> but not Record<string, unknown> microsoft/TypeScript#41746
Keep structured-error signatures out of TaskErrorState.perCategory.
recordErrorOccurrence() stores signatures like structured-error|executing command|Error|boom in the same perCategory map used by ToolErrorInterceptor counters. TaskErrorState.reset(category) deletes only that key, so buildStructuredErrorContent() keys are not reset by category-specific resetTaskState(task, "SHELL_INTEGRATION") and still consume the STUCK_LOOP_THRESHOLD. Keep the two namespaces separate and cap retained signature keys per task to avoid unbounded growth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/assistant-message/structuredError.ts` around lines 99 - 107, The
recordErrorOccurrence path must stop storing structured-error signatures in
TaskErrorState.perCategory, because category resets do not clear them. Add or
reuse a separate per-task signature-occurrence namespace, update
recordErrorOccurrence and its TaskErrorState accessors accordingly, and enforce
a bounded maximum number of retained signature keys per task while preserving
occurrence counting and resetTaskState category behavior.
| export function formatStructuredError(details: StructuredErrorDetails, byteLimit: number = 8000): string { | ||
| const version = "1.0" | ||
| const status = "error" | ||
| const category = details.pattern ? (details.pattern.split("/")[1] ?? "unknown") : "unknown" | ||
| // A `type` discriminator must not contain slashes; use the dotted form of | ||
| // the pattern id (e.g. "tool_execution.error_execution.001"). | ||
| const type = details.pattern ? details.pattern.toLowerCase().replace(/\//g, ".") : "unclassified_error" | ||
| const retryable = details.retryable ?? true | ||
| const occurrence = Math.max(1, details.occurrence ?? 1) | ||
| const patternId = details.pattern ?? "UNCLASSIFIED/000/000" | ||
| const recoveryDisposition = details.disposition ?? "correct_once" | ||
|
|
||
| const payload = { | ||
| version, | ||
| status, | ||
| type, | ||
| category, | ||
| what: details.what, | ||
| why: details.why, | ||
| next: details.next, | ||
| retryable, | ||
| occurrence, | ||
| pattern_id: patternId, | ||
| recovery_disposition: recoveryDisposition, | ||
| } | ||
|
|
||
| let json = JSON.stringify(payload, null, 2) | ||
|
|
||
| if (json.length > byteLimit && payload.next.length > 1) { | ||
| // Trim Next items to fit within byte limit, preserving the first one. | ||
| json = JSON.stringify({ ...payload, next: payload.next.slice(0, 1) }, null, 2) | ||
| } | ||
|
|
||
| if (json.length > byteLimit) { | ||
| // Truncate the free-text fields before serializing so the block stays valid JSON. | ||
| json = JSON.stringify( | ||
| { | ||
| ...payload, | ||
| what: truncateField(details.what, 160), | ||
| why: truncateField(details.why, 160), | ||
| next: payload.next.slice(0, 1), | ||
| }, | ||
| null, | ||
| 2, | ||
| ) | ||
| } | ||
|
|
||
| if (json.length > byteLimit) { | ||
| // Last resort: minimal payload that is still valid JSON. | ||
| json = JSON.stringify({ ...payload, what: "Error.", why: "Error.", next: [] }, null, 2) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
byteLimit is compared against a character count, not a byte count.
All three checks use json.length, which counts UTF-16 code units. A why field of non-ASCII text passes an 8000 "byte" limit while encoding to roughly three times that many bytes. The error-interception module already exports encodeUtf8Bytes and getPayloadByteLength from MessageTransformer for this measurement (see index.ts lines 23 and 28). Reuse them, or rename the parameter to charLimit and state the unit in the doc comment.
🐛 Proposed fix
+import { getPayloadByteLength } from "../tools/error-interception/MessageTransformer"
+
...
let json = JSON.stringify(payload, null, 2)
- if (json.length > byteLimit && payload.next.length > 1) {
+ if (getPayloadByteLength(json) > byteLimit && payload.next.length > 1) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/assistant-message/structuredError.ts` around lines 121 - 171, Update
formatStructuredError to measure the serialized payload in UTF-8 bytes rather
than comparing json.length against byteLimit. Reuse the existing encodeUtf8Bytes
or getPayloadByteLength helpers from MessageTransformer for all three size
checks, preserving the current trimming and fallback behavior and the byteLimit
API semantics.
| const nextSection = message.match(/Next:\n((?:\d+\..+\n?)+)/) | ||
| expect(nextSection).toBeDefined() | ||
| const items = nextSection![1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
toBeDefined() does not reject null.
String.prototype.match returns null when there is no match. expect(null).toBeDefined() passes, because null is not undefined. The assertion at line 95 therefore cannot catch a missing Next: section. The test still fails on line 96 through the non-null assertion, but with a TypeError instead of the intended message.
💚 Proposed fix
const nextSection = message.match(/Next:\n((?:\d+\..+\n?)+)/)
- expect(nextSection).toBeDefined()
+ expect(nextSection).not.toBeNull()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nextSection = message.match(/Next:\n((?:\d+\..+\n?)+)/) | |
| expect(nextSection).toBeDefined() | |
| const items = nextSection![1] | |
| const nextSection = message.match(/Next:\n((?:\d+\..+\n?)+)/) | |
| expect(nextSection).not.toBeNull() | |
| const items = nextSection![1] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts`
around lines 94 - 96, Update the assertion around the nextSection match in the
MessageTransformer test to explicitly reject null, using an assertion
appropriate for String.prototype.match results before accessing nextSection[1].
Preserve the existing validation of the captured section and avoid relying on
the non-null assertion to detect a missing match.
| it("flags an object with two known parameter keys", () => { | ||
| const signal = validateNestedParams({ input: { path: "a", regex: "b" } }, "search_files") | ||
| expect(signal).not.toBeNull() | ||
| }) | ||
|
|
||
| it("does not flag a single known key on its own when it is not a tool signature", () => { | ||
| const signal = validateNestedParams({ meta: { note: "x" } }, "some_tool") | ||
| expect(signal).toBeNull() | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the fixtures so they exercise the multi-known-keys heuristic.
{ path: "a", regex: "b" } matches the explicit ["path", "regex"] entry in TOOL_SIGNATURE_KEY_SETS, so detectToolSignature returns before the known-key counter runs. The multi-known-keys branch stays untested. The second test uses note, which is not in KNOWN_PARAMETER_KEYS, so the single-known-key boundary also stays untested. Use two known keys that form no declared signature, and one known key on its own.
💚 Proposed fix
it("flags an object with two known parameter keys", () => {
- const signal = validateNestedParams({ input: { path: "a", regex: "b" } }, "search_files")
- expect(signal).not.toBeNull()
+ const signal = validateNestedParams({ input: { mode: "a", slug: "b" } }, "some_tool")
+ expect(signal?.metadata.structuralReason).toBe("nested-tool-input:multi-known-keys")
})
it("does not flag a single known key on its own when it is not a tool signature", () => {
- const signal = validateNestedParams({ meta: { note: "x" } }, "some_tool")
+ const signal = validateNestedParams({ meta: { mode: "x" } }, "some_tool")
expect(signal).toBeNull()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("flags an object with two known parameter keys", () => { | |
| const signal = validateNestedParams({ input: { path: "a", regex: "b" } }, "search_files") | |
| expect(signal).not.toBeNull() | |
| }) | |
| it("does not flag a single known key on its own when it is not a tool signature", () => { | |
| const signal = validateNestedParams({ meta: { note: "x" } }, "some_tool") | |
| expect(signal).toBeNull() | |
| }) | |
| it("flags an object with two known parameter keys", () => { | |
| const signal = validateNestedParams({ input: { mode: "a", slug: "b" } }, "some_tool") | |
| expect(signal?.metadata.structuralReason).toBe("nested-tool-input:multi-known-keys") | |
| }) | |
| it("does not flag a single known key on its own when it is not a tool signature", () => { | |
| const signal = validateNestedParams({ meta: { mode: "x" } }, "some_tool") | |
| expect(signal).toBeNull() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/__tests__/StructuralValidator.spec.ts`
around lines 101 - 109, Update the fixtures in validateNestedParams tests:
change the first case to use two entries from KNOWN_PARAMETER_KEYS that do not
form any TOOL_SIGNATURE_KEY_SETS signature, so it reaches the multi-known-keys
heuristic; change the second case to use exactly one KNOWN_PARAMETER_KEYS entry
while keeping it a non-signature tool.
| function fitDetailsWithinByteLimit(payload: GuidancePayload, byteLimit: number): string { | ||
| const fullDetails = formatPayloadAsDetails(payload) | ||
| if (countUtf8Bytes(fullDetails) <= byteLimit) return fullDetails | ||
|
|
||
| let candidate = { ...payload } | ||
| const type = payload.type | ||
|
|
||
| // Phase 1: Remove Next items from the end, but always try to keep at | ||
| // least the first continuation action. | ||
| for (let nextCount = payload.next.length; nextCount >= 1; nextCount--) { | ||
| candidate = { | ||
| ...candidate, | ||
| next: payload.next.slice(0, nextCount), | ||
| } | ||
|
|
||
| let details = formatPayloadAsDetails(candidate) | ||
| if (countUtf8Bytes(details) <= byteLimit) return details | ||
|
|
||
| // Phase 2: Truncate Why before What (What carries the structural fact). | ||
| for (const targetBytes of [80, 50, 30]) { | ||
| candidate = { ...candidate, why: truncateString(candidate.why, targetBytes) } | ||
| details = formatPayloadAsDetails(candidate) | ||
| if (countUtf8Bytes(details) <= byteLimit) return details | ||
| } | ||
|
|
||
| // Phase 3: Truncate What. | ||
| for (const targetBytes of [120, 80, 50, 30]) { | ||
| candidate = { ...candidate, what: truncateString(candidate.what, targetBytes) } | ||
| details = formatPayloadAsDetails(candidate) | ||
| if (countUtf8Bytes(details) <= byteLimit) return details | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Truncation order contradicts the documented priority.
The doc comment at lines 351-359 says additional Next items are removed first, and that What is truncated last among content fields. The implementation nests phases 2 and 3 inside the nextCount loop, so it does the opposite.
On the first iteration nextCount equals payload.next.length, so no Next item has been dropped yet. The code still runs the full [80, 50, 30] and [120, 80, 50, 30] truncation ladders. An overflowing payload therefore gets What and Why clipped to 30 bytes while every Next item is retained. What carries the structural fact the model needs most.
A second effect compounds it. Lines 380 and 387 truncate candidate.why and candidate.what, not payload.why and payload.what, so the truncation is cumulative and is never restored between iterations. After the first iteration both fields are already at 30 bytes, so every later ladder call is a no-op and the original text cannot be recovered.
Move the content truncation outside the nextCount loop, and derive each candidate field from payload.
🐛 Proposed fix
- let candidate = { ...payload }
const type = payload.type
// Phase 1: Remove Next items from the end, but always try to keep at
// least the first continuation action.
for (let nextCount = payload.next.length; nextCount >= 1; nextCount--) {
- candidate = {
- ...candidate,
- next: payload.next.slice(0, nextCount),
- }
-
- let details = formatPayloadAsDetails(candidate)
+ const details = formatPayloadAsDetails({ ...payload, next: payload.next.slice(0, nextCount) })
if (countUtf8Bytes(details) <= byteLimit) return details
+ }
- // Phase 2: Truncate Why before What (What carries the structural fact).
- for (const targetBytes of [80, 50, 30]) {
- candidate = { ...candidate, why: truncateString(candidate.why, targetBytes) }
- details = formatPayloadAsDetails(candidate)
- if (countUtf8Bytes(details) <= byteLimit) return details
- }
+ // Only the first continuation action is retained from here on.
+ let candidate: GuidancePayload = { ...payload, next: payload.next.slice(0, 1) }
- // Phase 3: Truncate What.
- for (const targetBytes of [120, 80, 50, 30]) {
- candidate = { ...candidate, what: truncateString(candidate.what, targetBytes) }
- details = formatPayloadAsDetails(candidate)
- if (countUtf8Bytes(details) <= byteLimit) return details
- }
+ // Phase 2: Truncate Why before What (What carries the structural fact).
+ for (const targetBytes of [80, 50, 30]) {
+ candidate = { ...candidate, why: truncateString(payload.why, targetBytes) }
+ const details = formatPayloadAsDetails(candidate)
+ if (countUtf8Bytes(details) <= byteLimit) return details
+ }
+
+ // Phase 3: Truncate What.
+ for (const targetBytes of [120, 80, 50, 30]) {
+ candidate = { ...candidate, what: truncateString(payload.what, targetBytes) }
+ const details = formatPayloadAsDetails(candidate)
+ if (countUtf8Bytes(details) <= byteLimit) return details
}Add a test that asserts What retains substantive content when only a trailing Next item needs to be dropped. The existing test at src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts lines 491-509 does not assert What content, so it passes under both orderings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function fitDetailsWithinByteLimit(payload: GuidancePayload, byteLimit: number): string { | |
| const fullDetails = formatPayloadAsDetails(payload) | |
| if (countUtf8Bytes(fullDetails) <= byteLimit) return fullDetails | |
| let candidate = { ...payload } | |
| const type = payload.type | |
| // Phase 1: Remove Next items from the end, but always try to keep at | |
| // least the first continuation action. | |
| for (let nextCount = payload.next.length; nextCount >= 1; nextCount--) { | |
| candidate = { | |
| ...candidate, | |
| next: payload.next.slice(0, nextCount), | |
| } | |
| let details = formatPayloadAsDetails(candidate) | |
| if (countUtf8Bytes(details) <= byteLimit) return details | |
| // Phase 2: Truncate Why before What (What carries the structural fact). | |
| for (const targetBytes of [80, 50, 30]) { | |
| candidate = { ...candidate, why: truncateString(candidate.why, targetBytes) } | |
| details = formatPayloadAsDetails(candidate) | |
| if (countUtf8Bytes(details) <= byteLimit) return details | |
| } | |
| // Phase 3: Truncate What. | |
| for (const targetBytes of [120, 80, 50, 30]) { | |
| candidate = { ...candidate, what: truncateString(candidate.what, targetBytes) } | |
| details = formatPayloadAsDetails(candidate) | |
| if (countUtf8Bytes(details) <= byteLimit) return details | |
| } | |
| } | |
| function fitDetailsWithinByteLimit(payload: GuidancePayload, byteLimit: number): string { | |
| const fullDetails = formatPayloadAsDetails(payload) | |
| if (countUtf8Bytes(fullDetails) <= byteLimit) return fullDetails | |
| const type = payload.type | |
| // Phase 1: Remove Next items from the end, but always try to keep at | |
| // least the first continuation action. | |
| for (let nextCount = payload.next.length; nextCount >= 1; nextCount--) { | |
| const details = formatPayloadAsDetails({ ...payload, next: payload.next.slice(0, nextCount) }) | |
| if (countUtf8Bytes(details) <= byteLimit) return details | |
| } | |
| // Only the first continuation action is retained from here on. | |
| let candidate: GuidancePayload = { ...payload, next: payload.next.slice(0, 1) } | |
| // Phase 2: Truncate Why before What (What carries the structural fact). | |
| for (const targetBytes of [80, 50, 30]) { | |
| candidate = { ...candidate, why: truncateString(payload.why, targetBytes) } | |
| const details = formatPayloadAsDetails(candidate) | |
| if (countUtf8Bytes(details) <= byteLimit) return details | |
| } | |
| // Phase 3: Truncate What. | |
| for (const targetBytes of [120, 80, 50, 30]) { | |
| candidate = { ...candidate, what: truncateString(payload.what, targetBytes) } | |
| const details = formatPayloadAsDetails(candidate) | |
| if (countUtf8Bytes(details) <= byteLimit) return details | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/MessageTransformer.ts` around lines 360 -
391, Update fitDetailsWithinByteLimit so the nextCount loop removes trailing
Next items before applying content truncation, with the Why and What ladders
moved outside that loop and each candidate derived from the original payload
fields. Preserve the documented priority by retaining substantive What content
whenever dropping a trailing Next item is sufficient, and add a test in
MessageTransformer.spec.ts covering that behavior.
| function buildSignal( | ||
| source: InterceptionSignal["source"], | ||
| stage: InterceptionSignal["stage"], | ||
| toolName: string | undefined, | ||
| metadata: Readonly<Record<string, unknown>>, | ||
| ): InterceptionSignal { | ||
| return { | ||
| source, | ||
| stage, | ||
| taskId: "", | ||
| toolName, | ||
| metadata, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers of the structural validators and check whether they set taskId.
set -euo pipefail
rg -nP --type=ts -C6 '\b(validateCwdParameter|validateNestedParams)\s*\(' \
-g '!**/error-interception/StructuralValidator.ts'
echo "== taskId assignments in the interception module =="
fd -t f -e ts . src/core/tools/error-interception --exec rg -n 'taskId' {}Repository: Zoo-Code-Org/Zoo-Code
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Description: Inspect the file and related interception references without relying on potentially unavailable fd.
set -euo pipefail
echo "== files =="
git ls-files | rg 'src/core/tools/error-interception|types\.ts$' | sed -n '1,120p'
echo "== StructuralValidator outline/size =="
wc -l src/core/tools/error-interception/StructuralValidator.ts || true
ast-grep outline src/core/tools/error-interception/StructuralValidator.ts || true
echo "== buildSignal and callers in repository =="
rg -nP --type=ts -C5 '\bbuildSignal\(|validateCwdParameter\b|validateNestedParams\b|InterceptionSignal|taskId' . | sed -n '1,260p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 24334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== StructuralValidator relevant lines =="
cat -n src/core/tools/error-interception/StructuralValidator.ts | sed -n '90,265p'
echo "== error-interception types =="
cat -n src/core/tools/error-interception/types.ts | sed -n '1,180p'
echo "== ToolErrorInterceptor and TaskErrorState relevant lines =="
cat -n src/core/tools/error-interception/ToolErrorInterceptor.ts | sed -n '1,260p'
cat -n src/core/tools/error-interception/TaskErrorState.ts | sed -n '1,260p'
echo "== precise validator call occurrences =="
rg -nP --type=ts -C8 '(validateCwdParameter|validateNestedParams|buildSignal|InterceptionSignal)' src/core/tools/error-interception src/core/tools src/core src/workers apps/webview-?ui webview-ui 2>/dev/null | sed -n '1,260p'
echo "== all validator references repo-wide =="
rg -nP --type=ts '(validateCwdParameter|validateNestedParams|buildSignal)' . | sed -n '1,260p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
Pass taskId into the structural validators.
buildSignal() emits taskId: "", but transformSignal() only checks for a missing Task object, which accepts empty string taskIds with invalid per-task state. Make the signal contract explicit by passing taskId through validateCwdParameter / validateNestedParams before classification, so these validation signals cannot reuse an empty key later.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/StructuralValidator.ts` around lines 111 -
124, Update buildSignal and the structural-validator flow to accept and pass the
actual taskId through validateCwdParameter and validateNestedParams before
classification. Remove the hardcoded empty taskId so each validation signal
carries its originating task identifier and cannot be associated with an empty
per-task key.
| public reset(category?: string): void { | ||
| if (category !== undefined) { | ||
| this.perCategory.delete(category) | ||
| return | ||
| } | ||
| this.perCategory.clear() | ||
| } | ||
|
|
||
| /** Returns the pending native protocol guide without clearing it. */ | ||
| public getPendingNativeProtocolGuide(): string | undefined { | ||
| return this.pendingGuide | ||
| } | ||
|
|
||
| /** Queues a native protocol guide to be merged into the next tool_result. */ | ||
| public setPendingNativeProtocolGuide(guide: string): void { | ||
| this.pendingGuide = guide | ||
| } | ||
|
|
||
| /** Clears any pending native protocol guide. */ | ||
| public clearPendingNativeProtocolGuide(): void { | ||
| this.pendingGuide = undefined | ||
| } | ||
|
|
||
| /** | ||
| * Atomically reads and clears the pending native protocol guide. | ||
| * Returns undefined when no guide is queued. | ||
| */ | ||
| public consumePendingNativeProtocolGuide(): string | undefined { | ||
| const guide = this.pendingGuide | ||
| this.pendingGuide = undefined | ||
| return guide | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check every emit path that should consume the pending native protocol guide.
set -uo pipefail
echo "=== All pending-guide accessor call sites ==="
rg -nP --type=ts -C6 '(get|set|clear|consume)PendingNativeProtocolGuide\s*\('
echo "=== Full reset call sites that may drop a queued guide ==="
rg -nP --type=ts -C6 'getTaskErrorState\([^)]*\)\.reset\s*\('Repository: Zoo-Code-Org/Zoo-Code
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Locate relevant files ==="
fd -a 'TaskErrorState\.ts|ToolErrorInterceptor\.ts' . | sed 's#^\./##'
echo "=== Search pending-guide textual references ==="
rg -n -C4 'pending.*guide|native protocol guide|consumePending|getPending|setPending|clearPending|resetTaskState|pendingGuide' --type=ts .
echo "=== Inspect TaskErrorState relevant section ==="
file="$(fd 'TaskErrorState\.ts' . | head -n1)"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,170p' "$file" | nl -ba
fi
echo "=== Inspect ToolErrorInterceptor relevant section ==="
interceptor="$(fd 'ToolErrorInterceptor\.ts' . | head -n1)"
if [ -n "$interceptor" ]; then
wc -l "$interceptor"
sed -n '1,220p' "$interceptor" | nl -ba
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 36509
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== ToolErrorInterceptor resetTaskState implementation ==="
sed -n '1,180p' src/core/tools/error-interception/ToolErrorInterceptor.ts | cat -n
echo "=== TaskErrorState reset and pending methods ==="
sed -n '90,140p' src/core/tools/error-interception/TaskErrorState.ts | cat -n
echo "=== Pending native protocol guide references across tracked source files ==="
git ls-files '*.{ts,tsx}' | xargs rg -n '(pendingGuide|setPendingNativeProtocolGuide|consumePendingNativeProtocolGuide|getPendingNativeProtocolGuide|clearPendingNativeProtocolGuide|NativeProtocolGuide|native protocol guide)' || true
echo "=== Reset tests around resetTaskState ==="
sed -n '348,570p' src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts | cat -n
echo "=== TaskErrorState tests if present ==="
fd -i 'TaskErrorState.*spec|.*ToolErrorInterceptor.*spec' src/core/tools/error-interception | while read -r f; do
echo "--- $f"
wc -l "$f"
rg -n -C4 'pendingGuide|NativeProtocolGuide|resetTaskState|consumePending|getPending|clearPending' "$f" || true
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 26172
Clear pendingGuide during full reset.
reset() clears every category, but pendingGuide is reset-independent state that can still leak into later turns. A queued native protocol guide survives reset(), and ToolErrorInterceptor.resetTaskState(task) calls it for full resets in ToolErrorInterceptor.ts.
🐛 Proposed fix for the reset path
public reset(category?: string): void {
if (category !== undefined) {
this.perCategory.delete(category)
return
}
this.perCategory.clear()
+ this.pendingGuide = undefined
}Add a guardrail test that queues a guide, calls reset(), and asserts consumePendingNativeProtocolGuide() returns undefined.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public reset(category?: string): void { | |
| if (category !== undefined) { | |
| this.perCategory.delete(category) | |
| return | |
| } | |
| this.perCategory.clear() | |
| } | |
| /** Returns the pending native protocol guide without clearing it. */ | |
| public getPendingNativeProtocolGuide(): string | undefined { | |
| return this.pendingGuide | |
| } | |
| /** Queues a native protocol guide to be merged into the next tool_result. */ | |
| public setPendingNativeProtocolGuide(guide: string): void { | |
| this.pendingGuide = guide | |
| } | |
| /** Clears any pending native protocol guide. */ | |
| public clearPendingNativeProtocolGuide(): void { | |
| this.pendingGuide = undefined | |
| } | |
| /** | |
| * Atomically reads and clears the pending native protocol guide. | |
| * Returns undefined when no guide is queued. | |
| */ | |
| public consumePendingNativeProtocolGuide(): string | undefined { | |
| const guide = this.pendingGuide | |
| this.pendingGuide = undefined | |
| return guide | |
| } | |
| public reset(category?: string): void { | |
| if (category !== undefined) { | |
| this.perCategory.delete(category) | |
| return | |
| } | |
| this.perCategory.clear() | |
| this.pendingGuide = undefined | |
| } | |
| /** Returns the pending native protocol guide without clearing it. */ | |
| public getPendingNativeProtocolGuide(): string | undefined { | |
| return this.pendingGuide | |
| } | |
| /** Queues a native protocol guide to be merged into the next tool_result. */ | |
| public setPendingNativeProtocolGuide(guide: string): void { | |
| this.pendingGuide = guide | |
| } | |
| /** Clears any pending native protocol guide. */ | |
| public clearPendingNativeProtocolGuide(): void { | |
| this.pendingGuide = undefined | |
| } | |
| /** | |
| * Atomically reads and clears the pending native protocol guide. | |
| * Returns undefined when no guide is queued. | |
| */ | |
| public consumePendingNativeProtocolGuide(): string | undefined { | |
| const guide = this.pendingGuide | |
| this.pendingGuide = undefined | |
| return guide | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/TaskErrorState.ts` around lines 105 - 136,
Update TaskErrorState.reset so a full reset with no category also clears
pendingGuide, while preserving category-specific reset behavior. Add a guardrail
test that queues a guide, calls reset(), and verifies
consumePendingNativeProtocolGuide() returns undefined.
| /** | ||
| * Wraps the original raw handleError callback. The original callback is | ||
| * invoked first so UI/diagnostics receive the raw error, then a transformed | ||
| * model-facing result is pushed via pushToolResult. | ||
| */ | ||
| decoratedHandleError: HandleError |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the decoratedHandleError contract documentation.
The doc states the original callback runs first and the transformed result is pushed afterwards. The implementation at lines 186-206 does the opposite: it pushes the transformed result through rawPushToolResult first, then awaits rawHandleError. The inline comment at lines 186-188 explains that this order is intentional because the raw callback holds an exactly-once guard. Align the interface doc with the actual order so future changes do not "fix" the ordering and silently drop the guided payload.
📝 Proposed doc fix
/**
- * Wraps the original raw handleError callback. The original callback is
- * invoked first so UI/diagnostics receive the raw error, then a transformed
- * model-facing result is pushed via pushToolResult.
+ * Wraps the original raw handleError callback. The transformed model-facing
+ * result is pushed via pushToolResult first, so the exactly-once guard in
+ * the raw callback keeps the guided payload. The original callback is then
+ * invoked so UI/diagnostics still receive the raw error.
*/
decoratedHandleError: HandleError📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Wraps the original raw handleError callback. The original callback is | |
| * invoked first so UI/diagnostics receive the raw error, then a transformed | |
| * model-facing result is pushed via pushToolResult. | |
| */ | |
| decoratedHandleError: HandleError | |
| /** | |
| * Wraps the original raw handleError callback. The transformed model-facing | |
| * result is pushed via pushToolResult first, so the exactly-once guard in | |
| * the raw callback keeps the guided payload. The original callback is then | |
| * invoked so UI/diagnostics still receive the raw error. | |
| */ | |
| decoratedHandleError: HandleError |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/ToolErrorInterceptor.ts` around lines 26 -
31, Update the decoratedHandleError documentation in ToolErrorInterceptor to
state that the transformed result is pushed through rawPushToolResult before the
original rawHandleError callback is invoked. Preserve the intentional ordering
described by the inline comment and do not alter the implementation.
| if (trimmed.startsWith("Error:") || trimmed.startsWith("error:") || trimmed.startsWith("ERROR")) return true | ||
| if (trimmed.startsWith("<error_details>")) return true | ||
| if (trimmed.startsWith("File does not exist")) return true | ||
| if (trimmed.startsWith("cannot find path") || trimmed.startsWith("Path not found")) return true | ||
| if (trimmed.startsWith("apply_diff failed") || trimmed.includes("no sufficiently similar match")) | ||
| return true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the diff-failure marker case-insensitively.
trimmed.includes("no sufficiently similar match") is case-sensitive. The real message produced by the diff tool is capitalized: No sufficiently similar match found at line: 326. A result carrying that text, without a <error_details> or apply_diff failed prefix, returns false here and never reaches transformSignal. That is the primary failure mode this PR targets. Compare on a lowercased copy. The same applies to the ERROR / error: prefix checks, which currently need three separate comparisons.
🐛 Proposed fix
- if (trimmed.startsWith("Error:") || trimmed.startsWith("error:") || trimmed.startsWith("ERROR")) return true
+ const lowered = trimmed.toLowerCase()
+ if (lowered.startsWith("error:") || lowered.startsWith("error ")) return true
if (trimmed.startsWith("<error_details>")) return true
if (trimmed.startsWith("File does not exist")) return true
if (trimmed.startsWith("cannot find path") || trimmed.startsWith("Path not found")) return true
- if (trimmed.startsWith("apply_diff failed") || trimmed.includes("no sufficiently similar match"))
+ if (lowered.startsWith("apply_diff failed") || lowered.includes("no sufficiently similar match"))
return true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/error-interception/ToolErrorInterceptor.ts` around lines 329 -
334, Update the error-marker checks in the interceptor’s string-matching method
to compare a lowercased copy of trimmed, so “No sufficiently similar match
found…” is detected regardless of capitalization and reaches transformSignal.
Reuse that normalized value to consolidate the ERROR/error: prefix checks into
one case-insensitive comparison while preserving the existing markers and
behavior.
1 participant
Stack Position
feat/error-interception-middlewareDescription
Full Feature Description
feat/error-interception-middlewaresrc/core/tools/error-interceptionand the final integration pointpresentAssistantMessage.ts. Maintained as an internal middleware boundary without changing public provider/tool contracts.UNCLASSIFIEDwhile preserving the original text and cause. On transformation or structural validation failure, falls back to the original error. If presentation itself fails, interception is not recursively invoked. Repeated occurrences of the same fingerprint escalate tocorrect_once,change_strategy,await_user, etc. based on occurrence count, without producing duplicate messages. Metadata does not include sensitive values such as commands, absolute paths, or raw arguments.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Connects the interception runtime to
presentAssistantMessage.ts. Generates a structured recovery message only once per error, preserving normal assistant content and the legacy error path.Included Files
src/core/assistant-message/presentAssistantMessage.tssrc/core/assistant-message/__tests__/presentAssistantMessage.error-interception.spec.tsExclusion Scope
Summary by CodeRabbit