From be5df06a481e4cb55e232ab0571ee32a846b8f47 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Mon, 22 Jun 2026 21:48:27 +0200 Subject: [PATCH] feat(project-tool): bound-plan staleness check (content fingerprint) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pic plan` now records a fingerprint of the live state it diffed against; `pic apply` replays it and the server refuses (HTTP 409) if the app changed underneath the reviewed plan — the §4.2 "apply exactly what you reviewed" guarantee, in its content-addressed form (no migration, no changes to interactive write paths). Server (manager-core): - `state_token(CurrentState)`: deterministic FNV-1a fingerprint over what the diff keys on — script name+version (version bumps on any edit), route identity+binding/attrs, trigger membership+enabled, secret names. Order-independent; a collision can only yield a false "unchanged", never a false refusal. - `plan` returns it (flattened onto the plan JSON, so the wire stays additive); `apply` takes an optional `expected_token` and, under the apply lock before any write, returns `StateMoved` (409) on mismatch. CLI: - `.picloud/` link state (`linkstate`): `pic plan` writes the token scoped to the app slug; `pic apply` replays it, then clears it on success (the token is single-use — the next apply re-plans). `--force` skips the check; apply with no recorded plan still works standalone (today's behavior). `.picloud/` is already gitignored by `pic init`. The tree-structure version (the other half of §4.2's counter) stays a deliberate no-op until groups exist — it only guards reparent/structural moves, which don't exist single-app. Tested: state_token unit test (stable/order-independent/sensitive) + a staleness journey (plan → out-of-band deploy → apply refuses → --force applies); manager-core lib 363 + cli bins 31 + 13 journeys green; clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_api.rs | 19 +++- crates/manager-core/src/apply_service.rs | 126 ++++++++++++++++++++++- crates/picloud-cli/src/client.rs | 11 +- crates/picloud-cli/src/cmds/apply.rs | 30 +++++- crates/picloud-cli/src/cmds/plan.rs | 10 ++ crates/picloud-cli/src/linkstate.rs | 55 ++++++++++ crates/picloud-cli/src/main.rs | 9 +- crates/picloud-cli/tests/cli.rs | 1 + crates/picloud-cli/tests/init.rs | 5 +- crates/picloud-cli/tests/staleness.rs | 82 +++++++++++++++ 10 files changed, 338 insertions(+), 10 deletions(-) create mode 100644 crates/picloud-cli/src/linkstate.rs create mode 100644 crates/picloud-cli/tests/staleness.rs diff --git a/crates/manager-core/src/apply_api.rs b/crates/manager-core/src/apply_api.rs index bf8f071..82753e2 100644 --- a/crates/manager-core/src/apply_api.rs +++ b/crates/manager-core/src/apply_api.rs @@ -16,7 +16,9 @@ use serde::Deserialize; use serde_json::json; use crate::app_repo::AppRepository; -use crate::apply_service::{ApplyError, ApplyReport, ApplyService, Bundle, BundleTrigger, Plan}; +use crate::apply_service::{ + ApplyError, ApplyReport, ApplyService, Bundle, BundleTrigger, PlanResult, +}; use crate::authz::{require, AuthzDenied, Capability}; /// Build the apply/plan router. Mounted under `/api/v1/admin`. @@ -32,6 +34,10 @@ pub struct ApplyRequest { pub bundle: Bundle, #[serde(default)] pub prune: bool, + /// Optional bound-plan token from a prior `plan`. When present, apply + /// refuses (409) if the app's live state has changed since. + #[serde(default)] + pub expected_token: Option, } async fn apply_handler( @@ -89,7 +95,13 @@ async fn apply_handler( .map_err(map_authz)?; } let report = svc - .apply(app_id, &req.bundle, req.prune, principal.user_id) + .apply( + app_id, + &req.bundle, + req.prune, + principal.user_id, + req.expected_token.as_deref(), + ) .await?; Ok(Json(report)) } @@ -99,7 +111,7 @@ async fn plan_handler( Extension(principal): Extension, Path(id_or_slug): Path, Json(bundle): Json, -) -> Result, ApplyError> { +) -> Result, ApplyError> { let app_id = resolve_app_id(svc.apps.as_ref(), &id_or_slug).await?; // NOTE: the returned `Plan` discloses live secret NAMES (not values). That // is safe today only because `AppRead` and `AppSecretsRead` are co-granted @@ -139,6 +151,7 @@ impl IntoResponse for ApplyError { StatusCode::UNPROCESSABLE_ENTITY, json!({ "error": self.to_string() }), ), + Self::StateMoved => (StatusCode::CONFLICT, json!({ "error": self.to_string() })), Self::Forbidden => (StatusCode::FORBIDDEN, json!({ "error": self.to_string() })), Self::AuthzRepo(e) => { tracing::error!(error = %e, "apply authz repo error"); diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index ed44d37..fdb5b10 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -312,6 +312,16 @@ impl Plan { } } +/// What `plan` returns: the diff plus a fingerprint of the live state it was +/// computed against. The token is flattened onto the plan JSON, so the wire +/// shape stays `{ scripts, routes, triggers, secrets, state_token }`. +#[derive(Debug, Clone, Serialize)] +pub struct PlanResult { + #[serde(flatten)] + pub plan: Plan, + pub state_token: String, +} + // ---------------------------------------------------------------------------- // Current state snapshot // ---------------------------------------------------------------------------- @@ -335,6 +345,12 @@ pub enum ApplyError { AppNotFound(String), #[error("invalid manifest: {0}")] Invalid(String), + #[error( + "live state changed since `pic plan` (someone edited this app's scripts, \ + routes, triggers, or secrets); re-run `pic plan` to review, then apply — \ + or `pic apply --force` to skip the check" + )] + StateMoved, #[error("forbidden")] Forbidden, #[error("authorization repo error: {0}")] @@ -377,12 +393,17 @@ impl ApplyService { /// /// # Errors /// `Invalid` for a malformed bundle; `Backend` for repo failures. - pub async fn plan(&self, app_id: AppId, bundle: &Bundle) -> Result { + pub async fn plan(&self, app_id: AppId, bundle: &Bundle) -> Result { self.validate_bundle(bundle)?; self.validate_route_hosts(app_id, bundle).await?; let current = self.load_current(app_id).await?; validate_email_secrets_present(bundle, ¤t.secret_names)?; - Ok(compute_diff(¤t, bundle)) + Ok(PlanResult { + plan: compute_diff(¤t, bundle), + // Fingerprint of the live state this plan was computed against, so + // `apply` can refuse if the app changed underneath it (§4.2). + state_token: state_token(¤t), + }) } /// Reconcile app `app_id` to `bundle` in a single transaction. Creates @@ -398,6 +419,7 @@ impl ApplyService { bundle: &Bundle, prune: bool, actor: AdminUserId, + expected_token: Option<&str>, ) -> Result { self.validate_bundle(bundle)?; self.validate_route_hosts(app_id, bundle).await?; @@ -422,6 +444,15 @@ impl ApplyService { .map_err(|e| ApplyError::Backend(e.to_string()))?; let current = self.load_current(app_id).await?; + // Bound-plan check (§4.2): if the caller passed the token from a prior + // `pic plan`, refuse when the live state has changed since — computed + // under the apply lock so it reflects what we're about to write. Done + // before any mutation so a stale apply rolls back nothing. + if let Some(expected) = expected_token { + if state_token(¤t) != expected { + return Err(ApplyError::StateMoved); + } + } // Surface a missing email-secret reference here so `plan` and `apply` // agree, rather than only failing deep in `resolve_and_seal` below. validate_email_secrets_present(bundle, ¤t.secret_names)?; @@ -1564,6 +1595,62 @@ fn map_trig(e: TriggerRepoError) -> ApplyError { } } +/// A stable fingerprint of an app's live state, covering exactly what the +/// reconcile diff keys on: script identity + write-counter (`version` bumps on +/// any script edit), route identity + binding/attrs, trigger membership + +/// `enabled`, and secret names. Any out-of-band change to those flips the +/// token, so a `plan`-then-`apply` can detect that the app moved underneath it. +/// +/// Uses FNV-1a (deterministic across process restarts, unlike `DefaultHasher`'s +/// per-process seed) so a token stored by `pic plan` still matches on a later +/// `pic apply`. A hash collision can only ever yield a false "unchanged", which +/// is the same risk class as not checking at all — never a false refusal. +#[must_use] +pub fn state_token(current: &CurrentState) -> String { + let mut parts: Vec = Vec::with_capacity( + current.scripts.len() + + current.routes.len() + + current.triggers.len() + + current.secret_names.len(), + ); + for s in ¤t.scripts { + parts.push(format!("s|{}|{}", s.name.to_lowercase(), s.version)); + } + for r in ¤t.routes { + parts.push(format!( + "r|{}|{:?}|{:?}|{}", + route_key( + r.method.as_deref(), + r.host_kind, + &r.host, + r.path_kind, + &r.path + ), + r.script_id, + r.dispatch_mode, + r.host_param_name.as_deref().unwrap_or(""), + )); + } + for t in ¤t.triggers { + parts.push(format!("t|{:?}|{}", t.id, t.enabled)); + } + for n in ¤t.secret_names { + parts.push(format!("k|{n}")); + } + // Order-independent: sort the per-resource tokens before hashing. + parts.sort_unstable(); + let mut h: u64 = 0xcbf2_9ce4_8422_2325; + for p in &parts { + for b in p.as_bytes() { + h ^= u64::from(*b); + h = h.wrapping_mul(0x0000_0100_0000_01b3); + } + h ^= u64::from(b'\n'); + h = h.wrapping_mul(0x0000_0100_0000_01b3); + } + format!("{h:016x}") +} + /// Per-app advisory lock key, namespaced so it can't collide with the /// queue-trigger lock space. fn apply_lock_key(app_id: AppId) -> i64 { @@ -1875,6 +1962,41 @@ mod tests { .is_err()); } + #[test] + fn state_token_is_stable_order_independent_and_sensitive() { + let a = script("a", "x"); // version 1 + let b = script("b", "y"); + let base = CurrentState { + scripts: vec![a.clone(), b.clone()], + secret_names: vec!["S".into()], + ..CurrentState::default() + }; + // Deterministic + order-independent. + let reordered = CurrentState { + scripts: vec![b.clone(), a.clone()], + secret_names: vec!["S".into()], + ..CurrentState::default() + }; + assert_eq!(state_token(&base), state_token(&reordered)); + // A script edit bumps `version`, which must flip the token even if the + // source-by-name set is otherwise unchanged (out-of-band redeploy). + let mut a2 = a.clone(); + a2.version += 1; + let bumped = CurrentState { + scripts: vec![a2, b.clone()], + secret_names: vec!["S".into()], + ..CurrentState::default() + }; + assert_ne!(state_token(&base), state_token(&bumped)); + // Adding a secret name flips it too. + let with_secret = CurrentState { + scripts: vec![a, b], + secret_names: vec!["S".into(), "T".into()], + ..CurrentState::default() + }; + assert_ne!(state_token(&base), state_token(&with_secret)); + } + #[test] fn trigger_diff_create_noop_delete() { let s = script("h", "x"); diff --git a/crates/picloud-cli/src/client.rs b/crates/picloud-cli/src/client.rs index 53a0e64..fbaedbe 100644 --- a/crates/picloud-cli/src/client.rs +++ b/crates/picloud-cli/src/client.rs @@ -920,9 +920,14 @@ impl Client { app: &str, bundle: &serde_json::Value, prune: bool, + expected_token: Option<&str>, ) -> Result { let app = seg(app); - let body = serde_json::json!({ "bundle": bundle, "prune": prune }); + let body = serde_json::json!({ + "bundle": bundle, + "prune": prune, + "expected_token": expected_token, + }); let resp = self .request(Method::POST, &format!("/api/v1/admin/apps/{app}/apply")) .json(&body) @@ -965,6 +970,10 @@ pub struct PlanDto { pub triggers: Vec, #[serde(default)] pub secrets: Vec, + /// Fingerprint of the live state this plan was computed against; carried + /// in `.picloud/` and replayed to `apply` for the bound-plan check. + #[serde(default)] + pub state_token: String, } #[derive(Debug, Deserialize)] diff --git a/crates/picloud-cli/src/cmds/apply.rs b/crates/picloud-cli/src/cmds/apply.rs index 82c21c2..899ca55 100644 --- a/crates/picloud-cli/src/cmds/apply.rs +++ b/crates/picloud-cli/src/cmds/apply.rs @@ -14,7 +14,13 @@ use crate::config; use crate::manifest::Manifest; use crate::output::{KvBlock, OutputMode}; -pub async fn run(manifest_path: &Path, prune: bool, yes: bool, mode: OutputMode) -> Result<()> { +pub async fn run( + manifest_path: &Path, + prune: bool, + yes: bool, + force: bool, + mode: OutputMode, +) -> Result<()> { let creds = config::resolve()?; let client = Client::from_creds(&creds)?; @@ -26,7 +32,27 @@ pub async fn run(manifest_path: &Path, prune: bool, yes: bool, mode: OutputMode) confirm_prune(&manifest.app.slug)?; } - let report = client.apply(&manifest.app.slug, &bundle, prune).await?; + // Bound-plan check: replay the token from the last `pic plan` (for this + // app) so the server refuses if the app changed since it was reviewed. + // `--force` skips it; no recorded plan means no check (apply still works + // standalone). The token is single-use — cleared after a successful apply. + let expected_token = if force { + None + } else { + crate::linkstate::read_plan(base_dir) + .filter(|l| l.app == manifest.app.slug) + .map(|l| l.state_token) + }; + + let report = client + .apply( + &manifest.app.slug, + &bundle, + prune, + expected_token.as_deref(), + ) + .await?; + crate::linkstate::clear_plan(base_dir); let mut block = KvBlock::new(); block diff --git a/crates/picloud-cli/src/cmds/plan.rs b/crates/picloud-cli/src/cmds/plan.rs index 4e7f027..2ebdde2 100644 --- a/crates/picloud-cli/src/cmds/plan.rs +++ b/crates/picloud-cli/src/cmds/plan.rs @@ -23,6 +23,16 @@ pub async fn run(manifest_path: &Path, mode: OutputMode) -> Result<()> { let bundle = build_bundle(&manifest, base_dir)?; let plan = client.plan(&manifest.app.slug, &bundle).await?; + // Record the bound-plan token so a subsequent `pic apply` can detect the + // app changing underneath the reviewed plan (best-effort — a read-only + // plan still succeeds if the project dir isn't writable). + if !plan.state_token.is_empty() { + if let Err(e) = + crate::linkstate::write_plan(base_dir, &manifest.app.slug, &plan.state_token) + { + eprintln!("warning: could not record plan state for `pic apply`: {e}"); + } + } render(&plan, mode); Ok(()) } diff --git a/crates/picloud-cli/src/linkstate.rs b/crates/picloud-cli/src/linkstate.rs new file mode 100644 index 0000000..829f2b0 --- /dev/null +++ b/crates/picloud-cli/src/linkstate.rs @@ -0,0 +1,55 @@ +//! `.picloud/` link state — gitignored, per-project metadata the project tool +//! carries between CLI invocations. Today it holds just the bound-plan token: +//! `pic plan` records the fingerprint of the live state it diffed against, and +//! `pic apply` replays it so the server can refuse if the app moved underneath. +//! +//! All paths are relative to the manifest's directory (the project root). + +use std::fs; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use serde::{Deserialize, Serialize}; + +const DIR: &str = ".picloud"; +const PLAN_FILE: &str = "plan.json"; + +/// The recorded result of the last `pic plan`, scoped to the app it was for. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct PlanLink { + /// App slug the token belongs to — guards against replaying a token from a + /// different app if the manifest's `slug` changed. + pub app: String, + pub state_token: String, +} + +fn plan_path(base: &Path) -> PathBuf { + base.join(DIR).join(PLAN_FILE) +} + +/// Record the bound-plan token for `app` under `base/.picloud/`. +pub fn write_plan(base: &Path, app: &str, state_token: &str) -> Result<()> { + let dir = base.join(DIR); + fs::create_dir_all(&dir).with_context(|| format!("creating {}", dir.display()))?; + let link = PlanLink { + app: app.to_string(), + state_token: state_token.to_string(), + }; + let body = serde_json::to_vec_pretty(&link).context("encoding .picloud/plan.json")?; + fs::write(plan_path(base), body).context("writing .picloud/plan.json")?; + Ok(()) +} + +/// Read the recorded plan token, if any. Returns `None` when absent or +/// unreadable (treated as "no prior plan" — never an error). +#[must_use] +pub fn read_plan(base: &Path) -> Option { + let body = fs::read(plan_path(base)).ok()?; + serde_json::from_slice(&body).ok() +} + +/// Remove the recorded plan token (best-effort). Called after a successful +/// apply consumes it, so the next apply requires a fresh plan. +pub fn clear_plan(base: &Path) { + let _ = fs::remove_file(plan_path(base)); +} diff --git a/crates/picloud-cli/src/main.rs b/crates/picloud-cli/src/main.rs index 505fd2d..79ff7ab 100644 --- a/crates/picloud-cli/src/main.rs +++ b/crates/picloud-cli/src/main.rs @@ -12,6 +12,7 @@ use clap::{Args, Parser, Subcommand, ValueEnum}; mod client; mod cmds; mod config; +mod linkstate; mod manifest; mod output; @@ -191,6 +192,10 @@ struct ApplyArgs { /// non-interactively (CI). #[arg(long)] yes: bool, + /// Skip the bound-plan staleness check (apply even if the app changed + /// since the last `pic plan`). + #[arg(long)] + force: bool, } #[derive(Args)] @@ -1112,7 +1117,9 @@ async fn main() -> ExitCode { } Cmd::Logout => cmds::logout::run().await, Cmd::Whoami => cmds::whoami::run(mode).await, - Cmd::Apply(args) => cmds::apply::run(&args.file, args.prune, args.yes, mode).await, + Cmd::Apply(args) => { + cmds::apply::run(&args.file, args.prune, args.yes, args.force, mode).await + } Cmd::Plan(args) => cmds::plan::run(&args.file, mode).await, Cmd::Pull(args) => cmds::pull::run(&args.app, &args.dir, mode).await, Cmd::Init(args) => cmds::init::run( diff --git a/crates/picloud-cli/tests/cli.rs b/crates/picloud-cli/tests/cli.rs index 5cd0959..33cd52d 100644 --- a/crates/picloud-cli/tests/cli.rs +++ b/crates/picloud-cli/tests/cli.rs @@ -31,4 +31,5 @@ mod roles; mod routes; mod scripts; mod secrets; +mod staleness; mod triggers; diff --git a/crates/picloud-cli/tests/init.rs b/crates/picloud-cli/tests/init.rs index c739f34..10220e7 100644 --- a/crates/picloud-cli/tests/init.rs +++ b/crates/picloud-cli/tests/init.rs @@ -68,7 +68,10 @@ fn init_appends_to_an_existing_gitignore_once() { .assert() .success(); let gitignore = fs::read_to_string(dir.path().join(".gitignore")).unwrap(); - assert!(gitignore.contains("target/"), "must preserve existing rules"); + assert!( + gitignore.contains("target/"), + "must preserve existing rules" + ); assert_eq!( gitignore.matches(".picloud/").count(), 1, diff --git a/crates/picloud-cli/tests/staleness.rs b/crates/picloud-cli/tests/staleness.rs new file mode 100644 index 0000000..1ca545f --- /dev/null +++ b/crates/picloud-cli/tests/staleness.rs @@ -0,0 +1,82 @@ +//! Bound-plan staleness: `pic plan` records a state token under `.picloud/`, +//! and a later `pic apply` refuses (without `--force`) if the app changed +//! out-of-band since the plan was reviewed. + +use std::fs; + +use tempfile::TempDir; + +use crate::common; +use crate::common::cleanup::AppGuard; + +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn apply_refuses_when_state_moved_since_plan() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let slug = common::unique_slug("stale"); + common::pic_as(&env) + .args(["apps", "create", &slug]) + .assert() + .success(); + let _guard = AppGuard::new(&env.url, &env.token, &slug); + + let dir = TempDir::new().unwrap(); + fs::create_dir_all(dir.path().join("scripts")).unwrap(); + fs::write(dir.path().join("scripts/hello.rhai"), "let x = 1; x").unwrap(); + let manifest_path = dir.path().join("picloud.toml"); + let manifest = format!( + "[app]\nslug = \"{slug}\"\nname = \"Stale\"\n\n\ + [[scripts]]\nname = \"hello\"\nfile = \"scripts/hello.rhai\"\n" + ); + fs::write(&manifest_path, &manifest).unwrap(); + + // Establish hello, then plan — the plan records the current state token. + apply(&env, &manifest_path).assert().success(); + common::pic_as(&env) + .args(["plan", "--file"]) + .arg(&manifest_path) + .assert() + .success(); + assert!( + dir.path().join(".picloud/plan.json").exists(), + "plan must record the bound-plan token under .picloud/" + ); + + // Out-of-band change: deploy an extra script the manifest doesn't mention. + fs::write(dir.path().join("scripts/sneaky.rhai"), "let y = 2; y").unwrap(); + common::pic_as(&env) + .args(["scripts", "deploy"]) + .arg(dir.path().join("scripts/sneaky.rhai")) + .args(["--app", &slug]) + .assert() + .success(); + + // Apply must now refuse — the live state no longer matches the plan. + let refused = apply(&env, &manifest_path).output().expect("apply"); + assert!( + !refused.status.success(), + "apply must refuse after out-of-band change" + ); + let err = String::from_utf8_lossy(&refused.stderr); + assert!( + err.contains("changed since") || err.contains("pic plan"), + "refusal should explain the staleness:\n{err}" + ); + + // `--force` bypasses the check and applies anyway. + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(&manifest_path) + .arg("--force") + .assert() + .success(); +} + +fn apply(env: &common::TestEnv, manifest_path: &std::path::Path) -> assert_cmd::Command { + let mut cmd = common::pic_as(env); + cmd.args(["apply", "--file"]).arg(manifest_path); + cmd +}