From 99283ee4c7dbea835b7fdaa0ed4075062f39c9f6 Mon Sep 17 00:00:00 2001 From: Manuel Fittko Date: Sat, 11 Jul 2026 01:04:21 +0200 Subject: [PATCH] feat(loop): GitHub-native gist fallback for ui_review artifact hosting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Off-Claude, the Stage-4 report now publishes the self-contained HTML as a secret GitHub Gist (gh gist create) — a real per-run URL with zero repo pollution — instead of failing closed. decideHosting selects the portable github-gist strategy in the pure core; the thin CLI performs the publish IO and fails closed with a stated reason (never a fabricated link) when gist creation yields no URL. The review body links the gist and states it renders as source (open raw to view/download). Stage-5 teardown prunes the accreting gist: the report result carries the gist id, teardown consumes it via --report-result and deletes it (gh gist delete) under confirmation, recording the outcome in the always-emitted ledger. Recipe doc documents the gh gist scope a consuming repo must provide and the source-vs-rendered tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo --- docs/ui-review-recipe-contract.md | 32 ++-- packages/core/src/loop/ui-review-report.mjs | 36 ++--- packages/core/src/loop/ui-review-teardown.mjs | 49 +++++- scripts/loop/ui-review-report.mjs | 56 ++++++- scripts/loop/ui-review-teardown.mjs | 49 ++++-- test/loop/ui-review-report.test.mjs | 139 ++++++++++++++++-- test/loop/ui-review-teardown.test.mjs | 77 +++++++++- 7 files changed, 373 insertions(+), 65 deletions(-) diff --git a/docs/ui-review-recipe-contract.md b/docs/ui-review-recipe-contract.md index 727f7472..6c266520 100644 --- a/docs/ui-review-recipe-contract.md +++ b/docs/ui-review-recipe-contract.md @@ -44,11 +44,12 @@ drive your app. 4. **Report** — posts a head-pinned **pending** PR review and produces a self-contained HTML artifact. 5. **Teardown** — stops the booted app and, only with explicit confirmation, - removes the worktree (the working destructive step). Dev-DB row-drop currently - **fails closed** — the tagged-drop seam is not wired, so a confirmed manifest - is recorded as a drop failure and untagged rows may remain (a real tagged drop - is a tracked follow-up). A side-effect ledger is ALWAYS emitted, enumerating - what was done vs. left behind. + removes the worktree (the working destructive step) and prunes the Stage-4 + hosting gist (`gh gist delete`, when a gist id is recorded in the report + result). Dev-DB row-drop currently **fails closed** — the tagged-drop seam is + not wired, so a confirmed manifest is recorded as a drop failure and untagged + rows may remain (a real tagged drop is a tracked follow-up). A side-effect + ledger is ALWAYS emitted, enumerating what was done vs. left behind. ## Guardrails (non-negotiable) @@ -231,10 +232,23 @@ no external resources). - On the **Claude Code** harness the stage emits a publishable directive and the artifact is published via **Claude Code Artifacts** — a zero-setup hosted link the review body links to. The module never calls an Artifacts tool itself; the - orchestrating agent publishes. -- On **any other harness** hosting **fails closed with a stated reason**: the - review body states the artifact is unhosted and why, and never blocks on - hosting. A GitHub-native fallback publisher is a **tracked follow-up**. + orchestrating agent publishes. This is an enhancement layered on top of the + portable default below, not a dependency of the core report flow. +- On **any other harness** the portable **GitHub-native default** publishes the + self-contained HTML as a **secret GitHub Gist** (`gh gist create`) — a real + per-run URL with zero repo pollution. The review body links it. Two honest + caveats: a gist **renders HTML as source, not a live page**, so the review body + links the gist and points at its **raw** file (the plain-text/download view); and + a gist accretes one secret entry per run, so **Stage 5 teardown prunes it** + (`gh gist delete`) when the run records the gist id in the report result and + teardown is confirmed. If gist creation does not yield a URL the stage **fails + closed with a stated reason** — the review body states the artifact is unhosted + and why, and never links a fabricated URL. + + Setup a consuming repo must provide: an authenticated **`gh`** with the **`gist`** + scope (`gh auth login`/`gh auth refresh -s gist`). No `.devloops` config key is + involved — hosting rides on the same `gh` auth the loop already uses for reviews. + An explicit `--hosted-url` overrides the gist path when you host the HTML yourself. ## Config key reference diff --git a/packages/core/src/loop/ui-review-report.mjs b/packages/core/src/loop/ui-review-report.mjs index a62eb1ae..588c1d33 100644 --- a/packages/core/src/loop/ui-review-report.mjs +++ b/packages/core/src/loop/ui-review-report.mjs @@ -12,8 +12,9 @@ * - a self-contained, CSP-safe HTML artifact string (ranked findings + inline * screenshot evidence), and * - a harness-aware hosting directive (Claude Code -> a publishable Artifacts - * directive for the orchestrator; any other harness -> fail closed with a - * stated reason and a follow-up marker — no hosted link this stage). + * directive for the orchestrator; any other harness -> a GitHub-native + * gist-publish directive the CLI executes, which yields a real per-run URL + * or fails closed with a stated reason — never a fake link). * * All IO (reading the diagnose output + the screenshot bytes, writing the HTML, * invoking the poster) lives in the thin CLI. This module reads only its inputs. @@ -22,9 +23,6 @@ import { isClaudeHarness } from "./run-context.mjs"; import { sanitizeCopilotSummonTokens } from "../github/copilot-helpers.mjs"; -/** Follow-up marker for the descoped GitHub-native hosting fallback. */ -export const HOSTING_FOLLOWUP = "#1285"; - /** Findings past this cap are dropped from the artifact and the drop is logged. */ export const ARTIFACT_MAX_FINDINGS = 100; @@ -105,9 +103,11 @@ export function severityToEvent({ findings = [], submitAuthorized = false } = {} /** * Harness-aware hosting directive (pure). Claude Code -> a publishable Artifacts * directive for the orchestrator to host (this module never calls an agent tool - * itself). Any other harness / Artifacts unavailable -> fail closed with a - * stated reason and the follow-up marker. The self-contained HTML is produced - * regardless; only this link step is harness-aware. + * itself). Any other harness -> the portable GitHub-native default: publish the + * self-contained HTML as a secret GitHub Gist (a real per-run URL, zero repo + * pollution). This module decides the STRATEGY only; the CLI performs the gist + * publish IO and fails closed with a stated reason if it does not yield a URL. + * The self-contained HTML is produced regardless; only this link step differs. * * @param {{htmlPath: string, env?: Record}} input */ @@ -115,13 +115,7 @@ export function decideHosting({ htmlPath, env = process.env } = {}) { if (isClaudeHarness(env)) { return { hosting: "claude-artifact", publishable: true, htmlPath: htmlPath ?? null }; } - return { - hosting: "unavailable", - publishable: false, - htmlPath: htmlPath ?? null, - reason: "no hosted-artifact publisher on this harness; GitHub-native fallback is deferred", - followup: HOSTING_FOLLOWUP, - }; + return { hosting: "github-gist", publishable: true, htmlPath: htmlPath ?? null }; } /** One review-body line describing where the screenshot artifact lives. Links a @@ -129,14 +123,22 @@ export function decideHosting({ htmlPath, env = process.env } = {}) { * the review never blocks on hosting. */ function artifactBodyLine({ hosting, hostedUrl }) { if (typeof hostedUrl === "string" && hostedUrl.length > 0) { + // Only an ACTUALLY-published gist gets the source-rendered caveat: an explicit + // --hosted-url override leaves the strategy as github-gist but sets no gist, so + // a self-hosted (maybe live-rendered) URL falls through to the neutral line. + const rawUrl = hosting?.gist?.rawUrl; + if (rawUrl) { + // A gist renders HTML as source, not a live page; the raw file is the + // download/plain-text view — surface it so "open raw" is actually actionable. + return `Screenshot artifact (GitHub Gist — renders as source; open the raw file to view/download the HTML): ${hostedUrl} (raw: ${rawUrl})`; + } return `Screenshot artifact: ${hostedUrl}`; } if (hosting?.hosting === "claude-artifact") { return "Screenshot artifact prepared for Claude Artifacts hosting (published by the harness; see run output)."; } const reason = hosting?.reason ? ` (${hosting.reason})` : ""; - const followup = hosting?.followup ? ` [follow-up ${hosting.followup}]` : ""; - return `Screenshot artifact is unhosted this stage${reason}${followup}. Findings are included below.`; + return `Screenshot artifact is unhosted this stage${reason}. Findings are included below.`; } /** A finding is inlineable ONLY with a complete anchor buildDraftReviewPayload diff --git a/packages/core/src/loop/ui-review-teardown.mjs b/packages/core/src/loop/ui-review-teardown.mjs index 839495ad..49c23efa 100644 --- a/packages/core/src/loop/ui-review-teardown.mjs +++ b/packages/core/src/loop/ui-review-teardown.mjs @@ -60,6 +60,13 @@ const PROCESS_STATUS = Object.freeze({ SKIPPED: "skipped", }); +const GIST_STATUS = Object.freeze({ + DELETED: "deleted", + DELETE_FAILED: "delete-failed", + SKIPPED_UNCONFIRMED: "skipped-unconfirmed", + NONE: "none", +}); + /** * Did the Stage-2 drive potentially create dev-DB rows? Without row tagging this * is a coarse but honest signal: a drive that actually walked steps exercised @@ -82,8 +89,12 @@ function driveMayHaveCreatedRows(driveResult) { * signal (whether the drive walked mutating steps). Null when no drive ran. * @param {Array|null} [input.rowManifest] - Explicit rows to drop, when * a session tag/manifest is available. Absent/empty => untagged fallback. + * @param {{id?:string|null,url?:string|null}|null} [input.gist] - The Stage-4 + * GitHub-native hosting artifact (a secret gist) to prune, when one was + * published off-Claude. Absent => nothing to prune. * @param {boolean} [input.confirm] - Explicit authorization for the destructive - * steps (row drop, worktree removal). Fail-safe: absent means NOT confirmed. + * steps (row drop, worktree removal, gist deletion). Fail-safe: absent means + * NOT confirmed. * @param {boolean} [input.stopApp] - Stop the Stage-1 app (default true). This is * a clean shutdown, NOT gated on confirmation. * @param {object} seams @@ -92,12 +103,14 @@ function driveMayHaveCreatedRows(driveResult) { * signalling is unsupported) — mapped to MAY_BE_RUNNING (non-fatal), not KILL_FAILED. * @param {(a:{rows:Array})=>Promise<{ok:boolean,dropped:number,detail:string}>} seams.dropRows * @param {(a:{worktreePath:string})=>Promise<{removed:string|null,ok:boolean,detail:string}>} seams.removeWorktree + * @param {(a:{id:string})=>Promise<{ok:boolean,detail:string}>} [seams.deleteGist] - Prune the + * Stage-4 hosting gist. Only invoked when a gist id is present AND confirmed. * @param {(msg:string)=>void} [seams.log] * @returns {Promise<{ok:boolean,confirmed:boolean,ledger:object,errors:string[],logs:string[]}>} */ export async function teardown( - { provisionResult, driveResult = null, rowManifest = null, confirm = false, stopApp = true }, - { killProcess, dropRows, removeWorktree, log = () => {} } = {}, + { provisionResult, driveResult = null, rowManifest = null, gist = null, confirm = false, stopApp = true }, + { killProcess, dropRows, removeWorktree, deleteGist, log = () => {} } = {}, ) { const logs = []; const errors = []; @@ -229,6 +242,33 @@ export async function teardown( } } + // 4. Prune the Stage-4 hosting gist — DESTRUCTIVE, confirmation-gated. A gist + // accretes one secret entry per run; deleting it keeps the hosting target + // from piling up. Only ever acts on an explicit gist id from the report + // result; a missing id is NONE (nothing published, or Claude-hosted). + const gistId = typeof gist?.id === "string" && gist.id.trim().length > 0 ? gist.id.trim() : null; + let gistLedger; + if (!gistId) { + gistLedger = { id: null, url: gist?.url ?? null, deleted: false, status: GIST_STATUS.NONE, detail: "no hosting gist to prune" }; + } else if (!confirm) { + gistLedger = { id: gistId, url: gist?.url ?? null, deleted: false, status: GIST_STATUS.SKIPPED_UNCONFIRMED, detail: "hosting gist retained: teardown not confirmed" }; + record(`gist prune skipped (not confirmed): ${gistId} retained`); + } else { + try { + const del = await deleteGist({ id: gistId }); + if (del.ok) { + gistLedger = { id: gistId, url: gist?.url ?? null, deleted: true, status: GIST_STATUS.DELETED, detail: del.detail }; + record(`hosting gist deleted: ${gistId} (${del.detail})`); + } else { + gistLedger = { id: gistId, url: gist?.url ?? null, deleted: false, status: GIST_STATUS.DELETE_FAILED, detail: del.detail }; + fail(`hosting gist delete FAILED: ${gistId} (${del.detail})`); + } + } catch (err) { + gistLedger = { id: gistId, url: gist?.url ?? null, deleted: false, status: GIST_STATUS.DELETE_FAILED, detail: `deleteGist seam threw: ${err?.message ?? err}` }; + fail(`hosting gist delete FAILED: ${gistId} (deleteGist seam threw: ${err?.message ?? err})`); + } + } + // The ledger is ALWAYS emitted (every case), enumerating every known side // effect. Migrations are recorded as applied-not-reverted by design. const ledger = { @@ -241,10 +281,11 @@ export async function teardown( }, rows: rowsLedger, worktree: worktreeLedger, + gist: gistLedger, process: processLedger, }; return { ok: errors.length === 0, confirmed: confirm, ledger, errors, logs }; } -export { ROW_STATUS, WORKTREE_STATUS, PROCESS_STATUS }; +export { ROW_STATUS, WORKTREE_STATUS, PROCESS_STATUS, GIST_STATUS }; diff --git a/scripts/loop/ui-review-report.mjs b/scripts/loop/ui-review-report.mjs index 5c85b9ac..e73f438b 100644 --- a/scripts/loop/ui-review-report.mjs +++ b/scripts/loop/ui-review-report.mjs @@ -20,7 +20,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { parseArgs } from "node:util"; import { buildParseError, formatCliError, isDirectCliRun } from "../_core-helpers.mjs"; -import { requireTokenValue, parsePositiveInteger } from "../_cli-primitives.mjs"; +import { requireTokenValue, parsePositiveInteger, runChild } from "../_cli-primitives.mjs"; import { detectRepoSlug, normalizeRepoSlug } from "@dev-loops/core/github/repo-slug"; import { buildArtifactHtml, @@ -161,7 +161,34 @@ function postPendingReview({ repo, pr, reviewFile, cwd }) { return { reviewId: parsed.reviewId ?? null, reviewUrl: parsed.reviewUrl ?? null, commitSha: parsed.commitSha ?? null }; } -export async function runCli(argv = process.argv.slice(2), { stdout = process.stdout, stderr = process.stderr, env = process.env } = {}) { +/** Publish the self-contained HTML as a secret GitHub Gist and return its URL + + * id. This is the GitHub-native, harness-agnostic hosting default off-Claude: a + * real per-run URL with zero repo pollution, deletable by Stage-5 teardown (which + * consumes the recorded id). A gist renders HTML as source, so the raw file URL + * is the plain-text/download view. Fails closed: a non-zero gh exit or missing + * URL throws — the caller states the reason and never links a fake URL. */ +export async function publishGist({ htmlPath, pr, run = runChild, ghCommand = "gh", env = process.env }) { + const desc = `UI review findings — PR #${pr} (self-contained HTML; renders as source on GitHub Gist)`; + const result = await run(ghCommand, ["gist", "create", htmlPath, "--desc", desc], env); + if (result.code !== 0) { + throw new Error(`gh gist create failed: ${result.stderr.trim() || `exit code ${result.code}`}`); + } + const url = result.stdout + .split(/\r?\n/u) + .map((line) => line.trim()) + .filter((line) => line.length > 0) + .pop() ?? null; + if (url === null || !/^https?:\/\//u.test(url)) { + throw new Error(`gh gist create did not return a gist URL (got: ${result.stdout.trim() || ""})`); + } + const id = url.split("/").filter((seg) => seg.length > 0).pop() ?? null; + return { url, id, rawUrl: `${url}/raw` }; +} + +export async function runCli( + argv = process.argv.slice(2), + { stdout = process.stdout, stderr = process.stderr, env = process.env, run = runChild, loadLiveHead = loadLiveHeadSha, postReview = postPendingReview } = {}, +) { const options = parseUiReviewReportCliArgs(argv); if (options.help) { stdout.write(`${USAGE}\n`); return; } @@ -181,7 +208,7 @@ export async function runCli(argv = process.argv.slice(2), { stdout = process.st // Head pinning: the inline anchors bind to the reviewed commit. Fail closed // when the diagnose head is missing or the live head has advanced since. - const liveHeadSha = loadLiveHeadSha(options.pr, repo, cwd); + const liveHeadSha = loadLiveHead(options.pr, repo, cwd); if (!diagnosedHeadSha) { throw parseError("Stage-3 diagnose output has no pr.headSha; cannot head-pin the review. Failing closed."); } @@ -203,14 +230,31 @@ export async function runCli(argv = process.argv.slice(2), { stdout = process.st mkdirSync(path.dirname(path.resolve(options.htmlOutput)), { recursive: true }); writeFileSync(options.htmlOutput, html, "utf8"); - const hosting = decideHosting({ htmlPath: path.resolve(options.htmlOutput), env }); + let hosting = decideHosting({ htmlPath: path.resolve(options.htmlOutput), env }); const policy = severityToEvent({ findings, submitAuthorized: options.submitAuthorized }); + // Off-Claude default: publish the HTML as a secret gist to yield a real hosted + // URL. An explicit --hosted-url wins (caller already hosted it); --dry-run + // skips the side effect. A gist-publish failure fails closed here — hosting + // becomes `unavailable` with the stated reason, never a fabricated link. + let hostedUrl = options.hostedUrl ?? null; + if (!hostedUrl && hosting.hosting === "github-gist" && !options.dryRun) { + try { + const gist = await publishGist({ htmlPath: path.resolve(options.htmlOutput), pr: options.pr, run, env }); + hostedUrl = gist.url; + hosting = { ...hosting, gist: { id: gist.id, url: gist.url, rawUrl: gist.rawUrl } }; + } catch (err) { + const reason = `GitHub-native gist publish failed: ${err?.message ?? err}`; + hosting = { hosting: "unavailable", publishable: false, htmlPath: hosting.htmlPath, reason }; + caps.push(`hosting: ${reason}`); + } + } + const reviewInput = buildReviewInput({ findings, headSha: diagnosedHeadSha, hosting, - hostedUrl: options.hostedUrl ?? null, + hostedUrl, }); const reviewFile = options.reviewFile ? path.resolve(options.reviewFile) @@ -220,7 +264,7 @@ export async function runCli(argv = process.argv.slice(2), { stdout = process.st for (const cap of caps) stderr.write(`[ui-review-report] cap: ${cap}\n`); - const review = options.dryRun ? null : postPendingReview({ repo, pr: options.pr, reviewFile, cwd }); + const review = options.dryRun ? null : postReview({ repo, pr: options.pr, reviewFile, cwd }); const result = { ok: true, diff --git a/scripts/loop/ui-review-teardown.mjs b/scripts/loop/ui-review-teardown.mjs index 50501061..cb841340 100644 --- a/scripts/loop/ui-review-teardown.mjs +++ b/scripts/loop/ui-review-teardown.mjs @@ -3,20 +3,22 @@ * CLI wrapper for the ui_review teardown stage (Stage 5, terminal cleanup). * * Stops the app booted in Stage 1, drops the dev-DB rows the Stage-2 drive - * created (only from an explicit manifest, only on confirmation), and removes - * the provisioned worktree via the shared cleanup path. It ALWAYS emits a + * created (only from an explicit manifest, only on confirmation), removes the + * provisioned worktree via the shared cleanup path, and prunes the Stage-4 + * GitHub-native hosting gist (only on confirmation). It ALWAYS emits a * side-effect ledger enumerating what was torn down and what remains. * * Thin adapter: it wires the real IO seams — process kill (SIGTERM then a - * logged SIGKILL fallback), row drop, and worktree removal (the shared - * cleanup-worktree path) — into the pure core orchestrator + * logged SIGKILL fallback), row drop, worktree removal (the shared + * cleanup-worktree path), and gist deletion (`gh gist delete`) — into the pure + * core orchestrator * (packages/core/src/loop/ui-review-teardown.mjs), reading the prior-stage * result JSON from disk. */ import { readFileSync, statSync } from "node:fs"; import { parseArgs } from "node:util"; import { buildParseError, formatCliError, isDirectCliRun } from "../_core-helpers.mjs"; -import { requireTokenValue } from "../_cli-primitives.mjs"; +import { requireTokenValue, runChild } from "../_cli-primitives.mjs"; import { teardown } from "@dev-loops/core/loop/ui-review-teardown"; import { cleanupWorktree } from "./cleanup-worktree.mjs"; import { JQ_OUTPUT_PARSE_OPTIONS, JQ_OUTPUT_USAGE, emitResult, matchJqOutputToken } from "../lib/jq-output.mjs"; @@ -24,22 +26,24 @@ import { JQ_OUTPUT_PARSE_OPTIONS, JQ_OUTPUT_USAGE, emitResult, matchJqOutputToke const MAX_RESULT_BYTES = 16 * 1024 * 1024; const USAGE = `Usage: - ui-review-teardown.mjs --repo-root

--provision-result

[--drive-result

] [--row-manifest

] [--confirm] [--no-stop-app] + ui-review-teardown.mjs --repo-root

--provision-result

[--drive-result

] [--report-result

] [--row-manifest

] [--confirm] [--no-stop-app] Tear down the ui_review route's transient state and emit a side-effect ledger (Stage 5 of the ui_review route). Destructive steps (row drops, worktree -removal) run ONLY with --confirm; the ledger is emitted in every case. +removal, hosting-gist deletion) run ONLY with --confirm; the ledger is emitted +in every case. Required: --repo-root

Absolute path to the primary checkout (git cwd for worktree removal). --provision-result

Path to the Stage-1 provision JSON (boot.pid, migrations, worktreePath). Optional: --drive-result

Path to the Stage-2 drive JSON (the rows-created signal). + --report-result

Path to the Stage-4 report JSON (its hosting.gist id, pruned with --confirm). --row-manifest

Path to a JSON array of rows to drop (only used with --confirm). - --confirm Authorize the destructive steps (row drop + worktree removal). + --confirm Authorize the destructive steps (row drop + worktree removal + gist deletion). --no-stop-app Do NOT stop the app process (clean shutdown otherwise runs regardless of --confirm). -h, --help Show this help. Output (stdout, JSON): { "ok": bool, "confirmed": bool, "errors": [...], "logs": [...], - "ledger": { "migrations": {...}, "rows": {...}, "worktree": {...}, "process": {...} } } + "ledger": { "migrations": {...}, "rows": {...}, "worktree": {...}, "gist": {...}, "process": {...} } } ${JQ_OUTPUT_USAGE}`.trim(); @@ -48,7 +52,7 @@ const parseError = buildParseError(USAGE); export function parseUiReviewTeardownCliArgs(argv) { const options = { help: false, repoRoot: undefined, provisionResult: undefined, driveResult: undefined, - rowManifest: undefined, confirm: false, stopApp: true, + reportResult: undefined, rowManifest: undefined, confirm: false, stopApp: true, }; const { tokens } = parseArgs({ args: [...argv], @@ -57,6 +61,7 @@ export function parseUiReviewTeardownCliArgs(argv) { "repo-root": { type: "string" }, "provision-result": { type: "string" }, "drive-result": { type: "string" }, + "report-result": { type: "string" }, "row-manifest": { type: "string" }, confirm: { type: "boolean" }, "no-stop-app": { type: "boolean" }, @@ -73,6 +78,7 @@ export function parseUiReviewTeardownCliArgs(argv) { if (token.name === "repo-root") { options.repoRoot = requireTokenValue(token, parseError, { flagPattern: /^-/u }); continue; } if (token.name === "provision-result") { options.provisionResult = requireTokenValue(token, parseError, { flagPattern: /^-/u }); continue; } if (token.name === "drive-result") { options.driveResult = requireTokenValue(token, parseError, { flagPattern: /^-/u }); continue; } + if (token.name === "report-result") { options.reportResult = requireTokenValue(token, parseError, { flagPattern: /^-/u }); continue; } if (token.name === "row-manifest") { options.rowManifest = requireTokenValue(token, parseError, { flagPattern: /^-/u }); continue; } if (token.name === "confirm") { // Bare flag or `=true` confirms; explicit `=false`/`=0`/`=no` must NOT @@ -205,12 +211,31 @@ function removeWorktree(repoRoot, { worktreePath }) { return Promise.resolve({ removed: res.removed, ok: res.ok, detail: res.reason }); } -export async function runCli(argv = process.argv.slice(2), { stdout = process.stdout, stderr = process.stderr } = {}) { +/** Extract the Stage-4 hosting gist ({id,url}) from a report result, or null. + * Only the GitHub-native path publishes a gist; any other hosting has none. */ +function gistFromReport(reportResult) { + const hosting = reportResult?.hosting; + if (hosting?.hosting !== "github-gist") return null; + const g = hosting.gist; + const id = typeof g?.id === "string" && g.id.trim().length > 0 ? g.id.trim() : null; + return id ? { id, url: g.url ?? null } : null; +} + +/** Delete a hosting gist via `gh gist delete`. Maps a non-zero exit / thrown + * spawn to the teardown seam's fail-reported contract (never swallowed). */ +async function deleteGist({ id }, { run = runChild } = {}) { + const res = await run("gh", ["gist", "delete", id], process.env); + if (res.code === 0) return { ok: true, detail: "deleted via gh gist delete" }; + return { ok: false, detail: `gh gist delete exit ${res.code}: ${res.stderr?.trim() || "no stderr"}` }; +} + +export async function runCli(argv = process.argv.slice(2), { stdout = process.stdout, stderr = process.stderr, run = runChild } = {}) { const options = parseUiReviewTeardownCliArgs(argv); if (options.help) { stdout.write(`${USAGE}\n`); return; } const provisionResult = readResultJson(options.provisionResult); const driveResult = readResultJson(options.driveResult); + const reportResult = readResultJson(options.reportResult); const rowManifest = readRowManifest(options.rowManifest); const result = await teardown( @@ -218,11 +243,13 @@ export async function runCli(argv = process.argv.slice(2), { stdout = process.st provisionResult, driveResult, rowManifest, + gist: gistFromReport(reportResult), confirm: options.confirm, stopApp: options.stopApp, }, { killProcess, + deleteGist: (a) => deleteGist(a, { run }), // No real drop seam yet: Stage 2 does not tag rows, so an actual manifest // never arrives today and the ledger reports "may remain (untagged)". A // manifest handed in is dropped by whatever project mechanism supplies it; diff --git a/test/loop/ui-review-report.test.mjs b/test/loop/ui-review-report.test.mjs index 6c8f18cf..6f9cd047 100644 --- a/test/loop/ui-review-report.test.mjs +++ b/test/loop/ui-review-report.test.mjs @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { mkdtempSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -14,7 +14,8 @@ import { } from "@dev-loops/core/loop/ui-review-report"; import { buildDraftReviewPayload } from "@dev-loops/core/loop/reviewer-loop-state"; import { containsBareCopilotSummon } from "@dev-loops/core/github/copilot-helpers"; -import { parseUiReviewReportCliArgs, loadScreenshot } from "../../scripts/loop/ui-review-report.mjs"; +import { parseUiReviewReportCliArgs, loadScreenshot, publishGist, runCli } from "../../scripts/loop/ui-review-report.mjs"; +import { makeGhMock } from "../_helpers.mjs"; const HEAD_SHA = "a".repeat(40); @@ -211,17 +212,18 @@ test("severityToEvent: confirmed 500 stays PENDING unless submit is authorized", assert.equal(noteOnly.blocking, false); }); -test("decideHosting: Claude harness -> publishable directive; off-Claude -> fail closed with reason + follow-up", () => { +test("decideHosting: Claude harness -> Artifacts directive; off-Claude -> portable GitHub-native gist strategy", () => { const onClaude = decideHosting({ htmlPath: "/out/report.html", env: { CLAUDECODE: "1" } }); assert.equal(onClaude.hosting, "claude-artifact"); assert.equal(onClaude.publishable, true); assert.equal(onClaude.htmlPath, "/out/report.html"); + // Off-Claude is NOT harness-specific and never fails closed in the pure layer: + // it selects the portable GitHub-native gist strategy the CLI then publishes. const offClaude = decideHosting({ htmlPath: "/out/report.html", env: {} }); - assert.equal(offClaude.hosting, "unavailable"); - assert.equal(offClaude.publishable, false); - assert.ok(typeof offClaude.reason === "string" && offClaude.reason.length > 0); - assert.equal(offClaude.followup, "#1285"); + assert.equal(offClaude.hosting, "github-gist"); + assert.equal(offClaude.publishable, true); + assert.equal(offClaude.htmlPath, "/out/report.html"); }); test("buildArtifactHtml: self-contained + CSP-safe, inlines screenshot, logs caps", () => { @@ -301,14 +303,39 @@ test("artifactBodyLine/verdict: hosted URL vs off-Claude follow-up, and empty/no }); assert.match(hosted.summaryFindings[0].message, /https:\/\/example\.test\/report\.html/); - // Off-Claude with no hosted URL states the fail-closed reason + follow-up marker. - const offClaude = buildReviewInput({ + // Off-Claude gist: the body links the gist URL, surfaces the raw URL, and states + // it renders as source. + const gistHosted = buildReviewInput({ findings: [], headSha: HEAD_SHA, - hosting: decideHosting({ htmlPath: "/out/report.html", env: {} }), + hosting: { hosting: "github-gist", gist: { id: "abc123", url: "https://gist.github.com/u/abc123", rawUrl: "https://gist.github.com/u/abc123/raw" } }, + hostedUrl: "https://gist.github.com/u/abc123", }); - assert.match(offClaude.summaryFindings[0].message, /unhosted/i); - assert.match(offClaude.summaryFindings[0].message, /#1285/); + assert.match(gistHosted.summaryFindings[0].message, /gist\.github\.com\/u\/abc123/); + assert.match(gistHosted.summaryFindings[0].message, /renders as source/i); + assert.match(gistHosted.summaryFindings[0].message, /raw: https:\/\/gist\.github\.com\/u\/abc123\/raw/); + + // Explicit --hosted-url override (strategy still github-gist, but NO published + // gist): the self-hosted URL is NOT mislabeled as a source-rendered gist. + const selfHosted = buildReviewInput({ + findings: [], + headSha: HEAD_SHA, + hosting: { hosting: "github-gist" }, + hostedUrl: "https://pages.example.test/report.html", + }); + assert.equal(selfHosted.summaryFindings[0].message, "Screenshot artifact: https://pages.example.test/report.html"); + assert.doesNotMatch(selfHosted.summaryFindings[0].message, /renders as source/i); + + // Fail-closed hosting (gist publish failed): body states unhosted + the reason, + // and never links a fabricated URL. + const failClosed = buildReviewInput({ + findings: [], + headSha: HEAD_SHA, + hosting: { hosting: "unavailable", publishable: false, reason: "gist publish failed: boom" }, + }); + assert.match(failClosed.summaryFindings[0].message, /unhosted/i); + assert.match(failClosed.summaryFindings[0].message, /gist publish failed/i); + assert.doesNotMatch(failClosed.summaryFindings[0].message, /https?:\/\//); // Zero findings -> APPROVE, severity none. assert.equal(hosted.verdict, "APPROVE"); @@ -374,6 +401,94 @@ test("loadScreenshot: raw-file cap agrees with the data-URI budget (no read-then assert.equal(caps.length, 0); }); +test("publishGist: GitHub-native fallback yields a real per-run URL + id from the gist stdout (no live API)", async () => { + const gistUrl = "https://gist.github.com/octocat/abc123def456"; + const { runChild, calls } = makeGhMock([ + { stdout: `${gistUrl}\n`, assertArgs: ["gist", "create"] }, + ]); + const gist = await publishGist({ htmlPath: "/out/report.html", pr: 42, run: runChild }); + + assert.equal(gist.url, gistUrl); + assert.equal(gist.id, "abc123def456"); + assert.equal(gist.rawUrl, `${gistUrl}/raw`); + // The self-contained HTML file is what gets published. + assert.ok(calls[0].args.includes("/out/report.html")); +}); + +test("publishGist: fails closed on a non-zero gh exit — no fabricated link", async () => { + const { runChild } = makeGhMock([ + { exitCode: 1, stderr: "HTTP 401: Bad credentials (missing gist scope)\n" }, + ]); + await assert.rejects( + publishGist({ htmlPath: "/out/report.html", pr: 42, run: runChild }), + /gh gist create failed.*gist scope/i, + ); +}); + +test("publishGist: fails closed when gh returns no URL — never links empty/garbage", async () => { + const { runChild } = makeGhMock([{ stdout: "created but no url line\n" }]); + await assert.rejects( + publishGist({ htmlPath: "/out/report.html", pr: 42, run: runChild }), + /did not return a gist URL/i, + ); +}); + +// Off-Claude runCli end to end (no live API): the info/head + review-poster +// spawns are injected, and the sole gh call (gist create) is a makeGhMock. This +// covers the wiring decideHosting(github-gist) -> publishGist -> hostedUrl -> +// buildReviewInput body, plus the fail-closed catch. +function writeDiagnose(dir) { + const diagPath = path.join(dir, "diagnose.json"); + writeFileSync(diagPath, JSON.stringify({ + pr: { headSha: HEAD_SHA }, + counts: { total: 1, anchorable: 0, nonAnchorable: 1 }, + findings: [FINDINGS[2]], + })); + return diagPath; +} + +test("runCli off-Claude: publishes a gist and the review body links the gist + raw URL (no live API)", async () => { + const dir = mkdtempSync(path.join(tmpdir(), "ui-review-runcli-")); + const diagPath = writeDiagnose(dir); + const htmlPath = path.join(dir, "report.html"); + const gistUrl = "https://gist.github.com/octocat/abc123def456"; + const { runChild } = makeGhMock([{ stdout: `${gistUrl}\n`, assertArgs: ["gist", "create"] }]); + + const sink = { write: () => true }; + await runCli( + ["--pr", "42", "--repo", "o/n", "--diagnose-result", diagPath, "--html-output", htmlPath], + { stdout: sink, stderr: sink, env: {}, run: runChild, loadLiveHead: () => null, postReview: () => ({ reviewId: 1, reviewUrl: "u", commitSha: HEAD_SHA }) }, + ); + + const reviewInput = JSON.parse(readFileSync(`${htmlPath}.review.json`, "utf8")); + const artifactLine = reviewInput.summaryFindings[0].message; + assert.match(artifactLine, new RegExp(gistUrl.replace(/\//g, "\\/"))); + assert.match(artifactLine, /raw: .*abc123def456\/raw/); + assert.match(artifactLine, /renders as source/i); +}); + +test("runCli off-Claude: a failed gist publish fails closed — unhosted body, stated reason, no fabricated link", async () => { + const dir = mkdtempSync(path.join(tmpdir(), "ui-review-runcli-fail-")); + const diagPath = writeDiagnose(dir); + const htmlPath = path.join(dir, "report.html"); + const { runChild } = makeGhMock([{ exitCode: 1, stderr: "HTTP 401: missing gist scope\n" }]); + + let errText = ""; + const stderr = { write: (s) => { errText += s; return true; } }; + await runCli( + ["--pr", "42", "--repo", "o/n", "--diagnose-result", diagPath, "--html-output", htmlPath], + { stdout: { write: () => true }, stderr, env: {}, run: runChild, loadLiveHead: () => null, postReview: () => ({ reviewId: 1, reviewUrl: "u", commitSha: HEAD_SHA }) }, + ); + + const reviewInput = JSON.parse(readFileSync(`${htmlPath}.review.json`, "utf8")); + const artifactLine = reviewInput.summaryFindings[0].message; + assert.match(artifactLine, /unhosted/i); + assert.match(artifactLine, /gist publish failed/i); + assert.doesNotMatch(artifactLine, /https?:\/\//); + // The failure is logged as a cap, never swallowed. + assert.match(errText, /hosting: GitHub-native gist publish failed/i); +}); + test("buildArtifactHtml: oversized screenshot is omitted and logged, never silently dropped", () => { const big = "data:image/png;base64," + "A".repeat(10 * 1024 * 1024); const { html, caps } = buildArtifactHtml({ diff --git a/test/loop/ui-review-teardown.test.mjs b/test/loop/ui-review-teardown.test.mjs index fd5fabb5..8681f134 100644 --- a/test/loop/ui-review-teardown.test.mjs +++ b/test/loop/ui-review-teardown.test.mjs @@ -5,7 +5,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; -import { teardown, ROW_STATUS, WORKTREE_STATUS, PROCESS_STATUS } from "@dev-loops/core/loop/ui-review-teardown"; +import { teardown, ROW_STATUS, WORKTREE_STATUS, PROCESS_STATUS, GIST_STATUS } from "@dev-loops/core/loop/ui-review-teardown"; import { parseUiReviewTeardownCliArgs, killProcess, runCli } from "../../scripts/loop/ui-review-teardown.mjs"; // A Stage-1 provision result: a booted app (pid), applied migrations, a worktree. @@ -23,8 +23,11 @@ const DRIVE_STOPPED = { ok: false, stopped: true, steps: [] }; // Recording seams: every destructive seam logs whether it was called, so a test // can assert the confirmation gate actually blocked (not merely no-op'd) a step. +// A Stage-4 report result carrying a published GitHub-native hosting gist. +const REPORT_WITH_GIST = { ok: true, hosting: { hosting: "github-gist", gist: { id: "abc123def456", url: "https://gist.github.com/u/abc123def456" } } }; + function makeSeams(overrides = {}) { - const calls = { kill: 0, drop: 0, removeWorktree: 0 }; + const calls = { kill: 0, drop: 0, removeWorktree: 0, deleteGist: 0 }; const seams = { killProcess: async () => { calls.kill += 1; @@ -38,6 +41,10 @@ function makeSeams(overrides = {}) { calls.removeWorktree += 1; return { removed: "/repo/tmp/worktrees/dev-loops/pr-7", ok: true, detail: "removed" }; }, + deleteGist: async () => { + calls.deleteGist += 1; + return { ok: true, detail: "deleted via gh gist delete" }; + }, log: () => {}, ...overrides, }; @@ -47,27 +54,30 @@ function makeSeams(overrides = {}) { test("teardown always emits a ledger enumerating every known side effect (confirmed success)", async () => { const { seams, calls } = makeSeams(); const res = await teardown( - { provisionResult: PROVISION, driveResult: DRIVE_WITH_STEPS, rowManifest: [{ table: "users", id: 1 }], confirm: true }, + { provisionResult: PROVISION, driveResult: DRIVE_WITH_STEPS, rowManifest: [{ table: "users", id: 1 }], gist: REPORT_WITH_GIST.hosting.gist, confirm: true }, seams, ); assert.equal(res.ok, true); assert.equal(res.confirmed, true); - // Ledger enumerates ALL four side-effect categories. + // Ledger enumerates ALL side-effect categories. assert.ok(res.ledger.migrations, "migrations enumerated"); assert.ok(res.ledger.rows, "rows enumerated"); assert.ok(res.ledger.worktree, "worktree enumerated"); + assert.ok(res.ledger.gist, "gist enumerated"); assert.ok(res.ledger.process, "process enumerated"); // Migrations recorded as applied-not-reverted (non-goal: no rollback). assert.equal(res.ledger.migrations.applied, 2); assert.equal(res.ledger.migrations.reverted, false); - // Destructive steps ran (confirmed): app stopped, rows dropped, worktree removed. + // Destructive steps ran (confirmed): app stopped, rows dropped, worktree removed, gist deleted. assert.equal(res.ledger.process.status, PROCESS_STATUS.STOPPED); assert.equal(res.ledger.rows.status, ROW_STATUS.DROPPED); assert.equal(res.ledger.rows.dropped, 1); assert.equal(res.ledger.worktree.status, WORKTREE_STATUS.REMOVED); assert.equal(res.ledger.worktree.path, PROVISION.worktreePath); - assert.deepEqual(calls, { kill: 1, drop: 1, removeWorktree: 1 }); + assert.equal(res.ledger.gist.status, GIST_STATUS.DELETED); + assert.equal(res.ledger.gist.id, "abc123def456"); + assert.deepEqual(calls, { kill: 1, drop: 1, removeWorktree: 1, deleteGist: 1 }); }); test("confirmation gate blocks destructive steps (no --confirm): app stops, row-drop + worktree-removal do NOT run, ledger still emitted", async () => { @@ -364,6 +374,61 @@ test("runCli: present-but-malformed --row-manifest fails closed with a clear par ); }); +test("gist prune: no gist in the report result => GIST_STATUS.NONE, deleteGist not called", async () => { + const { seams, calls } = makeSeams(); + const res = await teardown({ provisionResult: PROVISION, confirm: true }, seams); + assert.equal(res.ledger.gist.status, GIST_STATUS.NONE); + assert.equal(calls.deleteGist, 0); +}); + +test("gist prune: unconfirmed retains the gist (SKIPPED_UNCONFIRMED), deleteGist not called", async () => { + const { seams, calls } = makeSeams(); + const res = await teardown({ provisionResult: PROVISION, gist: REPORT_WITH_GIST.hosting.gist, confirm: false }, seams); + assert.equal(res.ledger.gist.status, GIST_STATUS.SKIPPED_UNCONFIRMED); + assert.equal(res.ledger.gist.id, "abc123def456"); + assert.equal(calls.deleteGist, 0); + assert.equal(res.ok, true); +}); + +test("gist prune: a failed delete is reported (DELETE_FAILED, ok:false), never swallowed", async () => { + const { seams } = makeSeams({ + deleteGist: async () => ({ ok: false, detail: "gh gist delete exit 1: not found" }), + }); + const res = await teardown({ provisionResult: PROVISION, gist: REPORT_WITH_GIST.hosting.gist, confirm: true }, seams); + assert.equal(res.ledger.gist.status, GIST_STATUS.DELETE_FAILED); + assert.equal(res.ok, false); + assert.ok(res.errors.some((e) => /gist delete FAILED/i.test(e))); +}); + +test("a throwing deleteGist seam still yields a fully-emitted ledger (DELETE_FAILED, ok:false)", async () => { + const { seams } = makeSeams({ + deleteGist: async () => { throw new Error("network down"); }, + }); + const res = await teardown({ provisionResult: PROVISION, gist: REPORT_WITH_GIST.hosting.gist, confirm: true }, seams); + assert.equal(res.ledger.gist.status, GIST_STATUS.DELETE_FAILED); + assert.match(res.ledger.gist.detail, /deleteGist seam threw: network down/); + assert.equal(res.ok, false); +}); + +test("runCli: prunes the report-result gist via gh gist delete (confirmed), no live API", async () => { + const dir = mkdtempSync(join(tmpdir(), "teardown-gist-")); + const provisionPath = join(dir, "provision.json"); + const reportPath = join(dir, "report.json"); + writeFileSync(provisionPath, JSON.stringify({ ...PROVISION, worktreePath: undefined, boot: {} })); + writeFileSync(reportPath, JSON.stringify(REPORT_WITH_GIST)); + + const ghCalls = []; + const run = async (cmd, args) => { ghCalls.push([cmd, ...args]); return { code: 0, stdout: "", stderr: "" }; }; + + let out = ""; + const sink = { write: (s) => { out += s; return true; } }; + const argv = ["--repo-root", "/r", "--provision-result", provisionPath, "--report-result", reportPath, "--confirm", "--no-stop-app", "--silent"]; + await runCli(argv, { stdout: sink, stderr: sink, run }); + + // The gist id from the report result is deleted via gh gist delete (mock, no live API). + assert.deepEqual(ghCalls[0], ["gh", "gist", "delete", "abc123def456"]); +}); + test("parseUiReviewTeardownCliArgs: requires --repo-root and --provision-result", () => { assert.throws(() => parseUiReviewTeardownCliArgs(["--provision-result", "/p.json"]), /repo-root/); assert.throws(() => parseUiReviewTeardownCliArgs(["--repo-root", "/r"]), /provision-result/);