fix(spec): require an explicit spec source; drop the production default - #7
Merged
Merged
Conversation
`pnpm sync-spec` and `pnpm check-spec-drift` both 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 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. Neither script could read a local file before -- Node's fetch() rejects non-http(s) schemes -- so the committed contract was not merely a non-default source, it was unreachable. - scripts/spec-source.mjs: shared resolver, so the two scripts cannot disagree about the source or report a missing one differently. - sync-spec (writes): unconfigured is a hard error naming what to set. - check-spec-drift (advisory, non-blocking): unconfigured SKIPS with a notice. 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. Does NOT re-sync the spec or regenerate types -- see the PR description.
…is banned Review follow-up. Two additions, no reversal of the earlier decision not to hard-block production. 1. warnIfProduction(): 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 sync-spec and check-spec-drift; the latter matters most, since that is the step that runs unattended in CI. 2. The scripts now record 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's 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. 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
pnpm sync-specandpnpm check-spec-driftboth resolved their source like this:That default is production.
Why production is banned, not merely unwise. The vendored
openapi.jsonis a contract snapshot: it is the input topnpm build:types, and the thingsrc/__tests__/contract.test.tsasserts the SDK surface against. 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.ymlranpnpm check-spec-driftunconfigured on every push tomain, every pull request, and a weekly cron — so this public repo fetched production on a schedule, unattended.fetch()rejects non-http(s)schemes, soSENDLY_OPENAPI_URLpointing at a local file — bare path orfile://— died with an uncaughtTypeError: fetch failedand a raw stack trace. The only source either 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.jsonNew
scripts/spec-source.mjsholds the resolver and the unconfigured message, so the two scripts cannot drift apart on where the spec comes from or how they report a missing source.The two scripts differ deliberately when unconfigured:
sync-spec(writes) fails hard, exit 1, printing what to set. It is the mechanism this guardrail is about.check-spec-drift(advisory, never fails) 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. 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 "Refreshingopenapi.json" section with the correct invocation.What is deliberately NOT in this PR
No spec re-sync and no regenerated types.
openapi.jsonandsrc/types.generated.tsare 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.That also means the known scope bug stays open a little longer, on purpose:
src/types.generated.tsdocumentsPOST /api/v1/campaigns/{id}/sendas "Requires thecampaigns:writescope", but the route has enforcedcampaigns:sendsince the scope split. A customer who mints a narrow key from the SDK's own JSDoc gets a 403. Legacy full-access keys are grandfathered, so this bites only users who did the careful thing. It is fixed by regenerating from a current contract — which this PR makes possible and a follow-up will do.For reference, running the drift check against the current contract reports the SDK spec is now 8 operations behind (not the 5 recorded earlier — WP6's spec regeneration has since landed):
Verification
Run, not inferred from the diff.
Unconfigured —
sync-specfails loudly:openapi.jsonwas not touched.Unconfigured —
check-spec-driftskips, stays green:Configured — the legitimate invocation still works, all three forms:
All three were reverted with
git checkout -- openapi.json; the committed spec in this PR is unchanged.Repo gates:
pnpm format:check(clean),pnpm lint(no issues),pnpm check-types(clean),pnpm test(18 files, 140 tests passed).Companion
DevinoSolutions/sendly-pythongets 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:
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 bothsync-specandcheck-spec-drift— the latter matters most, since that is the step that runs unattended.The scripts also now record 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
contract.test.tscompares 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.