fix(bin): arm the teardown evidence gate only where a destination is configured - #4
Merged
Merged
Conversation
…configured Cleanup called bin/fm-evidence.sh unconditionally and treated every non-zero result as unpreserved evidence. The write-through is opt-in and off by default, so that made an inert feature a hard precondition of every teardown: any failure to RUN the helper - not a failure to preserve anything - refused cleanup even in a home that never opted in. Gate the block on config/evidence-repo so an opted-out home is inert rather than merely quiet, which is what bin/fm-evidence.sh's own contract already promises. The [ -f ] test is deliberately weaker than the helper's enablement predicate, which additionally requires a non-empty value, so every file the helper might accept still reaches it and the helper alone judges a configured destination. Refusal behavior where the feature IS on is unchanged: an unusable destination still stops cleanup, an unreachable remote still does not, and --force still warns and proceeds.
The regression that reached main was an over-armed gate: an opted-out home inherited a hard dependency on bin/fm-evidence.sh. No test caught it, because every case ran against a complete bin/ where the helper is always present, so "armed but harmless" and "not armed" were indistinguishable. Add the one condition that separates them - a bin/ missing only that helper - and assert both directions through it: an opted-out home still cleans up, and an opted-in home refuses because unknown custody is not absent custody. Each case is proven by the mutation that breaks exactly it: dropping the config/evidence-repo condition fails only the opted-out case, and letting a non-runnable helper pass the armed gate fails only the opted-in one.
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.
Intent
Repair the two teardown tests that are failing on the fork's default branch (tests/fm-gotmp.test.sh 'teardown exited non-zero with a valid tasktmp' and tests/fm-backend.test.sh 'old fm-teardown.sh (scout, report present) should succeed'), so the branch can go green and unblock fork PR #3, which fails only because it inherits this breakage through the shared suite.
Root cause, established by running and bisecting rather than reading: fork PR #2 (the checkpoint-commit discipline and opt-in evidence write-through, which merged with zero CI checks because the fork was diverged) wired bin/fm-evidence.sh into bin/fm-teardown.sh with an UNCONDITIONAL call whose every non-zero result was read as unpreserved evidence. That turned an opt-in, default-off feature into a hard precondition of every cleanup: any failure to RUN the helper - not a failure to preserve anything - refused cleanup, including in a home that never opted in. Both tests build a synthetic bin/ that predates the new sibling and so hit exactly that path.
Deliberate decisions the captain set, which a reviewer seeing only the diff would not know:
Fix the script, not the tests. The two failing assertions encode a contract teardown has always had and passed one commit earlier. Editing an assertion to turn a red suite green was explicitly the one thing this task must not produce. Neither failing assertion was touched.
Keep PR feat(bin): add checkpoint-commit discipline to briefs and an opt-in evidence write-through #2's evidence write-through. This is a regression fix, not a revert. Behavior where the feature IS enabled is unchanged: an unusable destination still refuses cleanup, an unreachable remote still does not block it, and --force still warns and proceeds.
The chosen fix arms the gate on the opt-in itself: 'if [ "$KIND" != secondmate ] && [ -f "$CONFIG/evidence-repo" ]'. This restores what both docs/configuration.md and bin/fm-evidence.sh's own header ALREADY promised - that with the config file absent, teardown behaves exactly as it does without the feature. The documented contract was correct; the wiring violated it. That is why no documentation change accompanies this fix.
The [ -f ] test is deliberately weaker than fm-evidence.sh's own enablement predicate, which additionally requires a non-empty value. That asymmetry is intentional and is explained in the code comment: any file the helper might accept still reaches it, so the helper alone judges whether a configured destination is usable, and teardown never decides that for it. Do not 'tidy' it into a content check.
Teardown's refusal path protects unlanded work, so it is guard-class code under the fleet mutation standard: a protection that is touched or added must be proven by the mutation that breaks exactly its own test. The prior test suite could not tell 'armed but harmless' from 'not armed', because every case ran against a complete bin/ where the helper is always present. The two added cases supply the one condition that separates them - a bin/ missing only that helper - and pin both directions. Each was verified against its own mutation: dropping the config/evidence-repo condition fails only the opted-out case, and adding [ -x ] so a non-runnable helper passes the armed gate fails only the opted-in case.
The added test fixture mirrors bin/ by symlinking the directory rather than listing sibling names, specifically so a future sibling cannot silently weaken it - the same failure mode that produced this regression.
Verified before validation: the two target suites plus tests/fm-teardown-evidence.test.sh and tests/fm-evidence.test.sh all pass; the portable-serial CI lane went from failed=2 to failed=1; bin/fm-lint.sh, bin/fm-test-run.sh --check-coverage, and bin/fm-doc-audience-check.sh are clean. The one remaining local failure is tests/fm-backend-herdr-focus-flash-e2e.test.sh, a real-Herdr live test that CI gate-skips (it accounts for the skipped_gate difference of 12 in CI versus 11 locally). It is unrelated to teardown and out of scope here.
Note for the PR description: the reason this reached the default branch at all - a diverged fork meant PR #2's checks never ran - is being prevented separately by a task filed as fm-fork-freshness-sweep. This task repairs the damage; that one stops the next. The PR description should mention the pairing so a later reader sees both halves rather than assuming this fix alone closed the hole.
What Changed
bin/fm-teardown.shnow runs the evidence write-through only whenconfig/evidence-repoexists ([ "$KIND" != secondmate ] && [ -f "$CONFIG/evidence-repo" ]), instead of callingbin/fm-evidence.shunconditionally and reading every non-zero result as unpreserved evidence. A home that never opted in is inert here again rather than acquiring a hard dependency on the helper, which is what madetests/fm-gotmp.test.shandtests/fm-backend.test.shfail on the default branch against their syntheticbin/directories. Behavior where the feature is enabled is unchanged: an unusable destination still refuses cleanup, an unreachable remote still does not block it, and--forcestill warns and proceeds. The[ -f ]test is deliberately weaker than the helper's own non-empty-value predicate so the helper alone judges whether a configured destination is usable; the code comment records why.tests/fm-teardown-evidence.test.shgains two cases that pin the arming from both sides using the one condition the prior suite could not express - abin/complete except forfm-evidence.sh: an opted-out home cleans up without the helper, and an opted-in home refuses when the helper cannot run. The fixture mirrorsbin/by symlinking each entry rather than listing sibling names, so a future sibling cannot silently weaken it. Each case was verified against the mutation that breaks exactly it: dropping the config condition fails only the opted-out case, and adding[ -x ]to the gate fails only the opted-in case.docs/configuration.mdandbin/fm-evidence.sh's header already promised this behavior - the wiring, not the contract, was wrong.The reason this reached the fork's default branch at all is that a diverged fork meant PR #2's checks never ran. This PR repairs the damage; the missing-checks hole itself is being closed separately under
fm-fork-freshness-sweep.Risk Assessment
✅ Low: A two-line, well-bounded regression fix that restores an already-documented opt-in contract, leaves all enabled-feature behavior and both failing assertions untouched, and adds mutation-separated coverage for both directions of the new arming condition.
Testing
Reproduced the red baseline by checking out the base commit and running the two named suites (both target assertions fail with the evidence REFUSED message), then confirmed both go green at the target commit, along with tests/fm-teardown-evidence.test.sh, tests/fm-evidence.test.sh, the runner's own --changed selection for this diff, and tests/fm-teardown-endpoint-safety.test.sh. Beyond the suites I drove bin/fm-teardown.sh directly against real fixture homes to capture an operator CLI transcript of the actual symptom and its repair, and of the opt-in paths (preserve-then-clean, refuse on an unusable destination, --force warn-and-proceed) staying unchanged. I also independently re-ran both guard mutations described in the intent and each one kills exactly its own new test case. No source or test files were left modified; the temporary script swap and mutations were restored and hash-verified. This change is CLI/script-level with no rendered UI surface, so the reviewer-visible evidence is a terminal transcript rather than a screenshot.
Evidence: Operator CLI transcript: regression and repair, plus opt-in paths unchanged
Evidence: Before/after excerpt (opted-out home, bin/ without fm-evidence.sh)
Evidence: Baseline red at base commit a5044fd (both named assertions)
Evidence: Both suites green at target commit 40c60ca
Evidence: Guard-class mutation proof: each mutation kills exactly its own case
### MUTATION: A - drop the config/evidence-repo condition (the regression) not ok - an opted-out home was blocked by the evidence gate ### MUTATION: B - add [ -x ] so a non-runnable helper passes the armed gate ok - fm-teardown: an opted-out home cleans up even when the evidence helper is absent not ok - cleanup proceeded with a configured destination it could not write toEvidence: Reproducible manual demo script
/tmp/no-mistakes-evidence/01KYX2VJD3RRQC5FZQQCBCRHSF/changed-selection.log)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
git checkout a5044fd && bin/fm-test-run.sh tests/fm-gotmp.test.sh tests/fm-backend.test.sh- baseline red, reproduces both named assertions failing, then restored HEAD to 40c60cabin/fm-test-run.sh tests/fm-gotmp.test.sh tests/fm-backend.test.shat the target commit - both suites greenbin/fm-test-run.sh tests/fm-teardown-evidence.test.sh tests/fm-evidence.test.sh- all 6 + 20 cases pass, including the two added arming casesbin/fm-test-run.sh --changed --base a5044fd- the runner's own changed-file selection (fm-pr-check-security, fm-pr-merge, fm-review-diff, fm-teardown, fm-x-mode, fm-teardown-evidence), 6/6 passbin/fm-test-run.sh tests/fm-teardown-endpoint-safety.test.sh- passesManual operator transcript: built real Firstmate homes past the completion gate and invokedbin/fm-teardown.sh <id>directly for six scenarios (opted-out home without the helper before/after the fix, opted-in home without the helper, opted-in home with a working evidence repo, opted-in home with an unusable destination, and the same with--force)Mutation proof A: replaced the gate withif [ "$KIND" != secondmate ]; thenand rantests/fm-teardown-evidence.test.sh- fails onlyan opted-out home was blocked by the evidence gateMutation proof B: added&& [ -x "$SCRIPT_DIR/fm-evidence.sh" ]to the gate and rantests/fm-teardown-evidence.test.sh- opted-out case passes, fails onlycleanup proceeded with a configured destination it could not write togit status --porcelainandgit hash-object bin/fm-teardown.shafter each temporary swap/mutation - worktree restored clean at 40c60ca✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.