Skip to content

feat(config): add get and unset commands - #36067

Closed
nima20002000 wants to merge 1 commit into
NousResearch:mainfrom
nima20002000:feat/36037-config-get-unset
Closed

feat(config): add get and unset commands#36067
nima20002000 wants to merge 1 commit into
NousResearch:mainfrom
nima20002000:feat/36037-config-get-unset

Conversation

@nima20002000

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds scriptable hermes config get <key> and hermes config unset <key> commands so the existing config set flow has matching read and removal operations.

config get reads the resolved config tree, supports dotted dict/list paths, prints scalars directly, and supports --json for structured values. config unset removes user-set YAML paths, supports env-backed keys written by config set, and clears mirrored terminal env values such as TERMINAL_ENV when unsetting terminal.backend.

Freshness / duplicate screening before opening:

Fixes #36037

Related Issue

Fixes #36037

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Add parser and command handling for hermes config get <key> [--json].
  • Add parser and command handling for hermes config unset <key>.
  • Share terminal config-to-env sync mapping between set/unset.
  • Add tests for resolved values, structured JSON, null values, missing env keys, dotted _token YAML paths, env-backed unsets, and terminal env cleanup.
  • Update README and config docs to mention get / unset.

How to Test

  1. uv run --extra dev pytest tests/hermes_cli/test_set_config_value.py tests/hermes_cli/test_placeholder_usage.py tests/tools/test_terminal_config_env_sync.py -q -> 53 passed.
  2. uv run --extra dev python -m ruff check hermes_cli/config.py hermes_cli/main.py tests/hermes_cli/test_set_config_value.py tests/tools/test_terminal_config_env_sync.py -> All checks passed.
  3. git diff --check -> no output.
  4. Temp HERMES_HOME CLI smoke: config set terminal.backend docker, config get terminal.backend, config get terminal --json, config unset terminal.backend, config get terminal.backend -> printed docker, structured JSON, unset confirmation, then resolved default local.

Codex review-mode wrapper:

  • /tmp/codex-review.HRjau6.txt found null-value handling bug; fixed.
  • /tmp/codex-review.KSlomm.txt found missing env-key handling bug; fixed.
  • /tmp/codex-review.0F3nhj.txt found dotted _token YAML-path classification bug; fixed.
  • /tmp/codex-review.WKCYbL.txt ended clean: no actionable correctness issues.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this feature
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: Linux worktree via focused pytest and CLI smoke

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — path handling remains pure config parsing
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles labels May 31, 2026

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — automated review passed. No security, quality, or test coverage issues detected.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for implementing the missing scriptable config operations. The feature remains needed on current main, but this branch predates several config and parser changes.

Problems

  • The active parser now lives in hermes_cli/subcommands/config.py:12-49 and is wired from hermes_cli/main.py:13161; the PR edits the extracted historical block instead (hermes_cli/main.py:12782-12796).
  • unset_config_value() catches config read failures as {} at hermes_cli/config.py:5850-5854 and can rewrite the file at line 5868. Current main deliberately fail-closes config replacements through atomic_config_write() at hermes_cli/config.py:6720-6743.
  • The new _CONFIG_TO_ENV_SYNC at hermes_cli/config.py:3330-3352 duplicates an incomplete subset of current main's TERMINAL_CONFIG_ENV_MAP / terminal_config_env_var_for_key() (hermes_cli/config.py:6814-6857).
  • Unset needs the managed-scope guard that current set_config_value() applies at hermes_cli/config.py:8110-8120.

Suggested changes

  • Port parser registration to hermes_cli/subcommands/config.py and test parser acceptance.
  • Reuse current managed-scope, atomic-write, and terminal-env mapping helpers; add regression coverage for unreadable config and a newer terminal mapping.

Automated hermes-sweeper review.

Comment thread hermes_cli/main.py
# config edit
config_subparsers.add_parser("edit", help="Open config file in editor")

# config get

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main no longer builds config subcommands here: build_config_parser() in hermes_cli/subcommands/config.py is invoked from hermes_cli/main.py:13161. Port these argparse additions to that builder so the commands are reachable after salvage.

Comment thread hermes_cli/config.py

_MISSING = object()

_CONFIG_TO_ENV_SYNC = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do not introduce another terminal mapping here. Current main has the authoritative TERMINAL_CONFIG_ENV_MAP and terminal_config_env_var_for_key() helper; this subset misses newer mappings such as docker_network, so unset would leave their mirrored .env values stale.

Comment thread hermes_cli/config.py
try:
with open(config_path, encoding="utf-8") as f:
user_config = yaml.safe_load(f) or {}
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This must fail closed rather than treating an unreadable or malformed existing config as {}. Current main's atomic_config_write() protects this exact overwrite class; use that guard before any rewrite.

Comment thread hermes_cli/config.py

def unset_config_value(key: str):
"""Remove a user-set configuration or .env value."""
if is_managed():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please also apply the per-key managed-scope guard used by current set_config_value(). is_managed() covers the package-manager lock, but does not protect administrator-pinned individual config keys.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #65540 (rebase-merged onto current main, head commit 53adb3f) — your commit was re-applied onto the refactored subcommands/config.py parser with your authorship preserved; env-mirror sync now routes through the canonical TERMINAL_CONFIG_ENV_MAP instead of a duplicate dict. Thanks for the contribution!

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: hermes config get <key> and config unset <key> — complete the config CRUD surface

4 participants