fix(agents): avoid duplicate recovery tool-map charge - #1555
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change updates the task-recovery warning, preserves existing ChangesAgent-task recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change is narrowly scoped to prevent duplicate recovery tool-map charging, with regression coverage and the listed checks passing; no actionable merge-blocking risk remains. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. UI screenshot waived by the |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/agent-task-recovery.test.ts`:
- Around line 183-188: Update the fetch mock and assertions in the recovery test
around the globalThis.fetch stub: record each request’s URL and headers, then
assert exactly one recovery request contains the expected authorization and
chatgpt-account-id headers. Also assert that the routed provider request
occurred, and perform these request assertions before validating mappingCharges.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9317099-ef32-4076-8e4b-d7c60b3519e2
📒 Files selected for processing (4)
src/server/index.tssrc/server/responses/core.tstests/agent-task-recovery.test.tstests/helpers/agent-task-recovery.ts
💤 Files with no reviewable changes (1)
- src/server/responses/core.ts
4dcbf00 to
27a3123
Compare
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
1 similar comment
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
The fix itself looks correct, including the security-sensitive parts. recoverEncryptedAgentTask only replaces the encrypted assignment/input item; it does not alter the declared tools, trailing additional_tools, or tool_choice, so keeping the original caller-authorized toolBridgeMaps is the right way to avoid charging the translator retained budget twice. The non-persistable guard remains in place, and the updated warning is more accurate about recovered plaintext.
I am blocking on branch freshness/integration validation before approval. This head is based on 5f9b783b, while current dev is 0a8147b3; dev is 17 commits ahead of the merge base, which is beyond the repository's normal freshness window. Current dev also changed src/server/responses/core.ts (the #1500 web-search streaming argument at the later runWithWebSearch call). That hunk is separate from this recovery change, so I do not see a semantic conflict, but this is a security-boundary server path and the full suite reported here was run against the older base.
Please rebase/update onto current dev and rerun the focused recovery/state tests, typecheck/privacy scan, and full prepush/CI. I found no additional code or security blocker in the present diff.
Recovery reparsing changes task content, not tool definitions. Keep the original bridge maps to avoid charging the translator budget twice, and make the plaintext retention warning explicit. Refs lidge-jun#92
27a3123 to
b7f468e
Compare
|
Thanks — I rebased this onto current The conflict was limited to the same recovery block. I kept #1556's fallback reroute intact and removed only the redundant Validation on the rebased head:
One transparent note: the React Doctor step printed three compiler diagnostics in unchanged |
|
🧠 Learnings used
|
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review on exact head b7f468ed4c4ce5bc8aed36505a2d236942a069d0: approved.
My previous blocker was branch freshness/integration validation. That is resolved: this head is rebased directly onto current dev cfc61c1cdb5e117d109a03996079aeeedfde770b, including #1556.
I rechecked the overlapping recovery block. #1556's post-recovery fallback re-selection and route update remain intact, as does the non-persistable plaintext guard. The production delta here is limited to removing the redundant second buildToolBridgeMaps call.
That removal is correct: recoverEncryptedAgentTask only replaces the encrypted assignment/input part and does not change the caller-declared tools or tool choice. The original maps therefore remain the caller-authorized mapping authority, while rebuilding them only charged the retained translator budget twice. The strengthened regression also proves one authenticated recovery request, one routed provider request, and one namespaced mapping charge.
The updated warning accurately discloses plaintext retention in the bounded process-local cache. I found no remaining correctness, privacy, or security blocker in the current diff.
Repository Actions have not run yet on this fork head (action_required with no jobs), so merge should still wait for exact-head Cross-platform CI / React Doctor approval and successful completion.
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
Wibias
added
the
gui-screenshot-waived
|
🧠 Learnings used
|
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
|
Thanks @soulbah — merged. This is useful because recovery only changes the recovered assignment content, so rebuilding the tool bridge maps after the reparse was both unnecessary and costly: it charged the translator retained budget twice and could make otherwise-valid recovered tasks fail near the limit. Keeping the original maps also preserves the caller-authorized tool mapping across recovery. The focused regression coverage and clearer plaintext-cache warning are appreciated too. |
2 participants
Summary
This follows up on the review left on #1540 and keeps the non-persistable response-state guard added before that PR merged. Refs #92.
The branch is rebased onto
devatcfc61c1cd, including #1556. The overlap incore.tswas resolved by preserving #1556's recovered-task fallback reroute and removing only the redundantbuildToolBridgeMapscall.Verification
Expected: 1,Received: 2.bun scripts/test.ts tests/agent-task-recovery.test.ts tests/agent-task-recovery-fallback.test.ts tests/responses-state.test.ts tests/agent-task-recovery-security.test.ts tests/agent-task-recovery-cache.test.ts tests/config.test.ts— 274 pass, 0 fail.bun run typecheckbun run privacy:scangit diff --check origin/dev...HEADbun run prepush— exited 0 with 11,454 pass, 8 skip, 0 fail on headb7f468ed4c4ce5bc8aed36505a2d236942a069d0(treef114cee4214664ed171c6bc4b5b745b1102b454a). React Doctor also reported three compiler diagnostics in unchangedtests/codex-account-pool-behaviour.test.tsx; this PR changes no GUI files.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests