Skip to content

fix(gateway): drop leftover include_editorial kwarg from GET /v1/skills - #108968

Open
Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/api-server-skills-include-editorial
Open

Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/api-server-skills-include-editorial

Conversation

@Adolanium

Copy link
Copy Markdown

What does this PR do?

GET /v1/skills always returned HTTP 500. The Collective Wisdom revert removed include_editorial from _find_all_skills but left it on the api_server handler, and a broad except Exception turned the TypeError into a generic 500. The existing test mocked the finder with a kwargs-blind MagicMock, so the mismatch never showed up.

This drops the leftover kwarg and adds a regression test that calls the real finder.

Related Issue

Fixes #108967

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/platforms/api_server.py: _handle_skills calls _find_all_skills(skip_disabled=False) only.
  • tests/gateway/test_api_server.py: test_skills_enumerates_real_catalog plants a skill under the isolated HERMES_HOME and hits GET /v1/skills without mocking the finder.

How to Test

  1. scripts/run_tests.sh tests/gateway/test_api_server.py -k TestSkillsEndpoint -q
  2. On the base commit that test fails with assert 500 == 200 and TypeError: _find_all_skills() got an unexpected keyword argument 'include_editorial'.
  3. After this change both skills tests pass (2 passed).

Ran on Windows 11: TestSkillsEndpoint 2 passed. The full test_api_server.py file had 119 passed and 1 failed (test_security_headers_present). That failure rewrites CSP to injections.adguard.org and is unrelated local AdGuard injection, not this change.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A

The Collective Wisdom revert removed include_editorial from
_find_all_skills but left it on the api_server handler. The TypeError
was swallowed by a broad except and returned 500, and the existing test
mocked the finder so it never saw the kwargs.

GET /v1/skills now calls _find_all_skills(skip_disabled=False) only.
A regression test hits the real finder instead of a MagicMock.
@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) labels Sep 12, 2026
@alexkruegger

Copy link
Copy Markdown

Verified this exact change in production on v0.21.3 (HEAD 2cfb655): applied _sort_skills(_find_all_skills(skip_disabled=False)), restarted the gateway, GET /v1/skills → 200 with 18 skills (previously 500 with TypeError). Downstream it unblocks job creation for a client that gates on /v1/skills. Same one-line change we patched locally, so +1 from a real deployment.

This branch has not been deployed

No deployments
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 P2 Medium — degraded but workaround exists 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]: GET /v1/skills always returns HTTP 500

3 participants