PiCloud/HANDBACK.md

v1.1.2 Implementation HANDBACK

1. Branch + commit count

• Branch: feat/v1.1.2-documents
• Base: main
• 9 commits ahead of main (7 original + 2 from iteration 2: a chore: cargo fmt fix and this HANDBACK update). Branch is not pushed, not merged.

docs(v1.1.2): handback §8 fresh post-fix attestation
bf26a25 chore: cargo fmt
dee23ff docs(v1.1.2): handback report for reviewer
277ba34 chore(release): bump workspace to v1.1.2 + CHANGELOG
2a047f1 feat(v1.1.2-docs): wire DocsServiceImpl into picloud binary
a66d4af feat(v1.1.2-docs): Rhai docs:: SDK module + ctx.event.docs + bridge tests
ef59309 feat(v1.1.2-docs): triggers framework + dispatcher + emitter extended for docs
06678f4 feat(v1.1.2-docs): manager-core docs service + repo + query DSL parser
3af8cc3 feat(v1.1.2-docs): migrations + shared DocsService trait + TriggerEvent::Docs

Iteration 2 note: the original v1 HANDBACK §8 claimed cargo fmt --check was green; that claim was false against HEAD at audit time (one single-line collapse diff in docs_service.rs::delete's $in arm). Iteration 2 adds the chore commit fixing that and this HANDBACK update replacing §8's attestation with one I actually verified post-fix. The discipline lesson is recorded for the v1.1.3 retro: never claim a gate is green without re-running it on the exact HEAD I'm handing back.

2. Scope coverage (Done / Partial / Skipped)

Scope item (from brief)	Status	Notes
docs service trait + impl + Postgres repo	Done	DocsService in picloud-shared; DocsServiceImpl + PostgresDocsRepo in manager-core; wired into Services.
Rhai SDK surface (docs::collection(name).{create,get,find,find_one,update,delete,list})	Done	executor-core/src/sdk/docs.rs. Handle pattern via engine.register_type_with_name::<DocsHandle> + register_fn per method.
Query DSL v1.1.2 subset ($eq, $ne, $gt, $gte, $lt, $lte, $in, dot paths to 5 levels, $sort, $limit)	Done	manager-core/src/docs_filter.rs parser + AST; SQL emitted by manager-core/src/docs_repo.rs::build_find_query. Unsupported operators throw with v1.2 pointer.
docs:* trigger kind	Done	TriggerKind::Docs, OutboxSourceKind::Docs, TriggerEvent::Docs { op, collection, id, data, prev_data }, docs_trigger_details table, POST /api/v1/admin/apps/{id}/triggers/docs endpoint.
Dispatcher routes OutboxSourceKind::Docs	Done	Single-line match-arm extension at dispatcher.rs:166: `Kv
Authz: Capability::AppDocsRead(AppId) + AppDocsWrite(AppId) mapped to script:read/script:write	Done	No new Scope variants added — honors the seven-scope commitment. Read at Viewer, write at Editor (mirrors KV).
Event emission (ServiceEvent { source: "docs", op, collection, key: id, payload, old_payload })	Done	Best-effort emit after each successful mutation; OutboxEventEmitter::emit_docs fans out to matching triggers.
ctx.event.docs.prev_data change-data-capture	Done	Repo's update/delete return the prior data via a CTE so the service can populate old_payload. trigger_event_to_dynamic in engine.rs builds the Rhai-visible map.
Migrations 0013 + 0014	Done	0013 = docs table + GIN-on-jsonb_path_ops. 0014 = CHECK extensions + docs_trigger_details.
Version bumps + CHANGELOG	Done	Workspace 1.1.1 → 1.1.2, SDK 1.2 → 1.3, dashboard 0.7.0 → 0.8.0, CHANGELOG entry with downgrade caveats + known limitations.
Tests (~30–50 new)	Done — 77 new tests	26 docs_filter + 10 docs_repo SQL-shape + 23 docs_service + 3 triggers_api (docs) + 15 bridge integration.
Optional: prune docs/v1.1.x-design-notes.md §1–4	Skipped	Left for a separate cleanup PR. §1–4 contain the rationale for v1.1.1 decisions that ship in code now; pruning is a doc-only change that doesn't touch v1.1.2's scope.

3. Query DSL implementation notes

Operator dispatch path

A script's filter is a Rhai Map. The bridge converts it to serde_json::Value via dynamic_to_json (no parsing here — the bridge stays thin) and hands it to DocsService::find. The service calls docs_filter::parse_filter which:

1. Validates the filter is a JSON object.
2. Iterates each top-level entry:
   • $-prefixed keys: $sort and $limit are accepted; anything else ($or, $and, etc.) returns FilterParseError::UnsupportedOperator with a script-visible message naming the operator + pointing at v1.2.
   • Other keys: parsed as a FieldPath (validates non-empty, no .., no $-prefixed segments, depth ≤ 5). The value is either a scalar (implicit $eq) or an operator object — an object where every key starts with $. Mixed-shape objects reject as InvalidFilter since the user almost certainly meant operator dispatch.
3. Inside an operator object, each $xxx key dispatches through ComparisonOp::from_dollar_key. Unknown operators return UnsupportedOperator.

The resulting DocsFilter { conditions, sort, limit } is purely descriptive — no SQL or Postgres concepts leak in.

Dot-path → JSONB navigation

FieldPath::parse splits on . and validates each segment. The PostgresDocsRepo SQL builder emits jsonb_extract_path_text(data, $N1, $N2, …) where each segment is bound as a separate text parameter. Postgres's jsonb_extract_path_text accepts a variadic text array, so depth doesn't change the SQL shape — only the bind count. This means depths 1 through 5 all flow through one helper (push_jsonb_path) without conditional branching on length.

Parser error → Rhai error pipeline

docs_filter::parse_filter
   └─ FilterParseError::{InvalidFilter, UnsupportedOperator}(String)
         └─ DocsServiceImpl::find via `From<FilterParseError> for DocsError`
               └─ DocsError::{InvalidFilter, UnsupportedOperator}(String)
                     └─ executor-core::sdk::docs::block_on
                           └─ EvalAltResult::ErrorRuntime("docs: <message>")

The error string flows verbatim from the parser. The Rhai bridge prefixes "docs: " and surfaces it through Box<EvalAltResult>. Snapshot tests in docs_filter::tests pin three representative error strings ($regex, multi-field $sort, depth-limit) so changing them is a deliberate act.

SQL builder — parameterised vs hardcoded

This is the load-bearing security surface. The reviewer should audit crates/manager-core/src/docs_repo.rs::build_find_query and the emit_condition / push_jsonb_path helpers.

Hardcoded SQL fragments (never come from user input):

• The base SELECT id, data, created_at, updated_at FROM docs WHERE app_id = prefix.
• The connector AND collection =, AND between conditions, ORDER BY, LIMIT, , id ASC (sort tiebreaker).
• The comparison operator tokens: =, IS DISTINCT FROM, IS NULL, IS NOT NULL, >, >=, <, <=, = ANY(.
• The sort direction tokens: ASC, DESC.
• The jsonb_extract_path_text(data opening + closing ).

Parameter-bound (every byte of user input):

• app_id (the cross-app isolation gate, always $1).
• collection (always $2).
• Every field-path segment (one $N per segment).
• Every comparison value (one $N per condition).
• The $in value list as a single $N bound as TEXT[].
• The $limit integer as $N bound as BIGINT.

The SQL-shape guardrail test (docs_repo::sql_shape_tests::every_query_starts_with_app_id_and_collection_predicate) asserts every emitted query starts with the literal prefix SELECT id, data, created_at, updated_at FROM docs WHERE app_id = $1 AND collection = $2. The companion no_user_string_literal_in_sql and no_user_path_literal_in_sql tests pass a filter whose values contain SQL keywords ("gold; DROP TABLE docs;--", "drop_table_users") and assert those strings never appear in the emitted SQL.

Semantic corner cases

• $ne uses IS DISTINCT FROM (not <>). jsonb_extract_path_text returns SQL NULL for missing paths + JSON nulls; <> would silently exclude those rows from $ne results. Tested in docs_repo::sql_shape_tests::ne_with_value_uses_is_distinct_from.
• $eq null emits IS NULL; $ne null emits IS NOT NULL. Avoids any = NULL / <> NULL shenanigans.
• Comparison ops are text-lex per the brief's contract (Decision E, confirmed). Known limitation surfaced in CHANGELOG + this HANDBACK: '10' < '9' is TRUE under any text collation, so unpadded numeric comparisons break across digit-count boundaries. Workaround for users: zero-pad numeric strings. v1.2's advanced-query expansion will add numeric-aware operators.

4. Schema decisions (beyond the brief)

The brief sketched the docs table; I refined it as follows:

• GIN index uses jsonb_path_ops (smaller index, supports @> containment for equality filter shapes). The default jsonb_ops would accelerate path-existence queries too — irrelevant for the v1.1.2 operator set.
• Migration sequencing: two migrations (0013_docs.sql + 0014_docs_triggers.sql) instead of one. Separates the data-plane addition from the triggers-framework extension cleanly; either could be reverted independently if needed.
• CHECK constraint names: relied on Postgres's auto-name convention for inline column-CHECKs (<table>_<column>_check). Migration 0014 drops triggers_kind_check + outbox_source_kind_check and re-adds the widened constraints. The reviewer should confirm these auto-names match the inline definitions in 0008/0009 on a fresh Postgres before deploy.
• docs_trigger_details.ops is TEXT[] NOT NULL without a DEFAULT '{}' — matches kv_trigger_details.ops. Callers always supply a (possibly empty) array.
• No dispatch_mode column on docs_trigger_details — the parent triggers.dispatch_mode is sufficient. KV does the same.

5. Tests added (one line each)

crates/shared/src/docs.rs

(no tests — type definitions only; behavior tests live in manager-core)

crates/manager-core/src/docs_filter.rs (26 tests in mod tests)

• empty_object_has_no_conditions — {} parses to empty filter.
• single_equality_top_level — { tier: "gold" } → one Eq condition.
• multi_field_equality_is_conjunctive — two fields produce two AND'd conditions.
• nested_dotted_path — "user.email" parses to two segments.
• depth_limit_rejects_six_segments — 6-segment path errors.
• double_dot_rejected / leading_dot_rejected / trailing_dot_rejected — empty segment errors.
• dollar_prefix_in_path_segment_rejected — segment can't start with $.
• each_supported_operator_parses — parametric over all 7 v1.1.2 operators.
• dollar_in_with_non_array_value_rejected — $in: "scalar" errors.
• scalar_op_with_object_value_rejected — $gt: { ... } errors.
• unsupported_operator_message_pins_v1_2_pointer — snapshot of $regex error string.
• unsupported_top_level_modifier_rejected — $or errors with v1.2 pointer.
• depth_limit_message_pinned — snapshot of depth-limit error string.
• mixed_shape_operator_object_rejected — { $gt: 1, other: 2 } errors.
• sort_asc_and_desc_parse — $sort: { x: 1 } and { x: -1 }.
• sort_with_bad_direction_rejected — direction must be 1 or -1.
• multi_field_sort_rejected_with_v1_2_pointer — snapshot of multi-field-sort error string.
• limit_accepts_non_negative_integer / limit_clamps_to_max / limit_rejects_negative / limit_rejects_non_integer.
• non_object_filter_rejected.
• dollar_eq_value_can_be_null — JSON null is a valid scalar for $ne.
• implicit_equality_with_array_value_accepts — array-shape value is implicit equality.

crates/manager-core/src/docs_repo.rs (10 tests in mod sql_shape_tests)

• every_query_starts_with_app_id_and_collection_predicate — load-bearing: pins the cross-app isolation prefix across 8 representative filter shapes.
• no_user_string_literal_in_sql — value containing "DROP TABLE" never lands in SQL text.
• no_user_path_literal_in_sql — path "drop_table_users" never lands in SQL text.
• empty_filter_sql_has_no_extra_conditions — {} produces bare base WHERE.
• eq_with_null_emits_is_null / ne_with_null_emits_is_not_null / ne_with_value_uses_is_distinct_from — NULL handling.
• in_emits_any_array — $in uses = ANY(...).
• sort_appends_tiebreaker_id_asc — sort always has , id ASC tail.
• jsonb_extract_path_used_for_field_access — field paths route through jsonb_extract_path_text.

crates/manager-core/src/docs_service.rs (23 tests in mod tests)

• create_then_get_round_trips / get_missing_returns_none / update_present_succeeds / update_missing_returns_not_found / delete_present_returns_true / delete_missing_returns_false — basic CRUD shape.
• empty_collection_rejected — "" collection.
• create_with_non_object_data_rejected / update_with_non_object_data_rejected — data must be a JSON object.
• cross_app_isolation_via_cx_app_id — load-bearing: app A's docs aren't visible to app B's get or find.
• anonymous_cx_skips_authz — script-as-gate semantics.
• authed_cx_with_no_role_is_forbidden_on_read / …_on_write.
• owner_principal_can_write / editor_member_can_write_via_role.
• find_with_equality_returns_matches / find_with_dollar_in_returns_subset.
• find_one_returns_first_or_none / find_one_explicit_limit_is_honoured.
• find_with_unsupported_operator_throws / find_with_invalid_filter_throws.
• list_cursor_pagination.
• noop_emitter_does_not_block_mutations.

crates/manager-core/src/triggers_api.rs (3 new docs tests)

• docs_trigger_create_succeeds — happy path + verifies the TriggerDetails::Docs round-trips with the right ops.
• docs_trigger_empty_glob_rejected — " " rejects with Invalid.
• docs_trigger_member_without_role_is_forbidden — denying authz repo + member principal denies.

crates/executor-core/tests/sdk_docs.rs (15 bridge integration tests)

• docs_create_then_get_round_trip / docs_get_missing_returns_unit / docs_get_with_invalid_uuid_throws.
• docs_find_equality_returns_matches / docs_find_with_in_operator / docs_find_with_gt_comparison.
• docs_find_one_returns_envelope_or_unit.
• docs_update_then_get_reflects_change / docs_update_missing_throws.
• docs_delete_returns_was_present.
• docs_unsupported_operator_throws_with_v1_2_pointer.
• docs_empty_collection_name_throws.
• docs_list_returns_docs_array.
• docs_bridge_preserves_cross_app_isolation — load-bearing: bridge + service together enforce isolation.
• docs_envelope_has_id_data_created_at_updated_at — pins Decision D's envelope shape.

6. Open questions for the reviewer

1. CHECK constraint name verification — 0014 drops constraints named triggers_kind_check and outbox_source_kind_check (Postgres's default for inline column-CHECKs). Please verify by running migrations from scratch + a fresh \d+ triggers / \d+ outbox against a stage DB before merge. The CHANGELOG includes a downgrade caveat but the upgrade path itself depends on this name match.
2. docs_repo Postgres-integration tests — I wrote SQL-shape tests against the QueryBuilder output (pure, no DB) but did not add #[ignore]-gated Postgres tests for the CRUD path. v1.1.1 also did not add them for KV's Postgres impl; following the precedent. If the reviewer wants live-DB tests for docs as a project standard, they can land in a follow-up — happy to do them in this branch if preferred.
3. Parser promotion to picloud-shared — Decision B says promote in v1.2 when dead_letters::list reuses it. If the reviewer wants the rename now (picloud_shared::query::{Filter, FieldPath, ComparisonOp}) to avoid the future rename, that's a quick mechanical move.
4. Doc envelope future-proofing — Decision D ships the explicit envelope. If a soft-delete deleted_at field gets added in v1.2, it should land inside the envelope (not inside data). The trait + repo would need a new optional column; the envelope shape stays flexible for it.
5. Whether find should support null-LHS searches — $eq: null correctly returns docs where the field is JSON-null OR missing (both produce SQL NULL via jsonb_extract_path_text). A user may expect $eq: null to mean only JSON-null (not missing). The current behavior matches the simplest mental model but I want this confirmed.

7. Deferred items beyond what the brief calls out

• Postgres-integration tests for docs_repo — see Open Question 2.
• Dashboard surface for docs — no UI in v1.1.2 (the brief notes this is fine; KV doesn't have completions in rhai-mode.ts either). Listed as a future UX-polish task.
• Stable cursor encoding for find — the v1.1.2 find doesn't paginate (returns all matches up to $limit). The v1.2 expansion (advanced query) should add cursor pagination to find to match list's shape.
• Dispatcher unit test for docs routing — I considered extending the v1.1.1 dispatcher unit-test fixture (per the plan's test list) but the dispatcher's match-arm change is a single-line Kv | DeadLetter | Docs extension that's already covered by the existing Kv and DeadLetter arm tests. Adding a Docs clone wouldn't catch anything new; flagged here so the reviewer can decide.

8. How to verify locally

# 1. Lint + format + build + tests
cargo fmt --all -- --check
cargo clippy --all-targets --all-features -- -D warnings
cargo test --workspace

# 2. Fresh-DB migration test (assumes docker compose is set up)
docker compose down -v
docker compose up -d postgres
cargo run -p picloud      # observe 0001..0014 apply cleanly

# 3. Schema-on-top-of-v1.1.1 test
git checkout main
cargo run -p picloud      # runs migrations through 0012
git checkout feat/v1.1.2-documents
cargo run -p picloud      # observe 0013 + 0014 apply incrementally

# 4. End-to-end smoke (from the brief's "Done" checklist)
#    a. Create an app + script via existing admin endpoints
#    b. Bind the script to a route
#    c. From a Rhai script via the route, exercise:
#         let users = docs::collection("users");
#         let id = users.create(#{ name: "Alice", tier: "gold", age: 30 });
#         let doc = users.get(id);
#         assert(doc.data.name == "Alice");
#         let gold = users.find(#{ tier: "gold" });
#         assert(gold.len() == 1);
#         users.update(id, #{ name: "Alice", tier: "platinum", age: 30 });
#    d. POST /api/v1/admin/apps/{id}/triggers/docs pointing at a
#       logging handler script
#    e. Update or delete the doc; verify the handler fires with
#       ctx.event.docs.prev_data showing the prior state

# 5. Negative smoke
#    users.find(#{ "$or": [...] })  → throws with v1.2 message
#    users.find(#{ "a.b.c.d.e.f": "x" })  → depth-limit error
#    docs::collection("")  → empty-collection throw

Iteration-2 attestation — run against this branch's HEAD (bf26a25 chore: cargo fmt) immediately before writing this section:

Gate	Result
cargo fmt --all -- --check	exit 0 (no diff)
cargo clippy --all-targets --all-features -- -D warnings	exit 0 (no warnings)
cargo test --workspace	320 passed, 0 failed, 132 ignored (Postgres-integration tests gated as expected)

The 77 new tests for v1.1.2 (26 docs_filter + 10 docs_repo SQL-shape + 23 docs_service + 3 triggers_api docs + 15 bridge integration) are all included in the 320 pass total. The original v1 HANDBACK §8 claimed these were green; the audit found a fmt diff that contradicted that claim. The chore commit bf26a25 fixed the diff, and the table above is what cargo actually printed when I re-ran the gates after the fix. The HANDBACK update commit carries no code changes — it only replaces this section's text.

9. Known limitations / rough edges

• Text-lex comparison for $gt/$gte/$lt/$lte — per the brief's contract (Decision E). Breaks across digit-count boundaries ('10' < '9' is TRUE under any text collation). Documented in CHANGELOG. Workaround: zero-pad numeric strings. v1.2 advanced query adds numeric-aware operators.
• Concurrent update() prev_data race — the CTE pattern (WITH prev AS (SELECT) UPDATE) mirrors KV's set and inherits the same last-writer-wins race under READ COMMITTED: two simultaneous updates can both emit the same prev_data if their reads race. KV accepts this; docs follows. If audit-grade prev_data semantics are needed later, the fix is WITH old AS (SELECT … FOR UPDATE).
• Rollback from v1.1.2 → v1.1.1 with queued docs-source outbox rows will cause the v1.1.1 dispatcher to fail TriggerEvent::Docs deserialization (#[serde(tag = "source")] rejects unknown variants). Drain or delete outbox WHERE source_kind = 'docs' before downgrading. Trunk-only deployments don't hit this.
• find doesn't paginate — v1.1.2 returns all matches in one array (subject to $limit). Pagination on filter queries is deferred to v1.2's advanced query expansion.
• Filter Map ordering not guaranteed — Rhai's Map doesn't preserve insertion order, so when a filter contains multiple top-level fields the resulting WHERE clause's condition order can vary between runs. Result set is identical (AND is commutative); only the SQL string differs. No correctness impact.
• The find integration tests use a custom InMemoryDocs impl that does its own minimal filter eval (because the executor-core crate can't depend on manager-core's parser). The fake replicates the unsupported-operator throw path so the v1.2-pointer test exercises the bridge's error-propagation pipeline end to end.

Closing note

Reviewer audits the branch; on approval, the next step is to write REVIEW.md mirroring v1.1.1's audit-report format. The branch is ready.