fix(bin): keep teardown's landed-work check working on git older than 2.38 - #9
Merged
Merged
Conversation
…rite-tree The landed-work content fallback used `git merge-tree --write-tree`, which exists only from git 2.38. Ubuntu 22.04 and its WSL image still ship git 2.34, where that call fails with a usage error and the whole check silently answered "not landed": teardown then refused genuinely landed work on every such host. Keep the full merge where git has it and fall back to a 3-way `read-tree -i -m` into a throwaway index otherwise. The fallback touches neither the worktree nor the real index, and resolves only trivial merges - a merge it cannot resolve leaves unmerged stages, `write-tree` fails, and the caller refuses, the same direction a merge conflict already takes. Two test fixes go with it, both for verdicts that depended on the test host: - Pin FM_HOME to a per-case sandbox. Teardown resolves its data dir, secondmate registry and Relay .env from FM_HOME, so an ambient one - every session firstmate itself launches exports one - made the run answer from the operator's real home. That refused at the secondmate registry binding and aborted the suite before the later cases ran at all. - Cover the content check with the newer subcommand shimmed away, so a modern git also proves the fallback, plus its refusal of genuinely unlanded work. Record the external-tool version axis in firstmate-coding-guidelines so the next change checks the oldest supported git before depending on a subcommand.
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
2 participants
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.
Intent
Fix the environment-dependent fm-teardown.test.sh failure that fails locally on this WSL machine while passing in GitHub CI, so it cannot abort the suite and hide later regressions.
Required approach, all of which was done before this run: reproduce the failure locally through the real test entry point (bin/fm-test-run.sh) before changing code; capture the exact expected behavior, observed failure, inputs, and repeatability; separate the initiating test operation, the local environment condition exposing it, and the visible early suite abort; compare the failing WSL path against the proven GitHub CI path; test the smallest counterfactual for each likely environment dependency; seek disconfirming evidence rather than assuming filesystem, git configuration, shell version, locale, or worktree layout is responsible; and confirm current behavior after the recent upstream merge rather than relying on an old observation from base 1cb900c.
Two distinct environment-dependent root causes were found and fixed.
Product bug behind the brief's 'content-landed' case. bin/fm-teardown.sh's content_in_default used 'git merge-tree --write-tree', which exists only from git 2.38. This host runs git 2.34 (Ubuntu 22.04 and its WSL image), where the call exits 128 and the whole landed-work content fallback silently answered 'not landed'. That is a real product bug, not just a test bug: on any such host teardown refuses genuinely landed work. The fix keeps the full merge where git provides it and falls back to a 3-way 'read-tree -i -m' into a throwaway index otherwise. The fallback touches neither the worktree nor the real index, and resolves only trivial merges: a merge it cannot resolve leaves unmerged stages, write-tree fails, and the caller refuses, the same direction a merge conflict already takes. The temp index is a mktemp -d directory because git rejects the zero-length file mktemp would otherwise create. The hybrid shape is deliberate: dropping every host to the conservative trivial merge would make teardown newly refuse a squash-merged branch whose default branch later gained overlapping edits in the same file, and that behavior could not be re-verified locally on git 2.34.
Test-host leak that, after the upstream merge, actually aborted the suite first at case 14 of 65, before the content-landed case ever ran. tests/fm-teardown.test.sh's run_teardown overrode state and config but not FM_HOME, and teardown resolves its data dir, secondmate registry and Relay .env from FM_HOME. Every session firstmate itself launches exports one, so the run answered from the operator's real ~/.firstmate registry and refused with 'no registry binding for secondmate task-x1'. FM_HOME is now pinned to a per-case sandbox directory. It is deliberately NOT the case dir itself, because teardown refuses removal targets inside the active firstmate home and the case worktrees live there.
Constraints honored: do not weaken teardown's content-landed, uncommitted-work, or unlanded-work protections; prefer making the test deterministic across supported environments over a skip; keep the change minimal and portable; leave executable regression coverage that fails under the identified local condition and still exercises all later cases.
Regression coverage added: two new cases (content-landed-old-git, content-unlanded-old-git) shim 'merge-tree' away from git entirely so a modern git also proves the fallback path and proves it still refuses genuinely unlanded work; and the existing forced-secondmate herdr child preflight case now exports a decoy ambient FM_HOME carrying a foreign secondmate registry, so the FM_HOME pin is asserted rather than assumed. Counterfactuals were run: removing the pin makes that case fail both with and without an ambient FM_HOME, and the merge-tree shim was verified to intercept every global-option form while passing all other git commands through.
One line of durable knowledge was recorded in .agents/skills/firstmate-coding-guidelines/SKILL.md under 'Compatibility and enforcement': external tool version is a compatibility axis, scripts must run on the oldest release a supported host ships, and a newer subcommand kept for its better behavior needs an older fallback covered by a test that shims the newer one away.
Validation already performed: the fm-teardown suite is green three times through bin/fm-test-run.sh, both with the ambient firstmate home present and with it unset (the CI-like condition); the pr-forge family is 5/5; bin/fm-lint.sh and bin/fm-doc-audience-check.sh are clean.
Evidence to classify, not hide: the portable lanes on this machine are broadly red independently of this change. 21 scripts fail (portable-serial 19/120, plus fm-test-run.test.sh and fm-arm-pretool-check.test.sh). None of them touch the three files this change edits. Spot-checking fm-update, fm-secondmate-sync, fm-session-start and fm-fleet-sync shows they fail with FM_HOME unset too, so they are a different cause: fm-update.test.sh specifically fails because git init defaults to master on this host and because a global check-agents-e2e pre-commit hook fires inside the test's own fixture repos. Those are separate pre-existing host-config leaks, out of scope for this task, and are reported rather than fixed here.
Delivery: open the PR against doitdigital0495/firstmate, never upstream. Drive every gate through the CI-ready return point. Escalate any ask-user finding instead of deciding it, and never use --yes.
What Changed
bin/fm-teardown.sh'scontent_in_defaultno longer depends ongit merge-tree --write-tree(git 2.38+). It still uses that path where git provides it, and otherwise falls back to a newtrivially_merged_treehelper that 3-way merges viaread-tree -i -minto a throwaway index directory. On hosts shipping git 2.34 (Ubuntu 22.04 / WSL) the old call exited 128 and the check silently answered "not landed", so teardown refused genuinely landed work. Unresolvable merges leave unmerged stages,write-treefails, and the caller still refuses.tests/fm-teardown.test.shnow pinsFM_HOMEto a per-case sandbox (not the case dir, which teardown refuses as inside the active firstmate home), so runs no longer resolve the data dir, secondmate registry and Relay.envfrom the operator's real~/.firstmateand abort the suite early. The forced-secondmate herdr child preflight case exports a decoy ambientFM_HOMEwith a foreign registry to assert the pin.content-landed-old-gitandcontent-unlanded-old-gitcases that shimmerge-treeaway from git so a modern git also exercises the fallback and proves it still refuses unlanded work; recorded the external-tool-version compatibility rule in.agents/skills/firstmate-coding-guidelines/SKILL.md.Risk Assessment
✅ Low: Bounded three-file change: the new merge fallback is strictly more conservative than the path it backstops (an unresolvable merge leaves unmerged stages, write-tree fails, and teardown still refuses), it has the only merge-tree call site in the codebase so no sibling path stays broken, and both root causes carry executable regression cases that exercise real teardown behavior rather than source text.
Testing
Ran the fm-teardown suite through its real entry point in four configurations to separate the two root causes. At base the suite aborted after 13 cases at herdr-child-preflight with the operator's ambient FM_HOME present, and base teardown paired with the new tests failed the content-landed case with exit 1 instead of 0, reproducing the git-2.34 product bug where teardown refuses genuinely landed work; the host was confirmed to reject
merge-tree --write-treewith exit 128. At the target commit the full script is green (64 cases, exit 0) both with the ambient firstmate home present and with FM_HOME unset, including the two new cases that shim merge-tree away and prove landed work is recognized while unlanded work is still refused. Evidence is CLI transcripts rather than visual artifacts because the change is a shell CLI teardown path with no rendered surface. The working tree was restored to HEAD and left clean.Evidence: Before/after summary across the four runs
Source: Before/after summary across the four runs
host: git version 2.34.1 (Ubuntu 22.04 / WSL) ambient FM_HOME at test time: /home/daan/.firstmate BASE ba5f4c4 (script+tests), ambient FM_HOME: not ok - herdr-child-preflight: refusal did not explain its non-mutating boundary FM_TEST_SUMMARY total=1 failed=1 skipped_gate=0 duration_ms=28085 cases passed before abort: 13 BASE bin/fm-teardown.sh + NEW tests (isolates the git-2.34 product bug): not ok - content-landed: teardown should succeed when content is already in the default branch: expected exit 0, got 1 FM_TEST_SUMMARY total=1 failed=1 skipped_gate=0 duration_ms=56550 TARGET 59b863c, ambient FM_HOME present: FM_TEST_SUMMARY total=1 failed=0 skipped_gate=0 duration_ms=122158 ok - content fallback recognizes landed work on a git without merge-tree --write-tree ok - the merge-tree-free content check still refuses genuinely unlanded work cases passed: 64 TARGET 59b863c, FM_HOME unset (CI-like): FM_TEST_SUMMARY total=1 failed=0 skipped_gate=0 duration_ms=123163 cases passed: 64Evidence: Host git rejects merge-tree --write-tree
Source: Host git rejects merge-tree --write-tree
# host git: git version 2.34.1 $ git merge-tree --write-tree master HEAD fatal: unknown rev --write-tree exit=128Evidence: Base run: suite aborts at case 14 (FM_HOME leak)
Source: Base run: suite aborts at case 14 (FM_HOME leak)
Evidence: Base teardown script + new tests: landed work wrongly refused on git 2.34
Source: Base teardown script + new tests: landed work wrongly refused on git 2.34
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-teardown.sh:927- Any non-zeromerge-tree --write-treeexit now routes intotrivially_merged_tree, conflating "subcommand missing" (exit 128/129) with "real merge conflict" (exit 1). The outcome is unchanged - a path merge-tree conflicts on is by definition not trivially resolvable, soread-tree -i -mleaves unmerged stages,write-treefails and the caller still refuses - but on a modern git a genuine conflict now pays an extra merge-base + read-tree + write-tree round trip before reaching the same refusal, and the code no longer says which failure it is handling. Branching on the exit status (fall back only when merge-tree is unsupported) would keep intent visible.bin/fm-teardown.sh:894- The throwaway index directory is removed inline but is not registered with the existing EXIT trap (trap teardown_release_locks EXIT, line 209). If teardown is interrupted betweenmktemp -dandrm -rf, a/tmp/fm-teardown-merge-index.*directory leaks. The normal path is clean; this only bites on signal/abort during the content check.tests/fm-teardown.test.sh:1872- Pinning FM_HOME to an always-empty$case_dir/homemeans$SECONDMATE_REGnever exists in any case, so the registry-binding refusal invalidate_firstmate_home_for_removal(bin/fm-teardown.sh:1787) is now skipped by the[ -e "$SECONDMATE_REG" ]guard in every test. The decoy home only proves the pin overrides an ambient FM_HOME; nothing asserts teardown still refuses when its own pinned registry carries a mismatched binding. This matches CI's prior behavior (FM_HOME unset resolved to a repo root with no data/secondmates.md), so it is not a regression against the green baseline - it does make the loss of that coverage permanent rather than host-dependent.✅ **Test** - passed
✅ No issues found.
bash bin/fm-test-run.sh tests/fm-teardown.test.shat target 59b863c with ambient FM_HOME=/home/daan/.firstmate -> exit 0, 64 cases okenv -u FM_HOME bash bin/fm-test-run.sh tests/fm-teardown.test.shat target (CI-like, no ambient home) -> exit 0, 64 cases okCounterfactual:git checkout ba5f4c4 -- bin/fm-teardown.sh tests/fm-teardown.test.shthenbash bin/fm-test-run.sh tests/fm-teardown.test.sh-> exit 1, aborts at caseherdr-child-preflightafter 13 passing cases (FM_HOME leak reproduced)Isolation of the product bug: basebin/fm-teardown.sh+ targettests/fm-teardown.test.sh->not ok - content-landed: teardown should succeed when content is already in the default branch: expected exit 0, got 1Target-only regression cases observed passing:content fallback recognizes landed work on a git without merge-tree --write-treeandthe merge-tree-free content check still refuses genuinely unlanded workHost environment check:git --version(2.34.1) andgit merge-tree --write-tree master HEADin a throwaway repo ->fatal: unknown rev --write-tree, exit 128git status --shortafter restoring both files to HEAD -> clean✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.