fix(codex): improve session attribution and manual triggering#326
Closed
zzzhy11 wants to merge 1 commit intositeboon:mainfrom
Closed
fix(codex): improve session attribution and manual triggering#326zzzhy11 wants to merge 1 commit intositeboon:mainfrom
zzzhy11 wants to merge 1 commit intositeboon:mainfrom
Conversation
Contributor
WalkthroughThis PR implements a multi-phase plan to fix Codex session discovery, attribution, and error handling. Changes include introducing Codex session caching with Windows path normalization, restructuring error reporting with standardized payloads, adding a health check endpoint, making node-pty optional, updating session ID resolution to be provider-aware, and enhancing message parsing for Codex events. Changes
Sequence DiagramssequenceDiagram
participant Client as ChatInterface (Client)
participant Server as Server (Express)
participant Codex as Codex CLI/Session Files
participant Cache as Cache Layer
Client->>Server: queryCodex(projectPath, sessionId)
Server->>Cache: Check codexSessionsScanCache
alt Cache Hit
Cache-->>Server: Return cached sessions
else Cache Miss
Server->>Codex: Scan session directory
Codex-->>Server: List .jsonl files
Server->>Cache: Store normalized sessions
end
Server->>Server: Normalize paths (Windows/Unix)
Server->>Server: Match project to sessions
Server->>Codex: Fetch session messages
Codex-->>Server: Return event stream
Server->>Server: Parse & deduplicate messages
Server->>Client: Return structured response or error
alt Error Path
Server->>Server: codexErrorToPayload()
Server->>Client: Structured error (code, message, details)
end
sequenceDiagram
participant Client as Client
participant Handler as ChatInterface
participant Server as openai-codex.js
participant Codex as Codex API/Thread
Client->>Handler: Send message (provider='codex')
Handler->>Handler: Resolve effectiveSessionId (provider-aware)
Handler->>Server: queryCodex(sessionId, model, options)
Server->>Server: Emit verbose context logging
Server->>Codex: resumeThread(sessionId)
alt Resume Success
Codex-->>Server: Thread resumed
Server->>Codex: Stream completion
Codex-->>Server: Tokens + content
Server->>Server: Emit token-budget message
Server-->>Handler: Streamed response
else Resume Failure
Codex-->>Server: Error response
Server->>Server: codexErrorToPayload(error)
Server-->>Handler: Structured error payload
Handler->>Client: Display error with hint & details
end
sequenceDiagram
participant Client as Health Check Client
participant Handler as /api/codex/health
participant Shell as Shell Execution
participant FS as Filesystem
participant Codex as Codex CLI/Sessions
Client->>Handler: GET /api/codex/health
Handler->>Shell: Execute 'codex --version'
alt CLI Available
Shell-->>Handler: Version output
Handler->>Handler: Set cli.available = true
else CLI Not Found
Shell-->>Handler: Error/timeout
Handler->>Handler: Set cli.available = false
end
Handler->>FS: Scan CODEX_SESSIONS_DIR recursively
FS-->>Handler: List .jsonl files with mtime
Handler->>Handler: Sort by mtime, extract top 5
Handler->>Handler: Count sessions, determine freshness
Handler-->>Client: JSON {success, cli, sessions, env}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
- 会话发现支持子目录/路径规范化并加入扫描缓存 - Codex sessionId 逻辑与 Cursor 隔离，避免跨 provider 续聊失败 - 默认 approvalPolicy 改为 never，新增 /api/codex/health 与更清晰错误 - 将 node-pty 设为 optionalDependencies 并在缺失时降级提示
zzzhy11
force-pushed
the
fix/codex-support-phase1-2
branch
from
45f8d17 to
66cdbb7
Compare
zzzhy11
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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-3885: Wrap thecodex-errorcase in a block to fix Biome'snoSwitchDeclarationslint error.The
constdeclarations (codexErrorCode,codexErrorDetails,codexErrorHint) inside this case are scoped to the entire switch statement, not just the case block, which violates Biome'snoSwitchDeclarationsrule. This creates a Temporal Dead Zone hazard. Wrap the case body in braces to properly scope these variables.Proposed 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; + }
🤖 Fix all issues with AI agents
In `@Codex支持整改计划.md`:
- Around line 322-342: The document uses bold-only lines as section headings
(e.g., the lines starting with **会话发现 / 归属（Phase 1）**, **手动触发 / sessionId
管理（Phase 2）**, **诊断与可观测性（Phase 0）**, **Codex-only 项目可见性（计划外但用于“看得见历史”的必要补齐）**,
**运行时依赖与兼容性（工程性变更）**) which triggers markdownlint MD036; replace each of those
bold-only lines with proper markdown headings (for example, prepend ### or
another appropriate heading level) so they become real headings, leaving the
surrounding bullet lists and file references (server/projects.js,
src/components/ChatInterface.jsx, server/openai-codex.js,
server/routes/codex.js) unchanged and preserving their content and indentation.
In `@server/index.js`:
- Around line 1063-1069: The fallback terminal error message sent by ws.send
when getPtyModule() returns null is hard-coded in Chinese; update the handler in
the getPtyModule() null branch to produce a localized message (e.g., via the
existing i18n/localization helper or by including both English and Chinese)
instead of a single-language string—modify the payload created for ws.send
(type: 'output', data: ...) so it uses the project's localization method or
returns a bilingual string; ensure you update the same branch where
getPtyModule() is awaited and the early return occurs.
In `@server/openai-codex.js`:
- Around line 186-224: codexErrorToPayload currently re-classifies every error
from resumeThread by parsing the message and drops any pre-set
err.code/err.details; update codexErrorToPayload to first check for an existing
error.code on the incoming error and, if present, return that code with message
= error.message || rawMessage and details = error.details || rawMessage
(preserving any localized message and original details), otherwise fall back to
the existing status/message-based classification logic; reference the function
codexErrorToPayload and the incoming properties error.code and error.details
when making the change.
In `@server/projects.js`:
- Around line 1867-1877: The deduplication key in the messages loop (variables:
messages, deduped, seen, role, content, key) is collapsing distinct tool events
when message.content is empty; include tool identifiers and output by extending
the key to incorporate m.toolCallId (or m.message.toolCallId), m.toolName (or
m.message.toolName) and the tool output (m.output or m.message.output) when
present so different tool calls at the same timestamp produce different keys;
update the key construction logic in that loop to concatenate these additional
fields with safe fallbacks to empty strings.
In `@server/routes/codex.js`:
- Around line 25-45: The CLI probe promise that spawns codex via spawn('codex',
['--version']) can hang; add a timeout inside the Promise (use setTimeout) that,
after e.g. 5s, kills the child (proc.kill()), sets result.cli.error to a timeout
message and resolves the promise; ensure you clear the timer on normal
completion (clearTimeout), and guard finalization so resolve() is only called
once by removing or ignoring event listeners (or using a finished flag) in the
handlers for proc.on('close') and proc.on('error') to avoid double-resolving;
update references around spawn, proc, and result.cli accordingly.
🧹 Nitpick comments (1)
server/routes/codex.js (1)
52-82: Consider capping or caching the recursive session scan.
A full recursive stat of all.jsonlfiles on every/healthcall can be expensive if the endpoint is polled. Consider reusing a cached scan helper or adding a file-count/time budget.
Comment on lines
+322
to
+342
| **会话发现 / 归属（Phase 1）** | ||
|
|
||
| - `server/projects.js`：重写 Codex 会话归属判定，支持 Windows `\\?\\`、分隔符/大小写/尾分隔符规范化，并将“项目根目录子目录运行”的会话正确归属到项目。 | ||
| - `server/projects.js`：Codex 会话扫描改为“全量返回”，并加入全盘扫描缓存（避免项目列表刷新时频繁扫描 `~/.codex/sessions` 造成卡顿）。 | ||
| - `server/projects.js`：补齐 Codex 历史回放对 `event_msg.user_message` 的解析，并对可能的重复事件做去重，改善“回放不全/只见 assistant”问题。 | ||
|
|
||
| **手动触发 / sessionId 管理（Phase 2）** | ||
|
|
||
| - `src/components/ChatInterface.jsx`：按 provider 隔离 `effectiveSessionId` 兜底逻辑，Codex 不再复用 `cursorSessionId`，避免跨 Provider resume 导致失败。 | ||
| - `server/openai-codex.js`：默认 `approvalPolicy` 调整为 `never`（在未实现 Codex 审批 UI 前，避免“等待审批/卡住”），并增强错误结构（code/details）与后端日志。 | ||
| - `src/components/ChatInterface.jsx`：Codex 错误展示增加可操作提示（如会话不存在/认证问题等）。 | ||
|
|
||
| **诊断与可观测性（Phase 0）** | ||
|
|
||
| - `server/routes/codex.js`：新增 `/api/codex/health`，用于检查 CLI 可用性、sessions 目录统计与关键环境变量状态（仍受鉴权保护）。 | ||
|
|
||
| **Codex-only 项目可见性（计划外但用于“看得见历史”的必要补齐）** | ||
|
|
||
| - `server/projects.js`：新增基于 `~/.codex/sessions` 的项目自动发现（按 `cwd` 推断 `git root`），并写入 `~/.claude/project-config.json`（标记 `source=codex-auto`），以便左侧项目列表能覆盖“仅 Codex 有会话、Claude 项目不存在”的目录。 | ||
|
|
||
| **运行时依赖与兼容性（工程性变更）** |
Contributor
There was a problem hiding this comment.
Replace bold-only “headings” with actual markdown headings.
markdownlint MD036 flags emphasis used as headings. Convert those bold lines to ### (or similar) headings for lint compliance.
✏️ Example fix
-**会话发现 / 归属（Phase 1）**
+### 会话发现 / 归属（Phase 1）
-**手动触发 / sessionId 管理（Phase 2）**
+### 手动触发 / sessionId 管理（Phase 2）
-**诊断与可观测性（Phase 0）**
+### 诊断与可观测性（Phase 0）
-**Codex-only 项目可见性（计划外但用于“看得见历史”的必要补齐）**
+### Codex-only 项目可见性（计划外但用于“看得见历史”的必要补齐）
-**运行时依赖与兼容性（工程性变更）**
+### 运行时依赖与兼容性（工程性变更）📝 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
| **会话发现 / 归属（Phase 1）** | |
| - `server/projects.js`：重写 Codex 会话归属判定，支持 Windows `\\?\\`、分隔符/大小写/尾分隔符规范化，并将“项目根目录子目录运行”的会话正确归属到项目。 | |
| - `server/projects.js`：Codex 会话扫描改为“全量返回”，并加入全盘扫描缓存（避免项目列表刷新时频繁扫描 `~/.codex/sessions` 造成卡顿）。 | |
| - `server/projects.js`：补齐 Codex 历史回放对 `event_msg.user_message` 的解析，并对可能的重复事件做去重，改善“回放不全/只见 assistant”问题。 | |
| **手动触发 / sessionId 管理（Phase 2）** | |
| - `src/components/ChatInterface.jsx`：按 provider 隔离 `effectiveSessionId` 兜底逻辑，Codex 不再复用 `cursorSessionId`，避免跨 Provider resume 导致失败。 | |
| - `server/openai-codex.js`：默认 `approvalPolicy` 调整为 `never`（在未实现 Codex 审批 UI 前，避免“等待审批/卡住”），并增强错误结构（code/details）与后端日志。 | |
| - `src/components/ChatInterface.jsx`：Codex 错误展示增加可操作提示（如会话不存在/认证问题等）。 | |
| **诊断与可观测性（Phase 0）** | |
| - `server/routes/codex.js`：新增 `/api/codex/health`，用于检查 CLI 可用性、sessions 目录统计与关键环境变量状态（仍受鉴权保护）。 | |
| **Codex-only 项目可见性（计划外但用于“看得见历史”的必要补齐）** | |
| - `server/projects.js`：新增基于 `~/.codex/sessions` 的项目自动发现（按 `cwd` 推断 `git root`），并写入 `~/.claude/project-config.json`（标记 `source=codex-auto`），以便左侧项目列表能覆盖“仅 Codex 有会话、Claude 项目不存在”的目录。 | |
| **运行时依赖与兼容性（工程性变更）** | |
| ### 会话发现 / 归属（Phase 1） | |
| - `server/projects.js`：重写 Codex 会话归属判定，支持 Windows `\\?\\`、分隔符/大小写/尾分隔符规范化，并将"项目根目录子目录运行"的会话正确归属到项目。 | |
| - `server/projects.js`：Codex 会话扫描改为"全量返回"，并加入全盘扫描缓存（避免项目列表刷新时频繁扫描 `~/.codex/sessions` 造成卡顿）。 | |
| - `server/projects.js`：补齐 Codex 历史回放对 `event_msg.user_message` 的解析，并对可能的重复事件做去重，改善"回放不全/只见 assistant"问题。 | |
| ### 手动触发 / sessionId 管理（Phase 2） | |
| - `src/components/ChatInterface.jsx`：按 provider 隔离 `effectiveSessionId` 兜底逻辑，Codex 不再复用 `cursorSessionId`，避免跨 Provider resume 导致失败。 | |
| - `server/openai-codex.js`：默认 `approvalPolicy` 调整为 `never`（在未实现 Codex 审批 UI 前，避免"等待审批/卡住"），并增强错误结构（code/details）与后端日志。 | |
| - `src/components/ChatInterface.jsx`：Codex 错误展示增加可操作提示（如会话不存在/认证问题等）。 | |
| ### 诊断与可观测性（Phase 0） | |
| - `server/routes/codex.js`：新增 `/api/codex/health`，用于检查 CLI 可用性、sessions 目录统计与关键环境变量状态（仍受鉴权保护）。 | |
| ### Codex-only 项目可见性（计划外但用于"看得见历史"的必要补齐） | |
| - `server/projects.js`：新增基于 `~/.codex/sessions` 的项目自动发现（按 `cwd` 推断 `git root`），并写入 `~/.claude/project-config.json`（标记 `source=codex-auto`），以便左侧项目列表能覆盖"仅 Codex 有会话、Claude 项目不存在"的目录。 | |
| ### 运行时依赖与兼容性（工程性变更） |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~326-~326: 您的意思是“"不"齐”？
Context: ...sessions造成卡顿）。 -server/projects.js：补齐 Codex 历史回放对 event_msg.user_message` 的...
(BU)
[uncategorized] ~338-338: 您的意思是“"不"齐”？/.co...
Context: ...护）。 Codex-only 项目可见性（计划外但用于“看得见历史”的必要补齐） - server/projects.js：新增基于 `
(BU)
🪛 markdownlint-cli2 (0.18.1)
322-322: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
328-328: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
334-334: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
338-338: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
342-342: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In `@Codex支持整改计划.md` around lines 322 - 342, The document uses bold-only lines as
section headings (e.g., the lines starting with **会话发现 / 归属（Phase 1）**, **手动触发 /
sessionId 管理（Phase 2）**, **诊断与可观测性（Phase 0）**, **Codex-only
项目可见性（计划外但用于“看得见历史”的必要补齐）**, **运行时依赖与兼容性（工程性变更）**) which triggers markdownlint
MD036; replace each of those bold-only lines with proper markdown headings (for
example, prepend ### or another appropriate heading level) so they become real
headings, leaving the surrounding bullet lists and file references
(server/projects.js, src/components/ChatInterface.jsx, server/openai-codex.js,
server/routes/codex.js) unchanged and preserving their content and indentation.
Comment on lines
+1063
to
+1069
| const ptyModule = await getPtyModule(); | ||
| if (!ptyModule) { | ||
| ws.send(JSON.stringify({ | ||
| type: 'output', | ||
| data: '[ERROR] node-pty 不可用，无法启动交互终端。请使用 Node 20 并在具备 C++ Build Tools 的环境重新安装依赖，或跳过终端功能。' | ||
| })); | ||
| return; |
Contributor
There was a problem hiding this comment.
Localize the user-facing fallback message.
The terminal error text is hard-coded in one language; consider making it bilingual or routing through i18n for consistency.
🤖 Prompt for AI Agents
In `@server/index.js` around lines 1063 - 1069, The fallback terminal error
message sent by ws.send when getPtyModule() returns null is hard-coded in
Chinese; update the handler in the getPtyModule() null branch to produce a
localized message (e.g., via the existing i18n/localization helper or by
including both English and Chinese) instead of a single-language string—modify
the payload created for ws.send (type: 'output', data: ...) so it uses the
project's localization method or returns a bilingual string; ensure you update
the same branch where getPtyModule() is awaited and the early return occurs.
Comment on lines
+186
to
+224
| function codexErrorToPayload(error) { | ||
| const rawMessage = String(error?.message || 'Unknown error'); | ||
| const status = error?.status || error?.response?.status; | ||
|
|
||
| const lower = rawMessage.toLowerCase(); | ||
| const looksLikeNotFound = | ||
| status === 404 || | ||
| lower.includes('not found') || | ||
| lower.includes('thread') && lower.includes('missing'); | ||
|
|
||
| if (looksLikeNotFound) { | ||
| return { | ||
| code: 'CODEX_THREAD_NOT_FOUND', | ||
| message: 'Codex 会话不存在或已失效：请在侧边栏选择一个有效会话，或新建会话后重试。', | ||
| details: rawMessage | ||
| }; | ||
| } | ||
|
|
||
| const looksLikeAuth = | ||
| status === 401 || | ||
| status === 403 || | ||
| lower.includes('api key') || | ||
| lower.includes('unauthorized') || | ||
| lower.includes('forbidden'); | ||
|
|
||
| if (looksLikeAuth) { | ||
| return { | ||
| code: 'CODEX_AUTH_FAILED', | ||
| message: 'Codex 认证失败：请检查环境变量/配置（如 OPENAI_API_KEY），或确认本机已完成 Codex 登录。', | ||
| details: rawMessage | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| code: 'CODEX_ERROR', | ||
| message: rawMessage, | ||
| details: rawMessage | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve pre‑classified error codes to keep UI hints.
Errors rethrown from resumeThread carry err.code/err.details, but codexErrorToPayload discards them and re‑classifies based on message text. If the message is localized, it will fall back to CODEX_ERROR, so the frontend loses CODEX_THREAD_NOT_FOUND / CODEX_AUTH_FAILED hints.
✅ Suggested fix
function codexErrorToPayload(error) {
+ if (error?.code) {
+ const msg = String(error.message || 'Unknown error');
+ return {
+ code: error.code,
+ message: msg,
+ details: error.details ?? msg
+ };
+ }
const rawMessage = String(error?.message || 'Unknown error');
const status = error?.status || error?.response?.status;📝 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
| function codexErrorToPayload(error) { | |
| const rawMessage = String(error?.message || 'Unknown error'); | |
| const status = error?.status || error?.response?.status; | |
| const lower = rawMessage.toLowerCase(); | |
| const looksLikeNotFound = | |
| status === 404 || | |
| lower.includes('not found') || | |
| lower.includes('thread') && lower.includes('missing'); | |
| if (looksLikeNotFound) { | |
| return { | |
| code: 'CODEX_THREAD_NOT_FOUND', | |
| message: 'Codex 会话不存在或已失效：请在侧边栏选择一个有效会话，或新建会话后重试。', | |
| details: rawMessage | |
| }; | |
| } | |
| const looksLikeAuth = | |
| status === 401 || | |
| status === 403 || | |
| lower.includes('api key') || | |
| lower.includes('unauthorized') || | |
| lower.includes('forbidden'); | |
| if (looksLikeAuth) { | |
| return { | |
| code: 'CODEX_AUTH_FAILED', | |
| message: 'Codex 认证失败：请检查环境变量/配置（如 OPENAI_API_KEY），或确认本机已完成 Codex 登录。', | |
| details: rawMessage | |
| }; | |
| } | |
| return { | |
| code: 'CODEX_ERROR', | |
| message: rawMessage, | |
| details: rawMessage | |
| }; | |
| } | |
| function codexErrorToPayload(error) { | |
| if (error?.code) { | |
| const msg = String(error.message || 'Unknown error'); | |
| return { | |
| code: error.code, | |
| message: msg, | |
| details: error.details ?? msg | |
| }; | |
| } | |
| const rawMessage = String(error?.message || 'Unknown error'); | |
| const status = error?.status || error?.response?.status; | |
| const lower = rawMessage.toLowerCase(); | |
| const looksLikeNotFound = | |
| status === 404 || | |
| lower.includes('not found') || | |
| lower.includes('thread') && lower.includes('missing'); | |
| if (looksLikeNotFound) { | |
| return { | |
| code: 'CODEX_THREAD_NOT_FOUND', | |
| message: 'Codex 会话不存在或已失效：请在侧边栏选择一个有效会话，或新建会话后重试。', | |
| details: rawMessage | |
| }; | |
| } | |
| const looksLikeAuth = | |
| status === 401 || | |
| status === 403 || | |
| lower.includes('api key') || | |
| lower.includes('unauthorized') || | |
| lower.includes('forbidden'); | |
| if (looksLikeAuth) { | |
| return { | |
| code: 'CODEX_AUTH_FAILED', | |
| message: 'Codex 认证失败：请检查环境变量/配置（如 OPENAI_API_KEY），或确认本机已完成 Codex 登录。', | |
| details: rawMessage | |
| }; | |
| } | |
| return { | |
| code: 'CODEX_ERROR', | |
| message: rawMessage, | |
| details: rawMessage | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@server/openai-codex.js` around lines 186 - 224, codexErrorToPayload currently
re-classifies every error from resumeThread by parsing the message and drops any
pre-set err.code/err.details; update codexErrorToPayload to first check for an
existing error.code on the incoming error and, if present, return that code with
message = error.message || rawMessage and details = error.details || rawMessage
(preserving any localized message and original details), otherwise fall back to
the existing status/message-based classification logic; reference the function
codexErrorToPayload and the incoming properties error.code and error.details
when making the change.
Comment on lines
+1867
to
+1877
| // Best-effort de-duplication (某些日志会同时记录 event_msg 与 response_item) | ||
| const deduped = []; | ||
| const seen = new Set(); | ||
| for (const m of messages) { | ||
| const role = m?.message?.role || m.type || ''; | ||
| const content = typeof m?.message?.content === 'string' ? m.message.content : (m.output || ''); | ||
| const key = `${m.timestamp || ''}::${role}::${content}`; | ||
| if (seen.has(key)) continue; | ||
| seen.add(key); | ||
| deduped.push(m); | ||
| } |
Contributor
There was a problem hiding this comment.
Strengthen Codex message de‑dup keys to avoid dropping tool events.
tool_use / tool_result entries often have empty message.content, so different tool calls at the same timestamp can collapse into one. Include toolCallId/toolName (and output) in the key to prevent data loss.
🔧 Proposed fix
- const deduped = [];
- const seen = new Set();
- for (const m of messages) {
- const role = m?.message?.role || m.type || '';
- const content = typeof m?.message?.content === 'string' ? m.message.content : (m.output || '');
- const key = `${m.timestamp || ''}::${role}::${content}`;
- if (seen.has(key)) continue;
- seen.add(key);
- deduped.push(m);
- }
+ const deduped = [];
+ const seen = new Set();
+ for (const m of messages) {
+ const role = m?.message?.role || m.type || '';
+ const content = typeof m?.message?.content === 'string' ? m.message.content : '';
+ const toolName = m.toolName || '';
+ const toolCallId = m.toolCallId || '';
+ const output = typeof m.output === 'string' ? m.output : JSON.stringify(m.output ?? '');
+ const key = `${m.timestamp || ''}::${role}::${toolName}::${toolCallId}::${content}::${output}`;
+ if (seen.has(key)) continue;
+ seen.add(key);
+ deduped.push(m);
+ }🤖 Prompt for AI Agents
In `@server/projects.js` around lines 1867 - 1877, The deduplication key in the
messages loop (variables: messages, deduped, seen, role, content, key) is
collapsing distinct tool events when message.content is empty; include tool
identifiers and output by extending the key to incorporate m.toolCallId (or
m.message.toolCallId), m.toolName (or m.message.toolName) and the tool output
(m.output or m.message.output) when present so different tool calls at the same
timestamp produce different keys; update the key construction logic in that loop
to concatenate these additional fields with safe fallbacks to empty strings.
Comment on lines
+25
to
+45
| // CLI availability | ||
| await new Promise((resolve) => { | ||
| const proc = spawn('codex', ['--version'], { stdio: ['ignore', 'pipe', 'pipe'] }); | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
| proc.stdout?.on('data', (d) => { stdout += d.toString(); }); | ||
| proc.stderr?.on('data', (d) => { stderr += d.toString(); }); | ||
| proc.on('close', (code) => { | ||
| if (code === 0) { | ||
| result.cli.installed = true; | ||
| result.cli.version = stdout.trim() || null; | ||
| } else { | ||
| result.cli.error = stderr.trim() || `Exited with code ${code}`; | ||
| } | ||
| resolve(); | ||
| }); | ||
| proc.on('error', (error) => { | ||
| result.cli.error = error?.code === 'ENOENT' ? 'Codex CLI not installed' : error.message; | ||
| resolve(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add a timeout to the CLI version probe.
If the codex --version process hangs, the /health request can stall indefinitely.
🛠️ Suggested fix (timeout + safe finalize)
// CLI availability
await new Promise((resolve) => {
const proc = spawn('codex', ['--version'], { stdio: ['ignore', 'pipe', 'pipe'] });
let stdout = '';
let stderr = '';
+ let settled = false;
+ let timeoutId = null;
+ const finish = () => {
+ if (settled) return;
+ settled = true;
+ if (timeoutId) clearTimeout(timeoutId);
+ resolve();
+ };
+ timeoutId = setTimeout(() => {
+ result.cli.error = 'Codex CLI version check timed out';
+ try { proc.kill('SIGKILL'); } catch {}
+ finish();
+ }, 5000);
proc.stdout?.on('data', (d) => { stdout += d.toString(); });
proc.stderr?.on('data', (d) => { stderr += d.toString(); });
proc.on('close', (code) => {
if (code === 0) {
result.cli.installed = true;
result.cli.version = stdout.trim() || null;
} else {
result.cli.error = stderr.trim() || `Exited with code ${code}`;
}
- resolve();
+ finish();
});
proc.on('error', (error) => {
result.cli.error = error?.code === 'ENOENT' ? 'Codex CLI not installed' : error.message;
- resolve();
+ finish();
});
});🤖 Prompt for AI Agents
In `@server/routes/codex.js` around lines 25 - 45, The CLI probe promise that
spawns codex via spawn('codex', ['--version']) can hang; add a timeout inside
the Promise (use setTimeout) that, after e.g. 5s, kills the child (proc.kill()),
sets result.cli.error to a timeout message and resolves the promise; ensure you
clear the timer on normal completion (clearTimeout), and guard finalization so
resolve() is only called once by removing or ignoring event listeners (or using
a finished flag) in the handlers for proc.on('close') and proc.on('error') to
avoid double-resolving; update references around spawn, proc, and result.cli
accordingly.
Author
|
Superseded by #328 (rebased on latest main, removes planning doc, and addresses review feedback). |
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
1 participant
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
Implements Phase 0/1/2 items from "Codex ??????":
Testing
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.