Make the client-facing contracts explicit and enforceable - #43
Merged
Conversation
Deploying codex with
|
| Latest commit: |
5100de6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fb218c0c.codex-asm.pages.dev |
| Branch Preview URL: | https://feat-native-client-requireme.codex-asm.pages.dev |
AshDevFr
changed the title
… of test code Aggregate reading time is summed across every device a reader uses, so "active time" has to mean the same thing on all of them. Until now the rules existed only as Vitest calls in the web tracker's suite, which no other client can consume. Extract them into contracts/reading-sessions.json: a versioned, language-neutral table carrying the idle timeout and checkpoint interval as data rather than assuming them, and rewrite the web suite as a runner over it. What is left in the suite is the mapping from a contract event onto this tracker's API, and the browser-specific storage failures the contract deliberately leaves to each platform. The contract constrains the state machine exactly: given a case's events and timestamps, a conformant tracker produces the same sessions, kinds, durations, pages, and positions, and the same result after a crash. It does not constrain which platform occurrence produces which event, so a client with real lifecycle callbacks can be more precise about when it pauses without the arithmetic drifting. Three assertions that held for every scenario rather than one became invariants checked on every case, and asserting the span invariant everywhere caught a real bug: a session rescued from a crash ends at its last recorded activity, but checkpointNow writes accrued time that no activity followed, so a session checkpointed on pagehide could report a minute of reading inside a zero-length span. toPayload now floors the end at startedAt + activeMs.
…ordering Grouping purely by commit type scatters API changes through Features and Bug Fixes among web, config, and tooling entries. Five of 2.2.0's seven entries were API changes a client author has to read, interleaved with Docusaurus and Vitest entries they must not. The scope was already on those commits and the grouping threw it away. Pull `api`-scoped commits into their own group ahead of the type-based ones, with a separate breaking variant. The scope pattern matches `api` as a whole element of a comma-separated list, so `feat(api, auth, config, db)` counts, of which this repo has many; `feat(komga-api)` does not, being a compatibility layer rather than the native API. Restricted to the types that can move the wire, since a `test(api)` commit in the section a client reads defeats the point of the section. The header now carries the release policy that makes the changelog usable as a compatibility reference: API features ship in minor releases and are never backported into a patch. A client can then treat the release a feature first appears under as a floor it can rely on, and decide what a server supports from the changelog rather than by probing. It lives here rather than in the docs so the promise sits directly above the data it is about, and is regenerated rather than maintained. Two ordering defects fixed while in the file. Groups sort as strings, so the unpadded `<!-- 10 -->` sorted between 1 and 2 and put Other third in every release; all keys are now zero-padded. And `build` commits matched no parser and fell through to Other, which is what put the Docusaurus upgrade there. CHANGELOG.md is generated and untouched. The regrouping applies retroactively, so the next regeneration will produce a large diff as past releases gain API sections.
…nes that don't The MSW fixture served `reader.fitMode`, `reader.readingDirection`, `library.defaultView`, `library.itemsPerPage`, `notifications.enabled`, and a bare `theme`. None of those keys exists, and none ever has. The DTO examples carried `reader.zoom` alongside them. Between them they read exactly like a record of what the preference store holds, and were taken as one: a grep over web/src returns them next to the four real keys with nothing to tell them apart. Reader settings are device-local Zustand state persisted to localStorage, and per-series overrides already exist there with their own versioned schema. They are not synced on purpose: a phone and a desktop legitimately want different fit modes. The four keys that really are preferences are now in the description of GET /api/v1/user/preferences, with their value shapes and defaults, so they land in openapi.json where a client generating against it will find them. The description says explicitly that the store is open and the list is not a whitelist, so the correction does not become a new false claim, and it records why reader settings are absent so the next reader finds the answer rather than the gap. The mocks now serve exactly those four, with a test asserting the mocked key set equals PREFERENCE_DEFAULTS. That fixture is the one place an invented key can re-enter, so that is where the guard sits.
…rent first The existing openapi_spec.rs invariants check the document's shape: no unparameterised generics, no orphaned components, path parameters matching their templates. They say nothing about its evolution. An operation removed, renamed, or moved passes all of them, and the generated Swift client is the only thing that notices, at re-vendor time, in another repo. Add an oasdiff gate on pull requests, comparing against the base branch and failing on ERR. It knows 219 breaking-change rules, covering cases a short hand-written check would miss: a response property changing type, a response code disappearing, an array becoming a scalar, a format narrowing. Its base accepts a git ref, so there is no baseline file to keep current. The gate is skipped when a PR carries the `breaking-change` label, so breaking the contract stays a deliberate, visible act rather than a red check people learn to ignore. `review` is off, because the action otherwise uploads an encrypted spec comparison to a third party by default. The freshness check in front of it is what makes any of this mean anything. web/openapi.json is committed and regenerated by a pre-commit hook, and that hook was the only thing enforcing it, so a commit made with --no-verify or from a clone without hooks installed leaves it stale. oasdiff would then compare two identical stale files, pass, and let the break through. Nothing is committed by CI: it regenerates to a temp file, fails, and says to run make openapi-all. make release-prepare now also reports whether the bump matches what changed since the last tag, naming the affected endpoints when breaking changes land in a non-major release. It never fails: by then the work is merged, and a breaking change is sometimes exactly what was intended. It only makes the number on the tin a decision rather than a habit. Note that oasdiff compares the document rather than the server, so correcting a spec that described an endpoint wrongly reads as breaking even when no running client changes behaviour. Against real history, 2.1.0 to 2.2.0 reports 49 breaking changes across 9 endpoints, all of them this cycle's defect fixes, shipped as a minor bump with nothing to say so at the time.
A red job means "fix this before merging". A deliberate contract change is not that: the version is decided at tag time rather than per PR, so a breaking change on a feature branch is a normal event on its way to a release numbered accordingly. Failing the job would mark a routine act as a failure, and a check that is routinely red is one people learn to skim past. It would have spent its own signal within a handful of PRs, and the bypass label would have become a chore performed without reading anything. Two signals had been collapsed into one. A stale committed spec is unambiguously a mistake, since the artifact no longer matches the code and nothing downstream can be trusted until it does, so that step stays fatal. A contract change is a judgement call that needs to be visible where the work is happening, which is not the same as needing to block anything. The findings now land as a pull-request comment, edited in place so the PR shows current state rather than a thread of stale reports, and rewritten to say the contract is unchanged once the findings are gone. A lingering "3 breaking changes" on a PR that no longer has any is worse than no comment at all. The action's own commenting is disabled with an empty github-token: it posts a link to the oasdiff.com upload rather than the findings, and that upload is off.
AshDevFr
force-pushed
the
feat/native-client-requirements
branch
from
August 23, 2026 05:23
b70bbec to
5100de6
Compare
API contract changesCompared against Report |
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.
Four changes that make what this server promises to its clients legible, and hard to break by
accident.
The reading-session rules are a shared file now, not test code
Aggregate reading time is summed across every device a reader uses, so "active time" has to mean the
same thing on all of them. Until now those rules existed only as Vitest calls in the web tracker's
suite, which a native client cannot consume.
contracts/reading-sessions.jsoncarries 26 cases and both thresholds as data, versioned so a changeto the definition of "active" is a visible diff. The web suite is now a runner over it: what remains
in the test file is the mapping from a contract event onto the tracker's API, plus the browser storage
failures the contract deliberately leaves to each platform.
The contract constrains the state machine exactly, and leaves the event mapping to each client, so a
client with real lifecycle callbacks can be more precise about when it pauses without the arithmetic
drifting.
Extracting it found a real bug. Asserting
activeDurationMs <= spanon every case rather than onone caught it: a session rescued from a crash ends at its last recorded activity, but
checkpointNowwrites accrued time that no activity followed, so a session checkpointed on
pagehidecould report aminute of reading inside a zero-length span.
toPayloadnow floors the end atstartedAt + activeMs.API changes get their own changelog group
Grouping purely by commit type scattered 2.2.0's six API changes across Features, Bug Fixes,
Documentation, and Other, interleaved with Vitest and Docusaurus entries a client author must not
read. They are one block now, ahead of the type-based groups, with a separate breaking variant.
The scope pattern matches
apias a whole element of a comma-separated list, sofeat(api, auth, config, db)counts, of which this repo has many.feat(komga-api)does not, beingthe compatibility layer rather than the native API. Verified against thirteen representative subjects
from this repo's history.
Two ordering defects fixed while in the file. Group keys sort as strings, so the unpadded
<!-- 10 -->sorted between 1 and 2 and put Other third in every release. Andbuildcommits matchedno parser at all, which is what put the Docusaurus upgrade there.
The header now carries the release policy that makes the changelog usable as a compatibility
reference: API features ship in minor releases and are never backported into a patch, so a client can
treat the release a feature first appeared in as a floor it can rely on.
Preference keys that exist, and fixtures that stop pretending
The MSW fixture served
reader.fitMode,reader.readingDirection,library.defaultView,library.itemsPerPage,notifications.enabled, and a baretheme. None of those keys exists. The DTOexamples carried
reader.zoomalongside them. Between them they read exactly like a record of what thepreference store holds.
Preference keys are a cross-client contract: two clients have to write the same ones or a user's
settings stop following them between devices. The four that really are preferences now appear in the
description of
GET /api/v1/user/preferenceswith their value shapes and defaults, so they land inopenapi.jsonwhere a client generating against it will find them. The description says explicitlythat the store is open and the list is not a whitelist, so the correction does not become a new false
claim, and it records why reader settings are absent: they are device-local state, deliberately not
synced, because a phone and a desktop legitimately want different fit modes.
The mocks serve exactly those four, with a test asserting the mocked key set equals
PREFERENCE_DEFAULTS. That fixture is the one place an invented key can re-enter.A gate on breaking API changes
tests/api/openapi_spec.rschecks the document's shape and says nothing about its evolution: anoperation removed, renamed, or moved passes every one of its invariants, and the generated Swift
client is the only thing that notices, at re-vendor time, in another repo.
oasdiffnow runs on pull requests, comparing against the base branch and failing on ERR. It knows219 breaking-change rules, covering things like a response property changing type, a response code
disappearing, an array becoming a scalar, and a format narrowing. Its base accepts a git ref, so there
is no baseline file to keep current.
The gate is skipped when a PR carries the
breaking-changelabel, so breaking the contract stays adeliberate, visible act rather than a red check people learn to ignore.
reviewis off, because theaction otherwise uploads an encrypted spec comparison to a third party by default.
The freshness check in front of it is what makes any of this mean anything.
web/openapi.jsoniscommitted and regenerated by a pre-commit hook, and that hook was the only thing enforcing it, so a
commit made with
--no-verifyor from a clone without hooks installed leaves it stale. oasdiff wouldthen compare two identical stale files, pass, and let the break through. CI commits nothing: it
regenerates to a temp file, fails, and says to run
make openapi-all.make release-preparealso reports whether the bump matches what changed since the last tag, namingthe affected endpoints when breaking changes land in a non-major release. It never fails: by then the
work is merged, and a breaking change is sometimes exactly what was intended.
First run against real history: 2.1.0 to 2.2.0 reports 49 breaking changes and 39 warnings across
9 endpoints, shipped as a minor bump. Note that oasdiff compares the document rather than the server,
so correcting a spec that described an endpoint wrongly reads as breaking even when no running client
changes behaviour. Expect the
breaking-changelabel to be used while the document is still beingtidied.
Testing
cargo clippy -- -D warningsclean, preference key validator tests passing