diff --git a/README.md b/README.md index 0cd48bd..c7f903c 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,14 @@ codex-channel-molecule The daemon runs in the foreground; logs go to stderr. For systemd hosts, register a unit; for one-off use, `nohup ... &` plus a log file works. +### Running under launchd / systemd (Node-on-PATH note) + +The codex binary is a `#!/usr/bin/env node` shim, so spawning it requires `node` to be discoverable on PATH. Under an interactive shell that's automatic; under `launchd` (macOS) and stripped-down `systemd` units PATH defaults to `/usr/bin:/bin`, and `env node` will 127-out silently. + +Since 0.1.2 the daemon resolves `codex` to its absolute path and prepends that directory to the subprocess PATH automatically — Node lives next to `codex` under nvm / brew / pnpm-global, so the shim's `env node` finds it. Operators typically don't need any PATH plumbing in their LaunchAgent / unit file beyond the three required env vars. + +If you're on an unusual install layout where `codex` and `node` live in different directories, set the LaunchAgent / unit `PATH` explicitly to include both. + ## Deprecation path When [`openai/codex#17543`](https://github.com/openai/codex/issues/17543) lands upstream — a generic path for handling MCP custom notifications in codex and forwarding them into the active session as user submissions — this daemon becomes redundant. Codex itself will accept inbound molecule messages as `Op::UserInput` directly through the MCP server already wired in `~/.codex/config.toml`. Until then, this is the operator-facing answer. diff --git a/codex_channel_molecule/bridge.py b/codex_channel_molecule/bridge.py index 90634ce..6b93252 100644 --- a/codex_channel_molecule/bridge.py +++ b/codex_channel_molecule/bridge.py @@ -64,8 +64,13 @@ class _RealTools: """ def __init__(self) -> None: + import molecule_runtime.inbox as inbox + from molecule_runtime.a2a_client import PLATFORM_URL, WORKSPACE_ID from molecule_runtime import a2a_tools + state = inbox.InboxState(cursor_path=inbox.default_cursor_path()) + inbox.activate(state) + inbox.start_poller_thread(state, PLATFORM_URL, WORKSPACE_ID) self._tools = a2a_tools async def wait_for_message(self, timeout_secs: float) -> str: @@ -253,6 +258,16 @@ async def _handle_one( ) result: CodexResult = await runner.run(message=body, session_id=session_id) + logger.info( + "codex result for %s: exit=%s session=%s stdout=%d stderr_tail=%d", + activity_id, + result.exit_code, + result.session_id or "none", + len(result.text), + len(result.stderr_tail), + ) + if result.exit_code != 0: + logger.warning("codex stderr tail for %s: %s", activity_id, result.stderr_tail) if result.session_id and result.session_id != session_id: store.set(chat_id, result.session_id) diff --git a/codex_channel_molecule/codex_runner.py b/codex_channel_molecule/codex_runner.py index 963bea1..ed949ff 100644 --- a/codex_channel_molecule/codex_runner.py +++ b/codex_channel_molecule/codex_runner.py @@ -69,6 +69,31 @@ class CodexRunner: ) self._codex_bin = resolved self._timeout_secs = timeout_secs + # The codex binary is a `#!/usr/bin/env node` shim, so spawning + # it requires `node` to be discoverable via the subprocess PATH. + # Under a shell launch this is fine (operator's interactive PATH + # has Node), but launchd and most systemd unit defaults strip + # PATH down to /usr/bin:/bin — `env node` then 127s out and the + # daemon silently fails on every turn. + # + # Fix: remember the directory containing the resolved codex + # binary. Node lives next to it under nvm / brew / pnpm + # global installs. Subprocesses get that dir prepended to their + # PATH (see _build_env). Operators don't have to thread PATH + # through their LaunchAgent / unit file — just $WORKSPACE_ID, + # $PLATFORM_URL, $MOLECULE_WORKSPACE_TOKEN per the install + # contract. + self._codex_bin_dir = os.path.dirname(resolved) + + def _build_env(self) -> dict[str, str]: + env = os.environ.copy() + existing_path = env.get("PATH", "") + if self._codex_bin_dir not in existing_path.split(os.pathsep): + env["PATH"] = ( + self._codex_bin_dir + + (os.pathsep + existing_path if existing_path else "") + ) + return env async def run( self, @@ -81,16 +106,23 @@ class CodexRunner: that session. When None, codex starts a fresh session and assigns a new id (returned in CodexResult.session_id). """ - args: list[str] = [self._codex_bin, "exec", "--skip-git-repo-check"] if session_id: - args.extend(["--resume", session_id]) - args.append(message) + args = [ + self._codex_bin, + "exec", + "resume", + "--skip-git-repo-check", + session_id, + message, + ] + else: + args = [self._codex_bin, "exec", "--skip-git-repo-check", message] proc = await asyncio.create_subprocess_exec( *args, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, - env=os.environ.copy(), + env=self._build_env(), ) try: diff --git a/pyproject.toml b/pyproject.toml index 341ef24..9a024f5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "codex-channel-molecule" -version = "0.1.1" +version = "0.1.2" description = "Bridge daemon for codex CLI ↔ Molecule platform — long-polls the platform inbox, runs `codex exec --resume ` per inbound message, replies via send_message_to_user MCP tool. Counterpart to hermes-channel-molecule." readme = "README.md" requires-python = ">=3.11" diff --git a/tests/test_codex_runner.py b/tests/test_codex_runner.py index 6ad48df..cd81c3d 100644 --- a/tests/test_codex_runner.py +++ b/tests/test_codex_runner.py @@ -25,7 +25,8 @@ _FAKE_CODEX_SCRIPT = textwrap.dedent("""\ \"\"\"Fake codex CLI for tests. Behaviors keyed on argv shape and env: - argv: codex exec [--skip-git-repo-check] [--resume ] + argv: codex exec [--skip-git-repo-check] + argv: codex exec resume [--skip-git-repo-check] Echoes a banner to stderr (\"session: \") and the input message to stdout. Honors FAKE_EXIT_CODE for failure-path tests. @@ -35,17 +36,27 @@ _FAKE_CODEX_SCRIPT = textwrap.dedent("""\ args = sys.argv[1:] assert args[0] == \"exec\", f\"unexpected first arg: {args[0]!r}\" + if \"--resume\" in args: + raise SystemExit(\"--resume is not supported\") + resume_id = None - i = 1 - while i < len(args): - if args[i] == \"--resume\": - resume_id = args[i + 1] - i += 2 - elif args[i].startswith(\"--\"): - i += 1 # skip unrecognized flag - else: - break - msg = args[i] if i < len(args) else \"\" + if len(args) > 1 and args[1] == \"resume\": + i = 2 + while i < len(args): + if args[i].startswith(\"--\"): + i += 1 + else: + break + resume_id = args[i] + msg = args[i + 1] if i + 1 < len(args) else \"\" + else: + i = 1 + while i < len(args): + if args[i].startswith(\"--\"): + i += 1 # skip unrecognized flag + else: + break + msg = args[i] if i < len(args) else \"\" sid = resume_id or os.environ.get(\"FAKE_NEW_SESSION_ID\", \"a1b2c3d4-1111-2222-3333-444455556666\") sys.stderr.write(f\"session: {sid}\\n\") @@ -89,6 +100,8 @@ async def test_run_resumes_existing_session(fake_codex): result = await runner.run(message="follow up", session_id=given) # Resume → no new session id is captured (input is echoed back as-is). assert result.session_id == given + assert result.exit_code == 0 + assert result.text == "echo: follow up" @pytest.mark.asyncio @@ -134,5 +147,103 @@ def test_extract_session_id_matches_alternate_banner_shape(): ) == "12345678-aaaa-bbbb-cccc-dddddddddddd" +@pytest.mark.asyncio +async def test_run_uses_exec_resume_subcommand_not_legacy_flag(fake_codex): + """Codex CLI 0.6+ rejects ``codex exec --resume ``; the correct + invocation is ``codex exec resume [--skip-git-repo-check] ``. + The fake-codex fixture raises SystemExit if it sees ``--resume`` in + argv, so the assertion is that the call SUCCEEDS — proving the + runner moved off the legacy flag. + """ + runner = CodexRunner(codex_bin=str(fake_codex), timeout_secs=10.0) + sid = "deadbeef-1111-2222-3333-444455556666" + result = await runner.run(message="follow", session_id=sid) + # Subprocess returned 0 because the fake didn't see --resume. If the + # runner ever regresses to the old shape, the fake exits non-zero + # with "--resume is not supported" → result.exit_code != 0. + assert result.exit_code == 0, ( + f"Expected exec resume subcommand shape, got " + f"exit_code={result.exit_code} stderr_tail={result.stderr_tail!r}" + ) + assert result.session_id == sid + + +@pytest.mark.asyncio +async def test_run_prepends_codex_bin_dir_to_subprocess_path(tmp_path): + """The codex binary is a `#!/usr/bin/env node` shim. Under launchd / + systemd, the parent process's PATH is stripped to /usr/bin:/bin, so + the shebang's `env node` lookup fails and codex 127s out silently. + The runner mitigates this by prepending the codex binary's own + directory (where node lives in nvm/brew/pnpm-global layouts) to the + subprocess PATH. Pin: subprocess sees that dir first in PATH. + """ + bin_dir = tmp_path / "fakebin" + bin_dir.mkdir() + fake = bin_dir / "codex" + # Echo the resolved subprocess PATH so the test can inspect it. + fake.write_text(textwrap.dedent("""\ + #!/usr/bin/env python3 + import os, sys + sys.stderr.write(f"session: a1b2c3d4-1111-2222-3333-444455556666\\n") + sys.stdout.write(os.environ.get("PATH", "")) + """)) + fake.chmod(0o755) + + runner = CodexRunner(codex_bin=str(fake), timeout_secs=10.0) + # Strip PATH down to the launchd-default minimum to prove the + # runner's own injection puts bin_dir back. + import os as _os + saved = _os.environ.get("PATH", "") + _os.environ["PATH"] = "/usr/bin:/bin" + try: + result = await runner.run(message="probe") + finally: + _os.environ["PATH"] = saved + + assert result.exit_code == 0 + seen_path = result.text + # bin_dir must appear FIRST so `env node` finds it before falling + # back to the (empty for launchd) system PATH. + assert seen_path.startswith(str(bin_dir) + os.pathsep), ( + f"expected codex_bin_dir prefix in subprocess PATH, got: {seen_path!r}" + ) + assert "/usr/bin" in seen_path, ( + f"expected existing PATH preserved after prefix, got: {seen_path!r}" + ) + + +@pytest.mark.asyncio +async def test_run_does_not_duplicate_codex_bin_dir_in_path(tmp_path): + """If the operator's PATH already includes the codex bin dir (the + shell-launched case), don't double it — keeps logs and downstream + tools sane. + """ + bin_dir = tmp_path / "alreadythere" + bin_dir.mkdir() + fake = bin_dir / "codex" + fake.write_text(textwrap.dedent("""\ + #!/usr/bin/env python3 + import os, sys + sys.stderr.write(f"session: a1b2c3d4-1111-2222-3333-444455556666\\n") + sys.stdout.write(os.environ.get("PATH", "")) + """)) + fake.chmod(0o755) + + runner = CodexRunner(codex_bin=str(fake), timeout_secs=10.0) + import os as _os + saved = _os.environ.get("PATH", "") + _os.environ["PATH"] = f"{bin_dir}:/usr/bin:/bin" + try: + result = await runner.run(message="probe") + finally: + _os.environ["PATH"] = saved + + seen_path = result.text + occurrences = seen_path.split(os.pathsep).count(str(bin_dir)) + assert occurrences == 1, ( + f"expected codex_bin_dir to appear exactly once, got {occurrences}: {seen_path!r}" + ) + + def test_extract_session_id_returns_none_on_no_match(): assert _extract_session_id("nothing relevant here") is None