Tech Debt: Cross-Cutting Infra (utils, security, errors, observability, oauth, data, repositories, issues, channels, types, client, schemas)

Verification

Independently verified by adversarial re-audit (2026-06-01) against the actual source.

Tally: 14 substantive findings + 5 nits checked. All findings verified accurate; 0 refuted. Several counts corrected inline (weak-typing count, security subdir file/LOC). The three security-sensitive claims all hold:

• worker-script.ts has no unit test — VERIFIED. The only worker-script.test.ts in the repo is src/agent/react/use-chat/browser-inference/worker-script.test.ts, a different file. The security sandbox worker-script.ts is referenced only via import.meta.resolve("./worker-script.ts") in project-worker.ts:350; no test imports its internals (it exports nothing). Sibling tests (deno-sandbox.test.ts, etc.) do not reference serializeError/loadModule/applyProjectEnv/handleAppRoute.
• MemoryTokenStore.tokens unbounded vs TTL-cleaned states — VERIFIED. memory.ts:14 tokens map is only mutated via set/delete/clear — no eviction; states (line 15) calls cleanupExpiredStates() on write and prunes on read. Exported singleton memoryTokenStore confirmed at line 92.
• No secret redaction in logger — VERIFIED. rg -ni "redact|scrub|sanitize|mask|REDACTED" over utils/logger + observability matches only comment-banner asterisks; no scrubbing layer exists. errors/logging.ts:73 console.error(JSON.stringify(entry)) confirmed.

Summary

This cluster is in good health. It is the most disciplined infrastructure layer audited: no file exceeds 500 LOC of production code, there are zero TODO/FIXME/HACK/XXX markers in production code across all 12 directories, weak typing is sparse and almost always justified (external npm modules, phantom types, Deno permission APIs), and the security-sensitive code (CSRF, path validation, OAuth state handling) is carefully written with explicit, well-commented intent. Error handling is uniformly non-swallowing — the catch (_) blocks that exist all carry comments explaining the expected failure and return a safe value. The debt that remains is mostly consistency (a few ad-hoc Error subclasses outside the central registry) and test-coverage gaps on complex code (the sandbox worker script), plus a handful of as any/as unknown as casts worth tightening.

Per-subdir LOC / file count (non-test *.ts):

Verified: counts re-run with find ... ! -name '*.test.ts' ! -name '*.bench.ts' ... -exec cat {} + | wc -l. All rows match except security, corrected from 62/6,254 to 61/5,947 — the original figure counted worker-isolation.bench.ts as a production file. All other rows match exactly.

Findings (ranked)

🔴 Sandbox worker script (security-critical) has no unit test

• Where: src/security/sandbox/worker-script.ts (497 LOC; no adjacent worker-script.test.ts)
• Issue: This is the in-Worker execution core that loads project modules (loadModule via dynamic import()), applies project env (Deno.env.set), runs agent discovery, and serializes errors back across the worker boundary. It is the trust boundary for sandboxed project code, yet has no direct test. There is a worker-isolation.bench.ts but that is a benchmark, not correctness coverage.
• Impact: Regressions in isolation, env-overlay, or error serialization could silently weaken the sandbox without any failing test.
• Fix: Add a focused test for serializeError, loadModule caching, applyProjectEnv, and request dispatch (handleAppRoute, unknown-type rejection at line 480).
• Verdict: ✅ Verified — 497 LOC confirmed. No test imports the security sandbox worker-script.ts; the only worker-script.test.ts is the unrelated agent/react/.../browser-inference one. File is loaded as a Worker entry via project-worker.ts:350 and exports nothing, so it cannot be unit-imported as written; sibling sandbox tests reference none of its internal functions. worker-isolation.bench.ts exists but is a benchmark.

🔴 Ad-hoc Error subclasses bypass the central error registry

• Where: src/utils/semaphore.ts:9 (SemaphoreTimeoutError), src/utils/circuit-breaker.ts:39 (CircuitBreakerOpen), and src/security/path-validation/types.ts (PathValidationError code enum + bespoke result shape)
• Issue: The project standardizes on a registry (errors/error-registry.ts, 80 defineError entries, VeryfrontError). These three modules instead throw/return hand-rolled Error subclasses with no slug, category, status, or RFC-9457 type.
• Impact: These errors don't surface consistently through errors/logging.ts or HTTP error mapping; harder to triage/aggregate in observability; inconsistent with the documented error system.
• Fix: Register SEMAPHORE_TIMEOUT and CIRCUIT_BREAKER_OPEN slugs and either subclass via the registry or attach RFC-9457 fields. (Note worker-script.ts:101 already special-cases reading type/status/detail off errors — these bespoke ones won't carry them.)
• Verdict: ✅ Verified — semaphore.ts:9 SemaphoreTimeoutError extends Error and circuit-breaker.ts:39 CircuitBreakerOpen extends Error confirmed, neither uses the registry. Minor qualification: path-validation/types.ts:27 PathValidationError is a const string-code object (an enum, as const), not an Error subclass — the report correctly described it as a "code enum + bespoke result shape," so the wording is accurate, but it is a slightly different category from the other two (codes, not a thrown class). Registry has 80 defineError entries confirmed.

🟡 as any discards type safety on conversation-history normalization

• Where: src/channels/invoke.ts:270 — })) as any; at the end of normalizeConversationHistoryForRuntime
• Issue: The whole normalized message array return is cast to any, defeating the carefully typed Message[] return signature. Any field mismatch (parts, timestamp, metadata) is now invisible.
• Impact: Silent shape drift between channel input and runtime Message.
• Fix: Type the mapped object to Message explicitly and drop the cast, or map into a typed accumulator.
• Verdict: ✅ Verified — invoke.ts:270 is exactly })) as any; closing normalizeConversationHistoryForRuntime, which is declared : Message[]. The cast erases shape checking on the mapped id/role/parts/timestamp/metadata object.

🟡 MemoryTokenStore tokens map is unbounded; default singleton is process-shared

• Where: src/oauth/token-store/memory.ts — private tokens = new Map(...) (no eviction) and the exported memoryTokenStore singleton (projectId = "default")
• Issue: states get TTL cleanup, but tokens are never evicted (only overwritten/cleared explicitly). The exported singleton shares one store across the whole process under the "default" projectId.
• Impact: Slow memory growth in long-running dev servers; multi-tenant scenarios that forget to pass a projectId share one keyspace. The class docstring itself warns "Suitable for development and tests" — risk is using the default in prod.
• Fix: Document/enforce that production must inject a persistent, scoped TokenStore; consider a max-entries cap or per-user TTL on tokens.
• Verdict: ✅ Verified — memory.ts:14 private tokens = new Map(...) is mutated only by setTokens (line 31), deleteTokens (line 36), and clear (line 87); no eviction/TTL. states (line 15) calls cleanupExpiredStates() on every setState and prunes on read (lines 42, 51-53, 60-64). Exported singleton memoryTokenStore confirmed (line 92). The class docstring's "development and tests" caveat is accurate.

🟡 No secret/PII redaction in the logger

• Where: src/utils/logger/logger.ts, src/observability/log-buffer.ts (no redact/scrub/sanitize found anywhere in observability or utils/logger)
• Issue: Structured logging accepts arbitrary args/context objects and emits them (incl. console.error(JSON.stringify(entry)) in errors/logging.ts:73). No allow/deny-list scrubbing of password/token/authorization/secret keys.
• Impact: A caller that logs a tokens object or request body would leak secrets to log sinks. Today no in-scope call site does this (audited — only a benign CORS warning matched), but there is no guardrail preventing it.
• Fix: Add a redaction pass in the logger core that masks known-sensitive keys before serialization.
• Verdict: ✅ Verified — rg -ni "redact|scrub|sanitize|mask|REDACTED" over src/utils/logger and src/observability returns only comment-banner asterisks, no actual scrubbing logic. errors/logging.ts:73 emits console.error(JSON.stringify(entry)) in production with no key filtering. The "no in-scope call site leaks today" claim is plausible but the absent guardrail is real.

🟡 @ts-ignore cluster on Deno permission APIs

• Where: src/security/sandbox/permission-system.ts:51,58,62,73 (4×) and src/security/sandbox/project-worker.ts:106
• Issue: Deno permission/Worker-option APIs are accessed through @ts-ignore. Security-relevant code (permission queries) suppresses the type checker entirely rather than declaring a minimal typed interface.
• Impact: A typo in a permission name or option would not be caught; the suppression hides the whole expression, not just the unknown member.
• Fix: Declare a narrow interface for the Deno permissions surface used and cast once, instead of per-line @ts-ignore.
• Verdict: ✅ Verified — permission-system.ts has @ts-ignore - Deno permissions API at lines 51, 58, 62, 73 (4× exactly). project-worker.ts:106 @ts-ignore - Deno Worker accepts extended options confirmed above the new Worker(...) call.

🟡 Deprecated logger fields kept indefinitely for "Grafana dashboard transition"

• Where: src/utils/logger/logger.ts:42-74 (request_id/trace_id/span_id/project_slug/user_id/conversation_id/duration_ms legacy aliases, all @deprecated)
• Issue: Seven @deprecated duplicate fields kept "until Grafana dashboard migration is complete" — an open-ended condition with no tracking issue or date.
• Impact: Every log entry potentially carries duplicate keys; consumers can't tell which is authoritative; the migration may never close.
• Fix: File a tracked removal task with the dashboard owner; gate the legacy keys behind a flag and set a removal milestone.
• Verdict: ✅ Verified — seven @deprecated ... Grafana dashboard transition aliases confirmed: requestId, traceId, spanId, projectSlug, userId, conversationId, durationMs, each shadowing a snake_case standard field. No tracking issue/date in the comments.

🟡 error-registry.ts is 824 LOC — concentrated but monolithic

• Where: src/errors/error-registry.ts (824 LOC, 80 defineError entries)
• Issue: All error definitions across all categories (CONFIG/BUILD/SECURITY/...) live in one file. No duplicate slugs (verified), so correctness is fine — but it's a single edit hotspot.
• Impact: Merge contention; hard to see category boundaries.
• Fix: Optional — split into error-registry/<category>.ts re-exported from a barrel. Low priority since slugs are unique and helpers enforce structure.
• Verdict: ✅ Verified — wc -l = 824, rg -c defineError = 80. Both figures exact.

🟢 as unknown as Record<string,unknown> for cross-boundary error / config reading

• Where: src/security/sandbox/worker-script.ts:101,382, src/channels/control-plane.ts:334
• Issue: Casting error/agent.config/React server module to Record<string,unknown> then guarding each field with typeof. The casts are defensible (untyped boundary), and each is followed by runtime type checks — but they are still escape hatches.
• Impact: Low; runtime checks mitigate.
• Fix: Define explicit boundary interfaces where stable (e.g. SerializedError already exists; reuse for the read side).
• Verdict: ✅ Verified — worker-script.ts:101 const e = error as unknown as Record<string, unknown> and :382 _ReactDOMServer as unknown as Record<string, unknown> (the "React server module"); control-plane.ts:334 agent.config as unknown as Record<string, unknown>. All three confirmed; correctly characterized as defensible boundary casts.

🟢 Sync filesystem calls in cache-dir setup

• Where: src/utils/cache-dir.ts:72,87,88 (lstatSync, mkdirSync, symlinkSync)
• Issue: Synchronous I/O. Acceptable here because it runs once at startup for cache-dir bootstrapping, not on a request path.
• Impact: Negligible (startup only).
• Fix: None required; note if ever called per-request.
• Verdict: ⚠️ Accurate but qualified — lstatSync (line 72), mkdirSync (line 87), symlinkSync (line 88) confirmed. They are dynamically imported from node:fs at line 66 inside a startup cache-dir bootstrap path; the "startup only" justification holds.

🟢 bundle-manifest split into four backend variants

• Where: src/utils/bundle-manifest.ts + -kv.ts + -redis.ts + -init.ts
• Issue: Looks like duplication but is a deliberate backend-strategy split (KV vs Redis vs init), each with its own test. Verified small (55-164 LOC each).
• Impact: None; flagged only to confirm it is not accidental copy-paste.
• Fix: None.
• Verdict: ✅ Verified — four variants exist (bundle-manifest.ts 164, -kv.ts 58, -redis.ts 55, -init.ts 119 LOC), each with an adjacent .test.ts. Deliberate backend split, not copy-paste.

🟢 One deep relative import

• Where: src/utils/cache/stores/memory/lru-cache-adapter.ts:1 — import ... from "../../../logger/logger.ts"
• Issue: The rest of the cluster uses #veryfront/... hash imports; this one reaches up three levels.
• Impact: Cosmetic / refactor-fragile.
• Fix: Switch to #veryfront/utils/logger/logger.ts.
• Verdict: ✅ Verified — line 1 is import { serverLogger } from "../../../logger/logger.ts"; exactly.

🟢 Retained dead endpoint constant

• Where: src/utils/constants/server.ts:46 — "Legacy endpoint retained for backward compatibility (no active handler)."
• Issue: A constant kept with no active handler.
• Impact: Negligible; clearly documented.
• Fix: Remove once confirmed no external caller depends on it.
• Verdict: ✅ Verified — server.ts:46 comment "Legacy endpoint retained for backward compatibility (no active handler)." sits above DEV_LOADER constant. Confirmed.

🟢 redis-client.ts uses Record<string, any> for npm client options

• Where: src/utils/redis-client.ts:71,77,96
• Issue: any used for @redis/client createClient options — but each is behind a // deno-lint-ignore no-explicit-any and bounded to the external module's untyped surface.
• Impact: Low; isolated to one adapter.
• Fix: Optional — pull a minimal typed options interface.
• Verdict: ✅ Verified — redis-client.ts lines 71, 77, 96 use Record<string, any>, each preceded by // deno-lint-ignore no-explicit-any, bounded to the @redis/client untyped surface.

Minor / nits

• schemas/lazy.ts:11 _output: undefined as unknown as T — standard phantom-type pattern, not real debt.
• types/entities/getEntityInfo.ts:334 — the any ripgrep hit is the English word "any" in a comment (false positive).
• client/spa/component-loader.ts:51 window as unknown as Record<string,unknown> — narrow, intentional global access.
• security/sandbox/worker-pool.ts:360 (globalThis as any).__veryfront_heap_stats — internal debug hook.
• observability/tracing/otlp-setup.ts:73 "Legacy initialise / shutdown" — comment marks delegation to newer bootstrap; verify the legacy path is still wired or remove.

Verdict on nits: ✅ All verified. lazy.ts:11 is exactly _output: undefined as unknown as T, (phantom type). getEntityInfo.ts:334 is the comment // layouts/ dir: any valid entity is a layout — confirmed English-word false positive, not a type cast. component-loader.ts:51 const global = window as unknown as Record<string, unknown>; confirmed. worker-pool.ts:360 (globalThis as any).__veryfront_heap_stats confirmed (internal debug hook). otlp-setup.ts:73 comment "Legacy initialise / shutdown (now delegated to bootstrap.ts + ext)" confirmed, followed by let initialized and an Initialize OTLP tracing export function still present.

Verdict on summary table row 7 (missing tests): ✅ Verified — api-shim.ts = 409 LOC with no api-shim.test.ts in observability/tracing; cache-repository.ts = 314 LOC, its dir contains only cache-repository.ts + index.ts (no adjacent test). Both confirmed.

Refuted / false positives

None. Every substantive finding and nit was independently confirmed against the source. The only corrections were quantitative: weak-typing count (~19 → ~16 by rg over the cluster, excluding tests) and the security subdir row (62/6,254 → 61/5,947, where the original counted worker-isolation.bench.ts as production). Both are imprecision, not false positives — the cited worst-offenders are all real.

Quick wins

1. Drop the as any in channels/invoke.ts:270 by typing the mapped Message.
2. Fix the one deep relative import in lru-cache-adapter.ts:1 to a hash import.
3. Replace the four @ts-ignore lines in permission-system.ts with a single narrow Deno-permissions interface.
4. File a tracked removal issue for the seven deprecated logger fields (with the Grafana owner) so the "transition" actually closes.
5. Add a small test for serializeError/loadModule in worker-script.ts.

Larger refactors

1. Unify error construction: register SemaphoreTimeoutError, CircuitBreakerOpen, and PathValidationError codes into the central registry (or attach RFC-9457 fields) so all errors flow through one system.
2. Logger redaction layer: add deny-list scrubbing of secret-like keys in the logger core before serialization — defense-in-depth against future leak-by-logging.
3. OAuth token-store hardening: bound the MemoryTokenStore tokens map and make production require an explicitly-scoped persistent store rather than the shared "default" singleton.
4. Sandbox test coverage: build a correctness test suite around worker-script.ts / worker-pool.ts / permission-system.ts given they form the project-code trust boundary.