Check the declared Lexical range with semver rules (#12319)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - People write almost every issue, goal, document, and chat message through the rich markdown editor, which wraps `@mdxeditor/editor` on top of Lexical > - #12317 fixed a dependency split that broke that editor everywhere, and it added `ui/src/lib/lexical-single-copy.test.ts` to keep the Lexical graph honest > - Greptile raised a P2 on the last assertion in that test. It compared the range `@mdxeditor/editor` declares to a literal string, so it rejected equivalent spellings such as `>=0.48.0 <0.49.0` even when the resolved version satisfies them > - A guard that fails on a healthy tree teaches people to delete it, which would give back the protection #12317 just added > - #12317 merged before the fix landed, so this pull request carries it > - This pull request stops the test from reading semver ranges at all, and asserts the mechanism that can actually break the graph > - The benefit is a guard that fails only on a real problem, and a smaller test file than before ## Linked Issues or Issue Description Refs #12317 — this addresses the Greptile P2 left on that pull request. No public issue exists, so the problem is described below with the bug report template. **What happened?** `ui/src/lib/lexical-single-copy.test.ts` asserted the declared range with an exact string comparison: ```ts const [major, minor] = versionOf("lexical", requireFromUi).split("."); expect(declared).toBe(`^${major}.${minor}.0`); ``` That accepts one spelling only. If `@mdxeditor/editor` published `>=0.48.0 <0.49.0`, or `^0.48.2`, or `^0.47.0 || ^0.48.0`, the test would fail even though the resolved Lexical version satisfies the declared range. The test would report a dependency split that does not exist. **Expected behavior** The test fails when the Lexical graph is actually split or forced. It passes on any healthy tree, whatever range syntax the editor happens to publish. **Steps to reproduce** 1. Check out `master` atc8a136fb0. 2. Edit `ui/node_modules/@mdxeditor/editor/package.json` and change `"lexical": "^0.48.0"` to the equivalent `"lexical": ">=0.48.0 <0.49.0"`. 3. Run `npx vitest run ui/src/lib/lexical-single-copy.test.ts`. 4. The range case fails, although 0.48.0 satisfies the range. **Paperclip version or commit** `master` atc8a136fb0. The assertion arrived with #12317. **Relevant logs or output** ``` AssertionError: expected '>=0.48.0 <0.49.0' to be '^0.48.0' // Object.is equality ``` ## What Changed The test no longer reads semver ranges. It asserts the mechanism instead. - Replaced the range assertion with a check that no `lexical` or `@lexical/*` key appears in the root `pnpm.overrides`. The failure message names the fix. - Removed the range assertion and the version-line comparison that stood in for it. Both needed range syntax to mean something specific. - Documented on the copy check why it now carries the whole guarantee. Why this is sufficient, and stronger: - An override is the only thing that can push a resolved version outside the range a package declares. Remove the override and pnpm honours every declared range by construction, so `@mdxeditor/editor` gets a Lexical it supports. No parser needed. - `@mdxeditor/editor` resolved its own Lexical from its own declared range. The copy check asserts the app resolved the same version. So an equal version proves the app sits on a line the editor supports, transitively. - It also closes the second half of the original bug. The old override list could only pin the packages it named, and `@lexical/extension` was reached transitively and never listed. A guard on "no Lexical override at all" has no such blind spot, while a guard on "the override pins the right version" would have. The file is 65 lines shorter than the first attempt and no longer carries a hand-rolled semver evaluator. I considered two alternatives and rejected both. A `semver` dependency in `ui` would work, but `ui` cannot resolve `semver` today and a new direct dependency is a heavy way to buy one assertion. A larger hand-rolled evaluator would have to cover partial versions, wildcards, hyphen ranges, and spaced comparators before it stopped producing false failures, which is a semver library with fewer tests. Nothing outside this test file changes. The Lexical pins from #12317 stay as they are, and `pnpm-lock.yaml` is untouched because no manifest changes. ## Verification - `npx vitest run ui/src/lib/lexical-single-copy.test.ts` — 3 passed. - `pnpm --filter @paperclipai/ui typecheck` — clean. - Regression still caught. I restored the pre-#12317 pins, which are the 0.49.0 `pnpm.overrides` block and the 0.49.0 pins in `ui/package.json`, ran `pnpm install`, and re-ran the guard. Two of the three cases failed, and the new one reports the cause and the fix together: ``` AssertionError: Pin Lexical through ui/package.json instead. An override cannot cover the packages it does not name, and it hides the range @mdxeditor/editor declares.: expected [ 'lexical', …(9) ] to deeply equal [] AssertionError: expected '0.48.0' to be '0.49.0' ``` I then restored the current pins and re-ran the guard, which passed 3 of 3. - `npx vitest run ui/src/lib/mention-deletion.test.ts ui/src/lib/mention-aware-link-node.test.ts ui/src/components/MarkdownEditor.test.tsx` — 42 passed, unchanged. ## Risks - Low risk. The change touches one test file and no application code. - The guard now forbids a legitimate tool. If someone later has a real need for a Lexical `pnpm.overrides` entry, this test blocks it. That is deliberate: an override is what broke the editor, and the block is one line to remove with a reviewer looking at it. The failure message states the supported alternative, which is pinning in `ui/package.json`. - The guard no longer checks the declared range directly. It relies on pnpm honouring declared ranges when no override is present, which is the package manager's contract and the same property `pnpm install --frozen-lockfile` depends on in CI. - The ordering #12317 set out still holds: a Lexical upgrade must wait for an `@mdxeditor/editor` release that supports it. Bumping `ui` past the editor's range now produces two copies, which the copy checks fail on. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used Claude (Anthropic), Claude Opus, agentic tool use via Claude Code. - Provider and model: Claude (Anthropic), Claude Opus - Capability: agentic tool use through Claude Code, with repository search, file edits, dependency installation, test and build runs ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (the reasoning lives in comments in the test file; no document states a Lexical version or pinning policy) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
parent
b06034d762
commit
d27998a230
|
|
@ -1,6 +1,7 @@
|
|||
import { readFileSync } from "node:fs";
|
||||
import { existsSync, readFileSync } from "node:fs";
|
||||
import { createRequire } from "node:module";
|
||||
import { dirname, join, parse } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
/**
|
||||
|
|
@ -25,7 +26,7 @@ describe("lexical single copy", () => {
|
|||
const requireFromMdxEditor = createRequire(mdxEditorEntry);
|
||||
|
||||
/** These packages block "./package.json" in exports, so read it off disk. */
|
||||
function manifestOf(specifier: string, from: NodeJS.Require): { version: string; dependencies?: Record<string, string> } {
|
||||
function manifestOf(specifier: string, from: NodeJS.Require): { version: string } {
|
||||
let dir = dirname(from.resolve(specifier));
|
||||
const { root } = parse(dir);
|
||||
while (true) {
|
||||
|
|
@ -42,6 +43,38 @@ describe("lexical single copy", () => {
|
|||
return manifestOf(specifier, from).version;
|
||||
}
|
||||
|
||||
/** Walks up from this file to the pnpm workspace root. */
|
||||
function workspaceRoot(): string {
|
||||
let dir = dirname(fileURLToPath(import.meta.url));
|
||||
const { root } = parse(dir);
|
||||
while (!existsSync(join(dir, "pnpm-workspace.yaml"))) {
|
||||
if (dir === root) throw new Error("could not find the pnpm workspace root");
|
||||
dir = dirname(dir);
|
||||
}
|
||||
return dir;
|
||||
}
|
||||
|
||||
it("keeps the Lexical family out of pnpm.overrides", () => {
|
||||
// An override outranks the range `@mdxeditor/editor` declares, so pnpm
|
||||
// stops guaranteeing that the editor gets a version it supports. It also
|
||||
// pins only the packages it names: `@lexical/extension` was reached
|
||||
// transitively, never appeared in the list, and stayed a minor behind
|
||||
// while the listed packages moved forward. That is what split the graph.
|
||||
//
|
||||
// With no override, pnpm honours every declared range, and the copy
|
||||
// checks below confirm the app and the editor landed on the same one.
|
||||
const manifest = JSON.parse(readFileSync(join(workspaceRoot(), "package.json"), "utf8"));
|
||||
const overrides: Record<string, string> = manifest.pnpm?.overrides ?? {};
|
||||
const forced = Object.keys(overrides).filter(
|
||||
(name) => name === "lexical" || name.startsWith("@lexical/"),
|
||||
);
|
||||
expect(
|
||||
forced,
|
||||
"Pin Lexical through ui/package.json instead. An override cannot cover the "
|
||||
+ "packages it does not name, and it hides the range @mdxeditor/editor declares.",
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("resolves the same lexical copy for the app and the editor", () => {
|
||||
expect(requireFromMdxEditor.resolve("lexical")).toBe(requireFromUi.resolve("lexical"));
|
||||
});
|
||||
|
|
@ -51,16 +84,13 @@ describe("lexical single copy", () => {
|
|||
// @lexical/link carries the mention-aware LinkNode the app subclasses, so
|
||||
// a version split here breaks node identity even with one core copy.
|
||||
expect(versionOf("@lexical/link", requireFromUi)).toBe(core);
|
||||
// pnpm resolved this copy from the range @mdxeditor/editor declares, and
|
||||
// the assertion above rules out an override bypassing that range. So an
|
||||
// equal version here also proves the app sits on a line the editor
|
||||
// supports, without this test having to parse a semver range itself.
|
||||
expect(versionOf("lexical", requireFromMdxEditor)).toBe(core);
|
||||
// @lexical/extension owns the LexicalBuilder invariant that throws on a
|
||||
// mixed graph, and it is reached transitively rather than declared.
|
||||
expect(versionOf("@lexical/extension", requireFromMdxEditor)).toBe(core);
|
||||
});
|
||||
|
||||
it("satisfies the editor's declared lexical range", () => {
|
||||
const declared = manifestOf("@mdxeditor/editor", requireFromUi).dependencies?.lexical;
|
||||
const [major, minor] = versionOf("lexical", requireFromUi).split(".");
|
||||
// A caret range on a 0.x version pins the minor: ^0.48.0 means <0.49.0.
|
||||
expect(declared).toBe(`^${major}.${minor}.0`);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue