Codex integration fixes + agent version detection - #407
Closed
openasocket wants to merge 212 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPR introduces agent version detection with enforced minimum versions, refactors Codex parser for dual format support (item/msg-envelope), implements pending-result tracking for multi-turn handling, adds activity/stall detection for hung tasks, establishes group chat operation locking, and displays detected agent versions in UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Detector as AgentDetector
participant Binary as Agent Binary
participant VersionCheck as Version Validator
participant Config as AgentConfig
Detector->>Binary: spawn with versionArgs (--version)
activate Binary
Binary-->>Detector: version output (timeout: 3s)
deactivate Binary
Detector->>Detector: parseVersionString(output)
Detector->>Config: set detectedVersion
VersionCheck->>Config: read detectedVersion
VersionCheck->>VersionCheck: compareVersions(detected, MIN_VERSION)
alt version < minimum
VersionCheck->>VersionCheck: emit warning
else version >= minimum
VersionCheck->>VersionCheck: proceed normally
end
sequenceDiagram
participant Parser as CodexOutputParser
participant Line as OutputLine
participant Router as routeMessage
participant Handlers as Format Handlers
participant State as Internal State
Line->>Parser: incoming message line
Parser->>Parser: parseJSON(line)
Router->>Router: detect envelope structure
alt msg-envelope detected
Router->>Handlers: transformNewFormat()
Handlers->>Handlers: route by event.type
Handlers->>State: manage toolNamesByCallId
else item-envelope detected
Router->>Handlers: transformOldFormat()
Handlers->>State: manage lastToolName
else config/non-envelope
Router->>Handlers: transformConfigLine()
Handlers->>State: update model, contextWindow
end
Handlers->>Parser: return formatted result
sequenceDiagram
participant Agent as Agent Execution
participant Activity as Activity Tracker
participant Monitor as Watchdog Monitor
participant Process as Process Manager
Agent->>Activity: markActivity() on onData
Agent->>Activity: markActivity() on onThinkingChunk
Agent->>Activity: markActivity() on onToolExecution
Monitor->>Activity: check lastActivityTime (30s interval)
Monitor->>Monitor: elapsed = now - lastActivityTime
alt elapsed > 5 minutes
Monitor->>Monitor: log inactivity
Monitor->>Process: kill(taskId)
Monitor->>Monitor: clear activity interval
else elapsed <= 5 minutes
Monitor->>Monitor: continue monitoring
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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🧪 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/agent/useAgentExecution.ts`:
- Around line 202-229: The watchdog currently swallows errors from
window.maestro.process.kill(...) via .catch(() => {}); update the inactivity
check in useAgentExecution (the interval using ACTIVITY_TIMEOUT_MS and
activityCheckInterval) to surface kill failures by replacing the empty catch
with an error reporter: call the app logger at error level with context
(targetSessionId, idleMs, agentType) and invoke the Sentry utility
captureException (imported from src/utils/sentry) with the caught error and the
same context so failures are recorded; do not silently suppress the exception.
Comment on lines
+202
to
+229
| // Activity tracking for per-task stall detection. | ||
| // Resets on any signal: data output, thinking chunks, or tool execution. | ||
| // This bridges the IPC channel gap for agents like Codex whose reasoning | ||
| // and tool events travel on separate channels from data output. | ||
| let lastActivityTime = Date.now(); | ||
| const markActivity = () => { | ||
| lastActivityTime = Date.now(); | ||
| }; | ||
|
|
||
| // Per-task activity timeout: if no activity (data, thinking, tool) for | ||
| // this duration, assume the agent is hung and kill it. This catches | ||
| // mid-execution hangs that the post-execution stall detector in | ||
| // useBatchProcessor cannot detect (it only triggers after task completion). | ||
| const ACTIVITY_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes | ||
| const activityCheckInterval = setInterval(() => { | ||
| const idleMs = Date.now() - lastActivityTime; | ||
| if (idleMs > ACTIVITY_TIMEOUT_MS) { | ||
| clearInterval(activityCheckInterval); | ||
| window.maestro.logger.log('warn', 'Auto Run task killed due to inactivity', 'AgentExecution', { | ||
| targetSessionId, | ||
| idleMs, | ||
| agentType: session.toolType, | ||
| }); | ||
| // Kill the hung process — onExit listener will fire and resolve the promise | ||
| window.maestro.process.kill(targetSessionId).catch(() => {}); | ||
| } | ||
| }, 30_000); // Check every 30 seconds | ||
|
|
There was a problem hiding this comment.
Don’t silently swallow watchdog kill failures.
kill() errors are currently ignored, which hides failures in the inactivity watchdog. Please capture and report the error so it surfaces in Sentry.
Proposed fix
-import { useCallback, useRef } from 'react';
+import { useCallback, useRef } from 'react';
+import { captureException } from '../../../utils/sentry';
@@
- window.maestro.process.kill(targetSessionId).catch(() => {});
+ window.maestro.process.kill(targetSessionId).catch((err) => {
+ captureException(err, {
+ contexts: {
+ activityWatchdog: {
+ targetSessionId,
+ agentType: session.toolType,
+ idleMs,
+ },
+ },
+ });
+ });As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking. Handle only expected/recoverable errors explicitly with specific error codes. Let unexpected errors bubble up to Sentry. Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
📝 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
| // Activity tracking for per-task stall detection. | |
| // Resets on any signal: data output, thinking chunks, or tool execution. | |
| // This bridges the IPC channel gap for agents like Codex whose reasoning | |
| // and tool events travel on separate channels from data output. | |
| let lastActivityTime = Date.now(); | |
| const markActivity = () => { | |
| lastActivityTime = Date.now(); | |
| }; | |
| // Per-task activity timeout: if no activity (data, thinking, tool) for | |
| // this duration, assume the agent is hung and kill it. This catches | |
| // mid-execution hangs that the post-execution stall detector in | |
| // useBatchProcessor cannot detect (it only triggers after task completion). | |
| const ACTIVITY_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes | |
| const activityCheckInterval = setInterval(() => { | |
| const idleMs = Date.now() - lastActivityTime; | |
| if (idleMs > ACTIVITY_TIMEOUT_MS) { | |
| clearInterval(activityCheckInterval); | |
| window.maestro.logger.log('warn', 'Auto Run task killed due to inactivity', 'AgentExecution', { | |
| targetSessionId, | |
| idleMs, | |
| agentType: session.toolType, | |
| }); | |
| // Kill the hung process — onExit listener will fire and resolve the promise | |
| window.maestro.process.kill(targetSessionId).catch(() => {}); | |
| } | |
| }, 30_000); // Check every 30 seconds | |
| import { useCallback, useRef } from 'react'; | |
| import { captureException } from '../../../utils/sentry'; | |
| // ... other code ... | |
| // Activity tracking for per-task stall detection. | |
| // Resets on any signal: data output, thinking chunks, or tool execution. | |
| // This bridges the IPC channel gap for agents like Codex whose reasoning | |
| // and tool events travel on separate channels from data output. | |
| let lastActivityTime = Date.now(); | |
| const markActivity = () => { | |
| lastActivityTime = Date.now(); | |
| }; | |
| // Per-task activity timeout: if no activity (data, thinking, tool) for | |
| // this duration, assume the agent is hung and kill it. This catches | |
| // mid-execution hangs that the post-execution stall detector in | |
| // useBatchProcessor cannot detect (it only triggers after task completion). | |
| const ACTIVITY_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes | |
| const activityCheckInterval = setInterval(() => { | |
| const idleMs = Date.now() - lastActivityTime; | |
| if (idleMs > ACTIVITY_TIMEOUT_MS) { | |
| clearInterval(activityCheckInterval); | |
| window.maestro.logger.log('warn', 'Auto Run task killed due to inactivity', 'AgentExecution', { | |
| targetSessionId, | |
| idleMs, | |
| agentType: session.toolType, | |
| }); | |
| // Kill the hung process — onExit listener will fire and resolve the promise | |
| window.maestro.process.kill(targetSessionId).catch((err) => { | |
| captureException(err, { | |
| contexts: { | |
| activityWatchdog: { | |
| targetSessionId, | |
| agentType: session.toolType, | |
| idleMs, | |
| }, | |
| }, | |
| }); | |
| }); | |
| } | |
| }, 30_000); // Check every 30 seconds |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useAgentExecution.ts` around lines 202 - 229, The
watchdog currently swallows errors from window.maestro.process.kill(...) via
.catch(() => {}); update the inactivity check in useAgentExecution (the interval
using ACTIVITY_TIMEOUT_MS and activityCheckInterval) to surface kill failures by
replacing the empty catch with an error reporter: call the app logger at error
level with context (targetSessionId, idleMs, agentType) and invoke the Sentry
utility captureException (imported from src/utils/sentry) with the caught error
and the same context so failures are recorded; do not silently suppress the
exception.
Contributor
Author
CodeRabbit Review — Fix AppliedFinding: Watchdog kill failure silently swallowed ( File: Issue: The inactivity watchdog's Fix (commit // Before
window.maestro.process.kill(targetSessionId).catch(() => {});
// After
window.maestro.process.kill(targetSessionId).catch((err) => {
window.maestro.logger.log('error', 'Failed to kill hung Auto Run process', 'AgentExecution', {
targetSessionId,
agentType: session.toolType,
idleMs,
error: String(err),
});
});Used 🤖 Generated with Claude Code |
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/hooks/agent/useAgentExecution.ts`:
- Around line 220-233: The watchdog kill catch currently only logs and swallows
errors; import Sentry helpers (captureException, captureMessage) from
'src/utils/sentry' and, inside the catch for
window.maestro.process.kill(targetSessionId), call captureException(err, {
extra: { targetSessionId, agentType: session.toolType, idleMs } }) (or
captureMessage for non-throw values) and then rethrow or return
Promise.reject(err) so the failure surfaces; ensure you only swallow
known/recoverable errors by explicitly checking error codes before swallowing
and keep the existing window.maestro.logger.log('error', ...) call for local
logs.
openasocket
force-pushed
the
feat/agent-version-detection
branch
from
February 18, 2026 07:04
9c7147f to
31dc215
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/batch/useBatchProcessor.ts (1)
820-835:⚠️ Potential issue | 🟠 MajorStalled documents can re-run forever in loop mode.
Stalled docs are skipped for the current pass but are retried in later loops, andanyTasksProcessedThisIterationstaystrueeven when no progress is made. If all documents stall andmaxLoopsis unset, Auto Run can loop indefinitely.Suggested fix
// Process each document in order for (let docIndex = 0; docIndex < documents.length; docIndex++) { + const docEntry = documents[docIndex]; + if (stalledDocuments.has(docEntry.filename)) { + continue; + } - const docEntry = documents[docIndex]; @@ } + + if (loopEnabled && stalledDocuments.size === documents.length) { + addFinalLoopSummary('All documents stalled'); + break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 820 - 835, The loop can run forever because stalled documents are retried and still count as progress; change the logic so that stalled documents are permanently skipped across loops (persist stalledDocuments between passes) and ensure skipping a doc due to hitting MAX_CONSECUTIVE_NO_CHANGES does NOT set anyTasksProcessedThisIteration to true. Concretely, update the per-document stall tracking (consecutiveNoChangeCount and stalledDocuments) so once a doc is added to stalledDocuments it is excluded from future iterations, and adjust the main loop condition to break when no non-stalled documents made progress in an iteration (treat stalls as no progress) so the auto-run terminates when all remaining docs are stalled or when maxLoops is reached/unspecified.
🧹 Nitpick comments (1)
src/renderer/App.tsx (1)
5228-5230: Align Codex default thinking across all session creation paths.Right now this sets Codex
showThinkingonly for direct creation. Wizard- and Symphony-created Codex sessions still usedefaultShowThinking(or omit it), so the UX diverges depending on how the agent is created. Consider centralizing this default and reusing it in those paths for consistency.♻️ Suggested refactor (reuse a single helper)
- showThinking: agentId === 'codex' ? 'on' : defaultShowThinking, + showThinking: getInitialShowThinking(agentId, defaultShowThinking),// e.g., near createNewSession helpers const getInitialShowThinking = (agentId: ToolType, fallback: ThinkingMode) => agentId === 'codex' ? 'on' : fallback;Apply the same helper where initial tabs are created in the wizard and symphony flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 5228 - 5230, Centralize the Codex default for showThinking by adding a helper (e.g., getInitialShowThinking(agentId: ToolType, fallback: ThinkingMode)) that returns 'on' when agentId === 'codex' and fallback otherwise, then replace the inline ternary at showThinking (and any other locations that set initial tabs or session options) with calls to this helper; update the createNewSession helpers plus the wizard and symphony session/tab creation code paths to call getInitialShowThinking when initializing showThinking so all session creation flows use the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 464-466: The fallback chain for building the tool output in
codex-output-parser is missing msg.stderr and should prefer
msg.aggregated_output even if empty; update the expression that sets rawOutput
(used by truncateToolOutput) to use nullish coalescing for aggregated_output and
include stderr in the chain (e.g., msg.aggregated_output ?? msg.stderr ??
msg.stdout ?? msg.formatted_output ?? '') so that stderr from the
CodexNewExecCommandEnd payload is returned when other fields are absent; locate
this logic where rawOutput is defined and where truncateToolOutput(rawOutput) is
called.
In `@src/renderer/components/shared/AgentSelector.tsx`:
- Around line 129-133: The JSX block inside the AgentSelector component uses
space indentation for the newly added lines (the <p> element showing {agent.path
?? agent.command} and the conditional {agent.detectedVersion} span); re-indent
those lines and any nested JSX to use tabs (matching the file's existing
tab-based style) so the <p> line, the {agent.detectedVersion && (...)} span, and
their closing tags align with surrounding code using tabs instead of spaces.
---
Outside diff comments:
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 820-835: The loop can run forever because stalled documents are
retried and still count as progress; change the logic so that stalled documents
are permanently skipped across loops (persist stalledDocuments between passes)
and ensure skipping a doc due to hitting MAX_CONSECUTIVE_NO_CHANGES does NOT set
anyTasksProcessedThisIteration to true. Concretely, update the per-document
stall tracking (consecutiveNoChangeCount and stalledDocuments) so once a doc is
added to stalledDocuments it is excluded from future iterations, and adjust the
main loop condition to break when no non-stalled documents made progress in an
iteration (treat stalls as no progress) so the auto-run terminates when all
remaining docs are stalled or when maxLoops is reached/unspecified.
---
Duplicate comments:
In `@src/renderer/hooks/agent/useAgentExecution.ts`:
- Around line 202-233: The watchdog's kill failure currently only logs and
swallows all errors—update the activityCheckInterval handler so the catch on
window.maestro.process.kill(targetSessionId) distinguishes expected benign
errors (e.g., already-exited) and ignores those, but for any unexpected error
call Sentry utilities (import captureException/captureMessage from
src/utils/sentry) to report with context (targetSessionId, idleMs,
session.toolType), and still call window.maestro.logger.log('error', ...) with
the same context; keep clearing the interval and existing log messages around
ACTIVITY_TIMEOUT_MS and lastActivityTime/markActivity unchanged.
---
Nitpick comments:
In `@src/renderer/App.tsx`:
- Around line 5228-5230: Centralize the Codex default for showThinking by adding
a helper (e.g., getInitialShowThinking(agentId: ToolType, fallback:
ThinkingMode)) that returns 'on' when agentId === 'codex' and fallback
otherwise, then replace the inline ternary at showThinking (and any other
locations that set initial tabs or session options) with calls to this helper;
update the createNewSession helpers plus the wizard and symphony session/tab
creation code paths to call getInitialShowThinking when initializing
showThinking so all session creation flows use the same logic.
Comment on lines
+464
to
+466
| // Use aggregated_output or stdout, with truncation | ||
| const rawOutput = msg.aggregated_output || msg.stdout || msg.formatted_output || ''; | ||
| const output = this.truncateToolOutput(rawOutput); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and understand the context around lines 464-466
cat -n src/main/parsers/codex-output-parser.ts | sed -n '450,480p'Repository: RunMaestro/Maestro
Length of output: 1028
🏁 Script executed:
# Let's see the full context of the function containing these lines
cat -n src/main/parsers/codex-output-parser.ts | sed -n '440,500p'Repository: RunMaestro/Maestro
Length of output: 2136
🏁 Script executed:
# Search for where msg object is created and how fields are populated
rg "aggregated_output|exec_command_end" src/main/parsers/ -A 3 -B 3Repository: RunMaestro/Maestro
Length of output: 4437
🏁 Script executed:
# Look for message type definitions or interfaces
rg "interface.*exec_command_end|type.*exec_command_end" src/Repository: RunMaestro/Maestro
Length of output: 1410
🏁 Script executed:
# Search for stderr usage in this file
rg "stderr" src/main/parsers/codex-output-parser.ts -B 2 -A 2Repository: RunMaestro/Maestro
Length of output: 765
Include stderr in the exec_command_end output fallback chain.
The CodexNewExecCommandEnd interface includes a stderr field, but the current code at line 465 omits it from the output fallback. If a command writes only to stderr and aggregated_output, stdout, and formatted_output are all missing or empty, the tool result will be empty instead of including the stderr output.
Proposed fix
- const rawOutput = msg.aggregated_output || msg.stdout || msg.formatted_output || '';
+ const rawOutput =
+ msg.aggregated_output ??
+ (msg.stdout || msg.stderr || msg.formatted_output || '');Using ?? (nullish coalescing) for aggregated_output also ensures that if it is explicitly provided—even as an empty string—it takes precedence; the fallback chain activates only when aggregated_output is undefined or null.
📝 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
| // Use aggregated_output or stdout, with truncation | |
| const rawOutput = msg.aggregated_output || msg.stdout || msg.formatted_output || ''; | |
| const output = this.truncateToolOutput(rawOutput); | |
| // Use aggregated_output or stdout, with truncation | |
| const rawOutput = | |
| msg.aggregated_output ?? | |
| (msg.stdout || msg.stderr || msg.formatted_output || ''); | |
| const output = this.truncateToolOutput(rawOutput); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/parsers/codex-output-parser.ts` around lines 464 - 466, The fallback
chain for building the tool output in codex-output-parser is missing msg.stderr
and should prefer msg.aggregated_output even if empty; update the expression
that sets rawOutput (used by truncateToolOutput) to use nullish coalescing for
aggregated_output and include stderr in the chain (e.g., msg.aggregated_output
?? msg.stderr ?? msg.stdout ?? msg.formatted_output ?? '') so that stderr from
the CodexNewExecCommandEnd payload is returned when other fields are absent;
locate this logic where rawOutput is defined and where
truncateToolOutput(rawOutput) is called.
Comment on lines
+129
to
+133
| <p className="text-xs truncate" style={{ color: theme.colors.textDim }}> | ||
| {agent.path ?? agent.command} | ||
| {agent.detectedVersion && ( | ||
| <span style={{ opacity: 0.7 }}> · v{agent.detectedVersion}</span> | ||
| )} |
There was a problem hiding this comment.
Fix indentation to tabs in this block.
The newly added lines are still indented with spaces; please re-indent using tabs to match repo style.
As per coding guidelines, src/**/*.{ts,tsx,js,jsx}: Use tabs for indentation, not spaces. Always match existing file indentation when editing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/shared/AgentSelector.tsx` around lines 129 - 133, The
JSX block inside the AgentSelector component uses space indentation for the
newly added lines (the <p> element showing {agent.path ?? agent.command} and the
conditional {agent.detectedVersion} span); re-indent those lines and any nested
JSX to use tabs (matching the file's existing tab-based style) so the <p> line,
the {agent.detectedVersion && (...)} span, and their closing tags align with
surrounding code using tabs instead of spaces.
7 tasks
Collaborator
|
@greptile what do you think about this PR? |
|
This is a high-quality infrastructure PR that solves real user pain points with a disciplined, test-driven approach. Here's my assessment: Strengths1. Clear Problem Statement 2. Comprehensive Fix Scope
This shows strong systems thinking—you traced the symptom to its root causes. 3. Backward Compatibility 4. Proactive Observability 5. Test Coverage
This is exemplary. The unit test breakdown (success/failure/timeout/unparseable) shows you thought through failure modes. Concerns / Questions1. Version Detection Timeout
2. Tool Output Truncation
3. Stall Detection Timeout 4. Breaking Change Risk 5. Minor Nits
VerdictMerge with confidence. This PR demonstrates mature engineering:
The 8-bug fix scope shows you didn't just patch the immediate issue—you hardened the entire Codex integration path. The test-driven approach (80 parser tests, 15 version tests) gives me confidence this won't regress. Recommended follow-ups (not blockers):
Ship it. 🚀 |
Greptile SummaryThis PR comprehensively fixes Codex CLI integration end-to-end and adds agent version detection infrastructure across the application. Codex Integration Fixes (8 bugs resolved)
Version Detection Infrastructure
Additional Fixes
Test Coverage
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start([Agent Spawn Request]) --> Detect[Agent Detection]
Detect --> VersionCheck{Has versionArgs?}
VersionCheck -->|Yes| RunVersion[Run --version<br/>3s timeout]
VersionCheck -->|No| Skip[Skip version check]
RunVersion --> ParseVer[Parse semver<br/>Strip v-prefix]
ParseVer --> StoreVer[Store detectedVersion]
Skip --> StoreVer
StoreVer --> CompareVer{Version < minimum?}
CompareVer -->|Yes & not SSH| WarnUser[Emit inline warning<br/>to terminal]
CompareVer -->|No| SpawnProc
WarnUser --> SpawnProc[Spawn Process]
SpawnProc --> InitParser[Initialize Parser]
InitParser --> RefreshConfig[refreshConfig<br/>Read ~/.codex/config.toml]
RefreshConfig --> StreamData[Stream JSONL Output]
StreamData --> ParseLine{Parse JSON Line}
ParseLine --> RouteMsg[routeMessage]
RouteMsg --> DetectFormat{Has msg field?}
DetectFormat -->|Yes| NewFormat[New Format Handler<br/>msg-envelope]
DetectFormat -->|No| OldFormat[Old Format Handler<br/>item-envelope]
NewFormat --> NewEvents[task_started<br/>agent_reasoning<br/>agent_message<br/>exec_command_begin/end<br/>token_count]
OldFormat --> OldEvents[thread.started<br/>turn.started<br/>item.completed<br/>turn.completed]
NewEvents --> Transform[Transform to ParsedEvent]
OldEvents --> Transform
Transform --> EmitEvent{Event Type?}
EmitEvent -->|text & isPartial| ThinkingChunk[Emit thinking-chunk<br/>Mark activity]
EmitEvent -->|tool_use| ToolExec[Emit tool-execution<br/>Mark activity]
EmitEvent -->|result| ResultCheck{toolType === codex?}
ThinkingChunk --> ActivityTimer
ToolExec --> ActivityTimer
ResultCheck -->|Yes| StorePending[Store in codexPendingResult]
ResultCheck -->|No| EmitResult[Emit result immediately]
StorePending --> TurnComplete{Turn completed?}
TurnComplete -->|Yes| EmitFinal[Emit codexPendingResult]
TurnComplete -->|No| Continue[Continue streaming]
EmitResult --> ActivityTimer[Update lastActivityTime]
EmitFinal --> ActivityTimer
Continue --> StreamData
ActivityTimer --> StallCheck{Idle > 5 min?}
StallCheck -->|Yes| KillStalled[Kill stalled agent]
StallCheck -->|No| StreamData
KillStalled --> ExitHandler
ActivityTimer --> ExitHandler[Process Exit]
ExitHandler --> FinalResult{Has result?}
FinalResult -->|No & Codex| UseCodexPending[Use codexPendingResult]
FinalResult -->|No & Other| UseStreamed[Use streamedText]
FinalResult -->|Yes| EmitExit[Emit exit event]
UseCodexPending --> EmitExit
UseStreamed --> EmitExit
EmitExit --> End([Complete])
Last reviewed commit: a222d80 |
Contributor
Author
|
Closing/reopening to refresh GitHub mergeability cache |
openasocket
added a commit
to openasocket/Maestro
that referenced
this pull request
Mar 1, 2026
…all loop, observability - Include stderr in Codex exec_command_end fallback chain so commands that only write to stderr don't produce blank tool results - Fix stalled document re-run loop: skip already-stalled documents at the start of each pass and break when all documents are stalled - Add logger.warn when tool output truncation occurs (10K char limit) - Extract 5-minute inactivity timeout to module-level constant with TODO for settings integration - Document AGENT_MIN_VERSIONS maintenance strategy in JSDoc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/parsers/codex-output-parser.ts (1)
498-500:⚠️ Potential issue | 🟡 MinorUse nullish coalescing for
aggregated_outputprecedence.Using
||treats''as absent and can incorrectly fall back to other fields.aggregated_outputshould remain authoritative when explicitly present but empty.Suggested fix
- const rawOutput = - msg.aggregated_output || msg.stdout || msg.stderr || msg.formatted_output || ''; + const rawOutput = + msg.aggregated_output ?? msg.stdout ?? msg.stderr ?? msg.formatted_output ?? '';#!/bin/bash set -euo pipefail rg -n "const rawOutput" src/main/parsers/codex-output-parser.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/codex-output-parser.ts` around lines 498 - 500, Change the fallback for msg fields to use nullish coalescing so an explicitly present empty aggregated_output is honored: in the block that sets rawOutput (variable rawOutput) replace the current logical-OR chain with a nullish-coalescing chain that checks msg.aggregated_output ?? msg.stdout ?? msg.stderr ?? msg.formatted_output ?? '' before passing into this.truncateToolOutput (method truncateToolOutput) and assigning to output; ensure you only change the operator semantics, leaving variable names rawOutput and output and the call to truncateToolOutput intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/agents/detector.ts`:
- Around line 61-67: The timeout currently aborting version detection doesn't
kill the child because execFileNoThrow is called with the old signature (env
passed as plain object) and ExecOptions lacks an env field; extend the
ExecOptions type to include env: NodeJS.ProcessEnv, update execFileNoThrow and
execFileWithInput to accept an options object and respect both timeout and env,
and modify detector.ts to call execFileNoThrow(binaryPath, versionArgs, { env,
timeout: VERSION_DETECT_TIMEOUT_MS }) so the underlying child process is
terminated on timeout and env vars are applied.
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 361-363: The conditional detecting old-format messages only checks
for parsed.error being a string, so payloads like { "error": { "message": "..."
} } are missed; update the if in codex-output-parser.ts (the branch that calls
this.transformOldFormat(parsed as unknown as CodexOldMessage)) to treat
parsed.error as either a string OR an object with a message property (e.g.,
typeof parsed.error === 'string' || (typeof parsed.error === 'object' &&
parsed.error?.message)), so those nested error objects are routed to
transformOldFormat like true string errors.
---
Duplicate comments:
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 498-500: Change the fallback for msg fields to use nullish
coalescing so an explicitly present empty aggregated_output is honored: in the
block that sets rawOutput (variable rawOutput) replace the current logical-OR
chain with a nullish-coalescing chain that checks msg.aggregated_output ??
msg.stdout ?? msg.stderr ?? msg.formatted_output ?? '' before passing into
this.truncateToolOutput (method truncateToolOutput) and assigning to output;
ensure you only change the operator semantics, leaving variable names rawOutput
and output and the call to truncateToolOutput intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.prettierignorepackage.jsonsrc/__tests__/main/parsers/codex-output-parser.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/renderer/components/DocumentGraph/mindMapLayouts.test.tssrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/components/auto-scroll.test.tsxsrc/__tests__/renderer/hooks/useBatchProcessor.test.tssrc/main/agents/definitions.tssrc/main/agents/detector.tssrc/main/index.tssrc/main/ipc/handlers/process.tssrc/main/parsers/codex-output-parser.tssrc/main/process-manager/handlers/ExitHandler.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.ts
✅ Files skipped from review due to trivial changes (5)
- src/tests/renderer/components/TerminalOutput.test.tsx
- .prettierignore
- src/tests/renderer/components/auto-scroll.test.tsx
- src/tests/renderer/hooks/useBatchProcessor.test.ts
- src/tests/renderer/components/DocumentGraph/mindMapLayouts.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/process-manager/spawners/ChildProcessSpawner.ts
- src/main/ipc/handlers/process.ts
Comment on lines
+61
to
+67
| const resultPromise = execFileNoThrow(binaryPath, versionArgs, undefined, env); | ||
| const timeoutPromise = new Promise<null>((resolve) => | ||
| setTimeout(() => resolve(null), VERSION_DETECT_TIMEOUT_MS) | ||
| ); | ||
|
|
||
| const result = await Promise.race([resultPromise, timeoutPromise]); | ||
| if (!result) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspecting detectAgentVersion timeout behavior ==="
rg -n "detectAgentVersion|Promise\.race|VERSION_DETECT_TIMEOUT_MS|execFileNoThrow" src/main/agents/detector.ts -C4
echo ""
echo "=== Full context of timeout handling section ==="
sed -n '54,104p' src/main/agents/detector.tsRepository: RunMaestro/Maestro
Length of output: 3855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspecting execFileNoThrow implementation ==="
rg -n "function execFileNoThrow|export.*execFileNoThrow|interface ExecOptions|type ExecOptions" src/main/utils/execFile.ts -C4
echo ""
echo "=== Full execFileNoThrow function implementation ==="
rg -n "execFileNoThrow" src/main/utils/execFile.ts -A30Repository: RunMaestro/Maestro
Length of output: 1886
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking for AbortController/AbortSignal usage in execFile ==="
rg -n "AbortController|AbortSignal|signal|timeout" src/main/utils/execFile.ts
echo ""
echo "=== Checking for process termination patterns in detector ==="
rg -n "kill|abort|cleanup|unref" src/main/agents/detector.tsRepository: RunMaestro/Maestro
Length of output: 901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full execFileNoThrow implementation with timeout handling ==="
sed -n '86,155p' src/main/utils/execFile.ts
echo ""
echo "=== Check other execFileNoThrow calls in detector.ts ==="
rg -n "execFileNoThrow" src/main/agents/detector.ts -B2 -A2Repository: RunMaestro/Maestro
Length of output: 3003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify if env is being merged into ExecOptions elsewhere ==="
rg -n "ExecOptions|{ env" src/main/agents/detector.ts -B2 -A2
echo ""
echo "=== Check how execFileWithInput handles both timeout and env ==="
sed -n '160,210p' src/main/utils/execFile.tsRepository: RunMaestro/Maestro
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full ExecOptions interface definition ==="
sed -n '7,15p' src/main/utils/execFile.ts
echo ""
echo "=== Check if ExecOptions includes env property ==="
rg -n "interface ExecOptions" -A10 src/main/utils/execFile.tsRepository: RunMaestro/Maestro
Length of output: 820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify execFileWithInput function signature ==="
sed -n '160,175p' src/main/utils/execFile.ts
echo ""
echo "=== Check how other callers pass env with timeout ==="
rg -n "execFileNoThrow" src/ -B1 -A1 | head -50Repository: RunMaestro/Maestro
Length of output: 3317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if ExecOptions needs env property added ==="
rg -n "env.*ExecOptions|ExecOptions.*env" src/
echo ""
echo "=== Verify execFileWithInput does NOT accept env ==="
sed -n '160,210p' src/main/utils/execFile.ts
echo ""
echo "=== Check actual Node.js execFile behavior with env ==="
sed -n '114,130p' src/main/utils/execFile.tsRepository: RunMaestro/Maestro
Length of output: 1996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify spawn call in execFileWithInput lacks env ==="
sed -n '160,180p' src/main/utils/execFile.ts
echo ""
echo "=== Check what properties spawn accepts for env ==="
sed -n '172,178p' src/main/utils/execFile.ts
echo ""
echo "=== Confirm detector.ts line 344 also passes env without timeout ==="
sed -n '340,350p' src/main/agents/detector.tsRepository: RunMaestro/Maestro
Length of output: 1292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Summary: ExecOptions interface definition ==="
cat src/main/utils/execFile.ts | sed -n '7,11p'
echo ""
echo "=== How detector.ts passes env (backward compat path) ==="
sed -n '98,107p' src/main/utils/execFile.ts
echo ""
echo "=== Conclusion: Can detector.ts pass both env AND timeout? ==="
echo "ExecOptions has only: input, timeout"
echo "No env property in ExecOptions interface"
echo "Calling with env object goes to old signature path (line 105)"
echo "Old path does NOT extract timeout (lines 98-107)"Repository: RunMaestro/Maestro
Length of output: 904
Version probe timeout does not terminate the child process.
Promise.race returns after 3s, but the underlying execFileNoThrow call continues running. The timeout is never passed to the exec helper—detector.ts passes env as a plain object (old signature), so the timeout option in ExecOptions is ignored. Additionally, ExecOptions lacks an env property, making it impossible to pass both environment variables and timeout simultaneously with the current API design.
Processes can accumulate across detections when --version commands hang. Extend ExecOptions to include env: NodeJS.ProcessEnv, update detector.ts to pass { env, timeout: VERSION_DETECT_TIMEOUT_MS }, and ensure execFileWithInput also applies env variables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/agents/detector.ts` around lines 61 - 67, The timeout currently
aborting version detection doesn't kill the child because execFileNoThrow is
called with the old signature (env passed as plain object) and ExecOptions lacks
an env field; extend the ExecOptions type to include env: NodeJS.ProcessEnv,
update execFileNoThrow and execFileWithInput to accept an options object and
respect both timeout and env, and modify detector.ts to call
execFileNoThrow(binaryPath, versionArgs, { env, timeout:
VERSION_DETECT_TIMEOUT_MS }) so the underlying child process is terminated on
timeout and env vars are applied.
Comment on lines
+361
to
+363
| // Old format detection: top-level `type` field or standalone `error` field | ||
| if (typeof parsed.type === 'string' || typeof parsed.error === 'string') { | ||
| return this.transformOldFormat(parsed as unknown as CodexOldMessage); |
There was a problem hiding this comment.
Top-level object error payloads are not routed to old-format handling.
A line like { "error": { "message": "..." } } currently falls through as system instead of becoming an error event.
Suggested fix
- if (typeof parsed.type === 'string' || typeof parsed.error === 'string') {
+ if (
+ typeof parsed.type === 'string' ||
+ typeof parsed.error === 'string' ||
+ (typeof parsed.error === 'object' && parsed.error !== null)
+ ) {
return this.transformOldFormat(parsed as unknown as CodexOldMessage);
}📝 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
| // Old format detection: top-level `type` field or standalone `error` field | |
| if (typeof parsed.type === 'string' || typeof parsed.error === 'string') { | |
| return this.transformOldFormat(parsed as unknown as CodexOldMessage); | |
| // Old format detection: top-level `type` field or standalone `error` field | |
| if ( | |
| typeof parsed.type === 'string' || | |
| typeof parsed.error === 'string' || | |
| (typeof parsed.error === 'object' && parsed.error !== null) | |
| ) { | |
| return this.transformOldFormat(parsed as unknown as CodexOldMessage); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/parsers/codex-output-parser.ts` around lines 361 - 363, The
conditional detecting old-format messages only checks for parsed.error being a
string, so payloads like { "error": { "message": "..." } } are missed; update
the if in codex-output-parser.ts (the branch that calls
this.transformOldFormat(parsed as unknown as CodexOldMessage)) to treat
parsed.error as either a string OR an object with a message property (e.g.,
typeof parsed.error === 'string' || (typeof parsed.error === 'object' &&
parsed.error?.message)), so those nested error objects are routed to
transformOldFormat like true string errors.
Contributor
Author
Review Fixes — Complete SummaryAll CodeRabbit actionable items (3) and Greptile concerns (5) have been addressed. Branch is now fully merged with CodeRabbit Fixes
Greptile Concerns
Merge & CI Fixes
Verification
|
Introduce maestro:// URL scheme support for navigating to agents, tabs, and groups from external apps and OS notification clicks. - Add deep-links module with URL parsing, protocol registration, single-instance locking, and cross-platform event handling - Wire notification click handlers to navigate to the originating agent/tab via deep link dispatch - Thread sessionId/tabId context through notification preload bridge - Add onDeepLink listener in renderer with routing to existing navigation handlers - Register maestro:// protocol in electron-builder config - Add 18 tests covering URL parsing and notification click wiring
- URI-encode sessionId/tabId when constructing deep link URLs in notification click handler to prevent malformed URLs with special chars - Add process.exit(0) after app.quit() so secondary instances exit immediately without running further module-level setup - Use useRef for sessions in deep link effect to avoid tearing down and re-registering the IPC listener on every sessions change - Guard against navigating to non-existent session IDs in deep link handler to prevent invalid UI state - Add cross-reference comment in global.d.ts linking to canonical ParsedDeepLink type (can't import in ambient declaration file) - Add test for URI-encoding round-trip in notification click handler
- Add shared deep-link-urls.ts with buildSessionDeepLink(),
buildGroupDeepLink(), and buildFocusDeepLink() utilities
- Add {{AGENT_DEEP_LINK}}, {{TAB_DEEP_LINK}}, {{GROUP_DEEP_LINK}}
template variables available in system prompts, custom AI commands,
and Auto Run documents
- Wire activeTabId and groupId into TemplateContext at all call sites
(agentStore, useInputProcessing, useRemoteHandlers,
useDocumentProcessor, useMergeTransferHandlers, batch-processor)
- Refactor notifications.ts to use shared buildSessionDeepLink()
- Add sessionId/tabId to notifyToast callers where context is available
(merge, transfer, summarize, PR creation)
- Add docs/deep-links.md documentation page with URL format, usage
examples, template variables, and platform behavior
- Add 8 tests for URL builders, 6 tests for template variable
substitution including URI encoding
…e activity indicators Broadcast new history entries via IPC when they are added, subscribe in the UnifiedHistoryTab with RAF batching and deduplication, and extend the HistoryStatsBar with spinning Active agent count and Queued message count indicators derived from the Zustand session store.
…orrect types - Replace unstable sessionNameMap Zustand selector (new Map per render) with a stable ref + subscribe pattern to avoid streaming effect re-subscription - Dedupe within batch before merging; compute setTotalEntries and setHistoryStats from deduplicated entries only (not raw batch) - Clear pendingEntriesRef on cleanup to prevent stale replay after resubscribe - Use HistoryEntry (not UnifiedHistoryEntry) in preload callback type since the wire payload lacks sourceSessionId - Use canonical UsageStats interface in global.d.ts (fixes pre-existing cacheReadTokens/cacheWriteTokens field name mismatch)
…URL registry support - Add `symphony: boolean` (default true) to EncoreFeatureFlags - Gate Symphony modal, menu item, keyboard shortcut (⇧⌘Y), and command palette entry - Add `symphonyRegistryUrls` setting for user-configured additional registry URLs - Replace single `fetchRegistry()` with `fetchRegistries()` that fetches default + custom URLs in parallel - Merge repositories by slug (default registry wins on conflicts), isolated per-URL error handling - Add Symphony toggle + Registry Sources UI in Settings > Encore tab - Update tests for new symphony flag across all encore feature assertions
- Redact registry URLs before logging to prevent credential leakage - Skip registry cache when custom source URLs are configured (stale cache fix) - Runtime-validate symphonyRegistryUrls from settings store - Reset modal-open flags when Encore Feature toggles are disabled - Normalize registry URLs before duplicate/default checks - Add aria-label to icon-only registry URL remove button - Expose setSymphonyRegistryUrls in getSettingsActions() - Validate persisted symphonyRegistryUrls with Array.isArray guard
…r, and Encore feature flag - Register maestroCue as an Encore Feature flag (EncoreFeatureFlags, DEFAULT_ENCORE_FEATURES) - Create src/main/cue/cue-types.ts with CueEventType, CueSubscription, CueSettings, CueConfig, CueEvent, CueRunStatus, CueRunResult, CueSessionStatus, and related constants - Add 'CUE' to HistoryEntryType across shared types, global.d.ts, preload, IPC handlers, and hooks - Add cueTriggerName, cueEventType, cueSourceSession optional fields to HistoryEntry - Add 'cue' log level to MainLogLevel, LOG_LEVEL_PRIORITY, logger switch/case, and LogViewer with teal color (#06b6d4), always-enabled filter, and agent name pill - Add 10 Cue-specific template variables (CUE_EVENT_TYPE, CUE_TRIGGER_NAME, etc.) with cueOnly flag - Extend TemplateContext with cue? field and substituteTemplateVariables with Cue replacements - Update TEMPLATE_VARIABLES_GENERAL filter to exclude cueOnly variables
Session rows and process rows now show a hover-visible ExternalLink button that navigates to the agent (session) or specific tab and closes the modal. Group chat processes get a similar button that navigates to the group chat.
Show a confirmation modal when closing AI tabs that have draft text or staged images. Applies to single tab close (Cmd+W, click X), and bulk operations (close all, close other, close left, close right).
… debug logs - Add codexPendingResult field to ManagedProcess to separate result capture from reasoning text accumulation in streamedText - Update StdoutHandler to store agent_message results in codexPendingResult and flush from there at turn.completed - Update ExitHandler fallback to prefer codexPendingResult over streamedText for Codex processes - Add turn.started reset to clear resultEmitted, codexPendingResult, and streamedText for multi-turn session support - Set supportsResultMessages: true for Codex in capabilities.ts - Remove 4 debug console.log statements from StdoutHandler.ts and GroupChat debug block from ExitHandler.ts - Update existing test to verify codexPendingResult usage and add new test ensuring reasoning text is never emitted as result Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…to 'on' for Codex agents, add length bypass for block-style reasoning - Override showThinking default to 'on' for Codex agents across 7 tab creation sites in App.tsx (worktree tabs, new tabs, new sessions, error recovery) since reasoning is the primary visibility into Codex agent activity - Add length-based bypass (<=500 chars) for isLikelyConcatenatedToolNames checks in useAgentListeners.ts — Codex emits reasoning as complete blocks, not incremental streams, so large content should not be misidentified as malformed tool names - Verified: isResultMessage correctly excludes reasoning events (type 'text' vs 'result'), no thinking/result duplication occurs - Verified: TerminalOutput renders thinking with accent border + MarkdownRenderer, correctly displaying formatReasoningText's **section** markers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re logging and completion safety checks - Add logger.info at result capture point in useDocumentProcessor with agent type, response length, and document name for debugging Codex Auto Run flows - Add safety warning log when agent exits successfully but captures no result text (prevents silent failures with Codex) - Improve synopsis extraction for short responses: use entire cleaned text as summary source for responses < 500 chars that don't follow structured **Summary:**/**Details:** format (agent-agnostic, benefits Codex's concise response style) - Document absence of per-task timeout in useBatchProcessor with note about Promise.race() as future enhancement Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s events, truncate large outputs, add Codex arg extraction - Add lastToolName tracking to carry tool name from tool_call to tool_result events (Codex emits these as separate item.completed events, losing the name on tool_result) - Truncate tool output to 10K chars to prevent UI performance degradation from large outputs - Add Codex-specific tool input extraction (path, cmd, code, content) in TerminalOutput - Add 10 new tests covering tool name carryover and output truncation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… selection via -m flag, fix singleton config staleness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…add activity tracking from thinking/tool events, guard responseText, add task completion logging Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ing leak, singleton state leak, dead stall detection - ExitHandler fallback now uses toolType-aware branching: Codex only emits codexPendingResult (never reasoning from streamedText), other agents keep streamedText fallback. Fixes reasoning text appearing as user-visible result on Codex process exit without agent_message. - ExitHandler JSON buffer path adds codexPendingResult to fallback chain (event.text → codexPendingResult → streamedText) for consistency. - CodexOutputParser.refreshConfig() now resets lastToolName to prevent tool name leakage across sessions (parser is a singleton). - Wire up lastActivityTime as actual stall detector: 5-minute inactivity timeout kills hung Auto Run agent processes mid-execution via setInterval check. Interval cleared on normal exit via cleanupFns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-detection in parser
Support both old (item-envelope, ≤v0.x) and new (msg-envelope, v0.41.0+) Codex CLI
output formats with automatic detection. New format uses { id, msg: { type, ... } }
envelope with exec_command_begin/end, agent_reasoning, agent_message, token_count
events. Tool name carryover uses call_id mapping for concurrent commands.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… field, detectAgentVersion() helper, AGENT_MIN_VERSIONS constants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…orted version Emits an inline [Maestro] warning in the terminal when an agent's detected version is below AGENT_MIN_VERSIONS. Skipped for SSH remote sessions where local version is irrelevant. Also logs agentVersion in spawn debug metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vering all detectAgentVersion() branches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tCard Add detectedVersion field to renderer AgentConfig types (global.d.ts and types/index.ts) and display it on 3 UI surfaces: - Edit Agent Modal: version shown inline after agent provider name - Wizard agent tiles: version shown below description for detected agents - AgentCard (shared): version appended after path with dot separator All version display is conditional — agents without detectedVersion render identically to before. No layout shifts, no new IPC calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…5 agents, fix v-prefix parsing - Fix detectAgentVersion() to strip leading v/V prefix before matching version tokens (e.g., "Gemini CLI v0.29.0" → "0.29.0", "Factory Droid V0.4.0" → "0.4.0") - Add versionArgs: ['--version'] to Gemini CLI, Qwen3 Coder, OpenCode, Factory Droid, and Aider - Add 4 new tests: v-prefix, uppercase V, path-prefixed output, and aider format - No changes to AGENT_MIN_VERSIONS (no enforcement for newly-probed agents) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Log error details (targetSessionId, agentType, idleMs) when the
inactivity watchdog fails to kill a hung Auto Run process, instead
of silently ignoring the failure with .catch(() => {}).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…se chat lock on moderator failure - agent-args.ts: Only filter --dangerously-bypass-approvals-and-sandbox from batchModeArgs in readOnly mode, preserving --skip-git-repo-check which Codex requires even in read-only sandbox mode - exit-listener.ts: Release chat lock when moderator exits with no output, empty parsed output, or chat load failure — prevents chats from becoming permanently locked and undeletable - Updated agent-args tests for new selective filtering behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…all loop, observability - Include stderr in Codex exec_command_end fallback chain so commands that only write to stderr don't produce blank tool results - Fix stalled document re-run loop: skip already-stalled documents at the start of each pass and break when all documents are stalled - Add logger.warn when tool output truncation occurs (10K char limit) - Extract 5-minute inactivity timeout to module-level constant with TODO for settings integration - Document AGENT_MIN_VERSIONS maintenance strategy in JSDoc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…essor Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add groupChatLock mock to exit-listener tests (required after chat lock feature merge) - Add reasoning/turn.started cases to StdoutHandler codex mock's parseJsonObject - Update agent-args tests to match new read-only batchModeArgs filtering behavior (only sandbox-bypass flags are filtered, safe args like --skip-git-repo-check preserved) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openasocket
force-pushed
the
feat/agent-version-detection
branch
from
March 11, 2026 03:43
1bdd525 to
cda144d
Compare
Contributor
Author
|
Retargeted from |
Contributor
|
@openasocket please run the following on your machine for this PR so it can move toward acceptance. Here is the prompt you should run: |
Collaborator
|
Hey! We're consolidating onto the |
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
5 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
Motivation
Maestro's Codex parser was originally built against Codex CLI v0.41.0, which used a flat item-envelope JSONL format (
{ type: 'item.completed', item: {...} }). Subsequent Codex CLI releases (current: v0.103.0) changed to a nested msg-envelope format ({ id, msg: { type: 'agent_message', ... } }). Users who upgraded Codex CLI got zero parsed output with no explanation because Maestro's parser only understood the old format.Beyond the format mismatch, the initial Codex integration had several bugs that caused:
tool_callandtool_resultevents (Codex emits these as separate events)streamedTextclobbered by reasoning, breaking result extractionshowThinkingdefaulting to'off'for Codex, hiding the primary visibility into agent activityThis PR fixes all of the above and adds version detection so Maestro can warn users before they hit these silent failures.
Changes
Codex Integration Fixes (8 commits)
0cf3fc4fcodexPendingResultfromstreamedTextto prevent reasoning text from clobbering the actual result3f1f3ca8showThinkingto'on'for Codex across 7 tab creation sites; add length bypass for block-style reasoninge07454d3d1c83270tool_call→tool_resultevents; truncate large tool outputs (10K limit)d23ece56modelArgsfor Codex (-mflag); fix parser singleton config staleness withrefreshConfig()6079a161b51f7e5clastToolNamereset), dead stall detection4857e93erouteMessage(), support both with format-specific transformersAgent Version Detection (5 commits)
33c7e957detectedVersionfield toAgentConfig, genericdetectAgentVersion()helper (3s timeout, semver scan),AGENT_MIN_VERSIONSconstanta4c8cbc6[Maestro] Warningin terminal when spawning an agent below minimum version; skip for SSH remote sessions41bdb4d9detectAgentVersion()branches (success, failure, timeout, unparseable, empty, unavailable, no versionArgs) + 3 integration tests for version comparisonbe33df8fdetectedVersionin Edit Agent Modal, Wizard agent tiles, and shared AgentCard componenta827f80av-prefix parsing (v0.29.0→0.29.0)Key Technical Details
Dual-Format Codex Parser
The parser auto-detects the format by inspecting each JSON line:
parsed.msgexists → new msg-envelope format (current versions)parsed.typeis string → old item-envelope format (v0.41.0-era)parsed.model/parsed.prompt→ config metadata lineNew format event mapping:
task_startedtype: 'system'withraw.type: 'turn.started'(triggers StdoutHandler turn reset)agent_reasoningtype: 'text'(thinking/reasoning display)agent_messagetype: 'result'(final answer)exec_command_begintype: 'tool_use'with tool name frommsg.message.nameexec_command_endtype: 'tool_use'with output frommsg.output, tool name fromcall_idmaptoken_counttype: 'usage'with token countsVersion Detection
<binary> --versionduring agent detection (non-blocking, 3s timeout)/^\d+\.\d+/)v/Vprefix for formats likeGemini CLI v0.29.00.41.0(baseline tested format), Claude Code1.0.0(stream-json support)versionArgs: ['--version']Files Changed (26 files, +1731 / -206)
Main Process
src/main/parsers/codex-output-parser.ts— Dual-format parser rewrite (+534 lines)src/main/agents/detector.ts—detectAgentVersion(),AGENT_MIN_VERSIONSsrc/main/agents/definitions.ts—detectedVersion,versionArgson all agentssrc/main/ipc/handlers/process.ts— Spawn-time version warningsrc/main/process-manager/handlers/StdoutHandler.ts—codexPendingResult, turn resetsrc/main/process-manager/handlers/ExitHandler.ts— toolType-aware result extractionRenderer
src/renderer/App.tsx—showThinking: 'on'defaults for Codexsrc/renderer/components/NewInstanceModal.tsx— Version in edit modalsrc/renderer/components/Wizard/screens/AgentSelectionScreen.tsx— Version on wizard tilessrc/renderer/components/shared/AgentSelector.tsx— Version in AgentCardsrc/renderer/components/TerminalOutput.tsx— Codex tool input extractionsrc/renderer/hooks/agent/useAgentExecution.ts— Codex showThinking defaultssrc/renderer/hooks/batch/useBatchProcessor.ts— Stall detection timersrc/renderer/hooks/batch/useDocumentProcessor.ts— Synopsis extraction fixTests
src/__tests__/main/parsers/codex-output-parser.test.ts— 80 tests (rewritten)src/__tests__/main/agents/detector.test.ts— 15 new version detection testsTest plan
npm run lint— all TypeScript configs passnpm run test— 19,759 tests pass (464 test files)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements