Skip to content

Commit 359cc18

Browse files
authored
feat(vnext): add source coordinate primitives (#173)
## Summary - publish the minimal `SqlTextRange` UTF-16 contract and make `SqlTextChange` extend it - add internal immutable identity/masked source snapshots with explicit `originalText` and `analysisText` - validate and freeze bounded embedded regions without invoking accessors or Proxy `get` traps - preserve length and every CR/LF offset while masking every other UTF-16 code unit - store source snapshots in document sessions, reusing them only for context-only updates - document the intentional boundary: no public transformer or source-map SPI yet - request Copilot once per PR, without later re-requests ## Invariants and risk - ranges are safe-integer, in-bounds, half-open UTF-16 offsets - embedded regions are non-empty, ordered, non-overlapping, and capped at 10,000 - source text is capped at 16 Mi UTF-16 code units - masking uses bounded 64 Ki-code-unit chunks and cannot allocate per newline - arbitrary values thrown by hostile Proxy traps are normalized without inspecting them - document updates create a new source snapshot; context-only updates retain the exact source identity ## Evidence - `pnpm run typecheck` - `pnpm exec oxlint` - `pnpm run test:coverage` (713 passed, 1 governed expected failure) - changed runtime coverage: 98.78% statements, 97.75% branches, 100% functions, 98.76% lines - deterministic randomized UTF-16 masking/range round trips - full 16,777,216-code-unit CRLF mask and 10,000-region smoke cases - full-limit newline mask succeeds under `node --max-old-space-size=128` - `pnpm run test:browser` - `pnpm run test:package` - `pnpm run test:integrity` - `pnpm run demo` ## Adversarial review Two independent exact-head reviewers challenged correctness and performance/API design. The first loop found a newline-density memory amplification and hostile thrown-Proxy escape. Both were redesigned and regression-tested. Both reviewers approve exact SHA `1993a6cdca85ec9d3b8a1ddaab5dda0188b85fab` with no blocker, high, or medium findings. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Add internal source coordinate primitives and length-preserving masking to support safe UTF-16 ranges and embedded-region analysis. Export `SqlTextRange` and update sessions to use immutable source snapshots, reusing them for context-only updates. - New Features - Export `SqlTextRange`; make `SqlTextChange` extend it. - Add immutable source snapshots with `originalText` and `analysisText`, plus length-preserving masking of embedded regions (CR/LF offsets preserved). No transformer or source-map SPI yet. - Validate and freeze inputs with strict caps (16 Mi code units, 10k regions) and accessor-safe normalization. - Store source snapshots in document sessions and reuse them on context-only updates. <sup>Written for commit 1993a6c. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/marimo-team/codemirror-sql/pull/173?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
1 parent 846c90b commit 359cc18

12 files changed

Lines changed: 956 additions & 71 deletions

File tree

docs/adr/0001-language-service-and-session.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,12 @@ adjacent SQL tokens. More complex transformers use a versioned map whose
389389
segments are ordered, non-overlapping, and validated for original/generated
390390
coverage.
391391

392+
The initial internal source primitive implements identity snapshots and this
393+
length-preserving masking only. It does not expose a transformer or source-map
394+
SPI. Sessions create a new source snapshot for document updates and reuse the
395+
existing snapshot for context-only updates. Non-identity generated/reordered
396+
source remains deferred until a concrete consumer validates the segment model.
397+
392398
Edits crossing an ambiguous or unmapped boundary are rejected. Mapping failure
393399
is unavailable analysis, not invalid SQL.
394400

docs/vnext/session-primitives.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ updates, opaque revisions, dialect registration, and lifecycle management. It
88
does not yet provide parsing, completion, diagnostics, hover, or navigation.
99
Those methods will be added only with working vertical slices.
1010

11+
See [source coordinates](./source-coordinates.md) for the shared UTF-16 range
12+
contract and the internal immutable source-snapshot model.
13+
1114
## Example
1215

1316
```ts

docs/vnext/source-coordinates.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# vNext Source Coordinates
2+
3+
Status: experimental core primitive
4+
5+
`SqlTextRange` is the public coordinate primitive:
6+
7+
```ts
8+
interface SqlTextRange {
9+
readonly from: number;
10+
readonly to: number;
11+
}
12+
```
13+
14+
Ranges are half-open UTF-16 offsets. Both ends are safe integers and satisfy
15+
`0 <= from <= to <= document.length`. Empty ranges, including the range at EOF,
16+
are valid. `SqlTextChange` extends this shape with `insert` and continues to use
17+
pre-update document coordinates.
18+
19+
The service internally owns an immutable source snapshot with separately named
20+
`originalText` and `analysisText`. A document update creates a new identity
21+
snapshot; a context-only update reuses the same source object. This separation
22+
allows later analysis transforms without making providers depend on editor
23+
state or ambiguous coordinate spaces.
24+
25+
## Length-preserving masking
26+
27+
The first internal transform masks explicit embedded regions:
28+
29+
- Regions are non-empty, ordered, non-overlapping, and in bounds.
30+
- Region and range inputs are copied into fresh frozen values.
31+
- Each non-CR/LF UTF-16 code unit becomes one space.
32+
- Every CR and LF code unit stays at its exact offset.
33+
- Astral characters therefore become two spaces, while a lone surrogate becomes
34+
one space.
35+
- `analysisText.length` always equals `originalText.length`, so range mapping is
36+
identity-preserving while still returning a fresh validated range.
37+
38+
At most 10,000 regions and 16 Mi UTF-16 code units are accepted. Masking uses
39+
bounded 64 Ki-code-unit chunks, including for newline-dense input, rather than
40+
a per-code-unit or per-newline array.
41+
42+
The source snapshot, embedded-region model, masking factory, and mapping
43+
functions remain internal. This slice intentionally does not publish a source
44+
transformer or source-map SPI. Generated or reordered source requires a
45+
versioned segment-map design and evidence from real consumers before becoming
46+
public.

implementation.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -590,8 +590,8 @@ disposition. Validate them through a protected reusable workflow from the base
590590
branch so a PR cannot approve itself. Maintainers approve classification
591591
overrides and rejected blocking findings.
592592

593-
Request GitHub Copilot review on every medium or large PR after the head commit
594-
is ready for review:
593+
Request GitHub Copilot review exactly once on every medium or large PR, after
594+
the PR has a coherent head that is ready for review:
595595

596596
```bash
597597
gh pr edit <number> --add-reviewer @copilot
@@ -600,8 +600,9 @@ gh pr edit <number> --add-reviewer @copilot
600600
Copilot is an additional advisory review, not one of the two independent
601601
commit-bound adversarial attestations and not a substitute for human approval.
602602
Resolve or explicitly disposition its actionable comments in the finding
603-
ledger. Re-request review after a material rewrite when the prior comments no
604-
longer cover the current head.
603+
ledger. Do not re-request Copilot after later pushes or material rewrites; the
604+
commit-bound adversarial reviewers and required CI provide exact-head
605+
revalidation.
605606

606607
## CI architecture
607608

@@ -814,7 +815,7 @@ Before semantic implementation:
814815
12. Add the invariant registry and test-suite integrity checks.
815816
13. Add PR risk template and review-packet generation.
816817
14. Add protected, commit-bound two-reviewer checks.
817-
15. Automate GitHub Copilot review requests for medium and large PRs.
818+
15. Automate one GitHub Copilot review request per medium or large PR.
818819
16. Add verified-artifact provenance to release CI.
819820
17. Add nightly mutation, fuzz, cross-browser, leak, and performance workflows.
820821
18. Add expiring scheduled-health and deduplicated failure issues.

scripts/package-smoke.mjs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,7 @@ import {
210210
createSqlLanguageService,
211211
defineSqlDialect,
212212
type SqlDocumentContext,
213+
type SqlTextRange,
213214
} from "@marimo-team/codemirror-sql/vnext";
214215
import commonKeywords from "@marimo-team/codemirror-sql/data/common-keywords.json" with { type: "json" };
215216
import duckdbKeywords from "@marimo-team/codemirror-sql/data/duckdb-keywords.json" with { type: "json" };
@@ -230,6 +231,7 @@ const session = service.openDocument({
230231
context: { dialect: "duckdb", engine: "local" },
231232
text: "SELECT 1",
232233
});
234+
const range: SqlTextRange = { from: 0, to: 6 };
233235
session.update({
234236
kind: "document",
235237
baseRevision: session.revision,
@@ -238,6 +240,7 @@ session.update({
238240
239241
void extensions;
240242
void parser;
243+
void range;
241244
void session;
242245
void BigQueryDialect;
243246
void DremioDialect;

src/vnext/__tests__/session.test.ts

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,28 @@ describe("document revisions", () => {
187187
});
188188
expect(second).not.toBe(first);
189189
});
190+
191+
it("reuses source snapshots only for context-only updates", () => {
192+
const { session } = openSession("SELECT 1");
193+
const initialSource = session.snapshotForTesting.source;
194+
expect(Object.isFrozen(initialSource)).toBe(true);
195+
expect(initialSource.analysisText).toBe(initialSource.originalText);
196+
197+
session.update({
198+
kind: "context",
199+
baseRevision: session.revision,
200+
context: { dialect: "duckdb", engine: "remote" },
201+
});
202+
expect(session.snapshotForTesting.source).toBe(initialSource);
203+
204+
session.update({
205+
kind: "document",
206+
baseRevision: session.revision,
207+
document: { kind: "changes", changes: [] },
208+
});
209+
expect(session.snapshotForTesting.source).not.toBe(initialSource);
210+
expect(session.snapshotForTesting.source.originalText).toBe("SELECT 1");
211+
});
190212
});
191213

192214
describe("document changes", () => {
@@ -204,7 +226,7 @@ describe("document changes", () => {
204226
},
205227
});
206228

207-
expect(session.snapshotForTesting.text).toBe(
229+
expect(session.snapshotForTesting.source.originalText).toBe(
208230
"SELECT customers.id FROM customers",
209231
);
210232
});
@@ -219,7 +241,7 @@ describe("document changes", () => {
219241
changes: [{ from: 1, insert: "X", to: 3 }],
220242
},
221243
});
222-
expect(session.snapshotForTesting.text).toBe("AXB");
244+
expect(session.snapshotForTesting.source.originalText).toBe("AXB");
223245
});
224246

225247
it("keeps same-position insertions ordered", () => {
@@ -235,7 +257,7 @@ describe("document changes", () => {
235257
],
236258
},
237259
});
238-
expect(session.snapshotForTesting.text).toBe("A12B");
260+
expect(session.snapshotForTesting.source.originalText).toBe("A12B");
239261
});
240262

241263
it("supports adjacent replacements and document boundaries", () => {
@@ -253,7 +275,7 @@ describe("document changes", () => {
253275
],
254276
},
255277
});
256-
expect(session.snapshotForTesting.text).toBe("XYZ!");
278+
expect(session.snapshotForTesting.source.originalText).toBe("XYZ!");
257279
});
258280

259281
it("deletes the entire document", () => {
@@ -263,7 +285,7 @@ describe("document changes", () => {
263285
baseRevision: session.revision,
264286
document: { kind: "changes", changes: [{ from: 0, insert: "", to: 8 }] },
265287
});
266-
expect(session.snapshotForTesting.text).toBe("");
288+
expect(session.snapshotForTesting.source.originalText).toBe("");
267289
});
268290

269291
it("allows edits at every UTF-16 boundary", () => {
@@ -280,7 +302,9 @@ describe("document changes", () => {
280302
],
281303
},
282304
});
283-
expect(session.snapshotForTesting.text).toBe("A\uD83DXe\u0301\nZ!\uD800");
305+
expect(session.snapshotForTesting.source.originalText).toBe(
306+
"A\uD83DXe\u0301\nZ!\uD800",
307+
);
284308
});
285309

