fix: support Streamable HTTP MCP servers for Codex CLI (closes #1260)#1262
Conversation
…uration and handling
… transport support
masaishi
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR updates the Codex CLI adapter so apm install can emit Streamable HTTP (remote) MCP server entries into Codex config.toml, while continuing to skip SSE remotes with an updated warning message. It also expands conflict detection to recognize Codex TOML “flat-key” server entries that are configured via url, and removes a README statement claiming Codex cannot use remote MCP servers.
Changes:
- Codex adapter: generate
mcp_servers.<name>TOML entries for remotestreamable-httpMCP servers (includinghttp_headers), and skip SSE remotes via formatter signaling. - Conflict detector: treat
mcp_servers.<name>flat-key configs containingurlas existing servers (not justcommand/args). - Tests + README: add unit coverage for streamable-http + conflict detection, and remove misleading README wording about Codex remote MCP support.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/adapters/client/codex.py |
Adds remote Streamable HTTP config emission for Codex and SSE-skip path via _format_server_config() returning None. |
src/apm_cli/core/conflict_detector.py |
Extends Codex flat-key server detection to include url-based configs. |
tests/unit/test_mcp_client_factory.py |
Updates SSE skip test to assert _rich_warning content; adds streamable-http formatting + install tests. |
tests/unit/test_conflict_detection.py |
Adds coverage for detecting Codex flat-key remote url entries. |
tests/unit/integration/test_mcp_integrator.py |
Adds coverage ensuring self-defined streamable-http builds a remotes entry. |
README.md |
Removes the statement that Codex CLI does not support remote MCP servers. |
| remote = self._select_remote_with_url(remotes) or remotes[0] | ||
| server_name = server_info.get("name", "") | ||
| if (remote.get("transport_type") or "").strip() == "sse": | ||
| _rich_warning( | ||
| f"Skipping MCP server '{server_name}' for Codex CLI: SSE transport " |
There was a problem hiding this comment.
APM always builds a single-element remotes list (mcp_integrator.py:216: info["remotes"] = [remote]), so the proposed “first SSE, later streamable-http” scenario cannot occur.
Additionally, the _select_remote_with_url(remotes) or remotes[0] pattern is already identical across the other adapters:
copilot.py:541gemini.py:131vscode.py:362
The recommendation to “mirror the other adapters’ validation” is therefore self-defeating, because the referenced implementations use the exact same logic.
| remote_config = { | ||
| "url": (remote.get("url") or "").strip(), | ||
| "id": server_info.get("id", ""), | ||
| } |
There was a problem hiding this comment.
codex.py:617-627’s _select_remote_with_url contractually never returns a remote with an empty URL.
Writing id as an empty string is intentional and consistent with copilot.py:565. This follows the conflict_detector schema convention (introduced in commit 6a44d74).
Omitting the key would break conflict detection behavior.
| # Generate server configuration with environment variable resolution | ||
| server_config = self._format_server_config(server_info, env_overrides, runtime_vars) | ||
|
|
||
| # Skip if formatter signaled "unsupported" (e.g. SSE remote on Codex) | ||
| if server_config is None: | ||
| return False |
| "headers": [ | ||
| {"name": "Authorization", "value": "Bearer ghp_xxx"}, | ||
| {"name": "X-Figma-Region", "value": "us-east-1"}, | ||
| ], | ||
| } |
There was a problem hiding this comment.
GitHub’s PAT format requires:
ghp_ + 36 alphanumeric characters
…g docstring to include None
sergio-sisternes-epam
added
panel-review
danielmeppiel
added
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Solid feature addition; one correctness gap (remotes+packages silently falls through to package path) and a missing type hint are the main items. None-as-sentinel is acceptable at this scope but worth noting. |
| CLI Logging Expert | 0 | 1 | 1 | SSE warning migration is correct and well-formed; one recommended follow-up on the adjacent success path, one nit on symbol= consistency. |
| DevX UX Expert | 0 | 2 | 2 | Silent success on streamable-HTTP install and URL-less fallback are the two real gaps; SSE deprecation message is good; remotes+packages ambiguity is low risk but worth a nit. |
| Supply Chain Security Expert | 1 | 2 | 1 | No URL scheme validation lets a compromised registry write non-HTTPS/localhost URLs to Codex config; resolved auth tokens in http_headers land on disk without explicit user consent warning. |
| OSS Growth Hacker | 0 | 2 | 2 | Unblocking Codex remote-MCP is a genuine "works with" expansion worth amplifying, but the README silently removes the disclaimer instead of adding a positive claim -- a missed conversion moment. |
| Auth Expert | 0 | 2 | 2 | Resolved Authorization headers are written plaintext to config.toml with no user-facing disclosure; static registry tokens pass through verbatim; otherwise no credential-leak vectors found. |
| Doc Writer | 0 | 2 | 2 | README removal is accurate but purely subtractive; two doc pages have stale or incomplete Codex remote-MCP coverage that will mislead users trying --transport http with Codex. |
| Test Coverage Expert | 0 | 1 | 1 | TOML write/read round-trip is covered at fixture tier; two unit-tier gaps remain: all-empty-URL fallback silently writes url='' and unknown transport_type silently falls through to streamable-HTTP path. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Enforce HTTPS-only scheme before writing
urlto Codex TOML; raise a hard error for any other scheme. -- A compromised registry entry can write(redacted)(redacted) or localhost URLs to a config file Codex reads at runtime. This is a persistent write vector with no mitigation today. Must close before merge. - [Auth Expert] Emit an explicit named warning when a resolved
Authorization/X-Api-Keyheader is about to be written to disk in plaintext. -- Auth-expert and supply-chain-security converged independently: static or fully-resolved tokens from the registry land inconfig.tomlsilently. Near-blocking given dual-lens convergence. - [DevX UX Expert] Guard the empty-URL fallback: raise
ValueErrorwhen every remote has an empty URL instead of writingurl=''and returningTrue. -- Test-coverage-expert confirmed no test asserts this path. The silent write ofurl=''produces an unusable Codex config with a false-success return. - [CLI Logging Expert] Replace bare
print()on the streamable-HTTP success path with_rich_success(); add_rich_info()whenremotes+packagessilently routes to packages. -- The SSE path was just migrated to_rich_warning; the adjacent success branch still uses bareprint(), breaking the traffic-light contract on the same code path. - [OSS Growth Hacker] Add a CHANGELOG entry for the Codex + streamable-HTTP unlock and add an affirmative README line where the disclaimer was removed. -- The disclaimer removal is purely subtractive. A CHANGELOG bullet seeds external mentions and upgrade motivation; a positive README claim converts Codex users who scan before trusting
--transport http.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<Abstract>>
+target_name: str
+mcp_servers_key: str
+get_config_path() str
+update_config(updates) bool
+configure_mcp_server(...) bool
}
class CodexClientAdapter {
<<ConcreteAdapter>>
+configure_mcp_server(...) bool
+_format_server_config(info, env, vars) dict | None
+_select_remote_with_url(remotes) dict | None
+_select_best_package(packages) dict
+update_config(updates) bool
}
class CopilotClientAdapter {
<<ConcreteAdapter>>
+_format_server_config(info, env, vars) dict
}
class CursorClientAdapter {
<<ConcreteAdapter>>
+_format_server_config(info, env, vars) dict
}
class MCPConflictDetector {
<<Collaborator>>
+get_existing_server_configs() dict
}
class SimpleRegistryClient {
<<Collaborator>>
+find_server_by_reference(ref) dict
}
MCPClientAdapter <|-- CodexClientAdapter : extends
MCPClientAdapter <|-- CopilotClientAdapter : extends
MCPClientAdapter <|-- CursorClientAdapter : extends
CodexClientAdapter *-- SimpleRegistryClient : owns
CodexClientAdapter ..> MCPConflictDetector : reads via
class CodexClientAdapter:::touched
class MCPConflictDetector:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install --transport http"]) --> B["configure_mcp_server()\ncodex.py:120"]
B --> C{"server_info cached?"}
C -- no --> D["[NET] registry_client.find_server_by_reference()"]
C -- yes --> E["use cached server_info"]
D --> E
E --> F["resolve config_key from server_name / server_url"]
F --> G["_format_server_config(server_info, env, vars)\ncodex.py:190"]
G --> H{"_raw_stdio present?"}
H -- yes --> I["return stdio config dict"]
H -- no --> J{"remotes and NOT packages?"}
J -- no --> K{"packages empty?"}
K -- yes --> L["raise ValueError (incomplete config)"]
K -- no --> M["_select_best_package() -> package dict"]
M --> N["build command/args/env config"]
N --> I
J -- yes --> O["_select_remote_with_url(remotes)"]
O --> P{"transport_type == 'sse'?"}
P -- yes --> Q["_rich_warning() + return None"]
P -- no --> R["build {url, id} remote_config"]
R --> S{"headers present?"}
S -- yes --> T["_resolve_variable_placeholders() for each header"]
T --> U["remote_config['http_headers'] = headers"]
S -- no --> V["return remote_config"]
U --> V
G --> W{"result is None?"}
I --> W
Q --> W
V --> W
W -- yes --> X["return False (skip, warning already emitted)"]
W -- no --> Y["[FS] update_config({config_key: server_config})\nwrites .codex/config.toml"]
Y --> Z["[I/O] MCPConflictDetector reads url/command/args keys\nconflict_detector.py:118-120"]
Z --> AA(["return True"])
Recommendation
Hold for the HTTPS scheme guard (supply-chain-security blocking finding) and pair it with the auth-disclosure warning (auth-expert + supply-chain-security convergence). Both are small diffs -- likely a single function and a one-line pre-write check. Once those two land, the empty-URL guard and the bare-print fix (each a one-liner) should come along in the same revision since they are already identified and trivially cheap. With those four items in the diff, this PR is clean to merge and the follow-up issue queue (doc updates, CHANGELOG, GitHub-server parity, remotes+packages comment) can be tracked as a post-merge milestone.
Full per-persona findings
Python Architect
-
[recommended] Mixed remotes+packages server silently ignores the streamable-HTTP remote and installs as stdio at
src/apm_cli/adapters/client/codex.py:222
The guardif remotes and not packagesonly enters the remote-config branch when there are no packages. A server that ships both a remote endpoint and a package falls through to the package path without any warning. If the operator intended a streamable-HTTP connection, they will silently get a local install instead.
Suggested: Add a precedence rule with a comment, or add_rich_warningwhen falling through to packages path when remotes also exists. -
[nit]
_select_remote_with_urllacks a type annotation on its return value atsrc/apm_cli/adapters/client/codex.py:616
The docstring saysdict or Nonebut no-> dict | Nonehint is present. All other helpers on this class carry return-type hints.
Suggested:def _select_remote_with_url(remotes: list[dict]) -> dict | None: -
[nit] None-as-sentinel from
_format_server_configdiverges from other adapters which return{}for skip
Copilot and Cursor return{}(empty dict) as the nothing-to-do signal; Codex now returnsNonefor the SSE case. Two conventions for the same concept in one inheritance tree.
Suggested: Add a one-line docstring note toMCPClientAdapter._format_server_configclarifying the sentinel contract.
CLI Logging Expert
-
[recommended] Streamable-HTTP success path still uses bare
print()immediately after the changed block atsrc/apm_cli/adapters/client/codex.py
The SSE path was just migrated to_rich_warning, but the success branch directly below it is left with a bareprint(). A user installing a streamable-HTTP server sees uncolored, un-symbolled output while a warning fires in yellow with[!]. This breaks the traffic-light contract for the same code path.
Suggested: Replaceprint(f"Successfully configured MCP server '{config_key}' for Codex CLI")with_rich_success(...). -
[nit]
symbol="warning"is valid but inconsistent with sibling adapters
Every other_rich_warningcall in vscode.py, copilot.py, and claude.py omitssymbol=entirely.
Suggested: Either adoptsymbol="warning"across all_rich_warningcall sites or drop it here to match the majority pattern.
DevX UX Expert
-
[recommended] Silent success: no confirmation emitted after streamable-HTTP install at
src/apm_cli/adapters/client/codex.py
After_format_server_configreturnsremote_configandupdate_configsucceeds, nothing tells the user the server was installed. Runningapm install --mcp server --transport httpproduces no output on the happy path -- indistinguishable from a silent skip.
Suggested: Emit_rich_success(f"MCP server '{server_name}' installed for Codex CLI (streamable-http)."). -
[recommended] Empty-URL fallback:
remotes[0]with no URL silently produces a broken config atsrc/apm_cli/adapters/client/codex.py
When every remote lacks a URL,_select_remote_with_urlreturns None and falls back toremotes[0], writingurl=''to Codex TOML.configure_mcp_serverreturns True but the entry is unusable.
Suggested:if not (remote.get('url') or '').strip(): raise ValueError(f"MCP server '{server_name}' has no usable URL for streamable-http transport.") -
[nit]
remotes+packagescase silently routes to packages-only without acknowledgment
Suggested: Add_rich_info(f"MCP server '{server_name}' has both remote and package variants; using local package for Codex CLI."). -
[nit] SSE deprecation warning missing the corrected command
Suggested: AppendRun: apm install --mcp <server> --transport streamable-httpto the warning string.
Supply Chain Security Expert
-
[blocking] No URL scheme validation -- registry can inject
(redacted)(redacted) or localhost URLs atsrc/apm_cli/adapters/client/codex.py:235
Theurlfield is taken directly from the registry response and only stripped before being written to Codex's TOML config. No scheme allow-list, no localhost/loopback block, no HTTPS enforcement. A compromised registry entry can supply(attacker.com/redacted) (MITM),(redacted) or(127.0.0.1/redacted) (SSRF). The attack is persistent -- written to a file Codex reads at runtime. *Suggested:*if not url.startswith('https://'): raise ValueError(f'Remote MCP URL must use HTTPS: {url}')` -
[recommended] Resolved auth tokens written to disk config without explicit user disclosure at
src/apm_cli/adapters/client/codex.py:246-248
_warn_input_variablesonly fires for unresolved\$\{input:...}placeholders. Fully-resolvedAuthorization: Bearer (token)values are written to TOML silently.
Suggested: Add a check for headers matchingAuthorization/X-Api-Keyand emit an explicit warning naming the config path. -
[recommended] Empty-string fallback for
idsilently breaks UUID-based conflict detection atsrc/apm_cli/adapters/client/codex.py:236
server_info.get('id', '')falls back to empty string. The conflict detector then matches any other server whoseidis also''-- false-positive conflict blocks.
Suggested: Skip writing theidkey when the value is falsy. -
[nit] Conflict detector
urlheuristic could match non-MCP TOML entries atsrc/apm_cli/core/conflict_detector.py:119
OSS Growth Hacker
-
[recommended] README removes limitation but adds no positive claim about what is now supported at
README.md
Suggested: Add an affirmative line near the Codex install example. -
[recommended] No CHANGELOG entry for this capability unlock at
CHANGELOG.md
Suggested: "Codex CLI now supports streamable-HTTP remote MCP servers;--transport httpno longer skips Codex." -
[nit] SSE deprecation warning should be mirrored in docs FAQ.
-
[nit] Works-with section could note all clients support both local and hosted MCP transports.
Auth Expert
-
[recommended] No disclosure to user that resolved Authorization headers are persisted in plaintext at
src/apm_cli/adapters/client/codex.py
_warn_input_variablesfires only for unresolved placeholders -- silent when the token is fully resolved. -
[recommended] Static tokens from registry entries are written verbatim with no warning
_resolve_variable_placeholderspasses values through unchanged when they contain no placeholder. A cheap regex check on resolved header values would catch raw tokens (ghp_,github_pat_, etc.). -
[nit] No GitHub-server detection unlike Cursor adapter
Cursor injects a live token fromGitHubTokenManagerand refuses registry-suppliedAuthorizationoverrides. Codex has no equivalent. -
[nit] No credential-leak via logs -- confirmed clean.
_log.debuglogs only the file path; no resolved token value surfaces in output.
Doc Writer
-
[recommended]
install-mcp-servers.mdtoken injection section does not mention Codex adapter atdocs/src/content/docs/consumer/install-mcp-servers.md
The section scopes automaticAuthorization: Bearerinjection to "Copilot CLI adapter" only. Users installing GitHub MCP against Codex with--transport httphave no guidance on whether token injection applies. -
[recommended]
targets-matrix.mdCodex section does not list supported MCP transport types atdocs/src/content/docs/reference/targets-matrix.md
With--transport httpnow working for Codex, the section is materially incomplete.
Suggested: Add: "MCP transports.stdioandstreamable-http(remote). See Install MCP servers for transport selection." -
[nit] README removes limitation note but adds no positive replacement.
-
[nit] No documentation captures SSE-to-streamable-http migration path.
Test Coverage Expert
-
[recommended]
_select_remote_with_urlall-empty-URL fallback writesurl=''to TOML with no test asserting the outcome attests/unit/test_mcp_client_factory.py
When every remote has an empty URL, the fallback path writesurl=''to the Codex TOML and returns True. Confirmed missing via grep.
Proof (missing at unit):tests/unit/test_mcp_client_factory.py::test_configure_mcp_server_all_remotes_have_empty_url-- proves: apm install of a remote-only server with no resolvable URL either fails loudly or writes a non-empty url [devx, secure-by-default] -
[nit] Unknown
transport_type(e.g. "websocket") silently falls through to streamable-HTTP path with no test
Proof (missing at unit):tests/unit/test_mcp_client_factory.py::test_format_server_config_unknown_transport_treated_as_streamable_http-- proves: unrecognised transport_type produces a known, documented outcome [devx]
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- fix: support Streamable HTTP MCP servers for Codex CLI (closes #1260) #1262
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #1262
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by PR Review Panel for issue #1262 · ● 3.3M · ◷
github-actions
Bot
removed
the
panel-review
|
Maintainer sweep [2026-05-15]: panel still |
|
@danielmeppiel |
PR-shepherd review (automated by orchestrator session)Verdict: ready-to-merge Root-cause alignmentALIGNED. The PR cleanly addresses the root cause of #1260.
Specialist panel findings (material only)The full 8-persona panel ran on 2026-05-14 (see prior comment by
None of the remaining items are blockers individually or in E2E coverageGOOD. The PR adds the right unit-tier scenarios for the changed
Missing-but-not-blocking (test-coverage-expert prior):
Empirical reproYES. Checked out
Behaviour matches the PR description and closes #1260 at the root. Suggested follow-upsTo file as separate issues post-merge (do NOT block this PR):
Docs checkREADME L155 status: REMOVED in this PR (
Orchestrator session: danielmeppiel/triage-issues-1363-1364-1366 |
…oft#1262) Six non-blocking follow-ups from PR microsoft#1262 shepherd review: 1. Auth-header disclosure (audit): no leak path found. _warn_input_variables in base.py logs only the ${input:VAR_ID} identifier, never the header value. Header values flow value -> http_headers dict -> toml.dump only. No code change required. 2. _rich_success on success: replace bare print() with green/bold _rich_success() to mirror Claude adapter pattern. Covers both stdio and streamable-http registration paths. 3. Empty-URL guard: reject remote entries whose url is empty / whitespace with an explicit 'has an empty url' warning, instead of falling through to a misleading 'no scheme' urlparse warning. 4. Hybrid (remotes + packages) precedence: add a code comment + _log.debug line documenting that Codex prefers the stdio package when a registry server publishes both. Behavior unchanged; the existing test_configure_mcp_server_hybrid_accepted test already asserts it. 5. CHANGELOG: add a [Unreleased] / Fixed bullet describing Codex streamable-http MCP support, with refs (microsoft#1260, microsoft#1262). 6. Docs sync: audited docs/src/content/docs/. No stale 'Codex does not support remote MCP' claim remained; README L155 was already removed in commit 4d7b017. No docs edit required. New tests in tests/unit/test_mcp_client_factory.py: - test_format_server_config_streamable_http_rejects_empty_url (covers '', ' ', None via subTest) - test_configure_mcp_server_streamable_http_emits_rich_success - test_configure_mcp_server_stdio_emits_rich_success - test_configure_mcp_server_hybrid_logs_precedence Verification: uv run --extra dev ruff check src/ tests/ # silent uv run --extra dev ruff format --check src/ tests/ # silent uv run --extra dev pytest tests/unit/test_mcp_client_factory.py tests/test_codex_empty_string_and_defaults.py tests/test_codex_docker_args_fix.py tests/unit/test_codex_runtime.py -> 45 passed, 6 subtests passed Closes follow-ups raised in microsoft#1262 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Polish follow-ups landed -- ready to mergePushed Addresses the six follow-ups from the shepherd review at #1262 (comment):
Test deltas
Local verification: @masaishi -- thanks for the PR. Handing back to maintainers for merge. |
…gates CI lint was failing the PR with R0801 (pylint duplicate-code). The duplication was introduced when main was merged into this branch: the recent MCP-client work on main left an identical `_select_best_package` method in both CodexClientAdapter and CopilotClientAdapter, each ~22 lines long. pylint --fail-on=R0801 catches this at the merge commit CI tests, so the PR cannot pass until the dup is resolved. Fix: hoist `_select_best_package` to the `MCPClientAdapter` base class (alongside the existing `_infer_registry_name` it already calls) and delete the duplicates from codex.py and copilot.py. Both call sites use `self._select_best_package(packages)`, so inheritance resolves the lookup transparently. Mirrors the in-flight refactor on `daf99e0f refactor(adapters): hoist _select_remote_with_url and _select_best_package to base` (not yet on main). Also: expand the canonical lint contract at `.apm/instructions/linting.instructions.md` to cover all seven CI Lint steps (ruff check, ruff format, YAML I/O guard, file-length guard, no raw `str(relative_to)`, pylint R0801, auth-signal lint) instead of only ruff. Adds the full `uv run ... && ...` mirror chain and calls out the merge-commit semantics (CI tests HEAD merged with main, so duplication on main can fail a clean PR diff). Apm compile flows the change into the generated AGENTS.md (gitignored at root) and into `.github/copilot-instructions.md`. Validation: * uv run --extra dev ruff check src/ tests/ -- clean * uv run --extra dev ruff format --check src/ tests/ -- clean * uv run --extra dev python -m pylint --disable=all --enable=R0801 \ --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ -- 10.00/10, exit 0 * bash scripts/lint-auth-signals.sh -- clean * uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py \ tests/integration/test_hook_root_source_drift_e2e.py -- 161 passed Pre-existing main-side test failures in `tests/unit/test_codex_adapter_compatibility.py` (introduced by #1262 / #1277 on main and tracked by the in-flight `32991fb6 fix(adapters): restore _resolve_env_placeholders shim and reject SSE remotes in tests` branch) are out of scope for this PR and will resolve when that follow-up lands on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ees (supersedes #1330, closes #1329) (#1392) * fix: stabilize root hook source ids * test: remove project-specific hook fixture names * fix(hooks): bound dependency hook source discovery * fix(hooks): reject symlinked dependency markers * test(hooks): subprocess-tier coverage + debug logs for #1329 - Add tests/integration/test_hook_root_source_drift_e2e.py: subprocess test that seeds stale .claude/settings.json + apm-hooks.json sidecar with old source-id, runs apm install, asserts entries heal to _local/<manifest-name> and user-owned hook entry survives. - Add two _log.debug lines in hook_integrator.py: - manifest-parse fallback in _get_root_local_package_name - heal count when stale same-content merged entries are removed - Update affected unit tests to read _apm_source from Claude sidecar (post-#1359 schema-strict layout). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(hooks): act on review-panel recommendations for #1329 follow-up Addresses the top recommended-tier follow-ups from the APM review panel on #1392 (#1392 (comment)). In-PR: * CHANGELOG: add [Unreleased] > Fixed entry with user-outcome framing, enumerating all five affected harnesses (Claude / Codex / Cursor / Gemini / Windsurf) and the explicit parenthetical that Copilot is unaffected because its hooks live in per-file namespaces rather than a shared merged config. (doc-writer + oss-growth-hacker convergence) * Integration coverage: parametrize the e2e heal regression test across all five merged-hook harnesses instead of Claude only. Uses an install-then-corrupt-then-reinstall pattern so the seeded entries match each target's on-disk shape (sidecar for Claude; in-file _apm_source for Codex / Cursor / Gemini / Windsurf), closing the silent-drift gap the test-coverage-expert flagged as highest-signal. * _safe_source_name hardening: collapse runs of 2+ dots to a single dot before stripping edges. No traversal risk today (the marker is JSON metadata), but guards any future caller that passes the name into a Path join. (supply-chain-security recommendation.) * Nits: include exception message (not just class name) in the manifest-unreadable debug log; replace O(n*m) list membership in the heal counter with an id-set; document the project_root parameter on _get_package_name; promote _get_root_local_package_name, _get_hook_source_marker, and _should_remove_prior_merged_entry to @staticmethod (they touch no instance state). The DependencySourceScanner extraction the python-architect proposed is explicitly out of scope for this PR per the panel's own guidance ("does not need to land in this PR but should not slip past the next meaningful touch to hook_integrator.py"). Validation: * uv run --extra dev ruff check src/ tests/ -- clean * uv run --extra dev ruff format --check src/ tests/ -- clean * uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py -- 156 passed * uv run --extra dev pytest tests/integration/test_hook_root_source_drift_e2e.py -- 5 passed (claude, codex, cursor, gemini, windsurf) * uv run --extra dev pytest tests/unit/integration/ tests/integration/test_hook_root_source_drift_e2e.py -- 1364 passed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(ci): resolve R0801 duplication; expand lint contract to all 7 CI gates CI lint was failing the PR with R0801 (pylint duplicate-code). The duplication was introduced when main was merged into this branch: the recent MCP-client work on main left an identical `_select_best_package` method in both CodexClientAdapter and CopilotClientAdapter, each ~22 lines long. pylint --fail-on=R0801 catches this at the merge commit CI tests, so the PR cannot pass until the dup is resolved. Fix: hoist `_select_best_package` to the `MCPClientAdapter` base class (alongside the existing `_infer_registry_name` it already calls) and delete the duplicates from codex.py and copilot.py. Both call sites use `self._select_best_package(packages)`, so inheritance resolves the lookup transparently. Mirrors the in-flight refactor on `daf99e0f refactor(adapters): hoist _select_remote_with_url and _select_best_package to base` (not yet on main). Also: expand the canonical lint contract at `.apm/instructions/linting.instructions.md` to cover all seven CI Lint steps (ruff check, ruff format, YAML I/O guard, file-length guard, no raw `str(relative_to)`, pylint R0801, auth-signal lint) instead of only ruff. Adds the full `uv run ... && ...` mirror chain and calls out the merge-commit semantics (CI tests HEAD merged with main, so duplication on main can fail a clean PR diff). Apm compile flows the change into the generated AGENTS.md (gitignored at root) and into `.github/copilot-instructions.md`. Validation: * uv run --extra dev ruff check src/ tests/ -- clean * uv run --extra dev ruff format --check src/ tests/ -- clean * uv run --extra dev python -m pylint --disable=all --enable=R0801 \ --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ -- 10.00/10, exit 0 * bash scripts/lint-auth-signals.sh -- clean * uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py \ tests/integration/test_hook_root_source_drift_e2e.py -- 161 passed Pre-existing main-side test failures in `tests/unit/test_codex_adapter_compatibility.py` (introduced by #1262 / #1277 on main and tracked by the in-flight `32991fb6 fix(adapters): restore _resolve_env_placeholders shim and reject SSE remotes in tests` branch) are out of scope for this PR and will resolve when that follow-up lands on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(adapters): restore _resolve_env_placeholders shim and reject SSE remotes in tests PR #1277 removed the _resolve_env_placeholders legacy wrapper from adapters and softened Codex remote-server rejection (https URLs are now accepted as streamable-http), but did not update the phase-3 and compatibility test suites. The result was 6 pre-existing test failures on Build & Test that this PR's Windows-targeted changes inherited. - Restore _resolve_env_placeholders on MCPClientAdapter as a thin delegate to _resolve_variable_placeholders (empty runtime_vars). External callers and 4 test cases rely on the legacy name. - Update test_returns_false_for_remote_only_server in both codex test files to use an SSE-transport remote, which is still the rejection contract post-#1277 (streamable-http is now accepted). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Koichi Takahashi <tkou15.hi@gmail.com> Co-authored-by: Koichi Takahashi <work@k1t.space> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Daniel Meppiel <copilot-rework@github.com>
Description
Codex CLI supports Streamable HTTP MCP servers, but
apm installwas skipping Codex for every remote MCP entry, andREADME.mddocumented this as intended behavior.This PR aligns the Codex adapter with the official Codex MCP spec:
~/.codex/config.tomlasmcp_servers.<name>entries withurl = "..."(andhttp_headerswhen provided).transport: streamable-http(SSE is deprecated by the MCP spec and absent from the Codex config schema).mcp_servers.<name>entries that useurl(previously onlycommand/argswere matched).README.md.Fixes #1260
Type of change
Testing
Tested locally
All existing tests pass
Added tests for new functionality (if applicable)
test_mcp_client_factory.pytest_conflict_detection.py_build_self_defined_info(test_mcp_integrator.py)