Compare commits

...

15 Commits

Author SHA1 Message Date
Colin McDonnell d04c1ca3da action: bump to 0.1.6 2026-05-13 18:23:45 +00:00
Colin McDonnell ae976e7159 parallel tool execution: enable opencode batch + nudge agents to parallelize (#719)
opencode: opt into `experimental.batch_tool` (anomalyco/opencode#2983) so the
`batch` tool registers and the model can bundle 1-25 independent calls into one
round trip. edit calls are excluded upstream.

instructions.ts: add a "Parallel tool execution" section to the SYSTEM Workflow
block, agent-specialized via ctx.agentId. uses Anthropic's canonical wording
("invoke all relevant tools simultaneously...") so Claude reliably emits multiple
tool_use blocks per message; tells OpenCode about the new `batch` affordance.

verified end-to-end against haiku-class models (sonnet for claude, default for
opencode) with a "read 3 files and report first lines" fixture. results:
- opencode used `batch` with 3 nested reads AND emitted 3 native parallel
  read calls in the same assistant turn
- claude went from 3 serial turns (1 read each) to 1 message with 3 parallel
  Read tool_use blocks
2026-05-13 18:05:39 +00:00
Colin McDonnell 5aabd1e4a9 fix(action): cap subprocess stdout/stderr retention to prevent RangeError crashes (#680) (#715)
* fix(action): cap subprocess stdout/stderr retention to prevent RangeError crashes (#680)

unbounded `stdoutBuffer += chunk` / `stderrBuffer += chunk` in
`action/utils/subprocess.ts` previously crashed the wrapper with
`RangeError: Invalid string length` once V8's ~1 GiB kMaxLength was
breached on long-lived agent runs. multi-lens opencode Reviews on large
monorepos (e.g. tambo-ai/buildy) hit this consistently — 23 runs in the
last 24h, 100% of Review-mode hard failures on that repo.

- add `retain: "tail" | "none"` to SpawnOptions, defaulting to "tail"
  with an 8 MiB cap. tail-mode prepends a `... [N MiB truncated] ...`
  sentinel so downstream consumers can detect truncation.
- export `TailBuffer` helper for callers that need the same bounded
  accumulator semantics at their own layer.
- wrap stream `data` listeners in try/catch as defense in depth — any
  synchronous throw inside a stream handler is otherwise fatal.
- opencode + claude pass `retain: "none"` (they drain via onStdout /
  onStderr) and switch their own `output` accumulators to TailBuffer.
  their error paths read the agent-layer bounded mirrors instead of
  the now-empty `result.stdout` / `result.stderr`.
- add `failure:string-length-overflow` heuristic to scripts/analyze-logs.ts
  so post-fix recurrences are visible at a glance instead of bucketing
  into `failure:unknown`.
- regression tests cover >1 MiB stderr without crash, retain:"none"
  contract, and TailBuffer truncation semantics.

* fix: avoid TS parameter property syntax in TailBuffer for strip-only node loader

* address review: clarify try/catch scope + lock retain default to "tail"

- the original comment claimed the try/catch caught "any synchronous throw"
  in the data listener, but `options.onStdout?.(chunk)` returns a Promise
  in the agent callers (claude.ts:569, opencode.ts:933) — a throw inside
  an async user callback surfaces as an unhandled Promise rejection, not
  a synchronous exception. reword to describe the actual protection:
  defense-in-depth for synchronous throws in the listener body, which is
  exactly the shape of the original RangeError on `+= chunk`.
- add a test that locks `retain` default to "tail" by spawning without
  the option and asserting `result.stderr` is non-empty. a future refactor
  that flipped the default to "none" would silently break gitAuth,
  package installs, and lifecycle hooks that read result.stderr for
  failure messages, and the rest of the suite wouldn't catch it.
2026-05-13 17:54:28 +00:00
Colin McDonnell 60cc8772a6 fix(log-audit): kill 404 noise from /api/github/installation-token at source (#693) (#708)
* fix(log-audit): kill 404 noise from `/api/github/installation-token` at source (#693)

Closes #693. Issue diagnosed a surface symptom (`log.error` on expected
404s) but missed the actual root causes. Investigation revealed two
distinct populations producing identical 3-call 404 bursts:

1. **Fork-CI on `pullfrog/pullfrog`**: `test-token.yml` and
   `trigger-sync.yml` ship with `on: push: main`, so every fork inherits
   them and 404s our token endpoint on first push. Self-inflicted noise
   that scales with fork count.
2. **Real users hitting the full action without installing the App**:
   `/api/repo/.../run-context` uses the caller's `GITHUB_TOKEN` to read
   the repo from GitHub and then unconditionally lazy-provisions
   Account+Repo rows via `fetchOrCreateRepo`, even when the App isn't
   installed. Generates phantom DB rows and false `new account created`
   team@ alerts. (Confirmed via Prisma: `ezcorp-org` has an Account row
   with `installerLogin: null`, never installed our App.)

Both populations then trip the client retry loop in
`acquireTokenViaOIDC`, which matched `"Token exchange failed"` and
retried 3× on terminal 4xx — tripling log volume and wasting CI time.

## Changes

- `action/.github/workflows/{test-token,trigger-sync}.yml`: gate jobs
  with `if: github.repository == 'pullfrog/pullfrog'`. Forks inherit
  the files but the jobs no-op.
- `app/api/repo/[owner]/[repo]/run-context/route.ts`: call
  `getRepoInstallation` first; return 404 with install URL if the App
  isn't installed, before any DB writes or GitHub repo fetch.
- `action/utils/github.ts`: introduce `TokenExchangeError` for non-2xx
  server responses; `acquireNewToken` no longer retries it. Retry now
  fires only on genuine network/timeout failures. 404 surfaces a
  user-actionable error pointing at the install URL.
- `app/api/github/installation-token/route.ts`: move `log.error` inside
  the 500 branch only. 404 branch is silent (expected user-state) and
  returns the same install URL message for consistency.

## Effect

- Better Stack `level=error` lines from this path: 6/day → 0.
- Failed user-trial CI time: 3 wasted token requests → 1.
- User-facing error: opaque `Token exchange failed: 404` → actionable
  install URL.
- No more phantom Account rows from never-installed callers.

Skipped per design discussion: phantom-account cleanup (conservative —
stop the bleed, leave history), `AGENTS.md` rule (overgeneralized).

* review: address oracle leak + per-env install URL + retryable 5xx

Addresses pullfrog[bot] (IMPORTANT) and Copilot review findings on #708:

- **Install-status oracle in `run-context`** [pullfrog, Copilot]:
  `getRepoInstallation` runs with our App's JWT, *before* the caller's
  bearer token is validated against the repo. Pre-PR the route was
  uniformly bad-token-shaped; the new install-specific 404 turned it
  into an unauthenticated oracle distinguishing "Pullfrog installed
  here" from "not installed". Collapsed the 404 message to match the
  outer catch's ambiguous "repository not found or token lacks access".
  Legit runners still get the actionable install URL from
  `/api/github/installation-token`, which IS gated by OIDC.

- **Hardcoded `github.com/apps/pullfrog`** [Copilot]: server-side
  `installation-token` now uses `GITHUB_APP_INSTALL_URL` from
  `app/globals.ts`, so dev/staging deployments with a different
  `GITHUB_APP_SLUG` direct users to the correct app. Action-side
  echoes the server's `error` body when present (single source of
  truth) and falls back to a generic message only if the body isn't
  JSON.

- **Transient 5xx/429 made terminal** [Copilot]: `shouldRetry` now
  returns `true` for `TokenExchangeError` with `status >= 500` or
  `status === 429`. 4xx remains terminal (the actual #693 fix). Real
  outages no longer fail the workflow immediately.

- **Stale comment** [pullfrog, Copilot]: reworded the comment at
  `installation-token/route.ts:141` to reflect the new retry policy
  ("the action surfaces this once (no retry)" instead of "the action
  retries on this").

* review: restore caller-token-first auth in run-context

Pre-PR, `getEnrichedRepo({owner, repo, token})` used the caller's
token as the auth boundary — `getRepo({token})` succeeding was the
proof-of-access check. My initial install-gate inverted the order
and ran the App-credentialed `getRepoInstallation` first, which is
how it became:

- an install-status oracle (pullfrog bot, addressed previously by
  matching the outer-catch wording), and
- an outbound amplifier against our App JWT for arbitrary `owner/repo`
  (pullfrog bot, this commit).

Reordered so `getRepo({token})` runs first. Garbage / unauthorized
bearers get rejected by github (mapped to 403 by the outer catch)
before any App-credentialed call fires. `getRepo` is cached 5min,
so `getEnrichedRepo` below remains a free re-hit.
2026-05-13 17:47:13 +00:00
Colin McDonnell 4260984257 attribute claude subagent log lines + per-session thinking timer; tighten lens calibration (#700)
* attribute claude subagent log lines + per-session thinking timer; tighten lens calibration

three orthogonal fixes diagnosed from the 10m PR-699 review run:

1. wire SessionLabeler into the Claude Code harness. claude-agent-sdk
   stamps every Assistant/User/System message with session_id and a
   non-null parent_tool_use_id when emitted from a subagent context, so
   the same FIFO labeler the OpenCode harness uses works here too.
   parallel reviewfrog dispatches now log with [lens:correctness] /
   [lens:operational-readiness] / etc. prefixes instead of being
   indistinguishable from the orchestrator. matches both "Task" and
   "Agent" tool names per the v2.1.63 rename.

2. one ThinkingTimer per session. the global timer treated cross-session
   interleaving (parent thinks → child tool_call, child returns →
   parent dispatches next) as parent thinking time, so individual
   "thought for Xs" numbers were untrustworthy. each session now owns
   its own timer and prefixes its own log line.

3. tighten the Review/IncrementalReview lens-add discipline. PR-699
   triggered 4 lenses on a typical refactor (no auth/billing/schema)
   when the prompt's own calibration says 2-3 is typical; the
   research-validated lens went deep on Resend idempotency window +
   prisma updateMany lost-updates without either being load-bearing.
   adds an explicit "name the failure mode this lens would catch
   that the diff plausibly introduces" bar, and tightens
   research-validated specifically: only when correctness depends on
   the third-party contract, not when the API is merely used.

side benefits from #1: subagents' TodoWrite events no longer clobber
the orchestrator's progress comment; subagent text no longer overwrites
finalOutput; system-event handler safely routes through eventLabel even
though SDK only emits system:init for the top-level query today.

* fix node strip-only mode: declare formatLine as field, not parameter property

* key claude subagent labels by parent_tool_use_id, not session_id

claude-agent-sdk runs subagents inside the orchestrator's session — they
share session_id — and stamps subagent messages with parent_tool_use_id
pointing at the Agent tool_use that spawned them. e2e on PR-700 with
preview-700-claude-labeling#1 confirmed the original session_id-keyed
wiring never differentiated subagent activity (only the dispatch line
got [lens:correctness] in the log; the subagent's reads, writes, and
todos all rendered as orchestrator).

extend SessionLabeler so labelFor accepts an optional parent_tool_use_id
and short-circuits to a direct map keyed by Agent tool_use id when set.
recordTaskDispatch optionally takes the Agent tool_use id (block.id at
dispatch time) and binds it. orchestrator events keep flowing through
the sessionID/FIFO path unchanged so opencode wiring is untouched.

* drop weak timer test that asserted only field isolation

per pullfrog review on PR-700: the 'two timers do not bleed timestamps'
test only verified that two ThinkingTimer instances have separate
private fields, which has always been true. doesn't earn its keep —
the per-session behavior is exercised by integration through claude.ts
+ opencode.ts.
2026-05-13 15:28:08 +00:00
Colin McDonnell d5f881e9fc action: trim sensitive env values before GitHub Actions log masking (#698)
* action: trim sensitive env values before GitHub Actions log masking

GitHub Actions' log masking is line-based: a secret value containing a
newline only registers the first line as a mask, leaving the remainder
exposed verbatim in logs. A trailing newline copied from a terminal into
a GitHub Actions secret (e.g. ANTHROPIC_API_KEY) was enough to leak
"a large part of the key" in run logs (pullfrog/pullfrog#41).

normalizeEnv now trims leading/trailing whitespace from any value whose
key matches the sensitive name pattern, masks the cleaned value, and
warns when whitespace was stripped so the user notices the source.
sanitizeSecret is reused for dbSecrets injection in main.ts. The three
secret-store PUT/POST routes also trim values defensively, matching the
existing name.trim() pattern.

Real multi-line secrets are not used in practice — even GITHUB_PRIVATE_KEY
PEMs are stored single-line with escaped \n and unescaped at the point of
use — so a straight trim() is safe.

* action: address review — use core.setSecret for masking, don't zero whitespace-only

Pullfrog's review of #698 caught two real issues in the original fix:

1. `console.log(\`::add-mask::\${trimmed}\`)` doesn't escape \r/\n. If a
   value survives trim with an embedded newline (PEMs, kubeconfigs, JSON),
   the runner only registers the first line as a mask and the rest leaks.
   `core.setSecret(trimmed)` routes through @actions/core which
   percent-encodes \r/\n so the runner V2 parser decodes back to the full
   value and registers every non-empty line as a separate mask. Removes
   the load-bearing "no embedded newlines" invariant from the fix.

2. Whitespace-only sensitive values silently became "". Downstream
   truthy checks would flip from "set" to "missing" with no log. Now
   sanitizeSecret returns null in that case and callers skip the
   process.env write, surfacing a clear missing-key error instead.

Tests rewritten to assert process.env state directly — no stdout spies.
Masking correctness is delegated to @actions/core (trusted dependency).
2026-05-13 15:27:13 +00:00
Colin McDonnell 1dc53043a6 chore: bump action to 0.1.5 2026-05-13 04:56:01 +00:00
Colin McDonnell 076e5a17b5 default Claude Code effort to high
max effort burns roughly 2x the wall time per turn for marginal quality
gain. high is the model's tuned default ('equivalent to not setting the
parameter' per Anthropic docs). full-send can be reintroduced as an
opt-in per-run override later if needed.
2026-05-13 04:49:07 +00:00
Colin McDonnell d5d8a0d7ac fix(#691): drop opencode/gpt-5-nano + opencode/mimo-v2-pro-free (not actually keyless on Zen) (#695)
* remove opencode/gpt-5-nano and opencode/mimo-v2-pro-free from catalog

#7 delete aliases. both were listed as `isFree: true, envVars: []` but
neither is keyless on opencode zen, producing a hard-fail
`UnknownError: Model not found: opencode/<id>` on every run without an
opencode_api_key. fixes pullfrog/app#691 (5 runs across 3 repos, 100%
failure rate in the last 24h).

root cause: opencode's provider gate
(`packages/opencode/src/provider/provider.ts` `opencode:` loader) keeps
a zen model only when models.dev reports `cost.input === 0` for it,
then signs requests with `apiKey: "public"`. paid zen models get
deleted from the autoloaded set and opencode surfaces the deletion as
"model not found".

- `opencode/gpt-5-nano`: models.dev reports `cost: {input: 0.05, output:
  0.4, cache_read: 0.005}`. paid → requires `OPENCODE_API_KEY`.
- `opencode/mimo-v2-pro-free`: free on models.dev but not in
  `https://opencode.ai/zen/v1/models` — zen never served it, so even
  the public-key path fails.

remaining free aliases (`opencode/big-pickle`,
`opencode/minimax-m2.5-free`) both pass both checks (cost.input === 0
in models.dev AND present in zen's served list) and continue to work
without a key — verified against the opencode source.

callers swept: `action/utils/apiKeys.test.ts`, `action/models.test.ts`,
`action/test/list-aliases.ts`, `action/test/model-smoke.ts`,
`components/ModelSelector.tsx` (`modelIdToUpstream`),
`wiki/model-resolution.md`, `wiki/models-catalog.md`. wrote up the
free-zen verification rule in models-catalog so the next maintainer
can sanity-check both conditions before adding any `isFree` alias.

users with a stored `opencode/gpt-5-nano` or `opencode/mimo-v2-pro-free`
will now fall through `resolveCliModel → undefined` into the auto-select
path — a strict improvement over today's hard fail. no DB migration
needed; the slugs are simply unknown and treated like any other
unrecognized stored value.

* rework: keep mimo deprecated, demote gpt-5-nano to paid, add free-zen invariants

revised approach after the first commit over-corrected. mimo was never
broken at runtime — `fallback: "opencode/big-pickle"` already routes
stored values through to a real free model before any zen call. the
literal `opencode/mimo-v2-pro-free` being absent from zen's served list
is irrelevant because `resolveCliModel` walks the chain first. restoring
it as-is.

the actual bug was `opencode/gpt-5-nano`: marked `isFree: true,
envVars: []` but `models.dev` reports `cost: {input: 0.05, output: 0.4}`
on the opencode provider, so opencode's keyless gate
(`packages/opencode/src/provider/provider.ts` `opencode:`) deletes it
when `OPENCODE_API_KEY` is missing and the run hard-fails with
`UnknownError: Model not found: opencode/gpt-5-nano`. demoting it to a
regular paid zen alias (drop `isFree`/`envVars: []`, add
`openRouterResolve: "openrouter/openai/gpt-5-nano"` — verified to exist
on openrouter at the same price). users without `OPENCODE_API_KEY` now
get our explicit "no API key found" error pointing at the secrets page
instead of opencode's cryptic upstream error. confirmed via
`https://opencode.ai/zen/v1/models` that zen serves no free GPT
variants, so there's no cheaper-than-`gpt-mini` free option to suggest
in its place.

CI gap analysis (why this slipped through):

- `models-catalog.main.test.ts` only checked existence + `status !==
  "deprecated"` on models.dev. paid-model-marked-free regressions and
  zen-served-list drift both passed.
- `models-live` (`model-smoke.ts`) runs with `OPENCODE_API_KEY` in env,
  so the keyless deletion gate never fires. `gpt-5-nano` returned "OK"
  in CI even though end users hit a hard fail.
- `model-smoke.ts` walks the fallback chain, so mimo would have been
  smoked as big-pickle anyway — the dead resolve target was never
  exercised directly. (this is the right design; the gap is at the
  catalog layer, not the smoke layer.)

new tests:

- PR-blocking, static (`action/test/models.test.ts`, `isFree
  invariants`): every `isFree` alias must live under `opencode`, have
  `envVars: []`, omit `openRouterResolve`, AND have a fallback chain
  whose terminal alias is also `isFree` (catches "deprecate a free
  alias to a paid target" — the worst silent-charge regression).
- main-only, network (`action/test/models-catalog.main.test.ts`,
  `opencode Zen served list`): every alias whose terminal-fallback
  resolve is `opencode/*` must appear in
  `https://opencode.ai/zen/v1/models`. catches zen dropping a model
  from its served list.
- main-only, network (same file, `isFree models.dev cost`): every
  `isFree` alias's terminal-fallback resolve must have `cost.input ===
  0` in the `opencode` provider block on `models.dev`. would have
  caught `gpt-5-nano` at the next models-bump run.

both network tests dedupe on terminal resolve, so deprecated aliases
sharing a target aren't double-counted. `pnpm vitest run`: 113 static
tests pass. `pnpm test:catalog`: 142 network tests pass against the
live `models.dev`, `openrouter.ai`, and `opencode.ai/zen/v1/models`
endpoints.

wiki/models-catalog.md: rewrote the new "Free-Zen aliases need Zen-side
verification" section to (a) describe the two conditions, (b) note
that a fallback to an isFree alias is the legitimate escape hatch
(mimo's pattern), and (c) point at the three tests by name so the next
maintainer can find the enforcement surface. wiki/model-resolution.md
points at the new section.

* make gpt-5-nano a deprecated free alias falling back to big-pickle

revising the previous "demote to paid" approach. the user-facing
ergonomics are cleaner: anyone who picked gpt-5-nano under the "Free"
badge gets transparent-upgraded to a real free model (big-pickle)
instead of suddenly being asked to set OPENCODE_API_KEY. matches the
existing mimo pattern exactly. the dropdown already filters
`!a.fallback`, so the slug disappears from the picker on its own and
the trigger renders it as "Big Pickle" via `resolveDisplayAlias`.

no other catalog or test surface changes — the isFree invariants and
the main-only zen/cost checks still pass (gpt-5-nano's terminal is
now big-pickle, which is both isFree and zero-cost on models.dev,
deduping with big-pickle's own row in both network tests).

* revise: keep gpt-5-nano as paid alias, backfill affected DB rows instead

dropping the deprecated-alias approach. `opencode/gpt-5-nano` is a
legitimate cheap paid model people may want with BYOK
(`OPENCODE_API_KEY`) — giving it `fallback: "opencode/big-pickle"`
would foreclose that for everyone going forward. correct fix is two
parts:

(a) reclassify in the catalog as a regular paid OpenCode alias:
  - drop `isFree: true` and `envVars: []` so the local validator
    demands `OPENCODE_API_KEY`
  - add `openRouterResolve: "openrouter/openai/gpt-5-nano"` to satisfy
    the completeness test and route BYOK-via-OpenRouter users
  - no `fallback` — slug stays visible in the picker as a paid option

(b) one-shot DB backfill of provably-affected repos
(`scripts/backfill-gpt5-nano-affected.ts`). scope:
  - `Repo.model = "opencode/gpt-5-nano"`
  - AND at least one `WorkflowRun` with `inputTokens IS NULL` (evidence
    of an attempted run that didn't get past the model-init gate)

skipped intentionally:
  - repos whose runs have `inputTokens > 0` — they have a key, gpt-5-
    nano works for them
  - repos with zero WorkflowRun rows — never dispatched; touching them
    would be presumptuous
  - `LearningsRevision.model` — audit trail of which model authored a
    revision, rewriting it would falsify history

ran against .env.prod: 2 repos stored the slug; 1 was provably
affected (sodown4thecause/seobot, 5/5 zero-token runs — matches #691's
3 failed runs from this repo plus 2 outside the 24h audit window).
1 was an internal test account that never dispatched (left as-is).
applied: 1 row updated. confirmed idempotent on re-run.

the other two repos in #691 (Nantiee/ALTA-breast-pump-tool,
keksiqc/ansible-setup-linux) don't store the slug in `Repo.model`;
their failed dispatches passed the model inline in the
`workflow_dispatch` `prompt` payload, so the catalog fix alone (no
longer offering it as free) is what helps them.

tests:
  - models.test.ts: `getModelEnvVars("opencode/gpt-5-nano")` now
    returns `["OPENCODE_API_KEY"]`, moved into the keyed-model group
  - apiKeys.test.ts: added "throws without OPENCODE_API_KEY" case
  - isFree invariants from the previous commit still pass — gpt-5-nano
    no longer triggers them since it's no longer isFree
  - main-only catalog tests still pass (gpt-5-nano served by Zen, just
    paid; no isFree cost check applies)

* docs: drop stale GPT Nano + MiMo V2 Pro from free-tier lists

addressing pullfrog auto-review feedback on #695. three mintlify pages
still advertised both as keyless after the catalog pivot, which now
makes the docs affirmatively wrong rather than merely stale:

- gpt nano is paid in the catalog (no `isFree`, inherits
  `OPENCODE_API_KEY`); a user following the docs would hit the same
  "missing API key" failure that's described 4 lines below in
  `docs/keys.mdx`.
- mimo v2 pro is hidden from the picker (`fallback` triggers
  `ModelSelector`'s `!a.fallback` filter); the alias only exists for
  legacy stored-value resolution. a user reading the docs cannot
  actually pick it.

surviving picker-visible free set: Big Pickle and MiniMax M2.5.

- `docs/keys.mdx`: drop both bullets from the "Free models" list
- `docs/billing.mdx`: drop both bullets from the "Free models" list
- `docs/getting-started.mdx`: collapse the inline mention from a
  4-model list to "Big Pickle and MiniMax M2.5"

* address third review: picker grouping + backfill classifier honesty

i had not pulled the third pullfrog review (`02:17:28Z`) when i declared
reviews triaged after the docs sweep — the fourth review flagged that
three findings remained pending. addressing them now.

1. picker grouping for now-selectable paid gpt-5-nano. when i removed
   `"gpt-5-nano": "OpenAI"` from `modelIdToUpstream` in the previous
   pivot-to-paid commit, i mistook it for dead code. it's not — the map
   IS consulted for paid opencode aliases via `groupByUpstream →
   getUpstreamLabel` inside the OpenCode submenu's
   `renderSubContent`. without the entry, `gpt-5-nano` falls back to
   `getProviderDisplayName("opencode")` = "OpenCode" and gets dropped
   into its own sub-header instead of joining opencode/gpt,
   opencode/gpt-pro, opencode/gpt-mini under the "OpenAI" upstream
   group. re-added with an explanatory comment so the next refactor
   doesn't make the same mistake.

2. JSDoc / code mismatch in `scripts/backfill-gpt5-nano-affected.ts`.
   the JSDoc said "at least one `WorkflowRun` with `inputTokens IS
   NULL`" but the code is `no WorkflowRun has inputTokens > 0` — a
   strictly broader filter (catches `null` AND `0`). rewrote the scope
   block to describe what the code actually does, with the operative
   classifier spelled out: "a billable run with `inputTokens > 0` is
   proof the agent successfully reached and called the model".

3. classifier breadth (raised in the same review). honest answer: the
   "no positive-token run" filter IS a heuristic — a repo whose only
   dispatches happened to fail or cancel for unrelated reasons would
   get false-positive-classified A. for THIS one-shot population (2
   repos, 1 with 5/5 zero-token runs — strong systematic-failure
   signal) the heuristic was good enough and the dry-run inspection
   confirmed before APPLY. for any larger reuse of this pattern, you
   need to cross-reference the runtime error string (`UnknownError:
   Model not found: opencode/gpt-5-nano`) from GitHub Actions logs or
   Better Stack — that error doesn't live on `WorkflowRun` rows. added
   a "Classifier limitations" section to the JSDoc making this
   explicit.

nothing about the actual applied backfill changes — the prod write
(1 repo: sodown4thecause/seobot → opencode/big-pickle) is unchanged
and re-running the script remains idempotent.
2026-05-13 02:43:08 +00:00
Colin McDonnell 159389fad2 fix(mcp): sanitize for gemini when model is unresolved (#697)
* fix(mcp): sanitize for gemini when model is unresolved

isGeminiRouted() previously required the effective model string to
contain "gemini" — but when payload.model="auto" (or any unresolved
slug) reaches addTools(), `effective` is the literal "auto", which
doesn't match. opencode then auto-selects gemini *after* the MCP
server has registered raw arktype schemas, and every tool turn dies
on `function_declarations[*].properties[*].any_of[*].enum: only
allowed for STRING type`.

widen the gate: any unresolved specifier (undefined / "auto" / a
slug without a `provider/` prefix) is treated as gemini-routed and
sanitized. the transforms are universally compatible normalizations
so the false-positive cost is negligible. tighten case 3 to preserve
`description` so the only lossy path no longer drops operator-facing
context.

fixes #676.

* revert case-3 description preservation

per pullfrog review on #697: keeping `description` as a peer of
`anyOf`/`oneOf` directly contradicts the file's own header (lines
19-21) and the upstream opencode #14659 rationale that gates this
sanitizer — gemini requires anyOf to be the ONLY field on a schema
node, sibling keywords trigger
`anyOf must be the only field in a schema node`. the change was
speculative scope creep with no evidence, and would silently
re-introduce a different gemini failure for any future schema using
`.describe().or(...)`. the bug fix for #676 doesn't need it (arktype
doesn't emit non-collapsible anyOf for current tool schemas).
2026-05-13 02:31:59 +00:00
Colin McDonnell 43bb14bf87 action: strip Content-Type on body-less apiFetch requests (#692) (#694)
* action: strip Content-Type on body-less apiFetch requests (#692)

Vercel's Next.js lambda adapter (Next 16.1.x) attempts to decode a
request body when Content-Type is set and throws
`SyntaxError: Unexpected end of data` before delegating to the route
handler, returning a 500. Hit /run-context exclusively because it was
the only body-less GET that sent `Content-Type: application/json`.

- Drop `Content-Type: application/json` from the GET in
  `action/utils/runContext.ts` (meaningless on a body-less request).
- Defensively strip any `content-type` header in `action/utils/apiFetch.ts`
  when no body is present so future callers can't reintroduce this.

* apiFetch: soften comment — empirical observation, RFC 9110 §8.3 framing
2026-05-13 02:03:24 +00:00
Colin McDonnell d8f825034f billing: $10 signup credit + lazy claim modal; disable welcome credit promo (#674)
* billing: $10 signup credit + lazy claim modal; disable welcome credit promo

Adds a per-Account $10 Router signup credit granted on first Router-tab
mount via a new admin-gated POST /api/account/[owner]/signup-credit/claim.
The endpoint is idempotent — the inserted CreditGrant row IS the dedup
state, so subsequent calls return granted:false. Client SignupCreditModal
fires the POST on mount (only when modelAccessMode === "router") and
opens a celebratory dialog when granted:true.

Disables the legacy welcome credit ($10 on first card add) via a new
WELCOME_CREDIT_PROMO_ACTIVE = false flag in utils/stripe.ts. Code path
stays intact — flip the flag to revive. Strips the now-untruthful
"$10 on enabling billing" copy from BillingCard, EnableRouterPrompt,
triggerWorkflow paywall comment, action router_requires_card summary,
email snippet, billing/pricing docs and wiki.

Cuts WELCOME_CREDIT_CENTS from 2000 to 1000 to reflect the lower amount
that would land if the flag is ever re-enabled. Adds "signup" reason
mapping to BillingCard wallet history.

Verified end-to-end against dev: admin+Router fires modal, admin+BYOK
gate-blocks mount, BYOK→Router transition fires modal on click, member
and collaborator paths skip the mount entirely, reload after grant is
idempotent. Wallet history shows "Router signup credit +$10.00".

* billing: address PR review (race fix, copy sweep, modal retry)

Correctness:
- Add @@unique([accountId, reason]) on CreditGrant + migration. The prior
  check-then-insert pattern in /signup-credit/claim and finalizeCheckoutSession
  raced at READ COMMITTED — two concurrent admin tabs could land two grants of
  the same reason on a fresh account ($10 each). Both write sites now rely on
  the unique index for dedup (P2002 = "already granted") and route updated to
  catch P2002 cleanly. Verified zero existing duplicates in prod before
  migration.
- Add log.info on signup grant insert so a successful grant has any chance of
  being caught by ops monitoring.
- Add retry: 2 with backoff to the claim mutation. Endpoint is idempotent so
  a server-side success that lost its response cleanly returns granted:false
  on retry.

Public copy that still advertised the (now-deleted) $20 welcome credit:
- app/page.tsx landing pricing card
- emails/announceBilling.ts broadcast template
- docs/keys.mdx BYOK note
- components/AgentSettings.tsx Router-without-billing warning
- utils/stripe.ts finalizeCheckoutSession JSDoc
- utils/email/snippets.ts ROUTER_CREDIT_PS_HTML JSDoc

Wiki staleness sweep:
- wiki/billing.md TOC, mermaid diagram (signup edge added; welcome marked
  dormant), test coverage list, key modules section, no-card wallet narrative
- wiki/pricing.md welcome-credit drawdown reference
- Rewrote my own internally-inconsistent dormancy paragraph to be honest
  about the $20-historical / $10-on-revival framing.

Trivia:
- ModelAccessCard JSX comment had a literal \\u2192 instead of →.

* billing: address PR review round 2

- Replace try/catch P2002 inside finalizeCheckoutSession's prisma.$transaction
  with createMany skipDuplicates. The previous form is broken on Postgres: a
  unique-violation poisons the surrounding TX, so the catch block returns
  cleanly but the outer commit fails and the account.update (stripeCustomerId)
  silently rolls back too. Currently armed only behind the dormant welcome-
  credit flag, but would have broken billing enablement the moment the flag
  flipped. createMany skipDuplicates yields a single ON CONFLICT DO NOTHING
  statement that returns count: 0 cleanly without aborting the TX.
- Apply the same createMany skipDuplicates pattern to the signup-credit route
  too — drops the exception-as-control-flow Prisma namespace import and is
  more uniform with the welcome path.
- Drop the now-orphaned credit_grants_accountId_idx in the same migration.
  The schema removed @@index([accountId]) when @@unique([accountId, reason])
  was added (covered by the leftmost prefix), but the migration only added
  the unique index, leaving prod drifted.

* billing: fix stale finalizeCheckoutSession JSDoc

The function-level JSDoc still described the abandoned try/catch P2002
mechanism after switching to createMany skipDuplicates. The inline
comment + code now agree on the new ON CONFLICT DO NOTHING shape.

* billing: decouple first-card alert, drop vestigial billing field, fix modal cents; sync copy

* docs+homepage: align Router credit copy with signup claim (no card-on-add carrot)

* homepage: add pricing screenshot and pay-as-you-go promo line

* billing: fix once-per-lifetime misframe on first-card alert

* billing: suppress signup credit for prior welcome-credit recipients

* billing: drop bogus '1000 users' cap; invalidate billing on signup-credit settle
2026-05-12 23:47:52 +00:00
Colin McDonnell f0805b78f5 learnings: surface persist failures as warnings, not debug
`persistLearnings` only emitted `log.info("» learnings updated")` on
success; every failure path (non-2xx, fetch throw, 10s timeout) was
`log.debug`, which is hidden unless `ACTIONS_RUNNER_DEBUG=true`. Survey
of recent runs caught at least one case where the agent definitively
edited the tmpfile but no DB row was written and no warning surfaced.

Promote both failure paths to `log.warning` so dropped agent work is
visible in CI logs. The unchanged-from-seed short-circuit stays at
debug — that's a genuine no-op.
2026-05-11 23:51:46 +00:00
Colin McDonnell e20b4d5515 action: bump to 0.1.4 2026-05-11 23:22:47 +00:00
David Blass 8c6cd2bda2 cancel + restart workflow run when @pullfrog mention is edited (#612)
* cancel + restart workflow run when @pullfrog mention is edited

- add `WorkflowRun.triggeringCommentId` (BigInt?, indexed) so the webhook
  handler can find the run that was fired by a given comment
- thread `triggeringCommentId` through `reserveRun` / `triggerWorkflow`
- factor `dispatchMentionRun` out of `issue_comment_created` so the same
  shape is reused on edit
- replace the `issue_comment_edited` stub: re-evaluates the trigger gate,
  cancels prior runs (`octokit.rest.actions.cancelWorkflowRun` + DB
  status='cancelled'), then re-dispatches with a `previousRunsNote`
  appended to `eventInstructions` so the agent acknowledges the prior
  run/PR/artifacts in its summary
- if the edit removes `@pullfrog`, cancel only (no restart)

Co-authored-by: Cursor <cursoragent@cursor.com>

* thread previousRunsNote via dedicated payload field

user prompt has precedence over eventInstructions, so stuffing the
prior-runs note into eventInstructions made it vanish whenever the
trigger comment contained an @pullfrog mention (which is always for the
edit path). pass it as its own payload field and render it alongside the
user's task so the agent actually sees it.

* delete cancelled run's progress comment on edit-restart

so the issue thread doesn't accumulate "This run was cancelled" stubs
on every edit. only deletes for runs we actively cancel; runs that were
already terminal (e.g. completed before the edit) keep their summary
comment in the thread, and `previousRunsNote` links to it so the new
agent can reference prior work.

post-cleanup is race-safe: the action's `validateStuckProgressComment`
swallows the 404 from the deleted comment and exits cleanly, so the
old run's post step cannot clobber the new run's leaping comment.

Co-authored-by: Cursor <cursoragent@cursor.com>

* also cancel + delete progress comment when triggering comment is deleted

mirrors the edit-removes-@pullfrog path: when an @pullfrog comment that
fired a run is hard-deleted, look up any prior runs by triggeringCommentId,
GH-cancel running ones, and delete their leaping progress comments.

skips trigger-gate re-eval (we're tearing down a run, not firing one) and
performs no restart. reuses the existing cancelRunsForTriggeringComment
helper; the returned previousRunsNote is discarded since no dispatch
follows.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix: move cancellation before trigger gate in issue_comment_edited

cancelRunsForTriggeringComment now runs before the triggerEnabled check,
so edits that remove @pullfrog still cancel in-flight runs even when the
repo mention trigger is currently disabled (e.g. for non-collaborators).

* anneal: scope cancel updates per-row + simplify edit gate

- replace blanket updateMany on (triggeringCommentId, repoId) with per-row, status-guarded updates so a parallel handler's freshly-reserved run cannot be clobbered into cancelled by a racing edit delivery.
- drop wasMention/isMention early-break in issue_comment_edited; always run cancelRunsForTriggeringComment (DB is the canonical "did this comment ever trigger a run" source). closes the missing-changes.body.from edge and lets us tear down a still-running prior run even if the admin disabled the mention trigger mid-flight.
- buildPreviousRunsNote returns undefined (not "") when no link lines materialize.
- doc cleanups + wiki/modes.md addendum noting issue_comment_edited / _deleted now drive cancel + restart.

Co-authored-by: Cursor <cursoragent@cursor.com>

* address review feedback on cancel/restart semantics

- guard workflow_run.completed update against status='cancelled' so a
  successful-but-uncancellable GH Actions job can't resurrect a cancelled
  row (and re-bill it) via the completed webhook.
- bucket only status='completed' runs into `preserved` in
  cancelRunsForTriggeringComment; cancelled/failed prior runs have stubs
  as their progress comment, not summaries worth referencing.
- emit previousRunsNote for the runId-null cancel case so the restarted
  agent always knows when it's superseding a prior dispatch.
- drop the agent-forbidden `gh pr list` hint and soften 'was cancelled'
  to 'was signalled to cancel' in the note body.
- post a fallback comment when the edit-path dispatch fails (prior run
  already torn down and progress comment already deleted).
- symmetrize the delete-handler's pullfrog guard with the edit handler
  (key off hook.comment.user, not hook.sender).
- trim misleading comments on the per-row DB update guard.

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
Co-authored-by: Colin McDonnell <colinmcd94@gmail.com>
2026-05-11 23:20:44 +00:00
28 changed files with 908 additions and 147 deletions
+4
View File
@@ -11,6 +11,10 @@ permissions:
jobs:
test-token:
# only run in the upstream publish target. forks inherit this file but
# haven't installed the pullfrog github app — running it there 404s our
# token endpoint and pollutes our error logs (see #693).
if: github.repository == 'pullfrog/pullfrog'
runs-on: ubuntu-latest
steps:
- name: Get installation token
+4 -2
View File
@@ -10,8 +10,10 @@ permissions:
jobs:
trigger:
# skip if pushed by our bot (breaks the loop)
if: github.actor != 'pullfrog[bot]'
# only run in the upstream publish target (forks inherit this file but
# can't dispatch into pullfrog/app), and skip if pushed by our bot (breaks
# the loop).
if: github.repository == 'pullfrog/pullfrog' && github.actor != 'pullfrog[bot]'
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
+130 -38
View File
@@ -18,17 +18,23 @@ import { performance } from "node:perf_hooks";
import { pullfrogMcpName } from "../external.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";
import { SPAWN_ACTIVITY_TIMEOUT_CODE, SpawnTimeoutError, spawn } from "../utils/subprocess.ts";
import {
DEFAULT_MAX_RETAINED_BYTES,
SPAWN_ACTIVITY_TIMEOUT_CODE,
SpawnTimeoutError,
spawn,
TailBuffer,
} from "../utils/subprocess.ts";
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 { formatWithLabel, ORCHESTRATOR_LABEL, SessionLabeler } from "./sessionLabeler.ts";
import {
type AgentResult,
type AgentRunContext,
@@ -90,10 +96,13 @@ function stripProviderPrefix(specifier: string): string {
return slashIndex > 0 ? specifier.slice(slashIndex + 1) : specifier;
}
// `max` effort is supported on Opus 4.6 / 4.7; other models fall back to `high`.
// claude-code deny-lists older opus/sonnet generations from `max` at invocation time.
function resolveEffort(model: string | undefined): "max" | "high" {
if (model?.includes("opus")) return "max";
// `high` is the model's tuned default ("equivalent to not setting the parameter"
// per Anthropic docs). `max` is "absolute maximum capability with no constraints
// on token spending" — meaningfully slower and burns more thinking budget per
// turn. We default everyone to `high`; PRs that genuinely need full-send can
// opt in via a future per-run override rather than paying the wall-time cost on
// every Opus run.
function resolveEffort(_model: string | undefined): "high" {
return "high";
}
@@ -111,13 +120,21 @@ interface ContentBlock {
[key: string]: unknown;
}
// SDK schema (per claude-agent-sdk docs) puts `session_id` and
// `parent_tool_use_id` at the top level of every Assistant/User/System/Result
// message, not inside `message`. Subagent events carry a non-null
// `parent_tool_use_id` pointing at the orchestrator's Task/Agent tool_use id.
interface ClaudeSystemEvent {
type: "system";
session_id?: string;
parent_tool_use_id?: string | null;
[key: string]: unknown;
}
interface ClaudeAssistantEvent {
type: "assistant";
session_id?: string;
parent_tool_use_id?: string | null;
message?: {
role?: string;
content?: ContentBlock[];
@@ -135,6 +152,8 @@ interface ClaudeAssistantEvent {
interface ClaudeUserEvent {
type: "user";
session_id?: string;
parent_tool_use_id?: string | null;
message?: {
role?: string;
content?: ContentBlock[];
@@ -233,7 +252,37 @@ function tailLines(text: string, maxCodeUnits: number): string {
export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
const startTime = performance.now();
let eventCount = 0;
const thinkingTimer = new ThinkingTimer();
// per-session labeler so parallel subagent log lines can be differentiated.
// claude-agent-sdk runs subagents inside the orchestrator's session — they
// share `session_id` — and stamps every subagent message with a non-null
// `parent_tool_use_id` pointing at the Agent tool_use that spawned them.
// we bind each Agent tool_use id to its dispatched label up front, then
// labelFor short-circuits to the direct mapping when parent_tool_use_id is
// set. orchestrator events (parent_tool_use_id === null) flow through the
// sessionID path and bind to ORCHESTRATOR_LABEL on first sighting.
const labeler = new SessionLabeler();
function eventLabel(event: { session_id?: string; parent_tool_use_id?: string | null }): string {
return labeler.labelFor(event.session_id ?? null, event.parent_tool_use_id ?? null);
}
function withLabel(label: string, message: string): string {
return label === ORCHESTRATOR_LABEL ? message : formatWithLabel(label, message);
}
// one ThinkingTimer per session — sharing a single timer across sessions
// conflated cross-session interleaving as parent thinking time. each timer
// formats its log lines through the session label so attribution is visible.
const thinkingTimers = new Map<string, ThinkingTimer>();
function timerFor(label: string): ThinkingTimer {
let t = thinkingTimers.get(label);
if (!t) {
const formatLine = (line: string) =>
label === ORCHESTRATOR_LABEL ? line : formatWithLabel(label, line);
t = new ThinkingTimer(formatLine);
thinkingTimers.set(label, t);
}
return t;
}
let finalOutput = "";
let sessionId: string | undefined;
@@ -277,18 +326,30 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
}
const handlers = {
system: (_event: ClaudeSystemEvent) => {
log.debug(`» ${params.label} system event`);
system: (event: ClaudeSystemEvent) => {
// claude-agent-sdk only emits system:init for the top-level query, so
// this binds the orchestrator label and never appears in subagent flow.
// we still route through eventLabel so a subagent system event (if the
// SDK ever adds one) wouldn't go silently misattributed.
const label = eventLabel(event);
log.debug(withLabel(label, `» ${params.label} system event`));
},
assistant: (event: ClaudeAssistantEvent) => {
const content = event.message?.content;
if (!content) return;
const label = eventLabel(event);
const boxTitle = label === ORCHESTRATOR_LABEL ? params.label : `${params.label} [${label}]`;
for (const block of content) {
if (block.type === "text" && block.text?.trim()) {
const message = block.text.trim();
log.box(message, { title: params.label });
finalOutput = message;
log.box(message, { title: boxTitle });
// only the orchestrator's text becomes the run's "output" — subagent
// report-back text would otherwise clobber the parent's final answer.
if (label === ORCHESTRATOR_LABEL) {
finalOutput = message;
}
} else if (block.type === "tool_use") {
const toolName = block.name || "unknown";
if (params.onToolUse) {
@@ -297,23 +358,34 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
input: block.input,
});
}
thinkingTimer.markToolCall();
log.toolCall({ toolName, input: block.input || {} });
timerFor(label).markToolCall();
const inputFormatted = formatJsonValue(block.input || {});
const toolCallLine =
inputFormatted !== "{}" ? `» ${toolName}(${inputFormatted})` : `» ${toolName}()`;
log.info(withLabel(label, toolCallLine));
// 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") {
// when the orchestrator dispatches a subagent, bind the Agent
// tool_use id to the dispatched label so future events carrying
// `parent_tool_use_id === block.id` resolve directly to the right
// lens. v2.1.63+ renamed the tool to "Agent"; older versions
// emitted "Task". match both for forward-compat.
if (
(toolName === "Task" || toolName === "Agent") &&
block.input &&
typeof block.input === "object"
) {
const taskInput = block.input as {
description?: string;
subagent_type?: string;
prompt?: string;
};
const label = deriveLabelFromTaskInput(taskInput);
const dispatchedLabel = labeler.recordTaskDispatch(taskInput, block.id ?? null);
log.info(
`» dispatching subagent: ${label}` +
(taskInput.subagent_type ? ` (subagent_type=${taskInput.subagent_type})` : "")
withLabel(
label,
`» dispatching subagent: ${dispatchedLabel}` +
(taskInput.subagent_type ? ` (subagent_type=${taskInput.subagent_type})` : "")
)
);
}
@@ -323,8 +395,14 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
params.todoTracker.cancel();
}
// parse TodoWrite events for live progress tracking
if (toolName === "TodoWrite" && params.todoTracker?.enabled) {
// parse TodoWrite events for live progress tracking. only honor the
// orchestrator's todos — subagents emit their own todo lists which
// would otherwise clobber the visible progress comment.
if (
toolName === "TodoWrite" &&
params.todoTracker?.enabled &&
label === ORCHESTRATOR_LABEL
) {
params.todoTracker.update(block.input);
}
}
@@ -345,10 +423,12 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
const content = event.message?.content;
if (!content) return;
const label = eventLabel(event);
for (const block of content) {
if (typeof block === "string") continue;
if (block.type === "tool_result") {
thinkingTimer.markToolResult();
timerFor(label).markToolResult();
const outputContent =
typeof block.content === "string"
@@ -366,9 +446,9 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
: String(block.content);
if (block.is_error) {
log.info(`» tool error: ${outputContent}`);
log.info(withLabel(label, `» tool error: ${outputContent}`));
} else {
log.debug(`» tool output: ${outputContent}`);
log.debug(withLabel(label, `» tool output: ${outputContent}`));
}
}
}
@@ -462,7 +542,8 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
let lastProviderError: string | null = null;
let output = "";
// capped accumulator — see opencode.ts for rationale (issue #680).
const output = new TailBuffer(DEFAULT_MAX_RETAINED_BYTES);
let stdoutBuffer = "";
try {
@@ -480,9 +561,14 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
// there's no shim-orphan issue like opencode-ai/bin/opencode, but
// detached + killGroup is the right default for any agent runtime.
killGroup: true,
// claude already drains every chunk via onStdout (NDJSON parsing) and
// onStderr (recentStderr ring buffer). retaining a second copy in the
// spawn wrapper would grow unbounded for long sessions and previously
// crashed the wrapper with RangeError. see issue #680.
retain: "none",
onStdout: async (chunk) => {
const text = chunk.toString();
output += text;
output.append(text);
markActivity();
stdoutBuffer += text;
@@ -592,20 +678,26 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
// hides the actionable provider message and pollutes the run log. cap
// the stdout fallback to the last 2KB so it stays readable when neither
// a structured error nor stderr is available.
const truncatedStdout = result.stdout ? tailLines(result.stdout, 2048) : "";
//
// result.stdout / result.stderr are empty because we pass retain:"none"
// to spawn (see issue #680); the agent layer keeps its own bounded
// mirrors via `output` (TailBuffer) and `recentStderr` (ring buffer).
const stdoutSnapshot = output.toString();
const stderrSnapshot = recentStderr.join("\n");
const truncatedStdout = stdoutSnapshot ? tailLines(stdoutSnapshot, 2048) : "";
const errorMessage =
lastResultError ||
result.stderr ||
stderrSnapshot ||
truncatedStdout ||
`unknown error - no output from Claude CLI${errorContext}`;
log.error(
`${params.label} exited with code ${result.exitCode}${errorContext}: ${errorMessage}`
);
log.debug(`stdout: ${result.stdout?.substring(0, 500)}`);
log.debug(`stderr: ${result.stderr?.substring(0, 500)}`);
log.debug(`stdout: ${stdoutSnapshot.substring(0, 500)}`);
log.debug(`stderr: ${stderrSnapshot.substring(0, 500)}`);
return {
success: false,
output: finalOutput || output,
output: finalOutput || stdoutSnapshot,
error: errorMessage,
usage,
sessionId,
@@ -615,7 +707,7 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
if (eventCount === 0 && lastProviderError) {
return {
success: false,
output: finalOutput || output,
output: finalOutput || output.toString(),
error: `provider error: ${lastProviderError}`,
usage,
sessionId,
@@ -625,14 +717,14 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
if (resultErrorSubtype) {
return {
success: false,
output: finalOutput || output,
output: finalOutput || output.toString(),
error: lastResultError || `result subtype: ${resultErrorSubtype}`,
usage,
sessionId,
};
}
return { success: true, output: finalOutput || output, usage, sessionId };
return { success: true, output: finalOutput || output.toString(), usage, sessionId };
} catch (error) {
params.todoTracker?.cancel();
const duration = performance.now() - startTime;
@@ -658,7 +750,7 @@ export async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
return {
success: false,
output: finalOutput || output,
output: finalOutput || output.toString(),
error: `${errorMessage} [${diagnosis}]`,
usage: buildUsage(),
sessionId,
+66 -15
View File
@@ -22,7 +22,13 @@ 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";
import { SPAWN_ACTIVITY_TIMEOUT_CODE, SpawnTimeoutError, spawn } from "../utils/subprocess.ts";
import {
DEFAULT_MAX_RETAINED_BYTES,
SPAWN_ACTIVITY_TIMEOUT_CODE,
SpawnTimeoutError,
spawn,
TailBuffer,
} from "../utils/subprocess.ts";
import { ThinkingTimer } from "../utils/timer.ts";
import type { TodoTracker } from "../utils/todoTracking.ts";
import { getDevDependencyVersion } from "../utils/version.ts";
@@ -59,6 +65,7 @@ type OpenCodeConfig = {
permission?: Record<string, unknown>;
provider?: Record<string, unknown>;
agent?: Record<string, unknown>;
experimental?: Record<string, unknown>;
model?: string;
enabled_providers?: string[];
[key: string]: unknown;
@@ -114,6 +121,15 @@ function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): s
[pullfrogMcpName]: { type: "remote", url: ctx.mcpServerUrl },
},
agent: buildReviewerAgentConfig(),
// opt into opencode's experimental `batch` tool (added in
// anomalyco/opencode PR #2983, opt-in via `experimental.batch_tool`). it
// exposes a single `batch` tool that runs 1-25 independent tool calls
// (read/grep/glob/bash/etc.) concurrently in one assistant turn, which
// collapses the dominant grep→20×read pattern into a single round trip.
// edits are explicitly disallowed inside the batch upstream. paired with
// the "Parallel tool execution" guidance in utils/instructions.ts so the
// model actually reaches for it. see wiki/prompt.md.
experimental: { batch_tool: true },
provider: {
google: {
models: Object.fromEntries(
@@ -377,7 +393,6 @@ type RunParams = {
async function runOpenCode(params: RunParams): Promise<AgentResult> {
const startTime = performance.now();
let eventCount = 0;
const thinkingTimer = new ThinkingTimer();
let finalOutput = "";
let accumulatedTokens = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 };
@@ -409,6 +424,23 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
return label === ORCHESTRATOR_LABEL ? message : formatWithLabel(label, message);
}
// one ThinkingTimer per session — sharing a single timer across sessions
// conflated cross-session interleaving (parent thinks → child tool_call,
// or child returns → parent dispatches next) as parent thinking time. each
// timer formats its log lines through the session label so the "thought
// for X" attribution is visible in the merged stream.
const thinkingTimers = new Map<string, ThinkingTimer>();
function timerFor(label: string): ThinkingTimer {
let t = thinkingTimers.get(label);
if (!t) {
const formatLine = (line: string) =>
label === ORCHESTRATOR_LABEL ? line : formatWithLabel(label, line);
t = new ThinkingTimer(formatLine);
thinkingTimers.set(label, t);
}
return t;
}
// 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
@@ -634,7 +666,7 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
});
}
thinkingTimer.markToolCall();
timerFor(label).markToolCall();
const inputFormatted = formatJsonValue(event.part?.state?.input || {});
const toolCallLine =
inputFormatted !== "{}" ? `» ${toolName}(${inputFormatted})` : `» ${toolName}()`;
@@ -671,7 +703,7 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
const output = event.part?.state?.output || event.output;
const label = eventLabel(event);
thinkingTimer.markToolResult();
timerFor(label).markToolResult();
// surface subagent completion at info level — opencode otherwise hides
// per-task timing in debug-only logs, so a parallel multi-lens fan-out
@@ -872,7 +904,12 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
let lastProviderError: string | null = null;
let agentErrorEvent: OpenCodeErrorEvent | null = null;
let output = "";
// capped accumulator for the agent's narration. used as a post-run fallback
// when `finalOutput` (the orchestrator's final assistant message) is empty.
// unbounded `output += text` previously grew to ~1 GiB on multi-lens Reviews
// and contributed to the wrapper-level RangeError. retain:"none" on spawn
// skips the duplicate buffer there; this TailBuffer caps the agent layer.
const output = new TailBuffer(DEFAULT_MAX_RETAINED_BYTES);
let stdoutBuffer = "";
try {
@@ -891,6 +928,11 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
// never fires — producing zombie runs. detached + killGroup nukes the
// whole tree.
killGroup: true,
// we already drain every chunk via onStdout/onStderr (NDJSON parsing
// + recentStderr ring buffer). retaining a second copy in the spawn
// wrapper would grow unbounded for multi-lens Reviews and previously
// crashed the wrapper with RangeError at ~1 GiB. see issue #680.
retain: "none",
// NB: we used to pass `isPausedExternally: isSubagentInFlight` to suspend
// the activity timer during subagent dispatches. unnecessary now that
// our injected plugin (action/agents/opencodePlugin.ts) re-emits
@@ -900,7 +942,7 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
// (~3.3 plugin events/sec during a typical subagent run).
onStdout: async (chunk) => {
const text = chunk.toString();
output += text;
output.append(text);
markActivity();
stdoutBuffer += text;
@@ -1025,22 +1067,31 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
if (result.exitCode !== 0) {
const errorContext = lastProviderError ? ` (${lastProviderError})` : "";
// result.stdout / result.stderr are empty because we pass retain:"none"
// to spawn (see issue #680); use the agent's bounded mirrors instead.
const stdoutSnapshot = output.toString();
const stderrSnapshot = recentStderr.join("\n");
const errorMessage =
result.stderr ||
result.stdout ||
stderrSnapshot ||
stdoutSnapshot ||
`unknown error - no output from OpenCode CLI${errorContext}`;
log.error(
`${params.label} exited with code ${result.exitCode}${errorContext}: ${errorMessage}`
);
log.debug(`stdout: ${result.stdout?.substring(0, 500)}`);
log.debug(`stderr: ${result.stderr?.substring(0, 500)}`);
return { success: false, output: finalOutput || output, error: errorMessage, usage };
log.debug(`stdout: ${stdoutSnapshot.substring(0, 500)}`);
log.debug(`stderr: ${stderrSnapshot.substring(0, 500)}`);
return {
success: false,
output: finalOutput || stdoutSnapshot,
error: errorMessage,
usage,
};
}
if (eventCount === 0 && lastProviderError) {
return {
success: false,
output: finalOutput || output,
output: finalOutput || output.toString(),
error: `provider error: ${lastProviderError}`,
usage,
};
@@ -1053,13 +1104,13 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
errorEvent.error?.data?.message || errorEvent.error?.name || JSON.stringify(errorEvent);
return {
success: false,
output: finalOutput || output,
output: finalOutput || output.toString(),
error: `${errorName}: ${errorMessage}`,
usage,
};
}
return { success: true, output: finalOutput || output, usage };
return { success: true, output: finalOutput || output.toString(), usage };
} catch (error) {
params.todoTracker?.cancel();
const duration = performance.now() - startTime;
@@ -1085,7 +1136,7 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
return {
success: false,
output: finalOutput || output,
output: finalOutput || output.toString(),
error: `${errorMessage} [${diagnosis}]`,
usage: buildUsage(),
};
+34
View File
@@ -146,6 +146,40 @@ describe("SessionLabeler", () => {
]);
});
test("Claude path: parent_tool_use_id resolves directly without consuming FIFO", () => {
// Claude runs subagents inside the orchestrator's session — they share
// session_id — and stamps subagent messages with parent_tool_use_id.
// recording dispatch with the Agent tool_use id binds it directly so
// future events resolve regardless of session_id.
const labeler = new SessionLabeler();
expect(labeler.labelFor("shared-session", null)).toBe(ORCHESTRATOR_LABEL);
labeler.recordTaskDispatch({ description: "correctness" }, "toolu_01");
labeler.recordTaskDispatch({ description: "security" }, "toolu_02");
// subagent events come through with shared session_id but distinct
// parent_tool_use_id — direct mapping wins
expect(labeler.labelFor("shared-session", "toolu_01")).toBe("lens:correctness");
expect(labeler.labelFor("shared-session", "toolu_02")).toBe("lens:security");
// orchestrator events on the same session still resolve correctly
expect(labeler.labelFor("shared-session", null)).toBe(ORCHESTRATOR_LABEL);
// pendingLabels is unused on the Claude path — FIFO never consumed
expect(labeler.pendingDispatchCount()).toBe(2);
expect(labeler.size()).toBe(1);
});
test("Claude path: unknown parent_tool_use_id falls through to sessionID/FIFO logic", () => {
// defensive: if a subagent event arrives with a parent_tool_use_id we
// never recorded (e.g. orchestrator dispatched off-stream, or a tool we
// didn't track), the labeler shouldn't crash — it should fall through
// to the sessionID-keyed path.
const labeler = new SessionLabeler();
labeler.labelFor("shared", null);
expect(labeler.labelFor("shared", "unknown-tool-id")).toBe(ORCHESTRATOR_LABEL);
});
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
+48 -18
View File
@@ -67,38 +67,68 @@ export function deriveLabelFromTaskInput(input: TaskDispatchInput): string {
}
/**
* Stateful tracker mapping sessionIDs to human labels.
* Stateful tracker mapping subagent activity back to human-readable 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.
* Two attribution channels are supported because the runtimes differ:
*
* - **OpenCode** spawns each subagent as its own opencode `Session` with
* a distinct `sessionID`. The harness records each Task dispatch into a
* pending FIFO queue; the next previously-unseen sessionID consumes the
* head of the queue and binds it to that label.
*
* - **Claude Code** runs subagents inside the orchestrator's session — they
* all share `session_id` — and instead stamps every subagent message with
* `parent_tool_use_id` pointing at the Agent tool_use id that spawned them.
* The harness binds each Agent tool_use id to its dispatched label up
* front, then `labelFor` looks the label up directly when an event arrives
* carrying that `parent_tool_use_id`.
*
* `labelFor(sessionID, parentToolUseId?)` accepts both: when
* `parentToolUseId` is set and known it short-circuits to the direct mapping;
* otherwise it falls through to the FIFO/sessionID path.
*/
export class SessionLabeler {
private readonly labels = new Map<string, string>();
private readonly labelsByToolUseId = new Map<string, string>();
private readonly pendingLabels: string[] = [];
private fallbackCounter = 0;
recordTaskDispatch(input: TaskDispatchInput): string {
/**
* Record a Task/Agent tool dispatch.
*
* @param input Task tool input — used to derive the lens label.
* @param toolUseId Optional Agent tool_use id. When provided, future events
* carrying `parent_tool_use_id === toolUseId` resolve
* directly to this label without consuming the FIFO queue
* (Claude path). Always also pushed to the FIFO queue so
* the OpenCode path still works when toolUseId is absent.
*/
recordTaskDispatch(input: TaskDispatchInput, toolUseId?: string | null): string {
const label = deriveLabelFromTaskInput(input);
this.pendingLabels.push(label);
if (toolUseId) this.labelsByToolUseId.set(toolUseId, 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.
* Return a label for the given event.
*
* @param sessionID Session id from the event (OpenCode: per-session;
* Claude: shared across orchestrator + subagents).
* @param parentToolUseId Claude's `parent_tool_use_id` — non-null on
* subagent messages. When set and known, takes
* priority over the FIFO/sessionID path.
*/
labelFor(sessionID: string | undefined | null): string {
labelFor(sessionID: string | undefined | null, parentToolUseId?: string | null): string {
// Claude path: subagent messages carry parent_tool_use_id pointing at
// the Agent tool_use that spawned them. resolve directly without
// touching the sessionID-keyed map (which is bound to the orchestrator
// for the shared session_id and would otherwise misattribute).
if (parentToolUseId) {
const direct = this.labelsByToolUseId.get(parentToolUseId);
if (direct) return direct;
}
if (!sessionID) return ORCHESTRATOR_LABEL;
const existing = this.labels.get(sessionID);
if (existing) return existing;
+7
View File
@@ -273,6 +273,13 @@ export interface WriteablePayload {
triggerer?: string | undefined;
/** event-level instructions for this trigger type (flag-expanded server-side) */
eventInstructions?: string | undefined;
/**
* system-injected note about prior superseded runs (e.g. when the
* triggering @pullfrog comment is edited). rendered alongside the user's
* prompt rather than via eventInstructions so it survives user-prompt
* precedence.
*/
previousRunsNote?: string | undefined;
/** event data from webhook payload - discriminated union based on trigger field */
event: PayloadEvent;
/** timeout for agent run (e.g., "10m", "1h30m") - defaults to "1h" */
+17 -9
View File
@@ -30,7 +30,7 @@ import { createOctokit, writeGitHubUsageSummaryToFile } from "./utils/github.ts"
import { resolveInstructions } from "./utils/instructions.ts";
import { readLearningsFile, seedLearningsFile } from "./utils/learnings.ts";
import { executeLifecycleHook } from "./utils/lifecycle.ts";
import { normalizeEnv } from "./utils/normalizeEnv.ts";
import { normalizeEnv, sanitizeSecret } from "./utils/normalizeEnv.ts";
import { aggregateUsage, patchWorkflowRunFields } from "./utils/patchWorkflowRunFields.ts";
import { resolvePayload, resolvePromptInput } from "./utils/payload.ts";
import { isRouterKeylimitExhaustedError } from "./utils/providerErrors.ts";
@@ -183,8 +183,9 @@ function billingConsoleUrl(owner: string, anchor: "billing" | "model-access"): s
*
* Branches:
* - `router_requires_card`: user is on Router mode with no card AND no
* wallet balance. Lead with the carrot ($20 free credit), link to
* `#model-access` where the Add Card flow lives.
* wallet balance (signup credit exhausted or not granted). Frame as
* "add a card to continue", link to `#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
@@ -206,7 +207,7 @@ function formatBillingErrorSummary(error: BillingError, owner: string): string {
return [
"**Add a card to start using Pullfrog Router.**",
"",
"Router proxies OpenRouter at raw cost — no platform markup, and your first $20 of usage is on us.",
"Router proxies OpenRouter at raw cost — no platform markup. Add a card and we'll auto-reload your wallet so runs keep flowing.",
"",
`[Add a card →](${billingConsoleUrl(owner, "model-access")})`,
].join("\n");
@@ -471,12 +472,16 @@ async function persistLearnings(ctx: ToolContext): Promise<void> {
});
if (!response.ok) {
const error = await response.text().catch(() => "(no body)");
log.debug(`learnings persist failed (${response.status}): ${error}`);
// promoted from debug → warning: this path means the agent edited the
// file (we already short-circuited the unchanged-from-seed case above)
// but the PATCH dropped it on the floor. silently losing real work is
// worse than the noise of a CI warning.
log.warning(`learnings persist failed (${response.status}): ${error}`);
return;
}
log.info("» learnings updated");
} catch (err) {
log.debug(`learnings persist failed: ${err instanceof Error ? err.message : String(err)}`);
log.warning(`learnings persist failed: ${err instanceof Error ? err.message : String(err)}`);
}
}
@@ -554,12 +559,15 @@ export async function main(): Promise<MainResult> {
const runContext = await resolveRunContextData({ octokit: initialOctokit, token: jobToken });
timer.checkpoint("runContextData");
// inject account-level secrets into process.env (YAML secrets take precedence)
// inject account-level secrets into process.env (YAML secrets take precedence).
// sanitizeSecret trims + masks so accidental trailing whitespace doesn't leak
// through GitHub Actions' line-based log masking. whitespace-only values
// return null and skip injection so the user sees a clear missing-key error.
if (runContext.dbSecrets) {
for (const [key, value] of Object.entries(runContext.dbSecrets)) {
if (!process.env[key]) {
process.env[key] = value;
core.setSecret(value);
const sanitized = sanitizeSecret(key, value);
if (sanitized !== null) process.env[key] = sanitized;
}
}
const count = Object.keys(runContext.dbSecrets).length;
+25 -6
View File
@@ -176,13 +176,32 @@ export function sanitizeToolForGemini<T extends Tool<any, any>>(tool: T): T {
}
/**
* true when the effective upstream model is served by google's generative
* language API — directly (`google/*`), via opencode (`opencode/gemini-*`),
* or via openrouter (`openrouter/google/gemini-*`). slug-substring match
* works because every gemini route's model id contains "gemini".
* true when the effective upstream model is — or might become — google
* generative language API traffic. matches:
* - direct `google/*`, opencode `opencode/gemini-*`, openrouter
* `openrouter/google/gemini-*` (slug substring "gemini" wins).
* - any unresolved specifier: `undefined`, `"auto"`, or a slug that
* didn't map through the alias registry (no `provider/` prefix).
* these flow through the agent's own auto-select, which may land
* on gemini *after* the MCP server has already registered tools —
* at which point sanitization is too late to apply. erring on the
* side of sanitizing is safe: cases 1 + 2 are universally
* compatible JSON-Schema normalizations (enum-only → typed string,
* collapsible const-unions → string enum); case 3 is gemini-
* specific but only fires on non-collapsible unions, which arktype
* does not emit for our current tool schemas. see issue #676 for
* the prod failure that motivated this widening.
*/
export function isGeminiRouted(ctx: ToolContext): boolean {
const effective = ctx.payload.proxyModel ?? ctx.resolvedModel ?? ctx.payload.model;
if (!effective) return false;
return effective.toLowerCase().includes("gemini");
if (!effective) return true;
const normalized = effective.toLowerCase();
if (normalized.includes("gemini")) return true;
// every concrete model resolved through the registry carries a
// `provider/` prefix (e.g. "anthropic/claude-opus-4-7"). anything
// without a slash is either the literal `"auto"` alias or an
// unrecognized slug that resolveModel logged a warning for — both
// route through the agent's late auto-select, which may pick gemini.
if (!normalized.includes("/")) return true;
return false;
}
+1 -1
View File
@@ -55,13 +55,13 @@ describe("getModelEnvVars", () => {
it("returns empty env vars for free opencode models", () => {
expect(getModelEnvVars("opencode/big-pickle")).toEqual([]);
expect(getModelEnvVars("opencode/gpt-5-nano")).toEqual([]);
expect(getModelEnvVars("opencode/mimo-v2-pro-free")).toEqual([]);
expect(getModelEnvVars("opencode/minimax-m2.5-free")).toEqual([]);
});
it("still requires OPENCODE_API_KEY for non-free opencode models", () => {
expect(getModelEnvVars("opencode/claude-opus")).toEqual(["OPENCODE_API_KEY"]);
expect(getModelEnvVars("opencode/gpt-5-nano")).toEqual(["OPENCODE_API_KEY"]);
});
});
+1 -2
View File
@@ -271,8 +271,7 @@ export const providers = {
"gpt-5-nano": {
displayName: "GPT Nano",
resolve: "opencode/gpt-5-nano",
envVars: [],
isFree: true,
openRouterResolve: "openrouter/openai/gpt-5-nano",
},
"mimo-v2-pro-free": {
displayName: "MiMo V2 Pro",
+4 -2
View File
@@ -207,6 +207,8 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`,
- **45 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.
**lens-add discipline.** Each lens needs to clear a specific bar before you dispatch it: name the concrete failure mode this lens would catch *that the diff plausibly introduces*, in one sentence. "Could apply", "good to have", "for completeness" do not qualify. If you can't name what the lens is going to find, drop it. The "when unsure, treat as non-trivial" rule above is for the trivial-vs-non-trivial gate at step 3 — it does not license expanding lens count without articulated risk. Every extra lens adds wall-time, log noise, and pulls subagent attention onto speculative angles, which biases the final review toward bloat-shaped findings.
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.
@@ -214,7 +216,7 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`,
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.
- **research-validated assumptions** — third-party API contracts, SDK semantics, framework directives, version-gated behavior. **only pick when the PR's correctness depends on the contract behaving a specific way** — not when the API is merely used. An idempotency key as a backstop, a timeout as a hint, a retry as belt-and-suspenders: not load-bearing, skip this lens. The bar is "if the third-party contract differs from what the diff assumes, the PR is incorrect." When dispatched, 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
@@ -304,7 +306,7 @@ ${PR_SUMMARY_FORMAT}`,
"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; 23 for typical features; 45 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.
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; 23 for typical features; 45 for high-stakes subsystem touches; 6+ is a smell). same **lens-add discipline** as Review mode applies: each lens needs to name the concrete failure mode it would catch *that the new commits plausibly introduce* — "could apply" doesn't qualify, drop it. **research-validated assumptions** specifically: only pick when the new commits' correctness depends on a third-party contract behaving a specific way; merely using an API doesn't qualify. 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 5 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 6), not in the subagent prompt
+1 -1
View File
@@ -1,6 +1,6 @@
{
"name": "pullfrog",
"version": "0.1.3",
"version": "0.1.6",
"type": "module",
"bin": {
"pullfrog": "dist/cli.mjs",
+3 -2
View File
@@ -50,8 +50,9 @@ const FLAGSHIPS = [
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.
// opencode FREE models (big-pickle, mimo-v2-pro-free, minimax-m2.5-free)
// are unique to opencode and used in prod — keep them. only prune the keyed
// mirrors.
return alias.provider === "opencode" && !alias.isFree;
}
+69 -1
View File
@@ -1,5 +1,5 @@
import { describe, expect, it } from "vitest";
import { modelAliases } from "../models.ts";
import { modelAliases, resolveDisplayAlias } from "../models.ts";
// ── catalog drift tests — main-only ─────────────────────────────────────────────
//
@@ -21,6 +21,7 @@ type ModelsDevModel = {
name: string;
status?: string;
release_date?: string;
cost?: { input?: number; output?: number };
};
type ModelsDevProvider = {
@@ -112,3 +113,70 @@ describe("openRouterResolve OpenRouter API validity", async () => {
});
}
});
// ── OpenCode Zen served-list + free-cost checks ────────────────────────────────
//
// these enforce the two dynamic conditions for "this opencode alias works for a
// user without OPENCODE_API_KEY" — the gap that let issue #691 ship:
// 1. the alias's terminal-fallback resolve appears in Zen's /v1/models (Zen
// actually serves it). caught nothing in #691 because mimo had a fallback
// to big-pickle which IS served, but would catch any future alias that
// points at a Zen-removed model without a fallback.
// 2. for isFree aliases, the terminal-fallback's models.dev `cost.input` is
// zero. caught the gpt-5-nano regression: $0.05/M input on models.dev,
// marked isFree in our catalog.
//
// we check the terminal-fallback (via resolveDisplayAlias) because deprecated
// aliases legitimately point at dead resolve targets — the terminal is what
// actually runs at the agent CLI.
type ZenModel = { id: string };
type ZenModelsResponse = { data: ZenModel[] };
const zenApi = fetch("https://opencode.ai/zen/v1/models").then(
(r) => r.json() as Promise<ZenModelsResponse>
);
describe("opencode Zen served list", async () => {
const zenData = await zenApi;
const zenIds = new Set(zenData.data.map((m) => m.id));
const seen = new Set<string>();
for (const alias of modelAliases) {
const terminal = resolveDisplayAlias(alias.slug);
if (!terminal) continue;
const parsed = parseResolve(terminal.resolve);
if (parsed.provider !== "opencode") continue;
if (seen.has(terminal.resolve)) continue;
seen.add(terminal.resolve);
it(`${alias.slug} terminal resolve ${terminal.resolve} is served by Zen`, () => {
expect(
zenIds.has(parsed.modelId),
`terminal resolve "${terminal.resolve}" for alias "${alias.slug}" is not in https://opencode.ai/zen/v1/models — Zen no longer serves it. either point a fallback at a Zen-served alias or remove the entry.`
).toBe(true);
});
}
});
describe("isFree models.dev cost", async () => {
const data = await api;
const seen = new Set<string>();
for (const alias of modelAliases.filter((a) => a.isFree)) {
const terminal = resolveDisplayAlias(alias.slug);
if (!terminal) continue;
const parsed = parseResolve(terminal.resolve);
if (seen.has(terminal.resolve)) continue;
seen.add(terminal.resolve);
it(`${alias.slug} terminal resolve ${terminal.resolve} has cost.input === 0`, () => {
const model = data[parsed.provider]?.models[parsed.modelId];
expect(model, `terminal resolve "${terminal.resolve}" missing on models.dev`).toBeDefined();
expect(
model?.cost?.input,
`isFree alias "${alias.slug}" walks to "${terminal.resolve}" which reports cost.input=${model?.cost?.input} on models.dev — either repoint the fallback or drop \`isFree\``
).toBe(0);
});
}
});
+47 -1
View File
@@ -1,5 +1,5 @@
import { describe, expect, it } from "vitest";
import { modelAliases, resolveCliModel } from "../models.ts";
import { getModelEnvVars, modelAliases, resolveCliModel, resolveDisplayAlias } from "../models.ts";
// ── pure alias-registry invariants ──────────────────────────────────────────────
//
@@ -42,3 +42,49 @@ describe("fallback chain resolution", () => {
});
}
});
// ── isFree invariants — sanity-check the catalog data shape ─────────────────────
//
// these catch the latent regressions that produced issue #691:
// - opencode/gpt-5-nano was marked `isFree` despite costing $0.05/M
// (no static check existed; demoted to paid in the same PR adding these tests)
// - opencode/mimo-v2-pro-free was free + fallback to big-pickle (correct shape),
// but nothing enforced that the terminal of an isFree fallback chain is itself
// free. if someone repointed big-pickle's fallback at a paid model, all of mimo
// and big-pickle's users would silently start hitting a paid endpoint.
//
// the cost.input check itself is network-dependent (lives in
// models-catalog.main.test.ts); these are the static sibling that runs on every PR.
describe("isFree invariants", () => {
for (const alias of modelAliases.filter((a) => a.isFree)) {
it(`${alias.slug} lives under the opencode provider`, () => {
expect(
alias.provider,
`isFree alias "${alias.slug}" must be under "opencode" (Zen's keyless gate is opencode-only)`
).toBe("opencode");
});
it(`${alias.slug} has empty envVars`, () => {
expect(
getModelEnvVars(alias.slug),
`isFree alias "${alias.slug}" must declare \`envVars: []\` so validateAgentApiKey doesn't demand OPENCODE_API_KEY`
).toEqual([]);
});
it(`${alias.slug} has no openRouterResolve`, () => {
expect(
alias.openRouterResolve,
`isFree alias "${alias.slug}" must omit \`openRouterResolve\` — free Zen models don't exist on OpenRouter`
).toBeUndefined();
});
it(`${alias.slug} fallback chain terminates at an isFree alias`, () => {
const terminal = resolveDisplayAlias(alias.slug);
expect(terminal, `fallback chain for "${alias.slug}" is broken`).toBeDefined();
expect(
terminal?.isFree,
`isFree alias "${alias.slug}" walks to "${terminal?.slug}" which is NOT isFree — users would silently start paying`
).toBe(true);
});
}
});
+12
View File
@@ -37,6 +37,18 @@ export async function apiFetch(options: ApiFetchOptions): Promise<Response> {
headers["x-vercel-protection-bypass"] = bypassSecret;
}
// never send Content-Type on body-less requests. empirically, Vercel's
// Next.js lambda adapter (Next 16.1.x) throws `SyntaxError: Unexpected
// end of data` before delegating to the route handler — returning a 500 —
// when Content-Type is set but no body is present. exact mechanism is
// unverified (minified runtime frame), but Content-Type on a body-less
// request has no defined semantics per RFC 9110 §8.3 anyway. see #692.
if (!options.body) {
for (const key of Object.keys(headers)) {
if (key.toLowerCase() === "content-type") delete headers[key];
}
}
log.debug(`api fetch: ${options.method ?? "GET"} ${url.pathname}`);
const init: RequestInit = {
+7 -5
View File
@@ -27,11 +27,7 @@ describe("validateAgentApiKey", () => {
});
it("passes for other free opencode models", () => {
for (const slug of [
"opencode/gpt-5-nano",
"opencode/mimo-v2-pro-free",
"opencode/minimax-m2.5-free",
]) {
for (const slug of ["opencode/mimo-v2-pro-free", "opencode/minimax-m2.5-free"]) {
expect(() => validateAgentApiKey({ ...base, model: slug })).not.toThrow();
}
});
@@ -59,6 +55,12 @@ describe("validateAgentApiKey", () => {
"no API key found"
);
});
it("throws for opencode/gpt-5-nano without OPENCODE_API_KEY (paid Zen alias)", () => {
expect(() => validateAgentApiKey({ ...base, model: "opencode/gpt-5-nano" })).toThrow(
"no API key found"
);
});
});
describe("no model (auto-select)", () => {
+46 -8
View File
@@ -99,6 +99,22 @@ type AcquireTokenOptions = {
permissions?: GitHubAppPermissions;
};
/**
* Thrown when our token-exchange endpoint returns a non-2xx response.
* The retry policy in `acquireNewToken` looks for this concrete type to
* skip retries — 4xx is terminal user state (not-installed, not-authorized)
* and 5xx is rare enough that re-running the workflow is the right escape
* hatch. Genuine network failures throw plain `Error` and stay retryable.
*/
class TokenExchangeError extends Error {
readonly status: number;
constructor(status: number, message: string) {
super(message);
this.name = "TokenExchangeError";
this.status = status;
}
}
async function acquireTokenViaOIDC(opts?: AcquireTokenOptions): Promise<string> {
const oidcToken = await core.getIDToken("pullfrog-api");
@@ -128,7 +144,22 @@ async function acquireTokenViaOIDC(opts?: AcquireTokenOptions): Promise<string>
clearTimeout(timeoutId);
if (!tokenResponse.ok) {
throw new Error(`Token exchange failed: ${tokenResponse.status} ${tokenResponse.statusText}`);
// prefer the server-side `error` field — it's the single source of
// truth for the install URL (uses GITHUB_APP_INSTALL_URL, which
// varies per env / GITHUB_APP_SLUG). fall back to a generic message
// if the body isn't JSON or doesn't carry an `error` field.
let serverMessage: string | undefined;
try {
const body = (await tokenResponse.json()) as { error?: unknown };
if (typeof body.error === "string") serverMessage = body.error;
} catch {
// body wasn't JSON — fall through to the generic message
}
throw new TokenExchangeError(
tokenResponse.status,
serverMessage ??
`Token exchange failed: ${tokenResponse.status} ${tokenResponse.statusText}`
);
}
const tokenData = (await tokenResponse.json()) as InstallationToken;
@@ -333,13 +364,20 @@ export async function acquireNewToken(opts?: AcquireTokenOptions): Promise<strin
if (isOIDCAvailable()) {
return await retry(() => acquireTokenViaOIDC(opts), {
label: "token exchange",
shouldRetry: (error) =>
error instanceof Error &&
(error.name === "AbortError" ||
error.message.includes("fetch failed") ||
error.message.includes("ECONNRESET") ||
error.message.includes("ETIMEDOUT") ||
error.message.includes("Token exchange failed")),
shouldRetry: (error) => {
// 4xx is terminal user state (app not installed, permissions wrong) —
// retrying just triples our log noise and the user's CI bill (see
// #693). 5xx/429 are transient (vercel cold start, github outage,
// rate limit) and should ride the existing backoff.
if (error instanceof TokenExchangeError) return error.status >= 500 || error.status === 429;
return (
error instanceof Error &&
(error.message.includes("timed out") ||
error.message.includes("fetch failed") ||
error.message.includes("ECONNRESET") ||
error.message.includes("ETIMEDOUT"))
);
},
});
} else {
// local development via GitHub App
+26 -4
View File
@@ -32,6 +32,7 @@ function buildRuntimeContext(ctx: InstructionsContext): string {
"~pullfrog": _,
prompt: _p,
eventInstructions: _ei,
previousRunsNote: _prn,
event: _e,
...payloadRest
} = ctx.payload;
@@ -146,17 +147,23 @@ In case of conflict between instructions, follow this precedence (highest to low
// section builders
// ---------------------------------------------------------------------------
// the user's task: blockquoted user prompt, or event-level instructions for auto-triggers
// the user's task: blockquoted user prompt, or event-level instructions for auto-triggers.
// `previousRunsNote` is system-injected context (e.g. prior runs superseded by a
// comment edit); it's appended regardless of which branch wins so it survives
// user-prompt precedence over eventInstructions.
function buildTaskSection(ctx: PromptContext): string {
const previousRunsNote = ctx.payload.previousRunsNote?.trim() ?? "";
if (ctx.userQuoted) {
const parts = [ctx.userQuoted, previousRunsNote].filter(Boolean);
return `************* YOUR TASK *************
${ctx.userQuoted}`;
${parts.join("\n\n")}`;
}
const eventInstructions = ctx.payload.eventInstructions ?? "";
if (eventInstructions) {
const parts = [ctx.eventTitle, eventInstructions].filter(Boolean);
if (eventInstructions || previousRunsNote) {
const parts = [ctx.eventTitle, eventInstructions, previousRunsNote].filter(Boolean);
return `************* YOUR TASK *************
${parts.join("\n\n")}`;
@@ -276,6 +283,21 @@ ${getStandaloneModeInstructions(ctx.payload.event.trigger, t, ctx.outputSchema)}
Trust the tools — do not repeatedly verify file contents or git status after operations. If a tool reports success, proceed to the next step. Only verify if you encounter an actual error. Exception: right before \`${t("push_branch")}\`, ensure the working tree is clean — that tool rejects dirty trees, and tests you ran earlier often leave untracked output.
### Parallel tool execution
For maximum efficiency, whenever you need to perform multiple independent operations, invoke all relevant tools simultaneously in a single assistant turn rather than sequentially. The dominant failure mode is grep → read → read → read → read across separate turns when one round trip would do. Always parallelize when calls are independent:
- reading multiple files (especially after a grep returns candidates)
- multiple greps with different patterns
- glob + grep + read combos
- listing multiple directories
- inspecting multiple MCP tools or resources
Do NOT parallelize operations that depend on prior output (e.g. create a file then read it), or ordered stateful mutations. Edits are not parallelizable — sequence those normally.${
ctx.agentId === "opencode"
? `\n\nOn OpenCode you also have a \`batch\` tool that bundles 1-25 independent calls into one wrapper call. Reach for it whenever you have >=2 independent calls. Native parallel tool_use and \`batch\` both achieve one round trip instead of N — use whichever your provider supports best.`
: `\n\nEmit multiple \`tool_use\` blocks in the same assistant message for independent calls — the runtime executes them concurrently. Do not wait for one tool result before issuing the next independent call.`
}
### Command execution
Never use \`sleep\` to wait for commands to complete. Commands run synchronously — when the shell tool returns, the command has finished.
+91
View File
@@ -0,0 +1,91 @@
import { afterEach, beforeEach, describe, expect, it } from "vitest";
import { normalizeEnv, sanitizeSecret } from "./normalizeEnv.ts";
/**
* These tests pin the load-bearing invariants of secret sanitisation:
* - sensitive values are trimmed before downstream code reads them
* - whitespace-only values are NOT silently zeroed (leave env unchanged)
* - case normalisation still happens
*
* Masking (`core.setSecret`) is delegated to `@actions/core` and trusted to
* work as documented — we don't spy on stdout to re-test the toolkit.
*/
describe("normalizeEnv: process.env state contract", () => {
let originalEnv: NodeJS.ProcessEnv;
beforeEach(() => {
// normalizeEnv() iterates the entire process.env, so the test must
// control it. snapshot + full wipe + restore is the cleanest isolation.
originalEnv = { ...process.env };
for (const k of Object.keys(process.env)) delete process.env[k];
});
afterEach(() => {
for (const k of Object.keys(process.env)) delete process.env[k];
Object.assign(process.env, originalEnv);
});
it("trims trailing newline from sensitive env vars", () => {
process.env.ANTHROPIC_API_KEY = "sk-ant-secret-value\n";
normalizeEnv();
expect(process.env.ANTHROPIC_API_KEY).toBe("sk-ant-secret-value");
});
it("trims surrounding whitespace including \\r\\n and spaces", () => {
process.env.OPENAI_API_KEY = " sk-openai-value\r\n ";
normalizeEnv();
expect(process.env.OPENAI_API_KEY).toBe("sk-openai-value");
});
it("leaves clean sensitive values untouched", () => {
process.env.ANTHROPIC_API_KEY = "sk-ant-clean";
normalizeEnv();
expect(process.env.ANTHROPIC_API_KEY).toBe("sk-ant-clean");
});
it("ignores non-sensitive env vars", () => {
process.env.NODE_ENV = "production\n";
normalizeEnv();
expect(process.env.NODE_ENV).toBe("production\n");
});
it("canonicalises case and trims the value", () => {
process.env.anthropic_api_key = "sk-ant-lowercase\n";
normalizeEnv();
expect(process.env.ANTHROPIC_API_KEY).toBe("sk-ant-lowercase");
expect(process.env.anthropic_api_key).toBeUndefined();
});
it("preserves whitespace-only values rather than silently zeroing them", () => {
// contract: don't mutate when value is whitespace-only. caller sees the
// misconfigured value verbatim and either fails clearly downstream or
// logs a missing-key error.
process.env.ANTHROPIC_API_KEY = " \n ";
normalizeEnv();
expect(process.env.ANTHROPIC_API_KEY).toBe(" \n ");
});
it("preserves embedded newlines (toolkit masks each line)", () => {
// multi-line PEMs aren't used in practice, but if one slipped in via a
// DB secret we don't want to silently mutate it. trim() only touches
// the ends; @actions/core handles per-line masking via the runner.
process.env.ANTHROPIC_API_KEY = "line1\nline2";
normalizeEnv();
expect(process.env.ANTHROPIC_API_KEY).toBe("line1\nline2");
});
});
describe("sanitizeSecret return value", () => {
it("returns the trimmed value for a sensitive secret with trailing newline", () => {
expect(sanitizeSecret("ANTHROPIC_API_KEY", "sk-ant-secret\n")).toBe("sk-ant-secret");
});
it("returns the value unchanged when no trimming is needed", () => {
expect(sanitizeSecret("ANTHROPIC_API_KEY", "sk-ant-clean")).toBe("sk-ant-clean");
});
it("returns null for whitespace-only input so caller can skip injection", () => {
expect(sanitizeSecret("ANTHROPIC_API_KEY", " \n")).toBeNull();
});
});
+45 -13
View File
@@ -1,11 +1,40 @@
import * as core from "@actions/core";
import { log } from "./cli.ts";
import { isSensitiveEnvName } from "./secrets.ts";
function maskValue(value: string | undefined) {
if (value && typeof value === "string" && value.trim().length > 0) {
// ::add-mask::value tells GitHub Actions to mask this value in logs
console.log(`::add-mask::${value}`);
/**
* Trim surrounding whitespace from a sensitive value and register it as a
* GitHub Actions log mask. Trailing newlines from terminal-copy paste are a
* common footgun: the value travels through GH Actions logs and any tool
* that re-emits parts of it leaks the unmasked tail. Trimming canonicalises
* the value so the mask matches exactly what downstream tools will print.
*
* Masking is delegated to `core.setSecret` (not raw `console.log`) so the
* toolkit percent-encodes `\r`/`\n`; the runner V2 parser decodes them and
* registers the full value plus every non-empty line as separate masks. That
* keeps us safe for embedded-newline values (PEMs, kubeconfigs, JSON blobs)
* even though they aren't currently used.
*
* Returns the trimmed value, or `null` when the input was whitespace-only —
* callers must leave `process.env` untouched in that case so a misconfigured
* value surfaces as a clear "missing key" downstream rather than silently
* mutating to the empty string.
*/
export function sanitizeSecret(key: string, value: string): string | null {
const trimmed = value.trim();
if (trimmed.length === 0) {
log.warning(
`» ${key} is whitespace-only — leaving env var unchanged. check your secret value.`
);
return null;
}
if (trimmed !== value) {
log.warning(
`» stripped whitespace from ${key} (whitespace in secret values breaks GitHub Actions log masking)`
);
}
core.setSecret(trimmed);
return trimmed;
}
/**
@@ -15,7 +44,8 @@ function maskValue(value: string | undefined) {
* If there are conflicts (same key with different capitalizations but different values),
* logs a warning and keeps the uppercase version.
*
* Also registers sensitive values as masks in GitHub Actions.
* Also trims and masks sensitive values so accidental trailing whitespace
* doesn't defeat GitHub Actions log masking.
*/
export function normalizeEnv(): void {
const upperKeys = new Map<string, string[]>();
@@ -30,14 +60,6 @@ export function normalizeEnv(): void {
// process each group
for (const [upperKey, keys] of upperKeys) {
// if sensitive, ensure we mask the value (regardless of whether we rename it or not)
if (isSensitiveEnvName(upperKey)) {
// mask all values associated with this key group
for (const key of keys) {
maskValue(process.env[key]);
}
}
if (keys.length === 1) {
const key = keys[0];
if (key !== upperKey) {
@@ -71,4 +93,14 @@ export function normalizeEnv(): void {
// set the uppercase version
process.env[upperKey] = preferredValue;
}
// trim + mask sensitive values after case normalisation so each key is
// visited exactly once with its final, canonical value
for (const key of Object.keys(process.env)) {
if (!isSensitiveEnvName(key)) continue;
const value = process.env[key];
if (typeof value !== "string" || value.length === 0) continue;
const sanitized = sanitizeSecret(key, value);
if (sanitized !== null) process.env[key] = sanitized;
}
}
+2
View File
@@ -21,6 +21,7 @@ export const JsonPayload = type({
"triggerer?": "string | undefined",
"eventInstructions?": "string",
"previousRunsNote?": "string",
"event?": "object",
"timeout?": "string | undefined",
"progressComment?": type({
@@ -157,6 +158,7 @@ export function resolvePayload(
// it's not a common use case but GITHUB_ACTOR can be a user when the workflow is manually triggered by a user through GitHub Actions UI
(!isPullfrog(process.env.GITHUB_ACTOR) ? process.env.GITHUB_ACTOR : undefined),
eventInstructions: jsonPayload?.eventInstructions,
previousRunsNote: jsonPayload?.previousRunsNote,
event,
timeout: inputs.timeout ?? jsonPayload?.timeout,
cwd: resolveCwd(inputs.cwd),
-1
View File
@@ -88,7 +88,6 @@ export async function fetchRunContext(params: {
try {
const headers: Record<string, string> = {
Authorization: `Bearer ${params.token}`,
"Content-Type": "application/json",
};
if (params.oidcToken) {
headers["X-GitHub-OIDC-Token"] = params.oidcToken;
+71 -1
View File
@@ -1,6 +1,6 @@
import { performance } from "node:perf_hooks";
import { describe, expect, it } from "vitest";
import { spawn } from "./subprocess.ts";
import { spawn, TailBuffer } from "./subprocess.ts";
describe("spawn error path", () => {
it("surfaces ENOENT-style spawn failures in stderr so callers can diagnose", async () => {
@@ -79,6 +79,76 @@ describe("spawn error path", () => {
expect(elapsed).toBeLessThan(10_000);
}, 20_000);
it('retain:"tail" caps stderr at maxRetainedBytes and prepends a truncation sentinel', async () => {
// regression for issue #680: unbounded `stderrBuffer += chunk` previously
// crashed the wrapper with `RangeError: Invalid string length` once V8's
// ~1 GiB kMaxLength was breached on long-lived agent runs. the fix caps
// retention with a TailBuffer; this test exercises the cap end-to-end by
// emitting ~2 MiB of stderr against a 256 KiB ceiling and asserts the
// wrapper does not crash, the result is bounded, and the sentinel is
// present so downstream consumers can detect the truncation.
const result = await spawn({
cmd: "bash",
// print ~2 MiB to stderr in 64 KiB chunks. `yes` + head gives us a
// reliable byte budget that's well above the 256 KiB cap below.
args: ["-c", "yes ABCDEFGH | head -c 2097152 1>&2"],
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
activityTimeout: 0,
maxRetainedBytes: 256 * 1024,
});
expect(result.exitCode).toBe(0);
expect(result.stderr).toMatch(/truncated by retain:tail cap/);
expect(result.stderr.length).toBeLessThan(256 * 1024 + 200);
}, 15_000);
it('retain:"none" returns empty stdout/stderr regardless of child output', async () => {
// long-lived agent callers (opencode, claude) drain via onStdout/onStderr
// and never read result.stdout/result.stderr — they pass retain:"none"
// to skip the per-chunk concatenation entirely. assert that contract:
// empty strings out, but onStdout still fires.
const chunks: string[] = [];
const result = await spawn({
cmd: "bash",
args: ["-c", "echo hello; echo world 1>&2"],
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
activityTimeout: 0,
retain: "none",
onStdout: (chunk) => chunks.push(chunk),
});
expect(result.exitCode).toBe(0);
expect(result.stdout).toBe("");
expect(result.stderr).toBe("");
expect(chunks.join("")).toContain("hello");
});
it('retain defaults to "tail" so short-lived callers keep failure-surfacing snapshots', async () => {
// lock the default explicitly. gitAuth, package installs, and lifecycle
// hooks all rely on `result.stderr` being non-empty on failure — flipping
// the default to "none" would silently break their error messages while
// all other tests in this file kept passing.
const result = await spawn({
cmd: "bash",
args: ["-c", "echo -n diagnostic-output 1>&2; exit 7"],
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
activityTimeout: 0,
});
expect(result.exitCode).toBe(7);
expect(result.stderr).toBe("diagnostic-output");
});
it("TailBuffer drops oldest bytes once the cap is exceeded", () => {
const buf = new TailBuffer(10);
buf.append("0123456789");
expect(buf.toString()).toBe("0123456789");
buf.append("abcde");
// 0-9 plus abcde = 15 chars; cap is 10, so we keep the last 10 = "56789abcde"
expect(buf.toString()).toMatch(/truncated by retain:tail cap/);
expect(buf.toString()).toContain("56789abcde");
});
it("reports signal-killed subprocesses as failures, not success", async () => {
// regression: before the fix, `child.on("close", (exitCode) => ...)`
// discarded the signal parameter and `exitCode || 0` coerced the
+111 -14
View File
@@ -88,6 +88,29 @@ function installSignalHandler(): void {
});
}
/**
* Controls what the wrapper retains in memory across the child's lifetime
* for the post-hoc `SpawnResult.stdout` / `SpawnResult.stderr` snapshots.
*
* Streaming callbacks (`onStdout` / `onStderr`) fire regardless — `retain`
* only governs the buffered snapshot returned in `SpawnResult`.
*
* - `"tail"` (default): keep the last `maxRetainedBytes` UTF-16 code units
* of each stream. Once the cap is exceeded, oldest bytes are sliced off
* and the result is prefixed with a `... [N MiB truncated] ...` sentinel.
* Right default for short-lived commands whose failure mode is in their
* final output (git errors, install failures, hook scripts).
* - `"none"`: skip the buffer entirely. `SpawnResult.stdout` / `.stderr`
* are empty strings. Use this for long-lived streaming agents that already
* drain via `onStdout` / `onStderr` and never read the buffered snapshot.
*
* Default cap is 8 MiB — well below V8's ~1 GiB `kMaxLength` so `+= chunk`
* can never throw `RangeError: Invalid string length`.
*/
export type RetainMode = "tail" | "none";
export const DEFAULT_MAX_RETAINED_BYTES = 8 * 1024 * 1024;
export interface SpawnOptions {
cmd: string;
args: string[];
@@ -115,6 +138,46 @@ export interface SpawnOptions {
// open, keeps emitting NDJSON, and `child.on("close")` never fires —
// producing zombie runs that hang until the GitHub Actions job timeout).
killGroup?: boolean;
retain?: RetainMode;
maxRetainedBytes?: number;
}
/**
* Bounded string accumulator that keeps the tail of appended chunks.
* Once the cap is exceeded, oldest bytes are sliced off and `toString()`
* prefixes the survivors with a sentinel describing the elided byte count.
*
* Exported because long-lived agent runtimes (opencode, claude) also
* accumulate per-run narration strings independently of the spawn wrapper
* and need the same protection against V8's `kMaxLength`.
*/
export class TailBuffer {
// explicit field declarations rather than constructor parameter properties:
// node's strip-only TS loader (used by action/test/run.ts in CI) rejects
// `constructor(private readonly cap: number)` with ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX.
private readonly cap: number;
private buffer = "";
private truncatedBytes = 0;
constructor(cap: number) {
this.cap = cap;
}
append(chunk: string): void {
if (this.cap <= 0) return;
this.buffer += chunk;
if (this.buffer.length > this.cap) {
const drop = this.buffer.length - this.cap;
this.truncatedBytes += drop;
this.buffer = this.buffer.slice(drop);
}
}
toString(): string {
if (this.truncatedBytes === 0) return this.buffer;
const mib = (this.truncatedBytes / 1024 / 1024).toFixed(1);
return `... [${mib} MiB truncated by retain:tail cap] ...\n${this.buffer}`;
}
}
export interface SpawnResult {
@@ -133,8 +196,15 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
installSignalHandler();
const startTime = performance.now();
let stdoutBuffer = "";
let stderrBuffer = "";
// capped accumulators — unbounded `+= chunk` previously crashed the wrapper
// with `RangeError: Invalid string length` once V8's ~1 GiB kMaxLength was
// breached on long-lived agent subprocesses (e.g. multi-lens opencode
// Reviews on large monorepos). retain:"none" skips the buffer entirely
// for callers that already drain via onStdout/onStderr.
const retain: RetainMode = options.retain ?? "tail";
const cap = options.maxRetainedBytes ?? DEFAULT_MAX_RETAINED_BYTES;
const stdoutBuffer = retain === "none" ? null : new TailBuffer(cap);
const stderrBuffer = retain === "none" ? null : new TailBuffer(cap);
const killGroup = options.killGroup ?? false;
@@ -234,20 +304,46 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
lastActivityTime = performance.now();
}
// wrap handlers in try/catch as defense in depth for synchronous throws
// inside the listener body. the historical `+= chunk` RangeError was such
// a throw — synchronous and fatal under node's default uncaught-exception
// policy. with the TailBuffer cap in place the wrapper-side `append` can
// no longer throw, but the catch keeps protecting against any future
// synchronous regression in this path.
//
// note: this does NOT catch rejections from async user callbacks —
// `options.onStdout?.(chunk)` returns a Promise in the agent callers
// (claude.ts, opencode.ts) and a throw inside an async callback surfaces
// as an unhandled Promise rejection, not a synchronous exception. agent
// callers handle their own NDJSON-parse failures internally; the
// synchronous protection here is what matters for the RangeError class
// of bugs (issue #680).
if (child.stdout) {
child.stdout.on("data", (data: Buffer) => {
updateActivity();
const chunk = data.toString();
stdoutBuffer += chunk;
options.onStdout?.(chunk);
try {
updateActivity();
const chunk = data.toString();
stdoutBuffer?.append(chunk);
options.onStdout?.(chunk);
} catch (err) {
log.debug(
`spawn stdout handler threw: ${err instanceof Error ? err.message : String(err)}`
);
}
});
}
if (child.stderr) {
child.stderr.on("data", (data: Buffer) => {
const chunk = data.toString();
stderrBuffer += chunk;
options.onStderr?.(chunk);
try {
const chunk = data.toString();
stderrBuffer?.append(chunk);
options.onStderr?.(chunk);
} catch (err) {
log.debug(
`spawn stderr handler threw: ${err instanceof Error ? err.message : String(err)}`
);
}
});
}
@@ -289,7 +385,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
// appeared to succeed when they'd actually been killed — caller
// checked `result.exitCode !== 0` and moved on.
let resolvedExitCode = exitCode ?? 0;
let resolvedStderr = stderrBuffer;
let resolvedStderr = stderrBuffer?.toString() ?? "";
if (exitCode === null && signal) {
const killMsg = `[spawn] ${options.cmd}: killed by signal ${signal}`;
resolvedStderr = resolvedStderr ? `${resolvedStderr}\n${killMsg}` : killMsg;
@@ -297,7 +393,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
}
resolve({
stdout: stdoutBuffer,
stdout: stdoutBuffer?.toString() ?? "",
stderr: resolvedStderr,
exitCode: resolvedExitCode,
durationMs,
@@ -319,11 +415,12 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
// per the guidance, and hit the same wall every run.
const errMsg = `[spawn] ${options.cmd}: ${error.message}`;
console.error(errMsg);
stderrBuffer = stderrBuffer ? `${stderrBuffer}\n${errMsg}` : errMsg;
const existingStderr = stderrBuffer?.toString() ?? "";
const finalStderr = existingStderr ? `${existingStderr}\n${errMsg}` : errMsg;
resolve({
stdout: stdoutBuffer,
stderr: stderrBuffer,
stdout: stdoutBuffer?.toString() ?? "",
stderr: finalStderr,
exitCode: 1,
durationMs,
});
+13
View File
@@ -203,5 +203,18 @@ describe("ThinkingTimer", () => {
expect(cli.log.info).toHaveBeenNthCalledWith(1, "» thought for 4 seconds");
expect(cli.log.info).toHaveBeenNthCalledWith(2, "» thought for 5 seconds");
});
it("routes log lines through the optional formatLine for per-session prefixing", () => {
const startTime = 1000000;
vi.mocked(performance.now)
.mockReturnValueOnce(startTime) // markToolResult
.mockReturnValueOnce(startTime + 4000); // markToolCall
const timer = new ThinkingTimer((line) => `[lens:security] ${line}`);
timer.markToolResult();
timer.markToolCall();
expect(cli.log.info).toHaveBeenCalledWith("[lens:security] » thought for 4 seconds");
});
});
});
+23 -3
View File
@@ -22,6 +22,15 @@ export class Timer {
const THINKING_THRESHOLD = 3000; // ms
/**
* Measures wall-clock gap between the last tool_result and the next tool_call,
* surfacing it as a "thought for Xs" log when over `THINKING_THRESHOLD`.
*
* Use one instance per logical session (orchestrator, each subagent) — sharing
* a single timer across sessions conflates cross-session interleaving as
* thinking time. The optional `formatLine` lets the caller prefix output with
* a session label so attribution is visible in the merged log stream.
*/
export class ThinkingTimer {
private readonly durationFormatter = new Intl.NumberFormat("en-US", {
style: "unit",
@@ -32,21 +41,32 @@ export class ThinkingTimer {
});
private lastToolResultTimestamp: number | null = null;
private readonly formatLine: (line: string) => string;
// node's native TS strip-only mode does not support parameter properties,
// so the formatter is declared as a field and assigned in the body.
constructor(formatLine: (line: string) => string = (l) => l) {
this.formatLine = formatLine;
}
markToolResult(): void {
this.lastToolResultTimestamp = performance.now();
log.debug(`» thinking timer: markToolResult at ${this.lastToolResultTimestamp}`);
log.debug(
this.formatLine(`» thinking timer: markToolResult at ${this.lastToolResultTimestamp}`)
);
}
markToolCall(): void {
const now = performance.now();
log.debug(
`» thinking timer: markToolCall at ${now}, lastToolResult=${this.lastToolResultTimestamp}`
this.formatLine(
`» thinking timer: markToolCall at ${now}, lastToolResult=${this.lastToolResultTimestamp}`
)
);
if (this.lastToolResultTimestamp === null) return;
const elapsed = now - this.lastToolResultTimestamp;
if (elapsed < THINKING_THRESHOLD) return;
const seconds = elapsed / 1000;
log.info(`» thought for ${this.durationFormatter.format(seconds)}`);
log.info(this.formatLine(`» thought for ${this.durationFormatter.format(seconds)}`));
}
}