[fix] suppress internal config keys from apm config get output (#564)#571
Open
edenfunf wants to merge 2 commits intomicrosoft:mainfrom
Open
[fix] suppress internal config keys from apm config get output (#564)#571edenfunf wants to merge 2 commits intomicrosoft:mainfrom
edenfunf wants to merge 2 commits intomicrosoft:mainfrom
Conversation
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
Contributor
There was a problem hiding this comment.
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 getto only print explicitly user-settable keys and silently skip internal keys likedefault_client. - Adjust unit tests to assert that
default_clientis not present inapm config getoutput 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_suppressedonly asserts thatdefault_clientis 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 whereapm config getcan 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.
Contributor
Author
|
@danielmeppiel Addressed both Copilot findings — pushed a follow-up commit. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
apm config get(no argument) printed every key in~/.apm/config.json, including the internaldefault_clientkey. Users sawdefault_client: vscodein the output but got an error if they triedapm config set default_client vscode:This get/set asymmetry is confusing and was flagged as a Medium severity finding in #564.
Why
The display loop in
commands/config.pymappedauto_integrate→auto-integratecorrectly, but had anelsefallthrough that printed any other key verbatim.default_clientis 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
elsefallthrough 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 ordefault_clientitself (still used internally bydefault_manager.py).Test
test_get_all_configto assertdefault_clientis not present inapm config getoutput when the config contains it.test_get_all_config_unknown_key_passthrough(which validated the buggy behaviour) withtest_get_all_config_internal_keys_suppressed, which asserts internal keys are silently hidden.All 23 existing config command tests pass.