refactor(apply): split reconcile_group_structure_tx create/reparent branches

Review #4 (LOW, cleanup). `reconcile_group_structure_tx` carried a
`#[allow(clippy::too_many_lines)]` over a ~145-line body doing create +
reparent + attach/RBAC/lock in one loop. Extract the create branch
(`create_group_node_tx`) and the divergence-resolution branch
(`reparent_diverged_group_tx`) into helper methods, threading the shared
attach-ceiling / principal / mode context through a small `ReconcileCtx` — the
loop is now thin enough to drop the allow. Also improve the child-before-parent
error in `resolve_declared_parent` to hint that group nodes must be ordered
parents-first (only a hand-rolled bundle hits it; the CLI already sorts).

No behavior change. Documents the Tier-1 review follow-ups in the design doc.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

crates/manager-core/src/apply_service.rs

@@ -2022,7 +2022,9 @@ impl ApplyService {
             .map(Some)
             .ok_or_else(|| {
                 ApplyError::Invalid(format!(
-                    "group `{}` declares parent `{pslug}`, which does not exist",
+                    "group `{}` declares parent `{pslug}`, which does not exist — a parent \
+                     group must be declared before its children in the tree (the CLI orders \
+                     nodes parents-first by directory nesting)",
                     n.slug
                 ))
             })
@@ -2054,15 +2056,15 @@ impl ApplyService {
     }
 
     /// §6 M1+M2: reconcile the group tree SHAPE inside the apply tx (parents-
-    /// first). A group the server lacks is CREATED; a group whose server parent
+    /// first). A group the server lacks is CREATED (`create_group_node_tx`); a
-    /// differs from the manifest's declared parent is a structural divergence,
+    /// group whose server parent differs from the manifest's declared parent is
-    /// resolved per `mode` (refuse / reparent / adopt-server). Returns the
+    /// a structural divergence, resolved per `mode` (`reparent_diverged_group_tx`
-    /// complete slug → id map (so `prepare_tree` resolves the whole tree) and the
+    /// — refuse / reparent / adopt-server). Returns the complete slug → id map
-    /// set of groups STRUCTURALLY changed here (created or reparented) — the
+    /// (so `prepare_tree` resolves the whole tree) and the set of groups
-    /// caller skips them in the pool-based attach re-check (their uncommitted row
+    /// STRUCTURALLY changed here (created or reparented) — the caller skips them
-    /// isn't visible). Enforces the attach ceiling + RBAC per mutation; the
+    /// in the pool-based attach re-check (their uncommitted row isn't visible).
+    /// Enforces the attach ceiling + RBAC per mutation (in the helpers); the
     /// coarse structural lock is taken only once, on the first mutation.
-    #[allow(clippy::too_many_lines)]
     async fn reconcile_group_structure_tx(
         &self,
         tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
@@ -2072,8 +2074,7 @@ impl ApplyService {
         mode: StructureMode,
     ) -> Result<(HashMap<String, GroupId>, HashSet<GroupId>), ApplyError> {
         use crate::group_repo::{
-            create_group_tx, read_group_id_by_slug_tx, read_group_node_tx, reparent_group_tx,
+            read_group_id_by_slug_tx, read_group_node_tx, GROUP_STRUCTURAL_LOCK_KEY,
-            GROUP_STRUCTURAL_LOCK_KEY,
         };
 
         let attach_slug = project.and_then(|p| p.parent_group.as_deref());
@@ -2088,6 +2089,12 @@ impl ApplyService {
             ),
             None => None,
         };
+        let ctx = ReconcileCtx {
+            attach_gid,
+            attach_slug,
+            principal,
+            mode,
+        };
 
         let mut map: HashMap<String, GroupId> = HashMap::new();
         let mut changed: HashSet<GroupId> = HashSet::new();
@@ -2121,96 +2128,147 @@ impl ApplyService {
                     .map_err(map_group_repo_err)?;
             }
 
-            let Some((gid, server_parent)) = existing else {
+            match existing {
-                // Create the missing group under its declared parent.
+                None => {
-                crate::groups_api::validate_slug_format(&n.slug).map_err(ApplyError::Invalid)?;
+                    let gid = self
-                if !self
+                        .create_group_node_tx(tx, n, declared_parent, &ctx, &within_attach)
-                    .parent_within_attach(declared_parent, attach_gid, &within_attach)
+                        .await?;
-                    .await?
-                {
-                    return Err(ApplyError::OutsideAttachPoint(format!(
-                        "group `{}` would be created outside this project's attach point `{}`",
-                        n.slug,
-                        attach_slug.unwrap_or_default()
-                    )));
-                }
-                // RBAC: a root group needs InstanceCreateGroup; a subgroup needs
-                // GroupAdmin on the parent (mirrors create_group).
-                let cap = match declared_parent {
-                    Some(p) => Capability::GroupAdmin(p),
-                    None => Capability::InstanceCreateGroup,
-                };
-                require(self.authz.as_ref(), principal, cap)
-                    .await
-                    .map_err(map_authz)?;
-                let name = n.name.clone().unwrap_or_else(|| n.slug.clone());
-                let gid = create_group_tx(
-                    tx,
-                    &n.slug,
-                    &name,
-                    n.description.as_deref(),
-                    declared_parent,
-                )
-                .await
-                .map_err(map_group_repo_err)?;
-                changed.insert(gid);
-                if attach_gid.is_some() {
-                    within_attach.insert(gid);
-                }
-                map.insert(key, gid);
-                continue;
-            };
-
-            // The group exists. If its server parent matches the manifest, it's
-            // in-sync; otherwise resolve the divergence per `mode`.
-            map.insert(key, gid);
-            if server_parent == declared_parent {
-                continue;
-            }
-            match mode {
-                StructureMode::AdoptServer => {} // keep the server shape
-                StructureMode::Refuse => {
-                    return Err(ApplyError::StructuralDivergence(format!(
-                        "group `{}` is under a different parent on the server than the manifest \
-                         declares — pass --force-local-structure to reparent it, or \
-                         --adopt-server-structure to keep the server's placement",
-                        n.slug
-                    )));
-                }
-                StructureMode::ForceLocal => {
-                    // §5.6 RBAC: admin on the node + source + destination.
-                    require(self.authz.as_ref(), principal, Capability::GroupAdmin(gid))
-                        .await
-                        .map_err(map_authz)?;
-                    for side in [server_parent, declared_parent].into_iter().flatten() {
-                        require(self.authz.as_ref(), principal, Capability::GroupAdmin(side))
-                            .await
-                            .map_err(map_authz)?;
-                    }
-                    if !self
-                        .parent_within_attach(declared_parent, attach_gid, &within_attach)
-                        .await?
-                    {
-                        return Err(ApplyError::OutsideAttachPoint(format!(
-                            "reparenting group `{}` would move it outside this project's attach \
-                             point `{}`",
-                            n.slug,
-                            attach_slug.unwrap_or_default()
-                        )));
-                    }
-                    reparent_group_tx(tx, gid, declared_parent)
-                        .await
-                        .map_err(map_group_repo_err)?;
                     changed.insert(gid);
                     if attach_gid.is_some() {
                         within_attach.insert(gid);
                     }
+                    map.insert(key, gid);
+                }
+                Some((gid, server_parent)) => {
+                    map.insert(key, gid);
+                    if server_parent == declared_parent {
+                        continue; // in-sync
+                    }
+                    if self
+                        .reparent_diverged_group_tx(
+                            tx,
+                            n,
+                            (gid, server_parent),
+                            declared_parent,
+                            &ctx,
+                            &within_attach,
+                        )
+                        .await?
+                    {
+                        changed.insert(gid);
+                        if attach_gid.is_some() {
+                            within_attach.insert(gid);
+                        }
+                    }
                 }
             }
         }
         Ok((map, changed))
     }
 
+    /// §6: create one missing group node under its declared parent, attach-
+    /// ceiling + RBAC gated (a root group needs `InstanceCreateGroup`, a subgroup
+    /// `GroupAdmin(parent)`). Returns the new id; the caller records it in the
+    /// changed / within-attach / slug→id sets.
+    async fn create_group_node_tx(
+        &self,
+        tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
+        n: &TreeNode,
+        declared_parent: Option<GroupId>,
+        ctx: &ReconcileCtx<'_>,
+        within_attach: &HashSet<GroupId>,
+    ) -> Result<GroupId, ApplyError> {
+        crate::groups_api::validate_slug_format(&n.slug).map_err(ApplyError::Invalid)?;
+        if !self
+            .parent_within_attach(declared_parent, ctx.attach_gid, within_attach)
+            .await?
+        {
+            return Err(ApplyError::OutsideAttachPoint(format!(
+                "group `{}` would be created outside this project's attach point `{}`",
+                n.slug,
+                ctx.attach_slug.unwrap_or_default()
+            )));
+        }
+        let cap = match declared_parent {
+            Some(p) => Capability::GroupAdmin(p),
+            None => Capability::InstanceCreateGroup,
+        };
+        require(self.authz.as_ref(), ctx.principal, cap)
+            .await
+            .map_err(map_authz)?;
+        let name = n.name.clone().unwrap_or_else(|| n.slug.clone());
+        crate::group_repo::create_group_tx(
+            tx,
+            &n.slug,
+            &name,
+            n.description.as_deref(),
+            declared_parent,
+        )
+        .await
+        .map_err(map_group_repo_err)
+    }
+
+    /// §6 M2: resolve an EXISTING group node whose server parent diverges from
+    /// the manifest, per `mode`. `Refuse` → `StructuralDivergence` (422);
+    /// `AdoptServer` → keep the server shape (`Ok(false)`); `ForceLocal` →
+    /// §5.6-gated (`GroupAdmin` on node + source + destination) + attach-
+    /// ceilinged reparent to the declared parent (`Ok(true)`). `Ok(false)` = no
+    /// structural change (the caller records the change only on `Ok(true)`).
+    async fn reparent_diverged_group_tx(
+        &self,
+        tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
+        n: &TreeNode,
+        existing: (GroupId, Option<GroupId>),
+        declared_parent: Option<GroupId>,
+        ctx: &ReconcileCtx<'_>,
+        within_attach: &HashSet<GroupId>,
+    ) -> Result<bool, ApplyError> {
+        let (gid, server_parent) = existing;
+        match ctx.mode {
+            StructureMode::AdoptServer => Ok(false), // keep the server shape
+            StructureMode::Refuse => Err(ApplyError::StructuralDivergence(format!(
+                "group `{}` is under a different parent on the server than the manifest \
+                 declares — pass --force-local-structure to reparent it, or \
+                 --adopt-server-structure to keep the server's placement",
+                n.slug
+            ))),
+            StructureMode::ForceLocal => {
+                // §5.6 RBAC: admin on the node + source + destination.
+                require(
+                    self.authz.as_ref(),
+                    ctx.principal,
+                    Capability::GroupAdmin(gid),
+                )
+                .await
+                .map_err(map_authz)?;
+                for side in [server_parent, declared_parent].into_iter().flatten() {
+                    require(
+                        self.authz.as_ref(),
+                        ctx.principal,
+                        Capability::GroupAdmin(side),
+                    )
+                    .await
+                    .map_err(map_authz)?;
+                }
+                if !self
+                    .parent_within_attach(declared_parent, ctx.attach_gid, within_attach)
+                    .await?
+                {
+                    return Err(ApplyError::OutsideAttachPoint(format!(
+                        "reparenting group `{}` would move it outside this project's attach \
+                         point `{}`",
+                        n.slug,
+                        ctx.attach_slug.unwrap_or_default()
+                    )));
+                }
+                crate::group_repo::reparent_group_tx(tx, gid, declared_parent)
+                    .await
+                    .map_err(map_group_repo_err)?;
+                Ok(true)
+            }
+        }
+    }
+
     /// Resolve nodes to owners, validate each (apps' route/trigger targets may
     /// bind to in-tree ancestor group scripts), compute each node's plan, and
     /// fold a combined bound-plan token. Shared by `plan_tree` / `apply_tree`.
@@ -4819,6 +4877,17 @@ struct ToCreateGroup<'a> {
     plan: Plan,
 }
 
+/// §6: the invariant context threaded through one `reconcile_group_structure_tx`
+/// pass — the attach ceiling (id for the subtree check, slug for messages) and
+/// the acting principal (for per-mutation RBAC). Groups the args the create /
+/// reparent helpers share.
+struct ReconcileCtx<'a> {
+    attach_gid: Option<GroupId>,
+    attach_slug: Option<&'a str>,
+    principal: &'a Principal,
+    mode: StructureMode,
+}
+
 /// FNV-1a over a set of already-sorted per-resource token parts — the same
 /// hashing `state_token` uses, lifted to combine many nodes into one tree
 /// token. Order-independent because the caller sorts `parts`.

