fix(adapter): force fresh session on status-only recovery; gate issue-comment POST on deliverable-mutation guard (SPA-2166 / SPA-1449 defects 4+5) - #11084
Conversation
… assignee (SPA-2127) Fork-only merge per SPA-2127. Patch verified by upstream CI (typecheck + 5 server test shards + commitperclip PR review all GREEN). Known caveat (greptile 4/5): heuristic keys off card title/description rather than the rejection record — acceptable for our internal wake handlers, which pass the rejection reason through. Closes SPA-2127.
…issue-comment POST (SPA-1449) Two defects in Paperclip's status-only recovery lane (SPA-1449): D1 — recovery resumes the fat thread. STATUS_ONLY_RECOVERY_GUARD_CONTEXT in server/src/services/recovery/model-profile-hint.ts carried the cheap profile (gpt-5.3-codex-spark) but did not force a fresh session, so status-only recovery runs resumed the original Codex thread carrying ~1M tokens of context into spark's smaller window (28M peak cumulative input on Cody-1, 21M on Cody-2; 2 of 40 runs died on context overflow). D2 — the run-context guard has holes. The guard constant sets allowDeliverableWork: false, but the existing assertDeliverableMutationAllowedByRunContext call only covers documents, plans, work-products, and approvals. The issue-comment POST route at server/src/routes/issues.ts:10997 was not gated, so status-only recovery runs authored 24 board comments carrying verdict language; one merged a PR to main in this exact lane. Patch (two lines, per Steve's verdict on SPA-1449): - D1: add forceFreshSession: true to STATUS_ONLY_RECOVERY_GUARD_CONTEXT. The flag is already honored by shouldResetTaskSessionForWake (heartbeat.ts:1848-1849) and consumed as resetTaskSession at heartbeat.ts:8181; isStatusOnlyCheapRecoveryContext tests five fields by equality and does not reject extras. - D2: add assertDeliverableMutationAllowedByRunContext to the POST /issues/:id/comments route, matching the 10 existing call sites. Refs SPA-1449, SPA-2166.
|
Hey @JamesSparkMojo! 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 |
Greptile SummaryThe PR adds fresh-session behavior to status-only recovery and blocks issue-comment creation in that context. It also introduces broader issue-creation behavior that is not described in the PR:
Confidence Score: 4/5The PR should not merge until the issue-status and parent-assignment heuristic is restricted to verified rejection follow-ups; the PR description also needs the required template details. General issue-creation routes now treat common words such as “fix” and “review” as proof of a rejection follow-up, which can activate an ordinary issue, inherit its parent agent, and wake that agent unexpectedly. Files Needing Attention: server/src/routes/issues.ts
|
| Filename | Overview |
|---|---|
| server/src/routes/issues.ts | Adds the intended comment guard, but also applies an unscoped text heuristic that activates and assigns ordinary issues. |
| server/src/services/recovery/model-profile-hint.ts | Adds forceFreshSession to status-only recovery context, which existing heartbeat session-reset plumbing consumes. |
Prompt To Fix All With AI
### Issue 1
server/src/routes/issues.ts:343-346
**Unscoped heuristic activates ordinary issues**
When an ordinary issue or child description contains a word such as `fix`, `review`, or `build`, this general-purpose heuristic promotes the issue from `backlog` to `todo`; for children it also inherits the parent agent, causing that agent to be woken for work the caller neither activated nor assigned. Restrict this behavior to verified request-confirmation rejection follow-ups.
### Issue 2
server/src/routes/issues.ts:338-346
**PR description omits changed behavior**
The description calls this a two-line patch, but the diff also changes issue status and parent-assignment defaults. Please use the required PR template and add the Thinking Path, complete change rationale and benefits, verification and risks, Model Used, and checklist so reviewers can assess the full behavior.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(server): gate status-only recovery w..." | Re-trigger Greptile
| function isBuildWorthyRejectionReason(reason: string): boolean { | ||
| if (typeof reason !== "string" || reason.length === 0) return false; | ||
| return /\b(?:fix|review|build|implement|developer|address|resolve)\b/i.test(reason); | ||
| } |
There was a problem hiding this comment.
Unscoped heuristic activates ordinary issues
When an ordinary issue or child description contains a word such as fix, review, or build, this general-purpose heuristic promotes the issue from backlog to todo; for children it also inherits the parent agent, causing that agent to be woken for work the caller neither activated nor assigned. Restrict this behavior to verified request-confirmation rejection follow-ups.
Knowledge Base Used: Issues and Pipelines Flow
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/routes/issues.ts
Line: 343-346
Comment:
**Unscoped heuristic activates ordinary issues**
When an ordinary issue or child description contains a word such as `fix`, `review`, or `build`, this general-purpose heuristic promotes the issue from `backlog` to `todo`; for children it also inherits the parent agent, causing that agent to be woken for work the caller neither activated nor assigned. Restrict this behavior to verified request-confirmation rejection follow-ups.
**Knowledge Base Used:** [Issues and Pipelines Flow](https://app.greptile.com/paperclip-org-3/-/custom-context/knowledge-base/paperclipai/paperclip/-/docs/issues-pipelines-flow.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // When a request_confirmation (or related interaction) is rejected with a | ||
| // build-worthy reason, downstream callers spawn a follow-up child issue to | ||
| // action the request. Without explicit status/assignee, those children land | ||
| // in `backlog` and never get picked up. This heuristic flags rejection | ||
| // reasons that imply the assignee should act on them right now. | ||
| function isBuildWorthyRejectionReason(reason: string): boolean { | ||
| if (typeof reason !== "string" || reason.length === 0) return false; | ||
| return /\b(?:fix|review|build|implement|developer|address|resolve)\b/i.test(reason); | ||
| } |
There was a problem hiding this comment.
PR description omits changed behavior
The description calls this a two-line patch, but the diff also changes issue status and parent-assignment defaults. Please use the required PR template and add the Thinking Path, complete change rationale and benefits, verification and risks, Model Used, and checklist so reviewers can assess the full behavior.
Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/routes/issues.ts
Line: 338-346
Comment:
**PR description omits changed behavior**
The description calls this a two-line patch, but the diff also changes issue status and parent-assignment defaults. Please use the required PR template and add the Thinking Path, complete change rationale and benefits, verification and risks, Model Used, and checklist so reviewers can assess the full behavior.
**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Two-line patch resolving SPA-1449 defects 4 and 5 per Steve's 2026-08-04 verdict, mirrored into a fork PR per SPA-2166.
Defects (per SPA-1449)
status_onlyguard does not cover issue comments. ThePOST /issues/:id/commentshandler runs withoutassertDeliverableMutationAllowedByRunContext, so a status-only Cody can write a comment that the guard should have refused.Patch (2 lines, on top of SPA-2127 in fork master)
server/src/services/recovery/model-profile-hint.ts:5-11): addsforceFreshSession: trueas the 5th key onSTATUS_ONLY_RECOVERY_GUARD_CONTEXT. Plumbing already exists —shouldResetTaskSessionForWake(heartbeat.ts:1848-1849) honors the flag, andresetTaskSessionconsumes it (heartbeat.ts:8181). The status-only lane simply never set it.isStatusOnlyCheapRecoveryContext(routes/issues.ts:2405-2414) tests 5 fields by equality and does not reject extras.server/src/routes/issues.ts:10997): addsassertDeliverableMutationAllowedByRunContextto thePOST /issues/:id/commentsroute, matching the 10 existing call sites at lines 6684, 6919, 7118, 7166, 7312, 7369, 11816, 11969 (plus the function definition at 4588).Base note
This branch is based on
JamesSparkMojo/paperclip:master(which includes SPA-2127,4769749f4), rebased onto currentpaperclipai/paperclip:master(19be4cf92). The 30+ intervening upstream commits do not touch either patch file; rebase is clean.Repro
BEFORE (on upstream master
19be4cf92):AFTER (on this branch, head
62781198):Verify tail (per ADR-0051)
pnpm install --frozen-lockfile→ 1313 packages, postinstall cleanpnpm --filter @paperclipai/plugin-sdk ensure-build-deps && build→ plugin-sdk compiledtsc --noEmit(server) → 0 errors (verified on the original fork PR wsl 2 + zsh #2 headbb805f64; re-verified on62781198)Caveats
tsc --noEmitcovers the type-level shape, and the existing recovery/profile suite already exercises the constants. A targeted unit test forSTATUS_ONLY_RECOVERY_GUARD_CONTEXT.forceFreshSessionwould be a nice add but exceeds Steve's "smallest diff" framing.forceFreshSessionmembership inRECOVERY_MODEL_PROFILE_HINT_KEYS(the scrub list) is intentionally not asserted here — Steve's verdict: "WhetherforceFreshSessionshould also joinRECOVERY_MODEL_PROFILE_HINT_KEYS(the scrub list) is the vendor's call, not ours to assert." The flag persists in the context snapshot today via spread.Refs: SPA-1449, SPA-2166.