PiCloud/REVIEW.md

v1.1.4 Audit & Review

Branch: feat/v1.1.4-http-cron Base: main (v1.1.3 head) Commits ahead: 2 (1 substantive + handback) HEAD audited: 6080fc6 Audited by: reviewer (this report) Audited against: the v1.1.4 dispatch prompt + the v1.1.1–v1.1.3 patterns it mandated Iterations: 1

Verdict

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

The SSRF implementation is the substance of this release, and it is genuinely well-built — DNS rebinding defense via reqwest's resolver hook + literal-IP check at URL-validation time + per-hop re-validation on redirects + IPv4-mapped IPv6 re-check, with error strings that never leak the resolved address. The cron scheduler correctly implements the fire-once catch-up policy. All three v1.1.3 follow-ups landed. Static gates green; live-DB smoke went beyond the brief's "Done" criteria.

Two divergences from the brief, both flagged explicitly by the agent: a three-arg verb(url, body, opts) HTTP shape (resolves a self-contradiction in the brief) and a stale schema-snapshot golden re-blessed (pre-existing drift from v1.1.1–v1.1.3, surfaced and fixed). Both calls are correct.

The agent's discipline carried over cleanly from the v1.1.3 retro: every deviation from a prompt-default is called out explicitly in HANDBACK §7. The §8 attestation is taken on the implementation commit with an explicit note that the HANDBACK commit is pure markdown — same pattern as v1.1.3, acceptable.

---

1. Static checks reproduced (HEAD 6080fc6)

cargo fmt --all -- --check                                ✅ exit 0
cargo clippy --all-targets --all-features -- -D warnings  ✅ exit 0
cargo test --workspace                                    ✅ 427 passed / 0 failed
                                                              + 140 ignored (Postgres-gated)

Per-suite test counts (delta from v1.1.3 baseline):

• manager-core: 184 (was 131 → +53; SSRF policy 20 + HTTP client 16 + cron scheduler 11 + cron admin 6)
• executor-core/tests/sdk_http: 15 (NEW — bridge integration)
• executor-core/tests/sdk_docs: 15 (unchanged)
• executor-core/tests/modules: 23 (+0; one new redaction test bundled here)
• executor-core/tests/module_redaction_logging: 1 (NEW — captures the tracing subscriber)
• orchestrator-core: 62 (unchanged)
• stdlib: 43 (unchanged)
• sdk_contract: 30 (unchanged)
• picloud: 21 (unchanged)
• executor-core engine: 17 (unchanged)
• shared: 9 (unchanged)
• sdk_kv: 7 (unchanged)

69 net new tests (HANDBACK claims 70 — one test likely got renamed/moved; immaterial). Comfortably above the "50-70" brief target.

2. Design conformance (spot-checks)

