docs+test(hierarchies): end-to-end review follow-ups (M5/M6/M7)

Findings from a 3-lens end-to-end review (correctness, security, tests).
Security review came back clean (cross-repo authz sound, isolation intact,
M5 gate sound modulo the documented manifest-trust boundary). No behavior
changes here — only accuracy + coverage:

- apply_service Phase B2: correct the descendant-expansion comment. The chain
  is COMMITTED ancestry (ancestors() reads the pool), so a reparent in the same
  apply takes effect next apply — same "one more apply" shape the in-tree path
  and group-create reparenting already have. Authz stays sound: the gate and the
  expansion consume the SAME chain (checked == written).
- doc §4.5: document the two known "one more apply / no data risk" limitations
  — reparent-in-same-apply template resolution, and the `{env}` source split
  (declared apps use --env; cross-repo descendants use apps.environment).
- approval journey: give the app real (`[vars]`) content so the editor member's
  AppVarsWrite is actually exercised — the admin-gate test now proves approval
  is ABOVE editor-write, not just "any non-admin is refused". (Vars cascade with
  the app; scripts are ON DELETE RESTRICT and would break AppGuard teardown.)
- templates journey: assert descendant expansions are idempotent (stable row
  ids on re-apply — no churn), matching the in-tree guarantee.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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
+            // written into it. The gate and the expansion below consume the SAME
-            // checked set is by construction the written set — no pre-tx TOCTOU
+            // `chain`, so the checked set is by construction the written set — no
-            // and a reparent-into-a-templated-group can't slip the gate. Sound via
+            // pre-tx TOCTOU, the per-app cap can't be slipped. Sound via the
-            // the hierarchy-aware effective_app_role: a group admin/editor is
+            // 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

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();

crates/picloud-cli/tests/templates.rs

@@ -247,6 +247,21 @@ fn expansion_paths(paths: &[String]) -> Vec<String> {
     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}")]),
+        expansion_paths(&want),
-        vec![format!("/x/{c}"), format!("/x/{d}")],
+        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();

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`