ThothII/docs/superpowers/plans/2026-07-20-full-audit-remediation-plan.md

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.