Repository navigation
feat(agents): add identity storage and validation contracts - #43720
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptileai Please reassess the positive lookup cache and retained authoritative reads at e171b8c; current verification details are in the description |
|
bugbot run. Please review the current identity lookup cache changes, including warm-cache revocation and authoritative policy failure behavior |
|
@greptileai Please review 605aa2e. Production code is unchanged; the description documents live cache results, authoritative-read rationale, and the registration ownership guard |
|
bugbot run. Please reassess the cross-agent reassignment finding using the documented live HTTP 409 evidence and registration ownership guard at this tip |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 605aa2e. Configure here.
TLDR
Problem this solves:
How it solves it:
Adds the schema migration, typed identity contracts and identity persistence helpers. Registration and Entra request authentication are activated by later PRs, keeping this prerequisite independently reviewable
The review follow-up reuses
UserApiKeyCachefor positive Entra application-to-agent lookup hints. Cache hits still load current agent policy and delegated subjects from the writer. Missing identities and authorization decisions are never cached. PR #43722 activates the shared-cache wiringStack 1/9. Targets
main. Review size: 1,835 additions + deletions, including 744 test lines and 1,091 other lines, measured against the immediate parentUser Flow
Before: the immediate parent provides the following experience
After: this PR provides the following experience
Relevant issues
Splits the approved scope from #43149 into the smaller review stack below. The nine PRs collectively replace #43149, #43229, #43230 and #43459; this PR covers only the scope described above. Original PRs remain open pending replacement review
Affected release
Linear ticket
Resolves LIT-8956
Pre-Submission checklist
Verification
Current head
605aa2e788fc75b371b458a51a39b566e9cefaf7, merge base7e383c9f6ace54463b6c06a115b5950822cb0913416 affected tests pass. The existing spend-log fixture now expects the five attribution fields introduced by this PR. The new read-count regression failed on the previous tip with two identity lookups, then passed with one lookup across two resolutions and two authoritative policy reads. Warm-cache tests cover disabled/deleted agents, inactive/rebound identities, role/mode changes, missing subjects and writer failures. Newly registered identities remain immediately discoverable because misses are not cached
Canonical whole-tree Ruff, test-tree lint, formatting, strict-rule, type-discipline, test-quality, basedpyright budget, E2E type, operation-boundary and import checks pass without increasing budgets. Existing Pydantic notices and an unrelated test-tree noqa warning remain
Changed executable-line coverage is 349/349 for the whole PR and 23/23 for this repair. Touched-function branch coverage is 78/80 for the whole PR, including two pre-existing API-key validator branches outside this repair; the repair's touched functions cover 26/26 branches. These are not repository-wide coverage percentages
Current-tip Veria and Cursor Bugbot both pass with no findings or annotations. Both original review threads were resolved by their respective bots. Greptile explicitly accepted the retained authoritative reads and withdrew its finding. Its summary still describes the prior tip, so current-tip summary confirmation remains pending
Independent PostgreSQL and Redis verification passed at this exact tip. The spend-log fixture failure is fixed locally. The same four external Google OpenAPI failures occurred before this repair at
e3209bad8a; that test and its implementation are unchanged from the merge base. Required CI, completed hosted coverage, and contributor-agreement evidence remain pendingThe e171b8c follow-up reuses UserApiKeyCache and get_management_object_ttl for positive application-to-agent lookup hints; #43722 passes the shared cache at both authentication sites. Repeated resolution now performs one identity lookup across two requests while re-reading policy twice. Policy and delegated-subject reads intentionally remain authoritative: caching them by TTL would delay disablement, role/mode changes and subject revocation across workers. Existing get_user_object also performs authoritative email-backfill reads on applicable cache hits (auth_checks.py:2582-2643), so the existing helper pattern does not guarantee zero request-time SQL. Greptile accepted this bounded optimization under the explicit freshness requirement and withdrew the finding, as linked above.
Screenshots / Proof of Fix
This foundation PR activates no endpoint. Independent source builds used isolated PostgreSQL databases with 190 migrations and the existing
UserApiKeyCache, both in-memory and backed by Redis. No LLM call is needed for storage verification. Harness and SQL logs are retained in the verification artifactsBefore (
7e383c9f6ace54463b6c06a115b5950822cb0913)AgentIdentityschema at the merge baseAfter (
605aa2e788fc75b371b458a51a39b566e9cefaf7)CACHE_MODE=redis DATABASE_URL=<isolated-database> .venv/bin/python cache_store_live.py S1-605aa2e7-redis, then repeat withCACHE_MODE=memorye3209bad8a, identity lookup calls drop from five to zero and PostgreSQL identity SELECTs from ten to five. Current policy and delegated subject reads, plus authentication writes, remain unchangedFor Bugbot's reassignment concern, the separate registration follow-up at
60915a7b672e30d239dfc5da03f33a3738924ab9passed 17 live checks: another agent's POST/PUT/PATCH claim to an owned application returns HTTP 409, while same-owner reactivation succeeds. This is guard evidence at that separate commit, not a combined cache-plus-registration build. SQL-only reassignment at the current tip denies access until hint expiry and never admits the old agentType
New Feature
Caveats (if any)
Medium
Final Attestation