Skip to content

feat(fork): surface PR comments and reviews as check wakes#15

Merged
Bre77 merged 2 commits into
mainfrom
fm/pr-comment-watch-d9
Jul 11, 2026
Merged

feat(fork): surface PR comments and reviews as check wakes#15
Bre77 merged 2 commits into
mainfrom
fm/pr-comment-watch-d9

Conversation

@Bre77

@Bre77 Bre77 commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Intent

  • Make PR comments and reviews a first-class input channel, not just merge state:
    • Captain feedback left directly on a fleet PR previously only reached firstmate via chat relay; now the tracked-PR poll itself wakes on it.
    • A secondmate's upstream PRs (e.g. the hass secondmate's Home Assistant PRs) get maintainer review feedback surfaced the same way, since secondmate homes run this same bin/.
    • One generic mechanism covers both: no special-casing of captains or PR authors - every new comment/review from anyone surfaces, and the supervising agent triages.
  • Approach: extend bin/fm-pr-check.sh's generated state/<id>.check.sh to poll for new PR activity alongside merge state.
    • New bin/fm-pr-activity-poll.sh queries issue comments, inline review comments, and review summaries via gh api, filtered against a durable per-task watermark (state/<id>.pr-activity-seen).
    • Watermark seeds to "now" on first arm so a task's pre-existing PR history never floods the first wake; each poll advances it to the newest surfaced item.
    • Merge detection still takes precedence in the same poll: once merged, the shim reports merged and skips the activity poll entirely (no wasted gh api calls on a landed task).
    • Re-running fm-pr-check.sh regenerates check.sh so an existing task upgrades to the activity poll on next arm; a not-yet-re-armed legacy merge-only check.sh keeps working untouched.
  • Scope boundaries:
    • No triage logic lives here - the wake line (pr-comment <task-id> <author> (<kind>): <text>) is deliberately dumb; AGENTS.md section 8 already routes any check: wake (including this one) through firstmate's normal triage (act, dispatch an amendment, relay to the captain, or ignore bot noise).
    • Reviews endpoint has no server-side since support, so it's fetched in full and filtered client-side by submitted_at; acceptable at PR scale, not paginated defensively beyond --paginate.

What Changed

  • bin/fm-pr-activity-poll.sh (new): the poll body, run by the generated check.sh once a tracked PR is confirmed not merged.
  • bin/fm-pr-check.sh: creates the watermark on first arm (preserved across re-arms), and generates a check.sh shim that runs the merge check first, then execs the activity poll only when the PR is still open.
  • tests/fm-pr-check.test.sh (new): 9 cases covering watermark seeding/preservation, cross-kind activity surfacing and formatting, truncation, merge precedence, legacy-script compatibility/upgrade, defensive watermark init, and a regression test for a real bug caught during live verification (see below).
  • AGENTS.md section 8: one line noting pr-comment wakes exist and are triaged like any other check wake.
  • docs/scripts.md: registers the new script and updates fm-pr-check.sh's description.

Risk Assessment

Low - additive only. A task that never re-arms keeps its old merge-only check.sh untouched. Once armed, the extra gh api calls are read-only (three per poll, on the existing CHECK_INTERVAL cadence) and are skipped entirely once a PR is merged.

Testing

  • shellcheck bin/*.sh bin/backends/*.sh tests/*.sh - clean.
  • bash tests/fm-pr-check.test.sh - 9/9 pass.
  • bash tests/fm-pr-merge.test.sh - 10/10 pass (no regression; it calls fm-pr-check.sh internally).

Live verification against a real fork PR

Ran against this fork's own PR #13 (merged), using a scratch state dir so nothing outside the worktree was touched:

$ FM_ROOT_OVERRIDE="$PWD" FM_STATE_OVERRIDE="$TMP/state" \
    ./bin/fm-pr-activity-poll.sh e11-verify https://github.com/Bre77/firstmate/pull/13
(silent - first arm, watermark seeded to now)

$ cat "$TMP/state/e11-verify.pr-activity-seen"
2026-07-11T10:37:51Z

$ gh-axi pr comment 13 --repo Bre77/firstmate --body "Automated verification comment for E11.1 ..."
commented: number: 13, status: ok

$ FM_ROOT_OVERRIDE="$PWD" FM_STATE_OVERRIDE="$TMP/state" \
    ./bin/fm-pr-activity-poll.sh e11-verify https://github.com/Bre77/firstmate/pull/13
pr-comment e11-verify Bre77 (comment): Automated verification comment for E11.1 (PR-comment wake feature). Safe to ignore/delete; posted from a crewmate testin

$ cat "$TMP/state/e11-verify.pr-activity-seen"
2026-07-11T10:37:56Z

# third run: confirms no re-surfacing / no duplicate wake
$ FM_ROOT_OVERRIDE="$PWD" FM_STATE_OVERRIDE="$TMP/state" \
    ./bin/fm-pr-activity-poll.sh e11-verify https://github.com/Bre77/firstmate/pull/13
(silent, watermark unchanged)

The test comment was deleted afterward (gh api repos/Bre77/firstmate/issues/comments/4945027834 --method DELETE) to leave the PR clean.

This live run caught a real bug the mocked unit tests could not: gh api silently switches from GET to POST whenever -f field parameters are present unless --method is pinned explicitly. The initial version of fm-pr-activity-poll.sh passed -f since=<watermark> without --method GET, so instead of filtering existing comments it attempted to create a new issue comment and failed with a 422 ("body" wasn't supplied). Fixed by adding --method GET to all three gh api calls; re-verified live per the transcript above.

Full narrative / original brief

[E11.1] Make PR comments a first-class input channel: every firstmate home's tracked-PR polls must also surface NEW comments and reviews - from ANYONE - as wakes, not just merge state.

Captain's intent (two consumers, one mechanism):

  1. The captain comments on a fleet PR -> the owning firstmate wakes, reads it, and acts (feedback-on-the-artifact instead of chat relay).
  2. The hass secondmate's upstream Home Assistant PRs get review feedback from maintainers/others -> its own polls surface those the same way. Because secondmate homes run this same bin/, ONE generic implementation covers both - do not special-case captains or authors; surface every new comment/review and let the supervising agent triage.

Bre77 added 2 commits July 11, 2026 20:37
fm-pr-check.sh's generated per-task check.sh now polls tracked PRs for new
issue comments, inline review comments, and review summaries from anyone,
not just merge state - so a captain's PR comment or a secondmate's upstream
review feedback wakes the owning firstmate the same way a merge does.

Tracked via a durable per-task watermark (state/<id>.pr-activity-seen),
seeded to "now" on first arm so history never floods the wake. Merge
detection still takes precedence and short-circuits the activity poll once
a PR lands, and re-arming an existing task upgrades a legacy merge-only
check.sh without resetting its watermark.
gh api silently switches from GET to POST whenever -f field parameters are
present unless --method is pinned explicitly. Live verification against a
real fork PR caught this: the since= filter was being sent as a POST body,
so gh attempted to create a new comment instead of listing existing ones,
failing with a 422.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 052dfaaea2

ℹ️ 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".

Comment thread bin/fm-pr-check.sh

mkdir -p "$STATE"
ACTIVITY_SEEN="$STATE/$ID.pr-activity-seen"
[ -f "$ACTIVITY_SEEN" ] || date -u +%Y-%m-%dT%H:%M:%SZ > "$ACTIVITY_SEEN"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve same-second PR activity

Captain, this seeds the activity watermark to the current UTC second, but the new activity poll only surfaces items with created_at/submitted_at strictly greater than that watermark. When a captain or maintainer posts a comment/review in the same second that the PR is armed, GitHub gives it an equal timestamp, so every later poll filters it out forever; store a tie-breaker/seen IDs or make equality safe with de-duping.

Useful? React with 👍 / 👎.

@Bre77 Bre77 merged commit bd11026 into main Jul 11, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant