fix(project-tool): address independent review of init + staleness
Remediation after an independent review of the two feature commits. pic init: - Fix the headline `pic init --dir <new-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) <noreply@anthropic.com>
This commit is contained in:
@@ -445,9 +445,13 @@ impl ApplyService {
|
|||||||
|
|
||||||
let current = self.load_current(app_id).await?;
|
let current = self.load_current(app_id).await?;
|
||||||
// Bound-plan check (§4.2): if the caller passed the token from a prior
|
// 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
|
// `pic plan`, refuse when the live state changed since. Computed from
|
||||||
// under the apply lock so it reflects what we're about to write. Done
|
// the same `load_current` snapshot the diff uses, before any mutation
|
||||||
// before any mutation so a stale apply rolls back nothing.
|
// (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 let Some(expected) = expected_token {
|
||||||
if state_token(¤t) != expected {
|
if state_token(¤t) != expected {
|
||||||
return Err(ApplyError::StateMoved);
|
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
|
/// A stable fingerprint of an app's live state, covering exactly what the
|
||||||
/// reconcile diff keys on: script identity + write-counter (`version` bumps on
|
/// reconcile diff keys on: script identity + write-counter (`version` bumps on
|
||||||
/// any script edit), route identity + binding/attrs, trigger membership +
|
/// any script edit), route identity + binding/attrs, trigger semantic identity
|
||||||
/// `enabled`, and secret names. Any out-of-band change to those flips the
|
/// (via `current_trigger_identity`, so dead-letter triggers — which the diff
|
||||||
/// token, so a `plan`-then-`apply` can detect that the app moved underneath it.
|
/// 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
|
/// 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
|
/// 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(""),
|
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<ScriptId, String> = current
|
||||||
|
.scripts
|
||||||
|
.iter()
|
||||||
|
.map(|s| (s.id, s.name.clone()))
|
||||||
|
.collect();
|
||||||
for t in ¤t.triggers {
|
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 {
|
for n in ¤t.secret_names {
|
||||||
parts.push(format!("k|{n}"));
|
parts.push(format!("k|{n}"));
|
||||||
@@ -1997,6 +2017,36 @@ mod tests {
|
|||||||
assert_ne!(state_token(&base), state_token(&with_secret));
|
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]
|
#[test]
|
||||||
fn trigger_diff_create_noop_delete() {
|
fn trigger_diff_create_noop_delete() {
|
||||||
let s = script("h", "x");
|
let s = script("h", "x");
|
||||||
|
|||||||
@@ -52,7 +52,7 @@ pub async fn run(
|
|||||||
expected_token.as_deref(),
|
expected_token.as_deref(),
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
crate::linkstate::clear_plan(base_dir);
|
crate::linkstate::clear_plan(base_dir, &manifest.app.slug);
|
||||||
|
|
||||||
let mut block = KvBlock::new();
|
let mut block = KvBlock::new();
|
||||||
block
|
block
|
||||||
|
|||||||
@@ -147,10 +147,18 @@ fn resolve_slug(dir: &Path, slug_arg: Option<&str>) -> Result<String> {
|
|||||||
}
|
}
|
||||||
return Ok(s.to_string());
|
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
|
let base = dir
|
||||||
.canonicalize()
|
.file_name()
|
||||||
.ok()
|
.map(|n| n.to_string_lossy().into_owned())
|
||||||
.and_then(|p| p.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();
|
.unwrap_or_default();
|
||||||
let derived = slugify(&base);
|
let derived = slugify(&base);
|
||||||
if !is_valid_slug(&derived) {
|
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.
|
/// git still gets a correct `.gitignore` for when it's initialized.
|
||||||
fn ensure_gitignored(dir: &Path) -> Result<()> {
|
fn ensure_gitignored(dir: &Path) -> Result<()> {
|
||||||
let path = dir.join(".gitignore");
|
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) {
|
if existing.lines().any(|l| l.trim() == GITIGNORE_LINE) {
|
||||||
return Ok(());
|
return Ok(());
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -31,6 +31,14 @@ fn plan_path(base: &Path) -> PathBuf {
|
|||||||
pub fn write_plan(base: &Path, app: &str, state_token: &str) -> Result<()> {
|
pub fn write_plan(base: &Path, app: &str, state_token: &str) -> Result<()> {
|
||||||
let dir = base.join(DIR);
|
let dir = base.join(DIR);
|
||||||
fs::create_dir_all(&dir).with_context(|| format!("creating {}", dir.display()))?;
|
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 {
|
let link = PlanLink {
|
||||||
app: app.to_string(),
|
app: app.to_string(),
|
||||||
state_token: state_token.to_string(),
|
state_token: state_token.to_string(),
|
||||||
@@ -48,8 +56,12 @@ pub fn read_plan(base: &Path) -> Option<PlanLink> {
|
|||||||
serde_json::from_slice(&body).ok()
|
serde_json::from_slice(&body).ok()
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Remove the recorded plan token (best-effort). Called after a successful
|
/// Remove the recorded plan token (best-effort) **iff it belongs to `app`**.
|
||||||
/// apply consumes it, so the next apply requires a fresh plan.
|
/// Called after a successful apply consumes it, so the next apply requires a
|
||||||
pub fn clear_plan(base: &Path) {
|
/// fresh plan — without clobbering a token recorded for a different app that
|
||||||
let _ = fs::remove_file(plan_path(base));
|
/// 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));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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]
|
#[test]
|
||||||
fn init_refuses_to_overwrite_without_force() {
|
fn init_refuses_to_overwrite_without_force() {
|
||||||
let dir = TempDir::new().unwrap();
|
let dir = TempDir::new().unwrap();
|
||||||
|
|||||||
Reference in New Issue
Block a user