fix(gateway): /model off the event loop; distribution allowlist enforced; hindsight env file 0600 - #75888
Merged
Merged
Conversation
…loop resolve_display_context_length() runs two blocking chains: the route comparison in should_clear_context_pin() and the provider probe ladder in get_model_context_length() (blocking requests calls to Anthropic /v1/models, Copilot, Nous, Codex, GMI, Ollama, models.dev and OpenRouter). The gateway message path already offloads both via get_model_context_length_async() and should_clear_context_pin_async(), but the /model slash-command handlers (_handle_model_command, _finish_switch) called the sync helper directly, freezing the whole event loop for the duration of the probe ladder - no messages processed on any platform, and the Discord heartbeat timeouts that get_model_context_length_async() was introduced to prevent. Add resolve_display_context_length_async(), a thin asyncio.to_thread wrapper mirroring the two existing *_async helpers (no logic duplication), and await it at both handlers.
…ioral offload tests Follow-ups to the previous commit (#74155 by @Drexuxux): - enrich_model_switch_warnings_for_gateway() -> merge_preflight_compression_warning() still called the sync resolve_display_context_length() provider probe ladder inline in both async /model call sites; dispatch it via asyncio.to_thread. - Replace the inspect.getsource() test (source-reading tests are banned by AGENTS.md) with behavioral tests that drive the real _handle_model_command: assert the resolver runs off the loop thread and that the warning enrichment is dispatched through asyncio.to_thread.
…_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
…ing 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.
The embedded Hindsight daemon's profile env file carries the plaintext HINDSIGHT_API_LLM_API_KEY but was written via bare write_text(), leaving it with umask-derived (typically world-readable) permissions. - Create/truncate the file via os.open(..., 0o600); chmod a pre-existing file to 0600 BEFORE writing new secret bytes. - Post-write validation on POSIX: verify 0600, retry chmod, and raise if the file still isn't owner-only. - If validation fails, unlink the secret file so a plaintext key is never left behind with unverified permissions. - Regression tests under tests/plugins/ for fresh-write mode, tightening a pre-existing 0644 file, and cleanup on validation failure. Narrowed reimplementation of #74236 confined to plugins/memory/hindsight/; the core utils.py atomic-replace opt-out from the PR was dropped. Co-authored-by: carrion256 <carrion256@proton.me>
Contributor
૮ >ﻌ< ა ci reviewran on dfef31e ℹ️ InfoDesktop E2E visual evidence · View test artifacts · View job3 visual diffs. inline evidence upload failed. Failed to upload diff-1508682a2ae8-boot-ready-diff.png with gh image (exit code 1): Error uploading /home/runner/work/_temp/e2e-evidence/diff-1508682a2ae8-boot-ready-diff.png: step 0 (get upload token): uploadToken not found on repo page — do you have write access to NousResearch/hermes-agent? (or, if NousResearch enforces SAML SSO, authorize at https://github.com/orgs/NousResearch/sso) |
Contributor
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
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.
salvage: gateway /model loop offload, distribution_owned enforcement, hindsight env perms
Combines three narrowed community fixes onto
main(branchsalvage/gateway-misc-fixes).Fixes #74373
1.
/modelcontext-length resolution off the gateway event loop — credit @Drexuxux (#74155)Cherry-picked as-authored: the sync
resolve_display_context_length()(routecomparison + provider probe ladder of blocking
requestscalls) was invokeddirectly inside the async
/modelhandlers ingateway/slash_commands.py,freezing the whole event loop (~2s measured, worse on cold caches). The PR adds
resolve_display_context_length_async()— a thinasyncio.to_threadwrappermirroring the existing
get_model_context_length_async()/should_clear_context_pin_async()siblings — and awaits it at both handlers.Our follow-up commit:
enrich_model_switch_warnings_for_gateway()→merge_preflight_compression_warning()still called the sync resolver inlineat both gateway call sites; both now dispatch via
asyncio.to_thread.inspect.getsource()ongateway.slash_commands(source-reading tests are banned by AGENTS.md).Replaced with behavioral tests in
tests/gateway/test_model_command_context_offload.pythat drive the real_handle_model_commandwith a mocked switch pipeline and assert (a) theresolver runs off the loop thread and (b) the warning enrichment is
dispatched through a spied
asyncio.to_thread. The other 3 behavioral testsfrom the PR are kept unchanged.
2.
distribution_ownedallowlist enforcement — credit @webtecnica (#74414, issue #74373)Cherry-picked only the allowlist-enforcement commit (
a329bac):_copy_dist_payload()iterated every staged entry and never consulted themanifest's
distribution_owned, making the documented allowlist declaration-only.Dropped: commit
1b82637(credential_pool/gateway/electron tokenwrite-through) — unrelated to the allowlist bug and introduces a token with no
reader; out of scope for this salvage.
Our follow-up commit fixed two defects in the cherry-picked filter:
owned_paths(), which falls back toDEFAULT_DIST_OWNED— so distributionsthat omit
distribution_owned(the common case) would silently stop copyingundeclared-but-legitimate top-level payload. Restored the legacy contract:
when the list is omitted, everything outside
USER_OWNED_EXCLUDEis copied.Enforcement now applies only when an explicit allowlist is declared.
nested entries like
skills/research/andcron/digest.json. Explicitallowlists are now resolved path-aware: nested entries select exactly that
subtree/file (siblings under the same parent are not dragged along), and
traversal segments (
.., absolute paths) andUSER_OWNED_EXCLUDEroots arerejected.
allowlist) on top of the PR's own tests.
On
Fixes #74373: yes — the salvaged behavior resolves the issue. The coredefect (allowlist never consulted from the mutation path) is fixed; unlisted
entries are no longer installed or updated when a manifest declares
distribution_owned. The issue's secondary concern (target-only childrenremoved by wholesale directory replacement) is also addressed by the path-aware
semantics: in the issue's repro (
distribution_owned: [SOUL.md]),skills/isno longer touched at all, so
skills/local-only/survives update; and authorscan now scope ownership to
skills/research/to keep sibling subtreesuser-owned. Wholesale replacement remains only for paths an author explicitly
declares as distribution-owned, which is the documented ownership boundary the
issue asked to make unambiguous.
3. Hindsight embedded profile env file permissions — credit @carrion256 (#74236)
Reimplemented as a minimal fix confined to
plugins/memory/hindsight/(authored by us with
Co-authored-by: carrion256 <carrion256@proton.me>):_materialize_embedded_profile_env()wrote the plaintextHINDSIGHT_API_LLM_API_KEYvia barewrite_text(), leaving the file withumask-derived (typically world-readable) permissions.
os.open(..., O_CREAT|O_TRUNC, 0o600); apre-existing file is chmod'd to 0600 before the new secret bytes land.
raises
PermissionErrorotherwise.behind with unverified permissions.
tests/plugins/memory/test_hindsight_env_perms.py(fresh write under a permissive umask, tightening an existing 0644 file,
cleanup on validation failure).
Dropped from the PR: the core
utils.pyatomic-replace opt-out(
allow_copy_fallback=False) and the mkstemp/fsync/atomic-rename machineryplus the daemon-restart metadata plumbing built on it — that changes a shared
core utility for a plugin-local concern and violates the plugin-boundary rule
(plugin fixes touch only
plugins/+tests/plugins/). The 0600 + cleanupguarantee is achieved without it.
Verification
tests/hermes_cli/test_model_switch_context_offload.py,tests/gateway/test_model_command_context_offload.py,tests/gateway/test_model_command_async_offload.py,tests/gateway/test_model_switch_persistence.py,tests/hermes_cli/test_profile_distribution.py,tests/plugins/memory/— 288 passed.ruff checkclean on all touched files.origin/mainbefore push.Infographic