Repository navigation
fix(ai): claim chat approvals before executing writes - #1072
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
The latest updates on your projects. Learn more about Unkey Deploy
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
Claim and deny repeated approvals packages/ai/src/ai/agents/conversation.ts, packages/ai/src/ai/agents/conversation.test.ts |
claimToolApprovals checks eligible approved tool parts in the latest message. If a claim fails, it marks the part denied and sets approval to false. Tests verify that resubmitting an approval does not execute the write again. |
Redis-backed API route claims apps/api/src/routes/agent.ts, apps/api/src/routes/agent-business-context.test.ts |
The route claims approvals with Redis SET NX and a one-day expiry, using a key scoped by user, organization, chat, and approval ID. Redis errors are captured and allow the approval through. Route tests cover identity scope, concurrent requests, model failure after a write, and Redis claim rejection. |
Priority: ⬇️ Low
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant AgentRoute
participant claimToolApprovals
participant Redis
participant ToolExecution
AgentRoute->>claimToolApprovals: Pass validated messages and claim callback
claimToolApprovals->>Redis: Claim scoped approval key with NX and one-day expiry
Redis-->>claimToolApprovals: Return claim result
claimToolApprovals-->>AgentRoute: Return messages with duplicate approval denied
AgentRoute->>ToolExecution: Process approved tool call
Merge Risk: ⚪ Minimal · up to f50ab
Approved tool actions now claim their approval once, so retries and concurrent dashboard requests cannot repeat the same approved write. A caller who deliberately changes the approval identifier could rerun an action they are already allowed to approve, but this does not grant extra access. The change appears ready to merge once its outstanding CI checks pass.
Security Architecture Review
Security architecture risk: 🟡 Moderate · up to f50ab
Identity-scoped atomic claims reduce duplicate writes without expanding caller permissions. Replay using a different approval identifier remains possible, however, and duplicate protection is unavailable during Redis failures. Claims also record an execution attempt rather than its outcome.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred — The supported replay path requires an authenticated caller able to submit an approved mutation message. Organization membership or API-key organization matching, accessible-website checks, and persisted-chat ownership constrain the route. The PR does not establish cross-tenant reachability or increase tool privileges; exact downstream resource limits were not fully inspected.
Security Findings and Attack Paths
- observed — The retained reportable finding concerns altered-ID replay: a caller changes the submitted approval ID to obtain a fresh claim for the mutation payload. This condition predates the PR, whose base already accepted client approval messages without server-side approval binding. The new gate reduces identical-ID replay but does not resolve that existing finding.
Trust Boundaries and Controls
- observed — Strong counterevidence limits the finding to replay rather than new authority: authorization precedes claiming, message validation remains, unsuccessful claims become denied approvals before conversion, and mutation tools retain confirmation checks and forward the original request headers into RPC context.
Resilience and Maintainability Implications
- observed — Test assertions cover one write under concurrent identical claims and claim retention after a model failure following a write. Redis errors deliberately permit execution, and expiry ends duplicate protection. These controls provide bounded deduplication, not exactly-once mutation semantics or outcome reconciliation.
Hardening Proposals
- proposed — For stronger replay protection, bind approvals to authoritative server-issued records containing caller, organization, tool, and payload identity. Track execution outcomes for reconciliation, preserve reservations for uncertain writes, and explicitly choose the failure policy for mutations when claim storage is unavailable.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: claiming chat approvals before executing writes to prevent duplicate actions. |
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Please re-review final head |
|
@coderabbitai review Please re-review final head |
✅ Action performedReview finished.
|
9ba2c14 to
f50ab5c
Compare
|
Fixed the general review's claim-namespace finding in Retaining a consumed claim after a model/stream failure is deliberate: a write may already have happened. Releasing that claim without a durable execution outcome would allow duplicate writes. The existing 24-hour lifetime and logged Redis fail-open policy are preserved; an execution outcome ledger would be a separate behavior change. Declining the optional 80% docstring advisory. There is no corresponding required repository coverage gate; the small claim helper and its behavioral tests express this slice's contract. Adding generic documentation to otherwise unchanged functions would expand its scope. The changed source still needs fresh configured review and exact-head CI before merging. |
|
Please re-review final head |
|
@coderabbitai review Please re-review final head |
✅ Action performedReview finished.
|
f50ab5c to
c977b90
Compare
|
Please re-review final head |
…wo tabs An approval card open in two tabs could be approved twice, and each request ran the write. The chat route now claims every approved call in Redis (per chat and approval id, for a day) before the agent runs; a second claim turns that call into a denial saying it was already used, so the model reports it instead of writing again. A Redis failure lets the approval through and is logged, since the user did approve it.
c977b90 to
1b10597
Compare
|
Please re-review final head |
This branch was successfully deployed
1 participant
When two tabs approve the same chat write, the API now claims the approval in Redis before running the agent. Claims include caller, organization, chat and approval identity with a24-hour lifetime; duplicate approvals become denied tool results. Existing Redis failure behavior is preserved.
Validation: formatting, root lint/types,20native route tests,10conversation tests and101memory tests on current main after #1067. The rebase preserves allthree owned patches and four reviewed source files exactly. Actual CodeRabbit source coverage is the completed four-file baseline plus the completed two-file namespace fix review; the final head has fresh Greptile and CI requests. Historical failed checks are not carried as passes.
Independent slice against main. Maintainer contribution with AI assistance; all review findings have supported fixes or dispositions. Merge waits for final-head checks and every configured reviewer to finish.