# 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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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:

```text
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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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 `KKK`s, 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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/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 `K`s 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).