fix(mcp): honor optional registry inputs#1734
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes MCP registry optional env/input handling so that optional fields are no longer treated like required prompts or forcibly materialized into runtime configs; optional unset fields are omitted and existing user-edited optional values are preserved on reinstall.
Changes:
- Filter/normalize registry
required/is_requiredmetadata and omit optional env vars when no value is available. - Update Copilot + VS Code MCP config generation to avoid emitting placeholders/prompts for unset optional env vars, while preserving existing user edits on reinstall.
- Add unit regression coverage and update docs/changelog to reflect the behavior.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/registry/operations.py |
Normalizes required metadata and filters optional-unset env vars out of the shared env collection/prompting flow. |
src/apm_cli/adapters/client/base.py |
Adds a shared “is required?” helper and updates legacy prompt resolution to avoid writing empty optional values. |
src/apm_cli/adapters/client/copilot.py |
Skips optional env placeholders unless a value was observed via install-time collection. |
src/apm_cli/adapters/client/vscode.py |
Omits optional-unset env entries/inputs and preserves existing optional env edits from the current .vscode/mcp.json on reinstall. |
tests/unit/install/test_mcp_optional_inputs.py |
Adds regression tests for optional env behavior (no prompt/write when unset; preserve existing edit on reinstall). |
packages/apm-guide/.apm/skills/apm-usage/dependencies.md |
Documents the optional-unset omission + reinstall preservation behavior for registry-declared fields. |
docs/src/content/docs/reference/manifest-schema.md |
Updates manifest schema guidance for registry-backed server prompt generation (required-only). |
docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md |
Documents how optional registry env/input variables are handled and preserved. |
docs/src/content/docs/consumer/install-mcp-servers.md |
Documents install-time behavior for required vs optional registry env vars. |
CHANGELOG.md |
Adds an Unreleased “Fixed” entry describing the behavioral change. |
Copilot's findings
- Files reviewed: 10/10 changed files
- Comments generated: 1
| ### Fixed | ||
|
|
||
| - `apm install` now treats optional MCP registry env/input fields as optional: unset values are no longer prompted or written into generated runtime config, and reinstalls preserve user-edited optional values. (Refs #20) |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean shared predicate + read-before-write; one duplication nit in operations.py. |
| CLI Logging Expert | 0 | 0 | 1 | Quiet happy path is correct; a verbose note on skipped optionals would aid agents. |
| DevX UX Expert | 0 | 1 | 0 | The no-force-prompt win is exactly right; offer an opt-in to set optionals at install. |
| Supply Chain Security | 0 | 0 | 0 | Write path is safer: no clobber, malformed-config guards, refs not secrets. |
| OSS Growth Hacker | 0 | 0 | 0 | Strong "respects your config" story; CHANGELOG line is share-ready. |
| Test Coverage Expert | 0 | 1 | 0 | Solid unit regression traps for both #20 symptoms; add an integration-tier trap. |
| Doc Writer | 0 | 0 | 0 | Docs synced across consumer/producer/reference + apm-usage skill + CHANGELOG (Rule 4). |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Test Coverage Expert] Add an integration-tier regression trap that drives optional-omission and reinstall-preservation through a real
apm install --target vscode(real fixture config + real read/write/diff). -- The install pipeline's floor tier is integration; the current traps are unit-tier and the end-to-end path is only covered by the manual How-to-test checklist, so cross-adapter drift could land unnoticed. - [DevX UX Expert] Offer an opt-in way to supply an optional value at install (env override / flag) instead of requiring a hand-edit of the generated config. -- Preserves the "ship in 5 minutes" ergonomics for the user who actually wants the optional token; today the only paths are pre-set env or manual edit.
- [Python Architect] Reuse the shared
_registry_field_is_requiredhelper insideregistry/operations.pyrather than re-derivingvar_info.get("required", True) is not Falsein three loops. -- Single source of truth for the optional-metadata semantics; prevents the resolver and the operations paths from drifting apart later. - [CLI Logging Expert] Emit a
--verbosedim line noting optional env vars skipped for lack of a value. -- Gives an agent debugging "why is my optional var absent from config" a signal without polluting the human happy path.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<AbstractAdapter>>
+configure_mcp_server()
+_resolve_env_vars_with_prompting(env_vars, overrides)
+_resolve_environment_variables(env_vars, overrides)
}
class CopilotClientAdapter {
+_resolve_environment_variables()
+_format_runtime_env_placeholder(name)
}
class VSCodeClientAdapter {
+_format_server_config(server_info, env_overrides, existing_server_config)
+_value_for_declared_vscode_env(env_var, input_ref)
+_existing_mapping_value(cfg, section, name)
+_has_value(value)
}
class MCPServerOperations {
+collect_environment_variables(servers, registry)
+_prompt_for_environment_variables(required_vars)
}
class EnvVarRequiredGuard {
<<PureFunction>>
+_registry_field_is_required(field) bool
}
MCPClientAdapter <|-- CopilotClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
MCPClientAdapter ..> EnvVarRequiredGuard : gates prompt
CopilotClientAdapter ..> EnvVarRequiredGuard : gates placeholder
VSCodeClientAdapter ..> EnvVarRequiredGuard : gates emit/preserve
MCPServerOperations ..> EnvVarRequiredGuard : honors required/is_required
note for EnvVarRequiredGuard "Guard Clause: required unless\nrequired/is_required is explicitly False"
note for VSCodeClientAdapter "Read-before-write: preserve\nuser-edited optional env on reinstall"
class EnvVarRequiredGuard:::touched
class CopilotClientAdapter:::touched
class VSCodeClientAdapter:::touched
class MCPServerOperations:::touched
class MCPClientAdapter:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install --target vscode"] --> B["MCPServerOperations.collect_environment_variables()"]
B --> C{"_registry_field_is_required(env_var)?"}
C -->|required| D["[I/O] _prompt_for_environment_variables() prompt or os.getenv"]
C -->|optional + no value| E["skip: not added to all_required_vars"]
D --> F["env_overrides -> adapter.configure_mcp_server()"]
E --> F
F --> G{"adapter type"}
G -->|VS Code| H["VSCodeClientAdapter._format_server_config()"]
G -->|Copilot| I["CopilotClientAdapter._resolve_environment_variables()"]
H --> J["[FS] get_current_config() read existing .vscode/mcp.json"]
J --> K{"_value_for_declared_vscode_env()"}
K -->|required| L["emit ${input:...} + promptString"]
K -->|optional + existing user edit| M["preserve existing value"]
K -->|optional + override value| N["emit ${env:NAME}"]
K -->|optional + no value| O["omit env entry"]
I --> P{"required or has_override?"}
P -->|yes| Q["emit runtime placeholder"]
P -->|optional + no value| R["omit"]
L --> S["[FS] write runtime config"]
M --> S
N --> S
O --> S
Q --> S
R --> S
Recommendation
Ship it. There are no blocking-severity findings; the fix is surgical, the #20 symptoms are each defended by a passing regression trap, and docs are synced in the same PR. Track follow-up #1 (an integration-tier trap through real apm install) as the highest-signal hardening item so the cross-adapter behavior cannot silently drift; the other three are low-cost polish. #20 correctly stays open as the anchor for the deferred export / custom-registry / token-allow-list / private-repo work.
Full per-persona findings
Python Architect
-
[nit]
registry/operations.pyre-derives optional semantics withvar_info.get("required", True) is not Falsein three separate loops instead of reusing the new_registry_field_is_requiredhelper frombase.py.
The PR introduces a clean shared predicate and threads it throughbase.py,copilot.py, andvscode.py;operations.pyis the one path that re-implements the same boolean by hand. Consolidating keeps the optional-metadata rule in one place.
Suggested: import and call the shared helper inoperations.pyso all four consumers share one definition of "required".Design patterns
- Used in this PR: Guard Clause / shared predicate --
_registry_field_is_requiredcentralizes the optional-vs-required decision; Template Method --MCPClientAdapterbase with Copilot/VS Code overrides of_resolve_environment_variables; Read-before-write --get_current_config()is read before_format_server_config()so user-edited optional values survive reinstall. - Pragmatic suggestion: none beyond the nit above -- the current shape is the simplest correct design at this scope; no new abstraction would pay off here.
- Used in this PR: Guard Clause / shared predicate --
CLI Logging Expert
- [nit] Optional env vars are silently dropped from generated config; correct for the human happy path, but a
--verbosedim line ("skipped optional env VAR -- no value provided") would help an agent understand why an expected variable is absent.
Signal-to-noise is right for default output; this is purely an opt-in verbose affordance and is skippable.
DevX UX Expert
- [recommended] A user who does want to set an optional value at install time now has no prompt path -- optional fields are skipped, so the only routes are a pre-set environment variable or a manual edit of the generated config.
The board-scoped behavior (do not force-prompt optionals as required) is exactly right and fixes the reported context7 friction. The follow-up is to offer an explicit opt-in (env override already works; a flag or a clearly-skippable prompt would close the loop) so the "want the token" case stays one copy-paste away rather than a hand-edit. Non-blocking.
Supply Chain Security Expert
No findings. The change strengthens the write path: it reads existing config before formatting and preserves user-edited optional values instead of overwriting them, adds isinstance guards against malformed servers/inputs sections, and never materializes secret values -- VS Code gets ${input:...} / ${env:NAME} references and Copilot gets runtime placeholders, so APM still neither reads nor stores the optional secret. No new download, path, or token surface.
OSS Growth Hacker
No findings. The "APM respects optional inputs and never clobbers your hand edits on reinstall" framing is a clean, share-ready trust signal, and the CHANGELOG Fixed entry already states it in one reposting-shaped line. No conversion-surface regression.
Test Coverage Expert
- [recommended] The optional-omission and reinstall-preservation behaviors are defended by unit tests that exercise the adapters directly; the install-pipeline floor tier is
integration-with-fixtures, and there is no integration-tier test that drives the behavior through a realapm install.
Proof (passed):tests/unit/install/test_mcp_optional_inputs.py::test_collect_environment_variables_does_not_prompt_optional_without_value-- proves: optional registry env vars are not force-prompted as required [pragmatic_as_npm, secure_by_default]
Proof (passed):tests/unit/install/test_mcp_optional_inputs.py::test_vscode_reinstall_preserves_user_edited_optional_env-- proves: reinstall preserves a user-edited optional VS Code env value [pragmatic_as_npm]
Proof (missing) at:tests/integration/test_mcp_integrator_install.py-- proves: end-to-endapm installomits unset optional env and preserves user edits across adapters
The unit traps are strong and pass on this commit; the gap is only the end-to-end tier (currently covered by the manual How-to-test checklist). Severity recommended: this is a no-clobber/write-path surface, not an auth/governance surface.
Doc Writer
No findings. Documentation is synced in the same PR per Rule 4: docs/src/content/docs/consumer/install-mcp-servers.md, producer/author-primitives/mcp-as-primitive.md, and reference/manifest-schema.md all describe the required/optional distinction; packages/apm-guide/.apm/skills/apm-usage/dependencies.md carries the matching skill note; and CHANGELOG.md has a Fixed entry. Framing is consistent across the surfaces.
Auth Expert -- inactive
The diff touches MCP client adapters (base.py, copilot.py, vscode.py) and registry/operations.py only; it does not touch auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, install/validation.py, install/pipeline.py, or registry_proxy.py, and changes no host/token/credential/fallback selection. The optional fields are MCP-server env vars (rendered as references, never stored); the consumer-side token allow-list is explicitly deferred under #20.
Performance Expert -- inactive
The diff touches none of deps/**, cache/**, install/phases/**, install/pipeline.py, install/resolve.py, or transport, and makes no performance claim. The one added cost is a single get_current_config() read on the VS Code reinstall path, which is negligible and off the routed perf surfaces.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The #20 fix intentionally changed two behaviors the board mandated: declared-but-optional MCP vars with no value are now OMITTED from the resolved env (not force-defaulted to empty), and configure_mcp_server now threads env_overrides + existing_server_config into _format_server_config for reinstall-preservation. Six pre-existing tests in TestPromptForEnvironmentVariables and one in test_vscode_docker_runtime_args asserted the OLD behavior/call signature and broke. Reconcile them to the new contract: - e2e optional-var test now asserts optional-no-value is omitted while an optional var carrying a declared default is still honored. - CI-mode-detection probes switch to required vars (an optional var is now skipped in BOTH CI and interactive mode, so it can no longer distinguish the two; a required var is auto-defaulted only in CI). - docker-runtime test asserts runtime_vars is still threaded without over-constraining the now-wider call signature. Refs #20 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
danielmeppiel/20-mcp-optional-inputs
branch
from
c699fa9 to
3be8e0e
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Architecture is sound; the only concerns are naming nits on a cross-module helper and a variable that now also carries optional metadata. |
| Cli Logging Expert | 0 | 0 | 1 | No user-facing CLI output changed; a verbose trace for skipped optional fields would help future debugging. |
| Devx Ux Expert | 0 | 0 | 2 | The user promise is strong: optional tokens stop interrupting install, and required vars keep their recovery path. |
| Supply Chain Security Expert | 0 | 0 | 1 | The change is fail-closed on metadata and avoids token leakage or empty credential materialization. |
| Oss Growth Hacker | 0 | 0 | 2 | This is a good adoption fix for zero-prompt installs; the docs could make that benefit more shareable. |
| Auth Expert | 0 | 0 | 1 | Optional credential handling is safe and does not bypass AuthResolver; runtime-env expectations are the only minor note. |
| Doc Writer | 0 | 1 | 1 | Docs are accurate and scoped; the repeated optional-input rule should eventually point to one canonical reference. |
| Test Coverage Expert | 0 | 1 | 0 | Five targeted unit/adapter tests cover no-prompt, no-emit, and reinstall preservation; one integration fixture would further defend the full install path. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add fixture-based integration test covering install-to-VS-Code config for omitted optional env vars. -- Existing unit tests prove each layer, but the orchestration seam between registry collection and adapter writing is unguarded at integration level.
- [Doc Writer] Consolidate the optional-variable rule into one canonical reference in manifest-schema; shorten producer/consumer copies to cross-links. -- The rule is restated in four places today. When deferred [FEATURE] Extended MCP Server Configuration in apm.yml #20 work lands, drift risk multiplies.
- [Python Architect] Rename _registry_field_is_required to a public name or move to a shared helper. -- Three modules import this private-named function as their canonical required-vs-optional predicate.
- [Devx Ux Expert] Rename all_required_vars to collected_env_vars or env_vars_to_resolve. -- The collection now holds optional metadata when values/defaults exist, so the current name misleads future edits.
- [Oss Growth Hacker] Lead the CHANGELOG entry with user benefit: optional-auth MCP servers now install without prompting for tokens. -- Shareable release wording reinforces APM zero-friction positioning.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<ABC>>
+_resolve_env_vars_with_prompting()
}
class CopilotClientAdapter {
+_resolve_environment_variables()
}
class VSCodeClientAdapter {
+configure_mcp_server()
+_format_server_config()
+_value_for_declared_vscode_env()
}
class MCPServerOperations {
+collect_environment_variables()
+_prompt_for_environment_variables()
}
class RegistryFieldRequired {
<<Pure Function>>
+required_or_is_required(field) bool
}
MCPClientAdapter <|-- CopilotClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
CopilotClientAdapter ..> RegistryFieldRequired : required predicate
VSCodeClientAdapter ..> RegistryFieldRequired : required predicate
MCPClientAdapter ..> RegistryFieldRequired : required predicate
MCPServerOperations ..> RegistryFieldRequired : inline equivalent
class MCPClientAdapter:::touched
class CopilotClientAdapter:::touched
class VSCodeClientAdapter:::touched
class MCPServerOperations:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[Registry MCP metadata] --> B[MCPServerOperations collects env metadata]
B --> C{Required or value/default exists?}
C -->|Required| D[Prompt or runtime placeholder path]
C -->|Optional with value/default| E[Carry value forward]
C -->|Optional unset| F[Omit from collected env]
D --> G{Target adapter}
E --> G
G --> H[Copilot config: emit required/provided placeholders]
G --> I[VS Code config: preserve existing optional value]
F --> J[No prompt and no generated runtime entry]
Recommendation
This PR is ready for the maintainer to merge at will. CI is green, the scope fence is tight, and the core user promise is covered by targeted tests. Track the integration fixture as the main follow-up rather than holding this scoped bug fix.
Full per-persona findings
Python Architect
- [nit] _registry_field_is_required is private-named but imported by sibling adapters at
src/apm_cli/adapters/client/base.py:32
Leading underscore suggests module-private, but copilot.py and vscode.py import it as the canonical required-vs-optional predicate. Renaming would make the adapter-internal contract clearer.
Suggested: Rename to registry_field_is_required or move to a small shared helper module. - [nit] all_required_vars now also includes optional vars with defaults or existing values at
src/apm_cli/registry/operations.py:360
The collection can now hold optional metadata when a value/default exists, so the name is semantically stale and can mislead future edits.
Suggested: Rename to collected_env_vars or env_var_metadata.
Cli Logging Expert
- [nit] Silent optional-field skips have no verbose/debug breadcrumb at
src/apm_cli/registry/operations.py:427
Default silence is appropriate, but agent/debug users in verbose mode benefit from seeing why a registry env var was omitted rather than prompted or written.
Suggested: If a logger is available in this path later, add a verbose-only skip line for optional unset fields.
Devx Ux Expert
- [nit] Unresolved required env warning could offer a concrete next action at
src/apm_cli/adapters/client/base.py:863
When a required env var remains empty, the user sees what is missing but not the fastest recovery step. This is pre-existing wording surfaced by the changed path, not a correctness issue.
Suggested: Append a short next action such as "Set NAME in your environment and rerun apm install." - [nit] all_required_vars naming no longer matches contents at
src/apm_cli/registry/operations.py:360
The collection can now include optional vars with defaults or existing env values, which makes the identifier surprising to future maintainers.
Suggested: Rename to env_vars_to_resolve or collected_env_vars.
Supply Chain Security Expert
- [nit] Required predicate helper is private-named while serving a security-adjacent shared contract at
src/apm_cli/adapters/client/base.py:32
The helper controls whether credential fields prompt or omit. Making its cross-module contract explicit improves greppability for future security reviews.
Suggested: Rename to registry_field_is_required or place it in an explicit shared helper.
Oss Growth Hacker
- [nit] CHANGELOG entry can lead with the user benefit at
CHANGELOG.md:24
A release note like "Context7-style optional tokens no longer interrupt install" is more shareable than implementation wording.
Suggested: Consider leading with the zero-prompt benefit in release notes. - [nit] Consumer docs could add a one-line why-you-care hook at
docs/src/content/docs/consumer/install-mcp-servers.md:78
The docs state the rule accurately, but a quick example helps first-time users connect it to optional-auth servers.
Suggested: Add a short example such as "Servers with optional auth install without a token prompt until you choose to configure one."
Auth Expert
- [nit] VS Code optional override path relies on the VS Code process environment at
src/apm_cli/adapters/client/vscode.py:486
When an optional value was collected and emitted as ${env:NAME}, VS Code must have NAME in its process environment. That is safe, but a comment/docstring would reduce support ambiguity.
Suggested: Document that ${env:...} resolution depends on the VS Code process environment.
Doc Writer
- [recommended] Optional-variable behavior is restated in four places without a canonical source at
docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md:125
The behavior is accurate, but consumer docs, producer docs, manifest reference, and apm-guide now each carry the same rule. Keeping one authoritative reference with short cross-links elsewhere reduces drift when deferred [FEATURE] Extended MCP Server Configuration in apm.yml #20 work lands.
Suggested: Keep the full rule in manifest-schema and shorten the producer/consumer copies to a summary plus relative link. - [nit] Producer page wording is looser than the reference wording at
docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md:125
"Does not force a prompt" is understandable but less precise than "does not generate a prompt or runtime config entry". Matching terms keeps the docs corpus consistent.
Suggested: Use "does not generate a prompt or runtime config entry" for consistency.
Test Coverage Expert
- [recommended] No integration test covers the full install-to-VS Code config round-trip for omitted optional env vars at
tests/integration/test_vscode_adapter_phase3w4.py
The new unit tests prove the adapter and collection behavior directly, including reinstall preservation. Because the changed contract spans registry collection into adapter config writing, a fixture-backed install test would catch orchestration drift between those layers.
Suggested: Add a fixture-based integration test that runs the install flow with optional env metadata unset and asserts .vscode/mcp.json omits the optional env/input entries.
Proof (missing at):tests/integration/test_vscode_adapter_phase3w4.py::test_optional_env_vars_omitted_from_vscode_config_on_install-- proves: Full install flow omits unset optional MCP env vars from generated VS Code config. [devx,secure-by-default]
assert 'CONTEXT7_API_KEY' not in server.get('env', {})
Performance Expert -- inactive
PR #1734 touches MCP client adapters, registry env collection, docs, and unit tests, with no dependency download, cache, materialization, transport, parallelism, or performance claim.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Optional-input handling is architecturally contained; only two low-risk naming/consistency nits remain. |
| CLI Logging Expert | 0 | 0 | 2 | Warning/debug paths remain acceptable; two terminal wording nits are low-risk polish only. |
| DevX UX Expert | 0 | 0 | 1 | Optional-input omission matches package-manager expectations; reinstall preservation is the right UX; no substantive concerns. |
| Supply Chain Security Expert | 0 | 0 | 0 | No supply-chain surface expands; unknown metadata defaults required and optional unset values are omitted rather than written. |
| OSS Growth Hacker | 0 | 0 | 2 | Good friction-reduction fix; release-note story is clear, with only optional messaging polish left. |
| Auth Expert | 0 | 0 | 0 | No auth bypass or credential leakage; optional token/env behavior is auth-adjacent but safe. |
| Doc Writer | 0 | 1 | 1 | Docs consolidate the optional-input rule into the manifest reference with clean cross-refs; one wording nuance remains optional polish. |
| Test Coverage Expert | 0 | 0 | 0 | The no-prompt, omit-from-config, and preserve-on-reinstall promises have unit and integration regression traps; no coverage gaps. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 2 follow-ups
- [Doc Writer] Generalize consumer-page optional-input wording beyond auth-specific examples to cover all optional env/input vars. -- The canonical rule is general but the consumer summary narrows the framing to auth tokens, which could under-signal the breadth of the behavior.
- [Python Architect] Centralize the required/is_required field predicate so registry operations and adapter layers share one source of truth. -- Prevents drift if registry metadata gains another compatibility spelling; low urgency since only two call sites exist today.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<ABC>>
+_resolve_environment_variables(env_vars, env_overrides)
+_resolve_env_vars_with_prompting(env_vars, ...)
}
class VSCodeClientAdapter {
+configure_mcp_server()
+_format_server_config(server_info, runtime_vars, env_overrides, existing_server_config)
+_value_for_declared_vscode_env(env_var, ...)
+_existing_mapping_value(existing, section, name)
}
class CopilotClientAdapter {
+_resolve_environment_variables(env_vars, env_overrides)
}
class MCPServerOperations {
+collect_environment_variables(server_refs, server_info_cache)
+_prompt_for_environment_variables(required_vars)
}
class registry_field_is_required {
<<Pure>>
+__call__(field) bool
}
MCPClientAdapter <|-- VSCodeClientAdapter
MCPClientAdapter <|-- CopilotClientAdapter
VSCodeClientAdapter ..> registry_field_is_required : checks required metadata
CopilotClientAdapter ..> registry_field_is_required : checks required metadata
MCPClientAdapter ..> registry_field_is_required : checks required metadata
MCPServerOperations ..> registry_field_is_required : mirrors predicate
class VSCodeClientAdapter:::touched
class CopilotClientAdapter:::touched
class MCPClientAdapter:::touched
class MCPServerOperations:::touched
class registry_field_is_required:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[Registry MCP metadata] --> B[MCPServerOperations.collect_environment_variables]
B --> C{required or value/default present?}
C -->|yes| D[Prompt or reuse existing/default value]
C -->|optional unset| E[Omit from shared env state]
D --> F[Adapter runtime config formatting]
E --> F
F --> G{VS Code reinstall has user-edited optional value?}
G -->|yes| H[Preserve existing env value]
G -->|no| I[Omit optional unset env/input entry]
F --> J[Copilot emits required/provided placeholders only]
H --> K[Write runtime config]
I --> K
J --> K
sequenceDiagram
participant User
participant Install as apm install
participant Registry as Registry metadata
participant Adapter as Runtime adapter
User->>Install: install registry MCP server
Install->>Registry: read env/input metadata
Registry-->>Install: required=false optional token
Install->>Install: no env/default value found
Install->>Adapter: omit optional unset value
Adapter-->>User: config written without prompt/input entry
Recommendation
Merge as-is. The doc wording nuance and naming nits are genuine polish but carry no user-facing correctness risk and can land in a follow-up without delaying this bug fix. Track the doc-writer generalization as the highest-signal post-merge item.
Full per-persona findings
Python Architect
- [nit] Required-field predicate is duplicated inline in registry operations at
src/apm_cli/registry/operations.py:369
registry/operations.py rebuilds the same required/is_required interpretation used by the shared adapter helper. The current code is correct, but a second local predicate is easier to drift if registry metadata grows another compatibility spelling.
Suggested: Consider centralizing required/is_required interpretation when registry operations and adapter layers can share a neutral helper without introducing layering inversion. - [nit] Parameter name required_vars is stale after optional-with-default vars can flow through at
src/apm_cli/registry/operations.py:390
_prompt_for_environment_variables now receives the collected promptable variables, not strictly required variables. The old name can mislead future maintainers into treating optional defaulted entries as required.
Suggested: Rename required_vars to collected_vars or promptable_vars in a future cleanup.
CLI Logging Expert
- [nit] Non-interactive warning renders a Python list repr instead of comma-joined names at
src/apm_cli/adapters/client/base.py:811
Warning output should be immediately actionable. Rendering ['FOO', 'BAR'] is noisier than FOO, BAR for humans and agent log parsers.
Suggested: Use ', '.join(var_names) in the warning f-string. - [nit] Optional-skip debug message is longer than necessary at
src/apm_cli/registry/operations.py:375
The debug line is gated behind verbose, so it is acceptable. A shorter structured message would be easier to scan when diagnosing generated config.
Suggested: Optional: shorten to 'Omitting optional env var %s for %s (no value)'.
DevX UX Expert
- [nit] Verbose mode could include a breadcrumb when optional env vars are skipped at
src/apm_cli/registry/operations.py
Happy-path silence is correct, but a verbose user diagnosing generated config might benefit from a single debug line explaining that an optional unset variable was omitted.
Suggested: If keeping the existing debug line, no action needed; otherwise standardize on 'Skipping optional env %s (no value)'.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
- [nit] CHANGELOG entry could be punchier for release-note mining at
CHANGELOG.md
The current line is accurate. A shorter benefit-first line would be easier to reuse in release notes and social updates.
Suggested: Optional: 'Optional MCP env vars are no longer force-prompted or written into config; user edits survive reinstall. (Refs [FEATURE] Extended MCP Server Configuration in apm.yml #20)' - [nit] Consumer docs could add a concrete before/after example at
docs/src/content/docs/consumer/install-mcp-servers.md
The prose is correct. A tiny example would make the behavior easier to share and skim, but is not necessary for this scoped bug fix.
Suggested: Consider a follow-up snippet showing optional env absent from .vscode/mcp.json.
Auth Expert
No findings.
Doc Writer
- [recommended] Consumer page examples lean auth-specific while the canonical rule covers all optional env/input variables at
docs/src/content/docs/consumer/install-mcp-servers.md:79
The reference text correctly covers every declared-optional env/input field. The consumer summary uses optional auth/token language, which is illustrative but may under-signal that non-auth optional variables follow the same omit-and-preserve rule.
Suggested: Generalize to 'Optional variables, including optional auth, are not prompted or written when unset; reinstall preserves values you set later.' - [nit] New cross-refs use a numbered-section anchor that could drift if the reference is renumbered at
docs/src/content/docs/reference/manifest-schema.md:535
The current anchor is correct. Because the slug includes the 4.2.4 heading number, future reference renumbering could break multiple inbound links at once.
Test Coverage Expert
No findings.
Performance Expert -- inactive
Touched files are registry config collection, client adapter runtime config formatting, docs, changelog, and tests; no cache/deps/install pipeline hot path, fetch transport, resolve tier, or materialization code is in scope.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
…ges (#1757) * chore: release v0.20.0 Bump pyproject.toml + uv.lock to 0.20.0 and roll the [Unreleased] CHANGELOG block into [0.20.0] - 2026-06-11. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.20.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(integration): refresh stale integration tests after v0.20.0 changes The release integration suite (which gates releases, not per-PR runs) surfaced 29 failures after v0.20.0 was tagged. Each was a stale test that had not been updated to match an intentional source behavior change merged this cycle. Source is correct throughout; only tests change here. Clusters and causing PRs: - A (#1735): download_github_file now calls _host._resolve_dep_auth_ctx; mock host helpers never stubbed it, forcing the wrong auth path. Added the stub to make_host/_make_host in 4 download test files. Also #1735 made generic raw-URL network errors raise RuntimeError instead of falling back to the Contents API -- rewrote 2 stale fallback tests to assert the raise. - B (#1742): apm compile -t copilot suppresses empty AGENTS.md shells when .github/instructions/*.md exists. Flipped 3 assertions to expect no AGENTS.md and assert the instruction files are present. - C (#1739): marketplace fetches moved to a shared requests.Session (_HTTP_SESSION.get); updated 6 tests to mock that seam. - D (#1734): optional registry env inputs are omitted without an override; updated 2 placeholder tests to assert the var is absent. - E (#1734): same omit-optional change; assert var not in result. - F (#1720): tar symlink rejection wording changed; updated the regex. Verified locally: 738 passed, 16 skipped (token-gated e2e). The two runnable cluster-B e2e tests (mixed_deps, guardrailing) pass against the v0.20.0 build; ado_e2e is identical logic, validated in CI. Full lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r changes (#1758) * chore: release v0.20.0 Bump pyproject.toml + uv.lock to 0.20.0 and roll the [Unreleased] CHANGELOG block into [0.20.0] - 2026-06-11. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.20.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(integration): refresh stale integration tests after v0.20.0 changes The release integration suite (which gates releases, not per-PR runs) surfaced 29 failures after v0.20.0 was tagged. Each was a stale test that had not been updated to match an intentional source behavior change merged this cycle. Source is correct throughout; only tests change here. Clusters and causing PRs: - A (#1735): download_github_file now calls _host._resolve_dep_auth_ctx; mock host helpers never stubbed it, forcing the wrong auth path. Added the stub to make_host/_make_host in 4 download test files. Also #1735 made generic raw-URL network errors raise RuntimeError instead of falling back to the Contents API -- rewrote 2 stale fallback tests to assert the raise. - B (#1742): apm compile -t copilot suppresses empty AGENTS.md shells when .github/instructions/*.md exists. Flipped 3 assertions to expect no AGENTS.md and assert the instruction files are present. - C (#1739): marketplace fetches moved to a shared requests.Session (_HTTP_SESSION.get); updated 6 tests to mock that seam. - D (#1734): optional registry env inputs are omitted without an override; updated 2 placeholder tests to assert the var is absent. - E (#1734): same omit-optional change; assert var not in result. - F (#1720): tar symlink rejection wording changed; updated the regex. Verified locally: 738 passed, 16 skipped (token-gated e2e). The two runnable cluster-B e2e tests (mixed_deps, guardrailing) pass against the v0.20.0 build; ado_e2e is identical logic, validated in CI. Full lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(release): refresh release-validation harness for v0.20.0 behavior changes The release-validation shell harness carries its own copies of behavior assertions that duplicate the integration suite. Two of them went stale this cycle from the same PRs that broke the integration tests (#1757): - GH-AW compat (#1720): `apm pack --archive` now emits .zip by default; the archive check grepped only `build/*.tar.gz`. Accept either extension, testing each glob independently (a single `ls a b` exits non-zero when either pattern is unmatched, even if the other matches). - Hero scenario 2 / AGENTS.md (#1742): copilot `apm compile` omits the empty AGENTS.md shell when installed instructions already live under `.github/instructions/`. The check insisted AGENTS.md exist; now accept AGENTS.md OR a populated `.github/instructions/`, mirroring the merged pytest fix in test_guardrailing_hero_e2e.py. Same fixes applied to the Windows .ps1 (AGENTS.md only; it has no archive check). Predicates validated locally against apm v0.20.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sync the Stage 2 complexity/file-length refactor branch with main's 22 feature commits (Hermes #1726, Kiro IDE #1741, multi-host dep identity #1735, same-repo remote path deps #1732, git_file_transport #1740, revision pins #1738, marketplace sourceBase/source parity/inherit description #1736/#1739/#1755, pack --archive .zip #1720, mcp optional registry inputs #1734, and the v0.19.0/v0.20.0 releases). Conflict resolution preserved both sides: main's new features ported through the branch's extracted sibling modules, branch's tightened ruff thresholds (max-statements=120, max-branches=40, max-complexity=35, max-returns=8, max-args=12) and 800-line file limit retained. All 7 CI-mirror lint gates pass; full unit suite green (17099 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fix(mcp): honor optional registry inputs
TL;DR
This PR fixes the scoped #20 bug where registry-declared optional MCP env/input fields were treated like required prompts and re-written into runtime config. Optional fields with no value are now omitted, while existing user-edited optional values are preserved on reinstall. Required fields keep the existing prompt/placeholder behavior.
Note
Part of #20 only. This intentionally does not add
apm mcp export, custom-registry URL wiring, consumer token allow-lists, or private MCP repo sourcing.Problem (WHY)
.vscode/mcp.jsonafter a user deleted the optional env config; the issue says "on subsequent installs, that configuration is overridden".Why this matters: APM should keep generated config predictable and reviewable. The repo docs describe
apm.ymlas the "source of truth" for runtime config, but optional registry inputs with no user value are not source data and should not be materialized.Approach (WHAT)
required/is_requiredmetadata once and use it before prompting or generating env entries.MCPServerOperations.collect_environment_variables()so shared install state does not force downstream adapters to write them.Implementation (HOW)
src/apm_cli/adapters/client/base.pysrc/apm_cli/adapters/client/copilot.pysrc/apm_cli/adapters/client/vscode.pysrc/apm_cli/registry/operations.pyrequiredandis_required.tests/unit/install/test_mcp_optional_inputs.pyDiagrams
Legend: the dashed nodes are the new behavior added by this PR: optional unset fields are omitted, and existing user edits survive reinstall.
flowchart LR subgraph Registry[Registry metadata] E1[environment variables] end subgraph Collect[Collect] C1[MCPServerOperations] D1{required?} end subgraph Runtime[Runtime config] R1[Prompt or placeholder] R2[Omit optional unset] R3[Preserve existing edit] end E1 --> C1 C1 --> D1 D1 -->|"yes"| R1 D1 -->|"no value"| R2 D1 -->|"existing value"| R3 classDef new stroke-dasharray: 5 5; class R2,R3 new;Trade-offs
required/is_required; rejected a newapm.ymloptional-input schema because the board scoped this to the bug only.inputsentries; removing stale runtime config safely is broader cleanup behavior.Benefits
enventry and no generatedinputsprompt.${input:...}.Validation
uv run --extra dev pytest tests/unit/install/test_mcp_optional_inputs.py tests/integration/test_vscode_adapter_phase3w4.py tests/unit/test_registry_operations_phase3.py tests/unit/test_registry_operations_state.py tests/unit/test_vscode_adapter.py -q:uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ && uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ && bash scripts/lint-auth-signals.sh:Scenario Evidence
tests/unit/install/test_mcp_optional_inputs.py::test_collect_environment_variables_does_not_prompt_optional_without_value(regression-trap for #20)tests/unit/install/test_mcp_optional_inputs.py::test_base_env_resolver_omits_optional_env_when_no_value_providedtests/unit/install/test_mcp_optional_inputs.py::test_copilot_omits_optional_env_placeholder_when_no_value_providedtests/unit/install/test_mcp_optional_inputs.py::test_vscode_omits_optional_env_inputs_when_no_value_providedtests/integration/test_vscode_adapter_phase3w4.py::TestConfigureMcpServer::test_optional_env_vars_omitted_from_vscode_config_on_installtests/unit/install/test_mcp_optional_inputs.py::test_vscode_reinstall_preserves_user_edited_optional_env(regression-trap for #20)tests/integration/test_vscode_adapter_phase3w4.py::TestConfigureMcpServer::test_optional_env_vars_preserved_on_reinstallHow to test
apm.ymlwith a registry MCP server whose package metadata declares an optional env var and no local value; runapm install --target vscode; expect no env/input prompt for that optional field..vscode/mcp.json; rerun install; expect the value to remain unchanged.uv run --extra dev pytest tests/unit/install/test_mcp_optional_inputs.py -q; expect all tests to pass.Refs #20
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com