From 99e7100f0bf238735301c982a9ce58958ca75ddf Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Thu, 25 Jun 2026 21:11:59 +0200 Subject: [PATCH] fix(apply): resolve inherited-bound triggers in diff, state-token, and prune MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of Phase 4-lite found two real gaps, both rooted in the apply engine resolving script `id → name` from `current.scripts` (app-own) only — so a route/trigger bound to an inherited GROUP script (whose id is not app-own) couldn't be named: * **Prune gap (reachable via pure declarative apply):** a trigger bound to an inherited group script, once dropped from the manifest, never diffed to a Delete (the diff couldn't resolve its identity once the bundle stopped referencing the name), so `apply --prune` silently orphaned it. * **Bound-plan false-negative:** `state_token` excluded such a trigger from its fingerprint, so the StateMoved check missed a concurrent edit to that binding class. (Routes were already safe — they hash the raw `script_id`.) Fix: `resolve_current_target_names` resolves the names of group scripts the app's CURRENT routes/triggers are bound to (by id, independent of the bundle), and that map now feeds `compute_diff_with_names`, `state_token_with_names`, and the prune trigger-identity map. App-owned bindings are unaffected (their names are already in `current.scripts`). Also fixes a stale "in this app" conflict message for group-owned scripts. New journey `inherited_bound_trigger_is_pruned_when_dropped` proves prune now removes an inherited-bound cron trigger. Full journey suite 105/105. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 84 +++++++++++++++++++---- crates/manager-core/src/repo.rs | 2 +- crates/picloud-cli/tests/group_scripts.rs | 84 +++++++++++++++++++++++ 3 files changed, 154 insertions(+), 16 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index a38cefa..347f561 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -423,12 +423,13 @@ impl ApplyService { self.validate_bundle(bundle, &keys_set(&inherited))?; self.validate_route_hosts(app_id, bundle).await?; let current = self.load_current(app_id).await?; + let current_names = self.resolve_current_target_names(¤t).await?; validate_email_secrets_present(bundle, ¤t.secret_names)?; Ok(PlanResult { - plan: compute_diff_with_inherited(¤t, bundle, &inherited), + plan: compute_diff_with_names(¤t, bundle, ¤t_names), // Fingerprint of the live state this plan was computed against, so // `apply` can refuse if the app changed underneath it (§4.2). - state_token: state_token(¤t), + state_token: state_token_with_names(¤t, ¤t_names), }) } @@ -471,6 +472,9 @@ impl ApplyService { .map_err(|e| ApplyError::Backend(e.to_string()))?; let current = self.load_current(app_id).await?; + // Phase 4: names of group scripts the current routes/triggers bind to, + // so the token / diff / prune resolve inherited bindings (see method). + let current_names = self.resolve_current_target_names(¤t).await?; // Bound-plan check (§4.2): if the caller passed the token from a prior // `pic plan`, refuse when the live state changed since. Computed from // the same `load_current` snapshot the diff uses, before any mutation @@ -480,14 +484,14 @@ impl ApplyService { // apply-vs-interactive-write window — it is not a substitute for the // tx-scoped read the follow-up will add. if let Some(expected) = expected_token { - if state_token(¤t) != expected { + if state_token_with_names(¤t, ¤t_names) != expected { return Err(ApplyError::StateMoved); } } // Surface a missing email-secret reference here so `plan` and `apply` // agree, rather than only failing deep in `resolve_and_seal` below. validate_email_secrets_present(bundle, ¤t.secret_names)?; - let plan = compute_diff_with_inherited(¤t, bundle, &inherited); + let plan = compute_diff_with_names(¤t, bundle, ¤t_names); let mut report = ApplyReport::default(); // §4.7 warning: an enabled binding pointing at a disabled script is // deployed-but-unreachable. Not an error (it's valid desired state), @@ -700,10 +704,14 @@ impl ApplyService { // Secret pruning is deliberately deferred (destructive + irreversible): // stale secrets are surfaced in the plan but never deleted here. if prune { + // Include inherited group-script bindings (Phase 4) so a trigger + // bound to a group script resolves its identity and is pruned when + // dropped from the manifest — not silently orphaned. let id_to_name: HashMap = current .scripts .iter() .map(|s| (s.id, s.name.clone())) + .chain(current_names.iter().map(|(id, n)| (*id, n.clone()))) .collect(); let trigger_id_by_identity: HashMap = current .triggers @@ -981,6 +989,38 @@ impl ApplyService { Ok(out) } + /// Phase 4: resolve the names of group-owned scripts the app's CURRENT + /// routes/triggers are bound to, `script_id → name`. These ids aren't in + /// `current.scripts` (app-own only), so this is what lets the diff / + /// state-token / prune resolve an existing inherited binding back to its + /// name — independent of whether the bundle still references it (so a + /// dropped inherited binding still diffs to a Delete, and an inherited-bound + /// trigger still moves the state token). + async fn resolve_current_target_names( + &self, + current: &CurrentState, + ) -> Result, ApplyError> { + let own: HashSet = current.scripts.iter().map(|s| s.id).collect(); + let mut ids: HashSet = HashSet::new(); + for r in ¤t.routes { + if !own.contains(&r.script_id) { + ids.insert(r.script_id); + } + } + for t in ¤t.triggers { + if !own.contains(&t.script_id) { + ids.insert(t.script_id); + } + } + let mut out = HashMap::new(); + for id in ids { + if let Some(s) = self.scripts.get(id).await.map_err(map_repo)? { + out.insert(id, s.name); + } + } + Ok(out) + } + /// `inherited_endpoints` (lowercased names) are group-owned endpoint /// scripts reachable from the app's chain that a route/trigger may bind to /// even though the manifest doesn't declare them (Phase 4). They count as @@ -1175,26 +1215,28 @@ impl ApplyService { /// Diff desired `bundle` against `current`. Pure — unit-tested directly. #[must_use] pub fn compute_diff(current: &CurrentState, bundle: &Bundle) -> Plan { - compute_diff_with_inherited(current, bundle, &HashMap::new()) + compute_diff_with_names(current, bundle, &HashMap::new()) } -/// `compute_diff` with the set of inherited group-endpoint targets (Phase 4), -/// `name(lowercased) → script_id`. Those ids are added to the script-name map -/// so a route/trigger bound to an inherited group script resolves to its name -/// and re-apply is idempotent (otherwise the unknown id reads as a rebind -/// every plan). The public `compute_diff` passes an empty map (no inheritance). -fn compute_diff_with_inherited( +/// `compute_diff` with `current_names` — the names of group-owned (inherited) +/// scripts the app's CURRENT routes/triggers are bound to, `script_id → name` +/// (Phase 4). Those ids aren't in `current.scripts` (app-own only), so without +/// them a current binding to a group script resolves to no name: it reads as a +/// perpetual rebind, and a binding dropped from the manifest never diffs to a +/// Delete (so `--prune` can't remove it). The public `compute_diff` passes an +/// empty map (no inheritance). +fn compute_diff_with_names( current: &CurrentState, bundle: &Bundle, - inherited: &HashMap, + current_names: &HashMap, ) -> Plan { let script_name_by_id: HashMap = current .scripts .iter() .map(|s| (s.id, s.name.clone())) - // Inherited group targets, inverted to id → name. Disjoint from - // app-own ids by construction, so order doesn't matter. - .chain(inherited.iter().map(|(name, id)| (*id, name.clone()))) + // Inherited group bindings' ids → names. Disjoint from app-own ids by + // construction, so order doesn't matter. + .chain(current_names.iter().map(|(id, n)| (*id, n.clone()))) .collect(); Plan { @@ -1991,6 +2033,17 @@ fn map_trig(e: TriggerRepoError) -> ApplyError { /// is the same risk class as not checking at all — never a false refusal. #[must_use] pub fn state_token(current: &CurrentState) -> String { + state_token_with_names(current, &HashMap::new()) +} + +/// `state_token` with `current_names` (group-bound binding ids → names, Phase +/// 4) so a trigger bound to an inherited group script contributes to the +/// fingerprint. Without it such a trigger is silently excluded and the +/// bound-plan (StateMoved) check has a false-negative for that binding class. +fn state_token_with_names( + current: &CurrentState, + current_names: &HashMap, +) -> String { let mut parts: Vec = Vec::with_capacity( current.scripts.len() + current.routes.len() @@ -2031,6 +2084,7 @@ pub fn state_token(current: &CurrentState) -> String { .scripts .iter() .map(|s| (s.id, s.name.clone())) + .chain(current_names.iter().map(|(id, n)| (*id, n.clone()))) .collect(); for t in ¤t.triggers { if let Some(id) = current_trigger_identity(t, &script_name_by_id) { diff --git a/crates/manager-core/src/repo.rs b/crates/manager-core/src/repo.rs index 4d37517..c247412 100644 --- a/crates/manager-core/src/repo.rs +++ b/crates/manager-core/src/repo.rs @@ -591,7 +591,7 @@ pub(crate) async fn update_script_tx( Ok(None) => return Err(ScriptRepositoryError::NotFound(id)), Err(sqlx::Error::Database(e)) if e.is_unique_violation() => { return Err(ScriptRepositoryError::Conflict( - "a script with that name already exists in this app".into(), + "a script with that name already exists in this owner".into(), )); } Err(e) => return Err(e.into()), diff --git a/crates/picloud-cli/tests/group_scripts.rs b/crates/picloud-cli/tests/group_scripts.rs index 97eae5a..339553d 100644 --- a/crates/picloud-cli/tests/group_scripts.rs +++ b/crates/picloud-cli/tests/group_scripts.rs @@ -196,6 +196,90 @@ fn manifest_binds_route_to_inherited_group_script_idempotently() { ); } +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn inherited_bound_trigger_is_pruned_when_dropped() { + // Regression: a trigger bound to an inherited group script must still + // resolve its identity in the diff/prune path even after the manifest + // stops referencing it — otherwise `--prune` silently orphans it. + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("gst-grp"); + let child = common::unique_slug("gst-child"); + + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + + let dir = manifest_dir(); + fs::write(dir.path().join("scripts/work.rhai"), "\"ok\"").unwrap(); + common::pic_as(&env) + .args(["scripts", "deploy"]) + .arg(dir.path().join("scripts/work.rhai")) + .args(["--group", &group, "--name", "work"]) + .assert() + .success(); + let gscript_id = group_script_id(&env, &group); + let _gs = ScriptGuard::new(&env.url, &env.token, &gscript_id); + + let _child = AppGuard::new(&env.url, &env.token, &child); + common::pic_as(&env) + .args(["apps", "create", &child, "--group", &group]) + .assert() + .success(); + + let manifest_path = dir.path().join("picloud.toml"); + // 1. Manifest binds a cron trigger to the INHERITED `work`. + let with_trigger = format!( + "[app]\nslug = \"{child}\"\nname = \"GST\"\n\n\ + [[triggers.cron]]\nscript = \"work\"\nschedule = \"0 0 * * * *\"\ntimezone = \"UTC\"\n" + ); + fs::write(&manifest_path, &with_trigger).unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(&manifest_path) + .assert() + .success(); + let listed = String::from_utf8( + common::pic_as(&env) + .args(["triggers", "ls", "--app", &child]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + assert!( + listed.contains("cron"), + "cron trigger bound to inherited `work` should exist:\n{listed}" + ); + + // 2. Drop the trigger + `--prune` → it must be removed (not orphaned). + let empty = format!("[app]\nslug = \"{child}\"\nname = \"GST\"\n"); + fs::write(&manifest_path, &empty).unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(&manifest_path) + .args(["--prune", "--yes"]) + .assert() + .success(); + let after = String::from_utf8( + common::pic_as(&env) + .args(["triggers", "ls", "--app", &child]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + assert!( + !after.contains("cron"), + "prune must remove the inherited-bound cron trigger:\n{after}" + ); +} + /// First script id from `pic scripts ls --group `. fn group_script_id(env: &common::TestEnv, group: &str) -> String { let ls = common::pic_as(env)