feat(workflow-executor): use stored OAuth credentials for MCP steps [PRD-367]

[hercemer42]

What & why

Executor-side runtime use of stored OAuth credentials for OAuth2-protected MCP servers (PRD-367, PR2). At an oauth2 MCP step the executor looks up the stored credential by (user, server), runs a refresh-token grant against the stored token_endpoint behind an expiry-skew cache, injects the Bearer token before connecting, retries once on an upstream 401 (covering both list-tools and the tool call), and pauses the step awaiting-input with awaitingInputReason: needs-oauth-reauth when there is no usable credential or the refresh is rejected. Bearer/none steps are unchanged.

Stacked PR / deploy ordering

• Based on feat/prd-367-pr1-executor-oauth-credentials (PR feat(workflow-executor): add OAuth credential store + deposit endpoint (PRD-367 PR1) #1619), not main: PR2 depends on PR1's credential store / encryption / deposit endpoint, which is not yet merged. Review the diff against that branch.
• Behaviour stays dormant until the Forest server serves authType + accepts the typed awaitingInputReason (PR1.5) and the frontend ships (PR3) — deploy the orchestrator first. Safe to deploy alone: the oauth2 path only activates once authType=oauth2 is served; bearer/none is unchanged.

Notable choices (large-PR annotation)

• Serialization (option B): in-process mutex + DB re-read + single retry on invalid_grant, not a Postgres row lock — the executor and token endpoints can sit behind a client VPN, so no DB lock is held across the refresh HTTP call. Row lock is the documented hardening path if a strict reuse-detection provider plus real cross-process contention appears.
• Executor isolation: the executor gains only a re-loadable tool source (reloadWithFreshAuth) plus a typed OAuthReauthRequiredError -> awaiting-input mapping; all token/credential/HTTP logic stays behind RemoteToolFetcher and the new OAuth token service.
• Shared @forestadmin/ai-proxy change (consumed by the agent too) is additive-only (new loadToolsWithFailures / loadRemoteToolsWithFailures + mcp-auth-error export) plus behaviour-preserving delegation; authType is stripped in the McpClient constructor like id.
• In-memory executor (dev-only) raises a ConfigurationError for oauth2 steps (no credential store wired).

Tests

294 tests across the touched suites (ai-proxy 59 + workflow-executor 235); build, lint, tsc clean; >=90% line/stmt coverage on changed files.

Known limitation

A re-auth at tool-call time leaves the step at idempotencyPhase=executing, so it needs a manual retry after re-auth; the common list-tools-time re-auth (pre-executor) resumes cleanly.

Part of PRD-367.

🤖 Generated with Claude Code

Note

Add stored OAuth credential support for MCP steps in workflow executor

• Introduces OAuthTokenService to retrieve, cache, and refresh OAuth access tokens per (user, MCP server), with mutex-serialized refresh, rotation persistence, and OAuthReauthRequiredError on unrecoverable invalid_grant.
• Extends RemoteToolFetcher to inject Bearer tokens for oauth2-typed MCP servers and retry tool loading with a forced token refresh on auth failure.
• Updates McpStepExecutor to catch OAuthReauthRequiredError during both tool fetching and tool invocation, pausing the step with awaiting-input / needs-oauth-reauth rather than failing it; a single reauth retry is attempted before pausing.
• Adds loadRemoteToolsWithFailures to McpClient, AiClient, and all adapter layers so per-server auth/connection failures are classified and surfaced without aborting the entire tool load.
• Propagates awaitingInputReason through McpStepOutcomeSchema and step-outcome-to-update-step-mapper so the reason reaches the server update request.

📊 Macroscope summarized 0e05efe. 21 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

[linear-code]

PRD-367

PRD-624

[hercemer42]

Claude Opus 4.8 — [Should fix] On the invalid_grant retry path, runGrantWithRotationRetry re-reads latest and grants with it, but this write-back passes the pre-retry credential for the non-token fields (clientId / clientSecret / tokenEndpoint / scopes). refreshTokenEnc and encKeyVersion are correct (from the fresh encrypt), so this is not token/key corruption — but if a peer concurrently re-deposited new config, those fields get reverted: a narrow lost-update on the exact multi-instance path this logic guards. Thread the credential actually used for the successful grant through to the write-back.

[hercemer42]

Claude Opus 4.8 — resolved (2b5e5e5). Agreed. runGrantWithRotationRetry now returns the credential whose token produced the grant, and refreshAndCache writes the rotated token back onto that (current) row — so a concurrent re-deposit is no longer partially reverted. Added a regression test asserting the write-back uses the re-read credential fields.

[hercemer42]

Claude Opus 4.8 — [Should fix] 403 is treated like 401, so a 403 on the tool call forces a refresh and (on a second 403) pauses with needs-oauth-reauth. The spec specifies retry on 401; a 403 is usually insufficient-scope, which re-consent often cannot fix, so the user re-auths back into the same 403. Confirm 403 should drive the reauth flow, or narrow the refresh/reauth trigger to 401 and surface 403 as a normal failure. (Classifying 403 as kind: auth is fine — this is only about the reauth trigger.)

[hercemer42]

Claude Opus 4.8 — resolved (8d441d2). Narrowed to 401 per the spec: 403 is no longer treated as a refreshable auth error, so it surfaces as an ordinary failure instead of forcing a refresh + reauth loop. Treating a scope-403 as a re-consent opportunity is a separate, out-of-scope enhancement. isMcpAuthError / classifyMcpLoadError now match 401/unauthorized only; tests updated.

[hercemer42]

Claude Opus 4.8 — [Preferential] This message-text fallback can match a bare 401/403 appearing for non-auth reasons in an error body (IDs, counts, timestamps). It only runs when no structured status field is present (those are checked first), so the blast radius is small — flagging as a known fuzzy edge, not a blocker.

[hercemer42]

Claude Opus 4.8 — acknowledged, keeping as-is. Deliberate last-resort fallback, used only when no structured status field is present (code/status/statusCode are checked first), so the misclassification window is narrow and low-probability. Tightening the message regex risks false negatives on real 401s that carry the status only in text. Leaving it as the documented fuzzy edge.

[qltysh]

10 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many parameters (count = 4): constructor	4
qlty	Structure	Function with high complexity (count = 12): invokeWithReauthRetry	3
qlty	Structure	Function with many returns (count = 9): create	3

[qltysh]

Coverage Impact

⬆️ Merging this pull request will increase total coverage on feat/prd-367-pr1-executor-oauth-credentials by 0.03%.

Modified Files with Diff Coverage (20)

Rating	File	% Diff	Uncovered Line #s
	packages/workflow-executor/src/runner.ts	100.0%
	packages/workflow-executor/src/executors/step-executor-factory.ts	100.0%
	packages/workflow-executor/src/build-workflow-executor.ts	100.0%
	packages/workflow-executor/src/remote-tool-fetcher.ts	100.0%
	packages/workflow-executor/src/http/executor-http-server.ts	94.1%	332
	packages/workflow-executor/src/adapters/server-ai-adapter.ts	100.0%
	packages/workflow-executor/src/types/validated/step-outcome.ts	100.0%
	packages/ai-proxy/src/mcp-client.ts	100.0%
	packages/workflow-executor/src/adapters/ai-client-adapter.ts	100.0%
	packages/ai-proxy/src/ai-client.ts	100.0%
	packages/workflow-executor/src/errors.ts	100.0%
	packages/workflow-executor/src/executors/mcp-step-executor.ts	100.0%
	...ow-executor/src/adapters/step-outcome-to-update-step-mapper.ts	100.0%
	...s/workflow-executor/src/adapters/always-error-ai-model-port.ts	100.0%
	packages/workflow-executor/src/defaults.ts	100.0%
	packages/ai-proxy/src/index.ts	100.0%
	packages/workflow-executor/src/oauth/refresh-grant.ts	100.0%
	packages/workflow-executor/src/oauth/token-service.ts	98.2%	147
	packages/ai-proxy/src/mcp-auth-error.ts	92.0%	25-27
	packages/workflow-executor/src/oauth/keyed-mutex.ts	100.0%
	Total	98.3%

🤖 Increase coverage with AI coding...

In the `feat/prd-367-pr2-executor-oauth-runtime` branch, add test coverage for this new code:

- `packages/ai-proxy/src/mcp-auth-error.ts` -- Line 25-27
- `packages/workflow-executor/src/http/executor-http-server.ts` -- Line 332
- `packages/workflow-executor/src/oauth/token-service.ts` -- Line 147

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

[hercemer42]

Agent code review — Claude Opus 4.8 (2026-06-18)

Reviewed the folded-in commits 7bf9357b (key-rotation re-consent) and 842d4680 (GET /list-mcp-tools) via the pr-review-toolkit code-reviewer; the runtime/max-run path was covered in the earlier review pass. No issues found.

Verified:

• list-mcp-tools — user_id is taken from the validated JWT (never the request); the mcpServerId query is validated (400 if absent). Responses leak no secrets (200 → {name, description} per tool; 409 → {awaitingInputReason, mcpServerId}). The typed needs-oauth-reauth 409 is set on ctx directly so it survives the error middleware (which would otherwise remap OAuthReauthRequiredError → 400).
• Key-rotation classification (toGrantParams) — the try wraps only the decrypt, so a missing key stays terminal (ExecutorEncryptionKeyMissingError rethrown) and a key-present decrypt failure → needs-oauth-reauth; the grant call sits outside the try, so invalid_grant/transient/5xx errors cannot be misclassified into a re-consent loop.
• Domain error classes (no raw Error), no PRD refs in comments, tool serialization type-safe.

[macroscopeapp]

🟡 Medium oauth/token-service.ts:179

The this.encryption.encrypt call in persistRotatedRefreshToken is outside the try block, so an encryption failure causes getAccessToken to throw instead of returning the already-obtained valid access token. Move the encrypt call inside the try so it is treated as a non-fatal write-back failure.

  ): Promise<void> {
-    const encrypted = this.encryption.encrypt(refreshToken);
-
    try {
+      const encrypted = this.encryption.encrypt(refreshToken);
      await this.store.upsert({

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/oauth/token-service.ts around lines 179-183:

The `this.encryption.encrypt` call in `persistRotatedRefreshToken` is outside the `try` block, so an encryption failure causes `getAccessToken` to throw instead of returning the already-obtained valid access token. Move the encrypt call inside the `try` so it is treated as a non-fatal write-back failure.

Evidence trail:
packages/workflow-executor/src/oauth/token-service.ts lines 176-202 (REVIEWED_COMMIT): `encrypt` on line 180 is before `try` on line 182; comment on lines 194-196 states write-back failures are non-fatal. packages/workflow-executor/src/oauth/token-service.ts lines 89-110 (REVIEWED_COMMIT): `persistRotatedRefreshToken` is called on line 106, and if it throws, `return result.accessToken` on line 109 is never reached. packages/workflow-executor/src/crypto/credential-encryption.ts lines 48-60 and 79-88 (REVIEWED_COMMIT): `encrypt` calls `deriveKey()` which can throw `ExecutorEncryptionKeyMissingError`.

[macroscopeapp]

🟡 Medium oauth/token-service.ts:194

persistRotatedRefreshToken() logs but swallows store.upsert() failures, leaving the old refresh token in storage. The authorization server has already rotated the token, so the stored row is now invalid. On the next refresh, runGrantWithRotationRetry() re-reads the stale row, sees wasRotated === false, and incorrectly raises OAuthReauthRequiredError instead of retrying. A transient database failure therefore forces unnecessary user re-authorization.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/oauth/token-service.ts around line 194:

`persistRotatedRefreshToken()` logs but swallows `store.upsert()` failures, leaving the old refresh token in storage. The authorization server has already rotated the token, so the stored row is now invalid. On the next refresh, `runGrantWithRotationRetry()` re-reads the stale row, sees `wasRotated === false`, and incorrectly raises `OAuthReauthRequiredError` instead of retrying. A transient database failure therefore forces unnecessary user re-authorization.

[macroscopeapp]

🟡 Medium http/executor-http-server.ts:322

handleListMcpTools only intercepts OAuthReauthRequiredError in its catch block; any other error (including ExecutorEncryptionKeyMissingError, which OAuthTokenService deliberately throws for operator-misconfiguration) falls through to the generic error middleware and returns 500. When the encryption key is missing, callers receive an opaque "Internal server error" instead of the intended service-unavailable response that would prompt an admin to provision the key.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/http/executor-http-server.ts around line 322:

`handleListMcpTools` only intercepts `OAuthReauthRequiredError` in its catch block; any other error (including `ExecutorEncryptionKeyMissingError`, which `OAuthTokenService` deliberately throws for operator-misconfiguration) falls through to the generic error middleware and returns 500. When the encryption key is missing, callers receive an opaque "Internal server error" instead of the intended service-unavailable response that would prompt an admin to provision the key.

[macroscopeapp]

🟡 Medium oauth/token-service.ts:65

getAccessToken() returns a cached token from this.cache without invalidating when the underlying credential is deleted or updated via the HTTP endpoints (/mcp-oauth-credentials). Since OAuthTokenService is a long-lived singleton and the endpoints only modify the store, a user who disconnects or reconnects an MCP server continues to receive the stale cached token until expiry — allowing API calls with the old authorization after disconnect and potentially the wrong account after reconnect. Consider invalidating the cache entry on credential updates/deletes, or checking store freshness before serving from cache.

Also found in 1 other location(s)

packages/workflow-executor/src/build-workflow-executor.ts:240

Passing the shared mcpOAuthTokenService into Runner at line 240 makes workflow steps keep using any access token already cached in OAuthTokenService even after /mcp-oauth-credentials/:mcpServerId deletes the stored credential. getAccessToken() serves the cache before reading the store, and there is no cache invalidation on delete, so a user who disconnects an MCP server can still have subsequent steps execute against that server until the cached token expires.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/oauth/token-service.ts around line 65:

`getAccessToken()` returns a cached token from `this.cache` without invalidating when the underlying credential is deleted or updated via the HTTP endpoints (`/mcp-oauth-credentials`). Since `OAuthTokenService` is a long-lived singleton and the endpoints only modify the store, a user who disconnects or reconnects an MCP server continues to receive the stale cached token until expiry — allowing API calls with the old authorization after disconnect and potentially the wrong account after reconnect. Consider invalidating the cache entry on credential updates/deletes, or checking store freshness before serving from cache.

Also found in 1 other location(s):
- packages/workflow-executor/src/build-workflow-executor.ts:240 -- Passing the shared `mcpOAuthTokenService` into `Runner` at line `240` makes workflow steps keep using any access token already cached in `OAuthTokenService` even after `/mcp-oauth-credentials/:mcpServerId` deletes the stored credential. `getAccessToken()` serves the cache before reading the store, and there is no cache invalidation on delete, so a user who disconnects an MCP server can still have subsequent steps execute against that server until the cached token expires.

[macroscopeapp]

🟠 High executors/mcp-step-executor.ts:87

When executeToolAndPersist() throws OAuthReauthRequiredError, the beforeCall hook has already persisted idempotencyPhase: 'executing' to the run store. doExecute() catches the error and converts it to an awaiting-input outcome, but never clears that marker. On retry after the user re-authorizes, checkIdempotency() finds idempotencyPhase: 'executing' and throws StepStateError, blocking resumption of the step. Consider updating the state to idempotencyPhase: 'done' (or removing the executing marker) before returning the awaiting-input outcome.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/executors/mcp-step-executor.ts around line 87:

When `executeToolAndPersist()` throws `OAuthReauthRequiredError`, the `beforeCall` hook has already persisted `idempotencyPhase: 'executing'` to the run store. `doExecute()` catches the error and converts it to an `awaiting-input` outcome, but never clears that marker. On retry after the user re-authorizes, `checkIdempotency()` finds `idempotencyPhase: 'executing'` and throws `StepStateError`, blocking resumption of the step. Consider updating the state to `idempotencyPhase: 'done'` (or removing the executing marker) before returning the `awaiting-input` outcome.