PiCloud/HANDBACK.md

# v1.1.2 Implementation HANDBACK

## 1. Branch + commit count

- Branch: `feat/v1.1.2-documents`
- Base: `main`
- 7 commits ahead of `main`. Branch is **not pushed**, **not merged**.

```
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
```

## 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](crates/manager-core/src/dispatcher.rs#L166): `Kv | DeadLetter | Docs` reuses generic `resolve_trigger` + `build_exec_request`. |
| 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

```sh
# 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
```

`cargo test --workspace` from this branch passes 320+ tests (77 new for v1.1.2). `cargo fmt --check` and `cargo clippy -- -D warnings` are clean.

## 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.