Conversation
📝 WalkthroughWalkthroughAdds Codex support to Lumen by introducing an installer command that registers the plugin with Codex, configures session-start hooks for index warming, manages skills linking/copying, and registers the MCP server. Includes a doctor command for diagnostics, TOML/JSON hook management utilities, and comprehensive test coverage. Updates documentation and hook host support. ChangesCodex Installation & Diagnostics
sequenceDiagram
participant User
participant Installer as codex install
participant Paths as Path Resolution
participant Config as config.toml
participant Hooks as hooks.json
participant Skills as skills directory
participant MCP as MCP Registry
User->>Installer: Run codex install
Installer->>Paths: Resolve CODEX_HOME, LUMEN_PLUGIN_ROOT
Paths-->>Installer: Paths resolved
Installer->>Config: Read config.toml
Config-->>Installer: Config loaded
Installer->>Config: Merge codex_hooks = true
Config-->>Installer: Feature flag enabled
Installer->>Hooks: Read hooks.json
alt Hooks exist
Hooks-->>Installer: Hooks loaded
else Hooks missing
Installer-->>Hooks: Initialize empty
end
Installer->>Hooks: Merge SessionStart hook with launcher command
Hooks-->>Installer: Hook merged & deduplicated
Installer->>Hooks: Write with atomic temp-file replace
Hooks-->>Installer: Written (preserving symlinks)
Installer->>Skills: Check skills destination
alt Symlink exists and valid
Skills-->>Installer: Symlink OK (skip)
else Directory with marker exists
Skills-->>Installer: Copied skills OK (skip)
else Missing or stale
Installer->>Skills: Create symlink to plugin/skills
Skills-->>Installer: Symlink created
end
Installer->>MCP: Run codex mcp get lumen
alt MCP installed
MCP-->>Installer: Return current config
alt Config matches launcher & stdio
Installer-->>Installer: No change needed
else Config mismatched
Installer->>MCP: codex mcp remove lumen
MCP-->>Installer: Removed
Installer->>MCP: codex mcp add lumen ... stdio
MCP-->>Installer: Added with correct config
end
else MCP not found
Installer->>MCP: codex mcp add lumen ... stdio
MCP-->>Installer: Added
end
Installer-->>User: Install complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Does codex now support hooks? |
|
Yes: https://developers.openai.com/codex/hooks Looks like it's been there at least since 0.114.0: https://github.com/openai/codex/releases/tag/rust-v0.114.0 They have also been marked stable since 0.124.0. It's been quite effective in my local setup, I've been using it for a few days now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
cmd/codex_hooks.go (2)
309-333: 💤 Low valueOwnership heuristic is sound; consider documenting it.
isLumenCodexSessionStartCommandrecognises a Lumen-owned hook when the launcher path includes/scripts/run.shor/scripts/run.cmdand the parent dir (case-insensitive) starts withlumen. Combined with the explicitexpectedCommandandstatusMessagechecks inisOwnedLumenCodexSessionStartCommand, this correctly preserves foreignhook session-start lumencommands rooted elsewhere (covered byTestMergeCodexSessionStartHook_PreservesForeignSessionStartCommands). Worth a short doc comment so the "starts with lumen" heuristic isn't accidentally broadened or tightened later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/codex_hooks.go` around lines 309 - 333, Add a short doc comment above isLumenCodexSessionStartCommand and isOwnedLumenCodexSessionStartCommand that explains the ownership heuristic: that isLumenCodexSessionStartCommand checks for launcher paths ending with "/scripts/run.sh" or "/scripts/run.cmd" and then verifies the parent directory name (case-insensitive) starts with "lumen", and that isOwnedLumenCodexSessionStartCommand additionally allows an exact expectedCommand match or a codex-specific statusMessage match before falling back to the heuristic; include the rationale that this preserves foreign "hook session-start lumen" commands rooted elsewhere and warn not to broaden or tighten the "starts with lumen" check without updating tests.
228-247: 💤 Low valueTOML "comment splitter" doesn't handle multiline strings.
splitTomlLineCommentcorrectly handles single-line'…'/"…"plus backslash escaping inside"…", but it has no notion of TOML multiline strings ("""…""",'''…'''). A#inside a multiline literal that spans lines could be misread as a comment if the opening triple-quote is on a prior line. In practice Codex'sconfig.tomlis unlikely to contain such constructs around the[features]block, so this is fine for the current scope — worth a one-line comment scoping the parser's intent (per-line, no multiline string awareness) so future maintainers don't repurpose it elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/codex_hooks.go` around lines 228 - 247, The splitTomlLineComment function does not understand TOML multiline string delimiters ('''...''' and """...""") and can misinterpret a '#' inside a multiline string that began on a prior line as a comment; add a clear one-line doc comment immediately above splitTomlLineComment stating that this parser is per-line only and intentionally does not handle TOML multiline strings (triple-quoted literals) so future maintainers won't reuse it for multiline-aware parsing, or alternatively implement multiline detection if you want full TOML compliance.cmd/codex.go (2)
307-328: ⚡ Quick winSurface "codex CLI missing" early in install, like doctor does.
runCodexDoctorcleanly mapserrors.Is(err, exec.ErrNotFound)frommcp getto"MCP lumen: unavailable: codex command not found".ensureCodexMCPdoes not — when thecodexbinary is missing, themcp getfailure silently falls through tomcp add, which also fails withErrNotFound, and the user seesadd Codex MCP server: exec: "codex": executable file not found in $PATH. By that point hooks/skills/feature-flag are already installed, which is fine, but the error message obscures the actual cause. A small early-exit onerrors.Is(err, exec.ErrNotFound)here would give the same clear UX as doctor and avoid the redundantmcp addattempt.♻️ Suggested early-exit
func ensureCodexMCP(ctx context.Context, paths codexPaths, runner commandRunner, stdout, _ io.Writer) error { out, err := runner.Run(ctx, "codex", "mcp", "get", "lumen") if err == nil && codexMCPOutputMatches(out, paths) { _, _ = fmt.Fprintln(stdout, "Codex MCP server lumen already configured") return nil } if err == nil { if _, removeErr := runner.Run(ctx, "codex", "mcp", "remove", "lumen"); removeErr != nil { return fmt.Errorf("remove mismatched Codex MCP server: %w", removeErr) } if _, addErr := runner.Run(ctx, "codex", "mcp", "add", "lumen", "--", paths.launcher, "stdio"); addErr != nil { return fmt.Errorf("add Codex MCP server: %w", addErr) } _, _ = fmt.Fprintln(stdout, "Updated Codex MCP server lumen") return nil } + if errors.Is(err, exec.ErrNotFound) { + return fmt.Errorf("codex command not found in PATH; install the Codex CLI and re-run `lumen codex install`") + } + if !codexMCPGetOutputReportsMissing(out) { + return fmt.Errorf("query Codex MCP server: %w", err) + } if _, addErr := runner.Run(ctx, "codex", "mcp", "add", "lumen", "--", paths.launcher, "stdio"); addErr != nil { return fmt.Errorf("add Codex MCP server: %w", addErr) } _, _ = fmt.Fprintln(stdout, "Added Codex MCP server lumen") return nil }If you take this, double-check
TestRunCodexInstall_WritesHookAndAddsMCPWhenMissing— it currently usesgetErr: exec.ErrNotFoundto simulate "no server yet" rather than "no CLI". Switching that fixture to a non-ErrNotFounderror or tocodexMCPGetOutputReportsMissing-style stdout would keep the test's intent intact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/codex.go` around lines 307 - 328, ensureCodexMCP currently treats a missing codex binary the same as other runner errors and falls through to attempting mcp add; update ensureCodexMCP to check the error returned by the first runner.Run(ctx, "codex", "mcp", "get", "lumen") and if errors.Is(err, exec.ErrNotFound) return a clear, early error (same UX as runCodexDoctor: e.g. "MCP lumen: unavailable: codex command not found") instead of proceeding to mcp add; locate this logic in ensureCodexMCP, use the existing runner.Run result and codexMCPOutputMatches to distinguish the paths, and adjust any tests that assumed getErr == exec.ErrNotFound to simulate a different "no server" condition.
599-602: 💤 Low valueOwnership detection couples to two literal SKILL.md headings.
lumenSkillsDirLooksOwnedrequires the exact strings# Lumen Doctorand# Lumen Reindexto consider the destination "installer-managed". If those headings are ever edited (refactor, branding, frontmatter rearrangement), the installer would silently treat its own previous output as user-owned and refuse to update or uninstall it. Consider keying ownership off the.lumen-skills-sourcemarker alone (whichcopyCodexSkillsalready writes) and reserving the heading check as a redundant signal, so renames toSKILL.mdcontent don't strand existing users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/codex.go` around lines 599 - 602, The ownership check in lumenSkillsDirLooksOwned currently requires exact SKILL.md headings and is fragile; change lumenSkillsDirLooksOwned to first check for the presence of the marker file that copyCodexSkills writes (the ".lumen-skills-source" file in the given path) and return true if that marker exists, and only fall back to the current fileContains heading checks as a redundant signal; update lumenSkillsDirLooksOwned (and any uses of fileContains within it) to prefer the marker-based test so renaming/editing SKILL.md headings won't break installer ownership detection.cmd/codex_test.go (1)
50-87: 💤 Low valueLauncher assertions are Unix-shaped; consider GOOS-gating or a build tag if Windows testing is added.
makeCodexPluginRootonly createsscripts/run.sh, and tests like this one assertpaths.launcher == filepath.Join(pluginRoot, "scripts", "run.sh"). On Windows,codexLauncherPathreturns therun.cmdform, so these assertions would fail. Currently, Go tests only run onubuntu-latestin CI, so this isn't an issue in practice. However, if Windows testing is added to the Go test matrix, gate these assertions withif runtime.GOOS == "windows" { t.Skip(...) }or compute the expected launcher viacodexLauncherPath(pluginRoot)instead of hardcodingrun.sh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/codex_test.go` around lines 50 - 87, TestCodexPathsFromEnv hardcodes a Unix launcher path which will fail on Windows; change the assertion to compute the expected launcher using codexLauncherPath(pluginRoot) (or skip the assertion on windows via runtime.GOOS == "windows") so it matches the platform-specific value returned by resolveCodexPaths; update the test (TestCodexPathsFromEnv) to call codexLauncherPath(pluginRoot) for the expected launcher (or conditionally t.Skip) and keep other assertions the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/codex_hooks.go`:
- Around line 309-333: Add a short doc comment above
isLumenCodexSessionStartCommand and isOwnedLumenCodexSessionStartCommand that
explains the ownership heuristic: that isLumenCodexSessionStartCommand checks
for launcher paths ending with "/scripts/run.sh" or "/scripts/run.cmd" and then
verifies the parent directory name (case-insensitive) starts with "lumen", and
that isOwnedLumenCodexSessionStartCommand additionally allows an exact
expectedCommand match or a codex-specific statusMessage match before falling
back to the heuristic; include the rationale that this preserves foreign "hook
session-start lumen" commands rooted elsewhere and warn not to broaden or
tighten the "starts with lumen" check without updating tests.
- Around line 228-247: The splitTomlLineComment function does not understand
TOML multiline string delimiters ('''...''' and """...""") and can misinterpret
a '#' inside a multiline string that began on a prior line as a comment; add a
clear one-line doc comment immediately above splitTomlLineComment stating that
this parser is per-line only and intentionally does not handle TOML multiline
strings (triple-quoted literals) so future maintainers won't reuse it for
multiline-aware parsing, or alternatively implement multiline detection if you
want full TOML compliance.
In `@cmd/codex_test.go`:
- Around line 50-87: TestCodexPathsFromEnv hardcodes a Unix launcher path which
will fail on Windows; change the assertion to compute the expected launcher
using codexLauncherPath(pluginRoot) (or skip the assertion on windows via
runtime.GOOS == "windows") so it matches the platform-specific value returned by
resolveCodexPaths; update the test (TestCodexPathsFromEnv) to call
codexLauncherPath(pluginRoot) for the expected launcher (or conditionally
t.Skip) and keep other assertions the same.
In `@cmd/codex.go`:
- Around line 307-328: ensureCodexMCP currently treats a missing codex binary
the same as other runner errors and falls through to attempting mcp add; update
ensureCodexMCP to check the error returned by the first runner.Run(ctx, "codex",
"mcp", "get", "lumen") and if errors.Is(err, exec.ErrNotFound) return a clear,
early error (same UX as runCodexDoctor: e.g. "MCP lumen: unavailable: codex
command not found") instead of proceeding to mcp add; locate this logic in
ensureCodexMCP, use the existing runner.Run result and codexMCPOutputMatches to
distinguish the paths, and adjust any tests that assumed getErr ==
exec.ErrNotFound to simulate a different "no server" condition.
- Around line 599-602: The ownership check in lumenSkillsDirLooksOwned currently
requires exact SKILL.md headings and is fragile; change lumenSkillsDirLooksOwned
to first check for the presence of the marker file that copyCodexSkills writes
(the ".lumen-skills-source" file in the given path) and return true if that
marker exists, and only fall back to the current fileContains heading checks as
a redundant signal; update lumenSkillsDirLooksOwned (and any uses of
fileContains within it) to prefer the marker-based test so renaming/editing
SKILL.md headings won't break installer ownership detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0a6d3b6-aedb-4473-8479-77a75247ea32
📒 Files selected for processing (8)
.codex/INSTALL.mdREADME.mdcmd/codex.gocmd/codex_hooks.gocmd/codex_hooks_test.gocmd/codex_test.gocmd/hook.gocmd/hook_test.go
2 participants
Summary
Add a Codex-specific installer that configures the Lumen MCP server, shared skills, Codex hook feature flag, and a user-level SessionStart hook for proactive project index warmup.
[features] codex_hooks = truein Codexconfig.tomlso installed hooks actually run.Test Plan
Summary by CodeRabbit
Release Notes
New Features
codex installcommand to automate Codex configuration, MCP registration, and skills setupcodex doctorcommand for installation diagnostics and status verificationDocumentation
Tests