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	✅ Mirrors KV's shape exactly
Identity tuple (app_id, collection, id), server-generated UUID	0013_docs.sql:18-26	✅ Primary key + server-generated id
Error convention (throw on failure, () for absent, bool for predicates)	docs_service.rs, 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	✅ 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	✅ 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	✅ 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 + 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	✅ Seven-scope commitment honored
Per-mutation ServiceEvent emission via injected emitter	outbox_event_emitter.rs emit_docs	✅ Best-effort emit after success; mirrors KV

3. Substantive strengths

SQL builder audit holds end-to-end. docs_repo.rs:319-420 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.