Skip to content

[FEATURE] Support global OpenCode MCP installation - #3075

Closed
Christopher Lunt (tnulc) wants to merge 30 commits into
microsoft:mainfrom
tnulc:feat/opencode-global-install
Closed

Christopher Lunt (tnulc) wants to merge 30 commits into
microsoft:mainfrom
tnulc:feat/opencode-global-install

Conversation

@tnulc

Copy link
Copy Markdown

Adds global MCP installation support for OpenCode, including target paths, integration behavior, documentation, and tests.

Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved moderate issues affect OpenCode key handling, scope-aware cleanup, and custom-path cleanup.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 8 Low severity

Open (9)
What changed in this PR

Adds global OpenCode MCP installation with configurable configuration paths, scope-aware integration, tests, and documentation.

Changes:

  • Adds OPENCODE_CONFIG_DIR/XDG path resolution.
  • Enables global OpenCode detection, writes, and cleanup.
  • Updates tests, documentation, and changelog.
File Summary Final review notes
tests/​unit/​test_opencode_mcp.py Tests path resolution and global writes. No final comments.
tests/​unit/​test_global_mcp_scope.py Updates scope expectations. Nit (1 vote): Ensure OpenCode is tested with a present global config rather than filtered as workspace-only.
tests/​unit/​integration/​test_targets.py Tests global target resolution. No final comments.
src/​apm_cli/​integration/​targets.py Resolves configurable OpenCode roots. Moderate (1 vote): Use the canonical user-root helper for fallback prefixes so custom paths are cleaned up correctly.
src/​apm_cli/​integration/​opencode_paths.py Defines path precedence. Nit (3 votes): Register the module as the canonical owner and add the required static guard.
src/​apm_cli/​integration/​mcp_integrator.py Handles scope-aware cleanup. Moderate (3 votes): Derive effective user scope from both legacy and new parameters, with explicit project scope taking precedence.
src/​apm_cli/​integration/​mcp_integrator_install.py Detects global OpenCode configuration. Nit (2 votes): Include OpenCode in the no-runtime warning’s supported list.
src/​apm_cli/​adapters/​client/​opencode.py Supports global MCP writes. Moderate (1 vote): Set mcp_servers_key to mcp for conflict and ownership handling. Nit (4 votes): Update the stale module documentation about user-scope directory creation.
docs/​src/​content/​docs/​reference/​targets-matrix.md Documents OpenCode targets. Nit (2 votes): Fix the unresolved opencode_config_dir fragment link.
docs/​src/​content/​docs/​reference/​environment-variables.md Documents environment variables. Nits (1 vote each): Add the missing anchor and correct the OPENCODE_CONFIG_DIR/XDG auto-detection descriptions.
docs/​src/​content/​docs/​reference/​cli/​install.md Documents global installation. Nits (2, 1 votes): Fix the unresolved environment-variable fragment and update stale packaged skill guidance.
docs/​src/​content/​docs/​reference/​cli/​compile.md Documents global compilation. Nit (2 votes): Fix the unresolved opencode_config_dir fragment link.
docs/​src/​content/​docs/​producer/​compile.md Updates compile destinations. Nit (2 votes): Fix the unresolved opencode_config_dir fragment link.
docs/​src/​content/​docs/​integrations/​ide-tool-integration.md Documents MCP config paths. No final comments.
docs/​src/​content/​docs/​consumer/​install-mcp-servers.md Updates the MCP target matrix. No final comments.
docs/​src/​content/​docs/​concepts/​primitives-and-targets.md Updates target overview. No final comments.
CHANGELOG.md Records the feature. Nit (4 votes): Add the required PR-number suffix to the Unreleased entry.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/apm_cli/integration/mcp_integrator.py Outdated
Comment thread CHANGELOG.md Outdated
Comment thread docs/src/content/docs/producer/compile.md Outdated
Comment thread docs/src/content/docs/reference/cli/compile.md Outdated
Comment thread docs/src/content/docs/reference/cli/install.md Outdated
Comment thread docs/src/content/docs/reference/targets-matrix.md Outdated
Comment thread src/apm_cli/adapters/client/opencode.py Outdated
Comment thread src/apm_cli/integration/mcp_integrator_install.py Outdated
Comment thread src/apm_cli/integration/opencode_paths.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/apm_cli/integration/mcp_integrator.py
Comment thread tests/unit/test_opencode_mcp.py Outdated
Comment thread docs/src/content/docs/reference/cli/install.md
Co-authored-by: OpenCode <noreply@opencode.ai>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Legacy scope handling can modify global Claude configuration unexpectedly, and the documentation and tests contain inconsistencies.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Isolate Path.home in target discovery test

tests/​unit/​integration/​test_targets.py:112

active_targets_user_scope() also scans every target directory under the real Path.home(). On a developer or runner that already has another supported client configured, this exact-list assertion returns additional targets and becomes machine-dependent. Patch Path.home to the test's temporary root before discovery.

This issue also appears on line 114 of the same file.

Comment thread src/apm_cli/integration/mcp_integrator.py Outdated
Comment thread docs/src/content/docs/consumer/install-mcp-servers.md
Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Also fixed the Copilot test-isolation finding in 2407ac1: OpenCode user-scope discovery tests now patch Path.home() to their temporary root.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The adapter schema registry test will fail for the new mcp key, and the new canonical path owner lacks required architecture guardrails.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread src/apm_cli/adapters/client/opencode.py
Comment thread tests/unit/test_global_mcp_scope.py Outdated
Co-authored-by: OpenCode <noreply@opencode.ai>
Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc
Christopher Lunt (tnulc) marked this pull request as draft September 25, 2026 12:16
Co-authored-by: OpenCode <noreply@opencode.ai>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Configured-root symlinks remain traversable, Windows external roots can evade cleanup, and the new path authority lacks required architecture guardrails.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity · 2 Low severity

Open (4)
Resolved since last review (2)

Comment thread src/apm_cli/integration/mcp_integrator.py Outdated
Comment thread src/apm_cli/integration/targets.py Outdated
Comment thread docs/src/content/docs/reference/environment-variables.md Outdated
Comment thread tests/unit/test_mcp_integrator_remove_stale.py Outdated
Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc
Christopher Lunt (tnulc) requested a balanced review from Copilot September 25, 2026 13:23
Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Addressed the remaining global OpenCode lifecycle issues in 75aa5bed9:

  • preserve user scope for files inside globally deployed OpenCode skill bundles;
  • clean only the removed target-relative locator when targets share a compatibility path;
  • keep the surviving manifest/hash entry for that shared path.

Validated with:
.venv/bin/python -m pytest tests/unit/test_global_mcp_scope.py tests/unit/test_opencode_mcp.py tests/unit/integration/test_targets.py tests/unit/install/test_services.py tests/unit/install/phases/test_lockfile_union.py -q

Result: 217 passed.


Generated by receiving-code-review. This comment is AI-generated and may contain errors.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Symlink validation and target-relative cleanup still have security and ownership correctness gaps.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Locator grouping is duplicated instead of centralized

src/​apm_cli/​install/​manifest_reconcile.py:60

This duplicates the locator-grouping decision in install/phases/cleanup.py:38-44. Move the grouping behavior to the canonical deployment-ledger owner and call it from both reconciliation paths so fixes to composite locator handling cannot diverge.

Medium severity Duplicate locator grouping implementations can diverge

src/​apm_cli/​install/​phases/​cleanup.py:44

This introduces a second implementation of locator grouping; install/manifest_reconcile.py:54-60 contains the same decision. Because cleanup correctness depends on preserving every composite locator sharing a compatibility value, centralize this operation on the deployment-ledger owner and have both callers use it rather than allowing the two copies to drift.

Comment thread src/apm_cli/integration/cleanup.py
Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Addressed the follow-up symlink-validation and target-relative ownership findings in 3d37ab325:

  • preserve lexical OpenCode roots for symlink checks while retaining resolved paths where containment needs them;
  • reject symlinked user roots before global deployment or MCP cleanup;
  • resolve target-relative cleanup through persisted target profiles, including custom external OpenCode roots;
  • preserve locator identity for deleted and retained cleanup outcomes so same-valued paths cannot affect another target's ledger record;
  • retain legacy compatibility projections and Claude custom-config cleanup behavior.

Validated with:
.venv/bin/python -m pytest tests/unit/cache/test_cleanup.py tests/unit/install/test_manifest_reconcile_target_relative.py tests/unit/test_target_env_roots.py tests/unit/test_opencode_mcp.py tests/unit/test_mcp_integrator_remove_stale.py tests/unit/integration/test_cleanup_helper.py tests/unit/integration/test_targets.py tests/unit/install/phases/test_lockfile_union.py tests/integration/test_mcp_integrator_characterisation.py -q

Result: 286 passed.


Generated by receiving-code-review. This comment is AI-generated and may contain errors.

Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Follow-up fix pushed in 7feaaa800 for the mixed cleanup outcome case:

  • deleted and user-edited locators with the same compatibility value now retain outcomes by DeploymentLocator.key;
  • value-based legacy retention is used only for records with no identity outcome;
  • the deleted target is no longer restored when another target's file is user-edited.

Validated with:
.venv/bin/python -m pytest tests/unit/install/test_manifest_reconcile_target_relative.py tests/unit/install/phases/test_lockfile_union.py tests/unit/integration/test_cleanup_helper.py -q

Result: 69 passed.


Generated by receiving-code-review. This comment is AI-generated and may contain errors.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

External-root validation and cleanup contain correctness and symlink-safety gaps, and several tests remain non-portable.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity · 1 Low severity

Open (4)
Resolved since last review (1)

Comment thread src/apm_cli/integration/targets.py
Comment thread src/apm_cli/install/manifest_reconcile.py
Comment thread src/apm_cli/integration/cleanup.py
Comment thread tests/unit/test_target_env_roots.py Outdated
Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Addressed the four follow-up review comments in e34608955:

  • allow supported absolute external OpenCode roots while retaining lexical symlink rejection;
  • finalize deferred skill-directory cleanup outcomes instead of retaining deleted locators;
  • accept target-relative skills/<name> directory locators;
  • make target-root symlink tests skip cleanly when directory symlinks are unavailable.

Validated with:
.venv/bin/python -m pytest tests/unit/test_target_env_roots.py tests/unit/integration/test_cleanup_skill_dirs.py tests/unit/integration/test_cleanup_helper.py tests/unit/install/test_manifest_reconcile_target_relative.py tests/unit/integration/test_targets.py tests/integration/test_target_resolution_e2e.py -q

Result: 180 passed, 2 xfailed.


Generated by receiving-code-review. This comment is AI-generated and may contain errors.

@tnulc

Copy link
Copy Markdown
Author

The current head e34608955 contains the requested runtime fixes:

  • absolute external user roots are accepted after lexical symlink validation;
  • deferred skill-directory entries are finalized only as deleted/retained outcomes;
  • target-relative skills/<name> directories are recognized and cleaned safely.

These comments remain open in GitHub despite the fixes being present in the current diff. Focused validation passed: 180 passed, 2 xfailed.

Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Added the canonical-owner architecture guard in efc54f1a6:

  • registered opencode_paths.py as the owner of OpenCode config-root precedence and lexical/resolved path identity;
  • added a static guard rejecting direct OPENCODE_CONFIG_DIR/XDG_CONFIG_HOME reads and duplicated default-path resolution outside the owner;
  • registered the guard and added owner-rule mutation plus rule-inventory coverage.

Ruff and Ruff format pre-commit hooks passed, and git diff --check passed. The architecture boundary script reports only the two pre-existing baseline findings in deployment_ledger.py and manifest_reconcile.py; focused pytest could not run in the clean checkout because its pyenv environment has no pytest executable.


Generated by receiving-code-review. This comment is AI-generated and may contain errors.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Multi-target locator identity, cleanup ownership filtering, and symlink-boundary handling remain unsafe.

Review effort: Balanced
Findings: 2 High severity · 1 Low severity

Open (3)
Resolved since last review (4)
Previously missed (1)

In code that hasn't changed since last review

Low severity Remove unreachable duplicate Claude project condition

src/​apm_cli/​install/​deployed_paths.py:72

This second Claude/project condition is unreachable: the identical condition immediately above always either returns or continues. Remove the dead branch so the absolute-root projection has one clear implementation.

Comment thread src/apm_cli/install/phases/cleanup.py Outdated
Comment thread src/apm_cli/integration/targets.py
Comment thread tests/unit/test_target_env_roots.py Outdated
@tnulc

Copy link
Copy Markdown
Author

Implemented the remaining canonical-owner architecture follow-up in efc54f1:\n\n- registered src/apm_cli/integration/opencode_paths.py in the architecture owner registry;\n- added a static guard preventing consumers from duplicating OpenCode environment/default path resolution;\n- registered the guard and added mutation plus rule-inventory coverage.\n\nRuff hooks and git diff --check passed. The architecture linter reports only the two pre-existing baseline findings in deployment_ledger.py and manifest_reconcile.py. Focused pytest was unavailable in the clean checkout because its pyenv environment has no pytest executable.\n\n---\n\nGenerated by receiving-code-review. This comment is AI-generated and may contain errors.

Co-authored-by: OpenCode <noreply@opencode.ai>
@tnulc

Copy link
Copy Markdown
Author

Addressed the three latest open review comments in 5621eb483:

  • filter cleanup locator mappings by the package being cleaned, preventing cross-package deletion when compatibility paths collide;
  • apply bounded lexical symlink-component validation to absolute external roots, matching MCP and primitive deployment safety;
  • skip symlink setup tests on both OSError and NotImplementedError.

Validation: 118 focused tests passed; Ruff, Ruff format, and git diff --check passed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

External-root hashing, locator identity, and nested symlink checks remain unsafe or incomplete.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Low severity Skip symlink test when filesystem lacks symlink support

tests/​unit/​test_opencode_mcp.py:76

This unguarded symlink creation fails on Windows or restricted filesystems before the behavior can be tested. The repository convention is to catch OSError/NotImplementedError and skip when symlinks are unavailable (for example, tests/unit/marketplace/test_builder.py:2061-2064).

Comment on lines +74 to +82
locator = DeploymentLedgerCodec.locator_for_path(
target_path,
project_root=project_root,
target=_t,
scope=scope,
)
except RuntimeError:
continue
return locator.value
Comment on lines +72 to +85
boundary = Path(config_root or config_path.parent)
symlink_candidates.add(boundary)
try:
relative = config_path.parent.relative_to(boundary)
except ValueError:
# A lexical root may itself be a symlink to the resolved root.
# Walk the supplied lexical path instead of comparing resolved
# ancestry; components above the lexical root remain ignored.
relative = Path()
boundary = config_path.parent
current = boundary
for part in relative.parts:
current /= part
symlink_candidates.add(current)
@tnulc

Copy link
Copy Markdown
Author

The work from this PR has been split into two replacement PRs:

Closing this umbrella PR so the two focused PRs can proceed independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants