fix: restore safe firstmate teardown behavior - #51
Merged
Conversation
Restores `main` to green on the `Lint shell scripts` and `Behavior tests`
checks. Both were failing for unrelated reasons.
Behavior tests: `validate_secondmate_project_clones` resolved the parent's
project clones from `$expected_source/projects` unless FM_PROJECTS_OVERRIDE
happened to be set, so `bin/fm-teardown.sh` was the only script that never
defined the canonical `PROJECTS="${FM_PROJECTS_OVERRIDE:-$FM_HOME/projects}"`
that fm-spawn, fm-bootstrap, fm-home-seed, fm-fleet-sync, fm-fleet-snapshot,
and fm-checkout-refresh all share. With FM_HOME pointing at a home outside the
repo - the documented multi-home layout of AGENTS.md section 2 - teardown looked
for the clones in the repo root, found nothing, and refused to retire any
secondmate. Resolve through the shared definition, mirroring how the registry
lookup beside it already resolves through $DATA. The refusal also printed an
empty path, because it reported the variable the failed assignment had just
cleared; report the candidate instead.
Lint: `id=minimum-age-cohort` and `legacy_id=minimum-age-legacy` are string
literals, but unquoted they parse as arithmetic, which is what SC2100 was
reporting. Quote them, matching the surrounding assignments.
The lifecycle fixture also had to stop tripping teardown's unlanded-work proof:
the fake no-mistakes marks a clone with `.no-mistakes-init`/`.no-mistakes-doctor`
touch files, while the real one leaves the clone with nothing to commit, so the
markers are dropped once the assertions have consumed them. Seeding proves a home
against its source repo's default-branch tip, so the suite additionally stood up
an isolated default-branch source repo; without it the lifecycle could only run
while the checkout itself sat on its default branch, never from a task branch.
Those two fixture changes are carried over from the unlanded work on
fm/fm-account-rescue-a2; the rest of that branch is left alone.
Note in CONTRIBUTING.md that CI runs the behavior loop under `set -eu` while the
documented local loop does not, so a red CI log is a lower bound on the failures
rather than the whole list.
CI's behavior loop is fail-fast, so it stopped at fm-secondmate-lifecycle-e2e
and had never reached these three suites. All three were already red on their
own, identically with and without the teardown fix in the previous commit, and
identically with treehouse/herdr hidden - so neither a regression nor a
missing-backend artifact, just failures the fail-fast log could not show.
Fixtures for these repairs already existed unlanded on fm/fm-account-rescue-a2;
they are carried here verbatim rather than rewritten, and the rest of that
branch (its capacity-rescue feature, watcher work, and CI changes) is left
alone. What they correct:
- sync: the T10/T11 worlds never registered `sm` in the secondmate registry, so
spawn refused before it could fast-forward the home, and T11 still asserted a
"sync skipped before launch" warning that the product replaced with a hard
refusal ("refusing secondmate launch because its home freshness is
unresolved"). The assertion now pins the behavior the product actually has.
- liveness: the enforced-recovery world used a bare directory where a real
default-branch repo is required, and stubbed node wholesale even though the
sweep genuinely needs it for fm-contained-read.cjs.
- safety: two tests re-derived the private checkout-lock hash by hand and got it
wrong - the product hashes the LOWERCASED path, so the hand-rolled key only
matched when mktemp happened to pick an all-lowercase suffix, making the lock
assertions pass or fail with the random temp name. They now ask the lock
library itself via fm_checkout_lock_path, which is the single owner of that
identity and cannot drift from it.
sync and liveness now pass. safety advances to a real remaining product defect
rather than the fixture noise that was masking it: when a Treehouse-acquired
home is the active firstmate home, fm-home-seed takes the home lifecycle lock
for that path and then waits the full FM_ACCOUNT_LIFECYCLE_LOCK_WAIT_SECONDS on
a lock it already holds, timing out instead of refusing cleanly with
"secondmate home cannot be the active firstmate home" as the test expects. That
lock-ordering fix is left to a dedicated change.
The secondmate home lifecycle locks are keyed on the home path and are not reentrant. Seed takes one for the active $FM_HOME up front, then a second for the destination home. The explicit-home branch proves the destination is legal with refuse_active_home_path BEFORE taking that second lock; the Treehouse-acquired branch never proved it at all. So an acquired home that IS the active firstmate home resolved to the lock seed already held, and the second acquisition could only wait out FM_ACCOUNT_LIFECYCLE_LOCK_WAIT_SECONDS against itself: error: timed out waiting for secondmate home lifecycle lock for home-<key> Prove the destination first in the acquired branch too, mirroring the explicit branch, so the caller gets the actionable refusal instead of a timeout: error: secondmate home cannot be the active firstmate home: <home> The check lands after retention is armed, so an acquired home is still retained for manual recovery rather than returned. tests/fm-secondmate-safety.test.sh already asserted this refusal; it had never been reached because the suite fail-fasts and an earlier fixture defect stopped it first.
Two more links in this suite's stale-fixture chain, both reached only after the
self-deadlock fix in the previous commit let execution get this far.
Empty-charter-fields: the two seed attempts shared one destination path. The
refresher records each covered checkout's physical identity, so recreating a home
at a path it already recorded reads as "covered checkout physical identity
drifted" and the second seed was refused for an unrefreshable default branch long
before it could reach the charter-field validation under test. Give the second
attempt its own destination. That test also left its identity records in the real
${XDG_STATE_HOME:-$HOME/.local/state}/firstmate/checkout-refresh, so it both wrote
through to the operator's own state and depended on what earlier runs left there;
scope FM_CHECKOUT_REFRESH_STATE_ROOT into the case.
Uninspectable projects: the home was a bare directory, but seed runs the
refresher's preflight on it before inspecting projects and that preflight requires
an exact Git repository root, so the home was refused as unrefreshable and never
reached the inspection failure. Use a real clone, as the sibling conversion case
already does.
Neither assertion changed. The suite still does not pass: with an unreadable
projects directory the preflight cannot read the tree and fails first, so the
specific "cannot inspect existing projects directory" message is unreachable
without reordering seed's --no-projects precondition checks ahead of the
refresher preflight. That is a product-ordering decision, not fixture staleness,
and it is left to a dedicated change rather than guessed at here.
Four links of this suite's stale-fixture chain. Tests passing in the suite go from 11 to 14; it still does not pass, and the next link is characterized in data/fm-main-green-g7/report.md rather than guessed at. Endpoint visibility (generation-lock cases, parent and child): both use a blocking `tmux kill-window` as the synchronization point that lets the test race a concurrent metadata update and prove teardown locked the generation first. The stub answered every other subcommand with silent success, so `list-windows` returned nothing, the endpoint read as absent, and teardown skipped endpoint cleanup entirely - `kill-window` was never called and the marker never appeared. A stub pinned to either state cannot work: always-absent skips the kill, always-present makes teardown refuse because it then requires confirmed absence. The stub is now observable until the kill lands and gone after it. kind=secondmate metadata: five fixtures set `project=` to a directory beside the home, but for a secondmate the product requires the project metadata to resolve to the registered home, and a real spawn records project == home (confirmed against the metadata a real secondmate spawn wrote). Normalized to the home. Unreachable assertion (zellij child endpoint): the test demanded stderr say the endpoint "is still alive", but fm_backend_agent_alive implements tmux and herdr only and answers `unknown` for every other backend, so no fixture could ever have produced that string for zellij. What the test exists to prove - that an endpoint whose state cannot be PROVEN still blocks destructive cleanup - is pinned by the refusal plus the retained lease, metadata, and worktree, all of which already pass. The assertion now names the message that path can actually emit, which is the stronger safety statement anyway. Nothing was skipped, suppressed, or relaxed; the one assertion change is argued above and its safety property remains fully asserted.
Partial: this repairs the home shape (a real firstmate clone with the full operational directory set and a registered project clone sharing its source's origin), which is what teardown proves first. The case still fails, now on a different and deeper point: its child worktree is created as a linked worktree of the home's own project clone, and validate_secondmate_repository_worktree_graph requires that clone to own exactly itself, so retirement refuses before reaching the child lock refusal the case pins. Resolving that needs the real-world shape of a child worktree relative to a secondmate's project clone worked out; not guessed here.
A secondmate holds its project clones under home/projects while its crewmates' worktrees live outside in the Treehouse pool as linked worktrees of those clones - the ordinary running state of any secondmate with live crewmates. Requiring the home's clone to own exactly itself refused that ordinary case. The graph check stays where it is, before any destruction, and now admits a linked worktree only when a REGISTERED child of that home vouches for it by recording both that exact worktree and that exact project, with both paths canonicalized before comparison. An unenumerable child state is an error, never an empty set. Two new regression tests pin the refusals that keep this from becoming a hole: an unregistered linked worktree, and one registered against a different project.
…l home shape Adds a shared make_secondmate_home_and_source / clone_registered_secondmate_project pair building the standard two-sided fixture - a real firstmate source, a plain-clone home with the full operational directory set, and a registered project clone existing on both sides sharing an origin - and converts the two operational-dir symlink cases onto it. Partial: both cases still fail, now on the open ordering question rather than on home shape. bin/fm-teardown.sh's cleanliness proof runs before its operational-dir proof, so a symlinked operational directory is reported as "unlanded changes: ?? state" instead of by the operational-dir proof that is the subject of both cases. That ordering is a product decision and is not guessed here.
validate_firstmate_home_for_removal proved the home's cleanliness before its structural shape, and a structural fault surfaces to `git status` as untracked content, so an operational directory symlinked out of the home was reported as "secondmate home has unlanded changes: ?? state". An operator debugging a refused teardown was told the wrong cause and would go looking for uncommitted work that does not exist. The operational-directory and project-clone proofs now run before the landed-state proof, and the operational-directory proof runs before the authority and lease proofs that also read the home's state directory. Every one of these is read-only and every one already ran before any destruction, so this changes only which refusal speaks first: each still returns non-zero on its own failure, so a home with both a structural fault and genuinely unlanded work is still refused. Adds test_secondmate_teardown_reports_structural_faults_before_cleanliness, whose negative assertion is the point - the cleanliness message must NOT be what surfaces for a containment violation.
Teardown is destructive and proves what it deletes by exact physical identity - canonical paths and device:inode checks - and a symlink is precisely what makes the logical path differ from the physical target that would be removed. secondmate_state_metadata already refused any symlinked state directory for that reason, while validate_operational_dirs permitted one resolving inside the home, so the product both permitted and forbade the same shape. The operational-dir proof now refuses it for all four directories, and names the symlink and its target so an operator is not left guessing which path is at fault. The inside-symlink case is retargeted to assert that refusal, because the decision changes what correct behaviour is rather than because the old assertion was inconvenient; it keeps its safety pins (home, symlink target, and metadata all retained, and no endpoint stopped before the home was proven). The outside-symlink case keeps its own containment message, which the proof-ordering change restored.
'contains registered secondmate home' exists nowhere in bin/, so no nested-home specific refusal is reachable. The product instead fails closed on a registry it cannot parse unambiguously, which this fixture's decoy 'mentions home:' line produces and which is the same safety outcome: nothing is removed. Only that one assertion changes; the safety pins it exists for - refusal, both homes retained, both metas retained, no endpoint stopped - are untouched. The fixture also gains the real home shape so the refusal is reached at all.
…rtion
Same unemittable grep as its sibling, but here the product has a message that states
this case's property exactly - "secondmate home still registers child homes" - so
that is the target rather than a fallback. The fixture also gains the real home shape
so the refusal is reached.
Partial: the case still fails on a LATER assertion, and it is a distinct finding.
require_empty_secondmate_registry runs at line ~4801, after endpoint quiescence, so
the endpoint is stopped before teardown discovers it will refuse - and the test
asserts no window is killed before this refusal. That is the same principle the
identity-drift case already pins ("endpoint was stopped before home identity was
proved") and the same shape as the proof-ordering decision: a read-only structural
proof that gates an irreversible step should run before it. Not changed here.
…e endpoint Stopping a supervisor is irreversible - it cannot be un-stopped and its in-flight context is gone - so a read-only proof that gates it must run first. A non-force retirement of a home that still registers child homes was always going to refuse, but quiesce_secondmate_endpoint ran before require_empty_secondmate_registry, so the operator lost a live secondmate to a teardown that never had a chance of succeeding. The proof now also runs before quiescence on the non-force path, and still runs at its original place, so nothing is skipped. Non-force only, deliberately: on the --force path cleanup_firstmate_home_children removes child registry entries before the later check, so that check legitimately observes post-cleanup state and hoisting it there would change what it observes rather than only when it runs.
…deriver The content-landed case hand-rolled its expected checkout-lock path by hashing the raw physical common-dir path. The product lowercases that path first (fm_checkout_stable_path_key ends in `print lc($path)`), so the two keys diverge whenever the mktemp directory contains an uppercase character - which mktemp templates essentially guarantee. The fake Treehouse then found no lock at the named path and failed the return with exit 91, so the case failed at the return step rather than on anything it was written to prove; every landed-work proof had already passed. Measured on a real repo: raw 1896fea0fa46324daaab18f9 vs product 68cbc9a67747a42792c3362a, and lowercasing alone accounts for the difference. Delegated to the suite's checkout_lock_path helper, which the three sibling lock cases already use and which resolves the common dir itself. The assertion that a normal teardown holds the common checkout lock across the Treehouse return is unchanged and still enforced.
… path Same drift as the content-fallback case, opposite symptom. This case plants a held lock to prove the landing proof refuses on contention, but derived the lock path by hashing the raw physical common-dir path, skipping the lowercasing in fm_checkout_stable_path_key. The lock landed where nothing looks for it, so teardown saw no contention and succeeded - the case failed as "expected exit 1, got 0", reporting the absence of a safety refusal that had simply never been triggered. Now derived through the suite's checkout_lock_path helper, so the contention is real and the refusal is genuinely exercised.
…l home shape The case proves a --force retirement refuses to remove a child worktree that lives inside the active firstmate home. Its home was built with bare mkdir, so home validation refused first with "secondmate project metadata is not an exact repository root" and the child worktree was never inspected - the case failed reporting a missing explanation for a rejection it never reached. Rebuilt on make_secondmate_home_and_source plus clone_registered_secondmate_project, matching the passing sibling cases, with the child worktree still pointed inside the active home so the property under test is unchanged. The asserted string is emitted by validate_removal_target (bin/fm-teardown.sh:2172) and is now actually reached.
add_git_status_lock_failure simulated a locked-index git status failure only when the third argument was exactly --porcelain, but every dirty check in fm-teardown.sh passes --porcelain=v1 --untracked-files=all. The stub had become a silent no-op, so the stale-lock cleanup path was never entered: teardown's first dirty check succeeded and it refused for uncommitted changes without ever clearing the lock or reporting it. Now matched in either spelling. The case's assertions are unchanged and it genuinely exercises clearing a provably-stale lock and re-running the dirty check afterwards. The helper has a single caller, so the wider match affects nothing else.
…id shapes Two cases that never got as far as the behaviour they name. The child-repo-descendant case built its home with bare mkdir, so home validation refused on shape before the child worktree inside FM_ROOT was ever inspected. Rebuilt on the real-clone helpers, hanging the child worktree off the fixture's own source root so it is still inside FM_ROOT. The source root already ships the quiet bin/fm-guard.sh the case hand-rolled and ignores data/, so the root stays clean. The non-linked index.lock case replaced its worktree with an independent clone, which teardown refuses outright: validate_teardown_target_identity requires the worktree and project to be distinct exact roots sharing one common Git dir, and a standalone clone shares none. That invariant is worth keeping, so the fixture changed instead - the worktree is now a clone's own primary checkout and the project a linked worktree of that same clone. Identity is satisfied honestly and `git rev-parse --git-path index.lock` still answers with the relative .git/index.lock spelling, which is the only spelling that exercises the worktree-anchored branch this case exists to pin.
… real repo Two stacked causes. The home was built with bare mkdir, so home validation refused on shape first; and once that was repaired the case still failed, because its child worktree was also a bare directory and validate_child_worktree_removal_target proves "is an exact Git root" before it proves "belongs to the child project". The case asserts the second message, so the first was speaking instead. The home now uses the real-clone helpers, and the child worktree is its own initialised repository - which is what an unregistered child worktree actually is. Its distinct common Git dir is what makes the belongs-to-project proof refuse, so the case exercises the rule it names.
…hermetic The secondmate-exemption case seeds a home cloned from this checkout, and fm-spawn's verify-home resolves the LIVE upstream default tip of FM_ROOT through probe_upstream's `git ls-remote --symref origin HEAD`. Unstubbed that escapes the fixture to the real remote, so the home reads as stale whenever the checkout's HEAD is ahead of origin/main - which is every unmerged branch, including any PR. Isolated rather than assumed: the suite passes 17/17 on unmodified main, and adding a single EMPTY commit on top of unmodified main reproduces the failure exactly, so it tracks the checkout's position and not its content. The fakebin now answers that one probe from this checkout's own default branch and execs real git for everything else. Verified 17/17 both on this branch and on main-plus-an-empty-commit, the previously failing shape. The product is unchanged; comparing a real home against the live upstream tip is correct behaviour.
…o not stub the guard The prior fix monkeypatched builder._require_safe_ancestry for the whole test class. For system_only it checked only S_ISDIR and the write bits, dropping the uid check that raises "ancestry is not owned by current uid or root" - so /usr, the exact path failing on the runner, passed inside the tests. The product file was untouched, but nothing exercised the ownership dimension of that rule any more, so a regression in it would ship silently. Green obtained that way is a false signal. The selector now enforces the builder's own validation, which carries the ancestry rule, plus the manifest's separate root-ownership requirement, and returns None when nothing qualifies. setUp raises SkipTest with the reason. Two alternatives were measured and rejected. A test-created fallback cannot work: load_manifest requires st_uid == 0 unconditionally for system tools and a test process cannot own a file as root - that yields "must be root-owned" instead. An ancestry-aware selector with no skip cannot work either, because every candidate resolves under the failing /usr and next() finds nothing. Verified: a normal host runs 55/55 with zero skips, so coverage is unchanged wherever the environment is sound; under a faithful runner shape the result is 0 failures, 0 errors, 55 skipped with the reason stated. A skip is visible and explains itself; a stub is invisible. The product ancestry check is untouched. The underlying cause is a GitHub runner-image change that affects main equally.
ruby-dlee
added a commit
that referenced
this pull request
Jul 31, 2026
* fix(teardown): resolve source projects from the firstmate home
Restores `main` to green on the `Lint shell scripts` and `Behavior tests`
checks. Both were failing for unrelated reasons.
Behavior tests: `validate_secondmate_project_clones` resolved the parent's
project clones from `$expected_source/projects` unless FM_PROJECTS_OVERRIDE
happened to be set, so `bin/fm-teardown.sh` was the only script that never
defined the canonical `PROJECTS="${FM_PROJECTS_OVERRIDE:-$FM_HOME/projects}"`
that fm-spawn, fm-bootstrap, fm-home-seed, fm-fleet-sync, fm-fleet-snapshot,
and fm-checkout-refresh all share. With FM_HOME pointing at a home outside the
repo - the documented multi-home layout of AGENTS.md section 2 - teardown looked
for the clones in the repo root, found nothing, and refused to retire any
secondmate. Resolve through the shared definition, mirroring how the registry
lookup beside it already resolves through $DATA. The refusal also printed an
empty path, because it reported the variable the failed assignment had just
cleared; report the candidate instead.
Lint: `id=minimum-age-cohort` and `legacy_id=minimum-age-legacy` are string
literals, but unquoted they parse as arithmetic, which is what SC2100 was
reporting. Quote them, matching the surrounding assignments.
The lifecycle fixture also had to stop tripping teardown's unlanded-work proof:
the fake no-mistakes marks a clone with `.no-mistakes-init`/`.no-mistakes-doctor`
touch files, while the real one leaves the clone with nothing to commit, so the
markers are dropped once the assertions have consumed them. Seeding proves a home
against its source repo's default-branch tip, so the suite additionally stood up
an isolated default-branch source repo; without it the lifecycle could only run
while the checkout itself sat on its default branch, never from a task branch.
Those two fixture changes are carried over from the unlanded work on
fm/fm-account-rescue-a2; the rest of that branch is left alone.
Note in CONTRIBUTING.md that CI runs the behavior loop under `set -eu` while the
documented local loop does not, so a red CI log is a lower bound on the failures
rather than the whole list.
* test(secondmate): carry unlanded fixture repairs for three suites
CI's behavior loop is fail-fast, so it stopped at fm-secondmate-lifecycle-e2e
and had never reached these three suites. All three were already red on their
own, identically with and without the teardown fix in the previous commit, and
identically with treehouse/herdr hidden - so neither a regression nor a
missing-backend artifact, just failures the fail-fast log could not show.
Fixtures for these repairs already existed unlanded on fm/fm-account-rescue-a2;
they are carried here verbatim rather than rewritten, and the rest of that
branch (its capacity-rescue feature, watcher work, and CI changes) is left
alone. What they correct:
- sync: the T10/T11 worlds never registered `sm` in the secondmate registry, so
spawn refused before it could fast-forward the home, and T11 still asserted a
"sync skipped before launch" warning that the product replaced with a hard
refusal ("refusing secondmate launch because its home freshness is
unresolved"). The assertion now pins the behavior the product actually has.
- liveness: the enforced-recovery world used a bare directory where a real
default-branch repo is required, and stubbed node wholesale even though the
sweep genuinely needs it for fm-contained-read.cjs.
- safety: two tests re-derived the private checkout-lock hash by hand and got it
wrong - the product hashes the LOWERCASED path, so the hand-rolled key only
matched when mktemp happened to pick an all-lowercase suffix, making the lock
assertions pass or fail with the random temp name. They now ask the lock
library itself via fm_checkout_lock_path, which is the single owner of that
identity and cannot drift from it.
sync and liveness now pass. safety advances to a real remaining product defect
rather than the fixture noise that was masking it: when a Treehouse-acquired
home is the active firstmate home, fm-home-seed takes the home lifecycle lock
for that path and then waits the full FM_ACCOUNT_LIFECYCLE_LOCK_WAIT_SECONDS on
a lock it already holds, timing out instead of refusing cleanly with
"secondmate home cannot be the active firstmate home" as the test expects. That
lock-ordering fix is left to a dedicated change.
* fix(home-seed): refuse an acquired active home instead of deadlocking
The secondmate home lifecycle locks are keyed on the home path and are not
reentrant. Seed takes one for the active $FM_HOME up front, then a second for the
destination home. The explicit-home branch proves the destination is legal with
refuse_active_home_path BEFORE taking that second lock; the Treehouse-acquired
branch never proved it at all.
So an acquired home that IS the active firstmate home resolved to the lock seed
already held, and the second acquisition could only wait out
FM_ACCOUNT_LIFECYCLE_LOCK_WAIT_SECONDS against itself:
error: timed out waiting for secondmate home lifecycle lock for home-<key>
Prove the destination first in the acquired branch too, mirroring the explicit
branch, so the caller gets the actionable refusal instead of a timeout:
error: secondmate home cannot be the active firstmate home: <home>
The check lands after retention is armed, so an acquired home is still retained
for manual recovery rather than returned.
tests/fm-secondmate-safety.test.sh already asserted this refusal; it had never
been reached because the suite fail-fasts and an earlier fixture defect stopped
it first.
* test(secondmate-safety): isolate two seed fixtures from refresher state
Two more links in this suite's stale-fixture chain, both reached only after the
self-deadlock fix in the previous commit let execution get this far.
Empty-charter-fields: the two seed attempts shared one destination path. The
refresher records each covered checkout's physical identity, so recreating a home
at a path it already recorded reads as "covered checkout physical identity
drifted" and the second seed was refused for an unrefreshable default branch long
before it could reach the charter-field validation under test. Give the second
attempt its own destination. That test also left its identity records in the real
${XDG_STATE_HOME:-$HOME/.local/state}/firstmate/checkout-refresh, so it both wrote
through to the operator's own state and depended on what earlier runs left there;
scope FM_CHECKOUT_REFRESH_STATE_ROOT into the case.
Uninspectable projects: the home was a bare directory, but seed runs the
refresher's preflight on it before inspecting projects and that preflight requires
an exact Git repository root, so the home was refused as unrefreshable and never
reached the inspection failure. Use a real clone, as the sibling conversion case
already does.
Neither assertion changed. The suite still does not pass: with an unreadable
projects directory the preflight cannot read the tree and fails first, so the
specific "cannot inspect existing projects directory" message is unreachable
without reordering seed's --no-projects precondition checks ahead of the
refresher preflight. That is a product-ordering decision, not fixture staleness,
and it is left to a dedicated change rather than guessed at here.
* test(teardown): repair stale endpoint and secondmate metadata fixtures
Four links of this suite's stale-fixture chain. Tests passing in the suite go
from 11 to 14; it still does not pass, and the next link is characterized in
data/fm-main-green-g7/report.md rather than guessed at.
Endpoint visibility (generation-lock cases, parent and child): both use a
blocking `tmux kill-window` as the synchronization point that lets the test race
a concurrent metadata update and prove teardown locked the generation first. The
stub answered every other subcommand with silent success, so `list-windows`
returned nothing, the endpoint read as absent, and teardown skipped endpoint
cleanup entirely - `kill-window` was never called and the marker never appeared.
A stub pinned to either state cannot work: always-absent skips the kill,
always-present makes teardown refuse because it then requires confirmed absence.
The stub is now observable until the kill lands and gone after it.
kind=secondmate metadata: five fixtures set `project=` to a directory beside the
home, but for a secondmate the product requires the project metadata to resolve to
the registered home, and a real spawn records project == home (confirmed against
the metadata a real secondmate spawn wrote). Normalized to the home.
Unreachable assertion (zellij child endpoint): the test demanded stderr say the
endpoint "is still alive", but fm_backend_agent_alive implements tmux and herdr
only and answers `unknown` for every other backend, so no fixture could ever have
produced that string for zellij. What the test exists to prove - that an endpoint
whose state cannot be PROVEN still blocks destructive cleanup - is pinned by the
refusal plus the retained lease, metadata, and worktree, all of which already
pass. The assertion now names the message that path can actually emit, which is
the stronger safety statement anyway.
Nothing was skipped, suppressed, or relaxed; the one assertion change is argued
above and its safety property remains fully asserted.
* test(teardown): answer parent and child tmux probes per target
* test(teardown): give forced-child cases a resolvable per-target endpoint
* test(teardown): align the forced-child parent meta with a real secondmate spawn
* fix(teardown): preserve forced child cleanup status instead of flattening to 1
* test(teardown): derive checkout lock paths with the product helper
* test(teardown): scope the broken ps stub to the return under test
* test(teardown): resolve the child endpoint in the lock-contention case
* test(teardown): give the herdr case a parseable session list
* test(teardown): classify the report-publication endpoint as present-idle
* test(teardown): classify post-quiescence endpoints as present-idle
* test(teardown): register the drifted secondmate home before the identity check
* fix(home-seed): honour --no-projects before the destination tree read
* test: classify idle fake tmux panes instead of leaving them unknown
* docs: record the tmux absence-proof contract for fixtures
* test(secondmate-safety): lay leased teardown homes out in a Treehouse pool
* test(secondmate-safety): give leased teardown homes real registered clones
* fix(teardown): lock the container when proving a nested surviving repository
* test(secondmate-safety): make the plain-clone teardown home a real clone
* test(secondmate-safety): make the unproven-lock home a real clone
Partial: this repairs the home shape (a real firstmate clone with the full
operational directory set and a registered project clone sharing its source's
origin), which is what teardown proves first. The case still fails, now on a
different and deeper point: its child worktree is created as a linked worktree of
the home's own project clone, and validate_secondmate_repository_worktree_graph
requires that clone to own exactly itself, so retirement refuses before reaching
the child lock refusal the case pins. Resolving that needs the real-world shape of
a child worktree relative to a secondmate's project clone worked out; not guessed
here.
* docs: record the https-origin requirement for local suite reproduction
* fix(teardown): admit only a registered child's linked project worktree
A secondmate holds its project clones under home/projects while its crewmates'
worktrees live outside in the Treehouse pool as linked worktrees of those clones -
the ordinary running state of any secondmate with live crewmates. Requiring the
home's clone to own exactly itself refused that ordinary case.
The graph check stays where it is, before any destruction, and now admits a linked
worktree only when a REGISTERED child of that home vouches for it by recording both
that exact worktree and that exact project, with both paths canonicalized before
comparison. An unenumerable child state is an error, never an empty set. Two new
regression tests pin the refusals that keep this from becoming a hole: an
unregistered linked worktree, and one registered against a different project.
* test(teardown): classify the retiring endpoint and use the registered clone
* test(secondmate-safety): give the symlink teardown fixtures their real home shape
Adds a shared make_secondmate_home_and_source /
clone_registered_secondmate_project pair building the standard two-sided fixture -
a real firstmate source, a plain-clone home with the full operational directory
set, and a registered project clone existing on both sides sharing an origin - and
converts the two operational-dir symlink cases onto it.
Partial: both cases still fail, now on the open ordering question rather than on
home shape. bin/fm-teardown.sh's cleanliness proof runs before its operational-dir
proof, so a symlinked operational directory is reported as "unlanded changes: ??
state" instead of by the operational-dir proof that is the subject of both cases.
That ordering is a product decision and is not guessed here.
* fix(teardown): report structural home faults by their own proof
validate_firstmate_home_for_removal proved the home's cleanliness before its
structural shape, and a structural fault surfaces to `git status` as untracked
content, so an operational directory symlinked out of the home was reported as
"secondmate home has unlanded changes: ?? state". An operator debugging a refused
teardown was told the wrong cause and would go looking for uncommitted work that
does not exist.
The operational-directory and project-clone proofs now run before the landed-state
proof, and the operational-directory proof runs before the authority and lease
proofs that also read the home's state directory. Every one of these is read-only
and every one already ran before any destruction, so this changes only which
refusal speaks first: each still returns non-zero on its own failure, so a home
with both a structural fault and genuinely unlanded work is still refused.
Adds test_secondmate_teardown_reports_structural_faults_before_cleanliness, whose
negative assertion is the point - the cleanliness message must NOT be what
surfaces for a containment violation.
* fix(teardown): refuse a symlinked operational directory outright
Teardown is destructive and proves what it deletes by exact physical identity -
canonical paths and device:inode checks - and a symlink is precisely what makes the
logical path differ from the physical target that would be removed.
secondmate_state_metadata already refused any symlinked state directory for that
reason, while validate_operational_dirs permitted one resolving inside the home, so
the product both permitted and forbade the same shape. The operational-dir proof now
refuses it for all four directories, and names the symlink and its target so an
operator is not left guessing which path is at fault.
The inside-symlink case is retargeted to assert that refusal, because the decision
changes what correct behaviour is rather than because the old assertion was
inconvenient; it keeps its safety pins (home, symlink target, and metadata all
retained, and no endpoint stopped before the home was proven). The outside-symlink
case keeps its own containment message, which the proof-ordering change restored.
* test(secondmate-safety): retarget the unemittable nested-home assertion
'contains registered secondmate home' exists nowhere in bin/, so no nested-home
specific refusal is reachable. The product instead fails closed on a registry it
cannot parse unambiguously, which this fixture's decoy 'mentions home:' line
produces and which is the same safety outcome: nothing is removed. Only that one
assertion changes; the safety pins it exists for - refusal, both homes retained,
both metas retained, no endpoint stopped - are untouched. The fixture also gains the
real home shape so the refusal is reached at all.
* test(secondmate-safety): retarget the child-registry nested-home assertion
Same unemittable grep as its sibling, but here the product has a message that states
this case's property exactly - "secondmate home still registers child homes" - so
that is the target rather than a fallback. The fixture also gains the real home shape
so the refusal is reached.
Partial: the case still fails on a LATER assertion, and it is a distinct finding.
require_empty_secondmate_registry runs at line ~4801, after endpoint quiescence, so
the endpoint is stopped before teardown discovers it will refuse - and the test
asserts no window is killed before this refusal. That is the same principle the
identity-drift case already pins ("endpoint was stopped before home identity was
proved") and the same shape as the proof-ordering decision: a read-only structural
proof that gates an irreversible step should run before it. Not changed here.
* fix(teardown): prove a childless registry before stopping a secondmate endpoint
Stopping a supervisor is irreversible - it cannot be un-stopped and its in-flight
context is gone - so a read-only proof that gates it must run first. A non-force
retirement of a home that still registers child homes was always going to refuse, but
quiesce_secondmate_endpoint ran before require_empty_secondmate_registry, so the
operator lost a live secondmate to a teardown that never had a chance of succeeding.
The proof now also runs before quiescence on the non-force path, and still runs at its
original place, so nothing is skipped. Non-force only, deliberately: on the --force
path cleanup_firstmate_home_children removes child registry entries before the later
check, so that check legitimately observes post-cleanup state and hoisting it there
would change what it observes rather than only when it runs.
* test(teardown): name the content-fallback lock through the product's deriver
The content-landed case hand-rolled its expected checkout-lock path by hashing
the raw physical common-dir path. The product lowercases that path first
(fm_checkout_stable_path_key ends in `print lc($path)`), so the two keys diverge
whenever the mktemp directory contains an uppercase character - which mktemp
templates essentially guarantee. The fake Treehouse then found no lock at the
named path and failed the return with exit 91, so the case failed at the return
step rather than on anything it was written to prove; every landed-work proof had
already passed.
Measured on a real repo: raw 1896fea0fa46324daaab18f9 vs product
68cbc9a67747a42792c3362a, and lowercasing alone accounts for the difference.
Delegated to the suite's checkout_lock_path helper, which the three sibling lock
cases already use and which resolves the common dir itself. The assertion that a
normal teardown holds the common checkout lock across the Treehouse return is
unchanged and still enforced.
* test(teardown): contend the shared checkout lock at the product's own path
Same drift as the content-fallback case, opposite symptom. This case plants a
held lock to prove the landing proof refuses on contention, but derived the lock
path by hashing the raw physical common-dir path, skipping the lowercasing in
fm_checkout_stable_path_key. The lock landed where nothing looks for it, so
teardown saw no contention and succeeded - the case failed as "expected exit 1,
got 0", reporting the absence of a safety refusal that had simply never been
triggered.
Now derived through the suite's checkout_lock_path helper, so the contention is
real and the refusal is genuinely exercised.
* test(secondmate-safety): give the active-home-descendant case its real home shape
The case proves a --force retirement refuses to remove a child worktree that
lives inside the active firstmate home. Its home was built with bare mkdir, so
home validation refused first with "secondmate project metadata is not an exact
repository root" and the child worktree was never inspected - the case failed
reporting a missing explanation for a rejection it never reached.
Rebuilt on make_secondmate_home_and_source plus
clone_registered_secondmate_project, matching the passing sibling cases, with the
child worktree still pointed inside the active home so the property under test is
unchanged. The asserted string is emitted by validate_removal_target
(bin/fm-teardown.sh:2172) and is now actually reached.
* test(teardown): match both porcelain spellings in the status-lock stub
add_git_status_lock_failure simulated a locked-index git status failure only when
the third argument was exactly --porcelain, but every dirty check in
fm-teardown.sh passes --porcelain=v1 --untracked-files=all. The stub had become a
silent no-op, so the stale-lock cleanup path was never entered: teardown's first
dirty check succeeded and it refused for uncommitted changes without ever
clearing the lock or reporting it.
Now matched in either spelling. The case's assertions are unchanged and it
genuinely exercises clearing a provably-stale lock and re-running the dirty check
afterwards. The helper has a single caller, so the wider match affects nothing
else.
* test: reach the repo-descendant and non-linked-lock cases through valid shapes
Two cases that never got as far as the behaviour they name.
The child-repo-descendant case built its home with bare mkdir, so home validation
refused on shape before the child worktree inside FM_ROOT was ever inspected.
Rebuilt on the real-clone helpers, hanging the child worktree off the fixture's
own source root so it is still inside FM_ROOT. The source root already ships the
quiet bin/fm-guard.sh the case hand-rolled and ignores data/, so the root stays
clean.
The non-linked index.lock case replaced its worktree with an independent clone,
which teardown refuses outright: validate_teardown_target_identity requires the
worktree and project to be distinct exact roots sharing one common Git dir, and a
standalone clone shares none. That invariant is worth keeping, so the fixture
changed instead - the worktree is now a clone's own primary checkout and the
project a linked worktree of that same clone. Identity is satisfied honestly and
`git rev-parse --git-path index.lock` still answers with the relative
.git/index.lock spelling, which is the only spelling that exercises the
worktree-anchored branch this case exists to pin.
* test(secondmate-safety): reach the unregistered-child proof through a real repo
Two stacked causes. The home was built with bare mkdir, so home validation
refused on shape first; and once that was repaired the case still failed, because
its child worktree was also a bare directory and
validate_child_worktree_removal_target proves "is an exact Git root" before it
proves "belongs to the child project". The case asserts the second message, so
the first was speaking instead.
The home now uses the real-clone helpers, and the child worktree is its own
initialised repository - which is what an unregistered child worktree actually
is. Its distinct common Git dir is what makes the belongs-to-project proof refuse,
so the case exercises the rule it names.
* test(spawn-dispatch-profile): make the seeded home's freshness proof hermetic
The secondmate-exemption case seeds a home cloned from this checkout, and
fm-spawn's verify-home resolves the LIVE upstream default tip of FM_ROOT through
probe_upstream's `git ls-remote --symref origin HEAD`. Unstubbed that escapes the
fixture to the real remote, so the home reads as stale whenever the checkout's
HEAD is ahead of origin/main - which is every unmerged branch, including any PR.
Isolated rather than assumed: the suite passes 17/17 on unmodified main, and
adding a single EMPTY commit on top of unmodified main reproduces the failure
exactly, so it tracks the checkout's position and not its content.
The fakebin now answers that one probe from this checkout's own default branch
and execs real git for everything else. Verified 17/17 both on this branch and on
main-plus-an-empty-commit, the previously failing shape. The product is unchanged;
comparing a real home against the live upstream tip is correct behaviour.
* no-mistakes(review): Make behavior-test guidance Herdr-safe
* no-mistakes(document): Document teardown safety ordering
* no-mistakes: apply CI fixes
* no-mistakes: apply CI fixes
* no-mistakes: apply CI fixes
* no-mistakes: apply CI fixes
* test(sealed-bridge): skip when no builder-valid system tool exists, do not stub the guard
The prior fix monkeypatched builder._require_safe_ancestry for the whole test
class. For system_only it checked only S_ISDIR and the write bits, dropping the
uid check that raises "ancestry is not owned by current uid or root" - so /usr,
the exact path failing on the runner, passed inside the tests. The product file
was untouched, but nothing exercised the ownership dimension of that rule any
more, so a regression in it would ship silently. Green obtained that way is a
false signal.
The selector now enforces the builder's own validation, which carries the
ancestry rule, plus the manifest's separate root-ownership requirement, and
returns None when nothing qualifies. setUp raises SkipTest with the reason.
Two alternatives were measured and rejected. A test-created fallback cannot
work: load_manifest requires st_uid == 0 unconditionally for system tools and a
test process cannot own a file as root - that yields "must be root-owned"
instead. An ancestry-aware selector with no skip cannot work either, because
every candidate resolves under the failing /usr and next() finds nothing.
Verified: a normal host runs 55/55 with zero skips, so coverage is unchanged
wherever the environment is sound; under a faithful runner shape the result is
0 failures, 0 errors, 55 skipped with the reason stated. A skip is visible and
explains itself; a stub is invisible. The product ancestry check is untouched.
The underlying cause is a GitHub runner-image change that affects main equally.
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.
What this does
Repairs the red behaviour suites that were keeping
mainuntrustworthy, continuing the existing red-main chain branch so a single PR lands the whole chain.tests/fm-teardown.test.shtests/fm-secondmate-safety.test.shtests/fm-spawn-dispatch-profile.test.shbin/fm-lint.shCounts are from full-suite runs on a clean committed proxy (a fresh clone with
mainreset to the branch head, clean tree, httpsorigin).This PR does NOT represent a full-suite pass on CI
Stated plainly so the summary is not misread. The pipeline's
teststep was skipped deliberately, so the 80-suite behaviour loop was never run as a whole by the gate. Four behaviour suites remain red and were not run at all:fm-afk-launch,fm-backend-autodetect-smoke,fm-backend-herdr-workspace-per-home-e2e), deferred to a crewmate briefed with the Herdr isolation contract. They drive Herdr lifecycle against the default workspace, which on a developer machine hosts the live agent fleet.fm-account-routing, which fails on unmodifiedmainand is not a defect this branch introduced.What CI does cover is listed under Validation, and it passes.
On the sealed-bridge fixture change
tests/test_build_sealed_bridge_runtimes.pyis the one file here unrelated to the teardown work. It is carried only because it was blocking this branch.mainequally — an ancestry rule failing on/usr. At the head where it first appeared, this branch touched neither that test nortools/bridge-cutover/build_sealed_bridge_runtimes.py; both were byte-identical tomain.tools/bridge-cutover/is untouched by this branch. An earlier automated attempt made CI green by monkeypatching that guard so its ownership check stopped running; that was removed, not kept.macOS-onlyreasons unrelated to this change.SkipTestpath exists for a host where no root-owned system tool with safe ancestry exists at all. It did not trigger on this runner. Where it does trigger it states its reason, so the gap is visible rather than silent.Notable changes
Product —
bin/fm-teardown.sh. Retiring a secondmate home that still registers child homes always refused, but the proof only ran at the removal boundary, ~280 lines afterquiesce_secondmate_endpointhad already killed the home's supervisor: the operator was told the home was preserved while its agent was gone. The read-only registry proof now runs before that irreversible step on the non-force path. The original late check stays for both paths, so nothing became skippable. Non-force only is deliberate — on--force,cleanup_firstmate_home_childrenlegitimately empties those entries first, so the late check observes post-cleanup state there.Product —
bin/fm-home-seed.sh.--no-projectspreconditions run before the refresher preflight, so an unusableprojects/reports its specific, actionable reason instead of a generic refusal.Fixtures. Each failure was root-caused individually rather than pattern-matched. Two teardown cases hand-rolled their expected checkout-lock path by hashing the raw common-dir path while the product lowercases it first, so under a mixed-case
mktempdirectory they named a file that never existed — one failedexpected 0, got 1, the otherexpected 1, got 0, from a single cause. Agit statusstub keyed on a bare--porcelainhad silently become a no-op once the product moved to--porcelain=v1. Several secondmate fixtures were baremkdirhomes refused on shape long before the behaviour they name.Where an assertion demanded something the product structurally forbids, the scenario was retargeted and the safety property kept pinned — never the assertion weakened. The non-linked
index.lockcase is the clearest example: it built a task worktree as a standalone clone, whichvalidate_teardown_target_identityrefuses by design, so the fixture changed rather than the invariant.Validation
bin/fm-lint.shexit 0 at the final head. Both target suites verified green end to end on the clean committed proxy. The sealed-bridge fixture change was measured on both a normal host and a faithful simulation of the failing runner shape.Pipeline
Updates from git push no-mistakes
Run
01KYSQP663V7SB5Z77WD5JS9YC. Steps: intent, rebase, review, test, document, lint, push, pr, ci.The test step was skipped deliberately, on operator instruction, and that is the one thing to read carefully here. It runs
for t in tests/*.test.shover all 80 suites with no exclusion, and three of them drive Herdr lifecycle against the default Herdr workspace. On a developer machine that workspace hosts the live crewmate fleet, so running it risks destroying other agents' unlanded work; CI is safe only because its runner is disposable. A first attempt was stopped at suite 2 of 80 before reaching them. The compensating evidence is the per-suite verification listed above, run against a clean committed proxy.