diff --git a/Cargo.lock b/Cargo.lock index 35db3e13..3cb66315 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2780,7 +2780,7 @@ checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" [[package]] name = "orchestrator-cli" -version = "0.7.0-rc.12" +version = "0.7.0-rc.13" dependencies = [ "animus-actor", "animus-control-protocol", diff --git a/crates/orchestrator-cli/Cargo.toml b/crates/orchestrator-cli/Cargo.toml index 2c3165fa..8c9151ff 100644 --- a/crates/orchestrator-cli/Cargo.toml +++ b/crates/orchestrator-cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "orchestrator-cli" -version = "0.7.0-rc.12" +version = "0.7.0-rc.13" edition = "2021" license = "Elastic-2.0" default-run = "animus" diff --git a/crates/orchestrator-cli/src/cli_types/workflow_types.rs b/crates/orchestrator-cli/src/cli_types/workflow_types.rs index b2c7a0cb..339789da 100644 --- a/crates/orchestrator-cli/src/cli_types/workflow_types.rs +++ b/crates/orchestrator-cli/src/cli_types/workflow_types.rs @@ -170,6 +170,11 @@ pub(crate) enum WorkflowConfigCommand { AgentSet(WorkflowConfigAgentSetArgs), /// Remove one agent definition (read-modify-write the full config). AgentRemove(WorkflowConfigEntityIdArgs), + /// Create or replace one phase definition on the config_source base + /// (read-modify-write). Writes `WorkflowConfig.phase_definitions`, NOT the + /// agent-runtime overlay that `workflow phases upsert` writes — so a + /// subsequently-set workflow that references the phase validates. + PhaseSet(WorkflowConfigPhaseSetArgs), /// Create or replace one workflow definition (read-modify-write). WorkflowSet(WorkflowConfigWorkflowSetArgs), /// Remove one workflow definition (read-modify-write). @@ -204,6 +209,18 @@ pub(crate) struct WorkflowConfigAgentSetArgs { pub(crate) input_json: String, } +#[derive(Debug, Args)] +pub(crate) struct WorkflowConfigPhaseSetArgs { + #[arg(long, value_name = "PHASE_ID", help = "Phase definition id to create or replace.")] + pub(crate) id: String, + #[arg( + long, + value_name = "JSON", + help = "Phase execution definition JSON payload (the value of phase_definitions.)." + )] + pub(crate) input_json: String, +} + #[derive(Debug, Args)] pub(crate) struct WorkflowConfigWorkflowSetArgs { #[arg(long, value_name = "JSON", help = "Workflow definition JSON payload (must include an 'id' field).")] diff --git a/crates/orchestrator-cli/src/services/operations/ops_workflow/config.rs b/crates/orchestrator-cli/src/services/operations/ops_workflow/config.rs index 24839427..24021ac5 100644 --- a/crates/orchestrator-cli/src/services/operations/ops_workflow/config.rs +++ b/crates/orchestrator-cli/src/services/operations/ops_workflow/config.rs @@ -557,6 +557,17 @@ pub(crate) fn set_config_agent_payload(project_root: &str, id: &str, input_json: write_back_payload(project_root, &config) } +/// `animus workflow config phase-set` — upsert one phase definition on the +/// config_source base (`WorkflowConfig.phase_definitions`), NOT the agent-runtime +/// overlay that `workflow phases upsert` writes. A workflow set afterwards that +/// references this phase then resolves during post-pack-merge validation. +pub(crate) fn set_config_phase_payload(project_root: &str, id: &str, input_json: &str) -> Result { + let definition: orchestrator_config::PhaseExecutionDefinition = serde_json::from_str(input_json) + .context("invalid phase execution definition JSON for workflow config phase-set")?; + let config = orchestrator_core::set_phase_definition(Path::new(project_root), id, definition)?; + write_back_payload(project_root, &config) +} + /// `animus workflow config agent-remove` — remove one agent definition. pub(crate) fn remove_config_agent_payload(project_root: &str, id: &str) -> Result { let config = orchestrator_core::remove_agent_profile(Path::new(project_root), id)?; diff --git a/crates/orchestrator-cli/src/services/operations/ops_workflow/mod.rs b/crates/orchestrator-cli/src/services/operations/ops_workflow/mod.rs index e18142e7..3e420dc9 100644 --- a/crates/orchestrator-cli/src/services/operations/ops_workflow/mod.rs +++ b/crates/orchestrator-cli/src/services/operations/ops_workflow/mod.rs @@ -756,6 +756,9 @@ pub(crate) async fn handle_workflow( WorkflowConfigCommand::AgentRemove(args) => { print_value(config::remove_config_agent_payload(project_root, &args.id)?, json) } + WorkflowConfigCommand::PhaseSet(args) => { + print_value(config::set_config_phase_payload(project_root, &args.id, &args.input_json)?, json) + } WorkflowConfigCommand::WorkflowSet(args) => { print_value(config::set_config_workflow_payload(project_root, &args.input_json)?, json) } @@ -1023,6 +1026,17 @@ mod tests { assert!(message.contains("workflow agent-runtime set --help")); } + #[test] + fn set_config_phase_payload_reports_actionable_json_error() { + let error = + set_config_phase_payload("/tmp/unused", "my-phase", "{invalid").expect_err("invalid payload should fail"); + let message = format!("{error:#}"); + assert!( + message.contains("invalid phase execution definition JSON for workflow config phase-set"), + "expected actionable phase-set JSON error, got: {message}" + ); + } + #[test] fn resolve_workflow_run_dispatch_builds_custom_title_dispatch() { let dispatch = resolve_workflow_run_dispatch( diff --git a/crates/orchestrator-cli/src/shared/output.rs b/crates/orchestrator-cli/src/shared/output.rs index a176fce4..be2af03d 100644 --- a/crates/orchestrator-cli/src/shared/output.rs +++ b/crates/orchestrator-cli/src/shared/output.rs @@ -80,7 +80,7 @@ pub(crate) fn emit_cli_error(err: &anyhow::Error, json: bool) { let envelope = CliErrorEnvelope { schema: CLI_SCHEMA, ok: false, - error: CliErrorBody { code: code.to_string(), message: err.to_string(), exit_code, details }, + error: CliErrorBody { code: code.to_string(), message: json_error_message(err), exit_code, details }, }; eprintln!("{}", serialize_compact_json(&envelope).unwrap_or_else(|_| { format!("{{\"schema\":\"{}\",\"ok\":false,\"error\":{{\"code\":\"internal\",\"message\":\"serialization failure\",\"exit_code\":1}}}}", CLI_SCHEMA_ID) @@ -93,6 +93,17 @@ pub(crate) fn emit_cli_error(err: &anyhow::Error, json: bool) { } } +/// Build the `--json` error message from an anyhow error. +/// +/// Uses anyhow's alternate Display (`{:#}`) so the message carries the FULL +/// context chain — the outer `.context(...)` plus every underlying source, +/// joined by ": " — matching what the human (`error: {:#}`) path prints. Plain +/// `err.to_string()` yields ONLY the top-level context and silently drops the +/// real cause, which made `--json` consumers blind to the underlying failure. +fn json_error_message(err: &anyhow::Error) -> String { + format!("{err:#}") +} + fn should_emit_help_hint(message: &str) -> bool { !message.to_ascii_lowercase().contains("--help") } @@ -216,6 +227,30 @@ mod tests { assert_eq!(classify_exit_code(&long), 2); } + #[test] + fn json_error_message_includes_full_context_chain() { + use anyhow::Context; + // Mirror the real failure shape: an inner cause wrapped by an outer + // `.context(...)`. The --json message must surface BOTH — the outer + // context and the inner source — not just the top-level context. + let err = Err::<(), anyhow::Error>(anyhow!( + "workflow 'X' references unknown phase 'Y'; add it to phase_catalog or define it under phases" + )) + .context("the config to write is invalid once pack overlays are merged; refusing to persist") + .unwrap_err(); + let message = json_error_message(&err); + assert!( + message.contains("the config to write is invalid once pack overlays are merged"), + "message must contain the outer context, got: {message}" + ); + assert!( + message.contains("references unknown phase 'Y'"), + "message must contain the inner source cause, got: {message}" + ); + // Plain Display would drop the cause; assert we improved on it. + assert_ne!(message, err.to_string(), "alternate Display must add the source chain"); + } + #[test] fn should_emit_help_hint_is_case_insensitive() { assert!(!should_emit_help_hint("Run with --HELP for usage")); diff --git a/crates/orchestrator-config/src/workflow_config/config_write.rs b/crates/orchestrator-config/src/workflow_config/config_write.rs index 8037cafc..f01a1a11 100644 --- a/crates/orchestrator-config/src/workflow_config/config_write.rs +++ b/crates/orchestrator-config/src/workflow_config/config_write.rs @@ -12,7 +12,7 @@ use std::path::Path; use anyhow::{anyhow, Context, Result}; -use animus_config_protocol::agent_types::AgentProfileOverlay; +use animus_config_protocol::agent_types::{AgentProfileOverlay, PhaseExecutionDefinition}; use super::config_source_client::{resolve_plugin_base, write_plugin_config}; use super::loading::compile_workflow_config_onto_base; @@ -98,6 +98,34 @@ pub fn remove_agent_profile(project_root: &Path, agent_id: &str) -> Result Result { + let phase_id = phase_id.trim(); + if phase_id.is_empty() { + return Err(anyhow!("phase id must not be empty")); + } + read_modify_write(project_root, |config| { + // Phase ids resolve case-insensitively everywhere else (validation's + // reference check and the runtime `phase_execution` lookup both use + // `eq_ignore_ascii_case`). Drop any existing key that differs only by + // case before inserting, so an upsert REPLACES the phase instead of + // leaving a stale duplicate that could win by map order. + config.phase_definitions.retain(|existing, _| !existing.eq_ignore_ascii_case(phase_id)); + config.phase_definitions.insert(phase_id.to_string(), definition); + Ok(()) + }) +} + /// Upsert (create or replace) a workflow definition by its `id`. The definition /// replaces any existing entry with the same id; otherwise it is appended. pub fn upsert_workflow_definition(project_root: &Path, definition: WorkflowDefinition) -> Result { @@ -223,6 +251,46 @@ mod tests { assert!(msg.contains("invalid"), "error must flag the invalid config, got: {msg}"); } + #[test] + fn phase_authored_on_base_lets_a_referencing_workflow_validate() { + // The whole point of `set_phase_definition`: a phase written to the + // config_source base's `phase_definitions` must resolve when a + // subsequently-set workflow references it — no "references unknown + // phase". We drive the validate gate directly (write_full_workflow_config + // validates BEFORE it attempts the plugin write), so phase-reference + // resolution is exercised without a live writable plugin. + let dir = tempfile::tempdir().expect("tempdir"); + let mut config = builtin_workflow_config(); + // Minimal valid phase definition (agent mode needs no agent_id). + let phase: PhaseExecutionDefinition = serde_json::from_str(r#"{"mode":"agent"}"#).expect("valid phase json"); + config.phase_definitions.insert("authored-phase".to_string(), phase); + let mut wf = sample_workflow("uses-authored"); + wf.phases = + vec![animus_config_protocol::workflow_types::WorkflowPhaseEntry::Simple("authored-phase".to_string())]; + config.workflows = vec![wf]; + + // Validation PASSES: the only remaining failure is the write step (no + // writable config_source plugin under test), never an "unknown phase". + let err = write_full_workflow_config(dir.path(), &config).expect_err("no writable source under test"); + let msg = format!("{err:#}"); + assert!(!msg.contains("unknown phase"), "phase authored on the base must resolve; got: {msg}"); + assert!( + msg.contains("no config_source plugin") || msg.contains("does not support writes"), + "should fail only at the write step, got: {msg}" + ); + + // Control: WITHOUT the phase definition the same workflow reference is + // rejected as unknown — proving it is the authored phase that unblocks it. + let mut missing = builtin_workflow_config(); + let mut wf2 = sample_workflow("uses-authored"); + wf2.phases = + vec![animus_config_protocol::workflow_types::WorkflowPhaseEntry::Simple("authored-phase".to_string())]; + missing.workflows = vec![wf2]; + let err2 = write_full_workflow_config(dir.path(), &missing).expect_err("unknown phase must be rejected"); + let msg2 = format!("{err2:#}"); + assert!(msg2.contains("unknown phase"), "control must fail as unknown phase; got: {msg2}"); + } + #[test] fn write_to_non_writable_or_absent_source_is_actionable() { // A VALID config that passes the validate gate then hits the write path. diff --git a/crates/orchestrator-config/src/workflow_config/mod.rs b/crates/orchestrator-config/src/workflow_config/mod.rs index 92bd4cc7..478c8e5b 100644 --- a/crates/orchestrator-config/src/workflow_config/mod.rs +++ b/crates/orchestrator-config/src/workflow_config/mod.rs @@ -61,8 +61,8 @@ pub use animus_config_protocol::overlay::{ // --------------------------------------------------------------------------- pub use config_write::{ - remove_agent_profile, remove_workflow_definition, upsert_agent_profile, upsert_workflow_definition, - write_full_workflow_config, + remove_agent_profile, remove_workflow_definition, set_phase_definition, upsert_agent_profile, + upsert_workflow_definition, write_full_workflow_config, }; pub use environment_routing::resolve_environment; pub use loading::{ diff --git a/crates/orchestrator-core/src/lib.rs b/crates/orchestrator-core/src/lib.rs index 4b11126f..a1dab27f 100644 --- a/crates/orchestrator-core/src/lib.rs +++ b/crates/orchestrator-core/src/lib.rs @@ -170,7 +170,7 @@ pub use workflow_config::{ load_workflow_config_with_metadata, merge_yaml_into_config, missing_project_skill_reference_warnings, missing_skill_reference_warnings_for_sources, missing_skill_yaml_warnings, parse_yaml_workflow_config, remove_agent_profile, remove_generated_workflow_phase, remove_workflow_definition, resolve_workflow_phase_plan, - resolve_workflow_rework_attempts, resolve_workflow_skip_guards, resolve_workflow_variables, + resolve_workflow_rework_attempts, resolve_workflow_skip_guards, resolve_workflow_variables, set_phase_definition, resolve_workflow_verdict_routing, unenforced_project_yaml_warnings, unenforced_yaml_field_warnings, upsert_agent_profile, upsert_generated_workflow_phase, upsert_generated_workflow_pipeline, upsert_workflow_definition, validate_and_compile_yaml_workflows, validate_workflow_and_runtime_configs, diff --git a/docs/reference/cli/index.md b/docs/reference/cli/index.md index d23d520b..bf95fc39 100644 --- a/docs/reference/cli/index.md +++ b/docs/reference/cli/index.md @@ -186,6 +186,7 @@ animus │ │ ├── set Replace the full config via the writable config_source plugin (validates first; rejected on read-only sources) │ │ ├── agent-set Create or replace one agent definition (read-modify-write the full config) │ │ ├── agent-remove Remove one agent definition (read-modify-write the full config) +│ │ ├── phase-set Create or replace one phase definition on the config_source base (read-modify-write; writes phase_definitions, not the agent-runtime overlay) │ │ ├── workflow-set Create or replace one workflow definition (read-modify-write) │ │ └── workflow-remove Remove one workflow definition (read-modify-write) │ ├── state-machine @@ -1925,6 +1926,15 @@ the plugin — nothing is partially written. model back. This is the **definition**-management verb; it does not collide with the runtime `animus agent {list,get,run,...}` surface. - `animus workflow config agent-remove --id ` — remove one agent. +- `animus workflow config phase-set --id --input-json ` — upsert + one phase definition on the RAW config_source base + (`WorkflowConfig.phase_definitions`). Read-modify-write, same validate-before-write + path as the other entity verbs. This writes the **config_source base**, NOT the + agent-runtime overlay that `animus workflow phases upsert` writes — so a phase + authored here resolves when a subsequently-set workflow references it, instead of + failing `workflow-set` with "references unknown phase". The JSON is a + `PhaseExecutionDefinition` (the value of `phase_definitions.`), e.g. + `--input-json '{"mode":"agent","agent_id":"builder"}'`. - `animus workflow config workflow-set --input-json ` — upsert one workflow definition (the JSON must include an `id`). - `animus workflow config workflow-remove --id ` — remove one workflow. @@ -1941,7 +1951,7 @@ watch (gated on the `config_watch` capability). | Metric | Count | |---|---| | Top-level commands | 28 | -| Nested command entries (all levels) | 214 | +| Nested command entries (all levels) | 215 | Counting basis: counts are derived from the command tree above. "Top-level commands" counts the column-0 tree roots (`animus `); "Nested