Skip to content

Address review findings from v0.1 skills OpenAPI PR#4802

Merged
samuv merged 1 commit intomainfrom
fn-open-api-registry
Apr 14, 2026
Merged

Address review findings from v0.1 skills OpenAPI PR#4802
samuv merged 1 commit intomainfrom
fn-open-api-registry

Conversation

@samuv
Copy link
Copy Markdown
Contributor

@samuv samuv commented Apr 14, 2026

Summary

Addresses the three review comments from #4800:

  • Clamp out-of-range limit values to the max (200) instead of silently falling back to the default (50) — ?limit=201 now returns 200 results
  • Remove dead @Description annotation on skillsV01Metadata (swag overrides it with the field comment from skillsV01Response)
  • Add @Description and field comments to registryErrorResponse so the generated schema includes a description

Ref: #4800

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Spec regenerated with task docs

Changes

File Change
pkg/api/v1/registry_v01_skills.go Clamp limit to max instead of ignoring; remove dead @Description
pkg/api/v1/registry.go Add @Description and field comments to registryErrorResponse
pkg/api/v1/registry_v01_skills_test.go Update "limit over max" expectation to 200; add "limit at max" case
docs/server/* Regenerated spec

Does this introduce a user-facing change?

Yes — ?limit=N where N > 200 now returns 200 items (clamped) instead of silently returning 50.

@samuv samuv requested review from JAORMX and amirejaz as code owners April 14, 2026 11:40
@samuv samuv self-assigned this Apr 14, 2026
@github-actions github-actions bot added the size/XS Extra small PR: < 100 lines changed label Apr 14, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.00%. Comparing base (943bafb) to head (4753588).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4802      +/-   ##
==========================================
+ Coverage   68.96%   69.00%   +0.03%     
==========================================
  Files         517      517              
  Lines       54829    54833       +4     
==========================================
+ Hits        37815    37837      +22     
+ Misses      14098    14076      -22     
- Partials     2916     2920       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samuv samuv requested a review from rdimitrov April 14, 2026 11:51
Comment thread pkg/api/v1/registry_v01_skills.go
rdimitrov
rdimitrov previously approved these changes Apr 14, 2026
Clamp out-of-range limit values to the maximum (200) instead of
silently falling back to the default (50). Cap page to prevent
integer overflow in the (page-1)*limit calculation that could
panic on slice bounds.

Remove the dead @description annotation on skillsV01Metadata — swag
overrides it with the field-level comment from skillsV01Response.

Add @description and field-level doc comments to registryErrorResponse
so the generated schema includes descriptions consistent with the
other types promoted by the OpenAPI annotations.

Regenerate docs/server/ to reflect all changes.

Ref: #4800
Made-with: Cursor
@samuv samuv force-pushed the fn-open-api-registry branch from 567da35 to 4753588 Compare April 14, 2026 12:07
@github-actions github-actions bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 14, 2026
@samuv samuv requested a review from rdimitrov April 14, 2026 12:23
@samuv samuv merged commit daea31b into main Apr 14, 2026
41 checks passed
@samuv samuv deleted the fn-open-api-registry branch April 14, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants