Replace drifted GCI optionality sources with a typed header parser and registry - #512
Draft
MatiasFernandez wants to merge 14 commits into
Draft
Replace drifted GCI optionality sources with a typed header parser and registry#512MatiasFernandez wants to merge 14 commits into
MatiasFernandez wants to merge 14 commits into
Conversation
MatiasFernandez
force-pushed
the
mfernandez/migrate-gci-tests-phase-6
branch
3 times, most recently
from
September 1, 2026 19:52
675b080 to
aaea483
Compare
MatiasFernandez
changed the title
Parses vendor/gci-headers/<revision>/gcits.hf to determine which GCI entry points are declared per vendored revision and whether each is gated behind #if defined(FLG_UNIX). This is the first of Phase 6's two typed modules replacing the four drifted, hand-maintained sources of optionality truth in gciLibrary.ts and its tests. Self-checks its own declaration count against raw EXTERN_GCI_DEC occurrences so a regex that silently stops matching cannot pass unnoticed.
Two silent misclassifications in the gcits.hf parser, both latent against the currently vendored headers but waiting for the next vendor drop: - A frame modelled `#else` as an inversion of `unixOnly`, so an `#elif` overwrote it and the chain forgot what earlier branches had established. `#if !defined(FLG_UNIX) / #elif X / #else` left both later branches classified non-UNIX, when reaching either one means FLG_UNIX is defined. A frame now carries what *reaching* a later branch implies, and that accumulates across #elif. - `#if 0` is the other idiomatic way a vendor retires a declaration in place, and it walked straight through into the map as a live symbol. Dead regions are now skipped whole, before the occurrence counter, so the "silently missed one" invariant still holds. Also: accept `defined FLG_UNIX` without parentheses, and name the revision and the likely cause in the missing-header and unparseable-declaration errors. On the tests, the fs mock is gone. It was primed per call with mockReturnValueOnce, which left a queued listing to leak into whichever shuffled test ran next, and it stubbed out the readdir and filter behavior the tests existed to check. vendoredRevisions takes a root seam instead and the tests build a real temp tree. New coverage for CRLF input, condition spacing, and the argument list of a declaration that is itself UNIX-only. The parser then collapses four per-line regex execs into one directive reader, replaces the Object.assign frame mutation with three named transitions, and reduces seven mutable loop locals to two by making "not capturing" unrepresentable. A third silent misclassification, same family: the throw-rather- than-guess safety net only fired for conditions mentioning the literal FLG_UNIX token, so a declaration gated on any other platform flag (FLG_MSWIN32, a typo'd lookalike, ...) fell through as fully unconstrained instead. Broaden the check to the vendor's FLG_* naming convention generally, and make it structural rather than an immediate throw: a condition can be unclassifiable without erroring (an empty gated region, or one nested inside a frame that already proves unixOnly), and only errors once a declaration actually lands inside one whose platform requirement is truly undetermined. Also join backslash-continued conditions before classifying them, and extend the check to #ifdef/#ifndef, which previously had none.
The rule's default caps expect() at exactly 1 argument, which also blocks Chai's legitimate two-argument message form that vitest's own type definitions preserve. Raising maxArgs to 2 keeps the arity guard against real mistakes (0 or 3+ args) while allowing descriptive failure messages. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MatiasFernandez
force-pushed
the
mfernandez/migrate-gci-tests-phase-6
branch
from
September 2, 2026 00:35
aaea483 to
76c27c8
Compare
Introduces GCI_OPTIONAL_FUNCTIONS as the single source of truth for which GciTs* bindings may be absent from a loaded library, replacing scattered optionalFunc() call sites and hand-claimed version floors. Verifies every entry against vendor/gci-headers/ in both directions, grouped by why a symbol is optional (version-gated, platform-gated, both, or pending removal) rather than by which fields happen to be set. Also checks completeness both ways: every registry entry matches exactly one category, and every symbol the headers themselves mark optional has a registry entry.
The 19 optional bindings collapse into one `_optional` object literal
keyed by `GciOptionalFunctionName`, so a missing key, a stale key, and a
key/name mismatch are all compile errors instead of drift nobody
notices. `optionalFunc` and `isAvailable` narrow from `string` to that
key type, which also rejects a typo'd name at compile time.
Fold the `GciTsNbLogin` / `GciTsNbLoginFinished` try/catch into the
literal so `_missing` finally records them: `isAvailable('GciTsNbLogin')`
returned `true` on Windows, where the symbol is absent. Their two
hand-written null guards go — `optionalFunc`'s stub throws the identical
message. `supportsNonBlockingLogin()` now reads from `isAvailable`.
Promote `GciI32ToOop` / `GciTsI32ToOop` to required. Their optional
treatment guarded against a pre-3.6.2 library and is unreachable: both
are declared in all 10 vendored revisions, and 13 symbols bound as
required are absent from 3.4.5, so the constructor throws on one of
those first. Rewrite the three comments claiming that floor.
`gciVersionGated.test.ts` and `__tests__/gci/gciSpecials.test.ts` do not
pass yet; the next two commits own them.
Pure rename, no content change, so history follows through the move ahead of rewriting it to be driven by the optional-functions registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the hand-maintained OPTIONAL_FUNCTIONS array in
missingGciFunctions.test.ts with GCI_OPTIONAL_FUNCTIONS itself, so the
mocked "missing symbol" test can no longer drift from what gciLibrary.ts
actually treats as optional. A Record<GciOptionalFunctionName, () =>
unknown> invocation table makes covering all 19 bindings a compile-time
requirement instead of the previous 10-of-19 hand-written cases, and adds
the isAvailable('GciTsNbLogin') / supportsNonBlockingLogin() assertions
that catch the Windows non-blocking-login bug directly.
gciVersionGated.test.ts now imports GCI_OPTIONAL_FUNCTIONS instead of
regex-scraping gciLibrary.ts, and gates on entries carrying addedIn
(absent from 3.6.2) rather than treating every optional binding as
version-gated.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fold gci/gciSpecials.test.ts's GciI32ToOop / GciTsI32ToOop describe into the existing gciSpecials.integration.test.ts, joining the shared useIntegrationTest harness instead of opening its own session (required by the no-restricted-syntax rule against a second GciLibrary instance). The itIfPresent/hasI32ToOop gate is dropped in the same stroke: Phase 6 already narrowed isAvailable() to GciOptionalFunctionName, and neither symbol is in that registry anymore, so the gate no longer type-checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
'encodes when the library exports it, otherwise throws' asserted both branches of an if/isAvailable/else, so it could never fail — the exact anti-pattern the optionality-registry migration is meant to close out. Delete it rather than migrate it; the preceding tests already cover the encoding behavior unconditionally now that both symbols are required. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MatiasFernandez
force-pushed
the
mfernandez/migrate-gci-tests-phase-6
branch
2 times, most recently
from
September 2, 2026 04:01
3d715d9 to
6d6d7d8
Compare
The file had no top-level docstring: a reader hit the koffi mock with no statement of purpose. It now leads with the role — proving the optionalFunc contract from the library's own side — and names the one thing the setup doesn't say out loud, that refusing all 19 symbols is a synthetic worst case rather than a real Windows DLL, which is missing only the five FLG_UNIX ones. The describe title claimed the same thing and was the first line a reader saw, so it's renamed to describe the scenario the mock actually builds. The two Windows-specific tests keep their own names. Remaining comments drop the restatements (the banner, and a beforeEach comment that was the test name below it in other words) and keep what isn't visible in the code: why the GciLibrary import sits mid-file, and the real bug the isAvailable test guards — GciTsNbLogin was once bound by a bare try/catch that never recorded it as missing, so isAvailable answered true on Windows and the login path never fell back.
Every `_optional` entry writes the symbol three times -- as the key, as `optionalFunc`'s `name` argument, and inside the koffi prototype -- and only the first two are related by the `__gciOptional` brand. A copy-pasted entry naming a neighbouring symbol would bind the wrong native function while `_missing` and `isAvailable` reported the one it was keyed by, and `optionalFunc`'s catch would hide it: the whole suite stays green today if `GciTsNbPoll` is given `GciTsKeepAliveCount`'s prototype. Extract the declared symbol from the prototype and compare it exactly. A substring check is not enough -- `GciTsNbLogin` is a prefix of both `GciTsNbLogin_` and `GciTsNbLoginFinished`, three adjacent entries, so it would miss the likeliest divergence. The check sits outside the try, or the catch turns it into a silent "missing" record under the wrong name. No new assertion is needed to cover the real literal: every test that constructs a `GciLibrary` now validates all 19 entries. The new file covers the guard itself, including its placement, and joins the existing whole-file exemptions from the harness-session rule for the same reason as the other two -- koffi is mocked, so there is no session to arm.
MatiasFernandez
force-pushed
the
mfernandez/migrate-gci-tests-phase-6
branch
from
September 2, 2026 04:28
6d6d7d8 to
1c07fca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the four drifted, hand-maintained sources of GCI optionality truth in
gciLibrary.tsand its tests with two typed modules, each checked against the vendored headers by a different mechanism, plus the tests and docs that depend on them.headerDeclarations.tsparsesvendor/gci-headers/<revision>/gcits.hfforEXTERN_GCI_DECdeclarations, tracking#ifnesting so it can reportFLG_UNIX-gating too. Self-checks its own declaration count against raw regex occurrences so a silently-broken match can't pass unnoticed.optionalFunctions.tsis the hand-written, machine-verified registry of whichGciTs*bindings may be absent, and why (addedIn/absentOn: 'win32'/removedIn).optionalFunctions.headers.test.tsasserts every field against the parsed headers in both directions.gciLibrary.ts's optional bindings are now keyed off the registry's type, so an unregistered optional binding (or a registry entry with no binding) is a compile error instead of a runtime surprise.gciOptionalFunctions,gciSpecialsI32ToOop, transcript sink) are migrated onto the integration harness and out of theclient/src/__tests__/gci/project, per the ongoing migration.docs/explanation/gci-version-compatibility.mddocuments the whole chain — vendored headers → parser → registry →gciLibrary.ts— and which mechanism catches which mistake.One production behavior change
GciI32ToOop/GciTsI32ToOopmove fromoptionalFuncto requiredthis.lib.func, so alibrary missing either now fails
GciLibraryconstruction (a failed login) instead ofdegrading to a stub that throws only if called.
The optional binding existed to keep a pre-3.6.2 library loadable; the deleted comment named
3.4.5. But of the 84 symbols
gciLibrary.tsbinds as required, seven aren't declared in3.4.5's headers at all — and four are on the non-blocking execution path with production
callers:
GciTsNbExecute(codeExecutor, notebook,browserQueries),GciTsNbResult(
nbRunner,transcriptSink,debugQueries),GciTsNbPerform(debugQueries) andGciTsSocket(nbRunner). The other three —GciTsI32ToOop,GciTsNbLogout,GciTsDirtyObjsInit— have no production callers.So a 3.4.5 library would have constructed fine and then failed on the first evaluation,
debugger step or transcript read. The tolerance bought nothing but the suggestion otherwise.
3.6.2 is the declared minimum (
versionManager.ts) and is CI-covered on Linux and Windows,so this is a deliberate narrowing: a hard floor for construction, not just for features.
Known gaps
gciVersionGated.test.tsonly gates the 3.6.2 floor (addedIn), not every optional function, and doesn't verifyabsentOn: 'win32'gating at all. Broadening it is separate follow-up work.eslint.config.mjscan't import the TypeScript registry. Worth revisiting if the config ever moves to TS.