Skip to content

fix(api): restore GET /v1/skills listing - #120836

Closed
KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/120831-skills-endpoint-discovery-contract
Closed

KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/120831-skills-endpoint-discovery-contract

Conversation

@KoNit-K

@KoNit-K KoNit-K commented Sep 24, 2026

Copy link
Copy Markdown

What does this PR do?

GET /v1/skills returned HTTP 500 because APIServerAdapter._handle_skills passed
include_editorial=True to _find_all_skills, whose current public signature accepts
only skip_disabled. This change calls the discovery helper with its supported argument
so authenticated API clients receive the normal sorted skill-list envelope.

Related Issue

Fixes #120831

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/platforms/api_server.py — removed the obsolete include_editorial keyword from the skills discovery call.
  • tests/gateway/test_api_server.py — made the endpoint mock signature-aware, so incompatible helper keywords make the HTTP contract test fail.

How to Test

  • HERMES_PYTHON=/Library/Frameworks/Python.framework/Versions/3.11/bin/python3 scripts/run_tests.sh tests/gateway/test_api_server.py -k test_skills_returns_list_envelope -q — 1 passed
  • /Library/Frameworks/Python.framework/Versions/3.11/bin/python3 -m ruff check gateway/platforms/api_server.py tests/gateway/test_api_server.py — All checks passed

Evidence

  • BEFORE RED: the focused endpoint test returned 500 and the autospecced helper raised TypeError: got an unexpected keyword argument 'include_editorial'.
  • AFTER GREEN: the same focused HTTP request returned the list envelope; 1 test passed.
  • CONTROL: the test still verifies both returned skills and their name, description, and category fields.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run relevant tests locally (see How to Test)
  • I've added tests for my changes
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • Documentation update: N/A
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md or AGENTS.md: N/A
  • Cross-platform impact considered
  • Tool descriptions/schemas: N/A

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery tool/skills Skills system (list, view, manage) sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages duplicate This issue or pull request already exists labels Sep 24, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Duplicate of #108968: both remove the stale include_editorial argument, and #108968 already has real-catalog endpoint regression coverage.

@cYoren cYoren left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the call contract against tools/skills_tool.py: _find_all_skills accepts skip_disabled only, so dropping include_editorial fixes the 500. Adding autospec=True makes the regression test enforce that signature instead of allowing invalid kwargs through.

Local focused verification on current origin/main checkout: scripts/run_tests.sh tests/gateway/test_api_server.py::TestSkillsEndpoint::test_skills_returns_list_envelope — 1 test passed. No GitHub CI checks are reported yet. GitHub does not grant my account explicit repository access for an approval review, so I am leaving this verification as a comment rather than claiming approval.

KoNit-K commented Sep 24, 2026

Copy link
Copy Markdown
Author

Closing this in favor of the earlier #108968. Both PRs remove the same unsupported include_editorial argument from _find_all_skills(...); #108968 also exercises GET /v1/skills with the real finder and an isolated skill directory, so it catches the original HTTP 500 and verifies discovery and the response payload.

The autospec=True addition here is useful test hardening, but it does not justify a separate PR for the same production fix. If useful, it can be picked up in #108968. The fix has not landed in main yet, so #108968 should remain open for review. Thanks to @cYoren for verifying the call contract.

@KoNit-K KoNit-K closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: TypeError: _find_all_skills() got an unexpected keyword argument 'include_editorial' in GET /v1/skills

3 participants