-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Milestone 5 - Scoring & Reputation #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
docs(ci): DB integration workflow + README milestone focus
Signed-off-by: James Ross <james@flyingrobots.dev>
…outes, CLI commands, web summary, tests, docs
…s; keep manual + weekly schedule; add concurrency
…bmit ON CONFLICT to include client_nonce; summary reads via view; fix tests and web polling; map CLI HTTP errors to exit codes
…atisfy dependency
…via import/core-modules
… ~/Codex note practices
… CI; Next build validates imports
…use dorny/paths-filter to run only when web/** changes (or on push to main)
…web paths to fix concurrency + resolver issues
…e; prove sequential DB tests incl. verify run
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR adds verification, final-vote and scoring features across DB, server, CLI, and web: new tables/views/RPCs for verification, final votes, scores, reputation (Elo), RLS policies, CLI commands, web UI dialogs, audit logging, and extensive integration + pgTAP tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (bin/db8.js)
participant Server as Server RPC (server/rpc.js)
participant DB as Database (rpc/sql + schema)
participant Audit as Admin Audit Log
rect rgba(100,150,200,0.15)
Note over CLI,DB: Final Vote Submission
CLI->>Server: POST /rpc/vote.final {round_id, voter_id, approval, ranking, client_nonce}
Server->>DB: CALL vote_final_submit(...)
DB->>DB: Insert/upsert final_votes (client_nonce dedupe)
DB->>DB: Possibly update room.status = 'closed'
DB->>Audit: admin_audit_log_write(entity='final_vote', action='vote', actor=voter_id,...)
DB-->>Server: {id}
Server-->>CLI: {ok: true, id}
end
par Asynchronous
DB->>DB: reputation_update_round(round_id) -- compute Elo, update reputation/reputation_tag
DB->>Audit: admin_audit_log_write(entity='reputation', action='update', ...)
end
sequenceDiagram
autonumber
participant Web as Browser UI
participant Server as Server RPC
participant DB as Database
participant SSE as SSE / Event stream
rect rgba(150,200,150,0.12)
Note over Web,DB: Verification Submit Flow
Web->>Server: POST /rpc/verify.submit {round_id, reporter_id, submission_id, claim_id, verdict, rationale, client_nonce}
Server->>DB: CALL verify_submit(...)
DB->>DB: Upsert verification_verdicts (idempotent by keys)
DB->>Audit: admin_audit_log_write(entity='verification_verdicts', action='create'/'update', ...)
DB-->>Server: {id}
Server->>SSE: publish verdict event
Server-->>Web: {ok: true, id}
Web->>Server: GET /rpc/verify.summary?round_id=... (poll)
Server->>DB: SELECT verify_summary(...)
DB-->>Server: aggregated rows
Server-->>Web: summary JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 19.05% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'feat: Milestone 5 - Scoring & Reputation' is directly related to the main changeset, which implements scoring rubric, Elo reputation system, aggregation views, and related APIs. |
| Description check | ✅ Passed | The description substantively covers the key accomplishments: rubric scoring, Elo system, aggregation, API integration, and test coverage, all of which are present in the changeset. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
milestone/m5
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 help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/test/audit.integration.test.js (2)
21-49: CI is screaming: FK constraint violations because your seeding is silently failing.Pipeline failures reveal:
- Line 41:
submissions_round_id_fkeyviolation — the round doesn't exist whensubmission_upsertruns.- Line 69:
round not found: 33343334-0000-0000-0000-000000000002— confirming the round was never inserted.Root cause: Your
ON CONFLICT DO NOTHINGstrategy (lines 27, 32, 36) is a silent failure factory. If any constraint conflicts (perhaps from previous test runs or stale state), the inserts are skipped and you proceed blissfully unaware.The unforgivable sin: You never verify your seeds succeeded. You fire-and-forget
INSERTstatements, then act surprised when downstream queries fail.🔎 Proposed fix: Verify seeds or use RETURNING
it('room_create should be audit-logged (implied via watcher or manual call)', async () => { const roomId = '33343334-0000-0000-0000-000000000001'; const roundId = '33343334-0000-0000-0000-000000000002'; const participantId = '33343334-0000-0000-0000-000000000003'; // Seed data - await pool.query('insert into rooms(id, title) values ($1, $2) on conflict do nothing', [ + const roomRes = await pool.query( + `insert into rooms(id, title) values ($1, $2) + on conflict (id) do update set title = excluded.title + returning id`, + [roomId, 'Audit Room Unique'] + ); + expect(roomRes.rows.length).toBe(1); - roomId, - 'Audit Room Unique' - ]); - await pool.query( - 'insert into rounds(id, room_id, idx, phase) values ($1, $2, 0, $3) on conflict do nothing', - [roundId, roomId, 'submit'] + + const roundRes = await pool.query( + `insert into rounds(id, room_id, idx, phase) values ($1, $2, 0, $3) + on conflict (id) do update set phase = excluded.phase + returning id`, + [roundId, roomId, 'submit'] ); + expect(roundRes.rows.length).toBe(1); + - await pool.query( - 'insert into participants(id, room_id, anon_name) values ($1, $2, $3) on conflict do nothing', - [participantId, roomId, 'audit_anon_unique'] + const partRes = await pool.query( + `insert into participants(id, room_id, anon_name) values ($1, $2, $3) + on conflict (id) do update set anon_name = excluded.anon_name + returning id`, + [participantId, roomId, 'audit_anon_unique'] ); + expect(partRes.rows.length).toBe(1);
61-84: Test isolation nightmare: reusing stale IDs across tests.This test at line 61 reuses
roundIdandparticipantIdfrom the first test (lines 62-63), assuming they still exist. But:
- If the first test's seeding failed (as CI shows), this test inherits the failure.
- You update the round to
published(line 66), but if the round doesn't exist, this UPDATE affects 0 rows and you don't check.- The
vote_submitcall then fails with "round not found."Each test should be self-contained. Don't rely on side effects from previous tests.
🔎 Proposed fix: Self-contained seeding per test
it('vote_submit should be audit-logged', async () => { - const roundId = '33343334-0000-0000-0000-000000000002'; - const participantId = '33343334-0000-0000-0000-000000000003'; + const roomId = '33343334-0000-0000-0000-000000000010'; + const roundId = '33343334-0000-0000-0000-000000000011'; + const participantId = '33343334-0000-0000-0000-000000000012'; + + // Self-contained seeding + await pool.query( + `insert into rooms(id, title) values ($1, $2) + on conflict (id) do update set title = excluded.title`, + [roomId, 'Vote Test Room'] + ); + await pool.query( + `insert into rounds(id, room_id, idx, phase) values ($1, $2, 0, $3) + on conflict (id) do update set phase = excluded.phase`, + [roundId, roomId, 'published'] + ); + await pool.query( + `insert into participants(id, room_id, anon_name) values ($1, $2, $3) + on conflict (id) do update set anon_name = excluded.anon_name`, + [participantId, roomId, 'vote_test_anon'] + ); - // Set round to published - await pool.query('update rounds set phase = $1 where id = $2', ['published', roundId]);server/rpc.js (1)
974-984:author_anon_nameis referenced but never selected from the database.Line 978 maps
row.author_anon_nameinto the transcript, but the SQL query at lines 956-967 doesn't SELECTauthor_anon_name. You're selecting fromsubmissions_with_flags_view, which DOES haveauthor_anon_nameaccording todb/rpc.sqlline 436.Wait—let me re-examine. The view definition at line 436 in
db/rpc.sqldoes includeauthor_anon_name. But I don't see it in the SELECT list at lines 956-967:select id, author_id, content, canonical_sha256, submitted_at, flag_count, flag_details from submissions_with_flags_viewThe column exists in the view but you forgot to SELECT it. Result:
author_anon_namewill beundefinedfor all transcript entries.🔎 Proposed fix: Add author_anon_name to SELECT
db.query( `select id, author_id, + author_anon_name, content, canonical_sha256, submitted_at, flag_count, flag_details from submissions_with_flags_view where round_id = $1 order by submitted_at asc nulls last, id asc`, [roundRow.round_id] ),
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
bin/db8.jsdb/rls.sqldb/rpc.sqldb/schema.sqlserver/rpc.jsserver/schemas.jsserver/test/attribution.test.jsserver/test/audit.integration.test.jsserver/test/auth.ssh.test.jsserver/test/final_tally.test.jsserver/test/lifecycle.test.jsserver/test/scoring.test.jsserver/test/sse.db.events.test.jsserver/test/voting.final.test.jsweb/app/room/[roomId]/page.jsx
🧰 Additional context used
📓 Path-based instructions (7)
{web,server,bin}/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use only JavaScript across web, server, and CLI; no TypeScript
Files:
server/test/sse.db.events.test.jsserver/test/lifecycle.test.jsserver/test/auth.ssh.test.jsserver/schemas.jsserver/test/attribution.test.jsserver/test/final_tally.test.jsserver/test/voting.final.test.jsbin/db8.jsserver/test/scoring.test.jsserver/rpc.jsserver/test/audit.integration.test.js
{server,bin}/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
{server,bin}/**/*.js: Validate inputs with Zod at the edges (endpoints, CLI)
Canonical JSON must use JCS (RFC 8785) by default; legacy sorted-key option available via CANON_MODE=sorted
Files:
server/test/sse.db.events.test.jsserver/test/lifecycle.test.jsserver/test/auth.ssh.test.jsserver/schemas.jsserver/test/attribution.test.jsserver/test/final_tally.test.jsserver/test/voting.final.test.jsbin/db8.jsserver/test/scoring.test.jsserver/rpc.jsserver/test/audit.integration.test.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint + Prettier for code style (see eslint.config.js and .prettierrc)
Files:
server/test/sse.db.events.test.jsserver/test/lifecycle.test.jsserver/test/auth.ssh.test.jsserver/schemas.jsserver/test/attribution.test.jsserver/test/final_tally.test.jsweb/app/room/[roomId]/page.jsxserver/test/voting.final.test.jsbin/db8.jsserver/test/scoring.test.jsserver/rpc.jsserver/test/audit.integration.test.js
server/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
server/**/*.js: Use Zod schemas at the edges of Express endpoints for request validation
Server endpoints must include Zod validation, in-memory fallback patterns, and optional DATABASE_URL persistence
Signatures (Ed25519) must include strict author binding when participants.ssh_fingerprint is configured; return 400 with expected/got on mismatch
Files:
server/test/sse.db.events.test.jsserver/test/lifecycle.test.jsserver/test/auth.ssh.test.jsserver/schemas.jsserver/test/attribution.test.jsserver/test/final_tally.test.jsserver/test/voting.final.test.jsserver/test/scoring.test.jsserver/rpc.jsserver/test/audit.integration.test.js
db/**/*.sql
📄 CodeRabbit inference engine (AGENTS.md)
db/**/*.sql: Database schema migrations must be tracked and schema must maintain idempotency via ON CONFLICT for RPCs
Submission verification (verify_submit) must enforce judge/host roles and published/final round phases
Database RLS policies must restrict visibility by role and status; use SECURITY BARRIER on views when pre-publish leakage is a risk
Files:
db/schema.sqldb/rpc.sqldb/rls.sql
web/**/*.{jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Web/React components must use AbortController for async operations to prevent setState after unmount
Files:
web/app/room/[roomId]/page.jsx
bin/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
CLI commands must use process.execPath and isolate temporary files; validate with Zod at entry points
Files:
bin/db8.js
🧠 Learnings (7)
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to db/test/**/*.sql : pgTAP invariants must cover tables, uniques, views, and RPC contracts with coverage for happy path, idempotency reuse, boundary acceptance, and failure cases
Applied to files:
server/test/lifecycle.test.jsserver/test/attribution.test.jsserver/test/final_tally.test.jsserver/test/voting.final.test.jsserver/test/scoring.test.jsserver/test/audit.integration.test.js
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to server/**/*.js : Signatures (Ed25519) must include strict author binding when participants.ssh_fingerprint is configured; return 400 with expected/got on mismatch
Applied to files:
server/test/auth.ssh.test.jsserver/rpc.js
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to server/**/*.js : Server endpoints must include Zod validation, in-memory fallback patterns, and optional DATABASE_URL persistence
Applied to files:
server/test/auth.ssh.test.jsserver/schemas.js
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to server/**/*.js : Use Zod schemas at the edges of Express endpoints for request validation
Applied to files:
server/schemas.js
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to db/**/*.sql : Submission verification (verify_submit) must enforce judge/host roles and published/final round phases
Applied to files:
db/schema.sqldb/rpc.sqlserver/test/voting.final.test.jsserver/rpc.jsserver/test/audit.integration.test.jsdb/rls.sql
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to db/**/*.sql : Database RLS policies must restrict visibility by role and status; use SECURITY BARRIER on views when pre-publish leakage is a risk
Applied to files:
db/rls.sql
📚 Learning: 2025-12-23T09:54:30.371Z
Learnt from: CR
Repo: flyingrobots/db8 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T09:54:30.371Z
Learning: Applies to db/**/*.sql : Database schema migrations must be tracked and schema must maintain idempotency via ON CONFLICT for RPCs
Applied to files:
db/rls.sql
🧬 Code graph analysis (8)
server/test/sse.db.events.test.js (1)
server/test/sse.db.journal.test.js (3)
server(18-18)port(19-19)roomId(20-20)
server/test/auth.ssh.test.js (2)
server/test/audit.integration.test.js (2)
dbUrl(7-10)pool(6-6)server/rpc.js (1)
__setDbPool(43-45)
server/test/final_tally.test.js (2)
server/test/lifecycle.test.js (2)
pool(6-6)dbUrl(7-10)server/test/attribution.test.js (2)
pool(6-6)dbUrl(7-10)
web/app/room/[roomId]/page.jsx (5)
web/app/journal/[roomId]/page.jsx (1)
roomId(35-35)web/app/spectate/[roomId]/page.jsx (2)
roomId(9-9)state(10-10)web/components/ui/button.jsx (1)
Button(19-22)web/components/ui/badge.jsx (1)
Badge(3-15)web/components/ui/card.jsx (2)
Card(3-5)CardContent(11-13)
server/test/voting.final.test.js (2)
server/test/lifecycle.test.js (2)
pool(6-6)dbUrl(7-10)server/rpc.js (1)
__setDbPool(43-45)
server/test/scoring.test.js (2)
scripts/prepare-db.js (1)
pg(7-7)server/rpc.js (7)
__setDbPool(43-45)roomId(254-254)roomId(933-933)roomId(1106-1106)roomId(1357-1357)roomId(1393-1393)roundId(847-847)
server/rpc.js (1)
server/schemas.js (8)
FinalVote(132-138)FinalVote(132-138)ScoreSubmit(140-150)ScoreSubmit(140-150)ScoreGet(152-154)ScoreGet(152-154)ReputationGet(156-159)ReputationGet(156-159)
server/test/audit.integration.test.js (1)
server/rpc.js (1)
__setDbPool(43-45)
🪛 GitHub Actions: ci
server/test/audit.integration.test.js
[error] 41-41: insert or update on table "submissions" violates foreign key constraint "submissions_round_id_fkey" during audit-logged room_create test.
[error] 69-69: round not found: 33343334-0000-0000-0000-000000000002 during audit-logged vote_submit test.
🔇 Additional comments (24)
db/rls.sql (3)
131-145: LGTM - Policy logic is consistent.The read policy correctly mirrors the
verification_verdictspattern: visible to the voter immediately, visible to all afterpublished/final. Write denial viaSECURITY DEFINERRPCs is the correct approach.
153-167: Correct restriction to 'final' phase only.Unlike
final_voteswhich reveals afterpublished, scores are appropriately restricted tofinalphase only. This prevents score leakage during the voting period. The judge-sees-own pattern is consistent.
175-193: LGTM - Public read for reputation data is appropriate.Reputation and tag-based Elo are public metrics. Unrestricted read access with write denial is the correct posture. Updates flow through
reputation_update_roundvia SECURITY DEFINER.server/test/sse.db.events.test.js (2)
29-35: LGTM - Room creation via RPC is the correct pattern.Using
room_createRPC with a unique nonce ensures idempotency and follows the established pattern. ThebeforeAllwill throw on failure, which is acceptable for test setup.
51-55: LGTM - Defensive teardown.The
if (server)guard prevents crashes whenbeforeAllfails before server instantiation. Clean.server/test/auth.ssh.test.js (1)
127-169: LGTM - Validates participant binding enforcement with DB.This test correctly verifies that
/auth/verifyreturns 404 when the participant doesn't exist in the room. The try/finally ensures__setDbPool(null)andpool.end()run even on failure. Per coding guidelines, author binding must be enforced when fingerprint is configured.server/test/voting.final.test.js (2)
58-63: LGTM - Audit log verification is valuable.Testing that
admin_audit_logis populated withaction='vote'for final votes ensures the audit trail is functional. Good coverage.
31-34: Phase validation missing invote_final_submitRPC function.The
vote_final_submitfunction (db/rpc.sql:739+) verifies participant status but performs zero phase validation. It accepts final votes duringsubmitphase when they should only be accepted duringfinalphase. The test incorrectly creates a round withphase='submit'and expects vote submission to succeed—this should fail.Add phase validation to
vote_final_submitto enforce the round is infinalphase before accepting votes, matching the phase-aware principle applied to submission verification operations.⛔ Skipped due to learnings
Learnt from: CR Repo: flyingrobots/db8 PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-23T09:54:30.371Z Learning: Applies to db/**/*.sql : Submission verification (verify_submit) must enforce judge/host roles and published/final round phasesserver/test/final_tally.test.js (1)
22-48: No idempotency test forvote_final_submitwith duplicate nonce.Per coding guidelines, pgTAP invariants should cover idempotency. This test only exercises the happy path. A duplicate
client_nonceshould be silently ignored (idempotent reuse). Consider adding:// Idempotency: resubmit with same nonce should not double-count await pool.query("select vote_final_submit($1, $2, true, '[]', 'n1')", [roundId, p1]); const res2 = await pool.query('select * from view_final_tally where round_id = $1', [roundId]); expect(res2.rows[0].approves).toBe('2'); // Still 2, not 3⛔ Skipped due to learnings
Learnt from: CR Repo: flyingrobots/db8 PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-23T09:54:30.371Z Learning: Applies to db/test/**/*.sql : pgTAP invariants must cover tables, uniques, views, and RPC contracts with coverage for happy path, idempotency reuse, boundary acceptance, and failure casesLearnt from: CR Repo: flyingrobots/db8 PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-23T09:54:30.371Z Learning: Applies to db/**/*.sql : Submission verification (verify_submit) must enforce judge/host roles and published/final round phasesbin/db8.js (1)
98-99: Commands registered correctly.The new
vote:continueandvote:finalcommands are properly added to the allowed set.server/rpc.js (4)
15-19: Schema imports are correct and complete.All new schemas (
FinalVote,ScoreSubmit,ScoreGet,ReputationGet) are properly imported.
43-45: Test injection helper is clean.
__setDbPoolallows tests to inject a mock pool. The double-underscore prefix correctly signals internal/test-only usage.
150-159: Auth verification now properly scopes participant lookup to room.The query at lines 152-155 correctly validates that the participant exists in the specified room, returning 404 if not found. This closes a potential authorization gap where a participant from room A could authenticate for room B.
1248-1252: UNLISTEN for db8_final_vote added correctly.Proper cleanup of the new LISTEN channel on connection close.
server/schemas.js (1)
152-159: Read schemas look correct.
ScoreGetandReputationGetare simple query schemas with appropriate UUID validation.web/app/room/[roomId]/page.jsx (2)
53-54: New modal state hooks are correct.Clean boolean state for controlling vote modal visibility.
655-657: Fallback fromauthor_anon_nametoauthor_idis correct.Good defensive coding—displays the anon name when available, falls back to UUID otherwise.
db/schema.sql (3)
7-11: Session participant helper is correctly implemented.The use of
current_setting(..., true)withNULLIFgracefully handles missing settings by returning NULL instead of throwing. This enables RLS policies to safely reference the current participant.
17-18: Room status and config columns are well-designed.The
CHECKconstraint onstatuslimits values to a known set, andconfigas JSONB with default'{}'provides flexibility for future expansion.
135-148: Reputation tables are correctly structured.
reputationusesparticipant_idas PK — correct for global Eloreputation_taguses composite PK(participant_id, tag)— correct for per-topic Elo- Default Elo of 1200.0 follows standard conventions
db/rpc.sql (4)
345-358: Room closure on final transition is a good addition.When a round transitions to 'final', the room status is now set to 'closed' with proper audit logging. This provides clear lifecycle semantics.
383-399: Attribution masking logic is security-conscious.The CASE expression at lines 383-391 correctly:
- Hides
author_idduring submit phase whenattribution_mode='masked'and viewer isn't the author- Returns the ID (for anon_name lookup) after submit phase
- Returns full
author_idwhen not maskedCombined with
security_barrier = true, this prevents qual pushdown attacks.
842-857: Score aggregation view is correct.The weighted composite formula at lines 851-852:
E*0.35 + R*0.30 + C*0.20 + V*0.05 + Y*0.10Weights sum to 1.0 ✓. Evidence and Reasoning are weighted highest, which aligns with typical debate scoring priorities.
security_barrier = trueis correctly applied.
859-868: Final tally view is correct and simple.FILTER clauses for counting approves/rejects are the idiomatic PostgreSQL approach.
| case 'vote:final': { | ||
| const approval = args.approve !== undefined ? Boolean(args.approve !== 'false') : true; | ||
| const ranking = args.rank | ||
| ? String(args.rank) | ||
| .split(',') | ||
| .map((s) => s.trim()) | ||
| : []; | ||
| if (!room || !participant || !jwt) { | ||
| printerr('Missing room/participant credentials. Run db8 login or set env.'); | ||
| return EXIT.AUTH; | ||
| } | ||
| const cn = String(args.nonce || randomNonce()); | ||
| try { | ||
| const res = await fetch(`${apiUrl.replace(/\/$/, '')}/rpc/vote.final`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| authorization: `Bearer ${jwt}` | ||
| }, | ||
| body: JSON.stringify({ | ||
| round_id: '00000000-0000-0000-0000-000000000002', // loose stub | ||
| voter_id: participant, | ||
| approval, | ||
| ranking, | ||
| client_nonce: cn | ||
| }) | ||
| }); | ||
| const body = await res.json().catch(() => ({})); | ||
| if (!res.ok) { | ||
| printerr(body?.error || `Server error ${res.status}`); | ||
| return EXIT.NETWORK; | ||
| } | ||
| if (args.json) print(JSON.stringify(body)); | ||
| else print('ok'); | ||
| return EXIT.OK; | ||
| } catch (e) { | ||
| printerr(e?.message || String(e)); | ||
| return EXIT.NETWORK; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nightmare: hardcoded round_id and no validation for vote:final.
-
Line 858: Another hardcoded
round_id. This code will never work correctly in production. -
Lines 839-844: The
approvalderivation is clever but dangerously permissive—args.approve !== 'false'means any truthy string becomestrue. This is begging for user error. Use Zod with explicit boolean coercion. -
Lines 840-844: The
rankingparsing has no UUID validation. Users can pass garbage like--rank "foo,bar,baz"and it will be sent to the server unvalidated, only to fail there. Fail early at the CLI edge. -
Missing
--roundrequirement: Unlike other commands, there's no enforcement that--roundis required.
🔎 Proposed fix with proper validation
case 'vote:final': {
- const approval = args.approve !== undefined ? Boolean(args.approve !== 'false') : true;
- const ranking = args.rank
- ? String(args.rank)
- .split(',')
- .map((s) => s.trim())
- : [];
+ const FinalVoteArgs = z.object({
+ round: z.string().uuid(),
+ approve: z.preprocess(
+ (v) => v === undefined ? true : v !== 'false' && v !== '0',
+ z.boolean()
+ ),
+ rank: z.preprocess(
+ (v) => v ? String(v).split(',').map(s => s.trim()).filter(Boolean) : [],
+ z.array(z.string().uuid())
+ ).optional()
+ });
+ const parsed = FinalVoteArgs.safeParse({
+ round: args.round,
+ approve: args.approve,
+ rank: args.rank
+ });
+ if (!parsed.success) {
+ printerr('vote final requires --round <uuid>; --rank must be comma-separated UUIDs');
+ return EXIT.VALIDATION;
+ }
+ const { round: roundId, approve: approval, rank: ranking } = parsed.data;
if (!room || !participant || !jwt) {
printerr('Missing room/participant credentials. Run db8 login or set env.');
return EXIT.AUTH;
}
const cn = String(args.nonce || randomNonce());
try {
const res = await fetch(`${apiUrl.replace(/\/$/, '')}/rpc/vote.final`, {
method: 'POST',
headers: {
'content-type': 'application/json',
authorization: `Bearer ${jwt}`
},
body: JSON.stringify({
- round_id: '00000000-0000-0000-0000-000000000002', // loose stub
+ round_id: roundId,
voter_id: participant,
approval,
- ranking,
+ ranking: ranking || [],
client_nonce: cn
})
});📝 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.
| case 'vote:final': { | |
| const approval = args.approve !== undefined ? Boolean(args.approve !== 'false') : true; | |
| const ranking = args.rank | |
| ? String(args.rank) | |
| .split(',') | |
| .map((s) => s.trim()) | |
| : []; | |
| if (!room || !participant || !jwt) { | |
| printerr('Missing room/participant credentials. Run db8 login or set env.'); | |
| return EXIT.AUTH; | |
| } | |
| const cn = String(args.nonce || randomNonce()); | |
| try { | |
| const res = await fetch(`${apiUrl.replace(/\/$/, '')}/rpc/vote.final`, { | |
| method: 'POST', | |
| headers: { | |
| 'content-type': 'application/json', | |
| authorization: `Bearer ${jwt}` | |
| }, | |
| body: JSON.stringify({ | |
| round_id: '00000000-0000-0000-0000-000000000002', // loose stub | |
| voter_id: participant, | |
| approval, | |
| ranking, | |
| client_nonce: cn | |
| }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) { | |
| printerr(body?.error || `Server error ${res.status}`); | |
| return EXIT.NETWORK; | |
| } | |
| if (args.json) print(JSON.stringify(body)); | |
| else print('ok'); | |
| return EXIT.OK; | |
| } catch (e) { | |
| printerr(e?.message || String(e)); | |
| return EXIT.NETWORK; | |
| } | |
| } | |
| case 'vote:final': { | |
| const FinalVoteArgs = z.object({ | |
| round: z.string().uuid(), | |
| approve: z.preprocess( | |
| (v) => v === undefined ? true : v !== 'false' && v !== '0', | |
| z.boolean() | |
| ), | |
| rank: z.preprocess( | |
| (v) => v ? String(v).split(',').map(s => s.trim()).filter(Boolean) : [], | |
| z.array(z.string().uuid()) | |
| ).optional() | |
| }); | |
| const parsed = FinalVoteArgs.safeParse({ | |
| round: args.round, | |
| approve: args.approve, | |
| rank: args.rank | |
| }); | |
| if (!parsed.success) { | |
| printerr('vote final requires --round <uuid>; --rank must be comma-separated UUIDs'); | |
| return EXIT.VALIDATION; | |
| } | |
| const { round: roundId, approve: approval, rank: ranking } = parsed.data; | |
| if (!room || !participant || !jwt) { | |
| printerr('Missing room/participant credentials. Run db8 login or set env.'); | |
| return EXIT.AUTH; | |
| } | |
| const cn = String(args.nonce || randomNonce()); | |
| try { | |
| const res = await fetch(`${apiUrl.replace(/\/$/, '')}/rpc/vote.final`, { | |
| method: 'POST', | |
| headers: { | |
| 'content-type': 'application/json', | |
| authorization: `Bearer ${jwt}` | |
| }, | |
| body: JSON.stringify({ | |
| round_id: roundId, | |
| voter_id: participant, | |
| approval, | |
| ranking: ranking || [], | |
| client_nonce: cn | |
| }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) { | |
| printerr(body?.error || `Server error ${res.status}`); | |
| return EXIT.NETWORK; | |
| } | |
| if (args.json) print(JSON.stringify(body)); | |
| else print('ok'); | |
| return EXIT.OK; | |
| } catch (e) { | |
| printerr(e?.message || String(e)); | |
| return EXIT.NETWORK; | |
| } | |
| } |
🤖 Prompt for AI Agents
In bin/db8.js around lines 838 to 877, replace the hardcoded round_id and add
strict CLI-side validation: require a --round argument and validate it as a UUID
(use Zod z.string().uuid()) instead of the fixed '0000...' stub; change approval
parsing to use Zod coercion (z.coerce.boolean()) so only valid boolean inputs
are accepted and ambiguous strings are rejected; parse ranking by splitting and
trimming, then validate each element as a UUID (z.string().uuid()) and return an
error/EXIT.INPUT if any rank entry is invalid; ensure validated values
(round_id, voter_id/participant, approval boolean, and ranking array) are used
in the POST body and that validation failures short-circuit with a clear error
and non-OK exit code.
| INSERT INTO final_votes (round_id, voter_id, approval, ranking, client_nonce) | ||
| VALUES (p_round_id, p_voter_id, p_approval, COALESCE(p_ranking, '[]'::jsonb), COALESCE(p_client_nonce, gen_random_uuid()::text)) | ||
| ON CONFLICT (round_id, voter_id, client_nonce) | ||
| DO UPDATE SET approval = EXCLUDED.approval, ranking = EXCLUDED.ranking | ||
| RETURNING id INTO v_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ON CONFLICT uses the problematic unique constraint.
Line 770: ON CONFLICT (round_id, voter_id, client_nonce)
This is consistent with the schema, but as noted in the schema review, this allows a voter to submit multiple conflicting votes by using different nonces. The conflict resolution only triggers for the EXACT same (voter + nonce) tuple.
This needs to align with whatever decision is made on the schema constraint.
See the db/schema.sql review comment for the proposed fix.
🤖 Prompt for AI Agents
In db/rpc.sql around lines 768–772, the ON CONFLICT target currently includes
client_nonce (ON CONFLICT (round_id, voter_id, client_nonce)), which allows the
same voter to create multiple conflicting votes by changing the nonce; update
the statement to use the conflict key that enforces one vote per voter per round
(e.g., ON CONFLICT (round_id, voter_id)) so upserts deduplicate by voter+round,
and adjust the DO UPDATE clause as needed to persist the desired
client_nonce/fields (for example include client_nonce = EXCLUDED.client_nonce if
you want to record the latest nonce); ensure this matches the agreed schema
unique constraint.
| PERFORM admin_audit_log_write( | ||
| 'update', | ||
| 'submission', -- conceptually scoring a submission | ||
| p_participant_id, | ||
| p_judge_id, | ||
| NULL, | ||
| jsonb_build_object('client_nonce', p_client_nonce), | ||
| jsonb_build_object('e', p_e, 'r', p_r, 'c', p_c, 'v', p_v, 'y', p_y) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Audit log uses 'submission' entity_type for scores, which is semantically incorrect.
Line 830: 'submission' as entity_type, but this is a SCORE, not a submission.
Line 831: p_participant_id as entity_id, but scores have their own id (returned in v_id).
The audit log CHECK constraint at db/schema.sql line 211 allows: 'room','round','submission','vote','participant','flag','system'. There's no 'score' option.
Options:
- Add
'score'to the allowed entity_types - Use
'vote'as entity_type (scores are judge votes in a sense) - Use
'system'(awkward)
🔎 Proposed fix: Add 'score' entity type or use proper semantics
Option A - Add 'score' to schema CHECK constraint:
-- In db/schema.sql line 211:
CHECK (entity_type IN (
'room','round','submission','vote','participant','flag','system','score'
))Then in db/rpc.sql:
PERFORM admin_audit_log_write(
- 'update',
- 'submission',
- p_participant_id,
+ 'create',
+ 'score',
+ v_id,
p_judge_id,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In db/rpc.sql around lines 828–836, the audit call currently logs the
entity_type as 'submission' and uses p_participant_id as the entity_id, but this
should record the SCORE entity and its id (v_id); update the PERFORM
admin_audit_log_write call to use entity_type 'score' and entity_id v_id, and
simultaneously add 'score' to the audit log CHECK constraint in db/schema.sql
(around line 211) so the new entity_type is allowed.
|
|
||
| describe('Final Voting (M4)', () => { | ||
| let pool; | ||
| const dbUrl = process.env.DATABASE_URL || 'postgresql://postgres:test@localhost:54329/db8_test'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing DB8_TEST_DATABASE_URL in fallback chain - INCONSISTENT.
Every other test file in this PR uses:
const dbUrl = process.env.DB8_TEST_DATABASE_URL || process.env.DATABASE_URL || 'fallback';This file skips DB8_TEST_DATABASE_URL:
const dbUrl = process.env.DATABASE_URL || 'fallback';This will cause CI failures if DB8_TEST_DATABASE_URL is set but DATABASE_URL is not.
🔎 Proposed fix
- const dbUrl = process.env.DATABASE_URL || 'postgresql://postgres:test@localhost:54329/db8_test';
+ const dbUrl =
+ process.env.DB8_TEST_DATABASE_URL ||
+ process.env.DATABASE_URL ||
+ 'postgresql://postgres:test@localhost:54329/db8_test';🤖 Prompt for AI Agents
In server/test/voting.final.test.js around line 8, the dbUrl fallback chain
omits process.env.DB8_TEST_DATABASE_URL which other tests use; update the
assignment to check process.env.DB8_TEST_DATABASE_URL first, then
process.env.DATABASE_URL, then the existing hardcoded fallback so CI uses the
per-suite DB8_TEST_DATABASE_URL when present.
| afterAll(async () => { | ||
| await pool.end(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Missing __setDbPool(null) - fourth instance.
At this point, I'm questioning whether there's a systemic misunderstanding of the cleanup pattern. Fix all instances.
🔎 Proposed fix
afterAll(async () => {
+ __setDbPool(null);
await pool.end();
});📝 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.
| afterAll(async () => { | |
| await pool.end(); | |
| }); | |
| afterAll(async () => { | |
| __setDbPool(null); | |
| await pool.end(); | |
| }); |
🤖 Prompt for AI Agents
In server/test/voting.final.test.js around lines 18-20, the test teardown calls
await pool.end() but omits calling __setDbPool(null); add a call to
__setDbPool(null) after the pool is closed (e.g., immediately after await
pool.end()) to fully clear the global DB pool reference; also audit and update
the other three test teardown locations across the codebase to follow the same
pattern (close pool, then call __setDbPool(null)).
| async function onContinueVote(choice) { | ||
| setActionBusy(true); | ||
| try { | ||
| const r = await fetch(`${apiBase()}/rpc/vote.continue`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| ...(jwt ? { authorization: `Bearer ${jwt}` } : {}) | ||
| }, | ||
| body: JSON.stringify({ | ||
| room_id: roomId, | ||
| round_id: state.round.round_id, | ||
| voter_id: participant, | ||
| choice, | ||
| client_nonce: window.crypto.randomUUID() | ||
| }) | ||
| }); | ||
| if (r.ok) setShowContinueVote(false); | ||
| else window.alert('Vote failed'); | ||
| } catch (err) { | ||
| window.alert(String(err)); | ||
| } finally { | ||
| setActionBusy(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing AbortController on async fetch violates coding guidelines.
Per coding guidelines: "Web/React components must use AbortController for async operations to prevent setState after unmount."
If the user navigates away while the vote is in flight, setShowContinueVote(false) and setActionBusy(false) will fire on an unmounted component, causing React warnings and potential memory leaks.
Also, line 70: state.round.round_id access is unsafe. If state is null/undefined (before snapshot loads) or state.round is undefined, this throws. The button is only shown when state?.round?.phase === 'published', but there's a race condition if state updates mid-click.
🔎 Proposed fix: Add AbortController and null guard
async function onContinueVote(choice) {
+ const roundId = state?.round?.round_id;
+ if (!roundId) {
+ window.alert('Round not loaded yet');
+ return;
+ }
+ const controller = new AbortController();
setActionBusy(true);
try {
const r = await fetch(`${apiBase()}/rpc/vote.continue`, {
method: 'POST',
headers: {
'content-type': 'application/json',
...(jwt ? { authorization: `Bearer ${jwt}` } : {})
},
body: JSON.stringify({
room_id: roomId,
- round_id: state.round.round_id,
+ round_id: roundId,
voter_id: participant,
choice,
client_nonce: window.crypto.randomUUID()
- })
+ }),
+ signal: controller.signal
});
- if (r.ok) setShowContinueVote(false);
- else window.alert('Vote failed');
+ if (!controller.signal.aborted) {
+ if (r.ok) setShowContinueVote(false);
+ else window.alert('Vote failed');
+ }
} catch (err) {
- window.alert(String(err));
+ if (err.name !== 'AbortError') window.alert(String(err));
} finally {
- setActionBusy(false);
+ if (!controller.signal.aborted) setActionBusy(false);
}
}As per coding guidelines, AbortController is required for async operations in React components.
📝 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.
| async function onContinueVote(choice) { | |
| setActionBusy(true); | |
| try { | |
| const r = await fetch(`${apiBase()}/rpc/vote.continue`, { | |
| method: 'POST', | |
| headers: { | |
| 'content-type': 'application/json', | |
| ...(jwt ? { authorization: `Bearer ${jwt}` } : {}) | |
| }, | |
| body: JSON.stringify({ | |
| room_id: roomId, | |
| round_id: state.round.round_id, | |
| voter_id: participant, | |
| choice, | |
| client_nonce: window.crypto.randomUUID() | |
| }) | |
| }); | |
| if (r.ok) setShowContinueVote(false); | |
| else window.alert('Vote failed'); | |
| } catch (err) { | |
| window.alert(String(err)); | |
| } finally { | |
| setActionBusy(false); | |
| } | |
| } | |
| async function onContinueVote(choice) { | |
| const roundId = state?.round?.round_id; | |
| if (!roundId) { | |
| window.alert('Round not loaded yet'); | |
| return; | |
| } | |
| const controller = new AbortController(); | |
| setActionBusy(true); | |
| try { | |
| const r = await fetch(`${apiBase()}/rpc/vote.continue`, { | |
| method: 'POST', | |
| headers: { | |
| 'content-type': 'application/json', | |
| ...(jwt ? { authorization: `Bearer ${jwt}` } : {}) | |
| }, | |
| body: JSON.stringify({ | |
| room_id: roomId, | |
| round_id: roundId, | |
| voter_id: participant, | |
| choice, | |
| client_nonce: window.crypto.randomUUID() | |
| }), | |
| signal: controller.signal | |
| }); | |
| if (!controller.signal.aborted) { | |
| if (r.ok) setShowContinueVote(false); | |
| else window.alert('Vote failed'); | |
| } | |
| } catch (err) { | |
| if (err.name !== 'AbortError') window.alert(String(err)); | |
| } finally { | |
| if (!controller.signal.aborted) setActionBusy(false); | |
| } | |
| } |
| async function onFinalVote(approval, ranking = []) { | ||
| setActionBusy(true); | ||
| try { | ||
| const r = await fetch(`${apiBase()}/rpc/vote.final`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| ...(jwt ? { authorization: `Bearer ${jwt}` } : {}) | ||
| }, | ||
| body: JSON.stringify({ | ||
| round_id: state.round.round_id, | ||
| voter_id: participant, | ||
| approval, | ||
| ranking, | ||
| client_nonce: window.crypto.randomUUID() | ||
| }) | ||
| }); | ||
| if (r.ok) setShowFinalVote(false); | ||
| else window.alert('Final vote failed'); | ||
| } catch (err) { | ||
| window.alert(String(err)); | ||
| } finally { | ||
| setActionBusy(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same AbortController issue for onFinalVote.
Identical problems as onContinueVote:
- No AbortController
- Unsafe access to
state.round.round_id(line 95)
Apply the same fix pattern.
🤖 Prompt for AI Agents
In web/app/room/[roomId]/page.jsx around lines 85 to 109, onFinalVote lacks an
AbortController and unsafely accesses state.round.round_id; create an
AbortController before the fetch, pass controller.signal into fetch, and handle
AbortError in the catch (silently or no alert); use optional chaining
(state.round?.round_id) and early-return or show an error if round_id is missing
before calling fetch; ensure setActionBusy(true) is set before starting and
setActionBusy(false) in finally, and abort the controller on component cleanup
(same pattern used to fix onContinueVote).
| {showContinueVote && ( | ||
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center p-4 z-50"> | ||
| <Card className="w-full max-w-md"> | ||
| <CardContent className="p-6 space-y-4 text-center"> | ||
| <h3 className="text-lg font-semibold">Round Complete</h3> | ||
| <p>Should the debate continue to the next round?</p> | ||
| <div className="flex justify-center gap-4 pt-2"> | ||
| <Button | ||
| variant="secondary" | ||
| onClick={() => onContinueVote('end')} | ||
| disabled={actionBusy} | ||
| > | ||
| End Debate | ||
| </Button> | ||
| <Button onClick={() => onContinueVote('continue')} disabled={actionBusy}> | ||
| Continue | ||
| </Button> | ||
| </div> | ||
| <Button variant="ghost" className="w-full" onClick={() => setShowContinueVote(false)}> | ||
| Cancel | ||
| </Button> | ||
| </CardContent> | ||
| </Card> | ||
| </div> | ||
| )} | ||
|
|
||
| {showFinalVote && ( | ||
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center p-4 z-50"> | ||
| <Card className="w-full max-w-md"> | ||
| <CardContent className="p-6 space-y-4"> | ||
| <h3 className="text-lg font-semibold">Final Approval</h3> | ||
| <p>Do you approve the results/conclusions of this debate?</p> | ||
| <div className="flex justify-center gap-4 pt-2"> | ||
| <Button | ||
| variant="destructive" | ||
| onClick={() => onFinalVote(false)} | ||
| disabled={actionBusy} | ||
| > | ||
| Reject | ||
| </Button> | ||
| <Button | ||
| className="bg-green-600 hover:bg-green-700" | ||
| onClick={() => onFinalVote(true)} | ||
| disabled={actionBusy} | ||
| > | ||
| Approve | ||
| </Button> | ||
| </div> | ||
| <Button variant="ghost" className="w-full" onClick={() => setShowFinalVote(false)}> | ||
| Cancel | ||
| </Button> | ||
| </CardContent> | ||
| </Card> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modal accessibility: no keyboard trap or focus management.
Both vote modals (lines 840-864, 866-894) lack:
- Focus trap: Users can Tab out of the modal into background content
- Escape key handling: No way to dismiss via keyboard
- Initial focus: Focus doesn't move to the modal when opened
- aria-modal: Missing ARIA attributes
This is a minor accessibility issue for a demo, but for production would be a blocker.
🔎 Proposed fix: Add keyboard handling
+ {showContinueVote && (
+ <div
+ className="fixed inset-0 bg-black/50 flex items-center justify-center p-4 z-50"
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="continue-vote-title"
+ onKeyDown={(e) => e.key === 'Escape' && setShowContinueVote(false)}
+ >
<Card className="w-full max-w-md">
<CardContent className="p-6 space-y-4 text-center">
- <h3 className="text-lg font-semibold">Round Complete</h3>
+ <h3 id="continue-vote-title" className="text-lg font-semibold">Round Complete</h3>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/app/room/[roomId]/page.jsx around lines 840-894, both vote modal blocks
lack proper keyboard and assistive-focus management; implement a focus trap and
initial focus when the modal opens, add Escape-key handling to close the modal,
set appropriate ARIA attributes (role="dialog", aria-modal="true" and
aria-labelledby pointing to the modal title), and restore focus to the
previously focused element when the modal closes; ensure Tab/Shift+Tab loop
inside the modal (or use a small utility/hook or a library like
focus-trap-react), and mark background content inert or aria-hidden while the
modal is open so keyboard and screen-reader users cannot tab into or access
background controls.
40bdad9 to
7e6bb13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 88
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
db/test/20_submissions_votes.pgtap (1)
1-80: Insufficient test coverage—expand to meet pgTAP best practices.This test suite only covers happy-path table and unique-constraint existence checks. Based on learnings, pgTAP tests must include:
- ✅ Happy path: Basic insertion (covered)
- ✅ Tables & uniques: Existence checks (covered)
- ❌ Idempotency reuse: Attempting duplicate inserts to verify unique constraints actually work
- ❌ Boundary acceptance: Edge cases (NULL handling, max lengths, invalid FKs)
- ❌ Failure cases: Constraint violations, invalid enum values, RLS denials
The test doesn't actually verify idempotency by attempting a duplicate insert with the same
(round_id, author_id, client_nonce). It also doesn't test votes insertion at all despite checking the votes table exists.🔧 Recommended test additions
Add these test cases to achieve comprehensive coverage:
-- Test actual idempotency: duplicate insert should be blocked by unique constraint SELECT throws_ok( $$ INSERT INTO submissions (round_id, author_id, content, claims, citations, canonical_sha256, client_nonce) VALUES ('00000000-0000-0000-0000-0000000000aa'::uuid, '00000000-0000-0000-0000-0000000000bb'::uuid, 'duplicate', '[]'::jsonb, '[]'::jsonb, 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef', 'nonce-rt-1') $$, '23505', -- unique_violation 'duplicate submission with same (round_id, author_id, client_nonce) rejected' ); -- Test FK constraint: invalid round_id should fail SELECT throws_ok( $$ INSERT INTO submissions (round_id, author_id, content, claims, citations, canonical_sha256, client_nonce) VALUES ('99999999-9999-9999-9999-999999999999'::uuid, '00000000-0000-0000-0000-0000000000bb'::uuid, 'orphan', '[]'::jsonb, '[]'::jsonb, 'badc0ffebadc0ffebadc0ffebadc0ffebadc0ffebadc0ffebadc0ffebadc0ffe', 'nonce-orphan') $$, '23503', -- foreign_key_violation 'submission with invalid round_id rejected' ); -- Test votes insertion (table exists but never tested) SELECT lives_ok( $$ INSERT INTO votes (round_id, voter_id, kind, submission_id, value, client_nonce) VALUES ('00000000-0000-0000-0000-0000000000aa'::uuid, '00000000-0000-0000-0000-0000000000bb'::uuid, 'some_kind', '00000000-0000-0000-0000-0000000000cc'::uuid, 5, 'vote-nonce-1') $$, 'basic vote insertion succeeds' );Update the plan count accordingly:
SELECT plan(9);Based on learnings, ...
docs/CLI-Quickstart.md (1)
2-2: Update the stalelastUpdateddate.The
lastUpdatedfield shows2025-10-02, yet this PR was created on2025-12-23and introduces new content (Room creation section). The date is nearly 3 months stale and violates the freshness expectation for documentation frontmatter.🔎 Proposed fix
--- -lastUpdated: 2025-10-02 +lastUpdated: 2025-12-23 ---docs/CLI.md (1)
2-2: Update the stalelastUpdateddate.The
lastUpdatedfield shows2025-10-05, yet this PR was created on2025-12-23and introduces new content (db8 room create). The date is over 2 months stale and violates the freshness expectation for documentation frontmatter.🔎 Proposed fix
--- -lastUpdated: 2025-10-05 +lastUpdated: 2025-12-23 ---docs/Formal-Design-Spec.md (1)
2-2: Fix frontmatter to comply with spec document requirements.The frontmatter is structurally incomplete. Per coding guidelines, spec documents require
tags: [spec]andmilestone:fields. Currently, the frontmatter only containslastUpdated.Update:
- Add
tags: [spec]- Add appropriate
milestone:designation (verify which milestone this foundational spec aligns with—or if it predates the milestone structure)- Correct
lastUpdatedto2025-12-23--- lastUpdated: 2025-10-02 +tags: [spec] +milestone: [VERIFY_MILESTONE] ---Also verify that the design spec content remains accurate relative to the current M5+ architecture, particularly the schema and RPC patterns introduced in this PR.
AGENTS.md (1)
150-223: Remove the Neo4j section or move to a design document with "PLANNED" marker—it's unimplemented vaporware.Codebase verification confirms: Neo4j is referenced nowhere outside AGENTS.md. Zero integration in package.json, environment files, or any configuration. The 74-line "Neo4j Shared Memory" section (lines ~170–223) is pure aspirational documentation with no backing implementation whatsoever.
Either delete this section entirely, or migrate it to
docs/Neo4jMemory.mdwith a clearPLANNED/FUTUREheading and link it from AGENTS.md—don't embed imaginary systems in agent operational instructions.Secondary: AGENTS.md structure must follow coding guidelines: session debrief event blocks (Summary, References, Key Decisions, Action Items, Notes) require separation by horizontal rules (
---) for readability. The Neo4j section violates this constraint entirely.db/test/43_flags_rls.pgtap (1)
90-99: Consider adding aRESET ROLEbeforeROLLBACKfor explicitness.While the transaction rollback will discard the role context, explicitly resetting the role before
finish()improves readability and prevents accidental role leakage if the test structure changes later.Additionally, per learnings, pgTAP tests should cover failure cases. Consider adding a test that verifies
db8_readercannotINSERTintosubmission_flags:🔎 Proposed addition
-- And flag_details contains at least one element SELECT ok( (SELECT jsonb_array_length(COALESCE(flag_details,'[]'::jsonb)) FROM submissions_with_flags_view v WHERE v.round_id = '40000000-0000-0000-0000-000000000002') >= 1, 'Post-publish: view includes flag details' ); +RESET ROLE; + SELECT finish(); ROLLBACK;docs/Architecture.md (1)
1-3: StalelastUpdatedfrontmatter.The frontmatter shows
lastUpdated: 2025-10-02, but this PR modifies the file in December 2025. Update to the current date:--- lastUpdated: 2025-12-23 ---Per coding guidelines: "Markdown files must include YAML frontmatter with lastUpdated (ISO date)."
♻️ Duplicate comments (32)
server/test/sse.db.events.test.js (1)
20-20: Dead initialization -roomIdis still immediately overwritten.Line 20 initializes
roomIdto a hardcoded UUID'11111111-0000-0000-0000-000000000001', but line 35 unconditionally overwrites it with the result fromroom_create. This initialization serves no purpose and misleads readers into thinking the hardcoded UUID matters.This issue was already flagged in a previous review (see past_review_comments), but the fix was not applied. The initialization remains dead code.
🔎 Proposed fix
let server = null; let port; - let roomId = '11111111-0000-0000-0000-000000000001'; + let roomId; let roundId;bin/db8.js (3)
799-837: STILL hardcoded. STILL no validation. This is embarrassing.Lines 819 and 858: You're STILL using
'00000000-0000-0000-0000-000000000002'as a hardcodedround_id. This was flagged in previous reviews and you shipped it anyway.Line 800-804:
choiceis pulled fromargs._[2]with zero validation until after the fact. No Zod schema. Per coding guidelines (bin/**/*.js), CLI commands MUST validate with Zod at entry points.This will fail 100% of the time in production. Every single vote will go to the wrong round.
🔎 Fix (see previous review for full details)
case 'vote:continue': { + const ContinueVoteArgs = z.object({ + choice: z.enum(['continue', 'end']), + round: z.string().uuid() + }); + const parsed = ContinueVoteArgs.safeParse({ + choice: args._[2], + round: args.round + }); + if (!parsed.success) { + printerr('vote continue requires --round <uuid> and choice'); + return EXIT.VALIDATION; + } + const { choice, round: roundId } = parsed.data; - const choice = args._[2]; - if (choice !== 'continue' && choice !== 'end') { - printerr('vote continue requires "continue" or "end"'); - return EXIT.VALIDATION; - } ... body: JSON.stringify({ room_id: room, - round_id: '00000000-0000-0000-0000-000000000002', + round_id: roundId,
838-877: vote:final is equally broken.Line 858: Hardcoded round_id again.
Lines 839-844:
approvalparsing is permissive garbage:args.approve !== 'false'means'0','no','nope'all becometrue. Use proper Zod boolean coercion.rankingsplits on comma but NEVER validates the UUIDs. Users can pass--rank "foo,bar,baz"and the server will reject it. Fail at the CLI edge per guidelines.🔎 Proper validation
case 'vote:final': { + const FinalVoteArgs = z.object({ + round: z.string().uuid(), + approve: z.preprocess( + (v) => v === undefined ? true : v !== 'false' && v !== '0', + z.boolean() + ), + rank: z.preprocess( + (v) => v ? String(v).split(',').map(s => s.trim()).filter(Boolean) : [], + z.array(z.string().uuid()) + ).optional() + }); + const parsed = FinalVoteArgs.safeParse({ round: args.round, approve: args.approve, rank: args.rank }); + if (!parsed.success) { + printerr('vote final: --round required, --rank must be UUIDs'); + return EXIT.VALIDATION; + } + const { round: roundId, approve: approval, rank: ranking } = parsed.data; - const approval = args.approve !== undefined ? Boolean(args.approve !== 'false') : true; - const ranking = args.rank ? String(args.rank).split(',').map((s) => s.trim()) : []; ... body: JSON.stringify({ - round_id: '00000000-0000-0000-0000-000000000002', + round_id: roundId,
154-177: verify:submit validation is acceptable but inconsistent.You DID add Zod-style validation for
verify:submit(lines 154-172) andverify:summary(lines 173-176), which is correct per guidelines. However:
Inconsistency: Why did you validate verify commands properly but leave vote commands as hardcoded stubs? This makes the codebase look schizophrenic.
Minor issue (Line 162):
allowedVerdictsuses a Set for lookup, but you already validated the enum in lines 161-167. ThetoLowerCase()normalization is good, but the Set is redundant if you're going to manually check again.Consider moving ALL argument validation into a centralized Zod schema object at the top of validateArgs to avoid this pattern inconsistency.
db/rpc.sql (3)
768-772: ON CONFLICT is STILL wrong.Line 770:
ON CONFLICT (round_id, voter_id, client_nonce)allows a voter to submit MULTIPLE conflicting final votes by changing the nonce.This was flagged in previous reviews. The conflict key should be
(round_id, voter_id)to enforce one vote per voter per round.🔎 Fix
INSERT INTO final_votes (round_id, voter_id, approval, ranking, client_nonce) VALUES (p_round_id, p_voter_id, p_approval, COALESCE(p_ranking, '[]'::jsonb), COALESCE(p_client_nonce, gen_random_uuid()::text)) - ON CONFLICT (round_id, voter_id, client_nonce) + ON CONFLICT (round_id, voter_id) DO UPDATE SET approval = EXCLUDED.approval, ranking = EXCLUDED.ranking, client_nonce = EXCLUDED.client_nonce
828-836: Audit entity_type is STILL semantically wrong.Line 830:
entity_typeis'submission', but this is a SCORE record, not a submission. Line 831:entity_idisp_participant_id, but the actual entity created isv_id(the score row).This was flagged before. Either:
- Add
'score'to the allowed entity_types indb/schema.sqlline 211, OR- Use a different entity_type that makes sense (e.g.,
'system')As written, the audit log is LYING about what entity was created.
🔎 Proposed fix (add 'score' entity type)
In
db/schema.sql:CHECK (entity_type IN ( - 'room','round','submission','vote','participant','flag','system' + 'room','round','submission','vote','participant','flag','system','score' ))In
db/rpc.sql:PERFORM admin_audit_log_write( - 'update', - 'submission', - p_participant_id, + 'create', + 'score', + v_id,
870-945: Elo calculation has ordering bias (O(n²) and asymmetric).Lines 892-933: The nested loop updates
reputation.eloincrementally during iteration. When participant B is compared against participant A (line 899), A's Elo has ALREADY been modified by previous iterations. This creates ordering sensitivity.Standard Elo implementations:
- Snapshot ALL ratings before any updates
- Calculate deltas using the snapshot
- Apply all deltas simultaneously
Your implementation violates this. For participants [Alice, Bob, Carol]:
- Alice vs Bob: Alice's Elo updated
- Alice vs Carol: Alice's Elo updated again
- Bob vs Alice: Bob's delta calculated against Alice's already-modified Elo
This is incorrect and makes the results non-deterministic depending on participant ordering.
🔎 Fix: snapshot first, apply later
-- 1. Snapshot current Elos CREATE TEMP TABLE _elo_snapshot AS SELECT participant_id, COALESCE(r.elo, 1200.0) as elo FROM view_score_aggregates s LEFT JOIN reputation r ON r.participant_id = s.participant_id WHERE s.round_id = p_round_id; -- 2. Calculate deltas using snapshot CREATE TEMP TABLE _elo_deltas AS SELECT p.participant_id, SUM(delta) as total_delta FROM ( SELECT a.participant_id, v_k * (CASE WHEN a.composite_score > b.composite_score THEN 1.0 WHEN a.composite_score < b.composite_score THEN 0.0 ELSE 0.5 END - (1.0 / (1.0 + pow(10.0, (b.elo - a.elo) / 400.0)))) as delta FROM view_score_aggregates a JOIN _elo_snapshot ea ON ea.participant_id = a.participant_id CROSS JOIN view_score_aggregates b JOIN _elo_snapshot eb ON eb.participant_id = b.participant_id WHERE a.round_id = p_round_id AND b.round_id = p_round_id AND a.participant_id <> b.participant_id ) deltas GROUP BY participant_id; -- 3. Apply deltas atomically INSERT INTO reputation (participant_id, elo) SELECT participant_id, 1200.0 + total_delta FROM _elo_deltas ON CONFLICT (participant_id) DO UPDATE SET elo = reputation.elo + EXCLUDED.elo - 1200.0;server/test/final_tally.test.js (1)
18-20: Incomplete test cleanup - missing__setDbPool(null).As flagged in a previous review, other tests in this PR consistently reset the global pool reference before ending the pool. This prevents stale references from leaking into subsequent Vitest suites.
🔎 Proposed fix
afterAll(async () => { + __setDbPool(null); await pool.end(); });server/test/voting.final.test.js (2)
8-8: INCONSISTENT: MissingDB8_TEST_DATABASE_URLin fallback chain.Every other test file in this PR follows the pattern:
const dbUrl = process.env.DB8_TEST_DATABASE_URL || process.env.DATABASE_URL || 'fallback';This file skips the first check, which will break CI when
DB8_TEST_DATABASE_URLis set butDATABASE_URLis not.🔎 Proposed fix
- const dbUrl = process.env.DATABASE_URL || 'postgresql://postgres:test@localhost:54329/db8_test'; + const dbUrl = + process.env.DB8_TEST_DATABASE_URL || + process.env.DATABASE_URL || + 'postgresql://postgres:test@localhost:54329/db8_test';
18-20: INCONSISTENT: Missing__setDbPool(null)in cleanup.The global DB pool reference remains stale after the pool closes. This pattern violation appears across multiple test files.
🔎 Proposed fix
afterAll(async () => { + __setDbPool(null); await pool.end(); });server/test/lifecycle.test.js (3)
19-21: INCONSISTENT: Missing__setDbPool(null)in cleanup.The global DB pool reference remains stale after the pool closes. Apply the same fix as the other test files.
🔎 Proposed fix
afterAll(async () => { + __setDbPool(null); await pool.end(); });
33-36: TEST RELIABILITY: Stale state persists across reruns.The round upsert uses
DO NOTHING, which allows a pre-existingphase='final'row to persist. On subsequent test runs, the test will pass trivially without exercising the actual transition logic.The room upsert (line 29) correctly uses
DO UPDATE SETto reset state. Apply the same pattern to the round.🔎 Proposed fix
await pool.query( - "insert into rounds(id, room_id, idx, phase, continue_vote_close_unix) values ($1, $2, 0, 'published', 100) on conflict (id) do nothing", + "insert into rounds(id, room_id, idx, phase, continue_vote_close_unix) values ($1, $2, 0, 'published', 100) on conflict (id) do update set phase = 'published', continue_vote_close_unix = 100", [roundId, roomId] );
34-46: READABILITY: Undocumented magic values.
continue_vote_close_unix=100(line 34) and'{"choice": "end"}'(line 44) are unexplained magic values. Future maintainers will ask:
- Why 100? (Any past timestamp forces closed voting window)
- What ballot schema? What other choices exist?
🔎 Proposed fix
+ // Use past timestamp (100) to simulate closed voting window await pool.query( "insert into rounds(id, room_id, idx, phase, continue_vote_close_unix) values ($1, $2, 0, 'published', 100) on conflict (id) do update set phase = 'published', continue_vote_close_unix = 100", [roundId, roomId] ); await pool.query( 'insert into participants(id, room_id, anon_name) values ($1, $2, $3) on conflict (id) do nothing', [participantId, roomId, 'voter_unique_1'] ); - // Tally is No (or equal), so it should transition to final + // Vote to END round (ballot choice: "end" vs "continue") + // Tally determines transition to final phase await pool.query( "insert into votes(round_id, voter_id, kind, ballot, client_nonce) values ($1, $2, 'continue', '{\"choice\": \"end\"}', 'nonce-lifecycle-1')", [roundId, participantId] );server/test/attribution.test.js (3)
20-22: UNRESOLVED: Missing__setDbPool(null)cleanup - previously flagged.This is the EXACT issue raised in prior review. The
afterAllcleanup must call__setDbPool(null)beforepool.end()to properly reset the module-level singleton.🔎 Required fix (as previously suggested)
afterAll(async () => { + __setDbPool(null); await pool.end(); });
48-48: UNRESOLVED: Inconsistent session variable setting - previously flagged.Line 48 uses raw
SETwhile line 58 usesset_config(). This inconsistency was flagged before. Use parameterizedset_config()consistently to avoid SQL injection risks.🔎 Required fix (as previously suggested)
- await pool.query("set db8.participant_id = '00000000-0000-0000-0000-000000000000'"); + await pool.query("select set_config('db8.participant_id', $1, false)", ['00000000-0000-0000-0000-000000000000']);Apply the same fix to line 73.
66-79: UNRESOLVED: Test coupling - Test 2 depends on Test 1 state - previously flagged.Test 2 assumes data seeded by Test 1 exists. If Test 1 fails or tests run in isolation, Test 2 fails mysteriously. This fragility was already raised. Either seed independently or combine into one test.
db/rls.sql (1)
10-14: Missing test-delete policies for new RLS-enabled tables.RLS is enabled for
verification_verdicts,final_votes,scores,reputation, andreputation_tag, but unlikerooms(lines 44-49) androunds(lines 68-73), these tables lack correspondingtest_delete_policyentries. This creates an asymmetric policy landscape that will bite when test teardown attempts directDELETEstatements.As per coding guidelines: "pgTAP invariants must cover tables, uniques, views, and RPC contracts."
server/test/auth.ssh.test.js (1)
127-169: DB-backed test is well-isolated with proper cleanup.The
try/finallypattern ensures__setDbPool(null)andpool.end()run even on assertion failure. The explicittruncate rooms cascadeguarantees a clean slate.However, per the past review comment, the UUIDs (
10000000-*) overlap with the in-memory tests above. While this works because in-memory tests use__setDbPool(null), it's still fragile. Consider using a distinct prefix like10000001-*.server/test/scoring.test.js (5)
21-23: Missing__setDbPool(null)in afterAll - STILL NOT FIXED.Past review flagged this exact issue. The teardown does not reset the module-level pool reference, risking cross-test contamination if tests run in different orders or pools are reused.
🔎 Required fix
afterAll(async () => { + __setDbPool(null); await pool.end(); });
46-60: Test does not verify database persistence - STILL NOT FIXED.Past review noted: "A 200 response doesn't guarantee the data was persisted correctly." The test only checks HTTP response, not actual DB state.
Add DB verification after line 59:
// Verify DB persistence const dbRes = await pool.query( 'select * from scores where round_id = $1 and judge_id = $2 and participant_id = $3', [roundId, judgeId, debaterId] ); expect(dbRes.rows.length).toBe(1); expect(dbRes.rows[0].e).toBe(80); expect(dbRes.rows[0].r).toBe(75); expect(dbRes.rows[0].c).toBe(90); expect(dbRes.rows[0].v).toBe(70); expect(dbRes.rows[0].y).toBe(85);
62-69: Weak assertion:composite_score > 0is COMPLETELY USELESS - STILL NOT FIXED.Past review noted this proves nothing. With input scores E=80, R=75, C=90, V=70, Y=85, the composite should be approximately 80 (assuming equal weights). The current assertion
> 0would pass even if the calculation returned 0.001.Replace with meaningful assertion:
const composite = parseFloat(res.body.rows[0].composite_score); expect(composite).toBeGreaterThan(70); expect(composite).toBeLessThan(90); // Or if you know the exact formula, assert with tolerance: // expect(composite).toBeCloseTo(80, 1);
96-101: Elo verification is LAUGHABLY INADEQUATE - STILL NOT FIXED.Past review correctly noted: "You claim 'deterministic' Elo updates but only verify the value changed." With debater scoring ~80 avg (winner) and opponent scoring 50 avg (loser):
- Debater's Elo MUST increase (> 1200)
- Opponent's Elo MUST decrease (< 1200)
- Zero-sum property: total Elo change MUST equal zero
Current test only checks
not.toBe(1200)which would pass if Elo became 1199 (incorrect direction).const debaterRep = await supertest(app) .get('/rpc/reputation.get') .query({ participant_id: debaterId }); const opponentRep = await supertest(app) .get('/rpc/reputation.get') .query({ participant_id: opponentId }); expect(debaterRep.body.elo).toBeGreaterThan(1200); // Winner gains expect(opponentRep.body.elo).toBeLessThan(1200); // Loser loses // Zero-sum check const delta1 = debaterRep.body.elo - 1200; const delta2 = opponentRep.body.elo - 1200; expect(delta1 + delta2).toBeCloseTo(0, 1);
103-118: Test is COMPLETELY POINTLESS - no tag reputation data seeded - STILL NOT FIXED.Past review noted: "You create a room with tags but never create a round in that room, submit scores for that round, or run reputation.update for that room."
This test proves NOTHING about tag-based reputation functionality. It only verifies the API returns a default Elo when no data exists.
Either:
Option A: Seed complete tag-based reputation data:
// Create round in tagged room const tagRoundId = '90000000-0000-0000-0000-000000000020'; await pool.query( "insert into rounds(id, room_id, idx, phase) values ($1, $2, 0, 'published')", [tagRoundId, roomIdTag] ); // Add participants and scores for that round await pool.query( 'insert into participants(id, room_id, anon_name, role) values ($1, $2, $3, $4)', [debaterId, roomIdTag, 'debater_tag', 'debater'] ); // Submit scores and run reputation.update for roomIdTag await supertest(app).post('/rpc/score.submit').send({ round_id: tagRoundId, judge_id: judgeId, participant_id: debaterId, e: 90, r: 90, c: 90, v: 90, y: 90, client_nonce: 'tag-score' }); await supertest(app).post('/rpc/reputation.update').send({ room_id: roomIdTag }); // Then verify tag-specific Elo is not default const res = await supertest(app) .get('/rpc/reputation.get') .query({ participant_id: debaterId, tag: 'science' }); expect(res.body.elo).not.toBe(1200);Option B: Rename test to
"returns default Elo when no tag reputation exists"and assert default value explicitly.server/rpc.js (3)
569-570:score.submitstill returnsscore_idwithout null check.Line 570 returns
{ ok: true, score_id }without verifyingscore_idis truthy. Compare tovote.final(line 533) which properly throws on missing ID.🔎 Proposed fix
const score_id = r.rows?.[0]?.id; + if (!score_id) throw new Error('score_submit_missing_id'); return res.json({ ok: true, score_id });
614-627:reputation.updatestill lacks Zod validation—query can run with NULL parameters.Lines 617-626: If
req.bodyis{}, bothroom_idandround_idare undefined. The query at line 623 then runs with$1 = undefined(becomes NULL in SQL), potentially returning the latest round from ANY room or none at all.This endpoint is the only one in the new batch without Zod validation.
🔎 Proposed fix
+const ReputationUpdateIn = z.object({ + room_id: z.string().uuid().optional(), + round_id: z.string().uuid().optional() +}).refine(v => v.room_id || v.round_id, { + message: 'Either room_id or round_id must be provided' +}); + app.post('/rpc/reputation.update', async (req, res) => { try { - const { room_id, round_id } = req.body || {}; + const { room_id, round_id } = ReputationUpdateIn.parse(req.body);
1178-1180: Stale TODO masquerading as documentation persists.Lines 1179-1180 state that
final_votedoesn't carryroom_id, yet the filter logic at line 1178 still checkspayload.room_id !== roomId. This is a known issue from prior review that remains unaddressed.Either fix the DB notification to include
room_idor fix the filter to handlefinal_votechannel differently.web/app/room/[roomId]/page.jsx (3)
59-83: Missing AbortController and unsafe state access (previously flagged).This issue was flagged in a previous review and remains unresolved:
- No AbortController to cancel fetch on unmount
state.round.round_idaccess on line 70 is unsafe if state/round is undefinedApply the fix from the previous review comment.
As per coding guidelines, AbortController is required for async operations in React components.
85-109: Same AbortController and unsafe access issues (previously flagged).Identical to the previous comment on
onContinueVote:
- Missing AbortController
- Unsafe
state.round.round_idaccess on line 95Apply the same fix pattern.
As per coding guidelines, AbortController is required for async operations in React components.
840-894: Modal accessibility issues (previously flagged).These vote modals were flagged in a previous review for missing:
- Focus trap
- Escape key handling
aria-modalattributesThe issue remains unresolved. Apply the fixes from the previous review comment.
server/schemas.js (1)
132-150: Inconsistent client_nonce requirements (previously flagged).This issue was flagged in a previous review and remains unresolved:
FinalVote(line 137) andScoreSubmit(line 149) have optionalclient_nonce, whileVerifySubmit(line 129) andContinueVote(line 39) have requiredclient_nonce.Optional nonces break idempotency. If a user retries a final vote without a nonce, the DB generates a new one, creating duplicate records. Make them all required.
As per coding guidelines, use Zod schemas at the edges for request validation.
db/schema.sql (2)
102-114: UNIQUE constraint allows duplicate final votes (previously flagged).This CRITICAL issue was flagged in a previous review and remains unresolved:
Line 111:
UNIQUE (round_id, voter_id, client_nonce)A voter can submit multiple conflicting final votes for the same round by using different
client_noncevalues. Each different nonce creates a new row.The constraint should be:
UNIQUE (round_id, voter_id)And the RPC
vote_final_submitmust useON CONFLICT (round_id, voter_id)instead ofON CONFLICT (round_id, voter_id, client_nonce).Apply the fix from the previous review comment immediately.
Based on learnings, submission verification must enforce proper constraints.
116-133: UNIQUE constraint allows duplicate scores (previously flagged).This CRITICAL issue was flagged in a previous review and remains unresolved:
Line 129:
UNIQUE (round_id, judge_id, participant_id, client_nonce)A judge can submit multiple rubric scores for the same participant in the same round by using different
client_noncevalues. The aggregation view will then AVERAGE them, which is nonsensical.The constraint should be:
UNIQUE (round_id, judge_id, participant_id)Apply the fix from the previous review comment immediately.
Based on learnings, submission verification must enforce proper constraints.
| #!/usr/bin/env bash | ||
| msg_file="$1" | ||
| pattern='^(revert: )?(feat|fix|docs|style|refactor|perf|test|build|ci|chore|ops|sec)(\(.+\))?: .+' | ||
| merge_pattern='^Merge (branch|pull request|remote-tracking branch)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete merge pattern: tag merges will be incorrectly rejected.
The pattern covers branch, pull request, and remote-tracking branch merges, but omits tag merges. Git generates messages like Merge tag 'v1.0.0' which will fail the conventional commits check.
🔎 Fix to include tag merges
-merge_pattern='^Merge (branch|pull request|remote-tracking branch)'
+merge_pattern='^Merge (branch|pull request|remote-tracking branch|tags?)'Note: tags? handles both "Merge tag" (singular) and "Merge tags" (multiple tags).
📝 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.
| merge_pattern='^Merge (branch|pull request|remote-tracking branch)' | |
| merge_pattern='^Merge (branch|pull request|remote-tracking branch|tags?)' |
🤖 Prompt for AI Agents
.githooks/commit-msg around line 4: the current merge_pattern only matches
branch, pull request, and remote-tracking branch merges and will reject tag
merge messages like "Merge tag 'v1.0.0'"; update the regex to also accept "tag"
or "tags" (e.g., add tags? into the alternation) so merge messages for tags are
allowed by the conventional commit check.
| - name: Detect path changes | ||
| id: changes | ||
| uses: dorny/paths-filter@v3 | ||
| with: | ||
| filters: | | ||
| web: | ||
| - 'web/**' | ||
| - 'public/**' | ||
| - 'eslint.config.js' | ||
| - 'web/package.json' | ||
| - 'web/package-lock.json' | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Next.js config files in web/
echo "=== Searching for Next.js config files ==="
fd -e js -e mjs -e ts 'next\.config' web/ 2>/dev/null || echo "No next.config found"
# Find other potential web config files
echo ""
echo "=== Searching for other web config files ==="
fd -e json -e js -e mjs 'tsconfig|jsconfig|postcss|tailwind' web/ 2>/dev/null || echo "No matching files found"
# List root files in web/ directory
echo ""
echo "=== All files in web/ root directory ==="
ls -la web/ | grep -E '^\-' | awk '{print $NF}'
# Check if eslint.config.js exists at repo root
echo ""
echo "=== Checking eslint.config.js location ==="
ls -la eslint.config.js 2>/dev/null || echo "No eslint.config.js at root"
# Find all package.json files in web tree
echo ""
echo "=== All package.json files in web/ ==="
fd 'package.json' web/Repository: flyingrobots/db8
Length of output: 546
Add missing web configuration files to the path filter.
The filter is critically incomplete. It fails to include several essential web build dependencies:
web/next.config.js— changes here bypass the web rebuild entirelyweb/jsconfig.json— module resolution configurationweb/postcss.config.js— CSS processing configurationweb/tailwind.config.js— Tailwind CSS compilation configuration
Changes to any of these files will not trigger the web workflow. Add all four to the filter. The eslint.config.js concern remains secondary — verify whether linting config changes warrant a rebuild in your workflow design.
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 45 to 56 the paths-filter for the "web"
group is missing several web configuration files so changes to them don't
trigger the web workflow; add web/next.config.js, web/jsconfig.json,
web/postcss.config.js, and web/tailwind.config.js to the 'web' filter list so
modifications to these config files will cause the web workflow to run, and
verify whether eslint.config.js should remain included based on whether lint
config changes should trigger a rebuild.
| - name: Build Web (Next.js) | ||
| if: github.event_name == 'push' || steps.changes.outputs.web == 'true' | ||
| env: | ||
| NODE_ENV: production | ||
| NEXT_TELEMETRY_DISABLED: '1' | ||
| run: npm --prefix web run build | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Asymmetric build condition: always build on push, conditional on PRs.
The Build Web step condition github.event_name == 'push' || steps.changes.outputs.web == 'true' means:
- On
pushevents (to main branch): always builds web, regardless of which files changed. - On
pull_requestevents: only builds if the path filter detects web-related changes.
This asymmetry introduces a risk: if a PR that doesn't touch web files is merged to main, the main build will run even though the PR build didn't. This could mask build failures that only manifest in production builds.
Consider either:
- Making both events conditional on the path filter:
steps.changes.outputs.web == 'true' - Documenting why main builds must always run (e.g., for deployment purposes).
🔎 Proposed fix to make the condition symmetric
- name: Build Web (Next.js)
- if: github.event_name == 'push' || steps.changes.outputs.web == 'true'
+ if: steps.changes.outputs.web == 'true'
env:
NODE_ENV: production
NEXT_TELEMETRY_DISABLED: '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.
| - name: Build Web (Next.js) | |
| if: github.event_name == 'push' || steps.changes.outputs.web == 'true' | |
| env: | |
| NODE_ENV: production | |
| NEXT_TELEMETRY_DISABLED: '1' | |
| run: npm --prefix web run build | |
| - name: Build Web (Next.js) | |
| if: steps.changes.outputs.web == 'true' | |
| env: | |
| NODE_ENV: production | |
| NEXT_TELEMETRY_DISABLED: '1' | |
| run: npm --prefix web run build |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 60 to 66: the Build Web step condition is
asymmetric (always runs on push but only conditional on PRs); change the if
expression to use the path filter for both events by replacing the current
condition with steps.changes.outputs.web == 'true' so the build only runs when
web files changed, or alternatively add an explicit comment explaining why
builds must run on push (e.g., required for deployment) if you intend to keep
the current behavior.
| - name: Create test database | ||
| env: | ||
| PGPASSWORD: test | ||
| run: | | ||
| until pg_isready -h localhost -p 5432 -U postgres; do sleep 2; done | ||
| psql -h localhost -U postgres -c 'CREATE DATABASE db8_test;' | ||
| - name: Prepare schema/RPC/RLS | ||
| env: | ||
| DATABASE_URL: postgresql://postgres:test@localhost:5432/db8_test | ||
| DB8_TEST_OUTPUT: quiet | ||
| run: node scripts/prepare-db.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Hardcoded test credentials are acceptable for ephemeral CI containers.
The Checkov warning (CKV_SECRET_4) on lines 57-58 is a false positive. These credentials exist solely for a throwaway Postgres container that lives only during the CI run. The DATABASE_URL is correctly scoped to the job and not persisted.
However, if you want to silence the static analysis noise and follow the principle of least surprise, you could reference POSTGRES_PASSWORD from the service definition:
🔎 Optional: DRY the password reference
- name: Prepare schema/RPC/RLS
env:
- DATABASE_URL: postgresql://postgres:test@localhost:5432/db8_test
+ DATABASE_URL: postgresql://postgres:${{ env.PGPASSWORD }}@localhost:5432/db8_test
DB8_TEST_OUTPUT: quiet
run: node scripts/prepare-db.jsNote: This would require setting PGPASSWORD as a job-level env var. The current approach is fine.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 57-58: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
In .github/workflows/db-tests.yml around lines 49 to 59 the CI job hardcodes the
test database password and DATABASE_URL which triggers the Checkov CKV_SECRET_4
false positive; replace the literal password by referencing the service-level
POSTGRES_PASSWORD env var (or set PGPASSWORD as a job-level env var) and use
that variable in the DATABASE_URL so the secret is not inline in the job; ensure
the service definition declares POSTGRES_PASSWORD and that the job env uses it
(and update the psql invocation to rely on PGPASSWORD) so credentials remain
ephemeral but are no longer hardcoded in the workflow.
|
|
||
| - Issues: closed/moved — #67, #68, #70, #30, #117, #121, #9, #10 (closed); #11, #12, #29, #7 (→ M3); #31, #15 (→ M6); #32, #13, #14 (→ M7) | ||
| - PRs: #144 (CLI SSH verify + docs), #145/#146/#142 (deps alignment), #148 (db‑tests workflow + README milestone focus) | ||
| - Files: `server/test/cli.journal.verify.test.js`, `docs/Provenance.md`, `.gitignore`, `server/rpc.js`, `.github/workflows/db-tests.yml`, `README.md` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivial: Use "GitHub" with capital H.
Static analysis correctly flags this. Fix it.
🔎 One-character fix
- - Files: `server/test/cli.journal.verify.test.js`, `docs/Provenance.md`, `.gitignore`, `server/rpc.js`, `.github/workflows/db-tests.yml`, `README.md`
+ - Files: `server/test/cli.journal.verify.test.js`, `docs/Provenance.md`, `.gitignore`, `server/rpc.js`, `.github/workflows/db-tests.yml`, `README.md`Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1036-~1036: The official name of this software platform is spelled with a capital “H”.
Context: ...nce.md, .gitignore, server/rpc.js, .github/workflows/db-tests.yml, README.md` #...
(GITHUB)
🤖 Prompt for AI Agents
In AGENTS.md around line 1036 the word "GitHub" is incorrectly cased; update the
text to use the correct capitalization "GitHub" (capital G and H) wherever that
instance appears on this line so the repository uses the proper trademark
casing.
| ```text | ||
| ███████████████████████████████████████░░░░░░░░░░░░░░░░░░░░░ | ||
| | | | | | | | | | ||
| 0 M1 M2 M3 M4 M5M6 M7 | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch milestone data to verify progress bar alignment
# Try to list milestones and their issue counts
# Check if gh CLI is available
if command -v gh &> /dev/null; then
echo "Attempting to fetch milestone data via gh CLI..."
# Note: This may fail due to authentication or repository access in sandbox
gh issue list --repo flyingrobots/db8 --state all --json milestone,labels --limit 1000 2>&1 || echo "gh CLI access failed"
else
echo "gh CLI not available in sandbox environment"
# Alternative: search for README.md to examine the context around the visualization
echo "Searching for README.md to analyze the progress bar context..."
fi
# List files to understand repository structure
echo -e "\n=== Repository structure ==="
git ls-files | head -30Repository: flyingrobots/db8
Length of output: 41825
🏁 Script executed:
#!/bin/bash
# Parse the issue data and compute weighted milestone completion
python3 <<'PYEOF'
import json
# The raw gh issue output (already fetched above)
raw_json = '''[...]''' # This is too large—let me use a different approach
# Instead, re-fetch with better formatting for analysis
import subprocess
result = subprocess.run([
'gh', 'issue', 'list',
'--repo', 'flyingrobots/db8',
'--state', 'all',
'--json', 'milestone,labels,state',
'--jq', 'group_by(.milestone.title) | map({milestone: .[0].milestone.title, issues: .})'
], capture_output=True, text=True)
if result.returncode != 0:
print(f"Error: {result.stderr}")
exit(1)
data = json.loads(result.stdout)
weights = {'p0': 8, 'p1': 5, 'p2': 3, 'p3': 1}
print("=== Milestone Completion Analysis ===\n")
cumulative_closed = 0
cumulative_total_weight = 0
for group in data:
milestone_title = group['milestone']
if not milestone_title:
continue
issues = group['issues']
total_issues = len(issues)
closed_count = sum(1 for i in issues if i['state'] == 'CLOSED')
total_weight = 0
closed_weight = 0
for issue in issues:
# Extract priority from labels
priority = 'default'
for label in issue.get('labels', []):
if label['name'].startswith('priority/p'):
priority = label['name'].replace('priority/', '')
break
weight = weights.get(priority, 1)
total_weight += weight
if issue['state'] == 'CLOSED':
closed_weight += weight
percent = (closed_weight / total_weight * 100) if total_weight > 0 else 0
cumulative_closed += closed_weight
cumulative_total_weight += total_weight
print(f"{milestone_title}: {closed_count}/{total_issues} issues | Weighted: {closed_weight}/{total_weight} | {percent:.1f}%")
if cumulative_total_weight > 0:
overall = (cumulative_closed / cumulative_total_weight * 100)
print(f"\n=== Overall Cumulative Completion (M0-M5) ===")
print(f"Weighted: {cumulative_closed}/{cumulative_total_weight} = {overall:.1f}%")
print(f"\nExpected bar fill: ~{int(overall * 61 / 100)} filled chars out of 61")
PYEOFRepository: flyingrobots/db8
Length of output: 653
The progress bar visualization is factually incorrect and significantly misleads about project completion status.
The bar shows approximately 67% completion (41 filled chars of 61 total), but actual weighted completion through M5 is only 50% (41/82 weighted points):
- M1: MVP Loop = 100% (11/11 weighted)
- M2: Provenance & Journals = 100% (24/24 weighted)
- M3: Verification = 0% (0/5 weighted)
- M4: Votes & Final = 0% (0/9 weighted)
- M5: Scoring & Elo = 0% (0/16 weighted) ← Should be visually incomplete, not filled
The visualization overstates progress by approximately 17 percentage points and incorrectly implies M5 is complete when zero of its four issues are closed. Correct the bar rendering to accurately represent 50% cumulative weighted completion.
🤖 Prompt for AI Agents
In README.md around lines 15 to 19, the ASCII progress bar hardcodes filled
characters and milestone markers which overstates progress; change the rendering
to compute filled length from weighted completion (use total weighted points =
82 and completed = 41 -> 50%) and set the number of filled characters
proportionally rather than hardcoding 41/61, recalculate milestone marker
positions using cumulative weights so M5 is shown incomplete, and update the
text/bar so the visual accurately reflects 50% cumulative weighted completion.
| import { describe, it, expect } from 'vitest'; | ||
| import request from 'supertest'; | ||
| import app from '../rpc.js'; | ||
|
|
||
| describe('GET /rpc/participant', () => { | ||
| it('returns role=judge for judge-* IDs (memory fallback)', async () => { | ||
| const res = await request(app) | ||
| .get('/rpc/participant') | ||
| .query({ room_id: '00000000-0000-0000-0000-000000000001', id: 'judge-123' }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.ok).toBe(true); | ||
| expect(res.body.role).toBe('judge'); | ||
| }); | ||
|
|
||
| it('returns role=debater for other IDs (memory fallback)', async () => { | ||
| const res = await request(app) | ||
| .get('/rpc/participant') | ||
| .query({ room_id: '00000000-0000-0000-0000-000000000001', id: 'user-456' }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.ok).toBe(true); | ||
| expect(res.body.role).toBe('debater'); | ||
| }); | ||
|
|
||
| it('returns 400 if params missing', async () => { | ||
| const res = await request(app).get('/rpc/participant'); | ||
| expect(res.status).toBe(400); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Tests are adequate but superficial.
Lines 6-24: You test the memory fallback for judge-* and other IDs, which is good. However:
-
Missing DB path test: This only tests memory fallback. Where's the test that exercises the actual DB query when
DATABASE_URLis set? -
No RLS test: Does
/rpc/participantrespect RLS policies? Can participant A see participant B's details? -
No error cases: What if the participant doesn't exist in the DB?
The tests you have are fine, but they're incomplete.
Add a DB-backed test that seeds participants and verifies RLS:
describe('GET /rpc/participant (DB)', () => {
it('respects RLS and returns participant from DB', async () => {
// Seed DB, set DATABASE_URL, test
});
});🤖 Prompt for AI Agents
In server/test/rpc.participant.get.test.js around lines 1-30, the tests only
cover the memory fallback and miss DB-backed scenarios, RLS behavior, and
missing-participant errors; add a new describe block "GET /rpc/participant (DB)"
that sets up a test Postgres (or test database) by setting DATABASE_URL for the
process, seeds participants (including two participants in the same room with
different IDs and roles), ensures RLS is enforced by running the request as one
participant and asserting it cannot see another's private data while it can see
its own, asserts the correct role is returned for an existing DB participant,
asserts 404 or appropriate error when the participant does not exist, and cleans
up seeded data and restores DATABASE_URL/env after tests.
| const first = await request(app).post('/rpc/verify.submit').send(payload); | ||
| if (first.status !== 200) { | ||
| console.error('verify.submit first failed', first.status, first.body); | ||
| } | ||
| expect(first.status).toBe(200); | ||
| const second = await request(app).post('/rpc/verify.submit').send(payload).expect(200); | ||
| expect(first.body.ok).toBe(true); | ||
| expect(second.body.id).toEqual(first.body.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Inconsistent assertion style between first and second requests.
Lines 50-54 use a separate expect() call after awaiting, while line 55 chains .expect(200) inline. Pick one style for consistency:
🔎 Proposed fix for consistency
const first = await request(app).post('/rpc/verify.submit').send(payload);
if (first.status !== 200) {
console.error('verify.submit first failed', first.status, first.body);
}
expect(first.status).toBe(200);
- const second = await request(app).post('/rpc/verify.submit').send(payload).expect(200);
+ const second = await request(app).post('/rpc/verify.submit').send(payload);
+ expect(second.status).toBe(200);
expect(first.body.ok).toBe(true);
expect(second.body.id).toEqual(first.body.id);📝 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.
| const first = await request(app).post('/rpc/verify.submit').send(payload); | |
| if (first.status !== 200) { | |
| console.error('verify.submit first failed', first.status, first.body); | |
| } | |
| expect(first.status).toBe(200); | |
| const second = await request(app).post('/rpc/verify.submit').send(payload).expect(200); | |
| expect(first.body.ok).toBe(true); | |
| expect(second.body.id).toEqual(first.body.id); | |
| const first = await request(app).post('/rpc/verify.submit').send(payload); | |
| if (first.status !== 200) { | |
| console.error('verify.submit first failed', first.status, first.body); | |
| } | |
| expect(first.status).toBe(200); | |
| const second = await request(app).post('/rpc/verify.submit').send(payload); | |
| expect(second.status).toBe(200); | |
| expect(first.body.ok).toBe(true); | |
| expect(second.body.id).toEqual(first.body.id); |
🤖 Prompt for AI Agents
In server/test/rpc.verify.submit.test.js around lines 50 to 57, the two POST
requests use inconsistent assertion styles (the first checks status with a
separate expect after awaiting, the second chains .expect(200)); make them
consistent by choosing one style — either chain .expect(200) on both requests or
await and then use expect(response.status).toBe(200) for both — and update the
test so both requests follow the same pattern and keep subsequent body
assertions unchanged.
| it('rejects non-existent submission_id', async () => { | ||
| const res = await request(app).post('/rpc/verify.submit').send({ | ||
| round_id: ROUND_ID, | ||
| reporter_id: REPORTER_ID, | ||
| submission_id: '00000000-0000-0000-0000-00000000ffff', | ||
| verdict: 'true', | ||
| client_nonce: 'ver-missing' | ||
| }); | ||
| expect(res.status).toBeGreaterThanOrEqual(400); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Non-existent submission test is valuable but assertion could be tighter.
The test correctly expects a 4xx response for non-existent submission_id. Consider asserting for a specific status (likely 400 or 404) and checking the error message to ensure the right error path is triggered:
🔎 Proposed tighter assertion
it('rejects non-existent submission_id', async () => {
const res = await request(app).post('/rpc/verify.submit').send({
round_id: ROUND_ID,
reporter_id: REPORTER_ID,
submission_id: '00000000-0000-0000-0000-00000000ffff',
verdict: 'true',
client_nonce: 'ver-missing'
});
- expect(res.status).toBeGreaterThanOrEqual(400);
+ expect(res.status).toBe(400);
+ expect(res.body.error).toMatch(/submission|not found/i);
});🤖 Prompt for AI Agents
In server/test/rpc.verify.submit.test.js around lines 93 to 102, the test
currently only asserts a generic 4xx response for a non-existent submission_id;
tighten it to assert the exact expected status (e.g., 404 or 400 depending on
API contract) and verify the error payload to ensure the correct error path is
hit. Update the test to expect the specific status code (replace
toBeGreaterThanOrEqual(400) with toBe(404) or toBe(400) per the API), and add an
assertion that the response body contains the expected error field/message
(e.g., res.body.error or res.body.message includes "submission" or "not found")
so the test fails if a different error or success path is returned.
| with pub as ( | ||
| select r.room_id, r.id as round_id, r.idx, r.phase, r.submit_deadline_unix, r.published_at_unix, r.continue_vote_close_unix | ||
| from rounds r | ||
| from rounds_view r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Search for rounds_view definition and security_barrier configuration
find db -name "*.sql" -type f | head -20Repository: flyingrobots/db8
Length of output: 140
🏁 Script executed:
# Look for rounds_view creation and security_barrier settings
rg -n "CREATE.*VIEW rounds_view" db/
rg -n "ALTER VIEW rounds_view.*security_barrier" db/
rg -n "rounds_view" db/ -A 3 -B 1Repository: flyingrobots/db8
Length of output: 693
🏁 Script executed:
# Inspect watcher.js to see what it queries and expects
head -100 server/watcher.jsRepository: flyingrobots/db8
Length of output: 3569
🏁 Script executed:
# Check for RLS policies on rounds or rounds_view
rg -n "ALTER TABLE.*rounds" db/ | grep -i "policy\|rls\|security"
rg -n "CREATE POLICY" db/ | grep -i "rounds"Repository: flyingrobots/db8
Length of output: 42
🏁 Script executed:
# Direct inspection of RLS policy definitions
cat db/rls.sqlRepository: flyingrobots/db8
Length of output: 7137
🏁 Script executed:
# Check if there's a service role/user defined for watcher
rg -n "watcher\|service.*account\|watcher.*role" server/ db/ --ignore-caseRepository: flyingrobots/db8
Length of output: 42
🏁 Script executed:
# Check round_publish_due and round_open_next function definitions for RLS behavior
rg -n "CREATE OR REPLACE FUNCTION round_publish_due\|CREATE OR REPLACE FUNCTION round_open_next" db/Repository: flyingrobots/db8
Length of output: 42
Round phase filtering must be enforced at RLS layer, not application layer.
The column alignment and security_barrier = true are correct, but the rounds_read_policy violates the required RLS architecture. The policy uses using (true), granting unrestricted SELECT access to all rounds at all phases to any role. This defeats the purpose of RLS and leaves unpublished round data exposed to any database connection.
Per architectural requirement: RLS policies must restrict visibility by role and status. The rounds_read_policy should be replaced with a role and phase-aware policy that gates access to published rounds for public roles, while allowing service/privileged roles access only during appropriate operations. Alternatively, the watcher's service account should use a dedicated connection with explicit grants rather than relying on broken RLS.
🤖 Prompt for AI Agents
In server/watcher.js around line 30, the PR is leaving RLS bypassed by a policy
that uses "using (true)" which grants unrestricted SELECTs; replace that
application-layer filtering with a proper DB-side RLS rule: remove/replace the
rounds_read_policy using(true) and create a role-and-phase-aware USING
expression that only exposes rows where phase = 'published' to public roles and
allows service/privileged roles (the watcher) access when appropriate;
alternatively, ensure the watcher uses a dedicated DB connection role with
explicit GRANTs and no reliance on a permissive RLS policy; keep
security_barrier and column alignment as-is and update DB migrations/policies
accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review continued from previous batch...
| - name: Run DB-gated tests (glob) | ||
| env: | ||
| DB8_TEST_PG: '1' | ||
| DB8_TEST_DATABASE_URL: postgresql://postgres:test@localhost:5432/db8_test | ||
| run: | | ||
| npx vitest run "server/test/**/*.db.*.js" server/test/journal.byidx.test.js --reporter verbose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Explore server/test directory structure and file naming
echo "=== Directory structure of server/test ==="
find server/test -type f -name "*.js" 2>/dev/null | sort | head -30
echo ""
echo "=== Files matching *.db.*.js pattern ==="
find server/test -type f -name "*.db.*.js" 2>/dev/null | sort
echo ""
echo "=== Checking for journal.byidx.test.js ==="
find server/test -type f -name "*journal.byidx*" 2>/dev/null
echo ""
echo "=== All .test.js files in server/test ==="
find server/test -type f -name "*.test.js" 2>/dev/null | sort | head -30Repository: flyingrobots/db8
Length of output: 2657
Fix the naming convention violation or justify the explicit inclusion.
The glob "server/test/**/*.db.*.js" correctly matches 6 database test files (rpc.db.integration.test.js, rpc.db.postgres.test.js, rpc.db.verify.test.js, sse.db.events.test.js, sse.db.journal.test.js, watcher.db.flip.test.js). However, journal.byidx.test.js is manually appended outside the glob—it doesn't follow the .db. naming convention. If this is a database test, rename it to follow the pattern (e.g., journal.byidx.db.test.js). If it isn't, remove it from this DB-specific test run.
🤖 Prompt for AI Agents
.github/workflows/db-tests.yml around lines 64 to 69: the workflow runs a glob
matching DB tests ("server/test/**/*.db.*.js") but then explicitly appends
server/test/journal.byidx.test.js which does not follow the .db. naming
convention; either rename the test file to follow the pattern (e.g.,
server/test/journal.byidx.db.test.js) and update any imports/refs so it is
covered by the glob, or remove the explicit filename from this DB-specific step
if that test is not a database test; ensure any other workflow steps or scripts
referencing the old filename are updated accordingly.
| -- Seed a room/round/submission and participants (judge + author) | ||
| DO $$ | ||
| DECLARE | ||
| rid uuid := '20000000-0000-0000-0000-000000000001'; | ||
| r0 uuid := '20000000-0000-0000-0000-000000000002'; | ||
| author uuid := '20000000-0000-0000-0000-000000000003'; | ||
| judge uuid := '20000000-0000-0000-0000-000000000004'; | ||
| sub uuid; | ||
| BEGIN | ||
| INSERT INTO rooms(id,title) VALUES (rid,'Verify Room') ON CONFLICT DO NOTHING; | ||
| INSERT INTO rounds(id,room_id,idx,phase,submit_deadline_unix,published_at_unix) | ||
| VALUES (r0,rid,0,'published',0, extract(epoch from now())::bigint) | ||
| ON CONFLICT DO NOTHING; | ||
| INSERT INTO participants(id,room_id,anon_name,role) | ||
| VALUES (author,rid,'author_v','debater') | ||
| ON CONFLICT DO NOTHING; | ||
| INSERT INTO participants(id,room_id,anon_name,role) | ||
| VALUES (judge,rid,'judge_v','judge') | ||
| ON CONFLICT DO NOTHING; | ||
| INSERT INTO submissions(round_id, author_id, content, canonical_sha256, client_nonce) | ||
| VALUES (r0,author,'Hello','aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa','n-ver-1') | ||
| RETURNING id INTO sub; | ||
| PERFORM ok(sub IS NOT NULL, 'seeded submission id'); | ||
| END $$; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Find the verify_submit function definition
fd -t f '\.sql' | head -20Repository: flyingrobots/db8
Length of output: 140
🏁 Script executed:
# Search for verify_submit function
rg "verify_submit" -A 5 --type sql | head -100Repository: flyingrobots/db8
Length of output: 358
🏁 Script executed:
# Look at the test file in context
fd 'verification.pgtap' --type fRepository: flyingrobots/db8
Length of output: 89
🏁 Script executed:
# Get the full verify_submit function definition
rg "verify_submit" -A 50 --type sql db/rpc.sqlRepository: flyingrobots/db8
Length of output: 1671
🏁 Script executed:
# Get the full test file to see what's already tested
cat -n db/test/44_verification.pgtapRepository: flyingrobots/db8
Length of output: 3714
🏁 Script executed:
# Verify the exact test count in plan(9)
rg "PERFORM ok|SELECT ok|SELECT throws_ok|SELECT results_eq" db/test/44_verification.pgtap | wc -lRepository: flyingrobots/db8
Length of output: 61
🏁 Script executed:
# Check if there are other verification tests elsewhere that might cover host role or final phase
fd '\.pgtap$' --type f | xargs rg "verify_submit|'host'|'final'" --type sql 2>/dev/null | head -20Repository: flyingrobots/db8
Length of output: 1468
Test coverage missing 'final' phase and 'host' role constraints enforced by verify_submit.
The function enforces phase IN ('published','final') and role IN ('judge','host') (confirmed in db/rpc.sql), but the current test only exercises published+judge. Per learning requirements, pgTAP invariants must cover boundary acceptance cases.
Add comprehensive phase×role matrix testing to cover all four combinations:
- published+judge (happy path—already covered)
- published+host (missing)
- final+judge (missing)
- final+host (missing)
This requires 3 additional test blocks after line 39, each seeding a new round with distinct phase/role and validating verify_submit succeeds or fails appropriately.
| -- verify_submit idempotency on (round, reporter, submission, claim) | ||
| DO $$ | ||
| DECLARE | ||
| r0 uuid := '20000000-0000-0000-0000-000000000002'; | ||
| judge uuid := '20000000-0000-0000-0000-000000000004'; | ||
| sub uuid := (SELECT id FROM submissions ORDER BY submitted_at DESC LIMIT 1); | ||
| id1 uuid; id2 uuid; id3 uuid; | ||
| BEGIN | ||
| SELECT verify_submit(r0, judge, sub, NULL, 'true', 'looks good', 'n1') INTO id1; | ||
| SELECT verify_submit(r0, judge, sub, NULL, 'true', 'still good', 'n1') INTO id2; | ||
| PERFORM ok(id1 = id2, 'verify_submit idempotent for same tuple (no claim)'); | ||
| -- Different claim_id should yield a different row | ||
| SELECT verify_submit(r0, judge, sub, 'c1', 'false', 'nope', 'n2') INTO id3; | ||
| PERFORM ok(id3 <> id1, 'verify_submit separate row for different claim'); | ||
| END $$; | ||
|
|
||
| -- Bounds/role checks | ||
| SELECT throws_ok( | ||
| $$SELECT verify_submit('20000000-0000-0000-0000-000000000002','20000000-0000-0000-0000-000000000003',(SELECT id FROM submissions LIMIT 1),NULL,'true',NULL,'n3')$$, | ||
| '42501', | ||
| 'reporter_role_denied' | ||
| ); | ||
|
|
||
| SELECT throws_ok( | ||
| $$SELECT verify_submit('20000000-0000-0000-0000-000000000002','20000000-0000-0000-0000-000000000004',(SELECT id FROM submissions LIMIT 1),NULL,'maybe',NULL,'n4')$$, | ||
| '22023', | ||
| 'invalid_verdict' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Insufficient boundary and error case coverage.
Current tests verify happy path, idempotency, role denial, and one invalid verdict. Missing critical boundary cases:
- NULL verdict: Should verify_submit reject NULL verdict or is it allowed?
- Empty string verdict: Is
''a valid verdict value? - Cross-room violation: Can a judge from Room A verify a submission in Room B?
- Non-existent submission_id: Does verify_submit gracefully handle invalid submission UUID?
- Non-published phase: Does verify_submit reject rounds in 'submit' or 'open' phase?
- Excessively long notes: DoS risk if notes field is unbounded.
Add boundary tests after line 68:
-- Test NULL verdict rejection
SELECT throws_ok(
$$SELECT verify_submit('20000000-0000-0000-0000-000000000002',
'20000000-0000-0000-0000-000000000004',
(SELECT id FROM submissions LIMIT 1),
NULL, NULL, NULL, 'n-null')$$,
'Verdict cannot be NULL'
);
-- Test non-existent submission
SELECT throws_ok(
$$SELECT verify_submit('20000000-0000-0000-0000-000000000002',
'20000000-0000-0000-0000-000000000004',
'00000000-0000-0000-0000-000000000000'::uuid,
NULL, 'true', NULL, 'n-noexist')$$,
'submission_not_found'
);🤖 Prompt for AI Agents
In db/test/44_verification.pgtap after line 68, add focused boundary tests
covering NULL verdict, empty-string verdict, cross-room judge/submission
mismatch, non-existent submission_id, verifies during non-published phases, and
excessively long notes: for each case call verify_submit within throws_ok (or ok
where appropriate) asserting the correct SQLSTATE or custom error message (e.g.,
'verdict_required' or 'invalid_verdict' for NULL/empty, 'submission_not_found'
for missing UUID, an authorization error code for cross-room or wrong-phase
attempts, and length/validation error for oversized notes). Ensure each test
uses explicit UUIDs or phase-setting fixtures so they target the intended
condition and label each assertion with a descriptive test name.
| -- Summary aggregates | ||
| SELECT results_eq( | ||
| $$ SELECT total::int FROM verify_summary('20000000-0000-0000-0000-000000000002'::uuid) WHERE claim_id IS NULL $$, | ||
| ARRAY[1::int], | ||
| 'summary counts one overall-verdict row' | ||
| ); | ||
|
|
||
| SELECT finish(); | ||
| ROLLBACK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weak summary aggregation test - only verifies row count.
The test only asserts total::int = 1 for the no-claim case. It does not verify:
- Claim-specific aggregation: After line 53 creates a verdict with
claim_id='c1', verify_summary should return a separate row for that claim. - Aggregate columns: The returned row should contain counts (e.g.,
true_count,false_count,unclear_count). This test doesn't verify those columns exist or are correct. - Multiple verdicts: Seed 2+ verdicts with different values ('true' and 'false') and verify counts aggregate correctly.
Replace the weak test with comprehensive aggregation verification:
-- Verify summary structure and counts
DO $$
DECLARE
r0 uuid := '20000000-0000-0000-0000-000000000002';
summary record;
BEGIN
-- Query summary for no-claim verdicts
SELECT * INTO summary FROM verify_summary(r0) WHERE claim_id IS NULL;
PERFORM ok(summary.total = 1, 'total count is 1 for no-claim');
PERFORM ok(summary.true_count = 1, 'true_count is 1');
PERFORM ok(summary.false_count = 0, 'false_count is 0');
-- Query summary for claim 'c1'
SELECT * INTO summary FROM verify_summary(r0) WHERE claim_id = 'c1';
PERFORM ok(summary.total = 1, 'total count is 1 for claim c1');
PERFORM ok(summary.false_count = 1, 'claim c1 has 1 false verdict');
END $$;🤖 Prompt for AI Agents
In db/test/44_verification.pgtap around lines 70 to 78, the current test only
asserts total::int = 1 for the no-claim row; replace it with a comprehensive
aggregation verification: seed at least two verdicts (including one for
claim_id='c1' and one no-claim), call verify_summary and assert rows exist for
both claim_id IS NULL and claim_id='c1', and explicitly check aggregate columns
(total, true_count, false_count, unclear_count as applicable) for correct values
for each row; implement these checks using pgtap ok/assert functions (or SELECT
results_eq with expected arrays) to validate both structure and counts.
| -- Reporter can always read their own (regardless of publish) | ||
| SET ROLE db8_reader; | ||
| SELECT set_config('db8.participant_id','21000000-0000-0000-0000-000000000004', false); | ||
| SELECT results_eq( | ||
| $$ SELECT count(*)::int FROM verification_verdicts $$, | ||
| ARRAY[1::int], | ||
| 'reporter sees their verdict row' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test claim "regardless of publish" is not verified.
Line 46 comment claims "Reporter can always read their own (regardless of publish)", but the seeded round on line 24 is already in 'published' phase. This test only verifies visibility in the published state, not pre-publish. To validate the "regardless of publish" claim, you'd need either:
- A separate seed with
phase='submit'orphase='final' - Or update the round phase after the first visibility check
🔎 Proposed fix to actually test pre-publish visibility
-- Seed minimal data
DO $$
DECLARE rid uuid := '21000000-0000-0000-0000-000000000001';
r0 uuid := '21000000-0000-0000-0000-000000000002';
a1 uuid := '21000000-0000-0000-0000-000000000003';
j1 uuid := '21000000-0000-0000-0000-000000000004';
sub uuid;
BEGIN
INSERT INTO rooms(id,title) VALUES (rid,'RLS Verify') ON CONFLICT DO NOTHING;
- INSERT INTO rounds(id,room_id,idx,phase,submit_deadline_unix,published_at_unix)
- VALUES (r0,rid,0,'published',0,extract(epoch from now())::bigint)
+ INSERT INTO rounds(id,room_id,idx,phase,submit_deadline_unix)
+ VALUES (r0,rid,0,'final',0)
ON CONFLICT DO NOTHING;
-- ... rest of seedThen add a pre-publish check before updating to published for the "others" test.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In db/test/45_verification_rls.pgtap around lines 46 to 53, the test claims
"Reporter can always read their own (regardless of publish)" but the seeded
round is already 'published', so it only verifies post-publish visibility;
update the test to actually assert visibility pre-publish by either seeding a
second round with phase='submit' or 'final' and repeating the reporter
visibility check against that seed, or modify the existing seed within the test
flow to set phase to a non-published state before asserting the reporter can see
their verdict, then restore or publish as needed for subsequent checks.
| await pool.query( | ||
| `insert into rooms (id, title) | ||
| values ('30000000-0000-0000-0000-000000000001', 'Verify Room PG') | ||
| on conflict (id) do nothing` | ||
| ); | ||
| await pool.query( | ||
| `insert into rounds (id, room_id, idx, phase, submit_deadline_unix, published_at_unix) | ||
| values ('30000000-0000-0000-0000-000000000002', '30000000-0000-0000-0000-000000000001', 0, 'published', 0, extract(epoch from now())::bigint) | ||
| on conflict (id) do nothing` | ||
| ); | ||
| await pool.query( | ||
| `insert into participants (id, room_id, anon_name, role) | ||
| values | ||
| ('30000000-0000-0000-0000-000000000003', '30000000-0000-0000-0000-000000000001', 'author', 'debater'), | ||
| ('30000000-0000-0000-0000-000000000004', '30000000-0000-0000-0000-000000000001', 'judge', 'judge') | ||
| on conflict (id) do nothing` | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Seed data uses UUID prefix 30000000-*—verify collision isolation.
The UUIDs here (30000000-0000-0000-0000-00000000000X) are distinct from other test files I've seen (10000000-*, 00000000-*), which is good. However, the ON CONFLICT DO NOTHING clauses mask potential test pollution if rows already exist from a failed prior run.
Consider adding explicit cleanup in beforeAll or using truly random UUIDs to guarantee isolation. If a previous test run crashed mid-flight, stale data could cause silent assertion failures.
🤖 Prompt for AI Agents
In server/test/rpc.db.verify.test.js around lines 37 to 53, the seed uses fixed
UUIDs with ON CONFLICT DO NOTHING which can hide stale rows from previous runs;
modify the test setup to guarantee isolation by either (A) explicitly deleting
any existing rows with those fixed IDs before inserting (run DELETE FROM
participants/rounds/rooms WHERE id IN (...)) in the beforeAll or immediately
before these inserts, or (B) switch to generating fresh UUIDs per test (e.g.,
assign uuid variables in beforeAll and use those variables in the insert
statements and subsequent assertions) so inserts cannot collide; apply one
approach and ensure subsequent test code references the same IDs/variables.
| beforeEach(async () => { | ||
| const tables = ['verification_verdicts', 'submissions']; | ||
| const existing = []; | ||
| for (const table of tables) { | ||
| const res = await pool.query('select to_regclass($1) as reg', [`public.${table}`]); | ||
| if (res.rows[0]?.reg) existing.push(`"public"."${table}"`); | ||
| } | ||
| if (existing.length > 0) { | ||
| await pool.query(`TRUNCATE ${existing.join(', ')} RESTART IDENTITY CASCADE;`); | ||
| // eslint-disable-next-line no-console | ||
| console.log('[truncate]', existing.join(', ')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
TRUNCATE logs to console—remove or guard behind DEBUG flag.
Line 71 unconditionally logs [truncate] to stdout. In a 83-test suite, this noise accumulates. Either remove the console.log or gate it behind process.env.DEBUG.
🔎 Proposed fix
if (existing.length > 0) {
await pool.query(`TRUNCATE ${existing.join(', ')} RESTART IDENTITY CASCADE;`);
- // eslint-disable-next-line no-console
- console.log('[truncate]', existing.join(', '));
+ if (process.env.DEBUG) {
+ // eslint-disable-next-line no-console
+ console.log('[truncate]', existing.join(', '));
+ }
}📝 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.
| beforeEach(async () => { | |
| const tables = ['verification_verdicts', 'submissions']; | |
| const existing = []; | |
| for (const table of tables) { | |
| const res = await pool.query('select to_regclass($1) as reg', [`public.${table}`]); | |
| if (res.rows[0]?.reg) existing.push(`"public"."${table}"`); | |
| } | |
| if (existing.length > 0) { | |
| await pool.query(`TRUNCATE ${existing.join(', ')} RESTART IDENTITY CASCADE;`); | |
| // eslint-disable-next-line no-console | |
| console.log('[truncate]', existing.join(', ')); | |
| } | |
| }); | |
| beforeEach(async () => { | |
| const tables = ['verification_verdicts', 'submissions']; | |
| const existing = []; | |
| for (const table of tables) { | |
| const res = await pool.query('select to_regclass($1) as reg', [`public.${table}`]); | |
| if (res.rows[0]?.reg) existing.push(`"public"."${table}"`); | |
| } | |
| if (existing.length > 0) { | |
| await pool.query(`TRUNCATE ${existing.join(', ')} RESTART IDENTITY CASCADE;`); | |
| if (process.env.DEBUG) { | |
| // eslint-disable-next-line no-console | |
| console.log('[truncate]', existing.join(', ')); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
In server/test/rpc.db.verify.test.js around lines 61 to 73 the beforeEach
truncation always prints a console.log('[truncate]', ...) which pollutes test
output; remove the unconditional console.log or wrap it in a DEBUG check (e.g.,
if (process.env.DEBUG) console.log(...)) so the message only appears when
debugging is enabled, ensuring test runs stay quiet by default.
| it('verify_submit stores and verify_summary aggregates', async () => { | ||
| // Seed a submission | ||
| const sub = await pool.query( | ||
| `insert into submissions (round_id, author_id, content, canonical_sha256, client_nonce) | ||
| values ('30000000-0000-0000-0000-000000000002','30000000-0000-0000-0000-000000000003','Hello','aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa','nonce') | ||
| returning id` | ||
| ); | ||
| const submission_id = sub.rows[0].id; | ||
|
|
||
| const body = { | ||
| round_id: '30000000-0000-0000-0000-000000000002', | ||
| reporter_id: '30000000-0000-0000-0000-000000000004', | ||
| submission_id, | ||
| verdict: 'true', | ||
| client_nonce: 'pg-ver-1' | ||
| }; | ||
| const first = await request(app).post('/rpc/verify.submit').send(body).expect(200); | ||
| const second = await request(app).post('/rpc/verify.submit').send(body).expect(200); | ||
| expect(second.body.id).toEqual(first.body.id); | ||
|
|
||
| const summary = await request(app) | ||
| .get('/verify/summary?round_id=30000000-0000-0000-0000-000000000002') | ||
| .expect(200); | ||
| const rows = summary.body.rows || []; | ||
| const overall = rows.find((r) => r.claim_id === null || r.claim_id === undefined); | ||
| expect(overall?.true_count).toBe(1); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Idempotency test is solid but lacks SHA256 content verification.
You verify that submitting twice returns the same id (line 93), which confirms idempotency. However, you never verify that the returned id is a valid UUID or that the stored verdict matches what was submitted. A malicious or buggy RPC could return {id: null} twice and this test would pass.
Add an assertion like:
expect(first.body.id).toMatch(/^[0-9a-f-]{36}$/);🤖 Prompt for AI Agents
In server/test/rpc.db.verify.test.js around lines 75 to 101, the idempotency
test only checks duplicate IDs but doesn't validate the id format or that the
stored verdict matches the submitted verdict; add an assertion that
first.body.id matches a UUID v4-ish pattern (36-char hex/dash) and add an
assertion that the stored verdict for that returned id equals the submitted
verdict (either by inspecting the RPC response payload fields or by querying the
database/summary row for that id) so the test fails if a null/invalid id or
wrong verdict is returned.
| it('rejects invalid submission_id', async () => { | ||
| const body = { | ||
| round_id: '30000000-0000-0000-0000-000000000002', | ||
| reporter_id: '30000000-0000-0000-0000-000000000004', | ||
| submission_id: '99999999-9999-9999-9999-999999999999', | ||
| verdict: 'true', | ||
| client_nonce: 'pg-ver-invalid' | ||
| }; | ||
| const res = await request(app).post('/rpc/verify.submit').send(body); | ||
| expect(res.status).toBeGreaterThanOrEqual(400); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Imprecise status assertion—be explicit about expected code.
toBeGreaterThanOrEqual(400) would pass for 500 (server error), which is semantically different from 400/404 (client error). If the RPC should return a specific code for "submission not found," assert it exactly.
🔎 Proposed fix
const res = await request(app).post('/rpc/verify.submit').send(body);
- expect(res.status).toBeGreaterThanOrEqual(400);
+ // Expect 404 for non-existent submission, or 400 for validation failure
+ expect([400, 404]).toContain(res.status);
+ expect(res.body.ok).toBe(false);📝 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.
| it('rejects invalid submission_id', async () => { | |
| const body = { | |
| round_id: '30000000-0000-0000-0000-000000000002', | |
| reporter_id: '30000000-0000-0000-0000-000000000004', | |
| submission_id: '99999999-9999-9999-9999-999999999999', | |
| verdict: 'true', | |
| client_nonce: 'pg-ver-invalid' | |
| }; | |
| const res = await request(app).post('/rpc/verify.submit').send(body); | |
| expect(res.status).toBeGreaterThanOrEqual(400); | |
| }); | |
| it('rejects invalid submission_id', async () => { | |
| const body = { | |
| round_id: '30000000-0000-0000-0000-000000000002', | |
| reporter_id: '30000000-0000-0000-0000-000000000004', | |
| submission_id: '99999999-9999-9999-9999-999999999999', | |
| verdict: 'true', | |
| client_nonce: 'pg-ver-invalid' | |
| }; | |
| const res = await request(app).post('/rpc/verify.submit').send(body); | |
| // Expect 404 for non-existent submission, or 400 for validation failure | |
| expect([400, 404]).toContain(res.status); | |
| expect(res.body.ok).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
In server/test/rpc.db.verify.test.js around lines 103 to 113, the test currently
uses a loose assertion (toBeGreaterThanOrEqual(400)) which will accept
500-series errors; change it to assert the exact expected HTTP status for a
missing submission (e.g., expect(res.status).toBe(404) or
expect(res.status).toBe(400) depending on the RPC spec), and optionally assert
the error message/code in the response body to ensure the RPC returns the
correct "submission not found" behavior.
| it('upserts a verdict idempotently by (round, reporter, submission, claim)', async () => { | ||
| // Create a submission first | ||
| // If server enforces issued nonces, obtain one for the author | ||
| const issued = await request(app) | ||
| .post('/rpc/nonce.issue') | ||
| .send({ round_id: ROUND_ID, author_id: AUTHOR_ID, ttl_sec: 60 }) | ||
| .then((r) => r.body) | ||
| .catch(() => ({ ok: false })); | ||
|
|
||
| const sub = { | ||
| room_id: ROOM_ID, | ||
| round_id: ROUND_ID, | ||
| author_id: AUTHOR_ID, | ||
| phase: 'submit', | ||
| deadline_unix: 0, | ||
| content: 'Verification target', | ||
| claims: [{ id: 'c1', text: 'Abc', support: [{ kind: 'logic', ref: 'a' }] }], | ||
| citations: [{ url: 'https://example.com/a' }, { url: 'https://example.com/b' }], | ||
| client_nonce: issued?.ok ? issued.nonce : 'nonce-sub-ver-1' | ||
| }; | ||
| const createRes = await request(app).post('/rpc/submission.create').send(sub); | ||
| // Debug if failing in CI/local | ||
| if (createRes.status !== 200) { | ||
| console.error('submission.create failed', createRes.status, createRes.body); | ||
| } | ||
| expect(createRes.status).toBe(200); | ||
| const submission_id = createRes.body.submission_id; | ||
|
|
||
| const payload = { | ||
| round_id: ROUND_ID, | ||
| reporter_id: REPORTER_ID, | ||
| submission_id, | ||
| verdict: 'true', | ||
| rationale: 'looks good', | ||
| client_nonce: 'ver-123456' | ||
| }; | ||
| const first = await request(app).post('/rpc/verify.submit').send(payload); | ||
| if (first.status !== 200) { | ||
| console.error('verify.submit first failed', first.status, first.body); | ||
| } | ||
| expect(first.status).toBe(200); | ||
| const second = await request(app).post('/rpc/verify.submit').send(payload).expect(200); | ||
| expect(first.body.ok).toBe(true); | ||
| expect(second.body.id).toEqual(first.body.id); | ||
|
|
||
| // Different claim should yield a different id | ||
| const third = await request(app) | ||
| .post('/rpc/verify.submit') | ||
| .send({ ...payload, claim_id: 'c1', client_nonce: 'ver-234567' }) | ||
| .expect(200); | ||
| expect(third.body.id).not.toEqual(first.body.id); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== 1. Find all verify-related test files ==="
fd -e js 'verify' server/test/
echo ""
echo "=== 2. Search for role enforcement patterns in verify flows ==="
rg -n '(judge|host|role)' server/test/*verify* --color=never 2>/dev/null || echo "No role references found"
echo ""
echo "=== 3. Check verify.submit endpoint implementation ==="
fd -e js 'verify' server/ | grep -v test | grep -v node_modules
echo ""
echo "=== 4. Search for judge/host role checks in server code ==="
rg -n '(judge|host)' server/ --type js -i --color=never 2>/dev/null | head -30Repository: flyingrobots/db8
Length of output: 4053
🏁 Script executed:
#!/bin/bash
echo "=== 5. Examine verify.submit endpoint ==="
fd -e js . server/ | xargs grep -l 'verify.submit\|verify_submit' | grep -v test | grep -v node_modules
echo ""
echo "=== 6. Check for role enforcement in endpoint definitions ==="
rg -n 'verify.*submit|submit.*verify' server/ -A 10 --type js --color=never 2>/dev/null | head -50
echo ""
echo "=== 7. Look for Zod schema validation on endpoints ==="
rg -n 'rpc.*verify\|verify.*rpc' server/ --type js -A 5 --color=never 2>/dev/null | head -40Repository: flyingrobots/db8
Length of output: 3379
🏁 Script executed:
#!/bin/bash
echo "=== 8. Check if DB constraints are mentioned in learnings context ==="
fd -e sql db/ 2>/dev/null | head -20
echo ""
echo "=== 9. Search for role/judge/host in SQL files ==="
rg -n '(judge|host|role)' db/ --type sql -i --color=never 2>/dev/null | head -20
echo ""
echo "=== 10. List test file structure in server/test ==="
ls -la server/test/*verify* 2>/dev/null || echo "No verify test files found"Repository: flyingrobots/db8
Length of output: 2593
🏁 Script executed:
#!/bin/bash
echo "=== 11. Check if there are separate unit vs integration test suites ==="
find server/test -name '*.test.js' -o -name '*.spec.js' | head -20
echo ""
echo "=== 12. Look at the test file in question more closely ==="
wc -l server/test/rpc.verify.submit.test.jsRepository: flyingrobots/db8
Length of output: 990
🏁 Script executed:
#!/bin/bash
echo "=== 13. Check describe blocks and test structure ==="
rg -n '^describe|^\s+it\(' server/test/rpc.verify.submit.test.js --color=neverRepository: flyingrobots/db8
Length of output: 450
🏁 Script executed:
#!/bin/bash
echo "=== Examine rpc.db.verify.test.js to see what's tested ==="
head -100 server/test/rpc.db.verify.test.js
echo ""
echo "=== Check if role enforcement tests exist in DB test ==="
rg -n 'role|judge|host' server/test/rpc.db.verify.test.js -C 2Repository: flyingrobots/db8
Length of output: 4984
🏁 Script executed:
#!/bin/bash
echo "=== Look at the full DB verify test structure ==="
cat server/test/rpc.db.verify.test.jsRepository: flyingrobots/db8
Length of output: 4720
🏁 Script executed:
#!/bin/bash
echo "=== Examine the verify_submit SQL function ==="
rg -n 'CREATE FUNCTION verify_submit' db/rpc.sql -A 80 --color=neverRepository: flyingrobots/db8
Length of output: 106
🏁 Script executed:
#!/bin/bash
echo "=== Search for verify_submit function definition ==="
rg -n 'verify_submit' db/rpc.sql --color=never | head -20
echo ""
echo "=== Get the full function ==="
rg -n 'verify_submit' db/rpc.sql -A 100 --color=neverRepository: flyingrobots/db8
Length of output: 3956
🏁 Script executed:
#!/bin/bash
echo "=== Check what role checks are in verify_submit ==="
cat db/rpc.sql | sed -n '/verify_submit/,/^CREATE FUNCTION/p' | head -150Repository: flyingrobots/db8
Length of output: 5030
Add negative test to DB-backed suite for role rejection.
The memory-path test correctly has no role enforcement (line 11 disables DB). However, rpc.db.verify.test.js must test that non-judge/non-host participants are rejected with 403. The SQL function enforces this at lines 677–678 (raising reporter_role_denied), but the DB test only covers happy-path with a judge. Add a case where a debater attempts verification and confirm it returns 403.
🤖 Prompt for AI Agents
In server/test/rpc.verify.submit.test.js around lines 14–65, add a negative
DB-backed test case after the happy-path verifies: reuse the created
submission_id but send a verify.submit request where the reporter is a
non-judge/non-host participant (e.g., a debater/author id used elsewhere in the
tests) with a unique client_nonce, and assert the response is HTTP 403 (and/or
contains the reporter_role_denied error). Ensure this case runs only in the
DB-backed suite (do not change the memory-path test) and is executed after the
submission is created so the DB enforces the role-rejection logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review continued from previous batch...
| -- M3: Verification verdicts (per-claim/per-submission) | ||
| -- Records fact-check style verdicts from reporters (judges/hosts) about a submission | ||
| CREATE TABLE IF NOT EXISTS verification_verdicts ( | ||
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| round_id uuid NOT NULL REFERENCES rounds(id) ON DELETE CASCADE, | ||
| submission_id uuid NOT NULL REFERENCES submissions(id) ON DELETE CASCADE, | ||
| reporter_id uuid NOT NULL REFERENCES participants(id) ON DELETE CASCADE, | ||
| claim_id text, | ||
| verdict text NOT NULL CHECK (verdict IN ('true','false','unclear','needs_work')), | ||
| rationale text, | ||
| client_nonce text, | ||
| created_at timestamptz NOT NULL DEFAULT now() | ||
| ); | ||
|
|
||
| -- Idempotency: include client_nonce to allow multiple rows for the same tuple when nonce differs | ||
| -- Drop legacy unique if present to avoid conflicts | ||
| DO $$ | ||
| BEGIN | ||
| IF EXISTS (SELECT 1 FROM pg_indexes WHERE schemaname='public' AND indexname='ux_verification_verdicts_unique') THEN | ||
| EXECUTE 'DROP INDEX IF EXISTS ux_verification_verdicts_unique'; | ||
| END IF; | ||
| END $$; | ||
|
|
||
| -- New uniqueness covers (round, reporter, submission, claim-coalesced, client_nonce) | ||
| CREATE UNIQUE INDEX IF NOT EXISTS ux_verification_verdicts_unique_nonce | ||
| ON verification_verdicts (round_id, reporter_id, submission_id, coalesce(claim_id, ''), (COALESCE(NULLIF(client_nonce, ''), ''))); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_verification_verdicts_round ON verification_verdicts (round_id); | ||
| CREATE INDEX IF NOT EXISTS idx_verification_verdicts_submission ON verification_verdicts (submission_id); | ||
| CREATE INDEX IF NOT EXISTS idx_verification_verdicts_reporter ON verification_verdicts (reporter_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNIQUE index allows duplicate verification verdicts with different nonces.
Lines 259-260: The UNIQUE index includes client_nonce:
CREATE UNIQUE INDEX ... ON verification_verdicts (
round_id, reporter_id, submission_id,
coalesce(claim_id, ''),
(COALESCE(NULLIF(client_nonce, ''), ''))
);This has the SAME CRITICAL BUG as final_votes and scores:
A reporter (judge/host) can submit multiple conflicting verdicts for the same submission+claim by using different client_nonce values. Each different nonce creates a new row.
The verify_summary RPC will then count ALL of them, inflating true_count, false_count, etc., making the verification system gameable.
The constraint should be:
CREATE UNIQUE INDEX IF NOT EXISTS ux_verification_verdicts_unique_no_nonce
ON verification_verdicts (
round_id,
reporter_id,
submission_id,
coalesce(claim_id, '')
);Remove client_nonce from the uniqueness check. The nonce is for idempotency (resubmitting the SAME verdict), not for allowing multiple verdicts.
🔎 Proposed fix
-CREATE UNIQUE INDEX IF NOT EXISTS ux_verification_verdicts_unique_nonce
- ON verification_verdicts (round_id, reporter_id, submission_id, coalesce(claim_id, ''), (COALESCE(NULLIF(client_nonce, ''), '')));
+CREATE UNIQUE INDEX IF NOT EXISTS ux_verification_verdicts_unique_no_nonce
+ ON verification_verdicts (round_id, reporter_id, submission_id, coalesce(claim_id, ''));And update the RPC verify_submit in db/rpc.sql to use:
ON CONFLICT (round_id, reporter_id, submission_id, coalesce(claim_id, ''))Based on learnings, submission verification must enforce judge/host roles and published/final round phases.
| @@ -0,0 +1,344 @@ | |||
| # Expert 001: Round 2 Response | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing YAML frontmatter violates coding guidelines.
Per coding guidelines: "Markdown files must include YAML frontmatter with lastUpdated (ISO date)".
This file starts directly with the H1 heading. Add frontmatter at the top with at minimum:
---
lastUpdated: 2025-12-20
---If this is a spec document (debate materials may qualify), also include tags: [spec] and the milestone string.
🔎 Proposed fix
+---
+lastUpdated: 2025-12-20
+tags: [spec]
+---
+
# Expert 001: Round 2 ResponseAs per coding guidelines, all Markdown files require YAML frontmatter with lastUpdated.
🤖 Prompt for AI Agents
In docs/debates/fixed-timestep-vs-event-driven/round-2/expert-001.md around
lines 1 to 1, the file is missing the required YAML frontmatter; add a
frontmatter block at the very top containing at minimum lastUpdated with an ISO
date (e.g. 2025-12-20). If this file is a spec/debate material also include
tags: [spec] and the appropriate milestone string in the frontmatter. Ensure the
block is the first thing in the file and uses valid YAML delimiters (---) so
linting and guidelines pass.
| const roundId = String(req.query.round_id || ''); | ||
| if (!/^[0-9a-f-]{8,}$/i.test(roundId)) | ||
| return res.status(400).json({ ok: false, error: 'invalid_round_id' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/verify/summary uses weak regex validation instead of Zod.
Line 848's regex /^[0-9a-f-]{8,}$/i accepts strings like "12345678" or "a-b-c-d-e-f-g-h" as valid round IDs. This is far too permissive for UUID validation.
Other endpoints use proper Zod UUID validation. This should be consistent.
🔎 Proposed fix
+const VerifySummaryIn = z.object({ round_id: z.string().uuid() });
+
app.get('/verify/summary', async (req, res) => {
try {
- const roundId = String(req.query.round_id || '');
- if (!/^[0-9a-f-]{8,}$/i.test(roundId))
- return res.status(400).json({ ok: false, error: 'invalid_round_id' });
+ const { round_id: roundId } = VerifySummaryIn.parse(req.query);🤖 Prompt for AI Agents
In server/rpc.js around lines 847 to 849, the endpoint uses a weak regex to
validate round_id; replace the regex check with the project's Zod UUID
validation (e.g., import { z } from 'zod' or use the shared zod schema/util used
elsewhere), parse/validate req.query.round_id via z.string().uuid().safeParse
(or equivalent), and if validation fails return res.status(400).json({ ok:
false, error: 'invalid_round_id' }); otherwise use the validated value; ensure
imports are added and behavior matches other endpoints.
| export const AuthVerifyIn = z.object({ | ||
| room_id: z.string().uuid(), | ||
| participant_id: z.string().uuid(), | ||
| nonce: z.string().min(8), | ||
| signature_kind: z.enum(['ed25519', 'ssh']), | ||
| sig_b64: z.string().min(1), | ||
| public_key_ssh: z.string().optional(), | ||
| public_key_b64: z.string().optional() | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
AuthVerifyIn: Consider validating public key requirements.
AuthVerifyIn has both public_key_ssh and public_key_b64 optional (lines 173-174).
If the backend doesn't have the public key stored (e.g., for first-time auth), the request MUST include one. If the key is always stored in participants.ssh_fingerprint, then these are truly optional.
Consider adding a refinement similar to ParticipantFingerprintSet (lines 98-108) to enforce:
- At least one key is provided when needed, OR
- Document that keys are optional because they're always fetched from DB
Without seeing the RPC handler, I can't confirm this is a bug, but the schema's intention is unclear.
🔎 Possible refinement if keys are required
export const AuthVerifyIn = z.object({
room_id: z.string().uuid(),
participant_id: z.string().uuid(),
nonce: z.string().min(8),
signature_kind: z.enum(['ed25519', 'ssh']),
sig_b64: z.string().min(1),
public_key_ssh: z.string().optional(),
public_key_b64: z.string().optional()
-});
+}).refine(
+ v => v.public_key_ssh || v.public_key_b64,
+ { message: 'public_key_ssh_or_public_key_b64_required', path: ['public_key_ssh'] }
+);As per coding guidelines, use Zod schemas at the edges for request validation.
🤖 Prompt for AI Agents
In server/schemas.js around lines 167-175, AuthVerifyIn currently makes
public_key_ssh and public_key_b64 optional which leaves intent unclear; either
enforce that at least one key is provided when the backend doesn't already store
the participant's key, or document that keys are optional because they are
always retrieved from the DB. Fix by adding a Zod refinement (like
ParticipantFingerprintSet) that checks (public_key_ssh || public_key_b64) is
present when needed, returning a clear error message, or alternatively update
the schema comment and handler to assert the key is always available server-side
so both fields can remain optional.
| async function onVerifySubmit(e) { | ||
| e.preventDefault(); | ||
| if (!verifying) return; | ||
| const form = new window.FormData(e.target); | ||
| const verdict = form.get('verdict'); | ||
| const rationale = form.get('rationale'); | ||
| const claim_id = form.get('claim_id'); | ||
| setActionBusy(true); | ||
| try { | ||
| const clientNonce = lastNonceRef.current || String(Date.now()); // simplified | ||
| const payload = { | ||
| round_id: '00000000-0000-0000-0000-000000000002', // Ideally from state.round.round_id | ||
| reporter_id: participant, | ||
| submission_id: verifying.submission_id, | ||
| verdict, | ||
| rationale, | ||
| claim_id: claim_id || undefined, | ||
| client_nonce: clientNonce | ||
| }; | ||
| const r = await fetch(`${apiBase()}/rpc/verify.submit`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| ...(jwt ? { authorization: `Bearer ${jwt}` } : {}) | ||
| }, | ||
| body: JSON.stringify(payload) | ||
| }); | ||
| if (r.ok) { | ||
| setVerifying(null); | ||
| // Trigger verification refresh logic here if possible, | ||
| // effectively handled by the polling effect eventually | ||
| } else { | ||
| window.alert('Verify failed'); | ||
| } | ||
| } catch (err) { | ||
| window.alert(String(err)); | ||
| } finally { | ||
| setActionBusy(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing AbortController violates coding guidelines.
Per coding guidelines: "Web/React components must use AbortController for async operations to prevent setState after unmount."
The fetch on line 465 can complete after the component unmounts, causing setVerifying(null) and setActionBusy(false) to fire on an unmounted component.
🔎 Proposed fix
async function onVerifySubmit(e) {
e.preventDefault();
if (!verifying) return;
+ const controller = new AbortController();
const form = new window.FormData(e.target);
const verdict = form.get('verdict');
const rationale = form.get('rationale');
const claim_id = form.get('claim_id');
setActionBusy(true);
try {
const clientNonce = lastNonceRef.current || String(Date.now());
const payload = { /* ... */ };
const r = await fetch(`${apiBase()}/rpc/verify.submit`, {
method: 'POST',
headers: { /* ... */ },
- body: JSON.stringify(payload)
+ body: JSON.stringify(payload),
+ signal: controller.signal
});
- if (r.ok) {
+ if (!controller.signal.aborted && r.ok) {
setVerifying(null);
} else {
- window.alert('Verify failed');
+ if (!controller.signal.aborted) window.alert('Verify failed');
}
} catch (err) {
- window.alert(String(err));
+ if (err.name !== 'AbortError') window.alert(String(err));
} finally {
- setActionBusy(false);
+ if (!controller.signal.aborted) setActionBusy(false);
}
}As per coding guidelines, AbortController is required for async operations in React components.
🤖 Prompt for AI Agents
In web/app/room/[roomId]/page.jsx around lines 446-485, the fetch in
onVerifySubmit lacks an AbortController which can cause setState after unmount;
create an AbortController (store it on a ref so the component can access it),
pass controller.signal to fetch, and before calling any state setters
(setVerifying, setActionBusy) check that !controller.signal.aborted; also add a
useEffect cleanup that aborts the current controller on unmount (and clear the
ref) to ensure the pending request is cancelled and no state updates run after
unmount.
| async function onFlagSubmit(e) { | ||
| e.preventDefault(); | ||
| if (!flagging) return; | ||
| const form = new window.FormData(e.target); | ||
| const reason = form.get('reason'); | ||
| setActionBusy(true); | ||
| try { | ||
| const payload = { | ||
| submission_id: flagging.submission_id, | ||
| reporter_id: participant, | ||
| reporter_role: role || 'participant', | ||
| reason | ||
| }; | ||
| const r = await fetch(`${apiBase()}/rpc/submission.flag`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| ...(jwt ? { authorization: `Bearer ${jwt}` } : {}) | ||
| }, | ||
| body: JSON.stringify(payload) | ||
| }); | ||
| if (r.ok) { | ||
| setFlagging(null); | ||
| // Ideally trigger state refresh to update flag counts | ||
| } else { | ||
| window.alert('Flag failed'); | ||
| } | ||
| } catch (err) { | ||
| window.alert(String(err)); | ||
| } finally { | ||
| setActionBusy(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing AbortController violates coding guidelines.
Same issue as onVerifySubmit: fetch on line 500 without AbortController can cause setState on unmounted component (setFlagging(null) line 509, setActionBusy(false) line 517).
Apply the same AbortController pattern as proposed for onVerifySubmit.
As per coding guidelines, AbortController is required for async operations in React components.
| {verifying && ( | ||
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center p-4 z-50"> | ||
| <Card className="w-full max-w-md"> | ||
| <CardContent className="p-6 space-y-4"> | ||
| <h3 className="text-lg font-semibold">Verify Submission</h3> | ||
| <p className="text-xs font-mono text-muted-foreground break-all"> | ||
| {verifying.submission_id} | ||
| </p> | ||
| <form onSubmit={onVerifySubmit} className="space-y-4"> | ||
| <div> | ||
| <label className="text-sm font-medium">Claim (optional)</label> | ||
| <select name="claim_id" className="w-full mt-1 border rounded p-2 bg-background"> | ||
| <option value="">Full Submission</option> | ||
| {(verifying.claims || []).map((c) => ( | ||
| <option key={c.id} value={c.id}> | ||
| {c.id}: {c.text.slice(0, 30)}... | ||
| </option> | ||
| ))} | ||
| </select> | ||
| </div> | ||
| <div> | ||
| <label className="text-sm font-medium">Verdict</label> | ||
| <select name="verdict" className="w-full mt-1 border rounded p-2 bg-background"> | ||
| <option value="true">True</option> | ||
| <option value="false">False</option> | ||
| <option value="unclear">Unclear</option> | ||
| <option value="needs_work">Needs Work</option> | ||
| </select> | ||
| </div> | ||
| <div> | ||
| <label className="text-sm font-medium">Rationale</label> | ||
| <textarea | ||
| name="rationale" | ||
| required | ||
| className="w-full mt-1 border rounded p-2 bg-background min-h-[100px]" | ||
| placeholder="Explain your verdict..." | ||
| /> | ||
| </div> | ||
| <div className="flex justify-end gap-2"> | ||
| <Button type="button" variant="ghost" onClick={() => setVerifying(null)}> | ||
| Cancel | ||
| </Button> | ||
| <Button type="submit" disabled={actionBusy}> | ||
| {actionBusy ? 'Saving...' : 'Submit Verdict'} | ||
| </Button> | ||
| </div> | ||
| </form> | ||
| </CardContent> | ||
| </Card> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modal lacks accessibility attributes and keyboard support.
This verify modal has multiple accessibility issues:
- Missing
role="dialog"andaria-modal="true" - No Escape key handler to dismiss
- No focus trap (users can Tab out)
- Labels on lines 766, 777, 786 missing
htmlForattributes (static analysis is correct)
For production, implement:
- Add
role="dialog"andaria-modal="true"to outer div - Add
aria-labelledbypointing to the h3 on line 760 - Add
onKeyDown={(e) => e.key === 'Escape' && setVerifying(null)} - Add
htmlForto all labels or nest inputs inside labels - Consider focus trap library (e.g.,
focus-trap-react)
🔎 Quick fix for labels
- <label className="text-sm font-medium">Claim (optional)</label>
- <select name="claim_id" className="w-full mt-1 border rounded p-2 bg-background">
+ <label htmlFor="verify-claim" className="text-sm font-medium">Claim (optional)</label>
+ <select id="verify-claim" name="claim_id" className="w-full mt-1 border rounded p-2 bg-background">🧰 Tools
🪛 Biome (2.1.2)
[error] 766-767: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 777-778: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 786-787: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
🤖 Prompt for AI Agents
In web/app/room/[roomId]/page.jsx around lines 756 to 805, the verify modal is
missing accessibility and keyboard support: add role="dialog" and
aria-modal="true" to the outer fixed div, give the h3 a unique id and add
aria-labelledby on the dialog pointing to that id, add an onKeyDown handler on
the dialog that closes the modal on Escape (e.g., call setVerifying(null) when
e.key === 'Escape'), ensure every form control has a matching id and update each
label to include htmlFor pointing to that id (or wrap inputs in their labels)
for the Claim/select, Verdict/select, and Rationale/textarea, and optionally
wrap the modal content with a focus trap (e.g., focus-trap-react) to prevent
tabbing out while open.
| {flagging && ( | ||
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center p-4 z-50"> | ||
| <Card className="w-full max-w-md"> | ||
| <CardContent className="p-6 space-y-4"> | ||
| <h3 className="text-lg font-semibold">Flag Submission</h3> | ||
| <p className="text-xs font-mono text-muted-foreground break-all"> | ||
| {flagging.submission_id} | ||
| </p> | ||
| <form onSubmit={onFlagSubmit} className="space-y-4"> | ||
| <div> | ||
| <label className="text-sm font-medium">Reason</label> | ||
| <textarea | ||
| name="reason" | ||
| required | ||
| className="w-full mt-1 border rounded p-2 bg-background min-h-[80px]" | ||
| placeholder="Why are you flagging this?" | ||
| /> | ||
| </div> | ||
| <div className="flex justify-end gap-2"> | ||
| <Button type="button" variant="ghost" onClick={() => setFlagging(null)}> | ||
| Cancel | ||
| </Button> | ||
| <Button type="submit" variant="destructive" disabled={actionBusy}> | ||
| {actionBusy ? 'Flagging...' : 'Flag'} | ||
| </Button> | ||
| </div> | ||
| </form> | ||
| </CardContent> | ||
| </Card> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag modal has same accessibility issues.
Identical problems as the verify modal:
- Missing
role="dialog",aria-modal="true",aria-labelledby - No Escape key handler
- No focus trap
- Label on line 818 missing
htmlFor
Apply the same accessibility fixes as proposed for the verify modal.
🧰 Tools
🪛 Biome (2.1.2)
[error] 818-819: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
🤖 Prompt for AI Agents
In web/app/room/[roomId]/page.jsx around lines 808 to 838, the flag modal lacks
accessibility features: add role="dialog", aria-modal="true", and
aria-labelledby that points to the modal title id; ensure the title element (h3)
has that id; update the label for the textarea to include htmlFor matching the
textarea id; implement keyboard handling to close the modal on Escape (add a
keydown listener when flagging opens and remove it on close) and add a focus
trap (move initial focus into the first focusable element when opened and
restrict/tabloop focus inside the modal until closed, restoring focus to the
previously focused element on close); ensure Cancel and Flag buttons remain
reachable and that the dialog container has the fixed overlay but the dialog
element carries the ARIA attributes and focus management.
This PR completes Milestone 5 of the db8 roadmap:
M5: Scoring & Reputation
scorestable andscore_submitRPC for E/R/C/V/Y evaluation.reputation_update_roundfunction for deterministic global and tag-specific Elo updates.view_score_aggregateswith security-barrier hardening and weighted composite scoring./rpcendpoints with numeric coercion for accuracy.All 83 tests passing.