Refactor: Launch‑gated global CLI/daemon appliance (strict JSON launch, project attach, docs)#61
Refactor: Launch‑gated global CLI/daemon appliance (strict JSON launch, project attach, docs)#61clduab11 wants to merge 5 commits into
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds project configuration and schema, new Codex config, extensive documentation (launch-gate, usage, sandbox), a strict JSON launch-reporting flow with checks/fixes, optional engine adapters, and CLI project attach/list commands + persistent attached-projects store. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as codex CLI
participant Spawn as OS / spawn()
participant Daemon as Daemon (stateDir)
participant Engines as Optional Engines
participant Store as Attached Projects Store
participant Report as JSON Report Builder
User->>CLI: codex-synaptic launch --strict --json
CLI->>Spawn: run checks (node/npm, npm ci --dry-run, build, test, tsc)
Spawn-->>CLI: check results / possible remediation commands
CLI->>Daemon: validate daemon.health & state location
Daemon-->>CLI: health status / remediation hint
CLI->>Engines: detect ruflo / ruv-fann availability
Engines-->>CLI: versions / availability
CLI->>Report: aggregate checks, fixes, capabilities -> build JSON
Report-->>User: LaunchStrictJsonReport { ok, capabilities, checks, fixes, nextActions }
alt attach command
User->>CLI: codex project attach <path>
CLI->>Store: read/write attached-projects.json (record configs present)
Store-->>CLI: updated record
CLI-->>User: attached-project record (JSON)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
coderabbitai
Bot
added
documentation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the Codex-Synaptic project's operational and configuration model, shifting towards a more appliance-grade global CLI and daemon architecture. Key changes include the addition of new configuration files (.codex-synaptic/project.json, .codex-synaptic/project.schema.json, .codex/config.toml) and extensive documentation updates (AGENTS.md, README.md, docs/codex-usage.md, docs/launch-gate.md, docs/sandbox-and-rules.md). The AGENTS.md and README.md files are completely rewritten to focus on a 'Launch-Gated Operating Contract', emphasizing a mandatory codex-synaptic launch --strict --json command as the primary readiness check, defining command policies, and detailing worktree/daemon state management. New CLI commands (project attach, project list) are added to manage repository attachments, and the launch command now produces a structured JSON report with detailed checks, remediations, and capabilities, including detection of optional engines like ruflo and ruv-fann. The doctor command logic is simplified, delegating to a centralized runDoctor function. Review comments highlight a critical command injection vulnerability in the remediationIsSafe function due to a weak regex, which could allow arbitrary command execution, and suggest a more robust regex. Additionally, naming inconsistencies for the ruvFann optional engine (appearing as ruvFann, ruv-FANN, and ruv-fann across files) are noted, with a recommendation to standardize on ruv-fann in project.json, its schema, and documentation for consistency and correct functionality.
| function remediationIsSafe(command: string): boolean { | ||
| return !/(docker-login|brew\s+install|npm\s+install(?!\s+--no-save))/i.test(command); | ||
| } |
There was a problem hiding this comment.
The remediationIsSafe function has a critical command injection vulnerability. Its weak regex fails to account for shell metacharacters (e.g., ;, |, &) when determining if a command is safe. This allows an attacker to inject malicious commands via unsanitized user input (like profileNames from --mcp-profiles), leading to arbitrary command execution if these "safe" fixes are executed. Additionally, the current regex incorrectly flags a simple npm install as unsafe, contradicting documentation. The regex should be updated to specifically target unsafe npm install variants (e.g., global installs) and, more importantly, to prevent shell metacharacter injection.
| function remediationIsSafe(command: string): boolean { | |
| return !/(docker-login|brew\s+install|npm\s+install(?!\s+--no-save))/i.test(command); | |
| } | |
| function remediationIsSafe(command: string): boolean { | |
| return !/(docker-login|brew\s+install|npm\s+install\s+(-g|--global))/i.test(command); | |
| } |
| "ruflo": true, | ||
| "ruvFann": true |
There was a problem hiding this comment.
There's an inconsistency in the naming of the optional engine. Here it's ruvFann (camelCase), but the implementation in src/adapters/optional-engines.ts and src/cli/launch.ts expects ruv-fann (kebab-case). The documentation also uses ruv-FANN or ruv-fann. This discrepancy will prevent the configuration from being correctly applied.
To ensure consistency and correct functionality, I recommend standardizing on ruv-fann across all configuration, code, and documentation. This is also a more common convention for keys representing tool names in configuration files.
| "ruflo": true, | |
| "ruvFann": true | |
| "ruflo": true, | |
| "ruv-fann": true |
| "type": "object", | ||
| "properties": { | ||
| "ruflo": { "type": "boolean" }, | ||
| "ruvFann": { "type": "boolean" } |
There was a problem hiding this comment.
To align with the suggested change of standardizing the optional engine name to ruv-fann, the JSON schema should also be updated to expect ruv-fann instead of ruvFann.
| "ruvFann": { "type": "boolean" } | |
| "ruv-fann": { "type": "boolean" } |
| ## 6) Optional engines | ||
|
|
||
| The Codex-Synaptic agent system provides a comprehensive framework for enhancing OpenAI's Codex with advanced distributed computing capabilities. Through careful orchestration of various agent types, consensus mechanisms, and coordination patterns, the system can tackle complex computational challenges that require distributed intelligence, fault tolerance, and scalable coordination. | ||
| `ruflo` and `ruv-FANN` are optional adapters. Missing engines must never block core launch readiness. |
There was a problem hiding this comment.
There's a naming inconsistency for the optional engine ruv-FANN. Throughout the codebase and configuration files, it's referred to as ruvFann or ruv-fann. For consistency, it would be best to standardize on one format. I've suggested ruv-fann in other comments. It would be good to update the documentation here as well to reflect that.
| `ruflo` and `ruv-FANN` are optional adapters. Missing engines must never block core launch readiness. | |
| `ruflo` and `ruv-fann` are optional adapters. Missing engines must never block core launch readiness. |
![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: 4fd79eeb67
ℹ️ 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".
| function remediationIsSafe(command: string): boolean { | ||
| return !/(docker-login|brew\s+install|npm\s+install(?!\s+--no-save))/i.test(command); | ||
| } |
There was a problem hiding this comment.
The safeUnderSandbox classifier currently treats any npm install remediation as unsafe, but this conflicts with the launch contract where npm install is explicitly allowed under workspace-write and only host-level installs (for example npm install -g) should be unsafe. When repo.dependencies fails, automations that apply only safeUnderSandbox=true fixes will skip the required dependency remediation and can get stuck in a failing launch loop.
Useful? React with 👍 / 👎.
| const cwdPrefix = `${cwd}/`; | ||
| const stateOutsideRepo = !daemonPaths.stateDir.startsWith(cwdPrefix) && daemonPaths.stateDir !== cwd; | ||
| checks.push({ |
There was a problem hiding this comment.
This worktree-safety check compares daemonPaths.stateDir as a raw string against an absolute cwd, so a relative override like CODEX_SYNAPTIC_STATE_DIR=.codex-synaptic is incorrectly reported as outside the repo even though it resolves inside the worktree. In that case launch reports worktree.state_location=pass and permits a repo-unsafe daemon state location, violating the stated safety guarantee.
Useful? React with 👍 / 👎.
| if (scripts.test) { | ||
| const testCheck = await runCheckCommand(spawnCommand, cwd, 'repo.test', 'npm', ['run', 'test'], 'Run `npm run test`.'); | ||
| checks.push(testCheck.check); |
There was a problem hiding this comment.
The strict JSON gate runs npm run test whenever a test script exists, but this call has no timeout or CI-mode guard. On repositories where test defaults to watch/interactive mode, launch --strict --json never returns, which blocks the mandatory launch gate and prevents automation from progressing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
.codex-synaptic/project.schema.json (1)
7-22: AddadditionalProperties: falseto nested objects for consistency.The top-level and
optionalEnginesobjects correctly useadditionalProperties: false, butlaunchGateanddaemonobjects are missing this constraint. This inconsistency allows unexpected keys in those sub-objects.🔧 Add strict validation to nested objects
"launchGate": { "type": "object", "required": ["required", "strictJsonCommand"], "properties": { "required": { "type": "boolean" }, "strictJsonCommand": { "type": "string" } - } + }, + "additionalProperties": false }, "daemon": { "type": "object", "required": ["stateDir", "allowStateInsideRepo"], "properties": { "stateDir": { "type": "string" }, "allowStateInsideRepo": { "type": "boolean" } - } + }, + "additionalProperties": false },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex-synaptic/project.schema.json around lines 7 - 22, The launchGate and daemon object schemas are missing additionalProperties: false; update the "launchGate" and "daemon" definitions to include "additionalProperties": false alongside their existing "type", "required", and "properties" so unexpected keys are rejected (keep the existing required arrays and property definitions intact)..codex-synaptic/project.json (1)
12-15: Minor naming inconsistency:ruvFannvsruv-FANN.The schema and config use
ruvFann(camelCase), but documentation (README.md, AGENTS.md) refers toruv-FANN. Consider aligning the naming for clarity—either update docs to useruvFannor add a note explaining the mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex-synaptic/project.json around lines 12 - 15, The optionalEngines key uses the camelCase symbol "ruvFann" but docs reference "ruv-FANN"; pick one canonical name and make them consistent: either rename the config key "ruvFann" to "ruv-FANN" (or vice versa) where optionalEngines is defined, and update all documentation references (README.md, AGENTS.md) to match, or keep "ruvFann" in the config and add a short mapping note in the docs clarifying that "ruv-FANN" in docs refers to the "ruvFann" config flag; ensure the symbol "optionalEngines" and the engine identifier are identical across code and docs..codex/config.toml (1)
18-20: Add trailing newline.POSIX convention expects files to end with a newline character. This prevents potential issues with some tools.
🔧 Add newline
[mcp_servers.desktop_commander] command = "npx" args = ["-y", "@modelcontextprotocol/server-desktop-commander"] +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/config.toml around lines 18 - 20, The file is missing a trailing newline after the TOML table for [mcp_servers.desktop_commander]; open the file and add a single newline character at the end so the last lines "command = \"npx\"" and "args = [\"-y\", \"@modelcontextprotocol/server-desktop-commander\"]" are terminated by a newline, ensuring POSIX-compliant file termination.src/cli/index.ts (1)
3068-3128: Surface the newprojectflow from the root help output.This adds a new top-level coordination primitive, but
codex-synaptic --helpstill highlights the older entry points only. Addingproject attach/project list(and likelylaunch) to the root help examples would make the golden path much easier to discover.Based on learnings, "Applies to src/cli/index.ts : Update CLI help text in
src/cli/index.tswhen exposing new coordination primitives or agent commands".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 3068 - 3128, The root CLI help does not mention the new "project" flow; update the global help examples so users discover project attach/list (and related launch commands). Add entries for "project attach <path>" and "project list --json" (and optionally "launch") to the top-level help generation where the program's examples are composed—look for the root help builder or call site that uses decorateCommandHelp/program.helpInformation and append these examples so they appear alongside existing root examples; ensure the text references projectCmd semantics (attach/list) and mirrors the short usage shown in projectCmd.action output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.codex/config.toml:
- Around line 14-20: The mcp_servers.desktop_commander entry references a
non-existent npm package "@modelcontextprotocol/server-desktop-commander" so
update the mcp_servers.desktop_commander args to the correct package (e.g.,
replace "@modelcontextprotocol/server-desktop-commander" with the verified
package name such as "@wonderwhy-er/desktop-commander" or your actual server
package) or remove the entry if unused; additionally pin versions for
reproducibility by changing mcp_servers.playwright args from
"@playwright/mcp@latest" to a specific version (and likewise pin the
desktop_commander/server-filesystem packages to explicit versions like
"@playwright/mcp@0.0.68" and "@modelcontextprotocol/server-filesystem@2026.1.14"
or your chosen versions) so npx calls are deterministic.
In `@src/adapters/optional-engines.ts`:
- Around line 15-35: detectOptionalEngines currently uses detectVersion which
lets spawnCommand rejections bubble up into Promise.all and fail the whole
operation; modify detectVersion (or wrap its call in detectOptionalEngines) to
catch per-engine errors and return an OptionalEngineStatus with name set to the
engine ('ruflo' or 'ruv-fann'), available: false and version: '' when
spawnCommand rejects or throws. Specifically, update detectVersion (or the
Promise.all arguments) to handle errors from SpawnLike calls so each engine
detection is failure-tolerant and does not reject the overall
detectOptionalEngines promise.
In `@src/cli/launch.ts`:
- Around line 293-356: The JSON output is missing projections of many failing
launch steps into checks/fixes and uses report.ok alone to set ok; update the
logic that builds checks/fixes to iterate all report.steps (not just
runtime.daemon and mcp.codex_register) and push a corresponding check entry for
each step (use step.id, step.ok -> status 'pass'|'fail'|'warn', and step.details
for detail), and for any step with !step.ok and step.remediation
splitRemediationCommands(step.remediation) into fixes (preserving
remediationIsSafe(command)); then compute ok from checks (no failing status)
rather than only report.ok, and change nextActions/exit behavior so when ok is
false the launcher returns the JSON with remediations and does not proceed with
repository changes (i.e., treat launch as a hard gate).
- Around line 247-255: The repo.dependencies launch-gate check currently runs a
mutating command via runCheckCommand (spawnCommand, cwd, 'repo.dependencies',
'npm', ['install','--package-lock-only','--ignore-scripts'], ...); change this
to a read-only verification (for example use ['ci','--dry-run'] or another npm
read-only flag supported in your environment) so the check does not rewrite
package-lock.json; update the args passed into runCheckCommand for the
'repo.dependencies' check and ensure the check still returns ok=false on drift
(so launch remains a hard gate) while never modifying repo files.
In `@src/cli/project-attach.ts`:
- Around line 40-50: attachProject currently resolves the path but doesn't
validate it; before building/persisting the AttachedProjectRecord (in the
attachProject function using absolutePath and record), check that the resolved
path exists and is a directory (use existsSync and fs.statSync or fs.lstatSync
and isDirectory()). If the check fails, throw a descriptive Error or return a
failure result instead of creating/persisting record; only proceed to populate
agentsFile, codexConfig, synapticConfig and return the record after the
directory validation passes.
---
Nitpick comments:
In @.codex-synaptic/project.json:
- Around line 12-15: The optionalEngines key uses the camelCase symbol "ruvFann"
but docs reference "ruv-FANN"; pick one canonical name and make them consistent:
either rename the config key "ruvFann" to "ruv-FANN" (or vice versa) where
optionalEngines is defined, and update all documentation references (README.md,
AGENTS.md) to match, or keep "ruvFann" in the config and add a short mapping
note in the docs clarifying that "ruv-FANN" in docs refers to the "ruvFann"
config flag; ensure the symbol "optionalEngines" and the engine identifier are
identical across code and docs.
In @.codex-synaptic/project.schema.json:
- Around line 7-22: The launchGate and daemon object schemas are missing
additionalProperties: false; update the "launchGate" and "daemon" definitions to
include "additionalProperties": false alongside their existing "type",
"required", and "properties" so unexpected keys are rejected (keep the existing
required arrays and property definitions intact).
In @.codex/config.toml:
- Around line 18-20: The file is missing a trailing newline after the TOML table
for [mcp_servers.desktop_commander]; open the file and add a single newline
character at the end so the last lines "command = \"npx\"" and "args = [\"-y\",
\"@modelcontextprotocol/server-desktop-commander\"]" are terminated by a
newline, ensuring POSIX-compliant file termination.
In `@src/cli/index.ts`:
- Around line 3068-3128: The root CLI help does not mention the new "project"
flow; update the global help examples so users discover project attach/list (and
related launch commands). Add entries for "project attach <path>" and "project
list --json" (and optionally "launch") to the top-level help generation where
the program's examples are composed—look for the root help builder or call site
that uses decorateCommandHelp/program.helpInformation and append these examples
so they appear alongside existing root examples; ensure the text references
projectCmd semantics (attach/list) and mirrors the short usage shown in
projectCmd.action output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29e2196c-7f53-4ca9-8fe0-5973b3a5b2ae
📒 Files selected for processing (12)
.codex-synaptic/project.json.codex-synaptic/project.schema.json.codex/config.tomlAGENTS.mdREADME.mddocs/codex-usage.mddocs/launch-gate.mddocs/sandbox-and-rules.mdsrc/adapters/optional-engines.tssrc/cli/index.tssrc/cli/launch.tssrc/cli/project-attach.ts
| [mcp_servers.playwright] | ||
| command = "npx" | ||
| args = ["-y", "@playwright/mcp@latest"] | ||
|
|
||
| [mcp_servers.desktop_commander] | ||
| command = "npx" | ||
| args = ["-y", "@modelcontextprotocol/server-desktop-commander"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of @modelcontextprotocol/server-filesystem npm package?
💡 Result:
The latest published version of @modelcontextprotocol/server-filesystem on npm is 2026.1.14. [1] [2]
🌐 Web query:
What is the latest version of @playwright/mcp npm package?
💡 Result:
The latest stable version published for the @playwright/mcp npm package is 0.0.68. [1]
There are also newer pre-release (alpha) builds under the 0.0.68-alpha-... version line. [2]
Sources: [1] [2]
🌐 Web query:
What is the latest version of @modelcontextprotocol/server-desktop-commander npm package?
💡 Result:
I couldn’t verify any published version for @modelcontextprotocol/server-desktop-commander on the public npm ecosystem (no third‑party index I checked shows a version number for that exact package name). Some directories/config examples reference it, but they don’t include a resolvable “latest version” tag. [1]
In practice, “Desktop Commander” is commonly installed/run as @wonderwhy-er/desktop-commander (often via @latest) rather than @modelcontextprotocol/server-desktop-commander. [2]
Sources:
[1] (agenticskills.io)
[2] (libraries.io)
Citations:
- 1: https://agenticskills.io/mcp/desktop-commander?utm_source=openai
- 2: https://libraries.io/npm/%40ildunari%2Fmcp-gateway?utm_source=openai
🏁 Script executed:
cat .codex/config.tomlRepository: clduab11/codex-synaptic
Length of output: 495
Verify the desktop_commander package name—@modelcontextprotocol/server-desktop-commander does not exist on npm.
The config references a package that cannot be resolved. Consider checking if you meant @wonderwhy-er/desktop-commander or another alternative server package. This will cause installation failures when attempting to launch the MCP server.
Separately, consider pinning MCP server versions for reproducibility. Currently, @playwright/mcp uses @latest while others use implicit latest. Pinning to specific versions avoids unexpected behavior changes during CI or automation runs. For example:
@modelcontextprotocol/server-filesystem@2026.1.14@playwright/mcp@0.0.68
Without pinning, each npx invocation may pull different versions, reducing determinism in your automation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.codex/config.toml around lines 14 - 20, The mcp_servers.desktop_commander
entry references a non-existent npm package
"@modelcontextprotocol/server-desktop-commander" so update the
mcp_servers.desktop_commander args to the correct package (e.g., replace
"@modelcontextprotocol/server-desktop-commander" with the verified package name
such as "@wonderwhy-er/desktop-commander" or your actual server package) or
remove the entry if unused; additionally pin versions for reproducibility by
changing mcp_servers.playwright args from "@playwright/mcp@latest" to a specific
version (and likewise pin the desktop_commander/server-filesystem packages to
explicit versions like "@playwright/mcp@0.0.68" and
"@modelcontextprotocol/server-filesystem@2026.1.14" or your chosen versions) so
npx calls are deterministic.
| async function detectVersion( | ||
| spawnCommand: SpawnLike, | ||
| cwd: string, | ||
| name: 'ruflo' | 'ruv-fann' | ||
| ): Promise<OptionalEngineStatus> { | ||
| const result = await spawnCommand(name, ['--version'], { cwd, encoding: 'utf8' }); | ||
| return { | ||
| name, | ||
| available: result.status === 0, | ||
| version: (result.stdout || result.stderr || '').trim() | ||
| }; | ||
| } | ||
|
|
||
| export async function detectOptionalEngines( | ||
| spawnCommand: SpawnLike, | ||
| cwd: string | ||
| ): Promise<OptionalEngineStatus[]> { | ||
| return Promise.all([ | ||
| detectVersion(spawnCommand, cwd, 'ruflo'), | ||
| detectVersion(spawnCommand, cwd, 'ruv-fann') | ||
| ]); |
There was a problem hiding this comment.
Make optional engine detection failure-tolerant.
If spawnCommand rejects here, Promise.all() rejects and launch --json fails outright instead of downgrading these engines to warnings. Please catch per-engine failures and return { available: false, version: '' } so optional adapters stay non-blocking.
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/adapters/optional-engines.ts` around lines 15 - 35, detectOptionalEngines
currently uses detectVersion which lets spawnCommand rejections bubble up into
Promise.all and fail the whole operation; modify detectVersion (or wrap its call
in detectOptionalEngines) to catch per-engine errors and return an
OptionalEngineStatus with name set to the engine ('ruflo' or 'ruv-fann'),
available: false and version: '' when spawnCommand rejects or throws.
Specifically, update detectVersion (or the Promise.all arguments) to handle
errors from SpawnLike calls so each engine detection is failure-tolerant and
does not reject the overall detectOptionalEngines promise.
| const daemonStep = report.steps.find((step) => step.id === 'runtime.daemon'); | ||
| checks.push({ | ||
| name: 'daemon.health', | ||
| status: daemonStep?.ok ? 'pass' : 'fail', | ||
| detail: daemonStep?.details ?? 'Daemon health check was not executed.' | ||
| }); | ||
| if (daemonStep && !daemonStep.ok && daemonStep.remediation) { | ||
| for (const command of splitRemediationCommands(daemonStep.remediation)) { | ||
| fixes.push({ why: 'Daemon is not healthy', command, safeUnderSandbox: remediationIsSafe(command) }); | ||
| } | ||
| } | ||
|
|
||
| const mcpConfigStatus = report.steps.find((step) => step.id === 'mcp.codex_register'); | ||
| checks.push({ | ||
| name: 'mcp.config', | ||
| status: mcpConfigStatus?.ok ? 'pass' : 'fail', | ||
| detail: mcpConfigStatus?.details ?? 'MCP configuration check did not run.' | ||
| }); | ||
|
|
||
| const daemonPaths = getDaemonPaths(); | ||
| const cwdPrefix = `${cwd}/`; | ||
| const stateOutsideRepo = !daemonPaths.stateDir.startsWith(cwdPrefix) && daemonPaths.stateDir !== cwd; | ||
| checks.push({ | ||
| name: 'worktree.state_location', | ||
| status: stateOutsideRepo ? 'pass' : 'fail', | ||
| detail: stateOutsideRepo | ||
| ? `Daemon state directory resolves outside repository: ${daemonPaths.stateDir}` | ||
| : `Daemon state directory must be outside repository/worktree. Current: ${daemonPaths.stateDir}` | ||
| }); | ||
| if (!stateOutsideRepo) { | ||
| fixes.push({ | ||
| why: 'Daemon state location is worktree-unsafe', | ||
| command: 'Export CODEX_SYNAPTIC_STATE_DIR to a stable path outside the repository (for example ~/.codex-synaptic).', | ||
| safeUnderSandbox: false | ||
| }); | ||
| } | ||
|
|
||
| const optionalEngines = await detectOptionalEngines(spawnCommand, cwd); | ||
| for (const engine of optionalEngines) { | ||
| checks.push({ | ||
| name: `optional.${engine.name}`, | ||
| status: engine.available ? 'pass' : 'warn', | ||
| detail: engine.available | ||
| ? `${engine.name} detected: ${engine.version || '--version returned success.'}` | ||
| : `${engine.name} not detected; continuing in core mode.` | ||
| }); | ||
| if (engine.available) { | ||
| capabilities.push(`${engine.name}-adapter`); | ||
| } | ||
| } | ||
|
|
||
| for (const step of report.steps) { | ||
| if (step.ok || !step.remediation) { | ||
| continue; | ||
| } | ||
| for (const command of splitRemediationCommands(step.remediation)) { | ||
| fixes.push({ why: `${step.id} gate failed`, command, safeUnderSandbox: remediationIsSafe(command) }); | ||
| } | ||
| } | ||
|
|
||
| const ok = checks.every((check) => check.status !== 'fail') && report.ok; | ||
| const nextActions = ok | ||
| ? ['Launch gate passed. Continue with bounded workflow execution and report generation.'] | ||
| : ['Apply safe remediations in order, then rerun `codex-synaptic launch --strict --json`.']; |
There was a problem hiding this comment.
The strict JSON report is dropping several root-cause failures.
ok is derived from report.ok, but checks only mirror runtime.daemon and mcp.codex_register from report.steps. If repo.preflight, codex.auth, mcp.up, or doctor.strict fails, automation gets ok: false without a matching failed check explaining why. Please project every failing launch step into checks/fixes before returning this JSON shape.
Based on learnings, "Run codex-synaptic launch --json before repository work to verify readiness; treat launch as a hard gate—if ok is false or the command exits non-zero, stop and return remediation commands instead of proceeding with repository changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` around lines 293 - 356, The JSON output is missing
projections of many failing launch steps into checks/fixes and uses report.ok
alone to set ok; update the logic that builds checks/fixes to iterate all
report.steps (not just runtime.daemon and mcp.codex_register) and push a
corresponding check entry for each step (use step.id, step.ok -> status
'pass'|'fail'|'warn', and step.details for detail), and for any step with
!step.ok and step.remediation splitRemediationCommands(step.remediation) into
fixes (preserving remediationIsSafe(command)); then compute ok from checks (no
failing status) rather than only report.ok, and change nextActions/exit behavior
so when ok is false the launcher returns the JSON with remediations and does not
proceed with repository changes (i.e., treat launch as a hard gate).
| export function attachProject(targetPath: string): AttachedProjectRecord { | ||
| const absolutePath = resolve(targetPath); | ||
| const record: AttachedProjectRecord = { | ||
| path: absolutePath, | ||
| attachedAt: new Date().toISOString(), | ||
| agentsFile: existsSync(join(absolutePath, 'AGENTS.md')) ? join(absolutePath, 'AGENTS.md') : null, | ||
| codexConfig: existsSync(join(absolutePath, '.codex', 'config.toml')) ? join(absolutePath, '.codex', 'config.toml') : null, | ||
| synapticConfig: existsSync(join(absolutePath, '.codex-synaptic', 'project.json')) | ||
| ? join(absolutePath, '.codex-synaptic', 'project.json') | ||
| : null | ||
| }; |
There was a problem hiding this comment.
Reject non-existent or non-directory attach targets.
project attach currently reports success even for typos or files, because the path is only resolve()d and then persisted. Please validate that the target exists and is a directory before writing the record.
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/project-attach.ts` around lines 40 - 50, attachProject currently
resolves the path but doesn't validate it; before building/persisting the
AttachedProjectRecord (in the attachProject function using absolutePath and
record), check that the resolved path exists and is a directory (use existsSync
and fs.statSync or fs.lstatSync and isDirectory()). If the check fails, throw a
descriptive Error or return a failure result instead of creating/persisting
record; only proceed to populate agentsFile, codexConfig, synapticConfig and
return the record after the directory validation passes.
| const store = readStore(); | ||
| const filtered = store.projects.filter((project) => project.path !== absolutePath); | ||
| filtered.push(record); | ||
| writeStore({ projects: filtered }); |
There was a problem hiding this comment.
This registry update can lose attachments under concurrent CLI use.
The store is updated with an unsynchronized read-modify-write cycle. Two project attach commands running at the same time can each read the old file and the last writer wins, dropping the other attachment; an interrupted write also becomes silent data loss because readStore() treats parse failures as an empty registry. Please switch this to an atomic write path and coordinate concurrent writers.
|
@copilot , verify each finding against the current code and only fix it if needed. In |
…-dry-run Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
fix: make repo.dependencies launch-gate check read-only
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cli/launch.ts (1)
293-310:⚠️ Potential issue | 🟠 MajorThe strict JSON still hides the actual failing launch step.
Lines 293-310 only project
runtime.daemonandmcp.codex_registerintochecks[], while Lines 344-356 add fixes from every failed step but still computeokfromreport.ok. Ifrepo.preflight,codex.auth,mcp.up, ordoctor.strictfails, callers getok: falsewithout a corresponding failed check explaining why. Please emit a check for everyreport.stepsentry and deriveokfrom the projected checks so the JSON stays self-describing. Based on learnings, "Runcodex-synaptic launch --jsonbefore repository work to verify readiness; treat launch as a hard gate—ifokisfalseor the command exits non-zero, stop and return remediation commands instead of proceeding with repository changes".Also applies to: 344-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/launch.ts` around lines 293 - 310, Iterate over every entry in report.steps and push a corresponding check into the checks array (use each step.id for name, step.ok for status, and step.details for detail fallback) instead of only projecting runtime.daemon and mcp.codex_register; compute the overall ok by deriving it from the aggregated checks (e.g., ok = checks.every(c => c.status === 'pass')) rather than using report.ok, and when ok is false return/emit the remediation commands collected from splitRemediationCommands(step.remediation) (using remediationIsSafe to set safeUnderSandbox) and stop further repository actions so the CLI exits non-zero with the remediation payload. Include references to report.steps, checks, fixes, splitRemediationCommands, remediationIsSafe and the overall ok computation when making the changes.
🤖 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/launch.ts`:
- Around line 166-168: The current remediationIsSafe function uses a denylist
which treats everything not matching as safe; change it to an explicit
allowlist: replace the inverse-regex approach in remediationIsSafe with a
whitelist of exact or anchored-safe command patterns (e.g., short, read-only
commands) stored in an array (e.g., safeCommands or safeCommandPatterns) and
return true only when the input command matches one of those entries (use
startsWith/anchored regex for robust matching); alternatively, remove global
logic and mark safeUnderSandbox explicitly at each remediation source where
safety intent is known (update callers that rely on remediationIsSafe to use the
new allowlist function or the per-remediation flag).
- Around line 155-164: splitRemediationCommands currently treats the whole
remediation string as a shell command which yields non-executable prose when
callers pass human instructions; update splitRemediationCommands to parse out
machine-executable pieces by first extracting code spans/backticked segments
(and then splitting those on '&&' and trimming) and return only the executable
fragments (fallback to empty array if none), then change the code that builds
fixes to use the first returned fragment for fix.command and keep the original
remediation text for human-facing fields; also apply the same
parsing/normalization logic where similar logic exists (the block around the
function and the other helper at lines ~196-204) so fix.command is always a
normalized executable string or omitted.
- Around line 273-280: Replace the fallback command that runs TypeScript in
runCheckCommand to avoid npm prompting/installing: instead of ['exec', 'tsc',
'--', '--noEmit'] invoke npm with the explicit no-install form (i.e. use the
equivalent of "exec --no --package=typescript -- tsc --noEmit") so that
spawnCommand is given a deterministic, local-only check; update the arguments
passed where 'repo.typecheck' is used (and/or add a preflight check that
TypeScript exists locally before calling runCheckCommand) to ensure no automatic
installs or interactive prompts occur.
---
Duplicate comments:
In `@src/cli/launch.ts`:
- Around line 293-310: Iterate over every entry in report.steps and push a
corresponding check into the checks array (use each step.id for name, step.ok
for status, and step.details for detail fallback) instead of only projecting
runtime.daemon and mcp.codex_register; compute the overall ok by deriving it
from the aggregated checks (e.g., ok = checks.every(c => c.status === 'pass'))
rather than using report.ok, and when ok is false return/emit the remediation
commands collected from splitRemediationCommands(step.remediation) (using
remediationIsSafe to set safeUnderSandbox) and stop further repository actions
so the CLI exits non-zero with the remediation payload. Include references to
report.steps, checks, fixes, splitRemediationCommands, remediationIsSafe and the
overall ok computation when making the changes.
| function splitRemediationCommands(raw?: string): string[] { | ||
| if (!raw) { | ||
| return []; | ||
| } | ||
|
|
||
| return raw | ||
| .split('&&') | ||
| .map((item) => item.trim()) | ||
| .filter(Boolean); | ||
| } |
There was a problem hiding this comment.
Keep fix.command machine-executable.
These helpers treat remediation as a shell command, but several callers below pass prose like Run \npm run build`.on Line 466 andRun `codex login` then re-run `codex login status`.on Line 502. That means the strict JSON can emit non-executablefix.commandvalues, which breaks the machine-readable launch-gate contract. Please separate human remediation text from executable commands, or normalize the strings before adding them tofixes`.
Also applies to: 196-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` around lines 155 - 164, splitRemediationCommands currently
treats the whole remediation string as a shell command which yields
non-executable prose when callers pass human instructions; update
splitRemediationCommands to parse out machine-executable pieces by first
extracting code spans/backticked segments (and then splitting those on '&&' and
trimming) and return only the executable fragments (fallback to empty array if
none), then change the code that builds fixes to use the first returned fragment
for fix.command and keep the original remediation text for human-facing fields;
also apply the same parsing/normalization logic where similar logic exists (the
block around the function and the other helper at lines ~196-204) so fix.command
is always a normalized executable string or omitted.
| function remediationIsSafe(command: string): boolean { | ||
| return !/(docker-login|brew\s+install|npm\s+install(?!\s+--no-save))/i.test(command); | ||
| } |
There was a problem hiding this comment.
safeUnderSandbox is currently too permissive.
This denylist marks every non-matching command as safe, so stateful remediations such as codex login, codex-synaptic background start, codex-synaptic env up, and codex-synaptic env codex-register all become safeUnderSandbox: true. Automation consuming this JSON can then auto-run auth-changing or global-state-changing commands in a sandbox. Please switch this to an allowlist of known-safe commands, or set the flag at each remediation source where the intent is explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` around lines 166 - 168, The current remediationIsSafe
function uses a denylist which treats everything not matching as safe; change it
to an explicit allowlist: replace the inverse-regex approach in
remediationIsSafe with a whitelist of exact or anchored-safe command patterns
(e.g., short, read-only commands) stored in an array (e.g., safeCommands or
safeCommandPatterns) and return true only when the input command matches one of
those entries (use startsWith/anchored regex for robust matching);
alternatively, remove global logic and mark safeUnderSandbox explicitly at each
remediation source where safety intent is known (update callers that rely on
remediationIsSafe to use the new allowlist function or the per-remediation
flag).
| const tscCheck = await runCheckCommand( | ||
| spawnCommand, | ||
| cwd, | ||
| 'repo.typecheck', | ||
| 'npm', | ||
| ['exec', 'tsc', '--', '--noEmit'], | ||
| 'Run `npm exec tsc -- --noEmit`.' | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the current npm CLI documentation, does npm exec tsc -- --noEmitinstall or prompt fortypescript when it is not already available locally? What supported non-install flag or pattern should be used for a deterministic read-only check instead?
💡 Result:
Yes. The npm CLI docs for npm exec say that if any requested packages aren’t present in the local project dependencies, npm will print a prompt and then install the requested packages into an npm-cache folder that’s added to PATH (the prompt can be suppressed with --yes or --no). (docs.npmjs.com)
So, npm exec tsc -- --noEmit is not a deterministic “read-only” check for local TypeScript availability—if tsc (as a package specifier) isn’t satisfied locally, npm may prompt and install something into its cache. (docs.npmjs.com)
For a deterministic non-install check, use the supported “decline install” switch and name the actual package that provides tsc:
npm exec --no --package=typescript -- tsc --noEmit--no-install exists only as a deprecated legacy alias and is converted to --no. (docs.npmjs.com)
Citations:
- 1: https://docs.npmjs.com/cli/v11/commands/npm-exec
- 2: https://docs.npmjs.com/cli/v11/commands/npm-exec
- 3: https://docs.npmjs.com/cli/v11/commands/npm-exec
🏁 Script executed:
# Find the runCheckCommand implementation and understand how it constructs the npm exec call
rg -A 20 "function runCheckCommand|const runCheckCommand" src/cli/launch.tsRepository: clduab11/codex-synaptic
Length of output: 808
🏁 Script executed:
# Get more context around lines 273-280 in launch.ts to see if there's error handling
sed -n '260,290p' src/cli/launch.tsRepository: clduab11/codex-synaptic
Length of output: 1326
Use npm exec --no --package=typescript -- to prevent installation or prompts.
When no typecheck script exists, npm exec tsc -- --noEmit will prompt and install TypeScript into an npm cache folder if not available locally. This breaks the determinism of the launch gate—it may hang on prompts, fail due to network issues, or install unexpectedly. According to npm CLI documentation, use the --no flag with an explicit --package=typescript to decline installation and perform a purely local check:
npm exec --no --package=typescript -- tsc --noEmit
Alternatively, verify TypeScript is available locally before running the fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/launch.ts` around lines 273 - 280, Replace the fallback command that
runs TypeScript in runCheckCommand to avoid npm prompting/installing: instead of
['exec', 'tsc', '--', '--noEmit'] invoke npm with the explicit no-install form
(i.e. use the equivalent of "exec --no --package=typescript -- tsc --noEmit") so
that spawnCommand is given a deterministic, local-only check; update the
arguments passed where 'repo.typecheck' is used (and/or add a preflight check
that TypeScript exists locally before calling runCheckCommand) to ensure no
automatic installs or interactive prompts occur.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cli/launch.ts (1)
344-351:⚠️ Potential issue | 🟠 MajorEnsure all failing launch steps are projected into
checks.This loop adds remediations to
fixesfor all failed steps, but the corresponding failures aren't projected intochecks. Steps likerepo.preflight,codex.auth,mcp.up, anddoctor.strictcan fail without a matching check entry explaining whyokisfalse.Per the learning, "treat launch as a hard gate—if
okisfalse, stop and return remediation commands." Automation consuming this JSON needs visibility into which step failed, not just remediation commands.♻️ Suggested approach
for (const step of report.steps) { + // Project every launch step into checks + const stepStatus = step.ok ? 'pass' : 'fail'; + checks.push({ + name: step.id, + status: stepStatus, + detail: step.details + }); + if (step.ok || !step.remediation) { continue; } for (const command of splitRemediationCommands(step.remediation)) { fixes.push({ why: `${step.id} gate failed`, command, safeUnderSandbox: remediationIsSafe(command) }); } }This ensures every launch step has a corresponding check entry regardless of pass/fail status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/launch.ts` around lines 344 - 351, The loop that only pushes remediation commands into fixes neglects to record failing steps in the checks array; update the logic in the report.steps iteration (the block that calls splitRemediationCommands and pushes into fixes) to also push a corresponding check entry into checks for any step where step.ok is false (e.g., checks.push({ id: step.id, ok: false, message: step.remediation || step.message || 'step failed' })), so every failed step like repo.preflight, codex.auth, mcp.up, doctor.strict is represented in checks even when remediations are emitted.
🧹 Nitpick comments (1)
src/cli/launch.ts (1)
208-365: Consider decomposing this function for maintainability.At 157 lines, this function handles many responsibilities: runtime checks, repo checks, daemon health, MCP config, worktree validation, and optional engine detection. While the logic is linear and readable, extracting logical groups into smaller helper functions (e.g.,
runRuntimeChecks,runRepoChecks,runInfraChecks) would improve testability and make future changes less risky.This isn't urgent—the current implementation works—but worth considering as the launch gate evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/launch.ts` around lines 208 - 365, The function buildLaunchStrictJsonReport is too large and mixes concerns; split it into smaller helpers to improve maintainability and testability: extract runtime checks (nodeCheck, npmCheck, depsCheck, optionalEngines detection) into runRuntimeChecks, repo-related checks (readPackageScripts, build/typecheck/test checks) into runRepoChecks, and infrastructure/daemon/mcp/worktree handling into runInfraChecks (or similar). Each helper should accept the current parameters used in the function (spawnCommand, cwd, report) and return the relevant slices (checks array, fixes array, and capability additions) so buildLaunchStrictJsonReport composes their results, updates capabilities, and computes ok/nextActions as before; refer to symbols buildLaunchStrictJsonReport, runCheckCommand, readPackageScripts, detectOptionalEngines, getDaemonPaths, splitRemediationCommands, and remediationIsSafe to locate logic to move.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli/launch.ts`:
- Around line 344-351: The loop that only pushes remediation commands into fixes
neglects to record failing steps in the checks array; update the logic in the
report.steps iteration (the block that calls splitRemediationCommands and pushes
into fixes) to also push a corresponding check entry into checks for any step
where step.ok is false (e.g., checks.push({ id: step.id, ok: false, message:
step.remediation || step.message || 'step failed' })), so every failed step like
repo.preflight, codex.auth, mcp.up, doctor.strict is represented in checks even
when remediations are emitted.
---
Nitpick comments:
In `@src/cli/launch.ts`:
- Around line 208-365: The function buildLaunchStrictJsonReport is too large and
mixes concerns; split it into smaller helpers to improve maintainability and
testability: extract runtime checks (nodeCheck, npmCheck, depsCheck,
optionalEngines detection) into runRuntimeChecks, repo-related checks
(readPackageScripts, build/typecheck/test checks) into runRepoChecks, and
infrastructure/daemon/mcp/worktree handling into runInfraChecks (or similar).
Each helper should accept the current parameters used in the function
(spawnCommand, cwd, report) and return the relevant slices (checks array, fixes
array, and capability additions) so buildLaunchStrictJsonReport composes their
results, updates capabilities, and computes ok/nextActions as before; refer to
symbols buildLaunchStrictJsonReport, runCheckCommand, readPackageScripts,
detectOptionalEngines, getDaemonPaths, splitRemediationCommands, and
remediationIsSafe to locate logic to move.
|
@copilot , Verify each finding against the current code and only fix it if needed. Duplicate comments:
Nitpick comments:
|
Motivation
Description
buildLaunchStrictJsonReportand extended thelaunchcommand to emit the required JSON shape withok,capabilities,checks,fixes, andnextActions.safeUnderSandbox.src/adapters/optional-engines.ts) to detectrufloandruv-fannand downgrade them to non-blocking warnings.codex-synaptic project attach <path>andcodex-synaptic project list) that store attached-project metadata in the daemon state directory (outside worktrees) viasrc/cli/project-attach.ts.README.md, replaced rootAGENTS.mdwith a launch-gated operating contract, added.codex/config.toml,.codex-synaptic/project.json+ schema, and new docs (docs/launch-gate.md,docs/codex-usage.md,docs/sandbox-and-rules.md).CODEX_AUTO_LINK=0in spawned checks to avoid interactive link hooks during automated runs and keep launch strict semantics deterministic.Testing
CODEX_AUTO_LINK=0 npm run buildwas run and completed successfully (TypeScript build + dev CLI link step passed).npm run test -- tests/cli/launch.test.tsran and all tests passed (5/5).node dist/cli/index.js project attach . --jsonran and returned JSON includingAGENTS.md,.codex/config.toml, and.codex-synaptic/project.jsonfor the repository.CODEX_AUTO_LINK=0 node dist/cli/index.js launch --strict --skip-codex-auth --mcp-profiles none --jsonwas executed in this environment but did not produce a complete JSON output before the interactive timeout and was terminated; core unit/build checks above succeeded and the launch JSON builder is exercised by unit tests, so follow-up e2e timeout hardening is listed in the backlog.Codex Task