diff --git a/agents/claude.ts b/agents/claude.ts index f7be1b2..c2e2095 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -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; diff --git a/agents/opencode.test.ts b/agents/opencode.test.ts new file mode 100644 index 0000000..371bf2d --- /dev/null +++ b/agents/opencode.test.ts @@ -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" } }, + }); + } + }); +}); diff --git a/agents/opencode.ts b/agents/opencode.ts index 0be9fbd..5dc3a9d 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -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 { + 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, diff --git a/agents/postRun.ts b/agents/postRun.ts index 344d3b1..d78d14a 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -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 = 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. diff --git a/agents/subagentModels.test.ts b/agents/subagentModels.test.ts index faf361d..f136b64 100644 --- a/agents/subagentModels.test.ts +++ b/agents/subagentModels.test.ts @@ -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", () => { diff --git a/gha.ts b/gha.ts index 63e97bf..f7f5f09 100644 --- a/gha.ts +++ b/gha.ts @@ -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 = { diff --git a/mcp/review.ts b/mcp/review.ts index f0403f1..e8931e8 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -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(); diff --git a/models.ts b/models.ts index b819fba..ec42f77 100644 --- a/models.ts +++ b/models.ts @@ -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", diff --git a/modes.ts b/modes.ts index 7ef7eb1..a18e140 100644 --- a/modes.ts +++ b/modes.ts @@ -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: -TL;DR — 1-3 sentences on what the PR does and why. Focus on intent, not mechanics. -NOTE: use HTML bold TL;DR, 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 \`
Review metadata
\` block. +2. **Cross-cutting issue sections** (zero or more) — one \`### \` heading per concern, with a human-readable problem write-up and a collapsed \`
Technical 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 -Summary | {file_count} files | {commit_count} commits | base: \`{base}\` ← \`{head}\` -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 \`TL;DR\`): -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. -
+- **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 +
Review metadata -> **Before:** [old behavior/state]
**After:** [new behavior/state] -IMPORTANT: Before and After MUST be on a SINGLE blockquote line with an inline
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: ->
How does X work? -> Extended explanation here. ->
+
+\`\`\` -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 (
, ,
, , 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-\` 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
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.} + +
Technical details + +\\\`\\\`\\\`\\\`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.} +\\\`\\\`\\\`\\\` + +
+\`\`\` + +**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 \`
\`, \`\`, \`
\`, \`\` 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\`, \`TL;DR\`, or \`Summary\`. 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", diff --git a/package.json b/package.json index 0d2430a..798d35d 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/utils/skills.ts b/utils/skills.ts index fc0b9b9..77f1c5e 100644 --- a/utils/skills.ts +++ b/utils/skills.ts @@ -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")}`; } diff --git a/utils/workflow.ts b/utils/workflow.ts index ae62eff..61154cd 100644 --- a/utils/workflow.ts +++ b/utils/workflow.ts @@ -24,18 +24,29 @@ export async function resolveRun(params: ResolveRunParams): Promise 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}`); } }