fix(apply): guard app-owned collections + test nearest-group-wins (§11.6 review)
Two review findings:
- F2 (defense-in-depth): validate_bundle_for now rejects `collections` on an
app node — the inverse of the existing group route/trigger guard. The CLI
already prevents it (ManifestApp has no field + deny_unknown_fields), but a
hand-rolled wire Bundle POSTed to /apps/{id}/apply would otherwise insert an
inert app_id-owned group_collections row that no resolver reads.
- F1 (coverage): a journey for nearest-ancestor-group-wins, the CoW-shadowing
guarantee the resolver's `ORDER BY depth LIMIT 1` provides. A parent and a
child group both declare `catalog`; an app under the child writes, and a
second app directly under the parent reads its (separate, empty) store —
proving the deep write stayed in the nearest (child) store. Previously only
the FakeResolver and flat sibling groups were exercised, so the ordering was
untested.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -576,6 +576,19 @@ impl ApplyService {
|
||||
));
|
||||
}
|
||||
}
|
||||
// §11.6: shared collections are owned by GROUPS. Reject them on an app
|
||||
// node — the inverse of the group route/trigger guard above. The CLI
|
||||
// already prevents this (`ManifestApp` has no `collections` field), so
|
||||
// this is defense-in-depth against a hand-rolled wire `Bundle`; without
|
||||
// it the reconcile would insert an inert `app_id`-owned marker row that
|
||||
// no resolver ever reads.
|
||||
if matches!(owner, ApplyOwner::App(_)) && !bundle.collections.is_empty() {
|
||||
return Err(ApplyError::Invalid(
|
||||
"an app manifest cannot declare shared collections — \
|
||||
they are owned by a group"
|
||||
.into(),
|
||||
));
|
||||
}
|
||||
self.validate_bundle(bundle, inherited_endpoints)
|
||||
}
|
||||
|
||||
|
||||
@@ -158,6 +158,111 @@ fn shared_collection_is_read_write_across_the_subtree() {
|
||||
);
|
||||
}
|
||||
|
||||
/// Nearest-ancestor-group-wins (§11.6, the CoW-shadowing guarantee). A parent
|
||||
/// AND a child group both declare `catalog`; an app under the child must bind
|
||||
/// the CHILD's store, not the parent's. Discriminated by a second app directly
|
||||
/// under the parent: it resolves to the PARENT's store, so it must NOT see what
|
||||
/// the deep app wrote. If the resolver's `ORDER BY depth LIMIT 1` regressed to
|
||||
/// the farther group, the shallow app would see the deep app's value.
|
||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||
#[test]
|
||||
fn nearest_declaring_group_wins() {
|
||||
let Some(fx) = common::fixture_or_skip() else {
|
||||
return;
|
||||
};
|
||||
let env = common::admin_env(fx);
|
||||
let root = common::unique_slug("nw-root");
|
||||
let team = common::unique_slug("nw-team");
|
||||
let deep = common::unique_slug("nw-deep");
|
||||
let shallow = common::unique_slug("nw-shallow");
|
||||
|
||||
let _gr = GroupGuard::new(&env.url, &env.token, &root);
|
||||
let _gt = GroupGuard::new(&env.url, &env.token, &team);
|
||||
common::pic_as(&env)
|
||||
.args(["groups", "create", &root])
|
||||
.assert()
|
||||
.success();
|
||||
common::pic_as(&env)
|
||||
.args(["groups", "create", &team, "--parent", &root])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
// BOTH groups declare `catalog` (two distinct stores, same name).
|
||||
let dir = manifest_dir();
|
||||
for g in [&root, &team] {
|
||||
let m = format!("[group]\nslug = \"{g}\"\nname = \"NW\"\n\ncollections = [\"catalog\"]\n");
|
||||
let p = dir.path().join(format!("{g}.toml"));
|
||||
fs::write(&p, &m).unwrap();
|
||||
common::pic_as(&env)
|
||||
.args(["apply", "--file"])
|
||||
.arg(&p)
|
||||
.assert()
|
||||
.success();
|
||||
}
|
||||
|
||||
// `deep` under team, `shallow` directly under root.
|
||||
let _ad = AppGuard::new(&env.url, &env.token, &deep);
|
||||
let _as = AppGuard::new(&env.url, &env.token, &shallow);
|
||||
common::pic_as(&env)
|
||||
.args(["apps", "create", &deep, "--group", &team])
|
||||
.assert()
|
||||
.success();
|
||||
common::pic_as(&env)
|
||||
.args(["apps", "create", &shallow, "--group", &root])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
// The deep app writes — must land in TEAM's store (nearest declarer).
|
||||
fs::write(
|
||||
dir.path().join("scripts/w.rhai"),
|
||||
r#"kv::shared_collection("catalog").set("k", "team-value"); "ok""#,
|
||||
)
|
||||
.unwrap();
|
||||
common::pic_as(&env)
|
||||
.args(["scripts", "deploy"])
|
||||
.arg(dir.path().join("scripts/w.rhai"))
|
||||
.args(["--app", &deep, "--name", "w"])
|
||||
.assert()
|
||||
.success();
|
||||
common::pic_as(&env)
|
||||
.args(["scripts", "invoke", &app_script_id(&env, &deep, "w")])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
// The deep app reads its own write back (team's store).
|
||||
fs::write(
|
||||
dir.path().join("scripts/r.rhai"),
|
||||
r#"kv::shared_collection("catalog").get("k")"#,
|
||||
)
|
||||
.unwrap();
|
||||
common::pic_as(&env)
|
||||
.args(["scripts", "deploy"])
|
||||
.arg(dir.path().join("scripts/r.rhai"))
|
||||
.args(["--app", &deep, "--name", "r"])
|
||||
.assert()
|
||||
.success();
|
||||
assert_eq!(
|
||||
invoke_body(&env, &app_script_id(&env, &deep, "r")),
|
||||
serde_json::json!("team-value"),
|
||||
"the deep app reads back its own write to the nearest (team) store"
|
||||
);
|
||||
|
||||
// The shallow app reads catalog → ROOT's store, which is empty. Seeing
|
||||
// `team-value` here would mean the write leaked to the farther group.
|
||||
common::pic_as(&env)
|
||||
.args(["scripts", "deploy"])
|
||||
.arg(dir.path().join("scripts/r.rhai"))
|
||||
.args(["--app", &shallow, "--name", "r"])
|
||||
.assert()
|
||||
.success();
|
||||
assert_eq!(
|
||||
invoke_body(&env, &app_script_id(&env, &shallow, "r")),
|
||||
serde_json::Value::Null,
|
||||
"the shallow app resolves the ROOT store (empty) — nearest-wins kept the \
|
||||
deep write in the team store"
|
||||
);
|
||||
}
|
||||
|
||||
/// Id of the named script in an app (`pic scripts ls --app <a>`).
|
||||
fn app_script_id(env: &common::TestEnv, app: &str, name: &str) -> String {
|
||||
let ls = common::pic_as(env)
|
||||
|
||||
Reference in New Issue
Block a user