Code review round 1 — GPT-6 via Codex, 2026-09-09

Reviewer: GPT-6 (Codex), read-only sandbox. Pinned to cae60ed; the checkout advanced during the review, so C1-5 and C1-6 were already fixed on the branch by the time it landed. Fixes applied on fix/codex-code-r1.

The review, verbatim

Review pinned to cae60ed. The checkout advanced during review; later changes are excluded. No files modified. Tests were inspected, not executed in the read-only sandbox.

C1-1 — P1 — internal/calls/store.go:55, internal/calls/auth.go:23: Outsider addresses persist after verification.

ensurePerson writes every verified outsider’s plaintext address into persons.email and its local part into persons.name. Auth leaves SubjectFor unset, so rastrillo also writes the plaintext address into sessions.subject. Joining guest_grants to persons identifies outsiders by meeting. Fingerprinting pending_joins does not protect these copies.

This follows §2.1’s person model but contradicts the stated outsider-address privacy guarantee; the conflict is real, not resolved by sealing pending rows.

Correction: use an opaque session subject, a keyed address lookup, and sealed address/name fields. Resolve the conflicting model contract explicitly and test the complete outsider journey against database and WAL bytes.

C1-2 — P1 — internal/calls/auth.go:27, cmd/calls/main.go:30: Production error logging exposes outsider addresses.

Rastrillo’s mail/mail.go:163 wraps every SMTP delivery failure as send to <address>: <error>. Its auth/handlers.go:51 logs that error through Meet’s logger. An outsider requesting a link during an SMTP outage therefore leaves their address in logs before ever signing in.

Separately, db.Open enables GORM warning/slow-query logging without ParameterizedQueries. Slow or failed ensurePerson queries can log interpolated addresses.

Correction: sanitize mail errors before returning them to auth, and configure database logging to exclude parameter values. Add captured-log tests for failed SMTP delivery and slow/failed person queries.

C1-3 — P1 — internal/calls/handlers_join.go:13, internal/calls/handlers_join.go:36: A replaced link can mint a new grant.

The signed-in path checks the link outside the grant transaction:

1. A signed-in outsider requests link A; liveLink returns it as live.
2. A member replaces A with B and commits.
3. The outsider’s request calls mintGrant using the previously loaded meeting.

mintGrant never checks A. The resulting grant remains usable because standing checks the grant, not its originating link. SQLite’s single writer does not serialize these separate reads and writes into one operation.

Correction: resolve the originating link, check revocation and meeting state, and mint the grant within one transaction. Add a controlled concurrent replacement test.

C1-4 — P2 — internal/calls/auth.go:223, internal/calls/store.go:304: Continuation trusts a pending row that may already have been consumed.

latestPending runs before the transaction. consumePending deletes rows but never verifies that its supplied pend still exists or remains the newest row.

For example, request R1 reads pending A and pauses. A newer pending B is written and consumed by R2, deleting both rows. R1 then resumes and can mint a grant for A from its stale Go value, despite no pending row remaining. The unique grant index only prevents duplicates for the same meeting.

Correction: select the current pending row inside the same transaction that validates, grants and deletes. Test overlapping continuations involving different meetings.

C1-5 — P2 — cmd/calls/main.go:21: The two flag parsers make supported launch modes fail.

The global parser knows only -env, so meet -addr :8080 and activation with --socket … --db … exit before rastrillo.Resolve.

Conversely, meet -env development passes the first parser but fails in Resolve, which reparses the unchanged os.Args and recognizes only socket, addr and db. The advertised development mode cannot start.

Correction: establish one coordinated argument parser, or remove app-specific arguments before passing the remainder to the framework. Test actual executable invocations for development, TCP, socket activation and serve.

C1-6 — P2 — cmd/calls/main.go:38: Platform mail configuration is ignored.

At cae60ed, only CALLS_SMTP_HOST and CALLS_SMTP_FROM create a mailer, and mail.FromEnv("MEET", …) reads only that prefix. With only the platform-delivered CARLOS_SMTP_* variables, production refuses startup.

Meet cannot send using carlos email enable configuration alone. The existing binary can send without a code change if an operator separately supplies matching CALLS_SMTP_* variables.

Correction: add per-setting CARLOS fallbacks while retaining explicit Meet overrides. Trim HOST/FROM before testing presence: whitespace currently passes the outer check but makes FromEnv return a logging mailer, which production accepts as non-nil.

C1-7 — P2 — internal/calls/store.go:278, internal/calls/app.go:69: Expired sign-in records are never swept.

Expired pending rows are excluded from reads, not deleted. Only consumption deletes them. Meet also never calls auth.Sweep; rastrillo deletes an auth_links row when its token is presented, otherwise only through that explicit sweep.

An unclicked or unsuccessfully delivered magic link therefore leaves its plaintext outsider address in auth_links indefinitely, beyond its fifteen-minute validity. Abandoned pending rows and expired sessions also accumulate.

