fix(crew-state): report a PR merge only when the merge record proves it - #5
Merged
Merged
Conversation
`outcome: passed` 只说明 no-mistakes 管线跑完,不代表 PR 已经合并——合并授权关闭时,合并仍等队长批准。此前 `bin/fm-crew-state.sh` 对每个 passed 运行都写死 `run passed: PR merged/closed`;实测 PR #4 在 `gh pr view 4` 报 OPEN/MERGEABLE 的同时,状态行却称它 merged,读它的 agent 会据此误报。 - passed 分支改为按证据说话:只有 merge-notified 记录(`bin/fm-pr-lib.sh` 的 `fm_pr_poll_merge_already_notified`,读写契约不动)证实了该任务规范 PR 身份的合并,才写 `run passed: PR merged`;否则写 `run passed: PR held for merge` 并在 `state/<id>.meta` 有 `pr=` 时附上 URL。`RUN_STATE=done` 的分类不变(绿检查后的 held-for-merge 仍是 done)。 - 文件头新增状态用词表(本脚本作为单一 owner),写明「未证实不得说 merged」,供后续加分支的人照抄。 - `tests/fm-crew-state.test.sh` 加三条用例:无合并记录 -> held for merge 且带 URL;有合并记录 -> 精确输出 `PR merged`;记录属于另一个 PR -> 不构成证据。三条在旧代码上均失败。
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.
摘要
修的是机队读任务状态的工具
bin/fm-crew-state.sh一处会误报的措辞:终端passed运行的状态行原先固定写run passed: PR merged/closed,可是 no-mistakes 的outcome: passed只说明管线跑完,不等于 PR 已经合并——合并授权关闭时,合并仍等队长批准。实测这条:PR #4 当时在gh pr view报 OPEN / MERGEABLE,而状态行称它 merged;任何读这行的 agent(包括我)都会据此误报。现在这条状态行只说被证实的 PR 状态:
bin/fm-pr-lib.sh的 merge-notified 记录,"由真实合并观察写入")→run passed: PR merged: <url>,并把被证实的 PR 明确写出来,读者可当场核对;run passed: PR held for merge[: <url>],绝不出现 merged 字样;RUN_STATE=done分类不变(绿检查后的 held-for-merge 仍是 done),只改措辞的诚实度;pr:为准,仅当 run 没记pr:时才回落到任务state/<id>.meta的pr=——这样只会把结论从 merged 移向 held for merge,不会反向,即严格更保守。bin/fm-crew-state.sh文件头新增状态用词表(该脚本作为单一 owner):PR merged/PR held for merge/checks green: PR ready for review/parked at <gate>/validating (running|fixing)/ci running/run failed,并写死「未证实不得说 merged」这条铁律,后续加分支的人照抄即可。bin/fm-pr-lib.sh与bin/fm-merge-outcome-lib.sh的对外契约未动。验证
live-verify,ALL-PASS):绑真实no-mistakes axi status(cbm-axi的 completed run,outcome: passed)+ 真实 forge 观察经bin/fm-pr-poll.sh/fm_merge_outcome_report写 merge-notified 记录。同一个 run:无记录读PR held for merge: <url>;有记录读PR merged: <url>;记录属于另一个 PR 时不算证据;记录存在但读不出(空文件)也不算证据。aba68fc读run passed: PR merged/closed;中间版本e40b4b4(按 meta 优先)对「旧 meta PR 已合并、本次 run 是另一个未合并 PR」会误报 merged——本 PR 已修成 run 优先。tests/fm-crew-state.test.sh89 项全绿,其中 5 条为本 PR 新增:无记录→held for merge;有记录→精确输出PR merged;身份不符→不构成证据;run 的pr压过过时的 meta;完全没有 PR 身份→不声称合并也不编造 URL。bin/fm-lint.sh(pinned ShellCheck 0.11.0 + actionlint 1.7.12)通过。风险
低。只改一个状态报告器的措辞;
state:/source:两个 token 不变,仓库内没有消费者依赖被删掉的旧串;所有读不到证据的路径都倒向保守措辞,因此不存在「未证实却说 merged」的方向。English
Intent
队长原话:「修」。
指的是这条实测缺陷(crew-state-passed-mislabels-unmerged-pr):机队读任务状态的工具会把「管线跑完」说成「PR 已合并」,可实际上 PR 还开着等你批。
现场:bin/fm-crew-state.sh 对 PR #4 输出 state: done · source: run-step · run passed: PR merged/closed,而同一时刻 gh pr view 4 是 OPEN / MERGEABLE、未合并(yolo=off,正等队长批准)。这行字会让人(和任何读它的 agent)以为已经合了 —— 队长刚差点被我据此误报。
队长要的是:状态行不能凭空声称一个没被证实的合并。
What Changed
bin/fm-crew-state.shno longer prints "run passed: PR merged/closed" for every terminalpassedrun. A passed outcome now states only what is proven:run passed: PR merged: <url>when the merge-notified record owned bybin/fm-pr-lib.shexists for this run's PR, otherwiserun passed: PR held for merge[: <url>].nm_pr_url/nm_pr_merge_proven, which resolve the attributed run's ownprfield first (falling back to the task'sstate/<id>.metapr=) and accept the merge-notified record as the only proof — an unknown, unparseable, or identity-mismatched URL is not proof, so a stale meta PR can no longer vouch for a different, still-open PR.outcome: passedmeans the pipeline finished, never that the PR merged.tests/fm-crew-state.test.shadds five cases covering unmerged-passes-read-held-for-merge, proven-merge wording, an identity-mismatched merge record, runproutranking stale meta, and a passed run with no PR identity (no merge claim, no invented URL).Risk Assessment
✅ Low: The change is presentation-only (state/source words unchanged, detail wording gated by a durable proof), the four new tests discriminate the fixed behaviour from both the old unconditional claim and the stale-meta identity mix-up, and every unreadable/absent-proof path fails toward the hedged wording, so nothing unproven can be claimed as merged.
Testing
Drove the real bin/fm-crew-state.sh end-to-end against real run data: a real no-mistakes completed run with outcome passed (cbm-axi, branch docs/agent-pr-axi-usage, head == run head) plus the real merge-proof path (fm-pr-poll.sh reading the real forge with gh, fm_merge_outcome_report writing the merge-notified record), all with task meta and merge records in a throwaway home. Before the change the same real run read 'run passed: PR merged/closed' with no merge record present; the fixed script reads 'run passed: PR held for merge: <url>', upgrades to 'run passed: PR merged: <url>' only once a real merge observation is recorded, and refuses to be proved by a merge record for a different PR (the meta-first revision e40b4b4 claimed 'run passed: PR merged' on those same live inputs). An unreadable merge record was also driven live and stayed a hedge. The repo's targeted suite (tests/fm-crew-state.test.sh, 89 checks) passes, including the regression pair. Two shapes could not be bound live on this host: a passed run whose PR is still open on the forge (every bindable completed run references a merged PR, and all other candidates fail the branch+head attribution rule), and a passed run with no PR identity at all (the only such runs, cyber-mux and prime-agent, fail the head rule). The open-PR wording is covered by the repo suite's identity-matching case, and the no-PR-identity criterion is covered by a focused exact-line test added in this run; neither gap leaves doubt about the intent, since the product keys the claim on firstmate's own merge record rather than on the forge or on the pipeline outcome.no-mistakes axi status(outcome: passed, pr .../cbm-axi/pull/2), state dir in a throwaway home…state: done · source: run-step · run passed: PR merged/closedbin/fm-pr-poll.sh --validated github ... github.com onyx-space/cbm-axi 2returnedmergedfrom the real forge,fm_merge_outcome_reportwrote the merge-notified…live-d.pr-poll-merge-notifiedfile -> held for merge, no merged claimEvidence: Live verification driver (runs the real script against real no-mistakes/gh data; also runs base aba68fc and meta-first e40b4b4 on the same inputs)
Source: Live verification driver (runs the real script against real no-mistakes/gh data; also runs base aba68fc and meta-first e40b4b4 on the same inputs)
Evidence: Live verification transcript (ALL-PASS, reproducible)
Source: Live verification transcript (ALL-PASS, reproducible)
=== A. terminal passed run, NO merge record: no merge may be claimed === fm-crew-state.sh live-a (fixed): state: done · source: run-step · run passed: PR held for merge: https://github.com/onyx-space/cbm-axi/pull/2 fm-crew-state.sh live-a (aba68fc): state: done · source: run-step · run passed: PR merged/closed === B. merge PROVEN by the real poll === fm-crew-state.sh live-b (fixed): state: done · source: run-step · run passed: PR merged: https://github.com/onyx-space/cbm-axi/pull/2 === C. stale meta names another merged PR, the run works a different one === fm-crew-state.sh live-c (fixed): state: done · source: run-step · run passed: PR held for merge: https://github.com/onyx-space/cbm-axi/pull/2 fm-crew-state.sh live-c (e40b4b4): state: done · source: run-step · run passed: PR merged === result: ALL-PASS ===Evidence: Targeted repo suite log: tests/fm-crew-state.test.sh (89 checks) — includes the four new cases and the added no-PR-identity case
Source: Targeted repo suite log: tests/fm-crew-state.test.sh (89 checks) — includes the four new cases and the added no-PR-identity case
ok - terminal passed run with an open PR reads held-for-merge, never merged ok - terminal passed run with a proven merge record reads PR merged ok - an identity-mismatched merge record does not prove this PR merged ok - the attributed run's PR identity outranks a stale task meta PR ok - a passed run with no PR identity claims no merge and names no URL all fm-crew-state tests passedEvidence: Evidence README: what was driven live, how to reproduce, and the live limits
Source: Evidence README: what was driven live, how to reproduce, and the live limits
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed (3) ✅
bin/fm-crew-state.sh:41- The new HARD RULE is stated absolutely ("no detail line may contain &fix(bin): split the silent-pane readout three ways and defer the focus-blocked teardown close #34;merged&fix(bin): split the silent-pane readout three ways and defer the focus-blocked teardown close #34; unless nm_pr_merge_proven confirms it"), but it is not true of this file. On the no-attributed-run fallback path (line 924) the helper relays a crew-authored status note verbatim (status_line_note returns the text after the verb, bin/fm-classify-lib.sh:381), so a crew line likedone: PR merged(a shape this repo's own fixtures use, tests/fm-inactive-reconcile.test.sh:740) yieldsstate: done · source: status-log · PR mergedwith no proof check. Because the new header declares itself the single owner of the vocabulary, a future maintainer enforcing the sentence literally would have to suppress the crew's claim. Scope the sentence to the detail lines this helper derives from the run outcome (source: run-step); the relayed line already carries source: status-log as its own provenance.bin/fm-crew-state.sh:560- The pre-existing nm_ci_checks_state comment still asserts the opposite of what this change adds in its header: 'for a repo where merge is left to the captain ... it only reaches outcome=passed once the PR is actually merged (or failed/cancelled if closed)'. The change's premise (PR fix(spawn): pre-answer Pi's project-trust gate on pi and pi-signed launches #4 read OPEN/MERGEABLE while the run-step line claimed a passed outcome, yolo=off) and the new header lines 39-44 say passed means the PIPELINE finished, without asserting a merge. The file now carries two contradictory statements of one invariant, and a reader trusting this comment would restore the old unconditional 'run passed: PR merged/closed' wording that the intent forbids. Reconcile the comment with the evidence-bounded reading (the ci-log heuristic it justifies stands on its own markers either way).bin/fm-crew-state.sh:731- Evidence-bounded ceiling of the new probe, not a defect. Concrete sequence: a ship task whose crew never ran bin/fm-pr-check.sh (no armed poll, so no merge-notified record can be written) is later approved by the captain in the forge UI; the run-step line then staysrun passed: PR held for merge: <url>indefinitely although the merge landed, so a reader learns 'held for merge' means 'no proof of merge here', not 'the PR is still open'. The header documents this as the intended safe word whenever the record is absent or unreadable (lines 27-31), and the alternative (probing the forge on every read, in a path the watcher, fm-classify-lib.sh's absorb check, and the fleet snapshot call repeatedly under a timeout) is a larger change the intent does not ask for. No action; recorded so the threshold is explicit rather than implicit.🔧 Fix applied.
1 warning still open:
bin/fm-crew-state.sh:504- The new proof is looked up for the URLnm_pr_urlprefers, and it prefers the task's metapr=over the run's ownpr:field. The claim is about the run the line reports, but the identity proved is whichever PR the meta happens to record, and the merged wording names no URL (line 733:run passed: PR merged), so a reader cannot see which PR was proved. Concrete reachable sequence: a task's PR fix(herdr): exec the server launch so no forked shell waits on it #1 is announced (fm-pr-check.sh rewrites metapr=to fix(herdr): exec the server launch so no forked shell waits on it #1, arming its poll) and fix(herdr): exec the server launch so no forked shell waits on it #1 merges, writing the merge-notified record for fix(herdr): exec the server launch so no forked shell waits on it #1; the task record survives (teardown, which removes both meta and marker, has not run) and follow-up work starts a new run on the same task whose pipeline PR is fix(bin): 停止将 mate home 的父通道读取为任务日志 #2. If the crew's ready signal for fix(bin): 停止将 mate home 的父通道读取为任务日志 #2 is not routed through bin/fm-pr-check.sh (a step the brief asks a model to remember; this repo's own designs avoid depending on an agent remembering), meta still names fix(herdr): exec the server launch so no forked shell waits on it #1, sonm_pr_merge_provenreturns true from fix(herdr): exec the server launch so no forked shell waits on it #1's record and the terminal passed run for fix(bin): 停止将 mate home 的父通道读取为任务日志 #2 printsrun passed: PR mergedwhile fix(bin): 停止将 mate home 的父通道读取为任务日志 #2 is OPEN - the exact unproven-merge claim the intent forbids. The same stale identity also makes the hedge actively misleading: the not-proven branch appends that meta URL, yieldingrun passed: PR held for merge: <#1 url>and pointing the reader at a PR that is in fact already merged. Something like this has to be considered a real shape here: the change's own new test test_terminal_passed_merge_record_for_other_pr_is_not_proof constructs a task whose meta PR and merge record are different PRs. Note the file already resolves 'this task's PR' a second, different way in a sibling run-step path: nm_reclassify_failed_run_as_held_green (line ~492) reads the run's ownprfield only. Smallest honest remedy: when the attributed run records apr:, bind the proof (and any URL printed) to that identity and use meta only when the run records none - that can only move a claim from 'merged' to 'held for merge', never the reverse; optionally also name the proved URL in the merged wording. The meta-first order is documented as deliberate in the helper's comment above line 498, so this is a decision for the author rather than a mechanical patch.🔧 Fix applied.
1 info still open:
tests/fm-crew-state.test.sh:994- The three new tests call the real marker writer in-process:fm_pr_poll_merge_mark_notifiedat tests/fm-crew-state.test.sh:994, :1021 and :1042. That function runsumask 077(bin/fm-pr-lib.sh:991) and deliberately never restores it, because its production callers (bin/fm-watch.sh, bin/fm-pr-merge.sh) are one-shot script processes. Here it is called from the sourced test shell, so umask 077 leaks for the remainder of the run — every test after this point (the file continues for ~1500 more lines of test invocations, including all the run-attribution, remote-secondmate and coarse-ledger cases) executes with umask 077 instead of theumask 022that tests/lib.sh:34 pins on purpose for the state-root/process-event fixture contracts, and unlike the repo's own convention for scoped restrictive umasks (tests/fm-captain-hold-lifecycle.test.sh:203 does(umask 077; mkdir -p ...), tests/fm-procevent.test.sh:345 scopes it into a child process). The direction is currently harmless — a stricter umask only makes new files/dirs more private, and the only mode gate on that root (bin/fm-procevent-lib.sh:535) merely refuses group/world-writable state dirs, so no test fails today — so this is fixture hygiene, not a demonstrated breakage. Mechanical remedy: run the writer in a subshell, e.g.( fm_pr_poll_merge_mark_notified "$d/state" <id> github github.com o/r 1 ) || fail ..., or re-umask 022after each call, so the suite keeps running under the umask its fixtures were written against.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
no-mistakes axi status(outcome: passed, pr .../cbm-axi/pull/2), state dir in a throwaway home…state: done · source: run-step · run passed: PR merged/closedbin/fm-pr-poll.sh --validated github ... github.com onyx-space/cbm-axi 2returnedmergedfrom the real forge,fm_merge_outcome_reportwrote the merge-notified…live-d.pr-poll-merge-notifiedfile -> held for merge, no merged claimtests/fm-crew-state.test.sh(targeted repo suite, real script under test, stubbed upstream tools): 89 checks, all green, including the four new cases and the regression pair~/.no-mistakes/evidence/01M26XWAR71A7JAAGCGYNJ3F4F/live-verify.sh <worktree>— live driver, re-run twice with identical ALL-PASS resultsLive case A: real passed run (cbm-axi,outcome: passed,pr: .../pull/2), no merge record -> fixed readsrun passed: PR held for merge: <url>; baseaba68fcreadsrun passed: PR merged/closedLive case B: real forge observation viabin/fm-pr-poll.sh --validated github <url> github.com onyx-space/cbm-axi 2plusfm_merge_outcome_report-> fixed readsrun passed: PR merged: <url>Live case C (adversarial): merge record for a different real merged PR while the run's own PR has none -> fixed stays held-for-merge naming the run's PR and never names the stale PR;e40b4b4(meta-first) claimedrun passed: PR mergedon the same inputsLive case D (adversarial): an unreadable (empty) merge record is not proof -> held for mergeAdded focused testtest_terminal_passed_without_pr_identity_names_no_urlfor acceptance criterion 2 (no URL append when no PR identity exists anywhere)Read-only scan of every initialized no-mistakes repo on this host (sqlite3 ~/.no-mistakes/state.sqlite,no-mistakes runs,git merge-base --is-ancestor) to establish that onlycbm-axiandtasks-axihave a bindable live completed run, both with already-merged PRs✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.