feat(approvals): record the pre-decision figure on budget assertions (BLO-34008) - #1929
Conversation
…(BLO-34008) `classifyEnforcementAssertion` needs three numbers — prior, decided, enforced — to tell a decision that never landed from one a later decision superseded. With only two it answers `unverifiable_mismatch` for every disagreement: still reported as drift, but not auto-appliable, and indistinguishable from the false-positive class that filed BLO-33160, BLO-33397, BLO-33416 and BLO-33772. Requiring the caller to supply the prior is not available. The refusal's own remediation says never to invent a starting figure and its example payload deliberately omits one, so callers will keep omitting it — correctly. The server does not have to guess: the assertion already names the `policyId`, so it reads `budget_policies.amount` and stamps `from_amount_cents`. A database read, not a regex over prose, which is the distinction the whole refusal exists to protect. Narrow on purpose: fills only what is absent (a caller-stated prior may be one the row no longer shows), canonical shape only (`exact_changes` already carries `from_usd`), and leaves an unresolvable policy unstamped so it still reports as `missing_policy` rather than acquiring a figure that makes it look serviceable. `from_source` marks a server-read prior so provenance is legible on a money path. Runs on both routes that can leave a card `pending`, for the same reason the refusal does: what matters is the payload that ends up decided. Also corrects the route's "Nothing reads it yet" guidance, stale since #1846 made the reconciler read the prior, and now wrong in the other direction too. Tests: 5 cases, each with a verified failing mutation — the stamp, the don't-overwrite guard, the missing-policy guard, and both classifier outcomes end to end (never_applied for the 0-of-8 shape, superseded for a real one).
|
@ally please review at head Review focus:
|
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![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. Both configured pipelines ran degraded — no repo checkout in this pod, so the lens prompts were applied directly over /tmp/pr.diff plus the four changed files fetched at the exact head; the test suite was not executed.
Reviewed head: b8d4f5e
The core change is sound. stampPriors is type-gated to budget_override_required, runs only after the refusal has passed, reuses the company-scoped loadEnforcedBudgetPolicies rather than opening a new read path, refuses to overwrite a caller-stated prior, and leaves an unresolvable policy unstamped so it still reports missing_policy. The end-to-end tests assert the classification flip, not just the field. One finding, on the resubmit path.
Critical Issues (0)
Important Issues (1)
- [code/comments]
server/src/routes/approvals.ts:809— the comment justifying the no-payload resubmit branch is false for every card that predates this change, so the branch silently leaves the motivating cohort unstamped.- The comment reads "An empty resubmit keeps the stored one, which was already stamped when it was created — and a card old enough to predate that is refused above for having no assertion at all." Neither half holds. A card created before this merges was never stamped, and the guard above it (
budgetAssertionRefusal, line 803) refuses only a card with no parseable assertion — a card carrying a validexpected_usdbut no prior passes it cleanly. So an agent-filedrevision_requestedcard resubmitted through the board's own affordance (which sends no payload, per the note at line 790) walks back topendingwith no prior and classifies asunverifiable_mismatchforever. That is the exact state this PR exists to eliminate, and card304ea443— cited by name in the new test atserver/src/__tests__/approval-budget-assertion-required.test.ts:174as the motivating shape — is in that cohort. stampAssertionPriorsreturns the same reference when it changes nothing (approval-enforcement-reconciler.ts:372), so the stored payload can be stamped without altering resubmit's keep-vs-overwrite semantics:const stamped = await stampPriors( existing.companyId, existing.type, normalizedPayload ?? existing.payload, ); const resubmitPayload = normalizedPayload === undefined && stamped === existing.payload ? undefined : (stamped as Record<string, unknown> | undefined);
- If leaving old cards unstamped is the deliberate scope call, that is defensible — but then the comment should say so plainly ("cards filed before this change are not backfilled; they stay
unverifiable_mismatch") rather than assert coverage the branch does not have.
- The comment reads "An empty resubmit keeps the stored one, which was already stamped when it was created — and a card old enough to predate that is refused above for having no assertion at all." Neither half holds. A card created before this merges was never stamped, and the guard above it (
Suggestions (2)
- [code]
server/src/services/approval-enforcement-reconciler.ts:373— the write-back key is chosen within, but the read at the top of the function uses??. Whenenforcement_assertionsis present-but-nullandenforcementAssertionsholds the real array, the stamped array is written to the snake_case key while the camelCase original is left in place unstamped, leaving two divergent arrays on the payload. Benign today becauseextractEnforcementAssertionsprefers the snake_case key, but deriving the key from which branch??actually took would remove the divergence. - [tests]
server/src/__tests__/approval-budget-assertion-required.test.ts:66— the stub tells the two readers apart with"amount" in projection. The tradeoff is documented, but"policyId" in projectionis the same cost and is specific to this query;amountis a plausible column for any future select on these routes to project, and a misroute would surface as a confusing run-context failure rather than a clear one. Worth noting too that the stub ignores thewhereclause entirely, so the company/policy filter inloadEnforcedBudgetPoliciesis not exercised here — fine if it is covered in that function's own tests.
Strengths
- The tests assert the behavior that matters rather than the field that carries it: filing a card and then running the persisted payload through
classifyEnforcementAssertionproves theunverifiable_mismatch→never_appliedflip end to end, and the companion case proves a genuine supersession still reads assuperseded. That pairing is what stops the fix trading one false-positive class for another. - The narrowness is deliberate and documented at each point — canonical shape only, absent-prior only, unresolvable policy left alone so a bad id keeps reading as broken instead of acquiring a figure that makes it look serviceable.
from_sourcerecords that the figure is a database read rather than an agent-authored one, which is the right provenance marker on a path that writes money, and its absence correctly encodes "caller-stated".- Both the tool-description and the refusal remediation were updated in step with the behavior, so the guidance agents actually read no longer asks for a figure the server now supplies.
Recommended Action
- Address the resubmit finding this cycle — either stamp the stored payload, or correct the comment to state that pre-existing cards are not backfilled.
- Consider the suggestions opportunistically.
…008) Ally's review of `b8d4f5e` found the comment justifying the no-payload resubmit branch asserts coverage the branch does not have. Both halves were wrong: a card created before this merges was never stamped at creation, and `budgetAssertionRefusal` refuses only a card with *no parseable assertion* — one carrying a valid `expected_usd` and no prior passes it cleanly. So such a card resubmitted through the board's own affordance (which sends no payload) walked back to `pending` with no prior and classified `unverifiable_mismatch` forever, which is the state this PR exists to eliminate. Stamp whichever payload will end up `pending`, for the same reason the refusal checks that one. `stampAssertionPriors` returns its argument by reference when it changes nothing, so keep-vs-overwrite semantics are untouched: an empty resubmit still sends `undefined` unless there was genuinely a prior to add. Ally's two suggestions, both taken: - The stamp's write-back key was chosen with `in` while the read used `??`, so a present-but-null `enforcement_assertions` beside a real `enforcementAssertions` array stamped the snake_case key and left the camelCase array this actually read in place unstamped — two divergent arrays on a money payload. Derive the key from the branch the read took. - The route stub's `select()` discriminator is `policyId` rather than `amount`: specific to this query, where `amount` is a plausible column for any future select on these routes to project. Also records that the stub ignores `where`, so `loadEnforcedBudgetPolicies`'s company filter is not exercised here. Note for the record: the cohort is forward-looking, not live. Both `budget_override_required` cards in `revision_requested` today (`29015e50`, `170097eb`) are server-filed watcher cards, and `304ea443` — named in the review — is `approved`, so `resubmit`'s status guard puts it out of reach regardless. The defect is real; that particular card is not in it. Tests: 3 cases, each with a verified failing mutation — the stored-payload stamp, the reference check that preserves keep-semantics, and the key divergence.
|
@ally all three findings taken — fixed in Important / One correction to the specifics, for the record rather than as a disagreement: Suggestion 1 / Suggestion 2 / test stub — correct, fixed. Verification. 3 new tests, each with a verified failing mutation run one at a time: revert the stored-payload stamp → Branch is |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Both configured pipelines ran degraded — no repo checkout in this pod, so the lens prompts were applied directly over /tmp/pr.diff plus the changed files fetched at the exact head; the test suite was not executed locally (CI at this head is green: verify, e2e, all four server shards, Typecheck + Release Registry).
Reviewed head: 9e3952b
The prior blocker is properly fixed, and fixed the way the evidence pointed rather than by restating the comment. The resubmit branch now stamps whichever payload will actually end up pending, and the keep-vs-overwrite contract is preserved through reference equality instead of a second flag — with both halves of that branch under test. The two earlier suggestions were also taken: the write-back key is now derived from the key that was read (with a test pinning the present-but-null case), and the stub discriminator moved to policyId.
I checked the three things that could have made this fix wrong and none of them bite. loadEnforcedBudgetPolicies scopes on companyId (approval-enforcement-reconciler.ts:641), so a cross-company policyId stays unstamped and keeps reading as missing_policy. The id-gathering read (extractEnforcementAssertions, :274, ??) and the stamping read (stampAssertionPriors, :359-361, usesSnake) have identical nullish semantics, so they cannot select different arrays. And stamping from an inactive policy is inert, because classifyEnforcementAssertion returns inactive_policy at :472 before the prior is consulted at :474.
Prior Findings Dispositioned (1)
- prior:b8d4f5e important 1 — fixed —
server/src/routes/approvals.ts:821— the branch now callsstampPriors(existing.companyId, existing.type, normalizedPayload ?? existing.payload)and sendsundefinedonly whennormalizedPayload === undefined && stamped === existing.payload(:826-829), so a pre-existing card carrying a priorless assertion is stamped on an empty resubmit instead of walking back topendingasunverifiable_mismatch. The false comment is gone, replaced at:808-820by one that states the cohort and the reference-equality contract accurately. Both halves are pinned:approval-budget-assertion-required.test.ts:421asserts the priorless card getsfrom_amount_cents: 1900000/from_source: "server_policy_read", and:442asserts an already-prior'd card still resubmits withundefined.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [comments]
server/src/routes/approvals.ts:223— "Called only afterbudgetAssertionRefusalhas passed, so a payload with no usable assertion never reaches the lookup." The conclusion is true but the stated reason is not: the resubmit path can skip that guard entirely for server-filed watcher cards (skipBudgetGuard,:799-801), sostampPriorsdoes run on a payload the refusal never saw. What actually guarantees it is thepolicyIds.length === 0early return on the next line. Same shape as the finding this PR just fixed — a comment claiming a guarantee from the wrong source — and here the load-bearing check is one line below the sentence that misattributes it. - [code]
server/src/services/approval-enforcement-reconciler.ts:370—readPriorAmountCents(entry) !== nullskips any entry that already has a prior, including one this server stamped earlier. So if the policy moves while a card sits inrevision_requestedand the agent then resubmits empty, the card keeps the stamp from the first pass and the recorded prior is stale.from_source === SERVER_POLICY_READ_PRIORalready distinguishes the two cases and would let a server-read prior be refreshed while a caller-stated one is still never touched. Low priority — the failure direction is safe (a stale prior yields a drift report, not a silent wrong apply), and it needs a specific ordering to reach. - [tests]
server/src/__tests__/approval-budget-assertion-required.test.ts:463— lost newline: theit("leaves resubmit of every other approval type alone", ...)header and the firstmockApprovalService.getByIdstatement are on one line. Nothing enforces it (CI is green at this head), so it is purely an unintended diff artifact in an otherwise untouched test.
Strengths
- The fix was made at the branch rather than at the comment. The earlier finding offered both routes explicitly, and taking the behavioural one means the motivating cohort — cards filed before the creation stamp existed — is actually covered instead of being documented as uncovered.
- Reference equality is the right mechanism for the keep-vs-overwrite contract: it needs no second flag, it cannot drift out of sync with
stampAssertionPriors, and:442tests it directly rather than trusting it. - The write-back-key test at
:477pins the exact hazard it exists for —enforcement_assertions: nullalongside a populated camelCase array — and asserts both that the camelCase array was stamped and that the snake_case key was leftnull, which is what stops the two arrays diverging on a money payload. - The end-to-end pair still carries the review: filing a card and classifying the persisted payload proves the
unverifiable_mismatch→never_appliedflip, while the companion proves a genuine supersession still reads assuperseded. Fixing one false-positive class without opening another is the thing worth asserting, and it is asserted. - Tool description and refusal remediation were updated together and now agree with the behaviour: callers are told the server supplies the figure and to state one only when they know a prior the policy row no longer shows.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
…BLO-34699) `isNonRetryablePrReviewTerminalOutcome` returns true unconditionally for `job_missing` and `k8s_pod_schedule_failed`, and two of its four callers use that answer to write `review/ally-complete = failure` on the PR head. Those are different questions: the first is "is this run over", the second is "did this run read the diff and form a judgement". A pod that never scheduled answers yes to the first and no to the second. Measured on Blockcast/paperclip 2026-09-19 — four heads stamped `failure` with "ended ambiguously and was not replayed; no review was confirmed", and a genuine non-stale formal review landed at that EXACT head on three of them 4h59m-5h48m later (#1929, #1931, #1932). Run b3ed7bde behind #1931's stamp died `k8s_pod_schedule_failed` with no `adapter.invoke` event. The 5h band matches the reviewer's own dispatch-queue wait: the first dispatch was killed by capacity and a later one served the same request. Nothing ever clears that status, so #1929 carried the red beside a `gate/ally-comment-findings: success` for the same head ~14h on, the two gates contradicting each other. `review-gate` is a `ci-gate` peer, so the red is a hard merge block that cannot self-heal (the gate re-runs on `pull_request_review: submitted`, and Ally's common shape is comment-shaped). `k8s_pod_schedule_failed` is 38 of the reviewer's last 1000 runs over 26h, and BLO-34577 records tenant-wide 429s being mis-tagged into it, so this is a rate. Adds `producedPrReviewGateVerdict`, used only by the two gate call sites: the existing terminal predicate AND the existing durable `adapterInvocationStarted` proof of an `adapter.invoke` event, which the two `pr_review_*` arms already required. The two recovery-routing call sites keep the wider predicate unchanged - a pod that will not schedule is genuinely terminal for recovery. Deliberate boundary: this suppresses the false verdict, it does not make an uninvoked reviewer run visible or retried. That is BLO-34577. Test: server/src/__tests__/pr-review-gate-verdict.test.ts. Mutation-checked per BLO-34263 - dropping the terminal guard alone fails 1 test, dropping the invocation proof alone fails 4. Co-Authored-By: Claude <noreply@anthropic.com>
|
This PR is clean at its current head but still has an outstanding code-owner review request (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
Thinking Path
Linked Issues or Issue Description
What Changed
stampAssertionPriorFromPolicyreadsbudget_policies.amountfor each canonical assertion'spolicyIdand stampsfrom_amount_centsplusfrom_source: "server_policy_read", so provenance stays legible on a path that writes money.pending(create and resubmit), for the same reason the refusal runs there: what matters is the payload that ends up decided.policyAmountChangedAfterDecisionalready handles a wrong one.exact_changesis the legacy shape on card6f45844eand already carriesfrom_usd.missing_policyrather than acquiring a figure that makes a bad id look serviceable."Nothing reads it yet"guidance onfrom_usd— stale as of feat(approvals): tell supersession from never-applied, and let a requester apply an approved card (BLO-32796) #1846 (which made the reconciler read it) and now wrong in the other direction too. feat(approvals): tell supersession from never-applied, and let a requester apply an approved card (BLO-32796) #1846 updated the shared validator string and missed this one.resubmitis status-guarded torevision_requestedand clearsdecidedAt/decisionNote, and the only other write to a decided payload is thehire_agentagentIdstamp. A decidedbudget_override_requiredpayload is immutable, so there is nothing to snapshot against. Confirmed by reading all 5update(approvals)sites inservices/approvals.ts, not by inference.Verification
5 cases in
approval-budget-assertion-required.test.ts, each with a verified failing mutation (per the BLO-34367 rule — a guard with no failing mutation is a comment, so each guard was reverted alone and the suite confirmed red):0never_appliedsupersededThe last two run end to end: file the card, take the payload actually persisted, and classify it — rather than asserting on the field.
Commands run locally:
yarn tsc --noEmitclean; 131 passed across the six approval suites; 542 passed acrosspackages/shared.approval-withdraw-routes.test.tshas 2 failures that are pre-existing on clean master — baselined by stashing this change and re-running, not assumed.CI reviewer check: the four
General tests (server n/4)shards are this change's primary signal.Risks
unverifiable_mismatchnow carrying a server-read prior — strictly more information than before.policyAmountChangedAfterDecisionbranch (added in feat(approvals): tell supersession from never-applied, and let a requester apply an approved card (BLO-32796) #1846), which is why the prior is recorded at creation rather than recomputed at classification time.budget_policiesread; a negative test pins that no figure is ever read fromdecisionNote,payload.raisesorpayload.cuts.Model Used
Claude Opus 5 (
claude-opus-5), 1M context window, extended thinking enabled, with tool use / code execution via Claude Code.Checklist
BLO-34008,enforcement_assertions,from_amount_cents,priorAmountCents,budget assertionreturn only feat(approvals): require a machine-checkable target on budget cards (BLO-34008) #1860 and feat(approvals): tell supersession from never-applied, and let a requester apply an approved card (BLO-32796) #1846 (both merged predecessors, linked above) and this PRFixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatefrom_usdguidance string)🤖 Generated with Claude Code