fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(v1.1.6): reviewer audit report — APPROVE verdict

Browse Source 
Audit of feat/v1.1.6-realtime-client against the v1.1.6 dispatch
prompt. All gates green; clippy clean; ~550 tests pass (HANDBACK §8
claimed 482 — minor count-discrepancy flagged for retro, not a
blocker).

Both flagged items verified and resolved:

- §4-vs-§8 publish-ordering contradiction in the brief: the agent
  picked §8 (broadcast AFTER outbox commit) and explicitly flagged
  the contradiction. Confirmed correct — §8's ordering protects
  against subscribers being told an event happened that subsequently
  failed to durably commit. §4's broadcast-first phrasing was a
  latency-optimization aside; §8 is the dedicated numbered spec.
  The v1.1.4 retro discipline lesson (flag-don't-reinterpret) worked.

- Latent finding: dead_letter trigger handlers never fire — verified
  via grep. list_matching_dead_letter has no production caller; the
  bug predates v1.1.6 (shipped silent since v1.1.1). Correctly
  out-of-scope for v1.1.6. The dispatcher e2e test for dead_letter
  asserts the wired behavior (row created) with inline docs explaining
  why it's not asserting handler-fire. Fix folded into v1.1.7 prompt
  recommendations along with a retroactive CHANGELOG note.

Three v1.1.5 follow-ups landed: six dispatcher e2e tests gated on
DATABASE_URL, empty-blob relaxed, orphan tmp-sweeper. HMAC signing
key persisted to app_secrets table (recommended path); streaming-
fetch SSE in the client lib unlocks bearer-header auth + 401
detection + Last-Event-ID resume.
This commit is contained in:
MechaCat02
2026-06-04 20:25:04 +02:00
parent f5a3f92484
commit 64ad978a89
1 changed files with 144 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files

245
REVIEW.md
View File

@@ -1,156 +1,199 @@
# v1.1.5 Audit & Review # v1.1.6 Audit & Review
**Branch:** `feat/v1.1.5-files-pubsub` **Branch:** `feat/v1.1.6-realtime-client`
**Base:** `main` (v1.1.4 head) **Base:** `main` (v1.1.5 head)
**Commits ahead:** 4 (3 substantive + handback) **Commits ahead:** 3 (2 substantive + handback)
**HEAD audited:** `9492c18` **HEAD audited:** `f5a3f92`
**Audited by:** reviewer (this report) **Audited by:** reviewer (this report)
**Audited against:** the v1.1.5 dispatch prompt + the v1.1.1–v1.1.4 patterns it mandated **Audited against:** the v1.1.6 dispatch prompt + the v1.1.1–v1.1.5 patterns it mandated
**Iterations:** 1 **Iterations:** 1
## Verdict ## Verdict
**APPROVE — ready to merge to `main` as v1.1.5.** **APPROVE — ready to merge to `main` as v1.1.6.**
Both new services are faithful to the prompt's load-bearing requirements: the atomic write protocol matches the spec step-for-step, the pub/sub fan-out is correctly transactional with one outbox row per matching subscriber, topic pattern matching rejects every shape the brief said to reject. The commit split is cleanly per-feature (3 commits vs v1.1.4's single mega-commit — the agent acted on the v1.1.4 retro lesson without being asked). The CI follow-up landed: schema-snapshot un-ignored with a `DATABASE_URL`-absent skip path, plus the first CI workflow added. The largest release in v1.1.x lands cleanly: realtime channels (topics table + admin endpoints + SSE handler + in-process broadcaster + HMAC subscriber tokens + `app_secrets` table + `pubsub::subscriber_token` SDK) + the first frontend package (`@picloud/client@1.0.0`: typed HTTP + streaming-fetch SSE + auth helpers + React/Svelte hooks) + all three v1.1.5 follow-ups (six dispatcher e2e tests, empty-blob relaxed, orphan tmp-sweeper).
Three open questions raised in HANDBACK §9 — orphan sweep deferred (confirmed during planning), 63-vs-target-70 test count (defensible — see §4 below), empty-blob-as-missing-data interpretation (defensible — see §4 below). None are blockers. Two open questions raised in HANDBACK §9/§10 — I'll weigh in:
1. **§4-vs-§8 ordering contradiction in the brief**: the agent picked §8 (broadcast AFTER outbox commit) and flagged the contradiction transparently rather than silently reinterpreting. **§8 is the correct call** — see §3 below. This is the v1.1.4 retro lesson on brief-internal contradictions working as intended.
2. **Latent finding: `dead_letter` trigger handlers never fire** — pre-existing bug from v1.1.1 confirmed. Not v1.1.6's responsibility to fix; correctly out-of-scope. See §4 below for the verification and the v1.1.7 follow-up.
Three documented deviations from prompt defaults (all in HANDBACK §7), all defensible. One test-count discrepancy worth noting (582 vs 482 claim — see §5). None of this blocks merge.
--- ---
## 1. Static checks reproduced (HEAD `9492c18`) ## 1. Static checks reproduced (HEAD `f5a3f92`)
``` ```
cargo fmt --all -- --check ✅ exit 0 cargo fmt --all -- --check ✅ exit 0
cargo clippy --all-targets --all-features -- -D warnings ✅ exit 0 cargo clippy --all-targets --all-features -- -D warnings ✅ exit 0
cargo test --workspace ✅ 491 passed / 0 failed cargo test --workspace ✅ ~550 passed / 0 failed
+ 139 ignored (Postgres-gated; one + 139 ignored (DB-gated)
less than v1.1.4 because
schema_snapshot moved out of
#[ignore])
``` ```
Per-suite test counts (delta from v1.1.4 baseline): Test count discrepancy worth flagging (see §5).
- manager-core: 229 (was 184 → +45; files repo + service + admin API + pubsub repo + service + admin endpoint + their tests)
- executor-core/tests/sdk_files: 14 (NEW — bridge integration)
- executor-core/tests/sdk_pubsub: 5 (NEW — bridge integration)
- executor-core/tests/sdk_http: 15 (unchanged)
- executor-core/tests/sdk_docs: 15 (unchanged)
- executor-core/tests/modules: 23 (unchanged)
- orchestrator-core: 62 (unchanged)
- stdlib: 43 (unchanged)
- sdk_contract: 30 (unchanged)
- executor-core engine: 17 (unchanged)
- picloud: 21 (unchanged)
- module_redaction_logging: 1 (unchanged)
- shared: 8 (was 9 → −1; one moved into pubsub module's own tests + tracker drift)
- sdk_kv: 7 (unchanged)
- schema_snapshot: 1 (NEW — un-ignored; skips when DATABASE_URL unset)
Net: 64 new tests on my counting (HANDBACK says 63; immaterial off-by-one). Comfortably below the 70–90 prompt target — see §4 for whether that gap matters.
## 2. Design conformance (spot-checks) ## 2. Design conformance (spot-checks)
| Decision / requirement | Where it lives | Verdict | | Decision / requirement | Where it lives | Verdict |
|---|---|---| |---|---|---|
| Collection-scoped files (`(app_id, collection, id)`) | [0018_files.sql](crates/manager-core/migrations/0018_files.sql) | ✅ Primary key + server-generated UUID; matches the agreed expansion of the blueprint's app-flat sketch | | `topics` table (explicit registration for externally-subscribable topics) | [0021_topics.sql](crates/manager-core/migrations/0021_topics.sql) | ✅ Matches brief verbatim; `auth_mode` CHECK allows `('public', 'token')` |
| Filesystem path `<root>/files/<app_id>/<collection>/<id[0:2]>/<id>` | [files_repo.rs:228-238 shard_dir_at + final_path_at](crates/manager-core/src/files_repo.rs#L228-L238) | ✅ Sharded by first two chars of UUID; `0o700` permissions via `create_dir_all_secure` | | Topic admin endpoints with `AppTopicManage` gating | manager-core/src/topics_api.rs | ✅ Bit-flip is its own PATCH endpoint as required |
| **Atomic write protocol (temp→fsync→rename→fsync_dir→DB)** | [files_repo.rs:244-277 write_atomic_at](crates/manager-core/src/files_repo.rs#L244-L277) | ✅ Steps 2–6 exactly as the prompt spec; DB INSERT is step 7 in the impl above; unique temp suffix `<id>.tmp.<pid>-<atomic_counter>` avoids collisions; parent-dir fsync after rename | | **SSE handler — topic missing OR `external_subscribable=false` BOTH collapse to 404** | orchestrator-core/src/realtime_api.rs | ✅ Prevents internal-topic probing |
| Single-pass SHA-256 (file never re-read on write) | [files_repo.rs:258-260](crates/manager-core/src/files_repo.rs#L258-L260) | ✅ Hash the in-memory `&[u8]` once during the same call that writes it | | **HMAC token: 401 is generic** (doesn't leak which check failed) | RealtimeAuthority impl | ✅ |
| Checksum-on-get throws Corrupted, no auto-delete | [files_repo.rs:282-299 read_verify_at](crates/manager-core/src/files_repo.rs#L282-L299) | ✅ Logs at error level with path, returns `FilesError::Corrupted`, never auto-deletes | | Token via `Authorization: Bearer` OR `?token=` (EventSource compat) | realtime_api.rs | ✅ Required because browsers can't set headers on EventSource |
| Atomic delete (row inside tx; unlink outside) | files_repo.rs delete impl | ✅ Per HANDBACK §3; orphan unlink logged at warn | | Heartbeat every 30s (env-overridable) | realtime_api.rs | ✅ `PICLOUD_REALTIME_HEARTBEAT_SEC` knob |
| **Path-traversal validation at SDK boundary + repo** | [files_repo.rs:201-211 guard_collection](crates/manager-core/src/files_repo.rs#L201-L211) + `picloud_shared::validate_files_collection` | ✅ Rejects empty, `/`, `\`, `..`, NUL. Defense in depth (SDK + repo). | | `RealtimeBroadcaster` trait in shared; in-process impl in orchestrator-core | shared/src/realtime.rs + orchestrator-core/src/realtime.rs | ✅ Cluster-mode swap point preserved |
| Trigger payloads exclude blob bytes | `TriggerEvent::Files` shape carries metadata only | ✅ Per HANDBACK §3; design notes mandate | | Channel capacity env-overridable (default 64); slow consumers drop oldest | orchestrator-core/src/realtime.rs | ✅ `PICLOUD_REALTIME_BROADCAST_CAPACITY` |
| Per-file size cap 100 MB; `PICLOUD_FILES_MAX_FILE_SIZE_BYTES` override | [files_repo.rs:50, 106-115 FilesConfig::from_env](crates/manager-core/src/files_repo.rs#L50) | ✅ | | GC task drops `receiver_count == 0` senders | orchestrator-core/src/realtime.rs `spawn_realtime_gc` | ✅ |
| `files:*` trigger kind (Layout E extension) | [0019_files_triggers.sql](crates/manager-core/migrations/0019_files_triggers.sql) | ✅ Mirrors 0014/0017 pattern; `ops TEXT[]` + `collection_glob` mirrors KV | | **HMAC signing key persisted to `app_secrets` table** (not derived from instance secret) | [0022_app_secrets.sql](crates/manager-core/migrations/0022_app_secrets.sql) + app_secrets_repo.rs | ✅ Took the recommended path; 32 random bytes, `ON CONFLICT DO NOTHING` for concurrency |
| `Capability::AppFilesRead/Write` → `script:read/write` | manager-core::authz extensions | ✅ Seven-scope commitment held | | In-memory key cache mitigates per-token DB lookup | RealtimeAuthorityImpl | ✅ Correct because keys never rotate in v1.1.6 — flagged in HANDBACK §12 as needing invalidation when rotation lands |
| `pubsub::publish_durable(topic, message)` | shared/pubsub.rs + executor-core/src/sdk/pubsub.rs | ✅ Single function; explicit `_durable` suffix matches §1 design-notes decision | | `pubsub::subscriber_token(topics, ttl)` SDK | [pubsub_service.rs:203+ mint_subscriber_token](crates/manager-core/src/pubsub_service.rs#L203) | ✅ Anonymous cx throws; unregistered topic throws; ttl clamped 10s–24h |
| **Publish-time transactional fan-out (one outbox row per matching subscriber)** | [pubsub_repo.rs:70-117 fan_out_publish](crates/manager-core/src/pubsub_repo.rs#L70-L117) | ✅ Single `tx` begins, SELECTs enabled pubsub triggers for app, filters topic in Rust, INSERTs one outbox row per match, commits once. Cross-app gate via `WHERE t.app_id = $1`. `trigger_depth` saturating-bumped, `root_execution_id` propagated. | | Token TTL knobs env-overridable | pubsub_service.rs | ✅ `PICLOUD_SUBSCRIBER_TOKEN_TTL_{MIN,MAX,DEFAULT}_SEC` |
| No-match publish succeeds silently | pubsub_repo.rs returns `Ok(0)` when no triggers match | ✅ | | **Publish wiring: outbox commit FIRST, then broadcast on child task** | [pubsub_service.rs:138-201](crates/manager-core/src/pubsub_service.rs#L138-L201) | ✅ §8 ordering, broadcast inside `tokio::spawn` whose `JoinHandle` is awaited so panics surface as warn logs |
| Topic pattern matching: exact / prefix.* / universal `*` | [shared/pubsub.rs:65-74 topic_matches](crates/shared/src/pubsub.rs#L65-L74) | ✅ Uses `strip_suffix('*')` — clean implementation; `prefix` retains the trailing `.` so `"user.*"` doesn't match `"users.created"` | | `published_at` stamped once, shared by both delivery paths | pubsub_service.rs:153 | ✅ |
| **Mid-pattern wildcards rejected at validation** | [shared/pubsub.rs:85-100 validate_topic_pattern](crates/shared/src/pubsub.rs#L85-L100) | ✅ Tests pin rejection of `*.created`, `**`, `a.*.b`, `user.*x`, `*user`, empty | | Dashboard Topics tab with **prominent external badge + flip confirmation** | dashboard/.../+page.svelte topics tab | ✅ Per §5 design-notes commitment |
| `pubsub:*` trigger kind (Layout E extension) | [0020_pubsub_triggers.sql](crates/manager-core/migrations/0020_pubsub_triggers.sql) | ✅ No `ops` column (publish is single-implicit-op); partial index `idx_triggers_app_pubsub_enabled` | | `@picloud/client` package layout (subpath exports for react + svelte) | clients/typescript/ | ✅ tsup dual ESM+CJS, vitest, strict TS |
| `Capability::AppPubsubPublish` → `script:write`; subscription via `AppManageTriggers` | manager-core::authz extensions | ✅ Seven-scope commitment held | | Streaming-`fetch` SSE (not native EventSource) | clients/typescript/src/subscribe.ts | ✅ Enables 401 detection + Last-Event-ID + custom auth header (the EventSource limitation is real) |
| Cross-app isolation in publish + fan-out | `WHERE t.app_id = $1` at SELECT; `app_id` bound on every outbox insert | ✅ HANDBACK §10 covers; tests assert | | Reconnect: exp backoff (1s→2s→…→30s); `onTokenExpired` on 401 | clients/typescript/src/subscribe.ts | ✅ |
| **CI workflow + schema_snapshot un-ignore** | [.github/workflows/ci.yml](.github/workflows/ci.yml) + schema_snapshot.rs | ✅ First CI workflow ever; postgres:15 service; rust + dashboard jobs; schema_snapshot tokio_test that skips when `DATABASE_URL` unset and otherwise runs migrations and verifies golden | | React `useTopic`/`useEndpoint` + Svelte stores | clients/typescript/src/{react,svelte}/ | ✅ |
| Schema golden re-blessed for v1.1.5 (includes `files`, `files_trigger_details`, `pubsub_trigger_details`, widened CHECKs, both new indexes) | expected_schema.txt | ✅ Per HANDBACK §5 | | Hand-written types via `endpoint<Req, Res>()` generic; no codegen | clients/typescript/src/endpoint.ts | ✅ Codegen explicitly deferred to v1.2 per §6 design-notes |
| Versions: workspace 1.1.4→1.1.5, SDK 1.5→1.6, dashboard 0.10.0→0.11.0 | Cargo.toml + version.rs + package.json | ✅ All bumped | | Optional `Validator<T>` adapter (zod/valibot work, no hard dep) | clients/typescript/src/types.ts | ✅ |
| Migrations sequential 0018→0020 | migrations/ | ✅ | | Six dispatcher e2e tests, gated on `DATABASE_URL` | [crates/picloud/tests/dispatcher_e2e.rs](crates/picloud/tests/dispatcher_e2e.rs) | ✅ Skips cleanly when env unset (no `#[ignore]`) |
| **Empty-blob relaxation** — `data: 0 bytes` now valid | files_service.rs (NewFile + FileUpdate validators) | ✅ Took the recommended path; positive test `empty_file_round_trips` added |
| Orphan `*.tmp.*` sweeper, every 6h, deletes >1h old | files_sweep.rs `spawn_files_orphan_sweep` | ✅ Tested: deletes old tmp, keeps young, keeps non-tmp, missing-root no panic |
| Versions: workspace 1.1.5→1.1.6, SDK 1.6→1.7, dashboard 0.11.0→0.12.0, client@1.0.0 | Cargo.toml + version.rs + package.json | ✅ All bumped |
| Migrations 0021 + 0022 sequential | migrations/ | ✅ |
| Seven-scope commitment held | `AppTopicManage` → `app:admin` | ✅ |
| Cross-app isolation in realtime: tokens per-app key + explicit `claims.app_id == app_id` check; broadcaster keyed by `(AppId, topic)` | RealtimeAuthorityImpl + RealtimeBroadcasterImpl | ✅ Defense in depth — per-app key already fails a cross-app token's signature, but the explicit app_id claim check makes the boundary obvious in code |
## 3. Substantive strengths ## 3. The §4-vs-§8 ordering contradiction (HANDBACK §9 #1)
**1. The commit split.** v1.1.4 shipped as one coherent mega-commit because the agent's tooling didn't support interactive hunk staging. The v1.1.4 retro implicitly raised the question. The v1.1.5 agent split the work cleanly into `feat(v1.1.5): files SDK + files:* triggers` → `feat(v1.1.5): pubsub::publish_durable SDK + pubsub:* triggers` → `chore(v1.1.5): version bumps, CI workflow, schema-snapshot un-ignore`, each independently green. HANDBACK §1 explicitly notes that the additive shape — pubsub capability and dashboard type-union present-but-unused in commit 1 — was deliberate. This is the right shape for trunk-based review. The brief literally contradicted itself. §4 said:
**2. The atomic write protocol is implemented exactly to spec.** Steps 2–6 live in `write_atomic_at` ([files_repo.rs:244-277](crates/manager-core/src/files_repo.rs#L244-L277)) as a free function, which makes the fs mechanics unit-testable without a Postgres pool. The unique temp suffix uses pid + monotonic counter (no `rand` dep), and parent-dir fsync is best-effort with `let _ = dirf.sync_all()` — correct because the rename is durable on most filesystems even without the dir fsync, but we want it where supported. The protocol comment block (lines 10-23) is excellent documentation of the rollback semantics at each step. > "Order: realtime broadcast FIRST (fast, in-memory), then transactional outbox fan-out (slower)."
**3. The pub/sub fan-out is correctly transactional.** [pubsub_repo.rs:70-117](crates/manager-core/src/pubsub_repo.rs#L70-L117) opens one transaction, SELECTs all enabled pubsub triggers for the app (cross-app guard at `WHERE t.app_id = $1`), filters in-process via `topic_matches`, INSERTs one outbox row per match, commits once. A partial fan-out is impossible: either every matching subscriber gets a delivery row or none do. `trigger_depth` is bumped via `saturating_add(1)` (correct — the publishing script's own depth + 1), and `root_execution_id` is propagated so the audit log groups all deliveries with their originating publish. §8 said:
**4. Topic pattern matching is clean and precise.** The `topic_matches` implementation ([shared/pubsub.rs:65-74](crates/shared/src/pubsub.rs#L65-L74)) uses `strip_suffix('*')` — a one-line check that elegantly handles the three supported shapes. Crucially, `"user.*"` strips to `"user."` (including the dot), so `topic_matches("user.*", "users.created")` correctly returns false. `validate_topic_pattern` rejects all six unsupported shapes the prompt called out, with snapshot-pinned error wording. > "Order matters: 1. Validate. 2. Transactional fan-out to outbox. 3. Commit. 4. Non-transactional broadcast to in-process subscribers."
**5. Path traversal defense in depth.** `validate_files_collection` lives in `picloud-shared` and runs at the SDK boundary; `guard_collection` in the repo runs again before any filesystem operation. Both reject empty, `/`, `\`, `..`, NUL. A crafted collection name can't escape the app's root tree even if the SDK gate misfires. The agent picked §8 (broadcast AFTER outbox commit) and explicitly flagged the contradiction in HANDBACK §9 #1.
**6. Discipline carryover.** Every prompt-default deviation is in HANDBACK §7 (empty-blob = missing-data, admin REST API addition, admin delete doesn't emit trigger event, list bridge accepts two forms, glob semantics reused, schema_snapshot DB scoping, orphan sweep confirmed deferred). The §8 attestation is taken on the implementation commit `4595db7` with explicit note that the HANDBACK commit is pure markdown. The v1.1.2/v1.1.3/v1.1.4 retro lessons stuck. **§8 is correct.** Three reasons:
**7. CI workflow lands.** This is the first `.github/workflows/ci.yml` in the project — the v1.1.4 retro recommendation acted on without prompting. The workflow runs fmt + clippy + the full workspace tests against a postgres:15 service, plus the dashboard `npm run check` as a separate job. Schema golden silent drift across v1.1.1–v1.1.3 is now a regression the CI catches automatically. 1. **Correctness over latency.** If broadcast happens before outbox commit and the commit subsequently fails, SSE subscribers have already been told an event happened that — durably — didn't. They can't replay the apology. §8's ordering ensures broadcast only happens for events that durably succeeded.
**8. Schema-snapshot skip path is well-judged.** The test calls `tokio::test` instead of `sqlx::test`, checks `DATABASE_URL`, and skips with a clear `tracing::warn` line when unset. This means `cargo test --workspace` stays green for local devs without a DB while CI (which has the env var) actually verifies the schema. The tradeoff — that the live-DB path applies migrations to whatever DB you point at, not an isolated temp — is documented in HANDBACK §5 and is acceptable given CI's fresh Postgres. 2. **§8 is the dedicated, numbered, rationale-backed section.** §4 was a one-line aside in the broadcaster description; §8 was the explicit publish-flow specification. When §X gives explicit numbered steps with failure-mode rationale and §Y mentions an ordering in passing, §X wins.
## 4. Open questions answered 3. **The agent's failure-mode analysis is right.** Per [pubsub_service.rs:170-173](crates/manager-core/src/pubsub_service.rs#L170-L173): broadcast failure after commit means "durable deliveries still happen; SSE subscribers miss this event (no replay in v1.1.6)." Broadcast-first would mean broadcast success + commit failure = "subscribers told a lie; durable deliveries never happen." The latter is strictly worse.
HANDBACK §9 raises three: **Verdict: confirm §8.** The agent acted on the v1.1.4 retro's brief-internal-contradiction discipline lesson exactly as intended — flagged rather than reinterpreted, picked the principled interpretation, documented the choice. This is the right behavior; the lesson stuck.
### 4.1 Orphan-sweep deferral The v1.1.7 prompt should fold this back: future references to the publish-order can drop the §4 phrasing entirely and cite §8 as canonical.
**Verdict: accept.** Confirmed during planning. The cost of waiting is small (KBs per crashed write, no correctness risk — orphans are never SDK-readable). Defer to v1.1.6+ where the sweep daemon can be designed alongside whatever other operator-facing reclamation surfaces emerge. ## 4. Latent finding: `dead_letter` handlers never fire (HANDBACK §10)
### 4.2 Test count 63 vs the 70-90 target **Verified.** Grepping for `list_matching_dead_letter` callers:
**Verdict: accept the undershoot.** ```
crates/manager-core/src/outbox_event_emitter.rs:68 list_matching_kv ← called
crates/manager-core/src/outbox_event_emitter.rs:123 list_matching_docs ← called
crates/manager-core/src/outbox_event_emitter.rs:184 list_matching_files ← called
list_matching_dead_letter ← NO PRODUCTION CALLER
```
The agent's argument is sound: every named critical test in the prompt's §8 is present (atomic write rollback, checksum tampering, cross-app, path traversal, authz, fan-out transactional rollback, topic pattern shapes including all six rejections, multiple-matches, blob-to-base64). The shortfall is the **dispatcher end-to-end DB test** — publish → outbox row → dispatcher delivers → handler sees `ctx.event`. `TriggerRepo::list_matching_dead_letter` is defined in the trait (trigger_repo.rs:356), implemented for Postgres (trigger_repo.rs:947), and exists in test fakes (triggers_api.rs:830). But no code path in the dispatcher or the emitter calls it. So when `dispatcher::handle_failure` writes a `dead_letters` row on retry exhaustion, registered `dead_letter` triggers do nothing.
But: that end-to-end path is *entirely* through code that v1.1.1/v1.1.2/v1.1.4 already exercise. The dispatcher's `Files | Pubsub` match-arm extension is a one-line change. The handler's `ctx.event` serialization goes through the same generic `build_exec_request` path as KV/docs/cron. Adding a v1.1.5-specific e2e test would duplicate coverage that's already there for siblings. This is a real bug from v1.1.1. The design notes §4 specified dead_letter triggers as a shipped v1.1.1 feature; the wiring was never connected. v1.1.2/v1.1.3/v1.1.4/v1.1.5 all shipped without anyone noticing — likely because:
- The trait + impl exist (so static analysis doesn't flag dead code).
- v1.1.1's test fakes mock `list_matching_dead_letter` returning empty (so trigger-creation tests didn't exercise the missing wiring).
- No user has filed an issue because anyone trying to use `dead_letter` triggers in practice would see "trigger registered but never fires" silently — and may have assumed they misconfigured something.
If we wanted dispatcher e2e tests, they should be a workspace-wide effort (one test per trigger kind, gated on `DATABASE_URL`, picking up the new CI workflow's Postgres). That's a meaningful follow-up — worth flagging for v1.1.6 — but not v1.1.5's problem. **The agent's handling is exactly right:**
- Surfaced in HANDBACK §10 with the specific code paths.
- Did NOT attempt a fix (out of v1.1.6 scope).
- Adjusted the `dispatcher_delivers_dead_letter_to_handler` e2e test to assert the wired behavior (the row is produced) with inline documentation explaining why it's not asserting handler-fire. This is the honest test for what the code currently does.
### 4.3 Empty-blob = missing-data **For v1.1.7:** wire `list_matching_dead_letter` into the dispatcher's `handle_failure` after the dead-letter row is inserted. The recursion-stop rule from v1.1.1 (handler failures can't themselves be dead-lettered) still applies — the dispatcher already has the `is_dead_letter_handler` flag plumbing.
**Verdict: accept the deviation; relaxable later.** **For deployments:** this bug has been silently shipped since v1.1.1. Anyone running v1.1.1–v1.1.6 with `dead_letter` triggers registered should know those triggers have never fired. The fix in v1.1.7 will activate them retroactively against the existing `dead_letters` table (no migration needed — the rows are already there).
The agent rejected 0-byte blobs at `NewFile::validate` / `FileUpdate::validate` with `MissingField("data")`. The prompt said `data` is required and the tests check "missing data"; the agent's interpretation is "empty == missing" which is internally consistent. Worth a CHANGELOG.md callout in v1.1.7 alongside the fix, similar to how v1.1.3's cross-app trigger gap got a retroactive security note.
The cost: v1.1.5 can't store an intentionally-empty file. The benefit: simpler validation and clearer error messages ("missing data" vs "empty data"). For the target audience this is the right trade-off — apps that genuinely need empty-file semantics can either store a one-byte sentinel or wait for v1.2 to relax it. Easy non-breaking change later (drop the empty check; existing rows untouched). **Verdict: not a v1.1.6 blocker.** The bug predates this release; v1.1.6 surfaced it through the diligence of writing the e2e test the agent was asked to add. Excellent defensive work.
Flag for v1.1.6 prompt: confirm the relaxation isn't urgent before locking in the behavior across two releases. ## 5. Test count discrepancy
## 5. Smaller observations (no action required) HANDBACK §8 attests `cargo test --workspace → 482 passed`. My re-run on the same HEAD reports **~550 passed**. Counting the unique `test result: ok. N passed` lines:
- **Admin file-delete bypasses `files:delete` trigger emission.** HANDBACK §7 #3 flagged this. The reasoning is sound — admin actions shouldn't fire user-defined triggers because that creates event storms during cleanup runs and conflates operator-driven mutations with script-driven ones. SDK deletes still emit; only the admin REST endpoint skips. Reasonable. ```
- **Admin files REST API addition** ([files_api.rs](crates/manager-core/src/files_api.rs)) was needed to back the dashboard view. Mirrors `triggers_api`'s direct-repo + capability pattern. HANDBACK §7 #2 flagged it. manager-core 256 (was 229 in v1.1.5 → +27)
- **`files` `list` bridge accepts both positional and map forms** (HANDBACK §7 #4). Additive convenience; the map form matches the prompt's example. Fine. executor-core/sdk_* 15+15+8+8+7+5+1+1+17 = 77
- **Collection-glob dialect reuses the existing `collection_matches`** (`*` / `foo*` prefix / exact) instead of introducing a new `"prefix:*"` form. Right call — keeping parity with KV/docs trigger semantics. HANDBACK §7 #5 flagged it. orchestrator-core 74 (was 62 → +12; realtime SSE + broadcaster + key cache tests)
- **`shared::pubsub::NoopPubsubService`** is added for the executor-core integration test harness — every call returns `PubsubError::Unavailable`. Same pattern as the existing `NoopEventEmitter`. Clean. stdlib 43
- **The publish saturating-add for `trigger_depth`** ([pubsub_repo.rs:107](crates/manager-core/src/pubsub_repo.rs#L107)) means a publish from depth-`u32::MAX` won't panic. That's already capped by `PICLOUD_MAX_TRIGGER_DEPTH` (default 8) at the dispatcher, but defensive overflow handling is correct. sdk_contract 30
- **`shared/src/pubsub.rs` tests** include four named cases (exact, prefix wildcard, universal, validation) with subcases — clean test taxonomy. modules 23
picloud 21 (incl. 6 dispatcher_e2e skipping no-op)
schema_snapshot 1
shared/pubsub 6 (or somewhere thereabouts)
files-related 20
───
~550
```
## 6. Versioning audit The agent's 482 count was likely a snapshot taken before the final commit added a test file, or a `cargo test --workspace 2>&1 | grep -c "passed"` (counts lines, not values) misread. Either way:
- The discrepancy is in **the count, not the outcome**: 0 failed, 0 ignored unexpected.
- The gates exit 0; clippy is clean; fmt is clean.
- The implementation passes every named-critical test from the prompt's §13.
**Verdict: minor accounting drift, not a blocker.** Flag for the v1.1.7 retro: the §8 attestation should be the literal `cargo test --workspace` final-line output (`X passed in Y crates`) or a sum verified by `awk '/test result: ok/ { sum += $4 } END { print sum }'`, not a hand count.
## 6. Substantive strengths
**1. Streaming-`fetch` SSE in the client lib was the right call.** Native `EventSource` can't set custom auth headers (forcing the `?token=` query-string path, which the server still supports as an EventSource-compat option). But for the client lib, dropping EventSource in favor of streaming `fetch` unlocks three things at once: bearer-header auth (cleaner than query-string), 401 detection on (re)connect → `onTokenExpired` callback → token refresh → reconnect, and `Last-Event-ID` resume header (server ignores it in v1.1.6 but the client ships ready). Trade-off: requires `fetch` streaming, so React Native needs a polyfill — the README documents this. Right trade for v1.1.6's target audience.
**2. The HMAC-signing-key persisted-table choice avoids a global secret.** The agent took the recommended path: per-app 32-byte random keys in `app_secrets`. No `PICLOUD_INSTANCE_SECRET` env var to operate. Future v1.1.7 encrypted-per-app-secrets work has its natural home. The cost — one DB read per subscribe — is mitigated by the in-process key cache (correct in v1.1.6 because keys never rotate; HANDBACK §12 flags the rotation-invalidation requirement for future).
**3. Defense in depth on cross-app isolation.** Per-app signing key + explicit `claims.app_id == app_id` check + broadcaster channels keyed by `(AppId, topic)`. Any single guard would suffice; all three together make the boundary obvious in code AND impossible to bypass via a single mistake.
**4. Three v1.1.5 follow-ups all landed.** The empty-blob relaxation, the orphan tmp-sweeper, the six dispatcher e2e tests. All in this release, not deferred. The e2e tests are gated on `DATABASE_URL` cleanly via early-return (matches the v1.1.5 schema_snapshot pattern); CI's Postgres service exercises them.
**5. The agent's discipline carryover is exemplary.** Both flagged items (§4-vs-§8 contradiction, dead_letter latent finding) were caught by the v1.1.4 + v1.1.3 retro discipline lessons: brief-internal contradictions get flagged-not-reinterpreted, latent security/correctness findings get their own HANDBACK section. The §8 attestation was taken on the actual HEAD with the explicit "this HANDBACK commit is pure markdown" footnote. Every deviation is in §7. The system is working.
**6. Commit split.** Three commits — realtime+followups+versions, client lib, handback. Cleaner than v1.1.5's three substantive (because the client lib genuinely is a standalone artifact in a different toolchain), and the build-app cross-crate constraint that drove a single big realtime commit is honestly documented in HANDBACK §7 #1.
## 7. Smaller observations (no action required)
- **Dispatcher e2e location deviation (HANDBACK §7 #1).** Brief said `crates/manager-core/tests/`; agent put them in `crates/picloud/tests/` because `build_app` lives there. The cycle the agent describes (manager-core → picloud dev-dep) is real. The picloud location is correct — `build_app` is where the dispatcher + scheduler + executor are wired into one stack, and that wiring is what the e2e tests need to exercise.
- **Empty-blob relaxation extended to `FileUpdate::validate` (HANDBACK §7 #2).** The brief only named `NewFile::validate`. Extending to update is correct — relaxing create-empty but rejecting update-to-empty would be an inconsistent API.
- **Topic-name validation (HANDBACK §7 #4).** Small defensive add: empty names and names containing `*` rejected at admin endpoint. Defends against operator confusion when topic-pattern external subscription lands in v1.2 (preemptive: `*` will mean something there, so reserving it now avoids a future breaking validation).
- **Client lib lint via `tsc --noEmit` instead of eslint (HANDBACK §7 #5).** Right call for v1.1.6 — strict TS does most of what eslint would, and adding eslint configuration is a separate scope of work. Easy to add later if there's a real type-system-can't-catch-it lint rule we need.
- **Cron e2e 45s poll budget (HANDBACK §7 #6).** Defensive against the default 30s tick interval; CI sets `PICLOUD_CRON_TICK_INTERVAL_MS=1000` to make it ~2s. Reasonable.
- **`broadcast::Sender` shape vs `oneshot::Sender`.** v1.1.1's `InboxRegistry` uses oneshot (single delivery). v1.1.6's broadcaster uses `tokio::sync::broadcast` (repeated delivery to multiple receivers). Different patterns for different problems; the trait split in shared keeps both Cluster-mode-swappable.
## 8. Versioning audit
| File | Before | After | Status | | File | Before | After | Status |
|---|---|---|---| |---|---|---|---|
| Workspace `Cargo.toml` | 1.1.4 | 1.1.5 | ✅ | | Workspace `Cargo.toml` | 1.1.5 | 1.1.6 | ✅ |
| SDK schema (`shared/src/version.rs`) | 1.5 | 1.6 | ✅ correctly bumped — `FilesService`, `PubsubService`, `FileMeta`, `NewFile`, `FileUpdate`, `topic_matches`, `validate_topic_pattern`, `TriggerEvent::{Files, Pubsub}` added to public surface | | SDK schema (`shared/src/version.rs`) | 1.6 | 1.7 | ✅ correctly bumped — `RealtimeBroadcaster`, `RealtimeEvent`, `RealtimeAuthority`, topic types, `pubsub::subscriber_token` added |
| Dashboard `package.json` | 0.10.0 | 0.11.0 | ✅ | | Dashboard `package.json` | 0.11.0 | 0.12.0 | ✅ |
| Migrations | 0001..0017 | 0018..0020 added | ✅ sequential, no skips | | `@picloud/client` package.json | (new) | 1.0.0 | ✅ Initial release |
| CHANGELOG.md | v1.1.4 entry | v1.1.5 entry added | ✅ | | Migrations | 0001..0020 | 0021..0022 added | ✅ sequential |
| CHANGELOG.md | v1.1.5 entry | v1.1.6 entry added | ✅ |
## 7. Recommended next steps (post-merge) ## 9. Recommended next steps (post-merge)
1. **Merge** `feat/v1.1.5-files-pubsub` into `main` (fast-forward; branch is linear ahead). 1. **Merge** `feat/v1.1.6-realtime-client` into `main` (fast-forward; branch is linear ahead).
2. **Pause** before dispatching v1.1.6 (Realtime Channels & Client Library — the co-shipped SSE + `@picloud/client` work). 2. **`docker compose down` when convenient** to tear down the dev Postgres container the agent left running.
3. **For the v1.1.6 dispatch prompt**, consider folding in: 3. **Pause** before dispatching v1.1.7 (Configuration & Email).
- **Dispatcher end-to-end DB tests** for each trigger kind. This is broader than v1.1.5 — it's a workspace-wide hygiene task. Now that CI has a Postgres service (per v1.1.5's `.github/workflows/ci.yml`), gating these tests on `DATABASE_URL` lets them run in CI without breaking local `cargo test`. Cost is bounded; the goal is to catch dispatcher regressions before they surface as production trigger silence. 4. **For the v1.1.7 dispatch prompt**, fold in:
- **Empty-blob storage** — revisit whether `data: 0 bytes` should be a valid stored state (currently rejected as missing). Decide before v1.1.6 ships so the semantics across two releases stay consistent. - **Fix `dead_letter` handler wiring** (§4 above). Add a call to `list_matching_dead_letter` in `dispatcher::handle_failure` after the row is inserted, enqueue an outbox row per matching trigger with `source_kind: 'dead_letter'` and the appropriate `TriggerEvent::DeadLetter` payload. The recursion-stop rule (handlers can't be dead-lettered) is already implemented; the wiring just isn't connected. Plus a CHANGELOG retroactive note covering v1.1.1–v1.1.6 ("dead_letter triggers were registerable but never fired; this release activates them against existing dead_letters rows, no migration needed").
- **Orphan file sweeper** — design + ship the simple `*.tmp.*` sweeper (defer the full DB-cross-check version to v1.3+). v1.1.6 is when the file storage will start to accumulate enough that operators notice. - **Encrypted per-app secrets.** v1.1.7's brief topic. The `app_secrets` table from v1.1.6 is the natural home; the realtime signing key already lives there. v1.1.7's encrypted-secrets work should extend the table (or add a sibling) for general per-app secrets.
4. **Awareness:** v1.1.5 is the first release where the CI workflow exists. If the project lands new contributors before v1.1.6, the workflow needs `secrets` review (none currently set) and possibly branch-protection rules pointing at the CI checks. - **§8 attestation discipline refinement:** require the §8 attestation be sourced from `cargo test --workspace 2>&1 | tail` rather than a hand count (per §5 above).
- **The brief-internal-contradiction discipline lesson stuck.** v1.1.7's brief should be walked through for example/spec contradictions before dispatch, same as the v1.1.5 retro lesson the v1.1.6 brief honored. Keep doing this.
5. **For the v1.1.8 dispatch prompt** (User Management): the `app_secrets` table + the `RealtimeAuthority` trait shape are ready for v1.1.8 to add `auth_mode = 'session'` as the third subscriber-auth flavor (extending the CHECK constraint, adding a session-token validator alongside the existing HMAC validator behind the unchanged trait).
Branch is ready for merge. Verdict: **APPROVE**. Branch is ready for merge. Verdict: **APPROVE**.