Make the client-facing contracts explicit and enforceable

.github/workflows/ci.yml

@@ -6,6 +6,8 @@ name: CI
 permissions:
   contents: read
   packages: write
+  # The API Contract job posts its report as a pull-request comment.
+  pull-requests: write
 
 on:
   pull_request:
@@ -90,6 +92,122 @@ jobs:
       - name: Clippy check
         run: cargo clippy --features rar -- -D warnings
 
+  # Guard the API contract: the committed spec must match the code, and any
+  # change to what clients depend on is surfaced on the run.
+  #
+  # The freshness check is not optional decoration. `web/openapi.json` is a
+  # committed artifact regenerated by a pre-commit hook, so a commit made with
+  # `--no-verify`, or from a clone where `make setup-hooks` was never run,
+  # leaves it stale. oasdiff would then compare two identical stale files, pass,
+  # and let the breaking change through: the gate would be theatre.
+  api-contract:
+    name: API Contract
+    runs-on: ubuntu-latest
+    timeout-minutes: 30
+    env:
+      SCCACHE_GHA_ENABLED: "true"
+      SCCACHE_GHA_VERSION: openapi
+      RUSTC_WRAPPER: sccache
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install mold linker
+        run: sudo apt-get update && sudo apt-get install -y mold
+      - name: Install Rust
+        uses: dtolnay/rust-toolchain@master
+        with:
+          toolchain: 1.95.0
+      - name: Setup sccache
+        uses: mozilla-actions/sccache-action@v0.0.9
+
+      - name: Check the committed spec matches the code
+        run: |
+          cargo run -- openapi --output /tmp/openapi-fresh.json
+          if ! diff -q /tmp/openapi-fresh.json web/openapi.json >/dev/null; then
+            echo "::error::web/openapi.json is stale. Run 'make openapi-all' and commit the result."
+            diff -u web/openapi.json /tmp/openapi-fresh.json | head -60 || true
+            exit 1
+          fi
+          if ! diff -q web/openapi.json docs/api/openapi.json >/dev/null; then
+            echo "::error::docs/api/openapi.json differs from web/openapi.json. Run 'make openapi'."
+            exit 1
+          fi
+          echo "OpenAPI spec is in sync with the backend."
+
+      - name: Fetch the base branch
+        run: git fetch --depth=1 origin ${{ github.base_ref }}
+
+      # Reports rather than fails, and the distinction is deliberate.
+      #
+      # A red job means "fix this before merging". A contract change is not
+      # that: breaking changes are a normal mid-cycle event here, because the
+      # version is decided at tag time rather than per PR. Failing the job would
+      # mark a routine, intentional act as a failure, and a check that is
+      # routinely red is one people learn to skim past, which costs more than it
+      # catches.
+      #
+      # `continue-on-error` keeps the job green while still flagging the step, so
+      # the run page shows the change without claiming anything is broken. The
+      # decision this feeds is made by `make release-prepare`, which compares
+      # against the previous tag at the moment the version is actually chosen.
+      #
+      # The stale-spec check above stays fatal, because that one really is a
+      # mistake: the committed artifact does not match the code.
+      - name: Report API contract changes
+        id: contract
+        continue-on-error: true
+        uses: oasdiff/oasdiff-action/breaking@v0
+        with:
+          base: origin/${{ github.base_ref }}:web/openapi.json
+          revision: HEAD:web/openapi.json
+          fail-on: ERR
+          # The spec is public, but uploading it to a third party should be a
+          # decision rather than a default. `github-token: ''` disables the
+          # action's own comment, which posts a link to that upload rather than
+          # the findings; the report is posted below instead.
+          review: false
+          github-token: ""
+
+      # One comment, edited in place, so the PR always shows the current state
+      # rather than a thread of stale reports. It is rewritten to say the
+      # contract is unchanged once the findings are gone, because a lingering
+      # "3 breaking changes" on a PR that no longer has any is worse than no
+      # comment at all.
+      - name: Comment the contract report
+        if: always() && github.event.pull_request.head.repo.full_name == github.repository
+        env:
+          GH_TOKEN: ${{ github.token }}
+          PR: ${{ github.event.pull_request.number }}
+          REPO: ${{ github.repository }}
+          REPORT: ${{ steps.contract.outputs.breaking }}
+        run: |
+          MARKER='<!-- api-contract-report -->'
+
+          if [ -n "$REPORT" ]; then
+            BODY=$(
+              printf '%s\n### API contract changes\n\n' "$MARKER"
+              printf 'Compared against `%s`. These are changes a client generated from the\n' "${{ github.base_ref }}"
+              printf 'previous document would notice. Not a failure: breaking changes are a\n'
+              printf 'release-time decision, and `make release-prepare` checks the bump against\n'
+              printf 'them when the version is chosen.\n\n'
+              printf '<details><summary>Report</summary>\n\n```\n'
+              printf '%s\n' "$REPORT" | head -c 55000
+              printf '```\n\n</details>\n'
+            )
+          else
+            BODY="${MARKER}"$'\n### API contract changes\n\nNone. Nothing a client generated from the previous document would notice.'
+          fi
+
+          ID=$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \
+                 --jq "[.[] | select(.body | startswith(\"${MARKER}\")) | .id] | first // empty")
+
+          if [ -n "$ID" ]; then
+            gh api -X PATCH "repos/${REPO}/issues/comments/${ID}" -f body="$BODY" >/dev/null
+            echo "Updated comment ${ID}"
+          else
+            gh api -X POST "repos/${REPO}/issues/${PR}/comments" -f body="$BODY" >/dev/null
+            echo "Created comment"
+          fi
+
   # Run frontend tests and build
   frontend:
     name: Frontend

