Status: Active plan for issue #131

# Plan: Make /foundation_ui_compliance fail-closed

## Goal

Transform the bundled `/foundation_ui_compliance` saved workflow from a prompt-driven best-effort loop into a fail-closed delivery engine: host-side gates with structured argv, worktree isolation, deterministic path scopes, structured visual verification, and evidence-based finalization.

## Verified current behavior

| Symptom | Evidence |
|---------|----------|
| Read-only agents told to run shell commands | `foundation_ui_compliance.workflow.mjs:L66` — `gate-diagnose` (marked `readOnly: true`) is prompted with `Run: ${GATE_ENV}${GATE} --json`. After #130 strips `bash` from read-only sessions, this silently produces garbage. |
| editAllow/editDeny are prompt-only | Template lines 98–100 instruct the fix agent via text ("Edit ONLY", "NEVER touch"). No host-side path validation exists after edits. |
| Shell-string command injection | Line 57: `GATE` is built by concatenating `appSrc`, `buildCmd`, `baseline`, `urls` directly into a shell string. Malicious args inject arbitrary commands. |
| Visual FAIL does not block delivery | Lines 115–130: `visual-verify` result is logged but never checked. If `gatesCleared` is true, delivery proceeds regardless. |
| trace-assert claims unavailable capability | Lines 139–148: prompt tells trace-assert to "Read this run's subagent transcripts" — no trace API is exposed to the script. |
| No worktree isolation | `saved-commands.ts:76-91` — `savedWorkflowExecutionPolicy` has no worktree/seam concept for mutating workflows. Primary checkout can be corrupted. |
| No finalization evidence | Lines 135–148: deliver agent commits/pushes/opens PR, but no `checkFinalization()` or gate-evidence collection verifies cleanliness. |

## Dependencies

- **#129** (SDK compat): Must land first so `runWorkflow` and `agent()` compile against Pi ≥ 0.80.8. No plan-level assumption changes if this lands cleanly.
- **#130** (tool policy enforcement): Must land first. This is the prerequisite that exposes the broken read-only agent shell problem as visibly failing, forcing the host-runner redesign. Without #130, prompt-only enforcement still works.

## Allowed paths

- `src/foundation-ui-compliance.ts` — typed host gate runner and bundled definition
- `src/workflow-saved.ts` — bundled execution-policy metadata contract
- `src/saved-commands.ts` / `src/workflow-manager.ts` — fail-closed run-level isolation and result retention wiring
- `src/workflow.ts` — narrowly typed injected host callbacks/globals only if required
- a dedicated host-runner/scope-validation module under `src/` if that keeps `stage-check.ts` focused
- `docs/workflows/templates/foundation_ui_compliance.workflow.mjs` — script rewrite
- `docs/workflows/foundation-ui-compliance.md`, catalog/lock if the shipped contract/hash changes
- `tests/foundation-ui-compliance.test.ts` and focused saved-command/manager tests

`src/worktree.ts` should not need changes unless a missing lifecycle primitive is proven; reuse the manager's existing fail-closed run-level isolation rather than creating/removing worktrees inside the command handler.

## Denied / out-of-scope paths

- **Vendored foundation (`third_party/frontend-foundation/`)** — outside this repo's authority.
- **`src/agent.ts` core session construction** — tool policy enforcement belongs in #130. We only consume the enforced boundaries.
- **General host-runner abstraction** — do NOT design a universal host-command DSL. Build a narrowly-scoped `foundationGate()` tailored to this workflow's gate contract.
- **New SDK features** — no Pi API proposals. Work within current `runWorkflow`/`agent()`/`stageCheck`/`checkFinalization`.
- **Broad test matrix expansion** — only tests covering the 7 acceptance criteria.

## Dependency-ordered implementation steps

### Step 1: Host-side foundation gate runner (src/foundation-ui-compliance.ts + src/stage-check.ts)

**Goal**: Replace agent-initiated shell execution with host-side structured argv execution, returning bounded validated JSON.

- Add a narrowly scoped host runner that accepts typed Foundation options (`foundation`, `appSrc`, `buildCmd`, URLs, baseline, screenshot directory), validates repo-relative paths/URLs, constructs argv internally, invokes only `<foundation>/scripts/run-foundation-gates.mjs` with `shell: false`, applies output/time bounds, and validates the documented `{ summary, results }` JSON contract and exit-code semantics.
- Inject that callback into `runWorkflow`/the VM as a Foundation-specific global only for runs whose bundled execution policy authorizes it, or expose a generally reusable **typed callback injection seam**. Do not expose a free-form `hostExec(argv)` capability to arbitrary trusted workflow scripts as an accidental authority expansion.
- Add the new callback identity/version to resume hashing or invalidate saved-template journals when its semantics change.
- `createBundledFoundationUiComplianceWorkflow()` supplies execution-policy metadata; it does not itself “register globals.” Manager/run options wire authorized host callbacks into the engine.

**Key invariant**: All args are passed as structured argv (never concatenated into shell strings). Gate output is validated against a JSON schema before surfaced to any agent.

### Step 2: Rewrite the template script (foundation_ui_compliance.workflow.mjs)

**Phases to modify**:

- **Gate-Diagnose**: Call `foundationGate({ argv, env })` directly. If structured output has `ok: true`, set `gatesCleared = true` and report CLEAN. Otherwise surface the typed violation list. No `agent()` needed for gate execution — use agent only for reading/interpreting results if needed.
- **Fix <-> Re-gate loop**: Keep fix agent (mutation-authorized). After each fix, call `foundationGate()` again (not `agent()` with shell prompt). Parse result structurally.
- **Visual verify**: Use `schema` option on the agent call with a strict JSON schema: `{ type: "object", properties: { verdict: { enum: ["PASS", "FAIL"] }, defects: [...] } }`. If verdict is FAIL, malformed, or null → `visualPass = false` and **block delivery**.
- **Deliver**: Before commit/push, require `gatesCleared` and, when URLs were requested, `visualPass`. After the delivery agent returns, call `checkFinalization(cwd)` and accept only statuses allowed by this repository's existing finalization contract (for example, `completed` or explicitly pending/finalizing when checks are still running); do not invent a narrower string check without reading the helper.
- **Remove Trace-assert**: Delete the phase. Replace it with a host-collected evidence object (gate JSON results, changed-path validation, visual verdict, delivery/finalization result) returned in the run result and logs.

### Step 3: Deterministic path scope validation

**Goal**: After every fixer round, validate that changed files match `editAllow` and do not touch `editDeny`.

- Add a host-side changed-path collector using structured git argv and parse rename/copy source plus destination, untracked paths, and paths containing spaces. Reuse proven porcelain parsing from conductor finalization where possible.
- Normalize and validate user globs without adding a dependency unless necessary. Always-denied prefixes override `editAllow`; path traversal/absolute patterns fail input validation.
- Inject a Foundation-specific `validateEditScope` callback under the same authorized execution-policy seam as the gate runner. Call after every fix round and immediately before delivery. Return exact offending paths and fail closed.

### Step 4: Worktree isolation seam

**Goal**: The primary checkout cannot be mutated.

- Add explicit execution-policy metadata to `SavedWorkflowDefinition` (for example, `worktreeRequired: true` plus an authorized host-capabilities list), validate it when loading bundled/project/user definitions, and include it in command registration/resume identity as applicable.
- Pass `worktreeRequired: true` to the existing `WorkflowManager.startInBackground()` path. Let `WorkflowManager` create the worktree fail-closed and bind tools to the worktree cwd; do not create a second worktree inside `saved-commands.ts`.
- Define the inline/no-manager behavior explicitly: either create equivalent fail-closed isolation through a shared runner or refuse the mutating bundled workflow with an actionable error. Never silently run it in the primary checkout.
- Mark only the bundled Foundation definition as requiring this policy; project/user overrides must not be able to silently weaken a package-owned command's minimum safety policy.

### Step 5: Execution-policy seam for review-only output retention

**Goal**: `deliver=false` preserves inspectable output.

- Extend manager settlement policy so a successful `deliver=false` Foundation run retains the isolated worktree **or** exports a binary-safe patch/artifact before cleanup. Return the exact artifact/worktree path in the final result and recovery notification.
- Bound artifact size and represent binary/untracked files honestly; a plain `git diff HEAD` alone is insufficient for untracked/binary output.
- When `deliver=true`, use the existing commit/push/PR and finalization path, then apply the manager's normal terminal cleanup policy.

### Step 6: Update docs/workflows/foundation-ui-compliance.md

Update the user-facing docs to reflect:

- Structured args contract (no shell strings)
- New phases: Gate-Diagnose → Fix ↔ Re-gate → Visual verify → Deliver + Finalization
- Removed Trace-assert phase
- Worktree isolation guarantee
- `deliver=false` output retention

## Targeted tests (tests/foundation-ui-compliance.test.ts)

Add integration tests covering each acceptance criterion:

1. **Host gate with structured argv** — mock `foundationGate` to return typed JSON; verify the script calls it with argv (not a shell string). Assert hostile args (`"; rm -rf /"`) reach the runner as a literal arg, not interpreted.
2. **Denied paths fail the run** — mock `validateEditScope` to reject a file in `editDeny`; assert the run fails before delivery.
3. **Red gates block delivery** — gate runner returns `ok: false`; assert deliver agent is never invoked.
4. **Visual failure blocks delivery** — visual-verify agent returns `verdict: "FAIL"`; assert deliver agent is skipped.
5. **Null/malformed visual verdict blocks delivery** — agent returns non-schema-compliant output; assert delivery blocked.
6. **Isolation failure** — worktree creation fails; assert the run aborts before any mutation.
7. **Review-only retention** — `deliver=false`; assert worktree path is logged and worktree is NOT removed.
8. **Finalization evidence** — mock `checkFinalization` returning `status: "completed"`; assert run completes. Mock returning `"failed"`; assert run fails.

## Floor / latest compatibility

- No floor-breaking changes: all new fields are optional with safe defaults (`worktreeIsolation: "none"` preserves existing behavior for other saved workflows).
- The template script is rewritten but the saved workflow name and basic arg contract (`appSrc`, `editAllow`, etc.) remain compatible. Existing `key=value` args continue to work.
- `foundationGate` and `validateEditScope` are new workflow globals — backward compat for other workflows unaffected.
- Full gate: `npm test` (biome + build + all unit tests).

## Acceptance checklist (mapped to issue #131)

- [ ] **Gate execution does not depend on agent shell** — Step 1 uses a typed host runner with `shell: false`.
- [ ] **Cannot be command-injected through args** — structured argv is constructed only after host validation.
- [ ] **Out-of-scope edits fail the run** — host changed-path validation runs after each fix.
- [ ] **Red gates prevent delivery** — structured `ok: false` blocks delivery.
- [ ] **Failed/missing visual verdicts prevent delivery** — strict schema and control flow block delivery.
- [ ] **Primary checkout cannot be mutated** — manager-level worktree isolation is required and fail-closed.
- [ ] **Review-only changes remain inspectable** — retained worktree or bounded artifact is returned.
- [ ] **Final success requires host gate + finalization evidence**.
- [ ] **Integration tests cover hostile args, denied/renamed/untracked paths, red gates, visual failure, isolation failure, review-only retention, and finalization**.

## Rollback / recovery

- If host-gate integration fails, stop/revert the candidate release and keep the command unavailable/blocked rather than falling back to the known-insecure agent-shell path.
- If required isolation cannot be created, abort before any mutation with an actionable error.
- Keep persisted-definition loading backward-compatible, but enforce package minimum safety policy when a project/user override uses the canonical bundled command name.

## Deferred work

- **Universal host-runner DSL**: If multiple workflows need structured host commands, generalize `foundationGate` into `hostExec(argv, env, schema)`. Out of scope for this issue.
- **Trace API for workflow scripts**: Replacing trace-assert with a real host-collected trace requires a runtime tracing mechanism. Filed separately.
- **Foundation gate schema registry**: Currently the gate JSON shape is hardcoded. A registry would allow foundations to declare their output schema. Deferred.