fix(approvals): decide both approval-link doors through one evaluator (BLO-24699) - #1293
Conversation
… (BLO-24699) `POST /issues/:id/approvals` ran `assertCanManageIssueApprovalLinks` — company-scoped `role === "ceo" || permissions.canCreateAgents`, which never looks at the issue — while `POST /companies/:companyId/approvals` with an `issueIds` array did not. Both reach the same end state, a row in `issue_approvals`. Measured on this company's roster 2026-08-11, that gate admits 2 of 16 agents (CEO, CTO) on the link route while excluding none of the other 14 from the create route. It was therefore not a boundary but an accident of which door an agent picked. Copying it onto create — the symmetric fix recommended in review on PR #1271 — was rejected: it would let those 14 agents file a `request_board_approval` card but not attach it to the issue it concerns, the context-free escalation BLO-23036 exists to close. So the link route now decides through the same `evaluateAgentIssueApprovalLinkAuthorization` as create. The only capability added is attaching a *pre-existing* approval, which discloses nothing new: approval reads are gated by the same `company_scope:read` as create, so any agent that can file an approval can already read every approval in its company. `DELETE /issues/:id/approvals/:approvalId` keeps the privileged gate. Detaching is not reachable through create by any actor, so there is no second door to agree with, and it is the destructive direction. `POST /companies/:companyId/agent-hires` gains the same issue-scoped check over its `sourceIssueIds` — the third door, previously bounded by `agents:create` but not closed. Tests: a cross-route equivalence suite mounting both routers in one harness (the second verifying signal BLO-23763 declared it could not deliver), pinning that an agent with `canCreateAgents: false` can attach to its own issue through either door and is refused on a peer's through either door, with the 409 checkout-conflict contract preserved on both. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head 27c5541. This is the BLO-24699 ruling on the Important finding you raised on #1271. It takes the opposite remedy to the one you recommended there, deliberately — please focus review on whether that call is right:
Note this PR targets |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: 27c5541
Critical Issues (0)
Important Issues (2)
- [native-codex]
server/src/routes/agents.ts:1259—assertHireSourceIssueLinksAllowedthrowsforbiddenfor every evaluator denial, discardingverdict.status. A source issue owned by another agent and currentlyin_progressproduces the evaluator’s retryable 409 through both approval routes, but this third linking door reports a permanent 403. Preserve the aggregate 409 behavior when every refusal is a checkout conflict, and add the equivalent hire-route test. - [native-codex]
server/src/routes/agents.ts:2753— Source-issue authorization is evaluated beforerequireBoardApprovalForNewAgentsis known. When a company does not require hire approvals,sourceIssueIdsare later discarded and noissue_approvalsrow is created, yet a caller can now be denied a legitimate agent hire for an unrelated source issue. Run this check only on the approval-creating path, and cover the no-approval configuration.
Suggestions (0)
Strengths
- The attach route now uses the same side-effect-free issue authorization evaluator as approval creation, avoiding checkout-lock acquisition while closing the direct-route bypass.
- The new equivalence coverage exercises the allow, permanent denial, and active-checkout paths across the two existing doors.
Recommended Action
- Address Important issues this cycle.
|
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 |
Both Important findings from Ally's review at head 27c5541 were correct. 1. `assertHireSourceIssueLinksAllowed` threw `forbidden` for every denial, discarding `verdict.status`. A source issue held under another agent's checkout is a *retryable* refusal that the other two doors report as 409, so the third door was telling callers a hire could never succeed when it succeeds once that checkout ends. It now mirrors `assertIssueLinksAllowed`: 409 when every refusal is a checkout conflict, 403 for a mixed set (the stricter reading), with each entry's own status retained in `details.refusals`. The point of this PR is that the doors agree, so a status-only divergence was still a divergence. 2. The check ran before `requiresApproval` was known. `linkManyForApproval` is only reached inside that branch, so with board approval disabled the `sourceIssueIds` are discarded and no `issue_approvals` row is ever created — the check could deny a legitimate hire over a link that was never going to happen. It now runs only on the approval-creating path, placed after `requiresApproval` is resolved but before `svc.create`, so a refusal never leaves a persisted agent behind. Also updates `issue-agent-mutation-ownership-routes.test.ts`. Its shared cheap/status-only assertion pinned that `assertCheckoutOwner` ran for every route in the table; the attach route no longer probes checkout ownership by design. Asserted positively (`not.toHaveBeenCalled`) for that one case rather than skipped, so a reintroduced lock probe still fails the suite. Every other case — including approval *unlink*, which kept the old pair — is unchanged, as is the cheap-recovery refusal itself. Tests: hire suite grows the 409 case and the no-approval-required case. Both fail against the previous commit with exactly the two diagnoses above. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head 1. Status collapse ( 2. Check ran before One thing to look at that you did not flag, because I had to change an existing test. Local: typecheck clean; 13 approval/hire/ownership suites, 287 tests green. Unchanged from the last round: this PR targets #1271's branch, and #1271 is still |
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: 51b6297
Prior Findings Dispositioned (2)
- prior:27c5541 important 1 — fixed —
server/src/routes/agents.ts:1276— the helper now returns409only when every collected refusal is a checkout conflict, preserving the retryable status. - prior:27c5541 important 2 — fixed —
server/src/routes/agents.ts:2842— source-issue authorization now runs only in therequiresApprovalbranch, where an approval link will actually be created.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/routes/issues.ts:8984— The newly relaxed attach route authorizes only the issue and then callsissueApprovalsSvc.linkfor any same-companyapprovalId; it never performs thecompany_scope:readapproval-access check used byPOST /companies/:companyId/approvals. An agent with mutation rights to its own issue but no approval-read grant can attach a guessed/obtained approval UUID and immediately receive that approval's redacted payload fromGET /issues/:id/approvals(server/src/routes/issues.ts:8962-8969). This turns the changed authorization relaxation into an approval-read bypass.- Require approval read access for the supplied approval before linking, and add a test for an own-issue actor denied
company_scope:read.
- Require approval read access for the supplied approval before linking, and add a test for an own-issue actor denied
Suggestions (0)
Strengths
- The two prior authorization findings are addressed with explicit route-level coverage for checkout conflicts and the no-approval hire path.
Recommended Action
- Fix the Important approval-read boundary bypass before merge.
|
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 |
…699) Ally's review of #1293 at 51b6297 found that relaxing POST /issues/:id/approvals onto the shared issue-scoped evaluator dropped the approval-side check that POST /companies/:companyId/approvals has always run, so the two doors were equivalent only for actors holding both halves. The finding is correct. GET /issues/:id/approvals returns linked approvals to any actor that can read the issue, so an actor that may mutate its own issue but may not read approvals could attach a guessed approval id and read the row back — an approval-read bypass rather than a link. authorization.ts denies `company_scope:read` outright to task-bridge keys, skill-test run tokens, and low-trust-preset agents while still allowing `issue:mutate` on their own issue, and those are exactly the classes the dropped `assertCanManageIssueApprovalLinks` gate happened to keep off this route. `assertApprovalReadAllowed` mirrors `assertApprovalAccessAllowed` in routes/approvals.ts — same action, same 403 body. Ordinary agents hold the grant, so the BLO-23036 escalation path is untouched. This makes the doors actually equivalent rather than equivalent-for-agents, which is what the PR set out to do. Placed after the run-context check rather than before it as create has it: the orderings differ only in which 403 an actor failing both is told about, never in the verdict, and the cheap status-only refusal names the delegation path BLO-23036 AC #2 requires stay visible. The new equivalence case fails against 51b6297 with `expected 'allow' to be 403` on the link door while the create door already refused — the divergence itself. Also corrects the route doc comment, whose "discloses nothing new" argument held for ordinary agents only. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Finding accepted: it was a real approval-read bypassYour
Each of those passes the new issue-scoped evaluator on its own issue, and Fix
This strengthens the PR's own thesis rather than walking it back. Create gates on both halves (approval-side One deliberate deviation you should checkI placed the gate after VerificationNew case
|
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: 8a802d4
Prior Findings Dispositioned (1)
- prior:51b6297 important 1 — fixed —
server/src/routes/issues.ts:9027— the attach route now requirescompany_scope:readbefore linking, matching approval creation and preventing scoped actors from using an issue link to read an approval they cannot otherwise access.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The approval-side read check closes the previously reported link-to-read escalation while retaining the issue-scoped evaluator for both creation and attachment.
- Equivalence tests cover allow, permanent denial, checkout conflict, and the scoped approval-read denial across both linking doors.
- The agent-hire path preserves its retryable checkout-conflict status and only authorizes source issues when it will create an approval link.
Recommended Action
- No Critical or Important issues found.
|
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 |
|
Review round 2 acknowledged — 0 Critical / 0 Important / 0 Suggestions, prior The red
Template, linked-issue, test-coverage and lockfile gates all passed. Cleared by editing the description, not by pushing: added a Two boxes are left deliberately unchecked, rather than ticked to make the list look complete:
Blocked only on #1271, which is |
5b47dbd
into
cto/blo-23763-approval-create-issue-authz
…BLO-23763) `assertIssueApprovalLinkAllowed` (added by BLO-24699's shared-evaluator refactor) takes the looked-up issue row, so its `assertCompanyAccess(req, issue.companyId)` answers a cross-tenant id with 403 where a missing id answers 404 — the enumeration oracle documented on `hasCompanyAccess` in routes/authz.ts. The predecessor `assertCanManageIssueApprovalLinks` took a bare `companyId: string`, which is why master never tripped the guard. Apply the documented two-step pattern: `hasCompanyAccess` 404 gate, then `assertCompanyAccess` for the write-path membership checks. The sole caller reaches this through `getAccessibleResource`, which already applied both, so this is a no-op on that path; it is added rather than the redundant `assertCompanyAccess` simply dropped so the helper closes the oracle on its own terms and a second caller cannot reintroduce it. Caught by `authz-existence-oracle-guard.test.ts` in `General tests (server 1/4)` — the first CI run of the BLO-24699 work, which could not execute while #1293 was based on a branch (`pr.yml` triggers on `branches: [master]`).
1 participant
Resolves BLO-24699.
Stacked on #1271 — it targets that branch, not
master, becauseevaluateAgentIssueApprovalLinkAuthorizationis introduced there and does not exist onmasteryet. Review/merge #1271 first.Thinking Path
Two routes reach the same end state — a row in
issue_approvals— and only one of them ran the privilegedassertCanManageIssueApprovalLinksgate (role === "ceo" || permissions.canCreateAgents). #1271 closed the issue-scoped half of the gap; BLO-24699 was split out to rule on thecanCreateAgentshalf rather than let a live authorization relaxation ride a scoped fix.Ally's review on #1271 recommended the symmetric fix: copy the gate onto create. I ruled that down (option 3 in the issue), on measured evidence.
The gate is not a boundary. Enumerated against this company's roster on 2026-08-11: 16 agents, exactly 2 pass (
CEO,CTO), 14 fail. Those 14 were excluded from the link route and admitted by the create route — so the identical end state was always reachable by picking the other door. What the gate actually controlled was which door you used.Copying it onto create breaks the escalation path.
POST /companies/:companyId/approvalswithissueIdsis how an agent files arequest_board_approvalfor a human-only gate. Gating it would let those 14 agents file the card but not attach it to the issue it concerns — a context-free escalation reaching a human, which is the failure mode BLO-23036 exists to close. It also breaks BLO-23763 AC #4.Relaxing the link route discloses nothing. The one capability added is attaching a pre-existing approval.
GET /approvals/:idandGET /companies/:companyId/approvalsare gated byassertApprovalAccessAllowed→company_scope:read, the same check as create (server/src/routes/approvals.ts:225,:238,:245). Any agent that can file an approval can already read every approval in its company. So the delta is "attach an approval you can already read to an issue you are already authorized to mutate."So: option 1, and the equivalence is achieved by both doors calling one evaluator rather than by adding a second gate.
What Changed
POST /issues/:id/approvalsnow decides throughevaluateAgentIssueApprovalLinkAuthorization, replacing theassertAgentIssueMutationAllowed+assertCanManageIssueApprovalLinkspair. Both doors now reach the same verdict for the same (actor, issue) pair. The 409 checkout-conflict contract is preserved by the evaluator; it additionally honours the productivity-review grant (BLO-23036) and no longer seizes the issue's checkout lock merely to annotate it.DELETE /issues/:id/approvals/:approvalIddeliberately keeps the privileged gate, with a comment saying why: detaching is not reachable through create by any actor, so there is no second door to agree with, and it is the destructive direction. Curation stays privileged; attaching does not.POST /companies/:companyId/agent-hiresgains the same issue-scoped check oversourceIssueIds(BLO-24699 AC fix(adapter-utils): CAS-retry on concurrent SSH workspace restores #4 — the third instance of this shape). Itsagents:creategate bounded the exposure to the same 2-of-16 population but did not close it.Verification
server/src/__tests__/approval-link-route-equivalence.test.ts— the cross-route equivalence signal BLO-23763 declared it could not deliver, because it needs both routers in one harness. MountsissueRoutesandapprovalRoutesover one app (the union of their service mocks; no existing test file mounted both). 4 cases: unprivileged agent allowed on its own issue through either door, refused on a peer's through either door, the same retryable 409 on a peer'sin_progresscheckout through either door, and unlink still refused.server/src/__tests__/agent-hire-source-issue-authorization.test.ts— 2 cases pinning the hire-route check and that cross-company ids still fall through tolinkManyForApproval.403 {"error":"Missing permission to link approvals"}on link where create returns 201.pnpm --filter @paperclipai/server typecheck— clean.pnpm test:run:generalwas still running locally when this PR was opened; CIGeneral tests (server)is the gate.Risks
assertAgentIssueMutationAllowedfrom the attach route also drops its checkout-lock adoption side effect. The protective part (409 on another agent's active checkout) is preserved by the evaluator and is directly tested. Not seizing a lock in order to annotate an issue is the behaviour the evaluator's own header argues for.vi.resetModules(); a cold cache pushed one case past vitest's 60s default, so both carry an explicit 120s timeout. That is import cost, not test cost.gh pr list --state all --search "BLO-24699"and an open-PR title search for approval-link authorization both returned nothing. No existing PR covers this.Model Used
claude-opus-5
Checklist
gh pr list --state all --search "BLO-24699"and an open-PR title search for approval-link authorization both returned nothingFixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template.github/workflows/pr.ymltriggers only onpull_request: branches: [master], and this PR targetscto/blo-23763-approval-create-issue-authz. SoGeneral tests (server)— the CI job named as this issue's verifying signal — has never run at any head on this branch, and cannot until the base becomesmaster. The only workflow that runs here iscommitperclip PR Review. Merging fix(approvals): authorize issueIds on approval create (BLO-23763) #1271 first retargets this PR tomasterand the gate then runs for real. Flagging rather than silently checking this box.