Repository navigation
feat(omp): open OMP in the structured chat over ACP #26401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import type { ToolCallUpdate } from '../generated/acp-protocol.generated' | ||
| import { OMP_ACP_DIALECT } from './omp-dialect' | ||
|
|
||
| const text = (value: string) => ({ | ||
| type: 'content' as const, | ||
| content: { type: 'text' as const, text: value } | ||
| }) | ||
| const normalize = (update: ToolCallUpdate) => OMP_ACP_DIALECT.normalizeToolUpdate!(update) | ||
|
|
||
| describe('OMP tool updates', () => { | ||
| it('leaves a tool without a command echo as it came', () => { | ||
| const update: ToolCallUpdate = { | ||
| toolCallId: 'call-1', | ||
| status: 'completed', | ||
| rawOutput: { content: [{ type: 'text', text: 'file body' }], details: {} }, | ||
| content: [text('file body')] | ||
| } | ||
| expect(normalize(update)).toBe(update) | ||
| }) | ||
|
|
||
| it('keeps a result that itself starts with "$ "', () => { | ||
| const update: ToolCallUpdate = { | ||
| toolCallId: 'call-1', | ||
| status: 'completed', | ||
| rawOutput: { content: [{ type: 'text', text: '$ 5.00' }], details: {} }, | ||
| content: [text('$ 5.00')] | ||
| } | ||
| expect(normalize(update)).toBe(update) | ||
| }) | ||
|
|
||
| it('keeps output that only looks like a notice when OMP sent no matching detail', () => { | ||
| const update = normalize({ | ||
| toolCallId: 'call-1', | ||
| status: 'completed', | ||
| rawOutput: { | ||
| content: [{ type: 'text', text: 'done\n\nCommand exited with code 3' }], | ||
| details: {} | ||
| }, | ||
| content: [text('$ ./run'), text('done\n\nCommand exited with code 3')] | ||
| }) | ||
| expect(update).toMatchObject({ | ||
| content: [], | ||
| rawOutput: { stdout: 'done\n\nCommand exited with code 3' } | ||
| }) | ||
| expect(update.rawOutput).not.toHaveProperty('exitCode') | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { z } from 'zod' | ||
| import type { ToolCallContent, ToolCallUpdate } from '../generated/acp-protocol.generated' | ||
| import type { AcpDialect } from './acp-dialect' | ||
|
|
||
| // OMP sends a tool's result as `rawOutput: {content: [{type: 'text', text}], details}`, with a | ||
| // command's non-zero exit at `details.exitCode`, and repeats the text as content behind a | ||
| // `$ <command>` echo. The shared reader takes content text first, so that echo became the output. | ||
| const textBlockSchema = z.looseObject({ type: z.literal('text'), text: z.string() }) | ||
| const toolResultSchema = z.looseObject({ | ||
| content: z.array(z.unknown()), | ||
| details: z | ||
| .looseObject({ | ||
| exitCode: z.number().int().safe().optional(), | ||
| wallTimeMs: z.number().optional() | ||
| }) | ||
| .optional() | ||
| }) | ||
|
|
||
| function contentText(block: ToolCallContent | undefined): string | undefined { | ||
| return block?.type === 'content' && block.content.type === 'text' ? block.content.text : undefined | ||
| } | ||
|
|
||
| /** OMP appends these notice lines to a command's output; the row shows exit and timing itself. */ | ||
| function commandOutput(text: string, exitCode: number | undefined, timed: boolean): string { | ||
| let output = text | ||
| const exitNotice = `\n\nCommand exited with code ${exitCode}` | ||
| if (exitCode !== undefined && output.endsWith(exitNotice)) { | ||
| output = output.slice(0, -exitNotice.length) | ||
| } | ||
| return timed ? output.replace(/\n\nWall time: \d+(?:\.\d+)? seconds$/u, '') : output | ||
| } | ||
|
|
||
| function normalizeToolUpdate(update: ToolCallUpdate): ToolCallUpdate { | ||
| const echo = contentText(update.content?.[0]) | ||
| if (!echo?.startsWith('$ ')) { | ||
| return update | ||
| } | ||
| const parsed = toolResultSchema.safeParse(update.rawOutput) | ||
| const texts = parsed.success | ||
| ? parsed.data.content.flatMap((block) => textBlockSchema.safeParse(block).data?.text ?? []) | ||
| : [] | ||
| if (texts.includes(echo)) { | ||
| return update | ||
| } | ||
| // The result's own text moves to `stdout`; content keeps anything else it carried. | ||
| const content = (update.content ?? []).slice(1).filter((block) => { | ||
| const text = contentText(block) | ||
| return text === undefined || !texts.includes(text) | ||
| }) | ||
| if (!parsed.success) { | ||
| return { ...update, content } | ||
| } | ||
| const exitCode = parsed.data.details?.exitCode | ||
| const stdout = commandOutput( | ||
| texts.join('\n'), | ||
| exitCode, | ||
| parsed.data.details?.wallTimeMs !== undefined | ||
| ) | ||
| return { | ||
| ...update, | ||
| content, | ||
| rawOutput: { ...parsed.data, stdout, ...(exitCode === undefined ? {} : { exitCode }) } | ||
| } | ||
| } | ||
|
|
||
| export const OMP_ACP_DIALECT: AcpDialect = { normalizeToolUpdate } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,14 @@ import { | |
| import { AGENT_HOOK_RUNTIME_ENV_KEYS } from '../ipc/pty/host-env/spawn-env-keys' | ||
| import type { AcpDialect } from './acp-dialects/acp-dialect' | ||
| import { GROK_ACP_DIALECT } from './acp-dialects/grok-dialect' | ||
| import { OMP_ACP_DIALECT } from './acp-dialects/omp-dialect' | ||
| import { OPENCODE_ACP_DIALECT } from './acp-dialects/opencode-dialect' | ||
| import { directoryAccountBinding, type AcpAccountBinding } from './acp-account-binding' | ||
| import { openCodeAcpAccountBinding } from '../opencode/opencode-structured-account-home' | ||
| import { scrubOpenCodeAcpEnvironment } from '../opencode/opencode-acp-environment' | ||
| import { openCodeStoredUserMessagesReader } from '../opencode/opencode-acp-stored-messages' | ||
| import type { AcpStoredUserMessagesReader } from './acp-recovery-history' | ||
| import { isStableCliVersionOnLine } from '../agent-cli-version-probe' | ||
| import { isStableCliVersionFrom, isStableCliVersionOnLine } from '../agent-cli-version-probe' | ||
| import type { TuiAgent } from '../../shared/tui-agent' | ||
|
|
||
| export type AcpLaunchSpec = { | ||
|
|
@@ -92,7 +93,31 @@ const OPENCODE_LAUNCH_SPEC: AcpLaunchSpec = { | |
| readStoredUserMessages: openCodeStoredUserMessagesReader() | ||
| } | ||
|
|
||
| export const ACP_LAUNCH_SPECS: readonly AcpLaunchSpec[] = [GROK_LAUNCH_SPEC, OPENCODE_LAUNCH_SPEC] | ||
| // OMP serves ACP through `omp acp`; its environment reaches it as the user set it. | ||
| const OMP_LAUNCH_SPEC: AcpLaunchSpec = { | ||
| agent: 'omp', | ||
| command: 'omp', | ||
| // `omp acp` takes no flags: full access answers each permission request yes. | ||
| args: () => ['acp'], | ||
| env: {}, | ||
| dialect: OMP_ACP_DIALECT, | ||
| // OMP signs in from its own `/login`. | ||
| loginCommand: ['omp'], | ||
| // The directory OMP's terminal chats read too; its default is OMP's own. | ||
| account: directoryAccountBinding('PI_CODING_AGENT_DIR', (homePath) => | ||
| join(homePath, '.omp', 'agent') | ||
| ), | ||
| // OMP's installers use directories the shared resolver already searches after PATH. | ||
| installDirectories: () => [], | ||
| // Stable releases from 17.0.5, the release verified to serve `omp acp`. | ||
| supportsVersion: (version) => isStableCliVersionFrom(version, '17.0.5') | ||
|
Comment on lines
+112
to
+113
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Do not select ACP when OMP cannot honor its approval mode. The version gate admits an OMP installation configured with |
||
| } | ||
|
|
||
| export const ACP_LAUNCH_SPECS: readonly AcpLaunchSpec[] = [ | ||
| GROK_LAUNCH_SPEC, | ||
| OPENCODE_LAUNCH_SPEC, | ||
| OMP_LAUNCH_SPEC | ||
| ] | ||
|
|
||
| export function acpLaunchSpecFor(agent: string): AcpLaunchSpec | null { | ||
| return ACP_LAUNCH_SPECS.find((spec) => spec.agent === agent) ?? null | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import type { AgentSessionRecord } from '../../shared/agent-session-record' | ||
| import { agentSessionRecordFixture } from '../../shared/agent-session-record.test-fixture' | ||
| import { createProviderSpawnSpec } from '../provider-process/provider-process-supervisor' | ||
| import { ACP_CHILD_ENV_TO_DELETE, acpLaunchSpecFor } from './acp-launch-specs' | ||
| import { | ||
| createAcpStructuredLaunchResolver, | ||
| type AcpStructuredLaunchResolverDeps | ||
| } from './acp-structured-launch-resolution' | ||
|
|
||
| const OMP = acpLaunchSpecFor('omp')! | ||
| const identity = { | ||
| sessionId: 'session-alpha-1', | ||
| workspaceId: 'workspace-1', | ||
| hostId: 'local', | ||
| agent: 'omp', | ||
| providerHandle: null | ||
| } | ||
|
|
||
| function ompRecord( | ||
| accountHome: AgentSessionRecord['accountHome'] = { | ||
| variable: 'PI_CODING_AGENT_DIR', | ||
| path: '/home/user/.omp/agent' | ||
| } | ||
| ): AgentSessionRecord { | ||
| return { ...agentSessionRecordFixture(), provider: 'omp', providerHandleChain: [], accountHome } | ||
| } | ||
|
|
||
| function resolver( | ||
| record: AgentSessionRecord, | ||
| options: { | ||
| launchEnv?: Record<string, string> | ||
| fullAccess?: boolean | ||
| probeVersion?: AcpStructuredLaunchResolverDeps['probeVersion'] | ||
| } = {} | ||
| ) { | ||
| return createAcpStructuredLaunchResolver(OMP, { | ||
| store: { getRecord: () => record }, | ||
| readJournal: () => null, | ||
| resolveWorkspacePath: async () => '/repo/worktree', | ||
| resolveEnvironment: async () => ({ PATH: '/usr/bin', HOME: '/home/user' }), | ||
| resolveLaunchEnv: () => options.launchEnv ?? {}, | ||
| resolveFullAccess: () => options.fullAccess ?? false, | ||
| resolveCommand: (command) => `/resolved/${command}`, | ||
| inheritedEnv: {}, | ||
| probeVersion: options.probeVersion ?? (async (_input, supports) => supports('17.0.5')) | ||
| }) | ||
| } | ||
|
|
||
| describe('OMP ACP launch resolution', () => { | ||
| it('runs `omp acp` pointed at the agent directory the chat pinned, with or without full access', async () => { | ||
| for (const fullAccess of [false, true]) { | ||
| const launch = await resolver(ompRecord(), { fullAccess })({ identity }) | ||
| expect(launch).toMatchObject({ command: '/resolved/omp', args: ['acp'], fullAccess }) | ||
| expect(launch.env.PI_CODING_AGENT_DIR).toBe('/home/user/.omp/agent') | ||
| } | ||
| }) | ||
|
|
||
| it("passes the user's own OMP environment through unchanged", async () => { | ||
| const userEnv = { | ||
| OMP_PROFILE: 'work', | ||
| PI_CONFIG_DIR: 'relative/config', | ||
| PI_SMOL_MODEL: 'fast', | ||
| ANTHROPIC_API_KEY: 'sk-test' | ||
| } | ||
| const launch = await resolver(ompRecord(), { launchEnv: userEnv })({ identity }) | ||
| expect(launch.envToDelete).toEqual([]) | ||
| const child = createProviderSpawnSpec( | ||
| { | ||
| command: launch.command, | ||
| args: launch.args, | ||
| env: launch.env, | ||
| envToDelete: [...ACP_CHILD_ENV_TO_DELETE, ...launch.envToDelete] | ||
| }, | ||
| { ORCA_PANE_KEY: 'tab-1:pane-1' }, | ||
| 'darwin' | ||
| ).env | ||
| expect(child).toMatchObject(userEnv) | ||
| // A pane's identity would let OMP's Orca status extension report for this chat too. | ||
| expect(child.ORCA_PANE_KEY).toBeUndefined() | ||
| }) | ||
|
|
||
| it('keeps a chat on the directory it pinned when the launch env names another', async () => { | ||
| const launch = await resolver(ompRecord(), { | ||
| launchEnv: { PI_CODING_AGENT_DIR: '/data/other-agent' } | ||
| })({ identity }) | ||
| expect(launch.env.PI_CODING_AGENT_DIR).toBe('/home/user/.omp/agent') | ||
| }) | ||
|
|
||
| it("refuses a record that pins another agent's directory", async () => { | ||
| await expect( | ||
| resolver(ompRecord({ variable: 'GROK_HOME', path: '/home/user/.grok' }))({ identity }) | ||
| ).rejects.toThrow(/pin PI_CODING_AGENT_DIR/) | ||
| }) | ||
|
|
||
| it.each(['17.0.5', '17.2.12', '18.4.5'])('admits OMP %s', async (version) => { | ||
| const launch = resolver(ompRecord(), { | ||
| probeVersion: async (_input, supports) => supports(version) | ||
| }) | ||
| await expect(launch({ identity })).resolves.toMatchObject({ args: ['acp'] }) | ||
| }) | ||
|
|
||
| it.each(['17.0.4', '16.9.0', '18.0.0-beta.1'])( | ||
| 'refuses OMP %s before spawning it, as a host that cannot run the chat', | ||
| async (version) => { | ||
| const launch = resolver(ompRecord(), { | ||
| probeVersion: async (_input, supports) => supports(version) | ||
| }) | ||
| await expect(launch({ identity })).rejects.toMatchObject({ | ||
| refusal: { | ||
| code: 'structured_agent_session_unsupported', | ||
| details: { reason: 'hostUnsupported' } | ||
| } | ||
| }) | ||
| } | ||
| ) | ||
|
|
||
| it('refuses when the version cannot be read', async () => { | ||
| const launch = resolver(ompRecord(), { probeVersion: async () => false }) | ||
| await expect(launch({ identity })).rejects.toMatchObject({ | ||
| refusal: { code: 'structured_agent_session_unsupported' } | ||
| }) | ||
| }) | ||
|
|
||
| it('asks the version of exactly the binary, folder and environment it spawns', async () => { | ||
| const asked: Parameters<NonNullable<AcpStructuredLaunchResolverDeps['probeVersion']>>[0][] = [] | ||
| const launch = await resolver(ompRecord(), { | ||
| probeVersion: async (input, supports) => (asked.push(input), supports('17.0.5')) | ||
| })({ identity }) | ||
| expect(asked).toEqual([{ program: launch.command, cwd: launch.cwd, env: launch.env }]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Pin the directory OMP actually uses.
When
PI_CONFIG_DIRchanges the default config root, this binding still setsPI_CODING_AGENT_DIRto~/.omp/agent. OMP then starts with a different agent directory from the one the user configured. Named profiles introduce a separate persistence failure: OMP ignoresPI_CODING_AGENT_DIRfor them, but this binding records the default directory. If the user changesOMP_PROFILEbefore reopening a chat, the launch can select another profile instead of the chat's original profile. Resolve the effective directory from OMP's environment and persist the profile selection needed to restore it. (github.com)