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

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

Browse Source 
APPROVE — final v1.1.x release lands clean. Full scope: queue::*
+ queue:receive trigger + dispatcher queue arm + visibility-timeout
reclaim + invoke()/invoke_async + retry::* (with → run per Rhai
reserved keyword). All five F1-F5 follow-ups from the v1.1.8 retro
implemented; seven deviations transparently flagged in HANDBACK §7
with sound rationale.

§8 attestation discipline visibly leveled up from v1.1.8:
schema-snapshot re-blessed in-branch, literal fmt/clippy output,
F4 grep verification, four new DB-gated integration binaries
(queue_e2e 4, invoke_e2e 4, retry_e2e 3, migration_queue_messages 4)
matching the brief's non-negotiable test-density minimums.

Reviewer-independent verification reproduced all material claims:
fmt green, clippy green (incremental), F4 grep returns exactly
one hit, schema replay matches the committed golden, all 15 new
DB-gated tests pass against a fresh dev Postgres. Aggregate
lighter-slice attestation: 666 passed / 0 failed across the
load-bearing crates — matches agent's claimed ~660.
This commit is contained in:
MechaCat02
2026-06-07 11:21:12 +02:00
parent 7d3ced0776
commit 450badaabf
1 changed files with 117 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files

248
REVIEW.md
View File

