feat: define orchestrator cleanup ownership
This commit is contained in:
@@ -1438,6 +1438,28 @@ mod tests {
|
||||
assert!(resolved.manifest.feature.workspace_worker_discovery.enabled);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn builtin_orchestrator_keeps_cleanup_tool_providers_enabled() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let resolved = ProfileResolver::new()
|
||||
.with_workspace_base(tmp.path())
|
||||
.resolve(
|
||||
&ProfileSelector::source_named(ProfileRegistrySource::Builtin, "orchestrator"),
|
||||
ProfileResolveOptions::with_worker_name("orchestrator-worker"),
|
||||
)
|
||||
.unwrap();
|
||||
let feature = resolved.manifest.feature;
|
||||
|
||||
assert!(feature.worker.enabled);
|
||||
assert!(!feature.worker.direct_spawn);
|
||||
assert!(feature.manage_workdir.enabled);
|
||||
assert!(feature.merge_request.show);
|
||||
assert!(feature.merge_request.readiness_check);
|
||||
assert!(feature.merge_request.complete);
|
||||
assert!(!feature.merge_request.open);
|
||||
assert!(!feature.merge_request.review);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn profile_resolution_requires_runtime_worker_name() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
|
||||
@@ -920,4 +920,22 @@ mod tests {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn builtin_orchestrator_cleanup_policy_renders_with_common_includes() {
|
||||
let rendered = PromptCatalog::builtins_only()
|
||||
.unwrap()
|
||||
.render_name("role.orchestrator", Value::UNDEFINED)
|
||||
.unwrap();
|
||||
|
||||
assert!(rendered.contains("This policy governs naming only"));
|
||||
assert!(rendered.contains("Coder cleanup is a separate post-completion decision"));
|
||||
assert!(rendered.contains("perform one cleanup pass before ending the orchestration turn"));
|
||||
assert!(rendered.contains("Never predeclare `delete_on_completion`"));
|
||||
assert!(rendered.contains("call `WorkerStop`"));
|
||||
assert!(rendered.contains("call `WorkerRemove`"));
|
||||
assert!(rendered.contains("only then call `WorkdirDelete`"));
|
||||
assert!(rendered.contains("`CurrentAssignment` means unassign and reread"));
|
||||
assert!(!rendered.contains("{% include"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ use agen::llm_client::{ClientError, LlmClient, Request};
|
||||
use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput};
|
||||
use async_trait::async_trait;
|
||||
use futures::{Stream, StreamExt};
|
||||
use manifest::{ProfileRegistrySource, ProfileResolveOptions, ProfileResolver, ProfileSelector};
|
||||
use session_store::{CombinedStore, FsWorkerStore};
|
||||
use session_store::{FsStore, LogEntry};
|
||||
use workdir::{
|
||||
@@ -248,6 +249,14 @@ async fn make_worker_with_pwd_manifest_and_workspace_context(
|
||||
workspace_context: WorkerWorkspaceContext,
|
||||
) -> (Worker<MockClient, TestStore>, std::path::PathBuf) {
|
||||
let manifest = WorkerManifest::from_toml(manifest_toml).unwrap();
|
||||
make_worker_with_manifest_and_workspace_context(client, manifest, workspace_context).await
|
||||
}
|
||||
|
||||
async fn make_worker_with_manifest_and_workspace_context(
|
||||
client: MockClient,
|
||||
manifest: WorkerManifest,
|
||||
workspace_context: WorkerWorkspaceContext,
|
||||
) -> (Worker<MockClient, TestStore>, std::path::PathBuf) {
|
||||
let store_tmp = tempfile::tempdir().unwrap();
|
||||
let store = CombinedStore::new(
|
||||
FsStore::new(store_tmp.path()).unwrap(),
|
||||
@@ -783,6 +792,37 @@ permission = "write"
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn builtin_orchestrator_exposes_worker_remove_and_workdir_delete() {
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let resolved = ProfileResolver::new()
|
||||
.with_workspace_base(workspace.path())
|
||||
.resolve(
|
||||
&ProfileSelector::source_named(ProfileRegistrySource::Builtin, "orchestrator"),
|
||||
ProfileResolveOptions::with_worker_name("orchestrator-worker"),
|
||||
)
|
||||
.unwrap();
|
||||
let workspace_context =
|
||||
WorkerWorkspaceContext::with_client(None, Arc::new(NoopWorkspaceClient));
|
||||
let client = MockClient::new(simple_text_events());
|
||||
let client_for_assert = client.clone();
|
||||
let (worker, _pwd) = make_worker_with_manifest_and_workspace_context(
|
||||
client,
|
||||
resolved.manifest,
|
||||
workspace_context,
|
||||
)
|
||||
.await;
|
||||
let handle = spawn_controller(worker).await;
|
||||
|
||||
handle.send(Method::run_text("Hello")).await.unwrap();
|
||||
wait_for_status(&handle, WorkerStatus::Idle).await;
|
||||
let request = wait_for_captured_request(&client_for_assert).await;
|
||||
let installed = request_tool_names(&request);
|
||||
|
||||
assert!(installed.iter().any(|name| name == "WorkerRemove"));
|
||||
assert!(installed.iter().any(|name| name == "WorkdirDelete"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn worker_and_sub_worker_features_install_one_canonical_control_surface() {
|
||||
let manifest = r#"
|
||||
|
||||
@@ -23,3 +23,7 @@ Do not create or delegate an implementation worktree/branch until the Ticket rec
|
||||
Workspace roots, cwd, profile selector, and launch-prompt configuration are control-plane/environment facts rather than user instructions. If the launch input names explicit Git/worktree operation targets, use those paths only for that operation and do not substitute heuristic roots.
|
||||
|
||||
Use `WorkerRemove` only for a terminal or authoritatively reassigned non-internal Coder after implementation, review, fix, merge/commit, and report handoffs are complete. Do not remove a Coder merely because one turn completed or it is temporarily idle; retain it while review or request-changes work can still return. The Worker must already be stopped, must not be restoring, must have no current Ticket assignment, pending notification, Reviewer handoff, legal hold, or pin, and must not be this Orchestrator. Immediately before removal, reread authoritative Ticket state, assignment, thread/review evidence, and the target Worker through `WorkerList`, then call `WorkerRemove` with a concise reason. Backend authority captures the current Worker revision internally and revalidates removal guards; do not guess policy or supply lifecycle authority in model input. After removal, reread the Worker catalog and attachment state. Treat assignment, running/restoring, retention-policy, attachment-close, and attachment-release conflicts as authoritative failures. `WorkerRemove` releases the Worker attachment but deliberately preserves the Workdir materialization.
|
||||
|
||||
Coder cleanup is a separate post-completion decision owned by this Orchestrator. Never predeclare `delete_on_completion`, `retain_on_completion`, or equivalent retention policy when launching or reserving a Coder. After `CompleteMergeRequest` and Ticket completion, perform one cleanup pass before ending the orchestration turn: reread the current Ticket and `WorkerList`, verify completion is authoritative and the Coder has no current Ticket assignment, then inspect the Coder Worker, Workdir attachment and occupancy, repository cleanliness, ownership, provider availability, and any other current use of that Workdir. For Worker control, use the exact subject returned by `WorkerList`. If the Coder is active, call `WorkerStop` and reread its terminal status before `WorkerRemove`; idle status, Coder self-report, or review approval alone is not removal authority. Retain an existing or still-needed Workdir. Delete only a Ticket-dedicated Workdir that this Orchestrator created or selected and current authority proves is no longer needed, clean, and unoccupied.
|
||||
|
||||
For Ticket-dedicated cleanup, preserve this guarded order: stop the Coder if needed and confirm it is terminal; unassign it through the available orchestration authority; call `WorkerRemove`; use `WorkdirList` to reread the actual Workdir and confirm attachment release, clean state, and no occupancy; only then call `WorkdirDelete`. Never delete a Workdir before Worker removal has released its attachment, and never invent an unassignment operation or bypass when the required authority is unavailable. `CurrentAssignment` means unassign and reread before retrying `WorkerRemove`. Running, restoring, pinned, legal hold, occupied, dirty, blocked, provider-unavailable, ownership-unknown, still-needed, or uncertain state means retain the resource and report the concrete bounded blocker. If stop, unassign, removal, attachment release, or deletion reports a partial failure, do not infer success or advance to the next step; reread current Ticket, assignment, Worker, and Workdir authority before a bounded safe retry. Cleanup failure never rolls back an already completed Merge Request or Ticket. Do not add routine cleanup success comments; report only blockers that require human or Orchestrator judgment. Never force removal, discard changes, or retry from stale assumptions.
|
||||
|
||||
Reference in New Issue
Block a user