feat(merge): gate unexecuted test findings per project and add batch supersessions - #6
Merged
Merged
Conversation
…essions Rework fm-pr-merge.sh's finding consumption to parse all three classes the test-keep gate reports - missing, failing, unexecuted - and drive the refuse/proceed decision off the parsed-and-policy-applied counts rather than off any single-class path or the detector's exit code alone. The previous handler only parsed missing/failing, so an unexecuted finding riding alongside a supersession-excused missing/failing one was silently ignored and the merge proceeded; that is closed, with a regression test. Unexecuted findings block only for a project the captain has enabled with a data/exec-gate/<project> marker, read at merge time rather than from task meta so enabling a project takes effect for already-in-flight tasks. Absent means warn-only, which is the behavior every project had before the class existed, so the class ships inert until a project is explicitly enabled. Extend the supersession grammar back-compatibly with optional ids: (a glob, the batch mechanism) and kind: (missing, failing, unexecuted, or any). An absent kind means any, so every pre-existing entry behaves exactly as before. kind is a safety field: a dependency-bump batch written `ids: * | kind: unexecuted` cannot silently excuse a deleted or rewritten assertion, and the header warns that a kindless `ids: *` disables the gate for that project. Parsing also fails closed on both id and ids, an unrecognized or unparseable field, an invalid kind, and any field written after reason, since a swallowed kind would widen an entry to any. The script header stays the one owner of the entry grammar, the marker contract, and the decision rule; AGENTS.md and docs/architecture.md carry cross-references only.
…erge.sh decision owner
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
Make firstmate's prior-tests merge gate (bin/fm-pr-merge.sh) consume the detector's new 'unexecuted:' finding class under a per-project enablement marker, and extend the captain-approved supersession record with a back-compatible batch form, so a test file that was never actually executed can block a merge per project instead of reading as a silent pass. This is PR4 of a five-PR plan whose spec is data/kept-exec-plan-w2/report.md sections 5 and 7; PR2 (the pytest execution runner) lands AFTER this and is currently blocked on it, because with 'unexecuted:' exiting non-zero and nothing yet reading a marker the gate would refuse every firstmate merge. Deliberate decisions a reviewer reading only the diff would not know: (1) The captain's binding verdict governs the design - a green result that means 'nothing ran' is worse than no result because it reads as verified. Exiting clean when only unverified findings exist was explicitly considered and REJECTED; do not propose reverting to warn-only-everywhere or to a clean exit. (2) The enablement marker is $FM_HOME/data/exec-gate/, read at MERGE time rather than from the task's meta, deliberately so that enabling a project takes effect for already-in-flight tasks. The plan records and rejects the alternative (a '+exec-gate' posture flag in data/projects.md recorded into meta at spawn) because it binds enablement to spawn time and fm-pr-merge does not read the registry. It mirrors the ownership model of data/supersessions/.md: gitignored data/, lazily created, captain-controlled, never auto-generated. Default absent for every project, so the class ships inert. (3) data/exec-gate/firstmate is deliberately NOT created here; enabling firstmate is PR5's job, separated so the plumbing is proven inert before it blocks anything. (4) The refuse/proceed decision is driven off parsed-and-policy-applied counts for all three classes rather than the detector's exit code or any single-class path. This closes a leak the review gate found on PR2: the old handler parsed only missing:/failing:, so an unexecuted: finding riding alongside a supersession-EXCUSED missing/failing one was silently ignored and the merge proceeded (excused=1, unexcused=0, neither refusal condition fired). That has its own regression test. The old 'excused==0' defensive refusal was replaced by an explicit 'exit 1 but no parseable finding line' refusal, because the old form would have wrongly blocked the legitimate all-unexecuted-and-not-gated case. rc==2 stays its own separate unverifiable refusal, unchanged. (5) The supersession grammar gains two OPTIONAL fields: 'ids:' (a glob matched with bash pattern matching, the batch mechanism so one entry covers many files from a single cause such as a dependency bump) and 'kind:' (missing|failing|unexecuted|any) constraining which class an entry excuses. An ABSENT kind means 'any' - that is exactly what preserves back-compat for every existing entry, and it is intentional, not an oversight. kind: is a safety feature, not a convenience: 'ids: * | kind: unexecuted' can never silently excuse a genuinely deleted (missing) or rewritten (failing) assertion, while a batch entry without kind is a loaded gun. The adversarial test proving that 'ids: *' with no kind excuses EVERY class is deliberate - it proves the documented back-compat behavior rather than quietly narrowing it - and the header warns the captain about exactly that. (6) Parsing additionally fails closed on cases the plan did not enumerate: both id: and ids: present, an unparseable or unrecognized field name, an invalid kind value, and any recognized field written AFTER reason:. That last one is mine, not the plan's: reason must be last because it is the only field allowed to contain ' | ', so a kind: written after reason would be swallowed into the reason text and silently widen the entry to 'any', which is the exact loaded-gun case. Refusing beats parsing loosely there. (7) Per firstmate's one-owner rule, bin/fm-pr-merge.sh's header is the single owner of the supersession entry grammar, the marker contract, and the decision rule; AGENTS.md (one layout line plus one sentence) and docs/architecture.md carry cross-references only, never a second copy. (8) Tests are driven off constructed fm-assert-tests-kept.sh stdout via a shim bin/ holding symlinks to the real fm-pr-merge.sh and fm-pr-check.sh, because the unexecuted: class does not exist in the detector yet (PR2 adds it) and because one case can then mix classes no single fixture could produce together. Verification already run locally: 30/30 in tests/fm-pr-merge.test.sh, bin/fm-lint.sh clean (exit 0, zero findings, pinned ShellCheck 0.11.0), full suite 70/72. The two failures are pre-existing and environmental, NOT from this branch: tests/fm-session-start.test.sh (known local artifact) and tests/fm-afk-launch.test.sh, which I verified fails with the identical assertion at base commit f7b7b7a from a clean archive. Do not attribute either to this change.
What Changed
bin/fm-pr-merge.shnow consumes a thirdunexecuted:finding class fromfm-assert-tests-kept.shand decides refuse/proceed from parsed-and-policy-applied counts across all three classes instead of the detector's exit code or a single-class path. Unexecuted findings block only for a project with a$FM_HOME/data/exec-gate/<project>marker (read at merge time, absent by default, so the class ships inert and warn-only); the oldexcused==0defensive refusal is replaced by an explicit "exit 1 but no parseable finding line" refusal, and therc>1unverifiable refusal is unchanged.ids:(a bash glob so one entry covers many identifiers from a single cause) andkind:(missing|failing|unexecuted|any, absent meaninganyfor back-compat). Parsing fails closed on bothid:andids:present, unrecognized or unparseable field names, duplicated fields, an invalid kind, and any recognized field written afterreason:(with or without a space after the colon), since a swallowedkind:would silently widen an entry toany.bin/fm-pr-merge.sh's header becomes the single owner of the entry grammar, the marker contract, and the decision rule, withAGENTS.mdanddocs/architecture.mdcarrying cross-references only.tests/fm-pr-merge.test.shadds cases driven off a constructed detector stdout via a shimbin/of symlinks to the real scripts, covering the marker on/off paths, glob and kind matching, each fail-closed parse refusal, and the regression where an excused missing finding alongside a gated unexecuted one previously let the merge proceed.Risk Assessment
✅ Low: The fix commit closes both fail-open parse paths from the prior round with matching regression tests, keeps every previously honored entry shape working, and the only residual issue is a low-likelihood whitespace variant of an already-narrowed typo case.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-pr-merge.sh:226- The "field after reason" guard only matches" | $req: "(colon followed by a space), so a near-miss like... | reason: bumped runner | kind:unexecutedpasses the guard, gets swallowed into the reason text, and the entry defaults to kind=any. Verified: that line parses as ids=*, kind=any with no warning, i.e. a project-wide excusal of missing/failing/unexecuted from an entry the captain wrote intendingkind: unexecuted. Note the asymmetry: the same typo written before reason is refused as an unparseable field. Consider matching" | kind:"/" | id:"etc. (colon without requiring the space) in the after-reason guard.bin/fm-pr-merge.sh:245- The head-parsing loop assigns recognized keys unconditionally, so a duplicated field silently takes the last value with no warning. Verified:kind: unexecuted | kind: anyyields kind=any, andids: tests/legacy/* | ids: *yields ids=*. Both widen an entry beyond what the captain wrote, which is the same silent-widening failure mode the after-reason guard was added to prevent. The header enumerates fail-closed cases (both id and ids, unrecognized field, invalid kind, field after reason) but not duplicates; refusing a duplicated key would close it.bin/fm-pr-merge.sh:238- Splitting the head on every " | " means anid:(orids:) value containing " | " is no longer parseable:- id: tests/x.test.sh::handles a | b | project: ...is now refused as "an unparseable field 'b'", whereas the old parser (${rest%% | project: *}) honored it. Fail-closed, so no merge leaks, but a base assertion whose test name contains " | " can no longer be excused by anid:entry, while the header still documents id as "an exact <file>::<name>, matched by string equality". Either document the restriction in the grammar section or parse id/ids up to the next recognized key.bin/fm-pr-merge.sh:344- The finding loop useswhile IFS= read -r linewithout the|| [ -n "$line" ]continuation that supersession_approved's loop has, so a final finding line emitted without a trailing newline is dropped from parsing. Today's detector always prints with printf/'\n' so it is unreachable, but if it ever were, a dropped unexcused finding alongside an excused one would let the merge proceed (parsed>0 suppresses the no-parseable-line refusal). One-token fix keeps the gate's fail-closed property independent of detector output framing.tests/fm-pr-merge.test.sh:819- Two of the four fail-closed parse refusals added in this change have no case: an entry carrying bothid:andids:(bin/fm-pr-merge.sh:259) and an unrecognized/unparseable field name (bin/fm-pr-merge.sh:243,251). The invalid-kind, field-after-reason, and no-identifier branches are covered, so these two are the only new refusal paths that could regress silently to a fail-open parse.bin/fm-pr-merge.sh:99- Transient documentation skew, expected per the five-PR plan: this header and AGENTS.md now state that check 2 reportsunexecuted: <file>::<name>, while bin/fm-assert-tests-kept.sh's own header (lines 54-60) still documents only missing:/failing: and its code emits only those two classes until PR2 lands. Noting it so a reader comparing the two headers in the interim does not read it as a bug; no action needed if PR2 follows closely.🔧 Fix: fail closed on duplicated and no-space supersession fields
1 info still open:
bin/fm-pr-merge.sh:240- The tightened after-reason guard now matches" | $req:", which closes the no-space typo, but a whitespace variant still slips through:... | reason: bumped the runner | kind: unexecuted(two spaces after the pipe) does not contain " | kind:", so it is swallowed into the reason and the entry silently becomes kind=any. Verified: the same double-space field written BEFORE reason is refused as an unrecognized field ' kind', so the asymmetry the fix removed for one-character typos persists for this two-character one. Cheap close: run the guard against a whitespace-squeezed copy of entry_reason (or match*" |"*"$req:"*style) so any spacing after the separator is caught, keeping the existing 'reason must be the last field' wording. Lower likelihood than the typo just fixed, but it is the same silent scope-widening the header now says this file must never have.command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bin/fm-pr-merge.sh:128- bin/fm-pr-merge.sh's header states in the present tense that "check 2 reportsunexecuted: <file>::<name>", and AGENTS.md:469 / the exec-gate layout line say the same, but bin/fm-assert-tests-kept.sh's header (the owner of the detector's output contract, lines 54-65) documents onlymissing:/failing:lines and reports non-executable base test files as stderr name-check-only WARNINGs. The class genuinely does not exist in the detector yet; per the stated plan the pytest execution runner that emits it lands in the following PR. Left as-is deliberately: addingunexecuted:to the detector header would document behavior that does not exist, and hedging the merge header ("not yet emitted") would itself go stale the moment the runner lands. Worth a one-line check when that PR merges, that fm-assert-tests-kept.sh's "Output and exit status" section gains the third class so the two headers agree.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.