feat(work-graph): lifecycle bindings + stale truth

[Hmbown]

Summary

• bind durable tasks, workers, workflows, Fleet tasks, and Lanes to authoritative owner lifecycle snapshots while keeping ephemeral shells restart-stale
• register operation intent before execution, retain bounded pre-truncation byte receipts, record typed cancellation outcomes, and keep verification approval as provenance rather than delegated authority
• reconcile restored owners atomically against the target workspace and persist workflow/Fleet/Lane lifecycle high-water marks without inventing liveness
• re-anchor active operation identity after compaction while replacing stale prior re-anchors instead of accumulating duplicates
• fail closed across task queue-write rollback, missing workflow controllers, workflow journal recovery, and owner sequence regression

Verification

• cargo fmt --all -- --check
• strict locked all-workspace/all-feature clippy with the repository CI allowances
• cargo test -p codewhale-tui --bin codewhale-tui --locked (7,480 passed, 3 ignored)
• cargo test -p codewhale-lane --locked (17 passed)
• cargo test -p codewhale-tui --test release_runtime_qa --locked (5 passed, 1 heavy benchmark ignored)
• cargo test -p codewhale-tui --test integration_mock_llm --locked (42 passed)
• gitleaks dir --no-banner --redact --exit-code 1 .
• gitleaks protect --staged --no-banner --redact --exit-code 1

Known local linker diagnostic: macOS reports the existing oversized __eh_frame compact-unwind warning for the TUI test binary; all commands exit successfully.

No tag, release, registry publication, deploy, or branch deletion is included.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @Hmbown's task in 1m 32s —— View job

---

Claude is reviewing this PR

Todo list

• Gather context (diff vs origin/main, read changed files)
• Review correctness (bugs, panics, races, error handling) — 3 parallel deep-dives running (work_graph core, tools/operation-intent, TUI surface + lane registry)
• Review provider/model/route safety invariant (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608)
• Review reuse/simplification opportunities
• Review test coverage
• Review security (secrets, shell/exec policy, input validation)
• Post final review summary

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 8m 23s —— View job

---

Review: lifecycle bindings + stale truth

Todo list

• Gather context (diff vs origin/main, read changed files)
• Review correctness (bugs, panics, races, error handling)
• Review provider/model/route safety invariant (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608)
• Review reuse/simplification opportunities
• Review test coverage
• Review security (secrets, shell/exec policy, input validation)
• Post final review summary

Reviewed via three parallel deep-dives (work_graph core, tools/operation-intent, TUI surface + lane registry), then verified the highest-severity findings by hand.

Correctness bugs

1. crates/tui/src/tools/shell.rs:2873 (confirmed, inline comment posted) — interactive exec_shell's lifecycle receipt hardcodes raw_bytes to 0 instead of the real stdout/stderr length (contrast with the background-shell path at line 708, which passes real bytes). Every interactive command's byte receipt reports 0 bytes, defeating the "bounded pre-truncation byte receipt" guarantee for that path.
2. crates/tui/src/tools/tasks.rs:274-281 (confirmed, inline comment posted) — TaskCancelTool derives the typed CancelOutcome from a prior snapshot fetched via an earlier, separate get_task call rather than from what cancel_task actually did. A task that finishes between the two calls can produce a wrong outcome (e.g. Requested for what was really a no-op).
3. crates/tui/src/tui/work_surface/model.rs:234-247 (confirmed) — graph_rows's running/waiting/ready/blocked bucket roll-up excludes NodeState::Initializing. Per-row rendering correctly tags an Initializing node as live (▸, WorkTone::Live), but the "Work · N running · M ready · K blocked" header can read "0 running" while a live row is visible — the inverse of the invented-liveness bug this PR otherwise guards against. GitHub wouldn't let me anchor an inline comment here (line falls outside the diff hunk).
4. crates/tui/src/tools/shell.rs:2869-2878 — if the post-completion lifecycle.observe() call errors, the already-computed real ShellResult is discarded and an error returned instead; the ShellSpawnIntentGuard's drop then records a possibly-incorrect Failed terminal state even though the shell command itself may have succeeded.
5. crates/tui/src/tools/tasks.rs:~141 — if reconcile_task_record fails after add_task_with_id already succeeded, the tool surfaces an error for an already-created task, risking a duplicate on client retry.
6. crates/tui/src/tools/shell.rs:706 — publish_lifecycle bumps lifecycle_seq before calling observe(); a failing observe() leaves the bump in place, causing phantom increments on retry (low impact — the ledger takes max()).

Everything else in the core lifecycle machinery checked out: owner-sequence regression handling, the stale-recovery-at-equal-seq case, the optimistic-concurrency check in reconcile_workspace_owner_bindings (work_graph/runtime.rs:957-990), the compaction re-anchor/strip logic (core/engine.rs), task_manager.rs's stage→queue-write→promote fail-closed path, cancel_workflow's missing-controller fail-closed-to-Stale path, and fleet/ledger.rs's high-water-mark logic are all correctly implemented and covered by tests.

Two minor edge cases, not confirmed bugs: reconcile_workspace_snapshot (runtime.rs:874) and external_identity_is_well_formed (model.rs:372) both split fleet externals on the first /, ambiguous if a run_id ever contains /.

Provider/model/route safety (EPIC #2608)

No issues — this PR doesn't touch model/provider selection or routing; none of the changed files parse model strings or infer provider from prefixes or prompt text.

Reuse/simplification

• TaskStatus → OwnerState mapping (plus the ended_at.or(started_at).unwrap_or(created_at) timestamp derivation) is now duplicated three times: tools/tasks.rs:314-325, core/engine.rs:3604-3623 (new), and tui/ui.rs:1855-1864 (new). The PR already centralizes similar owner-mapping helpers in work_graph/liveness.rs (fleet_task_owner_snapshot, lane_owner_snapshot); a task_owner_snapshot(&TaskRecord) helper belongs there too so a future TaskStatus variant can't silently drift out of sync in two of the three copies.
• crates/tui/src/tools/workflow.rs:176 — attach_bound_workflow_lifecycles re-reconciles every bound run on every tool call, including read-only status/list calls — an avoidable cost on hot paths.

Test coverage

• The PR's most novel, I/O-touching logic — workspace-owner reconciliation (reconcile_workspace_snapshot, restore_with_workspace_owner_bindings, reconcile_workspace_owner_bindings in work_graph/runtime.rs) and core/engine.rs's reconcile_restored_work_bindings (~90 lines, including OwnerMissing fallback branches) — has no direct test coverage; its only exercised path is production code in tui/app.rs.
• crates/tui/src/tui/ui/tests.rs's +1 line only adds a struct field for compilation — none of the ~59 new lines in ui.rs are exercised (no test calls refresh_active_task_panel or the new periodic checkpoint-flush logic).
• No test for exactly-at-boundary or non-UTF8-boundary byte-receipt truncation, and none for the interactive-shell discard-on-observe-failure path (bug Remove Normal mode and consolidate to Agent #4 above).
• Where tests were added — engine/tests.rs's reanchor test, app/tests.rs's Fleet-restore/sequence-regression tests, task_manager.rs's failed_queue_write_leaves_no_replayable_task_record, tasks.rs's cancellation_without_controller_fails_closed_as_stale — they're strong and specifically verify the atomicity/fail-closed claims in the PR description.

Security

Nothing to flag — no unsafe, no new shell/exec invocations, no secret handling changes.

---

Solid PR overall: the fail-closed behaviors described in the summary are genuinely implemented and well-tested where they matter most (queue-write rollback, missing controller, sequence regression). The gaps are concentrated in the newer, more peripheral wiring (interactive shell receipts, cancel-outcome derivation, UI aggregate counts, and the workspace-owner reconciliation test gap).

[claude]

Claude finished @Hmbown's task in 1m 33s —— View job

---

Re-review: latest push (4 new commits since the previous review)

Todo list

• Gather context (identify new commits since last review)
• Review the 4 new commits for correctness
• Verify previously flagged findings (raw_bytes=0, cancel outcome, Initializing rollup) are actually fixed
• Check for any new issues introduced
• Post final review summary

New commits: 75a7ba5, a8be54e, d3c13c3, 555117e. All four of the correctness findings from my previous review that had confirmed fixes available are addressed, and I re-verified each against the actual diff rather than trusting the commit message alone:

1. Interactive raw_bytes hardcoded to 0 (shell.rs:2873, previously confirmed) — Fixed in a8be54e. raw_bytes is now result.stdout_len.saturating_add(result.stderr_len), the real pre-truncation length, matching the background-shell path.
2. TaskCancelTool deriving outcome from a stale prior snapshot (tasks.rs:274-281, previously confirmed) — Fixed in a8be54e. TaskManager::cancel_task now returns a TaskCancellation { task, disposition } computed inside the same state-lock transaction as the mutation, so the disposition (Forced/Requested/AlreadyFinished) can no longer race a concurrent completion. New test cancel_finished_task_returns_atomic_already_finished_outcome covers the case that was previously unguarded. runtime_api.rs and ui.rs call sites were updated for the new return type.
3. graph_rows header excluding Initializing from the running count (work_surface/model.rs:234-247, previously confirmed) — Fixed in d3c13c3. The filter now matches NodeState::Initializing | NodeState::Active, with a new regression test asserting the header reads "2 running" for a mix of initializing/active/ready nodes.
4. Discarding a real ShellResult on observe() failure (shell.rs:2869-2878, previously noted) — Fixed in a8be54e. spawn_guard.disarm() now runs before observe() is attempted (so a graph-write failure can't relabel a successful command Failed), and an observe() error is downgraded to a lifecycle_warning field on the tool result instead of discarding the successful result.
5. Duplicate-task risk if reconcile_task_record fails after add_task_with_id (tasks.rs:~141, previously noted) — Fixed in a8be54e as a side effect: task_create's reconciliation error is now captured as lifecycle_warning rather than propagated as a hard Err, so a client retry won't see a spurious failure for an already-created task.
6. publish_lifecycle bumping lifecycle_seq before observe() (shell.rs:706, previously noted, low impact) — Fixed in a8be54e: next_seq is computed and only assigned to self.lifecycle_seq after observe() succeeds.

The remaining two commits are narrowly-scoped test fixes: 75a7ba5 initializes the new work_lifecycle/lifecycle_seq/last_lifecycle_* fields on the Windows-only BackgroundShell test fixture (keeps cross-platform compilation working), and 555117e adds a settle delay in release_runtime_qa.rs so a paste-burst heuristic window doesn't race the post-cancel liveness assertion — both are test-only and unremarkable.

No new correctness issues surfaced in this diff. The ShellSpawnContext struct introduced to bundle owner_agent/work_lifecycle params is a reasonable readability win (was a growing positional-arg list). TaskCancelDisposition is a clean way to make the outcome derivation impossible to get wrong going forward, and directly closes the class of bug the original finding was pointing at (deriving state from a second, non-atomic read).

Two items from my original review remain unaddressed but were not marked as blocking, and neither was touched by this push, so they still stand as open recommendations if the author wants to fold them in:

• No direct test coverage for reconcile_workspace_snapshot / restore_with_workspace_owner_bindings / reconcile_workspace_owner_bindings (work_graph/runtime.rs) or core/engine.rs's reconcile_restored_work_bindings — still only exercised indirectly via app.rs production code paths.
• TaskStatus → OwnerState mapping is still duplicated across tools/tasks.rs, core/engine.rs, and tui/ui.rs — a shared task_owner_snapshot(&TaskRecord) helper in work_graph/liveness.rs (alongside the existing fleet_task_owner_snapshot/lane_owner_snapshot) would still remove the drift risk.

Note: I wasn't able to run cargo build/cargo test locally in this environment (sandboxed shell required approval that isn't available here), so this review is based on static reading of the diff. CI (CI, DCO, Web Frontend) was in progress/passing at review time — worth confirming it's fully green before merge.

[Hmbown]

Exact-head repair receipt for 555117e: all six Claude correctness findings are addressed. Task cancellation disposition is atomic; successful task/shell outcomes survive Work reconciliation failures with explicit warnings; interactive byte receipts use ShellResult lengths; shell sequences advance only after successful observation; Initializing rows count as running; and the macOS six-worker PTY miss was classified as a rapid-key paste-window race and hardened. Local gates: cargo fmt check; diff check; strict locked workspace/all-target/all-feature clippy under Rust 1.97; locked TUI 7,482 passed, 0 failed, 3 ignored; release PTY 5 passed, 0 failed, 1 heavy benchmark ignored; the six-worker test also passed five consecutive pre-fix reproductions; delta and 1.05 GB full-tree gitleaks scans found no leaks. Both inline threads have fix-specific replies and are resolved. Awaiting exact-head CI and review; no publication action taken.

[claude]

Claude finished @Hmbown's task in 1m 13s —— View job

---

Re-review: commit 976f955 (1 new commit since exact-head repair receipt)

Todo list

• Gather context (identify new commit since last review)
• Review new commit 976f955 (owner-snapshot centralization)
• Verify Hunter's repair-receipt claims against actual diff at HEAD
• Check for any regressions/new issues
• Post final review summary

Hunter's 10:23 repair receipt covered commit 555117e and claimed all six correctness findings from my original review were fixed. I re-verified each against that diff in my previous update (10:21) and confirmed all six. Since then, one more commit landed: 976f955 ("refactor(runtime): centralize task owner snapshots").

976f955 review

This directly closes the one reuse/simplification item I flagged as still open (the TaskStatus → OwnerState mapping duplicated three ways). It adds task_owner_snapshot() to work_graph/liveness.rs:91-112 — a single exhaustive match over all five TaskStatus variants plus the ended_at.or(started_at).unwrap_or(created_at) timestamp precedence — and switches all three former call sites to use it:

• core/engine.rs:3600 (reconcile_restored_work_bindings)
• tools/tasks.rs:313-330 (reconcile_task_record)
• tui/ui.rs:1855-1865 (refresh_active_task_panel)

Verified by hand, not just by the commit message:

• The helper builds the task:{id} external id internally, matching the format each call site independently builds for its own lookup/dedup logic (external = format!("task:{}", task.id) in engine.rs and tasks.rs) — no id-format drift.
• Import cleanup is correct: TaskStatus is fully removed from tools/tasks.rs (no longer referenced there) but still imported and used in tui/ui.rs:1831 for an unrelated liveness check, so nothing is left dangling either way.
• OperationOwnerSnapshot/OwnerState remain imported and used in tools/tasks.rs for the pre-creation-failure path (tasks.rs:132-141, when add_task_with_id itself fails and there's no real TaskRecord to translate) — correctly left as a direct construction rather than forced through the new helper.
• Two new unit tests (task_owner_snapshot_exhaustively_maps_status_and_prefers_terminal_time, task_owner_snapshot_falls_back_from_started_to_created_time) cover all five status variants and both branches of the timestamp fallback.
• mod.rs export updated (pub use liveness::{..., task_owner_snapshot}), so the previously-scattered logic now has exactly one source of truth as intended.

No new issues introduced by this commit. This is a clean, narrowly-scoped follow-up.

Overall status

Both items outstanding from my previous reviews are now resolved:

• ✅ All six correctness findings (interactive raw_bytes=0, stale cancel-outcome derivation, Initializing excluded from the running rollup, discard-on-observe-failure, duplicate-task risk on reconciliation failure, premature lifecycle_seq bump) — fixed in a8be54e/d3c13c3, re-verified against the diff.
• ✅ TaskStatus → OwnerState duplication — fixed in 976f955.

The one remaining open recommendation from my first review — no direct test coverage for reconcile_workspace_snapshot / restore_with_workspace_owner_bindings / reconcile_workspace_owner_bindings in work_graph/runtime.rs and core/engine.rs's reconcile_restored_work_bindings (still only exercised indirectly via app.rs production paths) — was never marked blocking and remains untouched by this push. Worth folding in before/after merge at the author's discretion, but doesn't affect the correctness of what's here now.

Note: same as previous reviews, I wasn't able to run cargo build/cargo test/clippy locally in this environment (sandboxed shell requires approval that isn't available here), so this is a static read of the diff, cross-checked against the actual call sites and imports rather than the commit message alone.

[Hmbown]

Follow-up exact head 976f955 closes the remaining lifecycle-drift recommendation from review: model tools, periodic TUI refresh, and engine restore now share one exhaustive TaskStatus/sequence/timestamp owner adapter. Added tests cover all five statuses plus terminal-to-started-to-created timestamp precedence. Existing App tests already exercise target-workspace Fleet restore and prove sequence-regression failure leaves prior Work state intact, so no redundant restore test was added. Refreshed local gates: strict locked workspace/all-target/all-feature clippy; locked TUI 7,484 passed, 0 failed, 3 ignored; release PTY 5 passed, 0 failed, 1 ignored; fmt/diff and one-commit gitleaks clean. Awaiting fresh exact-head CI/review; no publication action taken.