docs/design/groups-and-project-tool.md

@@ -838,6 +838,20 @@ environment"); an unlisted / `confirm = false` env applies freely. `require_env_
 
 **§6/§3 group-tree Tier 1 (M1 create · M2 divergence/reparent · M3 per-env approval) is COMPLETE.**
 
+**Review follow-ups (post-Tier-1 hardening).** A code review of the Tier-1 branch surfaced four fixes: (1)
+M1's `authz_tree` skip for a to-create group opened a race window (a group created by a racer between the
+API-layer authz check and the in-tx reconcile could have its content written unauthorized) — closed by an
+in-tx content-write re-check in `apply_tree` for every group NOT structurally created/reparented by that apply
+(`require_group_node_writes` moved from `apply_api` onto `ApplyService`; a group WE create is covered by
+create-RBAC and its uncommitted row is invisible to the authz cascade, so it's skipped). (2) The M3 approval
+gate could silently fail open — now `EnvPolicy.confirm` is a REQUIRED field (an empty `prod = {}` is a load
+error, not a silent no-gate), a single-file `pic apply --file <leaf>` discovers the governing `[project]` by
+walking up the directory tree, and a non-root `[project].environments` is dropped with an explicit warning.
+(3) Divergence detection is unified — the plan-path `divergence_preview` reuses the already-loaded group rows
+(no per-node re-fetch) and compares parent slugs case-insensitively so it can't disagree with the apply path's
+id-based check. (4) `reconcile_group_structure_tx`'s create/reparent branches are extracted into helpers.
+Pinned by new cases in `tests/group_create.rs` + `tests/env_approval.rs`.
+
 ---
 
 ## 8. Diagrams