Repository navigation
feat: port the VCS seam from Go upstream; correct the v0.2.0 release notes - #3
Merged
Merged
Conversation
Closes the architectural gap that the 27 earlier fixes worked around: the Go original grew an internal/vcs seam with pluggable backends, and this port was still a flat git-only GitBackend. Six of the sixteen drift items were gated on that one rewrite. The seam (crates/treehouse-core/src/vcs/) - BackendRegistry, extensible by register() alone -- no trait edit, no enum. - worktree_backend_name reproduces Go's marker rule exactly: lstat then one stat, so a DANGLING marker is a read error and never an "absent marker" that falls through to the enclosing repository. - destructive_backend_for_worktree REFUSES a markerless path rather than letting git walk up and reset the repository enclosing an in-project pool. - The guarded-reset pair: is_worktree_safe_to_reset resolves the ref once to an immutable SHA, and reset_worktree_to_ref takes git's own HEAD.lock (O_CREAT|O_EXCL), re-reads HEAD under it, refuses if it moved, re-checks dirtiness, and only then read-tree/clean. This is now on the LIVE acquire path (pool.rs reset_for_acquisition), which was the whole point -- a pair nobody calls does not mitigate the race. Backends and capabilities - jj backend (vcs/jj.rs, registered at vcs/mod.rs:301). jj was never installed on the build machine, so there is NO end-to-end coverage; it is unit-tested against a faked command layer and the release notes say so plainly. - APFS clonefile sharing behind --apfs-sharing, degrading to a plain copy on any failure. Documented limitation: it never engages through a symlinked pool root, which includes the macOS default TMPDIR and /tmp. - .worktreeinclude seeding wired into Pool::get; seeded paths recorded in state, and a reset deletes only files the tool itself seeded -- a missing or empty inventory authorises zero deletions. Self-update - `treehouse update` now reaches the download -> verify-sha256 -> extract -> atomic-replace pipeline. An earlier round wrote that pipeline but left it uncalled while cmd_update printed success; that is fixed and now proven by a fixture laid out the way release.yml names assets. - release_workflow_contract: 7 tests parse the real release.yml and assert every published asset has a sidecar the updater accepts. Nothing tied those two files together before, which is why a naming mismatch shipped. - Enabled --features toon in the release build. It was off by default, so released binaries silently emitted JSON for --format toon. Docs corrected against the code, not the other way round: several release claims did not match what the binary does -- jj's real coverage, the apfs/include-file flags that exist, seeding being on the acquire path, and a TOON claim of "~40-60% fewer tokens" that measurement contradicted (TOON is ~5% LARGER on a one-slot pool). Known and disclosed: a doubled "git git" prefix in some git error labels (cosmetic, pre-existing); worktree creation still calls git.worktree_add directly rather than through dispatch, so jj support is asymmetric. Tests 401 -> 493, all passing; clippy clean under -D warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The branch's CI was red at 549c6aa on all four jobs. Reproduced the Linux failures in a container rather than guessing at them, and the two causes were not what they looked like. `destroy --include-in-use` was still a no-op on Linux. The post-kill survivor re-scan found the pid it had just killed and skipped the worktree. sysinfo refreshes `cwd` as `UpdateKind::OnlyIfNotSet` (system.rs:2478) and gates the re-read on `p.cwd.is_none()` (system.rs:2336), so a cwd it already holds is never re-read: a SIGKILLed child becomes a zombie, its /proc/<pid>/cwd vanishes, and the cached path — the worktree it died in — is what destroy saw. A zombie has already terminated, so it must not read as "still using this worktree". Excluding zombies fixes the flag on Linux. `status` is re-read on every refresh, so it is the fresh signal to filter on. The gc test asserted `!context.repo_root.starts_with(&pool.root)`, but `Pool::root` IS the repo root (`pool.rs:476`), so it compared the path with itself. It passed on macOS only because git resolves /var to /private/var and the two strings stopped matching; on Linux /tmp is not a symlink and they matched. The mistake worth catching is the repo collapsing to the pool directory, so compare against that instead. Two Windows tests hardcoded POSIX quoting while `quote_path` is correctly platform-branched (`main.rs:1815`) — cmd.exe groups on double quotes. Each now asserts its own platform's real form, including the doubled-quote escape. Also ran `cargo fmt`: jj.rs, vcs/mod.rs, pool.rs, shell.rs and cli.rs were never formatted, which is what the fmt job was rejecting. Verified: 493 tests pass and clippy is clean with -D warnings locally; the treehouse-core suite is 387/387 on Linux in a container as a non-root user, matching the exact count the ubuntu job was failing on. The Windows branch is unverified by execution and is left to CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`cargo test --workspace` ran without --no-fail-fast, so cargo stopped at the
first failing test target and every later one reported nothing. On Windows the
`treehouse` bin target failed first, so treehouse-core's lib tests never ran
there — 17 failures, across pool, state, updater, git::shell and the VCS seam,
had never been executed. On Linux the lib target failed first, so update_e2e
never ran. And --features hardening did not compile, behind a step that only
reached it once the step before passed.
Two of the failures were product defects, not test noise.
validate_seed_inventory used `Path::is_absolute()` to decide what "absolute"
means for a seed inventory. That inventory is git's path language, not a host
path: git spells every separator `/` on every platform, which is why Go checks
it with `path.IsAbs` (gitvcs.go:959). On Windows `Path::is_absolute()` is
`has_root() && prefix().is_some()`, so `/etc/passwd` is not absolute, passed
validation, and then `worktree.join("/etc/passwd")` dropped everything after the
drive prefix and resolved to C:\etc\passwd. A gate whose job is to authorise
deleting ignored files *inside* a worktree could name files outside it. Now
rejected on every platform.
destroy --include-in-use was a no-op on both platforms. sysinfo refreshes a
process's cwd only when it is unset (UpdateKind::OnlyIfNotSet), so a killed
process keeps reporting the directory it died in. On Linux that process is a
zombie; on Windows it lingers until the last handle closes, and sysinfo's
Windows backend assigns `status` once as Run and never reassigns it, so a
zombie comparison alone can never fire there. Zombies are now excluded on unix,
and Windows asks the OS via GetExitCodeProcess, failing closed.
The remaining 15 are tests asserting POSIX spelling on a Windows host: CRLF
checkouts (the repo-level core.autocrlf=false line was lost from the port —
Go has it at cmd/e2e_test.go:744), git's forward-slash .git pointer, verbatim
\\?\ paths from canonicalize, "/pool" as an absolute root, a gzip fixture for a
platform that extracts zip, hand-formatted JSON containing backslashes, a
display() path spliced into a file:// URL, and a staged binary exec'd while
still open for writing.
Verified by execution: 493 tests pass locally; treehouse-core is 404/404 under
an injected core.autocrlf=true (it was 4/404 before this change, so the CRLF
fix is reproduced, not assumed); treehouse-core is 387/387 on Linux in a
container as a non-root user; --features hardening now compiles.
Not verified by execution: every Windows path. This machine cannot run them, so
the Windows fixes are reasoned from the assertion text and from reading
sysinfo's Windows backend, and CI is the first thing to actually execute them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…elling Windows runners hand Rust the 8.3 short form of a temp path (RUNNER~1) and git writes the long form (runneradmin), so the two strings name the same directory and the containment check could not hold. Normalising separators, which is what this asserted before, only fixes the other half of the problem. Compare the resolved paths instead, which is what the product's own same_file (pool.rs:2032) does — dev+inode on unix, canonicalize on Windows, the latter of which expands the 8.3 name. The product was already correct here, so this is a test fix, and the failure was never evidence of a clone-identity defect. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands the VCS seam from Go upstream, and corrects the release notes so they describe the code that actually exists.
Why
The previous PR fixed 27 behavioural gaps. Six of the sixteen remaining drift items were gated on one thing: the Go original grew an
internal/vcsseam with pluggable backends, and this port was still a flat git-onlyGitBackend. That is one rewrite, not six gaps.The seam —
crates/treehouse-core/src/vcs/BackendRegistry, extensible byregister()alone — no trait edit, no enum, no dispatch-site change.worktree_backend_namereproduces Go's marker rule exactly:lstatthen onestat, so a dangling marker is a read error and never an "absent marker" that falls through to the enclosing repository.destructive_backend_for_worktreerefuses a markerless path rather than letting git walk up and reset the repository that encloses an in-project pool.The guarded-reset pair — the part that matters
is_worktree_safe_to_resetresolves the ref once to an immutable SHA and records HEAD.reset_worktree_to_reftakes git's ownHEAD.lock(O_CREAT|O_EXCL), re-reads HEAD under it, refuses if it moved, re-checks dirtiness under it, and only then doesread-tree/clean.It is now on the live acquire path (
pool.rs::reset_for_acquisition). An earlier round shipped this pair correctly implemented and well tested — and not called, because it would have made every slot refuse reuse. Wiring it is what actually mitigates the HEAD-moved and became-dirty-between-check-and-reset races.Backends and capabilities
vcs/jj.rs, registered atvcs/mod.rs:301).--apfs-sharing, degrading to a plain copy on any failure.Documented limitation: it never engages through a symlinked pool root, which includes the macOS default TMPDIR and
/tmp..worktreeincludeseeding, wired intoPool::get; seeded paths recorded in state. A reset deletes only files the tool itself seeded — a missing or empty inventory authorises zero deletions.Self-update — and a test that was lying
treehouse updatenow reaches the download → verify-sha256 → extract → atomic-replace pipeline. An earlier round wrote that pipeline and left it uncalled whilecmd_updateprintedSuccessfully updated; that is fixed.The root cause was that nothing tied
release.ymltoupdater.rs.release_workflow_contractadds 7 tests that parse the real workflow file viainclude_str!and assert every published asset has a sidecar the updater accepts. Mutation-tested in both directions — narrowing the updater fails 4 tests, renaming the asset fails 3, deleting the sidecar writes fails 2.Also enabled
--features toonin the release build: it was off by default, so released binaries silently emitted JSON for--format toon.Docs corrected against the code
Several shipped claims did not match the binary. The release notes said "the jj backend is not implemented" while a 2016-line backend was registered. The CHANGELOG claimed TOON uses "~40-60% fewer tokens" — measured, TOON is ~5% larger on a one-slot pool. It also claimed no
--apfs-sharing/--include-fileflags existed (they do), that seeding had no caller (it does), and pointed at anAGENTS.mdthat does not exist.Known and disclosed
git gitprefix in some git error labels. Cosmetic, pre-existing, not fixed here.git.worktree_adddirectly rather than through dispatch, so jj support is asymmetric: reset dispatches, creation does not.Verification
cargo test --workspace --no-fail-fast: 493 passing, 0 failing.cargo clippy --workspace --all-targets -- -D warnings: clean.Every fix is mutation-verified — the wiring is reverted in a scratch check and a test is confirmed to fail.
🤖 Generated with Claude Code