Repository navigation
fix(bin): isolate per-home Treehouse pool roots and read null required checks as empty - #4932
mehulbhagwani wants to merge 16 commits into
Conversation
|
Bug report with reproduction: #4977 |
3dcbcf7 to
e2afb57
Compare
|
Rebased onto current main ( |
|
Attestation is bound to head
Linked issue with reproduction: #4977 |
|
Speaking as a user's firstmate. We hit this bug today with two firstmate homes on one machine. Both homes hold their own clone of the same remote ( A plain Our local workaround is an untracked This PR fixes the problem at the source, and we would like to see it land. It currently conflicts with |
e2afb57 to
903fc30
Compare
|
Author note Fix for #4977 from a multi-home fleet that shared one treehouse pool by project name. Rebased to main at |
a1b9a2a to
23369f0
Compare
|
Re-raised through the no-mistakes gate and attested on
Ready for maintainer workflow approval / merge. If |
203a7d8 to
65e0425
Compare
mehulbhagwani
changed the title
65e0425 to
ee14b64
Compare
|
One data point from a two-home fleet that hit #5597 and ran a local fix for a few days, on where the pool lives rather than which pool is used. Our first fix put each non-root home's pool inside that home ( This PR's default |
ee14b64 to
d0731de
Compare
d0731de to
75625c9
Compare
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
75625c9 to
bc10c4b
Compare
Treehouse keys a worktree pool by the repository's resolved origin rather than by the clone, so every home holding its own clone of one repository allocated from a single shared pool. Homes competed for the same numbered slots, and a returned slot read free while its checkout was still a linked worktree of another home's clone; fm-spawn.sh's isolation assertion then refused it and the task stopped until a human released the slot from the home that owned it. Derive a per-home worktree root from the home's own resolved path and pass it to both acquisition sites, so two homes never see each other's slots. Returns stay flagless on purpose: treehouse resolves a return's pool from the worktree path it is handed, so every worktree already leased under the previously shared root stays returnable and nothing is migrated. Bootstrap now probes the global --root flag alongside the durable lease, and the CI treehouse pin moves to v2.3.0 because v2.0.1 has no such flag.
… treehouse Adding the global --root flag and the second capability probe changed two things the suites pin: the acquire command sent to a worker's shell, and what a fake treehouse must advertise to be accepted. Match the acquire around its root rather than pinning the bare command, so the assertions stay meaningful instead of passing because the old spelling disappeared, and teach the stubs that --root precedes the subcommand. The bootstrap suite gains the has-the-lease-but-no-root case beside its existing lease case, and a both-present case so neither probe can pass for the other's reason.
Treehouse keys a worktree pool by the repository's resolved origin rather than by the clone, so every home holding its own clone of one repository allocated from a single shared pool. Homes competed for the same numbered slots, and a returned slot read free while its checkout was still a linked worktree of another home's clone; fm-spawn.sh's isolation assertion then refused it and the task stopped until a human released the slot from the home that owned it. Derive a per-home worktree root from the home's own resolved path and pass it to both acquisition sites, so two homes never see each other's slots. Returns stay flagless on purpose: treehouse resolves a return's pool from the worktree path it is handed, so every worktree already leased under the previously shared root stays returnable and nothing is migrated. Bootstrap now probes the global --root flag alongside the durable lease, and the CI treehouse pin moves to v2.3.0 because v2.0.1 has no such flag.
…ends.md after dedup
…ide firstmate homes
…ocs already accurate
… fm_test_run_spawn fixture
…/fm-wake-lib.sh) was correct. In fm_treehouse_home_root, when TREEHOUSE_ROOT's leaf doesn't exist yet, the old code only stripped trailing slashes and never resolved the path via `pwd -P`. If an existing ancestor directory in that path was a symlink into the active Firstmate home or its root home, the derived root stayed unresolved (lexical), so the later prefix check against the resolved active_home/root_home never matched — even though `treehouse --root` would physically create the pool inside that home once the OS resolved the symlink on creation. This reintroduced the nested-home behavior the guard exists to prevent. Fix: when the base doesn't exist, walk up to the nearest existing ancestor, resolve that ancestor with `cd -P`, and reattach the non-existent suffix, so the derived path is physically resolved (modulo path segments that don't exist yet) before the prefix comparison runs. This applies uniformly to the single code path that builds `derived`, so there's no sibling site left unfixed. Added a regression test (`test_root_rejects_base_via_symlinked_ancestor_into_home` in tests/fm-treehouse-home-root.test.sh) that reproduces the exact scenario: a not-yet-created TREEHOUSE_ROOT reached through a symlinked ancestor into the active home. Verified it fails on the pre-fix code (git stash sanity check) and passes with the fix; the full existing test file (7 tests) still passes; shellcheck is clean around the change. The five "Behavior portable serial N" CI failures (ci-1..ci-5) remain unaddressed by code changes: their logs show only routine `actions/checkout` post-job cleanup with no actual test failure content, consistent with the prior two rounds' conclusion (partial local reruns of the relevant treehouse-root tests all passed). No code defect was found causing those; this is most likely a stale/pre-fix check run or CI infra issue rather than something in the current diff. That conclusion is unchanged by this round's work
…mehul/code/firstmate/data/fm-4932-land/ci-spawn-root.patch, which sets a default TREEHOUSE_ROOT (sibling to the test home) in five test fixtures so the new home-root symlink-resolution guard from round 3's fix doesn't reject the tests' own synthetic home paths. Only tests/*.test.sh files changed, no bin/ edits. Ran all five prescribed suites individually (the combined single invocation only exercised the first file, so each was run separately): fm-trace-context-spawn, fm-agy-harness, fm-rovo-harness, and fm-fleet-ledger all pass fully. fm-kimi-harness fails one test ('Kimi fallback did not expand HOME into an absolute executable'), but verified via a temporary stash of just that file that the same suite also fails (with a different symptom — 'verified kimi launch-then-send should succeed') on the pre-patch code, confirming this is a pre-existing environment-dependent flake tied to the real /opt/homebrew/bin/kimi binary on this machine, not caused by the patch or the underlying fm-wake-lib.sh fix. Working tree is clean apart from the five fixture edits
A branch protected only by review rules can report protected:true with required_status_checks:null. That is readable emptiness for classic checks, not an unreadable producer that must refuse the merge. Keep non-object non-null shapes as unreadable.
Only an explicit required_status_checks: null inside a readable protection object means a protected branch has no classic required checks. A protected branch payload with no protection object, or a protection object without the required_status_checks key, stays an unreadable source and refuses the merge. Restore the missing-protection refusal case and cover the missing-key case.
9338d4d to
3f698ba
Compare
mehulbhagwani
changed the title
…..' segment is now refused in fm_treehouse_home_root (error names the path and says to use a path without dot segments), before any containment check or derivation. An existing base keeps its pwd -P normalization. Added test_root_refuses_dot_segments_in_absent_base covering the '..' case, the './' case, and an ordinary absent base still deriving. tests/fm-treehouse-home-root.test.sh passes in full (10 tests), shellcheck reports only info-level notes. ci-1 (shellcheck OOM upstream) left untouched as instructed
|
The two red jobs here are known upstream flakes this PR does not touch: Behavior portable serial 4 is tests/fm-remote-delta-read.test.sh (#6404), and Lint 2 is shellcheck running out of memory on bin/fm-teardown.sh (#6410). Could you re-run those two failed jobs? I don't have rerun rights on this repo. |
|
Superseded by two slim replacements rebased onto current main:
Closing this bundled PR in favor of those two. |
Intent
Make every open mehulbhagwani PR on kunchenguid/firstmate clean for Kun to merge.
PRs: #5954 #5953 #5952 #5951 #5919 #5709 #4932
For each: checkout that PR head branch on origin/fork, rebase onto current upstream main, fix red CI (behavior portable serial jobs), ensure no-mistakes attestation head_sha MATCHES PR head, push to the PR branch (update existing PR in place — do NOT open new PRs), confirm CI green and mergeable CLEAN.
Priority: 5954 and 5952 already green—verify still clean; then 5953, 5951 red serial jobs; 5919 conflicts; 5709 attestation mismatch; 4932 two serial fails.
Do not merge on Kun's repo (no authority). Leave PRs ready. Report final status table.
Fix all 7 fast.
This run validates and updates existing PR #4932 in place (branch fm/pool-roots on the mehulbhagwani fork): each firstmate home derives its own Treehouse worktree pool root so homes cloning the same origin never share a pool, and a TREEHOUSE_ROOT that resolves inside a firstmate home (including through an existing ancestor that is a symlink into the active or root home) is rejected. bin/fm-pr-merge.sh: a protected branch whose readable protection object carries an explicit required_status_checks: null has no classic required checks (readable emptiness, the merge is not refused for that source), while a protected payload with no protection object or no required_status_checks key, or a non-object value, stays unreadable and refuses. The branch was rebased onto main, so the push must update the existing PR branch (force with lease) and the PR body attestation must bind to the new head. Do not open a new PR.
What Changed
TREEHOUSE_ROOTthat resolves inside a firstmate home is rejected, including when it reaches the home through an existing ancestor that is a symlink. The derivation lives inbin/fm-wake-lib.shand is used byfm-spawn.sh,fm-bootstrap.sh,fm-home-seed.sh,fm-install-treehouse.shandfm-test-run.sh. Docs and the bootstrap-diagnostics skill are updated to match.bin/fm-pr-merge.sh: a protected branch whose readable protection object has an explicitrequired_status_checks: nullis treated as having no classic required checks, so the merge is not refused on that basis. A protected payload with no protection object, norequired_status_checkskey, or a non-object value is still unreadable and still refuses.fm-treehouse-home-rootand live pool-isolation e2e suites, newfm-pr-mergecases, and fixture updates that set a defaultTREEHOUSE_ROOToutside the test home so existing suites pass the new guard.Risk Assessment
--rootflag and a hand-written path-normalization routine, and I did not run anything.Testing
Ran three targeted executable suites, which drive the real scripts and the real treehouse binary in disposable homes. All exited 0, including the symlinked-ancestor rejection and protected+null required-checks cases. No UI surface, so no visual artifacts.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-treehouse-home-root.test.shbash tests/fm-pr-merge.test.shbash tests/fm-treehouse-pool-isolation-live-e2e.test.sh✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.