Conversation
1df612f to
43c1426
Compare
43c1426 to
5905317
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. |
![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: 59053174d1
ℹ️ 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".
5905317 to
8a61d96
Compare
8a61d96 to
8756192
Compare
8756192 to
9056c9d
Compare
00f1f11 to
c107234
Compare
c107234 to
9bb16ff
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bb16ff487
ℹ️ 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".
| last, err := s.plcClient.Last(ctx, rec.ID) | ||
| if _, deactivated := errors.AsType[*plc.DeactivatedDIDError](err); !deactivated { |
There was a problem hiding this comment.
When the DID was tombstoned by a deletion attempt running the parent revision and that attempt then failed during any later cascade operation, this branch incorrectly assumes revocations were already published. The parent implementation deactivated the DID before touching key delegations and never contacted Swarf, so a retry after upgrading skips publication, deletes the remaining local state, and returns success while gateway-cached grants remain usable until expiry. Persist or verify that revocation completed before taking this skip path, with an explicit recovery strategy for legacy tombstones.
AGENTS.md reference: AGENTS.md:L35-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The sequence is reachable and the outcome is what you describe. Two corrections to the framing, and the gap cannot be closed from here.
This branch does not create the gap. main's tenant deletion deactivates the did:plc first and never contacts Swarf at all: the file has no revocations field and does not import pkg/grant. Under that code no tenant deletion ever revoked anything, the happy path included. This branch narrows it to the one case where an older attempt tombstoned the DID and then failed in its cascade.
The skip is correct, and the premise the comment asserts checks out against Swarf. It resolves the invocation issuer live (pkg/fx/app.go:158) and re-validates the revoked delegation, whose issuer is the same tenant (pkg/fx/app.go:178). A tombstoned did:plc fails both, so once the DID is deactivated no revocation for that tenant can land by any path. Persisting a "revocation completed" marker would not help: there is nothing hilt could do with it on the retry.
A second constraint on any recovery. For keys whose DeleteByAudience already succeeded in the failed attempt, grant.PublishRevocations has no delegation objects left to revoke, so even a Swarf that accepted them could not be given them.
Raised against Swarf as fil-forge/swarf#23, since revocation-on-exit is a real workflow and plc.Resolver.Resolve currently cannot tell a tombstone from directory downtime. The comment at the branch head already names this case in prose.
9bb16ff to
79ee879
Compare
c1dc404 to
e1884f1
Compare
e1884f1 to
c507485
Compare
c507485 to
a1da790
Compare
18fe37f to
49a9f67
Compare
49a9f67 to
089bf13
Compare
089bf13 to
44b6a34
Compare
44b6a34 to
6c30e66
Compare
6c30e66 to
348995e
Compare
348995e to
9c2c8f2
Compare
9c2c8f2 to
1a85719
Compare
1a85719 to
32a32ee
Compare
32a32ee to
2bca8b2
Compare
2bca8b2 to
7560b40
Compare
DELETE /tenants/{tenantId} deleted a disabled tenant's keys and their
delegations without publishing anything, so a warm key kept serving from
the gateway's cache until the next UTC midnight. The removal now publishes
a revocation for every delegation the tenant's keys hold, service and
principal-bound alike, in one Swarf request signed by the tenant, before
it deactivates the tenant's did:plc, because Swarf verifies the
revocations against that DID. A failed publish leaves the tenant, its DID
and its keys in place for a retry.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…vated The revocations are published before the did:plc is tombstoned, since Swarf verifies them against it; a removal that tombstoned the DID and then failed in the cascade could not publish again on retry and never finished. Delete now reads the DID's last operation once, up front, and skips the publish and the tombstone when it is already deactivated: the earlier attempt published first. The publish and the removal of the keys' delegations are one delegation-store Replace under the keys' locks, as a key deletion and a principal removal already do, so a failed publish leaves every delegation in place and a successful one leaves none behind for the cascade. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7560b40 to
a780c66
Compare
Makes tenant removal revoke every key's delegations in one Swarf request before the tenant's DID is deactivated, so warm keys stop serving at once. A failed publish leaves everything in place for a retry.
tenant.Service.Deleterevokes and removes all key delegations before deactivating🤖 Generated with Claude Code