Compare commits
8 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 17b610e1a1 | |||
| ca913c76ea | |||
| 20d4b12522 | |||
| ec43c0e0d1 | |||
| 93cc7b1a44 | |||
| 851e49e2d7 | |||
| 4101df566b | |||
| 9d04cad360 |
@@ -407,6 +407,12 @@ async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
|
|||||||
activityTimeout: 300_000,
|
activityTimeout: 300_000,
|
||||||
onActivityTimeout: params.onActivityTimeout,
|
onActivityTimeout: params.onActivityTimeout,
|
||||||
stdio: ["ignore", "pipe", "pipe"],
|
stdio: ["ignore", "pipe", "pipe"],
|
||||||
|
// run claude in its own process group so SIGKILL on activity timeout /
|
||||||
|
// outer cancellation reaches any subprocesses it spawns (rg, file
|
||||||
|
// watchers, mcp transports, etc). claude itself is a node bundle so
|
||||||
|
// there's no shim-orphan issue like opencode-ai/bin/opencode, but
|
||||||
|
// detached + killGroup is the right default for any agent runtime.
|
||||||
|
killGroup: true,
|
||||||
onStdout: async (chunk) => {
|
onStdout: async (chunk) => {
|
||||||
const text = chunk.toString();
|
const text = chunk.toString();
|
||||||
output += text;
|
output += text;
|
||||||
|
|||||||
@@ -59,6 +59,22 @@ type OpenCodeConfig = {
|
|||||||
[key: string]: unknown;
|
[key: string]: unknown;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Per-inference `max_tokens` reservation the agent sends to the upstream
|
||||||
|
* model. OpenCode's default is 32_000 (sized for long-running TUI sessions
|
||||||
|
* where a human user might want big outputs). Pullfrog runs are headless and
|
||||||
|
* short — typical outputs are 1-3K tokens — so we cap at 5_000. This
|
||||||
|
* drastically reduces the upfront budget reservation OpenRouter requires per
|
||||||
|
* call (~$0.38 vs ~$2.40 for Opus), which is what lets low-wallet runs
|
||||||
|
* actually start.
|
||||||
|
*
|
||||||
|
* Plumbed via `OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX` env var rather than the
|
||||||
|
* config JSON. OpenCode's `OUTPUT_TOKEN_MAX` (session/llm.ts) is sourced
|
||||||
|
* exclusively from this env var; top-level `limit.output` in the config
|
||||||
|
* has no read site and is silently dropped on merge.
|
||||||
|
*/
|
||||||
|
const PULLFROG_OPENCODE_OUTPUT_LIMIT = 5000;
|
||||||
|
|
||||||
function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): string {
|
function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): string {
|
||||||
const config: OpenCodeConfig = {
|
const config: OpenCodeConfig = {
|
||||||
permission: {
|
permission: {
|
||||||
@@ -352,6 +368,16 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
|||||||
// mismatched callID" signal).
|
// mismatched callID" signal).
|
||||||
const knownNonTaskCallIDs = new Set<string>();
|
const knownNonTaskCallIDs = new Set<string>();
|
||||||
|
|
||||||
|
// a subagent is "in flight" while at least one `task` tool_use is awaiting
|
||||||
|
// its tool_result. used to suspend spawn's activity timer (which only sees
|
||||||
|
// opencode's stdout) so a long-running subagent — whose internal events
|
||||||
|
// don't surface on the parent NDJSON stream — can't trip the 5min timeout.
|
||||||
|
// boolean state instead of a heartbeat so there's no race window between a
|
||||||
|
// periodic tick and a subagent that finishes between ticks.
|
||||||
|
function isSubagentInFlight(): boolean {
|
||||||
|
return taskDispatchByCallID.size > 0 || pendingTaskDispatches.length > 0;
|
||||||
|
}
|
||||||
|
|
||||||
function emitSubagentFinished(
|
function emitSubagentFinished(
|
||||||
dispatch: TaskDispatch,
|
dispatch: TaskDispatch,
|
||||||
status: string,
|
status: string,
|
||||||
@@ -693,6 +719,20 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
|||||||
activityTimeout: 300_000,
|
activityTimeout: 300_000,
|
||||||
onActivityTimeout: params.onActivityTimeout,
|
onActivityTimeout: params.onActivityTimeout,
|
||||||
stdio: ["ignore", "pipe", "pipe"],
|
stdio: ["ignore", "pipe", "pipe"],
|
||||||
|
// node_modules/opencode-ai/bin/opencode is a Node shim that spawnSyncs
|
||||||
|
// the native opencode-<plat>-<arch> binary with stdio:"inherit". without
|
||||||
|
// a process-group kill, SIGKILL hits only the shim, the native binary
|
||||||
|
// is reparented to PID 1, holds our stdout pipe open, and `child.close`
|
||||||
|
// never fires — producing zombie runs. detached + killGroup nukes the
|
||||||
|
// whole tree.
|
||||||
|
killGroup: true,
|
||||||
|
// suspend the inner activity timer while a `task` subagent is in flight.
|
||||||
|
// opencode's task tool encapsulates subagent execution in-process — the
|
||||||
|
// subagent's internal events don't surface on the parent NDJSON stream,
|
||||||
|
// so without this the 5min timeout would falsely fire mid-subagent.
|
||||||
|
// suspend/resume is preferable to a heartbeat because there's no race
|
||||||
|
// between a periodic tick and a subagent finishing between ticks.
|
||||||
|
isPausedExternally: isSubagentInFlight,
|
||||||
onStdout: async (chunk) => {
|
onStdout: async (chunk) => {
|
||||||
const text = chunk.toString();
|
const text = chunk.toString();
|
||||||
output += text;
|
output += text;
|
||||||
@@ -929,6 +969,7 @@ export const opencode = agent({
|
|||||||
...homeEnv,
|
...homeEnv,
|
||||||
OPENCODE_CONFIG_CONTENT: buildSecurityConfig(ctx, model),
|
OPENCODE_CONFIG_CONTENT: buildSecurityConfig(ctx, model),
|
||||||
OPENCODE_PERMISSION: permissionOverride,
|
OPENCODE_PERMISSION: permissionOverride,
|
||||||
|
OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX: PULLFROG_OPENCODE_OUTPUT_LIMIT.toString(),
|
||||||
GOOGLE_GENERATIVE_AI_API_KEY:
|
GOOGLE_GENERATIVE_AI_API_KEY:
|
||||||
process.env.GOOGLE_GENERATIVE_AI_API_KEY || process.env.GEMINI_API_KEY,
|
process.env.GOOGLE_GENERATIVE_AI_API_KEY || process.env.GEMINI_API_KEY,
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -35,6 +35,7 @@ import { executeLifecycleHook } from "./utils/lifecycle.ts";
|
|||||||
import { normalizeEnv } from "./utils/normalizeEnv.ts";
|
import { normalizeEnv } from "./utils/normalizeEnv.ts";
|
||||||
import { aggregateUsage, patchWorkflowRunFields } from "./utils/patchWorkflowRunFields.ts";
|
import { aggregateUsage, patchWorkflowRunFields } from "./utils/patchWorkflowRunFields.ts";
|
||||||
import { resolvePayload, resolvePromptInput } from "./utils/payload.ts";
|
import { resolvePayload, resolvePromptInput } from "./utils/payload.ts";
|
||||||
|
import { isRouterKeylimitExhaustedError } from "./utils/providerErrors.ts";
|
||||||
import { readSummaryFile, seedSummaryFile } from "./utils/prSummary.ts";
|
import { readSummaryFile, seedSummaryFile } from "./utils/prSummary.ts";
|
||||||
import { postReviewCleanup } from "./utils/reviewCleanup.ts";
|
import { postReviewCleanup } from "./utils/reviewCleanup.ts";
|
||||||
import { handleAgentResult } from "./utils/run.ts";
|
import { handleAgentResult } from "./utils/run.ts";
|
||||||
@@ -186,6 +187,14 @@ function billingConsoleUrl(owner: string, anchor: "billing" | "model-access"): s
|
|||||||
* - `router_requires_card`: user is on Router mode with no card AND no
|
* - `router_requires_card`: user is on Router mode with no card AND no
|
||||||
* wallet balance. Lead with the carrot ($20 free credit), link to
|
* wallet balance. Lead with the carrot ($20 free credit), link to
|
||||||
* `#model-access` where the Add Card flow lives.
|
* `#model-access` where the Add Card flow lives.
|
||||||
|
* - `router_balance_exhausted`: user has a card on file but auto-reload is
|
||||||
|
* disabled and they've spent past their $5 overdraft buffer. Frame as
|
||||||
|
* "balance ran out" and surface both remediation paths (top up, or flip
|
||||||
|
* on auto-reload).
|
||||||
|
* - `router_keylimit_exhausted`: OpenRouter rejected mid-run because the
|
||||||
|
* per-run key budget was exhausted while the agent was working. The
|
||||||
|
* wallet is now negative; same remediation as `router_balance_exhausted`
|
||||||
|
* but framed for the after-the-fact case ("this run was cut short").
|
||||||
* - `needsReauthentication`: issuer requires 3DS on every off-session
|
* - `needsReauthentication`: issuer requires 3DS on every off-session
|
||||||
* charge. Re-adding the card won't help — the only escape is a manual
|
* charge. Re-adding the card won't help — the only escape is a manual
|
||||||
* top-up where 3DS runs interactively in Stripe Checkout.
|
* top-up where 3DS runs interactively in Stripe Checkout.
|
||||||
@@ -205,6 +214,26 @@ function formatBillingErrorSummary(error: BillingError, owner: string): string {
|
|||||||
].join("\n");
|
].join("\n");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (error.code === "router_balance_exhausted") {
|
||||||
|
return [
|
||||||
|
"**Your Pullfrog Router balance is exhausted.**",
|
||||||
|
"",
|
||||||
|
"You have a card on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer.",
|
||||||
|
"",
|
||||||
|
`[Top up balance →](${billingConsoleUrl(owner, "billing")}) · [Enable auto-reload →](${billingConsoleUrl(owner, "model-access")})`,
|
||||||
|
].join("\n");
|
||||||
|
}
|
||||||
|
|
||||||
|
if (error.code === "router_keylimit_exhausted") {
|
||||||
|
return [
|
||||||
|
"**This run was cut short — your Pullfrog Router balance ran out mid-run.**",
|
||||||
|
"",
|
||||||
|
"OpenRouter stopped the agent because the per-run budget was exhausted. Your wallet is now negative; top up or enable auto-reload to keep runs flowing.",
|
||||||
|
"",
|
||||||
|
`[Top up balance →](${billingConsoleUrl(owner, "billing")}) · [Enable auto-reload →](${billingConsoleUrl(owner, "model-access")})`,
|
||||||
|
].join("\n");
|
||||||
|
}
|
||||||
|
|
||||||
if (error.needsReauthentication) {
|
if (error.needsReauthentication) {
|
||||||
const code = error.declineCode ?? "authentication_required";
|
const code = error.declineCode ?? "authentication_required";
|
||||||
return [
|
return [
|
||||||
@@ -549,6 +578,12 @@ export async function main(): Promise<MainResult> {
|
|||||||
const resolvedModel = payload.proxyModel ? undefined : resolveModel({ slug: payload.model });
|
const resolvedModel = payload.proxyModel ? undefined : resolveModel({ slug: payload.model });
|
||||||
const agent = resolveAgent({ model: resolvedModel });
|
const agent = resolveAgent({ model: resolvedModel });
|
||||||
|
|
||||||
|
// surface the effective model in comment/review footers. payload.model is
|
||||||
|
// just the stored slug (often undefined for router/oss runs that derive
|
||||||
|
// the target from proxyModel). matching priority with resolveModelForLog
|
||||||
|
// so the "Using `…`" badge reflects what actually ran.
|
||||||
|
toolState.model = payload.proxyModel ?? resolvedModel ?? payload.model;
|
||||||
|
|
||||||
validateAgentApiKey({
|
validateAgentApiKey({
|
||||||
agent,
|
agent,
|
||||||
model: payload.proxyModel ?? resolvedModel ?? payload.model,
|
model: payload.proxyModel ?? resolvedModel ?? payload.model,
|
||||||
@@ -887,16 +922,32 @@ export async function main(): Promise<MainResult> {
|
|||||||
killTrackedChildren();
|
killTrackedChildren();
|
||||||
log.error(errorMessage);
|
log.error(errorMessage);
|
||||||
|
|
||||||
|
// Reclassify OpenRouter "key budget exhausted" mid-run errors as
|
||||||
|
// BillingError. The agent runtime surfaces this as a generic APIError,
|
||||||
|
// but it's a Pullfrog billing concern — the user's Router wallet ran
|
||||||
|
// out partway through the run. Route through the same formatBillingErrorSummary
|
||||||
|
// path as proxy-token 402s so the user gets actionable copy + a top-up
|
||||||
|
// CTA on both the job summary and the PR progress comment, instead of
|
||||||
|
// a generic "❌ Pullfrog failed" stack-trace dump.
|
||||||
|
const billingError = isRouterKeylimitExhaustedError(errorMessage)
|
||||||
|
? new BillingError(errorMessage, { code: "router_keylimit_exhausted" })
|
||||||
|
: null;
|
||||||
|
|
||||||
// best-effort summary — write the error so it's visible in the Actions summary tab
|
// best-effort summary — write the error so it's visible in the Actions summary tab
|
||||||
try {
|
try {
|
||||||
const errorSummary = `### ❌ Pullfrog failed\n\n\`\`\`\n${errorMessage}\n\`\`\``;
|
const errorSummary = billingError
|
||||||
|
? formatBillingErrorSummary(billingError, runContext.repo.owner)
|
||||||
|
: `### ❌ Pullfrog failed\n\n\`\`\`\n${errorMessage}\n\`\`\``;
|
||||||
const usageSummary = formatUsageSummary(toolState.usageEntries);
|
const usageSummary = formatUsageSummary(toolState.usageEntries);
|
||||||
const parts = [errorSummary, toolState.lastProgressBody, usageSummary].filter(Boolean);
|
const parts = [errorSummary, toolState.lastProgressBody, usageSummary].filter(Boolean);
|
||||||
await writeSummary(parts.join("\n\n"));
|
await writeSummary(parts.join("\n\n"));
|
||||||
} catch {}
|
} catch {}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
await reportErrorToComment({ toolState, error: errorMessage });
|
const commentBody = billingError
|
||||||
|
? formatBillingErrorSummary(billingError, runContext.repo.owner)
|
||||||
|
: errorMessage;
|
||||||
|
await reportErrorToComment({ toolState, error: commentBody });
|
||||||
} catch {
|
} catch {
|
||||||
// error reporting failed, but don't let it mask the original error
|
// error reporting failed, but don't let it mask the original error
|
||||||
}
|
}
|
||||||
|
|||||||
+65
-7
@@ -11,6 +11,7 @@ import {
|
|||||||
} from "../utils/diffCoverage.ts";
|
} from "../utils/diffCoverage.ts";
|
||||||
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
|
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
|
||||||
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
|
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
|
||||||
|
import { retry } from "../utils/retry.ts";
|
||||||
import { deleteProgressComment } from "./comment.ts";
|
import { deleteProgressComment } from "./comment.ts";
|
||||||
import type { ToolContext } from "./server.ts";
|
import type { ToolContext } from "./server.ts";
|
||||||
import { execute, tool } from "./shared.ts";
|
import { execute, tool } from "./shared.ts";
|
||||||
@@ -21,6 +22,29 @@ function getHttpStatus(err: unknown): number | undefined {
|
|||||||
return typeof status === "number" ? status : undefined;
|
return typeof status === "number" ? status : undefined;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* detect GitHub's generic server-side 422 ("An internal error occurred,
|
||||||
|
* please try again.") that sometimes fires on `POST /pulls/{n}/reviews`.
|
||||||
|
*
|
||||||
|
* the body is stable across occurrences and distinct from every other 422
|
||||||
|
* cause we care about (anchor validation, body length, malformed suggestion
|
||||||
|
* blocks) — those all cite the specific problem. treating this as a
|
||||||
|
* transient server error unlocks bounded in-tool retry instead of surfacing
|
||||||
|
* it to the agent with the generic "likely causes (1)(2)(3)" prompt, which
|
||||||
|
* induces whack-a-mole comment dropping on content that was never the issue.
|
||||||
|
*/
|
||||||
|
export function isTransientReviewError(err: unknown): boolean {
|
||||||
|
if (getHttpStatus(err) !== 422) return false;
|
||||||
|
const msg = err instanceof Error ? err.message : String(err);
|
||||||
|
return /internal error occurred, please try again/i.test(msg);
|
||||||
|
}
|
||||||
|
|
||||||
|
// backoff schedule for transient GitHub 422 "internal error" responses on the
|
||||||
|
// reviews endpoint. 3 attempts total (initial + 2 retries) with 1s/3s delays
|
||||||
|
// — most transient GH errors clear within a few seconds, and longer delays
|
||||||
|
// push review submission past agent-perceived responsiveness.
|
||||||
|
export const TRANSIENT_REVIEW_RETRY_DELAYS_MS = [1_000, 3_000];
|
||||||
|
|
||||||
type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number];
|
type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number];
|
||||||
export type CommentableLines = { RIGHT: Set<number>; LEFT: Set<number> };
|
export type CommentableLines = { RIGHT: Set<number>; LEFT: Set<number> };
|
||||||
|
|
||||||
@@ -483,16 +507,50 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
|
|||||||
|
|
||||||
// no body → single-step createReview (no footer needed)
|
// no body → single-step createReview (no footer needed)
|
||||||
// has body → pending + submit so we can build footer with Fix links using review ID
|
// has body → pending + submit so we can build footer with Fix links using review ID
|
||||||
|
//
|
||||||
|
// wrap the submission in `retry` so GitHub's transient 422 "internal
|
||||||
|
// error" body (distinct from anchor / body-length / suggestion 422s,
|
||||||
|
// which all cite the specific cause) clears on its own instead of
|
||||||
|
// surfacing through the generic 422 handler — that framing sent the
|
||||||
|
// agent dropping valid inline comments chasing a non-issue.
|
||||||
|
// `shouldRetry` scopes retries to the transient body only, so real
|
||||||
|
// validation 422s still fail fast.
|
||||||
let result;
|
let result;
|
||||||
try {
|
try {
|
||||||
result = body
|
result = await retry(
|
||||||
? await createAndSubmitWithFooter(ctx, params, {
|
() =>
|
||||||
body,
|
body
|
||||||
approved: approved ?? false,
|
? createAndSubmitWithFooter(ctx, params, {
|
||||||
hasComments: (params.comments?.length ?? 0) > 0,
|
body,
|
||||||
})
|
approved: approved ?? false,
|
||||||
: await createReviewWithStrandedRecovery(ctx, params);
|
hasComments: (params.comments?.length ?? 0) > 0,
|
||||||
|
})
|
||||||
|
: createReviewWithStrandedRecovery(ctx, params),
|
||||||
|
{
|
||||||
|
delaysMs: TRANSIENT_REVIEW_RETRY_DELAYS_MS,
|
||||||
|
shouldRetry: isTransientReviewError,
|
||||||
|
label: "review submission",
|
||||||
|
}
|
||||||
|
);
|
||||||
} catch (err: unknown) {
|
} catch (err: unknown) {
|
||||||
|
// GitHub's transient 422 "internal error" is distinct from anchor /
|
||||||
|
// body-length / suggestion validation failures — framing it with the
|
||||||
|
// generic "likely causes (1)(2)(3)" prompt sends the agent dropping
|
||||||
|
// comments that were never the problem. after bounded in-tool retry
|
||||||
|
// we surface a dedicated message that tells the agent to wait-and-
|
||||||
|
// retry or fall back to a body-only review.
|
||||||
|
if (isTransientReviewError(err)) {
|
||||||
|
const rawMsg = err instanceof Error ? err.message : String(err);
|
||||||
|
throw new Error(
|
||||||
|
`GitHub returned a transient 422 "internal error" on the reviews endpoint after ${TRANSIENT_REVIEW_RETRY_DELAYS_MS.length + 1} attempts. ` +
|
||||||
|
`This is a GitHub-side issue, not a problem with your review content. ` +
|
||||||
|
`Do NOT modify or drop inline comments — their content is not the cause. ` +
|
||||||
|
`Wait ~30 seconds and call this tool once more with the SAME arguments. ` +
|
||||||
|
`If it still fails, submit a body-only review (move all inline feedback into \`body\` as text) so nothing is lost. ` +
|
||||||
|
`GitHub said: ${rawMsg}`,
|
||||||
|
{ cause: err }
|
||||||
|
);
|
||||||
|
}
|
||||||
if (getHttpStatus(err) !== 422 || !params.comments?.length) throw err;
|
if (getHttpStatus(err) !== 422 || !params.comments?.length) throw err;
|
||||||
|
|
||||||
const details = params.comments.map((c) => {
|
const details = params.comments.map((c) => {
|
||||||
|
|||||||
+1
-1
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "pullfrog",
|
"name": "pullfrog",
|
||||||
"version": "0.0.205",
|
"version": "0.1.1",
|
||||||
"type": "module",
|
"type": "module",
|
||||||
"bin": {
|
"bin": {
|
||||||
"pullfrog": "dist/cli.mjs",
|
"pullfrog": "dist/cli.mjs",
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { resolveDisplayAlias } from "../models.ts";
|
import { modelAliases, resolveDisplayAlias } from "../models.ts";
|
||||||
|
|
||||||
export const PULLFROG_DIVIDER = "<!-- PULLFROG_DIVIDER_DO_NOT_REMOVE_PLZ -->";
|
export const PULLFROG_DIVIDER = "<!-- PULLFROG_DIVIDER_DO_NOT_REMOVE_PLZ -->";
|
||||||
|
|
||||||
@@ -28,7 +28,13 @@ export interface BuildPullfrogFooterParams {
|
|||||||
function formatModelLabel(slug: string): string {
|
function formatModelLabel(slug: string): string {
|
||||||
// walk the fallback chain so a deprecated stored slug shows the model the
|
// walk the fallback chain so a deprecated stored slug shows the model the
|
||||||
// run actually executed against (e.g. "GPT", not "GPT Codex").
|
// run actually executed against (e.g. "GPT", not "GPT Codex").
|
||||||
const alias = resolveDisplayAlias(slug);
|
const alias =
|
||||||
|
resolveDisplayAlias(slug) ??
|
||||||
|
// reverse-lookup: when the caller passes an effective model (proxy or
|
||||||
|
// resolved target like "openrouter/anthropic/claude-opus-4.7") instead of
|
||||||
|
// a stored alias slug, find the alias whose resolve target matches so we
|
||||||
|
// still render a friendly display name.
|
||||||
|
modelAliases.find((a) => a.resolve === slug || a.openRouterResolve === slug);
|
||||||
if (!alias) return `\`${slug}\``;
|
if (!alias) return `\`${slug}\``;
|
||||||
return alias.isFree ? `\`${alias.displayName}\` (free)` : `\`${alias.displayName}\``;
|
return alias.isFree ? `\`${alias.displayName}\` (free)` : `\`${alias.displayName}\``;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -14,7 +14,6 @@ export type WorkflowRunArtifactPatchKey =
|
|||||||
| "issueNodeId"
|
| "issueNodeId"
|
||||||
| "reviewNodeId"
|
| "reviewNodeId"
|
||||||
| "planCommentNodeId"
|
| "planCommentNodeId"
|
||||||
| "summaryCommentNodeId"
|
|
||||||
| "summarySnapshot";
|
| "summarySnapshot";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -38,7 +37,6 @@ const STRING_KEYS: WorkflowRunArtifactPatchKey[] = [
|
|||||||
"issueNodeId",
|
"issueNodeId",
|
||||||
"reviewNodeId",
|
"reviewNodeId",
|
||||||
"planCommentNodeId",
|
"planCommentNodeId",
|
||||||
"summaryCommentNodeId",
|
|
||||||
"summarySnapshot",
|
"summarySnapshot",
|
||||||
];
|
];
|
||||||
|
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { detectProviderError } from "./providerErrors.ts";
|
import { detectProviderError, isRouterKeylimitExhaustedError } from "./providerErrors.ts";
|
||||||
|
|
||||||
describe("detectProviderError", () => {
|
describe("detectProviderError", () => {
|
||||||
describe("false positives previously seen in production", () => {
|
describe("false positives previously seen in production", () => {
|
||||||
@@ -78,3 +78,50 @@ describe("detectProviderError", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("isRouterKeylimitExhaustedError", () => {
|
||||||
|
it("matches the canonical OpenRouter mid-run error", () => {
|
||||||
|
expect(
|
||||||
|
isRouterKeylimitExhaustedError(
|
||||||
|
"APIError: This request requires more credits, or fewer max_tokens. " +
|
||||||
|
"You requested up to 32000 tokens, but can only afford 22800. " +
|
||||||
|
"To increase, visit https://openrouter.ai/settings/keys and create a key with a higher total limit"
|
||||||
|
)
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("matches the 'requires more credits' phrasing on its own", () => {
|
||||||
|
expect(
|
||||||
|
isRouterKeylimitExhaustedError("This request requires more credits, or fewer max_tokens.")
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("matches the 'requested up to ... can only afford' phrasing on its own", () => {
|
||||||
|
expect(
|
||||||
|
isRouterKeylimitExhaustedError("You requested up to 8000 tokens but can only afford 1234")
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not match generic out-of-credit text", () => {
|
||||||
|
expect(isRouterKeylimitExhaustedError("Your account has insufficient credits")).toBe(false);
|
||||||
|
expect(isRouterKeylimitExhaustedError("rate_limit_exceeded")).toBe(false);
|
||||||
|
expect(isRouterKeylimitExhaustedError('{"limit": 0}')).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not match unrelated mentions of max_tokens", () => {
|
||||||
|
expect(isRouterKeylimitExhaustedError("max_tokens parameter must be a positive integer")).toBe(
|
||||||
|
false
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("matches across newlines (defends against upstream wrapping/reformatting)", () => {
|
||||||
|
expect(
|
||||||
|
isRouterKeylimitExhaustedError(
|
||||||
|
"APIError: This request requires more credits, or\nfewer max_tokens. You requested up to 32000 tokens"
|
||||||
|
)
|
||||||
|
).toBe(true);
|
||||||
|
expect(
|
||||||
|
isRouterKeylimitExhaustedError("You requested up to 32000 tokens,\nbut can only afford 22800")
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -36,3 +36,29 @@ export function detectProviderError(text: string): string | null {
|
|||||||
}
|
}
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* OpenRouter's response when the per-run key's remaining budget can't cover
|
||||||
|
* the agent's `max_tokens` reservation. Distinct from a generic provider error
|
||||||
|
* because it's a Pullfrog billing concern, not an upstream outage — the user's
|
||||||
|
* Router wallet ran out (or the key budget was undersized at mint time and the
|
||||||
|
* agent ran out of headroom partway through).
|
||||||
|
*
|
||||||
|
* Match must be specific to this exact OpenRouter error class. Generic "credits"
|
||||||
|
* or "limit" text shows up in unrelated errors and would mis-classify them.
|
||||||
|
*
|
||||||
|
* Sample:
|
||||||
|
* `APIError: This request requires more credits, or fewer max_tokens.
|
||||||
|
* You requested up to 32000 tokens, but can only afford 22800.`
|
||||||
|
*/
|
||||||
|
// `/s` (dotAll) lets `.*?` cross newlines so we still detect the error if any
|
||||||
|
// upstream layer reformats the message onto multiple lines. Without it, a
|
||||||
|
// single inserted `\n` would silently bypass the BillingError reclassification
|
||||||
|
// and the user would see the generic `❌ Pullfrog failed` dump instead of the
|
||||||
|
// actionable top-up CTA.
|
||||||
|
const ROUTER_KEYLIMIT_EXHAUSTED_PATTERN =
|
||||||
|
/requires more credits.*?fewer max_tokens|requested up to \d+ tokens.*?can only afford/is;
|
||||||
|
|
||||||
|
export function isRouterKeylimitExhaustedError(text: string): boolean {
|
||||||
|
return ROUTER_KEYLIMIT_EXHAUSTED_PATTERN.test(text);
|
||||||
|
}
|
||||||
|
|||||||
+14
-3
@@ -4,6 +4,12 @@ import { log } from "./cli.ts";
|
|||||||
export type RetryOptions = {
|
export type RetryOptions = {
|
||||||
maxAttempts?: number;
|
maxAttempts?: number;
|
||||||
delayMs?: number;
|
delayMs?: number;
|
||||||
|
/**
|
||||||
|
* explicit delay schedule — one entry per retry (length N ⇒ N+1 attempts).
|
||||||
|
* when set, overrides `maxAttempts` and `delayMs`. e.g. `[1_000, 3_000]`
|
||||||
|
* means up to 3 attempts, sleeping 1s before retry 2 and 3s before retry 3.
|
||||||
|
*/
|
||||||
|
delaysMs?: readonly number[];
|
||||||
shouldRetry?: (error: unknown) => boolean;
|
shouldRetry?: (error: unknown) => boolean;
|
||||||
label?: string;
|
label?: string;
|
||||||
};
|
};
|
||||||
@@ -20,10 +26,15 @@ const defaultShouldRetry = (error: unknown): boolean => {
|
|||||||
};
|
};
|
||||||
|
|
||||||
export async function retry<T>(fn: () => Promise<T>, options: RetryOptions = {}): Promise<T> {
|
export async function retry<T>(fn: () => Promise<T>, options: RetryOptions = {}): Promise<T> {
|
||||||
const maxAttempts = options.maxAttempts ?? 3;
|
|
||||||
const delayMs = options.delayMs ?? 1000;
|
|
||||||
const shouldRetry = options.shouldRetry ?? defaultShouldRetry;
|
const shouldRetry = options.shouldRetry ?? defaultShouldRetry;
|
||||||
const label = options.label ?? "operation";
|
const label = options.label ?? "operation";
|
||||||
|
const delays = options.delaysMs
|
||||||
|
? Array.from(options.delaysMs)
|
||||||
|
: Array.from(
|
||||||
|
{ length: (options.maxAttempts ?? 3) - 1 },
|
||||||
|
(_, i) => (options.delayMs ?? 1000) * (i + 1)
|
||||||
|
);
|
||||||
|
const maxAttempts = delays.length + 1;
|
||||||
|
|
||||||
let lastError: unknown;
|
let lastError: unknown;
|
||||||
|
|
||||||
@@ -37,7 +48,7 @@ export async function retry<T>(fn: () => Promise<T>, options: RetryOptions = {})
|
|||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
|
|
||||||
const delay = delayMs * attempt;
|
const delay = delays[attempt - 1]!;
|
||||||
log.info(`» ${label} failed (attempt ${attempt}/${maxAttempts}), retrying in ${delay}ms...`);
|
log.info(`» ${label} failed (attempt ${attempt}/${maxAttempts}), retrying in ${delay}ms...`);
|
||||||
await sleep(delay);
|
await sleep(delay);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import { performance } from "node:perf_hooks";
|
||||||
import { describe, expect, it } from "vitest";
|
import { describe, expect, it } from "vitest";
|
||||||
import { spawn } from "./subprocess.ts";
|
import { spawn } from "./subprocess.ts";
|
||||||
|
|
||||||
@@ -48,6 +49,79 @@ describe("spawn error path", () => {
|
|||||||
expect(afterHandles).toBeLessThanOrEqual(beforeHandles);
|
expect(afterHandles).toBeLessThanOrEqual(beforeHandles);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("killGroup: true propagates SIGKILL to grandchildren so close fires promptly", async () => {
|
||||||
|
// regression: node_modules/opencode-ai/bin/opencode is a Node shim that
|
||||||
|
// spawnSyncs the native binary with stdio:"inherit". without killGroup,
|
||||||
|
// child.kill("SIGKILL") hit only the shim — the native binary was
|
||||||
|
// reparented to PID 1, kept holding our stdout pipe via the inherited
|
||||||
|
// fds, and `child.on("close")` never fired (because pipes stayed open).
|
||||||
|
// a 5-min outer safety-net timer eventually rejected the agent promise,
|
||||||
|
// but the grandchild kept running until the GitHub Actions job-level
|
||||||
|
// timeout. this test replicates the shape with bash + a backgrounded
|
||||||
|
// sleep grandchild: with killGroup, close fires promptly after SIGKILL;
|
||||||
|
// without it, the parent would wait for sleep to exit (30s).
|
||||||
|
//
|
||||||
|
// the activity-check interval is fixed at 5s so the earliest the kill
|
||||||
|
// can fire is ~5s after start. budget 15s end-to-end.
|
||||||
|
const before = performance.now();
|
||||||
|
const result = await spawn({
|
||||||
|
cmd: "bash",
|
||||||
|
args: ["-c", "sleep 30 & wait"],
|
||||||
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
||||||
|
activityTimeout: 1000,
|
||||||
|
killGroup: true,
|
||||||
|
}).catch((err) => err);
|
||||||
|
const elapsed = performance.now() - before;
|
||||||
|
|
||||||
|
expect(result).toBeInstanceOf(Error);
|
||||||
|
// 10s ceiling: 5s activity-check tick + signal delivery. a regression
|
||||||
|
// here (no killGroup) would hang for the full 30s sleep.
|
||||||
|
expect(elapsed).toBeLessThan(10_000);
|
||||||
|
}, 20_000);
|
||||||
|
|
||||||
|
it("isPausedExternally suspends the activity timer while it returns true", async () => {
|
||||||
|
// opencode's `task` tool encapsulates subagent execution in-process —
|
||||||
|
// subagent-internal events don't reach our parent NDJSON stream. this
|
||||||
|
// means the child's stdout falls silent for the full subagent duration
|
||||||
|
// even when real work is happening. opencode passes a predicate keyed
|
||||||
|
// off its in-flight task dispatch tracker; while it returns true, the
|
||||||
|
// activity timer must skip the kill decision and reset its baseline.
|
||||||
|
const result = await spawn({
|
||||||
|
cmd: "bash",
|
||||||
|
args: ["-c", "sleep 8; echo done"],
|
||||||
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
||||||
|
activityTimeout: 1000,
|
||||||
|
isPausedExternally: () => true,
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.exitCode).toBe(0);
|
||||||
|
expect(result.stdout).toContain("done");
|
||||||
|
}, 20_000);
|
||||||
|
|
||||||
|
it("isPausedExternally fires normally once it returns false again", async () => {
|
||||||
|
// verifies suspend/resume rather than unconditional bypass: the predicate
|
||||||
|
// flips to false after a few hundred ms, so the timer should resume from
|
||||||
|
// a fresh baseline (advanced during the paused window) and fire after
|
||||||
|
// activityTimeout idle time. critical that lastActivityTime is reset
|
||||||
|
// while paused — otherwise the resume tick would see a stale baseline
|
||||||
|
// and kill instantly.
|
||||||
|
let paused = true;
|
||||||
|
setTimeout(() => {
|
||||||
|
paused = false;
|
||||||
|
}, 500);
|
||||||
|
|
||||||
|
const result = await spawn({
|
||||||
|
cmd: "sleep",
|
||||||
|
args: ["30"],
|
||||||
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
||||||
|
activityTimeout: 1000,
|
||||||
|
isPausedExternally: () => paused,
|
||||||
|
}).catch((err) => err);
|
||||||
|
|
||||||
|
expect(result).toBeInstanceOf(Error);
|
||||||
|
expect(String(result)).toMatch(/activity timeout/i);
|
||||||
|
}, 20_000);
|
||||||
|
|
||||||
it("reports signal-killed subprocesses as failures, not success", async () => {
|
it("reports signal-killed subprocesses as failures, not success", async () => {
|
||||||
// regression: before the fix, `child.on("close", (exitCode) => ...)`
|
// regression: before the fix, `child.on("close", (exitCode) => ...)`
|
||||||
// discarded the signal parameter and `exitCode || 0` coerced the
|
// discarded the signal parameter and `exitCode || 0` coerced the
|
||||||
|
|||||||
+54
-5
@@ -106,6 +106,25 @@ export interface SpawnOptions {
|
|||||||
stdio?: ("pipe" | "ignore" | "inherit")[];
|
stdio?: ("pipe" | "ignore" | "inherit")[];
|
||||||
onStdout?: (chunk: string) => void;
|
onStdout?: (chunk: string) => void;
|
||||||
onStderr?: (chunk: string) => void;
|
onStderr?: (chunk: string) => void;
|
||||||
|
// when true, spawn the child detached (its own process group) and route all
|
||||||
|
// kill paths (timeout, activity timeout, ctrl-c) through `process.kill(-pid, ...)`
|
||||||
|
// so signals reach grandchildren too. critical for binaries that fork through
|
||||||
|
// a shim (e.g. node_modules/opencode-ai/bin/opencode is a Node shim that
|
||||||
|
// spawnSync's the native binary; without killGroup, SIGKILL only hits the
|
||||||
|
// shim and the native binary is reparented to PID 1, holds our stdout pipe
|
||||||
|
// open, keeps emitting NDJSON, and `child.on("close")` never fires —
|
||||||
|
// producing zombie runs that hang until the GitHub Actions job timeout).
|
||||||
|
killGroup?: boolean;
|
||||||
|
// optional pause predicate consulted on every activity check. when it
|
||||||
|
// returns true the activity timer skips the kill decision *and* resets
|
||||||
|
// its idle baseline so a clean unpause can't immediately fire on a stale
|
||||||
|
// lastActivityTime. opencode uses this because its `task` tool encapsulates
|
||||||
|
// subagent execution in-process — subagent-internal events don't surface
|
||||||
|
// on the parent's NDJSON stream, so the local stdout-only signal would
|
||||||
|
// falsely fire mid-subagent. preferred over fake-activity heartbeats
|
||||||
|
// because there's no race window between a heartbeat tick and a subagent
|
||||||
|
// that finishes between ticks.
|
||||||
|
isPausedExternally?: () => boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface SpawnResult {
|
export interface SpawnResult {
|
||||||
@@ -127,6 +146,8 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
let stdoutBuffer = "";
|
let stdoutBuffer = "";
|
||||||
let stderrBuffer = "";
|
let stderrBuffer = "";
|
||||||
|
|
||||||
|
const killGroup = options.killGroup ?? false;
|
||||||
|
|
||||||
return new Promise((resolve, reject) => {
|
return new Promise((resolve, reject) => {
|
||||||
// security: caller must provide complete env object, not merged with process.env
|
// security: caller must provide complete env object, not merged with process.env
|
||||||
const child = nodeSpawn(options.cmd, options.args, {
|
const child = nodeSpawn(options.cmd, options.args, {
|
||||||
@@ -136,10 +157,28 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
},
|
},
|
||||||
stdio: options.stdio || ["pipe", "pipe", "pipe"],
|
stdio: options.stdio || ["pipe", "pipe", "pipe"],
|
||||||
cwd: options.cwd || process.cwd(),
|
cwd: options.cwd || process.cwd(),
|
||||||
|
detached: killGroup,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// sends `signal` to the entire process group when killGroup is set, so
|
||||||
|
// grandchildren (e.g. the native opencode binary spawned by the
|
||||||
|
// opencode-ai Node shim) die with the parent. falls back to a direct
|
||||||
|
// child kill if the process-group send fails (common when the child
|
||||||
|
// already exited or was never made a process group leader).
|
||||||
|
const killSelf = (signal: NodeJS.Signals): void => {
|
||||||
|
if (killGroup && child.pid) {
|
||||||
|
try {
|
||||||
|
process.kill(-child.pid, signal);
|
||||||
|
return;
|
||||||
|
} catch {
|
||||||
|
// fall through to direct kill
|
||||||
|
}
|
||||||
|
}
|
||||||
|
child.kill(signal);
|
||||||
|
};
|
||||||
|
|
||||||
// track child for cleanup on Ctrl+C
|
// track child for cleanup on Ctrl+C
|
||||||
trackChild({ child });
|
trackChild({ child, killGroup });
|
||||||
|
|
||||||
let timeoutId: NodeJS.Timeout | undefined;
|
let timeoutId: NodeJS.Timeout | undefined;
|
||||||
let sigkillEscalatorId: NodeJS.Timeout | undefined;
|
let sigkillEscalatorId: NodeJS.Timeout | undefined;
|
||||||
@@ -157,7 +196,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
if (options.timeout) {
|
if (options.timeout) {
|
||||||
timeoutId = setTimeout(() => {
|
timeoutId = setTimeout(() => {
|
||||||
isTimedOut = true;
|
isTimedOut = true;
|
||||||
child.kill("SIGTERM");
|
killSelf("SIGTERM");
|
||||||
|
|
||||||
// track the escalator so a graceful SIGTERM response (close fires
|
// track the escalator so a graceful SIGTERM response (close fires
|
||||||
// before the 5s elapses) can clear it. without capture, this timer
|
// before the 5s elapses) can clear it. without capture, this timer
|
||||||
@@ -165,7 +204,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
// past a timed-out subprocess's clean exit.
|
// past a timed-out subprocess's clean exit.
|
||||||
sigkillEscalatorId = setTimeout(() => {
|
sigkillEscalatorId = setTimeout(() => {
|
||||||
if (!child.killed) {
|
if (!child.killed) {
|
||||||
child.kill("SIGKILL");
|
killSelf("SIGKILL");
|
||||||
}
|
}
|
||||||
}, 5000);
|
}, 5000);
|
||||||
}, options.timeout);
|
}, options.timeout);
|
||||||
@@ -177,6 +216,16 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
`spawn activity timer: pid=${child.pid} cmd=${options.cmd} timeout=${activityTimeoutMs}ms`
|
`spawn activity timer: pid=${child.pid} cmd=${options.cmd} timeout=${activityTimeoutMs}ms`
|
||||||
);
|
);
|
||||||
activityCheckIntervalId = setInterval(() => {
|
activityCheckIntervalId = setInterval(() => {
|
||||||
|
// when an external pause predicate says we're suspended (e.g.
|
||||||
|
// opencode in a long-running task subagent whose internal events
|
||||||
|
// don't surface on stdout), advance lastActivityTime to "now" so a
|
||||||
|
// clean unpause doesn't immediately fire on a stale baseline, and
|
||||||
|
// skip the kill decision for this tick.
|
||||||
|
if (options.isPausedExternally?.()) {
|
||||||
|
lastActivityTime = performance.now();
|
||||||
|
log.debug(`spawn activity check: pid=${child.pid} paused externally`);
|
||||||
|
return;
|
||||||
|
}
|
||||||
const idleMs = performance.now() - lastActivityTime;
|
const idleMs = performance.now() - lastActivityTime;
|
||||||
log.debug(
|
log.debug(
|
||||||
`spawn activity check: pid=${child.pid} idle=${Math.round(idleMs)}ms / ${activityTimeoutMs}ms`
|
`spawn activity check: pid=${child.pid} idle=${Math.round(idleMs)}ms / ${activityTimeoutMs}ms`
|
||||||
@@ -186,9 +235,9 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
killedAtIdleMs = idleMs;
|
killedAtIdleMs = idleMs;
|
||||||
const idleSec = Math.round(idleMs / 1000);
|
const idleSec = Math.round(idleMs / 1000);
|
||||||
log.info(
|
log.info(
|
||||||
`no output for ${idleSec}s from pid=${child.pid} (${options.cmd}), killing process`
|
`no output for ${idleSec}s from pid=${child.pid} (${options.cmd}), killing process${killGroup ? " group" : ""}`
|
||||||
);
|
);
|
||||||
child.kill("SIGKILL");
|
killSelf("SIGKILL");
|
||||||
clearInterval(activityCheckIntervalId);
|
clearInterval(activityCheckIntervalId);
|
||||||
try {
|
try {
|
||||||
options.onActivityTimeout?.();
|
options.onActivityTimeout?.();
|
||||||
|
|||||||
Reference in New Issue
Block a user