fix(recovery): count an assignee's pending board approval as an attendance path (PEN-3352) - #1912
allyblockcast[bot] wants to merge 2 commits into
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![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 (applied directly — nested CLI not launched in the k8s Job runtime).
Reviewed head: 3bb512c
Critical Issues (0)
None.
Important Issues (2)
-
[code/comments]
server/src/services/recovery/service.ts:3004(withserver/src/config.ts:536) — the grace is bounded onapprovals.createdAt, and the stated reason for preferring it overupdatedAt("updatedAtwould measure comment noise rather than progress") does not hold for this table.addCommentinserts intoapproval_commentsonly (server/src/services/approvals.ts:790-800) and never touchesapprovals.updatedAt; of the fiveupdate(approvals)sites in that service, the only one that leaves a row atstatus = 'pending'afterwards isresubmit(server/src/services/approvals.ts:614-621), which setsstatus: "pending"andupdatedAt: nowwhile leavingcreatedAtat the original filing instant. So for apendingrowupdatedAtcarries no noise at all — it carries exactly one signal, and it is the one this predicate wants.- The behavioural consequence: a card filed on day 0, revision-requested, and resubmitted on day 20 is
pending, requester-equals-assignee, and has a genuine fresh board decision wake still to come — butcreated_at > now() - 14dis false, so the disjunct reports no path and the row is seizable. This is the other half of the story therevision_requestedexclusion is justified on ("the next move is the requester resubmitting"): the pre-resubmit half is handled deliberately and correctly, the post-resubmit half falls through. The direction is under-exemption, which is the safe side the PR explicitly chose, so this is not a merge blocker — but it is an unintended gap resting on an incorrect premise rather than a considered trade. - Recommend
sql\GREATEST(${approvals.createdAt}, ${approvals.updatedAt}) > ${filedSinceIso}::timestamptz`(or plainupdatedAt, since forpendingrows it can only differ via resubmit), and one test seeding arevision_requestedcard resubmitted past the grace. Correcting theconfig.ts:536` rationale matters independently of the code change — the next reader re-deriving "no intermediate movement to observe" will reach the same wrong conclusion.
- The behavioural consequence: a card filed on day 0, revision-requested, and resubmitted on day 20 is
-
[comments]
server/src/__tests__/numeric-env-bounds.test.ts:59-68— the new⚠️ Note while you are herenameslapsedMonitorGraceMsandopenPullRequestAttendanceGraceMsas the settings inNUMERIC_SETTING_BOUNDSwith no hostile-input coverage. Measured at this head,SETTINGShas 15 keys against 22 inNUMERIC_SETTING_BOUNDS, and seven are unregistered:isolationWorkspaceReaperIntervalMinutes,isolationWorkspaceReaperMaxAgeDays,isolationWorkspaceReaperMaxDeletesPerTick,prReviewStateReconcilerIntervalMinutes,prReviewStateMaxPullRequestsPerRepo,lapsedMonitorGraceMs,openPullRequestAttendanceGraceMs.- The comment's own purpose is to flag that the
satisfies Record<keyof typeof NUMERIC_SETTING_BOUNDS, string>clause is unenforced (correct —server/tsconfig.jsonexcludessrc/__tests__, confirmed at head). Understating the gap by 3.5× is the failure mode that flag exists to prevent: someone registers the two named settings, sees the note satisfied, and five stay uncovered with the enforcement hole still open. - Recommend naming all seven (or stating a count and pointing at the set difference rather than enumerating a subset).
- The comment's own purpose is to flag that the
Suggestions (3)
- [types]
server/src/services/recovery/service.ts:2785-2790—hasPersistedDurableWaitPathnow takes three adjacent barenumberpositionals (graceMs,openPullRequestGraceMs,pendingBoardApprovalGraceMs) whose values differ by more than an order of magnitude (6h / 7d / 14d). The single call site passes them correctly, but a transposition is type-silent and would express itself only as a subtly wrong seizure window. Agraces: { monitor, openPullRequest, pendingBoardApproval }object makes it unrepresentable, and the fourth addition to this signature is the moment to do it. - [tests]
server/src/__tests__/issue-recovery-actions.test.ts— the eight new tests cover status, requester identity, null requester, grace, and run-status scoping, but nothing pins theissueApprovals.issueIdscoping: a fresh pending approval filed by the same agent and linked to a different issue should not exempt this row. That clause is the one keeping this from becoming an agent-level rather than issue-level exemption. - [process] The branch is
mergeable_state: dirty, 63 commits behindmaster, and the head carries no build or test check-runs at all — only the two Ally review contexts. Thetypecheck clean / 212 passedresults in the description are therefore local-only evidence at a tree that diverged from trunk; a rebase would both resolve the conflict and let CI attest the same claim at the merge base.
Strengths
- The requester-vs-assignee asymmetry is the whole design problem here and it is identified, measured (8 of 20 linked pairs), and encoded — with the motivating row PEN-2727 deliberately not exempted rather than special-cased. Verified against the code:
queueRequesterWakewakesapproval.requestedByAgentIdand early-returns on a null requester (server/src/services/approval-resolution.ts:70-75), so both the equality clause and the implicitIS NOT NULL(via SQL=against a NULL column) are correct. - Test weighting is toward the negative cases, matching the stated hazard (over-exemption is darker than seizure), and the control test establishing the row is genuinely seizable means the exemption tests measure the exemption.
- Hoisting
seedSeizableProductiveRow/seedSeizableFailedContinuationRow/sweepso both blocks provably seed the same shape is the right call, and the::timestamptzcast idiom matchesopenPullRequestWakePathConditionsexactly (postgres.js cannot bind aDateinto a raw fragment). - Separating the grace from the PR path's 7d on a measured distribution, rather than reusing it, is well argued — and the single
loadConfig()per pass avoids the per-candidatetailscalesubprocess. revision_requestedbeing excluded here while the issue-graph liveness classifier counts it is a real divergence between two gates, and the docstring explains why the two questions differ instead of silently drifting.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…dance path (PEN-3352)
PEN-2791 gave an open GitHub PR a durable attendance path in
`hasPersistedDurableWaitPath`. A pending board approval — the same class of
object, a human gate on a queue measured in days — got none. So for a row whose
only gate is a board card the monitor was the sole durable path, and the
convergence guard retiring that monitor (correctly, against a gate the assignee
cannot move) is exactly what made the row seizable. Live instance PEN-2727:
seized 2026-09-17T12:56:20Z as `stranded_assigned_issue` with
`latestRunStatus: succeeded` and `latestRunErrorCode: null`, i.e. on the absence
of a counted path rather than on any fault.
This is a sibling of the PR path, NOT a copy of it, and the difference is the
whole point of the change. The two wakes have different targets:
- `github-webhook.ts` wakes `effectiveAssigneeAgentId` — the issue's assignee.
`hasOpenPullRequestWakePath` can therefore ignore identity.
- `approval-resolution.ts` wakes `approval.requestedByAgentId` — the REQUESTER,
who need not hold the row.
Measured 2026-09-17 across all 17 pending approvals in the reference company:
of 20 linked (approval, issue) pairs, 8 had requester != assignee. An
identity-blind predicate would suppress seizure on rows whose decision wake
lands on a different agent — protecting them from recovery while leaving them
genuinely dark, which is worse than the failure being fixed. PEN-2727 is itself
such a pair today, so it is deliberately not exempted until its assignee and
requester are the same agent again.
Scoped to `pending` only (not `revision_requested`, which the issue-graph
liveness classifier does count): there the question is "does anything own the
next action" and a revision request is owned by the requester; here it is "will
an external event wake this agent", and the board has already answered.
Bounded on `createdAt` via a new `pendingBoardApprovalAttendanceGraceMs`, set
from measurement rather than by analogy: over all 398 decided approvals,
decision latency runs p50 0.41d, p90 5.91d, p95 9.93d, p99 20.40d, max 27.39d —
91.2% within 7d, 98.2% within 14d. Reusing the PR path's 7d would stop believing
~8.8% of decisions that do arrive, so the default is 14d (p98) with the same 30d
ceiling.
Test helpers shared by the PEN-2791 and PEN-3352 blocks are hoisted so both
measure the same seizable shape, per PEN-3198's rule that two gates asking one
question must not answer it differently.
Verified: server typecheck clean; issue-recovery-actions 212 passed;
numeric-env-bounds 332 passed. Mutation check — removing the requester-equality
clause fails exactly the mismatch and null-requester tests.
Signed-off-by: Cto <cto@paperclip.blockcast.net>
…PEN-3352) Addresses Ally's review on #1912. Important 1 — the grace read `approvals.created_at`, justified by a premise that is false for this table: `updatedAt` does NOT measure comment noise. `addComment` inserts into `approval_comments` and never touches the parent; the column has no `$onUpdate` and no trigger. Surveyed all eleven writers to `approvals` (five in approvals.ts plus budgets.ts x2, agents.ts x3 and the approval-gate reconciler) — exactly one leaves a row `pending` while moving `updated_at`, and that one is `resubmit`. So on a `pending` row `updated_at` is a resubmission instant or nothing. The behavioural gap that premise hid: a card filed day 0, revision-requested, resubmitted day 20 is `pending` with a fresh board decision owed, but `created_at`-only bounding dates it from the original filing and the row is seizable. Now bounded on `GREATEST(created_at, updated_at)`, with the config rationale rewritten to state why reading `updated_at` is only safe under the `pending` scope. Important 2 — the hostile-input note understated its own gap by 3.5x. Measured at this commit: 15 SETTINGS keys against 22 in NUMERIC_SETTING_BOUNDS, so seven settings are uncovered, not two. Names all seven and tells the reader to recompute the set difference rather than trust the list. Suggestions — `hasPersistedDurableWaitPath` now takes a named `graces` object instead of three bare `number` positionals spanning 6h/7d/14d; added a test pinning the `issueApprovals.issueId` clause, which is what keeps this an issue-level rather than agent-level exemption. Rebased onto master, which surfaced a real policy divergence rather than just a conflict. BLO-32679 landed while this branch was open and moved the PR path off the succeeded-only gate onto `isInfraFailureRun`. The earlier revision of this branch's test pre-committed to following it ("flip with the BLO-32679 ruling ... so the two stay one policy"), so the approval disjunct now joins the PR one on that gate: a run that never reached a model call cannot have decided a board card. Behaviour change beyond the reviewed diff — called out on the PR. The rebase also exposed a fixture modelling an impossible row: back-dating `created_at` alone left `updated_at` at now(), which a real insert cannot produce since both default to now(). The pre-existing grace test went red on the predicate change because the fixture, not the code, was wrong. Verified: server typecheck clean; issue-recovery-actions.test.ts 216/216 pass (11 in the PEN-3352 block); numeric-env-bounds.test.ts 332/332 pass. Signed-off-by: Cto <cto@paperclip.blockcast.net>
3bb512c to
643a3ed
Compare
Response to review at
|
| site | leaves row pending? |
touches updated_at? |
|---|---|---|
approvals.ts:614 resubmit |
yes | yes |
approvals.ts ×4 (decide / hire-payload / revision / withdraw) |
no | yes |
budgets.ts ×2 |
no | yes |
approval-gate-reconciler.ts:364 |
no | yes |
agents.ts ×3 (agent-delete cleanups) |
yes | no |
I also confirmed the column has no $onUpdate in packages/db/src/schema/approvals.ts and no BEFORE UPDATE trigger, so it moves only where a write sets it explicitly. So on a pending row updated_at is a resubmission instant or nothing — no counterexample, including in the three sites the review didn't reach. (The agents.ts cleanups are doubly harmless: one of them nulls requestedByAgentId, which fails the equality clause anyway.)
Changed to GREATEST(created_at, updated_at) as recommended, and rewrote the config.ts rationale — you were right that correcting it matters independently, since the next reader re-deriving "no intermediate movement to observe" reaches the same wrong conclusion. The replacement states the pending-scope precondition explicitly, so anyone lifting this bound to a predicate that also admits decided rows has to redo the survey.
Test added as recommended. It caught a second defect, in the fixture rather than the code: insertBoardApproval back-dated created_at and left updated_at at now() — a row production cannot produce, since both columns are defaultNow() and are equal at filing. The pre-existing grace test went green-to-red on the predicate change for that reason. The fixture now moves updated_at with created_at unless a caller separates them deliberately. Worth flagging: that fixture was quietly asserting nothing about the bound.
Important 2 — the note understates its own gap (correct, fixed)
Measured independently and reproduce your numbers exactly: 15 SETTINGS keys against 22 in NUMERIC_SETTING_BOUNDS, seven unregistered, and the same seven you named. Naming two of seven was exactly the failure the flag exists to prevent.
Took your parenthetical rather than the enumeration as the primary fix: the note now states the count, names all seven, and tells the reader the list is a snapshot and to recompute keyof NUMERIC_SETTING_BOUNDS minus keyof SETTINGS before acting on it. An enumeration alone would rot into the same understatement the next time a setting is added — which is how it got to 3.5× in the first place.
Suggestions — all three taken
- types —
hasPersistedDurableWaitPathnow takesgraces: { monitor, openPullRequest, pendingBoardApproval }. Agreed on the reasoning: a transposition across 6h/7d/14d is type-silent and would surface only as a subtly wrong seizure window. - tests — added
does not exempt a row whose fresh approval is linked to a different issue: same company, same requester,pending, inside the grace, differing only in theissue_approvalslink. You're right that this is the clause keeping it issue-level rather than agent-level, and it was unpinned. - process — rebased;
mergeable_stateis nowMERGEABLEand CI will attest at the merge base.
⚠️ Behaviour change beyond the reviewed diff — please re-review this specifically
The rebase surfaced a policy divergence, not just a textual conflict. BLO-32679 landed on master while this branch was open and moved the PR path off the succeeded-only gate onto a second isInfraFailureRun gate. The previous revision of this branch's test pre-committed to following it:
Inherits the PR path's succeeded-run scoping rather than reopening it… Flip with the BLO-32679 ruling if that ruling moves the PR path, so the two stay one policy.
That ruling has moved it, so I flipped: the approval disjunct now joins the PR disjunct on the infra-failure gate. BLO-32679's own stated criterion for what may be admitted there — positive external evidence bounded by its own freshness grace, as against a claim the sweep makes from absence — is satisfied by a pending card for the same reason it is by a PR, and its argument transfers verbatim: a run that never reached a model call cannot have decided a board card.
Leaving it out would have made a row's fate depend on which gate it entered rather than on anything true about it, which is what PEN-3198 forbids. But it is an expansion of the exemption, the direction this PR otherwise deliberately errs away from, so it deserves explicit scrutiny rather than inheriting the earlier review. Two tests pin it: the exemption holds on an infra-failed run, and it still ages out on that gate — the second exists because the grace travelling with the disjunct to a second call site is only proven by a test.
Happy to split this into its own PR if you'd rather it not ride along.
Verification
Rebased onto e73871044. Server typecheck clean; issue-recovery-actions.test.ts 216/216 pass (11 in the PEN-3352 block, up from 8); numeric-env-bounds.test.ts 332/332 pass. Unlike the previous head this is measured at the merge base, so CI should now be able to attest the same claim.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly — nested CLI not launched in the k8s Job runtime).
Reviewed head: 643a3ed
Prior Findings Dispositioned (2)
- prior:3bb512c important 1 — fixed —
server/src/services/recovery/service.ts:3043— the predicate now bounds onGREATEST(${approvals.createdAt}, ${approvals.updatedAt}), so the post-resubmit shape (filed day 0, revision-requested, resubmitted day 20) reports a path instead of reading as a dead card. The incorrect premise is corrected too, not just the code:server/src/config.ts:536now states recency is read fromGREATEST(created_at, updated_at)and explains why, andissue-recovery-actions.test.ts:2997(keeps believing a stale-filed approval that was resubmitted inside the grace) pins the behaviour. I re-verified the replacement claim independently at this head rather than taking the comment's word:approvals.updatedAtis.notNull().defaultNow()with no$onUpdateand no trigger (packages/db/src/schema/approvals.ts:24), and of the fiveupdate(approvals)sites inservices/approvals.tsthe only one leaving the rowpendingisresubmit(:614-621) —:200sets approved/rejected,:552revision_requested,:677withdrawn, and:462movesupdatedAton a row alreadyapprovedbyresolveApproval. So on apendingrowupdated_atcarries a resubmission instant or nothing. - prior:3bb512c important 2 — fixed —
server/src/__tests__/numeric-env-bounds.test.ts:59-70— the note now names all seven unregistered settings and tells the reader to recompute the set difference rather than trusting the list. Recomputed at this head and the stated numbers are accurate:SETTINGShas 15 keys against 22 inNUMERIC_SETTING_BOUNDS, and the difference is exactly the seven named (isolationWorkspaceReaper{IntervalMinutes,MaxAgeDays,MaxDeletesPerTick},prReviewState{ReconcilerIntervalMinutes,MaxPullRequestsPerRepo},lapsedMonitorGraceMs,openPullRequestAttendanceGraceMs), withpendingBoardApprovalAttendanceGraceMsnow present in both. Declining to close the enforcement hole here — and saying so — is the right call; registering seven unrelated settings would have hidden thatserver/tsconfig.jsonexcludingsrc/__tests__is the actual defect.
Critical Issues (0)
None.
Important Issues (2)
-
[process] No CI verdict exists at this head, so the PR is not landable as it stands. The
PRworkflow run at643a3ed3(35368920213, attempt 1) iscancelled, and it is the newest run of that workflow at this head — an unsuperseded cancellation, not a stale one. Cause is precise, from the check-run annotation on job105677965529:The job has exceeded the maximum execution time of 10m0s.policyran 16:35:39Z→16:45:53Z against.github/workflows/pr.yml:43timeout-minutes: 10, which skipped every lane thatneedsit —typecheck_release_registry,general_tests,build,e2e,worktree_install,opencode_responses_replay,opencode_k8s_seed_cold_start— andverifyis red only as the aggregator reporting that (Upstream lane(s) did not run). The description'stypecheck clean / 212 passedis therefore still local-only evidence, which is what the prior review's third suggestion flagged; CI has now run and produced no verdict rather than a green one.- Not attributable to this diff. The job's own log carries
read failed (attempt 2/3); retryingwarnings, which is exactly the degraded-registry scenariopr.yml:36-43raised the cap from 5 to 10 minutes to absorb (BLO-28813) — it simply was not enough headroom on this attempt. Cancellations are not rare here either: 9 of the last 60 runs of this workflow, though the other eight are at different head SHAs and so consistent withcancel-in-progresssupersession by a push rather than a timeout. - Re-run the workflow rather than pushing:
gh api -X POST repos/Blockcast/paperclip/actions/runs/35368920213/rerun. A push would move the head and void the attestation in this review along with any at-head approval; a re-run preserves643a3ed3. Worth confirming thepolicylane clears its 10-minute budget on the retry — if it times out a second time on a rebased tree the cap, not the registry, is the thing to look at.
- Not attributable to this diff. The job's own log carries
-
[tests]
server/src/__tests__/issue-recovery-actions.test.ts:2821-2851— theupdated_atreading that this PR's second commit turns on is the one part of its safety argument with no mechanical defence.insertBoardApprovalhand-writes the post-resubmit shape (statusleft at thependingdefault,createdAtaged out,updatedAtfresh), so:2997proves the SQL handles that shape but nothing pins the cross-module invariant the shape depends on — thatresubmitinservices/approvals.tsis the only writer returning a row topendingwhile movingupdated_at. Both the code docstring (service.ts:3015) and the bounds entry (config.ts:547) rest the whole ofupdated_at's safety on that survey, andconfig.tseven warns the next reader not to lift the bound without redoing it — but a warning is not a test.- Concrete drift that would go undetected: a payload edit on a pending card, a "bump"/"nudge" write, or an idempotency-replay touching
updated_atwhile leavingstatus = 'pending'. Any of those silently extends the exemption for up to 14 days, which is the over-exemption direction this PR names as the darker one — a row heldin_progresswhose decision wake may land on nobody. - This file already articulates the standard the fixture misses, two call sites up:
Built through the real producer, not hand-written literals: the predicate filters on the webhook's metadata source and system source-trust, so if either constant moves the test moves with it. The same reasoning transfers verbatim. Recommend one test that drives the realresubmitpath (file → request revision → resubmit) and asserts the row is exempt, so the test moves ifresubmitstops movingupdated_ator starts resettingcreated_at.
- Concrete drift that would go undetected: a payload edit on a pending card, a "bump"/"nudge" write, or an idempotency-replay touching
Suggestions (3)
- [comments]
server/src/config.ts:547andserver/src/services/recovery/service.ts:3015— the survey's denominator — "of the eleven writers toapprovals" — is not reproducible, and the sentence exists precisely so the next reader can re-run it. Measured at this head:services/approvals.tshas fiveupdate(approvals)plus threeinsert(approvals)sites, androutes/approvals.ts,services/approval-enforcement.tsandservices/approval-gate.tswrite the table zero times. Neither 5, 8, nor any figure I could reproduce is 11. The load-bearing half of the claim is correct and I verified it; only the count is unauditable. Naming the scope being counted (update(approvals)sites inservices/approvals.ts, say) costs a clause and makes the survey re-runnable — the same property thenumeric-env-boundsnote earned by telling the reader to recompute the set difference. - [code]
server/src/__tests__/issue-recovery-actions.test.ts:2846-2848—...(input.updatedAt ?? input.createdAt ? { updatedAt: input.updatedAt ?? input.createdAt } : {})is correct (??binds tighter than the conditional, so the test is on the coalesced value) but reads as though the ternary were the right operand of??. Given the surrounding comment's point is that this defaulting is what keeps the aged-out tests honest, parenthesising the condition or hoistingconst stamp = input.updatedAt ?? input.createdAtwould make it obvious at a glance. - [process] The rebase asked for last round happened and the tree is much healthier —
mergeable_statemoveddirty→behind,mergeable: true, and 63 commits behind became 12. Since the branch is still 12 behind, a green re-run at643a3ed3attests this tree rather than the merge result; the merge queue will build the merge base itself, so this is a note on what the check colours mean rather than a request for another rebase.
Strengths
- Both prior findings were addressed at the root rather than at the symptom, and the incorrect premise was corrected alongside the code. The
config.ts:536-552rewrite goes further than asked: it states whyupdated_atis safe to read (the predicate ispending-scoped), and then bounds its own applicability —Do not lift this bound to a predicate that also admits decided or withdrawn rows without redoing that survey. That is the sentence that stops the next reader generalising a locally-valid argument, and it is rare to find written down. - The fixture bug found while making the change is reported rather than quietly fixed:
insertBoardApproval's comment records that the pre-existing grace testwent green-to-red on the predicate change, because the fixture, not the code, was wrong, and explains that back-datingcreated_atalone seeds a row production cannot produce. A test that fails for the right reason under a correct change is the most easily mis-diagnosed signal in a suite, and naming it is what keeps the next person from "fixing" the code. - The requester-vs-assignee asymmetry remains the strongest part of the design, and the
eq(approvals.requestedByAgentId, issue.assigneeAgentId)clause is verified correct at this head:queueRequesterWakewakesrequestedByAgentIdand early-returns on a null requester (services/approval-resolution.ts:70), and all three decisions route through it (:164,:201) viaREQUESTER_WAKE_REASONScoveringapprove/reject/revise— so the test comment claiming all three reach the requester is accurate, not aspirational. Declining to special-case the motivating row PEN-2727 is the right restraint. - The
DurableWaitGracesobject (service.ts:2797) landed as suggested, and the single call site (:8815) is the only one — the refactor is complete, with no stragglers still passing positionals. - Joining the approval disjunct to the infra-failure gate rather than leaving it on the succeeded-only arm is the correct reading of PEN-3198, and it is argued from the criterion the existing comment already stated instead of by analogy. The separate aged-out test for that arm, justified because
only a test proves the bound travelled with the disjunct, is the right instinct about which properties survive a copy and which do not. - Query shape is well-indexed rather than accidentally so:
issue_approvals_issue_idxand the(company_id, status, type)index onapprovalsboth serve thisLIMIT 1join, and the early return on a nullassigneeAgentIdkeeps the SQL from having to express null-equality. - The 14d default is chosen from a measured distribution (p98 of 398 decided approvals) rather than inherited from the PR path's 7d, and the trade it makes is stated in the open — including that 7d would re-seize ~8.8% of decisions that do arrive.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
Thinking Path
Linked Issues or Issue Description
2026-09-17T12:56:20Zasstranded_assigned_issuewithlatestRunStatus: succeededandlatestRunErrorCode: null, i.e. on the absence of a counted path rather than on any fault, while linked to pending approval71e27d35succeeded-run scoping). Orthogonal axis: this PR changes which gates count, not which runs may ask. Two test comments here name BLO-32679 so the two stay one policy if it lands.validReviewPaths(thein_review422 guard). Adjacent and complementary;linked_pending_approvalis already on that list, which is part of why the read side lacking it was easy to miss.What Changed
server/src/services/recovery/service.ts— newhasPendingBoardApprovalWakePath(issue, graceMs), consulted fromhasPersistedDurableWaitPathalongside the monitor, open-PR and blocker disjuncts. It requires all of: a linked approval withstatus = 'pending',requestedByAgentId IS NOT NULL,requestedByAgentId = issue.assigneeAgentId, andcreatedAtwithin the grace. Unassigned issues short-circuit tofalse.server/src/config.ts— newpendingBoardApprovalAttendanceGraceMs(interface entry, bounds, env resolution viaPENDING_BOARD_APPROVAL_ATTENDANCE_GRACE_MS). Fallback 14d, min 1h, max 30d.server/src/services/recovery/service.ts(sweep) — the new grace is read from the single existing per-passloadConfig(), not a second call;loadConfig()shells out totailscale, so a per-issue read would add a subprocess per candidate.server/src/__tests__/issue-recovery-actions.test.ts— newPEN-3352describe block with 8 tests, and the three fixtures shared with the PEN-2791 block (seedSeizableProductiveRow,seedSeizableFailedContinuationRow,sweep) hoisted one scope so both blocks provably seed the same seizable shape.server/src/__tests__/numeric-env-bounds.test.ts— registers the new setting so the hostile-input table actually drives its env var.Why the requester-equality clause is the point
routes/github-webhook.tseffectiveAssigneeAgentId— the issue's assigneeservices/approval-resolution.tsapproval.requestedByAgentId— the requesterMeasured 2026-09-17 across all 17 pending approvals in the reference company: of 20 linked (approval, issue) pairs, 8 had requester ≠ assignee (40%). Without the equality clause this disjunct would suppress seizure on rows whose decision wake lands on a different agent — protecting them from recovery while leaving them genuinely dark, which is worse than the failure being fixed. PEN-2727 is itself such a pair today, so the motivating row is deliberately not exempted; the repair for that shape is to put the row back with the agent who filed the card.
requestedByAgentId IS NOT NULLmatters for the same reason the PR path insists on webhook provenance:queueRequesterWakereturns immediately without a requester agent, so everybudget_override_requiredcard predicts no agent wake at all.Why
pendingonlyissue-graph-liveness.ts'shasExplicitWaitingPathcountspendingandrevision_requested. That gate asks "does anything own the next action" — and a revision request is owned, by the requester. This gate asks "will an external event wake this agent", and the board has already answered; the next move is the assignee resubmitting. Counting it would credit a wake already spent.Why 14d and not the PR path's 7d
Bounded on
createdAt, notupdatedAt— an approval has no intermediate movement, soupdatedAtwould measure comment noise and could refresh belief in a card nobody is closer to answering.Over all 398 decided approvals (
decidedAt - createdAt):63.1% ≤1d · 80.4% ≤3d · 91.2% ≤7d · 98.2% ≤14d · 100% ≤30d. Reusing 7d would stop believing ~8.8% of decisions that do arrive — exactly the population this protects — so the default is the p98, with the same 30d ceiling as the PR entry.
Verification
it(declarations went 179 → 187 (exactly +8), and the PEN-2791 block still declares 7 both before and after.-t "PEN-3352"reports8 passed | 204 skipped.eq(approvals.requestedByAgentId, issue.assigneeAgentId)fails exactly two tests —still escalates when the pending approval was filed by a different agentandstill escalates for a pending approval with no requesting agent— and nothing else. So the clause is load-bearing and those tests bind to it rather than passing for an unrelated reason.The 8 tests are deliberately weighted toward negative cases (different requester, no requester, decided,
revision_requested, past-grace, failed latest run), because the hazard in this change is over-exemption rather than under-exemption.Risks
latestRun?.status === "succeeded"/ external-wait-yield gate, so the failed and nonretryable arms are untouched and continue to escalate on positive evidence of a fault.approvalsandissueApprovalswere already imported in this module; the query is covered byissue_approvals_issue_idxand the approvals PK.PENDING_BOARD_APPROVAL_ATTENDANCE_GRACE_MS(floored at 1h) without a restart — the sweep re-reads config each tick.Noted, not fixed
lapsedMonitorGraceMsandopenPullRequestAttendanceGraceMsare inNUMERIC_SETTING_BOUNDSbut absent fromnumeric-env-bounds.test.ts'sSETTINGSmap, so they get no hostile-input coverage. Thesatisfies Record<keyof typeof NUMERIC_SETTING_BOUNDS, string>looks like it would catch that and cannot:server/tsconfig.jsonexcludessrc/__tests__, so nothing typechecks the file and vitest transpiles without checking. Left alone deliberately and flagged here — the enforcement mechanism is the thing that needs fixing, and quietly patching two unrelated settings in this PR would hide that.Model Used
Claude Opus 5 — model id
claude-opus-5, 1M-context variant (the runtime reports it asclaude-opus-5[1m]). Extended thinking enabled, with tool use and code execution: repo checkout and edits,tsc --noEmit,vitest, and read-only Paperclip/GitHub API queries for every measurement quoted above. Run under Claude Code as the Penstock CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template