| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
| Back | FazBrowse Home | New Git URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality🟡 The PR description states that a client test was added confirming a network $ref in an output schema is validated without being dereferenced, but that test was deliberately dropped in commit c7522f2 ('Drop the Python no-deref unit test') and the final diff contains only the conformance handler and the two expected-failures baselines. This is a description/documentation discrepancy only — either update the description (including the Verification section) to reflect that coverage now lives solely in the external conformance scenario, or re-add the in-repo test.
Extended reasoning...What the issue is. The PR description's "What changed" section claims: "Added a client test confirming a network $ref in an output schema, under a property the structured content does not set, is validated without being dereferenced." The Verification section likewise reads as if SDK-level test coverage exists. However, the final diff touches only three files — .github/actions/conformance/client.py, expected-failures.yml, and expected-failures.2026-07-28.yml — and contains no test file at all.
How this happened. The branch history shows the test did exist at one point: commit 2c47fc4 ("Guard the no-deref test with a socket tripwire") added/modified a test in tests/client/test_output_schema_validation.py, and the final commit c7522f2 ("Drop the Python no-deref unit test") deliberately removed it, on the rationale that the conformance scenario already proves the no-fetch behavior end to end. The drop was intentional and reasonable — but the PR description was never updated to match.
Step-by-step verification.
Why it matters. Reviewers and future maintainers use the PR description to understand what coverage exists. As written, it overstates the in-repo coverage: the only thing asserting the no-dereference behavior is the external json-schema-ref-no-deref conformance scenario, which lives in the conformance harness rather than the SDK's own test suite. If that scenario is ever reorganized or the harness pin changes, there is no local regression test backing the behavior — and anyone relying on the description would believe there is.
This is not a code defect. All verifiers agreed the test removal itself was a defensible scoping decision (the unit test was a redundant local mirror of the conformance scenario), so this is purely a documentation discrepancy and does not block the PR.
How to fix. Either (a) update the PR description — remove or reword the "Added a client test…" bullet and adjust the Verification section to say the no-deref behavior is covered by the json-schema-ref-no-deref conformance scenario only — or (b) re-add the dropped unit test from 2c47fc4 if maintainers prefer in-repo coverage.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.