Skip to content

[fix] suppress internal config keys from apm config get output (#564)#571

Open
edenfunf wants to merge 2 commits intomicrosoft:mainfrom
edenfunf:fix/config-get-hides-internal-keys
Open

[fix] suppress internal config keys from apm config get output (#564)#571
edenfunf wants to merge 2 commits intomicrosoft:mainfrom
edenfunf:fix/config-get-hides-internal-keys

Conversation

@edenfunf
Copy link
Copy Markdown
Contributor

@edenfunf edenfunf commented Apr 4, 2026

What

apm config get (no argument) printed every key in ~/.apm/config.json, including the internal default_client key. Users saw default_client: vscode in the output but got an error if they tried apm config set default_client vscode:

[x] Unknown configuration key: 'default_client'. Valid keys: auto-integrate

This get/set asymmetry is confusing and was flagged as a Medium severity finding in #564.

Why

The display loop in commands/config.py mapped auto_integrateauto-integrate correctly, but had an else fallthrough that printed any other key verbatim. default_client is an internal key used by the package manager adapter; it is not user-settable and should not appear in user-facing output.

How

Remove the else fallthrough so only explicitly mapped, user-settable keys are printed. Internal keys are silently skipped. This is the minimal change — no new abstraction, no changes to the underlying config storage or default_client itself (still used internally by default_manager.py).

Test

  • Updated test_get_all_config to assert default_client is not present in apm config get output when the config contains it.
  • Replaced test_get_all_config_unknown_key_passthrough (which validated the buggy behaviour) with test_get_all_config_internal_keys_suppressed, which asserts internal keys are silently hidden.

All 23 existing config command tests pass.

apm config get (no argument) iterated the raw config dict and passed any
key it did not recognise to a bare else branch, printing it as-is.  This
caused the internal default_client key to appear in the output even though
users cannot set it via apm config set — leading to a confusing
get/set asymmetry reported in microsoft#564.

Remove the else fallthrough so that only user-settable keys (currently
auto-integrate) are printed.  Internal keys are silently skipped.

Update the test that asserted the old passthrough behaviour and add an
explicit assertion that default_client is absent from the output.

Fixes microsoft#564
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a CLI consistency issue where apm config get (with no key) exposed internal config keys (notably default_client) that cannot be set via apm config set, causing confusing get/set asymmetry.

Changes:

  • Update apm config get to only print explicitly user-settable keys and silently skip internal keys like default_client.
  • Adjust unit tests to assert that default_client is not present in apm config get output and replace the previous “unknown key passthrough” expectation.
Show a summary per file
File Description
src/apm_cli/commands/config.py Removes the fallback that printed arbitrary config keys, so internal keys are no longer displayed.
tests/unit/test_config_command.py Updates tests to ensure internal keys (e.g., default_client) are suppressed from config get output.

Copilot's findings

Comments suppressed due to low confidence (1)

tests/unit/test_config_command.py:242

  • test_get_all_config_internal_keys_suppressed only asserts that default_client is hidden, but it doesn't assert that at least the supported user-facing key(s) (e.g. auto-integrate) are still shown with their default value when the config contains only internal keys. Adding that assertion would catch the current behavior where apm config get can print an empty config list on a fresh config file.
    def test_get_all_config_internal_keys_suppressed(self):
        """Internal config keys are not shown to users."""
        fake_config = {"default_client": "vscode"}
        with patch("apm_cli.config.get_config", return_value=fake_config):
            result = self.runner.invoke(config, ["get"])
        assert result.exit_code == 0
        assert "default_client" not in result.output
  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Address Copilot review feedback:
- Use get_auto_integrate() to always show auto-integrate with its
  effective value (including default), fixing the case where a fresh
  install shows no user-settable keys at all.
- Replace em dash with ASCII hyphen in test comments to comply with
  the repo ASCII-only rule for source files.
- Remove unused get_config import.
@edenfunf
Copy link
Copy Markdown
Contributor Author

edenfunf commented Apr 4, 2026

@danielmeppiel Addressed both Copilot findings — pushed a follow-up commit. Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants