feat(automation): archive terminal runs on delete

[asto18089]

Summary

Deleting an automation used to drop its run history outright: delete_automation removed the live runs directory, so a user deleting a task lost every record of what it executed. This keeps terminal run history in a bounded archive instead.

Pinvou (the embedder this fork serves) ships a richer app-side capability (ArchivedScheduledTaskSnapshot with display metadata and deleted_at). This engine archive is the foundation layer that preserves the run records themselves; the app-side delegation is tracked as follow-up work in Pinvou/pinvou-agent#653 (its Chop D is deliberately not chopped there), since the app contract additionally needs deletion timestamps, display metadata, and an uncapped-history mode that this PR does not attempt.

Archive layout and retention

• Terminal runs (completed, failed, canceled — the AutomationRunStatus vocabulary) are moved to <automation_root>/archive/<automation_id>/<created_stamp>-<run_id>.json before the live paths are removed; legacy {run_id}.json live files are normalized to the sortable name. Each record is written with the existing write_json_atomic helper (rename-atomic, not fsynced — process-crash safe, not power-loss durable); no schema changes to persisted records.
• The most recent 50 terminal runs per automation are kept, ordered by ended_at falling back to created_at (the same recency rule as terminal_run_prune_candidates); only that slice is written, and a retention sweep still bounds any leftovers from an interrupted earlier attempt. Per automation is the only bound: the archive root keeps one capped directory per deleted automation and is never compacted.
• The archive tree sits beside — never inside — automations/, runs/, and triggers/, so list_runs, get_runs_by_ids, terminal_run_prune_candidates, and the scheduler scan (collect_due_runs → list_automations) cannot see it. A deleted automation cannot come back due from its archive.
• Deleting is refused while any persisted run is still queued or running, mirroring the existing delete_terminal_run refusal. Scope note: a run whose enqueue is still awaiting the task manager is not yet on disk, so deletion in that in-flight window is not refused; the record is persisted afterward outside the archive — pre-existing behavior, pinned by finish_scheduled_run_persists_run_when_automation_deleted_mid_enqueue. Upstream enforces this case with its transaction/claim machinery, which the fork base does not have; closing it here is follow-up work, not part of this PR.
• The CodeWhale TUI delete receipt and preview still say the recorded runs were deleted (AutomationDeletedRunsDetail / AutomationDeletePreview, 15 locales); under this change those runs are archived (up to 50) rather than destroyed. Rewriting the copy needs a 15-locale translation pass and is follow-up work; upstream feat(automation): archive terminal runs on delete codewhale-hq/Codewhale#6864 carries the same copy.
• Known failure modes, consistent with run-file reads elsewhere in the module: a corrupt or newer-schema file in the live runs or the archive fails the delete until it is resolved externally, and a queued/running run whose task record is gone cannot be settled by reconciliation and must also be cleaned up externally.

API surface

• AutomationManager::list_archived_runs(automation_id) -> Result<Vec<AutomationRunRecord>> (anyhow, matching every sibling method) — retained archived runs, newest-first by ended_at/created_at.
• GET /v1/automations/{id}/runs?archived=true serves the archive (honoring limit), porting the upstream runtime-API surface (feat(automation): archive terminal runs on delete codewhale-hq/Codewhale#6864).

Verification

• cargo fmt --all -- --check — clean.
• cargo test -p codewhale-tui --lib — full suite green, including the runtime endpoint smoke workspace_and_automation_endpoints_work updated for the refusal contract: DELETE with an active run → 400 naming the reason with the automation and its run untouched; a run-free automation deletes and its archived=true listing round-trips a seeded record. Upstream feat(automation): archive terminal runs on delete codewhale-hq/Codewhale#6864 merged with that smoke test red; fixed here. The manager-level tests now pin both refusal arms (queued, running), the canceled filter arm, ended_at-over-created_at archive ordering, archived file naming with legacy normalization, and the scheduler's due-before/due-after behavior around the archive.
• cargo clippy -p codewhale-tui --lib — no warnings for this change.

No-Issue: engine foundation for the Pinnou scheduled-task archive; app-side delegation tracked in Pinvou/pinvou-agent#653 (Chop D).

[asto18089]

Stacked consumer: Pinvou/pinvou-agent#653 pins upstream/integration (= pinjou3-clean + these upstream contributions) and chops the app-side duplicates onto the new APIs. That parent PR is draft until this PR merges — its verify-public-submodule gate requires the parent gitlink to equal the public pinjou3-clean head. After merge, the pin re-points to the branch head and the chop lands.

[asto18089]

Fresh full review at head 6e1bc9324 (already based on the current pinjou3-clean tip 44921cf02 — no rebase needed; single commit, single file). Method: six parallel review lanes (upstream parity vs merged codewhale-hq#6864, concurrency/crash semantics, app-embedder integration, test mutation-strength, performance/edges, body-claim audit) plus local reproduction of every gate the body cites. Direction: changes requested — one blocker, two majors, no smuggling.

What is solid (verified, not taken on faith)

• The backport is faithful. 13 of 15 deltas vs upstream are provably forced by fork-base gaps (with_transaction, list_runs_with_visibility, execution_scope, receipts, fixture_due_automation all absent on 44921cf02). Core semantics, retention rule, and layout are ported essentially byte-for-byte.
• Smuggling: zero. One file, every hunk on-topic, tests map 1:1 to features, the one behavior change (delete refusal) is disclosed at manager level.
• Isolation claims verified for every named reader: list_runs, get_runs_by_ids, terminal_run_prune_candidates, and collect_due_runs → list_automations all read only automations//runs/; nothing in the engine (doctor, sweeps) or the app walks the automations root in a way the new archive/ sibling can perturb.
• "No schema changes" verified (identical struct + serializer on both write paths); "same recency rule as terminal_run_prune_candidates" verified (textually identical sort keys); the mirror of delete_terminal_run's refusal verified; the disclosure that base delete_automation dropped active runs is accurate.
• Crash windows (i)/(iii)/(iv) are cleanly retryable and idempotent (deterministic archive paths, atomic overwrite); no interleaving loses a persisted terminal record or resurrects a deleted automation from the archive.
• Body verification numbers all reproduce exactly at head: cargo fmt --all -- --check clean; cargo test -p codewhale-tui --lib automation_manager = 40 passed, 0 failed (count audited: exactly 40 test fns in the module); cargo clippy -p codewhale-tui --lib clean for this change (the red Lint lane is clippy-1.99 drift in files this PR doesn't touch; Version drift/Integrations/cargo-audit/cargo-deny redness is likewise pre-existing baseline).

Blocker

B1 — Deterministically breaks an existing test, undisclosed; full --lib suite is red. runtime_api::tests::workspace_and_automation_endpoints_work (crates/tui/src/runtime_api/tests.rs:1875) POSTs .../run — enqueue_run_task marks the run Running on successful enqueue (automation_manager.rs:1808) — then DELETEs the automation. The new refusal bails, map_automation_err maps it to 400, and error_for_status() aborts. Reproduced locally and red on all three OS lanes of this PR's own CI (14,557 tests: exactly 1 failed). Note upstream codewhale-hq#6864 merged with the identical failure (their CI shows the same single test red), so this is inherited — but the body's Verification section reports only the green filtered run and never mentions it. Fix: update the smoke test to the new contract — assert DELETE now returns 400 with the automation and its runs untouched, settle the run to a terminal status, then DELETE expecting 200 and GET expecting 404. Also add a Closes #… or No-Issue: … trailer so the required link gate stops failing (it is red for exactly that reason).

Major

M1 — The documented refusal guarantee is not enforced for in-flight enqueues. Both enqueue paths deliberately drop the manager mutex across the task-manager await and persist the run record only afterward (run_now_with phases 2–3, automation_manager.rs:1841–1870; scheduler_tick_shared, :1896–1903). A delete taking the lock during that window passes the refusal scan (nothing on disk yet), deletes the automation — and the run then persists anyway as a live orphan outside the archive, never reconciled (collect_pending_runs iterates list_automations() only). This exact outcome is pinned as deliberate base behavior by the pre-existing test finish_scheduled_run_persists_run_when_automation_deleted_mid_enqueue (:3420, present on base). Upstream actually enforces the documented semantics structurally: it saves the run record (Queued) inside with_transaction before the enqueue await, so its refusal scan sees in-flight work. The fork ports the doc comment ("Deleting is refused while any run is still queued or running … an in-flight task must not lose its run record") but not the enforcing structure. Minimum fix: scope the claim to persisted runs and disclose the in-flight window, in both the doc comment and the body bullet. (Persisting before the enqueue await is portable and matches upstream, but on its own it reintroduces a zombie-queued-run risk on a crash between the two saves, which upstream's receipt machinery settles — flag it as follow-up rather than half-porting it here.)

M2 — The API-surface story overstates what ships; the archive is write-only in this fork. list_archived_runs has zero production consumers: the upstream runtime-API archived query param was dropped wholesale (the fork base does have AutomationRunsQuery/list_automation_runs and could take the identical patch), the app never calls it, and parent PR codewhale-hq#653's own body says its scheduled-archive chop is explicitly "NOT chopped … delegation is tracked as follow-up work" — because the app's UI contract (ArchivedScheduledTaskSnapshot + deleted_at + display name/model + uncapped history, stores.rs:124–163) cannot be satisfied by a metadata-free, 50-capped run-record archive. Similarly, "The archive is bounded" is per-automation only: <root>/archive/ itself grows one capped directory per deleted automation forever with no GC anywhere (UUID ids are never reused), while the app's own sidecar compact()s deleted ids — the body should not say "bounded" without that qualifier. Fix: reword the body (engine-side first step; per-automation bound; delegation needs deleted-at/display metadata per codewhale-hq#653 Chop D), and either port the runtime-API archived param or state explicitly that it is deferred to the consumer PR. The author comment saying codewhale-hq#653 "chops the app-side duplicates onto the new APIs" needs the same qualifier for the scheduled-archive part.

Minor

• m1 — std::io::Result on list_archived_runs diverges from everything. Every sibling method returns anyhow Result, upstream's signature is anyhow Result, and the flattening via io::Error::other mislabels corrupt-JSON and ensure_safe_storage_id failures as IO errors while dropping the context chain. No consumer justifies it; the future runtime-API port would plug straight into map_automation_err(anyhow::Error) if it were anyhow. Return Result<Vec<AutomationRunRecord>>.
• m2 — two new permanent-failure modes. (a) One corrupt or schema-newer file in archive/<id>/ makes every subsequent delete_automation fail forever (retention re-reads with fail-hard read_run_file; consistent with list_runs' convention, but newly reachable from delete). (b) A zombie Queued/Running run whose task record is gone now blocks deletion permanently with no recovery path (reconciliation skips runs it can't resolve; base deleted regardless). Both low-likelihood; at least document them, ideally add a recovery story.
• m3 — comment accuracy. The crash comment's "is dropped with the live tree" is wrong for the window between automation-file removal and remove_dir_all (the orphan is permanent — no sweep exists, and a retry fails at get_automation), and write_json_atomic has no fsync, so "a crash mid-delete leaves the history intact" holds for process crashes, not power loss.
• m4 — test-strength gaps (mutation-checked). The Canceled arm of the terminal filter, the Running arm of the refusal, the ended_at-over-created_at ordering key, archived file naming (the "legacy-named runs are normalized" doc claim), and refusal-before-archive ordering are all mutable with zero test failures; the scheduler test's negative asserts are vacuous without upstream's due-before positive control. One mixed-fixture extension of delete_archives_terminal_runs_and_drops_live_paths (queued + canceled + completed/failed: assert refusal leaves everything untouched, settle the queued run to Canceled, then assert ended_at order, file name shape, and canceled presence) closes most of these.
• m5 — retention does avoidable work. It writes all T terminal files, then re-reads and re-parses everything it just wrote to delete the overflow. Sort terminal in memory and write only the newest 50; the delete path then needs no archive re-read at all.
• m6 — body wording. "Cannot pass against the prior implementation" is trivially true for three of the four tests (they don't compile on base — no archive API/constant); only the refusal test fails a genuine behavioral assertion.

Verdict

Useful, root-caused, faithfully adapted, smuggling-free — but not shippable as-is: the PR knowingly leaves the engine's own endpoint smoke test red without disclosing it (B1), claims a refusal guarantee its own base design can bypass in the exact window it exists for (M1), and presents a write-only, unbounded-at-the-root archive as a bounded API an embedder will delegate to (M2). B1 + the m1 signature change + the M1/M2 rewording are small, well-scoped fixes; happy to re-review on push.

[asto18089]

Fix wave pushed (4 commits, head 9770286c8) covering the round-1 findings — self-applied since the review is against my own PR.

B1 (blocker) — red endpoint smoke test: workspace_and_automation_endpoints_work is updated to the new refusal contract: DELETE with the active run now asserts 400 + "Refusing to delete" in the body, then verifies the automation and its single run are untouched. A run-free second automation deletes (200 → 404), and its archived=true listing round-trips a seeded archived record while the live listing stays empty. Upstream codewhale-hq#6864 merged with this exact test red; fixed here.

M1 (major) — refusal claim scope: the doc comment and body now say persisted runs block deletion and explicitly disclose the in-flight-enqueue window (the record persists afterward outside the archive — pre-existing behavior pinned by finish_scheduled_run_persists_run_when_automation_deleted_mid_enqueue). Closing the window itself needs upstream's transaction/claim machinery and is recorded as follow-up, not half-ported.

M2 (major) — write-only archive: GET /v1/automations/{id}/runs?archived=true now serves the archive with limit (the upstream runtime-API surface, dropped in the original backport without acknowledgment), and list_archived_runs returns anyhow Result like every sibling method (m1), so it plugs into map_automation_err directly. The body now describes the archive as the engine-side foundation layer, states the per-automation-only bound (the root is never compacted), and points at codewhale-hq#653 Chop D for the app-side delegation prerequisites. The earlier stack-comment claim that codewhale-hq#653 "chops the app-side duplicates" onto these APIs overstated Chop D — that PR deliberately does not chop the scheduled-archive read.

m3/m2 — comment accuracy and failure modes: the crash comment now states what the windows actually guarantee (rename-atomic writes survive a process crash; an automation removed before the live tree leaves it orphaned, not "dropped"), notes the absence of fsync, and the docs list the externally-resolved failure modes (corrupt/newer-schema file, task-less active run).

m5 — retention waste: the delete path sorts terminal runs and writes only the newest-50 slice; the retention sweep remains as the bound on leftovers from an interrupted earlier attempt.

m4 — test pins (mutation-checked): the refusal test loops over Queued and Running and asserts the active run survives untouched via list_runs; the archive test's fixture adds a canceled run created earliest but ended latest (pinning the canceled filter arm and ended_at-over-created_at ordering), a legacy-named live file (pinning normalization), and asserts the exact archived file-name set (created_at stamping); the scheduler test gains the due-before positive control (within the misfire grace) so the post-delete negative asserts are no longer vacuous.

Verification at head 9770286c8: cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --lib clean; cargo test -p codewhale-tui --lib automation_manager 40 passed / 0 failed; workspace_and_automation_endpoints_work green; full --lib suite green locally except a pre-existing, environment-specific stack-overflow abort in acp_server::tests::concurrent_sessions_each_get_their_own_tool_registry that reproduces identically at the pre-fix head 6e1bc9324 on this aarch64 machine (CI x86 runners are green there). PR body updated: refusal-scope and bound wording corrected, No-Issue: trailer added for the link gate.

[asto18089]

Fresh round-2 review at head 9770286c8 (base 44921cf02 = the current pinjou3-clean tip — no rebase needed; all 10 CI checks green on this head). Method: nine parallel review lanes — claim-by-claim audit against the code, upstream parity against codewhale-hq#6864, concurrency/crash semantics, manager line-by-line, HTTP surface + test audit, performance, hunk-by-hunk smuggling classification, mutation analysis (10 seeded mutations), elegance/repo conventions — with every contested finding re-verified against the tree before acceptance.

Verdict: real, complete within its stated scope, faithful to upstream, and bug-free as far as nine lanes plus mutation analysis could establish. Two documentation/test-completeness gaps are being fixed in a follow-up wave below; one copy decision is disclosed for input.

1. Real value and root cause — verified. The premise is real at base: delete_automation did remove_dir_all over the live runs with no archive. All three production delete callers (HTTP, agent tool, TUI) funnel through the single fixed method, and the fail-closed ordering (archive writes → sweep → unlink automation file → remove live tree) means no path added by this PR can lose a record: any failure aborts before a live path is touched, and deletes are retryable after the disclosed external fixes. All 20 in-tree body claims verified against code; 0 false, 0 overclaimed. Crash windows were audited explicitly: the last run is always in the archive before any live path is removed, so the "crash between live-dir removal and archive write" window cannot exist.

2. Upstream parity — faithful. The endpoint hunk is byte-identical to codewhale-hq#6864; retention (50), naming, refusal message, and archive helpers are identical. The two structural upstream mechanisms (transaction/claim serialization, suppressed-run visibility) do not exist in the fork base and are disclosed as deliberate follow-ups, not silent drops. The body's "upstream merged with that smoke test red" claim is verified against upstream CI logs: the ubuntu job's only failure is workspace_and_automation_endpoints_work hitting the new refusal's 400, and this PR's updated smoke is the fix. Fork-side test coverage is stronger than upstream in spots (both refusal arms, legacy-name normalization, mixed-status ordering — none of which upstream tests).

3. Smuggling — CLEAN. Exactly the 3 described files; each of the 5 commits contains only its claimed scope; hunk-by-hunk classification found zero non-feature changes; no weakened or deleted tests (all base assertions of the replaced test retained verbatim); no debug leftovers, secrets, or personal paths. CHANGELOG.md untouched — per CONTRIBUTING that is required, not a gap.

4. Bugs — none found. Concurrency: the single shared manager mutex serializes delete against every reader, the scheduler tick phases, and the tool callers; status matching is wildcard-free over the exhaustive 5-variant enum, so a queued/running run can never be silently archived; the archive is provably invisible to list_runs, get_runs_by_ids, terminal_run_prune_candidates, and collect_due_runs (pinned). Performance: delete goes from O(unlink) to O(live runs) read+parse — intrinsic to the refusal contract, since status lives only in file content — plus a constant ~150 archive ops; the "only the retained slice is written" claim is verified (≤50 writes); the lock hold becomes user-visible only at ≳5k unpruned runs under a single automation, a pre-existing growth property; per-tick scheduler and per-run persist costs are unchanged. Mutation analysis: 8/10 seeded mutations are caught by the existing suite; the 3 survivors are the coverage gaps below, not code defects.

Findings

• [MAJOR][docs] docs/RUNTIME_API.md documents this exact endpoint (GET /v1/automations/{id}/runs?limit=20) and the repo maintains per-parameter provenance there (archived_only=true … Added in v0.8.10 (#563)). The new archived parameter and the DELETE 400-on-active-runs behavior are undocumented. → fixing in the wave below.
• [MAJOR][tests] Three contracts the body/rustdoc explicitly claims are unpinned: (a) ?archived=true&limit=N — the handler's truncate branch has zero coverage; (b) the retention sweep's removal loop is never driven by any test (with take(50) writes an interrupted attempt cannot exceed the budget today, so the loop only fires after a retention-policy change or external writes); (c) the corrupt/newer-schema file fails the delete (fail-closed) is claimed but untested in both locations. → fixing with three pinning tests plus a smoke-test precondition assert.
• [MINOR][disclosed for decision] The TUI delete receipt/preview copy ("recorded runs deleted: {run_count}" / "To delete the definition and run history", all 15 locales) is now imprecise: terminal runs are archived (≤50, retrievable via archived=true), not destroyed, and the count overstates the archived set beyond 50. Upstream feat(automation): archive terminal runs on delete codewhale-hq/Codewhale#6864 carries the same copy, and the app-side delegation is tracked in refactor: adopt engine skill and safety primitives pinvou-agent#653, but this string lives in the CodeWhale TUI itself. Updating 15 locale strings deserves its own translation-quality pass, so I am disclosing it in the body as follow-up work rather than bundling it here — say the word if it should land in this PR instead.
• [NIT] Recorded for the follow-up list, no action now: crash-leftover .json.tmp files in the archive are never cleaned (invisible to readers, bounded per interrupted target); a failed or interrupted delete can leave partial duplicate archive history visible via archived=true for a still-live automation (idempotent on retry); archive_run_path keys on run.automation_id while the sweep keys on the delete target (mismatch reachable only via external tampering; upstream identical); the "retention sweep still bounds any leftovers from an interrupted earlier attempt" wording oversells — with capped writes that case cannot exceed the budget today, so the sweep is future-proofing for retention changes.

Considered and rejected (recording so they are not re-raised): moving limit truncation from the handler into list_archived_runs — the handler-side truncation is upstream's exact design, byte-identical here; dropping the post-refusal terminal filter as tautological — it is defense-in-depth and matches upstream; adding a CHANGELOG entry — CONTRIBUTING forbids changelog hunks in PRs.

Fix wave incoming: RUNTIME_API.md documentation + the three test pins + the smoke precondition assert, then the full gate set re-run.

[asto18089]

Fix wave pushed (2 commits, head cde8b6294) covering the round-2 findings:

• [MAJOR][docs] RUNTIME_API.md: the automations section now documents archived=true (the 50-run archive it reads, ended_at-over-created_at ordering, limit handling), the endpoint list line becomes GET /v1/automations/{id}/runs?limit=20&archived=false, and the DELETE 400-on-active-runs refusal is stated — in the same per-parameter provenance style the threads section uses for archived_only.
• [MAJOR][tests] three pins added:
  • delete_bounds_archive_leftovers_beyond_the_retention_limit drives the retention sweep's removal loop for real: 52 pre-seeded archived leftovers (an interrupted attempt) + 55 live terminal runs → the delete leaves exactly the newest 50, verified both in the listing and on disk, with every leftover swept.
  • delete_fails_closed_on_unreadable_run_files pins the corrupt-live-file arm and the newer-schema-archive-file arm: each fails the delete naming the offending file, leaving the automation and its runs untouched.
  • The endpoint smoke now covers ?archived=true&limit=1 (exactly the newest record) and limit=0 (empty), asserts the HTTP-level newest-first ordering with two seeded records, and asserts its own refusal precondition (the run really is active on disk before DELETE). The manager filename assertion hardcodes one stamp so the run_file_stamp format itself is pinned, not just the helper's own output.
• [MINOR][disclosure] TUI copy: the body now notes that the CodeWhale TUI delete receipt/preview still says "recorded runs deleted" (15 locales) — follow-up work, not bundled, per the round-2 review.

Gates at cde8b6294: cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --lib no warnings; 43/43 automation_manager tests + the endpoint smoke green locally. CI is authoritative for the full suite.