b6c57547ca5e841fefd6801cfeeb4d874c0eeaaa
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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. |
||
|
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
|
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
|
ba7f5a0b89 |
action: surface agent hang context in progress comment (#733)
* action: surface agent hang context in progress comment When the activity-timeout watchdog kills a stalled opencode subprocess, the user used to see a bare "activity timeout: no output for 30Xs" — no provider context, no stderr trace, no clue why the run died. Investigation of the six runs in #728 showed the same shape every time: opencode hangs after a non-retryable provider event (auth 401, 502 stream lost, free-tier flake), and the only useful signal was buried in stderr where the user couldn't see it without diving into Actions logs. Stop trying to prevent the hang. Surface it. Add a small `AgentDiagnostic` handle on `toolState` that the harness mutates as a run progresses (recent stderr ring buffer reference, last provider-error label, event count). `formatAgentHangBody` renders that into a markdown body — bold headline, one-line explanation, collapsible `<details>` with the last ~10 stderr lines (capped to 3KB) — used by both the agent harness's own catch path and main.ts's outer catch when the watchdog wins the race against the harness. Both paths converge on one formatter; the existing "View workflow run ➔" footer affordance in `reportErrorToComment` is unchanged, so the user still has one click from the comment to the raw logs to develop their own thesis. * address review: gate hang body on isHang; fix contradictory copy - Only render `hangBody` when `isHang`. The harness sets `agentDiagnostic` on entry, so any non-hang throw past `runOpenCode`'s own catch (post-success `output_schema` validator, late cleanup throws) was rendering "Pullfrog failed — N events processed…" with the real exception message dropped — including for runs that actually succeeded before a late throw. - When `lastProviderError` already names the cause in the headline, the zero-events sentence "check whether the model provider is reachable" contradicts it (a 401 produces zero events but isn't a reachability issue). Drop the nudge in that case; keep it for the silent-stall path where it's still actionable. * address copilot review: fence escape, idle parsing, secret redaction, tests - pick a backtick fence longer than any backtick run in the rendered stderr tail. opencode error JSON occasionally embeds triple backticks in tool input dumps; the fixed three-tick fence let those terminate the fence early and corrupt the rest of the comment markdown. - parse idle seconds out of the timer reject string ("activity timeout: no output for 301s") and use that for the hang explanation. previously rendered total runtime, which overstated the stall by 20+ minutes for runs that streamed for a long time before going quiet (e.g. Rohithgilla12/data-peek#25784038918, 1230s elapsed but 304s idle). - redact sensitive env-var values from the rendered stderr tail before it lands in the PR comment / job summary. workflow log writes already go through `core.setSecret` masking; PR comments and summaries bypass that pipeline entirely. matches against `isSensitiveEnvName` (the same *_KEY/*_TOKEN/*_SECRET/*_PASSWORD/*_CREDENTIAL surface that `normalizeEnv` registers with the runner) and only redacts values >= 8 chars to avoid false-positive substring hits. - add `agentHangReport.test.ts` covering the branchy bits: idle-seconds parsing, eventCount-zero copy with and without provider error, fence-escape against embedded triple backticks, 3 KB tail truncation, null-on-no-diagnostic, and secret redaction. `startedAtMs` is dropped from `AgentDiagnostic` — total runtime was the only consumer and idle seconds replaces it. * strip slop: drop tests, drop redactSecrets, simplify ternary - delete `agentHangReport.test.ts`. half the cases just pinned literal copy ("**Pullfrog stalled**", "check whether the model provider is reachable") which is exactly the "performative tests to every string utility" pattern AGENTS.md flags. the other half tested 2-5 line pure helpers (parseIdleSec / pickFence / truncation) that code review catches. the formatter is a best-effort string output; pinning it in tests creates churn without catching real regressions. - remove `redactSecrets` and revert the formatter's import. theatrical defense: opencode doesn't dump env on startup, bearer tokens aren't in request bodies, bash is denied. the action has many other PR-comment write paths that don't redact (comment.ts, errorReport.ts, the progress writer) — if PR-comment secret hygiene matters, it's a cross-cutting concern at the comment-write layer, not bolted onto one formatter. - factor the explanation triple-ternary into `formatExplanation` with early returns. same logic, easier to read. `isHang` gate, fence-length escaping, and idle-seconds parsing stay — those are real correctness fixes. |
||
|
Colin McDonnell
|
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.
|