Repository navigation
Conversation
4dea6aa to
b40cfff
Compare
b40cfff to
5995fe0
Compare
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
5995fe0 to
58119ff
Compare
f99d836 to
a224ba5
Compare
a224ba5 to
40fa2a6
Compare
9ecaaaa to
eeff218
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeff2180cd
ℹ️ 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".
eeff218 to
0dc1de7
Compare
50efa9e to
3696ebb
Compare
9336ce2 to
e280593
Compare
e280593 to
3faef28
Compare
ff7ae3c to
7185e40
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7185e40f00
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a24d02a421
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e70fc95f17
ℹ️ 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".
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The rotator is not wired into production writes, principal removal retains reusable key records, and lock-holding operations remain unbounded.
Review effort: Balanced
Findings: 3
Open (6)
Apply BatchTimeout to Replace operations · New Delete bound keys and secrets during principal removal · New Reject delegations with mismatched audiences in Replace · New Re-pin Swarf to a merged or released revision · New Wire Rotator into production policy and principal callbacks · New Validate delegation audience in the memory backend · New
What changed in this PR
Adds infrastructure to rotate and revoke principal-bound key delegations as policies and principals change.
Changes:
- Adds delegation rotation, batched revocation publishing, and shared test utilities.
- Introduces atomic delegation replacement with locking across storage backends.
- Consolidates tenant issuer loading and updates Swarf integration.
| File | Description |
|---|---|
pkg/vault/issuer.go |
Adds shared tenant issuer loading. |
pkg/store/delegation/postgres/store.go |
Adds transactional replacement and advisory locking. |
pkg/store/delegation/memory/store.go |
Adds synchronized in-memory replacement. |
pkg/store/delegation/delegation.go |
Extends the store interface with Replace. |
pkg/store/delegation/delegation_test.go |
Tests replacement and lock behavior. |
pkg/rpc/service/bucket/service.go |
Batches bucket revocations. |
pkg/rpc/service/bucket/service_test.go |
Updates bucket revocation tests. |
pkg/rpc/service/auth/auth.go |
Uses the shared tenant issuer helper. |
pkg/grant/rotator.go |
Implements delegation rotation and revocation. |
pkg/grant/rotator_test.go |
Tests rotator behavior. |
pkg/fx/revocation.go |
Exposes the shared revocation publisher. |
pkg/api/service/accesskey/service.go |
Serializes key creation and deletion with rotations. |
pkg/api/service/accesskey/service_test.go |
Updates access-key lifecycle tests. |
pkg/api/access_keys_test.go |
Updates the revocation test stub. |
internal/testutil/swarf.go |
Adds a batch-aware fake Swarf client. |
go.mod |
Pins Swarf for PublishBatch. |
go.sum |
Updates Swarf checksums. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| github.com/fil-forge/libforge v0.0.0-20260914100934-63f5b20252fe | ||
| github.com/fil-forge/smelt v0.0.0-20260914083257-f594cf02655c | ||
| github.com/fil-forge/swarf v0.0.1-0.20260821142121-d5d1a0a56f00 | ||
| github.com/fil-forge/swarf v0.0.1-0.20260922133653-0b2e5b2e1c6c |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
A policy change has to make the gateway drop what it cached for the keys of the principals it affects, and a principal's removal has to do the same for every key the principal holds. A principal-bound key holds the delegations its principal's effective actions map to, one per bucket and Forge command, so the record that clears the gateway is the ordinary revocation of those delegations through Swarf's /ucan/revoke. pkg/grant gains Rotator. Rotate sets every key's delegations over one bucket to the commands the principal's new actions map to and revokes the ones it replaces; a key that held nothing over the bucket costs no revocation, and an empty action set leaves it with nothing over the bucket. Revoke revokes every delegation of the principal's keys and leaves them empty. Both run in one delegation-store transaction over all the keys, and every revocation of one write goes to Swarf in a single request through the client's new PublishBatch, so the write waits on one round trip however many keys it touches. No revocation carries a nonce: a retry after a rolled-back write republishes the same invocations, which Swarf records once. The delegation store's Replace takes several audiences and swaps their sets in one transaction under their sorted advisory locks; DeleteBySubject locks the audiences it touches for the same reason. One RevocationPublisher interface, in pkg/grant, replaces the copies the access-key and bucket services declared, and both services publish through PublishBatch: a key deletion revokes its delegations under the key's lock, and a bucket deletion revokes every grant over the bucket in one request. vault.TenantIssuer is shared by the access-key service and the rotator. Swarf is pinned to the commit that adds PublishBatch, and ucantone to the one that exposes batch execution. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lock A rotation lists a principal's keys and then waits on their delegation locks; a removal of one of those keys holding the lock first published its revocations and cleared its delegations, and the rotation went on to issue it fresh ones that outlived the record. The access-key service now deletes the record inside the same Replace as the revocation, and Rotate re-reads each key once it holds the lock and leaves a removed one alone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The memory delegation store took its RWMutex across Replace's callback, so a second writer waited for as long as the callback ran, ignoring its ctx and the bound the interface promises. The writers now take a one-slot channel for the whole call, waiting at most LockWait (store.LockTimeout) or until ctx is done, and give up with store.ErrLockTimeout the way the Postgres advisory lock does under lock_timeout. The RWMutex now guards only the map, so reads are answered while next runs, as on Postgres. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…gations Delete loaded the tenant issuer before its Replace, so a vault read failure blocked deleting a key that held nothing to revoke. A principal-bound key whose principal reaches no bucket is that key, and it is the common case. The issuer is now resolved inside the callback, only when the key's current delegations are non-empty. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Rotate and Revoke relied on their callers for the deadline BatchTimeout documents, and a Revoke from a request context with no deadline held the keys' delegation locks for as long as a stalled publish took. Each call now shortens its ctx to BatchTimeout itself; an earlier caller deadline still wins. Test: both operations publish with at most BatchTimeout left. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n the caller's transaction Replace checked only for nil, so a delegation returned under a map key other than its own audience was written under an audience the call had not locked. Both backends now refuse it with ErrInvalidArgument through the shared CheckReplacement. The Postgres store opens its transactions through pglock.Begin and hands next a ctx carrying the transaction, so a store write the callback makes commits with the replacement or not at all. Suite test on both backends: a delegation to one audience filed under another is refused and nothing lands under the other. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…key for a retry On Postgres the access-key row's delete inside the deletion's Replace callback committed on its own, so a failure after it left the delegation rows and the vault secret behind with no row, and a retry answered not-found. The row delete now joins the delegation transaction (through the stores' pglock.Begin adoption below this PR), so the Postgres test here cancels the request right after the row delete and checks the row, the delegations and the secret survive and a retry removes all three. The access-key service test gains a Postgres fixture for it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
PublishRevocations is the callback body of every Replace that revokes what keys hold: the rotator's, a key deletion's, a bucket deletion's and a tenant removal's. Only the rotator applied BatchTimeout, so a key deletion from a request with no deadline held the key's lock behind a stalled publish. The publish now shortens its ctx to BatchTimeout itself, whoever calls it. Test: a publish from a caller with no deadline has at most BatchTimeout left. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>


Rotates a principal-bound key's delegations when its policies change and revokes them when the principal goes. Revocations go to Swarf in one request per write, and the gateway should drop its delagations from caches.
grant.Rotator(Rotate,Revoke),PublishRevocations,BatchTimeoutReplaceswaps several keys' sets in one transactionRevocationPublishershared by key, bucket and policy writestestutil.FakeSwarfPublishBatch); re-pin before merge🤖 Generated with Claude Code