coreci-chat/.ciagent/GRILL.md

GRILL.md — M1 Plan Adversarial Review

Reviewer: CIAgent griller (red-team persona) Subject: .ciagent/PLAN.md — M1 plan (5 waves A–E + final phase) Scope: 17 M1 REQs (001–014, 038, 039, 040) + 4 PO high-stakes claims Date: 2026-08-24 Method: 9-axis adversarial review with binding verdicts. Confidence ≥ 0.60 = binding; < 0.60 = escalate.

---

Verdict Summary

Axis	Verdict	One-line rationale
1. Feasibility	PASS	Each wave is a coherent vertical slice; Go binary + install script + WS server are well-trodden territory; no wave implies unsolved tech.
2. Scope	PASS-WITH-FIXES	Plan stays within the 17 M1 REQs, but the /api/byom/test-inference endpoint and the runtime health-check audit task are un-spec'd scope additions that need explicit PO acknowledgment.
3. Cost	PASS-WITH-FIXES	Decomposition is efficient and rework-minimizing, but Wave A ships Trigger.dev bootstrap (M3 infra) and Wave D ships an SSH whitelist hook with no caller in M1 — both are deliberate pre-investments that must be explicitly logged as debt-for-future-value, not hidden as "M1 work."
4. Dependencies	PASS-WITH-FIXES	Ordering is correct, but Wave D's WS server depends on Wave B's /api/relay/issue-token (auth token issuance) and the plan admits B and D must "coordinate the contract in the plan" — that contract is not specified here, creating a cross-wave coupling risk.
5. Testability	PASS	Every M1 REQ maps to ≥1 must-have pass/fail item; the 4 review deliverables are producible; coverage gate ≥80% is explicit.
6. Security	PASS-WITH-FIXES	RLS, audit REVOKE, secret provider, RBAC-from-first-endpoint, and SSH whitelist are sound patterns, but the per-tenant hash-chain has a chain-verification gap for concurrent writers, and the "no SSH execution in M1" whitelist hook can't be integration-tested against a real exec path — only unit-tested.
7. Architecture drift	PASS	Plan is faithful to ARCHITECTURE.md + all 5 CLARIFY decisions; no contradictions found.
8. Requirements coverage	PASS-WITH-FIXES	All 17 REQs have tasks + must-haves, but REQ-026 is listed as "whitelist hook only" in Wave D while its acceptance criteria (spec §4) describe full SSH-key auth + whitelist execution — the M1/M2 split is underspecified and the plan leans on a parenthetical, not a contract.
9. Operational readiness	PASS-WITH-FIXES	The M1 gate (spec §2.3) will pass and all 4 review deliverables are producible, but the install logs deliverable requires 3 OSes (Ubuntu 24.04, Debian 12+, unsupported) and the test strategy only lists 3 CI containers — Fedora as the "unsupported" case is an assumption, not a spec mandate; an unsupported-OS matrix needs explicit sign-off.

Final Verdict: PASS-WITH-FIXES — The plan is sound and shippable. The fixes below are binding; none warrant a FAIL (escalation), but each must be resolved before the wave it touches ships.

---

Axis 1: Feasibility

Verdict: PASS

Each wave is a vertical slice of known-complexity work. Wave A (Postgres RLS + append-only audit + secret provider + Trigger.dev bootstrap) is the densest but is well-researched (RESEARCH.md R-001/003/004) with concrete patterns. Wave D (Go binary + install script + WebSocket + whitelist) is the most heterogeneous but the Go persona is correctly scoped (PERSONAS.md) and gorilla/websocket + systemd is commodity. No wave implies an unsolved technical problem or a "learn as we go" risk on the delivery path. The one feasibility flag — Trigger.dev bootstrap with no tasks in M1 — is explicitly a no-op health check, which is the right de-risking choice.

---

Axis 2: Scope

Verdict: PASS-WITH-FIXES

The plan maps cleanly to the 17 M1 REQs; no M2 (REQ-015–027) or M3 (REQ-028–037, 041–044) work is silently included. The out-of-scope list (REQUIREMENTS.md §Out of Scope) is respected. However, two un-spec'd scope additions exist in the plan and should be made explicit rather than smuggled in:

1. POST /api/byom/test-inference (Wave C, Task 3) is not in spec §4. Spec REQ-008 says "100% of LLM inference calls are sent to the configured BYOM endpoint (verified via outbound traffic log)" — in M1 there is no chat/orchestration to drive inference. The plan invents a test endpoint to prove REQ-008 without M3. This is a reasonable proxy, but it is a new surface and should be flagged as a plan-time scope addition, not implied by REQ-008.
2. The Trigger.dev runtimeHealthCheck task that "appends an audit entry every 5 min" (Wave A, Task 7; R-001) writes synthetic audit entries with no business event behind them. REQ-038 lists "prompt, tool call, SSH command, response" as auditable events — a health-check tick is none of those. This pollutes the audit store with non-spec'd events and sets a precedent that "anything can append to audit_log."

Required fixes:

1. Add a one-line note to Wave C Task 3 that /api/byom/test-inference is a plan-time proxy endpoint to satisfy REQ-008 in the absence of M3 orchestration; mark it for removal/deprecation when M3 lands. Get PO acknowledgment (non-blocking, but recorded).
2. Change the Wave A Trigger.dev health task to write to a separate runtime_health table or log, NOT audit_log. REQ-038's audit store is for business events only. If a health tick must be auditable, define a new event_type: "system.health" and add it to the spec's auditable-event list via a follow-up — do not silently widen REQ-038's scope in Wave A.

---

Axis 3: Cost

Verdict: PASS-WITH-FIXES

The decomposition is efficient: A→B/C parallel→D parallel→E is a near-critical path with real parallelism, and each wave ships a patch (releasable), avoiding a big-bang. The pre-investments are sound: shipping the whitelist hook in M1 (D-002 rationale) and the Trigger.dev runtime in M1 (R-001) are explicitly to avoid M2/M3 rewrites — this is the right trade. But the plan presents these as M1 deliverables without quantifying the cost-vs-future-value, which is exactly how "we'll add it later" debt gets hidden:

1. Wave A's Trigger.dev bootstrap (Task 7) is M3 infrastructure shipped in M1. It has no M1 caller. Its only M1 value is "proves the runtime works." That's a spike, not a deliverable — and spikes belong on a spike line, not the M1 acceptance gate.
2. Wave D's SSH whitelist hook (Task 6) ships CheckCommand + whitelist JSON + unit tests with no SSH execution path in M1. This is correctly per the PO claim ("M2 plugs the adapter into the existing hook"), but it means M1 pays the cost of designing a hook against an imaginary caller. The cost is justified if and only if M2 actually uses the hook as-shipped. The plan provides no contract guaranteeing that.

Required fixes: 3. Annotate Wave A Task 7 and Wave D Task 6 in PLAN.md as "pre-investment for M2/M3" with a one-line expected-payoff (avoids rewrite of X). This makes the cost visible in the plan rather than buried in a task list. Non-blocking, but required for audit traceability. 4. Add a binding note to Wave D Task 6: "The CheckCommand(cmd) error signature and whitelist JSON schema are the M2 SSH adapter contract. M2 must consume them as-shipped; any signature change requires a documented migration." This locks the future-value claim the plan is spending M1 cost on.

---

Axis 4: Dependencies

Verdict: PASS-WITH-FIXES

The wave ordering is correct: A is the true foundation (withTenant/audit/secrets), B and C depend only on A, D depends on A + a piece of B (auth token issuance), E depends on B + D. The parallelism diagram (PLAN.md §Wave ordering) is accurate. The gap is the one the plan itself flags but does not resolve:

• PLAN.md line 235: "D can run in parallel after A (independent of B/C; the WS server in D needs B's auth token-issuance endpoint — coordinate the contract in the plan, then D's WS server + B's token endpoint can land in the same wave window)."

This is an admission that D is not independent of B — it depends on POST /api/relay/issue-token (Wave B Task 1, wait — actually this endpoint is listed in Wave D Task 1, owned by backend-engineer). There's a territorial ambiguity: the token-issuance endpoint is in Wave D's task list (D Task 1) but the plan's parallelism note says it lives in B's window. Which wave owns the token contract? If D's go-engineer is blocked waiting on B's auth middleware to issue tokens, D does not truly parallelize.

