feat: error-interception-middleware (2/3) - #1126
Conversation
…ask keys getTaskState guarded only falsy keys and the module-level getTaskErrorState/hasTaskErrorState had no guard at all, so a primitive non-null key (e.g. a string taskId, an easy mistake since InterceptorOptions.taskId is a string) still threw TypeError on WeakMap.set(). Both accessors now fail open: invalid keys get an ephemeral state that is never stored, matching the existing fail-open philosophy.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughChangesThe PR adds an error-interception subsystem. It defines error contracts and patterns, validates tool arguments, classifies and sanitizes failures, transforms them into bounded guidance, tracks task-scoped state, and intercepts tool callbacks. Error interception middleware
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ToolCallback
participant ToolErrorInterceptor
participant ErrorClassifier
participant TaskErrorState
participant MessageTransformer
ToolCallback->>ToolErrorInterceptor: provide result or exception
ToolErrorInterceptor->>ErrorClassifier: classify interception signal
ErrorClassifier-->>ToolErrorInterceptor: return sanitized classification
ToolErrorInterceptor->>TaskErrorState: record occurrence
ToolErrorInterceptor->>MessageTransformer: transform classification
MessageTransformer-->>ToolErrorInterceptor: return bounded error_details message
ToolErrorInterceptor-->>ToolCallback: forward transformed or unchanged result
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/tools/error-interception/ErrorClassifier.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/tools/error-interception/MessageTransformer.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/core/tools/error-interception/StructuralValidator.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
awaiting-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (18)
src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts (3)
296-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the constant name in the test title.
The title names
SHELL_INTEGRATION_THRESHOLD. The exported constant isSHELL_CIRCUIT_THRESHOLD, which the body uses on line 308.🤖 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, Update the test title in the circuit-opening test to reference the exported SHELL_CIRCUIT_THRESHOLD constant name instead of SHELL_INTEGRATION_THRESHOLD, matching the constant used by the test body.
842-845: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the pushed content, not only the call count.
These three tests assert
toHaveBeenCalledTimes(1)alone. The decorated callback always calls the raw callback exactly once, both when it transforms the content and when it fails open. The assertions therefore pass for either outcome and cannot detect a regression in the inferred status. Assert the pushed payload.💚 Example for the `file-not-found` case on lines 842-845
decoratedPushToolResult(content) expect(pushToolResult).toHaveBeenCalledTimes(1) + const pushed = (pushToolResult.mock.calls[0] as [Array<Record<string, unknown>>])[0] + expect(String(pushed[0].text)).toContain("Category: FILE_NOT_FOUND") })Also applies to: 863-867, 884-888
🤖 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 842 - 845, Update the three tests around decoratedPushToolResult to assert the payload passed to pushToolResult, not just that it was called once. Verify the expected transformed content for the successful case and the original content for the fail-open cases, preserving the single-call assertion if useful.
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the avoidable
as unknown asdouble assertions in this spec. Both sites cast values that a typed API or the plain literal type already covers, so the assertions only hide type drift. The documented double assertions on lines 611-614 remain acceptable because no typed alternative exists there.
src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts#L15-L16: replace the casts withvi.fn<HandleError>()andvi.fn<PushToolResult>(), and delete the now-unusedMockHandleErrorandMockPushToolResultaliases.src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts#L752-L753: pass the string literals directly, becauseToolResponsealready acceptsstring; apply the same removal on line 771.As per coding guidelines, "Avoid
as any; use typed APIs, bracket notation for private members when necessary, precise test doubles, orunknownwith a type guard. Use double assertions only as a last resort and document them."🤖 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 15 - 16, Remove the avoidable double assertions in src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts:15-16 by using typed vi.fn<HandleError>() and vi.fn<PushToolResult>() and deleting the now-unused MockHandleError and MockPushToolResult aliases. At src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts:752-753 and line 771, pass string literals directly where ToolResponse accepts string; leave the documented assertions at lines 611-614 unchanged.Source: Coding guidelines
src/core/tools/error-interception/types.ts (1)
87-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local
ToolResponseinterface to avoid a collision with the shared type.
src/shared/tools.tsalready exportsToolResponseasstring | Array<Anthropic.TextBlockParam | Anthropic.ImageBlockParam>. This file declares a different, object-shapedToolResponse, andindex.tsre-exports it on line 17. A consumer that importsToolResponsefrom the error-interception barrel receives a structurally incompatible type with the same name. Rename the local type, for example toStructuredToolResult, and updateInterceptionSignal.resultplus the barrel export.♻️ Proposed rename
-export interface ToolResponse { +export interface StructuredToolResult { type?: string status?: string error?: unknown text?: string toolUseId?: string [key: string]: unknown }Then update the signal field and the barrel export:
// types.ts result?: StructuredToolResult // index.ts export type { /* ... */ StructuredToolResult /* ... */ } from "./types.ts"🤖 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 87 - 94, Rename the object-shaped ToolResponse interface in the error-interception types module to StructuredToolResult, update InterceptionSignal.result to use the renamed type, and replace the ToolResponse barrel export in index.ts with StructuredToolResult while preserving the existing fields and exports.src/core/tools/error-interception/errorPatterns.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the module specifier style with the other files in this module.
This file imports
"./types.ts"with the explicit extension.StructuralValidator.tsline 1 imports"./types"without it, andindex.tsmixes both styles. Pick one style for the module so the resolver configuration stays predictable.🤖 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 1, Standardize the relative import specifiers across the error-interception module, including the `ErrorPattern`, `InterceptionSignal`, and `RecoveryDisposition` import in `errorPatterns.ts`, to use one consistent extension style. Align `StructuralValidator.ts` and the mixed imports in `index.ts` with the chosen module-wide convention.src/core/tools/error-interception/__tests__/StructuralValidator.spec.ts (2)
101-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
multi-known-keysheuristic and fix the second test name.The first test uses
{ path: "a", regex: "b" }. That input matches the explicit["path", "regex"]entry inTOOL_SIGNATURE_KEY_SETS, so themulti-known-keysbranch indetectToolSignaturestays uncovered. The second test uses{ note: "x" }, andnoteis not a member ofKNOWN_PARAMETER_KEYS, so it does not test "a single known key" either.💚 Proposed test changes
- it("flags an object with two known parameter keys", () => { - const signal = validateNestedParams({ input: { path: "a", regex: "b" } }, "search_files") - expect(signal).not.toBeNull() - }) + it("flags an object with two known parameter keys via the heuristic", () => { + 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") - expect(signal).toBeNull() - }) + it("does not flag a single known key on its own", () => { + const signal = validateNestedParams({ meta: { mode: "x" } }, "some_tool") + expect(signal).toBeNull() + })As per path instructions, "Add focused tests for UI binding and save behavior, persistence or normalization … cover both true and false/unset defaulting cases."
🤖 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 tests around validateNestedParams to exercise detectToolSignature’s multi-known-keys heuristic with two KNOWN_PARAMETER_KEYS that do not match an explicit TOOL_SIGNATURE_KEY_SETS entry, and assert the signal is detected. Rename and revise the single-key test to use an actual known parameter key while preserving the expectation that it is ignored for a non-tool signature.Source: Path instructions
150-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the depth and node-limit tests able to fail.
Both fixtures contain no tool signature at any level.
validateNestedParamstherefore returns null for reasons unrelated to the bounds. IfNESTED_DETECTION_MAX_DEPTHorNESTED_DETECTION_MAX_NODESis removed, both tests still pass. Place a real signature beyond each limit, and add a positive control inside each limit.💚 Proposed test changes
it("bounds recursion to NESTED_DETECTION_MAX_DEPTH", () => { - let deep: Record<string, unknown> = { leaf: 1 } - for (let i = 0; i < NESTED_DETECTION_MAX_DEPTH + 3; i += 1) { - deep = { wrap: deep } - } - expect(NESTED_DETECTION_MAX_DEPTH).toBeGreaterThan(0) - const signal = validateNestedParams({ outer: deep }, "some_tool") - expect(signal).toBeNull() + const wrap = (levels: number): Record<string, unknown> => { + let node: Record<string, unknown> = { command: "x" } + for (let i = 0; i < levels; i += 1) { + node = { wrap: node } + } + return node + } + // Signature inside the limit is detected. + expect(validateNestedParams({ outer: wrap(1) }, "some_tool")).not.toBeNull() + // Signature beyond the limit is not reached. + expect(validateNestedParams({ outer: wrap(NESTED_DETECTION_MAX_DEPTH + 3) }, "some_tool")).toBeNull() }) it("bounds total visited nodes to NESTED_DETECTION_MAX_NODES", () => { const wide: Record<string, unknown> = {} for (let i = 0; i < NESTED_DETECTION_MAX_NODES + 10; i += 1) { wide[`k${i}`] = { child: i } } - expect(NESTED_DETECTION_MAX_NODES).toBeGreaterThan(0) + // A signature placed after the node budget is exhausted is not reached. + wide[`k${NESTED_DETECTION_MAX_NODES + 10}`] = { command: "x" } const signal = validateNestedParams({ outer: wide }, "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 150 - 168, Update the recursion and node-bound tests around validateNestedParams so each fixture includes a recognizable tool signature beyond its respective NESTED_DETECTION_MAX_DEPTH or NESTED_DETECTION_MAX_NODES limit, causing validation to return the expected signal when limits are enforced. Add a positive control with the same signature within each limit and assert detection, ensuring the tests fail if either bound is removed.src/core/tools/error-interception/ErrorClassifier.ts (4)
20-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe character blacklist at Line 25 is unreachable.
SAFE_IDENTIFIER_REallows only[a-zA-Z_]followed by[\w.]*, and\wis[A-Za-z0-9_]. No newline, quote, bracket, pipe, semicolon, backtick, or backslash can pass Line 23. Line 25 therefore never rejects anything. Keep it only as intentional defense-in-depth. If you keep it, state that intent in the comment so a future reader does not treat it as an active filter.🤖 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 20 - 27, The blacklist in isValidIdentifier is unreachable under SAFE_IDENTIFIER_RE; retain it only as explicit defense-in-depth and update the nearby comment to state that intent, without changing validation behavior.
240-248: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winResolve the catch-all pattern by identity, not by array position.
Line 241 assumes the last element of
ERROR_PATTERNSis theUNCLASSIFIEDcatch-all.ERROR_PATTERNSis ordered by descending priority, so a future pattern with the lowest priority can take that slot. The classifier would then return a wrong category, andToolErrorInterceptor.transformSignalwould no longer fail open. Look the pattern up by category or id, and keep a defined fallback.♻️ Proposed refactor
- const fallback = ERROR_PATTERNS[ERROR_PATTERNS.length - 1] + const fallback = + ERROR_PATTERNS.find((p) => p.category === "UNCLASSIFIED") ?? ERROR_PATTERNS[ERROR_PATTERNS.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/tools/error-interception/ErrorClassifier.ts` around lines 240 - 248, Update the catch-all handling in the error-classification function to locate the UNCLASSIFIED pattern by its stable category or id instead of ERROR_PATTERNS array position. Preserve the existing fallback response and ensure a defined fallback remains available if the expected pattern cannot be found, so ToolErrorInterceptor.transformSignal continues to fail open.
132-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the scanned text before the regex passes.
tryExtractParamNameFromTextreceives untrusted text.signal.result.textcan hold whole file contents or long shell output. Three regexes then scan the full string, and[^'"']+can backtrack across the remaining text for eachparameter 'occurrence. A bounded prefix removes the cost and keeps behavior for real error messages, which put the parameter name near the start.♻️ Proposed refactor
+const MAX_SCANNED_TEXT_LENGTH = 4096 + function tryExtractParamNameFromText(text: string): string | undefined { + const scanned = text.length > MAX_SCANNED_TEXT_LENGTH ? text.slice(0, MAX_SCANNED_TEXT_LENGTH) : text // Pattern: "parameter 'name'" or "parameter \"name\"" or "parameter: name" - const paramQuoteMatch = text.match(/parameter\s*['"']([^'"']+)['"']/i) + const paramQuoteMatch = scanned.match(/parameter\s*['"']([^'"']+)['"']/i) if (paramQuoteMatch) return paramQuoteMatch[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/ErrorClassifier.ts` around lines 132 - 148, Bound the input inside tryExtractParamNameFromText before running any regex matches, using a reasonable fixed-length prefix of the untrusted text. Apply all three existing patterns to that bounded value, preserving extraction behavior for parameter names near the beginning while preventing scans and backtracking across entire file contents or long command output.
169-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueArray facts stay aliased to the caller metadata after
Object.freeze.
Object.freezeis shallow. String arrays copied at Line 171 keep the reference fromsignal.metadata, so a later mutation of that array changes the frozen facts. Copy the array to make the returned facts effectively immutable.♻️ Proposed refactor
if (Array.isArray(value) && value.every((item) => typeof item === "string")) { - facts[key] = value + facts[key] = Object.freeze([...value]) }Also applies to: 207-207
🤖 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 169 - 173, Update the primitive-string array handling in ErrorClassifier’s facts construction to store a shallow copy of value rather than the caller-provided array, including the equivalent handling at the other occurrence. Preserve the existing validation and ensure the copied arrays cannot mutate the frozen facts through the original metadata reference.src/core/tools/error-interception/__tests__/TaskErrorState.spec.ts (1)
88-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the pending native protocol guide.
setPendingNativeProtocolGuide,getPendingNativeProtocolGuide,clearPendingNativeProtocolGuide, andconsumePendingNativeProtocolGuidehave no coverage. The class doc states the guide must not leak into later turns, so the consume-once behavior needs a test. Also assert the intended interaction withreset(), which currently leavespendingGuidein place.💚 Proposed tests
describe("pending native protocol guide", () => { it("returns undefined when no guide is queued", () => { expect(new TaskErrorState().getPendingNativeProtocolGuide()).toBeUndefined() }) 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 clear the pending guide", () => { const state = new TaskErrorState() state.setPendingNativeProtocolGuide("guide") state.reset() expect(state.getPendingNativeProtocolGuide()).toBe("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/__tests__/TaskErrorState.spec.ts` around lines 88 - 120, Add a “pending native protocol guide” test suite covering TaskErrorState.getPendingNativeProtocolGuide returning undefined initially, setPendingNativeProtocolGuide followed by consumePendingNativeProtocolGuide returning the guide only once, clearPendingNativeProtocolGuide removing the guide, and reset() preserving a queued guide.src/core/tools/error-interception/MessageTransformer.ts (2)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
isRetryable; Lines 102-103 return the same value.
retryPolicy === "auto-recover"returnstrue, and the final statement also returnstrue. The branch has no effect. Remove it, or list each policy explicitly so the intent foralternate-toolandcorrect-and-retryis visible.♻️ Proposed refactor
function isRetryable(retryPolicy: ErrorClassification["retryPolicy"], category: ErrorCategory): boolean { if (category === "DUPLICATE_CALL" || category === "INVALID_TOOL_PROTOCOL") return false - if (retryPolicy === "do-not-retry") return false - if (retryPolicy === "auto-recover") return true - return true + return retryPolicy !== "do-not-retry" }🤖 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 99 - 104, Simplify isRetryable by removing the redundant retryPolicy === "auto-recover" branch and preserving the existing behavior: excluded categories and "do-not-retry" return false, while all other policies return true.
258-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the unchecked
ErrorSourcecast with a guard.Line 261 casts
facts["errorSource"], which is typedunknown, directly toErrorSource | undefined.factscan carry any string here, so the cast asserts a type the code does not verify. Use a narrow guard, and reuse thefactsvalue destructured at Line 218.♻️ Proposed refactor
+function toErrorSource(value: unknown): ErrorSource | undefined { + return typeof value === "string" ? (value as ErrorSource) : undefined +} @@ - type: payloadType(classification.facts["errorSource"] as ErrorSource | undefined), + type: payloadType(toErrorSource(facts["errorSource"])),A stricter option is to compare against the known
ErrorSourcevalues and returnundefinedfor anything else.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members when necessary, precise test doubles, orunknownwith a type guard."🤖 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 258 - 270, Replace the unchecked cast in the returned object’s `type` field with a type guard that validates the destructured `facts["errorSource"]` against the known `ErrorSource` values, returning `undefined` for invalid strings. Reuse the `facts` value destructured near line 218 and preserve the existing `payloadType` behavior for valid sources.Source: Coding guidelines
src/core/tools/error-interception/ToolErrorInterceptor.ts (2)
286-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the project logger instead of
console.warn.This branch runs for every unclassified failure, so it can emit high-volume output on a hot path. Route it through the project logger at debug level, and keep the fields structured. The message already avoids raw error text, which is correct.
🤖 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 286 - 291, Replace the console.warn call in the unclassified-error branch of ToolErrorInterceptor with the project logger’s debug-level method. Preserve the existing message context while passing toolName and patternId as structured fields, and keep the raw error text excluded.
98-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse one WeakMap key guard.
Lines 102-104 repeat the check that
isWeakMapKeyalready performs insrc/core/tools/error-interception/TaskErrorState.tsLines 150-152. Export that helper and call it here, so both modules keep the same fail-open rule.🤖 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 98 - 111, Export the existing isWeakMapKey helper from TaskErrorState.ts and replace the duplicated type guard in ToolErrorInterceptor.getTaskState with a call to that shared helper. Preserve the current fail-open behavior by returning a fresh default state for invalid keys, while continuing to store and return task-specific state for valid WeakMap keys.src/core/tools/error-interception/TaskErrorState.ts (1)
34-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the category key as
ErrorCategory.
perCategoryusesstringkeys, whileInterceptorTaskState.categoryCountsinsrc/core/tools/error-interception/ToolErrorInterceptor.tsusesMap<ErrorCategory, number>. The looser key type lets a caller store an arbitrary string, which no reset path would ever clear becauseresetTaskStatepasses onlyErrorCategoryvalues. ImportErrorCategoryfrom./typesand use it for every category parameter.🤖 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 34 - 59, Update TaskErrorState to import ErrorCategory from ./types and change perCategory, getOrCreate, and getOccurrence category types from string to ErrorCategory. Use ErrorCategory consistently for all category parameters so callers cannot store keys outside the categories handled by resetTaskState.src/core/tools/error-interception/__tests__/ErrorClassifier.spec.ts (1)
890-896: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
classifyToolResultcase withouttoolCallId.The only test passes
"call-1", so it always supplies tool context. Add a case that omitstoolCallId. The test then documents the result for a caller that provides onlytaskId, which is the gap flagged insrc/core/tools/error-interception/ErrorClassifier.tsLines 252-272.🤖 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 890 - 896, Add a test in the classifyToolResult suite that invokes classifyToolResult with only the structured result and taskId, omitting toolCallId, and assert the expected classification and facts for that context. Keep the existing toolCallId test unchanged.
🤖 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 `@codecov.yml`:
- Around line 16-22: Update the webview-patch coverage policy documented in
webview-ui/AGENTS.md to reflect that coverage is informational rather than a
blocking 70% gate. Replace the obsolete requirement for modified webview-ui/src/
lines while preserving the surrounding contributor guidance.
- Line 1: Normalize the line endings in codecov.yml from CRLF to LF so the file
uses \n line endings and YAMLlint passes.
In `@src/core/tools/error-interception/__tests__/MessageTransformer.spec.ts`:
- Around line 94-105: Update the MessageTransformer test’s nextSection assertion
to use not.toBeNull(), matching String.match’s null failure value before
accessing nextSection[1]. Import and use NEXT_ITEM_COUNT_LIMIT and
NEXT_ITEM_CHAR_LIMIT in place of the literal item-count and character-length
limits.
In `@src/core/tools/error-interception/ErrorClassifier.ts`:
- Around line 252-272: Update classifyToolResult to accept an optional toolName
parameter and include it in the InterceptionSignal so tool-bound patterns can
match when toolCallId is absent. Update ToolErrorInterceptor.transformToolResult
to forward toolName from its options while preserving existing taskId and
toolCallId behavior.
In `@src/core/tools/error-interception/MessageTransformer.ts`:
- Around line 410-441: Address the byte-limit guarantee in
transformErrorToMessage and the Phase 6 minimal payload path: either define and
document a minimum supported byteLimit, or clamp requested limits to the size
required by the minimal payload. Ensure the documented contract matches actual
behavior, including variable pattern_id and category lengths, while preserving
the existing minimum payload fields and tags.
- Around line 204-215: The retryPolicy handling in the disposition inference
must not map non-duplicate failures to discard_duplicate. Update the logic
around the retryPolicy === "do-not-retry" branch so TOOL_NOT_FOUND,
MODE_RESTRICTION, FILE_RESTRICTION, and UNCLASSIFIED resolve to an appropriate
change_strategy or await_user disposition, while preserving discard_duplicate
for actual duplicate/protocol failures.
In `@src/core/tools/error-interception/StructuralValidator.ts`:
- Around line 250-255: Change OBJECT_ALLOWED_PARAMETERS to a Map or
prototype-less record so validateNestedParams only resolves explicitly
configured tool names; preserve the existing allow-list membership check while
ensuring dynamic names such as constructor, toString, and valueOf cannot resolve
inherited values or cause allowList.has to throw.
In `@src/core/tools/error-interception/ToolErrorInterceptor.ts`:
- Around line 298-315: Make TaskErrorState the single owner of occurrence
counters and circuit status. In
src/core/tools/error-interception/ToolErrorInterceptor.ts lines 298-315, update
incrementAndGetCount to delegate to
getTaskErrorState(task).incrementOccurrence(category), check
TaskErrorState.isOpen(category) instead of taskState.shellCircuitOpen, and
import the shared threshold rather than declaring SHELL_CIRCUIT_THRESHOLD. In
src/core/tools/error-interception/TaskErrorState.ts lines 20-111, retain
ownership of counters, fingerprints, and circuit state, and export
STUCK_LOOP_THRESHOLD for both modules.
- Around line 221-234: Update the JSON parsing logic in the string-content
branch to accept parsed results only when they are non-null objects; otherwise
set the result to { text: content }. Keep the existing object shape and metadata
handling in commonSignal unchanged, and adjust the parsed value handling around
JSON.parse rather than asserting every JSON value matches the expected
structure.
---
Nitpick comments:
In `@src/core/tools/error-interception/__tests__/ErrorClassifier.spec.ts`:
- Around line 890-896: Add a test in the classifyToolResult suite that invokes
classifyToolResult with only the structured result and taskId, omitting
toolCallId, and assert the expected classification and facts for that context.
Keep the existing toolCallId test unchanged.
In `@src/core/tools/error-interception/__tests__/StructuralValidator.spec.ts`:
- Around line 101-109: Update the tests around validateNestedParams to exercise
detectToolSignature’s multi-known-keys heuristic with two KNOWN_PARAMETER_KEYS
that do not match an explicit TOOL_SIGNATURE_KEY_SETS entry, and assert the
signal is detected. Rename and revise the single-key test to use an actual known
parameter key while preserving the expectation that it is ignored for a non-tool
signature.
- Around line 150-168: Update the recursion and node-bound tests around
validateNestedParams so each fixture includes a recognizable tool signature
beyond its respective NESTED_DETECTION_MAX_DEPTH or NESTED_DETECTION_MAX_NODES
limit, causing validation to return the expected signal when limits are
enforced. Add a positive control with the same signature within each limit and
assert detection, ensuring the tests fail if either bound is removed.
In `@src/core/tools/error-interception/__tests__/TaskErrorState.spec.ts`:
- Around line 88-120: Add a “pending native protocol guide” test suite covering
TaskErrorState.getPendingNativeProtocolGuide returning undefined initially,
setPendingNativeProtocolGuide followed by consumePendingNativeProtocolGuide
returning the guide only once, clearPendingNativeProtocolGuide removing the
guide, and reset() preserving a queued guide.
In `@src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts`:
- Line 296: Update the test title in the circuit-opening test to reference the
exported SHELL_CIRCUIT_THRESHOLD constant name instead of
SHELL_INTEGRATION_THRESHOLD, matching the constant used by the test body.
- Around line 842-845: Update the three tests around decoratedPushToolResult to
assert the payload passed to pushToolResult, not just that it was called once.
Verify the expected transformed content for the successful case and the original
content for the fail-open cases, preserving the single-call assertion if useful.
- Around line 15-16: Remove the avoidable double assertions in
src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts:15-16
by using typed vi.fn<HandleError>() and vi.fn<PushToolResult>() and deleting the
now-unused MockHandleError and MockPushToolResult aliases. At
src/core/tools/error-interception/__tests__/ToolErrorInterceptor.spec.ts:752-753
and line 771, pass string literals directly where ToolResponse accepts string;
leave the documented assertions at lines 611-614 unchanged.
In `@src/core/tools/error-interception/ErrorClassifier.ts`:
- Around line 20-27: The blacklist in isValidIdentifier is unreachable under
SAFE_IDENTIFIER_RE; retain it only as explicit defense-in-depth and update the
nearby comment to state that intent, without changing validation behavior.
- Around line 240-248: Update the catch-all handling in the error-classification
function to locate the UNCLASSIFIED pattern by its stable category or id instead
of ERROR_PATTERNS array position. Preserve the existing fallback response and
ensure a defined fallback remains available if the expected pattern cannot be
found, so ToolErrorInterceptor.transformSignal continues to fail open.
- Around line 132-148: Bound the input inside tryExtractParamNameFromText before
running any regex matches, using a reasonable fixed-length prefix of the
untrusted text. Apply all three existing patterns to that bounded value,
preserving extraction behavior for parameter names near the beginning while
preventing scans and backtracking across entire file contents or long command
output.
- Around line 169-173: Update the primitive-string array handling in
ErrorClassifier’s facts construction to store a shallow copy of value rather
than the caller-provided array, including the equivalent handling at the other
occurrence. Preserve the existing validation and ensure the copied arrays cannot
mutate the frozen facts through the original metadata reference.
In `@src/core/tools/error-interception/errorPatterns.ts`:
- Line 1: Standardize the relative import specifiers across the
error-interception module, including the `ErrorPattern`, `InterceptionSignal`,
and `RecoveryDisposition` import in `errorPatterns.ts`, to use one consistent
extension style. Align `StructuralValidator.ts` and the mixed imports in
`index.ts` with the chosen module-wide convention.
In `@src/core/tools/error-interception/MessageTransformer.ts`:
- Around line 99-104: Simplify isRetryable by removing the redundant retryPolicy
=== "auto-recover" branch and preserving the existing behavior: excluded
categories and "do-not-retry" return false, while all other policies return
true.
- Around line 258-270: Replace the unchecked cast in the returned object’s
`type` field with a type guard that validates the destructured
`facts["errorSource"]` against the known `ErrorSource` values, returning
`undefined` for invalid strings. Reuse the `facts` value destructured near line
218 and preserve the existing `payloadType` behavior for valid sources.
In `@src/core/tools/error-interception/TaskErrorState.ts`:
- Around line 34-59: Update TaskErrorState to import ErrorCategory from ./types
and change perCategory, getOrCreate, and getOccurrence category types from
string to ErrorCategory. Use ErrorCategory consistently for all category
parameters so callers cannot store keys outside the categories handled by
resetTaskState.
In `@src/core/tools/error-interception/ToolErrorInterceptor.ts`:
- Around line 286-291: Replace the console.warn call in the unclassified-error
branch of ToolErrorInterceptor with the project logger’s debug-level method.
Preserve the existing message context while passing toolName and patternId as
structured fields, and keep the raw error text excluded.
- Around line 98-111: Export the existing isWeakMapKey helper from
TaskErrorState.ts and replace the duplicated type guard in
ToolErrorInterceptor.getTaskState with a call to that shared helper. Preserve
the current fail-open behavior by returning a fresh default state for invalid
keys, while continuing to store and return task-specific state for valid WeakMap
keys.
In `@src/core/tools/error-interception/types.ts`:
- Around line 87-94: Rename the object-shaped ToolResponse interface in the
error-interception types module to StructuredToolResult, update
InterceptionSignal.result to use the renamed type, and replace the ToolResponse
barrel export in index.ts with StructuredToolResult while preserving the
existing fields and exports.
🪄 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: ae796f3c-4382-41ba-9438-cfafdfd821a5
📒 Files selected for processing (14)
codecov.ymlsrc/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
| comment: | ||
| layout: "diff, flags, components" | ||
| behavior: default | ||
| coverage: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Convert the file to LF line endings.
YAMLlint reports wrong new line character: expected \n at line 1. Normalize codecov.yml to LF line endings to clear the lint error.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
🤖 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 `@codecov.yml` at line 1, Normalize the line endings in codecov.yml from CRLF
to LF so the file uses \n line endings and YAMLlint passes.
Source: Linters/SAST tools
| default: | ||
| informational: true # patch coverage is advisory, not blocking | ||
| webview-patch: | ||
| informational: true # patch coverage is advisory, not blocking | ||
| flags: | ||
| - webview-ui | ||
| - webview-ui-ct |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the documented webview patch-coverage policy.
webview-ui/AGENTS.md lines 35-40 still state that modified webview-ui/src/ lines require a 70% patch gate. This configuration makes webview-patch informational. Update the document so contributors do not rely on an obsolete blocking requirement.
🤖 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 `@codecov.yml` around lines 16 - 22, Update the webview-patch coverage policy
documented in webview-ui/AGENTS.md to reflect that coverage is informational
rather than a blocking 70% gate. Replace the obsolete requirement for modified
webview-ui/src/ lines while preserving the surrounding contributor guidance.
| const nextSection = message.match(/Next:\n((?:\d+\..+\n?)+)/) | ||
| expect(nextSection).toBeDefined() | ||
| const items = nextSection![1] | ||
| .trim() | ||
| .split("\n") | ||
| .filter((l) => l.trim().length > 0) | ||
| expect(items.length).toBeLessThanOrEqual(3) | ||
| for (const item of items) { | ||
| // Each line is "N. <text>" — strip the prefix for length check | ||
| const text = item.replace(/^\d+\.\s/, "") | ||
| expect(text.length).toBeLessThanOrEqual(160) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
toBeDefined() cannot fail here; use not.toBeNull().
String.prototype.match returns null when there is no match. expect(null).toBeDefined() passes, so Line 95 asserts nothing, and Line 96 then throws on the non-null assertion. Also import NEXT_ITEM_COUNT_LIMIT and NEXT_ITEM_CHAR_LIMIT instead of the literals 3 and 160, so the test tracks the constants.
💚 Proposed fix
const nextSection = message.match(/Next:\n((?:\d+\..+\n?)+)/)
- expect(nextSection).toBeDefined()
+ expect(nextSection).not.toBeNull()
const items = nextSection![1]
.trim()
.split("\n")
.filter((l) => l.trim().length > 0)
- expect(items.length).toBeLessThanOrEqual(3)
+ expect(items.length).toBeLessThanOrEqual(NEXT_ITEM_COUNT_LIMIT)
for (const item of items) {
// Each line is "N. <text>" — strip the prefix for length check
const text = item.replace(/^\d+\.\s/, "")
- expect(text.length).toBeLessThanOrEqual(160)
+ expect(text.length).toBeLessThanOrEqual(NEXT_ITEM_CHAR_LIMIT)
}🤖 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 - 105, Update the MessageTransformer test’s nextSection
assertion to use not.toBeNull(), matching String.match’s null failure value
before accessing nextSection[1]. Import and use NEXT_ITEM_COUNT_LIMIT and
NEXT_ITEM_CHAR_LIMIT in place of the literal item-count and character-length
limits.
| export function classifyToolResult( | ||
| result: InterceptionSignal["result"], | ||
| taskId: string, | ||
| toolCallId?: string, | ||
| ): ErrorClassification { | ||
| const metadata: Record<string, unknown> = {} | ||
| if (result && typeof result === "object") { | ||
| if (result.status) metadata.status = result.status | ||
| if (result.type) metadata.type = result.type | ||
| } | ||
|
|
||
| const signal: InterceptionSignal = { | ||
| source: "tool_result", | ||
| stage: "result", | ||
| taskId, | ||
| toolCallId, | ||
| result: result ?? undefined, | ||
| metadata, | ||
| } | ||
| return classifyError(signal) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
classifyToolResult cannot match tool-bound patterns when toolCallId is omitted.
The signal built at Lines 263-270 sets no toolName, and toolCallId is optional. hasToolContext then returns false, isEligible rejects every pattern that sets requiresToolContext, and the function returns UNCLASSIFIED. ToolErrorInterceptor.transformToolResult forwards an optional toolCallId, so a caller that passes only taskId silently receives no guidance. Accept an optional toolName and forward it.
🐛 Proposed fix
export function classifyToolResult(
result: InterceptionSignal["result"],
taskId: string,
toolCallId?: string,
+ toolName?: string,
): ErrorClassification {
@@
const signal: InterceptionSignal = {
source: "tool_result",
stage: "result",
taskId,
toolCallId,
+ toolName,
result: result ?? undefined,
metadata,
}Then forward toolName from ToolErrorInterceptor.transformToolResult options.
🤖 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 252 - 272,
Update classifyToolResult to accept an optional toolName parameter and include
it in the InterceptionSignal so tool-bound patterns can match when toolCallId is
absent. Update ToolErrorInterceptor.transformToolResult to forward toolName from
its options while preserving existing taskId and toolCallId behavior.
| // Default inference from retryPolicy and category. | ||
| if (occurrence >= 3) return "change_strategy" | ||
|
|
||
| if (category === "DUPLICATE_CALL") return "discard_duplicate" | ||
| if (category === "INVALID_TOOL_PROTOCOL") return "discard_duplicate" | ||
|
|
||
| if (retryPolicy === "do-not-retry") return "discard_duplicate" | ||
| if (retryPolicy === "auto-recover") return "correct_once" | ||
| if (retryPolicy === "alternate-tool") return "correct_once" | ||
| // correct-and-retry | ||
| return "correct_once" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List the RecoveryDisposition union and every pattern with a do-not-retry policy.
fd -t f 'types.ts' src/core/tools/error-interception --exec rg -n -A 12 'RecoveryDisposition'
fd -t f 'errorPatterns.ts' src/core/tools/error-interception --exec rg -n -B 6 'do-not-retry'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3448
🏁 Script executed:
#!/bin/bash
# Inspect the relevant code paths and recovery disposition usages.
sed -n '1,260p' src/core/tools/error-interception/MessageTransformer.ts
printf '\n--- usages of selectRecoveryDisposition/dispositions ---\n'
rg -n 'selectRecoveryDisposition|recoveryDisposition|recovery_disposition|discard_duplicate|await_user|change_strategy|correct_once' src/core/tools -g '*.ts' -g '*.tsx'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 18827
🏁 Script executed:
#!/bin/bash
# Inspect the relevant code paths and recovery disposition usages.
sed -n '1,260p' src/core/tools/error-interception/MessageTransformer.ts
printf '\n--- usages of selectRecoveryDisposition/dispositions ---\n'
rg -n 'selectRecoveryDisposition|recoveryDisposition|recovery_disportation|discard_duplicate|await_user|change_strategy|correct_once' src/core/tools -g '*.ts' -g '*.tsx'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 18503
🏁 Script executed:
#!/bin/bash
# Inspect RecoveryDisposition semantics and the do-no-retry patterns' category/metadata.
sed -n '35,55p' src/core/tools/error-interception/types.ts
printf '\n--- relevant do-not-retry pattern entries ---\n'
sed -n '50,80p;105,125p;130,170p;170,185p;210,220p;326,336p;570,610p;700,716p' src/core/tools/error-interception/errorPatterns.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 8695
Avoid discard_duplicate for non-duplicate failures.
Line 210 maps every non-DUPLICATE_CALL/non-INVALID_TOOL_PROTOCOL do-not-retry category to discard_duplicate, including TOOL_NOT_FOUND, MODE_RESTRICTION, FILE_RESTRICTION, and UNCLASSIFIED. That disposition says “Do not resend the malformed sibling,” so the guidance calls a missing tool or mode/path restriction a duplicate call. Map these categories to change_strategy/await_user or add a new disposition that expresses “do not retry this invocation; continue the task.”
🤖 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 204 -
215, The retryPolicy handling in the disposition inference must not map
non-duplicate failures to discard_duplicate. Update the logic around the
retryPolicy === "do-not-retry" branch so TOOL_NOT_FOUND, MODE_RESTRICTION,
FILE_RESTRICTION, and UNCLASSIFIED resolve to an appropriate change_strategy or
await_user disposition, while preserving discard_duplicate for actual
duplicate/protocol failures.
| export function validateNestedParams(args: Record<string, unknown>, toolName: string): InterceptionSignal | null { | ||
| const allowList = OBJECT_ALLOWED_PARAMETERS[toolName] | ||
| for (const [key, value] of Object.entries(args)) { | ||
| if (allowList && allowList.has(key)) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the allow-list lookup against prototype keys.
OBJECT_ALLOWED_PARAMETERS is a plain object literal, so the index access on line 251 resolves inherited members. toolName can be a dynamic MCP tool name, and types.ts line 69 documents that. If toolName is "constructor", "toString", or "valueOf", allowList becomes a truthy function and allowList.has(key) on line 253 throws TypeError: allowList.has is not a function. That aborts preflight validation for the tool call. Use a Map, or a prototype-less record, so only own keys resolve.
🛡️ Proposed fix using a `Map`
-const OBJECT_ALLOWED_PARAMETERS: Readonly<Record<string, ReadonlySet<string>>> = {
- read_file: new Set(["indentation"]),
- use_mcp_tool: new Set(["arguments"]),
-}
+const OBJECT_ALLOWED_PARAMETERS: ReadonlyMap<string, ReadonlySet<string>> = new Map([
+ ["read_file", new Set(["indentation"])],
+ ["use_mcp_tool", new Set(["arguments"])],
+]) export function validateNestedParams(args: Record<string, unknown>, toolName: string): InterceptionSignal | null {
- const allowList = OBJECT_ALLOWED_PARAMETERS[toolName]
+ const allowList = OBJECT_ALLOWED_PARAMETERS.get(toolName)
for (const [key, value] of Object.entries(args)) {
- if (allowList && allowList.has(key)) {
+ if (allowList?.has(key)) {
continue
}📝 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.
| export function validateNestedParams(args: Record<string, unknown>, toolName: string): InterceptionSignal | null { | |
| const allowList = OBJECT_ALLOWED_PARAMETERS[toolName] | |
| for (const [key, value] of Object.entries(args)) { | |
| if (allowList && allowList.has(key)) { | |
| continue | |
| } | |
| export function validateNestedParams(args: Record<string, unknown>, toolName: string): InterceptionSignal | null { | |
| const allowList = OBJECT_ALLOWED_PARAMETERS.get(toolName) | |
| for (const [key, value] of Object.entries(args)) { | |
| if (allowList?.has(key)) { | |
| continue | |
| } |
🤖 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 250 -
255, Change OBJECT_ALLOWED_PARAMETERS to a Map or prototype-less record so
validateNestedParams only resolves explicitly configured tool names; preserve
the existing allow-list membership check while ensuring dynamic names such as
constructor, toString, and valueOf cannot resolve inherited values or cause
allowList.has to throw.
| if (typeof content === "string") { | ||
| let parsed: { status?: string; type?: string; error?: unknown } | undefined | ||
| try { | ||
| parsed = JSON.parse(content) as { status?: string; type?: string; error?: unknown } | ||
| } catch { | ||
| parsed = undefined | ||
| } | ||
| const signal = commonSignal({ | ||
| result: parsed ?? { text: content }, | ||
| metadata: { | ||
| ...options.metadata, | ||
| hasErrorResult: true, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the JSON.parse result before you treat it as an object.
JSON.parse returns any JSON value. For content such as 123, "text", true, or […], parsed is not an object, yet Line 224 asserts the shape { status?, type?, error? } and Line 229 passes it as result. Accept the parsed value only when it is a non-null object; otherwise keep the { text: content } shape.
🐛 Proposed fix
- let parsed: { status?: string; type?: string; error?: unknown } | undefined
- try {
- parsed = JSON.parse(content) as { status?: string; type?: string; error?: unknown }
- } catch {
- parsed = undefined
- }
+ let parsed: { status?: string; type?: string; error?: unknown } | undefined
+ try {
+ const raw: unknown = JSON.parse(content)
+ parsed =
+ typeof raw === "object" && raw !== null && !Array.isArray(raw)
+ ? (raw as { status?: string; type?: string; error?: unknown })
+ : undefined
+ } catch {
+ parsed = 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.
| if (typeof content === "string") { | |
| let parsed: { status?: string; type?: string; error?: unknown } | undefined | |
| try { | |
| parsed = JSON.parse(content) as { status?: string; type?: string; error?: unknown } | |
| } catch { | |
| parsed = undefined | |
| } | |
| const signal = commonSignal({ | |
| result: parsed ?? { text: content }, | |
| metadata: { | |
| ...options.metadata, | |
| hasErrorResult: true, | |
| }, | |
| }) | |
| if (typeof content === "string") { | |
| let parsed: { status?: string; type?: string; error?: unknown } | undefined | |
| try { | |
| const raw: unknown = JSON.parse(content) | |
| parsed = | |
| typeof raw === "object" && raw !== null && !Array.isArray(raw) | |
| ? (raw as { status?: string; type?: string; error?: unknown }) | |
| : undefined | |
| } catch { | |
| parsed = undefined | |
| } | |
| const signal = commonSignal({ | |
| result: parsed ?? { text: content }, | |
| metadata: { | |
| ...options.metadata, | |
| hasErrorResult: 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 221 -
234, Update the JSON parsing logic in the string-content branch to accept parsed
results only when they are non-null objects; otherwise set the result to { text:
content }. Keep the existing object shape and metadata handling in commonSignal
unchanged, and adjust the parsed value handling around JSON.parse rather than
asserting every JSON value matches the expected structure.
| const occurrence = this.incrementAndGetCount(task, taskState, classification.category) | ||
|
|
||
| if (classification.category === "SHELL_INTEGRATION" && occurrence >= SHELL_CIRCUIT_THRESHOLD) { | ||
| taskState.shellCircuitOpen = true | ||
| return CIRCUIT_OPEN_DETAILS | ||
| } | ||
|
|
||
| return transformErrorToMessage(classification, { occurrence }) | ||
| } | ||
|
|
||
| /** | ||
| * Increments the per-category counter and returns the new occurrence count. | ||
| */ | ||
| private incrementAndGetCount(task: object, taskState: InterceptorTaskState, category: ErrorCategory): number { | ||
| const next = (taskState.categoryCounts.get(category) ?? 0) + 1 | ||
| taskState.categoryCounts.set(category, next) | ||
| return next | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Two per-task stores hold the same occurrence and circuit state. InterceptorTaskState.categoryCounts and TaskErrorState.perCategory both track per-category occurrences, and both track a circuit flag. transformSignal increments only the interceptor store, so TaskErrorState counters stay at 0 and TaskErrorState.isOpen never becomes true, even though resetTaskState clears both. SHELL_CIRCUIT_THRESHOLD and STUCK_LOOP_THRESHOLD also duplicate the value 3 in two modules. Pick one store as the owner of occurrence and circuit state.
src/core/tools/error-interception/ToolErrorInterceptor.ts#L298-L315: makeincrementAndGetCountdelegate togetTaskErrorState(task).incrementOccurrence(category), and read the circuit flag fromTaskErrorState.isOpen(category)instead oftaskState.shellCircuitOpen. Import the shared threshold rather than declaringSHELL_CIRCUIT_THRESHOLD.src/core/tools/error-interception/TaskErrorState.ts#L20-L111: keep this class as the single owner of occurrence counters, fingerprints, and per-category circuit status, and exportSTUCK_LOOP_THRESHOLDas the one threshold both modules use.
📍 Affects 2 files
src/core/tools/error-interception/ToolErrorInterceptor.ts#L298-L315(this comment)src/core/tools/error-interception/TaskErrorState.ts#L20-L111
🤖 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 298 -
315, Make TaskErrorState the single owner of occurrence counters and circuit
status. In src/core/tools/error-interception/ToolErrorInterceptor.ts lines
298-315, update incrementAndGetCount to delegate to
getTaskErrorState(task).incrementOccurrence(category), check
TaskErrorState.isOpen(category) instead of taskState.shellCircuitOpen, and
import the shared threshold rather than declaring SHELL_CIRCUIT_THRESHOLD. In
src/core/tools/error-interception/TaskErrorState.ts lines 20-111, retain
ownership of counters, fingerprints, and circuit state, and export
STUCK_LOOP_THRESHOLD for both modules.
b27c707 to
367d4b6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
Adds interceptor, message transformer, structural validator, task-scoped occurrence/error state, and original error fallback using B01 contracts. Does not change
presentAssistantMessage.ts.Included Files
src/core/tools/error-interception/ToolErrorInterceptor.tssrc/core/tools/error-interception/StructuralValidator.tssrc/core/tools/error-interception/TaskErrorState.tssrc/core/tools/error-interception/MessageTransformer.tsExclusion Scope
src/core/assistant-message/presentAssistantMessage.tsand B03 integration testSummary by CodeRabbit
New Features
Bug Fixes
Tests