Require reviewed workflow source hash on start

[andrei-hasna]

Problem

Managed workflow creation upserts a specification under a stable workflow record ID. Before this change, start accepted that record ID without the hash of the reviewed source, so a same-thread upsert could replace the reviewed YAML before start and the run could execute different source without an identity mismatch.

Transactional, immutable-source solution

Workflow creation now returns the SHA-256 of the canonical stored YAML. Both the direct manage_workflow start action and the app-server v2 start request require that reviewed hash. Run creation reads the current specification and compares the expected hash inside the same database transaction; a mismatch fails before a run or goal plan is created. A matching start stores both the hash and the full source YAML on the run, and later activation reads that immutable run snapshot instead of a mutable specification. Idempotency replays also refuse a different source hash.

Acceptance behavior

• A matching reviewed source hash starts the workflow and records that hash on the run.
• A post-review upsert that keeps the workflow record ID but changes the YAML is rejected before any run or goal-plan side effect.
• Omitting the expected source hash is rejected on both supported start surfaces.
• Manager-tool and app-server behavior remain aligned.
• Source canonicalization removes exactly one terminal LF or CRLF before hashing; no terminal line ending and multiple terminal line endings remain deterministic and covered by tests.
• Updating a workflow after a run starts does not change the run source snapshot, and an idempotency replay cannot cross source hashes.

Migration and compatibility

Migration 0071_workflow_run_source_yaml_snapshot.sql adds a non-null source snapshot column with an empty default so older binaries remain writable during migration. Existing rows are hydrated only when the current stored specification bytes and SHA exactly match the run's recorded source SHA. A legacy row whose specification changed is terminally rejected before activation instead of being backfilled from mutable source or retried indefinitely.

Existing workflow record IDs, list/read behavior, and same-thread upsert identity remain unchanged. Start is intentionally tightened: callers must pass the source hash returned by workflow create/read (expected_source_yaml_sha256 for the manager tool and expectedSourceYamlSha256 for app-server v2).

Candidate and verification

• Reviewed base: 2fd98cf2544e08f445a57bd76f27b311eeb23819
• Current head: 476c5d02b852c520c09842b7cfb7dacf9a61bc43
• Merge-result tree: acf45474371a71c95449c48a45c908784915817c
• Exact affected-lane Testbox: 31577192220 (running; final result will be recorded before review)

Exact affected-lane command:

cd codex-rs && cargo fmt -- --config imports_granularity=Item --check && just test --test-threads 4 -p codex-state -p codex-workflows-extension -p codex-app-server-protocol -p codex-app-server -p codex-tui && just argument-comment-lint

Tracking and review

Todos implementation task: OPE2-00322 — 40083146-e1a1-4c59-a7b6-a164e27ff37b.

Focused remediation review task: OPE2-00323 — 5d9bec41-b90c-4d51-93e1-159b23fbe2ff. The same independent Codewith reviewer fixed before the initial pass will review only the initial P1 migration/activation findings and direct regressions after the exact Testbox gate succeeds.

[andrei-hasna]

[REVIEW] NO_GO — #557 @ 275f903 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Reviewed the exact range 9301a835ec48dc555f1fe3be8882d68a87c1bbe6...275f903661da3094cf4ea69fb6046222cfcc4a7a.

What I ran and read:

• git log --oneline origin/main..HEAD — exit 0; one commit: 275f90366 Require reviewed workflow source hash on start.
• git diff origin/main...HEAD --stat — exit 0; 26 files, 1,853 insertions, 99 deletions.
• Full diff of all 26 changed files plus surrounding app-server protocol/request processing, workflow save/create/snapshot/migration, activation/supervision, manager tool, goal-plan projection, cancellation/branch-agent/automation cleanup, TUI start/menu/session, and associated tests — read in full.
• bun install — exit 0; setup only, not a test gate; pass/fail count not applicable. The generated untracked bun.lock was removed after setup, leaving the review worktree clean.
• package.json declares no typecheck script.
• THIS REPO DECLARES NO TEST GATE in package.json; there is no test script. I did not invent or run a substitute local gate.
• gh pr view 557 --repo hasna/codewith --json statusCheckRollup — exit 0; 28 checks: 19 success, 4 failure, 2 in progress, 3 skipped.
• Additional exact-head workflow diagnostic, not a package.json gate: gh run view 31520753856 --repo hasna/codewith --job 93876916425 --log — exit 0 for log retrieval; the remote Testbox build command exited 100 with 493 tests run: 492 passed, 1 failed, 0 skipped.