Correction: run bounded expiry cleanup at boot and periodically. Test unclicked links and abandoned pending rows. Deletion does not erase historical WAL or backup copies; sealing the auth link store is required for that stronger guarantee.

C1-8 — P2 — internal/callstest/join_test.go:131, internal/callstest/holding_test.go:22: Required gates remain missing or pass for the wrong reason.

The supposedly shared-limiter test sends six requests for one address. It proves the five-per-address limit, not exhaustion of the twenty-per-client allowance. It would pass if the client limiter disappeared.

The no-standing test requests a nonexistent meeting, so it passes before standingFor runs. The replaced-link test’s final signed-out request cannot prove that no grant row exists. The database privacy test silently skips every file-read error, allowing a vacuous pass.

Correction: assert the intended state directly, exercise existing meetings, fail unexpected file-read errors, and complete this §13 coverage:

| Slice 1 gate | Coverage at cae60ed | |---|---| | Meeting storage, replacement and ending | TestCreateMeetingSealsTitleAndMintsOneLiveLink, TestReplaceLinkRevokesTheOldOne, TestEndMeetingKillsItsLinks | | Grant expiry and idempotency | TestGrantExpiresTwoHoursAfterStartOrCreation, TestExpiredGrantIsRefusedEntry; concurrent minting missing | | One-404 rule | Unknown/replaced/ended statuses covered across TestUnknownRevokedAndEndedLinksAreOne404 and the two meeting-action tests; response equality missing | | Outsider emailed-link round trip, cookie present | TestSignedOutOutsiderSignsInThroughTheLinkInTheSameBrowser | | Same round trip, cookie absent at click | TestEmailedLinkWorksInAnotherBrowser | | Authorize admits pending address | Covered by both round-trip tests | | Authorize refuses unknown verified address | Missing; refusing an outsider before Begin does not exercise Authorize | | Revocation during sign-in | TestLinkReplacedBetweenMailAndClickAdmitsNobody; concurrent revocation missing | | /continue: live pending, member/non-member | TestSignedOutMemberFollowsALinkAndLandsOnItsHoldingPage and outsider round-trip tests | | /continue: absent pending, member/non-member | TestMemberSignsInAndLandsOnTheList, TestNonMemberSessionSeesWelcomeNotTheList | | /continue: expired pending, member/non-member | Member covered by TestExpiredPendingRowAdmitsNobody; non-member stops at Authorize, so its continuation case is missing | | Signed-out member completes in another browser | Missing; member test uses the same browser | | Replacement before click: outsider 403, signed-in fallback, no grants | Outsider 403 covered; signed-in / and /welcome fallbacks and direct no-grant assertions missing | | Outsider budgets | TestOutsiderBudgetsStopMailAtFivePerLinkAndTenOverall, TestBudgetSlidesItsWindow; concurrency missing | | One member address exhausts shared allowance | Missing; existing test reaches only the address limit | | Standing table test | Missing; no-standing HTTP substitute uses an absent meeting | | Database/WAL privacy before and after checkpoint | TestTheDatabaseNeverHoldsAPlaintextTitle; file-read failures are ignored | | make ci | Prior record reports green; not independently executed here |

Verdict: request changes.

Dispositions

| Finding | Disposition | |---|---| | C1-1 — outsider addresses in persons.email and sessions.subject | Deferred: suite question, filed. Controller ruling: the persons row with a plaintext email is the suite-wide shape (Docs, Slides, Calendar), and spec §2.1 says so. The plan's "outsider addresses never reach the database" overstated §0.1, which promises a fingerprint for pending_joins only. AGENTS.md's seal bullet now says which table is which. No code change. | | C1-2 — mail errors carry the address into the log | Closed at internal/calls/mailredact.go:30 (RedactMailErrors), wired at cmd/calls/main.go:49. The wrapper logs the failure itself, with the root cause's type and every occurrence of the recipient replaced by [address], and returns a bare meet: mail delivery failed, so auth/handlers.go:51 never sees an address. | | C1-2 — GORM logs interpolated parameters | Closed at internal/calls/app.go:92: db.Open's GORM logger is replaced with one carrying ParameterizedQueries: true at the same thresholds, writing through the app's slog logger (gormWriter, internal/calls/app.go:25). A slow or failing ensurePerson now logs ?, not an address. | | C1-3 — a replaced link can mint a grant | Closed at internal/calls/store.go:265 (mintGrantViaLink) and internal/calls/handlers_join.go:41. The link is re-read by id inside the grant's own transaction; a revoked link or an ended meeting answers false, and followLink turns that into the same 404 an unknown token gets. mintGrant stays for consumePending, which already runs in a transaction that re-checks the link. | | C1-4 — a stale pending row can mint | Closed at internal/calls/store.go:354. consumePending re-selects the newest unexpired row for the fingerprint inside the transaction and proceeds only when its id is the one the caller was handed. A mismatch deletes nothing, so the row that superseded it survives and the caller falls through to / or /welcome. | | C1-5 — two flag parsers | Closed before this wave (85025bc, fd626d6): extractEnvFlag strips -env from argv before rastrillo.Resolve reparses it (cmd/calls/main.go:111). | | C1-6 — platform mail configuration ignored | Closed before this wave (85025bc, fd626d6): smtpSetting and smtpSecret read CALLS_SMTP_* first and fall back to CARLOS_SMTP_* per variable, trimming HOST and FROM (cmd/calls/main.go:80). | | C1-7 — expired records never swept | Closed at internal/calls/store.go:378 (store.sweep) and internal/calls/app.go:56 (app.sweepOnce), run once at boot (internal/calls/app.go:123) and hourly thereafter (internal/calls/app.go:124). The hourly pass also calls auth.Sweep, which clears rastrillo's auth_links and its expired sessions. Counts at info, failures at error. | | C1-8 — tests | Closed. Per-gate below. |

