[kanfil]

What

adlc-cli mcp install [DIR] --agent NAME reads the project's .mcp.json (the standard declaration, PDR-138) and writes each named agent's MCP config in that agent's own shape. Implements ADR-455 / PDR-139 / PDR-140.

Update: team setup and team update now chain the install (user decision) — setup chains it after skills install, before the agent session, so the session starts with its MCP servers configured; update chains it before the repair pass. An MCP failure aborts the team command with exit 1 (fail-hard); the gap report runs in the chained context exactly as standalone. Bonus fix surfaced by the success-path test: cmdTeamRepair returned result.status || 1, coercing every successful repair's exit 0 into 1 — now ?? 1.

Design

• MCP_AGENTS adapter table beside EVENT_AGENTS in src/registry.mjs — two independent axes per row: format (entry shape) and var_syntax (string rewriting). Day-one agents: claude-code, opencode, codex, cursor. Agent five is a registry row plus a builder, not new engine code.
• Name-keyed ownership, no lock file. A name present in .mcp.json is tool-owned and regenerated on each run — including hand-edited bodies (updated). Overrides live in .mcp.json (disabled: true), never in the agent's file. A bare {"enabled": false} override in a target is regenerated, not kept.
• Honest refusals. An entry that cannot be fully expressed in a target is refused for that target only, with the reason (e.g. ${VAR} in a codex url — codex has no substitution; an env rename codex cannot reproduce). Never half-written, never eagerly resolved from the machine's environment. Refusals and target-level errors fail the run: exit 0 only when every named agent is configured. Exit codes are 0/1 only — 3 stays reserved for PAUSED.
• .mcp.json is never written. Three independent layers enforce it: the claude-code no-op (the standard is its file — every server reports kept), write-layer source-path guards in both merge paths, and byte-compare tests. disabled: true for claude reports "delete by hand".
• Codex TOML is line surgery with marker pairs (mirroring the events generator), never a whole-file re-serialisation. Malformed marker pairs fail closed with the file untouched.
• Copy-once gap report (PDR-140). After the install, drift against the team's .mcp.json (missing servers, unknown top-level keys "not carried", dropped ${VAR:-default} fallbacks) prints as warnings on stderr — never written, never blocking, never changing the exit code. A missing team source is reported, not degraded to "no gap".
• Merge into foreign files. Target files hold providers, plugins, and the user's own servers; every unrelated key is preserved.

Verification

• TDD throughout (subagent-driven development: fresh implementer + reviewer per task, whole-branch review, one branch fix round, one user-added chaining task with its own review).
• Suite: 815/815 pass + e2e gate green, rebased onto current main.
• Review-focus traces pinned end-to-end: no-command/no-url entry refused (never command:[null]); codex-only refusal with other targets still written; hand-edited tool-owned slot overwritten + reported; missing precondition loud; re-run byte-identical with kept; disabled→re-enabled two-run regression for both JSON and TOML.
• Chaining pinned: setup aborts before the agent session (real ADLC_MOCK_AGENT seam proves the session never started); update aborts before repair (marker-file observable); gap report runs in the chained context.
• Refusal contract single-sourced: translateEntry defers to describeRefusal, agreement matrix cross-products 22 specs × 7 rows including synthetic cross-axis rows.
• Security: prototype-chain rejection in getMcpAgent, __proto__ server-name pollution test, setOwn/hasOwn throughout.

Known limitations (deliberate)

• No PowerShell twin — the single cross-platform Node implementation is the point (PDR-139).
• JSON targets normalised to 2-space indent on write; TOML foreign whitespace runs collapsed.
• An init-options.json that records no agent now fails team update loudly at the chained install (previously such a hand-maintained file completed) — disclosed in the CHANGELOG.
• The GITIGNORE_RULES_ALLOWLIST flip (tracking .mcp.json) is a recorded obligation in adlc-team-skills (PR #105) — deliberately out of scope here.
• PDR-139 amendment line (team-setup as trigger via CLI chaining, skill stays precondition) to follow in adlc-workspace MR !51.

[kanfil]

factory-review verdict: not-merge-ready

Reviewed revision: head bf487a8f693e4ec15c80dba985ce1439cf57bb58 · base f00f274f8e0707ab3c3d2cedf795f8d1327e0e19 · merge-base f00f274f (fully rebased). Reviewed from a detached checkout at the exact head (factory-review-bf487a8), not from a rendered diff.

Policy: the repo has no REVIEW.md, so the factory-review default policy was applied: passes bugs, security, spec-compliance (Important) and quality (Nit), against ADR-455 + PDR-138/139/140.

Verdict: 1 Important finding is open, so the PR is blocked from merge under the review policy until it is resolved. Everything else is in strong shape — see “What passed” below.

---

🔴 Important — type: "sse" entries are silently written to opencode in a transport it cannot speak (PDR-138 promise / refusal contract)

Where: src/mcp/translate.mjs:239-241 (the sse refusal is codex-only) and src/mcp/translate.mjs:273-280 (buildOpenCode drops spec.type for url entries and always writes {type: "remote", …}).

Repro (real registry rows, exact head):

$ node -e '…translateEntry(sseSpec, MCP_AGENTS[name])…'
codex    → REFUSED: sse is not expressible in codex entries: its remote transport is StreamableHttp, not SSE
opencode → {"type":"remote","enabled":true,"url":"https://example.com/sse","headers":{"Authorization":"Bearer {env:TOKEN}"}}
cursor   → {"type":"sse","url":"https://example.com/sse","headers":{"Authorization":"Bearer ${env:TOKEN}"}}

Evidence that opencode cannot consume it:

• OpenCode's documented remote schema (opencode.ai/docs/mcp-servers → Remote) is type/url/enabled/headers/oauth/timeout — there is no sse type.
• Code search of sst/opencode: SSEClientTransport → 0 matches. OpenCode's remote MCP client speaks streamable HTTP only.

Impact: an .mcp.json declaring "type": "sse" (a real legacy shape — e.g. the old GitHub MCP /sse endpoint) lands in opencode.json as a remote entry that fails to connect at agent runtime, while mcp install reports it added/updated and exits 0. That is exactly the silent-failure class PDR-138 exists to eliminate ("the bug being fixed is reporting success while [the config is wrong]"), and exactly what the codex refusal one branch above prevents. The PR's own contract says an entry that cannot be fully expressed in a target is refused for that target and reported — opencode is the one target where that promise is not kept for sse.

Test gap: tests/mcp-translate.test.mjs pins "codex … refuses sse" and "cursor keeps the http/sse type", but has no opencode-sse case — the matrix cross-products the axes, yet this row×spec combination goes unasserted.

Required correction: extend the sse refusal to the opencode row (same "cannot be fully expressed" wording — cursor's keep-sse is correct and needs no change), pin it with a test in the agreement matrix, and carry the codex-style reason so the report explains why. If opencode is believed to negotiate sse endpoints, that belief needs a source and a pinning test instead.

---

💬 Nit — server names with newlines/control characters produce corrupt .codex/config.toml

Where: src/mcp/install.mjs:376-377 (beginMark/endMark interpolate the raw server name into marker comment lines) and src/mcp/install.mjs:398-400 (tomlString escapes only \ and ", not newlines or control characters).

A .mcp.json server named with a JSON \n escape writes a raw newline into the marker comment (line injection into the generated TOML) and/or into a quoted table key, leaving config.toml unparseable for codex — the whole-file failure mode the malformed-marker guard (install.mjs:249-252) exists to prevent. Foreign user content is preserved (nothing is lost), and the trigger requires a pathological .mcp.json, so this is defensive-hardening rather than data loss — but the fix is cheap: reject control characters in server names (or escape them in tomlString and sanitize the marker names) and fail closed like every other malformed-input path. JSON targets are unaffected (JSON.stringify escapes).

💬 Nit — no REVIEW.md in the repository root

Every review run against this repo falls back to the factory default policy. Committing a REVIEW.md (even the default template, specialized with this repo's skip lists — tests/e2e-node-gate.mjs fixtures, generated surfaces) would make the review contract explicit and versioned.

---

What passed (verified, not assumed)

• Suite green at the exact head: 815/815 unit tests + e2e node gate pass in the detached checkout (reproduced locally; matches the PR's claim).
• Spec-compliance vs ADR-455 / PDR-138/139/140: name-keyed ownership without a lock file; hand-edited owned bodies regenerated and reported; .mcp.json never written (claude no-op + write-layer source-path guards in both merge paths); refusals fail the run and name agent/target/reason; exit contract is 0/1 only (3 untouched); gap report is stderr-only, never blocking, and reports an unavailable team source instead of degrading to "no gap"; disabled maps correctly per target (off switch / removal / no-op).
• Codex field mapping checked against the official Codex config reference: env_vars, env_http_headers, bearer_token_env_var, http_headers, enabled, url are all real mcp_servers.<id> fields — the literal-header http_headers branch is correct.
• Security: prototype-chain rejection in getMcpAgent (Object.hasOwn), setOwn via defineProperty on all writes, __proto__ server-name test present; no shell invocation, no secret material in logs or reports (only variable names are printed).
• TOML safety: marker-pair line surgery with bounded scanning; unterminated/orphan marker pairs fail closed with the file untouched; foreign file content preserved byte-for-byte when unchanged.
• Chaining: team setup runs the install after skills install and before the agent session; team update before the repair; the no-agent init-options behavior change and the cmdTeamRepair || 1 → ?? 1 fix are both disclosed in the CHANGELOG; README/help/dispatch (--agents alias) all wired.
• Quality: TDD structure with the review-focus traces the PR body claims are all present and pinned (no-command/no-url refusal, codex-only refusal with other targets still written, hand-edit overwrite, missing precondition, byte-identical re-run, disabled→re-enabled for JSON and TOML).

---

Separation of duties: this review neither approves nor merges. Merge-readiness is carried by the marker above (outcome=not-merge-ready) and is re-evaluated on any new head — a fix push invalidates this verdict and triggers a full re-review.

[kanfil]

self-heal iter 0 — adversarial review (Round 1 + Round 2 + synthesis)

Three-persona Round 1 (Auditor / Adversary / Pragmatist, fresh detached checkout at bf487a8f), Round 2 cross-examination (16/16 findings independently validated, 0 challenges except P8 self-downgrade, 0 adds), deterministic synthesis (synthesize-findings.sh). Governing contract: ADR-455, PDR-138/139/140. Suite green at head: 815/815 + e2e gate.

Synthesis tags — fix gate

ID	Severity	Tag	Title
S2	Important	cross-validated	tomlString writes raw control chars/newlines as invalid TOML, reported as success
A1	Important	consensus	mergeToml unguarded readFileSync — unreadable .codex/config.toml crashes past the error-reporting contract
S1	Important	consensus	Newline in a server name injects lines into .codex/config.toml via raw marker-comment interpolation
S3	Important	consensus	Symlinked target bypasses the source-write guard (resolve ≠ realpath) — .mcp.json can be rewritten
R1	consensus	Important	type: "sse" silently written to opencode in a streamable-HTTP-only transport
S4	Nit (advisory)	consensus	CR/LF in server names forges stdout report lines
P3	Nit (advisory)	cross-validated	No-agent team update misreports as a dangling --agent requires a NAME flag
P1,P2,P4,P5,P6,P7	Nit (advisory)	consensus	Agent-coupling disclosure · "byte-for-byte" comment drift · tripled helpers · JSDoc alias · written[] naming · alias surface
P8	Nit (advisory)	disputed	flags.cwd asymmetry — --cwd is never parsed; dead code (self-downgraded to cosmetic)

The 5 tagged Important findings are actionable and go to the fix step. Nits stay advisory and are not fixed by the agent.

Review performed by factory-review self-heal (cross-runtime lane unavailable — degraded to same-runtime personas, disclosed). This agent neither approves nor merges.

[kanfil]

Fix — iteration 1 (inline execution; subagent lane unavailable — disclosed degradation)

Finding: Windows CI red on this PR's own chained tests (team update: chains mcp install before repair, team update: MCP failure aborts…) on all three Node versions; every other platform green.

Root cause — one root, three layers:

1. The test's npx PATH stub was a #!/bin/sh script. On Windows spawnNpxSync goes through cmd.exe (shell: true), which resolves commands via PATHEXT and cannot execute an extensionless script — so CI ran the real npx: a live network install that crashes on Node 18 (util.styleText is Node 20+) and ENOENTs on Node 20/22 (~/.codex/prompts doesn't exist on the runner).
2. The success path ends in cmdTeamRepair's bare spawnSync("bash", …) — on windows-latest Git's bin is not reliably on a Node-spawned process's PATH (documented in tests/e2e-windows-path.test.mjs).
3. team.mjs passed the raw Windows script path to Git Bash, which treats backslashes as escape characters — src/dispatcher.mjs established the forward-slash convention; team.mjs predated it.

Fix (f3e508b):

• tests/team-mcp-chain.test.mjs: an npx.cmd twin beside the POSIX stub (CRLF endings); Git for Windows' bash dir injected into the child's PATH (same lookup order as resolveBash: bare bash first, then PROGRAMFILES); both PATH/Path spellings set to the same value to kill env-block case-variant ambiguity; fail-open skip of the success-path test only when no bash exists at all (the repo's SKIP_NO_BASH convention — the runners always have one).
• src/commands/team.mjs: hand bash a forward-slash script path on win32 — the same convention dispatcher.mjs's sh branch uses. One line, pre-existing line whose adjacent return this PR already touched, exercised on Windows by this PR's own tests.

Verification: suite 825/825 + e2e green locally; CI green on all 10 jobs including windows-latest 18/20/22 at f3e508b.

[kanfil]

Converge — iteration 1 (independent verification at a fresh detached checkout of f3e508b)

• All 5 iteration-0 Important findings independently re-verified as fixed — 9/9 scenario checks PASS: R1 (opencode/codex refuse sse, cursor keeps it), S1 (newline marker key refused, file untouched), S2 (control chars escaped \n/\uXXXX, no raw control char in the block), A1 (unreadable target reported, no crash), S3 (symlink onto .mcp.json / outside the project / dangling all refused, source byte-identical).
• Iteration-1 finding (Windows CI red) verified fixed: all 10 CI jobs pass, including windows-latest 18/20/22.
• Full suite at the checkout: 825/825 + e2e gate green. One one-off provider-transport.test.mjs Node test-runner IPC deserialization crash on the first run — passes in isolation and on every re-run; pre-existing infra flake also observed at 430a24a, unrelated to the diff.
• No new findings introduced by the fix commit.

Finding-set comparison: iteration-1 set (1 finding) ⊂ iteration-0 set (5 actionable Important) → progress; none remaining.

Verdict: DONE → merge-ready review verdict follows.

[kanfil]

🟢 PASS — Ready for human review

PR #62 @ f3e508b — feat(mcp): adlc-cli mcp install — materialize .mcp.json into each agent's own config

Reviewed against ADR-455 + PDR-138/139/140 (REVIEW.md absent → default 4-pass policy: bugs/security/spec-compliance Important, quality Nit). Iteration 0 ran the adversarial trio (Auditor/Adversary/Pragmatist) with cross-examination and deterministic synthesis; iterations 0–1 ran the self-heal fix → independent converge loop.

Important findings: 0 open — all 5 from the adversarial round are fixed and pinned by tests (430a24a), and the Windows CI failure is fixed (f3e508b):

#	Finding	Fix
R1	sse servers silently written for opencode (unconnectable at agent runtime, reported as success)	refused — opencode/codex remote transport is streamable HTTP; cursor keeps native sse
S1	newline in a server name escapes the TOML marker comment → live TOML injection	refused per-server (marker keys must be one line)
S2	control characters in values brick the whole config.toml while reporting success	full \uXXXX escaping (\n \r \t by name)
A1	unreadable TOML target crashes the run, aborting the remaining named agents	target-level error, reported; write failures too
S3	symlinked target bypasses the source-write guard	guard judges real paths: onto .mcp.json / outside the project / dangling all refused
CI	Windows red on the PR's own chained tests (3-layer stub/bash/path root cause)	npx.cmd stub twin + Git-bash PATH injection + forward-slash script path

CI: all 10 jobs green — ubuntu/macos/windows × Node 18/20/22 + e2e.

Advisory nits (12, non-blocking, deliberately outside the fix gate):

• S4 and P1–P8 from the iteration-0 review — unchanged, see that review for detail.
• Pre-existing issues surfaced by the Windows diagnosis (not introduced by this PR):
  1. src/commands/skills.mjs:339 — cmdUpdate writes commands into commands_dir without mkdirSync; a real Windows user without ~/.codex/prompts crashes after a real skills install. A mkdirSync(dirname(filepath), { recursive: true }) would close it.
  2. cmdTeamRepair's bare spawnSync("bash") won't resolve bash on real Windows machines (Git bin not on PATH for GUI-spawned processes) and exits 1 silently; dispatcher.mjs's findLauncher pattern is the in-repo precedent for robust resolution.
  3. tests/provider-transport.test.mjs intermittently crashes the full suite with a Node test-runner IPC deserialization error (passes in isolation; observed at both the 430a24a and f3e508b checkouts).

This is a review verdict only — approval and merge remain with the code-owner.