From b53eabb786c17086e849e5e460f48b431321fb68 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Fri, 26 Jun 2026 07:22:25 +0200 Subject: [PATCH] =?UTF-8?q?feat(modules):=20lexical=20import=20resolver=20?= =?UTF-8?q?=E2=80=94=20seal=20imports=20to=20defining=20node=20(Phase=204b?= =?UTF-8?q?=20C3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make `import` resolution lexical (§5.5): an inherited group script's imports resolve against the module set at its OWN defining node, walking up from there — a leaf app can't shadow them, and a group script behaves identically under every inheriting app. Mechanism — Rhai's `_source` carries the importing AST's origin tag: - The entry script has no source tag → resolve from `default_origin` (its defining node, threaded in via C2). - Each resolved module's AST source is set to `encode(module.owner())` before `eval_ast_as_new`, so its nested `import`s carry the module's own node as `_source` and resolve from there — lexical chaining. - `encode_origin`/`decode_origin` map a `ScriptOwner` to/from `app:` / `group:`. Cache rekeyed from `(app_id, name)` to the resolved `ScriptId`: a compiled module is lexically self-contained, so its body is a pure function of `(script_id, updated_at)` — this also dedupes a shared group module across inheriting apps and keeps same-named cross-app modules distinct. Adds two executor-core unit tests (origin-keyed fake) proving the trust boundary: a group module's import binds the group's module even when an app-owned module of the same name exists; `ScriptOwner` gains `Hash`. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/executor-core/src/module_resolver.rs | 119 +++++++++++++----- crates/executor-core/tests/modules.rs | 133 +++++++++++++++++++- crates/shared/src/script.rs | 2 +- 3 files changed, 217 insertions(+), 37 deletions(-) diff --git a/crates/executor-core/src/module_resolver.rs b/crates/executor-core/src/module_resolver.rs index e5a09b2..53dcb8b 100644 --- a/crates/executor-core/src/module_resolver.rs +++ b/crates/executor-core/src/module_resolver.rs @@ -1,25 +1,33 @@ -//! `PicloudModuleResolver` — the v1.1.3 per-app Rhai module resolver. +//! `PicloudModuleResolver` — the Rhai module resolver (v1.1.3; made +//! origin-aware / lexical in v1.2 Phase 4b). //! //! Replaces `DummyModuleResolver` in `Engine::build_engine`. Constructed -//! fresh per `Engine::execute` call: holds an `Arc` so every -//! `import ""` request resolves against the calling app -//! (`cx.app_id`). The script-side `name` argument carries no `app_id` -//! — that's the load-bearing cross-app isolation property. +//! fresh per `Engine::execute` call. +//! +//! **Lexical resolution (§5.5).** An `import ""` resolves against the +//! module set visible at the **importing script's own defining node**, +//! walking up its group chain — not the inheriting app's effective view. +//! The importing node is read from Rhai's `_source` (the importing AST's +//! source tag, which the resolver sets to each module's owner) and falls +//! back to `default_origin` (the entry script's defining node) for the +//! top-level AST. So an inherited group script's imports seal to the group +//! (a leaf can't shadow them — the trust boundary), while every SDK call +//! *inside* a module still scopes to the inheriting app via `cx.app_id`. //! //! Three runtime invariants are enforced: //! -//! 1. **Cross-app isolation** — `ModuleSource::lookup` is called with -//! `&cx`; the Postgres impl scopes by `cx.app_id` (never by a -//! script-passed argument). +//! 1. **Lexical isolation** — `ModuleSource::resolve` is keyed by the +//! importing node's owner (a trusted dispatch-derived value), never a +//! script-passed argument. //! 2. **Cycle detection** — an in-progress-imports stack rejects //! `A → B → A` with `ErrorInModule(... circular import detected ...)`. //! 3. **Depth limit** — guards against deep but acyclic chains //! (default 8, override via `PICLOUD_MODULE_IMPORT_DEPTH_MAX`). //! -//! Compiled modules are cached per `(app_id, name)` and invalidated by -//! `updated_at` change — no explicit pub/sub. The cache is owned by -//! `Engine` and shared across calls; only the resolver state (stack, -//! depth) is per-call. +//! Compiled modules are cached by resolved `ScriptId` (a compiled module +//! is lexically self-contained) and invalidated by `updated_at` change — +//! no explicit pub/sub. The cache is owned by `Engine` and shared across +//! calls; only the resolver state (stack, depth) is per-call. use std::num::NonZeroUsize; use std::sync::{Arc, Mutex}; @@ -27,7 +35,7 @@ use std::sync::{Arc, Mutex}; use chrono::{DateTime, Utc}; use lru::LruCache; use picloud_shared::{ - AppId, ModuleSource, ModuleSourceError, ScriptOwner, SdkCallCx, ValidatedScript, + ModuleSource, ModuleSourceError, ScriptId, ScriptOwner, SdkCallCx, ValidatedScript, }; use rhai::module_resolvers::ModuleResolver; use rhai::{Engine as RhaiEngine, EvalAltResult, Module, Position, Shared, AST}; @@ -37,10 +45,38 @@ use rhai::{Engine as RhaiEngine, EvalAltResult, Module, Position, Shared, AST}; /// `sync` feature that the workspace pins. type SharedRhaiModule = Shared; -/// Cache key: `(app_id, module name)`. v1.1.3 enforces module names as -/// a conservative identifier shape (migration 0015 `scripts_module_name_shape` -/// CHECK) so the `String` here is bounded by ~64 bytes. -pub type ModuleCacheKey = (AppId, String); +/// Cache key: the resolved module's `ScriptId` (Phase 4b). A compiled +/// module is lexically self-contained (its nested imports resolve from +/// its own defining node, baked in at compile time), so the *body* is a +/// pure function of `(script_id, updated_at)` — independent of which +/// script imported it. Keying by id also dedupes a shared group module +/// across every inheriting app, and keeps cross-app modules of the same +/// name distinct (distinct ids). +pub type ModuleCacheKey = ScriptId; + +/// Encode a defining node as an AST `source` tag (`app:` / +/// `group:`). Set on a resolved module's AST so its own nested +/// `import`s carry *its* node as Rhai's `_source` — the lexical chaining +/// that seals each module's imports to where it was authored (§5.5). +fn encode_origin(owner: ScriptOwner) -> String { + match owner { + ScriptOwner::App(a) => format!("app:{a}"), + ScriptOwner::Group(g) => format!("group:{g}"), + } +} + +/// Inverse of [`encode_origin`]. `None` for an unrecognised tag (e.g. a +/// source set by something other than this resolver) — the caller then +/// falls back to the resolver's `default_origin`. +fn decode_origin(s: &str) -> Option { + let (kind, id) = s.split_once(':')?; + let uuid = uuid::Uuid::parse_str(id).ok()?; + match kind { + "app" => Some(ScriptOwner::App(uuid.into())), + "group" => Some(ScriptOwner::Group(uuid.into())), + _ => None, + } +} /// Cache value: the freshness comparator + the compiled module Rhai /// hands to importing scripts. Cloning the `Shared` is an Arc bump. @@ -67,17 +103,19 @@ pub fn new_module_cache(capacity: usize) -> Arc { /// The v1.1.3 module resolver. One per `Engine::execute` call. pub struct PicloudModuleResolver { - /// Backend the resolver consults for `(app_id, name)`. The bridge - /// runs Rhai's sync `resolve()` and the async `lookup()` together - /// via `tokio::runtime::Handle::block_on(...)` — safe because + /// Backend the resolver consults to resolve a module name against an + /// origin node's chain. The bridge runs Rhai's sync `resolve()` and the + /// async `ModuleSource::resolve()` together via + /// `tokio::runtime::Handle::block_on(...)` — safe because /// `LocalExecutorClient` runs `Engine::execute` inside /// `spawn_blocking`, which puts us on a Tokio blocking thread /// that still carries a `Handle`. source: Arc, - /// Calling context. `cx.app_id` is the cross-app isolation - /// boundary; the resolver passes `&cx` to every `ModuleSource` - /// call so the backend can scope its queries. + /// Calling context. `cx.app_id` is the execution boundary every SDK + /// call inside a module scopes to; the resolver keeps it for diagnostic + /// logging. (Module *resolution* is keyed by the importing node's owner, + /// not `cx.app_id` — see the module docs.) cx: Arc, /// Compiled-module cache. Shared across executions; invalidated @@ -234,7 +272,7 @@ impl ModuleResolver for PicloudModuleResolver { fn resolve( &self, engine: &RhaiEngine, - _source: Option<&str>, + importer_source: Option<&str>, path: &str, pos: Position, ) -> Result> { @@ -329,12 +367,16 @@ impl ModuleResolver for PicloudModuleResolver { )) })?; - // Lexical resolution (§5.5): resolve `path` against the importing - // node's chain. For the entry script that's `default_origin` (its - // defining node); nested-import origin via `_source` lands in C3. - // An app-rooted walk reaches ancestor group modules; a group-rooted - // walk is sealed from app modules below it (the trust boundary). - let origin = self.default_origin; + // Lexical resolution (§5.5): resolve `path` against the *importing + // node's* chain. Rhai gives `_source` = the importing AST's source + // tag — set to the module's defining node for a nested import, or + // unset (`None`) for the top-level entry script, where we fall back + // to `default_origin` (the executing script's node). An app-rooted + // walk reaches ancestor group modules; a group-rooted walk is sealed + // from app modules below it (the trust boundary). + let origin = importer_source + .and_then(decode_origin) + .unwrap_or(self.default_origin); let lookup_result: Result, ModuleSourceError> = tokio::task::block_in_place(|| handle.block_on(self.source.resolve(origin, path))); @@ -371,8 +413,10 @@ impl ModuleResolver for PicloudModuleResolver { }; // Cache lookup: hit only if both key matches AND updated_at - // matches (cache is invalidated lazily on version change). - let cache_key = (self.cx.app_id, path.to_string()); + // matches (cache is invalidated lazily on version change). Keyed by + // the resolved module's id (lexically self-contained — see + // `ModuleCacheKey`). + let cache_key = module_row.script_id; { let mut cache = self.cache.lock().expect("module cache lock poisoned"); if let Some(cached) = cache.get(&cache_key) { @@ -405,7 +449,7 @@ impl ModuleResolver for PicloudModuleResolver { // already been gated at create-time (admin endpoint runs // `validate_module`), but we revalidate here to catch DB-direct // inserts that bypass the API surface. - let ast = engine.compile(&module_row.source).map_err(|e| { + let mut ast = engine.compile(&module_row.source).map_err(|e| { // Wrap as an ErrorRuntime to preserve the parse message // text without trying to reconstruct rhai's internal // ParseErrorType variant (which would require matching on @@ -420,6 +464,15 @@ impl ModuleResolver for PicloudModuleResolver { )) })?; + // Seal this module's own imports to its defining node (§5.5): tag + // the AST source with the resolved module's owner so that when + // `eval_ast_as_new` evaluates its nested `import`s, Rhai hands us + // that tag as `_source` and we resolve them lexically from *here*, + // not from the original importer. Fall back to the lookup origin + // for a corrupt owner row (never reached under the DB CHECK). + let module_owner = module_row.owner().unwrap_or(origin); + ast.set_source(encode_origin(module_owner)); + if let Err(msg) = Self::check_module_shape(&ast, path) { return Err(Box::new(EvalAltResult::ErrorInModule( path.to_string(), diff --git a/crates/executor-core/tests/modules.rs b/crates/executor-core/tests/modules.rs index ce7993c..8054dad 100644 --- a/crates/executor-core/tests/modules.rs +++ b/crates/executor-core/tests/modules.rs @@ -16,9 +16,9 @@ use async_trait::async_trait; use chrono::{DateTime, Utc}; use picloud_executor_core::{Engine, ExecRequest, InvocationType, Limits}; use picloud_shared::{ - AppId, ExecutionId, ModuleScript, ModuleSource, ModuleSourceError, NoopDeadLetterService, - NoopDocsService, NoopEventEmitter, NoopHttpService, NoopKvService, RequestId, ScriptId, - ScriptOwner, ScriptSandbox, Services, + AppId, ExecutionId, GroupId, ModuleScript, ModuleSource, ModuleSourceError, + NoopDeadLetterService, NoopDocsService, NoopEventEmitter, NoopHttpService, NoopKvService, + RequestId, ScriptId, ScriptOwner, ScriptSandbox, Services, }; use tokio::sync::Mutex; @@ -609,3 +609,130 @@ fn validate_endpoint_skips_dynamic_imports_in_imports_list() { v.imports ); } + +// --------------------------------------------------------------------------- +// Phase 4b — lexical (origin-aware) resolution. +// +// `LexicalModuleSource` keys modules by their *exact* defining node, with no +// chain walk. That's enough to prove the resolver passes the RIGHT origin: +// `default_origin` for the entry AST, and each module's own owner (decoded +// from Rhai's `_source`) for its nested imports. The chain-walk + group-tree +// semantics are covered end-to-end by the CLI journey tests against Postgres. +// --------------------------------------------------------------------------- + +#[derive(Default)] +struct LexicalModuleSource { + table: Mutex>, +} + +impl LexicalModuleSource { + fn new() -> Arc { + Arc::new(Self::default()) + } + + async fn put(self: &Arc, owner: ScriptOwner, name: &str, source: &str) { + let (app_id, group_id) = match owner { + ScriptOwner::App(a) => (Some(a), None), + ScriptOwner::Group(g) => (None, Some(g)), + }; + self.table.lock().await.insert( + (owner, name.to_string()), + ModuleScript { + script_id: ScriptId::new(), + app_id, + group_id, + name: name.to_string(), + source: source.to_string(), + updated_at: Utc::now(), + }, + ); + } +} + +#[async_trait] +impl ModuleSource for LexicalModuleSource { + async fn resolve( + &self, + origin: ScriptOwner, + name: &str, + ) -> Result, ModuleSourceError> { + Ok(self + .table + .lock() + .await + .get(&(origin, name.to_string())) + .cloned()) + } +} + +fn req_with_owner(app_id: AppId, owner: ScriptOwner) -> ExecRequest { + let mut r = req(app_id); + r.script_owner = Some(owner); + r +} + +/// A group module's `import` resolves from the GROUP (its own defining +/// node), not the inheriting app — even when an app-owned module of the +/// same name exists. This is the §5.5 trust boundary: a leaf can't shadow +/// a module an inherited group script depends on. +#[tokio::test(flavor = "multi_thread")] +async fn lexical_nested_import_seals_to_module_owner() { + let source = LexicalModuleSource::new(); + let app = AppId::new(); + let group = GroupId::new(); + + // Two "inner" modules: the group's (real) and the app's (a trap). + source + .put(ScriptOwner::Group(group), "inner", "fn v() { 42 }") + .await; + source + .put(ScriptOwner::App(app), "inner", "fn v() { 999 }") + .await; + // The group's "outer" imports "inner" — must bind the group's inner. + source + .put( + ScriptOwner::Group(group), + "outer", + r#"import "inner" as i; fn val() { i::v() }"#, + ) + .await; + + let engine = engine_with(source.clone()); + // Entry script runs as the inherited GROUP script (default_origin = group). + let resp = engine + .execute( + r#"import "outer" as o; o::val()"#, + req_with_owner(app, ScriptOwner::Group(group)), + ) + .expect("should execute"); + assert_eq!( + resp.body, + serde_json::json!(42), + "group module's import must seal to the group's `inner`, not the app's trap" + ); +} + +/// The same registry, entered as an APP-owned script, reaches the app's +/// module — proving the fake distinguishes origins and the entry uses +/// `default_origin`. +#[tokio::test(flavor = "multi_thread")] +async fn lexical_entry_origin_selects_app_module() { + let source = LexicalModuleSource::new(); + let app = AppId::new(); + let group = GroupId::new(); + source + .put(ScriptOwner::Group(group), "inner", "fn v() { 42 }") + .await; + source + .put(ScriptOwner::App(app), "inner", "fn v() { 999 }") + .await; + + let engine = engine_with(source.clone()); + let resp = engine + .execute( + r#"import "inner" as i; i::v()"#, + req_with_owner(app, ScriptOwner::App(app)), + ) + .expect("should execute"); + assert_eq!(resp.body, serde_json::json!(999)); +} diff --git a/crates/shared/src/script.rs b/crates/shared/src/script.rs index 14c83ae..7ae8c2d 100644 --- a/crates/shared/src/script.rs +++ b/crates/shared/src/script.rs @@ -152,7 +152,7 @@ pub struct Script { /// a `CHECK ((group_id IS NULL) <> (app_id IS NULL))`; this enum is the /// in-memory witness of that invariant so call sites can `match` exhaustively /// instead of juggling two `Option`s. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] pub enum ScriptOwner { App(AppId), Group(GroupId),