amadan
plan: claim Plan 2 rev 4, Codex round 3
A delete-marker COPY is 400 InvalidRequest (mapped by message), and an empty version id is ErrNoVersionID from every exact-version call.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+45
−5
docs/superpowers/plans/2026-10-05-host-lifecycle-claim-2-versioned-storage.md+45 −5
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 b8a1dbd0b..a5a89966f 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 |
| @@ -45,7 +45,8 @@ dependency); `net/http/httptest`; OpenTofu. |
| **Spec:** `docs/superpowers/specs/2026-10-02-host-lifecycle-claim-design.md` |
| 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). |
| +answers round 2 (two P2s, two P3s); rev 4 answers round 3 (one P2, |
| +one P3). |
| **Depends on:** nothing. |
| @@ -67,8 +68,18 @@ answers round 2 (two P2s, two P3s). |
| entries. |
| - **Delete markers can't be read or copied.** `GetVersion`, |
| `OpenVersion` or `CopyVersion` on a version id that is a delete marker |
| - returns `ErrIsDeleteMarker` (S3: 405 with `x-amz-delete-marker: true`). |
| + returns `ErrIsDeleteMarker`. On the wire a GET answers 405 with |
| + `x-amz-delete-marker: true`; a COPY answers 400 `InvalidRequest` with |
| + the message "The source of a copy request may not specifically refer |
| + to a delete marker by version id." `CopyVersion` maps that one message |
| + (matched on "delete marker") to `ErrIsDeleteMarker`; any other |
| + `InvalidRequest` stays a generic error. The COPY shape is from |
| + LocalStack's provider, not verified on AWS (§13 has no row for it); an |
| + unmatched message is still an error, never success. |
| A missing version returns `lease.ErrMirrorNotFound`. |
| +- **An empty version id is `ErrNoVersionID`** from `GetVersion`, |
| + `OpenVersion`, `CopyVersion` and `DeleteVersion` alike, in both |
| + implementations, with no request sent. |
| - **Copy rules.** |
| - The source is read by version id. `CopySource` is |
| `httpbinding.EscapePath(srcBucket+"/"+srcKey, false)` followed by |
| @@ -491,11 +502,20 @@ func RunVersionedStoreContract(t *testing.T, mk Maker) { |
| if err := st.DeleteVersion(ctx, b, "k", ""); !errors.Is(err, lease.ErrNoVersionID) { |
| t.Fatalf("empty version id = %v, want ErrNoVersionID", err) |
| } |
| + if _, err := st.GetVersion(ctx, b, "k", ""); !errors.Is(err, lease.ErrNoVersionID) { |
| + t.Fatalf("GetVersion(\"\") = %v, want ErrNoVersionID", err) |
| + } |
| + if _, err := st.OpenVersion(ctx, b, "k", ""); !errors.Is(err, lease.ErrNoVersionID) { |
| + t.Fatalf("OpenVersion(\"\") = %v, want ErrNoVersionID", err) |
| + } |
| + if _, _, err := st.CopyVersion(ctx, b, "k", "", b, "k2", false); !errors.Is(err, lease.ErrNoVersionID) { |
| + t.Fatalf("CopyVersion(\"\") = %v, want ErrNoVersionID", err) |
| + } |
| if m.Snapshot() != before { |
| t.Fatal("state changed on an empty-version delete") |
| } |
| if served != nil && served() != reqs { |
| - t.Fatal("a request was sent for an empty-version delete") |
| + t.Fatal("a request was sent for an empty version id") |
| } |
| _ = v1 |
| }) |
| @@ -817,6 +837,9 @@ func (f *VersionedBucket) ListVersions(ctx context.Context, bucket, prefix strin |
| } |
| func (f *VersionedBucket) read(bucket, key, versionID string) ([]byte, error) { |
| + if versionID == "" { |
| + return nil, lease.ErrNoVersionID |
| + } |
| if err := f.known(bucket); err != nil { |
| return nil, err |
| } |
| @@ -850,6 +873,9 @@ func (f *VersionedBucket) OpenVersion(ctx context.Context, bucket, key, versionI |
| } |
| func (f *VersionedBucket) copyLocked(srcBucket, srcKey, srcVersionID, dstBucket, dstKey string, createOnly bool) (string, string, error) { |
| + if srcVersionID == "" { |
| + return "", "", lease.ErrNoVersionID |
| + } |
| if err := f.known(srcBucket); err != nil { |
| return "", "", err |
| } |
| @@ -1104,12 +1130,17 @@ type listVersionsResult struct { |
| Entries []any |
| } |
| -func s3err(w http.ResponseWriter, status int, code string) { |
| +func s3err(w http.ResponseWriter, status int, code string) { s3errMsg(w, status, code, code) } |
| + |
| +func s3errMsg(w http.ResponseWriter, status int, code, msg string) { |
| w.Header().Set("Content-Type", "application/xml") |
| w.WriteHeader(status) |
| - io.WriteString(w, "<Error><Code>"+code+"</Code><Message>"+code+"</Message></Error>") |
| + io.WriteString(w, "<Error><Code>"+code+"</Code><Message>"+msg+"</Message></Error>") |
| } |
| +// copyMarkerMsg is S3's answer to a COPY naming a delete marker's version. |
| +const copyMarkerMsg = "The source of a copy request may not specifically refer to a delete marker by version id." |
| + |
| // writeErr maps a model error onto the S3 response real S3 gives. |
| func writeErr(w http.ResponseWriter, err error) { |
| switch { |
| @@ -1255,6 +1286,10 @@ func (f *VersionedBucket) serveCopy(w http.ResponseWriter, r *http.Request, buck |
| } |
| sb, sk, _ := strings.Cut(unesc, "/") |
| id, etag, err := f.copyLocked(sb, sk, sq.Get("versionId"), bucket, key, r.Header.Get("If-None-Match") == "*") |
| + if errors.Is(err, lease.ErrIsDeleteMarker) { |
| + s3errMsg(w, http.StatusBadRequest, "InvalidRequest", copyMarkerMsg) // a COPY is 400, not GET's 405 |
| + return |
| + } |
| if err != nil { |
| writeErr(w, err) |
| return |
| @@ -1442,9 +1477,12 @@ func (s *versionedS3) CopyVersion(ctx context.Context, srcBucket, srcKey, srcVer |
| out, err := c.CopyObject(ctx, in) |
| if err != nil { |
| code, status := apiCode(err) |
| + var ae smithy.APIError |
| switch { |
| case createOnly && (code == "PreconditionFailed" || status == http.StatusPreconditionFailed): |
| return "", "", ErrDestinationExists |
| + case code == "InvalidRequest" && errors.As(err, &ae) && strings.Contains(strings.ToLower(ae.ErrorMessage()), "delete marker"): |
| + return "", "", ErrIsDeleteMarker |
| default: |
| if m := mapReadErr(err); m != err { |
| return "", "", m |
| @@ -1631,6 +1669,8 @@ Each finding from the rev 1 review, and where it's fixed: |
| | 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 | |
| +| R3 P2, marker COPY is 400 InvalidRequest | the server answers it; `CopyVersion` maps the delete-marker message only | |
| +| R3 P3, empty-id parity | `ErrNoVersionID` guards in the model's `read` and `copyLocked`; contract checks all four with zero requests | |
| | P3, consumer contract | `NewVersionedS3`, `OpenVersion`, and destination recovery via `ListVersions` | |
| The types are consistent throughout: `VersionedStore`, `ObjectVersion`, |