Skip to content

Commit 846c90b

Browse files
authored
feat(vnext): add atomic document sessions (#172)
## Summary - add the framework-independent `@marimo-team/codemirror-sql/vnext` entry point - implement immutable dialect registration, one session per document, opaque revision identity/metadata, discriminated atomic updates, and terminal disposal - validate and own bounded plain-data contexts; normalize hostile JavaScript inputs and guard reentrant update/open lifecycle races - add compile-only API tests, 83 runtime invariant tests, current-API documentation, and packed isolation smoke coverage without CodeMirror or node-sql-parser available ## Evidence - `pnpm run typecheck` - `pnpm exec oxlint` - `pnpm run test:coverage` (670 passed, 1 governed expected failure) - changed vNext runtime coverage: 99% statements, 98.47% branches, 100% functions, 98.99% lines - `pnpm run test:browser` - `pnpm run test:package` - `pnpm run test:integrity` - `pnpm run demo` ## Adversarial review Two independent reviewers challenged correctness and API design. The first loop found and reproduced reentrant revision overwrite, service-open disposal, dialect TOCTOU, variance, and optional-property issues. The implementation and tests were redesigned; both reviewers now approve with no blocker/high findings. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds vNext atomic document sessions in `@marimo-team/codemirror-sql/vnext` with immutable dialects, per-document sessions, and strict revision/lifecycle invariants. Strengthens smoke tests with dependency isolation and rollback checks, and exports `./vnext` for consumers. - **New Features** - Public service/session operations are exposed as readonly, bound function fields to remain safe when passed as callbacks or destructured. - Packaging/docs/tests: new session primitives guide, `package.json` `./vnext` subpath export, and smoke tests that run without `@codemirror` or `node-sql-parser`, verify `vnext` exports, revision identity, and validate isolation rollback. - **Bug Fixes** - Hardened API invariants: reentrant-update guard, early stale-revision checks before context inspection, idempotent terminal disposal for session/service, stricter optional-field handling (`exactOptionalPropertyTypes`), and consistent `SqlSessionError` codes when normalizing accessors/proxies. <sup>Written for commit ce6d09a. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/marimo-team/codemirror-sql/pull/172?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 c96bbeb commit 846c90b

11 files changed

Lines changed: 2651 additions & 22 deletions

File tree

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

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,10 @@ The session updates text and context atomically:
125125

126126
```ts
127127
session.update({
128+
kind: "document",
128129
baseRevision: session.revision,
129130
document: {
131+
kind: "changes",
130132
changes: [{ from: 14, to: 19, insert: "customers" }],
131133
},
132134
context: nextContext,
@@ -147,13 +149,15 @@ update without partial mutation.
147149
The public revision is an opaque immutable service-generated token. Callers
148150
cannot construct one or supply their own version number.
149151

150-
Internally, the revision records:
152+
Internally, the session snapshot associated with the revision tracks:
151153

152-
- Session identity
153154
- Monotonic sequence
154155
- Document revision
155156
- Context and template revision
156-
- Relevant environment/catalog epoch
157+
158+
Relevant environment/catalog epochs will join that snapshot when those
159+
subsystems are implemented. The revision token itself remains only an opaque
160+
immutable identity; it does not duplicate snapshot metadata.
157161

158162
The public model is deliberately global per session: any accepted text,
159163
context, template, relevant catalog, or provider-configuration invalidation
@@ -251,34 +255,34 @@ Explicit methods are preferred over one generic `request({ kind })` API:
251255
interface SqlDocumentSession<Context extends SqlDocumentContext> {
252256
readonly revision: SqlRevision;
253257

254-
update(update: SqlDocumentUpdate<Context>): SqlRevision;
258+
readonly update: (update: SqlDocumentUpdate<Context>) => SqlRevision;
255259

256-
complete(
260+
readonly complete: (
257261
request: SqlCompletionRequest,
258-
): Promise<SqlRequestResult<SqlCompletionList>>;
262+
) => Promise<SqlRequestResult<SqlCompletionList>>;
259263

260-
diagnostics(
264+
readonly diagnostics: (
261265
request?: SqlDiagnosticsRequest,
262-
): Promise<SqlRequestResult<SqlDiagnosticSet>>;
266+
) => Promise<SqlRequestResult<SqlDiagnosticSet>>;
263267

264-
hover(
268+
readonly hover: (
265269
request: SqlPositionRequest,
266-
): Promise<SqlRequestResult<SqlHover | null>>;
270+
) => Promise<SqlRequestResult<SqlHover | null>>;
267271

268-
definition(
272+
readonly definition: (
269273
request: SqlPositionRequest,
270-
): Promise<SqlRequestResult<readonly SqlLocation[]>>;
274+
) => Promise<SqlRequestResult<readonly SqlLocation[]>>;
271275

272-
references(
276+
readonly references: (
273277
request: SqlPositionRequest,
274-
): Promise<SqlRequestResult<readonly SqlLocation[]>>;
278+
) => Promise<SqlRequestResult<readonly SqlLocation[]>>;
275279

276-
format(
280+
readonly format: (
277281
request?: SqlFormatRequest,
278-
): Promise<SqlRequestResult<readonly SqlTextEdit[]>>;
282+
) => Promise<SqlRequestResult<readonly SqlTextEdit[]>>;
279283

280-
isCurrent(revision: SqlRevision): boolean;
281-
dispose(): void;
284+
readonly isCurrent: (revision: SqlRevision) => boolean;
285+
readonly dispose: () => void;
282286
}
283287
```
284288

docs/vnext/session-primitives.md

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
# vNext Session Primitives
2+
3+
Status: experimental walking skeleton
4+
Import: `@marimo-team/codemirror-sql/vnext`
5+
6+
This entry point currently provides document ownership, atomic text/context
7+
updates, opaque revisions, dialect registration, and lifecycle management. It
8+
does not yet provide parsing, completion, diagnostics, hover, or navigation.
9+
Those methods will be added only with working vertical slices.
10+
11+
## Example
12+
13+
```ts
14+
import {
15+
createSqlLanguageService,
16+
defineSqlDialect,
17+
type SqlDocumentContext,
18+
} from "@marimo-team/codemirror-sql/vnext";
19+
20+
interface AppSqlContext extends SqlDocumentContext {
21+
readonly engine: string;
22+
}
23+
24+
const service = createSqlLanguageService<AppSqlContext>({
25+
dialects: [
26+
defineSqlDialect({ id: "duckdb", displayName: "DuckDB" }),
27+
],
28+
});
29+
30+
const session = service.openDocument({
31+
text: "SELECT * FROM users",
32+
context: { dialect: "duckdb", engine: "local" },
33+
});
34+
35+
const revision = session.update({
36+
kind: "document",
37+
baseRevision: session.revision,
38+
document: {
39+
kind: "changes",
40+
changes: [{ from: 14, to: 19, insert: "customers" }],
41+
},
42+
});
43+
44+
if (session.isCurrent(revision)) {
45+
// Results produced for this revision may still be applied.
46+
}
47+
48+
session.dispose();
49+
service.dispose();
50+
```
51+
52+
Use `{ kind: "replace", text }` for full replacement and
53+
`{ kind: "context", baseRevision, context }` for a context-only update.
54+
55+
## Contract
56+
57+
- Every accepted update creates a new frozen, service-issued revision.
58+
- `baseRevision` is checked by identity and cannot come from another session.
59+
- Incremental ranges are ordered, non-overlapping, half-open UTF-16 offsets in
60+
the pre-update document.
61+
- Text and context are validated completely before the session snapshot changes.
62+
- Context is structured-cloned and recursively frozen. Accepted values are
63+
finite primitives, arrays, and string-keyed plain objects. Cycles and shared
64+
references are retained. Accessors, symbols, functions, class instances,
65+
typed collections, and non-finite numbers are rejected.
66+
- Public service and session operations remain bound when passed as callbacks
67+
or destructured.
68+
- Session and service disposal are idempotent and terminal.
69+
- Synchronous public failures use `SqlSessionError` and a stable `code`; caught
70+
platform errors are not exposed as causes.
71+
72+
The safety envelope currently permits at most 10,000 changes in one update, a
73+
16 Mi-code-unit document, 1,000 registered dialects, and bounded context graph
74+
depth, size, properties, and string data. These are defensive walking-skeleton
75+
limits, not release performance claims.
76+
77+
The `/vnext` name is provisional until the packaging ADR fixes the next-major
78+
subpath layout.

implementation.md

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,19 @@ 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:
595+
596+
```bash
597+
gh pr edit <number> --add-reviewer @copilot
598+
```
599+
600+
Copilot is an additional advisory review, not one of the two independent
601+
commit-bound adversarial attestations and not a substitute for human approval.
602+
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.
605+
593606
## CI architecture
594607

595608
### Required fast PR lane
@@ -801,10 +814,11 @@ Before semantic implementation:
801814
12. Add the invariant registry and test-suite integrity checks.
802815
13. Add PR risk template and review-packet generation.
803816
14. Add protected, commit-bound two-reviewer checks.
804-
15. Add verified-artifact provenance to release CI.
805-
16. Add nightly mutation, fuzz, cross-browser, leak, and performance workflows.
806-
17. Add expiring scheduled-health and deduplicated failure issues.
807-
18. Define the supported runtime and dependency matrix.
817+
15. Automate GitHub Copilot review requests for medium and large PRs.
818+
16. Add verified-artifact provenance to release CI.
819+
17. Add nightly mutation, fuzz, cross-browser, leak, and performance workflows.
820+
18. Add expiring scheduled-health and deduplicated failure issues.
821+
19. Define the supported runtime and dependency matrix.
808822

809823
## Final recommendation
810824

package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,12 @@
7878
"default": "./dist/dialects/index.js"
7979
}
8080
},
81+
"./vnext": {
82+
"import": {
83+
"types": "./dist/vnext/index.d.ts",
84+
"default": "./dist/vnext/index.js"
85+
}
86+
},
8187
"./data/common-keywords.json": "./src/data/common-keywords.json",
8288
"./data/duckdb-keywords.json": "./src/data/duckdb-keywords.json"
8389
},

0 commit comments

Comments
 (0)