fix(logger): redact paths and scrub err.* before pino emits (closes #52) (#89)

Co-authored-by: chrisleekr-bot[bot] <2890262+chrisleekr-bot[bot]@users.noreply.github.com>
Co-authored-by: Chris Lee <chrisleekr@users.noreply.github.com>
Co-authored-by: chrisleekr-bot[bot] <chrisleekr-bot[bot]@users.noreply.github.com>

Author: 3 people

IMPLEMENT.md

@@ -1,101 +1,112 @@
-# Issue #51 — docs(ci): close docs-drift gap with PR-wide build, version-pin guard, and src-citation verifier
+# Implement — issue #52: pino logger redact paths + err scrubbing
+
+Closes #52.
 
 ## Summary
 
-Closes the silent docs-drift gap in `.github/workflows/docs.yml` by removing
-the `pull_request:` `paths:` filter (so every PR runs `mkdocs build --strict`,
-not just doc-touching PRs), and adds two project-specific CI gates that
-`mkdocs --strict` cannot do on its own — a Bun-version pin verifier and a
-`src/<path>:<line>` citation verifier. Also refreshes the surface-to-page
-map in `CLAUDE.md` for the post-reorg `docs/` layout and documents the new
-gates. Closes #51.
+Adds path-based redaction and a composed `err` serializer to the root
+pino instance at `src/logger.ts:174` so every log line — across every
+child logger and every call site — passes through a single chokepoint
+that scrubs:
+
+- Named credential fields (`authorization`, `x-hub-signature-256`,
+  `privateKey`, `webhookSecret`, `installationToken`,
+  `claudeCodeOauthToken`, `daemonAuthToken`, `awsBearerTokenBedrock`,
+  `awsSecretAccessKey`, `awsSessionToken`, `anthropicApiKey`, `token`,
+  `*.password`, plus the `headers.*` / `*.headers.*` /
+  `req.headers.*` / `request.headers.*` / `response.data.token` shapes
+  for the same fields).
+- Free-text leakage in error `message`, `stack`, `request.headers.*`,
+  and `response.data.*`. The serializer reuses the already-tested
+  `redactGitHubTokens` regex (`src/utils/sanitize.ts:77-89`) and adds
+  an inline `scheme://user:pass@host` scrubber that mirrors
+  `redactValkeyUrl` (`src/orchestrator/valkey.ts:62-73`).
+
+The serializer operates on a copy, so the original `Error` instance is
+never mutated. No new npm dependencies, no env var changes, no edits
+to the ~27 callers of `logger.*`.
 
 ## Files changed (path · one-line rationale)
 
-- `.github/workflows/docs.yml` · drop `paths:` from `pull_request:` so
-  code-side changes that invalidate doc facts still trip the docs job;
-  add `oven-sh/setup-bun@v2` plus two `Verify…` steps before
-  `mkdocs build --strict`.
-- `scripts/check-docs-versions.ts` · new Bun script. Reads canonical Bun
-  version from `.tool-versions`, asserts `package.json` `engines.bun` /
-  `packageManager` and `Dockerfile.{orchestrator,daemon}`
-  `FROM oven/bun:<ver>` lines agree, then scans every `docs/**/*.md` for
-  `oven/bun:<ver>` and for loose Bun semvers (only inside lines that
-  mention the word `bun`, to avoid false matches on Node / openssl pins)
-  and fails on disagreement.
-- `scripts/check-docs-citations.ts` · new Bun script. Walks
-  `docs/**/*.md`, extracts every `src/<path>.<ext>:<line>` (or
-  `:<start>-<end>`) citation, and verifies the file exists and the cited
-  line / range is in bounds. Bare `src/foo.ts` references without a
-  `:line` suffix are intentionally out of scope — they don't claim a
-  line and can't go stale on a shift.
-- `package.json` · adds `check:docs-versions` and `check:docs-citations`
-  scripts and chains both into the unified `check` script.
-- `CLAUDE.md` · refreshes the `Documentation` surface-to-page map for
-  the audience-first `docs/` layout (`operate/`, `build/`, `use/`) and
-  documents the two new CI-enforced gates.
-- `IMPLEMENT.md` · this artifact (replaces the prior run's tracking
-  comment body).
+- `src/logger.ts` · Adds `REDACT_PATHS` + composed `errSerializer`,
+  wires both into the root pino instance, exports them so tests can
+  rebuild the same config against a captured destination.
+- `test/utils/logger.test.ts` · New file — eight unit tests covering
+  every leak vector from the issue: App JWT in
+  `err.request.headers.authorization`, `ghs_…` in `err.message` and
+  `err.stack`, top-level `privateKey` field, `x-hub-signature-256`
+  header, Valkey URL credentials in `err.message`, non-mutation of
+  the original Error, `response.data.token` redaction, and the
+  non-error pass-through branch.
+- `docs/operate/observability.md` · New "Log redaction" section
+  documenting the two-layer approach (paths + err serializer),
+  citing `src/logger.ts:17` and `src/logger.ts:113`, and noting that
+  `redactGitHubTokens` / `redactValkeyUrl` remain in place for their
+  non-log call sites.
 
 ## Commits (sha · subject)
 
-- `<sha>` · ci(docs): run docs build on every PR + add version-pin and
-  src-citation guards
+- `36a23bf` · fix(logger): redact paths and scrub err.\* before pino emits
+- `854c8df` · test(logger): cover redact paths and err serializer scrubbing
+- `8e13042` · docs(observability): document the logger as canonical redaction chokepoint
 
 ## Tests run (command · result)
 
-- `bun run typecheck` · pass (clean).
-- `bun run lint` · pass — 0 errors, 261 pre-existing warnings unchanged
-  (none in the two new scripts).
-- `bun run format` · pass after `bun run format:fix` normalised
-  `scripts/check-docs-citations.ts`.
-- `bun run check:no-destructive` · pass.
-- `bun run check:docs-versions` · pass on clean tree
-  (`OK: every Bun version reference matches .tool-versions canonical 1.3.13`).
-- `bun run check:docs-citations` · pass on clean tree
-  (`OK: every src/<path>:<line> citation in docs/ points at an in-range location`).
-- Negative-path for `check:docs-versions`: edited `docs/operate/setup.md`
-  to say `1.3.8`; rerun exited 1 with
-  `docs/operate/setup.md:9 [...] found '1.3.8', expected '1.3.13'`. Reverted.
-- Negative-path for `check:docs-citations`: appended
-  `src/k8s/ephemeral-daemon-spawner.ts:99999` to `docs/operate/deployment.md`;
-  rerun exited 1 with `start line 99999 out of range (file has 226 lines)`.
-  Reverted.
+- `bun run typecheck` · clean (0 errors)
+- `bun run lint` · 0 errors / 277 warnings (identical to pre-change baseline of 277)
+- `bun run format` · `All matched files use Prettier code style!`
+- `bun test test/utils/logger.test.ts` · 8 pass / 0 fail / 25 expect() calls
+- `bun test test/utils/sanitize.test.ts` · 30 pass / 0 fail (regression check)
+- `bun run scripts/check-docs-citations.ts` · OK (every `src/<path>:<line>` citation in docs is in-range)
+- `bun run scripts/check-docs-versions.ts` · OK (Bun version pins agree)
+- `mkdocs build --strict` · clean
 
-`bun run test` and `bun run docs:build` were not run locally — the test
-suite goes through `scripts/test-isolated.sh` and depends on
-Postgres/Valkey docker containers, and `mkdocs build --strict` needs
-the Python deps from `docs/requirements.txt`. Both run as part of CI on
-this PR.
+The wider `bun test` suite has 186 fail / 24 errors that are
+infrastructure-dependent (require real Postgres + Valkey) and were
+present on `main` before this PR (verified by `git stash && bun test`
+baseline: 187 fail / 25 errors — this PR actually removes one failure
+and one error).
 
 ## Verification
 
-Acceptance criteria from the plan:
-
-1. **Docs build runs on non-doc PRs.** `pull_request:` no longer carries
-   a `paths:` filter (`.github/workflows/docs.yml`); the
-   `Deploy to GitHub Pages` step keeps its
-   `if: github.event_name == 'push' || github.event_name == 'workflow_dispatch'`
-   guard so PR runs validate without publishing.
-2. **Version-pin check is wired.** `scripts/check-docs-versions.ts` exits
-   0 on a clean tree and 1 on any disagreement with `.tool-versions`.
-   Wired into `bun run check` and as a `Verify docs version pins` step
-   in `.github/workflows/docs.yml`.
-3. **Citation check is wired.** `scripts/check-docs-citations.ts` exits
-   0 on a clean tree and 1 on any out-of-range citation. Wired into
-   `bun run check` and as a `Verify docs src citations` step in
-   `.github/workflows/docs.yml`.
-4. **`bun run check` includes both gates.** Updated in `package.json`.
-5. **Workflow gate fails the job.** Both `Verify…` steps use
-   `bun run …` without `continue-on-error`, so a non-zero exit fails
-   the `Docs / build` check.
-6. **Strict build still green.** No content changes to docs that the
-   verifier would flag; `oven-sh/setup-bun@v2` is added before the
-   Python setup so `bun` is on `PATH` for the new steps.
+1. **T1 — redact paths:** `src/logger.ts:17` enumerates every path the
+   issue asked for. The list lives next to the logger so a new
+   secret-bearing config field added in `src/config.ts` is one place
+   to update.
+2. **T2 — composed err serializer:** `src/logger.ts:131` defers to
+   `pino.stdSerializers.err` and only then runs string-scrubbers,
+   preserving downstream tooling compatibility while catching the
+   four-segment-deep `err.request.headers.authorization` path that
+   pino's wildcard syntax cannot reach (pino's `*.foo.bar` only
+   matches 3-segment paths). Verified by the
+   `redacts request.headers.authorization carrying an App JWT` test.
+3. **T3 — Valkey URL credential scrubbing folded into logger:**
+   `redactCredentialUrls` in `src/logger.ts:54` is invoked by
+   `scrubString`, which the err serializer applies to `message`,
+   `stack`, and string values inside `request.headers` /
+   `response.data`. The point helper at `src/orchestrator/valkey.ts:64`
+   stays in place for the info-log call site at
+   `src/orchestrator/valkey.ts:33`. Verified by
+   `scrubs Valkey URL credentials embedded in err.message`.
+4. **T4 — unit tests:** `test/utils/logger.test.ts` covers all five
+   plan-mandated assertions plus the response.data.token / non-error
+   branches. Coverage on `src/logger.ts` is 100% functions / 98.97%
+   lines (above the 90 % per-file gate).
+5. **T5 — docs:** `docs/operate/observability.md` gains a "Log
+   redaction" section that describes both layers and cross-links to
+   the point helpers; `mkdocs build --strict` passes, citation guard
+   passes.
+6. **Non-mutation:** The serializer uses object spread to produce
+   fresh `request` / `response` objects, leaving the original `Error`
+   instance untouched. Verified by the
+   `does not mutate the original Error instance` test.
 
