fix(goal): harden blocked-goal hold semantics

[andrei-hasna]

Task: 78b9a1ee-a33b-4924-8c54-4ace858f82a4

Architecture

• Persist and bound deduplicated blocker-audit state by goal, turn, and stable fingerprint.
• Pause the first two matching turn-error observations and transition the third consecutive matching goal turn to blocked, while preserving resume and goal-plan semantics.
• Hold recurring schedules for matching held goals, preserve terminal schedule precedence, and retain default-derived goal objectives across generic submit failures.
• Harden error fingerprint propagation and failure finalization so stale accounting and state-store failures do not corrupt the next turn.

Regression evidence

• git diff --check: pass.
• Focused blocker audit: 4/4 passed.
• Full codex-state: 358/358 passed.
• Full goal_extension_backend: 61/61 passed.
• thread_schedule_runtime: 54/54 passed, 462 skipped.
• Core lifecycle slice: 18/18 passed, 2945 skipped.
• Release invalid_image_lifecycle: 1/1 passed.
• just fmt and just fmt-check: pass.
• Scoped Clippy for all changed crates: pass with explicit allows for two unchanged upstream-only lints (while_let_loop, byte_char_slices).
• just bazel-lock-update and just bazel-lock-check: pass.
• Final moving-base check against aae8f81a: virtual merge clean; only overlapping path was codex-rs/Cargo.lock, whose effective delta is one sqlx dependency entry. cargo metadata --locked and Bazel lock update/check passed on the hypothetical merged tree.
• Redacted committed-range secret scan: 2 commits, no leaks.
• Two independent adversarial reviews: no P0/P1 findings.

Accepted risks and toolchain caveats

• A narrow schedule-finish versus external goal-resume TOCTOU can produce at most one extra fired or paused run; no data-loss path was found.
• The invalid-image regression is release-only by cfg. An unbounded release build exceeded the 8 GiB runner; the bounded release profile passed.
• Combined isolated targets exhausted the 21 GiB runner disk. Exact affected suites were rerun in clean targets and passed.
• Existing Bazel well-known-crate warnings remain unchanged.

No merge, publish, install, restart, deploy, migration, or rollout was performed.

---

Round 2 — review follow-ups (head 15bb302, rebased onto origin/main @ b71a05e)

Blocker 1 — the invalid-image test never executed in CI (fixed)

codex-rs/core/tests/invalid_image_lifecycle.rs opened with
#![cfg(not(debug_assertions))], so it compiled to an empty test binary in every
lane that exists: Bazel builds --compilation_mode=fastbuild (debug assertions on) and
"Verify release build" only builds, never tests. The headline fix
(emit_turn_error_lifecycle_with_protocol_error in core/src/session/turn.rs) therefore
had zero executed coverage, and the previous "Release invalid_image_lifecycle: 1/1
passed" line in this description was a local, unverifiable claim. That line is retracted.

The gate was investigated rather than worked around; there were two real causes, and
both are fixed at the source:

1. Stack overflow, not a debug assertion. A full turn future does not fit in the
   2 MiB default #[tokio::test] worker stack in a debug build — the worker aborts with
   fatal runtime error: stack overflow. Reproduced locally at the previous head. The
   shipped binary already runs turns on 16 MiB worker stacks
   (TOKIO_WORKER_STACK_SIZE_BYTES in codex-arg0), so the test now builds its own
   runtime with the same sizing, matching the existing repo pattern
   (core/tests/suite/rmcp_client.rs, app-server/src/in_process.rs, tui/src/app/tests.rs).
   This is env-independent: it does not rely on a CI lane exporting RUST_MIN_STACK.
2. error_or_panic on a non-invariant path. The CodexErr::InvalidImageRequest arm
   called crate::util::error_or_panic, which panics in every debug build and aborts
   the turn task before the recovery/emit code runs. An upstream 400 for a bad image is an
   externally triggered condition, not a broken local invariant, and the branch below it is
   the designed handling for exactly that. It now logs with error! — byte-identical to
   what error_or_panic already did in release, so release behaviour is unchanged.

Proof both were required, at the new head: with the stack fix but error_or_panic
restored, the test fails timeout waiting for event (the turn task panicked); with both
fixes it passes in a plain debug build.

cargo nextest run --locked -p codex-core -E 'binary_id(codex-core::invalid_image_lifecycle)'
    PASS [0.552s] (1/1) codex-core::invalid_image_lifecycle
                        invalid_image_run_turn_preserves_bad_request_and_stable_fingerprint

core/tests/suite/view_image.rs::replaces_invalid_local_image_after_bad_request keeps its
own cfg(not(debug_assertions)) gate: it is network-gated and lives in the
codex-core::all binary, so ungating it could not be proven in this PR's CI. Left as
pre-existing.

Blocker 2 — dead _and_pause API removed

complete_thread_schedule_run_and_pause and fail_thread_schedule_run_and_pause were new
public ScheduleStore methods with no production callers, and the pause_schedule: true
branch of finish_thread_schedule_run_once was unreachable outside mod tests. Production
pauses a schedule only through a goal hold, via the *_thread_schedule_run_for_goal entry
points and the pause_for_goal_hold probe.

Both wrappers and the pause_schedule field on FinishScheduleRun are removed;
pause_schedule is now simply pause_for_goal_hold. The three tests that used the dead
API were rewritten onto the real goal-hold path with a Blocked goal, which is strictly
better coverage — completed_schedule_run_for_held_goal_pauses_without_rearming now also
passes a non-None next_run_at, proving the hold beats a caller-requested rearm.

Quality nits (all three applied)

• claim_thread_schedule_once and finish_thread_schedule_run_once now open the goals.db
  probe with a deferred begin() instead of begin_with("BEGIN IMMEDIATE"). The probe is a
  read-only SELECT EXISTS that is always rolled back; the write lock was held across the
  state.db commit for no benefit. Lock order is consistently state -> goals at both sites,
  so nothing needed the immediate lock to preserve ordering.
• Removed the guaranteed no-op re-insert loop in observe_active_thread_goal_blocker_once:
  it ran exactly in the preserves_current_streak branch where the observed_turns rows
  were never deleted, so every INSERT hit ON CONFLICT DO NOTHING. The now-unused
  created_at_ms column was dropped from the probing SELECT.
• The unreachable!() in finish_thread_schedule_run_once is downgraded to an
  anyhow::bail!, so drift in the goal_tx/goal_hold_can_pause invariant cannot panic
  out of a state-store write path.

Documented tradeoff (behaviour intentionally unchanged)

Blocking requires 3 consecutive turns with an identical fingerprint, and fingerprints embed
the HTTP status, so a flapping upstream (502/503/502) resets the streak. This matches the
model-facing contract at ext/goal/src/spec.rs:298 and is left as-is; a doc comment on
fingerprint_with_http_status now records it as a known tradeoff rather than an accident.

Rebase and migration numbering