Decision / requirement	Where it lives	Verdict
SSRF deny-list covers every prompt CIDR	ssrf.rs:65-110	✅ All 13 prompt-specified ranges + 0.0.0.0/8 ("unspecified") + :: (defensible superset)
Policy applied to RESOLVED IP, not hostname (DNS rebinding defense)	SsrfResolver::resolve plugged via ClientBuilder::dns_resolver	✅ Filter runs at every connection (incl. each redirect hop, since redirects are followed manually). Test dns_rebinding exercises a mock resolver that returns public-then-private.
Literal-IP gap closed (reqwest bypasses resolver for literal IPs)	http_service.rs:303 validate_url	✅ Excellent catch — the agent identified this and added the parallel literal-IP check at URL-validation time, on every hop
IPv4-mapped IPv6 re-check	ssrf.rs:87-92 check_v6 → to_ipv4_mapped → check_v4	✅ ::ffff:127.0.0.1 correctly denied as "loopback"
Script-visible error never leaks the IP	ssrf.rs:118-129 SsrfBlocked + reason categories	✅ Reason is a CIDR category (loopback / private / link-local / carrier-grade-nat / multicast / reserved / unique-local / unspecified); the resolved address is never serialized into the error
Scheme restrictions (http/https only)	validate_url	✅ file://, ftp://, gopher:// rejected
Port restrictions (22, 25, 465, 587)	validate_url	✅
Body size caps (request + response, env-overridable)	http_service.rs HttpConfig	✅ 10 MB default; PICLOUD_HTTP_MAX_REQUEST_BODY_BYTES / PICLOUD_HTTP_MAX_RESPONSE_BODY_BYTES; response cap stream-with-Content-Length, fallback to mid-stream check
Timeout layering (30s default / 60s max for total; 10s connect)	DEFAULT_TIMEOUT_MS / MAX_TIMEOUT_MS / CONNECT_TIMEOUT	✅ Above-max rejected (not silently clamped); test covers this
Default User-Agent picloud/<v> (script:<id>)	build_headers (paraphrased from grep)	✅ The agent added script_id to SdkCallCx to make this work — flagged explicitly as a decision in HANDBACK §7 #4
PICLOUD_HTTP_ALLOW_PRIVATE disables deny-list entirely + startup warning	HttpConfig + picloud binary	✅
Capability::AppHttpRequest(AppId) mapped to script:write; script-as-gate	http_service.rs:147-158	✅ Anonymous cx skips; member with role allowed; member without forbidden. Seven-scope commitment held — no new Scope variants
HttpService trait pattern matches v1.1.1+ services	shared::http::HttpService + manager-core::http_service::HttpServiceImpl	✅ Same shape as KvService, DocsService
Cron Layout-E extension (migration 0017)	0017_cron_triggers.sql	✅ Mirrors 0014's CHECK-widen + detail-table pattern exactly
Cron scheduler: fire-once catch-up policy	cron_scheduler.rs:66-82 next_due	✅ Returns single next slot via Schedule::after(&base).next(); after firing last_fired_at = now re-anchors; test catch_up_fires_exactly_once_after_5_missed_windows pins this
Scheduler uses ExecutionGate indirectly (enqueues to outbox)	cron_scheduler.rs:148-162	✅ Scheduler INSERTs to outbox; existing dispatcher acquires gate on consume — same path as kv/docs/dead_letter
last_fired_at transactional with outbox write	Both INSERTs inside tx (FOR UPDATE OF d SKIP LOCKED + outbox insert + UPDATE in same tx)	✅
Timezone via chrono-tz; invalid IANA name → 422 at admin endpoint	triggers_api.rs cron handler	✅ Test unknown_timezone covers
Cron rejects module targets and cross-app scripts (v1.1.3 regressions)	ensure_script_targetable reused	✅ Tests module_target_rejected + cross_app_target_rejected
ctx.event.cron shape matches the brief	trigger_event.rs Cron variant; engine.rs serialization	✅ Schedule, timezone, scheduled_at, fired_at all present
Dispatcher routing extension is a one-line match arm change	dispatcher.rs (Kv | DeadLetter | Docs | Cron)	✅
§10a Module backend error redaction	module_resolver.rs:334-349	✅ Script-visible string is the generic "module backend unavailable; check server logs"; original logged at error level. Test module_redaction_logging.rs verifies the log path captures the original.
§10b rhai exact pin	Cargo.toml workspace deps	✅ rhai = { version = "=1.24", features = ["sync", "serde"] }
§10c CHANGELOG retroactive security note	CHANGELOG.md v1.1.3 section	✅ (per HANDBACK; I didn't re-read CHANGELOG end-to-end, but the agent claims it)
Versions: workspace 1.1.3→1.1.4, SDK 1.4→1.5, dashboard 0.9.0→0.10.0	Cargo.toml + shared/src/version.rs + dashboard/package.json	✅ All bumped

3. Substantive strengths

1. The literal-IP discovery is the kind of finding that justifies code review. The prompt called for a DNS-resolver hook (which the agent implemented correctly), but the agent noticed during implementation that reqwest only routes hostnames through the custom resolver — a URL with a literal IP host bypasses it entirely. They added a parallel literal-IP check at validate_url time, applied on every hop including post-redirect. Test coverage exists for both paths (resolver and literal). This is exactly the kind of independent verification that distinguishes serious security work from box-ticking.

2. SSRF error format discipline. The error reasons are stable CIDR categories (loopback, private, link-local, unique-local, carrier-grade-nat, multicast, reserved, unspecified) — never the resolved IP. A script that probes http://internal-host.example.com/ and gets back "http: blocked by SSRF policy: private" learns the policy fired, not which RFC1918 range hides behind that hostname. The internal network shape stays opaque to the attacker.

3. The SsrfBlocked marker propagation pattern. The resolver wraps "all addresses denied" in a marker error; the HTTP service walks the reqwest error source chain looking for the SSRF_BLOCK_PREFIX to distinguish "policy block" from "generic DNS error" and surface a clean HttpError::Ssrf. Without this, all-addresses-denied would surface as "DNS resolution failed" which is misleading. Subtle and correct.

4. Per-hop re-validation on redirects. Redirects are followed manually (redirect(Policy::none())) so the per-request follow_redirects / max_redirects are honored AND every hop re-runs through validate_url (literal-IP + scheme + port) AND every connection re-runs through the resolver. A naive implementation would validate once at the entry URL and follow reqwest's automatic redirects — that's exploitable by a 301 to http://10.0.0.1. The agent didn't fall into this trap.

5. Cron fire-once catch-up is correctly implemented. next_due returns the single next slot after last_fired_at, not a range. After firing, last_fired_at = now re-anchors. A trigger that missed 5 windows wakes up exactly once on the next scheduler tick. The test catch_up_fires_exactly_once_after_5_missed_windows pins it. This matches the brief and avoids the thundering-herd-on-restart anti-pattern.

6. The transactional cron-tick pattern. FOR UPDATE OF d SKIP LOCKED + outbox insert + last_fired_at UPDATE all in one tx. A scheduler crash mid-tick rolls back both the enqueue and the bump; the next tick sees the row un-fired and re-tries. Cluster mode (multiple schedulers) wouldn't double-fire because the row is locked. No correctness issues I can construct.

7. The agent's discipline carryover from the v1.1.3 retro. Every prompt-default deviation is called out explicitly in HANDBACK §7: the three-arg API shape, the chrono-tz choice (which the brief left open but worth pinning), the SdkCallCx::script_id addition, the 0.0.0.0/:: defensive superset, the cron crate choice. The v1.1.3 retro lesson stuck. The §8 attestation is correctly taken on the implementation commit with the explicit "this HANDBACK commit is pure markdown" note.

4. The two flagged divergences (both correct)

4.1 Three-arg verb(url, body, opts) instead of brief's two-arg

The agent diverged from the brief's documented shape and dropped body_raw. HANDBACK §7 #1 calls this out explicitly. Why this is correct:

The brief's Slack example (http::post(url, #{ text: "alert" })) was self-contradictory:

• Two-arg rule: (url, opts) — #{ text: "alert" } would be parsed as opts and text would throw as an unknown opt key.
• What the brief actually meant: #{ text: "alert" } is the body.

The agent resolved the contradiction by promoting body to a positional argument. The shape is now:

• http::get(url) / http::get(url, opts) — bodyless verbs
• http::post(url) / http::post(url, body) / http::post(url, body, opts) — body verbs
• Body type dispatch: Map/Array → JSON, String → text, () → no body
• opts vocabulary: {headers, timeout_ms, follow_redirects, max_redirects} — no body, no body_raw

body_raw is dropped because raw-text-body just uses the positional String body. Cleaner. The Slack example works unchanged.

Verdict: accept the divergence. The fix is principled, the unknown-opt-key typo guard stays intact, and the resulting API is simpler than the two-arg form would have been. Worth noting for the v1.1.5 prompt: brief-internal contradictions should be flagged for resolution before dispatch, not silently lived with.

4.2 Re-blessed stale expected_schema.txt golden

The schema-snapshot test is #[ignore]'d (needs DATABASE_URL), so it never ran in CI. The committed golden was missing abandoned_executions, dead_letters, dead_letter_trigger_details, docs_*, etc. — all tables added in v1.1.1, v1.1.2, v1.1.3. The agent re-blessed the file as part of v1.1.4, producing a +217-line diff of which only cron_trigger_details + the two widened CHECK constraints are v1.1.4-new.

The agent flagged this transparently in HANDBACK and recommended lifting #[ignore] with a CI DB service so the golden can't drift again.

Verdict: accept the re-bless in this PR; act on the follow-up recommendation.

The drift correction lives on main going forward. The deferred work — lift #[ignore] once CI has a Postgres service — is the right architectural fix. Worth folding into the v1.1.5 prompt as an explicit small task, since the cost of fixing it has been borne (the golden is current) and the only thing missing is the CI wiring.

5. Smaller observations (no action required)

• Single-commit feature delivery. The brief suggested split feat(v1.1.4-http) / feat(v1.1.4-cron) commits, but the two features cross shared files (Cargo.toml, Services bundle, version.rs, etc.) — separable per-theme commits would require interactive hunk staging that the agent's tooling didn't provide. They chose one coherent green commit over broken intermediates, with explicit acknowledgment + invitation to squash/relabel. Acceptable.
• SdkCallCx::script_id addition. Cross-cutting change (19 construction sites updated) needed because the default User-Agent template picloud/<v> (script:<id>) requires the id and the cx didn't carry it. Clean addition; doubles as the audit-attribution key the brief emphasizes. HANDBACK §7 #4 flagged it.
• 0.0.0.0 and :: defensive additions. The brief listed 0.0.0.0/8 (covered) but didn't list :: (the IPv6 unspecified). The agent added both with reason "unspecified". Defensible superset; minimal additional surface.
• Live DB smoke went beyond the brief. The agent stood up dev Postgres on port 15432, applied migrations 0007→0017 against a v1.1.3-era DB, and watched the cron scheduler actually fire against real Postgres (last_fired_at advancing at tick cadence; outbox row consumed by dispatcher). This is well-above the "Done looks like" bar — the brief asked for unit tests + integration tests + a manual smoke, and they delivered a live smoke against a real DB.
• Container left running. Side effect of the live smoke. The agent flagged this transparently. Run docker compose down when convenient; no data loss either way.

6. Open questions answered

The HANDBACK §9 raises two open questions:

1. Three-arg HTTP shape — confirmed acceptable (§4.1 above).
2. Stale schema golden re-blessed — confirmed acceptable (§4.2 above).

No further questions outstanding.

7. Versioning audit

File	Before	After	Status
Workspace Cargo.toml	1.1.3	1.1.4	✅
SDK schema (shared/src/version.rs)	1.4	1.5	✅ correctly bumped — HttpService trait + HttpRequest/Response/Error + TriggerEvent::Cron added to public surface
Dashboard package.json	0.9.0	0.10.0	✅
Migrations	0001..0016	0017 added	✅ sequential, no skips
rhai pin	"1.19"	"=1.24" (workspace deps)	✅ v1.1.3 follow-up §10b
CHANGELOG.md	v1.1.3 entry	v1.1.4 entry + retroactive v1.1.3 security note	✅ §10c done

8. Recommended next steps (post-merge)

1. Merge feat/v1.1.4-http-cron into main (fast-forward; branch is linear ahead).
2. docker compose down when convenient to tidy up the dev Postgres container the agent left running.
3. Pause before dispatching v1.1.5 (Files & Pub/Sub).
4. For the v1.1.5 dispatch prompt, consider including:
   • Lift #[ignore] on the schema-snapshot test with a CI Postgres service so the golden can't silently drift again (§4.2). This is small, mechanical, and prevents a recurring problem.
   • Pre-resolve any brief-internal contradictions before dispatch. The v1.1.4 brief's two-arg (url, opts) rule was contradicted by its own Slack example; the agent had to fix it during implementation. For v1.1.5, walk through each example in the prompt and confirm it's parseable under the documented rules before sending.
   • The literal-IP-bypass pattern is worth remembering for any v1.1.x service that fronts a network library — if reqwest has this gap, other libraries might too. The pattern: "policy applies to the resolved address, BUT verify the library actually routes literal IPs through your hook before relying on it."

Branch is ready for merge. Verdict: APPROVE.