PiCloud/REVIEW.md

v1.1.1 Audit & Review

Branch: feat/v1.1.1-storage-and-events Base: main (v1.1.0) Commits ahead: 12 Audited by: reviewer (this report) Audited against: docs/v1.1.x-design-notes.md §1–4 (Decided 2026-06-01) + the original v1.1.1 dispatch prompt

Verdict

APPROVE — ready to merge to main as v1.1.1.

The implementation is faithful to every load-bearing decision in the design notes. Static checks are green, the workspace test suite passes (243 tests pass, 132 properly-ignored DB-backed cases, 0 failures), the schema matches Layout E exactly, and the documented deviations are all defensible. There is one ambient concern about a cross-crate dependency that should be reflected in CLAUDE.md after the merge, but it is not a merge blocker.

---

1. Static checks reproduced

cargo fmt --all -- --check         ✅ clean
cargo clippy --all-targets --all-features -- -D warnings   ✅ no findings
cargo test --workspace             ✅ 243 passed / 0 failed
                                      (132 ignored — DB-backed integration tests,
                                       same convention as v1.1.0; documented in HANDBACK §4)

Test distribution per crate matches HANDBACK §4:

• manager-core: 63
• orchestrator-core: 56
• stdlib: 43
• sdk_contract: 30
• picloud: 21
• executor-core (engine): 17
• sdk_kv: 7
• shared: 6

47 of these are new in v1.1.1; the rest are v1.1.0's existing suite still passing.

2. Design-notes conformance (spot-checks)

Decision	Where it lives	Verdict
Layout E trigger storage (parent + per-kind detail)	0008_triggers.sql:22-72	✅ matches exactly; parent has common columns + the four retry/dispatch knobs + registered_by_principal; per-kind detail tables for kv and dead_letter only
routes stays separate from triggers parent	0012_routes_dispatch_mode.sql, 0009_outbox.sql:13-18	✅ HTTP rows use source_kind = 'http' and trigger_id references routes.id; non-HTTP references triggers.id; polymorphism in Rust per the design-notes deferral of the column-set refinement
Sync HTTP via outbox + NATS-style inbox	inbox.rs:30-89, dispatcher.rs:359-394	✅ oneshot::Sender<InboxResult> keyed by inbox_id; deliver() returns Delivered or Abandoned exactly per the design-notes failure-mode table
reply_to.is_some() never retries	dispatcher.rs:376-394	✅ failure path checks reply_to first; delivers single outcome to inbox; deletes outbox row regardless of error
Status code table (422/502/503/504/507/500)	dispatcher.rs:555-564, test failure_kind_status_codes_match_design_notes	✅ exact mapping; covered by a dedicated test
dispatch_mode = async returns 202 Accepted + JSON body	api.rs:325-332	✅ body shape is {"accepted_at": rfc3339, "execution_id": uuid} — matches design notes §2 verbatim
Default retry: 3/exp/1000ms/±20% jitter	trigger_config.rs, tests exponential_backoff_doubles_per_attempt, jitter_within_pct_of_base	✅ env-overridable; jitter test exercises the ±20% bound across 100 samples
abandoned_executions written on dropped receiver	dispatcher.rs:480-509	✅ written only when InboxDeliveryOutcome::Abandoned returns; ordinary timeout-with-receiver-still-alive does not write a row
Dead-letter recursion stop (flag on execution)	dispatcher.rs:396-425, trigger_repo.rs TriggerKind::DeadLetter → is_dead_letter_handler	✅ flag set when dispatcher resolves a kind = 'dead_letter' trigger; on failure, original DL annotated with resolution = 'handler_failed', row deleted, never retried, never DL'd
Sync HTTP failures do NOT dead-letter	dispatcher.rs:378-394	✅ early return before the DL-write block
dead_letters::list NOT shipped (deferred to v1.2)	executor-core/src/sdk/dead_letters.rs:13	✅ explicit doc-comment citing design notes §4; only replay + resolve registered
Trigger execution runs as registrant's principal	dispatcher.rs:249-253 + registered_by_principal column	✅ principal resolved from the trigger row at dispatch time
30-day DL retention, env-overridable	gc.rs	✅
7-day abandoned-executions retention	gc.rs	✅
Trigger-depth limit (default 8); depth-exceeded does NOT dead-letter	dispatcher.rs:122-137	✅ design-notes §4 honored ("depth-exceeded means you built a loop") — row dropped + logged, no DL spam
Dashboard surface: badge + list view + Replay + Mark resolved	dashboard/src/routes/apps/+page.svelte, dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte	✅ all required columns + actions + expandable row detail; npm run check reports 0 errors
Status: workspace 1.1.0 → 1.1.1, SDK 1.1 → 1.2, dashboard 0.6.0 → 0.7.0, CHANGELOG.md created	last commit 66b661f	✅
ctx.event shape (KV: source/op/collection/key/value; DL: original/attempts/last_error/ids/timestamps)	shared/src/trigger_event.rs, executor-core engine tests	✅ matches design notes §4 shape exactly; tests verify both variants + the "absent for direct invocations" rule

I sampled the design-notes diff (git diff main..HEAD -- docs/v1.1.x-design-notes.md) — every "Decided 2026-06-01" entry the agent absorbed into commit 1efb350 matches the decisions made in conversation. No drift.

3. Deviations from the prompt (all reviewed, all acceptable)

The HANDBACK's §3 lists nine deviations / mid-implementation decisions. My take on each:

1. Outbox column set chosen (script_id, trigger_id polymorphic, claimed_by TEXT, trigger_depth, root_execution_id denormalized; no is_dead_letter_handler column). The design notes explicitly deferred this set to implementation. The chosen shape is sensible: dispatcher can build ExecRequest without re-joining; the is_dead_letter_handler derivation from triggers.kind at dispatch time is cleaner than storing redundant state. ✅

2. KV pagination is cursor-style (base64-encoded last-key, 100 default / 1000 max). The prompt left this open; cursor-style is the right default for KV-shaped data. ✅

3. KV TTL deferred. Blueprint §8.1 reserved expires_at but v1.1.1 SDK doesn't surface TTL. Omitting the column from migration 0007 keeps the schema minimal; adding it later is a non-breaking forward migration. ✅ (CHANGELOG records the deferral.)

4. Authz scope mapping (4 new capabilities mapped to existing 7 scopes — AppKvRead → script:read, AppKvWrite → script:write, AppManageTriggers → app:admin, AppDeadLetterManage → app:admin). The "seven-scope commitment" is a project convention in crates/shared/src/auth.rs:103 the prompt didn't mention; honoring it is correct. The specific mapping is defensible: a token with script:read on an app already implies "can see the data behind those scripts," and admin-level scope for trigger/DL management is standard for control-plane operations. ✅

5. Script-as-gate authz (if cx.principal.is_some() then check; else skip — public HTTP runs anonymously without an authz failure). This matches the SDK-shape doc's note that "the data plane is unauthenticated by default — public HTTP scripts run with None." Cross-app isolation is preserved regardless (every query keyed by cx.app_id). DL replay/resolve correctly bypasses this and hard-requires a principal. ✅

6. Trait split OutboxRepo vs OutboxWriter. Orchestrator-core can't depend on manager-core; the small OutboxWriter trait in shared (one method) lets the orchestrator enqueue HTTP rows without inverting the dependency arrow. ✅ Pattern mirrors the existing members_concrete/AuthzRepo split.

7. InboxResolver in shared, InboxRegistry in orchestrator-core. Same split rationale. Cluster mode (v1.3+) swaps the impl behind the unchanged trait. ✅

8. manager-core now depends on executor-core. ⚠️ See §4 below — flagged, accepted, but should be reflected in CLAUDE.md.

9. Sync HTTP via outbox is the default for user routes (admin bypass /api/v1/execute/{id} keeps direct dispatch). Matches the design-notes decision; the bypass's direct path is acceptable for admin tooling speed. ✅

4. The one concern worth surfacing: manager-core → executor-core

CLAUDE.md working rules say:

Honor the three-service boundary. Don't reach across *-core crates. If orchestrator-core needs something from manager-core, define a trait in shared and inject the impl.

The dispatcher in manager-core directly imports ExecRequest, ExecResponse, ExecError, and InvocationType from executor-core:

// crates/manager-core/src/dispatcher.rs:27
use picloud_executor_core::{ExecError, ExecRequest, ExecResponse, InvocationType};

The HANDBACK justifies this as "DTOs vs behavior — types are fine, behavior is the bright line." That's a defensible interpretation, but not what CLAUDE.md actually says.

Two options the project can pick:

• (a) Accept the dependency and update CLAUDE.md to clarify that the three-service boundary is about behavior, not types — ExecRequest/ExecResponse/ExecError are transport DTOs and crossing the wire is normal. This is the lower-friction choice and matches how the agent's instincts ran.
• (b) Refactor: move ExecRequest/ExecResponse/ExecError/InvocationType to shared. About 200 lines of moves; would land cleanly as a follow-up PR.

My recommendation: (a). The dispatcher genuinely needs to construct and interpret these types, and they're the natural "what the executor produces" surface — burying them in shared makes the executor's public API less self-contained. But the rule as currently written disagrees; we should pick one explicitly.

This is not a merge blocker for v1.1.1 — the implementation already exists and works. The CLAUDE.md update can land as a small commit on main after the merge.

5. Smaller observations (no action required)

• HTTP outbox rows synthesize a ResolvedTrigger with a sentinel zero AdminUserId (dispatcher.rs:342). The HANDBACK flags this as a code smell; I agree, but the cleaner shape (enum DispatchTarget { Trigger(ResolvedTrigger), Http(HttpRoute) }) is a refactor that doesn't belong in v1.1.1. Worth doing in v1.1.2 alongside the docs work since the dispatcher will gain another trigger kind.
• Triggers parent dispatch_mode defaults to 'async' (0008_triggers.sql:30) with sync allowed by the CHECK constraint but unsupported in v1.1.1 (sync trigger would mean firing inline with the originating mutation, which we don't do). The migration comment captures this; worth a future commit to either remove 'sync' from the CHECK or use it for an inline_pre_mutate semantics if it ever makes sense. Not v1.1.1's problem.
• Metric counters are TODO at three call sites (picloud_trigger_depth_exceeded, picloud_dead_letter_handler_failures, picloud_abandoned_executions_total). The events are logged via tracing::warn/error in the meantime. Per the prompt and roadmap, metrics surface is v1.1.7+. ✅
• Dispatcher tick cadence is 100ms with CLAIM_BATCH = 8, serial per tick. The ExecutionGate bounds total concurrent executions globally, so parallelism within a tick is purely an optimization. Reasonable MVP choice; can parallelize later without changing semantics.
• Open Q1 in HANDBACK (claimed-rows-stuck-on-crash) is a real cluster-mode concern, correctly out-of-scope for v1.1.1 (single dispatcher per process). Cluster mode adds a stale-claim sweeper — track for v1.3+.
• Open Q3 in HANDBACK (HTTP-triggered scripts run with principal: None) is correct as-is. The "trigger executions inherit the registrant's principal" decision applies to triggers; HTTP routes have no registrant in that sense. Public HTTP is anonymous by design.

6. Versioning audit

File	Before	After	Status
Workspace Cargo.toml (workspace.package.version)	1.1.0	1.1.1	✅
SDK schema version (shared/src/version.rs)	1.1	1.2	✅ correctly bumped — the SDK surface added KvService + DeadLetterService + TriggerEvent
Dashboard package.json	0.6.0	0.7.0	✅
Migrations	0001..0006	0007..0012 added	✅ sequential, no skips
CHANGELOG.md	not present	created at repo root	✅ first entry covers v1.1.1

7. Manual smoke recommendation

The reviewer (you) does not need to run the manual end-to-end smoke before merging — the automated tests + the static review above cover the contracts. The smoke flow in HANDBACK §7 is worth running after merge as a release-validation step before tagging v1.1.1 (if the project tags releases). Specifically:

1. docker compose up -d (fresh DB)
2. cargo run -p picloud
3. Create app + script-that-throws + KV trigger
4. Trigger a KV write → wait ~7s → confirm DL row appears
5. Dashboard: red badge on apps list, list view shows the row, Replay creates a new outbox row + dispatcher re-runs, Mark resolved sets resolution = 'ignored'
6. Async route test: POST /work with dispatch_mode=async route → expect 202 + JSON body

If any of those misbehave post-merge, revert is straightforward (12 commits, ahead of main, no dependencies have pulled changes yet).

8. Recommended next steps (post-merge)

1. Merge feat/v1.1.1-storage-and-events into main (fast-forward; branch is linear ahead).
2. Tag v1.1.1 if release tagging is the project convention (git log shows v1.1.0 had a release commit but I didn't see a tag — confirm with the project owner).
3. Small CLAUDE.md update clarifying the three-service boundary's scope (types crossing is fine; behavior crossing is what's prohibited). One-paragraph change.
4. Pause before dispatching the v1.1.2 (Documents) agent — the v1.1.1 work shipped substantial infrastructure that v1.1.2 will lean on, and there may be small lessons from the v1.1.1 implementation to fold into the v1.1.2 prompt (e.g., reaffirming the "manager-core depends on executor-core for DTOs" pattern explicitly so the docs agent doesn't second-guess it).

Branch is ready for merge. Verdict: APPROVE.