Add the tables one payment record lives in - #2007
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds six durable payment tables with shared SQLite constraints, indexes, encrypted-field rules, migration registration, ordered schema assembly, and tests covering metadata and database behavior. ChangesPayment records schema
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationRegistry
participant PaymentMigration
participant SchemaAssembly
participant SQLite
MigrationRegistry->>PaymentMigration: Load payment records migration
PaymentMigration->>SchemaAssembly: Declare payment tables and indexes
SchemaAssembly->>SQLite: Create payment schema objects
SQLite-->>SchemaAssembly: Return schema metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67036adfde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Money can never be given back beyond what was taken. This holds whatever | ||
| // the refund is doing, so it belongs with the money rather than the runtime. | ||
| "refunded_amount IS NULL OR captured_amount IS NULL OR refunded_amount BETWEEN 0 AND captured_amount", | ||
| `provider IS NULL OR resource_kind IS NULL OR ${providerMatchesKind}`, |
There was a problem hiding this comment.
When a charge supplies only one identifier, this clause short-circuits to true because either provider IS NULL or resource_kind IS NULL passes the whole check. That admits records that cannot satisfy RESOURCE_KIND_BY_PROVIDER, leaving reconciliation or refund code without a complete provider/resource pair; require both fields to be null or both present and matched.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| /** What a decision may never be, whatever else is true of it. */ | ||
| const aboutTheDecision = alsoAbout([ | ||
| "(attempt_count = 0) = (last_attempt_at IS NULL)", | ||
| "next_retry_at IS NULL OR last_attempt_at IS NULL OR next_retry_at >= last_attempt_at", |
There was a problem hiding this comment.
When next_retry_at is set while last_attempt_at is null, this OR accepts the row; the preceding invariant then requires attempt_count = 0, yet the retry index can expose it as due work. A retry must require a non-null last-attempt time and occur no earlier than that attempt, rather than treating the missing attempt as valid.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| wholeNumberOrNull("evidence_redacted_at", "resolved_at"), | ||
| ], | ||
| ["revision", wholeNumber("revision", 1, 1)], | ||
| ["resolved_at", aboutTheCase("INTEGER")], |
There was a problem hiding this comment.
When a case is saved as resolved without resolved_at, or with resolved_at while still retrying/needs_action, this definition accepts it because it only validates a timestamp when one happens to be present. That leaves the state and lifecycle timestamp contradictory; notably, a resolved case without the timestamp cannot later satisfy the evidence_redacted_at >= resolved_at rule. Enforce that resolved_at is present exactly when state = 'resolved'.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| ["provider", oneOfOrNull("provider", PROVIDERS)], | ||
| ["resource_kind", oneOfOrNull("resource_kind", RESOURCE_KINDS)], | ||
| ["provider_reference", sealedEitherWay("provider_reference")], | ||
| ["reference_index", wordsOrNull("reference_index")], |
There was a problem hiding this comment.
When a current/provider-backed charge omits reference_index, wordsOrNull accepts it even though provider_reference is required. The resulting money record cannot be found through idx_payment_charges_reference, so provider reconciliation or callbacks using the blind lookup code can miss it; allow a missing index only for an explicitly justified legacy shape, not for current charges.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| "alert_sent_revision IS NULL OR (alerted_revision IS NOT NULL AND alert_sent_revision = alerted_revision)", | ||
| "alert_sent_at IS NULL OR (alerted_at IS NOT NULL AND alert_sent_at >= alerted_at)", | ||
| "(alert_lease_token IS NULL) = (alert_lease_expires_at IS NULL)", | ||
| `alert_lease_expires_at IS NULL OR (typeof(alert_lease_expires_at) = 'integer' AND alert_lease_expires_at >= first_observed_at)`, |
There was a problem hiding this comment.
When first_observed_at = 1, alerted_at = 100, and alert_lease_expires_at = 50, this rule accepts the claim even though its lease expired before the alert it is meant to send. That lets the alert queue treat the claim as stale while the first sender may still believe it owns the work, risking duplicate owner alerts; a claimed alert must have an alerted_at and expire no earlier than it.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| const saysSomething = (name: string): string => | ||
| `${isText(name)} AND length(trim(${name})) > 0`; |
There was a problem hiding this comment.
When an identifier or lease token consists only of tabs, newlines, or carriage returns, SQLite's one-argument trim() does not remove those characters, so saysSomething accepts the value as nonblank. This affects every column built with words, wordsOrNull, or keyWords, admitting effectively empty payment IDs, lookup codes, and worker claims; check all whitespace rather than only ordinary spaces.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| `typeof(observed_at) = 'integer' AND observed_at >= 0`, | ||
| // Money can never be given back beyond what was taken. This holds whatever | ||
| // the refund is doing, so it belongs with the money rather than the runtime. | ||
| "refunded_amount IS NULL OR captured_amount IS NULL OR refunded_amount BETWEEN 0 AND captured_amount", |
There was a problem hiding this comment.
When refunded_amount is positive but refund_state = 'none', the amount check accepts the row as long as it does not exceed the captured amount. That creates contradictory money facts: code using the state can try to refund money the amount says was already returned, while reporting or reconciliation using the amount sees the opposite result. At minimum, the none state must forbid a positive refunded amount.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| // be copied from a given old table per payment, so anything the upgrade | ||
| // does not know about would be a second, distinct "nowhere" that the | ||
| // unique index happily accepts. | ||
| ["legacy_source", oneOfOrNull("legacy_source", LEGACY_SOURCES)], |
There was a problem hiding this comment.
When origin = 'legacy' and legacy_source is null, this nullable column accepts the row and SQLite's unique index treats each null as distinct. A repeated legacy copy can therefore insert the same source charge again—especially when its lookup index is also unavailable—defeating the migration's idempotency guard and potentially duplicating recorded money. Require every legacy charge to name one of LEGACY_SOURCES, and forbid that field on current charges.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
|
Answering all eight findings from the last review together, because seven of them are declined for the same reason and it is a decision rather than a disagreement about the code. The tables are being cut back to what a column is, not what a payment may say. Six tables carried 104 rules; every other table in this site carries eight between them. SQLite cannot change a rule once rows exist — the table is rebuilt — a broken rule gives back a constraint string rather than a stack trace, and testing one needs a database. Six rounds of review on #1973 went almost entirely on these rules rather than on the code using them, and this review is seven more in the same direction. That cost is the point. So the tables now say only a column's type, whether it may be missing, what it starts as, and which lookups stay unique. Everything about how a payment behaves lives in Against that, each finding:
None of these is wrong as an observation. They are all correct about what the table currently permits — they are declined because the table is no longer the place that answers. The eighth is different and is now moot. You were right that SQLite's one-argument One rule stays on the tables, and it is worth saying why, since it is the exception that proves the rule: anything holding the buyer's details must really be text and carry a whole envelope. That check asks what actually landed in the column rather than what the code meant to put there, which is the one thing TypeScript cannot see — a type says This PR will be rebuilt on that basis — the vocabularies move back to #1973 with the record layer, leaving the slice as the tables and their migration. Generated by Claude Code |
Six tables, the migration that creates them, and the words each column is built from. Nothing writes to them yet. The tables say only what a column is — its type, whether it may be missing, what it starts as — and one thing more: that a column holding the buyer's details really contains encrypted text. That last rule checks what landed rather than what the code meant to write, which is the one thing TypeScript cannot see, since a type says "string" while the value is bytes. How a payment may behave is the record layer's to enforce, in code, where a broken rule says what is wrong in words, can be tried without a database, and can be changed without rebuilding a table already full of real money.
b2a5bee to
1f80fa8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f80fa8cf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const words = (fallback?: string): string => | ||
| withDefault("TEXT NOT NULL", fallback && `'${fallback}'`); |
There was a problem hiding this comment.
When a low-level write binds a Uint8Array for a words() field, SQLite stores it as a BLOB despite the TEXT affinity. The payment_id, case resource, effect, and delivery keys built with this helper then do not match the same value bound as a string, and SQLite's unique indexes allow both BLOB and TEXT copies, potentially making child records unreachable or allowing duplicate completion work. Add a typeof(column) = 'text' constraint rather than relying on TEXT affinity alone.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — deliberately, and the repo owner made the call directly when I put this trade-off to them.
Your mechanism is right, and it is the same fault we fixed on the encrypted columns: SQLite keeps a bound Uint8Array as a BLOB under TEXT affinity, the unique index treats it as distinct from the string, and the child rows become unreachable. That is exactly why sealed starts with typeof(x) = 'text'.
The reason it does not extend to words() is what this slice is now about. These tables deliberately carry only one rule — that a column holding the buyer's details really contains encrypted text — because a rule in a table says nothing useful when it fails, cannot be tried without a database, and cannot be changed once real money is in the table without rebuilding it. The encrypted-column check is the single kept exception, on the grounds that TypeScript genuinely cannot see the difference between a string type and a bytes value.
I argued your position: a byte-bound payment_id is precisely that same blind spot, so the exception ought to cover it. Put as a straight choice, the answer was to leave the tables bare — we write these columns ourselves, we always bind strings, and trusting our own code is the point.
So this is an accepted gap rather than a fixed one, and I would rather say that plainly than dress it up. The neighbouring case that ordinary code can create — a key that is blank or only whitespace — is refused in the record layer on #1973 (allSaySomething).
Same answer on the sibling threads about keyWords and the whole-number helpers over on #1973.
Generated by Claude Code
| * Nothing writes to them yet. The code that does — the repositories, the | ||
| * runtime, and the copy that brings older payments across — follows in its own | ||
| * changes, so each arrives small enough to read. |
There was a problem hiding this comment.
When the follow-up repositories and runtime land, this permanent migration comment will become false, while the explanation that the work was split into smaller changes still says nothing about how the migration operates. Keep only the migration's current purpose here and leave the temporary sequencing rationale in the PR or commit history so future readers are not told that nothing writes these tables.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 7c12956. The comment now says what the tables are for and nothing about when other code arrives:
The tables one payment record lives in: the payment itself, the money taken
against it, the work and messages that follow, and any problem for the owner
to settle.
Your distinction is the right one and it is the house rule here too — comments describe current code, and git history keeps the rest. "Nothing writes to them yet" was true when written and would have quietly become a lie, which is worse than saying nothing, because a future reader has no way to tell a stale comment from a current one.
The sequencing now lives only in this PR's description and the commit message, which is where it belongs.
Generated by Claude Code
| wordsOrNull, | ||
| } from "./columns.ts"; | ||
|
|
||
| /* jscpd:ignore-end */ |
There was a problem hiding this comment.
This jscpd:ignore-end marker has no matching jscpd:ignore-start anywhere in the file, so it cannot delimit the permitted import-only exclusion and instead leaves a misleading suppression directive in production code. Add the matching import-block start before the imports, as in sessions.ts, or remove this unmatched marker.
AGENTS.md reference: AGENTS.md:L73-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 7c12956.
The opening half was lost when the tables' rules were stripped — the imports shrank, that line went with them, and the closing marker was left delimiting nothing. Restored to match sessions.ts.
Worth saying why the marker is wanted rather than just deleting the orphan, since removal was your other option: import blocks are the one place jscpd:ignore is sanctioned in this repo, and the six table files necessarily import overlapping sets of column builders. Without the pair that genuine duplication is reported at the 0% threshold, and there is no helper to extract — the imports are the duplication.
Verified: deno task cpd clean, 66 table tests pass.
Generated by Claude Code
The charges table had an import marker's closing half with no opening half, so it delimited nothing. The migration's comment said nothing writes to these tables yet, which stops being true as soon as something does; it now says what the tables are for.
This branch was rebuilt — eight earlier review threads are now about code that no longer existsSince this PR was opened, the repo owner decided the tables should stop carrying behaviour rules. The six tables went from 104 CHECK constraints to 14, and the 14 that remain all do one job: verify that a column holding the buyer's details really contains encrypted text — the one thing TypeScript cannot see, because a type says Everything else moved to the record layer in TypeScript, where a broken rule says what is wrong in words, can be tried without a database, and can be changed without rebuilding a table already full of real money. This branch has been rebuilt on current What this means for the open threadsEight threads on this PR describe faults in CHECK constraints that are gone. They were all correct about the code as it stood, and I am not dismissing them — but the specific constraint each one names no longer exists, so there is nothing to change at the line it points at. Where the underlying rule still mattered, it now lives in
The one that has no replacement is the Two threads on the current code were fixed
ChecksRun against this branch on its own, on top of current
Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/db/migrations/schema/payments/columns.ts`:
- Around line 30-31: Update words to accept the column name and append a CHECK
constraint enforcing length(trim(<column>)) > 0 alongside TEXT NOT NULL, while
preserving fallback handling. Update every words caller in the payment schema
migrations to pass its corresponding column name so required text fields reject
blank or whitespace-only values.
In `@test/shared/db/migrations/registry.test.ts`:
- Around line 50-56: Update the migration-order assertions in
test/shared/db/migrations/registry.test.ts at lines 50-56 and 58-68 to first use
strong exact assertions confirming the prerequisite IDs
2026-07-22_maintenance_completion and 2026-07-28_note_entities are present, then
compare their positions with the dependent migrations. Preserve the existing
ordering checks while preventing missing IDs from being treated as valid index
-1 values.
In `@test/shared/db/migrations/schema/payments/decisions.test.ts`:
- Around line 39-50: Extend the accepted payment decision insert coverage near
the existing decision cases to include a valid row with claim set to NULL, while
retaining the current malformed plaintext rejection tests. Use the existing
payment_case_decisions insert pattern and assert that the nullable encrypted
claim is accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03678f47-db33-44bf-9698-35aae91b6121
📒 Files selected for processing (25)
src/shared/db/migrations/2026-07-26_payment_records.tssrc/shared/db/migrations/registry.tssrc/shared/db/migrations/schema/index.tssrc/shared/db/migrations/schema/payments/cases.tssrc/shared/db/migrations/schema/payments/charges.tssrc/shared/db/migrations/schema/payments/columns.tssrc/shared/db/migrations/schema/payments/completion-deliveries.tssrc/shared/db/migrations/schema/payments/completion-effects.tssrc/shared/db/migrations/schema/payments/decisions.tssrc/shared/db/migrations/schema/payments/index.tssrc/shared/db/migrations/schema/payments/sessions.tssrc/shared/db/migrations/schema/version.tstest/shared/db/migrations/2026-07-26_payment_records.test.tstest/shared/db/migrations/registry.test.tstest/shared/db/migrations/schema/index.test.tstest/shared/db/migrations/schema/payments/cases.test.tstest/shared/db/migrations/schema/payments/charges.test.tstest/shared/db/migrations/schema/payments/columns.test.tstest/shared/db/migrations/schema/payments/completion-deliveries.test.tstest/shared/db/migrations/schema/payments/completion-effects.test.tstest/shared/db/migrations/schema/payments/decisions.test.tstest/shared/db/migrations/schema/payments/refuses.tstest/shared/db/migrations/schema/payments/schema.test.tstest/shared/db/migrations/schema/payments/sessions.test.tstest/shared/db/migrations/schema/version/guard.test.ts
The migration order tests compared positions with indexOf, where a name that is not in the list at all reads as -1 and loses to every real position — so deleting a migration these depend on would have kept them green. Each name is confirmed present before the two are compared. A decision's record now has a case where what was decided and what went wrong are both missing, which the rules on those columns allow. Every accepted row until now filled them in, so a rule that wrongly demanded them would not have been caught.
2 participants
The first slice peeled off #1973, which had grown to 8,300 lines and become unreviewable. This is about 1,400 lines and can be read on its own.
It adds the six tables a payment record lives in and the migration that creates them. Nothing writes to them yet — the repositories, the runtime, and the copy that brings older payments across stay in #1973 and follow separately.
Why six tables
A payment is not one fact. It is a checkout we opened, money that may or may not have arrived, work that has to happen once it does, messages that have to go out, and — when something does not add up — a problem for the owner to settle. Those things change at different times, are written by different code, and are looked up in different ways. Keeping them in one row would mean every small update rewrites the lot, and would make "has this already been done?" impossible to ask cheaply.
So: one row for the payment, one row per thing taken, one row per job, one row per message, one row per problem, one row per decision.
payment_sessionspayment_chargespayment_completion_effectspayment_completion_deliveriespayment_casespayment_case_decisionsWhat the tables say, and what they leave to the code
The tables say what each column is — its type, whether it may be missing, what it starts as — and nothing about how a payment may behave.
Rules about behaviour live in the code instead. A rule in a table is hard to maintain: it says nothing useful when it fails, it cannot be tried without a database, and changing one means rebuilding a table already full of real money. The same rule in TypeScript names itself when it breaks, can be unit tested in isolation, and can be changed freely. So a payment's behaviour is checked where it can be understood and where a failure explains itself.
One rule stays on the tables, because it is the one thing the code cannot check itself: that a column holding the buyer's details really contains encrypted text. A type says
stringwhile the value is bytes — TypeScript cannot see the difference, and the table can. That rule sits at the last point before the data comes to rest.That check is stricter than it looks, and for a reason found by probing rather than reading: SQLite leaves a blob alone in a
TEXTcolumn, butGLOBturns one into text just long enough to compare it. So bytes spelling a real envelope passed, and were stored as bytes — a row written, looking correct, that nothing binding the same value as a string could ever find again. The rule now asks for real text first, and the tests bind real bytes rather than writing them out.What this does not do
Nothing writes to these tables. They are wired into the schema, so every part of this slice has a real user and nothing here is exempted from the unused-code check.
The migration is new, so no site has these tables yet and there is nothing to re-shape.
Checks
Run against this branch on its own, on top of current
main, not inherited from #1973:deno task precommitpasses, including the check that every export has a real usercases39/39,charges34/34,columns24/24,decisions25/25,sessions50/50,completion-effects8/8,completion-deliveries13/13🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests