Skip to content

chore: Require descriptions on every external API field - #3153

Open
pulpdrew wants to merge 1 commit into
mainfrom
drew/add-more-open-api-descriptions
Open

pulpdrew wants to merge 1 commit into
mainfrom
drew/add-more-open-api-descriptions

Conversation

@pulpdrew

Copy link
Copy Markdown
Contributor

Summary

This PR

  1. Updates the OpenAPI Spec Linter to require descriptions on every field, to avoid blank descriptions being generated for the Cloud OpenAPI Specs.
  2. Adds descriptions to the few fields that were missing them previously.

Screenshots or video

How to test on Vercel preview

References

  • Linear Issue:
  • Related PRs:

@changeset-bot

changeset-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0368c1d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Sep 18, 2026 1:08pm UTC
hyperdx-storybook Ready Ready Preview Sep 18, 2026 1:08pm UTC

Request Review

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Sep 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Touches API routes or data models — hidden complexity risk

Additional context: grazes a critical path (9 lines in packages/api/src/routers/external-api/v2/charts.ts) inside a 44-line PR, so it is tiered on size

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 5
  • Production lines changed: 44
  • Branch: drew/add-more-open-api-descriptions
  • Author: pulpdrew

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the generated contract satisfies the new description rules and remains aligned with its route annotations.

Summary

This PR strengthens validation of the published external OpenAPI contract and documents the remaining undescribed fields.

  • Requires descriptions for path and query parameters.
  • Extends schema-property description checks to nested and inline schemas.
  • Keeps route annotations and the generated OpenAPI artifact aligned.

Reviews (1) · Last reviewed commit: "chore: Require descriptions on every ext..."

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This diff is documentation- and CI-config-only: it adds description strings to a handful of OpenAPI error-response properties and two path parameters, regenerates openapi.json, and tightens the Spectral lint rule to require descriptions on every schema property and parameter. The TypeScript edits live entirely inside @openapi JSDoc comment blocks — no executable code changes. The generated openapi.json and its JSDoc sources were verified in sync, and the additions are purely additive (no field, type, or required-ness changes), so there is no consumer-visible contract impact. CI enforcement is real: main.yml runs ci:lint (which invokes lint:openapispectral lint) on PRs and pushes, and a dedicated check-openapi-sync.sh step asserts the spec stays regenerated from its sources.

🔵 P3 nitpicks (1)
  • packages/api/.spectral.yaml:36 — the recursive given: $..properties[*] matches any object keyed properties at any depth, not only schema property maps, so a future example/examples value containing a properties object or a data field literally named properties would trip a confusing "must have a description" error on non-schema content; no current false positives exist because all example values are scalars/arrays.
    • Fix: scope the JSONPath to schema contexts (e.g. retain a component path plus a paths-scoped .schema..properties[*] selector) or note the intentional breadth in the adjacent comment.

Reviewers (3): api-contract, testing, maintainability.

Comment thread packages/api/src/routers/external-api/v2/search.ts
Comment thread packages/api/.spectral.yaml
@github-actions

Copy link
Copy Markdown
Contributor

PR Review

3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor

2 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor
  • 🔵 packages/api/.spectral.yaml:46component-schema-property-example left at top-level scope while the description rule went recursive → The two rules now enforce different depths, so a nested string field gets a mandatory description but is silently exempt from the example requirement — openapi.json:3471 (Filter's id, inside an allOf branch) is a type: string property with a description and no example, and it lints clean. Broaden this rule's given the same way as schema-property-description (scoped per the note above) and add the missing examples, or say in the comment that examples are deliberately only required on top-level component properties.

Severity is the reviewer's own estimate and is used for ordering, not filtering.

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 361 passed • 1 skipped • 1440s

Status Count
✅ Passed 361
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant