amadan
plan: claim Plan 2 rev 3, Codex round 2
Constructor arity, NoSuchBucket kept as an error, per-endpoint bucket restriction with a wrong-endpoint test, noncurrent-delete contract case, §13 row 9 narrowed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+87
−16
docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md+87 −16
CI passed — run details
diff --git a/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md b/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md| index 8332e02c5..b8a1dbd0b 100644 |
| --- a/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md |
| +++ b/docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md |
| @@ -43,8 +43,9 @@ exist); smithy-go `encoding/httpbinding` (already a transitive |
| dependency); `net/http/httptest`; OpenTofu. |
| **Spec:** `docs/superpowers/specs/2026-10-02-host-lifecycle-claim-design.md` |
| -rev 6.12. Read C3, §6.2, §6.5 and §13 before starting. Rev 2 of this |
| -plan answers Codex's review of rev 1: two P1s and seven P2s. |
| +rev 6.13. Read C3, §6.2, §6.5 and §13 before starting. Rev 2 of this |
| +plan answered Codex's review of rev 1 (two P1s, seven P2s); rev 3 |
| +answers round 2 (two P2s, two P3s). |
| **Depends on:** nothing. |
| @@ -110,7 +111,7 @@ plan answers Codex's review of rev 1: two P1s and seven P2s. |
| | 6, `PutObject If-Match` | **no**: claim CASes go through `internal/release.Store`, which already has it | |
| | 7, `DeleteObject If-Match` | **no**: `release.DeleteIfMatch` already covers it | |
| | 8, signature age | **no**: Plan 3/6's staging writer owns signing at the deadline check (CC19) | |
| -| 9, multipart to single-part ETag on copy | yes | |
| +| 9, multipart to single-part ETag on copy | the ETag half only; the conditional-copy half (`CopySourceIfMatch` rejection) is out of scope, since no caller copies conditionally on the source | |
| ## Review Focus |
| @@ -167,7 +168,6 @@ import ( |
| "io" |
| "time" |
| - awsconfig "github.com/aws/aws-sdk-go-v2/config" |
| awss3 "github.com/aws/aws-sdk-go-v2/service/s3" |
| ) |
| @@ -231,7 +231,7 @@ func NewVersionedS3(ctx context.Context, regionByBucket map[string]string) (Vers |
| c, ok := byRegion[region] |
| if !ok { |
| var err error |
| - if c, err = newLeaserClient(ctx, region); err != nil { |
| + if c, _, err = newLeaserClient(ctx, region); err != nil { |
| return nil, fmt.Errorf("lease: versioned client for %s: %w", region, err) |
| } |
| byRegion[region] = c |
| @@ -271,14 +271,11 @@ func (s *versionedS3) PutIfAbsent(ctx context.Context, bucket, key string, body |
| } |
| var _ VersionedStore = (*versionedS3)(nil) |
| -var _ = awsconfig.LoadDefaultConfig // newLeaserClient's dependency; remove if unused |
| ``` |
| -`newLeaserClient(ctx, region)` exists in `internal/lease/endpoint.go` |
| -(around lines 99-110). If its real signature differs, adapt the call and |
| -keep its semantics: default chain, `s3compat.Options()`, and the |
| -`CARLOS_S3_ENDPOINT` override. Delete the `awsconfig` placeholder line |
| -if the import isn't needed. |
| +`newLeaserClient(ctx, region) (*awss3.Client, string, error)` is in |
| +`internal/lease/endpoint.go:99` (default chain, `s3compat` options, the |
| +`CARLOS_S3_ENDPOINT` override); the middle result is discarded. |
| - [ ] **Step 2: Write `export_test.go`** |
| @@ -503,6 +500,34 @@ func RunVersionedStoreContract(t *testing.T, mk Maker) { |
| _ = v1 |
| }) |
| + t.Run("delete a noncurrent version keeps the current one", func(t *testing.T) { |
| + st, m, b, _ := mk(t) |
| + v1 := m.Put(b, "k", []byte("one")) |
| + v2 := m.Put(b, "k", []byte("two")) |
| + if err := st.DeleteVersion(ctx, b, "k", v1); err != nil { |
| + t.Fatal(err) |
| + } |
| + if cur, ok := m.Current(b, "k"); !ok || string(cur) != "two" || m.LatestID(b, "k") != v2 { |
| + t.Fatalf("current after a noncurrent delete = %q %v %s", cur, ok, m.LatestID(b, "k")) |
| + } |
| + if _, err := st.GetVersion(ctx, b, "k", v1); !errors.Is(err, lease.ErrMirrorNotFound) { |
| + t.Fatalf("deleted version still readable: %v", err) |
| + } |
| + }) |
| + |
| + t.Run("a missing bucket is an error, never absence", func(t *testing.T) { |
| + st, _, _, _ := mk(t) |
| + if err := st.DeleteVersion(ctx, "nobucket", "k", "v000001"); err == nil { |
| + t.Error("delete in a missing bucket succeeded") |
| + } |
| + if _, err := st.GetVersion(ctx, "nobucket", "k", "v000001"); err == nil || errors.Is(err, lease.ErrMirrorNotFound) { |
| + t.Errorf("get in a missing bucket = %v", err) |
| + } |
| + if _, err := st.ListVersions(ctx, "nobucket", ""); err == nil { |
| + t.Error("list of a missing bucket succeeded") |
| + } |
| + }) |
| + |
| t.Run("put if absent", func(t *testing.T) { |
| st, m, b, _ := mk(t) |
| if v, err := st.PutIfAbsent(ctx, b, "m/closed.json", []byte("{}")); err != nil || v == "" { |
| @@ -973,13 +998,16 @@ func wireStore(t *testing.T) (lease.VersionedStore, *leasetest.VersionedBucket, |
| m := leasetest.NewVersionedBucket() |
| m.AddBucket("data") |
| m.AddBucket("other") |
| - h1, n1 := leasetest.NewS3Server(m) |
| - h2, n2 := leasetest.NewS3Server(m) |
| + h1, n1 := leasetest.NewS3Server(m, "data", "nobucket") |
| + h2, n2 := leasetest.NewS3Server(m, "other") |
| s1, s2 := httptest.NewServer(h1), httptest.NewServer(h2) |
| t.Cleanup(s1.Close) |
| t.Cleanup(s2.Close) |
| + // "nobucket" is bound but absent from the model: the server answers |
| + // NoSuchBucket, for the missing-bucket contract case. |
| st := lease.NewVersionedS3WithClients(map[string]*awss3.Client{ |
| "data": wireClient(t, s1.URL), "other": wireClient(t, s2.URL), |
| + "nobucket": wireClient(t, s1.URL), |
| }) |
| return st, m, func() int { return n1() + n2() } |
| } |
| @@ -991,6 +1019,31 @@ func TestVersionedS3Contract(t *testing.T) { |
| }) |
| } |
| +// Each bucket reaches only its own endpoint: a client bound to the wrong |
| +// endpoint gets 301 and the operation fails. |
| +func TestVersionedS3RoutesEachBucketToItsEndpoint(t *testing.T) { |
| + ctx := context.Background() |
| + st, m, _ := wireStore(t) |
| + for _, b := range []string{"data", "other"} { |
| + v := m.Put(b, "k", []byte(b)) |
| + if vs, err := st.ListVersions(ctx, b, ""); err != nil || len(vs) != 1 { |
| + t.Fatalf("list %s = %v, %v", b, vs, err) |
| + } |
| + if err := st.DeleteVersion(ctx, b, "k", v); err != nil { |
| + t.Fatalf("delete %s = %v", b, err) |
| + } |
| + } |
| + m2 := leasetest.NewVersionedBucket() |
| + m2.AddBucket("data") |
| + h, _ := leasetest.NewS3Server(m2, "other") |
| + srv := httptest.NewServer(h) |
| + t.Cleanup(srv.Close) |
| + wrong := lease.NewVersionedS3WithClients(map[string]*awss3.Client{"data": wireClient(t, srv.URL)}) |
| + if _, err := wrong.ListVersions(ctx, "data", ""); err == nil { |
| + t.Fatal("a wrong-region endpoint answered") |
| + } |
| +} |
| + |
| func TestVersionedS3RefusesUnboundBucket(t *testing.T) { |
| st, _, _ := wireStore(t) |
| if _, err := st.ListVersions(context.Background(), "unbound", ""); err == nil { |
| @@ -1074,9 +1127,16 @@ func writeErr(w http.ResponseWriter, err error) { |
| // NewS3Server speaks the subset of the S3 wire protocol VersionedStore uses, |
| // path-style, over f, holding f.mu for each request. The second result |
| -// counts requests served. |
| -func NewS3Server(f *VersionedBucket) (http.Handler, func() int) { |
| +// counts requests served. With serves non-empty, the endpoint answers only |
| +// for those request buckets (a regional endpoint); any other bucket gets |
| +// 301 PermanentRedirect, as S3 answers a wrong-region request. A copy's |
| +// source bucket may be any bucket in f. |
| +func NewS3Server(f *VersionedBucket, serves ...string) (http.Handler, func() int) { |
| var n atomic.Int64 |
| + allowed := map[string]bool{} |
| + for _, b := range serves { |
| + allowed[b] = true |
| + } |
| h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| n.Add(1) |
| bucket, rawKey, _ := strings.Cut(strings.TrimPrefix(r.URL.EscapedPath(), "/"), "/") |
| @@ -1086,6 +1146,10 @@ func NewS3Server(f *VersionedBucket) (http.Handler, func() int) { |
| return |
| } |
| q := r.URL.Query() |
| + if len(allowed) > 0 && !allowed[bucket] { |
| + s3err(w, http.StatusMovedPermanently, "PermanentRedirect") |
| + return |
| + } |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| if err := f.known(bucket); err != nil { |
| @@ -1314,9 +1378,13 @@ func apiCode(err error) (string, int) { |
| return code, status |
| } |
| +// mapReadErr normalizes only object or version absence. A missing bucket |
| +// (NoSuchBucket, also a 404) stays an error: it is never "already gone". |
| func mapReadErr(err error) error { |
| code, status := apiCode(err) |
| switch { |
| + case code == "NoSuchBucket": |
| + return err |
| case code == "NoSuchKey" || code == "NoSuchVersion" || code == "NotFound" || status == http.StatusNotFound: |
| return ErrMirrorNotFound |
| case status == http.StatusMethodNotAllowed: |
| @@ -1559,7 +1627,10 @@ Each finding from the rev 1 review, and where it's fixed: |
| | P2, the false-positive safety test | request count plus a full state snapshot | |
| | P2, sibling cleanup | assigned to the destination region; no sibling grants change | |
| | P2, missing helpers | complete helpers, with locking stated | |
| -| P3, §13 coverage | the coverage table | |
| +| P3, §13 coverage | the coverage table (row 9 narrowed, rev 3), and a noncurrent-delete case | |
| +| R2 P2, constructor arity | `c, _, err = newLeaserClient(...)` | |
| +| R2 P2, missing bucket read as absence | `mapReadErr` keeps `NoSuchBucket`; a missing-bucket contract case | |
| +| R2 P3, ineffective routing test | per-endpoint bucket restriction (301), a wrong-endpoint test | |
| | P3, consumer contract | `NewVersionedS3`, `OpenVersion`, and destination recovery via `ListVersions` | |
| The types are consistent throughout: `VersionedStore`, `ObjectVersion`, |