From a4a5010441de9bfece579ba4e39592ea7fb695b8 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Mon, 11 May 2026 22:04:19 +0000 Subject: [PATCH] gemini-3: default thinkingLevel to medium + restrict eager prep to frozen install (#663) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * gemini-3: default thinkingLevel to medium + don't `npm ci` without a lockfile upstream opencode hardcodes `thinkingLevel: "high"` for every gemini-3 model on the direct google SDK (see `packages/opencode/src/provider/transform.ts` `options()`). that added 30-60s of pre-tool-call TTFT and 5-46s of post-tool jabber per turn, which is overkill for the tool-routing decisions that dominate agentic loops — and the variance caused the `providers-live (google/gemini-pro)` smoke job to time out at 4 minutes (see job 75405504847 on run 25684766415). three changes: - inject `provider.google.models..options.thinkingConfig.thinkingLevel = "medium"` for the two curated gemini-3 slugs in `buildSecurityConfig`. deep-merges over the upstream default; explicit `--variant high` / user opencode config still wins. flash stays at medium too — low-effort flash is visibly worse and the latency win isn't meaningful (flash is already fast). - bump the `providers-live` harness step from 4 → 6 minutes. the job-level 8-minute cap stays as the upper bound, but gemini's intrinsic TTFT variance was eating most of the 4-minute slack on its own. - in `installNodeDependencies`, pick `frozen` only when a lockfile was actually detected. previously a package.json-only repo (like the smoke fixture's `pullfrog/test-repo`) always triggered `npm ci` and emitted a noisy `EUSAGE` error before falling through. * prep: skip eager install when neither lockfile nor `packageManager` field present the previous commit changed the no-lockfile path from `npm ci` (always errored `EUSAGE`, never wrote any artifact) to a successful `npm install`, which had an unintended side effect: it generated `package-lock.json` in the working tree, tripping the post-run dirty-tree gate. the agent then committed the lockfile and opened a real PR — and in the openai/gpt smoke run on PR #663, the agent overwrote the `SMOKE TEST PASSED` output with the PR URL, failing the smoke validator. a repo with `package.json` but no lockfile and no `packageManager` field has not committed dependency state. eagerly installing produces state the repo doesn't track, which is the dirty-tree problem above. skip the eager install entirely in that case; the agent can opt in via `await_dependency_installation` when it actually needs deps. repos with a lockfile or a `packageManager` field keep the existing frozen-install behavior unchanged. * post-run: suppress dirty-tree gate in non-committing modes (Review / IncrementalReview / Plan) the dirty-tree post-run gate currently fires for every mode and tells the agent to commit and push whatever is in the working tree. that's wrong for modes that complete by submitting a review (`Review` / `IncrementalReview`) or posting a Plan comment (`Plan`) — those modes never touch files as part of their contract, so any tree dirt at end-of-run is incidental tool noise on an ephemeral worktree. nudging the agent to commit it can produce a spurious PR, as seen in the openai/gpt smoke run on PR #663 where a stray `package-lock.json` from `npm install` led the agent to open pullfrog/test-repo#32 and overwrite the smoke output. introduce `NON_COMMITTING_MODES` in `action/modes.ts` and consult it in `collectPostRunIssues`. when the selected mode is read-only, log the suppression for visibility but skip populating `issues.dirtyTree`. modes that legitimately commit (`Build`, `AddressReviews`, `Fix`, `ResolveConflicts`, `Task`) keep the existing nudge. * prep: restore eager frozen-install, drop non-frozen fallback eager dependency prep is non-mutating by contract — it runs before the agent starts and any artifact it leaves in the tree (e.g. a generated `package-lock.json`) trips the dirty-tree post-run gate and can lead the agent to open a spurious PR (seen on the openai/gpt smoke run earlier in this PR). revert the previous skip-when-no-lockfile branch: that was the wrong layer to enforce the invariant. instead, run `frozen` (`npm ci` / `pnpm install --frozen-lockfile` / etc.) unconditionally and drop the `|| install` fallback that could silently mutate the tree when `frozen` is missing. frozen commands fail cleanly without writing artifacts when there's no lockfile, which is exactly the safety contract we want. repos that need a real install must opt in explicitly via a `setup` lifecycle hook. * review nits: single getGitStatus call, tighten gemini-3 override scope comment addresses two inline nits from the PR review: - `collectPostRunIssues` was calling `getGitStatus()` (spawns `git status --porcelain`) in both branches of the mode check. lift the call above the conditional and branch on the result; same behavior, one git invocation. - the JSDoc on `GEMINI_3_DIRECT_API_IDS` said the override applies "across the board," but the constant only covers the two curated slugs in `action/models.ts`. tighten the wording to call out that other gemini-3 ids in models.dev keep the upstream "high" default. skipped the bot's yarn-1 concern after reading yarn 1's `install.js`: `bailout()` (lines 461-465) throws `frozenLockfileError` when `frozenLockfile && (!lockfileClean || missingPatterns.length > 0)`, which fires before `linker.init()` writes node_modules or runs lifecycle scripts. the existing comment's claim that frozen commands fail without artifacts holds for yarn 1 too. --- agents/opencode.ts | 34 +++++++++++++++++++++++++++++++++ agents/postRun.ts | 16 +++++++++++++++- modes.ts | 15 +++++++++++++++ prep/installNodeDependencies.ts | 13 ++++++++++--- 4 files changed, 74 insertions(+), 4 deletions(-) diff --git a/agents/opencode.ts b/agents/opencode.ts index e0da910..7aefcc4 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -80,6 +80,26 @@ type OpenCodeConfig = { */ const PULLFROG_OPENCODE_OUTPUT_LIMIT = 5000; +/** + * upstream opencode hardcodes `thinkingLevel: "high"` as the default for every + * gemini-3 model on the direct google SDK (`provider/transform.ts` `options()`). + * that adds 30-60s of pre-tool-call TTFT and 5-46s of post-tool jabber per turn, + * which is overkill for agentic loops where most steps are tool-routing + * decisions. we override to "medium" for the curated slugs we ship in + * `action/models.ts`; users who want max quality can still pick the `-high` + * variant explicitly. flash stays at "medium" too — low-effort flash is + * visibly worse on harder tasks and the latency savings aren't meaningful + * (flash is already fast). other gemini-3 ids that exist in models.dev but + * aren't in our curated alias map keep the upstream `"high"` default. + * + * keyed by upstream api id (matches the slugs in `action/models.ts`). the + * merge order in opencode `session/llm.ts` is `base ← model.options ← agent.options ← variant`, + * deep-merged — so an explicit `--variant high` still wins, and explicit + * model.options in a user-provided opencode config would also win. + */ +const GEMINI_3_DIRECT_THINKING_LEVEL = "medium"; +const GEMINI_3_DIRECT_API_IDS = ["gemini-3.1-pro-preview", "gemini-3-flash-preview"]; + function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): string { const config: OpenCodeConfig = { permission: { @@ -94,6 +114,20 @@ function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): s [pullfrogMcpName]: { type: "remote", url: ctx.mcpServerUrl }, }, agent: buildReviewerAgentConfig(), + provider: { + google: { + models: Object.fromEntries( + GEMINI_3_DIRECT_API_IDS.map((id) => [ + id, + { + options: { + thinkingConfig: { thinkingLevel: GEMINI_3_DIRECT_THINKING_LEVEL }, + }, + }, + ]) + ), + }, + }, }; if (model) { diff --git a/agents/postRun.ts b/agents/postRun.ts index 58d5d3e..35c64ea 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -1,5 +1,6 @@ import { readFile } from "node:fs/promises"; import { LIFECYCLE_HOOK_TIMEOUT_MS } from "../lifecycle.ts"; +import { NON_COMMITTING_MODES } from "../modes.ts"; import type { ToolState } from "../toolState.ts"; import { log } from "../utils/cli.ts"; import { @@ -181,8 +182,21 @@ export async function collectPostRunIssues( const failure = await executeStopHook(ctx.stopScript); if (failure) issues.stopHook = failure; } + // dirty-tree gate fires only in modes that legitimately commit. Review / + // IncrementalReview / Plan complete via review submission or a Plan + // comment, not by touching files — any tree dirt is incidental (e.g. a + // tool-installed `node_modules/`) and the worktree is ephemeral, so + // nudging the agent to commit it would produce a spurious PR. see + // `NON_COMMITTING_MODES` in `action/modes.ts`. const status = getGitStatus(); - if (status) issues.dirtyTree = status; + const mode = ctx.toolState.selectedMode; + if (status) { + if (mode && NON_COMMITTING_MODES.has(mode)) { + log.info(`» dirty-tree gate suppressed: mode \`${mode}\` does not commit`); + } else { + issues.dirtyTree = status; + } + } const summaryFilePath = ctx.toolState.summaryFilePath; const summarySeed = ctx.toolState.summarySeed; if (!options.skipSummaryStale && summaryFilePath && summarySeed !== undefined) { diff --git a/modes.ts b/modes.ts index 9bde11a..d713824 100644 --- a/modes.ts +++ b/modes.ts @@ -433,3 +433,18 @@ ${PR_SUMMARY_FORMAT}`, // static export for UI display — uses opencode format as the readable default export const modes: Mode[] = computeModes("opencode"); + +/** + * modes that legitimately never modify the working tree. used by the post-run + * dirty-tree gate to suppress the "commit and push" nudge — those modes + * complete by submitting a review (`Review` / `IncrementalReview`) or by + * posting a Plan comment (`Plan`), not by touching files. any leftover in the + * tree at end-of-run is incidental tool noise (e.g. a `node_modules/` from a + * stray install attempt) on an ephemeral worktree; nudging the agent to + * commit it would produce a spurious PR. + */ +export const NON_COMMITTING_MODES: ReadonlySet = new Set([ + "Review", + "IncrementalReview", + "Plan", +]); diff --git a/prep/installNodeDependencies.ts b/prep/installNodeDependencies.ts index d315217..034bb48 100644 --- a/prep/installNodeDependencies.ts +++ b/prep/installNodeDependencies.ts @@ -135,14 +135,21 @@ export const installNodeDependencies: PrepDefinition = { } } - // get the frozen install command (or fallback to regular install) - const resolved = resolveCommand(agent, "frozen", []) || resolveCommand(agent, "install", []); + // frozen-lockfile install only. eager prep is non-mutating by contract: + // we run it before the agent starts and any artifact it leaves in the + // tree (e.g. a generated `package-lock.json`) trips the dirty-tree + // post-run gate and produces a spurious PR. `frozen` commands + // (`npm ci`, `pnpm install --frozen-lockfile`, etc.) fail cleanly + // without modifying state when there's no lockfile, which is exactly + // what we want — repos that need a non-frozen install must opt in via + // a `setup` lifecycle hook (`action/utils/lifecycle.ts`). + const resolved = resolveCommand(agent, "frozen", []); if (!resolved) { return { language: "node", packageManager, dependenciesInstalled: false, - issues: [`no install command found for ${agent}`], + issues: [`no frozen-install command available for ${agent}`], }; }