diff --git a/.github/actions/bun-install/action.yml b/.github/actions/bun-install/action.yml index 5ae10831d..13ef90c9d 100644 --- a/.github/actions/bun-install/action.yml +++ b/.github/actions/bun-install/action.yml @@ -58,4 +58,3 @@ runs: - name: Install dependencies shell: bash run: bun install --frozen-lockfile - diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97bd998ea..e49ec3059 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -337,6 +337,7 @@ jobs: timeout-minutes: 25 steps: - uses: actions/checkout@v4 + - uses: ./.github/actions/setup-system-deps - uses: ./.github/actions/bun-install - name: Resolve Linux x64 native artifact run @@ -367,6 +368,7 @@ jobs: timeout-minutes: 25 steps: - uses: actions/checkout@v4 + - uses: ./.github/actions/setup-system-deps - uses: ./.github/actions/bun-install - name: Resolve Linux x64 native artifact run @@ -429,6 +431,7 @@ jobs: timeout-minutes: 25 steps: - uses: actions/checkout@v4 + - uses: ./.github/actions/setup-system-deps - uses: ./.github/actions/bun-install - name: Resolve Linux x64 native artifact run @@ -459,6 +462,7 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@v4 + - uses: ./.github/actions/setup-system-deps - uses: ./.github/actions/bun-install - name: Resolve Linux x64 native artifact run @@ -577,6 +581,9 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 with: bun-version: "1.3" + env: + SCCACHE_BUCKET: "" + AWS_ACCESS_KEY_ID: "" - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: node-version: "24" diff --git a/docs/config-usage.md b/docs/config-usage.md index 0db2adab0..e4269b695 100644 --- a/docs/config-usage.md +++ b/docs/config-usage.md @@ -72,6 +72,14 @@ Project-level bases: `CONFIG_DIR_NAME` is `.omp` (`packages/utils/src/dirs.ts`). +## Profiles + +A named profile (`omp --profile `, the `--alias` shortcut, or `OMP_PROFILE` / `PI_PROFILE`) relocates the OMP user base. When a profile is active, every OMP-native user-level path written here as `~/.omp/agent/...` resolves to `~/.omp/profiles//agent/...` instead. + +The relocation is uniform across the native provider (`builtin.ts`) and the generic `config.ts` helpers, so it covers slash commands, rules, prompts, instructions, hooks, tools, extensions, settings, skills, and MCP, plus the top-level `SYSTEM.md` / `RULES.md` / `AGENTS.md` files and runtime state (sessions, blobs, `agent.db`). A profile sees only its own OMP config, never the default profile's `~/.omp/agent`. + +The other source bases are not profile-scoped and load identically under every profile: the external-tool bases (`~/.claude`, `~/.codex`, `~/.gemini`) belong to those tools, and the project-level bases (`/.omp`, `/.claude`, ...) are keyed to the working directory. Throughout this document, read `~/.omp/agent` as shorthand for the active profile's agent directory. + ## Important constraint The generic helpers in `src/config.ts` do **not** include `.pi` in source discovery order. diff --git a/docs/extension-loading.md b/docs/extension-loading.md index d5b9778e7..5cfe94ee0 100644 --- a/docs/extension-loading.md +++ b/docs/extension-loading.md @@ -34,7 +34,7 @@ Native `extension-module` discovery comes from: - User directory: `~/.omp/agent/extensions` - Native legacy/settings JSON entries: `/.omp/settings.json#extensions` and `~/.omp/agent/settings.json#extensions` -Path roots come from the native provider (`SOURCE_PATHS.native`). Project lookup is cwd-only for these native roots; it does not walk ancestors. +The project root is the native provider's `.omp` directory (`SOURCE_PATHS.native.projectDir`), cwd-only; it does not walk ancestors. The user root is the active profile's agent directory via `getAgentDir()`, so under `omp --profile ` it becomes `~/.omp/profiles//agent/extensions` (and it honors `PI_CODING_AGENT_DIR`). See [Profiles](./config-usage.md#profiles). Notes: diff --git a/docs/mcp-config.md b/docs/mcp-config.md index 182e02f12..aa2ae40e5 100644 --- a/docs/mcp-config.md +++ b/docs/mcp-config.md @@ -15,7 +15,7 @@ Source of truth in code: OMP can discover MCP servers from multiple tools (`.claude/`, `.cursor/`, `.vscode/`, `opencode.json`, and more), but for OMP-native configuration you should usually use one of these primary files: - Project: `.omp/mcp.json` -- User: `~/.omp/agent/mcp.json` +- User: `~/.omp/agent/mcp.json` (or `~/.omp/profiles//agent/mcp.json` when a named profile is active — see [Profiles](#profiles)) The native provider also reads `.omp/.mcp.json` and `~/.omp/agent/.mcp.json` for compatibility, but OMP writes to the primary `mcp.json` paths above. @@ -26,6 +26,19 @@ OMP also accepts fallback standalone files in the project root: Use `.omp/mcp.json` or `~/.omp/agent/mcp.json` when you want OMP to own the configuration. Use root `mcp.json` / `.mcp.json` only when you want a portable fallback file that other MCP clients may also read. +### Profiles + +Named profiles (`omp --profile `, the `--alias` shortcut, or `OMP_PROFILE`/`PI_PROFILE`) isolate user-level MCP config. When a profile is active, the **user** scope resolves to the profile's agent directory instead of the default one: + +- Default profile: `~/.omp/agent/mcp.json` +- Profile ``: `~/.omp/profiles//agent/mcp.json` + +Discovery, the `/mcp` commands, and the config writer all follow the active profile, so a profile sees **only** its own user-level servers — never the default profile's `~/.omp/agent/mcp.json`. Add a server to a profile by launching under it (`omp --profile `) and running `/mcp add` → User level, or by editing `~/.omp/profiles//agent/mcp.json` directly. + +Project-scoped MCP config (`.omp/mcp.json`) is keyed to the working directory, not the profile, so it applies under every profile. External-tool configs (`.claude/`, `.cursor/`, etc.) are also profile-independent because they belong to those tools rather than to an OMP profile. + +MCP follows the same profile rules as the rest of OMP-native config; see [Configuration Discovery → Profiles](./config-usage.md#profiles). + ## Add a schema reference Add this line at the top of the file for editor autocomplete and validation: @@ -61,7 +74,7 @@ Top-level keys: - `$schema` — optional JSON Schema URL for tooling - `mcpServers` — map of server name to server config -- `disabledServers` — user-level denylist used to turn off discovered servers by name; runtime loading reads this list from `~/.omp/agent/mcp.json` +- `disabledServers` — user-level denylist used to turn off discovered servers by name; runtime loading reads this list from the active profile's user MCP file (`~/.omp/agent/mcp.json`, or `~/.omp/profiles//agent/mcp.json` under a named profile) Server names must match `^[a-zA-Z0-9_.-]{1,100}$`. @@ -185,6 +198,33 @@ OMP understands two auth-related objects. Use this when OMP should remember how to rehydrate credentials for a server. +You normally do not need to write this block: when OMP completes an OAuth flow +for an `http`/`sse` server it stores the credential under a deterministic id +derived from the active profile and server URL +(`mcp_oauth:profile::`), with the refresh material embedded. Any +config that points at the same URL — including a *definition-only* entry in a +shared project `mcp.json` with no `auth` block at all — resolves the active +profile's own credential automatically, including when auth storage is backed by +a shared auth broker. This is what makes project-scoped servers safe across +profiles: commit the definition, and each profile authorizes (and stays signed +in as) its own account via `/mcp reauth `. An explicit `credentialId` is +still honored when it resolves; if it points at another profile's row, OMP falls +back to the profile-scoped url-keyed binding. + +`/mcp reauth` on a definition-only entry leaves the file untouched — the +credential (refresh material included) lives entirely in the active profile's +auth storage (local `agent.db` or broker), so a committed project config never +picks up local auth state. An explicitly +configured `Authorization` header always wins over the url-keyed binding. + +The binding is per profile but not per project: once a profile has authorized +a URL, *any* checkout whose `mcp.json` defines a server at that URL connects +with that profile's credential automatically. Committed MCP definitions are +trusted input — the same already applies to `stdio` entries, which run +arbitrary commands — so review a repository's `mcp.json` before opening it +with a profile that holds credentials you care about, or use a dedicated +profile for untrusted checkouts. + ### `oauth` ```json @@ -193,12 +233,15 @@ Use this when OMP should remember how to rehydrate credentials for a server. "clientSecret": "...", "redirectUri": "...", "callbackPort": 3334, - "callbackPath": "/oauth/callback" + "callbackPath": "/oauth/callback", + "prompt": "consent" } ``` Use this when the MCP server requires explicit OAuth client settings. +`prompt` controls the OAuth `prompt` parameter sent with the authorization request. It defaults to `"consent"` so the provider always shows its consent/account screen — without it, a provider with an active browser session silently re-approves the same account, making it impossible to switch accounts or workspaces when reauthorizing (e.g. to use a different Linear workspace per OMP profile). Set it to `""` to omit the parameter for providers that reject it, or to another value the provider understands (e.g. `"select_account"`). + Slack is the clearest current example. Slack's MCP server is hosted at `https://mcp.slack.com/mcp`, uses Streamable HTTP, and requires confidential OAuth with your Slack app's client credentials. Example: diff --git a/docs/slash-command-internals.md b/docs/slash-command-internals.md index 560795c33..f4d17a405 100644 --- a/docs/slash-command-internals.md +++ b/docs/slash-command-internals.md @@ -167,7 +167,7 @@ The Extensions dashboard also loads `slash-commands` capability and displays act 1. **Extension commands** (`#tryExecuteExtensionCommand`) If `/name` matches extension-registered command, handler executes immediately and prompt returns. -2. **TypeScript custom commands and MCP prompt commands** (`#tryExecuteCustomCommand`) +2. **TypeScript custom commands and MCP prompt commands** (`#tryExecuteCustomCommand`) Boundary only: if matched, it executes and may return: - `string` -> replace prompt text with that string - `void/undefined` -> treated as handled; no LLM prompt diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index fdb559226..a45f973ed 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Added +- Added isolated profile support via `--profile ` / `OMP_PROFILE` and shell alias bootstrap via `--alias `, including launch/ACP bootstrap handling, extension-flag-safe parsing, profile-scoped user config discovery, and symlinked extension-directory discovery. - Fixed paste and image placeholders crashing when the editor renders before theme initialization. - Added `ModelRegistry.create(authStorage, modelsPath?)` async factory that runs the JSON → YAML migration step on `models.{yml,yaml}` asynchronously ahead of the sync constructor's bundled-model load. The sync `new ModelRegistry(...)` constructor still works (tests rely on it); production boot paths now use the factory so the migration's I/O lands off the event-loop hot path. - Added `ConfigFile.tryLoadAsync()`, `ConfigFile.loadAsync()`, `ConfigFile.loadOrDefaultAsync()`, `ConfigFile.getMtimeMsAsync()`, and `ConfigFile.warmup(file)` so the rest of the codebase can migrate config reads off the sync path. @@ -176,6 +177,9 @@ - Fixed a collapsed, still-streaming tool preview (an `eval`/`bash`/`ssh` box with output streaming in) reading as "weirdly truncated" — top border and head rows missing — once its box outgrew the viewport, snapping back to whole only while expanded with `ctrl+o` and breaking again when collapsed. A streaming preview was classified commit-unstable whenever collapsed, so the transcript offered none of its rows to native scrollback; once the box outgrew the window its head fell into the gap between the commit boundary and the window top, committed nowhere and repainted nowhere. The `provisionalPendingPreview` flag now applies only to the pending call preview (before any result) — once a streaming result exists the result renderer is the live, top-anchored shape and the block is commit-stable in both collapsed and expanded states, so its durable head always reaches scrollback. - Fixed a crash in subagent task execution and extensions when a string (instead of a string array) was returned or set for the system prompt. Gracefully wrap string values in arrays. - Fixed the todo completion reminder escalating 1/3 → 2/3 → 3/3 within a single user pause: `#checkTodoCompletion` appended a `` and then scheduled `agent.continue()`, so a text-only acknowledgement ("paused at your instruction") triggered another `agent_end` that re-ran the same check and fired the next reminder — no user input required. `AgentSession` now tracks `#todoReminderAwaitingProgress`: a reminder sets it, any `toolResult` (real progress) or a new user prompt clears it, and `#checkTodoCompletion` stays silent while it is set. Escalation through `todo.reminders.max` still works when the agent makes tool-level progress between stops ([#2590](https://github.com/can1357/oh-my-pi/issues/2590)). +- Fixed profile bootstrap so an extension-shadowed `--plan` flag no longer swallows a following global `--profile`. +- Fixed MCP OAuth URL-keyed credentials to stay profile-scoped under shared auth-broker storage and to clear discovered definition-only server auth during `/mcp unauth`. +- Fixed auto-learn managed skills to use the active profile's agent directory, so authored profile skills keep priority over managed fallbacks. ## [15.13.0] - 2026-06-14 diff --git a/packages/coding-agent/src/autolearn/managed-skills.ts b/packages/coding-agent/src/autolearn/managed-skills.ts index ecfc176c8..fa190a3be 100644 --- a/packages/coding-agent/src/autolearn/managed-skills.ts +++ b/packages/coding-agent/src/autolearn/managed-skills.ts @@ -9,11 +9,9 @@ */ import { constants as fsConstants, type Stats } from "node:fs"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; -import { isEnoent } from "@oh-my-pi/pi-utils"; +import { getAgentDir, isEnoent } from "@oh-my-pi/pi-utils"; import { YAML } from "bun"; -import { SOURCE_PATHS } from "../discovery/helpers"; /** Provider id stamped on discovered managed skills (distinguishes them from authored). */ export const MANAGED_SKILLS_PROVIDER_ID = "omp-managed"; @@ -24,8 +22,8 @@ export const MAX_MANAGED_SKILL_BYTES = 64_000; const SKILL_NAME_PATTERN = /^[a-z0-9][a-z0-9-]{0,63}$/; /** Resolve the isolated managed-skills directory (`~/.omp/agent/managed-skills`). */ -export function getManagedSkillsDir(home: string = os.homedir()): string { - return path.join(home, SOURCE_PATHS.native.userAgent, "managed-skills"); +export function getManagedSkillsDir(agentDir: string = getAgentDir()): string { + return path.join(agentDir, "managed-skills"); } /** diff --git a/packages/coding-agent/src/capability/mcp.ts b/packages/coding-agent/src/capability/mcp.ts index d89298fba..33990ccfb 100644 --- a/packages/coding-agent/src/capability/mcp.ts +++ b/packages/coding-agent/src/capability/mcp.ts @@ -38,13 +38,14 @@ export interface MCPServer { clientSecret?: string; resource?: string; }; - /** OAuth configuration (clientId, clientSecret, redirectUri, callbackPort, callbackPath) for servers requiring explicit client credentials */ + /** OAuth configuration (clientId, clientSecret, redirectUri, callbackPort, callbackPath, prompt) for servers requiring explicit client credentials */ oauth?: { clientId?: string; clientSecret?: string; redirectUri?: string; callbackPort?: number; callbackPath?: string; + prompt?: string; }; /** Transport type */ transport?: "stdio" | "sse" | "http"; diff --git a/packages/coding-agent/src/cli.ts b/packages/coding-agent/src/cli.ts index d3ae9423d..417f8cd66 100755 --- a/packages/coding-agent/src/cli.ts +++ b/packages/coding-agent/src/cli.ts @@ -15,8 +15,17 @@ try { * lightweight CLI runner from pi-utils. */ import type { CliConfig } from "@oh-my-pi/pi-utils/cli"; -import { APP_NAME, MIN_BUN_VERSION, VERSION } from "@oh-my-pi/pi-utils/dirs"; -import { declareWorkerHostEntry } from "@oh-my-pi/pi-utils/env"; +import { + APP_NAME, + getActiveProfile, + MIN_BUN_VERSION, + resolveProfileEnv, + setProfile, + VERSION, +} from "@oh-my-pi/pi-utils/dirs"; +import { declareWorkerHostEntry } from "@oh-my-pi/pi-utils/worker-host"; +import { installProfileAlias, resolveProfileAliasCommandFromProcess } from "./cli/profile-alias"; +import { extractProfileFlags } from "./cli/profile-bootstrap"; if (Bun.semver.order(Bun.version, MIN_BUN_VERSION) < 0) { process.stderr.write( @@ -27,11 +36,11 @@ if (Bun.semver.order(Bun.version, MIN_BUN_VERSION) < 0) { process.title = APP_NAME; -// Declare this module as the worker-host entry: Worker threads and worker -// subprocesses re-enter `Bun.main` with a hidden argv selector instead of -// loading separate worker entrypoints (single-entry contract across source, -// npm bundle, and compiled binary). -declareWorkerHostEntry(); +// Worker-host entry declaration (Worker threads and worker subprocesses +// re-enter `Bun.main` with a hidden argv selector instead of loading separate +// worker entrypoints) happens inside `runCli` after profile bootstrap: +// `@oh-my-pi/pi-utils/env` eagerly loads `.env` from the agent directory at +// import time, so it must not be imported before `setProfile` runs. async function showHelp(config: CliConfig): Promise { const { renderRootHelp } = await import("@oh-my-pi/pi-utils/cli"); @@ -196,15 +205,67 @@ async function runTinyWorker(): Promise { /** Run the CLI with the given argv (no `process.argv` prefix). */ export async function runCli(argv: string[]): Promise { - if (argv[0] === "--smoke-test") { - await runSmokeTest(); + let resolvedArgv = argv; + try { + const extracted = extractProfileFlags(resolvedArgv); + resolvedArgv = extracted.argv; + if (extracted.profile !== undefined) { + setProfile(extracted.profile); + } else { + // No explicit --profile: activate any OMP_PROFILE/PI_PROFILE inherited + // from the environment. Module-load resolution deliberately swallows an + // invalid value to avoid an uncaught throw before this try/catch is in + // scope (see `readProfileFromEnvSafe` in dirs.ts), and callers may set + // OMP_PROFILE after importing this module (profile aliases/tests). Surfacing + // validation here turns `OMP_PROFILE=.. omp --version` into a clean error; + // calling setProfile keeps every later path helper on the env-selected + // profile instead of the default agent directory. + setProfile(resolveProfileEnv(process.env.OMP_PROFILE, process.env.PI_PROFILE)); + } + if (extracted.aliasName !== undefined) { + const profile = extracted.profile ?? getActiveProfile(); + if (!profile) { + throw new Error("--alias requires --profile or OMP_PROFILE"); + } + const result = await installProfileAlias({ + profile, + aliasName: extracted.aliasName, + command: resolveProfileAliasCommandFromProcess(), + }); + process.stdout.write( + `Created ${result.aliasName} for profile ${result.profile} in ${result.configPath}\n` + + `Restart your shell or run: ${result.reloadedWith}\n` + + `Then use: ${result.aliasName} update, ${result.aliasName} --version, or ${result.aliasName}\n`, + ); + return; + } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + process.stderr.write(`Error: ${message}\n`); + process.exitCode = 1; return; } - if (TINY_WORKER_ARGS.has(argv[0] ?? "")) { + + // Worker-thread entry dispatch must run before the first `await`: the + // stats sync worker's buffering onmessage handler is installed in the + // synchronous prefix of `runWorkerEntrypoint`, and Bun flushes the + // worker's parked initial messages as soon as the entry module's + // top-level evaluation finishes. + if (TINY_WORKER_ARGS.has(resolvedArgv[0] ?? "")) { await runTinyWorker(); return; } - if (await runWorkerEntrypoint(argv[0])) { + if (await runWorkerEntrypoint(resolvedArgv[0])) { + return; + } + + // Declare this module as the worker-host entry now that the active profile + // is resolved. The worker-host module is side-effect-free; importing + // `@oh-my-pi/pi-utils/env` here would snapshot the wrong agent `.env`. + declareWorkerHostEntry(); + + if (resolvedArgv[0] === "--smoke-test") { + await runSmokeTest(); return; } const [{ run }, { commands, resolveCliArgv }] = await Promise.all([ @@ -213,7 +274,7 @@ export async function runCli(argv: string[]): Promise { ]); // --help and --version are handled by run() directly, don't rewrite those. // Everything else that isn't a known subcommand routes to "launch". - const resolved = resolveCliArgv(argv); + const resolved = resolveCliArgv(resolvedArgv); if ("error" in resolved) { process.stderr.write(`error: ${resolved.error}\n`); process.exitCode = 1; @@ -226,7 +287,13 @@ export async function runCli(argv: string[]): Promise { // lowering) builds to fail, and the entrypoint needs nothing after this. // The catch mirrors what an unhandled TLA rejection produced: error dump to // stderr, exit code 1. Success paths resolve without touching the exit code. -runCli(process.argv.slice(2)).catch((err: unknown) => { - process.stderr.write(`${Bun.inspect(err, { colors: process.stderr.isTTY === true })}\n`); - process.exit(1); -}); +// Guarded so importing `runCli` (profile CLI tests, SDK embedding) does not +// launch the agent as a side effect. Worker threads re-enter this module as +// their entry with `import.meta.main === false`, so the worker-host dispatch +// is admitted via `!Bun.isMainThread`. +if (import.meta.main || !Bun.isMainThread) { + runCli(process.argv.slice(2)).catch((err: unknown) => { + process.stderr.write(`${Bun.inspect(err, { colors: process.stderr.isTTY === true })}\n`); + process.exit(1); + }); +} diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index ef0dffa72..ee315935a 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -5,12 +5,22 @@ import { type Effort, THINKING_EFFORTS } from "@oh-my-pi/pi-catalog/effort"; import { APP_NAME, CONFIG_DIR_NAME, logger } from "@oh-my-pi/pi-utils"; import chalk from "chalk"; import { parseEffort } from "../thinking"; -import { BUILTIN_TOOLS } from "../tools"; +import { BUILTIN_TOOL_NAMES } from "../tools/builtin-names"; +import { + OPTIONAL_FLAGS, + OPTIONAL_VALUE_FLAGS, + type ParseDeps, + PROFILE_BOOTSTRAP_BOUNDARY_ARG, + STRING_SETTERS, + STRING_VALUE_FLAGS, +} from "./flag-tables"; export type Mode = "text" | "json" | "rpc" | "acp" | "rpc-ui"; export interface Args { cwd?: string; + profile?: string; + alias?: string; allowHome?: boolean; provider?: string; model?: string; @@ -68,6 +78,19 @@ export interface Args { unrecognizedFlags: string[]; } +/** + * Runtime dependencies the data-driven setters need. Constructed once at + * module load and passed to every {@link STRING_SETTERS} call so the + * setter table itself can stay free of `@oh-my-pi/pi-utils` runtime imports + * (which would otherwise trip the profile bootstrap's env-init ordering). + */ +const PARSE_DEPS: ParseDeps = { + logger, + parseEffort, + builtinToolNames: BUILTIN_TOOL_NAMES, + thinkingEfforts: THINKING_EFFORTS, +}; + export function parseArgs(inputArgs: string[], extensionFlags?: Map): Args { // Work on a copy: the `--option=value` handling below splices the value // into the array, and callers reuse the same argv (the post-extension @@ -81,19 +104,19 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map s.trim()); } else if (arg === "--no-tools") { result.noTools = true; } else if (arg === "--no-lsp") { result.noLsp = true; } else if (arg === "--no-pty") { result.noPty = true; - } else if (arg === "--tools" && i + 1 < args.length) { - const toolNames = args[++i] - .split(",") - .map(s => s.trim().toLowerCase()) - .filter(Boolean); - const validTools: string[] = []; - for (const name of toolNames) { - if (name in BUILTIN_TOOLS) { - validTools.push(name); - } else { - logger.warn("Unknown tool passed to --tools", { - tool: name, - validTools: Object.keys(BUILTIN_TOOLS), - }); - } - } - result.tools = validTools; - } else if (arg === "--thinking" && i + 1 < args.length) { - const rawThinking = args[++i]; - const thinking = parseEffort(rawThinking); - if (thinking !== undefined) { - result.thinking = thinking; - } else { - logger.warn("Invalid thinking level passed to --thinking", { - level: rawThinking, - validThinkingLevels: THINKING_EFFORTS, - }); - } } else if (arg === "--hide-thinking") { result.hideThinking = true; } else if (arg === "--print" || arg === "-p") { result.print = true; - } else if (arg === "--export" && i + 1 < args.length) { - result.export = args[++i]; - } else if (arg === "--hook" && i + 1 < args.length) { - result.hooks = result.hooks ?? []; - result.hooks.push(args[++i]); - } else if ((arg === "--extension" || arg === "-e") && i + 1 < args.length) { - result.extensions = result.extensions ?? []; - result.extensions.push(args[++i]); - } else if (arg === "--plugin-dir" && i + 1 < args.length) { - result.pluginDirs = result.pluginDirs ?? []; - result.pluginDirs.push(args[++i]); } else if (arg === "--no-extensions") { result.noExtensions = true; } else if (arg === "--no-skills") { @@ -236,19 +200,6 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map s.trim()); } else if (arg.startsWith("@")) { result.fileArgs.push(arg.slice(1)); // Remove @ prefix } else if (!arg.startsWith("-") || arg === "-") { @@ -344,13 +295,14 @@ export function getExtraHelpText(): string { ANTHROPIC_SEARCH_BASE_URL - Anthropic web search base URL (override; pairs with ANTHROPIC_SEARCH_API_KEY) ${chalk.dim("# Configuration")} + OMP_PROFILE - Named profile for isolated agent state (same as --profile) + Use \`omp --profile --alias \` to create a shell shortcut for a profile PI_CODING_AGENT_DIR - Session storage directory (default: ~/${CONFIG_DIR_NAME}/agent) PI_PACKAGE_DIR - Override package directory (for Nix/Guix store paths) PI_SMOL_MODEL - Override smol/fast model (see --smol) PI_SLOW_MODEL - Override slow/reasoning model (see --slow) PI_PLAN_MODEL - Override planning model (see --plan) PI_NO_PTY - Disable PTY-based interactive bash execution - For complete environment variable reference, see: ${chalk.dim("docs/environment-variables.md")} ${chalk.bold("Available Tools (default-enabled unless noted):")} diff --git a/packages/coding-agent/src/cli/completion-gen.ts b/packages/coding-agent/src/cli/completion-gen.ts index 33e8b0458..60aecbc17 100644 --- a/packages/coding-agent/src/cli/completion-gen.ts +++ b/packages/coding-agent/src/cli/completion-gen.ts @@ -15,7 +15,7 @@ * knob and is keyed by flag name so it stays stable as flags are added. */ import type { ArgDescriptor, CliConfig, CommandCtor, FlagDescriptor } from "@oh-my-pi/pi-utils/cli"; -import { BUILTIN_TOOLS } from "../tools"; +import { BUILTIN_TOOL_NAMES } from "../tools/builtin-names"; export type Shell = "bash" | "zsh" | "fish"; @@ -77,7 +77,7 @@ function flagValue(name: string, desc: FlagDescriptor): ValueSource { if (MODEL_FLAGS[name]) return { kind: "models", multiple: false }; if (name === "models") return { kind: "models", multiple: true }; if (SESSION_FLAGS[name]) return { kind: "sessions" }; - if (name === "tools") return { kind: "list", values: Object.keys(BUILTIN_TOOLS) }; + if (name === "tools") return { kind: "list", values: BUILTIN_TOOL_NAMES }; if (DIR_FLAGS[name]) return { kind: "dir" }; if (desc.kind === "integer") return { kind: "value" }; return { kind: "file" }; diff --git a/packages/coding-agent/src/cli/flag-tables.ts b/packages/coding-agent/src/cli/flag-tables.ts new file mode 100644 index 000000000..4e2354e12 --- /dev/null +++ b/packages/coding-agent/src/cli/flag-tables.ts @@ -0,0 +1,270 @@ +/** + * Single source of truth for argv flag classification, shared by: + * - `parseArgs` in `./args.ts` (the launch-time CLI parser) + * - `extractProfileFlags` in `./profile-bootstrap.ts` (the early + * `--profile` / `--alias` pre-parser) + * + * `parseArgs` dispatches string-valued flags by looking up their setter in + * {@link STRING_SETTERS}. Optional-value flags use {@link OPTIONAL_FLAGS} so + * per-flag quirks (currently empty-string rejection for `--resume`) live here + * instead of being hard-coded in the dispatch loop. + * + * The bootstrap doesn't dispatch — it only needs to know which flags consume + * a value — so it consults {@link STRING_VALUE_FLAGS} and + * {@link OPTIONAL_VALUE_FLAGS}, both derived from `Object.keys(...)` on the + * setter/config records below. + * + * The deliberate consequence: a string-valued flag exists in this CLI surface + * iff it has an entry here. Adding a new string-valued flag means adding a + * setter/config entry in this file; both `args.ts` and the bootstrap pick it + * up automatically, so the two cannot drift out of sync. + * + * IMPORT RULE: this module MUST NOT import any runtime value from + * `@oh-my-pi/pi-utils` (or anything that transitively does). That package's + * `env.ts` eagerly loads `.env` files from `getAgentDir()` during module + * initialization, which would race the profile bootstrap. Type-only imports + * are erased at runtime and are therefore safe. + * + * If a setter needs runtime dependencies (logging, validators, lookup + * tables), they're passed in through {@link ParseDeps} and `args.ts` wires the + * real implementations at the dispatch site. + */ + +import type { Effort } from "@oh-my-pi/pi-ai"; +import type { Args } from "./args"; + +/** + * Runtime dependencies injected into setters that need to validate input or + * warn about bad values. `args.ts` constructs one object at module load and + * passes it to each {@link STRING_SETTERS} call. + * + * Keeping these out of the setter closures means this module stays free of + * runtime imports from `@oh-my-pi/pi-utils`, which is the whole reason it can + * be safely imported by `profile-bootstrap.ts` before `setProfile` runs. + */ +export interface ParseDeps { + logger: { warn: (message: string, meta?: Record) => void }; + parseEffort: (value: string | null | undefined) => Effort | undefined; + builtinToolNames: readonly string[]; + thinkingEfforts: readonly string[]; +} + +export type StringSetter = (result: Args, value: string, deps: ParseDeps) => void; + +/** + * Setter for a flag that may or may not consume the next argv token. + * Receives `undefined` for the bare form (`--resume` with no value, etc.). + */ +export type OptionalSetter = (result: Args, value: string | undefined) => void; + +/** + * Per-flag optional-value consumption policy. + * + * Every optional flag always rejects tokens that start with `-` — that shared + * rule lives in the dispatch site. These booleans capture the *additional* + * per-flag quirks: + * + * - `rejectEmpty`: treat `""` like “no value provided”. Needed for + * `--resume` / `-r` / `--session`. Without it, an empty string + * gets consumed as the session prefix and downstream resolution can match + * every session. + */ +export interface OptionalFlagConfig { + set: OptionalSetter; + rejectEmpty?: boolean; +} + +// Shared setters for flags that alias the same field. +const setExtension: StringSetter = (result, value) => { + result.extensions = result.extensions ?? []; + result.extensions.push(value); +}; + +const setResume: OptionalSetter = (result, value) => { + result.resume = value !== undefined ? value : true; +}; + +/** + * Setters for flags with string values. Most built-ins consume the next argv + * token even when it starts with `-`; flags listed in + * {@link EXTENSION_SHADOWABLE_STRING_FLAGS} use extension-style consumption so + * a registered boolean extension can shadow them before profile bootstrap. + */ +export const STRING_SETTERS: Record = { + "--cwd": (result, value) => { + result.cwd = value; + }, + "--config": (result, value) => { + result.config = [...(result.config ?? []), value]; + }, + "--mode": (result, value) => { + if (value === "text" || value === "json" || value === "rpc" || value === "acp" || value === "rpc-ui") { + result.mode = value; + } + }, + "--fork": (result, value) => { + result.fork = value; + }, + "--provider": (result, value) => { + result.provider = value; + }, + "--model": (result, value) => { + result.model = value; + }, + "--smol": (result, value) => { + result.smol = value; + }, + "--slow": (result, value) => { + result.slow = value; + }, + "--plan": (result, value) => { + result.plan = value; + }, + "--api-key": (result, value) => { + result.apiKey = value; + }, + "--system-prompt": (result, value) => { + result.systemPrompt = value; + }, + "--append-system-prompt": (result, value) => { + result.appendSystemPrompt = value; + }, + "--provider-session-id": (result, value) => { + result.providerSessionId = value; + }, + "--session-dir": (result, value) => { + result.sessionDir = value; + }, + "--models": (result, value) => { + result.models = value.split(",").map(s => s.trim()); + }, + "--tools": (result, value, deps) => { + const names = value + .split(",") + .map(s => s.trim().toLowerCase()) + .filter(Boolean); + const valid: string[] = []; + for (const name of names) { + if (deps.builtinToolNames.includes(name)) { + valid.push(name); + } else { + deps.logger.warn("Unknown tool passed to --tools", { + tool: name, + validTools: deps.builtinToolNames, + }); + } + } + result.tools = valid; + }, + "--thinking": (result, value, deps) => { + const thinking = deps.parseEffort(value); + if (thinking !== undefined) { + result.thinking = thinking; + } else { + deps.logger.warn("Invalid thinking level passed to --thinking", { + level: value, + validThinkingLevels: deps.thinkingEfforts, + }); + } + }, + "--export": (result, value) => { + result.export = value; + }, + "--hook": (result, value) => { + result.hooks = result.hooks ?? []; + result.hooks.push(value); + }, + "--extension": setExtension, + "-e": setExtension, + "--plugin-dir": (result, value) => { + result.pluginDirs = result.pluginDirs ?? []; + result.pluginDirs.push(value); + }, + "--skills": (result, value) => { + result.skills = value.split(",").map(s => s.trim()); + }, + "--approval-mode": (result, value, deps) => { + if (value === "always-ask" || value === "write" || value === "yolo") { + result.approvalMode = value; + } else { + deps.logger.warn("Invalid value passed to --approval-mode", { + value, + validValues: ["always-ask", "write", "yolo"], + }); + } + }, +}; + +/** + * Optional-value flags. Setters receive `undefined` for the bare form. + * + * The dispatch in `args.ts` applies the shared "doesn't start with `-`" + * check for every flag, then consults the per-flag booleans below for the + * remaining quirks. + */ +export const OPTIONAL_FLAGS: Record = { + "--resume": { set: setResume, rejectEmpty: true }, + "-r": { set: setResume, rejectEmpty: true }, + "--session": { set: setResume, rejectEmpty: true }, +}; + +/** + * Derived from {@link STRING_SETTERS}. A flag is in this set if and only if + * it has a setter — by construction, drift between "the bootstrap thinks + * this flag accepts a value" and "the launch parser can set one" is + * structurally impossible. + */ +export const STRING_VALUE_FLAGS: ReadonlySet = new Set(Object.keys(STRING_SETTERS)); + +/** + * Built-in string flags known to be shadowed by bundled/common boolean + * extensions before extension metadata is available. They still accept a + * value-like successor for the built-in form (`--plan opus`), but a + * flag-looking successor remains a fresh flag (`--plan --profile work`). + */ +export const EXTENSION_SHADOWABLE_STRING_FLAGS: ReadonlySet = new Set(["--plan"]); + +/** + * Derived from {@link OPTIONAL_FLAGS}. Same single-source contract as + * {@link STRING_VALUE_FLAGS}. + */ +export const OPTIONAL_VALUE_FLAGS: ReadonlySet = new Set(Object.keys(OPTIONAL_FLAGS)); + +/** + * Internal marker inserted by the profile bootstrap when removing `--profile` + * or `--alias` would otherwise make the following value-like token become the + * value of a preceding optional/extension flag. `parseArgs` ignores it, but its + * flag-looking shape preserves argv boundaries during the second parse. + */ +export const PROFILE_BOOTSTRAP_BOUNDARY_ARG = "--omp-profile-boundary"; + +/** + * Long-form launch flags that take NO value (booleans). The bootstrap pre-parser + * needs this to tell a known value-less flag (whose successor is a fresh + * argument — `omp --print --profile work` still selects a profile) apart from an + * UNKNOWN long option that might be an extension string flag consuming the next + * token as its value (so the bootstrap must not steal that token as a global + * `--profile`/`--alias`). MUST mirror the value-less flag arms of `parseArgs` + * in `./args.ts`: adding a new boolean launch flag there means adding it here, + * or `-- --profile X` stops selecting a profile. Short aliases + * (`-h`/`-v`/`-c`/`-p`) are intentionally omitted — the protection rule only + * fires for `--`-prefixed tokens. + */ +export const VALUELESS_FLAGS: ReadonlySet = new Set([ + "--help", + "--version", + "--allow-home", + "--continue", + "--no-session", + "--no-tools", + "--no-lsp", + "--no-pty", + "--hide-thinking", + "--print", + "--no-extensions", + "--no-skills", + "--no-rules", + "--no-title", + "--auto-approve", + "--yolo", +]); diff --git a/packages/coding-agent/src/cli/profile-alias.ts b/packages/coding-agent/src/cli/profile-alias.ts new file mode 100644 index 000000000..1bc50c9f3 --- /dev/null +++ b/packages/coding-agent/src/cli/profile-alias.ts @@ -0,0 +1,338 @@ +import * as os from "node:os"; +import * as path from "node:path"; +import { normalizeProfileName } from "@oh-my-pi/pi-utils/dirs"; + +export type ProfileAliasShell = "bash" | "zsh" | "fish" | "powershell" | "pwsh"; + +function quoteForShell(pathValue: string): string { + return `'${pathValue.replace(/'/g, `'"'"'`)}'`; +} + +function quoteForPowerShell(pathValue: string): string { + return `'${pathValue.replace(/'/g, `''`)}'`; +} + +export interface ProfileAliasCommand { + display: string; + posix: string; + fish: string; + powerShell: string; +} + +const DEFAULT_ALIAS_COMMAND: ProfileAliasCommand = { + display: "omp", + posix: "omp", + fish: "omp", + powerShell: "omp", +}; + +export interface ProfileAliasInstallOptions { + profile: string; + aliasName: string; + shellPath?: string; + platform?: NodeJS.Platform; + homeDir?: string; + env?: NodeJS.ProcessEnv; + readFile?: (filePath: string) => Promise; + command?: ProfileAliasCommand; + writeFile?: (filePath: string, content: string) => Promise; +} + +export interface ProfileAliasInstallResult { + shell: ProfileAliasShell; + configPath: string; + aliasName: string; + profile: string; + command: string; + reloadedWith: string; +} + +const ALIAS_NAME_RE = /^[A-Za-z_][A-Za-z0-9_-]{0,63}$/; +const POSIX_RESERVED_ALIAS_NAMES: ReadonlySet = new Set([ + "case", + "coproc", + "do", + "done", + "elif", + "else", + "esac", + "fi", + "for", + "function", + "if", + "in", + "select", + "then", + "time", + "until", + "while", +]); +const FISH_RESERVED_ALIAS_NAMES: ReadonlySet = new Set([ + "and", + "begin", + "break", + "builtin", + "case", + "command", + "continue", + "else", + "end", + "exec", + "for", + "function", + "if", + "not", + "or", + "return", + "switch", + "while", +]); +const POWERSHELL_RESERVED_ALIAS_NAMES: ReadonlySet = new Set([ + "begin", + "break", + "catch", + "class", + "continue", + "data", + "do", + "dynamicparam", + "else", + "elseif", + "end", + "enum", + "exit", + "filter", + "finally", + "for", + "foreach", + "from", + "function", + "if", + "in", + "param", + "process", + "return", + "switch", + "throw", + "trap", + "try", + "until", + "using", + "var", + "while", + "workflow", +]); + +// Keep local: importing the pi-utils root here would eagerly load env before +// cli.ts has applied --profile, regressing profile-specific .env loading. +function isEnoentError(error: unknown): boolean { + return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT"; +} + +function getReservedAliasNames(shell: ProfileAliasShell): ReadonlySet { + switch (shell) { + case "bash": + case "zsh": + return POSIX_RESERVED_ALIAS_NAMES; + case "fish": + return FISH_RESERVED_ALIAS_NAMES; + case "powershell": + case "pwsh": + return POWERSHELL_RESERVED_ALIAS_NAMES; + } +} + +function validateAliasName(aliasName: string, shell: ProfileAliasShell): string { + const normalized = aliasName.trim(); + if (!ALIAS_NAME_RE.test(normalized)) { + throw new Error(`Invalid alias "${aliasName}". Alias names must match ${ALIAS_NAME_RE.source}.`); + } + if (normalized.toLowerCase() === "omp") { + throw new Error('Invalid alias "omp". Refusing to shadow the base omp command.'); + } + if (getReservedAliasNames(shell).has(normalized.toLowerCase())) { + throw new Error(`Invalid alias "${aliasName}". Refusing to create a ${shell} reserved word.`); + } + return normalized; +} + +// On Windows the launching shell is rarely exported through $SHELL, so when it +// is missing we infer the PowerShell edition from the inherited environment. +// PowerShell 7 (pwsh) always seeds PSModulePath with separator-delimited +// ".../PowerShell/..." module directories (plus the Windows PowerShell ones for +// back-compat), whereas Windows PowerShell 5.1 only ever lists +// ".../WindowsPowerShell/...". The separator anchors keep "WindowsPowerShell" +// from matching. POWERSHELL_DISTRIBUTION_CHANNEL is set only by some pwsh +// distributions, so it stays a secondary hint rather than the primary signal. +function detectWindowsPowerShell(env: NodeJS.ProcessEnv): ProfileAliasShell { + const modulePath = env.PSModulePath ?? env.PSMODULEPATH ?? env.psmodulepath ?? ""; + if (/[\\/]PowerShell[\\/]/i.test(modulePath)) return "pwsh"; + if (env.POWERSHELL_DISTRIBUTION_CHANNEL) return "pwsh"; + return "powershell"; +} + +function normalizeShellName( + shellPath: string | undefined, + platform: NodeJS.Platform, + env: NodeJS.ProcessEnv, +): ProfileAliasShell { + const shell = path + .basename(shellPath ?? "") + .toLowerCase() + .replace(/\.exe$/, ""); + if (shell === "zsh") return "zsh"; + if (shell === "bash") return "bash"; + if (shell === "fish") return "fish"; + if (shell === "pwsh") return "pwsh"; + if (shell === "powershell") return "powershell"; + if (platform === "win32") return detectWindowsPowerShell(env); + throw new Error(`Unsupported shell${shell ? ` "${shell}"` : ""}. Supported shells: bash, zsh, fish, PowerShell.`); +} + +export function resolveProfileAliasCommandFromProcess( + argv: readonly string[] = process.argv, + cwd: string = process.cwd(), +): ProfileAliasCommand { + const runtime = argv[0]; + const script = argv[1]; + if (!runtime || !script || !/\.[cm]?[jt]s$/.test(script)) return DEFAULT_ALIAS_COMMAND; + + const scriptPath = path.resolve(cwd, script); + const posix = `${quoteForShell(runtime)} ${quoteForShell(scriptPath)}`; + return { + display: `${runtime} ${scriptPath}`, + posix, + fish: posix, + powerShell: `${quoteForPowerShell(runtime)} ${quoteForPowerShell(scriptPath)}`, + }; +} + +function resolveShellConfigPath( + shell: ProfileAliasShell, + homeDir: string, + platform: NodeJS.Platform, + env: NodeJS.ProcessEnv, +): string { + switch (shell) { + case "zsh": + return path.join(env.ZDOTDIR || homeDir, ".zshrc"); + case "bash": + return platform === "darwin" ? path.join(homeDir, ".bash_profile") : path.join(homeDir, ".bashrc"); + case "fish": { + // fish sources conf.d from $XDG_CONFIG_HOME/fish (default ~/.config/fish); + // a hard-coded ~/.config would be silently ignored when the user relocates + // their XDG config root, leaving the alias unsourced after a restart. + const configHome = env.XDG_CONFIG_HOME || path.join(homeDir, ".config"); + return path.join(configHome, "fish", "conf.d", "omp-profiles.fish"); + } + case "pwsh": + return platform === "win32" + ? path.join(homeDir, "Documents", "PowerShell", "Microsoft.PowerShell_profile.ps1") + : path.join(homeDir, ".config", "powershell", "Microsoft.PowerShell_profile.ps1"); + case "powershell": + return path.join(homeDir, "Documents", "WindowsPowerShell", "Microsoft.PowerShell_profile.ps1"); + } +} + +function renderAliasBlock( + shell: ProfileAliasShell, + aliasName: string, + profile: string, + command: ProfileAliasCommand, +): { block: string; command: string } { + const profiledCommand = `${command.display} --profile=${profile}`; + const start = `# >>> omp profile alias: ${aliasName} >>>`; + const end = `# <<< omp profile alias: ${aliasName} <<<`; + let body: string; + switch (shell) { + case "fish": + body = [ + `function ${aliasName} --wraps omp --description 'OMP profile ${profile}'`, + ` command ${command.fish} --profile=${profile} $argv`, + "end", + ].join("\n"); + break; + case "powershell": + case "pwsh": + body = [`function ${aliasName} {`, ` & ${command.powerShell} --profile=${profile} @args`, "}"].join("\n"); + break; + default: + body = [`${aliasName}() {`, ` command ${command.posix} --profile=${profile} "$@"`, "}"].join("\n"); + break; + } + return { block: `${start}\n${body}\n${end}`, command: profiledCommand }; +} + +function upsertBlock(content: string, aliasName: string, block: string): string { + const start = `# >>> omp profile alias: ${aliasName} >>>`; + const end = `# <<< omp profile alias: ${aliasName} <<<`; + const startIndex = content.indexOf(start); + if (startIndex !== -1) { + const endIndex = content.indexOf(end, startIndex + start.length); + if (endIndex === -1) { + throw new Error( + `Found "${start}" without a matching "${end}" in the shell config. ` + + `The managed alias block is malformed; remove the stale marker line and rerun --alias.`, + ); + } + const afterEnd = endIndex + end.length; + const prefix = content.slice(0, startIndex).replace(/[\t ]*\n?$/, ""); + const suffix = content.slice(afterEnd).replace(/^\n?/, ""); + return [prefix, block, suffix].filter(Boolean).join("\n\n").replace(/\n*$/, "\n"); + } + const trimmed = content.replace(/\s*$/, ""); + return `${trimmed}${trimmed ? "\n\n" : ""}${block}\n`; +} + +function readAliasConfigText(filePath: string): Promise { + return Bun.file(filePath).text(); +} + +export async function readProfileAliasConfigFile( + filePath: string, + readText: (filePath: string) => Promise = readAliasConfigText, +): Promise { + try { + return await readText(filePath); + } catch (error) { + if (isEnoentError(error)) return ""; + throw error; + } +} + +export async function installProfileAlias(options: ProfileAliasInstallOptions): Promise { + const profile = normalizeProfileName(options.profile); + if (!profile) { + throw new Error("--alias requires a named --profile value."); + } + const platform = options.platform ?? process.platform; + const homeDir = options.homeDir ?? os.homedir(); + const env = options.env ?? process.env; + const shell = normalizeShellName(options.shellPath ?? env.SHELL, platform, env); + const aliasName = validateAliasName(options.aliasName, shell); + const configPath = resolveShellConfigPath(shell, homeDir, platform, env); + const { block, command } = renderAliasBlock(shell, aliasName, profile, options.command ?? DEFAULT_ALIAS_COMMAND); + const readFile = options.readFile ?? readProfileAliasConfigFile; + const writeFile = + options.writeFile ?? + (async (filePath, content) => { + await Bun.write(filePath, content); + }); + + const current = await readFile(configPath); + await writeFile(configPath, upsertBlock(current, aliasName, block)); + + return { + shell, + configPath, + aliasName, + profile, + command, + reloadedWith: + shell === "fish" + ? `source ${quoteForShell(configPath)}` + : shell === "powershell" || shell === "pwsh" + ? `. ${quoteForPowerShell(configPath)}` + : `. ${quoteForShell(configPath)}`, + }; +} diff --git a/packages/coding-agent/src/cli/profile-bootstrap.ts b/packages/coding-agent/src/cli/profile-bootstrap.ts new file mode 100644 index 000000000..bc3b4a825 --- /dev/null +++ b/packages/coding-agent/src/cli/profile-bootstrap.ts @@ -0,0 +1,243 @@ +/** + * Bootstrap-time argv preparser for the global `--profile` / `--alias` flags. + * + * Profile selection MUST happen before any module reads `getAgentDir()` (notably + * `@oh-my-pi/pi-utils/env`, which eagerly loads `.env` from the agent directory + * during its own import). The full `parseArgs` from `./args.ts` lives downstream + * of those imports, so we can't rely on it for profile bootstrap — we have to + * crack open argv before the lazy command modules load. + * + * Because of that, this preparser must respect the same value-consumption + * contract as `args.ts`: known string-valued flags usually consume the next + * token even when it starts with `-`, except for string flags that can be + * shadowed by preloaded boolean extensions (currently `--plan`). Optional-value + * flags (`--resume`, `--session`, `-r`) consume the next token only when it + * doesn't look like another flag. Without this, `omp --system-prompt --profile + * foo` silently activates profile `foo` + * instead of passing the literal `--profile` to the system prompt and `foo` + * as a positional message. + * + * The shared classification lives in {@link ./flag-tables}, imported below, + * so the bootstrap and `args.ts` reference one source of truth instead of + * maintaining parallel constants. + * + * An unclassified bare long option (one not in any flag table) is treated as a + * possible extension string flag, but the bootstrap mirrors `parseArgs`' + * extension-flag rules ({@link ./args}): a string extension flag consumes its + * successor ONLY when that successor is value-like (does not start with `-`), and + * a boolean extension flag consumes nothing. So the successor is forwarded + * untouched (and never read as a global `--profile`/`--alias`) only when it is + * value-like; a flag-looking successor is left for normal processing, so + * `omp --some-ext-flag --profile work` still selects a profile. Known value-less + * launch flags ({@link VALUELESS_FLAGS}) are exempt so a trailing profile after + * them also activates (`omp --print --profile work`). + */ + +import { isSubcommand } from "../cli-commands"; +import { + EXTENSION_SHADOWABLE_STRING_FLAGS, + OPTIONAL_FLAGS, + OPTIONAL_VALUE_FLAGS, + PROFILE_BOOTSTRAP_BOUNDARY_ARG, + STRING_VALUE_FLAGS, + VALUELESS_FLAGS, +} from "./flag-tables"; + +function isProfileBootstrapSubcommand(arg: string): boolean { + return arg === "launch" || arg === "acp"; +} + +function isUnknownLongValueCandidate(arg: string): boolean { + return ( + arg.startsWith("--") && + !arg.includes("=") && + !STRING_VALUE_FLAGS.has(arg) && + !OPTIONAL_VALUE_FLAGS.has(arg) && + !VALUELESS_FLAGS.has(arg) + ); +} + +function needsBoundaryAfterGlobalStrip(stripped: readonly string[]): boolean { + const previous = stripped[stripped.length - 1]; + return ( + previous !== undefined && + (OPTIONAL_VALUE_FLAGS.has(previous) || + EXTENSION_SHADOWABLE_STRING_FLAGS.has(previous) || + isUnknownLongValueCandidate(previous)) + ); +} + +export interface ProfileBootstrapResult { + argv: string[]; + profile?: string; + aliasName?: string; +} + +/** + * Strip `--profile` / `--alias` from argv while preserving the surrounding + * argument structure, returning the residual argv to hand to the launch parser + * and the captured flag values. + * + * Global flag extraction stops only when the first residual argv token names a + * registered command that owns its own flags (e.g. `grep`): everything from + * that token onward is forwarded verbatim so a subcommand's own flags and + * positionals are never stolen (`omp grep --profile ` greps for + * `--profile`; it does not select a profile). `launch` and `acp` are explicit + * spellings of launch-shaped commands, so `omp launch --profile work` and + * `omp acp --profile work` still select profile `work`. + * + * Throws when either flag is supplied without a value. + */ +export function extractProfileFlags(argv: readonly string[]): ProfileBootstrapResult { + const stripped: string[] = []; + let profile: string | undefined; + let aliasName: string | undefined; + let passThrough = false; + let sawSubcommand = false; + let canDispatchSubcommand = true; + let insertBoundaryBeforeNextValue = false; + for (let index = 0; index < argv.length; index += 1) { + const arg = argv[index]; + + if (passThrough || sawSubcommand) { + stripped.push(arg); + continue; + } + + if (insertBoundaryBeforeNextValue) { + if (!arg.startsWith("-")) { + stripped.push(PROFILE_BOOTSTRAP_BOUNDARY_ARG); + } + insertBoundaryBeforeNextValue = false; + } + + // `--` ends option processing. Anything that follows is forwarded verbatim + // so users can pass arbitrary tokens (including a literal `--profile`) to + // downstream tools without the bootstrap stealing them. + if (arg === "--") { + passThrough = true; + stripped.push(arg); + continue; + } + + if (arg === "--profile") { + const value = argv[index + 1]; + if (!value || value.startsWith("-")) { + throw new Error("--profile requires a profile name"); + } + profile = value; + insertBoundaryBeforeNextValue = needsBoundaryAfterGlobalStrip(stripped); + index += 1; + continue; + } + if (arg.startsWith("--profile=")) { + const value = arg.slice("--profile=".length); + if (!value) { + throw new Error("--profile requires a profile name"); + } + profile = value; + insertBoundaryBeforeNextValue = needsBoundaryAfterGlobalStrip(stripped); + continue; + } + if (arg === "--alias") { + const value = argv[index + 1]; + if (!value || value.startsWith("-")) { + throw new Error("--alias requires a command name"); + } + aliasName = value; + insertBoundaryBeforeNextValue = needsBoundaryAfterGlobalStrip(stripped); + index += 1; + continue; + } + if (arg.startsWith("--alias=")) { + const value = arg.slice("--alias=".length); + if (!value) { + throw new Error("--alias requires a command name"); + } + aliasName = value; + insertBoundaryBeforeNextValue = needsBoundaryAfterGlobalStrip(stripped); + continue; + } + + // Known string flags normally consume flag-looking values (for example + // `--system-prompt --profile foo` means the system prompt is literally + // `--profile`). A small allow-list of built-ins can be shadowed by boolean + // extensions before extension metadata is loaded; those mirror extension + // consumption here so `--plan --profile work` still activates `work`. + if (EXTENSION_SHADOWABLE_STRING_FLAGS.has(arg)) { + canDispatchSubcommand = false; + stripped.push(arg); + const next = argv[index + 1]; + if (next !== undefined && !next.startsWith("-")) { + stripped.push(next); + index += 1; + } + continue; + } + + // Forward both the flag and its value untouched so the downstream parser + // gets exactly what the user typed. Critical for `--system-prompt + // --profile foo`: the bootstrap must NOT interpret `--profile` here, it + // belongs to `--system-prompt`. + if (STRING_VALUE_FLAGS.has(arg)) { + canDispatchSubcommand = false; + stripped.push(arg); + if (index + 1 < argv.length) { + stripped.push(argv[index + 1]); + index += 1; + } + continue; + } + + if (OPTIONAL_VALUE_FLAGS.has(arg)) { + canDispatchSubcommand = false; + stripped.push(arg); + const config = OPTIONAL_FLAGS[arg]; + const next = argv[index + 1]; + if (next !== undefined && !next.startsWith("-") && !(config.rejectEmpty === true && next.length === 0)) { + stripped.push(next); + index += 1; + } + continue; + } + + // An unclassified bare long option (`--xxx` with no `=`) may be an extension + // string flag that consumes the next token as its value. The bootstrap runs + // before extensions load, so it cannot consult the extension flag table; it + // therefore mirrors the value-consumption rule `parseArgs` applies to + // extension flags (./args.ts): a string extension flag consumes its successor + // ONLY when that successor is value-like (does not start with `-`), and a + // boolean extension flag consumes nothing. So protect (forward + skip) the + // successor only when it is value-like — `omp --bar val --profile work` keeps + // `val` with `--bar` and still extracts the trailing profile — and otherwise + // forward just the flag, letting the loop process a flag-looking successor so + // a trailing global flag still applies (`omp --some-ext-bool --profile work` + // selects profile `work`). A `--` successor is deliberately NOT protected + // here: it falls through to the end-of-options arm above, keeping `--` a + // single, consistent meaning instead of being swallowed as a flag value. + // Known value-less launch flags are exempt so a trailing profile still + // activates (`omp --print --profile work`). + if (isUnknownLongValueCandidate(arg)) { + canDispatchSubcommand = false; + stripped.push(arg); + const next = argv[index + 1]; + if (next !== undefined && !next.startsWith("-")) { + stripped.push(next); + index += 1; + } + continue; + } + + // Only the first residual argv token can be the dispatched subcommand. Once + // any other token has been forwarded, later subcommand names are launch text. + // `launch` and `acp` are explicit spellings of launch-shaped commands, so + // global launch flags that follow them must still be extracted. + if (canDispatchSubcommand && isSubcommand(arg) && !isProfileBootstrapSubcommand(arg)) { + sawSubcommand = true; + } + canDispatchSubcommand = false; + stripped.push(arg); + } + + return { argv: stripped, profile, aliasName }; +} diff --git a/packages/coding-agent/src/commands/launch.ts b/packages/coding-agent/src/commands/launch.ts index bb515e56a..b18c4efc8 100644 --- a/packages/coding-agent/src/commands/launch.ts +++ b/packages/coding-agent/src/commands/launch.ts @@ -49,6 +49,12 @@ export default class Index extends Command { "allow-home": Flags.boolean({ description: "Allow starting in ~ without auto-switching to a temp dir", }), + profile: Flags.string({ + description: "Use an isolated profile for auth, sessions, settings, and caches", + }), + alias: Flags.string({ + description: "Create a shell shortcut for the selected profile and exit", + }), cwd: Flags.string({ description: "Directory to start in (overrides the launch cwd)", }), @@ -151,6 +157,7 @@ export default class Index extends Command { `# Include files in initial message\n ${APP_NAME} @prompt.md @image.png "What color is the sky?"`, `# Non-interactive mode (process and exit)\n ${APP_NAME} -p "List all .ts files in src/"`, `# Continue previous session\n ${APP_NAME} --continue "What did we discuss?"`, + `# Create a shell shortcut for a work profile\n ${APP_NAME} --profile work --alias omp-work`, `# Use different model (fuzzy matching)\n ${APP_NAME} --model opus "Help me refactor this code"`, `# Limit model cycling to specific models\n ${APP_NAME} --models claude-sonnet,claude-haiku,gpt-4o`, `# Export a session file to HTML\n ${APP_NAME} --export ~/.omp/agent/sessions/--path--/session.jsonl`, diff --git a/packages/coding-agent/src/config/mcp-schema.json b/packages/coding-agent/src/config/mcp-schema.json index 5cdd7ef88..05a669915 100644 --- a/packages/coding-agent/src/config/mcp-schema.json +++ b/packages/coding-agent/src/config/mcp-schema.json @@ -89,6 +89,10 @@ }, "callbackPath": { "type": "string" + }, + "prompt": { + "type": "string", + "description": "OAuth `prompt` parameter sent during authorization (default: \"consent\" so the provider always shows its account/consent screen; set to \"\" to omit)." } }, "description": "Explicit OAuth client settings for servers that need them during /mcp reauth or initial connect." diff --git a/packages/coding-agent/src/discovery/builtin.ts b/packages/coding-agent/src/discovery/builtin.ts index 060c5de65..94a17e2bc 100644 --- a/packages/coding-agent/src/discovery/builtin.ts +++ b/packages/coding-agent/src/discovery/builtin.ts @@ -4,7 +4,7 @@ * Primary provider for OMP native configs. Supports all capabilities. */ import * as path from "node:path"; -import { logger, parseFrontmatter, tryParseJson } from "@oh-my-pi/pi-utils"; +import { getAgentDir, logger, parseFrontmatter, tryParseJson } from "@oh-my-pi/pi-utils"; import { YAML } from "bun"; import { getManagedSkillsDir, MANAGED_SKILLS_PROVIDER_ID } from "../autolearn/managed-skills"; import { registerProvider } from "../capability"; @@ -61,7 +61,9 @@ async function getConfigDirs(ctx: LoadContext): Promise/agent), like sessions and MCP. + const userDir = await ifNonEmptyDir(getAgentDir()); if (userDir) { result.push({ dir: userDir, level: "user" }); } @@ -178,6 +180,7 @@ async function loadMCPServers(ctx: LoadContext): Promise> redirectUri?: string; callbackPort?: number; callbackPath?: string; + prompt?: string; } | undefined, transport: serverConfig.type as "stdio" | "sse" | "http" | undefined, @@ -187,11 +190,14 @@ async function loadMCPServers(ctx: LoadContext): Promise> return result; }; + // User scope tracks the active profile via getAgentDir() (not ctx.home), so it + // stays in sync with getMCPConfigPath("user") and the /mcp config writer. + const userAgentDir = getAgentDir(); const paths = [ { path: path.join(ctx.cwd, PATHS.projectDir, "mcp.json"), level: "project" as const }, { path: path.join(ctx.cwd, PATHS.projectDir, ".mcp.json"), level: "project" as const }, - { path: path.join(ctx.home, PATHS.userAgent, "mcp.json"), level: "user" as const }, - { path: path.join(ctx.home, PATHS.userAgent, ".mcp.json"), level: "user" as const }, + { path: path.join(userAgentDir, "mcp.json"), level: "user" as const }, + { path: path.join(userAgentDir, ".mcp.json"), level: "user" as const }, ]; const contents = await Promise.allSettled( @@ -226,7 +232,7 @@ registerProvider(mcpCapability.id, { async function loadSystemPrompt(ctx: LoadContext): Promise> { const items: SystemPrompt[] = []; - const userPath = path.join(ctx.home, PATHS.userAgent, "SYSTEM.md"); + const userPath = path.join(getAgentDir(), "SYSTEM.md"); const userContent = await readFile(userPath); if (userContent) { items.push({ @@ -277,7 +283,7 @@ async function loadSkills(ctx: LoadContext): Promise> { // User-level scan from ~/.omp/agent/skills/ const userScan = scanSkillsFromDir(ctx, { - dir: path.join(ctx.home, PATHS.userAgent, "skills"), + dir: path.join(getAgentDir(), "skills"), providerId: PROVIDER_ID, level: "user", requireDescription: true, @@ -297,7 +303,7 @@ async function loadSkills(ctx: LoadContext): Promise> { const MANAGED_SKILLS_PRIORITY = 5; async function loadManagedSkills(ctx: LoadContext): Promise> { return scanSkillsFromDir(ctx, { - dir: getManagedSkillsDir(ctx.home), + dir: getManagedSkillsDir(), providerId: MANAGED_SKILLS_PROVIDER_ID, level: "user", requireDescription: true, @@ -373,7 +379,7 @@ async function loadRules(ctx: LoadContext): Promise> { // the current turn so they keep hold across long conversations". // User scope: ~/.omp/agent/RULES.md // Project scope: nearest .omp/RULES.md walking up from cwd to repoRoot - const userRulesFile = path.join(ctx.home, PATHS.userAgent, "RULES.md"); + const userRulesFile = path.join(getAgentDir(), "RULES.md"); const userRule = await loadStickyRulesFile(userRulesFile, "user"); if (userRule) items.push(userRule); @@ -890,7 +896,7 @@ async function loadContextFiles(ctx: LoadContext): Promise + subEntries.some(e => e.name === name && (e.isFile() || e.isSymbolicLink())); + if (hasEntry("package.json")) packageJsonFiles.push({ path: `${entry.name}/package.json` }); + if (hasEntry("index.ts")) indexFiles.push({ path: `${entry.name}/index.ts` }); + else if (hasEntry("index.js")) indexFiles.push({ path: `${entry.name}/index.js` }); + } + // Process direct files for (const match of directFiles) { if (match.path.includes("/")) continue; diff --git a/packages/coding-agent/src/discovery/mcp-json.ts b/packages/coding-agent/src/discovery/mcp-json.ts index 9fa675711..fe5de944b 100644 --- a/packages/coding-agent/src/discovery/mcp-json.ts +++ b/packages/coding-agent/src/discovery/mcp-json.ts @@ -46,6 +46,7 @@ interface MCPConfigFile { redirectUri?: string; callbackPort?: number; callbackPath?: string; + prompt?: string; }; } >; diff --git a/packages/coding-agent/src/discovery/omp-extension-roots.ts b/packages/coding-agent/src/discovery/omp-extension-roots.ts index f4aa7b801..a16bb8055 100644 --- a/packages/coding-agent/src/discovery/omp-extension-roots.ts +++ b/packages/coding-agent/src/discovery/omp-extension-roots.ts @@ -17,7 +17,7 @@ */ import * as fs from "node:fs/promises"; import * as path from "node:path"; -import { isEnoent, logger, tryParseJson } from "@oh-my-pi/pi-utils"; +import { getAgentDir, isEnoent, logger, tryParseJson } from "@oh-my-pi/pi-utils"; import { readDirEntries, readFile } from "../capability/fs"; import type { LoadContext } from "../capability/types"; import { getEnabledPlugins } from "../extensibility/plugins/loader"; @@ -82,7 +82,7 @@ interface ScopeDirs { function scopeDirs(ctx: LoadContext): ScopeDirs { return { project: path.join(ctx.cwd, ".omp"), - user: path.join(ctx.home, ".omp", "agent"), + user: getAgentDir(), }; } diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index 72e8aab1b..74a7c0943 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -27,7 +27,12 @@ import { unsubscribeFromResources, } from "./client"; import { loadAllMCPConfigs, validateServerConfig } from "./config"; -import { refreshMCPOAuthToken } from "./oauth-flow"; +import { + lookupMcpOAuthCredential, + type MCPOAuthCredentialLookup, + selectMcpOAuthRefreshMaterial, +} from "./oauth-credentials"; +import { type MCPStoredOAuthCredential, refreshMCPOAuthToken } from "./oauth-flow"; import type { MCPToolDetails } from "./tool-bridge"; import { DeferredMCPTool, MCPTool } from "./tool-bridge"; import type { MCPToolCache } from "./tool-cache"; @@ -400,9 +405,15 @@ export class MCPManager { } // Wire auth refresh for HTTP transports so 401s trigger token refresh. - if (connection.transport instanceof HttpTransport && config.auth?.type === "oauth") { + // Gate on a resolvable managed credential, not on the auth block: + // definition-only configs (url-keyed fallback) get Bearer injection + // too and need the same mid-session refresh hook. + if ( + connection.transport instanceof HttpTransport && + lookupMcpOAuthCredential(this.#authStorage, config) + ) { connection.transport.onAuthError = async () => { - const refreshed = await this.#resolveAuthConfig(config, true); + const refreshed = await this.#resolveAuthConfig(config, { forceRefresh: true }); if (refreshed.type === "http" || refreshed.type === "sse") { return refreshed.headers ?? null; } @@ -673,9 +684,11 @@ export class MCPManager { /** * Resolve auth and shell-command substitutions in config before connecting. + * Pass `oauth: false` to skip OAuth credential injection (used by reauth's + * unauthenticated probe, which must observe the server's bare 401). */ - async prepareConfig(config: MCPServerConfig): Promise { - return this.#resolveAuthConfig(config); + async prepareConfig(config: MCPServerConfig, options?: { oauth?: boolean }): Promise { + return this.#resolveAuthConfig(config, options); } /** @@ -925,9 +938,10 @@ export class MCPManager { this.#connections.set(name, connection); // Wire auth refresh for HTTP transports, and reconnect for any transport. - if (connection.transport instanceof HttpTransport && config.auth?.type === "oauth") { + // Same gate as connectServers: any resolvable managed credential. + if (connection.transport instanceof HttpTransport && lookupMcpOAuthCredential(this.#authStorage, config)) { connection.transport.onAuthError = async () => { - const refreshed = await this.#resolveAuthConfig(config, true); + const refreshed = await this.#resolveAuthConfig(config, { forceRefresh: true }); if (refreshed.type === "http" || refreshed.type === "sse") { return refreshed.headers ?? null; } @@ -1169,78 +1183,101 @@ export class MCPManager { /** * Resolve OAuth credentials and shell commands in config. + * `oauth: false` skips credential injection (reauth's unauthenticated probe); + * `forceRefresh` bypasses the expiry buffer (401/403 auth-error hook). */ - async #resolveAuthConfig(config: MCPServerConfig, forceRefresh = false): Promise { + async #resolveAuthConfig( + config: MCPServerConfig, + opts?: { forceRefresh?: boolean; oauth?: boolean }, + ): Promise { let resolved: MCPServerConfig = { ...config }; const auth = config.auth; - if (auth?.type === "oauth" && auth.credentialId && this.#authStorage) { - const credentialId = auth.credentialId; + const lookup: MCPOAuthCredentialLookup | undefined = + opts?.oauth !== false ? lookupMcpOAuthCredential(this.#authStorage, config) : undefined; + if (lookup && this.#authStorage) { + const { credentialId } = lookup; try { - let credential = this.#authStorage.get(credentialId); - if (credential?.type === "oauth") { - // Proactive refresh: 5-minute buffer before expiry - // Force refresh: on 401/403 auth errors (revoked tokens, clock skew, missing expires) - const REFRESH_BUFFER_MS = 5 * 60_000; - const shouldRefresh = - forceRefresh || (credential.expires && Date.now() >= credential.expires - REFRESH_BUFFER_MS); - if (shouldRefresh && credential.refresh && auth.tokenUrl) { - const resource = - auth.resource ?? (config.type === "http" || config.type === "sse" ? config.url : undefined); - try { - const refreshed = await refreshMCPOAuthToken( - auth.tokenUrl, - credential.refresh, - auth.clientId, - auth.clientSecret, - resource, - ); - const refreshedCredential = { type: "oauth" as const, ...refreshed }; - await this.#authStorage.set(credentialId, refreshedCredential); - credential = refreshedCredential; - } catch (refreshError) { - const errorMsg = refreshError instanceof Error ? refreshError.message : String(refreshError); - if (isDefinitiveOAuthFailure(errorMsg)) { - // `invalid_grant` / `invalid_token` / 401 from the token endpoint means - // the server has retired this credential — keeping the stale access - // token would just re-fail with 401 on every MCP request and leave a - // poisoned row in agent.db that survives restarts. Drop it now so the - // next connect attempt surfaces a clean "needs reauth" failure and - // the user can recover with `/mcp reauth ` (or `/mcp unauth` - // to forget the server entirely). - logger.warn("MCP OAuth refresh failed definitively; cleared credential", { - credentialId, - error: errorMsg, - }); - await this.#authStorage.remove(credentialId); - credential = undefined; - } else { - logger.warn("MCP OAuth refresh failed, using existing token", { - credentialId, - error: refreshError, - }); - } + let credential: MCPStoredOAuthCredential | undefined = lookup.credential; + // Refresh material comes from ONE source: the credential's embedded + // fields (written atomically with the tokens they minted — tokenUrl + // always present) or, for legacy rows that predate embedding, the + // config auth block. Never mix the two: a shared file's auth block + // can belong to another profile, whose client the grant is NOT + // bound to. + const material = selectMcpOAuthRefreshMaterial(credential, auth); + const tokenUrl = material?.tokenUrl; + const clientId = material?.clientId; + const clientSecret = material?.clientSecret; + const resource = + material?.resource ?? (config.type === "http" || config.type === "sse" ? config.url : undefined); + // Proactive refresh: 5-minute buffer before expiry + // Force refresh: on 401/403 auth errors (revoked tokens, clock skew, missing expires) + const REFRESH_BUFFER_MS = 5 * 60_000; + const shouldRefresh = + opts?.forceRefresh || (credential.expires && Date.now() >= credential.expires - REFRESH_BUFFER_MS); + if (shouldRefresh && credential.refresh && tokenUrl) { + try { + const refreshed = await refreshMCPOAuthToken( + tokenUrl, + credential.refresh, + clientId, + clientSecret, + resource, + ); + // Spread the old credential first so embedded refresh material survives rotation. + const refreshedCredential: MCPStoredOAuthCredential = { + ...credential, + ...refreshed, + tokenUrl, + clientId, + clientSecret, + resource, + }; + await this.#authStorage.set(credentialId, refreshedCredential); + credential = refreshedCredential; + } catch (refreshError) { + const errorMsg = refreshError instanceof Error ? refreshError.message : String(refreshError); + if (isDefinitiveOAuthFailure(errorMsg)) { + // `invalid_grant` / `invalid_token` / 401 from the token endpoint means + // the server has retired this credential — keeping the stale access + // token would just re-fail with 401 on every MCP request and leave a + // poisoned row in agent.db that survives restarts. Drop it now so the + // next connect attempt surfaces a clean "needs reauth" failure and + // the user can recover with `/mcp reauth ` (or `/mcp unauth` + // to forget the server entirely). + logger.warn("MCP OAuth refresh failed definitively; cleared credential", { + credentialId, + error: errorMsg, + }); + await this.#authStorage.remove(credentialId); + credential = undefined; + } else { + logger.warn("MCP OAuth refresh failed, using existing token", { + credentialId, + error: refreshError, + }); } } + } - if (credential?.type === "oauth") { - if (resolved.type === "http" || resolved.type === "sse") { - resolved = { - ...resolved, - headers: { - ...resolved.headers, - Authorization: `Bearer ${credential.access}`, - }, - }; - } else { - resolved = { - ...resolved, - env: { - ...resolved.env, - OAUTH_ACCESS_TOKEN: credential.access, - }, - }; - } + if (credential) { + if (resolved.type === "http" || resolved.type === "sse") { + resolved = { + ...resolved, + headers: { + ...resolved.headers, + Authorization: `Bearer ${credential.access}`, + }, + }; + } else { + resolved = { + ...resolved, + env: { + ...resolved.env, + OAUTH_ACCESS_TOKEN: credential.access, + }, + }; } } } catch (error) { diff --git a/packages/coding-agent/src/mcp/oauth-credentials.ts b/packages/coding-agent/src/mcp/oauth-credentials.ts new file mode 100644 index 000000000..f1d0a5b46 --- /dev/null +++ b/packages/coding-agent/src/mcp/oauth-credentials.ts @@ -0,0 +1,96 @@ +import { expandEnvVarsDeep } from "../discovery/helpers"; +import type { AuthStorage } from "../session/auth-storage"; +import { isManagedMCPOAuthCredentialId, type MCPStoredOAuthCredential, mcpOAuthCredentialId } from "./oauth-flow"; +import type { MCPAuthConfig, MCPServerConfig } from "./types"; + +export interface MCPOAuthCredentialLookup { + credentialId: string; + credential: MCPStoredOAuthCredential; +} + +export type MCPOAuthRefreshMaterial = MCPStoredOAuthCredential | MCPAuthConfig | undefined; + +export function mcpOAuthCredentialIdsForServerUrl(serverUrl: string | undefined): string[] { + if (!serverUrl) return []; + const ids: string[] = []; + for (const url of [expandEnvVarsDeep(serverUrl), serverUrl]) { + const id = mcpOAuthCredentialId(url); + if (!ids.includes(id)) ids.push(id); + } + return ids; +} + +export function hasMcpAuthorizationHeader(config: MCPServerConfig): boolean { + if (config.type !== "http" && config.type !== "sse") return false; + return Object.keys(config.headers ?? {}).some(header => header.toLowerCase() === "authorization"); +} + +export function lookupMcpOAuthCredentialForServer( + authStorage: AuthStorage | null | undefined, + auth: MCPAuthConfig | undefined, + serverUrl: string | undefined, + options: { allowUrlKeyedFallback?: boolean } = {}, +): MCPOAuthCredentialLookup | undefined { + if (!authStorage) return undefined; + if (auth && auth.type !== "oauth") return undefined; + const urlKeyedCredentialIds = mcpOAuthCredentialIdsForServerUrl(serverUrl); + if ( + auth?.credentialId && + (!auth.credentialId.startsWith("mcp_oauth:profile:") || urlKeyedCredentialIds.includes(auth.credentialId)) + ) { + const credential = authStorage.get(auth.credentialId); + if (credential?.type === "oauth") { + return { credentialId: auth.credentialId, credential }; + } + } + if (options.allowUrlKeyedFallback === false) return undefined; + for (const credentialId of urlKeyedCredentialIds) { + const credential = authStorage.get(credentialId); + if (credential?.type === "oauth") { + return { credentialId, credential }; + } + } + return undefined; +} + +export function lookupMcpOAuthCredential( + authStorage: AuthStorage | null | undefined, + config: MCPServerConfig, +): MCPOAuthCredentialLookup | undefined { + const auth = config.auth; + if (config.type !== "http" && config.type !== "sse") { + return lookupMcpOAuthCredentialForServer(authStorage, auth, undefined); + } + if (hasMcpAuthorizationHeader(config)) { + return lookupMcpOAuthCredentialForServer(authStorage, auth, config.url, { allowUrlKeyedFallback: false }); + } + return lookupMcpOAuthCredentialForServer(authStorage, auth, config.url); +} + +export function selectMcpOAuthRefreshMaterial( + credential: MCPStoredOAuthCredential, + auth: MCPAuthConfig | undefined, +): MCPOAuthRefreshMaterial { + return credential.tokenUrl ? credential : auth; +} + +export async function removeManagedMcpOAuthCredential( + authStorage: AuthStorage, + credentialId: string | undefined, +): Promise { + if (!isManagedMCPOAuthCredentialId(credentialId)) return false; + if (authStorage.get(credentialId)?.type !== "oauth") return false; + await authStorage.remove(credentialId); + return true; +} + +export async function removeManagedMcpOAuthCredentials( + authStorage: AuthStorage, + credentialIds: readonly (string | undefined)[], +): Promise { + let removed = false; + for (const credentialId of credentialIds) { + removed = (await removeManagedMcpOAuthCredential(authStorage, credentialId)) || removed; + } + return removed; +} diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index 1c2dab383..7954e6f1c 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -9,6 +9,44 @@ import type { OAuthCallbackFlowOptions } from "@oh-my-pi/pi-ai/oauth/callback-se import { OAuthCallbackFlow } from "@oh-my-pi/pi-ai/oauth/callback-server"; import type { OAuthController, OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types"; import type { FetchImpl } from "@oh-my-pi/pi-ai/types"; +import { getActiveProfile } from "@oh-my-pi/pi-utils/dirs"; +import type { OAuthCredential } from "../session/auth-storage"; + +/** Credential-id prefix for OMP-managed MCP OAuth credentials keyed by profile and server URL. */ +const MCP_OAUTH_URL_CREDENTIAL_PREFIX = "mcp_oauth:"; + +/** + * Deterministic credential id for an MCP server URL scoped to an OMP profile. + * + * Local profile stores are already separate, but auth-broker storage shares one + * provider namespace across profiles. Including the profile in the provider key + * keeps a shared project `mcp.json` definition from making profile B overwrite + * or read profile A's OAuth row for the same server URL. The URL is used + * verbatim (query string included) because it can carry tenant selectors such + * as `?project_ref=`. + */ +export function mcpOAuthCredentialId(serverUrl: string, profile: string | undefined = getActiveProfile()): string { + return `${MCP_OAUTH_URL_CREDENTIAL_PREFIX}profile:${profile ?? "default"}:${serverUrl}`; +} + +/** Whether a credential id was minted by OMP's MCP OAuth flows (either era). */ +export function isManagedMCPOAuthCredentialId(credentialId: string | undefined): credentialId is string { + return ( + !!credentialId && + (credentialId.startsWith("mcp_oauth_") || credentialId.startsWith(MCP_OAUTH_URL_CREDENTIAL_PREFIX)) + ); +} + +/** + * Stored MCP OAuth credential. Refresh material is embedded so token refresh + * works without any `auth` block persisted in (possibly shared) config files. + */ +export interface MCPStoredOAuthCredential extends OAuthCredential { + tokenUrl?: string; + clientId?: string; + clientSecret?: string; + resource?: string; +} const DEFAULT_PORT = 3000; const CALLBACK_PATH = "/callback"; @@ -126,6 +164,15 @@ export interface MCPOAuthConfig { clientSecret?: string; /** OAuth scopes (space-separated) */ scopes?: string; + /** + * `prompt` parameter for the authorization request. Defaults to `"consent"` + * so the provider always shows its authorize screen instead of silently + * re-approving the browser's current session — without it, reauthorizing to + * switch accounts/workspaces is impossible once a session cookie exists + * (RFC 6749 §3.1 requires servers to ignore the param when unsupported). + * Set to `""` to omit the parameter entirely. + */ + prompt?: string; /** Exact redirect URI to advertise to the provider */ redirectUri?: string; /** Custom callback port (default: 3000) */ @@ -202,6 +249,10 @@ export class MCPOAuthFlow extends OAuthCallbackFlow { if (this.config.scopes && !params.get("scope")) { params.set("scope", this.config.scopes); } + const prompt = this.config.prompt ?? "consent"; + if (prompt && !params.get("prompt")) { + params.set("prompt", prompt); + } const existingResource = params.get("resource")?.trim(); if (existingResource) { this.#resource = resolveResourceUri(existingResource); diff --git a/packages/coding-agent/src/mcp/types.ts b/packages/coding-agent/src/mcp/types.ts index dd7641fa7..ae9af5039 100644 --- a/packages/coding-agent/src/mcp/types.ts +++ b/packages/coding-agent/src/mcp/types.ts @@ -74,6 +74,8 @@ interface MCPServerConfigBase { redirectUri?: string; callbackPort?: number; callbackPath?: string; + /** `prompt` param for the authorization request (default "consent"; "" to omit) */ + prompt?: string; }; } diff --git a/packages/coding-agent/src/modes/components/mcp-add-wizard.ts b/packages/coding-agent/src/modes/components/mcp-add-wizard.ts index 899f0530c..92b49ce4c 100644 --- a/packages/coding-agent/src/modes/components/mcp-add-wizard.ts +++ b/packages/coding-agent/src/modes/components/mcp-add-wizard.ts @@ -49,14 +49,19 @@ type WizardStep = /** * Result of the wizard's OAuth callback. `credentialId` is mandatory; - * `clientId`/`clientSecret` are populated when the OAuth provider performed - * dynamic client registration (or when the caller pre-supplied them) so the - * wizard can fold them into the final `mcp.json` entry for refresh. + * `clientId` is populated when the OAuth provider performed dynamic client + * registration (or when the caller pre-supplied it) so the wizard can fold it + * into the final `mcp.json` entry. Refresh material (including any DCR client + * secret) is embedded in the stored credential, never written to config files. */ export interface MCPAddWizardOAuthResult { credentialId: string; clientId?: string; - clientSecret?: string; + resource?: string; +} + +interface MCPAddWizardOAuthOptions { + serverUrl?: string; resource?: string; } @@ -125,7 +130,7 @@ export class MCPAddWizard extends Container { clientId: string, clientSecret: string, scopes: string, - resource?: string, + options?: MCPAddWizardOAuthOptions, ) => Promise) | null = null; #onTestConnectionCallback: ((config: MCPServerConfig) => Promise) | null = null; @@ -140,7 +145,7 @@ export class MCPAddWizard extends Container { clientId: string, clientSecret: string, scopes: string, - resource?: string, + options?: MCPAddWizardOAuthOptions, ) => Promise, onTestConnection?: (config: MCPServerConfig) => Promise, onRender?: () => void, @@ -1157,14 +1162,16 @@ export class MCPAddWizard extends Container { this.#state.oauthClientId, this.#state.oauthClientSecret, this.#state.oauthScopes, - oauthResource || undefined, + { + serverUrl: this.#state.url || undefined, + resource: oauthResource || undefined, + }, ); - // Store credential ID + any dynamically-registered client credentials, - // so the final mcp.json entry persists everything needed for refresh. + // Store credential ID + any dynamically-registered client id. DCR client + // secrets stay embedded in the stored credential, never in mcp.json. this.#state.oauthCredentialId = oauthResult.credentialId; if (oauthResult.clientId) this.#state.oauthClientId = oauthResult.clientId; - if (oauthResult.clientSecret) this.#state.oauthClientSecret = oauthResult.clientSecret; this.#state.oauthResource = oauthResult.resource ?? oauthResource; // Show success message diff --git a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts index 8bf8ff263..9b32edae8 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -7,6 +7,7 @@ import * as path from "node:path"; import { type Component, replaceTabs, Spacer, Text } from "@oh-my-pi/pi-tui"; import { getMCPConfigPath, getProjectDir } from "@oh-my-pi/pi-utils"; import type { SourceMeta } from "../../capability/types"; +import { expandEnvVarsDeep } from "../../discovery/helpers"; import { analyzeAuthError, discoverOAuthEndpoints, MCPManager } from "../../mcp"; import { connectToServer, disconnectServer, listTools } from "../../mcp/client"; import { @@ -17,7 +18,13 @@ import { setServerDisabled, updateMCPServer, } from "../../mcp/config-writer"; -import { MCPOAuthFlow } from "../../mcp/oauth-flow"; +import { + lookupMcpOAuthCredentialForServer, + mcpOAuthCredentialIdsForServerUrl, + removeManagedMcpOAuthCredential, + removeManagedMcpOAuthCredentials, +} from "../../mcp/oauth-credentials"; +import { MCPOAuthFlow, type MCPStoredOAuthCredential, mcpOAuthCredentialId } from "../../mcp/oauth-flow"; import { clearSmitheryApiKey, createSmitheryCliAuthSession, @@ -34,7 +41,6 @@ import { toConfigName, } from "../../mcp/smithery-registry"; import type { MCPAuthConfig, MCPServerConfig, MCPServerConnection } from "../../mcp/types"; -import type { OAuthCredential } from "../../session/auth-storage"; import { shortenPath } from "../../tools/render-utils"; import { urlHyperlinkAlways } from "../../tui"; import { openPath } from "../../utils/open"; @@ -116,17 +122,16 @@ class McpConnectingBlock extends ChatBlock { /** * Outcome of {@link MCPCommandController}'s OAuth handler. * - * `clientId`/`clientSecret` are populated when the OAuth provider required (or - * accepted) dynamic client registration; callers MUST persist them alongside - * `credentialId` so subsequent token refreshes and reauthorizations can reuse - * the same registered client. Both are also set when the caller pre-supplied a - * client id via the wizard or `oauth.clientId` in `mcp.json`, in which case the - * write-back is a no-op. + * `credentialId` is deterministic per server URL when the URL was supplied, so + * every profile resolves its own credential row under the same id. Refresh + * material (token URL, client id/secret) is embedded in the stored credential; + * the returned `clientId` may be folded into `mcp.json` for pre-auth reuse. + * DCR-issued client secrets stay embedded in the stored credential and are + * deliberately not surfaced here, so they cannot leak into config files. */ interface OAuthFlowResult { credentialId: string; clientId?: string; - clientSecret?: string; resource?: string; } @@ -490,38 +495,28 @@ export class MCPCommandController { } try { - const oauthClientSecret = finalConfig.oauth?.clientSecret ?? ""; const oauthResource = oauth.resource ?? finalConfig.url; const oauthResult = await this.#handleOAuthFlow( oauth.authorizationUrl, oauth.tokenUrl, oauth.clientId ?? finalConfig.oauth?.clientId ?? "", - oauthClientSecret, + finalConfig.oauth?.clientSecret ?? "", oauth.scopes ?? "", - finalConfig.oauth?.callbackPort, - finalConfig.oauth?.callbackPath, - finalConfig.oauth?.redirectUri, - oauthResource, + { + callbackPort: finalConfig.oauth?.callbackPort, + callbackPath: finalConfig.oauth?.callbackPath, + redirectUri: finalConfig.oauth?.redirectUri, + prompt: finalConfig.oauth?.prompt, + serverUrl: finalConfig.url, + resource: oauthResource, + }, ); - const persistedClientId = oauthResult.clientId ?? oauth.clientId ?? finalConfig.oauth?.clientId; - const persistedClientSecret = oauthResult.clientSecret ?? finalConfig.oauth?.clientSecret; - const persistedResource = oauthResult.resource ?? oauthResource; - finalConfig = { - ...finalConfig, - auth: { - type: "oauth", - credentialId: oauthResult.credentialId, - tokenUrl: oauth.tokenUrl, - resource: persistedResource, - clientId: persistedClientId, - clientSecret: persistedClientSecret, - }, - oauth: { - ...finalConfig.oauth, - clientId: persistedClientId ?? finalConfig.oauth?.clientId, - clientSecret: persistedClientSecret ?? finalConfig.oauth?.clientSecret, - }, - }; + finalConfig = this.#persistOAuthResult(finalConfig, oauthResult, { + tokenUrl: oauth.tokenUrl, + resource: oauthResource, + clientId: oauth.clientId, + userClientSecret: finalConfig.oauth?.clientSecret, + }); } catch (oauthError) { this.ctx.showError( `OAuth flow failed for "${parsed.initialName}": ${oauthError instanceof Error ? oauthError.message : String(oauthError)}`, @@ -553,25 +548,8 @@ export class MCPCommandController { done(); this.#handleWizardCancel(); }, - async ( - authUrl: string, - tokenUrl: string, - clientId: string, - clientSecret: string, - scopes: string, - resource?: string, - ) => { - return await this.#handleOAuthFlow( - authUrl, - tokenUrl, - clientId, - clientSecret, - scopes, - undefined, - undefined, - undefined, - resource, - ); + async (authUrl: string, tokenUrl: string, clientId: string, clientSecret: string, scopes: string, options) => { + return await this.#handleOAuthFlow(authUrl, tokenUrl, clientId, clientSecret, scopes, options); }, async (config: MCPServerConfig) => { return await this.#handleTestConnection(config); @@ -598,10 +576,14 @@ export class MCPCommandController { clientId: string, clientSecret: string, scopes: string, - callbackPort?: number, - callbackPath?: string, - redirectUri?: string, - resource?: string, + opts?: { + callbackPort?: number; + callbackPath?: string; + redirectUri?: string; + prompt?: string; + serverUrl?: string; + resource?: string; + }, ): Promise { const authStorage = this.ctx.session.modelRegistry.authStorage; let parsedAuthUrl: URL; @@ -637,10 +619,11 @@ export class MCPCommandController { clientId: resolvedClientId, clientSecret: resolvedClientSecret, scopes: scopes || undefined, - redirectUri, - callbackPort, - callbackPath, - resource, + prompt: opts?.prompt, + redirectUri: opts?.redirectUri, + callbackPort: opts?.callbackPort, + callbackPath: opts?.callbackPath, + resource: opts?.resource, }, { onAuth: (info: { url: string; instructions?: string }) => { @@ -712,22 +695,29 @@ export class MCPCommandController { new Text(theme.fg("success", "✓ Authorization completed in browser."), 1, 0), ]); - // Generate a unique credential ID - const credentialId = `mcp_oauth_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`; + // Deterministic per-URL id: every profile resolves its own credential row + // under the same key, so shared project configs stay profile-isolated. + // Random fallback only for flows that never knew the server URL. + const credentialId = opts?.serverUrl + ? mcpOAuthCredentialId(opts.serverUrl) + : `mcp_oauth_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`; - // Store credentials in auth storage - const oauthCredential: OAuthCredential = { + // Embed refresh material so the credential is self-contained: token + // refresh must work for configs that carry no auth block at all. + const oauthCredential: MCPStoredOAuthCredential = { type: "oauth", ...credentials, + tokenUrl, + clientId: flow.resolvedClientId ?? resolvedClientId, + clientSecret: flow.registeredClientSecret ?? resolvedClientSecret, + resource: flow.resource, }; - // Store under a synthetic provider name await authStorage.set(credentialId, oauthCredential); return { credentialId, clientId: flow.resolvedClientId, - clientSecret: flow.registeredClientSecret, resource: flow.resource, }; } catch (error) { @@ -750,20 +740,52 @@ export class MCPCommandController { } } + /** + * Fold a completed OAuth flow back into a server config. Owns the + * persistence policy in one place: the auth block records the credential + * pointer plus refresh material, the oauth block echoes the client id for + * pre-auth reuse, and only a user-supplied client secret is ever written — + * DCR-issued secrets stay embedded in the stored credential so they cannot + * leak into (possibly shared/committed) config files. + */ + #persistOAuthResult( + config: MCPServerConfig, + result: OAuthFlowResult, + opts: { tokenUrl: string; resource?: string; clientId?: string; userClientSecret?: string }, + ): MCPServerConfig { + const clientId = result.clientId ?? opts.clientId ?? config.oauth?.clientId; + const resource = result.resource ?? opts.resource ?? config.auth?.resource; + return { + ...config, + auth: { + type: "oauth", + credentialId: result.credentialId, + tokenUrl: opts.tokenUrl, + clientId, + clientSecret: opts.userClientSecret, + resource, + }, + oauth: { + ...config.oauth, + clientId, + }, + }; + } + /** * Test connection to an MCP server. * Throws an error if connection fails (used for auto-detection). */ - async #handleTestConnection(config: MCPServerConfig): Promise { + async #handleTestConnection(config: MCPServerConfig, options?: { oauth?: boolean }): Promise { // Create temporary connection using a test name const testName = `test_${Date.now()}`; let resolvedConfig: MCPServerConfig; if (this.ctx.mcpManager) { - resolvedConfig = await this.ctx.mcpManager.prepareConfig(config); + resolvedConfig = await this.ctx.mcpManager.prepareConfig(config, options); } else { const tempManager = new MCPManager(getProjectDir()); tempManager.setAuthStorage(this.ctx.session.modelRegistry.authStorage); - resolvedConfig = await tempManager.prepareConfig(config); + resolvedConfig = await tempManager.prepareConfig(config, options); } const connection = await connectToServer(testName, resolvedConfig); @@ -850,11 +872,6 @@ export class MCPCommandController { }; } - async #removeManagedOAuthCredential(credentialId: string | undefined): Promise { - if (!credentialId?.startsWith("mcp_oauth_")) return; - await this.ctx.session.modelRegistry.authStorage.remove(credentialId); - } - #stripOAuthAuth(config: MCPServerConfig): MCPServerConfig { const next = { ...config } as MCPServerConfig & { auth?: MCPAuthConfig }; delete next.auth; @@ -868,11 +885,26 @@ export class MCPCommandController { scopes?: string; resource?: string; }> { + // Stdio servers manage credentials inside the child process; OMP's OAuth + // flow only applies to http/sse transports. Without this guard the + // unauthenticated preflight below spawns the child, which happily reuses + // its own cached tokens (e.g. mcp-remote's machine-wide ~/.mcp-auth) and + // produces the misleading "reauthorization is not required". + if (config.type !== "http" && config.type !== "sse") { + const remoteUrl = config.args?.find(arg => /^https?:\/\//.test(arg)); + const httpHint = `{ "type": "http", "url": ${JSON.stringify(remoteUrl ?? "")} }`; + const usesMcpRemote = [config.command, ...(config.args ?? [])].some(part => part?.includes("mcp-remote")); + throw new Error( + usesMcpRemote + ? `this server proxies OAuth through mcp-remote, which caches tokens machine-wide in ~/.mcp-auth (shared across every OMP profile). Clear ~/.mcp-auth to force a fresh login, or replace the proxy with ${httpHint} so OMP manages OAuth per profile.` + : `stdio servers manage their own credentials, so OMP has no OAuth to reauthorize. If the service supports OAuth over HTTP, configure it as ${httpHint} instead.`, + ); + } // First test if server actually needs auth by connecting without OAuth let connectionSucceeded = false; let connectionError: Error | undefined; try { - await this.#handleTestConnection(this.#stripOAuthAuth(config)); + await this.#handleTestConnection(this.#stripOAuthAuth(config), { oauth: false }); connectionSucceeded = true; } catch (error) { connectionError = error as Error; @@ -1433,12 +1465,34 @@ export class MCPCommandController { } const currentAuth = (found.config as MCPServerConfig & { auth?: MCPAuthConfig }).auth; - if (found.discovered && currentAuth?.type !== "oauth") { - this.#showMessage(["", theme.fg("muted", `No stored OAuth auth to remove for "${name}".`), ""].join("\n")); - return; - } + const authStorage = this.ctx.session.modelRegistry.authStorage; if (currentAuth?.type === "oauth") { - await this.#removeManagedOAuthCredential(currentAuth.credentialId); + await removeManagedMcpOAuthCredential(authStorage, currentAuth.credentialId); + } + // Also drop this profile's url-keyed binding so the server is truly + // signed out even when the config carries no auth block. Runtime + // discovery expands `${...}` URL values before MCPManager looks up the + // deterministic credential row, so unauth must clear that same key. + let removedUrlKeyedCredential = false; + if ((found.config.type === "http" || found.config.type === "sse") && found.config.url) { + removedUrlKeyedCredential = await removeManagedMcpOAuthCredentials( + authStorage, + mcpOAuthCredentialIdsForServerUrl(found.config.url), + ); + } + + if (found.discovered && currentAuth?.type !== "oauth") { + if (!removedUrlKeyedCredential) { + this.#showMessage( + ["", theme.fg("muted", `No stored OAuth auth to remove for "${name}".`), ""].join("\n"), + ); + return; + } + await this.#reloadMCP(); + this.#showMessage( + ["", theme.fg("success", `- Cleared auth for "${name}" (${found.scope} config)`), ""].join("\n"), + ); + return; } const updated = this.#stripOAuthAuth(found.config); @@ -1472,52 +1526,70 @@ export class MCPCommandController { } const currentAuth = (found.config as MCPServerConfig & { auth?: MCPAuthConfig }).auth; - if (currentAuth?.type === "oauth") { - await this.#removeManagedOAuthCredential(currentAuth.credentialId); - } - + const authStorage = this.ctx.session.modelRegistry.authStorage; const baseConfig = this.#stripOAuthAuth(found.config); - const oauth = await this.#resolveOAuthEndpointsFromServer(baseConfig); - const oauthClientSecret = found.config.oauth?.clientSecret ?? currentAuth?.clientSecret ?? ""; + const runtimeBaseConfig = expandEnvVarsDeep(baseConfig); + // Resolve endpoints first: this fails fast for stdio transports and + // probes http/sse with { oauth: false }, so nothing destructive has + // happened yet if the server turns out not to need (or support) OAuth. + // Use the same env-expanded config shape runtime discovery passes to + // MCPManager; the raw file value may contain `${...}` placeholders. + const oauth = await this.#resolveOAuthEndpointsFromServer(runtimeBaseConfig); + const serverUrl = + runtimeBaseConfig.type === "http" || runtimeBaseConfig.type === "sse" ? runtimeBaseConfig.url : undefined; + // A user-supplied client secret may live in either block (the wizard + // writes it to auth.clientSecret); DCR secrets are embedded in the + // stored credential and never echoed back into config files. + const configuredClientId = found.config.oauth?.clientId ?? currentAuth?.clientId; + const existingCredential = lookupMcpOAuthCredentialForServer(authStorage, currentAuth, serverUrl)?.credential; + const flowClientId = oauth.clientId ?? configuredClientId ?? existingCredential?.clientId ?? ""; + const storedClientSecret = + existingCredential?.clientId === flowClientId ? existingCredential.clientSecret : undefined; + const userClientSecret = found.config.oauth?.clientSecret ?? currentAuth?.clientSecret; + const flowClientSecret = userClientSecret ?? storedClientSecret ?? ""; this.#showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n")); + const currentAuthResource = currentAuth?.resource ? expandEnvVarsDeep(currentAuth.resource) : undefined; const oauthResource = - oauth.resource ?? currentAuth?.resource ?? ("url" in baseConfig ? baseConfig.url : undefined); + oauth.resource ?? currentAuthResource ?? ("url" in runtimeBaseConfig ? runtimeBaseConfig.url : undefined); const oauthResult = await this.#handleOAuthFlow( oauth.authorizationUrl, oauth.tokenUrl, - oauth.clientId ?? found.config.oauth?.clientId ?? "", - oauthClientSecret, + flowClientId, + flowClientSecret, oauth.scopes ?? "", - found.config.oauth?.callbackPort, - found.config.oauth?.callbackPath, - found.config.oauth?.redirectUri, - oauthResource, + { + callbackPort: found.config.oauth?.callbackPort, + callbackPath: found.config.oauth?.callbackPath, + redirectUri: found.config.oauth?.redirectUri, + prompt: found.config.oauth?.prompt, + serverUrl, + resource: oauthResource, + }, ); - const persistedClientId = oauthResult.clientId ?? oauth.clientId ?? found.config.oauth?.clientId; - const persistedClientSecret = oauthResult.clientSecret ?? (oauthClientSecret || undefined); - const persistedResource = oauthResult.resource ?? oauthResource; + // The flow overwrote (or minted) this profile's row; a superseded + // pointer row from the legacy random-id era is now orphaned. GC only + // after success so cancelling the browser step leaves the previous + // session signed in. + if (currentAuth?.type === "oauth" && currentAuth.credentialId !== oauthResult.credentialId) { + await removeManagedMcpOAuthCredential(authStorage, currentAuth.credentialId); + } - const updated: MCPServerConfig = { - ...baseConfig, - auth: { - type: "oauth", - credentialId: oauthResult.credentialId, + // Definition-only entries resolve through the url-keyed binding alone; + // skip the write-back so a committed project mcp.json stays clean. + const urlKeyedId = serverUrl ? mcpOAuthCredentialId(serverUrl) : undefined; + if (currentAuth || oauthResult.credentialId !== urlKeyedId) { + const updated = this.#persistOAuthResult(baseConfig, oauthResult, { tokenUrl: oauth.tokenUrl, - resource: persistedResource, - clientId: persistedClientId, - clientSecret: persistedClientSecret, - }, - oauth: { - ...found.config.oauth, - clientId: persistedClientId ?? found.config.oauth?.clientId, - clientSecret: persistedClientSecret ?? found.config.oauth?.clientSecret, - }, - }; - await updateMCPServer(found.filePath, name, updated); + clientId: oauth.clientId, + userClientSecret, + resource: oauthResource, + }); + await updateMCPServer(found.filePath, name, updated); + } await this.#reloadMCP(); const state = await this.#waitForServerConnectionWithAnimation(name); diff --git a/packages/coding-agent/src/tools/builtin-names.ts b/packages/coding-agent/src/tools/builtin-names.ts new file mode 100644 index 000000000..f7acebb6d --- /dev/null +++ b/packages/coding-agent/src/tools/builtin-names.ts @@ -0,0 +1,35 @@ +export const BUILTIN_TOOL_NAMES = [ + "read", + "bash", + "edit", + "ast_grep", + "ast_edit", + "render_mermaid", + "ask", + "debug", + "eval", + "ssh", + "github", + "find", + "search", + "lsp", + "inspect_image", + "browser", + "checkpoint", + "rewind", + "task", + "job", + "irc", + "todo", + "web_search", + "search_tool_bm25", + "write", + "memory_edit", + "retain", + "recall", + "reflect", + "learn", + "manage_skill", +] as const; + +export type BuiltinToolName = (typeof BUILTIN_TOOL_NAMES)[number]; diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index 6b5256433..f0a588395 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -37,6 +37,7 @@ import { AstEditTool } from "./ast-edit"; import { AstGrepTool } from "./ast-grep"; import { BashTool } from "./bash"; import { BrowserTool } from "./browser"; +import type { BuiltinToolName } from "./builtin-names"; import { type CheckpointState, CheckpointTool, RewindTool } from "./checkpoint"; import { DebugTool } from "./debug"; import { EvalTool } from "./eval"; @@ -413,7 +414,7 @@ export function filterInitialToolsForDiscoveryAll( * Public callable factory map. External callers may invoke `BUILTIN_TOOLS.read(session)` or * `BUILTIN_TOOLS[name](session)` to construct a tool directly. */ -export const BUILTIN_TOOLS: Record = { +export const BUILTIN_TOOLS: Record = { read: s => new ReadTool(s), bash: s => new BashTool(s), edit: s => new EditTool(s), @@ -455,7 +456,7 @@ export const HIDDEN_TOOLS: Record = { goal: s => new GoalTool(s), }; -export type ToolName = keyof typeof BUILTIN_TOOLS; +export type ToolName = BuiltinToolName; /** * Create tools from BUILTIN_TOOLS registry. diff --git a/packages/coding-agent/test/agent-session-concurrent.test.ts b/packages/coding-agent/test/agent-session-concurrent.test.ts index 588287564..cf7504b18 100644 --- a/packages/coding-agent/test/agent-session-concurrent.test.ts +++ b/packages/coding-agent/test/agent-session-concurrent.test.ts @@ -153,6 +153,7 @@ describe("AgentSession concurrent prompt guard", () => { expect(session.queuedMessageCount).toBe(1); // Cleanup + session.agent.clearAllQueues(); await session.abort(); await firstPrompt.catch(() => {}); }); @@ -169,6 +170,7 @@ describe("AgentSession concurrent prompt guard", () => { expect(session.queuedMessageCount).toBe(1); // Cleanup + session.agent.clearAllQueues(); await session.abort(); await firstPrompt.catch(() => {}); }); diff --git a/packages/coding-agent/test/autolearn-discovery.test.ts b/packages/coding-agent/test/autolearn-discovery.test.ts index caf6ce952..b1541a31f 100644 --- a/packages/coding-agent/test/autolearn-discovery.test.ts +++ b/packages/coding-agent/test/autolearn-discovery.test.ts @@ -5,6 +5,7 @@ import * as path from "node:path"; import { getManagedSkillsDir } from "@oh-my-pi/pi-coding-agent/autolearn/managed-skills"; import "@oh-my-pi/pi-coding-agent/discovery"; import { loadSkills } from "@oh-my-pi/pi-coding-agent/extensibility/skills"; +import { getAgentDir, setAgentDir } from "@oh-my-pi/pi-utils/dirs"; async function writeSkill(dir: string, name: string, description: string): Promise { const file = path.join(dir, name, "SKILL.md"); @@ -18,13 +19,16 @@ describe("managed-skills discovery", () => { let managedDir: string; let authoredDir: string; + let originalAgentDir: string; beforeEach(async () => { + originalAgentDir = getAgentDir(); tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-managed-disco-home-")); // cwd MUST live under the fake home so loadSkills' ancestor walk is bounded // and cannot pick up ambient /tmp/.omp or /.omp fixtures (full-suite-safe). tempCwd = path.join(tempHome, "work"); await fs.mkdir(tempCwd, { recursive: true }); spyOn(os, "homedir").mockReturnValue(tempHome); + setAgentDir(path.join(tempHome, ".omp", "agent")); managedDir = getManagedSkillsDir(); // Authored user skills live in the sibling `skills/` dir under .../agent. authoredDir = path.join(path.dirname(managedDir), "skills"); @@ -32,6 +36,7 @@ describe("managed-skills discovery", () => { afterEach(async () => { spyOn(os, "homedir").mockRestore(); + setAgentDir(originalAgentDir); await fs.rm(tempHome, { recursive: true, force: true }); }); diff --git a/packages/coding-agent/test/autolearn-managed-skills.test.ts b/packages/coding-agent/test/autolearn-managed-skills.test.ts index 292051780..7ce8453e4 100644 --- a/packages/coding-agent/test/autolearn-managed-skills.test.ts +++ b/packages/coding-agent/test/autolearn-managed-skills.test.ts @@ -11,17 +11,22 @@ import { writeManagedSkill, } from "@oh-my-pi/pi-coding-agent/autolearn/managed-skills"; import { parseFrontmatter } from "@oh-my-pi/pi-utils"; +import { getAgentDir, setAgentDir } from "@oh-my-pi/pi-utils/dirs"; describe("managed-skills primitives", () => { let tempHome: string; + let originalAgentDir: string; beforeEach(async () => { + originalAgentDir = getAgentDir(); tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-managed-skills-")); spyOn(os, "homedir").mockReturnValue(tempHome); + setAgentDir(path.join(tempHome, ".omp", "agent")); }); afterEach(async () => { spyOn(os, "homedir").mockRestore(); + setAgentDir(originalAgentDir); await fs.rm(tempHome, { recursive: true, force: true }); }); diff --git a/packages/coding-agent/test/autolearn-tools-gating.test.ts b/packages/coding-agent/test/autolearn-tools-gating.test.ts index e7adfc55a..04c4725b5 100644 --- a/packages/coding-agent/test/autolearn-tools-gating.test.ts +++ b/packages/coding-agent/test/autolearn-tools-gating.test.ts @@ -10,6 +10,7 @@ import type { MnemopiSessionState } from "@oh-my-pi/pi-coding-agent/mnemopi/stat import { createTools, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { LearnTool } from "@oh-my-pi/pi-coding-agent/tools/learn"; import { ManageSkillTool } from "@oh-my-pi/pi-coding-agent/tools/manage-skill"; +import { getAgentDir, setAgentDir } from "@oh-my-pi/pi-utils/dirs"; function makeSession( settingsOverrides: Partial> = {}, @@ -104,14 +105,18 @@ describe("autolearn tool gating", () => { describe("manage_skill execute", () => { let tempHome: string; + let originalAgentDir: string; beforeEach(async () => { + originalAgentDir = getAgentDir(); tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-manage-skill-")); spyOn(os, "homedir").mockReturnValue(tempHome); + setAgentDir(path.join(tempHome, ".omp", "agent")); }); afterEach(async () => { spyOn(os, "homedir").mockRestore(); + setAgentDir(originalAgentDir); resetActiveSkillsForTests(); await fs.rm(tempHome, { recursive: true, force: true }); }); @@ -178,6 +183,7 @@ describe("manage_skill execute", () => { describe("learn execute", () => { let tempHome: string; let remembered: string[]; + let originalAgentDir: string; function learnSession(): ToolSession { const fakeState = { @@ -195,13 +201,16 @@ describe("learn execute", () => { } beforeEach(async () => { + originalAgentDir = getAgentDir(); tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-learn-")); spyOn(os, "homedir").mockReturnValue(tempHome); + setAgentDir(path.join(tempHome, ".omp", "agent")); remembered = []; }); afterEach(async () => { spyOn(os, "homedir").mockRestore(); + setAgentDir(originalAgentDir); await fs.rm(tempHome, { recursive: true, force: true }); }); diff --git a/packages/coding-agent/test/discovery/builtin-rules-md.test.ts b/packages/coding-agent/test/discovery/builtin-rules-md.test.ts index f4fa9be10..d1f62d3f3 100644 --- a/packages/coding-agent/test/discovery/builtin-rules-md.test.ts +++ b/packages/coding-agent/test/discovery/builtin-rules-md.test.ts @@ -4,8 +4,8 @@ * from both `~/.omp/agent/RULES.md` (user) and the nearest `.omp/RULES.md` * (project, walked up from cwd to repoRoot). * - * Calls the native provider's `load` directly to bypass `loadCapability`'s - * hardcoded `os.homedir()` so the user scope can be staged inside a tempdir. + * Calls the native provider's `load` directly with the agent dir pointed at a + * tempdir (via setAgentDir) so the user scope can be staged in isolation. */ import { afterEach, beforeEach, expect, test } from "bun:test"; import * as fs from "node:fs"; @@ -17,11 +17,15 @@ import { type Rule, ruleCapability } from "@oh-my-pi/pi-coding-agent/capability/ import type { LoadContext } from "@oh-my-pi/pi-coding-agent/capability/types"; // Register all discovery providers as a side effect. import "@oh-my-pi/pi-coding-agent/discovery"; +import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; let tempDir: string; let home: string; let project: string; +const originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; +const fallbackAgentDir = path.join(getConfigRootDir(), "agent"); + function writeFile(filePath: string, content: string): void { fs.mkdirSync(path.dirname(filePath), { recursive: true }); fs.writeFileSync(filePath, content); @@ -44,10 +48,17 @@ beforeEach(() => { fs.mkdirSync(home, { recursive: true }); fs.mkdirSync(project, { recursive: true }); fs.mkdirSync(path.join(project, ".git"), { recursive: true }); + setAgentDir(path.join(home, ".omp", "agent")); }); afterEach(() => { clearCache(); + if (originalAgentDirEnv) { + setAgentDir(originalAgentDirEnv); + } else { + setAgentDir(fallbackAgentDir); + delete process.env.PI_CODING_AGENT_DIR; + } fs.rmSync(tempDir, { recursive: true, force: true }); }); diff --git a/packages/coding-agent/test/discovery/mcp-profile.test.ts b/packages/coding-agent/test/discovery/mcp-profile.test.ts new file mode 100644 index 000000000..3257cf38b --- /dev/null +++ b/packages/coding-agent/test/discovery/mcp-profile.test.ts @@ -0,0 +1,110 @@ +/** + * Regression: user-level MCP discovery must follow the active profile. + * + * A named profile relocates the agent directory to ~/.omp/profiles//agent. + * The native config provider used to read user-scope mcp.json from the literal + * home (~/.omp/agent/mcp.json) via `ctx.home`, so a profile never saw its own + * user-level servers while the default profile's servers leaked into every + * profile. Discovery now resolves the user scope through getAgentDir(), matching + * the /mcp config writer and getMCPConfigPath("user"). + * + * `os.homedir()` is mocked so the *old* code path (ctx.home + ".omp/agent") + * points at the tempdir decoy below; without the fix the profile case fails + * because it would load the decoy default server instead of the profile server. + */ +import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { clearCache as clearFsCache } from "@oh-my-pi/pi-coding-agent/capability/fs"; +import { type MCPServer, mcpCapability } from "@oh-my-pi/pi-coding-agent/capability/mcp"; +import { loadCapability } from "@oh-my-pi/pi-coding-agent/discovery"; +import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; + +const originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; +const fallbackAgentDir = path.join(getConfigRootDir(), "agent"); + +async function writeMcpJson(dir: string, servers: Record): Promise { + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, "mcp.json"), JSON.stringify({ mcpServers: servers }, null, 2)); +} + +async function loadNativeUserServers(cwd: string): Promise { + clearFsCache(); + const result = await loadCapability(mcpCapability.id, { cwd, providers: ["native"] }); + return result.items; +} + +describe("native user-level MCP discovery follows the active profile", () => { + let tempHome = ""; + let projectDir = ""; + let originalHome: string | undefined; + + beforeEach(async () => { + originalHome = process.env.HOME; + tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-profile-home-")); + projectDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-profile-project-")); + process.env.HOME = tempHome; + vi.spyOn(os, "homedir").mockReturnValue(tempHome); + clearFsCache(); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + clearFsCache(); + if (originalAgentDirEnv) { + setAgentDir(originalAgentDirEnv); + } else { + setAgentDir(fallbackAgentDir); + delete process.env.PI_CODING_AGENT_DIR; + } + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + await fs.rm(tempHome, { recursive: true, force: true }); + await fs.rm(projectDir, { recursive: true, force: true }); + }); + + test("active profile loads its own user server, not the default profile's", async () => { + // Active profile's agent dir (stand-in for ~/.omp/profiles//agent). + const profileAgentDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-profile-agent-")); + setAgentDir(profileAgentDir); + + // Decoy: the default profile's user file at the literal-home path the old + // (buggy) loader read. It must NOT leak into the active profile. + await writeMcpJson(path.join(tempHome, ".omp", "agent"), { + "default-only": { command: "default-cmd" }, + }); + await writeMcpJson(profileAgentDir, { + "profile-only": { command: "profile-cmd" }, + }); + + const servers = await loadNativeUserServers(projectDir); + const names = servers.map(s => s.name); + + expect(names).toContain("profile-only"); + expect(names).not.toContain("default-only"); + + const profileServer = servers.find(s => s.name === "profile-only"); + expect(profileServer?.command).toBe("profile-cmd"); + expect(profileServer?._source.level).toBe("user"); + expect(profileServer?._source.path).toBe(path.join(profileAgentDir, "mcp.json")); + + await fs.rm(profileAgentDir, { recursive: true, force: true }); + }); + + test("default profile loads the user server from ~/.omp/agent", async () => { + const defaultAgentDir = path.join(tempHome, ".omp", "agent"); + setAgentDir(defaultAgentDir); + await writeMcpJson(defaultAgentDir, { + "default-only": { command: "default-cmd" }, + }); + + const servers = await loadNativeUserServers(projectDir); + + const found = servers.find(s => s.name === "default-only"); + expect(found).toBeDefined(); + expect(found?.command).toBe("default-cmd"); + expect(found?._source.level).toBe("user"); + expect(found?._source.path).toBe(path.join(defaultAgentDir, "mcp.json")); + }); +}); diff --git a/packages/coding-agent/test/discovery/omp-plugins.test.ts b/packages/coding-agent/test/discovery/omp-plugins.test.ts index f091cd50a..f2711d9ce 100644 --- a/packages/coding-agent/test/discovery/omp-plugins.test.ts +++ b/packages/coding-agent/test/discovery/omp-plugins.test.ts @@ -32,6 +32,7 @@ import { clearOmpExtensionCliRoots, injectOmpExtensionCliRoots, } from "@oh-my-pi/pi-coding-agent/discovery/omp-extension-roots"; +import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; const PROVIDER_ID = "omp-plugins"; @@ -40,6 +41,9 @@ let home: string; let project: string; let ext: string; +const originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; +const fallbackAgentDir = path.join(getConfigRootDir(), "agent"); + function writeFile(filePath: string, content: string): void { fs.mkdirSync(path.dirname(filePath), { recursive: true }); fs.writeFileSync(filePath, content); @@ -92,11 +96,18 @@ beforeEach(() => { fs.mkdirSync(project, { recursive: true }); fs.mkdirSync(path.join(project, ".git"), { recursive: true }); buildExtensionPackage(ext); + setAgentDir(path.join(home, ".omp", "agent")); }); afterEach(() => { clearCache(); clearOmpExtensionCliRoots(); + if (originalAgentDirEnv) { + setAgentDir(originalAgentDirEnv); + } else { + setAgentDir(fallbackAgentDir); + delete process.env.PI_CODING_AGENT_DIR; + } fs.rmSync(tempDir, { recursive: true, force: true }); }); diff --git a/packages/coding-agent/test/discovery/pi-config-dir.test.ts b/packages/coding-agent/test/discovery/pi-config-dir.test.ts index 0ccf78d27..50b0bb171 100644 --- a/packages/coding-agent/test/discovery/pi-config-dir.test.ts +++ b/packages/coding-agent/test/discovery/pi-config-dir.test.ts @@ -4,6 +4,7 @@ import * as path from "node:path"; import type { LoadContext } from "@oh-my-pi/pi-coding-agent/capability/types"; import { getConfigDirs } from "@oh-my-pi/pi-coding-agent/config"; import { getUserPath } from "@oh-my-pi/pi-coding-agent/discovery/helpers"; +import { getAgentDir } from "@oh-my-pi/pi-utils"; describe("PI_CONFIG_DIR", () => { const original = process.env.PI_CONFIG_DIR; @@ -15,16 +16,18 @@ describe("PI_CONFIG_DIR", () => { } }); - test("getUserPath uses PI_CONFIG_DIR for native userAgent", () => { - process.env.PI_CONFIG_DIR = ".config/omp"; + test("getUserPath resolves the native user scope via getAgentDir (profile-aware)", () => { const ctx: LoadContext = { cwd: "/work/project", home: "/home/tester", repoRoot: null, }; - - const result = getUserPath(ctx, "native", "commands"); - expect(result).toBe(path.join(ctx.home, ".config/omp/agent", "commands")); + // Native user config follows the active profile through getAgentDir(), not + // ctx.home, so it stays in sync with builtin.ts and getMCPConfigPath("user"). + // The old behavior joined ctx.home + ".omp/agent" and leaked the default + // profile's config into every profile. + expect(getUserPath(ctx, "native", "commands")).toBe(path.join(getAgentDir(), "commands")); + expect(getUserPath(ctx, "native", "commands")).not.toContain(ctx.home); }); test("getConfigDirs respects PI_CONFIG_DIR for user base", () => { diff --git a/packages/coding-agent/test/discovery/profile-isolation.test.ts b/packages/coding-agent/test/discovery/profile-isolation.test.ts new file mode 100644 index 000000000..d82131695 --- /dev/null +++ b/packages/coding-agent/test/discovery/profile-isolation.test.ts @@ -0,0 +1,111 @@ +/** + * Regression: OMP-native user-level config discovery must follow the active + * profile. A profile relocates the agent directory to ~/.omp/profiles//agent; + * the native provider used to read user config (commands, skills, rules, etc.) + * from the literal home (~/.omp/agent) via `ctx.home`, leaking the default + * profile's config into every profile. Discovery now resolves the user scope + * through getAgentDir(), so a profile sees only its own config. + * + * Covers two code paths: getConfigDirs() (slash commands) and a direct + * getAgentDir() join (skills). `os.homedir()` is mocked so the old code path + * (ctx.home + ".omp/agent") points at the tempdir decoys below; without the fix + * each test would load the default-profile fixture instead of the profile one. + * + * MCP has its own regression in mcp-profile.test.ts (separate paths array). + */ +import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { clearCache as clearFsCache } from "@oh-my-pi/pi-coding-agent/capability/fs"; +import { type Skill, skillCapability } from "@oh-my-pi/pi-coding-agent/capability/skill"; +import { type SlashCommand, slashCommandCapability } from "@oh-my-pi/pi-coding-agent/capability/slash-command"; +import { loadCapability } from "@oh-my-pi/pi-coding-agent/discovery"; +import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; + +const originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; +const fallbackAgentDir = path.join(getConfigRootDir(), "agent"); + +async function writeFile(filePath: string, content: string): Promise { + await fs.mkdir(path.dirname(filePath), { recursive: true }); + await fs.writeFile(filePath, content); +} + +async function writeSkill(skillsDir: string, name: string): Promise { + await writeFile( + path.join(skillsDir, name, "SKILL.md"), + `---\nname: ${name}\ndescription: Skill ${name}.\n---\nBody.\n`, + ); +} + +describe("native user-level config discovery follows the active profile", () => { + let tempHome = ""; + let projectDir = ""; + let profileAgentDir = ""; + let originalHome: string | undefined; + + beforeEach(async () => { + originalHome = process.env.HOME; + tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "omp-profile-iso-home-")); + projectDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-profile-iso-project-")); + profileAgentDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-profile-iso-agent-")); + process.env.HOME = tempHome; + vi.spyOn(os, "homedir").mockReturnValue(tempHome); + setAgentDir(profileAgentDir); + + // Active profile's config. + await writeFile(path.join(profileAgentDir, "commands", "profile-cmd.md"), "Profile command.\n"); + await writeSkill(path.join(profileAgentDir, "skills"), "profile-skill"); + + // Decoy: default profile's config at the literal-home path the old loader read. + const defaultAgentDir = path.join(tempHome, ".omp", "agent"); + await writeFile(path.join(defaultAgentDir, "commands", "default-cmd.md"), "Default command.\n"); + await writeSkill(path.join(defaultAgentDir, "skills"), "default-skill"); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + clearFsCache(); + if (originalAgentDirEnv) { + setAgentDir(originalAgentDirEnv); + } else { + setAgentDir(fallbackAgentDir); + delete process.env.PI_CODING_AGENT_DIR; + } + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + await fs.rm(tempHome, { recursive: true, force: true }); + await fs.rm(projectDir, { recursive: true, force: true }); + await fs.rm(profileAgentDir, { recursive: true, force: true }); + }); + + test("slash commands resolve from the profile, not the default agent dir", async () => { + clearFsCache(); + const result = await loadCapability(slashCommandCapability.id, { + cwd: projectDir, + providers: ["native"], + }); + const names = result.items.map(c => c.name); + + expect(names).toContain("profile-cmd"); + expect(names).not.toContain("default-cmd"); + expect(result.items.find(c => c.name === "profile-cmd")?._source.path).toBe( + path.join(profileAgentDir, "commands", "profile-cmd.md"), + ); + }); + + test("skills resolve from the profile, not the default agent dir", async () => { + clearFsCache(); + const result = await loadCapability(skillCapability.id, { + cwd: projectDir, + providers: ["native"], + }); + const names = result.items.map(s => s.name); + + expect(names).toContain("profile-skill"); + expect(names).not.toContain("default-skill"); + expect(result.items.find(s => s.name === "profile-skill")?._source.path).toBe( + path.join(profileAgentDir, "skills", "profile-skill", "SKILL.md"), + ); + }); +}); diff --git a/packages/coding-agent/test/extension-flag-dispatch.test.ts b/packages/coding-agent/test/extension-flag-dispatch.test.ts new file mode 100644 index 000000000..b61706ea4 --- /dev/null +++ b/packages/coding-agent/test/extension-flag-dispatch.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "bun:test"; +import { applyExtensionFlags, type ExtensionFlagSink } from "../src/cli/extension-flags"; + +class FakeExtensionFlagSink implements ExtensionFlagSink { + #values = new Map(); + + get values(): ReadonlyMap { + return this.#values; + } + + getFlags(): Map { + return new Map([ + ["foo", { type: "boolean" }], + ["bar", { type: "string" }], + ]); + } + + setFlagValue(name: string, value: boolean | string): void { + this.#values.set(name, value); + } +} + +describe("extension flag dispatch", () => { + it("stops scanning raw argv at the end-of-options marker", () => { + const sink = new FakeExtensionFlagSink(); + + const args = applyExtensionFlags(sink, ["--", "--foo", "bar"]); + + expect(sink.values.size).toBe(0); + expect(args?.messages).toEqual(["--foo", "bar"]); + }); + + it("keeps -- as end-of-options after a string extension flag", () => { + const sink = new FakeExtensionFlagSink(); + + const args = applyExtensionFlags(sink, ["--bar", "--", "--foo", "bar"]); + + expect(sink.values.has("bar")).toBe(false); + expect(sink.values.size).toBe(0); + expect(args?.messages).toEqual(["--foo", "bar"]); + }); +}); diff --git a/packages/coding-agent/test/extension-flag-initial-message.test.ts b/packages/coding-agent/test/extension-flag-initial-message.test.ts index 04e7baac7..43ab9fe43 100644 --- a/packages/coding-agent/test/extension-flag-initial-message.test.ts +++ b/packages/coding-agent/test/extension-flag-initial-message.test.ts @@ -54,6 +54,18 @@ describe("extension flags vs initial message", () => { expect(parsed.print).toBeUndefined(); expect(parsed.messages).toEqual(["hello"]); }); + it("keeps standalone -- as end-of-options after a string extension flag", () => { + const parsed = parseArgs(["--spawn-peer", "--", "--model", "opus", "hello"], extFlags); + expect(parsed.unknownFlags.has("spawn-peer")).toBe(false); + expect(parsed.model).toBeUndefined(); + expect(parsed.messages).toEqual(["--model", "opus", "hello"]); + }); + it("consumes literal -- string values only in equals form", () => { + const parsed = parseArgs(["--spawn-peer=--", "--model", "opus", "hello"], extFlags); + expect(parsed.unknownFlags.get("spawn-peer")).toBe("--"); + expect(parsed.model).toBe("opus"); + expect(parsed.messages).toEqual(["hello"]); + }); it("treats an @-prefixed string value as the flag's value, not a file arg (P1#1)", () => { const parsed = parseArgs(["--spawn-peer", "@notes.md", "hello"], extFlags); expect(parsed.unknownFlags.get("spawn-peer")).toBe("@notes.md"); diff --git a/packages/coding-agent/test/extensions-discovery.test.ts b/packages/coding-agent/test/extensions-discovery.test.ts index da228c346..3f6ab9362 100644 --- a/packages/coding-agent/test/extensions-discovery.test.ts +++ b/packages/coding-agent/test/extensions-discovery.test.ts @@ -290,6 +290,90 @@ describe("extensions discovery", () => { expect(result.extensions).toHaveLength(3); }); + it("discovers a symlinked extension directory with index.ts", async () => { + // A single extension dir shared across profiles via a symlink: the real + // directory lives outside extensions/ and is linked into it. Native glob + // never descends into the symlink, so this exercises the symlink fallback. + const realDir = path.join(tempDir.path(), "external", "shared-ext"); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, "index.ts"), extensionCode); + fs.symlinkSync(realDir, path.join(extensionsDir, "linked-ext"), "dir"); + + const result = await discoverForTest(); + + expect(result.errors).toHaveLength(0); + expect(result.extensions).toHaveLength(1); + expect(result.extensions[0].path).toContain("linked-ext"); + expect(result.extensions[0].path).toContain("index.ts"); + }); + + it("discovers a symlinked extension directory with a package.json manifest", async () => { + // Mirrors the real-world shape: a packaged extension (package.json + index.ts) + // symlinked into a profile's extensions/ dir. + const realDir = path.join(tempDir.path(), "external", "ctk"); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, "index.ts"), extensionCodeWithTool("ctk-tool")); + fs.writeFileSync( + path.join(realDir, "package.json"), + JSON.stringify({ name: "ctk", omp: { extensions: ["./index.ts"] } }), + ); + fs.symlinkSync(realDir, path.join(extensionsDir, "ctk"), "dir"); + + const result = await discoverForTest(); + + expect(result.errors).toHaveLength(0); + // Manifest declares index.ts; it must be discovered exactly once (no double + // from the synthesized index.ts match colliding with the manifest entry). + expect(result.extensions).toHaveLength(1); + expect(result.extensions[0].path).toContain("index.ts"); + expect(result.extensions[0].tools.has("ctk-tool")).toBe(true); + }); + + it("discovers a symlinked extension file", async () => { + // Symlinked *files* resolve through the native file-type filter; guards that + // the directory fallback does not regress the file case. + const realFile = path.join(tempDir.path(), "external", "shared.ts"); + fs.mkdirSync(path.dirname(realFile), { recursive: true }); + fs.writeFileSync(realFile, extensionCode); + fs.symlinkSync(realFile, path.join(extensionsDir, "linked.ts"), "file"); + + const result = await discoverForTest(); + + expect(result.errors).toHaveLength(0); + expect(result.extensions).toHaveLength(1); + expect(result.extensions[0].path).toContain("linked.ts"); + }); + + it("does not crash on a dangling symlinked extension directory", async () => { + // A profile symlink pointing at a since-deleted shared extension. The fallback + // reads the (missing) target, gets [], and must yield no extension and no + // error rather than throwing. + fs.symlinkSync(path.join(tempDir.path(), "external", "gone"), path.join(extensionsDir, "broken"), "dir"); + + const result = await discoverForTest(); + + expect(result.errors).toHaveLength(0); + expect(result.extensions).toHaveLength(0); + }); + + it("discovers a symlinked extension directory whose name ends in .ts", async () => { + // Odd but legal: a *.ts-named symlink that targets a directory. The native + // file-type filter rejects it as a direct file (target is a dir), so it must + // resolve exactly once via the synthesized subdir index — never double-counted + // as both a direct file and a subdir entry. + const realDir = path.join(tempDir.path(), "external", "weird"); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, "index.ts"), extensionCode); + fs.symlinkSync(realDir, path.join(extensionsDir, "weird.ts"), "dir"); + + const result = await discoverForTest(); + + expect(result.errors).toHaveLength(0); + expect(result.extensions).toHaveLength(1); + expect(result.extensions[0].path).toContain("weird.ts"); + expect(result.extensions[0].path).toContain("index.ts"); + }); + it("skips non-existent paths declared in package.json", async () => { const subdir = path.join(extensionsDir, "my-package"); fs.mkdirSync(subdir); diff --git a/packages/coding-agent/test/flag-tables.test.ts b/packages/coding-agent/test/flag-tables.test.ts new file mode 100644 index 000000000..70db464c9 --- /dev/null +++ b/packages/coding-agent/test/flag-tables.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from "bun:test"; +import { parseArgs } from "../src/cli/args"; +import { OPTIONAL_VALUE_FLAGS, STRING_VALUE_FLAGS } from "../src/cli/flag-tables"; + +/** + * Catches the set → args.ts direction of drift between + * `cli/flag-tables.ts` and `cli/args.ts`: + * + * - If `STRING_VALUE_FLAGS` claims a flag consumes a value but + * `parseArgs` treats it as boolean (or doesn't handle it), then + * ` --profile work` would leave `--profile` standing — and + * parseArgs would activate the profile branch. We assert + * `result.profile` is undefined: the only way that's true is if the + * flag actually swallowed `--profile` as its value. + * + * - If `OPTIONAL_VALUE_FLAGS` claims a flag releases `-`-prefixed + * tokens but `parseArgs` swallows them anyway, then + * ` --profile work` would suppress the profile activation. We + * assert `result.profile === "work"`: the flag must NOT have eaten + * `--profile`, so parseArgs sees and activates it. + * + * The reverse direction (args.ts handler missing from the set) cannot + * be reflected on without parsing args.ts source — it's covered by + * per-flag regression tests in `profile-bootstrap.test.ts` and by + * user-facing scenarios in `profile-cli.test.ts`. + */ +describe("STRING_VALUE_FLAGS table is honored by args.ts parseArgs", () => { + for (const flag of STRING_VALUE_FLAGS) { + it(`${flag} consumes the next token unconditionally`, () => { + const result = parseArgs([flag, "--profile", "work"]); + expect( + result.profile, + `parseArgs should treat --profile as the value of ${flag}, not as a profile activation`, + ).toBeUndefined(); + }); + } +}); + +describe("OPTIONAL_VALUE_FLAGS table is honored by args.ts parseArgs", () => { + for (const flag of OPTIONAL_VALUE_FLAGS) { + it(`${flag} releases tokens that start with -`, () => { + const result = parseArgs([flag, "--profile", "work"]); + expect( + result.profile, + `parseArgs should release --profile back to its own handler when it follows ${flag}`, + ).toBe("work"); + }); + } +}); + +describe("OPTIONAL_FLAGS per-flag quirks", () => { + it("treats empty string as bare resume for --resume", () => { + const result = parseArgs(["--resume", ""]); + expect(result.resume).toBe(true); + expect(result.messages).toEqual([""]); + }); + + it("treats empty string as bare resume for -r", () => { + const result = parseArgs(["-r", ""]); + expect(result.resume).toBe(true); + expect(result.messages).toEqual([""]); + }); + + it("treats empty string as bare resume for --session", () => { + const result = parseArgs(["--session", ""]); + expect(result.resume).toBe(true); + expect(result.messages).toEqual([""]); + }); +}); + +describe("parseArgs end-of-options (--)", () => { + it("treats tokens after -- as literal messages, not flags", () => { + const result = parseArgs(["--", "--profile", "work"]); + expect(result.profile).toBeUndefined(); + expect(result.messages).toEqual(["--profile", "work"]); + }); + + it("does not interpret @ args or known value flags after --", () => { + const result = parseArgs(["--", "@file.md", "--model", "opus"]); + expect(result.model).toBeUndefined(); + expect(result.fileArgs).toEqual([]); + expect(result.messages).toEqual(["@file.md", "--model", "opus"]); + }); + + it("parses flags before -- and forwards the rest as text", () => { + const result = parseArgs(["--print", "hello", "--", "--no-tools"]); + expect(result.print).toBe(true); + expect(result.noTools).toBeUndefined(); + expect(result.messages).toEqual(["hello", "--no-tools"]); + }); +}); diff --git a/packages/coding-agent/test/mcp-command-reauth.test.ts b/packages/coding-agent/test/mcp-command-reauth.test.ts new file mode 100644 index 000000000..a27725a0b --- /dev/null +++ b/packages/coding-agent/test/mcp-command-reauth.test.ts @@ -0,0 +1,258 @@ +import { Database } from "bun:sqlite"; +import { afterEach, beforeAll, beforeEach, describe, expect, test, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai"; +import * as mcpClient from "@oh-my-pi/pi-coding-agent/mcp/client"; +import * as oauthFlow from "@oh-my-pi/pi-coding-agent/mcp/oauth-flow"; +import type { MCPServerConfig } from "@oh-my-pi/pi-coding-agent/mcp/types"; +import { MCPCommandController } from "@oh-my-pi/pi-coding-agent/modes/controllers/mcp-command-controller"; +import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import { getConfigRootDir, getMCPConfigPath, getProjectDir, setAgentDir, setProjectDir } from "@oh-my-pi/pi-utils"; + +const RAW_SERVER_URL = `https://\${MCP_HOST}/mcp`; +const EXPANDED_SERVER_URL = "https://mcp.example.com/mcp"; +const AUTH_ERROR = new Error( + 'HTTP 401: {"authorization_url":"https://auth.example.com/authorize","token_url":"https://auth.example.com/token"}', +); + +type TestConfigFile = { + mcpServers?: Record; +}; + +const originalProjectDir = getProjectDir(); +const originalAgentDir = process.env.PI_CODING_AGENT_DIR; +const fallbackAgentDir = path.join(getConfigRootDir(), "agent"); + +function restoreEnvValue(name: string, value: string | undefined): void { + if (value === undefined) { + delete Bun.env[name]; + delete process.env[name]; + return; + } + Bun.env[name] = value; + process.env[name] = value; +} +function createController(authStorage: AuthStorage, mcpManagerOverrides: Record = {}) { + const showError = vi.fn(); + const prepareConfig = vi.fn(async (config: MCPServerConfig) => config); + const mcpManager = { + prepareConfig, + disconnectAll: vi.fn(async () => {}), + discoverAndConnect: vi.fn(async () => ({ errors: new Map() })), + getTools: vi.fn(() => []), + waitForConnection: vi.fn(async () => {}), + getConnectionStatus: vi.fn(() => "connected"), + ...mcpManagerOverrides, + }; + const controller = new MCPCommandController({ + chatContainer: { addChild: vi.fn() }, + present: vi.fn(), + ui: { requestRender: vi.fn() }, + editor: {}, + showError, + showStatus: vi.fn(), + oauthManualInput: { + hasPending: vi.fn(() => false), + pendingProviderId: undefined, + tryClaimInput: vi.fn(), + }, + session: { + refreshMCPTools: vi.fn(), + modelRegistry: { authStorage }, + }, + mcpManager, + } as never); + + return { controller, showError, prepareConfig, mcpManager }; +} + +describe("/mcp auth commands", () => { + let projectDir = ""; + let agentDir = ""; + let configPath = ""; + let originalMcpHost: string | undefined; + + beforeAll(() => { + initTheme(); + }); + + beforeEach(async () => { + projectDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-reauth-project-")); + agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-reauth-agent-")); + configPath = path.join(projectDir, ".mcp.json"); + originalMcpHost = Bun.env.MCP_HOST; + Bun.env.MCP_HOST = "mcp.example.com"; + process.env.MCP_HOST = "mcp.example.com"; + setProjectDir(projectDir); + setAgentDir(agentDir); + await Bun.write( + configPath, + `${JSON.stringify( + { + mcpServers: { + envserver: { + type: "http", + url: RAW_SERVER_URL, + }, + }, + }, + null, + 2, + )}\n`, + ); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + restoreEnvValue("MCP_HOST", originalMcpHost); + setProjectDir(originalProjectDir); + if (originalAgentDir) { + setAgentDir(originalAgentDir); + } else { + setAgentDir(fallbackAgentDir); + delete process.env.PI_CODING_AGENT_DIR; + } + await fs.rm(projectDir, { recursive: true, force: true }); + await fs.rm(agentDir, { recursive: true, force: true }); + }); + + test("stores definition-only OAuth credentials under the expanded URL key", async () => { + const authStorage = new AuthStorage(new SqliteAuthCredentialStore(new Database(":memory:"))); + await authStorage.reload(); + const connectToServer = vi.spyOn(mcpClient, "connectToServer").mockRejectedValue(AUTH_ERROR); + vi.spyOn(oauthFlow.MCPOAuthFlow.prototype, "login").mockResolvedValue({ + access: "fresh-access", + refresh: "fresh-refresh", + expires: Date.now() + 3_600_000, + }); + const { controller, showError, prepareConfig } = createController(authStorage); + + await controller.handle("/mcp reauth envserver"); + + expect(showError).not.toHaveBeenCalled(); + expect(prepareConfig).toHaveBeenCalledWith( + expect.objectContaining({ url: EXPANDED_SERVER_URL }), + expect.objectContaining({ oauth: false }), + ); + expect(connectToServer).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ url: EXPANDED_SERVER_URL }), + ); + expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL))).toMatchObject({ + type: "oauth", + access: "fresh-access", + tokenUrl: "https://auth.example.com/token", + resource: EXPANDED_SERVER_URL, + }); + expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(RAW_SERVER_URL))).toBeUndefined(); + + const saved = JSON.parse(await Bun.file(configPath).text()) as TestConfigFile; + const savedServer = saved.mcpServers?.envserver; + const savedUrl = savedServer?.type === "http" || savedServer?.type === "sse" ? savedServer.url : undefined; + expect(savedUrl).toBe(RAW_SERVER_URL); + expect(savedServer?.auth).toBeUndefined(); + }); + + test("reuses embedded DCR client secret during reauth token exchange", async () => { + const authStorage = new AuthStorage(new SqliteAuthCredentialStore(new Database(":memory:"))); + await authStorage.reload(); + await authStorage.set(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL), { + type: "oauth", + access: "old-access", + refresh: "old-refresh", + expires: Date.now() + 3_600_000, + tokenUrl: "https://auth.example.com/token", + clientId: "dcr-client", + clientSecret: "dcr-secret", + resource: EXPANDED_SERVER_URL, + } as oauthFlow.MCPStoredOAuthCredential); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response( + JSON.stringify({ + access_token: "fresh-access", + refresh_token: "fresh-refresh", + expires_in: 3600, + token_type: "Bearer", + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + vi.spyOn(mcpClient, "connectToServer").mockRejectedValue(AUTH_ERROR); + vi.spyOn(oauthFlow.MCPOAuthFlow.prototype, "login").mockImplementation(function (this: oauthFlow.MCPOAuthFlow) { + return this.exchangeToken("authorization-code", "state", "http://127.0.0.1/callback"); + }); + const { controller, showError } = createController(authStorage); + + await controller.handle("/mcp reauth envserver"); + + expect(showError).not.toHaveBeenCalled(); + const tokenRequestBody = String(fetchSpy.mock.calls[0]?.[1]?.body ?? ""); + const tokenRequest = new URLSearchParams(tokenRequestBody); + expect(tokenRequest.get("client_id")).toBe("dcr-client"); + expect(tokenRequest.get("client_secret")).toBe("dcr-secret"); + expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL))).toMatchObject({ + type: "oauth", + access: "fresh-access", + clientId: "dcr-client", + clientSecret: "dcr-secret", + }); + }); + + test("clears both expanded and stale raw URL-keyed credentials on unauth", async () => { + const authStorage = new AuthStorage(new SqliteAuthCredentialStore(new Database(":memory:"))); + await authStorage.reload(); + await authStorage.set(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL), { + type: "oauth", + access: "expanded-access", + refresh: "expanded-refresh", + expires: Date.now() + 3_600_000, + }); + await authStorage.set(oauthFlow.mcpOAuthCredentialId(RAW_SERVER_URL), { + type: "oauth", + access: "raw-access", + refresh: "raw-refresh", + expires: Date.now() + 3_600_000, + }); + const { controller, showError } = createController(authStorage); + + await controller.handle("/mcp unauth envserver"); + + expect(showError).not.toHaveBeenCalled(); + expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL))).toBeUndefined(); + expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(RAW_SERVER_URL))).toBeUndefined(); + const saved = JSON.parse(await Bun.file(configPath).text()) as TestConfigFile; + const savedServer = saved.mcpServers?.envserver; + const savedUrl = savedServer?.type === "http" || savedServer?.type === "sse" ? savedServer.url : undefined; + expect(savedUrl).toBe(RAW_SERVER_URL); + expect(savedServer?.auth).toBeUndefined(); + }); + + test("clears url-keyed auth for discovered definition-only servers", async () => { + const authStorage = new AuthStorage(new SqliteAuthCredentialStore(new Database(":memory:"))); + await authStorage.reload(); + await authStorage.set(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL), { + type: "oauth", + access: "discovered-access", + refresh: "discovered-refresh", + expires: Date.now() + 3_600_000, + }); + const { controller, showError } = createController(authStorage, { + getServerConfig: vi.fn(() => ({ type: "http", url: EXPANDED_SERVER_URL })), + getSource: vi.fn(() => ({ provider: "test", path: "/tmp/discovered.json" })), + }); + + await controller.handle("/mcp unauth discovered"); + + expect(showError).not.toHaveBeenCalled(); + expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL))).toBeUndefined(); + const userConfigPath = getMCPConfigPath("user", projectDir); + const userConfig = JSON.parse( + await Bun.file(userConfigPath) + .text() + .catch(() => "{}"), + ) as TestConfigFile; + expect(userConfig.mcpServers?.discovered).toBeUndefined(); + }); +}); diff --git a/packages/coding-agent/test/mcp-profile-auth-binding.test.ts b/packages/coding-agent/test/mcp-profile-auth-binding.test.ts new file mode 100644 index 000000000..fc5813a5b --- /dev/null +++ b/packages/coding-agent/test/mcp-profile-auth-binding.test.ts @@ -0,0 +1,316 @@ +/** + * Contract tests for per-profile MCP OAuth bindings (url-keyed credentials). + * + * A server *definition* may live in a shared project `mcp.json` while each + * profile holds its own credential row in agent.db under the deterministic + * `mcp_oauth:profile::` id. Before this scheme, the random + * `auth.credentialId` written into the shared file pointed at exactly one + * profile's row, so two profiles reauthorizing the same project server + * clobbered each other. + */ +import { Database } from "bun:sqlite"; +import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test"; +import { AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai"; +import { MCPManager } from "@oh-my-pi/pi-coding-agent/mcp/manager"; +import * as oauthFlow from "@oh-my-pi/pi-coding-agent/mcp/oauth-flow"; +import { mcpOAuthCredentialId } from "@oh-my-pi/pi-coding-agent/mcp/oauth-flow"; +import type { MCPServerConfig } from "@oh-my-pi/pi-coding-agent/mcp/types"; +import { getActiveProfile, setProfile } from "@oh-my-pi/pi-utils/dirs"; + +const SERVER_URL = "https://mcp.example.com/mcp"; +const URL_KEY_ID = mcpOAuthCredentialId(SERVER_URL); + +function authorizationHeader(config: MCPServerConfig): string | undefined { + if (config.type !== "http" && config.type !== "sse") return undefined; + return config.headers?.Authorization; +} + +describe("per-profile MCP OAuth binding", () => { + let manager: MCPManager; + let authStorage: AuthStorage; + let originalProfile: string | undefined; + + beforeEach(async () => { + originalProfile = getActiveProfile(); + const store = new SqliteAuthCredentialStore(new Database(":memory:")); + authStorage = new AuthStorage(store); + await authStorage.reload(); + manager = new MCPManager(process.cwd()); + manager.setAuthStorage(authStorage); + }); + + afterEach(() => { + setProfile(originalProfile); + vi.restoreAllMocks(); + }); + + test("scopes url-keyed credentials by active profile in a shared auth namespace", async () => { + const workKey = mcpOAuthCredentialId(SERVER_URL, "work"); + const personalKey = mcpOAuthCredentialId(SERVER_URL, "personal"); + expect(workKey).not.toBe(personalKey); + await authStorage.set(workKey, { + type: "oauth", + access: "work-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + await authStorage.set(personalKey, { + type: "oauth", + access: "personal-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + setProfile("work"); + expect(authorizationHeader(await manager.prepareConfig({ type: "http", url: SERVER_URL }))).toBe( + "Bearer work-token", + ); + + setProfile("personal"); + expect(authorizationHeader(await manager.prepareConfig({ type: "http", url: SERVER_URL }))).toBe( + "Bearer personal-token", + ); + }); + + test("ignores another profile's explicit profile-scoped credentialId in shared storage", async () => { + const workKey = mcpOAuthCredentialId(SERVER_URL, "work"); + const personalKey = mcpOAuthCredentialId(SERVER_URL, "personal"); + await authStorage.set(workKey, { + type: "oauth", + access: "work-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + setProfile("personal"); + expect( + authorizationHeader( + await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { type: "oauth", credentialId: workKey }, + }), + ), + ).toBeUndefined(); + + await authStorage.set(personalKey, { + type: "oauth", + access: "personal-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + expect( + authorizationHeader( + await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { type: "oauth", credentialId: workKey }, + }), + ), + ).toBe("Bearer personal-token"); + }); + + test("resolves the url-keyed credential when the file's credentialId belongs to another profile", async () => { + // This profile authed the server (url-keyed row exists), but the shared + // project file still carries a credentialId minted by a different profile. + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "this-profile-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { type: "oauth", credentialId: "mcp_oauth_1234_other_profile" }, + }); + + expect(authorizationHeader(prepared)).toBe("Bearer this-profile-token"); + }); + + test("resolves the url-keyed credential for a definition-only config (no auth block)", async () => { + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "bound-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ type: "http", url: SERVER_URL }); + + expect(authorizationHeader(prepared)).toBe("Bearer bound-token"); + }); + + test("prepareConfig({ oauth: false }) skips injection so the reauth probe sees the bare server", async () => { + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "bound-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ type: "http", url: SERVER_URL }, { oauth: false }); + + expect(authorizationHeader(prepared)).toBeUndefined(); + }); + + test("never clobbers an explicitly configured Authorization header", async () => { + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "bound-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + headers: { authorization: "Bearer user-pinned" }, + }); + + expect(prepared.type === "http" ? prepared.headers?.authorization : undefined).toBe("Bearer user-pinned"); + expect(authorizationHeader(prepared)).toBeUndefined(); + }); + + test("refreshes with embedded material and preserves it across rotation", async () => { + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "expired-token", + refresh: "old-refresh", + expires: Date.now() - 60_000, + tokenUrl: "https://mcp.example.com/token", + clientId: "embedded-client", + clientSecret: "embedded-secret", + } as oauthFlow.MCPStoredOAuthCredential); + + const refreshSpy = vi.spyOn(oauthFlow, "refreshMCPOAuthToken").mockResolvedValue({ + access: "fresh-token", + refresh: "fresh-refresh", + expires: Date.now() + 3_600_000, + }); + + // Definition-only config: refresh material must come from the credential. + const prepared = await manager.prepareConfig({ type: "http", url: SERVER_URL }); + + expect(refreshSpy).toHaveBeenCalledWith( + "https://mcp.example.com/token", + "old-refresh", + "embedded-client", + "embedded-secret", + SERVER_URL, + ); + expect(authorizationHeader(prepared)).toBe("Bearer fresh-token"); + // Embedded refresh material must survive rotation, or the *next* refresh + // of this definition-only binding would be impossible. + expect(authStorage.get(URL_KEY_ID)).toMatchObject({ + type: "oauth", + access: "fresh-token", + refresh: "fresh-refresh", + tokenUrl: "https://mcp.example.com/token", + clientId: "embedded-client", + resource: SERVER_URL, + }); + }); + + test("does not inject oauth for configs with explicit apikey auth", async () => { + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "bound-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { type: "apikey" }, + }); + + expect(authorizationHeader(prepared)).toBeUndefined(); + }); + + test("an explicit credentialId that resolves wins over the url-keyed row", async () => { + await authStorage.set("mcp_oauth_1234_pinned", { + type: "oauth", + access: "pinned-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "url-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { type: "oauth", credentialId: "mcp_oauth_1234_pinned" }, + }); + + expect(authorizationHeader(prepared)).toBe("Bearer pinned-token"); + }); + + test("url-keyed fallback never overrides a pinned Authorization header, even past a stale auth block", async () => { + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "bound-token", + refresh: "r", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + headers: { Authorization: "Bearer user-pinned" }, + auth: { type: "oauth", credentialId: "mcp_oauth_1234_other_profile" }, + }); + + expect(authorizationHeader(prepared)).toBe("Bearer user-pinned"); + }); + + test("refresh uses the credential's embedded client, not another profile's auth block", async () => { + // Shared file carries profile A's refresh material; this profile's + // url-keyed row embeds its own DCR client. Refresh tokens are bound to + // the client that minted them, so the embedded material must win or the + // refresh dies with invalid_grant and the row gets purged. + await authStorage.set(URL_KEY_ID, { + type: "oauth", + access: "expired-token", + refresh: "my-refresh", + expires: Date.now() - 60_000, + tokenUrl: "https://mcp.example.com/token", + clientId: "my-dcr-client", + } as oauthFlow.MCPStoredOAuthCredential); + + const refreshSpy = vi.spyOn(oauthFlow, "refreshMCPOAuthToken").mockResolvedValue({ + access: "fresh-token", + refresh: "my-refresh", + expires: Date.now() + 3_600_000, + }); + + const prepared = await manager.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { + type: "oauth", + credentialId: "mcp_oauth_1234_other_profile", + tokenUrl: "https://mcp.example.com/token", + clientId: "other-profiles-client", + clientSecret: "other-profiles-secret", + }, + }); + + expect(refreshSpy).toHaveBeenCalledWith( + "https://mcp.example.com/token", + "my-refresh", + "my-dcr-client", + undefined, + SERVER_URL, + ); + expect(authorizationHeader(prepared)).toBe("Bearer fresh-token"); + }); +}); diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index f80898171..879da2db2 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -69,6 +69,68 @@ describe("mcp oauth flow", () => { expect(authUrl.searchParams.get("state")).toBe("test-state"); }); + it("defaults prompt=consent so reauth can switch accounts despite an active browser session", async () => { + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + clientId: "client-id", + }, + {}, + ); + + const { url } = await flow.generateAuthUrl("test-state", "http://127.0.0.1:53180/callback"); + + expect(new URL(url).searchParams.get("prompt")).toBe("consent"); + }); + + it("passes an explicit prompt value through to the authorization request", async () => { + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + clientId: "client-id", + prompt: "select_account", + }, + {}, + ); + + const { url } = await flow.generateAuthUrl("s", "http://127.0.0.1:53181/callback"); + + expect(new URL(url).searchParams.get("prompt")).toBe("select_account"); + }); + + it("omits the prompt parameter entirely when configured as the empty string", async () => { + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + clientId: "client-id", + prompt: "", + }, + {}, + ); + + const { url } = await flow.generateAuthUrl("s", "http://127.0.0.1:53182/callback"); + + expect(new URL(url).searchParams.has("prompt")).toBe(false); + }); + + it("keeps a prompt value already embedded in the authorization URL", async () => { + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize?prompt=none", + tokenUrl: "https://provider.example/token", + clientId: "client-id", + }, + {}, + ); + + const { url } = await flow.generateAuthUrl("test-state", "http://127.0.0.1:53183/callback"); + + expect(new URL(url).searchParams.get("prompt")).toBe("none"); + }); + it("uses configured callbackPath for the local redirect URI", async () => { let observedRedirectUri = ""; let tokenRequestBody = ""; diff --git a/packages/coding-agent/test/profile-alias.test.ts b/packages/coding-agent/test/profile-alias.test.ts new file mode 100644 index 000000000..fd55786cd --- /dev/null +++ b/packages/coding-agent/test/profile-alias.test.ts @@ -0,0 +1,348 @@ +import { describe, expect, it } from "bun:test"; +import { + installProfileAlias, + readProfileAliasConfigFile, + resolveProfileAliasCommandFromProcess, +} from "../src/cli/profile-alias"; + +describe("profile alias installer", () => { + it("writes a bash-compatible function that forwards subcommands through omp", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/bin/bash", + platform: "linux", + homeDir: "/home/me", + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.configPath).toBe("/home/me/.bashrc"); + expect(result.command).toBe("omp --profile=work"); + expect(files.get("/home/me/.bashrc")).toContain("omp-work() {"); + expect(files.get("/home/me/.bashrc")).toContain('command omp --profile=work "$@"'); + }); + + it("resolves source invocations without forcing the source checkout as cwd", () => { + const command = resolveProfileAliasCommandFromProcess(["/bin/bun", "src/cli.ts"], "/repo/packages/coding-agent"); + + expect(command.display).toBe("/bin/bun /repo/packages/coding-agent/src/cli.ts"); + expect(command.posix).toBe("'/bin/bun' '/repo/packages/coding-agent/src/cli.ts'"); + expect(command.fish).toBe("'/bin/bun' '/repo/packages/coding-agent/src/cli.ts'"); + expect(command.powerShell).toBe("'/bin/bun' '/repo/packages/coding-agent/src/cli.ts'"); + }); + + it("can target the current source invocation instead of the installed omp binary", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/bin/zsh", + platform: "darwin", + homeDir: "/Users/me", + command: { + display: "bun /repo/packages/coding-agent/src/cli.ts", + posix: "bun '/repo/packages/coding-agent/src/cli.ts'", + fish: "bun /repo/packages/coding-agent/src/cli.ts", + powerShell: "bun '/repo/packages/coding-agent/src/cli.ts'", + }, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.command).toBe("bun /repo/packages/coding-agent/src/cli.ts --profile=work"); + expect(files.get("/Users/me/.zshrc")).toContain("omp-work() {"); + expect(files.get("/Users/me/.zshrc")).toContain( + `command bun '/repo/packages/coding-agent/src/cli.ts' --profile=work "$@"`, + ); + }); + + it("installs the zsh alias under ZDOTDIR when set", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/bin/zsh", + platform: "darwin", + homeDir: "/Users/me", + env: { ZDOTDIR: "/Users/me/.config/zsh" }, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.configPath).toBe("/Users/me/.config/zsh/.zshrc"); + expect(files.get(result.configPath)).toContain("omp-work() {"); + }); + + it("writes a fish function that forwards argv", async () => { + const files = new Map(); + + await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/opt/homebrew/bin/fish", + platform: "darwin", + homeDir: "/Users/me", + env: {}, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + const content = files.get("/Users/me/.config/fish/conf.d/omp-profiles.fish") ?? ""; + expect(content).toContain("function omp-work --wraps omp"); + expect(content).toContain("command omp --profile=work $argv"); + }); + + it("installs the fish alias under XDG_CONFIG_HOME when set", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/usr/bin/fish", + platform: "linux", + homeDir: "/home/me", + env: { XDG_CONFIG_HOME: "/home/me/.dotfiles/config" }, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.configPath).toBe("/home/me/.dotfiles/config/fish/conf.d/omp-profiles.fish"); + expect(files.get(result.configPath)).toContain("function omp-work --wraps omp"); + }); + + it("writes a PowerShell function because aliases cannot carry arguments", async () => { + const files = new Map(); + + await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "pwsh.exe", + platform: "win32", + homeDir: "C:\\Users\\me", + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + const content = files.get("C:\\Users\\me/Documents/PowerShell/Microsoft.PowerShell_profile.ps1") ?? ""; + expect(content).toContain("function omp-work"); + expect(content).toContain("& omp --profile=work @args"); + }); + + it("detects pwsh from PSModulePath when SHELL is unset on Windows", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + platform: "win32", + homeDir: "C:\\Users\\me", + env: { + PSModulePath: + "C:\\Users\\me\\Documents\\PowerShell\\Modules;C:\\Program Files\\PowerShell\\7\\Modules;C:\\Users\\me\\Documents\\WindowsPowerShell\\Modules", + }, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.shell).toBe("pwsh"); + expect(result.configPath).toBe("C:\\Users\\me/Documents/PowerShell/Microsoft.PowerShell_profile.ps1"); + expect(files.get(result.configPath)).toContain("& omp --profile=work @args"); + }); + + it("selects Windows PowerShell when only WindowsPowerShell modules are present", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + platform: "win32", + homeDir: "C:\\Users\\me", + env: { + PSModulePath: + "C:\\Users\\me\\Documents\\WindowsPowerShell\\Modules;C:\\WINDOWS\\system32\\WindowsPowerShell\\v1.0\\Modules", + }, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.shell).toBe("powershell"); + expect(result.configPath).toBe("C:\\Users\\me/Documents/WindowsPowerShell/Microsoft.PowerShell_profile.ps1"); + }); + + it("treats POWERSHELL_DISTRIBUTION_CHANNEL as a pwsh hint when no module paths disambiguate", async () => { + const files = new Map(); + + const result = await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + platform: "win32", + homeDir: "C:\\Users\\me", + env: { POWERSHELL_DISTRIBUTION_CHANNEL: "MSI:Windows 10 Pro" }, + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + expect(result.shell).toBe("pwsh"); + expect(result.configPath).toBe("C:\\Users\\me/Documents/PowerShell/Microsoft.PowerShell_profile.ps1"); + }); + + it("replaces a previous block for the same alias", async () => { + const files = new Map([ + [ + "/home/me/.zshrc", + [ + "before", + "# >>> omp profile alias: omp-work >>>", + "alias omp-work='command omp --profile=old'", + "# <<< omp profile alias: omp-work <<<", + "after", + ].join("\n"), + ], + ]); + + await installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/bin/zsh", + platform: "darwin", + homeDir: "/home/me", + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }); + + const content = files.get("/home/me/.zshrc") ?? ""; + expect(content).toContain("before"); + expect(content).toContain("after"); + expect(content).toContain('command omp --profile=work "$@"'); + expect(content).not.toContain("--profile=old"); + }); + + it("refuses to rewrite a malformed managed block missing its end marker", async () => { + // A start marker without its matching end marker means a previous install + // was interrupted or hand-edited. Appending a fresh block would let the + // *next* install splice from the stale start through the new end, deleting + // the user config in between. Refuse and preserve the file untouched. + const original = ["# >>> omp profile alias: omp-work >>>", "omp-work() {", "export SECRET=keepme"].join("\n"); + const files = new Map([["/home/me/.zshrc", original]]); + let wrote = false; + + await expect( + installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/bin/zsh", + platform: "darwin", + homeDir: "/home/me", + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + wrote = true; + files.set(filePath, content); + }, + }), + ).rejects.toThrow(/without a matching/); + + expect(wrote).toBe(false); + expect(files.get("/home/me/.zshrc")).toBe(original); + }); + + it("refuses to shadow the base omp command case-insensitively", async () => { + for (const aliasName of ["omp", "OMP"]) { + await expect( + installProfileAlias({ + profile: "work", + aliasName, + shellPath: "/bin/bash", + homeDir: "/home/me", + }), + ).rejects.toThrow("Refusing to shadow"); + } + }); + + it("rejects shell reserved words before rendering alias functions", async () => { + for (const { aliasName, shellPath } of [ + { aliasName: "if", shellPath: "/bin/bash" }, + { aliasName: "end", shellPath: "/opt/homebrew/bin/fish" }, + { aliasName: "foreach", shellPath: "pwsh.exe" }, + ]) { + await expect( + installProfileAlias({ + profile: "work", + aliasName, + shellPath, + platform: shellPath === "pwsh.exe" ? "win32" : "linux", + homeDir: "/home/me", + }), + ).rejects.toThrow("reserved word"); + } + }); + + it("rejects POSIX sh because it does not read bash config files", async () => { + await expect( + installProfileAlias({ + profile: "work", + aliasName: "omp-work", + shellPath: "/bin/sh", + platform: "linux", + homeDir: "/home/me", + }), + ).rejects.toThrow('Unsupported shell "sh"'); + }); + + it("treats missing shell config as empty but preserves other read failures", async () => { + await expect( + readProfileAliasConfigFile("/home/me/.bashrc", async () => { + throw Object.assign(new Error("missing"), { code: "ENOENT" }); + }), + ).resolves.toBe(""); + + await expect( + readProfileAliasConfigFile("/home/me/.bashrc", async () => { + throw Object.assign(new Error("denied"), { code: "EACCES" }); + }), + ).rejects.toThrow("denied"); + }); + + it("validates profile names before rendering shell code", async () => { + const files = new Map(); + + await expect( + installProfileAlias({ + profile: "work'; touch /tmp/pwn; #", + aliasName: "omp-work", + shellPath: "/bin/bash", + platform: "linux", + homeDir: "/home/me", + readFile: async filePath => files.get(filePath) ?? "", + writeFile: async (filePath, content) => { + files.set(filePath, content); + }, + }), + ).rejects.toThrow("Invalid OMP profile"); + expect(files.size).toBe(0); + }); +}); diff --git a/packages/coding-agent/test/profile-bootstrap.test.ts b/packages/coding-agent/test/profile-bootstrap.test.ts new file mode 100644 index 000000000..d3a3255e6 --- /dev/null +++ b/packages/coding-agent/test/profile-bootstrap.test.ts @@ -0,0 +1,277 @@ +import { describe, expect, it } from "bun:test"; +import { parseArgs } from "../src/cli/args"; +import { PROFILE_BOOTSTRAP_BOUNDARY_ARG } from "../src/cli/flag-tables"; +import { extractProfileFlags } from "../src/cli/profile-bootstrap"; + +describe("extractProfileFlags", () => { + it("extracts --profile without disturbing other tokens", () => { + expect(extractProfileFlags(["--profile", "work"])).toEqual({ + argv: [], + profile: "work", + aliasName: undefined, + }); + expect(extractProfileFlags(["foo", "--profile=work", "bar"])).toEqual({ + argv: ["foo", "bar"], + profile: "work", + aliasName: undefined, + }); + }); + + it("does not eat the value of known string-valued flags", () => { + // `omp --system-prompt --profile foo` must pass the literal `--profile` + // through to the launch parser (it's the system prompt) and `foo` is the + // positional message. The previous implementation would silently activate + // profile `foo` here, dropping the user's prompt. + const result = extractProfileFlags(["--system-prompt", "--profile", "foo", "bar"]); + expect(result.profile).toBeUndefined(); + expect(result.argv).toEqual(["--system-prompt", "--profile", "foo", "bar"]); + }); + it("does not eat the value of --approval-mode", () => { + // `--approval-mode` is a string-valued flag in args.ts (`args[++i]` with + // no `-` check). The pre-parser must mirror that contract or + // `omp --approval-mode --profile foo` silently activates profile `foo` + // instead of letting the launch parser surface the invalid mode value. + const result = extractProfileFlags(["--approval-mode", "--profile", "foo", "bar"]); + expect(result.profile).toBeUndefined(); + expect(result.argv).toEqual(["--approval-mode", "--profile", "foo", "bar"]); + }); + + it("honors extension-shadowed --plan before a global profile", () => { + const extracted = extractProfileFlags(["--plan", "--profile", "work", "follow up"]); + expect(extracted).toEqual({ + argv: ["--plan", PROFILE_BOOTSTRAP_BOUNDARY_ARG, "follow up"], + profile: "work", + aliasName: undefined, + }); + + const parsed = parseArgs(extracted.argv, new Map([["plan", { type: "boolean" }]])); + expect(parsed.unknownFlags.get("plan")).toBe(true); + expect(parsed.plan).toBeUndefined(); + expect(parsed.messages).toEqual(["follow up"]); + }); + + it("still extracts --profile after an unrelated string-valued flag", () => { + // Mirror image: when the user does mean to activate a profile *after* + // a string-valued flag, we must skip past the flag's value but still + // pick up the trailing `--profile`. + const result = extractProfileFlags(["--system-prompt", "hello", "--profile", "work"]); + expect(result.profile).toBe("work"); + expect(result.argv).toEqual(["--system-prompt", "hello"]); + }); + + it("treats optional-value flags as consuming the next token only when it doesn't look like a flag", () => { + // `--resume ` consumes the id, `--resume` alone is a picker. + const consumed = extractProfileFlags(["--resume", "abc123", "--profile", "work"]); + expect(consumed.argv).toEqual(["--resume", "abc123"]); + expect(consumed.profile).toBe("work"); + + const picker = extractProfileFlags(["--resume", "--profile", "work"]); + expect(picker.argv).toEqual(["--resume"]); + expect(picker.profile).toBe("work"); + + // `--list-models` mirrors args.ts and does not consume `@`-prefixed + // tokens (they're file args); the pre-pass releases them and the + // trailing `--profile work` still activates. + const filePrefixed = extractProfileFlags(["--list-models", "@models.txt", "--profile", "work"]); + expect(filePrefixed.argv).toEqual(["--list-models", "@models.txt"]); + expect(filePrefixed.profile).toBe("work"); + }); + + it("preserves optional-flag boundaries when stripping a profile before prompt text", () => { + const extracted = extractProfileFlags(["--resume", "--profile", "work", "follow up"]); + expect(extracted).toEqual({ + argv: ["--resume", PROFILE_BOOTSTRAP_BOUNDARY_ARG, "follow up"], + profile: "work", + aliasName: undefined, + }); + + const parsed = parseArgs(extracted.argv); + expect(parsed.resume).toBe(true); + expect(parsed.messages).toEqual(["follow up"]); + }); + + it("preserves extension-flag boundaries when stripping a profile before prompt text", () => { + const extracted = extractProfileFlags(["--some-ext-flag", "--profile", "work", "follow up"]); + expect(extracted).toEqual({ + argv: ["--some-ext-flag", PROFILE_BOOTSTRAP_BOUNDARY_ARG, "follow up"], + profile: "work", + aliasName: undefined, + }); + + const parsed = parseArgs(extracted.argv, new Map([["some-ext-flag", { type: "string" }]])); + expect(parsed.unknownFlags.has("some-ext-flag")).toBe(false); + expect(parsed.messages).toEqual(["follow up"]); + }); + + it("does not consume empty-string resume values before a trailing profile", () => { + // Shared OPTIONAL_FLAGS metadata drives the bootstrap too. Empty string is + // "no value" for resume/session aliases, so the bootstrap must release it + // and still activate the trailing --profile. + const result = extractProfileFlags(["--resume", "", "--profile", "work"]); + expect(result.argv).toEqual(["--resume", ""]); + expect(result.profile).toBe("work"); + }); + + it("honors `--` and stops scanning for flags", () => { + const result = extractProfileFlags(["--", "--profile", "foo", "--alias", "bar"]); + expect(result.profile).toBeUndefined(); + expect(result.aliasName).toBeUndefined(); + expect(result.argv).toEqual(["--", "--profile", "foo", "--alias", "bar"]); + }); + + it("rejects --profile without a value", () => { + expect(() => extractProfileFlags(["--profile"])).toThrow("--profile requires a profile name"); + expect(() => extractProfileFlags(["--profile", "--version"])).toThrow("--profile requires a profile name"); + expect(() => extractProfileFlags(["--profile="])).toThrow("--profile requires a profile name"); + }); + + it("rejects --alias without a value", () => { + expect(() => extractProfileFlags(["--alias"])).toThrow("--alias requires a command name"); + expect(() => extractProfileFlags(["--alias", "--profile"])).toThrow("--alias requires a command name"); + expect(() => extractProfileFlags(["--alias="])).toThrow("--alias requires a command name"); + }); + + it("stops extracting global flags at a subcommand boundary", () => { + // `omp grep --profile ` must reach the grep subcommand intact; the + // bootstrap must not treat `--profile ` as a profile selection. + const result = extractProfileFlags(["grep", "--profile", "packages/coding-agent/src/cli.ts"]); + expect(result.profile).toBeUndefined(); + expect(result.argv).toEqual(["grep", "--profile", "packages/coding-agent/src/cli.ts"]); + }); + + it("extracts a global --profile that precedes a subcommand", () => { + const result = extractProfileFlags(["--profile", "work", "grep", "foo"]); + expect(result.profile).toBe("work"); + expect(result.argv).toEqual(["grep", "foo"]); + }); + + it("treats explicit launch as the default command and keeps extracting globals", () => { + expect(extractProfileFlags(["launch", "--profile", "work", "--alias", "omp-work"])).toEqual({ + argv: ["launch"], + profile: "work", + aliasName: "omp-work", + }); + }); + + it("treats explicit acp as launch-shaped and keeps extracting globals", () => { + expect(extractProfileFlags(["acp", "--profile", "work"])).toEqual({ + argv: ["acp"], + profile: "work", + aliasName: undefined, + }); + }); + + it("treats later subcommand-shaped words as launch text after explicit launch", () => { + const result = extractProfileFlags(["launch", "grep", "--profile", "work"]); + expect(result.profile).toBe("work"); + expect(result.argv).toEqual(["launch", "grep"]); + }); + + it("still extracts --profile after a non-subcommand positional (launch message)", () => { + const result = extractProfileFlags(["hello", "--profile", "work"]); + expect(result.profile).toBe("work"); + expect(result.argv).toEqual(["hello"]); + }); + + it("continues extracting launch profiles after later subcommand-shaped words", () => { + const result = extractProfileFlags(["hello", "grep", "--profile", "work"]); + expect(result.profile).toBe("work"); + expect(result.argv).toEqual(["hello", "grep"]); + }); + + it("continues extracting launch profiles after launch flags before subcommand-shaped words", () => { + const result = extractProfileFlags(["--model", "opus", "grep", "--profile", "work"]); + expect(result.profile).toBe("work"); + expect(result.argv).toEqual(["--model", "opus", "grep"]); + }); + + it("does not treat a --profile value that names a subcommand as a boundary", () => { + const result = extractProfileFlags(["--profile", "config", "later"]); + expect(result.profile).toBe("config"); + expect(result.argv).toEqual(["later"]); + }); + + it("exempts known value-less launch flags so a trailing profile still activates", () => { + // Boolean launch flags (--print, --yolo, --no-tools, -p) take no value, so + // the token after them is a fresh argument: `omp --print --profile work` + // must still select the profile. + expect(extractProfileFlags(["--print", "--profile", "work"])).toEqual({ + argv: ["--print"], + profile: "work", + aliasName: undefined, + }); + expect(extractProfileFlags(["--yolo", "--profile", "work"])).toEqual({ + argv: ["--yolo"], + profile: "work", + aliasName: undefined, + }); + expect(extractProfileFlags(["--no-tools", "--profile", "work"])).toEqual({ + argv: ["--no-tools"], + profile: "work", + aliasName: undefined, + }); + expect(extractProfileFlags(["-p", "--profile", "work"])).toEqual({ + argv: ["-p"], + profile: "work", + aliasName: undefined, + }); + }); + + it("protects a value-like successor of an unknown (extension) string flag", () => { + // The bootstrap runs before extensions load and cannot know that `--bar` + // is a string flag consuming its next token. When the successor is + // value-like (does not start with `-`), `parseArgs` consumes it as the + // extension flag's value, so the bootstrap forwards it untouched and never + // mis-reads it as a global flag — even when it spells a subcommand name. + expect(extractProfileFlags(["--bar", "value", "--profile", "work"])).toEqual({ + argv: ["--bar", "value"], + profile: "work", + aliasName: undefined, + }); + expect(extractProfileFlags(["--bar", "config"])).toEqual({ + argv: ["--bar", "config"], + profile: undefined, + aliasName: undefined, + }); + }); + + it("does not hide a global --profile/--alias behind an unknown flag with a flag-looking successor", () => { + // `parseArgs` never hands a flag-looking successor to an extension flag: + // boolean extension flags consume nothing, and string extension flags only + // consume value-like (non-`-`) successors. So `omp --some-ext-flag --profile + // work` must still select profile `work`; the prior bootstrap forwarded + // `--profile` as a protected successor and silently fell back to default. + expect(extractProfileFlags(["--some-ext-flag", "--profile", "work"])).toEqual({ + argv: ["--some-ext-flag"], + profile: "work", + aliasName: undefined, + }); + expect(extractProfileFlags(["--some-ext-flag", "--alias", "omp-work"])).toEqual({ + argv: ["--some-ext-flag"], + profile: undefined, + aliasName: "omp-work", + }); + }); + + it("treats a `--` successor of an unknown flag as end-of-options, not a protected value", () => { + // `--` is the parser's end-of-options marker even after a string extension + // flag. The bootstrap keeps that single meaning: everything after is + // forwarded verbatim, so a `--profile` fenced behind `--` never silently + // activates. + expect(extractProfileFlags(["--some-ext-flag", "--", "--profile", "work"])).toEqual({ + argv: ["--some-ext-flag", "--", "--profile", "work"], + profile: undefined, + aliasName: undefined, + }); + }); + + it("still extracts a trailing profile after an unknown flag that carries its own =value", () => { + // `--bar=x` carries its value inline, so the following token is a fresh + // argument and the trailing --profile is a genuine global flag. + expect(extractProfileFlags(["--bar=x", "--profile", "work"])).toEqual({ + argv: ["--bar=x"], + profile: "work", + aliasName: undefined, + }); + }); +}); diff --git a/packages/coding-agent/test/profile-cli.test.ts b/packages/coding-agent/test/profile-cli.test.ts new file mode 100644 index 000000000..00232cad1 --- /dev/null +++ b/packages/coding-agent/test/profile-cli.test.ts @@ -0,0 +1,322 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import * as url from "node:url"; +import { + __resetProfileSnapshotForTests, + APP_NAME, + getActiveProfile, + getAgentDbPath, + getAgentDir, + setAgentDir, + setProfile, + VERSION, +} from "@oh-my-pi/pi-utils/dirs"; +import { Snowflake } from "@oh-my-pi/pi-utils/snowflake"; +import { runCli } from "../src/cli"; +import * as profileAliasCli from "../src/cli/profile-alias"; + +const repoRoot = path.resolve(import.meta.dir, "..", "..", ".."); +const cliEntry = path.join(repoRoot, "packages", "coding-agent", "src", "cli.ts"); + +async function readStream(stream: ReadableStream): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let text = ""; + try { + while (true) { + const { value, done } = await reader.read(); + if (done) break; + text += decoder.decode(value, { stream: true }); + } + return text + decoder.decode(); + } finally { + reader.releaseLock(); + } +} + +describe("global --profile flag", () => { + let configDir = ""; + let originalProfile: string | undefined; + let originalAgentDir = ""; + let originalAgentDirEnv: string | undefined; + let originalOmpProfileEnv: string | undefined; + let originalPiProfileEnv: string | undefined; + let originalConfigDir: string | undefined; + + beforeEach(() => { + originalProfile = getActiveProfile(); + originalAgentDir = getAgentDir(); + originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; + originalOmpProfileEnv = process.env.OMP_PROFILE; + originalPiProfileEnv = process.env.PI_PROFILE; + originalConfigDir = process.env.PI_CONFIG_DIR; + configDir = `.omp-profile-cli-test-${Snowflake.next()}`; + process.env.PI_CONFIG_DIR = configDir; + process.exitCode = 0; + }); + + afterEach(async () => { + vi.restoreAllMocks(); + setProfile(undefined); + if (originalConfigDir === undefined) { + delete process.env.PI_CONFIG_DIR; + } else { + process.env.PI_CONFIG_DIR = originalConfigDir; + } + if (originalProfile) { + setProfile(originalProfile); + } else if (originalAgentDirEnv !== undefined) { + setAgentDir(originalAgentDir); + } else { + setProfile(undefined); + } + if (originalOmpProfileEnv === undefined) { + delete process.env.OMP_PROFILE; + } else { + process.env.OMP_PROFILE = originalOmpProfileEnv; + } + if (originalPiProfileEnv === undefined) { + delete process.env.PI_PROFILE; + } else { + process.env.PI_PROFILE = originalPiProfileEnv; + } + if (originalAgentDirEnv === undefined) { + delete process.env.PI_CODING_AGENT_DIR; + } else { + process.env.PI_CODING_AGENT_DIR = originalAgentDirEnv; + } + __resetProfileSnapshotForTests(); + process.exitCode = 0; + await fs.rm(path.join(os.homedir(), configDir), { recursive: true, force: true }); + }); + + it("activates a profile before dispatching root flags", async () => { + const writeSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runCli(["--profile=work", "--version"]); + + expect(process.exitCode).toBe(0); + expect(writeSpy).toHaveBeenCalled(); + expect(getActiveProfile()).toBe("work"); + expect(getAgentDir()).toBe(path.join(os.homedir(), configDir, "profiles", "work", "agent")); + }); + + it("activates a profile inherited from OMP_PROFILE at run time", async () => { + const writeSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + setProfile(undefined); + process.env.OMP_PROFILE = "work"; + delete process.env.PI_PROFILE; + + await runCli(["--version"]); + + expect(process.exitCode).toBe(0); + expect(writeSpy).toHaveBeenCalled(); + expect(getActiveProfile()).toBe("work"); + expect(getAgentDir()).toBe(path.join(os.homedir(), configDir, "profiles", "work", "agent")); + expect(getAgentDbPath()).toBe(path.join(os.homedir(), configDir, "profiles", "work", "agent", "agent.db")); + }); + + it("accepts the profile flag after other root flags", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runCli(["--version", "--profile", "office"]); + + expect(process.exitCode).toBe(0); + expect(getActiveProfile()).toBe("office"); + expect(getAgentDir()).toBe(path.join(os.homedir(), configDir, "profiles", "office", "agent")); + }); + + it("installs a shell alias and exits before command dispatch", async () => { + const installSpy = vi.spyOn(profileAliasCli, "installProfileAlias").mockResolvedValue({ + shell: "bash", + configPath: "/home/me/.bashrc", + aliasName: "omp-work", + profile: "work", + command: "omp --profile=work", + reloadedWith: ". '/home/me/.bashrc'", + }); + const outSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runCli(["--profile", "work", "--alias", "omp-work", "--version"]); + + expect(process.exitCode).toBe(0); + expect(installSpy).toHaveBeenCalledWith( + expect.objectContaining({ + profile: "work", + aliasName: "omp-work", + }), + ); + const output = outSpy.mock.calls.map(call => String(call[0] ?? "")).join("\n"); + expect(output).toContain("Created omp-work"); + expect(output).not.toContain(`${APP_NAME}/${VERSION}`); + }); + + it("installs a shell alias when launch is explicit", async () => { + const installSpy = vi.spyOn(profileAliasCli, "installProfileAlias").mockResolvedValue({ + shell: "bash", + configPath: "/home/me/.bashrc", + aliasName: "omp-work", + profile: "work", + command: "omp --profile=work", + reloadedWith: ". '/home/me/.bashrc'", + }); + const outSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runCli(["launch", "--profile", "work", "--alias", "omp-work", "--version"]); + + expect(process.exitCode).toBe(0); + expect(installSpy).toHaveBeenCalledWith( + expect.objectContaining({ + profile: "work", + aliasName: "omp-work", + }), + ); + const output = outSpy.mock.calls.map(call => String(call[0] ?? "")).join("\n"); + expect(output).toContain("Created omp-work"); + expect(output).not.toContain(`${APP_NAME}/${VERSION}`); + }); + + it("installs a shell alias when acp is explicit", async () => { + const installSpy = vi.spyOn(profileAliasCli, "installProfileAlias").mockResolvedValue({ + shell: "bash", + configPath: "/home/me/.bashrc", + aliasName: "omp-work", + profile: "work", + command: "omp --profile=work", + reloadedWith: ". '/home/me/.bashrc'", + }); + const outSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runCli(["acp", "--profile", "work", "--alias", "omp-work", "--version"]); + + expect(process.exitCode).toBe(0); + expect(installSpy).toHaveBeenCalledWith( + expect.objectContaining({ + profile: "work", + aliasName: "omp-work", + }), + ); + expect(getActiveProfile()).toBe("work"); + const output = outSpy.mock.calls.map(call => String(call[0] ?? "")).join("\n"); + expect(output).toContain("Created omp-work"); + expect(output).not.toContain(`${APP_NAME}/${VERSION}`); + }); + + it("rejects missing profile values without dispatching", async () => { + const errSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + const outSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runCli(["--profile", "--version"]); + + expect(process.exitCode).toBe(1); + expect(errSpy.mock.calls.map(call => String(call[0] ?? "")).join("\n")).toContain( + "--profile requires a profile name", + ); + expect(outSpy).not.toHaveBeenCalled(); + }); + + it("loads profile agent .env before command modules import pi-utils env", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-profile-cli-env-")); + try { + const home = path.join(root, "home"); + const configDir = ".omp-profile-cli-env"; + const defaultAgentDir = path.join(home, configDir, "agent"); + const profileAgentDir = path.join(home, configDir, "profiles", "work", "agent"); + await fs.mkdir(defaultAgentDir, { recursive: true }); + await fs.mkdir(profileAgentDir, { recursive: true }); + await Bun.write(path.join(defaultAgentDir, ".env"), "OMP_PROFILE_BOOTSTRAP_SENTINEL=default\n"); + await Bun.write(path.join(profileAgentDir, ".env"), "OMP_PROFILE_BOOTSTRAP_SENTINEL=work\n"); + + const probePath = path.join(root, "probe.ts"); + await Bun.write( + probePath, + [ + `import { runCli } from ${JSON.stringify(url.pathToFileURL(cliEntry).href)};`, + 'await runCli(["--profile", "work", "--help"]);', + 'process.stdout.write("\\nSENTINEL=" + (Bun.env.OMP_PROFILE_BOOTSTRAP_SENTINEL ?? ""));', + ].join("\n"), + ); + + const childEnv: Record = { + ...process.env, + HOME: home, + PI_CONFIG_DIR: configDir, + PI_NO_TITLE: "1", + NO_COLOR: "1", + }; + delete childEnv.OMP_PROFILE; + delete childEnv.PI_PROFILE; + delete childEnv.PI_CODING_AGENT_DIR; + delete childEnv.OMP_PROFILE_BOOTSTRAP_SENTINEL; + + const proc = Bun.spawn([process.execPath, probePath], { + cwd: repoRoot, + stdout: "pipe", + stderr: "pipe", + env: childEnv, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + readStream(proc.stdout as ReadableStream), + readStream(proc.stderr as ReadableStream), + proc.exited, + ]); + + expect(exitCode, stderr).toBe(0); + expect(stdout).toContain("SENTINEL=work"); + expect(stdout).not.toContain("SENTINEL=default"); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it("surfaces an invalid OMP_PROFILE env as a clean error, not an import crash", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-profile-cli-env-bad-")); + try { + const home = path.join(root, "home"); + await fs.mkdir(home, { recursive: true }); + + const probePath = path.join(root, "probe.ts"); + await Bun.write( + probePath, + [ + `import { runCli } from ${JSON.stringify(url.pathToFileURL(cliEntry).href)};`, + 'await runCli(["--version"]);', + // Reached only if the module import did NOT throw — i.e. the invalid + // env was deferred to runCli's error handler instead of crashing the + // process during the static import of dirs.ts. + 'process.stdout.write("HANDLED");', + ].join("\n"), + ); + + const childEnv: Record = { + ...process.env, + HOME: home, + PI_CONFIG_DIR: ".omp-profile-cli-env-bad", + OMP_PROFILE: "..", + NO_COLOR: "1", + }; + delete childEnv.PI_PROFILE; + delete childEnv.PI_CODING_AGENT_DIR; + + const proc = Bun.spawn([process.execPath, probePath], { + cwd: repoRoot, + stdout: "pipe", + stderr: "pipe", + env: childEnv, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + readStream(proc.stdout as ReadableStream), + readStream(proc.stderr as ReadableStream), + proc.exited, + ]); + + expect(stdout, stderr).toContain("HANDLED"); + expect(stderr).toContain("Invalid OMP profile"); + expect(exitCode).toBe(1); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/coding-agent/test/tools/browser-stealth-targets.test.ts b/packages/coding-agent/test/tools/browser-stealth-targets.test.ts index 7fb3f14dd..2f726b9a2 100644 --- a/packages/coding-agent/test/tools/browser-stealth-targets.test.ts +++ b/packages/coding-agent/test/tools/browser-stealth-targets.test.ts @@ -204,6 +204,6 @@ describe("browser stealth target setup", () => { const elapsed = performance.now() - started; expect(elapsed).toBeGreaterThanOrEqual(45); - expect(elapsed).toBeLessThan(150); + expect(elapsed).toBeLessThan(500); }); }); diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 9b510acff..6b09b0471 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -41,6 +41,7 @@ - Fixed `blockRangeAt` (and thus the edit tool's `replace block` / `insert after block` ops) failing on extensionless shell rc/profile files. `Path::extension` returns `None` for both bare (`zshrc`) and dotfile (`.zshrc`, `.bashrc`) forms, so language inference fell through to "unrecognized" and block resolution was permanently unresolvable on those files — an agent retrying the block op would loop on the same error. Known shell rc/profile basenames (`zshrc`/`zshenv`/`zprofile`/`zlogin`/`zlogout`/`bashrc`/`bash_profile`/`bash_login`/`bash_logout`/`bash_aliases`/`profile`/`kshrc`/`mkshrc`/`shrc`, with or without a leading dot) now resolve to the bash grammar. - Fixed native crash-log directory resolution diverging from the JS logger when `PI_CONFIG_DIR` is absolute: the config root now mirrors `path.join(homedir, PI_CONFIG_DIR)` semantics (absolute values re-rooted under `$HOME`, `.`/`..` components normalized), and an empty `PI_CODING_AGENT_DIR` no longer disables XDG state-dir resolution. - Fixed shell-output minimization condensing `pyright`/`basedpyright` `--outputjson` runs into a diagnostics summary; machine-readable JSON output now passes through untouched. +- Fixed Linux native builds hard-failing when `RUSTC_WRAPPER=sccache` points at an unavailable shared cache backend. The native build script now retries the `napi` build once without the sccache wrapper after a cache-storage startup failure, so install smoke tests and local fallback builds can proceed while preserving the cached fast path when the backend is healthy. - Fixed `pi-natives` aborting Bun on Windows with `memory allocation of N bytes failed` and no backtrace whenever the native cdylib hit a Rust panic or out-of-memory condition. The release profile uses `panic = "abort"`, so neither default handler emitted any context — Bun received only the bare message and tore down the TUI session before flushing. Module load now installs `std::panic::set_hook` and `std::alloc::set_alloc_error_hook` via `#[napi::module_init]`; both hooks capture `Backtrace::force_capture()` (so it works without `RUST_BACKTRACE=1`) and write a structured report — pid, thread, size/alignment for OOM, source location and message for panics, full backtrace — to the same logs directory the JS logger uses (`$XDG_STATE_HOME/omp/logs/` on Linux/macOS when the user has migrated to XDG and `PI_CODING_AGENT_DIR` isn't customized, otherwise `~/.omp/logs/`) and to stderr before the host process exits. The OOM hook prints the canonical allocation-failure line before any allocation-prone diagnostics and aborts immediately on re-entry, so real process-wide OOM still surfaces the fallback message instead of recursing in the report path ([#2211](https://github.com/can1357/oh-my-pi/issues/2211)). - Fixed cross-line grep being a silent no-op on real files: `multiline` set the `(?m)` flag on the regex matcher but never enabled `multi_line` on the `Searcher`, which stayed line-oriented, so any pattern spanning a `\n` returned zero matches with no error. - Fixed the native `copyToClipboard` leaving the X11 clipboard empty on Linux even while the process kept running. arboard answers clipboard `SelectionRequest`s from a background thread that lives only as long as a `Clipboard` instance exists, and the binding dropped its transient `Clipboard` immediately after `set_text` — tearing that thread down so the selection lost its owner and the clipboard read back empty (matching the `returned ok but clipboard=''` symptom). The Linux path now holds a single `Clipboard` for the lifetime of the process so the owner thread keeps serving, with no `xclip`/`wl-copy` subprocess; macOS/Windows keep the transient write on the calling thread ([#2075](https://github.com/can1357/oh-my-pi/issues/2075)). @@ -49,7 +50,6 @@ - Fixed `wrapTextWithAnsi` hanging (infinite loop) on text containing a BEL-terminated string escape — DCS/SOS/PM/APC (`ESC P`/`ESC X`/`ESC ^`/`ESC _`) closed by `BEL` instead of `ST`. `ansi_seq_len_u16` only accepted the `ST` (`ESC \`) terminator for these (OSC already accepted both), so a BEL-terminated APC such as the TUI cursor marker (`ESC _ pi:c BEL`) was left unclassified: it was miscounted as visible width and `break_long_word`'s non-ESC scan could not advance past the `ESC`, spinning forever. The terminator set now matches OSC (ST **or** BEL), and `break_long_word` defensively emits and steps over any escape it cannot classify so a malformed/unknown sequence can never wedge the wrap loop. - Fixed an interactive shell inside a **pipeline** (`zsh -i ... | awk`, `time zsh -i | cat`, etc.) suspending the embedded host with `suspended (tty input)`. The earlier embedded-host fix `setsid`-detached external children so they could not seize the host's controlling tty, but carved pipeline stages out because a later stage that `setpgid`-joined a detached leader failed with EPERM — leaving every pipeline stage in the host session, where an interactive child opened `/dev/tty`, `tcsetpgrp`'d itself to the foreground, and stopped the host (OMP) on its next tty read. `pi_shell` now detaches pipeline stages too: `child_session_action` returns `DetachSession` for any non-terminal-stdin child regardless of pipeline membership, and `execute_external_command` skips `process_group(...)` entirely for detached children so no cross-session `setpgid` is attempted. Pipeline stages no longer share one process group, which the embedded host does not rely on (cancellation walks the descendant tree and pipes are session-independent). - Fixed shell cancellation cleanup failing to reap child processes inside containers whose guest kernel was built without `CONFIG_PROC_CHILDREN` (e.g. some Kata/microVM guests): the Linux descendant walk relied solely on `/proc//task//children`, which does not exist there, so `children()` / `live_descendants()` returned empty and termination waves never reached the children. It now falls back to scanning `/proc` and grouping by parent pid (the primitive the macOS path already uses) when no `children` file is readable, keeping the cheap per-task fast path on kernels that support it. - ## [15.13.0] - 2026-06-14 ## [15.12.6] - 2026-06-14 diff --git a/packages/natives/scripts/build-native.ts b/packages/natives/scripts/build-native.ts index edb56b4f9..f19f09b05 100644 --- a/packages/natives/scripts/build-native.ts +++ b/packages/natives/scripts/build-native.ts @@ -333,10 +333,32 @@ if (!napiBin) { throw new Error("Could not locate @napi-rs/cli `napi` binary in node_modules/.bin"); } +async function runNapiBuildWithSccacheFallback() { + let buildResult = await $`${napiBin} ${napiArgs}`.nothrow(); + let stderr = buildResult.stderr?.toString("utf-8") ?? ""; + if ( + buildResult.exitCode !== 0 && + process.env.RUSTC_WRAPPER === "sccache" && + stderr.includes("sccache: error") && + stderr.includes("cache storage failed") + ) { + const retryEnv = { ...process.env }; + delete retryEnv.RUSTC_WRAPPER; + delete retryEnv.SCCACHE_BUCKET; + delete retryEnv.SCCACHE_ENDPOINT; + delete retryEnv.SCCACHE_REGION; + delete retryEnv.AWS_ACCESS_KEY_ID; + delete retryEnv.AWS_SECRET_ACCESS_KEY; + console.log("sccache storage unavailable; retrying native build without RUSTC_WRAPPER"); + buildResult = await $`${napiBin} ${napiArgs}`.env(retryEnv).nothrow(); + stderr = buildResult.stderr?.toString("utf-8") ?? ""; + } + return { buildResult, stderr }; +} + try { - const buildResult = await $`${napiBin} ${napiArgs}`.nothrow(); + const { buildResult, stderr } = await runNapiBuildWithSccacheFallback(); if (buildResult.exitCode !== 0) { - const stderr = buildResult.stderr?.toString("utf-8") ?? ""; throw new Error(`napi build failed${stderr ? `:\n${stderr}` : ""}`); } diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index 4e75be04b..fc2632f9c 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -4,6 +4,18 @@ ### Added +- Added profile-aware directory helpers and isolated profile state roots, while keeping the install ID shared across profiles. + +## [15.12.4] - 2026-06-13 + +### Fixed + +- Fixed abortable stream wrappers to cancel the source stream on abort, so timeout watchdogs release upstream HTTP bodies instead of only stopping the local reader. + +## [15.12.0] - 2026-06-12 + +### Added + - Added support for a runtime `overrides` map in `RuntimeInstallSpec`, which is now written into generated runtime `package.json` manifests to force dependency pins (including transitive ones) across the runtime tree - Added a lightweight loop-phase breadcrumb stack (`pushLoopPhase`/`popLoopPhase`/`currentLoopPhase`, plus `takeRecentLoopPhase` which returns the live phase or the most recently popped one and clears it) so the TUI event-loop watchdog can attribute a main-thread block to the phase that caused it — including a synchronous phase already popped before the watchdog's delayed tick runs ([#2485](https://github.com/can1357/oh-my-pi/issues/2485)) - Added `FetchWithRetryOptions.timeout` (forwarded to the underlying `fetch` call). `false` disables Bun's native ~300s pre-response timeout; a positive number overrides the ceiling. Bare browser/Node fetch ignores it ([#2422](https://github.com/can1357/oh-my-pi/issues/2422)) @@ -13,7 +25,7 @@ - Added the `path-tree` module (`buildPathTree`, `walkPathTree`, `formatGroupedPaths`, `isUrlLikePath`), moved from the coding agent's grouped file output so compaction file lists can share the same prefix-folded directory-tree rendering; `formatGroupedPaths` gains an optional `annotate` callback for per-file suffixes - Restored `PI_DEBUG_STARTUP` streaming startup markers: `logger.time` now writes a synchronous `[startup] :start` / `:done` / `:fail` stderr line per phase (independent of `PI_TIMING`), so a startup that hangs hard still names the phase it is stuck in — the `PI_TIMING` tree only prints after startup completes and is structurally unable to diagnose a hang. The CLI runner emits `cli:load:` markers around each lazily-imported command module for the same reason. - Added `logger.openSpanPath()`: ops of the currently-open timing-span chain (root → deepest), used by the coding agent's startup watchdog to name the in-flight phase of a stalled startup. -- Added `declareWorkerHostEntry()` / `workerHostEntry()` (env): self-dispatching CLI entrypoints declare `Bun.main` as the worker host so worker spawn sites can re-enter the single entry module with `WorkerOptions.argv` selectors across source, npm-bundle, and compiled distributions +- Added `declareWorkerHostEntry()` / `workerHostEntry()` in the side-effect-free `worker-host` module (also re-exported from `env`): self-dispatching CLI entrypoints declare `Bun.main` as the worker host so worker spawn sites can re-enter the single entry module with `WorkerOptions.argv` selectors across source, npm-bundle, and compiled distributions - Added `getAuthBrokerSnapshotCachePath()` with `OMP_AUTH_BROKER_SNAPSHOT_CACHE` override support for isolating the encrypted broker snapshot cache. - Added color helpers `colorLuma` (perceptual luma), `relativeLuminance` (WCAG, linearized sRGB), and `hslToHex` to the color utilities. The luminance helpers parse `#rgb`/`#rrggbb` hex and 256-color palette indices, returning `undefined` for unparseable values. - Added `peekFileEnds`, a single-open head-and-tail file peek helper that reuses the head bytes for the tail when the file fits the head window. diff --git a/packages/utils/README.md b/packages/utils/README.md index 9cdb2c4cd..fb5f308a7 100644 --- a/packages/utils/README.md +++ b/packages/utils/README.md @@ -15,7 +15,7 @@ Shared utilities for [oh-my-pi](https://github.com/can1357/oh-my-pi) packages. Z | `which` | `$which()` binary lookup with caching | | `fetch-retry` | `fetch` with retry/backoff policies | | `fs-error` | Errno guards (`isEnoent` and friends) | -| `env` | Environment plumbing, worker-host entry contract (`workerHostEntry`) | +| `env` / `worker-host` | Environment plumbing and side-effect-free worker-host entry contract (`workerHostEntry`) | | `abortable` / `async` | AbortSignal-aware stream/promise helpers | | `peek-file` | Read the first N bytes of a file with pooled buffers | | `frontmatter`, `glob`, `mime`, `temp`, `format`, `color`, `snowflake`, `tab-spacing`, `path-tree`, `sanitize-text` | Smaller single-purpose helpers | diff --git a/packages/utils/src/dirs.ts b/packages/utils/src/dirs.ts index de038f783..445fdc7a4 100644 --- a/packages/utils/src/dirs.ts +++ b/packages/utils/src/dirs.ts @@ -28,6 +28,102 @@ export const VERSION: string = version; /** Minimum Bun version */ export const MIN_BUN_VERSION: string = engines.bun.replace(/[^0-9.]/g, ""); +const PROFILE_NAME_RE = /^[a-z0-9][a-z0-9._-]{0,63}$/; +const PROFILE_ENV_KEYS = ["OMP_PROFILE", "PI_PROFILE"] as const; + +/** + * Names Windows treats as reserved device aliases. Matches the basename + * itself as well as any `BASENAME.` form, because Windows reserves + * `CON.foo`/`PRN.txt`/etc. too — using them as a profile name would let + * `setProfile` accept the input only for directory creation to fail later + * with a confusing `ENOENT`/`EINVAL`. Case-insensitive: NTFS treats `CON` + * and `con` identically. + */ +const WINDOWS_RESERVED_BASENAME_RE = /^(?:CON|PRN|AUX|NUL|COM[0-9]|LPT[0-9])(?:\..*)?$/i; + +/** + * Normalize and validate a profile name. Returns `undefined` for the implicit + * default (empty string, whitespace, or the explicit "default" sentinel) and + * throws for syntactically invalid or platform-reserved names. + * + * Exported so consumers of `@oh-my-pi/pi-utils/dirs` (CLI bootstrap, tests, + * downstream tools) can validate user input without re-deriving the rules. + */ +export function normalizeProfileName(profile: string | undefined): string | undefined { + const normalized = profile?.trim(); + if (!normalized || normalized === "default") return undefined; + if ( + normalized === "." || + normalized === ".." || + normalized.endsWith(".") || + !PROFILE_NAME_RE.test(normalized) || + WINDOWS_RESERVED_BASENAME_RE.test(normalized) + ) { + throw new Error( + `Invalid OMP profile "${profile}". Profile names must match ${PROFILE_NAME_RE.source}, ` + + `cannot be "." or "..", cannot end with ".", and cannot be a Windows reserved device name ` + + `(CON, PRN, AUX, NUL, COM0-9, LPT0-9, or any of those with an extension).`, + ); + } + return normalized; +} + +/** + * Resolve the active profile from the two profile env vars. `OMP_PROFILE` is the + * canonical variable and takes precedence; `PI_PROFILE` is the legacy + * compatibility fallback, consulted only when `OMP_PROFILE` is undefined. An + * explicitly-empty `OMP_PROFILE` therefore selects the default profile rather + * than silently inheriting `PI_PROFILE`. Delegates validation/normalization to + * {@link normalizeProfileName} (which throws on a syntactically invalid value). + */ +export function resolveProfileEnv(omp: string | undefined, pi: string | undefined): string | undefined { + return normalizeProfileName(omp !== undefined ? omp : pi); +} + +function getProfileFromEnv(): string | undefined { + return resolveProfileEnv(process.env.OMP_PROFILE, process.env.PI_PROFILE); +} + +/** + * Module-load profile resolution. Unlike {@link getProfileFromEnv}, an invalid + * OMP_PROFILE/PI_PROFILE value does NOT throw here — a bad env var must not + * crash a bare `import` of this module with an uncaught stack trace before the + * CLI's error handling is in scope. The default profile is used instead; the + * CLI re-validates the env (see `runCli` in coding-agent/src/cli.ts) so the + * user still gets a clean "Invalid OMP profile" message. + */ +function readProfileFromEnvSafe(): string | undefined { + try { + return getProfileFromEnv(); + } catch { + return undefined; + } +} + +function getBaseConfigRoot(): string { + return path.join(os.homedir(), getConfigDirName()); +} + +function getProfileConfigRoot(profile: string | undefined): string { + const root = getBaseConfigRoot(); + return profile ? path.join(root, "profiles", profile) : root; +} + +function readPiProfileFromEnvSafe(): string | undefined { + try { + return normalizeProfileName(process.env.PI_PROFILE); + } catch { + return undefined; + } +} + +function getProfileAgentDir(profile: string): string { + return path.join(getProfileConfigRoot(profile), "agent"); +} + +function isProfileDerivedAgentDir(profile: string | undefined, agentDirEnv: string | undefined): boolean { + return profile !== undefined && agentDirEnv === getProfileAgentDir(profile); +} // ============================================================================= // Project directory // ============================================================================= @@ -96,7 +192,8 @@ export function getConfigDirName(): string { /** Get the config agent directory name relative to home (e.g. ".omp/agent" or PI_CONFIG_DIR + "/agent"). */ export function getConfigAgentDirName(): string { - return `${getConfigDirName()}/agent`; + const profile = getActiveProfile(); + return profile ? path.join(getConfigDirName(), "profiles", profile, "agent") : `${getConfigDirName()}/agent`; } // ============================================================================= @@ -123,29 +220,47 @@ class DirResolver { readonly #rootCache = new Map(); readonly #agentCache = new Map(); - constructor(agentDirOverride?: string) { - this.configRoot = path.join(os.homedir(), getConfigDirName()); + constructor(options: { agentDirOverride?: string; profile?: string } = {}) { + const profile = normalizeProfileName(options.profile); + this.configRoot = getProfileConfigRoot(profile); const defaultAgent = path.join(this.configRoot, "agent"); + const agentDirOverride = profile ? undefined : options.agentDirOverride; this.agentDir = agentDirOverride ? path.resolve(agentDirOverride) : defaultAgent; const isDefault = this.agentDir === defaultAgent; - // XDG is a Linux convention. On other platforms, or for non-default - // profiles, all categories resolve to the legacy paths. + // XDG is a Linux convention. On supported platforms, default profile state + // resolves under $XDG_*_HOME/omp once `omp config init-xdg` has migrated + // the user's data. Named profiles follow a stricter rule: the XDG choice + // is keyed on the profile-specific XDG path, never the base app root. + // + // Why: if we consulted the base app root for named profiles too, the same + // profile could resolve to `~/.omp/profiles/` on first activation + // (when no $XDG_*_HOME/omp exists yet) and then silently move to + // `$XDG_*_HOME/omp/profiles/` the moment the base appeared, orphaning + // the earlier state. Pinning on the profile path means a profile's location + // is decided at first activation and stays put until the user explicitly + // migrates it (e.g. by mkdir'ing the XDG profile dir). let xdgData: string | undefined; let xdgState: string | undefined; let xdgCache: string | undefined; if ((process.platform === "linux" || process.platform === "darwin") && isDefault) { const resolveIf = (envVar: string) => { const value = process.env[envVar]; - if (value) { - try { - const joined = path.join(value, APP_NAME); - if (fs.existsSync(joined)) { - return joined; + if (!value) return undefined; + try { + const appRoot = path.join(value, APP_NAME); + if (profile) { + const profilePath = path.join(appRoot, "profiles", profile); + if (fs.existsSync(profilePath)) { + return profilePath; } - } catch {} - } + return undefined; + } + if (fs.existsSync(appRoot)) { + return appRoot; + } + } catch {} return undefined; }; xdgData = resolveIf("XDG_DATA_HOME"); @@ -190,8 +305,49 @@ class DirResolver { } } -let dirs = new DirResolver(process.env.PI_CODING_AGENT_DIR); +/** + * Decide which `PI_CODING_AGENT_DIR` value to capture as the pre-profile + * baseline. A value equal to a profile's derived agent dir is profile-derived + * (propagated by a parent's `setProfile`), so it must NOT be snapshotted as the + * default-mode baseline — otherwise default mode would resolve to the profile's + * agent dir. The profile source can be the active profile or a lower-priority + * `PI_PROFILE` that was bypassed because `OMP_PROFILE` explicitly selected the + * default profile. Returns `undefined` in those cases so reset falls back to the + * standard `~/.omp/agent`. + */ +function resolvePreProfileAgentDir( + profile: string | undefined, + agentDirEnv: string | undefined, + profileAgentDirSource: string | undefined = profile, +): string | undefined { + return isProfileDerivedAgentDir(profile ?? profileAgentDirSource, agentDirEnv) ? undefined : agentDirEnv; +} +let activeProfile = readProfileFromEnvSafe(); +const moduleLoadProfileAgentDirSource = activeProfile ?? readPiProfileFromEnvSafe(); +const moduleLoadAgentDirOverride = activeProfile + ? undefined + : resolvePreProfileAgentDir(undefined, process.env.PI_CODING_AGENT_DIR, moduleLoadProfileAgentDirSource); +let dirs = new DirResolver({ + agentDirOverride: moduleLoadAgentDirOverride, + profile: activeProfile, +}); +/** + * Snapshot of `PI_CODING_AGENT_DIR` from before the first named-profile + * activation. Reset paths restore this value (or its absence) instead of + * unconditionally deleting the env var. Without the snapshot, a process started + * with `PI_CODING_AGENT_DIR=/custom` then `setProfile("work")` then + * `setProfile(undefined)` would silently lose `/custom` and fall back to + * `~/.omp/agent`. Captured at module load — ignoring a profile-derived value + * inherited from a parent's `setProfile` (see {@link resolvePreProfileAgentDir}) + * — and refreshed on `setAgentDir`, since that call is the user explicitly + * redefining the baseline. + */ +let preProfileAgentDirEnv: string | undefined = resolvePreProfileAgentDir( + activeProfile, + process.env.PI_CODING_AGENT_DIR, + moduleLoadProfileAgentDirSource, +); // Anchor home for the resolver. Captured at module load to stay stable across // test mocks of `os.homedir()`. `getPluginsDir(home)` compares against this so // production callers (`home === RESOLVER_HOME`) hit the XDG-aware resolver while @@ -209,10 +365,74 @@ export function getConfigRootDir(): string { /** Set the coding agent directory. Creates a fresh resolver, invalidating all cached paths. */ export function setAgentDir(dir: string): void { - dirs = new DirResolver(dir); + activeProfile = undefined; + dirs = new DirResolver({ agentDirOverride: dir }); process.env.PI_CODING_AGENT_DIR = dir; + preProfileAgentDirEnv = dir; + for (const key of PROFILE_ENV_KEYS) { + delete process.env[key]; + } } +/** + * Test-only: reset the pre-profile `PI_CODING_AGENT_DIR` snapshot to whatever + * the current environment looks like. Cross-suite test pollution can otherwise + * leak a stale snapshot through `setAgentDir` and corrupt `setProfile(undefined)` + * restore semantics. Production code MUST NOT call this — the snapshot's + * lifecycle is owned by `setAgentDir` / `setProfile` and a runtime caller has + * no business clearing it. + */ +export function __resetProfileSnapshotForTests(): void { + preProfileAgentDirEnv = resolvePreProfileAgentDir( + activeProfile, + process.env.PI_CODING_AGENT_DIR, + activeProfile ?? readPiProfileFromEnvSafe(), + ); +} + +/** Activate a named profile. Passing undefined or "default" returns to the default profile. */ +export function setProfile(profile: string | undefined): void { + const next = normalizeProfileName(profile); + if (next && !activeProfile) { + // First activation of a named profile in this process: snapshot the + // current PI_CODING_AGENT_DIR so a later reset can restore the user's + // explicit override. Subsequent profile switches keep the original + // snapshot — the "pre-profile" baseline is the state before profiles + // entered the picture, not the state between two activations. + preProfileAgentDirEnv = resolvePreProfileAgentDir( + undefined, + process.env.PI_CODING_AGENT_DIR, + readPiProfileFromEnvSafe(), + ); + } + activeProfile = next; + if (activeProfile) { + dirs = new DirResolver({ profile: activeProfile }); + process.env.OMP_PROFILE = activeProfile; + process.env.PI_PROFILE = activeProfile; + process.env.PI_CODING_AGENT_DIR = dirs.agentDir; + } else { + for (const key of PROFILE_ENV_KEYS) { + delete process.env[key]; + } + if (preProfileAgentDirEnv === undefined) { + delete process.env.PI_CODING_AGENT_DIR; + } else { + process.env.PI_CODING_AGENT_DIR = preProfileAgentDirEnv; + } + dirs = new DirResolver({ agentDirOverride: preProfileAgentDirEnv }); + } +} + +/** Get the active named profile. Undefined means the default profile. */ +export function getActiveProfile(): string | undefined { + return activeProfile; +} + +/** Resolve the config root that backs a profile without activating it. */ +export function getProfileRootDir(profile: string | undefined): string { + return getProfileConfigRoot(normalizeProfileName(profile)); +} /** Get the agent config directory (~/.omp/agent). */ export function getAgentDir(): string { return dirs.agentDir; @@ -534,10 +754,15 @@ const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/ * winner's id). Survives independently of agent state: deleting * `~/.omp/agent/` does not regenerate it. Server-side dedup for grievance * pushes (and similar telemetry) keys on this id. + * + * Anchored to the base config root (`~/.omp/install-id`) regardless of the + * active profile: install identity is per-install, not per-profile, so every + * profile shares one id and the global cache stays correct no matter the + * profile / `getInstallId` call order. */ export function getInstallId(): string { if (cachedInstallId) return cachedInstallId; - const filePath = path.join(getConfigRootDir(), INSTALL_ID_FILE); + const filePath = path.join(getBaseConfigRoot(), INSTALL_ID_FILE); let observedInvalid = false; try { diff --git a/packages/utils/src/env.ts b/packages/utils/src/env.ts index 8ba2fc661..9e3bf1349 100644 --- a/packages/utils/src/env.ts +++ b/packages/utils/src/env.ts @@ -3,6 +3,8 @@ import * as os from "node:os"; import * as path from "node:path"; import { getAgentDir, getConfigRootDir } from "./dirs"; +export * from "./worker-host"; + const ENV_NAME_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; /** @@ -172,26 +174,6 @@ export function isCompiledBinary(): boolean { return url.includes("$bunfs") || url.includes("~BUN") || url.includes("%7EBUN"); } -/** - * Main-module path declared by self-dispatching CLI entrypoints — entries - * whose top-level argv handling routes hidden `__omp_*` worker selectors. - * Worker spawn sites re-enter this module via `new Worker(entry, { argv })`, - * so every distribution (source, npm bundle, compiled binary) needs exactly - * one JavaScript entrypoint. Never set under `bun test`, SDK embedding, or - * standalone package bins — those hosts load worker modules directly. - */ -let workerHostMain: string | null = null; - -/** Called by CLI entrypoints whose main module dispatches worker argv selectors. */ -export function declareWorkerHostEntry(): void { - workerHostMain = Bun.main; -} - -/** Main-module path of the self-dispatching CLI host, or null outside it. */ -export function workerHostEntry(): string | null { - return workerHostMain; -} - const TRUTHY: Dict = { "1": true, Y: true, diff --git a/packages/utils/src/worker-host.ts b/packages/utils/src/worker-host.ts new file mode 100644 index 000000000..13eb39d1a --- /dev/null +++ b/packages/utils/src/worker-host.ts @@ -0,0 +1,19 @@ +/** + * Main-module path declared by self-dispatching CLI entrypoints — entries + * whose top-level argv handling routes hidden `__omp_*` worker selectors. + * Worker spawn sites re-enter this module via `new Worker(entry, { argv })`, + * so every distribution (source, npm bundle, compiled binary) needs exactly + * one JavaScript entrypoint. Never set under `bun test`, SDK embedding, or + * standalone package bins — those hosts load worker modules directly. + */ +let workerHostMain: string | null = null; + +/** Called by CLI entrypoints whose main module dispatches worker argv selectors. */ +export function declareWorkerHostEntry(): void { + workerHostMain = Bun.main; +} + +/** Main-module path of the self-dispatching CLI host, or null outside it. */ +export function workerHostEntry(): string | null { + return workerHostMain; +} diff --git a/packages/utils/test/install-id.test.ts b/packages/utils/test/install-id.test.ts index a82fddc11..6cf3c4d45 100644 --- a/packages/utils/test/install-id.test.ts +++ b/packages/utils/test/install-id.test.ts @@ -8,6 +8,7 @@ import { getConfigRootDir, getInstallId, setAgentDir, + setProfile, } from "@oh-my-pi/pi-utils/dirs"; import { Snowflake } from "@oh-my-pi/pi-utils/snowflake"; @@ -75,4 +76,24 @@ describe("getInstallId", () => { const onDisk = (await fs.readFile(path.join(getConfigRootDir(), "install-id"), "utf8")).trim(); expect(onDisk).toBe(id); }); + + it("anchors the install id to the base config root regardless of active profile", async () => { + // Default mode creates the id under the base config root. + const baseId = getInstallId(); + const baseFile = path.join(getConfigRootDir(), "install-id"); + expect((await fs.readFile(baseFile, "utf8")).trim()).toBe(baseId); + + // Activating a profile must not relocate the id or mint a new one: install + // identity is per-install, and the global cache must stay correct. + __resetInstallIdCacheForTests(); + setProfile("work"); + try { + const profileRoot = getConfigRootDir(); + expect(profileRoot).not.toBe(path.dirname(baseFile)); + expect(getInstallId()).toBe(baseId); + expect(await Bun.file(path.join(profileRoot, "install-id")).exists()).toBe(false); + } finally { + setProfile(undefined); + } + }); }); diff --git a/packages/utils/test/profiles.test.ts b/packages/utils/test/profiles.test.ts new file mode 100644 index 000000000..7b02f0352 --- /dev/null +++ b/packages/utils/test/profiles.test.ts @@ -0,0 +1,417 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import * as url from "node:url"; +import { + __resetProfileSnapshotForTests, + getActiveProfile, + getAgentDbPath, + getAgentDir, + getConfigAgentDirName, + getConfigRootDir, + getPythonGatewayDir, + getSessionsDir, + getStatsDbPath, + normalizeProfileName, + resolveProfileEnv, + setAgentDir, + setProfile, +} from "@oh-my-pi/pi-utils/dirs"; +import { Snowflake } from "@oh-my-pi/pi-utils/snowflake"; + +async function readStream(stream: ReadableStream): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let text = ""; + try { + while (true) { + const { value, done } = await reader.read(); + if (done) break; + text += decoder.decode(value, { stream: true }); + } + return text + decoder.decode(); + } finally { + reader.releaseLock(); + } +} + +describe("profile directories", () => { + let tempRoot = ""; + let configDir = ""; + let originalAgentDir = ""; + let originalProfile: string | undefined; + let originalAgentDirEnv: string | undefined; + let originalOmpProfileEnv: string | undefined; + let originalPiProfileEnv: string | undefined; + let originalConfigDir: string | undefined; + let originalXdgDataHome: string | undefined; + let originalXdgStateHome: string | undefined; + let originalXdgCacheHome: string | undefined; + + beforeEach(async () => { + originalAgentDir = getAgentDir(); + originalProfile = getActiveProfile(); + originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; + originalOmpProfileEnv = process.env.OMP_PROFILE; + originalPiProfileEnv = process.env.PI_PROFILE; + originalConfigDir = process.env.PI_CONFIG_DIR; + originalXdgDataHome = process.env.XDG_DATA_HOME; + originalXdgStateHome = process.env.XDG_STATE_HOME; + originalXdgCacheHome = process.env.XDG_CACHE_HOME; + tempRoot = path.join(os.tmpdir(), "pi-utils-profiles", Snowflake.next()); + configDir = `.omp-profile-test-${Snowflake.next()}`; + await fs.mkdir(tempRoot, { recursive: true }); + process.env.PI_CONFIG_DIR = configDir; + // Other suites that run before this one (e.g. dirs-python-gateway) may have + // called `setAgentDir`, which permanently mutates the module-level + // pre-profile snapshot. Reset it here so each test starts from a clean + // `PI_CODING_AGENT_DIR` baseline matching the env we just configured. + delete process.env.PI_CODING_AGENT_DIR; + __resetProfileSnapshotForTests(); + delete process.env.XDG_DATA_HOME; + delete process.env.XDG_STATE_HOME; + delete process.env.XDG_CACHE_HOME; + }); + + afterEach(async () => { + setProfile(undefined); + if (originalConfigDir === undefined) { + delete process.env.PI_CONFIG_DIR; + } else { + process.env.PI_CONFIG_DIR = originalConfigDir; + } + if (originalXdgDataHome === undefined) { + delete process.env.XDG_DATA_HOME; + } else { + process.env.XDG_DATA_HOME = originalXdgDataHome; + } + if (originalXdgStateHome === undefined) { + delete process.env.XDG_STATE_HOME; + } else { + process.env.XDG_STATE_HOME = originalXdgStateHome; + } + if (originalXdgCacheHome === undefined) { + delete process.env.XDG_CACHE_HOME; + } else { + process.env.XDG_CACHE_HOME = originalXdgCacheHome; + } + if (originalProfile) { + setProfile(originalProfile); + } else if (originalAgentDirEnv !== undefined) { + setAgentDir(originalAgentDir); + } else { + setProfile(undefined); + } + if (originalOmpProfileEnv === undefined) { + delete process.env.OMP_PROFILE; + } else { + process.env.OMP_PROFILE = originalOmpProfileEnv; + } + if (originalPiProfileEnv === undefined) { + delete process.env.PI_PROFILE; + } else { + process.env.PI_PROFILE = originalPiProfileEnv; + } + await fs.rm(tempRoot, { recursive: true, force: true }); + await fs.rm(path.join(os.homedir(), configDir), { recursive: true, force: true }); + }); + + it("moves agent and root data under the named profile root", () => { + setProfile("work"); + + const root = path.join(os.homedir(), configDir, "profiles", "work"); + const agent = path.join(root, "agent"); + expect(getActiveProfile()).toBe("work"); + expect(getConfigRootDir()).toBe(root); + expect(getConfigAgentDirName()).toBe(path.join(configDir, "profiles", "work", "agent")); + expect(getAgentDir()).toBe(agent); + expect(getAgentDbPath()).toBe(path.join(agent, "agent.db")); + expect(getSessionsDir()).toBe(path.join(agent, "sessions")); + expect(getStatsDbPath()).toBe(path.join(root, "stats.db")); + }); + + it("treats the default profile as regular mode", () => { + setProfile("default"); + + const root = path.join(os.homedir(), configDir); + expect(getActiveProfile()).toBeUndefined(); + expect(getConfigRootDir()).toBe(root); + expect(getAgentDir()).toBe(path.join(root, "agent")); + }); + + it("keeps XDG-backed named profile state under profile-specific roots", async () => { + if (process.platform === "win32") return; + + process.env.XDG_DATA_HOME = path.join(tempRoot, "data"); + process.env.XDG_STATE_HOME = path.join(tempRoot, "state"); + process.env.XDG_CACHE_HOME = path.join(tempRoot, "cache"); + // Named profiles only adopt XDG when their *own* XDG path already exists, + // so the profile location stays stable across activations. + await fs.mkdir(path.join(process.env.XDG_DATA_HOME, "omp", "profiles", "work"), { recursive: true }); + await fs.mkdir(path.join(process.env.XDG_STATE_HOME, "omp", "profiles", "work"), { recursive: true }); + await fs.mkdir(path.join(process.env.XDG_CACHE_HOME, "omp", "profiles", "work"), { recursive: true }); + + setProfile("work"); + + expect(getAgentDbPath()).toBe(path.join(process.env.XDG_DATA_HOME, "omp", "profiles", "work", "agent.db")); + expect(getSessionsDir()).toBe(path.join(process.env.XDG_DATA_HOME, "omp", "profiles", "work", "sessions")); + expect(getPythonGatewayDir()).toBe( + path.join(process.env.XDG_STATE_HOME, "omp", "profiles", "work", "python-gateway"), + ); + }); + + it("does not silently switch a named profile to XDG once the base app dir appears", async () => { + if (process.platform === "win32") return; + + process.env.XDG_DATA_HOME = path.join(tempRoot, "data"); + process.env.XDG_STATE_HOME = path.join(tempRoot, "state"); + process.env.XDG_CACHE_HOME = path.join(tempRoot, "cache"); + + // Fresh install: XDG vars are set (typical Linux) but no $XDG/omp exists yet. + // First activation must land in ~//profiles/work because + // the profile-specific XDG path does not exist. + setProfile("work"); + const firstAgentDir = getAgentDir(); + expect(firstAgentDir).toBe(path.join(os.homedir(), configDir, "profiles", "work", "agent")); + + // Later, the base XDG app dir materializes (e.g. via `omp config init-xdg` + // migrating only the default-profile data). The named profile must stay + // in its original location until the user explicitly migrates it. + await fs.mkdir(path.join(process.env.XDG_DATA_HOME, "omp"), { recursive: true }); + await fs.mkdir(path.join(process.env.XDG_STATE_HOME, "omp"), { recursive: true }); + await fs.mkdir(path.join(process.env.XDG_CACHE_HOME, "omp"), { recursive: true }); + + setProfile(undefined); + setProfile("work"); + expect(getAgentDir()).toBe(firstAgentDir); + }); + + it("rejects path-like profile names", () => { + expect(() => setProfile("../work")).toThrow("Invalid OMP profile"); + expect(() => setProfile("work/team")).toThrow("Invalid OMP profile"); + }); + + it("rejects trailing-dot profile names to avoid Windows path collisions", () => { + for (const name of ["work.", "work.."]) { + expect(() => setProfile(name)).toThrow("cannot end with"); + } + }); + + it("restores the pre-profile PI_CODING_AGENT_DIR override on reset", () => { + const customAgentDir = path.join(tempRoot, "custom-agent"); + setAgentDir(customAgentDir); + expect(getAgentDir()).toBe(customAgentDir); + expect(process.env.PI_CODING_AGENT_DIR).toBe(customAgentDir); + + setProfile("work"); + expect(getActiveProfile()).toBe("work"); + expect(getAgentDir()).not.toBe(customAgentDir); + + setProfile(undefined); + expect(getActiveProfile()).toBeUndefined(); + // Critical: reset must restore the user's override, not delete it. + expect(process.env.PI_CODING_AGENT_DIR).toBe(customAgentDir); + expect(getAgentDir()).toBe(customAgentDir); + }); + + it("clears PI_CODING_AGENT_DIR on reset when nothing was set originally", () => { + delete process.env.PI_CODING_AGENT_DIR; + // Force a baseline snapshot of "no override" via setProfile so a stale + // module-load snapshot from a previous test cannot leak in. + setProfile("work"); + setProfile(undefined); + expect(process.env.PI_CODING_AGENT_DIR).toBeUndefined(); + }); + + it("rejects Windows reserved device names case-insensitively", () => { + for (const name of ["CON", "con", "PRN", "AUX", "NUL", "COM0", "COM9", "lpt1", "LPT9", "CON.txt", "com1.bak"]) { + expect(() => setProfile(name)).toThrow("Windows reserved device name"); + } + }); + + it("does not restore a profile-derived agent dir as the default baseline", () => { + // Reproduces a child process that inherited OMP_PROFILE=work plus the + // profile-derived PI_CODING_AGENT_DIR that setProfile propagates to + // children. The module-load snapshot must not capture that profile dir as + // the default baseline, or setProfile(undefined) would resolve default + // mode into the work profile's agent dir. + setProfile("work"); + const workAgentDir = path.join(os.homedir(), configDir, "profiles", "work", "agent"); + expect(getAgentDir()).toBe(workAgentDir); + expect(process.env.PI_CODING_AGENT_DIR).toBe(workAgentDir); + + // Re-snapshot exactly as module load would, now that OMP_PROFILE and the + // profile-derived PI_CODING_AGENT_DIR are present in the environment. + __resetProfileSnapshotForTests(); + + setProfile(undefined); + expect(getActiveProfile()).toBeUndefined(); + expect(process.env.PI_CODING_AGENT_DIR).toBeUndefined(); + expect(getAgentDir()).toBe(path.join(os.homedir(), configDir, "agent")); + }); +}); + +describe("profile env + name validation", () => { + it("honors OMP_PROFILE precedence and treats empty/default as the default profile", () => { + // OMP_PROFILE is canonical and wins over the legacy PI_PROFILE fallback. + expect(resolveProfileEnv("work", "other")).toBe("work"); + // PI_PROFILE is consulted only when OMP_PROFILE is undefined. + expect(resolveProfileEnv(undefined, "work")).toBe("work"); + // An explicitly-empty OMP_PROFILE selects the default profile; it must NOT + // fall through to the lower-precedence PI_PROFILE. + expect(resolveProfileEnv("", "work")).toBeUndefined(); + expect(resolveProfileEnv(" ", "work")).toBeUndefined(); + expect(resolveProfileEnv("default", "work")).toBeUndefined(); + expect(resolveProfileEnv(undefined, undefined)).toBeUndefined(); + }); + + it("rejects uppercase profile names so isolation is filesystem-independent", () => { + // `work` and `WORK` would collide on case-insensitive macOS/Windows but + // differ on Linux; reject uppercase to keep profile identity stable. + expect(() => normalizeProfileName("WORK")).toThrow("Invalid OMP profile"); + expect(() => normalizeProfileName("Work")).toThrow("Invalid OMP profile"); + expect(normalizeProfileName("work")).toBe("work"); + expect(normalizeProfileName("work-2.0_a")).toBe("work-2.0_a"); + }); +}); + +describe("dirs module import behavior", () => { + it("does not scrub inherited macOS malloc logging env variables on import", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "pi-utils-dirs-import-")); + try { + const probePath = path.join(root, "probe.ts"); + const dirsUrl = url.pathToFileURL(path.join(import.meta.dir, "..", "src", "dirs.ts")).href; + await Bun.write( + probePath, + [ + `import ${JSON.stringify(dirsUrl)};`, + "process.stdout.write(JSON.stringify({", + " malloc: process.env.MallocStackLogging,", + " compact: process.env.MallocStackLoggingNoCompact,", + "}));", + ].join("\n"), + ); + + const childEnv: Record = { + ...process.env, + MallocStackLogging: "0", + MallocStackLoggingNoCompact: "0", + }; + const proc = Bun.spawn([process.execPath, probePath], { + stdout: "pipe", + stderr: "pipe", + env: childEnv, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + readStream(proc.stdout as ReadableStream), + readStream(proc.stderr as ReadableStream), + proc.exited, + ]); + + expect(exitCode, stderr).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + malloc: "0", + compact: "0", + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + it("exposes worker-host without loading agent env", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "pi-utils-worker-host-import-")); + try { + const workerHostUrl = import.meta.resolve("@oh-my-pi/pi-utils/worker-host"); + const agentDir = path.join(root, "agent"); + await fs.mkdir(agentDir, { recursive: true }); + await Bun.write(path.join(agentDir, ".env"), "OMP_WORKER_HOST_PROBE=from-agent-env\n"); + const probePath = path.join(root, "probe.ts"); + await Bun.write( + probePath, + [ + `import { declareWorkerHostEntry, workerHostEntry } from ${JSON.stringify(workerHostUrl)};`, + "declareWorkerHostEntry();", + "process.stdout.write(JSON.stringify({", + " envProbe: process.env.OMP_WORKER_HOST_PROBE ?? null,", + " hostDeclared: workerHostEntry() === Bun.main,", + "}));", + ].join("\n"), + ); + + const childEnv: Record = { + ...process.env, + PI_CODING_AGENT_DIR: agentDir, + }; + delete childEnv.OMP_WORKER_HOST_PROBE; + const proc = Bun.spawn([process.execPath, probePath], { + stdout: "pipe", + stderr: "pipe", + env: childEnv, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + readStream(proc.stdout as ReadableStream), + readStream(proc.stderr as ReadableStream), + proc.exited, + ]); + + expect(exitCode, stderr).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + envProbe: null, + hostDeclared: true, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it("ignores inherited profile agent dir when OMP_PROFILE explicitly selects default", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "pi-utils-dirs-default-profile-")); + const probeConfigDir = `.omp-default-profile-${Snowflake.next()}`; + try { + const dirsUrl = url.pathToFileURL(path.join(import.meta.dir, "..", "src", "dirs.ts")).href; + const workAgentDir = path.join(os.homedir(), probeConfigDir, "profiles", "work", "agent"); + const defaultAgentDir = path.join(os.homedir(), probeConfigDir, "agent"); + + for (const ompProfile of ["", "default"]) { + const probePath = path.join(root, `default-profile-${ompProfile || "empty"}.ts`); + await Bun.write( + probePath, + [ + `import { getActiveProfile, getAgentDir } from ${JSON.stringify(dirsUrl)};`, + "process.stdout.write(JSON.stringify({", + " activeProfile: getActiveProfile() ?? null,", + " agentDir: getAgentDir(),", + "}));", + ].join("\n"), + ); + + const childEnv: Record = { + ...process.env, + PI_CONFIG_DIR: probeConfigDir, + OMP_PROFILE: ompProfile, + PI_PROFILE: "work", + PI_CODING_AGENT_DIR: workAgentDir, + }; + const proc = Bun.spawn([process.execPath, probePath], { + stdout: "pipe", + stderr: "pipe", + env: childEnv, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + readStream(proc.stdout as ReadableStream), + readStream(proc.stderr as ReadableStream), + proc.exited, + ]); + + expect(exitCode, stderr).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + activeProfile: null, + agentDir: defaultAgentDir, + }); + } + } finally { + await fs.rm(root, { recursive: true, force: true }); + await fs.rm(path.join(os.homedir(), probeConfigDir), { recursive: true, force: true }); + } + }); +});