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 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/CLAUDE.md b/CLAUDE.md index 71ea8cf..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, 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. **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__/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__/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 0000000..b3fb1ac Binary files /dev/null and b/__tests__/fixtures/summoningrituals-mc-stubs.jar differ 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__/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..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, @@ -32,7 +34,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 || ''})`, ); @@ -114,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/access-transformer-service.test.ts b/__tests__/services/access-transformer-service.test.ts index 4a50f7b..a13bf05 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,108 @@ 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('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 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 +507,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 +651,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 +676,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 +751,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 +978,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 +1063,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,12 +1076,12 @@ 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); }); }); 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. @@ -779,11 +1090,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 () => { @@ -793,7 +1111,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/__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/__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/__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/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/docs/specs/access-transformer-support.md b/docs/specs/access-transformer-support.md index 4585044..e12eb71 100644 --- a/docs/specs/access-transformer-support.md +++ b/docs/specs/access-transformer-support.md @@ -2,7 +2,47 @@ **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`). +- **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 + 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/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": { 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'}`, ); } } diff --git a/src/server/tools.ts b/src/server/tools.ts index 691232d..3d22a4c 100644 --- a/src/server/tools.ts +++ b/src/server/tools.ts @@ -2,8 +2,14 @@ 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 { getAccessWidenerService } from '../services/access-widener-service.js'; +import { + accessTransformerEntryToString, + getAccessTransformerService, +} from '../services/access-transformer-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'; @@ -17,7 +23,9 @@ import { getSearchIndexService } from '../services/search-index-service.js'; import { getVersionManager } from '../services/version-manager.js'; import type { AccessTransformer, + AccessTransformerEntry, AccessWidener, + AccessWidenerEntry, MappingType, MixinClass, } from '../types/minecraft.js'; @@ -1674,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, @@ -1734,18 +1765,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 c450c89..525d1d5 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 { bytecodeUnavailableMessage, 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']); @@ -114,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 @@ -141,205 +154,181 @@ 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) -// --------------------------------------------------------------------------- +/** 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; +} -/** 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]; +/** 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) : ''; } -/** 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]; +/** + * 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 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[], + classSuggestionPool?: string[], ): { 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`); + // 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 }; } // --- 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 +336,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 +343,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 +394,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 +413,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 +651,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,28 +681,67 @@ 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: bytecodeUnavailableMessage(mcVersion, mapping), }); 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); + // `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) { + const enclosing = internal.slice(0, idx); + needed.add(enclosing); + idx = enclosing.lastIndexOf('$'); + } + } + + 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 }; + } - // 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(); + // JAR-wide class list for same-package "did you mean" suggestions on + // 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. + // Validate each entry against bytecode. for (const entry of accessTransformer.entries) { - const validation = this.validateEntry( + const validation = validateEntryAgainstBytecode( entry, - decompiledPath, - symbolsCache, + classMap, accessTransformer.entries, + // 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) => ({ @@ -761,43 +763,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/access-widener-service.ts b/src/services/access-widener-service.ts index 887ae8d..e741c40 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,124 @@ 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 { bytecodeUnavailableMessage, 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) -// --------------------------------------------------------------------------- +/** 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) : ''; +} -/** 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]; +/** + * 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); } -/** 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, + classSuggestionPool?: string[], ): { 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}`); + // 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 }; + } - // --- 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 +156,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 +184,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 +314,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 +330,75 @@ 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: bytecodeUnavailableMessage(mcVersion, mapping), }); 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 }; + } + + // JAR-wide class list for same-package "did you mean" suggestions on + // 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; + }; - // Validate each entry for (const entry of accessWidener.entries) { - const validation = this.validateEntry(entry, decompiledPath, symbolsCache); + 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((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 +408,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 */ diff --git a/src/services/bytecode-index-service.ts b/src/services/bytecode-index-service.ts new file mode 100644 index 0000000..81dd210 --- /dev/null +++ b/src/services/bytecode-index-service.ts @@ -0,0 +1,259 @@ +/** + * 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 { 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. */ + 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); + let cache = this.loadCache(cachePath, signature); + + const wanted = [...new Set(internalNames)]; + // 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); + // 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; + } + 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; + } + + /** + * 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)); + 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 { + const tmp = `${cachePath}.${process.pid}.tmp`; + try { + 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)}`); + } + } + + /** + * 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 + } + + // 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); + 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..3afa835 100644 --- a/src/services/decompile-service.ts +++ b/src/services/decompile-service.ts @@ -1,13 +1,15 @@ -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'; @@ -89,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, @@ -104,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' }; } @@ -126,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' }; } @@ -148,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) { @@ -156,11 +178,47 @@ 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 * - 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 +233,12 @@ export class DecompileService { getDatabase().deleteJob(version, mapping); logger.info(`Force: clearing FTS5 index entries for ${version}/${mapping}`); getSearchIndexService().clearIndex(version, mapping); + // 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); } /** 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'], };