Repository navigation
Follow-ups to the sponsor governance change - #285
Conversation
- Restore the incremental first-match loop in wallet_is_or_descends_from so the permit-creation rule stops at the first match again (a direct child is one lookup). Answers are unchanged; wallet_strictly_governs and the shared walk it uses are untouched. - Say in api-key-rotation.md and the emergency_revoke docstring that an ancestor's create_new_key is ignored and that a JWT ancestor must send create_new_key false. - Add a test that counts session.get calls and pins "stops at the first match". Closes #284 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyhDYK2VFo5T1XxHSxvksN Signed-off-by: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title accurately identifies the pull request as follow-up work for the sponsor governance change. It is concise and related to the documented and code changes. |
| Description check | |
The description directly explains the documentation updates, incremental wallet hierarchy walk, tests, validation, and unchanged authorization scope. |
| Linked Issues check | |
Directly linked issue #284 has three coding requirements. The documentation update states that a database-key ancestor ignores create_new_key and returns no replacement key, and that a JWT ancestor … |
| Out of Scope Changes check | |
The changed files are limited to the issue-linked API-key documentation, the emergency_revoke docstring, the permit-creation hierarchy walk, and focused sponsor-governance tests. These changes suppo… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… |
Full details: Linked Issues check
-
Explanation
Directly linked issue
#284has three coding requirements. The documentation update states that a database-key ancestor ignorescreate_new_keyand returns no replacement key, and that a JWT ancestor must set it tofalsebecause the default returns403 jwt_cannot_mint_api_keybefore revocation. Theemergency_revokedocstring states the strict-ancestor replacement-key rule.wallet_is_or_descends_fromuses an incremental parent walk and stops at the first match. The added tests cover lookup counts, malformed links above a match, and the unchanged strict rule. The reported sponsor and permit tests pass unchanged, and the required documentation tests pass.
Full details: Out of Scope Changes check
-
Explanation
The changed files are limited to the issue-linked API-key documentation, the
emergency_revokedocstring, the permit-creation hierarchy walk, and focused sponsor-governance tests. These changes support issue#284. The strict walk, depth cap, descendant authority, tenant checks, actor records, schema, migration, and OpenAPI output remain unchanged according to the reviewed PR evidence. No unrelated change is demonstrated.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b85b77adc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace the invented parent map and fake wallet objects with the real database-backed world fixture and a wrapper that counts get calls on a real session. The malformed chains are made by tampering real rows. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyhDYK2VFo5T1XxHSxvksN Signed-off-by: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
REVIEW: PASS hermes a78c9ce ScopeClaude follow-ups to merged #282 (sponsor governs descendants' permits and keys). Restores the incremental first-match walk in Authorship
Behind main
Correctness / security
Tests / CI
VerdictPASS. No blockers. Behind main by 1 — delta after Pi merges main. |
…ance-followups Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com>
|
Independent check of a78c9ce by the coordinating session (not a QA verdict; hermes already passed this head).
Generated by Claude Code |
|
REVIEW: PASS hermes 551652a Delta review after Pi merged main (09fae4d) into PASSed a78c9ce.
No push. Nothing else changed in the PR patch. |
|
READY FOR QA 551652a This head is the maintainer's merger merging main into the branch (merge commit 551652a, parents a78c9ce and 09fae4d). I pushed nothing. REVIEW: PASS hermes already names a78c9ce; the delta since then is the merge only. Files the merge of main touched (main's own change, #207):
The PR's own 4 files, none of which the merge touched (no overlap):
Evidence:
Generated by Claude Code |
Summary
Three small follow-ups from review of #282. No authorization behavior changes.
gateway/docs/api-key-rotation.md: an ancestor using a database key ignorescreate_new_key(no replacement key,new_keyis null); an authorized JWT ancestor must setcreate_new_keyto false, because the default true is refused with403jwt_cannot_mint_api_keybefore revocation.emergency_revokedocstring ingateway/app/routers/api_keys.pysays the same. The route's OpenAPI description is set explicitly, sodocs/openapi.jsonis unchanged.wallet_is_or_descends_from(the permit-creation rule) again reads the chain one wallet at a time and returns at the first parent equal to the ancestor, as it did before Let a sponsor govern its descendants permits and keys #282. A direct child is onesession.getinstead of two, and an ancestor near the child no longer loads up to 32 wallets. The answers are the same as before (the differential fuzz in the Let a sponsor govern its descendants permits and keys #282 review found 0 mismatches over 1,815,000 cases).wallet_strictly_governsandwalk_wallet_ancestryare not touched, so the strict rule still needs the complete chain and keeps its depth cap.Not changed on purpose: who may act on a descendant, the depth cap on the strict walk, tenant checks, actor records.
Related issues
Closes #284
Area and license
spec/,verifier/,sdk/python/,sdk/typescript/,integrations/, andexamples/are Apache-2.0.gateway/contributions are under FSL-1.1-ALv2 (Fair Source).Spec changes
Validation
Two new tests in
gateway/tests/test_sponsor_downward_governance.pyuse the real database-backedworldfixture (sponsor S, agent A, grandchild G, a sibling) and a thin wrapper that countsgetcalls on a real session; no invented wallet data:test_permit_creation_rule_stops_at_the_first_match: direct child (A under S, G under A) is True after 1 lookup; ancestor at hop 2 is True after 2 lookups; no match walks to the root and answers False (2 lookups); equal ids need 0 lookups; the strict rule needs the whole chain (3 lookups for S over G).test_permit_creation_rule_ignores_a_malformed_link_above_the_match: with a cycle above the match (S points back at A) and with a missing row above the match (S points at a wallet that does not exist, made by tampering the real row with foreign keys suspended), the creation rule is still True after the first lookup whilewallet_strictly_governsis False for the same chain.Mutation check: with the full-chain walk put back into
wallet_is_or_descends_from, both new tests fail ((True, 2) == (True, 1)and(True, 3) == (True, 1)) and the other 19 sponsor tests pass. The fix was restored and all 21 pass.Commands (serial,
python -m pytest, fromgateway/with the pinned requirements and the local SDKverifyextra):tests/test_sponsor_downward_governance.py tests/test_permits.py tests/test_permit_break_it.py tests/test_owner_enforcement.py tests/test_api_keys.py tests/test_rotate_api_keys.py tests/test_route_auth_inventory.pyplus the docs teststest_docs_reference_integrity.py test_docs_links.py test_check_doc_references.py test_operator_doc_contracts.py test_onboarding_doc_contracts.py test_testing_tiers_doc.py test_proof_matrix_doc.py: 203 passed. No existing test was edited.TRUST_MODE_ENABLED=true ALLOW_LEGACY_UNPERMITTED_MCP=false ENABLE_PROOF_SURFACES=false,-m production_trust): 12 passed.ruff check .andruff format --check .(pinned version): clean.mypy==2.4.0 mypy app/: no issues in 213 files.python scripts/export_openapi.py --check: matches.Risk and compatibility
wallet_is_or_descends_fromduplicates a few lines of the shared walk on purpose, so the strict rule is not touched.Contributor checklist
https://claude.ai/code/session_01GyhDYK2VFo5T1XxHSxvksN
Generated by Claude Code
Note
Low Risk
Lookup optimization preserves prior permit-creation semantics; changes are docs plus tests with strict governance paths untouched.
Overview
Follow-up to sponsor governance review: no authorization rule changes, only clearer docs, a cheaper permit-creation hierarchy walk, and tests.
wallet_is_or_descends_fromagain walks parent links one wallet at a time and returns as soon as the ancestor matches, instead of loading the full chain viawalk_wallet_ancestry. Same boolean answers as before; direct-child checks use one DBgetinstead of walking up to 32 rows.wallet_strictly_governsis unchanged and still requires a complete, sound chain.Emergency revoke behavior is documented in the runbook and
emergency_revokedocstring: strict ancestors acting on a descendant never get a replacement key (create_new_keyignored for DB keys; JWT with defaultcreate_new_key: truegets403 jwt_cannot_mint_api_keybefore revocation).Two integration tests assert lookup counts and that malformed links above a match do not alter the permit-creation rule while the strict rule still fails closed.
Reviewed by Cursor Bugbot for commit 551652a. Bugbot is set up for automated code reviews on this repo. Configure here.