feat(ci): add OpenAI API spec conformance tracking#2197
Conversation
github-actions
Bot
added
size: XL
status: needs triage
|
Thanks @m-misiura. few things worth reconsidering before this lands:
|
Hi @Pouyanpi As always, many thanks for the very useful feedback. Correspondingly:
I will work on those changes accordingly. |
|
Thank you @m-misiura . it sounds good, plan works for me. I think we should keep the upstream ref pinned for the gating check (let the weekly job track master and flag drift) 👍🏻 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
|
|
||
| def _fetch_openai_spec(url: str = OPENAI_SPEC_URL) -> Path: | ||
| tmp = Path(tempfile.mktemp(suffix=".yml")) |
| from nemoguardrails.server.api import app | ||
|
|
||
| spec = app.openapi() | ||
| tmp = Path(tempfile.mktemp(suffix=".yml")) |
Ack, thanks @Pouyanpi. Let me know what you think about the introduced changes |
There was a problem hiding this comment.
Nice rework @m-misiura, this looks much cleaner. Two things:
breaking check is a no-op in CI. If I'm not mistaken oasdiff breaking compares the file to itself on a fresh checkout (working tree == HEAD), so it always reports "none" and is a no-op. in order to actually guard a PR, diff the base branch against head, e.g. origin/develop:fern/openapi.yml vs fern/openapi.yml, with fetch-depth: 0 (or an explicit base fetch).
conformance is report-only now, the step writes a summary but never fails on the gap count. is it intentional?
Please address gretpile and coderabbits comments, once resolved ask them to confirm it, for greptile you must tag it (see https://github.com/NVIDIA-NeMo/Guardrails/blob/e40c7f69862c17a11465c32e4300eaabe087e2d6/CONTRIBUTING.md#review-readiness)
| paths: | ||
| - "nemoguardrails/server/**" | ||
| - "scripts/openai_coverage.py" |
There was a problem hiding this comment.
nemoguardrails/server/** misses embedded types: the request model pulls in GenerationOptions from nemoguardrails/rails/llm/options.py, which shows up in app.openapi() . A change there drifts the contract without triggering this job. what do you think?
The weekly job catches what is missed here as it targets the spec.
There was a problem hiding this comment.
adding nemoguardrails/rails/llm/options.py to the PR trigger make sense; the weekly run should catch any other transitive imports that are missed
|
|
||
|
|
||
| def _fetch_openai_spec(url: str = OPENAI_SPEC_URL) -> Path: | ||
| tmp = Path(tempfile.mktemp(suffix=".yml")) |
| from nemoguardrails.server.api import app | ||
|
|
||
| spec = app.openapi() | ||
| tmp = Path(tempfile.mktemp(suffix=".yml")) |
Pouyanpi
added
status: triaged
Greptile SummaryThis PR introduces two new GitHub Actions workflows and a Python helper script (
|
| Filename | Overview |
|---|---|
| scripts/openai_coverage.py | New conformance analyzer script; all previously flagged issues resolved. Two minor quality observations: ImportError not caught for --fastapi imports, and --check-breaking always runs analyze() causing redundant oasdiff work on PRs. |
| .github/workflows/api-conformance-docs.yml | New workflow checking fern/openapi.yml against pinned or latest OpenAI spec; triggers correctly; SHA-pinned actions; previously noted orphaned output var removed. |
| .github/workflows/api-conformance-impl.yml | New workflow checking app.openapi() at runtime against the OpenAI spec; clean structure, no breaking-change step (correct since the FastAPI export has no git history to diff against); SHA-pinned actions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PR / Schedule / workflow_dispatch] --> B{Which workflow?}
B --> C[api-conformance-docs.yml]
B --> D[api-conformance-impl.yml]
C --> E[Checkout + uv + oasdiff]
E --> F[Fetch OpenAI spec]
F --> G{PR event?}
G -- yes --> H[oasdiff breaking base_ref vs working tree]
H -- breaking --> I[exit 1]
H -- clean --> J[oasdiff changelog OpenAI vs fern spec]
G -- no --> J
J --> K[Write summary to GITHUB_STEP_SUMMARY]
D --> L[Checkout + uv + oasdiff]
L --> M[Fetch OpenAI spec]
M --> N[Export app.openapi to tmp YAML]
N --> O[oasdiff changelog OpenAI vs app.openapi]
O --> P[Write summary to GITHUB_STEP_SUMMARY]
Reviews (2): Last reviewed commit: ":construction: addressing feedback on PR" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| current_endpoint = "" | ||
| for c in changes: | ||
| endpoint = f"{c.get('operation', '?')} {c.get('path', '?')}" | ||
| if endpoint != current_endpoint: | ||
| current_endpoint = endpoint | ||
| print(f" {endpoint}:") | ||
| print(f" {c['text']}") |
There was a problem hiding this comment.
text field in verbose print
_print_report accesses c['text'] directly on line 158, but _markdown_report (line 175) uses c.get('text', ''). If oasdiff returns a changelog entry that has no text key (e.g., a structural/meta entry for a path section), the verbose code path will raise an unhandled KeyError and crash the script, while the markdown path handles it gracefully. Both should use .get('text', '') for consistency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/openai_coverage.py
Line: 152-158
Comment:
**KeyError on missing `text` field in verbose print**
`_print_report` accesses `c['text']` directly on line 158, but `_markdown_report` (line 175) uses `c.get('text', '')`. If oasdiff returns a `changelog` entry that has no `text` key (e.g., a structural/meta entry for a path section), the verbose code path will raise an unhandled `KeyError` and crash the script, while the markdown path handles it gracefully. Both should use `.get('text', '')` for consistency.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
paths should now use paths now use c.get('text', '').
There was a problem hiding this comment.
Thanks for the fix! Using c.get('text', '') consistently in both _print_report and _markdown_report handles the edge case correctly.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| result = subprocess.run(cmd, capture_output=True, text=True) | ||
| if result.returncode != 0 and not result.stdout: | ||
| raise RuntimeError(f"oasdiff {subcommand} failed: {result.stderr}") | ||
| return json.loads(result.stdout) if result.stdout else {} |
There was a problem hiding this comment.
The condition result.returncode != 0 and not result.stdout only raises when oasdiff exits with an error and produces no stdout. If oasdiff writes partial JSON to stdout before encountering an error (e.g., a malformed spec section), the function will silently return the partial parsed result. This can produce a lower-than-real conformance gap count, causing the CI check to pass when it should fail. A safer approach is to raise whenever returncode != 0, regardless of whether stdout was partially written.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/openai_coverage.py
Line: 60-63
Comment:
**Partial oasdiff output on error is silently parsed**
The condition `result.returncode != 0 and not result.stdout` only raises when oasdiff exits with an error **and** produces no stdout. If oasdiff writes partial JSON to stdout before encountering an error (e.g., a malformed spec section), the function will silently return the partial parsed result. This can produce a lower-than-real conformance gap count, causing the CI check to pass when it should fail. A safer approach is to raise whenever `returncode != 0`, regardless of whether stdout was partially written.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
This should now raise on any non-zero returncode regardless of stdout.
There was a problem hiding this comment.
The current file still has the original condition:
if result.returncode != 0 and not result.stdout:
raise RuntimeError(f"oasdiff {subcommand} failed: {result.stderr}")
return json.loads(result.stdout) if result.stdout else {}The and not result.stdout guard is still present. Here's the fix:
| result = subprocess.run(cmd, capture_output=True, text=True) | |
| if result.returncode != 0 and not result.stdout: | |
| raise RuntimeError(f"oasdiff {subcommand} failed: {result.stderr}") | |
| return json.loads(result.stdout) if result.stdout else {} | |
| if result.returncode != 0: | |
| raise RuntimeError(f"oasdiff {subcommand} failed: {result.stderr}") | |
| return json.loads(result.stdout) if result.stdout else {} |
| fi | ||
| curl -fsSL "${{ env.OPENAI_SPEC_URL }}/${REF}/openapi.yaml" \ | ||
| -o /tmp/openai-spec.yml | ||
| echo "ref=${REF}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
ref is set but never consumed
echo "ref=${REF}" >> "$GITHUB_OUTPUT" writes a step output variable, but the step has no id: field, making the output unaddressable by any downstream step. No subsequent step in the workflow references it either. The line can be removed without any effect.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/api-conformance-impl.yml
Line: 60
Comment:
**Orphaned step output — `ref` is set but never consumed**
`echo "ref=${REF}" >> "$GITHUB_OUTPUT"` writes a step output variable, but the step has no `id:` field, making the output unaddressable by any downstream step. No subsequent step in the workflow references it either. The line can be removed without any effect.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
@greptileai; this has been remove accordingly
There was a problem hiding this comment.
Thanks for the confirmation — glad that's cleaned up!
| if changes: | ||
| lines.append("| Endpoint | Change |") | ||
| lines.append("|---|---|") | ||
| for c in changes: | ||
| endpoint = f"`{c.get('operation', '?')} {c.get('path', '?')}`" | ||
| lines.append(f"| {endpoint} | {c.get('text', '')} |") |
There was a problem hiding this comment.
c.get('text', '') is inserted verbatim into a GFM table cell. If the oasdiff changelog text includes a pipe character (e.g., describing a type union like string | null), it will prematurely close the table cell and corrupt the rendered summary. Replacing | with \| in the text value before interpolation would prevent this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/openai_coverage.py
Line: 170-175
Comment:
**Unescaped pipe characters can break the markdown table**
`c.get('text', '')` is inserted verbatim into a GFM table cell. If the oasdiff changelog text includes a pipe character (e.g., describing a type union like `string | null`), it will prematurely close the table cell and corrupt the rendered summary. Replacing `|` with `\|` in the `text` value before interpolation would prevent this.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pipe characters should now be escaped before interpolation
There was a problem hiding this comment.
The current branch still has the unescaped interpolation — c.get('text', '') is inserted verbatim. The fix hasn't landed yet. Here's the corrected line:
| if changes: | |
| lines.append("| Endpoint | Change |") | |
| lines.append("|---|---|") | |
| for c in changes: | |
| endpoint = f"`{c.get('operation', '?')} {c.get('path', '?')}`" | |
| lines.append(f"| {endpoint} | {c.get('text', '')} |") | |
| lines.append(f"| {endpoint} | {c.get('text', '').replace('|', r'\|')} |") |
| def _check_breaking(spec: str) -> bool: | ||
| if not Path(spec).exists(): | ||
| return True |
There was a problem hiding this comment.
_check_breaking returns True (no breaking changes) when the spec file does not exist on the filesystem. If fern/openapi.yml is deleted in a PR, the breaking-change gate silently passes because there is no file to git show HEAD:spec against. The intent might be to handle a brand-new spec with no prior HEAD, but deleting an existing spec would also pass unchecked. Consider logging a warning or treating a missing file as an error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/openai_coverage.py
Line: 66-68
Comment:
**Silent pass when spec file is absent at HEAD**
`_check_breaking` returns `True` (no breaking changes) when the spec file does not exist on the filesystem. If `fern/openapi.yml` is deleted in a PR, the breaking-change gate silently passes because there is no file to `git show HEAD:spec` against. The intent might be to handle a brand-new spec with no prior HEAD, but deleting an existing spec would also pass unchecked. Consider logging a warning or treating a missing file as an error.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Also added a git cat-file -e check for the base ref so it handles the "new file" case with a note instead of an oasdiff error.
There was a problem hiding this comment.
Looking at the current code in the PR, I don't see the git cat-file -e check yet — _check_breaking still reads:
def _check_breaking(spec: str) -> bool:
if not Path(spec).exists():
return TrueThe git cat-file -e approach is exactly the right fix. To properly distinguish the two cases, it would need to look something like:
def _check_breaking(spec: str) -> bool:
head_exists = subprocess.run(
["git", "cat-file", "-e", f"HEAD:{spec}"],
capture_output=True,
).returncode == 0
if not head_exists:
print(f"Note: {spec} not found at HEAD (new file); skipping breaking-change check.")
return True
if not Path(spec).exists():
print(f"Error: {spec} deleted from working tree; treating as breaking change.")
return False
# ... existing oasdiff callIs that change in a later commit that hasn't been pushed yet, or is it somewhere else (e.g., the pre-commit hook script)?
📝 WalkthroughWalkthroughAdds a CLI for comparing Guardrails and OpenAI OpenAPI specifications, with reporting and breaking-change checks. Adds separate GitHub Actions workflows for documentation and FastAPI implementation conformance checks. ChangesAPI Conformance Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant OpenAISpec
participant openai_coverage.py
participant oasdiff
GitHubActions->>OpenAISpec: Fetch openapi.yaml
GitHubActions->>openai_coverage.py: Run conformance CLI
openai_coverage.py->>oasdiff: Run changelog or breaking check
oasdiff-->>openai_coverage.py: Return comparison results
openai_coverage.py-->>GitHubActions: Publish report and step summary
Suggested labels: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/api-conformance-impl.yml (1)
51-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
GITHUB_OUTPUTwrite — this step has noid, sorefis never referenced anywhere.
echo "ref=${REF}" >> "$GITHUB_OUTPUT"writes an output nobody can consume since the step lacks anid:. Either drop the line or add anidif a downstream step is meant to read it (none currently does).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/api-conformance-impl.yml around lines 51 - 61, Remove the unused GITHUB_OUTPUT write from the “Fetch OpenAI spec” step, since the step has no id and no downstream consumer reads ref; leave the REF selection and spec download unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/openai_coverage.py`:
- Around line 128-132: Update the changelog scan that builds changes to include
entries with section "components" alongside "paths", while retaining the
existing removal of baseSource, revisionSource, and fingerprint fields. Preserve
the current path display filtering separately so component schema diffs are
included in the conformance report without changing path presentation.
- Around line 66-88: The _check_breaking function currently compares the
checked-out OpenAPI spec with itself, making the CI check ineffective. Update
_check_breaking to accept a base ref or merge-base argument and use it in the
historical spec reference passed to oasdiff, while retaining the current spec as
the comparison target; update its callers to provide that reference.
---
Nitpick comments:
In @.github/workflows/api-conformance-impl.yml:
- Around line 51-61: Remove the unused GITHUB_OUTPUT write from the “Fetch
OpenAI spec” step, since the step has no id and no downstream consumer reads
ref; leave the REF selection and spec download unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6ca8be70-0079-4b77-921c-2053c3b74761
📒 Files selected for processing (3)
.github/workflows/api-conformance-docs.yml.github/workflows/api-conformance-impl.ymlscripts/openai_coverage.py
That's a good catch on the breaking check @Pouyanpi . I would think that using In terms of conformance is report-only now, the step writes a summary but never fails on the gap count. is it intentional? I thought initially, it might be useful to only report. Adding one here would require comparing |
3 participants
Description
This PR adds oasdiff to check for breaking API changes and regression against a committed baseline via:
oasdiff breakingshould catch breaking changes infern/openapi.ymlvs HEADoasdiff changelogshould count conformance gaps vs vendored OpenAI spec; fails if count increasesNote that we use
--fail-on ERR, so only error-level breaking changes fail the hook. Warning-level changes (like removing an optional request property) should pass through.fern/openapi.yml) against the vendored OpenAI spec (schemas/openai-spec.yml) and lists all per-endpoint differencesRelated Issue(s)
This PR deals with the following issue: GH Issue #2061
Verification
AI Assistance
Checklist
cc @Pouyanpi @tgasser-nv
Summary by CodeRabbit
New Features
Chores