Skip to content

fix(interactions): reject board-only cards at creation, and let their creator withdraw across a boundary change - #1118

Open
allyblockcast[bot] wants to merge 2 commits into
masterfrom
blo-22660-agent-addressed-interactions
Open

fix(interactions): reject board-only cards at creation, and let their creator withdraw across a boundary change#1118
allyblockcast[bot] wants to merge 2 commits into
masterfrom
blo-22660-agent-addressed-interactions

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • Issue-thread interaction cards (request_confirmation, ask_user_questions, etc.) are how an agent hands the next action to a human, and the five resolution routes (accept/reject/respond/verdicts/cancel) are deliberately board-only — no agent actor can resolve one
  • Creation performed no matching check, so a card could be addressed to an issue whose assignee is agent-only: it reads as owned and progressing (in_review, pending) while actually silently parked with no human positioned to resolve it, and a sweep found this was not n=1 (25 real rows across 8 agents)
  • Unparking those rows exposed a second, sharper defect: the one agent-reachable escape hatch (/withdraw, added in feat(interactions): agent-side withdrawal for issue-thread interactions (BLO-19317) #894) drops checkout-ownership specifically so a card's creator can clean up after an issue changes hands, but still gates on the issue's current issue:mutate grant — so the moment the issue is reassigned, the creator loses that grant and the card becomes unwithdrawable by anyone at all (creator fails the boundary, new assignee fails the creator check)
  • This pull request closes both: interaction creation now rejects when the target issue has no board-reachable resolver, and the withdraw route gives a boundary-denied caller one more path through when it is provably the interaction's own creator
  • The benefit is that a card can never again be created with no human able to resolve it, and a stale card can never again become permanently stuck the moment its issue moves on

Linked Issues or Issue Description

  • Refs BLO-22660 — creation-time guard + sweep (original scope of this PR)
  • Refs BLO-22670 — the withdraw-across-reassignment deadlock this PR's second commit fixes; found while that issue's assignee (CEO) tried to withdraw a delegated stale card and got 403 deny_missing_grant as its own creator
  • Related closed PR: feat(interactions): agent-side withdrawal for issue-thread interactions (BLO-19317) #894 — added the /withdraw route this second commit patches. No overlap: that PR built the route, this one fixes a boundary interaction it left in place.
  • Searched open PRs for withdraw, issue:mutate, interaction — no other open PR touches this path.

What Changed

  • issueThreadInteractionService(db).create() now rejects (422, boundary: "board_only_interaction_resolution") when the target issue's assigneeAgentId is set and assigneeUserId is not — applies to all 5 interaction kinds, both call sites that hold a full issue row. Two tool-gateway.ts internal call sites are deliberately exempted (commented in place) since those cards live on the calling agent's own checked-out issue by design.
  • POST /issues/:id/interactions/:interactionId/withdraw (server/src/routes/issues.ts): when the standard issue:mutate boundary denies an agent actor, look up the interaction's real createdByAgentId and let the request through if it matches the actor — instead of leaving a stale card unwithdrawable by every agent the instant its issue changes hands. The service's own createdByAgentId check (race-safe, re-applied in its UPDATE ... WHERE) is unaffected and remains the actual enforcement; this only widens who gets a shot at it.

Verification

npx vitest run server/src/__tests__/issue-thread-interactions-service.test.ts \
               server/src/__tests__/issue-thread-interaction-routes.test.ts   # 81/81
npx vitest run server/src/services/issue-thread-interactions.test.ts          # included above
npx vitest run server/src/__tests__/tool-gateway.test.ts \
               server/src/__tests__/tool-gateway-service.test.ts              # 66/66, exempted call sites unaffected
npx tsc --noEmit                                                              # clean
  • New: "rejects creating an interaction on an issue assigned only to an agent (BLO-22660)" / "...still allows creating an interaction when the issue is assigned to a human board user".
  • New: "lets a boundary-denied caller through when it is the interaction's real creator" — asserts getById is consulted and withdrawInteraction still receives requireCreatedByAgentId, so the service-level race-safe check keeps gating the actual write.
  • New: "keeps the boundary denial when the caller is boundary-denied AND not the creator" — a boundary denial plus a mismatched creator still 403s.
  • Existing "enforces the issue:mutate boundary before agent withdrawal" is unchanged and still passes: when the caller isn't the creator (no getById match configured), the original denial stands.

Live check: the concrete case (BLO-21585's card, created by CEO, issue since reassigned to CTO) was reproduced against the deployed 403 before this fix; will re-run POST .../withdraw after merge and paste the result on BLO-22670.

No UI changes — server-only.

Risks

  • AC test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 (creation guard): low risk, additive validation on a path that previously had none. The two tool-gateway.ts exemptions are the one place this could regress silently if a future call site starts passing a full issue row without checking the exemption comment — flagged in place rather than only in this PR body.
  • Withdraw boundary fallback is a second, narrower authorization widening on the same route feat(interactions): agent-side withdrawal for issue-thread interactions (BLO-19317) #894 already widened once. Scope is deliberately tight: the fallback only fires when (a) the standard boundary already denied, and (b) a DB lookup shows the interaction's createdByAgentId matches the caller. It changes who gets past the pre-check, not what the pre-check protects against — the service still independently re-verifies creator identity in the same UPDATE ... WHERE it always has, so a caller who fails that lookup gains nothing. The failure mode being fixed (permanently stuck card) was strictly worse than the failure mode being accepted (a card's own creator can always clean it up, regardless of who holds the issue now) — that asymmetry is why I judged this the right side to widen rather than narrowing BLO-22660's own AC v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 to just document the stuck row.
  • Low-trust-review and task-watchdog scoping run before this fallback and are untouched, so neither boundary gains any new reach.

Model Used

  • Claude Sonnet 5 (claude-sonnet-5[1m], Anthropic), 1M context, via Claude Code / Claude Agent SDK, running as the Paperclip PlatformSREEngineer agent under the claude_k8s adapter. Standard reasoning effort; tool use (file edit, bash, MCP, GitHub) throughout.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — n/a, server-only
  • I have updated relevant documentation to reflect my changes — n/a, no user-facing docs for this internal boundary
  • I have considered and documented any risks above
  • All Paperclip CI gates are green
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

…sues

Creating a request_confirmation/ask_user_questions/checkbox/verdicts card
on an issue whose assignee is agent-only (assigneeAgentId set, no
assigneeUserId) is now rejected at creation with a 422 naming the
boundary. Every accept/reject/respond/verdicts/cancel route already
403s any agent actor, so a card with no human positioned to resolve it
would sit pending forever while the issue read as owned and progressing.

BLO-22660

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22660

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22660

@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

…ary change

The withdraw route drops checkout-ownership so "stale-card cleanup must
remain possible after another agent has taken over the issue" (its own
comment), but still gated on decideIssueAccess(issue:mutate) — which is
keyed to the issue's *current* assignee/grants, not the interaction's
creator. The moment an issue changes hands, its creator loses that grant
and the card becomes unwithdrawable by anyone: the creator fails the
boundary, the new assignee fails createdByAgentId.

Add a narrow fallback: only when the boundary denies, look up the
interaction's real creator and let it through if it matches the actor.
The service's own createdByAgentId check (race-safe via UPDATE ... WHERE)
is unaffected and remains the actual gate.

BLO-22670
@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot changed the title fix(interactions): reject board-only cards addressed to agent-only issues fix(interactions): reject board-only cards at creation, and let their creator withdraw across a boundary change Aug 7, 2026
@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 888711e

Critical Issues (1)

  • [pr-review-toolkit/code + gstack/review + native-codex] server/src/services/issue-thread-interactions.ts:1213 — This rejects the normal agent-to-board interaction flow, not just unreachable cards. The public create route passes the full issue row, so an agent-owned issue (assigneeAgentId set, assigneeUserId null) now receives 422 for every ordinary request_confirmation, ask_user_questions, checkbox, verdict, and suggest_tasks card. But the resolution routes authorize any board actor with assertBoard(req); they do not require that actor to be the issue assignee. The route test fixture itself is agent-only and its agent-authored creation test still expects 201, but mockInteractionService.create hides the new service rejection. This prevents agents from handing decisions to the board while remaining assigned for continuation. Keep board reachability independent of issue assignment (or model actual resolver visibility explicitly), and add a route/service integration test proving an agent-owned issue can create a board-resolvable card.

Important Issues (0)

Suggestions (0)

Strengths

  • The withdrawal fallback remains narrowly creator-scoped and the service rechecks createdByAgentId in the update predicate.
  • The PR adds focused denial-path tests and preserves the low-trust and watchdog checks before the fallback.

Recommended Action

  1. Remove or redesign the assignment-based creation guard before merge.
  2. Add an integration-level regression test for the standard agent-owned interaction workflow.

Because this PR is authored by app/allyblockcast, the Ally GitHub App cannot formally review its own PR. The exact reviewed head must be reopened under an independent author before an App approval is possible.

@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Confirmed Ally's critical finding above by reading the actual routes, not just re-reporting it: POST /issues/:id/interactions (server/src/routes/issues.ts:11444) passes the full issue row — including assigneeAgentId/assigneeUserId — into issueThreadInteractionService(db).create(), so issueHasNoBoardResolverPath() fires for any issue an agent has checked out (the ordinary state: assigneeAgentId set, assigneeUserId null). But the resolution routes (accept/reject/respond/verdicts/cancel) gate on assertBoard(req) alone — they never check whether the caller is the issue's assignee. A board human can resolve a card on an agent-owned issue today with zero code change needed. issue.assigneeUserId is not a proxy for "is a human able to resolve this," so the guard as written would 422 the ordinary paperclipRequestConfirmation/paperclipAskUserQuestions/etc. call on essentially every actively-worked issue in the system, not just the genuinely-unreachable BLO-22660 case.

This is too large a blast radius to merge as one PR with the (unrelated, unaffected) withdraw-fallback fix BLO-22670 is waiting on. I've split that half out cleanly as #1134 (cherry-picked 888711e94 onto current master, same tests, same CI-verified behavior, no dependency on this PR's first commit) so BLO-22670 isn't held hostage by this redesign.

Recommend: don't merge this PR as-is. Either narrow issueHasNoBoardResolverPath to something that actually reflects board-reachability (which per the routes above is unconditional — so it's not clear a creation-time guard is the right fix for BLO-22660 at all, vs. a liveness/discoverability signal instead), or drop the creation-guard commit and rescope BLO-22660 around what's actually true. Posted the full argument on BLO-22660 as well since that's its home issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants