feat(collab): agent delegation handshake + sponsor_contact_id + cascades (D1) - #2048
feat(collab): agent delegation handshake + sponsor_contact_id + cascades (D1)#2048hognek wants to merge 5 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds cross-user delegation request handling with scope validation, automatic or manual approval, sponsored-agent persistence, invite metadata propagation, revocation cascades, and contact/instance peer kill switches. ChangesCross-user collaboration delegation
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant DelegatingPeer
participant PeerInbox
participant DelegationHandler
participant ProjectStore
participant Decisions
participant ProjectInviteStore
participant AgentRegistryStore
DelegatingPeer->>PeerInbox: delegation_request envelope
PeerInbox->>DelegationHandler: process_delegation_request
DelegationHandler->>ProjectStore: verify membership and approval setting
alt automatic approval
DelegationHandler->>ProjectInviteStore: mint invite with sponsor metadata
else manual approval
DelegationHandler->>Decisions: create delegation gate
Decisions->>DelegationHandler: complete_delegation_approval
DelegationHandler->>ProjectInviteStore: mint approved invite
end
ProjectInviteStore-->>DelegatingPeer: invite id and status
ProjectInviteStore->>AgentRegistryStore: register sponsored identity on redemption
Possibly related PRs
Suggested reviewers: 🚥 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| reply = "delegation denied" | ||
| elif completed: | ||
| reply = "delegation approved - the task has been assigned" | ||
| else: |
There was a problem hiding this comment.
CRITICAL: reply is unbound on the approved path — regression from this PR's edit.
The original code had if not approved: ... elif completed: ... else: ... but this change deleted the if not approved: and elif completed: conditions, collapsing them into a bare else:. Now reply = f"The delegation to {to_agent} was denied." is only assigned in the denied branch, while await _route_answer_to_agent(decision, reply) at line 420 always runs. On any approved delegation this raises UnboundLocalError/NameError, breaking the existing #161 gated-delegation approval flow (not just the new collab gate).
| else: | |
| if not approved: | |
| reply = "delegation denied" | |
| elif completed: | |
| reply = "delegation approved - the task has been assigned" | |
| else: | |
| reply = "delegation approved, but assigning the task failed - please retry" |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return {"status": "error", "error": "registry not available"} | ||
|
|
||
| # Find all active agents sponsored by this contact. | ||
| sponsored = await registry.list_by_sponsor(contact_id, status="active") |
There was a problem hiding this comment.
WARNING: project_id is documented as restricting revocation to a single project, but the actual agent revocation ignores it.
list_by_sponsor(contact_id, status="active") returns every sponsored agent for the contact regardless of project, so set_status(..., "revoked") revokes ALL of the contact's sponsored identities across every project. Only the later task-unassign step honors project_id. The docstring says "When project_id is provided, only revoke tokens bound to that project (membership-revoke)" — the behavior contradicts the contract and causes over-revocation. Filter the sponsorees by project when project_id is set.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| contact_id = decision_metadata.get("contact_id", "") | ||
| agent_slug = decision_metadata.get("agent_slug", "") | ||
| display_name = decision_metadata.get("display_name", "") | ||
| granted_scopes = decision_metadata.get("granted_scopes", []) |
There was a problem hiding this comment.
WARNING: complete_delegation_approval mints the invite directly from stored granted_scopes without re-applying validate_delegation_scopes or binding to the authorized project.
The stored granted_scopes come from the original decision metadata. If those ever contain files_write/decisions_write they are minted verbatim — the hard-denylist is only enforced at request time (process_delegation_request), not at approval-completion time. Re-run validate_delegation_scopes(...) here and refuse/ strip any deny-scoped invite, and assert project_id matches the project the decision was created for, so a tampered or legacy decision can't mint an over-privileged invite.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Reversible by clearing the flag. | ||
| """ | ||
| # Set a flag that the peer routes check. | ||
| request.app.state._peer_disabled = True |
There was a problem hiding this comment.
SUGGESTION: The per-instance panic flag has no re-enable path.
kill_switch_per_instance sets request.app.state._peer_disabled = True and the docstring claims "Reversible by clearing the flag", but no function clears it (and nothing resets it on restart). Operators have no supported way to resume peer traffic after a panic. Add an explicit resume/clear function or document that a restart is required.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| from tinyagentos.projects.a2a import ensure_a2a_channel | ||
|
|
||
| if project_id: |
There was a problem hiding this comment.
SUGGESTION: A2A audit line is only emitted when project_id is set.
The "Collaboration with ... has been revoked" system message is guarded by if project_id:, so a contact-wide revoke (project_id=None) leaves no in-channel audit trail of which agents/tasks were revoked. Consider posting the summary to a default/admin channel or logging it even when project_id is absent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous Review Summaries (4 snapshots, latest commit d0f6628)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d0f6628)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previously Reported Issues — Now ResolvedAll 3 outstanding findings from the prior incremental review (at
Previous review (commit 68baed6)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previously Reported Issues — Now ResolvedAll 3 outstanding findings from the prior incremental review (at
Previous review (commit cccdaf9)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Previously Reported Issues — Now ResolvedThe 5 findings from the prior review (at
Files Reviewed (2 changed since prior review)
Fix these issues in Kilo Cloud Previous review (commit dc2caa1)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (9 files)
Reviewed by step-3.7-flash · Input: 135.4K · Output: 30.1K · Cached: 1.1M |
| # approval. The denylist is the authoritative gate; the stored scopes are | ||
| # the human-approved set, but the code must never mint tokens for | ||
| # hard-denied scopes regardless. | ||
| safe_scopes, re_denied = validate_delegation_scopes(granted_scopes) |
There was a problem hiding this comment.
WARNING: Scope denylist is re-applied here (good), but project-membership is still NOT re-validated at approval time. This is the remaining half of the earlier complete_delegation_approval finding (#2): process_delegation_request checked is_project_member(contact_id, ..., member_kind="human") at request time, but a Decisions card can be approved much later. If the contact was removed from project_id (or the decision metadata was tampered) between request and approval, this still mints a live project invite for a project the sponsor no longer belongs to. Re-run is_project_member(project_id, contact_id, member_kind="human") here and refuse when it fails.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for proj in (memberships or []): | ||
| pid = proj.get("id") or proj.get("project_id") | ||
| if not pid: | ||
| continue |
There was a problem hiding this comment.
WARNING: list_projects_for_member is called only inside a bare except AttributeError: memberships = []. This method is not defined anywhere in this PR (the new project_store helpers are is_project_member, get/set_project_setting), so the except AttributeError will trigger on every contact-wide revoke and silently produce no audit messages at all — defeating the entire point of this new branch. Either define list_projects_for_member on ProjectStore or drop the silent fallback so the missing-method bug surfaces loudly instead of hiding a missing audit trail for a security-relevant revocation.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| except Exception: | ||
| logger.warning( | ||
| "collab delegation completion failed for decision %s", | ||
| decision.get("id"), exc_info=True, |
There was a problem hiding this comment.
SUGGESTION: When complete_delegation_approval raises, this except Exception logs but routes no answer back to the agent. The decision answer was already persisted as approve, so the remote contact/agent is left permanently hanging with no notification that sponsor minting failed. Route a failure message (like the else branch at line 1316) inside the except so the caller learns the approval did not complete.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
@hognek Rebase needed - this has conflicts against current dev (GitHub reports Everything else on it looks fine from my side; it is purely staleness. Rebase onto current dev and I will take another look. |
…des (D1) Implement cross-user agent delegation (milestone D1 of jaylfc#2012): - Add sponsor_contact_id column to agent_registry (schema + migration + store API) - Delegation-request envelope dispatch in peer inbox (kind=delegation_request) - Policy gate with per-project auto_approve_delegation knob (default OFF) - Sponsored scope tiers: {a2a_send, a2a_receive, project_tasks, canvas_read, registry_feeds_read} - Hard-deny files_write and decisions_write at scope validation - Decisions card integration: collab_delegation_gate kind triggers invite mint on approve - Revoke cascade: contact/membership change → revoke sponsored identities → unassign tasks - 3-level kill-switch: per-agent (existing), per-contact pause, per-instance panic - Invite metadata column with sponsor_contact_id propagation through redeem flow - Project store helpers: is_project_member(), get/set_project_setting() Fixes: jaylfc#2019
CRITICAL: Restore reply assignment in _apply_delegation_grant (regression from collapsed if/elif/else) WARNING: cascade_sponsor_revoke now filters agents by project_id via project_store.is_project_member for membership-scoped revoke WARNING: complete_delegation_approval re-applies scope denylist (validate_delegation_scopes) before minting invite SUGGESTION: Add kill_switch_reenable() for per-instance panic recovery SUGGESTION: A2A audit line emitted for contact-wide revoke across all projects the contact is a member of
68baed6 to
aac8157
Compare
WARNING: Re-validate project membership at approval time in complete_delegation_approval (fail-closed runtime check per design spec 5) WARNING: Remove fragile list_projects_for_member call (method does not exist on project_store). Simplify A2A audit to project-scoped only. SUGGESTION: Route error answer back when complete_delegation_approval raises, so the remote contact receives feedback on approval failure.
aac8157 to
d0f6628
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/peer.py (1)
258-272: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
delegation_requestdispatch sits after the "unrecognised kind" log.Every delegation request emits
unrecognised kind — accepted, no dispatchbefore being correctly dispatched, which will mislead anyone debugging the D1 flow. Move the branch above the log (and drop the redundantbody_datare-read; it's already bound at line 250).♻️ Proposed fix
+ # Dispatch delegation requests to the cross-user collab handler (D1). + if kind == "delegation_request": + from tinyagentos.delegation_handler import process_delegation_request + + result = await process_delegation_request( + request, contact_id=contact_id, envelope_body=body_data, + ) + return {"status": result.get("status", "unknown"), "detail": result} + # Log unrecognised kinds for debugging; they are accepted but not dispatched. logger.info( "peer_inbox: contact=%s kind=%s nonce=%s (unrecognised kind — accepted, no dispatch)", contact_id, kind, envelope.get("nonce", "?"), ) - # Dispatch delegation requests to the cross-user collab handler (D1). - if kind == "delegation_request": - from tinyagentos.delegation_handler import process_delegation_request - - body_data = envelope.get("body", {}) - result = await process_delegation_request( - request, contact_id=contact_id, envelope_body=body_data, - ) - return {"status": result.get("status", "unknown"), "detail": result} - return {"status": "received", "kind": kind, "nonce": envelope.get("nonce")}🤖 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/peer.py` around lines 258 - 272, Move the delegation_request branch in the peer inbox handler above the unrecognised-kind logger so valid delegation requests are not logged as unrecognised. Reuse the existing body_data binding instead of rereading envelope.get("body", {}), while preserving the process_delegation_request call and response mapping.
🧹 Nitpick comments (5)
tinyagentos/routes/decisions.py (1)
476-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the extraneous
fprefix (Ruff F541).🧹 Proposed fix
- error_msg = ( - f"Delegation approval failed: an internal error occurred while " - f"setting up the sponsored agent. Please retry or check the logs." - ) + error_msg = ( + "Delegation approval failed: an internal error occurred while " + "setting up the sponsored agent. Please retry or check the logs." + )🤖 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/decisions.py` around lines 476 - 479, Remove the unnecessary f-string prefix from the `error_msg` assignment in the delegation approval error path, since the message contains no interpolated expressions and Ruff F541 flags it.Source: Linters/SAST tools
tinyagentos/projects/project_store.py (1)
377-396: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRead-modify-write on the
settingsJSON blob can lose a concurrent key update.Two concurrent
set_project_settingcalls for different keys both read the old blob and the later write wins. Acceptable for a low-traffic knob, but worth a single UPDATE using SQLite'sjson_setif settings become hotter.🤖 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/projects/project_store.py` around lines 377 - 396, Update set_project_setting to perform the settings mutation in a single SQLite UPDATE using json_set, rather than reading and rewriting the entire JSON blob in Python. Preserve existing settings and support updating the requested key without allowing concurrent calls for different keys to overwrite one another; retain the method’s boolean success contract.tinyagentos/routes/project_invites.py (1)
1124-1131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated metadata-parsing block — extract a helper.
The nine-line extraction is identical in both redeem paths (and
_row_to_dictalready deserialisesmetadata, so this is purely defensive). A small_sponsor_from_invite(invite)helper keeps the two paths from drifting.Also applies to: 1224-1231
🤖 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/project_invites.py` around lines 1124 - 1131, Extract the duplicated sponsor_contact_id metadata parsing from both redeem paths into a shared _sponsor_from_invite(invite) helper. Preserve the existing defensive handling for missing, string, invalid, and non-dict metadata, then replace both inline blocks with calls to the helper.tests/test_collab_d1_delegation.py (1)
126-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the delegation handler paths, not just validators and stores.
All 15 tests exercise pure helpers (
validate_delegation_scopes,_validate_delegation_envelope_body) and store CRUD.process_delegation_request,complete_delegation_approval, andcascade_sponsor_revokeare untested — a fakeapp.statewith the invite/decision/registry stores would have caught theinvite["id"]return-shape mismatch flagged intinyagentos/delegation_handler.py.🤖 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_collab_d1_delegation.py` around lines 126 - 300, Add async tests covering the delegation handler functions process_delegation_request, complete_delegation_approval, and cascade_sponsor_revoke, using a fake app.state configured with invite, decision, and registry stores. Exercise successful and relevant failure/revocation paths, and assert returned values match the stores’ actual invite identifier shape, including catching any invite["id"] versus invite["invite_id"] mismatch.tinyagentos/delegation_handler.py (1)
518-522: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSwallowed unassign failures are invisible.
except Exception: passhides per-task failures, socountunder-reports and a revoked agent can stay assigned with no log trace. Log at warning level (Ruff S110).🤖 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/delegation_handler.py` around lines 518 - 522, Update the exception handler around task_store.update_task in the unassignment flow to log each failure at warning level, including the task identifier and exception details, instead of silently passing. Preserve the existing count behavior so count increments only after a successful update, and satisfy Ruff S110.Source: Linters/SAST tools
🤖 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/delegation_handler.py`:
- Around line 294-298: The two mint call sites incorrectly read the invite
identifier from the top-level result. In tinyagentos/delegation_handler.py lines
294-298 and 378-382, update _auto_approve_delegation and
complete_delegation_approval to return the identifier from
invite["record"]["invite_id"] while preserving the existing approved response
shape.
- Around line 272-286: Update the invite creation calls in the current flow and
complete_delegation_approval to pass pin_required=False as the mint argument,
rather than only storing it in metadata. Keep the existing metadata and other
mint parameters unchanged so sponsored-agent invites persist a disabled PIN
requirement.
- Around line 414-420: Update the project-scoped revoke membership check in the
delegation revoke flow so a non-None project_id with a missing project_store
fails closed instead of skipping filtering. Return an appropriate error or
revoke no identities in that case; preserve the existing membership check for
available project stores and contact-wide behavior when project_id is None.
In `@tinyagentos/projects/project_store.py`:
- Around line 363-375: Update get_project_setting to validate that the project's
settings value is a dictionary before calling .get; return default for lists,
strings, or any other non-dict JSON value, while preserving the existing
behavior for missing settings and valid dictionaries.
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 455-458: Update the existing-identity reuse flow around
registry.set_sponsor so sponsorship is only assigned when the identity’s current
sponsor_contact_id is empty; preserve any existing sponsor, including when it
belongs to another contact, and avoid re-parenting it during sponsored
redemption.
---
Outside diff comments:
In `@tinyagentos/routes/peer.py`:
- Around line 258-272: Move the delegation_request branch in the peer inbox
handler above the unrecognised-kind logger so valid delegation requests are not
logged as unrecognised. Reuse the existing body_data binding instead of
rereading envelope.get("body", {}), while preserving the
process_delegation_request call and response mapping.
---
Nitpick comments:
In `@tests/test_collab_d1_delegation.py`:
- Around line 126-300: Add async tests covering the delegation handler functions
process_delegation_request, complete_delegation_approval, and
cascade_sponsor_revoke, using a fake app.state configured with invite, decision,
and registry stores. Exercise successful and relevant failure/revocation paths,
and assert returned values match the stores’ actual invite identifier shape,
including catching any invite["id"] versus invite["invite_id"] mismatch.
In `@tinyagentos/delegation_handler.py`:
- Around line 518-522: Update the exception handler around
task_store.update_task in the unassignment flow to log each failure at warning
level, including the task identifier and exception details, instead of silently
passing. Preserve the existing count behavior so count increments only after a
successful update, and satisfy Ruff S110.
In `@tinyagentos/projects/project_store.py`:
- Around line 377-396: Update set_project_setting to perform the settings
mutation in a single SQLite UPDATE using json_set, rather than reading and
rewriting the entire JSON blob in Python. Preserve existing settings and support
updating the requested key without allowing concurrent calls for different keys
to overwrite one another; retain the method’s boolean success contract.
In `@tinyagentos/routes/decisions.py`:
- Around line 476-479: Remove the unnecessary f-string prefix from the
`error_msg` assignment in the delegation approval error path, since the message
contains no interpolated expressions and Ruff F541 flags it.
In `@tinyagentos/routes/project_invites.py`:
- Around line 1124-1131: Extract the duplicated sponsor_contact_id metadata
parsing from both redeem paths into a shared _sponsor_from_invite(invite)
helper. Preserve the existing defensive handling for missing, string, invalid,
and non-dict metadata, then replace both inline blocks with calls to the helper.
🪄 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: f72dc1f2-8989-4e2b-b7d5-b3ea72fe40c2
📒 Files selected for processing (9)
tests/test_collab_d1_delegation.pytinyagentos/agent_registry_store.pytinyagentos/delegation_handler.pytinyagentos/projects/invite_store.pytinyagentos/projects/project_store.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/decisions.pytinyagentos/routes/peer.pytinyagentos/routes/project_invites.py
|
Deep review done against head Worth saying first: roughly 6 of 8 kilo findings are stale against head, fixed by A1. CRITICAL: A2. CRITICAL: the minted invite can never be redeemed, because its PIN is generated and discarded. A3. HIGH: the approval result reaches nobody. B1. HIGH: the sponsored scope tier is not enforced. This contradicts the design's own threat model at Exploit: a contact sends Fix: intersect with an allowlist rather than subtracting a denylist, and render anything outside the tier as distinct elevated scopes needing their own approval. B2. HIGH: revocation does not cut off delegated authority, because nothing calls the cascade. Exploit: after the owner removes a contact from the project and revokes it, the sponsored agent keeps its active registry row and JWT and continues reading board, canvas and A2A indefinitely. Fix: call C. HIGH: the task-unassign cascade is a silent no-op. E. Tests run but cover none of the risk. No skip guards, all imports resolve, 15 passed and 184 across the related suites with no regressions. But coverage is confined to pure functions and store CRUD: zero tests for Things I checked that are genuinely fine: peer-inbox auth is solid (contact_id derived from the hashed bearer token, envelope from/to binding, Ed25519 signature against the pinned pubkey, nonce replay rejection, rate limit, size cap). Cross-user targeting is gated by Two unrelated nits: the Docs: no CHANGELOG entry, and Minimum bar to flip this to fix-forward: items A1, A2 and the end-to-end test are non-negotiable, because without them this is a feature that has never once run. Then A3 delivery, B1 allowlist, B2 cascade call site, and either fix or remove |
CodeRabbit findings resolved (2026-07-27 scan)All 5 findings addressed in 5503bac: 🔴 CRITICAL: Mint return shape (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test_collab_d1_delegation.py`:
- Around line 376-384: Update cascade_sponsor_revoke to validate project_store
availability before calling list_by_sponsor or entering the sponsored-agent loop
whenever project_id is set. Add coverage for an empty sponsored-agent list,
asserting it returns an error containing “project store not available” rather
than “revoked,” while preserving existing behavior for available stores.
- Around line 434-437: The test currently checks the guard locally before
calling AgentRegistryStore.set_sponsor(), so it does not exercise the
implementation enforcing sponsor binding. Update the test to use
assign_approved_auth_agent_request() through the approval/reuse path for
delegated-agent sponsorship, or move the no-reparenting enforcement into
set_sponsor() and update direct coverage accordingly; verify an already
sponsored agent cannot be reassigned.
🪄 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: 71ff443f-a859-4919-962c-7848330e838a
📒 Files selected for processing (4)
tests/test_collab_d1_delegation.pytinyagentos/delegation_handler.pytinyagentos/projects/project_store.pytinyagentos/routes/agent_auth_requests.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tinyagentos/routes/agent_auth_requests.py
- tinyagentos/projects/project_store.py
- tinyagentos/delegation_handler.py
| result = await cascade_sponsor_revoke( | ||
| request, | ||
| contact_id="hub:sponsor", | ||
| project_id="prj-1", | ||
| ) | ||
|
|
||
| # Must fail closed — project_store absent with project_id set is an error | ||
| assert result["status"] == "error" | ||
| assert "project store not available" in result["error"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Cover the zero-sponsored-agent fail-closed path.
cascade_sponsor_revoke checks for project_store only inside the sponsored-agent loop. With list_by_sponsor() returning [], it currently returns "revoked" despite the missing store. Add that case and move the availability check before listing agents.
🤖 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_collab_d1_delegation.py` around lines 376 - 384, Update
cascade_sponsor_revoke to validate project_store availability before calling
list_by_sponsor or entering the sponsored-agent loop whenever project_id is set.
Add coverage for an empty sponsored-agent list, asserting it returns an error
containing “project store not available” rather than “revoked,” while preserving
existing behavior for available stores.
|
Re-reviewed at head Two corrections to my own earlier review first, because I was wrong on both and you should not spend time on them. I stopped inferring and ran it. I drove So A1 is genuinely dead, no KeyError. A2 and B1 are alive and now demonstrated rather than argued: the minted invite still cannot be redeemed by anyone, and
On A2 being worse: adding On C, the reason it silently does nothing, so it can be fixed properly: N1, and this is the one I would fix first after A2: three of the five new regression tests cannot fail. I checked out N2: the policy knob cannot be turned on. N3: the new fail-closed guard is inside the loop ( On the green checks. 694 tests pass across routes, registry, contacts, decisions and projects, plus 263 at store level, and the D1 file is 20/20. No Minimum to flip this to mergeable: fix A2 so If the cascades and kill switch are actually D2 scope, please drop them from the title and file them separately rather than shipping unreferenced code. Unreferenced code that reads as a working safety control is worse than no code, because the next person assumes the kill switch works. |
A2 fix (jaylfc#2048): redeem() always validated the PIN hash regardless of pin_required. When pin_required=False the invite stores 0 but the hash check still ran, making the invite unredeemable with any pin other than the random one mint() generated. Now gate the PIN verification on row['pin_required'] being truthy. N1 fix: 3 of 5 B1 collab regression tests only tested mint storage and never exercised the redeem path — they could not detect the A2 bug. Updated test_mint_collab_invite, test_mint_collab_invite_no_pin_required, and test_default_kind_is_agent to also verify redeem correctness. Added test_mint_redeem_pin_not_required_e2e: mints with pin_required=False and successfully redeems with empty string — kills A1+A2 together. Store tests: 41/41 pass.
5503bac to
877282a
Compare
jaylfc#2048) Fix three regressions introduced by 877282a: - Restore invite["record"]["invite_id"] (was broken to invite["id"]) - Restore pin_required=False as top-level mint arg (was moved to metadata) - Restore cascade fail-closed guard (was weakened to skip on None store) Add set_sponsor guard: never re-parent an already-sponsored identity (the guard is now in production code, not reimplemented in tests). Fix get_project_setting: handle non-dict settings (malformed DB row). Rewrite N1 regression tests through real production paths: - TestMintReturnShape: exercises process_delegation_request e2e - TestSetSponsorGuard: tests production set_sponsor guard directly - Add TestDelegationE2E: mint + redeem e2e (kills A1+A2 together) 31/31 tests pass (D1 + invite_store).
|
Fixes pushed to 035e722. Summary of corrections: A2 (PIN skip):
Guard added to
N1 tests rewritten through real production paths:
Tests: 22/22 (D1 delegation) + 41/41 (invite_store) + 12/12 (project_store) + 124/124 (agent_registry) = 199 passed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Verified all eight fix claims in your 16:06 summary against 035e722 at code level, including running the rewritten tests against the regressed commit (6/6 correctly fail there, pass at head). All eight are true, and the summary's scoping was honest about what it did not claim. This is the reporting standard; the 2182 comment earlier today was the opposite. State: the minimum-to-mergeable bar defined in my earlier review (A2 pin-skip + mint-to-redeem E2E) is met. Merge happens when a real bot review lands on this head (CodeRabbit re-triggered; its limit had eaten the last one). The A3/B1/B2/C items from the after-that list stay open as immediate follow-ups, with one flag raised in priority: A3 means the only reachable production path mints an invite whose ID is never delivered, so D1 cannot complete cross-instance end to end until A3 lands. Card follows on the board; treat it as next after the current batch. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/projects/invite_store.py (1)
473-481: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFailed JSON parse leaves
metadata/scopesas a raw string.Callers treat these as
dict/list(e.g. the invite-redemption path readsmetadata["sponsor_contact_id"]), so a malformed row surfaces as anAttributeErrorrather than a benign default.🛡️ Proposed fix
- for field in ("scopes", "metadata"): - if field in d and isinstance(d[field], str): - try: - d[field] = json.loads(d[field]) - except (ValueError, TypeError): - pass + for field, fallback in (("scopes", []), ("metadata", {})): + if field in d and isinstance(d[field], str): + try: + d[field] = json.loads(d[field]) + except (ValueError, TypeError): + d[field] = fallback🤖 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/projects/invite_store.py` around lines 473 - 481, Update the row deserialization logic around the metadata and scopes fields so failed JSON parsing replaces the raw string with the appropriate benign default for each field: a dictionary for metadata and a list for scopes. Preserve successfully decoded JSON values and return the normalized row from the existing deserialization function.
🤖 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.
Outside diff comments:
In `@tinyagentos/projects/invite_store.py`:
- Around line 473-481: Update the row deserialization logic around the metadata
and scopes fields so failed JSON parsing replaces the raw string with the
appropriate benign default for each field: a dictionary for metadata and a list
for scopes. Preserve successfully decoded JSON values and return the normalized
row from the existing deserialization function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f3d19e2-3e48-44aa-be31-343c0d2d19c2
📒 Files selected for processing (6)
tests/projects/test_invite_store.pytests/test_collab_d1_delegation.pytinyagentos/agent_registry_store.pytinyagentos/delegation_handler.pytinyagentos/projects/invite_store.pytinyagentos/projects/project_store.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tinyagentos/projects/project_store.py
- tinyagentos/agent_registry_store.py
- tinyagentos/delegation_handler.py
|
Two defects in this branch's D1 code, found and verified while reviewing #2196 (whose branch was accidentally cut from this one; it is being re-cut, nothing rides on this PR from that side):
Both reproduce by inspection against this head and both were independently flagged by qodo (findings 6 and 7 on the #2196 review, which ran over this code). Fix-forward here before any re-review; the rest of the qodo findings on that review also target this branch and are worth walking. |
|
Now CONFLICTING after #2194 merged (decisions.py moved underneath this branch). When you rebase onto origin/dev, fold in the two verified runtime bugs from my earlier comment in the same pass: the app.state key mismatch (project_invite_store vs project_invites) and the invite id shape (invite["id"] vs record.invite_id). Rebase without those fixes just re-arms a broken approval path on a fresh base. |
2 participants
Task: t_b72392f4. Fixes #2019.
Summary
Implement cross-user agent delegation (milestone D1 of EPIC #2012).
Changes
Files changed
Tests
311 tests pass across: test_collab_d1_delegation, test_agent_registry, test_project_store, test_invite_store, test_contacts_peer, test_auth_middleware, test_auth_requests_store
Summary by CodeRabbit