From 87292fc4e94c3fd9533d42b824c5fdd861a1eea6 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 7 Jul 2026 21:49:18 +0200 Subject: [PATCH] refactor(apply): split reconcile_group_structure_tx create/reparent branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/manager-core/src/apply_service.rs | 251 +++++++++++++++-------- docs/design/groups-and-project-tool.md | 14 ++ 2 files changed, 174 insertions(+), 91 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 676af0c..194df18 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/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 - /// differs from the manifest's declared parent is a structural divergence, - /// resolved per `mode` (refuse / reparent / adopt-server). Returns the - /// complete slug → id map (so `prepare_tree` resolves the whole tree) and the - /// set of groups STRUCTURALLY changed here (created or reparented) — the - /// caller skips them in the pool-based attach re-check (their uncommitted row - /// isn't visible). Enforces the attach ceiling + RBAC per mutation; the + /// first). A group the server lacks is CREATED (`create_group_node_tx`); a + /// group whose server parent differs from the manifest's declared parent is + /// a structural divergence, resolved per `mode` (`reparent_diverged_group_tx` + /// — refuse / reparent / adopt-server). Returns the complete slug → id map + /// (so `prepare_tree` resolves the whole tree) and the set of groups + /// STRUCTURALLY changed here (created or reparented) — the caller skips them + /// 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, HashSet), ApplyError> { use crate::group_repo::{ - create_group_tx, read_group_id_by_slug_tx, read_group_node_tx, reparent_group_tx, - GROUP_STRUCTURAL_LOCK_KEY, + read_group_id_by_slug_tx, read_group_node_tx, 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 = HashMap::new(); let mut changed: HashSet = HashSet::new(); @@ -2121,96 +2128,147 @@ impl ApplyService { .map_err(map_group_repo_err)?; } - let Some((gid, server_parent)) = existing else { - // Create the missing group under its declared parent. - crate::groups_api::validate_slug_format(&n.slug).map_err(ApplyError::Invalid)?; - if !self - .parent_within_attach(declared_parent, attach_gid, &within_attach) - .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)?; + match existing { + None => { + let gid = self + .create_group_node_tx(tx, n, declared_parent, &ctx, &within_attach) + .await?; 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, + ctx: &ReconcileCtx<'_>, + within_attach: &HashSet, + ) -> Result { + 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), + declared_parent: Option, + ctx: &ReconcileCtx<'_>, + within_attach: &HashSet, + ) -> Result { + 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, + 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`. diff --git a/docs/design/groups-and-project-tool.md b/docs/design/groups-and-project-tool.md index 292268c..e494cc7 100644 --- a/docs/design/groups-and-project-tool.md +++ b/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 ` 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