# Full-codebase audit — remediation plan (2026-07-20)

Source: adversarial review (3 Codex reviewers — Skeptic/Architect/Minimalist — per the
adversarial-review skill) + independent verification of every finding + objective checks.
Baseline at commit `803b98d`: harness 812 pytest pass (l0/l2 deselected), backend 228,
frontend 309, both `tsc` clean, **ruff 34 errors** (all in `harness/tests`).

Verdict: **REJECT** (per skill verdict logic: high-severity findings with multi-reviewer
consensus). The app is functional and green on tests, but ships 5 confirmed high-severity
defects. Nothing here is data-destroying in the happy path; the highs are silent-wrong-state
and workflow-integrity classes.

Every finding below was re-verified in the current code by the lead (line numbers checked).
Two reviewer findings were **rejected** in lead judgment and are listed at the bottom.

---

## FASE 1 — Pi-crash class closure (gate) — ~1h, no behavior change

**1.1 [HIGH] Five gate tools can still crash Pi on an uncaught throw.**
`harness/.pi/extensions/tht-gate.js` — `reviewer_memory_promote` (execute at ~1343),
`rewrite_question` (~1431), `write_schema_linking` (~1484), `write_cte_sql` (~1511),
`write_final_sql` (~1527). Same class as the crash fixed in `87cb806` for the four
`reviewer_*` tools: `currentPhase()`/`phaseId()`/`tht()` run outside any try/catch; a
throw rejects the async execute → Node unhandled rejection → Pi dies mid-session.
FIX: wrap each execute body in the same top-level try/catch → `textResult` pattern.
VERIFY: `node -c`; grep-test asserting every `registerTool` execute has a top-level try.

## FASE 2 — Workflow integrity (single-source the phase truth) — ~half day

**2.1 [HIGH] WorkflowBar phase labels are fiction from F2 onward.**
`frontend/src/shell/WorkflowBar.tsx:5-29` hardcodes
`F2 "Schema linking" / F3 "Exploration" / F4 "SQL plan" / F5 "SQL generation" / F6
"Validation" / F7 "Review"`, while `harness/workflow.yaml` defines
`F2 memoria / F3 riscrittura / F4 schema_linking / F5 sintesi / F6 cte / F7 sql_finale`.
Every live session shows the wrong phase name to the reviewer from F2 on.
FIX (minimal): correct the two static maps to the canonical sequence.
FIX (right): backend `GET /workflow/meta` shelling `tht phase meta --json`, frontend
fetches once per app load with the corrected static map as fallback.
VERIFY: unit test mapping `tht phase meta --json` ids/names ↔ rendered labels.

**2.2 [HIGH] Gate `forceAdvance` contradicts the canonical SKILL.md.**
`harness/.pi/skills/tht-sessione/SKILL.md:30-32`: "`advance:true` auto-advances only F2
(empty memory) and F6 (skipped/empty) — never a phase that recorded substantive
decisions." But since `6ee5bda`, `reviewer_decide` and `reviewer_schema_linking` with
`advance:true` **force-advance any phase** after persisting decisions
(`tht-gate.js` `forceAdvance`, used at ~895 and ~1002). The model reads one contract and
the gate implements another → nondeterministic workflow shape (phases may skip their
`reviewer_confirm kind:"phase"` summary gate depending on what the model passes).
The force-advance behavior was a deliberate product choice (kill the redundant approve
form); the defect is the contradiction, not the feature.
FIX: (a) restrict `forceAdvance` to the two designed auto-close gates
(`reviewer_schema_linking` in F4, `reviewer_memory_promote` in F8 via
`closeAfterPromotion`); `reviewer_decide` returns to `advanceIfReady` (exit-6 no-op);
(b) rewrite SKILL.md §"Phase map (advance cheat-sheet)" to document exactly which gates
auto-advance; (c) add the allowlist as data in `workflow.yaml` (e.g. `advance:
gate_closes`) so gate and skill read one source.
VERIFY: L1 test on the allowlist; live smoke on psd (F4 auto-advances, F5 does not).

## FASE 3 — Transport correctness (SSE) — ~half day

**3.1 [HIGH, 3/3 reviewer consensus] Stale `Last-Event-ID` from an older backend
generation is honored when ids collide.**
`backend/src/sse/sse-hub.ts:55-63`: the stale-cursor guard only catches
`cursor > lastId`. After a backend restart, ids restart at 1; a browser auto-reconnect
carrying cursor N from the old process silently suppresses the new generation's events
1..N (different content, same ids). Contract 11 in `brain/codebase/workflow-ui-contracts.md`
is only half-implemented.
FIX: make the event id a composite `"<generation>:<seq>"` (generation = process-start
epoch, e.g. `Date.now()` at Hub construction). `Last-Event-ID` is opaque to EventSource,
so no frontend change: on subscribe, parse the cursor; generation mismatch → treat as 0.
VERIFY: regression test — two Hub instances, cursor from A replayed against B must
replay from the beginning.

**3.2 [MEDIUM] Transport state never evicted for finished sessions.**
`sse-hub.ts:41-44` — `buffers`/`lastIds` grow per session; `forget()` is called only by
the delete route. Long-lived deployments accumulate state for every session ever touched.
FIX: call `hub.forget(id)` when a session reaches a terminal state (finalized+agent_end,
archived) — the transcript UI reads persisted documents, not the SSE buffer.
VERIFY: unit test — finalize → maps empty for that id.

## FASE 4 — Process & route robustness (backend) — ~half day

**4.1 [MEDIUM] `spawnFor` leaks a live child + registered runtime when `configure()`
rejects.** `backend/src/pi/pi-process-manager.ts:177-182`: `createFor` registers the
runtime; if `configure` (set_model/set_thinking RPC) rejects, `spawnFor` throws without
teardown → the map keeps an apparently-active runtime with a live Pi child; every later
start gets "session runtime already active".
FIX: try/catch in `spawnFor` → identity-checked teardown + rethrow.
VERIFY: unit test with a configure that rejects.

**4.2 [MEDIUM] No timeout on any `tht` call except `dbPing`.**
`backend/src/tht/tht-runner.ts:67-101`: `run()` supports `timeoutMs` but only `dbPing`
passes one. A DWH/vector op that hangs (VPN drop mid-call: `sql preview`, `search pack`,
`ollama ensure`) wedges the HTTP request forever.
FIX: default timeout in `run()` (60s) + explicit per-call values (dbPing 10s,
sqlPreview/searchPack 120s). On timeout the existing code-124 path already SIGKILLs.
VERIFY: unit test with a sleeping fake bin.

**4.3 [MEDIUM] Missing workspace YAML silently falls back to the default config.**
`tht-runner.ts:51-56`: `configArg("typo")` returns the default `-c` when
`workspaces/typo.yaml` doesn't exist → sessions/operations silently target the wrong
workspace (wrong DB, wrong sessions dir).
FIX: throw on a named-but-missing workspace; let routes surface 500 with the message.
VERIFY: unit test.

**4.4 [MEDIUM] Resume returns `200 alreadyActive` before the finalized/archived 409.**
`backend/src/routes/sessions.ts:288-296`: the runtime fast-path short-circuits the
read-only contract. A finalized session with a lingering running runtime resumes as if
live.
FIX: evaluate the manifest 409 first; only then the alreadyActive fast-path.
VERIFY: existing route test extended (finalized manifest + fake active runtime → 409).

**4.5 [LOW] `ollamaEnsure` treats exit-0 with unparseable stdout as ok.**
`tht-runner.ts:204-206`. FIX: on code 0 require parsed JSON (else ok:false with detail).

**4.6 [LOW] `respond()` sends stale gate responses with the wrong Pi RPC id.**
`backend/src/bridge/session-bridge.ts:110-117`: a retried response for gate A while B is
pending goes out with B's `pendingPiId`; Pi's descriptor-id check re-loops it (transient,
self-healing). FIX: drop the response unless `pending && uiResponse.id === pending.id`.

## FASE 5 — State integrity (harness + gate persistence) — ~1 day

**5.1 [MEDIUM] `phase reopen` deletes artifacts before recording `phase_reopened`.**
`harness/tht/cli/phase_cmd.py:124-128`: crash between `teardown_snapshot()` and
`append_decisions()` → artifacts of later phases deleted but ledger still at the old
phase; resume enters a phase whose expected artifacts are gone.
FIX: append `phase_reopened` FIRST, then teardown, and make teardown idempotent +
re-runnable: on session load, if the folded phase is behind surviving later-phase
artifacts, re-run the teardown repair pass. (Ledger-first means the half-state is
"reopened with stale extra artifacts", which the repair pass cleans deterministically.)
VERIFY: unit test simulating the crash window (teardown raises after ledger append).

**5.2 [MEDIUM] Schema-linking approval persists N decisions non-atomically.**
`tht-gate.js` ~967-1000: one `tht decision add` per table/column; a transient failure
mid-loop returns an error with the ledger half-written; a retry re-adds the first K
decisions (duplicate entries; projection is set-based so the artifact survives, but the
audit ledger lies).
FIX: batch the whole review into one CLI call (`tht decision add-batch --doc -` on the
model of the existing `add-join-set`), atomic single append.
VERIFY: L1 test; retry after injected failure produces no duplicates.

**5.3 [HIGH] Anti-bypass hook does not cover bash writes to protected state.**
`tht-gate.js:141-158` + hook 550-585: `FORBIDDEN` blocks specific `tht` subcommands and
`GATE_CODE_FILES` blocks the `write`/`edit` TOOLS, but plain bash can still mutate
protected state: `echo '{"type":"phase_approved"...}' >> review_decisions.jsonl`,
`sed -i` on `session_manifest.yaml`, `cat > .pi/extensions/tht-gate.js`.
This is defense against a *confused* model (it has already edited its own gate once —
see memory), not a hostile one; perfect sandboxing is out of scope.
FIX: extend the bash branch of the tool_call hook with a protected-path pattern: block
any bash command whose text references `review_decisions.jsonl`, `session_manifest.yaml`,
`cte_plan.json` or `.pi/extensions/` in a mutating context (`>`, `>>`, `tee`, `sed -i`,
`mv`, `cp`, `rm`, `python … open(...,'w')`). Read-only mentions (cat/grep) stay allowed.
VERIFY: L1 tests on the pattern (block list + allow list).

## FASE 6 — Docs & hygiene — ~1h

**6.1 [MEDIUM] CLAUDE.md/PROJECT_STATE describe a storage model that no longer exists.**
CLAUDE.md says "backend … no database", "App settings live in a JSON file
(`backend/data/settings.json`)". Since the portable-deployment merge the truth is:
`harness/tht/session/repository.py:69-86` selects **PostgresSessionRepository** when
`session_storage` is configured (server mode) vs filesystem; backend settings go through
harness preferences (`backend/src/app.ts:61-80`), with the JSON file as fallback only.
FIX: update CLAUDE.md architecture bullets + PROJECT_STATE.md; add one line on when each
repository/settings path is active.

**6.2 [LOW] ruff: 34 errors in `harness/tests` (19 E702, 13 F401, 1 F841, 1 F541).**
FIX: `ruff check . --fix` (14 auto), hand-fix the E702 semicolons. Add ruff to whatever
gate runs before commits (it exists in dev deps; it just isn't enforced).

**6.3 [LOW] Replay server drifts from the real REST surface.**
`tools/replay/server.mjs`: `/me` is missing (SPA calls it on boot; replay serves the SPA
HTML → JSON parse noise); resume-shape drift was just fixed (`803b98d`) — audit the
remaining routes against `backend/src/routes/*.ts` and stub what the SPA actually calls.
VERIFY: replay boot with devtools console clean.

**6.4 [LOW] `failSession` failures are silently discarded (deliberate).**
`backend/src/routes/sessions.ts:111`: keep the swallow (crash-path best effort) but add a
`console.error` so a storage outage is at least visible server-side.

---

## Rejected reviewer findings (lead judgment)

- *Minimalist:* "`harness/README.md` references `docs/testing.md` which is absent" —
  **rejected**: the file exists (`harness/docs/testing.md`).
- *Minimalist:* "bridge `respond()` lets a stale response wedge gate B" — **downgraded**
  to 4.6: Pi's own descriptor-id check re-loops the gate; the flaw is real but transient.
- *Skeptic:* "failSession swallow leaves contradictory state" [medium] — **downgraded**
  to 6.4: the catch is deliberate crash-path tolerance; only observability is missing.

## What went well (3/3 reviewers found no issues here)

- Pi runtime identity checks, duplicate-start protection, per-session lifecycle
  single-flight in the routes.
- `-c` per-subcommand placement and `--json` purity (checked end to end).
- Tool-event sanitization across the backend/client boundary (contract 5).
- Phase folding / ledger mutation core logic and its test coverage.

## Suggested execution order

Fase 1 (crash class, 1h) → 2.1+2.2 (workflow integrity) → 3.1 (SSE generation) →
4.1-4.4 → 5.1-5.3 → 3.2 + Fase 6. Fasi 1-4 are independent of each other and safe to
land as separate commits; 5.1 and 5.2 touch the ledger contract and deserve their own
review pass.

## Environment note

The machine's `codex` CLI was broken (configured default model requires a newer CLI);
upgraded via Homebrew 0.137.0 → 0.144.6 to run the reviewers.