Skip to content

feat(bin): wire GitHub issues into task delivery - #1271

Open
HelloWorldSungin wants to merge 5 commits into
kunchenguid:mainfrom
HelloWorldSungin:fm/fm-issue-lifecycle-wiring
Open

feat(bin): wire GitHub issues into task delivery#1271
HelloWorldSungin wants to merge 5 commits into
kunchenguid:mainfrom
HelloWorldSungin:fm/fm-issue-lifecycle-wiring

Conversation

@HelloWorldSungin

Copy link
Copy Markdown

Intent

Make GitHub issue lifecycle part of Firstmate's delivery machinery so issue-sourced work cannot remain merged-but-open merely because an agent forgot. Add an optional GitHub issue number to fm-brief.sh while keeping briefs generated without it byte-identical; issue briefs must require a substantive issue comment describing findings and actual changes and a Closes # PR-body keyword. Record the explicit issue identity into task metadata at spawn time and never infer it from PR text. After a successful GitHub PR merge, verify the recorded issue is closed, close it with the merged PR URL if needed, and make any bookkeeping failure loud without making the completed merge appear failed or retryable. Preserve behavior for tasks with no issue and for non-GitHub providers, add colocated tests for all paths, and keep scripts shellcheck-clean. During verification, two pre-existing suite failures were reproduced and corrected: coverage comparison now uses the same C locale as its sorted inputs, and the Calm E2E waits for all asynchronously hidden transcript rows before asserting.

What Changed

  • Add opt-in GitHub issue traceability to ship briefs, persist only explicit validated issue identities at spawn, and require substantive issue updates plus Closes #<number> PR keywords.
  • Reconcile recorded issues after successful GitHub PR merges by verifying and closing them with the merged PR URL, while keeping bookkeeping failures loud without making completed merges retryable.
  • Add lifecycle regression coverage and documentation, preserve byte-identical no-issue briefs, and stabilize locale-sensitive coverage checks and asynchronous Calm transcript assertions.

Risk Assessment

⚠️ Medium: Captain, the source is safe to merge with exact no-issue golden coverage added; residual risk is limited to the explicitly accepted external or queued-merge containment deferred to separate work.

Testing

Targeted brief, spawn, merge, locale, and real Pi Calm terminal E2E checks passed; the initial newly added brief assertion had a test-only matcher error that was corrected and rerun, lifecycle evidence was captured, and no screenshot was produced because the Calm change only stabilizes an existing E2E wait and changes no production UI.

Evidence: Issue lifecycle end-to-end transcript

Generated issue brief -> persisted issue=42 -> merged PR -> verified, closed, and re-verified issue; a simulated close failure remained exit 0 with a loud warning.

GENERATED BRIEF TRACEABILITY SECTION
<!-- firstmate-task-issue=42 -->
# GitHub issue traceability
Before reporting the PR ready, comment on GitHub issue #42 with a substantive summary of what you found and what you actually changed.
A bare "done" comment does not satisfy this contract: someone reading the issue later must be able to understand the outcome without opening the PR.
Put `Closes #42` in the PR body so merging the PR closes the issue atomically.

SPAWNED TASK METADATA
window=firstmate:fm-issue-lifecycle-e2e
endpoint_task_id=issue-lifecycle-e2e
worktree=/tmp/no-mistakes-evidence/01KYQNXS7R092KWEFFTN90HQ41/fm-spawn-worktree-settle.07KNld/lifecycle-e2e/wt
project=/tmp/no-mistakes-evidence/01KYQNXS7R092KWEFFTN90HQ41/fm-spawn-worktree-settle.07KNld/lifecycle-e2e/project
harness=codex
kind=ship
mode=no-mistakes
yolo=off
tasktmp=/tmp/fm-issue-lifecycle-e2e
model=default
effort=default
issue=42

POST-MERGE GITHUB CALLS
pr merge 31 --repo example/repo --squash
issue view 42 --repo example/repo --full
issue close 42 --repo example/repo --reason completed --comment Closed after merge of https://github.com/example/repo/pull/31.
issue view 42 --repo example/repo --full

BOOKKEEPING FAILURE EXIT CODE
0

