PiCloud/REVIEW.md

# v1.1.2 Audit & Review

**Branch:** `feat/v1.1.2-documents`
**Base:** `main` (v1.1.1 head)
**Commits ahead:** 9 (7 substantive + 2 from iteration 2)
**Audited by:** reviewer (this report)
**Audited against:** the v1.1.2 dispatch prompt + the v1.1.1-shipped patterns the prompt mandated
**Iterations:** 2 (iteration 1 returned for a format fix; iteration 2 fixed it cleanly)

## Verdict

**APPROVE — ready to merge to `main` as v1.1.2.**

Substantive work was excellent on iteration 1; the only blocker was a single autoformatter diff at `docs_service.rs:456-457` that the iteration-1 HANDBACK incorrectly claimed was clean. Iteration 2 fixed the line (`bf26a25 chore: cargo fmt`), re-verified all three gates fresh on the new HEAD, replaced HANDBACK §8 with an honest attestation table, and explicitly recorded the discipline lesson in HANDBACK §1 for the v1.1.3 retro. Re-audit on the new HEAD is clean.

The 9-commit branch reads as a coherent release. Nothing else in the implementation needed changes between iterations.

---

## 1. Static checks reproduced (iteration 2 HEAD: `fedc63b`)

```
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 properly-ignored DB-backed
                                                                integration tests
```

Per-crate test breakdown:
- manager-core: 125 (62 new for v1.1.2: 26 docs_filter + 10 docs_repo sql-shape + 23 docs_service + 3 triggers_api docs)
- orchestrator-core: 56 (unchanged from v1.1.1)
- stdlib: 43 (unchanged)
- sdk_contract: 30 (unchanged)
- picloud: 21 (unchanged)
- executor-core engine: 17 (unchanged)
- sdk_kv: 7 (unchanged)
- sdk_docs: 15 (new in v1.1.2)
- shared: 6 (unchanged)

77 new tests — comfortably above the prompt's "30-50 new tests" target.

## 2. Design conformance (spot-checks)

All items below were verified on iteration 1 and remain unchanged on iteration 2's HEAD (the format fix touched only whitespace).

