fix(prep): the dry-run scratch copy skips ignored paths #1590

Merged

quartermaster merged 3 commits from quartermaster/1543-scratch-skips-ignored into main 2026-10-22 02:17:20 +02:00

Conversation 4 Commits 3 Files changed 5 +410 -11

quartermaster commented 2026-10-22 01:53:26 +02:00

Owner

• Copy link
• Copy Markdown

Refs #1543.

On Windows rt prep --dry-run refused whenever another program held an ignored file open without read sharing — a Unity editor with the project open does exactly that — because copyTree opened every file it walked. The preview needs none of those files.

The measurement is @tallyman's, and the values sit beside the name that produced them

Verified on Windows at e08e68e4 (@tallyman), with the end-to-end preview compared byte-for-byte against main's unheld run:

arm	result
ignored file held, share None (in dist/ and root rt)	exit 0, stdout+stderr byte-identical to main's unheld preview
the same file, share Read	exit 0, byte-identical
tracked VERSION held	exit 1, path named once (AC5)
untracked non-ignored held	exit 1, path named once

🔑 The share-Read row is the discriminator: this is a sharing-mode defect, not a locking one, so a holder that permits reads reproduces nothing. The last two rows bound the fix — those refusals are legitimate and must survive.

scratch_windows_test.go ran and passed on Windows (@tallyman) — its first execution anywhere.

The decision, from #1543's own tree

AC1 offers preview (a, b) or refuse-before-copying (c). Taken: skip ignored paths, so the preview runs. A refusal naming the file would be honest but leaves the operator unable to preview a cut from their own checkout, which is what the flag is for.

Skip ignored, not whitelist listed. A git ls-files -co --exclude-standard whitelist works and drops submodule contents, because submodule files are not listed — @tallyman flagged that against his own throwaway. Skipping what git calls ignored reaches the same files with no side effect.

And the listings this depends on survive a held file (@tallyman): git ls-files -oi --exclude-standard --directory exits 0 naming dist/ and rt; git check-ignore -v exits 0 naming only the two ignored paths. The --directory form names an ignored directory as a unit, so the walk SkipDirs it and never opens what is inside.

🔴 The gap his mutation run found, and its Linux substitute

Against the whole package on Windows (@tallyman), a mutation widening copyFile to return nil on any open error except not-exist was killed only by the Windows arm. CI runs no Windows job, so the fix's narrowness was guarded by nothing that executes here. His note said no Linux substitute was known, and the reason is sound: root ignores chmod 000, and CI runs as root.

A NUL in the path is the substitute. Go returns EINVAL from os.Open before the syscall — not not-exist, and uid-independent:

os.Open("some\x00path")          -> "invalid argument"
errors.Is(err, os.ErrNotExist)   -> false
euid                             -> 1000, identical at 0

So that mutation now dies on Linux too. 🔴 But it is not the widening someone would actually write, and an earlier version of this section said otherwise. Measured on Windows at 0f8ff78 (@tallyman):

M2  nil on ANY error but not-exist   dies to the NUL arm AND the Windows arm
M3  nil on errno 32 only             dies ONLY to the Windows arm's tracked control

M3 is if errors.Is(err, syscall.Errno(32)) { return nil } — ERROR_SHARING_VIOLATION, which is #1543's option (b) without its "ignored" condition: what someone reaches for the first time Unity holds a tracked file. A NUL path yields EINVAL and never errno 32, and Linux has no sharing violations, so no Linux arm can kill it.

🔑 So scratch_windows_test.go is unenforced AND load-bearing at once. Deleting it loses the only check on this fix's narrowness; keeping it buys nothing automatic. Whoever touches copyFile's error handling has to run it by hand on Windows, and the file's own header now says so.

Arms

Seven GOOS-independent arms, mutation-verified — these are what gate the skip:

mutation	killed by
remove the skip	SkipsIgnoredPaths, MixedDirKeepsTheFragment
skip an ignored dir as a file (descend)	SkipsIgnoredPaths
restore the doubled path	ErrorNamesThePathOnce
-o instead of -oi	those two + IgnoredDirIsSkippedWhole
widen to nil on non-not-exist (M2)	PropagatesANonNotExistOpenError
-c added to the listing (M7)	ForceAddedTrackedFileSurvives
widen to nil on errno 32 (M3)	nothing on Linux — Windows arm only

MixedDirKeepsTheFragment comes from the listing shapes @tallyman enumerated: changelog.d/ holding a fragment beside an ignored .log keeps the fragment. If it did not, the preview would compose the wrong notes while reporting success.

⚠️ scratch_windows_test.go is a recorded procedure, not a gate, and says so in its own comment — while also being the only thing that kills M3. GOOS=windows go vet ./... is the only check this host can make of it.

No Windows runner exists, enumerated from each runner's own config rather than inferred from workflow labels (an earlier version of this line named an image as a label):

forgejo-runner           dotnet godot python go antora playwright squadron-test
                         (all docker:// Linux images), docker-build:host,
                         forgejo-deploy:host
gitea-runner-rt-probe    ubuntu-latest, codeberg-small-lazy
gitea-runner-tictactoe   ubuntu-latest, codeberg-small-lazy

13 labels, 3 runners, every one a Linux container image or :host — and the host is Debian. The org and repo runner APIs return empty arrays and the admin endpoint 404s on this version, so the configs are the authoritative surface.

🔑 Reviewer's qualifier, kept rather than smoothed away (@tallyman): "the only check on this fix's narrowness" is true for an errno-32 widening; the NUL arm still checks the broad one on Linux. The paragraph above scopes it, so the code was left as it stands.

Not verified

• Copy time on a Unity-sized checkout (@tallyman): not measured, on Windows or Linux.
• post_bump_hooks reading an ignored file now sees it absent. @tallyman's survey: 7 of the 30 visible frankenbit repos carry release-toolkit.yml, and only tmux-tell sets hooks — update-readme-version.sh, which reads and rewrites the tracked README.md and git adds it, touching no ignored file. Repos he cannot see and adopters off this forge are not covered, so the gap is real in principle and empty today. It is in copyTree's doc comment (AC4) rather than only here. His second opinion was non-blocking and I am taking it.
• A submodule's own ignored file, held, still refuses (@tallyman, at 0f8ff78): the superproject's listing does not name it, so the skip never reaches it. The submodule claim this body credits to him holds — -co names sub only — but that is the remaining shape where the original symptom survives.
• A stderr notice when ignored paths are skipped was offered and deliberately not taken: it would change the stderr the byte-identical arms compare, and it would diagnose a failure mode that is empty today. Worth revisiting the first time an adopter hook needs an ignored file.

Gates

go build, go vet, GOOS=windows go vet, go test ./... (30 packages), golangci-lint (0 issues), bats 257 arms, ascii-only-check, register-check, fragment-check, gitea-twin --check, shellcheck in CI's exact form, gofmt. All rc=0.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

Refs #1543. On Windows `rt prep --dry-run` refused whenever another program held an **ignored** file open without read sharing — a Unity editor with the project open does exactly that — because `copyTree` opened every file it walked. The preview needs none of those files. ## The measurement is @tallyman's, and the values sit beside the name that produced them Verified on Windows at `e08e68e4` (@tallyman), with the end-to-end preview compared byte-for-byte against main's unheld run: | arm | result | |---|---| | ignored file held, share **None** (in `dist/` and root `rt`) | exit 0, stdout+stderr **byte-identical** to main's unheld preview | | the same file, share **Read** | exit 0, byte-identical | | tracked `VERSION` held | exit 1, path named once (AC5) | | untracked non-ignored held | exit 1, path named once | 🔑 **The share-Read row is the discriminator**: this is a sharing-mode defect, not a locking one, so a holder that permits reads reproduces nothing. The last two rows **bound** the fix — those refusals are legitimate and must survive. `scratch_windows_test.go` **ran and passed on Windows (@tallyman)** — its first execution anywhere. ## The decision, from #1543's own tree AC1 offers preview (a, b) or refuse-before-copying (c). **Taken: skip ignored paths**, so the preview runs. A refusal naming the file would be honest but leaves the operator unable to preview a cut from their own checkout, which is what the flag is for. **Skip ignored, not whitelist listed.** A `git ls-files -co --exclude-standard` whitelist works and drops **submodule** contents, because submodule files are not listed — @tallyman flagged that against his own throwaway. Skipping what git calls ignored reaches the same files with no side effect. And the listings this depends on survive a held file (@tallyman): `git ls-files -oi --exclude-standard --directory` exits 0 naming `dist/` and `rt`; `git check-ignore -v` exits 0 naming only the two ignored paths. The `--directory` form names an ignored directory as a **unit**, so the walk `SkipDir`s it and never opens what is inside. ## 🔴 The gap his mutation run found, and its Linux substitute Against the whole package on Windows (@tallyman), a mutation widening `copyFile` to return `nil` on any open error **except** not-exist was killed **only** by the Windows arm. CI runs no Windows job, so the fix's *narrowness* was guarded by nothing that executes here. His note said no Linux substitute was known, and the reason is sound: root ignores `chmod 000`, and CI runs as root. **A NUL in the path is the substitute.** Go returns `EINVAL` from `os.Open` before the syscall — not not-exist, and uid-independent: ``` os.Open("some\x00path") -> "invalid argument" errors.Is(err, os.ErrNotExist) -> false euid -> 1000, identical at 0 ``` So **that** mutation now dies on Linux too. 🔴 **But it is not the widening someone would actually write, and an earlier version of this section said otherwise.** Measured on Windows at `0f8ff78` (@tallyman): ``` M2 nil on ANY error but not-exist dies to the NUL arm AND the Windows arm M3 nil on errno 32 only dies ONLY to the Windows arm's tracked control ``` M3 is `if errors.Is(err, syscall.Errno(32)) { return nil }` — ERROR_SHARING_VIOLATION, which is #1543's option (b) **without** its "ignored" condition: what someone reaches for the first time Unity holds a *tracked* file. A NUL path yields EINVAL and never errno 32, and Linux has no sharing violations, so **no Linux arm can kill it**. 🔑 **So `scratch_windows_test.go` is unenforced AND load-bearing at once.** Deleting it loses the only check on this fix's narrowness; keeping it buys nothing automatic. Whoever touches `copyFile`'s error handling has to run it by hand on Windows, and the file's own header now says so. ## Arms Seven GOOS-independent arms, mutation-verified — these are what gate the skip: | mutation | killed by | |---|---| | remove the skip | `SkipsIgnoredPaths`, `MixedDirKeepsTheFragment` | | skip an ignored dir as a file (descend) | `SkipsIgnoredPaths` | | restore the doubled path | `ErrorNamesThePathOnce` | | `-o` instead of `-oi` | those two + `IgnoredDirIsSkippedWhole` | | widen to nil on non-not-exist (M2) | `PropagatesANonNotExistOpenError` | | `-c` added to the listing (M7) | `ForceAddedTrackedFileSurvives` | | widen to nil on errno 32 (M3) | **nothing on Linux** — Windows arm only | `MixedDirKeepsTheFragment` comes from the listing shapes @tallyman enumerated: `changelog.d/` holding a fragment beside an ignored `.log` keeps the fragment. If it did not, the preview would compose the **wrong notes while reporting success**. ⚠️ **`scratch_windows_test.go` is a recorded procedure, not a gate, and says so in its own comment** — while also being the only thing that kills M3. `GOOS=windows go vet ./...` is the only check this host can make of it. **No Windows runner exists**, enumerated from each runner's own config rather than inferred from workflow labels (an earlier version of this line named an image as a label): ``` forgejo-runner dotnet godot python go antora playwright squadron-test (all docker:// Linux images), docker-build:host, forgejo-deploy:host gitea-runner-rt-probe ubuntu-latest, codeberg-small-lazy gitea-runner-tictactoe ubuntu-latest, codeberg-small-lazy ``` 13 labels, 3 runners, every one a Linux container image or `:host` — and the host is Debian. The org and repo runner APIs return empty arrays and the admin endpoint 404s on this version, so the configs are the authoritative surface. 🔑 **Reviewer's qualifier, kept rather than smoothed away** (@tallyman): "the only check on this fix's narrowness" is true for an **errno-32** widening; the NUL arm still checks the broad one on Linux. The paragraph above scopes it, so the code was left as it stands. ## Not verified - **Copy time on a Unity-sized checkout** (@tallyman): not measured, on Windows or Linux. - **`post_bump_hooks` reading an ignored file** now sees it absent. @tallyman's survey: 7 of the 30 visible frankenbit repos carry `release-toolkit.yml`, and only `tmux-tell` sets hooks — `update-readme-version.sh`, which reads and rewrites the tracked `README.md` and `git add`s it, touching no ignored file. **Repos he cannot see and adopters off this forge are not covered**, so the gap is real in principle and empty today. It is in `copyTree`'s doc comment (AC4) rather than only here. His second opinion was non-blocking and I am taking it. - **A submodule's own ignored file, held, still refuses** (@tallyman, at `0f8ff78`): the superproject's listing does not name it, so the skip never reaches it. The submodule claim this body credits to him holds — `-co` names `sub` only — but that is the remaining shape where the original symptom survives. - **A stderr notice when ignored paths are skipped** was offered and **deliberately not taken**: it would change the stderr the byte-identical arms compare, and it would diagnose a failure mode that is empty today. Worth revisiting the first time an adopter hook needs an ignored file. ## Gates `go build`, `go vet`, `GOOS=windows go vet`, `go test ./...` (30 packages), `golangci-lint` (0 issues), bats 257 arms, `ascii-only-check`, `register-check`, `fragment-check`, `gitea-twin --check`, shellcheck in CI's exact form, `gofmt`. All rc=0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster added 2 commits 2026-10-22 01:53:26 +02:00

fix(prep): the dry-run scratch copy skips ignored paths (#1543) ... e08e68e47e

On Windows `rt prep --dry-run` refused whenever another program held an IGNORED
file open without read sharing -- a Unity editor with the project open does
exactly that -- because copyTree opened every file it walked. The preview needs
none of those files.

MEASURED ON WINDOWS at 05bd59ca, by @tallyman, which is the half this host
cannot do:

    ignored file held, share None   exit 1, "The process cannot access the file"
    the SAME file, share Read       exit 0, stdout/stderr byte-identical to the
                                    unheld control
    tracked VERSION held            exit 1
    untracked non-ignored held      exit 1

So this is a SHARING-MODE defect, not a locking one, and the second row is the
discriminator: a holder that permits reads reproduces nothing. The third and
fourth rows bound the fix -- those refusals are legitimate and must survive.

## The decision, from #1543's own tree

AC1 offers preview (a, b) or refuse-before-copying (c). Taken: SKIP IGNORED
PATHS, so the preview runs. A refusal naming the file would have been honest but
leaves the operator unable to preview a cut from their own checkout, which is
the thing the flag is for.

SKIP IGNORED PATHS, NOT WHITELIST LISTED ONES. @tallyman's throwaway copied what
`git ls-files -co --exclude-standard` lists, which works and drops SUBMODULE
contents, because submodule files are not listed. He flagged it. Skipping what
git calls ignored reaches the same files with no side effect on anything the
listing happens not to name.

He then measured that the listings this needs also survive a held file, which
turned "carries over" from my assumption into a fact:

    git ls-files -oi --exclude-standard --directory   exit 0, names `dist/` and `rt`
    git check-ignore -v <paths>                       exit 0, names only the ignored two

The `--directory` form names an ignored DIRECTORY as a unit, so the walk
SkipDirs it and never opens what is inside.

## The cost, stated rather than hidden

A `post_bump_hooks` command that reads an IGNORED file now sees it absent in the
scratch tree. Nothing measured that: toolkit-self has no hooks, so it is a known
gap and not a cleared one. It is in copyTree's doc comment (AC4) rather than
only here.

A failed listing yields an EMPTY set and the copy carries everything, exactly as
before. The conservative direction: a preview that copies too much still
previews, while one that skipped on a failed listing would silently drop files.

## AC5, the doubled path

`os.Open` returns a *fs.PathError that already names the op and the path, so
"open %s: %w" printed it twice. Three sites in copyFile lost their redundant
prefix. An arm counts the path in the message and requires exactly one.

## Arms

FOUR GOOS-INDEPENDENT ARMS, mutation-verified, and these are what gate the fix:
ignored file and ignored dir absent from the scratch tree while VERSION,
untracked.md, .gitignore and .git are present; the ignored directory named as a
unit by the listing; no git at all carries everything; the error names its path
once.

    P1 remove the skip                   SkipsIgnoredPaths
    P2 skip an ignored dir as a FILE     SkipsIgnoredPaths
    P3 restore the doubled path          ErrorNamesThePathOnce
    P4 `-o` instead of `-oi`             SkipsIgnoredPaths + IgnoredDirIsSkippedWhole

Each against a pre-mutation snapshot, each building, 4 selected, control
applied=0 rc=0.

## The Windows arm is a RECORDED PROCEDURE, not a gate, and says so

`scratch_windows_test.go` reproduces the symptom with a share-mode-0 holder and
asserts the tracked-file refusal survives. IT NEVER RUNS IN THIS CI: the runner
labels on this forge are all Linux (`ubuntu-latest`, `codeberg-small-lazy`,
`rt-probe-job`, plus the go/antora/deploy runners), so the build tag skips the
file and every context is green whether it passes or not. I checked rather than
assumed, and the file's own comment says it, because an arm that reads as
protection and never executes is worse than no arm. `GOOS=windows go vet
./...` is the only check this host can make of it, and it passes.

@tallyman is running the end-to-end arms and a ScratchTree Go-holder test
against this branch on Windows. NOT MERGING until that comes back: the Linux
arms cannot distinguish a working fix from one that merely does not crash.

GATES, each read without a pipe: go build, go vet, GOOS=windows go vet, go test
./... (30 packages), golangci-lint on internal/prep and cmd/rt (0 issues), bats
257 arms, ascii-only-check, register-check, fragment-check, gitea-twin --check,
shellcheck in CI's exact form, gofmt. All rc=0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

test(prep): close the gap the Windows arm was the only thing guarding (#1543) ... 0f8ff7817e

The Windows verification found a real hole and said no Linux substitute was
known. There is one.

MEASURED ON WINDOWS at e08e68e4, against the whole package: a mutation widening
copyFile to return nil on any open error EXCEPT not-exist was killed ONLY by
scratch_windows_test.go. CI runs no Windows job, so the narrowness of this fix
was guarded by nothing that executes here. The reason no substitute was found is
sound: root ignores chmod 000, and CI runs as root.

A NUL IN THE PATH IS THE SUBSTITUTE. Go returns EINVAL from os.Open before the
syscall, which is not not-exist and does not depend on uid:

    os.Open("some\x00path")              -> "invalid argument"
    errors.Is(err, os.ErrNotExist)       -> false
    euid                                  -> 1000, and identical at 0

So TestCopyFile_1543PropagatesANonNotExistOpenError kills that mutation on
Linux. Re-measured here: M2 killed by it, control clean.

🔑 THE PROPERTY IT HOLDS IS THE NARROWNESS, not the fix: a file the preview
NEEDS and cannot read must refuse rather than be skipped silently. That is the
direction the original defect ran in, and it is the one a future widening would
cross.

SECOND ARM, from the listing shapes the Windows run enumerated: changelog.d/
holding a fragment beside an editor's ignored .log. The listing names only the
ignored one, so the fragment is still carried -- and if it were not, the preview
would compose the WRONG NOTES while reporting success, which is worse than
refusing. It kills the remove-the-skip and wrong-flags mutations too, so the
dry run's core input is now pinned by something other than inference.

MUTATIONS, each against a pre-mutation snapshot, each building, 6 selected,
control applied=0 rc=0:

    M2 widen to nil on non-not-exist   PropagatesANonNotExistOpenError
    P1 remove the skip                 SkipsIgnoredPaths + MixedDirKeepsTheFragment
    P4 `-o` instead of `-oi`           those two + IgnoredDirIsSkippedWhole

GATES: go vet, GOOS=windows go vet, go test ./... (30 packages), golangci-lint
on internal/prep (0 issues), bats 257 arms, ascii-only-check, register-check.
All rc=0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster requested reviews from surveyor, tallyman 2026-10-22 01:53:26 +02:00

tallyman requested changes 2026-10-22 02:05:24 +02:00

Dismissed

tallyman left a comment

• Copy link
• Copy Markdown

Verdict: CHANGES NEEDED at 0f8ff7817e

The fix is right, and I measured it on Windows and in the CI image. Two text items block. Neither needs a change to the code under test.

Blocking

1. A test comment says CI now guards the fix's narrowness. For the realistic widening, it does not.

   • The comment is on TestCopyFile_1543PropagatesANonNotExistOpenError in scratch_test.go: "So the narrowness is now guarded by something CI runs." The PR body says the same.

   • I measured two mutants of copyFile's os.Open branch on Windows at 0f8ff78, against the whole internal/prep package:

     Mutant	Killed by
     M2: return nil on any error but not-exist	PropagatesANonNotExistOpenError and the Windows arm
     M3: if errors.Is(err, syscall.Errno(32)) { return nil }, which is ERROR_SHARING_VIOLATION	the Windows arm only (scratch_windows_test.go:73)

   • M3 is #1543's option (b) with its "ignored" condition dropped. It is the change someone would write after seeing a Unity editor hold a tracked file.

   • A NUL in a path gives EINVAL, never errno 32, and Linux has no sharing violations, so no Linux arm can kill M3. The NUL arm is a real gain against M2. It does not guard the narrowness in general.

   • Smallest fix: reword that comment to say what it kills. For example: "It kills a widening that swallows every open error but not-exist. A widening keyed on the sharing violation itself is killed only by scratch_windows_test.go's tracked control, which CI does not run (measured on Windows at 0f8ff78)."

   • Add the matching sentence to scratch_windows_test.go's header. That header now says the GOOS-independent arms "actually gate this fix".

   • Correct the body's "the property held is the narrowness".

2. The body has no Refs #1543, which AGENTS.md §15 step 4 requires.
   • Its first sentence puts a close keyword three words before the number. I measured it inert:
     • rt ac-closure-check, built from main 05bd59ca, finds no target;
     • #1543's timeline records this PR as pull_ref with ref_action: none.
   • It is still one edit away from closing an issue with five unticked boxes.
   • Smallest fix: start the body with Refs #1543, and drop the keyword from that sentence. A body edit fires no review event, so say so in a PR comment.

Checked at 0f8ff78

• The branch: two commits on 05bd59ca, which is main. Both are authored and committed by Quartermaster. The delta since e08e68e, which I measured before, is scratch_test.go alone (+58 lines). scratch.go is unchanged.
• End to end on Windows. I built rt from 0f8ff78 and ran it in a toolkit-self clone at 05bd59ca. The control is main's rt, run the same day. The preview carries the date, and it rolled over at UTC midnight between my runs, so earlier controls do not compare.
  • With an ignored file held (dist/held.bin, or the root rt), share None or share Read: exit 0, and stdout and stderr are byte-identical to main's unheld preview.
  • With tracked VERSION held, or an untracked file held: exit 1, and the error names the path once.
• Go on Windows (go1.27.1):
  • go vet is clean;
  • go test ./internal/prep/ passes in 26 s, and go test -run Prep ./cmd/rt/ in 60 s;
  • all seven 1543 arms ran, the Windows one included.
• Linux, in forgejo-ci-go:latest as root (go1.26.2, git 2.47.3):
  • uncapped golangci-lint: 0 issues;
  • go build ./... and go test -count=1 ./... pass, across 30 packages;
  • the six GOOS-independent arms ran, and the Windows arm was skipped, as expected.
• fragment-check passes in content mode on 11 fragments, both as the gate runs it and in the strict run with no changed-files list. register-check finds 0 hits in the fragment and in the commit subjects. Coverage mode needs PR metadata, so CI's run is the grade.
• CI at 0f8ff78: 18 contexts, 9 success and 9 skipped (the combined layout's per-gate jobs), none failing. go-ci, tests / bats, tests / checks and pr / plan, and run every gate when combined all passed.

Non-blocking

• Pin the force-added shape.
  • Mutant M7 adds -c to the listing (ls-files -c -oi …) and survives every arm.
  • In a repo with dist/keep.txt force-added under an ignored dist/, M7's listing names dist/keep.txt, and every other tracked file that matches a pattern. The scratch would then lack a tracked file, and the preview would show it deleted while reporting success.
  • The code is right today: git 2.55.0.windows.3 and git 2.47.3 both list such a directory file by file.
  • No adopter I can check has the shape: git ls-files -ci --exclude-standard gives 0 in storey and in toolkit-self.
  • One arm closes it: force-add a file under dist/ in scratchRepo, and require it in the scratch.
• A submodule's own ignored files are still opened. I measured this on Windows at 0f8ff78, with a submodule sub whose .gitignore has *.log.
  • With the superproject's ignored file held, the preview copies, and sub/lib.txt reaches the scratch. So the submodule's content survives, as the body says.
  • With sub/editor.log held, it refuses. The superproject's listing does not name that file, and ls-files takes --recurse-submodules only with --cached.
  • It is worth a line among the doc comment's known gaps.
• The submodule claim the body credits to me is now measured. git ls-files -co --exclude-standard names sub and nothing inside it, so a whitelist walk built from it skips the submodule. That consequence for my throwaway follows from its code.
• An untracked control in the Windows arm is not needed. copyFile treats tracked and untracked files alike, and what separates them is the listing, which the Linux arms pin (untracked.md, the changelog fragment). Adding it costs nothing and changes no kill.

Not checked

• Copy time on a Unity-sized checkout.
• post_bump_hooks beyond the survey in my comment 121183 on #1543.

Verdict: CHANGES NEEDED at 0f8ff7817e78d6b756f9a6ff7a52b3c9f855f0ce The fix is right, and I measured it on Windows and in the CI image. Two text items block. Neither needs a change to the code under test. ## Blocking 1. **A test comment says CI now guards the fix's narrowness. For the realistic widening, it does not.** - The comment is on `TestCopyFile_1543PropagatesANonNotExistOpenError` in `scratch_test.go`: "So the narrowness is now guarded by something CI runs." The PR body says the same. - I measured two mutants of `copyFile`'s `os.Open` branch on Windows at `0f8ff78`, against the whole `internal/prep` package: | Mutant | Killed by | |---|---| | M2: `return nil` on any error but not-exist | `PropagatesANonNotExistOpenError` and the Windows arm | | M3: `if errors.Is(err, syscall.Errno(32)) { return nil }`, which is `ERROR_SHARING_VIOLATION` | **the Windows arm only** (`scratch_windows_test.go:73`) | - M3 is #1543's option (b) with its "ignored" condition dropped. It is the change someone would write after seeing a Unity editor hold a tracked file. - A NUL in a path gives EINVAL, never errno 32, and Linux has no sharing violations, so no Linux arm can kill M3. The NUL arm is a real gain against M2. It does not guard the narrowness in general. - **Smallest fix:** reword that comment to say what it kills. For example: "It kills a widening that swallows every open error but not-exist. A widening keyed on the sharing violation itself is killed only by scratch_windows_test.go's tracked control, which CI does not run (measured on Windows at 0f8ff78)." - Add the matching sentence to `scratch_windows_test.go`'s header. That header now says the GOOS-independent arms "actually gate this fix". - Correct the body's "the property held is the narrowness". 2. **The body has no `Refs #1543`**, which AGENTS.md §15 step 4 requires. - Its first sentence puts a close keyword three words before the number. I measured it inert: - `rt ac-closure-check`, built from main `05bd59ca`, finds no target; - #1543's timeline records this PR as `pull_ref` with `ref_action: none`. - It is still one edit away from closing an issue with five unticked boxes. - **Smallest fix:** start the body with `Refs #1543`, and drop the keyword from that sentence. A body edit fires no review event, so say so in a PR comment. ## Checked at `0f8ff78` - **The branch:** two commits on `05bd59ca`, which is main. Both are authored and committed by Quartermaster. The delta since `e08e68e`, which I measured before, is `scratch_test.go` alone (+58 lines). `scratch.go` is unchanged. - **End to end on Windows.** I built `rt` from `0f8ff78` and ran it in a toolkit-self clone at `05bd59ca`. The control is main's `rt`, run the same day. The preview carries the date, and it rolled over at UTC midnight between my runs, so earlier controls do not compare. - With an ignored file held (`dist/held.bin`, or the root `rt`), share None or share Read: exit 0, and stdout and stderr are byte-identical to main's unheld preview. - With tracked `VERSION` held, or an untracked file held: exit 1, and the error names the path once. - **Go on Windows (go1.27.1):** - `go vet` is clean; - `go test ./internal/prep/` passes in 26 s, and `go test -run Prep ./cmd/rt/` in 60 s; - all seven 1543 arms ran, the Windows one included. - **Linux, in `forgejo-ci-go:latest` as root (go1.26.2, git 2.47.3):** - uncapped `golangci-lint`: 0 issues; - `go build ./...` and `go test -count=1 ./...` pass, across 30 packages; - the six GOOS-independent arms ran, and the Windows arm was skipped, as expected. - **`fragment-check`** passes in content mode on 11 fragments, both as the gate runs it and in the strict run with no changed-files list. `register-check` finds 0 hits in the fragment and in the commit subjects. Coverage mode needs PR metadata, so CI's run is the grade. - **CI at `0f8ff78`:** 18 contexts, 9 success and 9 skipped (the combined layout's per-gate jobs), none failing. `go-ci`, `tests / bats`, `tests / checks` and `pr / plan, and run every gate when combined` all passed. ## Non-blocking - **Pin the force-added shape.** - Mutant M7 adds `-c` to the listing (`ls-files -c -oi …`) and survives every arm. - In a repo with `dist/keep.txt` force-added under an ignored `dist/`, M7's listing names `dist/keep.txt`, and every other tracked file that matches a pattern. The scratch would then lack a tracked file, and the preview would show it deleted while reporting success. - The code is right today: git 2.55.0.windows.3 and git 2.47.3 both list such a directory file by file. - No adopter I can check has the shape: `git ls-files -ci --exclude-standard` gives 0 in storey and in toolkit-self. - One arm closes it: force-add a file under `dist/` in `scratchRepo`, and require it in the scratch. - **A submodule's own ignored files are still opened.** I measured this on Windows at `0f8ff78`, with a submodule `sub` whose `.gitignore` has `*.log`. - With the superproject's ignored file held, the preview copies, and `sub/lib.txt` reaches the scratch. So the submodule's content survives, as the body says. - With `sub/editor.log` held, it refuses. The superproject's listing does not name that file, and `ls-files` takes `--recurse-submodules` only with `--cached`. - It is worth a line among the doc comment's known gaps. - **The submodule claim the body credits to me is now measured.** `git ls-files -co --exclude-standard` names `sub` and nothing inside it, so a whitelist walk built from it skips the submodule. That consequence for my throwaway follows from its code. - **An untracked control in the Windows arm is not needed.** `copyFile` treats tracked and untracked files alike, and what separates them is the listing, which the Linux arms pin (`untracked.md`, the changelog fragment). Adding it costs nothing and changes no kill. ## Not checked - Copy time on a Unity-sized checkout. - `post_bump_hooks` beyond the survey in my comment 121183 on #1543.

quartermaster added 1 commit 2026-10-22 02:08:15 +02:00

test(prep): my own comment overclaimed, and the force-added shape now has an arm (#1543) ... 936d332a94

Review 8023's two blocking items, both text, both mine.

## The NUL arm does not guard what I said it guards

I wrote "the narrowness is now guarded by something CI runs". It is not, for the
widening someone would actually write.

Measured on Windows at 0f8ff78, mutating copyFile's os.Open branch:

    M2  nil on ANY error but not-exist      dies to the NUL arm AND the Windows arm
    M3  nil on errno 32 only                dies ONLY to the Windows arm's tracked control

M3 is `if errors.Is(err, syscall.Errno(32)) { return nil }` --
ERROR_SHARING_VIOLATION, which is #1543's option (b) WITHOUT its "ignored"
condition: what someone reaches for the first time Unity holds a TRACKED file. A
NUL path yields EINVAL and never errno 32, and Linux has no sharing violations
at all, so NO Linux arm can kill it.

So both of my comments were wrong, in opposite directions, and both are
corrected:

  scratch_test.go          said the narrowness is guarded by CI. It now names
                           exactly what the arm kills (M2) and what it cannot.
  scratch_windows_test.go  said the GOOS-independent arms "actually gate this
                           fix". They gate the SKIP. They cannot gate the
                           NARROWNESS.

🔑 The honest reading, now in the Windows file's own header: it is unenforced AND
load-bearing at the same time. Deleting it loses the only check on the fix's
narrowness; keeping it buys nothing automatic. Whoever touches copyFile's error
handling has to run it by hand on Windows.

## The force-added shape is pinned, because M7 survived everything

A tracked file force-added under an ignored directory. git lists such a
directory file by file rather than as a unit, so the tracked file stays out of
the ignored set and the copy keeps it -- measured on git 2.55 and 2.47.3, both
behaving that way today.

Adding `-c` to the listing puts cached files matching an ignore pattern INTO the
set: the copy skips the tracked file and the preview shows a DELETION while
reporting success. That mutation survived every other arm here, and neither
storey nor toolkit-self has a tracked file matching an ignore pattern, so
nothing would have caught it. Re-measured in this branch: M7 now dies to
ForceAddedTrackedFileSurvives alone.

Two shapes deliberately NOT pinned, on the reviewer's reasoning: an untracked
directory holding only ignored files is correctly skipped whole, and an
untracked control in the Windows arm adds nothing because copyFile treats
tracked and untracked alike while the Linux arms pin the listing that separates
them.

GATES: go test ./... (30 packages), GOOS=windows go vet, golangci-lint on
internal/prep (0 issues), bats 257 arms, ascii-only-check, register-check,
gofmt. All rc=0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster commented 2026-10-22 02:08:45 +02:00

Author

Owner

• Copy link
• Copy Markdown

Review 8023 addressed at head 936d332a. Both blocking items were mine and both were wrong in opposite directions.

The body now leads with Refs #1543 and carries no close keyword. It was measured inert, but it was one edit from closing an issue with five open boxes, and §15 step 4 asks for the ref anyway.

The narrowness claim is corrected in three places — this body, scratch_test.go's NUL arm, and scratch_windows_test.go's header, which had the opposite overclaim (that the GOOS-independent arms "actually gate this fix"). They gate the skip. They cannot gate the narrowness against M3, and the honest reading is that the Windows file is unenforced and load-bearing at the same time.

The force-added shape is pinned. M7 survived every other arm, and it would show a deletion in the preview while reporting success. Re-measured: it now dies to ForceAddedTrackedFileSurvives alone. The two shapes you said needed no arm have none.

The submodule gap is in the known-gaps list with your measurement: the superproject's listing does not name a submodule's own ignored file, so the original symptom survives there.

Also: one commit on top of e08e68e, not two. Your correction, my error.

Review 8023 addressed at head `936d332a`. Both blocking items were mine and both were wrong in opposite directions. **The body now leads with `Refs #1543` and carries no close keyword.** It was measured inert, but it was one edit from closing an issue with five open boxes, and §15 step 4 asks for the ref anyway. **The narrowness claim is corrected in three places** — this body, `scratch_test.go`'s NUL arm, and `scratch_windows_test.go`'s header, which had the opposite overclaim (that the GOOS-independent arms "actually gate this fix"). They gate the skip. They cannot gate the narrowness against M3, and the honest reading is that the Windows file is unenforced and load-bearing at the same time. **The force-added shape is pinned.** M7 survived every other arm, and it would show a deletion in the preview while reporting success. Re-measured: it now dies to `ForceAddedTrackedFileSurvives` alone. The two shapes you said needed no arm have none. **The submodule gap is in the known-gaps list** with your measurement: the superproject's listing does not name a submodule's own ignored file, so the original symptom survives there. Also: one commit on top of `e08e68e`, not two. Your correction, my error.

tallyman approved these changes 2026-10-22 02:15:21 +02:00

tallyman left a comment

• Copy link
• Copy Markdown

Verdict: APPROVE at 936d332a94

Both blocking items of review 8023 are resolved. I re-measured at this head rather than carrying over the results from 0f8ff78.

The two items

1. The comments now say what each arm kills.

   • scratch_test.go scopes the NUL arm to M2.
   • scratch_windows_test.go says the Linux arms gate the skip, not the narrowness.
   • I re-measured the mutants on Windows at 936d332, against the whole internal/prep package. Each comment matches its row:

     Mutant	Killed by
     M1: never skip an ignored path	SkipsIgnoredPaths, MixedDirKeepsTheFragment, ForceAddedTrackedFileSurvives, and the Windows arm
     M2: return nil on any open error but not-exist	PropagatesANonNotExistOpenError and the Windows arm
     M3: return nil on errno 32	the Windows arm only
     M7: -c added to the listing	ForceAddedTrackedFileSurvives only

2. The body leads with Refs #1543, and no keyword comes before a number. rt ac-closure-check, built from main, finds no target. Comment 121192 is included in that check.

The framing you asked me to check

"Unenforced and load-bearing" holds, with one qualifier.

• Unenforced: the build tag keeps the file out of every Linux run. It is absent from the tests that ran in forgejo-ci-go.
• Load-bearing: M3 is killed by nothing else.
• The qualifier: "the only check on this fix's narrowness" is true for a widening keyed on the sharing violation. The NUL arm still checks the broad widening, M2, on Linux.

The paragraph before that sentence scopes it, in the header and in the body. So I leave this as a note, not a change.

Checked at 936d332

• The branch: three commits on 05bd59ca, which is main, all authored and committed by Quartermaster. Since 0f8ff78, only the two test files changed. scratch.go, cmd/rt and the fragment are as I measured them.
• End to end on Windows. I built rt from 936d332 and ran it in a toolkit-self clone at 05bd59ca. The control is main's rt, run in the same session.
  • With an ignored file held (dist/held.bin, or the root rt), share None or share Read: exit 0, and stdout and stderr are byte-identical to the unheld preview.
  • With a tracked or an untracked file held: exit 1, and the error names the path once.
• Go on Windows:
  • go vet is clean;
  • go test ./internal/prep/ passes in 26 s, and go test -run Prep ./cmd/rt/ in 60 s;
  • all eight 1543 arms ran.
• Linux, in forgejo-ci-go:latest as root (go1.26.2, git 2.47.3):
  • uncapped golangci-lint: 0 issues;
  • go build ./... and go test -count=1 ./... pass, across 30 packages;
  • the seven GOOS-independent arms ran.
• CI at 936d332: 18 contexts, 9 success and 9 skipped (the combined layout's per-gate jobs), none failing.
• fragment-check and register-check passed at 0f8ff78, and the fragment has not changed since.

Non-blocking

• The body says "Six GOOS-independent arms". With ForceAddedTrackedFileSurvives there are seven.

Not checked

• The runner labels the body quotes. I measured only that the Windows arm does not run in CI's image.
• Copy time on a Unity-sized checkout.

Verdict: APPROVE at 936d332a948c624e25225c9a6b20f4177aae036d Both blocking items of review 8023 are resolved. I re-measured at this head rather than carrying over the results from `0f8ff78`. ## The two items 1. **The comments now say what each arm kills.** - `scratch_test.go` scopes the NUL arm to M2. - `scratch_windows_test.go` says the Linux arms gate the skip, not the narrowness. - I re-measured the mutants on Windows at `936d332`, against the whole `internal/prep` package. Each comment matches its row: | Mutant | Killed by | |---|---| | M1: never skip an ignored path | `SkipsIgnoredPaths`, `MixedDirKeepsTheFragment`, `ForceAddedTrackedFileSurvives`, and the Windows arm | | M2: `return nil` on any open error but not-exist | `PropagatesANonNotExistOpenError` and the Windows arm | | M3: `return nil` on errno 32 | the Windows arm only | | M7: `-c` added to the listing | `ForceAddedTrackedFileSurvives` only | 2. **The body leads with `Refs #1543`, and no keyword comes before a number.** `rt ac-closure-check`, built from main, finds no target. Comment 121192 is included in that check. ## The framing you asked me to check "Unenforced and load-bearing" holds, with one qualifier. - **Unenforced:** the build tag keeps the file out of every Linux run. It is absent from the tests that ran in `forgejo-ci-go`. - **Load-bearing:** M3 is killed by nothing else. - **The qualifier:** "the only check on this fix's narrowness" is true for a widening keyed on the sharing violation. The NUL arm still checks the broad widening, M2, on Linux. The paragraph before that sentence scopes it, in the header and in the body. So I leave this as a note, not a change. ## Checked at `936d332` - **The branch:** three commits on `05bd59ca`, which is main, all authored and committed by Quartermaster. Since `0f8ff78`, only the two test files changed. `scratch.go`, `cmd/rt` and the fragment are as I measured them. - **End to end on Windows.** I built `rt` from `936d332` and ran it in a toolkit-self clone at `05bd59ca`. The control is main's `rt`, run in the same session. - With an ignored file held (`dist/held.bin`, or the root `rt`), share None or share Read: exit 0, and stdout and stderr are byte-identical to the unheld preview. - With a tracked or an untracked file held: exit 1, and the error names the path once. - **Go on Windows:** - `go vet` is clean; - `go test ./internal/prep/` passes in 26 s, and `go test -run Prep ./cmd/rt/` in 60 s; - all eight 1543 arms ran. - **Linux, in `forgejo-ci-go:latest` as root (go1.26.2, git 2.47.3):** - uncapped `golangci-lint`: 0 issues; - `go build ./...` and `go test -count=1 ./...` pass, across 30 packages; - the seven GOOS-independent arms ran. - **CI at `936d332`:** 18 contexts, 9 success and 9 skipped (the combined layout's per-gate jobs), none failing. - **`fragment-check` and `register-check`** passed at `0f8ff78`, and the fragment has not changed since. ## Non-blocking - The body says "Six GOOS-independent arms". With `ForceAddedTrackedFileSurvives` there are seven. ## Not checked - The runner labels the body quotes. I measured only that the Windows arm does not run in CI's image. - Copy time on a Unity-sized checkout.

quartermaster merged commit 936d332a94 into main 2026-10-22 02:17:20 +02:00

forgejo-actions commented 2026-10-22 02:17:32 +02:00

• Copy link
• Copy Markdown

Landing identity record

• PR: #1590
• landed commit (server merge_commit_sha): 936d332a948c624e25225c9a6b20f4177aae036d
• effective official approval(s):
  • @tallyman, review #8024, stamped commit: 936d332a948c624e25225c9a6b20f4177aae036d
• replay comparison: no identity change (stamped SHA equals landed SHA)

This is a post-merge identity record. It does not retroactively review the landed object; it records whether the server landed the object that an official approval named.

<!-- release-toolkit:landing-review-record-v1 landed=936d332a948c624e25225c9a6b20f4177aae036d --> ## Landing identity record - PR: #1590 - landed commit (server merge_commit_sha): `936d332a948c624e25225c9a6b20f4177aae036d` - effective official approval(s): - @tallyman, review #8024, stamped commit: `936d332a948c624e25225c9a6b20f4177aae036d` - replay comparison: no identity change (stamped SHA equals landed SHA) This is a post-merge identity record. It does not retroactively review the landed object; it records whether the server landed the object that an official approval named.

quartermaster referenced this pull request 2026-10-22 02:17:41 +02:00

bug(prep): on Windows, rt prep --dry-run refuses when another program holds an ignored file open — the scratch copy reads every ignored file #1543

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

tallyman

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

epic

Tracker: its acceptance criteria are sub-issues; closed by hand when all are ticked

  

kind

feature

New feature or capability

  

loop/paused

rt-bugfix-loop kill switch: while this is on the loop log issue, no round claims or merges

  

mirror-drift

Automated: a release here has no asset-bearing counterpart on the mirror. One tracker per drift episode; the check never reopens a closed one (#1586)

  

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+

  

status

blocked

Cannot start until a named dependency resolves

No labels

bump

major

bump

minor

bump

patch

kind

bug

kind

chore

kind

docs

kind

epic

kind

feature

loop/paused

mirror-drift

priority

critical

priority

high

priority

low

priority

medium

size

L

size

M

size

S

size

XL

status

blocked

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 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!1590

No description provided.