From e517f6dc8caf2c83197a7a10f352e2de28ef6604 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Thu, 25 Jun 2026 07:35:12 +0200 Subject: [PATCH] feat(apply): reconcile app-owned vars in the declarative engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the Phase-1 apply engine to manage app `vars` declaratively, alongside scripts/routes/triggers/secrets. The `Bundle` gains a `vars` map (key → JSON value; values are non-secret, so they live inline, unlike secret names). `diff_vars` is value-sensitive: a changed value is an Update, a live var the manifest dropped is a Delete (applied only under `--prune` — vars are prunable config, unlike never-pruned secrets). Writes go through `set_app_var_tx`/`delete_app_var_tx` inside the apply transaction (mirroring `PostgresVarsRepo::set` for `VarOwner::App`, scope `*`), so they commit atomically with the rest of the apply and roll back together on failure. `CurrentState` loads the app's OWN scope-`*`, non-tombstone vars (inherited group vars and tombstones are out of scope for the manifest); `state_token` folds them in so the bound-plan check catches an out-of-band `pic vars set` between plan and apply. Keys are validated against the same kebab rule as the vars admin API, and the apply handler now requires `AppVarsWrite` when vars are touched or `--prune` is set. Scope: app-owned vars at scope `*` (an app *is* an environment). Group vars via manifest and tombstone-via-manifest wait for the nested-manifest model. Unit tests cover the create/update/noop/delete diff and the state-token value sensitivity. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_api.rs | 9 + crates/manager-core/src/apply_service.rs | 250 ++++++++++++++++++++++- crates/picloud/src/lib.rs | 1 + 3 files changed, 259 insertions(+), 1 deletion(-) diff --git a/crates/manager-core/src/apply_api.rs b/crates/manager-core/src/apply_api.rs index 82753e2..6ecb78c 100644 --- a/crates/manager-core/src/apply_api.rs +++ b/crates/manager-core/src/apply_api.rs @@ -81,6 +81,15 @@ async fn apply_handler( .await .map_err(map_authz)?; } + if req.prune || !req.bundle.vars.is_empty() { + require( + svc.authz.as_ref(), + &principal, + Capability::AppVarsWrite(app_id), + ) + .await + .map_err(map_authz)?; + } // Email triggers resolve and decrypt a stored secret by name server-side, // which the secrets API guards with `AppSecretsRead`. Require it here too // so apply can't bind a secret a principal couldn't otherwise read — the diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index e899d0e..5e16a48 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -52,6 +52,7 @@ use crate::trigger_repo::{ delete_trigger_tx, insert_email_trigger_tx, insert_trigger_tx, Trigger, TriggerDetails, TriggerDispatchMode, TriggerRepo, TriggerRepoError, }; +use crate::vars_repo::{VarOwner, VarsRepo}; /// Reserved path prefixes that user routes may not claim (mirrors the /// route-create rejection — see CLAUDE.md "Reserved path prefixes"). @@ -74,6 +75,13 @@ pub struct Bundle { /// Declared secret *names* only; values are pushed out-of-band. #[serde(default)] pub secrets: Vec, + /// Declared app-owned config `vars` — key → JSON value. Unlike secrets, + /// var values are non-secret and live inline in the manifest. Written at + /// app scope `*` (an app *is* an environment, §2 — per-env scoping is a + /// group concern, deferred to the nested-manifest model). Tombstones are + /// not expressible here; use `pic vars set --tombstone`. + #[serde(default)] + pub vars: std::collections::BTreeMap, } #[derive(Debug, Clone, Deserialize)] @@ -304,6 +312,8 @@ pub struct Plan { pub routes: Vec, pub triggers: Vec, pub secrets: Vec, + #[serde(default)] + pub vars: Vec, } impl Plan { @@ -315,6 +325,7 @@ impl Plan { .chain(&self.routes) .chain(&self.triggers) .chain(&self.secrets) + .chain(&self.vars) .all(|c| c.op == Op::NoOp) } } @@ -340,6 +351,10 @@ pub struct CurrentState { pub routes: Vec, pub triggers: Vec, pub secret_names: Vec, + /// App's OWN `vars` at scope `*` (key, value), non-tombstone — the set + /// the manifest reconciles. Inherited group vars and tombstones are + /// excluded (the manifest manages only the app's own real values). + pub vars: Vec<(String, serde_json::Value)>, } // ---------------------------------------------------------------------------- @@ -380,6 +395,9 @@ pub struct ApplyService { pub routes: Arc, pub triggers: Arc, pub secrets: Arc, + /// App-owned `vars` reconciliation (read/diff path; the transactional + /// write goes through `set_app_var_tx` / `delete_app_var_tx`). + pub vars: Arc, pub apps: Arc, /// App domain claims — validates a route's host belongs to the app. pub domains: Arc, @@ -650,6 +668,22 @@ impl ApplyService { report.triggers_created += 1; } + // 3b. Vars — upsert every Create/Update at app scope `*`, in-tx so a + // later failure rolls them back with everything else. + for ch in &plan.vars { + match ch.op { + Op::Create => { + set_app_var_tx(&mut tx, app_id, &ch.key, &bundle.vars[&ch.key]).await?; + report.vars_created += 1; + } + Op::Update => { + set_app_var_tx(&mut tx, app_id, &ch.key, &bundle.vars[&ch.key]).await?; + report.vars_updated += 1; + } + Op::NoOp | Op::Delete => {} + } + } + // 4. Prune (only with --prune): delete stale triggers, then scripts // (route removals already happened in the route delete-pass above). // Secret pruning is deliberately deferred (destructive + irreversible): @@ -712,6 +746,15 @@ impl ApplyService { } } } + + // Vars are prunable config (unlike secrets, which are never + // pruned): a live app var the manifest dropped is deleted. + for ch in &plan.vars { + if ch.op == Op::Delete { + delete_app_var_tx(&mut tx, app_id, &ch.key).await?; + report.vars_deleted += 1; + } + } } tx.commit() @@ -984,6 +1027,12 @@ impl ApplyService { return Err(ApplyError::Invalid(format!("duplicate trigger `{id}`"))); } } + + // Vars: kebab keys (same rule the vars admin API enforces), so a + // manifest can't write a key the interactive surface would reject. + for key in bundle.vars.keys() { + validate_var_key(key)?; + } Ok(()) } @@ -1004,11 +1053,25 @@ impl ApplyService { .await .map_err(|e| ApplyError::Backend(e.to_string()))?; let secret_names = self.all_secret_names(app_id).await?; + // App's OWN vars, narrowed to scope `*` and real values: the manifest + // reconciles app-own env-agnostic config, not inherited group vars + // (those aren't in this owner's rows at all) nor tombstones (an + // imperative suppress the manifest can't express). + let vars = self + .vars + .list_for_owner(VarOwner::App(app_id)) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))? + .into_iter() + .filter(|r| r.environment_scope == "*" && !r.is_tombstone) + .map(|r| (r.key, r.value)) + .collect(); Ok(CurrentState { scripts, routes, triggers, secret_names, + vars, }) } @@ -1053,9 +1116,112 @@ pub fn compute_diff(current: &CurrentState, bundle: &Bundle) -> Plan { routes: diff_routes(current, bundle, &script_name_by_id), triggers: diff_triggers(current, bundle, &script_name_by_id), secrets: diff_secrets(current, bundle), + vars: diff_vars(current, bundle), } } +/// Diff app-owned vars by key. Unlike secrets (whose values live out-of-band), +/// a var's value IS in the manifest, so equality is value-sensitive: a changed +/// value is an `Update`. Live vars absent from the manifest are `Delete` +/// (applied only under `--prune`). +fn diff_vars(current: &CurrentState, bundle: &Bundle) -> Vec { + let live: HashMap<&str, &serde_json::Value> = + current.vars.iter().map(|(k, v)| (k.as_str(), v)).collect(); + + let mut out = Vec::new(); + for (key, value) in &bundle.vars { + match live.get(key.as_str()) { + Some(cur) if *cur == value => out.push(ResourceChange { + op: Op::NoOp, + key: key.clone(), + detail: None, + }), + Some(_) => out.push(ResourceChange { + op: Op::Update, + key: key.clone(), + detail: Some("value changed".into()), + }), + None => out.push(ResourceChange { + op: Op::Create, + key: key.clone(), + detail: None, + }), + } + } + for (key, _) in ¤t.vars { + if !bundle.vars.contains_key(key) { + out.push(ResourceChange { + op: Op::Delete, + key: key.clone(), + detail: Some("on server, not declared".into()), + }); + } + } + out +} + +/// Kebab var key (`^[a-z0-9][a-z0-9-]{0,127}$`) — mirrors `vars_api`'s +/// `validate_key` so the manifest and the interactive surface agree. +fn validate_var_key(key: &str) -> Result<(), ApplyError> { + let ok = !key.is_empty() + && key.len() <= 128 + && key + .chars() + .next() + .is_some_and(|c| c.is_ascii_lowercase() || c.is_ascii_digit()) + && key + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-'); + if ok { + Ok(()) + } else { + Err(ApplyError::Invalid(format!( + "var key `{key}` must be 1–128 chars of lowercase letters, digits, and hyphens, \ + starting with a letter or digit" + ))) + } +} + +/// Upsert one app-owned var at scope `*`, in the apply transaction. Mirrors +/// `PostgresVarsRepo::set` for `VarOwner::App` (the partial-index predicate is +/// restated in the conflict target) but runs against `&mut tx` so it commits +/// atomically with the rest of the apply. Clears any tombstone. +async fn set_app_var_tx( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + app_id: AppId, + key: &str, + value: &serde_json::Value, +) -> Result<(), ApplyError> { + sqlx::query( + "INSERT INTO vars (app_id, environment_scope, key, value, is_tombstone) \ + VALUES ($1, '*', $2, $3, FALSE) \ + ON CONFLICT (app_id, environment_scope, key) WHERE app_id IS NOT NULL DO UPDATE \ + SET value = EXCLUDED.value, is_tombstone = FALSE, updated_at = NOW()", + ) + .bind(app_id.into_inner()) + .bind(key) + .bind(value) + .execute(&mut **tx) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))?; + Ok(()) +} + +/// Delete one app-owned var at scope `*`, in the apply transaction. +async fn delete_app_var_tx( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + app_id: AppId, + key: &str, +) -> Result<(), ApplyError> { + sqlx::query("DELETE FROM vars WHERE app_id = $1 AND environment_scope = '*' AND key = $2") + .bind(app_id.into_inner()) + .bind(key) + .execute(&mut **tx) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))?; + Ok(()) +} + fn diff_scripts(current: &CurrentState, bundle: &Bundle) -> Vec { let by_name: HashMap = current .scripts @@ -1608,6 +1774,12 @@ pub struct ApplyReport { pub routes_deleted: u32, pub triggers_created: u32, pub triggers_deleted: u32, + #[serde(default)] + pub vars_created: u32, + #[serde(default)] + pub vars_updated: u32, + #[serde(default)] + pub vars_deleted: u32, #[serde(skip_serializing_if = "Vec::is_empty")] pub warnings: Vec, } @@ -1731,7 +1903,8 @@ pub fn state_token(current: &CurrentState) -> String { current.scripts.len() + current.routes.len() + current.triggers.len() - + current.secret_names.len(), + + current.secret_names.len() + + current.vars.len(), ); for s in ¤t.scripts { parts.push(format!( @@ -1775,6 +1948,16 @@ pub fn state_token(current: &CurrentState) -> String { for n in ¤t.secret_names { parts.push(format!("k|{n}")); } + // Vars are value-sensitive (the value lives in the manifest), so the token + // moves when a value changes — the bound-plan check then catches an edit + // between plan and apply. `to_string` on a serde_json::Value is stable + // (serde_json sorts object keys by default). + for (key, value) in ¤t.vars { + parts.push(format!( + "v|{key}|{}", + serde_json::to_string(value).unwrap_or_default() + )); + } // Order-independent: sort the per-resource tokens before hashing. parts.sort_unstable(); let mut h: u64 = 0xcbf2_9ce4_8422_2325; @@ -1843,6 +2026,7 @@ mod tests { routes: vec![], triggers: vec![], secrets: vec!["S".into()], + vars: std::collections::BTreeMap::new(), }; let plan = compute_diff(¤t, &bundle); assert_eq!(plan.scripts.len(), 1); @@ -1864,6 +2048,7 @@ mod tests { routes: vec![], triggers: vec![], secrets: vec!["S".into()], + vars: std::collections::BTreeMap::new(), }; let plan = compute_diff(¤t, &bundle); assert!(plan.is_noop(), "expected all no-op, got {plan:?}"); @@ -1880,6 +2065,7 @@ mod tests { routes: vec![], triggers: vec![], secrets: vec![], + vars: std::collections::BTreeMap::new(), }; let plan = compute_diff(¤t, &bundle); assert_eq!(plan.scripts[0].op, Op::Update); @@ -1897,6 +2083,7 @@ mod tests { routes: vec![], triggers: vec![], secrets: vec![], + vars: std::collections::BTreeMap::new(), }; let plan = compute_diff(¤t, &bundle); assert_eq!(plan.scripts[0].op, Op::Delete); @@ -1942,6 +2129,7 @@ mod tests { }], triggers: vec![], secrets: vec![], + vars: std::collections::BTreeMap::new(), }; let plan = compute_diff(¤t, &bundle); assert_eq!(plan.routes[0].op, Op::Update); @@ -1953,6 +2141,7 @@ mod tests { routes: vec![], triggers: vec![], secrets: vec![], + vars: std::collections::BTreeMap::new(), } } @@ -2140,6 +2329,65 @@ mod tests { assert_ne!(state_token(&base), state_token(&with_secret)); } + #[test] + fn diff_vars_create_update_noop_delete() { + let current = CurrentState { + vars: vec![ + ("region".into(), serde_json::json!("eu")), + ("keep".into(), serde_json::json!(1)), + ("stale".into(), serde_json::json!(true)), + ], + ..CurrentState::default() + }; + let mut vars = std::collections::BTreeMap::new(); + vars.insert("region".to_string(), serde_json::json!("us")); // value changed + vars.insert("keep".to_string(), serde_json::json!(1)); // identical + vars.insert("new".to_string(), serde_json::json!("x")); // not live + // "stale" is live but undeclared → Delete. + let bundle = Bundle { + vars, + ..empty_bundle() + }; + let plan = compute_diff(¤t, &bundle); + let op = |k: &str| plan.vars.iter().find(|c| c.key == k).unwrap().op; + assert_eq!(op("region"), Op::Update); + assert_eq!(op("keep"), Op::NoOp); + assert_eq!(op("new"), Op::Create); + assert_eq!(op("stale"), Op::Delete); + assert!(!plan.is_noop()); + } + + #[test] + fn state_token_is_sensitive_to_var_values_and_order_independent() { + let base = CurrentState { + vars: vec![("region".into(), serde_json::json!("eu"))], + ..CurrentState::default() + }; + // A value change must flip the token (covers the bound-plan check for + // an out-of-band `pic vars set` between plan and apply). + let changed = CurrentState { + vars: vec![("region".into(), serde_json::json!("us"))], + ..CurrentState::default() + }; + assert_ne!(state_token(&base), state_token(&changed)); + // Order-independent across multiple vars. + let ab = CurrentState { + vars: vec![ + ("a".into(), serde_json::json!(1)), + ("b".into(), serde_json::json!(2)), + ], + ..CurrentState::default() + }; + let ba = CurrentState { + vars: vec![ + ("b".into(), serde_json::json!(2)), + ("a".into(), serde_json::json!(1)), + ], + ..CurrentState::default() + }; + assert_eq!(state_token(&ab), state_token(&ba)); + } + #[test] fn state_token_ignores_dead_letter_triggers() { // The diff ignores dead-letter triggers (not manifest-representable), diff --git a/crates/picloud/src/lib.rs b/crates/picloud/src/lib.rs index 7dffc2b..d87241d 100644 --- a/crates/picloud/src/lib.rs +++ b/crates/picloud/src/lib.rs @@ -471,6 +471,7 @@ pub async fn build_app( routes: route_repo.clone(), triggers: trigger_repo.clone(), secrets: secrets_repo.clone(), + vars: Arc::new(PostgresVarsRepo::new(pool.clone())), apps: apps_repo.clone(), domains: domains_repo.clone(), authz: authz.clone(),