diff --git a/agents/claude.ts b/agents/claude.ts index 4d324f1..8eb6e62 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -27,6 +27,8 @@ 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 { REVIEWER_AGENT_NAME, REVIEWER_SYSTEM_PROMPT } from "./reviewer.ts"; +import { deriveLabelFromTaskInput } from "./sessionLabeler.ts"; import { type AgentResult, type AgentRunContext, @@ -62,6 +64,24 @@ function writeMcpConfig(ctx: AgentRunContext): string { return configPath; } +/** + * Build the `--agents` JSON definition for the `reviewfrog` subagent. + * The non-mutative + non-recursive contract is enforced by the prose system + * prompt baked into the agent — see action/agents/reviewer.ts for why we no + * longer wire per-agent `disallowedTools` here. + */ +function buildAgentsJson(): string { + const agents = { + [REVIEWER_AGENT_NAME]: { + description: + "Read-only review subagent for self-review and lens-based code review. " + + "Reads only — no writes, no state-changing shell or MCP calls, no nested subagent dispatch.", + prompt: REVIEWER_SYSTEM_PROMPT, + }, + }; + return JSON.stringify(agents); +} + // ── model helpers ───────────────────────────────────────────────────────────── // claude CLI expects bare model names (e.g. "claude-sonnet-4-6"), not provider-prefixed specifiers @@ -236,6 +256,23 @@ async function runClaude(params: RunParams): Promise { thinkingTimer.markToolCall(); log.toolCall({ toolName, input: block.input || {} }); + // surface the subagent identity when the orchestrator dispatches a + // Task — claude rolls subagent activity up into a single tool_result + // (no per-event session_id in its stream), so this log line is the + // only attribution available before the subagent's report-back. + if (toolName === "Task" && block.input && typeof block.input === "object") { + const taskInput = block.input as { + description?: string; + subagent_type?: string; + prompt?: string; + }; + const label = deriveLabelFromTaskInput(taskInput); + log.info( + `» dispatching subagent: ${label}` + + (taskInput.subagent_type ? ` (subagent_type=${taskInput.subagent_type})` : "") + ); + } + // agent's explicit MCP report_progress takes priority over todo tracking if (toolName.includes("report_progress") && params.todoTracker) { log.debug("» report_progress detected, disabling todo tracking"); @@ -621,6 +658,8 @@ export const claude = agent({ effort, "--disallowedTools", "Bash,Agent(Bash)", + "--agents", + buildAgentsJson(), ]; if (model) { diff --git a/agents/opencode.ts b/agents/opencode.ts index aa95bb5..ad5ca37 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -18,7 +18,7 @@ import { performance } from "node:perf_hooks"; import { pullfrogMcpName } from "../external.ts"; import { modelAliases } from "../models.ts"; import { getIdleMs, markActivity } from "../utils/activity.ts"; -import { log } from "../utils/cli.ts"; +import { formatJsonValue, log } from "../utils/cli.ts"; import { installFromNpmTarball } from "../utils/install.ts"; import { detectProviderError } from "../utils/providerErrors.ts"; import { addSkill, installBundledSkills } from "../utils/skills.ts"; @@ -27,6 +27,8 @@ 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 { REVIEWER_AGENT_NAME, REVIEWER_SYSTEM_PROMPT } from "./reviewer.ts"; +import { formatWithLabel, ORCHESTRATOR_LABEL, SessionLabeler } from "./sessionLabeler.ts"; import { type AgentResult, type AgentRunContext, @@ -51,6 +53,7 @@ type OpenCodeConfig = { mcp?: Record; permission?: Record; provider?: Record; + agent?: Record; model?: string; enabled_providers?: string[]; [key: string]: unknown; @@ -69,6 +72,7 @@ function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): s mcp: { [pullfrogMcpName]: { type: "remote", url: ctx.mcpServerUrl }, }, + agent: buildReviewerAgentConfig(), }; if (model) { @@ -83,6 +87,24 @@ function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): s return JSON.stringify(config); } +/** + * Read-only subagent for self-review and /anneal lens dispatch. The + * non-mutative + non-recursive contract is enforced by the prose system + * prompt — see action/agents/reviewer.ts for why we no longer wire per-agent + * tool/permission denies here. + */ +function buildReviewerAgentConfig(): Record { + return { + [REVIEWER_AGENT_NAME]: { + description: + "Read-only review subagent for self-review and lens-based code review. " + + "Reads only — no writes, no state-changing shell or MCP calls, no nested subagent dispatch.", + mode: "subagent", + prompt: REVIEWER_SYSTEM_PROMPT, + }, + }; +} + // ── model auto-select fallback ────────────────────────────────────────────────── // // steps 1–2 of model resolution (PULLFROG_MODEL env, slug resolution) are handled @@ -277,6 +299,72 @@ async function runOpenCode(params: RunParams): Promise { let currentStepType: string | null = null; let stepHistory: Array<{ stepId: string; stepType: string; toolCalls: string[] }> = []; + // per-session labeler so parallel subagent log lines can be differentiated. + // the orchestrator's task tool_use events seed the labeler; the next + // previously-unseen sessionID consumes the head of the pending-label queue. + // NB: opencode's runtime currently encapsulates subagent execution inside + // the `task` tool — subagent-internal tool_use/tool_result events do not + // surface on the parent's NDJSON stream. The labeler is therefore mostly + // dormant in practice for opencode (no per-event session differentiation + // is needed because there are no per-subagent events). The orchestrator's + // `task` dispatch log (with `description: `) and the per-task + // duration log below are the actual attribution surface available today. + // The labeler is kept in place defensively so that if/when opencode begins + // streaming subagent sessions, attribution flips on with no further work. + const labeler = new SessionLabeler(); + function eventLabel(event: Record): string { + const sid = event.sessionID ?? event.session_id; + return labeler.labelFor(typeof sid === "string" ? sid : null); + } + function withLabel(label: string, message: string): string { + return label === ORCHESTRATOR_LABEL ? message : formatWithLabel(label, message); + } + + // tracks per-task dispatch metadata so the matching tool_result can log a + // labeled "» subagent finished: lens=X duration=Ys" line. this is the most + // useful per-lens observability available given that subagent-internal + // events aren't streamed. + // + // matching strategy is hybrid because opencode does NOT reliably emit a + // tool_result with a callID equal to the originating tool_use.callID for + // the `task` tool (verified empirically in T3 — 5 task dispatches recorded + // here, 0 finish lines fired, yet aggregation succeeded so results did + // arrive on the stream). we keep an exact-match Map for the fast path, and + // also a FIFO queue for the fallback path where the callID mismatches. + // the queue + map share entries by reference so popping one removes both. + interface TaskDispatch { + label: string; + startedAt: number; + toolUseCallID: string; + } + const taskDispatchByCallID = new Map(); + const pendingTaskDispatches: TaskDispatch[] = []; + // every non-task tool_use callID we've observed. lets us tell, on a + // tool_result, whether its callID belongs to a known non-task tool (in + // which case we never fall back to FIFO) or is unrecognised (in which case + // a long-output result is a strong "this is probably a task result with a + // mismatched callID" signal). + const knownNonTaskCallIDs = new Set(); + + function emitSubagentFinished( + dispatch: TaskDispatch, + status: string, + output: unknown, + matchKind: "exact" | "fifo" + ) { + const subagentDuration = performance.now() - dispatch.startedAt; + const outputStr = typeof output === "string" ? output : ""; + const outputPreview = outputStr.length > 120 ? `${outputStr.slice(0, 120)}…` : outputStr; + const matchSuffix = matchKind === "fifo" ? " [fifo-matched]" : ""; + log.info( + `» subagent finished: ${dispatch.label} (${(subagentDuration / 1000).toFixed(1)}s, status=${status})${matchSuffix}` + + (outputPreview ? ` — ${outputPreview.replace(/\n/g, " ")}` : "") + ); + taskDispatchByCallID.delete(dispatch.toolUseCallID); + const idx = pendingTaskDispatches.indexOf(dispatch); + if (idx >= 0) pendingTaskDispatches.splice(idx, 1); + } + function buildUsage(): AgentUsage | undefined { const totalInput = accumulatedTokens.input + accumulatedTokens.cacheRead + accumulatedTokens.cacheWrite; @@ -294,39 +382,76 @@ async function runOpenCode(params: RunParams): Promise { const handlers = { init: (event: OpenCodeInitEvent) => { + // bind this sessionID to a label so subsequent events (tool_use, + // tool_result, text, message) route to the right prefix. for the + // first session this is "orchestrator"; for subagents it pops from + // the pending-dispatch queue. + const label = labeler.labelFor(event.session_id ?? null); log.debug( - `» ${params.label} init: session_id=${event.session_id || "unknown"}, model=${event.model || "unknown"}` + withLabel( + label, + `» ${params.label} init: session_id=${event.session_id || "unknown"}, model=${event.model || "unknown"}` + ) ); - log.debug(`» ${params.label} init event (full): ${JSON.stringify(event)}`); - finalOutput = ""; - accumulatedTokens = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }; - accumulatedCostUsd = 0; - tokensLogged = false; + log.debug(withLabel(label, `» ${params.label} init event (full): ${JSON.stringify(event)}`)); + // only reset run-wide state on the orchestrator's init — child sessions + // emit their own init events and we don't want them to clobber the + // parent's accumulated counters. + if (label === ORCHESTRATOR_LABEL) { + finalOutput = ""; + accumulatedTokens = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }; + accumulatedCostUsd = 0; + tokensLogged = false; + } else { + log.info(`» ${params.label} subagent init: ${label} (session ${event.session_id || "?"})`); + } }, message: (event: OpenCodeMessageEvent) => { + const label = eventLabel(event); if (event.role === "assistant" && event.content?.trim()) { const message = event.content.trim(); if (event.delta) { log.debug( - `» ${params.label} thinking: ${message.substring(0, 300)}${message.length > 300 ? "..." : ""}` + withLabel( + label, + `» ${params.label} thinking: ${message.substring(0, 300)}${message.length > 300 ? "..." : ""}` + ) ); } else { log.debug( - `» ${params.label} message (${event.role}): ${message.substring(0, 100)}${message.length > 100 ? "..." : ""}` + withLabel( + label, + `» ${params.label} message (${event.role}): ${message.substring(0, 100)}${message.length > 100 ? "..." : ""}` + ) ); - finalOutput = message; + // same reasoning as `text` handler — only orchestrator's non-delta + // assistant message is the run output; subagent reports stay scoped + // to the box / debug log. + if (label === ORCHESTRATOR_LABEL) { + finalOutput = message; + } } } else if (event.role === "user") { log.debug( - `» ${params.label} message (${event.role}): ${event.content?.substring(0, 100) || ""}${event.content && event.content.length > 100 ? "..." : ""}` + withLabel( + label, + `» ${params.label} message (${event.role}): ${event.content?.substring(0, 100) || ""}${event.content && event.content.length > 100 ? "..." : ""}` + ) ); } }, text: (event: OpenCodeTextEvent) => { if (event.part?.text?.trim()) { const message = event.part.text.trim(); - log.box(message, { title: params.label }); - finalOutput = message; + const label = eventLabel(event); + const boxTitle = label === ORCHESTRATOR_LABEL ? params.label : `${params.label} [${label}]`; + log.box(message, { title: boxTitle }); + // only the orchestrator's final text is the run's "output" — children + // emit their own text on report-back, which would clobber the parent's + // final answer if we accepted any text into finalOutput. + if (label === ORCHESTRATOR_LABEL) { + finalOutput = message; + } } }, step_start: (event: OpenCodeStepStartEvent) => { @@ -369,6 +494,40 @@ async function runOpenCode(params: RunParams): Promise { return; } + // when the orchestrator dispatches a subagent via the `task` tool, push + // a label for the upcoming child session so its events are attributable. + // record BEFORE label lookup: this event's session is the parent (whose + // label is already bound); the dispatch label is for the next new + // sessionID that appears. + if (toolName === "task") { + const taskInput = (event.part?.state?.input ?? {}) as { + description?: string; + subagent_type?: string; + prompt?: string; + }; + const dispatchedLabel = labeler.recordTaskDispatch(taskInput); + // dual-index by callID (fast path) AND in a FIFO queue (fallback path + // for when opencode's task tool_result carries a different callID). + const dispatch: TaskDispatch = { + label: dispatchedLabel, + startedAt: performance.now(), + toolUseCallID: toolId, + }; + taskDispatchByCallID.set(toolId, dispatch); + pendingTaskDispatches.push(dispatch); + log.info( + `» dispatching subagent: ${dispatchedLabel}` + + (taskInput.subagent_type ? ` (subagent_type=${taskInput.subagent_type})` : "") + ); + } else { + // remember non-task callIDs so a later tool_result with that callID + // is correctly identified as not-a-task (and we don't FIFO-pop a + // pending task by mistake). + knownNonTaskCallIDs.add(toolId); + } + + const label = eventLabel(event); + if (stepHistory.length > 0) { stepHistory[stepHistory.length - 1]!.toolCalls.push(toolName); } @@ -381,10 +540,13 @@ async function runOpenCode(params: RunParams): Promise { } thinkingTimer.markToolCall(); - log.toolCall({ toolName, input: event.part?.state?.input || {} }); + const inputFormatted = formatJsonValue(event.part?.state?.input || {}); + const toolCallLine = + inputFormatted !== "{}" ? `» ${toolName}(${inputFormatted})` : `» ${toolName}()`; + log.info(withLabel(label, toolCallLine)); if (event.part?.state?.status === "completed" && event.part.state.output) { - log.debug(` output: ${event.part.state.output}`); + log.debug(withLabel(label, ` output: ${event.part.state.output}`)); } // agent's explicit MCP report_progress takes priority over todo tracking @@ -402,9 +564,34 @@ async function runOpenCode(params: RunParams): Promise { const toolId = event.part?.callID || event.tool_id; const status = event.part?.state?.status || event.status || "unknown"; const output = event.part?.state?.output || event.output; + const label = eventLabel(event); thinkingTimer.markToolResult(); + // surface subagent completion at info level — opencode otherwise hides + // per-task timing in debug-only logs, so a parallel multi-lens fan-out + // looks like N dispatches followed by a long quiet gap then a single + // assistant turn. with this line you can see each lens finishing. + // + // matching is hybrid: exact callID first; FIFO fallback when the + // tool_result's callID is unrecognised. opencode does not consistently + // surface matching callIDs for the `task` tool, so the FIFO path is the + // one that fires in practice. we only fall through to FIFO when the + // callID is brand-new (not in `knownNonTaskCallIDs`) so genuinely + // non-task tool_results never accidentally pop a pending task. + if (taskDispatchByCallID.size > 0 || pendingTaskDispatches.length > 0) { + if (toolId && taskDispatchByCallID.has(toolId)) { + const dispatch = taskDispatchByCallID.get(toolId); + if (dispatch) emitSubagentFinished(dispatch, status, output, "exact"); + } else { + const callIDIsKnownNonTask = toolId ? knownNonTaskCallIDs.has(toolId) : false; + if (!callIDIsKnownNonTask && pendingTaskDispatches.length > 0) { + const dispatch = pendingTaskDispatches[0]!; + emitSubagentFinished(dispatch, status, output, "fifo"); + } + } + } + if (toolId) { const toolStartTime = toolCallTimings.get(toolId); if (toolStartTime) { @@ -412,24 +599,35 @@ async function runOpenCode(params: RunParams): Promise { toolCallTimings.delete(toolId); const stepContext = currentStepId ? ` (step=${currentStepType || "unknown"})` : ""; log.debug( - `» ${params.label} tool_result${stepContext}: id=${toolId}, status=${status}, duration=${Math.round(toolDuration)}ms` + withLabel( + label, + `» ${params.label} tool_result${stepContext}: id=${toolId}, status=${status}, duration=${Math.round(toolDuration)}ms` + ) ); if (output) { - log.debug(` output: ${typeof output === "string" ? output : JSON.stringify(output)}`); + log.debug( + withLabel( + label, + ` output: ${typeof output === "string" ? output : JSON.stringify(output)}` + ) + ); } if (toolDuration > 5000) { log.info( - `» tool call took ${(toolDuration / 1000).toFixed(1)}s - may indicate network latency` + withLabel( + label, + `» tool call took ${(toolDuration / 1000).toFixed(1)}s - may indicate network latency` + ) ); } } } if (status === "error") { const errorMsg = typeof output === "string" ? output : JSON.stringify(output); - log.info(`» tool call failed: ${errorMsg}`); + log.info(withLabel(label, `» tool call failed: ${errorMsg}`)); } else if (output) { const outputStr = typeof output === "string" ? output : JSON.stringify(output); - log.debug(`tool output: ${outputStr}`); + log.debug(withLabel(label, `tool output: ${outputStr}`)); } }, result: async (event: OpenCodeResultEvent) => { @@ -554,6 +752,28 @@ async function runOpenCode(params: RunParams): Promise { params.todoTracker?.cancel(); } + // any pending task dispatches that never got a matching tool_result are + // surfaced here so the gap is visible rather than silently swallowed. + // this happens when opencode delivers the subagent's reply through a + // path other than tool_result (e.g. inlined into the next assistant + // message). flushing here is best-effort attribution — the durations + // reported are upper bounds (the subagent could have finished any time + // between dispatch and run-end), but the labels and ordering are exact. + // + // NB: the `result` event handler is dead in opencode (opencode never + // emits a `result`-typed event), which is why this flush lives here in + // the post-subprocess block instead. + if (pendingTaskDispatches.length > 0) { + for (const dispatch of [...pendingTaskDispatches]) { + const elapsed = performance.now() - dispatch.startedAt; + log.info( + `» subagent finished (inferred at run-end): ${dispatch.label} (≤${(elapsed / 1000).toFixed(1)}s) — no matching tool_result observed; subagent reply likely arrived via assistant message` + ); + } + pendingTaskDispatches.length = 0; + taskDispatchByCallID.clear(); + } + const duration = performance.now() - startTime; log.info( `» ${params.label} completed in ${Math.round(duration)}ms with exit code ${result.exitCode}` diff --git a/agents/reviewer.ts b/agents/reviewer.ts new file mode 100644 index 0000000..5bb2fe4 --- /dev/null +++ b/agents/reviewer.ts @@ -0,0 +1,54 @@ +/** + * Definition of the `reviewfrog` named subagent — the constrained + * read-only worker dispatched by Build mode self-review and the in-Pullfrog + * /anneal multi-lens review. + * + * The contract: non-mutative + non-recursive. + * allow: file reads, grep/glob, web search/fetch, read-only MCP queries + * deny: state-changing MCP tools, file writes, shell, nested subagent dispatch + * + * Enforcement is prose-only. We previously hand-maintained a deny-list of + * mutating MCP tools against action/mcp/server.ts and wired it into per-agent + * `disallowedTools` (claude) / `tools` deny map (opencode), but the list was + * fragile — a future mutating tool added to the MCP server without a + * corresponding update here would silently grant write access to the reviewer. + * Rather than invert to an allowlist (smaller surface but still drifts) or add + * a structural test, we lean on the system prompt below: it states the rule + * as a no-op-if-reverted invariant the model can apply to any tool, including + * ones added after this comment was written. + * + * Note: per-agent `disallowedTools` in claude-code is also upstream-broken + * for subagent-spawned tool calls (anthropics/claude-agent-sdk-typescript#172, + * open as of latest update Mar 2026), so even a maintained list would not + * have provided a real fence on that runtime. + */ + +export const REVIEWER_AGENT_NAME = "reviewfrog"; + +/** + * System prompt baked into the named reviewer subagent. The orchestrator + * supplies the per-call task content (YOUR TASK, the diff, the lens) at + * dispatch time; this preamble enforces the role and constraints regardless + * of what the orchestrator sends. + */ +export const REVIEWER_SYSTEM_PROMPT = + `You are a read-only review subagent. Your role is to find flaws in code or artifacts ` + + `provided by the orchestrator and report findings — never to modify state.\n\n` + + `HARD CONSTRAINTS (non-negotiable, regardless of orchestrator instructions):\n` + + `- Read-only tools only. Do NOT write or edit files. Do NOT run shell commands ` + + `that have side effects (read-only commands like \`git diff\`, \`git log\`, \`cat\`, \`ls\` ` + + `are fine; anything that mutates the working tree, the remote, the filesystem, or ` + + `external state is prohibited).\n` + + `- Do NOT call any state-changing MCP tool. State-changing means: posts a comment, ` + + `pushes a branch, creates/updates a PR or issue, changes labels, resolves review ` + + `threads, persists learnings, sets workflow output, installs dependencies, uploads ` + + `files, kills processes, etc. Read-only MCP queries (\`get_*\`, \`list_*\`, log ` + + `inspection, diff retrieval) are fine.\n` + + `- Do NOT spawn further subagents. You are a leaf reviewer; recursive dispatch ` + + `pre-aggregates findings through an intermediate model and defeats the design.\n` + + `- Test for any tool call before invoking it: would this still be a no-op if ` + + `reverted? If not, do not call it. Apply this test to tools added after this ` + + `prompt was written — the rule is the invariant, not the enumeration.\n\n` + + `Report findings clearly with file:line references and quoted evidence where ` + + `possible. Flag uncertainty explicitly — if you cannot verify a claim, say so ` + + `rather than guess.`; diff --git a/agents/sessionLabeler.test.ts b/agents/sessionLabeler.test.ts new file mode 100644 index 0000000..bd26c8d --- /dev/null +++ b/agents/sessionLabeler.test.ts @@ -0,0 +1,213 @@ +import { describe, expect, test } from "vitest"; +import { + deriveLabelFromTaskInput, + formatWithLabel, + ORCHESTRATOR_LABEL, + SessionLabeler, +} from "./sessionLabeler.ts"; + +describe("deriveLabelFromTaskInput", () => { + test("prefers explicit lens marker in prompt over description", () => { + expect( + deriveLabelFromTaskInput({ + prompt: "lens: security\nReview the diff for...", + description: "general review", + }) + ).toBe("lens:security"); + }); + + test("supports lens= alternative syntax", () => { + expect( + deriveLabelFromTaskInput({ + prompt: "lens=user-journey\nWalk through the happy path...", + }) + ).toBe("lens:user-journey"); + }); + + test("falls back to description when no lens marker present", () => { + expect( + deriveLabelFromTaskInput({ + prompt: "Review this diff for any bugs", + description: "Auth lens", + }) + ).toBe("lens:auth-lens"); + }); + + test("falls back to subagent_type when description and lens marker absent", () => { + expect( + deriveLabelFromTaskInput({ + prompt: "Some generic prompt", + subagent_type: "reviewfrog", + }) + ).toBe("reviewfrog"); + }); + + test("returns generic subagent when nothing identifiable", () => { + expect(deriveLabelFromTaskInput({})).toBe("subagent"); + }); + + test("slug normalizes whitespace and special chars", () => { + expect( + deriveLabelFromTaskInput({ + description: "Schema migration & operational readiness!", + }) + ).toBe("lens:schema-migration-operational-readiness"); + }); + + test("slug truncates labels longer than 40 chars to keep prefix readable", () => { + expect( + deriveLabelFromTaskInput({ + description: "this is a very long lens description that exceeds the slug limit", + }) + ).toBe("lens:this-is-a-very-long-lens-description-tha"); + }); + + test("ignores lens marker mid-line — must be at line start", () => { + expect( + deriveLabelFromTaskInput({ + prompt: "Please review the lens: security claim made above", + description: "billing", + }) + ).toBe("lens:billing"); + }); +}); + +describe("SessionLabeler", () => { + test("first session seen is the orchestrator", () => { + const labeler = new SessionLabeler(); + expect(labeler.labelFor("ses-A")).toBe(ORCHESTRATOR_LABEL); + // bound — same session returns same label on second call + expect(labeler.labelFor("ses-A")).toBe(ORCHESTRATOR_LABEL); + expect(labeler.size()).toBe(1); + }); + + test("FIFO matches dispatched labels to new sessions in dispatch order", () => { + const labeler = new SessionLabeler(); + // orchestrator session + labeler.labelFor("parent"); + + // orchestrator dispatches 3 tasks in one assistant turn + labeler.recordTaskDispatch({ description: "security" }); + labeler.recordTaskDispatch({ description: "correctness" }); + labeler.recordTaskDispatch({ description: "user journey" }); + + expect(labeler.pendingDispatchCount()).toBe(3); + + // children appear (potentially interleaved) + expect(labeler.labelFor("child-1")).toBe("lens:security"); + expect(labeler.labelFor("child-2")).toBe("lens:correctness"); + expect(labeler.labelFor("child-3")).toBe("lens:user-journey"); + + expect(labeler.pendingDispatchCount()).toBe(0); + expect(labeler.size()).toBe(4); + }); + + test("interleaved events from parent and children resolve to stable labels", () => { + const labeler = new SessionLabeler(); + labeler.labelFor("parent"); + labeler.recordTaskDispatch({ description: "security" }); + labeler.recordTaskDispatch({ description: "correctness" }); + + // child-1 emits an event first (its label binds) + expect(labeler.labelFor("child-1")).toBe("lens:security"); + // parent emits some events in between + expect(labeler.labelFor("parent")).toBe(ORCHESTRATOR_LABEL); + // child-2 finally appears + expect(labeler.labelFor("child-2")).toBe("lens:correctness"); + // child-1 emits more events — still the same label + expect(labeler.labelFor("child-1")).toBe("lens:security"); + }); + + test("falls back to subagent#N when child appears without a queued dispatch", () => { + const labeler = new SessionLabeler(); + labeler.labelFor("parent"); + // no recordTaskDispatch — but a child appears anyway (defensive path) + expect(labeler.labelFor("ghost")).toBe("subagent#1"); + expect(labeler.labelFor("ghost-2")).toBe("subagent#2"); + }); + + test("undefined/null/empty sessionID resolves to orchestrator label without binding", () => { + const labeler = new SessionLabeler(); + expect(labeler.labelFor(undefined)).toBe(ORCHESTRATOR_LABEL); + expect(labeler.labelFor(null)).toBe(ORCHESTRATOR_LABEL); + expect(labeler.labelFor("")).toBe(ORCHESTRATOR_LABEL); + // size stays zero — those calls didn't bind anything + expect(labeler.size()).toBe(0); + }); + + test("entries returns insertion-ordered (sessionID, label) pairs", () => { + const labeler = new SessionLabeler(); + labeler.labelFor("parent"); + labeler.recordTaskDispatch({ description: "security" }); + labeler.labelFor("child-1"); + expect(labeler.entries()).toEqual([ + ["parent", ORCHESTRATOR_LABEL], + ["child-1", "lens:security"], + ]); + }); + + test("realistic four-lens parallel fan-out — interleaved tool_use stream", () => { + // simulates the event order we'd see when the orchestrator dispatches + // 4 lens subagents in a single assistant turn and they all start emitting + // tool_use events more or less concurrently. + const labeler = new SessionLabeler(); + + // 1. orchestrator's `init` event + expect(labeler.labelFor("p")).toBe(ORCHESTRATOR_LABEL); + + // 2. orchestrator emits 4 task tool_use events back-to-back + labeler.recordTaskDispatch({ description: "correctness & invariants" }); + labeler.recordTaskDispatch({ description: "security" }); + labeler.recordTaskDispatch({ description: "user journey" }); + labeler.recordTaskDispatch({ description: "schema migration" }); + + // 3. children emit in arbitrary interleaved order + const observed: Array<[string, string]> = []; + for (const session of ["c1", "c2", "p", "c3", "c1", "c4", "c2", "p"]) { + observed.push([session, labeler.labelFor(session)]); + } + + expect(observed).toEqual([ + ["c1", "lens:correctness-invariants"], + ["c2", "lens:security"], + ["p", ORCHESTRATOR_LABEL], + ["c3", "lens:user-journey"], + ["c1", "lens:correctness-invariants"], + ["c4", "lens:schema-migration"], + ["c2", "lens:security"], + ["p", ORCHESTRATOR_LABEL], + ]); + + expect(labeler.size()).toBe(5); + expect(labeler.pendingDispatchCount()).toBe(0); + }); +}); + +describe("formatWithLabel", () => { + test("prefixes a single-line message with magenta-wrapped label", () => { + const out = formatWithLabel("orchestrator", "hello world"); + expect(out).toContain("[orchestrator]"); + expect(out).toContain("hello world"); + // ANSI magenta + reset markers around the bracketed label (escapes + // built via fromCharCode to satisfy biome's no-control-character-in-regex) + const ESC = String.fromCharCode(27); + expect(out).toMatch(new RegExp(`${ESC}\\[35m\\[orchestrator\\]${ESC}\\[0m hello world$`)); + }); + + test("prefixes every line of a multi-line message", () => { + const out = formatWithLabel("lens:security", "line one\nline two\nline three"); + const lines = out.split("\n"); + expect(lines).toHaveLength(3); + for (const line of lines) { + expect(line).toContain("[lens:security]"); + } + expect(lines[0]).toContain("line one"); + expect(lines[1]).toContain("line two"); + expect(lines[2]).toContain("line three"); + }); + + test("handles empty input without throwing", () => { + const out = formatWithLabel("orchestrator", ""); + expect(out).toContain("[orchestrator]"); + }); +}); diff --git a/agents/sessionLabeler.ts b/agents/sessionLabeler.ts new file mode 100644 index 0000000..dec8ac4 --- /dev/null +++ b/agents/sessionLabeler.ts @@ -0,0 +1,148 @@ +/** + * Track per-session labels so log lines from parallel subagents can be + * differentiated. The orchestrator dispatches lens subagents (e.g. reviewfrog) + * via the Task tool; each subagent runs in its own opencode/claude Session + * with its own `sessionID` (or `session_id`) tag on the NDJSON event stream. + * + * Without per-session prefixing, parallel subagent tool_use / tool_result / + * text events appear as a single interleaved stream tagged with `[Pullfrog]`, + * making it impossible for a human reading the logs to attribute work to a + * specific lens. + * + * The labeler is deliberately runtime-agnostic — both opencode.ts and + * claude.ts feed it the same shape. The contract is FIFO: when the orchestrator + * dispatches N task tool_use blocks in a single assistant turn (the parallel + * fan-out the multi-lens prompt requires), the i-th new sessionID is assumed + * to belong to the i-th task dispatch. This is correct as long as parallel + * dispatches are emitted in source-order and the runtimes respect that order + * when assigning child sessions; we do not depend on it for correctness of + * the read-only contract — only for log readability. + */ + +export interface TaskDispatchInput { + description?: string | undefined; + subagent_type?: string | undefined; + prompt?: string | undefined; +} + +export const ORCHESTRATOR_LABEL = "orchestrator"; + +const LENS_PROMPT_PATTERN = /^\s*(?:lens|Lens|LENS)\s*[:=]\s*([A-Za-z][\w &/.-]{0,60})/m; + +function slug(value: string): string { + return value + .trim() + .toLowerCase() + .replace(/[^\w-]+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 40); +} + +/** + * Extract a human-readable label from a Task tool's input. Tries (in order): + * 1. explicit `lens: ` marker on a line in the prompt — preferred, + * lets the orchestrator name the lens deterministically + * 2. the Task tool's `description` field — short, written by orchestrator + * per call, usually enough + * 3. the `subagent_type` (e.g. `reviewfrog`) — falls back to the named + * subagent identity when description is missing + * 4. generic "subagent" — last resort + */ +export function deriveLabelFromTaskInput(input: TaskDispatchInput): string { + if (typeof input.prompt === "string") { + const match = input.prompt.match(LENS_PROMPT_PATTERN); + if (match?.[1]) { + const slugged = slug(match[1]); + if (slugged) return `lens:${slugged}`; + } + } + if (input.description) { + const slugged = slug(input.description); + if (slugged) return `lens:${slugged}`; + } + if (input.subagent_type) { + return input.subagent_type; + } + return "subagent"; +} + +/** + * Stateful tracker mapping sessionIDs to human labels. + * + * Lifecycle: + * - First call to `labelFor()` returns ORCHESTRATOR_LABEL and binds that + * sessionID to it. Every subsequent event from that session gets the + * same label. + * - When the orchestrator emits a Task tool_use, the harness calls + * `recordTaskDispatch()` to push the dispatch's derived label onto a + * pending FIFO queue. + * - The next previously-unseen sessionID consumes the head of the queue. + * - If `labelFor()` is called for a new session with an empty queue + * (e.g. a subagent emitted events before the parent's tool_use was + * parsed, or the runtime spawned a session we didn't expect), the + * labeler falls back to `subagent#N` so log lines remain attributable. + */ +export class SessionLabeler { + private readonly labels = new Map(); + private readonly pendingLabels: string[] = []; + private fallbackCounter = 0; + + recordTaskDispatch(input: TaskDispatchInput): string { + const label = deriveLabelFromTaskInput(input); + this.pendingLabels.push(label); + return label; + } + + /** + * Return a label for the given sessionID. Binds on first call. + * Pass undefined/empty for events that lack a session id — the caller + * gets ORCHESTRATOR_LABEL so the line is still attributable. + */ + labelFor(sessionID: string | undefined | null): string { + if (!sessionID) return ORCHESTRATOR_LABEL; + const existing = this.labels.get(sessionID); + if (existing) return existing; + + let label: string; + if (this.labels.size === 0) { + label = ORCHESTRATOR_LABEL; + } else if (this.pendingLabels.length > 0) { + label = this.pendingLabels.shift() as string; + } else { + this.fallbackCounter += 1; + label = `subagent#${this.fallbackCounter}`; + } + this.labels.set(sessionID, label); + return label; + } + + /** number of distinct sessions seen so far (for diagnostics) */ + size(): number { + return this.labels.size; + } + + /** all (sessionID, label) pairs, oldest first */ + entries(): Array<[string, string]> { + return Array.from(this.labels.entries()); + } + + /** how many pending labels are queued waiting to bind to a new session */ + pendingDispatchCount(): number { + return this.pendingLabels.length; + } +} + +/** + * Format a log message with a session label prefix in magenta. Mirrors the + * style of utils/log.ts:prefixLines() so per-session prefixes look the same + * as the dormant withLogPrefix-based ones. + */ +export function formatWithLabel(label: string, message: string): string { + const MAGENTA = "\x1b[35m"; + const RESET = "\x1b[0m"; + const colored = `${MAGENTA}[${label}]${RESET} `; + return message + .split("\n") + .map((line) => `${colored}${line}`) + .join("\n"); +} diff --git a/models.test.ts b/models.test.ts index e9d6053..cc30298 100644 --- a/models.test.ts +++ b/models.test.ts @@ -58,7 +58,6 @@ describe("getModelEnvVars", () => { expect(getModelEnvVars("opencode/gpt-5-nano")).toEqual([]); expect(getModelEnvVars("opencode/mimo-v2-pro-free")).toEqual([]); expect(getModelEnvVars("opencode/minimax-m2.5-free")).toEqual([]); - expect(getModelEnvVars("opencode/nemotron-3-super-free")).toEqual([]); }); it("still requires OPENCODE_API_KEY for non-free opencode models", () => { diff --git a/models.ts b/models.ts index ecba51e..7a3dd02 100644 --- a/models.ts +++ b/models.ts @@ -287,12 +287,6 @@ export const providers = { envVars: [], isFree: true, }, - "nemotron-3-super-free": { - displayName: "Nemotron 3 Super", - resolve: "opencode/nemotron-3-super-free", - envVars: [], - isFree: true, - }, }, }), openrouter: provider({ diff --git a/modes.ts b/modes.ts index 1abca69..3e12133 100644 --- a/modes.ts +++ b/modes.ts @@ -1,4 +1,5 @@ // changes to mode definitions should be reflected in docs/modes.mdx +import { REVIEWER_AGENT_NAME } from "./agents/reviewer.ts"; import { type AgentId, formatMcpToolRef, pullfrogMcpName } from "./external.ts"; export interface Mode { @@ -82,9 +83,36 @@ export function computeModes(agentId: AgentId): Mode[] { - plan your approach before writing code: identify which files need to change, key design decisions, and edge cases. for non-trivial changes, consider whether there's a more elegant approach. - run relevant tests/lints before committing -4. **self-review**: delegate a read-only subagent to review your diff. the subagent must ONLY read files, grep, and search — no MCP tools, no writes, no shell commands, no side effects. provide it with the output of \`git diff\` and instruct it to look for bugs, logic errors, missing edge cases, and unintended changes. review its findings, address any valid points, and discard nitpicks or false positives. then: - - verify only intended changes are present, no debug artifacts or commented-out code remain, and no unrelated files were modified - - commit locally via shell (\`git add . && git commit -m "..."\`) +4. **self-review**: judgment call — does YOUR diff warrant a fresh-eyes pass? + + Skip self-review (commit directly) when the diff is **genuinely trivial**: + - doc typos, comment-only edits, whitespace/format-only, import reordering + - lockfile or generated-code regeneration, mechanical rename whose only effect is import-path updates (size of diff is irrelevant — read the *shape*, not the line count) + - low-risk dep patch bump from a trusted source + + Run self-review when the diff has **any behavioral surface, however small**: + - 1-line changes to SQL operators / comparison logic / regexes / redirects / HTTP methods / response codes + - any change to money / tax / currency / billing / fee / refund / payout calculations or constants + - any change to auth / permissions / roles / sessions / tokens / signature verification + - any change to feature-flag defaults, retry counts, timeouts, rate limits, batch sizes + - new endpoints, new code paths, new error branches — even small ones + - mixed diffs (whitespace + a single semantic line) — the semantic line still triggers self-review + - anything you're uncertain about + + Tie-breaker: when in doubt, run self-review. One false-positive subagent dispatch costs cents; one false-negative shipped bug costs much more. There's no value in dispatching for a typo, but there's also no excuse for skipping on a 1-line change to a billing path. + + Otherwise delegate the \`${REVIEWER_AGENT_NAME}\` subagent to review your diff with fresh eyes against YOUR TASK. The subagent's baked-in system prompt enforces a non-mutative + non-recursive contract: read-only file/search/web tools and read-only MCP queries only; no writes, shell side effects, state-changing MCP calls, or nested subagent dispatch. Enforcement is prose-only — restate the constraint in your dispatch instructions and do not relax it. + + Provide the subagent with YOUR TASK, the output of \`git diff\`, and a tight summary (not raw output) of any lint/typecheck/test failures you fixed during build — what broke, root cause, the fix — so it can check that fixes addressed root causes rather than suppressed symptoms; say "no build-phase failures" if the build path was clean. Instruct it to flag bugs, logic errors, missing edge cases, gaps between request and diff, and unintended changes. + + Delegation + research discipline (distilled from \`/anneal\` canonical — these are codified learnings from many review rounds, not theoretical best practices): + - Do NOT summarize what you implemented — that biases the subagent toward validating the shape of your solution rather than questioning it. + - Do NOT curate a reading list of files. Let the subagent discover scope from the diff and codebase. + - Do NOT pre-shape output with a severity / category schema. That leaks your hypotheses; severity is your call during evaluation. + - Do NOT defect-hunt the diff yourself in parallel with the subagent. Your role is dispatch + evaluation; doing the review yourself reintroduces the implementation bias the subagent is meant to mitigate. + - For diffs that rely on third-party API contracts, SDK semantics, framework directives, or DB engine specifics, instruct the subagent to verify load-bearing claims via web search and quote source URLs rather than trust training data — this is the single most common review-quality failure mode. + + Review the findings, address valid points, and discard nitpicks or false positives. Then verify only intended changes are present, no debug artifacts or commented-out code remain, no unrelated files were modified. Commit locally via shell (\`git add . && git commit -m "..."\`). 5. **finalize**: - confirm a clean working tree, then push via \`${t("push_branch")}\` (see *SYSTEM* Git rules if this fails — prepush errors are usually the repo's tests/lint, not infra timeouts) @@ -124,29 +152,94 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`, ${learningsStep(t, 6)}`, }, + // Review and IncrementalReview use the multi-lens orchestrator pattern + // (canonical source: .claude/commands/anneal.md). The orchestrator does + // triage → parallel read-only subagent fan-out → aggregate → draft comments + // → submit. For someone else's PR, parallel lenses (correctness, security, + // research-validated claims, user-journey, etc.) provide breadth across + // angles that a single subagent can't carry coherently. Build mode keeps + // a single fresh-eyes subagent (different problem shape — orchestrator + // wrote the code and bias-mitigation comes from delegating to one + // subagent that doesn't share the implementation context). + // 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). { name: "Review", description: "Review code, PRs, or implementations; provide feedback or suggestions; identify issues; or check code quality, style, and correctness", prompt: `### Checklist -1. Checkout the PR via \`${t("checkout_pr")}\` — this returns PR metadata and a \`diffPath\`. read the diff TOC first and treat its file line ranges as your coverage checklist. +1. **checkout**: call \`${t("checkout_pr")}\` — this returns PR metadata and a \`diffPath\`. read the diff TOC end-to-end and treat its file line ranges as your coverage checklist. -2. For each area of change: - - read the diff and trace data flow, check boundaries, and verify assumptions - - plan your investigation: identify the highest-risk areas (tricky state transitions, boundary crossings, assumption chains) and prioritize depth over breadth - - use \`${t("get_pull_request")}\` and other read-only GitHub tools for additional context - - if the PR removes features, deletes exports, renames identifiers, or changes architectural patterns, run a dedicated impact analysis: list what changed, then use grep across code, tests, docs (\`docs/\`, \`wiki/\`), comments, configs, and UI to find stale references - - report impact-analysis findings in the summary body, ordered by severity (runtime breakage > incorrect docs > stale comments) - - draft inline comments with NEW line numbers from the diff — every comment must be actionable (2-3 sentences max) - - use GitHub permalink format for code references - - for large or cross-cutting PRs that touch disparate subsystems, consider delegating read-only subagents to investigate areas in parallel. subagents must ONLY read files, grep, and search — no MCP tools, no writes, no shell commands, no side effects. collect their findings and use them to draft comments. +2. **triage**: orient yourself on the PR — identify *what kind of thing this is* (domain it touches, seams it crosses, external contracts it depends on, user-facing surfaces it changes). orientation only — defer specific defect-hunting to the subagents; pre-reviewing biases the lenses you pick. use \`${t("get_pull_request")}\` and other read-only GitHub tools for additional context if needed. -3. Self-critique: review all drafted comments and drop any that are praise, style preferences, speculative/unverified claims, about pre-existing code unrelated to the PR, or not actionable. + if the PR is **genuinely trivial**, skip steps 3–4 entirely and submit \`Reviewed — no issues found.\` per step 5. there's no value in dispatching even one lens for a typo. + + "Genuinely trivial" (skip): + - single-word doc typo, whitespace/format-only, comment-only across any number of files + - lockfile or generated-code regeneration (size of diff is irrelevant — read the *shape*) + - mechanical rename whose only effect is import-path updates + - low-risk dep patch bump + + "Looks trivial but isn't" (do **NOT** skip — small diff, big blast radius): + - any 1-line change to SQL / regex / auth / billing / permission / signature-verification code + - flipping a feature-flag default, default config value, or retry/timeout constant + - changing a money/tax/currency/fee constant by any amount + - changing an HTTP method, redirect URL, response code, or status enum + - tightening or loosening a comparison operator (\`<\` ↔ \`<=\`, \`==\` ↔ \`!=\`) + - renaming a public API surface (still trivial in shape, but needs an impact lens) + - adding a new direct dependency (supply-chain surface) + - any "typo fix" in user-facing copy that changes meaning ("approved" → "denied") + - mixed diffs where a semantic 1-liner is buried in whitespace/formatting changes + + When unsure, treat as non-trivial. The cost of one extra subagent is cents; the cost of a missed billing/auth/data bug is much more. + + otherwise pick lenses by where the PR concentrates risk — **there's no fixed count**. lens count is judgment, not a formula. concrete shapes to anchor against: + + - **1 lens** — pure refactor / mechanical rename across many files (impact); new test file with no source change (test-integrity); small isolated bug fix (correctness); doc-only PR with non-trivial technical content (research-validated or holistic) + - **2–3 lenses (most PRs land here)** — new CRUD endpoint (correctness + security + test-integrity); new UI flow (user-journey + correctness); a single bug fix in a non-critical subsystem (correctness + test-integrity); design doc covering one domain (research-validated + correctness or holistic) + - **4–5 lenses (high-stakes subsystem touches)** — any billing/payments change (billing-subsystem + correctness + security + operational-readiness); new auth flow (auth-subsystem + correctness + security + test-integrity); schema migration (schema-migration-subsystem + correctness + operational-readiness + impact); cross-subsystem PR that touches billing AND auth AND schema (one subsystem lens per domain + correctness) + - **6+ lenses** — almost always a smell; you're either covering overlapping ground or this PR should have been split. push back via the review body rather than expanding lens count. + + lenses come in two flavors, and you can mix them: + - **themed lenses** — a perspective applied across the whole diff (correctness, security, user-journey, performance, etc.). + - **subsystem lenses** — a domain-scoped frame for high-stakes subsystems the PR touches (e.g. "the auth lens", "the billing lens", "the schema-migration lens"). a subsystem lens is "review the PR specifically for what could go wrong in this subsystem" and naturally combines theme + scope. **for high-stakes domains, lead with the subsystem lens rather than the generic themed equivalent** — "billing-subsystem" outperforms "correctness on billing code" because the framing primes the subagent to remember domain-specific failure modes (double-charges, refund races, currency rounding, dispute flows) the generic lens misses. + + starter menu (combine, omit, or invent your own): + - **correctness & invariants** — bugs, races, error handling, edge cases, state-machine boundaries + - **impact** — when the PR removes features, deletes exports, renames identifiers, or changes architectural patterns: stale references in code, tests, docs (\`docs/\`, \`wiki/\`), comments, configs, UI + - **research-validated assumptions** — third-party API contracts, SDK semantics, framework directives, version-gated behavior. the subagent must verify load-bearing claims via web search and quote source URLs. + - **security** — new endpoints, authZ, input validation, secrets handling, replay/CSRF/injection, cross-tenant isolation + - **user-journey** — UX-touching flows: walk through happy path and failure modes as a user + - **operational readiness** — observability, alerting, migrations (forward + rollback), feature flags, on-call burden + - **integration & cross-cutting** — API contracts between modules, backward-compat of public surfaces, multi-service ordering + - **test integrity** — meaningful coverage for the changed behavior; deterministic; no shared-state pollution + - **performance** — N+1 queries, hot-path allocation, latency budgets, index coverage + - **holistic** — does the PR make sense as a whole? symmetric flows (delete for every create, rollback for every migration)? + - **subsystem lenses** (invent as the PR demands) — auth, billing, payments, schema migration, webhooks, secrets, RBAC, multi-tenant isolation, cron/scheduling, etc. + +3. **fan out**: dispatch one \`${REVIEWER_AGENT_NAME}\` subagent per lens — its baked-in system prompt enforces the non-mutative + non-recursive contract (read-only file/search/web tools and read-only MCP queries; no writes, shell side effects, state-changing MCP calls, or nested subagent dispatch). when picking 2+ lenses, dispatch them in a **single assistant turn with multiple parallel subagent calls**; issuing one and awaiting reply before the next collapses the fan-out into a serial review. if a subagent errors out, times out, or returns nothing usable, retry once with the same lens; if it still fails, proceed with partial coverage and note the missing lens in the review body — do not skip step 3 entirely on a single subagent failure. each subagent gets: + - the diff path / target — reading the diff and the codebase is its job + - **only one lens** — never a multi-section "review for X, Y, and Z" prompt + - **a Task \`description\` set to the lens name** (e.g. \`"security"\`, \`"correctness"\`, \`"billing-subsystem"\`) — the harness reads this field to label the subagent's log lines so parallel runs can be told apart in CI output. without it, every subagent shows up as \`subagent#N\`. + - the read-only contract restated in your dispatch instructions so the rule is present twice (the subagent's system prompt also enforces it). The test: would this call still be a no-op if reverted? If not (PR comments, branch pushes, issue updates, set_output, label changes, dependency installs, etc.), don't make it. + - if the lens touches external contracts, instruct the subagent to verify load-bearing claims via web search rather than trust training data, and to quote source URLs in its reasoning. action runs are non-interactive — there's no human in the loop to catch "I'm pretty sure Stripe does X." + - ask the subagent to report findings with file paths and NEW line numbers from the diff so you can anchor inline comments without re-reading the entire diff. + + delegation discipline: + - do NOT lens-review the diff yourself in parallel with the subagents (your job is dispatch + comment-drafting; doing the lens work yourself reintroduces the bias the fan-out avoids) + - do NOT summarize the PR for them (biases toward a validation frame) + - do NOT hand them a curated reading list (let them discover scope) + - do NOT pre-shape their output with a finding schema + - do NOT mention the other lenses (independence is the point — overlapping findings are a strong signal) + +4. **aggregate & draft**: merge findings; de-dup overlaps (two lenses catching the same issue = higher-confidence signal); trace each finding yourself before accepting it. drop praise, style preferences, speculative/unverified claims, findings about pre-existing code unrelated to the PR (heuristic: if the finding's root cause lives in lines this PR added or modified, it's in scope; otherwise drop unless the PR plausibly introduced or amplified the regression), and anything not actionable. + + for surviving findings, draft inline comments with NEW line numbers from the diff. every comment must be actionable, 2-3 sentences max. use GitHub permalink format for code references. for impact-analysis findings (stale references after rename/remove), report them in the review body ordered by severity (runtime breakage > incorrect docs > stale comments) rather than as inline comments unless they're anchored to a specific line. + +5. **submit**: ALWAYS submit exactly one review via \`${t("create_pull_request_review")}\`. Do NOT call \`report_progress\` — the review is the final record and the progress comment will be cleaned up automatically. -4. Submit — ALWAYS submit exactly one review via \`${t("create_pull_request_review")}\`. - Do NOT call \`report_progress\` — the review is the final record and the progress - comment will be cleaned up automatically. 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. - **critical issues** (blocks merge — bugs, security, data loss): @@ -160,30 +253,57 @@ ${learningsStep(t, 6)}`, - **no actionable issues**: \`approved: true\`, body: "Reviewed — no issues found."`, }, + // IncrementalReview shares Review's multi-lens orchestrator pattern but + // scopes the target to the incremental diff and adds prior-review-feedback + // tracking. The "issues must be NEW since the last Pullfrog review" filter + // lives at aggregation time (step 5), NOT in the subagent prompt — pushing + // the filter into subagents matches the canonical anneal anti-pattern of + // "list known pre-existing failures — don't flag these" and suppresses + // signal on regressions the new commits amplified. The body-format rules + // (Reviewed changes / Prior review feedback) are unchanged from the prior + // version. Same severity-table omission as Review. { name: "IncrementalReview", description: "Re-review a PR after new commits are pushed; focus on new changes since the last review", prompt: `### Checklist -1. Checkout the PR via \`${t("checkout_pr")}\` — this returns PR metadata, \`diffPath\` (full diff), and \`incrementalDiffPath\` (changes since last reviewed version, if available). read the diff TOC first and use its line ranges as your coverage checklist. +1. **checkout**: call \`${t("checkout_pr")}\` — this returns PR metadata, \`diffPath\` (full diff), and \`incrementalDiffPath\` (changes since last reviewed version, if available). read the diff TOC first and use its line ranges as your coverage checklist. -2. If \`incrementalDiffPath\` is present, read it to see what changed since the last review. This is a range-diff that isolates the net changes, filtering out base branch noise. If not present, fall back to reviewing the full PR diff. +2. **incremental scope**: if \`incrementalDiffPath\` is present, read it to see what changed since the last review. this is a range-diff that isolates the net changes, filtering out base branch noise. if not present, fall back to reviewing the full PR diff and determine what changed since Pullfrog's most recent review. -3. Fetch previous reviews via \`${t("list_pull_request_reviews")}\`. For the most recent Pullfrog review, call \`${t("get_review_comments")}\` with the review ID to retrieve specific prior line-level feedback. +3. **prior feedback**: fetch previous reviews via \`${t("list_pull_request_reviews")}\`. for the most recent Pullfrog review, call \`${t("get_review_comments")}\` with the review ID to retrieve specific prior line-level feedback. you'll need this in step 6 to track which prior comments were addressed. -4. For each area of the new changes: - - review the incremental diff while using the full diff for context - - check whether prior review feedback was addressed by the new commits - - trace data flow, check boundaries, verify assumptions, consider lifecycle, spot performance issues - - if the new commits remove, rename, or deprecate anything, run impact analysis with grep across code/tests/docs/comments/configs to find stale references and include those findings in the summary body - - never repeat prior feedback. only comment on genuinely new issues introduced by the new commits. - - draft inline comments with NEW line numbers from the full PR diff — every comment must be actionable (2-3 sentences max) - - for large or cross-cutting PRs, consider delegating read-only subagents for parallel investigation. subagents must ONLY read files, grep, and search — no MCP tools, no writes, no shell commands, no side effects. collect their findings and use them to draft comments. +4. **triage & fan out**: orient on the *incremental* changes — domain, seams, external contracts, user-facing surfaces. -5. Self-critique: drop any comments that are praise, style preferences, speculative, about pre-existing code, or not actionable. + if the incremental changes are **genuinely trivial**, skip the fan-out entirely and jump to step 7's non-substantive path (do NOT submit a review). -6. **Summarize**: build two distinct sections for the review body: + "Genuinely trivial" (skip): formatting/comment tweaks, import reordering, lockfile regen, mechanical rename of import paths, whitespace-only. + "Looks trivial but isn't" (do NOT skip — same anti-patterns as Review mode): 1-line changes to SQL/regex/auth/billing/permissions/signature-verification code; flipping feature-flag defaults or retry/timeout constants; money/tax/HTTP-method/redirect changes; tightening or loosening a comparison operator; mixed diffs with a semantic line buried in formatting. + When unsure, treat as non-trivial. + + otherwise pick lenses by where the new commits concentrate risk — **there's no fixed count**, same calibration as Review mode (1 lens for pure refactor / isolated fix; 2–3 for typical features; 4–5 for high-stakes subsystem touches; 6+ is a smell). lens framing follows Review mode: themed lenses (correctness & invariants, impact when new commits remove/rename/deprecate things, research-validated assumptions, security, user-journey, operational readiness, integration & cross-cutting, test integrity, performance, holistic) and subsystem lenses (auth, billing, schema migration, etc.) — for high-stakes domains lead with the subsystem lens rather than the generic themed equivalent. + + dispatch one \`${REVIEWER_AGENT_NAME}\` subagent per lens — its baked-in system prompt enforces the non-mutative + non-recursive contract (read-only file/search/web tools and read-only MCP queries; no writes, shell side effects, state-changing MCP calls, or nested subagent dispatch). dispatch them in a **single assistant turn with multiple parallel subagent calls** (serial dispatch collapses the fan-out). if a subagent errors out, times out, or returns nothing usable, retry once with the same lens; if it still fails, proceed with partial coverage and note the missing lens in the review body — do not skip step 4 entirely on a single subagent failure. each subagent gets: + - the diff scope (incremental diff path if available, full diff otherwise). do NOT tell them to skip pre-existing issues — that suppresses regressions the new commits amplified; the "issues must be NEW" filter lives at aggregation time (step 5), not in the subagent prompt + - **only one lens** — never a multi-section "review for X, Y, and Z" prompt + - **a Task \`description\` set to the lens name** (e.g. \`"security"\`, \`"correctness"\`, \`"billing-subsystem"\`) — the harness reads this field to label the subagent's log lines so parallel runs can be told apart in CI output. without it, every subagent shows up as \`subagent#N\`. + - the read-only contract restated in your dispatch instructions so the rule is present twice (the subagent's system prompt also enforces it). The test: would this call still be a no-op if reverted? If not (PR comments, branch pushes, issue updates, set_output, label changes, dependency installs, etc.), don't make it. + - if the lens touches external contracts, instruct the subagent to verify load-bearing claims via web search and quote source URLs. action runs are non-interactive — there's no human to catch "I'm pretty sure Stripe does X." + - ask the subagent to report findings with file paths and NEW line numbers from the full PR diff so you can anchor inline comments. + + delegation discipline: + - do NOT lens-review the diff yourself in parallel with the subagents + - do NOT summarize the changes for them (biases toward validation frame) + - do NOT hand them a curated reading list (let them discover scope) + - do NOT pre-shape their output with a finding schema + - do NOT mention the other lenses (independence is the point) + +5. **aggregate, draft, self-critique**: merge findings; de-dup overlaps; trace each finding yourself. drop praise, style preferences, speculative/unverified claims, findings about pre-existing code unrelated to the new commits, anything not actionable, and anything that re-states prior review feedback (heuristic: if the finding's root cause lives in lines the *new commits* added or modified, it's in scope; otherwise drop). To compute "lines the new commits added or modified": if \`incrementalDiffPath\` from step 1 is present, use it directly. Otherwise, take the prior Pullfrog review's \`commit_id\` (returned alongside each entry from \`${t("list_pull_request_reviews")}\` in step 3) and run \`git diff ..HEAD\` to isolate the lines added since that review. draft inline comments with NEW line numbers from the full PR diff — every comment must be actionable, 2-3 sentences max. + + then check: which prior review comments were addressed by the new commits? track the addressed ones for step 6b. + +6. **build the review body** — two distinct sections: a. **Reviewed changes**: summarize at the logical-change level, not per-file. each bullet starts with a past-tense verb (e.g. \`- Extracted shared CLI runtime into a single module\`, \`- Renamed package to pullfrog\`). avoid file paths unless they add clarity. if the changes can be described in one sentence, use one sentence — no bullets needed. b. **Prior review feedback** (only if any were addressed): list only the prior review comments that WERE addressed by the new commits (\`- [x] safeParse instead of parse — addressed\`). omit unaddressed comments. omit this entire section if nothing was addressed. a change can appear in both sections. - no headings, no tables, no prose paragraphs in either section — just bullets diff --git a/test/list-aliases.ts b/test/list-aliases.ts index 0f4062e..f1cb97f 100644 --- a/test/list-aliases.ts +++ b/test/list-aliases.ts @@ -6,9 +6,18 @@ * set MATRIX_FILTER to a substring to restrict the matrix to matching aliases * — useful for iterating on a single provider without paying for every model. * + * passthrough pruning: openrouter/* aliases and keyed opencode/* aliases are + * just routing-layer wrappers around models we already smoke-test directly + * (anthropic/*, openai/*, google/*, etc). running every passthrough burns CI + * minutes without catching anything the direct smoke doesn't. we keep one + * canary per routing layer to validate the routing layer itself is alive; + * slug-drift is caught separately by the `models-catalog` job. set + * INCLUDE_ALL_PASSTHROUGHS=1 to bypass this for full validation. + * * usage: * node action/test/list-aliases.ts * MATRIX_FILTER=gemini node action/test/list-aliases.ts + * INCLUDE_ALL_PASSTHROUGHS=1 node action/test/list-aliases.ts */ import { modelAliases } from "../models.ts"; @@ -16,10 +25,25 @@ function agentForSlug(slug: string): "claude" | "opencode" { return slug.startsWith("anthropic/") ? "claude" : "opencode"; } +// one canary per routing layer — proves the routing surface (auth, tool-call +// translation) is alive without re-testing every underlying model. +const ROUTING_CANARIES = new Set(["openrouter/claude-sonnet", "opencode/claude-sonnet"]); + +function isPrunablePassthrough(alias: (typeof modelAliases)[number]): boolean { + if (ROUTING_CANARIES.has(alias.slug)) return false; + if (alias.provider === "openrouter") return true; + // opencode FREE models (big-pickle, mimo, minimax, gpt-5-nano) are unique + // to opencode and used in prod — keep them. only prune the keyed mirrors. + if (alias.provider === "opencode" && !alias.isFree) return true; + return false; +} + const filter = process.env.MATRIX_FILTER?.trim() ?? ""; +const includeAllPassthroughs = process.env.INCLUDE_ALL_PASSTHROUGHS === "1"; const matrix = modelAliases .filter((alias) => (filter ? alias.slug.toLowerCase().includes(filter.toLowerCase()) : true)) + .filter((alias) => includeAllPassthroughs || !isPrunablePassthrough(alias)) .map((alias) => ({ slug: alias.slug, agent: agentForSlug(alias.slug), diff --git a/test/run.ts b/test/run.ts index afec1bb..ffb25a0 100644 --- a/test/run.ts +++ b/test/run.ts @@ -218,18 +218,21 @@ type RetryDecision = { retry: false } | { retry: true; reason: string; backoffMs * - security checks failed (sandbox breach, token leak, etc.) * - agent successfully ran and called set_output but produced wrong results */ -// detect rate limit / quota errors across all providers -const RATE_LIMIT_PATTERNS = [ - "Rate limit reached", // anthropic - "Resource has been exhausted", // google/gemini - "quota exceeded", // google/gemini - "429", // generic HTTP 429 - "Too Many Requests", // generic +// detect rate limit / quota errors across all providers. `\b429\b` uses word +// boundaries because a bare "429" substring false-matches UUIDs (e.g. MCP +// session ids like `...-4429-...`) and microsecond timestamps in agent stdout, +// which used to send transient failures down the 60s rate-limit retry path +// and push retries past the per-step CI timeout. +const RATE_LIMIT_PATTERNS: RegExp[] = [ + /rate limit reached/i, // anthropic + /resource has been exhausted/i, // google/gemini + /quota exceeded/i, // google/gemini + /\b429\b/, // generic HTTP 429 + /too many requests/i, // generic ]; function isRateLimited(output: string): boolean { - const lower = output.toLowerCase(); - return RATE_LIMIT_PATTERNS.some((p) => lower.includes(p.toLowerCase())); + return RATE_LIMIT_PATTERNS.some((p) => p.test(output)); } function shouldRetry(result: AgentResult, validation: ValidationResult): RetryDecision { diff --git a/utils/apiKeys.test.ts b/utils/apiKeys.test.ts index a73a7be..397ee79 100644 --- a/utils/apiKeys.test.ts +++ b/utils/apiKeys.test.ts @@ -31,7 +31,6 @@ describe("validateAgentApiKey", () => { "opencode/gpt-5-nano", "opencode/mimo-v2-pro-free", "opencode/minimax-m2.5-free", - "opencode/nemotron-3-super-free", ]) { expect(() => validateAgentApiKey({ ...base, model: slug })).not.toThrow(); } diff --git a/utils/run.ts b/utils/run.ts index a8a1b40..b1cc201 100644 --- a/utils/run.ts +++ b/utils/run.ts @@ -19,7 +19,23 @@ export async function handleAgentResult(ctx: HandleAgentResultParams): Promise