fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(project-tool): address review findings on the apply foundation

Browse Source 
Remediation of the single-app reconcile foundation after two independent
review passes. No new feature surface — closes correctness, parity, and
safety gaps in pull/plan/apply/prune.

apply engine (manager-core):
- validate_bundle reached parity with the interactive trigger API: reject
  an empty kv/docs/files collection_glob and a malformed pubsub
  topic_pattern (previously written and silently never matched), and lift
  the queue visibility floor to MIN_QUEUE_VISIBILITY_TIMEOUT_SECS (30) —
  apply had accepted a [5,29] value the dashboard refuses. Per-kind checks
  extracted into a pure, unit-tested validate_trigger_shape.
- An omitted script `description` now means "leave as-is" (matching the
  other optional fields and the function's own documented contract)
  instead of clearing the stored value.
- Doc fixes: drop stale "next milestone" notes; record the one deliberate
  plan/apply divergence (set-but-empty secret); document delete_route_tx
  as intentionally idempotent for reconcile.

CLI:
- Gate destructive `apply --prune` behind confirmation: interactive y/N,
  or `--yes` for CI; a non-interactive prune without `--yes` refuses
  rather than silently deleting. Journey tests pass `--yes`; a new test
  proves the gate refuses and deletes nothing.
- Harden pull's filename safety: reject all control characters and the
  Unicode bidi-override / zero-width chars used for terminal/filename
  spoofing, and cap length at 200 bytes (NAME_MAX headroom).

Tested: manager-core lib (incl. queue-floor + sparse-description parity)
+ CLI bins + 9 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:
MechaCat02
2026-06-22 21:18:51 +02:00
parent 3b650a2b14
commit d9b3e9973c
8 changed files with 364 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

246
crates/manager-core/src/apply_service.rs
View File

@@ -347,8 +347,8 @@ pub enum ApplyError {
// Service
// ----------------------------------------------------------------------------
/// Reconcile engine. Holds trait-object repos for the read/diff path;
/// the transactional write path (next milestone) reuses the same handles.
/// Reconcile engine. Holds trait-object repos for the read/diff path; the
/// transactional write path (`apply`) reuses the same handles plus `pool`.
#[derive(Clone)]
pub struct ApplyService {
/// Pool for the transactional write path (`apply`).
@@ -385,9 +385,9 @@ impl ApplyService {
Ok(compute_diff(&current, bundle))
}
/// Reconcile app `app_id` to `bundle` in a single transaction.
/// Additive: creates and updates are applied; deletions are surfaced
/// in the diff but only executed when prune is set (next milestone).
/// Reconcile app `app_id` to `bundle` in a single transaction. Creates
/// and updates are always applied; deletions of resources absent from the
/// bundle are executed only when `prune` is set (secrets are never pruned).
///
/// # Errors
/// `Invalid` for a bad bundle; `Backend` for repo/transaction failures.
@@ -471,7 +471,10 @@ impl ApplyService {
let cur = current_scripts[&key];
let patch = ScriptPatch {
name: None,
description: Some(bs.description.clone()),
// Sparse: `None` (omitted in the manifest) leaves the
// stored description untouched; `Some(text)` sets it.
// Mirrors `script_update_reason`'s leave-as-is rule.
description: bs.description.clone().map(Some),
source: Some(bs.source.clone()),
timeout_seconds: bs.timeout_seconds,
memory_limit_mb: bs.memory_limit_mb,
@@ -909,49 +912,7 @@ impl ApplyService {
t.script()
)));
}
match t {
BundleTrigger::Email {
inbound_secret_ref, ..
} if inbound_secret_ref.trim().is_empty() => {
return Err(ApplyError::Invalid(format!(
"email trigger on `{}` needs inbound_secret_ref (a secret name)",
t.script()
)));
}
BundleTrigger::Queue {
queue_name,
visibility_timeout_secs,
..
} => {
if queue_name.trim().is_empty() {
return Err(ApplyError::Invalid("queue trigger needs queue_name".into()));
}
if let Some(v) = visibility_timeout_secs {
if !(5..=3600).contains(v) {
return Err(ApplyError::Invalid(format!(
"queue `{queue_name}`: visibility_timeout_secs must be in [5, 3600]"
)));
}
}
}
BundleTrigger::Cron {
schedule, timezone, ..
} => {
crate::cron_scheduler::validate_schedule(schedule).map_err(|e| {
ApplyError::Invalid(format!(
"cron trigger on `{}`: invalid schedule: {e}",
t.script()
))
})?;
crate::cron_scheduler::validate_timezone(timezone).map_err(|e| {
ApplyError::Invalid(format!(
"cron trigger on `{}`: invalid timezone: {e}",
t.script()
))
})?;
}
_ => {}
}
validate_trigger_shape(t)?;
let id = t.identity();
if !trig_keys.insert(id.clone()) {
return Err(ApplyError::Invalid(format!("duplicate trigger `{id}`")));
@@ -1084,8 +1045,13 @@ fn script_update_reason(cur: &Script, desired: &BundleScript) -> Option<String>
if cur.kind != desired.kind {
return Some("kind changed".into());
}
if cur.description != desired.description {
return Some("description changed".into());
// Sparse like the other optional fields: an omitted (`None`) description
// means "leave as-is", so only an explicitly-set value that differs
// forces an update. (Clearing a description is done in the dashboard.)
if let Some(d) = &desired.description {
if cur.description.as_ref() != Some(d) {
return Some("description changed".into());
}
}
if let Some(t) = desired.timeout_seconds {
if i64::from(cur.timeout_seconds) != i64::from(t) {
@@ -1286,6 +1252,12 @@ fn diff_secrets(current: &CurrentState, bundle: &Bundle) -> Vec<ResourceChange>
/// Reject any email trigger whose referenced secret isn't set, so `plan` and
/// `apply` give the same answer (apply also fails in `resolve_and_seal`, but
/// only after taking the lock — surfacing it here keeps plan honest).
///
/// This checks the secret *reference exists*. `resolve_and_seal` additionally
/// requires the resolved value to be a non-empty string; `plan` deliberately
/// does not decrypt secrets, so a reference to a set-but-empty secret passes
/// `plan` and is caught at `apply` — the one residual plan/apply divergence,
/// and a deliberate one (plan stays read-only and crypto-free).
fn validate_email_secrets_present(
bundle: &Bundle,
secret_names: &[String],
@@ -1396,6 +1368,89 @@ fn reject_reserved_path(path: &str) -> Result<(), ApplyError> {
Ok(())
}
/// Per-kind structural validation for one desired trigger — kept at parity
/// with the interactive trigger API so `apply` can't write a trigger the
/// dashboard would have rejected. Pure (no live state), so it's unit-tested
/// directly. The script-binding checks live in `validate_bundle`.
fn validate_trigger_shape(t: &BundleTrigger) -> Result<(), ApplyError> {
match t {
BundleTrigger::Email {
inbound_secret_ref, ..
} if inbound_secret_ref.trim().is_empty() => {
return Err(ApplyError::Invalid(format!(
"email trigger on `{}` needs inbound_secret_ref (a secret name)",
t.script()
)));
}
BundleTrigger::Queue {
queue_name,
visibility_timeout_secs,
..
} => {
if queue_name.trim().is_empty() {
return Err(ApplyError::Invalid("queue trigger needs queue_name".into()));
}
if let Some(v) = visibility_timeout_secs {
// Parity with the interactive API: the floor is the dispatcher
// tick/reclaim-cadence minimum (`triggers_api`), and the ceiling
// matches `trigger_repo`'s queue check. Apply previously allowed
// a [5, 29] floor the dashboard rejects.
let min = crate::triggers_api::MIN_QUEUE_VISIBILITY_TIMEOUT_SECS;
if !(min..=3600).contains(v) {
return Err(ApplyError::Invalid(format!(
"queue `{queue_name}`: visibility_timeout_secs must be in [{min}, 3600]"
)));
}
}
}
BundleTrigger::Cron {
schedule, timezone, ..
} => {
crate::cron_scheduler::validate_schedule(schedule).map_err(|e| {
ApplyError::Invalid(format!(
"cron trigger on `{}`: invalid schedule: {e}",
t.script()
))
})?;
crate::cron_scheduler::validate_timezone(timezone).map_err(|e| {
ApplyError::Invalid(format!(
"cron trigger on `{}`: invalid timezone: {e}",
t.script()
))
})?;
}
// Parity with the interactive trigger API, which rejects an empty
// collection_glob (`create_{kv,docs,files}_trigger`).
BundleTrigger::Kv {
collection_glob, ..
}
| BundleTrigger::Docs {
collection_glob, ..
}
| BundleTrigger::Files {
collection_glob, ..
} if collection_glob.trim().is_empty() => {
return Err(ApplyError::Invalid(format!(
"{} trigger on `{}` needs a non-empty collection_glob",
t.kind_str(),
t.script()
)));
}
// Parity with `create_pubsub_trigger`'s topic-pattern check — otherwise
// a malformed pattern is written and silently never matches at dispatch.
BundleTrigger::Pubsub { topic_pattern, .. } => {
picloud_shared::validate_topic_pattern(topic_pattern).map_err(|e| {
ApplyError::Invalid(format!(
"pubsub trigger on `{}`: invalid topic_pattern: {e}",
t.script()
))
})?;
}
_ => {}
}
Ok(())
}
/// Summary of what an `apply` changed.
#[derive(Debug, Default, Serialize)]
pub struct ApplyReport {
@@ -1731,6 +1786,95 @@ mod tests {
assert_eq!(compute_diff(&current, &chg2).scripts[0].op, Op::Update);
}
#[test]
fn description_is_sparse() {
// An omitted (`None`) description must mean "leave as-is", matching
// the other optional fields — not "clear it".
let mut cur = script("a", "let x = 1;");
cur.description = Some("kept".into());
let current = CurrentState {
scripts: vec![cur],
..CurrentState::default()
};
// Omitted → NoOp (leave the stored description alone).
let mut omit = empty_bundle();
omit.scripts = vec![bundle_script("a", "let x = 1;")];
assert_eq!(compute_diff(&current, &omit).scripts[0].op, Op::NoOp);
// Same explicit value → NoOp.
let mut same = empty_bundle();
let mut s = bundle_script("a", "let x = 1;");
s.description = Some("kept".into());
same.scripts = vec![s];
assert_eq!(compute_diff(&current, &same).scripts[0].op, Op::NoOp);
// Different explicit value → Update.
let mut chg = empty_bundle();
let mut s2 = bundle_script("a", "let x = 1;");
s2.description = Some("changed".into());
chg.scripts = vec![s2];
assert_eq!(compute_diff(&current, &chg).scripts[0].op, Op::Update);
}
#[test]
fn trigger_shape_validation_matches_interactive_api() {
// Empty collection_glob is rejected (kv/docs/files).
assert!(validate_trigger_shape(&BundleTrigger::Kv {
script: "h".into(),
collection_glob: " ".into(),
ops: vec![],
dispatch_mode: None,
retry_max_attempts: None,
})
.is_err());
// A non-empty glob passes.
assert!(validate_trigger_shape(&BundleTrigger::Docs {
script: "h".into(),
collection_glob: "users".into(),
ops: vec![],
dispatch_mode: None,
retry_max_attempts: None,
})
.is_ok());
// A malformed pubsub topic_pattern (mid-pattern wildcard) is rejected;
// a valid one passes.
assert!(validate_trigger_shape(&BundleTrigger::Pubsub {
script: "h".into(),
topic_pattern: "user.*.created".into(),
dispatch_mode: None,
retry_max_attempts: None,
})
.is_err());
assert!(validate_trigger_shape(&BundleTrigger::Pubsub {
script: "h".into(),
topic_pattern: "orders.created".into(),
dispatch_mode: None,
retry_max_attempts: None,
})
.is_ok());
// Queue visibility floor matches the interactive API (>= 30): a value
// below the floor is rejected; the floor itself and `None` are ok.
let queue = |vis| BundleTrigger::Queue {
script: "h".into(),
queue_name: "jobs".into(),
visibility_timeout_secs: vis,
dispatch_mode: None,
retry_max_attempts: None,
};
assert!(validate_trigger_shape(&queue(Some(10))).is_err());
assert!(validate_trigger_shape(&queue(Some(
crate::triggers_api::MIN_QUEUE_VISIBILITY_TIMEOUT_SECS
)))
.is_ok());
assert!(validate_trigger_shape(&queue(None)).is_ok());
// Empty email inbound_secret_ref is rejected.
assert!(validate_trigger_shape(&BundleTrigger::Email {
script: "h".into(),
inbound_secret_ref: " ".into(),
dispatch_mode: None,
retry_max_attempts: None,
})
.is_err());
}
#[test]
fn trigger_diff_create_noop_delete() {
let s = script("h", "x");

6
crates/manager-core/src/route_repo.rs
View File

@@ -202,6 +202,12 @@ pub(crate) async fn insert_route_tx(
}
/// Delete a route by id within an existing transaction.
///
/// Unlike the non-tx [`RouteRepository::delete`], this is intentionally
/// idempotent: a missing row is not an error. The only caller is the
/// reconcile engine (`ApplyService`), where "delete a route already gone"
/// (e.g. removed out-of-band between the diff read and the write) is a
/// no-op to converge on, not a failure to roll back the whole apply.
pub(crate) async fn delete_route_tx(
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
route_id: Uuid,

2
crates/manager-core/src/triggers_api.rs
View File

@@ -38,7 +38,7 @@ use crate::trigger_repo::{
/// in practice (the dispatcher itself ticks every 100ms; reclaim ticks
/// every 30s; an executor needs more wall-clock than this to do useful
/// work). Reject below this with a 422 + actionable error.
const MIN_QUEUE_VISIBILITY_TIMEOUT_SECS: u32 = 30;
pub(crate) const MIN_QUEUE_VISIBILITY_TIMEOUT_SECS: u32 = 30;
/// Default soft-warning ceiling when `PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC`
/// is unset. Re-exported from the dispatcher so the single source of