fix(gateway): apply platform-disabled skill gate to auto-skill channel bindings - #59478
pierrenode wants to merge 1 commit into
Conversation
|
looks mergeable I reviewed the gateway auto-skill disabled-gate fix against current GitHub Security evidence:
Signed: GPT-5.5-xhigh in Codex |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this remaining auto-skill path. The production change addresses a live gap: current gateway/run.py:11157-11185 loads and builds new-session auto-skills without consulting get_disabled_skill_names(), while the established bundle path already applies the same canonical-name/raw-identifier gate in agent/skill_bundles.py:289-320.
Problems
tests/gateway/test_auto_skill_platform_disabled.py:56parsesinspect.getsource(gateway_run)into an AST.AGENTS.md:1358-1411explicitly bans source-reading/source-shape tests; this test can pass without proving disabled content is excluded from the emitted gateway turn.
Suggested changes
- Replace the AST assertion with a focused behavioral gateway test that drives a new-session
event.auto_skillthrough the handler and asserts disabled content is absent, with an enabled-skill control.
Automated hermes-sweeper review.
| every new session bound to it via channel_skill_bindings / DM Topics. | ||
| """ | ||
| tree = ast.parse(inspect.getsource(gateway_run)) | ||
|
|
There was a problem hiding this comment.
Please replace this source/AST assertion with a behavioral gateway-path test. AGENTS.md:1358-1411 explicitly bans tests that read production source because they pin implementation shape rather than proving that disabled auto-skill content is excluded from the emitted turn.
197b818 to
1a78a1b
Compare
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the auto-skill channel-binding path that bypasses platform/global disabled-skill filtering. #59478 gates both raw identifiers and resolved display names and now exercises the behavior directly, while #59511 only gates raw identifiers before loading and includes an unrelated dashboard-auth change.
Related pull requests
- #59478
related— (+187/-17) — merge: Extracts auto-skill resolution, applies the platform-scoped disabled set to both the raw binding identifier and loaded display name before message construction, and behaviorally verifies that disabled content is excluded while enabled content remains. The diff addresses the keep_open review on #59478 by replacing the prohibited source-shape/AST test with focused behavioral tests, including raw-name, display-name, enabled-control, and platform-forwarding cases. - #59511 [closed]
duplicate— (+100/-1) — keep closed as duplicate of #59478: It targets the same bypass but checks only the raw identifier before payload loading, retains source-reading tests, and bundles an unrelated hermes_cli/dashboard_auth/prefix.py change. It remains relevant as the superseded duplicate that established the same intended gate but is less complete and branch-contaminated.
Duplicates
#59511 substantially duplicates #59478's auto-skill platform-disabled gate; #59478 is the cleaner and more complete canonical implementation.
Suggested consolidation
Merge #59478: its diff closes the reported bypass for both raw and canonical/display skill names and its revised behavioral tests explicitly resolve the keep_open review's test-design objection. Keep #59511 closed as a duplicate; its unrelated dashboard-auth change should only proceed through a separate clean PR if still needed.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup59478 ["PRs duplicating each other"]
P59478["PR #59478 (open)"]
P59511["PR #59511 (closed)"]
end
class P59478 open
class P59511 closed
class P59478 target
click P59478 "https://github.com/NousResearch/hermes-agent/pull/59478"
click P59511 "https://github.com/NousResearch/hermes-agent/pull/59511"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 15 kB of PR diffs, 5 kB of issue/PR text, 3 kB of discussion (5 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
1a78a1b to
d491740
Compare
|
looks mergeable Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
d491740 to
39fc513
Compare
|
Rebased onto current |
…l bindings _handle_message_with_agent's auto-skill block loads bound skills (channel_skill_bindings, Telegram DM Topics) via a raw identifier — the same bypass class the stacked (NousResearch#58888) and bundle (NousResearch#59156) invocation paths had: it skips get_skill_commands()'s scan-time disabled filter, so an operator who disables a skill for this platform (or globally) still had its full content injected into every new session bound to that channel/topic. Re-check get_disabled_skill_names() before injecting, mirroring the stacked/bundle gates. The loading+gate logic is extracted into a standalone _resolve_auto_skill_content() so it can be driven directly in tests instead of asserting on gateway/run.py's source shape.
39fc513 to
16d7871
Compare
What does this PR do?
_handle_message_with_agent()'s auto-skill block (Telegram DM Topics, Discordchannel_skill_bindings) loads bound skills via_load_skill_payload()with a raw identifier, bypassingget_skill_commands()'s scan-time disabled filter. Result: a skill an operator disables for a platform (or globally, viaskills.disabled) still gets its full content injected into every new session bound to that channel/topic.The stacked-skill (#58888) and bundle (#59156) invocation paths already re-check
get_disabled_skill_names()for exactly this reason — the auto-skill block was the one path ingateway/run.pystill missing it.Fix: check each resolved skill's name against
get_disabled_skill_names(platform=...)before injecting it, skip and log disabled ones. Mirrors the existing stacked/bundle gates exactly (same helper, same platform scoping, same log style). No behavior change for any binding that references an enabled skill.Related Issue
No filed issue — found via sibling-gap review of #58888 / #59156 (both merged in the last 24h to the same file).
Type of Change
Changes Made
gateway/run.py: re-checkget_disabled_skill_names()in the auto-skill loading loop, skipping disabled skills (+14 lines)tests/gateway/test_auto_skill_platform_disabled.py: new AST invariant regression test —_handle_message_with_agentrequires a large unrelated mocked harness to invoke directly, so this mirrors the existingtest_10710_auto_reset_evicts_cached_agent.pyapproach (verified: fails without the fix, passes with it)How to Test
Also re-ran the full sibling suite to confirm no regressions:
test_stacked_skill_platform_disabled.py,test_discord_channel_skills.py,test_slack_channel_skills.py,test_dm_topics.py,test_fresh_reset_skill_injection.py,test_skill_bundles.py,test_skill_commands.py— 156 passed.Checklist