Skip to content

List the commitment fields participants can see, not the ones they can't - #468

Merged
richshaw merged 2 commits into
mainfrom
claude/issue-447-commitment-allowlist
Oct 18, 2026
Merged

richshaw merged 2 commits into
mainfrom
claude/issue-447-commitment-allowlist

Conversation

@richshaw

Copy link
Copy Markdown
Owner

Fixes #447.

The problem

Participant responses (sign up, view, edit, move) were the whole commitments row with editTokenHash and idempotencyKeyHash removed by name (withoutServerOnlyHashes). If a new server-only column were added to commitments, it would go out to participants unless someone remembered to add it to that list.

The fix

  • src/lib/public-commitment.ts: withoutServerOnlyHashes is replaced by PUBLIC_COMMITMENT_FIELDS, the fields participants may see, and toPublicCommitment, which copies only those fields. The PublicCommitment type is a Pick over the same list.
  • The list holds exactly what responses return today: every column except the two hashes, in schema order. No response changes, as the owner's note on the issue asked.
  • src/services/commitments.ts: the six call sites and the PublicCommitmentRow alias now use toPublicCommitment and PublicCommitment. The hashes are still read only where they're checked (the edit-token checks and the sign-up retry lookup).
  • I copy fields from the row instead of selecting fewer columns. The edit-token checks and the retry lookup need the hashes from the same row, so selecting fewer columns would add statements or a second select shape on the sign-up path. Copying keeps the statement count the same.

No CHANGELOG entry, because nobody would notice the change.

Tests

  • src/lib/public-commitment.test.ts: the output has exactly the listed keys and neither hash. A field nobody listed (standing in for a new column) is dropped, and the row passed in isn't changed. One more test compares the list plus the named server-only fields against getTableColumns(commitments). It fails when a column is added until someone decides which list the column goes on.
  • The existing route DB tests that check sign up, GET, PATCH and move responses carry no hashes still pass.
  • Ran and passed: pnpm lint, pnpm typecheck, pnpm knip, pnpm test (1205), pnpm test:db (463), and src/app/query-budget.db.test.ts with the site env vars (3).

https://claude.ai/code/session_01U5EDJw5FqncJdPoUfBRKjF


Generated by Claude Code

Participant responses (sign up, view, edit, move) used to be the whole
commitments row minus the two hashes, removed by name. A column added to
commitments later would have gone out to participants unless someone
remembered that list. They are now built from PUBLIC_COMMITMENT_FIELDS,
which holds exactly the fields they carry today, so nothing in any
response changes. A test fails when commitments gains a column that is
neither on that list nor named as server-only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U5EDJw5FqncJdPoUfBRKjF
@richshaw richshaw added the pipeline:working A session is working on it right now label Oct 18, 2026 — with Claude
@richshaw
richshaw requested a balanced review from Copilot October 18, 2026 04:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@richshaw richshaw added pipeline:tophat pipeline:working A session is working on it right now and removed pipeline:working A session is working on it right now labels Oct 18, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Tophat: passed

Ran c238596 locally with pnpm dev (no worker: nothing here touches jobs or email), Postgres 16, Chromium at 1280×900 and 390×844. This is a refactor that should leave every participant response the same, so the main check runs one scripted API flow on the base (75ec13a) and again on the head, then compares the two sets of responses.

Check Result What I saw
Same responses as the base ✅ The flow: sign up (POST /api/slots/{id}/commitments with an idempotency key), retry it with the same key, GET with the edit token, PATCH notes, PATCH swapToSlotId (move), GET the moved commitment. All six responses are identical on base and head: the same JSON paths in the same key order, and the same values once ids, timestamps and tokens are normalised. Status codes are all 200 on both
No hashes go out ✅ Neither editTokenHash nor idempotencyKeyHash, nor any key containing "hash", appears in any response on base or head
Retry and move still work ✅ The retry returns the first commitment's id on both. The move response carries a new editToken and editUrl, and the new commitment reads confirmed
Public page, desktop ✅ Signing up on /s/[slug] shows "saved your spot", and the confirmation email arrives with its edit link. Revisiting the page shows "You're signed up for Fruit and water. Edit or cancel", which is the returning-participant path through getOwnCommitmentsForSignup (468-head-confirmed-desktop, 468-head-returning-desktop)
Edit page, phone ✅ The edit link opens "You're editing this as Sam Example (sam@example.com)", 390 px wide with no sideways scroll. Saving notes shows "Saved", and the notes are still there on reload (468-head-edit-phone, 468-head-edit-saved-phone). No browser console errors. The server log's only errors are the health check's pgboss.job lookups, because the worker never ran
Tests ✅ src/lib/public-commitment.test.ts 4 of 4. pnpm test:db for the commitment and slot routes and the commitments service: 119 passed

Not UX: every response, page and email reads the same as on main.

Screenshots are in https://claude.ai/code/session_01AzC16pfLqisuhEvQmPXft5.


Generated by Claude Code

@richshaw richshaw added pipeline:review pipeline:working A session is working on it right now and removed pipeline:tophat pipeline:working A session is working on it right now labels Oct 18, 2026 — with Claude

@richshaw richshaw left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: passed (round 1)

withoutServerOnlyHashes used to drop two fields by name. It's replaced by PUBLIC_COMMITMENT_FIELDS, a list of the fields participants may see, and toPublicCommitment, which copies only those. Every participant response now goes through it: the sign-up retry, a new sign-up, getOwnCommitment, getOwnCommitmentsForSignup and updateOwnCommitment. The move path returns what commitParticipantToSlot gives back, so it's covered too. No callers of the old helper remain. Outside the schema and the token module, the hashes are read only where they're checked (commitments.ts:272, :513, :555). The list is every column except the two hashes, as the owner's note on #447 asked, so responses don't change. The tophat compared all six responses on base and head and found them identical, key order included. Each call site passes a full row (select(), c: commitments or .returning()), and toPublicCommitment takes a Commitment, so a partial row can't type-check and quietly send undefined fields. Both "Done when" items are met. Responses come from an explicit list. The exact-keys test fails if anything off the list goes out, and the getTableColumns test fails when a column is added until someone puts it on one list or the other. The statement count doesn't change, because rows are still read once and copied.

Blocking (0): none
Optional (0): none
Earlier threads: none on this PR
Checks: CI is green on c238596 (lint-type-test, e2e, CodeQL). That covers lint, types, knip, unit, DB, build, budgets and the query budget. The tophat passed on this exact SHA. Locally I read the diff, #447 and the owner's note, and checked every call site and remaining hash reference with grep. I also checked open issues for this area: only #447. Copilot couldn't review (quota reached). The PR is behind main but has no conflict.

Not UX and not high-risk (src/lib/public-commitment.ts and src/services/commitments.ts only), so it goes straight to merge.

https://claude.ai/code/session_015xd1jtBQeKpE6mYy58K2Bu


Generated by Claude Code

@richshaw richshaw added pipeline:merge pipeline:working A session is working on it right now and removed pipeline:review pipeline:working A session is working on it right now labels Oct 18, 2026 — with Claude
@richshaw richshaw added pipeline:working A session is working on it right now and removed pipeline:working A session is working on it right now labels Oct 18, 2026 — with Claude
@richshaw
richshaw merged commit 302c4d7 into main Oct 18, 2026
5 checks passed
@richshaw
richshaw deleted the claude/issue-447-commitment-allowlist branch October 18, 2026 06:25
@richshaw richshaw removed pipeline:merge pipeline:working A session is working on it right now labels Oct 18, 2026 — with Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List the commitment fields participants can see, not the ones they can't

3 participants