286310
it.each([
@@ -305,7 +329,7 @@ describe("document changes", () => {
305329
});
306330

307331
expect(session.revision).toBe(revision);
308-
expect(session.snapshotForTesting.text).toBe("ABC");
332+
expect(session.snapshotForTesting.source.originalText).toBe("ABC");
309333
});
310334

311335
it("rejects unordered and overlapping changes atomically", () => {
@@ -325,7 +349,7 @@ describe("document changes", () => {
325349
});
326350
});
327351
expect(session.revision).toBe(revision);
328-
expect(session.snapshotForTesting.text).toBe("ABCDE");
352+
expect(session.snapshotForTesting.source.originalText).toBe("ABCDE");
329353
});
330354

331355
it("rejects non-string inserts from JavaScript callers", () => {
@@ -356,7 +380,7 @@ describe("document changes", () => {
356380
session.update({ kind: "document", baseRevision: revision, document } as never);
357381
});
358382
expect(session.revision).toBe(revision);
359-
expect(session.snapshotForTesting.text).toBe("ABC");
383+
expect(session.snapshotForTesting.source.originalText).toBe("ABC");
360384
});
361385

362386
it("rejects an empty JavaScript update", () => {
@@ -423,7 +447,7 @@ describe("document changes", () => {
423447
baseRevision: session.revision,
424448
document: { kind: "replace", text: "recovered" },
425449
});
426-
expect(session.snapshotForTesting.text).toBe("recovered");
450+
expect(session.snapshotForTesting.source.originalText).toBe("recovered");
427451
});
428452

429453
it.each([null, "invalid", 42])(
@@ -454,7 +478,7 @@ describe("document changes", () => {
454478
session.update(accessor as never);
455479
});
456480
expect(invoked).toBe(false);
457-
expect(session.snapshotForTesting.text).toBe("ABC");
481+
expect(session.snapshotForTesting.source.originalText).toBe("ABC");
458482
});
459483

460484
it("rejects undefined and accessor optional update fields", () => {
@@ -469,7 +493,7 @@ describe("document changes", () => {
469493
} as never);
470494
});
471495
expect(session.revision).toBe(revision);
472-
expect(session.snapshotForTesting.text).toBe("ABC");
496+
expect(session.snapshotForTesting.source.originalText).toBe("ABC");
473497

474498
let invoked = false;
475499
const update = {
@@ -486,7 +510,7 @@ describe("document changes", () => {
486510
});
487511
expect(invoked).toBe(false);
488512
expect(session.revision).toBe(revision);
489-
expect(session.snapshotForTesting.text).toBe("ABC");
513+
expect(session.snapshotForTesting.source.originalText).toBe("ABC");
490514
});
491515

492516
it("rejects accessor document fields without invoking them", () => {
@@ -571,7 +595,7 @@ describe("document changes", () => {
571595

572596
expect(nestedError?.code).toBe("reentrant-update");
573597
expect(session.isCurrent(revision)).toBe(true);
574-
expect(session.snapshotForTesting.text).toBe("outer");
598+
expect(session.snapshotForTesting.source.originalText).toBe("outer");
575599
});
576600

577601
it("cannot commit an update after reentrant disposal", () => {
@@ -600,7 +624,7 @@ describe("document changes", () => {
600624
});
601625
});
602626
expect(session.isCurrent(revision)).toBe(false);
603-
expect(session.snapshotForTesting.text).toBe("ABC");
627+
expect(session.snapshotForTesting.source.originalText).toBe("ABC");
604628
});
605629

606630
it("bounds fragmented and oversized document updates", () => {
@@ -631,7 +655,7 @@ describe("document changes", () => {
631655
},
632656
});
633657
});
634-
expect(session.snapshotForTesting.text).toBe("");
658+
expect(session.snapshotForTesting.source.originalText).toBe("");
635659
});
636660
});
637661

@@ -746,7 +770,7 @@ describe("document context", () => {
746770

747771
expect(session.snapshotForTesting).toMatchObject({
748772
context: { dialect: "postgres", engine: "warehouse" },
749-
text: "SELECT 2",
773+
source: { originalText: "SELECT 2" },
750774
});
751775

752776
const snapshot = session.snapshotForTesting;
@@ -768,7 +792,7 @@ describe("document context", () => {
768792
baseRevision: session.revision,
769793
context: { dialect: "postgres", engine: "warehouse" },
770794
});
771-
expect(session.snapshotForTesting.text).toBe("SELECT 1");
795+
expect(session.snapshotForTesting.source.originalText).toBe("SELECT 1");
772796
expect(session.snapshotForTesting.context.dialect).toBe("postgres");
773797
});
774798

@@ -821,7 +845,7 @@ describe("document context", () => {
821845
} as never);
822846
});
823847
expect(session.revision).toBe(revision);
824-
expect(session.snapshotForTesting.text).toBe("SELECT 1");
848+
expect(session.snapshotForTesting.source.originalText).toBe("SELECT 1");
825849
});
826850

827851
it("rejects accessors without invoking them", () => {

0 commit comments

Comments
 (0)