Adversarial review of the v1.2 Workflows track surfaced two HIGH footguns and several correctness/hardening gaps. Fixes: HIGH - pull round-trip: `pic pull` dropped `[[workflows]]`, so a later `apply --prune` silently deleted them. The list endpoint now returns the full `definition`; pull rebuilds the manifest block (inverse of workflow_to_wire). - case-colliding names: two workflows differing only by case collided in the reconcile diff (keyed by lower(name)), silently dropping one. Rejected up front in validate_bundle_for. MEDIUM - reclaim retry budget: a crashed attempt (no outcome) consumed the retry budget. reclaim_stale_steps now decrements `attempt` (floored) and clears `next_attempt_at`, so a crash no longer counts as a failed try. - on_error/backoff: typed the manifest fields against the shared enums so a bad value fails at TOML parse with a clear message, not an opaque 500. - dedupe depends_on: a repeated dependency inflated the Kahn in-degree past the single decrement, reporting a spurious cycle for a valid DAG. Count distinct. - canceled child: a canceled sub-workflow resolved the parent step with a message naming the cause instead of a generic "failed"; documented that a run-level cancel op is not yet supported. LOW - list_run_steps is now app-scoped at the query (JOIN workflow_runs) rather than relying on caller pre-verification. - partial failure is surfaced: a run that succeeds with on_error=continue failures records them in the run's `error` field. - admin-started runs log the root_execution_id against the principal. - documented the deliberate when(missing→false) vs template(missing→fail) asymmetry; corrected the claim's atomicity comment. Tests: unit (dedupe-deps), DB-gated reclaim-budget assertion, and two new CLI journeys (duplicate-name rejection, pull→plan clean round-trip). fmt + clippy -D warnings clean; 450 lib + 14 orchestrator DB + 5 workflow journeys pass; schema snapshot unchanged (no migration). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
373 lines
13 KiB
Rust
373 lines
13 KiB
Rust
//! v1.2 Workflows — M1 declarative authoring journey via `pic`.
|
|
//!
|
|
//! An app applies a `[[workflows]]` manifest block (a DAG referencing its
|
|
//! scripts, with `depends_on`, a `when`, and an input template) → the apply
|
|
//! report shows the workflow created; re-applying is a NoOp; a manifest with a
|
|
//! cyclic DAG is rejected atomically; pruning the block deletes the workflow.
|
|
//!
|
|
//! The durable execution of a run (the orchestrator) lands in M2; this journey
|
|
//! pins the authoring + reconcile path the operator uses.
|
|
|
|
use std::fs;
|
|
|
|
use tempfile::TempDir;
|
|
|
|
use crate::common;
|
|
use crate::common::cleanup::AppGuard;
|
|
|
|
fn manifest_dir() -> TempDir {
|
|
let dir = TempDir::new().expect("tempdir");
|
|
fs::create_dir_all(dir.path().join("scripts")).expect("scripts dir");
|
|
dir
|
|
}
|
|
|
|
fn seed_scripts(dir: &TempDir) {
|
|
fs::write(
|
|
dir.path().join("scripts/validate.rhai"),
|
|
"let body = #{ ok: true, value: 7 }; body",
|
|
)
|
|
.unwrap();
|
|
fs::write(
|
|
dir.path().join("scripts/charge.rhai"),
|
|
"let body = #{ charged: true }; body",
|
|
)
|
|
.unwrap();
|
|
}
|
|
|
|
const SCRIPTS_BLOCK: &str = "\
|
|
[[scripts]]\nname = \"validate\"\nfile = \"scripts/validate.rhai\"\n\n\
|
|
[[scripts]]\nname = \"charge\"\nfile = \"scripts/charge.rhai\"\n\n";
|
|
|
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
|
#[test]
|
|
fn workflow_apply_creates_then_noop_then_prunes() {
|
|
let Some(fx) = common::fixture_or_skip() else {
|
|
return;
|
|
};
|
|
let env = common::admin_env(fx);
|
|
let slug = common::unique_slug("wf");
|
|
common::pic_as(&env)
|
|
.args(["apps", "create", &slug])
|
|
.assert()
|
|
.success();
|
|
let _guard = AppGuard::new(&env.url, &env.token, &slug);
|
|
|
|
let dir = manifest_dir();
|
|
seed_scripts(&dir);
|
|
let with_wf = format!(
|
|
"[app]\nslug = \"{slug}\"\nname = \"WF Test\"\n\n{SCRIPTS_BLOCK}\
|
|
[[workflows]]\nname = \"pipeline\"\n\n\
|
|
[[workflows.steps]]\nname = \"v\"\nfunction = \"validate\"\n\
|
|
input = {{ order = \"{{{{ input.order }}}}\" }}\n\n\
|
|
[[workflows.steps]]\nname = \"c\"\nfunction = \"charge\"\n\
|
|
depends_on = [\"v\"]\nwhen = \"steps.v.output.ok == true\"\n"
|
|
);
|
|
let path = dir.path().join("picloud.toml");
|
|
fs::write(&path, &with_wf).unwrap();
|
|
|
|
// First apply: creates the two scripts + the workflow.
|
|
let out = common::pic_as(&env)
|
|
.args(["apply", "--file"])
|
|
.arg(&path)
|
|
.output()
|
|
.expect("apply");
|
|
let stdout = String::from_utf8_lossy(&out.stdout);
|
|
assert!(
|
|
out.status.success(),
|
|
"apply failed: {}\n{stdout}",
|
|
String::from_utf8_lossy(&out.stderr)
|
|
);
|
|
assert!(
|
|
stdout.contains("workflows") && stdout.contains("+1"),
|
|
"expected a workflow creation in the report:\n{stdout}"
|
|
);
|
|
|
|
// Re-plan is clean (desired state reached — the workflow is a NoOp).
|
|
let p = String::from_utf8(
|
|
common::pic_as(&env)
|
|
.args(["plan", "--file"])
|
|
.arg(&path)
|
|
.output()
|
|
.unwrap()
|
|
.stdout,
|
|
)
|
|
.unwrap();
|
|
assert!(
|
|
!p.contains("create") && !p.contains("update"),
|
|
"expected a clean plan after apply:\n{p}"
|
|
);
|
|
|
|
// Drop the workflow from the manifest and prune → it is deleted.
|
|
let without_wf = format!("[app]\nslug = \"{slug}\"\nname = \"WF Test\"\n\n{SCRIPTS_BLOCK}");
|
|
fs::write(&path, &without_wf).unwrap();
|
|
let pruned = common::pic_as(&env)
|
|
.args(["apply", "--file"])
|
|
.arg(&path)
|
|
.args(["--prune", "--yes"])
|
|
.output()
|
|
.expect("prune apply");
|
|
let ptext = String::from_utf8_lossy(&pruned.stdout);
|
|
assert!(
|
|
pruned.status.success(),
|
|
"prune apply failed: {}\n{ptext}",
|
|
String::from_utf8_lossy(&pruned.stderr)
|
|
);
|
|
assert!(
|
|
ptext.contains("workflows") && ptext.contains("-1"),
|
|
"expected a workflow deletion under --prune:\n{ptext}"
|
|
);
|
|
}
|
|
|
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
|
#[test]
|
|
fn workflow_with_a_cycle_is_rejected() {
|
|
let Some(fx) = common::fixture_or_skip() else {
|
|
return;
|
|
};
|
|
let env = common::admin_env(fx);
|
|
let slug = common::unique_slug("wfcycle");
|
|
common::pic_as(&env)
|
|
.args(["apps", "create", &slug])
|
|
.assert()
|
|
.success();
|
|
let _guard = AppGuard::new(&env.url, &env.token, &slug);
|
|
|
|
let dir = manifest_dir();
|
|
seed_scripts(&dir);
|
|
// a → b → a is a cycle; the server's topological sort must reject it.
|
|
let cyclic = format!(
|
|
"[app]\nslug = \"{slug}\"\nname = \"WF Cycle\"\n\n{SCRIPTS_BLOCK}\
|
|
[[workflows]]\nname = \"loop\"\n\n\
|
|
[[workflows.steps]]\nname = \"a\"\nfunction = \"validate\"\ndepends_on = [\"b\"]\n\n\
|
|
[[workflows.steps]]\nname = \"b\"\nfunction = \"charge\"\ndepends_on = [\"a\"]\n"
|
|
);
|
|
let path = dir.path().join("picloud.toml");
|
|
fs::write(&path, &cyclic).unwrap();
|
|
|
|
let out = common::pic_as(&env)
|
|
.args(["apply", "--file"])
|
|
.arg(&path)
|
|
.output()
|
|
.expect("apply");
|
|
assert!(!out.status.success(), "cyclic workflow should be rejected");
|
|
let err = String::from_utf8_lossy(&out.stderr);
|
|
assert!(err.contains("cycle"), "expected a cycle error, got:\n{err}");
|
|
}
|
|
|
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
|
#[test]
|
|
fn workflow_duplicate_name_is_rejected() {
|
|
let Some(fx) = common::fixture_or_skip() else {
|
|
return;
|
|
};
|
|
let env = common::admin_env(fx);
|
|
let slug = common::unique_slug("wfdup");
|
|
common::pic_as(&env)
|
|
.args(["apps", "create", &slug])
|
|
.assert()
|
|
.success();
|
|
let _guard = AppGuard::new(&env.url, &env.token, &slug);
|
|
|
|
let dir = manifest_dir();
|
|
seed_scripts(&dir);
|
|
// Two workflows whose names collide case-insensitively — the reconcile keys
|
|
// by lower(name), so this must be rejected before it silently drops one.
|
|
let dup = format!(
|
|
"[app]\nslug = \"{slug}\"\nname = \"WF Dup\"\n\n{SCRIPTS_BLOCK}\
|
|
[[workflows]]\nname = \"Pipe\"\n\n\
|
|
[[workflows.steps]]\nname = \"a\"\nfunction = \"validate\"\n\n\
|
|
[[workflows]]\nname = \"pipe\"\n\n\
|
|
[[workflows.steps]]\nname = \"a\"\nfunction = \"charge\"\n"
|
|
);
|
|
let path = dir.path().join("picloud.toml");
|
|
fs::write(&path, &dup).unwrap();
|
|
|
|
let out = common::pic_as(&env)
|
|
.args(["apply", "--file"])
|
|
.arg(&path)
|
|
.output()
|
|
.expect("apply");
|
|
assert!(!out.status.success(), "duplicate names should be rejected");
|
|
let err = String::from_utf8_lossy(&out.stderr);
|
|
assert!(
|
|
err.contains("duplicate workflow"),
|
|
"expected a duplicate-workflow error, got:\n{err}"
|
|
);
|
|
}
|
|
|
|
/// A workflow survives a `pull` → `plan` round-trip with no phantom drift.
|
|
/// Regression for the bug where `pic pull` dropped `[[workflows]]`, so a
|
|
/// subsequent `apply --prune` would silently delete them.
|
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
|
#[test]
|
|
fn workflow_pull_round_trips_clean() {
|
|
let Some(fx) = common::fixture_or_skip() else {
|
|
return;
|
|
};
|
|
let env = common::admin_env(fx);
|
|
let slug = common::unique_slug("wfpull");
|
|
common::pic_as(&env)
|
|
.args(["apps", "create", &slug])
|
|
.assert()
|
|
.success();
|
|
let _guard = AppGuard::new(&env.url, &env.token, &slug);
|
|
|
|
let dir = manifest_dir();
|
|
seed_scripts(&dir);
|
|
// Exercise the full step shape the pull mapper must round-trip: input
|
|
// template, when, depends_on, retry (with defaults omitted), on_error.
|
|
let manifest = format!(
|
|
"[app]\nslug = \"{slug}\"\nname = \"WF Pull\"\n\n{SCRIPTS_BLOCK}\
|
|
[[workflows]]\nname = \"pipeline\"\n\n\
|
|
[[workflows.steps]]\nname = \"v\"\nfunction = \"validate\"\n\
|
|
input = {{ order = \"{{{{ input.order }}}}\" }}\n\n\
|
|
[[workflows.steps]]\nname = \"c\"\nfunction = \"charge\"\n\
|
|
depends_on = [\"v\"]\nwhen = \"steps.v.output.ok == true\"\n\
|
|
on_error = \"continue\"\n[workflows.steps.retry]\nmax_attempts = 3\n"
|
|
);
|
|
let path = dir.path().join("picloud.toml");
|
|
fs::write(&path, &manifest).unwrap();
|
|
common::pic_as(&env)
|
|
.args(["apply", "--file"])
|
|
.arg(&path)
|
|
.assert()
|
|
.success();
|
|
|
|
// Pull the live state into a fresh dir, then plan it back — the workflow
|
|
// must round-trip to a clean no-op (no create / update / delete).
|
|
let out = TempDir::new().expect("tempdir");
|
|
common::pic_as(&env)
|
|
.args(["pull", &slug, "--dir"])
|
|
.arg(out.path())
|
|
.assert()
|
|
.success();
|
|
let pulled = out.path().join("picloud.toml");
|
|
let toml = fs::read_to_string(&pulled).unwrap();
|
|
assert!(
|
|
toml.contains("[[workflows]]") && toml.contains("pipeline"),
|
|
"pulled manifest is missing the workflow:\n{toml}"
|
|
);
|
|
|
|
let plan = common::pic_as(&env)
|
|
.args(["plan", "--file"])
|
|
.arg(&pulled)
|
|
.output()
|
|
.expect("plan");
|
|
let stdout = String::from_utf8_lossy(&plan.stdout);
|
|
assert!(
|
|
plan.status.success(),
|
|
"plan failed: {}\n{stdout}",
|
|
String::from_utf8_lossy(&plan.stderr)
|
|
);
|
|
// The workflow row must be a no-op; nothing created/updated/deleted.
|
|
assert!(
|
|
!stdout.contains("create") && !stdout.contains("update") && !stdout.contains("delete"),
|
|
"pull→plan should diff clean for workflows, got:\n{stdout}"
|
|
);
|
|
}
|
|
|
|
/// M5 end-to-end: `pic workflows run` starts a run, the durable orchestrator
|
|
/// (running inside the fixture's picloud) executes both steps, and
|
|
/// `run-status` shows it succeeded.
|
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
|
#[test]
|
|
fn workflow_run_executes_end_to_end() {
|
|
let Some(fx) = common::fixture_or_skip() else {
|
|
return;
|
|
};
|
|
let env = common::admin_env(fx);
|
|
let slug = common::unique_slug("wfe2e");
|
|
common::pic_as(&env)
|
|
.args(["apps", "create", &slug])
|
|
.assert()
|
|
.success();
|
|
let _guard = AppGuard::new(&env.url, &env.token, &slug);
|
|
|
|
let dir = manifest_dir();
|
|
seed_scripts(&dir);
|
|
// validate → charge; charge only runs when validate's output.ok is true.
|
|
let manifest = format!(
|
|
"[app]\nslug = \"{slug}\"\nname = \"WF E2E\"\n\n{SCRIPTS_BLOCK}\
|
|
[[workflows]]\nname = \"pipeline\"\n\n\
|
|
[[workflows.steps]]\nname = \"v\"\nfunction = \"validate\"\n\
|
|
input = {{ order = \"{{{{ input.order }}}}\" }}\n\n\
|
|
[[workflows.steps]]\nname = \"c\"\nfunction = \"charge\"\n\
|
|
depends_on = [\"v\"]\nwhen = \"steps.v.output.ok == true\"\n"
|
|
);
|
|
let path = dir.path().join("picloud.toml");
|
|
fs::write(&path, &manifest).unwrap();
|
|
common::pic_as(&env)
|
|
.args(["apply", "--file"])
|
|
.arg(&path)
|
|
.assert()
|
|
.success();
|
|
|
|
// `pic workflows ls` shows the definition.
|
|
let ls = String::from_utf8(
|
|
common::pic_as(&env)
|
|
.args(["workflows", "ls", "--app", &slug])
|
|
.output()
|
|
.unwrap()
|
|
.stdout,
|
|
)
|
|
.unwrap();
|
|
assert!(
|
|
ls.contains("pipeline"),
|
|
"workflows ls missing pipeline:\n{ls}"
|
|
);
|
|
|
|
// Start a run (JSON output so we can parse the run id).
|
|
let run_out = common::pic_as(&env)
|
|
.args([
|
|
"--output",
|
|
"json",
|
|
"workflows",
|
|
"run",
|
|
"--app",
|
|
&slug,
|
|
"pipeline",
|
|
"--input",
|
|
"{\"order\":\"o1\"}",
|
|
])
|
|
.output()
|
|
.expect("run");
|
|
assert!(
|
|
run_out.status.success(),
|
|
"workflows run failed: {}",
|
|
String::from_utf8_lossy(&run_out.stderr)
|
|
);
|
|
let started: serde_json::Value =
|
|
serde_json::from_slice(&run_out.stdout).expect("run output is JSON");
|
|
let run_id = started["run_id"]
|
|
.as_str()
|
|
.expect("run_id in output")
|
|
.to_string();
|
|
|
|
// Poll run-status until the run reaches a terminal state.
|
|
let mut final_status = String::new();
|
|
for _ in 0..40 {
|
|
let status_out = String::from_utf8(
|
|
common::pic_as(&env)
|
|
.args(["workflows", "run-status", "--app", &slug, &run_id])
|
|
.output()
|
|
.unwrap()
|
|
.stdout,
|
|
)
|
|
.unwrap();
|
|
final_status = status_out;
|
|
if final_status.contains("succeeded") || final_status.contains("failed") {
|
|
break;
|
|
}
|
|
std::thread::sleep(std::time::Duration::from_millis(300));
|
|
}
|
|
assert!(
|
|
final_status.contains("succeeded"),
|
|
"run did not succeed in time:\n{final_status}"
|
|
);
|
|
// Both steps ran (validate always; charge because when was true).
|
|
assert!(
|
|
final_status.contains('v') && final_status.contains('c'),
|
|
"expected both steps in the status detail:\n{final_status}"
|
|
);
|
|
}
|