fix(update): name new config options in migration prompt; skip prompt for pure version bumps (#35658)
The 'hermes update' config-migration prompt printed only counts ('1 new
config option available') then asked 'configure them now?' without ever
saying what the options were. Users said no because they couldn't tell what
they were agreeing to. For pure config-format version bumps (no new
env/config keys) it still asked the question, where saying yes just bumped
the version and looked like a no-op.
- List each new env var / config key by name + description before prompting
(cap at 8, then '… and N more'). The data was already available; we just
threw it away and printed a count.
- Pure version bump (no new options): apply the format migration
non-interactively and print what happened, instead of asking a misleading
yes/no.
Reported by ScottFive and Tt2021.
This commit is contained in:
@ -9557,16 +9557,61 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
||||
missing_config = get_missing_config_fields()
|
||||
current_ver, latest_ver = check_config_version()
|
||||
|
||||
needs_migration = missing_env or missing_config or current_ver < latest_ver
|
||||
has_new_options = bool(missing_env or missing_config)
|
||||
version_bump_only = (
|
||||
not has_new_options and current_ver < latest_ver
|
||||
)
|
||||
needs_migration = has_new_options or current_ver < latest_ver
|
||||
|
||||
if needs_migration:
|
||||
if version_bump_only:
|
||||
# Nothing for the user to fill in — only the config format version
|
||||
# changed (new defaults already merge in transparently). Asking
|
||||
# "configure new options now?" here is misleading: saying yes just
|
||||
# bumps the version and looks like a no-op (issue: ScottFive /
|
||||
# Tt2021). Apply it silently and say what actually happened.
|
||||
print()
|
||||
print(
|
||||
f" ℹ Updating config format (v{current_ver} → v{latest_ver})…"
|
||||
)
|
||||
try:
|
||||
migrate_config(interactive=False, quiet=True)
|
||||
print(" ✓ Config format updated (no new settings to configure)")
|
||||
except Exception as _mig_err:
|
||||
print(f" ⚠️ Config format update failed: {_mig_err}")
|
||||
print(" Run 'hermes config migrate' to retry.")
|
||||
elif needs_migration:
|
||||
print()
|
||||
# Show WHAT changed, not just a count, so the user can make an
|
||||
# informed yes/no decision (previously the prompt named nothing).
|
||||
def _print_items(items, label, key, fallback_key=None):
|
||||
if not items:
|
||||
return
|
||||
print(f" {label}:")
|
||||
shown = items[:8]
|
||||
for it in shown:
|
||||
if isinstance(it, dict):
|
||||
name = it.get(key) or (fallback_key and it.get(fallback_key)) or "?"
|
||||
desc = (it.get("description") or "").strip()
|
||||
else:
|
||||
# Defensive: some callers/mocks pass bare name strings.
|
||||
name = str(it)
|
||||
desc = ""
|
||||
if desc:
|
||||
print(f" • {name} — {desc}")
|
||||
else:
|
||||
print(f" • {name}")
|
||||
extra = len(items) - len(shown)
|
||||
if extra > 0:
|
||||
print(f" … and {extra} more")
|
||||
|
||||
if missing_env:
|
||||
print(
|
||||
f" ⚠️ {len(missing_env)} new required setting(s) need configuration"
|
||||
)
|
||||
_print_items(missing_env, "New settings", "name")
|
||||
if missing_config:
|
||||
print(f" ℹ️ {len(missing_config)} new config option(s) available")
|
||||
_print_items(missing_config, "New options", "key")
|
||||
|
||||
print()
|
||||
if assume_yes:
|
||||
|
||||
@ -281,6 +281,83 @@ class TestCmdUpdateBranchFallback:
|
||||
assert "API keys require manual entry" in captured.out
|
||||
|
||||
|
||||
class TestCmdUpdateMigrationPrompt:
|
||||
"""The config-migration prompt names what changed and skips the prompt
|
||||
entirely when only the config format version moved.
|
||||
|
||||
Regression guard for the contentless-prompt report (ScottFive / Tt2021):
|
||||
previously the prompt printed only counts ("1 new config option") and
|
||||
asked "configure them now?" even for pure version bumps, where saying
|
||||
yes looked like a no-op.
|
||||
"""
|
||||
|
||||
def test_version_bump_only_applies_silently_without_prompt(
|
||||
self, mock_args, capsys
|
||||
):
|
||||
"""Only the version moved → apply non-interactively, never prompt."""
|
||||
with patch("shutil.which", return_value=None), patch(
|
||||
"subprocess.run"
|
||||
) as mock_run, patch("builtins.input") as mock_input, patch(
|
||||
"hermes_cli.config.get_missing_env_vars", return_value=[]
|
||||
), patch(
|
||||
"hermes_cli.config.get_missing_config_fields", return_value=[]
|
||||
), patch(
|
||||
"hermes_cli.config.check_config_version", return_value=(5, 24)
|
||||
), patch(
|
||||
"hermes_cli.config.migrate_config",
|
||||
return_value={"env_added": [], "config_added": [], "warnings": []},
|
||||
) as mock_migrate:
|
||||
mock_run.side_effect = _make_run_side_effect(
|
||||
branch="main", verify_ok=True, commit_count="1"
|
||||
)
|
||||
|
||||
cmd_update(mock_args)
|
||||
|
||||
mock_input.assert_not_called()
|
||||
mock_migrate.assert_called_once_with(interactive=False, quiet=True)
|
||||
out = capsys.readouterr().out
|
||||
assert "Updating config format (v5 → v24)" in out
|
||||
assert "no new settings to configure" in out
|
||||
# The misleading question must NOT appear for a pure version bump.
|
||||
assert "configure them now" not in out.lower()
|
||||
|
||||
def test_new_options_are_listed_by_name_before_prompt(
|
||||
self, mock_args, capsys
|
||||
):
|
||||
"""New env/config keys are printed by name so the user can decide."""
|
||||
env_items = [
|
||||
{"name": "FOO_API_KEY", "description": "Foo service API key"},
|
||||
]
|
||||
cfg_items = [
|
||||
{"key": "display.new_widget", "description": "New config option: display.new_widget"},
|
||||
]
|
||||
with patch("shutil.which", return_value=None), patch(
|
||||
"subprocess.run"
|
||||
) as mock_run, patch("builtins.input", return_value="n"), patch(
|
||||
"hermes_cli.config.get_missing_env_vars", return_value=env_items
|
||||
), patch(
|
||||
"hermes_cli.config.get_missing_config_fields", return_value=cfg_items
|
||||
), patch(
|
||||
"hermes_cli.config.check_config_version", return_value=(1, 24)
|
||||
), patch(
|
||||
"hermes_cli.config.migrate_config",
|
||||
return_value={"env_added": [], "config_added": [], "warnings": []},
|
||||
), patch("hermes_cli.main.sys") as mock_sys:
|
||||
mock_sys.stdin.isatty.return_value = True
|
||||
mock_sys.stdout.isatty.return_value = True
|
||||
mock_run.side_effect = _make_run_side_effect(
|
||||
branch="main", verify_ok=True, commit_count="1"
|
||||
)
|
||||
|
||||
cmd_update(mock_args)
|
||||
|
||||
out = capsys.readouterr().out
|
||||
# Names, not just counts.
|
||||
assert "FOO_API_KEY" in out
|
||||
assert "Foo service API key" in out
|
||||
assert "display.new_widget" in out
|
||||
|
||||
|
||||
class TestCmdUpdateProfileSkillSync:
|
||||
"""cmd_update syncs bundled skills to all profiles, including the active one.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user