Declare the inbound Slack events endpoint on the capability ratchet - #132
Merged
Conversation
Main went red on a collision git could not see. #96 added surfaces.ts — a ratchet requiring every API route appear in exactly one list, so a new endpoint cannot ship without its author either binding it or writing down the deferral. #98 added /api/integrations/slack/events. Neither branch touched the other's file, both were green, and the two together fail: ● every API route declares its capability, or declares that it has not › no handler is unaccounted for + "/api/integrations/slack/events" This is the ratchet working, not a flaw in it: the endpoint really was unaccounted for. The reason it is DEFERRED rather than BOUND is read off the handler rather than assumed. Slack is the caller and the signature over the raw bytes is the authentication, so there is no session to gate; and one delivery resolves through teamId to every institution that connected that workspace, so there is no single tenant whose capability could be consulted. That is the same shape as the other infrastructure entries, so it sits with them. Gates from a clean worktree, exit codes captured before any pipe: prisma generate 0 / tsc --noEmit 0 (tsc 5.9.3, so not a silent 127) / jest 141 suites, 2183 passed / next build 0. Negative control, read per test rather than by suite exit code: deleting the entry flips exactly "no handler is unaccounted for" red and restoring it green. 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 reached
Next review available in: 14 minutes Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
satvikOS
pushed a commit
that referenced
this pull request
Aug 21, 2026
Main moved to 1ad75a5 while this merge was being verified. #132 declares `/api/integrations/slack/events` on the capability ratchet — the same red this branch had just fixed, arriving from the other direction, because the endpoint really was unaccounted for on main and both of us met it. One conflict, one key, two rationales. Resolved to MAIN's entry verbatim, so `surfaces.ts` is now byte-identical to `origin/main`. Its reason is the better informed of the two: it names the case where one delivery resolves through `teamId` to more than one institution, which is why there is no single tenant to ask about — where this branch's text said only that the tenant is derived from the verified body. Two rationales for one key is exactly the drift this ratchet exists to prevent, so there is one. Counts re-derived on the new merged tree rather than assumed unchanged: 47 models, 28 carrying `institutionId`, TENANT_SCOPED 28 + PLATFORM_GLOBAL 5 + UNENFORCEABLE 14 = 47. ADR-0021 is still this branch's alone (main has 17 ADR files, this has 18) and `20260820220000_seat_functions_and_tenant_config_packs` still collides with no migration on main or on any other remote branch. Gates re-run in full on the second merge, exit codes captured before any pipe: `prisma generate` 0 · `tsc --noEmit` 0 (tsc 5.9.3) · `jest --ci` 0 (149 suites, 2297 passed / 1 skipped) · `next build` 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
satvikOS
pushed a commit
that referenced
this pull request
Aug 21, 2026
#132 landed the same declaration on main while this branch carried its own. Both are additions to one object literal, in different places, so git merged them without a word and TypeScript refused the result: TS1117, "an object literal cannot have multiple properties with the same name" — one cause, three red jobs (type check, next build, the container build). Main's wording is kept because it is the better one: it names the reason there is no single tenant to ask about, which is that one delivery resolves through teamId to every institution that connected the workspace. Mine only said there is no session. The prose count stays at twenty-three, re-derived rather than inherited: 1 bound + 23 deferred = 24 = `find src/app/api -name route.ts | wc -l`, with no key appearing twice. #132 left it reading twenty-two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
satvikOS
added a commit
that referenced
this pull request
Aug 21, 2026
* feat(identity): a term window an officer can actually write, in Simon's own calendar rbac.ts has compared [startDate, endDate) on every decision since Tenure@46e1c46, but nothing could write one: startDate took the schema default and endDate was set only by the revocation itself. The rule was real and unreachable. term-window.ts is the write side. A form collects DAYS, authorization compares INSTANTS, and every conversion goes through lib/time.ts against the institution's zone rather than the server's — a term ending 15 May parsed as UTC midnight revokes the officer at 8pm on the last day of their own term. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(identity): one clock per request, so eighteen decisions cannot disagree assignmentInEffect takes its clock as a required argument — the compiler catches a MISSING one, but not eighteen call sites each reading their own new Date(). requestClock() is react/cache'd, so the default is one instant per request and passing a different one is an explicit act (which is what tests do). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(identity): separate the create and edit reads of a term window An existing assignment always has a start date, so parseTermWindowEdit returns a non-optional one and requires the field; a blank first day on an edit is a slip, not 'starts now'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(identity): write the term down — dated seats on both roster surfaces The read side has compared [startDate, endDate) since Tenure@46e1c46; nothing could write one. startDate took the schema default and endDate was stamped only by the revocation itself, so 'expired officers lose authority promptly' still meant 'somebody remembers to press End term'. - assignMember / adminAssignSeat take a first and last day, read as days in the institution's timezone. - setTermDates / adminSetAssignmentDates reschedule an existing term. ALUMNI is refused: that endDate is the record of when access was revoked. - Both roster surfaces put relation-loaded rows through withEffectiveStatus, so a seat whose last day has passed cannot render 'Active' beside a person the server refuses on every request. - role.schedule is a separate capability id at the same minRole, so the audit row says which of assign/remove/schedule happened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): the seat census reads the window, not the label A writable end date makes the census wrong in two directions rule 1 and rule 2 already forbid: a president whose last day was in May still FILLS the seat in September and is named as the current holder on the handoff packet, and a successor placed as ACTIVE from next August fills it today. toSeatFacts now takes the evaluation clock and narrows each assignment's status by its own window — narrowing, not filtering, so a pending term reads INCOMING rather than making the seat vanish. Every one of the ten call sites is a compile error until it passes a clock. The pure window rules move to lib/effective-status.ts so seats.ts can use them without importing the module that opens a Prisma client; rbac.ts re-exports them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(identity): the term survives PostgreSQL, and the badge gate term-window.itest.ts writes a term from two date keys and reads it back: stored as midnight in Rochester (2026-08-15T04:00Z, not T00:00Z), returned by the authorization filter at 23:59:59.999 on the last day and refused at 00:00:00.000 the next — with the status column asserted ACTIVE throughout. Plus the census across the same boundary, and the 25-hour fall-back day. The scanner gains the render-side gate: a file that draws AssignmentBadge must narrow through withEffectiveStatus first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(identity): name the zone, not the city, in the integration test fork-prevention scans *.itest.ts — its exemption is *.test.ts only — so a tenant's city in a comment is a tenant literal like any other. The assertions are unchanged; they were always about the IANA zone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(backlog): tick effective dates, with the half that was still missing named The read side shipped in #88; the window it read could never be written. Records the boundary choice (last day inclusive), the timezone rule, the precedence between dates and status, and the two silent-wrong-answers closed on the way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(identity): the term, in a browser — and the way back from a mistyped one Three specs against a real server and a real database: the inline refusal for a last day before the first; a term set to June that takes the club away in August (the row still reads ACTIVE — the roster prints 'term ended on its own, no revocation' exactly when a row is ALUMNI by dates and not by a human edit); and a term that has not opened, where the VP of Finance previews the budget and is told it is read-only. The e2e surfaced a real trap while being written: once a term expired by date the club roster had no form on the row, so a president who mistyped a date locked a board member out with no way back. A term that ended BY ITS DATES was never revoked, so it stays editable; a genuinely revoked row still gets no form, and setTermDates refuses it server-side either way. Both e2e controls: breaking orgRolesFor reddens the pending case only, breaking assembleUserContext reddens the expired case only — the two narrowing points are independently load-bearing. The spec restores what it changes, verified by running roster/handoff/club-cards on the state it leaves behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): five findings from review, each verified against the code first 1. adminTransferSeat silently discarded the typed term. The console renders ONE form with two submit buttons, so the date fields post to whichever was pressed — an administrator who filled in a term and pressed Transfer got a term starting now and never ending, with no message. 2. The zero-president guard fired on every date edit of a SHADOW president. effectiveStatus is the MINIMUM of status and window, so a shadow row can never read ACTIVE; asking only whether the NEW window grants was therefore always true of one. removesActiveAuthority compares BOTH windows, and the test that pins it goes red under the old one-sided form. 3. assignMember threw a plain Error for a bad date — the message the person needs, on the one field where a typo is likely, escalating to a card that cannot carry it. It is reportable now, behind a client form, like the term editor beside it. 4. The DST fixture added a second seat to the org whose census the suite asserts has exactly one. Its own club now, so neither describe depends on the other. 5. The e2e pinned fixed 2027 dates: the pending case becomes ACTIVE on new year's day. All offsets are relative to the run date. The fork-prevention ratchet caught a real improvement on the way: the roster's placeholder address moved out of the page and became student@eligibleDomain(), so the allowance was removed rather than relocated. Ceiling 34 -> 33. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(e2e): locate the roster email field by label, not by the tenant's domain Two existing specs filled it via getByPlaceholder("student@<domain>"). That placeholder is now an example address in the tenant's OWN domain, resolved from lib/tenant/eligible-domain.ts, so the locator stopped matching — CI caught it where my partial local runs had not. Fixed by asking for the field by its label. Pinning a spec to a placeholder's text put a tenant literal in the suite and would have broken the moment a second institution is served; a label is what the field IS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(admin): the placement form reports its refusals, and the spec restores on failure Two more findings, both verified first. adminAssignSeat and adminTransferSeat threw refusals nobody could read — true before this change ('that person already holds that seat') and worse after it, because a last day before the first is the refusal an operator is most likely to trip. Both are reportable now, behind SeatPlacementForm: one <form> because ConfirmInlineSubmit needs the picker's hidden inputs to survive the dialog, two useActionState hooks because the two buttons are genuinely different operations and a stale refusal from one must not appear under the other. The e2e restored the roster inline, so a test failing halfway left a seeded member expired for every spec after it. Restoration moved to afterEach, which skips rows the run never reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(backlog): correct the control count and record the browser evidence 12 controls, not 8, and two of them are worth reporting rather than hiding: one stayed green on a rename that left the matched substring in place, and one was run before committing and had its subject reverted by git checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): a term with no first day starts NOW, so a past last day is a refusal Three findings from an adversarial pass over #103, each verified against the code before it was changed. 1. `parseTermWindow` skipped its own "last day before the first" refusal whenever the FIRST DAY WAS BLANK, because the guard reads `if (startDate && endDate && …)` and a blank first day leaves `startDate` undefined so Prisma's `@default(now())` applies. Blank is the shape both roster forms invite — "Dates are optional" — so `lastDay: 2020-01-01` with no first day was accepted, and stored a window that ended before it began. Nothing surfaced: the assignment was created, granted nothing, told its holder "Your term runs Aug 20, 2026 – Jan 1, 2020", and the duplicate-holder guard (which reads the STORED status) then refused to place them again, because the dead row still counts as holding the seat. `parseTermWindow` now takes the instant that default would land on, and refuses. Required, not defaulted, for the reason the rest of this change set already gives: a defaulted clock is a call site that silently opts out. `parseTermWindowEdit` deliberately does NOT inherit the rule and no longer routes through `parseTermWindow` — an edit has an explicit start, so a term wholly in the past is how one is ended retroactively, which is the documented way back from a mistyped date. Both directions are tested. 2. The zero-president guard decided its two halves at two instants. It asked `removesActiveAuthority(…, ctx.evaluatedAt)` and then counted the other presidents at `assignmentInEffect(requestClock())` — the exact "one request, several clocks" this change set exists to remove, inside the guard it adds. Both now read `ctx.evaluatedAt`, in `setTermDates` and in `transitionAssignment`. 3. `requestClock()` does not do what its doc-comment claimed, and the claim is now corrected rather than repeated. Measured against a production `next build` of this app (Next 15.5 / React 19.2), two calls 25 ms apart: RSC render same instant — `react/cache` memoises Route handler DIFFERENT — the cache dispatcher is not installed Server action DIFFERENT — and the render that follows it in the same HTTP request gets a third instant React memoises only while its cache dispatcher is set, which Next sets for the Flight render and not for the action or route-handler phase. So this is one instant per RENDER, not one per request, and inside a server action it is precisely the `new Date()` it replaced. The seam is still worth keeping — one place to change if a request-scoped store ever carries the clock, and one thing the scanner can insist on — but `ctx.evaluatedAt` is what a caller holding a context must use, which is what fix 2 does. Gate after: tsc clean, jest 107 suites / 1629 passed, build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(merge): the three things the compiler caught between the two branches None of these conflicted textually. Each is one branch's change meeting the other's, which is the class of defect a clean merge hides. - Two `notifyUsers` call sites this branch added — the "your term was rescheduled" message on both the club roster and the admin console — predate main making `kind` required, so they had no sender class. They send `seat-term-changed`, the kind main registered for the SHADOW→ACTIVE transition: rescheduling a term IS a change of access, made by moving a date instead of pressing a button. Mandatory on the `access` stream, because a person whose term now ends in May must not learn it by finding a door locked. Both also now tag `organizationId`, as every other seat notification does. - `reconcileSeatMeter` called `toSeatFacts(r)` with no clock. It has one — its own `at`, already used for the meter side — and passing anything else would compare a roster census at one instant against a meter reading at another and report the difference between the two moments as a disagreement. * fix(billing): a term reschedule is a seat change, so it meters like one `seat-meter-boundary` went red on the merge and it was right to. Effective dates gave the product a second way to end a term — setting a last day — and both `setTermDates` actions wrote `roleAssignment.update` outside any transaction and metered nothing. An OSE Director winding a term up early, or a president typing a last day that has already passed, emptied the seat with nobody pressing anything, and the meter kept that occupancy open for ever: `reconcileSeatMeter` would report it `overMetered` every day thereafter while the institution was invoiced for a seat its own roster shows as vacant. Both writes now run in a transaction with `meterTermRescheduled`, which is honest about what the table can hold. `SeatMeterEvent` is unique on `(institutionId, sourceEventKey)` and an assignment mints one OCCUPIED key and one VACATED key, so the meter can record an occupancy CLOSING once and has no row shape for "the closing instant moved". The first reschedule that gives a live term an end writes the VACATED dated at that end — which closes the span by itself when the day arrives, since `readSeatMeterFacts` filters on `effectiveAt`. A later reschedule that moves the same end is a no-op rather than a P2002 that would abort an edit the product allows, and re-opening a closed term cannot be expressed at all. Both residues are bounded, stated in the function's own doc, and visible to `reconcileSeatMeter`. The guard's `METER_CALL` was two hard-coded names, so a transaction that emitted through a named wrapper read as unmetered. It now DERIVES the alternation from `seat-meter.ts` — every exported function there that reaches `tx.seatMeterEvent.create` — so a caller still cannot satisfy it with a plausible-looking name, and it will not rot when the next writer lands. `mail-has-one-door` 29 → 31: measured against the tree, not incremented. The count on `origin/main` was re-derived with the test's own scan (29) and the delta is exactly the two reschedule notifications this branch adds. * fix(billing): storedStatus is the enum, so a mistyped status cannot silently skip the meter Compared against a literal, so a call site passing the wrong shape would never meter and the seat would stay open for ever — the failure the transaction guard exists to catch, arriving through a typo instead of an omission. * fix(e2e): the account under test is the one the helper refuses `signIn` gained a guard on main (#110) that fails when an account lands on /access-pending — right for the 54 call sites that expect a workspace, and exactly wrong for the one test whose subject is losing it. The two merged cleanly and the suite went red on a helper assertion, not on the product: "Maya Johnson signed in but holds no workspace". Takes the route the guard's own message names, and the one entitlement.spec.ts already uses for the account that never gets in. The deep link to the club is kept and its URL asserted, because "takes the club away" is a claim about that page and the gate lives in the (app) layout. Verified discriminating: swapping the heading for the other access-pending branch ("You do not have access yet") fails the test, so this asserts a term that ran out reads as ENDED rather than never-granted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(reports): the pulse mock offers the clock the pulse now reads A jest module factory REPLACES the module, so a partial one is a claim that the subject uses nothing else. This branch made `loadInstitutionPulse` read `requestClock` for the instant it judges seat windows at; main added a route test mocking `@/lib/rbac` with only `getUserContext`. Both merged clean and the handler threw `(0, _rbac.requestClock) is not a function` on two of the four tests. The instant is fixed rather than `new Date()` so the new assertion can name it: `loadSeatFacts` must be handed the request clock. Dropping that argument is the failure nothing else here could see — the seat count would simply be a little stale, and it is the only one of 2,239 tests that catches it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(registry): the inbound Slack events endpoint states its deferral Inherited red, not caused here: #96 added the API surface ratchet and #98 added `/api/integrations/slack/events`. Neither saw the other, they merged clean, and main has been failing `no handler is unaccounted for` since — the enumeration collision the ratchet exists to make loud, landed one merge too late to be loud on either PR. Deferred rather than bound, with the real reason: Slack is the caller, authenticated by the signature over the raw bytes, and the two events it acts on say the bot token this deployment holds is dead. A capability gate there would suppress the revocation notice precisely when the connection is least entitled to keep reading ACTIVE. Counts re-derived by measuring, not by incrementing: 1 bound + 23 deferred = 24 = `find src/app/api -name route.ts | wc -l`, no duplicates across the two lists. The "twenty-two" in the prose was one of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(registry): one entry for the events endpoint, not two #132 landed the same declaration on main while this branch carried its own. Both are additions to one object literal, in different places, so git merged them without a word and TypeScript refused the result: TS1117, "an object literal cannot have multiple properties with the same name" — one cause, three red jobs (type check, next build, the container build). Main's wording is kept because it is the better one: it names the reason there is no single tenant to ask about, which is that one delivery resolves through teamId to every institution that connected the workspace. Mine only said there is no session. The prose count stays at twenty-three, re-derived rather than inherited: 1 bound + 23 deferred = 24 = `find src/app/api -name route.ts | wc -l`, with no key appearing twice. #132 left it reading twenty-two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <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.
Main went red on a collision git could not see. #96 added surfaces.ts — a
ratchet requiring every API route appear in exactly one list, so a new endpoint
cannot ship without its author either binding it or writing down the deferral.
#98 added /api/integrations/slack/events. Neither branch touched the other's
file, both were green, and the two together fail:
● every API route declares its capability, or declares that it has not
› no handler is unaccounted for
+ "/api/integrations/slack/events"
This is the ratchet working, not a flaw in it: the endpoint really was
unaccounted for. The reason it is DEFERRED rather than BOUND is read off the
handler rather than assumed. Slack is the caller and the signature over the raw
bytes is the authentication, so there is no session to gate; and one delivery
resolves through teamId to every institution that connected that workspace, so
there is no single tenant whose capability could be consulted. That is the same
shape as the other infrastructure entries, so it sits with them.
Gates from a clean worktree, exit codes captured before any pipe:
prisma generate 0 / tsc --noEmit 0 (tsc 5.9.3, so not a silent 127) /
jest 141 suites, 2183 passed / next build 0.
Negative control, read per test rather than by suite exit code: deleting the
entry flips exactly "no handler is unaccounted for" red and restoring it green.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com