Update documentation with 16 changed files (#4483) - #4490
Conversation
Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase.
📊 Coverage Summary
Coverage data from CI run. Test files matching |
rysweet
left a comment
There was a problem hiding this comment.
Comprehensive Code Review — Step 17b
Scope reviewed: src/typed_ooda/ledger.rs, src/typed_ooda/schema.rs (+ associated docs) at head a54809a2.
Independent verification performed:
cargo test --lib typed_ooda::→ 57 passed / 0 failed (incl. all 6 incident-derived tests: WAL/NORMAL/busy_timeout/FK config, legacy delete→WAL upgrade, 8-goal burst, startup-recovery collision).cargo clippy --lib→ clean (0 warnings, 0 errors).
Verdict: ✅ APPROVE — no blocking issues
The fix correctly identifies and addresses the root cause of #4483 on two reinforcing layers, with strong TDD coverage that reproduces the exact incident shape.
Review checklist
- Code quality & standards — idiomatic Rust;
WriteGuardDeref/DerefMutkeeps all existingself.lock()?call sites unchanged (minimal blast radius). - Test coverage adequate — regression tests exercise the public handler API under real thread contention (barrier-synchronized burst across 8 handlers + a fresh-handler recovery worker), plus per-connection pragma contracts and the legacy-ledger WAL upgrade path.
- No TODOs, stubs, or swallowed exceptions — every fallible path maps to
Errviapersistence/persistence_message; nounwrap/expectin production code (test-only). - No unimplemented functions.
- Logic correctness — see notes below.
- Edge case handling — legacy delete-mode ledgers, idempotent re-config, poisoned-lock, canonicalization-after-create all handled/tested.
Correctness analysis (positives)
- Root cause correct. Pragmas were previously set only inside
initialize, which early-returns onuser_version == SCHEMA_VERSION, so already-initialized ledgers (every run after the first) stayed in rollback-journal mode with nobusy_timeout. Moving config toconfigure_connection()invoked unconditionally on every open fixes this precisely. - Consistent lock order.
lock()always acquireswriter_lock→connectionmutex;WriteGuarddrops connection before writer (fields drop in declaration order = reverse acquisition). No lock-ordering deadlock. Confirmed all read/write paths route throughself.lock()— no directself.connection.lock()bypass. - Cross-handler serialization. Process-wide per-canonical-path
Arc<Mutex<()>>registry correctly serializes independent per-goal handlers and the startup-recovery path — the actual missing guarantee, since the in-process connection mutex only serializes within a single handler. - Canonicalize ordering is correct — file is created by
Connection::openbeforewriter_lock_forcanonicalizes. - Fail-visible. Poisoned locks and canonicalization failures surface as
Err, consistent with the fail-visible requirement and pre-existing mutex semantics.
Non-blocking observations (no changes required)
- Registry never evicts.
LEDGER_WRITER_LOCKSgrows one entry per distinct canonical ledger path for the process lifetime. In practice paths are ~1, so negligible — flagging only for awareness. - Writer lock also serializes reads. Because
lock()backs read paths too, the process-wide writer mutex negates WAL's in-process reader concurrency. Already adjudicated as acceptable (ledger ops are microsecond-scale vs. seconds-long goal cycles) — correctness-first is the right call. If read throughput ever matters, a read-only path that skipswriter_lock(WAL permits concurrent readers) would be the lever. - WAL sidecars. WAL introduces
-wal/-shmfiles; any backup/copy/move must checkpoint or include them. The PR's WAL recovery runbook + verified-backups doc updates appear to cover this — good.
Conclusion: Change set is complete, well-tested, lint-clean, and non-breaking (SCHEMA_VERSION unchanged, no public-API change). Cleared to merge.
rysweet
left a comment
There was a problem hiding this comment.
Security Review — Step 17c (MANDATORY)
Scope: src/typed_ooda/ledger.rs, src/typed_ooda/schema.rs (+ docs) at head a54809a2. The change adds a process-wide per-canonical-path writer lock and unconditional SQLite pragma configuration (WAL / synchronous=NORMAL / busy_timeout / foreign_keys) to eliminate the database is locked crash-loop (#4483).
Verdict: ✅ PASS — no security vulnerabilities identified. Zero required changes.
Checklist
| Area | Result | Notes |
|---|---|---|
| Injection (SQL/command) | ✅ PASS | All new SQL is static (PRAGMA batch, fixed queries). Test format! values (session/cycle/request keys) flow through existing parameterized params! bindings, never string-concatenated into SQL. No Command/process::/exec added. |
| New vulnerabilities | ✅ PASS | No unsafe, no FFI, no deserialization, no network I/O introduced. Pure in-process synchronization + connection pragmas. |
| Sensitive data handling | ✅ PASS | No secrets/PII introduced or logged. tracing::debug! emits only the canonical ledger file path (non-sensitive). Payloads (reason/raw_semantic) remain OpaqueBytes, unchanged and not logged. |
| Authentication / authorization | ✅ PASS | Capability model untouched — writes still require AuthenticatedToolContext + CapabilityGrant::RecordNoAction, cycle/goal binding preserved. The writer lock is orthogonal to authz and adds no bypass path. |
| Path handling | ✅ PASS (security-positive) | writer_lock_for uses std::fs::canonicalize, resolving symlinks/relative spellings so equivalent paths share one lock — closes a lock-evasion gap. Paths originate from process config, not untrusted input; no traversal exposure. |
| Denial-of-service / deadlock | ✅ PASS | Single consistent lock order (writer_lock → connection); WriteGuard fields drop in reverse (documented). No nested/reentrant lock(). Mutex poisoning is fail-visible — returns a CapabilityResult error instead of panicking or silently proceeding. |
| Availability regression | ✅ PASS | WAL + writer lock reduce contention (the actual DoS-shaped failure being fixed). synchronous=NORMAL is a documented, WAL-safe durability tradeoff — no integrity loss on app crash (only OS-crash window), acceptable for a recoverable outbox ledger. |
Non-blocking observations (no action required)
- Lock registry never evicts (
LEDGER_WRITER_LOCKS) — unbounded in theory, but bounded by the small number of distinct ledger paths per process. Not attacker-influenced; negligible. Already noted in prior passes. - WAL sidecar files (
-wal/-shm) now sit beside the ledger — same at-rest exposure surface as the main DB (no new sensitive data), and covered in the how-to doc. If the ledger ever holds sensitive payloads, existing filesystem permissions remain the control — unchanged by this PR.
Verification performed
- Added-line scan for
Command::/unsafe/std::env/process::/dynamic-PRAGMA/rawexecute(/query(string-building → none in production code. - Confirmed all write/read paths route through
self.lock()(noconnection.lock()bypass) — auth + serialization cannot be sidestepped.
Conclusion: No injection, no secret exposure, no authz weakening, no new attack surface. Cleared from a security standpoint.
Philosophy Guardian Review — Step 17dScope: Verdict: ✅ PASS — fully compliant, zero required changes. Compliance checklist
Non-blocking observations (no action required)
Both are already adjudicated in prior passes as premature-optimization; leaving as-is is the philosophy-aligned choice. Conclusion: The change embodies ruthless simplicity and zero-BS. Approved on philosophy grounds — cleared to merge. |
Summary
Concise workflow-generated PR for documentation.
Issue
Closes #4483
Changed files
Diff stat
Behavior
Implemented behavior through these branch commits:
a54809a wip: checkpoint after implementation (steps 7-8)
Validation
ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by getting the branch name and inspecting the current state of the changes.
● Get branch, status, and diff stat (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4483-fix-the-systemic-typed-outco
│ me-persistence-failed && echo "=== BRANCH ===" && git branch --show-current && echo "=== STATUS
│ ===" && git status --short && echo "=== DIFF STAT vs main ===" && git --no-pager diff --stat
│ main...HEAD 2>/dev/null | tail -30
└ 12 lines…
Changes are already committed. Let me inspect the actual code changes to understand what was implemented.
Risk
No high-risk subsystem pattern detected from changed paths.
Checklist
This PR was created as a draft for review before merging.