conformance: a path-less requestBody assertion pins the whole body - #854
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
An empty path remains schema-valid but has conflicting semantics across the six runners.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements path-less requestBody assertions as exact whole-body comparisons across all six SDK conformance runners and tightens path requirements for other assertions.
Changes:
- Adds whole-body request assertions to every runner.
- Updates schema and SPEC semantics.
- Adds a shared conformance fixture.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
SPEC.md |
Documents assertion semantics. |
conformance/schema.json |
Adds conditional path requirements. |
conformance/tests/schedule_entries_write.json |
Exercises whole-body equality. |
conformance/runner/go/main.go |
Implements Go handling. |
conformance/runner/python/runner.py |
Implements Python handling. |
conformance/runner/ruby/runner.rb |
Implements Ruby handling. |
conformance/runner/swift/Sources/ConformanceRunner/Assertions.swift |
Implements Swift handling. |
conformance/runner/typescript/runner.test.ts |
Implements TypeScript handling. |
kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt |
Implements Kotlin handling. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32f61514b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Converged: CI green on a2ae17f (41 checks), the one review finding (empty path) fixed in the schema and answered, and no further review activity since. Ready for a human look and merge. |
a2ae17f to
bbff957
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbff9574d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
conformance/schema.json declared path optional on requestBody while all six runners dereferenced it, so a schema-valid fixture crashed or misreported per language. The path-less form now means whole-body exact equality in every runner, and the schema requires path on the other path-taking assertion types, where no path-less form has a meaning. Fixes #587
…path-less form, Python compares at the JSON level, the schema requires expected
The Rust SDK arrived after this branch. Its conformance runner read an
omitted path as a key named the empty string, and its fixture-driven
composites harness (tests/composites_support) required one outright, so
the new schedule_entries_write assertion failed there alone; both take the
whole-body form now. Python's native equality made {"highlighted": false}
equal to {"highlighted": 0}, which every other runner rejects; both
request-body forms now compare through _json_equal. And a requestBody
assertion with no path had nothing left to require, so
{"type": "requestBody"} was schema-valid and failed differently per
runner; the schema now requires expected on it.
bbff957 to
a6e6d46
Compare
|
Rebased onto main at 95b83dd (#874) — a6e6d46, no content change. CI is green on that head except CodeQL |
Summary
conformance/schema.jsondeclaredpathoptional on arequestBodyassertion, and no runner implemented the path-less form: each of the six dereferencedpathunconditionally, so a schema-valid fixture produced a different crash or a misleading "key not present" per language.This takes the first of the issue's two options. A
requestBodyassertion with nopathnow means the whole captured body equalsexpected, exactly — a strictly stronger pin than the per-key form, because a key the SDK adds on its own is a failure instead of an unnoticed extra. All six runners implement it, andconformance/schema.jsonplus SPEC §19 document both forms.The other half of the class is closed by the schema rather than by six more branches: every other path-taking assertion type (
errorField,headerAbsent,headerInjected,headerPresent,headerValue,requestBodyAbsent,responseBody,responseMeta) has no meaning without a path, so anallOf/if/thennow requires it for those. A path-less fixture of that shape failsmake conformance-fixtures-checkwith a message naming the field instead of reaching a runner. A census ofconformance/tests/*.jsonshows the split is already clean: those eight always carrypathtoday, and no other type ever does.Red-proof
The fixture landed first.
schedule_entries_write.json"replace-omission-clears" — whose description already says the raw replace sends exactly what the caller passed — gained a path-lessrequestBodypinning the whole three-key body. Against the unmodified runners:Expected request body field "" on request index 0, but it was absentKeyError: 'path'runner.rb:1311:in 'TestRunner#fetch_body_key': undefined method 'split' for nil (NoMethodError)TypeError: Cannot read properties of undefined (reading 'split')atlookupBodyPathnavigateJsonPath(body, "")/body.navigate("")look up the empty key and report "key not present in request body"With the change, all six pass it, and the existing per-key and
requestBodyAbsentassertions on the same case stay as they were.Runner semantics
Shared across the six: no path → whole-body exact equality; a request with no JSON body fails; the failure message names the full expected and actual bodies. Each runner compares with the same helper it already used for the per-key form (
jsonEqual,toStrictEqual,==,compareJsonValues,compareJSON), so number and nesting handling is unchanged.Fixes #587
Summary by cubic
Fixes #587 by implementing the path-less
requestBodyassertion thatconformance/schema.jsonallowed but no runner handled. ArequestBodyassertion withoutpathnow means the whole captured body must equalexpectedexactly, and the other eight path-taking assertion types now requirepathin the schema.Details
_json_equalsofalseand0are distinct, matching the other runners' wire-type strictness. A request with no JSON body fails in either form.make conformance-fixtures-checkwith a message naming the field, and also requiresexpectedonrequestBody, which previously was schema-valid with nothing to say.pathstring: three runners treat""as absent and four as a lookup of the empty key, sominLength: 1keeps that disagreement out of any schema-valid fixture.schedule_entries_write.jsongained a path-lessrequestBodypinning the whole three-key body, which previously failed on every runner with different errors, and the Rust SDK's composites harness now takes the same form.Written for commit a6e6d46. Summary will update on new commits.