| Decision / requirement | Where it lives | Verdict |
|---|---|---|
| `docs::collection(name)` handle pattern + `::` namespace | [crates/executor-core/src/sdk/docs.rs](crates/executor-core/src/sdk/docs.rs) | ✅ Mirrors KV's shape exactly |
| Identity tuple `(app_id, collection, id)`, server-generated UUID | [0013_docs.sql:18-26](crates/manager-core/migrations/0013_docs.sql#L18-L26) | ✅ Primary key + server-generated id |
| Error convention (throw on failure, `()` for absent, `bool` for predicates) | [docs_service.rs](crates/manager-core/src/docs_service.rs), [sdk/docs.rs](crates/executor-core/src/sdk/docs.rs) | ✅ |
| `app_id` from `cx.app_id`, never from script args | Service layer + SQL builder | ✅ Cross-app isolation test covers service; `every_query_starts_with_app_id_and_collection_predicate` pins it at the builder |
| Query DSL: 7 operators only (`$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, `$in`) | [docs_filter.rs ComparisonOp](crates/manager-core/src/docs_filter.rs) | ✅ Enum has exactly 7 variants |
| Unsupported operators throw with v1.2 pointer | docs_filter parser + 3 snapshot tests | ✅ Snapshot tests pin the error wording |
| Dot-path field paths to depth 5 | [docs_filter.rs FieldPath::parse](crates/manager-core/src/docs_filter.rs) | ✅ Depth-limit + segment-validation tests |
| `$sort` single-field, `$limit` clamped | docs_filter parser | ✅ Multi-field-sort snapshot test; limit-clamp + negative-rejection tests |
| **SQL builder: every user input parameter-bound; no string interpolation** | [docs_repo.rs:319-420](crates/manager-core/src/docs_repo.rs#L319-L420) | ✅ Audited line-by-line; every value, every path segment, every `$in` array bound via `qb.push_bind(...)`. Only literal SQL is hardcoded keywords + operator tokens. `no_user_string_literal_in_sql` + `no_user_path_literal_in_sql` adversarial tests cover the safety net. |
| `WHERE app_id = $1 AND collection = $2` always first | `every_query_starts_with_app_id_and_collection_predicate` test pins this across 8 filter shapes | ✅ |
| `$ne` uses `IS DISTINCT FROM`; `$eq null` → `IS NULL`; `$ne null` → `IS NOT NULL` | docs_repo.rs `ComparisonOp::Ne` + tests | ✅ Avoids NULL-handling traps |
| `docs:*` triggers via Layout E extension | [0014_docs_triggers.sql](crates/manager-core/migrations/0014_docs_triggers.sql) + trigger_repo.rs | ✅ Mirrors `kv_trigger_details`; CHECK constraints widened (not replaced) |
| Dispatcher routes `OutboxSourceKind::Docs` | dispatcher.rs match-arm extension | ✅ One-line `Kv \| DeadLetter \| Docs` change; reuses generic resolution path |
| `ctx.event.docs.prev_data` change-data-capture | engine.rs `trigger_event_to_dynamic` + repo's update/delete return prior data | ✅ Works for update + delete; create has `prev_data = ()` |
| `Capability::AppDocsRead/Write` mapped to `script:read`/`script:write` (no new scopes) | [authz.rs](crates/manager-core/src/authz.rs) | ✅ Seven-scope commitment honored |
| Per-mutation `ServiceEvent` emission via injected emitter | [outbox_event_emitter.rs emit_docs](crates/manager-core/src/outbox_event_emitter.rs) | ✅ Best-effort emit after success; mirrors KV |

## 3. Substantive strengths

**SQL builder audit holds end-to-end.** [docs_repo.rs:319-420](crates/manager-core/src/docs_repo.rs#L319-L420) was traced line-by-line. Every user-controlled byte (path segments, scalar values, `$in` array contents, limit integer) is bound via `qb.push_bind(...)`. Only literal SQL the builder pushes is hardcoded keywords + operator tokens + structural punctuation. The cross-app isolation prefix is fixed at the top of every `build_find_query` call. The two adversarial-input tests (`no_user_string_literal_in_sql`, `no_user_path_literal_in_sql`) are exactly the safety net I'd want.

**`prev_data` CTE pattern is correct.** Returns `Some(prev_data)` from a `WITH old AS (SELECT) UPDATE ... RETURNING (SELECT data FROM old)` shape. The HANDBACK §9 "Concurrent update prev_data race" caveat is honest: under `READ COMMITTED`, two simultaneous updates can both report the same `prev_data`. Same tradeoff as KV. For audit-grade triggers (v1.2+) the escalation to `SELECT ... FOR UPDATE` is the right fix.

**Layout E extension is mechanically clean.** Adding `docs` as a trigger kind required exactly: one new `<kind>_trigger_details` table, two one-line CHECK widenings (`triggers.kind` + `outbox.source_kind`), one new `TriggerEvent::Docs` variant, one match-arm extension in the dispatcher. Future kinds (cron v1.1.4, pubsub v1.1.5) should follow this template — v1.1.2's implementation is the proof that Layout E pays its design rent.

**Operator-set is correct precedent.** The 7 operators are the right Pareto frontier — common cases that don't need parser infrastructure, while deferred operators (`$or`, `$and`, `$not`, `$regex`, `$exists`, etc.) all genuinely need infrastructure that v1.2 builds. The implicit-equality top-level + Mongo-style operator-object shape is consistent with what the TypeScript audience (v1.1.6 `@picloud/client`) will already know.

**Snapshot tests on error wording.** Three error messages pinned by snapshot tests (`$regex` rejection, multi-field-sort rejection, depth-limit rejection). Accidentally rephrasing during a future refactor will fail the build — right discipline because those strings are part of the user-facing contract.

## 4. Schema decisions audited

| HANDBACK §4 decision | Verdict |
|---|---|
| GIN with `jsonb_path_ops` opclass | ✅ Smaller index, accelerates `@>` containment; range operators fall back to scan within small `(app_id, collection)` partition |
| Two migrations (0013_docs.sql + 0014_docs_triggers.sql) | ✅ Each revertable independently |
| Auto-named CHECK constraints | ✅ Postgres's `<table>_<column>_check` convention is stable 9.6+; works as designed |
| `docs_trigger_details.ops` without `DEFAULT '{}'` | ✅ Mirrors KV |
| No `dispatch_mode` on `docs_trigger_details` | ✅ Parent column suffices |

## 5. HANDBACK open questions — my answers

**Q1: CHECK-constraint name verification.** The auto-naming convention `<table>_<column>_check` is stable in Postgres 9.6+. Run a fresh-DB migration test before deploy as recommended, but not expected to fail. **Not a merge blocker.**

**Q2: Postgres-integration tests for `docs_repo`.** Defer following v1.1.1's precedent (KV doesn't have live-DB tests either). If the project later decides live-DB tests are a workspace standard, that's its own PR adding both KV and docs together.

**Q3: Parser promotion to `picloud-shared` now or v1.2.** Defer to v1.2 as planned. Single consumer today; v1.2's "advanced query" expansion will mutate the parser's shape anyway; mechanical rename can land alongside `dead_letters::list`.

**Q4: Doc envelope future-proofing for `deleted_at`.** Current shape leaves it naturally addable as a sibling field of `data`. Right shape.

**Q5: `$eq: null` semantics.** Current behavior (matches both JSON-null and missing path) is correct for v1.1.2. Users who need to distinguish them can express that combination in v1.2 with `$exists: true AND $eq: null`.

## 6. Smaller observations

- `find` doesn't paginate in v1.1.2 — pagination on filter queries is deferred to v1.2 (HANDBACK §9). Acceptable.
- Filter `Map` ordering not stable (Rhai `Map` doesn't preserve insertion order). AND is commutative, so result sets are identical; only the emitted SQL string varies between runs.
- Text-lex comparison for range operators — `'10' < '9'` is TRUE under any text collation. Surfaced in CHANGELOG with the zero-pad workaround. v1.2's numeric-aware operators are the fix.
- Bridge integration tests use a custom `InMemoryDocs` fake that re-implements the unsupported-operator throw path (because executor-core can't depend on manager-core's parser). Acceptable; the real parser is exhaustively covered by manager-core unit tests.

## 7. Iteration 1 → iteration 2 deltas

Iteration 1 verdict was REQUEST-CHANGES on the sole basis of:
- `cargo fmt --check` failed at `docs_service.rs:456-457` (one-line collapse for the `$in` arm's `arr.iter().any(...)`)
- HANDBACK §8 explicitly claimed `cargo fmt --check` was green — false against the audited HEAD

Iteration 2 (2 new commits):
- `bf26a25 chore: cargo fmt` — the single-line collapse. Commit message honestly records the discipline gap ("the v1 HANDBACK §8 claimed `cargo fmt --check` was green; that claim was false against HEAD at audit time").
- `fedc63b docs(v1.1.2): handback §8 fresh post-fix attestation` — replaces §8's false claim with a verified-post-fix attestation table; adds an iteration note in §1 acknowledging the discipline gap for the v1.1.3 retro.

Re-verification on iteration-2 HEAD:
- fmt: exit 0 (no diff) ✓
- clippy: exit 0 (no warnings) ✓
- tests: 320 passed, 0 failed, 132 ignored ✓

All matches what the iteration-2 HANDBACK §8 claims. No drift between claim and reality this time.

## 8. Versioning audit

| File | Before | After | Status |
|---|---|---|---|
| Workspace `Cargo.toml` | 1.1.1 | 1.1.2 | ✅ |
| SDK schema (`shared/src/version.rs`) | 1.2 | 1.3 | ✅ Services bundle gains `docs: Arc<dyn DocsService>` |
| Dashboard `package.json` | 0.7.0 | 0.8.0 | ✅ (alignment with workspace) |
| Migrations | 0001..0012 | 0013, 0014 added | ✅ Sequential, no skips |
| CHANGELOG.md | v1.1.1 entry | v1.1.2 entry appended | ✅ |

## 9. Recommended next steps

1. **Merge** `feat/v1.1.2-documents` into `main` (fast-forward; branch is linear ahead).
2. **Pause** before dispatching v1.1.3 (Modules). The v1.1.2 work establishes the query-DSL precedent that v1.2 will lean on (`dead_letters::list`, "advanced docs query"); worth a brief mental check before the next dispatch that nothing in v1.1.2's shape has prompted a roadmap revision.
3. **Carry the discipline lesson forward.** The v1.1.3 prompt should include a "verify all three gates on the exact commit you're handing back, then write HANDBACK §8 from that fresh output" reminder. Cost is one sentence; benefit is removing the only audit finding from v1.1.2.

Branch ready for merge. **Verdict: APPROVE.**