Commit Graph

21 Commits

Author SHA1 Message Date
roboomp a98dc285a3 fix(tests): isolated CI-environment assumptions across the coding-agent suite
The CI gate failed in several non-overlapping ways once the full coding-agent suite ran here: tests wrote into the real $HOME (`/srv/agent-home`) which is read-only, a fixture rotated stored Anthropic API keys but Settings reloaded the user models.yml and shadowed them, an OAuth callback server bound to `hostname:"localhost"` (loopback unreachable in this runtime), a built-in tool metadata assertion only saw `github` when `gh` was installed, and the GithubTool `pr_checkout` worktree assertions assumed `~/.omp/wt` but `XDG_DATA_HOME` redirected `getWorktreesDir()` to `$XDG_DATA_HOME/omp/wt`.

Fixes:

- `packages/ai/src/registry/oauth/callback-server.ts`: drop `hostname: "localhost"` when no caller-supplied hostname overrides it. Bun on Linux refused inbound connections to the listener when bound explicitly to `localhost`; defaulting to Bun.serves default binding restores loopback connectivity.

- `packages/coding-agent/test/status-line-path.test.ts`: route the `~/Projects` fixtures through a writable temp home (spy `os.homedir()`), housed under the repo `.wt/` worktree scratch so the temp home is not classified as a status-line scratch root.

- `packages/coding-agent/test/skills.test.ts`: ditto for the `~/.pi-skills-test-*` mkdtemp in the tilde-expansion test.

- `packages/coding-agent/test/marketplace/project-scope.test.ts`: stop writing `~/.git`; build the entire home-dir guard fixture in a temp dir and spy `os.homedir()`.

- `packages/coding-agent/test/oauth-flow.test.ts`: wrap each callback fetch in a brief retry so the simulated browser redirect tolerates the few-ms gap before the Bun callback server starts accepting connections.

- `packages/coding-agent/test/tools/gh.test.ts`: extend `setupTempHome()` to clear `XDG_DATA_HOME`/`XDG_STATE_HOME`/`XDG_CACHE_HOME` for the duration of the test so the rebuilt dirs resolver routes `getWorktreesDir()` back through the spied home, then restore them on cleanup.

- `packages/coding-agent/test/tool-discovery/initial-tools.test.ts`: instantiate `GithubTool` directly in the metadata fixture so the assertion runs even when `gh` is unavailable (GithubTool.createIf returns null without `gh`).

- `packages/coding-agent/test/agent-session-retry-cap.test.ts`: pass an isolated `models.yml` path to `ModelRegistry` so the two-Anthropic-key fixture is the authoritative credential source instead of any user-level command-backed Anthropic key.

Verification:

- `bun check` → passed

- `bun run test` (full coding-agent suite, 4 buckets, all chunks) → 0 fails

Fixes #3639
2026-06-27 11:15:44 +02:00
roboomp 132531ee47 fix(mcp/oauth): preserve advertised origin resource indicators
Review on PR #3503 caught the last provenance edge case: some servers can
explicitly advertise an origin-only resource equal to the authorization-server
origin. That value is still authoritative provider metadata and must be sent;
only OMP-synthesized fallback resources should be stripped.

- `filterResourceIndicator` now strips same-origin values only when `stripSameOriginResource` is set. Provider-advertised `oauth.resource` and authorization-URL `?resource=` values preserve both origin-only and path-scoped forms.
- Updated grant tests to preserve advertised origin resources, trailing-slash origin resources, and URL-embedded origin resources while still stripping fallback origin/path resources for Plane.
- Updated refresh tests to preserve advertised origin resources and strip only fallback origin/path resources.
- Updated changelog wording to describe fallback-only stripping.

Fixes #3502
2026-06-25 22:04:49 +00:00
roboomp 3c3a552d49 fix(mcp/oauth): preserve authorization-url embedded path resources
Review on PR #3503 caught one remaining provenance hole: a provider can embed a
path-scoped same-host `resource` directly in the authorization URL while
`oauth.resource` remains undefined. The controller marks its separate
`config.url` resource as fallback, but the URL-embedded parameter is still
provider-authored and must not inherit the fallback same-origin stripping
policy.

- `generateAuthUrl` now filters an existing `?resource=` from the authorization URL with the path-preserving default, regardless of `stripSameOriginResource` on the caller-supplied fallback resource.
- Added a regression that simulates `authorize?resource=https://gateway.example.com/svc/mcp` plus fallback `resource=https://gateway.example.com`; the authorize URL, `flow.resource`, and token request all preserve `/svc/mcp`.
- Updated the changelog to explicitly mention authorization-URL-embedded path resources.

Fixes #3502
2026-06-25 21:55:59 +00:00
roboomp 80c0beb84a fix(mcp/oauth): preserve advertised path-scoped resource indicators
Review on PR #3503 caught that the broad same-origin filter broke valid
protected-resource discovery shapes such as
`https://gateway.example.com/my-service/mcp`: same host as the authorization
server, but a distinct MCP service identified by path. Those advertised
resources must be preserved for audience selection.

- Replaced the unconditional same-origin filter with a provenance-aware policy: exact auth-server-origin resources are always stripped, path-scoped same-origin resources are preserved by default, and only OMP-synthesized fallback resources opt into same-origin path stripping.
- Added `stripSameOriginResource` to `MCPOAuthConfig` and `RefreshMCPOAuthTokenOptions`; quick-add/reauth set it only when the resource came from `config.url` / `runtimeBaseConfig.url` fallback rather than `oauth.resource` or an existing auth resource.
- Refresh uses the same flag when `MCPManager.prepareConfig` falls back to `config.url`, and no longer persists fallback resources into the credential as if they were provider-advertised material.
- Updated RFC 8707 tests to cover both sides: gateway path resource preserved, Plane-style fallback `/http/mcp` stripped, refresh path mirrors the same distinction.

Fixes #3502
2026-06-25 21:48:14 +00:00
roboomp f7fa80e00e fix(mcp/oauth): strip same-origin path resource indicators too
Plane also rejects `resource=https://mcp.plane.so/http/mcp`, not only the bare
origin forms. That means the MCP OAuth resource filter must treat any resource
URL on the authorization-server origin as redundant for these MCP servers, not
just exact origin/origin-slash values.

- Broadened the filter to compare `new URL(resource).origin` with the persisted authorization-server origin.
- Updated grant and refresh RFC 8707 tests: same-origin path resources such as `/http/mcp` are now stripped from authorize, token exchange, and refresh; cross-origin resources remain preserved.
- Updated docs/changelog wording from exact self-referential origin to same-origin resource indicators.

Verified live against `https://mcp.plane.so/authorize`: patched `MCPOAuthFlow` with `resource=https://mcp.plane.so/http/mcp` generates no `resource` parameter and Plane redirects to `/consent?txn_id=…`.

Fixes #3502
2026-06-25 21:36:32 +00:00
roboomp 11c10640f2 fix(mcp/oauth): persist authorization-server origin so refresh filters against it
Review on PR #3503 flagged that the prior fix anchored the initial-grant filter
on `authorizationUrl` but the refresh filter on `tokenUrl`. RFC 8414 lets the
authorize and token endpoints sit on different origins, so when they do, a
`config.url` fallback equal to the auth-server origin survives the refresh
filter — the credential works until expiry, then refresh resurrects the same
self-referential `resource` the authorize/token exchange intentionally
omitted.

- `MCPStoredOAuthCredential.authorizationUrl?: string` — new field, the issuer the grant was minted against.
- `MCPOAuthFlow.authorizationUrl` getter exposes the value so the persistence site can write it (symmetric with `flow.resource`).
- `refreshMCPOAuthToken` accepts `{ authorizationUrl }` via the trailing options object; filters self-referential indicators against the supplied URL, falling back to `tokenUrl`'s origin for legacy credentials. New `RefreshMCPOAuthTokenOptions` interface keeps the positional resource form working.
- `mcp-command-controller.ts` persists `flow.authorizationUrl` on credential write; `manager.ts` extracts it from the embedded credential material (legacy `MCPAuthConfig` rows lack it and continue through the `tokenUrl` fallback) and threads it to `refreshMCPOAuthToken`.
- Tests: 3 new cross-origin refresh cases — stripped when resource equals auth-server origin with cross-origin token endpoint; preserved when resource points at a third origin; legacy `tokenUrl`-anchored fallback still works without `authorizationUrl`. Plus a `flow.authorizationUrl` getter test. Updated `mcp-manager-oauth-refresh.test.ts` to account for the new opts arg.

Fixes #3502
2026-06-25 21:04:58 +00:00
roboomp dbff881fba fix(mcp/oauth): apply self-referential resource filter on refresh too
When the initial grant strips a self-referential resource (per the previous
commit), the credential is stored with `resource: undefined`. On the next
refresh, `MCPManager.prepareConfig` (`packages/coding-agent/src/mcp/manager.ts:1232-1233`)
falls back from `material?.resource` to `config.url` and pipes it into
`refreshMCPOAuthToken` — re-introducing the same self-referential value that
broke the initial authorize against strict servers like Plane. RFC 8707 §2.2
also requires the token-request indicator to match the authorize indicator,
so dropping in one mandates dropping in the other.

- Hoisted `filterSelfReferentialResource(resource, serverUrl)` to module scope so the class-method form and the free `refreshMCPOAuthToken` share the rule. `MCPOAuthFlow.#filterResourceIndicator` is now a thin delegate.
- `refreshMCPOAuthToken` now passes the resource through the same filter, using `tokenUrl` as the origin yardstick (RFC 8414 puts authorize and token endpoints on the same issuer, and MCP discovery follows that contract).
- Added 3-test `RFC 8707 resource indicator (refresh)` suite covering: resource equals token-server origin (stripped), origin with trailing slash (stripped), and a path-bearing resource (preserved).

Fixes #3502
2026-06-25 20:54:16 +00:00
roboomp 093243bff5 fix(mcp/oauth): drop self-referential resource indicator from authorize/token requests
Some MCP authorization servers reject `resource=<auth-server-origin>` with
`server_error&error_description=An+unexpected+error+occurred` before the
consent screen is shown. Plane (`https://mcp.plane.so`) is the live example:
`resource=https://mcp.plane.so` or `https://mcp.plane.so/` errors; the same
authorize request with no resource (or a path-bearing resource like
`https://mcp.plane.so/sse`) succeeds.

Per RFC 8707 §2 the resource indicator distinguishes *other* resource servers
from the authorization server, so a self-referential value is never required.
`MCPOAuthFlow` now strips a resource that equals the authorization-server
origin (with or without trailing slash) in three places:

- the constructor, when resolving the configured resource;
- `generateAuthUrl()`, including the URL-override path that re-reads `?resource=` from the authorize URL;
- `exchangeToken()`, which reads `this.#resource` (RFC 8707 §2.2 requires the token request indicator to match the authorize request, so dropping in one mandates the other).

Verified live against `https://mcp.plane.so/authorize`: pre-fix the generated
URL produces `302 → /callback?error=server_error&…`; post-fix it produces
`302 → /consent?txn_id=…`.

Fixes #3502
2026-06-25 20:48:30 +00:00
Ogrodev ef3ae501fb Merge upstream/main into feat/profiles-and-alias 2026-06-11 13:07:52 -03:00
Ogrodev 2f60eaf938 feat(coding-agent): bind MCP OAuth credentials per profile via url-keyed ids
Store MCP OAuth credentials under deterministic mcp_oauth:<url> ids in each
profile's agent.db with refresh material embedded, so a definition-only entry
in a shared project mcp.json resolves each profile's own credential instead
of profiles clobbering each other's auth.credentialId pointer.

- Refresh material is single-source: embedded credential fields win over the
  config auth block (which may belong to another profile); legacy rows fall
  back to the auth block wholesale
- Wire the 401 refresh hook off the resolvable credential, not the auth
  block, so definition-only bindings refresh mid-session too
- The url-keyed fallback never overrides a pinned Authorization header
- Send prompt=consent by default (oauth.prompt to override, "" to omit) so
  reauth can switch accounts past an active browser session
- /mcp reauth fails fast on stdio transports (with an mcp-remote ~/.mcp-auth
  hint), probes http/sse without OAuth injection, GCs the superseded legacy
  row only after the flow succeeds, and leaves definition-only entries
  untouched on disk
- DCR-issued client secrets stay embedded in the stored credential and are
  never written into config files; user-supplied secrets survive reauth
2026-06-10 18:40:07 -03:00
Clark Tomlinson df97002df9 fix: include MCP OAuth resource indicator 2026-06-10 15:58:25 -04:00
danzaio 3b47ee93e9 fix(mcp): accept manual OAuth redirect input 2026-06-10 08:26:00 +02:00
can1357 eb1a46baf5 feat: added injectable fetch transport across AI and coding network flows
- Added optional FetchImpl fields to compaction, proxy, AI, coding-agent, and mnemopi options.
- Threaded injected fetch implementations through OAuth, discovery, and search/LLM request flows.
- Removed exported hookFetch utility and its package entrypoint from utils.
- Replaced global-fetch test monkeypatching with per-test FetchImpl mocks across test suites.
2026-06-09 04:51:17 +02:00
can1357 9d457f73d9 test: migrated test imports to package subpath exports
- Replaced relative `../src` imports with `@oh-my-pi/pi-ai` and `@oh-my-pi/pi-agent-core` subpaths.
2026-06-08 19:03:55 +02:00
Vilmos Nebehaj a7f73ee645 fix(coding-agent): persist dynamically registered MCP OAuth client_id
When an MCP server uses OAuth Dynamic Client Registration (RFC 7591) and
no client_id is pre-configured, MCPOAuthFlow registers a fresh public
PKCE client on each authorize, captures the issued client_id into a
private field, then discards it once the flow object goes out of scope.
At refresh time, MCPManager#resolveAuthConfig calls refreshMCPOAuthToken
with auth.clientId from mcp.json — which is empty for these servers —
so providers that require client_id on the refresh grant (e.g. Linear at
mcp.linear.app/token) reject with HTTP 401 invalid_client. The user is
forced to /mcp reauth manually every time the access token expires.

This change threads the resolved/registered client credentials back out
of the OAuth flow and persists them into mcp.json so refresh has what
it needs indefinitely:

- MCPOAuthFlow exposes resolvedClientId / registeredClientSecret getters.
- MCPCommandController#handleOAuthFlow returns OAuthFlowResult with
  credentialId + clientId + clientSecret, populated from the flow's
  post-login state.
- The initial-connect non-wizard path and /mcp reauth path persist the
  returned client credentials into both auth.{clientId,clientSecret}
  (used at refresh) and oauth.{clientId,clientSecret} (used by future
  /mcp reauth to skip re-registration).
- The wizard's onOAuth callback signature now returns the same shape;
  #launchOAuthFlow folds the registered credentials into wizard state so
  the final mcp.json entry built by #buildServerConfigWithAuth includes
  them under auth.{clientId,clientSecret}.

Servers that configure a static oauth.clientId in mcp.json (Notion,
Slack, Datadog) are unaffected: #tryRegisterClient short-circuits, the
returned clientId equals the configured one, and the write-back is a
no-op.

Adds two MCPOAuthFlow unit tests covering both paths.
2026-05-13 15:39:04 -07:00
can1357 d24d11a274 fix: resolved AI/OAuth helper duplication via shared modules
- Standardized missing-file read errors and now return `File not found: <path>` for absent edit targets.
- Centralized AI provider, usage, and OAuth helpers into shared modules to remove duplicated logic.
- Migrated OAuth/API-key login flows to shared factory helpers and removed inline prompt/token-exchange code.
- Reused shared tools and formatter utilities for discovery, stream tails, LSP batching, and source formatting.
- Consolidated repeated test helpers and fixtures into shared modules, replacing inline helper duplicates.
2026-04-23 21:02:14 +02:00
can1357 afc2276a12 refactor(ai): extracted OpenAI compat logic into dedicated module
- Extracted OpenAI compatibility detection and resolution logic into dedicated `openai-completions-compat` module.
- Refactored `detectCompat()` and `getCompat()` to delegate to new compat module functions with simplified conditional logic.
- Fixed OAuth redirect URI validation to preserve exact configured values without trailing slash normalization.
- Improved session deletion to return boolean status and display error messages in UI instead of silently failing.
- Added `/session delete` command with Delete key support and confirmation dialogs for session management.
2026-03-17 16:07:26 +01:00
can1357 e8eed16c1e chore: reformat 2026-03-17 14:52:06 +01:00
maximhar 571510bdf3 fix: MCP OAuth exact redirect URIs for Slack-style providers (#454)
* Add exact MCP OAuth redirect URI support

* Allow proxied HTTPS loopback MCP redirects

* Fix MCP OAuth busy-port test determinism

* Make MCP OAuth redirect test deterministic

* fix(coding-agent): honor exact loopback redirect ports

* fix(coding-agent): expand env vars in standalone MCP oauth config

---------

Co-authored-by: can1357 <me@can.ac>
2026-03-17 14:49:23 +01:00
can1357 005401e82a refactor: extracted fetch mocking into reusable hookFetch utility
- Extracted fetch mocking logic into reusable `hookFetch()` utility function with middleware-style handler pattern.
- Replaced manual `globalThis.fetch` assignment and restoration across 10 test files with `hookFetch()` calls using `using` statement for automatic cleanup.
- Implemented Disposable pattern with Symbol.dispose for fetch hook resource management, eliminating try-finally blocks.
- Exported `hookFetch` from utils public API to enable consistent fetch mocking across packages.
2026-03-08 16:16:54 +01:00
can1357 9af71f5f61 feat(oauth-flow): enabled Codex identity for OAuth client registration
- Updated OAuth client name from 'oh-my-pi MCP' to 'Codex' for dynamic client registration.
- Added test coverage for OAuth client registration with Codex identity.
2026-03-01 20:51:51 +01:00