fix(bin): tighten crewmate brief guidance on project memory, PR bodies, and GitHub reads - #1222
fix(bin): tighten crewmate brief guidance on project memory, PR bodies, and GitHub reads#1222bramdokman wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca36646bb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 3. For GitHub reads, prefer `gh api repos/OWNER/REPO/...` REST paths over `gh pr`/`gh issue` subcommands. | ||
| Use gh-axi for other GitHub operations and chrome-devtools-axi for browser operations. |
There was a problem hiding this comment.
Captain, every generated ship and scout brief now explicitly directs workers to invoke raw gh api, contradicting the repository-wide requirement that GitHub operations use gh-axi and the following sentence in the same generated rule. This causes routine REST reads to bypass the required GitHub integration path; retain REST-specific guidance without instructing workers to call raw gh.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
Intent
Fix the crewmate brief scaffold so generic project-memory guidance is conditional and explicitly subordinate to task-specific constraints, retaining its usefulness without allowing a run-helper versus never-run contradiction. For PR-producing ship briefs whose task names a GitHub issue, require a closing keyword in the PR body while stating plainly that GitHub may ignore it and firstmate verifies issue state after merge regardless; omit PR-only wording from local-only tasks. Prefer gh api repos/OWNER/REPO/... REST reads over gh pr and gh issue subcommands, and forbid workers from polling merge-queue state because firstmate already tracks it; apply worker guidance to ship and scout briefs while keeping secondmate charters sensible. Keep generated output concise, test through the rendered fm-brief CLI seam with TDD coverage for ship, scout, secondmate, and delivery-mode behavior, and do not add an agent co-author. The PR body must include a Completion Evidence section stating what was proved and what was not proved.
What Changed
bin/fm-brief.shnow makes the ship brief's project-memory section conditional: task-specific project-memory constraints take precedence, a task-level prohibition is decisive, and thefm-ensure-agents-md.shhelper step is reached only via the trailingOtherwise, ...branch — removing the run-helper-versus-never-run contradiction.direct-PRandno-mistakesmodes) emit a standalone**PR body:**step under Definition of done, before the step that creates the PR, requiring the closing keyword on its own line at the end of the body (never in the title) while stating plainly that GitHub may ignore it and firstmate verifies issue state after merge. Local-only briefs omit the step and keep their Rules list contiguous.GITHUB_READ_GUIDANCE: prefergh api repos/OWNER/REPO/...REST paths overgh pr/gh issuesubcommands, keep gh-axi/chrome-devtools-axi for other operations, and never poll merge-queue state. Secondmate charters are unaffected.tests/fm-brief.test.shaddstest_issue_linked_ship_requires_pr_closing_keywordandtest_github_rest_guidance_targets_crewmate_briefs, and strengthenstest_ship_project_memory_wordingwith ordering assertions.Completion Evidence
Proved:
bash tests/fm-brief.test.shis green at head (18 checks), including the two new tests and the strengthened project-memory test.bin/fm-brief.shfrom baseb6e351dfails the suite withnot ok - project-memory contract did not subordinate the generic guidance to task-specific constraints(exit 1); restoring head leavesgit status --porcelainempty.bin/fm-brief.shCLI seam for all four shapes — no-mistakes ship, direct-PR ship, local-only ship, scout, and a secondmate charter — and compared phrase-by-phrase against briefs rendered from the base commit.closing keyword/PR bodywording and no numbering gap between rules 3 and 4, and the secondmate charter contains neitherNever poll merge-queue statenor thegh api repos/OWNER/REPO/...guidance.git log --format='%B' b6e351d..ca36646 | grep -i co-authoredreturns nothing: no agent co-author trailer on any commit.tests/fm-instruction-owners.test.sh,tests/fm-ask-user-authority.test.sh, andtests/fm-secondmate-safety.test.shpass.Not proved:
AGENTS.mdsection 7 records no issue-verification step, and nobin/script (fm-pr-check.sh,fm-pr-merge.sh,fm-pr-poll.sh) reads GitHub issue state. The wording is required by the stated intent, so it was kept, but its owner is undocumented and unimplemented — a follow-up must either implement and document the duty inAGENTS.mdsection 7 or soften the wording. This branch does not change firstmate behavior.Risk Assessment
✅ Low: The final commit only reorders generated brief prose within the Definition-of-done block and tightens the corresponding CLI-seam assertion; re-rendering every delivery mode confirms all prior findings are resolved, no existing brief contract or rule numbering changed, and every source-verifiable intent criterion holds.
Testing
I exercised the change through the rendered fm-brief CLI seam rather than only the unit assertions: the fm-brief suite passes in full, and I proved the new tests are genuinely red by restoring the base script and re-running (first new assertion fails, exit 1), then restoring the worktree clean. I then generated real briefs for all five scaffold kinds at both base and head and compared them phrase by phrase — the project-memory section now leads with the task-specific precedence rule before the conditional
Otherwise, ...run the helperline (removing the run-helper vs never-run contradiction), both PR-producing modes carry a standalone**PR body:**closing-keyword step under Definition of done that is emitted before the step creating the PR and states plainly that GitHub may ignore it while firstmate verifies issue state after merge, the local-only brief carries none of that PR-only wording and keeps its Rules numbering contiguous, ship and scout briefs both prefergh api repos/OWNER/REPO/...and forbid merge-queue polling, and the secondmate charter inherits neither worker-only instruction. Commits carry no agent co-author, and generated briefs grew only modestly (ship no-mistakes 68 to 76 lines). Three adjacent suites that reference fm-brief also pass. No screenshot applies here because the end-user surface is generated Markdown consumed by a crewmate agent, so I captured the actual rendered brief.md files as the product-level artifact instead. One thing I could not prove: the repo's Bash 3.2 parse hazard (issue #1179) — only Bash 5.2 is available locally, so while the structural heredoc-in-command-substitution guard passes and the new blocks use the safeIFS= read -r -d ''form, real 3.2 enforcement remains with the macos-stock-bash CI job. The intent's PR-body Completion Evidence requirement is a PR-phase deliverable outside this phase.Evidence: Base-vs-head rendered-brief phrase matrix (proves every intent constraint at the CLI seam)
Rendered-brief phrase matrix: fm-brief.sh CLI output, base b6e351d vs head ca36646 REQUIRED in ship (no-mistakes) brief | BASE | HEAD ------------------------------------------------------------------------------ Task-specific project-memory constraints take precedence | ABSENT | present do not run the helper unless the task explicitly allows it | ABSENT | present PR body: if the task names a GitHub issue | ABSENT | present firstmate verifies issue state after merge regardless | ABSENT | present gh api repos/OWNER/REPO/... | ABSENT | present Never poll merge-queue state | ABSENT | present REQUIRED in direct-PR brief | BASE | HEAD ------------------------------------------------------------------------------ PR body: if the task names a GitHub issue | ABSENT | present gh api repos/OWNER/REPO/... | ABSENT | present REQUIRED in scout brief | BASE | HEAD ------------------------------------------------------------------------------ gh api repos/OWNER/REPO/... | ABSENT | present Never poll merge-queue state | ABSENT | present MUST STAY ABSENT in local-only brief (PR-only wording) | BASE | HEAD ------------------------------------------------------------------------------ closing keyword | ABSENT | ABSENT PR body | ABSENT | ABSENT MUST STAY ABSENT in secondmate charter (head render, worker-only guidance): "Never poll merge-queue state" 0 occurrence(s) "gh api repos/OWNER/REPO/..." 0 occurrence(s) Generated brief size, lines (base -> head): ship-nm-issue841 68 -> 76 ship-directpr-issue338 56 -> 64 ship-localonly-issue77 57 -> 61 scout-issue1190 44 -> 46Evidence: Rendered no-mistakes ship brief — the markdown a crewmate actually reads (key excerpt)
# Rules ... 3. For GitHub reads, prefergh api repos/OWNER/REPO/...REST paths overgh pr/gh issuesubcommands. Use gh-axi for other GitHub operations and chrome-devtools-axi for browser operations. Never poll merge-queue state; firstmate already tracks it. 4. Report status by appending one line: ... # Project memory Task-specific project-memory constraints take precedence over this generic section. If the task forbids, replaces, or narrows project-memory work, follow that constraint and do not run the helper unless the task explicitly allows it. Otherwise, ifAGENTS.mdorCLAUDE.mdalready exists, or if this task produced durable project-intrinsic knowledge, run.../bin/fm-ensure-agents-md.sh .in the worktree. # Definition of done The task is complete only when committed on your branch. PR body: if the task names a GitHub issue, put the closing keyword (Closes #123) on its own line at the end of the PR body - never in the PR title. This is required good practice, not proof of closure: GitHub may ignore the keyword, and firstmate verifies issue state after merge regardless. When you believe it is complete, appenddone: {summary}to the status file and stop. Firstmate will then instruct you to run /no-mistakes to validate and ship a PR.Evidence: Rendered local-only ship brief — PR-only wording correctly omitted, Rules numbering contiguous
# Rules 1. Never push to any remote and never open a PR. Work only on yourfm/ship-localonly-issue77branch; firstmate handles the merge into localmain. 2. Stay inside this worktree; modify nothing outside it. 3. For GitHub reads, prefergh api repos/OWNER/REPO/...REST paths overgh pr/gh issuesubcommands. Use gh-axi for other GitHub operations and chrome-devtools-axi for browser operations. Never poll merge-queue state; firstmate already tracks it. 4. Report status by appending one line: ... # Definition of done This project ships local-only: no remote, no PR, no pipeline. The task is complete only when committed on your branchfm/ship-localonly-issue77. Do NOT push, do NOT open a PR, do NOT merge. (no PR-body / closing-keyword step is emitted)Evidence: Rendered direct-PR ship brief — keyword step emitted before the PR-creation step
# Definition of done This project ships direct-PR: you raise the PR yourself, without the no-mistakes pipeline. The task is complete only when committed on your branch. PR body: if the task names a GitHub issue, put the closing keyword (Closes #123) on its own line at the end of the PR body - never in the PR title. This is required good practice, not proof of closure: GitHub may ignore the keyword, and firstmate verifies issue state after merge regardless. When it is implemented and committed, push your branch and open a PR withgh-axi, then appenddone: PR {url}to the status file and stop. Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome.Evidence: Rendered scout brief and secondmate charter (scout gets worker guidance, charter does not)
scout-issue1190/brief.md, Rules: 1. Never push to any remote and never open a PR. 2. Stay inside this worktree; the only files you may write outside it are the report and the status file below. 3. For GitHub reads, prefergh api repos/OWNER/REPO/...REST paths overgh pr/gh issuesubcommands. Use gh-axi for other GitHub operations and chrome-devtools-axi for browser operations. Never poll merge-queue state; firstmate already tracks it. 4. Report status by appending one line: ... sm-alpha/brief.md (secondmate charter): grep "Never poll merge-queue state" -> 0 occurrences grep "gh api repos/OWNER/REPO/..." -> 0 occurrencesEvidence: TDD red-state proof: new assertions fail against base b6e351d fm-brief.sh
ok - fm-brief.sh: bash -n succeeds ok - fm-brief.sh: no heredoc is nested inside a command substitution (Bash 3.2 parse-safe) ok - fm-brief.sh: --help renders the complete header ok - fm-brief.sh: no-mistakes/direct-PR/local-only briefs generate cleanly ok - fm-brief.sh: faster paths use configured authority without stacked review ok - fm-brief.sh: no-mistakes DOD keeps its apostrophe prose, now parse-safe not ok - project-memory contract did not subordinate the generic guidance to task-specific constraints exit=1Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (2) ✅
bin/fm-brief.sh:381- For local-only projects PR_BODY_GUIDANCE is empty (lines 346-353), so the bare$PR_BODY_GUIDANCEline at 381 renders as a stray blank line inside the numbered Rules list, between rule 3 and rule 4. Verified by rendering a local-only brief: rules 1-3, blank line, then '4. Report status...'. no-mistakes and direct-PR briefs have no such gap. Fix mechanically by folding the PR guidance into the rule-3 block (e.g. append it to GITHUB_READ_GUIDANCE only when non-empty) and emitting a single variable.bin/fm-brief.sh:349- The closing-keyword lines are indented three spaces, so they render as a continuation of rule 3 ('For GitHub reads, prefer gh api ... Never poll merge-queue state'). A PR-authoring obligation is therefore presented as part of the GitHub-read preference rule rather than as its own rule, which weakens how a crewmate scans it. Consider giving it its own numbered rule or attaching it to the Definition-of-done PR step.bin/fm-brief.sh:350- The brief tells crewmates 'firstmate verifies issue state after merge regardless'. The intent requires this wording, so it is authorized, but no such duty exists in the repo: bin/fm-pr-check.sh / fm-pr-poll.sh track PR merge state only, AGENTS.md's post-merge wake handling (line 353) covers clone refresh, and no script or doc reads GitHub issue state. The crewmate is told a verification happens that nothing currently performs.tests/fm-brief.test.sh:279- assert_no_grep uses grep -F (case-sensitive), and the pattern 'IfAGENTS.mdorCLAUDE.mdalready exists...' now differs from the emitted line 'Otherwise, ifAGENTS.mdorCLAUDE.mdalready exists...' only by the capital I. The assertion proves a capitalization difference, not that the helper step became conditional; the two positive assertions above it carry the real coverage.🔧 Fix: move PR closing-keyword step into definition of done
1 info still open:
bin/fm-brief.sh:315- The standalone**PR body:**step is emitted after the sentence it constrains in both PR modes: in direct-PR it follows "push your branch and open a PR withgh-axi, then appenddone: PR {url}... and stop" (line 313), and in no-mistakes it follows "Firstmate will then instruct you to run /no-mistakes to validate and ship a PR" (line 340), where the pipeline composes the body and the next paragraph says not to hand-edit while a run is active. A worker reading sequentially meets the body requirement after the step that creates the body. Moving the step above those sentences (or naming who carries the keyword into the pipeline-authored body) would make it unambiguous. Placement was the author's deliberate choice in the previous round, so this is a note, not a defect.🔧 Fix: emit PR-body keyword step before PR creation
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-brief.test.sh— full suite green (18 checks), including the newtest_issue_linked_ship_requires_pr_closing_keyword,test_github_rest_guidance_targets_crewmate_briefs, and the strengthenedtest_ship_project_memory_wordingTDD red-state proof: temporarily restoredbin/fm-brief.shfrom baseb6e351d, re-ranbash tests/fm-brief.test.sh→not ok - project-memory contract did not subordinate the generic guidance to task-specific constraints, exit 1; then restored head and confirmedgit status --porcelainemptyRendered end-user briefs through the CLI at head:FM_HOME=<ev> bin/fm-brief.sh ship-nm-issue841 acme-api,... ship-directpr-issue338 direct-proj,... ship-localonly-issue77 local-proj,... scout-issue1190 acme-api --scout,FM_SECONDMATE_CHARTER=... bin/fm-brief.sh sm-alpha --secondmate acme-apiRendered the same briefs from the base commit's script and built a base-vs-head phrase matrix over every intent-required and intent-forbidden phraseManually inspected the local-only brief: noclosing keyword/PR bodywording, and Rules numbering stays contiguous (rule 3 block → rule 4) after the REST guidance replaced the old rule 3Manually inspected the secondmate charter: zero occurrences ofNever poll merge-queue stateandgh api repos/OWNER/REPO/...git log --format='%B' b6e351d..ca36646 | grep -i co-authored— no agent co-author trailer on any of the three commitsbash tests/fm-instruction-owners.test.sh,bash tests/fm-ask-user-authority.test.sh,bash tests/fm-secondmate-safety.test.sh— adjacent suites that reference fm-brief, all greenbin/fm-brief.sh:301- Generated PR-producing briefs now tell crewmates "firstmate verifies issue state after merge regardless", but the owner of firstmate's post-merge duties (AGENTS.md section 7, "PR ready, landing, and teardown") records no issue-verification step - AGENTS.md contains zero mentions of GitHub issues, and no bin/ script (fm-pr-check.sh, fm-pr-merge.sh) reads issue state. The promise the scaffold makes to crewmates has no documented or implemented owner. I did not add it to AGENTS.md because recording a post-merge issue-verification duty would create new firstmate operating behavior rather than document an existing fact, which is outside this documentation phase and outside the stated intent. Decide whether firstmate genuinely owns that verification (then document it in AGENTS.md section 7 as a follow-up) or whether the brief wording should be softened.✅ **Push** - passed
✅ No issues found.