902 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
Colin McDonnell	159389fad2	fix(mcp): sanitize for gemini when model is unresolved (#697) ... * fix(mcp): sanitize for gemini when model is unresolved isGeminiRouted() previously required the effective model string to contain "gemini" — but when payload.model="auto" (or any unresolved slug) reaches addTools(), `effective` is the literal "auto", which doesn't match. opencode then auto-selects gemini *after* the MCP server has registered raw arktype schemas, and every tool turn dies on `function_declarations[*].properties[*].any_of[*].enum: only allowed for STRING type`. widen the gate: any unresolved specifier (undefined / "auto" / a slug without a `provider/` prefix) is treated as gemini-routed and sanitized. the transforms are universally compatible normalizations so the false-positive cost is negligible. tighten case 3 to preserve `description` so the only lossy path no longer drops operator-facing context. fixes #676. * revert case-3 description preservation per pullfrog review on #697: keeping `description` as a peer of `anyOf`/`oneOf` directly contradicts the file's own header (lines 19-21) and the upstream opencode #14659 rationale that gates this sanitizer — gemini requires anyOf to be the ONLY field on a schema node, sibling keywords trigger `anyOf must be the only field in a schema node`. the change was speculative scope creep with no evidence, and would silently re-introduce a different gemini failure for any future schema using `.describe().or(...)`. the bug fix for #676 doesn't need it (arktype doesn't emit non-collapsible anyOf for current tool schemas).	2026-05-13 02:31:59 +00:00
Colin McDonnell	43bb14bf87	action: strip Content-Type on body-less apiFetch requests (#692) (#694) ... * action: strip Content-Type on body-less apiFetch requests (#692) Vercel's Next.js lambda adapter (Next 16.1.x) attempts to decode a request body when Content-Type is set and throws `SyntaxError: Unexpected end of data` before delegating to the route handler, returning a 500. Hit /run-context exclusively because it was the only body-less GET that sent `Content-Type: application/json`. - Drop `Content-Type: application/json` from the GET in `action/utils/runContext.ts` (meaningless on a body-less request). - Defensively strip any `content-type` header in `action/utils/apiFetch.ts` when no body is present so future callers can't reintroduce this. * apiFetch: soften comment — empirical observation, RFC 9110 §8.3 framing	2026-05-13 02:03:24 +00:00
Colin McDonnell	d8f825034f	billing: $10 signup credit + lazy claim modal; disable welcome credit promo (#674) ... * billing: $10 signup credit + lazy claim modal; disable welcome credit promo Adds a per-Account $10 Router signup credit granted on first Router-tab mount via a new admin-gated POST /api/account/[owner]/signup-credit/claim. The endpoint is idempotent — the inserted CreditGrant row IS the dedup state, so subsequent calls return granted:false. Client SignupCreditModal fires the POST on mount (only when modelAccessMode === "router") and opens a celebratory dialog when granted:true. Disables the legacy welcome credit ($10 on first card add) via a new WELCOME_CREDIT_PROMO_ACTIVE = false flag in utils/stripe.ts. Code path stays intact — flip the flag to revive. Strips the now-untruthful "$10 on enabling billing" copy from BillingCard, EnableRouterPrompt, triggerWorkflow paywall comment, action router_requires_card summary, email snippet, billing/pricing docs and wiki. Cuts WELCOME_CREDIT_CENTS from 2000 to 1000 to reflect the lower amount that would land if the flag is ever re-enabled. Adds "signup" reason mapping to BillingCard wallet history. Verified end-to-end against dev: admin+Router fires modal, admin+BYOK gate-blocks mount, BYOK→Router transition fires modal on click, member and collaborator paths skip the mount entirely, reload after grant is idempotent. Wallet history shows "Router signup credit +$10.00". * billing: address PR review (race fix, copy sweep, modal retry) Correctness: - Add @@unique([accountId, reason]) on CreditGrant + migration. The prior check-then-insert pattern in /signup-credit/claim and finalizeCheckoutSession raced at READ COMMITTED — two concurrent admin tabs could land two grants of the same reason on a fresh account ($10 each). Both write sites now rely on the unique index for dedup (P2002 = "already granted") and route updated to catch P2002 cleanly. Verified zero existing duplicates in prod before migration. - Add log.info on signup grant insert so a successful grant has any chance of being caught by ops monitoring. - Add retry: 2 with backoff to the claim mutation. Endpoint is idempotent so a server-side success that lost its response cleanly returns granted:false on retry. Public copy that still advertised the (now-deleted) $20 welcome credit: - app/page.tsx landing pricing card - emails/announceBilling.ts broadcast template - docs/keys.mdx BYOK note - components/AgentSettings.tsx Router-without-billing warning - utils/stripe.ts finalizeCheckoutSession JSDoc - utils/email/snippets.ts ROUTER_CREDIT_PS_HTML JSDoc Wiki staleness sweep: - wiki/billing.md TOC, mermaid diagram (signup edge added; welcome marked dormant), test coverage list, key modules section, no-card wallet narrative - wiki/pricing.md welcome-credit drawdown reference - Rewrote my own internally-inconsistent dormancy paragraph to be honest about the $20-historical / $10-on-revival framing. Trivia: - ModelAccessCard JSX comment had a literal \\u2192 instead of →. * billing: address PR review round 2 - Replace try/catch P2002 inside finalizeCheckoutSession's prisma.$transaction with createMany skipDuplicates. The previous form is broken on Postgres: a unique-violation poisons the surrounding TX, so the catch block returns cleanly but the outer commit fails and the account.update (stripeCustomerId) silently rolls back too. Currently armed only behind the dormant welcome- credit flag, but would have broken billing enablement the moment the flag flipped. createMany skipDuplicates yields a single ON CONFLICT DO NOTHING statement that returns count: 0 cleanly without aborting the TX. - Apply the same createMany skipDuplicates pattern to the signup-credit route too — drops the exception-as-control-flow Prisma namespace import and is more uniform with the welcome path. - Drop the now-orphaned credit_grants_accountId_idx in the same migration. The schema removed @@index([accountId]) when @@unique([accountId, reason]) was added (covered by the leftmost prefix), but the migration only added the unique index, leaving prod drifted. * billing: fix stale finalizeCheckoutSession JSDoc The function-level JSDoc still described the abandoned try/catch P2002 mechanism after switching to createMany skipDuplicates. The inline comment + code now agree on the new ON CONFLICT DO NOTHING shape. * billing: decouple first-card alert, drop vestigial billing field, fix modal cents; sync copy * docs+homepage: align Router credit copy with signup claim (no card-on-add carrot) * homepage: add pricing screenshot and pay-as-you-go promo line * billing: fix once-per-lifetime misframe on first-card alert * billing: suppress signup credit for prior welcome-credit recipients * billing: drop bogus '1000 users' cap; invalidate billing on signup-credit settle	2026-05-12 23:47:52 +00:00
Colin McDonnell	f0805b78f5	learnings: surface persist failures as warnings, not debug ... `persistLearnings` only emitted `log.info("» learnings updated")` on success; every failure path (non-2xx, fetch throw, 10s timeout) was `log.debug`, which is hidden unless `ACTIONS_RUNNER_DEBUG=true`. Survey of recent runs caught at least one case where the agent definitively edited the tmpfile but no DB row was written and no warning surfaced. Promote both failure paths to `log.warning` so dropped agent work is visible in CI logs. The unchanged-from-seed short-circuit stays at debug — that's a genuine no-op.	2026-05-11 23:51:46 +00:00
Colin McDonnell	e20b4d5515	action: bump to 0.1.4 v0.1.4	2026-05-11 23:22:47 +00:00
David Blass	8c6cd2bda2	cancel + restart workflow run when @pullfrog mention is edited (#612) ... * cancel + restart workflow run when @pullfrog mention is edited - add `WorkflowRun.triggeringCommentId` (BigInt?, indexed) so the webhook handler can find the run that was fired by a given comment - thread `triggeringCommentId` through `reserveRun` / `triggerWorkflow` - factor `dispatchMentionRun` out of `issue_comment_created` so the same shape is reused on edit - replace the `issue_comment_edited` stub: re-evaluates the trigger gate, cancels prior runs (`octokit.rest.actions.cancelWorkflowRun` + DB status='cancelled'), then re-dispatches with a `previousRunsNote` appended to `eventInstructions` so the agent acknowledges the prior run/PR/artifacts in its summary - if the edit removes `@pullfrog`, cancel only (no restart) Co-authored-by: Cursor <cursoragent@cursor.com> * thread previousRunsNote via dedicated payload field user prompt has precedence over eventInstructions, so stuffing the prior-runs note into eventInstructions made it vanish whenever the trigger comment contained an @pullfrog mention (which is always for the edit path). pass it as its own payload field and render it alongside the user's task so the agent actually sees it. * delete cancelled run's progress comment on edit-restart so the issue thread doesn't accumulate "This run was cancelled" stubs on every edit. only deletes for runs we actively cancel; runs that were already terminal (e.g. completed before the edit) keep their summary comment in the thread, and `previousRunsNote` links to it so the new agent can reference prior work. post-cleanup is race-safe: the action's `validateStuckProgressComment` swallows the 404 from the deleted comment and exits cleanly, so the old run's post step cannot clobber the new run's leaping comment. Co-authored-by: Cursor <cursoragent@cursor.com> * also cancel + delete progress comment when triggering comment is deleted mirrors the edit-removes-@pullfrog path: when an @pullfrog comment that fired a run is hard-deleted, look up any prior runs by triggeringCommentId, GH-cancel running ones, and delete their leaping progress comments. skips trigger-gate re-eval (we're tearing down a run, not firing one) and performs no restart. reuses the existing cancelRunsForTriggeringComment helper; the returned previousRunsNote is discarded since no dispatch follows. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: move cancellation before trigger gate in issue_comment_edited cancelRunsForTriggeringComment now runs before the triggerEnabled check, so edits that remove @pullfrog still cancel in-flight runs even when the repo mention trigger is currently disabled (e.g. for non-collaborators). * anneal: scope cancel updates per-row + simplify edit gate - replace blanket updateMany on (triggeringCommentId, repoId) with per-row, status-guarded updates so a parallel handler's freshly-reserved run cannot be clobbered into cancelled by a racing edit delivery. - drop wasMention/isMention early-break in issue_comment_edited; always run cancelRunsForTriggeringComment (DB is the canonical "did this comment ever trigger a run" source). closes the missing-changes.body.from edge and lets us tear down a still-running prior run even if the admin disabled the mention trigger mid-flight. - buildPreviousRunsNote returns undefined (not "") when no link lines materialize. - doc cleanups + wiki/modes.md addendum noting issue_comment_edited / _deleted now drive cancel + restart. Co-authored-by: Cursor <cursoragent@cursor.com> * address review feedback on cancel/restart semantics - guard workflow_run.completed update against status='cancelled' so a successful-but-uncancellable GH Actions job can't resurrect a cancelled row (and re-bill it) via the completed webhook. - bucket only status='completed' runs into `preserved` in cancelRunsForTriggeringComment; cancelled/failed prior runs have stubs as their progress comment, not summaries worth referencing. - emit previousRunsNote for the runId-null cancel case so the restarted agent always knows when it's superseding a prior dispatch. - drop the agent-forbidden `gh pr list` hint and soften 'was cancelled' to 'was signalled to cancel' in the note body. - post a fallback comment when the edit-path dispatch fails (prior run already torn down and progress comment already deleted). - symmetrize the delete-handler's pullfrog guard with the edit handler (key off hook.comment.user, not hook.sender). - trim misleading comments on the per-row DB update guard. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: Colin McDonnell <colinmcd94@gmail.com>	2026-05-11 23:20:44 +00:00
Colin McDonnell	e4d0fc7e3d	biome: ignore .logs/ (was matching only logs/) v0.1.3	2026-05-11 23:06:33 +00:00
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.	2026-05-11 22:04:19 +00:00
Colin McDonnell	cf94773bf0	modes: make task-list authoring the explicit first step in every mode checklist (#665) ... * modes: make task-list authoring the explicit first step in every mode checklist The system prompt already instructs the agent to author an internal task list at the start of every run (action/utils/instructions.ts:291), but the rule lives several hundred tokens above the agent's first decision point and references the mode's checklist before the agent has it. Compliance is roughly coin-flip across opus runs — PR #610 dead-air for 9m20s was the extreme case; my own #664 e2e runs split 1-for-1 on `todowrite` compliance. Putting the directive *inside* the checklist that `select_mode` returns co-locates instruction with referent at the moment the agent decides what to do next. Same vocabulary as the existing rule (`task list`, agent-agnostic; the harness already maps to `todowrite`/`TodoWrite` per-agent in agents/opencode.ts and agents/claude.ts). The directive is deliberately non-prescriptive about list contents — the agent authors items based on the work it's about to do, not from a hand-shaped template. Touches all 8 built-in modes and the PlanEdit override: - Build / AddressReviews / Review / IncrementalReview / Plan / Fix / ResolveConflicts / Task: inserts `1. **task list**: create your task list for this run as your first action.` and renumbers existing steps. - action/mcp/selectMode.ts: same insertion in the PlanEdit override checklist. - All internal step cross-references shifted +1 (`step 5` → `step 6`, `skip steps 3–4` → `skip steps 4–5`, etc.) across Review, IncrementalReview, and ResolveConflicts modes. One code-comment reference in IncrementalReview's preamble updated to match. Complements #664 (live progress streaming): streaming guarantees the user sees *something* regardless of compliance; this PR raises the ceiling on what they see when the agent does comply (clean numbered checklist tracking through the run instead of just the latest assistant message). 488 action tests pass; typecheck, lint, format all clean. * postRun: fix stale 'step 7' reference missed during +1 renumbering	2026-05-11 21:57:11 +00:00
Colin McDonnell	8e36f76cfa	postrun: thread AgentRunContext through the retry loop instead of repackaging (#652) ... * postrun: thread AgentRunContext through the retry loop instead of repackaging drop the per-gate plumbing in `runPostRunRetryLoop`: the loop now receives `ctx: AgentRunContext` whole and reads `ctx.stopScript` + `ctx.toolState.*` directly. `getUnsubmittedReview` becomes a pure utility in postRun.ts instead of a closure shipped over `AgentRunContext`. `AgentRunContext` loses 4 fields that duplicated `toolState` (`summaryFilePath`, `summarySeed`, `learningsFilePath`, `getUnsubmittedReview`) and gains `toolState: ToolState`. both harness call sites collapse from 11 lines to 7; main.ts deletes the inline closure. `ToolState` and friends move from `action/mcp/server.ts` to `action/toolState.ts` so non-MCP code (agents, post-run loop) stops importing run-state types from the MCP server module. no behavior change. 503/503 tests green. * toolState: relocate `CommentableLines` to break dep cycle with mcp/review `action/toolState.ts` was importing `CommentableLines` from `mcp/review.ts`, which pulled the entire MCP server compile graph (24 files) into any consumer of `ToolState` — including `cf-worker-indexing` via the `pullfrog/internal` re-export chain through `utils/log.ts` → `agents/shared.ts` → `toolState.ts`. that exposed a pre-existing TS error in `mcp/issueEvents.ts` (octokit types resolve differently under cf-worker's `moduleResolution: bundler`). move `CommentableLines` (a small `{ RIGHT: Set<number>; LEFT: Set<number> }` state-shape type) to `toolState.ts` where it's used; re-export from `mcp/review.ts` for back-compat with test and call-site imports. cuts cf-worker's mcp/ compile inclusion from 24 files back to 0. * postRun: drop mock-heavy retry-loop tests; keep pure gate predicate `runPostRunRetryLoop` and `executeStopHook` were covered by ~560 lines of mock-heavy regression-gate tests that stubbed `spawn` / `getGitStatus` and fabricated `AgentRunContext` to drive orchestration paths. per AGENTS.md ("prefer no test over a mock-heavy test that only catches the most obvious form of regression") and the empirical track record — the one real production failure of this code path (#646) was a missing npm release, not a logic bug a unit test could catch — the value-to-ceremony ratio is poor. delete them. keep only the pure predicate: `getUnsubmittedReview(toolState)` is a decision function whose four input conditions have user-visible consequences when wrong. 5 assertions, no mocks, no ctx fabrication. 488 tests still pass. * toolState: import PrepResult from prep/types.ts, not the barrel same dep-cycle class as the previous CommentableLines fix. importing PrepResult from prep/index.ts pulled prep/installNodeDependencies.ts into the Next.js production build's typecheck graph (via pullfrog/internal → utils/log.ts → agents/shared.ts → toolState.ts → prep/index.ts → installNodeDependencies.ts), and Next.js's stricter NODE_ENV-required ProcessEnv shape rejected an existing `env: { PATH: ... }` literal. prep/types.ts is a leaf module with zero imports — re-routing the type import severs the chain. Vercel preview deploy goes from Error → Ready; preview-sync stops racing the deploy.	2026-05-11 18:47:08 +00:00
Colin McDonnell	dee13b160f	console: case-insensitive owner/repo slug resolution (#649) ... * console: case-insensitive owner/repo slug resolution URL slugs may be any case but GitHub treats logins and repo names as case-insensitive (and 301-redirects to canonical case). Internal find/filter sites compared with `===`, so mixed-case slugs (e.g. `/console/Pullfrog`) hard-403'd in resolveOwnerAccess and silently redirected from the per-repo console when currentRepo lookup missed. Lowercase both sides at every slug comparison: resolveOwnerAccess installation lookup, currentRepo lookup in repo + history pages, ConsoleHeader installation/repo lookups, getInstallations personal split, getOrgMembership user/org checks, getInstallationRepos node filter, getUserRole owner-as-collaborator check, and the action runtime's installation-repo access check. Caches keyed by raw input remain case-split across casings; that's fine since both entries resolve to the same canonical GitHub data and TTLs are short. * api: resolve targetAccountId by gh node id getAuthenticatedAccountContext was looking up Account by `name` using the raw URL slug, but `Account.name` is plain String populated from canonical GitHub login. Mixed-case URLs would render the page (since resolveOwnerAccess is now case-insensitive) but every billing/secrets API call would 403 on the find-by-name miss. Resolve by gh_${access.installation.account.node_id} instead — invariant to case-folding and login renames. Same pattern as the sibling owner page route already uses.	2026-05-11 18:45:20 +00:00
Colin McDonnell	ef394277c1	review: synthesize [!NOTE] informational tier with #644 alert judiciousness — 4-callout visual ladder + approved Fix-gate (#653) ... * review: NOTE-tier callout + `actionable` flag to suppress Fix buttons Adds an `actionable` parameter to the `create_pull_request_review` tool (defaults true) so the agent can opt out of the Fix-it/Fix-all/Fix-👍s footer affordance on informational reviews. Threaded through `createAndSubmitWithFooter` so the buttons are omitted when `actionable: false`. Updates `Review` and `IncrementalReview` mode prompts with a 4th tier: `> [!NOTE]` + `actionable: false` for mergeable, FYI-style observations (prior feedback addressed cleanly, minor stale doc reference, etc.). Calibration note: `[!IMPORTANT]`/`[!CAUTION]` are reserved for findings that warrant code changes, because that's what trains users to click Fix. `[!NOTE]` reviews must not carry inline comments — if a point is concrete enough to anchor to a line, upgrade the whole review tier. * review: drop redundant `actionable` flag, key Fix buttons off `approved` `approved` already encodes "this PR is mergeable, nothing for the Fix button to act on" — `actionable` was a second flag carrying the same signal. Drop it from the tool schema and `FooterOpts`; the footer gate stays `if (!opts.approved)` (unchanged from pre-PR behavior, with a new comment documenting the UX rationale). NOTE-tier reviews now use `approved: true` + `> [!NOTE]` body instead of `approved: false` + `actionable: false`. For repos with `prApproveEnabled: false`, the runtime already downgrades APPROVE to COMMENT, so the GitHub-side shape is identical to the prior design. * review: address Pullfrog feedback — drop ambiguous parenthetical + update postRun nudge - Review-mode calibration: drop the "(or no callout at all)" parenthetical that didn't map cleanly to a bullet; replace with explicit "both the `[!NOTE]` tier and the 'no actionable issues' tier below use approved: true" so the bullet-list anchor is obvious. - `buildUnsubmittedReviewPrompt` (Review mode): the fallback nudge for unsubmitted reviews now defers to the mode prompt's tier matrix and acknowledges that `> [!NOTE]` informational reviews submit with `approved: true` alongside the canonical "No new issues found." path. Previously the nudge only described the pre-NOTE binary world.	2026-05-11 17:14:03 +00:00
David Blass	ee479474ce	action: tighten review alert judiciousness in prompts (#644) ... The Review and IncrementalReview prompts unconditionally wrapped any non-critical review body in `> [!IMPORTANT]`, even for trivial nits or "rough edge" observations. The result is alert fatigue — full-width colored callouts dominate the page when the actual finding is a single JSDoc tweak. Adds an explicit judiciousness preamble to both Review step 5 and IncrementalReview step 7, and splits the prior single non-critical tier into two: - must-address non-critical (`[!IMPORTANT]`) — gated on real consequences if shipped (incorrect behavior, missing validation, regressions the author should fix before merge) - minor suggestions only (no alert) — single-line nits, doc/comment polish, defer-able observations, "rough edges" Critical tier wording also tightened to spell out the bar (`bugs, security, data loss, broken core flows`). Co-authored-by: Cursor <cursoragent@cursor.com>	2026-05-11 17:07:00 +00:00
Colin McDonnell	96910f0f50	fix(run-audit): drop summary comment, fall back to agent final message in job summary ... the audit agent's final 'post a short summary' instruction was ambiguous and, with no PR/issue context on schedule runs, caused the agent to invent a target — landing the summary as a comment on the most recent open PR (see #650). drop the comment instruction outright. writeJobSummary now falls back to the agent's final assistant message (result.output) when lastProgressBody is empty, so non-PR runs surface a real summary in the GitHub Actions job summary tab instead of just the usage table. lastProgressBody still wins when present to avoid duplicating the progress comment body.	2026-05-11 16:56:26 +00:00
Colin McDonnell	4cc6d95a91	ci: split per-alias resolution smoke from per-provider harness smoke (#650) ... * ci: split per-alias resolution smoke from per-provider harness smoke `models-live` previously ran the full Pullfrog harness (Docker + MCP + agent + structured-output validation) once per alias on every PR that touched `models.ts` or `agents/**`. That cost minutes and dollars per alias and re-validated tool-calling for every routing wrapper. The per-alias signal we actually need from `models.ts` changes is just "does this alias resolve and authenticate." Tool-calling correctness is a property of the underlying model, not the alias, and it doesn't change when someone adds a row to the catalog. Splitting the two concerns: - `models-live` now runs `action/test/model-smoke.ts` per alias — a top-level CLI invocation (`opencode run -m <resolve> "reply OK"` or `claude -p "reply OK" --model <bare>`) with no Docker, MCP, or Pullfrog harness. Validates resolution + auth in seconds at fractions of a cent. Lets us drop the `EXPENSIVE_RESOLVE_SUBSTRINGS` carve-out for `gpt-pro` since the cheap smoke covers it for free. - `providers-live` (new) runs the full harness smoke once per provider against a hand-curated standard-tier model (`anthropic/claude-sonnet`, `openai/gpt`, `google/gemini-pro`, `xai/grok`, `deepseek/deepseek-pro`, `moonshotai/kimi-k2`, `opencode/big-pickle`, `openrouter/claude-sonnet`). Catches provider-class regressions like the Gemini schema sanitizer or OpenAI tool-call format drift. ~8 jobs, ~$0.40/push, ~4min critical path in parallel. Net change per push that touches `models.ts`: ~$20 → ~$0.40. `list-aliases.ts` now branches on `MODE` to emit either matrix; the flagship list asserts each slug exists in `modelAliases` so renames break CI loudly. Wiki updated to reflect the new two-tier coverage and the operational rule for new Gemini aliases (cheap smoke covers auth, manual harness run still needed for sanitizer compatibility on non-flagship Gemini additions). * fix(model-smoke): walk fallback chain; address pr review comments - model-smoke now uses `resolveCliModel(slug)` instead of `alias.resolve` so deprecated aliases (those with `fallback` set, e.g. `opencode/mimo-v2-pro-free` → `opencode/big-pickle`) hit the replacement model the way production does. mimo-v2-pro-free was failing CI because the underlying opencode model is dead — the fallback chain is the whole point of marking it deprecated. - tighten stale `agentForSlug()` reference in model-smoke.ts comment (function was deleted in this same PR; classification is now inline in `list-aliases.ts toMatrixEntry`). - tighten `FLAGSHIPS` drift comment to call out that the assertion is one-way (catches slug-rename, but silently omits new providers). Update wiki step 4 of "To add a provider" to require adding the standard-tier slug to `FLAGSHIPS` for harness coverage. * docs: scrub stale env-knob refs in models-catalog parity section `wiki/models-catalog.md` cross-provider parity paragraph still pointed at `INCLUDE_ALL_PASSTHROUGHS` / `INCLUDE_EXPENSIVE` and the implicit filter→expensive-gate coupling — all removed in this PR. Aligned the copy with Step 9 (which was already updated): `INCLUDE_PASSTHROUGHS`, no expensive gate, `MATRIX_FILTER` applies to both aliases and flagships modes.	2026-05-11 16:36:14 +00:00
David Blass	10590993f4	checkout_pr: retry missing pull/N/head ref with PR-state guard (#627) ... * checkout_pr: retry missing pull/N/head ref with PR-state guard Co-authored-by: Cursor <cursoragent@cursor.com> * checkout_pr tests: satisfy ToolState required fields Co-authored-by: Cursor <cursoragent@cursor.com> * checkout_pr: tighten retry-helper semantics (anneal round 1) Co-authored-by: Cursor <cursoragent@cursor.com> * checkout_pr: use retry util, drop retry tests * Update action/mcp/checkout.ts Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>	2026-05-09 00:58:55 +00:00
David Blass	10aeaf8c11	action: dedupe identical reply_to_review_comment calls within a session (#623) ... * action: dedupe identical reply_to_review_comment calls within a session PR #610 reproduced a Kimi K2 stutter where the agent's tool_use surface showed one `pullfrog_reply_to_review_comment` call but GitHub recorded two byte-identical POSTs 3s apart, leaving a duplicate response on `action/mcp/review.ts:14`. Add `duplicateReplyDecision` (mirrors `duplicateReviewDecision`) and track per-session replies on `ToolState.reviewReplies`, keyed by parent `comment_id` + `bodyWithFooter`. Identical re-emissions short circuit with `{ skipped: true, reason }` instead of POSTing again. Body-keyed (not just id-keyed) so legitimate follow-up replies with different content still go through. Tighten `AddressReviews` step 5 to say *exactly once per comment* and note that the runtime dedupes identical bodies, so the agent has both prompt-level guidance and a server-side guarantee. Co-authored-by: Cursor <cursoragent@cursor.com> * address review: drop stale file ref in dedupe comment; soften tool description * remove comment.test.ts --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>	2026-05-09 00:30:51 +00:00
Colin McDonnell	85d25a6fe6	post-run gate: fail review-mode runs that don't submit a review or progress (#638) ... * post-run gate: fail the run when review mode finishes without a review or progress review-mode runs that ended in a text-only assistant turn ("now I have enough to draft the review...") were silently swallowed: the progress comment was deleted by stranded-comment cleanup and no review appeared on the PR. user- visible result was identical to "the agent never ran." caught in https://github.com/pullfrog/app/actions/runs/25583698781. new post-run gate alongside stopHook / dirtyTree / summaryStale: derived inline from toolState (selectedMode in {Review, IncrementalReview} && !review && !finalSummaryWritten && hadProgressComment) — no parallel toolState flag. when it fires, the resume prompt nudges the agent to call either create_pull_request_review or report_progress; persistent failure after MAX_POST_RUN_RETRIES surfaces as AgentResult.error. also: when the post-run loop returns success=false, write the error to the progress comment before the stranded-comment cleanup runs, and skip the delete in that case. previously a !success run from the loop would lose the error message into the void. IncrementalReview's trivial-skip branch now calls report_progress with a brief "no review warranted" note instead of exiting silently — keeps the contract symmetric with the gate and gives the user a visible signal even on no-op review runs. documents the literal-record design rule on the ToolState interface so future fields don't drift back into derived/absence-encoding state. * review feedback: mode-aware nudge, gate-error preservation, prompt order addresses three findings from the auto-review on this PR: 1. Review mode nudge no longer offers `report_progress` as an exit. Review mode's contract (modes.ts step 5) forbids it; the gate previously sent contradictory copy. IncrementalReview's nudge still offers both since its trivial-skip path legitimately allows `report_progress`. 2. `writeJobSummary` is now wrapped in try/catch on the success-path cleanup. without this, a throw there jumped to the outer catch and overwrote the gate's failure message in the progress comment with the (less actionable) writeJobSummary error — restoring exactly the invisible-failure UX this PR fixes. step-summary writes are informational; let them fail silently. 3. `buildPostRunPrompt` reorders gates to match the terminal hard-fail order: `stopHook` → `unsubmittedReview` → `dirtyTree` → `summaryStale`. when both hard-fail gates co-fire (rare in review modes), the prompt's emphasis now matches the user-visible failure message. new test asserts the IncrementalReview nudge offers both exits while the Review nudge offers only `create_pull_request_review`. e2e validation already passed against pullfrog/preview-638-review-stop-hook PR #1 (gate fired once; agent recovered on second turn). * mode-aware terminal error copy second auto-review caught a residual contradiction: the terminal hard-fail error string reported "create_pull_request_review or report_progress" for both modes, even though the new mode-aware nudge tells Review-mode agents "Review mode does not have a no-submit exit". the error message now mirrors the nudge — Review names only `create_pull_request_review`, IncrementalReview lists both. additional Review-mode hard-fail test asserts the absence of `report_progress` in the error.	2026-05-09 00:14:31 +00:00
David Blass	653fae47a5	claude: surface structured error from is_error result events instead of dumping NDJSON (#626) ... * claude: surface structured error from is_error result events instead of dumping NDJSON Co-authored-by: Cursor <cursoragent@cursor.com> * claude: tighten error-surface fixes (anneal round 1) Co-authored-by: Cursor <cursoragent@cursor.com> * claude: remove tests per request * claude: gate is_error short-circuit on subtype=success, restore error_* branches * claude: preserve fallback token table for error_* subtypes the `lastResultError === null` guard was too broad — `error_max_turns` / `error_during_execution` / `error_*` subtypes set `lastResultError` from `event.errors[]` and represent runs that genuinely consumed tokens, so suppressing the fallback table silently dropped billing visibility for those cases. gate on a dedicated `syntheticStopFailure` flag that's set only for the `subtype: "success"` + `is_error: true` case where `accumulatedTokens` is stale. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>	2026-05-09 00:07:50 +00:00
Colin McDonnell	363e4cbed8	ci: gate gpt-5.5-pro by resolve, refresh stale matrix docs (#639) ... * address review: gate by resolve, refresh stale doc claims - list-aliases.ts: gate EXPENSIVE on alias.resolve substring (catches opencode/gpt-pro and openrouter/gpt-pro, which both resolve to a gpt-5.5-pro variant — would have re-entered the matrix under INCLUDE_ALL_PASSTHROUGHS=1 and tripled the cost). - test.yml + models-catalog.md: stop describing the matrix as exhaustive. Mention pruning + INCLUDE_EXPENSIVE/MATRIX_FILTER opt-ins. * address review: clarify env vars are local-only, filter input is the CI knob Pullfrog review caught that wiki/models-catalog.md was advertising INCLUDE_EXPENSIVE / INCLUDE_ALL_PASSTHROUGHS as workflow_dispatch knobs — they're not, only `filter` (→ MATRIX_FILTER) is wired through. The filter coupling already implicitly opens the expensive gate, so dispatch + filter is the canonical CI path.	2026-05-09 00:01:30 +00:00
Colin McDonnell	c8888cecde	bump action version to 0.1.2 v0.1.2	2026-05-08 23:37:52 +00:00
Colin McDonnell	c0de70431e	ci: prune openai/gpt-pro from default models-live matrix (#637) ... * ci: prune openai/gpt-pro from default models-live matrix gpt-5.5-pro burns ~$2.40/run ($30/M input, $180/M output) — flagship reasoning tier with hidden reasoning tokens dominating cost. Multiplied by every push that touches a resolution-affecting file, the bill is untenable for a smoke that just verifies set_output works. Pruned by default; re-enable with INCLUDE_EXPENSIVE=1 or MATRIX_FILTER when validating the alias on demand. Also adds a comment-frugality rule to AGENTS.md. * ci: include list-aliases.ts in models paths-filter The matrix builder is resolution-affecting from a validation standpoint — a regression to it (e.g. accidentally pruning all aliases) wouldn't trigger models-live on its own commit.	2026-05-08 23:36:26 +00:00
Colin McDonnell	b0274e3265	local proxy-key testing via x-dev-repo bypass (#629) ... * local proxy-key testing via x-dev-repo bypass `pnpm play` previously couldn't exercise the proxy/router/oss code path — `resolveProxyModel` early-exits without OIDC credentials, and `mintProxyKey` always sends an OIDC bearer to `/api/proxy-token`. since GitHub Actions OIDC only exists in real workflow runs, billing flows (auto-reload, balance gates, key rotation, OSS subsidy) had no local feedback loop. a server-side dev bypass already exists at `app/api/proxy-token/route.ts` that accepts an `x-dev-repo: owner/repo` header instead of an OIDC bearer when `NODE_ENV === "development"`. wire the action side so it sends that header when there are no OIDC credentials AND `API_URL` resolves to localhost (i.e. the developer is talking to their own `pnpm dev` server). production is unreachable through this path because vercel never sets `NODE_ENV=development`. document the affordance in `wiki/action-tests.md` so the next person doesn't have to re-discover it (the server bypass had been sitting there undocumented since the WIP billing rewrite). verified end-to-end: `PLAY_LOCAL=1 GITHUB_REPOSITORY=pullfrog/app API_URL=http://localhost:3100 pnpm play …` now logs `» proxy: dev bypass (x-dev-repo) for pullfrog/app` → `» proxy: router → openrouter/ anthropic/claude-opus-4.7` → `» model: …(proxy)`, mints a real OpenRouter key against the dev DB, and the agent runs through the proxy. * wiki: cross-reference dev proxy-key affordance from main/e2e/stripe action-tests.md already documents the localhost+x-dev-repo path; mention it from the natural discovery points so the next person finds it without spelunking through git history again: - main.md: resolveProxyModel row in the dependencies table notes the two auth paths (OIDC bearer in prod, x-dev-repo in dev). - e2e-testing.md: "When to use this" calls out the lighter-weight alternative for proxy-only changes. - stripe.md: new "Loop including the action" subsection in the Dev workflow section, alongside the existing dev-script and cron-endpoint loops.	2026-05-08 23:35:58 +00:00
Colin McDonnell	8f36eca62a	action: use log.success for skill install confirmations	2026-05-08 23:32:20 +00:00
Colin McDonnell	3c9799adda	add models-bump cron + drop snapshot test ... every 12h, scripts/find-newer-models.ts scans models.dev for newer GA versions of every alias in action/models.ts and writes a focused per-alias diff. .github/workflows/models-bump.yml short-circuits when no candidates exist; otherwise hands the diff to pullfrog/pullfrog@main to evaluate against the policy in wiki/model-resolution.md and open a single living PR on the pullfrog/models-bump branch. drops the brittle "latest model per provider" snapshot block in action/test/models-catalog.main.test.ts (and its .snap file) — the cron keeps the registry in sync with upstreams, and the remaining validity tests act as the integrity gate on the bump PR.	2026-05-08 23:27:42 +00:00
Colin McDonnell	5f3e46c42d	fix: don't reuse disabled proxy key on workflow re-runs; non-fatal title-gen errors (#636) ... * fix: don't reuse disabled proxy key on workflow re-runs; non-fatal title-gen errors Three small surgical fixes addressing run https://github.com/pullfrog/app/actions/runs/25580969379: 1. **`/api/proxy-token` idempotency now checks `finalizedAt`.** GitHub re-runs share the same `run_id` (only `run_attempt` increments), so attempt N+1's action calls /api/proxy-token and inherits attempt N's `proxyKeyId`. The `workflow_run.completed` webhook between attempts retires that key on OpenRouter (`disableKey`), so attempt N+1 was getting back a disabled key and OpenRouter responded with `401 User not found` on every call. Falling through when finalized routes through the same billing gate (`handleRouterBilling` balance check), so no new attack surface. 2. **OpenCode title-gen / small-model errors no longer fatal.** OpenCode auto-spawns a small `agent=title small=true` background call at session start to name the thread, defaulting to `anthropic/claude-haiku-4.5` (anomalyco/opencode#1243). Pre-fix, the wrapper's `error` event handler treated any `type=error` as fatal, so a cosmetic title failure killed the run before primary inference even started. Now: stderr matching `small=true` sets a one-shot suppression flag for the next stdout `error` event, which is logged as a warning instead. 3. **Provider-error classifier puts auth patterns above rate-limit.** OpenRouter 401 payloads bundle `x-ratelimit-*` response headers, and the loose `\brate[_ ]limit/i` pattern was winning. Added 401/403 status, `User not found`, `Invalid authentication`, `No auth credentials found` patterns ahead of rate-limit. Updated the existing 401-headers regression test to assert correct auth classification rather than `null`. * opencode: correlate small-model error suppression by message, not by next-event Pullfrog self-review on #636 flagged a real concurrency hole. OpenCode forks the title-gen call (`session/prompt.ts:1452-1457` via `Effect.forkIn(scope)`) so it races primary inference. The previous one-shot `suppressNextErrorEvent` boolean had no per-call correlation: it was consumed by whichever stdout `type=error` event landed next, regardless of which subagent produced it. Under concurrent failures, a primary-agent error landing first could be silently downgraded to a warning while the small-model error then propagated fatally — the inverse of the bug the suppression was meant to prevent. Replaced the boolean with a `Set<string>` of pending small-model error messages. stderr extracts the inner `"message":"..."` from any classified provider error tagged `small=true`; the stdout `error` handler suppresses only when `event.error.data.message` matches a pending entry. Set is capped at 32 entries so a long stream of small-model failures can't wedge memory. Also corrected the comment that referenced "session summarizer" — verified in opencode source that summarize() does NOT use `small: true`; only the title generator does today (only `small: true` match in the codebase). * revert: drop opencode title-gen suppression We have no evidence — and can't construct a realistic scenario — where title-gen fails on an otherwise-successful run. Title-gen and primary share the same OPENROUTER_API_KEY and hit the same proxy/upstream; whatever breaks one breaks the other. The original repro on run 25580969379 is fully explained by the stale proxy key (fix #1) — title-gen happened to be the first call that surfaced the auth error, but every subsequent primary call would have died the same way. Suppression code adds complexity (cross-stream correlation logic, message matching, set capping) and a real failure mode of its own (a small-model error with a unique message could mask an unrelated primary error landing shortly after). Net negative. Removing.	2026-05-08 23:00:41 +00:00
Colin McDonnell	3d393c36a3	opencode: surface subagent events via injected plugin (#634) ... * opencode: surface subagent events via injected plugin opencode's cli/cmd/run.ts event loop filters all message.part.updated events to the orchestrator's session id (`part.sessionID !== sessionID` continue), so subagent-internal tool_use / text / step events were silently discarded by the CLI in --format json mode. opencode plugins, by contrast, receive every bus event via bus.subscribeAll() regardless of session. ship a per-run plugin (action/agents/opencodePlugin.ts) that re-emits non-orchestrator message.part.updated events as `pullfrog_bus_event` envelopes on opencode's stdout. the plugin is staged into <XDG_CONFIG_HOME>/opencode/plugin/pullfrog-events.ts which is already redirected to ctx.tmpdir — never the user's repo working tree. the plugin also forwards the orchestrator's task tool dispatch at state.status="running" — that's the first moment state.input is populated with description / subagent_type / prompt and it lands BEFORE the subagent's first message.part.updated. forwarding this lets SessionLabeler register the lens label early, so subagent events bind to the correct lens name (e.g. lens:correctness) instead of the subagent#N fallback. the existing tool_use handler dedupes on callID so the late status=completed event from the CLI doesn't double-record. the parent's pullfrog_bus_event handler synthesizes the equivalent CLI-style event for each part type (tool/step-start/step-finish/text) and dispatches through the same handlers used by orchestrator events, so labeling, tool-call rendering, and the formatWithLabel magenta prefix all share one code path. verified end-to-end via `pnpm play --local --raw` with a prompt that dispatches a reviewfrog subagent: orchestrator's task call now logs "» dispatching subagent: lens:read-readme-and-report-purpose" before the subagent runs, the subagent's read tool call surfaces with [lens:...] magenta prefix, and the run-end "subagent finished" attribution shows the lens name. also adds an AGENTS.md rule formalizing the no-write-to-repo invariant: action runtime must never write into the user's working tree; auxiliary files go in ctx.tmpdir via HOME / XDG_CONFIG_HOME. * drop opencodePlugin.test.ts — bullshit-test cleanup these tests spied on process.stdout.write, loaded the plugin source into a temp file via dynamic import, and asserted the output strings matched the plugin source i'd just hand-written. zero unique signal over the e2e run in preview repo, plus they violate AGENTS.md's "mocks tend to add ceremony and brittleness" rule. real signal lives in the e2e: lens label rendering, dispatch attribution, no double events. if a syntactic regression in the plugin source ever ships, opencode logs it on plugin load and the e2e fails fast — the unit tests would catch the same regression no faster. * remove isPausedExternally — plugin makes it unnecessary empirical proof from PR #634's e2e debug trace: ~3.3 pullfrog_bus_event lines per second arrive on the parent's child.stdout pipe during a typical subagent run. each one fires updateActivity() and resets lastActivityTime, so the inner spawn activity timer naturally stays armed-but-not-fired throughout the subagent's lifetime — no suspend predicate needed. drop: - SpawnOptions.isPausedExternally + the check in spawn()'s activity loop - isSubagentInFlight() in opencode.ts + its callsite - two isPausedExternally unit tests in subprocess.test.ts keep: - killGroup (the actual zombie-prevention fix; still tested) - the plugin (action/agents/opencodePlugin.ts; the architectural fix) - everything in opencode.ts that derives lens labels from task dispatches the only edge case isPausedExternally covered that the plugin doesn't is a non-streaming provider going silent for >5min during a single LLM call inside a subagent. that's a provider-behavior question, not a harness-architecture one — best fixed at the provider level if it shows up. defense-in-depth that adds indirection is harmful when the upstream architectural fix is already in place. * opencode: address review feedback on bus envelope routing three findings from PR #634 review (2026-05-08T22:13:44Z): 1. token/cost double-count: routing subagent step_finish through the orchestrator's handler folded subagent tokens/cost into the run-wide accumulators that flow to logTokenTable + AgentUsage. neighbouring init/text handlers all gate on ORCHESTRATOR_LABEL for exactly this reason. fix: drop step_start AND step_finish from the bus envelope handler — those carry orchestrator-scoped state (currentStepId, stepHistory, token accumulators) that subagent events shouldn't touch. tool calls and text from subagents still surface — that's the user-visible activity. 2. subagent tool errors invisible: routed status="error" tool parts into handlers.tool_use which only emits "» <tool>(...)" with no error indication. fix: extend handlers.tool_use itself to log "» tool call failed: <msg>" when state.status==="error". benefits the orchestrator path too — opencode CLI also emits failed tool calls as tool_use at status=error and we were swallowing the failure signal there as well. 3. stale comments + leaked local paths: plugin source had /tmp/opencode-investigate/... paths from my local clone, specific line numbers from opencode's dev branch that don't match v1.1.56, forkDetach claim that's wrong for the pinned version, and JSDoc that still listed message.updated/session.error in the forwarded set after the runtime filter narrowed to message.part.updated only. fix: drop machine-local paths, drop version-fragile line numbers, correct the forwarded-set list, generalize the "why no @opencode-ai/plugin import" rationale to be version-agnostic. second review (2026-05-08T22:27:58Z) confirms these are the only findings still open — no new issues from the isPausedExternally removal.	2026-05-08 22:46:43 +00:00
Colin McDonnell	d6de1c369a	learnings: edit-in-place tmpfile (drop update_learnings tool) (#635) ... * learnings: edit-in-place tmpfile (drop update_learnings tool) learnings now follow the PR-summary file pattern: server seeds `pullfrog-learnings.md` from `Repo.learnings` at startup, agent reads it as part of context, may edit in place during the post-run reflection turn, server reads back at end-of-run and PATCHes if changed. motivation: `update_learnings` required the agent to pass the FULL merged list as a string parameter — an output-token tax that grew linearly with the learnings size, and a constant prompt-context expansion since the contents were also inlined into the LEARNINGS section. for repos with mature learnings the prompt was getting visibly noisy in CI logs. key changes: - new `action/utils/learnings.ts` (seed/read helpers + 10k cap) - `main.ts`: always seed; `persistLearnings` mirrors `persistSummary` (success path, error path, exit-signal handler, idempotent guard, byte-trim equality skip); forwards `model` for `LearningsRevision.model` - `LEARNINGS` prompt section now contains only the file path + a one-line "read it" instruction (no contents inlined) - `update_learnings` MCP tool deleted; `action/mcp/learnings.ts` removed - reflection turn (`buildLearningsReflectionPrompt`) reframed around file editing with explicit prune-stale + leave-alone-if-nothing-new framing - `learningsStep` removed from every mode checklist — surface lives only in the LEARNINGS prompt section + the reflection turn now * learnings: harden seed step + refresh stale docs (review feedback) Three findings from PR review, all implemented: 1. wrap learnings seed in best-effort try/catch (action/main.ts) — the always-on seed block ran unconditionally and an unwrapped `seedLearningsFile` (mkdir + writeFile) failure (ENOSPC, EACCES, hostile sandbox) would unwind into the outer main() catch and flip an otherwise-successful run to "❌ Pullfrog failed" before the agent even started. asymmetric with `persistLearnings`'s own best-effort contract. wrap and log on failure; downstream consumers (`persistLearnings`, agent harnesses, `resolveInstructions`) already handle `learningsFilePath: undefined` cleanly. 2. refresh wiki/main.md — `resolveInstructions` parameter renamed from `learnings` to `learningsFilePath` in this PR; the data-flow diagram and the resolver dependency table both still showed the pre-refactor signature. 3. drop deleted `learnings.ts` from ROADMAP.md + RESEARCH.md "missing MCP tool tests" bullets — `action/mcp/learnings.ts` was removed in this PR; the bullets are otherwise still accurate.	2026-05-08 22:45:26 +00:00
Colin McDonnell	2e6c01670e	mcp: log artifact id after every github write (#633) ... makes debugging easier by emitting a single `» <verb> <kind> <id>` line after every successful GitHub write (and upload) the agent performs via the Pullfrog MCP, mirroring the chevron convention used elsewhere.	2026-05-08 21:48:28 +00:00
Colin McDonnell	17b610e1a1	bump action version to 0.1.1 v0.1.1	2026-05-08 21:32:06 +00:00
Colin McDonnell	ca913c76ea	spawn: kill process group + heartbeat subagent activity (#631) ... * spawn: kill process group + heartbeat subagent activity two compounding bugs produced zombie agent runs that stalled until the GitHub-Actions job-level timeout (observed on PR #622, run 25577068620). 1. SIGKILL hit the wrong process. node_modules/opencode-ai/bin/opencode is a Node shim that spawnSyncs the native opencode-<plat>-<arch> binary with stdio:"inherit". our spawn() ran without detached, so child.kill("SIGKILL") killed only the shim. the native binary was reparented to PID 1, kept holding our stdout pipe via inherited fds, and child.on("close") never fired — leaving the agent promise pending past the 5min outer safety-net timer ("agent still pending 5min after inner activity kill — forcing exit") and the grandchild running until the runner timed out. fix: SpawnOptions gains killGroup; when set, we spawn detached and route all kill paths (timeout, activity timeout, ctrl-c) through process.kill(-pid, signal). opencode + claude opt in. 2. inner activity timer false-fired during long task subagents. opencode's `task` tool encapsulates subagent execution in-process — subagent-internal events don't reach the parent NDJSON stream — so the parent looked idle for the full subagent duration even when real work was happening, and the 5min DEFAULT_ACTIVITY_TIMEOUT_MS would fire mid-subagent. fix: SpawnOptions gains externalActivitySource; the timer fires on min(local stdout idle, external idle). opencode passes getIdleMs() from the global activity tracker and runs a 30s heartbeat (markActivity()) while at least one task dispatch is in flight. action/utils/subprocess.test.ts covers both: a bash+sleep grandchild that proves close fires <10s with killGroup, and externalActivitySource keeping the timer armed during 8s of stdout silence. * opencode: suspend activity timer instead of heartbeat during subagent runs addresses review on prior commit: replace the 30s markActivity() heartbeat with a boolean isPausedExternally predicate keyed off opencode's existing taskDispatchByCallID + pendingTaskDispatches. no fake activity, no race window between a 30s tick and a subagent that finishes between ticks. while the predicate returns true, spawn's activity check skips the kill decision *and* advances lastActivityTime so a clean unpause can't fire on a stale baseline. tests cover both the suspended case (8s of stdout silence + activityTimeout=1s but paused → process exits cleanly) and the resume case (paused for 500ms then unpaused → 30s sleep gets killed by activity timeout as normal).	2026-05-08 21:29:22 +00:00
Colin McDonnell	20d4b12522	bump action version to 0.1.0 ... document direct-to-main exceptions in AGENTS.md (version bumps and other release-trigger commits when the user explicitly says "push to main"). v0.1.0	2026-05-08 21:26:53 +00:00
Colin McDonnell	ec43c0e0d1	router: fix bugs from PR #616 review (#625) ... Three real defects flagged in the post-merge review of #616, plus one cheap hardening: 1. OpenCode `limit.output` override was a silent no-op on opencode-ai@1.1.56. Top-level `limit.output` has no read site in OpenCode (verified against the v1.1.56 source: `OUTPUT_TOKEN_MAX = Flag.OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX || 32_000` in session/llm.ts; per-model `model.limit.output` has its own scope). Plumbed via `OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX=5000` env var on the OpenCode spawn instead. Drops dead `OpenCodeConfig.limit?` type field and the corresponding config write in `buildSecurityConfig`. This was the headline mechanism of #616 — without the env var, the upfront `max_tokens` reservation stayed at 32_000 and low-wallet runs continued failing the way #616 was supposed to prevent. 2. Phantom auto-reload buffer for detached-card accounts. DELETE /payment-method clears `stripeCustomerId` but leaves `autoReloadEnabled` intact, so an account with welcome-credit residue and a detached card could mint a key with `keyLimitCents = balance + autoReloadAmountCents` ($50 default, schema-cap $100K) of free spend headroom we have no way to bill. Conjunctive `account.autoReloadEnabled && hasCard` in the buffer selection closes this. Defense-in-depth follow-up worth doing: clear `autoReloadEnabled` in the card-detach handler. 3. The autoReloadEnabled 402 branch fired for phase-1 noop paths (`!stripeCustomerId`, `reloadAmountCents < 50`, `balance >= threshold`) where `result.failure == null`, returning `"insufficient balance"` with no actionable code. Gated on `result.status === "failed"` so non-charge paths fall through to the `hasCard` / no-card branches and emit `router_balance_exhausted` / `router_requires_card` instead. 4. (cheap) `ROUTER_KEYLIMIT_EXHAUSTED_PATTERN` now uses `/is` instead of `/i` so `.*?` crosses newlines. Defends the BillingError reclassification against any upstream layer that wraps the OpenRouter error onto multiple lines. Trivial. Test plan: 488/488 unit tests pass (1 new test for newline regex behavior).	2026-05-08 21:02:38 +00:00
Colin McDonnell	93cc7b1a44	show effective model in agent comment/review footers (#618) ... `toolState.model` was set only to `payload.model` (the stored slug, often undefined for router/oss runs that derive the target from `proxyModel`). the footer's "Using `…`" segment is gated on a truthy model, so router runs on repos without an explicit model setting shipped reviews/comments with no model badge — e.g. PR #614's review showed no model despite running `openrouter/anthropic/claude-opus-4.7` via proxy. now mirror the priority used by `resolveModelForLog` and `isGeminiRouted`: `payload.proxyModel ?? resolvedModel ?? payload.model`. also reverse-look up by `resolve`/`openRouterResolve` in `formatModelLabel` so a proxy target like "openrouter/anthropic/claude-opus-4.7" still renders as "Claude Opus".	2026-05-08 20:59:09 +00:00
pullfrog[bot]	851e49e2d7	action: retry transient GitHub 422 "internal error" on review submission (#610) ... * action: retry transient GitHub 422 "internal error" on review submission GitHub sometimes 422s POST /pulls/{n}/reviews with body "An internal error occurred, please try again." — a server-side hiccup that the existing 422 handler framed with the generic "likely causes (1)(2)(3)" prompt listing affected comments. the agent dutifully refetched the diff, dropped comments, and resubmitted, hitting the same transient error on a shifting affected-comments list until GitHub accepted. some runs logged 8+ spurious retries with ~11 minutes of wall-clock, dropping valid inline comments along the way. detect the transient body explicitly, retry in-tool twice with 1s/3s backoff, and surface a distinct error on exhaustion that tells the agent this is a GitHub-side issue — do not modify inline comments, wait and retry or fall back to a body-only review. closes #584. * action: use retry util for transient review 422, drop isTransientReviewError tests --------- Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>	2026-05-08 20:33:01 +00:00
Colin McDonnell	4101df566b	router: decouple per-run key budget from wallet, add overdraft buffer (#616) ... Replaces today's `keyLimitUsd = min(walletBalance, $25)` with population-aware buffers so users can use 100% of their credits before being paywalled, and opaque mid-run "more credits" failures (e.g. https://github.com/pullfrog/app/actions/runs/25531633203) get a clear PR comment instead of a generic stack-trace dump. Policy matrix: - Auto-reload accounts: `wallet + autoReloadAmountCents` (default $50, no cap) - Card + no-autoreload: `wallet + $5` overdraft buffer - No card: `wallet` (no buffer; existing zero-balance 402 stays) - OSS: `$10` (unchanged) Removes the $25 per-run cap entirely. Long Build runs at high-balance accounts no longer silently cap at $25. Other changes: - Classify mid-run OpenRouter "requires more credits, or fewer max_tokens" errors as `router_keylimit_exhausted` BillingError so users get an actionable PR comment. - Override OpenCode `max_tokens: 32000` default to `5000` via OpenCodeConfig.limit.output. Drops Opus per-call upfront budget reservation from ~$2.40 to ~$0.38 — what makes low-wallet runs viable at all. - Switch `findInitialComment` and `findExistingPaywallComment` to GraphQL `issueOrPullRequest(number:) { comments(last: 100) }` (single round trip, actually returns newest-100; REST listComments doesn't support sort/direction). Also fixes a latent `comments.find()` returning the OLDEST match instead of the most recent — now selects max(databaseId). - Wrap `syncAccountUsage` in `prisma.$transaction` with `SELECT ... FOR UPDATE` on the account row. Pre/post-balance reads inside the transaction enable deterministic low-balance edge detection (currently logs; will push the outreach.low_balance task once #592 lands). Plan: .cursor/plans/router-low-balance-paywall.plan.md (in companion wiki-billing branch)	2026-05-08 20:15:47 +00:00
Colin McDonnell	9d04cad360	drop legacy summaryCommentNodeId column (#617) ... Was retained on `workflow_runs` after PR #568 replaced the comment-based summary path with the snapshot architecture, with a "kept for backfill of pre-snapshot runs" annotation. No backfill is planned: pre-snapshot summary comments were written in the user-facing PR_SUMMARY_FORMAT (TL;DR + key changes blockquote + before/after sections), not the agent-context functional-summary format the snapshot now expects. Backfilling them would prime new runs with the wrong shape and pollute the agent context. Old comments stay on github.com as historical artifacts; the column on the DB row is dead weight. Strips the field from: - prisma schema + new migration `20260508190000_drop_summary_comment_node_id` - `app/api/workflow-run/[runId]/route.ts` STRING_FIELDS allowlist - `action/utils/patchWorkflowRunFields.ts` type union + STRING_KEYS - `utils/db/selectActiveWorkflowRuns.ts` select clause - `utils/github/enrichWorkflowRunsWithArtifactUrls.ts` node-id type, URL resolution, collectUniqueNodeIds + urlsForRun - `utils/webhooks/handleWorkflowRunWebhook.ts` two select clauses, the hasRecordedArtifact param, and the orphaned-leaping-comment alert text - `components/RunArtifactPills.tsx` ArtifactKey union + ARTIFACT_KEYS + switch cases (drops the "View summary" chip from the workflow run list) Verified: pnpm typecheck clean, pnpm lint clean (537 files), action build clean. Dev DB reset against production parent and the migration applied cleanly — column is gone from the workflow_runs table.	2026-05-08 19:47:38 +00:00
Colin McDonnell	e4e93ea6d3	PR summary as agent-edited tmpfile snapshot (#568) ... * PR summary as agent-edited tmpfile snapshot Replaces the comment-based PR summary path (and the in-progress update_pr_summary tool from #534) with a snapshot file the agent edits in place during Review / IncrementalReview / pr-summary Task runs. The server seeds the tmpfile with the previous snapshot (incremental) or a stable scaffold (first run), exposes the path via select_mode, and reads it back at end-of-run to persist to WorkflowRun.summarySnapshot and (when the prSummaryComment toggle is on) splice into the PR description body. Why a tmpfile rather than a tool call: incremental snapshot edits are output-token-cheap when the agent uses native file-editing tools, and range-diff cleanly across runs because section headings are stable. The agent never has to regurgitate the full snapshot to update it. Gating: snapshot generation is opt-in via either prSummaryComment="enabled" (splice into PR body) or prReReview="enabled" (snapshot feeds future incremental review runs as context). Users who disable both pay nothing end-to-end — no seeding, DB write, or body splice. Behavior changes: - Drop the Summarize mode and the Summary comment type entirely; the rolling summary is no longer a separate run shape. - pull_request_synchronize with re-review off and summary on still dispatches a silent pr-summary Task, but it edits the snapshot file instead of posting a fresh comment. - /api/repo/.../pr/.../summary-comment now returns { snapshot: string | null } from the DB instead of fetching a comment via GraphQL. URL kept stable so deployed older actions degrade gracefully. - summaryCommentNodeId is retained on WorkflowRun for legacy data and a future backfill of pre-snapshot comment-based summaries. Supersedes #534. The commit-tool/sub-agent direction in that PR is abandoned in favor of this file-based shape. * address review pass #1: synchronize fallback, splice idempotency, docs * address review pass #2: in-flight skip should not race summary fallback * address review pass #3: signal-handler flush, doc clarifications * address review pass #4: in-flight persist promise + bounded body-splice timeout * address review pass #5: defensive catch on persist worker, doc nit * add summary-stale post-run gate When generateSummary is set, we capture the bytes of the seeded snapshot file and pass them to the agent's post-run loop alongside the file path. After each agent attempt, the loop diffs the current file against the seed; if they're byte-identical the agent never touched it, and we nudge once via a resume turn (similar to the dirty-tree gate, but soft and fire-once so smaller models that legitimately decide no edit is warranted don't burn the retry budget). Mostly defends against forgetful smaller models on the Review path — their mode prompt asks them to edit the snapshot file, but the multi-step instruction can fall through when the diff is large. * trigger: retry vercel preview build * fix(action): drop unused re-export that pulled node:fs/promises into next bundle action/internal/index.ts was re-exporting DEFAULT_PR_SUMMARY_INSTRUCTIONS from action/utils/prSummary.ts, but nothing in the next.js app imports it. prSummary.ts uses node:fs/promises, and pullfrog/internal is aliased into the next bundle by next.config.ts, which made turbopack try to resolve node:fs/promises in client chunks and fail with: the chunking context (unknown) does not support external modules (request: node:fs/promises) drop the re-export — selectMode.ts (the only real consumer) already imports it directly from action/utils/prSummary.ts. * firewall PR summary snapshot from user instructions; resurrect rich format for Review The agent-internal snapshot (the markdown file the agent edits in place across runs) is exclusively durable context for future agent runs — user-supplied summarization instructions warp it and degrade that context. Drop the prSummaryCommentInstructions read path end-to-end: - handleWebhook: stop reading prSummaryCommentInstructions, stop passing prSummaryInstructions through dispatch options - action payload + ToolState + selectMode addendum: drop the instructions appendix; the snapshot prompt is fixed, not user-shaped - TriggersSettings: drop the InstructionsEditor for prSummaryCommentInstructions - prSummary.ts: reframe DEFAULT_PR_SUMMARY_INSTRUCTIONS as agent-targeted (durable context, not human-facing prose) Prisma columns (prSummaryComment, prSummaryCommentInstructions) and the matching zod schema entry stay for graceful retreat. Separately, resurrect PR_SUMMARY_FORMAT (deleted along with the Summarize mode in the original PR) and wire it into Review mode only. Initial PR reviews now include a structured summary section in the review body using the rich format (TL;DR, key changes, ## sections with before/after, file-link trails). IncrementalReview keeps its existing terser bullet-list shape since re-review bodies are deltas, not introductions. The user-facing review summary and the agent-internal snapshot are deliberately separate artifacts with separate prompts and zero shared content. * address review comments: prompt self-consistency + stale-doc cleanup PR 568 self-review (4232488109) flagged a self-contradiction the firewall commit introduced and three stale doc references that survived. - action/modes.ts: Review-mode step 2's trivial-PR shortcut said `submit "Reviewed — no issues found." per step 5`, but step 5's rewrite removed exactly that preamble. Aligned both: trivial PRs and no-actionable-issues PRs now produce a body that opens with "No new issues found." followed by the PR summary, so the user gets the headline up front and still sees what was reviewed. - docs/pr-reviews.mdx: dropped the "customize the summary style with Summary instructions in the console" sentence (the editor was removed in the firewall commit). Replaced with a note that the snapshot uses Pullfrog's built-in format and is not user-customizable. - wiki/prompt.md, wiki/modes.md: rewrote the snapshot-prompt entries to reflect the firewall — DEFAULT_PR_SUMMARY_INSTRUCTIONS is the entire prompt, prSummaryCommentInstructions is no longer wired in. * drop orphaned prSummaryCommentInstructions column Prod audit (455 repos): 5 non-null rows on a single account, all containing the literal placeholder text from the InstructionsEditor we removed in the firewall commit. No account has an intentional preference set, so silent-ignore (the keep-for-retreat option) costs us nothing meaningful while leaving an orphan column in the schema. Drop it. - prisma/schema.prisma: remove the column - prisma/migrations/20260506000000_drop_pr_summary_comment_instructions: ALTER TABLE ... DROP COLUMN - utils/schemas/triggers.ts: drop the matching zod entry * drop body splicing; snapshot is internal-only User-visible PR summarization continues to ship in Review and IncrementalReview review bodies (which already render PR_SUMMARY_FORMAT and "Reviewed changes" respectively). The snapshot tmpfile is now purely durable cross-run agent context — seed, edit-in-place, save to DB, feed the next run. Massive simplification: the body splice mechanics, the two-toggle gating matrix, the summaryHandlingCovered race tracking, and the synchronize summary-only Task fallback all go away. Code: - prSummary.ts: drop splice/strip/marker code (`splicePrSummary`, `stripExistingSummaryBlock`, `buildSummaryBlock`, `extractPrSummary`, PULLFROG_SUMMARY_START/END). keep scaffold, instructions, seed/read. - main.ts: rename persistAndPostSummary -> persistSummary; collapse to a single DB PATCH. drop pulls.get/pulls.update, drop AbortSignal timeout, drop in-flight promise machinery, drop prSummaryToBody plumbing. - ToolState: add summarySeed (replaces local var in main.ts so persist can compare). drop prSummaryToBody and summaryPersistInFlight. - persistSummary now compares against the seed and skips the DB write with a warning when unchanged — saving the seed verbatim is either a no-op or persists the placeholder scaffold, neither useful. - postRun.ts: when summary-stale is the only failing gate and the resume turn itself fails, restore the pre-resume successful result and break. symmetric with the existing reflection-failure preservation. summary-stale can no longer flip a successful run to failed. Webhook: - pull_request_opened: generateSummary follows prReReview only (the snapshot has no consumer when re-review is off). - pull_request_synchronize: collapses to "if prReReview enabled, dispatch IncrementalReview". the summaryHandlingCovered flag, the same-SHA/in-flight coordination it was protecting, and the summary-only Task fallback all delete cleanly. UI / config: - drop SummarizePRsTrigger (the toggle gated body splice; with that gone it has no behavior). drop sidebar entry, console import, Text icon import. - drop prSummaryComment from triggers zod schema, prisma schema, preview settings script. Migration: squash the two existing migrations into one timestamped 20260507000000_pr_summary_snapshot covering all three column changes (add summarySnapshot on workflow_runs, drop prSummaryCommentInstructions and prSummaryComment on repos). repo convention is one migration per PR. Action: bump 0.0.203 -> 0.0.205 (payload contract changed: prSummaryToBody removed; main is at 0.0.204). Out-of-diff cleanup: - review.ts:190 + review.test.ts:651 — "Reviewed — no issues found." -> "No new issues found." to match the canonical body in modes.ts. Verified: pnpm typecheck clean, pnpm lint clean, postRun + review tests pass, dev DB reset against production and the squashed migration applied cleanly (summarySnapshot present, prSummaryComment / prSummaryCommentInstructions both gone). * re-orient snapshot toward functional summary; drop prior-review-feedback section Empirical audit on preview-568 PR #5 showed the snapshot IS load-bearing for the orchestrator: lens-dispatch prompts on incremental runs carried forward context from the snapshot's risk register (e.g. "the JSDoc explicitly scopes to code points — do not flag grapheme-cluster issues" on the surrogate-pair fix run, "consistency with native padStart" on the padStart-added run). The orchestrator was reading the snapshot, reasoning about it, and using it to anti-prime / focus subagents — exactly the high-leverage path. My earlier "snapshot is write-only" claim was wrong. The shape, however, was steering it toward review-history-log instead of functional summary. This commit re-orients: - prSummary.ts: replace the four-section scaffold (~580 chars of placeholder italics under "What this PR does / Key changes / Risk / Reviewed in prior runs") with a minimal seed (~150 chars: just a header + a one-line comment about what the file is for). different PRs warrant different organization; forcing a refactor and a feature into the same template is procrustean. minimal seed also makes the unchanged-from-seed gate in persistSummary more sensitive. - selectMode.ts addendum: rewrite around three principles. (1) the snapshot is a FUNCTIONAL summary of what the PR does and the risks it carries, not a chronological review log — commit history can already be reconstructed from list_pull_request_reviews. (2) the orchestrator should USE the snapshot during triage and dispatch — concrete example given of carrying snapshot context into subagent lens prompts. (3) structure is the agent's call; stable headings make snapshots range-diff cleanly when they fit, but riff when they don't. - modes.ts IncrementalReview: drop the "Prior review feedback" checklist from the user-facing review body (step 6b gone, step 7 ELSE IFs cleaned up). It duplicated content that's already covered by the Reviewed-changes bullets and tracked durably in the snapshot for the next agent run; in the user-facing body it was noise. step 3 still fetches prior reviews but its role is now just filtering aggregation in step 5, not rendering. - AGENTS.md: codify "no follow-ups" rule. when an issue is identified during code review, fix it in this PR — PR scope does not constrain quality. follow-up TODOs are forbidden as a substitute for doing the work now. Empirical evidence supporting the re-orientation: - Run 25568912293 (PR#5 incr1, surrogate-pair fix): orchestrator's correctness lens dispatch said "Do NOT flag grapheme-cluster issues — the JSDoc scopes to code points." The grapheme-cluster framing was not in the diff; it was downstream of the snapshot's prior risk-section framing of truncate's contract. Snapshot influencing dispatch. - Run 25569054779 (PR#5 incr2, padStart added): orchestrator's correctness lens dispatch enumerated edge cases including "consistency with native String.prototype.padStart contract" and "fill = multi-code-point string (e.g. emoji)". Both threads carried over from the snapshot's prior truncate code-point-vs-code-unit discussion. Snapshot informing the shape of what was looked for. The cost of maintaining the snapshot (~800 tokens, ~$0.005/run) is trivially affordable when it materially improves orchestrator triage on the 1-5 lenses dispatched per review. v0.0.205	2026-05-08 19:28:24 +00:00
Colin McDonnell	ae8a634450	action: quieter, deep-linked billing error comments (#600) ... * action: quieter, deep-linked billing error comments The PR progress comment for billing errors led with a loud `### ❌ Pullfrog billing error` H3 and pointed at the bare `/console` index page regardless of which org owned the repo. Make the copy quieter and more actionable: - bold first line instead of an H3 (the comment already has Pullfrog branding in the footer, no need for a second header) - thread `runContext.repo.owner` into the formatters and deep-link to `pullfrog.com/console/<owner>#billing` (or `#model-access` for the router-needs-card branch) - split the old "insufficient balance" default into two branches: card declined (Stripe returned a declineCode — "we'll retry next run") vs. balance empty (no in-flight charge — "top up or enable auto-reload") - strip UX framing and pullfrog.com URLs from the proxy-token 402 responses; they're now terse signal-only strings, with all copy and links rendered by the action so there's a single source of truth * proxy-token: return 503 on phase-1 txn failure, not 402 Phase-1 only fails on server-side issues (serializable retry exhaustion, Prisma/DB flake) — no Stripe call has happened yet, so it's not a billing decline. Pre-PR this rendered as the generic "billing error — manage billing" copy, which was vague-but-not-wrong; under the new copy it would falsely tell the user their balance is empty. Returning 503 routes the action through TransientError ("temporarily unavailable, retry") which is the accurate framing. Caught by Pullfrog review on PR #600.	2026-05-07 21:40:07 +00:00
Colin McDonnell	cd9e00f8d6	test(catalog): refresh latest-model snapshot for google (gemini-3.1-flash-lite)	2026-05-07 21:29:11 +00:00
Colin McDonnell	f87e0f878c	action: minimize pullfrog.yml permissions and drop actions:read (#594) ... * action: minimize pullfrog.yml permissions and drop actions:read The recommended pullfrog.yml workflow asked for a permissions block that's broader than what the action actually uses with the workflow GITHUB_TOKEN — all real work (git push, PR comments, reviews) goes through installation tokens that the action mints via OIDC. Customer security scanners flagged the workflow-level block as too permissive. - Move permissions to the job level and reduce to id-token: write, pull-requests: write, issues: write. contents:read is the implicit default and covers actions/checkout; contents:write, checks:read are unused by any GITHUB_TOKEN consumer; actions:read was only used by post-cleanup's listJobsForWorkflowRun call. - Replace listJobsForWorkflowRun with a SIGTERM/SIGINT handler in main.ts that calls core.saveState("cancelled", "true"); post-cleanup reads it back via core.getState. Same cancel-vs-failure UX, no extra scope needed. - Sync the docs (headless-action, getting-started, action/README) and the two dogfood pullfrog.yml workflows to the new minimal block. Update the post-cleanup wiki to describe the saveState approach. * action: drop pull-requests/issues from required workflow scopes Switch postCleanup.ts to mint its own short-lived installation token via OIDC (acquireNewToken with issues:write + pull_requests:write) instead of using the workflow GITHUB_TOKEN. Same comment-update behavior, but the workflow no longer needs those scopes — the only permissions Pullfrog ever asks for are id-token:write (OIDC exchange) and contents:read (actions/checkout). Also fixes a bug from the previous commit: setting an explicit permissions block drops every unlisted scope to none (with metadata as the only exception), so omitting contents would have broken actions/checkout. Restored at both workflow and job level. * action: scope id-token:write to pullfrog job, not workflow level id-token:write is the powerful one — it lets a job mint OIDC tokens that can be exchanged for cloud credentials or our installation tokens. Keeping it at workflow level means any future job added to this file silently inherits it. Move it to the job level where it's actually used; leave only contents:read at workflow level as a safe baseline for any future jobs. * action: move stuck-comment cleanup server-side, drop write perms entirely The action's post-cleanup step lived inside the runner and used the workflow GITHUB_TOKEN to update the "Leaping into action…" progress comment when a run failed/cancelled, requiring pull-requests:write + issues:write at the workflow level. Move that responsibility to the workflow_run.completed webhook handler: it already has installation-token access via the GitHub App, runs server-side (no Pullfrog API dependency loop on failure), and lets us drop both write perms. Recommended workflow permissions block is now truly minimal: permissions: contents: read jobs: pullfrog: permissions: id-token: write contents: read Server side - handleWorkflowRunCompleted: when conclusion != "success" and the WorkflowRun has progressCommentId, mint installation octokit and update the stuck comment in place. Try issues.getComment first, fall back to pulls.getReviewComment on 404 (we don't store comment type — one wasted GET on the rarer review case). - Reuses buildPullfrogFooter and updateProgressComment from pullfrog/internal, matching the wording the action used to write client-side. Client side - Delete action/utils/postCleanup.ts and action/post.ts. - Remove post: + post-if: from action/action.yml. - Drop runPostCleanup wiring from action/commands/gha.ts and action/play.ts. - Remove the SIGTERM/saveState handler I added in main.ts in the previous commit (no longer needed; cancel/fail signal comes from the webhook hook payload). Plumbing - Extract isLeapingIntoActionCommentBody into action/utils/leapingComment.ts so the predicate can be re-exported via pullfrog/internal without dragging the MCP server's transitive type graph into the Next.js app's typecheck. - mcp/comment.ts re-exports from the new location for backward compat. Wiki - Delete wiki/post-cleanup.md (obsolete; cleanup is now a one-liner branch in the workflow_run webhook handler). * chore: ignore .worktrees in biome config Recently-added pnpm worktree feature creates nested git worktrees under .worktrees/, each with their own biome.jsonc declaring root. Biome's recursive scan trips on the nested config and fails pnpm lint. Excluding the directory matches the existing .gitignore entry. * fix: address PR #594 review findings Two real bugs caught by code review: 1. handleWorkflowRunWebhook.ts:323 — drop the /m flag on the stuck-comment detection regex. With /m, ^ matches any line start, so any finalized progress comment that embeds a task list (report_progress writes `- [x]`/`- [ ]` lines via todoTracking.ts) would be flagged as "stuck" and silently overwritten with the "This run croaked" boilerplate whenever the workflow concluded non-success after the agent's final summary already landed. Restores the body-start anchoring the original in-process postCleanup.ts:90 had. 2. action/scripts/check-entrypoint-imports.ts — drop ../post.ts from the esbuild entry-point list (the file was deleted in aa43b9af). The `pnpm check:entrypoints` step in test.yml would have failed on every run with an unresolvable-entry-point error. Plus three small follow-ups: - main.ts:580 — comment said "post-cleanup has its own verify-retry loop" but post-cleanup is gone. Updated to describe the new server-side path. - mcp/comment.ts:443 — comment said "so post script doesn't think the run failed". Updated to describe the actual current consumers of wasUpdated. - commands/gha.ts:84 — `--post` help text said "run post-cleanup flow" but with the post-cleanup path removed, --post is only valid alongside the `token` subcommand for installation-token revocation. Updated wording. * fix(action): scope --post help text to gha token subcommand Root gha help text was documenting --post, but --post only makes sense paired with the token subcommand (it's how the post step revokes the installation token previously acquired in the main step). Move it to a dedicated gha token help section and add a parser layer that rejects --post on the bare gha command. $ pullfrog gha --help usage: pullfrog gha [subcommand] ... options: -h, --help show help $ pullfrog gha token --help usage: pullfrog gha token [--post] ... options: -h, --help show help --post revoke the previously-acquired token (post-step usage only) * webhook: artifact-aware cleanup of stranded leaping comments on success Previously the workflow_run.completed cleanup only handled non-success conclusions. Extend it to also catch the rare case where a successful run leaves a "Leaping into action…" comment stuck (in-process cleanup at action/main.ts:723 normally handles this, but can be skipped on SIGKILL, runner host crash, or any exit path that bypasses main()'s finally block). New behavior in cleanupStuckProgressComment: - cancelled → update with "cancelled 🛑" body (unchanged) - failure (other) → update with "croaked 😵" body (unchanged) - success + artifact recorded → delete the comment (the artifact is the user-facing surface; the leaping comment is just stale UI noise at this point) - success + no artifact recorded → delete the comment AND alert team@pullfrog.com via emailAlert The "success + no artifact" path is "should never happen" territory: the run claims success but produced no review, PR, issue, plan, or summary comment. The team alert helps us catch in-process cleanup regressions or artifact-tracking gaps. hasRecordedArtifact reads {review,pr,issue, planComment,summaryComment}NodeId off the WorkflowRun row to make the call. * webhook: narrow stuck-comment detection to leaping prefix only Drop the stranded-todo-pattern branch from cleanupStuckProgressComment. The leaping prefix is highly specific and impossible to confuse with a legitimate summary; a leading todo line is not — the agent's error-reporting paths can produce useful explanatory comments whose body leads with a checklist (e.g. "here's what I was working on" + the incomplete todo list), and we don't want to silently overwrite those with the generic "croaked" boilerplate. In-process cleanup at action/main.ts:723 still handles the stranded-todo case in the common path (gated on !finalSummaryWritten with full access to the in-memory tool state). Missing the rare runner-died-mid-todo case server-side is a worthwhile trade vs. the false-positive risk on real explanatory comments.	2026-05-07 18:59:52 +00:00
Colin McDonnell	e2e29a19fc	accept pullfrog.yaml as well as pullfrog.yml (#596) ... * accept pullfrog.yaml as well as pullfrog.yml centralize the accepted workflow filenames in `utils/github/pullfrogWorkflow.ts` (`PULLFROG_WORKFLOW_FILES = ["pullfrog.yml", "pullfrog.yaml"]`) and use the new `findExistingWorkflowFile` helper at every read path: `getWorkflow` (cached), the verify-workflow API route, and the audit/sync/download/update scripts. `.yml` is always tried first so the common case still costs exactly one API call. webhook handlers (push cache-bust, `workflow_run_*`) now use the shared `isPullfrogWorkflowPath` matcher. action runtime (`reviewCleanup.ts`) derives the running workflow's filename from `process.env.GITHUB_WORKFLOW_REF` instead of hardcoding `.yml`, so the safety-net follow-up dispatch targets whichever file the user actually has — strictly more correct than today. write paths (`createWorkflowForRepo`, `createWorkflowPR`) intentionally still create `.yml`; existing 422 collision handling covers the rare double-install case. UI/wiki/onboarding copy keeps saying `pullfrog.yml`; one callout in `docs/getting-started.mdx` mentions `.yaml` works too. also drops dead code (`utils/github/findWorkflow.ts`, parallel single-file implementation with no importers) and the now-unused `WORKFLOW_FILENAME` export. * rename pullfrogWorkflow.ts -> findPullfrogWorkflow.ts (verb form) * add pre-flight check to workflow create paths `createWorkflowForRepo` and `createWorkflowPR` now check for any existing pullfrog workflow file (`.yml` or `.yaml`) before doing work, preventing the degenerate state where a repo with `pullfrog.yaml` ends up with both files dispatching on every event. costs one `getContent` call per first-time install. existing 422 branch in `createWorkflowForRepo` is retained as a race-condition safety net; the 409 branch now also handles the case where `createWorkflowPR` discovers an existing file in flight. `createWorkflowPR` return shape becomes a discriminated union; the standalone `/api/create-workflow-pr` route returns `{ alreadyInstalled: true }` instead of creating a redundant PR. * promote repo to active when /api/create-workflow-pr finds existing workflow extracts `promoteRepoToActive` from `createWorkflowForRepo`'s closure to a shared module-level function, and wires it into the standalone PR route's `alreadyInstalled` branch so a `needs_setup` repo with an existing `.yaml` file doesn't go stale (was only handled by the dashboard's own create path). addresses pullfrog review on #596.	2026-05-07 18:04:07 +00:00
pullfrog[bot]	6f76a6a9da	fix(action): tighten provider error detection and propagate agent error events (#580) ... * fix(action): tighten provider error detection and propagate agent error events Both bugs from #562: 1. detectProviderError used substring matches against "429", "rate limit", etc. — false-positives on commit SHAs containing 429 and on x-ratelimit-* response headers in dumped 401 error JSON. rewrote with anchored regexes: numeric status codes only match adjacent to a recognised status key, and `\brate[_ ]limit(?=[_ ]|\b)` rejects ratelimit-* headers (no separator). word-boundary anchors on INTERNAL / UNAVAILABLE / quota / limit:0 reject INTERNAL_SERVER_ERROR / time_limit:0 substrings. added 11-case regression test. 2. opencode 401s slipped through `eventCount === 0 && lastProviderError` because opencode's own type=error event increments eventCount before the guard runs. added an explicit `error:` handler that captures the event and propagates it to a non-success AgentResult. opencode emits the message under `error.data.message`, not the top level. mirror fix in claude.ts: error_max_turns / error_during_execution / any error* subtype on the result event now flips success: false. * fix(action): match quota inside identifiers like insufficient_quota \bquota\b missed insufficient_quota / quota_exceeded / quotaExceeded because _ is a word character and camelCase has no boundary. quota is specific enough to be matched as a plain substring. * fix(action): match `rate limited` and `rate limits exceeded` Drop the trailing `(?=[_ ]|\b)` lookahead from the rate-limit regex. The lookahead failed when `limit` was followed by another word character (`limited`, `limits`), so `rate limited` and `rate limits exceeded` were slipping past detection. The leading `\b` plus `[_ ]` separator already rejects `x-ratelimit-*` / `anthropic-ratelimit-*` headers without it. --------- Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass <david@arktype.io>	2026-05-07 16:31:05 +00:00
pullfrog[bot]	366af55f19	fix(action): sweep stale .git/*.lock and deepen-retry shallow git_fetch (#564) (#578) ... - checkoutPrBranch now removes .git/shallow.lock, .git/index.lock, and .git/objects/maintenance.lock when older than 30s before the first fetch. prior runs that crashed mid-fetch left these behind on self-hosted runners, causing checkout_pr to abort with `Unable to create '.git/shallow.lock': File exists` until the agent shelled out to rm -f. - GitFetchTool catches `Could not read <sha>` and `remote did not send all necessary objects` on shallow clones and retries once with --deepen=1000 instead of bouncing the failure back to the agent. agents previously had to fall back to checking out FETCH_HEAD, losing branch context. Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass <david@arktype.io>	2026-05-06 22:38:41 +00:00
pullfrog[bot]	4c1413d925	fix(action): flip wasUpdated on substantive MCP write tools (#563) (#577) ... * fix(action): flip wasUpdated on substantive MCP write tools (#563) Review/Respond/etc. agents that submit a `create_pull_request_review`, `create_issue_comment`, or `update_pull_request_body` and exit without calling `report_progress` were being marked as workflow failures by the strict completion check in handleAgentResult. Extend the set of tools that flip toolState.wasUpdated so a substantive user-visible artifact satisfies the check. The isReviewMode bypass is retained for IncrementalReview's non-substantive path. Flag is set BEFORE patchWorkflowRunFields / deleteProgressComment in each tool so a best-effort cleanup failure does not undo the signal. * fix(action): use finalSummaryWritten for stranded progress cleanup The stranded-progress-comment cleanup at the end of main() previously fired only when toolState.wasUpdated was false (or the tracker was the last writer). With wasUpdated now set by additional MCP write tools (create_issue_comment, update_pull_request_body), an agent that produced a substantive artifact via one of those tools and skipped report_progress would leave the placeholder "Leaping into action" comment intact — the post-script then converted it into an error message on a successful run. Key the cleanup off finalSummaryWritten instead. That flag is only set when report_progress actually wrote the progress comment, so it cleanly distinguishes "comment is finalized" from "agent did other work but never touched the progress comment". * refactor(mcp): extract markSubstantiveArtifact() helper replaces 4 inline `ctx.toolState.wasUpdated = true` flips in CreateCommentTool, UpdatePullRequestBodyTool, and CreatePullRequestReviewTool with a single helper in mcp/server.ts. JSDoc on the helper documents the contract (call BEFORE downstream patch/cleanup; gates the strict completion check and stranded-comment cleanup) so future MCP write tool authors only need to grep for one symbol. no behavioral change. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(mcp): only flip finalSummaryWritten after non-skipped write Previously the flag was set unconditionally on any non-plan call, including paths where reportProgress skipped (silent events, deleted comment, no issue/PR target). The cleanup check in main.ts is safeguarded by toolState.progressComment so the bug doesn't manifest today, but aligning the flag with actual writes matches the wasUpdated pattern and the design intent in the cleanup plan. * refactor(mcp): inline markSubstantiveArtifact helper --------- Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass <david@arktype.io> Co-authored-by: Cursor <cursoragent@cursor.com>	2026-05-06 21:05:53 +00:00
pullfrog[bot]	6db4a6d02e	fix(mcp): preserve coveragePreflightRan across checkout_pr refreshes (#576) ... checkout_pr unconditionally rebuilds ctx.toolState.diffCoverage via createDiffCoverageState, which initialised coveragePreflightRan to false. a second checkout_pr therefore reset the "one-time nudge per review session" guarantee in runDiffCoveragePreflight, and the next create_pull_request_review threw the diff-coverage pre-flight error again — even after the agent had already gone through the read-and-resubmit dance once. createDiffCoverageState now accepts an optional previous state and carries forward coveragePreflightRan. coveredRanges are intentionally not carried because their line numbers are tied to the previous diff's content (especially under incremental diffs). closes #566 Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass <david@arktype.io>	2026-05-06 20:54:34 +00:00
Colin McDonnell	3c8b493aee	modes: soften "two-out-of-three" rule from veto to look-harder signal ... The previous phrasing ("not enough — still degrades the codebase") read as a categorical claim that elegance vetoes correctness, which inverts the usual hierarchy and risks giving the agent a clean rationalization for rejecting genuine correctness fixes. Reframe as a prompt to keep searching for a fix that gets all three before accepting the trade — preserves the pressure without the absolute.	2026-05-06 03:01:06 +00:00
Colin McDonnell	560e27bda5	refactor progress comments into a bundled type + helper module (#567) ... * refactor progress comments into a single bundled type + helper module introduce ProgressComment ({ id, type: "issue" | "review" }) as the canonical handle for the GitHub comment a run uses to report progress, and route every read/update/delete/create through a single helper module (action/utils/progressComment.ts). previously every site that touched the progress comment hardcoded octokit.rest.issues.*Comment, which made adding a second comment type (review-thread replies) require duplicating the same branch in 6+ places — the same shape that bit pullfrog/app#445. new capability: when the address-reviews trigger fires for a one-off review comment, the "Leaping into action" comment is now posted as a reply in that review thread instead of as a top-level PR timeline comment. the helper handles failure (e.g. parent comment deleted) by silently falling back to a top-level issue comment, so the run never loses its progress surface. changes: - action/utils/progressComment.ts (new) — ProgressComment type + getProgressComment, updateProgressComment, deleteProgressCommentApi, createLeapingProgressComment. uses a structural Octokit interface to bridge the @octokit/rest version mismatch between the action package (v22) and the root project (v21). - action/internal/index.ts — re-export the new types and helpers for cross-boundary use. - action/external.ts, action/utils/payload.ts — replace progressCommentId: string with progressComment: { id: string, type: "issue" | "review" } in WriteablePayload + JsonPayload. wire-format breaking, no legacy fallback (in-flight runs across the deploy lose their progress comment, fine). - action/mcp/server.ts — ToolState.progressCommentId becomes progressComment: ProgressComment | null | undefined (same tristate semantics). - action/main.ts, action/mcp/comment.ts, action/utils/errorReport.ts, action/utils/postCleanup.ts — every issues.*Comment call against the progress comment routes through the helper module. zero hardcoded API branching outside the helper. - utils/github/triggerWorkflow.ts — drop createLeapingComment + updateCommentToLeaping; dispatchAndTrackWorkflow gains a resolution chain (existingComment → replyToReviewComment → triggeringIssue → none) and an existingComment: ProgressComment param plus replyToReviewComment: { pullNumber, commentId }. - utils/webhooks/handleWebhook.ts — dispatch closure threads replyToReviewComment through; the one-off review comment branch passes it and skips the now-redundant eyes reaction on the comment we're about to reply to. - app/trigger/[owner]/[repo]/[number]/page.tsx, utils/github/runActionLocal.ts, app/api/cli/dispatch/route.ts, app/api/dispatch-workflow/route.ts — call sites updated to new shape. no schema or DB column changes. the existing WorkflowRun.progressCommentId column is still written by id only; type lives only on the in-flight payload, which is sufficient for runtime since it's the only thing that needs to know which API to call. * anneal pass 1: fallback visibility + stale doc/comment updates - progressComment.ts: when reviewReply→issue fallback fires, prepend a [!NOTE] callout with a permalink back to the original review comment. without this, the parent comment showed no eyes reaction (deliberately skipped) and no reply, leaving the user with no signal that anything happened. - wiki/post-cleanup.md: update progressCommentId references to progressComment, document the new helper-based dispatch by type. - wiki/main.md: update initToolState({ progressCommentId }) → ({ progressComment }) in the resolver-chain diagram. - action/main.ts, action/mcp/review.ts: update two stale comments that referenced the old field name. * anneal pass 2: post-cleanup detection through fallback notice + log cleanup - isLeapingIntoActionCommentBody: strip a leading GFM blockquote/alert before testing the leaping prefix. without this, the [!NOTE] callout that the reviewReply→issue fallback prepends would prevent post-cleanup from recognizing the stuck "Leaping into action..." comment, leaving it permanently on the PR timeline if the workflow died before any progress update. - progressComment helper: switch from log.warning (action-flavored, emits a ::warning:: GitHub Actions annotation) to console.warn so the helper doesn't pollute Vercel logs when invoked from the webhook context. - triggerWorkflow.ts: drop the duplicate caller-side log on review-reply failure — the helper already speaks loudly. Reword the catch-branch log to reflect that it now only fires when both the reply AND the helper's internal fallback failed. - progressComment.ts: document that the [!NOTE] fallback notice is overwritten on the first report_progress call, and explain the trade-off vs persisting it through the action payload + ToolState. * debloat: drop the [!NOTE] fallback callout Reverting two pieces from the prior anneal pass: - progressComment.ts: drop the [!NOTE] callout that the reviewReply→issue fallback prepended to the leaping body. It disappeared on the agent's first report_progress call, which made it half-committed to visibility — worse than either properly persisting it (real engineering) or leaving the fallback silent (current choice). The console.warn diagnostic and the workflow-run footer link in the leaping comment itself give us enough signal for the rare case where both API endpoints fail at once. - isLeapingIntoActionCommentBody: revert the leading-blockquote stripping; only needed to compensate for the [!NOTE] callout. Keeping: the console.warn-vs-log.warning fix (real cross-runtime concern), the duplicate-log drop in triggerWorkflow.ts, the wiki updates, and the two stale source-comment fixes. * fix: prevent stranded task list overwriting post-cleanup message When a run is cancelled, the action's todoTracker may have an HTTP write in flight to GitHub when SIGTERM lands. The action process dies, but the request data has already left the socket — GitHub processes it and updates the comment body to the (stale) task list. Meanwhile post-cleanup, running in a separate process, writes the "This run was cancelled 🛑" message. If the tracker's in-flight write happens to land *after* post-cleanup's write, the user never sees the cancellation message. Two-layer fix: - Action side: cancel the tracker in the SIGTERM signal handler so no new debounced writes get scheduled. This shrinks the race window but can't un-send a request already on the wire. - Post-cleanup side: after writing, verify the body landed and re-issue if another write clobbered ours. Loops up to 3× with a 3s settle delay so delayed in-flight writes from the dying action have time to arrive before our read-back check decides whether to retry. * lint: import createLeapingProgressComment from pullfrog/internal in test script * address bot review findings: reply-target root, version bump, GET error handling Three real findings from the bot reviews on #567 plus a small DRY pass: 1. handleWebhook reply-target: `newComments[0]` may be a reply, not a top-level review comment. `getReviewCommentsWithReplies` returns root + replies for any thread the review touched, and `pull_request_review_id` filtering only narrows by *which review submitted*, not *root vs reply*. When a user submits a single reply as their entire review (e.g. replying to someone else's comment to ping @pullfrog), the reply ID flowed through to `createReplyForReviewComment`, which 422s on replies-to-replies and degraded to a top-level issue comment — exactly the polluted-PR-timeline behavior this PR was built to remove. Walk up `in_reply_to` from the already-fetched thread data to find the root and reply there instead. 2. action/package.json: bumped 0.0.202 → 0.0.204. main is at 0.0.203 and our wire format changed; without a bump validateCompatibility can't surface the mismatch on the deploy boundary, and the merge would have gone backwards. 3. postCleanup writeAndVerify: distinguish a thrown verify-GET from a "body got overwritten" mismatch. Treating a transient 5xx/rate-limit GET the same as a clobber wasted PUT attempts and printed a misleading "in-flight writes kept clobbering us" warning. We trust our PUT (which returned 200) and exit instead of amplifying writes against a flaky API. 4. Small DRY: extracted parseProgressComment for the `{ id: string; type } -> ProgressComment` parse that had drifted across server.ts and postCleanup.ts. v0.0.204	2026-05-06 01:50:58 +00:00
Colin McDonnell	1e17a76863	bump xai/grok to 4.3 and grok-fast to 4-1-fast ... #1 generational bump on both. xAI shipped grok-4.3 on 2026-05-01 and grok-4-1-fast on 2025-11-19; both are same brand tier as the existing slugs (`grok` and `grok-fast`), so resolve + openRouterResolve update in place with no DB migration needed. Mirrored on the openrouter provider side (openrouter/grok now also points at x-ai/grok-4.3). OpenRouter spells the fast variant `x-ai/grok-4.1-fast` (dot) where models.dev uses `grok-4-1-fast` (dash) — verified both forms against their respective live APIs before committing. See the "naming traps" section in wiki/models-catalog.md. Snapshot regenerated: openrouter latest-GA shifted from poolside/laguna-xs.2:free (2026-04-28) to x-ai/grok-4.3 (2026-05-01) as a mechanical consequence of the bump. Verified via `pnpm -C action test:catalog` (139/139 pass against live models.dev + OpenRouter API) and `pnpm -C action test` (458/458). Considered and explicitly rejected during this audit (recording for future archaeology): - Re-adding opencode/nemotron-3-super-free: removed twice in 71dff24c and 0f8117af with no commit-message rationale, but the removals are intentional per maintainer. - Adding gpt-nano (openai + opencode + openrouter) at gpt-5.4-nano: the snapshot has been silently tracking opencode/gpt-5.4-nano since 7dd80143 (2026-03-18) without a corresponding catalog addition — a deliberate non-add. Also would have collided with the existing opencode/gpt-5-nano displayName "GPT Nano". - Adding opencode/hy3-preview-free: never been in the catalog on main and no positive signal beyond models.dev availability. - Bumping opencode/gpt-5-nano (free) to opencode/gpt-5.4-nano: would silently turn a free alias paid ($0.20/$1.25 per M tokens) — not a generational bump, would require retire-and-replace if pursued.	2026-05-05 23:40:00 +00:00
David Blass	ada5584737	test(mcp): make checkout/reviewComments tests offline (fixture-driven) (#575) ... `mcp/checkout.test.ts` and `mcp/reviewComments.test.ts` previously hit live GitHub on every run via `acquireNewToken()`, requiring `GH_TOKEN` or `GITHUB_APP_ID` + `GITHUB_PRIVATE_KEY` in the env. that made them: - cred-gated — the action runtime filters `_KEY$` / `_TOKEN$` from subprocess env, so the husky pre-push hook (which runs `pnpm -r test`) blocked Pullfrog agents from pushing branches. issues #562, #563, #564, #566 all hit this exact blocker and never got their fixes pushed. - non-deterministic and slow (network round-trips for a snapshot test). both tests are really snapshot tests of pure formatters (`formatFilesWithLineNumbers`, plus `parseFilePatches` / `buildThreadBlocks` / `formatReviewThreads` for review data). the live fetches were just an inefficient way to obtain fixtures. changes: 1. extract a pure `formatReviewData({ review, threads, prFiles, ... })` from `getReviewData` in `mcp/reviewComments.ts`. `getReviewData` becomes thin orchestration: fetch + call formatter. preserves the "skip listFiles when no threads" perf optimization. 2. add `action/mcp/__fixtures__/` with checked-in JSON captures for the three fixture test cases (pullfrog/test-repo#1 listFiles, pullfrog/scratch#49 review 3485940013, pullfrog/scratch#64 review 3531000326). ~14KB total. fixtures store only the fields the formatter reads — volatile fields (sha, blob_url, etc.) are dropped. 3. rewrite both test files to load the fixtures and call the pure formatters directly. snapshot keys updated; snapshot content unchanged (verified by running existing snapshots against the refactored tests). 4. add `action/scripts/refresh-test-fixtures.ts` to re-fetch the fixtures from live GitHub on demand: `node action/scripts/refresh-test-fixtures.ts` (with creds in `.env` or env). re-run when the GitHub API response shape changes and review the snapshot diff. trade-off: a silent change to GitHub's `pulls.listFiles` / `pulls.getReview` / GraphQL `reviewThreads` response shape would no longer break this test on every push. that tradeoff is worth it: shape drift on those endpoints is rare (years between changes), and a dedicated cron that runs the refresh script and opens a PR on diff is a far better signal than a flaky cred-gated pre-push hook. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-05-05 23:25:46 +00:00