Files
PiCloud/crates/picloud-cli/tests/workflows.rs
MechaCat02 eaf5ace30f fix(workflows): close review gaps — pull round-trip, dup-names, reclaim budget
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>
2026-07-12 18:56:44 +02:00

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}"
);
}