From 2476493fef1d90f78ad360c0f5445d41e3a6dc52 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 21:07:39 +0000 Subject: [PATCH 1/9] fix(access-transformer): validate against bytecode, not decompiled source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT --- CLAUDE.md | 2 +- __tests__/fixtures/at-fixture-src/build.sh | 14 + .../com/mojang/serialization/Codec.java | 3 + .../critereon/LocationPredicate.java | 8 + .../advancements/critereon/MinMaxBounds.java | 5 + .../critereon/StatePropertiesPredicate.java | 9 + .../inventory/tooltip/ClientTextTooltip.java | 6 + .../minecraft/util/FormattedCharSequence.java | 2 + .../world/entity/boss/wither/WitherBoss.java | 7 + .../world/level/storage/loot/IntRange.java | 9 + .../loot/predicates/AnyOfCondition.java | 7 + .../CompositeLootItemCondition.java | 12 + .../loot/predicates/LootItemCondition.java | 2 + .../loot/providers/number/NumberProvider.java | 2 + .../fixtures/summoningrituals-mc-stubs.jar | Bin 0 -> 13676 bytes .../summoningrituals.accesstransformer.cfg | 19 + .../manual/access-transformer/real-mc.test.ts | 265 +++++++ .../access-transformer-service.test.ts | 719 ++++++++++++------ .../services/bytecode-index-service.test.ts | 147 ++++ docs/specs/access-transformer-support.md | 35 +- src/services/access-transformer-service.ts | 480 +++++------- src/services/bytecode-index-service.ts | 202 +++++ src/services/decompile-service.ts | 6 + src/services/documentation-service.ts | 2 +- 24 files changed, 1458 insertions(+), 505 deletions(-) create mode 100755 __tests__/fixtures/at-fixture-src/build.sh create mode 100644 __tests__/fixtures/at-fixture-src/com/mojang/serialization/Codec.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/LocationPredicate.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/MinMaxBounds.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/StatePropertiesPredicate.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/client/gui/screens/inventory/tooltip/ClientTextTooltip.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/util/FormattedCharSequence.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/world/entity/boss/wither/WitherBoss.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/IntRange.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/AnyOfCondition.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/CompositeLootItemCondition.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/LootItemCondition.java create mode 100644 __tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/providers/number/NumberProvider.java create mode 100644 __tests__/fixtures/summoningrituals-mc-stubs.jar create mode 100644 __tests__/fixtures/summoningrituals.accesstransformer.cfg create mode 100644 __tests__/manual/access-transformer/real-mc.test.ts create mode 100644 __tests__/services/bytecode-index-service.test.ts create mode 100644 src/services/bytecode-index-service.ts diff --git a/CLAUDE.md b/CLAUDE.md index 71ea8cf..794c241 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -329,7 +329,7 @@ Deterministic central-directory scan: any `.class` entry → decompile via VineF 8. **`compare_versions`** - Compare classes/registries between versions 9. **`analyze_mixin`** - Analyze and validate Mixin code 10. **`validate_access_widener`** - Validate access widener files -10a. **`validate_access_transformer`** - Validate Forge/NeoForge access transformer files (parses `.cfg`, checks signatures, detects record canonical-constructor crashes, inner-class accessibility, and conflicting modifiers; mojmap-only) +10a. **`validate_access_transformer`** - Validate Forge/NeoForge access transformer files (parses `.cfg`, checks signatures, flags record canonical-constructor widening, inner-class accessibility, and conflicting modifiers; mojmap-only). Validates against the **remapped JAR's bytecode** (via the bundled ASM `bytecode-dumper`), not decompiled `.java` — decompiled source omits compiler-generated record members (canonical constructors, component accessors), so a source-based check false-flags them as missing (issue #12). Bytecode carries every member with its true access flags and erased descriptors (the same facts `javap` shows). Only needs the remapped JAR, which `decompile_minecraft_version` produces; per-class results are cached in a `remapped/{version}-{mapping}.bytecode.json` sidecar keyed by JAR signature. 11. **`compare_versions_detailed`** - AST-level version comparison 12. **`index_minecraft_version`** - Create full-text search index 13. **`search_indexed`** - Fast FTS5 search on indexed versions diff --git a/__tests__/fixtures/at-fixture-src/build.sh b/__tests__/fixtures/at-fixture-src/build.sh new file mode 100755 index 0000000..e547c62 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/build.sh @@ -0,0 +1,14 @@ +#!/usr/bin/env bash +# Rebuild the Access Transformer fixture JAR from the stub sources. +# The compiled JAR (../summoningrituals-mc-stubs.jar) is committed so tests do +# not need a JDK at runtime; re-run this only when the stub sources change. +# Requires JDK 17+ (records). Usage: bash build.sh +set -euo pipefail +here="$(cd "$(dirname "$0")" && pwd)" +out="$here/../summoningrituals-mc-stubs.jar" +tmp="$(mktemp -d)" +trap 'rm -rf "$tmp"' EXIT +mapfile -t srcs < <(find "$here" -name '*.java') +javac -d "$tmp" "${srcs[@]}" +( cd "$tmp" && jar --create --file "$out" . ) +echo "Wrote $out" diff --git a/__tests__/fixtures/at-fixture-src/com/mojang/serialization/Codec.java b/__tests__/fixtures/at-fixture-src/com/mojang/serialization/Codec.java new file mode 100644 index 0000000..7bf4ec6 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/com/mojang/serialization/Codec.java @@ -0,0 +1,3 @@ +package com.mojang.serialization; +// Minimal stub so fixture descriptors resolve to com/mojang/serialization/Codec. +public interface Codec {} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/LocationPredicate.java b/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/LocationPredicate.java new file mode 100644 index 0000000..df97e8a --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/LocationPredicate.java @@ -0,0 +1,8 @@ +package net.minecraft.advancements.critereon; +// Public enclosing class so the inner-class accessibility check passes. +public class LocationPredicate { + // Package-private nested record: implicit canonical constructor + accessors + // exist in bytecode but never appear in decompiled source. Reproduces + // net.minecraft.advancements.critereon.LocationPredicate$PositionPredicate. + record PositionPredicate(MinMaxBounds.Doubles x, MinMaxBounds.Doubles y, MinMaxBounds.Doubles z) {} +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/MinMaxBounds.java b/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/MinMaxBounds.java new file mode 100644 index 0000000..264a462 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/MinMaxBounds.java @@ -0,0 +1,5 @@ +package net.minecraft.advancements.critereon; +public class MinMaxBounds { + // Nested type targeted only as a parameter type in descriptors. + public static final class Doubles {} +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/StatePropertiesPredicate.java b/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/StatePropertiesPredicate.java new file mode 100644 index 0000000..0f62392 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/advancements/critereon/StatePropertiesPredicate.java @@ -0,0 +1,9 @@ +package net.minecraft.advancements.critereon; +import java.util.Optional; +// Public enclosing class; nested records are package-private (as in vanilla). +public class StatePropertiesPredicate { + public interface ValueMatcher {} + record ExactMatcher(String value) implements ValueMatcher {} + record PropertyMatcher(String name, ValueMatcher valueMatcher) {} + record RangedMatcher(Optional minValue, Optional maxValue) implements ValueMatcher {} +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/client/gui/screens/inventory/tooltip/ClientTextTooltip.java b/__tests__/fixtures/at-fixture-src/net/minecraft/client/gui/screens/inventory/tooltip/ClientTextTooltip.java new file mode 100644 index 0000000..9070f99 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/client/gui/screens/inventory/tooltip/ClientTextTooltip.java @@ -0,0 +1,6 @@ +package net.minecraft.client.gui.screens.inventory.tooltip; +import net.minecraft.util.FormattedCharSequence; +public class ClientTextTooltip { + private final FormattedCharSequence text; + public ClientTextTooltip(FormattedCharSequence text) { this.text = text; } +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/util/FormattedCharSequence.java b/__tests__/fixtures/at-fixture-src/net/minecraft/util/FormattedCharSequence.java new file mode 100644 index 0000000..33e8ee2 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/util/FormattedCharSequence.java @@ -0,0 +1,2 @@ +package net.minecraft.util; +public interface FormattedCharSequence {} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/world/entity/boss/wither/WitherBoss.java b/__tests__/fixtures/at-fixture-src/net/minecraft/world/entity/boss/wither/WitherBoss.java new file mode 100644 index 0000000..1a09ca2 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/world/entity/boss/wither/WitherBoss.java @@ -0,0 +1,7 @@ +package net.minecraft.world.entity.boss.wither; +public class WitherBoss { + private final float[] xRotHeads = new float[3]; + private final float[] yRotHeads = new float[3]; + private final float[] xRotOHeads = new float[3]; + private final float[] yRotOHeads = new float[3]; +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/IntRange.java b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/IntRange.java new file mode 100644 index 0000000..8e843b0 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/IntRange.java @@ -0,0 +1,9 @@ +package net.minecraft.world.level.storage.loot; +import net.minecraft.world.level.storage.loot.providers.number.NumberProvider; +// Plain class (NOT a record). Private constructor + private fields (matches +// real 1.21.1: min/max are private, the (NumberProvider, NumberProvider) ctor is private). +public class IntRange { + private final NumberProvider min; + private final NumberProvider max; + private IntRange(NumberProvider min, NumberProvider max) { this.min = min; this.max = max; } +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/AnyOfCondition.java b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/AnyOfCondition.java new file mode 100644 index 0000000..53db5e0 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/AnyOfCondition.java @@ -0,0 +1,7 @@ +package net.minecraft.world.level.storage.loot.predicates; +import java.util.List; +public class AnyOfCondition extends CompositeLootItemCondition { + // Package-private constructor (as in vanilla). A constructor is NEVER + // overridable, so widening it must not produce an "overridable" warning. + AnyOfCondition(List terms) { super(terms); } +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/CompositeLootItemCondition.java b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/CompositeLootItemCondition.java new file mode 100644 index 0000000..9eb6c9f --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/CompositeLootItemCondition.java @@ -0,0 +1,12 @@ +package net.minecraft.world.level.storage.loot.predicates; +import com.mojang.serialization.Codec; +import java.util.List; +import java.util.function.Function; +public abstract class CompositeLootItemCondition implements LootItemCondition { + protected final List terms; + protected CompositeLootItemCondition(List terms) { this.terms = terms; } + protected static Codec createInlineCodec( + Function, T> factory) { + return null; + } +} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/LootItemCondition.java b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/LootItemCondition.java new file mode 100644 index 0000000..d477541 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/predicates/LootItemCondition.java @@ -0,0 +1,2 @@ +package net.minecraft.world.level.storage.loot.predicates; +public interface LootItemCondition {} diff --git a/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/providers/number/NumberProvider.java b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/providers/number/NumberProvider.java new file mode 100644 index 0000000..d474230 --- /dev/null +++ b/__tests__/fixtures/at-fixture-src/net/minecraft/world/level/storage/loot/providers/number/NumberProvider.java @@ -0,0 +1,2 @@ +package net.minecraft.world.level.storage.loot.providers.number; +public interface NumberProvider {} diff --git a/__tests__/fixtures/summoningrituals-mc-stubs.jar b/__tests__/fixtures/summoningrituals-mc-stubs.jar new file mode 100644 index 0000000000000000000000000000000000000000..b3fb1acf6ab02f83c7be55c4c9e2cdd5b0dbd8c0 GIT binary patch literal 13676 zcmbtb2Rzl^`xmnJmXbXqdy_4Dk0f<*jf`ttDDz&(V zd8g+bIh9l|DXI=S7glL;A%`qdo@22LV2YoQkGs9@xaN2UstFxKd0R_N2r5?)`UBH^ zypgETe>DZ!UOxE?>|bBlf^GxsED(H1dGtdkpraMQ#_A3LYz4C0XFGoG42aldKxRNw zPE#9zlauII(5M6&8kz!{nhP3}JX+oCw$aYSc5z!hcxPrfv#%|sudpqqZ(Gc3VLNWv z%ms~19?iG7twMI^d=$}McJc5mkv!V->Ao-ZZ81ZcI`U|^4+z*Vf*D6#lBC0MjGDja z$aNRk-4~YhGP*_eMY3m%f|7zViIT=A*q2nLg8Pxb72dvIbrZI#1E4lnLVr&78qBYD zKrjM#;r;r;*2)fO>Ig7L^exptz6F@M0PIYGwm>_u69OfSh#5^Ct-wG>Aj0)gJFN&6 zkm>$fYd8YUte`_6g9gaS3by)x{^npM+jXw16HAfoLWb-LLK`qJSjej_pBm}ar`P&8 zMoSLS5w`d$K8!*YQSIXO%*@c1C}YH1#=izbc4t*8LrP3+LvI9;@Cts zck#6+R<9J-2{N;}G^=vnQ!4Gc(b~I`8(p_GJn+6Y8g4^CloDO#fqI3 zf|L?D$)TgNyYeDVxklk{S2JCT7NsuBP4Y~ZxND~So0su^s34y$4UzbwowGF>at%x= zantmk)54v^H}mdR*;lyE^HVlI*4gXiZ)hGcWS8QT!C{ZriV$69+R65KIOnKLqNmz< z$-#t{P*h8TzvP)85b`{(k@Mv&_$vt|opt8S?Dxz$##gU29&%r1nam~q)Wi5r>1-IcNqPlmXA`6(y3z>w**a?p8MN|JLQ`Ah~0(&Sdp6Jr_Yy9Fyke0eET zLO4o@%6c^|Oe}j-V|dS}CH7aiw&RUjH!;5Cj6&@WBM^`a$)(G!YisL^Iy85fhfbEBq1()t(&Qm~= zhjg2fW%lMYo{;l%QEL<1X6zEBODKIz)59!l)U(aJO?plWhv0UU9=+N12EC#kq?eG$ z3`_b>%Tf7ALeXq5hzDO#*PXOYD?5Aw?t8;osL<4KaR3x)<6}dbpE^yp+R%JOEtP)> zRvDR z;vy^+^RM9!endt>Is-i=PCf+0{&k=ogjK3Wvl0q7f!C6qtMk+G5r4Ne8aEC6`FTYw zErLKp?2t_xm$*1vWX18lbq2Rplx-FVm&kACXy#KMvFMTCcfW5V+xzm`h4w`DgXfa`S z3$-H4lpAh^xH3dn^GfHrQ_5YR8SBnI)P40xORTPX6l`tiSUXp5qm79(Pcnm2{Vd9q z#eTDqCJ&pu@1f;fL-lN4IF_$pliM=3TGU3#ICJfm;e_k`3$EuEZ+c4ByDs~Aj09tj z`dHvIesjw${$$#gzo-$x9pQZj=H$-a*KdA8T^Ml&k&~~S*2$_?cB%k3X^^v>nG=I7 z$l1gO=yc%cc&MLSB?-Jl=Q@ zmIi~j<4k0QUarVx@g?d>c+eJw4>JlDHxTFOrg2blBq&{;Th6g^p2I6sxjwvtvDTua zLh<(O&q70niZUP|%Sj6@M`-QhKBR zHL5Hh19>gJoT(2LjXCcH(Hms)1i!-YoR+1;ba35BO?tA;YN0(-R8~h}-*zgaNV;81^u`pz1mcQsDyU2QP*4AyW2*#jNHRzRmehcJVj8^9E-3ILm0 z0v!+BUh?ZimbZM&QN%$yn`xv|XsbE$6PI6e*oQR{S^_tVkEzmFL=xsNU94wrdGglz{aHVdUV$Kr&&tjLTu3-`ui>|nID|G-GaXFZ@-9Q zniDOOKW8{tC_nB-Qf$fQ&iORg*gK~5T3iQ5EZIFivm7<#r51@`-7N9DrULzDFUGFY zF%s|=&XKx3xA78gT#1A$uyR( zGq%0$s*e3OpGpD;O(X5aQ(jMDpjW1f_myCOsriv`iR-ymgCRo$Qz+@TY_je+T`RfU z*BCoYCQ)8<`dg+w3v5LC7J$K^r+ee+mmciUh@H@nDoMSuMwv~0MNI~^bgQ!lxYNEL zjpK%$a#U_r6<5oi<^IBWw%Ckhl&Cg>5<4QCs+N_j{Ic|@*L&Rpj@(pMg3F)Ch{q&Y z#y6;LhoTRY}vw|9gXB|Stv-)qKJ?8G4?44HQi+EZa8AS0N1(syG&kE%RLr+UlG(?G9 z`#*=luh4Km8VR}wchlSfyX)kJY%KSTbOv0cMt<6!xlDnQOLL~hUqbaU{S8ak<&`ST zy>4_x1}3pBlx@w9O512@eG}=6=IS2bL?3@n5&pTnIjD|Qwk8scafk2AhkJA#QB!ly zb_sP)P3fejpj?Q3y@J5X$A^?&dp*%pTcO#=O@LS;w$5y2!?si_sz*?J zQvu5gt>Jku|Jo?a!$t$LH?~v7ID;{R4wU{l4s-ljykoDi(S)@Nf&fG z5^X9g8%KAIE#6s-ry1hEkX5Vpk@V*6`W0T1rx)@{TJQsJ0NB|#qkSzz**}}3IKAPM zSDn{#cdXMZA_Tr5owW4;AKR5Nt@U zY8Peha$LEo=Oo3$ciy^1zc;OnZ%no6xKtwQnt^-s~qulpM$oxz}{c9^{ZZajEl3EvYWN$q&sR^ zK#W_lA)Av_xL9Mm`mM;VU+zZdzGH0N`mS-?n>fs)%9y^2l&%Z)fw+NRb15Tb7I8a8 z17+S#SHBl$yW`eS`jUhL%=@S_Kl|tm@1Ug&YPT|o6BDk>8Hk`%X@UldnmSRt(Yl1_ z#Gg5j;SJ!`QzBC%KZE!vI24^t%UM`#nWkQ6Dsz^3f3mljeU8CM_s705tn6l-_6&OA&|@Ulj6paK*_-v(QM}gfe{ovj+BfrHf(i|tW|^O9V@q zV}X7^KLJxNhp{aN}hs{Y(d?4tK!yVeGY><{~_q zrQ2C4`F0Ie^x*r-cXha0R(BmWlP~;u`}XSKm_VJ)7=w0CUUOwC-HQc^3h%L$hnE}L z2R>HQrsZXio~w>|Btgp8Ku70Wn-3KvG)6C&|1` z&18WyVS^%5HF(juCg*R|s)9Br%g|oC&6682c_1n^Bg5oyVz&B-wfa3C4N6k zB?-XjOBbOt*9NhY1)1>&E5mlL$8FE2h-7oWC$#Ill<*lDrn6VtaA~*b`Zn-`^-1tD zE%kd$^KL=eo}ggU2&HpOq8CG^`Mr9`xwrO1@R~MuI4TMf(kQ|Z{O{K~J%Eif@JIya zL~yI2XyV9rVy@UFkQn(yB;WNT!N5F2M8Hbau)!O3(RP>aE^|=UG}3vja$g7pi`@I1 zwODfGi^r}=iAf_An4fsWZM4n#yuPIuHOzN$R3;K|2#0&&Ve2nU{LoKkPN;K;pRpIY zJ7m}C@wGPhl@L*i9k?vxnMH&)mX;Z+$Cs z@msQeUR9)~44|C-{3KqItK&tOgV+io+wfbP0X!RrRX(8trkk+aK)R!JXA?9%2_n1{ z%ly{_c5n~QiRp(v?MMP&d|@uPvM$mqQ6dk)b_N(ro@K?_ts-NeRF5sn%_hFD5-O3v z-W-TOEfXv#r9^53MEmqE1t9Th+ta(@E|O4tg8%t-0W4Ay0OqiQBip1#A^N7d5Zwz5 z0|j5KzFb)NCmF}{|E0}6Axrkq&7$RYP49d<(i%7frz-K0n z>B_1Q5la>Q!yK3Xd!Nk27`&K+XqnZnBwI#4_iwA(MLEwTjurQmwdty~Sh`OM;^zg?v|{e8qOy@IMcW1OM6i-^)F1vz0gS)ZwD76znC9qdwhLN`Yv^HU^S__{9e~QNVB`~Nc9&&b8)4V^!kFBV+ zo9=^wJc{*-s}{)gf~uNrsj6{hdGq+K$ttC{u$9Y8T)n3VU9}15@pCdzO8SqLGqtgT zqM8UmO&HL8^nLk@#lqPN0UIqMHYZa@AkYpGl*)b@b}KsB=k~Qq+bvS}{(aW2vX)m+e=G%XG-kUjWR&O+7Sc%LS!o?z-UTEeiii^svF{3FxKaMwZ zwVjJePgSodP0_@}>IYmdF>fmGq$~!y3Zn~cp}{tA?#j>uH0TEQ{*`);ub>eB!~4!) zD;osM#d$n~Jjl@&00sliWGn%W+CT?qAQay}SQ2>Y!)3qp5new`v@_9HKRlb+Rz+yExL|aVEFv&t#Tjum|%g}KBtj?h#ND8!0;`O@>QaZgolsT*OxGx1-2VpHX z{3`k6Pq`!7m-H$R_oIU%62(i@2^Xln5zwELV;EMxIvS9!AV(WB1TpZ>g2n*{Gh z9UMG3Y+2+yn(Fdy+8C8I#LqxuFtA8WUR9C!YTk%v0yd%SHqsTPdfvgFCh^U*cPT%& zet7Sp+6UO;qp6@LM3UJ+z6sq4aib>1_aupwqoZ8$FYcP9^{=b+cyEGPit?h9#}l$6 zeO00xD_Uo;VRoaUN1GGtes^-`9JpuUz9$K3<12d=9RHu5*Lt)GbK&}+EZ z?QG_Z@n6g^5oe^}Sj4?}L3I>6xmko&yO|*64Pk`CRw2(AEa??uxoKJkj14ygr1as1%Iju?OV_eR-SJ>+Bj};qvaA) zJ7>2P4>nV^7Vh;$t5%NwP49gO>2-Xz=0BAY`$81$!29p2gUwSCEd@)=a*eS98MKUyO=k=S z(E{+1q@}N7WMdJee@boBDTtw{vzw+)36KkH^Fy&`X%lKnlM~!cTidcS-`!8n+AFB` z(?6i;xy6*CPnO<|^#g08ngtIp&Xj{D`Fue0g$O3`;*14)jPi&m-_fDf!hB$EyZ<|N zr9|P&Kzd~fL8Z`w*&Hp%&5`+pq!=2VetYiSg-(T%WV*yV&zI%mmHNvJ>%OaAeC;hD zEn7qRnCJ!0{&)5AAdeishVuVPo|K)ty15L<&TPLZda$Ue8mh|BVnS*Oe|h=1 z%x6ErD@{9RINFI=Fq)~^sOxQGttelwB=l#`ynq#Lppa{uqS|fsCtOcI_g~tW@9$iA zBZ{N~h>}GAX0rGVaFOopeVGwP;XeOs<=*SfWaBerC8cBNmGZU?77dsN8g>-ysXrXw zvzzo~+qW9zPLa0Frutc`M7U+%a!U|TE*a#HeMPA&Cej%BPO|yy+p~F}+@Jkm8dCN0 z#X`Fet}E$fgosvF#Vm1He#9A-T5bJ&rep8XllS&~gcq@50?T9~cyH?jU0HZ~+w2{Q z$u+Q3TtKm*f%-sjy{g2MTF-0Tehyc9CIgXy*LjB@%QEpZp5? z<9)96uY4{8vbBd*p@AyUs8j?4ZU6FrwYDmV7+N%&%6@H*S;ZPCeHlYdHF~w-DOMq! zZef+5OctGklYrf9m$ia1D0)kG&L>Lf3;LIZRTPcRqVt7u_`c)*;x!BIQ&b-F9~L&1 zk%Gol2{K5tCR@*m;5=}0;H)Jx?|7>|U$KglszNyzgj3ScY(>7j*m3oan-8^&UG_o_ zPlf$V--i0g66@%?(QRY*SA+O_HFx7<8pSBnMYnl`fL=q}6#a=2S9 zWDu||#SMI@wB%An>RD9~kf?q6Sv~0PaNOie?C{Qa_jhAJBk2?|#MhrnQx~jSUG{$1 z`yjn~CnlK@xmD)en+!mgoyHrvvFyRzh~?PH%vgOh`ikyd!ACozHwo`%T!<&^zfd7r zUtg!HnKr$&KE?UnPtb&%eakHPZN_A!`Pc1pu>GM;>f4kK-5Xz_kLZ)jr;7h-f1I!_ zfT7%2Es_Lsd$OucJw2vE7EwMl7-VU;Cc^-&<_ZF)^$!BkS2ax9hV;eU+C3UBsv}+3 zA;pqTZL7ss^mHMA|8#!LK2DPkKlgQYi{`7Xxe-CbBdtXy_~YN#d|gVn=F_Cl->knvDk6@#bZiS?8PAH+;X5lCttz1|CiZ= zT&&E1j)*Qwu*RX|Zgj_-boP$F#o0OAnjpM6sr(12YWu>WnEk%(2gy@6R1~j;i$K-_ zg@K5aX!F(!$ylN&A>a3xqUk&3sX8aZ_s_t`fDreg#r_?KhBITCg&G7OXoAtn+X{sl zpUHmZ@aUd#SJ6~uUNKei97(#tF5;L3k&kn|c;4Fywnh(Z#vTl;JzHvLYFVdXuGSL0 z9Po9|Vq-)Z82|xcgzA&CJcR8~eCA1TAarsEYjg$~3+ea^2&^USDE;aBI%p6(gG_=1 zPk)D7xL|i|IKdvHe^?JSjAX{xpJ}%m{m-!)X;d zJjD2SW7yF-A8xN=fXjf6n4u})L@|dN#ZK>o9jHXuE=R86P0Dn*Ma0v-kb%m2L?tj&OYz_8r0}cb|uu2u2iU|Iz0msy> z{#JhkfrjgbjyR!7`M-JppP2pM+7F|sFq9ikHc;sFKWT?=wv)~)7&r?jCi1@({()*w zdlXzdNyGtkd$Jf>xEat<6e2NjDEDM>u=5j6FHR%wFtmHJU>LXyCkl!aJO-QYZ#&_F z;ZX0`g45r_|wDKzjoLPxrY1^t36Pc{pNIKe6Q z6w>~{mq%CPFt8>Fmk1q=onGkQa3&%@z%V*E8D2Qvp}!HklRZEO7Xlr{Ae8ePfkR~1 zQI8ItO5QI-@2E!)BTq0)4^HINh$j2n$YT&bM0Udhm$Szm2>K8|9)yC5)XU3!>Rt?i2w7FKe~2@pSiHNJe-K2FO>gz62G(@ a_p;YeJ_~&nAR)0sf63t^ArUgc{`7xQ&3IV= literal 0 HcmV?d00001 diff --git a/__tests__/fixtures/summoningrituals.accesstransformer.cfg b/__tests__/fixtures/summoningrituals.accesstransformer.cfg new file mode 100644 index 0000000..7730a46 --- /dev/null +++ b/__tests__/fixtures/summoningrituals.accesstransformer.cfg @@ -0,0 +1,19 @@ +public net.minecraft.advancements.critereon.LocationPredicate$PositionPredicate +public net.minecraft.advancements.critereon.LocationPredicate$PositionPredicate (Lnet/minecraft/advancements/critereon/MinMaxBounds$Doubles;Lnet/minecraft/advancements/critereon/MinMaxBounds$Doubles;Lnet/minecraft/advancements/critereon/MinMaxBounds$Doubles;)V +public net.minecraft.advancements.critereon.StatePropertiesPredicate$ExactMatcher +public net.minecraft.advancements.critereon.StatePropertiesPredicate$ExactMatcher value()Ljava/lang/String; +public net.minecraft.advancements.critereon.StatePropertiesPredicate$PropertyMatcher +public net.minecraft.advancements.critereon.StatePropertiesPredicate$PropertyMatcher name()Ljava/lang/String; +public net.minecraft.advancements.critereon.StatePropertiesPredicate$PropertyMatcher valueMatcher()Lnet/minecraft/advancements/critereon/StatePropertiesPredicate$ValueMatcher; +public net.minecraft.advancements.critereon.StatePropertiesPredicate$RangedMatcher +public net.minecraft.advancements.critereon.StatePropertiesPredicate$RangedMatcher maxValue()Ljava/util/Optional; +public net.minecraft.advancements.critereon.StatePropertiesPredicate$RangedMatcher minValue()Ljava/util/Optional; +public net.minecraft.client.gui.screens.inventory.tooltip.ClientTextTooltip text +public net.minecraft.world.entity.boss.wither.WitherBoss yRotHeads +public net.minecraft.world.entity.boss.wither.WitherBoss yRotOHeads +public net.minecraft.world.level.storage.loot.IntRange (Lnet/minecraft/world/level/storage/loot/providers/number/NumberProvider;Lnet/minecraft/world/level/storage/loot/providers/number/NumberProvider;)V +public net.minecraft.world.level.storage.loot.IntRange max +public net.minecraft.world.level.storage.loot.IntRange min +public net.minecraft.world.level.storage.loot.predicates.AnyOfCondition (Ljava/util/List;)V +public net.minecraft.world.level.storage.loot.predicates.CompositeLootItemCondition createInlineCodec(Ljava/util/function/Function;)Lcom/mojang/serialization/Codec; +public net.minecraft.world.level.storage.loot.predicates.CompositeLootItemCondition terms diff --git a/__tests__/manual/access-transformer/real-mc.test.ts b/__tests__/manual/access-transformer/real-mc.test.ts new file mode 100644 index 0000000..3e9989a --- /dev/null +++ b/__tests__/manual/access-transformer/real-mc.test.ts @@ -0,0 +1,265 @@ +import { existsSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { beforeAll, describe, expect, it } from 'vitest'; +import type { BytecodeClass } from '../../../src/java/bytecode-dumper.js'; +import { getBytecodeDumper } from '../../../src/java/bytecode-dumper.js'; +import { verifyJavaVersion } from '../../../src/java/java-process.js'; +import { getAccessTransformerService } from '../../../src/services/access-transformer-service.js'; +import { getBytecodeIndexService } from '../../../src/services/bytecode-index-service.js'; +import { getRemapService } from '../../../src/services/remap-service.js'; +import { getRemappedJarPath } from '../../../src/utils/paths.js'; + +/** + * Real-Minecraft Access Transformer tests (issue #12). + * + * The network+Java-heavy counterpart to the deterministic, committed-fixture + * test in `__tests__/services/access-transformer-service.test.ts`. Everything + * here validates against genuine downloaded+remapped Mojang bytecode (no fixture, + * no committed MC IP). + * + * The validator itself is version-agnostic: it reads whatever version's remapped + * bytecode you point it at. To PROVE that across the 1.21.x line, the + * cross-version block below builds a VERSION-CORRECT access transformer for each + * MC version from that version's own bytecode (records + their real component + * accessors) and asserts it validates cleanly — implicit record members found, + * informational (never "crash") record notes, no bogus "overridable" ctor + * warning. This is a real per-version regression of the exact issue-#12 fix. + * + * (The reporter's verbatim Summoning Rituals AT is 1.21.1-specific — it targets + * the `advancements.critereon` package, which Mojang renamed to `criterion` + * later in 1.21.x — so it is exercised only against 1.21.1.) + * + * Excluded from default CI (manual dir; network + full remap per version). + * Run with: + * npm run test:manual -- __tests__/manual/access-transformer + * AT_MC_VERSIONS=1.21.1,1.21.11 npm run test:manual -- __tests__/manual/access-transformer + */ + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const AT_FILE = join(__dirname, '..', '..', 'fixtures', 'summoningrituals.accesstransformer.cfg'); +const STUB_JAR = join(__dirname, '..', '..', 'fixtures', 'summoningrituals-mc-stubs.jar'); +const MAPPING = 'mojmap' as const; + +// Full 1.21.x line by default; override with AT_MC_VERSIONS (comma-separated). +const VERSIONS = ( + process.env.AT_MC_VERSIONS ?? + '1.21.1,1.21.2,1.21.3,1.21.4,1.21.5,1.21.6,1.21.7,1.21.8,1.21.9,1.21.10,1.21.11' +) + .split(',') + .map((v) => v.trim()) + .filter(Boolean); + +// A record-heavy vanilla class present across all of 1.21.x. Only the package +// name drifts (`critereon` -> `criterion`), so we probe both spellings. +const RECORD_SIMPLE_NAMES = [ + 'StatePropertiesPredicate$ExactMatcher', + 'StatePropertiesPredicate$PropertyMatcher', + 'StatePropertiesPredicate$RangedMatcher', +]; +const PACKAGE_SPELLINGS = [ + 'net/minecraft/advancements/criterion', + 'net/minecraft/advancements/critereon', +]; + +const vis = (flags: string[]): 'public' | 'protected' | 'private' | 'package' => + flags.includes('public') + ? 'public' + : flags.includes('protected') + ? 'protected' + : flags.includes('private') + ? 'private' + : 'package'; + +interface GeneratedAt { + /** AT file text targeting real members of THIS version. */ + text: string; + /** Dotted record class names widened (class-level entries). */ + records: string[]; + /** How many widened records have a non-public canonical ctor (== expected notes). */ + expectedCtorNotes: number; + /** Dotted accessor targets `Class name()desc` we assert resolve. */ + accessorCount: number; +} + +/** + * Build a version-correct AT from a version's real bytecode: widen each of the + * three matcher records and every one of their real component accessors (names + + * erased descriptors read straight from the record components). The canonical + * constructors are intentionally left un-widened, so each widened record yields + * exactly one informational note. + */ +async function buildRecordAt(version: string): Promise { + const index = getBytecodeIndexService(); + for (const pkg of PACKAGE_SPELLINGS) { + const internals = RECORD_SIMPLE_NAMES.map((n) => `${pkg}/${n}`); + const map = await index.getClassBytecode(version, MAPPING, internals); + const found = internals.filter((i) => map.get(i)?.isRecord); + if (found.length === 0) continue; + + const lines: string[] = []; + const records: string[] = []; + let expectedCtorNotes = 0; + let accessorCount = 0; + for (const internal of found) { + const bc = map.get(internal) as BytecodeClass; + const dotted = internal.replace(/\//g, '.'); + lines.push(`public ${dotted}`); // widen the record class + records.push(dotted); + for (const comp of bc.recordComponents ?? []) { + // Record accessor: name == component name, descriptor == (). + lines.push(`public ${dotted} ${comp.name}()${comp.descriptor}`); + accessorCount++; + } + const canon = bc.canonicalConstructor; + const ctor = canon + ? bc.methods.find((m) => m.name === '' && m.desc === canon) + : undefined; + if (ctor && vis(ctor.flags) !== 'public') expectedCtorNotes++; + } + return { text: lines.join('\n'), records, expectedCtorNotes, accessorCount }; + } + throw new Error(`No StatePropertiesPredicate matcher records found in MC ${version}`); +} + +async function dumpByName(jarPath: string): Promise> { + const dump = await getBytecodeDumper().dump(jarPath); + return new Map(dump.classes.map((c) => [c.name, c])); +} + +describe('Access Transformer vs REAL Minecraft (issue #12)', () => { + beforeAll(async () => { + await verifyJavaVersion(17); + }, 30000); + + // ---- Cross-version support: 1.21.1 -> 1.21.11 (version-correct AT each) ---- + describe.each(VERSIONS)('cross-version support: MC %s', (version) => { + beforeAll(async () => { + await getRemapService().getRemappedJar(version, MAPPING); + }, 600000); + + it('validates a version-correct record AT with zero false positives', async () => { + const gen = await buildRecordAt(version); + expect(gen.records.length).toBeGreaterThan(0); + expect(gen.accessorCount).toBeGreaterThan(0); + + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformer(gen.text); + expect(at.parseErrors).toEqual([]); + + const result = await svc.validateAccessTransformer(at, version, MAPPING); + + // Every entry targets a REAL member of this version's bytecode, so there + // must be zero errors — this is the core issue-#12 guarantee (implicit + // record accessors + canonical ctors are found, not falsely "not found"). + expect(result.errors).toEqual([]); + expect(result.isValid).toBe(true); + + // Records are handled: one informational note per widened record whose + // canonical ctor is non-public, worded as advisory (never "crash"). + const notes = result.warnings.filter((w) => w.message.includes('canonical constructor')); + expect(notes.length).toBe(gen.expectedCtorNotes); + expect(notes.every((w) => /INSTANTIATE/.test(w.message))).toBe(true); + expect(result.warnings.some((w) => /crash/i.test(w.message))).toBe(false); + // A constructor is never flagged overridable. + expect(result.warnings.some((w) => w.message.includes('overridable'))).toBe(false); + }, 600000); + + it('still rejects entries that do NOT exist in this version (no rubber-stamping)', async () => { + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformer( + [ + 'public net.minecraft.totally.MadeUpClass', + 'public net.minecraft.world.item.ItemStack thisMethodDoesNotExist()V', + ].join('\n'), + ); + const result = await svc.validateAccessTransformer(at, version, MAPPING); + expect(result.isValid).toBe(false); + expect( + result.errors.some((e) => + e.message.includes("Class 'net.minecraft.totally.MadeUpClass' not found"), + ), + ).toBe(true); + expect( + result.errors.some((e) => e.message.includes("Method 'thisMethodDoesNotExist' not found")), + ).toBe(true); + }, 600000); + }); + + // ---- 1.21.1 flagship: the reporter's verbatim Summoning Rituals AT ---- + describe('1.21.1 — verbatim Summoning Rituals AT', () => { + beforeAll(async () => { + await getRemapService().getRemappedJar('1.21.1', MAPPING); + }, 600000); + + it('validates the real mod AT with zero errors and 3 informational record notes', async () => { + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformerFile(AT_FILE); + expect(at.parseErrors).toEqual([]); + expect(at.entries).toHaveLength(19); + + const result = await svc.validateAccessTransformer(at, '1.21.1', MAPPING); + + // v1.2.4 reported 7 errors, all false positives. Must be zero. + expect(result.errors).toEqual([]); + expect(result.isValid).toBe(true); + expect(result.warnings.some((w) => w.message.includes('overridable'))).toBe(false); + + const recordNotes = result.warnings.filter((w) => + w.message.includes('canonical constructor'), + ); + expect(recordNotes).toHaveLength(3); + expect(recordNotes.every((w) => /INSTANTIATE/.test(w.message))).toBe(true); + expect(recordNotes.some((w) => /crash/i.test(w.message))).toBe(false); + // PositionPredicate is exempt: the AT widens its constructor (line 2). + const noteClasses = recordNotes.map((w) => w.entry.className); + expect(noteClasses).not.toContain( + 'net.minecraft.advancements.critereon.LocationPredicate$PositionPredicate', + ); + }, 600000); + + it('the committed stub reproduces real 1.21.1 bytecode on every validated dimension', async () => { + // Definitive drift guard: dump the real remapped JAR and the committed stub, + // then assert they agree on exactly what the validator reads for the mod's AT. + expect(existsSync(STUB_JAR)).toBe(true); + const real = await dumpByName(getRemappedJarPath('1.21.1', MAPPING)); + const stub = await dumpByName(STUB_JAR); + + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformerFile(AT_FILE); + const targets = [...new Set(at.entries.map((e) => e.className.replace(/\./g, '/')))]; + + const nonPublic = (flags: string[]) => !flags.includes('public'); + for (const internal of targets) { + const r = real.get(internal); + const s = stub.get(internal); + expect(r, `real MC missing ${internal}`).toBeDefined(); + expect(s, `stub missing ${internal}`).toBeDefined(); + if (!r || !s) continue; + + expect(s.isRecord, `${internal} isRecord`).toBe(r.isRecord); + expect(s.canonicalConstructor, `${internal} canonicalConstructor`).toBe( + r.canonicalConstructor, + ); + for (const entry of at.entries) { + if (entry.className.replace(/\./g, '/') !== internal) continue; + if (entry.memberType === 'method' && entry.memberName && entry.memberDescriptor) { + const rm = r.methods.find( + (m) => m.name === entry.memberName && m.desc === entry.memberDescriptor, + ); + const sm = s.methods.find( + (m) => m.name === entry.memberName && m.desc === entry.memberDescriptor, + ); + expect(rm, `real ${internal}#${entry.memberName}`).toBeDefined(); + expect(sm, `stub ${internal}#${entry.memberName}`).toBeDefined(); + if (rm && sm) expect(nonPublic(sm.flags)).toBe(nonPublic(rm.flags)); + } + if (entry.memberType === 'field' && entry.memberName) { + expect(r.fields.some((f) => f.name === entry.memberName)).toBe(true); + expect(s.fields.some((f) => f.name === entry.memberName)).toBe(true); + } + } + } + }, 600000); + }); +}); diff --git a/__tests__/services/access-transformer-service.test.ts b/__tests__/services/access-transformer-service.test.ts index 4a50f7b..9876d2d 100644 --- a/__tests__/services/access-transformer-service.test.ts +++ b/__tests__/services/access-transformer-service.test.ts @@ -1,25 +1,52 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { + mkdirSync, + mkdtempSync, + existsSync as nodeExistsSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import { describe, expect, it } from 'vitest'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { beforeAll, describe, expect, it } from 'vitest'; +import type { + BytecodeClass, + BytecodeField, + BytecodeInnerClass, + BytecodeMethod, +} from '../../src/java/bytecode-dumper.js'; +import { getBytecodeDumper } from '../../src/java/bytecode-dumper.js'; import { handleValidateAccessTransformer } from '../../src/server/tools.js'; import { + type ClassBytecodeMap, detectAccessTransformerConflicts, getAccessTransformerService, - validateEntryAgainstSource, - validateEntryAgainstSymbols, + validateEntryAgainstBytecode, } from '../../src/services/access-transformer-service.js'; import type { AccessTransformer, AccessTransformerEntry } from '../../src/types/minecraft.js'; import { AccessTransformerParseError } from '../../src/utils/errors.js'; -import { extractJavaSymbols } from '../../src/utils/java-symbols.js'; import { findSimilarClassFile } from '../../src/utils/suggestions.js'; import { TEST_VERSION } from '../test-constants.js'; +const __dirname = dirname(fileURLToPath(import.meta.url)); +const FIXTURE_JAR = join(__dirname, '..', 'fixtures', 'summoningrituals-mc-stubs.jar'); +const FIXTURE_AT = join(__dirname, '..', 'fixtures', 'summoningrituals.accesstransformer.cfg'); +const DUMPER_JAR = join( + __dirname, + '..', + '..', + 'tools', + 'bytecode-dumper', + 'build', + 'libs', + 'bytecode-dumper-1.0.0.jar', +); + /** * Build an AccessTransformerEntry with sensible defaults for the regression - * tests below. The validation exercises go through the `validateEntryAgainstSource` - * / `validateEntryAgainstSymbols` test seams (synthetic Java source, no decompiled - * Minecraft tree required). + * tests below. Validation exercises go through the `validateEntryAgainstBytecode` + * test seam, driven by hand-built {@link BytecodeClass} fixtures — bytecode is + * the same ground truth the production path reads from the remapped JAR. */ function makeEntry(partial: Partial): AccessTransformerEntry { return { @@ -31,6 +58,50 @@ function makeEntry(partial: Partial): AccessTransformerE }; } +// --- Bytecode fixture builders (mirror the ASM dumper's JSON shape) --------- + +function bcMethod(name: string, desc: string, flags: string[] = ['public']): BytecodeMethod { + return { name, desc, access: 0, flags, signature: null, exceptions: [] }; +} + +function bcField(name: string, desc = 'I', flags: string[] = ['private']): BytecodeField { + return { name, desc, access: 0, flags, signature: null, value: null }; +} + +function bcInner(name: string, flags: string[]): BytecodeInnerClass { + return { name, outerName: null, innerName: null, access: 0, flags }; +} + +function bcClass(partial: Partial & { name: string }): BytecodeClass { + return { + access: 0, + flags: ['public'], + superName: 'java/lang/Object', + interfaces: [], + signature: null, + isInterface: false, + isEnum: false, + isRecord: false, + isAnnotation: false, + isAbstract: false, + isFinal: false, + isSealed: false, + nestHost: null, + nestMembers: null, + permittedSubclasses: null, + recordComponents: null, + canonicalConstructor: null, + innerClasses: [], + fields: [], + methods: [], + ...partial, + }; +} + +function mapOf(...classes: BytecodeClass[]): ClassBytecodeMap { + return new Map(classes.map((c) => [c.name, c])); +} + describe('Access Transformer Service', () => { it('parses the four line forms (class, field, method, constructor)', () => { const content = [ @@ -236,49 +307,35 @@ describe('Access Transformer Service', () => { }); }); -describe('Access Transformer Validation (tree-sitter + descriptor matching)', () => { - it('does NOT match a method that is only called (not declared)', () => { - // `helper` is invoked inside doWork() but never declared. A name-only check - // against call sites would false-positive; the AST walk only sees declared - // members, so `helper` is correctly reported missing with no spurious suggestion. - const src = ` -package net.test; -public class Caller { - public void doWork() { - helper(); - } -} -`; +describe('Access Transformer Validation (bytecode + descriptor matching)', () => { + it('reports a method that is not declared in the class', () => { + const cls = bcClass({ + name: 'net/test/Caller', + methods: [bcMethod('doWork', '()V')], + }); const entry = makeEntry({ memberType: 'method', className: 'net.test.Caller', memberName: 'helper', memberDescriptor: '()V', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors.some((e) => e.includes('not found'))).toBe(true); expect(res.suggestion).toBeUndefined(); }); it('validates constructor descriptors (match vs arity mismatch)', () => { - const src = ` -package net.test; -public class Thing { - private int x; - private int y; - public Thing(int a, int b) { - this.x = a; - this.y = b; - } -} -`; + const cls = bcClass({ + name: 'net/test/Thing', + methods: [bcMethod('', '(II)V')], + }); const matching = makeEntry({ memberType: 'method', className: 'net.test.Thing', memberName: '', memberDescriptor: '(II)V', }); - expect(validateEntryAgainstSource(matching, src).errors).toEqual([]); + expect(validateEntryAgainstBytecode(matching, mapOf(cls)).errors).toEqual([]); const wrongArity = makeEntry({ memberType: 'method', @@ -287,27 +344,24 @@ public class Thing { memberDescriptor: '(I)V', }); expect( - validateEntryAgainstSource(wrongArity, src).errors.some((e) => + validateEntryAgainstBytecode(wrongArity, mapOf(cls)).errors.some((e) => e.includes('no overload matches'), ), ).toBe(true); }); it('reports overload mismatch with the found overloads and matches a correct overload', () => { - const src = ` -package net.test; -public class Over { - public void foo(int x) {} - public void foo(String s) {} -} -`; + const cls = bcClass({ + name: 'net/test/Over', + methods: [bcMethod('foo', '(I)V'), bcMethod('foo', '(Ljava/lang/String;)V')], + }); const mismatch = makeEntry({ memberType: 'method', className: 'net.test.Over', memberName: 'foo', memberDescriptor: '(II)V', }); - const res = validateEntryAgainstSource(mismatch, src); + const res = validateEntryAgainstBytecode(mismatch, mapOf(cls)); const err = res.errors.find((e) => e.includes('no overload matches')); expect(err).toBeDefined(); // The actual overloads are reported (raw JVM descriptors) so the user can @@ -321,23 +375,21 @@ public class Over { memberName: 'foo', memberDescriptor: '(Ljava/lang/String;)V', }); - expect(validateEntryAgainstSource(ok, src).errors).toEqual([]); + expect(validateEntryAgainstBytecode(ok, mapOf(cls)).errors).toEqual([]); }); it('checks field existence by name only and suggests a close name when missing', () => { - const src = ` -package net.test; -public class Box { - public int counter; -} -`; + const cls = bcClass({ + name: 'net/test/Box', + fields: [bcField('counter')], + }); // AT fields carry NO descriptor — name existence is the only check. const missing = makeEntry({ memberType: 'field', className: 'net.test.Box', memberName: 'countre', }); - const res = validateEntryAgainstSource(missing, src); + const res = validateEntryAgainstBytecode(missing, mapOf(cls)); expect(res.errors.some((e) => e.includes('not found'))).toBe(true); expect(res.suggestion).toContain('counter'); @@ -346,94 +398,84 @@ public class Box { className: 'net.test.Box', memberName: 'counter', }); - expect(validateEntryAgainstSource(ok, src).errors).toEqual([]); + expect(validateEntryAgainstBytecode(ok, mapOf(cls)).errors).toEqual([]); }); - it('warns (does not error) on static initializers', () => { - const src = ` -package net.test; -public class WithStatic { - static { - System.out.println("init"); - } -} -`; - const entry = makeEntry({ - memberType: 'method', - className: 'net.test.WithStatic', - memberName: '', - memberDescriptor: '()V', - }); - const res = validateEntryAgainstSource(entry, src); - expect(res.errors).toEqual([]); - expect(res.warnings.some((w) => w.includes(''))).toBe(true); + it('errors on a class that is absent from the JAR', () => { + const res = validateEntryAgainstBytecode( + makeEntry({ memberType: 'class', className: 'net.test.Ghost' }), + mapOf(bcClass({ name: 'net/test/Real' })), + ); + expect(res.errors.some((e) => e.includes("Class 'net.test.Ghost' not found"))).toBe(true); }); it('warns on override-narrowing for an overridable method but not a final one', () => { - const overridableSrc = ` -package net.test; -public class Base { - public void tick() {} -} -`; + const base = bcClass({ + name: 'net/test/Base', + methods: [bcMethod('tick', '()V', ['public'])], + }); const overridable = makeEntry({ memberType: 'method', className: 'net.test.Base', memberName: 'tick', memberDescriptor: '()V', }); - const overridableRes = validateEntryAgainstSource(overridable, overridableSrc); + const overridableRes = validateEntryAgainstBytecode(overridable, mapOf(base)); expect(overridableRes.errors).toEqual([]); expect(overridableRes.warnings.some((w) => w.includes('overridable'))).toBe(true); - const finalSrc = ` -package net.test; -public class Locked { - public final void tick() {} -} -`; + // Final method -> not overridable. + const lockedFinal = bcClass({ + name: 'net/test/LockedMethod', + methods: [bcMethod('tick', '()V', ['public', 'final'])], + }); const finalEntry = makeEntry({ memberType: 'method', - className: 'net.test.Locked', + className: 'net.test.LockedMethod', memberName: 'tick', memberDescriptor: '()V', }); - const finalRes = validateEntryAgainstSource(finalEntry, finalSrc); + const finalRes = validateEntryAgainstBytecode(finalEntry, mapOf(lockedFinal)); expect(finalRes.errors).toEqual([]); expect(finalRes.warnings.some((w) => w.includes('overridable'))).toBe(false); + + // Method in a final class -> not overridable. + const finalClass = bcClass({ + name: 'net/test/FinalClass', + isFinal: true, + methods: [bcMethod('tick', '()V', ['public'])], + }); + const inFinalClass = makeEntry({ + memberType: 'method', + className: 'net.test.FinalClass', + memberName: 'tick', + memberDescriptor: '()V', + }); + expect( + validateEntryAgainstBytecode(inFinalClass, mapOf(finalClass)).warnings.some((w) => + w.includes('overridable'), + ), + ).toBe(false); }); it('suggests a close method name when the targeted method is missing', () => { - // Distinct from the "only called, not declared" case: here a real, similarly - // named method exists, so the miss produces a "Did you mean" suggestion drawn - // from the class's declared method names. - const src = ` -package net.test; -public class Ticker { - public void onTick() {} -} -`; + const cls = bcClass({ + name: 'net/test/Ticker', + methods: [bcMethod('onTick', '()V')], + }); const entry = makeEntry({ memberType: 'method', className: 'net.test.Ticker', memberName: 'onTikc', memberDescriptor: '()V', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors.some((e) => e.includes('not found'))).toBe(true); expect(res.suggestion).toContain('onTick'); }); it('warns (does not error) on a method wildcard *() against an existing class', () => { - // The class must exist for the wildcard branch to run — the class-existence - // guard returns early otherwise. The wildcard itself is never resolved to - // members; it just emits the "targets all members" discouragement warning. - const src = ` -package net.test; -public class Widget { - public void go() {} -} -`; + const cls = bcClass({ name: 'net/test/Widget', methods: [bcMethod('go', '()V')] }); const entry = makeEntry({ memberType: 'method', className: 'net.test.Widget', @@ -441,65 +483,142 @@ public class Widget { memberDescriptor: '()', wildcard: true, }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors).toEqual([]); expect(res.warnings.some((w) => w.includes('targets all members'))).toBe(true); }); it('warns (does not error) on a field wildcard * against an existing class', () => { - const src = ` -package net.test; -public class Widget { - public int count; -} -`; + const cls = bcClass({ name: 'net/test/Widget', fields: [bcField('count')] }); const entry = makeEntry({ memberType: 'field', className: 'net.test.Widget', memberName: '*', wildcard: true, }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors).toEqual([]); expect(res.warnings.some((w) => w.includes('targets all members'))).toBe(true); }); }); +describe('Access Transformer regression: issue #12 false positives', () => { + // These are the exact cases the reporter hit: bytecode HAS the implicit record + // members that decompiled source omits, and a constructor is never overridable. + + it('finds an implicit record component accessor (no false "not found")', () => { + // ExactMatcher(String value): the `value()` accessor is compiler-generated + // and absent from decompiled source, but present in bytecode. + const rec = bcClass({ + name: 'net/mc/StatePropertiesPredicate$ExactMatcher', + isRecord: true, + recordComponents: [{ name: 'value', descriptor: 'Ljava/lang/String;', signature: null }], + canonicalConstructor: '(Ljava/lang/String;)V', + methods: [ + bcMethod('', '(Ljava/lang/String;)V', []), + bcMethod('value', '()Ljava/lang/String;'), + ], + }); + const entry = makeEntry({ + memberType: 'method', + className: 'net.mc.StatePropertiesPredicate$ExactMatcher', + memberName: 'value', + memberDescriptor: '()Ljava/lang/String;', + }); + const res = validateEntryAgainstBytecode(entry, mapOf(rec)); + expect(res.errors).toEqual([]); + }); + + it('finds an implicit record canonical constructor (no false "not found")', () => { + const rec = bcClass({ + name: 'net/mc/PositionPredicate', + isRecord: true, + recordComponents: [{ name: 'x', descriptor: 'D', signature: null }], + canonicalConstructor: '(D)V', + methods: [bcMethod('', '(D)V', [])], + }); + const entry = makeEntry({ + memberType: 'method', + className: 'net.mc.PositionPredicate', + memberName: '', + memberDescriptor: '(D)V', + }); + const res = validateEntryAgainstBytecode(entry, mapOf(rec)); + expect(res.errors).toEqual([]); + }); + + it('does NOT warn that a widened constructor is "overridable" (constructors never are)', () => { + // AnyOfCondition (List): a package-private ctor widened to public. The + // old source-based check wrongly flagged it as overridable. + const cls = bcClass({ + name: 'net/mc/AnyOfCondition', + methods: [bcMethod('', '(Ljava/util/List;)V', [])], + }); + const entry = makeEntry({ + memberType: 'method', + className: 'net.mc.AnyOfCondition', + memberName: '', + memberDescriptor: '(Ljava/util/List;)V', + }); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); + expect(res.errors).toEqual([]); + expect(res.warnings.some((w) => w.includes('overridable'))).toBe(false); + }); + + it('the record-widening note is informational, not a "will crash" claim', () => { + const rec = bcClass({ + name: 'net/mc/Rec', + isRecord: true, + recordComponents: [{ name: 'value', descriptor: 'Ljava/lang/String;', signature: null }], + canonicalConstructor: '(Ljava/lang/String;)V', + // Canonical ctor is package-private (not widened). + methods: [bcMethod('', '(Ljava/lang/String;)V', [])], + }); + const classEntry = makeEntry({ memberType: 'class', className: 'net.mc.Rec' }); + const res = validateEntryAgainstBytecode(classEntry, mapOf(rec), [classEntry]); + const warn = res.warnings.find((w) => w.includes('canonical constructor')); + expect(warn).toBeDefined(); + expect(warn).not.toMatch(/crash/i); + expect(warn).toMatch(/INSTANTIATE/); + }); +}); + describe('Access Transformer cross-entry quirks', () => { - // These checks need the full entry list (allEntries); validateEntryAgainstSource - // omits it and skips them. Drive them through validateEntryAgainstSymbols with - // extractJavaSymbols(src) and the full directive set. + // These checks need the full entry list (allEntries) to see sibling directives. it('warns when a widened record has no matching canonical directive', () => { - const src = ` -package net.test; -public record Point(int x, int y) {} -`; - const classEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, - memberType: 'class', - className: 'net.test.Point', - line: 1, + const rec = bcClass({ + name: 'net/test/Point', + isRecord: true, + recordComponents: [ + { name: 'x', descriptor: 'I', signature: null }, + { name: 'y', descriptor: 'I', signature: null }, + ], + canonicalConstructor: '(II)V', + methods: [bcMethod('', '(II)V', [])], }); - const res = validateEntryAgainstSymbols(classEntry, extractJavaSymbols(src), [classEntry]); + const classEntry = makeEntry({ memberType: 'class', className: 'net.test.Point', line: 1 }); + const res = validateEntryAgainstBytecode(classEntry, mapOf(rec), [classEntry]); const warn = res.warnings.find((w) => w.includes('canonical constructor')); expect(warn).toBeDefined(); - expect(warn).toContain('crash'); // The reconstructed canonical signature is rendered in the message. expect(warn).toContain('void (int, int)'); + // ... but it is NOT presented as a guaranteed crash (issue #12 correction). + expect(warn).not.toMatch(/crash/i); }); it('does not warn when a widened record has a matching at equal-or-wider access', () => { - const src = ` -package net.test; -public record Point(int x, int y) {} -`; - const classEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, - memberType: 'class', - className: 'net.test.Point', - line: 1, + const rec = bcClass({ + name: 'net/test/Point', + isRecord: true, + recordComponents: [ + { name: 'x', descriptor: 'I', signature: null }, + { name: 'y', descriptor: 'I', signature: null }, + ], + canonicalConstructor: '(II)V', + methods: [bcMethod('', '(II)V', [])], }); + const classEntry = makeEntry({ memberType: 'class', className: 'net.test.Point', line: 1 }); const initEntry = makeEntry({ modifier: { access: 'public', final: 'none' }, memberType: 'method', @@ -508,24 +627,22 @@ public record Point(int x, int y) {} memberDescriptor: '(II)V', line: 2, }); - const res = validateEntryAgainstSymbols(classEntry, extractJavaSymbols(src), [ - classEntry, - initEntry, - ]); + const res = validateEntryAgainstBytecode(classEntry, mapOf(rec), [classEntry, initEntry]); expect(res.warnings.some((w) => w.includes('canonical constructor'))).toBe(false); }); it('still warns when the record directive is at narrower access than the class', () => { - const src = ` -package net.test; -public record Point(int x, int y) {} -`; - const classEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, - memberType: 'class', - className: 'net.test.Point', - line: 1, + const rec = bcClass({ + name: 'net/test/Point', + isRecord: true, + recordComponents: [ + { name: 'x', descriptor: 'I', signature: null }, + { name: 'y', descriptor: 'I', signature: null }, + ], + canonicalConstructor: '(II)V', + methods: [bcMethod('', '(II)V', [])], }); + const classEntry = makeEntry({ memberType: 'class', className: 'net.test.Point', line: 1 }); // private (level 0) < public (level 3): not equal-or-wider -> warning fires. const initEntry = makeEntry({ modifier: { access: 'private', final: 'none' }, @@ -535,83 +652,74 @@ public record Point(int x, int y) {} memberDescriptor: '(II)V', line: 2, }); - const res = validateEntryAgainstSymbols(classEntry, extractJavaSymbols(src), [ - classEntry, - initEntry, - ]); + const res = validateEntryAgainstBytecode(classEntry, mapOf(rec), [classEntry, initEntry]); expect(res.warnings.some((w) => w.includes('canonical constructor'))).toBe(true); }); - it('does not emit a record warning for a widened non-record class', () => { - const src = ` -package net.test; -public class Plain {} -`; - const classEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, - memberType: 'class', - className: 'net.test.Plain', - line: 1, + it('does not warn when the record canonical constructor is already public in bytecode', () => { + // If the record's ctor is already public, widening the class needs no extra + // ctor directive — so no note at all. + const rec = bcClass({ + name: 'net/test/Open', + isRecord: true, + recordComponents: [{ name: 'x', descriptor: 'I', signature: null }], + canonicalConstructor: '(I)V', + methods: [bcMethod('', '(I)V', ['public'])], }); - const res = validateEntryAgainstSymbols(classEntry, extractJavaSymbols(src), [classEntry]); + const classEntry = makeEntry({ memberType: 'class', className: 'net.test.Open', line: 1 }); + const res = validateEntryAgainstBytecode(classEntry, mapOf(rec), [classEntry]); + expect(res.warnings.some((w) => w.includes('canonical constructor'))).toBe(false); + }); + + it('does not emit a record warning for a widened non-record class', () => { + const plain = bcClass({ name: 'net/test/Plain' }); + const classEntry = makeEntry({ memberType: 'class', className: 'net.test.Plain', line: 1 }); + const res = validateEntryAgainstBytecode(classEntry, mapOf(plain), [classEntry]); expect(res.errors).toEqual([]); expect(res.warnings.some((w) => w.includes('canonical constructor'))).toBe(false); }); it('warns when an inner-class target has an inaccessible, un-widened enclosing class', () => { - const src = ` -package net.test; -class Outer { - class Inner {} -} -`; + const outer = bcClass({ name: 'net/test/Outer', flags: [] }); // package-private + const inner = bcClass({ + name: 'net/test/Outer$Inner', + innerClasses: [bcInner('net/test/Outer$Inner', [])], + }); const innerEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, memberType: 'class', className: 'net.test.Outer$Inner', line: 1, }); - const res = validateEntryAgainstSymbols(innerEntry, extractJavaSymbols(src), [innerEntry]); + const res = validateEntryAgainstBytecode(innerEntry, mapOf(outer, inner), [innerEntry]); expect(res.warnings.some((w) => w.includes('enclosing class'))).toBe(true); }); - it('does not warn when the inner-class enclosing class is already public in source', () => { - const src = ` -package net.test; -public class Outer { - public class Inner {} -} -`; + it('does not warn when the inner-class enclosing class is already public', () => { + const outer = bcClass({ name: 'net/test/Outer', flags: ['public'] }); + const inner = bcClass({ name: 'net/test/Outer$Inner' }); const innerEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, memberType: 'class', className: 'net.test.Outer$Inner', line: 1, }); - const res = validateEntryAgainstSymbols(innerEntry, extractJavaSymbols(src), [innerEntry]); + const res = validateEntryAgainstBytecode(innerEntry, mapOf(outer, inner), [innerEntry]); expect(res.warnings.some((w) => w.includes('enclosing class'))).toBe(false); }); it('does not warn when the enclosing class is widened to public via a file directive', () => { - const src = ` -package net.test; -class Outer { - class Inner {} -} -`; + const outer = bcClass({ name: 'net/test/Outer', flags: [] }); + const inner = bcClass({ name: 'net/test/Outer$Inner' }); const innerEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, memberType: 'class', className: 'net.test.Outer$Inner', line: 1, }); const outerEntry = makeEntry({ - modifier: { access: 'public', final: 'none' }, memberType: 'class', className: 'net.test.Outer', line: 2, }); - const res = validateEntryAgainstSymbols(innerEntry, extractJavaSymbols(src), [ + const res = validateEntryAgainstBytecode(innerEntry, mapOf(outer, inner), [ innerEntry, outerEntry, ]); @@ -619,29 +727,207 @@ class Outer { }); it('errors on a non-existent inner class but accepts a real one', () => { - // Named nested classes are emitted by the AST walk (dot-qualified, matched - // to the `$` form by classNamesMatch), so a bogus inner target is caught by - // the class-existence check while the real sibling validates cleanly. - const src = ` -package net.test; -public class Outer { - public static class Inner {} -} -`; - const bogus = makeEntry({ - memberType: 'class', - className: 'net.test.Outer$NonExistent', - line: 1, - }); - const bogusRes = validateEntryAgainstSource(bogus, src); - expect(bogusRes.errors.some((e) => e.includes('not found'))).toBe(true); + const outer = bcClass({ name: 'net/test/Outer', flags: ['public'] }); + const inner = bcClass({ name: 'net/test/Outer$Inner' }); + const map = mapOf(outer, inner); - const real = makeEntry({ - memberType: 'class', - className: 'net.test.Outer$Inner', - line: 1, - }); - expect(validateEntryAgainstSource(real, src).errors).toEqual([]); + const bogus = makeEntry({ memberType: 'class', className: 'net.test.Outer$NonExistent' }); + expect( + validateEntryAgainstBytecode(bogus, map).errors.some((e) => e.includes('not found')), + ).toBe(true); + + const real = makeEntry({ memberType: 'class', className: 'net.test.Outer$Inner' }); + expect(validateEntryAgainstBytecode(real, map).errors).toEqual([]); + }); +}); + +// --- End-to-end: the reporter's real AT against a compiled fixture JAR ------- +// +// Reproduces the exact issue-#12 scenario. `summoningrituals-mc-stubs.jar` is a +// committed fixture whose classes recreate the vanilla 1.21.1 bytecode shapes +// the Summoning Rituals AT targets (records with implicit accessors + canonical +// constructors, a package-private constructor, a protected static method). We +// dump it with the real ASM dumper and validate the mod's verbatim +// `accesstransformer.cfg` — proving ZERO false-positive errors. Skips when the +// bundled dumper jar isn't built (dev/CI prerequisite, like bytecode-dumper.test). +const describeE2E = nodeExistsSync(DUMPER_JAR) ? describe : describe.skip; + +describeE2E('Access Transformer end-to-end (Summoning Rituals fixture, issue #12)', () => { + async function loadFixtureMap(): Promise { + const dump = await getBytecodeDumper().dump(FIXTURE_JAR); + return new Map(dump.classes.map((c) => [c.name, c])); + } + + it('validates the real Summoning Rituals AT with zero errors', async () => { + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformerFile(FIXTURE_AT); + expect(at.parseErrors).toEqual([]); + expect(at.entries).toHaveLength(19); + + const map = await loadFixtureMap(); + + const errors: string[] = []; + const warnings: string[] = []; + for (const entry of at.entries) { + const r = validateEntryAgainstBytecode(entry, map, at.entries); + errors.push(...r.errors); + warnings.push(...r.warnings); + } + + // The mod compiles and runs — there must be NO validation errors. Every + // former error (record accessors / canonical ctors "not found", the + // constructor "overridable" warning) is gone. + expect(errors).toEqual([]); + expect(warnings.some((w) => w.includes('overridable'))).toBe(false); + }, 60000); + + it('only surfaces informational record-widening notes for un-widened record ctors', async () => { + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformerFile(FIXTURE_AT); + const map = await loadFixtureMap(); + + const recordNotes: string[] = []; + for (const entry of at.entries) { + const r = validateEntryAgainstBytecode(entry, map, at.entries); + recordNotes.push(...r.warnings.filter((w) => w.includes('canonical constructor'))); + } + + // Exactly the three records whose canonical ctor the AT does NOT widen: + // ExactMatcher, PropertyMatcher, RangedMatcher. PositionPredicate is exempt + // because line 2 of the AT widens its constructor. + expect(recordNotes).toHaveLength(3); + expect(recordNotes.every((w) => /INSTANTIATE/.test(w))).toBe(true); + expect(recordNotes.some((w) => /crash/i.test(w))).toBe(false); + expect(recordNotes.some((w) => w.includes('ExactMatcher'))).toBe(true); + expect(recordNotes.some((w) => w.includes('PropertyMatcher'))).toBe(true); + expect(recordNotes.some((w) => w.includes('RangedMatcher'))).toBe(true); + expect(recordNotes.some((w) => w.includes('PositionPredicate'))).toBe(false); + }, 60000); +}); + +// --- Drift guard: the stub must keep reproducing real 1.21.1 shapes ---------- +// +// The committed stub can only diverge from vanilla if someone edits its sources +// (1.21.1 itself is a frozen release). This pins the stub's bytecode to the +// exact facts the validator relies on — every number below is the real 1.21.1 +// mojmap value (verified against `javap` on the remapped JAR, see issue #12's +// log). Any edit that drops an accessor, flips a record to a class, or changes a +// targeted descriptor fails here. The manual real-MC test additionally asserts +// stub == freshly-downloaded vanilla (belt and suspenders). +describeE2E('Access Transformer fixture fidelity (issue #12 drift guard)', () => { + const vis = (flags: string[]): string => + flags.includes('public') + ? 'public' + : flags.includes('protected') + ? 'protected' + : flags.includes('private') + ? 'private' + : 'package'; + + let byName: Map; + + beforeAll(async () => { + const dump = await getBytecodeDumper().dump(FIXTURE_JAR); + byName = new Map(dump.classes.map((c) => [c.name, c])); + }, 60000); + + const P = 'net/minecraft/advancements/critereon'; + const L = 'net/minecraft/world/level/storage/loot'; + + // Each record: isRecord + exact canonical-ctor descriptor + the implicit + // accessors the AT targets (name -> descriptor). These accessors are the exact + // members that decompiled source omits and that the pre-fix validator wrongly + // reported "not found". + const records: Array<{ + name: string; + canon: string; + accessors: Record; + }> = [ + { + name: `${P}/LocationPredicate$PositionPredicate`, + canon: `(L${P}/MinMaxBounds$Doubles;L${P}/MinMaxBounds$Doubles;L${P}/MinMaxBounds$Doubles;)V`, + accessors: { + x: `()L${P}/MinMaxBounds$Doubles;`, + y: `()L${P}/MinMaxBounds$Doubles;`, + z: `()L${P}/MinMaxBounds$Doubles;`, + }, + }, + { + name: `${P}/StatePropertiesPredicate$ExactMatcher`, + canon: '(Ljava/lang/String;)V', + accessors: { value: '()Ljava/lang/String;' }, + }, + { + name: `${P}/StatePropertiesPredicate$PropertyMatcher`, + canon: `(Ljava/lang/String;L${P}/StatePropertiesPredicate$ValueMatcher;)V`, + accessors: { + name: '()Ljava/lang/String;', + valueMatcher: `()L${P}/StatePropertiesPredicate$ValueMatcher;`, + }, + }, + { + name: `${P}/StatePropertiesPredicate$RangedMatcher`, + canon: '(Ljava/util/Optional;Ljava/util/Optional;)V', + accessors: { minValue: '()Ljava/util/Optional;', maxValue: '()Ljava/util/Optional;' }, + }, + ]; + + it.each(records)('record $name reproduces its 1.21.1 shape', ({ name, canon, accessors }) => { + const cls = byName.get(name); + expect(cls, `${name} missing from fixture`).toBeDefined(); + if (!cls) return; + // The record + its compiler-generated canonical constructor. + expect(cls.isRecord).toBe(true); + expect(cls.canonicalConstructor).toBe(canon); + const ctor = cls.methods.find((m) => m.name === '' && m.desc === canon); + expect(ctor, `${name} canonical ${canon} missing`).toBeDefined(); + // Real 1.21.1: these canonical ctors are non-public (the record-widening note + // hinges on this). javac emits package-private where Mojang emits private — + // both non-public, so the note behaves identically. + expect(vis(ctor?.flags ?? [])).not.toBe('public'); + // The implicit component accessors — present in bytecode, absent from source. + for (const [accName, accDesc] of Object.entries(accessors)) { + const acc = cls.methods.find((m) => m.name === accName && m.desc === accDesc); + expect(acc, `${name}#${accName}${accDesc} missing`).toBeDefined(); + } + }); + + it('IntRange is a plain class with the private (NumberProvider, NumberProvider) ctor + min/max', () => { + const cls = byName.get(`${L}/IntRange`); + expect(cls?.isRecord).toBe(false); + const ctorDesc = `(L${L}/providers/number/NumberProvider;L${L}/providers/number/NumberProvider;)V`; + expect(cls?.methods.some((m) => m.name === '' && m.desc === ctorDesc)).toBe(true); + expect(cls?.fields.some((f) => f.name === 'min')).toBe(true); + expect(cls?.fields.some((f) => f.name === 'max')).toBe(true); + }); + + it('AnyOfCondition (List) is a non-public CONSTRUCTOR (never "overridable")', () => { + const cls = byName.get(`${L}/predicates/AnyOfCondition`); + expect(cls?.isRecord).toBe(false); + const ctor = cls?.methods.find((m) => m.name === '' && m.desc === '(Ljava/util/List;)V'); + expect(ctor, 'AnyOfCondition (List)V missing').toBeDefined(); + expect(vis(ctor?.flags ?? [])).not.toBe('public'); + }); + + it('CompositeLootItemCondition.createInlineCodec is protected+static; terms field present', () => { + const cls = byName.get(`${L}/predicates/CompositeLootItemCondition`); + const m = cls?.methods.find( + (mm) => + mm.name === 'createInlineCodec' && + mm.desc === '(Ljava/util/function/Function;)Lcom/mojang/serialization/Codec;', + ); + expect(m, 'createInlineCodec missing').toBeDefined(); + expect(m?.flags).toContain('static'); + expect(vis(m?.flags ?? [])).toBe('protected'); + expect(cls?.fields.some((f) => f.name === 'terms')).toBe(true); + }); + + it('ClientTextTooltip.text and WitherBoss.yRotHeads/yRotOHeads fields exist', () => { + const tip = byName.get('net/minecraft/client/gui/screens/inventory/tooltip/ClientTextTooltip'); + expect(tip?.fields.some((f) => f.name === 'text')).toBe(true); + const wither = byName.get('net/minecraft/world/entity/boss/wither/WitherBoss'); + expect(wither?.fields.some((f) => f.name === 'yRotHeads')).toBe(true); + expect(wither?.fields.some((f) => f.name === 'yRotOHeads')).toBe(true); }); }); @@ -668,7 +954,7 @@ describe('Conflict detection', () => { // `detectAccessTransformerConflicts` is the pure conflict-detection helper // extracted from the service so it is unit-testable without a decompiled // Minecraft source tree (the full `validateAccessTransformer` flow only - // reaches conflict detection after the decompiled-source check passes). + // reaches conflict detection after the source-availability check passes). const method = (over: Partial): AccessTransformerEntry => makeEntry({ modifier: { access: 'public', final: 'none' }, @@ -753,8 +1039,9 @@ describe('Conflict detection', () => { }); it('validateAccessTransformer short-circuits on a non-decompiled version', async () => { - // Confirms the guard path: without a decompiled tree, the validator reports - // the missing-source error and never reaches conflict detection. + // Confirms the guard path: without a remapped JAR (produced by decompiling), + // the validator reports the missing-source error and never reaches conflict + // detection. const svc = getAccessTransformerService(); const at: AccessTransformer = { entries: [ @@ -765,7 +1052,7 @@ describe('Conflict detection', () => { }; const res = await svc.validateAccessTransformer(at, '0.0.0-conflict-test', 'mojmap'); expect(res.isValid).toBe(false); - expect(res.errors.some((e) => e.message.includes('not decompiled'))).toBe(true); + expect(res.errors.some((e) => e.message.includes('not available'))).toBe(true); }); }); diff --git a/__tests__/services/bytecode-index-service.test.ts b/__tests__/services/bytecode-index-service.test.ts new file mode 100644 index 0000000..2a9010a --- /dev/null +++ b/__tests__/services/bytecode-index-service.test.ts @@ -0,0 +1,147 @@ +import { copyFileSync, existsSync, rmSync, statSync, utimesSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { BytecodeIndexService } from '../../src/services/bytecode-index-service.js'; +import { ensureDir } from '../../src/utils/file-utils.js'; +import { getRemappedJarPath } from '../../src/utils/paths.js'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const FIXTURE_JAR = join(__dirname, '..', 'fixtures', 'summoningrituals-mc-stubs.jar'); +const DUMPER_JAR = join( + __dirname, + '..', + '..', + 'tools', + 'bytecode-dumper', + 'build', + 'libs', + 'bytecode-dumper-1.0.0.jar', +); + +// Runs against the committed fixture JAR via the bundled ASM dumper. Skips when +// the dumper jar isn't built (dev/CI prerequisite, like bytecode-dumper.test). +const describeIndex = existsSync(DUMPER_JAR) && existsSync(FIXTURE_JAR) ? describe : describe.skip; + +describeIndex('BytecodeIndexService (remapped-JAR bytecode cache)', () => { + // Use a throwaway version key so we never touch a real cached version. The + // service resolves the JAR path from (version, mapping); we stage the fixture + // JAR there and clean up both the JAR and its sidecar cache afterwards. + const VERSION = '0.0.0-bytecode-index-test'; + const MAPPING = 'mojmap' as const; + + function stageJar(): { jarPath: string; cachePath: string } { + const jarPath = getRemappedJarPath(VERSION, MAPPING); + ensureDir(dirname(jarPath)); + copyFileSync(FIXTURE_JAR, jarPath); + return { jarPath, cachePath: jarPath.replace(/\.jar$/i, '.bytecode.json') }; + } + + function cleanup(jarPath: string, cachePath: string): void { + rmSync(jarPath, { force: true }); + rmSync(cachePath, { force: true }); + for (const stray of [`${cachePath}.tmp`]) rmSync(stray, { force: true }); + } + + it('resolves requested classes, omits absent ones, and writes a sidecar cache', async () => { + const { jarPath, cachePath } = stageJar(); + try { + const svc = new BytecodeIndexService(); + const map = await svc.getClassBytecode(VERSION, MAPPING, [ + 'net/minecraft/world/level/storage/loot/IntRange', + 'net/minecraft/advancements/critereon/StatePropertiesPredicate$ExactMatcher', + 'net/minecraft/does/not/Exist', + ]); + + // Present classes are returned with authoritative bytecode. + const intRange = map.get('net/minecraft/world/level/storage/loot/IntRange'); + expect(intRange).toBeDefined(); + expect(intRange?.methods.some((m) => m.name === '')).toBe(true); + + // The record's implicit accessor + canonical ctor are present in bytecode. + const exact = map.get( + 'net/minecraft/advancements/critereon/StatePropertiesPredicate$ExactMatcher', + ); + expect(exact?.isRecord).toBe(true); + expect(exact?.canonicalConstructor).toBe('(Ljava/lang/String;)V'); + expect(exact?.methods.some((m) => m.name === 'value')).toBe(true); + + // Absent classes are simply omitted from the map. + expect(map.has('net/minecraft/does/not/Exist')).toBe(false); + + // The sidecar cache was written next to the JAR. + expect(existsSync(cachePath)).toBe(true); + } finally { + cleanup(jarPath, cachePath); + } + }, 60000); + + it('serves a second request from cache without re-reading the JAR', async () => { + const { jarPath, cachePath } = stageJar(); + try { + const svc = new BytecodeIndexService(); + await svc.getClassBytecode(VERSION, MAPPING, [ + 'net/minecraft/world/level/storage/loot/IntRange', + ]); + const cacheMtime = statSync(cachePath).mtimeMs; + + // A repeat request for the same (already-cached) class must not rewrite the + // cache file, since nothing was missing. + const map = await svc.getClassBytecode(VERSION, MAPPING, [ + 'net/minecraft/world/level/storage/loot/IntRange', + ]); + expect(map.has('net/minecraft/world/level/storage/loot/IntRange')).toBe(true); + expect(statSync(cachePath).mtimeMs).toBe(cacheMtime); + } finally { + cleanup(jarPath, cachePath); + } + }, 60000); + + it('invalidates the cache when the JAR signature changes', async () => { + const { jarPath, cachePath } = stageJar(); + try { + const svc = new BytecodeIndexService(); + await svc.getClassBytecode(VERSION, MAPPING, [ + 'net/minecraft/world/level/storage/loot/IntRange', + ]); + expect(existsSync(cachePath)).toBe(true); + + // Simulate a rebuilt JAR (new mtime) — the stored size:mtime signature no + // longer matches, so the stale cache is discarded and rebuilt cleanly. + const future = new Date(Date.now() + 60_000); + utimesSync(jarPath, future, future); + + const map = await svc.getClassBytecode(VERSION, MAPPING, [ + 'net/minecraft/advancements/critereon/StatePropertiesPredicate$RangedMatcher', + ]); + expect( + map.has('net/minecraft/advancements/critereon/StatePropertiesPredicate$RangedMatcher'), + ).toBe(true); + } finally { + cleanup(jarPath, cachePath); + } + }, 60000); + + it('clearCache removes the sidecar', async () => { + const { jarPath, cachePath } = stageJar(); + try { + const svc = new BytecodeIndexService(); + await svc.getClassBytecode(VERSION, MAPPING, [ + 'net/minecraft/world/level/storage/loot/IntRange', + ]); + expect(existsSync(cachePath)).toBe(true); + + svc.clearCache(VERSION, MAPPING); + expect(existsSync(cachePath)).toBe(false); + } finally { + cleanup(jarPath, cachePath); + } + }, 60000); + + it('throws when the remapped JAR does not exist', async () => { + const svc = new BytecodeIndexService(); + await expect( + svc.getClassBytecode('0.0.0-definitely-missing', MAPPING, ['net/minecraft/Foo']), + ).rejects.toThrow(/Remapped JAR not found/); + }); +}); diff --git a/docs/specs/access-transformer-support.md b/docs/specs/access-transformer-support.md index 4585044..9c085d1 100644 --- a/docs/specs/access-transformer-support.md +++ b/docs/specs/access-transformer-support.md @@ -2,7 +2,40 @@ **Tracking:** GitHub issue [#12 — Support for access transformers](https://github.com/MCDxAI/minecraft-dev-mcp/issues/12) **Author:** rlnt (Relentless) · **Triage:** labeled `enhancement`, assigned GhostTypes · **Opened:** 2026-06-29 -**Status:** Research complete — pending implementation. +**Status:** Implemented (v1.2.2). Revised to bytecode-based validation — see the update note below. + +--- + +## 0. Update — bytecode ground truth (issue #12 follow-up) + +The reporter tested v1.2.4 and hit **false positives**: implicit record members — +canonical constructors and component accessors (`value()`, `name()`, …) — were +reported "not found", because they are compiler-generated and **do not appear in +decompiled `.java`**. They forced a fallback to `javap` on the remapped JAR. + +The validator therefore now correlates entries against **bytecode**, not +decompiled source. It reads the remapped JAR through the bundled ASM +`bytecode-dumper` (`src/java/bytecode-dumper.ts`, `src/services/bytecode-index-service.ts`), +so it sees exactly what the AT is applied to at load time — the same facts +`javap` shows — with true access flags and erased descriptors. Consequences: + +- **Member existence / signature** is an exact bytecode lookup (implicit record + members included). No more source-omission false positives. +- **Record canonical-constructor widening (§6.2)** is downgraded from a + guaranteed "the game will crash at runtime" **error-grade** claim to an + **informational note**: widening the record class alone is fine for reading + components/codecs; a widened canonical ctor is only needed if you *instantiate* + the record. It fires only when bytecode shows the ctor is actually narrower + than the class's new access and no file directive widens it. +- **Constructors are never "overridable"** — the override-narrowing warning (§6.3) + now excludes ``/`` (a prior false positive on `AnyOfCondition`). +- Needs only the **remapped JAR** (produced by `decompile_minecraft_version`); + results are cached in a `remapped/{version}-{mapping}.bytecode.json` sidecar + keyed by the JAR's size+mtime signature, so the cache is always fresh and + never re-dumps the whole JAR. + +Sections below marked as tree-sitter/decompiled-source describe the original v1 +design and are superseded by this note. --- diff --git a/src/services/access-transformer-service.ts b/src/services/access-transformer-service.ts index c450c89..f7965bf 100644 --- a/src/services/access-transformer-service.ts +++ b/src/services/access-transformer-service.ts @@ -12,20 +12,23 @@ * `class`/`method`/`field` keyword), a method descriptor attached to the name * with no spaces, and first-class final control. * - * Validation is tree-sitter based: decompiled Java source is parsed once into an - * AST (via `extractJavaSymbols`) and entries are correlated against the declared - * symbols. There is NO regex Java-source inspection — the only string-pattern - * operations are AT-file-format parsing (`parseEntry`), class-name/path - * conversion, and JVM descriptor decode (shared via `descriptor-utils`). This is - * required by the spec (section 9). - * * Default mapping is `'mojmap'` (NOT `'yarn'` like the AW tool): Forge/NeoForge * dev toolchains are mojmap-only post-1.17 (spec 6.3). + * + * GROUND TRUTH IS BYTECODE, NOT DECOMPILED SOURCE. Validation runs against the + * remapped Minecraft JAR via the ASM bytecode-dumper (see + * `bytecode-index-service.ts`), NOT VineFlower `.java`. Decompiled source omits + * compiler-generated members — a record's canonical constructor and its + * component accessors (`value()`, `name()`, …) — so a source-based check reports + * them as "not found" even though they exist in the class file the AT is + * actually applied to. Bytecode has every member with its true access flags and + * erased descriptors — the same facts `javap` shows — which eliminates that + * whole class of false positives (issue #12). */ import { existsSync, readFileSync } from 'node:fs'; -import { join } from 'node:path'; import { getCacheManager } from '../cache/cache-manager.js'; +import type { BytecodeClass, BytecodeMethod } from '../java/bytecode-dumper.js'; import type { AccessTransformerParseError as ATParseError, AccessTransformer, @@ -35,33 +38,25 @@ import type { AccessTransformerValidation, MappingType, } from '../types/minecraft.js'; -import { - classNamesMatch, - descriptorsCompatible, - javaTypeToDescriptor, - paramToDescriptor, - parseParamDescriptors, - descriptorToReadable as sharedDescriptorToReadable, -} from '../utils/descriptor-utils.js'; +import { descriptorToReadable as sharedDescriptorToReadable } from '../utils/descriptor-utils.js'; import { AccessTransformerParseError } from '../utils/errors.js'; -import { extractJavaSymbols } from '../utils/java-symbols.js'; -import type { JavaSymbol } from '../utils/java-symbols.js'; import { logger } from '../utils/logger.js'; -import { getDecompiledPath } from '../utils/paths.js'; -import { findSimilarClassFile, findSimilarName } from '../utils/suggestions.js'; +import { findSimilarName } from '../utils/suggestions.js'; +import { getBytecodeIndexService } from './bytecode-index-service.js'; + +/** A map of internal class name (slashes, `$`) → its authoritative bytecode metadata. */ +export type ClassBytecodeMap = Map; // --------------------------------------------------------------------------- -// AT-local modifier + descriptor helpers (module-internal) +// AT-local modifier + parsing helpers (module-internal) // --------------------------------------------------------------------------- // -// `parseModifier` / `looksLikeDescriptor` / `buildMethodDescriptor` / -// `methodDescriptorMatches` / `methodDescriptorsMatch` are AT-validation-shaped. -// The generic primitives they build on — `javaTypeToDescriptor`, -// `paramToDescriptor`, `parseParamDescriptors`, `descriptorsCompatible`, -// `classNamesMatch` — live in `../utils/descriptor-utils.js` (shared with the -// AW + mixin validators). `descriptorToReadable` (descriptor decode) and the -// edit-distance suggestion helpers live in `descriptor-utils.js` / -// `suggestions.js` respectively. +// `parseModifier` / `looksLikeDescriptor` handle AT-file-format parsing. +// Member correlation is done directly against bytecode descriptors (erased, +// fully-qualified JVM form), so it is exact string comparison — no AST symbol +// reconstruction or simple-name fuzzy matching is needed. `descriptorToReadable` +// (descriptor decode) and the edit-distance suggestion helper live in +// `descriptor-utils.js` / `suggestions.js` respectively. /** The four raw JVM visibility keywords an AT entry may use. */ const ACCESS_KEYWORDS = new Set(['public', 'protected', 'default', 'private']); @@ -141,205 +136,162 @@ function looksLikeDescriptor(token: string): boolean { return false; } -/** Build the JVM descriptor string for an AST method/constructor symbol. */ -function buildMethodDescriptor(method: JavaSymbol): string { - const params = method.parameters ?? []; - return `(${params.map(paramToDescriptor).join('')})${javaTypeToDescriptor(method.returnType ?? 'void')}`; +/** Convert an AT dotted+`$` class name to an internal JVM name (slashes). */ +function toInternalName(className: string): string { + return className.replace(/\./g, '/'); } -/** Compare an AST method/constructor against a raw JVM method descriptor. */ -function methodDescriptorMatches( - method: JavaSymbol, - descriptor: string, -): { match: boolean; reason?: string } { - const open = descriptor.indexOf('('); - const close = descriptor.indexOf(')'); - if (open < 0 || close < 0 || close <= open) { - return { match: false, reason: `Malformed method descriptor: ${descriptor}` }; - } - - const descParams = parseParamDescriptors(descriptor.slice(open + 1, close)); - const descReturn = descriptor.slice(close + 1); - - const params = method.parameters ?? []; - const astParams = params.map(paramToDescriptor); - const astReturn = javaTypeToDescriptor(method.returnType ?? 'void'); - - if (astParams.length !== descParams.length) { - return { match: false, reason: `arity ${astParams.length} vs ${descParams.length}` }; - } - for (let i = 0; i < astParams.length; i++) { - if (!descriptorsCompatible(astParams[i], descParams[i])) { - return { match: false, reason: `param ${i}: ${astParams[i]} vs ${descParams[i]}` }; - } - } - if (!descriptorsCompatible(astReturn, descReturn)) { - return { match: false, reason: `return: ${astReturn} vs ${descReturn}` }; - } - return { match: true }; +/** Map a decoded JVM access-flag list to the AT visibility keyword. */ +function accessFromFlags(flags: string[]): AccessTransformerAccess { + if (flags.includes('public')) return 'public'; + if (flags.includes('protected')) return 'protected'; + if (flags.includes('private')) return 'private'; + return 'default'; } /** - * Compare two RAW JVM method descriptors for parameter/return compatibility - * (simple-name based, like the AST comparison). Used by the record - * canonical-constructor check (quirk 6.2) to match a reconstructed canonical - * descriptor against an `` directive written in the file. + * The effective visibility of a class. For a top-level class the class-file + * access bits are authoritative; for a nested class the real visibility + * (public/protected/private/package) lives in the enclosing `InnerClasses` + * attribute — a class file records an `InnerClasses` entry for itself, so we + * read the flags of the entry whose `name` equals the class's own name. */ -function methodDescriptorsMatch(a: string, b: string): boolean { - const openA = a.indexOf('('); - const closeA = a.indexOf(')'); - const openB = b.indexOf('('); - const closeB = b.indexOf(')'); - if (openA < 0 || closeA < 0 || openB < 0 || closeB < 0) return a === b; - - const paramsA = parseParamDescriptors(a.slice(openA + 1, closeA)); - const paramsB = parseParamDescriptors(b.slice(openB + 1, closeB)); - const returnA = a.slice(closeA + 1); - const returnB = b.slice(closeB + 1); - - if (paramsA.length !== paramsB.length) return false; - for (let i = 0; i < paramsA.length; i++) { - if (!descriptorsCompatible(paramsA[i], paramsB[i])) return false; +function classVisibility(cls: BytecodeClass): AccessTransformerAccess { + if (cls.name.includes('$')) { + const self = cls.innerClasses.find((ic) => ic.name === cls.name); + if (self) return accessFromFlags(self.flags); } - return descriptorsCompatible(returnA, returnB); + return accessFromFlags(cls.flags); } -// --------------------------------------------------------------------------- -// Pure helpers: member-name correlation + overridability (module-internal) -// --------------------------------------------------------------------------- - -/** Unique regular (non-constructor) method names declared by the given class symbols. */ -function uniqueMethodNames(classSymbols: JavaSymbol[]): string[] { - const set = new Set(); - for (const s of classSymbols) { - if (s.entryType === 'method' && s.isConstructor !== true && s.symbol) set.add(s.symbol); - } - return [...set]; -} - -/** Unique field names declared by the given class symbols. */ -function uniqueFieldNames(classSymbols: JavaSymbol[]): string[] { - const set = new Set(); - for (const s of classSymbols) { - if (s.entryType === 'field' && s.symbol) set.add(s.symbol); - } - return [...set]; +/** Simple (last-segment) name of an internal class name, for suggestions. */ +function simpleClassName(internal: string): string { + const slash = internal.lastIndexOf('/'); + const tail = slash >= 0 ? internal.slice(slash + 1) : internal; + const dollar = tail.lastIndexOf('$'); + return dollar >= 0 ? tail.slice(dollar + 1) : tail; } /** - * Is a method symbol overridable (and thus subject to the override-narrowing - * gotcha, quirk 6.3)? Overridable = not final, not static, not private, and - * declared in a non-final class. + * Is a method overridable (subject to the override-narrowing gotcha, quirk 6.3)? + * Constructors and static initializers are NOT overridable — a constructor is + * never inherited, so an AT on `` can never be defeated by a subclass + * override (the previous source-based check wrongly flagged constructors). + * Overridable = instance method, not final/static/private, in a non-final class. */ -function isOverridable(method: JavaSymbol, classFinal: boolean): boolean { - const mods = method.modifiers ?? []; - if (method.isFinal === true || mods.includes('final')) return false; - if (method.isStatic === true || mods.includes('static')) return false; - if (mods.includes('private')) return false; - if (classFinal) return false; - return true; +function isMethodOverridable(method: BytecodeMethod, cls: BytecodeClass): boolean { + if (method.name === '' || method.name === '') return false; + if ( + method.flags.includes('final') || + method.flags.includes('static') || + method.flags.includes('private') + ) { + return false; + } + return !cls.isFinal; } // --------------------------------------------------------------------------- -// Core validation against extracted symbols +// Core validation against authoritative bytecode // --------------------------------------------------------------------------- /** - * Validate a single access-transformer entry against the AST symbols of its - * class. Pure: no filesystem, no I/O. This is the heart of the validator. + * Validate a single access-transformer entry against the bytecode metadata of + * its class (and, for the cross-entry quirks, the whole file's classes). Pure: + * no filesystem, no I/O. + * + * `classMap` is keyed by internal class name (slashes, `$`) and must already + * contain the entry's class and — for nested targets — its enclosing classes. + * Because bytecode carries every compiler-generated member (record canonical + * constructors, component accessors) with true access flags and erased + * descriptors, member existence is an exact lookup and descriptor matching is + * exact string comparison — no source omissions to work around. * * `allEntries` (the full parsed file) enables the cross-entry quirk checks: * record canonical-constructor widening (6.2) and inner-class enclosing-class - * accessibility (6.1). When omitted (e.g. via the `validateEntryAgainstSource` - * test seam) those file-level checks are skipped and only per-member existence - * and signature checks run. + * accessibility (6.1). When omitted, those file-level checks are skipped. * * @internal */ -export function validateEntryAgainstSymbols( +export function validateEntryAgainstBytecode( entry: AccessTransformerEntry, - symbols: JavaSymbol[], + classMap: ClassBytecodeMap, allEntries?: AccessTransformerEntry[], ): { errors: string[]; warnings: string[]; suggestion?: string } { const errors: string[] = []; const warnings: string[] = []; let suggestion: string | undefined; - // Symbols declared by the entry's class. `classNamesMatch` reconciles JVM - // inner-class `$` separators with the AST's dotted declaringClass. Inner - // classes share their enclosing class's `.java` file, so the enclosing type's - // symbols are present in this same `symbols` array. - const classSymbols = symbols.filter((s) => classNamesMatch(entry.className, s.declaringClass)); - - // The target class must actually be declared. Named nested classes (regular, - // record, enum) are emitted by the AST walk with dot-qualified names, which - // `classNamesMatch` reconciles with the JVM `$` form — so a non-existent inner - // class (e.g. `Outer$NonExistent`) resolves to no class symbol and is caught - // here rather than silently passing. `classSym` is then reused below for the - // record and final-modifier checks. - const classSym = classSymbols.find((s) => s.entryType === 'class'); - if (!classSym) { + const internal = toInternalName(entry.className); + const cls = classMap.get(internal); + + // The target class must exist in the JAR. Nested classes are separate class + // files, so `Outer$NonExistent` simply has no entry and is caught here. + if (!cls) { errors.push(`Class '${entry.className}' not found`); + const similar = findSimilarName( + simpleClassName(internal), + [...classMap.keys()].map(simpleClassName), + ); + if (similar) suggestion = `Did you mean a class named: ${similar}?`; return { errors, warnings, suggestion }; } // --- Inner-class enclosing-class accessibility (quirk 6.1) --- - // Runs for ANY entry whose class is nested (`$` present): reaching a nested - // type through an inaccessible enclosing type is a Java access error. Per the - // issue, check the IMMEDIATE enclosing class only; transitive coverage emerges - // as each nesting level is targeted by its own directive. - if (allEntries && entry.className.includes('$')) { - const enclosing = entry.className.split('$').slice(0, -1).join('$'); - const enclosingSym = symbols.find( - (s) => s.entryType === 'class' && classNamesMatch(enclosing, s.declaringClass), - ); - const enclosingAlreadyAccessible = - !!enclosingSym && - (enclosingSym.modifiers?.includes('public') === true || - enclosingSym.modifiers?.includes('protected') === true); + // Reaching a nested type through an inaccessible enclosing type is a Java + // access error. Check the IMMEDIATE enclosing class only; deeper nesting is + // covered as each level is targeted by its own directive. + if (allEntries && internal.includes('$')) { + const enclosing = internal.slice(0, internal.lastIndexOf('$')); + const enclosingCls = classMap.get(enclosing); + const enclosingAccessible = + !!enclosingCls && + (classVisibility(enclosingCls) === 'public' || classVisibility(enclosingCls) === 'protected'); const widenedInFile = allEntries.some( (e) => e.memberType === 'class' && - classNamesMatch(e.className, enclosing) && + toInternalName(e.className) === enclosing && (e.modifier.access === 'public' || e.modifier.access === 'protected'), ); - if (!enclosingAlreadyAccessible && !widenedInFile) { + if (!enclosingAccessible && !widenedInFile) { warnings.push( - `Inner class '${entry.className}' targets an enclosing class '${enclosing}' that is not public/protected and not widened in this file — Java requires the enclosing class to be accessible`, + `Inner class '${entry.className}' targets an enclosing class '${enclosing.replace(/\//g, '.')}' that is not public/protected and not widened in this file — Java requires the enclosing class to be accessible`, ); } } // --- Class-level entry --- if (entry.memberType === 'class') { - // Record canonical-constructor widening (quirk 6.2). Widening a record to - // public/protected does NOT widen its canonical (component accessors - // do widen), violating record semantics and crashing at runtime. Reconstruct - // the canonical-ctor descriptor from the record components in order and - // require a matching directive at equal-or-wider access. + // Record canonical-constructor widening (quirk 6.2). A widened record whose + // canonical constructor stays narrower cannot be INSTANTIATED at the widened + // access — but reading its components or codec is fine (records don't need a + // widened ctor for that). So this is an informational note, not a crash: we + // only surface it when bytecode shows the canonical ctor is actually narrower + // than the class's new access AND the file doesn't already widen it. if ( allEntries && + cls.isRecord && + cls.canonicalConstructor && (entry.modifier.access === 'public' || entry.modifier.access === 'protected') ) { - if (classSym.kind === 'record') { - const components = classSym.recordComponents ?? []; - const canonicalDesc = `(${components.map(paramToDescriptor).join('')})V`; - const classLevel = ACCESS_LEVEL[entry.modifier.access]; - const hasMatchingInit = allEntries.some( - (e) => - e.memberType === 'method' && - e.memberName === '' && - classNamesMatch(e.className, entry.className) && - ACCESS_LEVEL[e.modifier.access] >= classLevel && - methodDescriptorsMatch(e.memberDescriptor ?? '', canonicalDesc), + const canonical = cls.canonicalConstructor; + const ctor = cls.methods.find((m) => m.name === '' && m.desc === canonical); + const classLevel = ACCESS_LEVEL[entry.modifier.access]; + const ctorLevel = ctor ? ACCESS_LEVEL[accessFromFlags(ctor.flags)] : 0; + const widenedInFile = allEntries.some( + (e) => + e.memberType === 'method' && + e.memberName === '' && + toInternalName(e.className) === internal && + e.memberDescriptor === canonical && + ACCESS_LEVEL[e.modifier.access] >= classLevel, + ); + if (!widenedInFile && ctorLevel < classLevel) { + warnings.push( + `Record '${entry.className}' is widened to ${entry.modifier.access} but its canonical constructor ${sharedDescriptorToReadable( + canonical, + )} is not (currently ${ctor ? accessFromFlags(ctor.flags) : 'package-private'}). This only matters if you INSTANTIATE the record (e.g. 'new', or codec/network deserialization in your code); reading its components or codec needs no ctor widening. If you do construct it, also add: '${entry.modifier.access} ${entry.className} ${canonical}'.`, ); - if (!hasMatchingInit) { - warnings.push( - `Record '${entry.className}' is widened but its canonical constructor ${sharedDescriptorToReadable( - canonicalDesc, - )} is not widened to equal-or-wider access — the game will crash at runtime`, - ); - } } } return { errors, warnings, suggestion }; @@ -347,15 +299,6 @@ export function validateEntryAgainstSymbols( // --- Method entry --- if (entry.memberType === 'method' && entry.memberName) { - // Static initializers are intentionally not emitted by the AST walk. They - // are rare and genuinely unverifiable from decompiled source. - if (entry.memberName === '') { - warnings.push( - "'' (static initializer) cannot be validated from decompiled source — verify manually", - ); - return { errors, warnings, suggestion }; - } - if (entry.wildcard) { warnings.push( "Wildcard '*()' targets all members — discouraged and may be removed from the AT spec", @@ -363,44 +306,40 @@ export function validateEntryAgainstSymbols( return { errors, warnings, suggestion }; } - const isCtor = entry.memberName === ''; - // Only DECLARED members are seen here — call sites and comments are not, so - // a method that is merely called (but not declared) is correctly reported - // as missing. - const methodSyms = classSymbols.filter((s) => - isCtor - ? s.entryType === 'method' && s.isConstructor === true - : s.entryType === 'method' && s.isConstructor !== true && s.symbol === entry.memberName, - ); + const methodSyms = cls.methods.filter((m) => m.name === entry.memberName); if (methodSyms.length === 0) { errors.push(`Method '${entry.memberName}' not found in ${entry.className}`); - const similar = findSimilarName(entry.memberName, uniqueMethodNames(classSymbols)); + // Suggest from real (non-synthetic, non-ctor) method names. + const candidates = [ + ...new Set( + cls.methods + .filter((m) => m.name !== '' && m.name !== '') + .map((m) => m.name), + ), + ]; + const similar = findSimilarName(entry.memberName, candidates); if (similar) suggestion = `Did you mean: ${similar}?`; return { errors, warnings, suggestion }; } - // Validate the JVM descriptor against the actual signature. The issue - // explicitly asks to "check the signature" — stricter than the name-only AW - // field check. + // Descriptor check: AT and bytecode descriptors are both erased, + // fully-qualified JVM form, so this is an exact match. The issue explicitly + // asks to "check the signature". if (entry.memberDescriptor) { - const descriptor = entry.memberDescriptor; - const matched = methodSyms.filter((m) => methodDescriptorMatches(m, descriptor).match); + const matched = methodSyms.filter((m) => m.desc === entry.memberDescriptor); if (matched.length === 0) { - const found = methodSyms.map((m) => buildMethodDescriptor(m)).join(', '); + const found = methodSyms.map((m) => m.desc).join(', '); errors.push( - `Method '${entry.memberName}' exists but no overload matches descriptor ${descriptor} (found: ${found})`, + `Method '${entry.memberName}' exists but no overload matches descriptor ${entry.memberDescriptor} (found: ${found})`, ); return { errors, warnings, suggestion }; } - // Override-narrowing warning (quirk 6.3): an AT only transforms the exact - // targeted method; subclass overrides are untouched, risking JVM - // link/verify errors if an override narrows visibility. Warn when the - // targeted overload is overridable. - const classFinal = - classSym.isFinal === true || classSym.modifiers?.includes('final') === true; - if (matched.some((m) => isOverridable(m, classFinal))) { + // Override-narrowing warning (quirk 6.3): an AT transforms only the exact + // targeted method; subclass overrides are untouched. Constructors are + // excluded — they are never overridable. + if (matched.some((m) => isMethodOverridable(m, cls))) { warnings.push( `Method '${entry.memberName}' is overridable — subclass overrides are not transformed by this AT and may cause JVM link/verify errors`, ); @@ -418,13 +357,14 @@ export function validateEntryAgainstSymbols( return { errors, warnings, suggestion }; } - const fieldSyms = classSymbols.filter( - (s) => s.entryType === 'field' && s.symbol === entry.memberName, - ); + const fieldSyms = cls.fields.filter((f) => f.name === entry.memberName); if (fieldSyms.length === 0) { errors.push(`Field '${entry.memberName}' not found in ${entry.className}`); - const similar = findSimilarName(entry.memberName, uniqueFieldNames(classSymbols)); + const similar = findSimilarName( + entry.memberName, + cls.fields.map((f) => f.name), + ); if (similar) suggestion = `Did you mean: ${similar}?`; return { errors, warnings, suggestion }; } @@ -436,23 +376,6 @@ export function validateEntryAgainstSymbols( return { errors, warnings, suggestion }; } -/** - * Validate a single access-transformer entry against a synthetic Java source - * string. Test seam for the existence/signature logic — it lets unit tests - * exercise validation without a decompiled Minecraft source tree. Production - * validation reads the class file, extracts symbols once (cached), and delegates - * to `validateEntryAgainstSymbols` (passing the full entry list so the - * cross-entry quirk checks also run). - * - * @internal - */ -export function validateEntryAgainstSource( - entry: AccessTransformerEntry, - source: string, -): { errors: string[]; warnings: string[]; suggestion?: string } { - return validateEntryAgainstSymbols(entry, extractJavaSymbols(source)); -} - /** * Detect duplicate and conflicting targets across a whole AT file. Pure: no * filesystem, no I/O. Exported so the conflict logic is unit-testable without @@ -691,14 +614,17 @@ export class AccessTransformerService { } /** - * Validate an access transformer against decompiled Minecraft source. + * Validate an access transformer against the remapped Minecraft JAR's + * bytecode. * * Default mapping is `'mojmap'` (Forge/NeoForge dev toolchains are - * mojmap-only post-1.17). Mirrors the access-widener flow: short-circuit when - * the source tree is absent, cache extracted symbols per class file, and - * delegate the pure per-entry validation to `validateEntryAgainstSymbols`. - * Cross-entry quirk checks (record ctor, inner-class accessibility) and - * duplicate/conflict detection run after the per-entry pass. + * mojmap-only post-1.17). Requires the version to have been decompiled (which + * also produces the remapped JAR this reads). The needed classes — each + * targeted class plus its enclosing classes, for the inner-class check — are + * resolved from bytecode once (cached), then the pure per-entry validation + * runs against `ClassBytecodeMap`. Cross-entry quirk checks (record ctor, + * inner-class accessibility) and duplicate/conflict detection run after the + * per-entry pass. */ async validateAccessTransformer( accessTransformer: AccessTransformer, @@ -718,29 +644,48 @@ export class AccessTransformerService { line: 0, }; - // Check if decompiled source exists. - if (!cacheManager.hasDecompiledSource(mcVersion, mapping)) { + const firstEntry = accessTransformer.entries[0] ?? syntheticEntry; + + // The remapped JAR (bytecode ground truth) is produced during decompilation. + // Require it explicitly; guide the user to decompile if it is absent. + if (!cacheManager.hasRemappedJar(mcVersion, mapping)) { errors.push({ - entry: accessTransformer.entries[0] ?? syntheticEntry, - message: `Minecraft ${mcVersion} source not decompiled. Run decompile_minecraft_version first.`, + entry: firstEntry, + message: `Minecraft ${mcVersion} (${mapping}) is not available locally. Run decompile_minecraft_version first.`, }); return { isValid: false, errors, warnings }; } - const decompiledPath = getDecompiledPath(mcVersion, mapping); + // Resolve the set of internal class names to load: each targeted class plus + // every enclosing prefix (needed for the inner-class accessibility check). + const needed = new Set(); + for (const entry of accessTransformer.entries) { + const internal = entry.className.replace(/\./g, '/'); + needed.add(internal); + let idx = internal.lastIndexOf('$'); + while (idx >= 0) { + const enclosing = internal.slice(0, idx); + needed.add(enclosing); + idx = enclosing.lastIndexOf('$'); + } + } - // Cache extracted symbols per source file for the duration of this call. - // Multiple entries targeting the same class parse the file only once. - const symbolsCache = new Map(); + let classMap: ClassBytecodeMap; + try { + classMap = await getBytecodeIndexService().getClassBytecode(mcVersion, mapping, [...needed]); + } catch (error) { + errors.push({ + entry: firstEntry, + message: `Failed to read bytecode for Minecraft ${mcVersion} (${mapping}): ${ + error instanceof Error ? error.message : 'Unknown error' + }`, + }); + return { isValid: false, errors, warnings }; + } - // Validate each entry. + // Validate each entry against bytecode. for (const entry of accessTransformer.entries) { - const validation = this.validateEntry( - entry, - decompiledPath, - symbolsCache, - accessTransformer.entries, - ); + const validation = validateEntryAgainstBytecode(entry, classMap, accessTransformer.entries); errors.push( ...validation.errors.map((message) => ({ entry, @@ -761,43 +706,6 @@ export class AccessTransformerService { }; } - /** - * Validate a single entry against the decompiled source tree. - * - * Reads the class file once (symbols cached per path within a single - * `validateAccessTransformer` call) and delegates the pure symbol-based - * validation to `validateEntryAgainstSymbols`, passing the full entry list so - * the cross-entry quirk checks run. - */ - private validateEntry( - entry: AccessTransformerEntry, - decompiledPath: string, - symbolsCache: Map, - allEntries: AccessTransformerEntry[], - ): { errors: string[]; warnings: string[]; suggestion?: string } { - // JVM inner classes nest with `$`; the source lives in the outer class's - // .java file, so resolve the file path from the top-level class segment. - const outerClassName = entry.className.split('$')[0]; - const classPath = join(decompiledPath, `${outerClassName.replace(/\./g, '/')}.java`); - - if (!existsSync(classPath)) { - const errors = [`Class not found: ${entry.className}`]; - let suggestion: string | undefined; - const similar = findSimilarClassFile(outerClassName, decompiledPath); - if (similar) suggestion = `Did you mean: ${similar}?`; - return { errors, warnings: [], suggestion }; - } - - let symbols = symbolsCache.get(classPath); - if (!symbols) { - const source = readFileSync(classPath, 'utf8'); - symbols = extractJavaSymbols(source); - symbolsCache.set(classPath, symbols); - } - - return validateEntryAgainstSymbols(entry, symbols, allEntries); - } - /** * Detect duplicate and conflicting targets across the whole file. Delegates * to the pure `detectAccessTransformerConflicts` helper so the logic is diff --git a/src/services/bytecode-index-service.ts b/src/services/bytecode-index-service.ts new file mode 100644 index 0000000..1b1e166 --- /dev/null +++ b/src/services/bytecode-index-service.ts @@ -0,0 +1,202 @@ +/** + * Bytecode Index Service + * + * Provides authoritative per-class bytecode metadata for a remapped Minecraft + * JAR, backed by the bundled ASM bytecode-dumper. This is the ground truth the + * Access Transformer validator runs against. + * + * WHY BYTECODE (not decompiled `.java`): decompiled source OMITS compiler- + * generated members — a record's canonical constructor and its component + * accessors (`value()`, `name()`, …) never appear in VineFlower output. Parsing + * source therefore reports those implicit members as "not found", producing + * false positives (issue #12). The bytecode has every member with its true + * access flags and erased descriptors — exactly what an AT is applied to at + * load time (and exactly what `javap` shows). Validating against the JAR is the + * single, consistent source of truth. + * + * CACHING: the remapped JAR (`remapped/{version}-{mapping}.jar`) is a + * deterministic byproduct of decompilation and never changes for a given + * (version, mapping) — so metadata derived from it is inherently fresh. We keep + * a small, incremental, on-disk cache next to the JAR + * (`{version}-{mapping}.bytecode.json`) that grows as classes are requested. The + * cache stores the JAR's size+mtime signature; if the JAR is rebuilt (e.g. + * `decompile_minecraft_version --force`) the signature changes and the cache is + * discarded automatically. Only the handful of classes an AT actually targets + * are ever dumped — never the whole ~15k-class JAR. + */ + +import { existsSync, readFileSync, renameSync, statSync, unlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import AdmZip from 'adm-zip'; +import { type BytecodeClass, getBytecodeDumper } from '../java/bytecode-dumper.js'; +import type { MappingType } from '../types/minecraft.js'; +import { logger } from '../utils/logger.js'; +import { getRemappedJarPath } from '../utils/paths.js'; + +/** On-disk cache shape. `classes` maps an internal name → metadata, or `null` (known-absent). */ +interface BytecodeCacheFile { + /** `${size}:${mtimeMs}` of the remapped JAR the cache was built from. */ + signature: string; + /** Internal class name (slashes, `$`) → metadata, or `null` when the JAR has no such class. */ + classes: Record; +} + +/** Compute the invalidation signature for a remapped JAR (size + mtime). */ +function jarSignature(jarPath: string): string { + const st = statSync(jarPath); + return `${st.size}:${st.mtimeMs}`; +} + +/** Path of the sidecar cache file for a remapped JAR. */ +function cachePathFor(jarPath: string): string { + return jarPath.replace(/\.jar$/i, '.bytecode.json'); +} + +/** + * Resolves bytecode metadata for specific classes of a remapped Minecraft JAR, + * with an incremental on-disk cache. Stateless between calls apart from the + * cache file (safe for the short-lived CLI process). + */ +export class BytecodeIndexService { + /** + * Return bytecode metadata for the requested internal class names (slash/`$` + * form, no `.class`). Classes absent from the JAR are simply omitted from the + * returned map (and remembered as absent so they are not re-dumped). + * + * @throws if the remapped JAR does not exist — callers should check + * `hasRemappedJar` first and surface a "run decompile first" message. + */ + async getClassBytecode( + version: string, + mapping: MappingType, + internalNames: string[], + ): Promise> { + const jarPath = getRemappedJarPath(version, mapping); + if (!existsSync(jarPath)) { + throw new Error(`Remapped JAR not found: ${jarPath}`); + } + + const signature = jarSignature(jarPath); + const cachePath = cachePathFor(jarPath); + const cache = this.loadCache(cachePath, signature); + + const wanted = [...new Set(internalNames)]; + const missing = wanted.filter((n) => !(n in cache.classes)); + + if (missing.length > 0) { + const dumped = await this.dumpClasses(jarPath, missing); + for (const name of missing) { + // `null` = confirmed absent from the JAR, so we never re-dump it. + cache.classes[name] = dumped.get(name) ?? null; + } + this.saveCache(cachePath, cache); + } + + const result = new Map(); + for (const name of wanted) { + const entry = cache.classes[name]; + if (entry) result.set(name, entry); + } + return result; + } + + /** Delete the sidecar cache for a (version, mapping). Used by force-clear paths. */ + clearCache(version: string, mapping: MappingType): void { + const cachePath = cachePathFor(getRemappedJarPath(version, mapping)); + if (existsSync(cachePath)) { + try { + unlinkSync(cachePath); + } catch (error) { + logger.debug(`Failed to clear bytecode cache ${cachePath}: ${String(error)}`); + } + } + } + + /** + * Load the sidecar cache, returning a fresh empty cache when the file is + * missing, unparseable, or built from a different JAR (signature mismatch). + */ + private loadCache(cachePath: string, signature: string): BytecodeCacheFile { + if (existsSync(cachePath)) { + try { + const parsed = JSON.parse(readFileSync(cachePath, 'utf8')) as Partial; + if ( + parsed && + parsed.signature === signature && + typeof parsed.classes === 'object' && + parsed.classes !== null + ) { + return { signature, classes: parsed.classes as Record }; + } + } catch (error) { + logger.debug(`Ignoring corrupt bytecode cache ${cachePath}: ${String(error)}`); + } + } + return { signature, classes: {} }; + } + + /** Persist the cache atomically (write-temp + rename) so a crash cannot truncate it. */ + private saveCache(cachePath: string, cache: BytecodeCacheFile): void { + try { + const tmp = `${cachePath}.${process.pid}.tmp`; + writeFileSync(tmp, JSON.stringify(cache), 'utf8'); + renameSync(tmp, cachePath); + } catch (error) { + // A failed cache write is non-fatal — validation still works, just uncached. + logger.debug(`Failed to persist bytecode cache ${cachePath}: ${String(error)}`); + } + } + + /** + * Dump the requested classes from the remapped JAR. Extracts only their + * `.class` entries into a tiny temporary JAR and runs the bundled dumper once + * over it, so a 19-entry AT touches ~10 classes instead of the whole JAR. + */ + private async dumpClasses( + jarPath: string, + internalNames: string[], + ): Promise> { + const source = new AdmZip(jarPath); + const subset = new AdmZip(); + let added = 0; + for (const name of internalNames) { + const entry = source.getEntry(`${name}.class`); + if (entry) { + subset.addFile(`${name}.class`, entry.getData()); + added++; + } + } + + const result = new Map(); + if (added === 0) { + return result; // none of the requested classes exist in the JAR + } + + const tmpJar = join(tmpdir(), `mdm-at-bytecode-${process.pid}-${Date.now()}.jar`); + subset.writeZip(tmpJar); + try { + const dump = await getBytecodeDumper().dump(tmpJar); + for (const cls of dump.classes) { + result.set(cls.name, cls); + } + } finally { + try { + unlinkSync(tmpJar); + } catch { + // best-effort temp cleanup + } + } + return result; + } +} + +// Singleton instance +let bytecodeIndexServiceInstance: BytecodeIndexService | undefined; + +export function getBytecodeIndexService(): BytecodeIndexService { + if (!bytecodeIndexServiceInstance) { + bytecodeIndexServiceInstance = new BytecodeIndexService(); + } + return bytecodeIndexServiceInstance; +} diff --git a/src/services/decompile-service.ts b/src/services/decompile-service.ts index 3046760..9917f9b 100644 --- a/src/services/decompile-service.ts +++ b/src/services/decompile-service.ts @@ -8,6 +8,7 @@ import { ClassNotFoundError, DecompilationError } from '../utils/errors.js'; import { extractSourcesJar, inspectJar } from '../utils/jar-inspector.js'; import { logger } from '../utils/logger.js'; import { classNameToPath, getDecompiledPath } from '../utils/paths.js'; +import { getBytecodeIndexService } from './bytecode-index-service.js'; import { getRemapService } from './remap-service.js'; import { getSearchIndexService } from './search-index-service.js'; @@ -161,6 +162,7 @@ export class DecompileService { * - decompiled source directory * - decompile_jobs row (so getOrCreateJob doesn't short-circuit) * - FTS5 search index entries (so stale results don't surface) + * - the AT validator's bytecode sidecar cache (rebuilt from the new JAR) * * The next decompile call will rebuild from scratch. Indexing must be * triggered explicitly via index_minecraft_version after re-decompile. @@ -175,6 +177,10 @@ export class DecompileService { getDatabase().deleteJob(version, mapping); logger.info(`Force: clearing FTS5 index entries for ${version}/${mapping}`); getSearchIndexService().clearIndex(version, mapping); + // The remapped JAR is regenerated on the next decompile (new size/mtime, so + // the cache would self-invalidate) — clear the sidecar eagerly for tidiness. + logger.info(`Force: clearing bytecode cache for ${version}/${mapping}`); + getBytecodeIndexService().clearCache(version, mapping); } /** diff --git a/src/services/documentation-service.ts b/src/services/documentation-service.ts index ed9353f..dd146d3 100644 --- a/src/services/documentation-service.ts +++ b/src/services/documentation-service.ts @@ -531,7 +531,7 @@ Member kind is inferred from the token shape (no class/method/field keyword): a Two common pitfalls: - Inner classes: a nested type is only reachable if its enclosing class is public/protected (or widened in this file), so widen the enclosing class too. -- Records: widening a record to public/protected does NOT widen its canonical constructor, which crashes at runtime — add a matching directive at equal-or-wider access. +- Records: widening a record to public/protected does NOT widen its canonical constructor. Reading the record's components or codec is fine, but INSTANTIATING it (via 'new' or codec/network deserialization from your code) needs a widened ctor too — add a matching directive at equal-or-wider access. `.trim(), seeAlso: ['Access Wideners'], }; From 4cd7737556c25686261743d6fe6512e677213116 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 22:43:42 +0000 Subject: [PATCH 2/9] refactor(access-transformer): compact, verdict-first tool output 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 Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT --- .../access-transformer-service.test.ts | 25 ++++++---- src/server/tools.ts | 48 ++++++++++++++++--- src/services/access-transformer-service.ts | 18 +++++++ 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/__tests__/services/access-transformer-service.test.ts b/__tests__/services/access-transformer-service.test.ts index 9876d2d..312b18b 100644 --- a/__tests__/services/access-transformer-service.test.ts +++ b/__tests__/services/access-transformer-service.test.ts @@ -1057,7 +1057,7 @@ describe('Conflict detection', () => { }); describe('validate_access_transformer tool', () => { - it('returns a well-formed parse + validation envelope', async () => { + it('returns a compact, verdict-first envelope', async () => { const result = await handleValidateAccessTransformer({ // Second line is a 3-token field directive (`exampleField`) so the file // parses to exactly 2 entries with no parse errors. @@ -1066,11 +1066,18 @@ describe('validate_access_transformer tool', () => { }); expect(result.content).toHaveLength(1); const data = JSON.parse(result.content[0]?.text ?? ''); - expect(data.accessTransformer.entryCount).toBe(2); - expect(data.accessTransformer.parseErrorCount).toBe(0); - expect(typeof data.validation.isValid).toBe('boolean'); - expect(Array.isArray(data.validation.errors)).toBe(true); - expect(Array.isArray(data.validation.warnings)).toBe(true); + // Verdict + one-line summary up front (the tool-ran flag lives on the outer + // CLI/MCP envelope, not here). + expect(typeof data.valid).toBe('boolean'); + expect(data.summary).toContain('2 entries'); + expect(data.version).toBe(TEST_VERSION); + // No parse errors -> the parseErrors section is omitted entirely (compact). + expect(data.parseErrors).toBeUndefined(); + // Findings, when present, are one-line directive strings (no nested entry). + for (const f of data.errors ?? []) { + expect(typeof f.directive).toBe('string'); + expect(typeof f.line).toBe('number'); + } }, 30000); it('collects bad lines instead of throwing on garbage content', async () => { @@ -1080,7 +1087,9 @@ describe('validate_access_transformer tool', () => { }); expect(result.content).toHaveLength(1); const data = JSON.parse(result.content[0]?.text ?? ''); - expect(data.accessTransformer.entryCount).toBe(0); - expect(data.accessTransformer.parseErrorCount).toBeGreaterThan(0); + expect(data.summary).toContain('0 entries'); + expect(Array.isArray(data.parseErrors)).toBe(true); + expect(data.parseErrors.length).toBeGreaterThan(0); + expect(data.summary).toMatch(/parse error/); }, 30000); }); diff --git a/src/server/tools.ts b/src/server/tools.ts index 691232d..3dee659 100644 --- a/src/server/tools.ts +++ b/src/server/tools.ts @@ -2,7 +2,10 @@ import { existsSync, readFileSync, readdirSync } from 'node:fs'; import { join, relative } from 'node:path'; import { z } from 'zod'; import { getCacheManager } from '../cache/cache-manager.js'; -import { getAccessTransformerService } from '../services/access-transformer-service.js'; +import { + accessTransformerEntryToString, + getAccessTransformerService, +} from '../services/access-transformer-service.js'; import { getAccessWidenerService } from '../services/access-widener-service.js'; import { getAstDiffService } from '../services/ast-diff-service.js'; import { getDecompileService } from '../services/decompile-service.js'; @@ -17,6 +20,7 @@ import { getSearchIndexService } from '../services/search-index-service.js'; import { getVersionManager } from '../services/version-manager.js'; import type { AccessTransformer, + AccessTransformerEntry, AccessWidener, MappingType, MixinClass, @@ -1734,18 +1738,48 @@ export async function handleValidateAccessTransformer(args: unknown) { mapping as MappingType, ); + // Compact output: each finding is a one-line directive + message (not the + // full nested entry object), and empty sections are omitted. The verdict + // (`valid` + `summary`) is up front so it can't be missed — `success` on the + // envelope only means the tool ran, `valid` is the validation result. + const compact = (f: { + entry: AccessTransformerEntry; + message: string; + suggestion?: string; + }) => ({ + line: f.entry.line, + directive: accessTransformerEntryToString(f.entry), + message: f.message, + ...(f.suggestion ? { suggestion: f.suggestion } : {}), + }); + + const entryCount = accessTransformer.entries.length; + const errors = validation.errors.map(compact); + const warnings = validation.warnings.map(compact); + const parseErrors = accessTransformer.parseErrors; + + const plural = (n: number, singular: string, pluralForm = `${singular}s`) => + `${n} ${n === 1 ? singular : pluralForm}`; + const summaryParts = [ + plural(entryCount, 'entry', 'entries'), + plural(errors.length, 'error'), + plural(warnings.length, 'warning'), + ]; + if (parseErrors.length) summaryParts.push(plural(parseErrors.length, 'parse error')); + return { content: [ { type: 'text', text: JSON.stringify( { - accessTransformer: { - entryCount: accessTransformer.entries.length, - parseErrorCount: accessTransformer.parseErrors.length, - parseErrors: accessTransformer.parseErrors, - }, - validation, + valid: validation.isValid, + summary: summaryParts.join(' · '), + version: mcVersion, + mapping, + ...(errors.length ? { errors } : {}), + ...(warnings.length ? { warnings } : {}), + ...(parseErrors.length ? { parseErrors } : {}), }, null, 2, diff --git a/src/services/access-transformer-service.ts b/src/services/access-transformer-service.ts index f7965bf..f0af5ff 100644 --- a/src/services/access-transformer-service.ts +++ b/src/services/access-transformer-service.ts @@ -109,6 +109,24 @@ function sameModifier(a: AccessTransformerModifier, b: AccessTransformerModifier return a.access === b.access && a.final === b.final; } +/** + * Render a parsed entry back to its one-line AT directive text (the same shape + * the user wrote it as, e.g. `public net.mc.Foo value()Ljava/lang/String;`). + * Used to keep tool output compact — a finding shows this string instead of the + * full nested entry object. + */ +export function accessTransformerEntryToString(entry: AccessTransformerEntry): string { + const mod = modifierToString(entry.modifier); + if (entry.memberType === 'class' || !entry.memberName) { + return `${mod} ${entry.className}`; + } + const member = + entry.memberType === 'method' + ? `${entry.memberName}${entry.memberDescriptor ?? ''}` + : entry.memberName; + return `${mod} ${entry.className} ${member}`; +} + /** * Two modifiers are "incompatible" (Forge fails the build — * "Invalid AT final conflicts") when the access levels differ OR one wants From b69f2d2e2865e986171f3308ecea0ace470d1eaf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 22:50:00 +0000 Subject: [PATCH 3/9] fix(bytecode-index): harden cache against concurrent access 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 Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT --- src/services/bytecode-index-service.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/services/bytecode-index-service.ts b/src/services/bytecode-index-service.ts index 1b1e166..f7fd677 100644 --- a/src/services/bytecode-index-service.ts +++ b/src/services/bytecode-index-service.ts @@ -79,13 +79,18 @@ export class BytecodeIndexService { const signature = jarSignature(jarPath); const cachePath = cachePathFor(jarPath); - const cache = this.loadCache(cachePath, signature); + let cache = this.loadCache(cachePath, signature); const wanted = [...new Set(internalNames)]; const missing = wanted.filter((n) => !(n in cache.classes)); if (missing.length > 0) { const dumped = await this.dumpClasses(jarPath, missing); + // Re-read the cache right before writing and merge into THAT: a concurrent + // call may have dumped other classes while we awaited, and blindly saving + // our older snapshot would drop them (lost update). Disk only grows for a + // fixed signature, so the reload is a superset of what we started with. + cache = this.loadCache(cachePath, signature); for (const name of missing) { // `null` = confirmed absent from the JAR, so we never re-dump it. cache.classes[name] = dumped.get(name) ?? null; @@ -173,7 +178,10 @@ export class BytecodeIndexService { return result; // none of the requested classes exist in the JAR } - const tmpJar = join(tmpdir(), `mdm-at-bytecode-${process.pid}-${Date.now()}.jar`); + // Random suffix in addition to pid+time: two calls in the same millisecond + // must not collide on the temp path (one deleting the other's file mid-read). + const unique = `${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}`; + const tmpJar = join(tmpdir(), `mdm-at-bytecode-${unique}.jar`); subset.writeZip(tmpJar); try { const dump = await getBytecodeDumper().dump(tmpJar); From 639d9ff9fb247a935c95be3771de9b35f3e5fd44 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 19:45:55 -0400 Subject: [PATCH 4/9] chore: enforce LF line endings via .gitattributes 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 Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT --- .gitattributes | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..416fcc2 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,13 @@ +# Enforce LF line endings for text files on every platform. Without this, Windows +# checkouts convert to CRLF (via core.autocrlf), which the Biome formatter flags +# as errors even though the committed blobs are LF. +* text=auto eol=lf + +# Binary assets — never line-ending normalize. +*.jar binary +*.png binary +*.jpg binary +*.gif binary +*.ico binary +*.gz binary +*.zip binary From 2f0de277751dd36269748489447122a7f8918087 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 19:49:27 -0400 Subject: [PATCH 5/9] chore: fix pre-existing Biome lint/format findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT --- __tests__/core/mapping-service.test.ts | 25 ++++++++----------- __tests__/core/version-manager.test.ts | 18 ++++++------- __tests__/manual/mcp/stdio-matrix.test.ts | 2 +- .../manual/mcp/stdio-server-smoke.test.ts | 6 ++++- __tests__/manual/patched/patched-jar.test.ts | 1 - src/java/mapping-io.ts | 4 +-- 6 files changed, 27 insertions(+), 29 deletions(-) diff --git a/__tests__/core/mapping-service.test.ts b/__tests__/core/mapping-service.test.ts index d9c96ee..fbcf1fe 100644 --- a/__tests__/core/mapping-service.test.ts +++ b/__tests__/core/mapping-service.test.ts @@ -355,14 +355,14 @@ describe('Mojmap Tiny v2 Structure Verification', () => { // Find Entity class line const entityLineIdx = lines.findIndex( - (l) => l.startsWith('c\t') && l.includes('Entity') && !l.includes('EntityType') + (l) => l.startsWith('c\t') && l.includes('Entity') && !l.includes('EntityType'), ); expect(entityLineIdx).toBeGreaterThan(0); // The line immediately after a class definition should be a field or method // (starts with a tab character), NOT another class definition const nextLine = lines[entityLineIdx + 1]; - if (nextLine && nextLine.trim()) { + if (nextLine?.trim()) { // If there's content, it should be a nested member (starts with tab) // or another class (no tab). We're checking that fields/methods // ARE nested under classes, not grouped separately. @@ -414,7 +414,7 @@ describe('Mojmap Tiny v2 Structure Verification', () => { TEST_VERSION, 'tick', 'mojmap', - 'intermediary' + 'intermediary', ); expect(result.found).toBe(true); @@ -455,28 +455,23 @@ describe('Unobfuscated version handling', () => { it('should throw actionable error for getMappings(yarn) on unobfuscated version', async () => { const mappingService = getMappingService(); - await expect( - mappingService.getMappings(UNOBFUSCATED_TEST_VERSION, 'yarn'), - ).rejects.toThrow(/unobfuscated.*mojmap/is); + await expect(mappingService.getMappings(UNOBFUSCATED_TEST_VERSION, 'yarn')).rejects.toThrow( + /unobfuscated.*mojmap/is, + ); }, 30000); it('should throw actionable error for getMappings(mojmap) on unobfuscated version', async () => { const mappingService = getMappingService(); - await expect( - mappingService.getMappings(UNOBFUSCATED_TEST_VERSION, 'mojmap'), - ).rejects.toThrow(/unobfuscated.*already in Mojang/is); + await expect(mappingService.getMappings(UNOBFUSCATED_TEST_VERSION, 'mojmap')).rejects.toThrow( + /unobfuscated.*already in Mojang/is, + ); }, 30000); it('should throw actionable error for lookupMapping on unobfuscated version', async () => { const mappingService = getMappingService(); // lookupMapping calls getMappings internally, which throws for unobfuscated versions await expect( - mappingService.lookupMapping( - UNOBFUSCATED_TEST_VERSION, - 'Entity', - 'mojmap', - 'yarn', - ), + mappingService.lookupMapping(UNOBFUSCATED_TEST_VERSION, 'Entity', 'mojmap', 'yarn'), ).rejects.toThrow(/unobfuscated/i); }, 30000); diff --git a/__tests__/core/version-manager.test.ts b/__tests__/core/version-manager.test.ts index 17acc3d..4ad9e08 100644 --- a/__tests__/core/version-manager.test.ts +++ b/__tests__/core/version-manager.test.ts @@ -72,14 +72,14 @@ describe('Version Management', () => { }, 30000); // Regression tests for https://github.com/MCDxAI/minecraft-dev-mcp/issues/5 - it.each([ - '26.1-snapshot-10', - '26.1-snapshot-11', - '26.1-rc-3', - ])('should return true for %s (issue #5)', async (version) => { - const versionManager = getVersionManager(); - const result = await versionManager.isVersionUnobfuscated(version); - expect(result).toBe(true); - }, 30000); + it.each(['26.1-snapshot-10', '26.1-snapshot-11', '26.1-rc-3'])( + 'should return true for %s (issue #5)', + async (version) => { + const versionManager = getVersionManager(); + const result = await versionManager.isVersionUnobfuscated(version); + expect(result).toBe(true); + }, + 30000, + ); }); }); diff --git a/__tests__/manual/mcp/stdio-matrix.test.ts b/__tests__/manual/mcp/stdio-matrix.test.ts index 11f5112..b591928 100644 --- a/__tests__/manual/mcp/stdio-matrix.test.ts +++ b/__tests__/manual/mcp/stdio-matrix.test.ts @@ -1,10 +1,10 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { getVersionManager } from '../../../src/services/version-manager.js'; import { type McpTestSession, createMcpSession, extractFirstText, } from '../../helpers/mcp-stdio.js'; -import { getVersionManager } from '../../../src/services/version-manager.js'; import { parseMatrixVersionsFromEnv } from './test-constants.js'; const matrixVersions = parseMatrixVersionsFromEnv(); diff --git a/__tests__/manual/mcp/stdio-server-smoke.test.ts b/__tests__/manual/mcp/stdio-server-smoke.test.ts index f212f15..45988ca 100644 --- a/__tests__/manual/mcp/stdio-server-smoke.test.ts +++ b/__tests__/manual/mcp/stdio-server-smoke.test.ts @@ -1,5 +1,9 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { type McpTestSession, createMcpSession, extractFirstText } from '../../helpers/mcp-stdio.js'; +import { + type McpTestSession, + createMcpSession, + extractFirstText, +} from '../../helpers/mcp-stdio.js'; import { TEST_MAPPING, TEST_VERSION, UNOBFUSCATED_TEST_VERSION } from '../../test-constants.js'; /** diff --git a/__tests__/manual/patched/patched-jar.test.ts b/__tests__/manual/patched/patched-jar.test.ts index d96f56d..e95c3eb 100644 --- a/__tests__/manual/patched/patched-jar.test.ts +++ b/__tests__/manual/patched/patched-jar.test.ts @@ -32,7 +32,6 @@ const SKIP = !PATCHED_JAR_PATH || !existsSync(PATCHED_JAR_PATH); const describePatched = SKIP ? describe.skip : describe; if (SKIP) { - // biome-ignore lint/suspicious/noConsole: visible signal in CI logs about why we're skipping console.warn( `[patched-jar.test] Skipping: PATCHED_JAR_PATH is unset or does not exist (${PATCHED_JAR_PATH || ''})`, ); diff --git a/src/java/mapping-io.ts b/src/java/mapping-io.ts index 4f32408..c01957a 100644 --- a/src/java/mapping-io.ts +++ b/src/java/mapping-io.ts @@ -30,7 +30,7 @@ export class MappingIOWrapper { proguardFile: string, intermediaryFile: string, outputFile: string, - options: MappingIOOptions = {} + options: MappingIOOptions = {}, ): Promise { const jarPath = getJavaResourceDownloader().getMappingIOCliJar(); @@ -67,7 +67,7 @@ export class MappingIOWrapper { } catch (error) { logger.error('MappingIO conversion failed', error); throw new Error( - `MappingIO conversion failed: ${error instanceof Error ? error.message : 'Unknown error'}` + `MappingIO conversion failed: ${error instanceof Error ? error.message : 'Unknown error'}`, ); } } From e30f240d7facbd0a4bb8d50dbeb78178bf158cb5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 19:55:24 -0400 Subject: [PATCH 6/9] refactor(access-widener): validate against bytecode, not decompiled source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01V3CkKZMkCya7Nu9DAnSttT --- .../services/access-widener-service.test.ts | 309 +++++++------- src/server/tools.ts | 41 +- src/services/access-widener-service.ts | 384 ++++++------------ 3 files changed, 323 insertions(+), 411 deletions(-) diff --git a/__tests__/services/access-widener-service.test.ts b/__tests__/services/access-widener-service.test.ts index cd8b80e..42b312f 100644 --- a/__tests__/services/access-widener-service.test.ts +++ b/__tests__/services/access-widener-service.test.ts @@ -2,20 +2,25 @@ import { rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; +import type { + BytecodeClass, + BytecodeField, + BytecodeMethod, +} from '../../src/java/bytecode-dumper.js'; import { handleValidateAccessWidener } from '../../src/server/tools.js'; import { getAccessWidenerService, - validateEntryAgainstSource, + validateEntryAgainstBytecode, } from '../../src/services/access-widener-service.js'; import type { AccessWidenerEntry } from '../../src/types/minecraft.js'; import { AccessWidenerParseError } from '../../src/utils/errors.js'; import { TEST_MAPPING, TEST_VERSION } from '../test-constants.js'; /** - * Build an AccessWidenerEntry with sensible defaults for the regression - * tests below. These exercise the tree-sitter + descriptor-matching validator - * via the `validateEntryAgainstSource` test seam (synthetic Java source, no - * decompiled Minecraft tree required). + * Build an AccessWidenerEntry with sensible defaults for the regression tests + * below. Validation exercises go through the `validateEntryAgainstBytecode` test + * seam, driven by hand-built `BytecodeClass` fixtures — bytecode is the same + * ground truth the production path reads from the remapped JAR. */ function makeEntry( partial: Partial & { className: string }, @@ -28,12 +33,52 @@ function makeEntry( }; } +// --- Bytecode fixture builders (mirror the ASM dumper's JSON shape) --------- + +function bcMethod(name: string, desc: string, flags: string[] = ['public']): BytecodeMethod { + return { name, desc, access: 0, flags, signature: null, exceptions: [] }; +} + +function bcField(name: string, desc = 'I', flags: string[] = ['public']): BytecodeField { + return { name, desc, access: 0, flags, signature: null, value: null }; +} + +function bcClass(partial: Partial & { name: string }): BytecodeClass { + return { + access: 0, + flags: ['public'], + superName: 'java/lang/Object', + interfaces: [], + signature: null, + isInterface: false, + isEnum: false, + isRecord: false, + isAnnotation: false, + isAbstract: false, + isFinal: false, + isSealed: false, + nestHost: null, + nestMembers: null, + permittedSubclasses: null, + recordComponents: null, + canonicalConstructor: null, + innerClasses: [], + fields: [], + methods: [], + ...partial, + }; +} + +function mapOf(...classes: BytecodeClass[]): Map { + return new Map(classes.map((c) => [c.name, c])); +} + /** * Access Widener Service Tests * * Tests the access widener service's ability to: * - Parse Access Widener files - * - Validate entries against Minecraft source + * - Validate entries against Minecraft bytecode * - Convert Java descriptors to readable format */ @@ -97,7 +142,7 @@ accessible field net/minecraft/entity/Entity age I expect(awService.descriptorToReadable('(II)V')).toBe('void (int, int)'); }); - it('should handle validate_access_widener tool', async () => { + it('should handle validate_access_widener tool (compact, verdict-first envelope)', async () => { const content = ` accessWidener v2 named @@ -114,15 +159,16 @@ accessible class net/minecraft/entity/Entity expect(result.content).toHaveLength(1); const data = JSON.parse(result.content[0].text); - // The single parsed class entry flows into the response verbatim. - expect(data.accessWidener.namespace).toBe('named'); - expect(data.accessWidener.version).toBe(2); - expect(data.accessWidener.entryCount).toBe(1); - // Validation ran against that entry and returned a well-formed result. - expect(data.validation).toBeDefined(); - expect(typeof data.validation.isValid).toBe('boolean'); - expect(Array.isArray(data.validation.errors)).toBe(true); - expect(Array.isArray(data.validation.warnings)).toBe(true); + expect(typeof data.valid).toBe('boolean'); + expect(data.summary).toContain('1 entry'); + expect(data.version).toBe(TEST_VERSION); + expect(data.mapping).toBe(TEST_MAPPING); + expect(data.namespace).toBe('named'); + // Findings, when present, are one-line directive strings (no nested entry). + for (const f of data.errors ?? []) { + expect(typeof f.directive).toBe('string'); + expect(typeof f.line).toBe('number'); + } }, 30000); it('should handle invalid access widener gracefully', async () => { @@ -136,12 +182,10 @@ accessible class net/minecraft/entity/Entity // The garbage content is not a file path and matches no valid access/target // type, so the parser rejects every line — zero entries — rather than - // producing a bogus parsed-success shape. (No throw: bad lines are skipped, - // not fatal.) This is stronger than merely "a response exists." + // producing a bogus parsed-success shape. (No throw: bad lines are skipped.) const data = JSON.parse(result.content[0].text); - expect(data.accessWidener.entryCount).toBe(0); - expect(data.accessWidener.namespace).toBe('named'); - expect(data.accessWidener.version).toBe(1); + expect(data.summary).toContain('0 entries'); + expect(data.namespace).toBe('named'); }); it('generateAccessWidener produces the exact expected text and round-trips', () => { @@ -262,19 +306,7 @@ accessible class net/minecraft/entity/Entity it('descriptorToReadable terminates on malformed input (no infinite loop)', () => { const awService = getAccessWidenerService(); - // Each case previously could hang parseType (a missing ';' rewound `i` to 0, - // or an unrecognized char never advanced the cursor). The 2s per-test - // timeout pins the regression: any hang fails this test rather than the - // suite. All must return a string without throwing. - const malformed = [ - '(Ljava/lang/String', // method, object param with no closing ';' - 'Lnet/mc/X', // bare object type, no ';' - '(X)V', // stray unrecognized char inside params - '', // empty string - '(', // lone open paren - '[Lnet/mc/X', // array of unterminated object type - '(II', // params with no closing ')' - ]; + const malformed = ['(Ljava/lang/String', 'Lnet/mc/X', '(X)V', '', '(', '[Lnet/mc/X', '(II']; for (const desc of malformed) { const out = awService.descriptorToReadable(desc); expect(typeof out).toBe('string'); @@ -282,50 +314,29 @@ accessible class net/minecraft/entity/Entity }, 2000); }); -describe('Access Widener Validation (tree-sitter + descriptor matching)', () => { - it('does NOT match a method that is only called (not declared)', () => { - // `helper` is invoked inside doWork() but never declared. The old regex - // `\bhelper\s*\(` matched the call site and reported a false positive. - const src = ` -package net.test; -public class Caller { - public void doWork() { - helper(); - } -} -`; +describe('Access Widener Validation (bytecode + descriptor matching)', () => { + it('reports a method that is not declared in the class', () => { + const cls = bcClass({ name: 'net/test/Caller', methods: [bcMethod('doWork', '()V')] }); const entry = makeEntry({ targetType: 'method', className: 'net.test.Caller', memberName: 'helper', memberDescriptor: '()V', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors.some((e) => e.includes('not found'))).toBe(true); - // No spurious suggestion for an undeclared name. expect(res.suggestion).toBeUndefined(); }); it('validates constructor descriptors against the real constructor', () => { - // The old code returned `source.includes('public ')` which is ALWAYS true. - const src = ` -package net.test; -public class Thing { - private int x; - private int y; - public Thing(int a, int b) { - this.x = a; - this.y = b; - } -} -`; + const cls = bcClass({ name: 'net/test/Thing', methods: [bcMethod('', '(II)V')] }); const matching = makeEntry({ targetType: 'method', className: 'net.test.Thing', memberName: '', memberDescriptor: '(II)V', }); - expect(validateEntryAgainstSource(matching, src).errors).toEqual([]); + expect(validateEntryAgainstBytecode(matching, mapOf(cls)).errors).toEqual([]); const wrongArity = makeEntry({ targetType: 'method', @@ -333,57 +344,47 @@ public class Thing { memberName: '', memberDescriptor: '(I)V', }); - const res = validateEntryAgainstSource(wrongArity, src); - expect(res.errors.some((e) => e.includes('no overload matches'))).toBe(true); + expect( + validateEntryAgainstBytecode(wrongArity, mapOf(cls)).errors.some((e) => + e.includes('no overload matches'), + ), + ).toBe(true); }); it('reports overload mismatch and lists the found overloads', () => { - const src = ` -package net.test; -public class Over { - public void foo(int x) {} - public void foo(String s) {} -} -`; - // foo(int) and foo(String) both take 1 arg; AW targets a 2-arg overload. + const cls = bcClass({ + name: 'net/test/Over', + methods: [bcMethod('foo', '(I)V'), bcMethod('foo', '(Ljava/lang/String;)V')], + }); const entry = makeEntry({ targetType: 'method', className: 'net.test.Over', memberName: 'foo', memberDescriptor: '(II)V', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); const err = res.errors.find((e) => e.includes('no overload matches')); expect(err).toBeDefined(); - // The actual overloads are reported so the user can pick the right one. expect(err).toContain('(I)V'); expect(err).toContain('(Ljava/lang/String;)V'); }); it('matches a correct overload by descriptor', () => { - const src = ` -package net.test; -public class Over { - public void foo(int x) {} - public void foo(String s) {} -} -`; + const cls = bcClass({ + name: 'net/test/Over', + methods: [bcMethod('foo', '(I)V'), bcMethod('foo', '(Ljava/lang/String;)V')], + }); const entry = makeEntry({ targetType: 'method', className: 'net.test.Over', memberName: 'foo', memberDescriptor: '(Ljava/lang/String;)V', }); - expect(validateEntryAgainstSource(entry, src).errors).toEqual([]); + expect(validateEntryAgainstBytecode(entry, mapOf(cls)).errors).toEqual([]); }); it('validates field descriptors (mismatch vs match)', () => { - const src = ` -package net.test; -public class Person { - public int age; -} -`; + const cls = bcClass({ name: 'net/test/Person', fields: [bcField('age', 'I')] }); const mismatch = makeEntry({ targetType: 'field', className: 'net.test.Person', @@ -391,7 +392,7 @@ public class Person { memberDescriptor: 'Ljava/lang/String;', }); expect( - validateEntryAgainstSource(mismatch, src).errors.some((e) => + validateEntryAgainstBytecode(mismatch, mapOf(cls)).errors.some((e) => e.includes('descriptor mismatch'), ), ).toBe(true); @@ -402,20 +403,17 @@ public class Person { memberName: 'age', memberDescriptor: 'I', }); - expect(validateEntryAgainstSource(ok, src).errors).toEqual([]); + expect(validateEntryAgainstBytecode(ok, mapOf(cls)).errors).toEqual([]); }); - it('warns on mutable for an already-non-final (incl. generic) field', () => { - // The old `(?!final)\w+` regex silently missed `List items` - // because `\w+` stopped at the `<`. - const src = ` -package net.test; -import java.util.List; -public class Holder { - public List items; - public int counter; -} -`; + it('warns on mutable for an already-non-final field', () => { + const cls = bcClass({ + name: 'net/test/Holder', + fields: [ + bcField('items', 'Ljava/util/List;', ['public']), + bcField('counter', 'I', ['public']), + ], + }); const items = makeEntry({ accessType: 'mutable', targetType: 'field', @@ -423,31 +421,16 @@ public class Holder { memberName: 'items', memberDescriptor: 'Ljava/util/List;', }); - const itemsRes = validateEntryAgainstSource(items, src); + const itemsRes = validateEntryAgainstBytecode(items, mapOf(cls)); expect(itemsRes.errors).toEqual([]); expect(itemsRes.warnings.some((w) => w.includes('already be mutable'))).toBe(true); - - const counter = makeEntry({ - accessType: 'mutable', - targetType: 'field', - className: 'net.test.Holder', - memberName: 'counter', - memberDescriptor: 'I', - }); - expect( - validateEntryAgainstSource(counter, src).warnings.some((w) => - w.includes('already be mutable'), - ), - ).toBe(true); }); it('does not warn on mutable for a final field', () => { - const src = ` -package net.test; -public class Fixed { - public final int locked = 1; -} -`; + const cls = bcClass({ + name: 'net/test/Fixed', + fields: [bcField('locked', 'I', ['public', 'final'])], + }); const entry = makeEntry({ accessType: 'mutable', targetType: 'field', @@ -455,51 +438,39 @@ public class Fixed { memberName: 'locked', memberDescriptor: 'I', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors).toEqual([]); expect(res.warnings.some((w) => w.includes('already be mutable'))).toBe(false); }); it('warns on extendable for a final class but not for a non-final one', () => { - const finalSrc = ` -package net.test; -public final class Locked {} -`; + const finalCls = bcClass({ name: 'net/test/Locked', isFinal: true }); const finalEntry = makeEntry({ accessType: 'extendable', targetType: 'class', className: 'net.test.Locked', }); expect( - validateEntryAgainstSource(finalEntry, finalSrc).warnings.some((w) => w.includes('is final')), + validateEntryAgainstBytecode(finalEntry, mapOf(finalCls)).warnings.some((w) => + w.includes('is final'), + ), ).toBe(true); - // The old `source.includes('final class')` substring matched the inner - // final class AND the comment, producing a false warning for `Open`. - const trickySrc = ` -package net.test; -// this comment mentions final class on purpose -public class Open { - public static final class Inner {} -} -`; + const openCls = bcClass({ name: 'net/test/Open', isFinal: false }); const openEntry = makeEntry({ accessType: 'extendable', targetType: 'class', className: 'net.test.Open', }); expect( - validateEntryAgainstSource(openEntry, trickySrc).warnings.some((w) => w.includes('is final')), + validateEntryAgainstBytecode(openEntry, mapOf(openCls)).warnings.some((w) => + w.includes('is final'), + ), ).toBe(false); }); it('suggests a similar field name when the target is missing', () => { - const src = ` -package net.test; -public class Box { - public int counter; -} -`; + const cls = bcClass({ name: 'net/test/Box', fields: [bcField('counter', 'I')] }); const entry = makeEntry({ accessType: 'mutable', targetType: 'field', @@ -507,28 +478,58 @@ public class Box { memberName: 'countre', memberDescriptor: 'I', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors.some((e) => e.includes('not found'))).toBe(true); expect(res.suggestion).toContain('counter'); }); - it('warns (does not error) on static initializers', () => { - const src = ` -package net.test; -public class WithStatic { - static { - System.out.println("init"); - } -} -`; + it('finds a static initializer present in bytecode (no false "not found")', () => { + // Bytecode contains when the class has a static initializer, so — + // unlike the old decompiled-source path — it validates without error. + const cls = bcClass({ + name: 'net/test/WithStatic', + methods: [bcMethod('', '()V', ['static'])], + }); const entry = makeEntry({ targetType: 'method', className: 'net.test.WithStatic', memberName: '', memberDescriptor: '()V', }); - const res = validateEntryAgainstSource(entry, src); + const res = validateEntryAgainstBytecode(entry, mapOf(cls)); expect(res.errors).toEqual([]); - expect(res.warnings.some((w) => w.includes(''))).toBe(true); + }); + + it('finds an implicit record component accessor (issue #12 regression)', () => { + // The record accessor `value()` is compiler-generated and absent from + // decompiled source, but present in bytecode — so widening it must not be a + // false "not found". + const rec = bcClass({ + name: 'net/mc/ExactMatcher', + isRecord: true, + recordComponents: [{ name: 'value', descriptor: 'Ljava/lang/String;', signature: null }], + canonicalConstructor: '(Ljava/lang/String;)V', + methods: [ + bcMethod('', '(Ljava/lang/String;)V', []), + bcMethod('value', '()Ljava/lang/String;'), + ], + }); + const entry = makeEntry({ + accessType: 'accessible', + targetType: 'method', + className: 'net.mc.ExactMatcher', + memberName: 'value', + memberDescriptor: '()Ljava/lang/String;', + }); + expect(validateEntryAgainstBytecode(entry, mapOf(rec)).errors).toEqual([]); + }); + + it('errors on a class absent from the JAR with a suggestion', () => { + const res = validateEntryAgainstBytecode( + makeEntry({ targetType: 'class', className: 'net.test.Ghost' }), + mapOf(bcClass({ name: 'net/test/Ghosts' })), + ); + expect(res.errors.some((e) => e.includes('Class not found'))).toBe(true); + expect(res.suggestion).toContain('Ghosts'); }); }); diff --git a/src/server/tools.ts b/src/server/tools.ts index 3dee659..3d22a4c 100644 --- a/src/server/tools.ts +++ b/src/server/tools.ts @@ -6,7 +6,10 @@ import { accessTransformerEntryToString, getAccessTransformerService, } from '../services/access-transformer-service.js'; -import { getAccessWidenerService } from '../services/access-widener-service.js'; +import { + accessWidenerEntryToString, + getAccessWidenerService, +} from '../services/access-widener-service.js'; import { getAstDiffService } from '../services/ast-diff-service.js'; import { getDecompileService } from '../services/decompile-service.js'; import { getDocumentationService } from '../services/documentation-service.js'; @@ -22,6 +25,7 @@ import type { AccessTransformer, AccessTransformerEntry, AccessWidener, + AccessWidenerEntry, MappingType, MixinClass, } from '../types/minecraft.js'; @@ -1678,18 +1682,41 @@ export async function handleValidateAccessWidener(args: unknown) { mapping as MappingType, ); + // Compact, verdict-first output (mirrors validate_access_transformer): each + // finding is a one-line directive + message, empty sections omitted. The + // envelope's `success` means "the tool ran"; `valid` is the verdict. + const compact = (f: { entry: AccessWidenerEntry; message: string; suggestion?: string }) => ({ + line: f.entry.line, + directive: accessWidenerEntryToString(f.entry), + message: f.message, + ...(f.suggestion ? { suggestion: f.suggestion } : {}), + }); + + const entryCount = accessWidener.entries.length; + const errors = validation.errors.map(compact); + const warnings = validation.warnings.map(compact); + + const plural = (n: number, singular: string, pluralForm = `${singular}s`) => + `${n} ${n === 1 ? singular : pluralForm}`; + const summary = [ + plural(entryCount, 'entry', 'entries'), + plural(errors.length, 'error'), + plural(warnings.length, 'warning'), + ].join(' · '); + return { content: [ { type: 'text', text: JSON.stringify( { - accessWidener: { - namespace: accessWidener.namespace, - version: accessWidener.version, - entryCount: accessWidener.entries.length, - }, - validation, + valid: validation.isValid, + summary, + version: mcVersion, + mapping, + namespace: accessWidener.namespace, + ...(errors.length ? { errors } : {}), + ...(warnings.length ? { warnings } : {}), }, null, 2, diff --git a/src/services/access-widener-service.ts b/src/services/access-widener-service.ts index 887ae8d..704f484 100644 --- a/src/services/access-widener-service.ts +++ b/src/services/access-widener-service.ts @@ -1,21 +1,26 @@ /** * Access Widener Service * - * Parses and validates Fabric Access Widener files. - * Access wideners allow mods to change the access level of classes, methods, and fields. + * Parses and validates Fabric Access Widener files. Access wideners let mods + * change the access level of classes, methods, and fields at load time. * - * Validation is tree-sitter based: decompiled Java source is parsed once into an - * AST (via `extractJavaSymbols`) and entries are correlated against the declared - * symbols. Access-widener method/field `memberDescriptor`s (JVM form) are now - * validated against the actual signatures, not just names. There is NO regex - * Java-source inspection left; the only string-pattern operations remaining are - * AW-file-format parsing (parseEntry), class-name/path conversion, and pure JVM - * descriptor decode logic (shared via `descriptor-utils`). + * GROUND TRUTH IS BYTECODE, NOT DECOMPILED SOURCE. Validation runs against the + * remapped Minecraft JAR via the ASM bytecode-dumper (see + * `bytecode-index-service.ts`), NOT VineFlower `.java`. Decompiled source omits + * compiler-generated members — a record's canonical constructor and its + * component accessors — so a source-based check reports them as "not found" + * even though they exist in the class file (the same issue-#12 class of false + * positive fixed for the access-transformer tool; both validators now share the + * bytecode path so they cannot diverge). Bytecode carries every member with its + * true access flags and erased descriptors, so member existence is an exact + * lookup and descriptor matching is exact string comparison. + * + * Default mapping is `'yarn'` (Fabric's toolchain). */ import { existsSync, readFileSync } from 'node:fs'; -import { join } from 'node:path'; import { getCacheManager } from '../cache/cache-manager.js'; +import type { BytecodeClass } from '../java/bytecode-dumper.js'; import type { AccessWidener, AccessWidenerEntry, @@ -24,174 +29,106 @@ import type { AccessWidenerValidation, MappingType, } from '../types/minecraft.js'; -import { - classNamesMatch, - descriptorsCompatible, - javaTypeToDescriptor, - paramToDescriptor, - parseParamDescriptors, - descriptorToReadable as sharedDescriptorToReadable, -} from '../utils/descriptor-utils.js'; +import { descriptorToReadable as sharedDescriptorToReadable } from '../utils/descriptor-utils.js'; import { AccessWidenerParseError } from '../utils/errors.js'; -import { extractJavaSymbols } from '../utils/java-symbols.js'; -import type { JavaSymbol } from '../utils/java-symbols.js'; import { logger } from '../utils/logger.js'; -import { getDecompiledPath } from '../utils/paths.js'; -import { findSimilarClassFile, findSimilarName } from '../utils/suggestions.js'; - -// --------------------------------------------------------------------------- -// AW-local descriptor helpers (module-internal) -// --------------------------------------------------------------------------- -// -// `buildMethodDescriptor` / `methodDescriptorMatches` / `fieldDescriptorMatches` -// are AW-validation-shaped (they take a `JavaSymbol`) and live here. The -// generic primitives they build on — `javaTypeToDescriptor`, `paramToDescriptor`, -// `parseParamDescriptors`, `descriptorsCompatible`, `classNamesMatch` — now -// live in `../utils/descriptor-utils.js` (shared with the mixin + AT validators). -// `descriptorToReadable` (descriptor decode) and the edit-distance suggestion -// helpers live in `descriptor-utils.js` / `suggestions.js` respectively. - -/** Build the JVM descriptor string for an AST method/constructor symbol. */ -function buildMethodDescriptor(method: JavaSymbol): string { - const params = method.parameters ?? []; - return `(${params.map(paramToDescriptor).join('')})${javaTypeToDescriptor(method.returnType ?? 'void')}`; -} +import { findSimilarName } from '../utils/suggestions.js'; +import { getBytecodeIndexService } from './bytecode-index-service.js'; -/** Compare an AST method/constructor against an AW method descriptor. */ -function methodDescriptorMatches( - method: JavaSymbol, - awDescriptor: string, -): { match: boolean; reason?: string } { - const open = awDescriptor.indexOf('('); - const close = awDescriptor.indexOf(')'); - if (open < 0 || close < 0 || close <= open) { - return { match: false, reason: `Malformed method descriptor: ${awDescriptor}` }; - } - - const awParams = parseParamDescriptors(awDescriptor.slice(open + 1, close)); - const awReturn = awDescriptor.slice(close + 1); - - const params = method.parameters ?? []; - const astParams = params.map(paramToDescriptor); - const astReturn = javaTypeToDescriptor(method.returnType ?? 'void'); +/** A map of internal class name (slashes, `$`) → its authoritative bytecode metadata. */ +type ClassBytecodeMap = Map; - if (astParams.length !== awParams.length) { - return { match: false, reason: `arity ${astParams.length} vs ${awParams.length}` }; - } - for (let i = 0; i < astParams.length; i++) { - if (!descriptorsCompatible(astParams[i], awParams[i])) { - return { match: false, reason: `param ${i}: ${astParams[i]} vs ${awParams[i]}` }; - } - } - if (!descriptorsCompatible(astReturn, awReturn)) { - return { match: false, reason: `return: ${astReturn} vs ${awReturn}` }; - } - return { match: true }; +/** Convert an AW dotted+`$` class name to an internal JVM name (slashes). */ +function toInternalName(className: string): string { + return className.replace(/\./g, '/'); } -/** Compare an AST field against an AW field descriptor. */ -function fieldDescriptorMatches( - field: JavaSymbol, - awDescriptor: string, -): { match: boolean; reason?: string } { - const astDesc = javaTypeToDescriptor(field.fieldType ?? ''); - return descriptorsCompatible(astDesc, awDescriptor) - ? { match: true } - : { match: false, reason: `${astDesc} vs ${awDescriptor}` }; +/** Simple (last-segment) name of an internal class name, for suggestions. */ +function simpleClassName(internal: string): string { + const slash = internal.lastIndexOf('/'); + const tail = slash >= 0 ? internal.slice(slash + 1) : internal; + const dollar = tail.lastIndexOf('$'); + return dollar >= 0 ? tail.slice(dollar + 1) : tail; } -// --------------------------------------------------------------------------- -// Pure helpers: member-name correlation (module-internal) -// --------------------------------------------------------------------------- - -/** Unique regular (non-constructor) method names declared by the given class symbols. */ -function uniqueMethodNames(classSymbols: JavaSymbol[]): string[] { - const set = new Set(); - for (const s of classSymbols) { - if (s.entryType === 'method' && s.isConstructor !== true && s.symbol) set.add(s.symbol); - } - return [...set]; -} - -/** Unique field names declared by the given class symbols. */ -function uniqueFieldNames(classSymbols: JavaSymbol[]): string[] { - const set = new Set(); - for (const s of classSymbols) { - if (s.entryType === 'field' && s.symbol) set.add(s.symbol); - } - return [...set]; +/** + * Render a parsed entry back to its one-line AW directive text (e.g. + * `accessible method net/minecraft/Foo tick ()V`). Used to keep tool output + * compact — a finding shows this string instead of the full nested entry object. + */ +export function accessWidenerEntryToString(entry: AccessWidenerEntry): string { + const head = `${entry.accessType} ${entry.targetType} ${entry.className}`; + if (entry.targetType === 'class' || !entry.memberName) return head; + const desc = entry.memberDescriptor ? ` ${entry.memberDescriptor}` : ''; + return `${head} ${entry.memberName}${desc}`; } // --------------------------------------------------------------------------- -// Core validation against extracted symbols (module-internal) +// Core validation against authoritative bytecode // --------------------------------------------------------------------------- /** - * Validate a single access-widener entry against the AST symbols of its class. - * Pure: no filesystem, no I/O. This is the heart of the validator. + * Validate a single access-widener entry against the bytecode metadata of its + * class. Pure: no filesystem, no I/O. `classMap` is keyed by internal class name + * (slashes, `$`) and must contain the entry's class. + * + * @internal */ -function validateEntryAgainstSymbols( +export function validateEntryAgainstBytecode( entry: AccessWidenerEntry, - symbols: JavaSymbol[], + classMap: ClassBytecodeMap, ): { errors: string[]; warnings: string[]; suggestion?: string } { const errors: string[] = []; const warnings: string[] = []; let suggestion: string | undefined; - // Symbols declared by the entry's class. `classNamesMatch` reconciles JVM - // inner-class `$` separators with the AST's dotted declaringClass. - const classSymbols = symbols.filter((s) => classNamesMatch(entry.className, s.declaringClass)); + const internal = toInternalName(entry.className); + const cls = classMap.get(internal); + if (!cls) { + errors.push(`Class not found: ${entry.className}`); + const similar = findSimilarName( + simpleClassName(internal), + [...classMap.keys()].map(simpleClassName), + ); + if (similar) suggestion = `Did you mean a class named: ${similar}?`; + return { errors, warnings, suggestion }; + } - // --- Class-level entry --- + // --- Class entry --- if (entry.targetType === 'class') { - if (entry.accessType === 'extendable') { - const classSym = classSymbols.find((s) => s.entryType === 'class'); - // Tied to the SPECIFIC entry.className via classNamesMatch above — the - // old whole-file `source.includes('final class')` substring check is - // gone (it matched inner classes and comments). - if (classSym && (classSym.isFinal || classSym.modifiers?.includes('final'))) { - warnings.push(`Class ${entry.className} is final - extendable may not work as expected`); - } + // `extendable` on a final class won't let you subclass it. Uses the real + // ACC_FINAL flag from bytecode. + if (entry.accessType === 'extendable' && cls.isFinal) { + warnings.push(`Class ${entry.className} is final - extendable may not work as expected`); } return { errors, warnings, suggestion }; } // --- Method entry --- if (entry.targetType === 'method' && entry.memberName) { - // Static initializers are intentionally not emitted by the AST walk. They - // are rare and genuinely unverifiable from decompiled source. - if (entry.memberName === '') { - warnings.push( - "'' (static initializer) cannot be validated from decompiled source — verify manually", - ); - return { errors, warnings, suggestion }; - } - - const isCtor = entry.memberName === ''; - // Only DECLARED members are seen here — call sites and comments are not, - // so a method that is merely called (but not declared) is correctly - // reported as missing. - const methodSyms = classSymbols.filter((s) => - isCtor - ? s.entryType === 'method' && s.isConstructor === true - : s.entryType === 'method' && s.isConstructor !== true && s.symbol === entry.memberName, - ); - - if (methodSyms.length === 0) { + const methods = cls.methods.filter((m) => m.name === entry.memberName); + if (methods.length === 0) { errors.push(`Method '${entry.memberName}' not found in ${entry.className}`); - const similar = findSimilarName(entry.memberName, uniqueMethodNames(classSymbols)); + const candidates = [ + ...new Set( + cls.methods + .filter((m) => m.name !== '' && m.name !== '') + .map((m) => m.name), + ), + ]; + const similar = findSimilarName(entry.memberName, candidates); if (similar) suggestion = `Did you mean: ${similar}?`; return { errors, warnings, suggestion }; } - // Validate the JVM descriptor against the actual signature. + // AW and bytecode descriptors are both erased, fully-qualified JVM form → + // exact match. if (entry.memberDescriptor) { - const descriptor = entry.memberDescriptor; - const matched = methodSyms.filter((m) => methodDescriptorMatches(m, descriptor).match); + const matched = methods.filter((m) => m.desc === entry.memberDescriptor); if (matched.length === 0) { - const found = methodSyms.map((m) => buildMethodDescriptor(m)).join(', '); + const found = methods.map((m) => m.desc).join(', '); errors.push( - `Method '${entry.memberName}' exists but no overload matches descriptor ${descriptor} (found: ${found})`, + `Method '${entry.memberName}' exists but no overload matches descriptor ${entry.memberDescriptor} (found: ${found})`, ); return { errors, warnings, suggestion }; } @@ -201,37 +138,27 @@ function validateEntryAgainstSymbols( // --- Field entry --- if (entry.targetType === 'field' && entry.memberName) { - const fieldSyms = classSymbols.filter( - (s) => s.entryType === 'field' && s.symbol === entry.memberName, - ); - - if (fieldSyms.length === 0) { + const field = cls.fields.find((f) => f.name === entry.memberName); + if (!field) { errors.push(`Field '${entry.memberName}' not found in ${entry.className}`); - const similar = findSimilarName(entry.memberName, uniqueFieldNames(classSymbols)); + const similar = findSimilarName( + entry.memberName, + cls.fields.map((f) => f.name), + ); if (similar) suggestion = `Did you mean: ${similar}?`; return { errors, warnings, suggestion }; } - const fieldSym = fieldSyms[0]; - - if (entry.memberDescriptor) { - if (!fieldDescriptorMatches(fieldSym, entry.memberDescriptor).match) { - const astDesc = javaTypeToDescriptor(fieldSym.fieldType ?? ''); - errors.push( - `Field '${entry.memberName}' descriptor mismatch: access widener says ${entry.memberDescriptor} but source declares ${fieldSym.fieldType ?? '?'} (${astDesc})`, - ); - return { errors, warnings, suggestion }; - } + if (entry.memberDescriptor && field.desc !== entry.memberDescriptor) { + errors.push( + `Field '${entry.memberName}' descriptor mismatch: access widener says ${entry.memberDescriptor} but bytecode declares ${field.desc}`, + ); + return { errors, warnings, suggestion }; } - // `mutable` on a field that is already non-final is a no-op → warn. Uses - // the AST modifier flag, replacing the fragile `(?!final)` regex (which - // silently missed generic-typed fields like `List items`). - if (entry.accessType === 'mutable') { - const isFinal = fieldSym.isFinal || fieldSym.modifiers?.includes('final') === true; - if (!isFinal) { - warnings.push(`Field '${entry.memberName}' appears to already be mutable`); - } + // `mutable` on a field that is already non-final is a no-op → warn. + if (entry.accessType === 'mutable' && !field.flags.includes('final')) { + warnings.push(`Field '${entry.memberName}' appears to already be mutable`); } return { errors, warnings, suggestion }; } @@ -239,22 +166,6 @@ function validateEntryAgainstSymbols( return { errors, warnings, suggestion }; } -/** - * Validate a single access-widener entry against a synthetic Java source - * string. Test seam for the descriptor-matching logic — it lets unit tests - * exercise validation without a decompiled Minecraft source tree. Production - * validation reads the class file, extracts symbols once (cached), and - * delegates to `validateEntryAgainstSymbols`. - * - * @internal - */ -export function validateEntryAgainstSource( - entry: AccessWidenerEntry, - source: string, -): { errors: string[]; warnings: string[]; suggestion?: string } { - return validateEntryAgainstSymbols(entry, extractJavaSymbols(source)); -} - // --------------------------------------------------------------------------- // Service // --------------------------------------------------------------------------- @@ -385,7 +296,11 @@ export class AccessWidenerService { } /** - * Validate access widener against Minecraft source + * Validate an access widener against the remapped Minecraft JAR's bytecode. + * + * Requires the version to have been decompiled (which also produces the + * remapped JAR this reads). The needed classes are resolved from bytecode once + * (cached), then the pure per-entry validation runs against `ClassBytecodeMap`. */ async validateAccessWidener( accessWidener: AccessWidener, @@ -397,46 +312,59 @@ export class AccessWidenerService { const cacheManager = getCacheManager(); - // Check if decompiled source exists - if (!cacheManager.hasDecompiledSource(mcVersion, mapping)) { + const fallbackEntry: AccessWidenerEntry = { + accessType: 'accessible', + targetType: 'class', + className: '', + line: 0, + }; + const firstEntry = accessWidener.entries[0] ?? fallbackEntry; + + // The remapped JAR (bytecode ground truth) is produced during decompilation. + if (!cacheManager.hasRemappedJar(mcVersion, mapping)) { errors.push({ - entry: accessWidener.entries[0] || { - accessType: 'accessible', - targetType: 'class', - className: '', - line: 0, - }, - message: `Minecraft ${mcVersion} source not decompiled. Run decompile_minecraft_version first.`, + entry: firstEntry, + message: `Minecraft ${mcVersion} (${mapping}) is not available locally. Run decompile_minecraft_version first.`, }); return { isValid: false, errors, warnings }; } - const decompiledPath = getDecompiledPath(mcVersion, mapping); - // Validate namespace matches if (accessWidener.namespace !== mapping && accessWidener.namespace !== 'named') { warnings.push({ - entry: accessWidener.entries[0] || { - accessType: 'accessible', - targetType: 'class', - className: '', - line: 0, - }, + entry: firstEntry, message: `Access widener namespace '${accessWidener.namespace}' may not match mapping '${mapping}'`, }); } - // Cache extracted symbols per source file for the duration of this call. - // Multiple entries targeting the same class parse the file only once. - const symbolsCache = new Map(); + const needed = new Set(); + for (const entry of accessWidener.entries) { + needed.add(toInternalName(entry.className)); + } + + let classMap: ClassBytecodeMap; + try { + classMap = await getBytecodeIndexService().getClassBytecode(mcVersion, mapping, [...needed]); + } catch (error) { + errors.push({ + entry: firstEntry, + message: `Failed to read bytecode for Minecraft ${mcVersion} (${mapping}): ${ + error instanceof Error ? error.message : 'Unknown error' + }`, + }); + return { isValid: false, errors, warnings }; + } - // Validate each entry for (const entry of accessWidener.entries) { - const validation = this.validateEntry(entry, decompiledPath, symbolsCache); + const validation = validateEntryAgainstBytecode(entry, classMap); errors.push( - ...validation.errors.map((e) => ({ entry, message: e, suggestion: validation.suggestion })), + ...validation.errors.map((message) => ({ + entry, + message, + suggestion: validation.suggestion, + })), ); - warnings.push(...validation.warnings.map((w) => ({ entry, message: w }))); + warnings.push(...validation.warnings.map((message) => ({ entry, message }))); } return { @@ -446,50 +374,6 @@ export class AccessWidenerService { }; } - /** - * Validate a single entry against the decompiled source tree. - * - * Reads the class file once (symbols cached per path within a single - * `validateAccessWidener` call) and delegates the pure symbol-based - * validation to `validateEntryAgainstSymbols`. - */ - private validateEntry( - entry: AccessWidenerEntry, - decompiledPath: string, - symbolsCache: Map, - ): { errors: string[]; warnings: string[]; suggestion?: string } { - const errors: string[] = []; - const warnings: string[] = []; - let suggestion: string | undefined; - - // JVM inner classes nest with `$`; the source lives in the outer class's - // .java file, so resolve the file path from the top-level class segment. - // (Class names literally containing `$` are an unresolved edge case; the - // ASM bytecode stage will make this authoritative.) - const outerClassName = entry.className.split('$')[0]; - const classPath = join(decompiledPath, `${outerClassName.replace(/\./g, '/')}.java`); - - if (!existsSync(classPath)) { - errors.push(`Class not found: ${entry.className}`); - - // Try to find similar classes (filesystem-based suggestion). - const similar = findSimilarClassFile(outerClassName, decompiledPath); - if (similar) { - suggestion = `Did you mean: ${similar}?`; - } - return { errors, warnings, suggestion }; - } - - let symbols = symbolsCache.get(classPath); - if (!symbols) { - const source = readFileSync(classPath, 'utf8'); - symbols = extractJavaSymbols(source); - symbolsCache.set(classPath, symbols); - } - - return validateEntryAgainstSymbols(entry, symbols); - } - /** * Generate an access widener file for common operations */ From e654b5aef84a244f6b0e21284965d757fce085c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 10:41:21 -0400 Subject: [PATCH 7/9] fix(validators): address Gemini round-2 review nits - 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 --- .../access-transformer-service.test.ts | 24 +++++++++++ src/services/access-transformer-service.ts | 42 ++++++++++++++++--- src/services/access-widener-service.ts | 32 +++++++++++--- src/services/bytecode-index-service.ts | 31 +++++++++++++- 4 files changed, 116 insertions(+), 13 deletions(-) diff --git a/__tests__/services/access-transformer-service.test.ts b/__tests__/services/access-transformer-service.test.ts index 312b18b..a13bf05 100644 --- a/__tests__/services/access-transformer-service.test.ts +++ b/__tests__/services/access-transformer-service.test.ts @@ -409,6 +409,30 @@ describe('Access Transformer Validation (bytecode + descriptor matching)', () => expect(res.errors.some((e) => e.includes("Class 'net.test.Ghost' not found"))).toBe(true); }); + it('suggests a similar class name from the JAR-wide pool (same package)', () => { + // The loaded classMap only holds the (missing) target; the suggestion must + // come from the wider JAR pool, not just the classes this file referenced. + const res = validateEntryAgainstBytecode( + makeEntry({ memberType: 'class', className: 'net.test.Blok' }), + mapOf(bcClass({ name: 'net/test/Unrelated' })), // classMap has no good candidate + undefined, + ['net/test/Block', 'net/test/Blocks', 'net/other/Blok'], + ); + expect(res.suggestion).toContain('Block'); + }); + + it('does not suggest a similar class from a different package', () => { + // `Bloc` is edit-distance 1 from `Block`, but only in a different package — + // package-scoped suggestions must not cross package boundaries. + const res = validateEntryAgainstBytecode( + makeEntry({ memberType: 'class', className: 'net.test.Bloc' }), + mapOf(bcClass({ name: 'net/test/Unrelated' })), + undefined, + ['net/other/Block', 'net/deep/nested/Block'], + ); + expect(res.suggestion).toBeUndefined(); + }); + it('warns on override-narrowing for an overridable method but not a final one', () => { const base = bcClass({ name: 'net/test/Base', diff --git a/src/services/access-transformer-service.ts b/src/services/access-transformer-service.ts index f0af5ff..a9073ab 100644 --- a/src/services/access-transformer-service.ts +++ b/src/services/access-transformer-service.ts @@ -190,6 +190,24 @@ function simpleClassName(internal: string): string { return dollar >= 0 ? tail.slice(dollar + 1) : tail; } +/** Package (path portion) of an internal class name, or '' for the default package. */ +function packageOf(internal: string): string { + const slash = internal.lastIndexOf('/'); + return slash >= 0 ? internal.slice(0, slash) : ''; +} + +/** + * Pick the best "did you mean" class name for a missing target. Draws from + * `pool` (JAR-wide internal names) restricted to the SAME package as the target, + * so suggestions stay relevant and bounded — a typo'd package yields nothing + * rather than a misleading match three packages away. + */ +function suggestClassName(targetInternal: string, pool: string[]): string | null { + const pkg = packageOf(targetInternal); + const samePackage = pool.filter((n) => packageOf(n) === pkg).map(simpleClassName); + return findSimilarName(simpleClassName(targetInternal), samePackage); +} + /** * Is a method overridable (subject to the override-narrowing gotcha, quirk 6.3)? * Constructors and static initializers are NOT overridable — a constructor is @@ -235,6 +253,7 @@ export function validateEntryAgainstBytecode( entry: AccessTransformerEntry, classMap: ClassBytecodeMap, allEntries?: AccessTransformerEntry[], + classSuggestionPool?: string[], ): { errors: string[]; warnings: string[]; suggestion?: string } { const errors: string[] = []; const warnings: string[] = []; @@ -247,10 +266,10 @@ export function validateEntryAgainstBytecode( // files, so `Outer$NonExistent` simply has no entry and is caught here. if (!cls) { errors.push(`Class '${entry.className}' not found`); - const similar = findSimilarName( - simpleClassName(internal), - [...classMap.keys()].map(simpleClassName), - ); + // Suggest from the JAR-wide class list (same package) when the caller + // supplies it; fall back to the loaded classMap for pure/unit callers. + const pool = classSuggestionPool ?? [...classMap.keys()]; + const similar = suggestClassName(internal, pool); if (similar) suggestion = `Did you mean a class named: ${similar}?`; return { errors, warnings, suggestion }; } @@ -680,8 +699,10 @@ export class AccessTransformerService { for (const entry of accessTransformer.entries) { const internal = entry.className.replace(/\./g, '/'); needed.add(internal); + // `idx > 0` (not `>= 0`): a leading `$` would slice to '' and add an empty + // name. Normal `Outer$Inner` nesting has its `$` at a positive index. let idx = internal.lastIndexOf('$'); - while (idx >= 0) { + while (idx > 0) { const enclosing = internal.slice(0, idx); needed.add(enclosing); idx = enclosing.lastIndexOf('$'); @@ -701,9 +722,18 @@ export class AccessTransformerService { return { isValid: false, errors, warnings }; } + // JAR-wide class list for same-package "did you mean" suggestions on + // class-not-found errors. Central-directory scan only — no bytecode dumped. + const suggestionPool = getBytecodeIndexService().listClassNames(mcVersion, mapping); + // Validate each entry against bytecode. for (const entry of accessTransformer.entries) { - const validation = validateEntryAgainstBytecode(entry, classMap, accessTransformer.entries); + const validation = validateEntryAgainstBytecode( + entry, + classMap, + accessTransformer.entries, + suggestionPool, + ); errors.push( ...validation.errors.map((message) => ({ entry, diff --git a/src/services/access-widener-service.ts b/src/services/access-widener-service.ts index 704f484..6922e8f 100644 --- a/src/services/access-widener-service.ts +++ b/src/services/access-widener-service.ts @@ -51,6 +51,23 @@ function simpleClassName(internal: string): string { return dollar >= 0 ? tail.slice(dollar + 1) : tail; } +/** Package (path portion) of an internal class name, or '' for the default package. */ +function packageOf(internal: string): string { + const slash = internal.lastIndexOf('/'); + return slash >= 0 ? internal.slice(0, slash) : ''; +} + +/** + * Pick the best "did you mean" class name for a missing target, drawn from + * `pool` (JAR-wide internal names) restricted to the target's own package so + * suggestions stay relevant and bounded. + */ +function suggestClassName(targetInternal: string, pool: string[]): string | null { + const pkg = packageOf(targetInternal); + const samePackage = pool.filter((n) => packageOf(n) === pkg).map(simpleClassName); + return findSimilarName(simpleClassName(targetInternal), samePackage); +} + /** * Render a parsed entry back to its one-line AW directive text (e.g. * `accessible method net/minecraft/Foo tick ()V`). Used to keep tool output @@ -77,6 +94,7 @@ export function accessWidenerEntryToString(entry: AccessWidenerEntry): string { export function validateEntryAgainstBytecode( entry: AccessWidenerEntry, classMap: ClassBytecodeMap, + classSuggestionPool?: string[], ): { errors: string[]; warnings: string[]; suggestion?: string } { const errors: string[] = []; const warnings: string[] = []; @@ -86,10 +104,10 @@ export function validateEntryAgainstBytecode( const cls = classMap.get(internal); if (!cls) { errors.push(`Class not found: ${entry.className}`); - const similar = findSimilarName( - simpleClassName(internal), - [...classMap.keys()].map(simpleClassName), - ); + // Suggest from the JAR-wide class list (same package) when the caller + // supplies it; fall back to the loaded classMap for pure/unit callers. + const pool = classSuggestionPool ?? [...classMap.keys()]; + const similar = suggestClassName(internal, pool); if (similar) suggestion = `Did you mean a class named: ${similar}?`; return { errors, warnings, suggestion }; } @@ -355,8 +373,12 @@ export class AccessWidenerService { return { isValid: false, errors, warnings }; } + // JAR-wide class list for same-package "did you mean" suggestions on + // class-not-found errors. Central-directory scan only — no bytecode dumped. + const suggestionPool = getBytecodeIndexService().listClassNames(mcVersion, mapping); + for (const entry of accessWidener.entries) { - const validation = validateEntryAgainstBytecode(entry, classMap); + const validation = validateEntryAgainstBytecode(entry, classMap, suggestionPool); errors.push( ...validation.errors.map((message) => ({ entry, diff --git a/src/services/bytecode-index-service.ts b/src/services/bytecode-index-service.ts index f7fd677..38be741 100644 --- a/src/services/bytecode-index-service.ts +++ b/src/services/bytecode-index-service.ts @@ -82,7 +82,10 @@ export class BytecodeIndexService { let cache = this.loadCache(cachePath, signature); const wanted = [...new Set(internalNames)]; - const missing = wanted.filter((n) => !(n in cache.classes)); + // Object.hasOwn (not `n in`) so an internal name colliding with an + // Object.prototype key (`toString`, `constructor`, …) can't be mistaken for + // an already-cached entry and silently skipped. + const missing = wanted.filter((n) => !Object.hasOwn(cache.classes, n)); if (missing.length > 0) { const dumped = await this.dumpClasses(jarPath, missing); @@ -106,6 +109,23 @@ export class BytecodeIndexService { return result; } + /** + * List every class's internal name in the remapped JAR via a central-directory + * scan (no bytecode dumped). Used to build package-scoped "did you mean" + * suggestion pools for class-not-found errors. Returns `[]` when the JAR is + * absent — suggestions are a nicety, never a hard dependency. + */ + listClassNames(version: string, mapping: MappingType): string[] { + const jarPath = getRemappedJarPath(version, mapping); + if (!existsSync(jarPath)) return []; + const names: string[] = []; + for (const entry of new AdmZip(jarPath).getEntries()) { + const n = entry.entryName; + if (n.endsWith('.class')) names.push(n.slice(0, -'.class'.length)); + } + return names; + } + /** Delete the sidecar cache for a (version, mapping). Used by force-clear paths. */ clearCache(version: string, mapping: MappingType): void { const cachePath = cachePathFor(getRemappedJarPath(version, mapping)); @@ -143,12 +163,19 @@ export class BytecodeIndexService { /** Persist the cache atomically (write-temp + rename) so a crash cannot truncate it. */ private saveCache(cachePath: string, cache: BytecodeCacheFile): void { + const tmp = `${cachePath}.${process.pid}.tmp`; try { - const tmp = `${cachePath}.${process.pid}.tmp`; writeFileSync(tmp, JSON.stringify(cache), 'utf8'); renameSync(tmp, cachePath); } catch (error) { // A failed cache write is non-fatal — validation still works, just uncached. + // Clean up the temp file if the rename never happened, so a failed write + // doesn't leave an orphan next to the cache. + try { + if (existsSync(tmp)) unlinkSync(tmp); + } catch { + // best-effort + } logger.debug(`Failed to persist bytecode cache ${cachePath}: ${String(error)}`); } } From bc3bfb728e5342cab18e26c2c88589a7b3ba3d5e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 11:41:12 -0400 Subject: [PATCH 8/9] chore: enforce lint in CI and bump v1.2.5 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. --- .github/workflows/test.yml | 5 +++++ biome.json | 7 ++++++- package-lock.json | 4 ++-- package.json | 11 ++--------- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3168296..1dc2afc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,6 +8,7 @@ on: - '__tests__/**' - '*.ts' - '*.js' + - 'biome.json' - 'package.json' - 'package-lock.json' - 'tsconfig.json' @@ -20,6 +21,7 @@ on: - '__tests__/**' - '*.ts' - '*.js' + - 'biome.json' - 'package.json' - 'package-lock.json' - 'tsconfig.json' @@ -75,6 +77,9 @@ jobs: - name: Build run: npm run build + - name: Run linter + run: npm run lint + # Isolate the heavy decompile pipeline into its own step so it is not racing # the vitest test timeout. Idempotent: a fast no-op when the cache above was # restored (hasDecompiledSource -> true); runs the full pipeline only on a diff --git a/biome.json b/biome.json index 531ad82..795ffe6 100644 --- a/biome.json +++ b/biome.json @@ -1,7 +1,12 @@ { "$schema": "https://biomejs.dev/schemas/1.9.4/schema.json", + "vcs": { + "enabled": true, + "clientKind": "git", + "useIgnoreFile": true + }, "files": { - "ignore": ["ai_reference", "node_modules", "dist"] + "ignore": ["ai_reference", "node_modules", "dist", ".claude", "docs"] }, "organizeImports": { "enabled": true diff --git a/package-lock.json b/package-lock.json index 343ccfd..bd71d20 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@mcdxai/minecraft-dev-mcp", - "version": "1.2.4", + "version": "1.2.5", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@mcdxai/minecraft-dev-mcp", - "version": "1.2.4", + "version": "1.2.5", "license": "MIT", "dependencies": { "@keqingmoe/tree-sitter": "^0.26.2", diff --git a/package.json b/package.json index bad2fbc..91d88dd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@mcdxai/minecraft-dev-mcp", - "version": "1.2.4", + "version": "1.2.5", "description": "MCP server and CLI for Minecraft mod development - decompile, remap, and explore Minecraft source code", "type": "module", "main": "./dist/index.js", @@ -43,14 +43,7 @@ "lint:fix": "biome check --write .", "clean": "rimraf dist" }, - "keywords": [ - "minecraft", - "fabric", - "mod-development", - "mcp", - "decompiler", - "mappings" - ], + "keywords": ["minecraft", "fabric", "mod-development", "mcp", "decompiler", "mappings"], "author": "GhostTypes", "license": "MIT", "repository": { From 1d391e67ad15552e13a47ebbb349ee879e13510f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 18:27:42 -0400 Subject: [PATCH 9/9] fix: register compiled patched JARs as remapped JARs so AT/AW validation works --- CLAUDE.md | 2 +- __tests__/manual/patched/patched-jar.test.ts | 32 ++++ .../services/patched-jar-bytecode.test.ts | 169 ++++++++++++++++++ docs/specs/access-transformer-support.md | 7 + src/services/access-transformer-service.ts | 19 +- src/services/access-widener-service.ts | 22 ++- src/services/bytecode-index-service.ts | 22 +++ src/services/decompile-service.ts | 68 ++++++- 8 files changed, 325 insertions(+), 16 deletions(-) create mode 100644 __tests__/services/patched-jar-bytecode.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 794c241..5315306 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -329,7 +329,7 @@ Deterministic central-directory scan: any `.class` entry → decompile via VineF 8. **`compare_versions`** - Compare classes/registries between versions 9. **`analyze_mixin`** - Analyze and validate Mixin code 10. **`validate_access_widener`** - Validate access widener files -10a. **`validate_access_transformer`** - Validate Forge/NeoForge access transformer files (parses `.cfg`, checks signatures, flags record canonical-constructor widening, inner-class accessibility, and conflicting modifiers; mojmap-only). Validates against the **remapped JAR's bytecode** (via the bundled ASM `bytecode-dumper`), not decompiled `.java` — decompiled source omits compiler-generated record members (canonical constructors, component accessors), so a source-based check false-flags them as missing (issue #12). Bytecode carries every member with its true access flags and erased descriptors (the same facts `javap` shows). Only needs the remapped JAR, which `decompile_minecraft_version` produces; per-class results are cached in a `remapped/{version}-{mapping}.bytecode.json` sidecar keyed by JAR signature. +10a. **`validate_access_transformer`** - Validate Forge/NeoForge access transformer files (parses `.cfg`, checks signatures, flags record canonical-constructor widening, inner-class accessibility, and conflicting modifiers; mojmap-only). Validates against the **remapped JAR's bytecode** (via the bundled ASM `bytecode-dumper`), not decompiled `.java` — decompiled source omits compiler-generated record members (canonical constructors, component accessors), so a source-based check false-flags them as missing (issue #12). Bytecode carries every member with its true access flags and erased descriptors (the same facts `javap` shows). Only needs the remapped JAR, which `decompile_minecraft_version` produces; per-class results are cached in a `remapped/{version}-{mapping}.bytecode.json` sidecar keyed by JAR signature. **Patched keys work too**: the patched flow does no remapping, so `decompileLocalJar` copies a *compiled* input JAR to `remapped/{version}-{mapping}.jar` — it already is that key's remapped JAR by definition — and every bytecode consumer then resolves a patched key exactly like a vanilla one. Patched *sources* JARs (NFRT/ForgeGradle `-sources.jar`) contain no bytecode and cannot be validated; the tool says so rather than telling you to decompile again. 11. **`compare_versions_detailed`** - AST-level version comparison 12. **`index_minecraft_version`** - Create full-text search index 13. **`search_indexed`** - Fast FTS5 search on indexed versions diff --git a/__tests__/manual/patched/patched-jar.test.ts b/__tests__/manual/patched/patched-jar.test.ts index e95c3eb..ca5b696 100644 --- a/__tests__/manual/patched/patched-jar.test.ts +++ b/__tests__/manual/patched/patched-jar.test.ts @@ -4,8 +4,10 @@ import { beforeAll, describe, expect, it } from 'vitest'; import { getCacheManager } from '../../../src/cache/cache-manager.js'; import { verifyJavaVersion } from '../../../src/java/java-process.js'; import { handleCompareVersions } from '../../../src/server/tools.js'; +import { getAccessTransformerService } from '../../../src/services/access-transformer-service.js'; import { getDecompileService } from '../../../src/services/decompile-service.js'; import { getSearchIndexService } from '../../../src/services/search-index-service.js'; +import { inspectJar } from '../../../src/utils/jar-inspector.js'; import { getDecompiledPath } from '../../../src/utils/paths.js'; import { LOADER_PACKAGE_PREFIX, @@ -113,6 +115,36 @@ describePatched(`Manual: Patched MC JAR pipeline (${PATCHED_VERSION || 'no-versi }, 30000); }); + describe('Access transformer validation (bytecode ground truth)', () => { + // The AT/AW validators read the REMAPPED JAR, which the patched flow does + // not produce by remapping — decompileLocalJar registers the input JAR as + // this key's remapped JAR instead. Without that, a NeoForge user gets + // "run decompile_minecraft_version first" right after decompiling. + const isSourcesJar = () => inspectJar(PATCHED_JAR_PATH as string).type === 'sources'; + + it("registers the patched JAR as the version key's remapped JAR", () => { + if (isSourcesJar()) return; // no bytecode exists in a sources-JAR flow + expect(getCacheManager().hasRemappedJar(PATCHED_VERSION, PATCHED_MAPPING)).toBe(true); + }, 30000); + + it('validates an AT against the patched key without a "not available" error', async () => { + const svc = getAccessTransformerService(); + // A class every patched MC JAR has, widened at class level only. + const at = svc.parseAccessTransformer('public net.minecraft.world.entity.Entity\n'); + const validation = await svc.validateAccessTransformer(at, PATCHED_VERSION, PATCHED_MAPPING); + + if (isSourcesJar()) { + // Sources JARs genuinely cannot be validated — but the message must + // explain that, not loop the user back to decompiling. + expect(validation.errors[0]?.message).toContain('sources JAR'); + return; + } + + expect(validation.errors.map((e) => e.message)).toEqual([]); + expect(validation.isValid).toBe(true); + }, 120000); + }); + describe('Source retrieval', () => { it('get_minecraft_source returns Entity from the patched JAR', async () => { const decompileService = getDecompileService(); diff --git a/__tests__/services/patched-jar-bytecode.test.ts b/__tests__/services/patched-jar-bytecode.test.ts new file mode 100644 index 0000000..753814a --- /dev/null +++ b/__tests__/services/patched-jar-bytecode.test.ts @@ -0,0 +1,169 @@ +import { copyFileSync, existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import AdmZip from 'adm-zip'; +import { afterEach, describe, expect, it } from 'vitest'; +import { getDatabase } from '../../src/cache/database.js'; +import { getAccessTransformerService } from '../../src/services/access-transformer-service.js'; +import { bytecodeUnavailableMessage } from '../../src/services/bytecode-index-service.js'; +import { DecompileService } from '../../src/services/decompile-service.js'; +import { ensureDir } from '../../src/utils/file-utils.js'; +import { getDecompiledPath, getRemappedJarPath } from '../../src/utils/paths.js'; + +/** + * Patched Forge/NeoForge JARs must stay validatable after the switch to + * bytecode ground truth. + * + * The bytecode validators gate on `hasRemappedJar`, but the patched-JAR flow + * (`decompileLocalJar`) does no remapping — so unless the input JAR is + * registered as that version key's remapped JAR, `validate_access_transformer` + * tells a NeoForge user to "run decompile_minecraft_version first" immediately + * after they did exactly that. These tests pin the registration and the + * sources-JAR fallback message. + * + * Deliberately Java-free: the branches exercised here (already-decompiled + * backfill, sources extraction) never invoke VineFlower, so this runs in default + * CI. The full compiled decompile → register → validate round trip lives in the + * manual patched suite. + */ + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const FIXTURE_JAR = join(__dirname, '..', 'fixtures', 'summoningrituals-mc-stubs.jar'); +const DUMPER_JAR = join( + __dirname, + '..', + '..', + 'tools', + 'bytecode-dumper', + 'build', + 'libs', + 'bytecode-dumper-1.0.0.jar', +); +// Only the round-trip validation needs the ASM dumper (and thus Java); the +// registration and message tests are pure filesystem. +const itWithDumper = existsSync(DUMPER_JAR) ? it : it.skip; + +// Conventional patched cache key (`--`) — the point +// is that an opaque key flows through the bytecode path like any vanilla one. +const PATCHED_VERSION = '0.0.0-neoforge-patched-test'; +const MAPPING = 'mojmap' as const; + +describe('patched JAR bytecode availability (Forge/NeoForge)', () => { + const staged: string[] = []; + const versions = new Set(); + + function track(path: string): string { + staged.push(path); + return path; + } + + afterEach(() => { + for (const path of staged.splice(0)) { + rmSync(path, { recursive: true, force: true }); + } + // decompileLocalJar writes a decompile_jobs row; drop it so these throwaway + // keys don't accumulate in the shared cache DB. + for (const version of versions) { + try { + getDatabase().deleteJob(version, MAPPING); + } catch { + // best-effort + } + } + versions.clear(); + }); + + /** Pretend this version key was already decompiled (no VineFlower needed). */ + function stageDecompiledSource(version: string): void { + versions.add(version); + const dir = track(getDecompiledPath(version, MAPPING)); + mkdirSync(join(dir, 'net', 'minecraft'), { recursive: true }); + writeFileSync(join(dir, 'net', 'minecraft', 'Marker.java'), 'class Marker {}\n', 'utf8'); + } + + /** A sources-only JAR, as NFRT/ForgeGradle `-sources.jar` produces. */ + function makeSourcesJar(): string { + const zip = new AdmZip(); + zip.addFile('net/minecraft/Foo.java', Buffer.from('package net.minecraft;\nclass Foo {}\n')); + const path = track(join(dirname(FIXTURE_JAR), 'tmp-patched-sources.jar')); + zip.writeZip(path); + return path; + } + + it("registers a compiled patched JAR as the version key's remapped JAR", async () => { + stageDecompiledSource(PATCHED_VERSION); + const remapped = track(getRemappedJarPath(PATCHED_VERSION, MAPPING)); + ensureDir(dirname(remapped)); + rmSync(remapped, { force: true }); + + // Source tree already present -> the "already decompiled" fast path, which + // must still backfill the remapped JAR (caches built before this existed). + const result = await new DecompileService().decompileLocalJar( + FIXTURE_JAR, + PATCHED_VERSION, + MAPPING, + ); + + expect(result.mode).toBe('decompiled'); + expect(existsSync(remapped)).toBe(true); + // It is the patched JAR itself — no remapping happens in this flow. + expect(new AdmZip(remapped).getEntries().length).toBe( + new AdmZip(FIXTURE_JAR).getEntries().length, + ); + }); + + itWithDumper( + 'validates an access transformer against a patched version key end to end', + async () => { + // Registration is what makes this work: stage the JAR exactly where + // decompileLocalJar puts it, then validate using the opaque patched key. + const remapped = track(getRemappedJarPath(PATCHED_VERSION, MAPPING)); + ensureDir(dirname(remapped)); + copyFileSync(FIXTURE_JAR, remapped); + track(remapped.replace(/\.jar$/i, '.bytecode.json')); + + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformer( + 'public net.minecraft.world.level.storage.loot.IntRange\n', + ); + const validation = await svc.validateAccessTransformer(at, PATCHED_VERSION, MAPPING); + + // The pre-fix behaviour was a hard "not available locally" error here. + expect(validation.errors.map((e) => e.message)).toEqual([]); + expect(validation.isValid).toBe(true); + }, + 60000, + ); + + it('does not register a sources JAR and explains why validation is unavailable', async () => { + const version = `${PATCHED_VERSION}-sources`; + versions.add(version); + const sourcesJar = makeSourcesJar(); + track(getDecompiledPath(version, MAPPING)); + const remapped = track(getRemappedJarPath(version, MAPPING)); + + const result = await new DecompileService().decompileLocalJar(sourcesJar, version, MAPPING); + + expect(result.mode).toBe('extracted'); + // Nothing to register — a sources JAR has no bytecode at all. + expect(existsSync(remapped)).toBe(false); + + // The user must NOT be told to re-run decompile: they already did, and + // re-running cannot produce bytecode from a sources JAR. + const message = bytecodeUnavailableMessage(version, MAPPING); + expect(message).toContain('sources JAR'); + expect(message).not.toContain('Run decompile_minecraft_version first'); + + const svc = getAccessTransformerService(); + const at = svc.parseAccessTransformer('public net.minecraft.Foo\n'); + const validation = await svc.validateAccessTransformer(at, version, MAPPING); + expect(validation.isValid).toBe(false); + expect(validation.errors[0]?.message).toBe(message); + }); + + it('still says "run decompile first" when the version was never processed', () => { + const message = bytecodeUnavailableMessage('0.0.0-never-processed', MAPPING); + expect(message).toContain('Run decompile_minecraft_version first'); + expect(message).not.toContain('sources JAR'); + }); +}); diff --git a/docs/specs/access-transformer-support.md b/docs/specs/access-transformer-support.md index 9c085d1..e12eb71 100644 --- a/docs/specs/access-transformer-support.md +++ b/docs/specs/access-transformer-support.md @@ -29,6 +29,13 @@ so it sees exactly what the AT is applied to at load time — the same facts than the class's new access and no file directive widens it. - **Constructors are never "overridable"** — the override-narrowing warning (§6.3) now excludes ``/`` (a prior false positive on `AnyOfCondition`). +- **Patched Forge/NeoForge keys keep working.** The bytecode path resolves the + remapped JAR, but `decompileLocalJar` never remaps — so a *compiled* patched + JAR is registered (copied) as `remapped/{version}-{mapping}.jar`, which it + already is by definition (the caller asserts it is in `mapping`). Downstream + code needs no "is this patched?" branch. A patched *sources* JAR has no + bytecode at all; the validators detect that case and say so instead of looping + the user back to `decompile_minecraft_version`. - Needs only the **remapped JAR** (produced by `decompile_minecraft_version`); results are cached in a `remapped/{version}-{mapping}.bytecode.json` sidecar keyed by the JAR's size+mtime signature, so the cache is always fresh and diff --git a/src/services/access-transformer-service.ts b/src/services/access-transformer-service.ts index a9073ab..525d1d5 100644 --- a/src/services/access-transformer-service.ts +++ b/src/services/access-transformer-service.ts @@ -42,7 +42,7 @@ import { descriptorToReadable as sharedDescriptorToReadable } from '../utils/des import { AccessTransformerParseError } from '../utils/errors.js'; import { logger } from '../utils/logger.js'; import { findSimilarName } from '../utils/suggestions.js'; -import { getBytecodeIndexService } from './bytecode-index-service.js'; +import { bytecodeUnavailableMessage, getBytecodeIndexService } from './bytecode-index-service.js'; /** A map of internal class name (slashes, `$`) → its authoritative bytecode metadata. */ export type ClassBytecodeMap = Map; @@ -688,7 +688,7 @@ export class AccessTransformerService { if (!cacheManager.hasRemappedJar(mcVersion, mapping)) { errors.push({ entry: firstEntry, - message: `Minecraft ${mcVersion} (${mapping}) is not available locally. Run decompile_minecraft_version first.`, + message: bytecodeUnavailableMessage(mcVersion, mapping), }); return { isValid: false, errors, warnings }; } @@ -723,8 +723,15 @@ export class AccessTransformerService { } // JAR-wide class list for same-package "did you mean" suggestions on - // class-not-found errors. Central-directory scan only — no bytecode dumped. - const suggestionPool = getBytecodeIndexService().listClassNames(mcVersion, mapping); + // class-not-found errors. Built LAZILY: it reads the whole JAR's central + // directory, and a clean AT (the common case) never needs it. + let suggestionPool: string[] | undefined; + const getSuggestionPool = (): string[] => { + if (!suggestionPool) { + suggestionPool = getBytecodeIndexService().listClassNames(mcVersion, mapping); + } + return suggestionPool; + }; // Validate each entry against bytecode. for (const entry of accessTransformer.entries) { @@ -732,7 +739,9 @@ export class AccessTransformerService { entry, classMap, accessTransformer.entries, - suggestionPool, + // Only a missing class needs the pool, and that is exactly when the + // class is absent from classMap — so resolve it only then. + classMap.has(entry.className.replace(/\./g, '/')) ? undefined : getSuggestionPool(), ); errors.push( ...validation.errors.map((message) => ({ diff --git a/src/services/access-widener-service.ts b/src/services/access-widener-service.ts index 6922e8f..e741c40 100644 --- a/src/services/access-widener-service.ts +++ b/src/services/access-widener-service.ts @@ -33,7 +33,7 @@ import { descriptorToReadable as sharedDescriptorToReadable } from '../utils/des import { AccessWidenerParseError } from '../utils/errors.js'; import { logger } from '../utils/logger.js'; import { findSimilarName } from '../utils/suggestions.js'; -import { getBytecodeIndexService } from './bytecode-index-service.js'; +import { bytecodeUnavailableMessage, getBytecodeIndexService } from './bytecode-index-service.js'; /** A map of internal class name (slashes, `$`) → its authoritative bytecode metadata. */ type ClassBytecodeMap = Map; @@ -342,7 +342,7 @@ export class AccessWidenerService { if (!cacheManager.hasRemappedJar(mcVersion, mapping)) { errors.push({ entry: firstEntry, - message: `Minecraft ${mcVersion} (${mapping}) is not available locally. Run decompile_minecraft_version first.`, + message: bytecodeUnavailableMessage(mcVersion, mapping), }); return { isValid: false, errors, warnings }; } @@ -374,11 +374,23 @@ export class AccessWidenerService { } // JAR-wide class list for same-package "did you mean" suggestions on - // class-not-found errors. Central-directory scan only — no bytecode dumped. - const suggestionPool = getBytecodeIndexService().listClassNames(mcVersion, mapping); + // class-not-found errors. Built LAZILY: it reads the whole JAR's central + // directory, and a clean access widener never needs it. + let suggestionPool: string[] | undefined; + const getSuggestionPool = (): string[] => { + if (!suggestionPool) { + suggestionPool = getBytecodeIndexService().listClassNames(mcVersion, mapping); + } + return suggestionPool; + }; for (const entry of accessWidener.entries) { - const validation = validateEntryAgainstBytecode(entry, classMap, suggestionPool); + const validation = validateEntryAgainstBytecode( + entry, + classMap, + // Only a missing class needs the pool — resolve it only then. + classMap.has(toInternalName(entry.className)) ? undefined : getSuggestionPool(), + ); errors.push( ...validation.errors.map((message) => ({ entry, diff --git a/src/services/bytecode-index-service.ts b/src/services/bytecode-index-service.ts index 38be741..81dd210 100644 --- a/src/services/bytecode-index-service.ts +++ b/src/services/bytecode-index-service.ts @@ -29,11 +29,33 @@ import { existsSync, readFileSync, renameSync, statSync, unlinkSync, writeFileSy import { tmpdir } from 'node:os'; import { join } from 'node:path'; import AdmZip from 'adm-zip'; +import { getCacheManager } from '../cache/cache-manager.js'; import { type BytecodeClass, getBytecodeDumper } from '../java/bytecode-dumper.js'; import type { MappingType } from '../types/minecraft.js'; import { logger } from '../utils/logger.js'; import { getRemappedJarPath } from '../utils/paths.js'; +/** + * Explain why bytecode is unavailable for a (version, mapping), for the + * validators to surface verbatim. + * + * Two genuinely different situations, and telling them apart matters: if the + * version was never processed, "run decompile" is the fix; if it WAS processed + * but from a sources JAR (NFRT / ForgeGradle `-sources.jar`), there is no + * bytecode anywhere in that flow and re-running decompile changes nothing — the + * user needs a different input JAR or a vanilla version. Telling them to + * "decompile first" in that case sends them in a loop. + */ +export function bytecodeUnavailableMessage(version: string, mapping: MappingType): string { + if (getCacheManager().hasDecompiledSource(version, mapping)) { + const why = 'was decompiled from a sources JAR, which contains no bytecode'; + const fix = + 'Re-run decompile_minecraft_version with a COMPILED patched JAR (one containing .class entries), or validate against the vanilla Minecraft version instead.'; + return `Minecraft ${version} (${mapping}) ${why} — validation needs compiled classes. ${fix}`; + } + return `Minecraft ${version} (${mapping}) is not available locally. Run decompile_minecraft_version first.`; +} + /** On-disk cache shape. `classes` maps an internal name → metadata, or `null` (known-absent). */ interface BytecodeCacheFile { /** `${size}:${mtimeMs}` of the remapped JAR the cache was built from. */ diff --git a/src/services/decompile-service.ts b/src/services/decompile-service.ts index 9917f9b..3afa835 100644 --- a/src/services/decompile-service.ts +++ b/src/services/decompile-service.ts @@ -1,13 +1,14 @@ -import { existsSync, readFileSync, rmSync } from 'node:fs'; -import { join } from 'node:path'; +import { copyFileSync, existsSync, readFileSync, rmSync } from 'node:fs'; +import { join, resolve } from 'node:path'; import { getCacheManager } from '../cache/cache-manager.js'; import { getDatabase } from '../cache/database.js'; import { getVineflower } from '../java/vineflower.js'; import type { MappingType } from '../types/minecraft.js'; import { ClassNotFoundError, DecompilationError } from '../utils/errors.js'; +import { ensureDir } from '../utils/file-utils.js'; import { extractSourcesJar, inspectJar } from '../utils/jar-inspector.js'; import { logger } from '../utils/logger.js'; -import { classNameToPath, getDecompiledPath } from '../utils/paths.js'; +import { classNameToPath, getDecompiledPath, getRemappedJarPath, paths } from '../utils/paths.js'; import { getBytecodeIndexService } from './bytecode-index-service.js'; import { getRemapService } from './remap-service.js'; import { getSearchIndexService } from './search-index-service.js'; @@ -90,6 +91,11 @@ export class DecompileService { * Sources JARs (no .class entries) are extracted directly. Compiled JARs * (and mixed JARs that contain any .class) are run through VineFlower. * No remapping is performed — caller asserts the JAR is already in `mapping`. + * + * A COMPILED input JAR is also registered as this version key's remapped JAR + * (see `registerAsRemappedJar`), which is what keeps the bytecode-backed + * validators (`validate_access_transformer` / `validate_access_widener`) + * working for patched Forge/NeoForge versions. */ async decompileLocalJar( jarPath: string, @@ -105,6 +111,15 @@ export class DecompileService { if (this.cache.hasDecompiledSource(version, mapping)) { logger.info(`${version}/${mapping} already present; skipping (use force to re-run)`); + // Backfill the remapped JAR for caches decompiled before it was registered + // here — otherwise the bytecode validators stay broken for this key until + // the user force re-decompiles. + if ( + !existsSync(getRemappedJarPath(version, mapping)) && + inspectJar(jarPath).type === 'compiled' + ) { + this.registerAsRemappedJar(jarPath, version, mapping); + } return { outputDir, mode: 'decompiled' }; } @@ -127,6 +142,9 @@ export class DecompileService { if (onProgress) onProgress(written, written); this.cache.completeJob(jobId); logger.info(`Sources extraction complete: ${outputDir}`); + // No bytecode exists anywhere in this flow, so there is nothing to + // register as a remapped JAR — the bytecode validators detect the + // sources-only case and say so explicitly. return { outputDir, mode: 'extracted' }; } @@ -149,6 +167,9 @@ export class DecompileService { }); this.cache.completeJob(jobId); + // Register AFTER a successful decompile so a failed run leaves no JAR + // claiming to back a source tree that does not exist. + this.registerAsRemappedJar(jarPath, version, mapping); logger.info(`Local JAR decompilation complete: ${outputDir}`); return { outputDir, mode: 'decompiled' }; } catch (error) { @@ -157,6 +178,41 @@ export class DecompileService { } } + /** + * Register a compiled local JAR as the remapped JAR for its version key. + * + * The patched-JAR flow does no remapping — the caller asserts the JAR is + * already in `mapping` — so the input JAR IS this version's remapped JAR by + * definition. Copying it under `remapped/{version}-{mapping}.jar` means every + * bytecode consumer (`cache-manager.hasRemappedJar`, `BytecodeIndexService`, + * and so the AT/AW validators) resolves a patched key through exactly the same + * path as a vanilla one — no branching on "is this patched?" anywhere + * downstream, matching how the opaque version key flows through the rest of + * the system. + * + * We copy rather than symlink/point-at-source because the user's JAR lives in + * a Gradle/NFRT cache that they may clean at any time; the copy is a snapshot + * consistent with the decompiled source tree taken from the same JAR, and + * `force: true` re-runs both together. Always overwrites, so re-running after + * a rebuilt patched JAR refreshes it (and the new size/mtime invalidates the + * bytecode sidecar automatically). + */ + private registerAsRemappedJar(jarPath: string, version: string, mapping: MappingType): void { + const dest = getRemappedJarPath(version, mapping); + if (resolve(jarPath) === resolve(dest)) return; + try { + ensureDir(paths.remapped()); + copyFileSync(jarPath, dest); + logger.info(`Registered ${jarPath} as remapped JAR for ${version}/${mapping}: ${dest}`); + } catch (error) { + // Non-fatal: decompiled source is already written and usable. Only the + // bytecode-backed validators degrade, and they report why. + logger.warn( + `Failed to register remapped JAR for ${version}/${mapping} — bytecode validation will be unavailable: ${String(error)}`, + ); + } + } + /** * Force-clear all cached state for (version, mapping): * - decompiled source directory @@ -177,8 +233,10 @@ export class DecompileService { getDatabase().deleteJob(version, mapping); logger.info(`Force: clearing FTS5 index entries for ${version}/${mapping}`); getSearchIndexService().clearIndex(version, mapping); - // The remapped JAR is regenerated on the next decompile (new size/mtime, so - // the cache would self-invalidate) — clear the sidecar eagerly for tidiness. + // The remapped JAR itself is deliberately KEPT (re-remapping a vanilla + // version is expensive, and the patched flow overwrites it on the next run), + // so its size/mtime signature would not necessarily change — clearing the + // sidecar here is what actually guarantees a fresh read after a force. logger.info(`Force: clearing bytecode cache for ${version}/${mapping}`); getBytecodeIndexService().clearCache(version, mapping); }