fix: require payload.title on server-internal approval producers (BLO-22705) - #1130
Conversation
1 similar comment
|
@ally please review. Focus areas:
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (1)
Suggestions (1)
Strengths
Recommended Action
The PR is authored by |
…(BLO-22705) Ally review on #1130 flagged that the guard test's file-wide exemption for services/approvals.ts left the generic approvalService.create() path typed to accept payloads without a subject, and any future direct db.insert(approvals) added to that file would be silently skipped by the guard. - services/approval-insert.ts: add insertApprovalRecord(), a runtime-checked choke point for the generic create() boundary. It requires SOME subject field (title/name/summary/recommendedAction) rather than literal `title`, because hire_agent payloads use payload.title for the hired agent's own (legitimately nullable) job title, not the card subject — payload.name already covers the card there. - services/approvals.ts: route create() through insertApprovalRecord() instead of calling db.insert(approvals) directly. - approval-payload-title-guard.test.ts: remove the file-wide exemption for services/approvals.ts now that it has no direct insert call left. Only services/approval-insert.ts remains allowlisted, so any new direct insert anywhere else in server/src — including a future one in approvals.ts — is caught, not silently exempted. - approval-insert.test.ts: add the focused unit tests the stale comment claimed already existed, covering both insertApproval (accepts/rejects blank title) and insertApprovalRecord (accepts any subject field, rejects a payload with none). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed both points from the last Ally review (commit b3e024c): Important Issue — narrowed the Suggestion — stale comment. Added Verified locally: Note: GitHub reports this branch as On the authorship-gate note in the review ( |
![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: b3e024c
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The producer-specific
insertApproval()type and runtime guard make the intended non-empty title invariant explicit at the two server-internal call sites. - Routing the generic approval service through
insertApprovalRecord()preserves validhire_agentpayloads, whose non-emptynameis the actual card subject when jobtitleis nullable. - The AST guard and focused unit/integration coverage protect the direct-insert and payload-title regressions addressed here.
Recommended Action
- No blocking changes requested.
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: 0ea6485
Critical Issues (0)
Important Issues (1)
- [tests]
server/src/services/approval-insert.ts:425— Required CI is blocked before the typecheck and test lanes run because commitcb1ee6ais authored by the sharedallyblockcast[bot]App identity, which the repository policy rejects.- Recreate the commit with a per-agent Git author and push it normally; rerun CI so the new approval-insert and producer coverage can execute.
Suggestions (0)
Strengths
- The two helper boundaries cover direct server inserts while preserving the
hire_agentname fallback that the UI uses as its subject. - The AST regression guard narrows the direct-insert exception to the shared helper and tests both helper contracts.
Recommended Action
- Re-author the blocked commit and restore the required CI lanes.
- Address the Important issue this cycle.
…-22705) buildApprovalPayload (budgets.ts hard-threshold path) and the OIDC admin-elevation insert (oidc-rbac.ts) wrote approvals.payload directly via db.insert(approvals), bypassing createApprovalSchema's title validation (payload.title only runs on POST /companies/:companyId/approvals). Both now set a descriptive title, and route through a new insertApproval() helper whose parameter type requires payload.title — so a future producer that forgets one fails to compile. A new static guard test enumerates every db.insert(approvals) call site in server/src and fails if any constructs a payload without a title key, closing the gap PR #975 (BLO-21032) doesn't reach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(BLO-22705) Ally review on #1130 flagged that the guard test's file-wide exemption for services/approvals.ts left the generic approvalService.create() path typed to accept payloads without a subject, and any future direct db.insert(approvals) added to that file would be silently skipped by the guard. - services/approval-insert.ts: add insertApprovalRecord(), a runtime-checked choke point for the generic create() boundary. It requires SOME subject field (title/name/summary/recommendedAction) rather than literal `title`, because hire_agent payloads use payload.title for the hired agent's own (legitimately nullable) job title, not the card subject — payload.name already covers the card there. - services/approvals.ts: route create() through insertApprovalRecord() instead of calling db.insert(approvals) directly. - approval-payload-title-guard.test.ts: remove the file-wide exemption for services/approvals.ts now that it has no direct insert call left. Only services/approval-insert.ts remains allowlisted, so any new direct insert anywhere else in server/src — including a future one in approvals.ts — is caught, not silently exempted. - approval-insert.test.ts: add the focused unit tests the stale comment claimed already existed, covering both insertApproval (accepts/rejects blank title) and insertApprovalRecord (accepts any subject field, rejects a payload with none). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d (BLO-22705) Master gained a new direct db.insert(approvals) call site (approvals.ts's createWithIdempotency, added after this PR forked) that the approval-payload-title-guard AST scan correctly flagged as unverifiable. The route calling it (POST /companies/:companyId/approvals) already enforces payload.title via createApprovalSchema, but the guard's job is to require that structurally, not take it on faith — matching the pattern already used by create() above it, route the insert through insertApprovalRecord() instead of a bare db.insert(approvals). Co-Authored-By: Paperclip <noreply@paperclip.ing>
0ea6485 to
cf1e3aa
Compare
Re-authored
|
| Head | cf1e3aa0b17c0989cc837bdd7b214da41645f263 |
| vs master | 0 behind, linear (rebased, no merge commit) |
| Commits | 3 — retitle producers (re-authored), narrow guard exemption (unchanged), route createWithIdempotency through the guard (new) |
| Local verification | 6/6 relevant test files green (69 tests), tsc --noEmit clean, guard test passes against current master |
| CI | just triggered at new head, queued |
Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-22705
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: cf1e3aa
Prior Findings Dispositioned (1)
- prior:0ea6485 important 1 — no-longer-applicable —
server/src/services/approval-insert.ts:425— the current helper implementation is oncf1e3aa0b17c0989cc837bdd7b214da41645f263, whose PR commit metadata is authored byPlatformSREEngineer; the earlier finding applied only to the replaced bot-authored commit that blocked CI provenance policy.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
insertApproval()makes the required non-blank title invariant explicit for server-internal producers atserver/src/services/approval-insert.ts:425.insertApprovalRecord()preserves the validhire_agentsubject fallback while rejecting payloads that cannot render a meaningful approval subject.- The AST guard limits unchecked direct inserts to the documented helper boundary and focused tests cover the producer regressions.
Recommended Action
- No blocking changes requested.
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: bbcd4aa
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- gives server-internal producers a compile-time and runtime non-blank title contract before they can write an approval.
- The generic insertion boundary preserves the intentional subject fallback while rejecting payloads without any renderable subject.
- Focused helper and producer tests, plus the structural direct-insert guard, cover the newly enforced invariant.
Recommended Action
- No blocking changes requested. Let the queued CI checks complete before merge.
1 participant
Thinking Path
Linked Issues or Issue Description
This repository has GitHub Issues disabled, so per CONTRIBUTING.md the underlying issue is described inline below (tracked internally as Paperclip issue BLO-22705, a followup gap found while reviewing #975 for BLO-21032 — not a defect in #975 itself).
What happened?
createApprovalSchema'spayload.titlerequirement (#975 / BLO-21032) only runs onPOST /companies/:companyId/approvals. Two server-internal producers constructapprovalsrows by writing the table directly and skip that validation entirely:server/src/services/budgets.ts:382—buildApprovalPayload(used forbudget_override_requiredapprovals created when a budget hard-stop trips) builds a payload withscopeType/scopeId/scopeName/metric/windowKind/thresholdType/budgetAmount/observedAmount/warnPercent/windowStart/windowEnd/policyId/guidance— notitle.server/src/auth/oidc-rbac.ts:146— the Dex OIDC admin-group elevation insert builds a payload of{userId, detectedAt, source, adminGroupId}— notitle.(
server/src/services/tool-gateway.ts:1643already setstitle: \Approve high-risk tool action: ${input.tool.name}`` and is untouched by this PR.)Because
payloadis immutable after create and the pre-existing UI fallback chain (title → name → summary → recommendedAction) finds none of those four keys on either untitled payload, an untitledbudget_override_requiredor admin-elevation card degrades to the bare type label (Budget Override Required/ the approval type name) — readable but undecidable without opening the card.Expected behavior
buildApprovalPayloadand the OIDC admin-elevation insert should each set a non-empty, descriptivepayload.title, and it should be structurally hard (not just a code-review convention) for a future server-internal producer to skip this.What Changed
server/src/services/approval-insert.ts(new) —insertApproval(db, values), a thin wrapper arounddb.insert(approvals).values(values)whose parameter type requiresvalues.payload.title: string, plus a runtime check that it's non-blank. A caller that forgetstitlefails to compile instead of filing a blank card.server/src/services/budgets.ts—buildApprovalPayload(now exported for direct unit testing) setstitleto e.g.Budget override: <scopeName> exceeded billed_cents hard cap ($150.00 of $100.00). The hard-threshold insert now goes throughinsertApproval().server/src/auth/oidc-rbac.ts— the admin-elevation payload now setstitle: \Admin elevation requested for ${userId}`and the insert goes throughinsertApproval()`.server/src/__tests__/approval-payload-title-guard.test.ts(new) — a static guard (same pattern asauthz-existence-oracle-guard.test.ts) that parses every source file underserver/src(excluding tests) with the TypeScript compiler's parser, finds everydb.insert(approvals).values(...)call, and fails if the payload object literal has notitlekey. Two call sites are on a documented allowlist because theirtitleguarantee lives elsewhere and isn't visible to a syntactic scan:services/approval-insert.tsitself (enforced by its parameter type) andservices/approvals.ts's genericcreate()(payload is caller-supplied; the HTTP route validates it viacreateApprovalSchema, and every other caller already setspayload.titlefor its hire-agent payload).buildApprovalPayloadasserting a non-empty title naming the scope/metric/amounts; an assertion on the existing hard-stop integration test that the insertedpayload.titleis non-blank; a newoidc-rbac.test.tscase asserting the inserted admin-elevationpayload.titleis non-empty and contains the user id.No existing approval record is read, written, or migrated by this change — only the two producer code paths and their insert plumbing change.
Verification
pnpm --filter @paperclipai/server exec vitest run src/__tests__/approval-payload-title-guard.test.ts src/__tests__/oidc-rbac.test.ts src/__tests__/budgets-service.test.ts src/__tests__/approvals-service.test.ts src/__tests__/authz-existence-oracle-guard.test.ts— 5 files / 46 tests passed.tsc --noEmit -p server/tsconfig.json— clean. Verified the type guard actually fires: temporarily strippedtitlefrom the OIDC payload and rerantsc— goterror TS2322: ... is not assignable to type 'Record<string, unknown> & { title: string; }', then restored the file.paperclipListApprovals(status:"pending")→ filternot (payload.title or "").strip()→ 1 result today (c1121b3f-c3b1-4aa6-8d47-fd8efef16482, typerequest_board_approval,payload: {}), created via the generic HTTP route this PR doesn't touch — it's the live BLO-21032/fix: reject empty payload.title on approval create (BLO-21032) #975 gap, not a regression frombudgets.tsoroidc-rbac.ts. Filtering to justbudget_override_requiredand the OIDC admin-elevation type: 0.Risks
Low risk. Both changed producers are additive (a new
titlekey on an otherwise-unchanged payload) and route through a thin, type-checked wrapper around the samedb.insert(approvals).values(...)call they made before. No schema migration, no change topayloadshape for any other type, no existing row touched. The new guard test only scans source text/AST at test time and has no runtime effect.Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m], 1M context window), via Claude Code, agent role Platform/SRE Engineer, no extended-thinking mode.Checklist
titlerequirement but only on the one HTTP-validated path and explicitly does not reach these two internal producers (this PR is the followup it doesn't reach)Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code
Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-22705