Action surface: self-verifying proof footer + tier-gated bounce reporting; gemini-3.5-flash; DEBATE off - #41
Conversation
…ng; gemini-3.5-flash; DEBATE off
- /s/[slug] action surface emits a commons.email/v/{credentialHash} verify URL in the proof footer only when the hash resolves (new auth-scoped users.getActiveCredentialHash — returns only the caller's own hash, no userId->hash->district enumeration oracle)
- action cards (DecisionMakerLandscapeCard/DistrictOfficialCard/PowerLandscape/RoleGroup): gate the bounce-report affordance to address-verified (tier 2+) users
- /s/[slug] server+page refactor (net smaller)
- gemini-client: pin gemini-3.5-flash (3-flash-preview ran away under MAX_TOKENS on subject-line calls) + thinkingLevel low; gemini-provider test updated to match
- features.ts: DEBATE off
- template-browser preview + emailService + subject-line tweaks
Deploying communique-site with
|
| Latest commit: |
f50f1d4
|
| Status: | 🚫 Build failed. |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR introduces a ChangesCredential Hash Feature
Bounce Reporting Refactor
Gemini Config and Feature Flag Updates
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (+page.svelte)
participant PageServer as +page.server.ts
participant Convex as convex/users.ts (getActiveCredentialHash)
participant PowerLandscape as PowerLandscape
participant RoleGroup as RoleGroup
participant Card as DistrictOfficialCard / DecisionMakerLandscapeCard
PageServer->>Convex: getActiveCredentialHash({ userId })
Convex-->>PageServer: credentialHash | null
PageServer-->>Browser: user { credentialHash, trust_tier, ... }
Browser->>Browser: canReportBounce = trust_tier >= 2
Browser->>PowerLandscape: canReportBounce, reportedBounces, reportingBounce, onReportBounce
PowerLandscape->>RoleGroup: same bounce props
RoleGroup->>Card: canReportBounce, reported, reporting, onReportBounce
Card-->>Browser: onReportBounce(email) on button click
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🪓 Brutalist Review
Both critics agree that the session-scoped bounce reporting is the headline problem: the "didn't arrive?" affordance is gated behind contacted state that evaporates on every reload, making the feature unreachable for its most common use-case (a user returns hours later after seeing a bounce). The credential-hash fix itself is correct — replacing 404-guaranteed truncated user IDs is the right call — but both critics flag that failure is now silent (.catch(() => null) is indistinguishable from "no credential"). Claude focuses on CSS scope escape and component duplication accumulating as tech debt; Codex emphasizes the operational risk of leaving maxOutputTokens: 65536 unchanged when that same ceiling caused the original runaway incident and could do so again under a prompt or schema regression. The Codex claim of a duplicate class attribute in DistrictOfficialCard.svelte was wrong — the actual file is clean — and was dropped.
Inline comments: 7 (1 🟠 high · 6 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 146448ms)
7 findings across security, performance, correctness, and maintainability. Headlined the session-scoped bounce reporting regression and the :global(.group:hover) CSS scope escape. Strong focus on the duplication accumulating across the two card components and the credential hash now living in page source. All claims verified against actual files.
✅ Codex (default, 126850ms)
6 findings after dropping 1 false positive (fabricated duplicate class attribute in DistrictOfficialCard that doesn't exist in the actual file). Valid catches: maxOutputTokens ceiling unchanged, silent credential hash failure, proof-link policy duplication, and DEBATE as a source-code kill switch. Corroborates Claude on the session-scoped bounce UX regression from the server-load angle.
❌ agy (default, 900010ms)
Timed out after 900s. No findings contributed.
Out-of-diff findings (6)
correctness
- 🟠 high
src/lib/components/action/DecisionMakerLandscapeCard.svelte— Claude [unanchored]: Bounce affordance gated on session-onlycontactedstate — invisible after any reload - 🔵 low
src/lib/components/action/DecisionMakerLandscapeCard.svelte— Claude [sub-threshold]: :global(.group:hover) .bounce-flag matches any .group ancestor, not just the immediate card wrapper - 🔵 low
src/routes/s/[slug]/+page.svelte— Claude [sub-threshold]: canReportBounce is stale: trust_tier from server load doesn't update reactively mid-session - 🔵 low
src/routes/s/[slug]/+page.server.ts— Claude [sub-threshold]: userId cast as Id<'users'> without non-empty validation before the Convex call
perf
- 🔵 low
src/routes/s/[slug]/+page.server.ts— Claude [sub-threshold]: getActiveCredentialHash placed in Batch 2 but depends only on locals — adds unnecessary serial latency
testing
- 🔵 low
tests/unit/agents/gemini-provider.test.ts— Codex [sub-threshold]: Updated tests only assert the new constant values — the regression they guard against (runaway output) is untested
Brutalist orchestrator schemaVersion=1 · context_id=6b42a854-7662-4e68-b7c3-d5d54539fa51
| })(), | ||
|
|
||
| // Batch 2: Queries depending on Batch 1 results. | ||
| // Delivery records are intentionally NOT loaded here: a mailto handoff is not |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Codex 🟠 high] correctness — Deliberate removal of delivery persistence makes bounce reporting unreachable after reload
The PR removes deliveredRecipients from the server load and the corresponding client-side restoration of contactedRecipients. The justification — a mailto handoff is not a confirmed send — is correct in principle, but the operational consequence is that bounce data will be systematically undercounted. Bounce discovery is inherently async: the user sends, receives a bounce reply, and returns to report it. Every return visit starts with contactedRecipients = new Set(), so the per-recipient 'didn't arrive?' button is invisible. The old aggregate bounce list below the landscape was a lower-fidelity UX, but it was reachable across sessions. The new per-recipient affordance is better UX when reachable, but unreachable is worse than imperfect. Recommend either restoring some form of delivery state (even session-storage) or adding a dedicated out-of-band bounce-report path that doesn't depend on transient contacted state.
| // Delivery records are intentionally NOT loaded here: a mailto handoff is not | |
| // Minimal sessionStorage approach in page.svelte $effect: | |
| // onMount(() => { | |
| // const stored = sessionStorage.getItem(`contacted:${templateId}`); | |
| // if (stored) contactedRecipients = new Set(JSON.parse(stored)); | |
| // }); | |
| // after handleWriteTo: sessionStorage.setItem(`contacted:${templateId}`, JSON.stringify([...contactedRecipients])); |
|
|
||
| // A bounce only means something for a direct email route; CWC/form delivery | ||
| // has no email to bounce. Tier gate is enforced by the parent (server too). | ||
| const canFlagBounce = $derived( |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] maintainability — Bounce logic duplicated verbatim in DecisionMakerLandscapeCard and DistrictOfficialCard
~80 lines of identical code now exist in both card components: the four-prop signature (canReportBounce, reported, reporting, onReportBounce), the canFlagBounce derived expression, the reportBounce() function, the reported/unreported template block, and the entire .bounce-flag CSS block with its @media (hover: hover) reveal rules and :disabled override. This is the same duplication pattern that already made the existing showRoleBadge, departing, and contactedRecipients logic hard to maintain across these two files — every feature addition has to be applied twice. The next change in this area will either introduce a regression in one component or force the shared-component refactor. Extracting a <BounceFlag> Svelte component would reduce this to a single pass-through prop each.
| const canFlagBounce = $derived( | |
| // Extract to src/lib/components/action/BounceFlag.svelte: | |
| // <script lang="ts"> | |
| // let { reported, reporting, onReportBounce }: { | |
| // reported: boolean; reporting: boolean; | |
| // onReportBounce: () => void; | |
| // } = $props(); | |
| // </script> | |
| // then in each card: <BounceFlag {reported} {reporting} onReportBounce={() => reportBounce(e)} /> |
| export const FEATURES = { | ||
| /** Deliberation surfaces, argument submission, LMSR market, resolution/appeal */ | ||
| DEBATE: true, | ||
| DEBATE: false, |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] maintainability — DEBATE: false is a hard source-code kill switch with no env override — rollback requires a deploy
Flipping DEBATE to false disables deliberation surfaces, argument submission, LMSR market, and resolution/appeal globally in every environment that picks up this build. There is no environment-variable override shown, so reverting the kill requires another full code deploy. The surrounding infrastructure (DebateRow types, debate route branches, load-path debate queries) still exists and still runs through the page load function — it just doesn't render. If the intent is a temporary gate, an env override like VITE_ENABLE_DEBATE !== 'false' would allow toggling without a redeploy. If it is a permanent removal, the dead code should be cleaned up to avoid maintaining it.
| model: 'gemini-3.5-flash', | ||
| defaults: { | ||
| temperature: 0.3, | ||
| maxOutputTokens: 65536, |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] perf — maxOutputTokens: 65536 unchanged — the 200s runaway ceiling remains in place for all agent calls
The comment above this config documents exactly the failure mode: the prior model 'generated to the 65k output cap (~64k tokens, ~200s, MAX_TOKENS truncation) on every subject-line call.' The fix switches model and lowers thinkingLevel — both good mitigations — but leaves the system-wide output cap at 65 536 tokens. A subject-line agent producing a JSON object with five fields has no legitimate need for that headroom. If the model, prompt, or schema enforcement regresses (including on a future Gemini rollout behind the pinned version), the full 200-second blast radius is still available. The call site in subject-line.ts should pass a per-call maxOutputTokens of 1 000–2 000 to cap the damage, leaving the default ceiling for agents that genuinely need it.
| footer += `\n${proofLine}`; | ||
| // Only emit the verify URL when it resolves: the active credential | ||
| // hash is the record /v/[hash] looks up. A truncated user id 404s. | ||
| if (user?.credentialHash) { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] maintainability — Proof-link policy ('only credentialHash may produce /v/[hash]') duplicated across three rendering paths
The rule 'only emit commons.email/v/[hash] when a resolved credentialHash exists' is now enforced independently in emailService.ts (line 318), PreviewContent.svelte (line 76), and +page.svelte (line 71). The previous truncated-user-id bug was precisely this kind of drift — each rendering path had its own implementation of the same policy and they diverged. A shared helper (e.g., getProofUrl(user): string | null) would make the invariant auditable in one place and prevent the next silent divergence.
| userId | ||
| ? serverQuery(api.users.getActiveCredentialHash, { userId: userId as Id<'users'> }) | ||
| .then((r) => r?.credentialHash ?? null) | ||
| .catch(() => null) |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — Credential hash lookup swallows all errors silently — outage is indistinguishable from 'no credential'
Every error path from getActiveCredentialHash — Convex outage, deployment mismatch, auth token problem, schema regression — collapses into null, which the downstream renderers interpret as 'user has no active credential; render no proof link.' The user still sends the email, but the verification attestation silently disappears. There is no monitoring signal distinguishing 'zero users have credentials' (valid) from 'the credential lookup is broken' (incident). At minimum, log the error before returning null so the failure is observable: catch((err) => { console.error('credential hash lookup failed', err); return null; }).
| .catch(() => null) | |
| .catch((err) => { | |
| console.error('[page.server] getActiveCredentialHash failed:', err); | |
| return null; | |
| }) |
| trust_tier: locals.user.trust_tier, | ||
| is_verified: locals.user.is_verified | ||
| is_verified: locals.user.is_verified, | ||
| credentialHash |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] security — Full 64-char credentialHash serialized into SvelteKit page payload / /__data.json
The complete credential hash is now placed in the server return payload and therefore written into the page HTML source and queryable unauthenticated at /__data.json. The doc comment in convex/users.ts argues it's 'public by design' — it already appears in email footers and resolveCredentialHash is unauthenticated. That's defensible, but the old code never put this value in the page source; it put 8 chars of a user ID. Any unauthenticated observer of the rendered page (CDN edge cache, browser history, shared device, network inspector) now has the full token needed to hit /v/[hash] and confirm 'this user has an active credential in this district.' Verify that resolveCredentialHash returns nothing beyond what is already public, and confirm you are comfortable with the full hash appearing in CDN caches.
1 participant
Recovers + melds the pre-spectrum-sweep WIP (set aside on 2026-06-14, preserved in a stash) onto current
main(b226693, post-spectrum #40). Clean meld: 0 file conflicts (main never touched these 13 files since the stash base), convex tsc 0, svelte-check 0 errors, 4,802 tests pass — DEBATE:false broke nothing (debate tests are flag-aware).What it adds
/s/[slug]action surface — emitscommons.email/v/{credentialHash}only when the hash resolves, backed by a new auth-scopedusers.getActiveCredentialHash(returns only the caller's own hash — no userId→hash→district enumeration oracle).DecisionMakerLandscapeCard/DistrictOfficialCard/PowerLandscape/RoleGroup) — affordance only for address-verified (tier 2+) users./s/[slug]refactor — server + page, net smaller.gemini-3.5-flash— pinned (the priorgemini-3-flash-previewran away under MAX_TOKENS on subject-line calls),thinkingLevel: low; thegemini-providertest updated to match (this is the source of the "2 gemini WIP-collateral" failures that shadowed every prior PR this session — now resolved).features.ts:DEBATE: false— intentional, per your direction to bring the WIP in including the flag.Evidence
0 conflicts on apply · convex tsc 0 · svelte-check 0 errors · vitest 4,802 passed (the prior 2 gemini failures fixed; regenerated
convex/_generated/api.d.tsfrom the merged function set).Follow-up flagged (not in this PR — a decision)
DEBATE off makes the strategy corpus's "Adversarial Quality (Debate Markets)" a dormant/flag-gated capability (like CONGRESSIONAL), not a live one. Whether that's a launch-gate or a deprecation is a call — the docs may want a caveat once decided.
Summary by CodeRabbit
New Features
Improvements
Chores