feat(approvals): require usable request payloads and add agent withdraw - #296
feat(approvals): require usable request payloads and add agent withdraw#296claudegoogl-sudo wants to merge 7 commits into
Conversation
Two defects compounded into an operator-inbox hazard: a
request_board_approval with an empty payload was creatable (201 Created),
and the requesting agent had no route to retract it, so a schema probe
left a permanently pending blank card sitting next to real decisions.
Creation side:
- discriminated per-type payload validation shared by the server route and
the MCP tool: request_board_approval now requires non-empty title and
summary; hire_agent / approve_ceo_strategy / budget_override_required
keep their existing (no extra) requirements
- POST /api/companies/{id}/approvals with payload {} returns 400 and
writes no row (previously 201)
Withdrawal side:
- POST /api/approvals/{id}/withdraw: only the requesting agent, only while
pending; sets a terminal `withdrawn` status (row kept for audit) and
logs an approval.withdrawn activity entry naming the agent and run
- retries by the same agent converge; losing a race with a board decision
surfaces 422 instead of double-writing
- paperclipApprovalDecision MCP tool gains a `withdraw` action
- docs: API reference (payload table, withdraw route, lifecycle) and the
board-operator approvals guide
Tests driven through createApp + supertest: empty/whitespace/missing/
non-string payloads rejected with no row written; usable payload accepted
and trimmed; other approval types unaffected; withdraw by the requester
logged, by a different agent or a board actor refused (403), on an
already-decided card refused (422), unknown id 404; service-level
idempotency and race coverage.
Verification: vitest suites for server approval routes/service, shared
approval validators, and mcp-server tools (incl. the new withdraw-routing
test) pass; typecheck green for shared, mcp-server, and server. Two
pre-existing base failures/flakes were A/B-verified at the base commit
(approve-idempotency cold-start timeout; mcp create-issue allowDuplicate
expectation) and are left untouched.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
… OpenAPI
CI caught two gaps in the withdraw route:
- The route looked the approval up and then asserted company access, so a
missing id returned 404 while a cross-tenant id returned 403 — an
approval-id existence oracle across companies. The route now goes through
the shared requireApprovalAccess helper, which folds the hasCompanyAccess
gate into the existence check (both cases 404) before the write-path
assertion, matching the pattern documented on hasCompanyAccess.
- POST /api/approvals/{id}/withdraw was missing from the OpenAPI registry;
the mounted-route coverage test failed. Registered with its real
outcome set (403 forbidden, 404 not found, 422 unprocessable).
Both regression points are covered by the existing cross-tenant existence
oracle guard and openapi mounted-route coverage suites.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
CI on the first run caught two real gaps in the withdraw route. Both fixed in 4211ca1:
Local re-verification: Behavior note for reviewers: cross-tenant approval ids now consistently return 404 on withdraw (existence folded into the access gate); the 403 path remains only for same-company actors that are not the requesting agent (different agent or board actor). |
… contract Export createApprovalRequestSchema (shape + discriminated payload refinement) from shared and consume it in both boundaries: the server route keeps its authoritative 4xx, and the paperclipCreateApproval tool now parses the request body before the HTTP call, so an undecidable request_board_approval card fails at the tool boundary with actionable field-level feedback instead of an API 400 after the fact. The contract is stated in the tool description, which is the agent-facing discovery surface. The tool schema itself stays a plain ZodObject because the MCP server registers tools from schema.shape. Also correct the operator guide: the tool is paperclipCreateApproval, not paperclipApprovalRequest. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…tool boundary Drives paperclipCreateApproval through a real execute: an empty payload returns the field-level zod issues (payload.title / payload.summary Required) and never reaches the API. Co-Authored-By: Paperclip <noreply@paperclip.ing>
… schema default createIssueInputSchema defaults allowDuplicate to false since upstream paperclipai#9650, so the parsed create body always carries it. The file is not in any CI shard, so the stale exact-equality assertion only surfaces on a local run; caught while adding the tool-boundary test above. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Dynamic coverage for the existence-oracle fold on the withdraw route: a pending approval from another company is indistinguishable from a missing one (404, no withdraw, no activity entry). Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Review changes pushed — head is now CI: all 24 checks green on run 34097177778, including the previously-red readiness shard. That failure was the known readiness-probe transport flake; instead of re-run roulette I merged master — which now contains #276 (the readiness-probe deflake, freshly landed with green CI and verified on master) — into this branch, so CI runs deterministically. Greptile: still no Greptile engagement anywhere on this repo (no comment or review on this PR, nor on merged #290/#291/#297). The 5/5 checklist item is left open with that note — enabling the integration is a repo-settings action, and I did not want to tick it vacuously. New in this push (review response):
Local verification: server request-integrity + approvals-service 26/26, MCP tools 15/15, shared validators 3/3; typecheck green in shared, mcp-server, and server. |
|
Coordination note from a sibling change: PR #310 ( Relationship to this PR:
Flagging here so reviewers see both shapes in one place and merge order can be chosen deliberately. |
1 participant
Thinking Path
Linked Issues or Issue Description
No public GitHub issue exists for this defect.
What happened?
POST /api/companies/{companyId}/approvalsvalidatedtypeand id fields but passedpayloadthrough unchecked. A request with{"type":"request_board_approval","payload":{}}returned201 Createdand wrote a pending approval with no title, no summary, no recommended action, and no risks. There was noDELETE/PATCH/withdraw route, so the card could only be cleared by a human.Expected behavior: An approval card must be decidable on its own. A
request_board_approvalwithout a usabletitleandsummaryshould fail with4xxbefore any row is written. The agent that created a pending card should be able to withdraw it, and the withdrawal should be logged, not deleted.Steps to reproduce:
POST /api/companies/{companyId}/approvalswith body{"type":"request_board_approval","payload":{}}.201 Createdand a permanently pending blank card appears in the operator approval queue.POST /api/approvals/{id}/withdrawreturnsAPI route not found, so the requesting agent cannot retract the card.What Changed
packages/shared/src/validators/approval.ts: addedrequestBoardApprovalPayloadSchema(non-emptytitle, non-emptysummary, passthrough for extra fields such asrisks) andrefineApprovalPayload, a discriminated per-type refinement. Types without an entry (hire_agent,approve_ceo_strategy,budget_override_required) keep their existing behavior, so no shared loose rule tightens them by accident.server/src/routes/approvals.ts: the create route now validates throughcreateApprovalRequestSchema(createApprovalSchema.superRefine(refineApprovalPayload)). An unusablerequest_board_approvalpayload returns400and writes no row. AddedPOST /api/approvals/{id}/withdraw: agent-actor only, must matchrequestedByAgentId, company access and run-context checks apply, and anapproval.withdrawnactivity-log entry records the agent and run.server/src/services/approvals.ts: addedwithdrawwith idempotent semantics. Onlypendingcards can be withdrawn; a repeat call by the same agent converges without a second log entry; a race lost to a board decision surfaces422via a conditional update onstatus = pending. The row is never deleted;withdrawnis a terminal status.packages/shared/src/constants.ts: addedwithdrawntoAPPROVAL_STATUSES. Theapprovals.statuscolumn istext, so no migration is needed. Every queue/inbox/badge query allows onlypendingandrevision_requested, so a withdrawn card leaves operator attention automatically.packages/mcp-server/src/tools.ts+tools.test.ts:paperclipApprovalDecisiongains awithdrawaction that routes to the new endpoint, giving agents a discoverable way to retract.packages/shared:createApprovalRequestSchema(shape + discriminated refinement) is exported as the single shared definition.packages/mcp-server/src/tools.ts:paperclipCreateApprovalparses its request body through that shared contract before the HTTP call, and its description states thetitle/summaryrequirement (the tool listing is the agent-facing discovery surface). The parse happens in the execute path, not as a schema wrapper, because the MCP server registers tools from the plain ZodObjectschema.shape.server/src/routes/approvals.tsconsumes the sharedcreateApprovalRequestSchemainstead of assembling it locally, so the route and the MCP tool cannot drift.docs/guides/board-operator/approvals.md: corrected the MCP tool name (paperclipCreateApproval, notpaperclipApprovalRequest).docs/api/approvals.md: documented the per-type payload requirements table, the withdraw route with its rules, and the updated lifecycle.docs/guides/board-operator/approvals.md: documented therequest_board_approvalcard type, thewithdrawntransition, and that withdrawal is logged and terminal.server/src/__tests__/approval-routes-request-integrity.test.tsdrives the real app withsupertest;approvals-service.test.tscovers the service contract; two existing idempotency payloads gained the now-requiredsummary.Verification
cd server && npx vitest run src/__tests__/approval-routes-request-integrity.test.ts src/__tests__/approvals-service.test.ts→ 26 passed (15 route + 11 service; adds the cross-tenant withdraw case: a pending approval from another company returns the same404as a missing one, with no withdraw and no activity entry). Route-level coverage throughcreateApp: empty payload →400with no row written; whitespace-only title, missing summary, and non-string summary →400; usable payload →201with trimmed fields and passthrough extras;hire_agent/budget_override_required/approve_ceo_strategypayloads unchanged; withdraw by the requesting agent →200with an activity entry naming agent + run; repeat withdraw → converging no-op; withdraw by a different agent or a board actor →403; withdraw of a decided card →422; unknown id →404. Service-level coverage: idempotency, refusal for non-requester, refusal for decided cards, and the lost-race path.cd packages/shared && npx vitest run src/validators/approval.test.ts→ 3 passed.cd packages/mcp-server && npx vitest run src/tools.test.ts→ 15 passed. New: executingpaperclipCreateApprovalwith an emptyrequest_board_approvalpayload returns the field-level zod issues (payload.title/payload.summaryRequired) and never calls the API. Drive-by (disclosed): the pre-existingallows create issue requests to omit status...exact-body expectation was updated for theallowDuplicate: falseschema default introduced by upstream fix(issues): deduplicate repeated creates paperclipai/paperclip#9650 — the file is not in any CI shard, so the stale assertion only failed on a local run; caught and fixed while adding the tool-boundary test.cd server && npx vitest run src/__tests__/approval-routes-idempotency.test.ts→ theapprove/rejectduplicate-side-effect tests hit a 5s cold-start timeout flake. A/B at the base commit: the same file fails the same way without this change (2 of 3 base runs). The two payloads in that file were only updated to carry the now-requiredsummary.Typecheck:
pnpm typecheckgreen inpackages/shared,packages/mcp-server, andserver(the server run needsNODE_OPTIONS=--max-old-space-size=6144in this sandbox; the default heap OOMs insideensure-plugin-build-deps).Latest CI: run 34097177778 (head
330dab994, includes the merge of master with the test(server): accept probe-timeout transport in readiness failure assertion #276 readiness deflake) — 24/24 checks green.Risks
request_board_approvalcards must now send a non-emptytitleandsummary. Requests that worked before but carried no summary now get400. This is the point of the change, and the MCP request tool and docs now state the requirement.403(they can reject instead), decided cards get422, and non-requester agents get403. Withdrawal keeps the row with a terminalwithdrawnstatus and writes an activity entry naming the agent and run, so an attempted escalation cannot be erased.withdrawnstatus is new to consumers. UI lists and badges use explicitpending/revision_requestedallow-lists, so unknown statuses already render inertly; the status pill shows the literal status text. A dedicated icon/color for the pill is a possible follow-up.approvals.statusis atextcolumn, and the new value is written through the same update path as existing statuses.400there). The one disclosed drive-by test fix (above) touches an expectation only, no production code path.Model Used
Exact model id not exposed to this session; operating as agent "Coder" under the Paperclip runtime (extended thinking + tool use: code search, edits, test execution, and GitHub CLI).
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template — described in-PR using the bug-report template fields (What happened / Expected behavior / Steps to reproduce)#NNN/github.com/paperclipai/paperclipURLs)feat/approval-request-integrity) and contains no internal Paperclip ticket id or instance-derived details330dab994: all 24 checks green, 0 failures (Storybook visual regression and review skipped as on every PR). The one earlier red (General tests (server (2/5))) was the readiness-probe transport flake; resolved by landing test(server): accept probe-timeout transport in readiness failure assertion #276 and merging master into this branch, not by re-running.Compliance statement: Follows
CONTRIBUTING.mdPR template — sections present: Thinking Path, Linked Issues or Issue Description, What Changed, Verification, Risks, Model Used, Checklist. Path-2#devpre-agreement gate: this is a small, focused (Path-1) change — one approval subsystem, targeted validation plus one new route — so the Path-2 gate does not apply.