diff --git a/crates/executor-core/src/module_resolver.rs b/crates/executor-core/src/module_resolver.rs index 7659dc7..f6cddc5 100644 --- a/crates/executor-core/src/module_resolver.rs +++ b/crates/executor-core/src/module_resolver.rs @@ -319,8 +319,19 @@ impl ModuleResolver for PicloudModuleResolver { )) })?; + // C1 (Phase 4b): resolve via the owner-aware `resolve`. Until C3 + // threads the importing script's true defining node through, the + // origin is the calling app — behaviour-preserving for app-owned + // scripts (group scripts can't yet carry imports). The app-rooted + // chain walk additionally lets an app import an ancestor group's + // modules, which is the intended Phase 4b capability. let lookup_result: Result, ModuleSourceError> = - tokio::task::block_in_place(|| handle.block_on(self.source.lookup(&self.cx, path))); + tokio::task::block_in_place(|| { + handle.block_on( + self.source + .resolve(picloud_shared::ScriptOwner::App(self.cx.app_id), path), + ) + }); let module_row = match lookup_result { Ok(Some(m)) => m, diff --git a/crates/executor-core/tests/module_redaction_logging.rs b/crates/executor-core/tests/module_redaction_logging.rs index 9850387..71f135c 100644 --- a/crates/executor-core/tests/module_redaction_logging.rs +++ b/crates/executor-core/tests/module_redaction_logging.rs @@ -16,7 +16,7 @@ use picloud_executor_core::{Engine, ExecRequest, InvocationType, Limits}; use picloud_shared::{ AppId, ExecutionId, ModuleScript, ModuleSource, ModuleSourceError, NoopDeadLetterService, NoopDocsService, NoopEventEmitter, NoopHttpService, NoopKvService, RequestId, ScriptId, - ScriptSandbox, SdkCallCx, Services, + ScriptOwner, ScriptSandbox, Services, }; use serde_json::Value; use tracing_subscriber::fmt::MakeWriter; @@ -27,9 +27,9 @@ struct FailingSource; #[async_trait] impl ModuleSource for FailingSource { - async fn lookup( + async fn resolve( &self, - _cx: &SdkCallCx, + _origin: ScriptOwner, _name: &str, ) -> Result, ModuleSourceError> { Err(ModuleSourceError::Backend(SENTINEL.to_string())) diff --git a/crates/executor-core/tests/modules.rs b/crates/executor-core/tests/modules.rs index 7a1607f..312f369 100644 --- a/crates/executor-core/tests/modules.rs +++ b/crates/executor-core/tests/modules.rs @@ -18,7 +18,7 @@ use picloud_executor_core::{Engine, ExecRequest, InvocationType, Limits}; use picloud_shared::{ AppId, ExecutionId, ModuleScript, ModuleSource, ModuleSourceError, NoopDeadLetterService, NoopDocsService, NoopEventEmitter, NoopHttpService, NoopKvService, RequestId, ScriptId, - ScriptSandbox, SdkCallCx, Services, + ScriptOwner, ScriptSandbox, Services, }; use tokio::sync::Mutex; @@ -55,7 +55,8 @@ impl CountingModuleSource { (app_id, name.to_string()), ModuleScript { script_id, - app_id, + app_id: Some(app_id), + group_id: None, name: name.to_string(), source: source.to_string(), updated_at, @@ -71,20 +72,27 @@ impl CountingModuleSource { #[async_trait] impl ModuleSource for CountingModuleSource { - async fn lookup( + async fn resolve( &self, - cx: &SdkCallCx, + origin: ScriptOwner, name: &str, ) -> Result, ModuleSourceError> { self.lookups.fetch_add(1, Ordering::SeqCst); if let Some(err) = self.fail_with.lock().await.as_ref() { return Err(ModuleSourceError::Backend(err.clone())); } + // This fake is flat/app-scoped — the inheritance + lexical + // resolution semantics are covered by the CLI journey tests + // against real Postgres. A group origin has no entries here. + let app_id = match origin { + ScriptOwner::App(a) => a, + ScriptOwner::Group(_) => return Ok(None), + }; Ok(self .table .lock() .await - .get(&(cx.app_id, name.to_string())) + .get(&(app_id, name.to_string())) .cloned()) } } diff --git a/crates/manager-core/src/config_resolver.rs b/crates/manager-core/src/config_resolver.rs index edbd1d5..42403c6 100644 --- a/crates/manager-core/src/config_resolver.rs +++ b/crates/manager-core/src/config_resolver.rs @@ -42,6 +42,22 @@ pub(crate) const CHAIN_LEVELS_CTE: &str = "\ WHERE c.depth < 64 \ )"; +/// Group-rooted variant of [`CHAIN_LEVELS_CTE`]: the chain starts at a +/// **group** (depth 0 = the group itself, `group_owner` set), then walks +/// its ancestor groups nearest-first. There is no `app_owner` level — a +/// group origin never sees app-owned rows below it (the §5.5 trust +/// boundary). Bind `$1 = group_id`. Used for the lexical module resolver +/// when the importing script's defining node is a group. +pub(crate) const GROUP_CHAIN_LEVELS_CTE: &str = "\ + WITH RECURSIVE chain AS ( \ + SELECT g.id AS group_owner, g.parent_id AS next_group, 0 AS depth \ + FROM groups g WHERE g.id = $1 \ + UNION ALL \ + SELECT g.id, g.parent_id, c.depth + 1 \ + FROM groups g JOIN chain c ON g.id = c.next_group \ + WHERE c.depth < 64 \ + )"; + /// Owner kind of a resolved value, for `--explain` provenance. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum OwnerKind { diff --git a/crates/manager-core/src/module_source.rs b/crates/manager-core/src/module_source.rs index 34e1caf..b353efc 100644 --- a/crates/manager-core/src/module_source.rs +++ b/crates/manager-core/src/module_source.rs @@ -1,18 +1,22 @@ //! `PostgresModuleSource` — the Postgres-backed `ModuleSource` impl. //! -//! Mirrors the structure of [`crate::kv_repo::PostgresKvRepo`] / -//! [`crate::docs_repo::PostgresDocsRepo`]: thin wrapper around a -//! `PgPool` that owns a single statement returning the module by -//! `(cx.app_id, name, kind = 'module')`. The resolver lives in -//! `executor-core` and consumes this trait through the `Services` -//! bundle, so manager-core stays the only crate that touches -//! Postgres. +//! Resolution is **lexical** (§5.5): a module is looked up by walking the +//! group chain rooted at the **importing script's defining node** +//! (`origin`), nearest-owner-wins — *not* the inheriting app's effective +//! view. An `App` origin walks `app → its group → ancestors` (reusing +//! [`CHAIN_LEVELS_CTE`]); a `Group` origin walks `group → ancestors` +//! ([`GROUP_CHAIN_LEVELS_CTE`]) and never sees app-owned modules below it +//! (the trust boundary). The resolver lives in `executor-core` and consumes +//! this trait through the `Services` bundle, so manager-core stays the only +//! crate that touches Postgres. use async_trait::async_trait; use chrono::{DateTime, Utc}; -use picloud_shared::{ModuleScript, ModuleSource, ModuleSourceError, SdkCallCx}; +use picloud_shared::{ModuleScript, ModuleSource, ModuleSourceError, ScriptOwner}; use sqlx::PgPool; +use crate::config_resolver::{CHAIN_LEVELS_CTE, GROUP_CHAIN_LEVELS_CTE}; + pub struct PostgresModuleSource { pool: PgPool, } @@ -27,7 +31,8 @@ impl PostgresModuleSource { #[derive(sqlx::FromRow)] struct ModuleRow { id: uuid::Uuid, - app_id: uuid::Uuid, + app_id: Option, + group_id: Option, name: String, source: String, updated_at: DateTime, @@ -37,7 +42,8 @@ impl From for ModuleScript { fn from(r: ModuleRow) -> Self { Self { script_id: r.id.into(), - app_id: r.app_id.into(), + app_id: r.app_id.map(Into::into), + group_id: r.group_id.map(Into::into), name: r.name, source: r.source, updated_at: r.updated_at, @@ -47,28 +53,47 @@ impl From for ModuleScript { #[async_trait] impl ModuleSource for PostgresModuleSource { - async fn lookup( + async fn resolve( &self, - cx: &SdkCallCx, + origin: ScriptOwner, name: &str, ) -> Result, ModuleSourceError> { - // The query is the cross-app isolation boundary: app_id comes - // from cx (never from the script-passed argument), and the - // CHECK constraint `kind IN ('endpoint','module')` plus the - // `kind = 'module'` filter together guarantee endpoint scripts - // are never importable. The `(app_id, kind)` index from - // migration 0015 makes this an index scan returning at most - // one row (per-app uniqueness on `name`). - let row: Option = sqlx::query_as( - "SELECT id, app_id, name, source, updated_at \ - FROM scripts \ - WHERE app_id = $1 AND kind = 'module' AND name = $2", - ) - .bind(cx.app_id.into_inner()) - .bind(name) - .fetch_optional(&self.pool) - .await - .map_err(|e| ModuleSourceError::Backend(e.to_string()))?; + // Lexical lookup: JOIN module scripts against the chain rooted at + // `origin`, take the nearest level (lowest depth). The `kind = + // 'module'` filter + the CHECK constraint guarantee endpoint + // scripts are never importable. Case-insensitive to match the + // per-owner `LOWER(name)` partial-unique indexes (0050). `origin` + // is a trusted dispatch-derived value, so the chain it roots is + // the script's true ancestry — the cross-app isolation boundary + // is preserved (a group origin can't reach an app's modules). + let query = match origin { + ScriptOwner::App(_) => format!( + "{CHAIN_LEVELS_CTE} \ + SELECT s.id, s.app_id, s.group_id, s.name, s.source, s.updated_at \ + FROM scripts s \ + JOIN chain c ON (s.app_id = c.app_owner OR s.group_id = c.group_owner) \ + WHERE s.kind = 'module' AND LOWER(s.name) = LOWER($2) \ + ORDER BY c.depth ASC LIMIT 1", + ), + ScriptOwner::Group(_) => format!( + "{GROUP_CHAIN_LEVELS_CTE} \ + SELECT s.id, s.app_id, s.group_id, s.name, s.source, s.updated_at \ + FROM scripts s \ + JOIN chain c ON s.group_id = c.group_owner \ + WHERE s.kind = 'module' AND LOWER(s.name) = LOWER($2) \ + ORDER BY c.depth ASC LIMIT 1", + ), + }; + let root = match origin { + ScriptOwner::App(a) => a.into_inner(), + ScriptOwner::Group(g) => g.into_inner(), + }; + let row: Option = sqlx::query_as(&query) + .bind(root) + .bind(name) + .fetch_optional(&self.pool) + .await + .map_err(|e| ModuleSourceError::Backend(e.to_string()))?; Ok(row.map(Into::into)) } } diff --git a/crates/shared/src/modules.rs b/crates/shared/src/modules.rs index 5e15fff..223b8dd 100644 --- a/crates/shared/src/modules.rs +++ b/crates/shared/src/modules.rs @@ -1,51 +1,88 @@ -//! `ModuleSource` — the v1.1.3 Rhai module-loading contract. +//! `ModuleSource` — the Rhai module-loading contract (v1.1.3; made +//! origin-aware in v1.2 Phase 4b). //! //! The executor-core `PicloudModuleResolver` calls into this trait to //! load `kind = 'module'` scripts referenced by `import "" as ;` //! statements. The Postgres impl in `manager-core` reads from the //! `scripts` table; tests pin in-memory fakes. //! -//! Implementations MUST derive `app_id` from `cx.app_id` and pass it -//! to every backend query. The `name` argument carries only the -//! script's name (the literal between the import quotes); the trait -//! has no way to express a cross-app lookup. That asymmetry is the -//! load-bearing cross-app isolation boundary — see `docs/sdk-shape.md`. +//! **Resolution is lexical (§5.5).** A module is resolved against the +//! module set visible at the **importing script's own defining node** +//! (`origin`), walking up the group chain from there — *not* the +//! inheriting app's effective view. So `resolve` is keyed by a +//! [`ScriptOwner`] origin, not by `cx.app_id`. This deliberately differs +//! from the SDK-call isolation boundary: a group script's `import` +//! resolves from the **group** (sealed from below — a leaf app cannot +//! shadow it), while every SDK call *inside* that module still scopes to +//! the inheriting app via `cx.app_id`. The `origin` argument is a trusted +//! dispatch-derived value (the resolved script's owner), never a +//! script-passed argument, so the cross-app isolation boundary holds. use async_trait::async_trait; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use thiserror::Error; -use crate::{AppId, ScriptId, SdkCallCx}; +use crate::{AppId, GroupId, ScriptId, ScriptOwner}; -/// A module script as returned by `ModuleSource::lookup`. Carries only -/// the fields the resolver needs: the id (for diagnostics), the source -/// (to compile), and `updated_at` (the cache-staleness comparator). +/// A module script as returned by `ModuleSource::resolve`. Carries the +/// fields the resolver needs: the id (cache key + diagnostics), the +/// resolved module's **owner** (so nested imports inside it resolve from +/// *its* defining node), the source (to compile), and `updated_at` (the +/// cache-staleness comparator). +/// +/// Ownership is polymorphic (Phase 4): exactly one of `app_id` / `group_id` +/// is set — mirroring [`crate::Script`] and the DB CHECK. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ModuleScript { pub script_id: ScriptId, - pub app_id: AppId, + /// Owning app, when app-owned. Mutually exclusive with `group_id`. + #[serde(default)] + pub app_id: Option, + /// Owning group, when group-owned (Phase 4b group modules). + #[serde(default)] + pub group_id: Option, pub name: String, pub source: String, pub updated_at: DateTime, } -/// Lookup contract used by `PicloudModuleResolver`. `lookup` MUST -/// scope by `cx.app_id`; cross-app reads must be unreachable. +impl ModuleScript { + /// The resolved module's defining node, reconstructed from the + /// polymorphic columns. Used by the resolver to seal *this* module's + /// own nested imports to its node (lexical chaining). Prefers the app + /// if a corrupt row sets both, so the hot path never panics. + #[must_use] + pub fn owner(&self) -> Option { + match (self.app_id, self.group_id) { + (Some(a), _) => Some(ScriptOwner::App(a)), + (None, Some(g)) => Some(ScriptOwner::Group(g)), + (None, None) => None, + } + } +} + +/// Lookup contract used by `PicloudModuleResolver`. `resolve` walks the +/// module set up the chain rooted at `origin` (the importing script's +/// defining node), nearest-owner-wins. The `origin` is trusted +/// (dispatch-derived); the `name` is the literal between the import +/// quotes. See the module docs for why this is lexical, not `cx`-scoped. #[async_trait] pub trait ModuleSource: Send + Sync { - /// Resolve a module script by `(cx.app_id, name)`. Returns `None` - /// when no row exists, or when a row exists but its `kind` is - /// `'endpoint'` (endpoints are never importable). The resolver - /// surfaces `None` as `ErrorModuleNotFound` to Rhai. - async fn lookup( + /// Resolve a `kind = 'module'` script named `name`, visible from the + /// `origin` node walking up its group chain (nearest depth wins). + /// Returns `None` when no module of that name exists anywhere on the + /// chain (or a row exists but its `kind` is `'endpoint'` — endpoints + /// are never importable). The resolver surfaces `None` as + /// `ErrorModuleNotFound` to Rhai. + async fn resolve( &self, - cx: &SdkCallCx, + origin: ScriptOwner, name: &str, ) -> Result, ModuleSourceError>; } -/// Failure modes surfaced from `ModuleSource::lookup`. "Not found" is +/// Failure modes surfaced from `ModuleSource::resolve`. "Not found" is /// not exceptional — it's `Ok(None)`. #[derive(Debug, Error)] pub enum ModuleSourceError { @@ -57,7 +94,7 @@ pub enum ModuleSourceError { } /// Stub used by the executor-core test harness so engine integration -/// tests don't need a real DB-backed source. Every lookup returns +/// tests don't need a real DB-backed source. Every resolve returns /// `Ok(None)` — `import "x"` always errors as "module not found" /// under this impl. #[derive(Debug, Default, Clone, Copy)] @@ -65,9 +102,9 @@ pub struct NoopModuleSource; #[async_trait] impl ModuleSource for NoopModuleSource { - async fn lookup( + async fn resolve( &self, - _cx: &SdkCallCx, + _origin: ScriptOwner, _name: &str, ) -> Result, ModuleSourceError> { Ok(None)