fix(profiles): reserve "main" + enforce _RESERVED_NAMES in create_profile#17880
Open
cola-runner wants to merge 1 commit into
Open
fix(profiles): reserve "main" + enforce _RESERVED_NAMES in create_profile#17880cola-runner wants to merge 1 commit into
cola-runner wants to merge 1 commit into
Conversation
…file Fixes NousResearch#17879. `hermes profile create main` succeeded today and produced session keys with the `agent:main:*` prefix — the exact prefix the default profile uses — re-introducing the cross-profile collision NousResearch#12099 (and the in-flight NousResearch#12266 fix) is meant to eliminate. The same hole let `root`, `hermes`, `test`, `tmp`, and `sudo` through too: `_RESERVED_NAMES` was defined but only consumed by `check_alias_collision()` (a wrapper-script-creation check), not by `validate_profile_name()` or `create_profile()`. Fix: - Add "main" to `_RESERVED_NAMES`. - Replace the one-name `if name == "default"` hardcode in `create_profile()` with a uniform `if name in _RESERVED_NAMES` check (the new error message still names the offending value and lists all reserved names). `validate_profile_name()` is intentionally left permissive so existing callers (`rename_profile`, `delete_profile`, `set_active_profile`, `export_profile`, `import_profile`, `resolve_profile_env`) keep working for any legacy `"main"` profile that may already exist on disk — only *creation* of a new reserved-name profile is restricted. Orthogonal to NousResearch#12266: even after that PR lands, a user who runs `hermes profile create main` still hits the original collision because `_resolve_session_key_prefix("main")` evaluates to `"agent:main"`. This PR closes that gap permanently. Tests: `tests/hermes_cli/test_profiles.py::TestReservedNames` parametrises all seven reserved names and verifies (a) each is rejected, (b) the error message lists the offending name plus the full reserved set, (c) no orphan directory is left behind. All 102 existing profile tests still pass.
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
2 participants
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.
Fixes #17879.
TL;DR
hermes profile create mainsucceeded and produced session keys with theagent:main:*prefix — the same prefix the default profile uses — silently re-creating the cross-profile collision that #12099 / the in-flight #12266 fix exists to eliminate. Same hole letroot,hermes,test,tmp,sudothrough._RESERVED_NAMESwas defined inhermes_cli/profiles.pybut only consulted bycheck_alias_collision()(wrapper-script creation), not by profile creation itself.This PR adds
"main"to_RESERVED_NAMESand makescreate_profile()reject any reserved name with a uniform message.Why this is independent of #12266
I noticed this while reviewing #12266 (which is excellent and which I think should land — see #12102 close note). #12266 fixes the runtime behaviour of session-key construction so two profiles with different names don't collide. This PR fixes the input-validation layer so a user can't ask the runtime to construct a "main"-named profile in the first place. The two fixes are orthogonal and can land in either order:
hermes profile create main→_resolve_session_key_prefix("main")→"agent:main"→ collision with default still happens.Diff
Net 5 lines of production change.
Behavior matrix
hermes profile create coderhermes profile create defaulthermes profile create mainhermes profile create roothermes profile create hermes/test/tmp/sudorename_profile,set_active_profile, etc. on a legacy"main"profileScope — explicitly NOT changed
validate_profile_name()stays permissive. It's called by 8 non-creation paths (rename,delete,set_active,export,import,resolve_env, …). Tightening it would break any user who somehow already has a legacy"main"profile on disk — they should still be able to rename or delete it. Only creating a new reserved-name profile is what we reject._resolve_session_key_prefixorbuild_session_key. That layer is fix(session): scope build_session_key by active profile, preserve default (#12099) #12266's scope and this PR doesn't touch it."main"profiles. Out of scope; a follow-up could detect and warn at startup, but the realistic exposure is near-zero (you'd have had to deliberately typehermes profile create main, which was undocumented).Tests
tests/hermes_cli/test_profiles.py::TestReservedNamesadds 9 cases:main,hermes,default,test,tmp,root,sudo) — each must raiseValueErrorwithmatch="reserved".hermes,default,mainso the user can correct without reading source._RESERVED_NAMES, this test fails and forces an intentional update.All 102 existing
tests/hermes_cli/test_profiles.pycases still pass on this branch (scripts/run_tests.sh tests/hermes_cli/test_profiles.py). The legacytest_default_raises_value_errorcontinues to pass because the new error message still contains the literal string"default".Pre-empted review questions
Q. Why is the new error message verbose?
The reserved set is small (7 names) and listing them is a lot cheaper than making the user grep
profiles.py. If you'd rather a terser message, happy to drop the list.Q. Why parametrise
_RESERVED_NAMESin tests instead of hardcoding["main"]?The bug is that the frozenset existed but wasn't enforced. Parametrising every name pins all seven so a future contributor who adds a new reserved name automatically gets a passing test, and one who removes the enforcement gets a failing one.
Q. Should
"default"be removed from_RESERVED_NAMESsincevalidate_profile_namealready special-cases it?No — keeping
"default"in_RESERVED_NAMESmakes the creation-side rejection uniform (one code path, one message). Thevalidate_profile_namespecial case is for the non-creation paths (rename, delete, etc.) where"default"is a valid alias for the built-in profile.