conformance: a path-less requestBody assertion pins the whole body - #854
conformance: a path-less requestBody assertion pins the whole body#854jeremy wants to merge 2 commits into
Conversation
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
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.
| "path": { | ||
| "type": "string", | ||
| "description": "JSONPath or dot-notation path for nested value assertions. For requestBody / requestBodyAbsent, the key (dot-notation) inside the captured request body." | ||
| "description": "JSONPath or dot-notation path for nested value assertions. For requestBody / requestBodyAbsent, the key (dot-notation) inside the captured request body. Optional for requestBody alone: omitted, `expected` is the WHOLE captured body and must equal it exactly, so a key the SDK added fails rather than passing unseen. Every other path-taking type requires it (the allOf below), so a path-less fixture is rejected here instead of reaching a runner that cannot execute it." |
There was a problem hiding this comment.
Fixed in a2ae17f: path carries minLength 1 in the schema, so an empty string is rejected at conformance-fixtures-check like a missing one, and the three-versus-three disagreement cannot reach a schema-valid fixture. The runners are unchanged; the schema is the layer that can see the value.
![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".
| "type": "string", | ||
| "description": "JSONPath or dot-notation path for nested value assertions. For requestBody / requestBodyAbsent, the key (dot-notation) inside the captured request body." | ||
| "description": "JSONPath or dot-notation path for nested value assertions. For requestBody / requestBodyAbsent, the key (dot-notation) inside the captured request body. Optional for requestBody alone: omitted, `expected` is the WHOLE captured body and must equal it exactly, so a key the SDK added fails rather than passing unseen. Every other path-taking type requires it (the allOf below), so a path-less fixture is rejected here instead of reaching a runner that cannot execute it." |
There was a problem hiding this comment.
Because this schema still accepts "path": "", a valid shared fixture can now have different meanings across runners: Go, Kotlin, and Swift treat the empty string like an omitted path and compare the whole body, while TypeScript, Python, and Ruby pass it to their path walkers and look for a key named "". Require a non-empty string here so omission remains the only whole-body form and every schema-valid fixture has consistent semantics.
Useful? React with 👍 / 👎.
| elif body_path is None: | ||
| if request["body"] != expected: | ||
| failures.append(f"Expected request body on request index {idx} to equal {expected!r} exactly, got {request['body']!r}") |
There was a problem hiding this comment.
For a path-less assertion containing a boolean, Python's native container equality can produce a false pass when the SDK emits a number: for example, {"highlighted": false} compares equal to {"highlighted": 0} because False == 0 in Python. The other runners reject that wire-type mismatch, so this new whole-body assertion is not exact or cross-SDK consistent unless the Python branch performs type-aware recursive JSON comparison.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| }, | ||
| "then": { "required": ["path"] } |
There was a problem hiding this comment.
A path-less requestBody assertion also needs expected, but the new conditional only requires paths for the other assertion types, so {"type":"requestBody"} still passes conformance-fixtures-check. It then raises KeyError: 'expected' in Python while the other runners report differing assertion failures, leaving the same schema-valid, cross-runner failure class this change is intended to eliminate; require expected when the type is requestBody (and distinguish an omitted property from an explicit JSON null).
Useful? React with 👍 / 👎.
|
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. |
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
make conformance-fixtures-checkwith a message naming the field, and a census of existing fixtures shows none omitpathon those types.pathstring: three runners treat""as absent and three 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 all six runners with different errors.Written for commit a2ae17f. Summary will update on new commits.