Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Confidence Score: 5/5The PR appears safe to merge, with the changed output matching the known run result and focused regression coverage protecting the intended behavior. The change only narrows an inaccurate status detail; existing repository consumers retain the unchanged state and source fields, and no accepted functional, security, or rule-compliance issue remains. Reviews (1): Last reviewed commit: "fix: report passed runs without merge cl..." | Re-trigger Greptile |
|
Speaking as Kun's firstmate: reviewed this HEAD ( I approved first-time fork CI 33393945837 and Require no-mistakes 33393945752 after a security read (one-line wording plus tests; no workflow files). This is waiting on CI, not on the author and not a captain hold. I will not merge until those checks are green, and I will merge at most this one of the overlap set if it stays green. |
Intent
Stop bin/fm-crew-state.sh from claiming a pull request merged when nothing landed. In the passed outcome mapping, run completion must not be presented as PR merged or closed: passed establishes only that the validation run completed successfully, not the pull request's forge merge or closure state. Report the known run result. Keep the checks-passed arm unchanged and prefer the small wording-only change; do not add a live forge read because it must be cheap and reliable at every call site, and wrong-but-quiet reporting is the defect. Add a colocated behavioral regression test through the executable/public interface that pins the passed-outcome wording and verifies a completed run with an open, unmerged PR is never described as merged or closed. Keep the diff tight and do not refactor the state reader. Do not include, depend on, or work around PR 3328's separate active-run trust changes. Do not push commits trying to turn action_required CI or Require no-mistakes gates green, and do not merge. Shellcheck must be clean on touched shell scripts.
What Changed
passedoutcome mapping inbin/fm-crew-state.shsoRUN_DETAILreportsrun passedinstead of claiming "PR merged/closed"; thechecks-passedarm is unchanged.tests/fm-crew-state.test.shpinning the passed-outcome wording and verifying a completed run with an unmerged PR is never described as merged or closed.Risk Assessment
✅ Low: A minimal wording-only change plus a colocated behavioral regression test that fails pre-fix, with lint clean and no reachable path left claiming merge/closure for a passed run.
Testing
Exercised the change through bin/fm-crew-state.sh's public CLI: the focused fm-crew-state suite passes with the new passed-outcome wording assertions, the new test was proven to fail against the pre-fix script (true regression coverage), shellcheck is clean on both touched scripts under the project's lint config, and a manual CLI transcript captured the actual user-visible output "state: done · source: run-step · run passed" for a completed run with an open unmerged PR — no merge or closure claim.
Evidence: fm-crew-state.sh CLI transcript for a completed passed run with an open, unmerged PR
Source: fm-crew-state.sh CLI transcript for a completed passed run with an open, unmerged PR
$ bin/fm-crew-state.sh feat-openpr # completed passed run, PR #1 still open state: done · source: run-step · run passed $ bin/fm-crew-state.sh feat-openpr | grep -iE "merged|closed" || echo "no merged/closed claim in output" no merged/closed claim in outputPipeline
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.
bash tests/fm-crew-state.test.sh— full focused crew-state behavior suite passes, includingtest_terminal_passedwith the new assertions (asserts "state: done · source: run-step · run passed" and absence of "merged"/"closed") against the real bin/fm-crew-state.sh executable via the harness fake no-mistakes/tmux/herdr driversRegression proof: temporarily checked out the base-commit bin/fm-crew-state.sh (0866a77) and re-ran the suite — the new assertion fails with "not ok - passed run does not claim the PR merged (unexpected: 'merged')", then restored the fixed script and confirmed byte-identical restorationshellcheck --norc --external-sources bin/fm-crew-state.sh tests/fm-crew-state.test.sh— clean under the project's pinned shellcheck 0.11.0 and fm-lint.sh's exact invocation flags, satisfying the shellcheck requirement on touched scriptsManual end-to-end CLI check mirroring the end-user surface: invoked bin/fm-crew-state.sh directly against the harness fixture (completed run, outcome passed, PR open and unmerged) and captured the transcript showingstate: done · source: run-step · run passedwith grep confirming no merged/closed claimbin/fm-crew-state.sh:318- The nm_ci_checks_state rationale comment still states that for captain-merge repos no-mistakes 'only reaches outcome=passed once the PR is actually merged'. This is pre-existing historical incident evidence for the ci-log check and was not made stale by this change (which altered only the passed-outcome presentation wording), so it was left untouched per scope discipline. It does carry a residual implication (passed implies merged) that contrasts with this change's premise that passed establishes only run completion; a future maintainer reconciling that comment with the new conservative wording may want to soften it, but verifying current no-mistakes outcome semantics was out of scope for this documentation pass.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.