fix(bin): scope the crew-state step-row reader to the steps table

[loom-loki]

Intent

Tighten the step-row reader in bin/fm-crew-state.sh so its merge reporting is correct by construction, then carry the result upstream to kunchenguid/firstmate.

Background: bin/fm-crew-state.sh claimed a no-mistakes run had merged a pull request when it had not. A run reaches outcome=passed once its steps finish without failing, which includes a run whose pr and ci steps were SKIPPED - such a run opened and merged nothing, yet the reader mapped outcome=passed to the fixed detail 'run passed: PR merged/closed'. The change already on this branch states the rule generally rather than special-casing one step: the merge claim is made only when the pr step AND every step after it actually completed.

The new work in this run: an automated review raised that the step parser should be constrained to the steps table, because a valid numeric file path on a finding whose id matches pr passes the row predicate and shadows the genuine PR step, producing an incorrect terminal detail. That shape does not occur on v1.60.2, because the encoder quotes a numeric-looking string field - but the guard was then correct only by accident of one version's output format, so it has been made correct by construction instead. The step-row reader is now scoped to the steps table by POSITION (rows after the steps[N]{...}: header, indented deeper than it, up to the first dedent), with the header anchored so active_steps[...] cannot open the section, replacing the column-content predicate that required an unquoted numeric third column. This also removes the opposite failure mode, where a quoted duration_ms would have made genuine step rows unreadable.

Deliberate decisions a reviewer reading only the diff would not know:

• The column-content predicate was removed rather than kept alongside the scoping. Keeping both would reintroduce the encoder-quoting dependency the change exists to eliminate.
• The existing negative test was EXTENDED, not replaced: its fixture's file column is parameterized and the case now runs for both an ordinary a.go and a bare numeric 123. The numeric case was verified to FAIL against the previous guard (reporting 'run passed, PR step warning: merge state unknown to the run' from the finding) and to pass after, because an assertion that would pass either way is worthless here.
• Comments in both files that justified the old numeric-third-column rule were rewritten rather than left to rot into false claims.
• This is the third time on this change that the rule has been stated generally rather than special-cased to the instance found; that consistency is intentional.

Scope: exactly two files, bin/fm-crew-state.sh and tests/fm-crew-state.test.sh. Touching a third is a stop-and-report condition. Because this is offered to another project, the pull request has to stand on its own for a maintainer with none of this repository's context.

The upstream checks require maintainer approval before they run on a first-time fork contribution; that is not a validation failure and must not be reported as one.

What Changed

• nm_step_rows in bin/fm-crew-state.sh now selects step rows by position instead of column content: an awk pass opens the section at a line-anchored steps[N]{...}: header (so active_steps[...] cannot open it), emits deeper-indented rows carrying a leading <field>,, and closes at the first non-blank dedent to the header's level. This replaces the grep predicate that required an unquoted numeric third column, which let a findings row with a numeric file column and an id of pr shadow the genuine PR step and report the finding's severity as the step status, and which conversely would have dropped genuine step rows had duration_ms ever rendered quoted.
• The skipped-PR terminal detail suffix now reads merge state unknown to the run rather than merge state unknown to it, matching the wording of the other unproven-merge details.
• Tests: the findings-row shadowing case is parameterized over its fixture's file column and runs for both a.go and a bare numeric 123; a new fixture and test render a live run's steps table alongside its active_steps{step,status,active_for,...} table and assert the ci step is still read from the steps row; four terminal-detail assertions were tightened from substring fragments to the full detail string.

Risk Assessment

✅ Low: The change is well-bounded to two files and one parser function, its central evidence claim reproduces exactly against the live store and real CLI output (657/657 rows across 73 runs; active_steps correctly excluded on the currently-running run), every detail branch is now pinned by a full-string assertion, and the only outstanding item is a comment-wording overclaim with no behavioral effect.

Testing

Completed 1 recorded test check.

• Outcome: ⚠️ 1 error across 1 run (3m19s)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info