@@ -1,199 +1,185 @@
# v1.1.8 Audit & Review
# v1.1.9 Audit & Review
**Branch:** `feat/v1.1.8-user-management`
**Base:** `main` (v1.1.7 head, `5cbb6ca`)
**Commits ahead:** 18 (16 feature + 1 chore-clippy + 1 docs-handback) + 1 reviewer-supplied chore-fmt + 1 reviewer-supplied schema-bless
**HEAD audited:** `2a76ea1` (agent's last commit, before reviewer fmt + bless)
**Branch:** `feat/v1.1.9-queues-invoke`
**Base:** `main` (v1.1.8 head, `b9e002a`)
**Commits ahead:** 20
**HEAD audited:** `7d3ced0` (agent's last commit)
**Audited by:** reviewer (this report)
**Iterations:** 1
## Verdict
**APPROVE — ready to merge to `main` as v1.1.8** *after* the two reviewer commits below are applied.
**APPROVE — ready to merge to `main` as v1.1.9.**
Substantial release: full `users::*` SDK + sessions + email verification + password reset + invitations + per-app roles + admin HTTP + dashboard Users tab + Invitations sub-tab, plus all three F-followups (drop plaintext realtime signing key, clippy `--all-targets` discipline, realtime `auth_mode = 'session'`). The deferrable piece (invitations) shipped in full. Code is structurally sound: cross-app isolation uniformly enforced, timing-flat login is real, atomic one-shot consume on every token table, SHA-256 token storage, Argon2id passwords, principal:None synthesis for internal email hops, defense-in-depth `user.app_id == app_id` recheck in the F3 session branch.
The final v1.1.x release. Full scope landed: `queue::*` SDK + `queue:receive` trigger + dispatcher queue arm + visibility-timeout reclaim, `invoke()` (sync, same-app) + `invoke_async()` over the universal outbox, `retry::*` (renamed `with → run` per D1), and all five F1–F5 follow-ups from the v1.1.8 retro. No deferrable piece slipped. Seven deviations from the brief, all transparently flagged in HANDBACK §7 with sound rationale.
Three §8 attestation gaps flagged honestly by the agent — handled by the reviewer rather than bounced back:
**§8 attestation discipline visibly leveled up from v1.1.8.** The agent re-blessed the schema snapshot in-branch (F2), produced literal fmt + clippy output (F3), deduped `TIMING_FLAT_DUMMY_HASH` with a grep verification line (F4), and shipped four DB-gated integration test binaries against the explicit names + minimums in the brief (F1). The host-freeze constraint on cold-cache clippy was acknowledged and CI is correctly designated the authoritative gate (F5).
1. **`cargo fmt --all -- --check` was NOT green** at agent HEAD — false attestation in HANDBACK §8. Reviewer applied `cargo fmt --all` as a separate chore commit (purely cosmetic; 8 files, line-wrapping only).
2. **Schema snapshot golden not re-blessed** by agent (DB-host work). Reviewer re-blessed; diff is exactly the 6 new tables, 1 dropped column (`realtime_signing_key`), 1 widened CHECK (`session`). Zero unrelated drift.
3. **Full `cargo test --workspace` not run** by agent (their host froze; flagged in §8). Reviewer ran a lighter targeted slice: **401 tests passed (manager-core lib 294 + shared lib 32 + orchestrator-core lib 74 + schema_snapshot 1), 0 failed.** The cold-cache `cargo clean && cargo clippy --workspace --all-targets` attestation also caused this reviewer's host to freeze; falling back to the agent's own §6 F2 attestation (which reported green from a hot incremental cache after their fixes) plus my own post-fmt clippy run (9 Checking lines, 0 warnings).
**Reviewer-independent verification reproduced everything material:**
Two further gaps flagged in HANDBACK §7 #7 + §12: **zero new integration test binaries** (brief asked for ~5–10), and **no live SMTP smoke** of the email-tied flows. Both are real coverage gaps but not blocking — the inline unit tests cover the load-bearing primitives (realtime auth session branch, authz mapping, app_secrets_repo encrypted-only path), the service-layer code is straightforward, and v1.1.5 / v1.1.6 / v1.1.7 reviews have set the precedent that the reviewer accepts coverage gaps when the code reads as sound. Folded into "next-release follow-ups" below.
- fmt: `cargo fmt --all -- --check` → no output, exit 0.
- clippy: `cargo clippy --workspace --all-targets --all-features -- -D warnings` → green (incremental cache; agent already ran the cold-cache attempt and fixed L1 lints).
- F4 grep: exactly one hit at [auth.rs:36](crates/manager-core/src/auth.rs#L36), the const declaration.
- F2: `cargo test -p picloud-manager-core --test schema_snapshot -- --include-ignored` → 1 passed (replay matches the committed golden).
- Four new DB-gated binaries against a fresh dev Postgres on `localhost:15432`:
- `migration_queue_messages` → 4 passed in 0.87s
- `retry_e2e` → 3 passed in 2.88s
- `invoke_e2e` → 4 passed in 3.75s
- `queue_e2e` → 4 passed in 37.20s
Aggregate lighter-slice attestation by the reviewer: **~666 passed / 0 failed** across manager-core lib (306) + shared lib (34) + orchestrator-core lib (74) + executor-core lib (18) + executor-core integration binaries (218 across 17 binaries) + the four new v1.1.9 DB-gated binaries (15) + schema_snapshot (1). Matches the agent's claim of ~660 essentially exactly. Picloud-crate prior dispatcher_e2e / api / authz / email_inbound binaries (29 tests per the agent's §8) not re-run; reviewer accepts those at agent's attestation.
---
## 1. Static checks reproduced
## 1. Static checks reproduced (HEAD `7d3ced0`)
```
cargo fmt --all -- --check ❌ at agent HEAD (false §8 attestation)
✅ after reviewer-supplied `cargo fmt --all`
cargo fmt --all -- --check ✅ no output (exit 0)
cargo clippy --workspace --all-targets --all-features -- -D warnings
✅ post-fmt incremental: 9 Checking lines, 0 warnings, 0 errors
(cold-cache attempt froze host; agent's own §6 F2 attestation accepted)
cargo test -p picloud-manager-core --lib ✅ 294 passed / 0 failed
cargo test -p picloud-shared --lib ✅ 32 passed / 0 failed
✅ Finished `dev` profile in 1.77s (warm-cache, agent's cold-cache run was already green)
cargo test -p picloud-manager-core --lib ✅ 306 passed / 0 failed
cargo test -p picloud-shared --lib ✅ 34 passed / 0 failed
cargo test -p picloud-orchestrator-core --lib ✅ 74 passed / 0 failed
cargo test -p picloud-manager-core --test schema_snapshot ✅ 1 passed (BLESS=1 wrote the new golden)
cargo test -p picloud-executor-core --tests ✅ 218 passed / 0 failed (17 binaries + lib)
cargo test -p picloud-manager-core --test schema_snapshot ✅ 1 passed (replay matches golden)
cargo test -p picloud-manager-core --test migration_queue_messages ✅ 4 passed
cargo test -p picloud --test queue_e2e ✅ 4 passed in 37.20s
cargo test -p picloud --test invoke_e2e ✅ 4 passed in 3.75s
cargo test -p picloud --test retry_e2e ✅ 3 passed in 2.88s
```
Reviewer-substituted attestation total: **401 passed / 0 failed** across the load-bearing slices.
Reviewer-substituted attestation total: **666 passed / 0 failed** across the load-bearing slices.
The full-workspace awk-sourced count attestation is missing from this release — the host-freeze risk is the same on agent and reviewer hardware. Recommend running it in CI once the merge lands; if any picloud-crate e2e binary regresses it'll surface there and can be addressed in a v1.1.8.1.
The full-workspace `--test-threads=2` run with the awk-sourced count is not produced — host-freeze risk remains the same on this hardware (v1.1.7/v1.1.8 lesson). CI is the authoritative gate for that; if any picloud-crate binary regresses there it's a v1.1.9.1 hotfix, not a re-roll.
## 2. Design conformance (spot-checks)
| Decision / requirement | Where it lives | Verdict |
|---|---|---|
| `app_users` distinct from `admin_users`, identity tuple `(app_id, id)`, uniqueness on `(app_id, lower(email))` | [0026_app_users.sql](crates/manager-core/migrations/0026_app_users.sql) | ✅ |
| Argon2id password hashing (reuse existing dep, no bcrypt/PBKDF2) | [auth.rs:41-45](crates/manager-core/src/auth.rs#L41-L45) — `hash_password` via `Argon2::default()` | ✅ |
| Cross-app isolation: every method derives `app_id` from `cx.app_id`; `verify_session_for_realtime` takes `app_id` explicitly (from the topic row, not script) | [users_service.rs:165-175,569-575,1086-1099](crates/manager-core/src/users_service.rs) | ✅ Uniformly enforced; `logout` also rejects cross-app token presentation silently |
| SHA-256-hashed session tokens; raw appears only in the login/accept response | [auth.rs:81-94](crates/manager-core/src/auth.rs#L81-L94) + [0027_app_user_sessions.sql:23](crates/manager-core/migrations/0027_app_user_sessions.sql#L23) | ✅ `token_hash TEXT PRIMARY KEY` |
| Sliding TTL via `last_used_at` + absolute cap (`PICLOUD_APP_USER_SESSION_TTL_HOURS`=24, `PICLOUD_APP_USER_SESSION_ABSOLUTE_HOURS`=720) | [users_service.rs:71-72,1102-1106](crates/manager-core/src/users_service.rs) | ✅ `expires_at = min(now + session_ttl, absolute_expires_at)` |
| Revocation explicit (logout, admin revoke-sessions, password reset) | [users_service.rs:560-566,718-720](crates/manager-core/src/users_service.rs) + `revoke_for_user` cascade in `complete_password_reset` | ✅ |
| Weekly GC sweeps for the four new token tables | [gc.rs](crates/manager-core/src/gc.rs) (new module) + wired in [picloud/src/lib.rs](crates/picloud/src/lib.rs) | ✅ Per HANDBACK §1 |
| **Timing-flat login** — bad-email runs `verify_password(TIMING_FLAT_DUMMY_HASH, password)`; same for unparseable-email-shape path; same `()` return value | [users_service.rs:486-512](crates/manager-core/src/users_service.rs#L486-L512) | ✅ Both branches go through the dummy verify; same wall-clock; same return |
| `TIMING_FLAT_DUMMY_HASH` is a real Argon2id PHC string | [auth.rs:35-36](crates/manager-core/src/auth.rs#L35-L36) | ✅ Constant exported; duplication of the inline admin-auth value is documented in HANDBACK §7 #2 |
| Atomic one-shot token consume (verification, password reset, invitations) via `UPDATE WHERE consumed_at IS NULL` (or `accepted_at IS NULL`) | App user verification/password-reset/invitation repos | ✅ Per HANDBACK §2; rowcount-driven; no race |
| `request_password_reset` no existence leak — silent Ok(()) on invalid shape and on missing user; swallows non-NotConfigured email errors | [users_service.rs:638-695](crates/manager-core/src/users_service.rs#L638-L695) | ✅ Only observable is `EmailNotConfigured`, which is the operator's known state |
| `accept_invite` returns `()` if user already exists (sign-up beat acceptance); invitation is consumed | [users_service.rs:818-826](crates/manager-core/src/users_service.rs#L818-L826) | ✅ Documented |
| `complete_password_reset` revokes all sessions for the user | [users_service.rs:718-720](crates/manager-core/src/users_service.rs#L718-L720) | ✅ Per brief |
| Roles: string-tagged only; no registry/hierarchy/permission matrix | [0031_app_user_roles.sql](crates/manager-core/migrations/0031_app_user_roles.sql) + service `add_role`/`remove_role`/`has_role` | ✅ Per brief — permission matrices explicitly v1.2 |
| Three new `Capability` variants (`AppUsersRead/Write/Admin`); seven-scope commitment held (Read → `script:read`, Write/Admin → `script:write`) | [authz.rs:759 + scope-map block](crates/manager-core/src/authz.rs) | ✅ No new `Scope` variant added |
| Admin HTTP surface: 10 endpoints under `/api/v1/admin/apps/{id}/{users,invitations}` | [users_admin_api.rs](crates/manager-core/src/users_admin_api.rs) | ✅ All present; never returns password hash / session token / unrotated reset token |
| Dashboard Users tab + Invitations sub-tab + `session` radio in Topics | [dashboard/src/routes/apps/[slug]/users/](dashboard/src/routes/apps/[slug]/users/+page.svelte) + Topics page diff | ✅ Per HANDBACK §1 |
| **F1 — drop plaintext `realtime_signing_key`** with guard refusing if any row still has plaintext-without-encrypted | [0032_drop_plaintext_realtime_signing_key.sql](crates/manager-core/migrations/0032_drop_plaintext_realtime_signing_key.sql) | ✅ DO-block guard + `DROP COLUMN IF EXISTS`; v1.1.7 `migrate_plaintext_keys` startup task + callsite deleted; read path uses encrypted-only |
| **F2 — clippy `--all-targets` discipline** | HANDBACK §6 F2 + commit `7610a16` | ✅ Agent ran cold-cache clippy themselves; deviation from `cargo clean` step documented; reviewer's own incremental clippy is green |
| **F3 — `auth_mode = 'session'`** | [0033_topics_auth_mode_session.sql](crates/manager-core/migrations/0033_topics_auth_mode_session.sql) + [realtime_authority.rs:113-134](crates/manager-core/src/realtime_authority.rs#L113-L134) | ✅ CHECK widened; `Session` arm calls `users.verify_session_for_realtime`; defense-in-depth `user.app_id == app_id` recheck; 4 new unit tests cover valid / missing / wrong / cross-app token |
| `RealtimeAuthorityImpl::new` signature change to accept `Arc<dyn UsersService>` | constructor + `picloud/src/lib.rs` construction-order reshuffle (users built before realtime_authority) | ✅ |
| Versions: workspace 1.1.7→1.1.8, SDK 1.8→1.9, dashboard 0.13.0→0.14.0; `@picloud/client` unchanged | Cargo.toml + version.rs + package.json | ✅ All correct |
| CHANGELOG v1.1.7-must-be-applied-first note | [CHANGELOG.md](CHANGELOG.md) | ✅ Per brief (commit `31402eb`) |
| Migrations 0026 → 0033 sequential, no skips | migrations/ | ✅ |
| Queue table IS the outbox for queue semantics (no double-buffering) | [queue_repo.rs::claim](crates/manager-core/src/queue_repo.rs#L177-L212) + [dispatcher.rs::tick_queue_arm](crates/manager-core/src/dispatcher.rs#L152-L164) | ✅ No `OutboxSourceKind::Queue`; the dispatcher's `tick_queue_arm` enumerates consumers and runs one atomic claim per `(app_id, queue_name)` per tick |
| Single round-trip atomic claim via `UPDATE ... WHERE id = (SELECT ... FOR UPDATE SKIP LOCKED LIMIT 1) RETURNING` | [queue_repo.rs:184-201](crates/manager-core/src/queue_repo.rs#L184-L201) | ✅ Verbatim per the brief; increments `attempt`, sets `claim_token` + `claimed_at` |
| Auto-ack: `DELETE WHERE id = $1 AND claim_token = $2` (lease guarantee) | [queue_repo.rs:218-224](crates/manager-core/src/queue_repo.rs#L218-L224) | ✅ Stale dispatcher whose lease expired can't delete a re-claimed message |
| Auto-nack: clear claim + set `deliver_after = NOW() + retry_delay` | [queue_repo.rs:232-245](crates/manager-core/src/queue_repo.rs#L232-L245) | ✅ Same lease-token filter; idempotent |
| Visibility-timeout reclaim every `PICLOUD_QUEUE_RECLAIM_INTERVAL_MS` (default 30s) | [dispatcher.rs::spawn lines 87-104](crates/manager-core/src/dispatcher.rs#L87-L104) + [queue_repo.rs::reclaim_visibility_timeouts](crates/manager-core/src/queue_repo.rs#L247-L262) | ✅ Separate tokio task; UPDATE JOIN triggers + queue_trigger_details; verified by `queue_e2e::queue_visibility_timeout_reclaim` |
| Dead-letter integration through existing `dead_letters` table + `fan_out_dead_letter` path | [dispatcher.rs::handle_queue_failure](crates/manager-core/src/dispatcher.rs#L292-L338) | ✅ Source `"queue"`, op `"receive"`; existing `fan_out_dead_letter` on the outbox arm reads the new row without special-casing |
| Exactly one consumer per queue enforced | [trigger_repo.rs::create_queue_trigger](crates/manager-core/src/trigger_repo.rs) via `pg_advisory_xact_lock` | ✅ Per D5 — partial unique cross-table index is infeasible; advisory-lock + SELECT-then-INSERT in one transaction is the right workaround |
| Trigger-execution principal = principal that registered the trigger | dispatcher.rs::dispatch_one_queue lines 231-235 (resolves via `principals.resolve(consumer.registered_by_principal)`) | ✅ Consistent with v1.1.1 design-notes §4 |
| Cross-app `invoke()` rejection at three layers | [invoke_service.rs::resolve_id](crates/manager-core/src/invoke_service.rs#L43-L64) (service); `get_by_name`/RouteTable snapshot (repo/routes); dispatcher `build_invoke_request` (defense-in-depth) | ✅ Verified by `invoke_e2e::invoke_cross_app_rejects` (returns `InvokeError::CrossApp`) |
| Same-app `invoke()` re-entrancy via shared engine + fresh `SdkCallCx` (trigger_depth + 1) | [engine.rs:91-101 + 197](crates/executor-core/src/engine.rs#L91-L197) + sdk/invoke.rs::invoke_blocking | ✅ `Engine::self_weak: OnceLock<Weak<Engine>>` + `set_self_weak` + `self_arc()`; cycle stays broken; bridge gets `None` only on engine drop |
| `Limits::trigger_depth_max` synced from `TriggerConfig::max_trigger_depth` (D4) | sandbox.rs Limits + picloud/src/lib.rs build_app | ✅ `PICLOUD_MAX_TRIGGER_DEPTH` governs both dispatcher fan-out and invoke depth |
| `invoke_async` runs once (no retry) | invoke_service.rs::enqueue_async + dispatcher.rs::build_invoke_request synthesizes `retry_max_attempts = 1` | ✅ Per D7 — callers wrap in `retry::run` if they want retries |
| `retry::policy` clamps: max_attempts ∈ [1,20], base_ms ∈ [1, 60_000], jitter_pct ∈ [0,100] | [retry.rs::build_policy](crates/executor-core/src/sdk/retry.rs#L135-L187) | ✅ Saturating clamps with clear errors on bogus backoff strings |
| `retry::run(policy, fn_ptr)` re-invokes FnPtr through NativeCallContext | [retry.rs::retry_run](crates/executor-core/src/sdk/retry.rs#L189-L200+) | ✅ Closure shares caller's engine + cx; inner `invoke()` calls get fresh cx via the invoke bridge — retry doesn't interfere |
| `retry::on_codes` substring filter (empty = retry every throw) | retry.rs::on_codes | ✅ |
| Sleep mechanics: `tokio::time::sleep` via `TokioHandle::block_on` inside `spawn_blocking` | retry.rs | ✅ Safe — already off the async worker pool |
| Migrations 0034 + 0035 sequential, no skips | migrations/ | ✅ |
| Schema-snapshot golden re-blessed in branch (F2) | [tests/expected_schema.txt](crates/manager-core/tests/expected_schema.txt) + commit `106394b` | ✅ Replay matches |
| Versions: workspace 1.1.8→1.1.9, SDK 1.9→1.10, dashboard 0.14.0→0.15.0 | Cargo.toml + version.rs + package.json | ✅ |
| CHANGELOG v1.1.9 entry, no upgrade constraint (pure additive) | [CHANGELOG.md](CHANGELOG.md) | ✅ |
| F4 — `TIMING_FLAT_DUMMY_HASH` dedup: grep returns exactly 1 hit | reviewer-verified: `grep -rn 'argon2id\\$v=19\\$m=19456,t=2,p=1\\$dGltaW5n' crates/` → 1 hit at auth.rs:36 | ✅ |
| Dashboard: Queues tab (list + drilldown), queue:receive trigger form, 0.15.0 | [dashboard/src/routes/apps/[slug]/queues/](dashboard/src/routes/apps/[slug]/queues/+page.svelte) + extended Triggers form | ✅ Per HANDBACK §5; pre-existing Playwright errors in tests/e2e unchanged |
## 3. The flagged items (HANDBACK §7)
## 3. The seven flagged deviations (HANDBACK §7)
### 3.1 `EmailTemplateOpts.from` required (§7 #1)
### D1. `retry::with` → `retry::run` (Rhai reserved keyword)
The brief's example sketched only `{ link_base, subject, body_template }`. The underlying `EmailService::send` needs `from`. Agent chose to require it in the template rather than default via a new env var.
`with` AND `call` are reserved at the Rhai parser; the agent verified this before committing the rename. Documented in the SDK module docstring, the SDK_VERSION 1.10 changelog, and the dashboard form copy.
**Verdict: correct call.** Requiring `from` keeps audit-log attribution unambiguous and avoids smuggling an operator-config surface in for marginal ergonomics. Plain-text mismatch with brief example is the kind of "walk through every code example before sending" lesson v1.1.7 retro flagged — own that on the prompting side for v1.1.9.
**Verdict: accept.** `run` reads cleanly script-side (`retry::run(policy, || ...)`). The brief example using `with` was sketched without parser knowledge; agent's own-the-fix is exactly the discipline lesson from v1.1.7's retro ("flag, don't reinterpret" applied correctly — this is a forced rename, not silent reinterpretation).
### 3.2 `TIMING_FLAT_DUMMY_HASH` duplicated (§7 #2)
### D2. Trait move skipped (was plan §5)
Agent factored the constant into shared `manager-core::auth::TIMING_FLAT_DUMMY_HASH` but did NOT refactor the inline value in `auth_api.rs::login` (admin auth path).
The brief speculated about moving `ExecutorClient` + `ScriptIdentity` from `orchestrator-core` to `shared` to enable an in-engine invoke bridge. Agent reconsidered during commit 7: the bridge lives in `executor-core` where `Engine` is in scope; `Engine::execute(&source, req)` is sync and returns `ExecResponse` directly. Per-call AST cache reuse is what's lost (invokes recompile each call — ms-scale; deferred to v1.2).
**Verdict: accept; fold dedup into v1.1.9.** The two strings are byte-identical; correctness is preserved. The instinct to keep the admin-auth code untouched in a v1.1.8 release is right.
**Verdict: accept.** The `self_weak` mechanism (Weak-Arc-OnceLock) is cleaner than a trait move and avoids the cascading signature churn in `LocalExecutorClient`. AST recompile cost is a known tradeoff. The brief was speculation; the agent built the simpler shape that works.
### 3.3 No `realtime_signing_key_nonce_LEGACY` column to drop (§7 #3)
### D3. Retry columns on parent triggers row only (was plan §1)
The brief speculated about a legacy nonce column; recon confirmed only `realtime_signing_key` (plaintext) and the encrypted+nonce pair exist. Migration 0032 drops only `realtime_signing_key`. Schema snapshot reflects this.
The brief sketch said `queue_trigger_details` "gets the same five retry override columns as the parent". The parent already has them. Duplicating would split the source of truth.
**Verdict: accept.** Brief was over-cautious. Agent verified against actual schema. Discipline working.
**Verdict: accept; the brief was wrong here.** Single source of truth on the parent is correct. The agent surfaced this per discipline reminder #2.
### 3.4 DELETE `/users/{user_id}` gated `AppUsersWrite`, not `AppUsersAdmin` (§7 #4)
### D4. `Limits::trigger_depth_max` (was plan §5)
Agent over-thought a brief-internal split that doesn't exist. The dispatch prompt did NOT say DELETE HTTP needs Admin-tier authority distinct from the SDK's Write-tier `delete`; it specified the SDK gating (`delete` → Write) and admin endpoints "all gated by `AppUsersRead/Write/Admin`" generically. The agent's question is moot because Read/Write/Admin all map to existing scopes (Read → `script:read`, Write/Admin → `script:write`) per the seven-scope commitment, so the effective gate of DELETE is `script:write`-or-higher anyway.
Added `Limits::trigger_depth_max: u32` (default 8) and synced from `TriggerConfig::from_env().max_trigger_depth` in the picloud binary. `PICLOUD_MAX_TRIGGER_DEPTH` governs both dispatcher fan-out and invoke depth-bound.
**Verdict: accept as implemented.** No HTTP-layer change needed for v1.1.8. If a per-role hierarchy lands in v1.2 (the permission-matrix work), revisit then.
**Verdict: accept.** Right level of abstraction — depth check inside `Engine` doesn't reach into `TriggerConfig`.
### 3.5 `cargo clean` skipped on F2 attestation (§6 F2 + §7 #5)
### D5. One-consumer-per-queue via advisory lock
Agent skipped the `cargo clean` step because the user (you) had asked for lighter subsequent builds after a previous `cargo test --workspace` froze the host. Agent confirms incremental output included `Checking <crate> ... (test)` lines — visually verified.
Partial unique index across `triggers.app_id` and `queue_trigger_details.queue_name` is impossible (partial indexes can't reference foreign columns). The agent uses `pg_advisory_xact_lock(hashtext(app_id || queue_name))` + SELECT-then-INSERT in one transaction.
**Verdict: accept.** This reviewer's own attempt to do the proper cold-cache attestation also froze the host. Cold-cache discipline on this hardware is environmentally infeasible; need to lean on CI for that signal going forward. The clippy attestation is otherwise solid (agent green, reviewer green).
**Verdict: accept.** The xact-scoped advisory lock is auto-released on commit/rollback. Alternative (denormalize `app_id` onto detail row) would duplicate state — the agent picked the right tradeoff. Verified by `queue_e2e::queue_one_consumer_per_queue_rejected`.
### 3.6 Schema snapshot not re-blessed (§7 #6)
### D6. `invoke()` exposed globally (not `invoke::*`)
Reviewer re-blessed in this audit. Diff verified to be exactly the 6 new tables + 1 dropped column + 1 widened CHECK. Folded into the merge.
The brief itself spelled `invoke(target, args)` not `invoke::call(target, args)`. Registered via `engine.register_global_module(...)`.
### 3.7 No new integration test binaries (§7 #7)
**Verdict: accept; the brief was self-consistent here.** Scripts write `invoke("worker", #{...})` which matches the brief example.
Brief asked for ~5–10 new binaries (`app_user_repo`, `app_user_session_repo`, `users_service`, `users_email_flows`, `users_admin_api`, `users_realtime_session`, `migration_0032_drop_plaintext`, `users_sdk`, `users_cross_app_isolation`). Agent shipped 0 (only 6 new inline unit tests in existing crates).
### D7. `invoke_async` runs once (synthetic `retry_max_attempts = 1`)
**Verdict: accept as a known coverage gap; do NOT block on it.** Reasoning:
- The lib-tier code (294 manager-core, 74 orchestrator-core, 32 shared, 1 schema_snapshot) passes cleanly.
- The new realtime session branch has 4 dedicated unit tests including cross-app token rejection — the load-bearing F3 surface.
- The cross-app isolation discipline is auditable at every method (see §2 above); the v1.1.3 lesson is genuinely applied, not just performatively flagged.
- The picloud-crate e2e binaries (dispatcher_e2e, email_inbound from prior releases) still pass — the platform plumbing is exercised.
- Adding 5–10 new DB-gated test binaries is the kind of work that can land in a v1.1.8.1 or be folded into v1.1.9's test-density target. Better as a CI-side investment.
Per the brief: "**NO — `invoke_async` runs once.** Callers who want retries wrap in `retry::with` instead." Implemented as `retry_max_attempts = 1` on the synthetic ResolvedTrigger inside `build_invoke_request`, short-circuiting the existing retry loop.
**v1.1.9 follow-up**: spec the integration tests in the v1.1.9 dispatch prompt as a hard expectation (not "match v1.1.5–v1.1.7 density"), and call out `users_cross_app_isolation` + `migration_0032_drop_plaintext` as non-negotiable. The v1.1.3 cross-app trigger gap precedent is the right model.
### 3.8 `admin_create_invitation` is a separate trait method (§7 #8)
Agent added `admin_create_invitation` because `invite` takes `&SdkCallCx` (script's) and the admin layer doesn't have one. Synthesizing a cx with the admin principal was an option but the dedicated method is cleaner.
**Verdict: accept.** Pragmatic separation. The internal authz check (`AppUsersAdmin`) fires identically in both paths.
### 3.9 Internal email sends synthesize cx with `principal: None` (§7 #9)
For `send_verification_email`, `request_password_reset`, and SDK-side `invite`, the internal email hop runs with `principal: None` because the `users::*` gate has already fired. `admin_create_invitation` is different — uses the admin's real principal because that path is the admin's first-line invocation of email.
**Verdict: accept.** Correct distinction. The script's `users::*` call satisfies the users-gate; the email-service's `AppEmailSend` gate is a separate concern and the agent's reasoning ("script-as-gate semantics already applied; internal hop is system-level") is consistent with how downstream stateful services interact.
### 3.10 `Services::new` signature change broke 10 test callers (§7 #10)
The new `users` arg is positional. 10 executor-core test files needed updating; agent updated them. Not backward-compatible at the type level.
**Verdict: accept.** Workspace-internal types; honest break is better than a smuggled default. A `Services::builder()` (§9 #3) is the kind of cosmetic refactor that should wait until the bundle stabilizes.
**Verdict: accept.** Surfaced misfires through the dead-letter path; callers retain explicit retry control via `retry::run`.
## 4. Substantive strengths
**1. Cross-app isolation discipline.** Every `users::*` method derives `app_id` from `cx.app_id` (read at line 165 onward in `users_service.rs`). `verify_session_for_realtime` takes `app_id` from the topic row (not script). `logout` *does the cross-app check at session-lookup time*, returning silently if the token belongs to a different app — this is the right shape (script can't probe cross-app sessions). The repo's API (`get(app_id, id)`, `find_by_email(app_id, email)`, etc.) has no "get-any-user" affordance, mirroring v1.1.3's lesson.
**1. The `Engine::self_weak` re-entrancy mechanism is exemplary.** `Weak<Engine>` in a `OnceLock<...>`, set by the picloud binary right after `Arc::new(Engine::new(...))`, upgraded on demand by `self_arc()`. The bridge gets `None` only on engine drop (shutdown). No reference cycle; no panic surface; no per-call overhead beyond an atomic load. Test-only callers that don't wire it get a clear error message from the SDK bridge. This is the right shape for the cluster-mode evolution path too — the `Weak` is process-local; a cluster-mode `invoke()` would swap the bridge for an `ExecutorClient` round-trip behind a feature flag.
**2. Timing-flat login is real.** Both bad-email-shape AND bad-email-no-match paths run `verify_password(TIMING_FLAT_DUMMY_HASH, password)` and discard the result; the dummy hash is a real Argon2id PHC string. Same wall-clock cost; same `()` return. The unconditional `verify_password` even on `validate_email`-failure is the detail that makes this work end-to-end.
**2. Cross-app `invoke()` isolation has three layers.** Repo (`get_by_name` takes explicit `app_id`), service (`resolve_id` checks `script.app_id != cx.app_id`), dispatcher (`build_invoke_request` re-checks when firing an `invoke_async` outbox row). A hand-edited outbox row that points to a cross-app script is still rejected. The v1.1.3 cross-app trigger gap lesson genuinely applied; the `invoke_cross_app_rejects` E2E test pins this in place.
**3. F1 migration is properly guarded.** The DO-block `RAISE EXCEPTION` clause refuses to drop the plaintext column if any row still has `realtime_signing_key IS NOT NULL AND realtime_signing_key_encrypted IS NULL`. This forces operators who skipped v1.1.7 to apply it first (and let its startup encryption sweep complete) — making the upgrade path robust to operator-error. The v1.1.7 `migrate_plaintext_keys` startup task and its callsite are correctly deleted; the read path consults only encrypted columns.
**3. Queue claim + ack + nack are correctly lease-token-keyed.** Both `ack` and `nack` filter `WHERE id = $1 AND claim_token = $2`. A stale dispatcher whose visibility-timeout expired and lost its lease cannot accidentally delete or re-defer a re-claimed message. The reclaim task's UPDATE...FROM JOIN against the live `(triggers, queue_trigger_details)` set ensures only enabled queue consumers' messages are reclaimed. Race-free in the strong sense.
**4. F3 implementation + tests are tight.** The session branch in `authorize_subscribe` delegates to `users.verify_session_for_realtime(app_id, token)`, which does the full `resolve_session` flow (cross-app check + sliding TTL bump). The defense-in-depth `user.app_id != app_id` check in the realtime authority is correct (cheap second line of defense). The 4 new unit tests cover valid/missing/wrong/cross-app token — including a properly-set-up `FakeUsersForRealtime` fixture that doesn't pollute the test against other realtime tests.
**4. The dispatcher gate-saturation handling for the queue arm.** When the gate is full, the queue arm immediately nacks-with-100ms-delay, keeping the lease accounting consistent and letting the next tick re-claim cleanly. This mirrors the outbox arm's reschedule semantics exactly — symmetric load-shed across both event-firing paths.
**5. One-shot tokens are atomic.** Verification, password reset, invitations all consume via `UPDATE WHERE consumed_at IS NULL` (or `accepted_at IS NULL` for invitations). Rowcount=1 means valid-and-now-used; rowcount=0 means missing/already-consumed/expired/wrong-app. No TOCTOU window. Mirrors the v1.1.5/v1.1.6 pattern.
**5. `retry::policy` clamping is saturating + transparent.** `max_attempts ∈ [1, 20]`, `base_ms ∈ [1, 60_000]`, `jitter_pct ∈ [0, 100]`. Bogus backoff strings surface a clear error pointing at the three valid values. Deterministic jitter via `DefaultHasher` over `(span, raw)` avoids pulling `rand` into the hot path — correctness-equivalent for retry purposes.
**6. `request_password_reset` has zero existence leak.** Silent Ok(()) on invalid email shape, on missing user, AND on non-NotConfigured email errors (logged server-side, surfaced as Ok). The only thing the script can observe is `EmailNotConfigured`, which is the operator's known state, not a per-user signal. The agent's reasoning in HANDBACK §4 is correct.
**6. The 20-commit split is exemplary, again.** Migration → types → repo → service → SDK → dispatcher arm → next service (invoke) → next dispatcher arm → next SDK (retry) → admin HTTP → dashboard → tests → F4 dedup → version bumps → schema bless → test fixes → clippy clean → docs. Each commit independently green. Best commit hygiene of any v1.1.x release alongside v1.1.7 + v1.1.8.
**7. The 18-commit split is exemplary.** Authz → app_users repo → sessions repo → service trait+CRUD → SDK module → email-verification → password-reset → invitations → roles → admin-HTTP → dashboard → F1 → F3 → GC sweep → version-bumps → clippy-cleanup → CHANGELOG → handback. Each commit independently green. Best commit hygiene of any v1.1.x release.
**7. Integration test density target met in full (F1).** Four new DB-gated binaries (15 tests), exactly the names the brief specified as non-negotiable. The `queue_visibility_timeout_reclaim` test actually exercises the reclaim by mutating `claimed_at` to simulate a crashed consumer — that's the load-bearing semantic, not a smoke test.
**8. Encrypted-only read path is correct.** `decode_signing_key` now returns `Ok(None)` if either encrypted column is NULL — no more fallback. The 3 remaining `app_secrets_repo` unit tests cover the encrypted-happy-path, missing-columns-None, and wrong-master-key-Crypto-error. Net -2 tests vs v1.1.7 is correct (plaintext-fallback test went away with the column).
## 5. Open questions answered
## 5. Smaller observations
HANDBACK §9 raises four:
- **`AppUser.roles` N+1 query** (§9 #4): one extra `SELECT` per user in `list` / `get`. Acceptable at v1.1.x scale. v1.2 can batch via a JOIN if it bites.
- **`UsersServiceImpl::new` is 10 args** (§10 #3): same `#[allow(clippy::too_many_arguments)]` pattern as `EmailServiceImpl`. Builder pattern is cosmetic; defer.
- **Realtime signing-key cache has no eviction** (§10 #2): bounded by app count. Flagged for the v1.2 rotation work. Not v1.1.8's problem.
- **`@picloud/client` not touched**: per brief. The v1.1.6 `auth.login`/`auth.logout` already wrap dev-defined endpoints; users-SDK-aware client helpers are a userland v1.2 concern.
1. **`retry::run` rename acceptable?** Yes. `run` is clear and unambiguous in this context. `attempt` would have been a contender but reads worse with closures. No change needed.
## 6. Versioning audit
2. **`invoke()` path-resolution method.** Defaulted to POST→GET fallback. Acceptable for v1.1.9; method-aware resolution is a v1.2 ergonomics item (per HANDBACK §11 deferred list).
3. **`ctx.trigger_depth` not in `ctx` map.** The depth value IS threaded through `SdkCallCx` correctly (verified by `invoke_depth_limit_exceeds_cleanly`). Surfacing it into Rhai's `ctx` map is a 5-line addition for v1.2.
4. **Cluster-mode reclaim coordination.** Out of scope per the brief; v1.3+ cluster work will need advisory-lock coordination for the reclaim task.
## 6. Smaller observations
- **L1 — pre-existing clippy lints surfaced during F3.** Six lints (two on new v1.1.9 code, four on new v1.1.9 SDK paths) — all fixed in `c3baa87 chore(v1.1.9): clippy clean`. Honest call; the lints were latent until the agent ran clippy properly per F3.
- **L2 — dashboard pre-existing Playwright errors (149)** in `tests/e2e/**/*.spec.ts`. Same as v1.1.8; not v1.1.9's concern.
- **`InvokeServiceImpl` constructor signature is 3 args** — well below the 10-arg threshold that triggers builder-pattern temptation. Clean.
- **Closures-across-`invoke()` rejected at the bridge** with a clear error per HANDBACK §12. Right call — captured environments don't translate across `SdkCallCx` boundaries.
## 7. Versioning audit
| File | Before | After | Status |
|---|---|---|---|
| Workspace `Cargo.toml` | 1.1.7 | 1.1.8 | ✅ |
| SDK schema (`shared/src/version.rs`) | 1.8 | 1.9 | ✅ Public surface added: `UsersService`, `users::*` SDK functions, `AppUser`, `EmailTemplateOpts`, `InviteOpts`, `TopicAuthMode::Session` |
| Dashboard `package.json` | 0.13.0 | 0.14.0 | ✅ |
| `@picloud/client` | 1.0.0 | 1.0.0 | ✅ (no client work this release) |
| Migrations | 0001..0025 | 0026..0033 added | ✅ Sequential, no skips |
| CHANGELOG.md | v1.1.7 entry | v1.1.8 entry + v1.1.7-must-be-applied-first note | ✅ Per brief |
## 7. Two reviewer-supplied commits
This audit produced two commits the reviewer applied directly to `feat/v1.1.8-user-management` to clean up the §8 attestation gaps without burning an iteration:
1. **`chore(v1.1.8): cargo fmt --all (reviewer)`** — purely cosmetic; 8 files, line-wrapping diffs from the F2 clippy-fix pass that landed un-fmt'd. No behavioral change.
2. **`chore(v1.1.8): re-bless schema snapshot (reviewer)`** — `tests/expected_schema.txt` reflecting the 6 new tables, the dropped plaintext column, and the widened `topics.auth_mode` CHECK. Verified diff has no unrelated drift.
Both are mechanical follow-ups to work the agent did; including them here lets us ff-merge cleanly.
| Workspace `Cargo.toml` | 1.1.8 | 1.1.9 | ✅ |
| SDK schema (`shared/src/version.rs`) | 1.9 | 1.10 | ✅ Public surface added: `QueueService`, `queue::*`, `invoke()`, `invoke_async()`, `retry::*`, `ctx.event.queue`, `TriggerEvent::Queue` |
| Dashboard `package.json` | 0.14.0 | 0.15.0 | ✅ |
| `@picloud/client` | 1.0.0 | 1.0.0 | ✅ (no client work this release, per brief) |
| Migrations | 0001..0033 | 0034..0035 added | ✅ Sequential |
| CHANGELOG.md | v1.1.8 entry | v1.1.9 entry (no upgrade constraint — pure additive) | ✅ |
## 8. Recommended next steps (post-merge)
1. **Merge** `feat/v1.1.8-user-management` into `main` (fast-forward; branch is linear ahead).
1. **Merge** `feat/v1.1.9-queues-invoke` into `main` (fast-forward; branch is linear ahead).
2. **`docker compose down` when convenient** to tear down the dev Postgres container.
3. **Pause** before dispatching v1.1.9 (Durable Queues & Function Composition).
4. **For the v1.1.9 dispatch prompt**, fold in:
- **Integration test density as a hard requirement, not a vibes target.** Explicit enumeration of binaries with non-negotiable minimums. Lesson from v1.1.8's 0-integration-binary outcome.
- **Schema-snapshot BLESS as part of the agent's gate.** Add it to the agent's `cargo clean` → fmt → clippy → test → BLESS sequence so the agent doesn't ship a release with a stale golden.
- **fmt attestation in §8 must be literal output, not "green" checkmark.** v1.1.8's HANDBACK §8 reported `cargo fmt --all -- --check` as green; it wasn't. Pattern: `cargo fmt --all -- --check 2>&1 | tail -3` (showing the literal "" or diff) goes into §8 verbatim.
- **`TIMING_FLAT_DUMMY_HASH` dedup from `auth_api.rs::login`** — a 5-line cleanup; fold into v1.1.9 housekeeping.
- **Permission matrix design for v1.2** — start the design-notes thread now so v1.1.9's queue/invoke work has it as context; the `users::has_role(role) -> bool` API is the foundation, but the policy layer needs explicit shape.
- **`cargo clean` step is environmentally infeasible on this dev hardware** (host freeze on both agent and reviewer machines). Move the cold-cache attestation requirement to CI exclusively; on dev, settle for incremental + visual verification of `Checking ... (test)` lines.
5. **Awareness from §1**: v1.1.8 ships without the full-workspace awk-sourced test count. CI's first run on `main` after merge will exercise the picloud-crate e2e binaries that the reviewer's targeted slice didn't reach. If anything regresses, it's a v1.1.8.1 hotfix, not a re-roll of v1.1.8.
3. **End of v1.1.x.** v1.2 (Workflows & Hierarchies — DAG execution, advanced docs query, interceptors, read triggers, audit log, script-mediated realtime auth, `dead_letters::list`, client-lib type codegen) is the next phase milestone.
4. **For v1.2 design intake**, fold in:
- **`ctx.trigger_depth` surface in the Rhai `ctx` map** (5-line addition per HANDBACK §9 #3).
- **`invoke()` method-aware route resolution** (POST→GET fallback today; explicit method arg per HANDBACK §9 #2).
- **AST cache reuse across `invoke()` calls** (D2 deferred optimization).
- **Permission matrix design for `users::*` roles** — v1.1.8 follow-up that's been waiting for the v1.2 design conversation.
- **Queue mutating admin endpoints** (purge, requeue, delete-message) — needs a payload-viewer UX story.
5. **For v2 design intake, archive `docs/v1.1.x-design-notes.md`** — every section's decisions have shipped (the doc's own lifecycle note says "Document deleted when v1.1.9 ships").
Branch is ready for merge after the two reviewer-supplied commits land. Verdict: **APPROVE**.
Branch is ready for merge. Verdict: **APPROVE**.