tools/js: test cross-file consistency of the pc data - #1320
Pix3lPirat3 wants to merge 3 commits into
Conversation
Every version's ids and names must be unique, registry ids (items, entities, sounds, and effects, enchantments and biomes from 1.20.3) must start at 0 without gaps, block state ids must be contiguous with the default state in range, and everything that points at something else must point at something that exists: block drops and harvest tools, material speed tables, foods, repairWith, recipe ids, block and entity loot names, enchantment exclusions and costs, collision shape coverage. The current data fails 152 of the 781 checks; those are listed in pc_consistency_known.json with the fix that is in flight for each and run as pending, and the run prints which entries can be dropped once they pass, so a fix removes its own entries and any new inconsistency fails the build.
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The baseline suite runs as reported: 629 passing and 152 pending. Two issues need correction before relying on it as a regression gate: known exceptions currently suppress unrelated new defects, and the entity namespace checks treat block flattening as the object-ID transition. The first is reproduced by an independent duplicate-item mutation; the second is checked against the actual 1.13.2→1.14 protocol translation and public object lookup. Details are inline.
Skills used: prismarine-protocol-data-review checked validator scope and ID namespaces; prismarine-architecture-review traced the loader API; prismarine-review verified current discussion and negative controls.
| describe(`pc ${version} consistency`, function () { | ||
| const check = (title, fn) => it(title, function () { | ||
| const key = `${version} | ${title}` | ||
| try { fn() } catch (err) { if (known[key]) return this.skip(); throw err } |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Please narrow the known-failure exception to the recorded offending entries, rather than skipping any exception from this whole version/check. The entry for 1.8 | ids and names are unique describes an existing duplicate biome, but it also hides a newly duplicated item. I appended another stone item to 1.8/items.json: this test still exits successfully as pending. Removing only the known-biome exception makes the same mutation fail with items: duplicate ids stone#1.
The problem also persists after a fix lands until its known key is manually removed: an unrelated new regression under that key silently becomes pending again. Record/compare the expected offending category and IDs/names (or otherwise fingerprint specific failures), fail on additional problems, and do not swallow arbitrary non-assertion errors. This preserves a usable baseline while making the promised new-regression protection real.
Skills used: prismarine-protocol-data-review checked whether the generic validator catches the defect class; prismarine-review required a mutation/negative control instead of accepting a green baseline as evidence.
| if (!Array.isArray(data)) continue | ||
| const dupIds = []; const dupNames = []; const ids = new Set(); const names = new Set() | ||
| for (const e of data) { | ||
| const idKey = kind === 'entities' && !flattened ? `${e.type}/${e.id}` : e.id // legacy mobs and objects have separate id spaces |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The separate entity/object ID spaces continue through 1.13.2, independently of block flattening. In those versions spawn_entity still carries an object-type byte: object ID 1 is a boat, while registry/mob ID 1 is an armor stand. ViaVersion's actual 1.13.2 → 1.14 translator explicitly converts that ObjectType into the later registry ID.
Consequently the 1.13/1.13.2 duplicates recorded as known failures here are valid separate namespaces; deleting the object table in #1319 would break objectsById. Keep the type-qualified uniqueness/name rules through this transition, and apply the contiguous-registry check below to the registry table rather than the sparse object-ID table. Use the entity protocol transition instead of reusing the block flattened condition.
Skills used: prismarine-protocol-data-review distinguished object wire IDs from registry IDs and checked the real version boundary; prismarine-architecture-review traced the public objectsById consumer; prismarine-review coordinated this with the companion data correction without endorsing deletion of valid data.
|
Both points addressed in 6da80d2.
Full run: 639 passing, 142 pending, 0 unexpected failures. |
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Verified 6da80d2: the 1.13/1.13.2 object-namespace correction is fixed. The suite runs with 639 passing and 142 pending. The known-failure change catches the previous stone mutation, but another independent mutation shows that a known earlier assertion still prevents later categories from being checked; details inline. That leaves this regression-gate repair incomplete.
Skills: prismarine-review verified the repair against prior feedback; prismarine-protocol-data-review checked the complete regression gate with mutation controls; prismarine-architecture-review checked the shared validator contract.
| } catch (err) { | ||
| if (recording) { recorded[key] = { reason: (entry && entry.reason) || (typeof entry === 'string' ? entry : 'unclassified'), error: err.message }; return this.skip() } | ||
| // Skip only the exact recorded failure; a new or changed failure of the same check must still throw. | ||
| if (entry && entry.error === err.message) return this.skip() |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The earlier duplicate-stone probe now fails correctly, but matching the first assertion message still hides additional failures later in this check. With the current 1.8 data, I appended a duplicate Speed#1 effect: the test still exits successfully with 0 passing, 1 pending, because the known duplicate biome throws before the effects category is examined. Removing only that existing duplicate biome makes the same mutation fail with effects: duplicate ids Speed#1.
Please gather the complete set of problems across the check before comparing it with the known baseline (or separate the categories/assertions). Compare complete, untruncated problem identities; the display message should not be the fingerprint. This is the remaining part of the earlier exception concern, not a request to repeat the now-fixed stone case.
Skills: prismarine-review verified the repair against prior feedback; prismarine-protocol-data-review checked the complete regression gate with mutation controls; prismarine-architecture-review checked the shared validator contract.
This branch has not been deployed
2 participants
Every version's ids and names must be unique, registry ids (items, entities, sounds, and effects, enchantments and biomes from 1.20.3) must start at 0 without gaps, block state ids must be contiguous with the default state in range, and everything that points at something else must point at something that exists: block drops and harvest tools, material speed tables, foods, repairWith, recipe ids, block and entity loot names, enchantment exclusions and costs, collision shape coverage. The current data fails 152 of the 781 checks; those are listed in pc_consistency_known.json with the fix that is in flight for each and run as pending, and the run prints which entries can be dropped once they pass, so a fix removes its own entries and any new inconsistency fails the build.