fix(backend): name the surface in the OpenAPI regenerate hint#10455
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for tightening this hint — including the explicit --surface is the right direction and the new regression coverage captures the damaging non-public default case.
One blocking issue before this can merge: the PR changes check_spec to require a keyword-only surface, but an existing unit test/call contract still invokes it with the old signature. CI is failing in tests/unit/test_openapi_contract.py::test_check_spec_detects_stale_file with:
TypeError: check_spec() missing 1 required keyword-only argument: 'surface'
Please either keep check_spec backward-compatible with a safe default such as surface='public', or update the existing test/call sites in the same PR so the backend unit suite is green. The core fix looks sound once that compatibility/test failure is resolved.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
…ce default Review (BasedHardware#10455): making `surface` keyword-only-required broke an existing caller — tests/unit/test_openapi_contract.py::test_check_spec_detects_stale_file called check_spec(path, generated) and hit `TypeError: check_spec() missing 1 required keyword-only argument: 'surface'`. Default surface='public', matching the --surface argparse default. The production caller in this module still passes surface=args.surface explicitly, so a non-public surface never silently gets the public hint (the BasedHardware#10217 defect). Added a regression test pinning the no-surface call. Verify (.openapi-venv): check_spec(path, generated) now raises the stale error with a `--surface public` hint; surface='app-client' still names it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
07f3e11 to
bc027ed
Compare
|
Fixed — The remaining |
kodjima33
left a comment
There was a problem hiding this comment.
fix(backend) name surface in OpenAPI regenerate hint — approve only (workflow-review, CHANGES_REQUESTED)
The stale/missing contract errors printed `export_openapi.py --write <path>` with no surface. `--surface` defaults to `public`, so following that hint verbatim for a non-public contract writes the PUBLIC surface into it — the app-client spec loses ~53k lines and the "fix" silently guts the file instead of refreshing it (BasedHardware#10217). Thread the surface into check_spec and build both messages from one regenerate_hint helper, so the printed command is exactly the command that regenerates that path. Refs BasedHardware#10217. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ce default Review (BasedHardware#10455): making `surface` keyword-only-required broke an existing caller — tests/unit/test_openapi_contract.py::test_check_spec_detects_stale_file called check_spec(path, generated) and hit `TypeError: check_spec() missing 1 required keyword-only argument: 'surface'`. Default surface='public', matching the --surface argparse default. The production caller in this module still passes surface=args.surface explicitly, so a non-public surface never silently gets the public hint (the BasedHardware#10217 defect). Added a regression test pinning the no-surface call. Verify (.openapi-venv): check_spec(path, generated) now raises the stale error with a `--surface public` hint; surface='app-client' still names it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bc027ed to
2f6a56c
Compare
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-reviewed the current head and the earlier blocker is resolved: check_spec is backward-compatible again with surface='public', while the CLI check path now passes the parsed surface explicitly so non-public OpenAPI contracts get a safe regenerate hint.
I also ran the targeted backend coverage locally:
.venv/bin/python -m pytest tests/unit/test_export_openapi_hint.py tests/unit/test_openapi_contract.py::test_check_spec_detects_stale_file -q
9 passed in 0.33s
This looks like the right fix for #10217, with regression coverage for the app-client/public-surface failure mode. Because this touches the backend OpenAPI contract-generation workflow, I’m leaving it as a positive maintainer signal rather than a formal approval; a human maintainer should still do the final workflow/API-contract signoff before merge.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Superseded by 2f6a56c: check_spec now defaults surface="public", the production caller passes surface explicitly, and targeted backend tests pass.
What
The OpenAPI contract checker tells you how to fix a stale contract, and the command it prints is wrong for every non-public surface.
Fixes #10217.
Why it matters
check_specprinted:--surfacedefaults topublic. So running that hint verbatim against the app-client contract exports the public surface into the app-client file — it doesn't refresh the contract, it guts it (~53k lines). The hint actively causes the damage it claims to repair.I hit this myself regenerating contracts today: you have to already know to pass
--surface app-client, which is precisely what the error message exists to tell you.Fix
Thread the surface into
check_specand build both the missing- and stale-file messages from oneregenerate_hinthelper, so the printed command is exactly the command that regenerates that path:Verified live by making the app-client contract stale and running the checker (contract restored afterwards, not part of the diff):
Tests
backend/tests/unit/test_export_openapi_hint.py— 7 cases:--write <path>form that defaults to publicNote on prior attempts
#10255 and #10294 both targeted this and were closed unmerged, so the issue is still live on
main— I verified the surface-less hint is still there before writing anything.Product invariants affected
none
Failure-Class: none