feat(bin): wake firstmate when the captain declines a PR by comment and close - #6
Merged
Merged
Conversation
…nt and close The captain declines work by leaving a comment on the pull request and closing it unmerged, not by filing a formal "request changes" review. Nothing read that signal, so the instruction only surfaced when someone thought to look. The existing per-task merge poll now also reports a GitHub pull request that is closed without merging and carries a comment authored by the account the home is authenticated to the forge as. That authenticated account is the identity source, so a review bot, a CI bot, or a pipeline comment can never be read as the captain speaking. The poll stays a pure read-only program: it reports the newest captain comment on every cycle, and the watcher's private <id>.pr-decline-seen record turns those repeats into one relay. The record is bound to the pull request as well as the comment, and is written only after the wake is durably queued, so an interruption costs a repeated relay rather than a dropped instruction. A damaged record fails the same direction. A decline is not terminal: nothing retires, the watch stays armed, and it still retires on the merge that follows the revisions. The new pr-decline-feedback skill owns the handling procedure and the authority boundary. Relaying the comment is autonomous because the comment plus the close is the decision; the ask-user contract for decisions the pipeline discovers itself is untouched. Decline detection is GitHub-only, matching fm-pr-merge.sh: reading a comment author needs identity output plain glab does not expose. Changing the poll template changes its hash, so polls armed by the previous release fail validation and the existing non-executing migration rebuilds them from recorded metadata at the next bootstrap.
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
1 participant
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.
Intent
The captain's real way of rejecting work on a pull request is to leave a plain GitHub comment and then close the PR unmerged - not GitHub's formal "Request changes" review. Nothing in firstmate read that signal, so his instruction only surfaced when somebody thought to look manually. He explicitly wants it picked up WITHOUT opening a firstmate session. Goal: make comment+close wake firstmate automatically, carrying the instruction, and have firstmate relay it to the same task worker and restart validation with no further approval prompt.
Deliberate decisions a reviewer reading only the diff would not know:
Reuse, not a new mechanism. The task explicitly required extending the existing per-task PR poll (bin/fm-pr-check.sh, bin/fm-pr-lib.sh, bin/fm-pr-poll.sh) and its byte-static template + validated sidecar + registration contract, rather than inventing a separate unregistered check path. Changing bin/fm-pr-poll.sh changes its template hash, which invalidates polls armed by the previous release; that is intentional and safe because bin/fm-pr-check-migrate.sh already rebuilds canonical polls from recorded metadata at the next bootstrap. I verified that upgrade path empirically against a poll armed with the pre-change template bytes: it was rebuilt and re-armed with metadata preserved.
Captain identity is the authenticated forge account (gh api user -q .login), deliberately NOT a new config file and not a hardcoded login. The task said to reuse an existing identity source; bootstrap already requires and validates GitHub auth and every forge action runs as that account. It is resolved fresh inside the poll rather than stored in the sidecar, specifically so the offline migration can still rebuild a poll from metadata alone and so there is no new tamperable field. gh 2.89 returns null for author.is_bot on pr view --json comments, so bot exclusion is by exact login match, not by that field - a bot such as github-actions can never equal the captain login. This is checked by test.
The poll stays a pure read-only program. It has no durable state and reports the newest captain comment on EVERY cycle; the watcher owns the durable dedup record (state/.pr-decline-seen). This was chosen over letting the poll write, because the poll runs under a kill/timeout wrapper where a partial write is possible.
Ordering is deliberately at-least-once, not at-most-once: the wake is durably queued FIRST and the seen record is written after. An interruption in between costs one repeated relay; the reverse order could silently drop the captain's instruction. A damaged, truncated, or wrong-mode seen record likewise reports "not delivered" and re-relays. Do not flag this as a dedup bug - losing the captain's word is the failure we are avoiding, and a repeat is loud and harmless. The record is bound to the PR URL as well as the comment id so re-arming a task on a different PR is never silenced by the old record.
The wake payload carries identifiers only ("declined "), never the comment body, matching the existing tiny check-output convention (merged, x-mention ) and the X-mode precedent of stashing payload out of the wake line. Firstmate fetches the body from the forge when it handles the wake. The comment id is charset- and length-validated in three places because it reaches a wake reason line.
A decline is deliberately NOT terminal: nothing retires, the poll stays armed, and it still retires normally on the merge that follows the revisions. Only an exact merged result retires, unchanged.
Decline detection is GitHub-only on purpose, matching bin/fm-pr-merge.sh which also serves GitHub alone. Reading a comment author needs identity output plain glab does not expose, and firstmate deliberately does not require a JSON processor. Documented in docs/gitlab-merge-watch.md rather than left implicit.
Relaying the comment is AUTONOMOUS - no captain approval, no yolo posture needed. That is the entire point of the request: the captain commenting and closing IS the decision. This deliberately does not touch the ask-user authority contract, which still owns every decision the worker's own pipeline discovers mid-run; the new skill states that boundary explicitly, and also carves out comment content that is out of scope or destructive/irreversible/security-sensitive as still needing the captain.
Documentation placement follows the repo's own firstmate-coding-guidelines skill (loaded before starting, as the task required): AGENTS.md gets only the load trigger and one word in the check-wake line, because a decline is a check: result and not a new wake kind; the full handling procedure lives in the new agent-only skill .agents/skills/pr-decline-feedback/SKILL.md, registered in docs/documentation-audiences.json. docs/supervision-protocols/ was deliberately left unchanged - those files document per-harness arm/wait mechanism, not per-wake-kind handling.
Verification already done locally: bin/fm-lint.sh clean with pinned ShellCheck 0.11.0; bin/fm-doc-audience-check.sh ok; 47 changed-selected test scripts pass with 0 failures, including 3 new tests covering closed-not-merged vs closed-merged, bot/pipeline-authored comments, comment-seen-before dedup, a newer comment re-waking, identity-bound and malformed delivery records, and hostile comment identifiers; plus teardown cleanup of the new sidecar folded into the existing teardown test.
What Changed
bin/fm-pr-poll.shnow also detects a decline: on a PR that is closed without being merged, it resolves the captain as the authenticated forge account (gh api user -q .login) and reportsdeclined <number> <comment-id>for the newest captain comment written within six hours of the close, so an incidental older comment cannot be relayed as the instruction. The poll stays read-only and non-terminal - only an exactmergedresult still retires it - and detection is GitHub-only, documented indocs/gitlab-merge-watch.md.bin/fm-watch.showns the durable dedup: helpers added inbin/fm-pr-lib.shparse the decline output and read/writestate/<id>.pr-decline-seen, bound to both the PR URL and the comment id. The wake is queued first and the record written after, so an interruption costs one repeated relay rather than a lost instruction; a damaged or wrong-mode record re-relays.bin/fm-teardown.shcleans up the new sidecar alongside the other poll artifacts..agents/skills/pr-decline-feedback/SKILL.md(registered indocs/documentation-audiences.json) describing the autonomous relay to the same task worker, with the load trigger wired intoAGENTS.mdanddocs/architecture.md;tests/fm-pr-check-security.test.shgains coverage for closed-not-merged vs merged, bot-authored comments, once-only delivery, identity-bound and malformed records, and teardown cleanup.Risk Assessment
✅ Low: The follow-up commit applies both accepted fixes exactly as directed - closing the only substantive gap from round 1 - without weakening any existing validation or the dedup contract, leaving only an informational test-fidelity note.
Testing
I ran the changed-selected PR/watcher suites (fm-pr-check-security plus teardown, watch-triage, wake-queue, documentation-audiences and pr-merge) and all passed with no failures. Because the suite's fake gh reimplements the six-hour comment window in bash rather than running the poll's real --json/-q filter, I additionally executed that filter through real jq against a GitHub-shaped payload and confirmed it drops a week-old comment, keeps in-window ones, picks the newest captain comment, and errors out (silent poll) on an unparseable closedAt. I then captured a full CLI transcript of the end-user flow with a fake gh serving real JSON through real jq: an open PR stays silent, the captain's comment plus an unmerged close produces
declined 42 IC_kwDOreview1, one watcher cycle queues that wake durably and writes the private 0600 delivery record, the next cycle stays silent while a control path still runs, a newer captain comment wakes again, the poll remains armed, and a later merge retires it normally. Finally I verified the upgrade path the diff alone cannot show: a poll armed with the previous release's template bytes is correctly invalidated by the new template hash and is rebuilt, re-armed and decline-capable after bin/fm-pr-check-migrate.sh, with sidecar metadata preserved. This change has no rendered UI surface - it is shell tooling whose user-visible output is the poll line and the watcher wake line, so CLI transcripts are the end-user artifact rather than screenshots. The one thing not machine-verifiable is that firstmate relays the fetched comment to the worker without an approval prompt, since that is agent-instruction behavior in the new skill file; its registration is covered by the doc-audience test.Evidence: End-to-end decline transcript (open -> comment+close -> wake -> dedup -> newer comment -> merge retires)
PR #42 is OPEN. Captain has commented, but has not closed it. poll output: [] <- silence: an open PR is not a decline Captain leaves the instruction and CLOSES the PR unmerged (github-actions also commented). poll output: [declined 42 IC_kwDOreview1] <- bot comment ignored, week-old comment ignored Watcher cycle -> firstmate is woken with the instruction, no session opened, no approval asked. stdout | check: .../state/task-a.check.sh: declined 42 IC_kwDOreview1 durable wake queue: 1785439805 1 check .../state/task-a.check.sh check: .../state/task-a.check.sh: declined 42 IC_kwDOreview1 delivery record task-a.pr-decline-seen (mode 600): fm-pr-decline-seen-v1 https://github.com/acme/widget/pull/42 IC_kwDOreview1 Next watcher cycle, same comment still on the closed PR -> no repeat wake. poll still says: [declined 42 IC_kwDOreview1] wake queue lines: 0 <- deduped by the delivery record Captain adds a NEWER comment -> that is a fresh instruction and wakes again. stdout | check: .../state/task-a.check.sh: declined 42 IC_kwDOreview2 durable wake queue: 1785439816 2 check .../state/task-a.check.sh check: .../state/task-a.check.sh: declined 42 IC_kwDOreview2 A decline is not terminal: the poll is still armed. poll artifacts still valid and armed: yes Crew reworks, captain merges -> the same poll retires normally. stdout | check: .../state/task-a.check.sh: merged poll artifacts after merge: removedEvidence: Upgrade path: pre-change poll invalidated then rebuilt by fm-pr-check-migrate.sh
A poll armed by the previous release (old template bytes): old template sha : e87aae384ec62ae28f78a0779020f52f7b891ba0cafea2551b5a2c4e967870cf new template sha : 7f10147dae9bf123736d08e0bce7835fa441217e41d4bb25a99bea1a88a097ee armed check sha : e87aae384ec62ae28f78a0779020f52f7b891ba0cafea2551b5a2c4e967870cf validates against the NEW template: NO - the release invalidated it, as intended Next bootstrap runs bin/fm-pr-check-migrate.sh ... PR_CHECK_MIGRATION: canonical polls rebuilt and armed; resume supervision for this home After migration: rebuilt check sha: 7f10147dae9bf123736d08e0bce7835fa441217e41d4bb25a99bea1a88a097ee re-armed and valid against the new template: yes metadata preserved in the sidecar: github https://github.com/acme/widget/pull/42 github.com acme/widget 42 The rebuilt poll detects the captain's decline: poll output: [declined 42 IC_kwDOreview1]Evidence: Poll's real jq filter run against a gh-shaped payload (real jq 1.8.1)
Evidence: Decline test results from tests/fm-pr-check-security.test.sh
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-pr-poll.sh:98- The decline signal is "CLOSED-not-merged AND any captain-authored comment exists" - there is no binding between the comment and the close. The poll selects the newest captain comment regardless of when it was written, and nothing compares it to the PR's closedAt. Concrete false positive: the captain leaves an incidental comment early on PR feat(spawn): add cb harness adapter for account-B crewmates kunchenguid/firstmate#40 ("looks good, merge once CI is green"), the work is later superseded and feat(spawn): add cb harness adapter for account-B crewmates kunchenguid/firstmate#40 is closed unmerged, the poll emitsdeclined 40 <old-comment-id>, and per .agents/skills/pr-decline-feedback/SKILL.md the relay is explicitly autonomous - firstmate steers the worker to redo the delivery path off a stale, unrelated comment with no approval prompt. The only guard is prose (skill step 2, "sanity-check the author"), which does not catch a genuine but stale captain comment.gh pr view --json closedAt,commentsalready exposes.comments[].createdAt, so requiring the selected comment to be at or after the close would cost no extra API call. Flagging rather than fixing because the commit message defines the signal this way deliberately.bin/fm-pr-poll.sh:98-gh pr view --json commentsfetches only the first 100 issue comments (gh's GraphQL query usescomments(first: 100)), oldest first. On a PR with more than 100 comments the captain's actual decline comment is outside the returned window, so the poll either stays silent or - worse, combined with the recency gap above - selects an old captain comment from the first 100 and relays it as the instruction. Worth a note in the poll's comment or a follow-up switch to a paginated/--jsonreversed query.bin/fm-pr-lib.sh:903-fm_pr_poll_retirement_recover_oneremoves the check, sidecar, registration, and receipt on merge, but not<id>.pr-decline-seen; only bin/fm-teardown.sh removes it. After a decline followed by the merge that retires the poll, the record lingers in state/ until teardown. Harmless for correctness (it is URL+comment bound so it can never silence a different PR or a different comment), but it does mean a retired task leaves a private artifact behind, and it makes validate_pr_poll_cleanup take the strict-validation path for an id whose poll artifacts are all gone.bin/fm-pr-poll.sh:90- A decline is deliberately non-terminal, so a PR that stays closed keeps the poll armed forever and each cycle now issues two extra GitHub API calls (gh api userplus the comments query) per closed task, indefinitely and even after the comment has already been delivered. Bounded and intentional given the non-terminal design, but on a home with several long-closed PR tasks and a short CHECK_INTERVAL this is steady, permanent API traffic that previously did not exist.🔧 Fix: bind relayed decline comment to the PR close
1 info still open:
tests/fm-pr-check-security.test.sh:83- The fake gh implements the six-hour window itself ([ "${offset:-0}" -ge -21600 ]) and prints the already-filtered<login>\t<id>lines, so the suite exercises the shell-side selection but never the real jq filter in bin/fm-pr-poll.sh:121. A typo in that expression - a wrong field name, a misplaced paren,fromdatevsfromdateiso8601- would leave every test green while gh exits non-zero in production, and because the poll is fail-silent by design the whole decline feature would simply stop firing with no error anywhere. Inherent to a fake-forge harness and the author states the query was verified manually against gh 2.89, so noting it rather than asking for a change; a cheap guard if it ever matters is asserting the exact--json/-qargument string recorded in FM_TEST_GH_LOG.✅ **Test** - passed
✅ No issues found.
bash bin/fm-test-run.sh tests/fm-pr-check-security.test.sh- full PR-poll/migration/teardown suite, exit 0, including the 3 new decline tests (test_declined_pull_request_detection,test_declined_comment_delivers_exactly_once,test_decline_delivery_record_is_identity_bound) and the extendedtest_teardown_removes_poll_artifactsbash bin/fm-test-run.sh tests/fm-teardown.test.sh tests/fm-watch-triage.test.sh tests/fm-wake-queue.test.sh tests/fm-documentation-audiences.test.sh tests/fm-pr-merge.test.sh- adjacent suites for the watcher, wake queue, teardown and doc-audience registration, exit 0Ran the poll's real query filter(.closedAt|fromdateiso8601) as $c | .comments[] | select((.createdAt|fromdateiso8601) >= ($c - 21600)) | [.author.login, .id] | @tsvthrough real jq 1.8.1 against agh pr view --json closedAt,comments-shaped payload (the suite's fake gh emulates this window in bash and never executes the filter): stale comment dropped, in-window comments kept, newest captain comment selected, null closedAt exits non-zeroManual end-to-end transcriptbash decline-e2e.sh- armed a canonical poll viafm_pr_poll_prepare/fm_pr_poll_publish_preparedwith a fakeghbacked by real JSON + real jq, then drove OPEN -> captain comment + CLOSED unmerged ->bin/fm-watch.shcycle -> repeat cycle -> newer comment -> merge, inspecting.wake-queue,task-a.pr-decline-seen(mode 600) and poll artifact state at each stepManual upgrade-path checkbash upgrade-migration.sh- armed a poll with the base-commitbin/fm-pr-poll.shbytes, confirmed it failsfm_pr_poll_artifacts_validagainst the new template, ranbin/fm-pr-check-migrate.sh, confirmed the rebuilt poll matches the new template sha with sidecar metadata preserved and then detects a decline✅ **Document** - passed
✅ No issues found.
🔧 Fix: suppress SC2016 for jq program in fm-pr-poll.sh
1 warning still open:
✅ **Push** - passed
✅ No issues found.