tsk-oju3st [OPEN] Consent approve: 'Assign later' / defer project bi - #2187
tsk-oju3st [OPEN] Consent approve: 'Assign later' / defer project bi#2187jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughApproval requests can defer project binding. Deferred approvals mint unbound tokens and grants, skip project membership side effects, and preserve later binding through ChangesDeferred agent binding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant approve_request_record
participant TokenAndGrantMinting
participant ProjectMembership
participant A2AChannelSync
Admin->>approve_request_record: approve with defer_binding
approve_request_record->>TokenAndGrantMinting: mint unbound token and grants
TokenAndGrantMinting->>ProjectMembership: skip project membership setup
TokenAndGrantMinting->>A2AChannelSync: skip project channel synchronization
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Good change, careful threading, and one blocker: the flag is never wired to the function, so the feature is inert.
return await approve_request_record(
request,
record=record,
granted_scopes=body.granted_scopes,
effective_project=effective_project,
decided_by=user.user_id,
project_id=body.project_id,
)
This is worse than the feature being absent. The endpoint accepts Fix is one line: project_id=body.project_id,
defer_binding=body.defer_binding,
)The rest of the change is right, and I checked it rather than assuming. Every write path is switched from What CI green means here. 17 checks pass with the feature completely inert, which tells us nothing exercises the flag end to end. Whatever fixes the wiring needs a test that sends One more worth adding while you are there: assert that |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/agent_auth_requests.py (1)
811-818: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
defer_bindinginto the shared approval flow.
approve_request_record()receives its defaultFalsehere, so consent approvals with"defer_binding": truestill require/bindproject_idand execute project side effects.Proposed fix
decided_by=user.user_id, project_id=body.project_id, + defer_binding=body.defer_binding, )🤖 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 `@tinyagentos/routes/agent_auth_requests.py` around lines 811 - 818, Pass the request body's defer_binding value into approve_request_record in this consent approval path. Preserve the existing fields and ensure true is forwarded so deferred approvals skip project binding and related side effects.
🤖 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 `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 93-96: Add tests for ApproveBody approval flows covering deferred
project-scoped approval without project_id, asserting unbound tokens/grants and
no membership or A2A creation, and covering non-deferred approval validation
plus project binding behavior. Reuse the existing approval, token, membership,
and A2A test fixtures and helpers.
---
Outside diff comments:
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 811-818: Pass the request body's defer_binding value into
approve_request_record in this consent approval path. Preserve the existing
fields and ensure true is forwarded so deferred approvals skip project binding
and related side effects.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b99125de-6a92-4155-8ca1-cb3d59054027
📒 Files selected for processing (1)
tinyagentos/routes/agent_auth_requests.py
| class ApproveBody(BaseModel): | ||
| granted_scopes: list[str] | ||
| project_id: Optional[str] = None | ||
| defer_binding: bool = False |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add deferred-binding approval tests.
This PR adds a public approval mode but changes no tests. Cover deferred project-scoped approval without project_id (unbound token/grants and no membership/a2a), plus the non-deferred validation/bound behavior.
🤖 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 `@tinyagentos/routes/agent_auth_requests.py` around lines 93 - 96, Add tests
for ApproveBody approval flows covering deferred project-scoped approval without
project_id, asserting unbound tokens/grants and no membership or A2A creation,
and covering non-deferred approval validation plus project binding behavior.
Reuse the existing approval, token, membership, and A2A test fixtures and
helpers.
PR Summary by QodoSupport deferred project binding when approving agent auth requests
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
nemotron-super review VERDICT: LGTM Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: The deferred binding feature is functionally correct but lacks test coverage and has a minor parameter validation gap.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 77.9K · Output: 28.1K · Cached: 362.8K |
|
nemotron-ultra-kilo review VERDICT: Conditional pass — security semantics of unbound project-scoped grants need verification; missing tests for new defer_binding path.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
Code Review by Qodo
1. Defer flag ignored
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| class ApproveBody(BaseModel): | ||
| granted_scopes: list[str] | ||
| project_id: Optional[str] = None | ||
| defer_binding: bool = False |
There was a problem hiding this comment.
1. Defer flag ignored 🐞 Bug ≡ Correctness
ApproveBody adds defer_binding, but _do_approve() never passes body.defer_binding into approve_request_record(), so the helper always uses its default False and the deferred-binding path is unreachable from the approve API. This breaks the intended “Assign later” behavior (including skipping the project_id-required guard) and makes the new field/docstrings misleading.
Agent Prompt
### Issue description
The approve endpoint accepts `defer_binding` in `ApproveBody`, but `_do_approve()` does not forward it into `approve_request_record()`. Therefore, `approve_request_record(..., defer_binding=...)` always receives the default `False`, and the new deferred-binding logic cannot be exercised.
### Issue Context
The PR’s new behavior is implemented in `approve_request_record()` and described in its docstring, but the route handler `_do_approve()` is the consent approval path that should activate it based on the request body.
### Fix Focus Areas
- tinyagentos/routes/agent_auth_requests.py[787-818]
- tinyagentos/routes/agent_auth_requests.py[93-97]
- tinyagentos/routes/agent_auth_requests.py[345-425]
### Implementation notes
- Pass `defer_binding=body.defer_binding` in the `_do_approve()` call to `approve_request_record()`.
- Add/extend tests to cover approving with `defer_binding=true` (e.g., project-scoped scopes granted with no `project_id` should succeed, mint token with no project claim, and skip membership/a2a sync until assign-agent is called).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Reviewed at code level. BLOCKED on two things, both cheap to fix. @hognek fix-forward on the existing branch, same batch as the rebases.
Do NOT chase kilo's warning that assign-agent must re-mint the token: false positive. Auth is grant-gated ( Follow-up card material, not blocking: |
1 participant
Autonomous build of board card tsk-oju3st.
Files:
tinyagentos/routes/agent_auth_requests.py | 55 ++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 16 deletions(-)
Summary by Gitar
defer_bindingflag toApproveBodyandapprove_request_recordto allow minting unbound tokens.This will update automatically on new commits.
Summary by CodeRabbit