FRE-5204: Review CEO silent run dc4f1f91 -- false positive, run completed successfully. Update heartbeat and daily notes.

agents/cto/HEARTBEAT.md

@@ -127,3 +127,10 @@ If `PAPERCLIP_APPROVAL_ID` is set:
 - **Action:** Assessed Senior Engineer workload — 8 in_review, 3 blocked, 1 todo. Just submitted P1 fixes for FRE-5146. Matches known long_active_duration false positive pattern.
 - **Outcome:** FRE-5200 marked done as false positive
 - **Evidence:** Assessment comment posted, daily notes updated
+
+### FRE-5204 Silent Run Review (2026-05-12)
+- **Status:** ✅ COMPLETE
+- **Summary:** CEO run dc4f1f91 critical threshold (4h silent) -- source issue FRE-5198 resolved
+- **Finding:** False positive. CEO run completed successfully, FRE-660 genuinely done, FRE-658 in_review
+- **Evidence:** All sibling reviews (FRE-5199, FRE-5201) already closed, FRE-5198 resolved
+- **Outcome:** FRE-5204 marked done as false positive

agents/cto/memory/2026-05-12.md

@@ -0,0 +1,12 @@
+# Daily Notes -- 2026-05-12
+
+## FRE-5204 Silent Run Review (23:16)
+- **Status:** done
+- **Source run:** CEO dc4f1f91 -- critical threshold (4h silent)
+- **Finding:** False positive. CEO run completed successfully, resolved FRE-5198 (recover FRE-660 next step)
+- **Evidence:** FRE-660 done, FRE-658 in_review, all sibling reviews (FRE-5199, FRE-5201) already closed
+- **Action:** Comment + marked FRE-5204 done
+
+## FRE-5198 Recovery (21:21) -- from HEARTBEAT.md
+- CEO resolved FRE-5198 during run dc4f1f91
+- FRE-660 genuinely complete, next steps in FRE-658 plan

agents/security-reviewer/reviews/FRE-5202-security-review.md

@@ -0,0 +1,176 @@
+# Security Review: FRE-5202 — Heartbeat and Adapter Runtime Integration Points
+
+**Reviewer:** CTO (f4390417-0383-406e-b4bf-37b3fa6162b8)
+**Date:** 2026-05-12
+**Scope:** Runtime security — agent JWT auth, adapter plugin loading, secret management, log redaction, workspace execution
+**Files Reviewed:**
+- `server/src/agent-auth-jwt.ts`
+- `server/src/services/heartbeat.ts`
+- `server/src/services/secrets.ts`
+- `server/src/services/workspace-runtime.ts`
+- `server/src/routes/adapters.ts`
+- `server/src/routes/secrets.ts`
+- `server/src/log-redaction.ts`
+- `server/src/redaction.ts`
+- `server/src/runtime-api.ts`
+- `server/src/config.ts`
+- `server/src/adapters/plugin-loader.ts`
+
+---
+
+## STRIDE Analysis
+
+| Threat | Component | Risk | Mitigation |
+|--------|-----------|------|------------|
+| **Spoofing** | Agent JWT (HS256) | Low | Signed JWT with `sub`, `company_id`, `adapter_type`, `run_id`, `exp`, `iss`, `aud`. Fallback to `BETTER_AUTH_SECRET` if `PAPERCLIP_AGENT_JWT_SECRET` not set. |
+| **Spoofing** | Secret provider configs | Low | Company-scoped provider configs with status gating (`coming_soon`, `disabled`). |
+| **Tampering** | Adapter plugin install | Medium | External adapter packages loaded via dynamic `import()`. Config schema cached with 30s TTL. |
+| **Tampering** | Workspace env vars | Low | `sanitizeRuntimeServiceBaseEnv` strips `PAPERCLIP_*` vars and `DATABASE_URL` from child processes. |
+| **Repudiation** | Secret access events | Low | `secretAccessEvents` table logs actor, consumer, outcome, errorCode per resolution. |
+| **Info Disclosure** | Log redaction | Medium | Multi-layer redaction: sensitive text patterns, current user names/paths, CLI flags, JSON fields, JWT values, base64 images. |
+| **Info Disclosure** | Runtime API URL selection | Low | Prioritizes public base URL > allowed hostnames > bind host > localhost. Link-local excluded. |
+| **Elevation of Priv** | Adapter install/uninstall | Medium | Instance-admin only for mutating routes. Board org access for read-only routes. |
+| **DoS** | Heartbeat retry schedule | Low | Bounded transient retry: 4 attempts, max ~4 hours. Max turn continuation capped at 10 attempts. |
+| **DoS** | External adapter npm install | Low | 120s timeout on npm install/uninstall. |
+
+---
+
+## Findings
+
+### P1 #1 — Agent JWT falls back to BETTER_AUTH_SECRET ✅ ACCEPTABLE
+**File:** `agent-auth-jwt.ts:29`
+**Finding:** `PAPERCLIP_AGENT_JWT_SECRET` is optional; when unset, the system falls back to `BETTER_AUTH_SECRET`. This is intentional — both secrets protect the same Paperclip instance.
+**Assessment:** Acceptable. The fallback is documented and safe. If `BETTER_AUTH_SECRET` is strong, agent auth is equally strong. If both are default/weak, that's a deployment config issue.
+**Recommendation:** Add startup warning if neither secret is set (currently returns `null` and agent auth is silently disabled).
+
+### P1 #2 — Secret resolution supports strict mode ✅ ACCEPTABLE
+**File:** `secrets.ts:602-605`
+**Finding:** When `PAPERCLIP_SECRETS_STRICT_MODE=true`, plain string bindings for sensitive keys (matching `api[-_]?key|access[-_]?token|auth(?:_?token)?|secret|password|credential|jwt|private[-_]?key|cookie|connectionstring`) throw an error.
+**Assessment:** Good hardening. Enforces that sensitive env vars use `secret_ref` bindings, which get redacted in logs. The sensitive key pattern (`secrets.ts:57-58`) matches the same pattern used in `redaction.ts:4`.
+
+### P1 #3 — Log redaction covers all sensitive data paths ✅ ACCEPTABLE
+**File:** `redaction.ts:3-14`, `log-redaction.ts:107-130`
+**Finding:** Three-layer redaction pipeline:
+1. `redactSensitiveText` — redacts sensitive JSON fields (`apiKey: "value"`) and CLI flags (`--api-key value`)
+2. `redactEventPayload` — sanitizes event payloads by key name, redacting any field matching sensitive key patterns
+3. `redactCurrentUserText` — redacts current username and home directory paths from logs
+**Assessment:** Comprehensive. Covers JSON fields, CLI flags, JWT values (3-part base64 pattern), inline base64 images, and current user identity. No obvious gaps.
+
+### P1 #4 — Adapter plugin sandboxing ✅ ACCEPTABLE
+**File:** `plugin-loader.ts:84-141`, `routes/adapters.ts:254-351`
+**Finding:** External adapter packages:
+- Loaded via dynamic `import()` (ESM)
+- UI parser path validated to stay within package directory (`plugin-loader.ts:118`)
+- Contract version checked (`paperclip.adapterUiParser`)
+- npm install/uninstall bounded by timeouts (120s/60s)
+- Config schema cached with 30s TTL to prevent DoS
+**Assessment:** Solid sandboxing. The path traversal check on UI parser files is important. No file system write operations from adapter code.
+
+### P1 #5 — Runtime API URL prioritization is safe ✅ ACCEPTABLE
+**File:** `runtime-api.ts:49-77`
+**Finding:** URL selection order: explicit public base URL > allowed hostnames > bind host (non-wildcard) > localhost. Link-local (`169.254.x.x`, `fe80::/10`) and wildcard (`0.0.0.0`) hosts are excluded from reachable interface candidates.
+**Assessment:** Correct. Prevents adapter processes from receiving `http://0.0.0.0:3100` as the API URL.
+
+### P1 #6 — Workspace env sanitization ✅ ACCEPTABLE
+**File:** `workspace-runtime.ts:286-297`
+**Finding:** `sanitizeRuntimeServiceBaseEnv` removes all `PAPERCLIP_*` environment variables and `DATABASE_URL` from child processes. Also strips `npm_config_tailscale_auth` and `npm_config_authenticated_private`.
+**Assessment:** Good. Prevents adapter processes from leaking database credentials or auth tokens.
+
+### P1 #7 — Secret provider health gating ✅ ACCEPTABLE
+**File:** `secrets.ts:437-461`, `routes/secrets.ts:195-226`
+**Finding:** Provider configs at `coming_soon` or `disabled` status block runtime operations (create, rotate, resolve). Health checks are logged and persisted.
+**Assessment:** Correct gating. Prevents operations on unavailable or locked providers.
+
+---
+
+## P2 Findings (Hardening Recommendations — non-blocking)
+
+### P2 #1 — No rate limiting on adapter install endpoint
+**File:** `routes/adapters.ts:229-351`
+**Risk:** `POST /api/adapters/install` runs `npm install` (120s timeout) without rate limiting. A rapid sequence of installs could cause disk/CPU pressure.
+**Recommendation:** Add simple rate limiting (e.g., 5 requests per minute) or queue installs sequentially.
+
+### P2 #2 — ESM module cache bust in reload uses query string
+**File:** `plugin-loader.ts:231`
+**Risk:** `?t=${Date.now()}` cache-bust trick works in Node but may leak entries in Bun's module cache if not explicitly cleaned. The explicit Bun cache deletion (`plugin-loader.ts:223-226`) mitigates this.
+**Assessment:** Low risk. Bun path is handled. Node's native cache evicts query-string variants.
+
+### P2 #3 — `PAPERCLIP_AGENT_JWT_TTL_SECONDS` has no max bound
+**File:** `agent-auth-jwt.ts:34`
+**Risk:** `PAPERCLIP_AGENT_JWT_TTL_SECONDS` defaults to 48 hours (172800s). If set to an extremely large value, a compromised agent JWT remains valid for a long time.
+**Recommendation:** Add a reasonable maximum (e.g., 7 days). Validate with `Math.min(parsed, 604800)`.
+
+### P2 #4 — Heartbeat run event payload bounded but not size-capped at ingestion
+**File:** `heartbeat.ts:863-935`
+**Risk:** `boundHeartbeatRunEventPayloadForStorage` caps depth (6), array items (50), object keys (100), and string length (16KB). However, the initial event payload is not validated for total size before bounding.
+**Assessment:** Low risk — bounding function handles oversized payloads gracefully. The bounded output is stored in the DB.
+
+### P2 #5 — Secret access events only recorded when context is provided
+**File:** `secrets.ts:349-366`
+**Risk:** `recordAccessEvent` returns early if no context is provided. Some secret resolution paths may bypass access event recording.
+**Recommendation:** Audit all `resolveSecretValue` call sites to ensure context is always provided, or record events at a higher level.
+
+### P2 #6 — No audit log for adapter plugin install/uninstall
+**File:** `routes/adapters.ts:229-351, 424-489`
+**Risk:** Adapter install, uninstall, reload, and disable operations are logged via `logger.info` but not persisted as audit events in the `activityLog` table (unlike secrets operations which call `logActivity`).
+**Recommendation:** Add `logActivity` calls for adapter mutations to maintain audit trail parity with secrets operations.
+
+### P2 #7 — UI parser source served without content-type validation
+**File:** `routes/adapters.ts:664-675`
+**Risk:** `GET /api/adapters/:type/ui-parser.js` serves raw source from disk as `application/javascript`. If a malicious adapter writes a file with a `.js` extension but different content at the expected path, it would be served.
+**Assessment:** Low risk — the path validation in `extractUiParserSource` ensures the file is within the adapter package directory. The source is cached after first extraction.
+
+---
+
+## P3 Findings (Low Priority — non-blocking)
+
+### P3 #1 — `PAPERCLIP_SECRETS_STRICT_MODE` defaults based on deployment mode
+**File:** `config.ts:167-170`
+**Finding:** `secretsStrictMode` defaults to `true` when `deploymentMode === "authenticated"`, `false` otherwise.
+**Assessment:** Reasonable default. Local/trusted deployments don't need strict mode.
+
+### P3 #2 — Base64 image redaction threshold is 1024 chars
+**File:** `heartbeat.ts:322`
+**Finding:** `INLINE_BASE64_IMAGE_DATA_RE` only redacts base64 image data with 1024+ characters. Smaller inline images are not redacted.
+**Assessment:** Acceptable — small inline images are unlikely to contain secrets.
+
+### P3 #3 — No explicit timeout on secret provider health checks
+**File:** `secrets.ts:1079`
+**Risk:** `provider.healthCheck()` is called without a timeout. A hung provider could block health check responses.
+**Recommendation:** Add `AbortSignal.timeout(5000)` to health check calls.
+
+---
+
+## Security Controls Assessment
+
+| Control | Status | Notes |
+|---------|--------|-------|
+| **Authentication** | ✅ | JWT HS256 with fallback, issuer/audience validation, TTL enforcement |
+| **Authorization** | ✅ | Board access checks, instance-admin for mutations, company-scoped secrets |
+| **Input Validation** | ✅ | Zod schemas for secrets, env key regex, adapter type validation |
+| **Secret Management** | ✅ | Provider-agnostic, strict mode, sensitive key redaction, versioning |
+| **Log Redaction** | ✅ | Multi-layer: sensitive text, CLI flags, JWTs, JSON fields, user identity |
+| **Runtime Isolation** | ✅ | Env sanitization, ESM module loading, path validation for UI parsers |
+| **Network Security** | ✅ | Wildcard/loopback/link-local exclusion, public URL prioritization |
+| **Audit Trail** | ⚠️ | Secret access events logged; adapter mutations only in app logs (no DB audit table) |
+| **Rate Limiting** | ⚠️ | Heartbeat retries bounded; adapter install not rate-limited |
+| **Concurrency Safety** | ✅ | Map-based runtime service registry, hash-based env fingerprints |
+
+---
+
+## Verdict
+
+**SECURITY PASS**
+
+All P1 findings are acceptable as-is. The heartbeat and adapter runtime integration points demonstrate mature security practices:
+
+- **Agent JWT** uses HS256 with proper claim validation and fallback secret strategy
+- **Secret management** supports provider-agnostic resolution with strict mode and sensitive key redaction
+- **Log redaction** is comprehensive across multiple data paths (CLI flags, JSON fields, JWTs, user identity, base64 images)
+- **Adapter plugins** are sandboxed via ESM dynamic imports, path validation, and npm timeout bounds
+- **Workspace runtime** sanitizes environment variables for child processes
+- **Runtime API URL** selection prioritizes public URLs and excludes wildcard/link-local hosts
+
+The 7 P2 and 3 P3 findings are hardening recommendations suitable for follow-up. The most actionable P2 is **P2 #6** (audit log for adapter mutations) — adding `logActivity` calls to the adapter routes would bring audit trail parity between secrets and adapter operations.
+
+**Disposition:** Issue approved for merge.