Required fixes: 5. Resolve the Wave B / Wave D token-issuance ownership in PLAN.md: explicitly state that POST /api/relay/issue-token (currently Wave D Task 1) is owned by backend-engineer and lands in whichever wave ships first, but that the contract (token format, scope, rotation) is defined in Wave A's secrets package so neither B nor D blocks on the other's implementation. Add the contract spec (token format: JWT? opaque? lifetime?) to Wave A or Wave B as a must-have.

---

Axis 5: Testability

Verdict: PASS

Every M1 REQ has ≥1 pass/fail must-have. REQ-001/002/003/004/005 → Wave B must-haves (SSO round-trip, 403 test, role-change-enforced-next-call). REQ-006/007/008/009 → Wave C must-haves (secret-ref scan, validation green/red, 400/503 reject paths). REQ-010/011/012/013 → Wave D must-haves (3 OS install matrix, registration metadata, reconnect-backoff). REQ-014 → Wave E must-haves (green-within-90s, yellow→red aging, T1≠T2 RLS). REQ-038/039/040 → Wave A must-haves (chain verification, UPDATE/DELETE rejected, cross-tenant zero rows, secrets-not-in-DB scan). The 4 review deliverables (per-REQ report, demo, pen test, install logs) are explicitly produced in the Final Phase. Coverage gate ≥80% (spec §6) is enforced. The one soft spot — the SSH whitelist hook has no integration test against a real exec path in M1 — is captured under Axis 6, not here, because the unit testability is complete.

---

Axis 6: Security

Verdict: PASS-WITH-FIXES

The patterns are sound and match RESEARCH.md R-003/004 and CLARIFY D-003/004:

• RLS: SET LOCAL app.tenant_id per-transaction, app role non-superuser, migrator role BYPASSRLS-gated, withTenant wrapper, query-outside-wrapper throws. Correct.
• Audit: append-only audit_log, REVOKE UPDATE/DELETE from coreci_app, hash-chain curr_hash = sha256(prev_hash || canonical(payload)), constraint trigger rejects forged prev_hash, write-failure rolls back the enclosing transaction (Edge 7). Correct.
• Secrets: SecretProvider interface, AWS SM (prod) + local-encrypted (dev), DB stores only secret_ref, SecretValue.toString() returns [REDACTED], lint rule bans console.log(secret). Correct.
• RBAC: enforced at API gateway from first endpoint (GET /api/me), role→route map, 403 test for Viewer→Admin route. Correct.
• SSH whitelist: fixed file, CheckCommand parses base + args, deny list catches -exec/redirection/operators, unit tests for rm -rf/find -exec/pipe-to-nc. Correct as far as it goes.

Two gaps:

