--- name: ui-review description: >- Internal routed strategy behind `dev-loop` for the UI-review route — the "prove it in the running app" review sibling of reviewer/fixer. Scaffold slice only: registers the route, its stop rules, and its acceptance self-validation. compatibility: Pi skill for git+GitHub repositories. Requires gh auth. allowed-tools: read bash user-invocable: false --- # UI Review (scaffold) When the public router selects `ui_review`, this route reviews a PR by proving the change in the running app from an isolated worktree, rather than reading the diff alone. It is the running-app review sibling of the `reviewer_fixer` route. The route's handoff envelope carries its stop rules and acceptance self-validation (defined in `handoff-envelope.mjs`): no product-code writes, worktree-only, outward review stays pending/draft, and destructive migrations must be acknowledged before they run. ## Provision + boot The route's first operational step provisions an isolated worktree for the PR head and boots the branch's app to a ready state, via `scripts/loop/ui-review-provision.mjs --repo-root

--pr ` (pure orchestration in `packages/core/src/loop/ui-review-provision.mjs`). It reuses the worktree machinery (`ensure-worktree`, `provision-worktree`), refuses to operate in the primary checkout, installs only the dependency-lock delta, runs pending dev-DB migrations, then boots the app and polls an HTTP readiness probe. It fails closed to a stated stop reason on: a primary-checkout target, a missing run recipe, a run-recipe `cwd` that resolves outside the provisioned worktree (worktree traversal), a destructive migration lacking `--ack-destructive-migration`, or a readiness probe that times out. Every bounded cap is logged. The run recipe is per-project and never hard-coded: a project declares `uiReview.run` in `.devloops` — a boot `command`, an HTTP `readyUrl`, probe `readyTimeoutMs`/`readyIntervalMs`, an optional worktree-relative `cwd`, and an optional `migrate` sub-recipe (`statusCommand`/`applyCommand`, plus a `destructivePattern` guard). The destructive guard matches `destructivePattern` against the migration STATUS OUTPUT, not the migration files: the shipped default only detects SQL-bearing status output (DROP/TRUNCATE/DELETE FROM). A project whose status output lists migration identifiers/filenames instead gets no protection from the default and MUST set a `destructivePattern` matching its own status format (or emit the destructive SQL/marker from `statusCommand`) — otherwise the guard is silently inert. Threat boundary: the run recipe is branch-controlled, and its `command` is executed as a shell command in the worktree. Every later stage inherits this assumption — a run recipe is trusted-branch input, not untrusted data. ## Drive Once the app is booted, the route drives the changed UI flows against the handed-off app URL via `scripts/loop/ui-review-drive.mjs --repo-root

--app-url --output-dir

[--changed-path

...]` (pure orchestration in `packages/core/src/loop/ui-review-drive.mjs`). It launches one headless WebKit context, authenticates as the change's target role through a project-provided dev-login recipe, dismisses config-declared interstitials once per context, then walks the selected flows — rendering each page and exercising its declared create/edit/reorder/upload/toggle interactions — capturing a step screenshot + sibling `state.json` per step via `captureNamedUiState`. It fails closed to a stated stop reason when it cannot authenticate, and drives nothing. Throughout the walk, `response`, `requestfailed`, and `pageerror` listeners run and the project server log is tailed, so a swallowed error response (a 500 the UI hides behind a success state) is still recorded. An error response is a status <200 or >=400; 3xx redirects are normal navigation (login/canonical) and are not flagged. The stage emits an ordered set of step screenshots plus a structured captured-failures list (error responses, request failures, page errors, server-log exceptions) that feeds the next stage. Every bounded cap — max screenshots, screens skipped, and the fixed no-retry policy — is logged explicitly. Which flows are driven is a bounded heuristic over an explicit allowlist, never an unbounded crawl: each `uiReview.flows` entry declares `pathPatterns` matched against the PR's changed file paths; an entry with none is always driven, and an unknown diff drives every allowlisted flow. The selection is capped and the overflow logged. The drive recipe is per-project and never hard-coded: a project declares `uiReview.login` (a `loginUrl`, optional username/password field selectors with their dev-only values, a `submitSelector`, and a `successSelector` proving the session), optional `interstitials` (dismiss selectors), the `flows` allowlist, optional `caps` (clamped to the shipped ceilings — a project may only tighten them), and an optional `serverLogPath` (with a `serverLogExceptionPattern` defaulting to a heuristic that a project MUST override when its log format differs). The login form is branch-controlled trusted input, same threat boundary as the run recipe. ## Diagnose + anchor Once failures are captured, the route maps each one to a source line and then to a PR diff line so the poster can anchor an inline comment on a real changed line, via `scripts/loop/ui-review-diagnose.mjs --pr --drive-result

