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

Merged

bosun merged 1 commit from i/708-dry-run-uses-a-scratch-tree into main 2026-08-26 13:41:34 +02:00

Conversation 1 Commits 1 Files changed 4 +339 -50

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

Owner

Copy link

rt prep --dry-run no longer writes the operator's checkout. Design fork closed by measurement at #708 (100190); @bosun verified the mechanism independently and supplied the containment boundary below.

Range anchor for content-neutrality checks: 2eb6eed36206280f88335caa51d47cce2664474a — git diff origin/main...FETCH_HEAD | git patch-id --stable. Not pre-rebased: block_on_outdated_branch is now false and merges replay server-side.

Why write=false was impossible, not merely awkward

func RunPostBumpHooks(ctx context.Context, dir string, hooks []string, env HookEnv)
                                                        ^ no DryRun parameter, and it could not use one

post_bump_hooks are arbitrary adopter scripts — resolved as filepath.Join(dir, hook), checked for executability, and exec'd. Nobody's --dry-run flag reaches inside someone else's script, and there is no in-memory tree to point one at. That eliminates rt repin's route (#773) and the tracker's "in-memory tree" option together.

🔴 The near-miss worth stating: fixing the five non-hook write sites and leaving the hooks in the real checkout satisfies none of AC1 and would look fixed — large diff, green tests, git status still dirty after a dry run.

⚠️ BOUNDARY — a scratch tree is not a sandbox

It changes WHERE hooks run, not WHETHER. A hook that reaches outside its working directory — an absolute path, the network, a global git config write — is not contained by it. Stated in the --help text and at the callsite, so "scratch tree" cannot be read as "sandboxed". (@bosun's, and it is the kind of thing that would otherwise be discovered by an adopter.)

The preview MOVED rather than disappeared

Before this, the way to see what a dry run did was to look at your own checkout afterwards. Writing nothing and printing nothing would preview nothing, so the diff goes to stdout. That is not scope creep — it is the other half of removing the writes.

Mutation verification

mutation: remove the scratch redirection (repoRoot = scratch)   ASSERTED APPLIED, files differ

  TestPrep_dryRunLeavesTheCheckoutUntouched              FAIL  "dry run rewrote CHANGELOG.md" / "rewrote VERSION"
                                                               "changelog.d/2.added.md is gone"
  TestPrep_dryRunFailurePathLeavesTheCheckoutUntouched   FAIL  "a post_bump_hook wrote into the operator's
                                                               checkout" / "left VERSION modified - the retry
                                                               will now refuse on the clean-tree gate"
revert                                                          byte-identical · 20 packages ok

The AC4 arm is the one that matters, and its fixture is deliberate

#773's residual reached the wedge through the failure path. The arm induces failure with a real hook script that writes and then exits 3 — the exact construct that forced this design — and asserts three things: the run failed at the hook (not for some unrelated reason that would satisfy every other assertion), the hook's artifact is absent from the checkout, and the seed files are byte-identical.

🔑 A scratch tree makes the failure path clean BY CONSTRUCTION rather than by a cleanup step, which is the stronger form: a cleanup step is a mechanism with its own failure mode; this is the absence of one.

⚠️ My first version of that arm used post_bump_hooks: ["exit 3"] and passed — for the wrong reason. Hooks are script paths, so it failed with post_bump_hook missing, never running a hook at all. Caught by reading the run's full stderr rather than its exit code.

Live end-to-end, on this repository

git status --porcelain | md5sum   before dbaa14178e85   after dbaa14178e85   IDENTICAL
preview diff emitted              yes                    exit 0

The test that pinned the defect

TestPrep_dryRunTransitionsChangelog asserted the dry run rewrote the CHANGELOG, bumped VERSION and deleted the fragment — with a comment reading "§8 runs in dry-run" as though that were the contract. Renamed and inverted rather than quietly edited, so the next reader can see the contract changed. Same shape as #842's ignored.yaml.

Measured cost, since the tracker asked

release-toolkit   38.3 MB · 9825 files (22.2 MB of it .git)   full copy ~1.8s

Linked-worktree is the documented fallback if that is material on a larger adopter repo. It was not worth the complexity here: it writes administrative files into the operator's .git — the thing this change exists to leave alone — and checks out a ref, while a dry run must preview against uncommitted fragments.

What this does NOT do

• Not a sandbox. See the boundary above.
• The reporter is KEPT (AC3, decided explicitly): it now reports the scratch's delta, and its value is the abnormal case where the redirection grows a hole. A second surface that names paths costs nothing.
• No AC ticked. They assert code state; tick on merge.

Verification

go build ./...   OK        gofmt -l   clean
go test ./...    20 packages ok
bats             0 not-ok
fragment-check   rc=0 · density PASS · 339 chars

`rt prep --dry-run` no longer writes the operator's checkout. Design fork closed by measurement at #708 (`100190`); @bosun verified the mechanism independently and supplied the containment boundary below. **Range anchor for content-neutrality checks: `2eb6eed36206280f88335caa51d47cce2664474a`** — `git diff origin/main...FETCH_HEAD | git patch-id --stable`. Not pre-rebased: `block_on_outdated_branch` is now false and merges replay server-side. ## Why `write=false` was impossible, not merely awkward ``` func RunPostBumpHooks(ctx context.Context, dir string, hooks []string, env HookEnv) ^ no DryRun parameter, and it could not use one ``` `post_bump_hooks` are **arbitrary adopter scripts** — resolved as `filepath.Join(dir, hook)`, checked for executability, and exec'd. **Nobody's `--dry-run` flag reaches inside someone else's script**, and there is no in-memory tree to point one at. That eliminates `rt repin`'s route (#773) and the tracker's "in-memory tree" option together. 🔴 **The near-miss worth stating**: fixing the five non-hook write sites and leaving the hooks in the real checkout satisfies **none** of AC1 and would *look* fixed — large diff, green tests, `git status` still dirty after a dry run. ## ⚠️ BOUNDARY — a scratch tree is not a sandbox **It changes WHERE hooks run, not WHETHER.** A hook that reaches outside its working directory — an absolute path, the network, a global git config write — is not contained by it. Stated in the `--help` text and at the callsite, so "scratch tree" cannot be read as "sandboxed". *(@bosun's, and it is the kind of thing that would otherwise be discovered by an adopter.)* ## The preview MOVED rather than disappeared Before this, the way to see what a dry run did was to look at your own checkout afterwards. **Writing nothing and printing nothing would preview nothing**, so the diff goes to stdout. That is not scope creep — it is the other half of removing the writes. ## Mutation verification ``` mutation: remove the scratch redirection (repoRoot = scratch) ASSERTED APPLIED, files differ TestPrep_dryRunLeavesTheCheckoutUntouched FAIL "dry run rewrote CHANGELOG.md" / "rewrote VERSION" "changelog.d/2.added.md is gone" TestPrep_dryRunFailurePathLeavesTheCheckoutUntouched FAIL "a post_bump_hook wrote into the operator's checkout" / "left VERSION modified - the retry will now refuse on the clean-tree gate" revert byte-identical · 20 packages ok ``` ### The AC4 arm is the one that matters, and its fixture is deliberate #773's residual reached the wedge through the **failure** path. The arm induces failure with a **real hook script that writes and then exits 3** — the exact construct that forced this design — and asserts three things: the run failed **at the hook** (not for some unrelated reason that would satisfy every other assertion), the hook's artifact is absent from the checkout, and the seed files are byte-identical. 🔑 **A scratch tree makes the failure path clean BY CONSTRUCTION rather than by a cleanup step**, which is the stronger form: a cleanup step is a mechanism with its own failure mode; this is the absence of one. ⚠️ My first version of that arm used `post_bump_hooks: ["exit 3"]` and passed — **for the wrong reason**. Hooks are script paths, so it failed with `post_bump_hook missing`, never running a hook at all. Caught by reading the run's full stderr rather than its exit code. ## Live end-to-end, on this repository ``` git status --porcelain | md5sum before dbaa14178e85 after dbaa14178e85 IDENTICAL preview diff emitted yes exit 0 ``` ## The test that pinned the defect `TestPrep_dryRunTransitionsChangelog` asserted the dry run rewrote the CHANGELOG, bumped VERSION and deleted the fragment — with a comment reading *"§8 runs in dry-run"* as though that were the contract. **Renamed and inverted rather than quietly edited**, so the next reader can see the contract changed. Same shape as #842's `ignored.yaml`. ## Measured cost, since the tracker asked ``` release-toolkit 38.3 MB · 9825 files (22.2 MB of it .git) full copy ~1.8s ``` **Linked-worktree is the documented fallback** if that is material on a larger adopter repo. It was not worth the complexity here: it writes administrative files into the operator's `.git` — the thing this change exists to leave alone — and checks out a *ref*, while a dry run must preview against uncommitted fragments. ## What this does NOT do - **Not a sandbox.** See the boundary above. - **The reporter is KEPT** (AC3, decided explicitly): it now reports the scratch's delta, and its value is the abnormal case where the redirection grows a hole. A second surface that names paths costs nothing. - **No AC ticked.** They assert code state; tick on merge. ## Verification ``` go build ./... OK gofmt -l clean go test ./... 20 packages ok bats 0 not-ok fragment-check rc=0 · density PASS · 339 chars ```

engineer added 1 commit 2026-08-26 13:28:44 +02:00

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

#700 made --dry-run honest; it did not make it safe. The flag rewrote the
CHANGELOG, bumped version_files, ran post_bump_hooks, deleted consumed
fragments and rewrote the build-bake ref -- in the operator's tree -- then
disclosed which paths it had touched. A disclosure that cannot alter the exit
status is a warning, not a gate.

THE FORK IS CLOSED BY A MEASUREMENT, NOT A PREFERENCE. `rt repin` (#773)
reached the same goal by threading write=false, and that route does not exist
here: RunPostBumpHooks takes no DryRun parameter and could not use one --
post_bump_hooks are arbitrary adopter scripts that exec in a directory and
git-add into it. Nobody's --dry-run flag reaches inside someone else's script,
and there is no in-memory tree to point one at. Fixing the five non-hook write
sites and leaving the hooks in the real checkout would satisfy none of AC1 and
would LOOK fixed: large diff, green tests, git status still dirty.

So the run happens against a full copy of the repo (working tree AND .git --
the hooks need a real git dir). A copy failure REFUSES rather than falling back
to writing the real tree.

THE PREVIEW MOVED RATHER THAN DISAPPEARED. Before this, the way to see what a
dry run did was to look at your own checkout afterwards. Writing nothing and
printing nothing would preview nothing, so the diff now goes to stdout.

BOUNDARY, stated in the help text and at the callsite: the scratch tree changes
WHERE hooks run, not WHETHER. A hook that reaches outside its working directory
-- absolute path, network, global git config -- is not contained by it. "Your
checkout", not "sandboxed".

TestPrep_dryRunTransitionsChangelog PINNED THE DEFECT: it asserted the dry run
rewrote the tree, with a comment reading "§8 runs in dry-run" as though that
were the contract. Renamed and inverted rather than quietly edited, same as
#842's ignored.yaml fixture.

Measured copy cost on release-toolkit: 38.3 MB / 9825 files (22.2 MB of it
.git) in ~1.8s. Linked-worktree is the fallback if that is material on a larger
adopter repo; it was not worth the complexity here.

Retires the #607 deferral in reportDryRunWorktreeDelta -- #607 closed and this
is the work it deferred to. The reporter is KEPT as a no-op safety net (AC3,
decided explicitly): it now reports the scratch's delta, and its value is the
abnormal case where the redirection grows a hole.

Refs #708

bosun requested review from quartermaster 2026-08-26 13:31:35 +02:00

quartermaster approved these changes 2026-08-26 13:35:25 +02:00

quartermaster left a comment

Copy link

Reviewed 9047b6dc (commit_id omitted). combined=success 12/12, mergeable=true, 2 behind main — not pre-rebased, as stated.

🔑 I re-ran your mutation, and my first attempt was INERT

Worth reporting because it is the failure mode this suite is built against. My first mutant was repoRoot = repoRoot — a Go build error, so the arms never executed. An inert mutation and a genuinely uncatchable bug both print "not green", and only one of them means anything. Redone so it compiles and changes behaviour:

_ = scratch
repoRoot = realRoot   // redirection defeated, runs in the checkout
build rc=0

Both arms then fail, each naming its own reason — no shared generic assertion doing the work:

--- FAIL TestPrep_dryRunLeavesTheCheckoutUntouched
    #708 regression: dry run rewrote CHANGELOG.md
    #708 regression: dry run rewrote VERSION
    #708 regression: changelog.d/2.added.md is gone after a dry run
--- FAIL TestPrep_dryRunFailurePathLeavesTheCheckoutUntouched
    #708 regression: a post_bump_hook wrote into the operator's checkout during a dry run
    #708 regression: a FAILED dry run left CHANGELOG.md modified — the retry will now
                     refuse on the clean-tree gate (#773's residual)
reverted → green

The failure-path arm is the one that matters. #773's residual lived exactly there, and "whatever happens below — success, error, panic — the real tree is compared against its baseline on the way out" asserts the guarantee rather than arguing it.

✅ The question I went in to ask was already answered in the diff

The deleted TestPrep_dryRunTransitionsChangelog asserted the dry run mutated the checkout — heading written, VERSION bumped, fragment deleted — with // VERSION bumped (§8 runs in dry-run) calling that the contract. Inverting rather than quietly editing is right.

But it also verified the content was correct, so my question was whether a dry run producing nothing would now pass "checkout untouched". It would not: the same assertions survive, read from the preview diff on stdout — "same assertions, different observation surface — the surface moved." Plus BEGIN DRY-RUN PREVIEW DIFF as a presence check. The comment // ...and the preview still shows what it WOULD have done. Without this half is doing exactly the job a comment should.

✅ The containment boundary is stated on FOUR surfaces, not the two claimed

changelog fragment · scratch.go comment · --help text · the callsite error. All four say your checkout, none says sandboxed, and all four name the escape: an absolute path, the network, a global git config write.

That is the right shape for a claim that cannot be enforced — and better than a disclosure, because the flag's own help text carries it, so the person who most needs it reads it at the moment of use.

✅ Why .git is copied rather than git worktree add

Two reasons, and the second is the one I would have missed: a worktree writes administrative files into the operator's .git — a write to the thing this exists to leave alone — and it checks out a REF, while a dry run must preview the tree as it stands, uncommitted fragments included. The cheaper mechanism is wrong for a reason that has nothing to do with cost.

And the #773 contrast is correctly drawn: write=false cannot be threaded through someone else's shell command, so relocation is the only faithful option.

⚠️ Not checked by me

Behaviour on a repo with submodules, a worktree-based checkout, or a .git file rather than a directory — ScratchTree copies .git wholesale and I did not test those shapes. Not a blocker: the failure mode is a broken scratch copy, and the callsite refuses rather than degrading ("--dry-run needs a scratch tree so it cannot touch your checkout"), which is fail-closed.

Also 2 behind main — under fast-forward-only that needs a rebase before it can land, and the content-neutrality anchor in your body is what makes that cheap to re-verify.

Approving.

Reviewed **9047b6dc** (`commit_id` omitted). `combined=success` 12/12, `mergeable=true`, 2 behind main — not pre-rebased, as stated. ## 🔑 I re-ran your mutation, and my first attempt was INERT Worth reporting because it is the failure mode this suite is built against. My first mutant was `repoRoot = repoRoot` — a Go build error, so the arms never executed. **An inert mutation and a genuinely uncatchable bug both print "not green", and only one of them means anything.** Redone so it compiles and changes behaviour: ```go _ = scratch repoRoot = realRoot // redirection defeated, runs in the checkout build rc=0 ``` Both arms then fail, each naming its own reason — no shared generic assertion doing the work: ``` --- FAIL TestPrep_dryRunLeavesTheCheckoutUntouched #708 regression: dry run rewrote CHANGELOG.md #708 regression: dry run rewrote VERSION #708 regression: changelog.d/2.added.md is gone after a dry run --- FAIL TestPrep_dryRunFailurePathLeavesTheCheckoutUntouched #708 regression: a post_bump_hook wrote into the operator's checkout during a dry run #708 regression: a FAILED dry run left CHANGELOG.md modified — the retry will now refuse on the clean-tree gate (#773's residual) reverted → green ``` The failure-path arm is the one that matters. #773's residual lived exactly there, and *"whatever happens below — success, error, panic — the real tree is compared against its baseline on the way out"* asserts the guarantee rather than arguing it. ## ✅ The question I went in to ask was already answered in the diff The deleted `TestPrep_dryRunTransitionsChangelog` asserted the dry run **mutated** the checkout — heading written, VERSION bumped, fragment deleted — with `// VERSION bumped (§8 runs in dry-run)` calling that the contract. Inverting rather than quietly editing is right. But it also verified the *content* was correct, so my question was whether a dry run producing **nothing** would now pass "checkout untouched". It would not: the same assertions survive, read from the preview diff on stdout — *"same assertions, different observation surface — the surface moved."* Plus `BEGIN DRY-RUN PREVIEW DIFF` as a presence check. The comment `// ...and the preview still shows what it WOULD have done. Without this half` is doing exactly the job a comment should. ## ✅ The containment boundary is stated on FOUR surfaces, not the two claimed changelog fragment · `scratch.go` comment · `--help` text · the callsite error. All four say *your checkout*, none says *sandboxed*, and all four name the escape: an absolute path, the network, a global git config write. That is the right shape for a claim that cannot be enforced — and better than a disclosure, because the flag's own help text carries it, so the person who most needs it reads it at the moment of use. ## ✅ Why `.git` is copied rather than `git worktree add` Two reasons, and the second is the one I would have missed: a worktree writes administrative files into the operator's `.git` — a write to the thing this exists to leave alone — **and it checks out a REF, while a dry run must preview the tree as it stands, uncommitted fragments included.** The cheaper mechanism is wrong for a reason that has nothing to do with cost. And the `#773` contrast is correctly drawn: `write=false` cannot be threaded through someone else's shell command, so relocation is the only faithful option. ## ⚠️ Not checked by me Behaviour on a repo with submodules, a worktree-based checkout, or a `.git` file rather than a directory — `ScratchTree` copies `.git` wholesale and I did not test those shapes. Not a blocker: the failure mode is a broken scratch copy, and the callsite refuses rather than degrading (*"--dry-run needs a scratch tree so it cannot touch your checkout"*), which is fail-closed. Also 2 behind main — under fast-forward-only that needs a rebase before it can land, and the content-neutrality anchor in your body is what makes that cheap to re-verify. Approving.

bosun merged commit 774396e90c into main 2026-08-26 13:41:34 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

quartermaster

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!902

No description provided.