Fix-forward PR 2308 (consent defer): untested defer+active-handle path, 409 advises a duplicate identity - #2313
Conversation
…ard) _do_approve never passed defer_binding to approve_request_record, so the flag added in PR 2187 was dead code: the project_id-required 400 guard still fired on the defer combo, and when project_id was present the token was bound anyway despite defer_binding:true. - Pass defer_binding=body.defer_binding at the call site. - Add a 400 guard for defer_binding:true with an explicit (non-blank) project_id -- the two are contradictory. - Route tests covering the defer combo (unbound token, unbound grants, no membership row, no a2a channel), the 400 guard, and the unchanged non-deferred path. Red run on buggy head (before the fix): test_defer_with_project_scopes_no_project_id_succeeds_unbound -> 400 (expected 200) test_defer_with_explicit_project_id_returns_400 -> 200 (expected 400) Lifecycle note on never-bound deferred identities: An unbound deferred identity is minted with project_id=None on both the token and every grant. It is currently indistinguishable from a global identity: no expiry, no pending binding flag, no visibility surface in the Permissions app. A follow-up card should track (a) a lifecycle/expiry mechanism so deferred-but-never-bound grants do not persist forever, and (b) a visibility surface so operators can see and reap stale deferred identities.
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe approval flow adds optional deferred project binding. Deferred approvals create unbound tokens and grants, skip project membership and A2A setup, reject explicit project conflicts, and return assignment guidance for active handles. Existing explicit project approvals retain their binding behavior. ChangesDeferred agent project binding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ApprovalRequest
participant _do_approve
participant approve_request_record
participant ProjectAccess
ApprovalRequest->>_do_approve: Submit defer_binding
_do_approve->>approve_request_record: Forward approval option
approve_request_record->>ProjectAccess: Create unbound credentials
approve_request_record-->>ApprovalRequest: Return approval result
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 |
|
|
PR Summary by QodoFix defer_binding approvals: mint unbound identities, validate conflicts, add coverage
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_routes_agent_auth_requests.py (1)
1447-1466: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an endpoint-level denial for the unbound agent.
This test checks the unbound
project_tasksgrant withproject_id=None, but the granted scope is still active. Add an authenticated request to a project-scoped endpoint such as/api/projects/{id}/tasks/...beforeassign-agentand assert the expected 403/404 response.🤖 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 `@tests/test_routes_agent_auth_requests.py` around lines 1447 - 1466, Extend the test around the unbound grant assertions to make an authenticated request as the approved agent to a project-scoped tasks endpoint before the assign-agent flow. Assert the endpoint rejects the unbound agent with the expected 403 or 404 response, while preserving the existing token, grant, membership, and channel assertions.
🤖 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.
Nitpick comments:
In `@tests/test_routes_agent_auth_requests.py`:
- Around line 1447-1466: Extend the test around the unbound grant assertions to
make an authenticated request as the approved agent to a project-scoped tasks
endpoint before the assign-agent flow. Assert the endpoint rejects the unbound
agent with the expected 403 or 404 response, while preserving the existing
token, grant, membership, and channel assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71f598b1-2927-4221-98e3-bd1187b0f021
📒 Files selected for processing (2)
tests/test_routes_agent_auth_requests.pytinyagentos/routes/agent_auth_requests.py
|
nemotron-super review VERDICT: Incomplete test coverage
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
Code Review by Qodo
1. Defer 409 message race
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # When deferring, the token and grants are minted unbound (project_id=None) | ||
| # regardless of effective_project; project-scoped calls 403 until the agent | ||
| # is bound to a project later via assign-agent. | ||
| binding_project = None if defer_binding else effective_project | ||
|
|
There was a problem hiding this comment.
1. Defer guard not centralized 🐞 Bug ⚙ Maintainability
approve_request_record will silently ignore any provided project binding when defer_binding=True (binding_project=None), but the contradictory-input rejection (defer_binding + explicit project_id) is only enforced in _do_approve, so direct callers can bypass the invariant and mint an unexpectedly unbound token/grants.
Agent Prompt
### Issue description
`approve_request_record()` is a shared minting helper used outside the consent route. After this PR, `defer_binding=True` forces `binding_project=None`, which mints an unbound token/grants even if a caller passes a non-empty `project_id`. The contradiction check currently exists only in `_do_approve()`, so any direct caller of `approve_request_record()` can bypass the invariant and get surprising behavior.
### Issue Context
- `_do_approve()` rejects `defer_binding` combined with an explicit `project_id`, but `approve_request_record()` itself does not.
- `approve_request_record()` is imported and called directly by other routes (e.g. project invites), so invariants should live in the shared helper to prevent future misuse.
### Fix Focus Areas
- tinyagentos/routes/agent_auth_requests.py[413-515]
- tinyagentos/routes/agent_auth_requests.py[829-852]
- tinyagentos/routes/project_invites.py[1127-1160]
### Proposed fix
1. Add an invariant check near the top of `approve_request_record()`:
- If `defer_binding` is true AND `project_id` is a non-empty string, raise `HTTPException(400, ...)` (or a typed exception that callers translate to 400).
2. Optionally keep the `_do_approve()` guard (defense in depth), but make `approve_request_record()` authoritative.
3. Add a small unit test that calls `approve_request_record()` (or hits a route that uses it) with `defer_binding=True` and `project_id` set, asserting a 400.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if defer_binding: | ||
| raise HTTPException( | ||
| status_code=409, | ||
| detail=( |
There was a problem hiding this comment.
1. Defer 409 message race 🐞 Bug ≡ Correctness
When defer_binding=true, handle collisions detected via the existing_active pre-check return a 409 instructing operators to use assign-agent, but the concurrent collision IntegrityError path still returns the generic “pick a different identity_claim” message. In a real approval race, this produces misleading remediation guidance for the deferred-binding workflow.
Agent Prompt
### Issue description
`approve_request_record()` adds defer_binding-specific 409 guidance when a handle collision is detected by the `existing_active` pre-check, but the concurrent handle-collision path (`except IntegrityError`) still returns the generic “pick a different identity_claim” message.
This creates inconsistent operator guidance under race conditions (between `get_by_handle()` and `set_status()`), even though the intended deferred flow is to bind the existing identity via `POST /api/projects/{id}/members/assign-agent`.
### Issue Context
- The PR introduces `defer_binding` and new 409 guidance in the `existing_active` collision branch.
- A handle collision can still surface as an `IntegrityError` during activation (e.g., another concurrent approval took the handle after the pre-check).
- The IntegrityError handler currently assumes a handle collision and returns a generic duplicate-identity message; it should return the same defer_binding guidance when defer_binding is true.
### Fix Focus Areas
- tinyagentos/routes/agent_auth_requests.py[475-483]
- tinyagentos/routes/agent_auth_requests.py[551-571]
### Implementation notes
- In the `except IntegrityError:` block, branch on `defer_binding`:
- If `defer_binding` is true, return a 409 detail consistent with the new pre-check message (ideally also including the existing canonical_id if you can safely look it up).
- Otherwise keep the current message.
- Consider defensively confirming the IntegrityError is the expected handle-uniqueness violation (or re-fetch `existing_active` by handle) before emitting handle-specific guidance, so unrelated integrity errors are not misreported.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 399c042 |
Review: the fix is correct. Two red checks — one is yours, one is not.CI only ran properly from 00:05Z: this PR had zero workflow runs until then, because it was opened during the GitHub Actions outage and its Result: 16 success, 2 failures. 1.
|
…ctive-handle 409 Documents the behaviour added in this PR: approving with defer_binding when the agent already holds an active handle returns 409 and points at assign-agent, rather than advising a duplicate identity. Clears the two doc-gate rules the PR was failing (user-visible-changelog and agent-manual).
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 `@docs/agent-coordination.md`:
- Around line 292-299: Update the deferred-binding paragraph to state that
minting the unbound token occurs only when the agent has no active handle.
Explicitly state that the 409 conflict path must not mint a second identity, and
direct users to the existing assign-agent route.
🪄 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: CHILL
Plan: Pro Plus
Run ID: a5b831c4-ba00-431f-beca-8a4c3c6006f7
📒 Files selected for processing (2)
changelog.d/2313-consent-defer-active-handle.mddocs/agent-coordination.md
| Deferred binding and an existing active handle are mutually exclusive. Approving | ||
| an auth-request with `defer_binding` mints the token and grants UNBOUND, so the | ||
| agent has no project until `assign-agent` binds it. If that agent ALREADY holds | ||
| an active handle the approve returns **409** and names | ||
| `POST /api/projects/{project_id}/members/assign-agent` as the route to use. Do | ||
| not resolve that 409 by minting a second identity: canonical ids are issued once | ||
| per agent (`{slug}-{YYYYMMDD}-{HHMMSS}`), and a duplicate splits the agent's | ||
| memory and grants across two ids that never reconcile. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify the minting behavior for the 409 path.
The paragraph first states that defer_binding mints an unbound token, then states that an existing active handle returns 409. State that minting applies only when no active handle exists, and that the conflict path must not mint a second identity.
🤖 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 `@docs/agent-coordination.md` around lines 292 - 299, Update the
deferred-binding paragraph to state that minting the unbound token occurs only
when the agent has no active handle. Explicitly state that the 409 conflict path
must not mint a second identity, and direct users to the existing assign-agent
route.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Overview
Files Reviewed (4 files)
Note: The Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 70.2K · Output: 13.6K · Cached: 817K |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
1 participant
CARD TITLE (intent, not commit subject): Fix-forward PR 2308 (consent defer): untested defer+active-handle path, 409 advises a duplicate identity
Autonomous build of board card tsk-55ygdn.
Files:
tests/test_routes_agent_auth_requests.py | 298 ++++++++++++++++++++++++++++++
tinyagentos/routes/agent_auth_requests.py | 68 +++++--
2 files changed, 353 insertions(+), 13 deletions(-)
Summary by CodeRabbit
New Features
Documentation