Plan and chunk 1 start [PAP-17] - #56
Merged
Merged
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Summary of ChangesHello @clduab11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and user experience of the Codex-Synaptic CLI by introducing dedicated Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
coderabbitai
Bot
added
ci/cd
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces a more robust and deterministic launch and diagnostic path for the Codex-Synaptic CLI, specifically targeting macOS environments. Key improvements include the extraction of environment bootstrapping logic into a dedicated module, a new launch command that acts as a hard readiness gate, and enhanced error classification for Docker Compose failures. The addition of comprehensive unit and smoke tests ensures the reliability of these new components. The feedback focuses on improving the robustness of the environment variable parser and the Codex MCP name extraction logic.
Comment on lines
+128
to
+130
| resolve(cwd, ".env"), | ||
| resolve(cwd, ".env.local"), | ||
| resolve(cwd, "src/cli/.env"), |
Contributor
There was a problem hiding this comment.
The current order of candidates in bootstrapCliEnv combined with the 'set if undefined' logic in loadEnvFile results in .env taking precedence over .env.local. Conventionally, .env.local is intended to override base .env values to allow for local developer customizations that shouldn't be committed.
Suggested change
| resolve(cwd, ".env"), | |
| resolve(cwd, ".env.local"), | |
| resolve(cwd, "src/cli/.env"), | |
| resolve(cwd, ".env.local"), | |
| resolve(cwd, ".env"), | |
| resolve(cwd, "src/cli/.env"), |
Comment on lines
+95
to
+98
| const endsWithQuote = value.endsWith('"') || value.endsWith("'"); | ||
| if (startsWithQuote && endsWithQuote && value.length >= 2) { | ||
| value = value.slice(1, -1); | ||
| } |
Contributor
There was a problem hiding this comment.
The logic for stripping quotes from values only checks if the string starts and ends with a quote, but it doesn't verify that the quotes match (e.g., it would incorrectly strip the first and last characters of "value'). It's safer to ensure the opening and closing quotes are identical.
Suggested change
| const endsWithQuote = value.endsWith('"') || value.endsWith("'"); | |
| if (startsWithQuote && endsWithQuote && value.length >= 2) { | |
| value = value.slice(1, -1); | |
| } | |
| const quote = value[0]; | |
| const isQuoted = (quote === '"' || quote === "'") && value.endsWith(quote); | |
| if (isQuoted && value.length >= 2) { | |
| value = value.slice(1, -1); | |
| } |
| continue; | ||
| } | ||
|
|
||
| let value = line.slice(separatorIndex + 1).trim(); |
Contributor
There was a problem hiding this comment.
The current implementation of loadEnvFile does not handle inline comments (e.g., KEY=VALUE # comment). The comment and the hash character will be included in the environment variable's value, which can lead to unexpected behavior. While a full parser is complex, a basic split on # for unquoted values would improve robustness.
Suggested change
| let value = line.slice(separatorIndex + 1).trim(); | |
| let value = line.slice(separatorIndex + 1).trim(); | |
| if (!value.startsWith("\"") && !value.startsWith("'")) { | |
| value = value.split("#")[0].trim(); | |
| } |
| } | ||
| } | ||
|
|
||
| throw new Error('Unsupported JSON format returned by `codex mcp list --json`.'); |
Contributor
There was a problem hiding this comment.
The parseCodexMcpNames function throws an error if the payload is an object but doesn't contain any of the expected keys (servers, items, mcpServers). If the Codex CLI returns an empty object or a different structure in the future, this will cause the doctor command to fail. Returning an empty array instead of throwing would make the diagnostic more resilient.
Suggested change
| throw new Error('Unsupported JSON format returned by `codex mcp list --json`.'); | |
| return []; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e6b154d91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+198
to
+199
| details: stdout || loginStatus.stderr?.trim() || 'No output', | ||
| remediation: ok ? undefined : 'Run `codex login` then re-run `codex login status`.' |
There was a problem hiding this comment.
When codex is not installed, spawnSync returns status = null with empty output, so this step emits No output and recommends codex login instead of an installation remediation. Because launch runs in strict fail-fast mode by default, users in fresh environments stop here and get a misleading auth action rather than the real fix, which makes the startup gate harder to recover from.
Useful? React with 👍 / 👎.
![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: 13
🧹 Nitpick comments (2)
src/cli/doctor.ts (1)
6-10: Consider an enum for MCP profile constants.This keeps the profile names centralized and type‑safe as the list grows.
Example enum usage
-export const DEFAULT_MCP_PROFILES = [ - 'mcp-filesystem', - 'mcp-playwright', - 'mcp-desktop-commander' -] as const; +export enum McpProfile { + Filesystem = 'mcp-filesystem', + Playwright = 'mcp-playwright', + DesktopCommander = 'mcp-desktop-commander' +} + +export const DEFAULT_MCP_PROFILES = [ + McpProfile.Filesystem, + McpProfile.Playwright, + McpProfile.DesktopCommander +] as const;As per coding guidelines: Use enums for constants with semantic meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/doctor.ts` around lines 6 - 10, Replace the string-array constant DEFAULT_MCP_PROFILES with a TypeScript enum (e.g., enum MCPProfile { Filesystem = 'mcp-filesystem', Playwright = 'mcp-playwright', DesktopCommander = 'mcp-desktop-commander' }) and export it; update any code that referenced DEFAULT_MCP_PROFILES to use the enum values (or an exported array derived from Object.values(MCPProfile) if iteration is needed) and adjust types to use MCPProfile for stronger typing throughout the CLI (search for DEFAULT_MCP_PROFILES and switch those references to MCPProfile or the derived array).tests/cli/launch.test.ts (1)
5-9:passingDoctorReportsummary count is inconsistent with emptychecksarray.
summary: { passed: 6, failed: 0, total: 6 }butchecks: []. While this works as a mock (nothing iterates the checks), any futurerunLaunchlogic that recomputes the summary fromcheckswould see zero totals. Aligning the mock avoids silent surprises.✨ Proposed fix
const passingDoctorReport: DoctorReport = { ok: true, - summary: { passed: 6, failed: 0, total: 6 }, + summary: { passed: 0, failed: 0, total: 0 }, checks: [] };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/launch.test.ts` around lines 5 - 9, The mock passingDoctorReport has summary counts (passed:6,total:6) but an empty checks array, which is inconsistent; update the passingDoctorReport object so the checks array contains 6 check entries matching the summary (or adjust the summary to reflect zero checks) to keep DoctorReport consistent — locate the passingDoctorReport constant in tests/cli/launch.test.ts and either populate checks with six passing check objects or change summary to { passed: 0, failed: 0, total: 0 } accordingly so runLaunch logic that derives summary from checks won’t be surprised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guides/quick-start.md`:
- Around line 20-23: The example output in the quick-start guide uses unquoted
JSON-like keys (ok: true, nextAction: continue); update it to valid JSON by
quoting string values and optionally the whole block: change nextAction:
continue to "nextAction": "continue" (and preferably present the example as a
JSON code block showing { "ok": true, "nextAction": "continue" }) so the
documented --json output matches real JSON; target the keys "ok" and
"nextAction" in the snippet.
In `@docs/uat/UAT_READINESS_TRACKER.md`:
- Around line 91-98: The listed entry embeds a local absolute path
(/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/CODEX_MACOS_UAT_RUNBOOK.md);
replace it with a repo-relative path (e.g. docs/uat/CODEX_MACOS_UAT_RUNBOOK.md)
in the UAT_READINESS_TRACKER.md entry and update any other occurrences of
absolute /Users/... paths in this file to repo-relative equivalents; ensure the
README-style bullet text and any links reference the repo-relative path
consistently so the doc is portable across environments.
In `@src/cli/doctor.ts`:
- Around line 219-252: The loop over profileNames can throw when calling
getServiceStatus(profileName) for unknown profiles; wrap the per-profile logic
(the calls to getServiceStatus, getCodexRegistration, registriesForProfiles and
the subsequent computation that builds details/remediation and calls
checks.push) in a try/catch so a thrown error becomes a failed check instead of
crashing the doctor command—on catch, push a check with id `mcp.${profileName}`,
ok=false, details that include the caught error message and a short note like
"invalid profile" (and optional diagnostics empty), and a remediation suggesting
verifying the profile name; keep existing fields (metadata) as undefined when
unavailable.
- Around line 50-86: Replace the generic Error thrown in parseCodexMcpNames with
the structured BridgeError so downstream logic can detect MCP parse failures;
import BridgeError and ErrorCode from src/core/errors.ts (or the module
exporting them) and throw new BridgeError('Unsupported JSON format returned by
`codex mcp list --json`.', ErrorCode.MCP_ERROR, { retryable: false }) instead of
the plain Error in the final fallback of parseCodexMcpNames.
- Around line 128-184: runDoctor currently uses a synchronous spawnCommand
(spawnSync) which blocks; update the DoctorDependencies.spawnCommand type to
return a Promise and replace the default implementation with a non-blocking
wrapper around child_process.spawn that resolves with an object matching the
existing shape (status, stdout, stderr). Then update runDoctor to await
spawnCommand wherever it's called (the default definition of spawnCommand, the
invocation that runs node distCliPath --help inside the distExists block, and
the invocation that runs codex login status) so those calls become asynchronous
and non-blocking.
In `@src/cli/env-bootstrap.ts`:
- Around line 1-115: The loadEnvFile implementation uses blocking fs calls
(existsSync/readFileSync) which should be converted to async to avoid blocking
the event loop: change loadEnvFile to an async function returning
Promise<boolean> and use fs.promises (or import { promises as fs } ) to read the
file (e.g., await fs.readFile) and/or check existence via await fs.access
wrapped in try/catch; preserve the parsing logic and the same return semantics
(true when any env var applied, false on non-existent file or error), ensure
callers of loadEnvFile are updated to await the new async function; other
helpers (parseBooleanFlag, shouldAutoLoadCliEnv, shouldShowCliEnvBanner) can
remain sync but any code that previously called loadEnvFile synchronously must
be updated.
In `@src/cli/launch.ts`:
- Around line 28-77: Replace the string union nextAction on the LaunchReport
interface with a dedicated enum (e.g., LaunchNextAction) and update all usages
to use that enum; change the LaunchReport type to reference LaunchNextAction
instead of 'continue' | 'stop', update buildLaunchReport to return
LaunchNextAction.continue or LaunchNextAction.stop based on ok, and update any
callers that construct or inspect LaunchReport.nextAction to use the enum values
to avoid stringly-typed comparisons.
- Around line 1-68: The code currently uses blocking APIs (spawnSync in
normalizeSpawn and existsSync import) inside async flows; update
LaunchDependencies to expose async spawnCommand (returning a Promise with
status/stdout/stderr) and fileExists (async boolean using fs.promises.access),
replace normalizeSpawn to return an async function that awaits deps.spawnCommand
(or wraps child_process.execFile/spawn), and update all callers in this file
that use normalizeSpawn/existsSync to await the async versions
(preflight/auth/registration checks). Also replace the nextAction union type
with a small enum (e.g., enum NextAction { Continue = 'continue', Stop = 'stop'
}) and update LaunchReport.nextAction to use that enum. Ensure types
(SpawnSyncReturns usage) are removed/adjusted to Promise-based return types and
keep EMPTY_DOCTOR_REPORT and other logic unchanged.
In `@src/env/service-manager.ts`:
- Around line 358-395: The current wrapComposeStartError builds a plain Error;
change it to construct and return a CodexSynapticError (or the project's typed
error class) so callers can handle it programmatically: replace the final new
Error(...) with new CodexSynapticError(...) including a stable error code like
"COMPOSE_START_FAILED" and pass structured context (diagnosis, remediation,
exitStatus, composeCmd=cmd, truncatedOutput or full output, images,
serviceName=name, profile reference) so upstream can inspect properties; also
add the necessary import for CodexSynapticError at the top of the file and
ensure the constructor/fields match the project's error type signature.
- Around line 325-333: Refactor dockerLogin to be async and remove shell
interpolation: change the method signature dockerLogin(registry: string): void
to an async function that validates registry, then call
child_process.spawn('docker', ['login', normalized], { stdio: 'inherit' }) and
await completion by attaching listeners to 'close'/'error' (or wrapping in a
Promise) and reject on non-zero exit codes; do not use execSync or a shell
string. Ensure errors are thrown/propagated so callers can await and handle
them, and update the caller in src/cli/index.ts to await dockerLogin(...)
accordingly.
In `@tests/cli/doctor.test.ts`:
- Around line 139-175: The test fails because the real
serviceManager.registriesForProfiles returns an empty array for
'mcp-filesystem', so the docker-login remediation is never added; update the
test deps passed to runDoctor to mock registriesForProfiles (or
serviceManager.registriesForProfiles) to return a non-empty array (e.g., an
array with a registry object) for ['mcp-filesystem'] so the condition in
doctor.ts (registriesForProfiles([profileName]).length > 0) is true and the
docker-login command is included in the remediation string; reference the
runDoctor call and the
registriesForProfiles/serviceManager.registriesForProfiles symbol when adding
this mock.
In `@tests/cli/env-bootstrap.test.ts`:
- Around line 14-29: The tempDir cleanup in the "does not override existing
environment variables" test can leak if an assertion throws because rmSync is
only called at the end; replace the direct rmSync(tempDir, ...) call with a
vitest onTestFinished(() => rmSync(tempDir, { recursive: true, force: true }))
registration immediately after creating tempDir so cleanup always runs, and
apply the same onTestFinished-based cleanup to the other test in this file that
creates a tempDir and currently relies on a trailing rmSync.
In `@tests/cli/launch.test.ts`:
- Around line 106-141: The test fails because registriesForProfiles is not
mocked so the real serviceManager.registriesForProfiles may return [] in CI and
the 'docker-login' remediation assertion becomes flaky; update the runLaunch
deps object in this test to include a mock registriesForProfiles (e.g.,
registriesForProfiles: async (profiles) => ['docker.io'] or similar) so the
remediation built by runLaunch includes the docker-login entry consistently;
locate the test using runLaunch in this file and add the registriesForProfiles
mock alongside fileExists/spawnCommand/ensureService to ensure the
mcpStep.remediation assertions pass reliably.
---
Nitpick comments:
In `@src/cli/doctor.ts`:
- Around line 6-10: Replace the string-array constant DEFAULT_MCP_PROFILES with
a TypeScript enum (e.g., enum MCPProfile { Filesystem = 'mcp-filesystem',
Playwright = 'mcp-playwright', DesktopCommander = 'mcp-desktop-commander' }) and
export it; update any code that referenced DEFAULT_MCP_PROFILES to use the enum
values (or an exported array derived from Object.values(MCPProfile) if iteration
is needed) and adjust types to use MCPProfile for stronger typing throughout the
CLI (search for DEFAULT_MCP_PROFILES and switch those references to MCPProfile
or the derived array).
In `@tests/cli/launch.test.ts`:
- Around line 5-9: The mock passingDoctorReport has summary counts
(passed:6,total:6) but an empty checks array, which is inconsistent; update the
passingDoctorReport object so the checks array contains 6 check entries matching
the summary (or adjust the summary to reflect zero checks) to keep DoctorReport
consistent — locate the passingDoctorReport constant in tests/cli/launch.test.ts
and either populate checks with six passing check objects or change summary to {
passed: 0, failed: 0, total: 0 } accordingly so runLaunch logic that derives
summary from checks won’t be surprised.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.codex-synaptic/memory.dbis excluded by!**/*.db
📒 Files selected for processing (18)
.github/workflows/ci-non-mcp-gates.ymlAGENTS.mdREADME.mddocs/guides/codex-macos-workflows.mddocs/guides/quick-start.mddocs/uat/CODEX_MACOS_UAT_RUNBOOK.mddocs/uat/UAT_READINESS_TRACKER.mdsrc/cli/doctor.tssrc/cli/env-bootstrap.tssrc/cli/index.tssrc/cli/launch.tssrc/core/errors.tssrc/env/service-manager.tstests/cli/doctor.test.tstests/cli/env-bootstrap.test.tstests/cli/launch.test.tstests/e2e/cli-smoke.test.tstests/env/service-manager.test.ts
Comment on lines
+325
to
+333
| dockerLogin(registry: string): void { | ||
| const normalized = registry.trim(); | ||
| if (!normalized) { | ||
| throw new Error('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' }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view the relevant lines
if [ -f "src/env/service-manager.ts" ]; then
echo "=== File exists ==="
wc -l src/env/service-manager.ts
echo ""
echo "=== Lines 320-340 ==="
sed -n '320,340p' src/env/service-manager.ts
else
echo "File not found"
fiRepository: clduab11/codex-synaptic
Length of output: 694
🏁 Script executed:
# Search for dockerLogin method usages
echo "=== Searching for dockerLogin usages ==="
rg "dockerLogin" --type tsRepository: clduab11/codex-synaptic
Length of output: 235
🏁 Script executed:
# Check the imports and overall structure of the service-manager file
echo "=== First 50 lines of service-manager.ts ==="
head -50 src/env/service-manager.tsRepository: clduab11/codex-synaptic
Length of output: 1419
🏁 Script executed:
# Search for other execSync usages in the file to understand patterns
echo "=== execSync usages in service-manager ==="
rg "execSync" src/env/service-manager.ts -B 2 -A 2Repository: clduab11/codex-synaptic
Length of output: 987
Refactor dockerLogin to use async spawn instead of shell-interpolated execSync.
The method currently interpolates the registry parameter into a shell command string, creating a shell injection vector if the registry contains special characters (e.g., ; malicious-command). Additionally, execSync blocks the event loop, violating the guideline "Don't block the event loop with synchronous operations."
Use spawn with an args array and make the method async. The caller in src/cli/index.ts will need to await the result.
Suggested safer async execution
-import { execSync } from 'child_process';
+import { execSync, spawn } from 'child_process';
@@
- dockerLogin(registry: string): void {
+ async dockerLogin(registry: string): Promise<void> {
const normalized = registry.trim();
if (!normalized) {
throw new Error('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' });
+ await new Promise<void>((resolve, reject) => {
+ const child = spawn('docker', ['login', normalized], { stdio: 'inherit' });
+ child.on('error', reject);
+ child.on('exit', (code) => {
+ code === 0 ? resolve() : reject(new Error(`docker login exited with ${code ?? 'unknown'}`));
+ });
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/env/service-manager.ts` around lines 325 - 333, Refactor dockerLogin to
be async and remove shell interpolation: change the method signature
dockerLogin(registry: string): void to an async function that validates
registry, then call child_process.spawn('docker', ['login', normalized], {
stdio: 'inherit' }) and await completion by attaching listeners to
'close'/'error' (or wrapping in a Promise) and reject on non-zero exit codes; do
not use execSync or a shell string. Ensure errors are thrown/propagated so
callers can await and handle them, and update the caller in src/cli/index.ts to
await dockerLogin(...) accordingly.
Collaborator
Author
|
@copilot , verify each finding against the current code and only fix it if needed. Inline comments:
In
In
In
In
In
In
In
In
Nitpick comments:
In
|
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2cf4ae628
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.logger.info('env', `Starting service ${name}`, { command: cmd }); | ||
| execSync(cmd, { stdio: 'inherit', env }); | ||
| try { | ||
| execSync(cmd, { stdio: 'pipe', env, encoding: 'utf8' }); |
There was a problem hiding this comment.
Using execSync(..., { stdio: 'pipe' }) here introduces Node's default 1 MiB output buffer limit, so docker compose up -d can fail with ENOBUFS when image-pull logs are large (a common first-run path for MCP images). In that scenario launch/env bootstrap fails even though Docker is otherwise healthy, because the child process is killed before startup completes. Please keep streaming output (inherit) or set an explicit larger maxBuffer while preserving error classification.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
@copilot , execute needed fix.
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/env/service-manager.ts (1)
190-200:⚠️ Potential issue | 🟠 MajorReplace
execSyncwith asyncexecFileorspawnto preserve CLI responsiveness.The method is declared async but uses
execSyncinternally (line 197), blocking the event loop during the docker compose startup. This contradicts the coding guideline "Don't block the event loop with synchronous operations" and defeats the benefit of async/await. Consider switching toexecFileorspawnfromchild_processwhile maintaining the stdout/stderr capture for error wrapping inwrapComposeStartError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 190 - 200, The ensureService method currently uses blocking execSync (composeCommand/ensureService) which stalls the event loop; replace it with an async child_process call (prefer spawn or execFile) invoked with await (or wrapped in a Promise) so the function remains non-blocking, stream stdout/stderr into buffers so you can still pass captured output and the original error to wrapComposeStartError(name, profile, cmd, error), and ensure env is passed via resolveExecEnv(name, options) and logging behavior (this.logger.info(..., { command: cmd })) is preserved.
♻️ Duplicate comments (6)
src/env/service-manager.ts (2)
325-333:⚠️ Potential issue | 🟠 Major
dockerLoginshould avoid shell‑interpolatedexecSync.
Shell interpolation creates an injection vector, andexecSyncblocks the event loop. Usespawn/execFilewith args and make the methodasyncso callers can await and handle failures.As per coding guidelines: "Don't block the event loop with synchronous operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 325 - 333, dockerLogin currently uses shell-interpolated execSync which blocks the event loop and allows shell injection; change dockerLogin to be async, accept/validate registry, construct arguments (e.g., ['login', normalized]) and invoke child_process.spawn or execFile (not execSync) with those args, forward stdio to inherit or pipe and await the child process completion via a Promise, catch and rethrow or log errors using this.logger (e.g., in 'env' context) so callers can await failures; update the method signature and all callers to await dockerLogin accordingly and avoid any string interpolation into a shell command.
358-395:⚠️ Potential issue | 🟠 MajorReturn a typed error instead of a generic
Error.
wrapComposeStartErrorshould surface a specific, structured error type so callers can react programmatically.As per coding guidelines: "Throw specific error types, not generic Error objects."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 358 - 395, wrapComposeStartError currently returns a generic Error; replace it with and return a specific error class (e.g., DockerComposeError) that extends Error and exposes structured properties (diagnosis, remediation, exitStatus, composeCmd, output, serviceName, images, profile: ServiceProfile) so callers can inspect fields programmatically; update the method signature to return DockerComposeError, construct and return a new DockerComposeError(instance) with the same computed values (diagnosis, remediation, exitStatus, truncatedOutput, cmd, name, images, profile) and export the DockerComposeError type so other modules can import and use it for programmatic handling.docs/uat/UAT_READINESS_TRACKER.md (1)
91-99:⚠️ Potential issue | 🟡 MinorPrefer repo‑relative paths over local absolute paths.
The repeated/Users/...paths reduce portability. Please convert all occurrences to repo‑relative paths.✍️ Example cleanup
-- Added `/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/CODEX_MACOS_UAT_RUNBOOK.md` +- Added `docs/uat/CODEX_MACOS_UAT_RUNBOOK.md`Also applies to: 127-135, 176-194, 229-238, 280-288, 371-379, 463-471, 505-513, 557-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/uat/UAT_READINESS_TRACKER.md` around lines 91 - 99, Replace all hardcoded local absolute paths in UAT_READINESS_TRACKER.md with repo-relative paths: search for occurrences of absolute local paths (e.g., the referenced CODEX_MACOS_UAT_RUNBOOK.md entry) and change them to relative references inside the repo (e.g., docs/uat/CODEX_MACOS_UAT_RUNBOOK.md or ./docs/uat/...), updating every occurrence noted in the review (the repeated absolute paths) so links and examples are portable and consistent across environments. Ensure any examples, link targets, and README entries use relative paths and update any surrounding text that assumes a /Users/... local layout.src/cli/launch.ts (2)
1-2:⚠️ Potential issue | 🟠 MajorAvoid synchronous FS/process calls inside the async launch gate.
runLaunchis async but relies onexistsSync/spawnSync, which blocks the event loop during preflight/auth/registration and can freeze the CLI on slow Docker/Codex calls. Prefer asyncfs.promises.access+execFile/spawn, and updateLaunchDependenciesto accept async variants.#!/bin/bash # Verify sync APIs used in launch gate rg -n "spawnSync|existsSync" src/cli/launch.tsAs per coding guidelines, "Don't block the event loop with synchronous operations".
Also applies to: 61-70, 149-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/launch.ts` around lines 1 - 2, runLaunch currently uses blocking APIs (existsSync and spawnSync) which can freeze the event loop; replace those with non-blocking equivalents and update the dependency types: use fs.promises.access (or fs.promises.stat) instead of existsSync to check file presence, and use child_process.execFile or spawn (returning a Promise or using the streaming callbacks) instead of spawnSync for external commands; update the LaunchDependencies interface/type to accept the async variants (e.g. async checkPath/access function and an execFile/spawn-based runner that returns a Promise<SpawnResult>), then refactor functions that call existsSync/spawnSync (referenced by names runLaunch and any helper preflight/auth/registration helpers in this file) to await the new async methods and propagate errors accordingly.
29-34: Use an enum fornextActionto avoid stringly-typed flow.
This is a semantic constant; an enum keeps intent clear and prevents typo bugs.#!/bin/bash # Locate nextAction usages for refactor rg -n "nextAction" src/cli/launch.ts♻️ Suggested update
+export enum LaunchNextAction { + Continue = 'continue', + Stop = 'stop' +} + export interface LaunchReport { ok: boolean; steps: LaunchStep[]; doctor: DoctorReport; - nextAction: 'continue' | 'stop'; + nextAction: LaunchNextAction; } function buildLaunchReport(steps: LaunchStep[], doctorReport: DoctorReport): LaunchReport { const ok = steps.every((step) => step.ok) && doctorReport.ok; return { ok, steps, doctor: doctorReport, - nextAction: ok ? 'continue' : 'stop' + nextAction: ok ? LaunchNextAction.Continue : LaunchNextAction.Stop }; }As per coding guidelines, "Use enums for constants with semantic meaning".
Also applies to: 72-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/launch.ts` around lines 29 - 34, Replace the string union for nextAction with a dedicated enum to prevent stringly-typed bugs: define an exported enum (e.g., NextAction { Continue = 'continue', Stop = 'stop' }), update the LaunchReport.nextAction type to use that enum, and refactor all usages that set or compare nextAction to use NextAction. Ensure the enum is exported from the module (or imported where needed) and update any runtime/serialization points to keep the same string values if required.tests/cli/launch.test.ts (1)
107-141: Duplicate: mockregistriesForProfilesto avoid env-dependent remediation assertions.Same concern as earlier review: without a mock, the real service manager can return
[]in CI and make remediation assertions flaky.Based on learnings: “Mock external dependencies and system resources in tests.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/launch.test.ts` around lines 107 - 141, The test is flaky because it relies on the real registriesForProfiles behavior; mock registriesForProfiles in this test to return a deterministic value (e.g., a non-empty array such as ['docker.io'] or a registry that matches the expected remediation) so the remediation assertions are stable; locate the test that calls runLaunch and add a mock/stub for registriesForProfiles (or the service manager method that resolves registries for profiles) in the injected test doubles so the mcpStep.remediation contains the expected registry-specific commands for profile 'mcp-filesystem'.
🧹 Nitpick comments (3)
docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.cmd.txt (1)
1-1: Prefer an immutable image reference for reproducible evidence.Using
:latestcan change over time, which makes UAT evidence less deterministic. Consider pinning an explicit version tag or digest in this evidence file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.cmd.txt` at line 1, The pull line uses an unstable tag "ghcr.io/context-labs/playwright-mcp:latest" which makes evidence non-reproducible; update the docker reference in this file to a pinned immutable identifier by replacing ":latest" with a concrete version tag or an image digest (sha256) so the pulled image is deterministic and reproducible for UAT evidence.package.json (1)
11-11: Consider whetherdocker/assets belong in the npm publish manifest.Including the entire
docker/directory ships build/deployment infrastructure to every npm consumer, inflating the tarball. Unless downstream users are expected to use these Docker configurations directly, consider removing this entry and relying on the GitHub repo for Docker assets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 11, The package.json currently lists "docker/" in the published files manifest which will include the entire Docker directory in npm package tarballs; remove "docker/" from the "files" array in package.json (or alternatively add docker/ to .npmignore) so build/deployment assets aren’t shipped to consumers, keeping the published package minimal and relying on the repository for Docker files.docs/uat/CODEX_MACOS_UAT_RUNBOOK.md (1)
72-76: Consider using a date placeholder for evidence folders.
Hardcoding2026-02-23will go stale quickly. A placeholder (or$(date +%F)) keeps runs consistent across dates.📝 Example tweak
-mkdir -p docs/uat/evidence/2026-02-23 +mkdir -p "docs/uat/evidence/$(date +%F)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/uat/CODEX_MACOS_UAT_RUNBOOK.md` around lines 72 - 76, The runbook currently hardcodes the evidence folder date in the mkdir line (mkdir -p docs/uat/evidence/2026-02-23), which will become stale; change that invocation to use a date placeholder or shell substitution (e.g., docs/uat/evidence/$(date +%F) or docs/uat/evidence/<DATE_PLACEHOLDER>) so each run creates a dated folder dynamically and update the surrounding text to mention using the placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.cmd.txt`:
- Line 1: The three UAT evidence files (00a-ghcr-pull-filesystem.cmd.txt,
00b-..., 00c-...) currently use the non-deterministic :latest tag; replace
:latest with a pinned reference (preferably the resolved digest like
`@sha256`:...) or a stable semver tag across all three files to ensure
reproducibility; to obtain the digest run docker pull
ghcr.io/context-labs/filesystem-mcp:latest and then docker inspect
--format='{{index .RepoDigests 0}}' ghcr.io/context-labs/filesystem-mcp:latest
and update each file so they all use the same pinned image reference.
In `@docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.out.txt`:
- Line 1: The UAT failure is caused by an invalid image reference
ghcr.io/context-labs/playwright-mcp:latest; either switch the image reference
wherever it’s used (e.g., in the UAT/runtime config or docker-compose/service
definition that refers to "playwright-mcp") to the official image
mcr.microsoft.com/playwright/mcp, or build+push a context-labs image to
ghcr.io/context-labs/playwright-mcp and update the reference to that pushed tag;
after choosing, update the UAT tracker to mark the missing image as resolved or
the blocker as still open if you intend to publish later.
In `@docs/uat/evidence/2026-02-24-rerun-1/launch.strict.json`:
- Around line 5-23: The JSON evidence contains absolute local paths (e.g.,
"/Users/chrisdukes/LocalProjects/codex-synaptic/..." inside the "details" values
for entries such as id "repo.preflight" and the raw docker output under id
"mcp.up"); update the file launch.strict.json to redact any absolute user/home
paths by replacing them with a placeholder like "<REPO_ROOT>" (or another agreed
placeholder) for all "details" fields and any other string values that match
"/Users/<username>/*" or similar patterns so PII is removed while preserving the
rest of the message.
In `@docs/uat/evidence/2026-02-24/08-env-up.out.txt`:
- Around line 3-5: The evidence log contains a local absolute path string like
"/Users/chrisdukes/LocalProjects/codex-synaptic/..." that must be redacted; open
the failing evidence file (the shown output) and replace all occurrences of that
local path prefix with the neutral placeholder "<REPO_ROOT>" (or run a single
search-and-replace for the pattern
"/Users/{username}/LocalProjects/codex-synaptic" -> "<REPO_ROOT>") so the
docker-compose warning and any other entries no longer include user-specific
filesystem paths before committing.
In `@docs/uat/evidence/2026-02-24/codex.mcp.list.json`:
- Around line 1-125: This JSON is an evidence-only snapshot (root is an array of
tool objects with "name" entries like "desktop-commander", "figma",
"filesystem-local", etc.) but does not include the required agent
manifest/topology/consensus sections; either move/rename this file to a non-JSON
manifest path (e.g., change extension to .txt or put under an excluded evidence
folder) or convert it into a proper Agent Manifest by adding the required
top-level sections: an "agents" (agent specifications with resource
requirements, capabilities, and deployment/scaling rules), a "topology" (network
topology and security policies referencing the existing tool entries by name),
and a "consensus" object (consensus mechanism parameters and policies); update
the document to include those keys and ensure each tool object (e.g., entries
with "name":"desktop-commander", "figma", etc.) is referenced appropriately in
the new "agents" and "topology" sections.
In `@docs/uat/evidence/2026-02-24/launch.strict.json`:
- Around line 1-41: The file launch.strict.json contains a non-JSON log prefix
on the first line that breaks parsing; remove the leading log line so the file
begins with the JSON object (the "{" that starts the payload) and move the
removed stdout line into a separate artifact (e.g., launch.strict.stdout.txt) to
preserve raw output; ensure the final launch.strict.json contains only the JSON
object shown (ok/steps/doctor/nextAction) so JSON tooling can parse it.
In `@docs/uat/evidence/2026-02-24/launch.strict.payload.json`:
- Around line 5-27: The JSON evidence contains raw local paths/usernames in
"details" fields (e.g., the repo.preflight "Found
/Users/chrisdukes/LocalProjects/..." string and mcp.up raw docker output), so
update the evidence-generation/sanitization step to redact local home paths and
usernames before saving; specifically modify the code that writes these
"details" entries (search for where "repo.preflight" and "mcp.up" payload
objects are composed) to run a sanitization function that replaces patterns like
^/Users/[^/]+(/|$) or OS home directory (os.homedir()) occurrences with a
placeholder such as "<REDACTED_PATH>" or "<REDACTED_USER>", and apply the same
regex to any multi-line logs captured for mcp.up so no PII (usernames or
absolute local paths) are stored in the JSON evidence.
In `@docs/uat/UAT_FINAL_REPORT.md`:
- Around line 24-32: Replace machine-specific absolute paths like
`/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/`
and
`/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/_status.tsv`
with repo-relative paths (e.g., `docs/uat/evidence/2026-02-24-rerun-1/` and
`docs/uat/evidence/2026-02-24-rerun-1/_status.tsv`) throughout
UAT_FINAL_REPORT.md, and apply the same replacement for the other occurrences
referenced in the comment (the sections around the other listed blocks).
In `@package.json`:
- Line 19: The package.json "files" manifest currently includes the entry
".codex-improvement/SCHEMA_MASTER.yaml"; verify whether that schema is intended
to be published to npm and if it is internal-only remove it from the "files"
array (or relocate it outside the package root) so it won't be packed, or
alternatively keep it but add an explicit exclusion (e.g., move to .npmignore or
a non-dot directory) to prevent accidental publishing; update the package.json
"files" array to remove ".codex-improvement/SCHEMA_MASTER.yaml" if it's internal
and commit the change.
In `@src/cli/index.ts`:
- Around line 4376-4402: Validate incoming profile names before calling
serviceManager.registriesForProfiles: if names were provided, check each against
the known profiles list (e.g., DEFAULT_MCP_PROFILES or
serviceManager.availableProfiles / similar source) and if any are unknown throw
a user-friendly error listing the valid profile names; do this in the envCmd
.action handler (before computing registries) so that when registriesForProfiles
returns empty it's only legitimate for valid profiles, and keep the existing
dry-run and dockerLogin logic unchanged (references:
envCmd.command('docker-login'), DEFAULT_MCP_PROFILES, registriesForProfiles,
serviceManager.dockerLogin).
In `@src/cli/launch.ts`:
- Around line 412-424: Wrap the call to executeDoctor inside runLaunch in a
try-catch so failures produce a deterministic LaunchReport: call
executeDoctor(...) in try, assign doctorReport on success, and in catch create a
doctorReport that includes a failed "doctor.strict" step with the caught error
message/details; ensure runLaunch then returns a structured LaunchReport
containing that doctorReport instead of letting the exception bubble. Reference
executeDoctor, runLaunch, doctorReport, and the "doctor.strict" step when making
the change.
---
Outside diff comments:
In `@src/env/service-manager.ts`:
- Around line 190-200: The ensureService method currently uses blocking execSync
(composeCommand/ensureService) which stalls the event loop; replace it with an
async child_process call (prefer spawn or execFile) invoked with await (or
wrapped in a Promise) so the function remains non-blocking, stream stdout/stderr
into buffers so you can still pass captured output and the original error to
wrapComposeStartError(name, profile, cmd, error), and ensure env is passed via
resolveExecEnv(name, options) and logging behavior (this.logger.info(..., {
command: cmd })) is preserved.
---
Duplicate comments:
In `@docs/uat/UAT_READINESS_TRACKER.md`:
- Around line 91-99: Replace all hardcoded local absolute paths in
UAT_READINESS_TRACKER.md with repo-relative paths: search for occurrences of
absolute local paths (e.g., the referenced CODEX_MACOS_UAT_RUNBOOK.md entry) and
change them to relative references inside the repo (e.g.,
docs/uat/CODEX_MACOS_UAT_RUNBOOK.md or ./docs/uat/...), updating every
occurrence noted in the review (the repeated absolute paths) so links and
examples are portable and consistent across environments. Ensure any examples,
link targets, and README entries use relative paths and update any surrounding
text that assumes a /Users/... local layout.
In `@src/cli/launch.ts`:
- Around line 1-2: runLaunch currently uses blocking APIs (existsSync and
spawnSync) which can freeze the event loop; replace those with non-blocking
equivalents and update the dependency types: use fs.promises.access (or
fs.promises.stat) instead of existsSync to check file presence, and use
child_process.execFile or spawn (returning a Promise or using the streaming
callbacks) instead of spawnSync for external commands; update the
LaunchDependencies interface/type to accept the async variants (e.g. async
checkPath/access function and an execFile/spawn-based runner that returns a
Promise<SpawnResult>), then refactor functions that call existsSync/spawnSync
(referenced by names runLaunch and any helper preflight/auth/registration
helpers in this file) to await the new async methods and propagate errors
accordingly.
- Around line 29-34: Replace the string union for nextAction with a dedicated
enum to prevent stringly-typed bugs: define an exported enum (e.g., NextAction {
Continue = 'continue', Stop = 'stop' }), update the LaunchReport.nextAction type
to use that enum, and refactor all usages that set or compare nextAction to use
NextAction. Ensure the enum is exported from the module (or imported where
needed) and update any runtime/serialization points to keep the same string
values if required.
In `@src/env/service-manager.ts`:
- Around line 325-333: dockerLogin currently uses shell-interpolated execSync
which blocks the event loop and allows shell injection; change dockerLogin to be
async, accept/validate registry, construct arguments (e.g., ['login',
normalized]) and invoke child_process.spawn or execFile (not execSync) with
those args, forward stdio to inherit or pipe and await the child process
completion via a Promise, catch and rethrow or log errors using this.logger
(e.g., in 'env' context) so callers can await failures; update the method
signature and all callers to await dockerLogin accordingly and avoid any string
interpolation into a shell command.
- Around line 358-395: wrapComposeStartError currently returns a generic Error;
replace it with and return a specific error class (e.g., DockerComposeError)
that extends Error and exposes structured properties (diagnosis, remediation,
exitStatus, composeCmd, output, serviceName, images, profile: ServiceProfile) so
callers can inspect fields programmatically; update the method signature to
return DockerComposeError, construct and return a new
DockerComposeError(instance) with the same computed values (diagnosis,
remediation, exitStatus, truncatedOutput, cmd, name, images, profile) and export
the DockerComposeError type so other modules can import and use it for
programmatic handling.
In `@tests/cli/launch.test.ts`:
- Around line 107-141: The test is flaky because it relies on the real
registriesForProfiles behavior; mock registriesForProfiles in this test to
return a deterministic value (e.g., a non-empty array such as ['docker.io'] or a
registry that matches the expected remediation) so the remediation assertions
are stable; locate the test that calls runLaunch and add a mock/stub for
registriesForProfiles (or the service manager method that resolves registries
for profiles) in the injected test doubles so the mcpStep.remediation contains
the expected registry-specific commands for profile 'mcp-filesystem'.
---
Nitpick comments:
In `@docs/uat/CODEX_MACOS_UAT_RUNBOOK.md`:
- Around line 72-76: The runbook currently hardcodes the evidence folder date in
the mkdir line (mkdir -p docs/uat/evidence/2026-02-23), which will become stale;
change that invocation to use a date placeholder or shell substitution (e.g.,
docs/uat/evidence/$(date +%F) or docs/uat/evidence/<DATE_PLACEHOLDER>) so each
run creates a dated folder dynamically and update the surrounding text to
mention using the placeholder.
In `@docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.cmd.txt`:
- Line 1: The pull line uses an unstable tag
"ghcr.io/context-labs/playwright-mcp:latest" which makes evidence
non-reproducible; update the docker reference in this file to a pinned immutable
identifier by replacing ":latest" with a concrete version tag or an image digest
(sha256) so the pulled image is deterministic and reproducible for UAT evidence.
In `@package.json`:
- Line 11: The package.json currently lists "docker/" in the published files
manifest which will include the entire Docker directory in npm package tarballs;
remove "docker/" from the "files" array in package.json (or alternatively add
docker/ to .npmignore) so build/deployment assets aren’t shipped to consumers,
keeping the published package minimal and relying on the repository for Docker
files.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.codex-synaptic/memory.dbis excluded by!**/*.dbdocs/uat/evidence/2026-02-24-rerun-1/_status.tsvis excluded by!**/*.tsvdocs/uat/evidence/2026-02-24/_status.tsvis excluded by!**/*.tsvpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (105)
docker/mcp/docker-compose.playwright.ymldocs/uat/CODEX_MACOS_UAT_RUNBOOK.mddocs/uat/UAT_FINAL_REPORT.mddocs/uat/UAT_READINESS_TRACKER.mddocs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.exitdocs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.out.txtdocs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.exitdocs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.out.txtdocs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.exitdocs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.out.txtdocs/uat/evidence/2026-02-24-rerun-1/01-build.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/01-build.exitdocs/uat/evidence/2026-02-24-rerun-1/01-build.out.txtdocs/uat/evidence/2026-02-24-rerun-1/02-codex-help.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/02-codex-help.exitdocs/uat/evidence/2026-02-24-rerun-1/02-codex-help.out.txtdocs/uat/evidence/2026-02-24-rerun-1/03-codex-mcp-help.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/03-codex-mcp-help.exitdocs/uat/evidence/2026-02-24-rerun-1/03-codex-mcp-help.out.txtdocs/uat/evidence/2026-02-24-rerun-1/04-codex-mcp-add-help.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/04-codex-mcp-add-help.exitdocs/uat/evidence/2026-02-24-rerun-1/04-codex-mcp-add-help.out.txtdocs/uat/evidence/2026-02-24-rerun-1/05-codex-login-status.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/05-codex-login-status.exitdocs/uat/evidence/2026-02-24-rerun-1/05-codex-login-status.out.txtdocs/uat/evidence/2026-02-24-rerun-1/06-env-plan.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/06-env-plan.exitdocs/uat/evidence/2026-02-24-rerun-1/06-env-plan.out.txtdocs/uat/evidence/2026-02-24-rerun-1/07-env-docker-login.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/07-env-docker-login.exitdocs/uat/evidence/2026-02-24-rerun-1/07-env-docker-login.out.txtdocs/uat/evidence/2026-02-24-rerun-1/08-env-up.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/08-env-up.exitdocs/uat/evidence/2026-02-24-rerun-1/08-env-up.out.txtdocs/uat/evidence/2026-02-24-rerun-1/09-env-status.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/09-env-status.exitdocs/uat/evidence/2026-02-24-rerun-1/09-env-status.out.txtdocs/uat/evidence/2026-02-24-rerun-1/10-env-codex-register.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/10-env-codex-register.exitdocs/uat/evidence/2026-02-24-rerun-1/10-env-codex-register.out.txtdocs/uat/evidence/2026-02-24-rerun-1/11-codex-mcp-list.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/11-codex-mcp-list.err.txtdocs/uat/evidence/2026-02-24-rerun-1/11-codex-mcp-list.exitdocs/uat/evidence/2026-02-24-rerun-1/11-codex-mcp-list.redaction.txtdocs/uat/evidence/2026-02-24-rerun-1/12-doctor-strict-json.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/12-doctor-strict-json.err.txtdocs/uat/evidence/2026-02-24-rerun-1/12-doctor-strict-json.exitdocs/uat/evidence/2026-02-24-rerun-1/13-launch-strict-json.cmd.txtdocs/uat/evidence/2026-02-24-rerun-1/13-launch-strict-json.err.txtdocs/uat/evidence/2026-02-24-rerun-1/13-launch-strict-json.exitdocs/uat/evidence/2026-02-24-rerun-1/codex.mcp.list.jsondocs/uat/evidence/2026-02-24-rerun-1/doctor.strict.jsondocs/uat/evidence/2026-02-24-rerun-1/launch.strict.jsondocs/uat/evidence/2026-02-24/01-build.cmd.txtdocs/uat/evidence/2026-02-24/01-build.exitdocs/uat/evidence/2026-02-24/01-build.out.txtdocs/uat/evidence/2026-02-24/02-codex-help.cmd.txtdocs/uat/evidence/2026-02-24/02-codex-help.exitdocs/uat/evidence/2026-02-24/02-codex-help.out.txtdocs/uat/evidence/2026-02-24/03-codex-mcp-help.cmd.txtdocs/uat/evidence/2026-02-24/03-codex-mcp-help.exitdocs/uat/evidence/2026-02-24/03-codex-mcp-help.out.txtdocs/uat/evidence/2026-02-24/04-codex-mcp-add-help.cmd.txtdocs/uat/evidence/2026-02-24/04-codex-mcp-add-help.exitdocs/uat/evidence/2026-02-24/04-codex-mcp-add-help.out.txtdocs/uat/evidence/2026-02-24/05-codex-login-status.cmd.txtdocs/uat/evidence/2026-02-24/05-codex-login-status.exitdocs/uat/evidence/2026-02-24/05-codex-login-status.out.txtdocs/uat/evidence/2026-02-24/06-env-plan.cmd.txtdocs/uat/evidence/2026-02-24/06-env-plan.exitdocs/uat/evidence/2026-02-24/06-env-plan.out.txtdocs/uat/evidence/2026-02-24/07-env-docker-login.cmd.txtdocs/uat/evidence/2026-02-24/07-env-docker-login.exitdocs/uat/evidence/2026-02-24/07-env-docker-login.out.txtdocs/uat/evidence/2026-02-24/08-env-up.cmd.txtdocs/uat/evidence/2026-02-24/08-env-up.exitdocs/uat/evidence/2026-02-24/08-env-up.out.txtdocs/uat/evidence/2026-02-24/09-env-status.cmd.txtdocs/uat/evidence/2026-02-24/09-env-status.exitdocs/uat/evidence/2026-02-24/09-env-status.out.txtdocs/uat/evidence/2026-02-24/10-env-codex-register.cmd.txtdocs/uat/evidence/2026-02-24/10-env-codex-register.exitdocs/uat/evidence/2026-02-24/10-env-codex-register.out.txtdocs/uat/evidence/2026-02-24/11-codex-mcp-list.cmd.txtdocs/uat/evidence/2026-02-24/11-codex-mcp-list.err.txtdocs/uat/evidence/2026-02-24/11-codex-mcp-list.exitdocs/uat/evidence/2026-02-24/12-doctor-strict-json.cmd.txtdocs/uat/evidence/2026-02-24/12-doctor-strict-json.err.txtdocs/uat/evidence/2026-02-24/12-doctor-strict-json.exitdocs/uat/evidence/2026-02-24/13-launch-strict-json.cmd.txtdocs/uat/evidence/2026-02-24/13-launch-strict-json.err.txtdocs/uat/evidence/2026-02-24/13-launch-strict-json.exitdocs/uat/evidence/2026-02-24/codex.mcp.list.jsondocs/uat/evidence/2026-02-24/doctor.strict.jsondocs/uat/evidence/2026-02-24/launch.strict.jsondocs/uat/evidence/2026-02-24/launch.strict.payload.jsondocs/uat/evidence/2026-02-24/launch.strict.stdout-prefix.txtpackage.jsonsrc/cli/index.tssrc/cli/launch.tssrc/env/service-manager.tstests/cli/launch.test.ts
✅ Files skipped from review due to trivial changes (61)
- docs/uat/evidence/2026-02-24/04-codex-mcp-add-help.exit
- docs/uat/evidence/2026-02-24-rerun-1/13-launch-strict-json.err.txt
- docs/uat/evidence/2026-02-24-rerun-1/07-env-docker-login.cmd.txt
- docs/uat/evidence/2026-02-24/06-env-plan.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/08-env-up.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/01-build.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/04-codex-mcp-add-help.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/09-env-status.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.out.txt
- docs/uat/evidence/2026-02-24/03-codex-mcp-help.exit
- docs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.exit
- docs/uat/evidence/2026-02-24/launch.strict.stdout-prefix.txt
- docs/uat/evidence/2026-02-24-rerun-1/12-doctor-strict-json.err.txt
- docs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.exit
- docs/uat/evidence/2026-02-24-rerun-1/03-codex-mcp-help.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/01-build.exit
- docs/uat/evidence/2026-02-24/13-launch-strict-json.exit
- docs/uat/evidence/2026-02-24-rerun-1/09-env-status.cmd.txt
- docs/uat/evidence/2026-02-24/12-doctor-strict-json.exit
- docs/uat/evidence/2026-02-24/06-env-plan.out.txt
- docs/uat/evidence/2026-02-24/12-doctor-strict-json.err.txt
- docs/uat/evidence/2026-02-24-rerun-1/07-env-docker-login.out.txt
- docs/uat/evidence/2026-02-24/10-env-codex-register.exit
- docs/uat/evidence/2026-02-24-rerun-1/11-codex-mcp-list.redaction.txt
- docs/uat/evidence/2026-02-24-rerun-1/04-codex-mcp-add-help.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/03-codex-mcp-help.out.txt
- docs/uat/evidence/2026-02-24/11-codex-mcp-list.exit
- docs/uat/evidence/2026-02-24/10-env-codex-register.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/09-env-status.exit
- docs/uat/evidence/2026-02-24-rerun-1/02-codex-help.exit
- docs/uat/evidence/2026-02-24/04-codex-mcp-add-help.cmd.txt
- docs/uat/evidence/2026-02-24/07-env-docker-login.out.txt
- docs/uat/evidence/2026-02-24/09-env-status.exit
- docs/uat/evidence/2026-02-24/13-launch-strict-json.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/13-launch-strict-json.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/02-codex-help.out.txt
- docs/uat/evidence/2026-02-24/01-build.exit
- docs/uat/evidence/2026-02-24-rerun-1/04-codex-mcp-add-help.exit
- docs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/05-codex-login-status.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.out.txt
- docs/uat/evidence/2026-02-24/02-codex-help.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/05-codex-login-status.exit
- docs/uat/evidence/2026-02-24/01-build.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/13-launch-strict-json.exit
- docs/uat/evidence/2026-02-24-rerun-1/12-doctor-strict-json.exit
- docs/uat/evidence/2026-02-24/05-codex-login-status.exit
- docs/uat/evidence/2026-02-24/12-doctor-strict-json.cmd.txt
- docs/uat/evidence/2026-02-24/02-codex-help.exit
- docs/uat/evidence/2026-02-24-rerun-1/08-env-up.cmd.txt
- docs/uat/evidence/2026-02-24/06-env-plan.exit
- docs/uat/evidence/2026-02-24-rerun-1/07-env-docker-login.exit
- docs/uat/evidence/2026-02-24-rerun-1/05-codex-login-status.cmd.txt
- docs/uat/evidence/2026-02-24-rerun-1/11-codex-mcp-list.exit
- docs/uat/evidence/2026-02-24-rerun-1/06-env-plan.exit
- docs/uat/evidence/2026-02-24-rerun-1/08-env-up.exit
- docs/uat/evidence/2026-02-24/05-codex-login-status.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/03-codex-mcp-help.exit
- docs/uat/evidence/2026-02-24/02-codex-help.out.txt
- docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.exit
- docs/uat/evidence/2026-02-24-rerun-1/02-codex-help.cmd.txt
| @@ -0,0 +1 @@ | |||
| docker pull ghcr.io/context-labs/filesystem-mcp:latest | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# List evidence files in the directory to confirm sibling files exist
find docs/uat/evidence/2026-02-24-rerun-1/ -type f | head -20Repository: clduab11/codex-synaptic
Length of output: 1358
🏁 Script executed:
# Search for PR-related context in README or documentation files
find . -name "README*" -o -name "*.md" | xargs grep -l "deterministic\|MCP" 2>/dev/null | head -10Repository: clduab11/codex-synaptic
Length of output: 382
🌐 Web query:
ghcr.io/context-labs/filesystem-mcp docker image tags versions
💡 Result:
I couldn’t find any public page/index that lists the available tags for ghcr.io/context-labs/filesystem-mcp via web search (it may be private or unlisted).
If you have access (PAT with read:packages), you can list tags/versions like this:
- GHCR tags API (returns JSON with tags):
curl -H "Authorization: Bearer <GITHUB_PAT>" \
https://ghcr.io/v2/context-labs/filesystem-mcp/tags/list[1]
- ghcrctl (lists versions + which tags point at each digest):
ghcrctl list versions context-labs/filesystem-mcp[2]
If you don’t have access to that org/package, you won’t be able to enumerate tags; in that case consider the public “filesystem” MCP images published elsewhere (e.g., Docker Hub mcp/filesystem, etc.). [3]
Sources: [1] [2] [3]
🏁 Script executed:
# Read the sibling cmd files to see their image tags
echo "=== 00a (filesystem) ===" && cat docs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.cmd.txt
echo -e "\n=== 00b (playwright) ===" && cat docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.cmd.txt
echo -e "\n=== 00c (desktop-commander) ===" && cat docs/uat/evidence/2026-02-24-rerun-1/00c-ghcr-pull-desktop-commander.cmd.txtRepository: clduab11/codex-synaptic
Length of output: 318
🏁 Script executed:
# Also check if there are any version or tag references in the project
rg "filesystem-mcp|playwright|desktop-commander" --type md -A 2 -B 2 | head -50Repository: clduab11/codex-synaptic
Length of output: 3291
Pin container images to specific versions or digests in all three UAT evidence files.
All three evidence file commands—00a, 00b, and 00c—currently use :latest tags. While this ensures pulling the most recent build, it undermines reproducibility: the same command run days or weeks later could pull a different image, making the recorded evidence non-deterministic and harder to correlate with specific test conditions.
For robust UAT records that support your goal of deterministic MCP launch paths, either:
- Pin to a stable semantic version tag (e.g.,
:v1.2.3), or - Pin to the specific digest (e.g.,
@sha256:abc123...) that was verified
To capture the resolved digest, run the pull and then inspect:
docker pull ghcr.io/context-labs/filesystem-mcp:latest
docker inspect --format='{{index .RepoDigests 0}}' ghcr.io/context-labs/filesystem-mcp:latestThen update all three *.cmd.txt files to use the same pinned reference for consistency. This ensures future test runs reference the exact image layers that passed validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/evidence/2026-02-24-rerun-1/00a-ghcr-pull-filesystem.cmd.txt` at
line 1, The three UAT evidence files (00a-ghcr-pull-filesystem.cmd.txt, 00b-...,
00c-...) currently use the non-deterministic :latest tag; replace :latest with a
pinned reference (preferably the resolved digest like `@sha256`:...) or a stable
semver tag across all three files to ensure reproducibility; to obtain the
digest run docker pull ghcr.io/context-labs/filesystem-mcp:latest and then
docker inspect --format='{{index .RepoDigests 0}}'
ghcr.io/context-labs/filesystem-mcp:latest and update each file so they all use
the same pinned image reference.
| @@ -0,0 +1 @@ | |||
| Error response from daemon: failed to resolve reference "ghcr.io/context-labs/playwright-mcp:latest": ghcr.io/context-labs/playwright-mcp:latest: not found | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ghcr.io context-labs playwright-mcp Docker image
💡 Result:
I couldn’t find any public GitHub Container Registry image published as ghcr.io/context-labs/playwright-mcp. The context-labs GitHub org’s Packages page shows “0 packages / No results matched your search” for playwright-mcp, which strongly suggests the image doesn’t exist publicly (or is private). [1]
If your goal is a Playwright MCP server in Docker, common public options are:
- Run Playwright MCP via npm (no Docker):
npx@playwright/mcp@latest(official Playwright MCP server). [2] - Use Playwright’s official Docker images (for Playwright + browsers/deps): e.g.
mcr.microsoft.com/playwright:<tag>(then install/run what you need inside). [3]
Next check: try docker pull ghcr.io/context-labs/playwright-mcp locally—if it exists but is private, GHCR will return an auth/permission error rather than “not found”. [1]
Sources: [1] [2] [3]
🌐 Web query:
playwright-mcp Docker image GHCR registry
💡 Result:
-
Official
microsoft/playwright-mcpDocker image: it’s published on Microsoft Container Registry (MCR), not GHCR:- Image:
mcr.microsoft.com/playwright/mcp(example usage shown in the repo README) [1]
- Image:
-
If you specifically need a GHCR-hosted Playwright MCP image, those are currently community images, e.g.:
ghcr.io/bradsjm/chrome-mcp:latest[2]ghcr.io/aakashh242/mcp-playwright:latest[3]
Sources: [1] [2] [3]
🌐 Web query:
context-labs playwright Docker images published
💡 Result:
I couldn’t find any evidence that context-labs publishes a Playwright Docker image (on Docker Hub or GHCR) under a contextlabs/*playwright* / context-labs/*playwright* name.
What I did find is that context-labs publishes at least this Docker image (not Playwright-related): docker.io/contextlabs/mcp-database:latest. [1]
If what you need is an official Playwright base image, Playwright publishes theirs as mcr.microsoft.com/playwright (versioned tags like v1.58.2-noble). [2]
Sources:
[1] MCP listing showing docker.io/contextlabs/mcp-database:latest
[2] Playwright official Docker docs (image name + tags)
Fix the GHCR image reference for playwright-mcp — this blocks UAT.
The captured error is legitimate: ghcr.io/context-labs/playwright-mcp:latest does not exist in the registry. The image has never been published to context-labs' GHCR namespace.
Before proceeding, clarify which approach this PR intends:
- Use the official Playwright MCP image — switch the reference to
mcr.microsoft.com/playwright/mcp(published on Microsoft Container Registry, not GHCR). - Publish a context-labs variant — build and push the image to
ghcr.io/context-labs/playwright-mcp(or another GHCR location) before UAT can pass.
Until this is resolved, any UAT scenario exercising the playwright-mcp service will fail at the pull step. Update the tracker to reflect this as a hard blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/evidence/2026-02-24-rerun-1/00b-ghcr-pull-playwright.out.txt` at
line 1, The UAT failure is caused by an invalid image reference
ghcr.io/context-labs/playwright-mcp:latest; either switch the image reference
wherever it’s used (e.g., in the UAT/runtime config or docker-compose/service
definition that refers to "playwright-mcp") to the official image
mcr.microsoft.com/playwright/mcp, or build+push a context-labs image to
ghcr.io/context-labs/playwright-mcp and update the reference to that pushed tag;
after choosing, update the UAT tracker to mark the missing image as resolved or
the blocker as still open if you intend to publish later.
Comment on lines
+5
to
+23
| "id": "repo.preflight", | ||
| "ok": true, | ||
| "details": "Found /Users/chrisdukes/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed." | ||
| }, | ||
| { | ||
| "id": "codex.auth", | ||
| "ok": true, | ||
| "details": "Logged in using ChatGPT" | ||
| }, | ||
| { | ||
| "id": "runtime.daemon", | ||
| "ok": true, | ||
| "details": "Background daemon already running (pid 64145)." | ||
| }, | ||
| { | ||
| "id": "mcp.up", | ||
| "ok": false, | ||
| "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker compose startup failed for mcp-filesystem (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Verify Docker is running, then retry. Raw docker output: time=\"2026-02-24T09:03:04-06:00\" level=warning msg=\"/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error failed to resolve reference \"ghcr.io/context-labs/filesystem-mcp:latest\": ghcr.io/context-labs/filesystem-mcp:latest: not found\nError respon…", | ||
| "remediation": "codex-synaptic env docker-login mcp-filesystem && codex-synaptic env up mcp-filesystem && codex-synaptic env codex-register mcp-filesystem --replace && codex-synaptic env status mcp-filesystem", |
Contributor
There was a problem hiding this comment.
Redact local user paths from committed evidence.
The absolute paths expose a local username. Replace with a placeholder like <REPO_ROOT> to avoid PII leakage in shared artifacts.
🛡️ Suggested redaction
- "details": "Found /Users/chrisdukes/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed."
+ "details": "Found <REPO_ROOT>/dist/cli/index.js; CLI executable check passed."
...
- "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker compose startup failed for mcp-filesystem (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Verify Docker is running, then retry. Raw docker output: time=\"2026-02-24T09:03:04-06:00\" level=warning msg=\"/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error failed to resolve reference \"ghcr.io/context-labs/filesystem-mcp:latest\": ghcr.io/context-labs/filesystem-mcp:latest: not found\nError respon…",
+ "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker compose startup failed for mcp-filesystem (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Verify Docker is running, then retry. Raw docker output: time=\"2026-02-24T09:03:04-06:00\" level=warning msg=\"<REPO_ROOT>/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error failed to resolve reference \"ghcr.io/context-labs/filesystem-mcp:latest\": ghcr.io/context-labs/filesystem-mcp:latest: not found\nError respon…",📝 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
| "id": "repo.preflight", | |
| "ok": true, | |
| "details": "Found /Users/chrisdukes/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed." | |
| }, | |
| { | |
| "id": "codex.auth", | |
| "ok": true, | |
| "details": "Logged in using ChatGPT" | |
| }, | |
| { | |
| "id": "runtime.daemon", | |
| "ok": true, | |
| "details": "Background daemon already running (pid 64145)." | |
| }, | |
| { | |
| "id": "mcp.up", | |
| "ok": false, | |
| "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker compose startup failed for mcp-filesystem (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Verify Docker is running, then retry. Raw docker output: time=\"2026-02-24T09:03:04-06:00\" level=warning msg=\"/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error failed to resolve reference \"ghcr.io/context-labs/filesystem-mcp:latest\": ghcr.io/context-labs/filesystem-mcp:latest: not found\nError respon…", | |
| "remediation": "codex-synaptic env docker-login mcp-filesystem && codex-synaptic env up mcp-filesystem && codex-synaptic env codex-register mcp-filesystem --replace && codex-synaptic env status mcp-filesystem", | |
| "id": "repo.preflight", | |
| "ok": true, | |
| "details": "Found <REPO_ROOT>/dist/cli/index.js; CLI executable check passed." | |
| }, | |
| { | |
| "id": "codex.auth", | |
| "ok": true, | |
| "details": "Logged in using ChatGPT" | |
| }, | |
| { | |
| "id": "runtime.daemon", | |
| "ok": true, | |
| "details": "Background daemon already running (pid 64145)." | |
| }, | |
| { | |
| "id": "mcp.up", | |
| "ok": false, | |
| "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker compose startup failed for mcp-filesystem (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Verify Docker is running, then retry. Raw docker output: time=\"2026-02-24T09:03:04-06:00\" level=warning msg=\"<REPO_ROOT>/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error failed to resolve reference \"ghcr.io/context-labs/filesystem-mcp:latest\": ghcr.io/context-labs/filesystem-mcp:latest: not found\nError respon…", | |
| "remediation": "codex-synaptic env docker-login mcp-filesystem && codex-synaptic env up mcp-filesystem && codex-synaptic env codex-register mcp-filesystem --replace && codex-synaptic env status mcp-filesystem", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/evidence/2026-02-24-rerun-1/launch.strict.json` around lines 5 - 23,
The JSON evidence contains absolute local paths (e.g.,
"/Users/chrisdukes/LocalProjects/codex-synaptic/..." inside the "details" values
for entries such as id "repo.preflight" and the raw docker output under id
"mcp.up"); update the file launch.strict.json to redact any absolute user/home
paths by replacing them with a placeholder like "<REPO_ROOT>" (or another agreed
placeholder) for all "details" fields and any other string values that match
"/Users/<username>/*" or similar patterns so PII is removed while preserving the
rest of the message.
Comment on lines
+3
to
+5
| 2026-02-24T14:27:01.165Z ✨ INFO [env] Starting service mcp-filesystem ~ all green—smooth sailing ✨ | data: command=docker compose -f docker/mcp/docker-c… | ||
| ❌ env.up failed: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time="2026-02-24T08:27:01-06:00" level=warning msg="/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion" | ||
| Image ghcr.io/context-labs/filesystem-mcp:latest Pulling |
Contributor
There was a problem hiding this comment.
Redact local user paths from evidence logs.
The docker-compose warning embeds an absolute path with a username. Replace with a neutral placeholder (<REPO_ROOT>) before committing.
🛡️ Suggested redaction
-❌ env.up failed: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time="2026-02-24T08:27:01-06:00" level=warning msg="/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion"
+❌ env.up failed: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time="2026-02-24T08:27:01-06:00" level=warning msg="<REPO_ROOT>/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion"📝 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
| 2026-02-24T14:27:01.165Z ✨ INFO [env] Starting service mcp-filesystem ~ all green—smooth sailing ✨ | data: command=docker compose -f docker/mcp/docker-c… | |
| ❌ env.up failed: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time="2026-02-24T08:27:01-06:00" level=warning msg="/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion" | |
| Image ghcr.io/context-labs/filesystem-mcp:latest Pulling | |
| 2026-02-24T14:27:01.165Z ✨ INFO [env] Starting service mcp-filesystem ~ all green—smooth sailing ✨ | data: command=docker compose -f docker/mcp/docker-c… | |
| ❌ env.up failed: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time="2026-02-24T08:27:01-06:00" level=warning msg="<REPO_ROOT>/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion" | |
| Image ghcr.io/context-labs/filesystem-mcp:latest Pulling |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/evidence/2026-02-24/08-env-up.out.txt` around lines 3 - 5, The
evidence log contains a local absolute path string like
"/Users/chrisdukes/LocalProjects/codex-synaptic/..." that must be redacted; open
the failing evidence file (the shown output) and replace all occurrences of that
local path prefix with the neutral placeholder "<REPO_ROOT>" (or run a single
search-and-replace for the pattern
"/Users/{username}/LocalProjects/codex-synaptic" -> "<REPO_ROOT>") so the
docker-compose warning and any other entries no longer include user-specific
filesystem paths before committing.
Comment on lines
+1
to
+125
| [ | ||
| { | ||
| "name": "desktop-commander", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "http://localhost:7070", | ||
| "bearer_token_env_var": null, | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "unsupported" | ||
| }, | ||
| { | ||
| "name": "figma", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "https://mcp.figma.com/mcp", | ||
| "bearer_token_env_var": null, | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "o_auth" | ||
| }, | ||
| { | ||
| "name": "filesystem-local", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "http://localhost:7040", | ||
| "bearer_token_env_var": null, | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "unsupported" | ||
| }, | ||
| { | ||
| "name": "linear", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "https://mcp.linear.app/mcp", | ||
| "bearer_token_env_var": "REDACTED_LOCAL_SECRET", | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "bearer_token" | ||
| }, | ||
| { | ||
| "name": "notion", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "https://mcp.notion.com/mcp", | ||
| "bearer_token_env_var": null, | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "o_auth" | ||
| }, | ||
| { | ||
| "name": "openaiDeveloperDocs", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "https://developers.openai.com/mcp", | ||
| "bearer_token_env_var": null, | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "unsupported" | ||
| }, | ||
| { | ||
| "name": "playwright", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": [ | ||
| "@playwright/mcp@latest" | ||
| ], | ||
| "env": null, | ||
| "env_vars": [], | ||
| "cwd": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "unsupported" | ||
| }, | ||
| { | ||
| "name": "playwright-local", | ||
| "enabled": true, | ||
| "disabled_reason": null, | ||
| "transport": { | ||
| "type": "streamable_http", | ||
| "url": "http://localhost:7030", | ||
| "bearer_token_env_var": null, | ||
| "http_headers": null, | ||
| "env_http_headers": null | ||
| }, | ||
| "startup_timeout_sec": null, | ||
| "tool_timeout_sec": null, | ||
| "auth_status": "unsupported" | ||
| } | ||
| ] |
Contributor
There was a problem hiding this comment.
JSON file doesn’t meet the required manifest/topology/consensus schema.
The current JSON is an evidence snapshot, but the repository guideline for **/*.{yaml,yml,json} requires agent specs, topology definitions, and consensus configurations. If this is intended as raw evidence, consider renaming to a non-JSON extension (e.g., .txt or .json.txt) or moving it to a path excluded by that rule; otherwise, add the required manifest sections.
As per coding guidelines: “**/*.{yaml,yml,json}: Define agent specifications in YAML/JSON Agent Manifests including resource requirements, capability declarations, network/security policies, and deployment/scaling rules… Define network topology specifications… Define consensus mechanism parameters…”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/evidence/2026-02-24/codex.mcp.list.json` around lines 1 - 125, This
JSON is an evidence-only snapshot (root is an array of tool objects with "name"
entries like "desktop-commander", "figma", "filesystem-local", etc.) but does
not include the required agent manifest/topology/consensus sections; either
move/rename this file to a non-JSON manifest path (e.g., change extension to
.txt or put under an excluded evidence folder) or convert it into a proper Agent
Manifest by adding the required top-level sections: an "agents" (agent
specifications with resource requirements, capabilities, and deployment/scaling
rules), a "topology" (network topology and security policies referencing the
existing tool entries by name), and a "consensus" object (consensus mechanism
parameters and policies); update the document to include those keys and ensure
each tool object (e.g., entries with "name":"desktop-commander", "figma", etc.)
is referenced appropriately in the new "agents" and "topology" sections.
Comment on lines
+5
to
+27
| "id": "repo.preflight", | ||
| "ok": true, | ||
| "details": "Found /Users/chrisdukes/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed." | ||
| }, | ||
| { | ||
| "id": "codex.auth", | ||
| "ok": true, | ||
| "details": "Logged in using ChatGPT" | ||
| }, | ||
| { | ||
| "id": "runtime.daemon", | ||
| "ok": true, | ||
| "details": "Background daemon already running (pid 64145)." | ||
| }, | ||
| { | ||
| "id": "mcp.up", | ||
| "ok": false, | ||
| "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time=\"2026-02-24T08:27:06-06:00\" level=warning msg=\"/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error error from registry: denied\ndenied\nError response from daemon: error from registry: denied\ndenied", | ||
| "remediation": "codex-synaptic env docker-login mcp-filesystem && codex-synaptic env up mcp-filesystem && codex-synaptic env codex-register mcp-filesystem --replace && codex-synaptic env status mcp-filesystem", | ||
| "metadata": { | ||
| "failedProfile": "mcp-filesystem", | ||
| "startedProfiles": [] | ||
| } |
Contributor
There was a problem hiding this comment.
Redact local user paths from evidence artifacts.
The payload includes a full local path and username, which can leak PII when shared externally. Please sanitize these strings before committing evidence.
🔒 Suggested redaction
- "details": "Found /Users/chrisdukes/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed."
+ "details": "Found /Users/<redacted>/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed."
...
- "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time=\"2026-02-24T08:27:06-06:00\" level=warning msg=\"/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error error from registry: denied\ndenied\nError response from daemon: error from registry: denied\ndenied",
+ "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time=\"2026-02-24T08:27:06-06:00\" level=warning msg=\"/Users/<redacted>/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error error from registry: denied\ndenied\nError response from daemon: error from registry: denied\ndenied",📝 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
| "id": "repo.preflight", | |
| "ok": true, | |
| "details": "Found /Users/chrisdukes/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed." | |
| }, | |
| { | |
| "id": "codex.auth", | |
| "ok": true, | |
| "details": "Logged in using ChatGPT" | |
| }, | |
| { | |
| "id": "runtime.daemon", | |
| "ok": true, | |
| "details": "Background daemon already running (pid 64145)." | |
| }, | |
| { | |
| "id": "mcp.up", | |
| "ok": false, | |
| "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time=\"2026-02-24T08:27:06-06:00\" level=warning msg=\"/Users/chrisdukes/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error error from registry: denied\ndenied\nError response from daemon: error from registry: denied\ndenied", | |
| "remediation": "codex-synaptic env docker-login mcp-filesystem && codex-synaptic env up mcp-filesystem && codex-synaptic env codex-register mcp-filesystem --replace && codex-synaptic env status mcp-filesystem", | |
| "metadata": { | |
| "failedProfile": "mcp-filesystem", | |
| "startedProfiles": [] | |
| } | |
| "id": "repo.preflight", | |
| "ok": true, | |
| "details": "Found /Users/<redacted>/LocalProjects/codex-synaptic/dist/cli/index.js; CLI executable check passed." | |
| }, | |
| { | |
| "id": "codex.auth", | |
| "ok": true, | |
| "details": "Logged in using ChatGPT" | |
| }, | |
| { | |
| "id": "runtime.daemon", | |
| "ok": true, | |
| "details": "Background daemon already running (pid 64145)." | |
| }, | |
| { | |
| "id": "mcp.up", | |
| "ok": false, | |
| "details": "Failed to start MCP profile mcp-filesystem after starting 0/3: Docker image pull/auth denied for mcp-filesystem (ghcr.io/context-labs/filesystem-mcp:latest) (exit=1, compose=docker compose -f docker/mcp/docker-compose.filesystem.yml up -d mcp-filesystem). Run `codex-synaptic env docker-login mcp-filesystem` and retry `codex-synaptic env up mcp-filesystem`. Raw docker output: time=\"2026-02-24T08:27:06-06:00\" level=warning msg=\"/Users/<redacted>/LocalProjects/codex-synaptic/docker/mcp/docker-compose.filesystem.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion\"\n Image ghcr.io/context-labs/filesystem-mcp:latest Pulling \n Image ghcr.io/context-labs/filesystem-mcp:latest Error error from registry: denied\ndenied\nError response from daemon: error from registry: denied\ndenied", | |
| "remediation": "codex-synaptic env docker-login mcp-filesystem && codex-synaptic env up mcp-filesystem && codex-synaptic env codex-register mcp-filesystem --replace && codex-synaptic env status mcp-filesystem", | |
| "metadata": { | |
| "failedProfile": "mcp-filesystem", | |
| "startedProfiles": [] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/evidence/2026-02-24/launch.strict.payload.json` around lines 5 - 27,
The JSON evidence contains raw local paths/usernames in "details" fields (e.g.,
the repo.preflight "Found /Users/chrisdukes/LocalProjects/..." string and mcp.up
raw docker output), so update the evidence-generation/sanitization step to
redact local home paths and usernames before saving; specifically modify the
code that writes these "details" entries (search for where "repo.preflight" and
"mcp.up" payload objects are composed) to run a sanitization function that
replaces patterns like ^/Users/[^/]+(/|$) or OS home directory (os.homedir())
occurrences with a placeholder such as "<REDACTED_PATH>" or "<REDACTED_USER>",
and apply the same regex to any multi-line logs captured for mcp.up so no PII
(usernames or absolute local paths) are stored in the JSON evidence.
Comment on lines
+24
to
+32
| Primary rerun evidence (authoritative for this report): | ||
|
|
||
| - `/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/` | ||
| - Step matrix: `/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/_status.tsv` | ||
|
|
||
| Prior blocked run (preserved for traceability): | ||
|
|
||
| - `/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24/` | ||
|
|
Contributor
There was a problem hiding this comment.
Use repo‑relative paths for portability.
The /Users/... paths are local to one machine and make the report harder to reuse in CI or by other operators. Prefer repo‑relative paths throughout the report.
✍️ Example cleanup
-- /Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/
+- docs/uat/evidence/2026-02-24-rerun-1/
@@
-- /Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24/
+- docs/uat/evidence/2026-02-24/
@@
-- /Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/launch.strict.json
+- docs/uat/evidence/2026-02-24-rerun-1/launch.strict.jsonAlso applies to: 77-85, 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/uat/UAT_FINAL_REPORT.md` around lines 24 - 32, Replace machine-specific
absolute paths like
`/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/`
and
`/Users/chrisdukes/LocalProjects/codex-synaptic/docs/uat/evidence/2026-02-24-rerun-1/_status.tsv`
with repo-relative paths (e.g., `docs/uat/evidence/2026-02-24-rerun-1/` and
`docs/uat/evidence/2026-02-24-rerun-1/_status.tsv`) throughout
UAT_FINAL_REPORT.md, and apply the same replacement for the other occurrences
referenced in the comment (the sections around the other listed blocks).
| "CHANGELOG.md", | ||
| "LICENSE", | ||
| "docs/codex-synaptic-cheat-codes.md", | ||
| ".codex-improvement/SCHEMA_MASTER.yaml" |
Contributor
There was a problem hiding this comment.
Verify whether .codex-improvement/SCHEMA_MASTER.yaml should be published.
The .codex-improvement/ directory's dot prefix strongly signals internal tooling. Including it in the files manifest will publish this schema artifact to all npm consumers. If it's only used for local development or internal repo tooling, it should be removed from this list.
🛡️ Suggested fix if this is internal-only
- ".codex-improvement/SCHEMA_MASTER.yaml"📝 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
| ".codex-improvement/SCHEMA_MASTER.yaml" | |
| "files": [ | |
| "dist/", | |
| "docker/", | |
| "config/", | |
| ".env.example", | |
| "README.md", | |
| "AGENTS.md", | |
| "CHANGELOG.md", | |
| "LICENSE", | |
| "docs/codex-synaptic-cheat-codes.md" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 19, The package.json "files" manifest currently
includes the entry ".codex-improvement/SCHEMA_MASTER.yaml"; verify whether that
schema is intended to be published to npm and if it is internal-only remove it
from the "files" array (or relocate it outside the package root) so it won't be
packed, or alternatively keep it but add an explicit exclusion (e.g., move to
.npmignore or a non-dot directory) to prevent accidental publishing; update the
package.json "files" array to remove ".codex-improvement/SCHEMA_MASTER.yaml" if
it's internal and commit the change.
Comment on lines
+4376
to
+4402
| envCmd | ||
| .command('docker-login') | ||
| .description('Authenticate Docker registries required by one or more service profiles') | ||
| .argument('[names...]', 'Service profile names (defaults to launch gate profiles)') | ||
| .option('--dry-run', 'Print docker login commands without executing them') | ||
| .action(handleCommand('env.docker-login', async (names: string[] = [], options) => { | ||
| const targets = names.length ? names : [...DEFAULT_MCP_PROFILES]; | ||
| const registries = serviceManager.registriesForProfiles(targets); | ||
|
|
||
| if (!registries.length) { | ||
| console.log(chalk.gray(`No registry authentication required for profiles: ${targets.join(', ')}`)); | ||
| return; | ||
| } | ||
|
|
||
| if (options.dryRun) { | ||
| console.log(chalk.blue('Docker registry login commands (dry-run):')); | ||
| registries.forEach((registry) => { | ||
| console.log(chalk.gray(` docker login ${registry}`)); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| for (const registry of registries) { | ||
| serviceManager.dockerLogin(registry); | ||
| console.log(chalk.green(`✅ Docker auth completed for ${registry}`)); | ||
| } | ||
| })); |
Contributor
There was a problem hiding this comment.
Validate profile names before computing registry logins.
If a caller passes an unknown profile, registriesForProfiles can return empty and we log “No registry authentication required,” which is misleading. Consider validating against known profiles and throwing a friendly error listing valid options.
🛡️ Suggested guard
.action(handleCommand('env.docker-login', async (names: string[] = [], options) => {
const targets = names.length ? names : [...DEFAULT_MCP_PROFILES];
+ const knownProfiles = new Set(serviceManager.listProfiles().map(({ name }) => name));
+ const unknown = targets.filter((name) => !knownProfiles.has(name));
+ if (unknown.length) {
+ throw new Error(`Unknown service profile(s): ${unknown.join(', ')}. Run "codex-synaptic env list" to see valid profiles.`);
+ }
const registries = serviceManager.registriesForProfiles(targets);As per coding guidelines, "Implement proper input validation and sanitization".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/index.ts` around lines 4376 - 4402, Validate incoming profile names
before calling serviceManager.registriesForProfiles: if names were provided,
check each against the known profiles list (e.g., DEFAULT_MCP_PROFILES or
serviceManager.availableProfiles / similar source) and if any are unknown throw
a user-friendly error listing the valid profile names; do this in the envCmd
.action handler (before computing registries) so that when registriesForProfiles
returns empty it's only legitimate for valid profiles, and keep the existing
dry-run and dockerLogin logic unchanged (references:
envCmd.command('docker-login'), DEFAULT_MCP_PROFILES, registriesForProfiles,
serviceManager.dockerLogin).
Comment on lines
+412
to
+424
| doctorReport = await executeDoctor( | ||
| { | ||
| cwd, | ||
| mcpProfiles: profileNames, | ||
| skipCodexAuth: Boolean(options.skipCodexAuth) | ||
| }, | ||
| { | ||
| fileExists, | ||
| spawnCommand, | ||
| getServiceStatus: deps.getServiceStatus, | ||
| getCodexRegistration: deps.getCodexRegistration | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 450 src/cli/launch.ts | tail -n 50Repository: clduab11/codex-synaptic
Length of output: 1228
🏁 Script executed:
sed -n '400,440p' src/cli/launch.tsRepository: clduab11/codex-synaptic
Length of output: 1039
🏁 Script executed:
rg -n "executeDoctor" src/cli/launch.ts -B 2 -A 15Repository: clduab11/codex-synaptic
Length of output: 1224
🏁 Script executed:
rg -n "try\|catch" src/cli/launch.ts | head -20Repository: clduab11/codex-synaptic
Length of output: 49
🏁 Script executed:
rg -n "LaunchStep\|appendStep\|buildLaunchReport" src/cli/launch.ts -B 2 -A 3Repository: clduab11/codex-synaptic
Length of output: 49
Guard executeDoctor so launch always returns a structured report.
If executeDoctor throws (e.g., service status error), the entire runLaunch function rejects and callers receive an error instead of a deterministic JSON report. Wrap the call in a try-catch block, emit a failed doctor.strict step on error, and return a structured LaunchReport. This aligns with the coding guideline "Always handle async operations with proper error catching" and existing error-handling patterns in the function.
Suggested guard
+ try {
doctorReport = await executeDoctor(
{
cwd,
mcpProfiles: profileNames,
skipCodexAuth: Boolean(options.skipCodexAuth)
},
{
fileExists,
spawnCommand,
getServiceStatus: deps.getServiceStatus,
getCodexRegistration: deps.getCodexRegistration
}
);
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ const doctorStep: LaunchStep = {
+ id: 'doctor.strict',
+ ok: false,
+ details: `Doctor execution failed: ${message}`,
+ remediation: 'Run `codex-synaptic doctor --strict --json` to inspect failing checks.'
+ };
+ const stop = appendStep(doctorStep);
+ return stop ?? buildLaunchReport(steps, doctorReport);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` around lines 412 - 424, Wrap the call to executeDoctor
inside runLaunch in a try-catch so failures produce a deterministic
LaunchReport: call executeDoctor(...) in try, assign doctorReport on success,
and in catch create a doctorReport that includes a failed "doctor.strict" step
with the caught error message/details; ensure runLaunch then returns a
structured LaunchReport containing that doctorReport instead of letting the
exception bubble. Reference executeDoctor, runLaunch, doctorReport, and the
"doctor.strict" step when making the change.
…Sync pipe Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Fix ENOBUFS in `ensureService` during Docker image pulls
Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b130961281
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+180
to
+182
| ok, | ||
| details: stdout || loginStatus.stderr?.trim() || 'No output', | ||
| remediation: ok ? undefined : 'Run `codex login` then re-run `codex login status`.' |
There was a problem hiding this comment.
When codex is not installed, spawnSync returns status = null with empty stdout/stderr (ENOENT is on the error field), so this gate reports No output and recommends codex login, which cannot succeed in that environment. This misroutes first-run remediation for users on fresh machines; please branch on missing-command results and return an install-focused fix instead of an auth-focused one.
Useful? React with 👍 / 👎.
Comment on lines
+416
to
+419
| if (/pull access denied|requested access to the resource is denied|insufficient_scope|unauthorized|authentication required|error from registry:\s*denied/i.test(output)) { | ||
| diagnosis = `Docker image pull/auth denied for ${name} (${images})`; | ||
| remediation = `Run \`codex-synaptic env docker-login ${name}\` and retry \`codex-synaptic env up ${name}\`.`; | ||
| } else if (/Cannot connect to the Docker daemon|Is the docker daemon running/i.test(output)) { |
There was a problem hiding this comment.
The compose error classifier only matches auth-denied/daemon/CLI patterns, so failures like failed to resolve reference ... not found fall through to the default Verify Docker is running remediation even when Docker is healthy and the image tag/repository is wrong. In that case users are directed to the wrong recovery path; add a dedicated not found/manifest unknown classification so launch guidance points to fixing image coordinates instead.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/env/service-manager.ts (2)
395-433:⚠️ Potential issue | 🟠 Major
wrapComposeStartErrorstill returns a plainError— addressed in a prior review but not yet resolved.Returning a generic
Errorprevents callers from programmatically distinguishing auth failures, daemon failures, and CLI-missing failures without string parsing. A typedCodexSynapticError(or equivalent project error class) with a stable code and structured context (images, exitStatus, profile, compose command) would allow clean upstream handling.🎯 Proposed typed error return
+import { CodexSynapticError, ErrorCode } from '../core/errors.js'; - private wrapComposeStartError(name: string, profile: ServiceProfile, cmd: string, error: unknown): Error { + private wrapComposeStartError(name: string, profile: ServiceProfile, cmd: string, error: unknown): CodexSynapticError { // ... existing classification logic ... - return new Error( - `${diagnosis} (exit=${exitLabel}, compose=${cmd}). ${remediation} Raw docker output: ${truncatedOutput}` - ); + return new CodexSynapticError( + ErrorCode.MCP_ERROR, + `${diagnosis} (exit=${exitLabel}, compose=${cmd}). ${remediation}`, + { profile: name, images, composeCmd: cmd, exitStatus, output: truncatedOutput }, + false + ); }As per coding guidelines: "Throw specific error types, not generic Error objects."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 395 - 433, wrapComposeStartError currently returns a generic Error which prevents callers from distinguishing failure types; change it to construct and return (or throw) a project-specific error (e.g., CodexSynapticError or a new DockerComposeError) that includes a stable error code (like 'docker.auth', 'docker.daemon', 'docker.cli', or 'docker.unknown') and a structured context object containing images, exitStatus, profile, composeCmd (cmd) and the raw/truncated output; keep the human-readable message similar to the current string for logs, import or implement the typed error class if missing, and replace the final "new Error(...)" with "new CodexSynapticError(code, message, { images, exitStatus, profile, composeCmd, output })" (or appropriate constructor shape) so callers can programmatically inspect error.code and error.context while preserving the message for display.
325-333:⚠️ Potential issue | 🟠 Major
dockerLoginstill blocks the event loop and is vulnerable to shell injection — addressed in a prior review but not yet fixed.The method interpolates
registrydirectly into a shell command string (docker login ${normalized}) and then runs it viaexecSync, violating both the "Don't block the event loop" guideline and safe shell usage. Whilenormalizedis trimmed, a value likeghcr.io; rm -rf /would still be interpreted by the shell.🔒 Proposed async, injection-safe fix
- dockerLogin(registry: string): void { + async dockerLogin(registry: string): Promise<void> { const normalized = registry.trim(); if (!normalized) { throw new Error('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' }); + await new Promise<void>((resolve, reject) => { + const child = spawn('docker', ['login', normalized], { stdio: 'inherit' }); + child.on('error', reject); + child.on('close', (code) => { + code === 0 ? resolve() : reject(new Error(`docker login exited with ${code ?? 'unknown'}`)); + }); + }); }Remember to update the caller in
src/cli/index.tstoawait dockerLogin(...)after this change.As per coding guidelines: "Don't block the event loop with synchronous operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 325 - 333, The dockerLogin function currently constructs a shell string and calls execSync, which blocks the event loop and allows shell injection; change dockerLogin(registry: string) to an async function that trims and validates registry, then invokes the docker binary directly without a shell using child_process.execFile or spawn (e.g., execFile('docker', ['login', normalized'], { stdio: 'inherit' }) wrapped in a Promise or using util.promisify) so input is passed as an argument (no interpolation) and execution is non-blocking; ensure errors from execFile are propagated (throw on non-zero exit) and update any callers to await dockerLogin(...).
🧹 Nitpick comments (2)
src/env/service-manager.ts (2)
363-393: Preferspawnwith an args array overshell: trueforrunComposeUp.Using
shell: truewith a full command string passes the command through/bin/sh, which adds an unnecessary shell layer. While the currentcmdcontent is built entirely from hardcodedPROFILESconstants (low injection risk today), splitting into['docker', 'compose', '-f', ...]args is more defensive and aligns with thedockerLoginfix direction. This could be done by refactoringcomposeCommand()to return{ bin: string; args: string[] }instead of a flat string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 363 - 393, The runComposeUp function currently spawns a shell with a single command string, which is unnecessary and risky; refactor composeCommand() to return a structured object like { bin: string, args: string[] } (or similar named type) and update runComposeUp to call spawn(bin, args, { shell: false, stdio: ['ignore','inherit','pipe'], env }) instead of spawn(cmd, { shell: true, ... }); update any callers of composeCommand()/runComposeUp to use the new shape, preserve collection of stderr via proc.stderr?.on('data', ...) and the existing close/error handling, and remove shell: true usage in runComposeUp.
78-132: Consider pinning Docker image tags beyond:latestfor reproducibility.All profiles use
:latestfloating tags. A newer image push can silently change behavior or break the service without a code change. Pinning to a specific version tag (e.g.,ghcr.io/context-labs/github-mcp:v1.2.3) or an immutable digest (e.g.,@sha256:...) would give reproducible, auditable deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 78 - 132, Several MCP profiles (e.g., keys 'mcp-context7', 'mcp-playwright', 'mcp-filesystem', 'mcp-desktop-commander', 'mcp-tavily', 'mcp-firecrawl', 'github-mcp' entry) currently use floating docker image tags in the dockerImages property (":latest"); change each dockerImages entry to a specific version tag or immutable digest (e.g., replace "ghcr.io/...:latest" with a stable "ghcr.io/...:vX.Y.Z" or "@sha256:...") to ensure reproducible deployments, and update any documentation/configuration that references these images; optionally add a comment or a configuration mechanism to allow overriding pins for dev/local runs.
🤖 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/env/service-manager.ts`:
- Around line 363-393: The runComposeUp promise can hang indefinitely if the
spawned "docker compose up" process blocks; update runComposeUp to add a
process-level timeout (e.g., a configurable constant or parameter) that starts a
timer when the process is spawned, and on timeout kills the child process
(proc.kill()), removes listeners, and rejects the Promise with a descriptive
timeout Error (include process status/null, stdout '', and stderr/message);
ensure the normal close/error handlers clear the timer so you don't
double-resolve/reject. Target the runComposeUp function and its proc/error/close
handlers when implementing this change (ensureService will then no longer be
blocked by a hung compose process).
---
Duplicate comments:
In `@src/env/service-manager.ts`:
- Around line 395-433: wrapComposeStartError currently returns a generic Error
which prevents callers from distinguishing failure types; change it to construct
and return (or throw) a project-specific error (e.g., CodexSynapticError or a
new DockerComposeError) that includes a stable error code (like 'docker.auth',
'docker.daemon', 'docker.cli', or 'docker.unknown') and a structured context
object containing images, exitStatus, profile, composeCmd (cmd) and the
raw/truncated output; keep the human-readable message similar to the current
string for logs, import or implement the typed error class if missing, and
replace the final "new Error(...)" with "new CodexSynapticError(code, message, {
images, exitStatus, profile, composeCmd, output })" (or appropriate constructor
shape) so callers can programmatically inspect error.code and error.context
while preserving the message for display.
- Around line 325-333: The dockerLogin function currently constructs a shell
string and calls execSync, which blocks the event loop and allows shell
injection; change dockerLogin(registry: string) to an async function that trims
and validates registry, then invokes the docker binary directly without a shell
using child_process.execFile or spawn (e.g., execFile('docker', ['login',
normalized'], { stdio: 'inherit' }) wrapped in a Promise or using
util.promisify) so input is passed as an argument (no interpolation) and
execution is non-blocking; ensure errors from execFile are propagated (throw on
non-zero exit) and update any callers to await dockerLogin(...).
---
Nitpick comments:
In `@src/env/service-manager.ts`:
- Around line 363-393: The runComposeUp function currently spawns a shell with a
single command string, which is unnecessary and risky; refactor composeCommand()
to return a structured object like { bin: string, args: string[] } (or similar
named type) and update runComposeUp to call spawn(bin, args, { shell: false,
stdio: ['ignore','inherit','pipe'], env }) instead of spawn(cmd, { shell: true,
... }); update any callers of composeCommand()/runComposeUp to use the new
shape, preserve collection of stderr via proc.stderr?.on('data', ...) and the
existing close/error handling, and remove shell: true usage in runComposeUp.
- Around line 78-132: Several MCP profiles (e.g., keys 'mcp-context7',
'mcp-playwright', 'mcp-filesystem', 'mcp-desktop-commander', 'mcp-tavily',
'mcp-firecrawl', 'github-mcp' entry) currently use floating docker image tags in
the dockerImages property (":latest"); change each dockerImages entry to a
specific version tag or immutable digest (e.g., replace "ghcr.io/...:latest"
with a stable "ghcr.io/...:vX.Y.Z" or "@sha256:...") to ensure reproducible
deployments, and update any documentation/configuration that references these
images; optionally add a comment or a configuration mechanism to allow
overriding pins for dev/local runs.
Harden launch/doctor MCP readiness gates with async execution, typed outcomes, and portable docs
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@copilot+5.3+codex , verify each finding against the current code and only fix it if needed. Inline comments:
Duplicate comments:
Nitpick comments:
|
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/cli/launch.ts (1)
456-469:⚠️ Potential issue | 🟠 MajorGuard
executeDoctorso launch always returns a structured report.If
runDoctorthrows,runLaunchrejects and the--jsoncontract breaks. Wrap it in try/catch and emit a faileddoctor.strictstep.Suggested guard
- doctorReport = await executeDoctor( - { - cwd, - mcpProfiles: profileNames, - skipCodexAuth: Boolean(options.skipCodexAuth) - }, - { - fileExists, - spawnCommand, - getServiceStatus: deps.getServiceStatus, - getCodexRegistration: deps.getCodexRegistration, - registriesForProfiles: deps.registriesForProfiles - } - ); + try { + doctorReport = await executeDoctor( + { + cwd, + mcpProfiles: profileNames, + skipCodexAuth: Boolean(options.skipCodexAuth) + }, + { + fileExists, + spawnCommand, + getServiceStatus: deps.getServiceStatus, + getCodexRegistration: deps.getCodexRegistration, + registriesForProfiles: deps.registriesForProfiles + } + ); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + const doctorStep: LaunchStep = { + id: 'doctor.strict', + ok: false, + details: `Doctor execution failed: ${message}`, + remediation: 'Run `codex-synaptic doctor --strict --json` to inspect failing checks.' + }; + const stop = appendStep(doctorStep); + return stop ?? buildLaunchReport(steps, doctorReport); + }As per coding guidelines: Always handle async operations with proper error catching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/launch.ts` around lines 456 - 469, Wrap the await executeDoctor call in runLaunch so any thrown error is caught and runLaunch still returns a structured doctorReport; specifically, catch errors around the executeDoctor(...) invocation and assign doctorReport to a failed structured report object that includes a doctor.strict step marked failed (include the error message/details), ensuring the --json contract is preserved; reference executeDoctor, runLaunch, doctorReport and the doctor.strict step when implementing the try/catch and constructing the fallback report.src/env/service-manager.ts (1)
370-405:⚠️ Potential issue | 🟡 MinorAdd a timeout guard so
docker compose upcan’t hang indefinitely.If compose stalls on a registry/network issue,
ensureServicewaits forever; a hard timeout keeps the launch gate deterministic.Possible timeout guard
private runComposeUp(cmd: string, env: NodeJS.ProcessEnv): Promise<void> { + const COMPOSE_UP_TIMEOUT_MS = 300_000; return new Promise<void>((resolve, reject) => { const proc = spawn(cmd, { shell: true, stdio: ['ignore', 'inherit', 'pipe'], env }); const stderrChunks: Buffer[] = []; + let settled = false; + const timer = setTimeout(() => { + if (settled) return; + settled = true; + proc.kill(); + reject(new Error(`docker compose up timed out after ${COMPOSE_UP_TIMEOUT_MS / 1000}s`)); + }, COMPOSE_UP_TIMEOUT_MS); proc.stderr?.on('data', (chunk: Buffer) => stderrChunks.push(chunk)); proc.on('close', (code) => { + if (settled) return; + settled = true; + clearTimeout(timer); if (code === 0) { resolve(); } else { const stderr = Buffer.concat(stderrChunks).toString('utf8'); @@ }); proc.on('error', (spawnErr) => { + if (settled) return; + settled = true; + clearTimeout(timer); const err = Object.assign(spawnErr, { status: null, stdout: '', stderr: spawnErr.message, }); reject(err); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 370 - 405, runComposeUp can hang indefinitely if the spawned docker compose process stalls; add a hard timeout that kills the child and rejects the Promise when elapsed. Inside runComposeUp (the Promise executor where you have const proc = spawn(...)), create a timer (e.g. const timer = setTimeout(..., COMPOSE_UP_TIMEOUT_MS)) that when fired does proc.kill('SIGKILL') and rejects with an Error describing a timeout (attach status: null, stdout: '', stderr: 'timeout'); ensure you clearTimeout(timer) on proc.on('close') and proc.on('error') so normal completion/error cancels the timeout. Use the existing error shape used in the handler (status, stdout, stderr) so wrap the timeout rejection consistently with other rejections.src/cli/index.ts (1)
4366-4392:⚠️ Potential issue | 🟡 MinorValidate docker‑login profile names before computing registries.
If a user passes an unknown profile,registriesForProfilescan return empty and produce a misleading “no auth required” message. Add an explicit validation step to surface a friendly error.🔧 Suggested guard for unknown profiles
.action(handleCommand('env.docker-login', async (names: string[] = [], options) => { const targets = names.length ? names : [...DEFAULT_MCP_PROFILES]; + const knownProfiles = new Set(serviceManager.listProfiles().map(({ name }) => name)); + const unknown = targets.filter((name) => !knownProfiles.has(name)); + if (unknown.length) { + throw new Error( + `Unknown service profile(s): ${unknown.join(', ')}. ` + + 'Run "codex-synaptic env list" to see valid profiles.' + ); + } const registries = serviceManager.registriesForProfiles(targets);As per coding guidelines, "Implement proper input validation and sanitization".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 4366 - 4392, The command handler for envCmd.command('docker-login') should validate incoming profile names before calling serviceManager.registriesForProfiles: check each requested name against the serviceManager's canonical profile list (e.g., serviceManager.profiles or serviceManager.getProfiles()/profileExists — add a profileExists(name) helper if none exists) and if any names are unknown, print a clear error listing the invalid profiles and exit early; only if all names are valid proceed to call serviceManager.registriesForProfiles(targets) and then serviceManager.dockerLogin(registry) as currently implemented.
🧹 Nitpick comments (2)
src/cli/doctor.ts (1)
15-19: Prefer an interface forSpawnCommandResult.This keeps the contract extensible and aligns with the TS guideline.
Suggested change
-export type SpawnCommandResult = { +export interface SpawnCommandResult { status: number | null; stdout: string; stderr: string; -}; +}As per coding guidelines: Prefer interfaces over types for extensibility in TypeScript.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/doctor.ts` around lines 15 - 19, Replace the exported type alias SpawnCommandResult with an exported interface to make the contract extensible; specifically, change the declaration "export type SpawnCommandResult = { status: number | null; stdout: string; stderr: string; }" to an interface "export interface SpawnCommandResult { status: number | null; stdout: string; stderr: string; }" and ensure all references/exports continue to compile (update any type-only usages if necessary).src/env/service-manager.ts (1)
326-343: Use a structured error for invalid docker registry input.
dockerLogincurrently throws a genericErroron empty registry values. Prefer aCodexSynapticErrorso callers can handle it consistently.Suggested change
- if (!normalized) { - throw new Error('Docker registry is required for docker login.'); - } + if (!normalized) { + throw new CodexSynapticError( + ErrorCode.BRIDGE_ERROR, + 'Docker registry is required for docker login.', + { registry } + ); + }As per coding guidelines: Throw specific error types, not generic Error objects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 326 - 343, The dockerLogin method currently throws a generic Error for empty registry values; replace that with throwing a CodexSynapticError so callers can handle it consistently: in dockerLogin, change the throw new Error('Docker registry is required for docker login.') to throw new CodexSynapticError('Docker registry is required for docker login.') (or construct with whatever standard fields your CodexSynapticError expects), and ensure CodexSynapticError is imported/available in this module; keep the original message and semantics so behavior is unchanged except for the error type.
🤖 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`:
- Around line 2-187: The runDoctor helper currently sets fileExists default to
the synchronous existsSync, which blocks the event loop; change the default to
an async implementation using fs/promises.access (or equivalent) that returns a
boolean so fileExists remains awaitable. Update imports (remove existsSync,
import access from 'fs/promises'), and replace the default assignment
(fileExists = deps.fileExists ?? existsSync) with an async wrapper that calls
access(distCliPath) and returns true on success / false on ENOENT, preserving
the existing call sites (e.g., runDoctor, fileExists, distCliPath).
---
Duplicate comments:
In `@src/cli/index.ts`:
- Around line 4366-4392: The command handler for envCmd.command('docker-login')
should validate incoming profile names before calling
serviceManager.registriesForProfiles: check each requested name against the
serviceManager's canonical profile list (e.g., serviceManager.profiles or
serviceManager.getProfiles()/profileExists — add a profileExists(name) helper if
none exists) and if any names are unknown, print a clear error listing the
invalid profiles and exit early; only if all names are valid proceed to call
serviceManager.registriesForProfiles(targets) and then
serviceManager.dockerLogin(registry) as currently implemented.
In `@src/cli/launch.ts`:
- Around line 456-469: Wrap the await executeDoctor call in runLaunch so any
thrown error is caught and runLaunch still returns a structured doctorReport;
specifically, catch errors around the executeDoctor(...) invocation and assign
doctorReport to a failed structured report object that includes a doctor.strict
step marked failed (include the error message/details), ensuring the --json
contract is preserved; reference executeDoctor, runLaunch, doctorReport and the
doctor.strict step when implementing the try/catch and constructing the fallback
report.
In `@src/env/service-manager.ts`:
- Around line 370-405: runComposeUp can hang indefinitely if the spawned docker
compose process stalls; add a hard timeout that kills the child and rejects the
Promise when elapsed. Inside runComposeUp (the Promise executor where you have
const proc = spawn(...)), create a timer (e.g. const timer = setTimeout(...,
COMPOSE_UP_TIMEOUT_MS)) that when fired does proc.kill('SIGKILL') and rejects
with an Error describing a timeout (attach status: null, stdout: '', stderr:
'timeout'); ensure you clearTimeout(timer) on proc.on('close') and
proc.on('error') so normal completion/error cancels the timeout. Use the
existing error shape used in the handler (status, stdout, stderr) so wrap the
timeout rejection consistently with other rejections.
---
Nitpick comments:
In `@src/cli/doctor.ts`:
- Around line 15-19: Replace the exported type alias SpawnCommandResult with an
exported interface to make the contract extensible; specifically, change the
declaration "export type SpawnCommandResult = { status: number | null; stdout:
string; stderr: string; }" to an interface "export interface SpawnCommandResult
{ status: number | null; stdout: string; stderr: string; }" and ensure all
references/exports continue to compile (update any type-only usages if
necessary).
In `@src/env/service-manager.ts`:
- Around line 326-343: The dockerLogin method currently throws a generic Error
for empty registry values; replace that with throwing a CodexSynapticError so
callers can handle it consistently: in dockerLogin, change the throw new
Error('Docker registry is required for docker login.') to throw new
CodexSynapticError('Docker registry is required for docker login.') (or
construct with whatever standard fields your CodexSynapticError expects), and
ensure CodexSynapticError is imported/available in this module; keep the
original message and semantics so behavior is unchanged except for the error
type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/guides/quick-start.mddocs/uat/UAT_READINESS_TRACKER.mdsrc/cli/doctor.tssrc/cli/env-bootstrap.tssrc/cli/index.tssrc/cli/launch.tssrc/env/service-manager.tstests/cli/doctor.test.tstests/cli/env-bootstrap.test.tstests/cli/launch.test.ts
Comment on lines
+2
to
+187
| import { existsSync } from 'fs'; | ||
| import { join } from 'path'; | ||
| import { serviceManager, type ServiceStatus } from '../env/service-manager.js'; | ||
| import { BridgeError, ErrorCode } from '../core/errors.js'; | ||
|
|
||
| export enum MCPProfile { | ||
| Filesystem = 'mcp-filesystem', | ||
| Playwright = 'mcp-playwright', | ||
| DesktopCommander = 'mcp-desktop-commander' | ||
| } | ||
|
|
||
| export const DEFAULT_MCP_PROFILES = Object.values(MCPProfile); | ||
|
|
||
| export type SpawnCommandResult = { | ||
| status: number | null; | ||
| stdout: string; | ||
| stderr: string; | ||
| }; | ||
|
|
||
| export interface DoctorCheck { | ||
| id: string; | ||
| ok: boolean; | ||
| details: string; | ||
| remediation?: string; | ||
| metadata?: Record<string, unknown>; | ||
| } | ||
|
|
||
| export interface DoctorSummary { | ||
| passed: number; | ||
| failed: number; | ||
| total: number; | ||
| } | ||
|
|
||
| export interface DoctorReport { | ||
| ok: boolean; | ||
| summary: DoctorSummary; | ||
| checks: DoctorCheck[]; | ||
| } | ||
|
|
||
| export interface DoctorOptions { | ||
| cwd?: string; | ||
| mcpProfiles?: string[]; | ||
| skipCodexAuth?: boolean; | ||
| } | ||
|
|
||
| export interface DoctorDependencies { | ||
| fileExists?: (path: string) => boolean | Promise<boolean>; | ||
| spawnCommand?: ( | ||
| command: string, | ||
| args: string[], | ||
| options: { cwd: string; encoding: BufferEncoding } | ||
| ) => Promise<SpawnCommandResult>; | ||
| getServiceStatus?: (name: string) => Promise<ServiceStatus>; | ||
| getCodexRegistration?: (name: string) => { codexName: string; url: string } | null; | ||
| registriesForProfiles?: (names: string[]) => string[]; | ||
| } | ||
|
|
||
| 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 BridgeError( | ||
| ErrorCode.MCP_ERROR, | ||
| 'Unsupported JSON format returned by `codex mcp list --json`.', | ||
| { retryable: false } | ||
| ); | ||
| } | ||
|
|
||
| export function parseProfileList(input: string | string[] | undefined, fallback = [...DEFAULT_MCP_PROFILES]): string[] { | ||
| if (Array.isArray(input)) { | ||
| const normalized = input | ||
| .map((item) => item.trim()) | ||
| .filter(Boolean); | ||
| return normalized.length ? normalized : [...fallback]; | ||
| } | ||
|
|
||
| if (typeof input === 'string') { | ||
| const normalized = input | ||
| .split(',') | ||
| .map((item) => item.trim()) | ||
| .filter(Boolean); | ||
| return normalized.length ? normalized : [...fallback]; | ||
| } | ||
|
|
||
| return [...fallback]; | ||
| } | ||
|
|
||
| export function collectDoctorRemediations(report: DoctorReport): string[] { | ||
| const unique = new Set<string>(); | ||
|
|
||
| for (const check of report.checks) { | ||
| if (check.ok || !check.remediation) { | ||
| continue; | ||
| } | ||
|
|
||
| const commands = check.remediation | ||
| .split('&&') | ||
| .map((item) => item.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| for (const command of commands) { | ||
| unique.add(command); | ||
| } | ||
| } | ||
|
|
||
| return Array.from(unique); | ||
| } | ||
|
|
||
| export async function runDoctor(options: DoctorOptions = {}, deps: DoctorDependencies = {}): Promise<DoctorReport> { | ||
| const cwd = options.cwd ?? process.cwd(); | ||
| const profileNames = parseProfileList(options.mcpProfiles); | ||
| const fileExists = deps.fileExists ?? existsSync; | ||
| const spawnCommand = deps.spawnCommand | ||
| ?? ((command, args, spawnOptions) => new Promise<SpawnCommandResult>((resolve) => { | ||
| const child = spawn(command, args, { | ||
| cwd: spawnOptions.cwd, | ||
| stdio: ['ignore', 'pipe', 'pipe'] | ||
| }); | ||
| if (child.stdout) { | ||
| child.stdout.setEncoding(spawnOptions.encoding); | ||
| } | ||
| if (child.stderr) { | ||
| child.stderr.setEncoding(spawnOptions.encoding); | ||
| } | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
| child.stdout?.on('data', (chunk: string) => { | ||
| stdout += chunk; | ||
| }); | ||
| child.stderr?.on('data', (chunk: string) => { | ||
| stderr += chunk; | ||
| }); | ||
| child.on('close', (status) => { | ||
| resolve({ status, stdout, stderr }); | ||
| }); | ||
| child.on('error', (error) => { | ||
| resolve({ | ||
| status: 1, | ||
| stdout, | ||
| stderr: stderr || `${error.name}: ${error.message}` | ||
| }); | ||
| }); | ||
| })); | ||
| const getServiceStatus = deps.getServiceStatus ?? ((name: string) => serviceManager.status(name)); | ||
| const getCodexRegistration = deps.getCodexRegistration | ||
| ?? ((name: string) => serviceManager.codexRegistration(name)); | ||
| const registriesForProfiles = deps.registriesForProfiles | ||
| ?? ((names: string[]) => serviceManager.registriesForProfiles(names)); | ||
|
|
||
| const checks: DoctorCheck[] = []; | ||
|
|
||
| const distCliPath = join(cwd, 'dist', 'cli', 'index.js'); | ||
| const distExists = await fileExists(distCliPath); | ||
| checks.push({ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the doctor.ts file to verify the async function and existsSync usage
head -200 src/cli/doctor.ts | tail -100Repository: clduab11/codex-synaptic
Length of output: 3138
🏁 Script executed:
# Also check the exact location of the existsSync assignment
rg "fileExists.*deps\.fileExists" src/cli/doctor.ts -A 5Repository: clduab11/codex-synaptic
Length of output: 363
🏁 Script executed:
# Compare with launch.ts to see the recommended pattern
rg "fileExists" src/cli/launch.ts -B 3 -A 3Repository: clduab11/codex-synaptic
Length of output: 636
🏁 Script executed:
# Check if spawn is imported (needed for the default implementation)
head -20 src/cli/doctor.tsRepository: clduab11/codex-synaptic
Length of output: 611
Replace the sync existsSync default with an async alternative to avoid blocking the event loop.
runDoctor is async and fileExists is awaited, but the sync existsSync default blocks during file checks. Use fs/promises.access instead, matching the pattern in launch.ts.
Suggested change
-import { existsSync } from 'fs';
+import { access } from 'fs/promises';
@@
- const fileExists = deps.fileExists ?? existsSync;
+ const fileExists = deps.fileExists ?? (async (path: string) => {
+ try {
+ await access(path);
+ return true;
+ } catch {
+ return false;
+ }
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/doctor.ts` around lines 2 - 187, The runDoctor helper currently sets
fileExists default to the synchronous existsSync, which blocks the event loop;
change the default to an async implementation using fs/promises.access (or
equivalent) that returns a boolean so fileExists remains awaitable. Update
imports (remove existsSync, import access from 'fs/promises'), and replace the
default assignment (fileExists = deps.fileExists ?? existsSync) with an async
wrapper that calls access(distCliPath) and returns true on success / false on
ENOENT, preserving the existing call sites (e.g., runDoctor, fileExists,
distCliPath).
…e tags Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
fix(service-manager): compose timeout, shell-free spawn, pinned image tags
This comment was marked as outdated.
This comment was marked as outdated.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/env/service-manager.ts (1)
228-248:⚠️ Potential issue | 🟠 MajorConvert blocking docker-compose calls to async.
Both
stopService()andstatus()useexecFileSync, which blocks the event loop. This violates the guideline "Don't block the event loop with synchronous operations" forsrc/**/*.tsfiles. Thestatus()method is particularly problematic—it's declaredasyncbut still blocks internally, defeating the async signature.Convert both methods to use
spawnwith proper Promise wrappers and error handling. Note: the CLI caller atsrc/cli/index.ts:5116currently doesn't awaitstopService()and will need updating once this is async.🛠️ Suggested change
- stopService(name: string): void { + async stopService(name: string): Promise<void> { const profile = this.getProfile(name); const { bin, args } = composeCommand(profile, 'down', profile.services); this.logger.info('env', `Stopping service ${name}`, { command: [bin, ...args].join(' ') }); - execFileSync(bin, args, { stdio: 'inherit' }); + await new Promise<void>((resolve, reject) => { + const child = spawn(bin, args, { stdio: 'inherit' }); + child.on('error', reject); + child.on('close', (code) => { + code === 0 + ? resolve() + : reject(new Error(`docker compose down exited with code ${code ?? 'unknown'}`)); + }); + }); } @@ - const output = execFileSync(bin, args, { stdio: 'pipe' }).toString(); + const output = await new Promise<string>((resolve, reject) => { + const child = spawn(bin, args, { stdio: ['ignore', 'pipe', 'pipe'] }); + const stdout: Buffer[] = []; + const stderr: Buffer[] = []; + child.stdout?.on('data', (chunk: Buffer) => stdout.push(chunk)); + child.stderr?.on('data', (chunk: Buffer) => stderr.push(chunk)); + child.on('error', reject); + child.on('close', (code) => { + if (code === 0) { + resolve(Buffer.concat(stdout).toString('utf8')); + return; + } + reject( + Object.assign(new Error(`docker compose ps exited with code ${code ?? 'unknown'}`), { + stderr: Buffer.concat(stderr).toString('utf8') + }) + ); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env/service-manager.ts` around lines 228 - 248, Both stopService and status are calling execFileSync (blocking); update them to non-blocking implementations that use child_process.spawn wrapped in Promises: change stopService(name: string): void to async stopService(name: string): Promise<void>, call composeCommand(profile, 'down', ...) and spawn(bin, args, { stdio: 'inherit' }) and await a Promise that resolves on 'close' (code === 0) and rejects on error or non-zero exit, and keep the logger.info with the composed command; in status(name: string): Promise<ServiceStatus> replace execFileSync with spawn(bin, args, { stdio: 'pipe' }), collect stdout via data events into a string, await completion similarly, preserve diagnostics collection and the /\bUp\b/.test(output) check to set running, and ensure errors from the spawned process are caught and turned into rejected Promises so callers can handle them (note: update callers such as the CLI to await stopService now that it returns a Promise).
🤖 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/env/service-manager.ts`:
- Around line 40-46: Replace the hardcoded COMPOSE_UP_TIMEOUT_MS with a
configurable value read from env: create a getter or assign
COMPOSE_UP_TIMEOUT_MS using parseInt(process.env.COMPOSE_UP_TIMEOUT_MS ??
'300000', 10) and validate fallback when parseInt yields NaN (use 300000 as
default); ensure the const name COMPOSE_UP_TIMEOUT_MS is used where
ComposeCommand or docker compose up timeouts are applied so existing code
continues to reference the same symbol.
- Around line 346-365: Replace generic Error usage in dockerLogin with the
project's typed CodexSynapticError: validate registry and throw a
CodexSynapticError when missing, and when the spawned docker process errors or
exits non‑zero, reject/throw a CodexSynapticError containing the exit code or
underlying error and include structured context (e.g., { registry: normalized,
command: 'docker login' }) so callers can handle typed errors consistently;
update the Promise rejection paths inside dockerLogin and the initial validation
to use CodexSynapticError instead of Error.
- Around line 457-507: Function wrapComposeStartError currently types its return
as Error but always constructs and returns a CodexSynapticError; change the
signature to return CodexSynapticError (private wrapComposeStartError(...):
CodexSynapticError) so the type matches the actual returned type, adjust any
related type annotations/usages if the narrower type surfaces compiler errors,
and ensure CodexSynapticError is imported/available in the module.
---
Outside diff comments:
In `@src/env/service-manager.ts`:
- Around line 228-248: Both stopService and status are calling execFileSync
(blocking); update them to non-blocking implementations that use
child_process.spawn wrapped in Promises: change stopService(name: string): void
to async stopService(name: string): Promise<void>, call composeCommand(profile,
'down', ...) and spawn(bin, args, { stdio: 'inherit' }) and await a Promise that
resolves on 'close' (code === 0) and rejects on error or non-zero exit, and keep
the logger.info with the composed command; in status(name: string):
Promise<ServiceStatus> replace execFileSync with spawn(bin, args, { stdio:
'pipe' }), collect stdout via data events into a string, await completion
similarly, preserve diagnostics collection and the /\bUp\b/.test(output) check
to set running, and ensure errors from the spawned process are caught and turned
into rejected Promises so callers can handle them (note: update callers such as
the CLI to await stopService now that it returns a Promise).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/env/service-manager.ts
Comment on lines
+40
to
+46
| interface ComposeCommand { | ||
| bin: string; | ||
| args: string[]; | ||
| } | ||
|
|
||
| /** Maximum time (ms) to wait for `docker compose up -d` before killing the process. */ | ||
| const COMPOSE_UP_TIMEOUT_MS = 300_000; // 5 minutes |
Contributor
There was a problem hiding this comment.
Make the compose-up timeout configurable.
Hardcoding this value makes tuning harder across environments; consider an env override with a default.
🛠️ Suggested change
-const COMPOSE_UP_TIMEOUT_MS = 300_000; // 5 minutes
+const COMPOSE_UP_TIMEOUT_MS = Number.parseInt(
+ process.env.CODEX_COMPOSE_UP_TIMEOUT_MS ?? '',
+ 10
+) || 300_000; // 5 minutesAs per coding guidelines: "Don't hardcode configuration values".
📝 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
| interface ComposeCommand { | |
| bin: string; | |
| args: string[]; | |
| } | |
| /** Maximum time (ms) to wait for `docker compose up -d` before killing the process. */ | |
| const COMPOSE_UP_TIMEOUT_MS = 300_000; // 5 minutes | |
| interface ComposeCommand { | |
| bin: string; | |
| args: string[]; | |
| } | |
| /** Maximum time (ms) to wait for `docker compose up -d` before killing the process. */ | |
| const COMPOSE_UP_TIMEOUT_MS = Number.parseInt( | |
| process.env.CODEX_COMPOSE_UP_TIMEOUT_MS ?? '', | |
| 10 | |
| ) || 300_000; // 5 minutes |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/env/service-manager.ts` around lines 40 - 46, Replace the hardcoded
COMPOSE_UP_TIMEOUT_MS with a configurable value read from env: create a getter
or assign COMPOSE_UP_TIMEOUT_MS using parseInt(process.env.COMPOSE_UP_TIMEOUT_MS
?? '300000', 10) and validate fallback when parseInt yields NaN (use 300000 as
default); ensure the const name COMPOSE_UP_TIMEOUT_MS is used where
ComposeCommand or docker compose up timeouts are applied so existing code
continues to reference the same symbol.
Comment on lines
+346
to
+365
| async dockerLogin(registry: string): Promise<void> { | ||
| const normalized = registry.trim(); | ||
| if (!normalized) { | ||
| throw new Error('Docker registry is required for docker login.'); | ||
| } | ||
| this.logger.info('env', 'Authenticating Docker registry', { registry: normalized }); | ||
| await new Promise<void>((resolve, reject) => { | ||
| const child = spawn('docker', ['login', normalized], { stdio: 'inherit' }); | ||
| child.on('error', (error) => { | ||
| reject(error); | ||
| }); | ||
| child.on('close', (code) => { | ||
| if (code === 0) { | ||
| resolve(); | ||
| return; | ||
| } | ||
| reject(new Error(`docker login exited with status ${code ?? 'unknown'}`)); | ||
| }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Use CodexSynapticError for dockerLogin failures.
Keeping typed errors and structured context aligns better with the rest of the error handling surface.
🛠️ Suggested change
- if (!normalized) {
- throw new Error('Docker registry is required for docker login.');
- }
+ if (!normalized) {
+ throw new CodexSynapticError(
+ ErrorCode.BRIDGE_ERROR,
+ 'Docker registry is required for docker login.',
+ { registry, code: 'DOCKER_LOGIN_FAILED' },
+ false
+ );
+ }
@@
- reject(new Error(`docker login exited with status ${code ?? 'unknown'}`));
+ reject(
+ new CodexSynapticError(
+ ErrorCode.BRIDGE_ERROR,
+ `docker login exited with status ${code ?? 'unknown'}`,
+ { registry: normalized, exitStatus: code ?? null, code: 'DOCKER_LOGIN_FAILED' },
+ false
+ )
+ );As per coding guidelines: "Throw specific error types, not generic Error objects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/env/service-manager.ts` around lines 346 - 365, Replace generic Error
usage in dockerLogin with the project's typed CodexSynapticError: validate
registry and throw a CodexSynapticError when missing, and when the spawned
docker process errors or exits non‑zero, reject/throw a CodexSynapticError
containing the exit code or underlying error and include structured context
(e.g., { registry: normalized, command: 'docker login' }) so callers can handle
typed errors consistently; update the Promise rejection paths inside dockerLogin
and the initial validation to use CodexSynapticError instead of Error.
Comment on lines
+457
to
+507
| private wrapComposeStartError(name: string, profile: ServiceProfile, cmd: string, error: unknown): Error { | ||
| const execError = error as { | ||
| status?: number | null; | ||
| stdout?: string | Buffer; | ||
| stderr?: string | Buffer; | ||
| message?: string; | ||
| }; | ||
|
|
||
| const stdout = this.toText(execError.stdout); | ||
| const stderr = this.toText(execError.stderr); | ||
| const combined = [stderr, stdout] | ||
| .filter(Boolean) | ||
| .join('\n') | ||
| .trim(); | ||
| const output = combined || (execError.message?.trim() ?? 'Unknown docker compose error'); | ||
| const exitStatus = typeof execError.status === 'number' ? execError.status : null; | ||
| const images = profile.dockerImages?.join(', ') || 'unknown image'; | ||
|
|
||
| let diagnosis = `Docker compose startup failed for ${name}`; | ||
| let remediation = 'Verify Docker is running, then retry.'; | ||
|
|
||
| if (/pull access denied|requested access to the resource is denied|insufficient_scope|unauthorized|authentication required|error from registry:\s*denied/i.test(output)) { | ||
| diagnosis = `Docker image pull/auth denied for ${name} (${images})`; | ||
| remediation = `Run \`codex-synaptic env docker-login ${name}\` and retry \`codex-synaptic env up ${name}\`.`; | ||
| } else if (/Cannot connect to the Docker daemon|Is the docker daemon running/i.test(output)) { | ||
| diagnosis = `Docker daemon unavailable while starting ${name}`; | ||
| remediation = 'Start Docker Desktop (or the Docker daemon) and retry.'; | ||
| } else if (/command not found|ENOENT/i.test(output)) { | ||
| diagnosis = `Docker CLI unavailable while starting ${name}`; | ||
| remediation = 'Install Docker with the Compose plugin and ensure `docker compose` works.'; | ||
| } | ||
|
|
||
| const truncatedOutput = output.length > 500 ? `${output.slice(0, 500)}…` : output; | ||
| const exitLabel = exitStatus === null ? 'unknown' : String(exitStatus); | ||
|
|
||
| return new CodexSynapticError( | ||
| ErrorCode.BRIDGE_ERROR, | ||
| `${diagnosis} (exit=${exitLabel}, compose=${cmd}). ${remediation} Raw docker output: ${truncatedOutput}`, | ||
| { | ||
| code: 'COMPOSE_START_FAILED', | ||
| diagnosis, | ||
| remediation, | ||
| exitStatus, | ||
| composeCmd: cmd, | ||
| output: truncatedOutput, | ||
| images, | ||
| serviceName: name, | ||
| profile: profile.composeFile | ||
| }, | ||
| false | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and search for wrapComposeStartError usage
rg -n "wrapComposeStartError" src/env/service-manager.ts -A 2 -B 2Repository: clduab11/codex-synaptic
Length of output: 737
🏁 Script executed:
#!/bin/bash
# Also search for all callers of this method
rg "wrapComposeStartError" --type ts -B 2 -A 2Repository: clduab11/codex-synaptic
Length of output: 1082
🏁 Script executed:
#!/bin/bash
# Check the method signature and full implementation around lines 457-507
sed -n '450,510p' src/env/service-manager.ts | cat -nRepository: clduab11/codex-synaptic
Length of output: 2952
Narrow the return type to CodexSynapticError.
The function always returns CodexSynapticError (never Error or other variants), so the return type should reflect this for stricter type safety.
Suggested change
- private wrapComposeStartError(name: string, profile: ServiceProfile, cmd: string, error: unknown): Error {
+ private wrapComposeStartError(
+ name: string,
+ profile: ServiceProfile,
+ cmd: string,
+ error: unknown
+ ): CodexSynapticError {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/env/service-manager.ts` around lines 457 - 507, Function
wrapComposeStartError currently types its return as Error but always constructs
and returns a CodexSynapticError; change the signature to return
CodexSynapticError (private wrapComposeStartError(...): CodexSynapticError) so
the type matches the actual returned type, adjust any related type
annotations/usages if the narrower type surfaces compiler errors, and ensure
CodexSynapticError is imported/available in the module.
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
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.
Plan
src/cli/launch.ts,src/cli/doctor.ts,src/env/service-manager.ts, and relevant docs to understand how MCP launch/doctor gates currently fail and what diagnostics exist.docs/uat/UAT_READINESS_TRACKER.mdwith outcomes before pausing for the next chunk.Chunk 1 in progress
Status will follow after implementation.
Continue Tasks:▶️ 1 queued — View all