main
323 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 wolfy
|
e8b2c9952b | fix: revert streaming, add dedup and format enforcement | ||
|
wolfy
|
e79affa257 | fix(agent): stream Ollama responses to prevent prefill timeout | ||
|
wolfy
|
a847c1f3c9 | fix: improve review inline/body discipline and agent reliability | ||
|
wolfy
|
11dc00b2b8 | chore: some more improvements to try and get as close to original as possible | ||
|
wolfy
|
1e839d36a9 | fix: review process and cleanup | ||
|
wolfy
|
f88377cd1d | chore: bump context window to match what zed uses | ||
|
wolfy
|
0dc0f7eb53 | feat: add read_file tool | ||
|
wolfy
|
3f0d9a80c7 | fix: issue with shell calls | ||
|
wolfy
|
fe85adfa53 | fix: reviews failing to call next tool | ||
|
wolfy
|
f7d59cad03 | fix: issue properly basing diffs when tagged on pr | ||
|
wolfy
|
93471c9408 | feat: handle "suggestions" better | ||
|
wolfy
|
19671c6299 | feat: branch protection + deps caching | ||
|
wolfy
|
d5d2e5b58e | fix: get diffs properly | ||
|
wolfy
|
0438688e32 | fix: issues with pagination not resolving correct url templates | ||
|
wolfy
|
2aca1a3aa3 | feat: adapt pullfrog for gitea + ollama | ||
 Colin McDonnell
|
36ac64a5b6 |
fix(oss-codex): prefer user's uploaded Codex auth over OSS subsidy (#844)
* fix(oss-codex): prefer user's uploaded Codex auth over OSS subsidy OSS-allowlisted repos with `CODEX_AUTH_JSON` uploaded via `pullfrog auth codex` were still being routed through the OSS OpenRouter subsidy because two paths ignored managed credentials: - `hasProviderKey()` only checked `provider.envVars`, so an `openai/*` model with only `CODEX_AUTH_JSON` present silently fell back to `opencode/big-pickle` via `selectFallbackModelIfNeeded` — the maintainer saw "opencode/big-pickle (resolved from openai/gpt)" on CI even though Codex was configured. - `run-context` set `proxyModel` for every OSS run unconditionally, which the action runtime threads through `payload.proxyModel` and uses to overwrite `OPENROUTER_API_KEY`. Even if `big-pickle` fallback hadn't fired, the runner would consume the $10 OSS subsidy key instead of the user's ChatGPT subscription. Fix: - Add `getModelAuthEnvVars()` covering both `envVars` and `managedCredentials` in `action/utils/apiKeys.ts`; route `hasProviderKey` + `validateAgentApiKey` through it. - `run-context` now skips `proxyModel` for OSS repos when the configured model's provider has matching auth in Pullfrog-stored account/repo secrets, so the runner authenticates directly with the user's Codex subscription (or any other user-provided provider auth). Triggered by mrlubos (`hey-api/openapi-ts`). Companion follow-up tracked for the opaque "(no error message)" classifier swallow that masked the OSS $10 cap exhaustion on PR #3872 runs 25815370844 + 25815443234. * fix(oss): force Kimi K2 for OSS proxy + hide picker in console UI OSS-funded runs were resolving `repo.model` through OpenRouter, so a single Opus / GPT-5.5 run could burn an entire `oss_subsidy` key against the per-key cap and crash mid-stream (e.g. `hey-api/openapi-ts` PR #3872 runs `25815370844` + `25815443234`, ~$9.20 each on a single key). Force `DEFAULT_PROXY_MODEL` (Kimi K2.6 — ~10-50× cheaper) for every OSS proxy mint, regardless of `repo.model`. Per-run spend stays bounded within the cap by structure, not by hope. `repo.model` stays in the DB unchanged — overriding at runtime means leaving the program restores the user's prior pick without a migration. UI: hide the model picker entirely on OSS repos in `AgentSettings`. The field is effectively inert until the repo leaves the program, so exposing it as if it were live was misleading. Replaced with a banner naming Kimi K2 and pointing to `pullfrog auth …` as the opt-out path — that lands the user on the existing #844 bug-2 branch (Pullfrog-stored auth suppresses the OSS proxy entirely; runner uses user credentials + their preferred model). ModelCostsInfo already has its own `isOss` branch for the cost copy, so that section is unchanged. * fix(oss): lowercase comment casing per AGENTS.md * fix(oss): revert banner copy to 'It's on us.' framing per review Maintainer felt 'Kimi K2' as the banner headline lost the warm 'we've got you covered' framing that the existing OSS cost banner uses. Restore 'It's on us.' as the headline, move the model name into the body where it explains the hardcoded choice and points to the opt-out (pullfrog auth codex / account secret). * docs(agents): screenshots must be of the live route, never synthetic Caught myself building a temp `/dev/oss-ui-preview` route with hardcoded JSX copy-pasted from the real component just to grab a screenshot — the result told us nothing about whether the actual integrated UI worked, and the user (rightly) called it out as a waste. Strengthen the rule: screenshots must come from the live route in the running app, driven by the actual component tree and real props. Note the GH OAuth interstitial gotcha so the next agent gets through Clerk → GitHub sign-in on the first try instead of bailing to a fake render. Also bans side-by-side comparison screenshots unless explicitly requested. * fix(oss): one 'It's on us.' banner, not two OSS Agent settings was showing the message twice — once in the Model section, once in the Model costs section right below it. Fold the cost coverage into the model banner ('at no cost to you' + the spend stat) and hide the Model costs subsection entirely for OSS. ModelCostsInfo no longer needs `isOss` / `ossSpendThisMonthUsd` props — call site is gated, so the OSS branch is dead. Removed it and the now-unused props. Non-OSS rendering is unchanged: full Model picker + Model costs subsection with Router / BYOK branches. * feat(action): corepack-aware package manager provisioning before setup customer setup scripts that did `npm i -g pnpm && pnpm install` were installing whatever pnpm "latest" happens to be on the day the run fires, not what the repo declares — and pnpm 11.3 silently writes a new `packageManagerDependencies` block into lockfiles, which the agent's "always push changes" rule then packages into a noisy PR (see #844). resolve the project's pnpm/yarn pin from `package.json` (honoring pnpm 11+ precedence: `devEngines.packageManager` over `packageManager`) and activate it via `corepack prepare ... --activate` BEFORE the setup hook runs. corepack is bundled with node, so this is a no-op on managed infra; failure (no corepack, no network, range-only version) degrades to a warning and the existing PATH binary still runs. also replaces the legacy `npm install -g <pm>@<v>` path in prep with the same helper so behavior is consistent end-to-end. bun/deno still use the legacy installer because corepack doesn't ship shims for them. * chore(console): drop 'npm i -g pnpm' anti-pattern from setup-script placeholder the suggested example trained customers to install pnpm unpinned, which silently picks up whatever's latest at run time. that's exactly the behavior #844 traced lockfile drift back to. now that prep handles package-manager provisioning via corepack from the repo's declared pin, the placeholder is just a frozen-lockfile install — load-bearing only when the repo wants `pnpm install` to actually run (prep already does that), but a much safer default for customers who do paste it in. * refactor(action): introspect opencode models for BYOK detection Replace the static `provider.envVars + provider.managedCredentials` catalog gate in `selectFallbackModelIfNeeded` + `validateAgentApiKey` with two `opencode models` captures around the auth merge: - `captureBaselineModels` BEFORE dbSecrets + Codex auth.json - `captureAuthorizedModels` AFTER both The authorized set is the authoritative source for "can OpenCode route this model" — strictly more accurate than the catalog, which can miss new auth shapes (Codex was one, there will be more). The diff between baseline and authorized is logged as `BYOK auth enabled N model(s)` for operator visibility. Sequencing changes in main.ts: - `createTempDirectory` hoisted out of the try block so `PULLFROG_TEMP_DIR` is set before the early opencode install - `agents.opencode.install()` + baseline capture before dbSecrets - `installCodexAuth()` hoisted up (idempotent — agent re-calls it inside run() and writes the same file) - authorized capture after Codex auth.json materializes - fallback + validateAgentApiKey receive the authorized set as a parameter; tests inject directly with no mocks Deleted: `hasProviderKey`, `getModelAuthEnvVars`, `knownApiKeys` in `action/utils/apiKeys.ts` (only `selectFallbackModelIfNeeded` consumed them, and PR #844's catalog-extension fix is superseded by introspection). `getModelEnvVars` / `getModelManagedCredentials` stay exported for UI and the server-side OSS proxy heuristic in run-context/route.ts. For the claude agent path, validateAgentApiKey keeps the static single-provider check on `ANTHROPIC_API_KEY` / `CLAUDE_CODE_OAUTH_TOKEN` — `opencode models` is opencode-specific. validateBedrockSetup / validateVertexSetup also stay; they cover region/location/model-id which `opencode models` doesn't catch. When fallback engages, the post-fallback model is the guaranteed-free `opencode/big-pickle`, so validateAgentApiKey is skipped — the fallback gate already authoritatively decided "this model is OK to run". * test(oss): temp add preview-844 to ossRepos for O4 e2e * Revert "test(oss): temp add preview-844 to ossRepos for O4 e2e" This reverts commit 8167e560126b2ac516c32ba1c63c36aa32ae4019. * test(oss): temp add preview-844 to ossRepos for O5 e2e * fix(action): skip validateAgentApiKey when proxyModel is set The new opencode-models BYOK introspection in PR #844 captures the authorized set BEFORE runProxyResolution mints OPENROUTER_API_KEY, so the proxy slug (e.g. `openrouter/moonshotai/kimi-k2.6`) is never in the set. validateAgentApiKey then spuriously threw "no API key found" on every OSS run, even though the proxy key was minted correctly and the inference would have worked. Mirrors the analogous skip in `selectFallbackModelIfNeeded`: when proxyModel is set, the server-side gate (`run-context/route.ts`) is the authority and the proxy mint itself is the validation. Caught by O5 e2e on `pullfrog/preview-844-heyapi-oss-bug`. * Revert "test(oss): temp add preview-844 to ossRepos for O5 e2e" This reverts commit 3bae075ceeb188ee272c45c13b5080e15bcd00a5. * fix(action): discard hook-generated tracked-file drift before agent sees it addresses bug 3 in #844: customer setup/post-checkout hooks like `pnpm install` or `corepack prepare` left the working tree dirty (e.g. `M pnpm-lock.yaml`), the agent took the prompt's "must push" rule literally, opened a spurious bot PR for the lockfile drift, and we ate runs+spend on noise. after each setup / post-checkout hook (opt-in via `normalizeWorkingTreeAfter`), discard tracked-file mods with `git restore --staged --worktree .`. untracked files are preserved — a hook that materializes a `.env` from a template, or emits codegen output, stays visible to the agent. guarded by a pre-hook `git status --porcelain` snapshot: if the tree was already dirty before the hook ran (shouldn't happen — setup runs before any working-tree writes; checkout_pr refuses to run dirty), we warn and skip the discard rather than clobber whatever was there. prepush hook (action/mcp/git.ts) intentionally does NOT opt in — its job is to read the about-to-be-pushed state, not normalize it. * test(oss): temp add preview-844 to ossRepos for bug 3 e2e (revert before merge) * fix(action): skip eager pnpm/npm/etc install when no lockfile exists second half of bug 3 in #844. the eager prep step assumed `pnpm install --frozen-lockfile` (and equivalents) would fail cleanly without a lockfile, leaving the tree untouched. that assumption is false for pnpm 11.1.1 against a no-deps `package.json`: the command reports "Already up to date" with exit 0 AND silently materializes an empty `pnpm-lock.yaml` despite the `--frozen-lockfile` flag. the resulting untracked file trips the post-run dirty-tree gate, the agent reads it as "must push uncommitted work", and a spurious "Add pnpm lockfile" PR lands. smoking gun: pullfrog/preview-844-heyapi-oss-bug PRs #1/#2/#3, all auto-opened by the bot against a repo that contains nothing but a one-line README + a no-deps package.json. guard explicitly with an `existsSync` per manager. if the lockfile is absent, skip eager prep entirely with an info log; the agent can install on demand via the `setup` lifecycle hook (which non-frozen `pnpm install` would handle correctly), or just leave deps uninstalled when the prompt doesn't need them (e.g. the O5 "tell me a joke" path). orthogonal to the lifecycle-hook normalization in 0051bd2a — together they cover the full bug 3 surface: - eager prep can't materialize a lockfile (this commit) - setup/postCheckout hooks that rewrite tracked files have the drift discarded before the agent sees it (prior commit) * fix(action): address Pullfrog review on hook normalization two fixes in `executeLifecycleHook` from review on f6f3b32: 1. pre-hook snapshot was `git status --porcelain` which counts untracked files; in practice any repo with pre-existing untracked content (e.g. `.plans/`, an ignored-but-not-yet-gitignored scratch dir, codegen artifacts) would trip the guard and silently skip normalization, defeating the fix. switch to `git diff --name-only HEAD` so the gate measures the same thing the discard targets — tracked-file mods only. pre-existing untracked files are safe regardless because `git restore --staged --worktree .` never touches them. 2. normalization fired only on the happy path; a hook that updated a lockfile then exploded on a peer-dep conflict left tracked drift for the agent. move the call into a `finally` so it runs on success, non-zero exit, timeout, AND spawn failure. the pre-hook guard still protects pre-existing work in every case. * Revert "test(oss): temp add preview-844 to ossRepos for bug 3 e2e (revert before merge)" This reverts commit f6f3b325d6bf9a1720754ed1d39d248dab76cfa8. * fix(action): use detect lockfile strategy for eager-prep gate addresses Pullfrog review on be3c207b. two findings, one root cause: - the hardcoded LOCKFILE_BY_MANAGER map missed `bun.lockb` and `npm-shrinkwrap.json`, two managers' accepted lockfile variants. - `existsSync(join(cwd, lockfile))` only checked the immediate directory, breaking monorepo subpackages where the lockfile lives at the workspace root. both fall out by replacing the custom check with `detect({ strategies: ["lockfile"] })`. the detector already walks up the tree (subpackage → workspace root) and recognizes every accepted lockfile name across all managers it supports. restricting to the `lockfile` strategy is load- bearing: the default strategy set also matches on `packageManager` / `devEngines.packageManager` package.json fields, which would return non-null and re-mask the very case we're trying to detect (declared manager, no lockfile committed — the O5 / hey-api preview repro). drops the LOCKFILE_BY_MANAGER map entirely; no need for a second detect() call since the existing one was only used for `agent` resolution and that consumer is now after the lockfile gate, where `detected` is guaranteed non-null. |
||
|
Colin McDonnell
|
b0868d48e6 |
git tool: reject {command, args[0]} duplicates with a directed error
models occasionally call `pullfrog_git({command:"status", args:["status"]})`,
which shells out to `git status status`. git silently treats args[0] as a
pathspec — when no file/dir matches, status prints "nothing to commit,
working tree clean" even on a dirty tree. observed in production
(Skn0tt/beckerbuch run 26519563044): the agent looped trying to reconcile
that against a real diff, burned ~$3 / ~6min of opus, and only escaped when
it switched to `args: ["--porcelain"]`. generalises to every subcommand
(`diff diff`, `log log`, ...).
guard `args[0]?.toLowerCase() === command.toLowerCase()` with a directed
throw pointing the model at the disambiguated `args: ["--", "<name>"]`
escape hatch for the rare legitimate pathspec case (`--` works under every
subcommand, unlike a bare positional which can be parsed as a ref by
log/diff/checkout/restore/reset).
description also leads with the no-args case and explicitly forbids
repeating the subcommand in args. schema already had args.optional().
|
||
|
Colin McDonnell
|
dc4dff98da |
allowlist middleapi/orpc for OSS, ship skill scaffold + no-mock audit
- utils/ossRepos.ts: add middleapi/orpc (orpc.dev, 5.2k stars) to the oss allowlist so any future install mints via mintOssKey ($10 cap, pullfrog absorbs). - scripts/skill.ts + pnpm skill: scaffold .agents/skills/<name>/SKILL.md + .claude/skills/<name> symlink. patch + skill skills written using it. - AGENTS.md: hard-ban vi.* mocking apis; document pnpm skill workflow. - audit follow-through: drop pure-mock test files (action/utils/lifecycle, action/utils/timer, test/handleNoInstall) and trim action/mcp/review to the non-mock cases. - wiki/scripts.md: row for scripts/skill.ts. |
||
|
Colin McDonnell
|
a0746dcc27 | release: bump action to 0.1.13 | ||
|
Colin McDonnell
|
01e4daa0b5 |
checkout_pr: refuse unconditionally on dirty working tree (#808)
* checkout_pr: refuse unconditionally on dirty working tree drop the live-HEAD comparison from the guard introduced in #796. any checkout_pr call with staged or unstaged changes now throws, even when HEAD is already on pr-N. no stashing, no idempotent escape hatch. motivation is the zed-industries/cloud (2026-05-18) incident: shared-cwd subagents make "carry edits along" semantics dangerous, and the HEAD-equality predicate let a re-checkout silently inherit working-tree state from a sibling agent. forcing commit/discard before any PR-context operation eliminates the entire carry-forward failure class. error names the PR number, lists dirty paths, and tells the agent to commit/push/restore/clean before retrying. * improve dirty-tree error: precise discard commands copilot caught two sloppy bits in the error string: - "push" alone does not clean a dirty tree (needs commit first) - bare `git clean` is a no-op without `-fd` reword to "commit (then push if needed), or discard with `git restore --staged --worktree .` / `git clean -fd`" so the guidance is actually actionable. * checkout_pr: initial-branch invariant setupGit captures `toolState.initialBranch` at run start via live `git rev-parse --abbrev-ref HEAD`. checkout_pr refuses unless current HEAD matches the run-entry branch or the target `pr-N` (idempotent same-PR re-checkout). uses live rev-parse, not toolState.issueNumber (poisonable per the PR #796 review). refusal error names the current branch, target PR, recovery path (`git checkout <initialBranch>` with the literal branch name), and explicitly states routing around via the `git` tool is not sanctioned. closes the zed-industries/cloud (2026-05-18) shape where a subagent parked HEAD on someone else's `pr-X` and the orchestrator's next checkout_pr inherited that position. * reviewfrog: enforce canonical diff + pre-commit halt; align Build dispatch extend REVIEWER_SYSTEM_PROMPT with two prepended HARD CONSTRAINTS: - first action MUST be `git diff origin/<base>` (single-rev, captures uncommitted). no other diff first; no checkout_pr; no alt-ref fetches; no branch listing; no `gh pr list`. - empty canonical diff + claimed-changes dispatch ⇒ reply exactly with `no changes detected — likely pre-commit Build self-review; orchestrator should commit then re-dispatch` and stop. do not guess PR numbers (the zed thrash that ended in `checkout_pr({2582})`). reshape Build mode reviewfrog dispatch step around a verbatim template that names: (a) the situation is pre-commit, (b) canonical diff command, (c) halt-on-empty-diff rule. orchestrator side now says the same thing as the reviewer's baked-in prompt. delegation-discipline bullets and orchestrator-evaluation guidance kept intact. * checkout_pr: handle detached-HEAD entry in initial-branch invariant pullfrog incremental review caught a defense-in-depth gap: `git rev-parse --abbrev-ref HEAD` returns the sentinel string `"HEAD"` on detached entry, which is the default `actions/checkout` state for `pull_request` events. with the previous string-typed `initialBranch`, both the captured value and the live probe would equal `"HEAD"` on any detached state, trivially satisfying the invariant — including a subagent doing `git checkout --detach <sha>`. discriminate the captured HEAD: probe `git symbolic-ref --short HEAD` first (works on named branches), fall back to `git rev-parse HEAD` (SHA) on detached entry. store as `{ kind: "branch"; name } | { kind: "detached"; sha }`. checkout_pr runs the identical probe at call time and compares like-with-like (branch name vs branch name, SHA vs SHA). refusal error renders both heads via a small `describeHead` helper and chooses the right `git checkout` recovery target (branch name or SHA). no inline-discriminant `as` casts — uses a top-level `headsEqual` that narrows via the discriminator. |
||
|
David Blass
|
d3b5340583 |
fix: audit batch — MCP timeouts, entryPost, vip_audit 404s, and 6 more (#824)
* fix: 9 unaddressed log-audit / run-audit findings Co-authored-by: Cursor <cursoragent@cursor.com> #815 entryPost stdlib-only imports; #823 MCP timeoutMs on checkout_pr/shell; #816 FREE_FALLBACK → opencode/big-pickle; #822 chunk GraphQL nodes ≤100; #817/#821 vip_audit 404 skip paths; #813 longer serializable retries; #818 run-context handler-entered log; #805 audit severity template. * fix: update footer test for big-pickle fallback slug Co-authored-by: Cursor <cursoragent@cursor.com> * fix: anneal round 1 — ghaCore getState casing, post-hook timeout Match @actions/core STATE_ key semantics (no uppercasing), cap postApiFetch at 30s, trim serializable retries to stay under GitHub's 10s webhook window, log Clerk failures in getUserTokenByGithubLogin. Co-authored-by: Cursor <cursoragent@cursor.com> * revert: drop run-context handler log (#818 deferred) The #692 client-side fix is already on main; residual SyntaxError hits are old action pins. Per-request log added noise without fixing anything. Co-authored-by: Cursor <cursoragent@cursor.com> * document per-issue Closes syntax for audit PRs GitHub only auto-closes the first issue when numbers are comma-separated; /audits and AGENTS.md now require Closes before each issue number. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: anneal round 2 — outreach privacy, alert resilience, vertex cleanup Filter private repos from VIP authority output, harden console alert lines against DB failures, drop spoofable changesets body check, and unset GOOGLE_APPLICATION_CREDENTIALS after vertex credential cleanup. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor: drop codexHome re-export of detectCodexRefresh Import detectCodexRefresh directly from codexRefreshDetect.ts everywhere; rename the unit test file to match. codexHome.ts stays install-only. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: drop deprecated minimax-m2.5-free; add paid minimax-m2.5 Remove the deprecated free MiniMax promo from the catalog, docs, and tests. BYOK fallback and picker copy stay on opencode/big-pickle. Add opencode/minimax-m2.5 and openrouter/minimax-m2.5 for Zen BYOK and Router. Pin #816 regressions with freeFallbackCatalog and runErrorRenderer unit tests. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: hidden minimax-m2.5-free fallback for stored slugs Re-add opencode/minimax-m2.5-free as a hidden fallback alias to big-pickle so repos with the legacy slug still resolve as free. Drop live Zen API experiment tests in freeFallbackCatalog.test.ts. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
Colin McDonnell
|
6c166ac1cc |
fix: prevent cross-PR push from subagent-induced branch switch (#796)
* fix: prevent cross-PR push from subagent-induced branch switch A workflow_dispatch run for zed-industries/cloud (workflow run 26036155393) force-pushed the orchestrator's work onto an unrelated engineer's PR branch (origin/reactivate-pro-plan, PR #2582). The orchestrator's reviewfrog subagent called checkout_pr({pull_number: 2582}), which (1) moved the shared working tree to pr-2582 and (2) persisted pushDest pointing at reactivate-pro-plan. The orchestrator's subsequent commit + push_branch then clobbered the victim PR. Recovery + disclosure in PR #2584. Three compounding bugs closed here: 1. checkout_pr dirty-tree guard had a first-call hole: the previous condition required ctx.toolState.issueNumber to already be set, so on workflow_dispatch runs the first checkout_pr (commonly from a subagent) bypassed the guard entirely. Now any PR switch with a dirty tree is refused, including the first switch of a run. Idempotent same-PR re-checkouts are still absorbed by alreadyOnBranch inside checkoutPrBranch. 2. push_branch trusted sticky pushDest blindly. Added a backstop: refuse pushes where the local branch matches /^pr-(\d+)$/ AND pushDest.remoteBranch differs from it AND the current run is not scoped to PR N (event.is_pr === true && event.issue_number === N). This catches subagent-induced silent branch switches even if a future bug reintroduces a first-call hole in fix #1. 3. Build-mode self-review prompt told the orchestrator to ship "the output of git diff" to the reviewer. The model in this run synthesized git diff main...HEAD, which excludes uncommitted work — and Build self-review runs BEFORE the commit, so the reviewer saw an empty diff and thrashed, eventually calling checkout_pr on a random PR to find something to look at. Prompt now specifies git diff origin/<base-branch> (two-dot, no HEAD), which compares the working tree against the remote base. Refs: zed-industries/cloud workflow run 26036155393 zed-industries/cloud#2582 (victim) zed-industries/cloud#2584 (disclosure) * review: key dirty-tree guard on current branch + drop 'two-dot' misnomer Address review feedback on PR #796. 1. checkout_pr dirty-tree guard now keys off the live current branch (git rev-parse --abbrev-ref HEAD), not ctx.toolState.issueNumber. issueNumber is ALSO set by get_issue / get_issue_comments / get_issue_events, so a subagent doing get_issue(N) followed by checkout_pr(N) on a dirty tree would have bypassed the original guard (issueNumber === pull_number). The current branch is the actual primitive for "would this call move HEAD" — querying it directly avoids correlating on toolState that other tools write to. 2. modes.ts: drop the wrong "two-dot" label on git diff origin/<base>. That's the single-rev form, not two-dot. Copilot was right that the label was confusing/contradictory with the actually-shown command. |
||
|
Colin McDonnell
|
cb0dbcd371 |
feat(action): make prepush hook non-blocking after one failure (#777)
* feat(action): make prepush hook non-blocking after one failure push_branch now treats the repository's prepush hook as best-effort: it runs at most once per run, surfaces the failure output if the script exits non-zero, and every subsequent push_branch call this run skips the hook so the agent isn't blocked by failures unrelated to its change. The agent can iterate by running the hook command itself via the shell tool when shell access is available; push_branch will not re-run the hook automatically after a failure. Why: a one-line OSS-allowlist change took 9 minutes (#776) because the agent retried push_branch six times against a prepush hook that was failing for env-leak and missing-build-artifact reasons unrelated to the change. CI catches the same checks on the GitHub side; the local prepush gate was duplicating work and blocking unrelated fixes. - ToolState: new prepushFailureCount counter (per-run, never resets) - executeLifecycleHook: returns structured failure (kind/output/exitCode) so prepush can compose its own agent-facing message instead of inheriting the generic retry/no-retry advice meant for setup - push_branch: composes a shell-mode-aware error message; surfaces prepushSkipped on the success payload + appends a note to the message - instructions.ts + wiki/prompt.md + docs/comparisons.mdx: updated to reflect best-effort semantics * fix(action): clarify prepush latch semantics + soften static guidance review fixes from PR #777: - toolState comment, instructions, success message, tool description: replace "runs at most once per run" / "first call only" wording with the actual semantic — successful prepush keeps running on later push_branch calls; only a hook FAILURE latches the bypass. - tool description: drop hardcoded "via the shell tool" guidance so the static description doesn't mislead in shell:disabled runs (the dynamic agent prompt in instructions.ts already does shell-conditional messaging). - LifecycleHookFailure.output JSDoc: match the implementation (stderr-preferred fallback to stdout, empty for timeout/spawn). * fix(action): shorten prepush-skip log to terse operator telemetry the previous log line tried to address the agent ("re-run the hook command yourself via shell"), but log.info writes to the action runtime's stdout — the agent never sees it. agent-facing skip guidance already lives in the error message from buildPrepushFailureMessage, the success message when bypassed, and the system prompt in instructions.ts. log line is now just operator telemetry. * refactor(action): drop slop from prepush soft-fail self-audit pass after the previous review-fix round. removed duplication between code-level comment and the five other places that already explain the same behavior, tightened verbose JSDoc, and collapsed redundant clauses in agent-facing strings. - LifecycleHookFailure → discriminated union. drops the optional exitCode/spawnError fields (and the empty-output sentinel for timeout/spawn) plus the corresponding ?? fallbacks in the helper. - PushBranchTool: 7-line code comment above the latch removed (toolState field comment + tool description + error message + success message + system prompt all already cover it). tool description third sentence dropped (restated the second). success message tightened to a parenthetical. - buildPrepushFailureMessage: 4-line JSDoc → 1 line. shared "if you think the failure could indicate a real bug in your code" prefix factored out across the shell-conditional branches. - ToolState.prepushFailureCount comment: 8 lines → 3. the "what" is in git.ts; comment now only documents the invariant (never decremented within a run). - instructions.ts prepush guidance: collapsed nested bullets + ternary into one paragraph; dropped the "so re-running via shell is the only way…" tail that restated "push_branch will NOT re-run it". * fix(action): hint prepush bypass on dirty tree after hook failure When push_branch blocks on a dirty working tree and the prepush latch is already set, tell the agent the hook will be skipped once the tree is clean. * fix(test): narrow CI matrix for lifecycle and toolState changes Remove lifecycle.ts from ALWAYS_RUN_ALL and add lifecycle.ts + toolState.ts to push/git agnostic test coverage so PRs touching prepush latch logic run targeted tests instead of the full matrix. |
||
|
Colin McDonnell
|
7e90e5cae6 |
Align Plan-mode prompts on report_progress as the canonical plan tool (#786)
* fix: align Plan-mode prompts on report_progress as the canonical plan tool Fixes #673. Three sites disagreed on where Plan output should be posted, letting a model synthesize a broken third interpretation (initial post via `report_progress({ target_plan_comment: true })`, which then misses the `existingPlanCommentId` precondition). This PR aligns all three on `report_progress` as canonical, with `target_plan_comment` reserved for revisions only: - `action/modes.ts` Plan step 4 — spell out that the initial plan post uses `report_progress` WITHOUT `target_plan_comment`, and that revisions go through `select_mode`'s PlanEdit override. - `action/mcp/comment.ts` `target_plan_comment` flag description — make the "revisions only" precondition explicit and call out the initial-post path by name. - `action/utils/instructions.ts` Progress reporting paragraph — drop the misleading "(e.g., Plan comments)" parenthetical that read as "use create_issue_comment for plans". `PlanEdit` (in `action/mcp/selectMode.ts`) was already correct and is unchanged. Intentionally out of scope (to keep the fix minimal): a `publish_plan` tool, removing the vestigial `create_issue_comment({ type: "Plan" })` branch, hardening the run-end cleanup guard for the `target_plan_comment but no existingPlanCommentId` fallthrough, and renaming `target_plan_comment`. * align create_issue_comment description with report_progress as canonical plan tool |
||
|
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.
|
||
|
Colin McDonnell
|
8dff91ac49 |
router: fix unspendable signup credit on no-card private repos (#792)
* router: fix unspendable signup credit on no-card private repos (#791) The bug ------- `run-context/route.ts` gated `proxyModel` minting on `isInfraCovered`, which is `oss || hasCard`. So a no-card account with positive wallet balance (signup credit, top-up, etc.) on a private repo would never get a `proxyModel` set on the run context. The action runtime then fell through to whatever provider keys happened to be in the workflow env — using the user's BYOK keys without their knowledge if any were configured, or failing the run entirely otherwise. Meanwhile `proxy-token/route.ts` already gated correctly on `oss || hasCard || balance > 0`. The two routes disagreed, with run-context being strictly more restrictive, so the agent never even attempted to call proxy-token for these accounts. The wiki at `billing.md:1052` documented the *intended* behavior ("a Router usage row can debit a wallet with no card on file"), aspirational against the actual code. The action side had a parallel bug at `action/utils/proxy.ts:151` — it re-derived `isInfraCovered({ isOss, plan })` and short-circuited mint even when the server set `proxyModel`. Belt-and-suspenders that was strictly more restrictive than the server. Production impact ----------------- Queried 55 router-mode no-card accounts holding signup credit: - ALL have wallet balance = exactly $10.00 (untouched) - ALL have 0 router proxy keys ever minted, 0 hwm usage - ~25 have successful runs (using BYOK env vars from their workflow, unaware their credit isn't being touched) - The rest have zero successes; some accumulated 25+ failures (e.g. `onechannelpe`: 25 failures, 0 successes, no card, $10 credit). The fix ------- - `run-context/route.ts`: widen `useRouter` to match proxy-token's gate. OSS short-circuits as before. Otherwise: router mode + card on file → mint; router mode + no card + positive balance → fetch balance, mint if > 0. Skip the balance read when a card is on file (auto-reload covers it without needing pre-flight balance — keeps the hot path single-query). - `action/utils/proxy.ts`: drop the redundant `isInfraCovered` check. `ctx.proxyModel` IS the signal — the server is the authority on funding decisions; the action just trusts and mints. - `wiki/pricing.md`: correct the Router proxy key minting gate row + add a paragraph explaining why this gate diverges from `isInfraCovered`. - `wiki/billing.md`: rewrite the misleading "proxy-token returns 402" paragraph to describe what actually happens at both routes. `isInfraCovered` is unchanged. It still gates Pullfrog-paid features (learnings writes, indexing). The bug was in conflating "Pullfrog pays for marginal infra" with "user can fund a Router run via wallet" — different concerns, now untangled. * action: drop dead isInfraCovered + plan param post-fix Cleanup the action-side dead code introduced by the previous commit's removal of the redundant `isInfraCovered` re-derivation in proxy.ts: - delete `isInfraCovered` from action/utils/runContext.ts (was the only callsite; mirror in server's utils/billing.ts is unchanged and still load-bearing for learnings/indexing) - drop unused `plan: AccountPlan` param from `resolveProxyModel` / `runProxyResolution` (and the corresponding `AccountPlan` import + the `plan: runContext.plan` arg at the main.ts call site) - update the action/mcp/server.ts comment that pointed at the now-gone action mirror to reference the server-side `utils/billing.ts` instead `AccountPlan` itself is still load-bearing (mcp/server, runContextData, run-context fetch), only `isInfraCovered` and the dead `plan` parameter go away. |
||
|
David Blass
|
dd26d35137 |
learnings: audit fixes — preamble in TOC, server-side line-boundary truncation, empty-repo intro (#743)
* learnings: surface preamble in TOC, mirror line-boundary truncation server-side, fix empty-repo intro copy three audit fixes on top of the recent learnings overhaul (#717): - `parseLearningsHeadings` now prepends a synthetic `(preamble)` entry when a body has non-whitespace content before the first heading. the prompt instructs the agent NOT to slurp the whole file when a TOC is present, so without this any preamble lines were silently invisible (realistic transitional case: an agent partially restructures a legacy free-text body and leaves bullets above the first `## `). - server-side PATCH route now applies the same line-boundary-aware truncation as the action (defense in depth via a shared `truncateAtLineBoundary` + `MAX_LEARNINGS_LENGTH` exported from `action/internal`). the raw `.slice` it used before could leave a mid-heading tail on any caller that bypassed the client-side truncate, breaking the next-seed TOC parse. removes the duplicated cap constant. - `buildLearningsSection` intro no longer asserts "accumulated by previous agent runs" — false for fresh repos with zero history. new copy is tense-neutral and works for empty + populated bodies. also nudges the agent to re-read after mid-run edits (the inlined TOC ranges are a run-start snapshot). Co-authored-by: Cursor <cursoragent@cursor.com> * learnings prompt: tighten to single evergreen test, allow tool-quirk bullets when they prevent repeat waste The blanket "no pullfrog tool quirks" ban was wrong — if the agent burned calls discovering a quirk this run, recording the workaround prevents the next run from repeating the waste. Reframe around one litmus ("would a future run do its work better because this bullet exists?") and trust it to subsume the scattered don'ts. Drop the 3+ months timeframe (arbitrary) and the four-example pullfrog/PR/date/play-by-play list (the rule underneath is "don't anchor facts to repo state that will move"). Cuts ~10 lines from a prompt the model was already mostly ignoring; the remaining anchor list is narrower and more enforceable. * audit-learnings-r2: align wiki + tighten re-read nudge - wiki/prompt.md described the post-run reflection prompt as "bans pullfrog-tool quirks (those belong in tool descriptions, not per-repo learnings), bans PR/review/commit/date references" — that's stale after the prompt rewrite. update to: single-litmus framing, expanded anchor list (now includes version pins + line numbers), and explicit allowance for tool-quirk workarounds when discovery burned calls. - buildLearningsSection re-read nudge said "re-read after editing" which can be read as "re-read the section you edited". in fact any edit shifts the line numbers of every later section in the TOC, not just the edited one. tighten to make that explicit. mirror the new wording in the wiki example block. update the test substring assertion accordingly. * postRun: refresh JSDoc to match the reflection prompt rewrite `buildLearningsReflectionPrompt`'s JSDoc still listed "PR-/review-/commit-/date-anchored facts" and "rediscovery of pullfrog-tool quirks" as failure modes the prompt pushes back on. after b586b4f8 the prompt no longer bans tool-quirk bullets (it explicitly allows them when the agent burned calls discovering the quirk), and the anchor list expanded to cover branch refs, version pins, and line numbers too. update the JSDoc so it describes the prompt that actually exists, and call out the cross-repo drift tradeoff that comes with allowing tool-quirk bullets. * fix(mcp/issueEvents): narrow event.event before Set.has lookup octokit's listEventsForTimeline union includes timeline-event members where `event` is `event?: string`. `("event" in event)` does not narrow that property to non-undefined, so `relevantEventTypes.has(event.event)` was passing `string | undefined` to a `Set<string>.has`. typescript only flagged this once `cf-worker-indexing` started seeing the file via the type graph that now reaches mcp through the new `truncateAtLineBoundary` re-export in `action/internal/index.ts`. fix the latent bug at the source: require `typeof event.event === "string"` before the Set lookup. * learnings: split truncation helpers into MCP-free module re-exporting `truncateAtLineBoundary` + `MAX_LEARNINGS_LENGTH` from `action/utils/learnings.ts` through `action/internal/index.ts` accidentally pulled the entire MCP type graph into the SDK barrel: `learnings.ts` imports `ToolContext` from `mcp/server.ts`, which transitively wires every tool module under `action/mcp/` into anything that imports from `pullfrog/internal`. for `cf-worker-indexing/tsconfig.json` (`customConditions: ["@pullfrog/source"]`) and the root `tsc` (which compiles the proprietary app routes that import from `pullfrog/internal`), this expanded the type-checked surface and surfaced two latent issues in unrelated files (`mcp/issueEvents.ts`, `utils/subprocess.ts`). a 6-line pure string helper has no business dragging mcp/server.ts into anyone else's type graph. move both symbols to `action/utils/learningsTruncate.ts`. `learnings.ts` re-exports them so existing callers keep working; `internal/index.ts` re-exports from the truncate-only module so the SDK barrel stays MCP-free. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Colin McDonnell <colinmcd94@gmail.com> |
||
|
Colin McDonnell
|
8ac954a27f | fix(handleIncompleteSetup): also skip nudge when repos are disabled, not just active | ||
|
Colin McDonnell
|
88f170e19a |
fix: 7 log-audit / run-audit findings (mega-PR) (#769)
* fix(#765): silence Clerk 400 (revoked OAuth) noise from getTokenForClerkId Branch on isClerkAPIResponseError + status<500 so the well-understood revoked-token redirect doesn't emit a level=error line in Better Stack on every request. Vercel maps console.warn -> error for non-streaming routes, so a downgrade to log.warn wouldn't help; only the unexpected shape (5xx, network) is worth surfacing. * fix(#742): stop logging input verbatim from yes.op retry-failure paths GitHub OAuth user tokens (ghu_...) were leaking to Better Stack on every yes.op retry-failure for any utils/github/get* helper that takes a token field — 38 leaks/7d in the most recent audit window. The leak path is console.log inside the yes package (its own log shim, not utils/log.ts). Drop input from the four log sites + the cache-key-derivation throw site. key (SHA-1 of input) is sufficient for retry correlation; error already carries request URL + status. Defense-in-depth comment so future contributors don't re-add the field. Operational follow-up (separate task): inventory ghu_... strings in Better Stack ingested in the last 90d, revoke matching Clerk grants, scrub cold-tier S3, rotate the BS source token. * fix(#759): handle GraphqlResponseError "Could not resolve to a node" as 404 When the stored planCommentNodeId references a comment that's been deleted on GitHub, octokit.graphql throws GraphqlResponseError before the existing `node === null` 404 branch is reached. Add a narrow isGraphqlNodeNotFound predicate in utils/errors.ts and a new catch branch in the plan-comment route. The action treats 404 as "no prior plan comment" and creates a fresh one, so behavior matches existing contract. * fix(#747): convert webhook GraphQL rate-limit 5xx into a Result<T> sentinel + 200 ack When GitHub's GraphQL responds with "API rate limit exceeded for installation ID N", _getReviewCommentsWithReplies threw, propagated through the bare yes.op wrapper (no rate-limit bail), out of the bare await in handleWebhook, and crashed /api/webhook/github with 500 — 77 webhook 500s/24h on the most recent audit window. GitHub redelivery plus R2 dedup also silently masked the legitimate handler from re-running once the rate-limit window cleared. Mirror the #658 / _getRepository pattern: detect GraphqlResponseError matching /rate limit (already )?exceeded/i, log.warn with the x-ratelimit-reset value (and [Installation N] prefix when available), return failure(...) with status 429. Webhook handler short-circuits the case with 200 + log.info so GitHub stops the redelivery storm against an exhausted budget, and the trigger page surfaces a clean ThrowClientError. Document the new pattern as a Tier 2 false-positive in wiki/log-audit.md so the next audit cron doesn't re-flag it. Note that returning [] silently (the issue's first suggestion) would have dropped @pullfrog mentions inline in review comments and dispatched an agent run that re-rate-limits — skip-the-whole-case is the correct semantics. Co-vulnerable getPullRequest / getWorkflow have zero occurrences in this window; per #737 policy, defer until they show up. NOTE: this commit and the bracket of touched files revert as a unit — the Result<T> shape change in getReviewCommentsWithReplies is breaking; partial revert breaks the type chain. * fix(#766): fold stderr+stdout into shell.ts errors + carve out merge-base --is-ancestor action/utils/shell.ts dropped stdout when constructing failure messages ($\{stderr || "Unknown error"\}), so git subcommands that write context-bearing diagnostics to stdout (merge conflicts, cherry-pick rejections, diff --exit-code, ls-files --error-unmatch) surfaced as "Command failed with exit code 1: Unknown error" through mcp__pullfrog__git. The agent burned an extra MCP round-trip calling git status to recover. Fold stderr + stdout into the thrown error message (stderr first, stdout fallback) so the agent always sees the real diagnostic. Plus a narrow carve-out for `git merge-base --is-ancestor` in action/mcp/git.ts: that subcommand uses exit code as data (0=ancestor, 1=not-an-ancestor, >1=error), so return { success: true, isAncestor } instead of throwing on exit 1. No caller in action/ string-matches on the old error format (verified). diff --exit-code and ls-files --error-unmatch are not carved out — both are zero-occurrence in the May audit window, and the stderr+stdout fold renders their output usefully anyway. * fix(#739): point customers at the actual fix when permissions: id-token: write is missing When a customer workflow runs in GitHub Actions but lacks permissions: id-token: write, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN aren't injected, isOIDCAvailable() is false, and acquireNewToken falls through to the local-dev-only acquireTokenViaGitHubApp path, which throws "GITHUB_APP_ID and GITHUB_PRIVATE_KEY must be set" — pointing at a self-hosted-app fix that doesn't apply. One affected customer burned 13 dispatches in 24h on this misleading error. Detect (GITHUB_ACTIONS=true) AND (no OIDC env vars) inside acquireNewToken before falling through to the local-dev branch, and throw an actionable message naming the missing permissions block, the exact YAML, and the docs anchor. The error surfaces via ##[error]action failed: ... in the workflow log (the only customer surface available before main()'s inner try opens). Local-dev path keeps the existing GITHUB_APP_ID message. * fix(#760): suspend activity watchdog across in-flight tool calls mcp__pullfrog__checkout_pr was hard-failing 6/24h on SenecaLabs/senecaWeb because git fetch+deepen on a large monorepo can take 4-5 min, the agent's stdout pipe goes silent the entire time (FastMCP is in-process HTTP, but Claude/opencode CLIs await the synchronous tools/call response), and both the spawn-level activity timer (300s in subprocess.ts) and the process-level activity monitor (300s in activity.ts) fire and kill the run. Re-introduce the bracket pattern that PR #634 removed: bracket suspendActivity()/resumeActivity() around tool_use -> tool_result in both agent harnesses, plumb isPausedExternally into spawn() so both timers suspend in lockstep. Bounded by MAX_TOOL_CALL_SUSPENSION_MS (15 min auto-resume) plus the outer 1h agent timeout — neither zombie-run avenue from #12 is reopened (subprocess.close still resolves on death; outer timeout is suspend-agnostic; suspends gated on explicit paired CLI events, not internal noise). opencode tool_use handler: gate suspendActivity() on non-terminal status (running/pending) so the bus_event re-dispatch path at line 915 — which only fires for completed/error subagent parts and never emits a paired tool_result — doesn't latch the watchdog into suspension until the 15min ceiling. Add a heuristic:activity-watchdog-ceiling classifier to scripts/analyze-logs.ts so a tool that genuinely hangs past MAX_TOOL_CALL_SUSPENSION_MS surfaces in run-audit instead of being bucketed into failure:unknown. NOTE: this commit and the bracket of touched files revert as a unit — activity.ts, subprocess.ts, and the two harnesses must move together or the bracketing breaks. * refactor(#747): swap Result<T> for InstallationRateLimitError typed throw The Result<T> shape from 3ebf6c4c was cargo-culted from the #658 _getRepository pattern, but _getReviewCommentsWithReplies has only one expected-error case (installation rate-limit) and two callers — Result imposes branching on the trigger-page caller that never cared about the rate-limit case specifically. A typed error class is lighter (~10 LoC vs ~33) and matches the actual need: - new InstallationRateLimitError(resetAt) thrown from _getReviewCommentsWithReplies; rate-limit log.warn unchanged. - handleWebhook catches it and breaks with log.info (unchanged semantics: 200 ack, no redelivery storm). - trigger page reverts to direct array access; any failure propagates to the page error boundary (the pre-#747-commit shape). - log-audit.md wording updated to match. |
||
|
Colin McDonnell
|
c0988e35b0 |
fix(security): block docker socket from sandboxed shell; disable opencode batch_tool
two real CI failures on main, both shipping bugs in the action: 1. `token-exfil-claude` was a real sandbox escape: GHA `ubuntu-latest` puts `runner` in the `docker` group, so a sandboxed shell could run `docker run --pid=host --privileged busybox cat /proc/<parent>/environ` and read the action process's env (which holds user secrets) — fully bypassing the unshare PID-namespace. fix: inside the sandbox's mount namespace (already private via `--mount-proc` which implies `--mount`), bind-mount /dev/null over /var/run/docker.sock (+ podman/containerd/crio variants) so any container-runtime socket connect from the sandbox fails. only affects sandboxed shells — host runner mount table is untouched, so user workflow steps outside pullfrog keep working. 2. `restricted-opencode` regressed in #719 (`experimental.batch_tool`). opencode's batch tool rejects MCP tools with `"Tool '<name>' not in registry. External tools (MCP, environment) cannot be batched."` when a model emits parallel `pullfrog_shell` (or any MCP) tool_use blocks, opencode internally routes them through batch, they all fail, the model misreads the error as "the tool doesn't exist", and gives up. caught by a `lens:` subagent in the restricted test concluding shell was unavailable and setting `DIAGNOSTIC_ID=empty`. drop `batch_tool: true` and the matching opencode-specific guidance in `instructions.ts` — native parallel tool_use (multiple tool_use blocks per assistant message) still works for both built-in and MCP tools without batch, so we lose only the 1-25 wrapper, not parallelism. |
||
|
Colin McDonnell
|
ddbc610569 |
review prompt: friendly green callouts + per-section severity emojis (#756)
* review prompt: friendly green callouts + per-section severity emojis - Replace `[!NOTE]` informational tier and the no-callout minor-suggestions tier with friendly green blockquotes (`> ✅` / `> 💡`). The two loud tiers (`[!CAUTION]` / `[!IMPORTANT]`) keep their GitHub admonitions. - Add a per-`##`-section severity-emoji rule (🚨/⚠️/💡/ℹ️) for cross-cutting review concerns that don't anchor to a line and would otherwise be buried in summary content. - Drop the `<br/>` between summary sections — heading + blank line carries enough visual spacing. - Skip the post-run learnings-reflection turn for `IncrementalReview`. It's the lowest-novelty mode (delta review against existing PR with prior summary already loaded) and almost never produces durable learnings — reflection there costs ~$0.50-0.80/run for nothing. - Surface real error info on `agent-browser` skill install failures (exit code + stdout + stderr + spawn error). The skills CLI uses a TUI that prints errors to stdout, so the prior stderr-only logging silently swallowed every failure. * review prompt: per-bullet severity emoji + bullets-only sections Section headings are plain again (no leading severity emoji). Severity moves to individual bullets so a section that mixes a 🚨 and a 💡 isn't mislabeled by either. Section bodies are now bullets only — paragraph prose under a heading is harder to scan and tends to bury the actionable point. Bullets can carry indented continuation content (sub-bullets, code fences, blockquotes) by indenting two spaces under the parent. * review prompt: cap section length + identifier discipline Bound each summary section to at most 4 bullets at most 2 lines each, and explicitly call out identifier-heavy prose as an anti-pattern. The reader is often a manager or non-author; identifier-dense paragraphs ('foo calls bar.fetch which dispatches to baz via qux...') are unreadable for them. Default to plain-language behavior descriptions, name an identifier only when it's the subject of an actionable concern or a public surface a reader would recognize, target 2-3 backtick tokens per bullet. Move the deep-explanation pattern from open blockquote to a default- collapsed details/summary so depth doesn't dominate the visible body. * review prompt: hard cap on bullet identifier density + worked rewrite example Soft 'aim for 2-3 tokens' guidance was ignored — first big-PR e2e showed 12 of 19 actionable bullets exceeded the target (avg 4.8 tokens, several over 8). Promote to a hard cap of 3 backticked tokens per bullet and pair with a concrete bad/good rewrite the agent can pattern- match against. Also tighten the per-bullet length cap from ~240 to ~200 chars and explicitly call it 'hard cap, not target'. * review prompt: tighten bullet length cap to 160 chars, dramatize the worked example V2 e2e test: token discipline improved (4.8 -> 3.3 avg, 12/19 -> 6/14 violations) but length got worse (235 -> 286 chars, 13/14 over the 200 cap). The agent compensated for fewer identifiers with more prose. Two changes: (1) tighten the cap from ~200 chars to 160 chars / 1 visual line and call out wrap-to-multiple-lines as the failure mode; (2) rewrite the worked example so the good version is genuinely half the length of the bad one, not just lower token count. The example was the thing the agent pattern-matches against; making the good version ~130 chars vs the bad version's ~290 chars sets the right shape. * review prompt: drop fixed bullet-count cap, keep length + identifier caps Per user feedback — section length should be governed by content, not an arbitrary count. Soft guidance ('past ~6, ask whether to split') is fine; the hard '≤ 4 bullets per section' rule was the wrong shape. Length cap (160c) and identifier cap (3 backtick tokens) stay; those target the actual scanability problem. * review prompt: drop ## subsystem sections, flat 'Issues found' list Per-section structure forced every concern into a subsystem frame and made the body read like a series of mini-essays. Replace with two parts: (1) TL;DR + Key changes as the dispassionate overview, (2) flat '### Issues found' list ordered by severity, intermixed across files and subsystems. Per-bullet rules (≤160c, ≤3 backtick tokens, severity emoji prefix, optional indented continuation) carry over unchanged. * review prompt: full v6 structure — preamble + cross-cutting H3s + nitpicks Replaces the flat 'Issues found' bullet list with the iterated v6 shape: - Preamble is a bolded inline 'Reviewed changes' lead-in plus bullets plus a collapsed 'Review metadata' block (mode/files/commits/refs/ reviewed commits list/prior pullfrog review/staleness note). - Each cross-cutting concern gets a '### emoji Title' section. The visible problem write-up is human-friendly and DESCRIBES THE PROBLEM ONLY — no asks, no suggested fixes, no 'the right thing to do is'. - Each section carries a collapsed 'Technical details' block wrapped in a 4-backtick markdown fence (so it can hold its own 3-tick code fences cleanly, agent-readable, one-click copyable). Standard four inner sections: Affected sites, Required outcome, optional Suggested approach, optional Open questions for the human. - '### ℹ️ Nitpicks' at the bottom for body-only nits that don't inline; simple bullets, no technical-details collapse. - Anti-paragraph-wall rule: never two successive plain paragraphs in visible '### ' sections; alternate prose with structure. - Inline-vs-body discipline: anything that anchors to a single line goes inline, body is for cross-cutting only. - Drops legacy '### Key changes', '### Issues found', '<b>TL;DR</b>', and the '<sub>Summary</sub>' line. * model effort: bump Gemini + GPT to high effort; drop Gemini Pro→Flash subagent E2E review eval against a substantive billing-module diff surfaced two related quality gaps: 1. Gemini Pro at thinkingLevel=medium (#663's CI-timeout fix) reviewed the diff only, took the 0-lens path, and missed a catastrophic camelCase/snake_case service-vs-schema mismatch. Bumping back to high — review work is exactly the wrong shape for the medium/high tradeoff #663 was optimizing for; the per-turn TTFT cost is worth paying when reasoning IS the value. 2. GPT had no reasoningEffort override, defaulting to upstream medium. Same diff, similar shallow result vs Claude. Adding reasoningEffort: high for the curated direct-OpenAI slugs, mirroring the Gemini pattern (Anthropic separately uses --effort high via the Claude Code CLI flag in claude.ts). 3. Gemini Pro's subagentModel was 'gemini-flash' — but Google has no in-between tier between Pro and Flash, and Flash is a meaningful capability cliff for review work. Dropping the override so subagents inherit Pro. Cost stays reasonable since Gemini Pro is already the cheapest of the flagship trio. Other providers unchanged: Anthropic opus→sonnet and OpenAI gpt→gpt-5.4 remain (each is a one-tier drop to a still-capable sibling). * model effort: revert orchestrator override, set explicit high on reviewfrog subagent Reshape the effort design after eval: - Drop the explicit Gemini and GPT model-level overrides — orchestrators now run at upstream defaults (Gemini high, GPT-5.x medium). Gemini's upstream IS high, so this is a no-op there; GPT goes back to upstream medium for orchestrator-level routing work. - Add explicit 'high' on the reviewfrog subagent via agent.options. OpenCode merge order is base ← model.options ← agent.options ← variant per session/llm.ts:141, so the subagent always runs at high regardless of which orchestrator dispatched it. Both thinkingConfig.thinkingLevel (Gemini) and reasoningEffort (GPT) keys included; irrelevant keys are ignored per provider. - Bump providers-live timeouts (12min job / 10min step, from 8/6) to budget for Gemini's TTFT variance at high effort. #663's 4min timeout was sized for the medium-effort override that's now removed. * model effort: restore Gemini explicit high override (no-override path breaks) Bare 'rely on upstream default' for Gemini failed in e2e — removing the model-level provider config produced 'Function call is missing a thought_signature' API errors on every gemini-pro run. Even though upstream opencode's options() returns the same thinkingLevel: high we were explicitly setting, opencode's resolution path differs subtly between the two cases. v2's explicit override worked; v3's removal broke. Reproducible across two consecutive runs. Restoring the explicit Gemini override (back to v2 design). GPT orchestrator stays UN-overridden — at upstream default (medium) — since removing that override didn't trigger the same failure pattern and the reviewfrog subagent agent.options high override compensates for the extra depth GPT loses at medium. * diag: remove reviewfrog agent.options to isolate Gemini thought_signature failure v3 (no Gemini orch override) failed with thought_signature error. v4 (restored Gemini orch override at v2-equivalent) ALSO failed, even though the orchestrator config matches v2. The variable between v2 (working) and v4 (failing) is the new reviewfrog agent.options block. Removing it to confirm — if Gemini works again, the agent.options addition is the culprit and we need a different shape for it. * opencode-ai: bump 1.1.56 → 1.15.0 + clean up gemini effort config opencode-ai@1.1.56 was published 2026-02-10 (3 months old). The Google API tightened thought_signature validation 24-48h ago (per https://discuss.ai.google.dev/t/gemini-thought-signature-patch/122555), and the bug class hits opencode's session→prompt serializer for MCP tool-call parts (anomalyco/opencode#4832, #8321). Latest stable bumps us through ~3 months of fixes; needed for Gemini-direct to stop dying with 'thought_signature is missing' on every multi-turn run. Companion cleanup: the gemini provider override in opencode.ts had 30-line block of comments, four unused constants, and a 6-line Object.fromEntries map for two entries. Replaced with one source-of- truth helper that loops modelAliases, filters provider==='google', strips the 'google/' prefix, and returns the override map. Adding any future Google alias to the registry now flows through automatically. Test added: action/agents/opencode.test.ts asserts the helper covers every direct-Google alias, strips the prefix correctly, and pins every entry to thinkingLevel high — catches drift in helper logic without hardcoding the API ids the test would have to update in lockstep with the registry. * fix(workflow): tolerate listJobsForWorkflowRun 404 in resolveRun PR #750 (docker testing rewrite) replaced the per-call env allowlist with full process.env passthrough into the test container. That now leaks GITHUB_RUN_ID + GITHUB_JOB into runs whose MCP token is scoped to a DIFFERENT repo (e.g. providers-live smoke runs the action against pullfrog/test-repo with pullfrog/app's run ID). The unconditional listJobsForWorkflowRun call 404s and crashes the entire run, breaking every providers-live job on main since #750 landed. jobId is purely cosmetic (deep-links 'View workflow run' footer to a specific job vs the run-level URL). Wrapping the API call in try/catch so a 404 logs a debug message and falls through to undefined jobId is the right fix — the failure mode is exactly what graceful degradation is for, and the alternative (filter the env vars at the docker boundary) re-introduces the kind of allowlist #750 was getting rid of. * opencode-ai: pin 1.14.51 instead of 1.15.0 (effect refactor breaks JSON output) opencode 1.15.0 (May 15) ships a major architectural refactor onto @effect — the run command boots an in-process server via @opencode-ai/sdk/v2 and the JSON event emission path through that SDK client doesn't surface on stdout the way our parser expects (CI run on 1.15.0 produced 0 stdout events but the agent still completed). Local invocation also hangs at the in-process server boot. The Gemini thought_signature fixes (the original reason for bumping) landed earlier in the 1.14.x line, so 1.14.51 (May 14) gets us the upstream fix without the Effect rewrite. Defer the 1.15.x bump until we're ready to rewire our parser/spawn around the new SDK. * opencode-ai: revert to 1.1.56; gha: filter outer-CI workflow-run vars at the docker boundary Two related changes for the docker testing harness's ergonomics: 1. Revert opencode-ai 1.14.51 → 1.1.56. The 1.14+ line ships an Effect refactor (the SDK-v2 client + in-process server architecture) that our --format json parser doesn't speak — even the 1.14.51 release, pre-dating the 1.15.0 Effect rename, produced 0 stdout events on our skill-invoke smoke. There's no clean pre-Effect version that ships the Gemini thought_signature fix; that fix needs a separate workstream once we're ready to rewire the parser onto SDK v2. 2. Filter outer-CI workflow-run identifiers (GITHUB_RUN_ID, GITHUB_JOB, GITHUB_WORKFLOW, GITHUB_ACTION, GITHUB_REF, GITHUB_SHA, etc.) from gha.ts's --env-file passthrough. PR #750's full-process.env design leaks pullfrog/app's CI run identifiers into runs that act against a different repo (e.g. pullfrog/test-repo); any code path inside the action that uses them as keys (most notably resolveRun's listJobsForWorkflowRun lookup) 404s. Filtering them here means the action sees undefined and skips the lookup, complementing the defensive try/catch in resolveRun (commit addc76d4). GITHUB_REPOSITORY and GITHUB_TOKEN are NOT filtered — those are genuinely needed. Companion to addc76d4 (resolveRun 404 tolerance). The two together make this class of bug 'either fix would have caught it' rather than 'silently breaks the entire test matrix'. * fix(deps): sync pnpm-lock.yaml with opencode-ai 1.1.56 manifest revert Forgot to refresh the lockfile after reverting the manifest in 02c6d8c1. CI's frozen-lockfile install was failing with 'lockfile: 1.14.51, manifest: 1.1.56' mismatch. |
||
|
Colin McDonnell
|
74b7329f64 |
fix(action): dedupe concurrent checkout_pr + guard cross-PR clobber (#735)
* fix(action): dedupe concurrent checkout_pr calls + guard cross-PR clobber (#642) agents occasionally emit duplicate parallel `checkout_pr` tool_use blocks in one turn, causing two `checkoutPrBranch` invocations to race the same `.git/shallow.lock` and one to fail with `File exists`. the prior fix (#564) added a 30s staleness sweep, but that very threshold protects the within-run concurrent case from itself. dedupe at the tool layer: a module-level `Map<pull_number, Promise>` shares a single in-flight promise across concurrent same-PR calls. the fetch race becomes architecturally impossible — first call does the work, duplicate gets the same `CheckoutPrResult`. cleared in `finally` so subsequent same-PR calls re-do the work normally. also reject cross-PR checkouts when the working tree is dirty, surfacing a clear error instead of silently overwriting uncommitted work from a prior PR. uses existing `toolState.issueNumber` (no new state). * review: use dedicated `pullNumber` toolState field for cross-PR guard per copilot review: the prior guard used `toolState.issueNumber`, which is also set by issue/comment lookup tools (issueInfo, issueComments, issueEvents, review). that conflation is intentional and correct for its only consumer (`report_progress` falls back to `issueNumber` to choose which issue/PR to comment on, and GitHub treats both via the same comment API). but it makes the field wrong for the cross-PR guard: a same-PR re-checkout after `get_issue(other)` would falsely fire and surface a misleading "from PR #other" message. introduce a separate `pullNumber` field, set only by `checkoutPrBranch` alongside `issueNumber` and `checkoutSha`. narrower invariant, no disturbance to the existing `issueNumber` semantics. * review: drop dual-write — single `issueNumber` is sufficient for the guard reverting the `pullNumber` addition. setting both `issueNumber` and `pullNumber` to the same value at the same site was a code smell — there is no scenario where they diverge. issues and PRs share GitHub's number space, and the cross-PR guard's actual job is "refuse to clobber a dirty tree when switching to a different number"; that's expressible with `issueNumber` alone. addresses copilot's original concern (misleading "from PR #X" message when X was an issue) by removing the prior-number reference from the error message entirely. the dirty paths are the actionable detail. |
||
|
Colin McDonnell
|
4ad649ebb9 |
action: extend shallow-unreachable deepen-retry to checkout_pr fetches (#734)
extracts the deepen-retry helper from `GitFetchTool` into shared `$gitFetchWithDeepen` and applies it to every fetch in `checkoutPrBranch` (baseRef, pull/N/head, before_sha temp branch). on shallow clones with deep PR ancestry — the failure mode behind ~10 of 51 `heuristic:very-slow` runs in 24h on `remotion-dev/remotion` — the baseRef fetch was throwing `Could not read <sha>` to the agent before the compare-api deepen block could run. agents then burned 10+ minutes retrying `checkout_pr` and falling back to ad-hoc shell `git fetch --deepen` workarounds. also splits the analyzer's `heuristic:git-error-recovered` into `heuristic:git-shallow-unreachable` and `heuristic:git-shallow-lock` buckets so future audits surface this without manual log-grep. closes #656. |
||
|
Colin McDonnell
|
2960d51493 |
shell tool: cap output at 5K chars and spill overflow to tempfile (#732)
unbounded shell tool output blows the agent's context window on commands
that dump big logs (test runners, build tools, grep on large trees). cap
the inline body at 5000 chars; on overflow, persist the full output to
${PULLFROG_TEMP_DIR}/shell-<id>.log and return the tail prefixed with a
sentinel pointing at the saved path. agents re-read the tempfile with
cat/tail/grep when they need more.
|
||
|
Colin McDonnell
|
206c11fe7c |
review: drop misleading 'with the same arguments' from diff-coverage nudge
agent is free to refine review body/comments on retry — there's no enforcement that the second call matches the first, and if reading the nudged region surfaces a new finding the agent should add it. |
||
|
Colin McDonnell
|
7414c1e9ca |
review: clarify diff-coverage nudge gives explicit license to skip generated artifacts
the one-time pre-flight nudge said "optionally read" but never told the agent it's free to retry without reading when every unread region is generated (lockfiles, codegen, snapshots, migration metadata). audit #677 surfaced ~21 runs/24h burning an extra model turn re-reading drizzle snapshots, pnpm-lock, and *.gen.ts files purely to satisfy the gate. mode prompts only mention generated content in the "skip self-review entirely" path, not the "in-progress substantive review" path, so the in-the-moment error message was the gap. behavior unchanged for legitimately-unread source regions. |
||
|
Colin McDonnell
|
b9f0938405 |
mcp: restore operational guidance dropped in #723
#723's revision pass cut four substantive strings along with the negative anchors. those strings address real, audit-observed failure modes and the positive examples don't carry them. restored: - push_branch: "if the response reports a timeout, the underlying push may have actually succeeded — verify with git log origin/<branch> before retrying" (was on the tool description) - create_pull_request_review commit_id .describe(): "must be the FULL 40-character SHA — abbreviated SHAs are rejected by GitHub with 422" - create_pull_request_review comments[].line .describe(): "must sit inside a `@@` hunk... dropped entries are reported under droppedComments in the response" - create_pull_request_review comments[].start_line .describe(): "both start_line and line must sit inside the same @@ hunk" also: get_commit_info example used a 31-character SHA (non-standard truncation). swapped to a 7-char short form, which is what git log --oneline emits and what agents see in practice. note that this tool accepts either full or abbreviated, unlike create_pull_request_review which requires full. |
||
|
Colin McDonnell
|
b8ac42e875 |
mcp: embed example calls in top-level tool descriptions (#723)
* mcp: embed example calls in top-level tool descriptions
agents (esp. claude sonnet) hallucinate param names from training-data
priors — `pr_number` instead of `pull_number`, `summary` instead of
`body`, full subcommand strings jammed into `git({command})` like it
were `shell({command})`. each error burns a tool round-trip plus a
follow-up ToolSearch, ~40+ events / 24h, no observable recovery cost
to us but visible to users in agent logs.
cheapest fix: add a sample formatted function call to every affected
tool's top-level description. example anchors are more reliable than
schema descriptions alone because the model treats descriptions as
narrative but call examples as canonical structure. for `git` and
`shell` (whose `command` fields collide), include explicit
counter-examples disambiguating which tool owns which shape.
no schema aliases / coercion yet — try the cheap thing first; if the
next audit window still shows the same hallucination rate, layer
aliases on top per #585's recommendation.
closes #585, closes #701
* mcp: drop negative anchors from tool descriptions
negation is a footgun in tool descriptions — telling the model "NOT
pr_number" makes pr_number more salient, not less. let the positive
example carry the schema and trust the model to read it.
removes:
- "the parameter is pull_number (a number), NOT pr_number" and
similar across checkout_pr, get_pull_request, list_pull_request_reviews,
get_review_comments, create_pull_request_review
- "NOT summary, message, or content" on report_progress
- "WRONG: git({ command: 'log --oneline' })" counter-example on git
- redundant param-type restatements after the example (e.g. "depth is a
number, not a string" on git_fetch, "description is required" on shell)
keeps a single positive example per tool. for tools with multiple call
shapes (git, git_fetch, push_branch), two positive examples instead of
one + a counter-example.
|
||
|
David Blass
|
5518890b18 |
learnings: TOC + section taxonomy + 100k cap, hygiene rules, tool-quirk descriptions (#717)
* audit learnings: reshape reflection prompt + bake tool quirks into descriptions (#619) Cross-repo audit of the 48 repos with non-null learnings turned up two recurring failure modes: 1. ~25-30% of bullets across the most-active repos are pullfrog-tool quirks ("shell timeout is in milliseconds", "git args must be a JSON array", "create_pull_request_review drops out-of-hunk comments", "push_branch may report timeout when push succeeded", "checkout_pr shallow.lock retries", "commit_id needs full 40-char SHA"). These are universal across repos and should live in tool descriptions, not be rediscovered and stored 48 times. Tool descriptions now surface them. 2. Bullets are routinely 200-1000 chars (paragraph-length), and 12 of 48 repos are at the 10k cap. The reflection prompt now: caps bullets at ~240 chars (one specific fact), bans PR/review/commit/date-anchored facts that decay within weeks, bans tool-quirk learnings, and tells the agent that cap pressure means compress+prune existing bullets, not skip new findings. Co-authored-by: Cursor <cursoragent@cursor.com> * learnings: add server-generated TOC, fixed section taxonomy, raise cap to 100k (#707) Cap goes 10k → 100k. Reads stay bounded because the seeded file now opens with a server-generated table of contents listing every `## ` section's line range — agents read the TOC, then `read_file offset/limit` just the sections relevant to the current task instead of slurping the whole file. ## Section taxonomy (fixed) `## Build & test`, `## CI`, `## Conventions`, `## Architecture`, `## Gotchas`. Free-form `### ` sub-headings inside a section are fine. Pre-taxonomy free-text rows get wrapped in a `## Legacy` carve-out on first seed so they remain visible while the agent gradually re-curates them during reflection turns. ## Storage shape unchanged `Repo.learnings` still holds raw markdown (no schema migration). The TOC is a pure read-side affordance: prepended at seed time, stripped from the agent-edited file before persist. Markers `<!-- pullfrog-learnings-toc:* -->` delimit the strip region. Agent edits inside the markers are discarded. ## Round-trip semantics `seedLearningsFile` now returns `{ path, canonicalSeed }` where `canonicalSeed` is the post-TOC body — same shape `readLearningsFile` returns at end-of-run, so `persistLearnings` byte-compares them directly to skip the no-op PATCH. Empty-repo first runs end up with the section scaffold both as seed and as read-back, so untouched runs still short-circuit cleanly. ## Reflection prompt Adds explicit section-placement guidance (place each new bullet under the most relevant `## `; do NOT add new top-level headings; do NOT edit anything between the TOC markers). Carries forward the bullet hygiene from the previous commit: ≤240 chars per bullet, no pullfrog-tool quirks (those belong in tool descriptions), no PR/review/commit/date references. The "near cap" framing is replaced with "compress and prune within a section when it grows noisy" since the cap pressure that drove cramming is gone. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal round 1: line-anchored taxonomy detect, partial-merge, line-boundary truncation, scaffold-empty UI Multi-lens review of the TOC + taxonomy diff surfaced a cluster of correctness and operational bugs. Fixes: - `hasAnyTaxonomyHeading` used `String.includes("## X")` which false-positives on `### X` (the `## ` substring sits inside `### `), prose containing `## CI`, fenced code documenting markdown, etc. Replaced with a line-anchored predicate that reuses `parseHeadings` so detection and TOC construction stay consistent. - The "any heading present → pass through verbatim" rule meant a body with one taxonomy heading would seed without the other four. Worse, requiring all five would flip a body back into Legacy when the agent legitimately pruned a section to empty. New `partial` kind: keep existing content in place, append missing sections in canonical order so the agent always has the full scaffold without losing pruning intent. - `stripLearningsToc` collapsed `\n{3,}` globally; `canonicalSeed` doesn't, so an untouched body with intentional triple-newline spacing would compare unequal and burn a spurious LearningsRevision row each run. Drop the global collapse — only the leading newlines that the strip itself introduces are normalized. - 100k truncation via `slice(0, 100_000)` could cut mid-line, breaking `parseHeadings` (whole-line `^## `) on the next seed and flipping a cut body back into Legacy. New `truncateAtLineBoundary` cuts at the last newline before the cap. - `LearningsSection.tsx` rendered a scaffold-only body as "has learnings" instead of the empty placeholder. Added a `hasOnlyEmptyScaffold` guard so the console behaves the same as pre-PR for the empty case. - Seed log line distinguishes `kind=structured/partial/legacy-wrapped/ empty` instead of `existing=yes/no`, so operators can spot legacy migration activity in logs. - New tests cover: substring false-positive (`### Build & test`, in-prose mentions), partial-taxonomy merge (no Legacy wrap), full-taxonomy structured pass-through, last-newline truncation, triple-newline preservation. Deferred (documented in PR body): deploy-ordering footgun (action before API), rollback for rows >10k, Gemini sanitizer dropping `description` on `anyOf` branches, reflection-on-failed-runs. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal r2: hard-truncate fallback when line boundary discards >4k Round-2 review caught a regression in `truncateAtLineBoundary`: when the only newline within the first 100k chars sits near the start (e.g. one heading + 100k+ char single line — pathological pasted log dumps), the line-boundary cut discards almost all of the body. losing one partial line is preferable to losing kilobytes; threshold the fallback at 4k. Co-authored-by: Cursor <cursoragent@cursor.com> * move TOC out of file: prompt-side rendering, server-parsed headings drops the in-file TOC + fixed taxonomy in favor of: - file on disk = verbatim Repo.learnings (no markers, no scaffold) - server parses headings (mdast-util-from-markdown) at run-context time and returns them as RepoSettings.learningsHeadings - action renders heading TOC into the LEARNINGS prompt section as parenthesized line ranges like `Build & test (L1-L42)` with hierarchy via 2-space indent off the shallowest depth - reflection prompt teaches agent-curated structure with a soft 300-line per-section cap and explicit guidance to restructure flat legacy lists cuts 8 helpers (ensureSections, stripLearningsToc, assembleFile, buildTocBlock, parseHeadings, buildSectionScaffold, hasAnyTaxonomyHeading, LEARNINGS_SECTIONS) and the canonicalSeed round-trip dance. action seedLearningsFile is now { path } only; main.ts byte-compares the trimmed read-back against (current ?? "").trim() to gate the persist PATCH. truncateAtLineBoundary kept for safety. new tests: - test/learningsToc.test.ts (11 parser cases incl. fenced-code, blockquote, arbitrary h1-h6 nesting, startLine-points-at-heading invariant) - action/utils/learningsTocRender.test.ts (7 renderer cases) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Colin McDonnell <colinmcd94@gmail.com> |
||
|
Colin McDonnell
|
159389fad2 |
fix(mcp): sanitize for gemini when model is unresolved (#697)
* fix(mcp): sanitize for gemini when model is unresolved isGeminiRouted() previously required the effective model string to contain "gemini" — but when payload.model="auto" (or any unresolved slug) reaches addTools(), `effective` is the literal "auto", which doesn't match. opencode then auto-selects gemini *after* the MCP server has registered raw arktype schemas, and every tool turn dies on `function_declarations[*].properties[*].any_of[*].enum: only allowed for STRING type`. widen the gate: any unresolved specifier (undefined / "auto" / a slug without a `provider/` prefix) is treated as gemini-routed and sanitized. the transforms are universally compatible normalizations so the false-positive cost is negligible. tighten case 3 to preserve `description` so the only lossy path no longer drops operator-facing context. fixes #676. * revert case-3 description preservation per pullfrog review on #697: keeping `description` as a peer of `anyOf`/`oneOf` directly contradicts the file's own header (lines 19-21) and the upstream opencode #14659 rationale that gates this sanitizer — gemini requires anyOf to be the ONLY field on a schema node, sibling keywords trigger `anyOf must be the only field in a schema node`. the change was speculative scope creep with no evidence, and would silently re-introduce a different gemini failure for any future schema using `.describe().or(...)`. the bug fix for #676 doesn't need it (arktype doesn't emit non-collapsible anyOf for current tool schemas). |
||
|
Colin McDonnell
|
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 |
||
|
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.
|
||
|
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.
|
||
|
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> |
||
|
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> |
||
|
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.
|
||
|
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.
|
||
|
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. |
||
|
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>
|
||
|
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. |