Skip to content

feat(game-asset-mcp): add open-source MCP server for 3D game asset libraries#82

Open
jbdevprimary wants to merge 3 commits into
mainfrom
feat/game-asset-mcp
Open

feat(game-asset-mcp): add open-source MCP server for 3D game asset libraries#82
jbdevprimary wants to merge 3 commits into
mainfrom
feat/game-asset-mcp

Conversation

@jbdevprimary
Copy link
Copy Markdown
Contributor

@jbdevprimary jbdevprimary commented Mar 7, 2026

Summary

  • New package game-asset-mcp — fully open-source MCP server for local 3D game asset libraries
  • Integrates into Claude Code, Cursor, Windsurf via Model Context Protocol
  • Published as pip install game-asset-mcp (pending PyPI release)

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)

Changes Made

Package: packages/game-asset-mcp/

Core:

  • catalog.py — SQLite + FTS5 catalog with search, browse, upsert, stats
  • glb_reader.py — pure-Python GLB binary parser (no Blender needed for ingest)
  • ingest.py — idempotent scanner: O(1) skip via in-memory size hash, stale removal
  • polyhaven.py — CC0 asset search/download via PolyHaven public API (httpx)
  • server.py — FastMCP 3.0 MCP server with 11 tools

Configuration (pydantic-settings):

  • config.py — layered config: TOML file → env vars (GAME_ASSET_*) → CLI flags
  • Fully configurable taxonomy: style_map, source_hints, skip_dirs
  • wizard.py — interactive setup wizard (game-asset-init --yes for CI/Docker)
  • pydantic-cli for IngestOptions: auto --help, JSON config file ingestion

MCP Tools:
search_assets, browse_taxonomy, list_categories, get_asset_info, copy_asset, get_preview, generate_preview, run_ingest, get_catalog_stats, search_polyhaven, download_polyhaven_asset

Testing:

  • 140 unit/integration tests — all passing in 0.57s
  • conftest.py with GLB binary builder, tmp DB, taxonomy fixtures, settings cache reset
  • Full coverage: catalog CRUD, FTS search, GLB parsing, PolyHaven mock API, server tools, wizard
  • E2E suite that runs against real ASSETS_ROOT when available (skips otherwise)

Monorepo integration:

  • Added to [tool.uv.workspace] members in root pyproject.toml
  • Added to testpaths in root pyproject.toml
  • project.json with Nx targets: test, test:e2e, lint, typecheck, ingest
  • Root README.md updated with package listing and directory tree entry

Add to Claude Code

pip install "game-asset-mcp[polyhaven]"
game-asset-init --root /path/to/assets --yes
claude mcp add game-asset-library -- game-asset-mcp

Testing

  • Unit tests added/updated (140 tests)
  • Integration tests added/updated
  • E2E tests added/updated (skipped unless ASSETS_ROOT is set)
  • Manual testing performed

Test Details

uv run pytest packages/game-asset-mcp/tests/ --ignore=packages/game-asset-mcp/tests/e2e/ -v
# 140 passed in 0.57s

Self-Review Checklist

Code Quality

  • Code follows project style guidelines
  • No linting errors
  • from __future__ import annotations in all files
  • Python 3.10–3.14 support (tomli fallback for 3.10)
  • Self-reviewed code for logic errors

Testing

  • All new code is covered by tests
  • All tests pass locally
  • Edge cases are tested (invalid GLB, missing files, HTTP errors)
  • Error cases are tested

Documentation

  • README.md with "Add to Claude Code", "Add to Cursor", "Add to Windsurf" instructions
  • CHANGELOG.md initialized
  • Root README updated

Commits

  • Commits follow conventional commit format

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added game-asset-mcp package for managing and searching local 3D game asset libraries.
    • Integrated PolyHaven free asset source with search and download capabilities.
    • Interactive configuration wizard for quick library setup.
    • Hybrid search combining keyword and semantic matching for asset discovery.
    • Automatic preview generation for imported models.
  • Documentation

    • Added comprehensive documentation and changelog for game-asset-mcp package.

jbdevprimary and others added 3 commits March 3, 2026 22:17
Patch release fixing the animation task responsePath bug (id → result).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…braries

New package `game-asset-mcp` — a fully open-source MCP server for local 3D
game asset libraries. Integrates directly into Claude Code, Cursor, and Windsurf
via the Model Context Protocol.

## Core Features
- SQLite catalog with FTS5 full-text search across all GLBs
- Idempotent ingest: O(1) skip check via in-memory hash, stale detection
- `browse_taxonomy` tool for macro→meso→micro→pack hierarchy navigation
- Pure-Python GLB stats reader (vertices, faces, materials, animations, textures)
- PolyHaven CC0 integration: search, preview, download with auto-taxonomy placement
- Headless Blender preview generation (auto-detected binary, no bpy dep)

## Configuration
- pydantic-settings: layered config (TOML → env vars → CLI flags)
- TOML config at ~/.config/game-asset-mcp/config.toml
- Fully configurable taxonomy: style_map, source_hints, skip_dirs
- Interactive setup wizard: `game-asset-init` (or `--yes` for non-interactive)
- pydantic-cli for IngestOptions: auto --help, JSON config file ingestion
- Python 3.10–3.14 support (tomli fallback for 3.10)

## Testing
- 140 unit/integration tests — all passing
- conftest.py with GLB binary builder, tmp DB, taxonomy fixtures
- Full coverage: catalog, ingest, glb_reader, polyhaven, server tools, wizard
- E2E test suite that runs against real ASSETS_ROOT when available

## Monorepo Integration
- Added to [tool.uv.workspace] members
- project.json with Nx targets (test, test:e2e, lint, typecheck, ingest)
- Root README updated with package listing and directory tree

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 7, 2026

📝 Walkthrough

Walkthrough

The PR introduces the complete game-asset-mcp package, a new MCP server for managing local 3D game asset libraries. It provides SQLite catalog management with FTS5 search, asset ingestion and preview generation via Blender, vector search capability, PolyHaven integration, configuration management via TOML, and a CLI setup wizard.

Changes

