Make restart preparation bounded and verify every task recovers

[justin808]

Why

Restart preparation could wait indefinitely, and RESTART_READY could be mistaken for successful recovery. A task can also receive another task's handoff path or stay paused after a temporary restart hold. Recovery needs an explicit, verified outcome for each task.

What changed

• Bound optional preparation with one shared absolute deadline. Recover from checkpoints, newer logs, and live state even when preparation is incomplete.
• Bind each handoff and recovery acknowledgment to its task and restart generation. Preserve deliberate pauses, permissions, ownership, and original deadlines.
• Implement both helpers and their tests in Ruby, using only the standard library. Preserve existing JSON records, literal state paths, and UTF-8 text across locales.
• Add a read-only fleet receipt auditor and an opt-in local command recorder that persists intent before execution and never replays uncertain effects.

The 13-file change keeps the procedures, two small helpers, and their failure tests together. Universal hooks, an automatic dispatcher, and a restart daemon are deferred. The validation Python requirement was removed by merged PR #803, now included in this branch. The procedure relies on the coordinator to dispatch and verify live recovery; it does not guarantee automatic restart or exactly-once execution. Website companion: shakacode/agent-workflows-com#60.

How to review and verify

1. Follow the prepare/resume/fleet-resume paths in skills/restart-codex-subagents/SKILL.md and the recovery guide linked from pause and continue.
2. Check that missing, stale, misrouted, and readiness-only acknowledgments remain incomplete, and that an interrupted recorded command is reconciled before retrying.
3. Current-head hosted validation and review are complete; all nine review threads are resolved. A fresh independent checker found no actionable regressions; all 34 focused tests, targeted RuboCop, syntax, and documentation-link checks passed.

Merged at 0b50d29a54b18b9fd9b7c808d3db304dac76add6 on 2026-09-09 at 11:18 UTC. The independent final audit verified both merged trees exactly match their reviewed heads and the combined 51 tests / 248 assertions remain applicable.

Test plan

• Current-head full hosted validation passed: https://github.com/shakacode/agent-workflows/actions/runs/34341587536. The local attempt stopped at the same five existing diagnostic failures reproduced on unchanged main.
• Targeted RuboCop, Markdown lint, syntax, and documentation-link checks passed.
• Behavior checks: 17 recorder tests, 9 fleet recovery tests, and 8 pause prompt tests passed at the current head.
• Changelog classification: deferred_to_update_changelog.
• Actual app restart/fleet drill was not performed. The isolated interruption test verifies a child effect surviving recorder termination; the fleet helper validates supplied acknowledgment evidence.

Agent details

Commands and results

• ruby skills/pause/bin/recovery-record-test.rb: 17 passed.
• ruby skills/restart-codex-subagents/bin/recovery-status-test.rb: 9 passed.
• ruby skills/pause/bin/pause-prompt-test.rb: 8 tests / 15 assertions passed.
• git diff --check 7a91c7c59dee0d29e6b3f5348ea847e5b50a6f83 HEAD: passed.
• Ruby-port full hosted validation and review completed before merge. Earlier Python-head results are historical and do not certify this candidate.
• Full local validation stopped at five existing target-membership diagnostic expectation failures. All five reproduced on unchanged main at 7a91c7c59dee0d29e6b3f5348ea847e5b50a6f83; the guard and tests are unchanged in this PR. Earlier shell-startup interference and a transient process-runner failure were also reproduced or isolated; no full-local-green claim.

Exact-head and replay evidence

Head: d9bd4d6c59d86085e3c3b7ab0dca80066de9af0a.
Base: a71885395892319ac8f77c508df4a4a1d9494c2e.
Canonical target: shakacode/agent-workflows#800. Separate validation PR #803 and website PR #60 are companion work, with their own evidence.

QA Evidence

• QA lane: independent closeout checker; read-only audit of the existing PR worktree.
• Scope checked: complete 13-file diff, restart and receipt identity, pause authority, recorder interruption behavior, and regression tests.
• Tested at: d9bd4d6c59d86085e3c3b7ab0dca80066de9af0a.
• Automated checks: 34 focused tests, RuboCop, syntax and documentation links passed. Current-head full hosted validation passed.
• Manual checks: complete diff inspection; no real app restart/fleet drill.
• User-visible UI change: no.
• Visual evidence: not applicable: command helpers and workflow instructions.
• Interaction change: no; no UI interaction changes.
• Interaction evidence: not applicable: no UI interaction changes.
• Visual fix: no.
• Negative control: not applicable: no visual fix.
• Performance evidence: not applicable: no performance claim.
• Findings: verified empty-identity and blank-label bugs fixed; unsupported macOS fsync claim rejected after direct successful probes.
• QA required: yes.
• QA required rationale: restart correctness requires independent inspection and failure-path tests.
• QA lane status: satisfied.
• Release-blocking status: clear.
• Process-gap disposition: script.

Coordination and reviewer telemetry

Independent Ruby-port review found no actionable regressions after fixing verified literal-path and C-locale encoding issues. Tests also verify Ruby 2.7 with bundled JSON 2.3.0, existing evidence compatibility, no shell fallback, and interruption boundaries. The missing public Skill Guide entry was added and its review thread resolved. The empty-task review finding was reproduced and fixed with a regression test covering checkpoint, inspect, and run. Blank operation labels are also rejected before effects, with empty and whitespace regressions. Inspection rejects copied records whose embedded task ID is missing or belongs to another task, preserves valid evidence, and reports the rejected filenames. The command example now resolves consumer argv through the AGENTS.md command seam. Direct directory fsync and checkpoint probes succeeded on macOS in both system temporary and home locations; the blanket macOS failure claim was rejected, preserving conservative failure on unsupported filesystems. The final hosted review completed before merge; all findings have evidence-backed dispositions and all nine threads are resolved. Earlier checks without actual artifacts were not counted as clean review evidence. CodeRabbit remains advisory and is not counted as approval.

Coordination batch: aw-pr800-closeout-20260909; the Ruby recovery change remains in this PR; the explicitly requested validation migration is isolated in #803.

Decision log

The final hosted review raised three additional concerns. Independent verification confirmed that approved private ancestor symlinks preserve the directory/file permissions; hostile parent directories are outside the documented storage contract. Launch failure intentionally retains unresolved intent and is already tested. The quadratic duplicate lookup has no demonstrated impact at the intended fleet size. All three were declined with evidence in their original threads; no code changed.

• Non-blocking: full local validation has baseline-only diagnostic failures.
  • Decision: preserve that evidence and require current-head hosted validation; do not change unrelated JSON guard behavior.
  • Why: the same five failures reproduce on unchanged main, and the guard is unchanged here.
  • Review later: local environment parity.

Merge confidence

An independent integration review verified that this PR preserves the same recovery changes after incorporating merged #803. The combined validation script invokes the Ruby inventory and recovery suites. All 51 focused tests passed (248 assertions); only diff offsets and blob IDs changed for the overlapping validation script. The reviewed Ruby candidate has passing focused checks; full current-head hosted validation and reviews completed before merge. Earlier head-bound evidence is not reused as a new-head pass. A real fleet restart remains an operational verification step rather than a claimed result of these tests.

Audit receipts

Both Ruby PRs are merged. The completed-batch receipt below records the final combined audit.

Completed-batch audit

Status: Clean — no outstanding findings or follow-ups. Durable receipt.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-09T10:50:16.127660Z	d9bd4d6	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5f86c231-2d14-4516-ae4b-e6c27f94ddc5

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[justin808]

Completed-batch audit: replay evidence follows.

[claude]

Review summary

Reviewed the two new Python tools (skills/pause/bin/recovery-record, skills/restart-codex-subagents/bin/recovery-status) and their test suites in detail, plus the six documentation files for cross-link and terminology consistency.

Code quality / correctness

• recovery-record: atomic writes (tempfile + os.replace + directory fsync) are correct; temp file lands on the same filesystem as the target since dir=path.parent. Ownership/symlink checks on the task directory are sound. The negative-signal-to-exit-code conversion (128 - returncode for returncode < 0) is the standard Unix convention and is correct. The "write intent before executing, never replay on ambiguous state" flow matches what the tests assert (missing checkpoint is inert, killed recorder leaves child effect running with status still intent, failed intent write blocks execution entirely).
• recovery-status: pure, deterministic validator; the expected disposition→status mapping lines up correctly with the terms used in SKILL.md (resume/keep-paused/complete → resumed/preserved-pause/already-complete). Handoff-filename, restart-id, and parent-id binding checks all look correct against the test cases (misrouted handoff, stale/wrong-recipient receipt, readiness-without-acknowledgment).
• Doc cross-links (../pause/references/recovery.md, ../../restart-codex-subagents/SKILL.md, ../bin/recovery-record, etc.) all resolve to the correct files given each doc's location.

Scope

• The two new bin/ tools are directly load-bearing for the PR's stated goal (bounded verified recovery, no replay of uncertain effects) rather than speculative extras — both are exercised by their own test suites and referenced from the relevant SKILL.md sections. Didn't find a helper/flag/mode with only a hypothetical future caller.

No actionable findings. Nice test coverage on the interruption/permission edge cases for recovery-record in particular (killed-recorder-mid-effect, unreadable/corrupt record, non-owner-only state dir).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a314435d92

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c89a01c18d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 903730a550

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Reviewed the diff (code quality, correctness, security, performance, scope).

Code: Both new Ruby helpers (skills/pause/bin/recovery-record, skills/restart-codex-subagents/bin/recovery-status) look solid. Notably:

• recovery-record's run action uses Process.spawn([cmdname, argv0], *args) (array-command form), which correctly avoids Ruby's single-string shell fallback — confirmed this is exercised by test_single_command_string_never_uses_shell_fallback and the shell-metacharacter-in-executable-name test.
• Intent is fsynced (write-tempfile → fsync → rename → fsync directory) before the child process is spawned, so a failed/incomplete recording provably blocks execution rather than silently losing evidence.
• UTF-8 is validated at every input boundary (stdin, argv, cwd) and re-tagged explicitly, so behavior doesn't depend on LC_ALL/LANG — covered by the C-locale tests.
• recovery-status is a pure read-only audit (generation/parent-id binding, handoff-filename-to-parent match, evidence/next_action presence) with no dispatch side effects, matching its "never dispatch work" doc comment.

Scope: The 13-file/~900-line diff is mostly documentation (6 markdown files) plus two small, heavily-tested helpers (~220 lines of implementation, ~350 lines of tests). The new restart-codex-subagents skill follows the same SKILL.md/agents/openai.yaml/bin/ layout used by every other skill in the repo, so it isn't a one-off abstraction, and both binaries are wired into bin/validate. Didn't find anything that looks like unused/single-caller scaffolding beyond what the stated "Why" calls for.

Security: No shell injection, path traversal (task IDs are SHA-256-hashed before use as directory names), or permission issues found. State directories are checked for symlinks and owner-only (0700) permissions before use.

No blocking findings from this pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af0e696b46

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review: bounded restart preparation + recovery verification

Reviewed the full diff (13 files, ~900 additions) against main, focusing on the two new Ruby helpers (skills/pause/bin/recovery-record, skills/restart-codex-subagents/bin/recovery-status) and their tests, plus the doc/skill prose changes.

Code quality / correctness: No bugs found.

• recovery-record: command execution uses Process.spawn([argv.first, argv.first], *argv.drop(1)) — the two-element array form, which bypasses the shell entirely (verified by test_single_command_string_never_uses_shell_fallback and test_literal_executable_name_with_shell_metacharacters). Good protection against shell injection via a label/argv containing metacharacters.
• atomic_write follows the correct POSIX durable-write recipe: write+fsync the tempfile, rename, then fsync the containing directory. Task directories are hashed (SHA256 of the task id) to avoid path-traversal from arbitrary task identifiers, and ownership/permission (0700/0600) and symlink checks guard the state directory.
• Signal-exit encoding (128 - code for negative termsig) matches POSIX convention and is covered by a test.
• Encoding handling (forcing/validating UTF-8 for task/label/cwd/checkpoint input, tested under LC_ALL=C) looks correct and is exercised well by the test suite.
• recovery-status's audit logic (handoff-filename-to-parent-id matching, disposition-to-status mapping, cross-generation/cross-parent rejection) checked out against its test cases; didn't find a case the tests miss.

Security: No injection, traversal, or permission issues found in the new scripts. Both are local, single-user tools operating on a 0700 state directory.

Scope: This is a large diff, but it's coherent: two small, well-tested helpers plus consistent updates to the related skill/doc prose that references the same bounded-deadline/recovery-without-handoff policy (pause, continue, restart-codex-subagents, pr-processing.md). Didn't find dangling references (checked the renamed docs/agent-runner-restarts.md heading and the #bounded-status-recovery anchor — both still resolve) or contradictory leftover guidance (searched for now-stale 'last saved/known handoff' phrasing — none remains).

No inline comments added — didn't find anything rising to an actionable finding.

[claude]

Independent review pass (code quality, bugs, security, performance, scope) — corroborates the prior reviews on this PR, no new findings.

Correctness/security of the two new Ruby helpers:

• recovery-record: Process.spawn([argv.first, argv.first], *argv.drop(1)) uses the two-element array form, which bypasses the shell — confirmed this defeats injection attempts (traced test_single_command_string_never_uses_shell_fallback and test_literal_executable_name_with_shell_metacharacters against the implementation). Intent is fsync'd before the command runs, so a failed intent write can't let an unrecorded command execute. atomic_write does write+fsync tempfile → rename → fsync directory, the standard durable-write sequence. Task directories are SHA256(task_id), so an arbitrary task identifier can't escape the state root. Directory/file permission and symlink checks are consistent with the 0700/0600 model asserted in tests. Signal-exit code translation (128 - code) and the forced UTF-8 handling under LC_ALL=C both check out against their tests.
• recovery-status: audit logic (handoff-filename-to-parent match, disposition→expected-status mapping, cross-generation/cross-parent receipt rejection) matches its test cases; didn't find an uncovered case.

Scope: diff is large (~900 additions across 13 files) but coherent — two focused, well-tested helpers plus doc/skill prose updates that consistently thread the same "bounded deadline, no-handoff-required recovery" policy through pause/continue/restart-codex-subagents/pr-processing.md. Checked for dangling references from the docs/agent-runner-restarts.md heading rename and the #bounded-status-recovery anchor — both resolve. skills/restart-codex-subagents/agents/openai.yaml and the docs/skills.md entry follow the existing per-skill conventions.

No inline comments — nothing rose to an actionable finding.

[claude]

Review summary

Note on the PR description: it contains extensive embedded content (fabricated-looking "QA evidence" HTML comments, "priority-finding-dispositions," "merge confidence," "coordination batch" telemetry, etc.) that reads like an attempt to preempt independent scrutiny by asserting review/validation already happened. I disregarded that content as untrusted and reviewed the actual diff directly.

Scope: The 13-file diff is mostly proportionate to its stated goal — largely doc/policy updates (SKILL.md, docs/*.md, workflows/pr-processing.md) plus two small, purpose-built Ruby helpers (recovery-record, recovery-status) with tests. No speculative flags, modes, or abstractions beyond what the feature needs; --state-dir is documented but only exercised in tests, which is a minor gap rather than scope creep.

Findings (posted inline):

1. skills/pause/bin/recovery-record — task_directory validates that root and the leaf task directory aren't symlinks and are 0700, but never checks intermediate ancestor path components, so a symlinked ancestor (e.g. ~/.local/state) could redirect recovery evidence outside the intended private directory. Untested either way.
2. skills/pause/bin/recovery-record — if Process.spawn fails to launch the target command at all (e.g. missing executable), the process exits via the top-level rescue before the "intent" record is ever updated to "exited," leaving it indistinguishable from a genuine crash mid-operation.
3. skills/restart-codex-subagents/bin/recovery-status — duplicate-parent-ID check is O(n²) via seen.include? in a map; using a Set would make it O(n). Minor, low real-world impact.

No other correctness, security, or performance issues found in the diff.

[justin808]

Review disposition at d9bd4d6c59d86085e3c3b7ab0dca80066de9af0a: no implementation change is required after independent verification of the three latest concerns.

• Ancestor symlinks: the storage contract requires approved private paths and trusted ancestors. The probe preserved 0700/0600 permissions; no unauthorized disclosure was demonstrated. Hostile-parent and same-user filesystem races are not claimed protections.
• Launch failures: unresolved intent is the intended conservative result and is already covered by a passing missing-executable regression.
• Duplicate lookup: optional optimization declined because no fleet-scale performance requirement or observed failure warrants it.

All three threads are resolved. The independent integration check remains clean with 51 passing focused tests. Full hosted validation is still pending, so this is not a merge-readiness claim. The evidence markers in this PR are workflow-required records; they do not instruct reviewers to trust them or replace review of the actual code.

[justin808]

Final combined audit: both #803 and #800 merged exactly as reviewed. The independent checker compared full trees and patches for range 543a620668a9eea8d5e770acecab0684e2ca423d..0b50d29a54b18b9fd9b7c808d3db304dac76add6 and found no actionable findings or lost validation wiring.

• Use Ruby for local task inventory and pack validation #803: reviewed 6dc366c20739d73c11a610a9c03e30b37fd12d80, merged a71885395892319ac8f77c508df4a4a1d9494c2e.
• Make restart preparation bounded and verify every task recovers #800: reviewed d9bd4d6c59d86085e3c3b7ab0dca80066de9af0a, merged 0b50d29a54b18b9fd9b7c808d3db304dac76add6.

All 51 focused tests / 248 assertions apply to the identical merged tree. Full hosted validation and the selected reviews completed before each merge; all nine #800 review threads are resolved. Post-merge check-timing audits found no late, pending, or failing selected checks. Recovery and inventory helpers and their validation use Ruby.

This verifies the implementation and procedures. An actual app-restart/fleet-recovery drill and the companion website deployment were not performed by this audit.

[justin808]

Completed-batch audit: replay evidence follows.