Update .claude with 11 changed files (#4483)

[rysweet]

Summary

Concise workflow-generated PR for .claude.

Issue

Closes #4483

Changed files

• .claude/.claude-4483-stash/runtime/launcher_context.json
• .claude/.claude-4483-stash/runtime/metrics/post_tool_use_metrics.jsonl
• .claude/.claude-4483-stash/runtime/sessions.jsonl
• docs/concepts/typed-outcome-ledger-shared-connection.md
• docs/howto/diagnose-typed-outcome-database-is-locked.md
• docs/index.md
• docs/reference/typed-outcome-ledger-connection-registry-api.md
• mkdocs.yml
• src/ooda_actions/advance_goal/typed_goal_session.rs
• src/typed_ooda/ledger.rs
• tests/typed_ooda_outcome_lock_regression.rs

Diff stat

 .../runtime/launcher_context.json                  |   9 +
 .../runtime/metrics/post_tool_use_metrics.jsonl    |  22 ++
 .claude/.claude-4483-stash/runtime/sessions.jsonl  |   2 +
 .../typed-outcome-ledger-shared-connection.md      | 144 +++++++++
 .../diagnose-typed-outcome-database-is-locked.md   | 156 ++++++++++
 docs/index.md                                      |   1 +
 ...typed-outcome-ledger-connection-registry-api.md | 329 +++++++++++++++++++++
 mkdocs.yml                                         |   3 +
 .../advance_goal/typed_goal_session.rs             |   7 +-
 src/typed_ooda/ledger.rs                           | 197 ++++++++----
 tests/typed_ooda_outcome_lock_regression.rs        | 216 ++++++++++++++
 11 files changed, 1031 insertions(+), 55 deletions(-)

Behavior

Implemented behavior through these branch commits:
cde7b9c docs: correct busy_backoff backoff-sequence comment
3f69bbd wip: checkpoint after implementation (steps 7-8)
9d59c43 fix(typed-ooda): serialize outcome ledger through one shared connection (#4483)
1463093 docs(typed-ooda): spec-first docs for shared-connection ledger fix (#4483)

Validation

ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by checking the branch and the changes made so far.

● Get branch and changed files (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4483-fix-systemic-typed-outcome-p
│ ersistence-failed-data && git branch --show-current && echo "---CHANGED FILES---" && git
│ --no-pager diff --stat HEAD~5..HEAD 2>/dev/null | tail -30 && echo "---STATUS---" && git
│ --no-pager status --short
└ 26 lines…

Changes are committed. Let me examine the actual fix and the new regression test.

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

• Branch has 4 commit(s) ahead of main
• Code review completed
• Philosophy check passed

---

This PR was created as a draft for review before merging.

---

Step 16b: Outside-In Testing Results

Tested from the PR branch (feat/issue-4483-fix-systemic-typed-outcome-persistence-failed-data) as a consumer of the typed-outcome ledger would exercise it.

Detected toolchain: Rust CLI (Cargo.toml at repo root, Cargo.lock present, cargo 1.95.0 / rustc 1.95.0). Per the qa-team skill's repo-type detection, Rust CLI repos validate via native cargo test rather than the gadugi-agentic-test framework.

Chosen strategy: Native cargo test at the library/integration boundary — the same public API (CapabilityHandler, OutboxWorker) that OODA cycles and outbox startup-recovery use — plus cargo clippy as the CI lint gate. This exercises the fix through the real external consumer boundary (concurrent handlers on one DB file), which is exactly where issue #4483 manifested.

#	Scenario	Command	Result	Key output
1 (simple/regression)	Concurrent OODA cycles (112 goals) + 16 startup-recovery workers race on one ledger file; assert no database is locked and every terminal is durably persisted	cargo test --test typed_ooda_outcome_lock_regression --locked	✅ PASS	test concurrent_cycles_and_startup_recovery_never_lock_the_outcome_ledger ... ok — 1 passed; 0 failed in 3.11s
2 (edge/integration)	Full typed-OODA ledger + executor + outbox behavior suite (session scoping, engineer claims/leases, approvals, terminal readback, effect drain/retry) to confirm shared-connection serialization preserves existing behavior	cargo test --lib typed_ooda --locked	✅ PASS	50 passed; 0 failed; 0 ignored in 0.98s
3 (lint gate)	Clippy on library + all tests (CI gate)	cargo clippy --lib --tests --locked	✅ PASS	Finished dev profile — no warnings

Fix count during outside-in testing: 0 (all scenarios passed on the first run; no diagnose→fix→retry iterations were required).

PR CI status at time of testing: MERGEABLE; security/audit/deny/vet/npm-audit/scripts-tests checks passing, with pre-commit and coverage still running.

Verification notes: No stray print!/println!/eprintln!/dbg! in the changed files — the former eprintln! at typed_goal_session.rs:151 now routes through tracing::error!. The regression asserts both the absence of lock errors and durable persistence of every terminal, so the fix cannot pass by silently dropping writes.

[rysweet]

Step 17b — Comprehensive Code Review

Verdict: ✅ APPROVE / merge-ready (non-blocking observations only)

Reviewed the full diff (src/typed_ooda/ledger.rs, src/ooda_actions/advance_goal/typed_goal_session.rs, new regression test, docs) against issue #4483 and the resolved design decisions D1–D8.

Verification performed (fresh, on cde7b9c9)

Check	Command	Result
Regression test	cargo test --test typed_ooda_outcome_lock_regression --locked	✅ 1 passed (2.88s)
Typed-OODA suite	cargo test --lib typed_ooda --locked	✅ 50 passed; 0 failed
Lint gate	cargo clippy --lib --tests --locked	✅ no warnings
Registry routing	grep Connection::open src/typed_ooda/	✅ exactly one — inside shared_connection factory (line 88); no bypassing opens remain
No stray IO	grep print!/println!/eprintln!/dbg! in changed files	✅ none; typed_goal_session.rs:151 now routes through tracing::error! with structured goal_id+error

Strengths

• Correct root-cause fix. The path-keyed OnceLock<Mutex<HashMap<PathBuf, Arc<Mutex<Connection>>>>> registry (D1/D2) serializes every write per DB file through one WAL + busy_timeout connection, eliminating the file-level write-lock race that produced database is locked on restart bursts. Path-keying correctly preserves per-test isolation.
• Fail-visible, never-swallowed errors. begin_immediate! retries only on genuine SQLITE_BUSY/DatabaseLocked (is_sqlite_busy), bounded at 5 attempts, and surfaces the unchanged persistence() error on exhaustion — outward behavior identical to the prior .map_err(persistence)?.
• Pragmas applied unconditionally at connect (D3): foreign_keys=ON, busy_timeout=5s, journal_mode=WAL.
• Strong regression test. Asserts BOTH the absence of lock errors AND durable read-back of every terminal — so the fix cannot pass by silently dropping writes. 112 concurrent goals + 16 startup-recovery workers reproduce the post-restart burst.
• Public API unchanged (D7): CapabilityHandler::open/with_engineer_liveness signatures identical; additive & non-breaking.
• Docs (concept/howto/reference) added and wired into mkdocs.yml.

Non-blocking observations

1. canonical_key relative-path aliasing. For a bare filename ("outcomes.sqlite3") the parent is empty → filtered out → key falls back to the raw relative path. But "./outcomes.sqlite3" has parent ".", which canonicalizes to an absolute dir → a different key for the same file, yielding two connections and defeating serialization. Production opens the ledger via a stable absolute path so this is latent, but consider canonicalizing the empty-parent case against CWD to close the gap defensively.
2. Process-wide poison blast radius. With one shared connection per path, a panic while holding the connection Mutex poisons it for the entire process lifetime — every subsequent op for that path then returns outcome ledger lock is poisoned. Previously each handler had an isolated connection. Transactions use RAII rollback so the risk is low, but the blast radius grew; consider clear_poison()/recovery or a doc note on the tradeoff.
3. Registry never evicts (acknowledged by design). Fine for production (bounded set of DB paths). In long-lived test processes entries accumulate, but the footprint is negligible.
4. busy_backoff shift bound. 1u64 << attempt.min(5) permits a shift of 5 while BUSY_RETRY_MAX_ATTEMPTS only drives attempt 1..=4; safe (.min(100) caps the value, no overflow) and the corrected doc comment now matches actual behavior (10/20/40/80ms).

Checklist

• Code quality and standards — idiomatic, well-documented, structured logging
• Test coverage adequate — targeted regression + full typed-OODA suite green
• No TODOs, stubs, or swallowed exceptions — retries surface errors unchanged
• No unimplemented functions
• Logic correctness — verified registry routing, retry predicate, pragmas
• Edge case handling — see non-blocking notes #1/#2 for defensive hardening

None of the observations block merge. Recommend merge once pre-commit/coverage CI complete.

[rysweet]

Step 17c — Security Review

Verdict: ✅ PASS (merge-ready) — 1 LOW-severity repo-hygiene finding, no exploitable vulnerabilities.

Reviewed commit cde7b9c9 against the security checklist. All automated security gates pass: GitGuardian, cargo-audit, cargo-deny, cargo-vet, npm-audit — all green.

Checklist results

Area	Result	Notes
Injection (SQL)	✅ PASS	Every execute/query_row/query_map/prepare in ledger.rs uses parameter binding (params![…], ?1 placeholders, []). No user input is string-interpolated into SQL. The two format! calls (L743/L750) build error-message strings only, never SQL.
Injection (shell/eval/deser)	✅ PASS	No Command/shell exec, no eval, no untrusted deserialization. WAL/foreign-key pragmas are static literals.
Sensitive-data handling	✅ PASS (code)	No secrets, tokens, keys, or credentials in code. Error paths use structured tracing::error! (replacing raw eprintln!) and carry only bounded, non-sensitive context.
AuthN / AuthZ	➖ N/A	Change is a local SQLite persistence layer; no auth surface introduced.
Path / traversal	✅ PASS	canonical_key canonicalizes the parent dir and rejoins the file name, falling back to the raw path on error. DB paths are internally derived (not attacker-controlled), so no traversal exploit. Registry lookup + Connection::open happen under the same mutex → no open-time TOCTOU.
Concurrency safety	✅ PASS	Shared Arc<Mutex<Connection>> per canonical path. Mutex poisoning is fail-visible — mapped to a persistence error, never unwrap()-panicked or silently ignored.
Error visibility	✅ PASS	Errors are bounded and surfaced; none are swallowed. Regression test asserts durable persistence (can't pass by dropping writes).

Finding — LOW: accidental commit of runtime session artifacts

Three files were committed that are not part of the feature:

.claude/.claude-4483-stash/runtime/launcher_context.json
.claude/.claude-4483-stash/runtime/metrics/post_tool_use_metrics.jsonl
.claude/.claude-4483-stash/runtime/sessions.jsonl

• Root cause: .gitignore ignores /.claude/runtime/, but these live under /.claude/.claude-4483-stash/runtime/, which the anchored pattern does not match — so they slipped past the ignore rule.
• Exposure: No secrets/tokens/credentials (verified by pattern scan + GitGuardian pass). They do contain local developer environment data: absolute paths (/home/azureuser/…), PIDs, session IDs, and internal workflow prompt logs. This is minor information disclosure / repo hygiene, not exploitable.
• Severity: LOW — non-blocking. Recommend before/after merge:
  1. git rm -r --cached .claude/.claude-4483-stash/ and drop from the PR, and
  2. tighten .gitignore (e.g. /.claude/**/runtime/ or .claude/.claude-*-stash/) so stash runtime dirs can't be re-committed.

Non-security notes (already tracked)

Registry never evicts (bounded by distinct DB paths — trivial), and relative-vs-absolute path aliasing in canonical_key — both previously raised as non-blocking defensive-hardening items. No security impact.

Conclusion: No new vulnerabilities introduced. The path-keyed shared-connection fix is secure. The only actionable item is removing the accidentally-committed .claude runtime artifacts; it does not block merge.

[rysweet]

Step 17d — Philosophy Guardian Review

Verdict: ✅ PASS / merge-ready — the code change is strongly philosophy-compliant. One non-blocking repo-hygiene note (stray runtime artifacts).

Compliance checklist

Principle	Status	Evidence
Ruthless simplicity	✅	Fixes the root cause of #4483 directly: one shared Arc<Mutex<Connection>> per canonical DB path via a small process-wide registry, replacing N independent connections that contended on the file-level write lock. Minimal correct fix — no speculative machinery.
Bricks & studs	✅	Clean seam: shared_connection / connection_registry / apply_connection_pragmas are private helpers internal to ledger.rs. The public stud — CapabilityHandler::open(path, policy) — is unchanged, so callers are unaffected.
Zero-BS (no stubs / faked APIs / swallowed errors)	✅	No stubs or placeholders. Busy-retry surfaces the underlying rusqlite::Error unchanged as PersistenceFailed on exhaustion (begin_immediate! early-returns persistence(err)); nothing is swallowed. Mutex-poison maps to an explicit PersistenceFailed via persistence_message, not .unwrap(). Startup-recovery eprintln! upgraded to structured tracing::error! with goal_id/error fields.
No over-engineering	✅	begin_immediate! is a macro (not a fn) specifically because it must early-return the enclosing method's error type after retry exhaustion — a justified Rust idiom, not gratuitous magic. Backoff is a 6-line pure function. Registry has no eviction, TTL, or metrics layered on speculatively — appropriately scoped for a bounded set of long-lived DB paths.
Clean module boundaries	✅	Concurrency concern is fully encapsulated in ledger.rs; typed_goal_session.rs only swaps a print for structured logging. No new cross-module coupling.

Non-blocking observation (repo hygiene — not a code-quality issue)

• Stray runtime artifacts committed. The PR includes .claude/.claude-4483-stash/runtime/ session state (launcher_context.json, metrics/post_tool_use_metrics.jsonl, sessions.jsonl — ~33 lines). These are ephemeral agent/session artifacts unrelated to the fix and mildly violate "keep the repo clean / ruthless simplicity." Recommend removing them from the branch (and confirming .claude/**/runtime/ is gitignored) before merge. Does not affect the shipped binary or tests.

Not changed (correct/intentional)

Retry logic, registry-no-eviction (by design for long-lived paths), public API signatures, and the WAL + 5s busy_timeout pragmas — all consistent with prior Pre-Commit / Security / Code reviews.

Conclusion: Philosophy-compliant. The one note is a trivial artifact cleanup, not a blocker.

[github-actions]

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module	Lines	Covered	Coverage
Total	194544	163498	84.0%

_{Coverage data from CI run. Test files matching tests?/ are excluded from line counts.}