docs(compat): document the exports map in the §6 package surface (#853) - #865
wojciechszyjka wants to merge 2 commits into
Conversation
BACKWARD_COMPATIBILITY.md §6 described a manifest that no longer exists: it said the package has no exports map while packages/cezar/package.json has declared one since the package split (#695), listed a published file the manifest had dropped in that same commit, and omitted the cezar-cli bin. That drift is what let #851 happen — the undocumented exports map silently governed resolution, the alias imported a subpath it did not expose, and every npx cezar-cli died with ERR_PACKAGE_PATH_NOT_EXPORTED. One undocumented surface broke a documented one. §6 now states the three exported subpaths, why ./app-type is type-only rather than a runtime library API, that first-party consumers import the bare specifier, and that adding or removing a subpath is breaking. bin and files match the manifest, in §1 too. bc-package-surface.test.ts is the cause fix: it holds §6's and §1's enumerations to what the manifest declares, in the idiom of the existing §2 route-inventory guard. All six assertions fail on the pre-fix document.
wojciechszyjka
added
review
|
🤖
|
🤖
|
|
🤖 |
|
🤖 |
Three robustness findings from the review of the guard added in the previous
commit, none of which changes what it checks:
- The vacuity guard asserted today's bin and exports counts. JSON.parse cannot
return a half-read manifest, so the manifest-side thresholds guarded nothing,
and pinning the numbers would have failed a legitimate (breaking, deprecated)
removal of a bin or a subpath on an assertion that is not about it. Now
non-emptiness on both sides; the equality comparisons remain the real check.
- The exports-bullet finder matched any line, and the trailing Breaking:
paragraph also names exports subpaths. A reworded bullet would have latched
onto that paragraph and failed with an unreadable comparison instead of the
intended 'no bullet' message. Bullets only now.
- §1's bin parser cut the line at indexOf('('), which is -1 when the
parenthetical is dropped — slice(0, -1) would then eat the last alias's
closing backtick and lose that alias silently.
wojciechszyjka
left a comment
wojciechszyjka
left a comment
There was a problem hiding this comment.
🔍 Code Review
🎯 Summary
This PR corrects BACKWARD_COMPATIBILITY.md §6, which described an npm manifest that has not existed since the package split (b47d507b, #695), and adds a test that keeps the description honest from now on. Two files change: the document, and a new packages/cezar/src/bc-package-surface.test.ts. No production code path is touched, so no runtime behavior can regress; the entire risk surface of this PR is "is the new prose true?" and "is the new guard correct and not over-strict?", which is what this review concentrated on.
The prose was verified fact by fact against the manifest, the consuming code, and the git history rather than taken on trust. All of it holds: packages/cezar/package.json declares exactly the three subpaths §6 now names; ./app-type is genuinely type-only and is genuinely consumed (packages/web/src/api/client.ts:115 and packages/api-client/src/client.ts:12 both import it as import type), so the decision to keep it rather than remove it — the alternative #853 raised — is correct and is now recorded with its reasoning instead of being left implicit; files matches the manifest's four entries, and git show b47d507b~1:package.json confirms web/open-mercato.svg really was dropped in that commit rather than never having existed; cezar-cli has been in bin since 0bf2281a, so adding it to both §6 and §1 is a correction, not a change.
Verdict: approve — three minor robustness findings were raised against the new test and all three were fixed in bb09087b during this run's autofix pass; nothing blocking remains. One procedural note the merger needs: this is a self-review, and GitHub does not permit approving one's own PR, so the review below is submitted as a comment. A human approval is still required before merge — the pipeline label therefore stays review rather than moving to merge-queue, which would misrepresent the state.
🧪 Validation Gate
Run in the configured order from .ai/agentic.config.json, on the final tree (bb09087b):
| Command | Result | Evidence |
|---|---|---|
npm run typecheck |
✅ pass | Both projects clean (@open-mercato/cezar via tsconfig.test.json, @open-mercato/cezar-web). |
npm test |
✅ pass | 320/320 files, 5930/5930 tests. |
npm run test:unit |
✅ pass | 36 tests: 35 passed, 1 skipped, 0 failed. |
npm run build |
✅ pass | Ends on check:pack ok — 478 files, 88 under web/dist (shell + assets present). |
npm run test:package |
✅ pass | 13/13 packaged-CLI E2E tests. |
The gate is green, but the route there is worth recording because an earlier run of it was red and the difference was environmental, not code. npm test failed three times in a row on this machine with different failure sets each time — 36 files, then 20, then 1 — and every failure was either Test timed out (47 occurrences in the second run) or ENOENT against a path under the OS temp directory, all inside tests that spawn processes and git worktrees (workflows/run.test.ts, git-worktree.test.ts, server/worktrees-api.test.ts and similar). Re-running those same 20 files serially with --no-file-parallelism gave 354/354. The final run above, on a quiet machine, is fully green. Nothing in the failing set shares a file with this diff, and non-determinism across identical runs rules out a code cause. This is recorded rather than waved away because "flaky" is exactly the excuse the gate rules forbid — the claim is backed by the serial re-run and by the clean full run, not by assertion.
CI on GitHub was still in progress when this verdict was submitted (license/cla green, Unit, build, E2E, and package pending). Per the pipeline contract the review does not wait on it; the CI outcome is followed up separately below.
🟡 Minor findings (all fixed in bb09087b)
1. packages/cezar/src/bc-package-surface.test.ts:112 — the vacuity guard pinned today's counts, which would fail a legitimate removal. It asserted Object.keys(manifest.bin).length > 2 and Object.keys(manifest.exports).length > 2. Neither guarded anything: JSON.parse cannot return a half-read manifest, so the failure mode a vacuity guard exists to catch — a reader that silently stopped seeing entries — is impossible on the manifest side. What the thresholds could do is fail a properly documented, properly deprecated removal of a bin alias or an exports subpath, on an assertion that has nothing to do with the change, pushing the author to delete the line. Fixed by asserting non-emptiness on both sides instead, with the reasoning recorded in the test; the equality comparisons above remain the real check, and a removal now fails there — where it belongs — or passes once the document is updated to match.
2. packages/cezar/src/bc-package-surface.test.ts:93 — the exports bullet finder could latch onto the wrong paragraph. It searched every line for one containing `exports` and subpaths, and §6's trailing Breaking: paragraph (line 127 of the document) now contains both. Today the bullet at line 122 comes first so find returns the right line, but a rewrite that dropped the word "subpaths" from the bullet would silently move the match to the Breaking: paragraph, which contains no .-rooted tokens — the test would fail with an empty-set comparison instead of the intended, readable "§6 no longer has a bullet describing the exports subpaths". Fixed by restricting the search to bullet lines (line.startsWith('- ')).
3. packages/cezar/src/bc-package-surface.test.ts:129 — unguarded indexOf('(') could drop an alias silently. The §1 parser cut the Bins: line at indexOf('(') to keep the parenthetical's own backticked `bin` out of the token set. indexOf returns -1 when the parenthetical is absent, and slice(0, -1) then removes the final character — the closing backtick of the last alias — so that alias would vanish from the parsed set and the test would report a mismatch the author did not cause. Fixed with an explicit -1 branch that keeps the whole line.
💥 Breaking-Changes Checklist
- Exported APIs — unchanged; the diff adds one test module and edits documentation.
- HTTP routes and response shapes — untouched.
- Event names / protocol — untouched.
- CLI flags and bins — untouched in the manifest. The document's
binlist is corrected to match what the manifest has declared since0bf2281a; correcting a description is not a contract change. - DB schema / migrations — none in this repository's scope, none in the diff.
- Config formats — untouched.
-
BACKWARD_COMPATIBILITY.mdprotected surfaces — this PR is the required path rather than a violation of it. §6 itself said "if it happens, this document gains a section first"; the section is now written, retroactively for a map that shipped in #695. The net effect is to add a protected surface (exportssubpaths, with adding or removing one declared breaking), which constrains future changes rather than relaxing anything. - State-file compatibility (
CODE_REVIEW.mdpriority 3) — nothing under.ai/cezar/is read, written or reshaped.
🧪 Test Coverage
The new guard is the coverage, and it was verified the way a regression test must be: by confirming it fails without the fix. Checked out against the pre-fix document (git show origin/main:BACKWARD_COMPATIBILITY.md), it fails on every drift the issue named, each independently — the missing cezar-cli bin in §6, the same omission in §1, the stale packages/web/public/open-mercato.svg in files, the absent exports bullet, and the Breaking: line that did not mention exports. It passes on the fixed document.
One deliberate consequence of finding 1 is worth stating plainly, since it changes a number quoted in the PR description: against the pre-fix document the guard now fails 5 of 6 assertions rather than 6 of 6. The sixth is the vacuity guard, which no longer trips merely because the old §6 listed two bins instead of three — correctly, because that test is not a drift test and should not double as one. All five drift assertions still fail, so nothing the issue reported has lost coverage.
Design-wise the guard follows the repository's own precedent, packages/cezar/src/server/bc-route-inventory.test.ts, which exists because §2's inventory is prose that nothing verified and had silently drifted. The same reasoning applies here with a concrete cost attached: the undocumented exports map is what made #851 reachable. Fixing only the text would have left #853 free to recur on the next manifest change, which is precisely how it arose. The guard is deliberately loose about prose and strict about enumerations, so §6 can be rewritten, reordered or expanded freely as long as the names it names stay true.
Repository-specific checklist (CODE_REVIEW.md): TypeScript strictness holds — no any, no non-null assertion used to silence the checker (the two ! uses are guarded by a preceding expect(...).toBeDefined()), node:-prefixed builtins, and typecheck passes under strict + noUncheckedIndexedAccess. No new dependency of any kind, so the server runtime dependency budget is untouched. No zod boundary applies — the file reads two repository files at test time, not untrusted input. No degradation path, no state file, no server surface is involved. The comments cite their motivating issues (#853, #851, #695), as the repository asks.
|
🤖 Two notes for whoever merges. First, the verdict had to be submitted as a comment rather than a formal approval, because GitHub does not allow approving one's own PR — a human approval is still required, which is also why the pipeline label stays The local validation gate is fully green on the final tree — typecheck, Lock retained — chain continues. |
wojciechszyjka
removed
the
in-progress
|
🤖 📸 UI: n/a — the diff is a documentation correction plus the test that guards it, with no route, screen, CLI flag or user-visible output involved, so there is nothing for A CI follow-up comment will report the GitHub Actions result; the Lock released. |
📦 npm preview published —
|
|
🤖
This settles the one open question the review left: the local The PR still reports |
1 participant
Closes #853
🎯 Goal
BACKWARD_COMPATIBILITY.md§6 should describe the npm package surface that ships, so the next manifest change cannot quietly introduce an undocumented compatibility surface the way theexportsmap did.🔍 Problem
§6 declared the published surface protected and then described a manifest that no longer exists. It stated the package has no
exports/library API whilepackages/cezar/package.jsondeclares three subpaths; it listedpackages/web/public/open-mercato.svgamong the publishedfiles, which the manifest does not publish; and it named onlycezar+cezas bins while the manifest hascezar-clitoo — the same omission §1 carried. The document contradicted the manifest in three places at once.🔍 Root Cause
The package split (
b47d507b, #695) introduced all three drifts in one commit. It added theexportsmap —@open-mercato/api-clientand the cockpit need./app-typeto type their client off the server's route table — and droppedweb/open-mercato.svgfromfiles, but in the document it only rewrote the path prefixes (web/dist→packages/cezar/web/dist,web/open-mercato.svg→packages/web/public/open-mercato.svg) instead of re-reading the manifest. The "noexports" sentence was left standing by the very commit that added one.cezar-clihad been inbinsince0bf2281aand was never written down.This is why it mattered: the undocumented
exportsmap silently governed resolution. Once a package declares one, Node serves only the listed subpaths and hard-blocks the rest, so the alias'simport('@open-mercato/cezar/dist/index.js')died withERR_PACKAGE_PATH_NOT_EXPORTEDat every user (#851, fixed in #852) — an undocumented surface breaking a documented one.What Changed
BACKWARD_COMPATIBILITY.md§6 — the "noexports" sentence is replaced by two bullets. The first names the three subpaths (.,./app-type,./package.json), records the decision to keep./app-typeand why the package is still not a runtime library (it is type-only by construction —import typeerases at build, which is what lets a browser bundle consume it without draggingnode:*in), and states that the map is load-bearing regardless because it is what keeps the deep-path surface closed. The second records that first-party consumers import the bare specifier and never a deep path, citing npx cezar-cli fails on 0.9.3: ERR_PACKAGE_PATH_NOT_EXPORTED (bin.js imports unexported subpath) #851 as the precedent and pointing at thealias-bin-exports.test.tsguard from fix(cli): import the bare specifier in the cezar-cli alias (#851) #852. Thefileslist now reads the manifest's owndist,web/dist,scripts,README.mdwith a note that manifest paths are package-relative, thebinlist gainscezar-cli, the heading points atpackages/cezar/package.json, and the Breaking line covers adding or removing anexportssubpath.BACKWARD_COMPATIBILITY.md§1 — theBins:line carried the same untruth aboutcezar-cli; a document that contradicts itself is worse than one that is merely stale.packages/cezar/src/bc-package-surface.test.ts(new) — the drift guard, in the idiom of the existing §2 route-inventory guard (src/server/bc-route-inventory.test.ts). It parses §6's and §1's backticked enumerations and compares them against the manifest, asserts §6 still promises that anexportschange is breaking, and carries a vacuity guard so a parser that silently stopped seeing the enumerations cannot pass by comparing empty sets. It is deliberately loose about prose and strict about lists, so the section can be rewritten freely as long as the names it names stay true.🧪 Tests
npx vitest run packages/cezar/src/bc-package-surface.test.ts— 6 passed. Against the pre-fix document all 6 fail, each drift caught separately: the missingcezar-clibin (§6 and §1), the stalefilesentry, the absentexportsbullet, and the Breaking line that did not mentionexports.npm run typecheck✅ ·npm run test:unit— 35 passed / 1 skipped ✅ ·npm run build✅ (check:pack ok — 478 files, 88 under web/dist) ·npm run test:package— 13 passed ✅.npm testis red locally on this machine and is not a regression from this PR: two identical runs produced different failure sets (36 then 20 files), every failure isTest timed out(47×) orENOENTunder the OS temp dir in tests that spawn processes and git worktrees, and re-running those same 20 files serially (--no-file-parallelism) gives 354/354 passing. None of them touch the changed files. Deferring to CI for the authoritative verdict.💥 Breaking Changes
exportsmap as a protected surface, which constrains future changes rather than making one.