bug(windows): three path gaps - the #181 version_files guard misses `..\` paths, recover-pending-cut joins an absolute changelog, an atomic write fails over a read-only file #1526

New issue

Closed

opened 2026-10-02 23:24:42 +02:00 by claude · 1 comment

claude commented

2026-10-02 23:24:42 +02:00

Member

Three Windows-only path gaps, listed as out of scope in #1502 because they had only been found by reading the code. All three are now measured. Each comes from treating / as the only separator or the only marker of an absolute path, or from relying on POSIX rename semantics.

Measured 2026-10-02 at 96d2646, on an LF clone. Windows: Go 1.27.1 windows/amd64, native, from Git Bash. Linux: go1.25.14 linux/amd64 in golang:1.25.

1. The `version_files` repo-relative guard (#181) misses `\` paths

Fixture: repo R whose release-toolkit.yml has version_files: ['..\outside/VERSION'], and a sibling R\..\outside\VERSION containing 9.9.9.

Windows, native rt manifest-check in R (exit 0):

  OK:   config valid: release-toolkit.yml
  OK:   version_files agree: 9.9.9

It reads the version from a file outside the repository.

Windows, native rt prep --dry-run in R, with TMP pointed at a fixture directory. The dry run copies the repo to %TMP%\rt-prep-dryrun-*, so the entry resolves to %TMP%\outside\VERSION. That file held 7.7.7 and afterwards held 0.1.0. The disclosure line does not name it:

[rt prep] updated ..\outside/VERSION -> 0.1.0 (VERSION strategy)
[rt prep] [DRY-RUN] working tree MODIFIED/DELETED by this run (undo: git checkout -- <path>): CHANGELOG.md, changelog.d/thing.added.md

A real rt prep joins the same entry to the checkout root (filepath.Join(repoRoot, vf), cmd/rt/prep.go:345). It would therefore write R\..\outside\VERSION. That is read from the code, not run.

Linux, same fixture: config valid, then FAIL: version_file missing: ..\outside/VERSION. There \ is an ordinary filename byte, and the entry names a directory inside the repository.

Cause. Load (internal/config/config.go:194) tests for a leading /, and hasTraversal (:349-353) tests for .. between / separators. On Windows \ is also a separator, and a drive letter or a leading \ makes a path absolute or rooted. The basename guard does not help, because path.Base still returns VERSION whenever the last separator is /.

2. `resolveUnderRoot` treats only a leading `/` as absolute

cmd/rt/recover_pending_cut.go:579, native on Windows:

resolveUnderRoot("C:\\repo", "C:\\Users\\alex\\AppData\\Local\\Temp\\...\\CHANGELOG.md")
  = "C:\\repo/C:\\Users\\alex\\AppData\\Local\\Temp\\...\\CHANGELOG.md"
os.Stat(that): The filename, directory name, or volume label syntax is incorrect.

On Linux the absolute path comes back unchanged. The doc comment promises "the same rule decide.resolve applies", and decide.resolve (internal/decide/decide.go:1362) uses filepath.IsAbs. On Windows, with an absolute changelog:, decide therefore reads the right file and recover-pending-cut does not. By the code at :245-250, the verb then stops with cannot read <changelog> (cannot-grade) where Linux proceeds.

3. An atomic write fails over a read-only destination

Three writers rename a same-directory temp file over their destination:

writeFileAtomic, internal/release/cutter.go:637: the cut's CHANGELOG seal and its rollback;
writeRecoveryFileAtomic, cmd/rt/recover_pending_cut.go:532;
the manifest store's Write, internal/manifest/store.go:253. #1502 did not list this one, but it has the same shape.

With the destination at chmod 0444 (on Windows that sets FILE_ATTRIBUTE_READONLY):

Writer	Windows	Linux
`writeFileAtomic`	`rename ...\.rt-tmp-2366756064 ...\CHANGELOG.md: Access is denied.`	replaced, still `-r--r--r--`
`writeRecoveryFileAtomic`	`rename ...\.rt-recover-2233725798 ...\CHANGELOG.md: Access is denied.`	replaced, still `-r--r--r--`
manifest `Write`	`manifest: rename ...json.tmp -> ...json: ... Access is denied.`	replaced, now `-rw-r--r--`

On Windows the destination keeps its old content, and no temp file is left behind. Go's Windows os.Remove clears the read-only attribute and retries (src/os/file_windows.go, func Remove), so the deferred cleanup still deletes a read-only temp file. os.Rename has no such retry.

git does not record the read-only attribute, so a checkout never produces this state. It takes a deliberate attrib +R, or a tool that sets the attribute.

Why it matters

#181's guard exists so that a config entry cannot make rt read or write outside the repository. On Windows, one entry does both. The --dry-run preview writes outside its scratch copy and does not report the write.
recover-pending-cut is the operator's recovery verb. On Windows it cannot grade a repository whose changelog: is absolute, although decide reads the same path correctly.
A read-only CHANGELOG fails the cut's seal write on Windows and passes on Linux. The error names a temp file and "Access is denied", not the attribute.

Out of scope

Rejecting \ in version_files on every OS. That would change what Linux accepts, and whether a checked-in config must be portable is a separate question.
Other path-handling sites not named above.

Acceptance criteria

On Windows, config.Load rejects a version_files entry that leaves the repository or is not local when \ is read as a separator: a .. segment, a leading \, a drive letter or a UNC prefix. On Linux, the same entries are accepted or rejected as before, with the same messages. A test fails on Windows before the change and is mutation-verified. — DONE @ #1527 (2fe9db5 and 81086ff, reviewed as 8086700). TestLoad_versionFilesBackslashPaths failed natively on Windows before each commit, and mutants M1a, M1b and M1c are each killed there. On Linux the same entries are graded as before: 51 of 51 rt runs at 8086700 are byte-identical to dd871e8, messages included.
On Windows, resolveUnderRoot returns an OS-absolute path unchanged, as decide.resolve does. Linux output is unchanged. A test fails on Windows first and is mutation-verified. — DONE @ #1527 (b46db1d, reviewed as 8086700). TestResolveUnderRootLeavesAnAbsolutePathAlone failed natively on Windows first, and mutant M2a (back to a leading-/ test) is killed there. On Linux filepath.IsAbs is HasPrefix(path, "/") (Go 1.27.1 source).
The three atomic writers either replace a read-only destination on Windows as they do on Linux, or refuse with a message that names the attribute. The PR records the choice as a decision tree (AGENTS.md section 7). Tests fail on Windows first and are mutation-verified. — DONE @ #1527 (5ca2973 and 8086700, reviewed as 8086700). Chosen: clear the attribute and retry, on Windows only, with the attribute put back if the retry fails. The decision tree is in #1527's body. The three writer tests and TestRename_replacesAReadOnlyDestination failed natively on Windows with Access is denied. first. Mutants M3a to M3f are each killed there.
go vet ./... && go test -count=1 ./... passes natively on Windows (Git Bash) and in golang:1.25. On Linux, the output of the touched verbs is byte-identical before and after. — DONE @ #1527 (8086700, reviewed as 8086700). go vet is clean, and go test passes 30 of 30 packages natively from Git Bash and in golang:1.25. Linux: --help, manifest-check, prep --dry-run and recover-pending-cut --dry-run are byte-identical to dd871e8 over 51 runs. Those runs did not reach recover-pending-cut's changelog write, the cut's seal or a manifest write. Those lines are identical off Windows by construction, and unit tests passing in golang:1.25 pin them.
A changelog.d fragment for the change. — DONE @ #1527 (2fe9db5, reviewed as 8086700). changelog.d/1526-windows-path-gaps.fixed.md; rt fragment-check in content mode passes.

Anchor

Split from #1502's out-of-scope list. The landed Windows build (96d2646) made a native measurement possible.

Three Windows-only path gaps, listed as out of scope in #1502 because they had only been found by reading the code. All three are now measured. Each comes from treating `/` as the only separator or the only marker of an absolute path, or from relying on POSIX rename semantics. Measured 2026-10-02 at `96d2646`, on an LF clone. Windows: Go 1.27.1 windows/amd64, native, from Git Bash. Linux: go1.25.14 linux/amd64 in `golang:1.25`. ## 1. The `version_files` repo-relative guard (#181) misses `\` paths Fixture: repo `R` whose `release-toolkit.yml` has `version_files: ['..\outside/VERSION']`, and a sibling `R\..\outside\VERSION` containing `9.9.9`. Windows, native `rt manifest-check` in `R` (exit 0): ``` OK: config valid: release-toolkit.yml OK: version_files agree: 9.9.9 ``` It reads the version from a file outside the repository. Windows, native `rt prep --dry-run` in `R`, with `TMP` pointed at a fixture directory. The dry run copies the repo to `%TMP%\rt-prep-dryrun-*`, so the entry resolves to `%TMP%\outside\VERSION`. That file held `7.7.7` and afterwards held `0.1.0`. The disclosure line does not name it: ``` [rt prep] updated ..\outside/VERSION -> 0.1.0 (VERSION strategy) [rt prep] [DRY-RUN] working tree MODIFIED/DELETED by this run (undo: git checkout -- <path>): CHANGELOG.md, changelog.d/thing.added.md ``` A real `rt prep` joins the same entry to the checkout root (`filepath.Join(repoRoot, vf)`, `cmd/rt/prep.go:345`). It would therefore write `R\..\outside\VERSION`. That is read from the code, not run. Linux, same fixture: `config valid`, then `FAIL: version_file missing: ..\outside/VERSION`. There `\` is an ordinary filename byte, and the entry names a directory inside the repository. **Cause.** `Load` (`internal/config/config.go:194`) tests for a leading `/`, and `hasTraversal` (`:349-353`) tests for `..` between `/` separators. On Windows `\` is also a separator, and a drive letter or a leading `\` makes a path absolute or rooted. The basename guard does not help, because `path.Base` still returns `VERSION` whenever the last separator is `/`. ## 2. `resolveUnderRoot` treats only a leading `/` as absolute `cmd/rt/recover_pending_cut.go:579`, native on Windows: ``` resolveUnderRoot("C:\\repo", "C:\\Users\\alex\\AppData\\Local\\Temp\\...\\CHANGELOG.md") = "C:\\repo/C:\\Users\\alex\\AppData\\Local\\Temp\\...\\CHANGELOG.md" os.Stat(that): The filename, directory name, or volume label syntax is incorrect. ``` On Linux the absolute path comes back unchanged. The doc comment promises "the same rule decide.resolve applies", and `decide.resolve` (`internal/decide/decide.go:1362`) uses `filepath.IsAbs`. On Windows, with an absolute `changelog:`, `decide` therefore reads the right file and `recover-pending-cut` does not. By the code at `:245-250`, the verb then stops with `cannot read <changelog>` (cannot-grade) where Linux proceeds. ## 3. An atomic write fails over a read-only destination Three writers rename a same-directory temp file over their destination: - `writeFileAtomic`, `internal/release/cutter.go:637`: the cut's CHANGELOG seal and its rollback; - `writeRecoveryFileAtomic`, `cmd/rt/recover_pending_cut.go:532`; - the manifest store's `Write`, `internal/manifest/store.go:253`. #1502 did not list this one, but it has the same shape. With the destination at `chmod 0444` (on Windows that sets `FILE_ATTRIBUTE_READONLY`): | Writer | Windows | Linux | |---|---|---| | `writeFileAtomic` | `rename ...\.rt-tmp-2366756064 ...\CHANGELOG.md: Access is denied.` | replaced, still `-r--r--r--` | | `writeRecoveryFileAtomic` | `rename ...\.rt-recover-2233725798 ...\CHANGELOG.md: Access is denied.` | replaced, still `-r--r--r--` | | manifest `Write` | `manifest: rename ...json.tmp -> ...json: ... Access is denied.` | replaced, now `-rw-r--r--` | On Windows the destination keeps its old content, and no temp file is left behind. Go's Windows `os.Remove` clears the read-only attribute and retries (`src/os/file_windows.go`, `func Remove`), so the deferred cleanup still deletes a read-only temp file. `os.Rename` has no such retry. git does not record the read-only attribute, so a checkout never produces this state. It takes a deliberate `attrib +R`, or a tool that sets the attribute. ## Why it matters - #181's guard exists so that a config entry cannot make `rt` read or write outside the repository. On Windows, one entry does both. The `--dry-run` preview writes outside its scratch copy and does not report the write. - `recover-pending-cut` is the operator's recovery verb. On Windows it cannot grade a repository whose `changelog:` is absolute, although `decide` reads the same path correctly. - A read-only CHANGELOG fails the cut's seal write on Windows and passes on Linux. The error names a temp file and "Access is denied", not the attribute. ## Out of scope - Rejecting `\` in `version_files` on every OS. That would change what Linux accepts, and whether a checked-in config must be portable is a separate question. - Other path-handling sites not named above. ## Acceptance criteria - [x] On Windows, `config.Load` rejects a `version_files` entry that leaves the repository or is not local when `\` is read as a separator: a `..` segment, a leading `\`, a drive letter or a UNC prefix. On Linux, the same entries are accepted or rejected as before, with the same messages. A test fails on Windows before the change and is mutation-verified. — DONE @ #1527 (`2fe9db5` and `81086ff`, reviewed as `8086700`). `TestLoad_versionFilesBackslashPaths` failed natively on Windows before each commit, and mutants M1a, M1b and M1c are each killed there. On Linux the same entries are graded as before: 51 of 51 `rt` runs at `8086700` are byte-identical to `dd871e8`, messages included. - [x] On Windows, `resolveUnderRoot` returns an OS-absolute path unchanged, as `decide.resolve` does. Linux output is unchanged. A test fails on Windows first and is mutation-verified. — DONE @ #1527 (`b46db1d`, reviewed as `8086700`). `TestResolveUnderRootLeavesAnAbsolutePathAlone` failed natively on Windows first, and mutant M2a (back to a leading-`/` test) is killed there. On Linux `filepath.IsAbs` is `HasPrefix(path, "/")` (Go 1.27.1 source). - [x] The three atomic writers either replace a read-only destination on Windows as they do on Linux, or refuse with a message that names the attribute. The PR records the choice as a decision tree (AGENTS.md section 7). Tests fail on Windows first and are mutation-verified. — DONE @ #1527 (`5ca2973` and `8086700`, reviewed as `8086700`). Chosen: clear the attribute and retry, on Windows only, with the attribute put back if the retry fails. The decision tree is in #1527's body. The three writer tests and `TestRename_replacesAReadOnlyDestination` failed natively on Windows with `Access is denied.` first. Mutants M3a to M3f are each killed there. - [x] `go vet ./... && go test -count=1 ./...` passes natively on Windows (Git Bash) and in `golang:1.25`. On Linux, the output of the touched verbs is byte-identical before and after. — DONE @ #1527 (`8086700`, reviewed as `8086700`). `go vet` is clean, and `go test` passes 30 of 30 packages natively from Git Bash and in `golang:1.25`. Linux: `--help`, `manifest-check`, `prep --dry-run` and `recover-pending-cut --dry-run` are byte-identical to `dd871e8` over 51 runs. Those runs did not reach `recover-pending-cut`'s changelog write, the cut's seal or a manifest write. Those lines are identical off Windows by construction, and unit tests passing in `golang:1.25` pin them. - [x] A `changelog.d` fragment for the change. — DONE @ #1527 (`2fe9db5`, reviewed as `8086700`). `changelog.d/1526-windows-path-gaps.fixed.md`; `rt fragment-check` in content mode passes. ## Anchor Split from #1502's out-of-scope list. The landed Windows build (`96d2646`) made a native measurement possible.

claude added the

labels

2026-10-02 23:24:42 +02:00