Authoritative forge check rollup read by name:

• Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — IN_PROGRESS
• cla — SUCCESS
• Check for spelling errors — SUCCESS
• Blob size policy — SUCCESS
• cargo-deny — SUCCESS
• build-test — SUCCESS
• Build Codewith Linux CLI — SUCCESS
• Detect changed areas — SUCCESS
• python-sdk — SUCCESS
• Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — IN_PROGRESS
• Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — SUCCESS
• Format / etc — SUCCESS
• sdks — SUCCESS
• Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — SUCCESS
• Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — FAILURE
• Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — SUCCESS
• Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED
• cargo shear — SUCCESS
• Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — FAILURE
• Argument comment lint package — SKIPPED
• Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — FAILURE
• Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS
• Argument comment lint - Linux — SUCCESS
• Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
• Argument comment lint - Windows — SUCCESS
• Bazel test on windows-latest for x86_64-pc-windows-gnullvm — FAILURE
• CI results (required) — SUCCESS
• [code]smith — SKIPPED

Blocking P0/P1 findings:

1. P1 — invalid source-snapshot failure can leave child agents and automation running after the parent is terminal. Migration 0071_workflow_run_source_yaml_snapshot.sql deliberately leaves legacy run snapshots empty. On restart, activation.rs:896-929 sends any nonterminal run whose snapshot cannot be hydrated or parsed to fail_workflow_run_if_source_snapshot_invalid. That function (state/src/runtime/workflows.rs:633-702) only marks workflow_runs failed, clears the parent lease, and appends an event. It does not stop linked background agents, cancel pending interactions, release their worktree leases, cancel workflow timers/monitor links, or terminalize child steps/verifiers. The established terminal cleanup path in workflow_orchestrator.rs:3623-3689 explicitly performs all of those operations. Therefore an upgraded active run whose mutable spec changed after the run was created reaches the new fail-closed branch, reports a terminal failed parent, and can continue executing already-admitted child work. Remedy: run the same branch-agent, automation, and child-state cleanup transaction when invalid snapshot failure terminalizes a run, then add a regression fixture with an active linked branch agent and active timer/monitor proving they are stopped/cancelled and leases are released.

2. P1 — exact-head build/test evidence is not green. The forge rollup has four failures and two checks still in progress. Independently, Blacksmith Testbox run 31520753856 fails the PR's new changed_spec_does_not_hydrate_empty_snapshot_and_fails_terminally test twice at state/src/runtime/workflows.rs:2870. The test changes Dental Lead SaaS to Changed Spec, which changes the workflow identity, then asserts the second save reused the original workflow_record_id; the measured result is 492 passed, 1 failed, process exit 100. Fix the fixture so it mutates content without changing workflow identity, and make the current forge rollup green.

Non-blocking follow-ups:

• package.json has no declared test gate. That is a real repository-level verification gap, but it is not a licence to invent a local gate and is not the basis of this NO_GO.
• The change is large (1,952 changed lines across 26 files). A smaller landing sequence would improve reviewability, but size alone is non-blocking.

[andrei-hasna]

[REVIEW] NO_GO — #557 @ 476c5d0 — fixed reviewer 1 of 1\n\nBlocking P1 findings:\n\n1. Invalid source-snapshot terminalization still skips established cleanup. The path in codex-rs/state/src/runtime/workflows.rs marks only the workflow run failed, clears its owner/lease, and appends an event. The reachable activation path does not stop linked branch agents, cancel pending interactions, release background-agent worktree leases, cancel timers/monitor links, or terminalize child steps/verifiers. The established cancellation path in workflow_orchestrator.rs performs those operations. Existing regression tests assert run status/generation/event count only and do not prove branch-agent, timer, monitor, lease, pending-interaction, step, or verifier cleanup.\n\n2. Required exact-head hosted gates are incomplete. The head/base pair is 476c5d0 / 2fd98cf and mergeStateStatus is UNSTABLE. Fourteen required checks were still in progress at review time, including Linux/Windows Bazel tests and clippy, release builds, Linux CLI build, SDKs, and argument-comment lint.\n\nThe prior workflow-identity fixture defect is fixed: the changed-spec test now preserves the same thread and workflow identity before testing the changed hash.\n\nEvidence: exact immutable head inspection across the workflow storage, activation, orchestration cleanup, automation, migration 0071, manager/API propagation, and changed-spec tests; git diff --check returned rc=0 with empty stdout/stderr. No additional reachable in-scope P0/P1 defect was identified. Focused remediation must address only the cleanup defect and direct regressions; required CI must reach terminal success.