amadan
release: stableTag stops claiming a tag that names a different iteration
Final whole-branch review fix wave, four items: - stableTag (internal/release/release.go): when EnsureTag returns an EXISTING tag whose Release differs from the version just promoted (tag 0.5.0 -> 0.5.0-1, stable now serves 0.5.0-2), res.Tag was set to the tag's Version regardless — a false "tagged 0.5.0" for a promotion the tag does not name. Now that case sets res.TagWarning naming the iteration the tag actually carries and leaves res.Tag empty. - ladder_test.go: pins the fix above (a second iteration under an already-tagged target warns instead of claiming the tag) and pins that a hotfix straight to stable still cuts the version's tag. - apiversion_test.go: pins the non-member byte-identical 404 on the PUT version-target door, the one handler in this file with a write-budget gate after apiAccountApp — mirrors the GET door's existing coverage. - apiversion.go: comments no longer claim a `carlos tags` CLI verb exists; reworded to say the tags routes feed `carlos channels`' annotations and the console's releases page today, with `carlos tags` noted as a possible future verb.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+120
−11
internal/console/apiversion.go+15 −11internal/console/apiversion_test.go+31 −0internal/release/ladder_test.go+68 −0internal/release/release.go+6 −0
diff --git a/internal/console/apiversion.go b/internal/console/apiversion.go| index f1c4fc3dd..32038b9ae 100644 |
| --- a/internal/console/apiversion.go |
| +++ b/internal/console/apiversion.go |
| @@ -1,9 +1,11 @@ |
| // The CLI's version-target, tags and releases API (release versioning spec, |
| -// 2026-08-08): `carlos version target`, `carlos tags`, and the releases list |
| -// behind `carlos releases`. Every handler opens with s.apiAccountApp — the |
| -// SAME account/app 404 apicli.go's other doors give, so a non-member cannot |
| -// tell an app they don't belong to apart from one that does not exist by |
| -// probing any of these five routes either. |
| +// 2026-08-08): `carlos version target` and the releases list behind `carlos |
| +// releases`. The tags routes have no CLI verb of their own yet — their rows |
| +// feed `carlos channels`' tag annotations and the console's releases page, |
| +// and a future `carlos tags` verb may read them directly. Every handler |
| +// opens with s.apiAccountApp — the SAME account/app 404 apicli.go's other |
| +// doors give, so a non-member cannot tell an app they don't belong to apart |
| +// from one that does not exist by probing any of these five routes either. |
| package console |
| import ( |
| @@ -103,7 +105,7 @@ func (s *Server) handleAPIVersionTargetSet(w http.ResponseWriter, r *http.Reques |
| writeJSON(w, http.StatusOK, map[string]string{"target": target}) |
| } |
| -// tagListRow is one row of `carlos tags` — deliberately WITHOUT Changelog, |
| +// tagListRow is one row of the tags list — deliberately WITHOUT Changelog, |
| // which can run to several KB per tag and is not what a list view needs; |
| // GetTag (below) is the door onto a single tag's full changelog. |
| type tagListRow struct { |
| @@ -113,8 +115,10 @@ type tagListRow struct { |
| Actor string `json:"actor,omitempty"` |
| } |
| -// handleAPITagsList answers `carlos tags`: every tag this app has cut, |
| -// semver-descending — release.ListTags' own order. |
| +// handleAPITagsList answers the tags list: every tag this app has cut, |
| +// semver-descending — release.ListTags' own order. `carlos channels` reads |
| +// this to annotate its TAG column and the console's releases page reads it |
| +// for the same join; there is no standalone `carlos tags` verb yet. |
| func (s *Server) handleAPITagsList(w http.ResponseWriter, r *http.Request) { |
| _, acct, app, ok := s.apiAccountApp(w, r) |
| if !ok { |
| @@ -141,9 +145,9 @@ func (s *Server) handleAPITagsList(w http.ResponseWriter, r *http.Request) { |
| writeJSON(w, http.StatusOK, map[string]any{"tags": out}) |
| } |
| -// handleAPITagGet answers `carlos tags <version>`: one tag, in full — |
| -// including the changelog the list route deliberately omits. A version this |
| -// app never tagged is a 404 naming the version, distinct from |
| +// handleAPITagGet answers a single tag, in full — including the changelog |
| +// the list route deliberately omits; the console's releases page renders it. |
| +// A version this app never tagged is a 404 naming the version, distinct from |
| // apiAccountApp's byte-identical membership 404 above it: a member is told |
| // exactly what is missing, because by the time this line runs membership is |
| // already proved and there is nothing left to protect by staying vague. |
diff --git a/internal/console/apiversion_test.go b/internal/console/apiversion_test.go| index 336f94ccb..4436884be 100644 |
| --- a/internal/console/apiversion_test.go |
| +++ b/internal/console/apiversion_test.go |
| @@ -145,6 +145,37 @@ func TestAPIVersionTargetSetStoreFailure(t *testing.T) { |
| } |
| } |
| +// TestAPIVersionTargetSetNonMemberByteIdentical404: the PUT door is the one |
| +// handler in this file with a second gate after apiAccountApp — the write |
| +// budget (handleAPIVersionTargetSet's s.cli.write.allow) — and that gate must |
| +// never run, let alone answer, before membership is settled: a non-member's |
| +// PUT has to meet the exact byte-identical 404 an unknown app's PUT gets, |
| +// the same property TestAPIVersionTargetGetSet already pins for the GET door. |
| +// |
| +// Mutation-mindedness: move the write-budget check ahead of apiAccountApp (or |
| +// give it its own error body) and this is the test that notices — a |
| +// non-member would get a 429 or a differently-worded 404 instead of the |
| +// account-existence-blind refusal every other apicli.go door gives. |
| +func TestAPIVersionTargetSetNonMemberByteIdentical404(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + now := time.Now() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "km", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + eve := cliToken(t, fx, "eve@keymail.dev", "code-version-target-put-eve") |
| + token := cliToken(t, fx, "paul@keymail.dev", "code-version-target-put-member") |
| + |
| + body := map[string]string{"target": "0.5.0"} |
| + statusMember, bodyMember := putRaw(t, fx.ts.URL+"/api/cli/apps/bab/km/version-target", body, eve) |
| + statusUnknown, bodyUnknown := putRaw(t, fx.ts.URL+"/api/cli/apps/bab/no-such-app/version-target", body, token) |
| + if statusMember != http.StatusNotFound || statusUnknown != http.StatusNotFound { |
| + t.Fatalf("non-member/unknown status = %d/%d, want both 404", statusMember, statusUnknown) |
| + } |
| + if bodyMember != bodyUnknown { |
| + t.Fatalf("non-member body %q != unknown-app body %q — the 404 is not byte-identical", bodyMember, bodyUnknown) |
| + } |
| +} |
| + |
| // TestAPITagsListAndGet: the tags list omits the changelog, semver-descending |
| // (0.10.0 above 0.6.0, which string order gets wrong); the single-tag door |
| // carries the full changelog; a version never tagged is a 404. |
diff --git a/internal/release/ladder_test.go b/internal/release/ladder_test.go| index 9014735b4..54f5d4028 100644 |
| --- a/internal/release/ladder_test.go |
| +++ b/internal/release/ladder_test.go |
| @@ -314,3 +314,71 @@ func TestStablePromoteTagFailureIsAWarningNotAnError(t *testing.T) { |
| t.Fatalf("stable pointer = %+v, %v", p, err) |
| } |
| } |
| + |
| +// A second iteration under a target already tagged by an earlier one must not |
| +// claim the earlier tag as its own. 0.5.0 got tagged at 0.5.0-1; when 0.5.0-2 |
| +// later reaches stable, EnsureTag finds that existing tag (it names the same |
| +// target) and hands it back unchanged — the promotion is real, but it did not |
| +// cut or move the tag, so res.Tag must stay empty and the operator must be |
| +// told which iteration the tag actually names. |
| +// |
| +// Mutation-mindedness: report res.Tag = tag.Version regardless of tag.Release |
| +// and this is the test that catches it — a false "tagged 0.5.0" for a |
| +// promotion the tag does not describe. |
| +func TestStablePromoteAfterEarlierTagWarnsInsteadOfClaimingIt(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + |
| + shipLabelledIteration(t, s, "bab", "km", "0.5.0-1", "first", "- one") |
| + climbToBeta(t, ctx, s, priv, "bab", "km", "0.5.0-1") |
| + if _, err := PromoteOnLadder(ctx, s, priv, "bab", "km", "stable", "0.5.0-1", PromoteOptions{}); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + shipLabelledIteration(t, s, "bab", "km", "0.5.0-2", "second", "- two") |
| + climbToBeta(t, ctx, s, priv, "bab", "km", "0.5.0-2") |
| + |
| + res, err := PromoteOnLadder(ctx, s, priv, "bab", "km", "stable", "0.5.0-2", PromoteOptions{}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if res.Tag != "" { |
| + t.Fatalf("res.Tag = %q; a tag belonging to a different iteration must not be claimed", res.Tag) |
| + } |
| + if !strings.Contains(res.TagWarning, "already tagged at 0.5.0-1") { |
| + t.Fatalf("TagWarning = %q; want it to name the iteration the tag actually carries", res.TagWarning) |
| + } |
| + // The promotion itself is real regardless of the tag warning. |
| + p, err := Channel(ctx, s, pub, "bab", "km", "stable") |
| + if err != nil || p.Version != "0.5.0-2" { |
| + t.Fatalf("stable pointer = %+v, %v", p, err) |
| + } |
| +} |
| + |
| +// A hotfix straight to stable — no rungs climbed — still cuts the version's |
| +// tag: the ladder gate is bypassed, but stable promotion's tag-cutting is not |
| +// gated on how the version got there. |
| +func TestHotfixToStableCutsTag(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, _ := testKeys(t) |
| + |
| + shipLabelledIteration(t, s, "bab", "km", "0.7.0-1", "urgent fix", "- patched the leak") |
| + |
| + res, err := PromoteOnLadder(ctx, s, priv, "bab", "km", "stable", "0.7.0-1", |
| + PromoteOptions{Hotfix: true, Actor: "paul@example.com"}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if !res.Hotfix { |
| + t.Fatalf("res.Hotfix = false for a hotfix promotion") |
| + } |
| + if res.Tag != "0.7.0" { |
| + t.Fatalf("res.Tag = %q, TagWarning = %q; want 0.7.0 cut even via hotfix", res.Tag, res.TagWarning) |
| + } |
| + tag, err := GetTag(ctx, s, "bab", "km", "0.7.0") |
| + if err != nil || tag.Release != "0.7.0-1" { |
| + t.Fatalf("tag after hotfix promote: %+v, %v", tag, err) |
| + } |
| +} |
diff --git a/internal/release/release.go b/internal/release/release.go| index 3fc8bee63..e76ec9743 100644 |
| --- a/internal/release/release.go |
| +++ b/internal/release/release.go |
| @@ -766,6 +766,12 @@ func stableTag(ctx context.Context, s Store, acct, app, version, actor string, r |
| res.TagWarning = "promoted, but cutting the tag failed: " + err.Error() |
| case tag == nil: |
| res.TagWarning = "no tag cut: " + version + " is not a <target>-<n> iteration" |
| + case tag.Release != version: |
| + // EnsureTag found an existing tag, but it names a DIFFERENT iteration |
| + // than the one just promoted (tag 0.5.0 -> 0.5.0-1, stable now serves |
| + // 0.5.0-2): the tag does not describe this promotion, so it is not |
| + // this promotion's tag. |
| + res.TagWarning = fmt.Sprintf("%s is already tagged at %s — this promotion carries no tag", tag.Version, tag.Release) |
| default: |
| res.Tag = tag.Version |
| } |