fix(settings): reconcile raw projections - #122
Conversation
Teakowa
left a comment
There was a problem hiding this comment.
Full QA review — fb52abf7281fd4454b3d738d1ca705b71c36b25b
Blocker
- The PR resolves projection conflicts by deleting reviewed export enum evidence instead of reconciling it into the canonical domains.
The full-contract raw-entry comparison is a correct improvement, but the data remediation removes several generated enum domains/members without migrating their non-overlapping members to the fixture-owned canonical domains. Examples removed by this PR include:
heroLimit:1PerTeam,2PerTeam,1PerGame,2PerGame;roleLimit:off;spawnHealthPacks:modeDependent,enabled;mapRotation:afterMirrorMatch,paused;returnToLobby:afterMirrorMatch.
The fixture-owned domains retained on this head are substantially narrower (for example heroLimit retains only off; spawnHealthPacks only disabled; mapRotation only afterAGame). These deleted members came from the reviewed Workshop-data export projection; removing them makes the validator green by discarding evidence rather than reconciling the two sources.
This is observable in the typed API: enum writes call table::enum_name(domain, member) and reject members that are absent from the canonical enum tables, so the removed members become invalid after this PR. That conflicts directly with #110's requirement to reconcile reviewed settings facts into one canonical dataset and to remove/reroute duplicate semantic tables only after the canonical catalog is equivalent or more accurate.
Please map the export-backed members into the chosen canonical enum domains/identities (with explicit provenance/reconciliation) rather than dropping them. Add a representative regression using a real affected domain so export-only members cannot disappear while workshop-catalog-gen check still passes.
Major
-
Enum conflict validation is only one-directional.
validate_enum_projection()detects the same(domain, member)mapping to two different display names, but it does not detect the inverse conflict: the same(domain, display name)mapped to different canonical member IDs. The data being reconciled already demonstrates this shape: fixturemapRotation.afterAGame -> "After A Game"versus generatedmapRotation.afterGame -> "After A Game"(similarly forreturnToLobby). Once the domains are correctly unified instead of deleted, this ambiguity would pass the current validator. Canonical enum member identity needs an explicit reconciliation rule or a reverse-collision check. -
The reconciliation is not currently encoded in a reproducible generation path. The PR edits
data/generated_entries.rsdirectly and changes its header to say certain fixture-owned conflicts are “reconciled out”, but no generator/source rule is changed that records which export entries/members are excluded or remapped.workshop-catalog-genvalidates the committed settings projection but does not regenerate this file. #110 explicitly requires a deterministic canonical dataset/generation path; the reconciliation decision should live in durable source/generation data rather than only in a hand-edited generated artifact, otherwise a future export refresh can reintroduce the same conflict.
Verified improvements
- raw duplicate paths now compare the full
TableEntrycontract, so enum-domain and presentation differences are no longer hidden by first-wins lookup; - projection diagnostics identify fixture vs Workshop-data export sources;
- orphaned enum domains and same-member/different-name conflicts are checked independently of lookup order;
- stable/MSRV Rust quality, Catalog check, and all five real-project scenarios are green on this exact head.
The PR is not ready to merge while the Blocker remains; #110 should stay open.
|
Addressed in 5fa213b.
Verified: |
Teakowa
left a comment
There was a problem hiding this comment.
Re-review — exact head 50b7a45b6df22b0d534eebd58b8c869728ae3770
The previous Blocker is resolved: the reviewed Workshop-data export projection is restored, export-only enum members are retained through explicit canonical mappings, reverse display-name/identity collisions are checked, and the typed heroLimit / mapRotation regressions demonstrate observable preservation. The reconciliation manifest also solves the previous reproducibility concern without editing the generated export artifact.
Major
- The reconciliation manifest is not itself conflict-checked, so contradictory canonical mapping rules can still pass
workshop-catalog-gen check.
validate_enum_projection() finds a mapping with mappings.iter().find(...) and tracks only whether a (sourceDomain, sourceMember) was consumed. If projection_reconciliation.json contains two mappings for the same source member with different targets, the first mapping wins; the second is then considered satisfied by the same mapped_sources key. No error is emitted. entryOverrides has the same shape: reconciled_entry_override() uses .find() by path, so duplicate rules for one path can silently shadow each other.
That leaves the new durable reconciliation source with the same first-wins class of failure this PR is intended to remove from the raw tables. Before #110 can treat this as a conflict-checked canonical generation path, validate the reconciliation data itself (at minimum unique entryOverrides.path and unique (sourceDomain, sourceMember) mappings; conflicting duplicate rules must fail). A small mutation/unit test for duplicate source mappings and duplicate path overrides would directly protect the contract.
Minor
-
The canonical enum provenance comment is now factually stale.
table.rsstill saysroleLimithas exactly one evidenced member and thatoffis rejected until a snapshot evidences it, while this PR intentionally acceptsroleLimit.offfrom the reviewed export and adds several other export-backed members toENUM_MEMBERS. Update the comment so it distinguishes fixture evidence from export-backed canonical members instead of documenting behavior that is now false. -
projection_reconciliation.jsondeclaresschemaVersion: 1, butReconciliationdoes not deserialize or validate that field. If the version is meant to be a durable data-contract marker, parse/check it (and preferably reject unsupported versions); otherwise remove the unused field rather than implying version enforcement.
Verified
- previous export-evidence loss is fixed;
afterGame -> afterAGameand other explicit identity mappings are represented durably;- full raw
TableEntrycontract comparison remains intact; - same-member/different-name and same-display/different-member conflicts are covered;
- stale enum mappings whose source disappears are rejected;
- exact-head CI is green across stable/MSRV Rust quality, Catalog check, and all five real-project scenarios.
No additional Blocker/Major findings were found across the current PR scope. The PR should stay open until the manifest self-conflict check is closed.
Teakowa
left a comment
There was a problem hiding this comment.
Final QA — exact head 8284a2a0d2e55718c3ffd1c5681dfc431f75ee4c
0 Blocker / 0 Major / 0 Minor.
The remaining reconciliation-manifest Major is resolved: the manifest now rejects duplicate entry override paths and duplicate enum mapping source identities before first-match lookup can be used, and unsupported schemaVersion values fail validation. The catalog check invokes this validation directly. The stale enum provenance comment was also corrected.
Previously resolved guarantees remain intact: reviewed export enum evidence is preserved, source identities are explicitly mapped into canonical domains, reverse display-name/member conflicts are checked, stale mappings are rejected, and representative typed heroLimit / mapRotation writes remain covered.
Exact-head CI is green for stable Rust quality, Rust 1.85 quality, Catalog check, and all five real-project scenarios.
This head is ready to merge for the scoped #110 projection-reconciliation fix.
Summary
Verification
Refs #110