P2: Declare 1.0.0 + versioning & schema stability policy (#178) - #198
Conversation
|
Thanks for pinning this down — a written stability contract is exactly what public consumers (the mina-explorer, o1js) need before 1.0, and the graphql-inspector "Check Schema" tie-in is a great enforcement hook. One high-value addition: the policy defines what is breaking and how to deprecate, but doesn't codify the practice this repo already relies on to protect existing clients — shipping backward-incompatible query/response changes OFF by default behind an env flag. We already do this with
Two small doc nits while you're in here:
|
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @SanabriaRusso — both done in Flag-gating policy. Added as its own section, close to your wording. I kept your reasoning about why rather than just the rule, because it's the part that makes it stick: the schema checker structurally cannot catch this. An unflagged change to a default response isn't schema-breaking — nothing errors, "Check Schema" stays green, health checks stay green, and the Explorer just blanks pages. A convention that only holds when someone remembers it is exactly the thing worth writing down. Verified both cited flags exist as described before codifying them: Nullability direction. Fixed, and split by position rather than patched, since the two are genuine mirror images:
The old single line was right for arguments and backwards for fields. Since this doc's whole job is to define "breaking" precisely, stating one rule and applying it to both positions was the bug. On |
|
Verdict: MERGEABLE ✅ Docs + a one-line version bump; no runtime code, no schema change ( What I checked
Mechanical prerequisite (must be done before merge)The PR is against a stale base — its // package-lock.json — two places
"version": "1.0.0", // line 3 (top level)
"version": "1.0.0", // packages[""].versionSimplest is The 1.0.0 / registry mismatch is worth a note in the PR bodyThree states diverge and the PR body's "merging this PR is the decision to declare 1.0" doesn't reconcile them:
Consequences worth calling out:
Recommended addition: name error-message text as part of the stable surfaceThis is the highest-value change to the doc. The policy covers types/fields/arguments, which ## Error messages and validation behaviour
GraphQL **validation and parse errors are part of the public contract.** Clients use
them for capability detection: they probe for a field or filter and fall back based on
the error text.
Covered by this policy:
- Validation and parse errors MUST be returned in `errors[]` of the response body with
their verbatim `graphql-js` wording — notably `Cannot query field "X" on type "Y".`,
`Unknown argument "X" on field "Y".`, `Unknown type "X".`, and unknown-input-field
errors that name the field (e.g. `inBestChain`).
- `errors[]` MUST still be present in the body when the HTTP status is non-2xx; clients
parse the body regardless of status code.
- Error masking (graphql-yoga `maskedErrors`) applies to **unexpected thrown runtime
errors only**. Widening it to cover validation or parse errors — or replacing their
text with a generic string, an error code, or a redacted message — is **breaking
(major)**.
Known consumers matching on this text today: [mina-explorer]
(`src/services/api/bestChainFilter.ts` — `message.includes('inBestChain')`) and
mina-explorer-api (`app/upstream/graphql.py` — `"Cannot query field"`,
`"Unknown argument"`, `"Unknown type"`; `app/upstream/archive.py` — `"inBestChain"`).
As with flag-gating, breaking this doesn't fail loudly: the schema checker stays green,
health checks stay green, and the Explorer serves empty views.Two more categories the "breaking changes" list is missingBoth are things the very next release actually does, so they're not hypothetical. Add under "Operational contract": - Raising the minimum supported Node.js runtime — the `engines` range, the Docker base
image, or the Node version CI publishes against — is **major**. It can make the
published package uninstallable, or the image unrunnable, for a consumer on the
previous LTS. (#194 moves the repo to Node 22; note that `package.json` declares no
`engines` field today, only `volta.node` — adding one is worth doing at 1.0 so the
constraint is machine-checkable rather than implied.)
- Enabling by default any behaviour that can reject, throttle, or block a
previously-accepted request — rate limiting, request-size caps, query-cost limits, an
origin allowlist — is **major**, whether or not the schema changes. (#184, #185.)Non-blocking nits
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
40a2a0d to
57e398d
Compare
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
57e398d to
f1ef5bf
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Every round-1 finding is addressed, and the two carry-forward items are the ones that matter most downstream. Approving.
Verified fixed
- Release-state claim. I re-checked live rather than trusting the doc: the package is
@o1-labs/mina-archive-node-graphql;npm view … versionsreturns only0.0.6,dist-tagsis{ latest: '0.0.6' }, and issue #208 is still open.f1ef5bfccorrectly makes both statements conditional ("once npm trusted publishing is configured"), and theMigrating from npm 0.0.6section accurately records that 0.0.7–0.0.9 were tagged but never published. Nothing untrue remains. - Version bump is internally consistent.
package.json0.0.9 → 1.0.0 against main's actual 0.0.9 (round 1 saw it diffing from a stale 0.0.6), andpackage-lock.jsonupdated in both places — top-level"version"andpackages[""].version. - Release procedure is coherent with the repo sitting at 1.0.0 — the "initial
1.0.0release only" carve-out replacesnpm versionwith a direct tag, which resolves round 1's self-contradiction. - "Error messages and validation behaviour" is exactly the section that was missing, and it is the strongest part of this PR. Pinning verbatim
graphql-jswording and declaring that widening masking over validation/parse errors is major turns both consumers' capability detection into a real contract instead of a lucky accident. - Both missing breaking-change categories are enumerated — minimum Node runtime (covering
engines, the base image, and CI's publishing Node, which is the one people forget) and default-on request-rejecting behaviour (rate limiting, size caps, cost limits, stricter CORS). Those are precisely what #194 / #184 / #185 do next. #194 now shipsengines: { "node": ">=22.12.0" }, so that category is immediately concrete. - Nullability direction is right in both cases: output
String!→Stringbreaking, inputString→String!breaking, with the mirror-image rationale spelled out.
One addition worth making to the error-text section
The section names Cannot query field. Verifying against the consumers, the contract is actually three literal strings, all equally load-bearing. mina-explorer-api matches on all three (app/upstream/graphql.py:33-42):
SCHEMA_ERROR_MARKERS: tuple[str, ...] = (
"Cannot query field",
"Unknown argument",
"Unknown type",
)A match on any of them classifies the response as UpstreamSchemaError, which drives tier fallback and poisons that consumer's capability cache — so a change to Unknown argument or Unknown type wording is exactly as breaking as one to Cannot query field, and the failure is sticky rather than per-request. Worth naming all three explicitly so a future reader does not assume only the first is protected. (mina-explorer keys on Cannot query field alone — transactions.ts:488,556,698,977, ZkAppsPage.tsx:183.)
Ordering: round 1's "#208 must land before #198" constraint no longer applies to the merge — the doc's caveat is now truthful, so this can go in. It moves to the release: #208 must be resolved before v1.0.0 is pushed, or the publish job fails ENEEDAUTH exactly as it did for v0.0.9. The Docker half of the pipeline is fine — GHCR has images through 0.0.9 and publishes on refs/tags/v*.
Non-blocking nits
-
The initial-release command does not work as written.
docs/versioning.md:git tag v1.0.0 && git push --follow-tagsgit tagwithout-a/-screates a lightweight tag, andgit push --follow-tagspushes only annotated tags. Followed verbatim, nothing is pushed and no release fires. (Thenpm versionpath above it is fine — npm creates annotated tags.) Fix:git tag -a v1.0.0 -m 'v1.0.0' # annotated: --follow-tags ignores lightweight tags git push --follow-tags
-
docs/getting-started.mdnow pulls a tag that does not exist yet. GHCR's tag list foro1-labs/archive-node-apiis currently0.0.1-test, latest, 0.0, 0, 0.0.1, 0.0.2, 0.0.4, 0.0.5, 0.0.8, 0.0.6, 0.0.9— no1.0.0. It resolves the momentv1.0.0is tagged, but between merge and release that copy-pasteable command fails. Either land this immediately before cutting the tag, or add "(available from the 1.0.0 release onward)". -
The Enforcement section overstates what CI enforces.
Check Schemaandapprove-label: expected-breaking-changeare real (.github/workflows/graphql-inspector.yaml), butmain's required status checks are["Run-Tests", "Linting"]only — a breaking-schema PR goes red without being blocked. Either addCheck Schemato branch protection (preferable, since the whole policy leans on it) or soften to "flags the PR red; treat a redCheck Schemaas requiring theexpected-breaking-changelabel and a major bump." -
The stable-endpoint list names
/readinessand/metrics, which arrive with #187 and #191. Fine as long as both land beforev1.0.0is tagged — worth stating explicitly next to the "these controls arrive in 1.0.0" framing used elsewhere in the batch.
Downstream: strongly positive, and the main reason to land this. The new error-message section converts mina-explorer's inBestChain probe and mina-explorer-api's three-marker capability detection from an undocumented accident into a written major-version contract, and the flag-gating section names the exact silent-degradation mode (green health checks, blank Explorer pages). No runtime behaviour changes in this PR.
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f1ef5bf to
8db560a
Compare
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8db560a to
4aac67f
Compare
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4aac67f to
0230e50
Compare
The package was pre-1.0 (0.0.6) with no documented stability guarantees, which public consumers need. - Bump version to 1.0.0 (package.json only — no tag; the actual release/publish remains a deliberate maintainer step via `npm version` + tag push). - Add docs/versioning.md: SemVer applied to the GraphQL schema / HTTP endpoints / config, a precise definition of breaking vs additive changes, a deprecation policy (@deprecated + one minor & 90 days before removal), and how the existing graphql-inspector "Check Schema" gate enforces it via the expected-breaking-change label. - Link the policy from the README. Closes #178. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
Codifies the practice the repo already relies on but never wrote down: changes to default response shape or exposed query surface ship default-off behind an env flag (ENABLE_BLOCK_TRANSACTION_DETAILS, ENABLED_QUERIES), flagged-off being minor and flipping the default major. This is the rule the schema checker cannot enforce — an unflagged change to a default response isn't schema-breaking, so nothing errors; the mina-explorer just blanks pages while health checks stay green. Also fixes the breaking-change direction for output fields. The doc said "a nullable field/argument to non-null" is breaking, which holds for arguments but is reversed for output fields: there the break is non-null → nullable (clients may now receive null), while nullable → non-null only strengthens the guarantee. The two are mirror images — the client supplies arguments and consumes fields — so the rules are now stated separately. Addresses review feedback on #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0230e50 to
c561a2c
Compare
What & why
Part of the production-readiness epic (#163). Closes #178.
The package was pre-1.0 (
0.0.6) with no documented stability guarantees — public consumers need them.Changes
1.0.0inpackage.json.docs/versioning.md— SemVer applied to the GraphQL schema / HTTP endpoints / config: a precise definition of breaking vs additive changes, a deprecation policy (@deprecated+ one minor & 90 days before removal), and how the existing graphql-inspector "Check Schema" gate enforces it via theexpected-breaking-changelabel.Important: this PR does not publish anything
The version bump is
package.json-only — no git tag, so no release is cut. Publishing is tag-triggered (npm version+git push --follow-tags) and remains a deliberate maintainer action. Treat merging this PR as the decision to declare 1.0; if you're not ready for that, hold the PR (the policy doc stands on its own). Sequence it after the other production-readiness PRs so 1.0.0 ships with them.Testing
Docs + version only.
prettier --debug-check .clean; build sane. No code changed.🤖 Generated with Claude Code