fix(farm): worktree containment refused a legitimate root when two spellings disagreed (#539) - #540
Merged
Merged
Conversation
…ellings disagreed (#539) `validateWorktreeRoot` compared paths LEXICALLY. `path.resolve` normalizes separators and `.`/`..`, but it neither expands a Windows 8.3 short name nor follows a link - so two spellings of ONE directory compared as different paths and a worktree root genuinely inside the repo was refused: FARM_WORKTREE_ROOT resolves to 'C:\Users\RUNNER~1\...', outside the repository root 'C:/Users/runneradmin/...' Both name the same directory. The message told the operator to point the root inside the repo, where it already was, and the only escape was FARM_ALLOW_EXTERNAL_WORKTREE_ROOT=1 - disabling the guard outright to work around a false positive in it. Found by #521's coverage-union job on its FIRST run. plugins/ca/tools had never run on Windows in CI (the tools job is ubuntu-only); the new host cell went red with 15 failures, all downstream of this one refusal. It does not reproduce on a developer box whose profile path is already 8.3-clean, which is why the suite passed locally at 521/521 the whole time. Both sides are now canonicalized before comparing. This TIGHTENS the guard rather than loosening it: resolving links means a junction inside the repo whose target is outside is now refused, where the lexical check accepted it. That was never exploitable - measured, `fs.rm` removes the link rather than recursing through it, and #163's delete containment was intact - but accepting it was still wrong. A missing path is normal here, since the root is created later, so canonicalization walks up to the deepest EXISTING ancestor and re-attaches the rest. Any realpath failure that is NOT absence refuses and names the path, per #163's fail-closed stance; realpath is injectable so that branch is reachable from a test rather than asserted in a comment. 11 tests, 5/5 mutants killed. Two survived the first round and both were real: the not-yet-created case was routed under the CANONICAL repo, where the lexical form is already inside, so it passed whether or not the walk-up worked; and nothing executed the fail-closed branch at all. The first is now routed through the link and asserts its own precondition; the second is covered by injecting EACCES, ELOOP and a code-less error. ca 2.10.2 -> 2.10.3: farm.js is a declared shipped artifact. Claude-Session: https://claude.ai/code/session_01WJgVfZw7J81PB7mwpHyUxx
) Found by reading the job's own log rather than its status. Every run since the union shipped reported: blobs merged: 0 no host produced a blob - nothing to merge (advisory) ...and PASSED, because the job is advisory. An advisory job that looks healthy and measures nothing is worse than an absent one. Cause: `actions/upload-artifact` defaults to `include-hidden-files: false`, and vitest's default blob directory is `.vitest-reports/` - a dot-directory, so the upload matched no files and the merge job had nothing to download. The warning was in the log the whole time: "No files were found with the provided path". Blobs now go to `coverage-blobs/`, which needs no flag to be correct. The defensive branch that made this visible was worth having: the merge job said it had nothing rather than printing a single-host figure as though it were the union. Kept, and now backed by a contract that fails if any union artifact path is hidden without `include-hidden-files: true` - asserted structurally, not by directory name, so the next hidden path fails too. Verified load-bearing: restoring the dot-directory turns test_ci_impact red. Claude-Session: https://claude.ai/code/session_01WJgVfZw7J81PB7mwpHyUxx
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 #539.
The bug
validateWorktreeRootcompared paths lexically.path.resolvenormalizes separators and./.., but it neither expands a Windows 8.3 short name nor follows a link — so two spellings of one directory compared as different paths:Both name the same directory. The error told the operator to point the root inside the repo — where it already was — and the only escape was
FARM_ALLOW_EXTERNAL_WORKTREE_ROOT=1, which disables the guard outright to work around a false positive in it.How it was found
By #521's coverage-union job, on its first run.
plugins/ca/toolshad never run on Windows in CI — thetoolsjob is ubuntu-only. The new host cell went red with 15 failures, every one downstream of this single refusal.It does not reproduce on a developer box whose profile path is already 8.3-clean, which is why the suite passed locally at 521/521 the entire time. A defect a second host was always going to find, and single-host CI never could.
Red proof
Two real, existing spellings of one directory (a junction reproduces the CI shape on any platform):
Before:
After: all five as expected.
It tightens, it does not loosen
Note the third row. Resolving links means a junction inside the repo whose target is outside is now refused, where the lexical check accepted it.
That was never exploitable — measured directly,
fs.rm({recursive:true})useslstatand removes the link rather than recursing through it, so #163's delete containment was intact:But accepting it was still wrong, and the canonical comparison is the correct semantics.
Handling absence
A missing root is normal — it is created later — so canonicalization walks up to the deepest existing ancestor and re-attaches the rest. Any realpath failure that is not absence refuses and names the path, per #163's fail-closed stance.
realpathis injectable so that branch is reachable from a test rather than asserted in a comment.Verification
11 new tests. Full tools suite 532 passed. 5/5 mutants killed — including the three half-fixes (canonicalize only the repo side, only the root side, or revert to lexical).
Two mutants survived the first round, and both were real
missing-path-gives-up-immediately. My not-yet-created test routed the path under the canonical repo — where the lexical form is already inside — so it passed whether or not the walk-up worked. Now routed through the link, and it asserts its own precondition (path.relative(...).startsWith("..")istrue) so it cannot quietly stop testing the thing it names.realpath-failure-falls-through. Nothing executed the fail-closed branch at all.EACCESandELOOPcannot be induced portably, which is exactly whyrealpathis now injectable; covered by injecting both plus a code-less error.Follow-on
The
Coverage union <os: windows-latest>cell should go green with this, closing the loop #521 opened.ca2.10.2 → 2.10.3 —farm.jsis a declared shipped artifact.