Skip to content

fix(harness): attach per-session Sapiom MCP configuration to Codex - #810

Merged
BYWallace merged 6 commits into
mainfrom
brettwallace/sap-3122-codex-session-mcp
Sep 9, 2026
Merged

fix(harness): attach per-session Sapiom MCP configuration to Codex#810
BYWallace merged 6 commits into
mainfrom
brettwallace/sap-3122-codex-session-mcp

Conversation

@BYWallace

@BYWallace BYWallace commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Fixes missing Sapiom tools in Codex sessions.

Problem and motivation

Studio creates a setup file that tells the agent how to connect to Sapiom's MCP tools. The Codex launch code ignored this file.

A new user could start Codex but have no Sapiom tools available.

Summary and scope

Read the file and convert its settings into Codex launch options. Apply those options on both start and resume.

  • Use names tied to the session so the new servers do not conflict with saved user settings.
  • Give each server its required settings and credentials. Keep forwarded credentials out of command arguments and shell environments.
  • Show a clear launch error if the setup file is invalid.
flowchart LR
    F["Studio creates tool settings"] --> B["Before: Codex ignores them"]
    B --> M["Sapiom tools can be missing"]
    F --> A["After: pass settings to Codex"]
    A --> T["Codex can connect to Sapiom tools"]
Loading

Related work

Fixes SAP-3122. #879 adds tests on top of this PR.

The cleanup race is fixed separately in #812. It is not part of this stack.

Validation

September 4 native demo of the template and MCP fixes, before this rebase:

agent-studio-codex-fix-demo.mp4
Diff, checklist, and release details

Diff overview

Changed area What changes Additions
Codex adapter + MCP conversion Read session settings; attach servers; protect credentials 226
Unit tests Settings conversion, launch, resume, invalid files 343
README + changeset Behavior and release note 16
Total 585

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Tests and documentation

Added adapter tests and updated the Harness README. #879 adds real Codex and authentication tests.

Compatibility and release impact

  • Breaking or externally visible changes: Codex receives Studio's MCP settings on launch and resume. Invalid settings produce a launch error. Existing user settings stay unchanged. No migration is required.
  • Changeset: added an @sapiom/harness patch changeset.

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

Codex implemented the change; validation is recorded above.

Checklist

  • I read CONTRIBUTING.md and followed the contribution policy.
  • This pull request addresses one focused problem.
  • I added or updated tests.
  • I ran the relevant build, typecheck, lint, and test commands as described above.
  • I updated documentation.
  • I added a Changeset.
  • I can explain and maintain every submitted change, including any AI-assisted work.

@BYWallace
BYWallace marked this pull request as ready for review September 5, 2026 00:16
@BYWallace
BYWallace changed the base branch from main to brettwallace/session-config-cleanup September 5, 2026 00:26
@BYWallace
BYWallace force-pushed the brettwallace/sap-3122-codex-session-mcp branch from 4a372cd to 5b5705b Compare September 5, 2026 00:26
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #810 (fix(harness): attach per-session Sapiom MCP configuration to Codex)

Public-copy check: clean. The changeset, README additions, JSDoc and test fixtures name only
Sapiom, sapiom-dev, Agent Map and the Codex/OpenAI vocabulary already public in this repo. No
third-party company, business arrangement, private host, or internal link. No new dependency;
@modelcontextprotocol/sdk is already a runtime dep, and tsconfig.build.json excludes
src/**/*.test.ts, so neither new test file reaches the tarball.


1. Per-server MCP env is flattened into the whole Codex session environment

packages/harness/src/core/adapters/codex-mcp.ts:108 and :137-141

env_http_headers and env_vars name variables that must exist in Codex's own process
environment. codex.ts:308/318 returns them as spec.env, and session-manager.ts:1795 merges
spec.env into the PTY environment — so values that were scoped to a single MCP subprocess in the
Claude-shaped file now sit in the environment of Codex and every process it spawns. Two concrete
consequences:

  • ELECTRON_RUN_AS_NODE=1 leaks into the session. The desktop host sets it for the
    sapiom-dev child only (packages/harness-desktop/src/main/boot.ts:544), with the comment
    directly above it stating the invariant this breaks: "rides the MCP config's own env block, so
    it applies to exactly this child and never leaks into the session at large."
    On the Codex path
    it now applies to every command the agent runs; an Electron-based CLI launched from the session
    (code ., cursor .) starts as bare Node and fails to open. packages/harness/CLAUDE.md states
    the same rule ("the env you hand a child is inherited wholesale by everything it spawns"), which
    is why HOST_ESBUILD_PIN is stripped at that boundary.
  • The live sk_ key becomes readable from the shell. SAPIOM_CODEX_MCP_0_HEADER_0 carries the
    remote sapiom header value; before this PR the key existed only in the mode-0600 generated
    file. Any env in an agent-run command now prints it.

codex-mcp.test.ts:84-95 asserts the flattening rather than constraining it, so nothing catches a
regression here. The lever is to keep the non-secret stdio values (ELECTRON_RUN_AS_NODE,
SAPIOM_ENVIRONMENT, SAPIOM_HARNESS_VERSION — none are credentials) as literals in the server's
own inline table rather than forwarding them through the shared env, reserving env forwarding for
values that genuinely must not appear in argv. If Codex's stdio env table isn't usable here, say
so in the JSDoc — right now the file documents the mechanism but not the blast radius.

2. The changeset omits the new hard-failure mode

.changeset/codex-session-mcp.md

The changeset describes only the attach behavior. It does not mention that an unreadable or
invalid generated MCP file now aborts the Codex launch (codex-mcp.ts:153-158, surfaced as a
500 by the session route) where the file was previously ignored and the session started anyway.
The PR body itself lists this under "externally visible changes"; the CHANGELOG that npm consumers
read is the surface where it matters. Add a sentence — patch level is still right for a bug fix,
but the changed failure mode has to be in the published text.

3. The default system prompt still names the un-aliased servers

packages/harness/src/profiles/default.ts:14-23

The prompt tells the agent it has servers named sapiom and sapiom-dev and to "use its
sapiom_dev_agents_* tools". On Codex those servers are now sapiom-<12 hex> /
sapiom-dev-<12 hex>, and Codex namespaces tool names by server, so neither the server names nor
the tool names in the prompt match what the model is offered. The appended one-liner
(codex-mcp.ts:166) lists the aliases but never reconciles them with the profile text, leaving the
model two contradictory names for one server — the failure mode is an agent reporting the
sapiom-dev server as unavailable, or hunting for a tool by the name the prompt gave it.
packages/harness/src/server/scaffold.ts:8's user-facing copy has the same problem. Either make
the appended line explicitly a rename ("sapiom-dev is registered in this session as
sapiom-dev-<hash>; the names in your instructions refer to it") or template the profile text.

Nit

  • codex-mcp.ts:153 — the blanket catch {} is right for JSON parse errors, but it also swallows
    EACCES, "Invalid server name" and "Conflicting environment values" with no console.error
    anywhere, so a session that refuses to start leaves no diagnostic. The adapter logs on the
    analogous systemPromptFile failure (codex.ts:466); log the error class here too, just not the
    message from the parse branch.

Verdict: Request changes — finding 1 (credential and ELECTRON_RUN_AS_NODE scope widened from
one MCP child to the whole Codex session) should be resolved or explicitly justified in the JSDoc
before merge; 2 and 3 are small edits.

@BYWallace
BYWallace force-pushed the brettwallace/sap-3122-codex-session-mcp branch from 5b5705b to 951dc19 Compare September 5, 2026 00:48
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #810

Delta since the reviewed commit: 951dc19e ("isolate Codex MCP settings from shell commands").
Public-copy check on the new changeset sentence and README paragraph: clean.

Round-1 findings

  1. Env scope — fixed. STDIO_SETTINGS keeps ELECTRON_RUN_AS_NODE / SAPIOM_ENVIRONMENT /
    SAPIOM_HARNESS_VERSION in the server's own env table, and remaining forwarded values are
    blanked per-variable via shell_environment_policy.set.<VAR>="" (codex-mcp.ts:150-163,194-196),
    preserving the user's other shell settings.
  2. Changeset — fixed. The launch-error failure mode is now in the published text.
  3. Prompt/alias mismatch — addressed. The appended line is now an explicit rename plus
    "References to the original server names in your other instructions mean these session-specific
    registrations."
  4. Nit (silent catch) — fixed. console.error with an error class, and asserted not to leak the
    path or file contents.

New finding

The env-table move is unverified where it matterscodex-mcp.ts:161,
codex-mcp.integration.test.ts:270-276.

The real generated config puts all three sapiom-dev variables in STDIO_SETTINGS, so on the
desktop path env_vars is empty and ELECTRON_RUN_AS_NODE now depends entirely on Codex honouring
mcp_servers.<alias>.env as an augment. Nothing in the suite checks that. The integration test
spawns process.execPath under vitest — real node, where ELECTRON_RUN_AS_NODE is inert — and it
never asserts the forwarded SAPIOM_REVIEW_STDIO_SECRET arrived at the child; the unit test only
matches the emitted TOML string. If Codex's env replaces the child environment rather than adding
to it (or is ignored when env_vars is also present), the packaged app spawns a second copy of
Agent Studio instead of Node and the parent waits forever — packages/harness/CLAUDE.md §3, and a
regression the previous shape did not have. Add a stdio fixture that reports its own process.env
and assert both the local and the forwarded variable.

Round 1 got one thing wrong

scaffold.ts:7-8 is a historical JSDoc narrative about copy that was removed, not live
user-facing copy; only profiles/default.ts:21 names sapiom_dev_agents_* today.

Verdict: Approve once the stdio env delivery is asserted against a child that can report it.

@BYWallace

BYWallace commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The child-environment coverage gap was valid. The two real Codex runs now observe the actual MCP child, verify local settings and a forwarded synthetic secret, and keep credentials out of command environments.

Kept the adapter and unit regressions here (585 additions), and moved the real CLI/auth validation to #879 (473). The combined tree is unchanged. Both Codex probes and all six auth lifecycle tests pass; build, typecheck, and lint pass too. Full-suite results are recorded in #879.

Cleanup remains in #812. The duplicate integration loop and unrelated background-task regression are removed. The earlier scaffold.ts finding referred to historical JSDoc; the active prompt explicitly maps the session-specific server names. Main's managed CLI arguments and background-task behavior are preserved.

@BYWallace
BYWallace force-pushed the brettwallace/sap-3122-codex-session-mcp branch 2 times, most recently from 80206a8 to 5c4793c Compare September 8, 2026 18:39
@BYWallace
BYWallace force-pushed the brettwallace/sap-3122-codex-session-mcp branch from 5c4793c to 9fb23cf Compare September 8, 2026 21:25
@BYWallace
BYWallace changed the base branch from brettwallace/session-config-cleanup to main September 8, 2026 21:25
@BYWallace
BYWallace force-pushed the brettwallace/sap-3122-codex-session-mcp branch from 9fb23cf to 40994e2 Compare September 8, 2026 22:41
@BYWallace
BYWallace merged commit 3d96d32 into main Sep 9, 2026
10 checks passed
@BYWallace
BYWallace deleted the brettwallace/sap-3122-codex-session-mcp branch September 9, 2026 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants