Phase 0: harden completion semantics and terminal integrity - #48
Conversation
…anonical iteration ids
Succeeded now requires EVERY declared criterion true under an explicit
completion_policy ({"mode": "all_required"}, the compatibility default for
legacy records), enforced by one shared evaluator (loop/completion.py) across
emit.terminate, integrations.to_terminal_state, and the contract doctor G1
check in both validation modes. Previously all three independently accepted
any single true criterion — a direct false-completion path contradicting the
normative spec.
terminal_state.json is now immutable: atomic create-once via a hard-link
claim (two concurrent terminators produce exactly one record), force=True
always raises, and the new emit.sync_state_to_terminal() is the narrow
repair for the terminal/state split-write window (runtime_monitor now also
treats an existing terminal record as authoritative). Iteration ids are
canonical non-negative integers (legacy decimal strings stay read-compatible
in state@1); terminal@1 gains additive completion_policy and encodes the
spec's existing evidence/criteria constraints.
Provenance: core migration authored externally (GPT-5.5 lane) against
f2e9347, applied after independent verification (premise check, security
review, manifest hashes), then hardened per an adversarial review (5
findings fixed: split-write recovery, cross-mode null policy agreement,
os.link OSError wrapping, hint narrowing, compat callout).
Full suite 455 passed / 9 skipped (was 433/9); pyyaml-only fallback 438/24
(was 417/23); self_eval 13/13; frontmatter 9/9.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c48f775311
ℹ️ 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".
| "evidence": list(outcome.artifacts), | ||
| "criteria_met": {key: value is True for key, value in canonical_criteria.items()}, | ||
| "completion_policy": normalized_policy, | ||
| "evidence": list(artifacts), |
There was a problem hiding this comment.
Validate evidence before returning failure bodies
When a higher-precedence failure is returned first (for example external_error, human_abort, or FailedSafety) and outcome.artifacts contains a blank entry or duplicates, this helper still copies those raw artifacts into evidence and never reaches the later evidence_error branch. Since emit.terminate now rejects blank/duplicate evidence for every terminal state, the adapter can produce a FailedBlocked/FailedSafety body that cannot be persisted instead of a valid failure record.
Useful? React with 👍 / 👎.
| terminal = state.get("terminal_state") | ||
| if terminal: | ||
| return terminal | ||
| terminal_path = paths.loop_dir / "terminal_state.json" |
There was a problem hiding this comment.
Use the resolved terminal path for orphan records
When terminal_state.json is in the root-level legacy location that resolve_loop_paths already supports, and state.json was never stamped, this hard-coded .loop path ignores the resolved paths.terminal. In that context the monitor falls through to stall/budget detection and can recommend replan or continue for an already terminal loop, so the new orphan-terminal handling should read the resolver-selected path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens “Succeeded” semantics and terminal-record integrity by centralizing completion-policy evaluation, enforcing all-required criteria for success across writer/projection/validation paths, and making terminal records immutable and concurrency-safe.
Changes:
- Introduces a shared completion-policy module and applies it consistently in
emit,integrations, and contract validation. - Makes terminal writes create-once/immutable (closing overwrite and concurrency race paths) and adds a narrow state-stamp repair helper.
- Updates schemas, templates, docs, and tests to reflect canonical integer iteration IDs and all-required success semantics.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/state.json.tmpl | Emit iteration_id as an unquoted value to support canonical integer IDs. |
| scripts/test_runtime_monitor.py | Adds regression coverage for “terminal exists but state.json not stamped” monitoring behavior. |
| scripts/test_integrations.py | Expands projection tests to pin all-required completion, policy failures, and evidence validation. |
| scripts/test_emit.py | Expands writer tests for stricter success semantics, immutability, concurrency, and repair path. |
| scripts/test_completion_policy.py | New tests pinning shared policy semantics and cross-validation-mode agreement. |
| scripts/runtime_monitor.py | Treats terminal_state.json as authoritative even when state.json lacks terminal_state. |
| schemas/terminal.schema.json | Adds completion_policy, tightens evidence/criteria shapes, and documents legacy interpretation. |
| schemas/state.schema.json | Tightens iteration_id to non-negative int or canonical decimal string for legacy compatibility. |
| reference/repo-os-contract.md | Updates normative contract to specify all-required completion via completion_policy. |
| README.md | Updates user-facing contract description to reflect immutable terminal records and stricter success rules. |
| loop/integrations.py | Uses shared policy evaluator; fails closed on partial/malformed criteria and invalid evidence/policy. |
| loop/emit.py | Enforces stricter success rules; implements atomic create-once terminal writes and adds sync repair helper. |
| loop/contract.py | Applies shared completion semantics in both validation modes; tightens structural checks. |
| loop/completion.py | New shared, deterministic completion-policy implementation (default all_required). |
| docs/adr/0001-proof-kernel-and-runtime.md | Adds ADR formalizing kernel-vs-runtime split and immutability/policy rules. |
Comments suppressed due to low confidence (1)
loop/emit.py:88
- _atomic_write_text() tries to delete the temp file but only ignores FileNotFoundError. If unlink fails for any other reason (e.g., permission/transient Windows file-lock), that exception will mask the original failure and/or change the error surfaced to callers.
except BaseException:
try:
os.unlink(tmp_name)
except FileNotFoundError:
pass
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def unmet_required_criteria(criteria_met: Mapping[str, object]) -> tuple[str, ...]: | ||
| """Return stable string identifiers for criteria not proven true.""" | ||
| return tuple(sorted(key for key, value in criteria_met.items() if value is not True)) |
| finally: | ||
| try: | ||
| os.unlink(tmp_name) | ||
| except FileNotFoundError: | ||
| pass |
| if isinstance(record, dict) and record.get("state"): | ||
| return str(record["state"]) | ||
| return "terminal" |
…en .loop/events.db exists (#77) 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).
Phase 0: harden completion semantics and terminal integrity
Summary
Establishes a single deterministic definition of success across the writer API,
runtime adapters, validator, schemas, templates, and normative documentation.
Succeedednow means what the public contract already says it means: everydeclared acceptance criterion is proven true, evidence is present, and the run
is not flagged as false completion.
Terminal records become immutable, and the concurrent check-then-overwrite race
in
loop.emit.terminate()is closed.Why
The previous implementation allowed
Succeededwhen only one entry incriteria_metwas true — in three independent places (loop/emit.py,loop/integrations.py, and the doctor's G1 check inloop/contract.py). Aterminal like this passed both write-time and validation gates:
{ "state": "Succeeded", "criteria_met": { "tests_pass": true, "security_review": false, "deployment_verified": false } }That contradicted the normative contract's success model
(
reference/repo-os-contract.mdprose already declarediteration_id | intand terminal_state.json "written exactly once" with no force clause) and
created a direct false-completion path.
force=Truealso meant a terminal record was not actually an immutable auditdecision, and the separate existence-check/replace operations let concurrent
writers race (TOCTOU).
Changes
Shared completion policy
loop/completion.py:normalize_completion_policy,criteria_satisfy_completion,unmet_required_criteria.{"mode": "all_required"}. Nonempty criteria map,exact boolean
truefor every entry (truthy1does not count).terminal@1records without the field are read asall_required.emit.terminate,integrations.to_terminal_state, andcontractG1 (both validation modes) —the write path, projection path, and read/validate path can no longer drift.
Writer hardening (
loop/emit.py)stored as canonical non-negative integers.
os.link(fsync'd temp fileterminal decision; the loser gets a clean
EmitError.force=Truecalls now always raise with an actionable message; theoriginal record is never replaced.
Runtime-neutral projection hardening (
loop/integrations.py)FailedUnverifiable; unsupported/malformed completion spec →FailedSpecGap; blank/duplicate evidence →FailedUnverifiable.preserved.
Contract and schema updates
validation modes.
completion_policyadded toterminal@1as an additive optional field(records without it still validate — conformance rule D2 holds).
iteration ids. These constraints encode what the normative spec prose
already required; the schemas had been deliberately loosened below the spec
and are now realigned with it.
state@1writers emit canonical integers; canonical decimal strings(
"0","7") remain read-compatible for legacy records.templates/state.json.tmplrendersiteration_idunquoted.reference/repo-os-contract.md§8 field table + example + §14 B2 updated tothe all-required wording; README force/writer sentences updated.
Architecture decision
docs/adr/0001-proof-kernel-and-runtime.md): separate theportable proof kernel from the first-party execution runtime. Governing
rule: agents propose; the kernel disposes. Next persistence milestone:
SQLite/WAL-backed immutable
EventStore.Provenance and verification
The core migration was authored externally (GPT-5.5 deep-review lane) against
f2e9347and applied here after independent verification:loop/emit.py:221,loop/integrations.py:119,loop/contract.py:222; docs/spec confirmed tosay "all"/"every"/"written exactly once".
paths; manifest hashes verified 14/14.
reference/repo-os-contract.mdandREADME.mdwere hand-edited (thebundle's doc anchors assumed a non-table format).
criteria_met(pre-existing
scripts/test_loop_contract_core.pypins the field name).os.linksemantics smoke-tested on WSL2/DrvFS (create +FileExistsErroron second link both correct).
An independent adversarial review of the applied diff then found five issues
(1 HIGH, 2 MEDIUM, 2 LOW), all fixed here with pinning tests:
force=Trueeliminated the only recovery pathfor the pre-existing split-write window between
terminal_state.jsonandstate.json, andscripts/runtime_monitor.py(which read onlystate.json) recommendedreplanfor an already-terminated loop. Fixedthree ways: new narrow repair op
emit.sync_state_to_terminal()(stampsstate.json from an existing terminal record, never touches the terminal
file);
runtime_monitor._terminal_dispositionnow treats an existingterminal_state.jsonas authoritative; and a failed post-link state writeraises
EmitErrornaming the repair op instead of a rawOSError."completion_policy": nullwas doctor-cleanin structural-fallback mode but a schema violation in jsonschema mode;
terminal@1now types the field["object", "null"]and a test pinscross-mode agreement.
FileExistsErrorOSErrors from theos.linkclaim (filesystems without hard-link support) now raise
EmitErrorlikeevery other refusal path.
loop/completion.pycriteria hints narrowed toMapping[str, object](the invariant every call site enforces).Succeededrecords with mixed criteria now fail doctor and need re-verification.
Gates (all green, after review fixes):
validate_frontmatter/self_evalpy_compile loop/*.py scripts/*.py+ schema JSON parseTest coverage strictly grows (17→21 emit tests, 14→18 integration tests, +6
policy tests); no existing assertion weakened — every changed assertion tracks
an intentional semantics change (string→int iteration id, force-overwrite→
force-refusal).
Compatibility
Additive for valid historical records: missing
completion_policymeansall_required; canonical legacy state id strings remain readable; new writersemit the stricter canonical forms. Behavior intentionally changes for
dishonest or ambiguous records: partial success, terminal overwrite, malformed
evidence, malformed criteria, and noncanonical ids now fail closed.
Follow-up
EventStore+ SQLite/WAL so terminal creation, state reduction, receipts,and runlog projections derive from one transactional immutable event stream
(removes the remaining split-write boundary between
terminal_state.jsonand
state.json;sync_state_to_terminalis the interim repair).state.json unstamped) so the crash residue is surfaced, not just tolerated.