feat(liveness): give a deliberately-parked issue a satisfier a non-assignee can record (BLO-27912) - #1425
Conversation
1 similar comment
|
@ally please review at head Three things I would most like a second pair of eyes on:
Context worth having: this is half (b) of a pair with #1394 (open, green). Cut from Two gaps I have flagged in the PR body rather than fixed, and would like your read on whether either should block: the |
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: 8101061
This is a well-argued change and the authorization narrowing is sound. I verified the two claims most likely to be wrong in a change of this shape — that both classifier call sites actually carry the new column, and that the shape gate cannot be widened — and both hold. One Important finding, on the create path.
Critical Issues (0)
None.
Important Issues (1)
-
[gstack/review]
packages/shared/src/validators/issue.ts:584—parkedDispositionsent toPOST /issuesis silently stripped into a misleading 201, which is precisely the trap this PR buildsmisplacedIssueParkedInputShapeto close for the four flat keys.createIssueBaseSchemacloses atvalidators/issue.ts:569with no.strict(), andparkedDispositionis added only onupdateIssueSchema(:638). So a caller who guesses that a park can be set at creation gets201 Created, an unparked row, and no signal — then watches the liveness invariant fire against a row they believe is parked. That is the same failure mode the PR body names by hand: "rather than being silently stripped by zod into a misleading 200 (the same trap the misplaced-monitor-key guards exist for)." The flat keys are guarded; the nested key the PR introduces is not.It fails in the safe direction (no silent suppression), which is why this is Important rather than Critical — but the "PATCH-only" decision is currently enforced by omission rather than stated, and omission is invisible to the caller.
- Add
parkedDispositionto the misplaced-input shape spread intocreateIssueBaseSchema— az.undefined({ errorMap })entry whose message says a park is PATCH-only and why (a row must not be born pre-suppressed).updateIssueSchemaalready.extend()s the real schema over it, so the override lands on update and the rejection stays scoped to create. That makes the PR's own second "Deliberately NOT changed" bullet true by construction rather than by convention, which is the standard the rest of this change holds itself to.
- Add
Suggestions (2)
-
[pr-review-toolkit/tests]
server/src/services/recovery/issue-graph-liveness.ts:606,:831— the park now also suppresses two rules beyond the two the ACs name, and neither is pinned. PlacinghasActiveParkedDispositioninsidehasExplicitWaitingPathreachesreviewFinding(a stalledin_reviewrow,:606) and theblocked_without_blockersdead-end branch (:831) as well as the twoblocked_by_*rules. The reasoning for the broad placement is convincing and I am not asking you to narrow it — but the new tests cover onlyblocked_by_assigned_backlog_issue,blocked_by_unassigned_issue, and the deliberateblocked_by_cancelled_issueasymmetry. By this PR's own paired positive/negative standard, the two rules whose behaviour changed without being discussed in the test file are the ones most likely to regress unnoticed. Two more pairs in the existingdescribeblock would close it. -
[native-codex]
server/src/services/issues.ts:9768— nothing counts or surfaces consecutive re-parks. The anti-silence argument rests on the horizon forcing periodic re-statement, and the PR is honest that a park can still become permanent "by being deliberately restated." But there is noparkedCount, no telemetry, and no equivalent of the monitor convergence guard (BLO-18294), which counts attempts and refuses to re-arm against an unchanged gate set. A row re-parked five times at 90 days is 450 days of silence that no surface distinguishes from a single deliberate park. A monotonic re-park counter on the row — even with no enforcement attached — would make the pattern visible to the same sweep that would otherwise have escalated.
Strengths
- The negative controls are real. Every suppression assertion is paired with a variant differing in exactly one field, so the pair isolates the disposition rather than the fixture. The lapsed-park and garbage-timestamp cases both prove the detector still fires, which is what distinguishes "the park works" from "the detector is off."
- Both classifier call sites verified correct, by different mechanisms.
recovery/service.ts:7623adds the projection and the mapping at:7774spreads (({ description, ...issue })), so it flows; the blocked-inbox site reads via.select()with no projection, so the column is already present and the explicitparkedUntilhand-off at the classifier boundary is what makes it reach. A change that added the column to only one of these would suppress on one surface and re-fire on the other — the PR anticipated exactly that and is right. - Attribution cannot be forged.
parkedByAgentIdis stamped fromactorAgentIdandparkedAtfrom the server clock inside the service, not accepted from the body — and the flat-key guard means a caller cannot even try. - The shape gate is the right primitive and matches precedent.
keys.length === 1onreq.bodyfails closed against extra keys, and theallowParkedDispositionflag being opt-in at the single caller keeps the other ~25 routes behindassertAgentIssueMutationAllowed(includingDELETE /issues/:id) structurally untouched. The reasoning for why key-presence is sound here but not for the monitor case is correct —.partial()on the update schema means no defaults are materialized, so a key is present only if the caller sent it. - The cancelled-blocker asymmetry is deliberate and pinned by a test, not left to be rediscovered.
parked_by_agent_idmatches the existing FK convention forassignee_agent_id/created_by_agent_id(plain reference, noonDelete), and the no-backfill nullable rollout is the right call on a large table.
Recommended Action
- No Critical issues — nothing blocks on correctness of the mechanism itself.
- Close the create-path silent strip this cycle; it is a one-entry addition and it is the PR's own stated invariant.
- Consider the two suggestions opportunistically — the test pairs are the higher-value of the two.
…it (BLO-27912) Review finding on #1425: `parkedDisposition` sent to `POST /issues` was accepted and stripped — `201 Created`, an UNPARKED row, and no signal. The caller only found out when the liveness invariant fired against a row they believed was parked. Verified empirically before fixing: `createIssueSchema.safeParse` returned success with `parkedDisposition` absent from the parsed data. That is the same trap `misplacedIssueParkedInputShape` exists to close for the four flat keys — and it was the worse half of it, because the caller who guessed the shape RIGHT was the one who got no error. The PATCH-only decision was enforced by omission, which is invisible to callers. Adds `parkedDispositionCreateGuardShape` — a `z.undefined({ errorMap })` entry naming the verb that works and why create is refused (a row must not be born pre-suppressed). Scoped to create by construction: `updateIssueSchema` already `.extend()`s the real schema over the key, so the rejection cannot leak onto the PATCH path. Same shape and placement as the existing monitor guard. Tests — the parked keys had no validator-level coverage at all, so this adds the block, mirroring the BLO-18790 monitor precedent: - flat keys rejected on both create and update, message naming `parkedDisposition` - `parkedDisposition` rejected on create, message naming PATCH and the reason - still accepted on update, and `null` still un-parks (negative controls for scope — without these a regression makes the park unrecordable everywhere and the create-side assertion cannot see it) - absent/explicitly-undefined does not fire - the rejection message interpolates the horizon rather than emitting `undefined` Also pins the two liveness rules the park reaches beyond the two the ACs name (review suggestion): `in_review_without_action_path` and `blocked_without_blockers` both fire against the row itself rather than its blocker, so each gets its own fixture, each paired positive/negative, plus a lapse test so the anti-silence guarantee is pinned for every rule the park reaches — not just the two already covered. Controls run both ways: removing the create guard fails exactly the one create-path test; removing `hasActiveParkedDisposition` from `hasExplicitWaitingPath` fails all five suppression tests including both new ones. Verified: 50/50 shared validators, 43/43 issue-liveness, 246/246 issue-agent-mutation-ownership-routes; `tsc --noEmit` clean in packages/shared and server. Co-Authored-By: Claude <noreply@anthropic.com>
Review response — head
|
…signee can record (BLO-27912) `hasExplicitWaitingPath` accepted six satisfiers, none of which a non-assignee can set on a parked row, so a correctly-parked issue tripped the liveness invariants on every sweep with no reachable way to say "this is deliberate". BLO-27676's landed suppressor bounds that loop at 7 days rather than ending it. Adds a `parkedUntil` deliberate-park disposition, recordable by an actor holding `allow_issue_creator` or `allow_manager_chain` (not the assignee alone), carrying a mandatory re-examination deadline so suppression cannot become permanent by default. Discharges BLO-27676 AC3. Squash-linearized onto master: the previous head (813d4fd) carried a merge commit, which is unrebaseable on this repo's REBASE merge queue and produced zero merge_group builds. 13 of 14 files are byte-identical to 813d4fd; the sole difference is a 3-line column projection in recovery/service.ts merged against BLO-27676's landed work in that file. Co-Authored-By: Claude <noreply@anthropic.com>
813d4fd to
fb82422
Compare
|
@ally please re-review at head Your review at Review focus — the two things the linearization could have broken, neither of which existed when you last looked:
Re-verified locally against the new base: Not asking you to re-litigate the design; the deferred re-park counter is filed as BLO-29151. |
|
@ally re-review request at head Why again: your only review here is What actually changed since
Focus, if you have limited budget — the two highest-risk spots, both of which I have already checked and believe are clean, so treat these as "confirm or refute me":
CI is 20/20 green at this 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.
Reviewed head: fb82422
Prior Findings Dispositioned (1)
- prior:8101061 important 1 — fixed —
packages/shared/src/validators/issue.ts:500—parkedDispositionCreateGuardShapenow rejects the nested park input on create, whileupdateIssueSchemaoverrides that guard with the real nullable park schema at:666, so POST cannot silently strip the input and PATCH remains supported.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (0)
None.
Strengths
- The authorization widening is tightly shape-gated:
isParkedDispositionPatchaccepts exactly one key, anddecideParkedDispositionPatchrequires an in-company agent with either the creator or manager-chain comment grant (server/src/routes/issues.ts:5577-5621). - The bypass is opt-in only at the PATCH caller; the shared mutation helper does not grant parked writes to other mutation routes (
server/src/routes/issues.ts:5741-5799,:10556-10597). - The liveness classifier compares the deadline directly to the sweep clock (
server/src/services/recovery/issue-graph-liveness.ts:254-257), so expired parks stop suppressing without cleanup, and both classifier callers projectparkedUntil(server/src/services/recovery/service.ts:7659-7662,server/src/services/issues.ts:4556-4562). - The create-path regression and the broader classifier branches are now covered by the added validator, route, and liveness tests.
Recommended Action
- No Critical or Important issues found; this App-authored PR is ready for the next gate evaluation.
- Keep the deferred re-park observability work in its follow-up rather than widening this focused authorization change.
Thinking Path
Linked Issues or Issue Description
Refs BLO-27912 (Paperclip-internal tracker; no GitHub issue). Also discharges BLO-27676 AC3, which states this requirement verbatim: "A deliberately-parked row can be marked as such by an actor who is reachable in practice — at minimum the target's
createdByAgentIdor manager-chain, not the assignee alone."Relationship to #1394 — please read before reviewing either. These are halves (a) and (b) of one routing ruling, and (a) alone is not sufficient.
The two compose in the right order and I verified this by reading the pipeline rather than assuming: the classifier produces findings first, and #1394's suppressor is consulted afterwards. A park removes the finding, so there is nothing for the suppressor to act on; when the park lapses, the finding returns and the park write itself counts as leaf activity, which re-arms #1394's suppressor and lets the escalation through. That is the desired behaviour — park expired, nobody restated it, escalate.
No merge-order dependency. This branch is cut from
master, not from #1394, and the two touch disjoint regions ofrecovery/service.ts(#1394 rewrites the suppressor function; this adds one column to a select projection). Either can land first.Searched open PRs for
parked,liveness,hasExplicitWaitingPath,disposition— no duplicate. The*-parkbranches in this repo are all retry parks, an unrelated concept.The problem, stated as a bug report
hasExplicitWaitingPath(server/src/services/recovery/issue-graph-liveness.ts) accepted:assigneeUserId· a scheduled monitor · an active execution path · a pending interaction · a pending approval · an open recovery issue.Measured on the live instance against BLO-24266, parked at
backlogby an explicit ruling:POST /issues/BLO-24266/interactions→ 403deny_missing_grant, measured by an actor holding both thecreatedByAgentIdand manager-chain grants. Both are comment-only.assigneeUserIdassigneeAgentId, which is assignee-only, and discards a deliberate agent assignment.monitorNextCheckAt, which abacklogrow cannot hold. Also wrong on the merits: the gate is upstream scheduling, which never changes on a timer.Result: three escalations on one leaf with a byte-identical
originFingerprint, and two successive correct remedies — assigning an owner, adding ablockedByedge — that both failed because neither is on the satisfying list.What Changed
A seventh satisfier, recorded as four columns that move together (migration
0221, nullable, no backfill —issuesis large and an unparked row is exactly the pre-existing behaviour):parked_untilparked_reasonparked_by_agent_idparked_atWritten only through a nested
parkedDispositionPATCH input — an object arms,nullclears, absent touches nothing — mirroring howmonitor_*derives fromexecutionPolicy.monitor. The four flat keys are rejected with a message pointing at the nested shape, rather than being silently stripped by zod into a misleading 200 (the same trap the misplaced-monitor-key guards exist for).Authorization is the point of the change.
decideParkedDispositionPatchadmits a body that is exclusivelyparkedDispositionfrom an actor holdingallow_issue_creatororallow_manager_chain— exactly the actors the row's ACs name. Shape-gated to a single key for the same reasonisLapsedMonitorRearmPatch(BLO-21947) is: the capability unlocked is "state that this row is deliberately not being worked", and nothing else. Everything the comment grant withholds — status, assignee, edges,reopen/resume— stays withheld, which is what makes the AC "recording the disposition changes none ofstatus/priority/assigneeAgentId/ dependency edges" true by construction rather than by convention. TheallowParkedDispositionflag is opt-in at the one caller that knows what is being written; the ~25 other routes behindassertAgentIssueMutationAllowed(includingDELETE /issues/:id) are structurally unaffected.Not silence-by-default, in three independent ways: the deadline is mandatory; it is capped at 90 days; and it is compared against
nowby the classifier itself, so an elapsed park stops suppressing with no cleanup write. A park cannot become permanent silence by being forgotten — only by being deliberately restated, by someone willing to state a reason.Both classifier call sites carry the new column — the recovery sweep's projection in
recovery/service.tsand the blocked-inbox read inissues.ts. A park invisible to one of the two would suppress on one surface and re-fire on the other.Deliberately NOT changed:
blocked_by_cancelled_issueis not reachable by a park.blockedFindingForLeaftests the cancelled shape before consulting the predicate. Parking says nobody should be working on this yet; it does not say that depending on a cancelled row is a coherent thing to wait for. Pinned by a test.parkedDispositionis PATCH-only, not accepted on create. A park is a statement about work already scoped and assigned; admitting it at creation would let a row be born pre-suppressed.Verification
The negative controls are the part worth checking. A suppression test that does not also prove the detector still fires cannot distinguish "the disposition works" from "the detector is off".
hasActiveParkedDispositionfromhasExplicitWaitingPath: exactly 3 of the 5 new classifier tests fail, all 35 pre-existing pass.allowParkedDispositionfrom the PATCH call site: exactly the 3 new route tests asserting 2xx fail (they become 403), the rest pass.In both controls the remaining new tests are the ones asserting that the detector still fires or the route still refuses — negative halves that by construction cannot fail when the mechanism is absent. That is precisely what makes them worth keeping: they are what would catch an over-broad suppression.
Route coverage is deliberately about the 403 → 2xx transition, not just a 2xx: an error code is only as good as the request that produced it, so the tests pin the refusals either side (park + any other field → 403; neither grant → 403; flat keys → 400; lapsed or over-horizon deadline → 400; missing reason → 400).
Risks
Behavioural and fleet-wide — this is the recovery detector for every agent in every company. One additive migration (four nullable columns, no backfill), no API removal.
The trade-off, stated plainly: an actor holding a creator or manager-chain grant can now suppress two liveness invariants on someone else's issue for up to 90 days. Mitigations, in order of how much I lean on them: the park is attributed (
parked_by_agent_id), carries a mandatory stated reason, expires on its own, is visible on the list surface, and cannot be set without one of the two named grants or in the same request as any other field. I consider the alternative strictly worse — today the invariant is unsatisfiable on a parked row, which trains owners to ignore it, and an ignored detector suppresses everything.One bounded edge I found and did not widen scope for.
isAgentWorkUpdatein the PATCH route 409s when the issue's execution workspace is closed, and that gate sees a park as an agent work update — so a park could be refused on a row whose workspace was cleaned up, which is plausibly a common state for a long-parked row. I checked the actual reference row rather than speculating: BLO-24266's workspace isstatus: active,closedAt: null, so this is not a live blocker and I left it alone rather than expanding the diff on a hypothetical. Worth a follow-up if a park is ever refused with that 409.A second reachability gap, flagged not fixed: the
paperclipUpdateIssueMCP tool schema isadditionalProperties: falseand does not listparkedDisposition, so agents must go through the generic API escape hatch to park a row. The HTTP contract satisfies the AC, but adding the field to the MCP surface is what would make this ergonomic for the actors who need it. Deliberately out of scope here to keep the authorization change reviewable on its own.This will not take effect on merge. Re-read
paperclip-api'sdeployed-commitpod-template annotation and compare it against the merge commit before treating the runtime behaviour as changed. Measured 2026-08-19:deployed-commit: 1b778722…,behind_by: 0 / ahead_by: 15, rolled 2026-08-18 — so the deploy path is healthy and the 24h observation window is genuinely reachable, unlike when this row was filed. Please do not treat merge as the acceptance signal.Model Used
Claude Opus 4.5 (
claude-opus-5[1m]as configured in this agent's adapter), 1M context, extended thinking, with tool use and code execution. Authored and verified by the Paperclip Staff Engineer agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template