Skip to content

fix(detection): stop quarantining ordinary English; close the Mode B PHI gap; remove 6,611 dead lines - #329

Open
thecelestialmismatch wants to merge 3 commits into
mainfrom
claude/codebase-quality-review-f1j6rp
Open

fix(detection): stop quarantining ordinary English; close the Mode B PHI gap; remove 6,611 dead lines#329
thecelestialmismatch wants to merge 3 commits into
mainfrom
claude/codebase-quality-review-f1j6rp

Conversation

@thecelestialmismatch

Copy link
Copy Markdown
Owner

Summary

A code-quality and maintainability review of the whole repo. It set out to find dead code and found two live detection defects on the way — those are the reason this PR leads with fix(detection) rather than chore(cleanup).

1. The shipped proxy was quarantining ordinary English. Task order / delivery order carried bare TO/DO in a case-insensitive alternation with no closing \b, so it matched inside "tomorrow", "tonight", "document", "download", "together". Under the i flag its [A-Z0-9]{4,} identifier also matched any word, so "to production" and "to review" read as order numbers. Verified end-to-end through the real scanMessages(), not a transcribed regex:

Prompt Before Matched on After
Going to production on Thursday QUARANTINE / HIGH to production ALLOW
We need to document the rollback steps QUARANTINE / HIGH to document ALLOW
Please download the onboarding document QUARANTINE / HIGH download ALLOW
Can we move the standup to 4 PM tomorrow? QUARANTINE / HIGH tomorrow ALLOW
What is the total spend to date? QUARANTINE / HIGH to date ALLOW
Summarize our CAGE code 1ABC2 contract BLOCK / CRITICAL CAGE code BLOCK / CRITICAL

10 of 10 ordinary sentences were held at HIGH risk. All 10 now pass; all 5 real task-order forms (TO 0001, delivery order N0001923D0001, task order no. 88213) still fire.

2. The two registries had drifted, against the deployment that matters. Health plan beneficiary number diverged: Mode B — the deployment the CUI/HIPAA claim rests on — recognised neither subscriber, policy nor group identifiers, so the shipped proxy let through 5 PHI strings the hosted demo blocked. Both registries now carry the byte-identical union, narrowed on noise (the identifier must contain a digit; Policy number pending matched before).

PM was not ported to the proxy — under i it fires on every clock time. It became a case-sensitive Program manager designator rule with a digit lookbehind, added to both registries (proxy 33 → 34, app 53 → 54; all 9 doc references updated, as the repo's own doc-counts guard required).

3. The guard that should have caught this said it could not. registry-drift.test.ts compared declared names as source text and its own header stated the ceiling: "two patterns sharing a name with different regexes still pass." It now imports both registries and compares behaviour across a 41-string corpus, with 15 regression anchors that must stay clean in both engines.

The maintainability findings — 6,611 unreachable lines, 95 removable packages, an abandoned scaffold — are in the same PR because the doc corrections they force overlap the detection docs. Full findings: docs/audit/DEAD-WEIGHT-AUDIT-2026-09-03.html.

Closes #

Change type

  • Bug fix
  • New capability
  • Security hardening
  • Documentation or developer-experience improvement
  • Refactor or maintenance
  • Breaking change

Impact and operating considerations

  • Authentication, sessions, identity, or authorization
  • Sensitive-data handling, logging, telemetry, or outbound requests
  • Proxy detection, policy evaluation, block/quarantine behavior, or performance
  • Database schema, migrations, retention, or access controls
  • Deployment, environment variables, integrations, or scheduled work
  • Evidence, reporting, or audit-chain behavior
  • None of the above

Operational impact, threat-model note, and rollback plan:

Detection behaviour changes in both directions, deliberately. It widens for PHI (Mode B now blocks 5 identifier forms it previously allowed) and narrows for the task-order rule (it no longer fires on prose). The narrowing is the larger behavioural change: any deployment currently relying on that rule's output will see far fewer quarantines — which is the point, since 10/10 sampled benign sentences were tripping it. Both registries were changed together and are now asserted byte-identical in behaviour by the parity guard, so the two planes cannot diverge again silently.

Pattern floor respected. CLAUDE.md's extend-never-replace rule and the CI Compliance Pattern Guard (ALL_PATTERNS.length >= 33) both hold: the count went up, to 34. Scan latency is unchanged — p99 0.693 ms against a 10 ms budget — despite the added lookaheads.

Rate limiting. /api/chat moved from the per-process limiter to the Postgres-backed shared limiter. It falls back to the server's own OPENROUTER_API_KEY, and the in-process Map gave a real ceiling of (10 × live instances), reset on every cold start — the exact spend exposure rate-limit-shared.ts was written to close. Requires migration 028_rate_limit_buckets.sql to be applied in production; until it is, the shared limiter fails open to the in-process limiter and flags degraded: true, so behaviour is no worse than today, never worse for a paying caller.

Health endpoint. /api/admin/health now returns real diagnostics. It stays admin-gated and fail-closed (404 for anonymous and non-admin, asserted in new tests), and buildHealthReport() is value-free by construction — every signal derives from the shape or presence of configuration, never its content. The public /api/health is untouched and still discloses nothing.

Rollback. Single commit, revertable as one. No migrations, no schema changes, no environment variables added or renamed. Reverting restores the previous detection behaviour, including both defects.

Validation

  • Web plane: npx tsc --noEmit
  • Web plane: npm run lint
  • Web plane: npm run test:coverage
  • Web plane: npm run build
  • Proxy: npm run lint
  • Proxy: npm run test:coverage
  • Proxy: npm run bench
  • Manual verification (described below)
  • Documentation-only validation

Results and manual verification:

Gate Result
Web tsc --noEmit exit 0
Web lint 0 errors, 39 warnings (unchanged from the documented baseline)
Web test:coverage 3,114 passed / 222 files, exit 0
Web coverage statements 38.23%, branches 36.29%, functions 35.03%, lines 38.61% — all above the 25% ratchet, and up from the 29.36% recorded in vitest.config.ts, because removing unreachable code removes uncovered lines from the denominator
Web build exit 0; ƒ Proxy (Middleware) present
Proxy lint exit 0
Proxy test:coverage 92 passed, statements 70.54%
Proxy bench p99 0.693 ms vs 10 ms budget — PASS
scripts/verify-structure.mjs PASS, 31 paths, 0 problems
npm ci after dependency removal clean, 861 → 766 packages

Baseline before any change was 223 files / 3,099 tests. The net file count drops because dead tests were removed with the code they covered; the net test count rises because new guards were added.

Manual verification was done by running prompts through the real proxy/scanner.ts entry point rather than reasoning about regexes — which is also how the task-order defect was confirmed to be a whole-prompt verdict (the scanner applies every pattern with no context gating and takes the strongest action). The before/after table above is that run. The 8 PHI strings were re-checked the same way: 0 of 8 still leak in Mode B.

Review checklist

  • The change is focused and does not include unrelated refactoring.
  • Tests cover changed behavior, or the omission is explained above.
  • No credentials, customer data, sensitive prompts, or production exports were added.
  • Public claims are scoped, evidence-based, and consistent with the selected deployment boundary.
  • Documentation and configuration guidance were updated where needed.
  • Database migrations, configuration changes, and rollout dependencies are documented where applicable.
  • I performed a self-review and addressed obvious failure paths.

Notes for the reviewer

Scope. This is larger than a single fix because the pattern-count change forced doc updates across 9 files, and the repo's doc-counts guard fails the build until every one is corrected. The dead-code removal is separable in principle; it is here because it touches the same rule and skill files.

Deliberately left open, and why. lib/gateway/zero-trust.ts, lib/cache/swr-cache.ts and lib/auth/reset-password-state.ts are built, tested and imported by nothing. They stay, each now carrying an explicit ponytail: note naming the decision needed. Removing an unwired deny-by-default access control is a founder call, not cleanup, and wiring one into the live gateway is a behaviour change deserving its own review.

Not acted on from static analysis. 20 API routes have no in-repo caller, but some of them are the product — /api/v1/* is the endpoint customers point their baseURL at, and /api/events/ingest is called by proxy/webhook.ts in a different package. That triage needs access logs, not an import graph, so nothing was deleted there.

Five false positives were caught and excluded before they reached the report, and are recorded in the audit so nobody re-derives them: demo-telemetry-seed.ts (loaded by a runtime-built path in a script), lib/rate-limit.ts (the deliberate fail-open fallback inside the shared limiter), the 8 lib/agent/tools/* files (side-effect registered, dynamically imported), providers/types.ts (stream-scanner.ts imports ScanAlert from it), and setup-stripe.mjs (a one-shot ops script, unreferenced by design).

🤖 Generated with Claude Code

https://claude.ai/code/session_018GK6NpeRNRpdzRrEP9vuyM


Generated by Claude Code

A maintainability audit of the repo turned up two live detection defects
before it got to the dead code it was looking for. Both are fixed here, and
the guard that should have caught them is upgraded so it can.

1. The shipped proxy was quarantining ordinary prose.
   `Task order / delivery order` carried bare TO/DO in a case-insensitive
   alternation with no closing \b, so it matched INSIDE "tomorrow",
   "tonight", "document", "download", "together"; and under the `i` flag its
   [A-Z0-9]{4,} identifier matched any word, so "to production" and
   "to review" read as order numbers. Verified end-to-end through the real
   scanMessages(): 10 of 10 ordinary sentences QUARANTINEd at HIGH risk.
   All 10 now ALLOW; all 5 real task-order forms still fire.

2. The two registries had drifted, against the deployment that matters.
   `Health plan beneficiary number` diverged: Mode B recognised neither
   subscriber, policy nor group identifiers, so the shipped proxy let
   through 5 PHI strings the hosted demo blocked. Both registries now carry
   the byte-identical union, narrowed on noise (the identifier must contain
   a digit — "Policy number pending" matched before).

   `PM` was NOT ported to the proxy: under `i` it fires on every clock time
   ("move the standup to 4 PM tomorrow"). It became a case-sensitive
   `Program manager designator` rule with a digit lookbehind, added to both
   registries. Proxy 33 -> 34, app 53 -> 54; all 9 doc references updated.

3. The drift guard's own header said it could not see this — "two patterns
   sharing a name with different regexes still pass". It now imports both
   registries and compares BEHAVIOUR across a 41-string corpus, with 15
   regression anchors that must stay clean in both engines.

Also in this change, from the same audit:

- Wired lib/health/service-status.ts and lib/auth/reset-diagnostics.ts
  (516 lines, built and tested, imported by nothing) into the admin-gated
  /api/admin/health, which now reports `degraded` and names the failing
  keys. CLAUDE.md's Session Start Protocol told every session to curl
  /api/health for integration health; that endpoint returns a hardcoded
  `ok`. Corrected, with 9 new tests.
- Moved /api/chat to the Postgres-backed shared rate limiter. It falls back
  to the server's own OPENROUTER_API_KEY and was limited to 10/min PER
  INSTANCE — the exact spend exposure rate-limit-shared.ts exists to close.
- Deleted 38 files / 6,611 lines unreachable from any of the 427 entry
  points, including the lib/gateway/providers registry that called itself
  "the single entry point for the streaming proxy" while stream-proxy.ts
  re-implemented the same table inline.
- Removed 9 unused dependencies: 95 packages, 67.5 MB, including a full
  react-native + expo tree. npm ci 861 -> 766 packages.
- Deleted an untouched create-next-app scaffold at the repo root: a
  tsconfig mapping @/* to a non-existent ./src/*, a postcss config needing
  a Tailwind v4 package nothing installs, and a yarn.lock beside a
  package-lock named "nostalgic-raman-26ee6a". No CI job built at root.
- Updated every doc, rule and skill naming a deleted file. One handed
  agents an import example that would no longer compile; CLAUDE.md's first
  Critical Rule was about a dead component, and is now stated generally
  (any Recharts component stays ssr:false).
- zero-trust.ts, swr-cache.ts and reset-password-state.ts are built but
  unwired. They stay, each carrying an explicit `ponytail:` note naming the
  decision needed — removing an unwired deny-by-default access control is a
  founder call, not cleanup.

Gates: app 3,114 tests / 222 files · proxy 92 · tsc 0 · lint 0 errors
(39 warnings, at baseline) · build EXIT 0 · scan p99 0.684ms against a 10ms
budget · verify-structure PASS.

Full findings: docs/audit/DEAD-WEIGHT-AUDIT-2026-09-03.html

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018GK6NpeRNRpdzRrEP9vuyM
@vercel

vercel Bot commented Sep 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
compliance-firewall-agent Ready Ready Preview Sep 3, 2026 11:04pm UTC

@supabase

supabase Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project qifynzuyrdxmxlumpsrq because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

Copy link
Copy Markdown
Owner Author

⚠️ Substantial overlap with #327 — read before merging either

#327 (claude/houndshield-presale-audit-eepk9u, open, CI green) contains a commit that deletes the same 23 files and the same 9 dependencies as this PR, from an independent session. Both arrive at npm ci 861 → 766. That work is genuinely duplicated and only one copy should land.

Identical in both: openclaw-templates.ts · PlatformDashboard + PlatformDashboardClient · WhyHoundshield · OperationalReadiness · lib/gateway/providers/{index,openai,anthropic,google,openrouter} · lib/integrations/siem/* (5) · memory-dna.ts · firecrawl-updater.ts · app/blog/posts/data.ts · lib/secret.ts · components/{LocalizedPrice,FloatingNav,SectionSpotlight,ScrollProgress} · components/ui/{AnchorBadge,CodeBlock,ComparisonFlow,CountdownTimer,PricingToggle,ThreatFeed} · the 9 unused deps.

Only in this PR (#329):

  • The detection fixes. Hash-chain the Mode B audit log, make the licence enforceable, restore the readiness gate #327 touches neither proxy/patterns/index.ts nor lib/classifier/*. The task-order rule that quarantined 10/10 ordinary sentences, the Health plan beneficiary number drift that left Mode B detecting 5 PHI strings fewer than the hosted demo, the new case-sensitive Program manager designator rule, and the behavioural parity guard that replaces name-comparison in registry-drift.test.ts — all exist only here.
  • /api/chat moved to the Postgres-backed shared rate limiter.
  • The abandoned create-next-app scaffold at the repo root (root tsconfig.json, next.config.ts, postcss.config.mjs, yarn.lock, root package-lock.json, two unrunnable eslint configs, the 5 default SVGs).
  • Navbar.tsx (537 lines, superseded by NavV3) and FeaturesGrid.tsx, plus the committed .claire/ and .playwright-mcp/ strays.
  • Doc corrections for every deleted file, including the two source comments still asserting middleware does not run in production.

Only in #327: the Mode B hash chain, proxy licence enforcement, Docker hardening, scripts/find-orphans.mjs, and a token-gated /api/health/ready.

On the health module specifically — #327's approach is better for the documented use case

Both PRs independently found that lib/health/service-status.ts was built, tested and imported by nothing while CLAUDE.md told every session to curl /api/health for integration health. This PR wires it into /api/admin/health. #327 adds a token-gated /api/health/ready and argues, correctly, that "/api/admin/health could not serve this: it requires an authenticated browser session, so it is unusable from curl in a session start protocol."

That critique lands. If #327 merges first, take its /api/health/ready for the Session Start Protocol; the change here is still additive (a signed-in admin gets real state from the admin endpoint instead of a hardcoded ok), but it is not the better answer to the curl workflow and the CLAUDE.md wording should follow #327's.

Suggested order

Merge #327 first — it is older, not a draft, and already CI-green. Then this branch rebases onto main: the duplicated deletions no-op, and what remains is the detection work, the rate limiter, the root scaffold and the doc corrections. I'll do that rebase once #327 lands rather than resolving a conflict against a moving base now.

CI status

No GitHub Actions run has been created for this PR. That is not specific to this branch — no workflow run has been created anywhere in the repo since 21:38 UTC, and this PR opened at 21:55. ci.yml has no draft gating (no if: conditions), so draft status is not the cause. Nothing to fix in the diff; it looks like an Actions queue or account-level pause. Vercel did build the preview successfully (Ready).

Every gate was run locally instead, and the results are in the PR body: app 3,114 tests / 222 files, coverage 38.23% statements (up from 29.36%), proxy 92, tsc 0, lint 0 errors, build exit 0, scan p99 0.693 ms, verify-structure PASS.


Generated by Claude Code

…dit gate

The Security Audit job (`npm audit --omit=dev --audit-level=high`) went red on
this branch. One advisory gates it:

  fast-uri  3.0.0 - 3.1.5  (high, 4 advisories)
  node_modules/fast-uri, via ajv under ajv-formats and schema-utils

It is a consequence of this PR, not a pre-existing failure: removing the nine
unused dependencies re-resolved the tree, and the new resolution landed on
fast-uri 3.1.5 where the old one had not. PR #327 hit the identical advisory
from the identical cause.

`npm audit fix` could not resolve it — the version is pinned through a
transitive parent — so this adds a lockfile-level override instead:

  "overrides": { "fast-uri": "^3.1.7" }

3.1.7 is outside the vulnerable range, and `^3.1.7` sits inside ajv's own
declared `^3.0.1`, so nothing is forced past a dependency's stated
compatibility. The change is 3 lines of lockfile (3.1.5 -> 3.1.7) plus the
overrides block; no direct dependency version moved, and `npm install`
reported "changed 1 package".

NOT fixed here, deliberately: `fflate` (moderate) resolves to 0.4.8 through
`posthog-js@^0.4.8`. It does not fail this gate, which is `--audit-level=high`.
Overriding it to 0.8.3 would push posthog-js outside its own declared range,
on a library that runs on the live marketing site, to clear an advisory that
blocks nothing. That trade is not worth taking without a reason to.

Validated: tsc 0 · 3,114 tests / 222 files · build EXIT 0. fast-uri feeds ajv
-> schema-utils -> webpack, so the build is the check that matters here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018GK6NpeRNRpdzRrEP9vuyM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants