fix(codex): improve session discovery and diagnostics#328
fix(codex): improve session discovery and diagnostics#328zzzhy11 wants to merge 1 commit intositeboon:mainfrom
Conversation
- Attribute Codex sessions to a project even when created from subdirectories (path normalization + realpath) - Avoid cross-provider sessionId reuse for Codex - Add /api/codex/health safeguards (CLI timeout + cached session stats) - Preserve error codes/details in Codex error mapping and improve message deduplication - Make node-pty optional and degrade terminal output with a bilingual message when unavailable
WalkthroughThis PR enhances Codex integration by making node-pty an optional dependency with graceful fallback, introducing Codex session caching and health checks, refining error handling with structured payloads, and updating approval policy defaults from 'untrusted' to 'never' across the application. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Server
participant Cache as Session Cache
participant FileSystem as File System
Client->>Server: GET /health
Server->>Cache: Check cached health data
alt Cache Hit & Valid
Cache-->>Server: Return cached health
else Cache Miss or Stale
Server->>FileSystem: Scan ~/.codex/sessions
FileSystem-->>Server: Return .jsonl files (up to 5000)
Server->>Server: Build health snapshot<br/>(timestamps, counts)
Server->>Cache: Store in cache (TTL)
end
par Health CLI Check
Server->>FileSystem: Spawn "codex --version"
FileSystem-->>Server: CLI status & version
end
Server->>Server: Gather env flags<br/>(OpenAI vars)
Server-->>Client: Return {cli, sessions, env}
sequenceDiagram
participant Client
participant Server
participant SessionCache as Codex Sessions Cache
participant FileSystem as File System
Client->>Server: getCodexSessionMessages(sessionId, limit, offset)
Server->>SessionCache: Check cache freshness
alt Cache expired or missing
Server->>FileSystem: Scan ~/.codex/sessions
FileSystem-->>Server: Return session .jsonl files
Server->>SessionCache: Refresh & store
else Cache valid
SessionCache-->>Server: Return cached list
end
Server->>FileSystem: Load sessionId .jsonl file
FileSystem-->>Server: Return entries<br/>(event_msg, response_item, user_message)
Server->>Server: Parse & deduplicate<br/>messages across entries
Server->>Server: Apply pagination<br/>(limit, offset)
Server->>Server: Preserve tokenUsage<br/>metadata
Server-->>Client: Return deduped messages<br/>with pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ChatInterface.jsx (1)
3866-3888: Wrap the Codex error case in a block to comply with Biome'snoSwitchDeclarationsrule.
Theconstdeclarations in this case (codexErrorCode,codexErrorDetails,codexErrorHint) are in the switch scope, which can cause TDZ issues if other cases reference them. Wrap the case body in{}to scope these bindings properly. This pattern is already used in adjacent cases likesession-abortedandsession-status.🛠️ Suggested fix
- case 'codex-error': + case 'codex-error': { // Handle Codex errors setIsLoading(false); setCanAbortSession(false); const codexErrorCode = latestMessage.code; const codexErrorDetails = latestMessage.details; const codexErrorHint = (() => { if (codexErrorCode === 'CODEX_THREAD_NOT_FOUND') { return '提示：请新建一个 Codex 会话，或在侧边栏重新选择有效会话后重试。'; } if (codexErrorCode === 'CODEX_AUTH_FAILED') { return '提示：请检查 OPENAI_API_KEY / 本机 Codex 登录状态，然后访问 设置 → Codex → Health 查看诊断信息。'; } return null; })(); setChatMessages(prev => [...prev, { type: 'error', content: [latestMessage.error || 'Codex 出错', codexErrorHint, codexErrorDetails ? `详情：${codexErrorDetails}` : null] .filter(Boolean) .join('\n'), timestamp: new Date() }]); - break; + break; + }
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 97-99: The error message that mentions "Node.js 20" when handling
the optional dependency "node-pty" (in package.json optionalDependencies entry
for "node-pty": "^1.1.0-beta34") is incorrect; change the message to state the
actual minimum Node.js requirement for node-pty (Node.js 16+) or add a clear
comment documenting why the project enforces Node.js 20+ if that's intentional;
update the string where the runtime check or fallback logs the Node.js
requirement so it references "Node.js 16+" (or include the rationale for
requiring 20+) and ensure any user-facing docs/comments reflect the same
requirement.
🧹 Nitpick comments (4)
server/index.js (2)
1063-1073: Close the socket after terminal init fails.After sending the error message, the socket stays open and will still accept inputs that go nowhere. Closing it makes the failure explicit and avoids lingering sessions.
♻️ Proposed tweak
const ptyModule = await getPtyModule(); if (!ptyModule) { ws.send(JSON.stringify({ type: 'output', data: '[ERROR] node-pty is not available, cannot start an interactive terminal. Please use Node.js 20 and reinstall dependencies with C++ Build Tools available, or skip the terminal feature.\r\n[错误] node-pty 不可用，无法启动交互终端。请使用 Node.js 20 并在具备 C++ Build Tools 的环境重新安装依赖，或跳过终端功能。' })); + ws.close(1011, 'Terminal unavailable'); return; }
79-87: Add error logging and retry capability to node-pty import handler.The current silent catch prevents visibility into why node-pty might fail to load. Logging the error and resetting the cached promise enables both better diagnostics and retry capability if the optional dependency is installed after startup.
♻️ Proposed improvement
let ptyModulePromise = null; async function getPtyModule() { if (!ptyModulePromise) { ptyModulePromise = import('node-pty') .then((mod) => mod?.default || mod) - .catch(() => null); + .catch((err) => { + console.warn('[WARN] node-pty load failed:', err); + ptyModulePromise = null; + return null; + }); } return ptyModulePromise; }server/routes/codex.js (1)
17-76: Avoid exposing absolute session paths in the health response.
recentcurrently returns full filesystem paths. Consider returning paths relative to the sessions dir (or just filenames) to avoid leaking user-home paths.♻️ Proposed tweak
sessions.recent = stats .filter(Boolean) .sort((a, b) => b.mtimeMs - a.mtimeMs) .slice(0, 5) - .map((s) => ({ file: s.file, mtime: new Date(s.mtimeMs).toISOString() })); + .map((s) => ({ + file: path.relative(codexSessionsDir, s.file), + mtime: new Date(s.mtimeMs).toISOString() + }));server/projects.js (1)
211-226: Consider logging errors in directory traversal.The empty
catch {}block at line 224 silently swallows all errors during recursive directory scanning. While ignoringENOENTis reasonable, permission errors or other issues might be worth logging for debugging purposes.Suggested improvement
} catch (error) { + if (error.code !== 'ENOENT') { + console.warn(`Could not read directory ${dir}:`, error.message); + } }
| "optionalDependencies": { | ||
| "node-pty": "^1.1.0-beta34" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
node-pty 1.1.0-beta34 Node.js version compatibility requirements
💡 Result:
node-pty v1.1.0-beta34 requires Node.js 16 (or newer) — or Electron 19 when used inside Electron. [1][2]
Sources:
[1] microsoft/node-pty README (requirements).
[2] node-pty v1.1.0-beta34 release.
🏁 Script executed:
cat -n package.json | grep -A 5 -B 5 '"engines"'Repository: siteboon/claudecodeui
Length of output: 47
🏁 Script executed:
wc -l package.jsonRepository: siteboon/claudecodeui
Length of output: 81
🏁 Script executed:
cat -n package.jsonRepository: siteboon/claudecodeui
Length of output: 4073
🏁 Script executed:
rg "node-pty" --type js --type ts -B 2 -A 2Repository: siteboon/claudecodeui
Length of output: 873
🏁 Script executed:
rg "Node.js 20" -B 5 -A 5Repository: siteboon/claudecodeui
Length of output: 892
Clarify Node.js version requirement in error message.
The optional dependency handling is correctly implemented with graceful fallback. However, the error message references Node.js 20 as a requirement, whereas node-pty 1.1.0-beta34 only requires Node.js 16+. Either update the error message to reflect the actual minimum requirement, or document why Node.js 20+ is being enforced.
🤖 Prompt for AI Agents
In `@package.json` around lines 97 - 99, The error message that mentions "Node.js
20" when handling the optional dependency "node-pty" (in package.json
optionalDependencies entry for "node-pty": "^1.1.0-beta34") is incorrect; change
the message to state the actual minimum Node.js requirement for node-pty
(Node.js 16+) or add a clear comment documenting why the project enforces
Node.js 20+ if that's intentional; update the string where the runtime check or
fallback logs the Node.js requirement so it references "Node.js 16+" (or include
the rationale for requiring 20+) and ensure any user-facing docs/comments
reflect the same requirement.
|
@zzzhy11 What issue did you face that this PR solves? Can you also share your reproduction steps for the error? |
2 participants
Summary
This PR improves Codex support by addressing session attribution, manual triggering reliability, and diagnostics.
Changes
/api/codex/healthsafeguards (Codex CLI probe timeout + cached session stats)error.code/error.detailsin Codex error mapping and strengthen JSONL message de-duplicationnode-ptyoptional and gracefully degrade terminal startup with a bilingual fallback messageTesting
node --checkon modified server modulesnpm run buildNotes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.