fix(cli): env-aware config, pull clobber guard, strict overlays, 409 hint
Four review fixes on the `pic` surface: - `config --effective` gains `--env`: it now resolves through the same overlay as `plan`/`apply`, so the masked report targets the app those commands act on instead of silently reading the base slug's secrets. - `pull` gains `--force` and refuses to overwrite an existing `picloud.toml` (and colliding `scripts/*.rhai`) without it — fail-fast before any network call or write, mirroring `init`. - Overlays (`ManifestOverlay`/`OverlayApp`) get `deny_unknown_fields`: a `[[scripts]]`/`[[routes]]`/`[[triggers]]` table or a typo'd key in an overlay now errors loudly instead of being silently dropped. +unit test. - `apply` 409 (StateMoved — the only 409 the endpoint emits) surfaces an actionable hint (re-run `pic plan`, or `--force`) instead of a bare `HTTP 409`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -933,6 +933,19 @@ impl Client {
|
|||||||
.json(&body)
|
.json(&body)
|
||||||
.send()
|
.send()
|
||||||
.await?;
|
.await?;
|
||||||
|
// The apply endpoint returns 409 only for a stale bound plan
|
||||||
|
// (`StateMoved`): the app changed since `pic plan` recorded its
|
||||||
|
// token. Surface an actionable next step instead of a bare
|
||||||
|
// `HTTP 409`.
|
||||||
|
if resp.status() == reqwest::StatusCode::CONFLICT {
|
||||||
|
let body = resp.text().await.unwrap_or_default();
|
||||||
|
let msg = parse_error_body(&body).unwrap_or(body);
|
||||||
|
return Err(anyhow!(
|
||||||
|
"{msg}\nThe app changed since your last `pic plan`. Re-run \
|
||||||
|
`pic plan` to review the new diff, then `pic apply` — or \
|
||||||
|
`pic apply --force` to apply without re-reviewing."
|
||||||
|
));
|
||||||
|
}
|
||||||
decode(resp).await
|
decode(resp).await
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -18,13 +18,21 @@ use crate::config;
|
|||||||
use crate::manifest::Manifest;
|
use crate::manifest::Manifest;
|
||||||
use crate::output::{OutputMode, Table};
|
use crate::output::{OutputMode, Table};
|
||||||
|
|
||||||
pub async fn run(manifest_path: &Path, effective: bool, mode: OutputMode) -> Result<()> {
|
pub async fn run(
|
||||||
|
manifest_path: &Path,
|
||||||
|
effective: bool,
|
||||||
|
env: Option<&str>,
|
||||||
|
mode: OutputMode,
|
||||||
|
) -> Result<()> {
|
||||||
if !effective {
|
if !effective {
|
||||||
bail!("`pic config` currently supports only `--effective`");
|
bail!("`pic config` currently supports only `--effective`");
|
||||||
}
|
}
|
||||||
let creds = config::resolve()?;
|
let creds = config::resolve()?;
|
||||||
let client = Client::from_creds(&creds)?;
|
let client = Client::from_creds(&creds)?;
|
||||||
let manifest = Manifest::load(manifest_path)?;
|
// Resolve against the same env overlay as `pic plan`/`apply` so the
|
||||||
|
// masked report targets the app those commands would act on — not the
|
||||||
|
// base slug — when an overlay re-points slug/secrets.
|
||||||
|
let manifest = Manifest::load_with_env(manifest_path, env)?;
|
||||||
|
|
||||||
let declared: BTreeSet<String> = manifest.secrets.names.iter().cloned().collect();
|
let declared: BTreeSet<String> = manifest.secrets.names.iter().cloned().collect();
|
||||||
let on_server: BTreeSet<String> = client
|
let on_server: BTreeSet<String> = client
|
||||||
|
|||||||
@@ -23,10 +23,24 @@ use crate::manifest::{
|
|||||||
};
|
};
|
||||||
use crate::output::{KvBlock, OutputMode};
|
use crate::output::{KvBlock, OutputMode};
|
||||||
|
|
||||||
pub async fn run(app_ident: &str, dir: &Path, mode: OutputMode) -> Result<()> {
|
pub async fn run(app_ident: &str, dir: &Path, force: bool, mode: OutputMode) -> Result<()> {
|
||||||
let creds = config::resolve()?;
|
let creds = config::resolve()?;
|
||||||
let client = Client::from_creds(&creds)?;
|
let client = Client::from_creds(&creds)?;
|
||||||
|
|
||||||
|
// Refuse to clobber an existing project (mirrors `pic init`). `pull`
|
||||||
|
// overwrites `picloud.toml` and every colliding `scripts/*.rhai`, so a
|
||||||
|
// stray `pic pull <wrong-app>` in a populated dir would destroy local
|
||||||
|
// edits. Fail fast — before any network call or file write — unless the
|
||||||
|
// operator opted in with `--force`.
|
||||||
|
let manifest_path = dir.join(MANIFEST_FILE);
|
||||||
|
if !force && manifest_path.exists() {
|
||||||
|
anyhow::bail!(
|
||||||
|
"{} already exists; refusing to overwrite. Re-run with --force to \
|
||||||
|
replace it (and any colliding scripts/*.rhai).",
|
||||||
|
manifest_path.display()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// One GET per resource kind (routes are per-script, below).
|
// One GET per resource kind (routes are per-script, below).
|
||||||
let app = client.apps_get(app_ident).await?;
|
let app = client.apps_get(app_ident).await?;
|
||||||
let scripts = client.scripts_list_by_app(app_ident).await?;
|
let scripts = client.scripts_list_by_app(app_ident).await?;
|
||||||
@@ -189,7 +203,6 @@ pub async fn run(app_ident: &str, dir: &Path, mode: OutputMode) -> Result<()> {
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
let manifest_path = dir.join(MANIFEST_FILE);
|
|
||||||
std::fs::write(&manifest_path, manifest.to_toml()?)
|
std::fs::write(&manifest_path, manifest.to_toml()?)
|
||||||
.with_context(|| format!("writing {}", manifest_path.display()))?;
|
.with_context(|| format!("writing {}", manifest_path.display()))?;
|
||||||
|
|
||||||
|
|||||||
@@ -224,6 +224,9 @@ struct PullArgs {
|
|||||||
/// Directory to write `picloud.toml` + `scripts/` into.
|
/// Directory to write `picloud.toml` + `scripts/` into.
|
||||||
#[arg(long, default_value = ".")]
|
#[arg(long, default_value = ".")]
|
||||||
dir: PathBuf,
|
dir: PathBuf,
|
||||||
|
/// Overwrite an existing `picloud.toml` (and colliding `scripts/*.rhai`).
|
||||||
|
#[arg(long)]
|
||||||
|
force: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Args)]
|
#[derive(Args)]
|
||||||
@@ -250,6 +253,10 @@ struct ConfigArgs {
|
|||||||
/// Show the effective (resolved) config with secrets masked.
|
/// Show the effective (resolved) config with secrets masked.
|
||||||
#[arg(long)]
|
#[arg(long)]
|
||||||
effective: bool,
|
effective: bool,
|
||||||
|
/// Resolve against the `picloud.<env>.toml` overlay (per-env slug +
|
||||||
|
/// secrets), matching `pic plan --env` / `pic apply --env`.
|
||||||
|
#[arg(long)]
|
||||||
|
env: Option<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Subcommand)]
|
#[derive(Subcommand)]
|
||||||
@@ -1151,8 +1158,10 @@ async fn main() -> ExitCode {
|
|||||||
.await
|
.await
|
||||||
}
|
}
|
||||||
Cmd::Plan(args) => cmds::plan::run(&args.file, args.env.as_deref(), mode).await,
|
Cmd::Plan(args) => cmds::plan::run(&args.file, args.env.as_deref(), mode).await,
|
||||||
Cmd::Pull(args) => cmds::pull::run(&args.app, &args.dir, mode).await,
|
Cmd::Pull(args) => cmds::pull::run(&args.app, &args.dir, args.force, mode).await,
|
||||||
Cmd::Config(args) => cmds::config::run(&args.file, args.effective, mode).await,
|
Cmd::Config(args) => {
|
||||||
|
cmds::config::run(&args.file, args.effective, args.env.as_deref(), mode).await
|
||||||
|
}
|
||||||
Cmd::Init(args) => cmds::init::run(
|
Cmd::Init(args) => cmds::init::run(
|
||||||
&args.dir,
|
&args.dir,
|
||||||
args.slug.as_deref(),
|
args.slug.as_deref(),
|
||||||
|
|||||||
@@ -112,7 +112,13 @@ fn overlay_path(base_path: &Path, env: &str) -> std::path::PathBuf {
|
|||||||
|
|
||||||
/// A sparse per-environment overlay (`picloud.<env>.toml`). Only the fields
|
/// A sparse per-environment overlay (`picloud.<env>.toml`). Only the fields
|
||||||
/// that vary per environment today — slug/name and secret names.
|
/// that vary per environment today — slug/name and secret names.
|
||||||
|
///
|
||||||
|
/// `deny_unknown_fields`: an overlay can carry *only* `[app]` and `[secrets]`.
|
||||||
|
/// Scripts/routes/triggers belong in the shared base manifest, so a
|
||||||
|
/// `[[scripts]]`/`[[routes]]`/`[[triggers]]` table (or a typo'd key) in an
|
||||||
|
/// overlay is a mistake — error loudly rather than silently dropping it.
|
||||||
#[derive(Debug, Clone, Default, Deserialize)]
|
#[derive(Debug, Clone, Default, Deserialize)]
|
||||||
|
#[serde(deny_unknown_fields)]
|
||||||
pub struct ManifestOverlay {
|
pub struct ManifestOverlay {
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub app: OverlayApp,
|
pub app: OverlayApp,
|
||||||
@@ -121,6 +127,7 @@ pub struct ManifestOverlay {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Clone, Default, Deserialize)]
|
#[derive(Debug, Clone, Default, Deserialize)]
|
||||||
|
#[serde(deny_unknown_fields)]
|
||||||
pub struct OverlayApp {
|
pub struct OverlayApp {
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub slug: Option<String>,
|
pub slug: Option<String>,
|
||||||
@@ -461,6 +468,26 @@ mod tests {
|
|||||||
assert_eq!(m.scripts.len(), 2);
|
assert_eq!(m.scripts.len(), 2);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn overlay_rejects_non_overlay_tables() {
|
||||||
|
// An overlay carries only [app]/[secrets]. Scripts/routes/triggers
|
||||||
|
// belong in the shared base, so a `[[scripts]]` table (or a typo'd
|
||||||
|
// key) in an overlay must error loudly, not be silently dropped.
|
||||||
|
let err = toml::from_str::<ManifestOverlay>(
|
||||||
|
"[app]\nslug = \"blog-staging\"\n\n\
|
||||||
|
[[scripts]]\nname = \"hello\"\nfile = \"scripts/hello.rhai\"\n",
|
||||||
|
)
|
||||||
|
.expect_err("overlay with [[scripts]] must be rejected");
|
||||||
|
assert!(
|
||||||
|
err.to_string().contains("scripts") || err.to_string().contains("unknown"),
|
||||||
|
"error should point at the offending table: {err}"
|
||||||
|
);
|
||||||
|
|
||||||
|
// Typo'd key inside [app] is likewise rejected.
|
||||||
|
toml::from_str::<ManifestOverlay>("[app]\nslag = \"oops\"\n")
|
||||||
|
.expect_err("overlay with a typo'd [app] key must be rejected");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn overlay_path_derivation() {
|
fn overlay_path_derivation() {
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
Reference in New Issue
Block a user