feat(codex): install session-start hook#149
Open
def324 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds a codex CLI with install/doctor, merges SessionStart hooks and enables hooks feature flag in config, ensures skills are symlinked or copied with ownership markers, manages MCP lumen registration, adds codex hook-host support, updates docs, and includes unit and integration tests. 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 dir"
participant MCP as "codex mcp"
User->>Installer: Run codex install
Installer->>Paths: Resolve CODEX_HOME and plugin root
Paths-->>Installer: Resolved paths
Installer->>Config: Read/merge features.hooks = true
Installer->>Hooks: Read and merge SessionStart hook (dedupe/remove owned)
Hooks-->>Installer: Hook merged and written atomically
Installer->>Skills: Check destination -> symlink or copy with marker
Skills-->>Installer: Created or verified
Installer->>MCP: codex mcp get lumen
alt matches expected
MCP-->>Installer: no change
else mismatch/missing
Installer->>MCP: codex mcp remove lumen (if needed)
Installer->>MCP: codex mcp add lumen ... stdio
MCP-->>Installer: added/updated
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 |
Member
|
Does codex now support hooks? |
Author
|
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
def324
force-pushed
the
feat/codex-session-start-hooks-upstream
branch
2 times, most recently
from
381da15 to
a04b8e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@cmd/codex_hooks.go`:
- Around line 314-320: The current classification uses substring matching (via
lumenLauncherRoot calls with "/scripts/run", "/scripts/run.sh",
"/scripts/run.cmd") which will falsely match prefixes like ".../scripts/runner";
update the checks to require exact path segment matches instead of prefix
matches by comparing the final path element (use filepath.Base or path.Base on
the cleaned/normalized path) against "run", "run.sh", and "run.cmd" when calling
lumenLauncherRoot; apply the same exact-segment correction to the duplicate
checks referenced around lines 338-347 so only true launcher filenames are
identified and removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90d96c54-e11e-4761-97b3-92ac396c002c
📒 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
✅ Files skipped from review due to trivial changes (2)
- README.md
- .codex/INSTALL.md
Comment on lines
+314
to
+320
| root, ok := lumenLauncherRoot(normalized, "/scripts/run") | ||
| if !ok { | ||
| root, ok = lumenLauncherRoot(normalized, "/scripts/run.sh") | ||
| } | ||
| if !ok { | ||
| root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd") | ||
| } |
There was a problem hiding this comment.
Avoid prefix matches when classifying Lumen-owned launcher commands.
Line 314 currently matches "/scripts/run" via strings.Index without a suffix boundary check. Paths like .../scripts/runner or .../scripts/runbook can be misclassified as Lumen-owned and then removed during merge, which risks clobbering user SessionStart hooks.
Suggested fix
func isLumenCodexSessionStartCommand(command string) bool {
normalized := strings.ReplaceAll(command, `\`, "/")
if !strings.Contains(normalized, "hook session-start lumen") {
return false
}
- root, ok := lumenLauncherRoot(normalized, "/scripts/run")
+ root, ok := lumenLauncherRoot(normalized, "/scripts/run.sh")
if !ok {
- root, ok = lumenLauncherRoot(normalized, "/scripts/run.sh")
+ root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd")
}
if !ok {
- root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd")
+ root, ok = lumenLauncherRoot(normalized, "/scripts/run")
}
if !ok {
return false
}
return strings.HasPrefix(strings.ToLower(filepath.Base(root)), "lumen")
}
func lumenLauncherRoot(command, suffix string) (string, bool) {
idx := strings.Index(command, suffix)
if idx < 0 {
return "", false
}
+ end := idx + len(suffix)
+ if end < len(command) {
+ switch command[end] {
+ case ' ', '\t', '"', '\'':
+ default:
+ return "", false
+ }
+ }
root := strings.Trim(command[:idx], `"' `)
if root == "" {
return "", false
}
return root, true
}Also applies to: 338-347
🤖 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 314 - 320, The current classification uses
substring matching (via lumenLauncherRoot calls with "/scripts/run",
"/scripts/run.sh", "/scripts/run.cmd") which will falsely match prefixes like
".../scripts/runner"; update the checks to require exact path segment matches
instead of prefix matches by comparing the final path element (use filepath.Base
or path.Base on the cleaned/normalized path) against "run", "run.sh", and
"run.cmd" when calling lumenLauncherRoot; apply the same exact-segment
correction to the duplicate checks referenced around lines 338-347 so only true
launcher filenames are identified and removed.
a04b8e4 to
7d71674
Compare
Add a Codex installer and doctor command that manage Lumen MCP registration, shared skills, the Codex hooks feature flag, and a user-level SessionStart hook for proactive index warmup. Keep the installer aligned with the upstream v0.0.40 launcher split by using scripts/run on POSIX and scripts/run.cmd on Windows, while still detecting legacy scripts/run.sh hook entries for cleanup.
def324
force-pushed
the
feat/codex-session-start-hooks-upstream
branch
from
7d71674 to
1f551d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/codex_hooks.go (1)
351-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire launcher filename boundary checks when classifying Lumen-owned commands.
Line 356 + Line 381 currently allow prefix matches (e.g.,
/scripts/runner), which can misclassify user hooks as Lumen-owned and remove them.🐛 Proposed fix
func isLumenCodexSessionStartCommand(command string) bool { normalized := strings.ReplaceAll(command, `\`, "/") if !strings.Contains(normalized, "hook session-start lumen") { return false } - root, ok := lumenLauncherRoot(normalized, "/scripts/run") + root, ok := lumenLauncherRoot(normalized, "/scripts/run.sh") if !ok { - root, ok = lumenLauncherRoot(normalized, "/scripts/run.sh") + root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd") } if !ok { - root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd") + root, ok = lumenLauncherRoot(normalized, "/scripts/run") } if !ok { return false } return strings.HasPrefix(strings.ToLower(filepath.Base(root)), "lumen") } func lumenLauncherRoot(command, suffix string) (string, bool) { idx := strings.Index(command, suffix) if idx < 0 { return "", false } + end := idx + len(suffix) + if end < len(command) { + switch command[end] { + case ' ', '\t', '"', '\'': + default: + return "", false + } + } root := strings.Trim(command[:idx], `"' `) if root == "" { return "", false } return root, true }#!/bin/bash set -euo pipefail # Inspect launcher ownership detection and root extraction implementation. rg -n -C4 'isLumenCodexSessionStartCommand|lumenLauncherRoot|/scripts/run' cmd/codex_hooks.go # Confirm there is no explicit regression test for "/scripts/runner"-style false positives. rg -n -C2 'runner|runbook' cmd/codex_hooks_test.goAlso applies to: 380-389
🤖 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 351 - 367, The current classification in isLumenCodexSessionStartCommand and lumenLauncherRoot allows prefix matches like "/scripts/runner"; update the matching so launcher filename checks require filename boundaries and exact matches: change calls that search for "/scripts/run", "/scripts/run.sh", "/scripts/run.cmd" to verify the matched substring is followed only by a path separator or end-of-string (or use path.Base/EqualFold on the extracted filename), and replace the loose strings.HasPrefix(strings.ToLower(filepath.Base(root)), "lumen") check with a case-insensitive exact comparison (e.g., strings.EqualFold(filepath.Base(root), "lumen")) so only true Lumen-owned launchers are classified. Ensure these changes touch isLumenCodexSessionStartCommand and lumenLauncherRoot usage.
🧹 Nitpick comments (3)
cmd/codex_test.go (1)
320-340: ⚡ Quick winConvert this multi-scenario test to table-driven form.
This test exercises several cases and is easier to maintain as a table-driven test.
As per coding guidelines, "Use table-driven tests for multiple test cases in Go."Refactor sketch
func TestCodexMCPOutputMatchesRequiresCommandAndArgs(t *testing.T) { paths := codexPaths{launcher: "/repo/lumen/scripts/run"} - if !codexMCPOutputMatches([]byte("transport: stdio\ncommand: /repo/lumen/scripts/run\nargs: stdio\n"), paths) { - t.Fatal("expected command and args output to match") - } - ... + tests := []struct { + name string + out []byte + want bool + }{ + {"text match", []byte("transport: stdio\ncommand: /repo/lumen/scripts/run\nargs: stdio\n"), true}, + {"json match", []byte(`{"command":"/repo/lumen/scripts/run","args":["stdio"]}`), true}, + {"missing command", []byte("transport: stdio\nnote: /repo/lumen/scripts/run\n"), false}, + {"missing args", []byte("command: /repo/lumen/scripts/run\nnote: stdio\n"), false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := codexMCPOutputMatches(tc.out, paths); got != tc.want { + t.Fatalf("got %v, want %v", got, tc.want) + } + }) + } }🤖 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 320 - 340, Refactor TestCodexMCPOutputMatchesRequiresCommandAndArgs into a table-driven test: define a slice of test cases (struct with name, input []byte, paths codexPaths, want bool) covering each existing scenario, then loop over cases with t.Run(tc.name, func(t *testing.T){ if got := codexMCPOutputMatches(tc.input, tc.paths); got != tc.want { t.Fatalf("... expected %v got %v", tc.want, got) } }), keeping the same inputs and assertions; reference the existing test function name TestCodexMCPOutputMatchesRequiresCommandAndArgs and the helper codexMCPOutputMatches/codexPaths to locate code to change.cmd/codex.go (2)
167-180: ⚡ Quick winUse
defer-based file handle cleanup in temp-write paths.Both temp-file writers close handles manually across branches; switch to a defer pattern for handle cleanup consistency and simpler error paths.
As per coding guidelines, "Always defer resource cleanup for database and file handles using
defer Close()."Also applies to: 229-242
🤖 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 167 - 180, The temp-file write path manually closes and removes the file in multiple branches (using tmp, tmpName after os.CreateTemp) — replace the manual closes with a defer-based cleanup: immediately after successful os.CreateTemp assign tmpName := tmp.Name() and defer func() { _ = tmp.Close(); if shouldRemove { _ = os.Remove(tmpName) }() } (or similar) so the file handle is always closed; then update the error branches for Write and Close to set shouldRemove appropriately (or remove only on error) instead of duplicating tmp.Close()/os.Remove calls; apply the same defer-based pattern to the other temp-write block using the same tmp/tmpName/Write/Close variables.
330-334: ⚡ Quick winAvoid false MCP mismatches for equivalent launcher paths.
Strict string equality can mark symlink-equivalent paths as mismatched, causing unnecessary
remove+addchurn.Proposed change
func codexMCPOutputMatches(out []byte, paths codexPaths) bool { command, args := parseCodexMCPOutput(out) - launcher := strings.ReplaceAll(paths.launcher, `\`, "/") - return command == launcher && len(args) == 1 && args[0] == "stdio" + if len(args) != 1 || args[0] != "stdio" { + return false + } + return sameFileTree(command, paths.launcher) }🤖 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 330 - 334, codexMCPOutputMatches is comparing the launcher path string-for-string which will treat symlink/relative/path-separator/case-equivalent paths as different; instead canonicalize both the extracted command and paths.launcher before comparing: call filepath.Abs + filepath.EvalSymlinks + filepath.Clean on both command and paths.launcher (falling back to Clean if Abs/EvalSymlinks error), normalize separators with filepath.ToSlash (or ToSlash on both), and on Windows use strings.EqualFold when comparing; then check equality and the args as before. Ensure you update references to parseCodexMCPOutput, command, and paths.launcher in the codexMCPOutputMatches function.
🤖 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.
Inline comments:
In `@cmd/codex_hooks.go`:
- Around line 369-375: The comparison in isOwnedLumenCodexSessionStartCommand
directly compares hook["statusMessage"] (type any) to codexHookStatusMessage and
can panic if statusMessage is a non-comparable type; update the function to
first type-assert hook["statusMessage"] to string (e.g., via s, ok :=
hook["statusMessage"].(string)) and only compare when ok is true, otherwise
treat it as not matching; keep the existing strings.Contains(command, "hook
session-start lumen") logic intact so behavior is unchanged for string
statusMessage values.
---
Duplicate comments:
In `@cmd/codex_hooks.go`:
- Around line 351-367: The current classification in
isLumenCodexSessionStartCommand and lumenLauncherRoot allows prefix matches like
"/scripts/runner"; update the matching so launcher filename checks require
filename boundaries and exact matches: change calls that search for
"/scripts/run", "/scripts/run.sh", "/scripts/run.cmd" to verify the matched
substring is followed only by a path separator or end-of-string (or use
path.Base/EqualFold on the extracted filename), and replace the loose
strings.HasPrefix(strings.ToLower(filepath.Base(root)), "lumen") check with a
case-insensitive exact comparison (e.g., strings.EqualFold(filepath.Base(root),
"lumen")) so only true Lumen-owned launchers are classified. Ensure these
changes touch isLumenCodexSessionStartCommand and lumenLauncherRoot usage.
---
Nitpick comments:
In `@cmd/codex_test.go`:
- Around line 320-340: Refactor TestCodexMCPOutputMatchesRequiresCommandAndArgs
into a table-driven test: define a slice of test cases (struct with name, input
[]byte, paths codexPaths, want bool) covering each existing scenario, then loop
over cases with t.Run(tc.name, func(t *testing.T){ if got :=
codexMCPOutputMatches(tc.input, tc.paths); got != tc.want { t.Fatalf("...
expected %v got %v", tc.want, got) } }), keeping the same inputs and assertions;
reference the existing test function name
TestCodexMCPOutputMatchesRequiresCommandAndArgs and the helper
codexMCPOutputMatches/codexPaths to locate code to change.
In `@cmd/codex.go`:
- Around line 167-180: The temp-file write path manually closes and removes the
file in multiple branches (using tmp, tmpName after os.CreateTemp) — replace the
manual closes with a defer-based cleanup: immediately after successful
os.CreateTemp assign tmpName := tmp.Name() and defer func() { _ = tmp.Close();
if shouldRemove { _ = os.Remove(tmpName) }() } (or similar) so the file handle
is always closed; then update the error branches for Write and Close to set
shouldRemove appropriately (or remove only on error) instead of duplicating
tmp.Close()/os.Remove calls; apply the same defer-based pattern to the other
temp-write block using the same tmp/tmpName/Write/Close variables.
- Around line 330-334: codexMCPOutputMatches is comparing the launcher path
string-for-string which will treat
symlink/relative/path-separator/case-equivalent paths as different; instead
canonicalize both the extracted command and paths.launcher before comparing:
call filepath.Abs + filepath.EvalSymlinks + filepath.Clean on both command and
paths.launcher (falling back to Clean if Abs/EvalSymlinks error), normalize
separators with filepath.ToSlash (or ToSlash on both), and on Windows use
strings.EqualFold when comparing; then check equality and the args as before.
Ensure you update references to parseCodexMCPOutput, command, and paths.launcher
in the codexMCPOutputMatches function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8737c470-54a9-4df6-8da5-76385db55dff
📒 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
✅ Files skipped from review due to trivial changes (1)
- README.md
Comment on lines
+369
to
+375
| func isOwnedLumenCodexSessionStartCommand(command string, hook map[string]any, expectedCommand string) bool { | ||
| if expectedCommand != "" && command == expectedCommand { | ||
| return true | ||
| } | ||
| if hook["statusMessage"] == codexHookStatusMessage && | ||
| strings.Contains(command, "hook session-start lumen") { | ||
| return true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify direct interface equality on statusMessage and inspect nearby logic.
rg -n -C3 'hook\["statusMessage"\]\s*==' cmd/codex_hooks.go
# Check whether malformed statusMessage type regression tests exist.
rg -n -C2 'statusMessage' cmd/codex_hooks_test.goRepository: ory/lumen
Length of output: 960
Guard statusMessage type before comparison to prevent runtime panic.
Line 373 directly compares hook["statusMessage"] (type any) to a string without type assertion. If statusMessage is a JSON object (unmarshals as map[string]any) or array (unmarshals as []any), the comparison will panic at runtime because maps and slices are non-comparable types.
This violates the coding guideline: "Panics are only allowed during package initialization, never in business logic." The test code (line 677) demonstrates the correct pattern using type assertion.
Proposed fix
func isOwnedLumenCodexSessionStartCommand(command string, hook map[string]any, expectedCommand string) bool {
if expectedCommand != "" && command == expectedCommand {
return true
}
- if hook["statusMessage"] == codexHookStatusMessage &&
+ statusMessage, hasStatusMessage := hook["statusMessage"].(string)
+ if hasStatusMessage && statusMessage == codexHookStatusMessage &&
strings.Contains(command, "hook session-start lumen") {
return true
}
return isLumenCodexSessionStartCommand(command)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func isOwnedLumenCodexSessionStartCommand(command string, hook map[string]any, expectedCommand string) bool { | |
| if expectedCommand != "" && command == expectedCommand { | |
| return true | |
| } | |
| if hook["statusMessage"] == codexHookStatusMessage && | |
| strings.Contains(command, "hook session-start lumen") { | |
| return true | |
| func isOwnedLumenCodexSessionStartCommand(command string, hook map[string]any, expectedCommand string) bool { | |
| if expectedCommand != "" && command == expectedCommand { | |
| return true | |
| } | |
| statusMessage, hasStatusMessage := hook["statusMessage"].(string) | |
| if hasStatusMessage && statusMessage == codexHookStatusMessage && | |
| strings.Contains(command, "hook session-start lumen") { | |
| return true | |
| } | |
| return isLumenCodexSessionStartCommand(command) | |
| } |
🤖 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 369 - 375, The comparison in
isOwnedLumenCodexSessionStartCommand directly compares hook["statusMessage"]
(type any) to codexHookStatusMessage and can panic if statusMessage is a
non-comparable type; update the function to first type-assert
hook["statusMessage"] to string (e.g., via s, ok :=
hook["statusMessage"].(string)) and only compare when ok is true, otherwise
treat it as not matching; keep the existing strings.Contains(command, "hook
session-start lumen") logic intact so behavior is unchanged for string
statusMessage values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a Codex-specific installer that configures the Lumen MCP server, shared skills, Codex hooks feature flag, and a user-level SessionStart hook for proactive project index warmup.
lumen codex installto install or repair Codex MCP, skills, and hooks.[features] hooks = truein Codexconfig.tomlso installed hooks run without the deprecatedcodex_hookswarning.[features] codex_hooksentries to[features] hooksduring install.lumen codex doctorto report missing, disabled, stale, duplicate, or malformed Codex integration state.hooks.jsonwithout clobbering unrelated user hooks.Test Plan
env CGO_ENABLED=1 go test -tags=fts5 -count=1 ./...make build-localgit diff --check upstream/main...HEAD./bin/lumen codex installand./bin/lumen codex doctorSummary by CodeRabbit
New Features
Documentation
Tests