From 627996cde7c2f45381110ffd635e25aad302248b Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Mon, 22 Jun 2026 22:07:31 +0200 Subject: [PATCH] fix(project-tool): address independent review of init + staleness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remediation after an independent review of the two feature commits. pic init: - Fix the headline `pic init --dir ` flow: slug derivation used `canonicalize()`, which fails on a not-yet-created directory → empty slug → bail. Derive from the path's own final component first, falling back to canonicalize only for `.`. Add a test that exercised the gap (the existing tests all passed an explicit slug). - `ensure_gitignored` no longer overwrites an existing-but-unreadable `.gitignore` (only a genuinely-absent file is treated as empty). Bound-plan staleness: - Token trigger coverage now mirrors the diff exactly via `current_trigger_identity`: dead-letter triggers (which the diff ignores and the manifest can't represent) and the currently-inert `enabled` flag are no longer hashed, so an out-of-band dead-letter change can't force a spurious `--force`. Add a test. - Correct two overclaiming comments: the check shares the diff's pool-read apply-vs-interactive-write window (it is not tx-scoped), and the token deliberately mirrors the diff's inputs. - `.picloud/` self-ignores via a `*` `.gitignore` written alongside the plan token, so projects created with `pic pull`/`plan` (not just `init`) keep it out of git. - `clear_plan` is now app-scoped: it won't discard a token recorded for a different app sharing the directory. Tested: manager-core lib 364 + cli bins 31 + 14 project-tool journeys green; clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 64 +++++++++++++++++++++--- crates/picloud-cli/src/cmds/apply.rs | 2 +- crates/picloud-cli/src/cmds/init.rs | 22 ++++++-- crates/picloud-cli/src/linkstate.rs | 20 ++++++-- crates/picloud-cli/tests/init.rs | 19 +++++++ 5 files changed, 111 insertions(+), 16 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index fdb5b10..f904be9 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -445,9 +445,13 @@ impl ApplyService { 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. + // `pic plan`, refuse when the live state changed since. Computed from + // the same `load_current` snapshot the diff uses, before any mutation + // (so a stale apply rolls back nothing). NOTE: like the diff, this read + // is on the pool, not `&mut *tx` (see the lock comment above), so it + // catches drift between plan and apply but shares that same narrow + // apply-vs-interactive-write window — it is not a substitute for the + // tx-scoped read the follow-up will add. if let Some(expected) = expected_token { if state_token(¤t) != expected { return Err(ApplyError::StateMoved); @@ -1597,9 +1601,13 @@ 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. +/// any script edit), route identity + binding/attrs, trigger semantic identity +/// (via `current_trigger_identity`, so dead-letter triggers — which the diff +/// ignores — are excluded), and secret names. Any out-of-band change to those +/// flips the token, so `plan`-then-`apply` can detect the app moving underneath. +/// +/// Deliberately mirrors the diff's inputs so a mismatch means the *reviewed +/// plan* would now differ — not merely that some unrelated row changed. /// /// 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 @@ -1631,8 +1639,20 @@ pub fn state_token(current: &CurrentState) -> String { r.host_param_name.as_deref().unwrap_or(""), )); } + // Mirror exactly what the trigger diff keys on: `current_trigger_identity` + // excludes dead-letter triggers (the diff ignores them) and keys on the + // semantic identity — NOT raw id/enabled. Hashing dead-letter rows or the + // (currently inert) `enabled` flag would force a false refusal over a + // change the manifest can't represent. + let script_name_by_id: HashMap = current + .scripts + .iter() + .map(|s| (s.id, s.name.clone())) + .collect(); for t in ¤t.triggers { - parts.push(format!("t|{:?}|{}", t.id, t.enabled)); + if let Some(id) = current_trigger_identity(t, &script_name_by_id) { + parts.push(format!("t|{id}")); + } } for n in ¤t.secret_names { parts.push(format!("k|{n}")); @@ -1997,6 +2017,36 @@ mod tests { assert_ne!(state_token(&base), state_token(&with_secret)); } + #[test] + fn state_token_ignores_dead_letter_triggers() { + // The diff ignores dead-letter triggers (not manifest-representable), + // so the token must too — otherwise an out-of-band dead-letter change + // would force a spurious `--force`. + let s = script("h", "x"); + let sid = s.id; + let base = CurrentState { + scripts: vec![s.clone()], + ..CurrentState::default() + }; + let with_dl = CurrentState { + scripts: vec![s], + triggers: vec![trig( + sid, + TriggerDetails::DeadLetter { + source_filter: None, + trigger_id_filter: None, + script_id_filter: None, + }, + )], + ..CurrentState::default() + }; + assert_eq!( + state_token(&base), + state_token(&with_dl), + "dead-letter triggers must not affect the token" + ); + } + #[test] fn trigger_diff_create_noop_delete() { let s = script("h", "x"); diff --git a/crates/picloud-cli/src/cmds/apply.rs b/crates/picloud-cli/src/cmds/apply.rs index 899ca55..88e08e9 100644 --- a/crates/picloud-cli/src/cmds/apply.rs +++ b/crates/picloud-cli/src/cmds/apply.rs @@ -52,7 +52,7 @@ pub async fn run( expected_token.as_deref(), ) .await?; - crate::linkstate::clear_plan(base_dir); + crate::linkstate::clear_plan(base_dir, &manifest.app.slug); let mut block = KvBlock::new(); block diff --git a/crates/picloud-cli/src/cmds/init.rs b/crates/picloud-cli/src/cmds/init.rs index 2c0b6c1..ee96e3c 100644 --- a/crates/picloud-cli/src/cmds/init.rs +++ b/crates/picloud-cli/src/cmds/init.rs @@ -147,10 +147,18 @@ fn resolve_slug(dir: &Path, slug_arg: Option<&str>) -> Result { } return Ok(s.to_string()); } + // Derive from the directory's own name. Use the path as given first — + // `canonicalize` requires the dir to already exist, which breaks the + // natural `pic init --dir new-project` flow. Fall back to canonicalizing + // only when the path has no final component of its own (e.g. `.`). let base = dir - .canonicalize() - .ok() - .and_then(|p| p.file_name().map(|n| n.to_string_lossy().into_owned())) + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .or_else(|| { + dir.canonicalize() + .ok() + .and_then(|p| p.file_name().map(|n| n.to_string_lossy().into_owned())) + }) .unwrap_or_default(); let derived = slugify(&base); if !is_valid_slug(&derived) { @@ -210,7 +218,13 @@ fn title_from_slug(slug: &str) -> String { /// git still gets a correct `.gitignore` for when it's initialized. fn ensure_gitignored(dir: &Path) -> Result<()> { let path = dir.join(".gitignore"); - let existing = fs::read_to_string(&path).unwrap_or_default(); + // Only a genuinely-absent file is treated as empty; an existing-but- + // unreadable `.gitignore` must error rather than be silently clobbered. + let existing = match fs::read_to_string(&path) { + Ok(s) => s, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => String::new(), + Err(e) => return Err(e).context("reading .gitignore"), + }; if existing.lines().any(|l| l.trim() == GITIGNORE_LINE) { return Ok(()); } diff --git a/crates/picloud-cli/src/linkstate.rs b/crates/picloud-cli/src/linkstate.rs index 829f2b0..e799407 100644 --- a/crates/picloud-cli/src/linkstate.rs +++ b/crates/picloud-cli/src/linkstate.rs @@ -31,6 +31,14 @@ fn plan_path(base: &Path) -> PathBuf { 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()))?; + // Self-ignore: a `.gitignore` of `*` inside `.picloud/` keeps the whole + // dir out of git regardless of the project root's `.gitignore` — so this + // is safe even when reached via `pic plan`/`pull` (which, unlike `init`, + // don't touch the root `.gitignore`). + let ignore = dir.join(".gitignore"); + if !ignore.exists() { + fs::write(&ignore, "*\n").context("writing .picloud/.gitignore")?; + } let link = PlanLink { app: app.to_string(), state_token: state_token.to_string(), @@ -48,8 +56,12 @@ pub fn read_plan(base: &Path) -> Option { 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)); +/// Remove the recorded plan token (best-effort) **iff it belongs to `app`**. +/// Called after a successful apply consumes it, so the next apply requires a +/// fresh plan — without clobbering a token recorded for a different app that +/// happens to share the directory. +pub fn clear_plan(base: &Path, app: &str) { + if read_plan(base).is_some_and(|l| l.app == app) { + let _ = fs::remove_file(plan_path(base)); + } } diff --git a/crates/picloud-cli/tests/init.rs b/crates/picloud-cli/tests/init.rs index 10220e7..bd388bc 100644 --- a/crates/picloud-cli/tests/init.rs +++ b/crates/picloud-cli/tests/init.rs @@ -36,6 +36,25 @@ fn init_scaffolds_a_deployable_project() { ); } +#[test] +fn init_derives_slug_from_a_not_yet_created_dir() { + // The natural `pic init --dir new-project` flow: the target doesn't exist + // yet, and the slug is derived from its name (not via canonicalize, which + // would fail on a missing path). + let parent = TempDir::new().unwrap(); + let target = parent.path().join("new-project"); + pic() + .args(["init", "--dir"]) + .arg(&target) + .assert() + .success(); + let toml = fs::read_to_string(target.join("picloud.toml")).unwrap(); + assert!( + toml.contains("slug = \"new-project\""), + "slug should derive from the dir name:\n{toml}" + ); +} + #[test] fn init_refuses_to_overwrite_without_force() { let dir = TempDir::new().unwrap();