Skip to content

fix: keep jev_classify ids collision-free and tally on a null prototype - #36

Merged
jkudish merged 1 commit into
jkudish:mainfrom
kittimzhe:fix/classify-id-collisions
Sep 26, 2026
Merged

jkudish merged 1 commit into
jkudish:mainfrom
kittimzhe:fix/classify-id-collisions

Conversation

@kittimzhe

Copy link
Copy Markdown
Contributor

What

Two jev_classify defects reported in the "found along the way" section of #34:

  1. Fallback id collisions. Generated fallback ids (item0/class0 style) were never checked against supplied ids, so an item omitting its id took the fallback item0 while a later item explicitly named item0 kept it too — duplicated result ids. For classes the same collision also collapsed probabilities keys, silently overwriting one class's distribution with another's. Duplicate supplied ids were already rejected; the gap was fallbacks vs supplied.
  2. Plain-object tally. by_class was a plain object literal, so a caller-supplied class id like __proto__ was swallowed by the prototype setter and never counted, while constructor read the inherited function and produced a garbage string.

How

  • Apply the two-phase pattern jev_rerank already uses for candidate ids: collect supplied ids first (still rejecting duplicates), then let every generated fallback avoid any supplied or already-used id.
  • Tally by_class on Object.create(null), matching the criteria map already built that way in the same tool.

No tool arguments, results, or README examples change.

Verification

  • npm run build ✓ · npm run typecheck ✓
  • npm test — 227/227 offline (baseline 224 + 3 new regression tests)
  • The 3 new tests fail on the unfixed src/ (224 pass / 3 fail), confirming they catch both defects: jev_classify fallback item ids never collide with explicit ones, jev_classify fallback class ids never collide with explicit ones, jev_classify tallies __proto__ and constructor class ids as plain keys.

Both issues were reported in #34; this PR fixes only the jev_classify items and leaves the rest of that list untouched.

Generated fallback ids (item0/class0 style) could collide with explicit
ids of the same shape: an item omitting its id took the fallback item0
and a later item explicitly named item0 kept it too, duplicating result
ids; for classes the same collision also collapsed probabilities keys,
silently overwriting one class's distribution with another's. Duplicate
supplied ids were rejected, but fallbacks were never checked against
supplied ones. Apply the two-phase pattern jev_rerank already uses:
collect supplied ids first (still rejecting duplicates), then let every
generated fallback avoid any supplied or already-used id.

The by_class summary tally was a plain object, so a caller-supplied
class id like __proto__ was swallowed by the prototype setter and
constructor read the inherited function. Tally on Object.create(null),
matching the criteria map already used in the same tool.

Both issues were reported in the "found along the way" section of jkudish#34.
shivasymbl added a commit to shivasymbl/jev-mcp that referenced this pull request Sep 26, 2026
@jkudish
jkudish merged commit a38bd05 into jkudish:main Sep 26, 2026
jkudish added a commit that referenced this pull request Sep 26, 2026
The third test now selects constructor as well and asserts the full
by_class; expected object is JSON.parsed so __proto__ stays an own key.

Amp-Thread-ID: https://ampcode.com/threads/T-01a0d53e-c271-716f-94dd-6df00c40c7b8

jkudish commented Sep 26, 2026

Copy link
Copy Markdown
Owner
  • Thank you for this!
  • Both fixes are exactly right — the two-phase id assignment jev_rerank uses, and the null-prototype tally.
  • One fix on merge: the third test now actually exercises the constructor tally.
  • Shipping in the next patch release.

@kittimzhe

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review and merge! And good call strengthening the third test — exercising the constructor tally directly makes it a much better regression guard than what I had. Glad this made it into 0.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants