Add launch orchestrator and docker auth support - #54
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a JSON-enabled launch/startup gate and docker-login CLI commands, implements Doctor and Launch modules for multi-step readiness gating, extends ServiceManager with Docker metadata, introduces CliGateError and new error codes, updates docs to replace doctor-based flows with launch-based gating, and adds unit and e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as codex-synaptic CLI
participant Daemon as Background Daemon
participant MCP as MCP Services
participant Codex as Codex Registry
participant Doctor
User->>CLI: codex-synaptic launch --json --strict
CLI->>CLI: verify dist/cli/index.js exists & executable
CLI->>Codex: (optional) check codex auth
CLI->>Daemon: start background daemon
Daemon-->>CLI: background started
CLI->>MCP: ensure services up (ensureService / health-wait)
MCP->>CLI: services healthy / status
CLI->>Codex: register MCP profiles (codex mcp add)
Codex-->>CLI: registration results
CLI->>Doctor: run aggregated health checks
Doctor-->>CLI: DoctorReport {ok, summary, checks}
CLI-->>User: LaunchReport {ok: true|false, nextAction, remediations}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
Comment |
This comment was marked as resolved.
This comment was marked as resolved.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
coderabbitai
Bot
added
codex
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/cli/launch.ts`:
- Around line 59-68: normalizeSpawn currently returns a synchronous wrapper that
may call spawnSync, which blocks the event loop during async runLaunch preflight
checks; change normalizeSpawn to be async and return an async spawn wrapper
(e.g., async (command, args, options) => { /* await deps.spawnCommand(...) or
run child process without blocking */ }) so it always returns a Promise of the
result, replace direct uses of spawnSync in normalizeSpawn with awaiting
deps.spawnCommand (or a non-blocking child process helper), and update all call
sites (the calls inside runLaunch that currently expect synchronous returns at
the checks for node --help, codex login status, and MCP registration) to await
the new async spawn function and handle its resolved
Pick<SpawnSyncReturns<string>, 'status'|'stdout'|'stderr'>-shaped result.
🧹 Nitpick comments (1)
src/cli/launch.ts (1)
28-33: Prefer an enum fornextActionto align with semantic-constant guidance.Using a string union here drifts from the enum-only convention and makes reuse across modules harder. Consider introducing a small enum and using it in the report builder.
♻️ Suggested enum for launch next action
+export enum LaunchNextAction { + Continue = 'continue', + Stop = 'stop' +} + export interface LaunchReport { ok: boolean; steps: LaunchStep[]; doctor: DoctorReport; - nextAction: 'continue' | 'stop'; + nextAction: LaunchNextAction; } ... - nextAction: ok ? 'continue' : 'stop' + nextAction: ok ? LaunchNextAction.Continue : LaunchNextAction.StopAs per coding guidelines: "Use enums for constants with semantic meaning".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/index.ts (1)
85-91:⚠️ Potential issue | 🔴 CriticalFix broken import block to resolve the TS1128 build failure.
The hive-mind helper identifiers are not preceded by an
import {statement, leaving a dangling block that triggers the TS1128 error during build. Add the missing import declaration.Proposed fix
import { collectLaunchRemediations, runLaunch } from './launch.js'; - executeGoapWorkflow, - executeTaskWithConsensus, - collectExecutionResults, - renderExecutionSummary, - setupWorkflowEventHandlers -} from './hive-mind-helpers.js'; +import { + executeGoapWorkflow, + executeTaskWithConsensus, + collectExecutionResults, + renderExecutionSummary, + setupWorkflowEventHandlers +} from './hive-mind-helpers.js';
🧹 Nitpick comments (2)
src/env/service-manager.ts (1)
331-339: Consider usingServiceManagerErrorinstead of genericError.For consistency with the module's error handling pattern, the validation error should use the specialized error class defined at the top of this file.
♻️ Suggested improvement
dockerLogin(registry: string): void { const normalized = registry.trim(); if (!normalized) { - throw new Error('Docker registry is required for docker login.'); + throw new ServiceManagerError('Docker registry is required for docker login.'); } const cmd = `docker login ${normalized}`; this.logger.info('env', 'Authenticating Docker registry', { registry: normalized }); execSync(cmd, { stdio: 'inherit' }); }As per coding guidelines: "Throw specific error types, not generic Error objects".
src/cli/launch.ts (1)
121-398: Consider extracting step handlers to reduce function complexity.The
runLaunchfunction spans ~280 lines with multiple distinct phases. While the structure is logical, extracting each step (preflight, auth, daemon, MCP up, codex register, doctor) into separate helper functions would improve readability and make individual steps easier to test in isolation.This is flagged by static analysis as a complex method. A potential refactor pattern:
// Example extraction pattern async function runPreflightStep(cwd: string, fileExists: Function, spawnCommand: Function): Promise<LaunchStep> { // ... preflight logic } async function runCodexAuthStep(options: LaunchOptions, spawnCommand: Function, cwd: string): Promise<LaunchStep> { // ... auth logic } // etc.
|
@copilot , implement fix for below observation: In
|
- Update DoctorDependencies.spawnCommand to return Promise - Update LaunchDependencies (extends DoctorDependencies) - Modify normalizeSpawn to return async wrapper function - Await all spawnCommand calls in launch.ts (preflight, auth, mcp registration) - Await all spawnCommand calls in doctor.ts (cli help, login status, mcp list) - Update launch.test.ts with async spawnCommand mocks - Update doctor.test.ts with async spawnCommand mocks - All tests pass (9/9) Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Replace wrapped spawnSync with proper async spawn implementation: - Add spawnAsync helper using child_process.spawn with Promise - Update doctor.ts to use spawnAsync by default - Update launch.ts to use spawnAsync by default - Prevents actual event loop blocking (not just wrapping sync in async) - All tests still pass (9/9) Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Document that spawnAsync always resolves (never rejects) to match spawnSync API: - Errors communicated via status code and stderr - Matches expected behavior of calling code - Prevents unnecessary try/catch blocks in callers - All tests still pass (9/9) Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Make normalizeSpawn truly async to prevent event loop blocking
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cli/doctor.ts`:
- Line 1: Remove the unused spawnSync import and the associated type
SpawnSyncReturns from the top-level child_process import; update the import line
that currently imports "spawn, spawnSync, type SpawnSyncReturns" to only import
"spawn" (and any other actually used symbols) so there are no unused imports
left (look for the import statement that mentions spawnSync and SpawnSyncReturns
in this module).
- Around line 94-130: The parseCodexMcpNames function currently throws a generic
Error for unsupported payloads; replace that with the project's specific error
type by importing BridgeError and ErrorCode and throw new
BridgeError(ErrorCode.MCP_ERROR, 'Unsupported JSON format returned by `codex mcp
list --json`.') (or, if preferred, create and throw a new Codex-specific error
class that extends CodexSynapticError) so MCP parsing failures are classified
consistently with the error handling used in src/cli/launch.ts.
- Around line 33-38: In spawnAsync (the child process handling code with
child.on('close')), change the close handler to accept both (code, signal), set
the resolved status to code ?? 1 (so signal-terminated children are treated as
failures), and include the signal value in the resolved object (e.g., add a
signal property alongside stdout and stderr) to provide better error context;
update any callers or types if necessary to account for the new signal field.
In `@src/cli/launch.ts`:
- Line 1: The import list in the module still includes the unused symbol
spawnSync from 'child_process' even though only the async spawn wrapper is used;
remove spawnSync from the import statement so it only imports spawn (and type
SpawnSyncReturns if still used) or drop the unused type import entirely to
eliminate the dead import.
- Around line 47-53: The close handler treats signal-terminated processes as
success by using code ?? 0; change the listener signature to (code, signal) and
set the returned status to a non-zero value when code is null (e.g., status =
code !== null ? code : 1) and include the signal name (signal) in the stderr or
add it to the resolved object so callers can see the termination reason; update
the child.on('close', ...) block that currently resolves { status: code ?? 0,
stdout, stderr } accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cli/doctor.tssrc/cli/launch.tstests/cli/doctor.test.tstests/cli/launch.test.ts
| @@ -0,0 +1,304 @@ | |||
| import { spawn, spawnSync, type SpawnSyncReturns } from 'child_process'; | |||
There was a problem hiding this comment.
Drop the unused spawnSync import.
It’s never referenced in this file.
🧹 Suggested fix
-import { spawn, spawnSync, type SpawnSyncReturns } from 'child_process';
+import { spawn, type SpawnSyncReturns } from 'child_process';📝 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.
| import { spawn, spawnSync, type SpawnSyncReturns } from 'child_process'; | |
| import { spawn, type SpawnSyncReturns } from 'child_process'; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 1-1: src/cli/doctor.ts#L1
'spawnSync' is defined but never used. (@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/doctor.ts` at line 1, Remove the unused spawnSync import and the
associated type SpawnSyncReturns from the top-level child_process import; update
the import line that currently imports "spawn, spawnSync, type SpawnSyncReturns"
to only import "spawn" (and any other actually used symbols) so there are no
unused imports left (look for the import statement that mentions spawnSync and
SpawnSyncReturns in this module).
| child.on('close', (code) => { | ||
| resolve({ | ||
| status: code ?? 0, | ||
| stdout, | ||
| stderr | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual code in the file
cat -n src/cli/doctor.ts | head -50Repository: clduab11/codex-synaptic
Length of output: 1753
🌐 Web query:
Node.js child_process close event code null signal termination behavior
💡 Result:
In Node.js child_process, the 'close' event callback receives (code, signal):
codeis the numeric exit code only if the child exited normally; otherwise it’snull.signalis the signal name (e.g.,'SIGTERM','SIGKILL') only if the child was terminated by a signal; otherwise it’snull.- Exactly one of
codeorsignalwill be non-null. If you seecode === null, interpret that as “the process did not exit normally; it was ended by a signal” and look atsignalto know which one. [1]
'close' vs 'exit':
'exit'fires when the process ends, but its stdio streams may still be open.'close'fires after the process ends and all stdio streams for that child are closed, and it will always happen after'exit'(or after'error'if the child failed to spawn). [1]
Source: Node.js child_process docs ('close' / 'exit' events). [1]
References
[1] Node.js v22.19.0 Documentation – child_process (ChildProcess events 'close' and 'exit'). (nodejs.org)
Citations:
🏁 Script executed:
# Check if spawnSync is used elsewhere in the file
rg "spawnSync" src/cli/doctor.ts -A 2 -B 2Repository: clduab11/codex-synaptic
Length of output: 466
🏁 Script executed:
# Check the entire file for any actual use of spawnSync (not just import/comment)
wc -l src/cli/doctor.ts && echo "---" && grep -n "spawnSync" src/cli/doctor.tsRepository: clduab11/codex-synaptic
Length of output: 253
Treat signal-terminated child processes as failures in spawnAsync.
When a child process is killed by signal, Node.js sets code to null and populates signal instead. The current code ?? 0 fallback incorrectly reports success for these signal terminations. Update the fallback to code ?? 1 and capture the signal parameter for better error context.
🔧 Suggested fix
- child.on('close', (code) => {
+ child.on('close', (code, signal) => {
resolve({
- status: code ?? 0,
+ status: code ?? 1,
stdout,
stderr
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/doctor.ts` around lines 33 - 38, In spawnAsync (the child process
handling code with child.on('close')), change the close handler to accept both
(code, signal), set the resolved status to code ?? 1 (so signal-terminated
children are treated as failures), and include the signal value in the resolved
object (e.g., add a signal property alongside stdout and stderr) to provide
better error context; update any callers or types if necessary to account for
the new signal field.
| function parseCodexMcpNames(payload: unknown): string[] { | ||
| if (Array.isArray(payload)) { | ||
| return payload | ||
| .map((entry) => { | ||
| if (!entry || typeof entry !== 'object') { | ||
| return undefined; | ||
| } | ||
| return String((entry as { name?: string }).name ?? ''); | ||
| }) | ||
| .filter(Boolean) as string[]; | ||
| } | ||
|
|
||
| if (payload && typeof payload === 'object') { | ||
| const candidateArrays = [ | ||
| (payload as { servers?: unknown }).servers, | ||
| (payload as { items?: unknown }).items, | ||
| (payload as { mcpServers?: unknown }).mcpServers | ||
| ]; | ||
|
|
||
| for (const candidate of candidateArrays) { | ||
| if (!Array.isArray(candidate)) { | ||
| continue; | ||
| } | ||
|
|
||
| return candidate | ||
| .map((entry) => { | ||
| if (!entry || typeof entry !== 'object') { | ||
| return undefined; | ||
| } | ||
| return String((entry as { name?: string }).name ?? ''); | ||
| }) | ||
| .filter(Boolean) as string[]; | ||
| } | ||
| } | ||
|
|
||
| throw new Error('Unsupported JSON format returned by `codex mcp list --json`.'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and read the relevant section
if [ -f src/cli/doctor.ts ]; then
echo "=== File exists, reading lines 90-135 ==="
sed -n '90,135p' src/cli/doctor.ts | cat -n
else
echo "File not found"
fiRepository: clduab11/codex-synaptic
Length of output: 1838
🏁 Script executed:
# Search for existing error classes in the codebase to understand patterns
rg "class.*Error.*extends" src/ -A 3 | head -50Repository: clduab11/codex-synaptic
Length of output: 2913
🏁 Script executed:
# Check if CodexMcpParseError is already defined anywhere
rg "CodexMcpParseError" src/Repository: clduab11/codex-synaptic
Length of output: 49
🏁 Script executed:
# Search for throw statements in the doctor.ts file
rg "throw" src/cli/doctor.ts -B 2 -A 1Repository: clduab11/codex-synaptic
Length of output: 156
🏁 Script executed:
# Check what errors are imported or used in doctor.ts
head -40 src/cli/doctor.ts | cat -nRepository: clduab11/codex-synaptic
Length of output: 1530
🏁 Script executed:
# Check the CodexSynapticError structure and ErrorCode enum
cat -n src/core/errors.ts | head -60Repository: clduab11/codex-synaptic
Length of output: 2089
🏁 Script executed:
# Check how other CLI files handle errors
rg "throw" src/cli/ -B 3 -A 1 | head -60Repository: clduab11/codex-synaptic
Length of output: 3274
Use a specific error type for unsupported MCP list payloads.
Line 129 throws a generic Error, which violates the guideline to use specific error types. This matters for consistent error classification and handling.
Consider using the existing error architecture: import BridgeError from ../core/errors.js and throw new BridgeError(ErrorCode.MCP_ERROR, 'Unsupported JSON format returned by codex mcp list --json.') to align with how src/cli/launch.ts handles MCP errors. Alternatively, create a dedicated error class extending CodexSynapticError if MCP parsing errors warrant their own type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/doctor.ts` around lines 94 - 130, The parseCodexMcpNames function
currently throws a generic Error for unsupported payloads; replace that with the
project's specific error type by importing BridgeError and ErrorCode and throw
new BridgeError(ErrorCode.MCP_ERROR, 'Unsupported JSON format returned by `codex
mcp list --json`.') (or, if preferred, create and throw a new Codex-specific
error class that extends CodexSynapticError) so MCP parsing failures are
classified consistently with the error handling used in src/cli/launch.ts.
| @@ -0,0 +1,443 @@ | |||
| import { spawn, spawnSync, type SpawnSyncReturns } from 'child_process'; | |||
There was a problem hiding this comment.
Remove unused spawnSync import.
It isn’t referenced after the async spawn wrapper was added.
🧹 Suggested fix
-import { spawn, spawnSync, type SpawnSyncReturns } from 'child_process';
+import { spawn, type SpawnSyncReturns } from 'child_process';📝 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.
| import { spawn, spawnSync, type SpawnSyncReturns } from 'child_process'; | |
| import { spawn, type SpawnSyncReturns } from 'child_process'; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 1-1: src/cli/launch.ts#L1
'spawnSync' is defined but never used. (@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` at line 1, The import list in the module still includes
the unused symbol spawnSync from 'child_process' even though only the async
spawn wrapper is used; remove spawnSync from the import statement so it only
imports spawn (and type SpawnSyncReturns if still used) or drop the unused type
import entirely to eliminate the dead import.
| child.on('close', (code) => { | ||
| resolve({ | ||
| status: code ?? 0, | ||
| stdout, | ||
| stderr | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the actual code in src/cli/launch.ts around the flagged lines
head -100 src/cli/launch.ts | cat -nRepository: clduab11/codex-synaptic
Length of output: 3309
🌐 Web query:
Node.js child_process close event code null signal terminated behavior
💡 Result:
In Node.js, the 'close' event callback is ('close', (code, signal) => { ... }), where:
codeis the numeric exit code only if the child exited normally; it isnullif the child was terminated by a signal. [1]signalis the signal name (e.g.,'SIGTERM','SIGKILL') only if the child was terminated by a signal; otherwise it isnull. [1]- Exactly one of
codeorsignalwill be non-null. [1] 'close'fires after the process ends and after its stdio streams are closed, and it always happens after the child’s'exit'event (or after'error'if the child failed to spawn). [1]
So if you see code === null and signal === 'SIGTERM' (or similar), that’s the expected “terminated by signal” behavior—not an error by itself. [1]
Sources:
[1] Node.js documentation — ChildProcess 'close' event semantics (exit code null when terminated by signal; ordering vs 'exit'). (nodejs.org)
Citations:
Treat signal-terminated processes as failures, not success.
Line 49 uses code ?? 0, which treats processes killed by signals (SIGTERM, SIGKILL, etc.) as having status 0 (success). In Node.js, when a child is terminated by a signal, the close event callback receives code === null with the signal name in the second parameter. The current code incorrectly handles this as success.
🔧 Suggested fix
- child.on('close', (code) => {
+ child.on('close', (code, signal) => {
resolve({
- status: code ?? 0,
+ status: signal ? 1 : (code ?? 1),
stdout,
stderr
});
});📝 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.
| child.on('close', (code) => { | |
| resolve({ | |
| status: code ?? 0, | |
| stdout, | |
| stderr | |
| }); | |
| }); | |
| child.on('close', (code, signal) => { | |
| resolve({ | |
| status: signal ? 1 : (code ?? 1), | |
| stdout, | |
| stderr | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` around lines 47 - 53, The close handler treats
signal-terminated processes as success by using code ?? 0; change the listener
signature to (code, signal) and set the returned status to a non-zero value when
code is null (e.g., status = code !== null ? code : 1) and include the signal
name (signal) in the stderr or add it to the resolved object so callers can see
the termination reason; update the child.on('close', ...) block that currently
resolves { status: code ?? 0, stdout, stderr } accordingly.
|
Closed; outdated. |
Summary:
Testing: