fix(skills): normalize path separators in content hash for Windows compatibility - #62313
Open
liuhao1024 wants to merge 38 commits into
Open
fix(skills): normalize path separators in content hash for Windows compatibility#62313liuhao1024 wants to merge 38 commits into
liuhao1024 wants to merge 38 commits into
Conversation
alt-glitch
added
type/bug
scripts/desktop-sandbox.sh runs a Hermes desktop instance in an isolated sandbox — separate HERMES_HOME, separate Electron userData, and a distinct app name (HERMES_DESKTOP_APP_NAME) so it doesn't compete with the main desktop instance's single-instance lock. Two modes: - Ephemeral (default): temp dir, cleaned up on exit - --persistent: stored under .hermes-sandbox/ in the worktree git root, survives restarts for repeat testing In the Nix devShell the script is available as 'sandbox'. Also makes APP_NAME overridable via HERMES_DESKTOP_APP_NAME in main.ts — app.setName() runs before requestSingleInstanceLock(), so the overridden name changes the lock key. collectRelaunchEnv already preserves HERMES_DESKTOP_* vars through self-update relaunches; test updated to cover the new env var.
Codex assigns assistant message items server-side ids that can run 400+ chars (base64 encrypted blobs), but the Responses API caps input[].id at 64 chars and rejects the whole request with a non-retryable HTTP 400. Once a session captures one of these long ids, every subsequent turn replays it and 400s forever, since the history persists it in codex_message_items. Add a 64-char length guard at both replay sites — the history-to- input converter and the final preflight gate — so oversized ids are dropped while short ids (msg_...) are kept for prefix-cache hits. Mirrors the existing pattern for reasoning items, which already strip their id before replay because store=False means the API can't resolve ids server-side anyway. Fixes NousResearch#27038 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…the render ref Fixes NousResearch#54527 — a message typed into one TUI session could be silently misrouted into (or overwritten by) another concurrently-open session. Root cause: activeQueueSessionKeyRef is written on every render, but the debounced draft-persist timer, the pagehide flush, and dispatchSubmit's reject-restore path all read it lazily at async-resolve time instead of capturing the scope that was active when the operation started. A session switch landing between capture and resolve relabels one session's text under the other session's key. A large paste widens the window (slower synchronous render), which matches the original report. Fix: introduce draftScopeRef, written only by the draft-swap effect (so it always reflects the session whose text is actually loaded in the editor) and read it instead of the render-time ref at both async write sites. dispatchSubmit's restore() now uses the submittedScope already captured at dispatch instead of re-reading the live ref. Also adds isPendingDraftPersistCurrent as defense-in-depth: before the debounce timer commits a write, it verifies its captured {scope, text} pair is still the one on file. This is a no-op under the fix above (a session swap or a newer keystroke already clears/replaces the pending entry via clearTimeout), but turns any future regression that reintroduces a stale/live-ref read at this call site into a dropped write instead of a silent cross-session misroute. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…earch#54527) Snapshot the selected stored session and route token for the full async submit pipeline so a mid-flight session switch cannot resume the wrong chat or misroute the user's text. Includes regression tests.
Cron jobs in the gateway process wedged before HTTP on later non-streaming API calls because interruptible_api_call spawned a daemon worker inside nested cron thread pools. Route cron platform turns through direct_api_call on the conversation thread instead.
Keep empty-tail recovery scoped to the current stream segment and bound fallback flood retries. Preserve Telegram's server retry hint without blocking final delivery through a long cooldown.
…ions Copilot (api.githubcopilot.com/responses) binds replayed assistant codex_message_items ids to a specific backend "connection". Credential- pool rotation, a gateway restart, or routine load-balancer churn between turns all invalidate that binding, and Copilot rejects the stale id with HTTP 401 "input item ID does not belong to this connection" — even for short ids well under the NousResearch#27038 64-char length cap, since this is a connection-scope problem, not a length problem. Once a session captures one of these ids it is persisted and replayed forever, permanently bricking the session. Thread an is_github_responses flag from build_kwargs/convert_messages into _chat_messages_to_responses_input and drop the id unconditionally on that path, mirroring how reasoning items already strip id on replay. phase/status/content are still replayed so cache-relevant signal isn't lost — only the connection-scoped id is unsafe to reuse. Written to apply independently of the NousResearch#27038 length-cap fix so the two PRs don't block each other; they touch adjacent conditions in the same block and merge cleanly in either order. Fixes NousResearch#32716 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_CodexCompletionsAdapter (agent/auxiliary_client.py) is a second, independent producer of Codex Responses input — used by auxiliary calls (context compression, flush_memories, MoA aggregation, session_search) that route through CodexAuxiliaryClient instead of the main agent's ResponsesApiTransport.build_kwargs. It calls _chat_messages_to_responses_input() directly without is_github_responses, so the previous commit's fix didn't cover it: an auxiliary call made against a Copilot-backed session could still replay a connection-scoped codex_message_items id and hit the same HTTP 401. Detect the Copilot host from the adapter's own client.base_url (same check the adapter already does further down for prompt_cache_key opt-out) and pass is_github_responses through, closing the gap. Still NousResearch#32716. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Require literal booleans for backend-specific replay policy and pin non-default status and content preservation through both response paths.
Reapply the endpoint-aware preflight after request and execution middleware so no override can reintroduce a connection-scoped ID.
Exercise request and execution middleware replacements through the real conversation loop and assert the provider payload is sanitized.
… in fetch_models fetch_models() sends Authorization: Bearer <api_key> plus any default_headers (x-api-key etc.) via urllib.request.urlopen, and urllib's redirect handler forwards every header when following a 3xx — including to a different host. A catalog endpoint (or a compromised/misconfigured proxy in front of it) answering with a redirect to another origin therefore received the provider API key. Install an HTTPRedirectHandler that drops authorization, x-api-key, api-key, x-goog-api-key and cookie when the redirect target hostname differs from the original request, mirroring the pattern already used in skills/creative/comfyui/scripts/_common.py. Same-host redirects keep credentials so legitimate path-level redirects still work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g credentials Review feedback: a same-host redirect to a different port can land on a different service, which must not inherit the provider API key. Compare (scheme, hostname, effective port) — with 80/443 defaults — instead of hostname alone, and add a two-server regression test for the same-host/different-port case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`hermes -t web chat` silently dropped the toolset filter (and the same hold true for `-m`, `--provider`, `--tui`, `--dev` placed before `chat`). Reported in NousResearch#28780 for `-t/--toolsets`; the others are sibling failures with the same root cause. Root cause: the chat subparser re-declared these flags with `default=None` (or `default=False` for store_true) on top of the matching top-level parser flags. When argparse dispatches into the subparser it shares the namespace via `dest`, so the subparser's default overwrites whatever the top-level parser parsed before the subcommand. `-s/--skills`, `-r/-c/-w`, `--yolo`, and `--pass-session-id` already use `default=argparse.SUPPRESS` for exactly this reason — the chat-subparser action becomes a no-op unless the user explicitly passes the flag after `chat`, and the parent value survives. Reproduction (origin/main, before fix): >>> parser.parse_known_args(["-t", "web", "chat"]).toolsets None >>> parser.parse_known_args(["chat", "-t", "web"]).toolsets 'web' After fix: >>> parser.parse_known_args(["-t", "web", "chat"]).toolsets 'web' >>> parser.parse_known_args(["chat", "-t", "web"]).toolsets 'web' Sibling flags fixed in the same commit because they share the exact same argparse pattern bug — verified via a new contract test that scans every chat-subparser action whose `dest` is also on the top-level parser and asserts `default is argparse.SUPPRESS`. The test fails on origin/main listing all five offenders and passes after this fix. Test additions in tests/hermes_cli/test_argparse_flag_propagation.py: - TestChatSubparserInheritedValueFlags exercising real `_parser` build (not the hand-rolled replica) so it catches future drift. - Parametrized before-chat / after-chat cases for `-t`, `--toolsets`, `-m`, `--model`, `--provider`. - Negative case: passing none of the flags leaves attrs at the top-level parser's `None` default (SUPPRESS does not remove existing attrs). - Combined case: all three value flags before `chat` simultaneously. - store_true cases for `--tui` / `--dev`. - Contract test asserting every shared-`dest` flag on chat uses SUPPRESS. Fixes NousResearch#28780.
teknium1
reviewed
Jul 11, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing the paired hash contract. The current implementation still records content_hash(install_dir) during installation (tools/skills_hub.py:3528) and compares it with bundle_content_hash(bundle) during update checks (tools/skills_hub.py:3637-3639), so normalizing both hash inputs is the right scope.
Problems
- The PR diff contains no test changes. Existing bundle/disk symmetry coverage at
tests/tools/test_skills_hub.py:1150-1169and:1212-1232uses forward-slash keys only, so it cannot catch the Windows separator and case-ordering divergence this patch fixes.
Suggested changes
- Add a nested backslash-key bundle fixture against an equivalent on-disk tree, plus a Windows-only mixed-case ordering assertion.
Automated hermes-sweeper review.
| """Compute a deterministic hash for an in-memory skill bundle.""" | ||
| """Compute a deterministic hash for an in-memory skill bundle. | ||
|
|
||
| Must stay symmetric with ``tools.skills_guard.content_hash`` — both |
Contributor
There was a problem hiding this comment.
Please add a regression test for this normalization contract. Existing symmetry tests use forward-slash bundle keys only, so they do not exercise the Windows backslash-path case this line fixes.
…hao/cron-bugfix-62310
Addresses teknium1 review feedback on PR NousResearch#62310. Three new tests: 1. test_bundle_disk_symmetry_with_backslash_paths - Simulates Windows-style backslash-separated bundle keys (e.g., "refs\notes.md") and verifies they hash identically to on-disk forward-slash paths. 2. test_path_sorting_is_deterministic_across_platforms - Catches the backslash (ASCII 92) vs forward slash (ASCII 47) sorting divergence that could cause non-deterministic hashes across platforms. 3. test_windows_mixed_case_path_ordering - Verifies hash stability when bundle keys use non-canonical casing (e.g., "Docs\README.md" on case-insensitive filesystems).
teknium1
added
the
sweeper:blast-moderate
14 tasks
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
12 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.
What does this PR do?
Fixes a permanent false-positive
update_availablebug inhermes skills checkon Windows. After a successfulhermes skills update, the check command continued listing all updated skills as needing updates forever because two hash functions —bundle_content_hash(in-memory) andcontent_hash(on-disk) — produced different digests for the same skill on Windows.Root cause: Path separator and sorting differences between the two functions:
bundle_content_hashusedsorted(bundle.files)which preserves Windows backslashes (e.g.references\methods\x.md) and sorts strings case-sensitivelycontent_hashnormalized to forward slashes viaas_posix()(e.g.references/methods/x.md) but sorted Path objects, which on Windows are case-insensitiveSince relative paths are mixed into the SHA-256 hash, skills with subdirectory files could never match between disk and bundle on Windows. POSIX systems were unaffected.
Fix: Normalize all paths to forward slashes and sort by the normalized path in both functions, ensuring deterministic ordering across platforms. Added a normalization contract to both docstrings and cross-referenced the functions in comments to prevent future divergence.
Related Issue
Fixes #62310
Type of Change
Changes Made
tools/skills_hub.py::bundle_content_hash: Normalize path separators to/and sort by normalized path before mixing into hashtools/skills_guard.py::content_hash: Sort byas_posix()normalized path to ensure deterministic ordering; added normalization contract to docstringHow to Test
references/methods/x.md)bundle_content_hash(bundle)andcontent_hash(skill_path)produce identical hashes regardless of path separator styleObserved result: Hash symmetry restored on all platforms (verified on macOS with backslash/foward-slash simulation; Windows users will see the fix after merging)
To manually verify the fix before Windows testing:
Checklist
Code
fix(skills):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A