Finish runtime-aware container MCP launcher - #227
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a7fd8786d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| codex-memoryd mcp codex preview --runtime container | ||
| codex-memoryd mcp codex apply --runtime container | ||
| codex-memoryd mcp codex status --runtime container |
There was a problem hiding this comment.
For a first-time container user, the default database parent (~/.codex-memoryd) does not yet exist, and this sequence never runs codex-memoryd init; the design also states that apply mutates only Codex config and does not start the container. Consequently, Codex reaches the bind mount before the application can create the directory, causing the mount to fail or be created with ownership that prevents the requested UID/GID from initializing SQLite. Add an initialization/directory-creation step to this path or explicitly make apply create the validated database parent.
Useful? React with 👍 / 👎.
| - image acquisition: pull when missing (`--pull=missing`) | ||
| - image: resolved `CODEX_MEMORYD_IMAGE`, defaulting to | ||
| `ghcr.io/joshyorko/codex-memoryd:latest` |
There was a problem hiding this comment.
When the default :latest image has already been cached, --pull=missing deliberately reuses that cached image, so upgrading the Homebrew CLI or publishing a newer release will not update the MCP server and users can remain on an old, potentially schema-incompatible binary indefinitely. Either generate an immutable version tag tied to the installed CLI or use an always-pull policy for the mutable latest default.
Useful? React with 👍 / 👎.
| - identity: resolved host UID/GID so SQLite files remain host-owned | ||
| - storage: mount only the database's parent directory read-write at `/data` | ||
| - database argument: the corresponding `/data/<filename>` path | ||
| - MCP tier: explicit `mcp stdio --read-only` |
There was a problem hiding this comment.
Mounting only the database directory and passing only --db makes the container load configuration from its own home rather than the host's selected/default config. Unlike the native entry, it therefore loses configured default profile/workspace and other runtime settings, so an existing database can appear empty or behave differently whenever the user relies on non-default configuration. Pass the necessary resolved settings as arguments/environment, or mount the selected config read-only and reference its in-container path.
Useful? React with 👍 / 👎.
| - `--runtime native` preserves the current direct-binary stdio entry. | ||
| - `--runtime container` emits a Docker or Podman stdio entry that starts the |
There was a problem hiding this comment.
Preserving the current direct-binary entry can persist a versioned Homebrew Cellar executable path; after brew upgrade removes that keg, Codex again points at a binary that no longer exists—the exact failure this design is intended to repair. The native Homebrew path should render a stable opt symlink or another durable launcher path, and the compatibility test should distinguish source-build behavior from Homebrew upgrades.
Useful? React with 👍 / 👎.
| - `--runtime native` preserves the current direct-binary stdio entry. | ||
| - `--runtime container` emits a Docker or Podman stdio entry that starts the | ||
| published image on demand and exits when Codex closes stdin. | ||
| - Omitting `--runtime` preserves the resolved/default native behavior. |
There was a problem hiding this comment.
The repository's global RuntimeKind also accepts auto and compose-dev, and because the option is global those values already parse for mcp codex; they can also be selected through runtime environment resolution. This decision specifies only native and container behavior, leaving valid invocations to be silently treated as native/container or rejected inconsistently. Specify how auto resolves and whether compose-dev is mapped or rejected before config mutation, and cover both values in the CLI tests.
Useful? React with 👍 / 👎.
| Container rendering fails before config mutation when: | ||
|
|
||
| - neither Docker nor Podman can be resolved; | ||
| - the database has no usable file name or parent directory; | ||
| - UID/GID or mount values cannot be represented safely. |
There was a problem hiding this comment.
If Docker or Podman has been uninstalled or is temporarily unavailable while the resolved runtime remains container, requiring container rendering to succeed before any config mutation prevents mcp codex remove from deleting the stale MCP block even though removal needs neither an engine nor an image. Exempt remove from container prerequisite resolution so cleanup remains possible in the failure scenario it is meant to recover from.
Useful? React with 👍 / 👎.
| The existing Codex allowlist, approval mode, startup timeout, and tool timeout | ||
| remain unchanged. The database directory mount, rather than a single-file |
There was a problem hiding this comment.
The container path pulls a missing image synchronously during MCP startup but explicitly preserves the existing 30-second startup timeout. On a first launch, downloading and unpacking the image can readily exceed that limit, causing Codex to abort the MCP server before onboarding completes even though the runtime is functioning normally. Give container entries a longer startup timeout or pull the image during an explicit setup step before Codex starts it.
Useful? React with 👍 / 👎.
72f9bb5 to
b8bb09a
Compare
joshyorko
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8bb09aade
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runtime.image.clone(), | ||
| "--db".to_string(), | ||
| format!("/data/{database_filename}"), |
There was a problem hiding this comment.
When CODEX_MEMORYD_PROFILE, CODEX_MEMORYD_WORKSPACE, or other settings come from the host config/runtime.env, this argv passes only the database path, so the image loads its own defaults and recalls from personal/default rather than the selected scope. The newly implemented argument vector is fresh evidence beyond the earlier design comment: either pass the resolved settings as container environment variables or mount and select the resolved configuration.
Useful? React with 👍 / 👎.
| "container MCP database parent cannot be represented safely in a Docker/Podman --volume mount: {parent}; use a path without ':' or control characters, or set CODEX_MEMORYD_DB to a safe directory" | ||
| ))); | ||
| } | ||
| Ok((parent.to_string(), filename.to_string())) |
There was a problem hiding this comment.
When the configured database file is a symlink whose target is outside its parent directory, only the parent is canonicalized and mounted while the original symlink filename is retained. Inside the container /data/<filename> then resolves to an unmounted host path and MCP startup cannot open the database; canonicalize an existing database file and mount its target parent/name, or reject symlinks that escape the mount.
Useful? React with 👍 / 👎.
| "Codex MCP does not support --runtime compose-dev; choose --runtime native or --runtime container", | ||
| )), | ||
| RuntimeKind::Container => { | ||
| let command = native_runtime::container_runtime(runtime).map_err(|err| { |
There was a problem hiding this comment.
When apply discovers Docker or Podman through the invoking shell's PATH, container_runtime returns the literal name (docker or podman), which is then persisted as the Codex command. If restarted Codex has a different PATH—for example, when launched as a desktop application—the generated MCP entry cannot find an engine that successfully passed validation during apply; configured relative wrapper paths have the same problem after the working directory changes. Resolve the executable to an absolute path before rendering the entry.
Useful? React with 👍 / 👎.
| "--user".to_string(), | ||
| format!("{uid}:{gid}"), |
There was a problem hiding this comment.
When the selected engine is rootless Podman, passing the host numeric UID/GID via --user does not preserve host identity: those container IDs are mapped through the rootless user namespace to subordinate host IDs, so the process commonly cannot create SQLite WAL files in the host-owned bind directory. Podman's keep-id user namespace mode maps the current rootless user's UID/GID to the same values inside the container; generate engine-specific identity arguments instead of applying Docker's --user strategy to both engines.
Useful? React with 👍 / 👎.
| let args = vec
Container MCP launcher
This PR finishes the committed container MCP launcher design from 5a7fd87.
Feature behavior:
Documentation:
Scope and deferred work
The final diff is intentionally limited to README.md, docs/getting-started.md, the recovered design document, src/cli.rs, the minimal src/native_runtime.rs visibility change, and focused tests/cli_smoke.rs.
Deferred and excluded:
The original mixed branch tree and all supplied dirty-state recovery artifacts are preserved locally at:
/home/kdlocpanda/.codex/recovery/codex-memoryd/container-mcp-launcher-20260827/
The prior mixed branch is preserved as recovery/container-mcp-launcher-preintegration-20260827.
Verification
Passed:
Full repository gates:
Unavailable or intentionally unproved:
Publication boundary
Published by this PR: the feature branch update and this pull request only.
Not performed: merge, tag, GitHub release, container image publication, Homebrew formula/tap modification, or Homebrew publication.
Base: master at 5c6f60c
Head: feat/container-mcp-launcher at b8bb09a