review prompt: friendly green callouts + per-section severity emojis (#756)

* review prompt: friendly green callouts + per-section severity emojis - Replace `[!NOTE]` informational tier and the no-callout minor-suggestions tier with friendly green blockquotes (`> ✅` / `> 💡`). The two loud tiers (`[!CAUTION]` / `[!IMPORTANT]`) keep their GitHub admonitions. - Add a per-`##`-section severity-emoji rule (🚨/⚠️/💡/ℹ️) for cross-cutting review concerns that don't anchor to a line and would otherwise be buried in summary content. - Drop the `<br/>` between summary sections — heading + blank line carries enough visual spacing. - Skip the post-run learnings-reflection turn for `IncrementalReview`. It's the lowest-novelty mode (delta review against existing PR with prior summary already loaded) and almost never produces durable learnings — reflection there costs ~$0.50-0.80/run for nothing. - Surface real error info on `agent-browser` skill install failures (exit code + stdout + stderr + spawn error). The skills CLI uses a TUI that prints errors to stdout, so the prior stderr-only logging silently swallowed every failure. * review prompt: per-bullet severity emoji + bullets-only sections Section headings are plain again (no leading severity emoji). Severity moves to individual bullets so a section that mixes a 🚨 and a 💡 isn't mislabeled by either. Section bodies are now bullets only — paragraph prose under a heading is harder to scan and tends to bury the actionable point. Bullets can carry indented continuation content (sub-bullets, code fences, blockquotes) by indenting two spaces under the parent. * review prompt: cap section length + identifier discipline Bound each summary section to at most 4 bullets at most 2 lines each, and explicitly call out identifier-heavy prose as an anti-pattern. The reader is often a manager or non-author; identifier-dense paragraphs ('foo calls bar.fetch which dispatches to baz via qux...') are unreadable for them. Default to plain-language behavior descriptions, name an identifier only when it's the subject of an actionable concern or a public surface a reader would recognize, target 2-3 backtick tokens per bullet. Move the deep-explanation pattern from open blockquote to a default- collapsed details/summary so depth doesn't dominate the visible body. * review prompt: hard cap on bullet identifier density + worked rewrite example Soft 'aim for 2-3 tokens' guidance was ignored — first big-PR e2e showed 12 of 19 actionable bullets exceeded the target (avg 4.8 tokens, several over 8). Promote to a hard cap of 3 backticked tokens per bullet and pair with a concrete bad/good rewrite the agent can pattern- match against. Also tighten the per-bullet length cap from ~240 to ~200 chars and explicitly call it 'hard cap, not target'. * review prompt: tighten bullet length cap to 160 chars, dramatize the worked example V2 e2e test: token discipline improved (4.8 -> 3.3 avg, 12/19 -> 6/14 violations) but length got worse (235 -> 286 chars, 13/14 over the 200 cap). The agent compensated for fewer identifiers with more prose. Two changes: (1) tighten the cap from ~200 chars to 160 chars / 1 visual line and call out wrap-to-multiple-lines as the failure mode; (2) rewrite the worked example so the good version is genuinely half the length of the bad one, not just lower token count. The example was the thing the agent pattern-matches against; making the good version ~130 chars vs the bad version's ~290 chars sets the right shape. * review prompt: drop fixed bullet-count cap, keep length + identifier caps Per user feedback — section length should be governed by content, not an arbitrary count. Soft guidance ('past ~6, ask whether to split') is fine; the hard '≤ 4 bullets per section' rule was the wrong shape. Length cap (160c) and identifier cap (3 backtick tokens) stay; those target the actual scanability problem. * review prompt: drop ## subsystem sections, flat 'Issues found' list Per-section structure forced every concern into a subsystem frame and made the body read like a series of mini-essays. Replace with two parts: (1) TL;DR + Key changes as the dispassionate overview, (2) flat '### Issues found' list ordered by severity, intermixed across files and subsystems. Per-bullet rules (≤160c, ≤3 backtick tokens, severity emoji prefix, optional indented continuation) carry over unchanged. * review prompt: full v6 structure — preamble + cross-cutting H3s + nitpicks Replaces the flat 'Issues found' bullet list with the iterated v6 shape: - Preamble is a bolded inline 'Reviewed changes' lead-in plus bullets plus a collapsed 'Review metadata' block (mode/files/commits/refs/ reviewed commits list/prior pullfrog review/staleness note). - Each cross-cutting concern gets a '### emoji Title' section. The visible problem write-up is human-friendly and DESCRIBES THE PROBLEM ONLY — no asks, no suggested fixes, no 'the right thing to do is'. - Each section carries a collapsed 'Technical details' block wrapped in a 4-backtick markdown fence (so it can hold its own 3-tick code fences cleanly, agent-readable, one-click copyable). Standard four inner sections: Affected sites, Required outcome, optional Suggested approach, optional Open questions for the human. - '### ℹ️ Nitpicks' at the bottom for body-only nits that don't inline; simple bullets, no technical-details collapse. - Anti-paragraph-wall rule: never two successive plain paragraphs in visible '### ' sections; alternate prose with structure. - Inline-vs-body discipline: anything that anchors to a single line goes inline, body is for cross-cutting only. - Drops legacy '### Key changes', '### Issues found', '<b>TL;DR</b>', and the '<sub>Summary</sub>' line. * model effort: bump Gemini + GPT to high effort; drop Gemini Pro→Flash subagent E2E review eval against a substantive billing-module diff surfaced two related quality gaps: 1. Gemini Pro at thinkingLevel=medium (#663's CI-timeout fix) reviewed the diff only, took the 0-lens path, and missed a catastrophic camelCase/snake_case service-vs-schema mismatch. Bumping back to high — review work is exactly the wrong shape for the medium/high tradeoff #663 was optimizing for; the per-turn TTFT cost is worth paying when reasoning IS the value. 2. GPT had no reasoningEffort override, defaulting to upstream medium. Same diff, similar shallow result vs Claude. Adding reasoningEffort: high for the curated direct-OpenAI slugs, mirroring the Gemini pattern (Anthropic separately uses --effort high via the Claude Code CLI flag in claude.ts). 3. Gemini Pro's subagentModel was 'gemini-flash' — but Google has no in-between tier between Pro and Flash, and Flash is a meaningful capability cliff for review work. Dropping the override so subagents inherit Pro. Cost stays reasonable since Gemini Pro is already the cheapest of the flagship trio. Other providers unchanged: Anthropic opus→sonnet and OpenAI gpt→gpt-5.4 remain (each is a one-tier drop to a still-capable sibling). * model effort: revert orchestrator override, set explicit high on reviewfrog subagent Reshape the effort design after eval: - Drop the explicit Gemini and GPT model-level overrides — orchestrators now run at upstream defaults (Gemini high, GPT-5.x medium). Gemini's upstream IS high, so this is a no-op there; GPT goes back to upstream medium for orchestrator-level routing work. - Add explicit 'high' on the reviewfrog subagent via agent.options. OpenCode merge order is base ← model.options ← agent.options ← variant per session/llm.ts:141, so the subagent always runs at high regardless of which orchestrator dispatched it. Both thinkingConfig.thinkingLevel (Gemini) and reasoningEffort (GPT) keys included; irrelevant keys are ignored per provider. - Bump providers-live timeouts (12min job / 10min step, from 8/6) to budget for Gemini's TTFT variance at high effort. #663's 4min timeout was sized for the medium-effort override that's now removed. * model effort: restore Gemini explicit high override (no-override path breaks) Bare 'rely on upstream default' for Gemini failed in e2e — removing the model-level provider config produced 'Function call is missing a thought_signature' API errors on every gemini-pro run. Even though upstream opencode's options() returns the same thinkingLevel: high we were explicitly setting, opencode's resolution path differs subtly between the two cases. v2's explicit override worked; v3's removal broke. Reproducible across two consecutive runs. Restoring the explicit Gemini override (back to v2 design). GPT orchestrator stays UN-overridden — at upstream default (medium) — since removing that override didn't trigger the same failure pattern and the reviewfrog subagent agent.options high override compensates for the extra depth GPT loses at medium. * diag: remove reviewfrog agent.options to isolate Gemini thought_signature failure v3 (no Gemini orch override) failed with thought_signature error. v4 (restored Gemini orch override at v2-equivalent) ALSO failed, even though the orchestrator config matches v2. The variable between v2 (working) and v4 (failing) is the new reviewfrog agent.options block. Removing it to confirm — if Gemini works again, the agent.options addition is the culprit and we need a different shape for it. * opencode-ai: bump 1.1.56 → 1.15.0 + clean up gemini effort config opencode-ai@1.1.56 was published 2026-02-10 (3 months old). The Google API tightened thought_signature validation 24-48h ago (per https://discuss.ai.google.dev/t/gemini-thought-signature-patch/122555), and the bug class hits opencode's session→prompt serializer for MCP tool-call parts (anomalyco/opencode#4832, #8321). Latest stable bumps us through ~3 months of fixes; needed for Gemini-direct to stop dying with 'thought_signature is missing' on every multi-turn run. Companion cleanup: the gemini provider override in opencode.ts had 30-line block of comments, four unused constants, and a 6-line Object.fromEntries map for two entries. Replaced with one source-of- truth helper that loops modelAliases, filters provider==='google', strips the 'google/' prefix, and returns the override map. Adding any future Google alias to the registry now flows through automatically. Test added: action/agents/opencode.test.ts asserts the helper covers every direct-Google alias, strips the prefix correctly, and pins every entry to thinkingLevel high — catches drift in helper logic without hardcoding the API ids the test would have to update in lockstep with the registry. * fix(workflow): tolerate listJobsForWorkflowRun 404 in resolveRun PR #750 (docker testing rewrite) replaced the per-call env allowlist with full process.env passthrough into the test container. That now leaks GITHUB_RUN_ID + GITHUB_JOB into runs whose MCP token is scoped to a DIFFERENT repo (e.g. providers-live smoke runs the action against pullfrog/test-repo with pullfrog/app's run ID). The unconditional listJobsForWorkflowRun call 404s and crashes the entire run, breaking every providers-live job on main since #750 landed. jobId is purely cosmetic (deep-links 'View workflow run' footer to a specific job vs the run-level URL). Wrapping the API call in try/catch so a 404 logs a debug message and falls through to undefined jobId is the right fix — the failure mode is exactly what graceful degradation is for, and the alternative (filter the env vars at the docker boundary) re-introduces the kind of allowlist #750 was getting rid of. * opencode-ai: pin 1.14.51 instead of 1.15.0 (effect refactor breaks JSON output) opencode 1.15.0 (May 15) ships a major architectural refactor onto @effect — the run command boots an in-process server via @opencode-ai/sdk/v2 and the JSON event emission path through that SDK client doesn't surface on stdout the way our parser expects (CI run on 1.15.0 produced 0 stdout events but the agent still completed). Local invocation also hangs at the in-process server boot. The Gemini thought_signature fixes (the original reason for bumping) landed earlier in the 1.14.x line, so 1.14.51 (May 14) gets us the upstream fix without the Effect rewrite. Defer the 1.15.x bump until we're ready to rewire our parser/spawn around the new SDK. * opencode-ai: revert to 1.1.56; gha: filter outer-CI workflow-run vars at the docker boundary Two related changes for the docker testing harness's ergonomics: 1. Revert opencode-ai 1.14.51 → 1.1.56. The 1.14+ line ships an Effect refactor (the SDK-v2 client + in-process server architecture) that our --format json parser doesn't speak — even the 1.14.51 release, pre-dating the 1.15.0 Effect rename, produced 0 stdout events on our skill-invoke smoke. There's no clean pre-Effect version that ships the Gemini thought_signature fix; that fix needs a separate workstream once we're ready to rewire the parser onto SDK v2. 2. Filter outer-CI workflow-run identifiers (GITHUB_RUN_ID, GITHUB_JOB, GITHUB_WORKFLOW, GITHUB_ACTION, GITHUB_REF, GITHUB_SHA, etc.) from gha.ts's --env-file passthrough. PR #750's full-process.env design leaks pullfrog/app's CI run identifiers into runs that act against a different repo (e.g. pullfrog/test-repo); any code path inside the action that uses them as keys (most notably resolveRun's listJobsForWorkflowRun lookup) 404s. Filtering them here means the action sees undefined and skips the lookup, complementing the defensive try/catch in resolveRun (commit addc76d4). GITHUB_REPOSITORY and GITHUB_TOKEN are NOT filtered — those are genuinely needed. Companion to addc76d4 (resolveRun 404 tolerance). The two together make this class of bug 'either fix would have caught it' rather than 'silently breaks the entire test matrix'. * fix(deps): sync pnpm-lock.yaml with opencode-ai 1.1.56 manifest revert Forgot to refresh the lockfile after reverting the manifest in 02c6d8c1. CI's frozen-lockfile install was failing with 'lockfile: 1.14.51, manifest: 1.1.56' mismatch.
2026-05-16 04:58:31 +00:00
parent a0dce200d0
commit ddbc610569
12 changed files with 285 additions and 118 deletions
@@ -33,7 +33,11 @@ import {
 import { ThinkingTimer } from "../utils/timer.ts";
 import type { TodoTracker } from "../utils/todoTracking.ts";
 import { getDevDependencyVersion } from "../utils/version.ts";
-import { buildLearningsReflectionPrompt, runPostRunRetryLoop } from "./postRun.ts";
+import {
+  buildLearningsReflectionPrompt,
+  runPostRunRetryLoop,
+  shouldRunReflection,
+} from "./postRun.ts";
 import { REVIEWER_AGENT_NAME, REVIEWER_SYSTEM_PROMPT } from "./reviewer.ts";
 import { formatWithLabel, ORCHESTRATOR_LABEL, SessionLabeler } from "./sessionLabeler.ts";
 import {
@@ -966,9 +970,10 @@ export const claude = agent({
      ctx,
      initialResult: result,
      initialUsage: result.usage,
-      reflectionPrompt: ctx.toolState.learningsFilePath
-        ? buildLearningsReflectionPrompt(ctx.toolState.learningsFilePath)
-        : undefined,
+      reflectionPrompt:
+        ctx.toolState.learningsFilePath && shouldRunReflection(ctx.toolState.selectedMode)
+          ? buildLearningsReflectionPrompt(ctx.toolState.learningsFilePath)
+          : undefined,
      canResume: (r) => Boolean(r.sessionId),
      resume: async (c) => {
        const sessionId = c.previousResult.sessionId;
@@ -0,0 +1,34 @@
+import { describe, expect, it } from "vitest";
+import { modelAliases } from "../models.ts";
+import { geminiHighThinkingOverrides } from "./opencode.ts";
+
+describe("geminiHighThinkingOverrides", () => {
+  // Expected truth pulled the same way the helper does — both must derive from
+  // the registry so the test exercises the wiring, not a hand-maintained list.
+  const expectedApiIds = modelAliases
+    .filter((a) => a.provider === "google")
+    .map((a) => a.resolve.replace(/^google\//, ""));
+  const overrides = geminiHighThinkingOverrides();
+
+  it("covers every direct-Google alias in the registry", () => {
+    expect(Object.keys(overrides).sort()).toEqual([...expectedApiIds].sort());
+  });
+
+  it("is non-empty (catches accidental whole-provider removal)", () => {
+    expect(Object.keys(overrides).length).toBeGreaterThan(0);
+  });
+
+  it("strips the `google/` prefix from each resolve to get the bare API id", () => {
+    for (const id of Object.keys(overrides)) {
+      expect(id).not.toMatch(/^google\//);
+    }
+  });
+
+  it("pins every entry to thinkingLevel: high", () => {
+    for (const [id, value] of Object.entries(overrides)) {
+      expect(value, `entry for ${id}`).toEqual({
+        options: { thinkingConfig: { thinkingLevel: "high" } },
+      });
+    }
+  });
+});
@@ -39,7 +39,11 @@ import {
  PULLFROG_OPENCODE_PLUGIN_FILENAME,
  PULLFROG_OPENCODE_PLUGIN_SOURCE,
 } from "./opencodePlugin.ts";
-import { buildLearningsReflectionPrompt, runPostRunRetryLoop } from "./postRun.ts";
+import {
+  buildLearningsReflectionPrompt,
+  runPostRunRetryLoop,
+  shouldRunReflection,
+} from "./postRun.ts";
 import { REVIEWER_AGENT_NAME, REVIEWER_SYSTEM_PROMPT } from "./reviewer.ts";
 import { formatWithLabel, ORCHESTRATOR_LABEL, SessionLabeler } from "./sessionLabeler.ts";
 import {
@@ -95,24 +99,20 @@ type OpenCodeConfig = {
 // on merge in session/llm.ts), so the env var is the only working knob.

 /**
- * upstream opencode hardcodes `thinkingLevel: "high"` as the default for every
- * gemini-3 model on the direct google SDK (`provider/transform.ts` `options()`).
- * that adds 30-60s of pre-tool-call TTFT and 5-46s of post-tool jabber per turn,
- * which is overkill for agentic loops where most steps are tool-routing
- * decisions. we override to "medium" for the curated slugs we ship in
- * `action/models.ts`; users who want max quality can still pick the `-high`
- * variant explicitly. flash stays at "medium" too — low-effort flash is
- * visibly worse on harder tasks and the latency savings aren't meaningful
- * (flash is already fast). other gemini-3 ids that exist in models.dev but
- * aren't in our curated alias map keep the upstream `"high"` default.
- *
- * keyed by upstream api id (matches the slugs in `action/models.ts`). the
- * merge order in opencode `session/llm.ts` is `base ← model.options ← agent.options ← variant`,
- * deep-merged — so an explicit `--variant high` still wins, and explicit
- * model.options in a user-provided opencode config would also win.
+ * Build the `provider.google.models[id].options` map that pins every direct-Google
+ * Gemini alias to `thinkingLevel: "high"`. Sourced from the model registry so
+ * adding/renaming a Google alias in `action/models.ts` flows through automatically.
 */
-const GEMINI_3_DIRECT_THINKING_LEVEL = "medium";
-const GEMINI_3_DIRECT_API_IDS = ["gemini-3.1-pro-preview", "gemini-3-flash-preview"];
+export function geminiHighThinkingOverrides(): Record<string, { options: object }> {
+  return Object.fromEntries(
+    modelAliases
+      .filter((a) => a.provider === "google")
+      .map((a) => [
+        a.resolve.replace(/^google\//, ""),
+        { options: { thinkingConfig: { thinkingLevel: "high" } } },
+      ])
+  );
+}

 function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): string {
  const config: OpenCodeConfig = {
@@ -142,20 +142,9 @@ function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): s
    // the "Parallel tool execution" guidance in utils/instructions.ts so the
    // model actually reaches for it. see wiki/prompt.md.
    experimental: { batch_tool: true },
-    provider: {
-      google: {
-        models: Object.fromEntries(
-          GEMINI_3_DIRECT_API_IDS.map((id) => [
-            id,
-            {
-              options: {
-                thinkingConfig: { thinkingLevel: GEMINI_3_DIRECT_THINKING_LEVEL },
-              },
-            },
-          ])
-        ),
-      },
-    },
+    // gemini-3 thinking pinned to high for review depth; gpt and anthropic
+    // effort set elsewhere (gpt: upstream default, anthropic: --effort flag in claude.ts).
+    provider: { google: { models: geminiHighThinkingOverrides() } },
  };

  if (model) {
@@ -1304,9 +1293,10 @@ export const opencode = agent({
      ctx,
      initialResult: result,
      initialUsage: result.usage,
-      reflectionPrompt: ctx.toolState.learningsFilePath
-        ? buildLearningsReflectionPrompt(ctx.toolState.learningsFilePath)
-        : undefined,
+      reflectionPrompt:
+        ctx.toolState.learningsFilePath && shouldRunReflection(ctx.toolState.selectedMode)
+          ? buildLearningsReflectionPrompt(ctx.toolState.learningsFilePath)
+          : undefined,
      resume: async (c) =>
        runOpenCode({
          ...runParams,
@@ -158,7 +158,7 @@ export function buildUnsubmittedReviewPrompt(mode: "Review" | "IncrementalReview
    return [
      `MISSING REVIEW OUTPUT — you selected Review mode but stopped without calling \`create_pull_request_review\`. the user has no visible signal that this run produced anything; the progress comment will be deleted on exit and no review will appear on the PR.`,
      "",
-      "call `create_pull_request_review` now with your aggregated review (body + inline comments). pick the tier per the mode prompt — Review mode has no no-submit exit, so even informational `> [!NOTE]` reviews and `No new issues found.` reviews must be submitted (both use `approved: true`). the first call may error once with a diff-coverage nudge — retry the same call to proceed.",
+      "call `create_pull_request_review` now with your aggregated review (body + inline comments). pick the tier per the mode prompt — Review mode has no no-submit exit, so even informational `> ✅ No new issues found.` reviews must be submitted (with `approved: true`). the first call may error once with a diff-coverage nudge — retry the same call to proceed.",
      "",
      "do NOT stop again until `create_pull_request_review` has been called successfully.",
    ].join("\n");
@@ -243,6 +243,24 @@ export function buildPostRunPrompt(issues: PostRunIssues): string {
  return parts.join("\n\n---\n\n");
 }

+/**
+ * modes for which the post-run reflection turn is skipped. reflection costs a
+ * full resume turn (~$0.50-0.80 per run on Opus, mostly cache-write) and only
+ * pays for itself when the run actually produced novel, durable findings.
+ *
+ * `IncrementalReview` is the lowest-novelty mode — it's a tight delta review
+ * against an existing PR with the prior summary already loaded as context.
+ * the agent rarely discovers anything generalizable to next runs, so the
+ * reflection turn is dead weight. initial `Review` still touches fresh PR
+ * territory and benefits; `Build` / `Fix` / `AddressReviews` definitely do.
+ */
+const REFLECTION_SKIP_MODES: ReadonlySet<string> = new Set(["IncrementalReview"]);
+
+export function shouldRunReflection(mode: string | undefined): boolean {
+  if (!mode) return true;
+  return !REFLECTION_SKIP_MODES.has(mode);
+}
+
 /**
 * prompt for a dedicated post-run reflection turn nudging the agent to edit
 * the rolling learnings file if it discovered anything worth persisting.
@@ -66,20 +66,23 @@ describe("deriveSubagentModels", () => {
    });
  });

-  describe("google (gemini) — pro → flash", () => {
-    it("direct google", () => {
+  describe("google (gemini) — inherit (Pro for both orchestrator and lenses)", () => {
+    // pro → flash was a meaningful capability cliff (Flash missed catastrophic
+    // cross-file bugs the v4 e2e test surfaced); Pro is cost-effective enough
+    // to keep on for lenses too. Google has no in-between tier.
+    it("direct google pro inherits", () => {
      expect(deriveSubagentModels("google/gemini-3.1-pro-preview")).toEqual({
-        reviewer: "google/gemini-3-flash-preview",
+        reviewer: undefined,
      });
    });
-    it("opencode-vendored gemini-pro", () => {
+    it("opencode-vendored gemini-pro inherits", () => {
      expect(deriveSubagentModels("opencode/gemini-3.1-pro")).toEqual({
-        reviewer: "opencode/gemini-3-flash",
+        reviewer: undefined,
      });
    });
-    it("openrouter-google-gemini-pro", () => {
+    it("openrouter gemini-pro inherits", () => {
      expect(deriveSubagentModels("openrouter/google/gemini-3.1-pro-preview")).toEqual({
-        reviewer: "openrouter/google/gemini-3-flash-preview",
+        reviewer: undefined,
      });
    });
    it("flash has no downshift", () => {
@@ -34,8 +34,11 @@ config({ path: join(actionDir, ".env") });
 config({ path: join(repoRoot, ".env") });

 // host env vars that would actively conflict with the container's own
-// configuration (paths, identity, shell). everything else passes through.
+// configuration (paths, identity, shell, and outer-CI workflow-run identifiers
+// that don't apply to whatever repo the harness is acting against). everything
+// else passes through.
 const HOST_ONLY_VARS = new Set([
+  // paths / identity / shell — would clobber the container's testuser setup
  "PATH",
  "HOME",
  "USER",
@@ -63,6 +66,25 @@ const HOST_ONLY_VARS = new Set([
  "COLORTERM",
  "ITERM_PROFILE",
  "ITERM_SESSION_ID",
+  // outer-CI workflow-run identifiers — when `pnpm runtest smoke` runs inside
+  // pullfrog/app's CI, these refer to pullfrog/app's run, NOT the test repo
+  // the harness is acting against (e.g. pullfrog/test-repo). Anything inside
+  // the action that uses them as keys to look up state on the test repo (most
+  // notably `resolveRun()`'s `actions.listJobsForWorkflowRun(...)` call) will
+  // 404. Filtering them here means the action sees them as undefined and
+  // skips the lookup, instead of misdirecting it. `GITHUB_REPOSITORY` and
+  // `GITHUB_TOKEN` are NOT filtered — those are genuinely needed inside.
+  "GITHUB_RUN_ID",
+  "GITHUB_RUN_NUMBER",
+  "GITHUB_RUN_ATTEMPT",
+  "GITHUB_JOB",
+  "GITHUB_WORKFLOW",
+  "GITHUB_ACTION",
+  "GITHUB_REF",
+  "GITHUB_SHA",
+  "GITHUB_HEAD_REF",
+  "GITHUB_BASE_REF",
+  "GITHUB_TRIGGERING_ACTOR",
 ]);

 type Args = {
@@ -316,7 +316,7 @@ export const CreatePullRequestReview = type({
    .optional(),
  approved: type.boolean
    .describe(
-      "Set to true to submit as an approval. Use for both 'no issues found' and informational `> [!NOTE]` reviews where the PR is mergeable as-is and nothing in the body warrants code changes — approving also suppresses the Fix-button footer affordance so users don't dispatch a fix run on non-actionable feedback. Reserve approved: false for `> [!IMPORTANT]` (recommended changes) and `> [!CAUTION]` (critical) reviews. Defaults to false (comment-only review). Rejections are not supported."
+      "Set to true to submit as an approval. Use for `> ✅ No new issues found.` reviews where the PR is mergeable as-is and nothing in the body warrants code changes — approving also suppresses the Fix-button footer affordance so users don't dispatch a fix run on non-actionable feedback. Reserve approved: false for `> ℹ️ ...` (minor suggestions inline), `> [!IMPORTANT]` (recommended changes), and `> [!CAUTION]` (critical) reviews. Defaults to false (comment-only review). Rejections are not supported."
    )
    .optional(),
  commit_id: type.string
@@ -850,8 +850,8 @@ async function createAndSubmitWithFooter(
  // introduce new throw paths. keep the whole body wrapped.
  try {
    // Fix buttons are suppressed on approving reviews — those are mergeable
-    // by definition (either "no issues found" or `> [!NOTE]` informational
-    // observations), so dispatching a fix run would be a UX trap.
+    // by definition (the `> ✅ No new issues found.` tier, with no inline
+    // comments), so dispatching a fix run would be a UX trap.
    const customParts: string[] = [];
    if (!opts.approved) {
      const apiUrl = getApiUrl();
@@ -170,7 +170,11 @@ export const providers = {
        resolve: "google/gemini-3.1-pro-preview",
        openRouterResolve: "openrouter/google/gemini-3.1-pro-preview",
        preferred: true,
-        subagentModel: "gemini-flash",
+        // Inherit (subagents stay on Pro). Google has no in-between tier;
+        // dropping to Flash for review work was a meaningful capability cliff
+        // (Flash missed the catastrophic camelCase/snake_case mismatch in
+        // the v4 e2e test). Pro is cost-effective enough to use for both
+        // orchestrator and lenses.
      },
      "gemini-flash": {
        displayName: "Gemini Flash",
@@ -319,7 +323,7 @@ export const providers = {
        displayName: "Gemini Pro",
        resolve: "opencode/gemini-3.1-pro",
        openRouterResolve: "openrouter/google/gemini-3.1-pro-preview",
-        subagentModel: "gemini-flash",
+        // Inherit — see google/gemini-pro for rationale.
      },
      "gemini-flash": {
        displayName: "Gemini Flash",
@@ -432,7 +436,7 @@ export const providers = {
        displayName: "Gemini Pro",
        resolve: "openrouter/google/gemini-3.1-pro-preview",
        openRouterResolve: "openrouter/google/gemini-3.1-pro-preview",
-        subagentModel: "gemini-flash",
+        // Inherit — see google/gemini-pro for rationale.
      },
      "gemini-flash": {
        displayName: "Gemini Flash",
@@ -18,52 +18,116 @@ export interface Mode {
 // shaped by user instructions — see selectMode.ts for the firewall.
 export const PR_SUMMARY_FORMAT = `### Default format

-Follow this structure exactly:
+The body has at most three parts in this exact order:

-<b>TL;DR</b> — 1-3 sentences on what the PR does and why. Focus on intent, not mechanics.
-NOTE: use HTML bold <b>TL;DR</b>, NOT markdown bold **TL;DR**.
+1. **Reviewed changes preamble** — one bolded inline lead-in describing what the PR does, then a bullet list of the substantive changes, then a collapsed \`<details>Review metadata</details>\` block.
+2. **Cross-cutting issue sections** (zero or more) — one \`### \` heading per concern, with a human-readable problem write-up and a collapsed \`<details>Technical details</details>\` block underneath.
+3. **\`### ℹ️ Nitpicks\`** at the very bottom (only if there are nits worth surfacing in the body) — a flat bullet list, no technical-details block.

-### Key changes
+Inline-vs-body discipline is the most important rule: any concern that anchors to a single line in the diff goes in an INLINE comment via the \`comments\` parameter, never the body. The body is reserved for **cross-cutting** concerns — design-level issues that span files, meta-patterns across endpoints, or symptoms whose root cause isn't on any one line. If everything you found can be inlined, the body has zero \`### \` issue sections — just the preamble + metadata.

- **Short human-readable title** — 1 sentence per change. Write a short prose phrase (title case or sentence case); when you name a file, type, or function, put that name in backticks (e.g. **Add \`TodoTracker\` for live checklists**). A reviewer should understand the full PR from this list alone.
+## 1. Reviewed changes preamble

-<sub><b>Summary</b> ｜ {file_count} files ｜ {commit_count} commits ｜ base: \`{base}\` ← \`{head}\`</sub>
-NOTE: the metadata line goes AFTER the bullet list, not before it.
+Open with a single bolded inline lead-in followed immediately by the bullet list (no \`### Key changes\` heading, no \`<b>TL;DR</b>\`):

-Then for each key change, a ## section with a short descriptive title that reads like a documentation heading (e.g. ## Live todo checklist tracking).
+\`\`\`
+**Reviewed changes** — one sentence on what the PR does and why. Focus on intent, not mechanics.

-<br/>
+- **Short human-readable title** — 1 sentence per substantive change. Write a short prose phrase; when you name a file, type, or function, put that name in backticks (e.g. **Add \\\`TodoTracker\\\` for live checklists**). A reviewer should understand the full PR from this list alone — this IS the dispassionate "what was reviewed and what changed" overview, so cover the substantive changes, not just the loudest ones.

-## Example readable section title
+<details><summary>Review metadata</summary>

-> **Before:** [old behavior/state]<br/>**After:** [new behavior/state]
-IMPORTANT: Before and After MUST be on a SINGLE blockquote line with an inline <br/> between them. Two separate \`>\` lines creates a double line break.
+- **Mode:** Review (initial)   *or*   IncrementalReview (delta against prior pullfrog review)
+- **Files reviewed:** {file_count}
+- **Commits reviewed:** {commit_count}
+- **Base:** \\\`{base_ref}\\\` (\\\`{base_sha_short}\\\`)
+- **Head:** \\\`{head_ref}\\\` (\\\`{head_sha_short}\\\`)
+- **Reviewed commits:**
+  - \\\`{sha_short}\\\` — {commit_subject}
+  - ...
+- **Prior pullfrog review:** none   *or*   \\\`{prior_sha_short}\\\` (linked to the prior review URL when available)
+- **Submitted at:** {iso_timestamp}

-1-2 sentences of explanation. Break up text with tables, blockquotes, or lists — NEVER 3+ plain paragraphs in a row.
+If \\\`HEAD\\\` of \\\`{head_ref}\\\` has advanced past \\\`{head_sha_short}\\\`, this review may be partially or fully stale — re-diff against \\\`{head_sha_short}\\\` before treating any technical-details block as current.

-If a change warrants deeper explanation, use a blockquoted details/summary framed as a question:
-> <details><summary>How does X work?</summary>
-> Extended explanation here.
-> </details>
+</details>
+\`\`\`

-End each section with a file links trail (3-4 key files max):
-[\`file.ts\`](https://github.com/{owner}/{repo}/pull/{number}/files#diff-{sha256hex_of_filepath}) · ...
+Pull every metadata field from the \`checkout_pr\` tool's response — file count, commit count, base/head ref + SHA, the commit list. For \`IncrementalReview\` runs, populate \`Prior pullfrog review\` with the prior review's commit_id (short SHA) and link to its URL via \`list_pull_request_reviews\`.

-Single-feature PRs: skip the ## sections. Fold before/after and explanation into the header after key changes.
+## 2. Cross-cutting issue sections (zero or more)

-CRITICAL — GitHub markdown rendering rule:
-GitHub's markdown parser requires a blank line between ALL block-level elements. This includes transitions between: HTML tags (<br/>, <sub>, <details>, <b>, etc.) and markdown syntax (headings, lists, blockquotes, paragraphs). Without a blank line, GitHub treats the following content as a continuation of the HTML block and renders markdown syntax as literal text. ALWAYS separate block-level elements with a blank line.
+For each cross-cutting concern, one \`### \` section. Use this exact shape:

-Rules:
- \`##\` titles and key-change bullet lead-ins are plain-language summaries; backtick only actual code tokens (files, types, functions) where they appear in the title
- ALL variable names, identifiers, and file names in body text must be in backticks
- ALL file references MUST link to the PR Files Changed view. Use the \`diff-<hex>\` anchor precomputed next to each filename in the \`checkout_pr\` TOC — do NOT run \`sha256sum\` or any other shell command to compute anchors. NEVER fabricate hex strings. If a file is not in the TOC, omit the \`#diff-\` anchor rather than guessing.
- Add <br/> before each ## heading for visual spacing. Do NOT use horizontal rules (---)
- Do NOT include raw diff stats like '+123 / -45' or line counts
- Do NOT include code blocks or repeat diff contents
- Do NOT include a changelog section — the key changes list serves this purpose
- Focus on *intent*, not *what* — the diff already shows what changed
- Get the file count and commit count from the checkout_pr metadata, not by counting manually`;
+\`\`\`
+### {emoji} {short, descriptive title — what's wrong, not what to do}
+
+{Human-readable problem write-up. Describes the PROBLEM only — what's broken, what the symptom is, what the blast radius is. NO asks, NO suggested fixes, NO "the right thing to do is...". Asks and fixes live in the technical-details block below; the visible part is for the human to *understand* the problem, not to implement it.}
+
+<details><summary>Technical details</summary>
+
+\\\`\\\`\\\`\\\`markdown
+# {title repeated}
+
+## Affected sites
+- {file path:line} — {what's wrong there}
+- ...
+
+## Required outcome
+- {what the fix needs to achieve, not how to achieve it}
+- ...
+
+## Suggested approach (optional)
+{When the fix shape is non-obvious, sketch one or more reasonable directions. Skip when the outcome alone makes the fix obvious.}
+
+## Open questions for the human (optional)
+- {Any decision an implementing agent shouldn't make unilaterally — pricing thresholds, breaking-change policy, naming, scope of follow-up.}
+\\\`\\\`\\\`\\\`
+
+</details>
+\`\`\`
+
+**Heading severity emoji** — every \`### \` heading carries one:
+
+- 🚨 critical — blocks merge (data loss, security, broken core flow)
+- ⚠️ important — must address before merging (regression, missing validation, incorrect behavior)
+- ℹ️ informational — surfaced for awareness; mergeable as-is
+
+**Visible problem write-up rules:**
+
+- **No asks, no suggested fixes** in the visible part. The visible portion describes the problem; the technical-details block describes the fix shape and any open questions. The exception: a fix so self-evident that NOT stating it would be weird (e.g. "the typo is missing an 'r'") — in that case, fold it into the problem statement and skip the suggested-approach block in technical details too.
+- **Never two successive plain paragraphs.** Every transition between block-level elements must alternate prose with structure: paragraph → bullet list → paragraph; paragraph → code fence → bullet list; paragraph → table → paragraph. Two consecutive paragraphs in a row create a wall of text that's impossible to digest. If you catch yourself writing one, find a way to split it: pull a list out of it, drop a 2-3 line code fence between them, or merge them into a single tighter paragraph.
+- **Per-paragraph budget:** ~3 sentences max. Past that, you're explaining where you should be structuring.
+- **Identifier discipline still applies** in the visible part. Lead with behavior in plain English; name an identifier only when it's the subject of the concern or a public surface a reader would recognize. The technical-details block is where dense identifier references belong.
+
+**Technical-details block rules:**
+
+- Wrapped in a 4-backtick markdown fence (\`\\\`\\\`\\\`\\\`markdown ... \\\`\\\`\\\`\\\`\`) so it's visually distinct, one-click copyable, and can contain its own 3-backtick code fences without escape gymnastics. The contents are agent-readable — a fix-agent will pull the body down and use this block as the brief.
+- File paths and \`file:line\` refs are encouraged (and necessary) — the next agent uses these to navigate. Identifier density is fine here.
+- Slightly more verbose than the absolute minimum is OK when it materially helps the next agent: a small code snippet showing the symptom, a short table of mismatched key/column pairs, a one-paragraph "why CI doesn't catch it" note. Skip massive regression-test scaffolding or full route rewrites — the implementing agent writes those.
+- Use the four standard sections (\`Affected sites\`, \`Required outcome\`, optional \`Suggested approach\`, optional \`Open questions for the human\`). Skip the optional sections when they wouldn't add anything.
+
+## 3. \`### ℹ️ Nitpicks\` (optional, last section)
+
+Only when there are nits that for some reason can't be inlined. Filepaths in nit text are fine — these are simple enough that a human or agent reads once and acts. No technical-details block.
+
+\`\`\`
+### ℹ️ Nitpicks
+
+- {nit, with file path inline if useful, ≤ ~200 chars}
+- ...
+\`\`\`
+
+## Body-wide rules
+
+- **Inline-vs-body discipline (repeated for emphasis):** anything that anchors to a single line goes inline, not in the body. The body is for cross-cutting concerns only.
+- **No \`### Issues found\` heading** above the issue sections — each \`### \` heading IS the issue.
+- **Severity emoji on every \`### \` heading** (🚨 / ⚠️ / ℹ️). No emoji on the preamble lead-in or anywhere else.
+- **GitHub block-level rendering**: GitHub's markdown parser requires a blank line between ALL block-level elements (HTML tags like \`<br/>\`, \`<sub>\`, \`<details>\`, \`<b>\` and markdown syntax like headings, lists, blockquotes, code fences, paragraphs). Without a blank line, GitHub treats following content as a continuation of the HTML block and renders markdown syntax as literal text. ALWAYS separate block-level elements with a blank line.
+- **Backtick-wrap** every variable, identifier, or file name when you mention one (in either visible or technical-details portions).
+- **Don't repeat diff content**, don't include raw \`+123 / -45\` stats, don't include a changelog section, don't use horizontal rules (\`---\`).
+- **Pull file/commit counts from \`checkout_pr\` metadata** — never count manually.
+- **Legacy headings REMOVED.** Do not use \`### Key changes\`, \`### Issues found\`, \`<b>TL;DR</b>\`, or \`<sub><b>Summary</b>\`. The new structure subsumes them.`;

 export function computeModes(agentId: AgentId): Mode[] {
  const t = (toolName: string) => formatMcpToolRef(agentId, toolName);
@@ -171,9 +235,12 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`,
    // the Review/IncrementalReview lens fan-out where independence between
    // perspectives is what's being purchased.
    //
-    // Deliberate omission vs canonical /anneal: severity categorization in
-    // the final message (the review body has its own CAUTION/IMPORTANT
-    // framing instead of a severity table).
+    // Severity categorization is split across two surfaces: the opening
+    // callout (CAUTION/IMPORTANT/ℹ️/✅) sets the review's overall tier, and
+    // per-bullet emoji prefixes (🚨/⚠️/ℹ️ in PR_SUMMARY_FORMAT) tag
+    // individual points inside summary sections — scoping severity to the
+    // specific bullet rather than the whole section keeps a section that
+    // mixes a 🚨 and an ℹ️ from being mislabeled by either of them.
    {
      name: "Review",
      description:
@@ -268,12 +335,12 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`,

   The review body is structured as: \`[optional alert blockquote]\` → \`[PR summary using the default format below]\`. Inline comments are passed via the \`comments\` parameter, not in the body.

-   GitHub alert blockquotes render at four visual intensities — the callout is what the author sees first, so pick the one that matches what you want them to do:
+   The opening callout is what the author sees first — pick the one that matches what you want them to do. Five tiers, from loudest to friendliest:

   - \`[!CAUTION]\` — large red banner. Reads as "this will break something."
   - \`[!IMPORTANT]\` — large purple banner. Reads as "you need to look at this before merging."
-   - \`[!NOTE]\` — small blue inline callout. Reads as "FYI, here's something worth noting."
-   - no callout — plain text. Reads as routine review output.
+   - \`> ℹ️ ...\` — informational blockquote. Reads as "minor suggestions, nothing blocking."
+   - \`> ✅ ...\` — green friendly blockquote. Reads as "no concerns, mergeable."

   Two reinforcing levers: callout intensity (above) and \`approved\` (which gates the footer Fix-button affordance — Fix renders on every non-approving review, so \`approved: true\` suppresses it). Wrapping mergeable feedback in \`[!IMPORTANT]\` trains users to click Fix on reviews that don't need fixing. Pick the tier the author's actual next action justifies.

@@ -282,11 +349,11 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`,
   - **must-address non-critical findings** (real consequences if shipped — incorrect behavior in non-critical paths, missing validation on user input, regressions the author should fix before merge):
     \`approved: false\`. Body opens with \`> [!IMPORTANT]\\n> ...\`, followed by the PR summary. Reserve this tier for findings with concrete fallout — do NOT use \`[!IMPORTANT]\` for nits, style preferences, or "consider also" suggestions. Include all inline comments via \`comments\`.
   - **minor suggestions only** (single-line nits, doc/comment polish, defer-able observations, "rough edges"):
-     \`approved: false\`. NO alert blockquote. Body opens directly with the PR summary. Include all inline comments via \`comments\`.
+     \`approved: false\`. Body opens with \`> ℹ️ No critical issues — minor suggestions inline.\\n\\n\` followed by the PR summary. Include all inline comments via \`comments\`. Vary the wording after the emoji to fit the review (e.g. "Minor suggestions only.", "Two rough edges worth a look."), but always keep the ℹ️ prefix and keep it short.
   - **informational observations** (mergeable as-is, nothing actionable — e.g. prior feedback addressed cleanly, surfacing a minor stale doc reference, calling out something noteworthy without recommending a change):
-     \`approved: true\`. Body opens with \`> [!NOTE]\\n> ...\`, followed by the PR summary. Do NOT include inline \`comments\` — \`[!NOTE]\` signals "no action needed", which contradicts an actionable anchor; if a point is concrete enough to anchor to a line, downgrade the whole review to "minor suggestions only" (\`approved: false\`) instead.
+     \`approved: true\`. Body opens with \`> ✅ No new issues found.\\n\\n\` followed by the PR summary. Do NOT include inline \`comments\` — the ✅ signals "no action needed", which contradicts an actionable anchor; if a point is concrete enough to anchor to a line, downgrade the whole review to "minor suggestions only" (\`approved: false\`) instead.
   - **no actionable issues**:
-     \`approved: true\`. Body opens with \`No new issues found.\` followed by the PR summary.
+     \`approved: true\`. Body opens with \`> ✅ No new issues found.\\n\\n\` followed by the PR summary.

 ${PR_SUMMARY_FORMAT}`,
    },
@@ -300,7 +367,7 @@ ${PR_SUMMARY_FORMAT}`,
    // "Reviewed changes" — a separate "Prior review feedback" checklist
    // would duplicate the rolling PR summary snapshot's record of what
    // earlier runs already addressed and add noise to the user-facing
-    // body. Same severity-table omission as Review.
+    // body. Same opening-callout + per-bullet emoji severity split as Review.
    {
      name: "IncrementalReview",
      description:
@@ -365,16 +432,16 @@ ${PR_SUMMARY_FORMAT}`,

 10. Submit — every run must end with EXACTLY ONE of \`${t("create_pull_request_review")}\` (substantive review) or \`${t("report_progress")}\` (no-review acknowledgement). do NOT call \`create_issue_comment\` for review output.

-   Same callout-intensity ladder as Review mode — \`[!CAUTION]\` (large red, "will break") → \`[!IMPORTANT]\` (large purple, "must address before merging") → \`[!NOTE]\` (small blue, "FYI") → no callout (plain text). And the same Fix-button lever: the footer renders a Fix button on every non-approving review, so \`approved: true\` suppresses it. Wrapping mergeable feedback in \`[!IMPORTANT]\` trains users to click Fix on reviews that don't need fixing — pick the tier the author's actual next action justifies.
+   Same callout ladder as Review mode — \`[!CAUTION]\` (red, "will break") → \`[!IMPORTANT]\` (purple, "must address before merging") → \`> ℹ️ ...\` (informational, "minor suggestions only") → \`> ✅ ...\` (green friendly, "no concerns"). Same Fix-button lever: the footer renders a Fix button on every non-approving review, so \`approved: true\` suppresses it. Wrapping mergeable feedback in \`[!IMPORTANT]\` trains users to click Fix on reviews that don't need fixing — pick the tier the author's actual next action justifies.

   Follow these rules:
   - note: the first create_pull_request_review submission may error with a one-time diff-coverage nudge listing unread TOC regions. retry the same call to proceed — optionally after reading the listed ranges. the pre-flight will not block again this session.
   - IF NO NEW ISSUES, NON-SUBSTANTIVE CHANGES ONLY (trivial formatting, import reordering, comment tweaks): do NOT submit a review. Instead call \`${t("report_progress")}\` with a 1-2 sentence note explaining no review was warranted (e.g. "No new issues. Changes since last review are formatting-only."). this leaves a visible signal that the run completed.
   - ELSE IF NEW CRITICAL ISSUES (blocks merge — bugs, security, data loss, broken core flows): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with \`> [!CAUTION]\\n> This PR introduces ...\`, then the Reviewed-changes summary.
   - ELSE IF NEW MUST-ADDRESS NON-CRITICAL FINDINGS (real consequences if shipped — incorrect behavior, missing validation, regressions the author should fix before merge): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with \`> [!IMPORTANT]\\n> ...\`, then the Reviewed-changes summary. Do NOT use this tier for nits, style preferences, or "consider also" suggestions.
-   - ELSE IF NEW MINOR SUGGESTIONS ONLY (single-line nits, doc/comment polish, defer-able observations, "rough edges"): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens directly with \`Reviewed the following changes:\\n\` (NO alert blockquote), then the Reviewed-changes summary.
-   - ELSE IF INFORMATIONAL OBSERVATIONS (mergeable as-is, but worth surfacing — e.g. prior feedback addressed cleanly with one minor stale doc reference, or a noteworthy positive observation): call \`${t("create_pull_request_review")}\` with \`approved: true\`, NO inline comments, and the review body. body opens with \`> [!NOTE]\\n> ...\` alert, then the Reviewed-changes summary. If a point is concrete enough to anchor to a line, downgrade the whole review to "minor suggestions only" (\`approved: false\`) instead — \`[!NOTE]\` and inline comments don't mix.
-   - ELSE IF NO NEW ISSUES, SUBSTANTIVE CHANGES (new functionality, behavior changes, or fixes to prior review feedback): call \`${t("create_pull_request_review")}\` to create a PR review. If all previous reviews have been properly addressed and no new issues were discovered, you can set \`approved: true\`. body opens with \`No new issues. Reviewed the following changes:\\n\`, then the Reviewed-changes summary.`,
+   - ELSE IF NEW MINOR SUGGESTIONS ONLY (single-line nits, doc/comment polish, defer-able observations, "rough edges"): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with \`> ℹ️ No critical issues — minor suggestions inline.\\n\\n\` (vary the wording after ℹ️ to fit the review), then the Reviewed-changes summary.
+   - ELSE IF INFORMATIONAL OBSERVATIONS (mergeable as-is, but worth surfacing — e.g. prior feedback addressed cleanly with one minor stale doc reference, or a noteworthy positive observation): call \`${t("create_pull_request_review")}\` with \`approved: true\`, NO inline comments, and the review body. body opens with \`> ✅ No new issues found.\\n\\n\` (or similar friendly green opener), then the Reviewed-changes summary. If a point is concrete enough to anchor to a line, downgrade the whole review to "minor suggestions only" (\`approved: false\`) instead — the ✅ signals "no action needed", which contradicts an actionable anchor.
+   - ELSE IF NO NEW ISSUES, SUBSTANTIVE CHANGES (new functionality, behavior changes, or fixes to prior review feedback): call \`${t("create_pull_request_review")}\` to create a PR review. If all previous reviews have been properly addressed and no new issues were discovered, set \`approved: true\`. body opens with \`> ✅ No new issues found.\\n\\nReviewed the following changes:\\n\`, then the Reviewed-changes summary.`,
    },
    {
      name: "Plan",
@@ -16,11 +16,8 @@
    "typecheck": "tsc --noEmit",
    "build": "node esbuild.config.js && tsc -p tsconfig.exports.json",
    "check:entrypoints": "node scripts/check-entrypoint-imports.ts",
-    "gha": "node gha.ts",
-    "play": "node gha.ts play.ts",
-    "play:local": "node play.ts",
-    "runtest": "node gha.ts test/run.ts",
-    "runtest:local": "node test/run.ts",
+    "play": "node play.ts",
+    "runtest": "node test/run.ts",
    "scratch": "node scratch.ts",
    "upDeps": "pnpm up --latest",
    "lock": "pnpm install --no-frozen-lockfile",
@@ -109,9 +109,25 @@ export function addSkill(params: {
  );
  if (result.status === 0) {
    log.success(`installed ${params.skill} skill (${params.agent})`);
-  } else {
-    const stderr = (result.stderr?.toString() || "").trim();
-    const errorMsg = result.error ? result.error.message : stderr;
-    log.info(`${params.skill} skill install failed: ${errorMsg}`);
+    return;
  }
+  // skills CLI uses a Clack-style TUI that prints errors to stdout (alongside
+  // spinner output), not stderr. report exit code + both streams + spawn
+  // error so failures aren't silent. tail-truncate streams to keep CI logs
+  // bounded.
+  const stdout = (result.stdout?.toString() || "").trim();
+  const stderr = (result.stderr?.toString() || "").trim();
+  const parts = [
+    `exit=${result.status ?? "null"} signal=${result.signal ?? "null"}`,
+    result.error ? `spawn error: ${result.error.message}` : null,
+    stderr ? `stderr:\n${tailLines(stderr, 20)}` : null,
+    stdout ? `stdout:\n${tailLines(stdout, 20)}` : null,
+  ].filter(Boolean);
+  log.warning(`${params.skill} skill install failed — ${parts.join(" | ")}`);
+}
+
+function tailLines(text: string, n: number): string {
+  const lines = text.split("\n");
+  if (lines.length <= n) return text;
+  return `...(truncated, last ${n} of ${lines.length} lines)\n${lines.slice(-n).join("\n")}`;
 }
@@ -24,18 +24,29 @@ export async function resolveRun(params: ResolveRunParams): Promise<ResolveRunRe
  }
  const [owner, repo] = githubRepo.split("/");

+  // jobId is purely cosmetic — only used to deep-link "View workflow run"
+  // footers to the specific job. Tolerate any failure (e.g. test harness
+  // running with the host's GITHUB_RUN_ID against a different repo's MCP
+  // token, which 404s; this happens since #750 dropped the env allowlist
+  // and now passes GITHUB_RUN_ID + GITHUB_JOB through to the test container)
+  // and fall back to the run-level link.
  let jobId: string | undefined;
  const jobName = process.env.GITHUB_JOB;
  if (jobName && runId) {
-    const jobs = await params.octokit.rest.actions.listJobsForWorkflowRun({
-      owner,
-      repo,
-      run_id: runId,
-    });
-    const matchingJob = jobs.data.jobs.find((job) => job.name === jobName);
-    if (matchingJob) {
-      jobId = String(matchingJob.id);
-      log.debug(`» found job ID: ${jobId}`);
+    try {
+      const jobs = await params.octokit.rest.actions.listJobsForWorkflowRun({
+        owner,
+        repo,
+        run_id: runId,
+      });
+      const matchingJob = jobs.data.jobs.find((job) => job.name === jobName);
+      if (matchingJob) {
+        jobId = String(matchingJob.id);
+        log.debug(`» found job ID: ${jobId}`);
+      }
+    } catch (err) {
+      const msg = err instanceof Error ? err.message : String(err);
+      log.debug(`» listJobsForWorkflowRun failed (jobId stays undefined): ${msg}`);
    }
  }