fix(spec): require an explicit spec source; drop the production default - #5
Merged
Merged
Conversation
`scripts/sync_spec.py` defaulted to https://api.sendly.now/api/openapi.json. Syncing the vendored SDK spec from the deployed API is forbidden by platform policy, which left the only mechanism for keeping tests/fixtures/openapi.json current as one nobody is allowed to run. SENDLY_OPENAPI_URL is now required and has no default. It accepts a filesystem path (the normal case: apps/web/openapi/openapi.json in the platform monorepo), a file:// URL, or an http(s):// URL. The script could not read a local file before -- a bare Windows path parsed its drive letter as a URL scheme, and a file:// URL failed the `status != 200` guard because a file response has no status -- so the committed contract was not merely a non-default source, it was unreachable. - write (default): unconfigured is a hard error naming what to set. - --check (advisory, non-blocking): unconfigured SKIPS with a notice and exits 0. It runs unattended on every PR including forks, which cannot supply a source; failing there would report a configuration gap as spec drift. - ci.yml: the drift step now reads the SENDLY_OPENAPI_URL repository variable instead of reaching for production on every push, PR and weekly cron. - Drive-by: --check no longer dies with UnicodeEncodeError on a legacy Windows console when drift exists -- the diff it exists to print contains non-ASCII. Kept deliberately in step with sendly-js so the two SDKs fail identically for the same missing configuration. Does NOT re-sync the spec -- see the PR description.
…is banned Review follow-up, matching sendly-js commit-for-commit. Two additions, no reversal of the earlier decision not to hard-block production. 1. warn_if_production(): when the resolved source resolves to api.sendly.now, print an unmissable banner to stderr and, under GITHUB_ACTIONS, emit a ::warning annotation. It does NOT refuse -- "what does production actually serve right now?" is a legitimate one-off. Quiet is the property that made the old default dangerous, not the host, so the fix is volume, not a block. Wired into both the write path and --check; the latter matters most, since that is the step that runs unattended in CI. 2. The module docstring now records WHY production is banned, not merely that it is: vendoring the spec from the deployed API makes the SDK mirror what is RUNNING rather than what the repo DECLARES, laundering code-vs-contract drift into "correct" and destroying the vendored spec ability to detect the very drift it exists to catch. Without the reason written down, a future maintainer reads the guardrail as superstition and deletes it. The banner text is byte-identical to sendly-js so the two SDKs warn the same. Verified: banner fires for api.sendly.now and does NOT fire for a local path or a localhost URL; annotation only under GITHUB_ACTIONS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
scripts/sync_spec.pyresolved its source like this:That default is production.
Why production is banned, not merely unwise. The vendored
tests/fixtures/openapi.jsonis a contract snapshot:tests/test_contract.pyasserts the SDK surface against it and never touches the network. Production serves whatever happens to be deployed at the instant you run the script — mid-deploy states, hotfixes, work that has not been reviewed and may be rolled back. A snapshot taken from it is unreproducible (two maintainers running the same command on the same commit can get different files) and unreviewable (the diff cannot be traced to a merged change). The committed contract in the platform monorepo is reviewed, versioned, and diffable; production is a moving target that happens to answer on port 443.The practical consequence was worse than a bad default:
.github/workflows/ci.ymlranpython scripts/sync_spec.py --checkunconfigured on every push tomain, every pull request, and a weekly cron — so this public repo fetched production on a schedule, unattended.urlopen error unknown url type: c(a Windows drive letter parsed as a URL scheme), and afile://URL failed withreturned HTTP None—getattr(response, "status", 200)yieldsNonefor a file response, which thestatus != 200guard rejected. The only source the script could actually consume was anhttp(s)URL, and the only one configured was production.So the mechanism that would keep the spec current was the one nobody was allowed to run, which is why it drifted.
What changed
SENDLY_OPENAPI_URLis now required, with no default, and accepts three forms:/path/to/sendly/apps/web/openapi/openapi.jsonfile://URLfile:///path/to/.../openapi.jsonhttp(s)://URL (local/staging)http://127.0.0.1:8931/openapi.jsonfetch_specbecomesload_spec, branching on scheme;_as_local_pathhandles thefile://case viaurl2pathnameso a URL-shaped habit still works.The two verbs differ deliberately when unconfigured:
--check(advisory, non-blocking) skips with a notice, exit 0. It runs unattended in CI on every pull request including forks, which cannot supply a source; turning that red would report a configuration gap as if it were spec drift.ci.ymlnow passes${{ vars.SENDLY_OPENAPI_URL }}to the drift step, and its annotation no longer hardcodes the production URL in the message. Until a maintainer sets that repository variable to a non-production contract, the drift step and the weekly cron are inert by design — which is the intended state, and better than the previous behaviour of silently probing production. The comments say so.README.mdgains a "Refreshing the vendored OpenAPI spec" section with the correct invocation.Drive-by fix.
--checkprinted its diff withsys.stdout.writelines(difflib.unified_diff(...)). The spec contains non-ASCII (e.g.⇒in descriptions), so on a legacy Windows console codepage every drifted check — precisely the case the command exists for — died with aUnicodeEncodeErrortraceback instead of showing the diff. Now routed through_print_diff, which falls back to a replacing encode. Windows-only; CI runs Linux and was unaffected. Included because it sits in the function this PR rewrites and it is the difference between the legitimate invocation working and not.What is deliberately NOT in this PR
No spec re-sync.
tests/fixtures/openapi.jsonis untouched. This PR fixes the tap, not the tank.The re-sync is queued behind two in-flight platform packages — WP6 (API-key create/rotate, domain setup handoff, the
revealUrlresponse field) and WP9 (mailbox tools). Re-syncing now would close today's gap and immediately open a new one.Also not here, and related: this SDK carries no scope information at all — zero scope strings in
src/, one README mention ofscope_missingas an error code. A user's only way to learn which scopes a method needs is to call it and read the 403. That is a spec-derived fix and it needs a current spec first.For reference, running
--checkagainst the current contract reports the vendored spec is now 8 operations behind (not the 5 recorded earlier — WP6's spec regeneration has since landed): the API-key management routes,GET /api/v1/projects,POST /api/users/me/projects,POST /api/v1/emails, andPOST /api/domains/{id}/dodomain-session.Verification
Run, not inferred from the diff.
Unconfigured — the write path fails loudly:
tests/fixtures/openapi.jsonwas not touched.Unconfigured —
--checkskips, stays green:Configured — the legitimate invocation still works, all three forms:
Configured
--checkagainst the contract correctly reports STALE, exit 1, printing a 3118-line diff with noUnicodeEncodeError(this is the drive-by fix — it crashed here before):All writes were reverted with
git checkout -- tests/fixtures/openapi.json; the committed fixture in this PR is unchanged.Repo gates:
ruff check(all passed),ruff format --check(42 files formatted),mypy srcandmypy scripts/sync_spec.py(no issues),pytest(167 passed).Companion
DevinoSolutions/sendly-jsgets the same change on the same branch name, with the same message text and the same write-fails / check-skips split, so the two SDKs do not fail differently for the same missing configuration.Update: production is loud, not blocked (review follow-up)
Production is not hard-blocked — "what does production actually serve right now?" is a legitimate one-off, and refusing it would break that. What must not happen is it happening quietly, which is exactly how the old default ran unnoticed on every push, every PR and a weekly cron. So the fix is volume:
The banner text is byte-identical to sendly-js. Under
GITHUB_ACTIONSit additionally emits a::warningannotation, so pointing the repository variable at production would surface on the run itself rather than hiding in a log. Wired into both the write path and--check— the latter matters most, since that is the step that runs unattended.The module docstring also now records why production is banned, not merely that it is. A bare guardrail with no stated reason reads as superstition to the next maintainer and gets deleted. The reason: vendoring from the deployed API makes the SDK mirror what is running rather than what the repo declares, so code-vs-contract drift is laundered into "correct" on the way in — and the vendored spec, whose entire job is to be the fixed reference
tests/test_contract.pycompares against, loses the ability to detect the drift it exists to catch.Verified: the banner fires for
api.sendly.now, and does not fire for a local path or alocalhostURL. Both CI runs on this branch show zero production-warning lines and the drift step still skipping. Gates re-run green.