• ⚠️ bin/fm-crew-state.sh:329 - The "Verified against all 495 step rows ... all 495 are read" sentence is pre-existing context (it survives unchanged from 542edd9) and was written to evidence the REMOVED column-content predicate: it measured that 495 rows matched an unquoted-numeric-third-column regex. This commit replaces that reader with a positional one, so the equivalent property is now "all 495 rows sit inside a steps[N] section indented deeper than its header" - which the old scan did not test. The sentence now sits inside the paragraph describing the positional rule, so a reader (including the upstream maintainer this is contributed to) takes it as verification of the current reader. Neither the diff nor 0630c0d's commit message states the scan was re-run. This matters more than usual here because the change's own stated discipline (and the intent's) is that comments justifying the old rule must be rewritten rather than left to rot into false claims - this is the one evidence claim carried across the implementation swap. Remedy: either re-run the store-wide scan against the positional reader and say so, or scope the sentence to what it actually established. Flagging rather than fixing because only the author knows whether the scan was re-run.
• ℹ️ tests/fm-crew-state.test.sh:580 - The comment says "The ci status must come from the steps row ci,running,0,0, never from the active_steps row ci,running,1m40s,...", but the fixture cannot fail on the "never" half. nm_step_status does nm_step_rows | grep -E "^[[:space:]]*ci," | head -1, and in run_ci_monitoring_with_active_steps the steps row precedes the active_steps row, so head -1 selects the steps row even if active_steps rows leaked into the stream - and both rows would yield the same status word running anyway. I confirmed this by tracing an intentionally unanchored header regex through the parser: the active_steps rows get appended after the steps rows and the derived ci status is unchanged, so the test still passes. The test IS a valid positive guard (dropping the steps rows makes CI_STEP_STATUS empty, the green-checks override never fires, and state: done fails), which is what the comment's later "pins that the narrowing left the ACTIVE-run path intact" clause claims. Only the "never from the active_steps row" clause is unpinned. Given the author's stated standard that "an assertion that would pass either way is worthless here", narrow the comment to the positive property it actually pins.

🔧 Fix: correct step-row evidence and active_steps test comments
2 issues (1 warning, 1 info) still open:

• ⚠️ bin/fm-crew-state.sh:415 - The skipped-pr detail emits run passed, PR step skipped: no PR was opened or merged, merge state unknown to the run. Commit d1d81a8 dropped by the run from the first clause while making the suffix uniform across the four branches. The sentence now asserts unscoped that no PR was opened or merged, and then says merge state is unknown to the run - self-contradictory, and false in the exact scenario this change exists for: the evidence comment at lines 390-394 records run 01M1EA5NJVP18AE7SPY5MBYW42 reporting pr,skipped,0,17 under outcome=passed while the forge still had that branch's pull request OPEN and unmerged. Concrete path: a run whose push provider cannot be resolved skips pr and ci, reaches outcome=passed, and fm-crew-state renders detail: run passed, PR step skipped: no PR was opened or merged, ... for a branch whose PR is open on the forge. The file's own doc comment at line 35 keeps the correct scoping ("a skipped pr step means the run opened and merged nothing"); only the user-visible string lost it. The base text at 542edd9 (no PR was opened or merged by the run, merge state unknown to it) was accurate. Remedy is restoring the run scoping in that clause (e.g. no PR was opened or merged by the run) while keeping the uniform merge state unknown to the run suffix the tests now assert - but the suffix uniforming was a deliberate prior-round decision, so the author should confirm the wording rather than have it silently reverted.
• ℹ️ bin/fm-crew-state.sh:329 - "the whole local v1.60.2 store ... all 73 runs" can be read as "73 runs produced by v1.60.2". In the store as it stands, 51 of the 73 runs were produced by v1.57.0 and only 22 by v1.60.2. This does not weaken the evidence - I reproduced the scan and the reader reads 657 of 657 rows with zero per-run mismatches against step_results - because output shape is set by the RENDERING binary (v1.60.2 for all 73), not by the producing version. If anything the claim is broader than stated: the reader also handles rows written by an older version. Noting it so the figure is not misread; no change required.

🔧 Fix: restore run scoping in skipped-PR detail, pin full strings
1 info still open:

• ℹ️ tests/fm-crew-state.test.sh:410 - The fixture comment asserts and denies the same fact in one sentence. It says "the durations observed across those samples span both second scale (7s, 18s, 19s) and minute scale (2m34s, 3m41s ...), so the 1m40s below is a duration string that version actually renders rather than a plausible-looking one invented to fill the column", then immediately concedes "only the step name and the durations differ here". 1m40s is not in the cited observed set {7s, 18s, 19s, 2m34s, 3m41s}; what the evidence supports is the minute-scale SHAPE, not that literal string. The bin/fm-crew-state.sh counterpart added by the same commit (1cc57ee) got this right - "the minute-scale third column the test fixture carries is a shape this version actually renders" - but commit 0630c0d rewrote that block and dropped the sentence, leaving only the imprecise string-level claim in the test file. Remedy is wording only: claim the shape ("a minute-scale duration of the form this version renders") and keep the observed sample list, so the sentence stops contradicting its own next clause. No behavior is affected - the active_steps third column is now rejected structurally and is never read.

⚠️ **Test** - 1 error

• 🚨 tests failed with exit code 1
• bin/fm-test-run.sh --changed --exclude-family real-herdr-gated

⚠️ **Document** - 1 info

• ℹ️ bin/fm-crew-state.sh:333 - Out-of-scope consolidation worth a follow-up, reported rather than done. The nm_step_rows comment is now ~50 lines, over half of it a verification narrative (657 of 657 rows across 73 runs, ground-truthed against step_results; live mid-step captures on v1.60.2 eb4e379 built 2026-08-29). Under the placement tree in .agents/skills/firstmate-coding-guidelines/SKILL.md that is tier-5 material - 'active reusable verification for a current guarantee' - whose classified home is a maintainer-verification record under docs/verification/, which docs/documentation-audiences.md says is the surface allowed to carry dates, versions, exact commands and exact output. Tier 7 gives the script header the mechanics (the positional rule itself), not the evidence run. I did not move it, for three reasons: the file already carried comparable inline evidence before this change (the v1.32.2 / '360+ real run logs' paragraph at line ~466), so this change follows the file's established convention rather than introducing drift; the evidence is tightly coupled to why the parser is shaped this way, which is a legitimate reason to keep a distilled form inline; and extracting it would touch a third file, which this run's stated constraints make a stop-and-report condition. If the split is wanted, the shape would be a docs/verification/ record owning the scan and the live captures, with a one-line pointer left in the comment - not a synchronized second copy.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.