Commit Graph

14 Commits

Author SHA1 Message Date
Colin McDonnell f3d18401ac eager signup credit + free-OpenCode fallback when BYOK has no key (#789)
* eager signup credit + free-OpenCode fallback when BYOK has no key

addresses the silent-churn pattern that took out 15 first-run-failure
accounts post-launch: GH Actions secret references resolved to empty
strings (because the secrets didn't exist on the repo), the action
launched Claude Code with no key, the LLM provider 401'd, and the run
died in seconds with a synthetic "Invalid API key" message. those
accounts had no Router credits to fall back to because the lazy claim
required a dashboard visit they never made.

three changes, one PR:

1. Eager $10 signup credit at account creation. Both account-creation
   sites (`upsertAccountByClerkId` for dashboard signin, `fetchOrCreateRepo`
   for CLI / GH-App-only) now insert the `CreditGrant { reason: "signup" }`
   in the same transaction as the `accounts` row. CLI installers who
   never sign in get the credit. The dashboard `/signup-credit/claim`
   POST stays as an idempotent backstop for accounts created before
   this shipped.

2. Free-OpenCode fallback in the action. When the configured BYOK slug
   needs a provider key the runner doesn't have, swap to
   `opencode/minimax-m2.5-free` before agent selection so the run still
   succeeds. Surfaced via a `» fell back from <slug> to <free>` warning
   in the action log. Skipped on Router runs (Pullfrog mints the key)
   and when no model is configured (auto-select-with-throw still fires
   for the genuinely-misconfigured case).

3. New action-test fixture `byok-no-keys-fallback` that empty-strings
   every known provider key (matching how GH Actions handles missing
   secrets) and asserts the run succeeds with the fallback log line
   present. plus a unit test for the helper covering each skip case.

skipping the schema flip from `byok` to `router` — that's coming via
the onboarding-stepper PR (#762).

* fallback: skip Bedrock + surface in PR-comment footer

addresses copilot review on #789 (real bug — parseModel throws on
Bedrock raw IDs that have no slash, would crash before
validateBedrockSetup could surface its own error) and the user-side
ask to make the fallback visible in PR comments.

- selectFallbackModelIfNeeded skips when resolvedModel has no '/' so
  Bedrock routing IDs (e.g. us.anthropic.claude-opus-4-7) don't crash
  inside hasProviderKey -> parseModel. unit test covers it.
- toolState.modelFallback records the configured slug we fell back
  from. set in main.ts when fallback engages.
- buildPullfrogFooter accepts fallbackFrom and renders
  "Using `MiniMax M2.5` (free) (credentials for Claude Opus not
  configured)" so the substitution is visible in PR comments,
  reviews, PR bodies, and error reports.
- threaded through all four action-side footer call sites
  (mcp/comment, mcp/pr, mcp/review, utils/errorReport). server-side
  call sites in triggerWorkflow.ts / handleWorkflowRunWebhook.ts
  fire pre-action and don't have toolState — left as-is.

* fallback footer: use provider display name + document email asymmetry

addresses pullfrog reviewer findings on #789:

- footer now shows 'credentials for Anthropic not configured' (provider
  display name from `providers.anthropic.displayName`) instead of the
  per-model name. credentials are provider-scoped (ANTHROPIC_API_KEY
  covers all Anthropic models), so this matches what the user actually
  needs to fix.

- document the intentional asymmetry between eager and lazy signup
  credit paths: eager skips both the signupCreditClaimedEmail and the
  per-grant team@ alert. comment explains why (the 'new account
  created' alert already covers it on the eager path; the user-facing
  email assumes a user-initiated action that hasn't happened yet for
  CLI/GH-App-only signups).

- skipping the backfill for the 15 historical accounts per user's
  earlier decision — they all uninstalled, so the cohort self-selected
  out of being reachable.

* fallback: gate on resolvedModel + skip resolveModel re-resolve post-swap

local agnostic fixture run surfaced two real bugs the unit tests didn't
catch:

1. fallback gate was on configuredSlug (=payload.model) but the test
   uses PULLFROG_MODEL to set the model, which is read by resolveModel
   AFTER its slug arg. configuredSlug stayed undefined → fallback never
   fired. drop configuredSlug from the helper signature; gate purely on
   resolvedModel since that's the same value regardless of how the
   model was specified (DB config vs PULLFROG_MODEL env).

2. when fallback engaged, the post-swap resolveModel({slug: fallback.to})
   call was ALSO honoring PULLFROG_MODEL, re-overriding the fallback
   target back to the unkeyed model. validateAgentApiKey then threw
   "no API key found" against the original model. fix: skip the
   re-resolve. fallback.to is already a CLI-ready specifier.

unit tests updated for the new helper signature (8 tests, all pass).
fallback log line confirmed emitted in the local run pre-second-fix;
the second fix unblocks the validation that previously threw.
2026-05-20 02:17:22 +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 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.
2026-05-06 01:50:58 +00:00
Colin McDonnell c6a3ee0e9a show model name in footer, drop pullfrog.com link (#484)
* show model name in footer, drop pullfrog.com link

add model slug to buildPullfrogFooter so every Pullfrog comment
displays the active model (e.g. "Using `Big Pickle` (free)" or
"Using `Claude Opus`"). remove the pullfrog.com link from all footers.

Made-with: Cursor

* reject <br/> tags in comment bodies, add prompt guidance

add runtime validation in addFooter that throws if <br/> is followed
by a non-blank line (breaks GitHub heading rendering). the agent sees
the error and retries with clean markdown. also update Summarize mode
prompt to explicitly forbid <br/> tags.

Made-with: Cursor

* fix <br/> guidance: move to event instructions, clarify blank line rule

the formatting rule belongs in DEFAULT_PR_SUMMARY_INSTRUCTIONS (event
instructions), not the Summarize mode prompt. clarify that <br/> must
always be followed by a blank line before headings.

Made-with: Cursor

* generalize block-level HTML rule in summary instructions

add a prominent top-level rule about requiring blank lines between ALL
block-level HTML elements and markdown syntax, not just <br/>.

Made-with: Cursor

* move model to toolState instead of threading through params

model is set once at startup and read everywhere — it belongs on
toolState, not threaded as a separate param through 8 call sites.
postCleanup runs without toolState so it just omits the model label.

Made-with: Cursor

* update models.dev snapshot (openai latest changed)

Made-with: Cursor

* add comment to models snapshot test explaining its purpose

Made-with: Cursor
2026-03-17 19:59:11 +00:00
pullfrog[bot] 20b08b5321 Add "Rerun failed job ➔" link to error comment footer (#355)
* add "Rerun failed job" link to error comment footer

* Remove issueNumber guard from rerun link

The rerun action only needs run_id — the issue number in the trigger
URL path is just a route segment requirement. Use 0 as a fallback
so the link is always shown when a run ID is available.

* Overload `[number]` path segment as `runId` for the rerun action

For the rerun trigger, the `[number]` path param now carries the
workflow run ID instead of an issue number. The rerun link changes
from `/trigger/o/r/ISSUE?action=rerun&run_id=RID` to
`/trigger/o/r/RID?action=rerun`.

- Remove `run_id` query param from page.tsx and searchParams type
- Add error handling around `reRunWorkflow` for invalid run IDs
- Drop `issueNumber` from `BuildErrorCommentBodyParams` and all
  rerun link builders (errorReport, exitHandler, postCleanup)

* Reorder validation: action first, then rerun, then issueNumber

* address review: remove early toolState assignment, restructure rerun into dedicated block

* revert self-contained rerun block, share auth logic via issueOrRunId

* improve error handling

* normalize runid early

---------

Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
2026-02-27 12:32:06 +00:00
Colin McDonnell 102417f442 Add post hooks for cleanup (#193)
* Add post hooks for cleanup

* Switch to signal-based cleanup

* Better exit handling
2026-01-28 01:59:15 +00:00
Colin McDonnell d98f6c8029 Switch back to grpahql for review threads 2026-01-20 23:59:52 +00:00
Colin McDonnell cb925556e8 refactor instructions to return object with full/system/user/event/runtime properties, fix duplicate modes and json prompt extraction (#110) 2026-01-16 21:43:54 +00:00
Colin McDonnell 9e019d89d2 Clean up actions and payloads (#98)
* Clean up actions and payloads

* Clean up action

* Cleanup
2026-01-16 07:16:25 +00:00
Mateusz Burzyński a483711fee Auto-retry ratelimited octokit requests 2026-01-07 17:17:20 +01:00
Colin McDonnell 26336d0ac2 Tool factories 2025-12-15 23:04:20 -08:00
Shawn Morreau fb7d9e0d34 move croaked logic, ensure API key error populates comment 2025-12-11 14:55:07 -05:00
Shawn Morreau 994d493e08 add branch logic mcp tool 2025-12-10 13:13:26 -05:00
Shawn Morreau 7f81415259 update working comment on error 2025-12-04 15:05:53 -05:00