efc1b67e7bad682e39f87ba10bbe1d9b33e6ea76
944 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Colin McDonnell
|
efc1b67e7b |
fix(test): skip models.dev existence check for fallback aliases
deprecated aliases (`fallback` set) legitimately point at dead resolve targets — xAI just retired grok-4-1-fast/grok-code-fast-1 and #761 wired them through the fallback chain. the terminal-fallback is validated separately by the Zen served-list test. |
||
|
Colin McDonnell
|
0a64659ee7 |
refactor: slim action/main.ts to an orchestrator + extract helpers (#755)
* refactor: extract helpers out of action/main.ts so non-orchestration churn stops touching the file main.ts had grown to ~1240 lines holding ~500 lines of helpers that have nothing to do with the resolver pipeline — billing-error UI/copy, proxy minting, summary/learnings persistence, log formatters, end-of-run cleanup waterfalls. any PR adding a new billing code branch or a new log line was forced to edit main.ts, and since main.ts is in ALWAYS_RUN_ALL the entire 52-job LLM CI matrix fired on what should have been a 0-job change (e.g. #748). extractions: - action/utils/billingErrors.ts — BillingError, TransientError, the format*Summary renderers, billingConsoleUrl - action/utils/proxy.ts — mintProxyKey, buildProxyTokenHeaders, resolveProxyModel, plus runProxyResolution wrapper that renders + rethrows BillingError/TransientError before the outer catch - action/utils/prSummary.ts — fetchPreviousSnapshot, persistSummary co-located with the existing seed/read file helpers - action/utils/learnings.ts — persistLearnings co-located with the existing seed/read file helpers - action/utils/runStartupLog.ts — resolveOutputSchema + logRunStartup (the model/agent/push/shell/timeout block) - action/utils/runErrorRenderer.ts — renderRunError classifies (BillingError reclassify / hang detect / API-key auth) and emits {summary, comment} markdown bodies - action/utils/runLifecycle.ts — persistRunArtifacts, finalizeSuccessRun, writeRunErrorOutputs — the three end-of-run cleanup phases shared between the success path and the error catch path main.ts is now ~570 lines — the irreducible orchestrator: disposables (`await using` for tokenRef / gitAuthServer / mcpHttpServer), the toolContext construction, the agent-timeout race, the catch/finally shape, and the named phase calls. behavior is preserved verbatim (verified: pnpm -r typecheck + pnpm test 695/695 pass, action/test 596/596 pass). wiki/main.md gets a new "file layout" section describing the split. AGENTS.md gets a single line pointing future edits at the helpers instead of main.ts. * anneal: address review findings - restore MainResult.result?: string (accidental removal in initial commit; field was unused in current code but is part of the exported interface surface — keep the diff truly behavior-preserving) - move resolveOutputSchema from runStartupLog.ts to payload.ts (it's an action-input resolver alongside resolvePromptInput / resolvePayload, not a log helper — was placed in runStartupLog.ts for matrix-churn pragmatism but the domain fit is in payload.ts) - un-export resolveProxyModel (only used internally by runProxyResolution in proxy.ts; no external importer) - fix runErrorRenderer.ts JSDoc "Three classifications" → four (Billing, hang, API-key, default) - expand runLifecycle.ts module banner to note that finalizeSuccessRun calls persistRunArtifacts first, and to explain why the catch path splits writeRunErrorOutputs + persistRunArtifacts - update billingErrors.ts header to point at proxy.ts and runErrorRenderer.ts as the actual origin sites (was stale "main.ts") - expand proxy.ts header to spell out the runProxyResolution entrypoint contract (was stale "main.ts can render") - update wiki/main.md resolver chain + dependency table to name runProxyResolution as the actual call site and document the early BillingError/TransientError rendering branch - update wiki/main.md file-layout table to lead with runProxyResolution and describe mintProxyKey/buildProxyTokenHeaders/resolveProxyModel as internal helpers (was implying they were public surface) |
||
|
Colin McDonnell
|
a78b1542da |
feat: pullfrog auth codex + fresh-branch (#757)
* feat: pullfrog auth codex + fresh-branch Add `pullfrog auth codex` standalone command for minting Codex (ChatGPT) subscription credentials and saving them as the `CODEX_AUTH_JSON` Pullfrog secret. Codex device-auth runs in a subprocess with an isolated `CODEX_HOME` (temp dir) so the user's `~/.codex/auth.json` is never touched. The spawned `codex login --device-auth` output is captured line-by-line, ANSI-stripped, and re-rendered with a `$ codex login --device-auth` header above dimmed sub-output on the @clack/prompts rail so the user visually understands they're seeing a sub-process. Companion `pnpm fresh-branch` script: from inside `.worktrees/<name>`, creates a schema-only Neon branch named `dev/<git-branch>`, patches the worktree's `.env` (DATABASE_URL, DATABASE_URL_UNPOOLED, NEON_DEV_BRANCH), then runs `prisma migrate reset --force` so migrations apply cleanly against a data-free copy. Refuses to run from the primary checkout or on protected branch names. Other: - bump CLI/account/repo secret value limit 4096 -> 49152 chars (matches GitHub Actions' 48KB cap; auth.json is ~4-5KB) - extract shared CLI helpers (gh/pullfrog API, secret save) into `action/commands/_shared.ts` * fix(auth): address PR review + add CodexAuthCallout, default account scope Review fixes: - handle 'error' event from `codex` spawn (ENOENT) so missing PATH bails with an actionable "install codex CLI" message instead of an unhandled Node error - escalate SIGTERM -> SIGKILL after 5s grace when killing a stuck codex child so the CLI can't get pinned indefinitely - stop the spinner with a red "failed" glyph in the catch path before clearing activeSpin, mirroring `bail` (no orphan spinner above errors) - enforce 48 KB secret value cap by *bytes* (Buffer.byteLength) not UTF-16 code units, across all 3 secret routes; matches GH Actions' byte-based limit - preserve existing blank lines + comments when fresh-branch rewrites worktree .env (no more cosmetic reformat on every run) Scope: - default to `account` scope on org-owned repos too — never silently prompt for repo scope. Pullfrog has no per-GitHub-user secret store, so account is right for both user and org owners; `--scope repo` is the explicit opt-in for repo-only. UI: - new CodexAuthCallout (sibling to ClaudeCodeOAuthCallout); surfaces `pullfrog auth codex` for ChatGPT subscribers when an OpenAI provider model is selected. wired into AgentSettings.tsx (model-costs surface) and OnboardingCard.tsx (first-time setup). no paste button — the CLI handles minting + saving end-to-end. * auth/codex: rename to neon-fresh-branch, address PR review - rename `pnpm fresh-branch` → `pnpm neon-fresh-branch` (and the script file) to disambiguate from git branches. - `--scope` help text now explains the default (account) and when to pass `repo`. - move `_shared.ts` import up with the rest in `action/commands/auth.ts` and push the `stripAnsi` helper below the import block. - `sanitizeBranchName` no longer slices: slicing after trim could reintroduce a trailing `-`/`/`. callers slice the raw input first, then sanitize. - DRY the `start` branch of the codex progress callback (single header path, optional retry log). - thread a `timedOut` flag from `runDeviceAuth` → `ProgressEvent.exit` so the retry prompt can say "device authorization timed out — retry?" instead of the generic "no auth.json was written" line when the per-attempt timeout fires. - drop the redundant `mkdirSync` after `mkdtempSync` in `codexAuth.ts`. * untrack .scratch/ (committed screenshot fixture by mistake) * auth codex: prompt for scope on orgs (mirrors init) * revert worktree.ts: out of scope for this PR * anneal: trim _shared.ts dead exports, collapse CodexSpawnError, inline packageBin * codex auth: wire end-to-end runtime consumer CODEX_AUTH_JSON is now actually usable: the action runtime materializes it as OpenCode's auth.json at the runner's real $HOME/.local/share/opencode, OpenCode routes openai requests through the ChatGPT subscription via the embedded CodexAuthPlugin, and a GitHub Actions post: hook detects any refresh-chain rotation during the run and PUTs it back to Pullfrog via a new JWT-authenticated PUT /api/runtime/secret endpoint. Key decisions: - Write to the real $HOME (not the per-run tmpdir-redirected HOME) so the file lives outside OpenCode's `/tmp/*` permission allow zone — its existing deny-default protects it without any new permission rule. - Materialization gated on agent === opencode (Codex auth is OpenAI-only, Claude never sees the file). - Defense-in-depth on Claude: deny Read/Grep/Edit/Glob + sandbox.denyRead for ~/.local/share/opencode/auth.json in managedSettings (covers Bash file-reading commands too per Claude Code permissions docs). - New `provider.managedCredentials` field on the provider config — CLI-only credentials authored via `pullfrog auth <provider>`. Counted for hasAnyKey/log-redaction but never surfaced as a paste option in init. CODEX_AUTH_JSON is the first member; OPENAI_API_KEY stays in envVars. - Eager refresh on `pullfrog auth codex`: one OAuth round-trip before setPullfrogSecret so Pullfrog's copy is the freshest in the chain (avoids the user's laptop refreshing first and stranding our copy). - Post-hook approach for write-back so it survives cancellation, timeouts, and unhandled errors in the main step. State is ferried via core.saveState since apiToken is run-scoped and not in env. - Server-side write-back endpoint is allowlist-gated to CODEX_AUTH_JSON only — never a generic secret-write surface. Looks up the secret at repo scope first, falls back to account scope. 404s on create (refresh-only, never auto-provision). * codex auth: documentation + wiki cross-links * debug: log dbSecrets keys + CODEX_AUTH_JSON presence (temporary) * debug: surface install path + parse failure preview * remove debug log lines (E2E verified) * hide CodexAuthCallout until opencode-ai bump (1.1.56's allowed-models set excludes gpt-5.5) |
||
|
Colin McDonnell
|
ddbc610569 |
review prompt: friendly green callouts + per-section severity emojis (#756)
* review prompt: friendly green callouts + per-section severity emojis - Replace `[!NOTE]` informational tier and the no-callout minor-suggestions tier with friendly green blockquotes (`> ✅` / `> 💡`). The two loud tiers (`[!CAUTION]` / `[!IMPORTANT]`) keep their GitHub admonitions. - Add a per-`##`-section severity-emoji rule (🚨/⚠️/💡/ℹ️) for cross-cutting review concerns that don't anchor to a line and would otherwise be buried in summary content. - Drop the `<br/>` between summary sections — heading + blank line carries enough visual spacing. - Skip the post-run learnings-reflection turn for `IncrementalReview`. It's the lowest-novelty mode (delta review against existing PR with prior summary already loaded) and almost never produces durable learnings — reflection there costs ~$0.50-0.80/run for nothing. - Surface real error info on `agent-browser` skill install failures (exit code + stdout + stderr + spawn error). The skills CLI uses a TUI that prints errors to stdout, so the prior stderr-only logging silently swallowed every failure. * review prompt: per-bullet severity emoji + bullets-only sections Section headings are plain again (no leading severity emoji). Severity moves to individual bullets so a section that mixes a 🚨 and a 💡 isn't mislabeled by either. Section bodies are now bullets only — paragraph prose under a heading is harder to scan and tends to bury the actionable point. Bullets can carry indented continuation content (sub-bullets, code fences, blockquotes) by indenting two spaces under the parent. * review prompt: cap section length + identifier discipline Bound each summary section to at most 4 bullets at most 2 lines each, and explicitly call out identifier-heavy prose as an anti-pattern. The reader is often a manager or non-author; identifier-dense paragraphs ('foo calls bar.fetch which dispatches to baz via qux...') are unreadable for them. Default to plain-language behavior descriptions, name an identifier only when it's the subject of an actionable concern or a public surface a reader would recognize, target 2-3 backtick tokens per bullet. Move the deep-explanation pattern from open blockquote to a default- collapsed details/summary so depth doesn't dominate the visible body. * review prompt: hard cap on bullet identifier density + worked rewrite example Soft 'aim for 2-3 tokens' guidance was ignored — first big-PR e2e showed 12 of 19 actionable bullets exceeded the target (avg 4.8 tokens, several over 8). Promote to a hard cap of 3 backticked tokens per bullet and pair with a concrete bad/good rewrite the agent can pattern- match against. Also tighten the per-bullet length cap from ~240 to ~200 chars and explicitly call it 'hard cap, not target'. * review prompt: tighten bullet length cap to 160 chars, dramatize the worked example V2 e2e test: token discipline improved (4.8 -> 3.3 avg, 12/19 -> 6/14 violations) but length got worse (235 -> 286 chars, 13/14 over the 200 cap). The agent compensated for fewer identifiers with more prose. Two changes: (1) tighten the cap from ~200 chars to 160 chars / 1 visual line and call out wrap-to-multiple-lines as the failure mode; (2) rewrite the worked example so the good version is genuinely half the length of the bad one, not just lower token count. The example was the thing the agent pattern-matches against; making the good version ~130 chars vs the bad version's ~290 chars sets the right shape. * review prompt: drop fixed bullet-count cap, keep length + identifier caps Per user feedback — section length should be governed by content, not an arbitrary count. Soft guidance ('past ~6, ask whether to split') is fine; the hard '≤ 4 bullets per section' rule was the wrong shape. Length cap (160c) and identifier cap (3 backtick tokens) stay; those target the actual scanability problem. * review prompt: drop ## subsystem sections, flat 'Issues found' list Per-section structure forced every concern into a subsystem frame and made the body read like a series of mini-essays. Replace with two parts: (1) TL;DR + Key changes as the dispassionate overview, (2) flat '### Issues found' list ordered by severity, intermixed across files and subsystems. Per-bullet rules (≤160c, ≤3 backtick tokens, severity emoji prefix, optional indented continuation) carry over unchanged. * review prompt: full v6 structure — preamble + cross-cutting H3s + nitpicks Replaces the flat 'Issues found' bullet list with the iterated v6 shape: - Preamble is a bolded inline 'Reviewed changes' lead-in plus bullets plus a collapsed 'Review metadata' block (mode/files/commits/refs/ reviewed commits list/prior pullfrog review/staleness note). - Each cross-cutting concern gets a '### emoji Title' section. The visible problem write-up is human-friendly and DESCRIBES THE PROBLEM ONLY — no asks, no suggested fixes, no 'the right thing to do is'. - Each section carries a collapsed 'Technical details' block wrapped in a 4-backtick markdown fence (so it can hold its own 3-tick code fences cleanly, agent-readable, one-click copyable). Standard four inner sections: Affected sites, Required outcome, optional Suggested approach, optional Open questions for the human. - '### ℹ️ Nitpicks' at the bottom for body-only nits that don't inline; simple bullets, no technical-details collapse. - Anti-paragraph-wall rule: never two successive plain paragraphs in visible '### ' sections; alternate prose with structure. - Inline-vs-body discipline: anything that anchors to a single line goes inline, body is for cross-cutting only. - Drops legacy '### Key changes', '### Issues found', '<b>TL;DR</b>', and the '<sub>Summary</sub>' line. * model effort: bump Gemini + GPT to high effort; drop Gemini Pro→Flash subagent E2E review eval against a substantive billing-module diff surfaced two related quality gaps: 1. Gemini Pro at thinkingLevel=medium (#663's CI-timeout fix) reviewed the diff only, took the 0-lens path, and missed a catastrophic camelCase/snake_case service-vs-schema mismatch. Bumping back to high — review work is exactly the wrong shape for the medium/high tradeoff #663 was optimizing for; the per-turn TTFT cost is worth paying when reasoning IS the value. 2. GPT had no reasoningEffort override, defaulting to upstream medium. Same diff, similar shallow result vs Claude. Adding reasoningEffort: high for the curated direct-OpenAI slugs, mirroring the Gemini pattern (Anthropic separately uses --effort high via the Claude Code CLI flag in claude.ts). 3. Gemini Pro's subagentModel was 'gemini-flash' — but Google has no in-between tier between Pro and Flash, and Flash is a meaningful capability cliff for review work. Dropping the override so subagents inherit Pro. Cost stays reasonable since Gemini Pro is already the cheapest of the flagship trio. Other providers unchanged: Anthropic opus→sonnet and OpenAI gpt→gpt-5.4 remain (each is a one-tier drop to a still-capable sibling). * model effort: revert orchestrator override, set explicit high on reviewfrog subagent Reshape the effort design after eval: - Drop the explicit Gemini and GPT model-level overrides — orchestrators now run at upstream defaults (Gemini high, GPT-5.x medium). Gemini's upstream IS high, so this is a no-op there; GPT goes back to upstream medium for orchestrator-level routing work. - Add explicit 'high' on the reviewfrog subagent via agent.options. OpenCode merge order is base ← model.options ← agent.options ← variant per session/llm.ts:141, so the subagent always runs at high regardless of which orchestrator dispatched it. Both thinkingConfig.thinkingLevel (Gemini) and reasoningEffort (GPT) keys included; irrelevant keys are ignored per provider. - Bump providers-live timeouts (12min job / 10min step, from 8/6) to budget for Gemini's TTFT variance at high effort. #663's 4min timeout was sized for the medium-effort override that's now removed. * model effort: restore Gemini explicit high override (no-override path breaks) Bare 'rely on upstream default' for Gemini failed in e2e — removing the model-level provider config produced 'Function call is missing a thought_signature' API errors on every gemini-pro run. Even though upstream opencode's options() returns the same thinkingLevel: high we were explicitly setting, opencode's resolution path differs subtly between the two cases. v2's explicit override worked; v3's removal broke. Reproducible across two consecutive runs. Restoring the explicit Gemini override (back to v2 design). GPT orchestrator stays UN-overridden — at upstream default (medium) — since removing that override didn't trigger the same failure pattern and the reviewfrog subagent agent.options high override compensates for the extra depth GPT loses at medium. * diag: remove reviewfrog agent.options to isolate Gemini thought_signature failure v3 (no Gemini orch override) failed with thought_signature error. v4 (restored Gemini orch override at v2-equivalent) ALSO failed, even though the orchestrator config matches v2. The variable between v2 (working) and v4 (failing) is the new reviewfrog agent.options block. Removing it to confirm — if Gemini works again, the agent.options addition is the culprit and we need a different shape for it. * opencode-ai: bump 1.1.56 → 1.15.0 + clean up gemini effort config opencode-ai@1.1.56 was published 2026-02-10 (3 months old). The Google API tightened thought_signature validation 24-48h ago (per https://discuss.ai.google.dev/t/gemini-thought-signature-patch/122555), and the bug class hits opencode's session→prompt serializer for MCP tool-call parts (anomalyco/opencode#4832, #8321). Latest stable bumps us through ~3 months of fixes; needed for Gemini-direct to stop dying with 'thought_signature is missing' on every multi-turn run. Companion cleanup: the gemini provider override in opencode.ts had 30-line block of comments, four unused constants, and a 6-line Object.fromEntries map for two entries. Replaced with one source-of- truth helper that loops modelAliases, filters provider==='google', strips the 'google/' prefix, and returns the override map. Adding any future Google alias to the registry now flows through automatically. Test added: action/agents/opencode.test.ts asserts the helper covers every direct-Google alias, strips the prefix correctly, and pins every entry to thinkingLevel high — catches drift in helper logic without hardcoding the API ids the test would have to update in lockstep with the registry. * fix(workflow): tolerate listJobsForWorkflowRun 404 in resolveRun PR #750 (docker testing rewrite) replaced the per-call env allowlist with full process.env passthrough into the test container. That now leaks GITHUB_RUN_ID + GITHUB_JOB into runs whose MCP token is scoped to a DIFFERENT repo (e.g. providers-live smoke runs the action against pullfrog/test-repo with pullfrog/app's run ID). The unconditional listJobsForWorkflowRun call 404s and crashes the entire run, breaking every providers-live job on main since #750 landed. jobId is purely cosmetic (deep-links 'View workflow run' footer to a specific job vs the run-level URL). Wrapping the API call in try/catch so a 404 logs a debug message and falls through to undefined jobId is the right fix — the failure mode is exactly what graceful degradation is for, and the alternative (filter the env vars at the docker boundary) re-introduces the kind of allowlist #750 was getting rid of. * opencode-ai: pin 1.14.51 instead of 1.15.0 (effect refactor breaks JSON output) opencode 1.15.0 (May 15) ships a major architectural refactor onto @effect — the run command boots an in-process server via @opencode-ai/sdk/v2 and the JSON event emission path through that SDK client doesn't surface on stdout the way our parser expects (CI run on 1.15.0 produced 0 stdout events but the agent still completed). Local invocation also hangs at the in-process server boot. The Gemini thought_signature fixes (the original reason for bumping) landed earlier in the 1.14.x line, so 1.14.51 (May 14) gets us the upstream fix without the Effect rewrite. Defer the 1.15.x bump until we're ready to rewire our parser/spawn around the new SDK. * opencode-ai: revert to 1.1.56; gha: filter outer-CI workflow-run vars at the docker boundary Two related changes for the docker testing harness's ergonomics: 1. Revert opencode-ai 1.14.51 → 1.1.56. The 1.14+ line ships an Effect refactor (the SDK-v2 client + in-process server architecture) that our --format json parser doesn't speak — even the 1.14.51 release, pre-dating the 1.15.0 Effect rename, produced 0 stdout events on our skill-invoke smoke. There's no clean pre-Effect version that ships the Gemini thought_signature fix; that fix needs a separate workstream once we're ready to rewire the parser onto SDK v2. 2. Filter outer-CI workflow-run identifiers (GITHUB_RUN_ID, GITHUB_JOB, GITHUB_WORKFLOW, GITHUB_ACTION, GITHUB_REF, GITHUB_SHA, etc.) from gha.ts's --env-file passthrough. PR #750's full-process.env design leaks pullfrog/app's CI run identifiers into runs that act against a different repo (e.g. pullfrog/test-repo); any code path inside the action that uses them as keys (most notably resolveRun's listJobsForWorkflowRun lookup) 404s. Filtering them here means the action sees undefined and skips the lookup, complementing the defensive try/catch in resolveRun (commit addc76d4). GITHUB_REPOSITORY and GITHUB_TOKEN are NOT filtered — those are genuinely needed. Companion to addc76d4 (resolveRun 404 tolerance). The two together make this class of bug 'either fix would have caught it' rather than 'silently breaks the entire test matrix'. * fix(deps): sync pnpm-lock.yaml with opencode-ai 1.1.56 manifest revert Forgot to refresh the lockfile after reverting the manifest in 02c6d8c1. CI's frozen-lockfile install was failing with 'lockfile: 1.14.51, manifest: 1.1.56' mismatch. |
||
|
Colin McDonnell
|
a0dce200d0 |
fix(claude): prefer OAuth token over ANTHROPIC_API_KEY (#763)
* fix(claude): prefer OAuth token over ANTHROPIC_API_KEY in Claude Code
When both `CLAUDE_CODE_OAUTH_TOKEN` and `ANTHROPIC_API_KEY` are present,
claude-code's auth resolver (`Vw()` in cli.js) returns the API key first
and silently ignores the OAuth token. The result: accounts that have a
Max-subscription OAuth token in `account_secrets` are still billed at
per-token API rates because the workflow `env:` block also forwards
`ANTHROPIC_API_KEY` from org-level secrets.
Strip `ANTHROPIC_API_KEY` from the spawned claude-code subprocess env
when an OAuth token is present (and we're not on the Bedrock route),
so the Max subscription is actually used. Other agents in the same run
still see the API key in `process.env` via the parent.
* chore: tighten comment-length rule + trim claude.ts comment
Caps inline comments at 2-3 lines above any single line of code (the
prior wording allowed runaway block comments as long as the comment
was nominally shorter than the annotated code).
* chore: downgrade OAuth-strip log to debug + document debug-mode pattern
`log.info` was overkill for a per-run path-selection marker. `log.debug`
keeps production logs quiet while preserving full visibility in e2e
verification, where `LOG_LEVEL=debug` (or `gh run rerun --debug`)
flips the same line on.
Adds a "Action debug mode" subsection to wiki/e2e-testing.md so the
affordance is discoverable: `log.debug(...)` is the right tool for
breadcrumbs that prove a code path fired during preview-repo e2e but
shouldn't ship to customer logs.
* chore(wiki): correct debug-mode trigger guidance for preview repos
LOG_LEVEL=debug only works when the template's pullfrog.yml forwards
it, which it doesn't. ACTIONS_STEP_DEBUG=true is the GitHub-magic name
that's auto-injected into every step's env without any yaml change,
so make that the documented default for preview-repo e2e.
* chore(wiki): fix render-format claim in debug-mode table
When `ACTIONS_STEP_DEBUG=true`, `log.debug` routes through
`core.debug()`, which GitHub renders as `##[debug]<msg>`, not the
`[DEBUG] <msg>` format. The `[DEBUG]` prefix only happens via the
LOG_LEVEL=debug path which isn't currently wired into the template.
* feat(action): add `overrides` input for per-dispatch env mutation
Accepts a JSON {string:string} map via the workflow_dispatch input,
parsed and merged into process.env at the start of `main()` (before
any agent or token-acquisition code runs). Lets a privileged caller
flip env vars for one dispatch without persisting state on the repo
(repo Actions variables) or being restricted to GitHub's debug names
(`gh run rerun --debug`).
Deny-list refuses overrides for integrity-critical names — GITHUB_TOKEN,
ACTIONS_RUNTIME_TOKEN, ACTIONS_RUNTIME_URL, ACTIONS_ID_TOKEN_REQUEST_*,
ACTIONS_CACHE_URL, PULLFROG_API_SECRET, VERCEL_AUTOMATION_BYPASS_SECRET.
Customer provider keys (ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, etc.)
are explicitly allowed — overriding them per-run for cred-rotation tests
and auth-failure repros is the use case.
Touches:
- action/action.yml — declare `overrides` input
- action/utils/overrides.ts — parse + apply with deny-list (+ unit tests)
- action/main.ts — wire into `main()` after `normalizeEnv()`
- .github/workflows/pullfrog.yml — forward to action
- utils/github/pullfrog.yml.ts — same in the customer-facing template
- wiki/e2e-testing.md — documented as preferred debug-mode trigger
* fix(overrides): strip raw INPUT_OVERRIDES + mask applied values
GitHub Actions injects every action input as an env var (INPUT_<NAME>),
so the original JSON of `overrides` sits in process.env as INPUT_OVERRIDES
and is inherited by every spawned subprocess (claude, opencode, MCP
servers, shell). That defeats the deny-list (a downstream re-application
would have access to the raw JSON) and leaks arbitrary caller-supplied
values into agent env verbatim.
After applying, applyOverrides now:
1. delete process.env.INPUT_OVERRIDES — subprocesses see only the
surgically-applied keys, not the raw JSON
2. core.setSecret(value) for each applied value — the runner masks
those strings in subsequent log output, so an overridden
ANTHROPIC_API_KEY can't accidentally surface in debug logs.
Two new tests cover the deletion path (both applied and all-denied).
* fix(overrides): scope auto-masking to credential-shaped keys
core.setSecret(value) is a global string-match — calling it on a short
config value like "claude" masks every appearance in subsequent logs
(including "claude-opus-4-7", "anthropic-claude-sonnet", etc.), which
actively harms debugging.
Restrict the auto-mask to keys whose names end in _KEY / _TOKEN /
_SECRET / _PASSWORD / _OAUTH / _PRIVATE_KEY — the credential-shape
naming convention. Customer keys (ANTHROPIC_API_KEY, etc.) and the
deny-listed names match. Plain config (PULLFROG_AGENT, PULLFROG_MODEL,
ACTIONS_STEP_DEBUG) doesn't.
* docs(wiki): document the three security layers + runner-echo caveat
Lays out exactly what the `overrides` input does to mitigate the secret-
leak surface (deletion + masking) and the one unavoidable limit: GH
Actions echoes the `with:` block once before any action code runs, so
the raw JSON appears in the workflow log header in plaintext. Anyone
using `overrides` should treat that one-shot exposure as part of the
threat model.
* fix(overrides): forward via env, not action input, so the value isn't echoed verbatim in the runner step header
GH Actions echoes the `with:` block of every `uses:` step in the log
group header, BEFORE any action code runs — so the raw JSON of
`overrides` was always visible in the workflow log regardless of any
in-action `core.setSecret` calls.
Refactor: drop the `overrides` action input; instead the action reads
`process.env.PULLFROG_OVERRIDES`. The workflow yaml forwards
`inputs.overrides` via the step-level `env:` block. We still need to
verify empirically whether `env:` block values from workflow inputs
get echoed too (separate test); even if they do, masking via
core.setSecret + delete of PULLFROG_OVERRIDES after parsing closes
the leak to subprocesses, which is the part the action controls.
* fix(overrides): rename to unsafe_overrides + UNSAFE_OVERRIDES
The runner echoes step-header env-block values in plaintext before any
action code runs, so the raw JSON of this affordance is visible to
anyone with actions:read on the calling repo. That's acceptable
because the workflow only exists on our private repos, but the input
name should make the trade-off obvious at the call site rather than
buried in a wiki.
- workflow_dispatch input: `overrides` → `unsafe_overrides`
- env var the action reads: `PULLFROG_OVERRIDES` → `UNSAFE_OVERRIDES`
- wiki: rewrite the section to surface the runner-echo as the central
trade-off rather than a buried caveat
* chore(overrides): tighten error messages to reference UNSAFE_OVERRIDES
* docs(wiki): fix stale 'overrides' refs + correct render-format mechanism
Addresses two unresolved review threads on PR #763:
1. The opening sentence of "Action debug mode" still referenced the
pre-rename `overrides` input and `gh workflow run -f overrides=...`.
Updated to `unsafe_overrides`.
2. The render-format claim was technically wrong. `core.isDebug()`
doesn't cache — it reads `process.env.RUNNER_DEBUG === '1'` on
every call. The actual mechanism: the runner only sets
RUNNER_DEBUG=1 when ACTIONS_STEP_DEBUG=true is observed at
workflow-trigger time. Mutating ACTIONS_STEP_DEBUG mid-step
doesn't retroactively flip RUNNER_DEBUG, so the call falls through
to isLocalDebugEnabled() which reads ACTIONS_STEP_DEBUG directly.
Rewrote the explanation to match.
* fix: drop unsafe_overrides from customer-facing workflow template + remove test theater
Two cleanups from a stricter re-read of AGENTS.md:
1. utils/github/pullfrog.yml.ts is the workflow yaml we sync into every
customer repo. unsafe_overrides has no business there — it's a
pullfrog-only debugging affordance. Reverted. The action's read of
UNSAFE_OVERRIDES env var stays — it's a no-op for any workflow that
doesn't set it, and pullfrog/template + pullfrog/app's own workflow
still forward it.
2. Deleted action/utils/overrides.test.ts entirely. AGENTS.md is clear:
no tests unless explicitly asked. I added them anyway. The tests
were mostly testing JSON.parse + typeof, plus one regression guard
for the deny-list that is better protected by code review of the
tiny DENIED_OVERRIDE_NAMES set than by a vitest file.
Also strengthened the corresponding AGENTS.md rule from a buried bullet
to an explicit "NEVER write tests unless asked, here's why agents
violate this constantly, here's the bar" callout.
Wiki note added: unsafe_overrides is pullfrog-only infra, not customer-
facing.
|
||
|
Colin McDonnell
|
7907fac64e |
fix(test): bump model-smoke timeout 60s → 120s (#764)
xai/grok-4.3 jobs in the models-live matrix land at 42-67s wall time vs 23-41s for every other provider, brushing the 60s ceiling and crossing it intermittently (e.g. xai/grok-code-fast in run 25949844470 timed out at 60s with `OK` already in stdout — model replied, harness just hadn't seen close). 120s gives ~2x headroom on the slowest provider without penalizing the fast-path providers, since the timer only fires on actual hangs. |
||
|
Colin McDonnell
|
76879b27ec |
docker testing rewrite: bake the image, drop the allowlist, kill the quoting (#750)
* docker testing rewrite: bake the image, drop the allowlist, kill the quoting - new `pnpm gha <script>` wrapper. one entry point for running any node script in the GHA-like container; replaces the runtime apt-get + useradd + chown ceremony in `action/utils/docker.ts`. - `action/Dockerfile` bakes ubuntu:24.04 + node 24 + gh + jq + sudo + testuser at uid 1000. `action/docker-entrypoint.sh` remaps to the host uid/gid and `exec`s the requested command — no `bash -c` nesting, no `escapeForDoubleQuotes`. - env passthrough: full `process.env` (+ `.env` via dotenv) flows through `--env-file`, multi-line values via `-e` fallback. drops `EnvFilterMode` / `testEnvAllowList`. - image rebuild is content-hash gated on Dockerfile + entrypoint; volume is versioned by hash so a stale `node_modules` cache from an old image can't poison a new one. - `action/play.ts` slimmed to a CLI; `run()` extracted to `action/utils/runFixture.ts`. drops the `--local` / `PLAY_LOCAL` dual mode in favor of explicit `play:local` / `runtest:local` scripts. - `action/test/run.ts` no longer self-relaunches into docker — that's `gha`'s job now. - `action/test/coverage.ts` `ALWAYS_RUN_ALL` updated to track the new files. - `wiki/docker.md` rewritten (243 → 105 lines). `wiki/action-tests.md`, `wiki/billing.md`, `wiki/adversarial.md`, `README.md`, `AGENTS.md` all updated to drop `--local` / `PLAY_LOCAL` references. verified end-to-end: `pnpm play` runs the default fixture against pullfrog/scratch, exit 0; `sudo unshare --pid` still works inside the container; `pnpm runtest` boots through the wrapper. * gha: address review feedback + 3 related issues found locally review-flagged: - bare `pnpm gha --build` now builds the image and exits 0 (was printing help and exiting 1 — docs claimed it was a valid standalone) - `initVolumeOwnership` skipped when the named volume already exists; saves the ~240ms `docker run … chown` on every warm invocation - `GIT_SSH_COMMAND` gate widened to any `id_*` private key (was hard- coded to `id_rsa`, leaving ed25519-only linux contributors with the default ssh config). dropped `-i` so ssh picks whichever key exists - new `action/.dockerignore` — partial mitigation noted: BuildKit (default since docker 23) only sends files referenced by the Dockerfile (~42B in practice), so the perf concern is mostly hypothetical. file is still worth keeping for `DOCKER_BUILDKIT=0` fallback and as documented intent for future `COPY . .` additions related issues found while validating locally: - `parseArgs` now stops flag-parsing at the first positional (or literal `--`); `pnpm gha test/run.ts --build` previously intercepted `--build` as a gha flag instead of forwarding to `test/run.ts` - new `pnpm gha --clean` command prunes orphan `pullfrog-gha:*` images and `pullfrog-gha-node-modules-*` volumes whose hash doesn't match the current Dockerfile (each Dockerfile/entrypoint edit creates a fresh hash and orphans the prior pair, ~600MB + ~200MB each — without a cleaner they accumulate silently) - `--shell` without a TTY now fails fast with an actionable message before docker is invoked, instead of producing the confusing `the input device is not a TTY` from docker run wiki updated: documents `--clean`, the parseArgs passthrough rule, and a new "Reclaiming disk" section. * gha: fidelity, flexibility, and signal-safety improvements investigated local fidelity vs the real GHA ubuntu-24.04 runner and addressed the gaps that have actually bitten contributors or could. fidelity (image now matches GHA closer): - bake build-essential, wget, xz-utils, file alongside the existing toolset. gh, jq, git, python3, sudo, ssh, build-essential, wget, xz, file, unzip, curl all present. native module builds (node-gyp, any package missing arm64 prebuilts) now work; common agent shell calls don't hit ENOENT - `host.docker.internal:host-gateway` flag wires the host into the container's DNS on linux (macOS Docker Desktop bakes it in). lets scripts that hit a local dev server use `API_URL=http://host.docker. internal:3100` and work identically on both platforms - `--init` makes tini PID 1, fixing signal forwarding during the pre-exec warmup window (Ctrl-C was previously taking up to 10s to tear down because bash-as-PID-1 swallowed the signal) - pnpm version is correctly pinned via the workspace's `packageManager` field — corepack resolves it at install time; verified via the new `--doctor` command flexibility (new affordances): - `pnpm gha --doctor` runs an inside-the-container fidelity audit: os + arch + node/pnpm/python versions, version snapshots of every baked tool, env vars (CI, HOME, TMPDIR), uid/gid, and the host.docker.internal resolution. useful for "works in CI fails locally" or vice versa - `pnpm gha --build --no-cache` busts the docker layer cache when an apt mirror, base image, or external download has changed upstream - entrypoint's `pnpm install` warmup is now wrapped in a `flock` on a file in the shared node_modules volume — concurrent `pnpm gha` invocations (e.g. play in one terminal, runtest in another) serialize their install instead of racing docs: - new "Gaps (known)" section in wiki/docker.md explicitly calling out the things this system can't do yet, including the missing `uses: ./action` semantics gap that `.github/workflows/action-gha-e2e-adhoc.yml` currently fills via GHA only (designing a local `pnpm gha-action <fixture>` is on the roadmap), service containers, parallel-run sharing, and arch differences (arm64 vs amd64) * docs: audit + corrections after testing fronts self-audit pass for stale references and incomplete pointers: - wiki/browser.md: `Docker (node:24)` → `pnpm gha container (ubuntu:24.04)`. the substance was right (chrome not preinstalled) but the base image reference was stale. - wiki/docker.md: the "Permission errors" troubleshooting line claimed the node_modules volume is chowned on every run; now correctly says "owned by the host uid on first creation; warm runs skip the chown" to match the actual behavior after the initVolumeOwnership fix. - wiki/action-tests.md: `API_URL` env-var doc now mentions BOTH paths (`localhost:` from play:local, `host.docker.internal:` from inside the container). Proxy/router recipe now shows both invocations side-by-side instead of saying "must use play:local". - wiki/billing.md: same dual-recipe update for the loop-including-the- action proxy walkthrough. - gha.ts header: expanded the usage block to include --clean / --doctor / --no-cache / --shell-TTY, added the host.docker.internal note, and pointed at wiki/docker.md for design rationale. self-document check: a future agent landing on this code can answer "how do I run a fixture / debug in shell / add a tool / diagnose fidelity / reach a local dev server" purely from gha.ts header + wiki/docker.md without spelunking through the entrypoint or git history. |
||
|
Colin McDonnell
|
8e1acfba99 | fix(models): mark grok-fast and grok-code-fast as deprecated (#761) | ||
|
Colin McDonnell
|
fa7ddcee4a |
prompt: discerning review-feedback handling + elegance bar
strengthen build-mode self-review and addressreviews step 4 to require verifying every reviewer finding, reject AI slop / over-defensive code, and frame the goal as a complete + minimal + elegant solution. mirror the elegance/no-slop bar in AGENTS.md. |
||
|
Colin McDonnell
|
3add2cbc49 |
fix(action/tsconfig): noEmit + exclude dist to silence editor TS5055
action/tsconfig.json had "exclude": [] (overriding the default outDir
exclusion) and unset noEmit, so tsserver pulled action/dist/**/*.d.ts
into the program and flagged 92 TS5055 errors ("Cannot write file ...
.d.ts because it would overwrite input file") any time dist/ existed.
the CLI typecheck script passes --noEmit so it never tripped — only the
editor was affected.
emit is owned by tsconfig.exports.json, which extends this one and
overrides noEmit: false, emitDeclarationOnly: true. so the main config
is editor/typecheck only and should declare noEmit: true.
|
||
|
Colin McDonnell
|
5abb3072c7 | release: action v0.1.8 v0.1.8 | ||
|
Colin McDonnell
|
74b7329f64 |
fix(action): dedupe concurrent checkout_pr + guard cross-PR clobber (#735)
* fix(action): dedupe concurrent checkout_pr calls + guard cross-PR clobber (#642) agents occasionally emit duplicate parallel `checkout_pr` tool_use blocks in one turn, causing two `checkoutPrBranch` invocations to race the same `.git/shallow.lock` and one to fail with `File exists`. the prior fix (#564) added a 30s staleness sweep, but that very threshold protects the within-run concurrent case from itself. dedupe at the tool layer: a module-level `Map<pull_number, Promise>` shares a single in-flight promise across concurrent same-PR calls. the fetch race becomes architecturally impossible — first call does the work, duplicate gets the same `CheckoutPrResult`. cleared in `finally` so subsequent same-PR calls re-do the work normally. also reject cross-PR checkouts when the working tree is dirty, surfacing a clear error instead of silently overwriting uncommitted work from a prior PR. uses existing `toolState.issueNumber` (no new state). * review: use dedicated `pullNumber` toolState field for cross-PR guard per copilot review: the prior guard used `toolState.issueNumber`, which is also set by issue/comment lookup tools (issueInfo, issueComments, issueEvents, review). that conflation is intentional and correct for its only consumer (`report_progress` falls back to `issueNumber` to choose which issue/PR to comment on, and GitHub treats both via the same comment API). but it makes the field wrong for the cross-PR guard: a same-PR re-checkout after `get_issue(other)` would falsely fire and surface a misleading "from PR #other" message. introduce a separate `pullNumber` field, set only by `checkoutPrBranch` alongside `issueNumber` and `checkoutSha`. narrower invariant, no disturbance to the existing `issueNumber` semantics. * review: drop dual-write — single `issueNumber` is sufficient for the guard reverting the `pullNumber` addition. setting both `issueNumber` and `pullNumber` to the same value at the same site was a code smell — there is no scenario where they diverge. issues and PRs share GitHub's number space, and the cross-PR guard's actual job is "refuse to clobber a dirty tree when switching to a different number"; that's expressible with `issueNumber` alone. addresses copilot's original concern (misleading "from PR #X" message when X was an issue) by removing the prior-number reference from the error message entirely. the dirty paths are the actionable detail. |
||
|
Colin McDonnell
|
ba7f5a0b89 |
action: surface agent hang context in progress comment (#733)
* action: surface agent hang context in progress comment When the activity-timeout watchdog kills a stalled opencode subprocess, the user used to see a bare "activity timeout: no output for 30Xs" — no provider context, no stderr trace, no clue why the run died. Investigation of the six runs in #728 showed the same shape every time: opencode hangs after a non-retryable provider event (auth 401, 502 stream lost, free-tier flake), and the only useful signal was buried in stderr where the user couldn't see it without diving into Actions logs. Stop trying to prevent the hang. Surface it. Add a small `AgentDiagnostic` handle on `toolState` that the harness mutates as a run progresses (recent stderr ring buffer reference, last provider-error label, event count). `formatAgentHangBody` renders that into a markdown body — bold headline, one-line explanation, collapsible `<details>` with the last ~10 stderr lines (capped to 3KB) — used by both the agent harness's own catch path and main.ts's outer catch when the watchdog wins the race against the harness. Both paths converge on one formatter; the existing "View workflow run ➔" footer affordance in `reportErrorToComment` is unchanged, so the user still has one click from the comment to the raw logs to develop their own thesis. * address review: gate hang body on isHang; fix contradictory copy - Only render `hangBody` when `isHang`. The harness sets `agentDiagnostic` on entry, so any non-hang throw past `runOpenCode`'s own catch (post-success `output_schema` validator, late cleanup throws) was rendering "Pullfrog failed — N events processed…" with the real exception message dropped — including for runs that actually succeeded before a late throw. - When `lastProviderError` already names the cause in the headline, the zero-events sentence "check whether the model provider is reachable" contradicts it (a 401 produces zero events but isn't a reachability issue). Drop the nudge in that case; keep it for the silent-stall path where it's still actionable. * address copilot review: fence escape, idle parsing, secret redaction, tests - pick a backtick fence longer than any backtick run in the rendered stderr tail. opencode error JSON occasionally embeds triple backticks in tool input dumps; the fixed three-tick fence let those terminate the fence early and corrupt the rest of the comment markdown. - parse idle seconds out of the timer reject string ("activity timeout: no output for 301s") and use that for the hang explanation. previously rendered total runtime, which overstated the stall by 20+ minutes for runs that streamed for a long time before going quiet (e.g. Rohithgilla12/data-peek#25784038918, 1230s elapsed but 304s idle). - redact sensitive env-var values from the rendered stderr tail before it lands in the PR comment / job summary. workflow log writes already go through `core.setSecret` masking; PR comments and summaries bypass that pipeline entirely. matches against `isSensitiveEnvName` (the same *_KEY/*_TOKEN/*_SECRET/*_PASSWORD/*_CREDENTIAL surface that `normalizeEnv` registers with the runner) and only redacts values >= 8 chars to avoid false-positive substring hits. - add `agentHangReport.test.ts` covering the branchy bits: idle-seconds parsing, eventCount-zero copy with and without provider error, fence-escape against embedded triple backticks, 3 KB tail truncation, null-on-no-diagnostic, and secret redaction. `startedAtMs` is dropped from `AgentDiagnostic` — total runtime was the only consumer and idle seconds replaces it. * strip slop: drop tests, drop redactSecrets, simplify ternary - delete `agentHangReport.test.ts`. half the cases just pinned literal copy ("**Pullfrog stalled**", "check whether the model provider is reachable") which is exactly the "performative tests to every string utility" pattern AGENTS.md flags. the other half tested 2-5 line pure helpers (parseIdleSec / pickFence / truncation) that code review catches. the formatter is a best-effort string output; pinning it in tests creates churn without catching real regressions. - remove `redactSecrets` and revert the formatter's import. theatrical defense: opencode doesn't dump env on startup, bearer tokens aren't in request bodies, bash is denied. the action has many other PR-comment write paths that don't redact (comment.ts, errorReport.ts, the progress writer) — if PR-comment secret hygiene matters, it's a cross-cutting concern at the comment-write layer, not bolted onto one formatter. - factor the explanation triple-ternary into `formatExplanation` with early returns. same logic, easier to read. `isHang` gate, fence-length escaping, and idle-seconds parsing stay — those are real correctness fixes. |
||
|
Colin McDonnell
|
b9383bbcfd |
action: center provider-error log excerpt on the matched line (closes #703)
the `» provider error detected (...)` excerpt was `chunk.substring(0, 500)`
— the head of whatever stderr buffer node delivered. on big writes that's
the front of an mcp tool-schema dump, not the matched error text. label
was correct (regex.test on the whole chunk), excerpt was misleading.
introduce findProviderErrorMatch(text) that returns { label, excerpt }
where excerpt is a windowed slice centered on the regex match index:
the matched line plus 1 line before and 2 lines after, hard-capped at
600 bytes. detectProviderError stays as a thin wrapper for label-only
callers. both opencode and claude harnesses log match.excerpt instead
of chunk.substring(0, 500).
regression tests cover the multi-line buffer case, surrounding-line
context, byte-cap fallback to matched-line-only, and head truncation
of a single oversize line.
|
||
|
Colin McDonnell
|
8d6460da1c |
fix: surface real tool error string in opencode log handler (#736)
opencode's `ToolStateError` carries the failure reason on `state.error`, not `state.output`. our log handler was reading `state.output` and falling back to `(no error message)`, so every tool failure logged a useless line. type the state as a discriminated union (mirrors @opencode-ai/sdk) so the field misread becomes a compile error. operator-facing only: the model already received the real error via opencode's tool-result envelope (verified by running webfetch against a known-404 URL — model reported "Error: Request failed with status code: 404" verbatim). closes #662 |
||
|
Colin McDonnell
|
1f4c3031be |
ci: filter test matrices by per-test coverage globs (#730)
* ci: filter test matrices by per-test coverage globs to cut LLM spend every test in `crossagent/`, `agnostic/`, and every provider entry now declares a `coverage: string[]` of repo-relative globs. the new `changes` job runs `paths-filter` for a docs-only short-circuit, then pipes the changed-file list into `action/test/matrix.ts`, which intersects each entry's coverage against the diff and emits filtered `agents`, `agnostic`, `flagships`, and `aliases` matrices. main pushes and `workflow_dispatch` set `FULL=1` to run everything as a stale-glob safety net. retires `changed-agents.sh` and the `MODE=flagships` branch in `list-aliases.ts` in favor of one consistent model. * ci(matrix): switch test discovery to dep-free static parsing the GHA `changes` job has no `node_modules` installed. the previous dynamic-import path pulled the test files transitively through `utils.ts` -> `agents/index.ts` -> `@actions/core`, which exploded with ERR_MODULE_NOT_FOUND. parse the test files via regex instead so matrix.ts stays zero-dep — the chain (matrix -> coverage / providers / list-aliases / models) imports only node builtins and relative TS files. * ci(matrix): address PR #730 review feedback - drop dangling `action/mcp/toolFiltering.ts` glob from `nobash`, `restricted`, `tokenExfil` (file doesn't exist; `.test.ts` does, but the runtime tooling lives in `mcp/shell.ts` and `agents/{claude,opencode}.ts`, both already covered). - drop unused `coverageForProvider` export and its `byName` map from `providers.ts` (matrix.ts builds its own lookup inline). - derive the active agent list from `agents/index.ts` via the same dep-free regex tactic as `parseTestFile` instead of hardcoding `["claude", "opencode"]` — adding a new harness file now wires it into the dynamic matrix automatically. - treat `coverage: []` as `coverage: undefined` in `shouldRun` so an accidentally-empty array doesn't silently skip CI on every PR. - add `action/utils/activity.ts` and `action/mcp/selectMode.ts` to the `timeout` test's coverage — the activity-timeout enforcement path was the original reason the test exists. - ungate the `root` job (lint/format/typecheck/vitest). it's a required status check on `main`, so gating it on `code == 'true'` would make docs-only PRs unmergeable (skipped jobs don't satisfy required-check rules). the real LLM savings come from skipping the four matrices, not from skipping `root`. - harden the four matrix-job `if:` guards from `outputs.matrix && ...` to `outputs.matrix != '' && ...` — explicit > implicit short-circuit. - document `expandBraces`'s flat-only support so a future author isn't surprised by `{a,{b,c}}` not expanding. - fix awkward sentence in `wiki/action-tests.md` "CI Cost Filtering". |
||
|
Colin McDonnell
|
4ad649ebb9 |
action: extend shallow-unreachable deepen-retry to checkout_pr fetches (#734)
extracts the deepen-retry helper from `GitFetchTool` into shared `$gitFetchWithDeepen` and applies it to every fetch in `checkoutPrBranch` (baseRef, pull/N/head, before_sha temp branch). on shallow clones with deep PR ancestry — the failure mode behind ~10 of 51 `heuristic:very-slow` runs in 24h on `remotion-dev/remotion` — the baseRef fetch was throwing `Could not read <sha>` to the agent before the compare-api deepen block could run. agents then burned 10+ minutes retrying `checkout_pr` and falling back to ad-hoc shell `git fetch --deepen` workarounds. also splits the analyzer's `heuristic:git-error-recovered` into `heuristic:git-shallow-unreachable` and `heuristic:git-shallow-lock` buckets so future audits surface this without manual log-grep. closes #656. |
||
|
Colin McDonnell
|
2960d51493 |
shell tool: cap output at 5K chars and spill overflow to tempfile (#732)
unbounded shell tool output blows the agent's context window on commands
that dump big logs (test runners, build tools, grep on large trees). cap
the inline body at 5000 chars; on overflow, persist the full output to
${PULLFROG_TEMP_DIR}/shell-<id>.log and return the tail prefixed with a
sentinel pointing at the saved path. agents re-read the tempfile with
cat/tail/grep when they need more.
|
||
|
Colin McDonnell
|
b6df2860c3 | action: bump to 0.1.7 v0.1.7 | ||
|
Colin McDonnell
|
d495f0b984 |
surface BYOK failures + chronic-failures card + WorkflowRunStatus mirrors GitHub conclusions (#722)
- Migrates `WorkflowRunStatus` from `running | completed | cancelled` to a 9-state mirror of `workflow_run.conclusion`. Backfill: old `completed → success`, `cancelled → failure`. New rows write `hook.workflow_run.conclusion` verbatim via `statusFromConclusion`. - Adds Discord links to `formatApiKeyErrorSummary` (both missing-key and 401 invalid-key shapes). - Repo console: `<ChronicFailuresCard>` fires when the last 3 terminal-state runs are all `failure`. Pure DB read; latest-run button hidden for pre-dispatch failures (`runId: null`). - `StatusIcon` distinguishes `cancelled` (gray X, intentional stop) from `failure` (red X) so the visual matches the chronic-card threshold. - Pre-dispatch failures (workflow lookup miss, dispatch API error) write `failure` instead of `cancelled` so they feed the card. - Cascade: every `status: "completed"` filter in billing routes / cron / cohort queries / analyzer becomes `status: "success"`. Verified end-to-end on `pullfrog/preview-722-failure-surfaces` — Better Stack logs confirm webhooks reached the preview deploy and all three e2e runs got `marked as failure (conclusion=failure)` via the new mapper. Closes #679, #702. |
||
|
Colin McDonnell
|
206c11fe7c |
review: drop misleading 'with the same arguments' from diff-coverage nudge
agent is free to refine review body/comments on retry — there's no enforcement that the second call matches the first, and if reading the nudged region surfaces a new finding the agent should add it. |
||
|
Colin McDonnell
|
7414c1e9ca |
review: clarify diff-coverage nudge gives explicit license to skip generated artifacts
the one-time pre-flight nudge said "optionally read" but never told the agent it's free to retry without reading when every unread region is generated (lockfiles, codegen, snapshots, migration metadata). audit #677 surfaced ~21 runs/24h burning an extra model turn re-reading drizzle snapshots, pnpm-lock, and *.gen.ts files purely to satisfy the gate. mode prompts only mention generated content in the "skip self-review entirely" path, not the "in-progress substantive review" path, so the in-the-moment error message was the gap. behavior unchanged for legitimately-unread source regions. |
||
|
Colin McDonnell
|
8f9208bd3f |
feat: Amazon Bedrock support via routing slug (#720)
* add Amazon Bedrock as a routing slug introduces a single `bedrock/byok` catalog entry that the harness translates to the appropriate Bedrock model ID at run time via `BEDROCK_MODEL_ID`. routes Anthropic IDs through claude-code (with `CLAUDE_CODE_USE_BEDROCK=1`) and everything else through opencode's `amazon-bedrock` provider — keeps the catalog flat for an audience that needs version pinning rather than aliasing. accepts either `AWS_BEARER_TOKEN_BEDROCK` or `AWS_ACCESS_KEY_ID` + `AWS_SECRET_ACCESS_KEY` for auth; both validated alongside `AWS_REGION` and `BEDROCK_MODEL_ID` in `validateAgentApiKey`. catalog drift tests, the bumps cron, and per-alias smoke scripts all skip routing slugs since there's no fixed `resolve` to validate. docs/bedrock.mdx walks through setup; wiki/model-resolution.md has a section explaining why bedrock breaks the usual alias pattern. closes pullfrog/pullfrog#40 * ci: add bedrock env vars to test workflows mirrors the new bedrock provider's required env vars (AWS_BEARER_TOKEN_BEDROCK inherited from org secret + AWS_REGION + BEDROCK_MODEL_ID hardcoded) into both .github/workflows/test.yml files so the ci.test "env vars cover all provider API keys" assertion passes. * docs(bedrock): clearer setup flow + screenshot of model selector restructures the setup section into three concrete steps in execution order: select Bedrock from the dropdown, store the bearer token as a secret (Pullfrog or GitHub — links to keys.mdx for the trade-off), then add region + model id directly in pullfrog.yml since neither is sensitive. enable-model-access in the Bedrock console moved to step 4 (only required once per model and only when AWS rejects the call, not blocking on first run). adds a screenshot of the console model selector with Amazon Bedrock selected so readers can recognize the UI state they're aiming for. * fix(bedrock): tolerate raw Bedrock model IDs in validateAgentApiKey main.ts passes the resolved model into validateAgentApiKey (`payload.proxyModel ?? resolvedModel ?? payload.model`). For Bedrock, `resolveModel` translates `bedrock/byok` into the raw AWS model ID (e.g. `us.anthropic.claude-opus-4-6-v1`), which has no `/` and so trips parseModel inside getModelEnvVars. Detect the no-slash case and re-run the bedrock setup check (auth + region; BEDROCK_MODEL_ID is already enforced upstream by resolveModel). Caught by PR #720 e2e dispatch on pullfrog/preview-720-bedrock — "invalid model slug 'us.anthropic.claude-opus-4-6-v1' — expected 'provider/model'". Two regression tests cover the raw-ID path. * fix(bedrock): always prepend amazon-bedrock/ prefix when bedrock-routed opencode.ts was gating the prefix-injection on `!isBedrockAnthropicId(rawModel)`, on the theory that Anthropic Bedrock IDs always go through claude-code. But `PULLFROG_AGENT=opencode` is a documented escape hatch — when it forces opencode for an Anthropic Bedrock model, the prefix still has to be added or opencode fails with 'Model not found: <modelId>/.'. The Anthropic-vs-other discriminant only belongs in resolveAgent. Once an agent is selected, it should consistently honor the bedrock route. Caught by the PULLFROG_AGENT=opencode + Opus 4.6 e2e on pullfrog/preview-720-bedrock — run 25823437606. * ui+docs(bedrock): bespoke setup callout + clearer docs UI: - BedrockSetupCallout in components/AgentSettings.tsx covers both the Model costs section and the onboarding card. Detects bedrock via resolveDisplayAlias().routing === "bedrock", shows a dedicated message ("store AWS_BEARER_TOKEN_BEDROCK as a secret, then put AWS_REGION + BEDROCK_MODEL_ID directly in pullfrog.yml") + link to the setup guide. Replaces the generic "X, Y, or Z is required" prompt that misrepresented the three values as three separate secrets to add (and used the wrong "or" connector for what's actually an AND). - OnboardingCard re-uses the same callout with the gradient-card variant. Docs: - Drop the obsolete "Enable model access" step. AWS retired the manual enrollment page; foundation models auto-enable on first invocation. Anthropic models still need a one-time use-case form for first-time users — surfaced under the AccessDenied troubleshooting entry. - Drop the "Testing a different model in one run" PULLFROG_MODEL note. It introduced the secrets-vs-vars distinction we want to keep out of the bedrock setup story. - Step 3 already recommends hardcoding region + model id in pullfrog.yml. Workflow template: - The default pullfrog.yml customers receive (utils/github/pullfrog.yml.ts) now references AWS_BEARER_TOKEN_BEDROCK from secrets but inlines AWS_REGION and BEDROCK_MODEL_ID as plain values. Matches the docs. * fix(bedrock): three review-caught edges in routing + UI copy Addresses three real issues from PR #720 review: 1. agent.ts: PULLFROG_MODEL=bedrock/byok no longer leaks the literal sentinel "bedrock" downstream. resolveCliModel returns the alias's resolve field verbatim, which for routing entries IS the sentinel. Refactored both the env-override and slug-lookup paths through a shared resolveSlug() that recognizes routing aliases and defers to their backing env var (BEDROCK_MODEL_ID). 2. models.ts: isBedrockAnthropicId() now anchors on a discrete dot/slash/colon-segment match (case-insensitive) instead of a substring contains. The substring check was fragile in both directions for inference-profile ARNs (BEDROCK_MODEL_ID accepts ARNs per AWS docs) — a non-Anthropic profile whose user-chosen name contained "anthropic" would mis-route to claude-code, and an Anthropic profile whose name omitted it would miss CLAUDE_CODE_USE_BEDROCK=1. 3. AgentSettings.tsx: BedrockSetupCallout's configured-state copy showed "AWS_BEARER_TOKEN_BEDROCK configured" even when the user satisfied the gate via AWS_ACCESS_KEY_ID + AWS_SECRET_ACCESS_KEY, gaslighting access-key users about a secret they never set. Detect which auth method is actually present and name the right secret(s) in the success message. Regression tests in models.test.ts (5 new isBedrockAnthropicId cases including positive and negative ARN forms) and agent.test.ts (2 new PULLFROG_MODEL=bedrock/byok cases). 171/171 action tests pass. * yml template: add commented AWS access-key alternative for Bedrock auth Mirrors the IAM access-key path verified end-to-end on PR #720 e2e run 25830764987. Bearer token stays as the primary nudge; the access-key pair is the fallback for users who can't mint Bedrock API keys. * yml template: drop redundant 'or, alternatively' annotation * ui+docs(bedrock): rewrite callout copy + refresh screenshot Reframes the BedrockSetupCallout away from generic BYOK language to a Bedrock-specific message: leads with "Amazon Bedrock is configured entirely via environment variables", lists all four (auth, region, model id), and ends with the requested CTA sentence ("click below to learn more about Bedrock support in Pullfrog"). Promotes the "Bedrock setup guide" docs link from an inline anchor to a prominent button (always visible, regardless of auth state). The "Add AWS_BEARER_TOKEN_BEDROCK" affordance is now a secondary chip shown only when no auth secret is configured. Refreshes docs/images/model-selector-bedrock.png to capture the new callout — the prior screenshot still showed the old generic "BYOK / X, Y, or Z required" wording. |
||
|
Colin McDonnell
|
1a9d3c1f82 |
fix bootstrap ETARGET when customer has npm min-release-age policy (#725)
* fix bootstrap ETARGET when customer has npm min-release-age policy set npm_config_min_release_age=0 in the action runtime env so `npx --yes pullfrog@<spec>` doesn't get rejected by a customer-side release-age gate (npm 11.5+'s min-release-age / pnpm's minimumReleaseAge). env vars beat .npmrc in npm config precedence, so this neutralises the policy regardless of where it's defined. pullfrog's npm version is server-stamped from a SHA-pinned action ref customers already vet at the action layer — it isn't a customer-vetted dep, so the release-age policy is the wrong affordance for our bootstrap and would otherwise hard-fail every run while the latest publish ages into the customer's window. closes #713 * also cover pnpm's minimumReleaseAge key for corepack fallback path * correct pnpm env var (pnpm v11+ uses pnpm_config_*, not npm_config_*) the prior commit set `npm_config_minimum_release_age=0` to cover the pnpm corepack-dlx fallback path, but pnpm v11+ only reads env vars prefixed `pnpm_config_*` / `PNPM_CONFIG_*` (the v10→v11 migration explicitly renamed the prefix). swap to the correct env var so the fallback path actually neutralises pnpm's `minimumReleaseAge`. also tighten the comment block, and add an AGENTS.md rule reminding us to fetch top-level reviews AND inline review comments together — they live on different endpoints and the inline set is easy to miss with `gh pr view --json reviews,comments` alone. * add scripts/pr-reviews.ts for one-shot review evaluation dumps top-level reviews + inline review threads (with resolved/outdated state) + PR-level conversation in a single GraphQL round trip, so agents don't miss inline-comment feedback. fixes the trap where `gh pr view --json reviews,comments` silently omits the inline `pulls/{n}/comments` set. borrows `gh auth token` so no env vars are required. registered in `wiki/scripts.md`; AGENTS.md rule updated to point at the script instead of the two-step gh-CLI workaround. * pr-reviews: dump raw JSON for jq piping |
||
|
Colin McDonnell
|
951745ec89 | disable stop hook (runtime + dashboard) (#727) | ||
|
Colin McDonnell
|
56793d4a81 |
claude: prefer non-JSON stdout over NDJSON tail in exit-1 fallback (#643) (#726)
Claude CLI under CLAUDE_CODE_OAUTH_TOKEN exits 1 without setting `is_error` when the OAuth subscription's quota is exhausted. The existing fallback chain (`lastResultError || stderr || tailLines(stdout)`) had nothing structured to grab and dumped ~2KB of `system/init` NDJSON into the progress comment, hiding the actionable quota notice the CLI had already printed as plain text. Capture non-JSON stdout lines into a 20-line ring buffer (mirroring the existing `recentStderr` pattern) and prefer it over the raw NDJSON tail. Generic — no regex on bubble text — so any human-readable line the CLI emits surfaces instead of the event stream. Also adds a `failure:claude-oauth-quota` bucket to `analyze-logs.ts`, ordered before the SIGTERM check so the NDJSON tail's `cancelled` / `cancel_url` substrings (from learnings content) stop shadowing it. |
||
|
Colin McDonnell
|
d857e06731 |
postrun: tighten unsubmitted-review gate to require create_pull_request_review for Review mode (#724)
The gate at `getUnsubmittedReview` accepted `toolState.finalSummaryWritten` as a valid Review exit, contradicting the post-failure error message which already says Review's only valid exit is `create_pull_request_review`. This let any caller that flipped `finalSummaryWritten` — including a `task`-dispatched `reviewfrog` subagent calling `pullfrog_report_progress` in violation of its prose-only read-only contract — silence the gate even when the orchestrator never submitted a review. Split per-mode: Review requires `toolState.review`, IncrementalReview keeps the existing `||` (its post-failure message explicitly accepts `report_progress` as a "no review warranted" exit). Test split mirrors the new semantics. closes #648 |
||
|
Colin McDonnell
|
b9f0938405 |
mcp: restore operational guidance dropped in #723
#723's revision pass cut four substantive strings along with the negative anchors. those strings address real, audit-observed failure modes and the positive examples don't carry them. restored: - push_branch: "if the response reports a timeout, the underlying push may have actually succeeded — verify with git log origin/<branch> before retrying" (was on the tool description) - create_pull_request_review commit_id .describe(): "must be the FULL 40-character SHA — abbreviated SHAs are rejected by GitHub with 422" - create_pull_request_review comments[].line .describe(): "must sit inside a `@@` hunk... dropped entries are reported under droppedComments in the response" - create_pull_request_review comments[].start_line .describe(): "both start_line and line must sit inside the same @@ hunk" also: get_commit_info example used a 31-character SHA (non-standard truncation). swapped to a 7-char short form, which is what git log --oneline emits and what agents see in practice. note that this tool accepts either full or abbreviated, unlike create_pull_request_review which requires full. |
||
|
Colin McDonnell
|
b8ac42e875 |
mcp: embed example calls in top-level tool descriptions (#723)
* mcp: embed example calls in top-level tool descriptions
agents (esp. claude sonnet) hallucinate param names from training-data
priors — `pr_number` instead of `pull_number`, `summary` instead of
`body`, full subcommand strings jammed into `git({command})` like it
were `shell({command})`. each error burns a tool round-trip plus a
follow-up ToolSearch, ~40+ events / 24h, no observable recovery cost
to us but visible to users in agent logs.
cheapest fix: add a sample formatted function call to every affected
tool's top-level description. example anchors are more reliable than
schema descriptions alone because the model treats descriptions as
narrative but call examples as canonical structure. for `git` and
`shell` (whose `command` fields collide), include explicit
counter-examples disambiguating which tool owns which shape.
no schema aliases / coercion yet — try the cheap thing first; if the
next audit window still shows the same hallucination rate, layer
aliases on top per #585's recommendation.
closes #585, closes #701
* mcp: drop negative anchors from tool descriptions
negation is a footgun in tool descriptions — telling the model "NOT
pr_number" makes pr_number more salient, not less. let the positive
example carry the schema and trust the model to read it.
removes:
- "the parameter is pull_number (a number), NOT pr_number" and
similar across checkout_pr, get_pull_request, list_pull_request_reviews,
get_review_comments, create_pull_request_review
- "NOT summary, message, or content" on report_progress
- "WRONG: git({ command: 'log --oneline' })" counter-example on git
- redundant param-type restatements after the example (e.g. "depth is a
number, not a string" on git_fetch, "description is required" on shell)
keeps a single positive example per tool. for tools with multiple call
shapes (git, git_fetch, push_branch), two positive examples instead of
one + a counter-example.
|
||
|
Colin McDonnell
|
868576a474 |
audit: format byok auth errors actionably + tighten audit prompt
- `action/utils/apiKeys.ts`: rewrite the missing-key body as Markdown with linked CTAs (repo secrets / model settings / docs). add `isApiKeyAuthError` + `formatApiKeyErrorSummary` covering both shapes: missing key (#679) and revoked/invalid 401 key (#702). - `action/main.ts`: reclassify in the result-failure branch and the catch block so the PR progress comment surfaces the actionable CTA instead of the raw `Invalid API key · Fix external API key` / numbered-list dump. - `scripts/analyze-logs.ts`: split `failure:user-misconfig` into `:no-key` and `:invalid-key` so both buckets are visible separately and the audit can ignore them as user-correctable. - `.github/workflows/run-audit.yml`: add three explicit prompt rules — cross-customer signal required (≥3 distinct accounts; single-customer concentration is not enough), recovered failures are not actionable, user misconfig is out of scope. closes the loop on #679 / #702 being filed in the first place. |
||
|
Colin McDonnell
|
b2b1e588e7 |
biome: exclude .scripts/ — gitignored operator scratchpad
Mirrors the gitignore. Same shape as the existing !**/logs / !**/.logs / !.worktrees exclusions in files.includes. Matches the upstream .gitignore policy for the .scripts/ directory. Without this, .scripts/ scripts (`.scripts/kyle-*.ts`, `.scripts/check-comment.ts`, etc.) get scanned by `pnpm lint` and `pnpm format` from the repo root and routinely fail husky pre-push even though they're explicitly intended to be local-only / personal. The companion to .gitignore — both are operator-owned scratchpads; neither participates in repo-wide hygiene. |
||
|
Colin McDonnell
|
5caeb75344 |
review: 0-or-2+ lens rule, parallel-or-bust, downshifted subagent models (#710)
* review: 0-or-2+ lens rule, parallel-or-bust, downshifted subagent models
PR review wall-time was dominated by two failure modes: orchestrator
serial-dispatching subagents (despite prompt asking for parallel) and
running every lens on the same Opus tier as the orchestrator. Sample of
recent runs showed 25-60min reviews on small PRs, with 8-10min idle
gaps between subagent dispatches.
Three changes:
1. `action/modes.ts` — replace the soft "1 trivial / 2-3 typical /
4-5 high-stakes" lens calibration with a binary 0-or-2+ rule. Default
is 0 lenses (orchestrator handles review solo with optional cheap
tracerfrog dispatches). 2+ parallel lenses only fire for substantive
PRs (>5 files AND >200 lines) or high-stakes-subsystem touches. Never
exactly one. Both Review and IncrementalReview prompts get loud
ALL-CAPS framing on parallel dispatch — emit ALL Task tool_use blocks
in a single assistant turn before reading any result. Drop the
"do NOT lens-review the diff yourself" advice; orchestrator pulls
context aggressively, in parallel with the lens fan-out.
2. New `tracerfrog` subagent for mechanical code tracing ("where is X
used / who calls Y / what depends on Z"). Pure read+grep+report with
no judgment — orchestrator can dispatch many tracers cheaply in
parallel. Defined in `action/agents/reviewer.ts`. Wired into both
claude.ts (`--agents` JSON) and opencode.ts (`agent` config block).
3. Per-subagent model downshifts via `deriveSubagentModels`:
- Anthropic: reviewfrog → Sonnet, tracerfrog → Haiku
- OpenAI: both → gpt-5.4-mini
- other providers (xai, deepseek, gemini, etc.): inherit (no
standard tier triplet to downshift to)
Claude Code path always runs Anthropic so the downshift is hardcoded
inline in claude.ts. OpenCode uses the helper since orchestrator
provider varies.
Both runtimes' subagent-definition formats verified directly against
their source: `--agents` JSON `model` field (claude-code's
`AgentJsonSchema` accepts model+effort+maxTurns+more) and OpenCode's
`agent.{name}.model` config field (parsed via Provider.parseModel,
applied per-task in tool/task.ts line 92). Parallel dispatch is
infra-supported in both — only the orchestrator model's tool_use
emission pattern was the bottleneck.
Tests: subagentModels.test.ts (14 tests covering provider matrix),
subagentRegistration.test.ts (6 source-asserts catching shape
regressions in buildAgentsJson / buildReviewerAgentConfig).
* subagentModels: add openrouter routes (proxy/router mode)
Initial helper missed the openrouter prefix used by Pullfrog's router
proxy. preview-710 e2e showed the OpenCode + openrouter path receiving
no downshift — orchestrator and lenses both ran on opus-4.7 because
'openrouter/anthropic/claude-opus-4.7' didn't match any of the
anthropic/openai prefixes the helper checked.
Add explicit branches for 'openrouter/anthropic/...' (uses dot notation:
claude-sonnet-4.6 / claude-haiku-4.5) and 'openrouter/openai/...'
(gpt-5.4-mini for both reviewer and tracer). Same opus->sonnet,
sonnet->keep-but-haiku-tracer, haiku->no-op semantics as the direct
anthropic path.
* opencode: log resolved subagent models at startup
So we can verify per-subagent model overrides actually take effect at
runtime. Prints once per run alongside the existing model/effort log
lines.
* drop tracerfrog: keep reviewfrog only, LSP-powered tracer planned later
Removes the cheap-haiku-tracer subagent (TRACER_AGENT_NAME +
TRACER_SYSTEM_PROMPT, registrations in claude.ts/opencode.ts, dispatch
guidance in modes.ts). The mechanical-tracing use case will be served
better by an LSP-powered tool than by a separately-prompted subagent.
deriveSubagentModels collapses to a single { reviewer } shape; the
reviewfrog-on-Sonnet downshift stays. Same source-assert + provider-
matrix tests, minus the tracer-specific cases.
modes.ts wording: drop the 'subagent type cheat sheet' bullet, drop
the parenthetical 'often better served by tracerfrog than reviewfrog'
on the impact lens, drop tracerfrog from the same-turn-context-pulling
hint. The 0-or-2+ rule and ALL-CAPS parallel emphasis are unchanged.
* subagentModels: broader downshift coverage (gpt-pro, gemini-pro, grok); drop gpt-mini target
Scanned every resolved orchestrator slug in action/models.ts against
models.dev pricing data. Identified five clear cases where the
orchestrator is meaningfully expensive AND has a cheaper sibling that
remains capable enough for review-style judgment work.
Changes:
- Anthropic: opus → sonnet (kept; -40%)
- OpenAI: gpt → gpt-5.4 (was: gpt-mini; -54% instead of -85% but
preserves review-quality judgment — gpt-mini was too dumb)
- OpenAI: gpt-pro → gpt (NEW; -93%, biggest single unlock —
gpt-5.5-pro is $30/Mtok in vs gpt-5.5 at $5)
- Google: gemini-pro → gemini-flash (NEW; -75%)
- xAI: grok-4.3 → grok-4-1-fast (NEW; -80%)
Every branch handles the three routes in use: direct provider slug,
opencode-vendored, and openrouter-proxied. Variants below the downshift
target (mini/nano/flash/fast/sonnet/haiku) inherit (no further drop).
Skipped:
- DeepSeek: v4-flash ($0.14/Mtok) is too far below review judgment
threshold; v4-pro orchestrator already cheap ($0.55 blended).
- Moonshot: kimi-k2-thinking would only save 32% and slug stability on
OpenRouter is uncertain; revisit if cost matters.
- o3: already mid-tier in OpenAI's reasoning family; no clean target.
* models: hoist subagent downshift into the registry, add hidden flag
The downshift relationship now lives next to each alias's resolve /
openRouterResolve as a sibling field. Two new ModelDef fields:
- subagentModel?: string — alias key (within same provider) of the
cheaper sibling reviewfrog should use as a lens-fanout subagent.
e.g. claude-opus → 'claude-sonnet'.
- hidden?: boolean — exclude from selectable lists (UI dropdown,
CLI init picker). Does NOT affect resolution; for that use
fallback. Used so internal-only subagent targets like openai/gpt-5.4
exist in the registry but never appear as a user-facing pick.
Wiring:
- anthropic.claude-opus → claude-sonnet (-40%)
- openai.gpt-pro → gpt (-93%, biggest unlock)
- openai.gpt → gpt-5.4 (-54%); gpt-5.4 added with hidden:true
- google.gemini-pro → gemini-flash (-75%)
- mirrored across opencode + openrouter providers (each provider
declares its own three-route data so the downshift declaration
is colocated with the rest of the alias definition).
deriveSubagentModels collapses from ~85 lines of prefix-matching to
a ~15-line registry reverse-lookup: find the alias whose resolve OR
openRouterResolve matches the orchestrator's spec, follow its
subagentModel pointer, return the matching field of the target alias.
Filter sites updated:
- components/ModelSelector.tsx: !a.fallback && !a.hidden
- action/commands/init.ts: same
Tests rewritten to exercise the registry through the public surface;
the matrix collapses to one assertion per (provider × route) pair.
* TEMP: log per-step cost+tokens for subagent model verification (PR #710)
* TEMP: also log SUBAGENT step_finish from bus envelope handler
* remove temporary per-step diagnostic logs (verification done)
Verified subagent model downshift takes effect end-to-end on the OpenCode
+ openrouter path. PR #8 in pullfrog/preview-710-review-perf dispatched
3 lenses (billing-subsystem / security / correctness) on the orchestrator's
opus-4.7 session, and per-subagent step_finish events showed actual cost
exactly matching Sonnet pricing rates (60% of what Opus would have cost):
session n actual if-Opus if-Sonnet match
T3VrUuF... 5 $0.2425 $0.4042 $0.2425 Sonnet ✓
93ZZR7E... 4 $0.2253 $0.3754 $0.2253 Sonnet ✓
Fb1Kr7b... 4 $0.2495 $0.4158 $0.2495 Sonnet ✓
The startup '» subagent models: reviewfrog=...' line stays — useful
permanent diagnostic showing the resolved subagent model per-run.
* TEMP: log per-event model from claude.ts assistant handler
* remove temporary per-event model log (claude.ts verification done)
Verified subagent model downshift takes effect end-to-end on the Claude
Code path. PR #9 in pullfrog/preview-710-review-perf dispatched 2 lenses
on an opus-4-7 orchestrator. Per-assistant-event model field from the
SDK's stream-json output, partitioned by parent_tool_use_id:
ORCH (parent_tool_use_id=null): 17 events all model=claude-opus-4-7
SUBAGENT lens:billing-subsystem: 17 events all model=claude-sonnet-4-6
SUBAGENT lens:security: 21 events all model=claude-sonnet-4-6
Zero leakage to opus from either subagent session. The per-subagent
'model' field in --agents JSON is honored by claude-code at the SDK
level, identical to the OpenCode path verified earlier.
* opencode: bump per-call output cap 5K → 16K to unblock large reviews
The 5K cap (added in #616 to lower OpenRouter upfront budget reservation
for low-wallet runs) was capping the entire response of a single LLM call,
not just the budget reservation. A single tool_use response — like a
`create_pull_request_review` with many inline comments — would truncate
mid-stream past 5K output tokens, leave the JSON unparseable, and the tool
would never actually invoke. We hit this on PR #710's verify-downshift PR:
review aggregated from 3 lenses had 11 inline comments + a long body,
truncated at out=5000 on every retry attempt, action exited with 'Review
mode finished without calling create_pull_request_review after 3 retry
attempts'.
Investigated whether OpenCode (or OpenAI/Anthropic/OpenRouter directly)
exposes a separate budget-reservation parameter that could stay small
while letting the response exceed it. They don't — `max_tokens` /
`max_completion_tokens` is the single value all four use for both the
upfront reservation and the hard output ceiling. No way to decouple them
at the API surface.
Bumped to 16K as a middle ground: 8× the prior cap (handles every review
shape we've observed plus headroom), still half of OpenCode's 32K default
so the wallet-burn benefit for low-balance accounts is preserved, just
smaller. For Opus 4.7 a typical ~50K-input call now reserves roughly
$0.65 instead of the prior $0.38.
Updated the constant comment to spell out the trade-off clearly so this
doesn't happen again.
* opencode: drop OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX override entirely
Verified the original rationale for the override is obsolete. From #616
the cap shrunk OpenRouter's per-call upfront budget reservation so a
single call's reservation wouldn't exceed the per-run key cap
(`ROUTER_PER_RUN_LIMIT_USD = 25`) and lock low-balance accounts out of
starting a run.
That per-run gate is gone. `app/api/proxy-token/route.ts` ~line 422
explicitly says: 'No upper cap (the old ROUTER_PER_RUN_LIMIT_USD = 25 is
gone). The natural ceiling is whatever the user has + their buffer.'
Router now mints keys with `keyLimitCents = balance + buffer` ($50 for
autoreload+card, $5 for card-only, $0 for no-card). A single call's
upfront reservation fits comfortably within that — no separate per-call
gate to fail past.
The cap had a real downside as a hard per-call output truncation. A
single `create_pull_request_review` tool_use with many inline comments
would truncate mid-stream past 5K output tokens, the JSON would be
unparseable, and the tool never invoked. Hit on PR #710's
verify-downshift PR.
Removing the override entirely; OpenCode falls back to its 32K default.
Left an explanatory note above the env-var assignment site so the next
person doesn't unknowingly re-add it.
|
||
|
David Blass
|
5518890b18 |
learnings: TOC + section taxonomy + 100k cap, hygiene rules, tool-quirk descriptions (#717)
* audit learnings: reshape reflection prompt + bake tool quirks into descriptions (#619) Cross-repo audit of the 48 repos with non-null learnings turned up two recurring failure modes: 1. ~25-30% of bullets across the most-active repos are pullfrog-tool quirks ("shell timeout is in milliseconds", "git args must be a JSON array", "create_pull_request_review drops out-of-hunk comments", "push_branch may report timeout when push succeeded", "checkout_pr shallow.lock retries", "commit_id needs full 40-char SHA"). These are universal across repos and should live in tool descriptions, not be rediscovered and stored 48 times. Tool descriptions now surface them. 2. Bullets are routinely 200-1000 chars (paragraph-length), and 12 of 48 repos are at the 10k cap. The reflection prompt now: caps bullets at ~240 chars (one specific fact), bans PR/review/commit/date-anchored facts that decay within weeks, bans tool-quirk learnings, and tells the agent that cap pressure means compress+prune existing bullets, not skip new findings. Co-authored-by: Cursor <cursoragent@cursor.com> * learnings: add server-generated TOC, fixed section taxonomy, raise cap to 100k (#707) Cap goes 10k → 100k. Reads stay bounded because the seeded file now opens with a server-generated table of contents listing every `## ` section's line range — agents read the TOC, then `read_file offset/limit` just the sections relevant to the current task instead of slurping the whole file. ## Section taxonomy (fixed) `## Build & test`, `## CI`, `## Conventions`, `## Architecture`, `## Gotchas`. Free-form `### ` sub-headings inside a section are fine. Pre-taxonomy free-text rows get wrapped in a `## Legacy` carve-out on first seed so they remain visible while the agent gradually re-curates them during reflection turns. ## Storage shape unchanged `Repo.learnings` still holds raw markdown (no schema migration). The TOC is a pure read-side affordance: prepended at seed time, stripped from the agent-edited file before persist. Markers `<!-- pullfrog-learnings-toc:* -->` delimit the strip region. Agent edits inside the markers are discarded. ## Round-trip semantics `seedLearningsFile` now returns `{ path, canonicalSeed }` where `canonicalSeed` is the post-TOC body — same shape `readLearningsFile` returns at end-of-run, so `persistLearnings` byte-compares them directly to skip the no-op PATCH. Empty-repo first runs end up with the section scaffold both as seed and as read-back, so untouched runs still short-circuit cleanly. ## Reflection prompt Adds explicit section-placement guidance (place each new bullet under the most relevant `## `; do NOT add new top-level headings; do NOT edit anything between the TOC markers). Carries forward the bullet hygiene from the previous commit: ≤240 chars per bullet, no pullfrog-tool quirks (those belong in tool descriptions), no PR/review/commit/date references. The "near cap" framing is replaced with "compress and prune within a section when it grows noisy" since the cap pressure that drove cramming is gone. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal round 1: line-anchored taxonomy detect, partial-merge, line-boundary truncation, scaffold-empty UI Multi-lens review of the TOC + taxonomy diff surfaced a cluster of correctness and operational bugs. Fixes: - `hasAnyTaxonomyHeading` used `String.includes("## X")` which false-positives on `### X` (the `## ` substring sits inside `### `), prose containing `## CI`, fenced code documenting markdown, etc. Replaced with a line-anchored predicate that reuses `parseHeadings` so detection and TOC construction stay consistent. - The "any heading present → pass through verbatim" rule meant a body with one taxonomy heading would seed without the other four. Worse, requiring all five would flip a body back into Legacy when the agent legitimately pruned a section to empty. New `partial` kind: keep existing content in place, append missing sections in canonical order so the agent always has the full scaffold without losing pruning intent. - `stripLearningsToc` collapsed `\n{3,}` globally; `canonicalSeed` doesn't, so an untouched body with intentional triple-newline spacing would compare unequal and burn a spurious LearningsRevision row each run. Drop the global collapse — only the leading newlines that the strip itself introduces are normalized. - 100k truncation via `slice(0, 100_000)` could cut mid-line, breaking `parseHeadings` (whole-line `^## `) on the next seed and flipping a cut body back into Legacy. New `truncateAtLineBoundary` cuts at the last newline before the cap. - `LearningsSection.tsx` rendered a scaffold-only body as "has learnings" instead of the empty placeholder. Added a `hasOnlyEmptyScaffold` guard so the console behaves the same as pre-PR for the empty case. - Seed log line distinguishes `kind=structured/partial/legacy-wrapped/ empty` instead of `existing=yes/no`, so operators can spot legacy migration activity in logs. - New tests cover: substring false-positive (`### Build & test`, in-prose mentions), partial-taxonomy merge (no Legacy wrap), full-taxonomy structured pass-through, last-newline truncation, triple-newline preservation. Deferred (documented in PR body): deploy-ordering footgun (action before API), rollback for rows >10k, Gemini sanitizer dropping `description` on `anyOf` branches, reflection-on-failed-runs. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal r2: hard-truncate fallback when line boundary discards >4k Round-2 review caught a regression in `truncateAtLineBoundary`: when the only newline within the first 100k chars sits near the start (e.g. one heading + 100k+ char single line — pathological pasted log dumps), the line-boundary cut discards almost all of the body. losing one partial line is preferable to losing kilobytes; threshold the fallback at 4k. Co-authored-by: Cursor <cursoragent@cursor.com> * move TOC out of file: prompt-side rendering, server-parsed headings drops the in-file TOC + fixed taxonomy in favor of: - file on disk = verbatim Repo.learnings (no markers, no scaffold) - server parses headings (mdast-util-from-markdown) at run-context time and returns them as RepoSettings.learningsHeadings - action renders heading TOC into the LEARNINGS prompt section as parenthesized line ranges like `Build & test (L1-L42)` with hierarchy via 2-space indent off the shallowest depth - reflection prompt teaches agent-curated structure with a soft 300-line per-section cap and explicit guidance to restructure flat legacy lists cuts 8 helpers (ensureSections, stripLearningsToc, assembleFile, buildTocBlock, parseHeadings, buildSectionScaffold, hasAnyTaxonomyHeading, LEARNINGS_SECTIONS) and the canonicalSeed round-trip dance. action seedLearningsFile is now { path } only; main.ts byte-compares the trimmed read-back against (current ?? "").trim() to gate the persist PATCH. truncateAtLineBoundary kept for safety. new tests: - test/learningsToc.test.ts (11 parser cases incl. fenced-code, blockquote, arbitrary h1-h6 nesting, startLine-points-at-heading invariant) - action/utils/learningsTocRender.test.ts (7 renderer cases) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Colin McDonnell <colinmcd94@gmail.com> |
||
|
Colin McDonnell
|
d04c1ca3da | action: bump to 0.1.6 v0.1.6 | ||
|
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 |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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). |
||
|
Colin McDonnell
|
1dc53043a6 | chore: bump action to 0.1.5 v0.1.5 | ||
|
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.
|
||
|
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. |
||
|
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). |
||
|
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 |
||
|
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 |
||
|
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.
|
||
|
Colin McDonnell
|
e20b4d5515 | action: bump to 0.1.4 v0.1.4 | ||
|
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> |
||
|
Colin McDonnell
|
e4d0fc7e3d | biome: ignore .logs/ (was matching only logs/) v0.1.3 | ||
|
Colin McDonnell
|
a4a5010441 |
gemini-3: default thinkingLevel to medium + restrict eager prep to frozen install (#663)
* gemini-3: default thinkingLevel to medium + don't `npm ci` without a lockfile upstream opencode hardcodes `thinkingLevel: "high"` for every gemini-3 model on the direct google SDK (see `packages/opencode/src/provider/transform.ts` `options()`). that added 30-60s of pre-tool-call TTFT and 5-46s of post-tool jabber per turn, which is overkill for the tool-routing decisions that dominate agentic loops — and the variance caused the `providers-live (google/gemini-pro)` smoke job to time out at 4 minutes (see job 75405504847 on run 25684766415). three changes: - inject `provider.google.models.<api-id>.options.thinkingConfig.thinkingLevel = "medium"` for the two curated gemini-3 slugs in `buildSecurityConfig`. deep-merges over the upstream default; explicit `--variant high` / user opencode config still wins. flash stays at medium too — low-effort flash is visibly worse and the latency win isn't meaningful (flash is already fast). - bump the `providers-live` harness step from 4 → 6 minutes. the job-level 8-minute cap stays as the upper bound, but gemini's intrinsic TTFT variance was eating most of the 4-minute slack on its own. - in `installNodeDependencies`, pick `frozen` only when a lockfile was actually detected. previously a package.json-only repo (like the smoke fixture's `pullfrog/test-repo`) always triggered `npm ci` and emitted a noisy `EUSAGE` error before falling through. * prep: skip eager install when neither lockfile nor `packageManager` field present the previous commit changed the no-lockfile path from `npm ci` (always errored `EUSAGE`, never wrote any artifact) to a successful `npm install`, which had an unintended side effect: it generated `package-lock.json` in the working tree, tripping the post-run dirty-tree gate. the agent then committed the lockfile and opened a real PR — and in the openai/gpt smoke run on PR #663, the agent overwrote the `SMOKE TEST PASSED` output with the PR URL, failing the smoke validator. a repo with `package.json` but no lockfile and no `packageManager` field has not committed dependency state. eagerly installing produces state the repo doesn't track, which is the dirty-tree problem above. skip the eager install entirely in that case; the agent can opt in via `await_dependency_installation` when it actually needs deps. repos with a lockfile or a `packageManager` field keep the existing frozen-install behavior unchanged. * post-run: suppress dirty-tree gate in non-committing modes (Review / IncrementalReview / Plan) the dirty-tree post-run gate currently fires for every mode and tells the agent to commit and push whatever is in the working tree. that's wrong for modes that complete by submitting a review (`Review` / `IncrementalReview`) or posting a Plan comment (`Plan`) — those modes never touch files as part of their contract, so any tree dirt at end-of-run is incidental tool noise on an ephemeral worktree. nudging the agent to commit it can produce a spurious PR, as seen in the openai/gpt smoke run on PR #663 where a stray `package-lock.json` from `npm install` led the agent to open pullfrog/test-repo#32 and overwrite the smoke output. introduce `NON_COMMITTING_MODES` in `action/modes.ts` and consult it in `collectPostRunIssues`. when the selected mode is read-only, log the suppression for visibility but skip populating `issues.dirtyTree`. modes that legitimately commit (`Build`, `AddressReviews`, `Fix`, `ResolveConflicts`, `Task`) keep the existing nudge. * prep: restore eager frozen-install, drop non-frozen fallback eager dependency prep is non-mutating by contract — it runs before the agent starts and any artifact it leaves in the tree (e.g. a generated `package-lock.json`) trips the dirty-tree post-run gate and can lead the agent to open a spurious PR (seen on the openai/gpt smoke run earlier in this PR). revert the previous skip-when-no-lockfile branch: that was the wrong layer to enforce the invariant. instead, run `frozen` (`npm ci` / `pnpm install --frozen-lockfile` / etc.) unconditionally and drop the `|| install` fallback that could silently mutate the tree when `frozen` is missing. frozen commands fail cleanly without writing artifacts when there's no lockfile, which is exactly the safety contract we want. repos that need a real install must opt in explicitly via a `setup` lifecycle hook. * review nits: single getGitStatus call, tighten gemini-3 override scope comment addresses two inline nits from the PR review: - `collectPostRunIssues` was calling `getGitStatus()` (spawns `git status --porcelain`) in both branches of the mode check. lift the call above the conditional and branch on the result; same behavior, one git invocation. - the JSDoc on `GEMINI_3_DIRECT_API_IDS` said the override applies "across the board," but the constant only covers the two curated slugs in `action/models.ts`. tighten the wording to call out that other gemini-3 ids in models.dev keep the upstream "high" default. skipped the bot's yarn-1 concern after reading yarn 1's `install.js`: `bailout()` (lines 461-465) throws `frozenLockfileError` when `frozenLockfile && (!lockfileClean || missingPatterns.length > 0)`, which fires before `linker.init()` writes node_modules or runs lifecycle scripts. the existing comment's claim that frozen commands fail without artifacts holds for yarn 1 too. |