The budget gate guarded a table with no reader - #256
Merged
Conversation
MEASURED FIRST, because the premise could have been wrong. It was not.
`budget.override` — Director-only, on CAPABILITIES_REQUIRING_A_STATED_REASON —
had ONE requireCapability call site in the tree: `adminAdjustBudget`, writing
`Budget.allocatedCents`. Nothing in apps/web/src reads that table. The only
`db.budget.*` calls anywhere are that action's own findUnique and update, and
`allocatedCents` appears nowhere outside schema.prisma and the preview seed. So
`every-listed-capability-is-wired.test.ts` was passing on that id VACUOUSLY.
The power the capability NAMES was exercised somewhere else. Club money is
`BudgetLine`; every write to it went through `canManageFinance`, which returned
true for an OSE Director on EVERY club in the institution. Seven allocation
writes across `orgs/[slug]/finance/actions.ts`, zero requireCapability calls in
that file, zero stated reasons. A Director could rewrite any club's allocation,
retire any club's line, or replace a club's whole plan from a spreadsheet, and
the trail said only that somebody with finance authority did something.
THE FIX. `financeAuthorityOf` (lib/rbac.ts) now answers WHERE the authority came
from, not merely whether it existed: SEAT for the club's own ACTIVE president or
VP of Finance, OSE_OVERRIDE for a Director holding no seat in the club, null for
everybody else. `canManageFinance` is derived from it rather than restated, so
the two cannot drift into two answers to one question. The seat is asked FIRST,
so a Director who is also this club's treasurer is doing the club's own work and
is not interrogated — and the same person is still an override next door.
`requireFinanceManager` then routes an OSE_OVERRIDE through
`requireBudgetOverride` (lib/finance-override.ts) for the four writes that change
what a club may SPEND: Finance.EditLine, CloseLine, ReopenLine, Import. That is
`requireCapability("budget.override", { statedReason })` — the gate that already
existed, refusing on the server, writing a DENY row for a groundless attempt and
putting the operator's own sentence verbatim on the ALLOW.
NOT GATED, and each named rather than omitted: Finance.SaveForecast writes a
projection, never authority. Finance.PostLedger and Finance.ReverseLedger record
that money MOVED — both already refuse without a description, or a reason code
plus a note. Whether an OSE ledger posting is itself an override is a real
question and a separate one; it is stated in finance-override.ts and asserted by
name in the tests so deciding it later is a decision rather than a discovery.
THE UI EXTENDS THE PRECEDENT, and is not the control. `components/forms/
ReasonField.tsx` — the asterisk the onboarding decline already uses — gains an
optional controlled mode, because the add-a-line form asks "did you mean to
re-budget?" and resubmits itself, and React empties an uncontrolled box when a
server action resolves. The close dialog reuses the note it ALREADY refuses
without rather than asking the same person twice, raising only its floor from 8
to 12 for an override and saying so. Reopen was one click; for an override it
now opens a dialog. A club officer sees none of this.
PR #214's DECISION IS NOT REOPENED. `adminAdjustBudget` stays, exempted with its
written reason. What changed is the ground under the exemption: it is no longer
"the only wiring of a listed capability", so deleting it would no longer weaken a
control. Both stale sentences that said "delete this in the same diff" are
corrected, along with the sweep header that recorded it as deleted.
THE GUARD IS A CLASS CONTROL, not a check of today's call site.
`spending-authority-is-gated.test.ts` walks the AST of every tracked source file,
finds each Prisma write on BudgetLine/Budget whose payload carries an allocation
column (plus every create and delete), resolves it to the OUTERMOST enclosing
function, and requires that function to reach a gate. It also derives
BUDGET_AUTHORITY_ACTIONS from the source and compares it with the declared list
in both directions, so a gated write under a name nobody listed — a gate present
and inert — fails too. Payloads only, never `where`: postToLedger names
`closedAt: null` as a compare-and-swap, and counting filters would read every
guarded movement as a retirement.
FIVE MUTATIONS RUN, each md5-verified as applied and restored:
strip requireCapability from the gate → guard test 1 red, endpoint test 20 red
unwire requireBudgetOverride → "routes an OSE override" red
new ungated write, same file → named grantExtraBudget() at its line
new ungated write, NEW file → named bumpBudget() at its line
gated write under an unlisted action → named "Finance.RaiseLine"
NEIGHBOURING BRANCHES, asked deliberately: a Director with an EXPIRED seat here
is an override (tested — reading storedStatus would have let a past treasurer
keep editing). A third model carrying `budgetedCents` would be invisible to the
walk, so the scan reads schema.prisma and fails if one appears. Raw SQL sits
under $allOperations and no AST walk keyed on the model API sees it, so that is
checked too — from the SQL itself, because a file-level grep reported
lib/tenancy/registry.ts, which only mentions both in prose.
Delegation is deliberately NOT honoured: the finance write resolves no delegated
authority of its own, and opening that door would admit a delegate the command
then refuses.
CONCURRENCY: the existing `db.auditEvent.create` in requireFinanceManager is
untouched — the capability check is added AFTER it, and no audit write site was
restructured.
307 tsc errors (parity with main), lint clean, jest 3 failed suites (the same
three that fail on pristine main), 5578 passed.
NOT DONE, and not claimed: no Playwright spec. The finance page has no e2e
harness to extend and one written blind would be an overclaim; the server-side
proof is `an-ose-override-with-no-reason-writes-nothing.test.ts`, which posts to
the exported server actions with no form and asserts no statement was issued.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
satvikOS has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Comment |
This was referenced Aug 25, 2026
satvikOS
added a commit
that referenced
this pull request
Aug 25, 2026
…te that (#264) MAIN IS RED AT c28c64e. Two PRs that were each green apart broke it together, which is the shape CI cannot see: every branch is tested against the main that existed when it ran, and neither #254 nor #256 could observe the other. #254 made `recordAuditEvent` READ THE TENANT'S LAST ROW to chain this one to it, and wrap that read-and-write in a transaction when handed the top-level client (`audit-record.ts:653`). #256 landed two suites whose `@/lib/db` doubles were written when a refusal wrote exactly one `auditEvent.create` and nothing else. The result is `tx.auditEvent.findFirst is not a function`, thrown from inside the capability guard, so a test asserting the refusal MESSAGE got a TypeError and the server action returned "Something went wrong on our side". This is the concrete form of the cost recorded in task #32: the second round trip does not just add latency, it CHANGES THE CONTRACT every caller's double must satisfy. Three changes, all to the doubles, none to what the tests claim: `auditEvent.findFirst` returning `null` — the honest answer. These fixtures have no prior row, and an unchained first row is exactly what the production code handles when a tenant's log is empty. `$transaction` EXECUTES its callback instead of throwing. It was a tripwire meaning "a refused action reaches no write", which was true when a refusal wrote one row and is false now that a refusal legitimately opens a transaction to chain its DENY row — so the tripwire was firing on the very row the gate exists to write. The `budgetLine.create`/`.update` traps stay, so a written line still fails the test loudly, which is the claim that actually matters. The two positive cases now assert `["budgetLine.create"]` rather than `["$transaction"]`. Not a weakening: the action reaches the real write and now NAMES it, where "it got as far as opening a transaction" was the vaguer claim that only held because the double threw before its callback ran. Measured against the documented baselines: 3 failed suites / 5698 passed — the same three stale-generated-client suites that fail on pristine main — and tsc at 307, the baseline exactly. Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The claim, measured before anything was written
The premise held. Every number below was taken from this tree, not assumed.
budget.overrideexists and is checkedlib/admin/capabilities.ts:176, OSE_DIRECTOR, and onCAPABILITIES_REQUIRING_A_STATED_REASONrequireCapability("budget.override")site in the whole tree:adminAdjustBudget, writingBudget.allocatedCents.db.budget.*appears twice in non-test source — that action's ownfindUniqueandupdate.allocatedCentsappears nowhere outsideschema.prismaand the preview seed.grep requireCapabilityinorgs/[slug]/finance/actions.ts→ 0 hits, across 7BudgetLinewrite sites.requireFinanceManagerwrote anAuditEventwithoutcomeand noreasonfield at all.canManageFinanceopened withif (isOseDirector(ctx, org.institutionId)) return true— every club, no seat required.So
every-listed-capability-is-wired.test.tswas green onbudget.overridevacuously: the declared control had no reader, and the thing with the readers had no control.What changed
financeAuthorityOf(lib/rbac.ts) now answers where the authority came from rather than only whether it existed:SEAT— the club's own ACTIVE President or VP of FinanceOSE_OVERRIDE— an OSE Director holding no seat in this clubnull— everybody elsecanManageFinanceis now derived from it rather than restated, so the predicate that decides whether and the one that decides in what right cannot drift into two answers to one question. The seat is asked first: a Director who is also this club's treasurer is doing the club's own work and is not interrogated — and is still an override in the club next door.requireFinanceManagerroutes anOSE_OVERRIDEthroughrequireBudgetOverride(lib/finance-override.ts) for the four writes that change what a club may spend:Finance.EditLineFinance.CloseLineFinance.ReopenLineFinance.ImportThat call is
requireCapability("budget.override", { statedReason })— the gate that already existed. It refuses on the server, writes aDENYrow for a groundless attempt, and puts the operator's own sentence verbatim on theALLOWrow'sreason, the field/admin/auditprints and searches. It is also threaded onto the club's ownrecordAdjustmentmetadata asoverrideReason, so the next treasurer reads why OSE changed their line without holdingaudit.view.Deliberately not gated, named rather than omitted
Finance.SaveForecastwritesforecastCents— what a club expects to spend, never what it may.Finance.PostLedger/Finance.ReverseLedgerrecord that money moved. Both already refuse without a description, or a reason code plus a note over the same 12-character floor. Whether an OSE ledger posting is itself an override is a real question and a separate one — it is stated infinance-override.tsand asserted by name in the tests, so deciding it later is a decision rather than a discovery.The UI extends the precedent, and is not the control
components/forms/ReasonField.tsx— the asterisk the onboarding decline already uses — gains an optional controlled mode, because the add-a-line form asks "did you mean to re-budget?" and resubmits itself, and React empties an uncontrolled box when a server action resolves. An uncontrolled field would have posted the confirmation with the reason blank.The close dialog reuses the note it already refuses without rather than asking the same person the same question in two boxes — raising only its floor from 8 to 12 for an override, and saying which applies. Reopen was one unguarded click; for an override it now opens a dialog. A club officer sees none of this.
PR #214's decision is respected, not undone
adminAdjustBudgetstays, exempted with its written reason and its standing control. What changed is the ground under the exemption: it is no longer "the only wiring of a listed capability", so deleting it would no longer weaken anything — it is now an ordinary product question about whether the console wants a form for a table with no readers.Three sentences that said the resolution meant "delete this in the same diff", and the sweep header that recorded it as already deleted, are corrected.
every-server-action-has-a-callerandevery-listed-capability-is-wiredboth stay green.The guard is a class control
spending-authority-is-gated.test.tswalks the TypeScript AST of every tracked source file, finds each Prisma write onBudgetLine/Budgetwhose payload carries an allocation column (plus every create and delete), resolves it to the outermost enclosing function, and requires that function to reach a gate. A file-level check would pass the moment the file contained a gate anywhere — which is exactly where somebody working on budgets is already typing.It also derives
BUDGET_AUTHORITY_ACTIONSfrom the source and compares it with the declared list in both directions, so a gated write under a name nobody listed — a gate present and inert — fails too.Payloads only, never
where:postToLedgernamesclosedAt: nullas a compare-and-swap, and counting filters would read every guarded movement as a retirement (the mistakeledger-single-writer.test.tshad to correct in its own regex).Five mutations, each md5-verified as applied and restored
requireCapabilityfrom the gaterequireBudgetOverridefrom the chokepointgrantExtraBudget()at its linebumpBudget()at its line"Finance.RaiseLine"Neighbouring branches, asked deliberately
storedStatuswould have let a past treasurer keep editing with nothing recorded.budgetedCentswould be invisible to the walk, so the scan readsschema.prismaand fails if one appears.$allOperations; no AST walk keyed on the model API sees it. Checked — from the SQL itself, because a file-level grep reportedlib/tenancy/registry.ts, which mentions both only in prose.financeAuthorityOf; asserted anyway, so a future widening of that predicate is caught by the capability table rather than silently handed institution-wide budget power.Concurrency
The existing
db.auditEvent.createinsiderequireFinanceManageris untouched — the capability check is added after it, and no audit write site was restructured. A groundless override leaves two rows:Finance.*saying the authority was there, andAdmin.budget.override/DENYsaying the override was refused. Two true statements; thatFinance.*row has never meant "the write happened".Verification
tsc --noEmit→ 307 errors, parity with pristine main (all from a stale generated Prisma client)next lint→ cleanjest→ 3 failed suites, 5578 passed — the same three that fail on pristine main (connectors/audience,nothing-manufactures-the-member-seat,identity/onboarding-form, allObject.values(<PrismaEnum>)against the stale client)rbac.test.tsNot done, and not claimed
No Playwright spec. The finance page has no e2e harness to extend, and one written blind would be an overclaim. The server-side proof is
an-ose-override-with-no-reason-writes-nothing.test.ts, which posts to the exported server actions with no form and no browser and asserts both the refusal and that Prisma was never asked to write anything.Budgetis still a table nothing reads. This diff binds the capability to the money that moves; it does not decide the console's Budget form. That remains #214's open product question.🤖 Generated with Claude Code