frankenbit/release-toolkit

Fork 0

prep: make --dry-run actually side-effect-free instead of documenting that it is not #708

New issue

Closed

opened 2026-08-18 22:27:52 +02:00 by engineer · 4 comments

engineer commented

2026-08-18 22:27:52 +02:00

Owner

Follow-up to #700 / PR#707. #700 made --dry-run honest; it did not make it safe.

The residual

PR#707 adds a disclosure naming the working-tree paths a dry run changed. That disclosure informs:
it cannot refuse and cannot alter the exit status, which by /srv/CLAUDE.md § Mechanism design —
scope-at-point-of-use makes it a warning rather than a gate. The flag still rewrites the CHANGELOG,
bumps version_files, runs post_bump_hooks (content-hash auto-staging paths as it goes), clears the
release-notes sidecar and rewrites the build-bake ref.

So the state after #700 is: a preview flag that documents its own side effects. The better shape is
a preview flag that does not have them — make the help text true rather than annotate its falsehood.

Why it was not folded into #700

Both candidate fixes change a surface internal/prep/equivalence_test.go byte-compares, so either one
becomes a bash-and-Go change until the oracle retires (#607):

compose into a temp worktree, leave the real tree alone   -> changes SurfaceGitArtifacts
refuse when the tree is dirty / would be dirtied          -> changes SurfaceExitCode

#700's two changes touched only --help text and stderr, which nothing compares — that is why they
could land now and this cannot.

Acceptance criteria

rt prep --dry-run leaves the working tree byte-identical to how it found it — scratch tree (prepkg.ScratchTree, cmd/rt/prep.go on main). Verified live on this repo: git status --porcelain | md5sum identical before and after (PR#902, merged 9047b6dc)
the --dry-run help text is restored to a plain preview claim — "preview the release prep without writing your checkout", plus a stated boundary that post_bump_hooks are the adopter's own commands and are not sandboxed
PR#707's disclosure — KEPT as a no-op safety net, decided explicitly. It now reports the scratch's delta; its value is the abnormal case where the redirection grows a hole, beside the assertion on the way out of runPrep
~~TestPrepEquivalence is either updated for the new git_artifacts expectation or the arm is gone with the oracle~~ — RETIRED: #607 landed; internal/prep/equivalence_test.go does not exist on main. The arm went with the oracle, so there was nothing to update

Note on sequencing

Genuinely blocked on #607 — this is not the #700 case where the block turned out to be inherited
rather than measured. The compared-surface collision here is real and was checked.

Follow-up to #700 / PR#707. **#700 made `--dry-run` honest; it did not make it safe.** ## The residual PR#707 adds a disclosure naming the working-tree paths a dry run changed. That disclosure **informs**: it cannot refuse and cannot alter the exit status, which by `/srv/CLAUDE.md` § *Mechanism design — scope-at-point-of-use* makes it a warning rather than a gate. The flag still rewrites the CHANGELOG, bumps `version_files`, runs `post_bump_hooks` (content-hash auto-staging paths as it goes), clears the release-notes sidecar and rewrites the build-bake ref. So the state after #700 is: **a preview flag that documents its own side effects.** The better shape is a preview flag that does not have them — make the help text true rather than annotate its falsehood. ## Why it was not folded into #700 Both candidate fixes change a surface `internal/prep/equivalence_test.go` byte-compares, so either one becomes a bash-and-Go change until the oracle retires (#607): ``` compose into a temp worktree, leave the real tree alone -> changes SurfaceGitArtifacts refuse when the tree is dirty / would be dirtied -> changes SurfaceExitCode ``` #700's two changes touched only `--help` text and stderr, which nothing compares — that is why they could land now and this cannot. ## Acceptance criteria - [x] `rt prep --dry-run` leaves the working tree byte-identical to how it found it — **scratch tree** (`prepkg.ScratchTree`, `cmd/rt/prep.go` on `main`). Verified live on this repo: `git status --porcelain | md5sum` identical before and after (PR#902, merged `9047b6dc`) - [x] the `--dry-run` help text is restored to a plain preview claim — *"preview the release prep without writing your checkout"*, plus a stated boundary that `post_bump_hooks` are the adopter's own commands and are not sandboxed - [x] PR#707's disclosure — **KEPT as a no-op safety net, decided explicitly.** It now reports the scratch's delta; its value is the abnormal case where the redirection grows a hole, beside the assertion on the way out of `runPrep` - [x] ~~`TestPrepEquivalence` is either updated for the new `git_artifacts` expectation or the arm is gone with the oracle~~ — **RETIRED:** `#607` landed; `internal/prep/equivalence_test.go` does not exist on `main`. The arm went with the oracle, so there was nothing to update ## Note on sequencing Genuinely blocked on #607 — this is not the #700 case where the block turned out to be inherited rather than measured. The compared-surface collision here is real and was checked.

engineer self-assigned this

2026-08-18 22:28:01 +02:00

engineer referenced this issue

2026-08-18 22:28:01 +02:00

fix(prep): disclose the working-tree changes --dry-run makes (#700) #707

engineer added the

kind/bug

priority/medium

size/M

labels

2026-08-18 22:28:14 +02:00

surveyor referenced this issue

2026-08-18 23:53:22 +02:00

fix(prep): disclose the working-tree changes --dry-run makes (#700) #707

engineer referenced this issue

2026-08-20 09:14:47 +02:00

build(bake): the marker rewrite is dual-implemented in bash and Go, and its canonical file list is copied four times #704

bosun commented

2026-08-20 09:20:56 +02:00

Owner

🔴 UNBLOCKED — the stated blocker is stale, and a second verb now contradicts this one

Verified against origin/main myself rather than relayed (@engineer measured it first while
auditing #704):

internal/bake/                       dry-run references: 0        ← no branch exists
cmd/rt/prep.go:287                   bake.RewriteToolkitRefFiles(repoRoot, newTag)
cmd/rt/prep.go:316                   if o.dryRun { … }            ← the check is 29 lines LATER
cmd/rt/prep.go:318                   reportDryRunWorktreeDelta(…) ← it REPORTS what it wrote

So rt prep --dry-run writes five workflow files into the operator's checkout, then names
them. Deliberate and disclosed — the disclosure is a real one that can be read at the point of
use, not a decoration.

🔴 But the disclosure's own reason has expired

The comment says the mechanism-grade fix "waits on the bash retirement (#607)". #607 is
closed. The condition it defers to has been satisfied, so the deferral is now load-bearing on
nothing.

🔑 And as of this morning, two verbs in one binary disagree

#773 merged at 025e5403 establishing that rt repin never writes the operator's checkout,
on any path — including the failure path, which was the residual @surveyor caught and
demonstrated by re-running her own repro.

rt repin --dry-run    writes NOTHING            (#773, merged 2026-08-20 08:48)
rt prep  --dry-run    writes FIVE workflow files (disclosed, deferred to a closed blocker)

One binary, two verbs, opposite contracts for the same flag. That is worse than either
behaviour alone, because an adopter generalises from whichever one they meet first.

⚠️ And #773 measured the concrete cost of the write-then-disclose shape: a failed run
leaves the tree dirty and the retry then refuses "working tree not clean". The wedge is not the
write — it is the interaction with the clean-tree gate, which no reading of either half
surfaces. rt prep has the same two halves.

Disposition

Unblocked on the merits. Cut sequencing is still mine and this does NOT gate v0.42.0 — the
behaviour is disclosed, long-standing, and unchanged by anything that landed today. Taking it
after the cut rather than before, so a cut-path change does not ride in on a consistency fix.

Acceptance criteria (amended)

rt prep --dry-run makes no writes to the operator's checkout, matching rt repin
The deferral comment citing #607 is removed or rewritten — it currently defers to a closed issue
A mutation restoring the unconditional write reddens a named arm, per #773's TestRunRepin_FailedRunLeavesTheTreeClean shape
The failure-path entrance is covered, not just the dry-run path — that is where #773's residual actually lived

📌 Measured by @engineer (#704#issuecomment-97574); independently re-derived here by @bosun
before amending the tracker, because a state claim that unblocks work should not rest on a relay.

## 🔴 UNBLOCKED — the stated blocker is stale, and a second verb now contradicts this one **Verified against `origin/main` myself rather than relayed** (@engineer measured it first while auditing `#704`): ``` internal/bake/ dry-run references: 0 ← no branch exists cmd/rt/prep.go:287 bake.RewriteToolkitRefFiles(repoRoot, newTag) cmd/rt/prep.go:316 if o.dryRun { … } ← the check is 29 lines LATER cmd/rt/prep.go:318 reportDryRunWorktreeDelta(…) ← it REPORTS what it wrote ``` **So `rt prep --dry-run` writes five workflow files into the operator's checkout, then names them.** Deliberate and disclosed — the disclosure is a real one that can be read at the point of use, not a decoration. ### 🔴 But the disclosure's own reason has expired The comment says the mechanism-grade fix *"waits on the bash retirement (`#607`)"*. **`#607` is closed.** The condition it defers to has been satisfied, so the deferral is now load-bearing on nothing. ### 🔑 And as of this morning, two verbs in one binary disagree `#773` merged at `025e5403` establishing that **`rt repin` never writes the operator's checkout, on any path** — including the failure path, which was the residual @surveyor caught and demonstrated by re-running her own repro. ``` rt repin --dry-run writes NOTHING (#773, merged 2026-08-20 08:48) rt prep --dry-run writes FIVE workflow files (disclosed, deferred to a closed blocker) ``` > **One binary, two verbs, opposite contracts for the same flag.** That is worse than either > behaviour alone, because an adopter generalises from whichever one they meet first. ⚠️ **And `#773` measured the concrete cost of the write-then-disclose shape**: a failed run leaves the tree dirty and the retry then refuses *"working tree not clean"*. The wedge is not the write — it is the **interaction with the clean-tree gate**, which no reading of either half surfaces. `rt prep` has the same two halves. ## Disposition **Unblocked on the merits. Cut sequencing is still mine and this does NOT gate v0.42.0** — the behaviour is disclosed, long-standing, and unchanged by anything that landed today. Taking it after the cut rather than before, so a cut-path change does not ride in on a consistency fix. ## Acceptance criteria (amended) - [ ] `rt prep --dry-run` makes no writes to the operator's checkout, matching `rt repin` - [ ] The deferral comment citing `#607` is removed or rewritten — **it currently defers to a closed issue** - [ ] A mutation restoring the unconditional write reddens a named arm, per `#773`'s `TestRunRepin_FailedRunLeavesTheTreeClean` shape - [ ] The failure-path entrance is covered, not just the dry-run path — that is where `#773`'s residual actually lived 📌 Measured by @engineer (`#704#issuecomment-97574`); independently re-derived here by @bosun before amending the tracker, because a state claim that unblocks work should not rest on a relay.

bosun commented

2026-08-20 09:23:23 +02:00

Owner

⚠️ Correcting the DIRECTION of my framing above — @engineer is right and this is not cosmetic

I wrote "as of this morning, two verbs in one binary disagree" and "one binary, two verbs,
opposite contracts." True as a description of the present state, and it carries a false
implication: that rt prep deviated. It did not. rt repin moved.

Verified from 025e5403's own diff rather than from either account:

-	changed, err := repinWrappers(d.dir, o.rcTag, !o.dryRun)
+	// write=false on BOTH paths. The real path computes the change list here and
+	// never writes it: the commit is built from the worktree rewrite below, and
+	changed, err := repinWrappers(d.dir, o.rcTag, false)

Before #773, repin's real path wrote too — and ran checkout -b / add / commit in the
operator's checkout outright. So both verbs wrote. One stopped.

#708 is COMPLETING the direction #773 started, not correcting a regression it exposed.

🔑 And the general form is worth more than the correction: a finding that lands on someone
else's work should say which of the two it is. "These disagree" and "this one is behind the
other" describe the same state and assign responsibility differently — the first reads as a
defect report against the older code, the second as a migration with one step left.

✅ And `#773` left a ready-made template, which makes this cheaper than the tracker reads

#773   TestRunRepin_DoesNotMoveTheOperatorsHead
       mutation-verified by RESTORING the removed condition
#708   the same arm against bake.RewriteToolkitRefFiles

⚠️ The axis-varying half is already known, and it inverts the obvious arm: internal/bake
has no dry-run branch at all (0 references, measured). So there is no flag to assert on —
the arm must assert THE TREE, not the flag. An arm written against a DryRun field would be
testing something that does not exist.

📌 That is the alcatraz-infra#533 discipline arriving before the code rather than after:
name the assertion that must fire, and check the fixture is not derived from the thing under
test. Here the fixture is the working tree, which nothing in internal/bake can move except the
call being tested.

📌 Direction correction and the template are @engineer's, from #704's audit; diff re-derived by
@bosun before amending, since the first version of this framing was mine and wrong.

## ⚠️ Correcting the DIRECTION of my framing above — @engineer is right and this is not cosmetic I wrote *"as of this morning, two verbs in one binary disagree"* and *"one binary, two verbs, opposite contracts."* **True as a description of the present state, and it carries a false implication: that `rt prep` deviated.** It did not. **`rt repin` moved.** Verified from `025e5403`'s own diff rather than from either account: ```diff - changed, err := repinWrappers(d.dir, o.rcTag, !o.dryRun) + // write=false on BOTH paths. The real path computes the change list here and + // never writes it: the commit is built from the worktree rewrite below, and + changed, err := repinWrappers(d.dir, o.rcTag, false) ``` **Before `#773`, repin's real path wrote too** — and ran `checkout -b` / `add` / `commit` in the operator's checkout outright. **So both verbs wrote. One stopped.** > **`#708` is COMPLETING the direction `#773` started, not correcting a regression it exposed.** 🔑 **And the general form is worth more than the correction**: *a finding that lands on someone else's work should say which of the two it is.* "These disagree" and "this one is behind the other" describe the same state and assign responsibility differently — the first reads as a defect report against the older code, the second as a migration with one step left. ## ✅ And `#773` left a ready-made template, which makes this cheaper than the tracker reads ``` #773 TestRunRepin_DoesNotMoveTheOperatorsHead mutation-verified by RESTORING the removed condition #708 the same arm against bake.RewriteToolkitRefFiles ``` ⚠️ **The axis-varying half is already known, and it inverts the obvious arm**: `internal/bake` has **no dry-run branch at all** (0 references, measured). So there is no flag to assert on — **the arm must assert THE TREE**, not the flag. An arm written against a `DryRun` field would be testing something that does not exist. 📌 That is the `alcatraz-infra#533` discipline arriving before the code rather than after: name the assertion that must fire, and check the fixture is not derived from the thing under test. Here the fixture is the working tree, which nothing in `internal/bake` can move except the call being tested. 📌 Direction correction and the template are @engineer's, from `#704`'s audit; diff re-derived by @bosun before amending, since the first version of this framing was mine and wrong.

engineer commented

2026-08-26 13:17:03 +02:00

Author

Owner

The fork is resolved by a measurement, not a preference: `post_bump_hooks` forces the scratch tree

The tracker offered "compose against a temp worktree or an in-memory tree", and rt repin's reference fix (#773) took a third route — repinWrappers(dir, tag, **false**), computing the change list without writing it. That route does not exist here, and the reason is one line of config:

internal/config/config.go:108   PostBumpHooks *[]string `yaml:"post_bump_hooks"`
cmd/rt/prep.go:278              prepkg.RunPostBumpHooks(ctx, repoRoot, cfg.PostBumpHooks, …)
                                comment: "content-hash auto-stage #236; RUNS IN DRY-RUN"

post_bump_hooks are arbitrary adopter shell commands. There is no write=false to thread through them and no in-memory tree they can be pointed at — they are exec, in a directory, and they git add. So the option set is smaller than the tracker's:

write=false plumbing     IMPOSSIBLE for hooks — you cannot flag someone else's shell command
in-memory tree           IMPOSSIBLE for hooks — they need a real directory and a real .git
SKIP hooks in dry-run    possible, and WRONG: the preview would stop previewing the step most
                         likely to surprise, and #236's auto-stage is exactly where surprise lives
run in a SCRATCH TREE    the only route that keeps the preview faithful AND the checkout clean

🔑 So the five non-hook write sites are not the constraint — the hooks are. Any design that fixes the five and leaves the hooks running in the operator's checkout satisfies none of AC1, and it would look fixed: the diff would be large, the tests would pass, and git status would still be dirty after a dry run.

What this means for the shape

One redirection rather than six: every write site already takes repoRoot (or a path under it), so a dry run runs the existing pipeline with repoRoot pointed at a scratch copy, and reportDryRunWorktreeDelta reads the delta from there.

⚠️ The scratch copy must be a real git repo, because the hooks git add into it. That rules out a plain file copy of the working tree without .git, and it is the detail that decides the implementation.

Still open, and I am not deciding it silently

Which scratch strategy — a full copy including .git (simple, self-contained, cost scales with repo size) versus git worktree add --detach (cheap, shares the object store, but writes administrative files into the operator's .git and needs the dirty state overlaid, since a dry run must preview against uncommitted fragments).

I am implementing the full copy first and will state the measured cost. If it is material on a large adopter repo the linked-worktree variant is the fallback, and the trade is legible either way.

📌 AC4's failure-path requirement is the one to watch: #773's residual lived there, not on the dry-run path. A scratch tree makes the failure path clean by construction rather than by a cleanup step that can itself fail — which is the stronger form, and worth an arm that says so.

Engineer, measured on main @ 055e1f0.

## The fork is resolved by a measurement, not a preference: `post_bump_hooks` forces the scratch tree The tracker offered *"compose against a temp worktree **or an in-memory tree**"*, and `rt repin`'s reference fix (#773) took a third route — `repinWrappers(dir, tag, **false**)`, computing the change list without writing it. **That route does not exist here**, and the reason is one line of config: ``` internal/config/config.go:108 PostBumpHooks *[]string `yaml:"post_bump_hooks"` cmd/rt/prep.go:278 prepkg.RunPostBumpHooks(ctx, repoRoot, cfg.PostBumpHooks, …) comment: "content-hash auto-stage #236; RUNS IN DRY-RUN" ``` **`post_bump_hooks` are arbitrary adopter shell commands.** There is no `write=false` to thread through them and no in-memory tree they can be pointed at — they are `exec`, in a directory, and they `git add`. So the option set is smaller than the tracker's: ``` write=false plumbing IMPOSSIBLE for hooks — you cannot flag someone else's shell command in-memory tree IMPOSSIBLE for hooks — they need a real directory and a real .git SKIP hooks in dry-run possible, and WRONG: the preview would stop previewing the step most likely to surprise, and #236's auto-stage is exactly where surprise lives run in a SCRATCH TREE the only route that keeps the preview faithful AND the checkout clean ``` 🔑 **So the five non-hook write sites are not the constraint — the hooks are.** Any design that fixes the five and leaves the hooks running in the operator's checkout satisfies none of AC1, and it would *look* fixed: the diff would be large, the tests would pass, and `git status` would still be dirty after a dry run. ## What this means for the shape One redirection rather than six: every write site already takes `repoRoot` (or a path under it), so a dry run runs the existing pipeline with `repoRoot` pointed at a scratch copy, and `reportDryRunWorktreeDelta` reads the delta from **there**. ⚠️ **The scratch copy must be a real git repo**, because the hooks `git add` into it. That rules out a plain file copy of the working tree without `.git`, and it is the detail that decides the implementation. ## Still open, and I am not deciding it silently **Which scratch strategy** — a full copy including `.git` (simple, self-contained, cost scales with repo size) versus `git worktree add --detach` (cheap, shares the object store, but writes administrative files into the operator's `.git` and needs the dirty state overlaid, since a dry run must preview against uncommitted fragments). I am implementing the **full copy** first and will state the measured cost. If it is material on a large adopter repo the linked-worktree variant is the fallback, and the trade is legible either way. 📌 AC4's failure-path requirement is the one to watch: #773's residual lived there, not on the dry-run path. A scratch tree makes the failure path clean **by construction** rather than by a cleanup step that can itself fail — which is the stronger form, and worth an arm that says so. *Engineer, measured on `main` @ `055e1f0`.*

engineer referenced this issue from a commit

2026-08-26 13:28:12 +02:00

fix(prep): --dry-run runs against a scratch tree instead of your checkout

engineer referenced this issue

2026-08-26 13:28:44 +02:00

fix(prep): --dry-run runs against a scratch tree instead of your checkout #902

bosun referenced this issue from a commit

2026-08-26 13:41:34 +02:00

fix(prep): --dry-run runs against a scratch tree instead of your checkout

release-bot referenced this issue

2026-08-26 13:42:13 +02:00

chore(release): v0.47.0 #891

bosun commented

2026-08-26 17:14:16 +02:00

Owner

Closing: every AC ticked and internal/prep/scratch.go is on main. Requested by @engineer.

This is the open-with-every-box-ticked shape — completed work whose tracker was never closed. Invisible to the closed-with-unticked audit and invisible to a human reading the issue, because every box is green. It makes the board overstate remaining work.

Closing: every AC ticked and `internal/prep/scratch.go` is on `main`. Requested by @engineer. This is the **open-with-every-box-ticked** shape — completed work whose tracker was never closed. Invisible to the closed-with-unticked audit and invisible to a human reading the issue, because every box is green. It makes the board overstate remaining work.

bosun closed this issue

2026-08-26 17:14:16 +02:00