Skip to content

fix: allow managed MCP cold starts - #165

Open
Taste4458 wants to merge 2 commits into
obra:mainfrom
Taste4458:codex/episodic-memory-startup-timeout
Open

Taste4458 wants to merge 2 commits into
obra:mainfrom
Taste4458:codex/episodic-memory-startup-timeout

Conversation

@Taste4458

@Taste4458 Taste4458 commented Sep 9, 2026

Copy link
Copy Markdown

Summary

  • declare a 120-second startup timeout in the Codex MCP manifest
  • add a packaging test that explicitly verifies the manifest continues to declare that timeout

Validation

  • TZ=UTC npm test: 62 test files, 348 tests passed
  • npm run build: passed
  • fresh stdio initialize, tools/list, and read-only search: passed
  • mcp-inspector strict schema check: passed

@obra obra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the diagnosis is right and the fix is correct. I verified the load-bearing part rather than taking it on faith, since a wrong field name here would be a silent no-op with a passing test.

startup_timeout_sec is real, and 120 has precedent

This is the thing worth checking: an unrecognised key in .mcp.json would be ignored silently, the cold start would still time out, and the new packaging assertion would pass anyway.

It checks out. startup_timeout_sec is a documented per-server Codex option (default 10s, alongside tool_timeout_sec at 60s), and OpenAI's own curated codex-security plugin ships "startup_timeout_sec": 120 in its .mcp.json — the same key and the same value. So the fix does what it says.

Three things before this lands

1. The version bump is a maintainer decision, and I'd drop it from this PR.

You bump 1.6.0 → 1.6.1 in four places. 1.6.0 was cut very recently, and release metadata is the maintainer's to move — batching a version bump into a functional fix means whoever cuts the next release either inherits your number or has to unpick it. The fix stands on its own without it.

2. The "known baseline failure" no longer exists.

The description cites an unchanged failure in test/show.test.ts date formatting. On current main that file is 22/22 green — I ran it. It was fixed by the date-pinning change (#130, "pin en-US/UTC timestamps") in the recent batch merge.

No criticism: you were working from an older base, and that is easy to do. I made the identical mistake today, on a 31-commit-stale tree, and reported a launch-blocking bug that had been fixed six days earlier. Worth rebasing so the description matches reality — and it means you have one less thing to worry about.

3. The new assertion pins the declaration, not the behaviour.

test/codex-plugin.test.ts compares .mcp.json against a literal object, so adding the key to both sides makes it pass by construction. It cannot fail unless someone edits one and not the other.

That is legitimate for a packaging test and I am not asking you to change it — but it is worth being explicit that it guards "the manifest still declares the timeout" and not "cold starts now succeed." If it is ever renamed to something like declares a startup timeout for slow cold starts, that would say what it actually checks.

Note on the diff size

+252/−236 across 8 files reads alarming for a one-key change; ~240 of those lines are esbuild minifier churn in dist/mcp-server.js. That is inherent to this repo committing build artifacts and is not something you introduced. Your actual source change is one line in .mcp.json plus the assertion.

Also worth knowing

This repo now has CI — build, typecheck and tests across Node 22 and 24 — and it runs on fork PRs, so your future contributions here will get real checks. This PR predates it and shows none; I ran the equivalent locally.

Happy to merge once the version bump is dropped and it's rebased. Genuinely useful find — a 10-second default against a cold start that loads native bindings was never going to hold.

Declare a 120-second startup budget in the managed manifest so Codex does not discard a valid cold launch after its 30-second default.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Leave release metadata to maintainers and name the packaging assertion after its manifest-level guarantee.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
@Taste4458
Taste4458 force-pushed the codex/episodic-memory-startup-timeout branch from 87c2dce to bb77b6e Compare September 9, 2026 22:27
@Taste4458

Copy link
Copy Markdown
Author

Thanks — addressed in bb77b6e after rebasing onto 28f0933.

  • Dropped the version bump and changelog entry.
  • Renamed the test to state that it verifies the manifest declaration, not cold-start behavior.
  • Kept the generated bundle unchanged.

Validation: TZ=UTC npm test passes (62 files, 348 tests); build, fresh stdio initialize/tools-list/read-only search, and the strict Inspector schema check pass.

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