fix: refuse resume of pre-cause State schema with actionable error - #91
Merged
Merged
Conversation
PR #74 added the `node.cause` column via `CREATE TABLE IF NOT EXISTS` only, which is a no-op against a `node` table that already exists. A run directory created by a caw version before that column therefore reopens a `node` table with no `cause` column, and the first terminal node write through `record_node_finished` (which always writes `cause`) crashed the resume with a raw `sqlite3.OperationalError: no such column: cause`. Resume offered no schema-version gate, so a stale-schema run directory failed with a confusing raw error mid-resume, after the Run row had already flipped back to `running`. caw is pre-1.0 with no documented state-schema-stability guarantee, so resume now refuses a pre-`cause` run directory up front with an actionable `ResumeError`, consistent with the #70 resume-guard pattern, rather than migrating it in place. The refusal fires before the Run row is flipped to `running`, so a refused resume leaves State untouched. - StateStore.node_table_has_cause() detects the missing column via PRAGMA table_info, where schema knowledge already lives. - resume_run() raises an actionable ResumeError when the column is absent, alongside the existing snapshot/checksum/adapter guards. - A seam test builds a pre-`cause` `node` schema and asserts resume refuses with the actionable error and never leaks the raw sqlite error. Closes #76 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
qinhaihong-red
added a commit
that referenced
this pull request
Jun 15, 2026
#66, #67) (#92) * feat(env): add shell-node env allow-list and document the env-policy scope (#66) Give shell Nodes env parity with agent Nodes: `ShellNodeInputs` gains a node-generic `env` allow-list of variable NAMES, resolved through the same policy as the Agent-CLI seam — only declared-and-present variables reach the process and their values never reach State, Events, or the snapshot. The allow-list ENGAGES only when a shell Node declares `env`; an undeclared shell Node inherits the parent environment unchanged, preserving pre-#66 behavior so existing shell Workflows keep working. A declaring Node owns listing every variable its command needs (including PATH), mirroring the claude.print Adapter contract. ADR 0006 now records that the env allow-list is node-generic (agent AND shell), the v0.1 scope, and the stdout-echo caveat: the policy guards env injection and kernel-held values, not output redaction — a Node that echoes a secret into its own output persists it verbatim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(contract): cache the compiled output validator and validate artifact existence (#67) Two #5-review follow-ups in the output-contract / artifact subsystem: - Compile one Output Contract validator per resolved schema path and reuse it across node attempts (and runs in one process) instead of re-reading, re-parsing, re-meta-schema-checking, and re-constructing it every time. The cache is keyed by path AND the file's modification stamp, so a rewritten schema recompiles rather than serving stale. The executor now runs the validator off the asyncio event loop via asyncio.to_thread, so the (cache-miss) blocking read+compile cannot stall the scheduler driving concurrent Nodes. - Validate an adapter-supplied artifact path is an existing FILE before indexing it in State, so State never over-promises a "durable file produced by the run" that never existed (a non-existent path or a directory is dropped). The full artifact lifecycle stays deferred to #16; this is the minimal existence guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(conftest): hoist shared seam helpers into conftest, drop duplicate copies (#67) The run-directory / State / Events inspection helpers (single_run_dir, state_rows, read_events) and the schema/fixture/agent-workflow builders (write_schema, write_fixture, agent_workflow) were copied verbatim across the seam test files. Move them into tests/conftest.py once and import them where used, so a future change edits one definition instead of several. The CLI-specific single_run_dir(tmp_path) and state_row (singular) in the run-directory-seam tests keep their own definitions — they have different signatures and are not duplicated — so only the verbatim copies are removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(env): reject value-shaped names and distinguish omitted from empty env (#66) Address the two blocking review points on PR #92. Reject value-shaped / invalid env entries: the env allow-list declares variable NAMES, never values, but the shared validator only rejected blank and duplicate names, so `env: ["API_TOKEN=s3cr3t"]` was accepted and a secret-looking value landed in the normalized snapshot. The shared validator now requires every entry to be a valid POSIX environment-variable name (`^[A-Za-z_][A-Za-z0-9_]*$`), rejecting an embedded `=`, a leading digit, a space, and the empty string, while keeping the duplicate check. Applied to both agent and shell node `env`. Distinguish an omitted `env` from an explicit empty `env: []`. An empty tuple was treated as undeclared, so the shell inherited the WHOLE parent environment, collapsing two distinct cases. The field default is now `None` (omitted): an omitted `env` preserves legacy parent-environment inheritance (executor env = None), while an explicit `env: []` is a declared empty allow-list and the shell receives no variables (executor env = {}), per ADR 0006. An agent Node never inherits the parent environment, so omitted and empty both resolve to {} there. The normalized snapshot serializes an omitted `env` as `null` (distinct from `[]`), so the distinction survives a resume round-trip. ADR 0006 now documents the name-validation and explicit-empty = no-vars rules. Also notes in `_existing_artifacts` that run-directory SCOPING of artifacts (accepting an existing file outside the run directory) is owned by #16, not the existence guard. Tests: reject `NAME=value` and one more invalid form for both an agent and a shell node; an explicit `env: []` shell node does not see a parent variable while an omitted-`env` node still inherits; omitted vs explicit-empty round-trip distinctly in the snapshot and across a resume. Fixed the misleading agent-seam test comment that claimed `name=value` was rejected (it only checked duplicates). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(contract): key the validator cache on the resolved schema path (#67) `compile_output_validator()` documented caching by resolved path but keyed the `lru_cache` on `str(schema_path)`, so two equivalent spellings of one schema file (a `.`/`..` detour, a symlink) compiled duplicate validators. Resolve the path before stating and keying it, so equivalent paths share one compiled validator — matching the documented behavior. Also documents that the `lru_cache` is intentionally not single-flight: a concurrent duplicate compile under a cold miss is harmless (idempotent, no shared mutable state, no correctness impact), so it is not worth a lock. Test: two equivalent paths to one schema file return the SAME validator object. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: satisfy ruff format on the merged test file The main-merge that brought #90/#91 into this branch left a hand-wrapped line in tests/test_executor_seam.py that `ruff format --check` (a CI gate) rejects. Reformat it; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: haihong.qin <haihongqin@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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
Resuming a run directory created before the
node.causecolumn existed crashed with a rawsqlite3.OperationalError: no such column: cause(issue #76). PR #74 addednode.causeviaCREATE TABLE IF NOT EXISTSonly, which is a no-op against an existingnodetable, so an older run directory reopens anodetable without the column. The first terminal node write throughrecord_node_finished(which always writescause) then crashed the resume — uncaught out of the scheduler — after the Run row had already flipped back torunning.How
The issue left the choice open between an idempotent
ALTER TABLEmigration and a clean refusal. This PR takes the refuse-with-actionable-ResumeErrorpath, consistent with the #70 resume-guard pattern (snapshot / checksum / adapter refusals), because caw is pre-1.0 with no documented state-schema-stability guarantee. The guard fires beforerecord_run_running, so a refused resume leaves State untouched rather than half-flipping an interrupted run torunning.StateStore.node_table_has_cause()— detects the missing column viaPRAGMA table_info(node), instate.pywhere schema knowledge already lives.resume_run()— raises an actionableResumeError("predates a State schema change … cannot be resumed") when the column is absent, alongside the existing Follow-ups from the #6 failure-semantics review #70 eligibility / snapshot / checksum / adapter guards. The CLI already mapsResumeErrorto exit code 2.Scope kept minimal and localized to the resume/schema concern (no refactor; no touch to
adapter.py/model.pyor the executor's artifact-indexing area).Testing
TDD red→green. A new seam test (
test_resuming_a_run_whose_node_table_predates_the_cause_column_is_refused) rebuilds a real failed run'snodetable to the pre-causeschema, then asserts resume refuses with the actionableResumeErrorand never leaks the rawsqlite3.OperationalError. Offline-testable behavior (no real agent CLI involved), so it is a non-e2e seam test by design — no real-agent e2e added.Gates:
uv run pytest -m "not e2e" -q→272 passed, 6 deselecteduv run ruff check .→ All checks passeduv run mypy src/caw tests→ Success, no issuesCloses #76
🤖 Generated with Claude Code