feat(agents): consent approval sets registry handle (#1800 slice 7)#1826
feat(agents): consent approval sets registry handle (#1800 slice 7)#1826jaylfc wants to merge 2 commits into
Conversation
…entity claim Derive the registry handle via _slugify from the already-sanitized identity claim in the approve path. Before minting the canonical identity, check get_by_handle(status='active'); if an active identity owns that handle, return 409 and leave the request PENDING so the approver can try a different variant. A SUSPENDED holder does not block reuse. After activating the new agent, update its handle via the store update setter. Tests: - TestHandleSetOnApprove: handle is set, agent passes the a2a no-handle gate - TestHandleCollisionActiveRejects: 409 + request stays pending when an active identity already has the handle - TestHandleCollisionSuspendedAllowsReuse: approval succeeds once the previous holder is suspended Docs-Reviewed: consent approve sets registry handle per lead-agent-identity-and-canvas-access.md
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? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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}
| # them to 'active' so they are NOT in the bus inactive/revocation feed and | ||
| # @taOSmd's identity-AND-grant gate accepts them. | ||
| await registry.set_status(canonical_id, "active", actor=user.user_id) | ||
| await registry.update(canonical_id, handle=handle) |
There was a problem hiding this comment.
WARNING: Handle assignment is racy and not uniqueness-guaranteed
The 409 guard at line 341 runs before the agent is registered, so two concurrent approvals for the same (or slug-colliding) claim both pass the check, register with handle="", flip to active, and then both write the same handle here. The handle column has no UNIQUE constraint (agent_registry_store.py:43) and update() performs a blind UPDATE ... WHERE canonical_id = ? with no collision check, so the "one active identity per handle" invariant this feature relies on can be violated under concurrency.
Consider serializing the check->register->update (this store already uses a lock for exactly this check-then-write pattern, e.g. _reporting_lock) or adding a partial unique index WHERE handle <> '' plus a collision-checked update.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| _claim = record["identity_claim"].strip().removeprefix("@").strip() | ||
| display_name = _claim or record["framework"] | ||
|
|
||
| handle = _slugify(_claim) |
There was a problem hiding this comment.
SUGGESTION: Handle falls back to the literal "agent" when the claim is empty
_slugify(_claim) returns "agent" for an empty/whitespace claim (the or "agent" fallback inside _slugify), but display_name falls back to record["framework"]. So every approval with an empty identity_claim produces handle "agent", which collides with any previously activated "agent" identity and yields a confusing 409 ("pick a different identity_claim") even though the user supplied no real claim. Derive the handle from the same fallback so the two stay coherent.
| handle = _slugify(_claim) | |
| handle = _slugify(display_name) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit f5f0253)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f5f0253)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by hy3:free · Input: 97.2K · Output: 12.3K · Cached: 479.2K |
…e orphan window Register the handle at birth in the approve path so an agent is never left ACTIVE with an empty handle, and add a partial unique index on agent_registry(handle) WHERE status = 'active' AND handle != '' so the DB atomically rejects a concurrent approval of the same identity_claim instead of granting two active agents the identical handle (a2a "from" spoofing). The concurrent loser now fails with 409 and its half-registered row is rolled back and removed. Docs-Reviewed: close consent handle-set TOCTOU + orphan window per lead-agent-identity-and-canvas-access.md
| -- Pending / suspended / revoked rows and empty handles are excluded from the | ||
| -- index, so (a) a handle can be reused once its owner leaves 'active', and (b) | ||
| -- pre-existing empty-handle active agents never block the index's creation. | ||
| CREATE UNIQUE INDEX IF NOT EXISTS ux_agent_active_handle |
There was a problem hiding this comment.
WARNING: Unique index creation can abort init() on existing databases with duplicate active handles
This CREATE UNIQUE INDEX IF NOT EXISTS ux_agent_active_handle runs unconditionally inside SCHEMA via executescript at init() (base_store.py:38), and the WHERE status='active' AND handle!='' guard only excludes empty-handle rows. The earlier, unindexed version of this very feature (flagged by the prior review) could already persist two ACTIVE rows sharing a non-empty handle, so any database written in that window will fail the index build with IntegrityError and abort init(), taking down the whole registry store on upgrade.
Build this in a guarded migration that first detects/clears duplicate active handles (e.g. keep the earliest, rename or suspend the rest) before creating the index, so init() cannot fail on pre-existing bad data. The test in TestPartialUniqueHandleIndex only covers fresh DBs, not the upgrade path.
Minor note (route agent_auth_requests.py:381): the rollback() here is effectively a no-op because register() already committed the pending row (agent_registry_store.py:424) — the real cleanup is the delete() call. The surrounding comment is slightly inaccurate but harmless.
|
Superseded by #1838, which landed all seven slices as one consistent set on dev (with the CI-caught active-handle migration-order fix folded). Closing. |
1 participant
slice 7 of #1800 identity epic, DO NOT MERGE
When an agent auth-request is approved, set the registry handle from the
sanitized identity claim (via _slugify). If the handle collides with an
ACTIVE identity, return 409 and leave the request PENDING so the approver
can pick a different variant. A SUSPENDED holder does not block reuse.