fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
b8a4f302197b817c16b25afe4ad6dcbc20c59cd6
PiCloud/AUDIT.md
MechaCat02 c1e4c3416b docs: comprehensive codebase audit (multi-agent) 
Multi-agent audit of main at v1.1.9 covering Security, Performance,
Code Quality, UI/UX, Migration/Schema, and TypeScript client.
115 actionable findings; severity-ranked and dedup'd.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-06-09 18:06:09 +02:00

84 KiB
Raw Blame History

PiCloud Codebase Audit — 2026-06-07

Scope: main at v1.1.9 head, commit 450bada. Methodology: Multi-agent audit — orchestrator + 6 parallel subagents sliced by (Security data-plane, Security auth/secrets, Performance, Code Quality, UI/UX, Migration+TS client). Reviewer-verified on the highest-severity findings (Q-001, T-001, U-006, P-001, P-004, S-002). Categories: Security (S), Performance (P), Code Quality (Q), UI/UX (U), Migration/Schema (M), TypeScript client (T).

Executive summary

  • One Critical: the manager-core / orchestrator-core boundary has inverted. manager-core/Cargo.toml depends on picloud-orchestrator-core and the dispatcher / route_admin / apps_api / repo modules reach into RouteTable, ExecutionGate, ExecutorClient, ScriptResolver, and InProcessBroadcaster for behavior, not just DTOs. CLAUDE.md's "Working Rules" call this exact pattern the bright line that keeps cluster mode a swap-not-rewrite. Today it's a swap-and-rewrite. See F-Q-001.
  • Load-bearing risk: every stateful Rhai SDK service silently accepts unbounded payloads. kv::set, docs::create/update, pubsub::publish_durable, queue::enqueue — none of them cap value size. Files + secrets + email have caps; the other four do not. An anonymous public-HTTP script can OOM Postgres JSONB columns or amplify one publish into N outbox rows × M MB each. See F-S-001.
  • Biggest performance leak: Argon2id on the async worker, on the hottest paths. attach_principal_if_present middleware runs an Argon2 verify per API-key candidate for every request carrying a Bearer header (including the data plane). Login and password reset do the same on async workers. Compounded by an unspawned-blocking call and no rate limit, the auth surface is one DoS vector for memory and three for CPU. See F-P-002, F-S-006.
  • Pool sized for 10, gate sized for 32. init_db opens max_connections = 10; the execution gate defaults to 32. Pool starvation under load is not a question of if. See F-P-003.
  • Most user-visible UX gap: the dashboard can't create half the trigger kinds. Backend exposes create for KV/docs/files/dead-letter triggers; the dashboard lists them but ships create forms only for cron/pubsub/email/queue. Operators must hit the API directly. See F-U-001. The queues drilldown also has a broken link that 404s — F-U-002.
  • A handful of dashboard pages render in light-theme fallback colors on a dark-theme app. Dead-letters, Files, App-Users, Invitations, Queues all reference var(--name, #fff) style fallbacks for CSS variables the dashboard never defines, so users see #666 text on #0f172a backgrounds. See F-U-004.
  • TypeScript client has a Rules-of-Hooks violation. useEndpoint().get() / .post() returns hook-calling functions; calling them conditionally or both in the same component produces undefined state. The test suite doesn't exercise it. See F-T-001.
  • The cleanest surface is the migration set. 35 sequential migrations with consistent FK-cascade discipline, partial indexes that match their predicates, and a working schema-snapshot test. Findings here are largely "redundant index" or "missing belt-and-suspenders CHECK" — refinement, not breakage.

Counts by severity

Severity Count
Critical 1
High 18
Medium 35
Low 39
Info 22

Findings

Security

Critical

(See Code Quality for the only Critical finding — the manager-core ↔ orchestrator-core boundary breach.)

High

F-S-001 — Unbounded payload sizes on kv::set, docs::*, pubsub::publish_durable, queue::enqueue
F-S-002 — users::request_password_reset and send_verification_email are unrate-limited and reachable from anonymous scripts
  • Severity: High
  • Location: crates/manager-core/src/users_service.rs:633, crates/manager-core/src/users_service.rs:575
  • Summary: Both methods short-circuit the authz gate when cx.principal == None. An attacker who can call any public route can trigger unbounded outbound email per call to arbitrary or attacker-supplied addresses — exhausting SMTP-relay quota, getting the relay blacklisted, or burning provider credits. No per-app rate limit, per-recipient cooldown, or per-execution counter. Fix shape: token-bucket rate limit keyed on (cx.app_id, recipient_email), with a per-app daily cap. Optionally require cx.principal.is_some() when invoked from a public route.
F-P-001 — users::list N+1: one fetch_roles query per user row
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/users_service.rs:466-471
  • Summary: list page-fetches users then loops calling self.fetch_roles(cx.app_id, row.id) — one query per user. Default limit 50, max 500, so the admin "users" page does 51-501 round-trips. The same fetch_roles is also called from verify_session_for_realtime (one extra query on every authenticated SSE subscribe). Fix: add AppUserRoleRepo::list_for_users(app_id, &[user_ids]) returning a HashMap<AppUserId, Vec<String>> and join in a single query (WHERE user_id = ANY($2)).
F-P-002 — Argon2id password / API-key verify runs synchronously on the Tokio async worker
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/auth_middleware.rs:206-208, crates/manager-core/src/auth_api.rs:98, crates/manager-core/src/users_service.rs:502
  • Summary: verify_password (Argon2id default params: m=19456 KiB, t=2) is CPU-bound at tens-to-hundreds of ms and is invoked synchronously on the Tokio worker. Worse, verify_api_key Argon2-verifies every candidate sharing the 8-char prefix on every authenticated /api/v1/admin/* request — a single hot user with N keys serializes every admin request behind N×Argon2 verifies. Fix: wrap each verify in tokio::task::spawn_blocking, and maintain a short-lived (60-300s) cache LruCache<(prefix, raw)→user_id> so a hot API key doesn't re-Argon2 every request.
F-P-003 — Postgres pool max_connections=10 vs ExecutionGate default 32
  • Category: Performance
  • Severity: High
  • Location: crates/picloud/src/lib.rs:583-588 vs crates/orchestrator-core/src/gate.rs
  • Summary: init_db hard-codes max_connections(10). The execution gate allows 32 concurrent script executions, each doing multiple sequential DB calls (script resolve, every SDK call inside the script, log sink, outbox emit), plus the dispatcher tick every 100ms, the cron tick, three GC sweeps, and the auth middleware running per request. Pool starvation manifests as acquire_timeout (5 s) errors and tail-latency spikes. Fix: scale max_connections to roughly gate.max_permits × estimated_queries_per_exec + headroom, expose a PICLOUD_DB_MAX_CONNECTIONS env knob, and document the relationship.
F-P-004 — LocalExecutorClient::execute and invoke() re-entry recompile AST every call
  • Category: Performance
  • Severity: High
  • Location: crates/orchestrator-core/src/client.rs:178-216, crates/executor-core/src/sdk/invoke.rs:192-200
  • Summary: Two code paths bypass the AST cache: ExecutorClient::execute (used in tests + fallback) calls engine.execute(&source, req) which compiles inline; and invoke() synchronous re-entry calls self_engine.execute(&resolved.source, req), re-parsing every callee on every invoke. Composed workflows multiply parse cost by depth. Fix: have InvokeService::resolve return (ScriptIdentity, source) and route through the cached LocalExecutorClient::get_or_compile, or expose Engine::execute_with_identity so the engine itself can cache.
F-P-005 — execution_logs admin list uses OFFSET pagination
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/repo.rs:511-535
  • Summary: list_for_script does ORDER BY created_at DESC LIMIT $2 OFFSET $3. Postgres has to scan + discard OFFSET rows on every page; deep pages get linearly slower. Sister tables in the same repo all use cursor pagination. Fix: switch to keyset cursor on (created_at, id) — WHERE script_id=$1 AND (created_at, id) < ($cursor_ts, $cursor_id). Same shape as every other list endpoint.
F-P-006 — queue::depth / depth_pending are COUNT(*) reachable per script call
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/queue_repo.rs:264-287, crates/manager-core/src/queue_repo.rs:289-322
  • Summary: Scripts call queue::depth(name) / queue::depth_pending(name) per invocation; each is an unbounded COUNT(*) over the queue_messages partial index. On a backed-up queue with millions of rows, every call scans the whole partition. list_for_app (dashboard queue overview) does 3× COUNT(*) FILTER (...) in one pass — same scan magnified. Fix: maintain per-(app_id, queue_name) running counters in a small table (incremented on enqueue, decremented on ack/dead-letter), or downgrade depth() to a presence check (SELECT 1 … LIMIT 1).
F-P-007 — Dispatcher tick loops queue consumers serially (one claim per consumer per tick)
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/dispatcher.rs:152-164, crates/manager-core/src/dispatcher.rs:166-290
  • Summary: tick_queue_arm calls list_active_queue_consumers() then iterates serially, awaiting one queue.claim(app, queue) per consumer per tick. With N queue consumers and a 100ms tick, worst-case throughput is N × (claim + executor) / tick. Each iteration is a full dispatch_one_queue (resolve script, resolve principal, executor round-trip) sequential on the dispatcher's task. Fix: bound concurrency with futures::stream::iter(consumers).for_each_concurrent(N, …) (the execution gate caps anyway), and/or cache the consumer list across ticks.
F-P-008 — TriggerRepo::list_for_app is N+1 — one detail-table query per parent row
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/trigger_repo.rs:971-987 (calls hydrate_one at ~:1351 per row)
  • Summary: list_for_app selects parent rows then for each one issues a SELECT … FROM <kind>_trigger_details WHERE trigger_id = $1. For N triggers on an app, that's N+1 queries on every dashboard GET /apps/{id}/triggers load. Fix: split by kind and issue one-per-kind JOIN <details> ON details.trigger_id = t.id WHERE t.app_id=$1 AND t.kind='<k>', or write a single CTE with LEFT JOIN to each *_trigger_details table.
F-P-009 — attach_principal_if_present runs on every data-plane request, including paths that don't need auth
  • Category: Performance
  • Severity: High
  • Location: crates/manager-core/src/auth_middleware.rs:141, crates/manager-core/src/auth_middleware.rs:192-229
  • Summary: Every request hitting /api/v1/execute/... or any user-route path goes through attach_principal_if_present. If the request carries any Authorization: Bearer pic_... header, the middleware does: prefix lookup + Argon2 verify per candidate + admin_users.get + touch_last_used UPDATE. Three DB round-trips + Argon2id per request — on a path that may not need authz at all. Fix: short-circuit when the route doesn't require auth; add a sliding-window cache of (token → principal) valid for 60-300s.
F-Q-001 — manager-core depends on orchestrator-core for behavior, reversing the architectural arrow
  • Category: Code Quality
  • Severity: Critical
  • Location: crates/manager-core/Cargo.toml:13, crates/manager-core/src/dispatcher.rs:28, crates/manager-core/src/route_admin.rs:15, crates/manager-core/src/apps_api.rs, crates/manager-core/src/invoke_service.rs:15, crates/manager-core/src/pubsub_service.rs:519
  • Summary: CLAUDE.md's bright line: "the orchestrator never imports executor-core directly — define a trait in shared". The same rule should hold for manager-core ↔ orchestrator-core. Instead, manager-core pulls picloud_orchestrator_core::routing::{RouteTable, AppDomainTable, matcher::CompiledRoute, pattern, conflict} plus {ExecutorClient, ExecutionGate, ScriptIdentity, ScriptResolver, ResolverError, InProcessBroadcaster}. RouteTable is a stateful Arc'd object with methods (replace_all, match_request_for_app) — not a DTO. This kills the cluster-mode swap: when manager and orchestrator are separate binaries the control plane shouldn't link the orchestrator's routing-table impl. Fix: move RouteTable, AppDomainTable, CompiledRoute, CompiledAppDomain, ScriptResolver, ResolverError, ExecutorClient, ExecutionGate, ScriptIdentity, and the broadcaster trait into shared/ (traits + DTOs); keep only in-process impls in orchestrator-core.
F-Q-002 — Every SDK module open-codes its own block_on helper
F-Q-003 — Each stateful service open-codes the same check_read / check_write authz idiom
F-Q-004 — Sibling services have inconsistent error-variant shapes, and QueueError is missing Forbidden entirely
  • Category: Code Quality
  • Severity: High
  • Location: crates/shared/src/kv.rs:139, crates/shared/src/queue.rs:60-77, crates/shared/src/pubsub.rs:85, crates/shared/src/invoke.rs:110
  • Summary: Same semantic error has two names: "Backend" in KvError/DocsError/FilesError/SecretsError; "Unavailable" in QueueError/PubsubError/InvokeError. Worse, QueueError has no Forbidden variant at all, so authz denial gets squashed into QueueError::Rejected("forbidden".into()) (queue_service.rs:68), losing the structured variant a 403-translation layer would need. And queue_service::depth/depth_pending skip authz entirely. Fix: pick one name (Backend), add Forbidden to every service's error enum uniformly, and gate depth/depth_pending on AppQueueRead (or document why they're public).
F-Q-005 — Authz failures collapse AuthzDenied::Repo into Forbidden via map_err(|_| …)
F-Q-006 — InboxRegistry::register silently no-ops on lock poisoning, leaking a dead id
  • Category: Code Quality
  • Severity: High
  • Location: crates/orchestrator-core/src/inbox.rs:46-53
  • Summary: register() returns (Uuid, Receiver) even when the inner Mutex is poisoned — the if let Ok(mut g) = self.inner.lock() swallows the error, the sender is never inserted, the later deliver(id, …) will see no entry and return Abandoned. The caller's await rx blocks until the orchestrator's outer timeout. Sister registries RouteTable (routing/table.rs:36) and AppDomainTable correctly panic with .expect("route table poisoned"). Fix: switch to expect("inbox poisoned") and document the policy.
F-T-001 — useEndpoint violates React Rules of Hooks
  • Category: Code Quality
  • Severity: High
  • Location: clients/typescript/src/react/index.ts:73-101
  • Summary: useEndpoint(path) returns { get: () => useResource(…), post: (body) => useResource(…) }. Each of .get() and .post() calls useResource, which calls useState and useEffect. React's Rules of Hooks require hook calls in the top level of a component, in the same order each render. Calling useEndpoint(path).get() conditionally, or calling both .get() and .post() from the same component, will violate hook ordering and produce undefined behavior (state from one call leaking into the other). react.test.tsx doesn't exercise useEndpoint at all — only useTopic — so this is uncaught. Fix: refactor to a flat hook (useEndpointGet(path) / useEndpointPost(path)) with consistent hook order.
F-U-001 — Dashboard cannot create KV, Docs, Files, or Dead-letter triggers
  • Category: UI/UX
  • Severity: High
  • Location: dashboard/src/routes/apps/[slug]/+page.svelte:1092-1342, dashboard/src/lib/api.ts:771-801
  • Summary: Backend triggers_api.rs exposes POST /apps/{id}/triggers/{kv,docs,files,dead_letter}, but the dashboard's Triggers tab ships create forms only for cron, pubsub, email, and queue. Listing shows kv/docs/files/dead_letter rows (with collection_glob and ops) but operators can't create them from the UI. Add four more submitCreate* forms (collection glob + ops checkboxes).
F-U-002 — Queue drilldown links to a non-existent app-scoped scripts route
  • Category: UI/UX
  • Severity: High
  • Location: dashboard/src/routes/apps/[slug]/queues/[name]/+page.svelte:71
  • Summary: The consumer-script link is {base}/apps/{slug}/scripts/{detail.consumer.script_id} but the actual script detail route is {base}/scripts/{id} — there is no apps/[slug]/scripts/[id] directory. Clicking the consumer name 404s. Either move scripts under apps in the route tree (the breadcrumb on the script page already wants this), or fix the link to {base}/scripts/{detail.consumer.script_id}.
F-U-003 — Files page has no collection list — operators must guess collection names
  • Category: UI/UX
  • Severity: High
  • Location: dashboard/src/routes/apps/[slug]/files/+page.svelte:96-110
  • Summary: The Files page can only list a collection if you already know its name and type it in. There's no "browse known collections" affordance, no list endpoint (GET /apps/{id}/files/collections is missing in the backend too — flag for v1.1.10+), so first-time operators have no recourse. Minimum fix: show a hint listing collection-glob patterns drawn from any registered files triggers. Longer-term: backend endpoint to list known collections.

Medium

F-S-003 — users::find_by_email exposes user existence to anonymous callers
  • Severity: Medium
  • Location: crates/manager-core/src/users_service.rs:400-413
  • Summary: find_by_email requires AppUsersRead, which means anonymous public-HTTP scripts can call it (the gate short-circuits on None). An attacker hitting any public route can iterate emails to enumerate registered users, then pivot to login / request_password_reset. The reset path is silent-on-miss; find_by_email is not. Fix: require an authenticated principal for find_by_email, or document that scripts must wrap it in their own auth check before calling from a public route.
F-S-004 — request_password_reset has an observable timing oracle for email existence
  • Severity: Medium
  • Location: crates/manager-core/src/users_service.rs:646-651
  • Summary: Handler returns immediately when email shape is invalid or user is unknown; on found user it generates an Argon2 token, INSERTs into app_user_password_resets, builds a link, and calls email.send (tens of ms + DB write + SMTP RTT). The wall-clock delta is enormous and externally observable. Combined with F-S-003, this gives a reliable enumeration oracle. Fix: when no user matches, still do a dummy generate_session_token() and a dummy email.send (against /dev/null or a fixed sleep). Better: run the full code path unconditionally and discard the result on no-match.
F-S-005 — users_admin_api::delete_user doesn't enforce AppUsersAdmin despite the brief
  • Severity: Medium
  • Location: crates/manager-core/src/users_admin_api.rs:250-272
  • Summary: Handler comment acknowledges the brief required AppUsersAdmin for the irreversible dashboard delete, but the implementation calls service.delete(...) which only gates on AppUsersWrite. v1.1.8 HANDBACK §7 lists this as known. Effect: any editor can delete app users via the admin HTTP API and dashboard button, not just admins. Fix: add an explicit authz::require(... AppUsersAdmin(app_id)) before service.delete.
F-S-006 — API-key bearer creates Argon2 CPU amplifier for arbitrary callers
  • Severity: Medium
  • Location: crates/manager-core/src/auth_middleware.rs:192-211
  • Summary: verify_api_key Argon2id-verifies every candidate sharing the 8-char prefix. An unauthenticated attacker can submit unlimited Authorization: Bearer pic_<8>... requests and force the server into per-request Argon2 (OWASP defaults: ~tens of ms each). A few hundred concurrent connections trivially DoSes the manager — the verify runs before any concurrency cap. The 32-byte random body has astronomical entropy, so Argon2 is overkill here. Fix: switch the key-verify hash to SHA-256 (high-entropy tokens don't need Argon2), cap the candidate set at a small constant (e.g. 16) and log when truncation occurs, and add per-IP rate limit at the reverse proxy.
F-S-007 — No rate limit on auth/login — Argon2 verify is a memory DoS
  • Severity: Medium
  • Location: crates/manager-core/src/auth_api.rs:74-101
  • Summary: Login always runs Argon2 verify (timing-flat — good), but no rate limit, no failed-attempt lockout, no captcha. OWASP-default Argon2 cost (m=19456 KiB ≈ 19 MB RAM per verify) makes this a credible memory + CPU DoS: a few hundred concurrent POST /admin/auth/login exhausts manager memory. Combined with F-S-006, the auth surface has no brute-force defense. Fix: per-IP throttling at the reverse proxy (documented Caddy rate-limit module) and a per-username sliding-window lockout in admin_user_repo.
F-S-008 — Realtime authority key cache never invalidates
  • Severity: Medium
  • Location: crates/manager-core/src/realtime_authority.rs:34,56-72
  • Summary: key_cache: Mutex<HashMap<AppId, Vec<u8>>> is populated on first read and never evicted, bounded, or cleared on app deletion. (1) Once key rotation lands, every running process keeps accepting tokens signed by the old key until restart. (2) Today, if an operator drops and re-creates an app (FK CASCADE removes app_secrets, fresh row inserted), the cache hands out the old key bytes. Also unbounded growth on a many-app install. Fix: TTL on cache entries, rotation eviction hook, or simplify by removing the cache (DB lookup is cheap; only fires on subscribe).
F-S-009 — PICLOUD_DEV_MODE deterministic master key is fatal if accidentally enabled in prod
  • Severity: Medium
  • Location: crates/shared/src/crypto.rs:206-231
  • Summary: When PICLOUD_DEV_MODE=true AND PICLOUD_SECRET_KEY is unset, the master key becomes SHA-256("picloud-dev-master-key-v1.1.7") — a fully public value. The warning is correct but the gate is a single env var. An operator who copies a dev docker-compose file into prod silently encrypts everything with a world-known key. Fix: add a startup check that refuses dev mode when prod indicators are present (non-localhost PICLOUD_PUBLIC_BASE_URL, etc.), or require BOTH PICLOUD_DEV_MODE=true AND PICLOUD_DEV_INSECURE_KEY=i-understand-this-is-insecure.
F-S-010 — Inbound-email HMAC verification has no replay protection
  • Severity: Medium
  • Location: crates/manager-core/src/email_inbound_api.rs:83-144
  • Summary: Receiver verifies HMAC-SHA256(secret, body) constant-time — good. But the signature covers only the body. No timestamp binding, no nonce/idempotency check. A captured webhook request can be replayed indefinitely, re-firing the trigger and enqueueing duplicate executions. Industry standard (Stripe, Slack) includes a timestamp in the signed payload and rejects requests outside a ~5-minute window. Fix: add X-Picloud-Timestamp to the HMAC input, reject requests outside the configured window, and dedupe on message_id in a TTL'd cache.
F-S-011 — admin_sessions has no revoked_at — password change can't invalidate live sessions
  • Severity: Medium
  • Location: crates/manager-core/src/admin_session_repo.rs:1-152
  • Summary: Unlike app_user_sessions (v1.1.8 — has revoked_at and revoke_for_user), admin_sessions only has expires_at and a delete_for_user(user_id) helper. No admin-side "revoke all sessions" gesture; an admin password change must explicitly call delete_for_user (audit the flow to confirm it does). Fix: add a revoked_at column for explicit revocation distinct from TTL expiry, and add a "log out all sessions" action.
F-S-012 — invoke_async has no authz check — anonymous scripts can fire any script in the app
  • Severity: Medium
  • Location: crates/manager-core/src/invoke_service.rs:121-161, crates/executor-core/src/sdk/invoke.rs:86-110
  • Summary: enqueue_async and the sync invoke() perform no authz::require check — no AppInvoke capability exists. Same-app isolation is preserved (cross-app guard works), but within one app, an anonymous public-HTTP script can trigger any other script (e.g. an admin-only worker that hits secrets/files/external HTTP). Worse: invoke_async runs the callee with principal: None, so the callee may itself hold capabilities the original public caller shouldn't. Fix: add Capability::AppInvoke(app_id) and gate invoke() / invoke_async() on it; consider a "private" vs "public" script tag for the anonymous case.
F-S-013 — users::invite accumulates pending invitations per email without dedup
  • Severity: Medium
  • Location: crates/manager-core/src/users_service.rs:720-778, crates/manager-core/migrations/0030_app_user_invitations.sql:16-26
  • Summary: No unique constraint on (app_id, email) for pending rows. Calling users::invite("alice@…") N times creates N rows with N valid tokens. Combined with accept_invite returning Ok(None) silently when the email already exists, an attacker who learns one invitation token can permanently consume it without effect. Fix: partial unique index (app_id, lower(email)) WHERE accepted_at IS NULL + handle conflict as 409 with "reissue?" UX.
F-S-014 — Topic edit modal warns only on internal → external flip, not on any other permissive change
  • Severity: Medium
  • Location: dashboard/src/routes/apps/[slug]/+page.svelte:1678-1722
  • Summary: Warning fires only for the internal→external transition (editFlipToExternal). Switching token → public or session → public while already external is equally risky (opens topic to anonymous subscribers) but produces no warning. Fix: fire the warning whenever the resulting (external, auth_mode) pair is strictly more permissive than the current one.
F-P-010 — list_active_queue_consumers (called every 100ms) lacks a matching index
  • Category: Performance
  • Severity: Medium
  • Location: crates/manager-core/src/trigger_repo.rs:1290-1303 vs crates/manager-core/migrations/0008_triggers.sql:47-49
  • Summary: Query is WHERE t.kind='queue' AND t.enabled=TRUE with no app_id filter; available index idx_triggers_app_kind_enabled (app_id, kind) WHERE enabled = TRUE requires an app_id predicate to be useful. With many apps × many trigger kinds, this query becomes a sequential scan every 100ms. Fix: add CREATE INDEX idx_triggers_kind_enabled ON triggers(kind) WHERE enabled = TRUE, or maintain an in-memory consumer registry refreshed by admin writes.
F-P-011 — RouteTable::replace_all rebuilds the entire per-app trie on every route CRUD write
F-P-012 — app_user_repo::list cursor lacks an (created_at, id) tiebreaker
  • Category: Performance
  • Severity: Medium
  • Location: crates/manager-core/src/app_user_repo.rs:200-225
  • Summary: ORDER BY created_at DESC, id DESC but cursor is WHERE created_at < $2 — when two users were created at the same instant, pagination can skip the second row at a page boundary or return it twice. Fix: encode (created_at, id) in the cursor and use WHERE (created_at, id) < ($ts, $id). Same shape used elsewhere in this repo.
F-P-013 — RhaiEngine + every SDK module registered per call
  • Category: Performance
  • Severity: Medium
  • Location: crates/executor-core/src/engine.rs:167-204, crates/executor-core/src/sdk/mod.rs:49-68
  • Summary: Each execute_ast calls build_engine (new Rhai engine, every limit setter, disable-symbol, register all stdlib static modules), then sdk::register_all registers 12 service modules. At 32 concurrent executions on a Pi, this is real per-call overhead even with AST caching. Fix shape: pool pre-built Rhai engines with stateless modules pre-registered, reset per-call state, and only install per-call closures that need SdkCallCx.
F-P-014 — regex::* SDK functions compile patterns on every call
  • Category: Performance
  • Severity: Medium
  • Location: crates/executor-core/src/sdk/stdlib/regex.rs:23-25
  • Summary: is_match, find, find_all, replace, replace_all, split, captures all call Regex::new(pattern) per invocation. A script doing regex::is_match("\\d+", x) in a tight loop pays the compile every iteration. Fix: thread-local LruCache<String, Arc<Regex>> bounded to e.g. 128 entries. Regex compile dominates is_match on short strings.
F-P-015 — OutboxEventEmitter inserts one row per matching trigger
  • Category: Performance
  • Severity: Medium
  • Location: crates/manager-core/src/outbox_event_emitter.rs:88-103
  • Summary: For each matched trigger, one separate outbox.insert round-trip. A KV mutation matched by 5 triggers becomes 5 sequential INSERTs inside the script's hot path, each cloning the JSONB payload. Fix: extend OutboxRepo::insert_many(rows) and emit one multi-row INSERT … SELECT * FROM UNNEST(...); bind payload once.
F-P-016 — app_user_session_repo::gc OR chain mismatches the available index
  • Category: Performance
  • Severity: Medium
  • Location: crates/manager-core/src/app_user_session_repo.rs:186-200 vs crates/manager-core/migrations/0027_app_user_sessions.sql:34-35
  • Summary: GC inner SELECT predicate is expires_at <= NOW() OR absolute_expires_at <= NOW() OR revoked_at IS NOT NULL. Available partial index is (expires_at) WHERE revoked_at IS NULL — only helps the first disjunct on non-revoked rows. The OR chain forces a sequential scan. Fix: add CREATE INDEX … ON app_user_sessions(revoked_at) WHERE revoked_at IS NOT NULL, and rewrite the GC as three UNION ALL subqueries (one per disjunct).
F-Q-007 — DispatcherError truncates the entire error chain into String
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/manager-core/src/dispatcher.rs:957-963 and many call sites (lines 129, 157, 176, 228, 235, 353, 365, 379, 391, 411, 453, 462, 488, 494, 545, 615, 724, 755, 779)
  • Summary: DispatcherError::{Outbox, ResolveTrigger} wrap String. Every ? calls .to_string() on the source, killing #[source] introspection and the cause chain. Logs show "outbox: database error: ..." with no programmatic way to discriminate. Fix: convert to #[from] sqlx::Error (or structured variants) so tracing::error!(error.cause_chain = ?err) works.
F-Q-008 — HttpError::Backend misclassifies validation errors
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/manager-core/src/http_service.rs:209,340,342
  • Summary: Method::from_bytes failing on a user-supplied HTTP method gets HttpError::Backend(format!("invalid method: {}", req.method)) — but that's user input, not a backend problem. Same for header-name / header-value validation. Misclassifying as Backend corrupts retry policies (operators may retry "backend" but not "invalid input"). Fix: add HttpError::InvalidArgs(String).
F-Q-009 — dispatcher::TICK_INTERVAL and ASYNC_EXEC_TIMEOUT are hard-coded; siblings are env-overridable
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/manager-core/src/dispatcher.rs:73,80
  • Summary: TICK_INTERVAL = 100ms and ASYNC_EXEC_TIMEOUT = 300s are const. Every other timing knob in the file (cron tick, queue reclaim, retry policy) is env-overridable via TriggerConfig::from_env(). Operators on a constrained Pi or a busier instance can tune retries but not dispatcher cadence. Fix: promote to TriggerConfig with PICLOUD_DISPATCHER_TICK_INTERVAL_MS and PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC.
F-Q-010 — Mutex poisoning policy inconsistent across orchestrator-core registries
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/orchestrator-core/src/inbox.rs:49,75 (silent), crates/orchestrator-core/src/routing/table.rs:36 (loud expect)
  • Summary: Two registries playing the same architectural role (in-memory caches behind Arc) handle poisoning oppositely. RouteTable / AppDomainTable expect("route table poisoned"); InboxRegistry silently swallows. Settle on one policy — the loud expect is correct (poisoning means a prior panic; nothing is recoverable). See also F-Q-006.
F-Q-011 — PostgresScriptRepoHandle newtype hand-delegates 11 methods
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/picloud/src/lib.rs:622-693
  • Summary: A 70-line newtype wraps Arc<PostgresScriptRepository> and re-implements every ScriptRepository method via self.0.method(...).await. Comment claims it exists because the resolver wants impl ScriptRepository "owned." Fix: provide a blanket impl<T: ScriptRepository + ?Sized> ScriptRepository for Arc<T> in manager-core, or accept &dyn/Arc<dyn> ScriptRepository everywhere. Hand-delegation invites silent skew when the trait gains a method.
F-Q-012 — build_app is ~480 lines of pure wiring
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/picloud/src/lib.rs:101-578 (#[allow(clippy::too_many_lines)])
  • Summary: Constructs ~30 Arc<dyn ...> handles, ~15 *State structs, ~12 routers, spawns 6 background tasks — all in one function with an opt-out clippy allow. Fix: extract wire_services(pool, master_key) -> Services, wire_admin_states(...) -> AdminStates, spawn_background_tasks(...). Current shape makes the "field-missing" mistake in F-Q-013 easy.
F-Q-013 — Services::new takes 13 positional Arc<dyn …> args — adding a service silently breaks all callers
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/shared/src/services.rs:118-148, crates/picloud/src/lib.rs:296-310
  • Summary: Constructor is #[allow(clippy::too_many_arguments)] and takes 13 positional handles (kv, docs, dl, events, modules, http, files, pubsub, secrets, email, users, queue, invoke). Adding workflows in v1.2 means every caller breaks silently if order is wrong. Fix: builder (Services::builder().kv(kv).docs(docs)….build()) or struct-literal constructor — both make field-name binding type-checked.
F-Q-014 — Hundreds of #[ignore]'d integration tests with no CI hook
  • Category: Code Quality
  • Severity: Medium
  • Location: crates/picloud/tests/api.rs (~60 ignored), crates/picloud/tests/authz.rs (~30 ignored), crates/picloud-cli/tests/* (~70 ignored)
  • Summary: Every test is #[ignore = "needs DATABASE_URL pointing at a running Postgres"]. Hundreds of tests don't run unless a developer explicitly runs cargo test -- --ignored with Postgres. No marker for "this was the test that was supposed to catch X." Fix: flip to the schema_snapshot.rs / dispatcher_e2e.rs convention (auto-skip when Postgres is absent, no #[ignore]), or document the CI workflow that runs them.
F-M-001 — idx_cron_triggers_due index doesn't match the actual scheduler query
  • Category: Migration/Schema
  • Severity: Medium
  • Location: crates/manager-core/migrations/0017_cron_triggers.sql:41, crates/manager-core/src/cron_scheduler.rs:117-123
  • Summary: Index is on (last_fired_at) with a comment claiming it serves the scheduler. The actual query is WHERE t.enabled = TRUE FOR UPDATE OF d SKIP LOCKED — no last_fired_at predicate. The index is unused; the query is effectively a full-table scan of cron details joined to enabled triggers. Fix: either the query should filter on last_fired_at (to skip very-recently-fired rows), or the index should be dropped.
F-M-002 — email_trigger_details encrypted-secret columns lack coupled-nullness CHECK
  • Category: Migration/Schema
  • Severity: Medium
  • Location: crates/manager-core/migrations/0024_email_triggers.sql:28-32
  • Summary: inbound_secret_encrypted and inbound_secret_nonce are each nullable independently. There is no CHECK that they be either both NULL or both NOT NULL. A bug or partial write could leave one populated and the other NULL — silently bypassing signature verification (receiver decrypts only if the encrypted column is set). Fix: CHECK ((inbound_secret_encrypted IS NULL) = (inbound_secret_nonce IS NULL)). Same pattern applies to app_secrets.realtime_signing_key_encrypted/nonce (0025).
F-M-003 — outbox.trigger_id is polymorphic — no FK, orphans accumulate after route/trigger delete
  • Category: Migration/Schema
  • Severity: Medium
  • Location: crates/manager-core/migrations/0009_outbox.sql:29
  • Summary: The migration notes trigger_id is polymorphic (routes.id when source_kind='http', else triggers.id) and has no FK. Deleting a route or trigger leaves orphan outbox rows pointing at non-existent parents. Dispatcher tolerates this (rows fail dispatch and dead-letter), but a route delete + dispatcher restart can produce orphan dead-letters referencing a script that no longer matches the original route. Fix: explicit cleanup in route/trigger delete paths; consider source-kind-keyed FKs via a partial constraint or trigger.
F-T-002 — useEndpoint().post() auto-fires on mount — a typo creates user records or sends emails per refresh
  • Category: Code Quality
  • Severity: Medium
  • Location: clients/typescript/src/react/index.ts:73-100
  • Summary: README says "the mutation variant (auto-fires once per mount)" — but a typical mutation is event-driven (click → POST), not fire-on-mount idempotent. Auto-firing a POST on mount creates user records, sends emails, etc. unconditionally. useEndpoint('/api/users').post({ name }) on a typo will create a user on every refresh. Fix: refactor to { mutate, data, loading, error } pattern; mutate(body) is event-driven, not auto-firing.
F-U-004 — Five dashboard pages render light-theme fallback colors on the dark-theme app
F-U-005 — Trigger create forms hide dispatch_mode and retry-policy knobs
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/apps/[slug]/+page.svelte:1101-1296
  • Summary: Trigger.dispatch_mode, retry_max_attempts, retry_backoff, retry_base_ms are all editable via the create API and visible in the list, but the cron/pubsub/email create forms never expose them — only queue has retry_max_attempts. Operators have no way to set sync vs async dispatch or backoff strategy from the UI. Fix: add an <details>Advanced</details> block per trigger form.
F-U-006 — Trigger enabled flag never displayed; no toggle UI
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/apps/[slug]/+page.svelte:1300-1340
  • Summary: Trigger.enabled: boolean is on the DTO and the backend supports disabled triggers, but the trigger list never shows the state and offers no pause/resume action. Add at minimum a "• disabled" pill; ideally a toggle. If no PATCH endpoint exists, flag for backend.
F-U-007 — Native confirm() and alert() used for route deletion
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/scripts/[id]/+page.svelte:282,287
  • Summary: removeRoute() uses window.confirm('Delete this route?') and surfaces errors via window.alert(). The rest of the dashboard adopted ConfirmModal for exactly this case. The browser modal is unstyled, can't show route detail, and the alert is a dead-end. Fix: convert to a ConfirmModal with route details in the body and an inline error region.
F-U-008 — Script-page "← Scripts" back-link drops out to /apps
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/scripts/[id]/+page.svelte:420
  • Summary: "← Scripts" link uses base + '/', which the root page redirects to /apps. After deleting a script the user is also bounced to base + '/'. The breadcrumb already resolves appSlug; the back-link should be {base}/apps/{appSlug}.
F-U-009 — Files page exposes no upload, download, or copy-id affordance
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/apps/[slug]/files/+page.svelte:120-153
  • Summary: Listing shows name/content-type/size/created/id; only mutation is Delete. No download link, no copy-id button (the UUID is shown but unclickable), no preview, no metadata refresh. Operators must use the admin API directly. Fix: add download link per row and a copy-id button.
F-U-010 — No instance-level configuration page surfacing PICLOUD_* env knobs
  • Category: UI/UX
  • Severity: Medium
  • Location: (missing) dashboard/src/routes/config/
  • Summary: The platform reads ~25 PICLOUD_* env vars at startup (concurrency cap, session TTL, sandbox ceilings, files root + max size, SMTP timeout/TLS/port, secret max bytes, email max bytes, queue reclaim, trigger retry, HTTP allow-private, script/module cache sizes, public base URL, cookie secure). None are visible from the dashboard. Operators debugging "why is upload rejected" or "why did session expire early" have to ssh in. Fix: add a read-only /admin/config page using a new GET /api/v1/admin/config endpoint that returns non-secret current values.
F-U-011 — Cron trigger schedule has no helper, preview, or field-count validation
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/apps/[slug]/+page.svelte:1112-1118
  • Summary: Cron input is a plain text box with placeholder 0 0 9 * * MON-FRI. Description says "6-field cron expressions (with seconds)" but a user typing a typical 5-field crontab (0 9 * * 1-5) gets a confusing 422 from the server. No "next 3 fires" preview, no client-side validation. Fix: split-count check before submit, info popover listing the 6 positions, and a cron-parser preview of the next fires.
F-U-012 — Logs viewer hard-capped at 50 rows, no pagination or filtering
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/scripts/[id]/+page.svelte:369,773-839
  • Summary: api.scripts.logs(id, { limit: 50 }) is the only call; backend supports offset (see api.ts:659) but there's no "load more". No filter by status (success/error/timeout/budget_exceeded), no filter by request path or log level, no time range. Operators investigating a failing script see only the most recent 50 with no way to drill back. Fix: offset pagination, status filter dropdown, and a level filter.
F-U-013 — Queues read-only pages have no auto-refresh or refresh button
F-U-014 — Email-inbound webhook URL uses window.location.origin — wrong under reverse proxy
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/apps/[slug]/+page.svelte:246-248
  • Summary: emailInboundUrl() builds ${window.location.origin}/api/v1/email-inbound/.... If the admin browses via an internal LAN address but the public webhook URL is on a different host, the dashboard hands the operator the wrong URL. VersionInfo.public_base_url already exists for exactly this case. Fix: use version.public_base_url instead of window.location.origin.
F-U-015 — No session-expiry warning; silent 401 redirect drops in-flight form state
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/lib/api.ts:438-446
  • Summary: On any 401 the fetch wrapper immediately calls clearSession() and goto('/login'). A user mid-edit on a long Rhai script loses the entire scratch. No warning before expiry, no interstitial showing lost state, no draft auto-save. Fix: session-warning toast ~5min before TTL; localStorage-backed source drafts; "your session expired, sign in again" interstitial preserving form state.
F-U-016 — Several modals lack focus trap — Tab moves focus to background elements
F-U-017 — Users tables grid columns overflow below ~700px viewport
F-U-018 — Dashboard can't create instance admins in the "owner" role
  • Category: UI/UX
  • Severity: Medium
  • Location: dashboard/src/routes/users/+page.svelte:452-462
  • Summary: Invite-user modal offers radios "admin" and "member" only. Owner can be granted only by editing an existing user. This is intentional friction (prevents the first-owner footgun) but is asymmetric with editRoleOptions (line 111) where owners can assign owner directly. Document the distinction visibly above the role group, or expose "Owner" for the bootstrapping case.

Low

  • F-S-015 — users::login requires AppUsersWrite so authenticated viewer-role scripts can't authenticate users — sharp edge for "login form on an authed admin page". users_service.rs:478-485 — Low
  • F-S-016 — Dispatcher trigger_depth > max vs invoke SDK cx.trigger_depth + 1 > max differ by one level; align the boundary semantics and add a unit test. dispatcher.rs:342, invoke.rs:141
  • F-S-017 — SmtpConfig derives Debug with raw password: String — accidental tracing::debug!(?cfg) leaks. email_service.rs:88-97. Hand-implement Debug with redaction.
  • F-S-018 — OutboxEventEmitter log-and-ignore on emit failure: triggers can silently miss events if the outbox insert fails after the primary write. kv_service.rs:114-130, docs_service.rs:122-138, files_service.rs:74-102. Bump a metric on emit failure; long-term make primary + outbox a single tx.
  • F-S-019 — retry::run allows cumulative sleep far exceeding the wall-clock timeout, tying up a gate permit. retry.rs:189-222. Cap cumulative sleep inside retry::run.
  • F-S-020 — request_password_reset email-send clamping accumulates dead reset tokens when SMTP misconfigured. Mark the just-inserted row consumed when email.send returns non-NotConfigured failures. users_service.rs:678-690
  • F-S-021 — Bearer token in Set-Cookie not validated against cookie-octet alphabet — if the token format ever changes, header injection becomes possible. auth_api.rs:207-223. Guard with [A-Za-z0-9_-]+.
  • F-S-022 — attach_principal_if_present swallows DB errors as anonymous — a transient DB blip strips authed callers and converts authed writes into anonymous writes that skip the capability gate (e.g. on secrets_service). auth_middleware.rs:119-130
  • F-S-023 — users::login cross-app belt-and-suspenders check is unreachable in practice. Assert the invariant at the repo boundary instead. users_service.rs:478-514
  • F-S-024 — PICLOUD_COOKIE_SECURE env-var parsing is inverted-boolean readability. Replace with explicit is_truthy parse identical to crypto.rs. auth_api.rs:212-218
  • F-S-025 — Realtime signing key plaintext lingers in Vec<u8> allocations without zeroize; heap dumps can leak. Use Zeroizing<Vec<u8>> for the realtime signing key and ZeroizeOnDrop for MasterKey. app_secrets_repo.rs:78-91, realtime_authority.rs:34, shared/crypto.rs:117
  • F-S-026 — Inbound-email decrypt failure → 401 conflates DB blip with bad signature; provider retries hammer the endpoint indefinitely. email_inbound_api.rs:149-166. Surface decryption failure as a logged 500.
  • F-S-027 — Migration 0006 backfills every Phase-3a admin to 'owner' — documented but permissive. Future audits may surface unexpected owner sprawl. 0006_users_authz.sql:24-30
  • F-S-028 — API-key prefix collisions force the middleware to Argon2-verify every candidate; cap the candidate set at a small constant (e.g. 16). auth_middleware.rs:198-211
  • F-S-029 — admin_reset_password_token returns raw token in JSON without rate limit. Mint a new token doesn't invalidate prior unconsumed tokens. users_service.rs:902-927
  • F-S-030 — GeneratedSession/GeneratedAccept/GeneratedToken derive Debug with raw tokens — same footgun as F-S-017. shared/users.rs:89-102, auth.rs:72
  • F-S-031 — auth_middleware::touch failure becomes 500, blocking authed requests on transient DB blips. Fail-soft (log + continue with existing expiry). auth_middleware.rs:170-176
  • F-S-032 — Cookie SameSite=Lax is not exploitable today (admin POSTs accept JSON only), but adding a Form<...> extractor would make it CSRF-able. Switch admin cookie to SameSite=Strict. auth_api.rs:175,220-223
  • F-P-017 — execute_by_id clones request body, headers, and path before executing — wasteful on large bodies. Serialize logged shape from &req after a Result branch. orchestrator-core/src/api.rs:117-159
  • F-P-018 — Sync HTTP path serializes body twice and clones a 3rd time for audit log. orchestrator-core/src/api.rs:363-373
  • F-P-019 — realtime_authority::signing_key clones a Vec<u8> under Mutex on every cache hit. Use Arc<[u8]> so clone is a refcount bump. realtime_authority.rs:56-72
  • F-P-020 — Files orphan sweep walks the entire blob tree synchronously every 6h. Skip subtrees by mtime, or track temp files in a files_pending_tmp table. files_sweep.rs:75-111
  • F-P-021 — dispatcher.handle_failure decodes payload JSON 3× per failure. Decode once at top of handle_failure. dispatcher.rs:741-848
  • F-P-022 — InProcessBroadcaster::publish allocates a String from the &str topic per publish for the lookup key, even when no subscriber exists. Use a borrowed equivalent (raw_entry_mut). orchestrator-core/src/realtime.rs:107-117
  • F-P-023 — dead_letters_api unresolved_count is COUNT(*) per dashboard load. Partial index covers it today; cap at 100 with a "100+" UI state. dead_letter_repo.rs:172-181
  • F-P-024 — Inline block_on for signing_key lookup in the realtime hot path on cold cache — fleet of reconnecting subscribers hits Postgres N times. Warm cache for active apps at startup. realtime_authority.rs:99-102
  • F-P-025 — attach_principal_if_present middleware applied twice on overlapping routers (data_plane_routed + user_routes). Confirm no double-Argon2. picloud/src/lib.rs:546-553
  • F-Q-015 — pubsub_service::publish_durable spawns a task only to immediately await its JoinHandle — accomplishes nothing the inline .await wouldn't. pubsub_service.rs:193-198
  • F-Q-016 — KvError::from(KvRepoError) does Backend(e.to_string()) for any repo error; files_service has the right shape (pattern-match and promote). Adopt uniformly. kv_service.rs:74-78, files_service.rs:111-119
  • F-Q-017 — kv_service.rs and docs_service.rs inline emit-and-log boilerplate 17 lines × 2 callsites; files_service / users_service already extract it. Apply the emit helper pattern. kv_service.rs:111-130, docs_service.rs:122-138
  • F-Q-018 — auth_api::login has a stale .unwrap() after the contract has been narrowed; future refactor of the predicate ordering can re-enable the panic. Use let Some(user_id) = user_id else { … };. auth_api.rs:102
  • F-Q-019 — SDK trait methods in shared/ lack per-method rustdoc on most public surfaces. shared/src/{kv,docs,queue,files,secrets,email,pubsub,users}.rs
  • F-Q-020 — Several tests use assert!(... .is_ok()) instead of asserting on the value — auth, realtime_authority, http_service especially. client.rs:408, http_service.rs:774, auth.rs:186, realtime_authority.rs:251
  • F-Q-021 — gc::SWEEP_INTERVAL (weekly) and 5000-row batch hardcoded; retention is env-overridable but cadence isn't. Promote to TriggerConfig. gc.rs:25,30
  • F-Q-022 — SmtpConfig::from_env uses bare literal .unwrap_or(30) for default timeout. Name it DEFAULT_SMTP_TIMEOUT_SECS. email_service.rs:128
  • F-M-004 — Redundant or under-targeted indexes: secrets (app_id) duplicates PK leftmost prefix; idx_kv_entries_app_collection duplicates PK; idx_queue_trigger_details_queue_name lacks app_id; idx_triggers_app_pubsub_enabled duplicates broader idx_triggers_app_kind_enabled. Each costs write amplification without enabling new plans. 0007_kv.sql:28, 0020_pubsub_triggers.sql:32, 0023_secrets.sql:24, 0035_queue_triggers.sql:40
  • F-M-005 — triggers.retry_max_attempts/retry_base_ms and queue_messages.max_attempts/attempt have no CHECK against negative or absurd values. Apply the scripts.timeout_seconds CHECK (> 0 AND <= 300) discipline. 0008_triggers.sql:35,38, 0034_queue_messages.sql:28-29
  • F-M-006 — dead_letters.resolution is NULL-able with no coupled-with-resolved_at CHECK. Same pattern as F-M-002. 0010_dead_letters.sql:37-40
  • F-M-007 — routes_unique_binding_idx uses COALESCE(method, '') allowing ambiguous binding (an empty-string method conflicts with a NULL method). Add CHECK method IS NULL OR method IN ('GET','POST',…). 0005_apps.sql:100
  • F-T-003 — subscribeTopic 401 refresh path recurses without backoff. If onTokenExpired returns a stale token, unbounded recursion. Track consecutive401Count. subscribe.ts:63-75
  • F-T-004 — subscribeTopic resets attempt = 0 after a successful connect, but a stream that errors immediately after one byte means the next reconnect is base * 2^0 (no backoff) — hot reconnect loop possible. Gate the reset on receiving the first event or an uptime threshold. subscribe.ts:82-96
  • F-T-005 — useTopic excludes opts from deps — a caller changing validate: zSchemaA to zSchemaB for the same topic won't take effect. Document that opts is captured once per topic. react/index.ts:39-55
  • F-T-006 — parseBody swallows JSON parse errors and silently falls through to text; the caller's validate.parse then receives raw text and throws a confusing "expected object". endpoint.ts:84-95
  • F-T-007 — joinUrl doesn't normalize repeated slashes (https://x/v1//users survives). Strip multiple consecutive / in the path portion. endpoint.ts:102-106
  • F-T-008 — tsup.config.ts externals don't include react/jsx-runtime — if anyone uses JSX syntax, the build bundles a copy. Pre-emptive add. tsup.config.ts:17
  • F-T-009 — AuthClient token storage is in-memory only; SSR and persistence-across-refresh aren't documented. Add a README section on the onToken callback + storage trade-off. auth.ts:26
  • F-T-010 — AuthClient.login hardcoded to { email, password } — overload with arbitrary credentials object would suit the hybrid-model intent. auth.ts:39-48
  • F-U-019 — Email-trigger inbound webhook URL has no copy button (compare API-key reveal pattern with clipboard support). +page.svelte:1321
  • F-U-020 — Version info fetched via api.version() but never displayed; header is the natural place for a build-info chip. scripts/[id]/+page.svelte:50
  • F-U-021 — Several pages override .container { max-width } inside a layout that already caps at 64rem — the override is silently ignored, and dead-letters' 8-column table overflows on narrow viewports. dead-letters/+page.svelte:176-180, files/+page.svelte:175-179
  • F-U-022 — $effect blocks reference variables via void slug; to force tracking — Svelte 5 cargo-cult; the synchronous deref inside load() already registers the dependency. dead-letters/+page.svelte:32-37
  • F-U-023 — Logs viewer expanded JSON has no pretty-print toggle, copy button, or syntax highlighting. scripts/[id]/+page.svelte:804-811
  • F-U-024 — Test-invoke headers field accepts any JSON value but the runtime coerces all to strings; {x-foo: null} becomes header "null" with no warning. scripts/[id]/+page.svelte:178-188
  • F-U-025 — Apps list refetches dead-letter counts in N+1 (one per app) on every mount. Backend endpoint to batch counts would be O(1). apps/+page.svelte:19-33
  • F-U-026 — Login form doesn't reset password on failed attempt — typo persists into next try. Either clear the password or add a "show password" toggle. login/+page.svelte:24-36
  • F-U-027 — App-user create form has no password strength meter or visibility toggle. The instance Users page uses generatePassword(16) + reveal-once — much safer for the admin-bootstraps-user flow. apps/[slug]/users/+page.svelte:202-205
  • F-U-028 — App-user roles input on Invitations is comma-separated string with no autocomplete from existing roles. Render as chips with autocomplete. apps/[slug]/users/invitations/+page.svelte:146-149
  • F-U-029 — Trigger creation dropdown doesn't decorate <option> text with existing-trigger counts per script — operators don't notice they're piling on. apps/[slug]/+page.svelte:1101-1296
  • F-U-030 — Members tab eligibleUsers errors only display once per session and disappear after submit attempt; the empty state for app_admin users is a dead-end with no actionable workflow. apps/[slug]/+page.svelte:519-540
  • F-U-031 — Members table inactive rows are opacity-greyed but the ActionMenu items remain fully active and the row gives no screen-reader cue. Add aria-disabled="true". apps/[slug]/+page.svelte:1048-1090
  • F-U-032 — Modal backdrop has no focus restoration on close — keyboard users lose their place. Capture document.activeElement on mount, re-focus on destroy. ConfirmModal.svelte:91-97
  • F-U-033 — Apps-list "No apps yet. Create one above…" misleads member instance-role users who can't create apps. Branch the empty-state copy. apps/+page.svelte:217-218
  • F-U-034 — ConfirmModal.confirmPhrasePrompt defaults to "Type the slug to confirm:" regardless of context (e.g. script delete uses script name, not slug). Make default neutral. ConfirmModal.svelte:121-122
  • F-U-035 — Triggers list shows full UUIDs inline; dead-letters truncates to 8 chars without tooltip or link. Render script name with full UUID as title and link to /scripts/{id}. apps/[slug]/+page.svelte:1329, dead-letters/+page.svelte:131
  • F-U-036 — No copy-to-clipboard for any UUID shown anywhere. Reusable <CopyableId> component. profile/+page.svelte:230, files/+page.svelte:138
  • F-U-037 — Timestamps via new Date(iso).toLocaleString() give no timezone hint. Show ISO UTC as title= tooltip or use relative time with absolute on hover. (Several pages.)
  • F-U-038 — Cron timezone dropdown is a hand-rolled 14-entry list; missing common zones. Use Intl.supportedValuesOf('timeZone') with datalist autocomplete. apps/[slug]/+page.svelte:34-50
  • F-U-039 — Pubsub topic-pattern input has no datalist from registered topics. Drawing from topics[].name would catch typos. apps/[slug]/+page.svelte:1152-1179
  • F-U-040 — Dashboard doesn't display public_base_url anywhere as a "this instance lives at" hint. Add to footer/header chip. api.ts:142-149
  • F-U-041 — Apps page DL-badge has title but no aria-label; screen readers hear only the bare count. apps/+page.svelte:228-232

Info

  • F-S-033 — users::accept_invite deliberately skips authz ("the invitation token IS the authorization"); document the threat model explicitly. users_service.rs:780-787
  • F-S-034 — HTTP outbox dispatch runs with principal: None even when caller was authenticated — design-as-documented but operators should know inbound-anon and triggered-from-authed-HTTP are indistinguishable inside the callee. dispatcher.rs:566
  • F-S-035 — users::admin_create_invitation synthesizes a SdkCallCx with random script_id/execution_id. Forensic noise; consider sentinel nil() values. users_service.rs:982-992
  • F-S-036 — Dispatcher uses true rand::thread_rng(); retry::run uses DefaultHasher-based pseudo-randomness that gives identical jitter for same (span, raw) pairs — defeats jitter purpose under concurrent retries. Unify. dispatcher.rs:1041 vs retry.rs:249-261
  • F-S-037 — SSRF DNS-rebind protection depends on reqwest re-resolving DNS on every connect. Verify connection-pool behavior empirically; disable connection pooling or set short idle timeout. ssrf.rs:154-221
  • F-S-038 — me endpoint has no scope check — an API key with only script:read learns the owning admin's username/instance_role/email. Document intent or filter for non-session principals. auth_api.rs:180-201
  • F-S-039 — Login response body includes raw session token in addition to setting cookie — same-origin XSS gadgets can read it. Document that the dashboard SPA should discard the response-body token and rely on the cookie. auth_api.rs:140-157
  • F-S-040 — Reveal-password modal exposes plaintext immediately on open. For shoulder-surfing protection, render as type="password" initially with a "Show value" button. apps/[slug]/+page.svelte:1744-1756
  • F-S-041 — Queue dispatcher livelocks when the registering principal becomes Inactive — message re-claimed every visibility-timeout cycle, attempt bumped each time, never reaching max_attempts. Catch Inactive/NotFound and dead-letter with reason "principal unavailable". dispatcher.rs:231-235
  • F-Q-023 — Services Arc<dyn> bundle + #[allow(clippy::too_many_arguments)] hides drift signal. See F-Q-013. shared/services.rs:117
  • F-M-008 — app_secrets/secrets/outbox/queue_messages/dead_letters JSONB value columns are NOT NULL but allow 'null'::jsonb. Probably not worth a CHECK; flag. 0007_kv.sql:18
  • F-M-009 — Migration 0006 default 'owner' widens authority on upgrade — see F-S-027. (Migration framing of same issue.)
  • F-M-010 — All migrations forward-only. Migration 0032 (drop plaintext realtime key) is a destructive point-of-no-return; CHANGELOG should mark it explicitly. 0032_drop_plaintext_realtime_signing_key.sql
  • F-M-011 — Asymmetric defaults: triggers.dispatch_mode = 'async', routes.dispatch_mode = 'sync'. Document in one place. 0008_triggers.sql:30, 0012_routes_dispatch_mode.sql:15
  • F-M-012 — app_users lacks indexes on email_verified_at / last_login_at despite likely admin-filter views. May be premature; flag for v1.2. 0026_app_users.sql:24-25
  • F-M-013 — Free-text columns (apps.slug, *_email, api_keys.scopes TEXT[], app_user_invitations.roles TEXT[], routes.host_kind 'strict' vs 'exact' naming) lack DB-level shape CHECKs. Validation lives in Rust; consistent across the repo; flagging for defense-in-depth conversation.
  • F-M-014 — pgcrypto extension declared once in 0001; subsequent migrations rely on gen_random_uuid() without re-declaring. Belt-and-suspenders CREATE EXTENSION IF NOT EXISTS pgcrypto in each migration that uses it.
  • F-T-011 — subscribeTopic sends Last-Event-ID header even though server-side replay isn't implemented (README is honest). Test suite doesn't assert the header is actually sent. subscribe.ts:51-52
  • F-T-012 — Svelte topicStore items closure resets when last subscriber unsubscribes — non-obvious lifecycle; README doesn't call it out. svelte/index.ts:20-32
  • F-T-013 — react.test.tsx fakeClient bypasses type safety with as unknown as PicloudClient. Public API additions won't be caught. Expose PicloudClientLike interface. tests/react.test.tsx:19
  • F-T-014 — package.json has both modern exports and legacy main/module/types; consistent today, but exports-only would be a single source of truth. clients/typescript/package.json:11-30
  • F-U-042 — let appBySlug = $derived(...) should be const. profile/+page.svelte:28
  • F-U-043 — /profile?denied=users deep-link doesn't replaceState-strip the param; refresh keeps the banner. profile/+page.svelte:35
  • F-U-044 — Test invoke result clobbers earlier with no history; small ring buffer would help iterative testing. scripts/[id]/+page.svelte:158-197
  • F-U-045 — route-utils.ts and slugify.ts lack vitest coverage; both are pure-function modules — easy to add. dashboard/src/lib/

Methodology notes

  • Subagents: 6 parallel general-purpose agents — Security data-plane, Security auth/secrets, Performance, Code Quality (Rust crates), UI/UX (dashboard), Migration/Schema + TypeScript client. Orchestrator (this conversation) read the project docs and verified the highest-severity findings against actual file/line state before writing the report.
  • Wall-clock: Single message of parallel agent dispatch + post-processing; agents took 4–22 minutes each.
  • Verification: F-Q-001 (manager-core cross-crate behavior import) confirmed via grep. F-T-001 (Rules of Hooks) confirmed by reading clients/typescript/src/react/index.ts. F-P-001 (users::list N+1) confirmed at users_service.rs:466-471. F-P-003 (pool=10) confirmed at picloud/src/lib.rs:583-588. F-S-001 (unbounded queue payload) confirmed at queue_service.rs:33-92. F-U-002 (queue drilldown 404) confirmed via grep against the routes tree.
  • Coverage gaps:
    • The dashboard/tests/e2e/ Playwright suite was out of scope per the brief (149 known errors from missing @playwright/test).
    • The clients/typescript/ package got a shallower pass than the Rust crates — appropriate since it's a deliberately minimal surface per v1.1.6 hybrid-model decision.
    • The orchestrator did not run the test suites; subagent line-number citations are accurate at audit time but may drift if files change.
    • SQL query plans were not measured — performance findings are static-analysis based, citing the predicate-vs-index mismatch shape rather than EXPLAIN output.
    • Caddyfile + docker-compose configurations not audited (the brief focused on application code).
    • crates/picloud-cli/ got a light pass (mainly the #[ignore]'d tests count).
  • Notes to next reviewer:
    • The Critical (F-Q-001) is structural and load-bearing for the cluster-mode roadmap (v1.3+) — addressing it is a v1.2+ release in its own right, not a v1.1.10 patch.
    • The High-severity performance findings (F-P-001..F-P-009) cluster around two themes: (1) auth middleware doing Argon2id on the async runtime per request, (2) N+1 / OFFSET / COUNT(*) anti-patterns that an instance with real traffic will feel immediately. A focused "perf clean-up" release (call it v1.1.10) could land 6–8 of these as a single bundled PR.
    • F-S-001 (unbounded payloads) is a one-PR fix that hardens four services at once; consider doing it before any external preview.
    • The dashboard CSS-variable issue (F-U-004) is one global theme-token fix that closes 5+ pages worth of light-on-dark rendering.
    • The TypeScript client useEndpoint (F-T-001) is a public API correctness bug — fix or undocument before any 1.x stability claim on the client.
Reference in New Issue View Git Blame Copy Permalink