Rebased onto origin/main @ b71a05e (picks up #393 and #406); clean, no conflicts. The
merged-tree migration-prefix check was re-run: no duplicate numeric prefixes in any of the
four migration dirs, state/migrations is contiguous 0001..0062 (62 files) with 0062
uniquely claimed, state/goals_migrations contiguous 0001..0009 (9 files), and the
runtime.rs baseline stays at 61.

Round-2 local verification (new head)

• cargo fmt -- --config imports_granularity=Item --check: clean.
• cargo clippy --tests --locked -p codex-state -p codex-core -p codex-goal-extension -p codex-app-server -p codex-extension-api -- -D warnings -A clippy::byte_char_slices -A clippy::while_let_loop: clean.
• cargo nextest run -p codex-state -p codex-goal-extension: 456/456 passed.
• cargo nextest run -p codex-core -E 'not binary_id(codex-core::all)' with RUST_MIN_STACK=8388608 (the value CI exports): 2193/2201 passed. The 8 failures are local-workstation artifacts unrelated to this diff — config_loader/git_info/realtime_context pick up a stray /tmp/.codewith and the surrounding git layout, and shell_snapshot needs a real login shell.
• Staged secrets scan: 0 findings.

Round-2 CI evidence (head 15bb302)

Bazel / release lanes on Linux — all four green on this head:

check	conclusion
Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu	SUCCESS
Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl	SUCCESS
Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu	SUCCESS
Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu	SUCCESS

Every other required check is SUCCESS too (CI results (required): SUCCESS). The only
non-success entries are two conditionally skipped jobs (Argument comment lint package,
Bazel test on windows-latest ... (native main)).

Blacksmith testbox — run on the new head, with -p codex-core and
-p codex-extension-api added so the changed codex-core code is executed, not merely
compiled by clippy --tests:

• Run 30146755675 (head 15bb302):
  3897 tests, 3895 passed, 2 failed. The invalid-image test appears by name as a PASS:
  PASS [0.312s] (2715/3897) codex-core::invalid_image_lifecycle invalid_image_run_turn_preserves_bad_request_and_stable_fingerprint.
  The 2 failures were codex-core shell_snapshot::tests::try_new_creates_and_deletes_snapshot_file
  and try_new_uses_distinct_generation_paths, both snapshot should be created: "validation_failed".
• Run 30147337015 (baseline on main @ 23a664c,
  same runner image, same two tests): 2 tests run: 0 passed, 2 failed, identical
  validation_failed panics. Those two are therefore pre-existing and environmental
  (the runner's login shell fails snapshot validation) and are not caused by this PR.
• Run 30147333548 (head 15bb302,
  same command with the two proven-pre-existing tests excluded): conclusion = success,
  3895 tests run: 3895 passed (1 slow), 27 skipped, again including
  PASS codex-core::invalid_image_lifecycle invalid_image_run_turn_preserves_bad_request_and_stable_fingerprint
  plus the rewritten schedule tests
  (completed_schedule_run_for_held_goal_pauses_without_rearming,
  explicit_expiry_terminalizes_active_run_before_late_completion,
  terminal_schedule_status_rejects_late_finalizers) and the two new fingerprint unit tests.

Exclusions kept, and why:

• binary_id(codex-core::all) — already red on main in this runner image
  (suite::rmcp_client / suite::shell_snapshot / sandbox failures, see run 30121948648 on main).
  Not this PR's to fix. Note the Bazel ubuntu-24.04 lanes do run that binary and are green there.
• test(background_agent_live) — live-network suite, unchanged from the previously agreed base command.
• test(shell_snapshot::tests::try_new_) — added only to the confirmation run, after the
  main baseline above proved both cases fail identically without this diff.

[andrei-hasna]

CI triage disposition: the Windows //codex-rs/hooks:hooks-unit-tests failure is deterministic on the exact base, not introduced by this PR. Base aae8f81a failed beforehand in run 29955421552 (jobs 89043145887 and 89043145714) with the same three trusted-hash/discovery assertions on the same windows-2025-vs2026 image and Bazel 9.0.0; PR job 89045247749 failed identically. fleet_comms.rs and codex-hooks dependencies are identical at base, head, and synthetic merge. No rerun or branch change was made because this is not a flake. Tracked separately in Todos task b36922d5-e6aa-4842-ad0b-aae41c498e2b; it should not be folded into #379.

[andrei-hasna]

Rebased onto main + proven green on a Blacksmith testbox

Rebase: rebased fix/goal-hold-blocked-semantics-78b9a1ee onto origin/main (86b7e78eb, which now carries .github/workflows/blacksmith-testbox.yml). No conflicts. Migration numbering re-verified after the rebase: state/migrations/0061 and goals_migrations/0009 are still the next free slots (main is at 0060 / 0008).

One new commit (fix(state): keep schedule updates clear of the live-owner lease guard) fixes a regression this PR introduced against the pre-existing thread_schedules_ignore_legacy_live_owner_claim trigger:

• thread_schedules_ignore_legacy_live_owner_claim is a BEFORE UPDATE OF lease_id trigger, so it fires on the mere mention of lease_id in a SET list and drops the whole statement via RAISE(IGNORE).
• This PR had started writing lease_id = CASE WHEN ? = 'active' THEN lease_id ELSE NULL END in update_thread_schedule, so an ordinary active-status edit was silently discarded whenever a legacy (non owner:-scoped) lease was held and a local session was live. Fix: emit the lease columns only for non-active transitions (where NEW.lease_id IS NULL and the trigger cannot fire anyway) — semantics are unchanged.
• The claim path had also switched to fetch_one, which would panic-error when the same trigger suppressed the claim update. Fix: fetch_optional, treat a suppressed claim as "unclaimed", and roll back the speculative expired-run reap so the live owner keeps its runs.
• Covered by a new regression test: update_active_thread_schedule_applies_while_legacy_lease_and_live_owner_exist.

Build gate — Blacksmith testbox

Run 30119889797 on blacksmith-16vcpu-ubuntu-2404 — conclusion: success.

sudo apt-get install -y --no-install-recommends pkg-config libcap-dev bubblewrap
cd codex-rs
cargo fmt -- --config imports_granularity=Item --check
cargo clippy --tests --locked -- -D warnings -A clippy::byte_char_slices -A clippy::while_let_loop
cargo nextest run --locked -p codex-state -p codex-goal-extension -p codex-app-server \
  --no-fail-fast -E 'not test(background_agent_live)'

Summary [373.210s] 1680 tests run: 1680 passed (1 slow), 18 skipped — fmt clean, clippy clean under -D warnings.

Why the two lint allows and the one test exclusion

Both are pre-existing on main, not introduced here:

• clippy::byte_char_slices (x2) fires in core/src/unified_exec/process_manager_tests.rs:151,176 — a file this PR does not touch (last changed by feat(unified-exec): bound output collection + parallelize write_stdin (upstream cluster #16) #327). clippy::while_let_loop is likewise upstream-only. Everything else still runs under -D warnings.
• codex-app-server::request_processors::background_agent_live::tests (6 tests) fail on Blacksmith runners with timed out waiting for worker process group N to reap. Baseline run 30119891679 executed exactly those 6 tests on main on the same runner class and they fail identically — an environment issue with the worker fixture, not a regression from this PR. This module is not touched by this PR.

Repo CI on the rebased head is otherwise green, including CI results (required), Format / etc, both Verify release build jobs, Bazel clippy, and the Windows Bazel shards. The Bazel test on ubuntu-24.04 jobs hit the same background_agent_live worker-reap timeout (plus one agent_start_list_read_and_events_survive_app_server_restart flake — that test passes in the Blacksmith nextest run above).

No merge, publish, install, deploy, or rollout performed.

[andrei-hasna]

Worktree triage note (stale-worktree sweep, 2026-07-24)

The original worktree for this branch (78b9a1ee-goal-hold-blocked-semantics, at /home/hasna/.hasna/repos/worktrees/station01/open-codewith/78b9a1ee-goal-hold-blocked-semantics) still held uncommitted work that is NOT on this PR's head (264d0cd27): ~1.9k added lines across 16 paths, including a new migration codex-rs/state/migrations/0062_thread_schedule_run_occurrence_idempotency.sql plus further work in state/src/runtime/schedules.rs, core/src/tasks/mod.rs, core/src/session/* and app-server/src/request_processors/thread_schedule_runtime.rs.

The 7 committed subjects on that local branch already exist on this PR under rebased SHAs, but the uncommitted delta does not. To avoid losing it I committed it as 72c6c0118 and pushed it to a preservation branch:

rescue/goal-hold-scheduler-lease-78b9a1ee

It is not built or tested (cold codex-rs build), and no competing PR was opened. Whoever drives #379 should cherry-pick or discard deliberately.

[andrei-hasna]

Follow-up: the two Bazel test on ubuntu-24.04 reds were confirmed flaky — re-running the failed jobs turned both green (background_agent::agent_start_list_read_and_events_survive_app_server_restart raced Starting vs the expected Queued; the background_agent_live worker-reap timeouts also cleared). Repo CI on 264d0cd is now 25 pass / 0 fail, PR state MERGEABLE / CLEAN, alongside the green Blacksmith testbox gate (run 30119889797).

[andrei-hasna]

Rebased onto main + duplicate sqlx migration version fixed

1. Rebase. Rebased fix/goal-hold-blocked-semantics-78b9a1ee onto origin/main
(54333c9e0) — clean, no conflicts. Head is now ddcdf33b6.

2. Blocker fixed: duplicate sqlx migration version (invisible to the merge button).
This branch shipped codex-rs/state/migrations/0061_thread_schedule_run_goal_id.sql, but
main landed 0061_background_agent_lifecycle_receipts.sql in #395 after this branch was
last numbered. Git reported the merge as MERGEABLE / CLEAN because the two files never
touch — yet the merged tree carried two migrations at version 61. sqlx 0.9 parses the
numeric filename prefix as the version and performs no duplicate-version check, so
sqlx::migrate! would have compiled cleanly and only failed at runtime, on every fresh
database.

• Renumbered to 0062_thread_schedule_run_goal_id.sql (next genuinely free slot).
• No tracked reference used the old filename (BUILD.bazel globs migrations/**).
• The paired test baseline moved from migrator_through(&STATE_MIGRATOR, 60) to 61 so the
  "legacy schema" fixture stays exactly one migration below the new one.
• goals_migrations/0009_thread_goal_blocker_audits.sql re-verified: main still tops out at
  0008, so 0009 is free and stays as-is.

Proof on the merged tree, not just the branch — git merge-tree --write-tree origin/main HEAD,
then listing every migration directory in the resulting tree:

migrations:        no duplicates   (… 0060, 0061_background_agent_lifecycle_receipts, 0062_thread_schedule_run_goal_id)
goals_migrations:  no duplicates   (… 0008_thread_goal_deferred, 0009_thread_goal_blocker_audits)
logs_migrations:   no duplicates
memory_migrations: no duplicates

Also checked every other open PR in this repo: none touches state/migrations or
state/goals_migrations, so 0062 / goals 0009 are uniquely claimed.

3. Build gate re-run on the new head. The earlier green run 30119889797 was for the
pre-rebase sha and no longer counts.

• Blacksmith testbox run 30145008904 — conclusion: success, sha ddcdf33b6, blacksmith-16vcpu-ubuntu-2404, 16 min.
• Command: cargo fmt -- --config imports_granularity=Item --check && cargo clippy --tests --locked -- -D warnings -A clippy::byte_char_slices -A clippy::while_let_loop && cargo nextest run --locked -p codex-state -p codex-goal-extension -p codex-app-server --no-fail-fast -E 'not test(background_agent_live)'
• Result: 1696 tests run, 1696 passed, 22 skipped; fmt and clippy (-D warnings) clean.

The previously accepted work is untouched: the schedules.rs fix for the
thread_schedules_ignore_legacy_live_owner_claim trigger (lease columns written only on
non-active transitions, fetch_optional claim, and its regression test) is preserved.

main has since advanced to b71a05ee9 (#393, #406) — both are TUI-only and share zero
files with this diff, so the green run above still characterises the merge result.