fix(profile): seed bundled skills for fresh WebUI-created profiles (#2305)

[WanderWang]

Problem

When creating a new profile via the Web UI without checking "Clone from active profile", the resulting profile has 0 skills (empty skills/ directory). This is inconsistent with the CLI behavior where hermes profile create foo automatically seeds bundled skills.

Root Cause

api/profiles.py::create_profile_api() never called seed_profile_skills(). The CLI entrypoint (hermes_cli/main.py) and the web server route (hermes_cli/web_server.py) both call it, but the WebUI's create_profile_api() bypassed this.

Fix

• Add seed_profile_skills() call in create_profile_api() when clone_from is None (fresh profile).
• Skip seeding for cloned profiles since create_profile() already copies skills from the source.
• Wrap in try/except so a seed failure is non-fatal — an empty skills/ directory is recoverable, a missing profile is not.
• Handle ImportError for Docker/standalone fallback paths where hermes_cli is unavailable.

Tests

Added tests/test_issue2305_seed_profile_skills.py with 6 tests:

1. Fresh profile (clone_from=None) calls seed_profile_skills
2. Cloned profile does NOT call seed_profile_skills
3. Seed failure is non-fatal
   4-6. Static analysis verifying the seed call exists, is conditional on clone_from is None, and has proper exception handling

Verification

python -m pytest tests/test_issue2305_seed_profile_skills.py -q

All 6 tests pass.

Refs #2305, #749

[nesquena-hermes]

Summary

Reading the diff against origin/master:api/profiles.py:1100-1160, the seed-skills slice itself is correct and matches the fix shape the maintainer endorsed in #2305 (clone_from gate, try/except, ImportError vs generic Exception split, quiet=True). One concern with the test file and one minor note on placement, both addressable.

Code reference

api/profiles.py:1070-1086 (PR head), placed after _write_model_defaults_to_config:

if clone_from is None:
    try:
        from hermes_cli.profiles import seed_profile_skills
        seed_profile_skills(profile_path, quiet=True)
    except ImportError:
        logger.debug("hermes_cli unavailable; skipping skill seed for %s", name)
    except Exception as e:
        logger.warning("seed_profile_skills failed for new profile %s: %s", name, e)

This is functionally fine — seed_profile_skills operates on profile_path directly and is independent of endpoint/model config order, so placing it after _write_model_defaults_to_config is OK. The maintainer's sketched fix in #2305 also placed it after the create call but didn't pin a specific line, so either placement is defensible. Compared to PR #2315 (the parallel competing fix) which puts it right after profile_path.mkdir, the trade-off is "seed runs even if endpoint/model writes fail" (current PR) vs "endpoint/model writes succeed even if seed fails" (#2315). Both are defensible; both seed and endpoint writes are non-fatal, so neither order materially differs in user-observable behaviour.

Concern — test isolation

tests/test_issue2305_seed_profile_skills.py:54-56:

monkeypatch.delitem(sys.modules, "api.profiles", raising=False)
import importlib
profiles_mod = importlib.import_module("api.profiles")

Deleting api.profiles from sys.modules and re-importing it inside a test is fragile. api/profiles.py defines module-level state (the _root_profile_name_cache, the _DEFAULT_HERMES_HOME Path, logger reference), and any other test in the same session that imports api.profiles will keep a reference to the original module object. The reimported profiles_mod is a fresh module with its own state; other tests that touched the old object now see stale state.

The static-source tests in TestStaticAssertions later in the same file (test_create_profile_api_contains_seed_call, test_seed_is_conditional_on_clone_from_none, test_seed_failure_wrapped_in_try_except) read api/profiles.py from disk, so they aren't affected. But mixing this test file into a suite-wide run alongside other api.profiles-touching tests is a recipe for order-dependent flakes.

Suggested fix: rather than reimporting, use monkeypatch.setattr(api.profiles, '_some_internal', ...) or inject the mock module before the first import — sys.modules['hermes_cli.profiles'] = fake_module and then call profiles.create_profile_api(...) directly. The from hermes_cli.profiles import seed_profile_skills inside create_profile_api is evaluated at call time, not import time, so the mock will be resolved without needing to reload api.profiles. PR #2315's tests/test_issue2305_profile_create_seeds_skills.py:67-77 does exactly this and avoids the reimport entirely.

Minor

• No CHANGELOG.md entry. AGENTS.md:Contribution-style asks for one on user-visible behavior changes; "WebUI-created profiles now match CLI skill seeding" qualifies.
• The three TestStaticAssertions source-string tests are weak — they verify the string "seed_profile_skills" exists in api/profiles.py, which can't catch behaviour regressions like "the call moved inside the wrong branch". The three behavioural tests above already cover everything those source-string tests cover. Consider dropping them.

Verdict

Code change is correct. Test file works for the explicit cases but the module reload is a footgun that should be removed before merge. CHANGELOG entry would be welcome. Note that PR #2315 is a parallel implementation of the same fix from another contributor; the maintainer will need to pick one.

[nesquena-hermes]

Thanks for the contribution — really appreciate the attention to detail on the test coverage and the careful import-error handling.

We received two PRs for #2305 on essentially the same timeline:

• This PR (fix(profile): seed bundled skills for fresh WebUI-created profiles (#2305) #2314) by @WanderWang
• Seed bundled skills for WebUI profile creation #2315 by @Michaelyklam

After comparing the two implementations they're nearly identical in shape — both call seed_profile_skills from hermes_cli.profiles after profile_path.mkdir, both skip seeding when clone_from is set, both wrap in try/except with warnings on failure. We're going with #2315 in this release for three small reasons:

1. CHANGELOG entry included — Seed bundled skills for WebUI profile creation #2315 adds the user-facing changelog line
2. Cleaner insertion order — Seed bundled skills for WebUI profile creation #2315 places the seed call immediately after profile_path.mkdir() and BEFORE the config writes, which feels more natural (seed first, configure second)
3. More thorough test coverage — Seed bundled skills for WebUI profile creation #2315's tests use a module-level sys.modules injection pattern that's slightly more robust than the import patching approach here

Both implementations were genuinely solid — this came down to small tiebreaker details. Closing this PR as superseded by #2315, but the contribution is appreciated and the analysis was on point.

If you have follow-up ideas around profile creation / skill seeding (e.g. interactive prompts in the WebUI to choose which bundled skills get seeded, or making the bundled-skills list configurable), those would be welcome new PRs. Thanks again.

— maintainer

[WanderWang]

@nesquena-hermes Thanks for the detailed review — all three points addressed in the latest push (e032d3b):

1. Module reload removed. The tests now inject hermes_cli.profiles into sys.modules before calling create_profile_api(), and use the already-imported profiles module directly. No more del sys.modules['api.profiles'] or importlib.reload — the mock is resolved at call time when from hermes_cli.profiles import seed_profile_skills executes inside create_profile_api.

2. Static source-string tests dropped. TestProfileCreateSkillSeedingStatic (3 tests) is gone — you're right that they couldn't catch behavioural regressions like a misplaced branch, and the 3 behavioural tests already cover the surface.

3. CHANGELOG entry added. Added under [Unreleased] → Fixed, following the existing style with pre-fix/post-fix description and regression-test note.

Let me know if anything else needs adjustment!