Repository navigation
fix: close 27 audited gaps against Go upstream v3.1.0 - #2
Merged
Merged
Conversation
Audit run as a 67-agent workflow: 6 domain finders compared the Rust port against a clone of kunchenguid/treehouse at v3.1.0, then every finding passed two adversarial lenses (existence, impact). 29 findings confirmed (4 CRITICAL / 11 HIGH / 14 MEDIUM), 1 refuted, 54 lower-severity leads recorded unverified. Headline: the port is faithful in the pool core but fails open exactly where it is most destructive. confirm() returns true on stdin EOF, so `treehouse return <p> < /dev/null` hard-resets and erases uncommitted work while exiting 0 — Go refuses. The port also lags six upstream releases, and 12 of the 16 drift items are in v3.0.0 alone, 6 of them gated on the same missing VCS seam (one rewrite, not six gaps). Doc-only; no code changes.
MISSING.md documented 29 verified gaps between this port and the Go baseline (kunchenguid/treehouse v3.1.0). This closes them. The port was faithful in the pool core but failed open exactly where it is most destructive. The headline: confirm() returned true on stdin EOF, so `treehouse return <p> < /dev/null` hard-reset a dirty worktree, erased uncommitted work and exited 0. Go refuses. Verified fixed end to end -- both the uncommitted edit and the untracked file now survive. Safety invariants - M-002 marker precondition on every destructive git call, so an in-project pool can no longer walk up and reset the repository enclosing it - M-003/M-004 process table is re-read per scan instead of snapshotted at Pool::open; post-kill re-scan now fails closed on a scan error - M-012 clone-identity scoping (two clones of one origin no longer share a slot), M-013 unlanded-work guard before reset --hard - M-009 prune re-classifies under the lock on fresh state and uses the non-forced remove; M-010 parent-dir fsync after rename Fixing M-004 also required a second cause the audit missed: protected_chain treated parent_pid() == None as fatal, and sysinfo returns None for any ppid == 0 -- including pid 1, which every ancestry walk ends at. So terminate_with_grace always failed on unix and destroy --include-in-use was permanently a no-op. Go handles this at terminate.go:88-90. Durability and CLI - M-027 `treehouse update` now really downloads, verifies sha256 and atomically replaces. It previously printed success without doing anything; round 1 wrote the pipeline but left it uncalled, which the review caught. - M-007 treehouse.toml without max_trees no longer breaks every command - M-008/M-023 --root and TREEHOUSE_ROOT with flag > env > config > default - M-016/M-017/M-026 return <name>, return --all, lease -- all shipped together with the release preconditions they require - M-024 -b (branch creation, distinct from --base), --unique-leaf, --worktree-path now reach the core - M-018 --format json|toon honored on destroy/prune/return/gc Exit code 3 for an unreturned dirty worktree ships BEHIND TREEHOUSE_EXIT_STRICT=1, default off. Go shipped it as a v3.0.0 BREAKING CHANGE and our own plan Appendix B records exit 0, so flipping the default would break existing scripts. Opt in deliberately, with a release note. Still open, deliberately: the VCS seam (M-028) and --apfs-sharing / --include-file, which depend on it. That is a multi-week rewrite (jj backend, APFS fileclone, .worktreeinclude seeding). M-002 and M-021 are implemented directly on the flat GitBackend so they were not blocked. Tests: 136 -> 317, all passing. Build and clippy warning-free. No test weakened, ignored or re-baselined. Every fix is mutation-verified -- the wiring is reverted in a scratch check and a test is confirmed to fail. MISSING.md carries the per-finding citations on both sides. 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
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.
Closes the 29 verified gaps catalogued in
MISSING.md, audited against a clone of kunchenguid/treehouse at v3.1.0 (cb7fb26).The port was faithful in the pool core — pool layout,
short_hash,next_name,owner_alive,ClassSet, the two-phase destroy reservation and the lock all match. But it failed open exactly where it is most destructive.The headline
confirm()returnedtrueon stdin EOF (main.rs:725, where the?onread_linewas dead code because EOF yieldsOk(0)). So:confirmed the destructive reset, ran
checkout --detach --force+reset --hard+clean -fdon a dirty worktree, erased uncommitted work, and exited 0. Every non-TTY caller — CI, Makefile, hook, agent script, cron — hit it. Go refuses (internal/ui/prompt.go:46-50,cmd/return_cmd.go:399-418).Verified fixed end to end:
return <p> < /dev/null, dirty treeexit 0Aborted.,exit 0, data intactTREEHOUSE_EXIT_STRICT=1exit 3, data intactSafety invariants
reset --hardpreviously walked up and hit the repository enclosing the pool.ProcessTablewas enumerated once atPool::openand never refreshed;refresh()had zero call sites. Now re-read per scan, and the post-kill survivor re-scan fails closed instead ofunwrap_or_default().getwould reset clone B's live slot.reset --hard.worktree remove(the forced variant was losing git's own dirty-tree guard).A second cause the audit missed
Fixing M-004 needed more than the stale snapshot.
protected_chaintreatedparent_pid() == Noneas fatal and failed closed — but sysinfo returnsNonefor any process withppid == 0, including pid 1, which every ancestry walk ends at. Soterminate_with_gracealways returnedErron unix anddestroy --include-in-usewas permanently a no-op. Go handles this atterminate.go:88-90.Two pre-existing tests used
if let Ok(...)and so passed whether the walk returnedOkorErr; they are replaced with real assertions rather than deleted.Durability and CLI
treehouse updatenow really downloads, verifies sha256 against the sidecar and atomically replaces. It previously printedSuccessfully updatedwhile downloading nothing, and returnedOk(0)on network failure. Addingflate2/tar/zip, all with default features off.treehouse.tomlwithoutmax_treeswas a hard parse error that broke every command in that repo. (A test had worked around it with a comment; the loader is now fixed.)--rootandTREEHOUSE_ROOT, precedenceflag > env > config > default.return <name>,return --all,lease— shipped together with the release preconditions they need. Adding the commands without the guards would let one caller release a slot another owns.-b(branch creation, distinct from the existing--basewhich is the reset target),--unique-leaf,--worktree-path.--format json|toonis now honored on destroy/prune/return/gc.Exit code 3 for an unreturned dirty worktree ships behind
TREEHOUSE_EXIT_STRICT=1, default off. Go made this a v3.0.0 BREAKING CHANGE; our owndocs/rust-port-plan.mdAppendix B records exit 0 as the contract, so flipping the default would break existingset -escripts. The opt-in is deliberate — flip it with a release note when you want Go parity.--env-pathis now a visible alias of--rootand its meaning changed:--env-path /tmp/poolsresolves to/tmp/pools/.treehouse(Go--rootsemantics) rather than the old no-op~/.treehouse. README updated.Deliberately still open
internal/vcs/+gitvcs/+jjvcs/+fileclone/; this port is still a flat git-onlyGitBackend. That is a multi-week rewrite (jj backend, APFS clonefile sharing,.worktreeincludeseeding). M-002 and M-021 are implemented directly on the flat backend so Tier 0/1 were not blocked.--apfs-sharing(#153) and--include-file(#131) — depend on the seam above.Roughly one rewrite, not six separate gaps: six of the sixteen drift items share that one blocker.
Tests
136 → 317, all passing. Build clean,
clippy --all-targetswarning-free. No test weakened, ignored, or re-baselined; the two#[cfg_attr(not(windows), allow(dead_code))]additions are on Windows-only updater functions so the linux/macOS build still type-checks that path.Every fix is mutation-verified: the wiring is reverted in a scratch check and a test is confirmed to fail — removing the
apply_intocall, disablingreturn --allJSON, removing theHeldOnlyByCwdexclusion, restoring thegc --allearly return, restoring the no-op nonzero-shell-exit branch, and routing lease mode back to the flag-dropping call.Review notes
MISSING.mdis attached — every finding carries the symptom,file:linecitations on both sides, a fix sketch, and the verifier's reasoning. Two things a reviewer should know:versiondoes not make Go quarantine everything (there is a carve-out atstate.go:143). The residual is a wire-format nit, not CRITICAL.P0_PARITYvsUPSTREAM_DRIFTbucket splits that carry decision weight were each re-checked against the v2.1.1 release commit939cb59.Audit ran as a 67-agent workflow with two adversarial lenses per finding; implementation as two further rounds (17 agents) with an adversarial review pass that caught an unwired updater round 1 had left looking complete.
🤖 Generated with Claude Code