review: security fixes, dead-code + duplication cleanup, convention reframing

[stevenn]

Output of a full codebase review (architecture/SoC, security, duplication, dead code). 7 commits, 98 files, +696/−549.

bun run verify green throughout: typecheck ×3, lint (432 files), 523 unit, 390 integration.

---

1. Security — five gaps closed (dcde5c4)

Each verified by reading the code, not taken on report. Three new tests lock them in.

• draft_skill leaked ungranted upstreams. It is open to every signed-in user but resolved slugs with a bare getUpstreamBySlug — exposing tool names, descriptions and input schemas for upstreams an admin never granted, and for tools hidden by the per-tool ACL. Now resolves against listUpstreamsVisibleToUser and filters with isToolAllowed, mirroring describe_upstream. Slugs are trivially discoverable (list_skills publishes them), so this was reachable.
• Prompt-injection via the drafter bundle. buildDraftSkillText inlines upstream tool descriptions into a template that opens with imperative first-party guidance — without the untrusted-text gate. An upstream could smuggle control characters and forge a ⟦ctxlayer⟧ segment, breaking the unforgeability invariant provenance.ts documents. sanitizeUntrustedText was file-local to tools-proxy.ts, which is precisely why this path could skip it; it now lives in provenance.ts.
• Collab WebSocket ignored account lifecycle. The upgrade verified only the cookie signature, so a suspended or deleted user kept read+write over the socket for the cookie's remaining 30 days. Now re-reads the row and requires status='active', matching resolveActiveUser on the REST path.
• Two upstream routes missing the visibility gate their sibling /:id/tools has. Beyond the existence oracle, PUT /:id/credentials warms the org-wide catalogue — an ungranted caller could replace shared upstream_tools rows with what their own token sees.
• resolveUserPrincipals was built three different ways (proxy, list_my_context, /api/tools). One home in db/queries/tool-access.ts so the ACL surfaces can't drift.

Plus (8b96821): control-char strip on tool results. An upstream reporting failure as { content, isError: true } never reaches the catch-path sanitiser, so results were the last path that could carry raw C0/C1 bytes to the model.

2. Dead code + duplication (7f40733, 3895505)

Every removal grepped across apps/, packages/, scripts/ first.

• getIncomingLinks/getOutgoingLinks — test-only, with docstrings claiming production roles ("move-rewrite driver", "for the editor panel") that getIncomingLinkDocs/getOutgoingLinkTargets actually fill. The test now drives the real readers.
• listGitConnections, DocLinkRow, FolderTreeResponse (leftover from a removed endpoint — there is no GET /api/folders), the ASSETS binding (env.ASSETS never called), scripts/slug-prefix-migration.sql, .info-strip CSS, 10 never-read custom properties.
• USAGE_ERROR_CODES was documented as "exported for the SPA's label/colour maps" but had zero importers — errors-table.tsx hand-wrote the same 7 keys. Now keyed by the shared UsageErrorCode, so a new server-side class is a compile error instead of a silent fall-through.
• newId ×16 → 1 (spelled newId/newUlid/newRevisionId by folder), errMessage ×37 → 1, errText/safeJson ×3 → 1, bodyMessage ×5 → 1, formatAbsolute → existing absDateTime.
• personLabel moved to lib/person.ts — it was re-exported from the docs-list route module, so the editor and an admin page imported a lazy-loaded route chunk just to format a name.
• noUnusedLocals + noUnusedParameters on. Biome's noUnusedVariables doesn't report unused imports and nothing else in verify reported unused anything — which is how this accumulated. Exactly one violation repo-wide; it then caught six orphaned imports the cleanup itself created.

3. Deployment-specific references removed (3afd79d)

All yuki/yukitools/dizzydata strings gone (4 source comments, 10 test files), replaced with this repo's existing acme + reserved .example convention. Each fixture keeps its shape so the tests still assert what they did — the multi-hyphen kebab slug for the tool-name regex, mixed case for the CF-Access lowercasing test, a distinct workers.dev host for the self-loop guard.

Two real bugs found there:

• ctxlayer-jobs was never provisioned. It's a live producer and consumer binding (async submit→poll) but was missing from bootstrap's QUEUES.
• patch() silently dropped provisioned ids. It returned false on a miss, and both patterns required a # <TODO> marker wrangler.toml has never carried — while PLACEHOLDER_RE looks for an all-zeros id. Bootstrapping from a zeroed id would create the resource, print the id, then throw it away: an orphan on the account and a config still on the placeholder. Now exits non-zero.

4. Convention: one concern per file (e4cfef5, 31e1658)

The ~200 LoC cap was stated in five places and had stopped carrying signal — 75 of 322 files exceeded it, holding 54% of all code. That's the norm, not an outlier set.

Reframed: one file = one concern is the rule; ~300 LoC is a review trigger, not a limit — a prompt to ask "is this still one thing?". Cohesive units are exempt at any length; splitting a standalone component to hit a number buys prop-drilling across a boundary that isn't there. E6 now names worked examples both ways (keep doc-room-do.ts, reindex-consumer.ts; split db/queries/docs.ts, tools-proxy.ts, docs-editor/index.tsx) and records that the number is a weak proxy — in this review it predicted none of the findings that mattered.

5. Docs corrected (8b96821)

• .claude/commands/seed.md actively misled: it promised "a Notion HTTP upstream, a second streamable_http upstream, and three demo docs". seed.mjs seeds three teams and two products and deliberately seeds no docs. CLAUDE.md repeated the claim.
• seed.mjs still said "M4 will add…" (M1–M8 retired); git-sources.ts listed git_source_visibility, replaced in migration 0030.
• CLAUDE.md security rules updated so code and doc stop contradicting each other, and gain the two rules this branch established: sanitise on every path, and gate agent-facing upstream reads twice.

---

Known gap, deliberate

Upstream isError result text is still forwarded to the agent (and replayed by poll_task). It's control-char stripped and provenance-defanged but not credential-scrubbed. This is tested behaviour and forwarding tool errors is functionally necessary — CLAUDE.md now records the gap instead of asserting a rule the code contradicts. Whether to scrub is an open decision, tracked in docs/review-2026-07.md.

Not in this PR

docs/review-2026-07.md (a dated snapshot backlog, not a maintained plan) carries what's left: four more open security items (destructive GET without origin check; redirects carrying Authorization; unbounded res.text() on git fetches; no consent step on /oauth/authorize + open DCR), the parked structural refactor (~1000 LoC of duplication — makeRevisionQueries, adminCrudRouter, splitting tools-proxy.ts, SPA ResourceListPage), the packages/shared zero-test gap across ~3000 LoC including the url-trust.ts security boundary, and two product decisions (skill_tags is a write-never table; documents.kind is dead end-to-end).

Worth flagging for review: PKCE enforcement, redirect_uri matching and auth-code single-use are delegated to @cloudflare/workers-oauth-provider, which isn't vendored — they could not be verified in-repo.