fix(client): target the skills /refresh route instead of /update#242
Conversation
SkillsClient.refreshSkill POSTed to /api/skills/installed/{name}/update,
which the agent-server does not expose. The route is /refresh. Updates
the mocked unit assertion, adds a focused unit test for the corrected
path, and adds an integration contract guard (404 for an uninstalled
skill) that proves the route exists on the pinned image.
Endpoint audit❌ 7 off-contract call(s) — not on the agent-server · classifiers: cloud
❌ Not on agent-server (gated, 7)⛔ (no known backend) — served by no backend we can see (6)
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. Verdict: ✅ Approved. One non-blocking 🟡 suggestion: the new |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — surgical one-line URL fix in skills-client.ts, with the test suite brought in line and a live-server contract guard added. No fundamental issues.
The fix matches the upstream route exactly (POST /api/skills/installed/{skill_name}/refresh, confirmed in software-agent-sdk/.../skills_router.py), and the new integration test pins the 404-for-uninstalled-skill contract end-to-end against the real agent-server image. CI is green across all 11 checks (including integration-test and endpoint-audit).
One non-blocking suggestion below.
What the PR actually changes (3 files, +19/-2 net):
src/client/skills-client.ts:80—/update→/refresh(the real bug).src/__tests__/api-clients.test.ts:513— existing combined test updated, plus a new dedicated unit test forrefreshSkill.src/__tests__/integration/deterministic-api.integration.test.ts— new live-server contract test that asserts the route exists and returns 404 for an uninstalled skill.
Why I like it:
- Smallest possible diff to fix a real production 404.
- Both halves are covered: a fast mocked test for the URL string, and a slow integration test that proves the route is actually wired up on the server side. The 404 path is the cheapest possible 'does this route exist?' probe — no fixtures to maintain.
- No unrelated formatting, refactoring, or 'while I'm here' changes.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| ); | ||
| }); | ||
|
|
||
| it('SkillsClient.refreshSkill POSTs to the /refresh route', async () => { |
There was a problem hiding this comment.
🟡 Suggestion — the new unit test mostly duplicates the combined-skills test above.
This test mocks fetch and asserts that fetch was called with the expected URL — that's the 'mocks aren't tests' pattern: it will pass even if SkillsClient.refreshSkill is rewritten to use fetch directly, hard-coded URL, or anything else, as long as it happens to hit that string. It doesn't exercise the HttpClient.post<T> → URL-encoding → response-parse path that the combined test at ~L485 already exercises thoroughly (including the toHaveBeenNthCalledWith(6, ..., { method: 'POST' }) URL assertion you updated at L513).
Two cleaner options, pick whichever you prefer:
- Delete this new test — the updated assertion in the combined test already pins the URL + method. The standalone integration test in
deterministic-api.integration.test.tsis what gives real coverage of the route. - Make this test do real work — drop the
global.fetchmock and pass a stubHttpClient(or a real one against anunfetch-style local adapter) so the assertion exercises actual URL building / response parsing, not just 'did you call fetch with this string.'
Not blocking — option 1 is fine if you want to keep the diff minimal.
|
🚀 Released in v1.32.0. |
2 participants
Summary
Split from #231 (one endpoint per PR).
SkillsClient.refreshSkill()POSTed to/api/skills/installed/{name}/update, but the agent-server does not expose that route — the correct route is/refresh(verified against the agent-server skills router inOpenHands/software-agent-sdk).POST /api/skills/installed/{name}/refresh/update)SkillsClient.refreshSkill()→/refreshTesting
src/__tests__/api-clients.test.ts, mockedfetch): updated the existing combined-skills assertion from/updateto/refresh, plus a new focused test assertingrefreshSkillPOSTs to the/refreshroute.deterministic-api.integration.test.ts, against the pinned agent-server image): a contract guard that a valid-but-uninstalled skill name reaches the handler and returns404— proving the route exists on the image and the client targets the corrected path (client↔server drift the mocked unit test cannot catch).npm run build,lint(0 errors), andformat:checkpass; full unit suite green.