Harden Code Mode runtime contracts#668
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change centralizes MCP schema validation, adds validated AI thread tool execution and orchestration normalization, introduces a shared AI tool registry, updates compaction and telemetry handling, and propagates structured tool presentation metadata through chat activity rendering. ChangesMCP operation validation
AI thread tooling modernization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AIThreadRuntime
participant OrchestrationTool
participant NestedToolConnector
participant OutcomeNormalizer
AIThreadRuntime->>OrchestrationTool: execute orchestration
OrchestrationTool->>NestedToolConnector: expose and execute nested tools
NestedToolConnector-->>OrchestrationTool: return nested results
OrchestrationTool->>OutcomeNormalizer: normalize orchestration output
OutcomeNormalizer-->>AIThreadRuntime: return calls and aggregated outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Greptile SummaryThis PR hardens Code Mode and AI tool runtime contracts. The main changes are:
Confidence Score: 5/5This PR appears safe to merge with low risk. The changed runtime seams validate inputs and outputs, normalize malformed orchestration results into errors, and include targeted tests for the main contract, telemetry, UI, and compaction paths. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (1): Last reviewed commit: "refactor(ai-chat): centralize tool prese..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6212b450d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| execute: async (args, context) => { | ||
| return runtime.execute(args, { | ||
| codemodeExecutionId: context?.executionId, | ||
| invocationId: crypto.randomUUID(), |
There was a problem hiding this comment.
When Code Mode invokes a nested workspace mutation more than once for the same logical connector call, this generates a fresh invocationId each time. The workspace tools pass that value through as the operation ID, and several mutations use it as their clientMutationId, so recovery/retry after the side effect but before the Code Mode result is durably logged can no longer be deduped and may apply the mutation again or turn the retry into a conflict. Derive the ID from the Code Mode execution/call identity instead of fresh randomness.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/ai/ai-thread-orchestration-contract.ts">
<violation number="1" location="src/features/workspaces/ai/ai-thread-orchestration-contract.ts:122">
P2: `getOrchestrationCallOutcome` treats the `"executing"` state as an error outcome (`failureCodes: ["codemode_tool_error"], failedCount: 1, status: "error"`), while `getOrchestrationCallStatus` maps the same `"executing"` state to `status: "running"`. This produces a self-contradictory normalized call where `state: "executing"`, `status: "running"`, but `outcome: { status: "error", failureCodes: ["codemode_tool_error"], ... }`. If Code Mode ever includes an executing-session call in a finalized output, consumers would see a call reported as still `"running"` while the outcome claims `"error"`.
Consider adding an explicit `"executing"` branch to `getOrchestrationCallOutcome` that returns a neutral outcome (e.g., `{ failureCodes: [], failedCount: 0, status: "success" }` or adding `"executing"` to the state→status mapping so both functions agree.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const childOutcome = aggregateAIToolOutcomes(calls.map((call) => call.outcome)); | ||
|
|
||
| if (parsed.data.status === "completed") { | ||
| return { |
There was a problem hiding this comment.
P2: getOrchestrationCallOutcome treats the "executing" state as an error outcome (failureCodes: ["codemode_tool_error"], failedCount: 1, status: "error"), while getOrchestrationCallStatus maps the same "executing" state to status: "running". This produces a self-contradictory normalized call where state: "executing", status: "running", but outcome: { status: "error", failureCodes: ["codemode_tool_error"], ... }. If Code Mode ever includes an executing-session call in a finalized output, consumers would see a call reported as still "running" while the outcome claims "error".
Consider adding an explicit "executing" branch to getOrchestrationCallOutcome that returns a neutral outcome (e.g., { failureCodes: [], failedCount: 0, status: "success" } or adding "executing" to the state→status mapping so both functions agree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/ai/ai-thread-orchestration-contract.ts, line 122:
<comment>`getOrchestrationCallOutcome` treats the `"executing"` state as an error outcome (`failureCodes: ["codemode_tool_error"], failedCount: 1, status: "error"`), while `getOrchestrationCallStatus` maps the same `"executing"` state to `status: "running"`. This produces a self-contradictory normalized call where `state: "executing"`, `status: "running"`, but `outcome: { status: "error", failureCodes: ["codemode_tool_error"], ... }`. If Code Mode ever includes an executing-session call in a finalized output, consumers would see a call reported as still `"running"` while the outcome claims `"error"`.
Consider adding an explicit `"executing"` branch to `getOrchestrationCallOutcome` that returns a neutral outcome (e.g., `{ failureCodes: [], failedCount: 0, status: "success" }` or adding `"executing"` to the state→status mapping so both functions agree.</comment>
<file context>
@@ -0,0 +1,265 @@
+ const childOutcome = aggregateAIToolOutcomes(calls.map((call) => call.outcome));
+
+ if (parsed.data.status === "completed") {
+ return {
+ status: parsed.data.status,
+ executionId: parsed.data.executionId,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/workspaces/components/ai-chat/ai-chat-display-state.ts (1)
184-206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLegacy codemode children bypass hidden-tool filtering.
getCodemodeCallActivities(the compact/call-log path) filters children bypresentation.visibility === "visible", butgetLegacyCodemodeChildren— the fallback used whenevercodemodePart.outputisn't in that compact shape — does not. Hidden tools likesandbox_bash,time_get_current,time_calculate_relative, andworkspace_link_itemswill still surface as children in legacy-rendered threads, contradicting the intended "filter hidden nested tools" behavior for those tools, and this gap isn't covered by the new registry-visibility test either.🐛 Proposed fix
function getLegacyCodemodeChildren( parts: AiChatMessagePart[], codemodePart: AiChatToolPart, ): AiChatToolChildActivity[] { return parts.flatMap((part) => { if (!isToolUIPart(part) || part === codemodePart || !isVisibleToolPart(part)) { return []; } const activity = getToolActivityForPart(part); - return activity + return activity && activity.presentation.visibility === "visible" ? [ { id: part.toolCallId, presentation: activity.presentation, status: activity.status, summary: activity.summary, toolName: activity.toolName, }, ] : []; }); }🤖 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/features/workspaces/components/ai-chat/ai-chat-display-state.ts` around lines 184 - 206, Update getLegacyCodemodeChildren to exclude child activities whose presentation visibility is not "visible", matching the filtering behavior in getCodemodeCallActivities. Apply this check after obtaining each tool activity and preserve the existing empty-result behavior for hidden or unavailable activities.
🧹 Nitpick comments (3)
src/features/workspaces/ai/ai-compaction.test.ts (1)
61-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a standard
tool-resultpart to the test fixture.The test currently verifies
dynamic-toolparts. It would be beneficial to add a native AI SDKtool-resultpart (which doesn't have anoutputfield) to ensure the adapter logic consistently stringifies standard outputs as well.🛠️ Proposed test addition
{ output: undefined, result: { accepted: true }, toolCallId: "legacy-tool-call", toolName: "legacy_tool", type: "dynamic-tool", }, + { + result: { nested: "object" }, + toolCallId: "standard-tool-call", + toolName: "standard_tool", + type: "tool-result", + }, ], role: "assistant", }, message("middle", [{ type: "text", text: "middle" }]), message("tail", [{ type: "text", text: "tail" }]), ] as never); expect(prompt).toContain('Input: {"path":"/workspace/report"}'); expect(prompt).toContain('Output: {"content":"'); expect(prompt).toContain('Output: {"accepted":true}'); + expect(prompt).toContain('{"nested":"object"}'); expect(prompt).not.toContain("[object Object]");🤖 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/features/workspaces/ai/ai-compaction.test.ts` around lines 61 - 76, Extend the fixture in the compaction test around the existing dynamic-tool part with a native AI SDK tool-result part that has no output field, then add an assertion for its stringified result in prompt. Keep the existing dynamic-tool assertions and ensure the new case verifies standard tool-result handling without producing “[object Object]”.src/features/workspaces/components/ai-chat/AiChatToolActivityRow.tsx (1)
299-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: make the icon switch exhaustive.
The
defaultbranch is currently unreachable givenAiToolActivityIconKind's closed union, so a future addition to that union would silently renderGlobe2instead of failing a type check.♻️ Optional exhaustiveness guard
function ToolActivityIcon({ icon }: { icon: AiToolActivityIconKind }) { switch (icon) { case "code": return <Code2 className="size-3.5" aria-hidden="true" />; case "edit": return <PencilLine className="size-3.5" aria-hidden="true" />; case "file": return <FileText className="size-3.5" aria-hidden="true" />; case "search": return <Search className="size-3.5" aria-hidden="true" />; case "web": return <Globe2 className="size-3.5" aria-hidden="true" />; - default: - return <Globe2 className="size-3.5" aria-hidden="true" />; + default: { + icon satisfies never; + return <Globe2 className="size-3.5" aria-hidden="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/features/workspaces/components/ai-chat/AiChatToolActivityRow.tsx` around lines 299 - 313, Make the switch in ToolActivityIcon exhaustive for the AiToolActivityIconKind union by removing the fallback Globe2 default and adding the project’s standard unreachable-case or exhaustive-check pattern. Ensure any future union member causes a type-check failure rather than silently rendering an unrelated icon.src/features/workspaces/ai/ai-thread-orchestration-contract.ts (1)
190-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
invalid_orchestration_resultsentinel across two files. Both files independently hard-code the identical{failureCodes: ["invalid_orchestration_result"], failedCount: 1, status: "error"}literal for the "invalid orchestration output" case; a future change to this sentinel is easy to apply in only one place and silently diverge.
src/features/workspaces/ai/ai-thread-orchestration-contract.ts#L190-L196: exportinvalidOutcome()(or a similarly named constant) from this module instead of keeping it file-private.src/features/workspaces/ai/ai-tool-outcome.ts#L24-L28: import and reuse the exported sentinel fromai-thread-orchestration-contract.tsinstead of re-declaring the literal inline.🤖 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/features/workspaces/ai/ai-thread-orchestration-contract.ts` around lines 190 - 196, Export the invalidOutcome sentinel from ai-thread-orchestration-contract.ts, preserving its existing value. In src/features/workspaces/ai/ai-tool-outcome.ts lines 24-28, import and reuse invalidOutcome instead of declaring the duplicate literal inline.
🤖 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/features/workspaces/ai/ai-compaction.ts`:
- Around line 89-100: Update the mapping logic in ai-compaction to stringify
both native result and output fields on tool-result parts. Preserve the existing
value selection and unchanged-part behavior, but when a value exists, overwrite
result alongside output with stringifyForCompaction so downstream summarization
cannot render objects as [object Object].
In `@src/features/workspaces/ai/ai-thread-orchestration-contract.ts`:
- Around line 233-251: The getOrchestrationCallOutcome function currently treats
executing as an error. Add an executing branch that returns the non-error
running outcome expected by aggregateAIToolOutcomes, and add regression coverage
for both executing and paused orchestration paths while preserving existing
applied, pending, reverted, and error behavior.
In `@src/features/workspaces/ai/ai-thread-runtime.ts`:
- Around line 240-251: Add registry coverage in ai-tool-registry.test.ts that
invokes each tool factory used by addAIThreadToolEntries—createSandboxTools,
createAIThreadCodeRunTools, createAIThreadWebTools, createAIThreadResearchTools,
createAIThreadTimeTools, and createAIThreadWorkspaceTools—with minimal or mocked
inputs, then verifies requireAiToolDefinition does not throw for every returned
tool key. Keep the test focused on detecting missing AI_TOOL_REGISTRY entries
during CI.
---
Outside diff comments:
In `@src/features/workspaces/components/ai-chat/ai-chat-display-state.ts`:
- Around line 184-206: Update getLegacyCodemodeChildren to exclude child
activities whose presentation visibility is not "visible", matching the
filtering behavior in getCodemodeCallActivities. Apply this check after
obtaining each tool activity and preserve the existing empty-result behavior for
hidden or unavailable activities.
---
Nitpick comments:
In `@src/features/workspaces/ai/ai-compaction.test.ts`:
- Around line 61-76: Extend the fixture in the compaction test around the
existing dynamic-tool part with a native AI SDK tool-result part that has no
output field, then add an assertion for its stringified result in prompt. Keep
the existing dynamic-tool assertions and ensure the new case verifies standard
tool-result handling without producing “[object Object]”.
In `@src/features/workspaces/ai/ai-thread-orchestration-contract.ts`:
- Around line 190-196: Export the invalidOutcome sentinel from
ai-thread-orchestration-contract.ts, preserving its existing value. In
src/features/workspaces/ai/ai-tool-outcome.ts lines 24-28, import and reuse
invalidOutcome instead of declaring the duplicate literal inline.
In `@src/features/workspaces/components/ai-chat/AiChatToolActivityRow.tsx`:
- Around line 299-313: Make the switch in ToolActivityIcon exhaustive for the
AiToolActivityIconKind union by removing the fallback Globe2 default and adding
the project’s standard unreachable-case or exhaustive-check pattern. Ensure any
future union member causes a type-check failure rather than silently rendering
an unrelated icon.
🪄 Autofix (Beta)
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
Run ID: b1b2efa6-e176-4c5e-9f28-361d680a2e02
📒 Files selected for processing (29)
src/features/mcp/mcp-operation-catalog.tssrc/features/mcp/mcp-operation-catalog.worker.test.tssrc/features/workspaces/ai/ai-codemode-types.worker.test.tssrc/features/workspaces/ai/ai-compaction.test.tssrc/features/workspaces/ai/ai-compaction.tssrc/features/workspaces/ai/ai-thread-orchestration-contract.tssrc/features/workspaces/ai/ai-thread-orchestration.tssrc/features/workspaces/ai/ai-thread-orchestration.worker.test.tssrc/features/workspaces/ai/ai-thread-posthog-recorder.tssrc/features/workspaces/ai/ai-thread-runtime.tssrc/features/workspaces/ai/ai-thread-tool.test.tssrc/features/workspaces/ai/ai-thread-tool.tssrc/features/workspaces/ai/ai-thread.tssrc/features/workspaces/ai/ai-tool-outcome.tssrc/features/workspaces/ai/ai-tool-outcome.worker.test.tssrc/features/workspaces/ai/ai-tool-presentation.tssrc/features/workspaces/ai/ai-tool-registry.test.tssrc/features/workspaces/ai/ai-tool-registry.tssrc/features/workspaces/ai/code-run-tools.tssrc/features/workspaces/ai/research-tools.tssrc/features/workspaces/ai/time-tools.tssrc/features/workspaces/ai/web-tools.tssrc/features/workspaces/ai/workspace-tools.tssrc/features/workspaces/components/ai-chat/AiChatToolActivityRow.tsxsrc/features/workspaces/components/ai-chat/ai-chat-codemode-activity.tssrc/features/workspaces/components/ai-chat/ai-chat-display-state.test.tssrc/features/workspaces/components/ai-chat/ai-chat-display-state.tssrc/integrations/firecrawl/research.tssrc/integrations/firecrawl/search.ts
💤 Files with no reviewable changes (1)
- src/features/workspaces/ai/ai-tool-presentation.ts
| const output = | ||
| ("output" in part ? part.output : undefined) ?? | ||
| ("result" in part ? part.result : undefined); | ||
| if (output === undefined) { | ||
| return part; | ||
| } | ||
|
|
||
| return { | ||
| ...part, | ||
| output: stringifyForCompaction(output), | ||
| }; | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure both result and output properties are stringified to prevent [object Object] rendering.
For standard AI SDK tool-result parts, this logic adds a new stringified output property but leaves the existing result property as an unstringified object. If the downstream agents summarizer formats tool-result parts by reading their native result field, it will encounter the original object and still render [object Object].
To fully adapt the results without relying on how agents extracts the value, overwrite result in place alongside output.
🐛 Proposed fix
- const output =
- ("output" in part ? part.output : undefined) ??
- ("result" in part ? part.result : undefined);
- if (output === undefined) {
- return part;
- }
-
- return {
- ...part,
- output: stringifyForCompaction(output),
- };
+ const output =
+ ("output" in part ? part.output : undefined) ??
+ ("result" in part ? part.result : undefined);
+ if (output === undefined) {
+ return part;
+ }
+
+ const stringified = stringifyForCompaction(output);
+ const newPart = { ...part, output: stringified } as any;
+ if ("result" in part) {
+ newPart.result = stringified;
+ }
+ return newPart;📝 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 output = | |
| ("output" in part ? part.output : undefined) ?? | |
| ("result" in part ? part.result : undefined); | |
| if (output === undefined) { | |
| return part; | |
| } | |
| return { | |
| ...part, | |
| output: stringifyForCompaction(output), | |
| }; | |
| }), | |
| const output = | |
| ("output" in part ? part.output : undefined) ?? | |
| ("result" in part ? part.result : undefined); | |
| if (output === undefined) { | |
| return part; | |
| } | |
| const stringified = stringifyForCompaction(output); | |
| const newPart = { ...part, output: stringified } as any; | |
| if ("result" in part) { | |
| newPart.result = stringified; | |
| } | |
| return newPart; | |
| }), |
🤖 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/features/workspaces/ai/ai-compaction.ts` around lines 89 - 100, Update
the mapping logic in ai-compaction to stringify both native result and output
fields on tool-result parts. Preserve the existing value selection and
unchanged-part behavior, but when a value exists, overwrite result alongside
output with stringifyForCompaction so downstream summarization cannot render
objects as [object Object].
| function getOrchestrationCallOutcome( | ||
| toolName: string, | ||
| state: z.output<typeof orchestrationCallStateSchema>, | ||
| result: unknown, | ||
| ): AIToolOutcome { | ||
| if (state === "applied") { | ||
| return getAIToolOutputOutcome(toolName, result); | ||
| } | ||
|
|
||
| if (state === "pending") { | ||
| return { failureCodes: ["approval_pending"], failedCount: 0, status: "partial" }; | ||
| } | ||
|
|
||
| return { | ||
| failureCodes: [state === "reverted" ? "codemode_tool_reverted" : "codemode_tool_error"], | ||
| failedCount: 1, | ||
| status: "error", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files src/features/workspaces/ai | sed -n '1,200p'Repository: ThinkEx-OSS/thinkex
Length of output: 2565
🏁 Script executed:
ast-grep outline src/features/workspaces/ai/ai-thread-orchestration-contract.ts --view expanded
printf '\n--- ai-tool-outcome ---\n'
ast-grep outline src/features/workspaces/ai/ai-tool-outcome.ts --view expanded
printf '\n--- orchestration worker test ---\n'
ast-grep outline src/features/workspaces/ai/ai-thread-orchestration.worker.test.ts --view expanded
printf '\n--- tool outcome worker test ---\n'
ast-grep outline src/features/workspaces/ai/ai-tool-outcome.worker.test.ts --view expandedRepository: ThinkEx-OSS/thinkex
Length of output: 2098
🏁 Script executed:
sed -n '180,265p' src/features/workspaces/ai/ai-thread-orchestration-contract.ts
printf '\n--- tool outcome ---\n'
sed -n '1,120p' src/features/workspaces/ai/ai-tool-outcome.ts
printf '\n--- search tests for executing/paused ---\n'
rg -n '"executing"|"paused"|executing|paused' src/features/workspaces/ai/*.test.tsRepository: ThinkEx-OSS/thinkex
Length of output: 5042
🏁 Script executed:
sed -n '1,260p' src/features/workspaces/ai/ai-thread-orchestration.ts
printf '\n--- telemetry format ---\n'
sed -n '1,260p' src/features/workspaces/ai/ai-thread-telemetry-format.ts
printf '\n--- search for executing in ai feature ---\n'
rg -n '\bexecuting\b|orchestrationCallOutcome|getOrchestrationCallOutcome|getOrchestrationCallStatus|normalizeAIThreadOrchestrationOutput' src/features/workspaces/aiRepository: ThinkEx-OSS/thinkex
Length of output: 11386
🏁 Script executed:
sed -n '1,240p' src/features/workspaces/ai/ai-thread-orchestration.worker.test.ts
printf '\n--- orchestration contract top ---\n'
sed -n '1,120p' src/features/workspaces/ai/ai-thread-orchestration-contract.tsRepository: ThinkEx-OSS/thinkex
Length of output: 8587
🏁 Script executed:
sed -n '120,260p' src/features/workspaces/ai/ai-thread-orchestration-contract.tsRepository: ThinkEx-OSS/thinkex
Length of output: 3692
🏁 Script executed:
rg -n '"running"|"approval_pending"|status: "paused"|status: "completed"|status: "error"' src/features/workspaces/ai/*.test.ts
printf '\n--- orchestration worker test around all cases ---\n'
sed -n '1,260p' src/features/workspaces/ai/ai-thread-orchestration.worker.test.tsRepository: ThinkEx-OSS/thinkex
Length of output: 7031
Handle executing as a non-error state. getOrchestrationCallStatus already marks it running, but getOrchestrationCallOutcome still returns status: "error", which rolls into aggregateAIToolOutcomes and marks the whole orchestration failed. Add a non-error branch for executing and a regression test for the executing/paused paths.
🤖 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/features/workspaces/ai/ai-thread-orchestration-contract.ts` around lines
233 - 251, The getOrchestrationCallOutcome function currently treats executing
as an error. Add an executing branch that returns the non-error running outcome
expected by aggregateAIToolOutcomes, and add regression coverage for both
executing and paused orchestration paths while preserving existing applied,
pending, reverted, and error behavior.
| function addAIThreadToolEntries(entries: AIThreadToolEntry[], tools: ToolSet) { | ||
| for (const [name, tool] of Object.entries(tools)) { | ||
| if (!tool) { | ||
| continue; | ||
| } | ||
|
|
||
| entries.push(entry); | ||
| } | ||
|
|
||
| function addAIThreadToolEntries( | ||
| entries: AIThreadToolEntry[], | ||
| tools: ToolSet, | ||
| descriptors: AIThreadToolDescriptor[], | ||
| ) { | ||
| for (const descriptor of descriptors) { | ||
| addAIThreadToolEntry(entries, { | ||
| ...descriptor, | ||
| tool: tools[descriptor.name], | ||
| entries.push({ | ||
| ...requireAiToolDefinition(name).model, | ||
| name, | ||
| tool, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unregistered tool name crashes the whole turn, with no safety net.
requireAiToolDefinition(name) throws for any tool key not present in AI_TOOL_REGISTRY. Since addAIThreadToolEntries runs over every key returned by createSandboxTools/createAIThreadCodeRunTools/createAIThreadWebTools/createAIThreadResearchTools/createAIThreadTimeTools/createAIThreadWorkspaceTools on every beforeTurn, a single new/renamed first-party tool that isn't added to AI_TOOL_REGISTRY will throw during catalog construction and break turn setup for every thread that reaches this code path — not just callers of that one tool. There's currently no test enforcing that the registry stays in sync with the actual tool factories (ai-tool-registry.test.ts only checks two hand-picked entries).
Consider adding a coverage test (e.g. in ai-tool-registry.test.ts) that calls each create*Tools(...) factory with minimal/mock inputs and asserts requireAiToolDefinition doesn't throw for any resulting key, so a missing registration is caught in CI rather than at request time.
🤖 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/features/workspaces/ai/ai-thread-runtime.ts` around lines 240 - 251, Add
registry coverage in ai-tool-registry.test.ts that invokes each tool factory
used by addAIThreadToolEntries—createSandboxTools, createAIThreadCodeRunTools,
createAIThreadWebTools, createAIThreadResearchTools, createAIThreadTimeTools,
and createAIThreadWorkspaceTools—with minimal or mocked inputs, then verifies
requireAiToolDefinition does not throw for every returned tool key. Keep the
test focused on detecting missing AI_TOOL_REGISTRY entries during CI.
There was a problem hiding this comment.
0 issues found across 10 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
1 participant
What changed
Why
@cloudflare/codemodecurrently invokes AI SDK tools withoutToolExecutionOptions, which caused nested workspace calls to throw while the outer orchestration span could still be recorded as successful. AI SDKoutputSchemaalso does not validate ordinary direct execution results at runtime.This keeps Cloudflare's runtime intact and adds a temporary source-level adapter rather than forking Code Mode or patching compiled compaction output. The adapter can be deleted when upstream supports execution-context propagation and output validation.
Impact
Validation
pnpm checkpnpm test— 97/97pnpm test:workers— 15/15pnpm knipnpx react-doctor@latest --verbose --scope changed— no changed-code issuespnpm build:app— application build, four local container images, and/blogprerender completedNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Tests