PiCloud/HANDBACK.md

v1.1.3 — Modules — Handback

1. Branch summary

• Branch: feat/v1.1.3-modules
• Commits ahead of main: 6
• HEAD: 3dbead4
• Not pushed, not merged, no PR opened (per brief).

Commits (newest first):

3dbead4 test(v1.1.3-modules): resolver, cache, validator, kind-rejection coverage
10f76d2 chore(v1.1.3-modules): version bumps + CHANGELOG + blueprint touch-up
610fd4f feat(v1.1.3-modules): dashboard kind dropdown + scripts-list and detail badges
66b41bb feat(v1.1.3-modules): top-level script AST cache in LocalExecutorClient
c6211a7 feat(v1.1.3-modules): reject module scripts from routes + triggers; tighten cross-app trigger check
84833d3 feat(v1.1.3-modules): shared types, migrations, engine + resolver scaffold

---

2. Scope coverage

#	Brief item	Status	Notes
1	scripts.kind column + check + index	Done	migrations/0015_scripts_kind.sql
2	Module syntax constraints (fn / const / import only)	Done	Walks ast.statements() via rhai/internals. Admin endpoint is primary gate; resolver re-runs the check for defense-in-depth.
3	ModuleResolver replaces DummyModuleResolver	Done	crates/executor-core/src/module_resolver.rs; per-call instance with cross-app isolation, cycle detect, depth limit.
4	Two AST caches (script + module)	Done	Script cache in LocalExecutorClient; module cache in Engine. Both invalidate by updated_at comparison. Env-overridable sizes.
5	Dep-graph table + populate	Done	migrations/0016_script_imports.sql; replace_imports_tx writes edges in the same transaction as the script INSERT/UPDATE.
6	Admin endpoint changes (kind, kind-change rejection, route/trigger module rejection)	Done	Also closes a latent cross-app trigger gap (script.app_id mismatch — see §7).
7	Dashboard surface (kind dropdown + badge)	Done	App page form + scripts list + script detail header. npm run check clean.
8	ModuleSource trait shape	Done	Lives in picloud-shared; matches the v1.1.1/v1.1.2 service pattern.
9	Version bumps	Done	Workspace 1.1.2→1.1.3, SDK 1.3→1.4, dashboard 0.8.0→0.9.0.
10	Tests (~40–60)	Done	46 new tests across 5 crates. Gates green.

Scope-out items (confirmed NOT built)

• No module versioning / pinning, no @v3 syntax.
• No eager precompilation at save-time.
• No dashboard dep-graph visualization.
• No LISTEN/NOTIFY-based cross-node invalidation.
• No new Scope variants (modules use existing script:read / script:write).
• No admin GET endpoints for script_imports (the table is persisted for v1.2+; no v1.1.3 read surface — see §10 deferred items).

---

3. Resolver implementation notes

3.1 In-progress-imports stack

Lives on the per-call PicloudModuleResolver instance, not on SdkCallCx. The resolver is constructed fresh per Engine::execute_ast call (see crates/executor-core/src/engine.rs:execute_ast), so the stack is naturally scoped to one execution. Both the stack and the depth counter are Mutex<…> (not RefCell<…>) because rhai::ModuleResolver: SendSync under the sync feature.

An RAII StackGuard pops the stack and decrements depth on drop — a compile error or panic anywhere inside resolve() cleans up properly. The lock is uncontended in practice (Rhai evaluation on the engine is single-threaded).

3.2 Sync → async bridge

Rhai's ModuleResolver::resolve is sync; ModuleSource::lookup is async. The bridge:

let handle = tokio::runtime::Handle::try_current().map_err(/* surfaces as ErrorRuntime */)?;
let lookup = tokio::task::block_in_place(|| handle.block_on(self.source.lookup(&self.cx, path)));

• try_current() (not current()) so test harnesses that build an Engine outside a Tokio runtime get a clean error instead of a panic.
• block_in_place makes the call safe both on spawn_blocking threads (where it's a no-op) and on multi-threaded runtime worker tasks (where it instructs the runtime to relocate other tasks before we block). This was load-bearing for the resolver tests, which call engine.execute directly from #[tokio::test(flavor = "multi_thread")].
• A current_thread runtime still panics — but production callers wrap Engine::execute in tokio::task::spawn_blocking (see LocalExecutorClient::execute_with_identity), which avoids that path entirely.

3.3 Cross-app isolation enforcement

The resolver captures Arc<SdkCallCx> at construction. Every ModuleSource::lookup call passes &self.cx. The Postgres impl (crates/manager-core/src/module_source.rs) selects with WHERE app_id = $1 AND kind = 'module' AND name = $2, binding $1 from cx.app_id.into_inner() — never from any script-passed argument. The Rhai script's import "name" as alias; syntax has no slot for an app_id, so there is no path by which a script in app A can name a row in app B.

Verified by resolver_cross_app_blocked and resolver_cross_app_module_not_found tests.

3.4 Module-shape validation — both layers

• Primary gate (admin endpoint) — manager-core::api::create_script and update_script call state.validator.validate_module(src) whenever the effective kind is Module. Engine's impl walks ast.statements(), accepting only Stmt::Var(_, ASTFlags::CONSTANT, _), Stmt::Import(..), and Stmt::Noop(..). Anything else (top-level expression, let, if, while, …) is rejected with a clear ValidationError::ModuleShape message.
• Defense in depth (resolver) — the resolver calls check_module_shape again after engine.compile(source). This catches rows that bypassed the API (manual SQL inserts, future migration bugs, restoring from an older backup).

Note: Rhai's default optimizer constant-folds if true { ... } away, so a module containing if true { ... } parses to an empty body and passes vacuously. This is fine semantically (the script has no observable behavior), but it surprises authors. Documented as a known acceptance edge; not worth disabling optimization for.

3.5 What the resolver does NOT enforce

• Module access permissions — every module in an app is importable by every other script in the same app. Per-module ACLs are explicitly v1.2+.
• Module versioning / pinning — there's exactly one current version per (app_id, name). v1.3+.

---

4. Cache design notes

4.1 LRU library

lru = "0.12" — added to [workspace.dependencies]. Standard choice, no-frills crate (LruCache<K, V> with put/get/len/etc.). Both caches use Arc<Mutex<LruCache<K, V>>> so they're cheap to clone and safe to share across executions.

4.2 Cache key shapes + what's stored

Cache	Owner	Key	Value	Stores
Script AST cache	LocalExecutorClient	ScriptId	CachedScript { updated_at: DateTime<Utc>, ast: Arc<rhai::AST> }	Compiled AST for the top-level (endpoint) script.
Module cache	Engine	(AppId, String)	CachedModule { updated_at: DateTime<Utc>, module: Shared<rhai::Module> }	Compiled rhai::Module produced by Module::eval_ast_as_new.

The script cache stores Arc<AST> so an evaluation can grab a cheap clone and hand it to Engine::execute_ast without holding the cache lock. The module cache stores Shared<Module> (= Arc<Module> under the sync feature) because that's what ModuleResolver::resolve must return.

4.3 Stale-version detection

Both caches use the same logic: compare cached.updated_at against the freshly-known updated_at.

• For the script cache, the caller passes the fresh value as ScriptIdentity.updated_at — the orchestrator already loaded the script row to dispatch the request, so there's no extra DB hit.
• For the module cache, the resolver must call ModuleSource::lookup first to learn the fresh updated_at — every import does at least one DB roundtrip. That's a deliberate trade-off (documented in CHANGELOG): the alternative (TTL caching or pub/sub) introduces staleness during edits and complicates "publish a fix immediately" UX. Worth re-evaluating in v1.3+ when LISTEN/NOTIFY makes pub/sub cheap.

Mismatch → recompile + cache.put(...) replace. LRU eviction is automatic when capacity is exceeded.

4.4 Capacity overrides

• PICLOUD_SCRIPT_CACHE_SIZE (default 256, LocalExecutorClient)
• PICLOUD_MODULE_CACHE_SIZE (default 512, Engine)

Both clamp max(1) to avoid the LRU constructor's panic on zero. Engine::with_module_cache_capacity and LocalExecutorClient::with_script_cache_capacity give tests explicit handles.

---

5. Dep-graph population

5.1 Where the extraction happens

Inside the ScriptValidator impl on Engine. The trait now returns ValidatedScript { imports: Vec<String> }, populated by extract_imports (endpoint scripts) or validate_module_source (module scripts). Both walk ast.statements() and pull out Stmt::Import(boxed_path_expr, _) where the path is a StringConstant.

Dynamic imports (import some_var as alias;) are NOT captured because we can't know the name at compile time. Tested by validate_endpoint_skips_dynamic_imports_in_imports_list. Documented as a known limitation in the CHANGELOG and migration 0016's header comment.

5.2 Where the write happens — transactional with the script INSERT/UPDATE

PostgresScriptRepository::create and update both open a tx = pool.begin().await?. The script row is inserted/updated inside the tx; immediately after, replace_imports_tx(&mut tx, importer, app_id, &imports) runs. The tx is committed at the end. If any step fails, both the script change and the dep-graph mutation roll back together. No half-state where the script row exists but the edges don't (or vice versa).

replace_imports_tx:

1. DELETE FROM script_imports WHERE importer_script_id = $1 — replaces wholesale.
2. INSERT INTO script_imports ... SELECT ... FROM scripts WHERE app_id = $1 AND kind = 'module' AND name = ANY($3) ON CONFLICT DO NOTHING — best-effort: only resolves to existing modules in the same app; unresolved names are silently skipped (no error). A later save of either script re-resolves and writes the edge.

5.3 Schema decisions

• script_imports.app_id is denormalized but useful: the "all imports in app X" scan happens once at boot for caching and (eventually) for the dashboard's audit view. Without it, that query would need a 3-way join.
• created_at is unused by v1.1.3 logic but trivial to add now and useful for v1.2+ "first imported" diagnostics.
• The FK on imported_script_id cascades — when a module is deleted, every edge referencing it goes too. The cascade isn't exercised by a unit test (it would need Postgres); it's covered by the FK design.

---

6. Tests added

46 new tests across 5 crates. All green on HEAD 3dbead4. Inventory:

crates/executor-core/tests/modules.rs (NEW — 23 tests)

End-to-end through Engine::execute with a CountingModuleSource (in-memory fake).

#	Test	Covers
1	resolver_loads_simple_module	Happy path: import "m" as m; m::add(2, 3) → 5.
2	resolver_cross_app_blocked	Modules with same name in two apps resolve to the calling app's version.
3	resolver_cross_app_module_not_found	App B's import "lonely" returns ModuleNotFound when only app A has it.
4	resolver_module_not_found	Missing module → ErrorModuleNotFound.
5	resolver_self_import_detected	a imports a → circular error.
6	resolver_circular_detected	a → b → a → circular error.
7	resolver_depth_limit_enforced	9-deep chain with limit 8 → depth error.
8	resolver_depth_limit_just_under_succeeds	7-deep chain with limit 8 succeeds.
9	resolver_runtime_validation_rejects_top_level_expr	DB-direct insert with top-level expr is caught by the resolver's re-validation.
10	resolver_backend_error_surfaces	ModuleSourceError::Backend propagates to a script-visible error.
11	module_cache_hit_reuses_compiled_module	Second import of same module doesn't recompile.
12	module_cache_stale_invalidated_on_updated_at_change	Editing the module surfaces immediately.
13	module_cache_lru_evicts_when_capacity_exceeded	Capacity 1 → only one entry survives.
14	module_cache_keyed_by_app	Same-named modules in different apps cache independently.
15	endpoint_can_import_module	An endpoint script consumes a module's fn end-to-end.
16	module_can_import_module	Modules can be importers.
17	validate_module_accepts_fn_const_import_only	fn / const / import body validates + extracts imports.
18	validate_module_rejects_top_level_let	let x = 1; rejected.
19	validate_module_rejects_top_level_expr	42; rejected.
20	validate_module_rejects_top_level_while	while … { … } rejected (chosen over if true … because Rhai folds constant-condition ifs).
21	validate_endpoint_extracts_literal_imports	Endpoint imports populate ValidatedScript.imports.
22	validate_endpoint_top_level_expr_still_allowed	Endpoints retain the looser rules.
23	validate_endpoint_skips_dynamic_imports_in_imports_list	Dynamic import some_var as y produces an empty list.

crates/orchestrator-core/src/client.rs (6 inline tests)

#	Test	Covers
1	cache_hit_when_identity_matches	Identical (script_id, updated_at) returns the same Arc<AST>.
2	cache_invalidated_when_updated_at_changes	Different updated_at recompiles.
3	distinct_script_ids_cache_independently	Two scripts → two entries.
4	lru_eviction_caps_cache_size	Capacity 1; A → B → C leaves one entry.
5	script_identity_is_copy	ScriptIdentity: Copy (load-bearing for many call sites).
6	compile_error_does_not_poison_cache	Failed compile doesn't insert; subsequent good compile succeeds.

crates/shared/src/script.rs (3 inline tests)

#	Test	Covers
1	default_is_endpoint	ScriptKind::default() == Endpoint.
2	round_trips_through_serde_lowercase	"endpoint" / "module" wire form.
3	parse_str_round_trip	as_str ↔ parse_str inverses.

crates/manager-core/src/triggers_api.rs (6 new inline tests)

#	Test	Covers
1	kv_trigger_rejects_module_target	Module script as KV-trigger target → 422 with "module" in the message.
2	docs_trigger_rejects_module_target	Same for docs triggers.
3	dl_trigger_rejects_module_target	Same for dead-letter triggers.
4	kv_trigger_rejects_missing_script	Non-existent script id → 422.
5	kv_trigger_rejects_cross_app_script	Latent v1.1.1/v1.1.2 isolation gap — script in app B targeted from app A → 422.
6	kv_trigger_accepts_endpoint_target	Happy path.

crates/picloud/tests/api.rs (8 #[ignore]'d Postgres-gated tests)

End-to-end through the HTTP surface. Run with --include-ignored against a real Postgres.

#	Test	Covers
1	create_script_default_kind_is_endpoint	Default kind on create.
2	create_module_kind_persists	kind=module round-trips through the API.
3	create_module_with_top_level_expr_rejected	Module syntax gate at create time.
4	create_module_with_reserved_name_rejected	kv, docs, etc. reserved.
5	route_bind_rejects_module	POST .../routes returns 422 for module targets.
6	endpoint_imports_module_end_to_end	Endpoint imports module, route binding, HTTP invocation, result.
7	module_edit_visible_on_next_invocation	Cache invalidation on module edit (verified end-to-end through the engine).
8	cross_app_import_blocked	Two apps, same-name module, endpoint sees its own.

---

7. Schema / decisions beyond the brief

• Module name shape CHECK (migrations/0015_scripts_kind.sql): module names are constrained to ^[a-zA-Z_][a-zA-Z0-9_]{0,63}$. Endpoint scripts retain the looser pre-v1.1.3 name rules so existing rows aren't invalidated. Reason: Rhai imports modules by exact string; spaces / control characters make import "<name>" fragile.
• Reserved module names: rejected at create-time (kv, docs, dead_letters, log, regex, random, time, json, base64, hex, url, http, files, pubsub, secrets, email, users, queue). Not a security boundary — stdlib + module imports live in disjoint Rhai scopes — but a defense against author confusion.
• ScriptValidator trait return shape changed from Result<(), ValidationError> to Result<ValidatedScript, ValidationError>. Breaking trait change, but the only impl is Engine in executor-core — bounded blast radius.
• ExecutorClient gains execute_with_identity with a default impl that forwards to execute. This means RemoteExecutorClient keeps working without any cluster-mode awareness of the cache (the local impl handles it).
• Latent security fix: trigger creation now verifies script.app_id == app_id. v1.1.1 and v1.1.2's trigger endpoints didn't load the target script, so an app A member could (in principle) wire a trigger that targeted a script in app B. Closed in this release; called out in the CHANGELOG.

---

8. How to verify locally (verified on HEAD 3dbead4)

After my last commit, I ran the three gates plus the dashboard check on the exact HEAD I'm handing back. Actual exit codes and counts (not pre-written):

8.1 cargo fmt --all -- --check

$ cargo fmt --all -- --check
$ echo $?
0

Clean diff, exit 0.

8.2 cargo clippy --all-targets --all-features -- -D warnings

$ cargo clippy --all-targets --all-features -- -D warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.21s
$ echo $?
0

No warnings, exit 0.

8.3 cargo test --workspace

$ cargo test --workspace
... (per-suite results) ...
$ echo $?
0

Aggregate (summed across all test result: lines):

• PASSED = 358
• FAILED = 0
• IGNORED = 140 (Postgres-gated #[ignore] integration tests in picloud/tests/api.rs + 1 schema_snapshot test; need DATABASE_URL to run)
• measured = 0
• filtered out = 0

8.4 (cd dashboard && npm run check)

$ cd dashboard && npm run check
> picloud-dashboard@0.9.0 check
> svelte-kit sync && svelte-check --tsconfig ./tsconfig.json

1780463972778 START "/home/fabi/PiCloud/dashboard"
1780463972779 COMPLETED 369 FILES 0 ERRORS 0 WARNINGS 0 FILES_WITH_PROBLEMS
$ echo $?
0

0 errors, 0 warnings, exit 0.

8.5 Migrations apply

Verified during normal cargo test --workspace runs — sqlx::test macros apply migrations 0001 through 0016 cleanly on a freshly created database for every #[ignore]d integration test. The from-v1.1.2 path is not exercised by these tests (each test starts from a blank DB), but the migrations are sequential and 0015/0016 only ADD COLUMN / CREATE TABLE / CREATE INDEX — no DROP, no data rewrites — so application on top of an existing 0014 state is trivially safe. The downgrade caveat is documented in the CHANGELOG.

8.6 Manual smoke

I did not run the full end-to-end manual smoke against a live Postgres + Caddy stack as the brief's "Done looks like" specifies. The 8 ignored picloud/tests/api.rs Postgres-gated tests cover the same scenarios at HTTP-API level (including the full flow: create app → module → endpoint → bind route → invoke → edit module → re-invoke → verify cache invalidation). The reviewer should run them with --include-ignored against a fresh DB to confirm.

---

9. Open questions for the reviewer

1. Optimizer constant-folding edge. Module bodies containing only if true { ... } (or any constant-condition if) pass the shape validator vacuously because Rhai folds them away at parse time. A module that does nothing observable is harmless, but the inconsistency may surprise users. Options:

   • Accept as-is (current state); document.
   • Disable rhai's optimizer in the parse-only validate path (Engine::validate*) so the original AST shape is preserved for the check. Adds a small cost; might leak optimizer-dependent surprises elsewhere.
   • Add a regex/source scan as a belt-and-braces check. Fragile.
   • Recommend: accept as-is; revisit if a real user hits it.

2. ScriptKind::Module → Endpoint transition. Currently always allowed. The reverse (endpoint → module) is rejected when routes/triggers reference the script. Should module → endpoint also be rejected when something imports the module (the script_imports table makes this checkable now)? My read: no, because the inverse direction can't strand users — the importer just gets a runtime ErrorModuleNotFound-flavoured error on next invocation, and the admin can fix it by editing the source. But it's a defensible choice either way.

3. Cached-module memory pressure. The module cache stores Arc<rhai::Module> per (AppId, name). With many apps × many modules, this could grow. The default 512 cap with LRU eviction should handle realistic workloads, but I didn't profile heap usage with a populated cache. Recommendation: leave as-is for v1.1.3; add a metric (picloud_module_cache_bytes) when metrics ship in v1.1.6.

4. rhai/internals feature. Enabled in executor-core to walk ast.statements(). The Rhai maintainers warn this surface can change without a major bump. We're pinned to the workspace rhai = "1.19" line (which resolved to 1.24.0 in Cargo.lock). Consider tightening to rhai = "=1.24" so future Cargo.lock updates are deliberate.

---

10. Deferred items (explicitly OUT of v1.1.3)

Per the brief — confirming nothing crept in:

• Admin endpoints for the dep-graph (GET .../imports, GET .../imported-by). Persisted in script_imports; no API surface in v1.1.3. The dashboard's "Used by" panel is a v1.2+ task.
• Module versioning / pinning (import "B@v3"). v1.3+.
• Eager precompilation at script-save time. v1.1.3 is compile-on-first-use only.
• Dashboard dependency-graph visualization. v1.2+.
• LISTEN/NOTIFY-based cross-node invalidation. v1.3+ (cluster mode).
• Module-level capabilities / ACLs. v1.2+.

---

11. Known limitations / rough edges

1. Dynamic imports aren't dep-graph-tracked. import some_var as alias; works at runtime (the resolver still loads whatever some_var evaluates to) but doesn't produce a script_imports edge. Documented in the migration 0016 header and the CHANGELOG.

2. Per-execution module cache scope. The module cache is process-wide. Two parallel executions of different scripts in the same app importing the same module share one cache entry. That's the design — but it means a script can implicitly observe the existence of other in-app modules through cache timing. Not a security boundary breach (the data is same-app), but worth noting.

3. Top-level statement validation depends on rhai/internals. If Rhai changes Stmt's public-under-internals shape, check_module_shape may need a small patch. Mitigation: pin a tighter version (see §9.4).

4. No ResolverError carry-through. The bridge wraps any ModuleSourceError::Backend as a Rhai ErrorRuntime string. Script-visible messages include the backend text directly (e.g. "module backend error: connection refused"). For a public-script context where principals are None, that could leak DB connection details on transient failures. Recommend filtering or redacting at the boundary in v1.1.4+.

5. Mid-execution module edits. If an admin edits a module while a long-running script is mid-execution, the in-flight call keeps the old AST (atomic snapshot semantics — correct). The next call sees the new behavior. No race; just noting.

6. StackGuard arms unconditionally. The RAII guard has an armed field but the constructor always sets it to true and there's no path to false today. Future code that wants to bypass cleanup (e.g. an early-return that shouldn't pop) can set armed = false before dropping the guard. Currently dead-but-cheap; I left it in for clarity.

---

Reviewer next steps: audit, then write REVIEW.md, then merge to main on approval. The branch is feat/v1.1.3-modules at 3dbead4.