From fa2d50feee868b363f7196bf5c0f9261c7b6425a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:42:55 -0700 Subject: [PATCH] fix(source-control): generate clean OpenCode messages locally and over SSH (#24613) * fix(source-control): generate clean OpenCode answers on local and SSH hosts Restack the original focused change onto current main, preserving every owned source and test blob and the merged CI contract and journal cleanup fixes. Original-commit: 64bb15e3f20799a21f108c849b0bdff83c692712 fix(source-control): generate clean OpenCode answers on local and SSH hosts Use configured models and JSON answer/error events, preserve run-first arguments, and handle the precise v2 variant rejection. Hydrate SSH execution-host PATH through the existing bounded login environment resolver before direct spawning. Credits: andy-murr (PR #5197 SSH environment intent) and coelho-doti (PR #13065 argument-order intent). Original-commit: 1b60ec5d11132a5fed0695faea5de782a228a8e5 fix(source-control): retry inline OpenCode model and variant options Original-commit: 98fdecc7a33ef1571dbf2854f309e4bfa1ec4300 Preserve OpenCode named errors without a data message Restacked-from: 98fdecc7a33ef1571dbf2854f309e4bfa1ec4300 Restacked-onto: f7b1f9d8be14d12244957ee0b710a3482ec62383 * fix(ci): prevent concurrent pnpm refresh during mobile typechecks (#24776) * fix(ci): run mobile typechecks without concurrent dependency refresh * test(ci): check effective Linux E2E package list * test(ci): preserve the mobile production compiler barrier --------- Co-authored-by: Orca Integration Recovery * test(terminal): restore the live fish fixture prerequisites (#24947) A restored pane waits for the initial status replay before subscribing to PTY output. This fixture never settled that replay, so fish printed its mode-2031 arm before the renderer connected. Its PTY API also omitted the reset-input listener required by the serializer, aborting attachment. Settle and dispose the existing startup-snapshot registration and provide the same reset-listener mock used by the other PTY tests. The real fish child-stdin assertions and timeouts remain unchanged. No production change. * fix(shortcuts): defer TUI editing chords in terminal-first mode (#24640) Restack the original focused change onto current main, preserving every owned source and test blob and the merged CI contract and journal cleanup fixes. Original-commit: f707cde14ab8cb2670b4076916cdfb7e28c79983 fix(shortcuts): defer TUI editing chords in terminal-first mode Original-commit: 0c6348e49e0d968e0170961d294a018adceaa9f3 docs(shortcuts): describe deferred preview terminal chords Original-commit: be62c1b6c6b305cb8091161c1ccdd943a6e1d470 Align worktree history shortcut metadata with terminal conflict policy Restacked-from: be62c1b6c6b305cb8091161c1ccdd943a6e1d470 Restacked-onto: f7b1f9d8be14d12244957ee0b710a3482ec62383 * Register supervised Qoder China and Qwen Code (#24616) * Add Qoder session history and search with real CLI coverage * Allow the real Qoder marker file to end with a newline * Keep Qoder tool output out of history previews and search * Keep Qoder search pages readable by older clients * Verify persisted Qoder history after a real generated and resumed task * Negotiate Qoder filters before searching an older execution host * Combine search client imports for the CI plugin gate * Keep the relay search oracle aligned with legacy agent filtering * Register supervised Qoder China and Qwen lifecycle integration * Cover Qoder China mobile assets and mixed-host resume gates * Verify Qoder provider tags against the older released wire parser * Verify China and Qwen keep independent Windows hook scripts * Verify Qoder registrations against the installed older Windows release * test(qoder): align search capability contracts and pin old-host fencing * fix(qoder): rank exact picker identities and command aliases first * test(qoder): preserve the regional CLI shared icon expectation Keep the full bundled-asset and no-remote-image checks, with an explicit shared-logo basename for Qoder China. The map also works with older catalog type unions. * fix(qoder): align China catalog entry with fallback order --------- Co-authored-by: Orca Integration Recovery * Use the measured pnpm lookup policy automatically in hosted root CI (#24951) * Select lookup automatically for the measured hosted root-install profile * Record hosted automatic-mode cold cache publication proof * fix: bound remote generation setup and honor OpenCode option terminators Count execution-host profile resolution inside the existing request deadline, cancel its waiter promptly, and pass only the remaining time to the child. Shared bounded profile probes keep their existing cache lifetime; the SSH transport margin is unchanged. Read OpenCode output format from active final-argv options before -- so literal prompt arguments cannot select the JSON finalizer. Fresh exact-source controls reproduce nine failures before; 139 related checks pass after, including primary/fallback delays, deadline boundaries, cancellation, parser metadata, and SSH lanes. Node typecheck and strict changed-file lint pass. * Continue Antigravity IDE and 2.0 history in new CLI conversations (#24692) * feat(antigravity): bridge IDE history into new CLI conversations * fix(antigravity): preserve fresh-launch model and environment for IDE references * fix(antigravity): forward IDE history opt-in through desktop IPC * fix(antigravity): rebuild remote IDE reference startup on its host * fix(antigravity): register IDE continuation action labels * fix(antigravity): confine IDE references and bound metadata reads * fix(antigravity): localize IDE continuation badges * Preserve scanner service cache assertions and refresh Antigravity opening metadata * Preserve Antigravity opening joins and target folder runtime authority * fix(opencode): retry timed-out SSH plugin updates (#24666) Preserve bounded retry behavior and the current-main status-envelope fields. Original-PR: #24124 Reviewed-source: 103144f9c5029f9217f64970271a5b7f69d3f823 Co-authored-by: Justas Brazauskas * fix(opencode): keep Go credentials private and resolve backend keys (#24615) Preserve the complete credential storage, migration, IPC, Settings and rate-limit refresh change alongside standalone GLM plans, current-main database diagnostics and the reviewed unknown-backend environment correction. Keep native discovery cancellation third and selected environment fourth. Original-topic-commit: 7903f1cddbc08fe1179f8bd964fff3dcb0e5d020 Original-topic-commit: 588117b5cf1dd57e2310c80068e3ddcc9f9044ec Original-topic-commit: dedd4f8c862a961393977dc3c2547f049391b509 Original-topic-commit: 6645dae104ee5271139fcc0e9564d9223b74835d Original-topic-commit: a25b80c02c1af7830b0e6a65e72d965b3ad98276 Restacked-from: a25b80c02c1af7830b0e6a65e72d965b3ad98276 Restacked-onto: b032867021c7ef1436ca606559e9d786580a84dc Co-authored-by: kespineira Co-authored-by: kevimux Reported-by: pullfrog Reviewed-full-source: 849fe093073f4c1606bd65d79a0c725d955d0d1f Native-helper-source: 80dbe23237db191c1e6280001c7e0563f3ca8846 Reviewed-full-current-source: 36acb57d44adb3d378c0289c8c15f7da0fda214c * fix(opencode): enforce deadline through executable startup Keep synchronous Windows PATH resolution and child startup within the existing request budget. --------- Co-authored-by: Orca Integration Recovery Co-authored-by: Justas Brazauskas --- .../ssh-git-noninteractive-provider.ts | 4 +- .../ssh-git-provider-commit-message.test.ts | 9 +- .../command-environment-process.test.ts | 5 +- .../commit-message-agent-environment.ts | 4 +- .../commit-message-model-discovery-policy.ts | 6 + .../opencode-generation-events.test.ts | 96 +++++ .../source-control-agent-failure.ts | 23 +- .../source-control-local-process.ts | 1 + .../source-control-remote-generation.ts | 1 + ...source-control-text-generation-requests.ts | 39 +- src/relay/agent-exec-handler.ts | 109 ++++-- .../agent-exec-shell-environment.test.ts | 356 ++++++++++++++++++ src/shared/command-option-occurrence.ts | 38 ++ src/shared/commit-message-agent-spec.test.ts | 8 +- .../commit-message-agent-specs-primary.ts | 26 +- src/shared/commit-message-plan.test.ts | 78 +++- src/shared/commit-message-plan.ts | 49 +-- .../opencode-generation-command.test.ts | 94 +++++ src/shared/opencode-generation-command.ts | 44 +++ src/shared/opencode-generation-output.test.ts | 84 +++++ src/shared/opencode-generation-output.ts | 53 +++ 21 files changed, 1000 insertions(+), 127 deletions(-) create mode 100644 src/main/text-generation/opencode-generation-events.test.ts create mode 100644 src/relay/agent-exec-shell-environment.test.ts create mode 100644 src/shared/command-option-occurrence.ts create mode 100644 src/shared/opencode-generation-command.test.ts create mode 100644 src/shared/opencode-generation-command.ts create mode 100644 src/shared/opencode-generation-output.test.ts create mode 100644 src/shared/opencode-generation-output.ts diff --git a/src/main/providers/ssh-git-noninteractive-provider.ts b/src/main/providers/ssh-git-noninteractive-provider.ts index 0550b1bba39..c0d8df0573f 100644 --- a/src/main/providers/ssh-git-noninteractive-provider.ts +++ b/src/main/providers/ssh-git-noninteractive-provider.ts @@ -29,7 +29,8 @@ export class SshGitNoninteractiveProvider extends SshGitReadProvider { stdin: plan.stdinPayload, ...(plan.env ? { env: plan.env } : {}), timeoutMs, - operation + operation, + shell: true }, undefined, operation @@ -100,6 +101,7 @@ export class SshGitNoninteractiveProvider extends SshGitReadProvider { timeoutMs: number env?: Record operation?: string + shell?: boolean }, signal?: AbortSignal, operation?: string diff --git a/src/main/providers/ssh-git-provider-commit-message.test.ts b/src/main/providers/ssh-git-provider-commit-message.test.ts index be024b643c8..70d31d6a4f2 100644 --- a/src/main/providers/ssh-git-provider-commit-message.test.ts +++ b/src/main/providers/ssh-git-provider-commit-message.test.ts @@ -179,7 +179,8 @@ describe('SshGitProvider', () => { stdin: null, env: { FLAG: 'literal $HOME' }, timeoutMs: 60_000, - operation: 'commit-message' + operation: 'commit-message', + shell: true }, { timeoutMs: 65_000 } ) @@ -228,7 +229,8 @@ describe('SshGitProvider', () => { cwd: '/home/user/repo', stdin: null, timeoutMs: 60_000, - operation: 'commit-message' + operation: 'commit-message', + shell: true }, { timeoutMs: 65_000 } ) @@ -241,7 +243,8 @@ describe('SshGitProvider', () => { cwd: '/home/user/repo', stdin: null, timeoutMs: 60_000, - operation: 'pull-request-fields' + operation: 'pull-request-fields', + shell: true }, { timeoutMs: 65_000 } ) diff --git a/src/main/text-generation/command-environment-process.test.ts b/src/main/text-generation/command-environment-process.test.ts index bdc04daca3f..3ae17c6e60d 100644 --- a/src/main/text-generation/command-environment-process.test.ts +++ b/src/main/text-generation/command-environment-process.test.ts @@ -155,6 +155,9 @@ describe('environment-prefixed commands with real child processes', () => { it('discovers models through the same override environment', async () => { await expect( discoverCommitMessageModelsLocal('opencode', target.env, override, { cwd: folder }) - ).resolves.toMatchObject({ success: true, models: [{ id: 'anthropic/claude-sonnet-4' }] }) + ).resolves.toMatchObject({ + success: true, + models: [{ id: 'default' }, { id: 'anthropic/claude-sonnet-4' }] + }) }) }) diff --git a/src/main/text-generation/commit-message-agent-environment.ts b/src/main/text-generation/commit-message-agent-environment.ts index 3c5523bc420..588a56058e1 100644 --- a/src/main/text-generation/commit-message-agent-environment.ts +++ b/src/main/text-generation/commit-message-agent-environment.ts @@ -51,7 +51,7 @@ function readInheritedOrShellEnvVar(name: string, sourceName?: string): string | function prepareShellConfigDirEnv(agentId: string): { ok: true; env?: NodeJS.ProcessEnv } | null { const configVar = - agentId === 'opencode' + agentId === 'opencode' || agentId === 'opencode2' ? 'OPENCODE_CONFIG_DIR' : agentId === 'pi' || agentId === 'omp' ? 'PI_CODING_AGENT_DIR' @@ -66,7 +66,7 @@ function prepareShellConfigDirEnv(agentId: string): { ok: true; env?: NodeJS.Pro // the Pi one (and vice versa). PI_CODING_AGENT_DIR is the binary-facing var // both kinds consume — see src/main/pi/titlebar-extension-service.ts. const sourceVar = - agentId === 'opencode' + agentId === 'opencode' || agentId === 'opencode2' ? 'ORCA_OPENCODE_SOURCE_CONFIG_DIR' : agentId === 'pi' ? 'ORCA_PI_SOURCE_AGENT_DIR' diff --git a/src/main/text-generation/commit-message-model-discovery-policy.ts b/src/main/text-generation/commit-message-model-discovery-policy.ts index fa2b8869a37..41bef79fb18 100644 --- a/src/main/text-generation/commit-message-model-discovery-policy.ts +++ b/src/main/text-generation/commit-message-model-discovery-policy.ts @@ -57,6 +57,12 @@ export function finalizeModelDiscoveryOutput( } return { success: false, error: `${spec.label} returned no available models.` } } + if (spec.id === 'opencode' || spec.id === 'opencode2') { + const configuredDefault = spec.models.find((model) => model.id === 'default') + if (configuredDefault && !models.some((model) => model.id === 'default')) { + models = [configuredDefault, ...models] + } + } // A sentinel model in the static spec (for example `default`) means the CLI // should keep its configured provider even when discovery lists concrete models. const defaultModelId = diff --git a/src/main/text-generation/opencode-generation-events.test.ts b/src/main/text-generation/opencode-generation-events.test.ts new file mode 100644 index 00000000000..894f70e679f --- /dev/null +++ b/src/main/text-generation/opencode-generation-events.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from 'vitest' +import { + generateCommitMessageFromContext, + generateBranchNameFromContext +} from './commit-message-text-generation' +import { finalizeModelDiscoveryOutput } from './commit-message-model-discovery-policy' +import { getCommitMessageAgentSpec } from '../../shared/commit-message-agent-spec' + +const text = (answer: string): string => + JSON.stringify({ type: 'text', part: { id: 'answer', text: answer } }) +const context = { branch: 'main', stagedSummary: 'M\tfile.ts', stagedPatch: '+new' } +const params = { agentId: 'opencode', model: 'default' } as const + +describe('OpenCode generation event handling across remote execution', () => { + it('extracts only the answer from a JSON stream', async () => { + const result = await generateCommitMessageFromContext(context, params, { + kind: 'remote', + cwd: '/repo', + missingBinaryLocation: 'remote PATH', + execute: async () => ({ + stdout: `${JSON.stringify({ type: 'tool_use', part: { text: 'tool output' } })}\n${text('fix: output only the answer')}`, + stderr: '', + exitCode: 0, + timedOut: false + }) + }) + expect(result).toEqual({ + success: true, + message: 'fix: output only the answer', + agentLabel: 'OpenCode' + }) + }) + + it.each([0, 1])('does not accept an error event with exit code %s', async (exitCode) => { + const result = await generateCommitMessageFromContext(context, params, { + kind: 'remote', + cwd: '/repo', + missingBinaryLocation: 'remote PATH', + execute: async () => ({ + stdout: JSON.stringify({ + type: 'error', + error: { type: 'provider.no-route', message: 'Model unavailable' } + }), + stderr: '', + exitCode, + timedOut: false + }) + }) + expect(result).toMatchObject({ + success: false, + error: expect.stringContaining('Model unavailable') + }) + }) + + it('retries the v2 flag rejection on the same execution host', async () => { + const argv: string[][] = [] + const result = await generateBranchNameFromContext( + { firstPrompt: 'Fix generation' }, + { ...params, model: 'fixture/chat', thinkingLevel: 'high' }, + { + kind: 'remote', + cwd: '/repo', + missingBinaryLocation: 'remote PATH', + execute: async (plan) => { + argv.push(plan.args) + return argv.length === 1 + ? { + stdout: 'Help', + stderr: 'ERROR\nUnrecognized flag: --variant in command opencode run', + exitCode: 1, + timedOut: false + } + : { stdout: text('fix-generation'), stderr: '', exitCode: 0, timedOut: false } + } + } + ) + expect(result).toMatchObject({ success: true, slug: 'fix-generation' }) + expect(argv[1]).toContain('fixture/chat#high') + expect(argv[1]).not.toContain('--variant') + }) + + it.each(['opencode', 'opencode2'] as const)( + 'retains Config default when %s discovery lists explicit models', + (agent) => { + const spec = getCommitMessageAgentSpec(agent) + if (!spec) { + throw new Error('missing spec') + } + expect(finalizeModelDiscoveryOutput(spec, 'fixture/chat\n', '', 0)).toMatchObject({ + success: true, + defaultModelId: 'default', + models: [{ id: 'default' }, { id: 'fixture/chat' }] + }) + } + ) +}) diff --git a/src/main/text-generation/source-control-agent-failure.ts b/src/main/text-generation/source-control-agent-failure.ts index 3bf549f663a..c924343e94d 100644 --- a/src/main/text-generation/source-control-agent-failure.ts +++ b/src/main/text-generation/source-control-agent-failure.ts @@ -9,6 +9,7 @@ import { type AgentGenerationFailureOutput } from './agent-failure-output' import type { InternalTextGenerationResult } from './source-control-text-generation-types' +import { parseOpenCodeGenerationOutput } from '../../shared/opencode-generation-output' export function formatAgentCliFailureMessage( label: string, @@ -45,17 +46,35 @@ export function finalizeFromAgentOutput(args: { emptyResultName: string includeLocalMacDnsHint?: boolean includeStdoutDetail?: boolean + outputFormat?: 'opencode-json' }): InternalTextGenerationResult { const { code, stdout, stderr, label, emptyResultName } = args + const parsed = + args.outputFormat === 'opencode-json' ? parseOpenCodeGenerationOutput(stdout) : null if (code !== 0) { console.error('[commit-message] Generator failed:', { label, exitCode: code, stdout, stderr }) return { success: false, - error: formatAgentCliFailureMessage(label, stdout, stderr, code, args), + error: formatAgentCliFailureMessage( + label, + stdout, + parsed && !parsed.ok && parsed.error !== 'OpenCode returned invalid JSON events.' + ? parsed.error + : stderr, + code, + args + ), failureOutput: captureFailureOutput(label, code, stdout, stderr) } } - const cleaned = cleanGeneratedCommitMessage(stdout) + if (parsed && !parsed.ok) { + return { + success: false, + error: sanitizeAgentFailureDetail(parsed.error) ?? 'OpenCode reported an error.', + failureOutput: captureFailureOutput(label, code, stdout, stderr) + } + } + const cleaned = cleanGeneratedCommitMessage(parsed?.ok ? parsed.text : stdout) if (cleaned) { return { success: true, rawOutput: cleaned, agentLabel: label } } diff --git a/src/main/text-generation/source-control-local-process.ts b/src/main/text-generation/source-control-local-process.ts index da1a2f9a139..4800868d068 100644 --- a/src/main/text-generation/source-control-local-process.ts +++ b/src/main/text-generation/source-control-local-process.ts @@ -186,6 +186,7 @@ export function runLocalSourceControlPlan(input: { stdout, stderr, label: plan.label, + outputFormat: plan.outputFormat, emptyResultName: input.emptyResultName, includeStdoutDetail: operation !== 'branch-name' }) diff --git a/src/main/text-generation/source-control-remote-generation.ts b/src/main/text-generation/source-control-remote-generation.ts index ece1a6de1d8..871345787a9 100644 --- a/src/main/text-generation/source-control-remote-generation.ts +++ b/src/main/text-generation/source-control-remote-generation.ts @@ -66,6 +66,7 @@ export async function runRemoteSourceControlPlan(input: { stdout: result.stdout, stderr: result.stderr, label: plan.label, + outputFormat: plan.outputFormat, emptyResultName: input.emptyResultName, includeLocalMacDnsHint: false, includeStdoutDetail: operation !== 'branch-name' diff --git a/src/main/text-generation/source-control-text-generation-requests.ts b/src/main/text-generation/source-control-text-generation-requests.ts index a061201ac8c..298d2ac2e39 100644 --- a/src/main/text-generation/source-control-text-generation-requests.ts +++ b/src/main/text-generation/source-control-text-generation-requests.ts @@ -28,6 +28,7 @@ import type { ResolvedSourceControlAiGenerationParams } from '../../shared/sourc import { formatLinkedIssueTemplateValue } from '../../shared/source-control-ai-action-variables' import { renderSourceControlActionCommandTemplate } from '../../shared/source-control-ai-actions' import { captureAgentGenerationFailureOutput } from './agent-failure-output' +import { openCodeVariantRetryPlan } from '../../shared/opencode-generation-command' import { runLocalPlanForAgent } from './source-control-local-generation' import { runRemoteSourceControlPlan } from './source-control-remote-generation' import type { @@ -61,21 +62,29 @@ async function executeGenerationPlan(input: { operation: TextGenerationOperation spawnAgent: SpawnSourceControlAgent }): Promise { - const result = await (input.target.kind === 'remote' - ? runRemoteSourceControlPlan({ - plan: input.plan, - target: input.target, - emptyResultName: input.emptyResultName, - operation: input.operation - }) - : runLocalPlanForAgent({ - agentId: input.params.agentId, - plan: input.plan, - target: input.target, - emptyResultName: input.emptyResultName, - operation: input.operation, - spawnAgent: input.spawnAgent - })) + const execute = (plan: CommitMessagePlan): Promise => + input.target.kind === 'remote' + ? runRemoteSourceControlPlan({ + plan, + target: input.target, + emptyResultName: input.emptyResultName, + operation: input.operation + }) + : runLocalPlanForAgent({ + agentId: input.params.agentId, + plan, + target: input.target, + emptyResultName: input.emptyResultName, + operation: input.operation, + spawnAgent: input.spawnAgent + }) + let result = await execute(input.plan) + if (!result.success && input.params.agentId === 'opencode') { + const retry = openCodeVariantRetryPlan(input.plan, result.failureOutput?.stderr ?? '') + if (retry) { + result = await execute(retry) + } + } // Why: only a custom command runs a raw model whose chat template can swallow // the opening think tag; a built-in agent's message may just mention the tag. // PR fields are JSON, so they strip only when parsing fails instead. diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index 152e8bd72fb..6a4cf295a6b 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -1,4 +1,5 @@ import { mergeCommandEnvironment } from '../shared/command-environment' +import { PromiseSettlementWaiters } from '../shared/promise-settlement-waiters' import { spawn, type ChildProcess } from 'node:child_process' import { existsSync } from 'node:fs' import { delimiter, join } from 'node:path' @@ -6,6 +7,7 @@ import type { RelayDispatcher, RequestContext } from './dispatcher' import { applyTerminalGitCredentialPromptGuard } from '../shared/terminal-git-credential-guard' import { mergeGitConfigEnvProtocol } from '../shared/git-credential-prompt-env' import { terminateRelaySubprocessTree } from './subprocess-tree-termination' +import { resolveLoginShellEnvironment } from '../main/startup/login-shell-environment' const DEFAULT_TIMEOUT_MS = 60_000 const MAX_TIMEOUT_MS = 5 * 60 * 1000 @@ -76,6 +78,7 @@ type ExecParams = { timeoutMs: unknown env: unknown operation: unknown + shell: unknown } type CancelParams = { @@ -88,7 +91,7 @@ function laneKeyFor(cwd: string, operation: unknown): string { return JSON.stringify([op, cwd]) } -type InFlightExec = { child: ChildProcess; cancel: () => void } +type InFlightExec = { child?: ChildProcess; cancel: () => void } type ExecResult = { stdout: string @@ -113,10 +116,6 @@ export class AgentExecHandler { // operation lanes let cancel target only the user-visible job that stopped. private inFlightByLane = new Map() - private laneKey(cwd: string, operation: unknown): string { - return laneKeyFor(cwd, operation) - } - constructor(dispatcher: RelayDispatcher) { dispatcher.onRequest('agent.execNonInteractive', (p, context) => this.exec(p as ExecParams, context) @@ -126,7 +125,7 @@ export class AgentExecHandler { private async cancel(params: CancelParams): Promise<{ canceled: boolean }> { const cwd = typeof params.cwd === 'string' ? params.cwd : '' - const entry = this.inFlightByLane.get(this.laneKey(cwd, params.operation)) + const entry = this.inFlightByLane.get(laneKeyFor(cwd, params.operation)) if (!entry) { return { canceled: false } } @@ -144,19 +143,56 @@ export class AgentExecHandler { const stdinPayload = typeof params.stdin === 'string' ? params.stdin : null const requestedTimeout = typeof params.timeoutMs === 'number' ? params.timeoutMs : DEFAULT_TIMEOUT_MS - const timeoutMs = Math.max(1_000, Math.min(MAX_TIMEOUT_MS, requestedTimeout)) + const deadline = Date.now() + Math.max(1_000, Math.min(MAX_TIMEOUT_MS, requestedTimeout)) const extraEnv = params.env && typeof params.env === 'object' && !Array.isArray(params.env) ? (params.env as Record) : null - const baseEnv = mergeCommandEnvironment( - process.env, - extraEnv ? {} : undefined, - process.platform - ) + let hostEnv = process.env + if (params.shell === true) { + const timeoutError = new Error('Profile resolution exceeded the request deadline') + const controller = new AbortController() + const key = laneKeyFor(cwd ?? '', params.operation) + const pending = { cancel: (): void => controller.abort() } + this.inFlightByLane.get(key)?.cancel() + this.inFlightByLane.set(key, pending) + context?.signal?.addEventListener('abort', pending.cancel, { once: true }) + if (context?.signal?.aborted) { + pending.cancel() + } + try { + hostEnv = await new PromiseSettlementWaiters( + resolveLoginShellEnvironment({ env: process.env }) + ).wait({ + signal: controller.signal, + timeoutMs: Math.max(1, deadline - Date.now()), + createTimeoutError: () => timeoutError + }) + } catch (error) { + if (error === timeoutError) { + return { stdout: '', stderr: '', exitCode: null, timedOut: true } + } + if (controller.signal.aborted) { + return { stdout: '', stderr: '', exitCode: null, timedOut: false, canceled: true } + } + throw error + } finally { + context?.signal?.removeEventListener('abort', pending.cancel) + if (this.inFlightByLane.get(key) === pending) { + this.inFlightByLane.delete(key) + } + } + if (controller.signal.aborted) { + return { stdout: '', stderr: '', exitCode: null, timedOut: false, canceled: true } + } + } + if (Date.now() >= deadline) { + return { stdout: '', stderr: '', exitCode: null, timedOut: true } + } + const baseEnv = mergeCommandEnvironment(hostEnv, extraEnv ? {} : undefined, process.platform) const overrides = mergeCommandEnvironment({}, extraEnv ?? undefined, process.platform) const spawnEnv = Object.fromEntries( - Object.entries(mergeGitConfigEnvProtocol(baseEnv ?? process.env, overrides)).filter( + Object.entries(mergeGitConfigEnvProtocol(baseEnv ?? hostEnv, overrides)).filter( (entry): entry is [string, string] => typeof entry[1] === 'string' ) ) @@ -171,6 +207,10 @@ export class AgentExecHandler { let child try { const { spawnCmd, spawnArgs } = getWindowsSafeSpawn(binary, args, spawnEnv) + if (Date.now() >= deadline) { + resolve({ stdout: '', stderr: '', exitCode: null, timedOut: true }) + return + } child = spawn(spawnCmd, spawnArgs, { cwd, env: spawnEnv, @@ -195,7 +235,7 @@ export class AgentExecHandler { let timedOut = false let canceled = false let settled = false - const laneKey = typeof cwd === 'string' ? this.laneKey(cwd, params.operation) : '' + const laneKey = typeof cwd === 'string' ? laneKeyFor(cwd, params.operation) : '' let entry: InFlightExec | null = null let timer: ReturnType | null = null let detachChildListeners = (): void => {} @@ -226,22 +266,10 @@ export class AgentExecHandler { // that process until timeout because future cancelExec calls reach only // the newest map entry. this.inFlightByLane.get(laneKey)?.cancel() - entry = { - child, - cancel: cancelCurrent - } + entry = { child, cancel: cancelCurrent } this.inFlightByLane.set(laneKey, entry) } - timer = setTimeout(() => { - timedOut = true - // Why: tree-kill because some CLIs trap SIGTERM and continue streaming; - // also Windows wraps `.cmd` shims in cmd.exe, so the immediate child - // is not the real node.exe process. - terminateRelaySubprocessTree(child) - finish({ stdout, stderr, exitCode: null, timedOut, canceled }) - }, timeoutMs) - const onStdoutData = (chunk: Buffer): void => { stdoutBytes += chunk.byteLength if (stdoutBytes > MAX_OUTPUT_BYTES) { @@ -258,18 +286,10 @@ export class AgentExecHandler { } stderr += chunk.toString('utf-8') } - const onError = (error: Error): void => { - finish({ - stdout, - stderr, - exitCode: null, - timedOut, - spawnError: error.message - }) - } - const onClose = (code: number | null): void => { + const onError = (error: Error): void => + finish({ stdout, stderr, exitCode: null, timedOut, spawnError: error.message }) + const onClose = (code: number | null): void => finish({ stdout, stderr, exitCode: code, timedOut, canceled }) - } child.stdout?.on('data', onStdoutData) child.stderr?.on('data', onStderrData) child.on('error', onError) @@ -281,6 +301,19 @@ export class AgentExecHandler { child.off('close', onClose) } + const expireCurrent = (): void => { + timedOut = true + // Why: wrappers and signal-trapping CLIs require terminating the whole tree. + terminateRelaySubprocessTree(child) + finish({ stdout, stderr, exitCode: null, timedOut, canceled }) + } + const remainingTimeoutMs = deadline - Date.now() + if (remainingTimeoutMs <= 0) { + expireCurrent() + return + } + timer = setTimeout(expireCurrent, remainingTimeoutMs) + if (context?.signal) { if (context.signal.aborted) { cancelCurrent() diff --git a/src/relay/agent-exec-shell-environment.test.ts b/src/relay/agent-exec-shell-environment.test.ts new file mode 100644 index 00000000000..a369251635a --- /dev/null +++ b/src/relay/agent-exec-shell-environment.test.ts @@ -0,0 +1,356 @@ +import { execFile, spawn } from 'node:child_process' +import { existsSync } from 'node:fs' +import type * as FileSystem from 'node:fs' +import type * as ChildProcess from 'node:child_process' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { resolveLoginShellEnvironment } from '../main/startup/login-shell-environment' +import { createFakeChild, createHandlers, requestContext } from './agent-exec-handler-test-harness' + +vi.mock('node:child_process', async (importOriginal) => ({ + ...(await importOriginal()), + spawn: vi.fn(), + execFile: vi.fn() +})) +vi.mock('../main/startup/login-shell-environment', () => ({ + resolveLoginShellEnvironment: vi.fn() +})) + +vi.mock('node:fs', async (importOriginal) => { + const original = await importOriginal() + return { ...original, existsSync: vi.fn(original.existsSync) } +}) + +describe('relay headless generation shell environment', () => { + afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() + }) + it('uses the execution host profile PATH and retains explicit command overrides', async () => { + vi.mocked(resolveLoginShellEnvironment).mockResolvedValue({ + PATH: '/profile/bin:/usr/bin', + MODEL: 'profile' + }) + const child = createFakeChild() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: This fake provides the streams and lifecycle used by the relay. + vi.mocked(spawn).mockReturnValue(child as never) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', shell: true, env: { MODEL: 'override' } }, + requestContext() + ) + await vi.waitFor(() => expect(spawn).toHaveBeenCalled()) + child.emit('close', 0) + await expect(pending).resolves.toMatchObject({ exitCode: 0 }) + expect(spawn).toHaveBeenLastCalledWith( + 'opencode', + ['run'], + expect.objectContaining({ + env: expect.objectContaining({ PATH: '/profile/bin:/usr/bin', MODEL: 'override' }) + }) + ) + }) + + it('does not start generation after cancellation during profile resolution', async () => { + vi.mocked(spawn).mockClear() + let resolveProfile: (env: NodeJS.ProcessEnv) => void = () => {} + vi.mocked(resolveLoginShellEnvironment).mockReturnValue( + new Promise((resolve) => { + resolveProfile = resolve + }) + ) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', operation: 'commit-message', shell: true }, + requestContext() + ) + await expect( + handlers.get('agent.cancelExec')?.( + { cwd: '/repo', operation: 'commit-message' }, + requestContext() + ) + ).resolves.toEqual({ canceled: true }) + resolveProfile({ PATH: '/profile/bin' }) + await expect(pending).resolves.toMatchObject({ canceled: true }) + expect(spawn).not.toHaveBeenCalled() + }) +}) + +describe('relay generation deadline includes profile resolution', () => { + afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() + }) + + it('counts both five-second primary and fallback profile probes in the generation budget', async () => { + vi.useFakeTimers() + vi.mocked(resolveLoginShellEnvironment).mockImplementation( + () => + new Promise((resolve) => + setTimeout(() => { + setTimeout(() => resolve({ PATH: '/profile/bin' }), 5_000) + }, 5_000) + ) + ) + const child = createFakeChild() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The existing fake supplies the relay streams and lifecycle. + vi.mocked(spawn).mockReturnValue(child as never) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', shell: true, timeoutMs: 12_000 }, + requestContext() + ) + let result: unknown + void pending?.then((value) => { + result = value + }) + await vi.advanceTimersByTimeAsync(10_000) + expect(spawn).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(1_999) + expect(result).toBeUndefined() + await vi.advanceTimersByTimeAsync(1) + expect(result).toMatchObject({ timedOut: true, exitCode: null }) + child.emit('close', 0) + await pending + }) + + it('settles at the request deadline while the shared profile probe remains pending', async () => { + vi.useFakeTimers() + let resolveProfile: (env: NodeJS.ProcessEnv) => void = () => {} + vi.mocked(resolveLoginShellEnvironment).mockReturnValue( + new Promise((resolve) => { + resolveProfile = resolve + }) + ) + const child = createFakeChild() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The existing fake supplies the relay streams and lifecycle. + vi.mocked(spawn).mockReturnValue(child as never) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', shell: true, timeoutMs: 1_000 }, + requestContext() + ) + let result: unknown + void pending?.then((value) => { + result = value + }) + await vi.advanceTimersByTimeAsync(1_000) + try { + expect(result).toMatchObject({ timedOut: true, exitCode: null }) + expect(spawn).not.toHaveBeenCalled() + } finally { + resolveProfile({ PATH: '/profile/bin' }) + await vi.advanceTimersByTimeAsync(0) + child.emit('close', 0) + await pending + } + expect(spawn).not.toHaveBeenCalled() + }) + + it.each(['cancel', 'abort'] as const)( + 'settles %s immediately while the shared profile probe remains pending', + async (kind) => { + vi.useFakeTimers() + let resolveProfile: (env: NodeJS.ProcessEnv) => void = () => {} + vi.mocked(resolveLoginShellEnvironment).mockReturnValue( + new Promise((resolve) => { + resolveProfile = resolve + }) + ) + const handlers = createHandlers() + const controller = new AbortController() + const pending = handlers.get('agent.execNonInteractive')?.( + { + binary: 'opencode', + args: ['run'], + cwd: '/repo', + operation: 'commit-message', + shell: true + }, + { ...requestContext(), signal: controller.signal } + ) + let result: unknown + void pending?.then((value) => { + result = value + }) + if (kind === 'cancel') { + await handlers.get('agent.cancelExec')?.( + { cwd: '/repo', operation: 'commit-message' }, + requestContext() + ) + } else { + controller.abort() + } + await vi.advanceTimersByTimeAsync(0) + try { + expect(result).toMatchObject({ canceled: true, timedOut: false }) + expect(spawn).not.toHaveBeenCalled() + } finally { + resolveProfile({ PATH: '/profile/bin' }) + await vi.advanceTimersByTimeAsync(0) + await pending + } + } + ) +}) + +describe('relay deadline boundaries and profile failures', () => { + afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() + }) + + it('retains the final millisecond of the request budget without resetting the minimum timeout', async () => { + vi.useFakeTimers() + vi.mocked(resolveLoginShellEnvironment).mockImplementation( + () => new Promise((resolve) => setTimeout(() => resolve({ PATH: '/profile/bin' }), 999)) + ) + const child = createFakeChild() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The existing fake supplies the relay streams and lifecycle. + vi.mocked(spawn).mockReturnValue(child as never) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', shell: true, timeoutMs: 1_000 }, + requestContext() + ) + let result: unknown + void pending?.then((value) => { + result = value + }) + await vi.advanceTimersByTimeAsync(999) + expect(spawn).toHaveBeenCalledTimes(1) + expect(result).toBeUndefined() + await vi.advanceTimersByTimeAsync(1) + expect(result).toMatchObject({ timedOut: true }) + child.emit('close', 0) + await pending + }) + + it('does not spawn when profile settlement consumes the whole request budget', async () => { + vi.useFakeTimers() + vi.mocked(resolveLoginShellEnvironment).mockImplementation( + () => new Promise((resolve) => setTimeout(() => resolve({ PATH: '/profile/bin' }), 1_000)) + ) + const child = createFakeChild() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The existing fake supplies the relay streams and lifecycle. + vi.mocked(spawn).mockReturnValue(child as never) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', shell: true, timeoutMs: 1_000 }, + requestContext() + ) + let result: unknown + void pending?.then((value) => { + result = value + }) + await vi.advanceTimersByTimeAsync(1_000) + try { + expect(result).toMatchObject({ timedOut: true }) + expect(spawn).not.toHaveBeenCalled() + } finally { + child.emit('close', 0) + await pending + } + }) + + it('preserves a profile rejection and removes the pending cancellation lane', async () => { + const failure = new Error('profile resolution refused') + vi.mocked(resolveLoginShellEnvironment).mockRejectedValue(failure) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', operation: 'commit-message', shell: true }, + requestContext() + ) + await expect(pending).rejects.toBe(failure) + expect(spawn).not.toHaveBeenCalled() + await expect( + handlers.get('agent.cancelExec')?.( + { cwd: '/repo', operation: 'commit-message' }, + requestContext() + ) + ).resolves.toEqual({ canceled: false }) + }) +}) + +describe('relay deadline covers synchronous command startup', () => { + afterEach(() => { + vi.useRealTimers() + vi.mocked(existsSync).mockReset() + vi.clearAllMocks() + }) + + it('does not spawn after a Windows PATH lookup exhausts the request budget', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + vi.mocked(resolveLoginShellEnvironment).mockImplementation(() => { + vi.setSystemTime(999) + return Promise.resolve({ PATH: 'C:\\slow-network-bin' }) + }) + vi.mocked(existsSync).mockImplementation(() => { + vi.setSystemTime(1_001) + return false + }) + const child = createFakeChild() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The existing fake supplies the relay streams and lifecycle. + vi.mocked(spawn).mockReturnValue(child as never) + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + const pending = createHandlers().get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: 'C:\\repo', shell: true, timeoutMs: 1_000 }, + requestContext() + ) + await vi.advanceTimersByTimeAsync(0) + try { + expect(existsSync).toHaveBeenCalled() + expect(spawn).not.toHaveBeenCalled() + await expect(pending).resolves.toMatchObject({ timedOut: true, exitCode: null }) + } finally { + child.emit('close', 0) + await pending + } + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('times out immediately when spawning consumes the remaining request budget', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + vi.mocked(existsSync).mockReturnValue(false) + const child = createFakeChild() + vi.mocked(spawn).mockImplementation(() => { + vi.setSystemTime(1_001) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The existing fake supplies the relay streams and lifecycle. + return child as never + }) + const handlers = createHandlers() + const pending = handlers.get('agent.execNonInteractive')?.( + { binary: 'opencode', args: ['run'], cwd: '/repo', timeoutMs: 1_000 }, + requestContext() + ) + let result: unknown + void pending?.then((value) => { + result = value + }) + await vi.advanceTimersByTimeAsync(0) + try { + expect(spawn).toHaveBeenCalledTimes(1) + expect(result).toMatchObject({ timedOut: true, exitCode: null }) + if (process.platform === 'win32') { + expect(execFile).toHaveBeenCalledWith( + 'taskkill', + ['/pid', String(child.pid), '/T', '/F'], + expect.any(Function) + ) + } else { + expect(child.kill).toHaveBeenCalled() + } + await expect( + handlers.get('agent.cancelExec')?.({ cwd: '/repo' }, requestContext()) + ).resolves.toEqual({ canceled: false }) + } finally { + child.emit('close', 0) + await pending + } + }) +}) diff --git a/src/shared/command-option-occurrence.ts b/src/shared/command-option-occurrence.ts new file mode 100644 index 00000000000..6dee668829b --- /dev/null +++ b/src/shared/command-option-occurrence.ts @@ -0,0 +1,38 @@ +function matchesOption(token: string, aliases: readonly string[]): boolean { + return aliases.some( + (alias) => + token === alias || + token.startsWith(`${alias}=`) || + (alias.startsWith('-') && + !alias.startsWith('--') && + token.startsWith(alias) && + token.length > alias.length) + ) +} + +export function findOptionOccurrence( + tokens: string[], + aliases: readonly string[], + stopAtTerminator: boolean +): { index: number; consumed: number; value?: string } | null { + for (let index = 0; index < tokens.length; index += 1) { + const token = tokens[index] + if (stopAtTerminator && token === '--') { + break + } + if (!matchesOption(token, aliases)) { + continue + } + const nextToken = tokens[index + 1] + const consumesNext = + aliases.includes(token) && nextToken !== undefined && !nextToken.startsWith('-') + const alias = aliases.find((name) => matchesOption(token, [name])) + const value = consumesNext + ? nextToken + : alias && token !== alias + ? token.slice(alias.length + (token[alias.length] === '=' ? 1 : 0)) + : undefined + return { index, consumed: consumesNext ? 2 : 1, value } + } + return null +} diff --git a/src/shared/commit-message-agent-spec.test.ts b/src/shared/commit-message-agent-spec.test.ts index 553f734a971..a4e52b604f2 100644 --- a/src/shared/commit-message-agent-spec.test.ts +++ b/src/shared/commit-message-agent-spec.test.ts @@ -550,7 +550,7 @@ describe('buildArgs (OpenCode)', () => { '--agent', 'build', '--format', - 'default' + 'json' ]) expect(args).not.toContain(prompt) expect(args).not.toContain('') @@ -571,7 +571,7 @@ describe('buildArgs (OpenCode)', () => { '--agent', 'build', '--format', - 'default', + 'json', '--variant', 'high' ]) @@ -604,7 +604,7 @@ describe('buildArgs (OpenCode 2)', () => { '--agent', 'build', '--format', - 'default' + 'json' ]) expect(args).not.toContain(prompt) expect(args).not.toContain('') @@ -625,7 +625,7 @@ describe('buildArgs (OpenCode 2)', () => { '--agent', 'build', '--format', - 'default' + 'json' ]) expect(args).not.toContain('--variant') }) diff --git a/src/shared/commit-message-agent-specs-primary.ts b/src/shared/commit-message-agent-specs-primary.ts index 729228824bf..ce7b35afcfb 100644 --- a/src/shared/commit-message-agent-specs-primary.ts +++ b/src/shared/commit-message-agent-specs-primary.ts @@ -161,32 +161,25 @@ export function buildPrimaryCommitMessageAgentSpecs({ promptDelivery: 'stdin', buildArgs: ({ model, thinkingLevel }) => [ 'run', - '--model', - model, + ...(model && model !== 'default' ? ['--model', model] : []), '--agent', 'build', '--format', - 'default', + 'json', ...(thinkingLevel ? ['--variant', thinkingLevel] : []) ], singletonOptions: [['--model', '-m'], ['--agent'], ['--format'], ['--variant']], modelSource: 'dynamic', modelDiscovery: { binary: 'opencode', args: ['models'], parse: parseLineModels }, models: [ - { - // Why: OpenCode's hosted GPT models can require workspace billing even - // when `opencode models` lists them. This free model is available in - // discovery and works as a usable out-of-the-box default. - id: 'opencode/deepseek-v4-flash-free', - label: 'OpenCode DeepSeek V4 Flash Free' - }, + { id: 'default', label: 'Config default' }, { id: 'opencode/gpt-5.4-mini', label: 'OpenCode GPT 5.4 Mini', ...withOpenAiThinking('gpt-5.4-mini') } ], - defaultModelId: 'opencode/deepseek-v4-flash-free' + defaultModelId: 'default' }, opencode2: { id: 'opencode2', @@ -195,25 +188,26 @@ export function buildPrimaryCommitMessageAgentSpecs({ promptDelivery: 'stdin', buildArgs: ({ model, thinkingLevel }) => [ 'run', - '--model', - thinkingLevel ? `${model}#${thinkingLevel}` : model, + ...(model && model !== 'default' + ? ['--model', thinkingLevel ? `${model}#${thinkingLevel}` : model] + : []), '--agent', 'build', '--format', - 'default' + 'json' ], singletonOptions: [['--model', '-m'], ['--agent'], ['--format']], modelSource: 'dynamic', modelDiscovery: { binary: 'opencode2', args: ['models'], parse: parseLineModels }, models: [ - { id: 'opencode/deepseek-v4-flash-free', label: 'OpenCode DeepSeek V4 Flash Free' }, + { id: 'default', label: 'Config default' }, { id: 'opencode/gpt-5.4-mini', label: 'OpenCode GPT 5.4 Mini', ...withOpenAiThinking('gpt-5.4-mini') } ], - defaultModelId: 'opencode/deepseek-v4-flash-free' + defaultModelId: 'default' }, pi: { id: 'pi', diff --git a/src/shared/commit-message-plan.test.ts b/src/shared/commit-message-plan.test.ts index 9e499ae4a95..435c4b30c3d 100644 --- a/src/shared/commit-message-plan.test.ts +++ b/src/shared/commit-message-plan.test.ts @@ -77,12 +77,13 @@ describe('planCommitMessageGeneration', () => { '--agent', 'build', '--format', - 'default', + 'json', '--variant', 'high' ], stdinPayload: 'PROMPT', - label: 'OpenCode' + label: 'OpenCode', + outputFormat: 'opencode-json' } }) }) @@ -109,10 +110,11 @@ describe('planCommitMessageGeneration', () => { '--agent', 'build', '--format', - 'default' + 'json' ], stdinPayload: 'PROMPT', - label: 'OpenCode' + label: 'OpenCode', + outputFormat: 'opencode-json' } }) }) @@ -481,7 +483,7 @@ describe('planCommitMessageGeneration', () => { expect(result).toMatchObject({ ok: true, plan: { - args: ['run', '--model', 'opencode/gpt-5.5', '--agent', 'build', '--format', 'default'], + args: ['run', '--model', 'opencode/gpt-5.5', '--agent', 'build', '--format', 'json'], stdinPayload: 'PROMPT' } }) @@ -496,7 +498,7 @@ describe('planCommitMessageGeneration', () => { expect(result).toMatchObject({ ok: true, plan: { - args: ['run', '-m', 'opencode/gpt-5.5', '--agent', 'build', '--format', 'default'] + args: ['run', '-m', 'opencode/gpt-5.5', '--agent', 'build', '--format', 'json'] } }) }) @@ -546,7 +548,7 @@ describe('planCommitMessageGeneration', () => { '--agent', 'build', '--format', - 'default', + 'json', '--share' ], stdinPayload: 'PROMPT' @@ -567,7 +569,7 @@ describe('planCommitMessageGeneration', () => { expect(result).toMatchObject({ ok: true, plan: { - args: ['run', '--model', 'opencode/first', '--agent', 'build', '--format', 'default'] + args: ['run', '--model', 'opencode/first', '--agent', 'build', '--format', 'json'] } }) }) @@ -610,7 +612,7 @@ describe('planCommitMessageGeneration', () => { '--agent', 'build', '--format', - 'default' + 'json' ] } }) @@ -641,7 +643,7 @@ describe('planCommitMessageGeneration', () => { '--agent', 'build', '--format', - 'default' + 'json' ] } }) @@ -662,7 +664,7 @@ describe('planCommitMessageGeneration', () => { ok: true, plan: { binary: 'opencode', - args: ['run', '--model', 'opencode/from-recipe', '--agent', 'build', '--format', 'default'] + args: ['run', '--model', 'opencode/from-recipe', '--agent', 'build', '--format', 'json'] } }) }) @@ -785,3 +787,57 @@ describe('backslash mode reaches every command the user can type (#11375)', () = expect(plan.ok && plan.plan.args).toContain('/my dir') }) }) + +describe('OpenCode format metadata respects option terminators', () => { + it.each(['opencode', 'opencode2'] as const)( + 'ignores literal recipe format values for %s', + (agentId) => { + const result = planCommitMessageGeneration( + { + agentId, + model: 'opencode/gpt-5.4-mini', + agentArgs: '--format default -- --format json' + }, + 'PROMPT' + ) + expect(result.ok).toBe(true) + if (!result.ok) { + throw new Error(result.error) + } + expect(result.plan.args).toContain('--') + expect(result.plan.outputFormat).toBeUndefined() + } + ) + + it('ignores literal equals-form flags after a command override terminator', () => { + const result = planCommitMessageGeneration( + { + agentId: 'opencode', + model: 'opencode/gpt-5.4-mini', + agentCommandOverride: 'opencode --format default -- --format=json' + }, + 'PROMPT' + ) + expect(result.ok).toBe(true) + if (!result.ok) { + throw new Error(result.error) + } + expect(result.plan.outputFormat).toBeUndefined() + }) + + it('retains JSON metadata for the active option before a literal default value', () => { + const result = planCommitMessageGeneration( + { + agentId: 'opencode', + model: 'opencode/gpt-5.4-mini', + agentArgs: '--format=json -- --format default' + }, + 'PROMPT' + ) + expect(result.ok).toBe(true) + if (!result.ok) { + throw new Error(result.error) + } + expect(result.plan.outputFormat).toBe('opencode-json') + }) +}) diff --git a/src/shared/commit-message-plan.ts b/src/shared/commit-message-plan.ts index 1d0bf1e02de..8fe541c6028 100644 --- a/src/shared/commit-message-plan.ts +++ b/src/shared/commit-message-plan.ts @@ -1,3 +1,4 @@ +import { findOptionOccurrence } from './command-option-occurrence' import { planAgentBinary } from './agent-command-plan' export { planAgentBinary } from './agent-command-plan' import type { CommandTemplateBackslash } from './commit-message-prompt' @@ -8,6 +9,7 @@ import { } from './commit-message-agent-spec' import { planCustomCommand, tokenizeCustomCommandTemplate } from './commit-message-prompt' import type { TuiAgent } from './tui-agent' +import { mergeOpenCodeGenerationArgs } from './opencode-generation-command' // Why: planning is a pure transformation from "user request + prompt text" // into "spawn-ready binary + argv". Keeping it in shared lets both the local @@ -37,6 +39,7 @@ export type CommitMessagePlan = { label: string /** Leading command assignments, applied on the execution host. */ env?: Record + outputFormat?: 'opencode-json' } export type CommitMessagePlanResult = @@ -60,39 +63,6 @@ function planAdditionalAgentArgs( const DEFAULT_SINGLETON_OPTIONS: readonly (readonly string[])[] = [['--model']] -function matchesOption(token: string, aliases: readonly string[]): boolean { - return aliases.some( - (alias) => - token === alias || - token.startsWith(`${alias}=`) || - (alias.startsWith('-') && - !alias.startsWith('--') && - token.startsWith(alias) && - token.length > alias.length) - ) -} - -function findOptionOccurrence( - tokens: string[], - aliases: readonly string[], - stopAtTerminator: boolean -): { index: number; consumed: number } | null { - for (let index = 0; index < tokens.length; index += 1) { - const token = tokens[index] - if (stopAtTerminator && token === '--') { - break - } - if (!matchesOption(token, aliases)) { - continue - } - const nextToken = tokens[index + 1] - const consumesNext = - aliases.includes(token) && nextToken !== undefined && !nextToken.startsWith('-') - return { index, consumed: consumesNext ? 2 : 1 } - } - return null -} - function applyRecipeOptionOverride(args: { generatedArgs: string[] recipeArgs: string[] @@ -307,13 +277,24 @@ export function planCommitMessageGeneration( promptDelivery: spec.promptDelivery, prompt: argvPrompt }) + const generationArgs = mergeOpenCodeGenerationArgs( + input.agentId, + command.binary, + merged.prefixArgs, + args + ) + const formatOption = + input.agentId === 'opencode' || input.agentId === 'opencode2' + ? findOptionOccurrence(generationArgs, ['--format'], true) + : null return { ok: true, plan: { binary: command.binary, - args: [...merged.prefixArgs, ...args], + args: generationArgs, stdinPayload: spec.promptDelivery === 'stdin' ? prompt : null, label: spec.label, + ...(formatOption?.value === 'json' ? { outputFormat: 'opencode-json' as const } : {}), ...(command.env ? { env: command.env } : {}) } } diff --git a/src/shared/opencode-generation-command.test.ts b/src/shared/opencode-generation-command.test.ts new file mode 100644 index 00000000000..945300a1c12 --- /dev/null +++ b/src/shared/opencode-generation-command.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from 'vitest' +import { planCommitMessageGeneration } from './commit-message-plan' +import { openCodeVariantRetryPlan } from './opencode-generation-command' + +const rejection = 'Unrecognized flag: --variant in command opencode run' +describe('OpenCode generation commands', () => { + it.each(['opencode', 'opencode.exe', 'opencode.cmd', 'opencode2'])( + 'keeps run before launch flags for %s', + (binary) => { + const result = planCommitMessageGeneration( + { agentId: 'opencode', model: 'default', agentCommandOverride: `${binary} --auto` }, + 'PROMPT' + ) + expect(result).toMatchObject({ + ok: true, + plan: { + args: ['run', '--auto', '--agent', 'build', '--format', 'json'], + stdinPayload: 'PROMPT', + outputFormat: 'opencode-json' + } + }) + } + ) + + it('keeps explicit formatted output recipes on their requested output format', () => { + const result = planCommitMessageGeneration( + { agentId: 'opencode', model: 'default', agentArgs: '--format default' }, + 'PROMPT' + ) + if (!result.ok) { + throw new Error(result.error) + } + expect(result.plan.outputFormat).toBeUndefined() + }) + + it('retries only the precise v2 argv rejection, retaining the prompt and selected model', () => { + const result = planCommitMessageGeneration( + { agentId: 'opencode', model: 'fixture/chat', thinkingLevel: 'high' }, + 'PROMPT' + ) + if (!result.ok) { + throw new Error(result.error) + } + expect(openCodeVariantRetryPlan(result.plan, rejection)).toMatchObject({ + args: ['run', '--model', 'fixture/chat#high', '--agent', 'build', '--format', 'json'], + stdinPayload: 'PROMPT' + }) + expect(openCodeVariantRetryPlan(result.plan, 'provider rejected the request')).toBeNull() + expect(openCodeVariantRetryPlan(result.plan, 'Unrecognized flag: --other')).toBeNull() + }) + it.each([ + '--model=fixture/chat --variant=high', + '-mfixture/chat --variant=high', + '--variant high --model=fixture/chat', + '--variant=high -m fixture/chat' + ])('retries the chosen recipe model and variant in %s', (agentArgs) => { + const result = planCommitMessageGeneration( + { agentId: 'opencode', model: 'other/model', thinkingLevel: 'low', agentArgs }, + 'PROMPT' + ) + if (!result.ok) { + throw new Error(result.error) + } + const retry = openCodeVariantRetryPlan(result.plan, rejection) + expect(retry?.args.join(' ')).toContain('fixture/chat#high') + expect(retry?.args.join(' ')).not.toContain('--variant') + expect(retry?.stdinPayload).toBe('PROMPT') + }) + + it('does not treat prompt tokens after the terminator as options', () => { + expect( + openCodeVariantRetryPlan( + { + binary: 'opencode', + label: 'OpenCode', + stdinPayload: null, + args: ['run', '--model=fixture/chat', '--', '--variant=high'] + }, + rejection + ) + ).toBeNull() + expect( + openCodeVariantRetryPlan( + { + binary: 'opencode', + label: 'OpenCode', + stdinPayload: null, + args: ['run', '--variant=high', '--', '--model=fixture/chat'] + }, + rejection + ) + ).toBeNull() + }) +}) diff --git a/src/shared/opencode-generation-command.ts b/src/shared/opencode-generation-command.ts new file mode 100644 index 00000000000..524cbe5bdbe --- /dev/null +++ b/src/shared/opencode-generation-command.ts @@ -0,0 +1,44 @@ +import { findOptionOccurrence } from './command-option-occurrence' +import type { CommitMessagePlan } from './commit-message-plan' + +export function mergeOpenCodeGenerationArgs( + agentId: string, + binary: string, + prefixArgs: string[], + args: string[] +): string[] { + if ( + (agentId === 'opencode' || agentId === 'opencode2') && + /(?:^|[\\/])opencode2?(?:\.(?:cmd|exe))?$/i.test(binary) && + args[0] === 'run' && + !prefixArgs.includes('--') + ) { + return ['run', ...prefixArgs, ...args.slice(1)] + } + return [...prefixArgs, ...args] +} + +export function openCodeVariantRetryPlan( + plan: CommitMessagePlan, + stderr: string +): CommitMessagePlan | null { + // Retry only an argv rejection, which happens before a model turn starts. + if (!stderr.includes('Unrecognized flag: --variant in command opencode run')) { + return null + } + const variant = findOptionOccurrence(plan.args, ['--variant'], true) + const model = findOptionOccurrence(plan.args, ['--model', '-m'], true) + if (!variant?.value || !model?.value) { + return null + } + const args = [...plan.args] + const replacement = `${model.value.split('#')[0]}#${variant.value}` + if (model.consumed === 2) { + args[model.index + 1] = replacement + } else { + const token = args[model.index] + args[model.index] = `${token.slice(0, token.length - model.value.length)}${replacement}` + } + args.splice(variant.index, variant.consumed) + return { ...plan, args } +} diff --git a/src/shared/opencode-generation-output.test.ts b/src/shared/opencode-generation-output.test.ts new file mode 100644 index 00000000000..1d0e8c4d9ce --- /dev/null +++ b/src/shared/opencode-generation-output.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from 'vitest' +import { parseOpenCodeGenerationOutput } from './opencode-generation-output' + +const frame = (type: string, text: string, id = 'answer'): string => + JSON.stringify({ type, part: { id, text } }) + +describe('OpenCode generation event output', () => { + it('uses the final answer after a tool step', () => { + const output = [ + frame('step_start', ''), + frame('text', 'I will inspect the staged diff.'), + frame('tool_use', 'git diff'), + frame('step_finish', 'tool-calls'), + frame('step_start', ''), + frame('text', 'fix: parse the final answer') + ].join('\n') + expect(parseOpenCodeGenerationOutput(output)).toEqual({ + ok: true, + text: 'fix: parse the final answer' + }) + }) + it('keeps assistant text and ignores tool, reasoning, progress and warning events', () => { + const output = [ + frame('step_start', 'starting'), + frame('reasoning', 'reasoning'), + frame('tool_use', 'git diff output'), + JSON.stringify({ type: 'warning', message: 'notice' }), + frame('text', 'fix: generate messages'), + frame('step_finish', 'done') + ].join('\n') + expect(parseOpenCodeGenerationOutput(output)).toEqual({ + ok: true, + text: 'fix: generate messages' + }) + }) + + it('replaces repeated parts and preserves multiple answer parts', () => { + expect( + parseOpenCodeGenerationOutput( + [frame('text', 'partial'), frame('text', 'subject'), frame('text', 'body', 'body')].join( + '\r\n' + ) + ) + ).toEqual({ ok: true, text: 'subject\nbody' }) + }) + + it.each([ + { name: 'UnknownError', data: { message: 'Model unavailable' } }, + { type: 'provider.no-route', message: 'Unsupported package' } + ])('reports errors instead of accepting a partial answer', (error) => { + const output = `${frame('text', 'partial')}\n${JSON.stringify({ type: 'error', error })}` + expect(parseOpenCodeGenerationOutput(output)).toEqual({ + ok: false, + error: error.message ?? error.data?.message + }) + }) + + it.each([ + { error: { name: 'MessageOutputLengthError', data: {} }, expected: 'MessageOutputLengthError' }, + { + error: { name: 'ProviderError', data: { retryable: false } }, + expected: 'ProviderError' + }, + { + error: { name: 'ProviderError', message: 'Provider rejected the request', data: {} }, + expected: 'Provider rejected the request' + } + ])('reports named errors without a data message', ({ error, expected }) => { + const output = `${frame('text', 'partial')}\n${JSON.stringify({ type: 'error', error })}` + expect(parseOpenCodeGenerationOutput(output)).toEqual({ ok: false, error: expected }) + }) + + it.each([ + '{broken', + '{"title":"not an event"}', + 'null', + JSON.stringify({ type: 'error', error: { name: 'ProviderError', data: { message: 42 } } }) + ])('rejects malformed events %s', (output) => { + expect(parseOpenCodeGenerationOutput(output)).toEqual({ + ok: false, + error: 'OpenCode returned invalid JSON events.' + }) + }) +}) diff --git a/src/shared/opencode-generation-output.ts b/src/shared/opencode-generation-output.ts new file mode 100644 index 00000000000..8a3ba24e494 --- /dev/null +++ b/src/shared/opencode-generation-output.ts @@ -0,0 +1,53 @@ +import { z } from 'zod' + +const eventSchema = z.object({ + type: z.string(), + part: z.object({ id: z.string().optional(), text: z.string().optional() }).optional(), + error: z + .object({ + name: z.string().optional(), + message: z.string().optional(), + data: z.object({ message: z.string().optional() }).optional() + }) + .optional() +}) + +export function parseOpenCodeGenerationOutput( + stdout: string +): { ok: true; text: string } | { ok: false; error: string } { + const parts = new Map() + let anonymousPart = 0 + for (const line of stdout.split(/\r?\n/)) { + if (!line.trim()) { + continue + } + let value: unknown + try { + value = JSON.parse(line) + } catch { + return { ok: false, error: 'OpenCode returned invalid JSON events.' } + } + const parsed = eventSchema.safeParse(value) + if (!parsed.success) { + return { ok: false, error: 'OpenCode returned invalid JSON events.' } + } + const event = parsed.data + if (event.type === 'step_start') { + parts.clear() + } + if (event.type === 'error') { + return { + ok: false, + error: + event.error?.data?.message ?? + event.error?.message ?? + event.error?.name ?? + 'OpenCode reported an error.' + } + } + if (event.type === 'text' && event.part?.text !== undefined) { + parts.set(event.part.id ?? `anonymous-${anonymousPart++}`, event.part.text) + } + } + return { ok: true, text: [...parts.values()].join('\n') } +}