ModPorter-AI entity detection (vibe-kanban)#1027
Conversation
…convert_mod() - Changed convert_mod() to use AST-first analysis which detects ALL entities - Falls back to MVP analysis only if AST fails - Detected entities are now converted via EntityConverter and written to addon - Added entities_detected and blocks_detected to result for audit visibility - Fixes #1023: entity detection regression (was detecting 1/entity, now detects all)
…t() as primary path in convert_mod() The remaining modified files are unrelated to this fix (frontend assets, binary files, etc.).
Reviewer's GuideSwitches the mod conversion CLI to an AST-first Java analysis pipeline that can detect entities and other features, then converts detected entities into Bedrock behavior/resource pack content and surfaces detection metrics in the CLI output, with mirrored changes in both ai-engine and modporter entrypoints and some repo tooling/assets updates. Sequence diagram for AST-first mod conversion with entity supportsequenceDiagram
actor User
participant CLI as convert_mod
participant FS as FileSystem
participant JavaAnalyzerAgent
participant BedrockBuilder as BedrockBuilderAgent
participant EntityConverter
participant PackagingAgent
User->>CLI: invoke convert_mod(jar_path, output_dir)
CLI->>FS: resolve jar_file path
Note over CLI,JavaAnalyzerAgent: Step 1 - AST-first analysis
CLI->>JavaAnalyzerAgent: analyze_jar_with_ast(jar_file)
alt AST analysis succeeds
JavaAnalyzerAgent-->>CLI: ast_analysis_result {success, features, assets}
CLI->>CLI: features = ast_analysis_result.features
CLI->>CLI: entities = features.entities
CLI->>CLI: blocks = features.blocks
CLI->>CLI: registry_name from first block or default
CLI->>CLI: texture_path from assets.block_textures or None
else AST analysis fails
JavaAnalyzerAgent-->>CLI: ast_analysis_result {success:false, error}
CLI->>CLI: log warning AST analysis failed
CLI->>JavaAnalyzerAgent: analyze_jar_for_mvp(jar_file)
JavaAnalyzerAgent-->>CLI: analysis_result {success, registry_name, texture_path}
alt MVP analysis succeeds
CLI->>CLI: registry_name = analysis_result.registry_name
CLI->>CLI: texture_path = analysis_result.texture_path
CLI->>CLI: entities = []
CLI->>CLI: features = {}
else MVP analysis fails
CLI->>CLI: raise RuntimeError(Analysis failed)
end
end
Note over CLI,BedrockBuilder: Step 2 - Build Bedrock add-on
CLI->>FS: create TemporaryDirectory()
CLI->>BedrockBuilder: build_behavior_and_resource_packs(temp_dir, registry_name, texture_path)
BedrockBuilder-->>CLI: build_result {behavior_pack_dir, resource_pack_dir, success}
alt build failed
CLI->>CLI: raise RuntimeError(Bedrock build failed)
end
Note over CLI,EntityConverter: Step 2b - Entity conversion (if any)
alt entities list is not empty
CLI->>EntityConverter: convert_entities(entities)
EntityConverter-->>CLI: bedrock_entities
alt bedrock_entities not empty
CLI->>FS: resolve bp_path from build_result.behavior_pack_dir
CLI->>FS: resolve rp_path from build_result.resource_pack_dir
CLI->>EntityConverter: write_entities_to_disk(bedrock_entities, bp_path, rp_path)
EntityConverter-->>CLI: written {entities, behaviors, animations}
else no bedrock_entities
CLI->>CLI: skip writing entities
end
else no entities
CLI->>CLI: skip entity conversion
end
Note over CLI,PackagingAgent: Step 3 - Package as .mcaddon
CLI->>PackagingAgent: package_addon(temp_dir, output_dir)
PackagingAgent-->>CLI: package_result {output_path, file_size, validation}
CLI->>CLI: result = {output_file, file_size, registry_name,
CLI->>CLI: entities_detected = len(entities),
CLI->>CLI: blocks_detected = len(features.blocks),
CLI->>CLI: validation}
CLI-->>User: return result and log counts of blocks and entities
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new AST-first analysis and entity-conversion flow is duplicated almost verbatim between
ai-engine/cli/main.pyandmodporter/cli/main.py; consider extracting this into a shared helper function to keep the logic DRY and reduce the chance of these paths diverging over time. - The log line
logger.info(f"Primary entity: {registry_name}")is using aregistry_namethat is derived from the first block rather than an entity, which can be misleading; consider renaming the message or using an entity-derived identifier when available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new AST-first analysis and entity-conversion flow is duplicated almost verbatim between `ai-engine/cli/main.py` and `modporter/cli/main.py`; consider extracting this into a shared helper function to keep the logic DRY and reduce the chance of these paths diverging over time.
- The log line `logger.info(f"Primary entity: {registry_name}")` is using a `registry_name` that is derived from the first block rather than an entity, which can be misleading; consider renaming the message or using an entity-derived identifier when available.
## Individual Comments
### Comment 1
<location path="modporter/cli/main.py" line_range="76-85" />
<code_context>
+ ast_analysis_result = java_analyzer.analyze_jar_with_ast(str(jar_file))
</code_context>
<issue_to_address>
**suggestion:** There is substantial duplicated AST-analysis and extraction logic that could be shared between CLIs
The AST-first analysis flow (invocation, feature extraction, block/entity handling, texture selection) is almost identical between `ai-engine/cli/main.py` and `modporter/cli/main.py`. Consider extracting this into a shared helper (e.g., returning `registry_name`, `texture_path`, `entities`, `features`) so changes to analysis behavior stay consistent and avoid drift between the CLIs.
Suggested implementation:
```python
# Step 1: Analyze the JAR file using shared AST-first analysis flow (detects ALL entities)
logger.info("Step 1: Analyzing Java mod (AST-first)...")
java_analyzer = JavaAnalyzerAgent()
# Use shared AST-first analysis helper so behavior is consistent across CLIs.
# The helper is responsible for:
# - Running AST-first analysis
# - Falling back to MVP analysis if AST fails
# - Returning a normalized analysis_result dict (including entities/features/texture/registry info)
analysis_result = run_ast_first_analysis(java_analyzer, jar_file, logger)
if not analysis_result.get("success", False):
```
To fully implement the refactor and actually share the AST-first analysis flow between CLIs, you should also:
1. **Introduce a shared helper** (for example in a new module like `shared/cli/analysis_flow.py` or similar, depending on your package layout):
```python
# shared/cli/analysis_flow.py
from pathlib import Path
from typing import Any, Dict
def run_ast_first_analysis(java_analyzer, jar_file: Path, logger) -> Dict[str, Any]:
"""
Shared AST-first analysis flow used by both ai-engine and modporter CLIs.
Responsibilities:
- Run AST-based JAR analysis first
- Fall back to MVP analysis if AST fails
- Return a normalized analysis_result dict containing:
- success: bool
- entities / blocks / items / etc.
- features
- texture_path / registry_name (if your analyzers populate these)
"""
# Try AST analysis first - this detects all entities, blocks, items, etc.
ast_analysis_result = java_analyzer.analyze_jar_with_ast(str(jar_file))
if ast_analysis_result.get("success", False):
return ast_analysis_result
# Fall back to MVP analysis if AST fails
logger.warning("AST analysis failed, falling back to MVP analysis...")
mvp_result = java_analyzer.analyze_jar_for_mvp(str(jar_file))
# Ensure we always return a dict with a success flag and normalized keys
if not mvp_result.get("success", False):
return mvp_result
return mvp_result
```
Adjust the normalization logic to ensure `registry_name`, `texture_path`, `entities`, and `features` are consistently present on the returned dict (or adapt it to return those values directly if the rest of your pipeline expects a tuple instead).
2. **Import the helper into `modporter/cli/main.py`** at the top of the file:
```python
from shared.cli.analysis_flow import run_ast_first_analysis
```
Update the import path to match your actual shared package/module structure.
3. **Update `ai-engine/cli/main.py`** to use the same helper instead of its own inlined AST-first analysis flow. Replace its AST + MVP logic with:
```python
analysis_result = run_ast_first_analysis(java_analyzer, jar_file, logger)
if not analysis_result.get("success", False):
# existing error handling
```
4. If your current CLIs unpack `registry_name`, `texture_path`, `entities`, and `features` separately instead of using the raw `analysis_result` dict, you can adjust `run_ast_first_analysis` to return a tuple:
```python
registry_name, texture_path, entities, features = run_ast_first_analysis(java_analyzer, jar_file, logger)
```
and then update both CLIs accordingly so they all rely on this shared helper for AST-first analysis behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ast_analysis_result = java_analyzer.analyze_jar_with_ast(str(jar_file)) | ||
|
|
||
| registry_name = analysis_result.get("registry_name", "unknown_block") | ||
| texture_path = analysis_result.get("texture_path") | ||
|
|
||
| logger.info(f"Found block: {registry_name}") | ||
| if not ast_analysis_result.get("success", False): | ||
| # Fall back to MVP analysis if AST fails | ||
| logger.warning("AST analysis failed, falling back to MVP analysis...") | ||
| analysis_result = java_analyzer.analyze_jar_for_mvp(str(jar_file)) | ||
| if not analysis_result.get("success", False): | ||
| raise RuntimeError( | ||
| f"Analysis failed: {analysis_result.get('error', 'Unknown error')}" | ||
| ) |
There was a problem hiding this comment.
suggestion: There is substantial duplicated AST-analysis and extraction logic that could be shared between CLIs
The AST-first analysis flow (invocation, feature extraction, block/entity handling, texture selection) is almost identical between ai-engine/cli/main.py and modporter/cli/main.py. Consider extracting this into a shared helper (e.g., returning registry_name, texture_path, entities, features) so changes to analysis behavior stay consistent and avoid drift between the CLIs.
Suggested implementation:
# Step 1: Analyze the JAR file using shared AST-first analysis flow (detects ALL entities)
logger.info("Step 1: Analyzing Java mod (AST-first)...")
java_analyzer = JavaAnalyzerAgent()
# Use shared AST-first analysis helper so behavior is consistent across CLIs.
# The helper is responsible for:
# - Running AST-first analysis
# - Falling back to MVP analysis if AST fails
# - Returning a normalized analysis_result dict (including entities/features/texture/registry info)
analysis_result = run_ast_first_analysis(java_analyzer, jar_file, logger)
if not analysis_result.get("success", False):To fully implement the refactor and actually share the AST-first analysis flow between CLIs, you should also:
-
Introduce a shared helper (for example in a new module like
shared/cli/analysis_flow.pyor similar, depending on your package layout):# shared/cli/analysis_flow.py from pathlib import Path from typing import Any, Dict def run_ast_first_analysis(java_analyzer, jar_file: Path, logger) -> Dict[str, Any]: """ Shared AST-first analysis flow used by both ai-engine and modporter CLIs. Responsibilities: - Run AST-based JAR analysis first - Fall back to MVP analysis if AST fails - Return a normalized analysis_result dict containing: - success: bool - entities / blocks / items / etc. - features - texture_path / registry_name (if your analyzers populate these) """ # Try AST analysis first - this detects all entities, blocks, items, etc. ast_analysis_result = java_analyzer.analyze_jar_with_ast(str(jar_file)) if ast_analysis_result.get("success", False): return ast_analysis_result # Fall back to MVP analysis if AST fails logger.warning("AST analysis failed, falling back to MVP analysis...") mvp_result = java_analyzer.analyze_jar_for_mvp(str(jar_file)) # Ensure we always return a dict with a success flag and normalized keys if not mvp_result.get("success", False): return mvp_result return mvp_result
Adjust the normalization logic to ensure
registry_name,texture_path,entities, andfeaturesare consistently present on the returned dict (or adapt it to return those values directly if the rest of your pipeline expects a tuple instead). -
Import the helper into
modporter/cli/main.pyat the top of the file:from shared.cli.analysis_flow import run_ast_first_analysis
Update the import path to match your actual shared package/module structure.
-
Update
ai-engine/cli/main.pyto use the same helper instead of its own inlined AST-first analysis flow. Replace its AST + MVP logic with:analysis_result = run_ast_first_analysis(java_analyzer, jar_file, logger) if not analysis_result.get("success", False): # existing error handling
-
If your current CLIs unpack
registry_name,texture_path,entities, andfeaturesseparately instead of using the rawanalysis_resultdict, you can adjustrun_ast_first_analysisto return a tuple:registry_name, texture_path, entities, features = run_ast_first_analysis(java_analyzer, jar_file, logger)
and then update both CLIs accordingly so they all rely on this shared helper for AST-first analysis behavior.
There was a problem hiding this comment.
Pull request overview
Updates the CLI conversion pipeline to address the entity-detection regression in v2 (Issue #1023) by routing analysis through AST-first detection and attempting to convert detected entities into the generated Bedrock add-on.
Changes:
- Switch
convert_mod()in both CLIs to tryanalyze_jar_with_ast()first, with MVP fallback. - Add an entity conversion step using
EntityConverterand surface detected entity/block counts in CLI output. - Add/convert multiple binary assets to Git LFS pointers (plus Git LFS hook scripts and miscellaneous binaries).
Reviewed changes
Copilot reviewed 6 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
modporter/cli/main.py |
AST-first analysis + entity conversion integration in the main ModPorter CLI pipeline. |
ai-engine/cli/main.py |
Same AST-first/entity conversion pipeline updates for the AI engine CLI entry point. |
verification_spinner.png |
Adds a Git LFS–tracked screenshot artifact. |
node-installer.msi |
Adds a Git LFS–tracked MSI binary. |
tests/fixtures/expected_output/.../copper_block.png |
Converts fixture image to Git LFS pointer. |
| `frontend/src/stories/assets/*.(svg | png)` |
| `.husky/_/(pre-push | post-merge |
modporter/cli/main.py
Outdated
| bp_path = ( | ||
| Path(build_result.get("behavior_pack_dir", temp_dir)) / "behavior_pack" | ||
| ) | ||
| rp_path = ( | ||
| Path(build_result.get("resource_pack_dir", temp_dir)) / "resource_pack" |
There was a problem hiding this comment.
BedrockBuilderAgent.build_block_addon_mvp() already returns absolute paths to the pack directories (e.g., .../behavior_pack and .../resource_pack). Appending another /"behavior_pack" and /"resource_pack" here will create nested directories (behavior_pack/behavior_pack) so the written entity files won’t be included by the packager (which zips temp_dir/behavior_pack). Use Path(build_result["behavior_pack_dir"]) / Path(build_result["resource_pack_dir"]) directly (and only fall back to Path(temp_dir)/... if those keys are missing).
| bp_path = ( | |
| Path(build_result.get("behavior_pack_dir", temp_dir)) / "behavior_pack" | |
| ) | |
| rp_path = ( | |
| Path(build_result.get("resource_pack_dir", temp_dir)) / "resource_pack" | |
| behavior_pack_dir = build_result.get("behavior_pack_dir") | |
| resource_pack_dir = build_result.get("resource_pack_dir") | |
| bp_path = ( | |
| Path(behavior_pack_dir) | |
| if behavior_pack_dir | |
| else Path(temp_dir) / "behavior_pack" | |
| ) | |
| rp_path = ( | |
| Path(resource_pack_dir) | |
| if resource_pack_dir | |
| else Path(temp_dir) / "resource_pack" |
modporter/cli/main.py
Outdated
| # If we detected entities, convert them and add to the addon | ||
| if entities: | ||
| logger.info(f"Step 2b: Converting {len(entities)} entities...") | ||
| entity_converter = EntityConverter() | ||
| bedrock_entities = entity_converter.convert_entities(entities) | ||
|
|
There was a problem hiding this comment.
The entities list coming from JavaAnalyzerAgent.analyze_jar_with_ast() contains items like {name, registry_name, methods, ...} (see JavaAnalyzerAgent._extract_features_from_ast / bytecode extraction), but EntityConverter expects each entry to have at least id (and ideally namespace). As written, _convert_java_entity() will default every entity to modporter:unknown_entity, causing dictionary key collisions and effectively leaving you with only one entity converted/written. Map analyzer entities into the schema EntityConverter expects (e.g., derive id from registry_name or name, and namespace from ast_analysis_result["mod_info"]["name"]) before calling convert_entities().
| if "block_textures" in assets and assets["block_textures"]: | ||
| texture_path = assets["block_textures"][0] |
There was a problem hiding this comment.
JavaAnalyzerAgent.analyze_jar_with_ast() populates assets with keys {textures, models, sounds, other} (see _analyze_assets_from_jar), but this code looks for assets["block_textures"], which will never be present. As a result texture_path will always stay None on the AST-success path. Filter assets["textures"] for a likely block texture (e.g., containing /textures/block/), or reuse the MVP _find_block_texture logic.
| if "block_textures" in assets and assets["block_textures"]: | |
| texture_path = assets["block_textures"][0] | |
| textures = assets.get("textures", []) | |
| if textures: | |
| block_textures = [ | |
| texture | |
| for texture in textures | |
| if "/textures/block/" in texture or "textures/block/" in texture | |
| ] | |
| texture_path = block_textures[0] if block_textures else textures[0] |
| raise RuntimeError( | ||
| f"Analysis failed: {analysis_result.get('error', 'Unknown error')}" | ||
| ) |
There was a problem hiding this comment.
On AST failure fallback, JavaAnalyzerAgent.analyze_jar_for_mvp() reports details in an errors list, not an error field. Using analysis_result.get('error', ...) will typically surface Unknown error even when real errors exist. Prefer analysis_result.get('error') OR join analysis_result.get('errors', []) for a useful message.
| raise RuntimeError( | |
| f"Analysis failed: {analysis_result.get('error', 'Unknown error')}" | |
| ) | |
| error_message = analysis_result.get("error") | |
| if not error_message: | |
| errors = analysis_result.get("errors", []) | |
| if isinstance(errors, list) and errors: | |
| error_message = "; ".join(str(error) for error in errors) | |
| else: | |
| error_message = "Unknown error" | |
| raise RuntimeError(f"Analysis failed: {error_message}") |
modporter/cli/main.py
Outdated
| @@ -99,6 +128,29 @@ def convert_mod(jar_path: str, output_dir: str = None) -> Dict[str, Any]: | |||
| f"Bedrock build failed: {build_result.get('error', 'Unknown error')}" | |||
| ) | |||
There was a problem hiding this comment.
BedrockBuilderAgent.build_block_addon_mvp() returns failures via an errors list (it does not set an error field). This branch will usually raise Bedrock build failed: Unknown error and hide the real cause. Use build_result.get('error') OR join build_result.get('errors', []) when constructing the exception message.
modporter/cli/main.py
Outdated
| # Step 1: Analyze the JAR file using AST-first approach (detects ALL entities) | ||
| logger.info("Step 1: Analyzing Java mod (AST-first)...") | ||
| java_analyzer = JavaAnalyzerAgent() | ||
| analysis_result = java_analyzer.analyze_jar_for_mvp(str(jar_file)) | ||
|
|
||
| if not analysis_result.get("success", False): | ||
| raise RuntimeError(f"Analysis failed: {analysis_result.get('error', 'Unknown error')}") | ||
| # Try AST analysis first - this detects all entities, blocks, items, etc. | ||
| ast_analysis_result = java_analyzer.analyze_jar_with_ast(str(jar_file)) | ||
|
|
There was a problem hiding this comment.
This change introduces a new primary execution path (AST-success) plus entity conversion/writing, but the existing CLI unit tests primarily mock analyze_jar_for_mvp() and don’t cover the AST-success branch or entity conversion output. Add/adjust tests to mock analyze_jar_with_ast() returning multiple entities and assert that EntityConverter.write_entities_to_disk() is called with the correct pack directories and that the result reports entities_detected accordingly.
| bp_path = ( | ||
| Path(build_result.get("behavior_pack_dir", temp_dir)) / "behavior_pack" | ||
| ) | ||
| rp_path = ( | ||
| Path(build_result.get("resource_pack_dir", temp_dir)) / "resource_pack" |
There was a problem hiding this comment.
BedrockBuilderAgent.build_block_addon_mvp() already returns absolute paths to the pack directories. Appending another /"behavior_pack" and /"resource_pack" here creates nested directories (behavior_pack/behavior_pack), so written entity files won’t be packaged (the packager zips temp_dir/behavior_pack). Use the returned behavior_pack_dir / resource_pack_dir paths directly.
| bp_path = ( | |
| Path(build_result.get("behavior_pack_dir", temp_dir)) / "behavior_pack" | |
| ) | |
| rp_path = ( | |
| Path(build_result.get("resource_pack_dir", temp_dir)) / "resource_pack" | |
| bp_path = Path( | |
| build_result.get("behavior_pack_dir", Path(temp_dir) / "behavior_pack") | |
| ) | |
| rp_path = Path( | |
| build_result.get("resource_pack_dir", Path(temp_dir) / "resource_pack") |
| # AST analysis succeeded - extract comprehensive features | ||
| features = ast_analysis_result.get("features", {}) | ||
| entities = features.get("entities", []) | ||
| blocks = features.get("blocks", []) | ||
|
|
||
| logger.info(f"AST analysis found: {len(blocks)} blocks, {len(entities)} entities") |
There was a problem hiding this comment.
The entities extracted from analyze_jar_with_ast() are analyzer feature dicts (e.g., {name, registry_name, methods}) but EntityConverter expects at least id (and typically namespace). With the current input, all entities default to modporter:unknown_entity, causing key collisions and effectively converting only one entity. Transform analyzer entities into the EntityConverter schema (derive id/namespace) before calling convert_entities().
| if "block_textures" in assets and assets["block_textures"]: | ||
| texture_path = assets["block_textures"][0] |
There was a problem hiding this comment.
analyze_jar_with_ast() asset analysis returns assets["textures"] (see JavaAnalyzerAgent._analyze_assets_from_jar), not assets["block_textures"]. This means texture_path will remain None on the AST-success path. Update this to select an appropriate entry from assets["textures"] (e.g., one under /textures/block/).
| if "block_textures" in assets and assets["block_textures"]: | |
| texture_path = assets["block_textures"][0] | |
| textures = assets.get("textures", []) | |
| if textures: | |
| texture_path = next( | |
| ( | |
| texture | |
| for texture in textures | |
| if "/textures/block/" in texture or "textures/block/" in texture | |
| ), | |
| textures[0], | |
| ) |
| #!/bin/sh | ||
| command -v git-lfs >/dev/null 2>&1 || { echo >&2 "\nThis repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks').\n"; exit 2; } | ||
| git lfs pre-push "$@" |
There was a problem hiding this comment.
These Git LFS hook scripts under .husky/_/ duplicate the existing hooks in .husky/ and will not run unless core.hooksPath is configured to point at .husky/_. This looks like an accidental commit from running git lfs install with a different local hooksPath; either remove these duplicates or update the repo’s Husky/Git hook configuration so there is a single, consistent hook location.
…n main.py Resolved merge conflicts in convert_mod function: - Unified AST-first analysis approach from PR with entity texture/model extraction - Preserved entity conversion logic with texture enrichment - Kept cleaner logging from PR while maintaining entity-only mod handling
When AST analysis succeeds, the block registry_name was derived only from the class name (e.g., 'emerald_block') without the namespace prefix. This caused test_cli_integration_e2e to fail because it expected 'cli_test:' to be in the registry name. The fix extracts mod_id from mod_info (which comes from fabric.mod.json) and prefixes the block registry name when it's not already namespaced. Note: Fixed both ai-engine/cli/main.py and modporter/cli/main.py as they both contain the convert_mod function used by different test imports.
- Add audit v3 report: 8/8 mods pass, 54.7% textures, 0% models/recipes - Update ROADMAP.md: replace Feb 2025 version with current 11-week launch plan - Update .factory/tasks.md: current sprint status and audit summary - Entity fix #1027 partially working (Create: 1→9 entities) - P0 gaps identified: models (4,806), recipes (3,852), BlockEntity classification - B2B readiness: ~25-30% weighted, targeting 60% after Week 3-4 sprint
Work on this GitHub issue #1023
Summary by Sourcery
Adopt an AST-first Java analysis pipeline for mod conversion, adding entity detection and conversion into the Bedrock add-on flow and exposing detection counts in the CLI results.
New Features:
Enhancements:
Build: