Skip to content

feat(bin): wake firstmate on new PR review activity, not merge alone - #1903

Open
ivannovak wants to merge 3 commits into
kunchenguid:mainfrom
ivannovak:fm/fm-pr-review-activity
Open

feat(bin): wake firstmate on new PR review activity, not merge alone#1903
ivannovak wants to merge 3 commits into
kunchenguid:mainfrom
ivannovak:fm/fm-pr-review-activity

Conversation

@ivannovak

Copy link
Copy Markdown

Intent

Extend firstmate's PR polling so a task's PR also wakes firstmate on new review activity - new reviews, new review comments, and new issue comments on the PR - not just on merge.

WHY: firstmate arms a per-task poll when a PR is opened, but it watches for merge only. It emits one 'merged' line and is silent otherwise, so nothing notices when a human reviewer comments on or reviews one of our PRs. This cost a real day: reviewer numeralsix submitted a review on auctic-core PR 6104 at 2026-08-06T16:16:23Z containing a blocking finding (the shipped feature was inert for its primary use case). Firstmate did not learn about it for roughly 21 hours, and then only because a human happened to ask. There are 20+ open PRs in this state, so this is a repeated need, not a one-off.

NON-NEGOTIABLE PROPERTIES PRESERVED (existing security properties of this subsystem; none may be weakened):

  1. The poll program stays byte-identical for every task. Task and PR data live only in the private validated sidecar and are NEVER interpolated into shell source.
  2. Silence on every error. A failed lookup, a network error, a changed output format, or a missing tool must produce no output. A wake must never be manufacturable from a failure.
  3. The check must finish within FM_CHECK_TIMEOUT.
  4. Poll bytes stay bound through the existing registration path; an unregistered or drifted check must not execute.
  5. The existing 'merged' signal must keep working exactly as it does today.

DESIGN DECISION, RESOLVED BY VERIFICATION RATHER THAN ASSUMPTION (this was an explicit requirement of the task): the task named two candidate shapes for avoiding a re-wake forever on the same review comment - (a) emit a line carrying the newest activity identifier and rely on the watcher's existing repeat-output suppression, or (b) maintain an explicit cursor. I was required to read bin/fm-watch.sh and prove which holds rather than assume. Candidate (a) is FALSE and I established that by reading the code: fm-watch.sh:43 documents check output as 'always actionable', fm_wake_append in bin/fm-wake-lib.sh appends unconditionally with no dedupe, and wake() in bin/fm-push-transition-lib.sh prints and exits. There is no repeat-output suppression for checks anywhere. The existing 'merged' line only avoids re-firing because it is terminal and retires its own poll. So shape (b) was required, and shipping (a) would have re-woken every poll cycle - a notification that cries wolf every cycle is worse than none.

WHERE THE CURSOR LIVES, AND WHY IT IS NOT IN THE POLL: the watcher invokes the poll as fm-pr-poll.sh --validated , which deliberately carries NO task identity, and a check must never write task state. A cursor kept inside the poll could therefore neither be addressed nor trusted. So the poll stays a stateless reporter of current activity and bin/fm-pr-lib.sh owns the per-task cursor, consumed by the watcher which already knows the task id and state dir. The cursor is committed only AFTER the wake is durably queued, so a watcher that dies between the two repeats a wake rather than swallowing one.

SECOND DESIGN QUESTION, ALSO VERIFIED RATHER THAN ASSUMED: whether review activity should extend the existing per-task PR poll or be a second registered check. I was required to verify that a task can only hold one state/.check.sh before committing to the obvious reasoning. Confirmed: the watcher globs state/*.check.sh and derives the task id from the basename, and fm-check-register.sh binds the single fixed path state/.check.sh, so a second check would need a second id namespace that teardown and metadata lookups would treat as a separate task. Extending the existing poll was the only correct option.

WHAT WAS BUILT:

  • bin/fm-pr-poll.sh reports a new line 'review-activity reviews= comments= latest=' for GitHub, as a stateless observation of right now. It is read in its OWN gh call, separate from the merge state call, deliberately so a change to the activity shape can never turn a working merge lookup silent. A merged result is reported ALONE and returns immediately, so the existing exact-match merge retirement is untouched.
  • Only validated counts and one validated instant reach the reported line. No string the PR's authors control (no login, no comment body, no title) ever reaches a wake line firstmate reads - this is a deliberate prompt-injection boundary. Firstmate reads the PR itself for content.
  • The marker is count-plus-instant on purpose: the instant alone would miss two comments landing in the same second, and the count alone would miss activity that replaces rather than adds.
  • bin/fm-pr-check.sh seeds the cursor at arming by running the poll itself (so the gh query has exactly one owner), meaning only activity arriving AFTER arming wakes firstmate rather than every bot comment a PR already carries when its checks go green. When it cannot establish a starting point it CLEARS the cursor rather than inheriting it, because one extra wake is always preferable to a stale cursor suppressing a real one.
  • bin/fm-teardown.sh removes the cursor with the task's other volatile per-task markers.

SCOPE EXCLUSION, EXPLICITLY ALLOWED AND EXPLICITLY DECLARED: the task said GitLab is also supported by the current poll, and that if extending review activity to GitLab is disproportionate I should scope this to GitHub and say so explicitly rather than silently leaving a half-covered surface. I scoped it to GitHub. plain glab has no field selector and firstmate does not require a JSON processor - the same constraint already recorded in docs/gitlab-merge-watch.md that leaves a GitLab task without a recorded pr_head. GitLab keeps merge state only, and this is stated in the poll header and the verification record rather than left silent. This is an accepted exclusion, not an oversight.

OTHER CONSTRAINTS HONORED: no webhook receiver, daemon, or service requiring a public endpoint - polling goes through the existing check machinery and the already-authenticated GitHub CLI. No change to merge behaviour, merge authority, or when firstmate may merge.

A DELIBERATE DELETION A REVIEWER MIGHT FLAG AS A GAP: while watching each guard fail, I found that a field-count reconstruction check I had written ([ "$activity" = "$reviews $comments $latest" ]) could reject NOTHING that the per-field digit and ISO-instant patterns did not already reject - proven across 23 adversarial inputs with zero divergence between builds with and without it. I removed it rather than ship a guard that can never fire or write a test that can never fail, and left a comment stating why there is deliberately no such check. The remaining per-field patterns fully constrain the shape because 'latest' is everything past the second space, so any missing, extra, padded, tab-joined or multi-line result leaves it holding a space or newline that no instant pattern matches.

VERIFICATION PERFORMED (this repo requires watching a check fail before trusting it): 12 colocated tests in tests/fm-pr-review-activity.test.sh, plus 13 separate mutations each run unpiped, restored, and re-confirmed green. Two mutations initially came back as inert controls and both were resolved honestly rather than papered over - one led to the deletion above, and the other revealed my own test used a task id ('../escape') that the cursor path template neuters into a harmless '...' component; the guard is real and './../escape' writes two directories outside the state dir without it, so the test was corrected to exercise the actual escape and watched going red. The lint instrument itself was verified by introducing a deliberate SC2086 and watching exit 1, then restoring to exit 0. The one-wake-then-silence property was also proven end to end on a real upgraded home: sweep 1 wakes once, sweeps 2 and 3 exit with no output.

UPGRADE PATH VERIFIED: changing the poll bytes invalidates already-armed polls (fm_pr_poll_artifacts_valid does cmp against the current template). I verified empirically that the existing non-executing migration rebuilds a poll armed with the PREVIOUS template onto the new one, so the 20+ open PRs are not stranded. After upgrade each such PR produces exactly ONE review-activity wake reporting its current totals and then goes quiet, which is the intended behavior since those PRs have unreviewed activity right now.

docs/verification/pr-review-activity.md is a maintainer-verification record capturing the forge-CLI facts this depends on by running the real gh - that the fields exist, that they carry those instants, and that GitHub creates a review row for every inline review comment INCLUDING a reply in an existing review thread (verified against real PRs), which is why counting reviews covers the inline surface without a third API call. The colocated tests stub gh and so cannot notice a real field going away, which is why that record exists and names itself as the thing to refresh after a gh upgrade.

KNOWN PRE-EXISTING FAILURE, NOT INTRODUCED BY THIS CHANGE: tests/fm-teardown.test.sh fails on 'herdr-preflight-missing-adapter: the retryable pre-return refusal was not explained visibly'. I verified it fails identically on the base commit with this work stashed. It is a Herdr-adapter environment issue in this worktree, unrelated to this change.

What Changed

  • bin/fm-pr-poll.sh now reports a review-activity reviews=<n> comments=<n> latest=<iso8601> line for GitHub pull requests, read in its own gh call so a change to that shape can never silence the merge lookup. Only validated counts and one validated instant reach the line — no login, body, or title — and comments the authenticated account authored are dropped via viewerDidAuthor. merged is still reported alone and returns immediately, and GitLab keeps merge state only.
  • Repeat suppression lives outside the poll, since a check is invoked with no task identity and must not write task state: bin/fm-pr-lib.sh gains a per-task state/.<id>.pr-activity-cursor with filter/commit/clear helpers, bin/fm-watch.sh filters poll output through it and commits only after the wake is durably queued, and a review-activity line continues the sweep (its wake queued and reported after the loop) instead of exiting, so a merge on a later poll is still observed the same cycle. bin/fm-pr-check.sh seeds the cursor at arming from the poll itself — GitHub only, no forge call added on the GitLab path — and clears rather than inherits when it cannot establish a starting point; bin/fm-teardown.sh removes it with the task's other volatile markers.
  • Adds tests/fm-pr-review-activity.test.sh (registered in the pr-forge family in bin/fm-test-run.sh) and docs/verification/pr-review-activity.md, a maintainer-verification record of the real-gh facts the projection depends on; AGENTS.md, docs/architecture.md, docs/configuration.md, and docs/scripts.md re-describe the poll as a PR poll covering merge plus GitHub review activity.

Risk Assessment

✅ Low: All four prior findings were fixed exactly as instructed with narrowly scoped changes that I verified against the real gh CLI and traced end-to-end through the wake queue's dedupe and drain path, every stated non-negotiable still holds, and the only outstanding items are informational.

Testing

I ran the colocated suite (tests/fm-pr-review-activity.test.sh, 16 green) plus the directly-affected neighbours — pr-merge, pr-check-security, watch-checkpoint, watch-triage and wake-queue — all green, and then proved the user intent at the product surface rather than by test counts: a scripted end-to-end transcript drives the real arming, watcher sweeps, wake-queue drain and teardown against a stubbed gh replaying the incident named in the intent, showing one wake for a reviewer's review, silence on repeats, silence for firstmate's own comment and for a failed forge read, the unchanged merged line, GitLab merge-only, and cursor removal at teardown. I verified that transcript is a live instrument by running it unchanged against the base commit, where the blocking review produces no wake at all and nothing is ever queued — the exact 21-hour silence the change exists to fix — and by mutating the cursor filter and the teardown cleanup and watching both the transcript and the suite go red before restoring to green with a clean worktree. I also exercised the upgrade path for already-armed polls and the two-PRs-in-one-sweep case. The one failure I hit, tests/fm-teardown.test.sh's herdr-preflight-missing-adapter assertion, reproduces identically on a base-commit tree and is pre-existing.

Evidence: End-to-end CLI transcript: reviewer activity wakes firstmate once, repeats stay silent, merge still works

1. The PR already carries bot chatter when its checks go green. $ bin/fm-pr-check.sh auctic-core-6104 https://github.com/auctic-software/auctic-core/pull/6104&#10;armed: state/auctic-core-6104.check.sh cursor seeded at arming: review-activity reviews=0 comments=3 latest=2026-08-05T12:15:01Z 2. Nothing new since arming. The three bot comments must not wake anyone. (watcher swept every armed poll and stayed silent - no wake) 3. Reviewer numeralsix submits a blocking review at 2026-08-06T16:16:23Z. WATCHER WAKE -> check: .../auctic-core-6104.check.sh: review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z 4. Two more sweeps with the review still unanswered: silence, not cry-wolf. (watcher swept every armed poll and stayed silent - no wake) (watcher swept every armed poll and stayed silent - no wake) 5. A human leaves a follow-up issue comment on the PR. WATCHER WAKE -> check: .../auctic-core-6104.check.sh: review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z 6. firstmate replies on the PR itself. Its own comment is not news. (watcher swept every armed poll and stayed silent - no wake) 7. What firstmate actually reads - the durable wake queue, drained. $ bin/fm-wake-drain.sh check: auctic-core-6104.check.sh: review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z 8. The forge goes wrong: the activity read fails outright. (watcher swept every armed poll and stayed silent - no wake) cursor after the failed read (unchanged, so no wake was swallowed): review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z 9. The PR is merged. The pre-existing terminal signal is untouched. WATCHER WAKE -> check: .../auctic-core-6104.check.sh: merged 10. GitLab is merge-state only, as the poll header declares. poll output for https://gitlab.com/g/p/-/merge_requests/7: (nothing: the merge request is open and GitLab reports no review activity) 11. Teardown removes the cursor with the task's other volatile markers. before teardown: .auctic-core-6104.pr-activity-cursor $ bin/fm-teardown.sh auctic-core-6104 --force teardown exit: 0 after teardown: no review-activity cursor remains

=== firstmate PR review-activity wake: end-to-end ===
firstmate checkout : /Users/ivannovak/.no-mistakes/worktrees/e7be5960b363/01KZEB73NYSXMKYABRKMNWR21C
commit             : 1ed415b
throwaway FM_HOME  : /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.qxm9IR/home
pull request       : https://github.com/auctic-software/auctic-core/pull/6104

1. The PR already carries bot chatter when its checks go green.
$ bin/fm-pr-check.sh auctic-core-6104 https://github.com/auctic-software/auctic-core/pull/6104
  ●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  ●  WATCHER DOWN - SUPERVISION IS OFF
  ●  1 task(s) in flight, but no watcher has a fresh beacon (last beat: never, grace 300s).
  ●  Trust the emitted supervision protocol for this harness; do not use shell & for watcher repair.
  ●  This is a supervision warning only; the guarded operation WILL still run.
  ●  watcher supervision needs Stop-owned automatic recovery; inspect the hook registration and startup status before ending the turn.
  ●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  armed: state/auctic-core-6104.check.sh
  cursor seeded at arming: review-activity reviews=0 comments=3 latest=2026-08-05T12:15:01Z

2. Nothing new since arming. The three bot comments must not wake anyone.
  (watcher swept every armed poll and stayed silent - no wake)

3. Reviewer numeralsix submits a blocking review at 2026-08-06T16:16:23Z.
  WATCHER WAKE -> check: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.qxm9IR/home/state/auctic-core-6104.check.sh: review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z

4. Two more sweeps with the review still unanswered: silence, not cry-wolf.
  (watcher swept every armed poll and stayed silent - no wake)
  (watcher swept every armed poll and stayed silent - no wake)

5. A human leaves a follow-up issue comment on the PR.
  WATCHER WAKE -> check: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.qxm9IR/home/state/auctic-core-6104.check.sh: review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z

6. firstmate replies on the PR itself. Its own comment is not news.
  (watcher swept every armed poll and stayed silent - no wake)

7. What firstmate actually reads - the durable wake queue, drained.
  durable rows queued so far (epoch/seq/kind/key/payload):
  | check: auctic-core-6104.check.sh: review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z
  | check: auctic-core-6104.check.sh: review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z
$ bin/fm-wake-drain.sh
  check: auctic-core-6104.check.sh: review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z
  (the drain keeps the newest row per key, which is the current activity total)

8. The forge goes wrong: the activity read fails outright.
  (watcher swept every armed poll and stayed silent - no wake)
  cursor after the failed read (unchanged, so no wake was swallowed):
  review-activity reviews=1 comments=4 latest=2026-08-07T09:30:00Z

9. The PR is merged. The pre-existing terminal signal is untouched.
  WATCHER WAKE -> check: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.qxm9IR/home/state/auctic-core-6104.check.sh: merged

10. GitLab is merge-state only, as the poll header declares.
  poll output for https://gitlab.com/g/p/-/merge_requests/7: (nothing: the merge request is open and GitLab reports no review activity)

11. Teardown removes the cursor with the task's other volatile markers.
  before teardown: .auctic-core-6104.pr-activity-cursor
$ bin/fm-teardown.sh auctic-core-6104 --force
  teardown exit: 0
  | teardown auctic-core-6104 complete (window firstmate:fm-auctic-core-6104, worktree /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.qxm9IR/wt)
  | Backlog: auctic-core-6104 just finished. Update data/backlog.md - move auctic-core-6104 to Done, keep Done to the 10 most recent, then re-scan Queued and dispatch only work whose blockers are gone and date is due.
  ! WARNING: queued wakes pending - drain them with bin/fm-wake-drain.sh before anything else.
  after teardown:  no review-activity cursor remains

=== end ===
Evidence: Control: the same transcript at base commit 70aeba8 — the blocking review is silently missed

1. The PR already carries bot chatter when its checks go green. armed: state/auctic-core-6104.check.sh cat: .../state/.auctic-core-6104.pr-activity-cursor: No such file or directory cursor seeded at arming: 3. Reviewer numeralsix submits a blocking review at 2026-08-06T16:16:23Z. (watcher swept every armed poll and stayed silent - no wake) 5. A human leaves a follow-up issue comment on the PR. (watcher swept every armed poll and stayed silent - no wake) 7. What firstmate actually reads - the durable wake queue, drained. cut: .../state/.wake-queue: No such file or directory 9. The PR is merged. The pre-existing terminal signal is untouched. WATCHER WAKE -> check: .../auctic-core-6104.check.sh: merged

=== firstmate PR review-activity wake: end-to-end ===
firstmate checkout : /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-base-70aeba8.cE5dGj
commit             : b0ca99c
throwaway FM_HOME  : /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.QtiHjO/home
pull request       : https://github.com/auctic-software/auctic-core/pull/6104

1. The PR already carries bot chatter when its checks go green.
$ bin/fm-pr-check.sh auctic-core-6104 https://github.com/auctic-software/auctic-core/pull/6104
  ●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  ●  WATCHER DOWN - SUPERVISION IS OFF
  ●  1 task(s) in flight, but no watcher has a fresh beacon (last beat: never, grace 300s).
  ●  Trust the emitted supervision protocol for this harness; do not use shell & for watcher repair.
  ●  This is a supervision warning only; the guarded operation WILL still run.
  ●  watcher supervision needs Stop-owned automatic recovery; inspect the hook registration and startup status before ending the turn.
  ●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  armed: state/auctic-core-6104.check.sh
cat: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.QtiHjO/home/state/.auctic-core-6104.pr-activity-cursor: No such file or directory
  cursor seeded at arming: 

2. Nothing new since arming. The three bot comments must not wake anyone.
  (watcher swept every armed poll and stayed silent - no wake)

3. Reviewer numeralsix submits a blocking review at 2026-08-06T16:16:23Z.
  (watcher swept every armed poll and stayed silent - no wake)

4. Two more sweeps with the review still unanswered: silence, not cry-wolf.
  (watcher swept every armed poll and stayed silent - no wake)
  (watcher swept every armed poll and stayed silent - no wake)

5. A human leaves a follow-up issue comment on the PR.
  (watcher swept every armed poll and stayed silent - no wake)

6. firstmate replies on the PR itself. Its own comment is not news.
  (watcher swept every armed poll and stayed silent - no wake)

7. What firstmate actually reads - the durable wake queue, drained.
  durable rows queued so far (epoch/seq/kind/key/payload):
cut: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.QtiHjO/home/state/.wake-queue: No such file or directory
$ bin/fm-wake-drain.sh
  (the drain keeps the newest row per key, which is the current activity total)

8. The forge goes wrong: the activity read fails outright.
  (watcher swept every armed poll and stayed silent - no wake)
cat: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.QtiHjO/home/state/.auctic-core-6104.pr-activity-cursor: No such file or directory
  cursor after the failed read (unchanged, so no wake was swallowed):
  

9. The PR is merged. The pre-existing terminal signal is untouched.
  WATCHER WAKE -> check: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.QtiHjO/home/state/auctic-core-6104.check.sh: merged

10. GitLab is merge-state only, as the poll header declares.
  poll output for https://gitlab.com/g/p/-/merge_requests/7: (nothing: the merge request is open and GitLab reports no review activity)

11. Teardown removes the cursor with the task's other volatile markers.
  before teardown: 
$ bin/fm-teardown.sh auctic-core-6104 --force
  teardown exit: 0
  | teardown auctic-core-6104 complete (window firstmate:fm-auctic-core-6104, worktree /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-review-activity-e2e.QtiHjO/wt)
  | Backlog: auctic-core-6104 just finished. Update data/backlog.md - move auctic-core-6104 to Done, keep Done to the 10 most recent, then re-scan Queued and dispatch only work whose blockers are gone and date is due.
  ! WARNING: queued wakes pending - drain them with bin/fm-wake-drain.sh before anything else.
  after teardown:  no review-activity cursor remains

=== end ===
Evidence: Upgrade path: a PR armed before the change, watched after it

1. Arm with the OLD firstmate (merge-watch only). armed: state/auctic-core-6104.check.sh poll matches OLD template : yes poll matches NEW template : no review-activity cursor : none (the old firstmate has no such thing) 2. Upgrade. The new watcher runs its own non-executing migration first. WATCHER WAKE -> PR_CHECK_MIGRATION: canonical polls rebuilt and armed; resume supervision for this home check: .../auctic-core-6104.check.sh: review-activity reviews=1 comments=2 latest=2026-08-06T16:16:23Z poll matches NEW template : yes cursor now : review-activity reviews=1 comments=2 latest=2026-08-06T16:16:23Z 3. Two more sweeps: the already-reported backlog does not re-wake. (upgraded watcher swept the migrated poll and stayed silent - no wake) (upgraded watcher swept the migrated poll and stayed silent - no wake) 4. A NEW review lands on that upgraded PR. WATCHER WAKE -> auctic-core-6104.check.sh: review-activity reviews=2 comments=2 latest=2026-08-07T14:02:00Z (upgraded watcher swept the migrated poll and stayed silent - no wake)

=== upgrade path: a PR armed BEFORE the change, watched AFTER it ===
armed with  : /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-base-70aeba8.cE5dGj
watched with: /Users/ivannovak/.no-mistakes/worktrees/e7be5960b363/01KZEB73NYSXMKYABRKMNWR21C (1ed415b)

1. Arm with the OLD firstmate (merge-watch only).
  armed: state/auctic-core-6104.check.sh
  poll matches OLD template : yes
  poll matches NEW template : no
  review-activity cursor    : none (the old firstmate has no such thing)

2. Upgrade. The new watcher runs its own non-executing migration first.
  WATCHER WAKE -> PR_CHECK_MIGRATION: canonical polls rebuilt and armed; resume supervision for this home
check: /var/folders/pq/43bb80gj31x7009tmpvfjcy40000gn/T//fm-upgrade-e2e.afdP2O/home/state/auctic-core-6104.check.sh: review-activity reviews=1 comments=2 latest=2026-08-06T16:16:23Z
  poll matches NEW template : yes
  cursor now                : review-activity reviews=1 comments=2 latest=2026-08-06T16:16:23Z

3. Two more sweeps: the already-reported backlog does not re-wake.
  (upgraded watcher swept the migrated poll and stayed silent - no wake)
  (upgraded watcher swept the migrated poll and stayed silent - no wake)

4. A NEW review lands on that upgraded PR.
  WATCHER WAKE -> auctic-core-6104.check.sh: review-activity reviews=2 comments=2 latest=2026-08-07T14:02:00Z
  (upgraded watcher swept the migrated poll and stayed silent - no wake)

=== end ===
Evidence: Two PRs with activity in the same sweep both reach firstmate

1. A reviewer touches BOTH pull requests before the next sweep. WATCHER PRINTED -> check: core-6104.check.sh: review-activity reviews=1 comments=0 latest=2026-08-06T16:16:23Z 2. Both wakes durably queued for firstmate to read: | check: core-6104.check.sh: review-activity reviews=1 comments=0 latest=2026-08-06T16:16:23Z | check: core-6110.check.sh: review-activity reviews=0 comments=1 latest=2026-08-06T17:01:00Z 3. bin/fm-wake-drain.sh - what firstmate is handed: check: core-6104.check.sh: review-activity reviews=1 comments=0 latest=2026-08-06T16:16:23Z check: core-6110.check.sh: review-activity reviews=0 comments=1 latest=2026-08-06T17:01:00Z 4. Next sweep, nothing new on either: silence. (no wake)

=== two PRs, review activity in the same sweep ===
armed: 2 polls, both quiet at arming

1. A reviewer touches BOTH pull requests before the next sweep.
  WATCHER PRINTED -> check: core-6104.check.sh: review-activity reviews=1 comments=0 latest=2026-08-06T16:16:23Z

2. Both wakes durably queued for firstmate to read:
  | check: core-6104.check.sh: review-activity reviews=1 comments=0 latest=2026-08-06T16:16:23Z
  | check: core-6110.check.sh: review-activity reviews=0 comments=1 latest=2026-08-06T17:01:00Z

3. bin/fm-wake-drain.sh - what firstmate is handed:
  check: core-6104.check.sh: review-activity reviews=1 comments=0 latest=2026-08-06T16:16:23Z
  check: core-6110.check.sh: review-activity reviews=0 comments=1 latest=2026-08-06T17:01:00Z

4. Next sweep, nothing new on either: silence.
  (no wake)

=== end ===
Evidence: Instrument verification: base-commit control plus two mutations watched going red

MUTATION A - bypass the per-task cursor in bin/fm-watch.sh 4. Two more sweeps with the review still unanswered: silence, not cry-wolf. WATCHER WAKE -> ... review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z WATCHER WAKE -> ... review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z not ok - an unchanged repeat woke firstmate again FM_TEST_SUMMARY total=1 failed=1 MUTATION B - drop the cursor from bin/fm-teardown.sh's rm list not ok - teardown left the review-activity cursor behind FM_TEST_SUMMARY total=1 failed=1 both restored; git status --porcelain empty; suite re-run green

Instrument verification - independent of the author, run against 1ed415b.
"A check you have never watched fail is not a check."

1) CONTROL: the same end-to-end demo run against the BASE commit (70aeba8).
   $ git archive 70aeba8 | tar -x -C <tmp>          # base tree, before the change
   $ FM_ROOT_OVERRIDE=<base tree> bash e2e-review-activity-demo.sh
   Result (full transcript: e2e-control-base-commit-transcript.txt):
     step 1  arming stores no cursor            ("No such file or directory")
     step 3  reviewer submits a blocking review -> NO WAKE
     step 5  human leaves an issue comment      -> NO WAKE
     step 7  .wake-queue does not exist         (nothing was ever queued)
     step 9  PR merged                          -> WAKE "merged"
   This is exactly the reported failure: 21 hours of silence on a blocking
   review, and it proves the pass transcript is measuring real behaviour rather
   than a demo that would print the same thing either way.

2) MUTATION A - bypass the per-task cursor in bin/fm-watch.sh
   (delete `out=$FM_PR_ACTIVITY_SURFACE` after fm_pr_activity_filter)

   a. the end-to-end transcript goes red - the cry-wolf behaviour returns:
      3. Reviewer numeralsix submits a blocking review at 2026-08-06T16:16:23Z.
        WATCHER WAKE -> ... review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z
      4. Two more sweeps with the review still unanswered: silence, not cry-wolf.
        WATCHER WAKE -> ... review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z
        WATCHER WAKE -> ... review-activity reviews=1 comments=3 latest=2026-08-06T16:16:23Z

   b. the colocated suite goes red on the same guard:
      not ok - an unchanged repeat woke firstmate again (watcher reported: 0 check:
               .../task-a.check.sh: review-activity reviews=1 comments=5 latest=2026-08-06T16:16:23Z)
      FM_TEST_SUMMARY total=1 failed=1

   restored; `git status --porcelain` empty.

3) MUTATION B - drop "$STATE/.$ID.pr-activity-cursor" from bin/fm-teardown.sh's rm list
      not ok - teardown left the review-activity cursor behind
      FM_TEST_SUMMARY total=1 failed=1
   restored; `git status --porcelain` empty.

4) PRE-EXISTING FAILURE CONFIRMED NOT INTRODUCED HERE
   tests/fm-teardown.test.sh fails on
     not ok - herdr-preflight-missing-adapter: the retryable pre-return refusal was not explained visibly
   at 1ed415b AND at the base tree extracted from 70aeba8 - same test, same
   assertion, same message. It is a Herdr-adapter environment issue in this
   worktree, unrelated to this change.
Evidence: Colocated suite output (tests/fm-pr-review-activity.test.sh)
FM_TEST_BEGIN 2026-08-07T16:19:40Z tests/fm-pr-review-activity.test.sh family=pr-forge expected_gate_skip=none
ok - poll reports the pull request's current review-activity totals and newest instant
ok - a merged pull request still reports exactly one merged line
ok - every activity failure mode reports nothing
ok - a multi-line activity result reports nothing rather than a forged line
ok - new review activity wakes exactly once and unchanged repeats never wake
ok - arming seeds the cursor so only activity arriving afterwards wakes firstmate
ok - arming clears a cursor it cannot confirm rather than suppressing a real wake
ok - a failed or timed-out sweep reports nothing and leaves the cursor intact
ok - the merge signal wakes firstmate regardless of the review-activity cursor
ok - teardown removes the review-activity cursor with the rest of the task's records
ok - a review-activity wake does not starve a later poll in the same sweep
ok - firstmate's own comments are not review activity and never move the instant
ok - arming a GitLab merge request makes no forge call for the review-activity seed
ok - a symlinked cursor is removed by the clear and repaired by re-arming
ok - the cursor refuses a symlinked destination and a traversing task id
ok - only a review-activity line is filtered; every other poll output passes through
FM_TEST_END 2026-08-07T16:21:37Z tests/fm-pr-review-activity.test.sh exit=0 duration_ms=117489 gate_skip=false
FM_TEST_SUMMARY total=1 failed=0 skipped_gate=0 duration_ms=117536
FM_TEST_SUMMARY_FAMILY family=pr-forge count=1 duration_ms=117489 failed=0
FM_TEST_SLOWEST rank=1 script=tests/fm-pr-review-activity.test.sh duration_ms=117489
Evidence: Reproduction scripts for the transcripts above
#!/usr/bin/env bash
# End-to-end demonstration of the PR review-activity wake, driven through the
# real bin/fm-pr-check.sh, bin/fm-pr-poll.sh, bin/fm-watch.sh, bin/fm-wake-drain.sh
# and bin/fm-teardown.sh in a throwaway FM_HOME.
#
# The only thing faked is the forge CLI: `gh` is a stub serving a scripted
# timeline for the incident named in the task - auctic-core PR 6104, reviewer
# numeralsix submitting a blocking review at 2026-08-06T16:16:23Z. The stub
# evaluates the poll's OWN -q projection with a real jq, so what runs is the
# shipped expression, not a restatement of it.
set -u

ROOT=${FM_ROOT_OVERRIDE:?set FM_ROOT_OVERRIDE to the firstmate checkout}
WORK=$(mktemp -d "${TMPDIR:-/tmp}/fm-review-activity-e2e.XXXXXX")
trap 'rm -rf "$WORK"' EXIT
HOME_DIR="$WORK/home"
STATE="$HOME_DIR/state"
SCENE="$WORK/scene"
FAKEBIN="$WORK/fakebin"
mkdir -p "$STATE" "$HOME_DIR/data" "$WORK/wt" "$SCENE" "$FAKEBIN"
BASE_PATH=/usr/bin:/bin:/usr/sbin:/sbin
ln -sf "$(command -v jq)" "$FAKEBIN/jq"

PR=https://github.com/auctic-software/auctic-core/pull/6104
MR=https://gitlab.com/g/p/-/merge_requests/7
TASK=auctic-core-6104

cat > "$FAKEBIN/gh" <<'SH'
#!/usr/bin/env bash
url=; query=; want=0
for a in "$@"; do
  if [ "$want" = 1 ]; then query=$a; want=0; continue; fi
  case "$a" in -q) want=1 ;; https://*) url=$a ;; esac
done
case " $* " in
  *" --json state "*)   cat "$SCENE/state" ;;
  *" --json reviews,comments "*) jq -r "$query" < "$SCENE/activity.json" ;;
  *) exit 1 ;;
esac
SH
chmod +x "$FAKEBIN/gh"

cat > "$FAKEBIN/glab" <<'SH'
#!/usr/bin/env bash
printf 'state:\topened\n'
SH
chmod +x "$FAKEBIN/glab"

say() { printf '\n%s\n' "$*"; }
run() { printf '$ %s\n' "$*"; }

scene() {  # scene <state> <activity-json>
  printf '%s\n' "$1" > "$SCENE/state"
  printf '%s\n' "$2" > "$SCENE/activity.json"
}

fm() { env FM_HOME="$HOME_DIR" FM_ROOT_OVERRIDE="$ROOT" SCENE="$SCENE" \
  PATH="$FAKEBIN:$BASE_PATH" "$@"; }

# One watcher sweep against the real bin/fm-watch.sh. It exits after it wakes;
# exit 124 with no output means the sweep found nothing worth waking for.
sweep() {
  local out rc
  rm -f "$STATE/.last-check"
  out=$(perl -e 'my $pid=fork; die unless defined $pid; if (!$pid) { exec @ARGV }
      local $SIG{ALRM}=sub { kill "TERM", $pid; waitpid $pid, 0; exit 124 };
      alarm 12; waitpid $pid, 0; alarm 0; exit($? >> 8)' \
    env FM_HOME="$HOME_DIR" FM_ROOT_OVERRIDE="$ROOT" SCENE="$SCENE" \
      FM_CHECK_INTERVAL=0 FM_CHECK_TIMEOUT=5 FM_POLL=0.02 FM_HEARTBEAT=999999 \
      FM_SIGNAL_GRACE=0 PATH="$FAKEBIN:$BASE_PATH" "$ROOT/bin/fm-watch.sh" 2>/dev/null)
  rc=$?
  if [ "$rc" = 124 ] && [ -z "$out" ]; then
    printf '  (watcher swept every armed poll and stayed silent - no wake)\n'
  else
    printf '  WATCHER WAKE -> %s\n' "$out"
  fi
}

printf '=== firstmate PR review-activity wake: end-to-end ===\n'
printf 'firstmate checkout : %s\n' "$ROOT"
printf 'commit             : %s\n' "$(git -C "$ROOT" rev-parse --short HEAD)"
printf 'throwaway FM_HOME  : %s\n' "$HOME_DIR"
printf 'pull request       : %s\n' "$PR"

# --- a task with a PR open, exactly as fm-pr-check.sh is called after push ---
{
  printf 'window=firstmate:fm-%s\n' "$TASK"
  printf 'endpoint_task_id=%s\n' "$TASK"
  printf 'worktree=%s\n' "$WORK/wt"
  printf 'project=%s\n' "$WORK/project"
  printf 'kind=ship\n'
  printf 'mode=no-mistakes\n'
} > "$STATE/$TASK.meta"

say "1. The PR already carries bot chatter when its checks go green."
scene OPEN '{"reviews":[],"comments":[
  {"createdAt":"2026-08-05T11:02:10Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T11:40:55Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T12:15:01Z","viewerDidAuthor":false}]}'
run "bin/fm-pr-check.sh $TASK $PR"
fm "$ROOT/bin/fm-pr-check.sh" "$TASK" "$PR" 2>&1 | sed 's/^/  /'
printf '  cursor seeded at arming: %s\n' "$(cat "$STATE/.$TASK.pr-activity-cursor")"

say "2. Nothing new since arming. The three bot comments must not wake anyone."
sweep

say "3. Reviewer numeralsix submits a blocking review at 2026-08-06T16:16:23Z."
scene OPEN '{"reviews":[{"submittedAt":"2026-08-06T16:16:23Z"}],"comments":[
  {"createdAt":"2026-08-05T11:02:10Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T11:40:55Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T12:15:01Z","viewerDidAuthor":false}]}'
sweep

say "4. Two more sweeps with the review still unanswered: silence, not cry-wolf."
sweep
sweep

say "5. A human leaves a follow-up issue comment on the PR."
scene OPEN '{"reviews":[{"submittedAt":"2026-08-06T16:16:23Z"}],"comments":[
  {"createdAt":"2026-08-05T11:02:10Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T11:40:55Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T12:15:01Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-07T09:30:00Z","viewerDidAuthor":false}]}'
sweep

say "6. firstmate replies on the PR itself. Its own comment is not news."
scene OPEN '{"reviews":[{"submittedAt":"2026-08-06T16:16:23Z"}],"comments":[
  {"createdAt":"2026-08-05T11:02:10Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T11:40:55Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-05T12:15:01Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-07T09:30:00Z","viewerDidAuthor":false},
  {"createdAt":"2026-08-07T10:05:00Z","viewerDidAuthor":true}]}'
sweep

say "7. What firstmate actually reads - the durable wake queue, drained."
printf '  durable rows queued so far (epoch/seq/kind/key/payload):\n'
cut -f5 "$STATE/.wake-queue" | sed "s#$STATE/##; s/^/  | /"
run 'bin/fm-wake-drain.sh'
fm "$ROOT/bin/fm-wake-drain.sh" 2>/dev/null | cut -f5 | sed "s#$STATE/##; s/^/  /"
printf '  (the drain keeps the newest row per key, which is the current activity total)\n'

say "8. The forge goes wrong: the activity read fails outright."
cat > "$FAKEBIN/gh" <<'SH'
#!/usr/bin/env bash
case " $* " in
  *" --json state "*) cat "$SCENE/state" ;;
  *) exit 1 ;;
esac
SH
chmod +x "$FAKEBIN/gh"
sweep
printf '  cursor after the failed read (unchanged, so no wake was swallowed):\n  %s\n' \
  "$(cat "$STATE/.$TASK.pr-activity-cursor")"

say "9. The PR is merged. The pre-existing terminal signal is untouched."
cat > "$FAKEBIN/gh" <<'SH'
#!/usr/bin/env bash
url=; query=; want=0
for a in "$@"; do
  if [ "$want" = 1 ]; then query=$a; want=0; continue; fi
  case "$a" in -q) want=1 ;; https://*) url=$a ;; esac
done
case " $* " in
  *" --json state "*)   cat "$SCENE/state" ;;
  *" --json reviews,comments "*) jq -r "$query" < "$SCENE/activity.json" ;;
  *) exit 1 ;;
esac
SH
chmod +x "$FAKEBIN/gh"
scene MERGED '{"reviews":[{"submittedAt":"2026-08-06T16:16:23Z"}],"comments":[
  {"createdAt":"2026-08-07T11:00:00Z","viewerDidAuthor":false}]}'
sweep

say "10. GitLab is merge-state only, as the poll header declares."
printf '  poll output for %s: ' "$MR"
out=$(env PATH="$FAKEBIN:$BASE_PATH" "$ROOT/bin/fm-pr-poll.sh" --validated \
  gitlab "$MR" gitlab.com g/p 7)
printf '%s\n' "${out:-(nothing: the merge request is open and GitLab reports no review activity)}"

say "11. Teardown removes the cursor with the task's other volatile markers."
printf '  before teardown: %s\n' "$(ls -1 "$STATE"/.*pr-activity-cursor 2>/dev/null | sed "s#$STATE/##")"
cat > "$FAKEBIN/tmux" <<'SH'
#!/usr/bin/env bash
exit 0
SH
chmod +x "$FAKEBIN/tmux"
sed -i.bak 's/^mode=no-mistakes/mode=local-only/' "$STATE/$TASK.meta"
rm -f "$STATE/$TASK.meta.bak"
# The merged task's worktree is already returned, which is the shape that
# reaches artifact cleanup without a live checkout to hand back.
rm -rf "$WORK/wt"
touch "$STATE/.last-watcher-beat"
run "bin/fm-teardown.sh $TASK --force"
# FM_GATE_REFUSE_BYPASS is what tests/lib.sh sets: this demo runs from inside a
# no-mistakes gate worktree, the one environment fm-gate-refuse-lib.sh refuses to
# let drive the fleet. It is an artifact of where this transcript was captured,
# not of the change under test.
FM_GATE_REFUSE_BYPASS=1 fm "$ROOT/bin/fm-teardown.sh" "$TASK" --force \
  > "$WORK/teardown.out" 2> "$WORK/teardown.err"
printf '  teardown exit: %s\n' "$?"
tail -6 "$WORK/teardown.out" | sed 's/^/  | /'
tail -6 "$WORK/teardown.err" | sed 's/^/  ! /'
if [ -e "$STATE/.$TASK.pr-activity-cursor" ]; then
  printf '  after teardown:  cursor STILL PRESENT\n'
else
  printf '  after teardown:  no review-activity cursor remains\n'
fi

printf '\n=== end ===\n'
- Outcome: ⚠️ 1 warning across 1 run (29m15s)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 2 infos
  • ⚠️ bin/fm-pr-lib.sh:723 - fm_pr_activity_cursor_clear refuses to act when the cursor path is a symlink ([ ! -L "$cursor" ] || return 1), which leaves in place the exact artifact the function exists to remove. rm -f -- "$cursor" unlinks the symlink itself and never follows it, so the guard blocks the safe cleanup rather than enabling it. Concrete reachable path: if state/.<id>.pr-activity-cursor is ever a symlink, fm_pr_activity_commit (bin/fm-pr-lib.sh:704) rejects it via fm_pr_regular_destination_or_absent and returns 1 on every sweep, while fm_pr_activity_filter cats through the link and surfaces the line. Re-arming cannot repair it either: fm-pr-check.sh:90 calls commit (fails) and fm-pr-check.sh:91 calls cursor_clear (refuses), both ||-true'd. The result is the same review-activity line waking firstmate every CHECK_INTERVAL forever plus a triage_log entry each time - precisely the cry-wolf-every-cycle outcome the cursor design was chosen to prevent. Only teardown clears it. The colocated test at tests/fm-pr-review-activity.test.sh:294 exercises the symlink guard on commit but only exercises cursor_clear against a path-unsafe task id, so this branch is untested.
  • ⚠️ bin/fm-pr-check.sh:87 - The cursor seed runs the poll unconditionally for every provider, but the poll can only ever report review activity for GitHub. For a GitLab merge request the seed's only possible outcomes are 'merged' or empty, both of which fall to the '*' branch and clear the cursor - yet reaching that conclusion costs a full, untimed 'glab mr view' network round trip. This introduces the first forge network call on the GitLab arming path: previously the only gh/glab call here was guarded by [ "$PROVIDER" = github ] at line 74, so arming a GitLab MR made zero network calls. fm-pr-check.sh is invoked in the foreground by firstmate (AGENTS.md:348) and by bin/fm-pr-merge.sh:73, so a hung or slow glab now blocks arming with no timeout. Guard the seed with [ "$PROVIDER" = github ] and clear the cursor directly for GitLab.
  • ⚠️ bin/fm-watch.sh:840 - The check sweep surfaces at most one check output per CHECK_INTERVAL for the entire home: wake() (bin/fm-push-transition-lib.sh:102) exits the watcher, and $STATE/.last-check is touched immediately before it at bin/fm-watch.sh:870, gating the whole 'for c in "$STATE"/*.check.sh' loop for another FM_CHECK_INTERVAL (default 300s). Until this change a PR poll was silent unless merged, so that contention was negligible. Now any task whose PR has new activity produces output on its sweep. With the intent's own scenario of 20+ open PRs, the post-upgrade burst does not arrive promptly - it drains at roughly one wake per 5 minutes, ~100 minutes total - and while it drains, a merge landing on any poll later in glob order is not observed until the earlier chatty polls go quiet, since each earlier poll's output ends the sweep before its own iteration is reached. The merged signal's content and retirement semantics are preserved exactly, but its detection latency under multi-task contention is not. Worth deciding explicitly whether the sweep should continue past a review-activity wake (queueing it and only exiting on a terminal signal) or accept the serialization.
  • ℹ️ bin/fm-pr-poll.sh:123 - The reported line is derived purely from counts and the newest instant, so firstmate's own activity on its own PR is indistinguishable from a reviewer's: every comment firstmate posts (a status note, a reply to a review thread) increments .comments or creates a review row, changes the line, and therefore wakes firstmate on the next sweep to tell it about itself. Because the line deliberately carries no author (a sound prompt-injection boundary), the only way to discover the activity was self-authored is to read the PR - a full turn. This is bounded at one extra wake per self-authored PR comment rather than a loop, since the cursor advances on that wake, but with 20+ PRs under active conversation it is a steady tax on the exact workflow the feature serves. gh does expose viewerDidAuthor on comments (see docs/verification/pr-review-activity.md:32), so self-authored activity could be excluded from the counts without any author string reaching the wake line. Flagging rather than changing, because excluding it is a product decision about what should count as activity.

🔧 Fix: address PR review-activity cursor, sweep, gitlab, and self-wake findings
2 infos still open:

  • ℹ️ tests/fm-pr-review-activity.test.sh:351 - The jq guard for the self-authored-exclusion test is placed inside the test function, so on a machine without jq the coverage disappears without the runner noticing. bin/fm-test-run.sh:1070 detects a gate skip only when the FIRST non-empty output line matches '^skip:'; here that line is preceded by ~13 'ok - ...' lines from earlier tests, so the file is recorded as an ordinary pass with skipped_gate=0 and the buried 'skip: jq not found' message is never counted. Every other jq-dependent test in this suite (fm-backend-herdr.test.sh:18, fm-decision-hold-lifecycle.test.sh:15, and ~15 more) places the guard at file scope with 'exit 0' precisely so the skip is the first line and is accounted for. The result is that the newly added regression guard for the review decision - the one behavior with no other coverage, since the FM_TEST_GH_ACTIVITY mode bypasses the projection entirely - can go silently uncovered while the suite reports clean. tests/fm-bootstrap.test.sh:135 shows the stricter in-repo precedent: 'command -v jq || fail "jq is required for ..."'. Given ~20 files in this suite already require jq, hard-failing here (or hoisting the guard to file scope) keeps the control visible.
  • ℹ️ bin/fm-pr-poll.sh:114 - Noting an accepted consequence of the mandated mechanism, not requesting a change. viewerDidAuthor is evaluated against whichever account gh is authenticated as, and that is the operator's own account rather than a distinct agent identity: 'gh api user' returns ivannovak here, and on auctic-core#6115 one of three comments comes back viewerDidAuthor=true for that account. So a plain PR comment the operator leaves on a firstmate PR from their own laptop is excluded and will not wake firstmate - the same silence the feature exists to eliminate, now scoped to the operator's own comments. This follows unavoidably from the decision as specified: the instruction chose viewerDidAuthor precisely to avoid an author-login comparison, and gh exposes no field distinguishing an agent-posted comment from an operator-posted one on the same credential. Two things materially limit the blast radius, which is why this is informational: reviews are deliberately not filtered (they carry no viewerDidAuthor), so a formal review submitted from the operator's account still wakes firstmate normally, and any third-party reviewer - the motivating numeralsix case - is unaffected. Only plain issue comments from the authenticated account are silently dropped.
⚠️ **Test** - 1 warning
  • ⚠️ tests/fm-teardown.test.sh:1568 - tests/fm-teardown.test.sh fails on herdr-preflight-missing-adapter: the retryable pre-return refusal was not explained visibly. I verified this is NOT introduced by this change: I extracted the base commit (70aeba8) with git archive into a temp tree and ran the same suite there, and it fails on the identical test with the identical message. The assertion greps stderr for "nothing was changed" after removing bin/backends/herdr.sh from a copied test root, so it is a Herdr-adapter environment issue in this worktree. No action is needed for this change; it is recorded so a reviewer running the pr-forge family is not surprised by a red suite.
  • bash bin/fm-test-run.sh tests/fm-pr-review-activity.test.sh — 16/16 colocated behavior tests green (poll line shape, silence on every failure mode, one-wake-then-silence, arming seed, cursor clear, merge unfiltered, sweep continuation, self-authored exclusion, GitLab no-forge-call, symlink/traversal cursor safety)
  • bash bin/fm-test-run.sh tests/fm-pr-merge.test.sh tests/fm-pr-check-security.test.sh — green; covers the untouched merge path and the poll-byte registration/binding properties
  • bash bin/fm-test-run.sh tests/fm-watch-checkpoint.test.sh tests/fm-watch-triage.test.sh tests/fm-wake-queue.test.sh — green; the watcher check loop and durable wake queue this change modifies
  • bash bin/fm-test-run.sh tests/fm-teardown.test.sh — 1 failure (herdr-preflight-missing-adapter), reproduced identically on a git archive 70aeba8 base tree, so pre-existing
  • Manual end-to-end: FM_ROOT_OVERRIDE=$PWD bash e2e-review-activity-demo.sh — real fm-pr-check.sh / fm-watch.sh / fm-wake-drain.sh / fm-teardown.sh in a throwaway FM_HOME with a stub gh that evaluates the poll's OWN -q projection through a real jq; replays PR 6104 from arming through reviewer review, repeat sweeps, human comment, self-authored comment, forge failure, merge, and teardown
  • Control: the same demo with FM_ROOT_OVERRIDE=&lt;base 70aeba8 tree&gt; — the reviewer's review produces NO wake and no .wake-queue is ever created, only merged wakes
  • Manual upgrade path: FM_BASE_ROOT=&lt;base tree&gt; FM_ROOT_OVERRIDE=$PWD bash e2e-upgrade-path-demo.sh — a poll armed on the old template is rebuilt by the watcher's migration and yields exactly one review-activity wake, then silence, then one wake for a genuinely new review
  • Manual fan-out: FM_ROOT_OVERRIDE=$PWD bash e2e-two-chatty-prs.sh — two PRs receiving activity in the SAME sweep both reach firstmate through the drained wake queue
  • Mutation A: removed out=$FM_PR_ACTIVITY_SURFACE from bin/fm-watch.sh → demo wakes on every sweep (cry-wolf) and the suite reports not ok - an unchanged repeat woke firstmate again; restored, git status --porcelain empty
  • Mutation B: removed &#34;$STATE/.$ID.pr-activity-cursor&#34; from bin/fm-teardown.sh → not ok - teardown left the review-activity cursor behind; restored and re-ran the suite green
⚠️ **Document** - 1 info
  • ℹ️ AGENTS.md:393 - Judgment call worth a follow-up, not a gap in this change: firstmate's procedure for a review-activity wake is now a single inline AGENTS.md line (read the PR, route it to the worker). Per the knowledge-placement tree in .agents/skills/firstmate-coding-guidelines/SKILL.md, a nameable wake type with a real procedure is tier 2 (an agent-only skill plus a one-line trigger). If handling a reviewer's blocking finding needs more than that one line - e.g. deciding between steering the worker, answering the reviewer, or reopening the task - it belongs in a new skill with its trigger declared in AGENTS.md section 13. I deliberately did not create that surface here, since this phase must not add a documentation surface to close a perceived gap.
✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

The per-task PR poll watched for merge only, so a reviewer's blocking finding
sat unnoticed until a human happened to ask. The poll now also reports a pull
request's current review activity, and firstmate wakes the first time that
activity changes.

The poll stays byte-identical for every task and stateless: it reports the
review and issue-comment totals plus the newest instant as an observation of
right now, in its own gh call so a change there cannot turn the merge lookup
silent. A merged result is still reported alone, so its existing retirement
stays an exact match.

bin/fm-watch.sh treats every non-empty check output as actionable and has no
repeat-output suppression, so an unchanged repeat would otherwise wake
firstmate once per sweep forever. The cursor that suppresses it therefore lives
with the watcher, in bin/fm-pr-lib.sh: a check is invoked with no task identity
and must never write task state. It is committed only after the wake is durably
queued, so a watcher that dies between the two repeats a wake rather than
swallowing one. fm-pr-check.sh seeds it at arming from the poll itself, so only
activity arriving afterwards wakes firstmate, and clears it when it cannot
establish a starting point.

Only validated counts and one validated instant reach the wake line; no string
the pull request's authors control does. Every failure mode reports nothing.
GitLab keeps merge state only, for the same missing field selector that already
leaves a GitLab task without a recorded pr_head.
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