-The four originally-cited stale facts (`SETUP.md:11` `>= 1.3.8`,
-`DEPLOYMENT.md:30` `oven/bun:1.3.12`, `DEPLOYMENT.md:167` and
-`DEPLOYMENT.md:215` `src/app.ts:<line>` pointers) were already re-synced
-during the audience-first docs reorg in commit `a9c919d`, so no
-doc-prose edits were needed in this PR — only the structural CI gates
-that prevent them from rotting again.
+The optional plan item ("fold `redactValkeyUrl` into the logger
+config so the remaining ad-hoc call at `src/orchestrator/valkey.ts:33`
+stops diverging from the global policy") was implemented via the
+inline regex scrubber inside the err serializer rather than by
+importing the existing helper, to avoid introducing a
+`logger -> valkey -> logger` import cycle. The existing
+`redactValkeyUrl` call at line 33 (info path) is still wanted: it
+runs at startup _before_ the logger emits, on a value (`config.valkeyUrl`)
+that is otherwise safe to log structurally.

docs/operate/observability.md

@@ -2,6 +2,18 @@
 
 Structured JSON logs via [pino](https://getpino.io) are the primary signal. Every dispatch decision and every pipeline step carries a `deliveryId` so you can reconstruct a request end-to-end from a single log query. When `DATABASE_URL` is configured, the same information is persisted to `executions` and `triage_results` for aggregate reporting.
 
+## Log redaction
+
+The exported `logger` in `src/logger.ts` is the canonical chokepoint for secret scrubbing — every child logger inherits its `redact.paths` list and its custom `err` serializer, so individual call sites do not need to remember to scrub. Two layers run on every emitted line:
+
+1. **Path-based redaction** — the exported `REDACT_PATHS` constant in `src/logger.ts` lists every field pino should replace with `[Redacted]` before the JSON is serialised. Paths covered: `authorization` and its `*.authorization` / `headers.authorization` / `*.headers.authorization` / `req.headers.authorization` / `request.headers.authorization` variants; the webhook signature header `x-hub-signature-256` (also wildcard-prefixed); `response.data.token`; and the named credential fields `token`, `installationToken`, `privateKey`, `webhookSecret`, `anthropicApiKey`, `claudeCodeOauthToken`, `daemonAuthToken`, `awsSecretAccessKey`, `awsSessionToken`, `awsBearerTokenBedrock`, `*.password`. The list is `Object.freeze`d so an accidental `push` from another module cannot silently weaken the policy.
+
+2. **`errSerializer` scrubbing** — the exported `errSerializer` in `src/logger.ts` defers to pino's `stdSerializers.err` and then runs the result's `message`, `stack`, `request.headers`, and `response.data` through `redactGitHubTokens` (`src/utils/sanitize.ts`) plus an inline credential-URL scrubber that mirrors `redactValkeyUrl` (`src/orchestrator/valkey.ts`). The walker recurses through nested objects/arrays and replaces any key matching the sensitive-field-name set wholesale, so `err.response.data.meta.token` and `err.request.headers.forwarded.authorization` are caught at any depth — this is necessary because pino's path-based rules cannot match four-or-more segments deep on `err.*`. It also catches `ghs_…` installation tokens and App JWTs echoed inside `err.message` / `err.stack`.
+
+The serializer operates on a copy, so the original Error instance is never mutated.
+
+If you add a new secret-bearing config field to `src/config.ts`, add its property name to `REDACT_PATHS` in the same PR. The point helpers `redactGitHubTokens` and `redactValkeyUrl` remain in place for their non-log call sites (prompt sanitisation and the Valkey startup info log respectively); the logger config is the system-wide default.
+
 ## Common log fields
 
 | Field                                | Meaning                                                                                               |