6edd2c4be994ea378b9f3045ede7ec0cb525aef7
13 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Teknium
|
cfa327e5dc | refactor(hclib): remaining hermes_cli library modules — dead code, unified helpers, flattened branches | ||
|
kshitij
|
3556728a54 |
refactor(utils): move mode+owner preservation into atomic_write_text
Follow-up to the salvaged #79323 commits. The three hand-rolled stat -> atomic_write_text -> chmod blocks (xai migration, uninstaller shell-rc rewrite, dashboard SOUL.md editor) collapse into an opt-in preserve_mode=True kwarg on utils.atomic_write_text, plus create_mode= on both atomic_write_text and atomic_yaml_write for first-create paths (SOUL.md first save, write_manifest's allowlist create path). Beyond deduplication this closes two gaps the hand-rolled copies had: - Owner preservation: the old in-place writes kept the inode, so file ownership survived root-run rewrites for free. atomic_write_text swaps in a new inode owned by the writing user, and the hand-rolled blocks restored only the mode -- a root-run 'hermes migrate xai' or sudo uninstall on a user-owned Docker/NAS volume would flip config.yaml / ~/.zshrc ownership to root. preserve_mode now routes through the same _preserve_file_owner/_restore_file_owner helpers atomic_yaml_write and atomic_json_write already use. - chmod-after-replace window: the mode is applied to the temp fd via fchmod BEFORE the replace (mirroring atomic_json_write's mode= param), so the target never transits through mkstemp's 0600. Also removes write_manifest's caller-side existed/chmod block (and its small TOCTOU) in favor of atomic_yaml_write(create_mode=0o644), and corrects the SOUL.md mode comment (the default profile's runtime seeder does run it through _secure_file; named profiles do not). preserve_mode defaults to False so the existing callers (memory store, skill manager, cron, agent importer) keep their current semantics. New tests in tests/test_atomic_write_text_metadata.py cover mode preservation, owner restore through symlinks, fchmod-before-replace, create_mode on both writers, and no-behavior-change without opt-in; all mutation-checked. |
||
|
briandevans
|
4541d30181 |
fix(cli): keep newly created SOUL.md and distribution.yaml at 0644
Both files were routed through the shared atomic writers earlier in this branch. tempfile.mkstemp creates the temp file 0600 and the atomic swap carries that mode onto the target, so the *create* paths silently tightened two files that previously landed at the umask default: - web_routers/profiles.py: the dashboard persona editor's first-ever Save has no prior SOUL.md to copy permissions from, so the existing guard skipped the chmod entirely -- contradicting the comment directly below it, which states profile SOUL.md is created 0644 and is not run through _secure_file. - profile_distribution.py: atomic_yaml_write only restores a mode it captured from a file that already existed. _materialize() calls write_manifest() with no manifest on disk whenever a distribution declares an explicit distribution_owned allowlist that omits distribution.yaml, so the staged copy is never placed in the profile. Both are fixed with a local chmod at the two sites this branch regressed; utils.py's public mode semantics are left alone. profiles.py now also distinguishes "no file yet" (FileNotFoundError -> 0644) from "stat failed for some other reason" (-> leave the mode alone rather than guess at it). uninstall.py and xai_retirement.py have no create path and are unchanged: the former captures prior_mode unconditionally after a successful read_text(), and the latter runs require_readable_config_before_write() first. |
||
|
briandevans
|
67827dd99e |
fix(cli): route the remaining destructive user-file rewrites through atomic writes
`utils.atomic_write_text`'s docstring states the invariant: it exists "so that
every destructive file rewrite in the codebase shares one implementation."
Four full-file rewrites of *existing user-authored files* still bypass it and
use a bare truncating `open(path, "w")` / `Path.write_text()`, which truncates
the target before the new content is produced. A crash, SIGINT, or ENOSPC
mid-write therefore leaves the file empty or half-written.
In all four cases the read half degrades silently to a default rather than
erroring, so the damage is invisible and the next write cements it:
* `xai_retirement.apply_migration()` rewrites the user's config.yaml. Merged
commit
|
||
|
Teknium
|
ee5a66ae3e |
fix(distribution): path-aware allowlist; preserve legacy copy-everything when omitted
Follow-ups to the previous commit (#74414 by @webtecnica, re #74373): - When distribution_owned is OMITTED, restore the legacy contract: every staged entry outside USER_OWNED_EXCLUDE is copied. The cherry-picked filter consulted owned_paths(), which silently narrowed omitted-list distributions to DEFAULT_DIST_OWNED and dropped undeclared payload (extra top-level files/dirs existing distributions legitimately ship). - Make explicit allowlists path-aware so documented nested entries like skills/research/ and cron/digest.json select exactly that subtree/file instead of being dropped by the top-level name comparison. Traversal segments (.., absolute) and USER_OWNED_EXCLUDE roots are still rejected. - Regression tests: omitted-list legacy behavior + nested-path allowlist. |
||
|
webtecnica
|
a42e3e8ba4 |
fix(distribution): respect distribution_owned allowlist in _copy_dist_payload
_copy_dist_payload() in profile_distribution.py iterated all staged entries without consulting the manifest's distribution_owned allowlist, so manifests that restricted distribution_owned only had cosmetic effect. Fix: compute manifest.owned_paths() at the top of _copy_dist_payload() and skip entries not in that set, after the USER_OWNED_EXCLUDE check. The owned_paths() method already existed on DistributionManifest and correctly falls back to DEFAULT_DIST_OWNED when no explicit distribution_owned is set, so the new filter preserves backward compatibility for existing manifests. Closes #74373 |
||
|
Teknium
|
58708c7066 |
fix(git): never block internal git calls on credential prompts
Port from openai/codex#34540 / #34612 ("detach non-interactive subprocesses from stdin"): internal git invocations that run with nobody attached — MCP catalog installs, plugin install/update, profile distribution staging, worktree base fetches, and the desktop review pane's git/gh backend — could hang on a credential prompt when a remote is private, misconfigured, or requires auth. git prompts on the inherited terminal (or via Git Credential Manager on Windows), so the operation silently waits until its timeout, or forever at sites without one (mcp_catalog clones have no timeout at all and inherit the parent terminal). - Add noninteractive_git_env() to hermes_cli/_subprocess_compat.py: GIT_TERMINAL_PROMPT=0 + GCM_INTERACTIVE=Never on a copy of the environment; GIT_ASKPASS/SSH_ASKPASS deliberately preserved so working non-interactive auth still succeeds. - Wire it + stdin=DEVNULL into: mcp_catalog._do_git_install (clone/ checkout), plugins_cmd (clone + pull), profile_distribution._git_clone, web_git._git/_gh (gh also gets GH_PROMPT_DISABLED=1), and cli.py's worktree base fetch helper. - Tests: env contract, a real-git E2E against a local 401 Basic-auth HTTP server proving fail-fast ("terminal prompts disabled") instead of a hang, and per-call-site plumbing assertions. Sabotage-verified: removing the env from web_git._git fails the site test. |
||
|
Teknium
|
d3d621f7c3 |
revert(windows): roll back terminal-popup PRs #53791 #53810 #53829 (#53853)
* Revert "fix(windows): capture is not a no-window boundary; route flashing spawns through chokepoint (#53829)" This reverts commit |
||
|
Teknium
|
2ecca1e7d3 |
fix(windows): capture is not a no-window boundary; route flashing spawns through chokepoint (#53829)
Follow-up to #53791 addressing review feedback: the footgun checker treated capture_output=/stdout=/stderr=/check_output as proof a subprocess can't pop a Windows console. That invariant is false — stream redirection controls where a child's output goes, not whether a console is allocated. From a console-less parent (Desktop/Electron, pythonw.exe, detached gateway/cron) a console-subsystem child still flashes a window even when fully captured. - check-windows-footguns.py: capture/redirect/check_output is no longer a blanket safe-pass. Added _WINDOWS_FLASHING_PROGRAMS (git/gh/npm/node/python/uv/ffmpeg/ docker/powershell/…); calls to those are flagged even when captured. Non-flashing programs keep the capture exemption (no 271-site noise). _subprocess_compat.run/ popen calls are inherently safe (wrapper injects CREATE_NO_WINDOW). - Routed the 35 genuine flashing git/gh/npm/uv/ffmpeg/docker spawns through the _subprocess_compat.run/popen chokepoint (Brooklyn's wrapper from #53810) — the durable fix, not per-site annotations. cmd.exe /c start stays # ok (intentional). - Updated tests + CONTRIBUTING.md rule #17 to the corrected invariant. |
||
|
islam666
|
e53b74c394 |
fix(dist): stop USER_OWNED_EXCLUDE from filtering nested directories
The copytree ignore lambda in _copy_dist_payload applied USER_OWNED_EXCLUDE recursively at every directory depth. This caused nested directories whose names matched exclude entries (bin, logs, cache, etc.) to be silently dropped during distribution install/update. Fix: only apply USER_OWNED_EXCLUDE filtering at the root of the staged tree, matching the two-tier pattern used by _clone_all_copytree_ignore and _default_export_ignore in profiles.py. Add 5 tests covering nested bin/logs/cache preservation and top-level filtering still working. Fixes #37954 |
||
|
nguyen binh
|
46d8b5dadf | fix(profile): reject symlinks in distributions (#25292) | ||
|
Teknium
|
3fde8c153d |
fix(skills): prune dependency/venv dirs from all skill scanners (#30042)
* fix(skills): skip dependency dirs in skill scan * fix(skills): widen sibling rglob scanners to use shared exclusion set Follow-up to PR #29968. The contributor's PR widened EXCLUDED_SKILL_DIRS in the canonical walker (iter_skill_index_files), which fixes the user-visible discovery path. This commit sweeps the ~12 other rglob('SKILL.md') sites that did their own ad-hoc filtering — most only checked .git/.hub, some had no filter at all — so dependency dirs (.venv, node_modules, site-packages, etc.) cannot leak ghost skills through the secondary paths. Adds agent.skill_utils.is_excluded_skill_path(path) helper. Migrates all 13 sites to use it. Removes 3 hardcoded duplicate filter sets. Sites touched: agent/curator_backup.py - skill backup file count gateway/run.py - disabled-skill response (2 sites) hermes_cli/dump.py - skill count in env dump hermes_cli/profile_describer.py- profile description (2 sites) hermes_cli/profile_distribution.py - profile install count hermes_cli/profiles.py - profile skill count hermes_cli/skills_hub.py - category detection tools/skill_manager_tool.py - skill name lookup (already used set, now uses helper) tools/skill_usage.py - usage tracking + skill dir lookup (2 sites) tools/skills_hub.py - optional skills find + scan (2 sites) tools/skills_sync.py - bundled skills sync E2E verified with the exact reported shape (bring/scripts/.venv/.../typer/.agents/skills/typer/SKILL.md): no sibling site picks up the ghost skill, all five legit-skill counts still return 1. * chore(infographic): retro-pop-grid bento for PR #30042 skill-scanner sweep --------- Co-authored-by: helix4u <4317663+helix4u@users.noreply.github.com> |
||
|
Teknium
|
f209a35859 |
feat(profile): shareable profile distributions via git (#20831)
* feat(profile): shareable profile distributions (pack/install/update/info) Closes #20456. Turns a profile into a portable, versioned artifact. Packs SOUL.md, config, skills, cron, and an env-var manifest into a tar.gz that others can install from a local path, URL, or git repo. Updates re-pull the distribution while preserving user data (memories, sessions, auth.json, .env) and the user's config.yaml overrides. New subcommands (under hermes profile, no parallel tree): hermes profile pack <name> [-o FILE] hermes profile install <source> [--name N] [--alias] [--force] [-y] hermes profile update <name> [--force-config] [-y] hermes profile info <name> Manifest (distribution.yaml at the profile root): name, version, hermes_requires, author, env_requires, distribution_owned. Security: - Installer shows manifest + env-var requirements before mutating disk; confirmation required unless -y. - auth.json and .env are never packed (same exclude set as profile export). - Cron jobs are packed but NOT auto-scheduled — user is pointed at 'hermes -p <name> cron list' to review. - Archive extraction rejects path traversal (../ members). - Alias creation is opt-in via --alias. Update semantics: - Distribution-owned paths (SOUL.md, skills/, cron/, mcp.json, manifest): replaced from the new archive. - config.yaml: preserved by default; --force-config to overwrite. - User-owned paths (memories/, sessions/, auth.json, .env, state.db*, logs/, workspace/, plans/, home/, *_cache/, local/): never touched. Version pin: hermes_requires accepts >=, <=, ==, !=, >, < or a bare version (treated as >=). Install fails with a clear error when the running Hermes version doesn't satisfy the spec. Sources supported by 'install': - Local .tar.gz / .tgz archive - Local directory - HTTP(S) URL pointing to a .tar.gz (uses httpx, already a dep) - Git URL (github.com/user/repo, https://..., git@..., ssh://, git://) Tests: 43 new unit tests (manifest parsing, version checks, env template, pack/install/update round-trip, config-preservation, security). E2E validated via real CLI invocations against an isolated HERMES_HOME covering pack, install with confirmation, update preservation, update --force-config, decline-preview, duplicate-install rejection, and version-requirement rejection. * refactor(profile-dist): git-only — drop tar.gz/HTTP transports and pack Scope-cut on top of the original distribution PR: a profile distribution is now exclusively a git repository (or a local directory during development). The tar.gz / HTTP archive transports and the matching `hermes profile pack` subcommand have been removed. Why: * GitHub tags, branches, and commits are already the right versioning primitive. Tag pushes do for us what 'pack + upload' did. * `hermes profile export` / `import` already cover local backup and restore; they are not a distribution format and stay untouched. * One transport means one install/update code path, one doc page, and one mental model. The extra source types doubled the surface for no real user win — GitHub auto-attaches release tarballs, and `git bundle` / `git clone --mirror` cover the airgap case. Changes: * hermes_cli/profile_distribution.py — removed pack_profile, _fetch_tar_archive (_http_fetch), _safe_extract, _archive_roots, _safe_parts, _find_dist_root, tarfile/io/urlparse imports. The new _stage_source has two arms: git URL → clone, local directory → use in place. * hermes_cli/main.py — removed the 'pack' subparser and action handler. Install help text updated to match the reduced source list. * tests/hermes_cli/test_profile_distribution.py — rewritten around a local-directory staging fixture. The install/update/describe suites now build a distribution tree on disk directly and install from it, which is what a real git clone produces after .git is stripped. Dropped TestPack, TestFindDistRoot, and the tar-specific security test. New tests cover _looks_like_git_url, env_example emission, hermes_requires enforcement, and 'installer does not import credentials if an author mistakenly leaks them in the staging tree'. * website/docs/reference/profile-commands.md — 'Distribution commands' section rewritten around git. Added a 'Publishing a distribution' section. export/import stay documented as local backup/restore. * website/docs/reference/cli-commands.md — dropped 'pack' from the profile subcommand table. * website/package.json — 'lint:diagrams' now passes --exclude-code-blocks to ascii-guard. Without it, markdown tables and box-drawing diagrams inside fenced code blocks were being misidentified as malformed ASCII boxes, blocking the PR's docs-site-checks CI with 8 false-positive errors. Validation: * Targeted suite: tests/hermes_cli/test_profile_distribution.py — 56/56 pass (down from 43 — reorganized to cover the new local-dir paths). * Regression: test_profiles.py + test_profile_export_credentials.py 102/102 still pass. export/import behaviour unchanged. * Docs lint: ascii-guard lint --exclude-code-blocks docs returns 0 errors (was 8 on the PR before the flag bump). * E2E: ran the real `hermes profile install`/`info` against a local staging dir under an isolated HERMES_HOME — install writes SOUL.md + skills to the target profile, info reads the manifest back, a bogus source produces a clear error, and `hermes profile pack` is now rejected by argparse as expected. * feat(profile-dist): distribution-aware list/show/delete + installed_at + env preview Polish pass on top of the git-only scope cut. Five additions, all small, wiring into existing commands rather than adding new surface. 1. `installed_at` timestamp on the manifest * Stamped automatically inside plan_install() on both fresh install and update — ISO-8601 UTC, seconds resolution. * Surfaced in `hermes profile info` as `Installed: <ts>`. * Lets users tell "installed 6 months ago, needs update" from "installed yesterday" without guessing from file mtimes. 2. `hermes profile list` grows a `Distribution` column * Plain profiles: "—" * Distribution profiles: "<name>@<version>" (e.g. `telemetry@1.2.3`) * ProfileInfo gains three optional fields — distribution_name, distribution_version, distribution_source — populated by a new _read_distribution_meta() helper that swallows manifest read errors so a broken distribution.yaml in one profile can't break `list` for the others. 3. `hermes profile show` and `hermes profile delete` surface distribution provenance * show: `Distribution: name@version` + `Installed from: <source>` plus a pointer to `hermes profile info <name>` for the full manifest. * delete: same lines in the pre-confirmation preview, so a user deleting "telemetry" can see it came from `github.com/kyle/telemetry-distribution` before they type `telemetry` to confirm. No change to the confirmation gate itself — deletion semantics are identical to plain profiles. 4. Install preview checks env vars against the current environment * Replaces the "Env vars you'll need to set:" header with a simpler "Env vars:" block. * Each required var is labeled: - `✓ set` — already in `os.environ` OR present as a key in the target profile's existing .env (update case). - `needs setting` — required but not found in either place. - `—` — optional. * Mirrors pip's "Requirement already satisfied" UX: no unnecessary nagging about keys the user already has configured. 5. Docs: private distributions * New "Private distributions" section in website/docs/reference/profile-commands.md explaining that we shell out to the user's `git` binary, so SSH keys / credential helpers / GitHub CLI stored creds all work transparently. One paragraph, two examples. * `hermes profile info` section updated to mention `Installed:`. Module-level hoist: * `from datetime import datetime, timezone` was previously lazy-imported inside plan_install(). Hoisted to module scope so tests can monkeypatch `hermes_cli.profile_distribution.datetime` to freeze time. Tests (+7): * TestInstalledAtStamp.test_install_stamps_installed_at — format check (4-digit year, 'T', +00:00 suffix). * TestInstalledAtStamp.test_update_refreshes_installed_at — freezes datetime.now() to 2099-01-01 and confirms update writes a new stamp. * TestProfileInfoDistribution.test_installed_distribution_shows_in_list — ProfileInfo.distribution_{name,version,source} populated after install. * TestProfileInfoDistribution.test_plain_profile_has_no_distribution_fields — plain profiles have None. * TestProfileInfoDistribution.test_malformed_manifest_does_not_break_list — broken distribution.yaml in one profile doesn't break list_profiles(). Validation: * 163/163 tests pass (56 distribution + 102 profile regression + 5 new from this commit — up from 158). * docs-lint: 0 errors. * E2E verified: install preview shows ✓/needs-setting per env var, `profile list` shows Distribution column, `profile show` + `delete` preview mentions source URL, `info` shows Installed: timestamp. * fix(profile-dist): clean errors + warn when overwriting plain profiles Two small polish fixes found during collision sweeps of the PR: 1. ValueError from validate_profile_name now caught cleanly * A distribution.yaml whose 'name' field can't be used as a profile identifier (spaces, path traversal, etc.) raises ValueError from hermes_cli.profiles.validate_profile_name, which was escaping as a raw Python traceback from 'hermes profile install/update/info'. * Broadened the except clause in all three handlers to catch (DistributionError, ValueError) — users now see: Error: Invalid profile name '../../etc/passwd'. Must match [a-z0-9][a-z0-9_-]{0,63} instead of a stack trace. 2. Install preview distinguishes plain profile overwrite from distribution re-install * When plan.target_dir exists and IS a distribution (has distribution.yaml), preview still shows the mild (profile exists — will overwrite distribution-owned files only) * When plan.target_dir exists but is a HAND-BUILT plain profile (no distribution.yaml), preview now shows a loud warning: ⚠ Profile exists but is NOT a distribution. Installing here will overwrite its SOUL.md, skills/, cron/, and mcp.json. Your memories, sessions, auth.json, and .env will be preserved, but any hand-edits to distribution-owned files will be lost. * Users who type 'hermes profile install foo --force' against a profile they hand-built now see what they're signing up for. User data is still safe (memories, sessions, auth, .env are in USER_OWNED_EXCLUDE), but custom SOUL/skills get stomped. Tests (+2): * TestErrorSurfaces.test_bad_profile_name_raises_valueerror_not_traceback * TestErrorSurfaces.test_path_traversal_name_rejected Validation: * 165/165 tests pass (was 163). * E2E: bad manifest names produce 'Error: Invalid profile name ...' with no traceback; installing over a plain profile shows the warning; re-installing over an existing distribution shows the normal overwrite message. * Bad HTTPS URLs still produce 'Error: git clone failed: ...' — git itself generates a clean enough message that no wrapper is needed. * 'install .' works correctly from any cwd. * fix(profiles): reject reserved names at validate time Before: `hermes profile create hermes` / `profile install` / `profile rename` all silently accepted reserved names like `hermes`, `test`, `tmp`, `root`, `sudo`. The profile directory was created; only alias creation failed (via check_alias_collision), leaving a confusingly-named profile on disk — e.g. `~/.hermes/profiles/hermes/` sitting next to `~/.hermes/` itself. The reserved set already exists (_RESERVED_NAMES, introduced alongside alias collision detection). This commit moves the check up one layer to validate_profile_name so every entry point — create, install, import, rename, dashboard web API — shares the same gate. The error message points the user at the cause without being cryptic: Error: Profile name 'hermes' is reserved — it collides with either the Hermes installation itself or a common system binary. Pick a different name. `default` continues to pass through (it's a special alias for ~/.hermes). _HERMES_SUBCOMMANDS (`chat`, `model`, `gateway`, etc.) stays at alias-collision time only — those are fine as bare profile names with `--no-alias`. Tests (+5): test_reserved_names_rejected parametrized over the full _RESERVED_NAMES set, matching the existing pattern in TestValidateProfileName. No existing test uses a reserved name as a profile identifier (greppped create_profile("hermes|test|tmp|root|sudo") — zero hits). Validation: * 170/170 tests pass in the profile suites. * E2E: `profile create hermes`, `profile install` with manifest name=hermes, and `profile install ... --name hermes` all produce the same clean `Error: Profile name 'hermes' is reserved ...` with rc=1 and no traceback. Normal names (`mybot`) still work. |