Fix access transformer false positives#16
Open
GhostTypes wants to merge 8 commits into
Open
Conversation
…urce The AT validator parsed decompiled .java, which omits compiler-generated record members (canonical constructors + component accessors). This produced false-positive "not found" errors for record accessors and canonical ctors, forcing a manual fallback to javap on the remapped JAR (issue #12). Validate against the remapped JAR's bytecode via the bundled ASM bytecode-dumper instead — the exact facts javap shows, with true access flags and erased descriptors. Member existence/signature is now an exact bytecode lookup, eliminating the whole class of source-omission false positives. - Add BytecodeIndexService: extracts only the referenced classes from the remapped JAR, dumps them, and caches per-class results in a remapped/{version}-{mapping}.bytecode.json sidecar keyed by JAR signature (always fresh, never dumps the whole JAR). Cleared on force re-decompile. - Rewrite the AT validation core onto BytecodeClass: - implicit record accessors + canonical constructors are found (no false "not found") - constructors are never "overridable" (fixes the AnyOfCondition <init> warning) - record canonical-ctor widening is an informational note (needed only when the record is instantiated), not a "will crash at runtime" error claim - uses real bytecode access flags for inner-class accessibility + ctor checks - Only requires the remapped JAR (produced by decompile_minecraft_version). Tests: - Rewrite AT unit tests onto bytecode fixtures with a regression block for every flagged false positive. - Add a committed compiled fixture JAR reproducing the vanilla shapes the Summoning Rituals AT targets + an end-to-end test on its verbatim accesstransformer.cfg (0 errors, 3 informational notes). Minecraft bytecode is never committed (IP); the stub reproduces only the shapes the validator reads. - Add an offline fixture-fidelity drift guard pinning the stub to the exact 1.21.1 bytecode facts, plus a manual real-MC test that (1.21.1) asserts the AT validates clean AND the stub matches freshly-downloaded vanilla, and (1.21.11) asserts the validator correctly reports the cross-version package rename (critereon -> criterion) as class-level drift, not member false positives. - Add BytecodeIndexService cache tests. Update CLAUDE.md, the spec, and the AT documentation resource. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The tool echoed the full nested entry object for every finding and pretty- printed empty sections, so a handful of results ran to ~150 lines. Reshape the output: lead with `valid` + a one-line `summary`, render each finding as its one-line AT directive + message, and omit empty sections. `success` on the envelope still means "the tool ran"; `valid` is the validation verdict. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT
GhostTypes
changed the title
Two concurrency fixes from PR review: - getClassBytecode re-reads the cache and merges into the latest copy right before saving, so a concurrent call's newly-dumped classes are not clobbered by an older snapshot (lost update). - dumpClasses adds a random suffix to the temp JAR name so two calls in the same millisecond cannot collide on the path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT
Windows checkouts convert to CRLF via core.autocrlf, which Biome flags as errors even though committed blobs are LF. Pin eol=lf for text, binary for jars. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT
Formatting (trailing commas, import order, it.each layout), one optional-chain, and a dead biome-ignore suppression — all pre-existing on main, surfaced once the CRLF noise was removed. No logic changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT
…ource Bring validate_access_widener onto the same BytecodeIndexService path as the access-transformer tool, so the two validators no longer diverge. AW previously validated Fabric access wideners against VineFlower-decompiled source, which omits compiler-generated record members (canonical constructors + component accessors) — the exact issue-#12 false-positive class. Reading the remapped JAR's bytecode fixes it: member existence is an exact lookup and descriptor matching is exact string comparison. - Preserve AW-specific semantics: 'extendable' warns on a final class, 'mutable' warns on an already-non-final field, field descriptor mismatch is an error. - Compact, verdict-first tool output matching validate_access_transformer. - Rewrite AW validation tests onto bytecode fixtures; add an issue-#12 record accessor regression test. Full offline suite green (533). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT
Contributor
Author
|
/gemini review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Use Object.hasOwn instead of `in` for the bytecode cache lookup so a class name colliding with an Object.prototype key can't be treated as already-cached. - Clean up the temp file if the atomic cache write fails to rename. - Guard the enclosing-class `$` walk with `idx > 0` so a leading `$` can't add an empty class name to the lookup set. - Restore useful "did you mean" class suggestions on class-not-found: draw candidates from the remapped JAR's full class list (central- directory scan, no bytecode dumped), scoped to the target's own package, in both the AT and AW validators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Configure Biome to honor .gitignore (enable vcs: enabled/clientKind: git/useIgnoreFile) and explicitly ignore tracked vendored dirs (.claude, docs), so lint stops flagging non-source files. The package.json keywords array is reformatted inline to satisfy the formatter. Add a `npm run lint` step to the test workflow, placed right after Build so lint failures fail fast before the slow decompile step, and add biome.json to the push/pull_request path filters so config-only changes still trigger CI. Bump version 1.2.4 -> 1.2.5 in package.json and package-lock.json.
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
2 participants
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.
Fixes the false positives in
validate_access_transformer(#12).Changes
🤖 Generated with Claude Code
Generated by Claude Code