docs: note restart_phase auto-salvage and slice worktrees [doc-updater] - #2544
Conversation
Update agent-recovery, post-agent-commit, and orchestrator-cli docs to reflect that restart_phase now also calls auto_salvage_pipeline before deleting worktrees (#2526), and that it enumerates worktrees from disk rather than guessing names so slice-scoped worktrees are included. Authored-by: egg
There was a problem hiding this comment.
Comment — docs-only PR, accurately reflects #2526.
I verified each claim against orchestrator/routes/pipelines.py:3262-3332 and orchestrator/agent_salvage.py:
- ✅
restart_phasecallsagent_salvage.auto_salvage_pipeline(...)before the deletion loop, mirroringcleanup_pipeline's #2429 invariant. Best-effort: salvage exceptions are caught and logged; deletion still runs. - ✅ Worktree enumeration is
enumerate_agent_worktrees(pipeline_id, validate_git=False)— disk-scanned, so slice-scoped ({pipeline_id}-slice-{N}-{role}) worktrees are included, and broken-mount worktrees aren't silently skipped. - ✅
worktrees_to_deleteis filtered to roles in the restarted phase, leaving the pipeline-level worktree (agent_role=None) and other-role worktrees alone.
Non-blocking suggestion
docs/reference/agent-recovery.md line 262 — the auto-salvage row now reads:
Best-effort, automatic — runs from
kubernetes_spawner.cleanup_pipelineandrestart_phasebefore worktree deletion. Skipped whenpreserve_worktrees=True(the worktree survives, no need to mirror it). Failures are logged and never block cleanup or restart
The preserve_worktrees=True skip is a property of cleanup_pipeline only — that flag is defined in kubernetes_spawner.cleanup_pipeline (kubernetes_spawner.py:947) and there is no analogous knob in restart_phase, which always salvages and always deletes worktrees for the targeted roles. As written, a reader could infer that restart_phase honors a preserve_worktrees opt-out, which it doesn't.
Suggested wording (split the salvage skip-condition so it stays attached to its real source):
Best-effort, automatic — runs from
kubernetes_spawner.cleanup_pipeline(skipped whenpreserve_worktrees=True, since the worktree survives and there's nothing to mirror) and fromrestart_phase(always runs against the worktrees of the roles being restarted). Failures are logged and never block cleanup or restart.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback: the previous wording could mislead readers into thinking restart_phase honors a preserve_worktrees opt-out, but that flag only exists on kubernetes_spawner.cleanup_pipeline. restart_phase always salvages and always deletes worktrees for the targeted roles. Authored-by: egg
Review feedback dispositionegg-reviewer (574deb8) — non-blocking suggestion on
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approve — non-blocking suggestion adopted verbatim, no new issues.
The delta since 574deb8c is a single one-line wording change in docs/reference/agent-recovery.md line 262, adopting the suggested rewrite from the prior review. Re-verified the technical claim against source:
preserve_worktrees=Trueis exclusive tokubernetes_spawner.cleanup_pipeline(orchestrator/kubernetes_spawner.py:947) — when set,cleanup_pipelinereturns early at line 1003 (skipping both salvage and worktree deletion).restart_phase(orchestrator/routes/pipelines.py:3035) has no analogous flag; the salvage call atpipelines.py:3299runs unconditionally wheneverworktrees_to_deleteis non-empty, and worktree deletion atpipelines.py:3313follows.
The new wording correctly attributes the skip-condition to cleanup_pipeline and notes that restart_phase always runs against the worktrees of the roles being restarted, removing the misleading implication of an opt-out that doesn't exist.
I also spot-checked the two other doc files this PR touches for the same misleading wording: docs/reference/orchestrator-cli.md (slice-aware enumeration / salvage description) and docs/reference/post-agent-commit.md (auto-salvage trigger). Neither carries a preserve_worktrees claim that would need parallel correction.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
Update documentation to reflect changes from #2526:
restart_phasenow callsauto_salvage_pipelinebefore deleting worktrees (same ascleanup_pipeline), preventing silent loss of committed-but-unpushed work during phase restarts{pipeline_id}-{role}names, so slice-scoped worktrees ({pipeline_id}-slice-{N}-{role}) are correctly includedUpdated docs:
docs/reference/post-agent-commit.md— note restart_phase in auto-salvage description and migration tabledocs/reference/agent-recovery.md— update "Auto-salvage" trigger row to include restart_phasedocs/reference/orchestrator-cli.md— add salvage + slice-aware enumeration note to phase restart descriptionTriggered by: #2526
Authored-by: egg