[--repo ]` (pure mapping in `packages/core/src/loop/ui-review-diagnose.mjs`). It reuses PR state from `loop info --pr` rather than re-fetching, fetches the PR's unified diff, and for each failure parses the exception type/message plus the top in-repo stack frame (JS `at` frame, Ruby/Python traceback frame; vendor/framework frames in `node_modules`/`gems`/`vendor` are skipped). It then resolves the source `file:line` to a diff anchor `{ path, line, side: RIGHT }` on the head. Only ADDED lines are anchor targets: an inline comment lands on code the PR introduced, never on an unchanged context line. A failure is NEVER silently dropped — one with no source location, a file that is not among the changed files, a line that is not on a changed diff line, or a file that maps ambiguously to more than one changed file is retained as a finding flagged non-anchorable (with a stated reason) so the poster body-attaches it instead of inlining. Each finding carries a reference to the drive's final captured screenshot/state artifact when one exists (null otherwise) — a single shared object across all findings, NOT per-failure attribution — so a Stage-4 consumer null-checks it and must not present it as proof of a specific finding. The findings list is ranked deterministically — severity, then anchorable-first, then kind, then source `file:line` — with no wall-clock or input-order dependence, so the same failures always produce the same ordered output. ## Report The terminal reporting stage turns the ranked findings into a head-pinned PENDING PR review plus a self-contained screenshot artifact, via `scripts/loop/ui-review-report.mjs --pr --diagnose-result

--html-output

[--repo ]` (pure decisions in `packages/core/src/loop/ui-review-report.mjs`). It reuses the shared pending-review poster (`scripts/github/stage-reviewer-draft.mjs` + `buildDraftReviewPayload`) — a caller/adapter, not a new poster. Each anchorable finding becomes an inline comment on its exact `{path,line,side:RIGHT}` anchor carrying the reproduced exception + a fix direction; non-anchorable findings are retained in the review body (never dropped). It reuses the live head SHA from `loop info` and fails closed when the diagnosed head is missing or the live head has advanced since diagnose, so the inline anchors always bind to the exact reviewed commit. The review defaults to **pending/draft** (no `event`) — this stage never auto-submits. A confirmed user-facing server error (a must-fix error-response / server-log-exception) maps to `REQUEST_CHANGES` **only when submit is authorized**; otherwise the review stays pending with the severity recorded. The severity->event decision is emitted as guidance; submitting via the events endpoint is a separate authorized action outside this stage. The self-contained artifact is always produced: a CSP-safe, fully inlined HTML (ranked findings + the reproduced-evidence screenshot as a data URI, no external resources). Hosting is harness-aware: on the **Claude Code** harness the stage emits a publishable directive (`{ hosting: "claude-artifact", htmlPath, publishable: true }`) for the orchestrating agent to publish via Claude Artifacts — the module never calls an Artifacts tool itself. On any other harness it **fails closed with a stated reason** (`{ hosting: "unavailable", reason, followup }`); the GitHub-native fallback publisher is deferred. The review body links a hosted artifact when one exists and otherwise states the artifact is unhosted (with the reason), so the review never blocks on hosting. Every bounded cap — findings truncated past the artifact cap, an oversized or unreadable evidence screenshot omitted — is logged, never silent. ## Teardown + side-effect ledger The terminal cleanup stage tears down the loop's transient state — stops the app booted in provision, drops the dev-DB rows the drive created, removes the provisioned worktree — and ALWAYS emits a side-effect ledger so nothing is silently orphaned, via `scripts/loop/ui-review-teardown.mjs --repo-root

--provision-result

[--drive-result

] [--row-manifest

] [--confirm] [--no-stop-app]` (pure decisions in `packages/core/src/loop/ui-review-teardown.mjs`). It reads the prior-stage result JSON — the app PID + applied migrations + worktree path from provision, and the rows-created signal from the drive. The destructive steps (dev-DB row drops and worktree removal) run ONLY with an explicit `--confirm`. Without it, those steps are skipped and the ledger records what remains; stopping the app still runs, because it is a clean shutdown of a process the loop itself started, not a mutation of persisted state. The app is stopped via the provision boot PID (SIGTERM, then a LOGGED SIGKILL fallback); a null PID is never a blind kill — the ledger reports the process may still be running. On win32 the app-stop fails closed (process-group kill is unsupported, so the kill is not attempted): the ledger reports may-be-running rather than a false stopped. Worktree removal delegates to `scripts/loop/cleanup-worktree.mjs`, which refuses any path outside the loop namespace and leaves the primary checkout untouched. The side-effect ledger is emitted in EVERY case (success, skip, partial failure) and enumerates: migrations applied (recorded as applied-not-reverted — reversing a dev-DB migration is a separate explicit action, never a default), rows created/dropped or left behind, the worktree path + whether it was removed, and the process status. A failed kill/drop/removal is reported in the ledger and the result's `errors` list, never swallowed. Row dropping is honest about a known limitation: the drive does not tag the dev-DB rows it creates with a session id or manifest, so this stage cannot know which rows to drop and MUST NOT guess. It drops rows only from an explicit manifest handed in; when the drive walked mutating flows without one, the ledger reports rows "may remain (untagged)". Making the drop real requires row/session tagging upstream in the drive. The CLI's row-drop seam is not yet wired: a confirmed manifest fails CLOSED (the ledger records a drop failure, never a drop) rather than silently no-op'ing, so a caller can never believe rows were dropped; wiring a real drop is a tracked follow-up. ## Non-goals The teardown stage never rolls back the branch's dev-DB migrations by default (the ledger records they were applied, not reverted) and never tears down a production DB. The stage does not auto-submit a review without explicit authorization, publish to a production/non-dev posting target, ship the GitHub-native hosted-artifact fallback, auto-fix the located defects, pixel-diff for visual regression, run a cross-browser matrix, or touch a production DB — those are later stages or explicit non-goals. It does not replace the product/eng `review` angle or the Copilot gate.