C1-8, gate by gate

| Gate | Now | |---|---| | Database/WAL privacy fails on unexpected read errors | TestTheDatabaseNeverHoldsAPlaintextTitle skips only os.IsNotExist and fails every other read error — internal/callstest/meetings_test.go:111. | | No standing on a meeting that exists | TestHoldingPageIs404ForNoStanding rewritten: a signed-in outsider holding a grant for one meeting gets 404 on a second real meeting, and 404 on the first once its grant has run out — internal/callstest/holding_test.go:27. | | Authorize refuses an unknown verified address | TestAuthorizeAdmitsMembersAndLivePendingRowsOnly, package-internal — internal/calls/auth_test.go:42. Unknown false; allowlisted true; a live pending row true; false once its link is replaced. | | Signed-out member completes in another browser | TestSignedOutMemberCompletesInAnotherBrowser — internal/callstest/join_test.go. | | Replacement before click: signed-in fallbacks, no grant | TestReplacedLinkFallbackForSignedInPerson — internal/callstest/join_test.go. Member lands on /; non-member on /welcome, and their holding page for that meeting 404s, which is the direct no-grant assertion. | | Standing table | TestStandingTable, package-internal — internal/calls/auth_test.go:76. Six cases over allowlisted × grant × expired. | | One 404, bytes and all | TestUnknownRevokedAndEndedLinksAreOne404 now requests an unknown token, a revoked token and an ended meeting's token, and asserts all three statuses are 404 and all three bodies are identical. | | One client exhausts the shared allowance | TestSharedClientAllowanceIsExhaustedByOneMemberAddress — twenty-one different allowlisted addresses from the RemoteAddr httptest fixes at 192.0.2.1:1234; the twenty-first answers 303 /signin?err=rate with exactly twenty mails sent. The limit is twenty: signin.Flow.IPBudget defaults to 20 and AddressBudget to 5 (github.com/keymaildev/signin@v0.1.1/flow.go:123,137). | | C1-2 regression | TestMailErrorsCarryNoAddress — internal/calls/mailredact_test.go. A failing inner sender whose error contains the address; neither the returned error nor a captured slog handler's output contains it. | | C1-3 regression | TestAReplacedLinkMintsNoGrant — internal/calls/store_test.go. Minting through the old link id after a replacement answers not-live and writes no grant row. | | C1-4 regression | TestAStalePendingRowMintsNothingAndDeletesNothing — internal/calls/store_test.go. Two rows for one address; consuming with the older id mints nothing and leaves both rows; the newer id then works and takes both. | | C1-7 regression | TestSweepDeletesOnlyExpiredPendingRows — internal/calls/store_test.go. |

Not covered, and named rather than claimed: the concurrency the review asks for (concurrent minting, concurrent revocation, concurrent budget consumption) is held by the transaction boundary and SQLite's single writer connection, not asserted by a racing test. auth.Sweep is exercised only by being called on the boot path.

Round 2