Makefile

@@ -597,6 +597,10 @@ release-prepare: ## Prepare a release (usage: make release-prepare VERSION=1.0.0
 	@$(MAKE) openapi-all
 	@echo "$(GREEN)✓$(NC) Regenerated OpenAPI spec and TypeScript types"
 
+	@# Check the bump against what actually changed in the API
+	@echo "$(YELLOW)Checking the version bump against the API contract...$(NC)";
+	@./scripts/check-release-bump.sh $(VERSION) || true
+
 	@# Generate changelog (skip if already modified)
 	@echo "$(YELLOW)Generating CHANGELOG.md...$(NC)";
 	@if git diff --quiet CHANGELOG.md 2>/dev/null && git diff --cached --quiet CHANGELOG.md 2>/dev/null; then \

cliff.toml

@@ -10,6 +10,11 @@ header = """
 
 All notable changes to Codex will be documented in this file.
 
+**API compatibility.** API features ship in minor releases and are never backported into a patch
+release. A patch release may fix an API bug; it may not add an operation, a parameter, a response
+field, or a new behaviour on an existing route. A client may therefore treat the release a feature
+first appears under in the 🔌 API sections below as a floor it can rely on.
+
 """
 body = """
 {% if version %}\
@@ -67,22 +72,34 @@ protect_breaking_commits = false
 # An array of regex based parsers for extracting data from the commit message.
 # Assigns commits to groups.
 # Optionally sets the commit's scope and can decide to exclude commits from further processing.
+# Ordering keys are zero-padded because groups sort as strings: an unpadded
+# "<!-- 10 -->" sorts between 1 and 2, which is why "Other" used to appear third.
+#
+# The API parsers come first, and therefore win over the type-based groups below,
+# because the audience for an API change is a client author regenerating against
+# openapi.json, and they need it separated from web and config work rather than
+# interleaved with it. Scoped to the types that can move the wire: a `test(api)`
+# or `style(api)` commit changes nothing a client can observe. The scope pattern
+# matches `api` as a whole element of a comma-separated scope list, so
+# `feat(api, db)` counts and `feat(komga-api)` does not.
 commit_parsers = [
-    { message = "^feat", group = "<!-- 0 -->🚀 Features" },
-    { message = "^fix", group = "<!-- 1 -->🐛 Bug Fixes" },
-    { message = "^doc", group = "<!-- 3 -->📚 Documentation" },
-    { message = "^perf", group = "<!-- 4 -->⚡ Performance" },
-    { message = "^refactor", group = "<!-- 2 -->🚜 Refactor" },
-    { message = "^style", group = "<!-- 5 -->🎨 Styling" },
-    { message = "^test", group = "<!-- 6 -->🧪 Testing" },
+    { message = "^(feat|fix|docs?|perf|refactor)\\((?:[a-z0-9_-]+, *)*api(?:, *[a-z0-9_-]+)*\\)!", group = "<!-- 00 -->🔌 API (breaking)" },
+    { message = "^(feat|fix|docs?|perf|refactor)\\((?:[a-z0-9_-]+, *)*api(?:, *[a-z0-9_-]+)*\\)", group = "<!-- 01 -->🔌 API" },
+    { message = "^feat", group = "<!-- 02 -->🚀 Features" },
+    { message = "^fix", group = "<!-- 03 -->🐛 Bug Fixes" },
+    { message = "^refactor", group = "<!-- 04 -->🚜 Refactor" },
+    { message = "^doc", group = "<!-- 05 -->📚 Documentation" },
+    { message = "^perf", group = "<!-- 06 -->⚡ Performance" },
+    { message = "^style", group = "<!-- 07 -->🎨 Styling" },
+    { message = "^test", group = "<!-- 08 -->🧪 Testing" },
     { message = "^chore\\(release\\): prepare for", skip = true },
     { message = "^chore\\(deps.*\\)", skip = true },
     { message = "^chore\\(pr\\)", skip = true },
     { message = "^chore\\(pull\\)", skip = true },
-    { message = "^chore|^ci", group = "<!-- 7 -->⚙️ Miscellaneous Tasks" },
-    { body = ".*security", group = "<!-- 8 -->🛡️ Security" },
-    { message = "^revert", group = "<!-- 9 -->◀️ Revert" },
-    { message = ".*", group = "<!-- 10 -->💼 Other" },
+    { message = "^chore|^ci|^build", group = "<!-- 09 -->⚙️ Miscellaneous Tasks" },
+    { body = ".*security", group = "<!-- 10 -->🛡️ Security" },
+    { message = "^revert", group = "<!-- 11 -->◀️ Revert" },
+    { message = ".*", group = "<!-- 12 -->💼 Other" },
 ]
 # Exclude commits that are not matched by any commit parser.
 filter_commits = false

contracts/README.md

@@ -0,0 +1,73 @@
+# Contracts
+
+Behavioural contracts that more than one Codex client has to implement identically.
+
+Unlike `docs/api/openapi.json`, nothing here is generated from the server. These files describe
+behaviour that lives in the *clients* but whose output lands in shared server data, so if two
+clients implement it differently the data they produce silently stops meaning one thing.
+
+Each file is hand-authored, versioned by a `version` field, and consumed as data by a test suite in
+every repo that implements it. A change to the definition is therefore a reviewable diff, and a
+client that has not adopted it fails its own suite.
+
+| File | Implemented by | Consumed by |
+| --- | --- | --- |
+| [`reading-sessions.json`](./reading-sessions.json) | web reader, codex-reader-ios | `web/src/lib/reading/ReadingSessionTracker.test.ts` |
+
+## `reading-sessions.json`
+
+The measurement rules behind `POST /api/v1/reading-sessions`. Aggregate reading time is summed
+across every device a reader uses, so "active time" has to mean the same thing on all of them. If
+one client idles at two minutes and another at five, the total is a blend of two metrics and means
+nothing.
+
+### What it constrains, and what it does not
+
+**Constrained, exactly:** the state machine and its arithmetic. Given the event sequence in a case,
+with those timestamps, a conformant tracker produces exactly the sessions the case lists: the same
+count, the same kinds, the same order, the same `activeDurationMs`, `pagesRead`, and position. The
+same holds for what survives a crash.
+
+**Not constrained:** which platform occurrence produces which event. The web reader decides that
+`visibilitychange` means `pause`; the native client decides that a `scenePhase` transition does.
+That mapping is each client's business, and a native client with real lifecycle callbacks is free to
+be more precise about *when* it emits `pause` than the web reader can be. Also unconstrained: how a
+checkpoint is persisted, and what an id looks like beyond being unique within a case.
+
+That line is where the earlier open question lands. Pause and resume *are* in the contract, because
+they are modelled as events rather than as platform occurrences. What a client detects, and how
+promptly, is latitude. What it does once it has detected it is not.
+
+### Schema
+
+- `version` — bump on any change to a threshold, an event's meaning, or a case's expectation.
+- `subject` — the book, device, and device name every case runs against, unless a case overrides
+  `bookId`.
+- `thresholds` — `idleTimeoutMs` and `checkpointIntervalMs`, carried here rather than assumed, so a
+  change to the definition of "active" shows up in this file.
+- `invariants` — properties asserted on every case in addition to its own expectations.
+- `cases[]`:
+  - `name`, `group`, optional `why`.
+  - `bookId` — overrides `subject.bookId`.
+  - `persistence: "unavailable"` — run this case with a store that cannot be written. How the runner
+    arranges that is platform-specific.
+  - `seedCheckpoints[]` — `{bookId, raw}` written into the store before the case runs, to model a
+    checkpoint left by an older or broken write.
+  - `events[]` — `atMs` is absolute from the start of the case, so a runner drives a fake clock to
+    each timestamp in turn. Kinds: `start`, `activity` (both take optional `page` or `percentage`),
+    `pause`, `resume`, `stop`, `complete` (optional position), `reset`, `checkpoint` (persist
+    without closing), `crash` (the process dies with no chance to close), `recover` (run orphan
+    recovery, appending its result to `recoveries`).
+  - `expect`:
+    - `sessions[]` — emitted sessions in order, in the wire shape of the endpoint. A field set to
+      `null` must be **absent** from the payload. `spanMs`, where present, is
+      `clientEndedAt - clientStartedAt`. Fields not listed are not asserted.
+    - `recoveries[]` — one entry per `recover` event, each an array of recovered sessions.
+    - `totalActiveDurationMs` — summed across `sessions`, where the point of the case is the total.
+    - `tracking` — whether a session is still open at the end.
+    - `checkpointedBookIds` — book ids with a checkpoint waiting, sorted.
+
+### Adding a case
+
+Add it here first, then make both suites pass. A case whose events duplicate an existing case
+belongs as extra expectations on that case rather than as a new row.