chore: hygiene pass — ruff clean, docs storage-model truth, replay /me, failSession log
Audit findings 6.1-6.4 + the audit's remediation plan itself (docs/superpowers/plans/2026-07-20-full-audit-remediation-plan.md). - ruff: 34 → 0 (unused imports/f-strings auto-fixed; E702 semicolon lines split in test files; one unused local dropped). Suite still 819 green. - CLAUDE.md + PROJECT_STATE.md no longer claim "no database / settings in settings.json": the harness selects filesystem OR PostgreSQL session storage (repository.py, server mode), and settings flow through harness preferences with the JSON file as fallback only. - tools/replay: stub /me (SPA boot was parsing the SPA's own HTML as JSON) and /runtime/prewarm. - failSession best-effort persistence now logs its failure server-side instead of vanishing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
@@ -0,0 +1,210 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user