amadan
Gate 9: no address, team name or queue payload reaches a log line Issue open
On branch feat/gate-9-no-content-in-logs
Review record for feat/gate-9-no-content-in-logs. Two commits: 0c00cd9 builds the gate, 1caabab applies a Fable review.
Why this branch exists
§13 step 1 is "Skeleton, AGENTS.md, gates 1, 2 and 9". Gates 1 and 2 need a database this app does not have. Gate 9 does not — it scans strings — so it was buildable from the skeleton's first commit and was simply missing. cmd/home/main.go has been emitting log lines since ef8a30f.
Review: Fable 5.1 — REQUEST CHANGES, fixed in 1caabab
Three confirmed ways for the gate to pass a line that leaks. Every finding re-verified against the Go and chi sources before applying.
1. A []byte attribute fails in the dangerous direction. slog's text handler quotes a byte slice as a string, so a queue payload logged as []byte is readable in production, while slog.Value.String goes through fmt.Sprint and yields [67 65 78 ...]. Confirmed by running both handlers side by side. Queue payloads are the sink most likely to be a byte slice.
2. The address canary could not be found by the test that sent it. The guard built its query with url.QueryEscape, turning @ into %40. Any request logger printing r.URL or RawQuery would log an address the scanner missed. found() now also checks the escape forms and scans case-folded, since an address is routinely lowercased before hashing.
3. The most likely first request logger bypassed the gate entirely. chi's middleware.Logger builds log.New(os.Stdout, ...) at package init and writes the request line — URL included — straight to stdout, touching neither slog nor the log default. middleware.Recoverer writes panics to os.Stderr. Nothing forbade either import. internal/guardstest gains TestNoLogSinkOutsideSlog, which forbids that import and — by AST walk, not text scan — os.Stdout, os.Stderr and the log package's printers in non-test code.
And a claim of mine that was simply false. I wrote that JSONHandler drops error text via json.Marshal. It does not: json_handler.go type-asserts error and appends err.Error(). A run confirms the text survives. That false reason was in the code comment and the first commit message. The custom handler is still the right call, for a narrower reason now stated correctly.
Also fixed: slog.SetDefault calls log.SetOutput/log.SetFlags(0) and restoring a default-handler logger skips that branch, so every later test in the package would have had its log output swallowed; the containment guard's rationale was wrong (test flags come from testing.Init since Go 1.13, not from importing testing).
What changed structurally
Routes are now enumerated with chi.Walk rather than hand-listed, and each is driven with GET and POST carrying a form body — sign-in will put the address in a POST body. A route added in step 2 is covered without anyone remembering.
What this gate does NOT cover — stated in the code, not just here
§3.4 names four sinks. This covers one and a half:
- Log lines — only those routed through slog.
- Error strings — only transitively, when an error is logged. One rendered into an HTTP response is not scanned.
- Job status messages — not covered. No queue yet; when there is one, rastrillo's
jobsturnserr.Error()into a status the owner sees. - Metric labels — not covered. No metrics exist.
Plus cmd/home/main.go's two logger.Error calls (no test seam), and rastrillo's own machinery — it resolves slog.Default() at construction, so swapping the default afterwards does not reach a logger it already holds.
Verification
Every guard was checked by mutation, not assumed. Eight mutations, eight failures. One test originally did not bite — the LogValuer case passed with Resolve removed, because fmt.Sprint of the test's own struct rendered the canary by accident. The type now has a String() returning "REDACTED", so only a resolving scanner sees it.
go test -race ./... passes. All four .amadan/ci.d gates green — none of which reads any of this, so they verify the build, not the gate.
Review status
Fable's minimum-to-approve list is fully applied. Not re-reviewed since 1caabab; §Review wants a pass by a model that did not write it, and the useful question now is narrow: do the three fixes close the paths they name, and does TestNoLogSinkOutsideSlog have false positives that will annoy step 2's author?
@paul — same open question as discussion 1: no main on this remote, so this merges to skeleton as de-facto trunk unless you rule otherwise.