feat(db): add room_create rpc and participants table - #61
Conversation
Summary by CodeRabbit
WalkthroughAdds a participants table, client_nonce to rooms, richer submissions and votes schemas with cascading FKs and dedupe nonces, implements room_create(...) to idempotently create/resolve rooms and seed participants, adjusts vote_submit signature, and expands tests/docs for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DB as Postgres
participant RPC as room_create()
participant Rooms as rooms
participant Rounds as rounds
participant Parts as participants
Client->>DB: CALL room_create(topic,cfg,client_nonce)
DB->>RPC: execute
rect rgb(255,245,230)
RPC->>Rooms: SELECT ... WHERE client_nonce = $nonce
alt found
RPC->>Rooms: UPDATE title if provided
else not found
RPC->>Rooms: INSERT room (client_nonce,title,...) -> room_id
RPC->>Rounds: INSERT initial round (phase='submit', deadline)
end
end
rect rgb(230,245,255)
RPC->>Parts: INSERT agent_1..agent_N (room_id, anon_name, role='debater') ON CONFLICT DO NOTHING
end
RPC-->>Client: RETURN room_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
db/schema.sql (2)
41-58: Submissions without FKs? Enjoy your orphaned garbage. Add the constraints.You reference rounds/participants by UUID with zero enforcement. That’s sloppy.
Patch:
CREATE TABLE IF NOT EXISTS submissions ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), - round_id uuid NOT NULL, - author_id uuid NOT NULL, + round_id uuid NOT NULL REFERENCES rounds(id) ON DELETE CASCADE, + author_id uuid NOT NULL REFERENCES participants(id) ON DELETE CASCADE, content text NOT NULL, ... UNIQUE (round_id, author_id, client_nonce) );Add a covering index for the idempotency key:
CREATE INDEX IF NOT EXISTS idx_submissions_nonce ON submissions (round_id, author_id, client_nonce);
62-76: Same problem in votes. Tie it to reality with FKs or this schema will rot.Room/round/voter must be coherent. At minimum, enforce round_id and voter_id.
Patch:
CREATE TABLE IF NOT EXISTS votes ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), - room_id uuid NOT NULL, - round_id uuid NOT NULL, - voter_id uuid NOT NULL, + room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, + round_id uuid NOT NULL REFERENCES rounds(id) ON DELETE CASCADE, + voter_id uuid NOT NULL REFERENCES participants(id) ON DELETE CASCADE, kind text NOT NULL, -- e.g., 'continue' ballot jsonb NOT NULL DEFAULT '{}'::jsonb, client_nonce text NOT NULL, created_at timestamptz NOT NULL DEFAULT now(), UNIQUE (round_id, voter_id, kind, client_nonce) );Optionally enforce (room_id, round_id) coherence with a trigger or drop room_id from votes and derive via joins.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
db/rpc.sql(1 hunks)db/schema.sql(1 hunks)db/test/10_rooms_rounds.pgtap(2 hunks)db/test/30_rpcs.pgtap(1 hunks)docs/LocalDB.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
docs/LocalDB.mddb/test/10_rooms_rounds.pgtapdb/schema.sqldb/rpc.sqldb/test/30_rpcs.pgtap
f97f579 to
bb203e4
Compare
|
Added FK constraints for submissions/votes, created nonce index, and updated pgTAP fixtures to seed the requisite rooms/participants so the tests cover the new constraints. |
bb203e4 to
055d75b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
db/test/30_rpcs.pgtap (3)
24-28: Same schema qualification problem here. Fix it.- to_regprocedure('submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL, + to_regprocedure('public.submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL,
53-56: Qualify the vote RPC too.- to_regprocedure('vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL, + to_regprocedure('public.vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL,
81-83: Existence checks for round ops are the floor, not the ceiling.At minimum, assert they “live” without exceptions using pgTAP’s lives_ok instead of a generic DO/pass block. Even better: seed a due round and verify publish/open transitions.
-SELECT ok(to_regprocedure('round_publish_due()') IS NOT NULL, 'round_publish_due exists'); -SELECT ok(to_regprocedure('round_open_next()') IS NOT NULL, 'round_open_next exists'); +SELECT ok(to_regprocedure('public.round_publish_due()') IS NOT NULL, 'round_publish_due exists'); +SELECT ok(to_regprocedure('public.round_open_next()') IS NOT NULL, 'round_open_next exists');Optionally replace the DO/pass with:
SELECT lives_ok($$ SELECT public.round_publish_due() $$, 'round_publish_due lives'); SELECT lives_ok($$ SELECT public.round_open_next() $$, 'round_open_next lives');Remember to adjust the plan if you change assertion counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
db/rpc.sql(1 hunks)db/schema.sql(2 hunks)db/test/10_rooms_rounds.pgtap(2 hunks)db/test/30_rpcs.pgtap(3 hunks)docs/LocalDB.md(1 hunks)server/test/rpc.db.postgres.test.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
@(server|web|cli)/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Validate external inputs with Zod at the edges (server/controllers, web handlers/forms, CLI parsing)
Files:
server/test/rpc.db.postgres.test.js
**/*.{test,spec}.js
📄 CodeRabbit inference engine (AGENTS.md)
Write minimal Vitest tests for canonical JSON and RPC schema validation
Files:
server/test/rpc.db.postgres.test.js
server/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not access process.env directly in server code; use SecretSource and ConfigBuilder
Files:
server/test/rpc.db.postgres.test.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
db/schema.sqldb/rpc.sqldb/test/30_rpcs.pgtapserver/test/rpc.db.postgres.test.jsdb/test/10_rooms_rounds.pgtapdocs/LocalDB.md
🪛 GitHub Actions: ci
server/test/rpc.db.postgres.test.js
[error] 51-51: npx vitest run server/test/rpc.db.postgres.test.js failed. Cannot truncate a table referenced in a foreign key constraint while executing: TRUNCATE submissions, votes RESTART IDENTITY;.
🔇 Additional comments (9)
db/schema.sql (3)
39-39: Index looks right.
idx_participants_roommatches the expected access pattern. Good.
44-46: FKs + ON DELETE CASCADE: correct.Linking submissions/votes to
participants(androomsfor votes) with CASCADE is the right call for cleanup and integrity. Keep it.Also applies to: 60-61, 67-70
27-37: Constrain role. Stop letting garbage in.
participants.roleis a free-for-all. Add a CHECK (or enum) so you don’t end up with “rabid_ferret” as a role.Apply:
CREATE TABLE IF NOT EXISTS participants ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, anon_name text NOT NULL, - role text NOT NULL DEFAULT 'debater', + role text NOT NULL DEFAULT 'debater' CHECK (role IN ('debater','host','judge')), jwt_sub text, ssh_fingerprint text, created_at timestamptz NOT NULL DEFAULT now(), UNIQUE (room_id, anon_name) );server/test/rpc.db.postgres.test.js (1)
36-42: Seeding participants: fine.Deterministic IDs + roles set. Works with FKs.
db/test/10_rooms_rounds.pgtap (1)
2-2: Your plan is lying. Test the constraints you added.You added a table but never asserted its unique and index invariants. Bump the plan and add checks.
Apply:
-SELECT plan(7); +SELECT plan(9); @@ -- participants table exists SELECT ok( EXISTS ( SELECT 1 FROM information_schema.tables WHERE table_schema = 'public' AND table_name = 'participants' ), 'participants table exists' ); + +-- participants unique (room_id, anon_name) exists +SELECT ok( + EXISTS ( + SELECT 1 + FROM pg_constraint c + JOIN pg_class t ON t.oid = c.conrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = 'public' + AND t.relname = 'participants' + AND c.contype = 'u' + AND pg_get_constraintdef(c.oid) ILIKE '%(room_id, anon_name)%' + ), + 'participants unique (room_id, anon_name) exists' +); + +-- idx_participants_room exists +SELECT ok( + EXISTS ( + SELECT 1 + FROM pg_indexes + WHERE schemaname = 'public' + AND tablename = 'participants' + AND indexname = 'idx_participants_room' + ), + 'idx_participants_room exists' +);Also applies to: 22-29
docs/LocalDB.md (1)
25-34: Document the right damn signature.You wrote
room_create(topic json). The code isroom_create(topic text, cfg jsonb DEFAULT '{}'::jsonb). Mismatch confuses everyone.Apply:
-### Create a Room + Seed Participants - -The new `room_create(topic json)` RPC seeds a room, round 0, and a roster of anonymous participants. Example: +### Create a Room + Seed Participants + +The new `room_create(topic text, cfg jsonb DEFAULT '{}'::jsonb)` RPC seeds a room, round 0, and a roster of anonymous participants. +Defaults: `participant_count=4`, `submit_minutes=5`. +Examples: + +-- simplest +psql ... -c "select room_create('Demo Topic');"-psql postgresql://postgres:test@localhost:54329/db8 -c "select room_create('Demo Topic', '{"participant_count":4,"submit_minutes":2}'::jsonb);"
+psql postgresql://postgres:test@localhost:54329/db8 -c "select room_create('Demo Topic', '{"participant_count":4,"submit_minutes":2}'::jsonb);"The function returns the `room_id` you can plug into API calls or the CLI.db/rpc.sql (1)
3-29: No bounds. No idempotency. No mercy. Fix both.You accept absurd inputs and you’ll mint duplicate rooms on retries. Validate and make room creation idempotent. Also stop hiding bugs with GREATEST in
generate_series.Apply:
CREATE OR REPLACE FUNCTION room_create( - p_topic text, - p_cfg jsonb DEFAULT '{}'::jsonb + p_topic text, + p_cfg jsonb DEFAULT '{}'::jsonb, + p_client_nonce text DEFAULT NULL ) RETURNS uuid LANGUAGE plpgsql AS $$ DECLARE v_room_id uuid; - v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); - v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); - v_now integer := extract(epoch from now())::int; - v_submit_deadline integer := v_now + GREATEST(v_submit_minutes, 1) * 60; + v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); + v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); + v_now integer := extract(epoch from now())::int; + v_submit_deadline integer; BEGIN - INSERT INTO rooms (title) - VALUES (NULLIF(p_topic, '')) - RETURNING id INTO v_room_id; + -- bounds: participants [1..64], submit_minutes [1..1440] + IF v_participants IS NULL OR v_participants < 1 OR v_participants > 64 THEN + RAISE EXCEPTION 'participant_count out of range [1..64]: %', v_participants USING ERRCODE = '22023'; + END IF; + IF v_submit_minutes IS NULL OR v_submit_minutes < 1 OR v_submit_minutes > 1440 THEN + RAISE EXCEPTION 'submit_minutes out of range [1..1440]: %', v_submit_minutes USING ERRCODE = '22023'; + END IF; + v_submit_deadline := v_now + v_submit_minutes * 60; + + -- idempotent room creation via client nonce + INSERT INTO rooms (title, client_nonce) + VALUES (NULLIF(p_topic, ''), NULLIF(p_client_nonce, '')) + ON CONFLICT (client_nonce) + DO UPDATE SET title = COALESCE(rooms.title, EXCLUDED.title) + RETURNING id INTO v_room_id; INSERT INTO rounds (room_id, idx, phase, submit_deadline_unix) VALUES (v_room_id, 0, 'submit', v_submit_deadline); INSERT INTO participants (room_id, anon_name, role) SELECT v_room_id, format('agent_%s', gs), 'debater' - FROM generate_series(1, GREATEST(v_participants, 1)) AS gs; + FROM generate_series(1, v_participants) AS gs; RETURN v_room_id; END; $$;And update schema to support nonce (rooms table):
-- db/schema.sql (rooms) ALTER TABLE rooms ADD COLUMN IF NOT EXISTS client_nonce text UNIQUE;Update docs/tests to pass
p_client_noncewhere you need idempotency (e.g., CLI/HTTP clients).db/test/30_rpcs.pgtap (2)
2-2: Your plan will be wrong the moment you test the real contract. Bump it.You’re adding assertions (see below). Update the plan accordingly instead of shipping a lie.
Apply once you add the two missing ok() in room_create:
-SELECT plan(10); +SELECT plan(12);
10-22: You still don’t assert phase/idx or participant role defaults. Finish the job.We asked for this already. Test the damned contract, not vibes.
Apply this patch to the DO block and bump the plan to 12 as shown earlier:
DO $$ DECLARE new_room uuid; participant_count integer; round_deadline integer; + round_phase text; + round_idx integer; + role_count integer; BEGIN SELECT room_create('Test Room', '{"participant_count":3,"submit_minutes":1}'::jsonb) INTO new_room; SELECT count(*) INTO participant_count FROM participants WHERE room_id = new_room; - SELECT submit_deadline_unix INTO round_deadline FROM rounds WHERE room_id = new_room AND idx = 0; + SELECT submit_deadline_unix, phase, idx + INTO round_deadline, round_phase, round_idx + FROM rounds WHERE room_id = new_room AND idx = 0; + SELECT count(*) INTO role_count FROM participants WHERE room_id = new_room AND role = 'debater'; PERFORM ok(participant_count = 3, 'room_create seeds configured participants'); PERFORM ok(round_deadline > extract(epoch from now())::int, 'room_create schedules submit deadline in future'); + PERFORM ok(round_phase = 'submit' AND round_idx = 0, 'room_create opens round 0 in submit phase'); + PERFORM ok(role_count = 3, 'participants default role = debater'); END$$;Based on learnings.
055d75b to
42c828e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
db/rpc.sql(1 hunks)db/schema.sql(2 hunks)db/test/10_rooms_rounds.pgtap(2 hunks)db/test/30_rpcs.pgtap(3 hunks)docs/LocalDB.md(1 hunks)server/test/rpc.db.postgres.test.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
@(server|web|cli)/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Validate external inputs with Zod at the edges (server/controllers, web handlers/forms, CLI parsing)
Files:
server/test/rpc.db.postgres.test.js
**/*.{test,spec}.js
📄 CodeRabbit inference engine (AGENTS.md)
Write minimal Vitest tests for canonical JSON and RPC schema validation
Files:
server/test/rpc.db.postgres.test.js
server/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not access process.env directly in server code; use SecretSource and ConfigBuilder
Files:
server/test/rpc.db.postgres.test.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
docs/LocalDB.mddb/rpc.sqldb/schema.sqldb/test/10_rooms_rounds.pgtapdb/test/30_rpcs.pgtapserver/test/rpc.db.postgres.test.js
🔇 Additional comments (9)
db/test/10_rooms_rounds.pgtap (1)
2-29: STOP HAND-WAVING THE INVARIANTS.Line 2: You “planned” seven tests and still refuse to assert the unique constraint or the participants index you just introduced. We’ve already been over this. Bump the plan, add the checks, and make the tests prove the structure exists. Based on learnings.
-SELECT plan(7); +SELECT plan(9);Append after the participants existence block:
-- participants unique (room_id, anon_name) exists SELECT ok( EXISTS ( SELECT 1 FROM pg_constraint c JOIN pg_class t ON t.oid = c.conrelid JOIN pg_namespace n ON n.oid = t.relnamespace WHERE n.nspname = 'public' AND t.relname = 'participants' AND c.contype = 'u' AND pg_get_constraintdef(c.oid) ILIKE '%(room_id, anon_name)%' ), 'participants unique (room_id, anon_name) exists' ); -- idx_participants_room exists SELECT ok( EXISTS ( SELECT 1 FROM pg_indexes WHERE schemaname = 'public' AND tablename = 'participants' AND indexname = 'idx_participants_room' ), 'idx_participants_room exists' );docs/LocalDB.md (1)
25-33: FIX THE LIE ABOUTroom_create’s SIGNATURE.Line 27: The function takes
(topic text, cfg jsonb DEFAULT '{}'::jsonb)but you’re still telling everyone it’stopic json. That misinformation sends users straight into syntax errors. Update the docs to match reality and mention the defaults while you’re there.-### Create a Room + Seed Participants - -The new `room_create(topic json)` RPC seeds a room, round 0, and a roster of anonymous participants. Example: +### Create a Room + Seed Participants + +The new `room_create(topic text, cfg jsonb DEFAULT '{}'::jsonb)` RPC seeds a room, round 0, and a roster of anonymous participants. Defaults: `participant_count=4`, `submit_minutes=5`. Example:db/schema.sql (1)
32-37: LOCK DOWNparticipants.roleBEFORE IT POISONS THE DB.Line 32: You’re letting anything slip into
participants.role. That’s amateur hour. The rest of the system clearly expects a small set of roles, yet you give callers a blank text field and pray. Enforce the contract with a check (or enum) and document it so junk data can’t leak in. Fix it now.CREATE TABLE IF NOT EXISTS participants ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, anon_name text NOT NULL, - role text NOT NULL DEFAULT 'debater', + role text NOT NULL DEFAULT 'debater' CHECK (role IN ('debater','host','judge')), jwt_sub text, ssh_fingerprint text, created_at timestamptz NOT NULL DEFAULT now(), UNIQUE (room_id, anon_name) );db/rpc.sql (2)
11-25: VALIDATE YOUR INPUTS BEFORE THEY DDOS THE DATABASE.Lines 11-25: You blindly coerce
participant_countandsubmit_minutes, slapGREATEST(...,1)on them, and call it a day. A malicious client can ask for 10 million participants or a 200-year deadline and you’ll happily queue that work. Validate the bounds, throw when they’re garbage, and only then compute the deadline andgenerate_series. Lose theGREATESThack once you’ve grown up.DECLARE v_room_id uuid; - v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); - v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); - v_now integer := extract(epoch from now())::int; - v_submit_deadline integer := v_now + GREATEST(v_submit_minutes, 1) * 60; + v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); + v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); + v_now integer := extract(epoch from now())::int; + v_submit_deadline integer; BEGIN + IF v_participants IS NULL OR v_participants < 1 OR v_participants > 64 THEN + RAISE EXCEPTION 'participant_count out of range [1..64]: %', v_participants USING ERRCODE = '22023'; + END IF; + IF v_submit_minutes IS NULL OR v_submit_minutes < 1 OR v_submit_minutes > 1440 THEN + RAISE EXCEPTION 'submit_minutes out of range [1..1440]: %', v_submit_minutes USING ERRCODE = '22023'; + END IF; + v_submit_deadline := v_now + v_submit_minutes * 60; ... - FROM generate_series(1, GREATEST(v_participants, 1)) AS gs; + FROM generate_series(1, v_participants) AS gs;
16-28: ADD A CLIENT NONCE OR ENJOY DUPLICATE ROOMS ON EVERY RETRY.Lines 16-28: This RPC is not idempotent. The moment the caller retries (which happens all the time in the real world) you mint another room, another round, another participant roster. You need a
client_noncecolumn onrooms, aUNIQUEconstraint, and an extra parameter in this function so retries return the existing room instead of detonating duplicates. Update the schema, the insert, and the call sites accordingly. No excuses.-CREATE OR REPLACE FUNCTION room_create( - p_topic text, - p_cfg jsonb DEFAULT '{}'::jsonb -) RETURNS uuid +CREATE OR REPLACE FUNCTION room_create( + p_topic text, + p_cfg jsonb DEFAULT '{}'::jsonb, + p_client_nonce text DEFAULT NULL +) RETURNS uuid ... - INSERT INTO rooms (title) - VALUES (NULLIF(p_topic, '')) - RETURNING id INTO v_room_id; + INSERT INTO rooms (title, client_nonce) + VALUES (NULLIF(p_topic, ''), NULLIF(p_client_nonce, '')) + ON CONFLICT (client_nonce) DO UPDATE SET title = COALESCE(rooms.title, EXCLUDED.title) + RETURNING id INTO v_room_id;And don’t forget to add
client_nonce text UNIQUEtoroomsindb/schema.sql, plus migrations/tests/docs to match.db/test/30_rpcs.pgtap (4)
6-6: QUALIFY THE DAMN FUNCTION LOOKUP.
If you don’t pin the schema, a stray search_path masks missing RPCs. Fix it.- to_regprocedure('room_create(text,jsonb)') IS NOT NULL, + to_regprocedure('public.room_create(text,jsonb)') IS NOT NULL,
33-50: FIXsubmission_upsertTEST TO PROVE IDEMPOTENCE.
Counting on UUID equality alone is lazy. Show that the table holds a single row for a repeated nonce and that a different nonce yields a new row.DECLARE room uuid := '00000000-0000-0000-0000-0000000000a0'; round uuid := '00000000-0000-0000-0000-0000000000aa'; author uuid := '00000000-0000-0000-0000-0000000000bb'; n text := 'nonce-sub-1'; - id1 uuid; id2 uuid; + n2 text := 'nonce-sub-2'; + id1 uuid; id2 uuid; id3 uuid; + sub_count integer; BEGIN @@ SELECT submission_upsert(round, author, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n) INTO id1; SELECT submission_upsert(round, author, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n) INTO id2; PERFORM ok(id1 = id2, 'submission_upsert idempotent for same nonce'); + SELECT count(*) INTO sub_count FROM submissions WHERE round_id = round AND author_id = author; + PERFORM ok(sub_count = 1, 'submission_upsert inserts one row for repeated nonce'); + SELECT submission_upsert(round, author, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n2) INTO id3; + PERFORM ok(id3 <> id1, 'submission_upsert creates a new row for different nonce'); END$$;
61-77: APPLY THE SAME RIGOR TOvote_submit.
You learned nothing from the submissions block. Prove the nonce uniqueness with counts and a different nonce.DECLARE v_room uuid := '00000000-0000-0000-0000-0000000000ac'; v_round uuid := '00000000-0000-0000-0000-0000000000ad'; voter uuid := '00000000-0000-0000-0000-0000000000ae'; n text := 'nonce-vote-1'; - id1 uuid; id2 uuid; + n2 text := 'nonce-vote-2'; + id1 uuid; id2 uuid; id3 uuid; + votes_count integer; BEGIN @@ SELECT vote_submit(v_room, v_round, voter, 'continue', '{}'::jsonb, n) INTO id1; SELECT vote_submit(v_room, v_round, voter, 'continue', '{}'::jsonb, n) INTO id2; PERFORM ok(id1 = id2, 'vote_submit idempotent for same nonce'); + SELECT count(*) INTO votes_count FROM votes WHERE round_id = v_round AND voter_id = voter AND kind = 'continue'; + PERFORM ok(votes_count = 1, 'vote_submit inserts one row for repeated nonce'); + SELECT vote_submit(v_room, v_round, voter, 'continue', '{}'::jsonb, n2) INTO id3; + PERFORM ok(id3 <> id1, 'vote_submit creates a new row for different nonce'); END$$;
2-21: STOP SHIPPING HALF-TESTEDroom_create()!
You were already told to prove the round opens in submit phase and that the seeded bodies are debaters. Still missing. Add the proper selects, assert the phase/idx/role, and bump the plan after you stack the other missing checks — this sloppy coverage is unacceptable. Based on learnings.-SELECT plan(10); +SELECT plan(16); DO $$ DECLARE new_room uuid; participant_count integer; round_deadline integer; + round_phase text; + round_idx integer; + role_count integer; BEGIN SELECT room_create('Test Room', '{"participant_count":3,"submit_minutes":1}'::jsonb) INTO new_room; SELECT count(*) INTO participant_count FROM participants WHERE room_id = new_room; - SELECT submit_deadline_unix INTO round_deadline FROM rounds WHERE room_id = new_room AND idx = 0; + SELECT submit_deadline_unix, phase, idx + INTO round_deadline, round_phase, round_idx + FROM rounds WHERE room_id = new_room AND idx = 0; + SELECT count(*) INTO role_count FROM participants WHERE room_id = new_room AND role = 'debater'; PERFORM ok(participant_count = 3, 'room_create seeds configured participants'); PERFORM ok(round_deadline > extract(epoch from now())::int, 'room_create schedules submit deadline in future'); + PERFORM ok(round_phase = 'submit' AND round_idx = 0, 'room_create opens round 0 in submit phase'); + PERFORM ok(role_count = participant_count, 'participants default role = debater'); END$$;
42c828e to
5d5a6e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
db/schema.sql (1)
20-22: 2038 called. It wants your 32‑bit epoch back. Use BIGINT.Integer epochs will overflow. Make unix timestamps BIGINT now instead of shipping a time bomb.
- submit_deadline_unix integer NOT NULL DEFAULT 0, - published_at_unix integer, - continue_vote_close_unix integer, + submit_deadline_unix bigint NOT NULL DEFAULT 0, + published_at_unix bigint, + continue_vote_close_unix bigint,db/test/30_rpcs.pgtap (3)
41-44: Schema‑qualify or use pgTAP helpers. Don’t play search_path roulette.Qualify submission_upsert with public. Or better, use has_function.
-SELECT ok( - to_regprocedure('submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL, +SELECT ok( + to_regprocedure('public.submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL, 'submission_upsert exists' );Or:
SELECT has_function('public', 'submission_upsert', ARRAY['uuid','uuid','text','jsonb','jsonb','text','text']);
75-78: Qualify vote_submit as well. Consistency matters.Same issue: add public. or switch to has_function.
- to_regprocedure('vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL, + to_regprocedure('public.vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL,
109-110: And the round ops. Finish the job.Qualify round_publish_due and round_open_next too.
-SELECT ok(to_regprocedure('round_publish_due()') IS NOT NULL, 'round_publish_due exists'); -SELECT ok(to_regprocedure('round_open_next()') IS NOT NULL, 'round_open_next exists'); +SELECT ok(to_regprocedure('public.round_publish_due()') IS NOT NULL, 'round_publish_due exists'); +SELECT ok(to_regprocedure('public.round_open_next()') IS NOT NULL, 'round_open_next exists');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
db/rpc.sql(1 hunks)db/schema.sql(3 hunks)db/test/10_rooms_rounds.pgtap(2 hunks)db/test/30_rpcs.pgtap(3 hunks)docs/LocalDB.md(1 hunks)server/test/rpc.db.postgres.test.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
@(server|web|cli)/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Validate external inputs with Zod at the edges (server/controllers, web handlers/forms, CLI parsing)
Files:
server/test/rpc.db.postgres.test.js
**/*.{test,spec}.js
📄 CodeRabbit inference engine (AGENTS.md)
Write minimal Vitest tests for canonical JSON and RPC schema validation
Files:
server/test/rpc.db.postgres.test.js
server/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not access process.env directly in server code; use SecretSource and ConfigBuilder
Files:
server/test/rpc.db.postgres.test.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
db/test/30_rpcs.pgtapdb/rpc.sqldb/schema.sqldocs/LocalDB.mdserver/test/rpc.db.postgres.test.jsdb/test/10_rooms_rounds.pgtap
🔇 Additional comments (10)
db/test/30_rpcs.pgtap (1)
22-38: Good. This actually tests the contract.Solid assertions for participant seeding, phase/idx, and nonce idempotency. Keep it.
server/test/rpc.db.postgres.test.js (1)
36-42: Seeding participants up front is the right move.This makes the RPC tests deterministic. Good.
db/schema.sql (2)
28-38: Stop letting garbage “roles” leak in. Constrain it.You added participants but left role as free‑text. That’s how bad data creeps in. Lock it down with a CHECK (or enum).
Apply:
CREATE TABLE IF NOT EXISTS participants ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, anon_name text NOT NULL, - role text NOT NULL DEFAULT 'debater', + role text NOT NULL DEFAULT 'debater' + CHECK (role IN ('debater','host','judge')), jwt_sub text, ssh_fingerprint text, created_at timestamptz NOT NULL DEFAULT now(), UNIQUE (room_id, anon_name) );
61-62: Drop the redundant nonce index. You already have it via UNIQUE.The UNIQUE (round_id, author_id, client_nonce) builds an index. Your extra idx_submissions_nonce is pointless bloat.
-CREATE INDEX IF NOT EXISTS idx_submissions_nonce ON submissions (round_id, author_id, client_nonce);db/test/10_rooms_rounds.pgtap (1)
2-2: Your plan is undercounting the invariants you should be asserting. Add them and bump the plan.You added participants but didn’t verify its unique and index. Test the schema, not your wishful thinking.
Apply:
-SELECT plan(7); +SELECT plan(9);Append after “participants table exists”:
-- participants unique (room_id, anon_name) exists SELECT ok( EXISTS ( SELECT 1 FROM pg_constraint c JOIN pg_class t ON t.oid = c.conrelid JOIN pg_namespace n ON n.oid = t.relnamespace WHERE n.nspname = 'public' AND t.relname = 'participants' AND c.contype = 'u' AND pg_get_constraintdef(c.oid) ILIKE '%(room_id, anon_name)%' ), 'participants unique (room_id, anon_name) exists' ); -- idx_participants_room exists SELECT ok( EXISTS ( SELECT 1 FROM pg_indexes WHERE schemaname = 'public' AND tablename = 'participants' AND indexname = 'idx_participants_room' ), 'idx_participants_room exists' );docs/LocalDB.md (1)
25-34: Docs are lying about the RPC signature. Fix it.You wrote room_create(topic json). The code is room_create(text, jsonb DEFAULT '{}'::jsonb, text DEFAULT NULL). Don’t mislead users.
Apply:
-### Create a Room + Seed Participants - -The new `room_create(topic json)` RPC seeds a room, round 0, and a roster of anonymous participants. Example: +### Create a Room + Seed Participants + +The new `room_create(topic text, cfg jsonb DEFAULT '{}'::jsonb, p_client_nonce text DEFAULT NULL)` RPC seeds a room, round 0, and a roster of anonymous participants. +Defaults: `participant_count=4`, `submit_minutes=5`. +Examples: + +-- simplest +psql postgresql://postgres:test@localhost:54329/db8 -c "select room_create('Demo Topic');"-psql postgresql://postgres:test@localhost:54329/db8 -c "select room_create('Demo Topic', '{"participant_count":4,"submit_minutes":2}'::jsonb, 'demo-room-nonce');"
+psql postgresql://postgres:test@localhost:54329/db8 -c "select room_create('Demo Topic', '{"participant_count":4,"submit_minutes":2}'::jsonb, 'demo-room-nonce');"-The function returns the `room_id` you can plug into API calls or the CLI. Repeating the call with the same nonce reuses the existing room. +The function returns the `room_id` you can plug into API calls or the CLI. Repeating the call with the same nonce reuses the existing room.db/rpc.sql (4)
12-16: Stop letting clients DOS your DB. Validate inputs and compute deadline after checks.You’re blindly coercing JSON and then hiding bad values with GREATEST(...). That’s sloppy and dangerous. Enforce sane bounds and fail fast; then remove the GREATEST in generate_series.
Apply this patch:
- v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); - v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); + -- parse with defaults; validate below + v_participants integer := COALESCE(NULLIF(p_cfg->>'participant_count','')::int, 4); + v_submit_minutes integer := COALESCE(NULLIF(p_cfg->>'submit_minutes','')::int, 5); v_now integer := extract(epoch from now())::int; - v_submit_deadline integer := v_now + GREATEST(v_submit_minutes, 1) * 60; + v_submit_deadline integer; v_client_nonce text := NULLIF(p_client_nonce, ''); v_created boolean := false; BEGIN + -- hard bounds; fail fast + IF v_participants IS NULL OR v_participants < 1 OR v_participants > 64 THEN + RAISE EXCEPTION 'participant_count out of range [1..64]: %', v_participants USING ERRCODE = '22023'; + END IF; + IF v_submit_minutes IS NULL OR v_submit_minutes < 1 OR v_submit_minutes > 1440 THEN + RAISE EXCEPTION 'submit_minutes out of range [1..1440]: %', v_submit_minutes USING ERRCODE = '22023'; + END IF; + v_submit_deadline := v_now + v_submit_minutes * 60; @@ - FROM generate_series(1, GREATEST(v_participants, 1)) AS gs + FROM generate_series(1, v_participants) AS gsAlso consider guarding casts to int with a regex if you want to default instead of erroring on non‑numeric strings. Based on learnings.
Also applies to: 41-41
19-27: Your “idempotency” is racy. Two retries and you 500. Use ON CONFLICT or the two-step safe pattern.Pre‑selecting by nonce and then inserting is a TOCTOU bug. Concurrent calls with the same nonce will collide on the UNIQUE constraint and error, instead of cleanly returning the same room_id. Fix it.
Apply this race‑free pattern:
- IF v_client_nonce IS NOT NULL THEN - SELECT id INTO v_room_id FROM rooms WHERE client_nonce = v_client_nonce; - END IF; - - IF v_room_id IS NULL THEN - INSERT INTO rooms (title, client_nonce) - VALUES (NULLIF(p_topic, ''), v_client_nonce) - RETURNING id INTO v_room_id; - v_created := true; - ELSE - UPDATE rooms - SET title = COALESCE(title, NULLIF(p_topic, '')) - WHERE id = v_room_id; - END IF; + -- Try insert; if conflict on nonce, fall back to select. This is idempotent and race-safe. + v_room_id := NULL; + INSERT INTO rooms (title, client_nonce) + VALUES (NULLIF(p_topic, ''), v_client_nonce) + ON CONFLICT (client_nonce) DO NOTHING + RETURNING id INTO v_room_id; + + IF v_room_id IS NULL AND v_client_nonce IS NOT NULL THEN + SELECT id INTO v_room_id FROM rooms WHERE client_nonce = v_client_nonce; + ELSE + v_created := true; + END IF; + + UPDATE rooms + SET title = COALESCE(title, NULLIF(p_topic, '')) + WHERE id = v_room_id;This delivers true idempotency under retries. If you insist on a single statement, use ON CONFLICT ... DO UPDATE RETURNING, but then you’ll need a reliable way to detect “inserted vs conflicted” if you want to gate seeding. Based on learnings.
41-41: Kill the GREATEST(...) in generate_series. It masks bugs.After proper bounds checks, use the validated value directly. Keeping GREATEST here hides upstream errors.
- FROM generate_series(1, GREATEST(v_participants, 1)) AS gs + FROM generate_series(1, v_participants) AS gs
34-43: Seeding guard confirmed: v_created only set on initial insert
v_created is assigned true only in the INSERT branch and remains false on retries (SELECT/UPDATE path), so rounds/participants won’t be re-seeded.
| v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); | ||
| v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); | ||
| v_now integer := extract(epoch from now())::int; | ||
| v_submit_deadline integer := v_now + GREATEST(v_submit_minutes, 1) * 60; | ||
| v_client_nonce text := NULLIF(p_client_nonce, ''); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Non-numeric JSON will blow up your casts. Decide: strict error vs graceful default.
(p_cfg->>'…')::int throws on “3min” or "04a". If that’s not desired, guard with a regex and default. If strict is desired, add explicit ERROR CODE and message so clients get a sane 400-class mapping.
I can patch to:
v_participants := COALESCE(CASE WHEN (p_cfg->>'participant_count') ~ '^\d+$' THEN (p_cfg->>'participant_count')::int END, 4);Confirm preference.
🤖 Prompt for AI Agents
In db/rpc.sql around lines 12–16, the JSON-to-int casts will raise on
non-numeric strings; switch to a guarded parse with a regex fallback so
malformed values gracefully default (participants -> 4, submit_minutes -> 5) and
keep v_now/v_submit_deadline logic unchanged. Replace each (p_cfg->>'...')::int
with a CASE/WHEN that checks the value matches '^\d+$' before casting, then
COALESCE to the default; keep v_client_nonce NULLIF behavior. Ensure both
participant_count and submit_minutes use the same guarded pattern.
| client_nonce text UNIQUE, | ||
| created_at timestamptz NOT NULL DEFAULT now() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Decide your nonce semantics and make the DDL match.
UNIQUE on client_nonce allows multiple NULLs. If room_create defaults p_client_nonce to NULL (docs say it can), you’ll happily create duplicate rooms. Either:
- Enforce NOT NULL and make callers pass a nonce, or
- Keep NULLs allowed but make uniqueness explicit with a partial unique index.
If you want “unique when present,” change to:
- client_nonce text UNIQUE,
+ client_nonce text,and add after the table:
CREATE UNIQUE INDEX IF NOT EXISTS uniq_rooms_client_nonce
ON rooms (client_nonce)
WHERE client_nonce IS NOT NULL;If you want strict idempotency always, mark client_nonce NOT NULL and keep the UNIQUE.
🤖 Prompt for AI Agents
In db/schema.sql around lines 11-12, the client_nonce column is declared UNIQUE
but allows NULLs so multiple rows can have NULL and break idempotency; decide
desired semantics and update the DDL accordingly: if you want uniqueness only
when a nonce is provided, keep the column nullable and add a partial unique
index on client_nonce WHERE client_nonce IS NOT NULL; if you want strict
idempotency, change the column to NOT NULL (and leave UNIQUE) and ensure callers
always supply a nonce.
Added the FK constraints and nonce index (see commit c34e2b0); integration tests now seed participants/ |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
db/rpc.sql (3)
97-109: Make vote_submit one statement; stop round-tripping.INSERT … ON CONFLICT DO NOTHING + SELECT is clumsy. Return the id directly via DO UPDATE RETURNING.
- INSERT INTO votes (room_id, round_id, voter_id, kind, ballot, client_nonce) - VALUES (p_room_id, p_round_id, p_voter_id, p_kind, p_ballot, p_client_nonce) - ON CONFLICT (round_id, voter_id, kind, client_nonce) - DO NOTHING - RETURNING id INTO v_id; - IF v_id IS NULL THEN - SELECT id INTO v_id FROM votes - WHERE round_id = p_round_id AND voter_id = p_voter_id AND kind = p_kind AND client_nonce = p_client_nonce - LIMIT 1; - END IF; + INSERT INTO votes (room_id, round_id, voter_id, kind, ballot, client_nonce) + VALUES (p_room_id, p_round_id, p_voter_id, p_kind, p_ballot, p_client_nonce) + ON CONFLICT (round_id, voter_id, kind, client_nonce) + DO UPDATE SET ballot = votes.ballot + RETURNING id INTO v_id;Also, validate p_kind against the allowed set (CHECK or enum). Add a test.
131-153: Rounds get stuck forever when there are zero votes. Your SUMs return NULL. Fix it.With no votes, yes/no are NULL; all your comparisons short-circuit and nothing advances/finalizes. COALESCE to 0 and simplify.
), tallied AS ( SELECT d.room_id, d.id as round_id, - SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='continue' THEN 1 ELSE 0 END) AS yes, - SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='end' THEN 1 ELSE 0 END) AS no + COALESCE(SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='continue' THEN 1 ELSE 0 END), 0) AS yes, + COALESCE(SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='end' THEN 1 ELSE 0 END), 0) AS no FROM due d LEFT JOIN votes v ON v.round_id = d.id GROUP BY d.room_id, d.id ), winners AS ( SELECT t.*, r.idx FROM tallied t JOIN rounds r ON r.id = t.round_id ) - UPDATE rounds r SET phase = CASE WHEN w.yes > w.no THEN r.phase ELSE 'final' END - FROM winners w - WHERE r.id = w.round_id AND w.yes <= w.no; + UPDATE rounds r + SET phase = 'final' + FROM winners w + WHERE r.id = w.round_id AND w.yes <= w.no; @@ - WHERE w.yes > w.no + WHERE w.yes > w.noAdd pgTAP to assert a published round with zero votes becomes final. Do it. Yesterday.
163-169: Your tally view also leaks NULLs. Normalize it.Expose zeros, not NULL, or clients will branch on nonsense.
- SELECT r.room_id, r.id as round_id, - SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='continue' THEN 1 ELSE 0 END) AS yes, - SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='end' THEN 1 ELSE 0 END) AS no + SELECT r.room_id, r.id as round_id, + COALESCE(SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='continue' THEN 1 ELSE 0 END), 0) AS yes, + COALESCE(SUM(CASE WHEN v.kind='continue' AND (v.ballot->>'choice')='end' THEN 1 ELSE 0 END), 0) AS no
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
AGENTS.mdis excluded by!*.md
📒 Files selected for processing (2)
db/rpc.sql(1 hunks)db/test/30_rpcs.pgtap(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
db/test/30_rpcs.pgtapdb/rpc.sql
🔇 Additional comments (7)
db/rpc.sql (5)
19-27: Bounds check: good. Now test the edges.The [1..64] and [1..1440] checks are sane. Add tests for exactly 1, 64, 1440 to prevent future “off-by-one optimizations.”
31-44: Stop the SELECT-then-INSERT nonsense—this is racy under concurrency. Use an upsert.Two clients with the same nonce can both miss the SELECT and race the INSERT. If you truly have a UNIQUE on rooms.client_nonce, this blows up with an unhandled exception; if you don’t, you mint dup rooms. Fix it with a single INSERT … ON CONFLICT … RETURNING and make seeding idempotent (it already is).
Apply this diff:
- IF v_client_nonce IS NOT NULL THEN - SELECT id INTO v_room_id FROM rooms WHERE client_nonce = v_client_nonce; - END IF; - - IF v_room_id IS NULL THEN - INSERT INTO rooms (title, client_nonce) - VALUES (NULLIF(p_topic, ''), v_client_nonce) - RETURNING id INTO v_room_id; - v_created := true; - ELSE - UPDATE rooms - SET title = COALESCE(title, NULLIF(p_topic, '')) - WHERE id = v_room_id; - END IF; + INSERT INTO rooms (title, client_nonce) + VALUES (NULLIF(p_topic, ''), v_client_nonce) + ON CONFLICT (client_nonce) + DO UPDATE SET title = COALESCE(rooms.title, EXCLUDED.title) + RETURNING id INTO v_room_id; + -- Seeding below is safe to run every time because of ON CONFLICT DO NOTHING. + v_created := true;And drop the conditional around seeding (see next hunk).
12-16: Casting JSON with ::int will explode on garbage. Pick a policy (strict vs graceful) and implement it.Right now “3min” blows up with a generic error, not your range checks. Either validate and raise a clear 22023, or guard-parse and default. Do it consistently for both fields.
Guarded-parse option:
- v_participants integer := COALESCE((p_cfg->>'participant_count')::int, 4); - v_submit_minutes integer := COALESCE((p_cfg->>'submit_minutes')::int, 5); + v_participants integer := COALESCE( + CASE WHEN (p_cfg->>'participant_count') ~ '^\d+$' THEN (p_cfg->>'participant_count')::int END, 4); + v_submit_minutes integer := COALESCE( + CASE WHEN (p_cfg->>'submit_minutes') ~ '^\d+$' THEN (p_cfg->>'submit_minutes')::int END, 5);Strict-with-message option:
IF p_cfg ? 'participant_count' AND NOT (p_cfg->>'participant_count') ~ '^\d+$' THEN RAISE EXCEPTION 'participant_count must be an integer string' USING ERRCODE='22023'; END IF;Which behavior do you want? Add pgTAP to lock it down.
3-7: Document the damn contract inline.State defaults and bounds for participant_count [1..64], submit_minutes [1..1440], and the idempotency nonce semantics. Keep callers and tests honest.
31-38: WHAT THE HELL –client_nonceis already UNIQUE in the schema
Theclient_noncecolumn indb/schema.sqlis declaredtext UNIQUE(line 11), so idempotency is already enforced.Likely an incorrect or invalid review comment.
db/test/30_rpcs.pgtap (2)
52-56: Stop relying on search_path roulette. Qualify your functions.Qualify all to_regprocedure() checks with public. You fixed one and forgot the rest.
-SELECT ok( - to_regprocedure('submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL, +SELECT ok( + to_regprocedure('public.submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL, @@ -SELECT ok( - to_regprocedure('vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL, +SELECT ok( + to_regprocedure('public.vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL, @@ -SELECT ok(to_regprocedure('round_publish_due()') IS NOT NULL, 'round_publish_due exists'); -SELECT ok(to_regprocedure('round_open_next()') IS NOT NULL, 'round_open_next exists'); +SELECT ok(to_regprocedure('public.round_publish_due()') IS NOT NULL, 'round_publish_due exists'); +SELECT ok(to_regprocedure('public.round_open_next()') IS NOT NULL, 'round_open_next exists');Also applies to: 86-90, 121-122
124-129: Running test coverage search… results incoming.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
db/test/30_rpcs.pgtap (1)
63-66: Stop playing search_path roulette. Use pgTAP has_function and qualify schema.Qualify all RPC existence checks (room_create already is; the rest aren’t).
-SELECT ok( - to_regprocedure('submission_upsert(uuid,uuid,text,jsonb,jsonb,text,text)') IS NOT NULL, - 'submission_upsert exists' -); +SELECT has_function('public','submission_upsert', ARRAY['uuid','uuid','text','jsonb','jsonb','text','text']); -SELECT ok( - to_regprocedure('vote_submit(uuid,uuid,uuid,text,jsonb,text)') IS NOT NULL, - 'vote_submit exists' -); +SELECT has_function('public','vote_submit', ARRAY['uuid','uuid','uuid','text','jsonb','text']); -SELECT ok(to_regprocedure('round_publish_due()') IS NOT NULL, 'round_publish_due exists'); -SELECT ok(to_regprocedure('round_open_next()') IS NOT NULL, 'round_open_next exists'); +SELECT has_function('public','round_publish_due', ARRAY[]::text[]); +SELECT has_function('public','round_open_next', ARRAY[]::text[]);Based on learnings.
Also applies to: 97-100, 131-132
db/schema.sql (4)
66-76: Enforce (room_id, round_id) coherence in votes or drop room_id.Right now nothing stops votes.room_id disagreeing with rounds.room_id. Add a composite FK.
CREATE TABLE IF NOT EXISTS votes ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), - room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, + room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, round_id uuid NOT NULL REFERENCES rounds(id) ON DELETE CASCADE, voter_id uuid NOT NULL REFERENCES participants(id) ON DELETE CASCADE, @@ UNIQUE (round_id, voter_id, kind, client_nonce) ); +-- Ensure room/round coherence for votes +CREATE UNIQUE INDEX IF NOT EXISTS uniq_rounds_room_id_id ON rounds (room_id, id); +ALTER TABLE votes + ADD CONSTRAINT fk_votes_room_round + FOREIGN KEY (room_id, round_id) REFERENCES rounds(room_id, id) + ON DELETE CASCADE;If you don’t want the redundancy, drop votes.room_id instead. Pick one and be consistent.
Also applies to: 78-78
19-23: 2038 time-bomb: stop storing epoch seconds in int4.Use bigint or, better, timestamptz. Don’t be that team.
- submit_deadline_unix integer NOT NULL DEFAULT 0, - published_at_unix integer, - continue_vote_close_unix integer, + submit_deadline_unix bigint NOT NULL DEFAULT 0, + published_at_unix bigint, + continue_vote_close_unix bigint,Or replace these with proper timestamptz columns and drop the “_unix” misfeature.
50-58: canonical_sha256 should be exactly 64 hex chars, not “some text”.Tighten the type/constraint.
- canonical_sha256 text NOT NULL, + canonical_sha256 char(64) NOT NULL CHECK (canonical_sha256 ~ '^[0-9a-f]{64}$'),
71-76: Lock down vote kinds. Free‑form text invites garbage.Add a CHECK or an enum.
- kind text NOT NULL, -- e.g., 'continue' + kind text NOT NULL CHECK (kind IN ('continue')),Expand the set as you add flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
db/schema.sql(3 hunks)db/test/30_rpcs.pgtap(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
db/schema.sqldb/test/30_rpcs.pgtap
🔇 Additional comments (6)
db/schema.sql (2)
61-62: Drop the duplicate nonce index. PostgreSQL already built it for the UNIQUE.You are doubling write amplification for nothing. Kill it.
-CREATE INDEX IF NOT EXISTS idx_submissions_nonce ON submissions (round_id, author_id, client_nonce);
11-11: Nullable UNIQUE nonce = idempotency hole. Decide and fix it.Either require a nonce (NOT NULL) or make uniqueness “when present” via partial index. Pick one and stop the footgun.
--- Option A: strict idempotency - client_nonce text UNIQUE, + client_nonce text UNIQUE NOT NULL, --- Option B: unique-when-present - client_nonce text UNIQUE, + client_nonce text, +); +CREATE UNIQUE INDEX IF NOT EXISTS uniq_rooms_client_nonce + ON rooms (client_nonce) WHERE client_nonce IS NOT NULL;db/test/30_rpcs.pgtap (4)
40-50: Test the edges, not just the failures.Add acceptance tests for min/max bounds.
SELECT throws_ok( @@ 'room_create rejects submit_minutes above range' ); + +-- Accepts min/max bounds +SELECT ok(room_create('Edge Low', '{"participant_count":1,"submit_minutes":1}'::jsonb, 'nonce-room-edge-1') IS NOT NULL, 'accepts min bounds'); +SELECT ok(room_create('Edge High', '{"participant_count":64,"submit_minutes":1440}'::jsonb,'nonce-room-edge-2') IS NOT NULL, 'accepts max bounds');Based on learnings.
102-128: vote_submit: Count both nonces. Prove two rows exist.You stop at “different id”. Count to 2.
SELECT vote_submit(v_room, v_round, voter, 'continue', '{}'::jsonb, n2) INTO id3; PERFORM ok(id3 <> id1, 'vote_submit creates new row for different nonce'); + PERFORM ok( + (SELECT count(*) FROM votes WHERE round_id = v_round AND voter_id = voter AND kind='continue') = 2, + 'vote_submit stores two rows for two different nonces' + );Based on learnings.
69-94: submission_upsert: Assert the ON CONFLICT actually updates.Idempotency is not enough; prove the DO UPDATE sets canonical.
SELECT submission_upsert(round, author, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n) INTO id1; SELECT submission_upsert(round, author, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n) INTO id2; PERFORM ok(id1 = id2, 'submission_upsert idempotent for same nonce'); + -- Change canonical and ensure the row updates on same nonce + PERFORM submission_upsert(round, author, 'hello', '[]'::jsonb, '[]'::jsonb, 'cafebabe', n); + PERFORM ok((SELECT canonical_sha256 FROM submissions WHERE id = id1) = 'cafebabe', + 'submission_upsert updates canonical on conflict');Based on learnings.
22-38: Idempotency isn’t “same id”. Prove “no duplicate seeds” for participants and rounds.Right now you could silently double-seed. Count before/after with same nonce.
DECLARE new_room uuid; participant_count integer; + participant_count_after integer; round_deadline integer; round_phase text; round_idx integer; role_count integer; same_room uuid; new_room2 uuid; + round_count integer; + round_count_after integer; BEGIN SELECT room_create('Test Room', '{"participant_count":3,"submit_minutes":1}'::jsonb, 'nonce-room-1') INTO new_room; SELECT count(*) INTO participant_count FROM participants WHERE room_id = new_room; + SELECT count(*) INTO round_count FROM rounds WHERE room_id = new_room; @@ SELECT room_create('Updated Title', '{"participant_count":5}'::jsonb, 'nonce-room-1') INTO same_room; PERFORM ok(same_room = new_room, 'room_create reuses room id for same nonce'); + SELECT count(*) INTO participant_count_after FROM participants WHERE room_id = new_room; + PERFORM ok(participant_count_after = participant_count, 'room_create does not duplicate participants on retry'); + SELECT count(*) INTO round_count_after FROM rounds WHERE room_id = new_room; + PERFORM ok(round_count_after = round_count, 'room_create does not create extra rounds on retry');Based on learnings.
| @@ -24,11 +25,25 @@ CREATE TABLE IF NOT EXISTS rounds ( | |||
|
|
|||
| CREATE INDEX IF NOT EXISTS idx_rounds_room_idx ON rounds (room_id, idx DESC); | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant rounds index. Drop it.
UNIQUE(room_id, idx) already gives you a btree usable in descending scans; the extra DESC index is waste.
-CREATE INDEX IF NOT EXISTS idx_rounds_room_idx ON rounds (room_id, idx DESC);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE INDEX IF NOT EXISTS idx_rounds_room_idx ON rounds (room_id, idx DESC); |
🤖 Prompt for AI Agents
In db/schema.sql around line 26, remove the redundant index creation "CREATE
INDEX IF NOT EXISTS idx_rounds_room_idx ON rounds (room_id, idx DESC);" because
the existing UNIQUE(room_id, idx) already provides a btree that supports
descending scans; simply delete this CREATE INDEX statement to avoid wasted
storage and maintenance overhead.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
db/schema.sql (2)
20-23: *Use bigint for _unix columns or enjoy overflow later.Epoch seconds won’t fit in int4 long‑term. Change rounds.*_unix to bigint.
- submit_deadline_unix integer NOT NULL DEFAULT 0, - published_at_unix integer, - continue_vote_close_unix integer, + submit_deadline_unix bigint NOT NULL DEFAULT 0, + published_at_unix bigint, + continue_vote_close_unix bigint,
66-76: Normalize votes: remove denormalized room_id or enforce a composite FK.Right now the schema allows mismatched (room_id, round_id). Either:
- Remove room_id from votes, or
- Add UNIQUE on rounds(room_id, id) and a FK votes(room_id, round_id) → rounds(room_id, id).
Prefer removal to avoid duplication and bugs.
db/rpc.sql (1)
89-113: Prevent mismatched room_id and round_id in votesvotes currently allows inserting a vote with a room_id that doesn’t match the referenced round’s room. Choices:
- Drop votes.room_id and derive it via JOIN on rounds.
- Add a composite FK (room_id, round_id) → rounds(room_id, id) (with a UNIQUE constraint on rounds(room_id, id)).
I recommend dropping votes.room_id and simplifying vote_submit accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
db/rpc.sql(1 hunks)db/schema.sql(3 hunks)db/test/10_rooms_rounds.pgtap(2 hunks)docs/LocalDB.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
db/rpc.sqldocs/LocalDB.mddb/test/10_rooms_rounds.pgtapdb/schema.sql
🔇 Additional comments (4)
db/schema.sql (3)
61-61: Drop this duplicate submissions index; the UNIQUE covers the prefix.Btree on (round_id, author_id, client_nonce) serves (round_id, author_id) lookups.
-CREATE INDEX IF NOT EXISTS idx_submissions_round_author ON submissions (round_id, author_id);
26-26: Drop the redundant rounds index. UNIQUE(room_id, idx) already covers it.It wastes write I/O and storage.
-CREATE INDEX IF NOT EXISTS idx_rounds_room_idx ON rounds (room_id, idx DESC);
11-11: Optional: make nonce semantics explicit and shrink the index.If NULL nonces are allowed (docs say optional), use a partial UNIQUE to exclude NULLs and reduce bloat.
- client_nonce text UNIQUE, + client_nonce text,Then:
CREATE UNIQUE INDEX IF NOT EXISTS uniq_rooms_client_nonce ON rooms (client_nonce) WHERE client_nonce IS NOT NULL;db/rpc.sql (1)
16-17: Casting untrusted JSON with ::int will throw opaque 22P02. Decide strict vs forgiving and document it.Either guard with a numeric regex and default, or raise 22023 with a clear message to map to a 4xx. Don’t leave clients guessing.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/Features.md (1)
13-20: Pick a name and stick to it. You document anon_1..anon_5, but the code seeds agent_1..N.Align docs to the implementation or, better, fix the implementation to anon_* and keep the docs. Right now you’ll confuse every client and test.
docs/Architecture.md (1)
709-745: Your sample server code doesn’t enforce Zod at the edges for votes and still accesses RPCs loosely.Per coding guidelines, validate inputs with Zod and pass client_nonce. Show the vote.continue route calling vote_submit with the 5‑arg signature. Fix the example or it will mislead implementers.
Also applies to: 747-753
server/test/rpc.db.integration.test.js (1)
27-34: Stop smuggling room_id — enforce the 5‑arg vote_submit contract.Remove room_id from all /rpc/vote.continue request bodies in tests and assert the DB call uses exactly 5 params (round_id, voter_id, kind, ballot, client_nonce).
Files to update: server/test/rpc.db.integration.test.js, server/test/rpc.db.postgres.test.js, server/test/rpc.vote_continue.test.js, server/test/watcher.transitions.test.js, server/test/state.enrichment.test.js
Add assertion in FakePool checks:
expect(voteCalls.at(-1).params.length).toBe(5); // (round_id, voter_id, kind, ballot, client_nonce)Add a negative test that missing client_nonce is rejected once the DB enforces it.
db/schema.sql (1)
45-58: Hard integrity hole: submissions can cross-contaminate rooms. Fix it with a constraint trigger.You FK to rounds and participants independently. Nothing stops author_id from room A submitting into a round in room B. That’s garbage data.
Add a DEFERRABLE constraint trigger to enforce (participants.room_id == rounds.room_id):
CREATE OR REPLACE FUNCTION check_submission_room_coherence() RETURNS trigger LANGUAGE plpgsql AS $$ DECLARE p_room uuid; r_room uuid; BEGIN SELECT room_id INTO p_room FROM participants WHERE id = NEW.author_id; SELECT room_id INTO r_room FROM rounds WHERE id = NEW.round_id; IF p_room IS NULL OR r_room IS NULL OR p_room <> r_room THEN RAISE EXCEPTION 'submissions(author_id %, round_id %) violate room coherence (% vs %)', NEW.author_id, NEW.round_id, p_room, r_room USING ERRCODE = '23514'; END IF; RETURN NEW; END $$; DROP TRIGGER IF EXISTS trg_submissions_room_coherence ON submissions; CREATE CONSTRAINT TRIGGER trg_submissions_room_coherence AFTER INSERT OR UPDATE OF round_id, author_id ON submissions DEFERRABLE INITIALLY DEFERRED FOR EACH ROW EXECUTE FUNCTION check_submission_room_coherence();While you’re here: stop using char(64) for hashes. It’s slow and pads spaces.
- canonical_sha256 char(64) NOT NULL CHECK (canonical_sha256 ~ '^[0-9a-f]{64}$'), + canonical_sha256 text NOT NULL CHECK (char_length(canonical_sha256) = 64 AND canonical_sha256 ~ '^[0-9a-f]{64}$'),And couple signature fields so half‑filled signatures don’t creep in:
ALTER TABLE submissions ADD CONSTRAINT chk_signature_fields CHECK ( (signature_kind IS NULL AND signature_b64 IS NULL AND signer_fingerprint IS NULL) OR (signature_kind IS NOT NULL AND signature_b64 IS NOT NULL) );Finally, define allowed status values or you’ll get a zoo. Add a CHECK once you finalize the enum.
I can add pgTAP tests that prove the cross-room insert/update fails and signature coupling works. Say the word.
Also applies to: 52-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
AGENTS.mdis excluded by!*.md
📒 Files selected for processing (11)
db/rpc.sql(4 hunks)db/schema.sql(3 hunks)db/test/10_rooms_rounds.pgtap(3 hunks)db/test/20_submissions_votes.pgtap(1 hunks)db/test/30_rpcs.pgtap(1 hunks)docs/Architecture.md(24 hunks)docs/Features.md(6 hunks)docs/LocalDB.md(1 hunks)server/rpc.js(1 hunks)server/test/rpc.db.integration.test.js(1 hunks)server/test/rpc.db.postgres.test.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
@(server|web|cli)/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Validate external inputs with Zod at the edges (server/controllers, web handlers/forms, CLI parsing)
Files:
server/test/rpc.db.postgres.test.jsserver/test/rpc.db.integration.test.jsserver/rpc.js
**/*.{test,spec}.js
📄 CodeRabbit inference engine (AGENTS.md)
Write minimal Vitest tests for canonical JSON and RPC schema validation
Files:
server/test/rpc.db.postgres.test.jsserver/test/rpc.db.integration.test.js
server/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not access process.env directly in server code; use SecretSource and ConfigBuilder
Files:
server/test/rpc.db.postgres.test.jsserver/test/rpc.db.integration.test.jsserver/rpc.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Applies to db/test/**/*.sql : Write/adjust pgTAP invariants for rooms, rounds, and submissions
Applied to files:
server/test/rpc.db.postgres.test.jsdb/test/20_submissions_votes.pgtapdb/test/30_rpcs.pgtapdb/test/10_rooms_rounds.pgtapdb/rpc.sqldb/schema.sqldocs/LocalDB.mddocs/Architecture.md
📚 Learning: 2025-09-29T10:17:53.577Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T10:17:53.577Z
Learning: Keep server and watcher thin; push heavy work to Supabase (SQL/RPC/RLS) or the worker
Applied to files:
docs/Architecture.md
🔇 Additional comments (4)
docs/LocalDB.md (1)
27-27: DOCUMENT THE ACTUAL FUNCTION SIGNATURE.
Line 27: The SQL definesroom_create(topic text, cfg jsonb DEFAULT '{}'::jsonb, client_nonce text DEFAULT NULL), but this doc pretends the third arg is a mandatorynonce text. Stop misleading users and write down the real contract.-The new `room_create(topic text, cfg jsonb DEFAULT '{}'::jsonb, nonce text)` RPC seeds a room, round 0, and a roster of anonymous participants. +The new `room_create(topic text, cfg jsonb DEFAULT '{}'::jsonb, client_nonce text DEFAULT NULL)` RPC seeds a room, round 0, and a roster of anonymous participants.db/rpc.sql (1)
16-20: Stop throwing 22P02 on garbage JSON. Validate and raise 22023 consistently.Direct casts (p_cfg->>'…')::int will explode with 22P02 before your bounds checks. Guard the parse and raise a clean 22023.
Apply:
- v_participants integer := COALESCE(NULLIF(p_cfg->>'participant_count', '')::int, 4); - v_submit_minutes integer := COALESCE(NULLIF(p_cfg->>'submit_minutes', '')::int, 5); + v_participants integer := COALESCE( + CASE WHEN (p_cfg->>'participant_count') ~ '^\d+$' THEN (p_cfg->>'participant_count')::int END, + 4 + ); + v_submit_minutes integer := COALESCE( + CASE WHEN (p_cfg->>'submit_minutes') ~ '^\d+$' THEN (p_cfg->>'submit_minutes')::int END, + 5 + );And keep the existing range checks to raise SQLSTATE 22023. Based on learnings.
db/schema.sql (2)
11-12: Nonce semantics are wrong. Decide and fix, or you’ll create duplicate rooms.UNIQUE with NULL allows unlimited NULLs. If callers can omit client_nonce, your idempotency is a joke.
Pick one:
- Strict idempotency: make it NOT NULL and keep UNIQUE.
- “Unique when present”: drop the inline UNIQUE and add a partial UNIQUE index.
Patch (unique-when-present):
CREATE TABLE IF NOT EXISTS rooms ( id uuid PRIMARY KEY DEFAULT gen_random_uuid(), title text, - client_nonce text UNIQUE, + client_nonce text, created_at timestamptz NOT NULL DEFAULT now() ); + +-- Ensure uniqueness only when provided +CREATE UNIQUE INDEX IF NOT EXISTS uniq_rooms_client_nonce + ON rooms (client_nonce) + WHERE client_nonce IS NOT NULL;State explicitly in docs whether client_nonce is required. If required, flip to NOT NULL instead of the partial index.
26-26: Drop the redundant rounds index. It buys you nothing and costs writes.UNIQUE(room_id, idx) already provides a btree that supports DESC scans. Delete this duplicate.
-CREATE INDEX IF NOT EXISTS idx_rounds_room_idx ON rounds (room_id, idx DESC);
| INSERT INTO rounds (room_id, idx, phase, submit_deadline_unix) | ||
| VALUES (v_room_id, 0, 'submit', v_submit_deadline) | ||
| ON CONFLICT (room_id, idx) DO NOTHING; | ||
|
|
There was a problem hiding this comment.
Round engine logic is bare‑bones; document “planned” or ship the missing invariants.
- No soft quorum.
- No per‑room advisory lock on open_next.
- Views tally only 'continue' ballots; fine, but make the semantics explicit.
Add TODOs or implement. Don’t pretend it’s complete.
Also applies to: 134-143, 155-156
🤖 Prompt for AI Agents
In db/rpc.sql around lines 40-43 (and similarly at 134-143 and 155-156), the
round-engine logic is incomplete and lacks explicit invariants: there is no
soft-quorum handling, no per-room advisory lock around open_next, and the
views/tally semantics (only counting 'continue' ballots) are not documented. Add
concise TODO comments at these locations (or implement the behavior) stating:
(1) soft-quorum policy needed and where to enforce it; (2) acquire/release a
per-room advisory lock around open_next to prevent concurrent round opens; and
(3) document that view-tally only considers 'continue' ballots and define the
expected outcome for other ballot types. Ensure each TODO references the
invariant it addresses and points to where a full implementation should be added
or why it is intentionally omitted.
| INSERT INTO participants (room_id, anon_name, role) | ||
| SELECT v_room_id, format('agent_%s', gs), 'debater' | ||
| FROM generate_series(1, v_participants) AS gs | ||
| ON CONFLICT (room_id, anon_name) DO NOTHING; |
There was a problem hiding this comment.
🧩 Analysis chain
Why are you seeding agent_%s when the rest of the stack says anon_%s? Pick one.
This discrepancy will break RLS mappings, docs, and clients. Use anon_%s.
- SELECT v_room_id, format('agent_%s', gs), 'debater'
+ SELECT v_room_id, format('anon_%s', gs), 'debater'🏁 Script executed:
#!/bin/bash
rg -n "anon_" -n -C2
rg -n "agent_" -n -C2Length of output: 4402
🏁 Script executed:
#!/bin/bash
# Display lines around the participants insert in db/rpc.sql
sed -n '1,100p' db/rpc.sqlLength of output: 3153
Replace agent_%s with anon_%s when seeding participants
Seeding names with agent_%s conflicts with the anon_%s prefix used in UI, tests and RLS policies—change to anon_%s to align the stack.
- SELECT v_room_id, format('agent_%s', gs), 'debater'
+ SELECT v_room_id, format('anon_%s', gs), 'debater'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSERT INTO participants (room_id, anon_name, role) | |
| SELECT v_room_id, format('agent_%s', gs), 'debater' | |
| FROM generate_series(1, v_participants) AS gs | |
| ON CONFLICT (room_id, anon_name) DO NOTHING; | |
| INSERT INTO participants (room_id, anon_name, role) | |
| SELECT v_room_id, format('anon_%s', gs), 'debater' | |
| FROM generate_series(1, v_participants) AS gs | |
| ON CONFLICT (room_id, anon_name) DO NOTHING; |
🤖 Prompt for AI Agents
In db/rpc.sql around lines 44 to 47, the INSERT uses format('agent_%s', gs) to
seed participant anon names but the rest of the stack expects the anon_%s
prefix; change the format string to format('anon_%s', gs) so seeded anon_name
values use the anon_ prefix (keep the rest of the INSERT, including ON CONFLICT
DO NOTHING, unchanged).
| IF p_kind <> 'continue' THEN | ||
| RAISE EXCEPTION 'unsupported vote kind: %', p_kind USING ERRCODE = '22023'; | ||
| END IF; | ||
|
|
||
| INSERT INTO votes (round_id, voter_id, kind, ballot, client_nonce) | ||
| VALUES (p_round_id, p_voter_id, p_kind, p_ballot, p_client_nonce) | ||
| ON CONFLICT (round_id, voter_id, kind, client_nonce) | ||
| DO NOTHING | ||
| DO UPDATE SET ballot = votes.ballot | ||
| RETURNING id INTO v_id; |
There was a problem hiding this comment.
🧩 Analysis chain
Your vote idempotency is broken. Different nonces = unlimited votes per voter.
UNIQUE on (round_id, voter_id, kind, client_nonce) permits a user to cast N votes by changing the nonce. That’s amateur hour.
- Enforce one vote per (round_id, voter_id, kind).
- Keep client_nonce only for retry dedupe.
If the schema has UNIQUE(round_id, voter_id, kind), use it:
- INSERT INTO votes (round_id, voter_id, kind, ballot, client_nonce)
+ INSERT INTO votes (round_id, voter_id, kind, ballot, client_nonce)
VALUES (p_round_id, p_voter_id, p_kind, p_ballot, p_client_nonce)
- ON CONFLICT (round_id, voter_id, kind, client_nonce)
- DO UPDATE SET ballot = votes.ballot
+ ON CONFLICT (round_id, voter_id, kind)
+ DO UPDATE SET ballot = votes.ballot
RETURNING id INTO v_id;If the triple UNIQUE doesn’t exist yet, add it in schema and keep a separate covering index for (round_id, voter_id, kind, client_nonce) if you insist. Also, VALIDATE the ballot:
IF p_kind <> 'continue' THEN
RAISE EXCEPTION 'unsupported vote kind: %', p_kind USING ERRCODE = '22023';
END IF;
+ IF (p_ballot->>'choice') NOT IN ('continue','end') THEN
+ RAISE EXCEPTION 'invalid continue ballot.choice: %', p_ballot->>'choice' USING ERRCODE = '22023';
+ END IF;🏁 Script executed:
#!/bin/bash
# Show vote uniques in schema
rg -nP "(create\s+table\s+votes|unique\s*\()" db -n -C3Length of output: 2965
Enforce a true one-vote policy per (round_id, voter_id, kind)
You’re letting users flood votes by swapping nonces—unacceptable. Fix it NOW:
- Add a
UNIQUE(round_id, voter_id, kind)constraint to thevotestable to ban duplicates outright. - Change the ON CONFLICT clause in
db/rpc.sqlto use(round_id, voter_id, kind)only—dropclient_noncethere. - Preserve a separate covering index on
(round_id, voter_id, kind, client_nonce)solely for retry dedupe, if you insist. - Validate
p_ballot->>'choice'against the allowed set ('continue','end') before inserting.
Diff example:
- ON CONFLICT (round_id, voter_id, kind, client_nonce)
+ ON CONFLICT (round_id, voter_id, kind)
DO UPDATE SET ballot = votes.ballotUpdate your schema migrations and pgtap tests to verify the new UNIQUE and remove the old four-column constraint. No excuses.
| submit_deadline_unix bigint NOT NULL DEFAULT 0, | ||
| published_at_unix bigint, | ||
| continue_vote_close_unix bigint, | ||
| UNIQUE (room_id, idx) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stop mixing time types. Use timestamptz or at least fence the epoch ints.
bigint “*_unix” beside timestamptz elsewhere is a footgun. Either store timestamptz or add checks + generated views to avoid off‑by‑timezone bugs.
Minimal hardening:
phase text NOT NULL DEFAULT 'submit' CHECK (phase IN ('submit','published','final')),
- submit_deadline_unix bigint NOT NULL DEFAULT 0,
- published_at_unix bigint,
- continue_vote_close_unix bigint,
+ submit_deadline_unix bigint NOT NULL DEFAULT 0 CHECK (submit_deadline_unix >= 0),
+ published_at_unix bigint CHECK (published_at_unix IS NULL OR published_at_unix >= 0),
+ continue_vote_close_unix bigint CHECK (continue_vote_close_unix IS NULL OR continue_vote_close_unix >= 0),Optionally expose sane columns (clients can keep using epoch):
ALTER TABLE rounds
ADD COLUMN IF NOT EXISTS submit_deadline_ts timestamptz
GENERATED ALWAYS AS (to_timestamp(submit_deadline_unix)) STORED,
ADD COLUMN IF NOT EXISTS published_at_ts timestamptz
GENERATED ALWAYS AS (CASE WHEN published_at_unix IS NULL THEN NULL ELSE to_timestamp(published_at_unix) END) STORED,
ADD COLUMN IF NOT EXISTS continue_vote_close_ts timestamptz
GENERATED ALWAYS AS (CASE WHEN continue_vote_close_unix IS NULL THEN NULL ELSE to_timestamp(continue_vote_close_unix) END) STORED;Do your queries need range scans over these timestamps? If yes, add btree indexes on the generated timestamptz columns instead of raw bigint.
🤖 Prompt for AI Agents
In db/schema.sql around lines 20 to 23, the schema mixes bigint epoch columns
with timestamptz elsewhere which is error-prone; add generated timestamptz
columns (or convert the bigint columns to timestamptz) and/or add sanity checks
so timestamps aren’t stored as raw ints without timezone context. Implement
generated STORED timestamptz columns (e.g., to_timestamp(...) or NULL-safe
variants) for submit_deadline_unix, published_at_unix and
continue_vote_close_unix so clients can continue using epoch ints but server
queries use proper timestamptz; if your queries do range scans on these
timestamps, create btree indexes on the generated timestamptz columns instead of
the raw bigint; also consider adding CHECK constraints or migrations to validate
epoch ranges if you keep the bigint columns.
| -- Participants: seeded roster for each room / agent configuration | ||
| CREATE TABLE IF NOT EXISTS participants ( | ||
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| room_id uuid NOT NULL REFERENCES rooms(id) ON DELETE CASCADE, | ||
| anon_name text NOT NULL, | ||
| role text NOT NULL DEFAULT 'debater' CHECK (role IN ('debater','host','judge')), | ||
| jwt_sub text, | ||
| ssh_fingerprint text, | ||
| created_at timestamptz NOT NULL DEFAULT now(), | ||
| UNIQUE (room_id, anon_name) | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_participants_room ON participants (room_id); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Participants looks better, but tighten name semantics.
If anon_name should be case‑insensitive unique per room, enforce it in the database.
Patch:
CREATE TABLE IF NOT EXISTS participants (
@@
- UNIQUE (room_id, anon_name)
+ UNIQUE (room_id, anon_name) -- keep if case-sensitive names are desired
);
-CREATE INDEX IF NOT EXISTS idx_participants_room ON participants (room_id);
+-- If you actually want case-insensitive uniqueness, replace the constraint with:
+-- DROP INDEX/CONSTRAINT as needed and then:
+-- CREATE UNIQUE INDEX IF NOT EXISTS uniq_participants_name_ci
+-- ON participants (room_id, lower(anon_name));
+-- This also serves room_id lookups; measure before keeping a separate room_id index.Also: decide whether jwt_sub/ssh_fingerprint need uniqueness (per room or global). If yes, add partial unique indexes with WHERE jwt_sub IS NOT NULL, etc.
List expected collation/uniqueness rules for anon_name. If it’s case-sensitive by design, say so in a COMMENT.
Also applies to: 40-41
🤖 Prompt for AI Agents
In db/schema.sql around lines 28 to 41 (and note 40-41), anon_name currently has
a UNIQUE(room_id, anon_name) but no case-insensitive enforcement; if anon_name
must be case‑insensitive unique per room, replace or supplement that constraint
with a unique index on (room_id, lower(anon_name)) to enforce CI uniqueness; if
instead anon_name is intentionally case‑sensitive, add a COMMENT on the column
stating that design decision and expected collation; additionally, if jwt_sub
and/or ssh_fingerprint must be unique (globally or per room), add appropriate
partial unique indexes (e.g., WHERE jwt_sub IS NOT NULL or per room via
(room_id, jwt_sub) with WHERE jwt_sub IS NOT NULL) to enforce those rules.
| -- participants role column has CHECK constraint enforcing allowed roles | ||
| SELECT ok( | ||
| EXISTS ( | ||
| SELECT 1 | ||
| FROM pg_constraint c | ||
| JOIN pg_class t ON t.oid = c.conrelid | ||
| JOIN pg_namespace n ON n.oid = t.relnamespace | ||
| WHERE n.nspname = 'public' | ||
| AND t.relname = 'participants' | ||
| AND c.contype = 'c' | ||
| AND pg_get_constraintdef(c.oid) ILIKE '%CHECK%role%' | ||
| ), | ||
| 'participants role check constraint exists' | ||
| ); |
There was a problem hiding this comment.
“role CHECK exists” is meaningless unless you prove allowed values. Test the set.
Assert that the CHECK def enumerates your allowed roles.
Add:
-- participants.role CHECK enumerates allowed roles
SELECT ok(
EXISTS (
SELECT 1
FROM pg_constraint c
JOIN pg_class t ON t.oid = c.conrelid
JOIN pg_namespace n ON n.oid = t.relnamespace
WHERE n.nspname = 'public'
AND t.relname = 'participants'
AND c.contype = 'c'
AND pg_get_constraintdef(c.oid) ILIKE '%CHECK%'
AND pg_get_constraintdef(c.oid) ILIKE '%role in (%debater%moderator%fact_checker%judge%viewer%)%'
),
'participants.role CHECK enumerates allowed roles'
);🤖 Prompt for AI Agents
In db/test/10_rooms_rounds.pgtap around lines 88–101 the test only asserts a
CHECK constraint exists on participants.role but does not verify which values
are allowed; add a new test that queries pg_constraint/pg_get_constraintdef and
asserts the CHECK definition enumerates the expected roles (debater, moderator,
fact_checker, judge, viewer) — e.g. add a SELECT ok(EXISTS(...)) that includes
c.contype='c' and pg_get_constraintdef(c.oid) ILIKE '%CHECK%' AND
pg_get_constraintdef(c.oid) ILIKE pattern(s) that confirm the role IN(...) list
contains each allowed role so the test fails if any role is missing or the list
differs.
| @@ -1,48 +1,176 @@ | |||
| BEGIN; | |||
| SELECT plan(8); | |||
| SELECT plan(27); | |||
There was a problem hiding this comment.
PLAN IS WRONG. FIX IT.
Line 2: You bolted on nine extra ok() calls plus the existing has_function(...) checks and that pass() at the end, yet you left the plan at 27. The suite now fires 29 assertions, so pgTAP will scream bloody murder. Bump the plan to 29 before CI slaps you again.
-SELECT plan(27);
+SELECT plan(29);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SELECT plan(27); | |
| -- db/test/30_rpcs.pgtap | |
| SELECT plan(29); |
🤖 Prompt for AI Agents
In db/test/30_rpcs.pgtap around line 2, the test plan is incorrect: you added
nine extra ok() assertions (in addition to the existing has_function(...) checks
and the final pass()), so the suite now runs 29 assertions but the plan is still
27; update the SELECT plan(...) call to SELECT plan(29); to match the actual
number of assertions so pgTAP will not fail the CI.
| export const ContinueVote = z.object({ | ||
| room_id: z.string().uuid(), | ||
| round_id: z.string().uuid(), | ||
| choice: z.enum(["continue","end"]) | ||
| choice: z.enum(['continue', 'end']) | ||
| }); |
There was a problem hiding this comment.
Schema mismatch: ContinueVote still requires room_id. You removed it from the RPC.
Update the wire schema to drop room_id and include client_nonce for idempotency.
-export const ContinueVote = z.object({
- room_id: z.string().uuid(),
- round_id: z.string().uuid(),
- choice: z.enum(['continue', 'end'])
-});
+export const ContinueVote = z.object({
+ round_id: z.string().uuid(),
+ choice: z.enum(['continue', 'end']),
+ client_nonce: z.string().min(1)
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const ContinueVote = z.object({ | |
| room_id: z.string().uuid(), | |
| round_id: z.string().uuid(), | |
| choice: z.enum(["continue","end"]) | |
| choice: z.enum(['continue', 'end']) | |
| }); | |
| export const ContinueVote = z.object({ | |
| round_id: z.string().uuid(), | |
| choice: z.enum(['continue', 'end']), | |
| client_nonce: z.string().min(1) | |
| }); |
🤖 Prompt for AI Agents
In docs/Architecture.md around lines 684 to 688, the ContinueVote Zod schema
still requires room_id but the RPC no longer sends it; update the wire schema by
removing room_id and adding a client_nonce field for idempotency (e.g.,
client_nonce: z.string().uuid() or z.string() if non-UUID nonces are used), and
ensure the exported ContinueVote object reflects this change so the schema
matches the RPC payload.
| - Votes table + RLS. | ||
| - RPC `vote_submit(room, round, voter, kind, ballot)`. | ||
| - RPC `vote_submit(round, voter, kind, ballot)`. | ||
| - Aggregates for continue pass/fail and final placements. | ||
| - Simple majority of cast ballots; soft quorum (≥3 votes) to avoid zombie rounds. | ||
| - Simple majority of cast ballots; soft quorum (≥3 votes) to avoid zombie rounds. | ||
|
|
There was a problem hiding this comment.
You removed room_id from vote_submit, but you also “forgot” the idempotency token.
Document the actual RPC: vote_submit(round_id, voter_id, kind, ballot, client_nonce). If you don’t require client_nonce, retries will double‑vote. Fix the docs.
🤖 Prompt for AI Agents
In docs/Features.md around lines 148 to 152, the RPC signature for vote_submit
is incorrect: it removed room_id and omitted the idempotency token. Update the
documented RPC to the accurate signature vote_submit(round_id, voter_id, kind,
ballot, client_nonce) (or include room_id if your protocol requires it) and add
a short note explaining client_nonce is required for idempotency to prevent
retry double-votes; ensure parameter names match server implementation and
adjust any surrounding text that references the old signature.
| - Simple majority of cast ballots; soft quorum (≥3 votes) to avoid zombie rounds. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stop lying about soft quorum. You don’t implement it anywhere.
Either mark it “planned” or implement the ≥3 votes rule in SQL before claiming it here. Right now round_open_next only checks yes > no.
Also applies to: 239-245
feat(db): add room_create rpc and participants table
1 participant
Summary
Changes
Tests
Next