Enum parity CI check (#95) + model Steam Deck as Pc (#103) - #106
Open
mforce wants to merge 24 commits into
Open
Conversation
Closes #95. Adds two independent checks that both fail on enum drift: - Collectify.Domain.Enums.EnumParity: machine-readable source of truth (member names + numeric values) derived via reflection. - Collectify.Tests.EnumParityTests: xUnit theory comparing each client table in services/types.ts against the server enum (set of members + numeric values for flags enums), plus a registration-completeness test so a new server enum cannot escape the check. - src/client/scripts/check-enum-parity.mjs: dependency-free Node script (parses the .cs source, no .NET runtime) running the same comparison in the client CI job. What is checked: member-set equality (with the documented MovieFormat.None exclusion) and numeric values for the flags enums. Dropdown order is deliberately not compared -- the client table is the Select order and may differ from server declaration order for display reasons. For GamePlatform the integer values are persisted, so a member removal or renumber is caught; a pure dropdown reorder is not because it changes no stored data.
Closes #103. A Steam Deck runs SteamOS (Linux) or Windows and plays desktop-PC games, so it is a PC -- a form factor / delivery mode, not a platform distinct from PC. This unifies it with the Steam-imported-game shape (#101): Platform=Pc, IsDigital=true, DigitalStore=Steam. - Remove GamePlatform.SteamDeck (60) from the enum. - GamePlatformMapping: 'steam deck' / 'steamdeck' now resolve to Pc. - Data migration ConvertSteamDeckToPc: UPDATE Games SET Platform = 1 WHERE Platform = 60 (pure data, no schema change; idempotent). Down is a no-op -- the original SteamDeck rows are not recoverable. - Client GAME_PLATFORMS table drops the SteamDeck entry. - Mapping tests: Steam Deck aliases now assert Pc.
…eck migration
Resolves the P1/P2 findings from both reviewers:
Migration (Postgres):
- Postgres builds via EnsureCreated() and never replays migrations, so
the 60->1 SteamDeck fixup would never run there. Move it into
GamePlatformBackfill (EF query, runs at startup on both providers).
Keep the SQL in the migration for the SQLite path + documentation.
- Quote identifiers in the raw migration SQL ("Games"/"Platform") so
it does not case-fold to lowercase on Postgres.
Parity check gaps:
- Add a golden test (PersistedEnumValuesAreStable) pinning every
GamePlatform name->int, catching a renumber the set/flags checks miss.
- Reject duplicate client entries (set-membership alone passed dupes).
- Hard-fail on an unparsable server member line (was failing open).
- Check the client union type, not just the table array.
- Make the two parsers agree: bracket/string-aware array-end scan in C#
(was a raw ';' search), reflect over the Domain assembly for enum
discovery instead of a directory listing, and derive the theory data
from ClientTable (MemberData) so the list can't drift.
Architecture:
- Remove Collectify.Domain.Enums.EnumParity from the Domain assembly
(test/reflection plumbing does not belong in Domain); the table map
and reflection helper now live in the test.
Tests: add GamePlatformBackfill retirement test (60 -> Pc) and the
golden persisted-value test. Mutation-tested: duplicate, renumber, and
unparsable-line mutations now all fail.
Address the two reviewers' second-round findings:
- Move the SteamDeck(60)->Pc(1) data fix into GamePlatformBackfill, which
runs on BOTH SQLite (EnsureCreated) and Postgres (EnsureCreated).
The SQLite-only migration would never run in production. Keep the
migration for the migrate-from-legacy path; quote identifiers in the
raw SQL.
- Parity checks:
* Expand PersistedEnumValuesAreStable from GamePlatform to ALL eight
persisted enums, and assert the server member set equals the pinned
set (catches new unpinned members).
* Add a reverse registration check: every Domain enum must be in
ClientTable (or on the notMirroredOnClient allowlist). Closes the
"add an enum to the client but not the table" gap.
* MJS: fail (not skip) on unparseable `public enum` declarations, so a
shape change cannot silently drop an enum from the check.
* MJS: validate the export type union as well as the table.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
last_query_stamp (graphify query cache) and an ad-hoc architecture review HTML were committed by accident via `git add -A`. Remove them from the branch and gitignore them so they stay local. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex review found three P2 gaps that let documented drift pass: - MJS: the union was compared against the table-scoped member list with 'None' stripped, so deleting 'None' from the MovieFormat union went undetected. Compare the union against the FULL member list (the type must name every member); keep the table-scoped 'None' omission only for the table check. - xUnit: PersistedEnumValuesAreStable only iterated existing golden maps, so a new enum added to ClientTable but omitted from PersistedValues escaped value pinning. Assert the two key sets agree first. - ci-local.sh: the client job now runs `npm run check:enums` to mirror the GitHub workflow (same-jobs guarantee in AGENTS.md). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude Code review surfaced three findings. Addressed the two in-scope parity gaps; the third (arbitrary-integer enum binding at the API write boundary) is pre-existing and cross-cutting, so it is tracked as #115. - Reserved value 60: document it as retired (was SteamDeck, #103) in GamePlatform.cs and pin it in a new ReservedValues map + ReservedValuesAreNotReusedByLiveMembers test. GamePlatformBackfill rewrites 60->Pc on every startup, so a live member reusing 60 would be silently clobbered; the new test blocks that. - MJS fail-open on a 2nd enum in a file: parseServerEnum used a non-global match, so only the first enum per file was checked. Split into parseServerEnums (matchAll) + parseOneEnum, iterate every parsed enum, and fail closed when a file declares more enums than were parsed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three mutation-verified fail-open paths in check-enum-parity.mjs: - Flags value expressions: the value regex captured a leading literal, so `value: 4 << 1` parsed as 4 and passed. Now extract the full value token and reject anything that is not a plain integer or a string; a numeric expression throws. - None exemption scope: 'None' was exempted from the table check for EVERY enum, but only MovieFormat has a None member. Gate the exemption on name === 'MovieFormat' so a future None on another enum must appear in the table (the union still lists it either way). - Missing union: a null parseClientUnion result skipped every union assertion. Now a missing/unparseable union is a hard failure, and the parser tolerates multi-line unions (`=\n | 'A' ...`) so GamePlatform still parses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 4 flagged that the C# half of the parity check had the same fail-open paths the MJS already fixed, plus an allowlist inconsistency: - None omission scope (C#): the filter dropped 'None' for every enum; gate it on enumName == "MovieFormat" to match the MJS and the documented exception. - Value token parsing (C#): the value regex captured a leading literal so `4 << 1` parsed as 4. Extract the full value token; a plain integer is a flags value, a quoted string is non-flags (null), anything else throws. - Server-only enum allowlist: add notMirroredOnClient to the MJS (in sync with the xUnit one) and skip those enums, so the documented server-internal-enum path does not break `npm run check:enums`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 5 found a verified fail-open path: a client table entry removed
by commenting it out (`// { value: 'Steam', ... }`) was still matched by the
raw `{...}` regex in both parsers, so the table falsely appeared complete.
Strip whole-line comments before collecting entries in both
check-enum-parity.mjs (parseClientTable) and EnumParityTests
(ParseClientTable), so a commented-out option is correctly reported missing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ecks
Codex round 6 found two more fail-open paths, both mutation-verified:
- Union broadened with a non-literal member: a client alias widened with
`| string` still parsed to the same literal set, so parity was reported
even though the client could now hold values absent from the server enum.
parseClientUnion now asserts the RHS consists only of string literals and
`|`/whitespace (leading or trailing pipes), and throws otherwise.
- Block-commented table entries: `/* { value: ... } */` was still matched by
the raw `{...}` regex because only whole-line `//` comments were dropped.
Strip block comments first in both check-enum-parity.mjs and
EnumParityTests.ParseClientTable.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ecks
Codex round 7 found two more mutation-verified issues:
- Inline comments: `{ live }, // { removed }` kept the line (only whole-line
`//` was dropped), so the raw `{...}` regex counted the commented entry.
Both parsers now truncate each line at the first `//` outside a string
literal, covering block, whole-line, and trailing-inline comments.
- Signed flag values: the MJS rejected `value: -1` while the server parser
and C# mirror accepted it, so a valid parity update using a conventional
negative flag (e.g. `All = -1`) would fail CI. Accept an optional leading
minus in the MJS value-token check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude round-8 review findings:
- [P2] Unanchored table lookup: both parsers used a bare
`indexOf("export const " + tableName)` with no delimiter after the name, so
a decoy const whose name merely starts with the real table name (e.g.
`GAME_PLATFORMS_V2` declared above the real `GAME_PLATFORMS`) could shadow
it and let the real table drift free. Anchor the match on a `:` or `=`
after the name and fail closed on ambiguity (0 or >1 hits) in both
check-enum-parity.mjs and EnumParityTests.ParseClientTable.
- [P3] Stale symbol reference: GamePlatform.cs pointed at
EnumParityTests.ReservedGamePlatformValues; the member is ReservedValues.
- [P3] Overstated lockstep comment: the clientTable maps in the .mjs and the
xUnit test do NOT read each other, so "enforces agreement" / "in lockstep"
was misleading. Reworded both to say agreement is enforced indirectly
(each half fails closed on its own) and to add an enum to BOTH maps.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 9 found a fail-open path: an old mirrored table wrapped in a block comment (`/* export const GAME_PLATFORMS: ... */`) was still matched by the anchored `export const X\s*[:=]` lookup, so the parser validated the dead declaration instead of the live table. Both parsers now strip comments from the WHOLE source (via a shared StripComments/stripComments helper) BEFORE the table lookup, so a commented-out (dead) table is invisible to it -- not just a commented-out entry inside a live table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 10 found the union half of the same fail-open the table half had: parseClientUnion matched on raw source, so a commented-out (dead) union (`// export type X = ...`) retained above the live union was validated in place of the live one -- a live union broadened with `| string` would then pass both check:enums and the TypeScript build. parseClientUnion now strips comments first and requires exactly one live declaration (fail closed on 0 or >1), mirroring parseClientTable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ents)
Codex round 11 found three mutation-verified issues, fixed in both halves:
- External spread: a table element like `...extraPlatforms` was silently
ignored by the `{...}` scan, letting a duplicate member ride in from an
external array. Both parsers now split the array on depth-0 commas (not a
naive split that breaks inside object literals) and reject any top-level
element that is not an inline object literal.
- Flags value shape: a quoted value in a flags table was misclassified as
non-flags and skipped numeric parity. Flags-ness now comes from the SERVER
enum (MJS parseServerEnums detects [Flags]; C# uses the type's
FlagsAttribute), and a quoted/missing value in a flags table is rejected.
- C# comment phantoms: a commented `// public enum X` in doc text could
register as a phantom server enum. Both the main loop and the reverse
registration pass now strip C# comments (a new comment-aware
stripCSharpComments helper) before counting and parsing enum declarations.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(round 12)
Claude round-12 review findings (independently verified, no P1):
- [P2] ReservedValues was a hand-copy of GamePlatformBackfill.RetiredPlatform
Values with no linkage: retiring a value in the backfill and forgetting the
test copy would let a later member reuse it and be silently clobbered on
every boot. RetiredPlatformValues is now public and ReservedValues derives
its keys from it, so the two can no longer drift.
- [P3] Post-declaration array mutations (.push/.concat/.splice/.sort/...)
were invisible to both parsers, which only read the array literal. Both
now scan from the array's close to the next top-level declaration and
reject any such mutation of the table.
- [P3] indexOf('= ') required a literal space; a `= [` without a space
mis-parsed. Both parsers now use indexOf('=') and fail closed if absent.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 13 found the mutation scan stopped at the next top-level declaration, so a `.push`/`.concat` hidden inside an immediately-following function (e.g. gamePlatformLabel) escaped inspection. Both parsers now scan the ENTIRE remaining comment-stripped source for a mutation of the table. The table name is already enforced unique (exactly one declaration), so the full-source scan is safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 14 found two P2s on the element/mutation shape:
- An indexed/property assignment (`GAME_PLATFORMS[0].value = 'Other'`) was
invisible because the mutation scan only recognized method calls. Both
parsers now also detect assignment to the table's own fields
(value/label/group/key), restricting to those field names so the check
does not false-positive on method calls (.find/.map) or `=>` in a
callback body. (The `readonly`-array alternative was rejected -- it broke
a consumer that assigns the table to a mutable array, wider blast radius
than the guard warrants.)
- A property spread inside an entry ({ value:'Pc', ...override }) was
already rejected by the "inline object literal" element check; no change
needed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 15 confirmed the regex mutation scan false-positives on ordinary reads (`GAME_PLATFORMS[0]` followed by any later `=`, a CI blocker) and on non-mutating `.concat`, while missing real mutators (`.fill`, `.copyWithin`). Each fix spawns new edge cases; a regex cannot robustly distinguish read from write in arbitrary TS. Removed the scan from both halves. It guarded a deliberate runtime mutation of the exported const -- which is NOT client/server drift (the literal still matches the server), so it is out of scope for the #95 guard. Such a mutation is a separate threat best addressed by typing the tables `readonly` (a deliberate, wider change touching consumers) or code review. The literal member-scan (set equality, values, duplicates, spreads, non-literals) remains the parity guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…criterion) Issue #95's acceptance criterion requires label lookup to use shared helpers rather than open-coded `.find(...)?.label`. This adds helpers for the five tables that still open-coded it (collectionStatus, condition, watchStatus, completionStatus, musicFormat) and migrates the five call sites (DetailView x2, ui.tsx x2, MusicList). The helpers return the label or undefined; call sites that want a raw-value fallback (StatusPill, ConditionPill) add `?? value` at the use site. No open-coded `.find(...)?.label` against the eight enum tables remains. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two non-blocking nits from the round-17 review: - GamePlatformBackfillTests: the XML doc claims RunAsync is idempotent; add a second-run-returns-0 assertion to actually test it. - check-enum-parity.mjs: wrap the per-table body in try/catch so an unexpected parse anomaly (unbalanced braces, ambiguous table, non-object element) surfaces as a clean `FAIL ...` line instead of a raw Node stack trace. Still fails closed (exit 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ty (P3)
Two P3 nits from the round-18 review, both defense-in-depth:
- check-enum-parity.mjs: parseServerEnums sat outside the per-table
try/catch, so a malformed C# enum (unbalanced braces) raw-stacked. Wrap
the per-file parse in the same try/fail pattern, keyed by filename.
- Both parsers: the `\{[^{}]*\}` entry scan matches only the inner brace
pair of a nested object literal (`{ meta: { value: 'X' }, value: 'Y' }`),
shadowing the entry's real value. Assert entries.length === non-empty
top-level element count so a nested literal is rejected. tsc's
excess-property check already blocks this at the build gate; this makes
the parity check correct on its own.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The codebase orders import members case-insensitively with value imports before type imports. MOVIE_FORMAT_FLAGS (m) sorts between gamePlatformLabel (g) and watchStatusLabel (w), not first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
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.
What
Two related data-model / safety-net changes, one PR per your call.
#95 — fail CI when server enums and client tables drift
The client mirrors every server enum by hand in
src/client/services/types.ts. Today nothing stops the two from drifting when a member is added, renamed, or renumbered on one side.Adds two independent checks, both failing CI on drift:
Collectify.Tests.Domain.EnumParityTests— xUnit. Reflects the server enums (member names + numeric values) and compares each client table against its enum. Fails closed on: a member added/renamed on either side, a renumber of a persisted or flags enum, a duplicate client entry, a missing/unparseable enum, a missing union member, a non-literal union, a commented-out (dead) entry or table, an unanchored/ambiguous table lookup, a non-object table element (spread), a nested object literal shadowing an entry, and a new server enum escaping the check (registration-completeness, with a server-only allowlist).src/client/scripts/check-enum-parity.mjs— dependency-free Node script (parses the.cssource, no.NETruntime) running the same comparison in the client CI job. Wired in asnpm run check:enums, added to.github/workflows/ci.yml.Scope of the check: member-set equality (with the documented
MovieFormat.Noneexclusion — the flags-zero value has no UI checkbox) and numeric values for the[Flags]enums. Dropdown order is deliberately not compared: the client table is the<Select>order and may legitimately differ from server declaration order for display reasons. ForGamePlatformthe integers are persisted, so a member removal or renumber is caught.Shared label helpers (acceptance criterion for #95): label lookup now uses shared helpers in
services/types.ts(gamePlatformLabel,collectionStatusLabel,conditionLabel,watchStatusLabel,completionStatusLabel,musicFormatLabel) instead of open-coded.find(...)?.label. The five pre-existing call sites (DetailView ×2, ui.tsx ×2, MusicList) were migrated.Post-declaration runtime mutation of an exported const (e.g.
TABLE.push(...)) is out of scope: it is not client/server drift (the literal still matches the server) — it is a separate threat best addressed by typing the tablesreadonly(a deliberate, wider change) or code review. A regex scan for it was tried and dropped because it false-positives on ordinary reads and non-mutating methods while missing real mutators (documented in both parsers).#103 — model Steam Deck as Pc, retire the
SteamDeckvalueGamePlatform.SteamDeck(60) is removed. A Steam Deck is a PC; its "store" dimension is already captured byIsDigital+DigitalStore.Steam.20260815070314_ConvertSteamDeckToPc— a data-onlyUPDATE(no schema change) that reclassifies persistedPlatform = 60rows toPc(1).GamePlatformBackfill— re-runs the same reclassification on every boot, so it is idempotent and covers both the SQLite (migration) and Postgres (EnsureCreated) paths.RetiredPlatformValuesis the single source of truth for retired values.EnumParityTests.ReservedValuesderives fromGamePlatformBackfill.RetiredPlatformValues, so value 60 is pinned as retired and a new member reusing it fails CI (it would be silently clobbered on every boot)."steam deck"resolves toPc(and OCR noise filtering still treats "steam deck" as noise, unchanged).Verification
dotnet buildclean;dotnet test→ 379 pass (incl. new reserved-value, registration, backfill-idempotency, and parity fail-close tests).npm run buildclean;npm test→ 108 pass;npm run check:enums→ all 8 tables in parity.SteamDeck = 60(even with a full client mirror) fails the reserved-value guard; a drift on either side fails both the xUnit test and the MJS script.Out of scope (tracked separately)