Cohort / File(s) Summary
Documentation & Root Configuration
README.md, pyproject.toml, CHANGELOG.md
Added game-asset-mcp to root workspace, extended test discovery, and documented initial release features.
Package Configuration
packages/game-asset-mcp/pyproject.toml, packages/game-asset-mcp/project.json, packages/game-asset-mcp/README.md
Configured build system, project metadata, CI/QA targets, dependencies, CLI entry points, and user-facing documentation for asset library MCP server.
Core Catalog & Search
packages/game-asset-mcp/src/game_asset_mcp/catalog.py, packages/game-asset-mcp/src/game_asset_mcp/search.py
Implemented SQLite catalog with FTS5 indexing for keyword search and vector embeddings for semantic search; includes asset upsert, filtering by style/category/properties, and hybrid ranking.
Asset Processing Pipeline
packages/game-asset-mcp/src/game_asset_mcp/glb_reader.py, packages/game-asset-mcp/src/game_asset_mcp/ingest.py, packages/game-asset-mcp/src/game_asset_mcp/render_preview.py
Added pure-Python GLB parser for mesh/material/animation stats, idempotent ingest workflow with taxonomy detection and stale record cleanup, and headless Blender rendering to generate asset previews.
Configuration & External Integration
packages/game-asset-mcp/src/game_asset_mcp/config.py, packages/game-asset-mcp/src/game_asset_mcp/polyhaven.py
Implemented Pydantic-based settings with TOML config file support and taxonomy management; added PolyHaven API client for asset search and multi-type downloads (models/HDRIs/textures).
Package Initialization & Server
packages/game-asset-mcp/src/game_asset_mcp/__init__.py, packages/game-asset-mcp/src/game_asset_mcp/server.py
Added module version export and comprehensive FastMCP 3.0 stdio server exposing tools for asset search, catalog management, preview generation, taxonomy browsing, and PolyHaven integration.
CLI Wizard
packages/game-asset-mcp/src/game_asset_mcp/wizard.py
Implemented interactive setup wizard (game-asset-init) for initial configuration, style detection, TOML generation, and optional ingest triggering.
Unit & Integration Tests
packages/game-asset-mcp/tests/conftest.py, packages/game-asset-mcp/tests/test_*.py, packages/game-asset-mcp/tests/e2e/*
Added comprehensive test fixtures (GLB binary generation, temp DB/filesystem setup, settings cache isolation) and test suites covering catalog ops, GLB parsing, ingest workflows, config handling, server tools, and end-to-end ingestion against real asset roots.

Sequence Diagrams

sequenceDiagram
    participant Client
    participant Server
    participant Catalog as SQLite Catalog
    participant Search as Search Engine
    
    Client->>Server: search_assets(query, style, category)
    Server->>Search: hybrid_search(query, filters)
    Search->>Catalog: fts_search(query, filters)
    Catalog-->>Search: FTS results
    Search->>Catalog: vec_search(query, filters)
    Catalog-->>Search: Vector results
    Search->>Search: deduplicate & rank
    Search-->>Server: merged results
    Server-->>Client: formatted asset list
Loading
sequenceDiagram
    participant User
    participant Wizard
    participant Filesystem
    participant Ingest
    participant Catalog as SQLite Catalog
    
    User->>Wizard: game-asset-init
    Wizard->>Filesystem: scan for style dirs
    Filesystem-->>Wizard: detected styles
    Wizard->>Filesystem: write config.toml
    Wizard->>Ingest: run_ingest()
    Ingest->>Filesystem: scan_glbs(root)
    Filesystem-->>Ingest: GLB paths
    Ingest->>Ingest: extract metadata (glb_reader)
    Ingest->>Catalog: upsert_asset()
    Catalog->>Catalog: rebuild_fts index
    Ingest-->>User: summary (added/skipped/etc)
Loading
sequenceDiagram
    participant Client
    participant Server
    participant Catalog as SQLite Catalog
    participant Blender
    participant Filesystem
    
    Client->>Server: generate_preview(glb_path)
    Server->>Catalog: check preview_path
    alt preview exists
        Server-->>Client: existing preview path
    else preview missing
        Server->>Blender: render_preview.py GLB→PNG
        Blender->>Filesystem: load GLB, render scene
        Filesystem-->>Blender: 512x512 PNG
        Blender-->>Server: render complete
        Server->>Catalog: upsert_asset(preview_path)
        Server-->>Client: new preview path
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A catalog of treasures, polished and bright,
From SQLite's depths to vector's light,
GLBs sorted, searched, and previewed with care,
PolyHaven's gifts now nestled there,
A warren of assets, organized just right! 🎨✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding an open-source MCP server for 3D game asset libraries, which is the primary objective of this PR.
Description check ✅ Passed The description covers all required sections: summary, type of change, detailed changes made, testing performed, and a comprehensive self-review checklist.
Docstring Coverage ✅ Passed Docstring coverage is 87.44% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/game-asset-mcp

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a comprehensive new Python package, game-asset-mcp, designed to serve as an open-source Model Context Protocol (MCP) server for managing local 3D game asset libraries. It provides robust capabilities for cataloging, searching, and interacting with GLB/GLTF assets, including integration with external asset sources like PolyHaven. The system is built for efficiency, offering idempotent ingestion and a flexible configuration, making it a powerful tool for developers working with 3D game assets across various environments.

Highlights

  • New Package Introduction: Introduced a new Python package game-asset-mcp as a fully open-source Model Context Protocol (MCP) server for managing local 3D game asset libraries.
  • Asset Cataloging and Search: Implemented a SQLite-based catalog with FTS5 for efficient full-text search, browsing, and management of GLB/GLTF assets.
  • GLB Parsing and Ingestion: Developed a pure-Python GLB binary parser to extract mesh statistics without external dependencies, and an idempotent ingestion system for efficient asset scanning and catalog updates.
  • PolyHaven Integration: Added integration with PolyHaven for searching and downloading free CC0 3D models, HDRIs, and textures directly into the local library.
  • MCP Server and Tools: Provided a FastMCP 3.0 server exposing 11 distinct tools for interacting with the asset library, including search, taxonomy browsing, asset copying, and preview generation.
  • Flexible Configuration: Included a flexible configuration system using pydantic-settings that supports TOML files, environment variables, and CLI flags, complemented by an interactive setup wizard.
  • Monorepo Integration: Integrated the new package into the monorepo's build system by updating pyproject.toml and adding a project.json with Nx targets for testing and development workflows.
  • Comprehensive Testing: Included 140 unit and integration tests, along with an end-to-end (E2E) test suite, ensuring robust functionality and coverage for the new features.
Changelog
  • packages/game-asset-mcp/CHANGELOG.md
    • Initial release.
    • SQLite catalog with FTS5 full-text search.
    • Idempotent ingest (O(1) skip check, stale removal).
    • browse_taxonomy tool for macro/meso/micro/pack hierarchy navigation.
    • search_assets hybrid keyword + FTS search.
    • copy_asset, get_preview, generate_preview tools.
    • PolyHaven CC0 integration (search_polyhaven, download_polyhaven_asset).
    • Pydantic-settings configuration with TOML file support.
    • Interactive setup wizard (game-asset-init).
    • Optional bpy extra for in-process Blender rendering.
    • Pure-Python GLB stats reader (no Blender required for ingest).
Activity
  • Developed 140 unit and integration tests covering core functionality, FTS search, GLB parsing, PolyHaven API mocks, server tools, and the setup wizard.
  • Implemented an end-to-end (E2E) test suite that runs against a real ASSETS_ROOT when configured.
  • Performed manual testing to validate functionality.
  • Ensured code quality by adhering to project style guidelines, resolving linting errors, and supporting Python 3.10–3.14.
  • Verified comprehensive test coverage, including edge and error cases.
  • Updated README.md with usage instructions and initialized CHANGELOG.md.
  • Confirmed commit messages follow conventional commit format.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@amazon-q-developer amazon-q-developer Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Issues Identified

This PR introduces a new MCP server package for 3D game asset libraries with comprehensive functionality and 140 passing tests. However, there are critical SQL injection vulnerabilities that must be fixed before merge.

Critical Security Issues (5)

Multiple SQL injection vulnerabilities were identified where f-strings are used to construct SQL queries with user-controlled input:

  1. catalog.py line 151: LIMIT clause constructed with f-string
  2. catalog.py lines 173-174: LIMIT clause constructed with f-string
  3. server.py lines 319-329: WHERE clause constructed with f-string
  4. server.py lines 341-357: WHERE clause constructed with f-string
  5. server.py lines 368-374: WHERE clause constructed with f-string

All of these must use parameterized queries instead. While max_results appears to be an integer parameter, using f-strings for SQL construction is dangerous and violates secure coding practices. The where_sql variable constructed from user input must also use proper parameterization.

Positive Aspects

  • Comprehensive test coverage (140 tests passing in 0.57s)
  • Well-structured code with clear separation of concerns
  • Pure-Python GLB parser eliminates Blender dependency for ingestion
  • Good error handling in PolyHaven integration
  • Proper use of parameterized queries in most database operations

Please fix the SQL injection vulnerabilities before merging.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

if has_textures is not None:
sql += " AND a.has_embedded_textures = ?"
params.append(1 if has_textures else 0)
sql += f" LIMIT {max_results}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Security Vulnerability: SQL injection in dynamic LIMIT clause. Use parameterized queries instead of f-strings for SQL construction.

Suggested change
sql += f" LIMIT {max_results}"
sql += " LIMIT ?"
params.append(max_results)

Comment on lines +173 to +174
sql += " ORDER BY rank"
sql += f" LIMIT {max_results}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Security Vulnerability: SQL injection in dynamic LIMIT clause. Use parameterized queries for all SQL parameters including LIMIT.

Suggested change
sql += " ORDER BY rank"
sql += f" LIMIT {max_results}"
sql += " ORDER BY rank"
sql += " LIMIT ?"
params.append(max_results)

Comment on lines +319 to +329
rows = conn.execute(f"""
SELECT style,
CASE WHEN instr(category, '/') > 0
THEN substr(category, 1, instr(category, '/') - 1)
ELSE category END AS meso,
COUNT(*) as count
FROM assets
WHERE {where_sql}
GROUP BY style, meso
ORDER BY style, meso
""", params).fetchall()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Security Vulnerability: SQL injection in f-string SQL construction. Use parameterized queries for WHERE clause to prevent injection attacks.

Suggested change
rows = conn.execute(f"""
SELECT style,
CASE WHEN instr(category, '/') > 0
THEN substr(category, 1, instr(category, '/') - 1)
ELSE category END AS meso,
COUNT(*) as count
FROM assets
WHERE {where_sql}
GROUP BY style, meso
ORDER BY style, meso
""", params).fetchall()
rows = conn.execute("""
SELECT style,
CASE WHEN instr(category, '/') > 0
THEN substr(category, 1, instr(category, '/') - 1)
ELSE category END AS meso,
COUNT(*) as count
FROM assets
WHERE """ + where_sql + """
GROUP BY style, meso
ORDER BY style, meso
""", params).fetchall()

Comment on lines +341 to +357
rows = conn.execute(f"""
SELECT style,
CASE WHEN instr(category, '/') > 0
THEN substr(category, 1, instr(category, '/') - 1)
ELSE category END AS meso,
CASE WHEN instr(category, '/') > 0
THEN (SELECT CASE WHEN instr(rest, '/') > 0
THEN substr(rest, 1, instr(rest, '/') - 1)
ELSE rest END
FROM (SELECT substr(category, instr(category, '/') + 1) as rest))
ELSE NULL END AS micro,
COUNT(*) as count
FROM assets
WHERE {where_sql}
GROUP BY style, meso, micro
ORDER BY style, micro
""", params).fetchall()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Security Vulnerability: SQL injection in f-string SQL construction. Use parameterized queries for WHERE clause to prevent injection attacks.

Suggested change
rows = conn.execute(f"""
SELECT style,
CASE WHEN instr(category, '/') > 0
THEN substr(category, 1, instr(category, '/') - 1)
ELSE category END AS meso,
CASE WHEN instr(category, '/') > 0
THEN (SELECT CASE WHEN instr(rest, '/') > 0
THEN substr(rest, 1, instr(rest, '/') - 1)
ELSE rest END
FROM (SELECT substr(category, instr(category, '/') + 1) as rest))
ELSE NULL END AS micro,
COUNT(*) as count
FROM assets
WHERE {where_sql}
GROUP BY style, meso, micro
ORDER BY style, micro
""", params).fetchall()
rows = conn.execute("""
SELECT style,
CASE WHEN instr(category, '/') > 0
THEN substr(category, 1, instr(category, '/') - 1)
ELSE category END AS meso,
CASE WHEN instr(category, '/') > 0
THEN (SELECT CASE WHEN instr(rest, '/') > 0
THEN substr(rest, 1, instr(rest, '/') - 1)
ELSE rest END
FROM (SELECT substr(category, instr(category, '/') + 1) as rest))
ELSE NULL END AS micro,
COUNT(*) as count
FROM assets
WHERE """ + where_sql + """
GROUP BY style, meso, micro
ORDER BY style, micro
""", params).fetchall()

Comment on lines +368 to +374
rows = conn.execute(f"""
SELECT style, pack, COUNT(*) as count
FROM assets
WHERE {where_sql}
GROUP BY style, pack
ORDER BY style, pack
""", params).fetchall()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Security Vulnerability: SQL injection in f-string SQL construction. Use parameterized queries for WHERE clause to prevent injection attacks.

Suggested change
rows = conn.execute(f"""
SELECT style, pack, COUNT(*) as count
FROM assets
WHERE {where_sql}
GROUP BY style, pack
ORDER BY style, pack
""", params).fetchall()
rows = conn.execute("""
SELECT style, pack, COUNT(*) as count
FROM assets
WHERE """ + where_sql + """
GROUP BY style, pack
ORDER BY style, pack
""", params).fetchall()

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 7, 2026

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 7, 2026

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 7, 2026

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 7, 2026

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive game-asset-mcp package, functioning as a new MCP server for managing 3D game assets. It includes features like an SQLite catalog with FTS5, pure-Python GLB parsing, idempotent ingestion, and PolyHaven integration, backed by an extensive test suite. However, several critical security vulnerabilities related to improper path handling were identified. The copy_asset tool allows for arbitrary file read/write, and the PolyHaven integration is vulnerable to path traversal via the asset_id parameter, with other tools also lacking proper path validation against the configured asset root. These issues should be addressed by implementing strict path validation and sanitization for all user-supplied filesystem paths. Additionally, enhancing robustness and maintainability through more specific exception handling and minor code cleanup opportunities is recommended.

Comment on lines +151 to +171
def copy_asset(src_path: str, dest_dir: str, rename: Optional[str] = None) -> dict:
"""
Copy a GLB asset to a destination directory (e.g. into your game project).

Args:
src_path: Absolute path to the source .glb file
dest_dir: Destination directory (will be created if needed)
rename: Optional new filename without extension (default: keep original name)

Returns:
{'dest': '/path/to/copied.glb', 'size_kb': 42}
"""
src = Path(src_path)
if not src.exists():
return {"error": f"Source not found: {src_path}"}
dest = Path(dest_dir)
dest.mkdir(parents=True, exist_ok=True)
out_name = (rename or src.stem) + ".glb"
out_path = dest / out_name
shutil.copy2(src, out_path)
return {"dest": str(out_path), "size_kb": out_path.stat().st_size // 1024}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The copy_asset tool allows copying files from an arbitrary src_path to an arbitrary dest_dir without any validation. This can be exploited to read sensitive files (e.g., SSH keys, configuration files) by copying them to a location accessible to the attacker or the LLM, or to write files to arbitrary locations (restricted to the .glb extension). You should validate that src_path is within ASSETS_ROOT and that dest_dir is a safe, allowed location.

Comment on lines +110 to +164
def download_ph_asset(
asset_id: str,
asset_type: str,
resolution: str = "1k",
) -> dict:
"""
Download asset from PolyHaven and save to taxonomy path.

For models: downloads the GLB at the requested resolution.
For HDRIs: downloads the .hdr file at the requested resolution.
For textures: downloads all available map files (diffuse, rough, metal,
normal, ao, disp, arm) at the requested resolution.

Returns:
{dest_dir: str, files: [str], asset_type: str, categories: list}
or {error: str} on failure.
"""
# Fetch asset info to get categories
try:
info = get_ph_info(asset_id)
except httpx.HTTPError as exc:
return {"error": f"Failed to fetch asset info: {exc}"}

categories = info.get("categories", [])

# Fetch file manifest
try:
files_data = ph_get(f"files/{asset_id}")
except httpx.HTTPError as exc:
return {"error": f"Failed to fetch file manifest: {exc}"}

dest_dir = get_taxonomy_path(asset_id, asset_type, categories)
dest_dir.mkdir(parents=True, exist_ok=True)

downloaded_files: list[str] = []

if asset_type == "models":
# Structure: {"gltf": {"1k": {"glb": {"url": "...", "size": N}, ...}, ...}, ...}
gltf_section = files_data.get("gltf", {})
res_section = gltf_section.get(resolution) or next(iter(gltf_section.values()), None)
if not res_section:
return {"error": f"No gltf files found for {asset_id}"}

glb_info = res_section.get("glb")
if not glb_info:
return {"error": f"No GLB variant found for {asset_id} at resolution {resolution}"}

url = glb_info["url"]
dest_file = dest_dir / f"{asset_id}.glb"
try:
data = _download_bytes(url)
except httpx.HTTPError as exc:
return {"error": f"Download failed: {exc}"}
dest_file.write_bytes(data)
downloaded_files.append(str(dest_file))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The download_ph_asset function uses the user-supplied asset_id to construct local filesystem paths without sanitization. An attacker can provide an asset_id containing path traversal sequences (e.g., ../../../../tmp/evil) to write downloaded assets to arbitrary locations on the system. It is recommended to sanitize asset_id using os.path.basename() or validate that the resulting path is within the expected subdirectory of ASSETS_ROOT.

Comment on lines +126 to +256
def get_asset_info(path: str) -> dict:
"""
Get full metadata for a specific asset by its absolute path.

Args:
path: Absolute path to the .glb file

Returns:
Full asset record including mesh stats, category, source, preview path.
Returns {'error': '...'} if not found.
"""
conn = get_connection()
asset = get_asset(conn, path)
conn.close()
if asset is None:
# Try live read if not in catalog
from .glb_reader import read_glb_stats
if Path(path).exists():
stats = read_glb_stats(path)
return {"path": path, "name": Path(path).stem, "note": "not in catalog", **(stats or {})}
return {"error": f"Asset not found: {path}"}
return _format_asset(asset)


@mcp.tool()
def copy_asset(src_path: str, dest_dir: str, rename: Optional[str] = None) -> dict:
"""
Copy a GLB asset to a destination directory (e.g. into your game project).

Args:
src_path: Absolute path to the source .glb file
dest_dir: Destination directory (will be created if needed)
rename: Optional new filename without extension (default: keep original name)

Returns:
{'dest': '/path/to/copied.glb', 'size_kb': 42}
"""
src = Path(src_path)
if not src.exists():
return {"error": f"Source not found: {src_path}"}
dest = Path(dest_dir)
dest.mkdir(parents=True, exist_ok=True)
out_name = (rename or src.stem) + ".glb"
out_path = dest / out_name
shutil.copy2(src, out_path)
return {"dest": str(out_path), "size_kb": out_path.stat().st_size // 1024}


@mcp.tool()
def generate_preview(glb_path: str, output_dir: Optional[str] = None) -> dict:
"""
Render a 512×512 PNG thumbnail of a GLB model using headless Blender.

The preview is saved alongside the GLB as <name>.png (or in output_dir).
Uses a standardized 3/4-view camera with 3-point studio lighting.

Args:
glb_path: Absolute path to the .glb file
output_dir: Optional output directory (default: same dir as GLB)

Returns:
{'preview_path': '/path/to/preview.png'} or {'error': '...'}
"""
src = Path(glb_path)
if not src.exists():
return {"error": f"GLB not found: {glb_path}"}

if not HAS_BLENDER:
return {"error": "Blender not available. Install bpy (pip install 'game-asset-mcp[blender]') or set BLENDER env var."}

if not PREVIEW_SCRIPT.exists():
return {"error": f"Preview script not found: {PREVIEW_SCRIPT}"}

out_dir = Path(output_dir) if output_dir else src.parent
out_dir.mkdir(parents=True, exist_ok=True)
out_png = out_dir / (src.stem + ".png")

result = subprocess.run(
[
str(BLENDER), "--background", "--python", str(PREVIEW_SCRIPT),
"--", "--input", str(src), "--output", str(out_png),
],
capture_output=True, text=True, timeout=60
)

if out_png.exists():
# Update catalog with preview path
conn = get_connection()
conn.execute(
"UPDATE assets SET preview_path=? WHERE path=?",
(str(out_png), str(src))
)
conn.commit()
conn.close()
return {"preview_path": str(out_png)}
else:
return {"error": "Blender render failed", "stderr": result.stderr[-500:]}


@mcp.tool()
def get_preview(glb_path: str) -> dict:
"""
Return the path to an existing PNG preview for a GLB, if available.

Does NOT generate a new preview — use generate_preview() for that.

Args:
glb_path: Absolute path to the .glb file

Returns:
{'preview_path': '/path/to/preview.png'} or {'preview_path': None, 'note': '...'}
"""
src = Path(glb_path)
# Check common preview locations
for candidate in [
src.with_suffix(".png"),
src.parent / "previews" / (src.stem + ".png"),
]:
if candidate.exists():
return {"preview_path": str(candidate)}

# Check catalog
conn = get_connection()
row = conn.execute(
"SELECT preview_path FROM assets WHERE path=?", (str(src),)
).fetchone()
conn.close()
if row and row["preview_path"] and Path(row["preview_path"]).exists():
return {"preview_path": row["preview_path"]}

return {"preview_path": None, "note": "No preview. Run generate_preview() to create one."}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Several tools (get_asset_info, generate_preview, get_preview) accept absolute paths as arguments and perform filesystem operations without validating them against ASSETS_ROOT. This allows for information disclosure (checking file existence) and potentially other path traversal attacks. All user-supplied paths should be resolved and validated to ensure they reside within the ASSETS_ROOT directory.


def upsert_asset(conn: sqlite3.Connection, asset: dict) -> int:
"""Insert or replace an asset record. Returns the row id."""
import json, time
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Imports should generally be at the top of the file, as per PEP 8 guidelines. Placing import json, time inside the function can lead to repeated import overhead if the function is called multiple times and can make dependencies harder to track.

import json
import time

if has_textures is not None:
sql += " AND a.has_embedded_textures = ?"
params.append(1 if has_textures else 0)
sql += f" LIMIT {max_results}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For security and consistency, it's best to avoid using f-strings to format values into SQL queries, even for numbers. Please use parameter substitution (?) for the LIMIT clause. You can append max_results to your params list before executing the query.

try:
with open(path, "rb") as f:
return f.read(4) == b"glTF"
except Exception:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a broad except Exception: can mask issues. It's better to catch a more specific exception, like IOError, for file operations.

Suggested change
except Exception:
except IOError:

import os
import re
import time
import json
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The json module is imported but never used in this file. It should be removed to keep the code clean and avoid confusion.


if not dry_run:
rebuild_fts(conn)
elapsed = time.time() - run_start
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable elapsed is assigned a value but is never used. It should be removed to avoid dead code.

DEFAULT_MODEL_PATH = "3DLowPoly/Props/Misc/polyhaven"

# Maps used by texture download: keys are PolyHaven map keys → local filename suffixes
TEXTURE_MAP_KEYS = ["diffuse", "rough", "metal", "nor_gl", "ao", "disp", "arm"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable TEXTURE_MAP_KEYS is defined but appears to be unused within the module. If it's not needed, it should be removed to avoid confusion and dead code.

sqlite_vec.load(conn)
conn.enable_load_extension(False)
return True
except Exception:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a broad except Exception: can mask unexpected errors and make debugging more difficult. It's better to catch specific exceptions that you expect to handle. This pattern appears in several places in this file.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

🧹 Nitpick comments (11)
packages/game-asset-mcp/pyproject.toml (1)

53-60: The all extra duplicates core dependency.

fastmcp>=3.0 is already in core dependencies (line 32), so including it again in all is redundant.

♻️ Suggested fix
 all = [
-    "fastmcp>=3.0",
     "httpx>=0.27",
     "sqlite-vec>=0.1",
     "sentence-transformers>=3.0",
     "langchain-community>=0.3",
     "langchain>=0.3",
 ]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/pyproject.toml` around lines 53 - 60, The extras
"all" list duplicates the already-declared core dependency fastmcp (present in
dependencies), so edit the pyproject.toml extras section to remove
"fastmcp>=3.0" from the all array; keep the other extras ("httpx>=0.27",
"sqlite-vec>=0.1", "sentence-transformers>=3.0", "langchain-community>=0.3",
"langchain>=0.3") unchanged so the all extra aggregates only non-core packages.
packages/game-asset-mcp/project.json (1)

13-16: Consider adding --fix flag to lint command for consistency.

The coding guidelines mention using uvx ruff check --fix . for linting. The current command doesn't include --fix, which is fine for CI (where auto-fixing may not be desired), but consider documenting this distinction.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/project.json` around lines 13 - 16, Update the lint
command in project.json to align with the coding guideline by either adding the
--fix flag to the existing "lint" command (change the "command" value for the
"lint" entry from "uvx ruff check {projectRoot}" to include "--fix") or add a
separate script (e.g., "lint:fix") that runs "uvx ruff check --fix
{projectRoot}" while keeping the CI-friendly non-fix "lint" target for
automation; edit the "lint" object in project.json to implement your chosen
approach so the intent is explicit.
packages/game-asset-mcp/tests/test_config.py (1)

16-21: Test creates new Settings() instead of using cached get_settings().

The test calls reset_settings() then Settings() directly. This tests that a fresh Settings instance reads from environment, but doesn't verify that get_settings() returns the updated settings after cache clear. If the intent is to test the caching mechanism, use get_settings().

♻️ Test the cached singleton behavior
 def test_settings_env_override(monkeypatch):
     monkeypatch.setenv("GAME_ASSET_ASSETS_ROOT", "/tmp/test-assets")
     reset_settings()
-    s = Settings()
+    from game_asset_mcp.config import get_settings
+    s = get_settings()
     assert s.assets_root == Path("/tmp/test-assets")
     reset_settings()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/tests/test_config.py` around lines 16 - 21, The test
is instantiating Settings() directly instead of exercising the cached singleton
returned by get_settings(); update test_settings_env_override to call
reset_settings(), set the env var, then call get_settings() (not Settings()) and
assert the assets_root equals Path("/tmp/test-assets"), and keep the
reset_settings() cleanup—this ensures the cache-clearing behavior and singleton
retrieval (reset_settings, get_settings, Settings, test_settings_env_override).
packages/game-asset-mcp/tests/test_server_tools.py (1)

219-235: Test test_no_preview_returns_none uses invalid GLB bytes.

Line 223 writes b"fake" which is not valid GLB. While this works for testing the "no preview found" path, it means get_preview is being tested with malformed input. Consider using minimal_glb_bytes fixture for consistency.

♻️ Use valid GLB bytes
-    def test_no_preview_returns_none(self, tmp_path: Path, tmp_db: Path, monkeypatch) -> None:
+    def test_no_preview_returns_none(self, tmp_path: Path, tmp_db: Path, minimal_glb_bytes: bytes, monkeypatch) -> None:
         """get_preview for a GLB with no preview should return preview_path=None."""
         glb = tmp_path / "model.glb"
-        glb.write_bytes(b"fake")
+        glb.write_bytes(minimal_glb_bytes)
         _patch_db(monkeypatch, tmp_db)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/tests/test_server_tools.py` around lines 219 - 235,
The test TestGetPreview.test_no_preview_returns_none writes invalid GLB bytes
(b"fake"); update the test to use the existing minimal_glb_bytes fixture instead
of raw b"fake" so get_preview receives a valid GLB blob—add a minimal_glb_bytes
parameter to the test signature and call glb.write_bytes(minimal_glb_bytes)
(leave TestGetPreview.test_existing_preview_sibling unchanged or similarly
switch it to minimal_glb_bytes if desired) so the test exercises the "no
preview" path with a well-formed GLB.
packages/game-asset-mcp/src/game_asset_mcp/render_preview.py (3)

24-33: Argument parsing may produce unclear error when invoked incorrectly.

If the script is invoked without the -- separator (e.g., direct python render_preview.py), args will be an empty list and argparse will fail on the required --input argument. This is technically correct behavior, but the error message won't indicate the correct invocation format.

♻️ Add clearer error handling
 def parse_args():
     if "--" in sys.argv:
         args = sys.argv[sys.argv.index("--") + 1:]
     else:
-        args = []
+        args = sys.argv[1:]  # Allow direct invocation for testing
     p = argparse.ArgumentParser()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/render_preview.py` around lines 24
- 33, The parse_args function assumes arguments are only passed after a "--"
separator and silently supplies an empty args list otherwise, causing argparse
to fail with an unclear message; update parse_args to either (a) accept regular
invocation by using sys.argv[1:] when "--" is absent, or (b) explicitly check
for the missing "--" and call parser.error or raise SystemExit with a clear
message explaining the required invocation format. Modify the code around
parse_args (the sys.argv handling and the return of p.parse_args(args)) to
implement one of these behaviors so users get a clear, actionable error or
normal CLI parsing; reference the parse_args function and the
argparse.ArgumentParser usage when making the change.

17-21: Unused HAS_BPY variable.

HAS_BPY is defined but never referenced. Consider removing it or using it to guard against running outside Blender.

♻️ Option 1: Remove unused code
-try:
-    import bpy  # noqa: F401
-    HAS_BPY = True
-except ImportError:
-    HAS_BPY = False
♻️ Option 2: Use it to guard main()
 if __name__ == "__main__":
+    if not HAS_BPY:
+        print("[preview] ERROR: Must be run inside Blender (blender --background --python ...)")
+        sys.exit(1)
     main()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/render_preview.py` around lines 17
- 21, The constant HAS_BPY is defined after the conditional import of bpy but
never used; either remove the HAS_BPY variable and keep the try/except import
(or let the ImportError propagate) or use HAS_BPY to guard Blender-specific
execution by checking it at module run-time or inside the main() entrypoint
(e.g., return/raise or skip Blender-only logic when not HAS_BPY). Update the
code around the import bpy try/except and the main() (or module-level execution)
to implement one of these two options and remove the unused symbol if you choose
the first option.

113-124: Consider EEVEE fallback for systems without Cycles/GPU support.

CYCLES requires either CPU rendering (slow) or GPU support. For preview generation, EEVEE would be faster and more universally available. This could fail silently or take a very long time on systems without GPU acceleration.

♻️ Use EEVEE for faster preview rendering
     # Render settings
     scene = bpy.context.scene
-    scene.render.engine = "CYCLES"
-    scene.cycles.samples = 32
-    scene.cycles.use_denoising = True
+    scene.render.engine = "BLENDER_EEVEE_NEXT"  # Faster for previews
+    scene.eevee.taa_render_samples = 32
     scene.render.resolution_x = args.size
     scene.render.resolution_y = args.size
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/render_preview.py` around lines
113 - 124, The current render always forces scene.render.engine = "CYCLES",
which can be very slow or fail on systems without Cycles/GPU support; update
render_preview.py to attempt to use Cycles but fall back to EEVEE when Cycles
isn't available or raises errors. Wrap the Cycles setup (scene.render.engine =
"CYCLES", scene.cycles.samples, scene.cycles.use_denoising) in a try/except or
capability check (checking for scene.cycles attribute or catching exceptions)
and on failure set scene.render.engine = "BLENDER_EEVEE" (or "EEVEE" if
appropriate in your Blender API) and configure reasonable EEVEE settings
(samples, use_bloom/use_ssr as needed) before calling
bpy.ops.render.render(write_still=True); keep using scene.render.filepath =
os.path.abspath(args.output) and ensure the fallback path is logged or
indicated.
packages/game-asset-mcp/src/game_asset_mcp/polyhaven.py (1)

200-202: Edge case in URL extension parsing.

The extension extraction logic could produce unexpected results for URLs with dots in unexpected positions (e.g., https://example.com/.hidden would yield hidden). While PolyHaven URLs are unlikely to hit this, a more robust approach would be safer.

♻️ More robust extension extraction
             url_path = url.split("?")[0]
-            ext = url_path.rsplit(".", 1)[-1] if "." in url_path else "png"
+            parts = url_path.rsplit(".", 1)
+            ext = parts[-1] if len(parts) == 2 and parts[-1] else "png"
             dest_file = dest_dir / f"{asset_id}_{map_key}.{ext}"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/polyhaven.py` around lines 200 -
202, The current extension parsing (using url.split and rsplit) is brittle for
edge cases like paths with leading dots; update the logic that derives ext from
url (the url_path/ext computation) to parse the URL path with
urllib.parse.urlparse and then use os.path.splitext on the parsed path to get a
proper extension, and if splitext returns an empty extension default to "png";
update any code using url_path and ext in polyhaven.py accordingly (refer to the
variables url, url_path, and ext).
packages/game-asset-mcp/src/game_asset_mcp/search.py (2)

21-22: Unused module-level variable _vec_conn.

The variable _vec_conn is declared but never used anywhere in the module. Consider removing it to avoid confusion.

 _embedder = None
-_vec_conn = None
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/search.py` around lines 21 - 22,
Remove the unused module-level variable declaration _vec_conn from search.py to
avoid confusion; locate the top-level declarations (e.g., _embedder = None and
_vec_conn = None) and delete the _vec_conn line only, leaving _embedder intact
and ensuring no other references to _vec_conn exist elsewhere in the module.

25-33: Repeated import attempts on failure.

If sentence_transformers is not installed, every call to _get_embedder() will re-attempt the import because _embedder remains None. Use a sentinel to distinguish "not yet loaded" from "failed to load."

♻️ Suggested fix
+_NOT_LOADED = object()
-_embedder = None
+_embedder = _NOT_LOADED
 _vec_conn = None


 def _get_embedder():
     global _embedder
-    if _embedder is None:
+    if _embedder is _NOT_LOADED:
         try:
             from sentence_transformers import SentenceTransformer
             _embedder = SentenceTransformer("all-MiniLM-L6-v2")
         except ImportError:
             _embedder = None
     return _embedder
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/search.py` around lines 25 - 33,
The _get_embedder function currently leaves the module-level _embedder as None
on ImportError, causing repeated import attempts; change it to use a sentinel
value or a loaded flag (e.g., define a module-level _UNAVAILABLE = object() or
_embedder_loaded = False) and on ImportError set _embedder = _UNAVAILABLE (or
_embedder_loaded = True) so subsequent calls to _get_embedder() can detect the
failed state and avoid re-attempting the import; update the function's checks to
return None when the sentinel is set and to only try importing when the state
indicates "not yet attempted" (reference: _get_embedder and the _embedder
variable).
packages/game-asset-mcp/src/game_asset_mcp/server.py (1)

423-437: Redundant Path import.

Path is already imported at line 19. Remove the local import at line 425.

     from .polyhaven import download_ph_asset
     from .ingest import ingest
-    from pathlib import Path
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/server.py` around lines 423 - 437,
The file imports Path twice; remove the redundant local import "from pathlib
import Path" in server.py (the duplicate near the download_ph_asset/ingest
block) so only the original import at the top remains; ensure imports around the
download_ph_asset and ingest usage (functions download_ph_asset and ingest)
still resolve after removing the duplicate import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/game-asset-mcp/pyproject.toml`:
- Line 9: The package currently declares the Python requirement via the
pyproject.toml key "requires-python = \">=3.10\"" which conflicts with the
monorepo root requirement (>=3.11) and the guideline to maintain 3.11+
compatibility; update the "requires-python" entry to ">=3.11" to align with the
root (or remove the entry if you intentionally rely on the root constraint) so
the package and monorepo are consistent.

In `@packages/game-asset-mcp/README.md`:
- Around line 144-147: The fenced code block showing the Python calls
search_polyhaven(...) and download_polyhaven_asset(...) is missing a language
specifier; update the block to use a Python fence (add "python" after the
opening ``` fence) so syntax highlighting is applied for the functions
search_polyhaven and download_polyhaven_asset.
- Around line 118-134: The fenced code block in README.md showing the
ASSETS_ROOT directory tree lacks a language specifier; update the block
delimiter from ``` to ```text (or ```plaintext) so the tree is treated as plain
text language (e.g., change the opening fence before "ASSETS_ROOT/" in the
README file to ```text) to satisfy the static analysis rule.

In `@packages/game-asset-mcp/src/game_asset_mcp/catalog.py`:
- Around line 15-19: Replace the hard-coded DB_PATH/DB default with the settings
layer so get_connection and init_db use Settings.catalog_db by default: import
the Settings object from config.py (Settings or get_settings), instantiate or
retrieve the app settings and use its catalog_db value as the default for
DB_PATH or as the default parameter for get_connection (and any init_db
function), falling back to the existing _DEFAULT_DB only if Settings.catalog_db
is unset; update references to DB_PATH so callers via get_connection()/init_db()
follow the GAME_ASSET_* configuration path exposed by Settings.catalog_db.
- Around line 20-23: Before calling sqlite3.connect(db_path) ensure the parent
directory exists: compute the parent with os.path.dirname(db_path) or
Path(db_path).parent and call os.makedirs(..., exist_ok=True) or
.mkdir(parents=True, exist_ok=True) to create the directory, then proceed with
conn = sqlite3.connect(db_path), conn.row_factory = sqlite3.Row and
conn.execute("PRAGMA journal_mode=WAL"); update the function that opens the DB
(the code block using sqlite3.connect, conn.row_factory and conn.execute) to
create the parent directory first.
- Around line 95-108: The ON CONFLICT(path) DO UPDATE SET clause currently only
updates stats-like columns and therefore leaves taxonomy/source fields stale;
change the conflict handler to also update taxonomy and source columns (name,
style, category, pack, source) in the ON CONFLICT(path) DO UPDATE SET list so
re-ingests overwrite those fields, and ensure any FTS index rebuild or trigger
that depends on those columns is invoked/updated after the upsert so the
full-text search reflects updated taxonomy/source values.

In `@packages/game-asset-mcp/src/game_asset_mcp/config.py`:
- Around line 97-116: The [library] values written by wizard.py are never
applied because assets_root and catalog_db are always set from
environment/defaults in the Field default_factory; update config loading so the
TOML config (auto-detected via config_file or the same detection used by
get_taxonomy) is read and its [library] keys override the defaults before the
Pydantic fields are finalized. Concretely: add a startup/config loader that
reads the TOML (reusing get_taxonomy's detection logic), parse
library.assets_root and library.catalog_db (and library.blender if present), and
assign those values to the Configuration object or provide them as explicit
Field defaults/validators for assets_root, catalog_db, and blender so values
from wizard.py are honored after restart.
- Around line 71-77: TaxonomyConfig.from_dict currently assigns skip_dirs
directly from data.get("skip_dirs"), but wizard.py writes skip_dirs as a dict
with key "dirs", so TaxonomyConfig.skip_dirs becomes {"dirs"} instead of the
actual list; update TaxonomyConfig.from_dict to detect when
data.get("skip_dirs") is a dict and extract the list via skip_dirs =
data.get("skip_dirs").get("dirs") (or accept a list unchanged), coerce to a
list/default to [] if missing, and pass that normalized list into the cls(...)
call so skipping logic (e.g., for .git, node_modules, Textures) works correctly.

In `@packages/game-asset-mcp/src/game_asset_mcp/ingest.py`:
- Around line 104-123: The current `known` dict is overloaded and causes
incorrect behavior when `force=True` and uses coarse KB rounding for skips;
split responsibilities by loading a membership set (e.g., existing_paths =
set(row["path"] for row in conn.execute("SELECT path FROM assets"))) that is
always populated and used to decide adds/updates/stale pruning, and separately
build a fast-path cache (e.g., size_cache mapping path -> st_size or mtime_ns)
used only for skipping when not force; in the ingest loop (symbols: scan_glbs,
glb_path, rel_str, known/current_size_kb) compare exact glb_path.stat().st_size
or .st_mtime_ns against the cache value (not rounded KB) and use rel_str or the
same DB path key consistently, and ensure when force=True you still use
existing_paths for update/stale logic so existing rows aren’t misreported as
adds and deletions can be pruned.

In `@packages/game-asset-mcp/src/game_asset_mcp/search.py`:
- Around line 157-171: The current search uses sqlite-vec MATCH with k = ? (in
assets_vec v) but applies style/category filters after fetching, so using params
= [vec_bytes, limit * 2] can return fewer than limit when many top-k are
filtered; update the logic in the search function to increase the pre-filter
fetch multiplier when style or category is provided (e.g., use a higher
multiplier like 4–5 or compute multiplier = min(maxMultiplier, ceil(limit *
factor))) and pass that larger k to the MATCH parameter (vec_bytes, computed_k)
while still applying the post-filters (style/category); also add a sensible
upper bound to computed_k to avoid excessively large queries.
- Around line 198-203: The connection returned by get_connection (conn) is not
closed if fts_search or vec_search raises; wrap the usage of conn in a
try/finally (or use a context manager on get_connection if it supports
__enter__/__exit__) so conn.close() is guaranteed to run; update the block
around calls to fts_search(...) and vec_search(...) to ensure conn.close() is
called in finally (or use "with get_connection(...) as conn:" and remove the
explicit close) while keeping the same arguments (query, style, category,
has_armature, has_textures, limit).

In `@packages/game-asset-mcp/src/game_asset_mcp/server.py`:
- Around line 440-455: The run_ingest docstring is missing the 'removed' and
'errors' keys returned by ingest(); update the run_ingest docstring (the
function run_ingest in server.py) to list all returned keys: 'added', 'updated',
'skipped', 'total_scanned', plus 'removed' (count of stale catalog entries
pruned) and 'errors' (count of parse/ingest failures), and include brief
descriptions for each to match ingest()'s actual return structure.
- Around line 24-25: The module imports an unused symbol fts_search and also
imports list_categories from .catalog which is then shadowed by a local function
named list_categories; remove fts_search from the import list and resolve the
name collision by removing list_categories from the from .catalog import
(get_connection, get_asset, list_categories, get_stats, DB_PATH) so the local
function list_categories is the one used (or alternatively rename the local
function if you intended to call the catalog version); update any references
accordingly so only the needed symbols (e.g., get_connection, get_asset,
get_stats, DB_PATH) remain imported.

In `@packages/game-asset-mcp/tests/e2e/conftest.py`:
- Around line 18-31: The test fixture real_assets_root accepts either
ASSETS_ROOT or GAME_ASSET_ASSETS_ROOT but config.py only reads ASSETS_ROOT
directly, causing inconsistent behavior; update config.py to mirror the fixture
by either honoring the env_prefix (e.g., use your configuration loader with
env_prefix="GAME_ASSET_") or add a fallback when reading the asset root (check
GAME_ASSET_ASSETS_ROOT if ASSETS_ROOT is missing) so both real_assets_root and
the application use the same environment variable names; locate the code that
currently calls os.environ.get("ASSETS_ROOT") in config.py and modify it to use
the same lookup logic as the real_assets_root fixture.

In `@packages/game-asset-mcp/tests/e2e/test_ingest_e2e.py`:
- Around line 19-51: Tests rely on a session-scoped e2e_db and therefore have
implicit ordering; make each test self-contained by ensuring the DB is
initialized before use — call init_db(e2e_db) at the start of
test_ingest_writes_records and test_second_ingest_skips_unchanged (in addition
to the existing call in test_ingest_dry_run_reports_assets) or replace that with
a small setup fixture that calls init_db(e2e_db) and is applied to each test;
reference functions init_db, ingest, get_connection, get_stats and
list_categories to locate where to add the initialization.

---

Nitpick comments:
In `@packages/game-asset-mcp/project.json`:
- Around line 13-16: Update the lint command in project.json to align with the
coding guideline by either adding the --fix flag to the existing "lint" command
(change the "command" value for the "lint" entry from "uvx ruff check
{projectRoot}" to include "--fix") or add a separate script (e.g., "lint:fix")
that runs "uvx ruff check --fix {projectRoot}" while keeping the CI-friendly
non-fix "lint" target for automation; edit the "lint" object in project.json to
implement your chosen approach so the intent is explicit.

In `@packages/game-asset-mcp/pyproject.toml`:
- Around line 53-60: The extras "all" list duplicates the already-declared core
dependency fastmcp (present in dependencies), so edit the pyproject.toml extras
section to remove "fastmcp>=3.0" from the all array; keep the other extras
("httpx>=0.27", "sqlite-vec>=0.1", "sentence-transformers>=3.0",
"langchain-community>=0.3", "langchain>=0.3") unchanged so the all extra
aggregates only non-core packages.

In `@packages/game-asset-mcp/src/game_asset_mcp/polyhaven.py`:
- Around line 200-202: The current extension parsing (using url.split and
rsplit) is brittle for edge cases like paths with leading dots; update the logic
that derives ext from url (the url_path/ext computation) to parse the URL path
with urllib.parse.urlparse and then use os.path.splitext on the parsed path to
get a proper extension, and if splitext returns an empty extension default to
"png"; update any code using url_path and ext in polyhaven.py accordingly (refer
to the variables url, url_path, and ext).

In `@packages/game-asset-mcp/src/game_asset_mcp/render_preview.py`:
- Around line 24-33: The parse_args function assumes arguments are only passed
after a "--" separator and silently supplies an empty args list otherwise,
causing argparse to fail with an unclear message; update parse_args to either
(a) accept regular invocation by using sys.argv[1:] when "--" is absent, or (b)
explicitly check for the missing "--" and call parser.error or raise SystemExit
with a clear message explaining the required invocation format. Modify the code
around parse_args (the sys.argv handling and the return of p.parse_args(args))
to implement one of these behaviors so users get a clear, actionable error or
normal CLI parsing; reference the parse_args function and the
argparse.ArgumentParser usage when making the change.
- Around line 17-21: The constant HAS_BPY is defined after the conditional
import of bpy but never used; either remove the HAS_BPY variable and keep the
try/except import (or let the ImportError propagate) or use HAS_BPY to guard
Blender-specific execution by checking it at module run-time or inside the
main() entrypoint (e.g., return/raise or skip Blender-only logic when not
HAS_BPY). Update the code around the import bpy try/except and the main() (or
module-level execution) to implement one of these two options and remove the
unused symbol if you choose the first option.
- Around line 113-124: The current render always forces scene.render.engine =
"CYCLES", which can be very slow or fail on systems without Cycles/GPU support;
update render_preview.py to attempt to use Cycles but fall back to EEVEE when
Cycles isn't available or raises errors. Wrap the Cycles setup
(scene.render.engine = "CYCLES", scene.cycles.samples,
scene.cycles.use_denoising) in a try/except or capability check (checking for
scene.cycles attribute or catching exceptions) and on failure set
scene.render.engine = "BLENDER_EEVEE" (or "EEVEE" if appropriate in your Blender
API) and configure reasonable EEVEE settings (samples, use_bloom/use_ssr as
needed) before calling bpy.ops.render.render(write_still=True); keep using
scene.render.filepath = os.path.abspath(args.output) and ensure the fallback
path is logged or indicated.

In `@packages/game-asset-mcp/src/game_asset_mcp/search.py`:
- Around line 21-22: Remove the unused module-level variable declaration
_vec_conn from search.py to avoid confusion; locate the top-level declarations
(e.g., _embedder = None and _vec_conn = None) and delete the _vec_conn line
only, leaving _embedder intact and ensuring no other references to _vec_conn
exist elsewhere in the module.
- Around line 25-33: The _get_embedder function currently leaves the
module-level _embedder as None on ImportError, causing repeated import attempts;
change it to use a sentinel value or a loaded flag (e.g., define a module-level
_UNAVAILABLE = object() or _embedder_loaded = False) and on ImportError set
_embedder = _UNAVAILABLE (or _embedder_loaded = True) so subsequent calls to
_get_embedder() can detect the failed state and avoid re-attempting the import;
update the function's checks to return None when the sentinel is set and to only
try importing when the state indicates "not yet attempted" (reference:
_get_embedder and the _embedder variable).

In `@packages/game-asset-mcp/src/game_asset_mcp/server.py`:
- Around line 423-437: The file imports Path twice; remove the redundant local
import "from pathlib import Path" in server.py (the duplicate near the
download_ph_asset/ingest block) so only the original import at the top remains;
ensure imports around the download_ph_asset and ingest usage (functions
download_ph_asset and ingest) still resolve after removing the duplicate import.

In `@packages/game-asset-mcp/tests/test_config.py`:
- Around line 16-21: The test is instantiating Settings() directly instead of
exercising the cached singleton returned by get_settings(); update
test_settings_env_override to call reset_settings(), set the env var, then call
get_settings() (not Settings()) and assert the assets_root equals
Path("/tmp/test-assets"), and keep the reset_settings() cleanup—this ensures the
cache-clearing behavior and singleton retrieval (reset_settings, get_settings,
Settings, test_settings_env_override).

In `@packages/game-asset-mcp/tests/test_server_tools.py`:
- Around line 219-235: The test TestGetPreview.test_no_preview_returns_none
writes invalid GLB bytes (b"fake"); update the test to use the existing
minimal_glb_bytes fixture instead of raw b"fake" so get_preview receives a valid
GLB blob—add a minimal_glb_bytes parameter to the test signature and call
glb.write_bytes(minimal_glb_bytes) (leave
TestGetPreview.test_existing_preview_sibling unchanged or similarly switch it to
minimal_glb_bytes if desired) so the test exercises the "no preview" path with a
well-formed GLB.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 327cddf4-859b-4d1d-9c36-f02abefb2544

📥 Commits

Reviewing files that changed from the base of the PR and between 0733b6d and 45d86c9.

📒 Files selected for processing (28)
  • README.md
  • packages/game-asset-mcp/CHANGELOG.md
  • packages/game-asset-mcp/README.md
  • packages/game-asset-mcp/project.json
  • packages/game-asset-mcp/pyproject.toml
  • packages/game-asset-mcp/src/game_asset_mcp/__init__.py
  • packages/game-asset-mcp/src/game_asset_mcp/catalog.py
  • packages/game-asset-mcp/src/game_asset_mcp/config.py
  • packages/game-asset-mcp/src/game_asset_mcp/glb_reader.py
  • packages/game-asset-mcp/src/game_asset_mcp/ingest.py
  • packages/game-asset-mcp/src/game_asset_mcp/polyhaven.py
  • packages/game-asset-mcp/src/game_asset_mcp/render_preview.py
  • packages/game-asset-mcp/src/game_asset_mcp/search.py
  • packages/game-asset-mcp/src/game_asset_mcp/server.py
  • packages/game-asset-mcp/src/game_asset_mcp/wizard.py
  • packages/game-asset-mcp/tests/__init__.py
  • packages/game-asset-mcp/tests/conftest.py
  • packages/game-asset-mcp/tests/e2e/__init__.py
  • packages/game-asset-mcp/tests/e2e/conftest.py
  • packages/game-asset-mcp/tests/e2e/test_ingest_e2e.py
  • packages/game-asset-mcp/tests/test_catalog.py
  • packages/game-asset-mcp/tests/test_config.py
  • packages/game-asset-mcp/tests/test_glb_reader.py
  • packages/game-asset-mcp/tests/test_ingest.py
  • packages/game-asset-mcp/tests/test_polyhaven.py
  • packages/game-asset-mcp/tests/test_server_tools.py
  • packages/game-asset-mcp/tests/test_wizard.py
  • pyproject.toml

name = "game-asset-mcp"
version = "0.1.0"
description = "MCP server and catalog library for local 3D game asset libraries — search, browse, and manage GLB/GLTF assets"
requires-python = ">=3.10"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Python version mismatch with monorepo root.

This package declares requires-python = ">=3.10", but the root pyproject.toml requires >=3.11. This inconsistency could cause confusion. The coding guidelines also specify "Maintain Python 3.11+ compatibility."

Consider aligning to >=3.11 for consistency, or document why this package specifically needs 3.10 support.

📝 Suggested fix for consistency
-requires-python = ">=3.10"
+requires-python = ">=3.11"

If keeping 3.10 support:

-    "tomli>=2.0; python_version < \"3.11\"",  # tomllib backport

This line can be removed if requiring 3.11+.

As per coding guidelines: "Maintain Python 3.11+ compatibility"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
requires-python = ">=3.10"
requires-python = ">=3.11"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/pyproject.toml` at line 9, The package currently
declares the Python requirement via the pyproject.toml key "requires-python =
\">=3.10\"" which conflicts with the monorepo root requirement (>=3.11) and the
guideline to maintain 3.11+ compatibility; update the "requires-python" entry to
">=3.11" to align with the root (or remove the entry if you intentionally rely
on the root constraint) so the package and monorepo are consistent.

Comment on lines +118 to +134
```
ASSETS_ROOT/
├── 3DLowPoly/
│ ├── Characters/
│ │ ├── Animated/
│ │ │ └── <pack-name>/ ← GLBs here
│ │ └── Animals/
│ ├── Props/
│ │ └── Weapons/
│ └── Environment/
│ └── Nature/
├── 3DPSX/
│ └── ...
└── 2DPhotorealistic/
├── HDRIs/
└── Textures/
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add language specifier to fenced code block.

The static analysis tool flags this block as missing a language specifier. For directory tree structures, use text or plaintext.

📝 Suggested fix
-```
+```text
 ASSETS_ROOT/
 ├── 3DLowPoly/
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
```
ASSETS_ROOT/
├── 3DLowPoly/
│ ├── Characters/
│ │ ├── Animated/
│ │ │ └── <pack-name>/ ← GLBs here
│ │ └── Animals/
│ ├── Props/
│ │ └── Weapons/
│ └── Environment/
│ └── Nature/
├── 3DPSX/
│ └── ...
└── 2DPhotorealistic/
├── HDRIs/
└── Textures/
```
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 118-118: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/README.md` around lines 118 - 134, The fenced code
block in README.md showing the ASSETS_ROOT directory tree lacks a language
specifier; update the block delimiter from ``` to ```text (or ```plaintext) so
the tree is treated as plain text language (e.g., change the opening fence
before "ASSETS_ROOT/" in the README file to ```text) to satisfy the static
analysis rule.

Comment on lines +144 to +147
```
search_polyhaven("oak tree", asset_type="models")
download_polyhaven_asset("oak_tree", asset_type="models", resolution="1k")
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add language specifier to fenced code block.

This code block shows Python function calls but lacks a language specifier.

📝 Suggested fix
-```
+```python
 search_polyhaven("oak tree", asset_type="models")
 download_polyhaven_asset("oak_tree", asset_type="models", resolution="1k")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
```
search_polyhaven("oak tree", asset_type="models")
download_polyhaven_asset("oak_tree", asset_type="models", resolution="1k")
```
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 144-144: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/README.md` around lines 144 - 147, The fenced code
block showing the Python calls search_polyhaven(...) and
download_polyhaven_asset(...) is missing a language specifier; update the block
to use a Python fence (add "python" after the opening ``` fence) so syntax
highlighting is applied for the functions search_polyhaven and
download_polyhaven_asset.

Comment on lines +15 to +19
_DEFAULT_DB = Path.home() / ".local" / "share" / "game-asset-mcp" / "catalog.db"
DB_PATH = Path(os.environ.get("CATALOG_DB", str(_DEFAULT_DB)))


def get_connection(db_path: Path = DB_PATH) -> sqlite3.Connection:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use the settings layer for the default DB path.

This default only consults CATALOG_DB, while config.py exposes Settings.catalog_db and documents the GAME_ASSET_* config path. Callers that rely on get_connection() / init_db() defaults can therefore read and write a different database than the CLI/config flow.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/catalog.py` around lines 15 - 19,
Replace the hard-coded DB_PATH/DB default with the settings layer so
get_connection and init_db use Settings.catalog_db by default: import the
Settings object from config.py (Settings or get_settings), instantiate or
retrieve the app settings and use its catalog_db value as the default for
DB_PATH or as the default parameter for get_connection (and any init_db
function), falling back to the existing _DEFAULT_DB only if Settings.catalog_db
is unset; update references to DB_PATH so callers via get_connection()/init_db()
follow the GAME_ASSET_* configuration path exposed by Settings.catalog_db.

Comment on lines +20 to +23
conn = sqlite3.connect(db_path)
conn.row_factory = sqlite3.Row
conn.execute("PRAGMA journal_mode=WAL")
return conn
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Create the parent directory before opening SQLite.

sqlite3.connect() will create the file, but not ~/.local/share/game-asset-mcp/. On a clean machine the default path fails until that directory is created manually.

Possible fix
 def get_connection(db_path: Path = DB_PATH) -> sqlite3.Connection:
+    db_path.parent.mkdir(parents=True, exist_ok=True)
     conn = sqlite3.connect(db_path)
     conn.row_factory = sqlite3.Row
     conn.execute("PRAGMA journal_mode=WAL")
     return conn
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
conn = sqlite3.connect(db_path)
conn.row_factory = sqlite3.Row
conn.execute("PRAGMA journal_mode=WAL")
return conn
db_path.parent.mkdir(parents=True, exist_ok=True)
conn = sqlite3.connect(db_path)
conn.row_factory = sqlite3.Row
conn.execute("PRAGMA journal_mode=WAL")
return conn
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/catalog.py` around lines 20 - 23,
Before calling sqlite3.connect(db_path) ensure the parent directory exists:
compute the parent with os.path.dirname(db_path) or Path(db_path).parent and
call os.makedirs(..., exist_ok=True) or .mkdir(parents=True, exist_ok=True) to
create the directory, then proceed with conn = sqlite3.connect(db_path),
conn.row_factory = sqlite3.Row and conn.execute("PRAGMA journal_mode=WAL");
update the function that opens the DB (the code block using sqlite3.connect,
conn.row_factory and conn.execute) to create the parent directory first.

Comment on lines +24 to +25
from .catalog import get_connection, get_asset, list_categories, get_stats, DB_PATH
from .search import hybrid_search, fts_search
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unused import fts_search and name collision with list_categories.

  1. fts_search is imported but never used in this module.
  2. list_categories is imported from .catalog at line 24, but a function with the same name is defined at line 99, shadowing the import.
♻️ Suggested fix
-from .catalog import get_connection, get_asset, list_categories, get_stats, DB_PATH
+from .catalog import get_connection, get_asset, get_stats, DB_PATH
-from .search import hybrid_search, fts_search
+from .search import hybrid_search
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/server.py` around lines 24 - 25,
The module imports an unused symbol fts_search and also imports list_categories
from .catalog which is then shadowed by a local function named list_categories;
remove fts_search from the import list and resolve the name collision by
removing list_categories from the from .catalog import (get_connection,
get_asset, list_categories, get_stats, DB_PATH) so the local function
list_categories is the one used (or alternatively rename the local function if
you intended to call the catalog version); update any references accordingly so
only the needed symbols (e.g., get_connection, get_asset, get_stats, DB_PATH)
remain imported.

Comment on lines +440 to +455
@mcp.tool()
def run_ingest(force: bool = False) -> dict:
"""
Re-scan the asset library and update the catalog database.

Only re-processes files whose size has changed (unless force=True).

Args:
force: Re-ingest all files even if unchanged

Returns:
{'added': N, 'updated': N, 'skipped': N, 'total_scanned': N}
"""
from .ingest import ingest
result = ingest(force=force)
return result
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Docstring incomplete - missing removed and errors keys.

Per ingest.py (lines 212-220), ingest() returns removed (count of stale entries pruned) and errors (count of parse failures) in addition to the documented keys. Update the docstring to reflect the full response.

📝 Suggested fix
     Returns:
-        {'added': N, 'updated': N, 'skipped': N, 'total_scanned': N}
+        {'added': N, 'updated': N, 'skipped': N, 'total_scanned': N, 'removed': N, 'errors': N}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/src/game_asset_mcp/server.py` around lines 440 - 455,
The run_ingest docstring is missing the 'removed' and 'errors' keys returned by
ingest(); update the run_ingest docstring (the function run_ingest in server.py)
to list all returned keys: 'added', 'updated', 'skipped', 'total_scanned', plus
'removed' (count of stale catalog entries pruned) and 'errors' (count of
parse/ingest failures), and include brief descriptions for each to match
ingest()'s actual return structure.

Comment on lines +103 to +130
_CONFIG_DIR.mkdir(parents=True, exist_ok=True)

style_map_toml = "\n".join(f'"{k}" = "{v}"' for k, v in style_map.items())
source_hints_toml = "\n".join(f'{k} = "{v}"' for k, v in default_hints.items())

config_content = textwrap.dedent(f"""\
# game-asset-mcp configuration
# Edit to customize your asset library setup.
# Full docs: https://github.com/jbcom/agentic/tree/main/packages/game-asset-mcp

[library]
assets_root = "{root}"
catalog_db = "{db_str}"

[taxonomy.style_map]
# Maps top-level directory prefix → style label
{style_map_toml}

[taxonomy.source_hints]
# Maps lowercase path keywords → source name
{source_hints_toml}

[taxonomy.skip_dirs]
# Directory names to skip during scan
dirs = ["_Archive", "__pycache__", ".git", "node_modules", "Textures", "textures"]
""")

_CONFIG_FILE.write_text(config_content)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Serialize TOML values safely instead of interpolating raw paths.

root, db_str, and generated style names are inserted verbatim into quoted TOML strings here. Backslashes or quotes in those values will produce an unreadable config file, and get_taxonomy() currently falls back to defaults on parse failure. Please also write the file as UTF-8 explicitly.

Possible fix
+import json
...
+    def _toml_string(value: str) -> str:
+        return json.dumps(value)
+
-    style_map_toml = "\n".join(f'"{k}" = "{v}"' for k, v in style_map.items())
-    source_hints_toml = "\n".join(f'{k} = "{v}"' for k, v in default_hints.items())
+    style_map_toml = "\n".join(
+        f"{_toml_string(k)} = {_toml_string(v)}" for k, v in style_map.items()
+    )
+    source_hints_toml = "\n".join(
+        f"{k} = {_toml_string(v)}" for k, v in default_hints.items()
+    )
...
-        assets_root = "{root}"
-        catalog_db = "{db_str}"
+        assets_root = {_toml_string(str(root))}
+        catalog_db = {_toml_string(db_str)}
...
-    _CONFIG_FILE.write_text(config_content)
+    _CONFIG_FILE.write_text(config_content, encoding="utf-8")

Comment on lines +18 to +31
@pytest.fixture(scope="session")
def real_assets_root() -> Path:
"""
Return the real asset library root from ASSETS_ROOT env var.

Skips the test session if ASSETS_ROOT is not set or doesn't exist.
"""
root_str = os.environ.get("ASSETS_ROOT") or os.environ.get("GAME_ASSET_ASSETS_ROOT")
if not root_str:
pytest.skip("ASSETS_ROOT not set — skipping e2e tests")
root = Path(root_str)
if not root.exists():
pytest.skip(f"ASSETS_ROOT does not exist: {root}")
return root
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify how assets_root is resolved in config.py
rg -n -A5 "assets_root.*Field" packages/game-asset-mcp/src/game_asset_mcp/config.py

Repository: jbcom/agentic

Length of output: 297


Address environment variable inconsistency between fixture and config.

The fixture defensively checks both ASSETS_ROOT and GAME_ASSET_ASSETS_ROOT, but config.py only reads ASSETS_ROOT directly via os.environ.get(), bypassing the env_prefix="GAME_ASSET_" mechanism. This means if only GAME_ASSET_ASSETS_ROOT is set, the fixture will use it but the application config won't. Either standardize on a single variable name or align config.py to respect both environment variables.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/tests/e2e/conftest.py` around lines 18 - 31, The test
fixture real_assets_root accepts either ASSETS_ROOT or GAME_ASSET_ASSETS_ROOT
but config.py only reads ASSETS_ROOT directly, causing inconsistent behavior;
update config.py to mirror the fixture by either honoring the env_prefix (e.g.,
use your configuration loader with env_prefix="GAME_ASSET_") or add a fallback
when reading the asset root (check GAME_ASSET_ASSETS_ROOT if ASSETS_ROOT is
missing) so both real_assets_root and the application use the same environment
variable names; locate the code that currently calls
os.environ.get("ASSETS_ROOT") in config.py and modify it to use the same lookup
logic as the real_assets_root fixture.

Comment on lines +19 to +51
def test_ingest_dry_run_reports_assets(
self, real_assets_root: Path, e2e_db: Path
) -> None:
"""Dry-run ingest should count assets without writing to the DB."""
from game_asset_mcp.catalog import init_db
init_db(e2e_db)
result = ingest(root=real_assets_root, db_path=e2e_db, dry_run=True)
assert result["total_scanned"] > 0
# No records written
conn = get_connection(e2e_db)
stats = get_stats(conn)
conn.close()
assert stats["total"] == 0

def test_ingest_writes_records(
self, real_assets_root: Path, e2e_db: Path
) -> None:
"""Full ingest should populate the DB with asset records."""
result = ingest(root=real_assets_root, db_path=e2e_db)
assert result["added"] > 0 or result["updated"] > 0
conn = get_connection(e2e_db)
stats = get_stats(conn)
cats = list_categories(conn)
conn.close()
assert stats["total"] > 0
assert len(cats) > 0

def test_second_ingest_skips_unchanged(
self, real_assets_root: Path, e2e_db: Path
) -> None:
"""Second ingest with no file changes should skip all previously indexed assets."""
result = ingest(root=real_assets_root, db_path=e2e_db)
assert result["skipped"] >= result["total_scanned"] - result["errors"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Tests have implicit ordering dependency due to session-scoped DB.

The e2e_db fixture is session-scoped (persists across all tests), creating implicit dependencies:

  • test_ingest_dry_run_reports_assets calls init_db(e2e_db)
  • test_ingest_writes_records assumes DB is already initialized
  • test_second_ingest_skips_unchanged depends on records from test_ingest_writes_records

If tests run in a different order or in isolation, they may fail. Consider either:

  1. Making each test self-contained by calling init_db at the start
  2. Documenting the required test order
  3. Using a class-level setup fixture
🛠️ Option: Use setup method to ensure DB is initialized
 `@pytest.mark.e2e`
 class TestRealIngest:
+    `@pytest.fixture`(autouse=True)
+    def _ensure_db_init(self, e2e_db: Path) -> None:
+        """Ensure DB is initialized before each test."""
+        from game_asset_mcp.catalog import init_db
+        init_db(e2e_db)  # init_db is idempotent
+
     def test_scan_finds_glbs(self, real_assets_root: Path) -> None:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/game-asset-mcp/tests/e2e/test_ingest_e2e.py` around lines 19 - 51,
Tests rely on a session-scoped e2e_db and therefore have implicit ordering; make
each test self-contained by ensuring the DB is initialized before use — call
init_db(e2e_db) at the start of test_ingest_writes_records and
test_second_ingest_skips_unchanged (in addition to the existing call in
test_ingest_dry_run_reports_assets) or replace that with a small setup fixture
that calls init_db(e2e_db) and is applied to each test; reference functions
init_db, ingest, get_connection, get_stats and list_categories to locate where
to add the initialization.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant