Pre-release hardening: shared-package extraction, canonical verdict contract, process supervision + DB integrity

[spencermarx]

Summary

Pre-release hardening that lands three completed OpenSpec changes plus a test-infra fix on top of main. The work reverses an inverted package dependency, makes the review verdict a single enforceable contract, and fixes a class of process/DB-integrity wedges observed in the dashboard. Each concern is a separate atomic commit; the OpenSpec proposals are archived into the canonical specs in the final commit.

Scope: 186 files, +5878/−1186. No GitHub issues are closed by this PR (#42 cross-repo reviews is intentionally out of scope). evolve-phase4-host-aware-spawning is intentionally untouched.

What's included

1. refact(shared) — extract persistence + config shared packages

Reverses the dishonest dashboard → cli dependency (dashboard imported 7 of cli's 8 library subpaths, cli/db at 38 sites, while declaring cli only as a devDependency). The shared lower layers move out of the cli app into two source-only packages so both apps depend on shared libraries and never on each other:

• @open-code-review/persistence — db/ + state/ + vendor-resume + test-support + runtime-checks, kept in one package because db and state form a mutually-recursive type cycle and the connection-cache singleton must be a single module instance.
• @open-code-review/config — models + runtime-config + team-config.

Both mirror the @open-code-review/platform precedent exactly — private, version 0.0.0, every exports condition points at ./src/*.ts (no build.mjs, no dist, no build target), declared as devDependency: workspace:*, inlined by esbuild — so they are excluded from the release set and never published (no npm trusted-publisher changes needed). Direct cutover: no re-export shims, and the old test-support → ./index.js external trick (issue #41) is deleted, not preserved. The CLAUDE.md "S27 / 9th-subpath" graduation rule is replaced with a cause-based rule (a slice graduates when consumed across a package boundary).

2. feat(verdict) — canonical 3-state verdict contract enforced end to end

Fixes the accept_with_followups off-vocabulary bug that rendered a meaningless "?" badge. Models the verdict as a closed 3-state merge gate (APPROVE / REQUEST CHANGES / NEEDS DISCUSSION), keeping residual work (follow-ups/suggestions) where it is already normalized — in finding categories — so the headline verdict and the finding list can never contradict each other.

• platform: canonical verdict vocabulary and one pure round-count derivation on Node-free ./verdict and ./counts subpaths.
• cli: fail-fast validation at complete-round (verdict enum, min title length, directional synthesis_counts cross-check).
• dashboard: read-time normalizeVerdict at ingestion, a 3-state badge with a subordinate residual-work chip, findings-table loading/empty/degraded states.
• lifecycle defects folded in: D1 (dashboard must not fabricate terminal completion from final.md alone — it maps to the synthesis phase), D2 (complete-round guarantees round-meta.json regardless of --file/--stdin), D3 (one shared count derivation).
• agents: verdict vocabulary unified across skill references + final template, synced to .ocr.

Fix-forward; no destructive migration.

3. feat(dashboard) — process supervision + DB integrity hardening

Addresses a dashboard-spawned review that finished its work yet wedged alive for 44+ minutes while the DB grew to ~298 MB (FK-orphan rows + a NULL-defeated unique index that let one artifact accumulate 775 duplicates).

• Supervision: detached spawns are unref'd and write to per-execution log files (a FileTailer streams them) instead of OS pipes; finalization is driven by the vendor result event + a per-execution watchdog, never stdio EOF. A cross-platform reapTree kills the whole descendant tree on cancel, watchdog, and singleton takeover. finishExecution is first-wins idempotent with cancel-wins applied centrally.
• Decomposition: the command-runner god class split into process-registry / spawn-markers / prompt-builder / watchdog / finalizer leaf modules.
• Liveness: parent-row heartbeat on output activity + supervisor tick.
• DB integrity: explicit UPDATE-or-INSERT markdown writer; migration v14 collapses duplicates + adds a NULL-safe unique index; operator maintenance (ocr db doctor/vacuum/prune) with snapshot-before-mutate.
• Singleton: a live prior dashboard is reaped and taken over rather than coexisting on an incremented port; startup reaps orphan tmp/exec-log files.

Incorporates the round-1 and round-2 multi-agent review remediations from PR #36 (watchdog liveness gating, invisible-char \p{Cf} strip, NaN-bypass closure, shutdown reap grace, finalizer/sweep ownership boundary, etc.).

4. test(dashboard-ui-e2e) — eliminate a flaky e2e

Off-CI the Nx Playwright preset ran fullyParallel with workers=undefined (one per core) and retries=0, pointing several browser contexts at one shared Vite dev server; concurrent cold loads raced Vite's dependency optimizer (forcing full-page reloads + transient console errors) and the auth spec waited on networkidle, which never settles under the dashboard's persistent socket.io connection. Serialized against the single shared server (fullyParallel:false, workers:1) — the correct model, not a retry mask (retries stay 0 so real flakes surface) — and replaced networkidle with the deterministic auth-resolved signal.

Also included

• test(dashboard) Harden model/config strings that reach Windows shell:true spawns #43 capture-boundary reject-branch coverage (pre-existing on the branch).
• refact(config) trailing import-path repoint for the team-models e2e.

Verification

All run green on the reconciled tree:

• nx run-many -t typecheck (9 projects)
• nx run-many -t test (5 projects)
• nx run-many -t e2e (cli-e2e, dashboard-api-e2e, dashboard-ui-e2e) — incl. the de-flaked suite run 5× + the full cross-project matrix
• cli:build + dashboard:build
• nx release --skip-publish --dry-run (computes v2.3.0 cleanly)
• openspec validate --strict for all three changes before archiving

Release note

This PR does not cut a release. The v2.3.0 release remains paused pending explicit authorization; publishing happens only via the tag-triggered CI workflow after merge.

🤖 Generated with claude-flow

[spencermarx]

Update: forward-resume + verdict-direction implemented, archived, and round-1 review addressed

This adds two OpenSpec changes (now archived into the live specs) on top of the existing pre-release hardening, then resolves the round-1 spec review.

What was built

1. Forward-resume of a stranded mid-pipeline review (the Review #146 class: active session, no round_completed, owning turn dead). Vendor-neutral, two-tier.

• packages/shared/persistence/src/state/forward-resume.ts — event-sourced current_phase derivation; single-writer session_resumed lease ({kind:"forward_resume"}, no phase/round column, one-transaction CAS, append-before-spawn cap counting, TTL + phase-transition renewal); owning-turn liveness; cap-exhaustion close via the existing session_auto_closed_stale reason (no taxonomy/schema migration).
• projection.ts — lifecycle fold ignores forward-resume leases (cannot regress current_phase).
• state/index.ts — stateInit refuses re-opening an active, incomplete session (would reset to context); stateStatus reports forward_resume/abort_or_fresh + remaining_phases + attempts.
• packages/cli/src/commands/review.ts — forward-only, lease-guarded --resume; baseline skill-handoff when no vendor id.
• packages/dashboard/.../forward-resume-sweep.ts — PID-death-gated auto-resume + cap-close, wired into the startup/periodic sweeps; ResumeCard exhausted-state UI (Start fresh / Mark abandoned).
• Config: forward_resume_max_attempts (2) + forward_resume_lease_seconds (1800).

2. Directional verdict↔blocker-count consistency at complete-round (round-meta.ts): APPROVE⟹0 blockers, REQUEST CHANGES⟹≥1, bound to the deduplicated canonical round blocker count.

Tests: persistence/config/cli/dashboard unit suites + cli-e2e green; openspec validate --strict passes for all 12 specs.

Round-1 review feedback — addressed

The review was spec-only (REQUEST CHANGES, 5 blockers). Corroboration confirmed every blocker was a spec-wording defect; the implementation already matched — so the fixes are spec text only, no code change.

Blockers (all 5):

1. package-architecture Purpose TBD → real purpose (also fixed the same TBD leak in config and review-map).
2. "Node-free subpath" → now a normative package-architecture requirement + scenario; sqlite-state references it by name.
3. Event-log count vocabulary → retired the stale critical_count/major_count/nitpick_count (never written by code) for the canonical category counts the code does write, deferring to the shared helper.
4. "Owning turn ended" → clarified as caller-relative (a human re-invocation is the takeover signal), so baseline forward_resume fires (CQ2).
5. Cap-exhaustion close → the CLI writes it on the no-daemon path (matches code); writer responsibility assigned per tier (CQ4).

Should-fixes / suggestions: lease write-side/read-side split; wedged-vs-stranded glossary + terminology alignment; discriminated-union kind/reason doc (CQ7); canonical "positive death evidence", "canonical round blocker count", and CONTROL-prompt definitions; SHALL NEVER→SHALL NOT; OpenCode exemption reframed as a host capability; complete-round self-heal source + at-commit-time qualifier; single-writer safety scenario; graduation edge cases + shared-package membership list (CQ5); lease TTL default named (1800s).

Deferred (review marked non-blocking): the three requirement-family refactors (split Resume Flag / Forward-Resume family / group validation scenarios) — prose is correct; deferred to avoid churning cross-references before the release tag.

Honest carve-outs (tracked in the change's tasks.md): the 4-host live forward-resume proof and live #146 recovery are manual obligations (need all four vendor CLIs); fully-unattended non-interactive adapter-prompt drive is a hardening follow-up. The guaranteed-unattended behaviors (detection, cap-close, lease, surfacing) are implemented and tested.

[spencermarx]

Addressed round-1 code-review feedback (the APPROVE'd review)

Although this review was APPROVE / 0 blockers, I worked the should-fix backlog genuinely — corroborating each against the actual code first. Implemented the safe, high-value items now; deferred the large refactors the review itself marked as backlog. Pushed in ddd1c77.

Implemented

• SF#2 — Playwright retries: 0 pinned. The Nx preset defaulted to 2 on CI, silently retrying (hiding) real flakes exactly where they matter. Now explicit on both sides of the CI boundary.
• SF#3 — first-wins CAS on execution-tracker.finish. Added AND finished_at IS NULL so a second finish() (the close + error paths can both fire) can't clobber the first's recorded exit code/output — mirrors finalizer.ts's de-dup so every finalize path in the package is first-wins.
• SF#4 — Node-free subpath contract test. New test asserts the platform /verdict + /counts subpaths stay Node-free transitively, guarding the primed browser-bundle trap (barrel pulls node:*).
• SF#5 — deleted the duplicate verdict alias table in final-parser.ts. Alias→canonical now delegates to the shared normalizeVerdict (one source of truth); prefix-extraction for "VERDICT — rationale" lines keys off the shared CANONICAL_VERDICTS. Behavior preserved (all final-parser.test.ts cases green, incl. the unknown-phrasing clip).
• SF#6 — RoundMeta.verdict: string → CanonicalVerdict. Write-boundary type safety; any future path bypassing validateRoundMeta now fails at compile time. Read DTOs stay string for legacy tolerance.
• SF#10 — exhaustive never default on the watchdog-tick switch.
• SF#11 — dropped the non-null assertions in the prompt-builder reviewer parse (destructure + guard).
• Sug#4 — verdict rejection echoes the raw value, not the sanitized form (matches the title/category paths).
• Sug#11/Feat/reviewers team ux #12 — CLAUDE.md accuracy: lead the db+state co-location with the type cycle (the cache singleton is the consequence); distinguish the consumer-side devDependency rule from a shared package's own runtime dependencies.

Verified: persistence / platform / cli / dashboard typecheck + unit suites green.

Deferred (review marked non-blocking backlog — agree)

• SF#1 — module-boundary lint rule (the review's highest-leverage item). It needs a net-new ESLint + @nx/eslint-plugin toolchain + CI wiring — there is no eslint config anywhere in the repo today. That's a dedicated infrastructure change deserving its own PR + CI verification, not a tack-on to a hotfix. Recommend it as the immediate follow-up — it's what converts the app → shared → shared DAG from human-review into a CI gate.
• SF#7 / SF#8 — the spawnAiCommand (~540 LOC) and filesystem-sync.ts (1517 LOC) decompositions. Behavior-preserving but large; the review explicitly sequences them as multi-PR backlog (and behind SF#1 so each step is DAG-verified).
• SF#9 — round-N S## comment-rot strip (opportunistic, as files are touched).

Re: clarifying questions

• Q2 (idempotent complete-round with a different payload): current behavior re-materializes the artifact from the current call while the recorded event keeps the first call's counts — a latent divergence. Leaving as-is for this hotfix; worth a follow-up to either reject the second differing payload or make it a true no-op. Flagging for a product call rather than changing finalize semantics under a hotfix.
• Q5 (extract a shared finalizeRow helper): used the inline CAS for now (minimal, hotfix-safe); the shared-helper consolidation folds naturally into the SF#7 extraction.