feat(doctor): event-store consistency gate — compose status/replay when .loop/events.db exists - #77
Merged
Merged
Conversation
…en .loop/events.db exists loop doctor now incorporates event-store consistency (ADR 0001 consequence #4, PR #48 follow-up): doctor_report composes validate_contract with a new appended loop/runtime.py event_consistency_issues() that calls the status/replay verbs unchanged as black boxes. Absent store stays byte-stable plus one additive event_store report key; unreadable stores (corrupt_store / empty_store / ambiguous_run_id) become typed doctor failures, never skips; ok only narrows True->False, issues only append. reference/repo-os-contract.md gains section 22 and the corrected section-16 scope boundary. 11 new tests (933/16 extras, 864/85 pyyaml-only, exact).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2863ec3745
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+208
to
+209
| status = status_report(target, mode=mode) | ||
| replay = replay_report(target, mode=mode) |
There was a problem hiding this comment.
When a writer commits an iteration or run-control event after status_report completes but before replay_report reads the store, the result combines state_json_agrees and event_count from the old stream with legality/determinism from the new stream. If that writer crashes before materializing state.json, doctor can therefore return ok: true despite a persistent state/event desynchronization; compare the two snapshots or obtain both reports from one snapshot.
Useful? React with 👍 / 👎.
Comment on lines
+756
to
+758
| event_store, event_issues = event_consistency_issues(target, mode=mode) | ||
| issues = report["issues"] + list(event_issues) if event_issues else report["issues"] | ||
| return {**report, "event_store": event_store, "issues": issues, |
There was a problem hiding this comment.
For a present, readable store, these calls validate every event against loop-engineer/event@1, but the returned doctor report preserves the file-only schemas_checked list from validate_contract. Consumers using this field to audit validation coverage are consequently told that event@1 was not checked even though the new gate checked it; append EVENT_SCHEMA_ID when event validation actually runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds an event-store consistency gate to loop doctor when .loop/events.db is present, by composing existing read-only runtime reports (status/replay) and folding their findings into the doctor output and ok decision.
Changes:
- Extend
doctor_report()to include anevent_storesection and to fail doctor when runtime consistency issues are detected. - Add
event_consistency_issues()toloop/runtime.pyto aggregatestatusdivergence +replayfindings into a single doctor-consumable result. - Add integration tests and update the normative contract documentation to describe the new gate.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/test_doctor_eventstore.py | New integration tests covering absent/readable/unreadable store behaviors and issue folding across validation modes. |
| reference/repo-os-contract.md | Updates spec scope boundary wording and adds §22 documenting the doctor event-store consistency gate. |
| loop/runtime.py | Adds event_consistency_issues() that calls status_report()/replay_report() and returns health + findings. |
| loop/contract.py | Updates doctor_report() to compose validate_contract() with event_consistency_issues() and fold findings into issues/ok. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+210
to
+213
| except RuntimeStoreError as exc: | ||
| return {"present": True, "readable": False, "error_code": exc.code}, [ | ||
| ContractIssue(exc.code, str(exc)) | ||
| ] |
Comment on lines
+768
to
+774
| When `.loop/events.db` exists, `loop doctor` composes the exact read-only | ||
| `status`/`replay` verbs (§16, §20) — never duplicating their fold/divergence | ||
| logic — and folds their findings into its own `issues`/`ok`. An absent store is | ||
| conformant: doctor reports `"event_store": {"present": false}` and every other | ||
| key is byte-identical to a store-less report. A present, readable store adds | ||
| `"event_store": {"present": true, "readable": true, "run_id", "event_count", | ||
| "state_json_agrees", "deterministic", "legal_sequence"}`; any of |
SollanSystems
added a commit
that referenced
this pull request
Jul 25, 2026
…0.0, slice 1/5) (#82) * feat(chain): canonical-JSON event hashing (pure stdlib leaf module) * feat(chain): incremental link verification + pure verify_chain over exported streams * feat(events): store generations — chain columns + user_version, shared feature-detected row reader with typed decode errors Task 3: - fresh-store DDL gains `prev_event_hash TEXT` + `event_hash TEXT NOT NULL` (design change D1: a pre-0.10.0 10-column INSERT now fails closed). The DDL is CREATE TABLE IF NOT EXISTS, so legacy tables are untouched — _connect() never upgrades a legacy store. - fresh stores get `PRAGMA user_version = 2`, set only when this connect actually created the events table (probed via sqlite_master beforehand). - EventRowDecodeError(ValueError) for an undecodable payload/artifact_hashes. - has_chain_columns(), store_user_version(), _BASE_COLUMNS, read_event_rows() — the single feature-detecting row projection shared by store, runtime and runner reads. Records always carry prev_event_hash/event_hash keys; legacy rows project None. It owns the JSON-decode translation each call site used to repeat. - read() rewritten onto read_event_rows(). - scripts/chain_fixtures.py: shared byte-faithful v0.9.0 legacy-store builder (test-only; the wheel force-includes scripts individually, so it does not ship). Folded in from Task 4 Step 3 — moved from Task 4 per controller end-green adjudication, and nothing else from Task 4: - append() computes prev_event_hash/event_hash via loop.chain on chained stores and writes a 12-column INSERT; legacy stores keep the v0.9.0 10-column INSERT and project both fields as None. - an unhashable record (ChainHashError) rolls back and raises EventValidationError. Why folded: `event_hash NOT NULL` is *defined* as the constraint that rejects a 10-column writer, and until Task 4 append() was itself a 10-column writer — so the DDL cannot land before the chained writer without breaking the plan's end-green invariant. Measured: the 2 DDL lines alone took the suite from 951 passed to 828 passed / 123 failed across 11 files. Still Task 4: EventStoreOperationalError, the sqlite3.OperationalError wrap, runcontrol/runner wiring, and all of Task 4's tests. Reviewed edits: NONE required. No existing test pinned the returned/read record key-set or the PRAGMA table_info column list, so the two new record keys and two new columns broke nothing. Gates: focused (test_event_chain + test_eventstore) green; full suite 955 passed / 14 skipped (was 951/14 — exactly the 4 new tests, no regressions); pyyaml-only fallback 886 passed / 83 skipped (skip count unchanged, so the new tests are unconditional in both validation modes). * feat(events): store-computed hash chain on append; typed operational error with a handled CLI path The chain computation inside append() itself landed in 4d96499 (Task 3) per controller adjudication; this commit adds its behavioral tests plus the typed error surface that was still missing. - EventStoreOperationalError(RuntimeError): the events table exists but cannot service the operation (schema drift, lock). append() wraps its transaction body so a drifted or locked store never leaks a raw sqlite3.OperationalError. - Handled CLI path: runcontrol._append_event and a new runner._store_append translate it into RuntimeStoreError("event_store_unusable", ...), which __main__ already catches for run/pause/resume/approve/cancel -> exit 2 with no traceback. - Tests pin store-computed chaining on fresh stores, caller-supplied chain fields being ignored, legacy stores staying unchained, the NOT NULL refusal of a pre-0.10.0 ten-column INSERT, both translation sites, and a subprocess probe that `loop run` / `loop pause` on a drifted store exit 2 traceback-free. * feat(schema): optional event@1 chain fields with structural-fallback parity * feat(migrate): explicit legacy-store chain migration verb loop/migrate.py + `loop migrate <workspace>`: the only path that upgrades a v0.9.0 legacy events.db to the chained v2 shape. Connect still never upgrades. Migration widens the table (nullable prev_event_hash/event_hash) and stamps user_version=2; it never rewrites rows — backfilling hashes is deliberately impossible because the append-only triggers forbid UPDATE. Pre-migration rows stay an unchained prefix and the first post-migration append is a chain genesis. Typed fail-loud: missing store -> RuntimeStoreError("missing_store"), corrupt store -> RuntimeStoreError("corrupt_store"); the CLI prints `migrate: <code>: ...` to stderr and exits 2 with no traceback. Reviewed edits (outside the brief's create-only file list): - scripts/test_loop_cli.py: added "migrate" to the command tuple in test_help_lists_every_command_with_a_description. That test is the only help/usage-parity assertion enumerating commands; leaving it unchanged would let the new verb drift out of the documented surface unchecked. Required by the task brief (Step 5) to land in this same commit. * feat(reducer): enforce hash chain at fold time via typed ChainBreakError chain.link_issue() runs per event inside _reduce_one, immediately after the non-monotonic-sequence check — the single chokepoint every folding verb (status/replay/run/simulate/doctor) passes through. A violation raises ChainBreakError(EventReplayError); the projection gains chain_head and unchained_prefix so verify_chain and the reducer report the same two numbers. Legacy/hand-built streams carry no event_hash, so link_issue returns None and they fold unchanged: verified by running, not assumed (test_reducer.py, test_adversarial_kernel.py 300-example property walks, test_adversarial_process.py all green). The raw-byte SQLite boundary pin is untouched — it asserts SQLiteEventStore.read() does not detect the tamper, which stays true; the chain adds detection one layer up, at the fold. No reviewed edits: zero existing tests required modification. Full suite +7 passed in both modes, zero regressions (extras 981/14 -> 988/14; pyyaml-only 911/84 -> 918/84). * feat(runtime): chain surfaced through status/replay/doctor/run; enforce event validation; race-safe immutable reads leave no sidecars Shared read path: new loop/runtime._read_only_connect (immutable=1 when no -wal sidecar exists, plain mode=ro otherwise) plus _read_store, which retries a failed read once as plain mode=ro so a lost race with a live append cannot mint a false corrupt_store (design change D4). Used by _read_events_readonly, _discover_run_id and runner._projection; read verbs now leave no -wal/-shm residue on a clean store (the PR #77 H4b finding). Row projection delegates to events.read_event_rows; EventRowDecodeError maps to RuntimeStoreError('corrupt_store', ...) at both call sites (design change D5), preserving the no-traceback invariant for in-row JSON corruption. runtime._events now enforces the per-event validation verdict it used to discard: a failing event raises RuntimeStoreError('invalid_event', ...) and the empty stream raises 'empty_store', replacing the 'assert validation is not None' that evaporates under python -O (decision 9a). All four folding surfaces report one code for a broken chain: status_report and replay_report catch ChainBreakError before EventReplayError, and runner._projection maps it before its generic except ValueError so run/simulate/run-control no longer relabel it invalid_event_stream (design change D3). status/replay reports gain chain_head + unchained_prefix (read via .get so the degraded projection cannot KeyError); doctor nests {'chain': {'head', 'unchained_prefix'}} under event_store. Reviewed edits: - scripts/test_doctor_eventstore.py test_synced_happy_path_is_doctor_clean gains two assertions on the new event_store['chain'] block (deliberate per Task 8 Step 4; the absent-store byte-stability pin is untouched and still passes). - loop/runner.py drops the now-unused EVENT_SCHEMA_ID import and its hand-rolled immutable-URI comment, both superseded by read_event_rows/_read_only_connect. Tests: 998 passed / 14 skipped with [schemas,yaml] extras (baseline 988/14); 928 passed / 84 skipped pyyaml-only (baseline 918/84). Delta is exactly the 10 new tests in both lanes, no skip movement. * fix(runner): D4 retry parity on the run/simulate read path + typed decode-error regression lock Finding 1: runner._projection used _read_only_connect directly, so a first-query sqlite3.DatabaseError became corrupt_store with no plain mode=ro reopen — the exact false alarm D4 exists to prevent, and simulate is a read-only monitoring verb that can race a live appender. Its two queries are now one read_stream closure passed to the shared runtime._read_store, giving run/simulate/run-control identical two-stage semantics to status/replay/doctor. Real corruption still fails both attempts and still maps to corrupt_store; the empty_store/ambiguous_run_id raises inside the closure are not DatabaseError and so are not retried. Shares the helper rather than duplicating the retry (imports _read_store in place of _read_only_connect, which is now reached through it). Finding 2: the runner EventRowDecodeError -> corrupt_store clause had no coverage. Adds scripts/test_doctor_eventstore.py::test_run_on_in_row_json_corruption_reports_ corrupt_store (same drop-trigger + corrupt-payload fixture as the doctor-side test, driven through dispatch_once). Negative control run: with the clause deleted the test fails with a bare loop.events.EventRowDecodeError escaping dispatch_once, outside the typed RuntimeStoreError family. Also adds scripts/test_event_chain.py::test_runner_read_path_retries_plain_mode_ro_ before_declaring_corruption, which pins Finding 1 falsifiably: it counts the sqlite3.connect calls a dispatch over a schema-drifted store makes and asserts exactly two (immutable first, plain mode=ro second, both mode=ro). It was red before the fix (1 == 2). Tests: 1000 passed / 14 skipped extras (was 998/14); 930 passed / 84 skipped pyyaml-only (was 928/84). +2 in both lanes = the two new tests, no skip movement. * feat(doctor): --expect-chain-head anchor gate, downgrade cross-check, absent-store sidecar tripwire event_consistency_issues gains expect_chain_head. The anchor fails hard on all four ways a store can fail to prove the expected head: absent, unreadable, unchained/broken (chain_head degrades to None), or diverged. A tampered store therefore cannot pass an anchored doctor even though its chain block is byte-identical to an unchained one. Absent-store branch also trips on -wal/-shm residue left behind by a deleted events.db (missing_event_store). With no store, no sidecars and no anchor the event_store block stays exactly {"present": False}. Readable branch cross-checks the lazy downgrade: user_version >= 2 with the chain columns dropped (chain_columns_missing). Reviewed edits vs the brief: - _store_declares_chain_without_columns routes its PRAGMA probe through _read_store, not a bare _read_only_connect, so a lost immutable=1 race retries plainly instead of being misreported as corruption (the D4 hole); it runs inside the existing try so a genuinely unreadable store surfaces as the typed unreadable branch rather than escaping doctor_report. - shared _anchor_mismatch helper for the three anchor issue sites. - added test_expect_chain_head_on_tampered_store_fails_doctor pinning the broken-chain composition end to end. * test(chain): adversarial coverage + four pinned honest limitations (recompute, truncation, downgrade, legacy) Eight integration tests over doctor's chain predicate (issue codes + the event_store.chain block, never global ok — design change D6). Detected: single-row splice with local recompute (the successor still cites the original), payload reorder across two same-type rows, and a mid-stream event_hash strip (the unchained-after-chained-prefix branch, until now only unit-pinned in loop/chain.py's own tests). Pinned honest limitations — these assert the attack SUCCEEDS, and keep reference/repo-os-contract.md #16 honest; if one starts failing the kernel gained a property and #16 must be updated in the same commit: - full history rewrite with a genesis re-chain plus a forged state.json and terminal_state.json (a FailedBlocked run laundered into Succeeded leaves doctor globally ok with zero issues), - trailing receipt_appended truncation (state-neutral, chain stays valid), - column-drop downgrade combined with PRAGMA user_version = 0 (defeats the D2 cross-check; without the PRAGMA reset chain_columns_missing still fires), - tamper on a never-migrated store (no retroactive coverage). In every one, --expect-chain-head is the control that catches it. Also pins the fourth fail-hard anchor shape: an anchor over a never-chained store fails rather than skipping. Reviewed edit outside the new file: scripts/test_doctor_eventstore.py's test_chain_columns_dropped_but_version_2_fails_doctor gains the same sqlite_version_info < (3, 35) skipif as the two DROP COLUMN tests here — on a contributor machine with older SQLite it would ERROR rather than skip. Tests only; loop/ and schemas/ byte-unchanged. * test(chain): pin event-store cleanliness flags on the full-rewrite honest limitation test_full_rewrite_with_recompute_passes_without_anchor_pinned asserted only the ABSENCE of named issue codes, so a future kernel that caught the full rewrite under a NEW code would have left the pin green — and the pin's headline claim (a FailedBlocked run laundered into Succeeded leaves doctor's event-store layer reporting nothing at all) rested on an out-of-tree probe rather than the test. Adds three positive assertions on the unanchored report's own event_store block: state_json_agrees / deterministic / legal_sequence are each True. Those are event-store-scoped predicates emitted by runtime.event_consistency_issues, so any new event-store-layer detection flips them, while unrelated validate_contract noise cannot. Global report["ok"] is deliberately still not asserted (design change D6). Verified they have teeth: on the same workspace with the history rewritten but the chain left unrepaired, state_json_agrees and legal_sequence both read False. Assertion-only change to one existing test: 1019 passed / 14 skipped with extras, unchanged from the previous commit. * test(zero-writes): read verbs proven side-effect-free on both store generations, zero carve-out on clean stores * feat(action): record the chain head on every run and optionally enforce it as an anchor * docs(contract): normative chain canonicalization, conformance vectors, integrity boundary, anchor trust assumptions * fix(review): whole-branch fix wave — claims-accuracy tightening, test locks, dedup 1. test_event_chain.py: delete the duplicated drifted-store CLI test (test_migrate_cli.py keeps the copy); drop the now-unused subprocess/sys imports and the duplicate `from loop.runtime import RuntimeStoreError`. 2. test_documented_conformance_vectors: also assert each vector's canonical preimage string appears literally in the contract. Verified first that all three published Preimage lines already are canonical_json output — no doc line needed regenerating. 3. test_cli_rejects_malformed_anchor_value: capture stderr and assert the "must be a 64-character lowercase hex sha256" message, so the test fails on a tree where the flag is swallowed as a positional target. 4. test_legacy_store_tamper_is_undetectable_pinned: staging self-guard — read the payload back and assert the tamper landed before the non-detection assertions. 5. New test_unhashable_record_breaks_chain: a bare NaN token in a payload (json.loads parses it, canonical_json refuses it) must surface as event_chain_broken, not a propagated ChainHashError. Required payload fields are kept intact because validate_event runs before the fold. 6. loop/reducer.py: drop the "or has an illegal gap" overclaim from ChainBreakError — a mid-stream deletion is a sequence error and a truncated tail is caught only by an anchor. 7. repo-os-contract.md 16: rewrap the mid-word break so "hash-chain-consistent" renders intact. 8. repo-os-contract.md 16: replace the full-rewrite bullet's parenthetical with what the committed pin actually asserts (three projection-disagreement codes absent, three event_store cleanliness flags true) plus the rule that a future standalone event-store cross-check must be added to the pin. 9. repo-os-contract.md 22: document the absent-store residue shape {"present": false, "sidecar_residue": true}; extend the fails-doctor code enumeration with the chain codes and invalid_event; state that chain.head is null for a BROKEN chain too and that event_chain_broken is the discriminator; state that a tamper also violating event@1 surfaces as invalid_event because validation runs before the fold.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
loop doctornow incorporates event-store consistency when.loop/events.dbexists — ADR 0001 consequence #4, the PR #48 follow-up.doctor_report(loop/contract.py) becomes a composer:validate_contractfirst with the same mode, then a lazy in-function import of the new appendedloop/runtime.pyevent_consistency_issues(target, *, mode=None), which calls thestatus/replayread-only verbs UNCHANGED as opaque black boxes.okonly ever narrows True→False;issuesonly ever appends; duplicateillegal_event_sequenceentries from status+replay are accepted, not deduped."event_store": {"present": false}.{present, readable, run_id, event_count, state_json_agrees, deterministic, legal_sequence}; any status/replay finding fails doctor with the identical issue code.corrupt_store/empty_store/ambiguous_run_id): typed doctor failure ({present: true, readable: false, error_code}), never a skip, never a traceback.reference/repo-os-contract.md: corrected §16 scope-boundary sentence + new §22 documenting the gate.scripts/test_doctor_eventstore.py: 11 nodes (2 importorskip('jsonschema') parametrizations; both validation modes covered).Evidence (governed Claudex lane — governor-run, outside the worker)
30fbac59without the fix.cx_doctor_events_a1(repair_requested — attempt 1 passedmode=values outside VALIDATION_MODES in 4 parametrized tests) →cx_doctor_events_a2(accepted, terra/high replacement session019f7073-fce8-7a23-b09c-b05e6e12fbb7).Discovered, not fixed (pre-existing kernel finding)
Holdout H4b caught that running
doctor_reporton a store-backed workspace leaves.loop/events.db-wal+.loop/events.db-shmsidecars behind. Reproduced on unfixed base viastatus_reportalone: themode=roURI connections in runtime.py recreate the WAL sidecars but cannot checkpoint/delete them on close. Zero content mutation (events.dbbyte-identical; no other file changes) — the read verbs are data-read-only but not tree-byte-identical, andstatus/replay/simulateon main share the behavior today. The fix surface (status/replay bodies, loop/events.py) is outside this packet's sanctioned scope, so this ships as a documented follow-up candidate rather than a scope expansion.