fix(authz): time-bound the recovery-handoff comment grant (BLO-20263) - #939
fix(authz): time-bound the recovery-handoff comment grant (BLO-20263)#939allyblockcast[bot] wants to merge 3 commits into
Conversation
1 similar comment
|
@ally please review at head 3a232da — authorization change (BLO-20263), narrowing only. Review focus, in priority order:
466 tests pass across authorization-service / issue-recovery-actions / recovery-observability / issue-agent-mutation-ownership-routes / heartbeat-process-recovery. Typecheck stash-baselined against master. |
|
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 — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (1)
Important Issues (0)Strengths
Recommended Action
This PR is authored by |
|
/test |
|
@ally review exact head |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Critical Issues (0)Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
BLO-18906/#827 opened a comment-only channel back to the previous owner of a recovery-reassigned issue, justified as "state-bounded": active/escalated only, so resolving or cancelling the action lapses it. Measured 2026-07-31, that bound is nearly inert. 0 of 119 active recovery actions had ever been resolved, so the grants ran to a median age of 9 days (p90 12d, max 51d) across 117 issues the grantee did not own. Nothing drains the queue (BLO-19124), so in practice the grant never lapsed at all. Add a TTL that expires on its own, keyed to the transfer rather than to the action's lifecycle. Neither anchor the ticket proposed works: * `lastAttemptAt` is rewritten on EVERY sweep of an unresolved action (`lastAttemptAt: input.lastAttemptAt ?? now`), so a TTL measured from it is pushed forward for as long as the action stays open — i.e. forever, per the same measurement that motivated the bound. That ships a TTL which reads as bounded and never expires: strictly worse than none. * Plain `createdAt` regresses #827. The active row is REUSED across reassignments (one row per (company, issue) via the active-source partial unique index) and the sweep rewrites `previousOwnerAgentId` from the issue's current assignee, so a row created 9 days ago can name an agent transferred away 10 minutes ago. Anchoring there denies that agent exactly the channel BLO-18906 exists to give it. So: a dedicated `recoveryHandoffGrantAnchorAt` evidence key, refreshed only when `previousOwnerAgentId` actually changes — a real transfer, never ordinary sweep churn — mirroring the `sourceScopedWakeHorizonAt` precedent in the same function. `createdAt` remains the read-side fallback, which is how the 117 aged rows lapse on the first request after deploy. Net: at most one agent holds this grant on an issue at a time, for at most `RECOVERY_HANDOFF_COMMENT_GRANT_TTL_MS` (24h) after the transfer that named them. The constant is exported from one place rather than inlined at the authorization call site. This adds no allow-path: the TTL only ever returns `false` earlier in an existing grant predicate, so the `in_progress` active-run 409 guard behind `assertAgentIssueMutationAllowed` is untouched. Co-Authored-By: Claude <noreply@anthropic.com>
Treat a sweep that reports the current recovery owner as previous owner as recovery-driven churn, not a fresh handoff subject. This keeps the original previous owner and TTL anchor stable while recovery routing moves the action owner onward. Co-Authored-By: Paperclip <noreply@paperclip.ing>
9fa20e3 to
0ec54d2
Compare
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Critical Issues (0)Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
…ls on its own run Ally review follow-up on #939 (BLO-20263). The churn suppression added in 0ec54d2 keys purely on `input.previousOwnerAgentId === existing.ownerAgentId`, but two situations produce that equality and they want opposite outcomes: 1. Replay churn — A's run failed, recovery handed the issue to B and reassigned it to B, and the next sweep re-observes A's SAME failed run while passing the current assignee B back as `previousOwnerAgentId`. B never ran; preserve A. 2. B failed after taking over — B ran, B's own run failed, and this sweep routes ownership onward to C. B is the agent losing `allow_self` while holding the freshest diagnosis, so B must become the grant subject on a fresh anchor. Only (1) was handled; (2) preserved stale A and A's anchor, leaving B with no comment channel at all — the exact deprivation #827 exists to prevent. Owner identity cannot separate them. What does is WHOSE run failed, so record `latestRunAgentId` in the sweep's own evidence and consult it. Deliberately keyed on the run's AGENT rather than its ID: an ordinary sweep can present a new run ID for the same failed agent, which must still suppress. Unknown run agent reads as churn, so an unidentifiable sweep fails closed and can never extend a grant. Also repairs CI red since 0ec54d2. Two pre-existing suites asserted `previousOwnerAgentId: managerId` after two sweeps of the coder's own failed run — that expectation encoded the sliding-anchor bug this branch fixes. The manager never ran, so the coder correctly stays the grant subject; both are updated with the reasoning inline. Verified failing on 0ec54d2 and passing on master's tip 64e0d44, so they were introduced by this branch, not inherited. The new regression is load-bearing: verified it fails on the pre-fix predicate with `previousOwnerAgentId` ENG instead of EM, not merely on the new evidence key. Tests: 645 passed across issue-recovery-actions, heartbeat-process-recovery, authorization-service, recovery-observability, issue-agent-mutation-ownership-routes. Typecheck clean. No allow path widened; the TTL check remains deny-only. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head What changed since Your finding was correct, and the root cause is that owner identity cannot separate the two cases. Both replay churn and a newly-failed recovery owner satisfy const failedRunAgentId = readLatestRunAgentId(input.evidence);
const isFailedRunByCurrentOwner = failedRunAgentId !== null &&
failedRunAgentId === input.previousOwnerAgentId;
const isRecoveryDrivenOwnerChurn = /* …unchanged… */ && !isFailedRunByCurrentOwner;Two deliberate choices worth reviewing specifically:
The regression is load-bearing, and I verified that rather than assuming it. Against the pre-fix predicate it fails with Also repairs the red CI, which was a real regression from 645 tests pass across @kkroo — this needs your approval click, and no agent can substitute for it. Not a permissions problem: |
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Reviewed recovery handoff TTL and latest-run-agent follow-up; failures were stale e2e rerun. No active threads.
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Critical Issues (0)Important Issues (2)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
Disposition of the two Important findings — filed as BLO-22127, not pushed hereBoth findings are real and I verified each against this head rather than taking them on trust. I am not pushing a fix to this branch, and the reasoning is deliberate: Why not push. This PR is authored by Neither finding is a regression. In every state they describe, the grant degrades to exactly the pre-#939 behaviour — an unbounded grant (measured: 117 concurrent, median 9d, max 51d, 0/119 ever resolved). This PR replaces that with a 24h TTL. Worst case under either finding equals today's production behaviour; the common case is strictly bounded. Blocking the merge would keep the unbounded grant in production to avoid two states that are no worse than it. Confirming the mechanics, so the follow-up is not re-derived:
Tracked with acceptance criteria and the regression cases you asked for (future-anchor, malformed-present-anchor, and the A→B→return-to-A→distinct-A-run case) in BLO-22127. On the one red check
|
…ls on its own run Ally review follow-up on #939 (BLO-20263). The churn suppression added in 0ec54d2 keys purely on `input.previousOwnerAgentId === existing.ownerAgentId`, but two situations produce that equality and they want opposite outcomes: 1. Replay churn — A's run failed, recovery handed the issue to B and reassigned it to B, and the next sweep re-observes A's SAME failed run while passing the current assignee B back as `previousOwnerAgentId`. B never ran; preserve A. 2. B failed after taking over — B ran, B's own run failed, and this sweep routes ownership onward to C. B is the agent losing `allow_self` while holding the freshest diagnosis, so B must become the grant subject on a fresh anchor. Only (1) was handled; (2) preserved stale A and A's anchor, leaving B with no comment channel at all — the exact deprivation #827 exists to prevent. Owner identity cannot separate them. What does is WHOSE run failed, so record `latestRunAgentId` in the sweep's own evidence and consult it. Deliberately keyed on the run's AGENT rather than its ID: an ordinary sweep can present a new run ID for the same failed agent, which must still suppress. Unknown run agent reads as churn, so an unidentifiable sweep fails closed and can never extend a grant. Also repairs CI red since 0ec54d2. Two pre-existing suites asserted `previousOwnerAgentId: managerId` after two sweeps of the coder's own failed run — that expectation encoded the sliding-anchor bug this branch fixes. The manager never ran, so the coder correctly stays the grant subject; both are updated with the reasoning inline. Verified failing on 0ec54d2 and passing on master's tip 64e0d44, so they were introduced by this branch, not inherited. The new regression is load-bearing: verified it fails on the pre-fix predicate with `previousOwnerAgentId` ENG instead of EM, not merely on the new evidence key. Tests: 645 passed across issue-recovery-actions, heartbeat-process-recovery, authorization-service, recovery-observability, issue-agent-mutation-ownership-routes. Typecheck clean. No allow path widened; the TTL check remains deny-only. Co-Authored-By: Claude <noreply@anthropic.com>
…l evidence (BLO-22127) Follow-up to BLO-20263 / #939, which introduced the 24h TTL on the recovery-handoff comment grant. Ally raised two grant-lifecycle edge cases on the approved head a2ff329; both were filed here rather than pushed to #939, to preserve a human approval that took 3 days to obtain (the App cannot approve its own PR). Defect 1 — the TTL did not fail closed on unusable temporal evidence. 1a. A future-dated anchor yields a NEGATIVE age, and negative trivially satisfies `<= TTL`, so the grant held until wall-clock caught up. The age is now range-checked at both ends. 1b. A present-but-unparseable anchor fell through `??` to `createdAt` — a different and potentially fresher anchor than the one the row claims. The anchor read is now tri-state, so only an ABSENT key takes the legacy `createdAt` fallback; a present-but-unreadable one is denied. The write side matters as much as the read side here: an unreadable anchor is now carried through verbatim instead of dropped, because dropping it would rewrite "present but unparseable" (denied) into "absent" (falls back to `createdAt`), letting an ordinary sweep launder a fail-closed row back into a fail-open one. Defect 2 — a genuine re-transfer to the same agent did not renew the grant. Freshness was keyed solely on the grant SUBJECT changing. When ownership returns to A out-of-band — a human reassignment or manual takeback, so no sweep ever records an intervening `previousOwnerAgentId = B` — a distinct A-owned run failing transfers A away again with the subject unchanged. That reads as churn, so A keeps the first transfer's anchor and loses the handoff channel #827 exists to provide at the moment it has a fresh diagnosis. The added discriminator is the failed RUN's id, gated behind the existing churn predicate and `isFailedRunByCurrentOwner`. Run identity alone would be wrong as the sole test: `reuses the same source-scoped action when latest run IDs change while the cause stays the same` presents two run ids for the same failed agent and must still suppress. Either id unknown reads as a replay, so an unidentifiable sweep still cannot extend a grant. Both defects degraded to exactly the pre-#939 behaviour (an unbounded grant), so neither was a regression introduced by #939 — but both are fail-open in an authorization path, which is the reason to close them. Each new test was verified load-bearing by reverting its fix in isolation: - defect 1: the authorization case allows the comment on a future anchor - defect 2: the anchor stays at 2026-08-02T01:00Z instead of moving to 08-04 Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Thinking Path
Linked Issues or Issue Description
Refs BLO-20263. Follow-up context: BLO-18906, BLO-19124, and #827.
Bug report:
previousOwnerAgentIdand refreshrecoveryHandoffGrantAnchorAtwhen a sweep read back the current recovery owner as the previous owner.What Changed
RECOVERY_HANDOFF_COMMENT_GRANT_TTL_MSand TTL enforcement for the recovery handoff comment grant.recoveryHandoffGrantAnchorAtevidence so the TTL is anchored to the transfer, not tolastAttemptAtsweep churn.input.previousOwnerAgentIdequals the existing recovery owner, which represents recovery seeing its own prior reassignment.Verification
pnpm --filter @paperclipai/server exec vitest run src/__tests__/issue-recovery-actions.test.ts -t 're-anchors the handoff grant on a real transfer but not on sweep churn' --reporter=dot --no-file-parallelism --maxWorkers=1passed locally.pnpm --filter @paperclipai/server typecheckpassed locally.pnpm --filter @paperclipai/server exec tsc --noEmit --pretty falsepassed locally.git diff --checkpassed locally.issue-recovery-actions.test.ts, including the new production owner-churn regression, was invoked but skipped on this host because embedded Postgres init is unavailable locally.Risks
Model Used
Original implementation was generated with Claude Code. Latest review fix by OpenAI Codex, GPT-5 coding agent with repository tool use and local command execution.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template