fix: clean up profile clone manifests and slash-command logging - #48290
fix: clean up profile clone manifests and slash-command logging#48290aniruddhaadak80 wants to merge 1 commit into
Conversation
|
Related: #48245 and #48288 both implement the slash-command logging portion of this PR (the |
|
Resolved the slash-command logging overlap by removing that portion from this PR (and its associated tests) to isolate the profile-clone and desktop fixes. Pushed the updates. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the clone-manifest and provider-title work after the logging overlap was removed.
Problems
tests/e2e/test_platform_commands.py:14addslogging, but the current PR diff contains no use after the logging test was removed.hermes_cli/profiles.py:199writes.bundled_manifestdirectly. The existing writer is deliberately atomic attools/skills_sync.py:150-180; retain that crash-safety for clone cleanup.- Current main requires
symlinks=Truefor the skills clone athermes_cli/profiles.py:1099(commitb7192b1cb). The older PR copytree call would drop that protection.
Suggested changes
- Remove the unused import, preserve symlink handling, and port the title/test to the extracted onboarding files (
apps/desktop/src/components/onboarding/providers.tsxandindex.test.tsx).
Automated hermes-sweeper review.
| @@ -11,6 +11,7 @@ | |||
| """ | |||
|
|
|||
| import asyncio | |||
| import logging | |||
There was a problem hiding this comment.
This import is now unused: the slash-command logging test was removed in the follow-up commit, and no remaining code in this file references logging. Please remove it.
|
|
||
| pruned = {name: hash_value for name, hash_value in manifest.items() if name in live_names} | ||
| if pruned == manifest: | ||
| return |
There was a problem hiding this comment.
Please keep manifest rewrites atomic. tools/skills_sync.py:150-180 uses a temp file and atomic replacement specifically to avoid leaving .bundled_manifest truncated if the process is interrupted; this direct write reintroduces that failure mode.
| @@ -889,7 +955,9 @@ def create_profile( | |||
| # same agent capabilities as the source profile. | |||
| source_skills = source_dir / "skills" | |||
| if source_skills.is_dir(): | |||
| shutil.copytree(source_skills, profile_dir / "skills", dirs_exist_ok=True) | |||
| dest_skills = profile_dir / "skills" | |||
There was a problem hiding this comment.
When this is salvaged onto current main, retain symlinks=True on this copytree call. Commit b7192b1 added it to prevent recursive cloning through parent-pointing symlinks.
58fe125 to
367532a
Compare
…e title
profile create --clone / --clone-all copy the source profile's whole
skills tree including its .bundled_manifest. Bundled skills are re-seeded
per profile only while missing, so manifest entries whose skill no longer
exists keep reporting phantom bundled provenance forever and block the
skill's absence from being treated as a deliberate deletion. Prune those
entries right after the copy using the shared skills-sync discovery rules
(frontmatter names, excluded-path filtering), writing the pruned manifest
through the shared atomic writer so mode bits survive.
Also give the claude-code OAuth catalog entry a readable display name in
the dashboard and desktop onboarding ("Anthropic OAuth (Claude Code)")
instead of leaking the internal eligibility note into the UI, and make
two .env permission assertions Windows-tolerant (no POSIX bits there).
Fixes feedback on NousResearch#48290.
367532a to
23d5655
Compare
|
All four points are addressed in the current head:
Verification: 9/9 clone/manifest/prune tests pass on Windows. The 4 remaining test_profiles.py failures on this host (symlink WinError 1314 privilege, POSIX wrapper-script naming, custom-alias .bat suffix) reproduce identically on a clean origin/main worktree - pre-existing Windows-environmental, not introduced here. |
…e title
profile create --clone / --clone-all copy the source profile's whole
skills tree including its .bundled_manifest. Bundled skills are re-seeded
per profile only while missing, so manifest entries whose skill no longer
exists keep reporting phantom bundled provenance forever and block the
skill's absence from being treated as a deliberate deletion. Prune those
entries right after the copy using the shared skills-sync discovery rules
(frontmatter names, excluded-path filtering), writing the pruned manifest
through the shared atomic writer so mode bits survive.
Also give the claude-code OAuth catalog entry a readable display name in
the dashboard and desktop onboarding ("Anthropic OAuth (Claude Code)")
instead of leaking the internal eligibility note into the UI, and make
two .env permission assertions Windows-tolerant (no POSIX bits there).
Fixes feedback on NousResearch#48290.
ec0fdd6 to
232888a
Compare
What does this PR do?
Two fixes to
hermes profile createcloning, rebuilt fresh off currentmain:--clone-all, skills are copied out of the repo's bundledskills/tree into<profile>/skills/. The copied.bundled_manifest.jsonstill listed every originally-bundled skill name, including ones stripped by_CLONE_ALL_STRIP(or deleted later), soskills list/sync treated user-visible state as if those skills were still bundled � they could never be "restored" and polluted listings. We now rewrite the manifest post-clone to match what actually landed on disk (frontmattername:wins over directory name;node_modules/excluded), preserving the v1 plain-name format so older readers stay compatible. Applies to both clone paths (--clone-alland--clone-config).Related Issue
N/A � reviewer-requested rebuild of #48290 itself.
Type of Change
Changes Made
hermes_cli/profiles.py: new_read_cloned_bundled_manifest()/_prune_cloned_bundled_manifest()helpers; wired into both clone paths; manifest written atomically viautils.atomic_write_text(tmp_prefix=".bundled_manifest_", mode preserved); reusestools.skills_sync._discover_bundled_skillsvia lazy import.hermes_cli/web_server.py+apps/desktop/src/components/onboarding/providers.tsx: claude-code title ? "Anthropic OAuth (Claude Code)".tests/hermes_cli/test_profiles.py: unit tests for the prune helper (frontmatter-name matching, node_modules exclusion, v1-format preservation) + integration tests through both clone paths; two pre-existing 0o600 assertions made Windows-tolerant (file-permission checks that only hold on POSIX).How to Test
pytest tests/hermes_cli/test_profiles.py -qhermes profile create t --clone-allin a checkout whose bundled skills include stripped entries, then inspect<profile>/skills/.bundled_manifest.json� it lists exactly the skills present in<profile>/skills/.Checklist
Code
pytest tests/ -qand all tests pass (59 passed locally; 4 unrelated Windows-local failures reproduce on cleanmain)Documentation & Housekeeping
cli-config.yaml.example� or N/ACONTRIBUTING.md/AGENTS.md� or N/AScreenshots / Logs
N/A.