diff --git a/CHANGELOG.md b/CHANGELOG.md index c591e6fe1..62b4038ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Required MCP runtime defaults are now overrideable prompts, while secret + defaults remain hidden and VS Code OCI launchers resolve every runtime + placeholder without writing secret values to `mcp.json`. (#2455) - `apm marketplace audit --strict` now fails when a local marketplace source is skipped, so CI cannot report success after an incomplete local audit. (#2460) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index ac1c385a5..f1368a873 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -549,6 +549,17 @@ "tests/spec_conformance/test_manifest_reqs.py::test_consumer_diagnoses_empty_skill_subset_match" ] }, + { + "conformance_class": "consumer", + "id": "req-mf-023", + "keyword": "MUST", + "section": "4.5", + "status": "active", + "test_count": 1, + "tests": [ + "tests/spec_conformance/test_manifest_reqs.py::test_consumer_resolves_runtime_argument_templates_without_secret_leakage" + ] + }, { "conformance_class": "governance", "id": "req-pl-001", @@ -1246,7 +1257,7 @@ "spec_version": "v0.1.1", "summary_by_class": { "consumer": { - "active": 79, + "active": 80, "skipped": 1, "unbound": 0, "xfail": 0 @@ -1270,5 +1281,5 @@ "xfail": 0 } }, - "total_requirements": 109 + "total_requirements": 110 } diff --git a/CONFORMANCE.md b/CONFORMANCE.md index df41a7e74..5d6e13bb2 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -19,7 +19,7 @@ All four conformance classes (Producer, Consumer, Registry, Governance) carry ac | Class | Active | Skipped | Xfail | Unbound | |-------|-------:|--------:|------:|--------:| | Producer | 12 | 0 | 0 | 0 | -| Consumer | 79 | 1 | 0 | 0 | +| Consumer | 80 | 1 | 0 | 0 | | Registry | 1 | 0 | 0 | 0 | | Governance | 16 | 0 | 0 | 0 | @@ -75,6 +75,7 @@ All four conformance classes (Producer, Consumer, Registry, Governance) carry ac | [req-mf-020](docs/src/content/docs/specs/openapm-v0.1.md#req-mf-020) | MUST | 4.1 | consumer | active | 1 | | [req-mf-021](docs/src/content/docs/specs/openapm-v0.1.md#req-mf-021) | MUST | 4.8 | producer | active | 1 | | [req-mf-022](docs/src/content/docs/specs/openapm-v0.1.md#req-mf-022) | MUST | 4.3.2 | consumer | active | 1 | +| [req-mf-023](docs/src/content/docs/specs/openapm-v0.1.md#req-mf-023) | MUST | 4.5 | consumer | active | 1 | | [req-pl-001](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-001) | MUST | 6.1 | governance | active | 1 | | [req-pl-002](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-002) | MUST | 6.2 | governance | active | 1 | | [req-pl-003](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-003) | MUST | 6.4 | governance | active | 1 | diff --git a/docs/public/specs/manifests/openapm-v0.1.requirements.yml b/docs/public/specs/manifests/openapm-v0.1.requirements.yml index a8ce3b637..160c43e41 100644 --- a/docs/public/specs/manifests/openapm-v0.1.requirements.yml +++ b/docs/public/specs/manifests/openapm-v0.1.requirements.yml @@ -111,6 +111,10 @@ requirements: keyword: MUST section: "4.3.2" conformance_class: consumer + - id: req-mf-023 + keyword: MUST + section: "4.5" + conformance_class: consumer - id: req-ext-001 keyword: MUST section: "4.1" diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index 8f6e87463..0f335f66b 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -114,6 +114,17 @@ VS Code prefers `npm`, PyPI, then OCI. OCI packages require Docker to be available when the harness starts the server; no per-target launcher configuration is needed. +When a required registry runtime variable has a default, APM prompts once +per variable and displays that default as the suggested answer. Press Enter +to accept it or provide an override. Secret defaults remain accepted on +Enter but are never displayed. For OCI/Docker launchers, non-secret selected +values replace every `{variable}` reference across the package's runtime and +package arguments before the native config is written. VS Code renders secret +variables as target-native secret-input references instead, so secret bytes +never enter `mcp.json`. A required variable without a collected value or +default declines that target configuration; VS Code treats `workspaceFolder` +as its built-in `${workspaceFolder}` token. + For VS Code and Copilot-family adapters, non-container `npm`, `pypi`, and generic packages preserve typed v0.1 `runtimeArguments` and `packageArguments` in authored order, with exactly one semantic package diff --git a/docs/src/content/docs/specs/openapm-v0.1.md b/docs/src/content/docs/specs/openapm-v0.1.md index 8e254ebbd..fe9682213 100644 --- a/docs/src/content/docs/specs/openapm-v0.1.md +++ b/docs/src/content/docs/specs/openapm-v0.1.md @@ -634,7 +634,7 @@ The OPTIONAL `devDependencies` block has the same structure as installed locally but excluded from packed plugin bundles produced by the producer toolchain. -### 4.5 Variable references in MCP env/headers +### 4.5 Variable references in MCP env/headers and runtime arguments Values inside `mcp[].env` and `mcp[].headers` MAY contain three placeholder syntaxes: @@ -656,6 +656,27 @@ literal text. When an unsupported placeholder is encountered for the active target, the consumer MUST emit a diagnostic and MAY refuse to write the generated config. +Registry OCI/Docker package `runtime_arguments` and `package_arguments` +entries MAY contain bare `{name}` templates in their `value` or `default` +fields. An entry's `variables` map declares metadata for variable names +across the package; `isSecret: true` marks a name secret. This syntax is +distinct from the `${...}` env/header forms above. + + +**[req-mf-023]** A conforming **consumer** implementation that renders +a registry OCI/Docker MCP package to VS Code configuration MUST apply a +resolved non-secret variable value to every `{name}` occurrence across +the package's runtime and package arguments, including an occurrence +whose argument does not repeat the variable metadata. Secret +classification is package-scoped: once any entry declares a name with +`isSecret: true`, the consumer MUST use the VS Code secret input +reference for every occurrence of that name rather than write the +resolved secret value into generated configuration bytes. The consumer +MUST NOT write a literal unresolved `{name}` template to generated VS +Code configuration; when a required runtime-argument variable cannot +be resolved, it MUST emit a diagnostic and MAY decline that package's +target configuration. + ### 4.6 Manifest extension surfaces #### 4.6.1 `policy` (consumer-side controls) @@ -752,6 +773,7 @@ This section's normative statements are: [req-mf-016](#req-mf-016), [req-mf-018](#req-mf-018), [req-mf-019](#req-mf-019), [req-mf-020](#req-mf-020), [req-mf-021](#req-mf-021), [req-mf-022](#req-mf-022), + [req-mf-023](#req-mf-023), [req-ext-001](#req-ext-001), [req-ext-002](#req-ext-002), [req-tg-004](#req-tg-004), [req-sc-006](#req-sc-006). @@ -2957,6 +2979,7 @@ conformance statement identifying: [req-mf-016](#req-mf-016), [req-mf-018](#req-mf-018), [req-mf-019](#req-mf-019), [req-mf-020](#req-mf-020), [req-mf-021](#req-mf-021), [req-mf-022](#req-mf-022), +[req-mf-023](#req-mf-023), [req-ext-001](#req-ext-001), [req-lk-001](#req-lk-001), [req-lk-002](#req-lk-002), [req-lk-003](#req-lk-003), [req-lk-004](#req-lk-004), @@ -3342,6 +3365,7 @@ renumbering of conformance classes. | [req-mf-020](#req-mf-020) | MUST | 4.1 | consumer | | [req-mf-021](#req-mf-021) | MUST | 4.8 | producer | | [req-mf-022](#req-mf-022) | MUST | 4.3.2 | consumer | +| [req-mf-023](#req-mf-023) | MUST | 4.5 | consumer | | [req-ext-001](#req-ext-001) | MUST | 4.1 | consumer | | [req-ext-002](#req-ext-002) | MUST | 4.1 | producer | | [req-lk-001](#req-lk-001) | MUST | 5.1 | consumer | @@ -3430,7 +3454,7 @@ renumbering of conformance classes. | [req-cf-001](#req-cf-001) | MUST | 12.5 | consumer | | [req-cf-002](#req-cf-002) | MUST | 12.3 | consumer | -**Total normative statements: 109** (104 MUST, 5 SHOULD). +**Total normative statements: 110** (105 MUST, 5 SHOULD). --- @@ -3465,6 +3489,7 @@ renumbering of conformance classes. | 0.1.23 | 2026-07-31 | Spec-citation fold for case-preserving dependency materialization (closes #2347). Added [req-lk-022] (Section 5.2, consumer MUST): a consumer that case-folds repository identity but retains different source spelling records `materialization_repo_url`, validates it maps to the same canonical identity, excludes it from identity/cache/sort/trust decisions, preserves exact virtual-path casing, and either transactionally migrates one stale case variant or fails closed without deleting colliding paths. Defined rollback semantics for case-only rename and preserved interrupted recovery state. Added the field to the lockfile schema and conformance fixture, plus migration and collision conformance oracles. Hardened lockfile schema: `repo_url` now carries `minLength: 1` to match the prose requirement that git-sourced entries provide a non-empty canonical identifier ([req-lk-003](#req-lk-003)). Section 5.7, Section 10.11, Section 11.3.2, and Appendix C updated. Statement count: 106 -> 107 (102 MUST, 5 SHOULD). | | 0.1.24 | 2026-08-03 | Spec-citation fold for fail-closed Kiro agent vocabulary gate (closes #2089 Mode-B silent-extension gate). Added [req-tg-009] (Section 8.5.1, consumer MUST): a consumer deploying an agent primitive into a target with a fixed, enumerable capability vocabulary MUST fail closed -- writing zero bytes and emitting an actionable diagnostic -- if any source-declared tool falls outside the approved set; the gate fires per agent independently and does not block vocabulary-conformant sibling agents; the gate applies only to targets included in the effective intersection under [req-tg-008]; content-identity fast-paths are not exempt. Added editorial note naming the Target Registry companion as the vocabulary authority and mandating version-pinning for conformance testing. Section 8.7, Section 11.3.2 Consumer enumeration, and Appendix C updated. Statement count: 107 -> 108 (103 MUST, 5 SHOULD). | | 0.1.25 | 2026-08-03 | Spec-citation fold for portable project-scoped Claude hooks (closes #2408 Mode-B silent-extension gate). Added [req-tg-010] (Section 8.5.4, consumer MUST): a project-scoped native hook that may launch outside the consumer project anchors its generated command through the target portable project-directory environment variable, preserves the relative hook path, executes successfully when the variable identifies the consumer project, and never embeds an absolute checkout path; shell-expansion path syntax is rejected. Claude uses `CLAUDE_PROJECT_DIR` in POSIX and `$env:CLAUDE_PROJECT_DIR` in PowerShell. Section 8.7, Section 11.3.2 Consumer enumeration, and Appendix C updated. Statement count: 108 -> 109 (104 MUST, 5 SHOULD). | +| 0.1.26 | 2026-08-03 | Spec-citation fold for VS Code OCI/Docker MCP runtime argument resolution (closes #2438). Added [req-mf-023] (Section 4.5, consumer MUST): a non-secret runtime variable resolves every `{name}` occurrence across package runtime and package arguments, an unresolved template is never written literally, and package-scoped secret metadata uses VS Code secret-input references instead of generated config bytes. Section 4.9, Section 11.3.2, and Appendix C updated. Statement count: 109 -> 110 (105 MUST, 5 SHOULD). | Errata (none at publication). diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 2aa46bd55..7a57a0f99 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -48,6 +48,14 @@ A normal project install creates or updates `apm.lock.yaml` when the manifest de `apm install --frozen` validates package and MCP lock state before lockfile, target config, deployment, or cache mutation. A missing or stale MCP-only lock exits nonzero without writing; run normal `apm install` to repair it. `--only=mcp` follows the same guard. Add-style `--mcp NAME` is incompatible with `--frozen` because it mutates `apm.yml`. +### Registry MCP runtime variables + +For registry MCP runtime variables, `apm install` prompts once for a required +non-secret default and accepts an override; secret defaults remain hidden. +Non-secret values resolve every matching `{variable}` launcher reference, +while VS Code uses secret-input references so secret bytes stay out of +`mcp.json`. A missing required value declines that target configuration. + ### Target resolution chain `apm install` resolves harness targets in strict priority order: diff --git a/scripts/lint-architecture-boundaries.sh b/scripts/lint-architecture-boundaries.sh index 83e28737c..0ddb34855 100755 --- a/scripts/lint-architecture-boundaries.sh +++ b/scripts/lint-architecture-boundaries.sh @@ -1481,6 +1481,18 @@ if [ "$apply_to_normalizer_defs" -ne 1 ] \ violations=$((violations + 1)) fi +echo "[*] AC32: MCP runtime argument variable authority" +mcp_runtime_variable_owner_defs=$(grep -rEc \ + '^[[:space:]]*def _substitute_runtime_variables\(' \ + src/apm_cli/adapters/client --include='*.py' \ + | awk -F: '{sum += $2} END {print sum + 0}') +if [ "$mcp_runtime_variable_owner_defs" -ne 1 ] \ + || ! grep -q '^ def _substitute_runtime_variables(' "$mcp_container_owner" \ + || ! grep -q 'cls\._substitute_runtime_variables(' src/apm_cli/adapters/client/vscode.py; then + echo "[x] MCP runtime argument variables must route through MCPClientAdapter" + violations=$((violations + 1)) +fi + if [ "$violations" -gt 0 ]; then echo "[x] $violations architecture boundary rule(s) failed" exit 1 diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index 3e7b1dac0..69c06d329 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -29,6 +29,7 @@ # Detects the legacy ```` placeholder syntax only. Used to aggregate # deprecation warnings across all servers in a single install run. _LEGACY_ANGLE_VAR_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>") +_RUNTIME_TEMPLATE_VARIABLE_RE = re.compile(r"(? dict[str, dict] | None: + """Return one validated runtime-variable declaration map for a package. + + Registry metadata may declare a variable on one argument and reference it + later without repeating the declaration. Keeping this map package-scoped + makes those references resolve consistently while rejecting malformed + secret metadata before an adapter can render a collected value. + """ + metadata_by_name: dict[str, dict] = {} + for field_name in ("runtime_arguments", "package_arguments"): + for argument in (package or {}).get(field_name) or []: + if not isinstance(argument, dict): + continue + variables = argument.get("variables") + if variables is None: + continue + if not isinstance(variables, dict): + return None + for name, metadata in variables.items(): + if not isinstance(name, str) or not isinstance(metadata, dict): + return None + secret_marker = metadata.get("isSecret", metadata.get("is_secret", False)) + if not isinstance(secret_marker, bool): + return None + existing = metadata_by_name.get(name) + if existing is not None and existing != metadata: + return None + metadata_by_name[name] = metadata + return metadata_by_name + + @staticmethod + def _substitute_runtime_variables( + template: str, + variables: dict[object, object] | None, + runtime_vars: dict[str, object] | None, + runtime_variable_fallbacks: dict[str, str] | None = None, + secret_variable_fallbacks: dict[str, str] | None = None, + ) -> str | None: + """Resolve every APM runtime variable in one registry argument template. + + Variable metadata may appear on an earlier argument while later + arguments reference the same value. Collected values therefore apply to + every template, not only the metadata-bearing entry. Target-specific + fallbacks are used only for declared variables with no collected value. + """ + secret_fallbacks = secret_variable_fallbacks or {} + values: dict[str, str] = dict(secret_fallbacks) + for name, value in (runtime_vars or {}).items(): + if ( + isinstance(name, str) + and name not in secret_fallbacks + and value is not None + and str(value) != "" + ): + values[name] = str(value) + + fallbacks = runtime_variable_fallbacks or {} + for name, metadata in (variables or {}).items(): + if not isinstance(name, str) or not isinstance(metadata, dict): + return None + placeholder = f"{{{name}}}" + if placeholder not in template or name in values: + continue + secret_marker = metadata.get("isSecret", metadata.get("is_secret", False)) + if not isinstance(secret_marker, bool): + return None + if secret_marker is True: + return None + fallback = fallbacks.get(name) + if fallback is not None: + values[name] = fallback + continue + configured = metadata.get("value", metadata.get("default")) + if isinstance(configured, str) and configured: + values[name] = configured + elif configured is not None: + return None + + for name, value in values.items(): + template = template.replace(f"{{{name}}}", value) + + if _RUNTIME_TEMPLATE_VARIABLE_RE.search(template): + return None + return template + @staticmethod def _processed_non_container_argument_groups( arguments: list[object], diff --git a/src/apm_cli/adapters/client/vscode.py b/src/apm_cli/adapters/client/vscode.py index f6b48bcdc..369a55d37 100644 --- a/src/apm_cli/adapters/client/vscode.py +++ b/src/apm_cli/adapters/client/vscode.py @@ -5,6 +5,7 @@ https://code.visualstudio.com/docs/copilot/chat/mcp-servers """ +import hashlib import json import re from pathlib import Path @@ -13,7 +14,13 @@ from ...registry.client import SimpleRegistryClient from ...registry.integration import RegistryIntegration from ...utils.console import _rich_warning -from .base import _ENV_VAR_RE, _INPUT_VAR_RE, MCPClientAdapter, registry_field_is_required +from .base import ( + _ENV_VAR_RE, + _INPUT_VAR_RE, + _RUNTIME_TEMPLATE_VARIABLE_RE, + MCPClientAdapter, + registry_field_is_required, +) # Legacy ```` placeholder (Copilot CLI / Codex only). VS Code does not # resolve angle-bracket placeholders, so emitting them produces literal @@ -298,7 +305,8 @@ def _format_server_config( existing_server_config=existing_server_config, ) secret_fallbacks, argument_inputs = self._declared_vscode_argument_secret_inputs( - package + package, + server_name=server_info.get("name", ""), ) seen_input_ids = {item.get("id") for item in input_vars} for input_definition in (*declared_inputs, *argument_inputs): @@ -342,19 +350,29 @@ def _format_server_config( # Handle docker packages elif is_docker: - args = self._docker_run_args(package, runtime_vars) + docker_kwargs = ( + {"secret_variable_fallbacks": secret_fallbacks} if secret_fallbacks else {} + ) + unresolved_variables: list[str] = [] + args = self._docker_run_args( + package, + runtime_vars, + unresolved_variables=unresolved_variables, + **docker_kwargs, + ) if args is None: if package.get("runtime_arguments") or package.get("package_arguments"): + variable_name = ( + unresolved_variables[0] if unresolved_variables else "unknown" + ) _rich_warning( - "Could not resolve container run options for " - f"'{package.get('name', '')}'; using the default launcher. " - "Set the required registry runtime variables and rerun " - "'apm install'." + "Could not resolve required container run option " + f"'{variable_name}' for '{package.get('name', '')}'; " + "target configuration was not changed. Set the required " + "registry runtime variables and rerun 'apm install'." ) - args = self._ensure_docker_image_arg( - ["run", "-i", "--rm"], - package.get("name"), - ) + return {}, [] + args = self._ensure_docker_image_arg(["run", "-i", "--rm"], package.get("name")) server_config = {"type": "stdio", "command": "docker", "args": args} @@ -547,50 +565,50 @@ def _declared_vscode_environment( return env_config, input_vars @staticmethod - def _declared_vscode_argument_secret_inputs(package): + def _vscode_argument_secret_input_id(server_name: str, variable_name: str) -> str: + """Return a collision-safe VS Code input ID for one server variable.""" + server_digest = hashlib.sha256(server_name.encode("utf-8")).hexdigest()[:12] + return f"mcp-{server_digest}-{variable_name.encode('utf-8').hex()}" + + @classmethod + def _declared_vscode_argument_secret_inputs(cls, package, *, server_name: str): """Return VS Code input indirections for secret argument variables.""" if not package: return {}, [] + package_variables = cls._package_runtime_variable_metadata(package) + if package_variables is None: + raise ValueError("MCP package argument variable metadata must be valid") fallbacks = {} input_vars = [] + templates = [] for field_name in ("runtime_arguments", "package_arguments"): for argument in package.get(field_name) or []: if not isinstance(argument, dict): continue - variables = argument.get("variables") - if not isinstance(variables, dict): - continue template = argument.get( "value", argument.get("default", argument.get("value_hint", "")), ) - for variable_name, variable_info in variables.items(): - if not isinstance(variable_name, str) or not isinstance( - variable_info, - dict, - ): - continue - if not isinstance(template, str) or f"{{{variable_name}}}" not in template: - continue - is_secret = variable_info.get( - "isSecret", - variable_info.get("is_secret", False), - ) - if is_secret is not True or variable_name in fallbacks: - continue - input_id = variable_name.lower().replace("_", "-") - fallbacks[variable_name] = f"${{input:{input_id}}}" - input_vars.append( - { - "type": "promptString", - "id": input_id, - "description": variable_info.get( - "description", - f"{variable_name} for MCP server", - ), - "password": True, - } - ) + if isinstance(template, str): + templates.append(template) + for variable_name, variable_info in package_variables.items(): + if not any(f"{{{variable_name}}}" in template for template in templates): + continue + if not variable_info.get("isSecret", variable_info.get("is_secret", False)): + continue + input_id = cls._vscode_argument_secret_input_id(server_name, variable_name) + fallbacks[variable_name] = f"${{input:{input_id}}}" + input_vars.append( + { + "type": "promptString", + "id": input_id, + "description": variable_info.get( + "description", + f"{variable_name} for MCP server", + ), + "password": True, + } + ) return fallbacks, input_vars @staticmethod @@ -776,7 +794,14 @@ def _extract_package_args(package, runtime_vars=None): return extracted @classmethod - def _docker_run_args(cls, package: dict, runtime_vars: dict | None = None) -> list[str] | None: + def _docker_run_args( + cls, + package: dict, + runtime_vars: dict | None = None, + *, + secret_variable_fallbacks: dict[str, str] | None = None, + unresolved_variables: list[str] | None = None, + ) -> list[str] | None: """Build ``docker run`` arguments from a container package's metadata. Container packages retain a dedicated assembler because Docker requires @@ -813,10 +838,27 @@ def _docker_run_args(cls, package: dict, runtime_vars: dict | None = None) -> li """ if not package: return None - run_options = cls._docker_arg_values(package.get("runtime_arguments"), runtime_vars) + package_variables = cls._package_runtime_variable_metadata(package) + if package_variables is None: + if unresolved_variables is not None: + unresolved_variables.append("invalid metadata") + return None + run_options = cls._docker_arg_values( + package.get("runtime_arguments"), + runtime_vars, + secret_variable_fallbacks, + package_variables, + unresolved_variables, + ) if run_options is None: return None - package_args = cls._docker_arg_values(package.get("package_arguments"), runtime_vars) + package_args = cls._docker_arg_values( + package.get("package_arguments"), + runtime_vars, + secret_variable_fallbacks, + package_variables, + unresolved_variables, + ) if package_args is None: return None if not run_options and package_args and package_args[0] == "run": @@ -855,7 +897,12 @@ def _docker_run_args(cls, package: dict, runtime_vars: dict | None = None) -> li @classmethod def _docker_arg_values( - cls, entries: list | None, runtime_vars: dict | None + cls, + entries: list | None, + runtime_vars: dict | None, + secret_variable_fallbacks: dict[str, str] | None, + package_variables: dict[str, dict], + unresolved_variables: list[str] | None, ) -> list[str] | None: """Resolve one registry argument list into CLI argument strings. @@ -903,47 +950,26 @@ def _docker_arg_values( continue template = str(raw) - variables = arg.get("variables") - if isinstance(variables, dict) and variables: - substituted = cls._substitute_runtime_variables(template, variables, runtime_vars) - if substituted is None: - if required: - return None - continue - template = substituted + substituted = cls._substitute_runtime_variables( + template, + package_variables, + runtime_vars, + {"workspaceFolder": "${workspaceFolder}"}, + secret_variable_fallbacks, + ) + if substituted is None: + if required: + if unresolved_variables is not None: + match = _RUNTIME_TEMPLATE_VARIABLE_RE.search(template) + unresolved_variables.append(match.group(1) if match else "unknown") + return None + continue + template = substituted if arg_type == "named" and arg.get("name"): args.append(str(arg["name"])) args.append(template) return args - @staticmethod - def _substitute_runtime_variables( - template: str, variables: dict, runtime_vars: dict | None - ) -> str | None: - """Substitute ``{var}`` placeholders, or return None if unresolvable. - - ``workspaceFolder`` resolves to VS Code's own ``${workspaceFolder}`` - token, which the editor expands at server start. Any other name that - this install did not collect a value for has no such fallback, so the - caller is told to decline rather than write a literal ``${name}`` into - a mount path. - """ - for var_name in variables: - placeholder = f"{{{var_name}}}" - # A descriptor the template never references cannot make it - # unresolvable; declining on one would drop a usable argument. - if placeholder not in template: - continue - supplied = (runtime_vars or {}).get(var_name) - if supplied is not None and str(supplied) != "": - replacement = str(supplied) - elif var_name == "workspaceFolder": - replacement = "${workspaceFolder}" - else: - return None - template = template.replace(placeholder, replacement) - return template - @staticmethod def _select_remote_with_url(remotes): """Return the first remote entry that has a non-empty URL. diff --git a/src/apm_cli/registry/operations.py b/src/apm_cli/registry/operations.py index 0be6be083..56bf522bf 100644 --- a/src/apm_cli/registry/operations.py +++ b/src/apm_cli/registry/operations.py @@ -324,7 +324,10 @@ def collect_runtime_variables( # Prompt user for collected runtime variables. if collected_runtime_vars: - return self._prompt_for_environment_variables(collected_runtime_vars) + return self._prompt_for_environment_variables( + collected_runtime_vars, + prompt_defaults=True, + ) return {} @@ -410,11 +413,19 @@ def collect_environment_variables( return shared_env_vars - def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> dict[str, str]: - """Prompt user for environment variables. + def _prompt_for_environment_variables( + self, + required_vars: dict[str, dict], + *, + prompt_defaults: bool = False, + ) -> dict[str, str]: + """Prompt user for registry-provided variables. Args: required_vars: Dictionary mapping var names to their metadata + prompt_defaults: Prompt for required defaults instead of applying + them silently. Runtime argument variables use this so a registry + default remains an overrideable suggestion. Returns: Dictionary mapping variable names to their values @@ -471,7 +482,12 @@ def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> d from rich.prompt import Prompt console = Console() - console.print("Environment variables needed:", style="cyan") + console.print( + "Registry runtime variables needed:" + if prompt_defaults + else "Environment variables needed:", + style="cyan", + ) for var_name in sorted(required_vars.keys()): var_info = required_vars[var_name] @@ -485,7 +501,7 @@ def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> d if existing_value: console.print(f" [+] {var_name}: [dim]using existing value[/dim]") env_vars[var_name] = existing_value - elif default_value: + elif default_value and not (prompt_defaults and required): if var_info.get("secret", False) is True: _rich_info(f"Using registry default for secret MCP variable '{var_name}'.") env_vars[var_name] = default_value @@ -502,7 +518,15 @@ def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> d if description: prompt_text += f" ({description})" - value = Prompt.ask(prompt_text, password=is_sensitive) + prompt_default = "" if is_sensitive else default_value + value = Prompt.ask( + prompt_text, + default=prompt_default, + show_default=bool(prompt_default), + password=is_sensitive, + ) + if not value and is_sensitive: + value = default_value env_vars[var_name] = value @@ -512,7 +536,11 @@ def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> d # Fallback to simple input import click - click.echo("Environment variables needed:") + click.echo( + "Registry runtime variables needed:" + if prompt_defaults + else "Environment variables needed:" + ) for var_name in sorted(required_vars.keys()): var_info = required_vars[var_name] @@ -525,7 +553,7 @@ def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> d if existing_value: click.echo(f" [+] {var_name}: using existing value") env_vars[var_name] = existing_value - elif default_value: + elif default_value and not (prompt_defaults and required): if var_info.get("secret", False) is True: _rich_info(f"Using registry default for secret MCP variable '{var_name}'.") env_vars[var_name] = default_value @@ -541,10 +569,16 @@ def _prompt_for_environment_variables(self, required_vars: dict[str, dict]) -> d keyword in var_name.lower() for keyword in ["password", "secret", "key", "token", "api"] ) + prompt_default = "" if is_sensitive else default_value value = click.prompt( - prompt_text, hide_input=is_sensitive, default="", show_default=False + prompt_text, + hide_input=is_sensitive, + default=prompt_default, + show_default=bool(prompt_default), ) + if not value and is_sensitive: + value = default_value env_vars[var_name] = value click.echo() diff --git a/tests/integration/test_architecture_authorities.py b/tests/integration/test_architecture_authorities.py index 46f50d2af..5432ade02 100644 --- a/tests/integration/test_architecture_authorities.py +++ b/tests/integration/test_architecture_authorities.py @@ -2861,3 +2861,64 @@ def test_mcp_noncontainer_launcher_guard_rejects_retired_extractor( assert result.returncode == 1 assert "MCP non-container launcher argv must route through MCPClientAdapter" in result.stdout + + +def test_mcp_runtime_argument_variables_have_one_canonical_owner() -> None: + """Runtime substitutions must stay in the shared MCP client adapter.""" + root = Path(__file__).parents[2] + owner = root / "src/apm_cli/adapters/client/base.py" + consumer = root / "src/apm_cli/adapters/client/vscode.py" + definitions = [ + node + for path in (owner, consumer) + for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == "_substitute_runtime_variables" + ] + + assert len(definitions) == 1 + assert any( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "_substitute_runtime_variables" + for node in ast.walk(ast.parse(consumer.read_text(encoding="utf-8"))) + ) + guard = (root / "scripts/lint-architecture-boundaries.sh").read_text(encoding="utf-8") + assert "MCP runtime argument variables must route through MCPClientAdapter" in guard + + +def test_mcp_runtime_argument_variable_guard_rejects_parallel_owner(tmp_path: Path) -> None: + """AC32 rejects a second adapter-local runtime variable resolver.""" + root = Path(__file__).parents[2] + sandbox = tmp_path / "repo" + shutil.copytree( + root, + sandbox, + ignore=shutil.ignore_patterns( + ".git", + ".venv", + ".pytest_cache", + "__pycache__", + "build", + "dist", + "node_modules", + ), + ) + vscode_path = sandbox / "src/apm_cli/adapters/client/vscode.py" + vscode_path.write_text( + vscode_path.read_text(encoding="utf-8") + + "\n def _substitute_runtime_variables(self):\n pass\n", + encoding="utf-8", + ) + + result = subprocess.run( + ("bash", "scripts/lint-architecture-boundaries.sh"), + cwd=sandbox, + capture_output=True, + text=True, + check=False, + timeout=300, + ) + + assert result.returncode == 1 + assert "MCP runtime argument variables must route through MCPClientAdapter" in result.stdout diff --git a/tests/integration/test_noncontainer_mcp_lifecycle_contract.py b/tests/integration/test_noncontainer_mcp_lifecycle_contract.py index cbdb0874d..ce69d9b93 100644 --- a/tests/integration/test_noncontainer_mcp_lifecycle_contract.py +++ b/tests/integration/test_noncontainer_mcp_lifecycle_contract.py @@ -11,6 +11,7 @@ import pytest +from apm_cli.adapters.client.vscode import VSCodeClientAdapter from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner from tests.utils.isolated_apm_environment import IsolatedApmEnvironment from tests.utils.lifecycle_state import LifecycleStateRoot, LifecycleStateSnapshot @@ -241,7 +242,11 @@ def _assert_idempotent_state( def _expected_args(case: _TargetCase) -> dict[str, list[str]]: """Return exact typed argv for all registry packages on one target.""" secret = ( - "${input:mcp-lifecycle-secret}" if case.runtime == "vscode" else f"${{{_SECRET_VARIABLE}}}" + "${input:" + f"{VSCodeClientAdapter._vscode_argument_secret_input_id(_GENERIC_SERVER, _SECRET_VARIABLE)}" + "}" + if case.runtime == "vscode" + else f"${{{_SECRET_VARIABLE}}}" ) return { _NPM_SERVER: [ diff --git a/tests/integration/test_oci_mcp_lifecycle_contract.py b/tests/integration/test_oci_mcp_lifecycle_contract.py index b1703debe..40a39351c 100644 --- a/tests/integration/test_oci_mcp_lifecycle_contract.py +++ b/tests/integration/test_oci_mcp_lifecycle_contract.py @@ -137,21 +137,27 @@ def _vscode_value_server_document() -> dict[str, object]: }, {"type": "named", "name": "--read-only"}, { - "type": "named", - "name": "--mount", - "value": f"type=bind,src={{{_WORKDIR_VARIABLE}}},dst=/workspace", - "valueHint": "mount_spec", + "type": "positional", + "value": "-v", + }, + { + "type": "positional", + "value": f"{{{_WORKDIR_VARIABLE}}}:{{{_WORKDIR_VARIABLE}}}", "variables": { _WORKDIR_VARIABLE: { "description": "Workspace path mounted into the container", "isRequired": True, + "default": "${workspaceFolder}", } }, }, { - "type": "named", - "name": "--workdir", - "default": "/workspace", + "type": "positional", + "value": "-w", + }, + { + "type": "positional", + "value": f"{{{_WORKDIR_VARIABLE}}}", }, ], "packageArguments": [ @@ -340,7 +346,7 @@ def test_oci_mcp_install_lifecycle_is_cross_adapter_and_idempotent( assert audit_payload["passed"] is True -def test_vscode_typed_value_mount_survives_install_update_and_offline_audit( +def test_mcp_runtime_variable_default_override_lifecycle( tmp_path: Path, apm_binary_path: Path, ) -> None: @@ -387,10 +393,10 @@ def test_vscode_typed_value_mount_survives_install_update_and_offline_audit( "-i", "--rm", "--read-only", - "--mount", - f"type=bind,src={_WORKDIR_VALUE},dst=/workspace", - "--workdir", - "/workspace", + "-v", + f"{_WORKDIR_VALUE}:{_WORKDIR_VALUE}", + "-w", + _WORKDIR_VALUE, _VALUE_SERVER_IMAGE, "--transport", "stdio", @@ -421,7 +427,7 @@ def test_vscode_typed_value_mount_survives_install_update_and_offline_audit( runner.run_sequence( (initial_install,), expected_returncodes=(0,), - scenario_id="vscode-value-initial-install", + scenario_id="mcp-runtime-variable-default-override-initial-install", cwd=project, env=install_env, ) @@ -441,7 +447,7 @@ def test_vscode_typed_value_mount_survives_install_update_and_offline_audit( runner.run_sequence( (reinstall, update), expected_returncodes=(0, 0), - scenario_id="vscode-value-reinstall-update", + scenario_id="mcp-runtime-variable-default-override-reinstall-update", cwd=project, env=install_env, ) @@ -454,7 +460,7 @@ def test_vscode_typed_value_mount_survives_install_update_and_offline_audit( before_audit = LifecycleStateSnapshot.capture(project, config_paths=(config_path,)) audit = runner.run( ("audit", "--ci", "--no-policy", "--format", "json"), - scenario_id="vscode-value-offline-audit", + scenario_id="mcp-runtime-variable-default-override-offline-audit", cwd=project, env=audit_env, ) diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index 41d1e6028..e477a08fa 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -1,6 +1,6 @@ """Manifest (apm.yml) + scheme + tag + conformance-class tests. -Covers req-mf-001..022, req-ext-001..002, req-sc-001..010, +Covers req-mf-001..023, req-ext-001..002, req-sc-001..010, req-tg-001..008, req-cf-001..002. Every requirement is exercised either by (a) schema validation @@ -18,6 +18,7 @@ import jsonschema import pytest +from apm_cli.adapters.client.base import MCPClientAdapter from apm_cli.install.phases.finalize import _hint_project_compile_needed from apm_cli.install.target_filter import resolve_effective_package_targets from apm_cli.integration.agent_integrator import AgentIntegrator @@ -220,6 +221,34 @@ def test_consumer_enforces_yaml_safe_subset(): assert_spec_contains("YAML safe", "&anchor", "MUST be rejected", "YAML 1.1 octal") +@pytest.mark.req("req-mf-023") +def test_consumer_resolves_runtime_argument_templates_without_secret_leakage(): + """Runtime templates resolve completely and keep secret bytes target-native.""" + assert_spec_contains( + "every `{name}` occurrence", + "MUST NOT write a literal unresolved `{name}` template", + "Secret\nclassification is package-scoped", + "VS Code secret input\nreference", + ) + assert ( + MCPClientAdapter._substitute_runtime_variables( + "{workdir}:{workdir}", + {"workdir": {"default": "/default"}}, + {"workdir": "/override"}, + ) + == "/override:/override" + ) + assert ( + MCPClientAdapter._substitute_runtime_variables( + "{token}:{token}", + {"token": {"isSecret": True}}, + {"token": "raw-secret"}, + secret_variable_fallbacks={"token": "${input:token}"}, + ) + == "${input:token}:${input:token}" + ) + + @pytest.mark.req("req-mf-021") def test_producer_workspaces_must_not_use_in_v0_1(): """req-mf-021 forbids workspaces in v0.1.""" diff --git a/tests/unit/adapters/test_noncontainer_launcher_args.py b/tests/unit/adapters/test_noncontainer_launcher_args.py index a464bbb85..c028a662c 100644 --- a/tests/unit/adapters/test_noncontainer_launcher_args.py +++ b/tests/unit/adapters/test_noncontainer_launcher_args.py @@ -333,13 +333,17 @@ def test_vscode_argument_secret_uses_existing_input_contract(tmp_path: Path) -> tmp_path, runtime_vars={"MCP_TEST_SECRET": "must-not-be-written"}, ) + input_id = VSCodeClientAdapter._vscode_argument_secret_input_id( + "com.example/noncontainer", + "MCP_TEST_SECRET", + ) - assert config["args"] == ["mcp-server-secret", "${input:mcp-test-secret}"] + assert config["args"] == ["mcp-server-secret", f"${{input:{input_id}}}"] assert "must-not-be-written" not in repr(config) assert inputs == [ { "type": "promptString", - "id": "mcp-test-secret", + "id": input_id, "description": "Secret used by the launcher", "password": True, } diff --git a/tests/unit/adapters/test_vscode_docker_runtime_args.py b/tests/unit/adapters/test_vscode_docker_runtime_args.py index 8a5ce222a..6b35cc2dc 100644 --- a/tests/unit/adapters/test_vscode_docker_runtime_args.py +++ b/tests/unit/adapters/test_vscode_docker_runtime_args.py @@ -4,7 +4,7 @@ """ import unittest -from unittest.mock import patch +from unittest.mock import ANY, patch from apm_cli.adapters.client.vscode import VSCodeClientAdapter @@ -230,7 +230,11 @@ def test_format_server_config_threads_runtime_vars(self): wraps=VSCodeClientAdapter._docker_run_args, ) as mock_builder: self.adapter._format_server_config(self.server_info, runtime_vars=runtime_vars) - mock_builder.assert_called_once_with(V01_DOCKER_PACKAGE, runtime_vars) + mock_builder.assert_called_once_with( + V01_DOCKER_PACKAGE, + runtime_vars, + unresolved_variables=ANY, + ) # --------------------------------------------------------------------------- diff --git a/tests/unit/adapters/test_vscode_v01_value_runtime_args.py b/tests/unit/adapters/test_vscode_v01_value_runtime_args.py index 6caadfb5a..844620abb 100644 --- a/tests/unit/adapters/test_vscode_v01_value_runtime_args.py +++ b/tests/unit/adapters/test_vscode_v01_value_runtime_args.py @@ -16,6 +16,7 @@ from __future__ import annotations +import json import unittest from unittest.mock import patch @@ -99,6 +100,26 @@ def test_collected_variable_reaches_the_mount_argument(self): args = VSCodeClientAdapter._docker_run_args(V01_VALUE_PACKAGE, RUNTIME_VARS) self.assertIn(f"{WORKDIR}:{WORKDIR}", args) + def test_collected_variable_substitutes_later_entry_without_metadata(self): + """One declaration must resolve every later reference to the variable (#2438).""" + package = _docker_package( + {"type": "positional", "value": "run"}, + { + "type": "positional", + "value": "{workdir}:{workdir}", + "variables": _WORKDIR_VAR, + }, + {"type": "positional", "value": "-w"}, + {"type": "positional", "value": "{workdir}"}, + ) + + args = VSCodeClientAdapter._docker_run_args(package, RUNTIME_VARS) + + self.assertEqual( + args, + ["run", "-i", "--rm", f"{WORKDIR}:{WORKDIR}", "-w", WORKDIR, IMAGE], + ) + def test_typed_entries_read_value_not_the_display_hint(self): """`value_hint` is the schema's display hint for a typed argument. @@ -267,6 +288,28 @@ def test_workspace_folder_uses_the_vscode_native_token(self): "${workspaceFolder}:/workspace", VSCodeClientAdapter._docker_run_args(package) ) + def test_workspace_folder_fallback_reaches_later_metadata_free_argument(self): + package = _docker_package( + {"value": "run", "type": "positional"}, + { + "value": "{workspaceFolder}:/workspace", + "type": "positional", + "variables": {"workspaceFolder": {"description": "ws"}}, + }, + {"value": "-w", "type": "positional"}, + {"value": "{workspaceFolder}", "type": "positional"}, + ) + + assert VSCodeClientAdapter._docker_run_args(package) == [ + "run", + "-i", + "--rm", + "${workspaceFolder}:/workspace", + "-w", + "${workspaceFolder}", + IMAGE, + ] + def test_optional_entry_with_a_collected_value_is_kept(self): """An optional mount whose variable the user filled in must survive -- discarding user-supplied configuration is the #2377 failure mode.""" @@ -440,16 +483,115 @@ def test_launcher_is_no_longer_the_bare_fallback(self): config = _config(V01_VALUE_PACKAGE, runtime_vars=RUNTIME_VARS) self.assertNotEqual(config.get("args"), FALLBACK_ARGS) - def test_unresolvable_package_keeps_the_previous_launcher(self): + def test_secret_runtime_value_uses_a_vscode_input_everywhere(self): + """Secret values must not be written into target configuration bytes.""" + secret = "registry-secret" + package = _docker_package( + {"type": "positional", "value": "run"}, + { + "type": "positional", + "value": "--token={access_token}", + "variables": { + "access_token": { + "description": "Registry access token", + "isRequired": True, + "isSecret": True, + }, + "secondary_token": { + "description": "Secondary registry access token", + "isRequired": True, + "isSecret": True, + }, + }, + }, + {"type": "positional", "value": "--again={access_token}"}, + {"type": "positional", "value": "--secondary={secondary_token}"}, + ) + + config, inputs = _make_vscode()._format_server_config( + {"id": "team/mcp-server", "name": "team/mcp-server", "packages": [package]}, + runtime_vars={"access_token": secret, "secondary_token": "secondary-secret"}, + ) + access_input_id = VSCodeClientAdapter._vscode_argument_secret_input_id( + "team/mcp-server", "access_token" + ) + secondary_input_id = VSCodeClientAdapter._vscode_argument_secret_input_id( + "team/mcp-server", "secondary_token" + ) + + assert config["args"] == [ + "run", + "-i", + "--rm", + f"--token=${{input:{access_input_id}}}", + f"--again=${{input:{access_input_id}}}", + f"--secondary=${{input:{secondary_input_id}}}", + IMAGE, + ] + assert secret not in json.dumps(config) + assert "secondary-secret" not in json.dumps(config) + assert inputs == [ + { + "type": "promptString", + "id": access_input_id, + "description": "Registry access token", + "password": True, + }, + { + "type": "promptString", + "id": secondary_input_id, + "description": "Secondary registry access token", + "password": True, + }, + ] + + def test_unresolvable_package_declines_target_configuration(self): with patch("apm_cli.adapters.client.vscode._rich_warning") as warning: config = _config(V01_VALUE_PACKAGE) - self.assertEqual(config.get("args"), FALLBACK_ARGS) + self.assertEqual(config, {}) warning.assert_called_once_with( - "Could not resolve container run options for " - f"'{IMAGE}'; using the default launcher. " + "Could not resolve required container run option " + f"'workdir' for '{IMAGE}'; target configuration was not changed. " "Set the required registry runtime variables and rerun 'apm install'." ) + def test_secret_runtime_input_ids_do_not_collapse_distinct_variable_names(self): + package = _docker_package( + {"type": "positional", "value": "run"}, + { + "type": "positional", + "value": "--first={ACCESS_TOKEN}", + "variables": { + "ACCESS_TOKEN": {"description": "First", "isSecret": True}, + "access_token": {"description": "Second", "isSecret": True}, + }, + }, + {"type": "positional", "value": "--second={access_token}"}, + ) + + config, inputs = _make_vscode()._format_server_config( + {"id": "team/mcp-server", "name": "team/mcp-server", "packages": [package]}, + runtime_vars={"ACCESS_TOKEN": "first-secret", "access_token": "second-secret"}, + ) + + assert inputs[0]["id"] != inputs[1]["id"] + assert config["args"][3] != config["args"][4] + assert "first-secret" not in json.dumps(config) + assert "second-secret" not in json.dumps(config) + + def test_malformed_secret_marker_declines_before_rendering_a_secret(self): + package = _docker_package( + {"type": "positional", "value": "run"}, + { + "type": "positional", + "value": "--token={access_token}", + "variables": {"access_token": {"isSecret": "true"}}, + }, + ) + + with pytest.raises(ValueError, match="variable metadata must be valid"): + _config(package, runtime_vars={"access_token": "registry-secret"}) + def test_package_without_runtime_args_keeps_the_synthesized_launcher(self): with patch("apm_cli.adapters.client.vscode._rich_warning") as warning: config = _config({"name": IMAGE, "registry_name": "oci", "runtime_hint": "docker"}) diff --git a/tests/unit/test_registry_operations_phase3.py b/tests/unit/test_registry_operations_phase3.py index d6f084ec4..8fd038ab1 100644 --- a/tests/unit/test_registry_operations_phase3.py +++ b/tests/unit/test_registry_operations_phase3.py @@ -389,7 +389,8 @@ def test_preserves_variable_default_and_secret_metadata(self) -> None: "value": "registry-default", "secret": True, } - } + }, + prompt_defaults=True, ) def test_skips_non_string_runtime_variable_default(self) -> None: @@ -426,6 +427,160 @@ def test_skips_non_string_runtime_variable_default(self) -> None: prompt.assert_not_called() assert result == {} + def test_prompts_for_required_runtime_default_as_an_overrideable_suggestion( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Required runtime defaults must remain an interactive suggestion.""" + for variable in ( + "APM_E2E_TESTS", + "CI", + "GITHUB_ACTIONS", + "TRAVIS", + "JENKINS_URL", + "BUILDKITE", + "workdir", + ): + monkeypatch.delenv(variable, raising=False) + ops = _make_ops() + cache = { + "server-a": { + "packages": [ + { + "runtime_arguments": [ + { + "variables": { + "workdir": { + "description": "Working directory", + "isRequired": True, + "default": "${workspaceFolder}", + } + } + } + ] + } + ] + } + } + prompt = MagicMock(return_value="/workspace/override") + + with ( + patch("rich.console.Console"), + patch("rich.prompt.Prompt.ask", prompt), + ): + result = ops.collect_runtime_variables(["server-a"], server_info_cache=cache) + + assert result == {"workdir": "/workspace/override"} + prompt.assert_called_once_with( + " workdir (Working directory)", + default="${workspaceFolder}", + show_default=True, + password=False, + ) + + def test_hides_required_secret_runtime_default_while_allowing_override( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Secret defaults must never be shown in the interactive prompt.""" + for variable in ( + "APM_E2E_TESTS", + "CI", + "GITHUB_ACTIONS", + "TRAVIS", + "JENKINS_URL", + "BUILDKITE", + "ACCESS_CODE", + ): + monkeypatch.delenv(variable, raising=False) + ops = _make_ops() + prompt = MagicMock(return_value="override") + + with ( + patch("rich.console.Console"), + patch("rich.prompt.Prompt.ask", prompt), + ): + result = ops._prompt_for_environment_variables( + { + "ACCESS_CODE": { + "description": "Access code", + "required": True, + "value": "registry-secret", + "secret": True, + } + }, + prompt_defaults=True, + ) + + assert result == {"ACCESS_CODE": "override"} + prompt.assert_called_once_with( + " ACCESS_CODE (Access code)", + default="", + show_default=False, + password=True, + ) + + def test_empty_secret_response_retains_hidden_registry_default( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Accepting a concealed default must retain its configured secret value.""" + for variable in ( + "APM_E2E_TESTS", + "CI", + "GITHUB_ACTIONS", + "TRAVIS", + "JENKINS_URL", + "BUILDKITE", + "ACCESS_CODE", + ): + monkeypatch.delenv(variable, raising=False) + ops = _make_ops() + prompt = MagicMock(return_value="") + + with ( + patch("rich.console.Console"), + patch("rich.prompt.Prompt.ask", prompt), + ): + result = ops._prompt_for_environment_variables( + { + "ACCESS_CODE": { + "description": "Access code", + "required": True, + "value": "registry-secret", + "secret": True, + } + }, + prompt_defaults=True, + ) + + assert result == {"ACCESS_CODE": "registry-secret"} + assert prompt.call_args.kwargs["default"] == "" + assert prompt.call_args.kwargs["show_default"] is False + + def test_ci_keeps_required_runtime_defaults_without_prompting( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Non-interactive installs retain registry defaults for resolution.""" + monkeypatch.delenv("APM_E2E_TESTS", raising=False) + monkeypatch.setenv("CI", "1") + monkeypatch.delenv("workdir", raising=False) + ops = _make_ops() + + result = ops._prompt_for_environment_variables( + { + "workdir": { + "description": "Working directory", + "required": True, + "value": "/registry/default", + } + }, + prompt_defaults=True, + ) + + assert result == {"workdir": "/registry/default"} + def test_skips_exception_servers(self) -> None: ops = _make_ops() # Cache has a server whose info raises on get @@ -680,6 +835,47 @@ def test_click_fallback_when_no_rich(self, monkeypatch: pytest.MonkeyPatch) -> N ) assert result.get("CLICK_VAR") == "click-value" + def test_click_fallback_prompts_for_required_default_override( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The fallback preserves the Rich prompt's editable default contract.""" + for variable in ( + "APM_E2E_TESTS", + "CI", + "GITHUB_ACTIONS", + "TRAVIS", + "JENKINS_URL", + "BUILDKITE", + "CLICK_DEFAULT", + ): + monkeypatch.delenv(variable, raising=False) + ops = _make_ops() + prompt = MagicMock(return_value="click-override") + + with ( + patch.dict("sys.modules", {"rich": None, "rich.console": None, "rich.prompt": None}), + patch("click.prompt", prompt), + patch("click.echo"), + ): + result = ops._prompt_for_environment_variables( + { + "CLICK_DEFAULT": { + "description": "desc", + "required": True, + "value": "registry-default", + } + }, + prompt_defaults=True, + ) + + assert result == {"CLICK_DEFAULT": "click-override"} + prompt.assert_called_once_with( + " CLICK_DEFAULT (desc)", + hide_input=False, + default="registry-default", + show_default=True, + ) + def test_rich_uses_existing_env_var_without_prompting( self, monkeypatch: pytest.MonkeyPatch ) -> None: