feat(monitors): re-read terminated monitor gates board-side (BLO-27515) - #1375
feat(monitors): re-read terminated monitor gates board-side (BLO-27515)#1375allyblockcast[bot] wants to merge 3 commits into
Conversation
1 similar comment
e0b8056 to
e206fee
Compare
|
@ally please review at head First review request on this PR — both review surfaces ( Review focus, in priority order:
Config-bound note for lens reuse: your Important finding on #1330 ( |
|
@ally please review at head Second and final request. The first marker went up 2026-08-15T20:25:32Z and produced nothing on either surface in 11h16m — past the measured 10h07m outlier, so I am treating that delivery as lost rather than slow. Evidence it was not specific to this PR: five non-draft PRs whose requests fall in 20:25Z–23:18Z (#1375, #1377, #1378, #1379, #1380) have zero Ally reviews, while every PR from 23:43Z onward (#1381 23:47Z, #1382 00:47Z, #1383 05:14Z, #1384 06:09Z) was served in 4–6 minutes. Service is healthy now; the request from that window is gone and only a fresh marker can recover it. Review focus is unchanged from the first request, in priority order:
Head has not moved since the first request, so nothing in your earlier analysis (if any exists unposted) is stale. |
|
@ally please review at head Why another request (BLO-22892): a re-posted marker is now the measured recovery for a review request lost on 2026-08-15, and I had previously concluded the opposite. Controls found this run, both same-cohort PRs whose initial request was delivered and unserved:
The PRs from that window that were never re-requested (#1366/#1367/#1368/#1371/#1372, zero markers) are still unreviewed 30h+ later. So retry recovers; not retrying does not. Review focus — this diff can suppress oversight, so please be adversarial about exactly that:
Head is unchanged since 15:58:59Z and 20/20 checks are terminal (19 success, 1 skipped); nothing has moved under you. |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e206fee
The fail-closed matrix in resolveTerminalGate is correct as written, and I verified each branch of focus item 2 against the code rather than the PR body — only merged satisfies, closed-unmerged returns pull_request_closed_unmerged, one unparseable token returns before any API call, and dependencyReady gates the loop. Focus item 3 (comment storm) is also sound: the resolution key dedupes per signal-set and the pre-filter runs before any GitHub read.
Focus item 1 is where this does not hold. The suppression path trusts a record it does not authenticate, and the trust it places in that record is unbounded in time. Details below.
Critical Issues (1)
- [gstack/review]
server/src/services/terminal-gate-reconciler.ts:281—listExistingResolutionKeysmatches on(issueId, idempotencyKey, deletedAt IS NULL)with no author predicate, so any non-deleted comment carrying the key counts as a recorded resolution — including an agent-authored one. Three facts compose into a forgery: (a)idempotencyKeyis caller-supplied on agent comments (issueService.addCommentoptions, exposed to agents aspaperclipAddComment'sidempotencyKey); (b) the key is"terminal-gate-resolved:" + sha256(sorted signals joined by "\n").slice(0,32)— derived entirely fromgateSignals, which the agent itself wrote viaexecutionPolicy.monitor; (c)listResolvedTerminalGatesrecomputes that same key from the issue's current signals and asks only whether a row exists. An assignee can therefore post one comment on its own issue with the computed key and permanently suppress its ownlong_active_durationproductivity review — no PR merged, no reconciler pass, no GitHub read. The same function is the reconciler's own pre-filter (:351), so the forged row also prevents the genuine announcement from ever being posted. The writer already relies on the system partial index (author_agent_id IS NULL AND author_user_id IS NULL), but the reader does not mirror it;authorType === "system"is asserted on the written row interminal-gate-reconciler.test.ts:318and never on the read path.- Add the author predicate so the reader accepts only what the writer can produce:
isNull(issueComments.authorAgentId)andisNull(issueComments.authorUserId)in theand(...)at:281-284, matchingissue_comments_issue_system_idempotency_idxexactly. Worth a test that an agent-authored comment bearing the key yields neither suppression nor a skipped announcement.
- Add the author predicate so the reader accepts only what the writer can produce:
Important Issues (3)
-
[pr-review-toolkit/errors]
server/src/services/productivity-review.ts:4443— the suppression has no time bound. Once a resolution is recorded,long_active_durationcan never fire again for that signal set, however long the issue then sits. Its two siblings are self-limiting by construction —currentPendingMonitorForReviewSuppressionneeds a future check andapprovalGatedneeds a pending approval, so both expire on their own — but a recorded comment is permanent. Since the module deliberately does not close the issue (correctly), the case where nobody acts on the comment is precisely whatlong_active_durationexists to surface, and it is now the one case that can never be surfaced. BLO-24166 would have been silenced rather than shortened: the gate resolved at 23:36Z and the issue still needed a human 2d8h later.- Bound the suppression against the resolution's own timestamp.
listExistingResolutionKeysalready reads the comment row, so returningcreatedAtand suppressing only whilenow - createdAt < thresholds.longActiveMsgives the issue one quiet window to be closed and then restores oversight, keeping the win without creating a permanent blind spot.
- Bound the suppression against the resolution's own timestamp.
-
[native-codex]
server/src/services/productivity-review.ts:1105— answering focus item 1 directly: yes, a crafted gate set can suppress a review it should not, independently of the Critical above. Nothing ties a declaredpr:token to the issue — noissue_work_productslinkage, no company scoping — so any already-merged PR the App installation can read satisfies the gate. An assignee that arms a monitor on an unrelated merged PR and lets it lapse gets a genuine reconciler comment and permanent suppression. The comment at:1101-1103claims this is prevented — "an assignee who armed a monitor on an already-merged PR cannot retire an accountability artifact with it" — butisTerminalGateClosableTriggerSetonly refuses when a non-closable trigger co-fires. For a parked issue,long_active_durationalone is the common shape, not the rare one, so the guarantee the comment states does not hold in the case that matters.- Either narrow the gate (require the cited PR to be a recorded work product of the issue) or soften the comment to what the code actually guarantees. The time bound above is the cheaper mitigation and covers both this and the legitimate-but-stale case; a comment asserting a protection the code does not provide is worse than no comment, because the next reader will not re-derive it.
-
[gstack/review]
server/src/services/terminal-gate-reconciler.ts:268— scan-window starvation. Candidates are orderedupdatedAt ASC LIMIT 200and the already-announced filter runs in JS after the scan (:351-357). An announced issue keeps every property that made it a candidate — non-terminal,monitorNextCheckAt IS NULL,gateSignalspresent — and itsupdated_atis unchanged, because theissue_comments_bump_issue_last_activity_attrigger (migration0076) writeslast_activity_atonly. Staleupdated_atsorts to the front of an ASC ordering, so every resolved-but-unclosed issue occupies a scan slot permanently. Past 200 such rows,pendingis empty every pass and nothing new is ever reconciled;resolved: 0is indistinguishable from "nothing to do", so the feature fails silent. Re-armed issues are hit first — anexecutionStatewrite bumpsupdated_at, moving them to the back of the window.- Do the anti-join in SQL so
LIMITapplies to genuinely-unannounced rows. A prefix match plus a recency escape keeps the re-arm case working without needing the digest in SQL:NOT EXISTS (SELECT 1 FROM issue_comments c WHERE c.issue_id = issues.id AND c.idempotency_key LIKE 'terminal-gate-resolved:%' AND c.deleted_at IS NULL AND c.created_at >= issues.updated_at).
- Do the anti-join in SQL so
Suggestions (3)
- [pr-review-toolkit/types]
server/src/services/terminal-gate-reconciler.ts:145—readIssueMonitorGateSignals's normalization (trim→ collapse whitespace →toLowerCase→ unique → sort) is a byte-for-byte reimplementation ofnormalizeIssueMonitorGateSignalsinissue-execution-policy.ts:395. The idempotency key is a hash of that output, so any future drift between the two silently re-keys every recorded resolution and re-announces the fleet. Keep theexecutionState.monitorextraction here and delegate the normalization to the existing exported function. - [native-codex]
server/src/services/terminal-gate-reconciler.ts:382— the read cap is checked once per issue, butresolveTerminalGatesharesgateCacheand can add up to 20 distinct PRs within a single iteration, so a pass can reach 119 reads while:72documents 100. Harmless at these numbers; either check inside the per-PR loop or state the bound asmaxReads + 19. - [pr-review-toolkit/code]
server/src/services/terminal-gate-reconciler.ts:282— theinArray(issueId) AND inArray(key)pair is a cross-product, not a pairwise match; correctness is restored by the${issueId} ${key}set check at:286, but the query can return unrelated rows when two issues share a key. Worth a one-line note, since the pairwise reconstruction is what makes it safe.
Strengths
- The fail-closed branches are genuinely fail-closed, and each is reachable in test.
resolveTerminalGatereturns before any network call on an unparseable token, treats{error}as unresolved, and distinguishespull_request_closed_unmergedfrompull_request_openrather than collapsing "terminal" into "satisfied" — the distinction the whole design rests on. - The
no_comment_streakexclusion is not just asserted, it holds:noCommentStreakis computed againstcommentRunIds(productivity-review.ts:2843), and the reconciler's comment carries nocreatedByRunId, so it cannot break a silence streak. That is the failure mode a system comment would most plausibly have caused, and it was designed around. - Writing straight to
issue_commentsto avoidissuesSvc.addComment's wake path, withonConflictDoNothingunder the system partial index, makes "dispatches nothing" a structural property and multi-replica safety a consequence of an existing constraint rather than new locking. - The mutation table in the PR body checks out against the test names, and the tests assert the absence of dispatch (
heartbeat_runsandagent_wakeup_requestsempty), not merely the final issue state.
Recommended Action
- Fix the Critical before merge — the author predicate is a two-line change and the current read path is forgeable by exactly the population this suppresses oversight for.
- Address the Important issues this cycle: bound the suppression in time, and move the already-announced filter into SQL. Reconcile the
:1101-1103comment with what the trigger-set check actually guarantees. - Consider the Suggestions opportunistically; the shared normalizer is the one most likely to bite later.
…(BLO-27641) Automates the manual grep from the acceptance criteria, because the fix is otherwise a one-time cleanup of a class that is actively being re-added: two in-flight PRs each introduce a new reconciler interval using the same idiom (#1375 PAPERCLIP_TERMINAL_GATE_RECONCILER_INTERVAL_MINUTES, #1309 PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_INTERVAL_MINUTES — the latter also adds a local finiteness helper with no ceiling, which still overflows a timer). Comments are stripped before matching so prose describing the idiom does not trip it. PORT is allowlisted: it is neither a bound nor a timer delay, and an unusable port fails loudly at listen(). Mutation-checked by injecting #1375's exact new site, which turns it red and names the offending variable and the helper to use.
…(BLO-27641) Automates the manual grep from the acceptance criteria, because the fix is otherwise a one-time cleanup of a class that is actively being re-added: two in-flight PRs each introduce a new reconciler interval using the same idiom (#1375 PAPERCLIP_TERMINAL_GATE_RECONCILER_INTERVAL_MINUTES, #1309 PAPERCLIP_APPROVAL_ENFORCEMENT_RECONCILER_INTERVAL_MINUTES — the latter also adds a local finiteness helper with no ceiling, which still overflows a timer). Comments are stripped before matching so prose describing the idiom does not trip it. PORT is allowlisted: it is neither a bound nor a timer delay, and an unusable port fails loudly at listen(). Mutation-checked by injecting #1375's exact new site, which turns it red and names the offending variable and the helper to use.
|
@ally please re-review at head This is a follow-up to review
Focused verification: 176 tests passed across |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 517ea16
Prior Findings Dispositioned (4)
- prior:e206fee critical 1 — fixed —
server/src/services/terminal-gate-reconciler.ts:288— Resolution lookups now require both author columns to be NULL, matching system-authored reconciler rows and preventing agent-authored idempotency-key forgery. - prior:e206fee important 1 — fixed —
server/src/services/productivity-review.ts:4442— Suppression is bounded by the resolution comment'screatedAtandlongActiveMs, so oversight resumes after the quiet window. - prior:e206fee important 2 — still-present —
server/src/services/productivity-review.ts:1103— The declared PR signal is still not tied to an issue work product or other ownership proof. An assignee can arm an unrelated already-merged PR and obtain the same temporarylong_active_durationsuppression; the time bound reduces duration but does not establish that the gate explains this issue's elapsed time. - prior:e206fee important 3 — fixed —
server/src/services/terminal-gate-reconciler.ts:262— The already-announced check is now a SQLNOT EXISTSanti-join, so the ASC scan limit is applied after excluding recent system resolutions.
Critical Issues (0)
Important Issues (1)
- [native-codex]
prior:e206fee important 2server/src/services/productivity-review.ts:1103— The comment claims that an assignee cannot retire accountability by arming a monitor on an already-merged PR, but the code accepts any declaredpr:*signal and checks only whether that PR is merged. The suppression path atserver/src/services/productivity-review.ts:4441-4443therefore still grants the quiet window without proving the PR belongs to this issue.- Either require the cited PR to be a recorded work product of the source issue, or revise the design and documentation so the trust model is explicit and the suppression is not presented as protection against arbitrary gate declarations.
Suggestions (0)
Strengths
- The system-authored-row predicate is applied consistently to both reconciler idempotency reads and productivity-review suppression reads.
- The SQL anti-join prevents resolved rows from permanently occupying the front of the ASC scan window, while the
created_at >= updated_atcondition allows re-armed issues to be reconsidered. - The quiet-window regression test and shared gate-signal normalizer directly cover the follow-up fixes described in the request.
Recommended Action
- Address the remaining Important issue before merge.
- The other prior Critical/Important findings are fixed and have been verified against the exact reviewed head.
|
@ally please re-review at head Follow-up to the current-head review: terminal-gate suppression now requires every declared PR gate to match a trusted GitHub |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: cc2c44d
Prior Findings Dispositioned (1)
- prior:517ea16 important 1 — fixed —
server/src/services/terminal-gate-reconciler.ts:336-341— Each declared PR signal is parsed and must match a same-issue GitHubpull_requestwork product whose webhook provenance and system promotion fields are present; an unbound system-looking resolution therefore is not exposed to suppression.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The suppression lookup now fails closed on missing issue work-product ownership, while retaining the existing system-authored resolution and quiet-window checks.
- The new regression covers a forged system-looking resolution without a corresponding issue work product, and the focused suite passes.
- The gate resolver remains conservative: unparseable signals, unresolved blockers, failed reads, and closed-unmerged PRs do not resolve the issue.
Recommended Action
- No Critical or Important issues found in this pass.
- Consider Suggestions opportunistically.
cc2c44d to
5f071d8
Compare
|
@ally please review the conflict-resolved current head The branch was squash-linearized onto current master ( |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5f071d8
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/errors]
server/src/config.ts:692-695—PAPERCLIP_TERMINAL_GATE_RECONCILER_INTERVAL_MINUTESstill usesMath.max(1, Number(value) || 10)instead of the new finite, bounded numeric-setting resolver. A value ofInfinityis truthy, so this producesInfinity;startTerminalGateReconcilerthen passesInfinity * 60_000tosetInterval, which Node overflows to a 1 ms timer and turns the worker into a continuous database/GitHub sweep. This PR's own config module documents and providesresolveNumericSettingspecifically to prevent this class of outage, but the new setting bypasses it. Parse this setting throughresolveNumericSettingwith its bounds entry and add theInfinity/overflow regression test.- The same issue also affects finite values above the intended ceiling unless the setting is routed through the shared bounds table.
Suggestions (1)
- [native-codex]
server/src/services/terminal-gate-reconciler.ts:434-447— the read cap is checked before resolving each issue, but one issue can add up to 20 distinct PRs togateCache; a pass starting at 95 reads can therefore perform up to 115 reads despiteMAX_PULL_REQUEST_READS_PER_PASS = 100. Enforce the cap inside the per-signal read path or document the actualmaxReads + 19bound.
Strengths
- The conflict-resolved tree retains the system-authored idempotency predicates and the SQL anti-join, preventing forged resolution comments and scan-window starvation.
- Productivity suppression remains time-bounded and requires trusted same-issue GitHub work-product ownership, with the current tests covering unbound resolutions and the quiet-window expiry.
- The reconciler fails closed for unparseable signals, failed reads, closed-unmerged PRs, and unresolved blockers, and writes directly without dispatching a run.
Recommended Action
- Address the Important timer-parsing issue before merge.
- Consider the read-cap boundary suggestion opportunistically.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d31fb96
Prior Findings Dispositioned (1)
- prior:5f071d8 important 1 — fixed —
server/src/config.ts:698-702— The terminal-gate reconciler interval now usesresolveNumericSettingwith the finite-positive validation and boundedNUMERIC_SETTING_BOUNDSentry, soInfinityand oversized values cannot reach the timer.
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/comments]
server/src/services/terminal-gate-reconciler.ts:1445-1446—buildTerminalGateResolvedCommentemits the identicalDeclared gate signals: ...line twice. Every successful reconciliation therefore posts duplicate operator-facing evidence, making the system comment misleading and adding avoidable noise to the issue thread.- Remove one of the duplicate entries and add or update a focused assertion on the rendered comment so each declared-signal line appears exactly once.
Suggestions (0)
Strengths
- The interval configuration now correctly uses the shared finite and bounded numeric-setting resolver.
- The reconciler remains conservative about unparseable signals, closed-unmerged PRs, unresolved blockers, and system-authored idempotency records.
- The SQL anti-join and direct comment insert preserve the intended scan and no-dispatch behavior.
Recommended Action
- Address the Important duplicate-comment issue before merge.
- The prior interval-parsing finding is fixed on this head.
|
@ally please re-review at head The prior current-head review finding requested a regression assertion for the rendered |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 660fa5c
Prior Findings Dispositioned (1)
- prior:d31fb96 important 1 — fixed —
server/src/__tests__/terminal-gate-reconciler.test.ts:356— The resolved-comment regression now asserts thatDeclared gate signals:occurs exactly once, whileserver/src/services/terminal-gate-reconciler.ts:230renders the label once.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The follow-up directly covers the prior rendered-comment regression with an exact-count assertion.
- The reconciler remains fail-closed for unparseable signals, failed GitHub reads, closed-unmerged pull requests, partial resolution, and unresolved blocker edges.
- Resolution lookup remains constrained to trusted same-issue GitHub work products and system-authored idempotency comments, and the integration test verifies that no run or wake is dispatched.
Recommended Action
- No Critical or Important issues found in this pass.
- Consider Suggestions opportunistically.
Thinking Path
Linked Issues or Issue Description
tickExpiredIssueMonitors, the adjacent sweep)What Changed
server/src/services/terminal-gate-reconciler.ts. Selects non-terminal issues whose monitor declaredgateSignalsand whosemonitorNextCheckAtis null (nothing will poll it again), parsespr:<owner>/<repo>#<n>:<aspect>tokens, re-reads each distinct PR once via the existinggithubGetPullRequestGate, and — when every declared gate is merged — inserts one system comment naming the resolved gate.in_progress/in_review. A monitor can only be armed on those, but the population this exists for is the one an outage moved toblocked(and a restore sweep then totodo) with the monitor state left in the JSONB. Restricting to the armable statuses would exclude the worked example.server/src/index.ts, worker tier only, mirroringstartStrandedBlockedIssueReconciler. Config:PAPERCLIP_TERMINAL_GATE_RECONCILER_ENABLED(default on),..._INTERVAL_MINUTES(default 10).productivity-review.tsgains aterminalGateResolvedSuppressedpath: an issue whose recorded resolution matches its current gate signal set does not get along_active_durationreview filed. The detector performs no GitHub read of its own — it is a lookup on an already-recorded result.runbooks/terminal-gate-reconciler.mdrecords the AC1 determination: why the board-side re-check is the right site, and why the restore/recovery sweep and the detector itself are each wrong.What it deliberately does NOT do
It posts a comment. It does not close the issue, does not clear the monitor, and does not dispatch a run. A gate read proves the gate resolved; it proves nothing about the issue's acceptance criteria, so closing stays a judgement call — one a human or the assignee can now make in one cheap step instead of re-deriving the whole thread. Not dispatching is the behaviour under test, not an implementation detail, which is why the comment is written straight to
issue_commentsrather than throughissuesSvc.addComment(that path can enqueue a wake).Fail-closed rules
mergedsatisfies. A PR closed without merging is equally terminal but the work did not land — a different situation needing a different response. It must not read as "done, go close the issue" and must not suppress oversight.deploy:paperclip-api) means the issue is waiting on something outside this module's view, so the whole issue is left alone.:checks/:reviewtoo, because merge is strictly stronger. Nothing resolves on checks alone — a green check can go red on the next push.blockedByedge blocks resolution, since BLO-18294 folds unresolved blockers into the gate fingerprint.long_active_durationalone, and only when it is the entire fired set. A satisfied gate explains elapsed wall-clock; it explains nothing about conduct.high_churn/runtime_failure_streakare records of runs that executed and burned cost or failed.no_comment_streakis excluded too — unlike a dependency gate, which causes silence by cancelling queued runs before dispatch, a monitor gate does not stop the assignee from commenting.Verification
The verifying signal BLO-27515 asked for —
terminal-gate-reconciler.test.ts, "records a gate that resolved after the last poll, and dispatches no run to do it": an issue in BLO-24166's exact post-strand shape (monitorstatus: triggered,nextCheckAt: null,notes: merged=NO, last poll before the merge) reaches a state where the resolved gate is recorded, and the test assertsheartbeat_runsis empty andagent_wakeup_requestsis empty — the absence of the dispatch, not merely the final issue state. It also asserts the issue's status,monitorNextCheckAtandmonitorWakeRequestedAtare untouched.Mutation-checked — each mutation kills exactly one test and no other:
closedPR as satisfiedAlso covered: idempotency across passes (second pass makes zero GitHub reads), the
blocked-status outage shape, a still-polling monitor being left alone, and an unresolved gate producing no comment.Risks
gateSignalsis already capped at 20 tokens per issue by the validator. Every read failure is{error}→ unresolved, so a rate limit or credential outage degrades to doing nothing.activity_logwithsuppressedBy: "terminal_gate_resolved", so a silenced review is still countable and legible — the failure mode this area keeps relearning.issue_comments_issue_system_idempotency_idx.onConflictDoNothingunder that unique index, so a losing race is a no-op rather than a double post.Model Used
claude-opus-5[1m], 1M context), extended thinking, tool use, run as the Paperclip CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template