amadan
GPT-6-Astra (Codex), reviewing for session 21a32889.
Reviewed the whole branch at a871701d2b911f581be309d5fc09046fd11313e1 against freshly fetched origin/main 6fa77852776bf3a431ba31171368a393d5cc0bbb, in detached scratch worktrees under /tmp. Also compared rastrillo d9c28d7 with the pinned main revision fb806a2.
VERDICT: CHANGES-REQUIRED — needs changes.
- P2 — Preserve cancellation of the database work in the team session reader.
internal/docs/team_session_reader.go:35–47starts an unrestricted goroutine for each lookup, but cancelling its context only abandons the result.sessions.Fromultimately callsDB.QueryRowwith a background context on Docs' one-connection writer pool. If a transaction holds that connection, every cancelled/expired stream lookup remains queued, retaining its goroutine and request until the connection becomes available. SQLite'sbusy_timeoutdoes not bound waiting for a Go connection-pool slot. Under concurrent streams and a slow or stalled writer, obsolete lookups accumulate and still compete for the writer when it recovers. The buffered answer channel prevents a blocked send; it does not cancel the queued query.
Reproduced against the actual reader and database: hold the writer in a transaction, queue a lookup, cancel after WaitCount advances, repeat 64 times. All 64 callers return “no session”, but the branch retains 64 session-lookup goroutines; main retains 0 with the same probe. Both have one open connection, so this is goroutine/pending-work buildup, not connection proliferation. The branch workers disappear only after releasing the writer. The existing TestTeamStreamActualSessionWriterWaitCannotExtendOriginalLease checks that the stream closes, not that its database work ends, so it remains green.
Restore a context-aware shared session lookup that carries the deadline into connection acquisition and QueryRowContext (preferably land that API on rastrillo main and pin it). Add a regression that cancels queued lookups while keeping the writer held and verifies their workers finish before releasing it. A bounded worker/admission mechanism would at least need an explicit bound on outstanding work; a goroutine per call is not such a bound. No session-authority bypass was found: cancellation correctly refuses the caller, and the abandoned lookup is read-only.
The other requested checks:
- CSP: comparing the actual pinned defaults, the only directive difference is
style-src: the new framework policy uses'self' 'unsafe-hashes'plus the specific honeypot hash. Docs''self' 'unsafe-inline'is the intended relaxation; copying the old policy loses no additional CSP directive. The new HSTS header is set separately by rastrillo and survivesOptions.CSP. Home correctly adds only the validated issuer to the finalform-action; confirm adds its framing CSP alongside the full policy. Minor hardening note: the revised policy test compares the app's constant with itself through serving middleware, so it no longer detects future framework hardening outsidestyle-src. Keep an independent comparison against the actual framework default, allowing only the intentional style/form-action differences. - ProofOff: preserves the established no-PoW behavior. Rate limiting and admission-gated mail delivery remain. Classification still precedes admission and may make outbound requests; ProofOff does not eliminate that pre-existing abuse surface. No new blocker from making this choice explicit.
- Confirm and CSRF: production-handler probes passed in standalone and Home mode: exactly one button; token occurs only in the hidden field; hostile token markup is escaped;
no-store,strict-origin,noindex, nofollow, DENY and both CSP headers survive. Cross-site, same-site, hostile Origin/Referer,Origin: null, and missing-evidence POSTs are refused; a subsequent same-origin POST still spends the link. Origin-only fallback also succeeds. The GET does not consume the token. Sign-in chrome does not copy the token into a return URL. - Refusal: the hook renders the 403 door, preserves existing response cookies and sets no-store. The email-revocation and actual Keymail integration tests pass, including refusal without minting a session. Removing response-text matching is sound.
- UTC/no backfill: Docs already writes its relevant document/grant timestamps in UTC. I found no SQLite
date/datetime/julianday/strftimearithmetic over those legacy timestamp columns. List ordering uses their common date/time prefix; the link-flow expiry comparison uses integer Unix seconds, and session storage continues to use explicitly formatted RFC3339 strings. No date-maths blocker found from leaving old rows in place.
Verification: TMPDIR=/tmp GOTOOLCHAIN=go1.26.5 .amadan/ci.d/10-gate passed in the branch worktree; remote CI also reports passed at a871701. The browser drive independently returned 43/44 on both branch and main, with identical 320px SVG-path failures at 562/592/622px. For main, a temporary external launcher opened its GET sign-in link in the debugging browser, avoiding the known hanging screenshot bootstrap; the main browser checks were unchanged. The reflow failure is pre-existing and is not a finding against this branch.
Review probes were supplied through Go overlays from /tmp/docs-review-77-probes; no repository source files were edited. Both worktrees remain clean. No commit, push, merge or deployment performed.