diff --git a/crates/manager-core/src/apply_api.rs b/crates/manager-core/src/apply_api.rs index b7bace2..2e7789f 100644 --- a/crates/manager-core/src/apply_api.rs +++ b/crates/manager-core/src/apply_api.rs @@ -428,6 +428,13 @@ async fn authz_tree( } } } + + // NOTE (§4.5, M7): templates also fan out to descendant apps NOT declared in + // this bundle (the cross-repo subtree). Those per-recipient write caps are + // gated AUTHORITATIVELY IN-TRANSACTION in `apply_tree` Phase B2 — against the + // post-reparent app set, each app already locked — so the checked set is by + // construction the written set (no pre-tx TOCTOU, and a reparent that moves + // apps under a templated group in the same apply can't slip the gate). Ok(()) } diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 68da18f..64c5e61 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -1329,6 +1329,22 @@ impl ApplyService { // Lock every node's apply key, in sorted order, so concurrent applies // touching any shared node serialize and the ordering can't deadlock. let mut lock_keys: Vec = prepared.iter().map(|p| apply_lock_key(p.owner)).collect(); + // Also lock every descendant app of an in-tree group up front (M7): + // Phase B2 expands templates into them. Their union is invariant under + // reparenting in-tree groups (a reparent changes a group's ancestors, + // not its descendants; created groups are empty), so this pool-computed + // set equals the post-Phase-0 set Phase B2 touches — one sorted batch, + // so there is no out-of-order mid-tx locking and no deadlock. + for p in &prepared { + if let ApplyOwner::Group(gid) = p.owner { + for aid in crate::group_repo::descendant_app_ids(&self.pool, gid) + .await + .map_err(map_group_err)? + { + lock_keys.push(apply_lock_key(ApplyOwner::App(aid))); + } + } + } lock_keys.sort_unstable(); lock_keys.dedup(); for k in lock_keys { @@ -1471,7 +1487,6 @@ impl ApplyService { // routes are reconciled (so a template can bind the app's own // script and collide-check against hand-declared routes). if let ApplyOwner::App(app_id) = p.owner { - let hand_declared_keys = bundle_route_keys(&p.node.bundle); self.expand_route_templates_tx( &mut tx, app_id, @@ -1480,11 +1495,9 @@ impl ApplyService { &p.app_chain, &name_to_id, &group_script_index, - &hand_declared_keys, &mut report, ) .await?; - let hand_trigger_ids = bundle_trigger_identities(&p.node.bundle); self.expand_trigger_templates_tx( &mut tx, app_id, @@ -1494,7 +1507,6 @@ impl ApplyService { &p.app_chain, &name_to_id, &group_script_index, - &hand_trigger_ids, &mut report, ) .await?; @@ -1502,6 +1514,127 @@ impl ApplyService { } } + // Phase B2 — cross-repo descendant expansion (§4.5, M7): a template at a + // group in this apply fans out to EVERY descendant app in the DB subtree, + // not only the app nodes present in the tree. Gather the descendants of + // every in-tree group, drop the ones already expanded in Phase B, and + // expand each remaining app once (app_id-ordered, deterministic locking). + // Each `expand_*` reads the app's own chain/vars/hand-declared rows from + // the tx, so a removed template reaps its expansions on these apps too. + let in_tree_apps: HashSet = prepared + .iter() + .filter_map(|p| match p.owner { + ApplyOwner::App(a) => Some(a), + ApplyOwner::Group(_) => None, + }) + .collect(); + let mut descendant_apps: BTreeSet = BTreeSet::new(); + for p in &prepared { + if let ApplyOwner::Group(gid) = p.owner { + for aid in crate::group_repo::descendant_app_ids_tx(&mut tx, gid) + .await + .map_err(map_group_err)? + { + if !in_tree_apps.contains(&aid) { + descendant_apps.insert(aid); + } + } + } + } + for app_id in descendant_apps { + any_app = true; + // Already locked up front (in the initial sorted batch). Slug + own + // environment (for `{env}`) + parent group (for the chain). + let (slug, app_env, group_id): (String, Option, Uuid) = + sqlx::query_as("SELECT slug, environment, group_id FROM apps WHERE id = $1") + .bind(app_id.into_inner()) + .fetch_one(&mut *tx) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))?; + let chain: Vec = self + .groups + .ancestors(group_id.into()) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))? + .iter() + .map(|g| g.id) + .collect(); + // Authoritative IN-TX per-recipient authz (§7.4, M7): the actor needs + // the matching write cap on THIS descendant before any expansion is + // written into it. Checked here (post-reparent, already locked) so the + // checked set is by construction the written set — no pre-tx TOCTOU + // and a reparent-into-a-templated-group can't slip the gate. Sound via + // the hierarchy-aware effective_app_role: a group admin/editor is + // implicitly authorized on its subtree; a narrow principal is refused. + let route_tmpls = crate::template_repo::list_for_groups_tx(&mut tx, &chain) + .await + .map_err(map_group_err)?; + let trig_tmpls = crate::template_repo::list_triggers_for_groups_tx(&mut tx, &chain) + .await + .map_err(map_group_err)?; + if !route_tmpls.is_empty() { + require( + self.authz.as_ref(), + principal, + Capability::AppWriteRoute(app_id), + ) + .await + .map_err(map_authz_denied)?; + } + if !trig_tmpls.is_empty() { + require( + self.authz.as_ref(), + principal, + Capability::AppManageTriggers(app_id), + ) + .await + .map_err(map_authz_denied)?; + // Email-trigger expansion resolves + seals the app's secret — + // same AppSecretsRead gate a hand-declared email trigger needs. + if trig_tmpls.iter().any(|t| { + t.spec.get("kind").and_then(serde_json::Value::as_str) == Some("email") + }) { + require( + self.authz.as_ref(), + principal, + Capability::AppSecretsRead(app_id), + ) + .await + .map_err(map_authz_denied)?; + } + } + // No own/in-tx group scripts for a descendant not in this apply: its + // template scripts resolve via in-tx group scripts (group_script_index) + // then committed inherited scripts (resolve_template_script fallback). + // Wrap errors with the descendant slug so e.g. an `{env}` template on a + // NULL-environment descendant names the offending app, not just "{env}". + self.expand_route_templates_tx( + &mut tx, + app_id, + &slug, + app_env.as_deref(), + &chain, + &HashMap::new(), + &group_script_index, + &mut report, + ) + .await + .map_err(|e| with_descendant_ctx(e, &slug))?; + self.expand_trigger_templates_tx( + &mut tx, + app_id, + &slug, + app_env.as_deref(), + actor, + &chain, + &HashMap::new(), + &group_script_index, + &mut report, + ) + .await + .map_err(|e| with_descendant_ctx(e, &slug))?; + } + // Phase C — structural prune (§7, M3): with `--prune`, delete groups // THIS project owns that the manifest no longer declares, leaf-first // (delete_tx is RESTRICT, so a group still holding apps/child-groups @@ -1519,18 +1652,6 @@ impl ApplyService { } } - // §4.5 M4a scope: expansion reaping only runs for app nodes IN this - // apply. A deleted template's expansions in descendant apps NOT in this - // tree survive until those apps are re-applied — warn so the operator - // isn't surprised that a pruned template left live routes elsewhere. - if report.route_templates_deleted > 0 || report.trigger_templates_deleted > 0 { - report.warnings.push( - "deleted template(s): expansions in descendant apps not included in this apply \ - remain until those apps are re-applied (`pic apply --dir` over them)" - .into(), - ); - } - tx.commit() .await .map_err(|e| ApplyError::Backend(e.to_string()))?; @@ -1966,10 +2087,12 @@ impl ApplyService { !n.bundle.route_templates.is_empty() || !n.bundle.trigger_templates.is_empty(); if n.kind == NodeKind::Group && has_templates { if let Some(gid) = group_id_by_slug.get(&n.slug.to_lowercase()).copied() { - let affected_apps = prepared - .iter() - .filter(|p| p.app_chain.contains(&gid)) - .count(); + // True blast radius (§4.2, M7): every descendant app in the DB + // subtree, not just the app nodes present in this apply. + let affected_apps = crate::group_repo::descendant_app_ids(&self.pool, gid) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))? + .len(); template_blast_radius.push(TemplateBlastRadius { group: n.slug.clone(), affected_apps, @@ -2157,7 +2280,6 @@ impl ApplyService { app_chain: &[GroupId], own_name_to_id: &HashMap, group_script_index: &HashMap>, - hand_declared_keys: &HashSet, report: &mut ApplyReport, ) -> Result<(), ApplyError> { // Visible templates: nearest-wins by name across the ancestor chain. @@ -2194,6 +2316,12 @@ impl ApplyService { crate::config_resolver::resolve(cands).0 }; + // Hand-declared routes (`from_template IS NULL`) of THIS app, read in-tx + // so an in-tree app's just-reconciled routes are included. A template + // expanding onto one of these is a hard collision — and for a descendant + // app not in this apply, these are its committed routes. + let hand_declared_keys = self.load_hand_declared_route_keys_tx(tx, app_id).await?; + // Build the desired expansion set (identity → resolved route + source). let mut desired: HashMap = HashMap::new(); for t in by_name.values() { @@ -2347,6 +2475,27 @@ impl ApplyService { Ok(rows.into_iter().map(|r| (r.identity(), r)).collect()) } + /// Route identities the app declares BY HAND (`from_template IS NULL`), read + /// in-tx. A template expansion colliding with one of these is rejected. For + /// an in-tree app this reflects its just-reconciled routes; for a descendant + /// not in the apply, its committed routes. + async fn load_hand_declared_route_keys_tx( + &self, + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + app_id: AppId, + ) -> Result, ApplyError> { + let rows: Vec = sqlx::query_as( + "SELECT id, script_id, host_kind, host, host_param_name, path_kind, path, \ + method, dispatch_mode, enabled \ + FROM routes WHERE app_id = $1 AND from_template IS NULL", + ) + .bind(app_id.into_inner()) + .fetch_all(&mut **tx) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))?; + Ok(rows.into_iter().map(|r| r.identity()).collect()) + } + /// Expand the TRIGGER templates visible to one descendant app (§4.5, M4b), /// the trigger analogue of `expand_route_templates_tx`. Each chain template /// fans into one concrete trigger, keyed by `from_template` (one trigger per @@ -2365,7 +2514,6 @@ impl ApplyService { app_chain: &[GroupId], own_name_to_id: &HashMap, group_script_index: &HashMap>, - hand_declared_trigger_ids: &HashSet, report: &mut ApplyReport, ) -> Result<(), ApplyError> { let templates = crate::template_repo::list_triggers_for_groups_tx(tx, app_chain) @@ -2392,6 +2540,17 @@ impl ApplyService { .iter() .map(|(n, id)| (*id, n.clone())) .collect(); + // App-own scripts too, so a hand-declared trigger bound to the app's own + // script (a descendant app's committed scripts aren't in own_name_to_id) + // still resolves an identity for the collision check below. + for s in self + .scripts + .list_for_app(app_id) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))? + { + id_to_name.entry(s.id).or_insert_with(|| s.name.clone()); + } for gid in app_chain { for s in self .scripts @@ -2421,11 +2580,18 @@ impl ApplyService { .list_for_app(app_id) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; - // template_id → (trigger_id, current semantic identity) + // template_id → (trigger_id, current semantic identity); and the + // identities the app declares BY HAND (every trigger NOT sourced from a + // template), so an expansion colliding with one is rejected. For an + // in-tree app these are its just-reconciled triggers; for a descendant + // not in this apply, its committed triggers. let mut existing: HashMap)> = HashMap::new(); + let mut hand_declared_trigger_ids: HashSet = HashSet::new(); for trg in &all_triggers { if let Some(tmpl) = by_trigger_id.get(&trg.id) { existing.insert(*tmpl, (trg.id, current_trigger_identity(trg, &id_to_name))); + } else if let Some(id) = current_trigger_identity(trg, &id_to_name) { + hand_declared_trigger_ids.insert(id); } } @@ -4203,34 +4369,6 @@ fn current_trigger_identity(t: &Trigger, name_by_id: &HashMap) } } -/// The semantic identities of a bundle's HAND-declared triggers — the set a -/// trigger-template expansion is collision-checked against (§4.5, M4b). -fn bundle_trigger_identities(bundle: &Bundle) -> HashSet { - bundle - .triggers - .iter() - .map(BundleTrigger::identity) - .collect() -} - -/// The route-identity keys of a bundle's HAND-declared routes — the set a -/// template expansion is collision-checked against (§4.5, M4a). -fn bundle_route_keys(bundle: &Bundle) -> HashSet { - bundle - .routes - .iter() - .map(|r| { - route_key( - r.method.as_deref(), - r.host_kind, - &r.host, - r.path_kind, - &r.path, - ) - }) - .collect() -} - fn route_key( method: Option<&str>, host_kind: HostKind, @@ -4681,6 +4819,20 @@ fn map_authz_denied(e: AuthzDenied) -> ApplyError { } } +/// Tag a template-expansion error from a cross-repo DESCENDANT (an app not +/// declared in this apply) with the app slug, so e.g. an `{env}` template on a +/// NULL-environment descendant points at the offending app rather than reading +/// like a missing `--env` flag on the apply itself. Atomicity is preserved (the +/// error still aborts the whole apply); this only improves diagnosability. +fn with_descendant_ctx(e: ApplyError, slug: &str) -> ApplyError { + match e { + ApplyError::Invalid(m) => { + ApplyError::Invalid(format!("descendant app `{slug}` (not in this apply): {m}")) + } + other => other, + } +} + fn map_group_err(e: crate::group_repo::GroupRepositoryError) -> ApplyError { use crate::group_repo::GroupRepositoryError as E; // Conflict (cycle, slug clash, non-empty delete) and not-found are caller diff --git a/crates/manager-core/src/group_repo.rs b/crates/manager-core/src/group_repo.rs index 8d120c4..da4b2ef 100644 --- a/crates/manager-core/src/group_repo.rs +++ b/crates/manager-core/src/group_repo.rs @@ -14,7 +14,7 @@ //! future CLI/orchestrator can detect structural drift (§6). use async_trait::async_trait; -use picloud_shared::{Group, GroupId}; +use picloud_shared::{AppId, Group, GroupId}; use sqlx::PgPool; use uuid::Uuid; @@ -523,6 +523,52 @@ pub(crate) async fn list_owned_groups_tx( .collect()) } +/// Recursive descendant-app query, the inverse of the ancestor `CHAIN_LEVELS_CTE` +/// in `config_resolver`: walk `groups.parent_id` DOWN from `$1` to collect the +/// whole subtree of group ids, then every app whose `group_id` falls in it. +/// Ordered by `app_id` so callers lock/iterate deterministically. Depth-bounded +/// `< 64` as a runaway guard (the cycle guard already forbids real cycles). +const DESCENDANT_APPS_SQL: &str = "\ + WITH RECURSIVE subtree AS ( \ + SELECT id, 0 AS depth FROM groups WHERE id = $1 \ + UNION ALL \ + SELECT g.id, s.depth + 1 FROM groups g JOIN subtree s ON g.parent_id = s.id \ + WHERE s.depth < 64 \ + ) \ + SELECT a.id FROM apps a WHERE a.group_id IN (SELECT id FROM subtree) ORDER BY a.id"; + +/// Every app in the subtree rooted at `group_id` (the group itself and all +/// descendant groups), `app_id`-ordered. The read-only pool variant — used by +/// `plan` to size a template's true blast radius across the whole DB subtree, +/// not just the apps present in the current apply. +/// +/// # Errors +/// Propagates sqlx errors. +pub async fn descendant_app_ids( + pool: &PgPool, + group_id: GroupId, +) -> Result, GroupRepositoryError> { + let rows: Vec<(Uuid,)> = sqlx::query_as(DESCENDANT_APPS_SQL) + .bind(group_id.into_inner()) + .fetch_all(pool) + .await?; + Ok(rows.into_iter().map(|(id,)| id.into()).collect()) +} + +/// The same subtree enumeration inside the apply tx, so groups/apps created +/// earlier in this transaction (M2 structural reconcile) are included when +/// fanning a template out to its descendants (§4.5, M7). +pub(crate) async fn descendant_app_ids_tx( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + group_id: GroupId, +) -> Result, GroupRepositoryError> { + let rows: Vec<(Uuid,)> = sqlx::query_as(DESCENDANT_APPS_SQL) + .bind(group_id.into_inner()) + .fetch_all(&mut **tx) + .await?; + Ok(rows.into_iter().map(|(id,)| id.into()).collect()) +} + #[derive(sqlx::FromRow)] struct GroupRow { id: Uuid, diff --git a/crates/picloud-cli/tests/templates.rs b/crates/picloud-cli/tests/templates.rs index d503b01..c1fc129 100644 --- a/crates/picloud-cli/tests/templates.rs +++ b/crates/picloud-cli/tests/templates.rs @@ -231,6 +231,134 @@ fn route_template_var_resolves_in_same_apply() { ); } +/// Template-expanded routes (`from_template IS NOT NULL`) whose path is one of +/// `paths`, as `path` strings — for asserting which descendant apps received an +/// expansion regardless of which apply declared them. +fn expansion_paths(paths: &[String]) -> Vec { + let url = std::env::var("DATABASE_URL").expect("DATABASE_URL"); + let mut pg = PgClient::connect(&url, NoTls).expect("pg connect"); + let rows = pg + .query( + "SELECT path FROM routes \ + WHERE from_template IS NOT NULL AND path = ANY($1) ORDER BY path", + &[&paths], + ) + .expect("query expansion paths"); + rows.iter().map(|r| r.get(0)).collect() +} + +/// M7 (§4.5): a route template fans out to EVERY descendant app in the DB +/// subtree, not only the app nodes present in the apply. An app created under +/// the group out-of-band (absent from the manifest) still receives the +/// expansion on the next apply, and removing the template reaps it there too. +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn route_template_reaches_out_of_tree_descendant() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("xrepo-g"); + let sg = common::unique_slug("xrepo-sg"); // subgroup (depth-2) + let a = common::unique_slug("xrepo-a"); + let c = common::unique_slug("xrepo-c"); // out-of-tree descendant (depth-1) + let d = common::unique_slug("xrepo-d"); // out-of-tree descendant (depth-2) + + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + let _aa = AppGuard::new(&env.url, &env.token, &a); + common::pic_as(&env) + .args(["apps", "create", &a, "--group", &group]) + .assert() + .success(); + + // Apply a tree of just (group ← app a) with an `/x/{app_slug}` template. + let dir = TempDir::new().expect("tempdir"); + fs::create_dir_all(dir.path().join("scripts")).unwrap(); + fs::write(dir.path().join("scripts/shared.rhai"), r#""hi""#).unwrap(); + let group_toml = |with_template: bool| { + let tmpl = if with_template { + "\n[[route_templates]]\nname = \"x\"\nscript = \"shared\"\n\ + host_kind = \"any\"\npath_kind = \"exact\"\npath = \"/x/{app_slug}\"\n" + } else { + "" + }; + format!( + "[group]\nslug = \"{group}\"\nname = \"G\"\n\n\ + [[scripts]]\nname = \"shared\"\nfile = \"scripts/shared.rhai\"\n{tmpl}" + ) + }; + fs::write(dir.path().join("picloud.toml"), group_toml(true)).unwrap(); + fs::create_dir_all(dir.path().join(&a)).unwrap(); + fs::write( + dir.path().join(&a).join("picloud.toml"), + format!("[app]\nslug = \"{a}\"\nname = \"App\"\n"), + ) + .unwrap(); + + common::pic_as(&env) + .args(["apply", "--dir"]) + .arg(dir.path()) + .assert() + .success(); + let _gs = ScriptGuard::new(&env.url, &env.token, &group_script_id(&env, &group)); + assert_eq!( + expansion_paths(&[format!("/x/{a}")]).len(), + 1, + "in-tree app a gets its expansion" + ); + + // Create app c (direct child) and a nested subgroup ← app d (depth-2), + // all OUTSIDE the manifest tree, to exercise the recursive descendant CTE. + let _ac = AppGuard::new(&env.url, &env.token, &c); + common::pic_as(&env) + .args(["apps", "create", &c, "--group", &group]) + .assert() + .success(); + let _sg = GroupGuard::new(&env.url, &env.token, &sg); + common::pic_as(&env) + .args(["groups", "create", &sg, "--parent", &group]) + .assert() + .success(); + let _ad = AppGuard::new(&env.url, &env.token, &d); + common::pic_as(&env) + .args(["apps", "create", &d, "--group", &sg]) + .assert() + .success(); + + // Re-apply the same tree (still only group + a). M7: c (depth-1) AND d + // (depth-2, under the subgroup) both receive the expansion. `--force` + // waives the bound token (the out-of-band creates bumped structure version). + common::pic_as(&env) + .args(["apply", "--dir"]) + .arg(dir.path()) + .arg("--force") + .assert() + .success(); + assert_eq!( + expansion_paths(&[format!("/x/{c}"), format!("/x/{d}")]), + vec![format!("/x/{c}"), format!("/x/{d}")], + "out-of-tree descendants at depth 1 AND 2 receive expansions" + ); + + // Remove the template + prune: every descendant's expansion is reaped, + // in-tree or not, at any depth. + fs::write(dir.path().join("picloud.toml"), group_toml(false)).unwrap(); + common::pic_as(&env) + .args(["apply", "--dir"]) + .arg(dir.path()) + .args(["--prune", "--yes", "--force"]) + .assert() + .success(); + assert!( + expansion_paths(&[format!("/x/{a}"), format!("/x/{c}"), format!("/x/{d}")]).is_empty(), + "removing the template reaps expansions on ALL descendants, any depth" + ); +} + #[ignore = "needs DATABASE_URL pointing at a running Postgres"] #[test] fn expansion_colliding_with_hand_declared_route_is_rejected() { diff --git a/docs/design/groups-and-project-tool.md b/docs/design/groups-and-project-tool.md index b87b963..27d4005 100644 --- a/docs/design/groups-and-project-tool.md +++ b/docs/design/groups-and-project-tool.md @@ -402,9 +402,13 @@ Two distinct constraints: > expansion is validated with the same rules its hand-declared counterpart gets — routes: > reserved-path rejection + structural path/host parse + host-claim; triggers: the per-kind shape > check (cron schedule, queue timeout, …) — so a `{var:…}`-injected reserved path, unclaimed strict -> host, or malformed schedule/timeout can't slip in. **Scope:** expansion targets app NODES present in the tree apply -> (the common `pic apply --dir` case), not yet every descendant in another repo — deleting a template -> leaves expansions in out-of-apply descendants until they're re-applied (the apply warns). +> host, or malformed schedule/timeout can't slip in. **Scope:** expansion targets **every descendant app +> in the DB subtree** (M7, 2026-06-28), not only the app nodes present in the apply — a template change at +> group N fans out to `subtree(N)` across repos, and removing a template reaps its expansions on those +> descendants too (the apply locks each extra descendant in `app_id` order). The actor must hold the +> per-recipient write cap (`AppWriteRoute`/`AppManageTriggers`/`AppSecretsRead`) on each descendant; the +> hierarchy-aware `effective_app_role` makes a group admin/editor implicitly authorized on its subtree, so +> this is sound rather than an escalation, and a narrowly-scoped principal is refused (naming the app). > `{var:NAME}` resolves **in-transaction** (M6, 2026-06-28) — a var set in the *same* apply is visible to > the template immediately, so var + template converge in one apply. A > trigger template whose only change is a non-identity field (e.g. dispatch_mode) is a no-op on