diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 4a5ca49..9723464 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -1615,12 +1615,20 @@ impl ApplyService { .iter() .map(|g| g.id) .collect(); + // The chain is the descendant's COMMITTED ancestry (`ancestors` reads + // the pool, not this tx). A reparent performed in THIS same apply is + // therefore not yet reflected, so a template gained/lost via that + // reparent fans out on the NEXT apply — the same "reparent takes one + // more apply" limitation the in-tree path has (prepare_tree also + // computes app chains pre-tx) and that group-create reparenting + // already documents above (§4.5 known limitation). + // // 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 + // written into it. The gate and the expansion below consume the SAME + // `chain`, so the checked set is by construction the written set — no + // pre-tx TOCTOU, the per-app cap can't be slipped. 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 diff --git a/crates/picloud-cli/tests/approval.rs b/crates/picloud-cli/tests/approval.rs index f26604e..aab1c4b 100644 --- a/crates/picloud-cli/tests/approval.rs +++ b/crates/picloud-cli/tests/approval.rs @@ -20,12 +20,18 @@ use crate::common::member; /// `production` is confirm-required, `staging` is not. fn project_dir(app: &str) -> TempDir { let dir = TempDir::new().expect("tempdir"); + // A `[vars]` entry so the app bundle has write-requiring content: an `editor` + // member must hold (and exercise) AppVarsWrite to pass authz_tree — which + // makes the admin-gate test prove the approval gate is ABOVE editor-write, + // not merely "any non-admin is refused". (Vars cascade-delete with the app, + // unlike scripts which are ON DELETE RESTRICT, so AppGuard teardown is clean.) fs::write( dir.path().join("picloud.toml"), format!( "[app]\nslug = \"{app}\"\nname = \"Gated App\"\n\n\ [[project.environments]]\nname = \"production\"\nconfirm = true\n\n\ - [[project.environments]]\nname = \"staging\"\nconfirm = false\n" + [[project.environments]]\nname = \"staging\"\nconfirm = false\n\n\ + [vars]\nregion = \"eu\"\n" ), ) .unwrap(); diff --git a/crates/picloud-cli/tests/templates.rs b/crates/picloud-cli/tests/templates.rs index c1fc129..57c6f37 100644 --- a/crates/picloud-cli/tests/templates.rs +++ b/crates/picloud-cli/tests/templates.rs @@ -247,6 +247,21 @@ fn expansion_paths(paths: &[String]) -> Vec { rows.iter().map(|r| r.get(0)).collect() } +/// `(path, id)` of template-expanded routes among `paths` — for asserting an +/// expansion is STABLE (same row id) across a re-apply, i.e. not churned. +fn expansion_rows_for(paths: &[String]) -> Vec<(String, String)> { + 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, id::text FROM routes \ + WHERE from_template IS NOT NULL AND path = ANY($1) ORDER BY path", + &[&paths], + ) + .expect("query expansion rows"); + rows.iter().map(|r| (r.get(0), r.get(1))).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 @@ -338,12 +353,28 @@ fn route_template_reaches_out_of_tree_descendant() { .arg("--force") .assert() .success(); + let want = [format!("/x/{c}"), format!("/x/{d}")]; assert_eq!( - expansion_paths(&[format!("/x/{c}"), format!("/x/{d}")]), - vec![format!("/x/{c}"), format!("/x/{d}")], + expansion_paths(&want), + want.to_vec(), "out-of-tree descendants at depth 1 AND 2 receive expansions" ); + // Idempotent re-apply: descendants must not churn (same row ids — provenance + // by `from_template`, not delete+recreate), same as the in-tree path. + let before = expansion_rows_for(&want); + common::pic_as(&env) + .args(["apply", "--dir"]) + .arg(dir.path()) + .arg("--force") + .assert() + .success(); + assert_eq!( + before, + expansion_rows_for(&want), + "re-apply must not churn descendant expansions (stable ids)" + ); + // 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(); diff --git a/docs/design/groups-and-project-tool.md b/docs/design/groups-and-project-tool.md index e2ad841..c80e08e 100644 --- a/docs/design/groups-and-project-tool.md +++ b/docs/design/groups-and-project-tool.md @@ -413,6 +413,16 @@ Two distinct constraints: > 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 > re-apply, mirroring the existing trigger-identity limitation (recreate to change those). +> +> **Known limitations (both "one more apply", no data risk):** (1) Template fan-out resolves against a +> node's **committed** ancestry — `apply` computes app chains before the transaction's structural +> reconcile — so a template gained or lost by a **reparent performed in the *same* apply** takes effect +> on the next apply (the same shape as "reparenting into a freshly-created group needs a second apply"). +> (2) The `{env}` placeholder resolves to the apply's `--env` for **declared** apps but to the app's own +> `apps.environment` column for **cross-repo descendants**; if a template uses `{env}` and those two +> disagree, that descendant's expansion can churn between applies (and a descendant with a NULL +> `environment` + an `{env}` template aborts the apply, naming the app). Set the descendant's environment, +> or avoid `{env}` in templates fanned across heterogeneous-env subtrees. ### 4.6 Secrets & `pull`