1. Per-tenant hash-chain concurrency. R-003 notes "Per-tenant chain is simpler... avoids cross-tenant ordering contention" and recommends partitioning by tenant_id or heavy indexing on (tenant_id, id). But the constraint trigger that enforces prev_hash = (last row's curr_hash for that tenant) requires reading "the last row for this tenant" — under concurrent writers in the same tenant (two simultaneous audit appends), both read the same prev_hash, both INSERT, and one's prev_hash will fail the constraint. That's correct (no corruption), but it means concurrent audit writes in one tenant will serialize-fail and roll back. For M1 volume (onboarding, dashboard) this is fine. For M3 (chat with parallel tool calls) it's a bottleneck. The plan should state this is a known M1-acceptable limitation with a documented M3 mitigation (advisory lock per tenant, or sequence-per-tenant, or accept the rollback-retry).

2. Whitelist hook has no integration test path in M1. CheckCommand is unit-tested, but the claim "Retrofit later = rewrite" (PO claim #1) rests on the hook being correct in the shape M2 will consume. With no exec path, M1 cannot prove the hook actually intercepts a real SSH command — only that it parses strings. An M2 discovery that exec.Command needs the command pre-split differently, or that the deny list misses a real-world escape (e.g., systemctl status; rm -rf / where ; is in the arg not the base), would force a rework despite the M1 pre-investment.

Required fixes: 6. Add to Wave A audit task (or RESEARCH.md R-003) an explicit note: "Per-tenant hash-chain serializes concurrent audit writes within one tenant via constraint-trigger rollback. Acceptable for M1 volume. M3 mitigation: per-tenant advisory lock (pg_advisory_xact_lock(hashtext(tenantId))) before the INSERT, or sequence-per-tenant." This documents the known limit so M3 isn't surprised. 7. Add to Wave D Task 6 a shadow integration test: a Go test that constructs an exec.Cmd from a parsed whitelist command (e.g., exec.Command("systemctl", "status", "nginx")) and asserts CheckCommand accepts it, plus a negative test that exec.Command("rm", "-rf", "/") is rejected before the Cmd would be started. This proves the hook composes with os/exec without needing a live SSH server. Closes the "M2 rework" risk the PO claim is hedging against.

---

Axis 7: Architecture drift

Verdict: PASS

Line-for-line, PLAN.md is faithful to ARCHITECTURE.md and all 5 CLARIFY decisions:

• D-001 (OpenAI-compatible BYOM): Wave C uses /v1/chat/completions. ✓
• D-002 (Go Relay Agent): Wave D ships a Go binary + systemd. ✓
• D-003 (AWS SM + local-encrypted behind interface): Wave A Task 6 ships both impls. ✓
• D-004 (Postgres append-only + hash-chain, S3 WORM deferred to M3): Wave A Task 5 matches exactly. ✓
• D-005 (Next.js App Router single SPA): Wave B/E ship dashboard in apps/control-plane/apps/dashboard. ✓
• The API-gateway-first invariant (ARCHITECTURE.md §invariants) is enforced by Wave B Task 4. ✓
• The withTenant discipline is owned by data-engineer (territory alignment matches PERSONAS.md). ✓
• No contradictions between PLAN.md, ARCHITECTURE.md, and CLARIFY.md found.

---

Axis 8: Requirements coverage

Verdict: PASS-WITH-FIXES

All 17 M1 REQs have explicit tasks and must-haves:

• REQ-001→005: Wave B. ✓
• REQ-006→009: Wave C. ✓
• REQ-010→013: Wave D. ✓
• REQ-014: Wave E. ✓
• REQ-038/039/040: Wave A. ✓

The single coverage problem is REQ-026. The plan lists "REQ-026 (whitelist hook only)" in Wave D (PLAN.md line 17, 154, 163). But spec §4 REQ-026's acceptance criteria reads: "Given a customer generates an SSH keypair... when the Relay Agent receives a tool call, then only commands on the approved whitelist are executed; non-whitelisted commands are rejected and audited." That is full SSH-key-auth + whitelist execution — which is M2 work (REQUIREMENTS.md traceability line 98 confirms "deferred (whitelist format + hook ships M1 Wave D)"). The plan's parenthetical "(whitelist hook only)" is doing a lot of load-bearing work and is the single most likely place for a scope dispute at the M1 review.

The REQUIREMENTS.md traceability table (line 98) correctly defers REQ-026 to M2 with the M1-hook note. The plan is consistent with REQUIREMENTS.md. But the spec §4 acceptance criteria for REQ-026 are NOT M1-eligible as written. This is a spec-vs-plan wording gap, not a plan defect — yet the plan inherits the ambiguity.

Required fixes: 8. Add to PLAN.md Wave D a one-line scope statement: "M1 ships REQ-026 partially: the whitelist file format + CheckCommand enforcement hook + unit tests. The spec §4 REQ-026 acceptance criteria (SSH keypair auth + tool-call-driven execution) are M2. M1's must-have is the hook + whitelist, NOT end-to-end SSH execution." This makes the partial-REQ-026 coverage explicit so the M1 review doesn't dispute it. (This is a clarification, not a spec change — the REQUIREMENTS.md traceability already says this.)

---

Axis 9: Operational readiness

Verdict: PASS-WITH-FIXES

The M1 acceptance gate (spec §2.3) will pass: the Happy Path (PLAN.md §Happy Path) walks SSO → BYOM green → install → register → green dashboard, and each step has a must-have. The 4 review deliverables (Final Phase §M1 review deliverables) are all producible:

1. Per-REQ report: each REQ has must-haves → pass/fail. ✓
2. Demo recording: Happy Path is walkable end-to-end (Wave E must-have line 197). ✓
3. Pen test: tests/pen/cross-tenant.test.ts (Wave A scaffold, Final full run). ✓
4. Install logs on 3 OSes: Wave D must-haves + CI matrix. ✓ (with the caveat below)

The gap: deliverable #4 requires install logs on "Ubuntu 24.04 (pass), Debian 12+ (pass), unsupported OS (clean abort)" (Final Phase line 217). The test strategy (PLAN.md line 244) says "CI matrix runs scripts/install.sh on Ubuntu 24.04, Debian 12, Fedora (expects abort) containers." Fedora as the representative unsupported OS is a plan choice, not a spec mandate. Spec §5 says "Install script aborts on unsupported OS" — it does not name Fedora. If the pen-test reviewer asks "did you test CentOS? RHEL? Alpine?" the plan has no answer. The "unsupported OS" deliverable is under-specified.

Required fixes: 9. Define the unsupported-OS test matrix explicitly in PLAN.md Wave D must-haves: at minimum 2 unsupported cases (e.g., Fedora + Alpine, or CentOS Stream + Arch) to prove detect_os aborts on a non-Debian-family OS and a wrong-version Debian-family OS. Single-OS "unsupported" is not sufficient evidence for the M1 review deliverable.

---

High-Stakes Claim Stress-Tests

Claim 1 — "SSH command whitelist (REQ-026) — ship the whitelist file format and the enforcement hook in the Relay Agent now. M2 plugs the adapter into the existing hook. Retrofit later = rewrite."

Verdict: CLAIM SUBSTANTIVELY MET, WITH ONE GAP.

Wave D Task 6 ships /etc/coreci/ssh-whitelist.json (versioned, with commands + arg-deny list) and apps/relay-agent/whitelist/check.go (CheckCommand(cmd) error) with unit tests. This is the file format + enforcement hook the PO demanded. The retrofit-later-rewrite risk is genuinely mitigated. The gap: with no SSH execution path in M1, the hook is only string-tested, not exec-composition-tested. If M2's exec.Command integration reveals the hook needs the command pre-split or the deny list misses real-world escapes, M2 will rework despite the M1 investment. The PO's "rewrite" framing assumes the hook is correct as-shipped; M1 cannot prove that without an exec path. Fix #7 (shadow exec.Cmd integration test) closes this. With that fix, the claim holds.

Claim 2 — "Audit log immutability (REQ-038) — append-only store from day one. The pattern you set in M1 propagates to every event in M2/M3."

Verdict: CLAIM MET, WITH ONE DOCUMENTED LIMIT.

Wave A Task 5 ships the Postgres append-only audit_log with hash-chain + REVOKE UPDATE/DELETE + constraint trigger on forged prev_hash + halt-on-write-failure. This is the right day-one pattern (CLARIFY D-004 rationale is correct: additive refactor to S3 WORM in M3, not a rewrite). The hash-chain pattern does propagate cleanly to M2/M3 because the append(event) contract is event-type-agnostic. The limit: per-tenant concurrent-write serialization (Axis 6 gap #1) is acceptable for M1 but will surface in M3 under parallel tool calls. Fix #6 documents this so M3 isn't a surprise. With that documentation, the claim holds for M1 and the propagation story is sound.

Claim 3 — "RBAC enforcement (REQ-005) — at the API gateway from the first endpoint. No 'we'll add auth later' stubs."

Verdict: CLAIM MET — NO STUB PERIOD.

Wave B Task 4 ships packages/auth/rbac.ts (role→route map) applied at the API gateway, with GET /api/me as the first protected endpoint and a Viewer → 403 on POST /api/byom test. There is no "auth-later" stub: the middleware runs on every request, the role map is enforced before the handler, and the test proves denial. Wave C's BYOM routes inherit this. The claim is satisfied as literally stated. No fix needed.

Claim 4 — "Secret manager (REQ-040) — every credential via the secret manager from the very first secret. No env vars, no config files, no DB columns. Ever."

Verdict: CLAIM MET IN LETTER, WITH A SEMANTIC GAP THAT MUST BE NAMED.

Wave A Task 6 ships SecretProvider + AWS SM + local-encrypted; Wave C stores BYOM keys via secrets.put; the DB holds only secret_ref. No tenant credential is in an env var, config file, or DB column. But ARCHITECTURE.md (line 61) and RESEARCH.md (R-001 line 15, R-004 line 54) explicitly carve out infra-level env vars: DATABASE_URL, WORKOS_API_KEY, AWS_REGION, SECRET_MASTER_KEY_DEV, TRIGGER_API_KEY. These are not tenant secrets — they are platform bootstrap credentials. The PO's "No env vars... Ever" is, read literally, false; read sensibly, it means "no tenant secret in env vars." This distinction is load-bearing and currently lives only in ARCHITECTURE.md §packages/config and RESEARCH.md footnotes — it is not surfaced in PLAN.md. A reviewer applying the PO's claim verbatim would flag WORKOS_API_KEY and TRIGGER_API_KEY as violations.

Required fix: 10. Add to PLAN.md Wave A (or a new "Credential taxonomy" note) an explicit two-tier model: (a) Infra/bootstrap credentials (DATABASE_URL, WORKOS_API_KEY, TRIGGER_API_KEY, AWS_REGION, SECRET_MASTER_KEY_DEV) loaded via packages/config from env vars — these are platform-level, not tenant-scoped. (b) Tenant credentials (BYOM key, Proxmox token, SSH key, Git token, tenant reg token) via SecretProvider only — never env/config/DB. State that the PO's "no env vars" claim applies to tier (b), and that tier (a) is the documented exception. Without this, the M1 security review will spend cycles re-litigating WORKOS_API_KEY.

---

Binding Fixes (consolidated)

Only items required by PASS-WITH-FIXES verdicts. Numbered G-001..G-010 for this grill session.

ID	Axis	Fix	Blocking wave
G-001	2	Flag /api/byom/test-inference as a plan-time proxy endpoint for REQ-008, marked for M3 deprecation; get PO acknowledgment.	Wave C
G-002	2	Stop writing Trigger.dev health-check ticks to audit_log; use a separate runtime_health table or define a new event_type: "system.health" via spec follow-up. Do not widen REQ-038's auditable-event list silently.	Wave A
G-003	3	Annotate Wave A Task 7 (Trigger.dev) and Wave D Task 6 (whitelist hook) as "pre-investment for M2/M3" with one-line expected payoff, so the cost is visible in the plan.	Wave A, D
G-004	3	Lock CheckCommand(cmd) error signature + whitelist JSON schema as the M2 SSH adapter contract; any signature change requires a documented migration.	Wave D
G-005	4	Resolve Wave B/D token-issuance ownership: POST /api/relay/issue-token owned by backend-engineer, contract (token format, scope, lifetime) defined in Wave A secrets package so B and D parallelize without blocking.	Wave A/B/D
G-006	6	Document per-tenant hash-chain concurrent-write serialization as a known M1-acceptable limit; record M3 mitigation (advisory lock or sequence-per-tenant).	Wave A
G-007	6	Add a shadow exec.Cmd integration test in Wave D proving CheckCommand composes with os/exec (positive: systemctl status nginx; negative: rm -rf / rejected pre-start).	Wave D
G-008	8	Add explicit Wave D scope statement: M1 ships REQ-026 partially (whitelist file + hook + tests); spec §4 SSH-key-auth + tool-call execution are M2.	Wave D
G-009	9	Define unsupported-OS test matrix as ≥2 cases (e.g., Fedora + Alpine) in Wave D must-haves, not a single Fedora container.	Wave D
G-010	6/claim4	Add a two-tier credential taxonomy to PLAN.md: infra/bootstrap creds (env vars, listed) vs tenant creds (SecretProvider only). State PO's "no env vars" applies to tenant creds only.	Wave A

No escalations. All 9 axes resolved at confidence ≥ 0.60. All 4 PO claims either met or met-with-named-fix. No axis FAILED.

---

Final Verdict

PASS-WITH-FIXES — The M1 plan is sound, faithful to spec and architecture, covers all 17 REQs, and will pass the M1 acceptance gate. The 10 binding fixes (G-001..G-010) are required before the wave each touches ships; none block the plan from proceeding to Wave A. The plan's central bet — pre-shipping the Trigger.dev runtime and the SSH whitelist hook to avoid M2/M3 rewrites — is the right call, provided fixes G-002, G-004, G-007, and G-010 lock those pre-investments into actual contracts rather than aspirations.