Reviewer: GPT-6 (Codex), read-only sandbox. Pinned to 3c105b5 (the fix/codex-code-r1 branch after round 1's commit, 5fdddcf, plus an unrelated planning-doc commit). Verdict: request changes. Verbatim below.

The review, verbatim

Verdict: request changes. Reviewed 3c105b50bac55197dba2840429e4078054b0ea86. No files modified; no network used. Go tests were inspected, not executed in the read-only sandbox.

Round-1 findings, in order

| Finding | Status | Code assessment | |---|---|---| | C1-1 | Half-closed; explicitly deferred | store.go:55 still writes plaintext email/name; auth.go:23 still leaves SubjectFor unset. The controller resolved its scope for this wave, not the storage concern. Not an acceptance blocker here. | | C1-2 | Half-closed | The returned mail error is safe, and GORM parameter interpolation is disabled. Meet’s own redacted log can still disclose the address: C2-1. Captured GORM-log coverage is missing. | | C1-3 | Closed in code | store.go:268 opens the transaction; the transaction’s store rereads the link and meeting before inserting the grant. followLink calls this method and checks both error and liveness. The resolver preserves transaction connections, so these reads do not escape to the reader pool. | | C1-4 | Closed in code | store.go:353 reselects the newest unexpired pending row inside the transaction. Missing/mismatched rows return before deletion. The subsequent link lookup uses current.LinkID, not the caller’s stale value. | | C1-5 | Closed in code | main.go:24 extracts the app flag and updates os.Args before Resolve. The competing global flag parser is gone. Executable-invocation regression tests remain absent. | | C1-6 | Closed in code | main.go:84 implements per-setting CARLOS fallbacks, trims HOST/FROM and preserves credential whitespace. Missing HOST/FROM returns nil, preserving the production startup refusal. | | C1-7 | Half-closed | Pending deletion and auth.Sweep now run at boot and hourly while alive. Hibernation, cancellation and bounded execution remain unresolved: C2-2/C2-3. | | C1-8 | Half-closed | Several previously misleading tests now exercise the intended state. Required coverage remains incomplete, and some assertions still permit false confidence: C2-4 and the test assessment below. |

C2-1 — P1 — Unicode lowercasing can leave the complete recipient in the error log.

Location: internal/calls/mailredact.go:79, emitted at line 52.

strings.ToLower can change UTF-8 byte lengths. The code finds an offset in the lowercase string, then uses that offset and the lowercase target length to slice the original string.

A concrete error is:

rastrillo/mail: send to guest@example.com: 550 KKKKKKKKK guest@example.com

Each Kelvin sign occupies three bytes but lowercases to one-byte k. The first address is removed correctly. For the second occurrence, the calculated offset is eighteen bytes early; the replacement cuts through the preceding Unicode text and leaves the complete guest@example.com in the output. A read-only Python reproduction of these byte operations confirmed this; Go’s local strings.ToLower source confirms the relevant Unicode mapping behavior.

The downstream path is otherwise sound: Send returns a fresh errors.New, without Unwrap; signin returns it directly; rastrillo’s auth/handlers.go:51 logs that safe error. Neither %v of that returned error nor %T of the original root cause exposes the original message. The remaining leak is Meet’s own line 52.

Correction: stop logging arbitrary SMTP error text, retaining a safe error type/status, or use matching that returns offsets into the original string. Add this Unicode regression and capture both Meet’s and rastrillo’s actual log lines.

C2-2 — P2 — Expiry cleanup has no scheduled wake under hibernation.

Locations: internal/calls/app.go:124, cmd/calls/main.go:69.

The only recurring trigger is an in-process ticker. Main configures neither Options.NextDue nor scheduled cleanup. Rastrillo’s serve.go:122 explicitly says an unset NextDue means the activator sees no schedule.

A link can be issued, the app can hibernate before its fifteen-minute expiry, and its plaintext auth_links.address can remain indefinitely without another activation. Boot cleanup removes it on the next launch, but cannot impose a retention bound while asleep.

Correction: arrange a platform wake for cleanup, with a bounded cleanup pass on activation and a recurring trigger for standalone serving. Test an unclicked link across shutdown, expiry and scheduled activation.

C2-3 — P3 — Each app construction starts an immortal, uncancellable sweeper.

Location: internal/calls/app.go:67.

sweepForever has no exit condition. Its deferred t.Stop() is unreachable during ordinary application teardown. Every build, including every test harness, retains the app, logger, database and ticker after callers close the database. An hourly tick subsequently queries those closed pools.

The pass also lacks an execution deadline. Pending deletion receives context.Background; rastrillo’s Auth.Sweep uses context-free Exec. Failures are logged and retried next hour, which is useful, but a blocked pass has no application cancellation path. The deletions also process all expired rows in one pass.

Correction: give background work an explicit owner, cancellation and completion wait; stop it before closing the database. Bound cleanup execution and large backlogs. Test shutdown and cleanup-error behavior.

C2-4 — P2 — The regression tests do not yet establish the requested closure.

Locations: internal/calls/store_test.go:150, internal/calls/mailredact_test.go:22, internal/callstest/join_test.go:294, internal/callstest/meetings_test.go:117.

Material gaps remain:

• The link and pending tests are sequential. Moving their rechecks outside the transaction would still pass them. The pending test uses one meeting and never retries an already-consumed row after another meeting’s continuation.
• No captured-log test exercises slow/failed person queries. The mail test calls the wrapper directly, so removing its production wiring would leave it green.
• The sweep test only calls store.sweep. Removing auth.Sweep entirely would leave the new tests green.
• An already-signed-in non-member with an expired pending row still lacks a /continue test. The existing expiry test stops at Authorize.
• The privacy test skips IsNotExist for the primary database as well as optional sidecars. A wrong database path can therefore still produce a vacuous pass.

Correction: add controlled transaction interleavings, cross-meeting consumed-row coverage, captured production-path logs, auth cleanup and authenticated expired-pending tests. Require the primary database file to exist; ignore missing files only for optional sidecars. Concurrent grant and budget tests requested in round 1 also remain outstanding.

GORM logging assessment

The ParameterizedQueries change is effective for the inspected request paths. GORM calls ParamsFilter before Dialector.Explain; the new logger removes the values. Transaction stores inherit that logger. Auth links and sessions use database/sql directly, without query tracing.

GORM still formats database errors with %v, and Meet logs returned store errors. ParameterizedQueries does not sanitize those messages. I found no concrete address-bearing database-error path in the current queries and schema, so this is not a separate finding. The missing captured-log regression prevents claiming tested closure.

Each new or changed test

| Test | What it actually establishes | |---|---| | TestMailErrorsCarryNoAddress | Correctly checks repeated, mixed-case ASCII addresses in a wrapped error, returned error and captured wrapper log. Misses C2-1 and framework/production wiring. | | TestASuccessfulSendIsUntouched | Checks nil return, no logging and nil-sender preservation. Does not verify that context, recipient, subject and body reach the inner sender unchanged. | | TestRedactAddressLeavesTheRestAlone | Checks repeated ASCII matching and empty-address handling. Does not establish Unicode-safe preservation. | | TestAuthorizeAdmitsMembersAndLivePendingRowsOnly | Directly exercises unknown/member/live-pending/revoked cases. Does not cover pending expiry or meeting end independently. | | TestStandingTable | Sound six-case member/guest/absent/expired table; setup errors are checked. | | TestAReplacedLinkMintsNoGrant | Proves sequential rejection and sequential idempotency. Does not prove transactional placement or concurrent replacement. Its negative grant lookup discards errors. | | TestAStalePendingRowMintsNothingAndDeletesNothing | Proves a superseded ID is rejected and preserves both pending rows. Does not cover the original cross-meeting, already-consumed sequence; the successful path does not directly assert the grant row. | | TestSweepDeletesOnlyExpiredPendingRows | Proves deletion of one expired pending row and preservation of one live row. Does not cover exact expiry, auth records, scheduling or failures. | | TestHoldingPageIs404ForNoStanding | Now exercises an authenticated guest against a real unrelated meeting, then an expired grant. The original nonexistent-meeting false pass is fixed. | | TestUnknownRevokedAndEndedLinksAreOne404 | Proves status/body equality for the three HTTP scenarios. Ending also revokes the token, so it cannot independently detect removal of the meeting-ended check. | | TestSharedClientAllowanceIsExhaustedByOneMemberAddress | Correctly isolates the shared client limit using 21 different addresses. Its name and the specified repeated-single-member scenario remain inaccurate/uncovered. | | TestSignedOutMemberCompletesInAnotherBrowser | Establishes the intended cross-browser member journey and successful holding-page access. | | TestReplacedLinkFallbackForSignedInPerson | Establishes both fallbacks and denied access to the guest’s second meeting while preserving access to the first. The member case never checks grant absence; the guest check establishes no usable grant, not direct row absence. | | TestTheDatabaseNeverHoldsAPlaintextTitle | Now fails ordinary read errors and scans before/after checkpoint. Still skips a missing primary database. |

The transaction fixes are sound on inspection. Address redaction and hibernating cleanup still need correction before acceptance.

Dispositions, C2-1 to C2-4

| Finding | Disposition | |---|---| | C2-1 — strings.ToLower offsets can leak a fragment of the address | Closed. internal/calls/mailredact.go no longer touches the message text at all beyond testing it: redact logs only the root cause's %T and a bool, address_seen, from addressSeen(err.Error(), to) — strings.Contains(strings.ToLower(msg), strings.ToLower(address)), no offsets and no slicing. redactAddress and its "\[address\]"-substitution approach are gone entirely. TestNoFragmentOfAnAddressReachesTheLog (internal/calls/mailredact_test.go) puts Kelvin@example.com (plain ASCII K) and İnes@example.com (LATIN CAPITAL LETTER I WITH DOT ABOVE, U+0130) in one error message alongside a run of ordinary KKKs, and asserts the captured log holds no @ and no fragment of either address — plus no U+FFFD, in case anything were still cutting mid-rune. | | C2-2 — no scheduled wake for expiry cleanup under hibernation | Closed. store.nextExpiry(ctx) (internal/calls/store.go) answers the earliest pending_joins.expires_at, or the zero time when none exists. App's signature changed to App(ctx, d, cfg) (*http.ServeMux, *Runtime, error); Runtime.NextDue is a.nextDue, which answers the earlier of nextExpiry and one hour from now, or the zero time when nothing is pending. cmd/calls/main.go sets opts.NextDue = rt.NextDue, matching serve.go's contract: zero means nothing scheduled, and the returned time is read by the platform's GET /api/next-due poll. TestNextExpiryIsTheEarliestExpiryNotTheLatestRow (internal/calls/store_test.go) covers the store half by construction (a row written last, at the higher id, with an earlier expiry than the one before it) — nothing exercises the platform's own poll route, which is rastrillo's surface, not Meet's. | | C2-3 — an immortal, uncancellable sweeper per app construction | Closed. build(ctx, d, cfg) takes a context and bounds the boot sweep to it with a 30-second timeout (context.WithTimeout); the hourly ticker (sweepForever) is started only by App, never by NewForTest, and exits on ctx.Done(). App's caller (cmd/calls/main.go) passes context.Background() — acceptable per the brief, since nothing in this app ever needs to cancel the sweep early — but the exit path exists for a caller that does. NewForTest returns a *TestHandle whose SweepForTest(now time.Time) runs one bounded pass (30-second timeout) with the given time for both store.sweep and auth.Sweep, so a test drives one clock, not two. store.sweep is now bounded to sweepLimit (1000) rows a call via a subselect on ids; the hourly pass (sweepHourly) loops while a call returns exactly that many, so a backlog larger than one page still clears within the hour rather than only ever removing its first 1000 rows. | | C2-4 — regression tests do not establish the requested closure | Half-closed, itemized: |

| C2-4 sub-item | Now | |---|---| | Transaction fixes proven only sequentially, not against interleaving | Not done. No racing test was added for mintGrantViaLink or consumePending. Named as uncovered below, as round 1 also left it. | | Pending test uses one meeting; no cross-meeting retry of an already-consumed row | Closed. TestConsumingOneMeetingsPendingRowLeavesAnothersIDStale (internal/calls/store_test.go): pending rows for meetings A then B for one address; consuming B's (the newer) mints B's grant and deletes every pending row for the address, A's included; a subsequent consumePending with A's now-stale id answers no grant and no error. | | No captured-log test for the production wiring (mail test calls the wrapper directly) | Closed. TestSigninOverAFailingMailerLeavesNoAddressInTheLog (internal/callstest/signin_test.go) builds the app with meet.App — the same entry point cmd/calls/main.go calls — over a Config.Mailer that fails and a real *slog.Logger passed as Config.Logger, drives a real POST /signin, and asserts the captured log carries no fragment of the address. This is the check that resolve wraps every mailer regardless of call site (Internal 3), not just that the wrapper itself is safe. | | Sweep test only calls store.sweep; removing auth.Sweep would leave it green | Closed. TestSweepClearsUnclickedMagicLinks (internal/callstest/join_test.go) asserts auth_links directly: a sign-in request leaves one row, a sweep at +11 minutes (inside auth_links' fifteen-minute life) leaves it, and a sweep at +16 removes it. This exercises auth.Sweep specifically — deleting it would fail the +16 assertion. | | No /continue test for an already-signed-in non-member with an expired pending row | Closed, by the substitute construction the brief itself allowed for: TestReplacedLinkFallbackForSignedInPerson's "non-member with an expired row lands on welcome with no grant" subtest (internal/callstest/join_test.go) signs the outsider in through meeting A, then has them POST /signin again for meeting B's link while already signed in (still writes a pending row), advances the clock sixteen minutes, and confirms /continue lands on /welcome, B's holding page 404s, and A's still 200s. | | Privacy test skips IsNotExist for the primary database too | Closed. TestTheDatabaseNeverHoldsAPlaintextTitle (internal/callstest/meetings_test.go) now only tolerates os.IsNotExist for the -wal/-shm suffixes; a missing primary file fails the test. | | TestAReplacedLinkMintsNoGrant's negative lookup discards its error | Closed — internal/calls/store_test.go now checks err alongside ok on the grantFor call. | | Shared-limiter test's name and scenario | Closed (round 1's own residue, named again in C1-8's table): renamed to TestSharedClientAllowanceIsExhaustedByOneClient, with a comment that behind the platform edge RemoteAddr is the proxy, so this is a real, filed gap (the shared budget is exhaustible by any twenty visitors of any team), not a quirk of the harness. |

Not covered, named rather than claimed: C2-4's core ask — controlled transaction interleavings for mintGrantViaLink and consumePending, and concurrent budget consumption — remains untested. Every invariant those paths rely on is enforced by the transaction boundary plus SQLite's single writer connection (SetMaxOpenConns(1)), not by a racing Go test; a test that actually raced two goroutines against it would be exercising the sqlite driver and the OS scheduler, not Meet's logic. This is the same position round 1 took and round 2 flagged as still open; it is being named again rather than resolved, because doing it properly needs a harness for deterministic interleaving (e.g. hooks around the transaction, or a mock DB) that does not exist yet.

Round 3

Reviewer: GPT-6 (Codex), read-only sandbox. Pinned to 9e067e4 (round 2's fix commit, before c5981ca and the further commit this section describes). Verdict: request changes. Verbatim below.

The review, verbatim

Verdict: request changes. Reviewed 9e067e41c2586cfbee41de42cffa43044f7c45fe. No files modified; no network used. Tests were inspected, not executed in the read-only sandbox.

| Finding | Status | Code assessment | |---|---|---| | C2-1 | Closed | mailredact.go:87 logs only the root cause’s type and address_seen; the returned error is fixed text without an unwrap chain. config.go:116 wraps configured mailers, including the development fallback. | | C2-2 | Half-closed | Main correctly assigns opts.NextDue = rt.NextDue. The callback overlooks auth links and sessions, so cleanup can still remain unscheduled: C3-1. | | C2-3 | Half-closed | Boot cleanup is synchronous; NewForTest starts no ticker; pending deletion is limited to 1000 rows per call. The auth cleanup remains unbounded and uncancellable, and some tests still create immortal tickers: C3-2. | | C2-4 | Half-closed | Production mail logging, auth-link deletion, cross-meeting stale continuation, expired continuation and primary-file checks now have coverage. Slow/failed person-query log coverage remains absent. Some new assertions are narrower than claimed; see below. |

C1-1 remains explicitly deferred. The declined transaction-interleaving and concurrent-budget tests remain recorded as uncovered; their absence does not independently block this verdict.

C3-1 — P2 — The wake callback reports “nothing scheduled” while auth records need cleanup.

Location: internal/calls/app.go:249, backed by internal/calls/store.go:397.

nextExpiry examines only pending_joins. A member requesting sign-in without a join cookie reaches auth without writing a pending row (auth.go:83). Signin writes an auth_links row before attempting delivery, but NextDue returns zero. If the app hibernates before expiry, that unclicked link has no scheduled cleanup wake.

The new TestSweepClearsUnclickedMagicLinks constructs precisely this situation, but invokes cleanup manually rather than checking its scheduling.

This also affects outsider requests: multiple magic links can exist for one address, while consuming one continuation deletes every pending row for that address. Remaining unclicked auth links then cease to have a wake deadline. Sessions are likewise absent from the calculation.

Rastrillo’s contract and implementation agree: zero means no schedule (serve.go:122, :554). Returning a past pending expiry is correct: the framework preserves its Unix timestamp, and the locally inspected platform selects positive deadlines at or before now. The defect is the incomplete calculation of “nothing scheduled.” Returning zero on a query error (app.go:247) also abandons the cleanup retry.

Correction: include all records that cleanup owns in the deadline calculation, or maintain a bounded recurring cleanup deadline independently of pending rows. Use a finite retry deadline on lookup failure. Test auth-only state, remaining links after pending consumption, empty state and overdue work through Runtime.NextDue.

C3-2 — P3 — The timeout does not bound the whole sweep, and teardown remains incomplete.

Location: internal/calls/app.go:84, also :109, :187; internal/callstest/signin_test.go:90.

The 30-second context reaches store.sweep, but auth.Sweep(now) receives no context. Rastrillo’s auth/store.go:119 and sessions/sessions.go:447 use context-free DB.Exec and delete all matching rows. A wait for the writer connection therefore ignores Meet’s timeout. Cancellation during an hourly pass also cannot interrupt this part.

The 1000-row pending limit is correctly implemented. It does not bound auth deletion or the hourly pass’s total duration.

Moving ticker creation out of build fixes the normal harness leak. However, the new production-mail test and the successful development/schema tests still call App(context.Background(), …) and close the database without cancelling the ticker. There is also no completion handle for callers to await before closing the database.

Correction: make auth cleanup context-aware and bounded; provide cancellation plus completion waiting. Use cancellable contexts in tests that intentionally exercise App. Add deadline and shutdown tests.

C3-3 — P3 — The expiry-ordering test also passes when selecting the latest row.

Location: internal/calls/store_test.go:311.

The earliest-expiring row is deliberately inserted last. Consequently, replacing Order("expires_at ASC") with Order("id DESC") still returns the expected row. The test does not establish its named distinction, “earliest expiry, not latest row.”

Correction: include a later-inserted row whose expiry is later than the expected minimum. A three-row fixture can distinguish expiry ordering from both ascending and descending ID ordering.

The new and substantively changed tests establish the following:

| Test | Does it prove its claim? | |---|---| | TestMailErrorsCarryNoAddress | Yes, for its fixture. Checks fixed returned text, absence of the local part, root type and address_seen=true. | | TestNoFragmentOfAnAddressReachesTheLog | Partly. Checks selected fragments of both fixture addresses. It does not reproduce C2-1’s Kelvin-prefix offset trap: its Ks are ASCII, and dotted I occurs after the matched recipient. The current implementation is nevertheless safe by inspection. | | TestAddressSeenIsCaseInsensitiveAndNeedsNoSlicing | Yes for the boolean cases. “Needs no slicing” is established by inspecting the implementation, not these assertions. | | TestTheWrapperForwardsDeliversAndSendList | Partly. Checks interface preservation, the logged sender’s false delivery status, successful list invocation and sanitized list failure. It does not verify forwarding the actual message fields or context. | | TestSigninOverAFailingMailerLeavesNoAddressInTheLog | Yes for the complete fixture address through the app path. Removing the wrapping would expose it and fail the test. Contrary to the disposition, it does not assert absence of address fragments or exercise person-query logging. | | TestConsumingOneMeetingsPendingRowLeavesAnothersIDStale | Yes for sequential stale rejection. Directly checks B’s grant, A’s empty destination and absence of A’s grant. It does not directly assert deletion of all pending rows; setup errors are also discarded. | | TestNextExpiryIsTheEarliestExpiryNotTheLatestRow | No for the ordering distinction: C3-3. The empty-table zero assertion is sound. It does not exercise Runtime.NextDue. | | TestSweepClearsUnclickedMagicLinks | Yes for explicit auth-link cleanup. Direct row counts establish preservation at +11 minutes and deletion at +16. Removing auth.Sweep would fail it. Scheduling, restart, sessions and execution bounds remain untested. | | Expired non-member /continue subtest | Substantially. Exercises an existing guest session, expired pending lookup, /welcome, denied B access and retained A access. It should assert the pending row exists before advancing time: sent=1 alone also represents refusal. | | TestTheDatabaseNeverHoldsAPlaintextTitle | Yes for its creation fixture. Missing primary files now fail; only missing optional sidecars are tolerated. Scans run before and after checkpoint. | | TestAReplacedLinkMintsNoGrant | Improved. The negative grant lookup now fails on database errors. Sequential scope is unchanged. | | TestSharedClientAllowanceIsExhaustedByOneClient | Yes. The renamed test accurately describes 21 different addresses exhausting one client’s allowance. |

TestASuccessfulSendIsUntouched still checks only successful return, silence and nil preservation—not argument forwarding. Existing small sweep fixtures establish expiry filtering, but cannot establish the new 1000-row limit or multi-page loop.

The remaining deployment blocker is C3-1: cleanup scheduling still misses the auth records it was introduced to remove.

Dispositions, C3-1 to C3-3

| Finding | Disposition | |---|---| | C3-1 — NextDue blind to auth_links and sessions; zero on query error | Closed, in two commits. c5981ca first closed the auth_links half: store.nextExpiry took the earlier of pending_joins and a new MIN(expires_at) read over auth_links. This section's own commit finished it: nextExpiry now also reads MIN(expires_at) over rastrillo's sessions table (same RFC3339-text shape, confirmed in sessions/migrations/0001_init.sql and sessions/sessions.go:157 — auth.Sweep cleans both auth_links and sessions in one call, auth/store.go:117 delegating to sessions.Sweep, so both need the same wake guarantee), and takes the earliest of all three sources. app.nextDue no longer returns the zero time on a query error: it now answers a.store.now().Add(sweepEvery) — the same one-hour retry deadline the healthy "nothing due sooner than the next sweep" path already uses — and still logs the error. TestNextExpiryAlsoReadsAuthLinks (round 2) and this round's fixes are covered by TestNextDueWakesForAPlainMemberSignin (internal/callstest/signin_test.go), which drives a real plain POST /signin with no pending row and checks TestHandle.NextDue() (new, forwarding to a.nextDue()) directly. Not added: a dedicated unit test for the query-error retry-deadline branch itself (would need a store whose nextExpiry can be made to fail on demand, which the current store has no seam for) and a sessions-only scenario through the full HTTP path (session rows come from sessions.SignIn, which every signed-in request already exercises, so it is covered incidentally by every other test but not asserted on directly). | | C3-2 — auth.Sweep is context-free; some tests still leak an immortal ticker | Partial, as the coordinator's brief for this round anticipated. auth.Sweep(now) (rastrillo's own method, auth/store.go:117) uses a.cfg.DB.Exec with no context parameter at all — there is no seam in rastrillo to bound it with, short of forking or wrapping the whole *rauth.Auth, which is out of scope for a Meet-side fix. This is noted here rather than solved. What is fixed: the tests C3-2 named — internal/callstest/signin_test.go's production-mail test, config_test.go's three meet.App call sites, and schema_test.go's one — now build their context with a new testCtx(t) helper (context.WithCancel plus t.Cleanup(cancel)), so App's hourly ticker is cancelled when the test ends rather than continuing to poll a database the test is about to close. harness_test.go's own newHarness already used meet.NewForTest, which starts no ticker at all (round 2), so it needed no change. | | C3-3 — the expiry-ordering test also passes under Order("id DESC") | Closed. TestNextExpiryIsTheEarliestExpiryNotTheLatestRow now writes three rows — first-inserted expiring at +20m, second-inserted (the expected minimum) at +5m, third/last-inserted at +30m — so neither an ascending nor a descending id sort would find the right answer; only sorting on expires_at does. |

Also addressed from the test-assessment table, though not itself a lettered finding: the expired non-member /continue subtest (internal/callstest/join_test.go) now asserts pendingJoins(t) == 1 right after the outsider's second POST /signin and before advancing the clock, closing the gap the table named — "?sent=1" alone is also the answer to a request that wrote nothing at all (spec §3.2's same-answer rule), so the row's existence needed a direct assertion, not an inference from the redirect.

Not addressed, named rather than claimed: auth.Sweep's own unboundedness (the context-free half of C3-2, rastrillo's surface, not Meet's, per above); a unit test for nextDue's query-error retry branch; and every item C2-4/C1-8 already recorded as uncovered (transaction interleavings, concurrent budget consumption, auth.Sweep execution-time bounds, Runtime.NextDue's platform-side poll route).