Never reuse a field number a deletion freed - #610
Merged
Eli Pinkerton (wallstop) merged 15 commits intoAug 30, 2026
Conversation
check-code-fence-syntax.sh was the one linter left on the missing-red-half work list: nothing under scripts/tests/ named it, so a green Repo Lint was evidence about docs/ and no evidence the gate still reports. Writing its self-test found a second defect. The gate counted issues but never counted files, so a corpus holding no markdown -- docs/ renamed, a tree moved, a find that stopped matching -- printed "No code fence syntax issues found" and exited 0. That is the exact shape test-empty-corpus- gates.ps1 exists to prevent, and this gate had never been in that family. It now reports its scan count and fails on an empty walk. The self-test carries a red half per rule the gate enforces: backtick and tilde fences, indented fences, fences longer than three backticks, a file in a subdirectory, a multi-offence count, a missing directory and an empty corpus. Verified falsifiable against three mutants -- suppressing the issue counter fails six cases, removing the empty-corpus guard fails one, and -maxdepth 1 fails the recursion case. missingRedHalf is empty again; no linter in scripts/ is unfalsifiable. Fixes #604 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A subtype's field number is a durable wire contract: a payload resolves a subtype by that number and nothing else. For the explicit forms -- [WProtoSubtype(typeof(Weapon), 100)] and [WProtoInclude(100, typeof(Melee))] -- the declaration was the only record that 100 had ever been spent, and it is deleted along with the type it sits on. WPROTO039 fires on two LIVE claims and has no memory, so a number freed by a deletion was indistinguishable from one never used: the next subtype added could be handed 100, and every payload written by an older build read that field back as the wrong type. No diagnostic, no exception. The planner now records an explicitly numbered declaration beside the assigned ones, which is what turns its deletion into a retirement, and the generator refuses any declaration -- subtype or include -- claiming a number the assembly has retired, naming the type that held it. The refusal is by NAME, so re-adding the deleted type still restores its own number. Two more of the same class, found while writing the tests: - Deleting the number from an attribute made the pair look brand new, so it was handed the smallest free number instead of keeping the one it wrote. Editing a number in place left no trace at all. Both now record the new number and retire what they left. - A pair that had retired two numbers -- what a hand-edited number leaves behind -- lost one of them on the next run, because re-emission was keyed by pair rather than by pair and number. A dropped retirement is a number that is free again a run later. Nine new plan and generator tests, red before the change. The three existing tests that asserted the old behaviour are updated in place: the "no manifest at all" guard now rests on FreshlyAssigned, which is the condition the unattended pass actually reads, so adopting the package still writes nothing into a project that invents no numbers. 689 generator tests pass against protobuf-net 3.2.56 and 688 against 2.4.9. The shipped analyzer is rebuilt and byte-identical to its sources. Fixes #606 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#603 asked for a decision: build a runtime registry so a consumer can derive from a package base, or keep refusing and explain why the refusal is not temporary. PLAN.md names refusal the safe default, and it is: every failure mode of a registry is silent data corruption rather than a build error. Unity's registrars run unordered, so a serialize before the last registrar writes under the wrong number or none; two packages picking one number on a shared base is undetectable at build time and type-confusing at read time; and the lookup has to survive managed stripping under IL2CPP. A build error you can see beats a player that writes an unreadable save. WPROTO040's message said the base's chain "was generated when its own assembly was compiled" and left the reader to guess whether that was a limit or a gap. It now states that this is how per-assembly generation works rather than a gap waiting to be filled, and names composition -- your own [WProtoContract] holding the base as a [WProtoMember] -- as the shape that does work. A diagnostic that names a fix has to name one that works, so a second test compiles that alternative against an upstream assembly and asserts it generates clean. The docs carry both shapes side by side, and say plainly what composition costs: a List<Weapon> cannot hold your type. Fixes #603 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The validation engine shipped without a way for anything but a person to run it, which is the difference between "you can write project checks" and "your project is checked". This is the headless half. ValidationBatch.ValidateFromCommandLine finds every IValidationRule through TypeCache, builds it, runs the lot, writes a JSON report and exits non-zero when anything at or above -validationFailOn stands unsuppressed. A rule that threw fails the run whatever the threshold: it produced no answer for that asset, and passing on it would report coverage the run does not have. A rule that cannot be constructed is reported and skipped rather than ending the run, because one broken rule hiding every other rule's findings is worse. ValidationSuppressions is one finding identity per line, so a diff shows exactly which check somebody switched off. It matches on rule, asset GUID and discriminator -- never the path, never the message -- so moving an asset or rewording a rule does not silently un-suppress it. Render() writes the path and message above each entry as a comment, because a rule name and a GUID tell a reviewer nothing. UnusedIn reports entries that matched nothing. A suppression that outlives the finding it silenced reads as a considered decision and is really a line nobody has looked at -- the same shape as a linter that cannot report. Its doc says plainly that only a whole-project run may be trusted for it. The report keeps suppressed findings and marks them, rather than dropping them: a report that omitted them would make a suppression file indistinguishable from a project with nothing wrong. Verified: compiles against Unity reference assemblies through typecheck:editor and typecheck:tests, changed-file preflight and the five relevant repo-lint gates pass. The 18 EditMode fixtures run in CI -- no Unity license or MCP bridge was reachable this session, so their execution is unverified locally. The JSON assertions round-trip through JsonUtility rather than matching pretty-printed spacing, so they test content rather than indentation. Addresses #288 (results window and automatic re-runs still open) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A member's field number is a durable wire contract, and the declaration that spends one is deleted along with the member it sits on. WPROTO002 refuses two members claiming one number at the same TIME, so it cannot see a number a deletion freed: delete Health at 3, add a string at 3, and every payload written by an older build reads that field back as a string. No diagnostic, no exception -- the same defect as #606, one level up, reaching far more code because every contract has members. [WProtoReserved(3)] records the removal, and [WProtoReserved("Health")] records the name. Names matter for the reason protobuf reserves both: a re-added Health at a DIFFERENT number still breaks anything matching by name -- a JSON projection, a generated .proto consumer, a schema registry -- while carrying data that means something else. WPROTO043 refuses a member that takes either. It is deliberately one diagnostic, not two: "a new member took a dead number" and "a reservation contradicts a live member" are the same state seen from two sides, nothing in the compiler can tell which is wrong, and a second code could never fire alongside the first. The message names both fixes instead. The schema exporter emits the matching proto3 reserved lines. Without them the exported schema permits, in the consumer's own toolchain, exactly the reuse this refuses. A reserved number outside proto3's range is dropped with a diagnostic naming it, because a schema nobody can parse is worse than one missing a reservation. Reservations are per contract. A base's does not bind its subtypes: their numbers live in a different space, so inheriting one would refuse a member for a collision that cannot happen. Verified: 700 generator tests against protobuf-net 3.2.56 and 699 against 2.4.9. The schema fixtures live in Tests/Runtime, which is excluded from the dotnet harness, so their exact expected text was produced by compiling the real WProtoSchemaText against the same fixtures in a scratch project rather than guessed. The shipped analyzer is rebuilt and byte-identical. Fixes #608 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lint:code-samples extracts 3,061 C# blocks out of docs/ and validates none
of them, so an example naming something moved or renamed reads as correct
forever. Session 236 found invented APIs in three pages by hand; nothing
stops the next one.
Two rules, chosen because they have no false positives. A general "does
this API exist" check needs a real parser -- a first attempt keyed on
Type.Member reported Serializer.ProtoDeserialize as missing, because a regex
over declarations cannot see a generic method, and a gate that cries wolf is
one people stop reading.
1. A `using WallstopStudios.UnityHelpers...;` must name a namespace some
.cs file declares. A using is unambiguous: it is a namespace, spelled
in full, or it does not compile.
2. An `<assembly fullname="WallstopStudios...">` in a link.xml example
must name an assembly some .asmdef declares.
Both found a real defect on the run that introduced them, and both shipped
in 3.5.1:
- The enum display-name example imported Core.Attribute, not Core.Attributes.
A reader copying it gets a compile error.
- The link.xml example preserved `WallstopStudios.UnityHelpers.Runtime`. The
runtime assembly is `WallstopStudios.UnityHelpers`, and the linker reports
nothing for a name it cannot match -- so that one surfaced as a stripped
player rather than as an error.
Ten self-test cases, red halves included, and the run-repo-lint meta-check
was verified to catch this linter when its self-test was unregistered.
Addresses #441
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review of the headless runner added an hour earlier found it carrying the exact defect the code-fence gate was fixed for this morning: a run with no rules, or no assets, exited 0 and reported "validation passed" having measured nothing. Both shapes are reachable with nothing looking wrong at the call site. ValidationTargets.Enumerate skips a folder that does not exist rather than reporting it -- deliberately, because Unity logs a warning per missing folder -- so one typo in -validationFolder yields zero targets and a green build. A project that has not written a rule yet yields zero rules. CoverageProblems is a pure function of the two counts and the folder list, separated from Run so it can be asserted without an asset database. Both reasons are reported rather than the first, so fixing one does not hide the other, and the empty-assets message names the folders it was given -- the reader cannot otherwise tell "the project is empty" from "I typed the path wrong". Addresses #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two real defects Cursor Bugbot found in the first draft, both silent data corruption. Restore dropped sibling retired numbers. `restoredRetirements` was keyed by subtype/base pair, and the final loop then dropped EVERY retirement for that pair. A pair can hold more than one -- a hand-edited number leaves one and a later deletion leaves another -- so re-adding the type under the first freed the second, which is the exact reuse the record exists to forbid. It is keyed by pair AND number now. Verified against a mutant: three new cases and two existing ones go red when the key is coarsened back. Reserved numbers skipped subtype tags. WPROTO043 inspected only [WProtoMember], while a base's includes are numbered against its members -- one space -- so a rule binding half of it was one an author steps around by writing the number on the other half. [WProtoInclude] and [WProtoSubtype] are both refused now, at their own diagnostics so each names what the author actually wrote. That has a second half Bugbot named: the assignment tool has to assign around reserved numbers as well, or it hands out a number the next compile rejects -- a deadlock, which is the thing that tool exists to remove. AddReserved now feeds [WProtoReserved] numbers in beside the members and includes. Bugbot's third finding, a validation run over an empty folder exiting 0, was already fixed in 21f0a34. 707 generator tests against protobuf-net 3.2.56, 706 against 2.4.9. Addresses #606, #608 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same shape as the reservation-skips-subtypes defect a review just found: the rule read one half of the space. [WProtoMember(9, Name = "Health")] presents itself as Health to a generated schema, a payload dump and anything matching by name, whatever the C# member is called -- so a reservation that inspected only the identifier was one an author steps around by renaming. Both names are checked now, and the diagnostic names whichever one was taken. The other direction stays legal, because it is the point of the override: a C# member called Health that presents itself as something else takes nothing back. The schema exporter already renders the override, so the two halves now agree -- a project that compiles can no longer produce a message carrying `reserved "Health";` beside `int32 Health = 9;`. 709 generator tests against protobuf-net 3.2.56. Addresses #608 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0c4279c. Configure here.
A reservation documents a wire contract, so the one outcome it must not have is changing one. Asserted by comparing the emitted formatter source with and without [WProtoReserved], which is stronger than comparing bytes for a handful of values: if the emitted code is the same code, there is no payload the two could disagree about. 710 generator tests against protobuf-net 3.2.56. Addresses #608 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Inserting the helper above AsksForZigZag's signature put it between that method's doc comment and the method, so AsksForZigZag lost its documentation and SchemaNameOf inherited a <summary> about ZigZag. Caught by lint-xml-doc-summaries, which exists for exactly this -- a member's documentation outliving the member (#591). Recorded rather than quietly fixed, because the miss was mine twice over: an anchor chosen on the signature rather than the doc block, and a push that ran the targeted gates for the docs I changed but not the one for the C# I added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 30, 2026
The empty-corpus rule was enforced by test-empty-corpus-gates.ps1 over six PowerShell linters and had never reached the JavaScript ones. Measured by pointing each at a directory holding nothing, through the scan-root override its own self-test already uses: lint-comparison-direction exit=0 lint-nested-type-placement exit=1 lint-xml-doc-summaries exit=0 lint-doc-identifiers exit=0 Three of four reported a clean run over nothing, including the linter this branch added an hour earlier -- the session's own theme, in the session's own new code, for the third time. All three fail an empty walk now and name what was empty. lint-doc-identifiers checks both halves, because either going empty makes every answer vacuous: no documents means nothing was read, and no namespaces means every using would resolve to nothing. It reports its document count on success too, so a corpus quietly shrinking is visible before it reaches zero. A red half per gate, in each gate's own self-test rather than in a central list, so the case sits beside the rules it guards. Addresses #556's rule for the JavaScript family Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WGroupEndAttribute already exposes IReadOnlyList<string> for the same shape, so an attribute handing back its own mutable array was the odd one out. The sets are constructed in the constructor and never change; the type now says so. Verified the schema exporter still renders identically -- reserved numbers and names in the same order, and the three out-of-range diagnostics unchanged -- by re-running the real WProtoSchemaText against the same fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eli Pinkerton (wallstop)
changed the title
The rule read the C# identifier alongside the schema name, so a member presenting a free name through [WProtoMember(Name = ...)] was refused when its identifier happened to be reserved. That contradicted this feature's own documentation, which already promised "a C# Health presenting itself as something else is not" refused, and the commit that introduced it, whose subject was reserving the name a consumer sees. The test could not have caught it: it named the member Vitality AND set Name = "Vitality", so it passed whichever of the two identities the rule happened to read. It now reserves "Health", names the member Health, and presents it as Vitality -- the only arrangement that tells them apart. Verified by mutating the rule back: that case is the one that goes red. Reported by Cursor Bugbot. 710 generator tests against protobuf-net 3.2.56. Addresses #608 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l be WPROTO040's message claimed the limit was "how per-assembly generation works rather than a gap waiting to be filled". The first half is true; the second overreached, and three measurements taken after that wording shipped show a mechanism that could lift it: - A consumer's compilation can already read the package base's [WProtoInclude] tags and every [WProtoMember] tag out of metadata, so collision detection needs no new information. - WProtoFormatterProvider already documents that a later registration replaces an earlier one, deliberately, so a consumer can supply a base's formatter. - The emitted chain is ordinary static code; nothing requires the assembly that declared the base to be the one that emits it. So the extending assembly could emit the base's whole chain itself. That is not the runtime registry #603 refused -- no lookup, no MakeGenericType, no unordered-registrar hazard -- and it is now tracked on #612. The message states the mechanism and the working alternative and stops predicting the future in either direction: it does not promise a release either, because the refusal is real today whatever #612 decides. The test that pinned the permanence claim now pins the mechanism and the fix instead. 710 generator tests against protobuf-net 3.2.56. Refs #603, #612 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eli Pinkerton (wallstop)
deleted the
dev/wallstop/session-237-issue-sweep
branch
August 30, 2026 06:34
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
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.
Why: A field number freed by a deletion was indistinguishable from one never used, so the next
type or member added could be handed it and read every older save back as the wrong thing.
What:
[WProtoSubtype]number so deleting the subtype retires it, and refuse anydeclaration claiming a retired number.
[WProtoReserved], so a removed[WProtoMember]'s number and name cannot be taken again;it binds subtype tags too, and the exported schema carries proto3
reservedlines.-executeMethod, with a JSON report and a reviewable suppressionfile.
do not have.
Fixes #603
Fixes #604
Fixes #606
Fixes #608
🤖 Generated with Claude Code
Note
Medium Risk
Changes affect save compatibility and subtype/manifest assignment for all WallstopProto users; CI validation batch can fail builds on coverage or unsuppressed findings, but behavior is opt-in via
-executeMethod.Overview
This PR closes the gap where deleting a WallstopProto member or subtype freed a field number with no durable record, so the next assignment could silently break older saves.
WallstopProto wire safety: Adds
[WProtoReserved]with build-timeWPROTO043when a live member, include, or subtype reuses a reserved number or schema name; reservations do not change emitted formatters and appear as proto3reservedlines in schema export. Subtype tag tooling now records hand-written[WProtoSubtype]numbers in the manifest, retires on delete, tracks multiple retirements per pair, treats[WProtoReserved]as spent numbers when auto-assigning, and the generator refuses explicit tags that collide with retired discriminators. Cross-assembly[WProtoSubtype]diagnostics are clarified to explain per-assembly dispatch and recommend composition via[WProtoMember]instead of promising a future registry.Editor validation in CI: Adds
ValidationBatch.ValidateFromCommandLinewith JSON reporting (ValidationReport), line-based suppressions (ValidationSuppressions), severity thresholds, folder scoping, and failure when rules throw or the run checked zero rules/assets.Docs and gates: Documents CI validation and member retirement; fixes broken doc examples (
Core.Attributes, runtime assembly name inlink.xml); addslint:doc-identifiersand makes several linters fail when their corpus is empty instead of passing vacuously.Reviewed by Cursor Bugbot for commit afd46ef. Bugbot is set up for automated code reviews on this repo. Configure here.