refactor(rpc): split the policy write out of the management PUT for the bucket service by pyropy · Pull Request #73 · fil-forge/hilt

pyropy · 2026-09-16T14:12:07Z

Splits the policy write out of the management PUT so the bucket service can use it. /s3/bucket/policy (#89) builds on it. CreateBucket is unchanged: a new bucket has no policy until a PutBucketPolicy writes one.

bucketpolicysvc.Service.Write, split from Put
the bucket service's policyWrites dependency
auth.HeaderValue exported
InvalidBucketPolicy receipt failure

Change log

2a05921: removed the x-bucket-policy create header and its rollback, after RFC 30 moved the console's default policy to a PutBucketPolicy after the create.

References

RFC: rfc: forge S3 tenant IAM fil-one/RFC#30 (commit 113d4ded: policy written after the create)

🤖 Generated with Claude Code

pyropy · 2026-09-16T15:06:12Z

@codex review

chatgpt-codex-connector · 2026-09-16T15:06:34Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-02T09:44:16.193280Z	`8414874`	Manual request

ℹ️ 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" or "@codex security review".

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 · 2026-09-16T15:10:30Z

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 7b99f81938

ℹ️ 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".

pyropy · 2026-09-24T12:29:58Z

@codex review

pyropy · 2026-10-01T11:14:11Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ed6b75de31

ℹ️ 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".

pyropy · 2026-10-02T09:41:10Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 841487419b

ℹ️ 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".

Copilot

Copilot review overview

🟡 Changes recommended

Rollback timeout and deletion ordering can leave a failed creation’s bucket in an inconsistent, unretryable state.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Use a fresh deadline for post-revocation cleanup · New
Delete the bucket before its policy and stop on failure · New

What changed in this PR

Adds signed x-bucket-policy support to CreateBucket, persisting policies and issuing principal grants during bucket creation.

Changes:

Decodes, verifies, validates, and stores header policies.
Adds rollback revocation and InvalidBucketPolicy handling.
Adds unit, wiring, and integration coverage.

File	Description
`pkg/rpc/service/bucket/service.go`	Implements policy handling and rollback.
`pkg/rpc/service/bucket/service_test.go`	Tests creation, policy persistence, and rollback.
`pkg/rpc/service/auth/operation.go`	Exports case-insensitive header lookup.
`pkg/rpc/rpc_test.go`	Updates bucket service construction.
`pkg/rpc/failure.go`	Maps invalid policies to receipt failures.
`pkg/rpc/failure_test.go`	Tests failure-name mapping.
`pkg/rpc/create.go`	Documents handler behavior.
`pkg/fx/rpc_test.go`	Adds policy-service wiring.
`pkg/bucketpolicy/bucketpolicy.go`	Updates package documentation.
`pkg/api/service/bucketpolicy/service.go`	Extracts reusable policy write logic.
`itest/stack_test.go`	Registers the integration scenario.
`itest/iam_test.go`	Tests policy-header behavior end to end.
`AGENTS.md`	Documents the new RPC contract.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…t-policy A CreateBucket request may carry the new bucket's policy as base64 JSON in the x-bucket-policy header. The header must be covered by the request signature and decode as a policy document; a refusal is the InvalidBucketPolicy failure, the name the management API uses. The document is written right after the bucket row through the policy service's Write, the same validation and rotation a management-API policy PUT gets: the principals it names have their keys issued delegations over the new bucket inside the write. The bucket is deleted if that write fails, so no bucket outlives a refused or failed policy. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

When a step after the policy write fails, Create's rollback publishes revocations for the grants that write issued, then deletes the grants, the policy and the bucket row. It did the deletes even when the publish failed, so a proof chain a gateway had already fetched outlived every record that could revoke it. The rollback now returns after a failed tenant-issuer load or publish, leaving the bucket for a DeleteBucket retry, which revokes before it deletes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

policyFromHeader read the header through auth.HeaderValue, which treats an empty value as absent, so a CreateBucket carrying the header with an empty value created a bucket with no policy and skipped the signed-header check. A present but empty or whitespace-only header is now an InvalidBucketPolicy refusal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…tenant key Create's rollback loaded the tenant issuer before it knew whether there was anything to revoke, and returned when the load failed. A policy write that fails on that same key lookup issues no grants, so the rollback left the bucket row behind: retries got BucketAlreadyOwnedByYou and DeleteBucket could not remove the row, needing the key and a Sprue space that was never provisioned. The rollback now lists the bucket's tenant-issued delegations first and loads the issuer and publishes only when there is one; with nothing to revoke it deletes directly. A publish that fails still keeps the bucket for a DeleteBucket retry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Create's rollback ran on a context detached from the request with no deadline, and the Swarf client has no timeout of its own, so a revocation service that accepted the request and never answered hung the create forever. The rollback's context now carries the Swarf batch bound, grant.BatchTimeout, on top of the detached context: a client disconnect still does not abort it, and a publish that times out fails and keeps the bucket for a DeleteBucket retry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The fixture kept the fake Swarf under two fields; the deadline-recording wrapper embeds it, so one field serves both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…s deletes safe A create that failed before storing the bucket's root, whose rollback could not publish the revocations, kept the bucket row for a DeleteBucket retry, but Delete first asked Sprue whether the space was empty, which needs the root proof, so the retry could never succeed and the name stayed taken. Delete now skips the emptiness check when the bucket holds no root: no space a proof can reach exists for it. The rollback's deletes shared the publish's deadline, so a publish returning near it left them an expired context; they now run under a fresh one. They also deleted the policy before the bucket row, so a failed row delete left a live bucket without its policy on the memory backend; they now run inside the policy store's DeleteByBucket callback as Delete's do, delegations and row first, the policy last. Tests: a bucket kept without its root is deleted on retry without a Sprue call and its name is free again; the deletes' deadline is later than the publish's; a row the rollback cannot delete keeps its policy. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

RFC 30 (113d4ded) makes CreateBucket plain again: a bucket's policy is set with PutBucketPolicy, never on the create. Create and its rollback return to their earlier shape (the rollback deletes the bucket row), and the header constant, its decoding, the blank-header refusal, the rollback's revoke and publish, the root check a header-less delete needed, and the header tests and docs go with it. What PutBucketPolicy builds on stays: the policy service's Write, the exported auth.HeaderValue, the bucket service's policyWrites dependency, the InvalidBucketPolicy failure mapping, and the API module in the RPC wiring test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

alanshaw · 2026-10-08T15:06:50Z

+		policies:     policies,
+		uploads:      uploads,
+		revocations:  revocations,
+		policyWrites: policyWrites,


Urm, is this PR needed now? This property seems unused.

I'll fold this it into #89 and close this PR. I left this PR as a standalone when refactoring as it was simpler.

pyropy added this pull request to stack #71 September 16, 2026 14:13

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from e035fcf to 7b99f81 Compare September 16, 2026 14:49

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 7b99f81 to 729b425 Compare September 17, 2026 13:34

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 729b425 to eb735f4 Compare September 21, 2026 11:47

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from eb735f4 to 7ff2638 Compare September 21, 2026 11:50

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch 2 times, most recently from 13fe304 to b73203a Compare September 22, 2026 17:08

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch 2 times, most recently from 4302b89 to 6beeafb Compare September 23, 2026 11:55

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 6beeafb to ff05320 Compare September 23, 2026 11:55

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from ff05320 to 63011b6 Compare September 23, 2026 12:39

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch 2 times, most recently from 639c1f2 to 579d49c Compare September 24, 2026 09:13

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 579d49c to 266e6c7 Compare September 24, 2026 11:35

pyropy removed this pull request from stack #71 September 24, 2026 11:48

pyropy added this pull request to stack #86 September 24, 2026 11:49

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 6f23555 to 553c2d9 Compare September 30, 2026 19:32

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 553c2d9 to 9498ebf Compare September 30, 2026 20:00

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 9498ebf to 64a19bf Compare October 1, 2026 07:29

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 64a19bf to ba1dc86 Compare October 1, 2026 09:05

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from ba1dc86 to ed6b75d Compare October 1, 2026 11:05

chatgpt-codex-connector Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread pkg/rpc/service/bucket/service.go Outdated

Comment thread pkg/rpc/service/bucket/service.go Outdated

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from ed6b75d to f994be5 Compare October 1, 2026 12:15

pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from f994be5 to 8414874 Compare October 1, 2026 14:37

chatgpt-codex-connector Bot reviewed Oct 2, 2026

View reviewed changes

Comment thread pkg/rpc/service/bucket/service.go Outdated

pyropy marked this pull request as ready for review October 2, 2026 12:12

Copilot AI balanced review requested due to automatic review settings October 2, 2026 12:12

Copilot started reviewing on behalf of pyropy October 2, 2026 12:13 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread pkg/rpc/service/bucket/service.go Outdated

Comment thread pkg/rpc/service/bucket/service.go Outdated

pyropy and others added 8 commits October 5, 2026 17:00

test(rpc): hold the create-with-policy fixture's publisher once

87232f7

The fixture kept the fake Swarf under two fields; the deadline-recording wrapper embeds it, so one field serves both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

alanshaw requested changes Oct 8, 2026

View reviewed changes

Conversation

pyropy commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change log

References

Uh oh!

pyropy commented Sep 16, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot commented Sep 16, 2026

Uh oh!

pyropy commented Sep 24, 2026

Uh oh!

pyropy commented Oct 1, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

pyropy commented Oct 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

alanshaw Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

pyropy Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pyropy commented Sep 16, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 16, 2026 •

edited

Loading