[COVAL-5819] Audit CLI request-body fields against the published OpenAPI specs - #122
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…API specs The parity audit mapped operations only, so nothing compared the published request-body properties with the serde fields on the Rust request structs the CLI actually sends. serde drops any field a struct does not declare, so a property the API adds is discarded in silence; COVAL-5805 (script_turns) was one instance of that class. For every covered POST/PATCH/PUT operation the audit now diffs the published application/json request-body properties against the serde field names on the struct the client serializes. A serde_json::Value body counts as full coverage because it forwards caller JSON verbatim and cannot drop a field. Operations with no first-class command are skipped; those are route gaps, already reported. known_field_gap and allowed_extra_field mirror the existing route exception mechanism, including staleness detection, and the snapshot, Markdown report, and console summary carry the new counts. The 80 current gaps and 18 current extras are recorded as reviewed exceptions so the weekly workflow starts failing on new drift immediately; each reason names the follow-up issue that closes it. Two of those exceptions are not CLI work. The run-template singular id fields are documented but not served, so modeling them would break the call (COVAL-5825), and the review-annotation status flags are advertised by the CLI but ignored by the API (COVAL-5824). The specs are fetched once and drive both layers, which also stops the audit aggregation tests reaching the network.
80b0c27 to
e33b10e
Compare
Why
scripts/audit_api_coverage.pymapped operations only —grep -c 'requestBody\|properties'returned 0. Nothing compared the published request-body properties with the serde fields on the Rust request structs the CLI actually sends, and serde silently drops any field a struct does not declare. COVAL-5805 (script_turns) was one instance of a systemic class; this makes the class visible.What the check does
For every covered
POST/PATCH/PUToperation, diff the publishedapplication/jsonrequest-body properties against the serde field names on the struct the client serializes.models::Xparameter, alet req = models::X { … }binding, orserde_json::Value.serde_json::Valuebody counts as full coverage — it forwards caller JSON verbatim and cannot drop a field. That is the metric baseline and threshold endpoints.$refall raise.known_field_gapandallowed_extra_fieldmirror the existingknown_gap/allowed_extramechanism, staleness detection included, and[snapshot], the Markdown report, and the console summary carry the new counts. The weeklyapi-parity-audit.ymlrun now catches field drift as well as route drift.Current state
313/360published request fields on covered operations are modeled. The 47 gaps and 18 extras are recorded as reviewed exceptions so this PR is green and new drift fails from here on. Each reason names the issue that closes it: COVAL-5821 (personas), COVAL-5822 (reviews, reports), COVAL-5823 (agents, test-sets, run-templates, conversations).Rebased onto
mainafter COVAL-5820 (#123) merged. That PR modeled the 33 metric fields this branch had recorded asknown_field_gap, so the audit reported them stale; they are removed here and the count moved from 280 to 313.Two exceptions are not CLI work, and the follow-up PRs must not "fix" them:
POST/PATCH /run-templatesdocumentagent_id,persona_id,test_set_id. The served model takes the plural arrays the CLI already sends and wraps them in a schema that forbids extra fields, so modeling the documented names would break the call. The published spec is wrong.PATCH /review-annotations/{id}sendsstatusandcompletion_status, exposed as--status/--completion-status. The served contract has neither and ignores unknown fields, so both flags report success and change nothing.The caveat this work surfaced
The published YAML is hand-written, not generated, so it can be wrong in either direction.
coval-ai/backendrecords its known spec/model divergence insrc/services/api/tests/v1/openapi_parity_baseline.txt; a field markedspec_extra_fieldthere is documented but not served. Every gap in this PR was cross-checked against the served Pydantic model before being classified, and the README now says to do that.Verification
Patch version bump to 0.8.1, on top of #123's 0.8.0.
Two notes on the test suite:
audit(); patching the new_published_specsseam closes that. The Python suite went from 61s to 0.02s.cargo testfailstest_update_check_notifies_when_outdatedunder parallel execution and passes with--test-threads=1. Reproduced 3/3 on cleanorigin/mainat3331efe, so it predates this branch:src/update_check.rsabandons the check afterWAIT_LIMIT = 500ms, which a parallel run regularly exceeds. Filed as COVAL-5826, not touched here.