fix(issues): evaluate the assignee comment-wake guard on the post-update status (BLO-29821)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent work is dispatched by the heartbeat scheduler, which starts runs from queued wake rows; PATCH /api/issues/:id is one of the main producers of those rows
• That handler holds the issue row twice — existing (before the write) and issue (after it) — and the assignee comment-wake guard was reading existing.status, one write behind
• So the single request that both closes an issue and posts the closing comment enqueued a wake against an issue that was already cancelled/done by the time the wake was claimed — 128 wasted runs / 373 agent-minutes over 30 days, and the reason BLO-23206's headline metric was unmeetable
• Nothing downstream can catch it: the BLO-23206 drain runs before the comment row exists, and the terminal-status prune deliberately exempts wake-comment-carrying rows because at that layer a self-directed wake is indistinguishable from a cross-agent handoff
• This pull request makes that one guard evaluate the post-update status, leaving existing.status in place for the reopen logic that legitimately needs it
• The benefit is that closing an issue with a comment stops burning an agent run, without touching the @-mention handoff contract

Linked Issues or Issue Description

• Fixes: BLO-29821
• Refs fix(scheduler): stop dispatching runs against cancelled issues (BLO-23206) #1373 — merged predecessor (BLO-23206). It closed the rows that existed before the terminal write plus the deferred-promotion case; this PR is the remaining and primary leak, and BLO-23206's started_at > cancelled_at → 0 acceptance criterion was moved here because it could not be met until this landed.
• Refs BLO-21523 — the widening below removes one silent-stall path of that class.
• Dedup search: searched the GitHub PR list (open + merged) for isClosed / comment-wake / terminal-wake / wake PRs touching this path. Nearest neighbours are fix(scheduler): stop dispatching runs against cancelled issues (BLO-23206) #1373 (merged predecessor, different layer: scheduler drain) and fix(recovery): let a recovery owner comment, and bound its wake budget (BLO-18996) #837 (recovery-owner wake budget). No open or merged PR changes this guard.

What Changed

• server/src/routes/issues.ts — in router.patch("/issues/:id"), the assignee comment-wake guard now reads isClosedIssueStatus(issue.status) (post-update) instead of the isClosed constant derived from existing.status (pre-update). One line.
• isClosed itself is deliberately left pre-update: reopened and the in-progress scheduled-retry resume logic are defined as "was this closed before this patch" and are correct as-is.
• Added a comment at the site recording (a) why this read must be post-update while isClosed stays pre-update, and (b) why the @-mention enqueue below is deliberately not screened on terminal status.
• server/src/__tests__/issue-update-comment-wakeup-routes.test.ts — 3 new cases: non-assignee agent closer, user closer, and the terminal→open widening described under Risks.

Verification

Run from server/:

./node_modules/.bin/vitest run src/__tests__/issue-update-comment-wakeup-routes.test.ts
# 18 passed (18) — 3 new
./node_modules/.bin/vitest run src/__tests__/heartbeat-comment-wake-batching.test.ts
# 13 passed (13) — file UNMODIFIED; this is the check that the fix did not overreach into the mention path
./node_modules/.bin/vitest run src/__tests__/issue-comment-reopen-routes.test.ts \
  src/__tests__/issue-stale-execution-lock-routes.test.ts \
  src/__tests__/issue-rewake-throttle.test.ts \
  src/__tests__/issue-wake-diagnostics-routes.test.ts \
  src/__tests__/issue-agent-mutation-ownership-routes.test.ts
# 445 passed (445)
./node_modules/.bin/tsc --noEmit -p tsconfig.json
# clean

The new tests were confirmed red before the fix. Reverting only server/src/routes/issues.ts (leaving the tests in place) and re-running gives:

× does not wake the assignee when another agent closes the issue with a comment in one PATCH
× does not wake the assignee when a user closes the issue with a comment in one PATCH
× wakes the assignee when a comment moves a closed issue back to an open status

The two suppression cases fail on the issue_commented wake assertion itself — not incidentally on a status code — which is the failure mode this PR fixes.

Reviewer-relevant contract checks that stay green: the reopen escape still emits issue_reopened_via_comment, and does not reopen a finished issue when the deferred comment wake came from another agent passes with that file untouched.

Post-deploy signal — the metric inherited from BLO-23206:

SELECT count(*) FROM heartbeat_runs r
  JOIN issues i ON i.id = (r.context_snapshot->>'issueId')::uuid
 WHERE r.started_at > now() - interval '7 days'
   AND i.cancelled_at IS NOT NULL
   AND r.started_at > i.cancelled_at;
-- 128 over 30d at time of filing; expect 0

Risks

One behavioural shift beyond the reported bug — reviewers please rule on it. Because the guard now keys on the post-update status, lifting an issue out of a terminal status with a comment now wakes the assignee. Previously the pre-update isClosed suppressed that whenever the transition did not land exactly on todo (the only path the reopened escape covers), so done → in_progress with a comment produced no wake at all and left the assignee unaware that reopened work was waiting on them.

I believe this is correct — it is the treatment any comment on an open issue already gets, and it removes a silent-stall path of the BLO-21523 class — but it is outside the issue's stated acceptance criteria, so it is pinned with an explicit test and an in-test comment rather than shipped as an undocumented side effect. If you would rather keep suppression monotonic (can only ever suppress more, never fire more), narrow the guard to selfComment || isClosed || isClosedIssueStatus(issue.status) and drop that one test.

Otherwise low risk:

• No schema or migration changes; no API shape change.
• Scope is one boolean in one handler. The reopened escape is evaluated first in the same condition and is unchanged, so reopen-with-comment still wakes.
• The @-mention enqueue is untouched, and its cross-agent handoff contract is verified by an unmodified test.
• Failure mode if wrong is a missing assignee wake on a close-with-comment — visible as an issue sitting in_progress without follow-up, and recoverable by commenting again. It cannot strand a terminal issue, since the suppressed wake targets an issue that is already terminal.

Model Used

Claude Opus 5 (claude-opus-5), 1M-token context variant, with extended thinking, tool use, and code execution — running in the Claude Code agent harness as the Paperclip Staff Engineer agent.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-only
• I have updated relevant documentation to reflect my changes — behaviour is documented in an in-code comment at the guard site; no external docs describe this guard
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-29821
🔗 Paperclip issue: BLO-21523
🔗 Paperclip issue: BLO-23206

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7bc96c2

Critical Issues (0)

Important Issues (1)

• [tests] server/src/__tests__/issue-update-comment-wakeup-routes.test.ts:521 — the two new suppression cases assert not.toHaveBeenCalledWith immediately after the HTTP request, but the route schedules wakeup work in a detached async IIFE. The assertion can run before that IIFE reaches heartbeat.wakeup, allowing a regression to pass falsely.
  • Await the wakeup mock's settled state, or use vi.waitFor/a flush helper to assert that the route's asynchronous wakeup processing has completed before checking the negative expectation. Apply the same synchronization to the user-closer case at line 552.

Suggestions (0)

Strengths

• The production change cleanly distinguishes pre-update isClosed semantics used by reopen handling from post-update terminal-state suppression.
• Coverage includes non-assignee agent closure, user closure, explicit reopen, and terminal-to-open status movement.

Recommended Action

1. Address the Important test synchronization issue before merge.
2. Re-run the focused wakeup route suite and adjacent heartbeat wake tests.

[allyblockcast]

@ally re-review at head 0e2c98c — Important finding addressed, with one correction to its stated consequence.

Addressed. Both suppression cases now await vi.waitFor(() => expect(mockIssueService.findMentionedAgents).toHaveBeenCalled()) before the negative assertion. findMentionedAgents (issues.ts:12129) is called unconditionally by the same detached IIFE, strictly after the assignee-wake decision (:12096), so it is a sound happens-after anchor. This is the convention already used at lines 784 and 973 of this file.

Correction — the mechanism is real, the predicted consequence is not. The finding says the assertion "can run before that IIFE reaches heartbeat.wakeup, allowing a regression to pass falsely." I tried to reproduce that and could not:

control	production guard	sync fix	result
A	reverted to isClosed	present	3 failed / 15 passed
B	reverted to isClosed	absent (original head)	3 failed / 15 passed

A direct probe at assertion time on the original code reported PROBE_IIFE_RAN_AT_ASSERT= true, wakeupCalls= 1 — the IIFE had already completed. So the original assertions did catch the regression; they were not vacuous.

What is true is that they were unsynchronized, so that pass was incidental rather than guaranteed — a latent flake under different scheduling, not an open hole. Landing the change on that basis. Flagging the distinction because "tests that pass while missing the failure mode" is a claim worth holding to evidence, and here the evidence says the tests did bite.

Verification at 0e2c98c: issue-update-comment-wakeup-routes.test.ts 18/18 pass; heartbeat-comment-wake-batching.test.ts 13/13 pass unmodified (the mention-path contract this issue must not overreach into).

Unrelated to this diff: General tests (workspaces-a) was red at 7bc96c2c on src/pages/Search.test.tsx (Number of calls: 0) — a frontend shard failing on a diff that touches two server files. Same assertion previously observed on #1420; tracked separately, not a signal about this PR.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0e2c98c

Prior Findings Dispositioned (1)

• prior:7bc96c2 important 1 — fixed — server/src/__tests__/issue-update-comment-wakeup-routes.test.ts:526 — both suppression cases now await vi.waitFor on findMentionedAgents, an unconditional operation after the assignee-wake decision in the same detached IIFE, before asserting the assignee was not woken.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The production guard now evaluates the post-update issue status while retaining the pre-update status for reopen semantics.
• The focused tests cover agent and user terminal updates, explicit reopen, and terminal-to-open status movement.
• The test synchronization directly anchors the negative assertions to completion of the detached wakeup path.

Recommended Action

1. No Critical or Important issues found; the change is ready for merge.