BOOKKEEPING FAILURE DIAGNOSTIC
error: issue close failed
warning: PR merge succeeded: https://github.com/example/repo/pull/33; GitHub issue bookkeeping did not complete: could not close issue #44
Evidence: No-issue byte-identity evidence

cmp exit=0 for all eight no-issue brief variants against fixed pre-issue fingerprints.

cmp exit=0 - generated no-issue brief fingerprints match the fixed pre-issue goldens byte for byte
c54d136824bd32100f22949e1341106bdab4362eedbb77db21e90fabe3e9e465 5710 golden-nm
292f1156c55c6630f9932fcb5bf263b48e0d201c3788efbc5342caced2b9380d 4664 golden-direct
9291d1b098370fffe43fb648ed72b1f97af7112f7839f200ece5ef4763978332 4885 golden-local
5aca7c99fd63b68a8e63199a6aa8dc20781651a199adf551eb955cd6cc9c0d7a 7349 golden-ship-herdr
003e320712d7b6b22d660b780e0b4c5093d2ea37c6de25118b8af89268daf881 3467 golden-scout
274adfc0040e80de876a0bff575db904ab552cd5b25efacad4591accdc148927 5103 golden-scout-herdr
fc1a9ead283eb329d09e9a2754e8e10b0441b3c440e09da94fab0c2798d2129a 5191 golden-secondmate
4e46baf68d83f1836312fcd9ca1cf2630d4924fed3faa05b6a637a469613724b 5424 golden-secondmate-empty
Evidence: Non-C locale coverage check

FM_TEST_COVERAGE ok total=102 parallel=29 serial=63 herdr=10

FM_TEST_COVERAGE ok total=102 parallel=29 serial=63 herdr=10

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 2 issues found → auto-fixed ✅
  • 🚨 bin/fm-pr-merge.sh:102 - The requirement says, "After a successful GitHub PR merge, verify the recorded issue is closed." Reconciliation runs only after this script invokes gh-axi pr merge. A captain can merge through GitHub, which fm-pr-poll.sh detects without calling this script, leaving a forgotten Closes keyword and the issue open. Conversely, forwarded --auto or merge-queue behavior can return success before the PR is merged, causing premature issue closure. Move reconciliation to the shared actual-MERGED observation boundary and cover external/queued merges.
  • 🚨 tests/fm-brief.test.sh:187 - The required "briefs generated without it byte-identical" guarantee is not tested. The new plain-brief assertions only check that two issue-specific strings are absent, so any unconditional whitespace or wording drift would pass. Add an exact byte comparison or golden fixtures for existing no-issue brief variants.

🔧 Fix: Add exact no-issue brief golden coverage
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • bin/fm-test-run.sh tests/fm-brief.test.sh tests/fm-spawn-worktree-settle.test.sh tests/fm-pr-merge.test.sh
  • bin/fm-test-run.sh tests/fm-test-run.test.sh tests/fm-calm-pi-extension.test.sh
  • bin/fm-test-run.sh tests/fm-spawn-worktree-settle.test.sh after adding explicit-identity guards
  • bin/fm-test-run.sh tests/fm-pr-merge.test.sh after adding verification, re-verification, and invalid-metadata cases
  • bin/fm-test-run.sh tests/fm-brief.test.sh after correcting a test-only fixed-string assertion and rerunning
  • Final combined rerun: bin/fm-test-run.sh tests/fm-brief.test.sh tests/fm-spawn-worktree-settle.test.sh tests/fm-pr-merge.test.sh
  • LC_ALL=en_US.utf8 bin/fm-test-run.sh --check-coverage
  • Manual isolated lifecycle: fm-brief.sh ... --issue 42 -> fm-spawn.sh ... -> fm-pr-merge.sh .../pull/31 using fake terminal and GitHub endpoints, with persisted metadata and API calls captured
✅ **Document** - passed

✅ No issues found.

🔧 **Lint** - 1 issue found → auto-fixed ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix: Suppress literal backtick fixture ShellCheck warning
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

@HelloWorldSungin
HelloWorldSungin force-pushed the fm/fm-issue-lifecycle-wiring branch from 1bca513 to c6ba037 Compare July 31, 2026 16:53
@kunchenguid

kunchenguid commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch.

When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again.

Noted for firstmate#1271 at c6ba0374.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants