fix(api_server): drop nonexistent include_editorial kwarg from GET /v1/skills - #117048
Closed
chrisworksai wants to merge 1 commit into
Closed
chrisworksai wants to merge 1 commit into
chrisworksai wants to merge 1 commit into
Conversation
…1/skills _handle_skills passed include_editorial=True to _find_all_skills, which only accepts skip_disabled. Every request raised TypeError, was swallowed by the broad except Exception, and returned HTTP 500 with an empty skills list for all API-server clients. The existing endpoint test patched _find_all_skills with a kwargs-blind MagicMock, so it accepted the bogus keyword and stayed green. Patch with autospec=True so the mock enforces the real signature.
Author
|
Closing as a duplicate. #108967 already tracks this with three independent reproductions, and #108968 / #113058 / #115681 are open against it — a fourth fix is noise. One correction I owe this thread before closing: the issue I opened alongside this PR claimed The one piece here that may still be worth salvaging is the test approach; I've left it as a note on #108967 rather than pressing this PR. |
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.
What does this PR do?
Fixes a hard HTTP 500 on
GET /v1/skills._handle_skillspassedinclude_editorial=Trueto_find_all_skills, which only acceptsskip_disabled:gateway/platforms/api_server.py:2710tools/skills_tool.py:183—def _find_all_skills(*, skip_disabled: bool = False)The broad
except Exceptionin the handler swallowed theTypeErrorand rewrote it as a generic"Failed to enumerate skills"500, so every api_server client saw an empty skills list with no useful error. The Hermes Browser Extension's SKILLS panel is permanently empty as a result.Why drop the argument rather than add the parameter:
include_editorialappears nowhere else in the tree (git grepfinds only that one line), andgit log -S include_editorial -- tools/skills_tool.pyis empty, so the keyword was never a parameter of_find_all_skills. There is no behavior to preserve. The four other callers (hermes_cli/banner.py,skills_config.py,skills_hub.py,web_server.py) all pass onlyskip_disabled. Implementing aninclude_editorialfeature here would be inventing one.Related Issue
Fixes #117046
Related: #108967 reports the same 500, with open PRs #108968 / #113058 / #115681. Happy to close this in favor of whichever the maintainers prefer — this one additionally hardens the test that let the bug ship.
Type of Change
Changes Made
gateway/platforms/api_server.py—_handle_skillsnow calls_find_all_skills(skip_disabled=False); removed theinclude_editorial=Truekeyword.tests/gateway/test_api_server.py—TestSkillsEndpoint::test_skills_returns_list_envelopenow patches withautospec=True. The existing plainMagicMockaccepted any keyword, so the test stayed green while the live route answered 500. Withautospec=Truethe mock enforces the real signature and the test fails on unpatchedmain.How to Test
main:Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the affected files, not the full suite.tests/gateway/test_api_server.pyis 125/125 green.tests/tools/test_skills_tool.pyhas 4 failures (TestSkillViewCollisionDetection,TestSameRootDuplicationResolves) that reproduce identically on unmodifiedupstream/main, so they are pre-existing and unrelated.Documentation & Housekeeping
docs/, docstrings) — N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A