feat(asset-gen): AI Asset Generation — 3D gen/import + 2D image (BYO-key)#1218
Merged
Scriptwonder merged 27 commits intoJun 30, 2026
Merged
Conversation
… 2D image) Providers: Tripo/Meshy/Hunyuan (3D gen), Sketchfab (3D import), fal.ai/OpenRouter (2D image). Hybrid arch: GUI configures keys, MCP tools trigger, C# executes the provider call and imports. OS-secure-store key handling; keys never cross the bridge. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
…8 integration) Partial — phases 0,2-7 to be authored just-in-time during incremental execution (plan-authoring workflow rate-limited on those phases). Phase 1 is fully detailed and serves as the task-granularity template. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
…ase 0) - Register 'asset_gen' tool group (off by default, parity with vfx/animation) - EditorPrefKeys.AssetGen.* consts (non-secret config only; keys go to secure store) - AssetGenPrefs helper (provider/format/output-root/normalize/enabled) + EditMode tests - Python scaffold test asserts group present and disabled by default Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
…/import_model/generate_image) Thin key-free pass-throughs in group asset_gen mirroring the C# command names: camelCase param mapping, None-strip, async status/job_id (manage_packages shape). CLI group 'asset-gen' registered in cli/main.py. No key/secret param exists on any tool; tests assert sent payloads carry no key/secret/token. 34 tests pass. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
… (Phase 1) OS secure store per platform: macOS Keychain (/usr/bin/security), Windows Credential Manager (advapi32 P/Invoke), Linux secret-tool; AES-256-CBC+HMAC encrypt-then-MAC fallback (CI-safe, master secret + machine id via PBKDF2, ciphertext under user app-data, never in repo). Env override (MCPFORUNITY_<P>_API_KEY, read-only) layered on top; SecretRedactor scrubs auth tokens. Keys never touch EditorPrefs/bridge/logs/git. EditMode tests for fallback round-trip, encryption-at-rest, env override, redaction. Compiles clean on Unity 2021.3.45f2 (floor). Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
…ase 2) IHttpTransport seam (UnityWebRequestTransport prod, FakeHttpTransport for tests) so adapters are unit-testable without network. IModelProviderAdapter/IImageProviderAdapter/ IMarketplaceProviderAdapter + DTOs (ModelGenRequest/ImageGenRequest/ProviderPollResult/ ProviderInfo). AssetGenProviders registry (Configured=SecureKeyStore.Has, key-free). TripoAdapter submit (text/image_to_model, Bearer) + poll with defensive output-url parsing (pbr_model/model, flat or nested). 8 EditMode tests on FakeHttpTransport. Compiles clean on Unity 2021.3.45f2. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
…(Phase 3) AssetGenJobManager: in-memory submit->poll->download->import state machine driven by EditorApplication.update (asset import does not recompile, so no domain reload mid-job); SessionState snapshots keep status queryable across unrelated reloads. Key read once at submit, held in memory only, never persisted/logged/serialized onto the job. ModelImportPipeline: ModelImporter settings for FBX/OBJ, glTFast-gated GLB with an actionable error, best-effort scale normalize. GenerateModel.cs [McpForUnityTool RequiresPolling] with generate/status/cancel/list_providers using PendingResponse polling contract. 13 EditMode tests (state machine via FakeHttpTransport, handler routing, no-key-leak, import guards). Compiles clean on Unity 2021.3.45f2. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
New 'Asset Gen' tab in MCPForUnityEditorWindow: per-provider API-key entry backed by SecureKeyStore (never EditorPrefs/project — field never reads the stored key back), enable toggles, Test presence-check, Clear; non-secret prefs (default format, output root, auto-normalize); glTFast-missing notice pointing to the Dependencies tab. No Generate button / prompt — generation is MCP/CLI only. Wired exactly like the Advanced tab. Compiles clean on Unity 2021.3.45f2. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
Generalized AssetGenJobManager to submit/poll/import delegates so model & image share one state machine; added an inline-bytes path (ProviderPollResult.InlineData) for sync providers that return base64. FalAdapter (queue API: submit -> poll status -> fetch result image url) and OpenRouterAdapter (chat/completions, base64 image inline). ImageImportPipeline (TextureImporter: Sprite vs Default, alphaIsTransparency, sRGB vs linear). GenerateImage.cs [McpForUnityTool generate_image]. AssetGenProviders.Image wired. 7 EditMode tests (adapters via FakeHttpTransport, handler routing, OpenRouter inline end-to-end). Key read once at submit, never persisted. Compiles on 2021.3.45f2. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
MeshyAdapter (text/image->3D, model_urls by format). SketchfabAdapter (IMarketplaceProviderAdapter: search/preview/resolve-download) + ImportModel.cs [McpForUnityTool import_model] + AssetGenJobManager.StartMarketplaceImport. Hunyuan: TencentCloud3Signer (TC3-HMAC-SHA256, golden test vs python hmac oracle) + HunyuanAdapter (multi-secret JSON, Submit/Query Job). SafeZipExtractor (path-traversal guarded) + ModelImportPipeline .zip handling (extract -> find fbx/obj/glb -> import) for Hunyuan/ Sketchfab; ProviderPollResult.ResultExt override. Adapters wired in AssetGenProviders. Tests on FakeHttpTransport incl zip guard + TC3 known-answer. Compiles on 2021.3.45f2. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
…ist (Phase 8) Add glTFast (com.unity.cloud.gltfast) as an optional dependency in the Dependencies tab (detect + Install/Remove + bulk Install-All), so GLB generation/import is one click away. README 'AI Asset Generation' section (providers, BYO-key in the Asset Gen tab, OS secure store, manage_tools to enable, async tool usage). docs/asset-gen-manual-verification.md checklist for live validation (real keys + licensed editor; Hunyuan TC3 two-header caveat). Drop an incidental 'manage_tools' mention from a Phase 0 test comment so the tool-symmetry quarantine guard stays honest. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
Drop Hunyuan (Tencent TC3-HMAC) — high effort and not testable by the maintainer: delete HunyuanAdapter + TencentCloud3Signer (+tests), unwire from AssetGenProviders, SecureKeyStoreConstants, the GUI provider list, and Python/CLI/README/manual-verify docs. Refresh model defaults to current (2026) SOTA, verified against provider docs: - fal: fal-ai/flux/dev -> fal-ai/flux-2 (FLUX.2 dev; cheaper and higher quality) - Tripo: v2.5-20250123 -> v3.1-20260211 (current recommended model) - Meshy: drop deprecated art_style (errors on Meshy-6); pin ai_model=meshy-6 - OpenRouter: gemini-2.5-flash-image-preview (delisted) -> gemini-2.5-flash-image Fix two tests that asserted the now-implemented 'meshy' provider throws (repointed to the now-removed 'hunyuan'). They were silently wrong: the headless --full runs never executed EditMode tests (Unity -quit exits before -runTests; compile-only). A real -runTests run now passes 62/62 AssetGen EditMode tests; Python suite 1299 passed. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
… deadlock ImportModel.HandleCommand was synchronous and blocked on .GetAwaiter().GetResult() for the Sketchfab search/preview UnityWebRequest calls. UnityWebRequest completes on the editor loop, which a blocked main thread cannot pump — freezing the editor. Make HandleCommand (and Search/Preview) async Task<object> (CommandRegistry detects the Task return type) and add a HandleCommand_IsAsync regression guard. Surfaced via live in-editor testing. Claude-Session: https://claude.ai/code/session_01Tjpb5gYgUe2AUJuRdXr7Lv
Design + step-by-step plan for a DCC-agnostic import_model_file tool and a blender-to-unity handoff skill (FBX-default, filesystem-seam handoff). Claude-Session: https://claude.ai/code/session_01NoHk4f7N1vUFs7gu817ihm
Adds ImportModelFile tool (import_model_file command) that copies an
on-disk model file (FBX/OBJ/GLB/glTF/zip) under Assets/ and runs it
through the shared ModelImportPipeline. Returns {asset_path, asset_guid}.
Adds ImportModelFileHandlerTests covering missing-source, unsupported
extension, and a real OBJ import.
Claude-Session: https://claude.ai/code/session_01NoHk4f7N1vUFs7gu817ihm
…ll tool-name fix Claude-Session: https://claude.ai/code/session_01NoHk4f7N1vUFs7gu817ihm
…zation + scale note Blender FBX imports ~100x oversized; import_model_file target_size only acts when Auto-normalize is on and is unreliable for Blender FBX (target_size=2 measured 200m in a live test). Step 4 now measures the placed model's world bounds and sets localScale to hit the target size deterministically. Adds a Scale note + checklist item. Claude-Session: https://claude.ai/code/session_01NoHk4f7N1vUFs7gu817ihm
…ge_path
Security review + code review of the asset-gen feature surfaced concrete issues;
this fixes them and adds regression tests (request-shaping layer, FakeHttpTransport).
Security
- SafeZipExtractor enforces an extension allowlist; ModelImportPipeline passes an
inert model/texture allowlist so a provider archive can't drop a .cs/.dll under
Assets/ and have the Editor compile/load it (code execution on import).
- AssetGenJobManager refuses non-http(s) download URLs before fetching
(file:// SSRF / local-file read into the project).
Provider correctness
- Meshy image->3D polls /openapi/v1/image-to-3d/{id} (was the v2 text URL).
- Meshy text->3D honors texture=true via the preview->refine two-phase flow.
- OpenRouter image->image attaches the reference image (content image_url part).
- fal image->image uses the /edit endpoint + image_urls array; width/height
forwarded as image_size.
- Sketchfab search forwards categories/count/cursor/downloadable; preview doc
corrected (returns metadata, not a base64 thumbnail).
- Job import calls AssetDatabase.Refresh() before importing a freshly written file.
Local image input (image_path)
- New LocalImage helper; image_path is read and sent inline as a base64 data URI
for Meshy / fal / OpenRouter. Tripo rejects local images with a clear error
(needs a hosted image_url; its upload flow is not wired).
Cleanup (no behavior change)
- Shared AssetGenPaths + ProviderHttp helpers, HttpResult.Ok, MissingKeyMessage,
cached glTFast probe, dead-field / per-frame-alloc removal, CLI _emit.
Docs: README + manual-verification updated (image_path support; transparency is
import-flag-only; width/height fal-only).
Verified: package compiles clean; Python 1306 passed / 3 skipped. Meshy refine,
fal /edit, and image_path data-URI paths are unit-tested at the request layer
only -- live smoke per provider (real keys) still pending.
Claude-Session: https://claude.ai/code/session_015DAUrMR5UaSEzEn2wNPrEP
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds secure key storage, provider adapters, import pipelines, job orchestration, tool handlers, Python/CLI pass-throughs, editor UI, and documentation for Unity asset generation. ChangesAI Asset Generation Pipeline
Sequence DiagramsequenceDiagram
participant MCP_Tool as generate_model (Python)
participant Unity_Handler as GenerateModel.HandleCommand (C#)
participant JobManager as AssetGenJobManager
participant Adapter as TripoAdapter / MeshyAdapter
participant ProviderAPI as Provider REST API
participant ImportPipeline as ModelImportPipeline
MCP_Tool->>Unity_Handler: action=generate, provider, prompt
Unity_Handler->>JobManager: StartModelGeneration(req)
JobManager->>JobManager: TryResolveKey (SecureKeyStore)
JobManager-->>Unity_Handler: AssetGenJob (Queued)
Unity_Handler-->>MCP_Tool: pending { job_id }
loop EditorApplication.update ticks
JobManager->>Adapter: SubmitAsync(req, apiKey, http, ct)
Adapter->>ProviderAPI: POST /task
ProviderAPI-->>Adapter: task_id
Adapter-->>JobManager: providerJobId
JobManager->>Adapter: PollAsync(providerJobId, apiKey, http, ct)
Adapter->>ProviderAPI: GET /task/{id}
ProviderAPI-->>Adapter: status + download_url
Adapter-->>JobManager: ProviderPollResult(Succeeded, url)
JobManager->>ProviderAPI: Download bytes (http/https only)
ProviderAPI-->>JobManager: file bytes
JobManager->>ImportPipeline: ImportInto(job, localFilePath)
ImportPipeline-->>JobManager: job.State = Done, AssetPath set
end
MCP_Tool->>Unity_Handler: action=status, job_id
Unity_Handler->>JobManager: GetJob(job_id)
JobManager-->>Unity_Handler: AssetGenJob (Done, asset_path, asset_guid)
Unity_Handler-->>MCP_Tool: success { asset_path, asset_guid }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Informational row (not a keyed provider) with a best-effort "Blender app detected" status and a pointer to the blender-to-unity workflow / import_model_file. BlenderMCP runs in the AI client and isn't detectable from Unity, so this reports only the locally installed Blender app. - BlenderDetection helper: checks well-known install paths + PATH per platform; the pure DetectIn(candidates, exists) core is unit-tested with a fake predicate. - McpAssetGenSection: AddBlenderHandoffRow() appended after the provider rows. - README: note the tab's best-effort detection + that BlenderMCP lives in the AI client. Verified: package compiles clean; EditMode 972/1018 pass, 0 failures (4 new BlenderDetection tests) via tools/local_harness.py. Claude-Session: https://claude.ai/code/session_015DAUrMR5UaSEzEn2wNPrEP
Follow-up to 2efb786, from a focused AI review of the image_path work: - Validate image input synchronously in the handlers: reject unsupported extensions (LocalImage.ResolveExisting) and Tripo local-image up front, so bad input fails immediately instead of returning a fake "pending" that only faults on the next poll. - Meshy: the "no task id" submit/refine error again includes the response body (the PostTask refactor had dropped it). - Sketchfab archive allowlist: add .dds/.ktx (real GPU texture formats) so glTF textures in those formats aren't silently skipped on extract. - fal: don't send image_size to the /edit (image->image) endpoint, which derives size from the source image; build the request_id fallback poll URL from the base model id rather than the /edit sub-path. - Meshy two-phase preview+refine now runs under a 600s job timeout (was 300s) so a default textured text->3D has room for both tasks. Deferred (perf-only, noted on the PR): the blanket AssetDatabase.Refresh double-import and the synchronous main-thread base64 of large local images. Verified: package compiles clean; EditMode 977/1023 pass, 0 failures via tools/local_harness.py. Claude-Session: https://claude.ai/code/session_015DAUrMR5UaSEzEn2wNPrEP
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxml-6-6 (1)
6-6: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThe storage disclaimer is too absolute.
This text says keys are stored in the OS secure store, but the feature also has an encrypted-file fallback when those backends are unavailable. The current wording overpromises the storage location and could mislead users about where secrets end up. Based on learnings from the PR summary and stack context describing the AES-256-CBC+HMAC encrypted fallback keystore.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxml` at line 6, The help text in McpAssetGenSection.uxml is too absolute about secret storage and should be updated to reflect both storage paths. Adjust the assetgen-help Label copy so it says API keys are stored in the OS secure store when available, with an encrypted-file fallback used otherwise, and avoid claiming they are never stored outside the OS keychain/credential store. Keep the wording aligned with the actual keystore behavior in the asset generation UI.docs/superpowers/specs/2026-06-28-blender-to-unity-handoff-design.md-41-76 (1)
41-76: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis spec currently trips markdownlint.
The fenced block at Line 41 needs a language (for example
text), and the parameters table needs blank lines around it. If docs lint is enforced, this file will keep warning until both are fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-06-28-blender-to-unity-handoff-design.md` around lines 41 - 76, The docs snippet is triggering markdownlint because the fenced diagram block in this section has no language tag and the parameters table is missing required blank lines. Update the fenced block near the BlenderMCP/UnityMCP flow to use an explicit language like text, and adjust the `Parameters` table in the `import_model_file` section so it is surrounded by blank lines per markdownlint rules.Source: Linters/SAST tools
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs-759-762 (1)
759-762: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefreshing only on tab switch leaves Asset Gen stale after external key changes.
This updates the section when the user switches into the tab, but not when the window regains focus while Asset Gen is already active. Keys added or removed via CLI will keep showing the old presence state until the user changes tabs.
Suggested fix
private void RefreshAllData() { // Debounce rapid successive calls (e.g., from OnFocus being called multiple times) double currentTime = EditorApplication.timeSinceStartup; if (currentTime - lastRefreshTime < RefreshDebounceSeconds) @@ advancedSection?.UpdatePathOverrides(); clientConfigSection?.RefreshSelectedClient(); + if (assetGenTabToggle != null && assetGenTabToggle.value) + { + assetGenSection?.Refresh(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs` around lines 759 - 762, The Asset Gen panel refresh only happens in the ActivePanel.AssetGen switch path, so external key changes are not reflected when the window regains focus while that tab is already selected. Update the existing focus/refresh flow in MCPForUnityEditorWindow, especially the AssetGenPanel and assetGenSection.Refresh handling, so the Asset Gen section is refreshed both on tab activation and when the editor window becomes active again. Keep the refresh logic centralized so the current key presence state is always reloaded before display.MCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs-28-48 (1)
28-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when the imported asset is not a texture.
If
AssetImporter.GetAtPath(rel)is not aTextureImporter, this method still recordsAssetGuidand returnsDone. That lets an arbitrary non-image file underAssets/be reported as a successful generated image.Proposed fix
- if (AssetImporter.GetAtPath(rel) is TextureImporter importer) - { - importer.textureType = asSprite ? TextureImporterType.Sprite : TextureImporterType.Default; - importer.alphaIsTransparency = transparent; - importer.sRGBTexture = isColor; // color maps sRGB; normal/roughness/metallic would be linear - if (asSprite) - { - importer.spriteImportMode = SpriteImportMode.Single; - importer.mipmapEnabled = false; - } - importer.SaveAndReimport(); - } + if (!(AssetImporter.GetAtPath(rel) is TextureImporter importer)) + return Fail(job, "Imported file is not a texture asset."); + + importer.textureType = asSprite ? TextureImporterType.Sprite : TextureImporterType.Default; + importer.alphaIsTransparency = transparent; + importer.sRGBTexture = isColor; // color maps sRGB; normal/roughness/metallic would be linear + if (asSprite) + { + importer.spriteImportMode = SpriteImportMode.Single; + importer.mipmapEnabled = false; + } + importer.SaveAndReimport();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs` around lines 28 - 48, The image import flow in ImageImportPipeline.Import is incorrectly marking non-texture assets as successful because it proceeds to set job.AssetPath, job.AssetGuid, and Done even when AssetImporter.GetAtPath(rel) is not a TextureImporter. Add an explicit failure path in the Import method for the non-TextureImporter case, so only actual texture imports continue to reimport and complete successfully; otherwise call Fail(job, ...) and stop before assigning AssetGuid or Done.TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.cs-20-42 (1)
20-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve the original
MCPFORUNITY_TRIPO_API_KEYvalue.
SetUp()overwrites this process-global environment variable andTearDown()always clears it, so the suite can leak state into later tests that rely on the caller's original override. Snapshot the old value before Line 30 and restore that exact value inTearDown()instead of hard-resetting tonull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.cs` around lines 20 - 42, The test setup in AssetGenJobManagerTests is clobbering the process-global MCPFORUNITY_TRIPO_API_KEY and not restoring it, which can leak state into other tests. In SetUp(), snapshot the current environment value before setting Secret, and in TearDown() restore that saved value instead of always clearing it to null; keep this change localized around SetUp and TearDown in AssetGenJobManagerTests.MCPForUnity/Editor/Tools/AssetGen/GenerateModel.cs-139-145 (1)
139-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter
list_providersto model adapters.This endpoint currently returns every registered provider, while
generateonly accepts model providers. That makesgenerate_model list_providersadvertise IDs that the same handler rejects on the next call.Suggested fix
private static object ListProviders() { var list = new List<object>(); foreach (ProviderInfo info in AssetGenProviders.List()) + { + if (info.Kind != "model") continue; list.Add(new { id = info.Id, kind = info.Kind, configured = info.Configured, capabilities = info.Capabilities }); + } return new SuccessResponse($"{list.Count} provider(s).", new { providers = list }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/AssetGen/GenerateModel.cs` around lines 139 - 145, The ListProviders endpoint currently returns all AssetGenProviders.List() entries, but GenerateModel.Generate only works with model adapters, so the advertised IDs can be invalid for the next call. Update ListProviders to filter ProviderInfo entries to model adapters only before building the providers list, using the existing ListProviders/GenerateModel symbols so the output matches what generate accepts.MCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs-65-70 (1)
65-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject or normalize non-rooted
source_pathvalues.
ResolveSource()treats every non-Assets/...path as absolute, but it returns relative inputs unchanged. That makesimport_model_filedepend on Unity’s current working directory, so the same command can resolve different files across launches.Suggested fix
private static string ResolveSource(string source) { string s = source.Replace('\\', '/'); if (s == "Assets" || s.StartsWith("Assets/")) return AssetGenPaths.ToAbsolute(s); - return s; // absolute path on disk + if (!Path.IsPathRooted(source)) + throw new ArgumentException("'source_path' must be absolute or project-relative under Assets/."); + return Path.GetFullPath(source); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs` around lines 65 - 70, ResolveSource currently falls through any non-Assets path as if it were absolute, but it leaves relative inputs unchanged and makes import_model_file depend on Unity’s working directory. Update ResolveSource to either reject non-rooted source_path values or normalize them to a stable absolute path before returning, while keeping the existing Assets/ handling in ImportModelFile.Server/src/services/tools/generate_model.py-49-49 (1)
49-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the local
formatparameter. Line 49 shadows Python’s builtin and triggers Ruff A002; useoutput_formatand keep the payload key as"format".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/services/tools/generate_model.py` at line 49, The generate_model function currently uses a local parameter named format, which shadows Python’s builtin and triggers Ruff A002. Rename the parameter in generate_model to output_format (and update any references inside the function), while keeping the serialized payload field name as "format" so the API contract remains unchanged.Source: Linters/SAST tools
🧹 Nitpick comments (6)
docs/superpowers/specs/2026-06-28-ai-asset-generation-design.md (1)
1-294: 📐 Maintainability & Code Quality | 🔵 TrivialHunyuan3D still listed in design spec despite removal.
The provider table (lines 55-78) includes Hunyuan3D, but PR objectives state "later hardening includes removal of Hunyuan." This design document is historical/spec-level and the actual implementation was updated. No action needed, but ensure the final README/docs don't reference Hunyuan if unsupported.
Static analysis warnings are design-doc only.
The markdownlint and LanguageTool flags apply to formatting/style in a design specification document, not production code. These can be ignored or addressed at editorial discretion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-06-28-ai-asset-generation-design.md` around lines 1 - 294, The AI Asset Generation design spec still lists Hunyuan3D in the Providers section, which conflicts with the current supported-surface intent. Update the spec entries under the Providers table and any related architecture/tooling references so they only describe supported providers, and remove or clearly mark Hunyuan as deferred/unsupported where it appears. Use the “Providers (v1)” table and the “generate_model”/provider-adapter references as the places to clean up so the document matches the implementation and final docs.Source: Linters/SAST tools
TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.cs (1)
102-111: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd Windows-style and rooted traversal cases here.
This only exercises
../evil.txt. Add..\\evil.txtand an absolute/rooted entry as well so Zip-Slip coverage doesn't miss platform-specific path parsing regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.cs` around lines 102 - 111, The traversal test only covers a Unix-style relative escape, so extend TraversalEntry_Throws in SafeZipExtractorTests to also cover Windows-style traversal and a rooted/absolute path entry. Add cases using the existing MakeZip and SafeZipExtractor.ExtractTo flow for entries like ..\evil.txt and an absolute/rooted name, and assert they all throw IOException and do not write outside the extraction directory.MCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.cs (1)
16-16: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider
ConcurrentBag<T>or locking forRecordedRequestsif tests exercise concurrent requests.
List<HttpRequestSpec>is not thread-safe. While EditMode tests are typically single-threaded, async provider adapter tests that fire parallel requests could corrupt the list or lose entries. Based on coding guidelines, Unity API version compatibility issues must use shims rather than#ifdirectives; this is not applicable here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.cs` at line 16, The RecordedRequests collection in FakeHttpTransport is not thread-safe and can lose entries if tests issue parallel requests. Update FakeHttpTransport to use a concurrency-safe approach for RecordedRequests, such as a ConcurrentBag<HttpRequestSpec> or a locked wrapper around the current collection, and adjust the request-recording logic accordingly while keeping the existing API usage in sync.MCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop the auth-token regex at delimiters.
\S{6,}keeps matching until whitespace, so compact JSON/query strings lose everything after the token. Tightening the terminator preserves the rest of the error payload while still masking the credential.♻️ Proposed fix
- private static readonly Regex SchemeToken = new Regex( - @"\b(Bearer|Key|Token)\s+\S{6,}", + private static readonly Regex SchemeToken = new Regex( + @"\b(Bearer|Key|Token)\s+[^\s""',&]+", RegexOptions.IgnoreCase | RegexOptions.Compiled);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs` around lines 13 - 16, The SecretRedactor.SchemeToken regex is over-matching because it consumes all non-whitespace after Bearer/Key/Token, which can strip the rest of compact JSON or query payloads. Update the pattern in SecretRedactor so the token match stops at common delimiters instead of using a broad non-whitespace tail, while still masking the credential value. Keep the change scoped to the SchemeToken regex definition and ensure the redaction logic preserves surrounding error text.TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.cs (1)
77-85: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise secret scrubbing in the failure-path test.
This only proves that an exception is thrown. Add a case where the fake response echoes
sfk_secret(orToken sfk_secret) and assert the surfaced message does not contain it, so the adapter’s “never logged” guarantee stays covered end to end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.cs` around lines 77 - 85, The failure-path test in ResolveDownloadUrl_MissingGltf_Throws only checks that an exception occurs and does not verify secret scrubbing. Update SketchfabAdapterTests to use a fake HTTP response that includes sfk_secret (or Token sfk_secret) and assert the thrown message from ResolveDownloadUrlAsync does not contain that secret. Keep the check focused on the ResolveDownloadUrlAsync / ResolveDownloadUrl_MissingGltf_Throws path so the “never logged” guarantee is covered end to end.TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.cs (1)
49-55: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that delete paths remove the persisted secret file.
These tests only verify
Has/TryGet. IfEncryptedFileKeyStorestops exposing the key but leaveskey_<provider>.binon disk, both tests still pass while the secret remains persisted.Suggested test tightening
[Test] public void Delete_RemovesKey() { _store.Set("fal", "k-123456"); + string file = Path.Combine(_dir, "key_fal.bin"); + Assert.IsTrue(File.Exists(file)); _store.Delete("fal"); Assert.IsFalse(_store.Has("fal")); Assert.IsFalse(_store.TryGet("fal", out _)); + Assert.IsFalse(File.Exists(file)); } [Test] public void Set_Empty_DeletesKey() { _store.Set("tripo", "abc"); + string file = Path.Combine(_dir, "key_tripo.bin"); + Assert.IsTrue(File.Exists(file)); _store.Set("tripo", ""); Assert.IsFalse(_store.Has("tripo")); + Assert.IsFalse(File.Exists(file)); }Also applies to: 76-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.cs` around lines 49 - 55, The delete tests only verify in-memory lookup state and do not confirm the persisted secret file is removed. Tighten the assertions in Delete_RemovesKey and the other delete-related test by checking the provider’s key file for that path is gone after calling EncryptedFileKeyStore.Delete, in addition to Has/TryGet. Use the existing SecureKeyStoreTests and EncryptedFileKeyStore delete flow to locate the affected assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Helpers/AssetGenPaths.cs`:
- Around line 15-27: The path conversion helpers in AssetGenPaths should enforce
the documented Assets-only contract instead of accepting arbitrary rooted or
non-project paths. Update ToAbsolute and ToProjectRelative to reject inputs that
are not under the Assets tree, and ensure ToAbsolute also normalizes and
re-validates containment after Path.GetFullPath so values like Assets/../...
cannot escape the project. Use the existing helpers ProjectRoot and
TryGetAssetsRelativePath as the key locations to apply the containment check,
and make ToProjectRelative stop falling back to returning non-Assets paths
unchanged.
In `@MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs`:
- Around line 89-100: The NewPsi helper in LinuxSecretToolKeyStore currently
launches /usr/bin/env with secret-tool, which still performs PATH-based
resolution. Update the process launch so secret-tool is invoked via a trusted
absolute path, or make the backend fail closed when a safe path cannot be
determined, and keep the change localized to NewPsi and the code that depends on
it.
- Around line 15-28: The availability check in
LinuxSecretToolKeyStore.IsAvailable() only verifies that secret-tool exists, so
SelectPlatform() can incorrectly choose this backend even when the Secret
Service session is unreachable. Update IsAvailable() to perform a real probe
against the Secret Service session using the existing NewPsi() flow, and only
return true when org.freedesktop.secrets can actually be reached; otherwise
return false so the fallback path can select EncryptedFileKeyStore. Keep the
change localized to LinuxSecretToolKeyStore and the platform selection logic
that depends on it.
In `@MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs`:
- Around line 27-32: The MacKeychainKeyStore.Set method is passing the API key
through the security command argv via Run(..., "-w", apiKey), which exposes the
secret to process inspection. Update Set so it writes the key through a non-argv
input path or switch MacKeychainKeyStore to the native Keychain APIs, keeping
providerId/Service handling the same while removing the secret from command-line
arguments.
In `@MCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.cs`:
- Around line 392-393: The AssetGen import flow currently writes
provider-supplied ResultExt values into files under Assets and then calls
AssetDatabase.Refresh before validation, so add an allowlist check in
AssetGenJobManager (around ImportFn/ImportInto and filename creation) to reject
executable or project-affecting extensions before any file is written. Use a
helper like IsAllowedResultExtension keyed off the job kind/result type, and
fail the job early for anything outside the approved image/model/marketplace
extensions so .cs, .dll, .asmdef, and similar are never imported.
- Around line 507-510: IsAllowedDownloadUrl currently only allows http(s), but
it still permits localhost, link-local, and private-network IP targets that can
be used for SSRF. Update the IsAllowedDownloadUrl check in AssetGenJobManager to
resolve/inspect the Uri host and reject loopback, private, and link-local
addresses (including IPv4 literals like 127.0.0.1, 169.254.169.254, 10/8,
172.16/12, 192.168/16, and equivalent IPv6 local ranges) before allowing the
download URL. Keep the existing absolute URI and scheme validation, but add the
network-target restriction as part of the same helper.
In `@MCPForUnity/Editor/Services/AssetGen/Http/UnityWebRequestTransport.cs`:
- Around line 42-52: The cancellation handling in UnityWebRequestTransport still
allows request.SendWebRequest() to run even when the CancellationToken is
already canceled. Update the SendAsync flow to check ct.IsCancellationRequested
(or equivalent) before registering the callback and before calling
request.SendWebRequest(), and return a canceled task immediately when the token
is already canceled. Keep the existing cancellation registration and Abort logic
in place for in-flight requests so submit/search calls do not reach the remote
API after cancellation.
In `@MCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.cs`:
- Around line 88-99: The extraction flow in ModelImportPipeline currently reuses
the existing folderAbs directory before calling SafeZipExtractor.ExtractTo, so
stale files from a previous archive can remain and affect FindFirstModel. Update
the import path setup to either clear folderAbs before extraction or extract
into a uniquely named temporary folder per ZIP, then use that cleaned folder for
AssetDatabase.ImportAsset and FindFirstModel.
- Around line 178-194: The normalization logic in ModelImportPipeline’s
max-dimension calculation is using sharedMesh.bounds directly, which ignores
child transform position/scale and can produce incorrect sizes for nested
hierarchies. Update the bounds aggregation in the method that scans MeshFilter
and SkinnedMeshRenderer objects so it measures renderer/world-space bounds or
transforms each mesh bound into a common space before encapsulating, then keep
the existing maxDim calculation based on the corrected accumulated Bounds.
In `@MCPForUnity/Editor/Services/AssetGen/Import/SafeZipExtractor.cs`:
- Around line 65-68: The extraction path in SafeZipExtractor currently copies
each ZipArchiveEntry stream to disk without any size check, so add a maximum
allowed extracted size before or during the copy in the safe extraction flow.
Update the code around entry.Open() and src.CopyTo(dst) to validate each entry’s
uncompressed length and enforce a cumulative bound for the archive, rejecting
oversized or suspicious entries in SafeZipExtractor before writing to target.
In `@MCPForUnity/Editor/Services/AssetGen/Providers/OpenRouterAdapter.cs`:
- Around line 25-27: The OpenRouter adapter is keeping terminal job state in
instance fields, which breaks restored jobs and can leak stale results across
reuse. Update the result flow in OpenRouterAdapter so `SubmitAsync` writes the
final outcome into the job payload/snapshot (or derives it from `providerJobId`)
and `PollAsync` reconstructs the result from persisted job data instead of
relying on `_inlineData`, `_downloadUrl`, and `_error`. Make the
synchronous-provider path and any terminal state handling reconstructible after
reload using the job’s persisted state rather than adapter lifetime.
In `@MCPForUnity/Editor/Services/AssetGen/Providers/TripoAdapter.cs`:
- Around line 203-217: The TripoAdapter.PollAsync success check currently treats
a missing or malformed Tripo response code as success because code defaults to
0. Change the parsing logic so a 200 response is only accepted when the JSON
contains a valid code field equal to 0, and otherwise fall through to the
failure path in the existing httpOk/code check. Use the TripoAdapter.PollAsync
response handling around codeTok and the throw with SecretRedactor.Scrub to
locate the fix.
In `@MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.cs`:
- Around line 283-296: The clear-button handler in McpAssetGenSection is always
updating the UI even when SecureKeyStore.Current.Delete(id) fails. Move the
keyField.SetValueWithoutNotify and SetStatus(statusLabel, "not set", false)
calls so they only run after a successful delete, and in the catch block set a
failure state instead of pretending the key was cleared. Use the existing
clearButton.clicked handler, SecureKeyStore.Current.Delete, keyField, and
SetStatus symbols to locate and update the flow.
In `@Server/src/services/tools/generate_image.py`:
- Around line 19-20: The `generate_image` tool is registered with an unsupported
MCP group value in its `@mcp_for_unity_tool` decorator. Update `generate_image`
to use one of the approved groups for `Server/src/services/tools/*.py` (such as
`core`, `vfx`, `animation`, `ui`, `scripting_ext`, `testing`, `probuilder`,
`profiling`, or `docs`), or make the matching tool-group contract change in the
same update so the registration stays compliant.
In `@Server/src/services/tools/import_model.py`:
- Around line 27-28: The `search` MCP surface in `import_model.py` is using
`count`, which breaks the repo’s standard paging contract. Update the `search`
request and response shape to use `page_size` with `cursor` input and
`next_cursor` in the result, and make sure the `search` tool’s schema/docs and
implementation stay aligned with that paging flow. Use the `search` tool
definition and any related response-building logic in `import_model.py` as the
main places to adjust.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenPrefsTests.cs`:
- Around line 15-34: The AssetGenPrefsTests cleanup currently deletes real
EditorPrefs state, which can wipe a user’s persisted Asset Gen settings during
test runs. Update the test fixture around Clear(), SetUp(), and TearDown() to
preserve and restore the original values for the EditorPrefKeys entries instead
of blindly calling EditorPrefs.DeleteKey, or isolate AssetGenPrefs behind a
test-only backend/prefix so the tests only touch disposable data.
---
Minor comments:
In `@docs/superpowers/specs/2026-06-28-blender-to-unity-handoff-design.md`:
- Around line 41-76: The docs snippet is triggering markdownlint because the
fenced diagram block in this section has no language tag and the parameters
table is missing required blank lines. Update the fenced block near the
BlenderMCP/UnityMCP flow to use an explicit language like text, and adjust the
`Parameters` table in the `import_model_file` section so it is surrounded by
blank lines per markdownlint rules.
In `@MCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs`:
- Around line 28-48: The image import flow in ImageImportPipeline.Import is
incorrectly marking non-texture assets as successful because it proceeds to set
job.AssetPath, job.AssetGuid, and Done even when AssetImporter.GetAtPath(rel) is
not a TextureImporter. Add an explicit failure path in the Import method for the
non-TextureImporter case, so only actual texture imports continue to reimport
and complete successfully; otherwise call Fail(job, ...) and stop before
assigning AssetGuid or Done.
In `@MCPForUnity/Editor/Tools/AssetGen/GenerateModel.cs`:
- Around line 139-145: The ListProviders endpoint currently returns all
AssetGenProviders.List() entries, but GenerateModel.Generate only works with
model adapters, so the advertised IDs can be invalid for the next call. Update
ListProviders to filter ProviderInfo entries to model adapters only before
building the providers list, using the existing ListProviders/GenerateModel
symbols so the output matches what generate accepts.
In `@MCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs`:
- Around line 65-70: ResolveSource currently falls through any non-Assets path
as if it were absolute, but it leaves relative inputs unchanged and makes
import_model_file depend on Unity’s working directory. Update ResolveSource to
either reject non-rooted source_path values or normalize them to a stable
absolute path before returning, while keeping the existing Assets/ handling in
ImportModelFile.
In `@MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxml`:
- Line 6: The help text in McpAssetGenSection.uxml is too absolute about secret
storage and should be updated to reflect both storage paths. Adjust the
assetgen-help Label copy so it says API keys are stored in the OS secure store
when available, with an encrypted-file fallback used otherwise, and avoid
claiming they are never stored outside the OS keychain/credential store. Keep
the wording aligned with the actual keystore behavior in the asset generation
UI.
In `@MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs`:
- Around line 759-762: The Asset Gen panel refresh only happens in the
ActivePanel.AssetGen switch path, so external key changes are not reflected when
the window regains focus while that tab is already selected. Update the existing
focus/refresh flow in MCPForUnityEditorWindow, especially the AssetGenPanel and
assetGenSection.Refresh handling, so the Asset Gen section is refreshed both on
tab activation and when the editor window becomes active again. Keep the refresh
logic centralized so the current key presence state is always reloaded before
display.
In `@Server/src/services/tools/generate_model.py`:
- Line 49: The generate_model function currently uses a local parameter named
format, which shadows Python’s builtin and triggers Ruff A002. Rename the
parameter in generate_model to output_format (and update any references inside
the function), while keeping the serialized payload field name as "format" so
the API contract remains unchanged.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.cs`:
- Around line 20-42: The test setup in AssetGenJobManagerTests is clobbering the
process-global MCPFORUNITY_TRIPO_API_KEY and not restoring it, which can leak
state into other tests. In SetUp(), snapshot the current environment value
before setting Secret, and in TearDown() restore that saved value instead of
always clearing it to null; keep this change localized around SetUp and TearDown
in AssetGenJobManagerTests.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-06-28-ai-asset-generation-design.md`:
- Around line 1-294: The AI Asset Generation design spec still lists Hunyuan3D
in the Providers section, which conflicts with the current supported-surface
intent. Update the spec entries under the Providers table and any related
architecture/tooling references so they only describe supported providers, and
remove or clearly mark Hunyuan as deferred/unsupported where it appears. Use the
���Providers (v1)” table and the “generate_model”/provider-adapter references as
the places to clean up so the document matches the implementation and final
docs.
In `@MCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs`:
- Around line 13-16: The SecretRedactor.SchemeToken regex is over-matching
because it consumes all non-whitespace after Bearer/Key/Token, which can strip
the rest of compact JSON or query payloads. Update the pattern in SecretRedactor
so the token match stops at common delimiters instead of using a broad
non-whitespace tail, while still masking the credential value. Keep the change
scoped to the SchemeToken regex definition and ensure the redaction logic
preserves surrounding error text.
In `@MCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.cs`:
- Line 16: The RecordedRequests collection in FakeHttpTransport is not
thread-safe and can lose entries if tests issue parallel requests. Update
FakeHttpTransport to use a concurrency-safe approach for RecordedRequests, such
as a ConcurrentBag<HttpRequestSpec> or a locked wrapper around the current
collection, and adjust the request-recording logic accordingly while keeping the
existing API usage in sync.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.cs`:
- Around line 102-111: The traversal test only covers a Unix-style relative
escape, so extend TraversalEntry_Throws in SafeZipExtractorTests to also cover
Windows-style traversal and a rooted/absolute path entry. Add cases using the
existing MakeZip and SafeZipExtractor.ExtractTo flow for entries like
..\evil.txt and an absolute/rooted name, and assert they all throw IOException
and do not write outside the extraction directory.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.cs`:
- Around line 49-55: The delete tests only verify in-memory lookup state and do
not confirm the persisted secret file is removed. Tighten the assertions in
Delete_RemovesKey and the other delete-related test by checking the provider’s
key file for that path is gone after calling EncryptedFileKeyStore.Delete, in
addition to Has/TryGet. Use the existing SecureKeyStoreTests and
EncryptedFileKeyStore delete flow to locate the affected assertions.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.cs`:
- Around line 77-85: The failure-path test in
ResolveDownloadUrl_MissingGltf_Throws only checks that an exception occurs and
does not verify secret scrubbing. Update SketchfabAdapterTests to use a fake
HTTP response that includes sfk_secret (or Token sfk_secret) and assert the
thrown message from ResolveDownloadUrlAsync does not contain that secret. Keep
the check focused on the ResolveDownloadUrlAsync /
ResolveDownloadUrl_MissingGltf_Throws path so the “never logged” guarantee is
covered end to end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeefd96c-7a97-4c6e-b0cd-4713cd17f681
📒 Files selected for processing (142)
.claude/skills/blender-to-unity/SKILL.md.claude/skills/blender-to-unity/manual-verify.mdMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Helpers/AssetGenPaths.csMCPForUnity/Editor/Helpers/AssetGenPaths.cs.metaMCPForUnity/Editor/Helpers/AssetGenPrefs.csMCPForUnity/Editor/Helpers/AssetGenPrefs.cs.metaMCPForUnity/Editor/Helpers/BlenderDetection.csMCPForUnity/Editor/Helpers/BlenderDetection.cs.metaMCPForUnity/Editor/Security.metaMCPForUnity/Editor/Security/SecureKeyStore.metaMCPForUnity/Editor/Security/SecureKeyStore/EncryptedFileKeyStore.csMCPForUnity/Editor/Security/SecureKeyStore/EncryptedFileKeyStore.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/EnvKeyOverride.csMCPForUnity/Editor/Security/SecureKeyStore/EnvKeyOverride.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/ISecureKeyStore.csMCPForUnity/Editor/Security/SecureKeyStore/ISecureKeyStore.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.csMCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.csMCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.csMCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStore.csMCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStore.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStoreConstants.csMCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStoreConstants.cs.metaMCPForUnity/Editor/Security/SecureKeyStore/WindowsCredentialKeyStore.csMCPForUnity/Editor/Security/SecureKeyStore/WindowsCredentialKeyStore.cs.metaMCPForUnity/Editor/Services/AssetGen.metaMCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.csMCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.cs.metaMCPForUnity/Editor/Services/AssetGen/Http.metaMCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.csMCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.cs.metaMCPForUnity/Editor/Services/AssetGen/Http/HttpRequestSpec.csMCPForUnity/Editor/Services/AssetGen/Http/HttpRequestSpec.cs.metaMCPForUnity/Editor/Services/AssetGen/Http/HttpResult.csMCPForUnity/Editor/Services/AssetGen/Http/HttpResult.cs.metaMCPForUnity/Editor/Services/AssetGen/Http/IHttpTransport.csMCPForUnity/Editor/Services/AssetGen/Http/IHttpTransport.cs.metaMCPForUnity/Editor/Services/AssetGen/Http/UnityWebRequestTransport.csMCPForUnity/Editor/Services/AssetGen/Http/UnityWebRequestTransport.cs.metaMCPForUnity/Editor/Services/AssetGen/Import.metaMCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.csMCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs.metaMCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.csMCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.cs.metaMCPForUnity/Editor/Services/AssetGen/Import/SafeZipExtractor.csMCPForUnity/Editor/Services/AssetGen/Import/SafeZipExtractor.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers.metaMCPForUnity/Editor/Services/AssetGen/Providers/AssetGenProviders.csMCPForUnity/Editor/Services/AssetGen/Providers/AssetGenProviders.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/FalAdapter.csMCPForUnity/Editor/Services/AssetGen/Providers/FalAdapter.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/IProviderAdapters.csMCPForUnity/Editor/Services/AssetGen/Providers/IProviderAdapters.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/LocalImage.csMCPForUnity/Editor/Services/AssetGen/Providers/LocalImage.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/MeshyAdapter.csMCPForUnity/Editor/Services/AssetGen/Providers/MeshyAdapter.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/OpenRouterAdapter.csMCPForUnity/Editor/Services/AssetGen/Providers/OpenRouterAdapter.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/ProviderHttp.csMCPForUnity/Editor/Services/AssetGen/Providers/ProviderHttp.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/ProviderModels.csMCPForUnity/Editor/Services/AssetGen/Providers/ProviderModels.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/SketchfabAdapter.csMCPForUnity/Editor/Services/AssetGen/Providers/SketchfabAdapter.cs.metaMCPForUnity/Editor/Services/AssetGen/Providers/TripoAdapter.csMCPForUnity/Editor/Services/AssetGen/Providers/TripoAdapter.cs.metaMCPForUnity/Editor/Tools/AssetGen.metaMCPForUnity/Editor/Tools/AssetGen/GenerateImage.csMCPForUnity/Editor/Tools/AssetGen/GenerateImage.cs.metaMCPForUnity/Editor/Tools/AssetGen/GenerateModel.csMCPForUnity/Editor/Tools/AssetGen/GenerateModel.cs.metaMCPForUnity/Editor/Tools/AssetGen/ImportModel.csMCPForUnity/Editor/Tools/AssetGen/ImportModel.cs.metaMCPForUnity/Editor/Tools/AssetGen/ImportModelFile.csMCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs.metaMCPForUnity/Editor/Windows/Components/AssetGen.metaMCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.csMCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.cs.metaMCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxmlMCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxml.metaMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxmlREADME.mdServer/src/cli/commands/asset_gen.pyServer/src/cli/main.pyServer/src/services/registry/tool_registry.pyServer/src/services/tools/generate_image.pyServer/src/services/tools/generate_model.pyServer/src/services/tools/import_model.pyServer/src/services/tools/import_model_file.pyServer/tests/test_asset_gen_image.pyServer/tests/test_asset_gen_import.pyServer/tests/test_asset_gen_import_file.pyServer/tests/test_asset_gen_model.pyServer/tests/test_asset_gen_scaffold.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenPrefsTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenPrefsTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenProvidersTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenProvidersTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/BlenderDetectionTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/BlenderDetectionTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/EnvKeyOverrideTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/EnvKeyOverrideTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/FalAdapterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/FalAdapterTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateImageTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateImageTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateModelHandlerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateModelHandlerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelFileHandlerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelFileHandlerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelHandlerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelHandlerTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/MeshyAdapterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/MeshyAdapterTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ModelImportPipelineTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ModelImportPipelineTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/OpenRouterAdapterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/OpenRouterAdapterTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecretRedactorTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecretRedactorTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/TripoAdapterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/TripoAdapterTests.cs.metadocs/asset-gen-manual-verification.mddocs/superpowers/plans/2026-06-28-ai-asset-generation.mddocs/superpowers/plans/2026-06-28-blender-to-unity-handoff.mddocs/superpowers/specs/2026-06-28-ai-asset-generation-design.mddocs/superpowers/specs/2026-06-28-blender-to-unity-handoff-design.md
Comment on lines
+15
to
+27
| public static string ToAbsolute(string projectRelative) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(projectRelative)) return projectRelative; | ||
| string p = projectRelative.Replace('\\', '/'); | ||
| string abs = Path.IsPathRooted(p) ? p : Path.Combine(ProjectRoot(), p); | ||
| return Path.GetFullPath(abs).Replace('\\', '/'); | ||
| } | ||
|
|
||
| /// <summary>Convert an absolute (or already-relative) path to a project-relative ("Assets/...") path.</summary> | ||
| public static string ToProjectRelative(string path) | ||
| { | ||
| if (TryGetAssetsRelativePath(path, out string rel)) return rel; | ||
| return path?.Replace('\\', '/'); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-Assets/... inputs in the conversion helpers.
ToAbsolute currently resolves any rooted path (and even Assets/../...) without re-checking containment, while ToProjectRelative falls back to returning non-project paths unchanged. That breaks the helper’s documented Assets-only contract and weakens the traversal protection this layer is supposed to provide for generated imports/outputs.
Proposed fix
public static string ToAbsolute(string projectRelative)
{
- if (string.IsNullOrWhiteSpace(projectRelative)) return projectRelative;
- string p = projectRelative.Replace('\\', '/');
- string abs = Path.IsPathRooted(p) ? p : Path.Combine(ProjectRoot(), p);
- return Path.GetFullPath(abs).Replace('\\', '/');
+ if (!TryGetAssetsRelativePath(projectRelative, out string rel))
+ throw new ArgumentException("Path must resolve inside the project's Assets folder.", nameof(projectRelative));
+
+ return Path.GetFullPath(Path.Combine(ProjectRoot(), rel)).Replace('\\', '/');
}
/// <summary>Convert an absolute (or already-relative) path to a project-relative ("Assets/...") path.</summary>
public static string ToProjectRelative(string path)
{
- if (TryGetAssetsRelativePath(path, out string rel)) return rel;
- return path?.Replace('\\', '/');
+ if (!TryGetAssetsRelativePath(path, out string rel))
+ throw new ArgumentException("Path must resolve inside the project's Assets folder.", nameof(path));
+
+ return rel;
}📝 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
| public static string ToAbsolute(string projectRelative) | |
| { | |
| if (string.IsNullOrWhiteSpace(projectRelative)) return projectRelative; | |
| string p = projectRelative.Replace('\\', '/'); | |
| string abs = Path.IsPathRooted(p) ? p : Path.Combine(ProjectRoot(), p); | |
| return Path.GetFullPath(abs).Replace('\\', '/'); | |
| } | |
| /// <summary>Convert an absolute (or already-relative) path to a project-relative ("Assets/...") path.</summary> | |
| public static string ToProjectRelative(string path) | |
| { | |
| if (TryGetAssetsRelativePath(path, out string rel)) return rel; | |
| return path?.Replace('\\', '/'); | |
| public static string ToAbsolute(string projectRelative) | |
| { | |
| if (!TryGetAssetsRelativePath(projectRelative, out string rel)) | |
| throw new ArgumentException("Path must resolve inside the project's Assets folder.", nameof(projectRelative)); | |
| return Path.GetFullPath(Path.Combine(ProjectRoot(), rel)).Replace('\\', '/'); | |
| } | |
| /// <summary>Convert an absolute (or already-relative) path to a project-relative ("Assets/...") path.</summary> | |
| public static string ToProjectRelative(string path) | |
| { | |
| if (!TryGetAssetsRelativePath(path, out string rel)) | |
| throw new ArgumentException("Path must resolve inside the project's Assets folder.", nameof(path)); | |
| return rel; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Helpers/AssetGenPaths.cs` around lines 15 - 27, The path
conversion helpers in AssetGenPaths should enforce the documented Assets-only
contract instead of accepting arbitrary rooted or non-project paths. Update
ToAbsolute and ToProjectRelative to reject inputs that are not under the Assets
tree, and ensure ToAbsolute also normalizes and re-validates containment after
Path.GetFullPath so values like Assets/../... cannot escape the project. Use the
existing helpers ProjectRoot and TryGetAssetsRelativePath as the key locations
to apply the containment check, and make ToProjectRelative stop falling back to
returning non-Assets paths unchanged.
Comment on lines
+15
to
+28
| internal static bool IsAvailable() | ||
| { | ||
| try | ||
| { | ||
| var psi = NewPsi(); | ||
| psi.ArgumentList.Add("--version"); | ||
| using (var p = Process.Start(psi)) | ||
| { | ||
| p.WaitForExit(2000); | ||
| return p.ExitCode == 0; | ||
| } | ||
| } | ||
| catch { return false; } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## file list"
git ls-files 'MCPForUnity/Editor/Security/SecureKeyStore/*' 'MCPForUnity/Editor/Security/*' | sed -n '1,200p'
echo
echo "## outline LinuxSecretToolKeyStore.cs"
ast-grep outline MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs --view expanded || true
echo
echo "## outline secure key store selector files"
for f in $(git ls-files 'MCPForUnity/Editor/Security/SecureKeyStore/*'); do
echo "### $f"
ast-grep outline "$f" --view expanded | sed -n '1,200p' || true
doneRepository: CoplayDev/unity-mcp
Length of output: 3754
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs \
MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStore.cs \
MCPForUnity/Editor/Security/SecureKeyStore/EncryptedFileKeyStore.cs \
MCPForUnity/Editor/Security/SecureKeyStore/EnvKeyOverride.cs \
MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs \
MCPForUnity/Editor/Security/SecureKeyStore/WindowsCredentialKeyStore.cs
do
echo "===== $f ====="
wc -l "$f"
cat -n "$f" | sed -n '1,240p'
echo
doneRepository: CoplayDev/unity-mcp
Length of output: 27595
🌐 Web query:
secret-tool --version does not require org.freedesktop.secrets service libsecret documentation
💡 Result:
The statement that secret-tool --version does not require the org.freedesktop.secrets service is correct. The secret-tool command is a utility provided by the libsecret library, which acts as a client for the Secret Service API [1][2]. The Secret Service API is a specification for managing secrets over D-Bus, where org.freedesktop.secrets is the well-known bus name for the service providing this functionality (typically implemented by applications like gnome-keyring or ksecretservice) [3][4]. When you run secret-tool --version, the command simply prints version information about the utility itself and exits. Because it does not perform any secret storage or retrieval operations, it does not need to communicate with the D-Bus session bus or interact with the Secret Service daemon. Consequently, it does not trigger the requirement for, nor attempt to connect to, the org.freedesktop.secrets service [5][3][6]. In contrast, other commands like secret-tool lookup or secret-tool store perform operations that mandate an active connection to the Secret Service [7][8][3]. If the service is not running or is unavailable, those commands will fail with a D-Bus error, such as: "The name org.freedesktop.secrets was not provided by any.service files" [5][3][6].
Citations:
- 1: https://github.com/GNOME/libsecret
- 2: secret-tool in libsecret not functional Homebrew/legacy-homebrew#44994
- 3: https://rtfm.co.ua/en/what-is-linux-keyring-gnome-keyring-secret-service-and-d-bus/
- 4: https://specifications.freedesktop.org/secret-service-spec/latest-single/
- 5: https://alioth-lists.debian.net/pipermail/pkg-gnome-maintainers/2015-April/118526.html
- 6: https://discourse.nixos.org/t/the-name-org-freedesktop-secrets-was-not-provided-by-any-service-files/36406
- 7: https://man.archlinux.org/man/core/libsecret/secret-tool.1.en
- 8: https://manpages.ubuntu.com/manpages/focal/man1/secret-tool.1.html
Probe a real Secret Service session before selecting this backend.
secret-tool --version only proves the binary exists. If libsecret is installed but org.freedesktop.secrets isn’t reachable, SelectPlatform() still chooses LinuxSecretToolKeyStore, and key reads/writes just fail instead of falling back to EncryptedFileKeyStore.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs` around
lines 15 - 28, The availability check in LinuxSecretToolKeyStore.IsAvailable()
only verifies that secret-tool exists, so SelectPlatform() can incorrectly
choose this backend even when the Secret Service session is unreachable. Update
IsAvailable() to perform a real probe against the Secret Service session using
the existing NewPsi() flow, and only return true when org.freedesktop.secrets
can actually be reached; otherwise return false so the fallback path can select
EncryptedFileKeyStore. Keep the change localized to LinuxSecretToolKeyStore and
the platform selection logic that depends on it.
Comment on lines
+89
to
+100
| private static ProcessStartInfo NewPsi(bool redirectIn = false) | ||
| { | ||
| var psi = new ProcessStartInfo("/usr/bin/env") | ||
| { | ||
| UseShellExecute = false, | ||
| CreateNoWindow = true, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| RedirectStandardInput = redirectIn, | ||
| }; | ||
| psi.ArgumentList.Add("secret-tool"); | ||
| return psi; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== file outline =='
ast-grep outline MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs --view expanded || true
printf '\n%s\n' '== relevant lines =='
sed -n '1,220p' MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs
printf '\n%s\n' '== search for secret-tool usage and backend selection =='
rg -n "secret-tool|LinuxSecretToolKeyStore|NewPsi\(" MCPForUnity -SRepository: CoplayDev/unity-mcp
Length of output: 5509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.csRepository: CoplayDev/unity-mcp
Length of output: 3889
Resolve secret-tool without PATH lookup. Using /usr/bin/env secret-tool still relies on PATH, so a malicious or unexpected secret-tool can receive the key on stdin. Execute a known absolute binary, or disable this backend when it can’t be resolved safely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs` around
lines 89 - 100, The NewPsi helper in LinuxSecretToolKeyStore currently launches
/usr/bin/env with secret-tool, which still performs PATH-based resolution.
Update the process launch so secret-tool is invoked via a trusted absolute path,
or make the backend fail closed when a safe path cannot be determined, and keep
the change localized to NewPsi and the code that depends on it.
Comment on lines
+27
to
+32
| public void Set(string providerId, string apiKey) | ||
| { | ||
| if (string.IsNullOrEmpty(providerId)) return; | ||
| if (string.IsNullOrEmpty(apiKey)) { Delete(providerId); return; } | ||
| // -U overwrites an existing item for this service/account. | ||
| Run("add-generic-password", "-U", "-s", Service, "-a", providerId, "-w", apiKey); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
git ls-files 'MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs'
echo '---'
cat -n MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs | sed -n '1,220p'
echo '--- security usage search ---'
rg -n --context 2 'add-generic-password|security|Run\(' MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.csRepository: CoplayDev/unity-mcp
Length of output: 4542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the security CLI documents -w as a password argument.
python3 - <<'PY'
import urllib.request, re, ssl
urls = [
"https://www.manpagez.com/man/1/security/",
"https://keith.github.io/xcode-man-pages/security.1.html",
]
for url in urls:
try:
with urllib.request.urlopen(url, timeout=10, context=ssl.create_default_context()) as r:
text = r.read().decode('utf-8', 'replace')
print(f"URL: {url}")
for pat in [r'add-generic-password', r'\-w', r'pass phrase', r'password']:
m = re.search(pat, text, re.I)
print(f" {pat}: {'yes' if m else 'no'}")
# Print a small relevant excerpt if possible
idx = text.lower().find('add-generic-password')
if idx != -1:
print(text[idx:idx+900])
print('---')
except Exception as e:
print(f"URL: {url} ERROR: {e}")
PYRepository: CoplayDev/unity-mcp
Length of output: 546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the implementation of the Run helper to see whether it shells out via ProcessStartInfo with argv.
ast-grep outline MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs --view expanded
echo '---'
rg -n --context 3 'class .*SecureKeyStore|static .*Run\(|void Run\(' MCPForUnity/Editor/Security/SecureKeyStoreRepository: CoplayDev/unity-mcp
Length of output: 6736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read any nearby helper code if the file is short enough or references another helper.
fd -a 'MacKeychainKeyStore.cs|SecureKeyStore' MCPForUnityRepository: CoplayDev/unity-mcp
Length of output: 977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the repository contains references to `security add-generic-password`, inspect them.
rg -n --context 3 'add-generic-password|security' MCPForUnityRepository: CoplayDev/unity-mcp
Length of output: 11788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Minimal check of the target file around the method.
sed -n '1,120p' MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.csRepository: CoplayDev/unity-mcp
Length of output: 2656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the security CLI documents -w as a password argument.
python3 - <<'PY'
import urllib.request, re, ssl
urls = [
"https://www.manpagez.com/man/1/security/",
"https://keith.github.io/xcode-man-pages/security.1.html",
]
for url in urls:
try:
with urllib.request.urlopen(url, timeout=10, context=ssl.create_default_context()) as r:
text = r.read().decode('utf-8', 'replace')
print(f"URL: {url}")
for pat in [r'add-generic-password', r'\-w', r'password']:
m = re.search(pat, text, re.I)
print(f" {pat}: {'yes' if m else 'no'}")
idx = text.lower().find('add-generic-password')
if idx != -1:
print(text[idx:idx+900])
print('---')
except Exception as e:
print(f"URL: {url} ERROR: {e}")
PYRepository: CoplayDev/unity-mcp
Length of output: 546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for native macOS Keychain / Security.framework interop or other secret-handling patterns.
rg -n --context 2 'SecKeychain|SecItem|Security\.framework|DllImport|P/Invoke|add-generic-password|find-generic-password|secret-tool|stdin' MCPForUnity/Editor MCPForUnity/RuntimeRepository: CoplayDev/unity-mcp
Length of output: 4596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Linux key store to compare its secret-handling pattern.
cat -n MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs | sed -n '1,220p'Repository: CoplayDev/unity-mcp
Length of output: 4610
Don't pass the API key via security argv. add-generic-password ... -w <key> puts the secret in the process arguments, which is exposed to local process inspection while the command runs. Use a non-argv input path or the native Keychain APIs instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs` around
lines 27 - 32, The MacKeychainKeyStore.Set method is passing the API key through
the security command argv via Run(..., "-w", apiKey), which exposes the secret
to process inspection. Update Set so it writes the key through a non-argv input
path or switch MacKeychainKeyStore to the native Keychain APIs, keeping
providerId/Service handling the same while removing the secret from command-line
arguments.
Comment on lines
+392
to
+393
| if (ImportOverrideForTests == null) AssetDatabase.Refresh(); | ||
| AssetGenJob imported = r.ImportFn(r.Job, r.LocalPath); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Reject provider-controlled executable extensions before writing into Assets/.
ResultExt flows straight into the filename, then AssetDatabase.Refresh() imports whatever was written. A compromised provider can hand back .cs, .dll, .asmdef, etc. and get code/plugin content into the project before ModelImportPipeline.ImportInto has any chance to fail the job.
Possible fix
private static string WriteFile(Runner r, byte[] bytes)
{
string chosen = !string.IsNullOrEmpty(r.OverrideExt) ? r.OverrideExt : r.Ext;
string ext = string.IsNullOrEmpty(chosen) ? "bin" : chosen.TrimStart('.').ToLowerInvariant();
+ if (!IsAllowedResultExtension(r.Job.Kind, ext))
+ throw new InvalidOperationException($"Unsupported generated asset extension '.{ext}'.");
string requestedRoot = !string.IsNullOrEmpty(r.OutputFolder) ? r.OutputFolder
: (AssetGenPrefs.OutputRoot + "/" + r.Subfolder);private static bool IsAllowedResultExtension(string kind, string ext)
{
switch (kind)
{
case "image":
return ext == "png" || ext == "jpg" || ext == "jpeg";
case "model":
return ext == "fbx" || ext == "obj" || ext == "glb" || ext == "gltf";
case "marketplace":
return ext == "zip" || ext == "glb" || ext == "gltf";
default:
return false;
}
}Also applies to: 411-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.cs` around lines 392
- 393, The AssetGen import flow currently writes provider-supplied ResultExt
values into files under Assets and then calls AssetDatabase.Refresh before
validation, so add an allowlist check in AssetGenJobManager (around
ImportFn/ImportInto and filename creation) to reject executable or
project-affecting extensions before any file is written. Use a helper like
IsAllowedResultExtension keyed off the job kind/result type, and fail the job
early for anything outside the approved image/model/marketplace extensions so
.cs, .dll, .asmdef, and similar are never imported.
Comment on lines
+203
to
+217
| int code = 0; | ||
| JToken codeTok = json?["code"]; | ||
| if (codeTok != null && codeTok.Type != JTokenType.Null) | ||
| { | ||
| try { code = codeTok.Value<int>(); } catch { code = -1; } | ||
| } | ||
|
|
||
| if (!httpOk || code != 0) | ||
| { | ||
| string detail = json?["message"]?.ToString() | ||
| ?? json?["error"]?.ToString() | ||
| ?? ProviderHttp.Truncate(text); | ||
| throw new Exception(SecretRedactor.Scrub( | ||
| $"Tripo {phase} failed (status={res?.Status}, code={code}): {detail}", apiKey)); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't treat a 200 response without Tripo's code field as success.
code defaults to 0 here, so a malformed/non-JSON 200 response is accepted. In PollAsync that degrades into Queued, which can leave the job polling forever instead of surfacing the provider failure.
🛠️ Proposed fix
- int code = 0;
+ int code = -1;
JToken codeTok = json?["code"];
if (codeTok != null && codeTok.Type != JTokenType.Null)
{
try { code = codeTok.Value<int>(); } catch { code = -1; }
}📝 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
| int code = 0; | |
| JToken codeTok = json?["code"]; | |
| if (codeTok != null && codeTok.Type != JTokenType.Null) | |
| { | |
| try { code = codeTok.Value<int>(); } catch { code = -1; } | |
| } | |
| if (!httpOk || code != 0) | |
| { | |
| string detail = json?["message"]?.ToString() | |
| ?? json?["error"]?.ToString() | |
| ?? ProviderHttp.Truncate(text); | |
| throw new Exception(SecretRedactor.Scrub( | |
| $"Tripo {phase} failed (status={res?.Status}, code={code}): {detail}", apiKey)); | |
| } | |
| int code = -1; | |
| JToken codeTok = json?["code"]; | |
| if (codeTok != null && codeTok.Type != JTokenType.Null) | |
| { | |
| try { code = codeTok.Value<int>(); } catch { code = -1; } | |
| } | |
| if (!httpOk || code != 0) | |
| { | |
| string detail = json?["message"]?.ToString() | |
| ?? json?["error"]?.ToString() | |
| ?? ProviderHttp.Truncate(text); | |
| throw new Exception(SecretRedactor.Scrub( | |
| $"Tripo {phase} failed (status={res?.Status}, code={code}): {detail}", apiKey)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Services/AssetGen/Providers/TripoAdapter.cs` around lines
203 - 217, The TripoAdapter.PollAsync success check currently treats a missing
or malformed Tripo response code as success because code defaults to 0. Change
the parsing logic so a 200 response is only accepted when the JSON contains a
valid code field equal to 0, and otherwise fall through to the failure path in
the existing httpOk/code check. Use the TripoAdapter.PollAsync response handling
around codeTok and the throw with SecretRedactor.Scrub to locate the fix.
Comment on lines
+283
to
+296
| clearButton.clicked += () => | ||
| { | ||
| try | ||
| { | ||
| SecureKeyStore.Current.Delete(id); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| McpLog.Warn($"Failed to delete {id} key: {ex.Message}"); | ||
| } | ||
|
|
||
| keyField.SetValueWithoutNotify(string.Empty); | ||
| SetStatus(statusLabel, "not set", false); | ||
| }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't report the key as cleared when deletion fails.
If SecureKeyStore.Current.Delete(id) throws, the catch only logs, but the UI still clears the field and sets the status to not set. That can leave the secret stored while telling the user it was removed. Update the label only after a successful delete, and show a failure state otherwise.
Suggested fix
clearButton.clicked += () =>
{
try
{
SecureKeyStore.Current.Delete(id);
+ keyField.SetValueWithoutNotify(string.Empty);
+ SetStatus(statusLabel, "not set", false);
}
catch (Exception ex)
{
McpLog.Warn($"Failed to delete {id} key: {ex.Message}");
+ SetStatus(statusLabel, "clear failed", false);
}
-
- keyField.SetValueWithoutNotify(string.Empty);
- SetStatus(statusLabel, "not set", false);
};📝 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
| clearButton.clicked += () => | |
| { | |
| try | |
| { | |
| SecureKeyStore.Current.Delete(id); | |
| } | |
| catch (Exception ex) | |
| { | |
| McpLog.Warn($"Failed to delete {id} key: {ex.Message}"); | |
| } | |
| keyField.SetValueWithoutNotify(string.Empty); | |
| SetStatus(statusLabel, "not set", false); | |
| }; | |
| clearButton.clicked += () => | |
| { | |
| try | |
| { | |
| SecureKeyStore.Current.Delete(id); | |
| keyField.SetValueWithoutNotify(string.Empty); | |
| SetStatus(statusLabel, "not set", false); | |
| } | |
| catch (Exception ex) | |
| { | |
| McpLog.Warn($"Failed to delete {id} key: {ex.Message}"); | |
| SetStatus(statusLabel, "clear failed", false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.cs` around
lines 283 - 296, The clear-button handler in McpAssetGenSection is always
updating the UI even when SecureKeyStore.Current.Delete(id) fails. Move the
keyField.SetValueWithoutNotify and SetStatus(statusLabel, "not set", false)
calls so they only run after a successful delete, and in the catch block set a
failure state instead of pretending the key was cleared. Use the existing
clearButton.clicked handler, SecureKeyStore.Current.Delete, keyField, and
SetStatus symbols to locate and update the flow.
Comment on lines
+19
to
+20
| @mcp_for_unity_tool( | ||
| group="asset_gen", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
asset_gen is outside the repo’s approved MCP tool-group list.
This decorator is introducing a group value that does not match the allowed set in the current repo guidance for Server/src/services/tools/*.py. Either move the tool into an approved group or update the repo’s tool-group contract in the same change so the new registration is policy-compliant. As per coding guidelines, Server/src/services/tools/*.py: Python MCP tools must include a group parameter in the @mcp_for_unity_tool decorator (core, vfx, animation, ui, scripting_ext, testing, probuilder, profiling, or docs).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Server/src/services/tools/generate_image.py` around lines 19 - 20, The
`generate_image` tool is registered with an unsupported MCP group value in its
`@mcp_for_unity_tool` decorator. Update `generate_image` to use one of the
approved groups for `Server/src/services/tools/*.py` (such as `core`, `vfx`,
`animation`, `ui`, `scripting_ext`, `testing`, `probuilder`, `profiling`, or
`docs`), or make the matching tool-group contract change in the same update so
the registration stays compliant.
Source: Coding guidelines
Comment on lines
+27
to
+28
| "- search: Search Sketchfab. Params: query, categories, downloadable, count, " | ||
| "cursor -> results with model uids.\n" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Adopt the repo paging contract for search.
This new search surface exposes count instead of the required page_size, so the MCP API now diverges from the standard large-result paging contract and will need special handling in other layers. Please align the request/response shape here with page_size + cursor + next_cursor before this ships. As per coding guidelines, "API results that could be large (hierarchies, components, search results) must implement paging using page_size and cursor parameters with next_cursor in response`."
Also applies to: 47-51, 60-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Server/src/services/tools/import_model.py` around lines 27 - 28, The `search`
MCP surface in `import_model.py` is using `count`, which breaks the repo’s
standard paging contract. Update the `search` request and response shape to use
`page_size` with `cursor` input and `next_cursor` in the result, and make sure
the `search` tool’s schema/docs and implementation stay aligned with that paging
flow. Use the `search` tool definition and any related response-building logic
in `import_model.py` as the main places to adjust.
Source: Coding guidelines
Comment on lines
+15
to
+34
| private static readonly string[] Keys = | ||
| { | ||
| EditorPrefKeys.AssetGenSelectedModelProvider, | ||
| EditorPrefKeys.AssetGenSelectedImageProvider, | ||
| EditorPrefKeys.AssetGenDefaultFormat, | ||
| EditorPrefKeys.AssetGenOutputRoot, | ||
| EditorPrefKeys.AssetGenAutoNormalize, | ||
| EditorPrefKeys.AssetGenProviderEnabledPrefix + "tripo", | ||
| }; | ||
|
|
||
| private void Clear() | ||
| { | ||
| foreach (string k in Keys) EditorPrefs.DeleteKey(k); | ||
| } | ||
|
|
||
| [SetUp] | ||
| public void SetUp() => Clear(); | ||
|
|
||
| [TearDown] | ||
| public void TearDown() => Clear(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't erase real EditorPrefs values from the user's editor session.
These tests delete the production Asset Gen keys in both SetUp() and TearDown(). Because EditorPrefs is persisted user state, running the suite locally will wipe the non-secret settings that the new Asset Gen UI stores. Please snapshot the original values and restore them after each test, or route AssetGenPrefs through a test-specific backend/key prefix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenPrefsTests.cs`
around lines 15 - 34, The AssetGenPrefsTests cleanup currently deletes real
EditorPrefs state, which can wipe a user’s persisted Asset Gen settings during
test runs. Update the test fixture around Clear(), SetUp(), and TearDown() to
preserve and restore the original values for the EditorPrefKeys entries instead
of blindly calling EditorPrefs.DeleteKey, or isolate AssetGenPrefs behind a
test-only backend/prefix so the tests only touch disposable data.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new optional asset_gen tool group to MCP for Unity that supports bring-your-own-key AI asset workflows: 3D generation (Tripo/Meshy), Sketchfab import, and 2D image generation (fal/OpenRouter). It introduces provider adapters + HTTP transport seams, secure key storage, safe archive extraction, Unity-side import pipelines, and extensive unit/editmode coverage plus docs.
Changes:
- Introduces
asset_gentool group (off by default) with new Python MCP tools/CLI wiring and Unity C# handlers for generate/import flows. - Adds secure key storage backends (OS stores + encrypted-file fallback), secret redaction, and hardened import paths (safe zip extraction allowlist).
- Adds Unity Editor “Asset Gen” tab UI scaffold and broad test coverage (Python pass-through + Unity EditMode adapter/handler/import tests) plus manual verification checklist.
Reviewed changes
Copilot reviewed 126 out of 142 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen.meta | New Unity test folder metadata for AssetGen EditMode tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/TripoAdapterTests.cs | EditMode tests for Tripo adapter request shaping and poll mapping. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/TripoAdapterTests.cs.meta | Unity meta for Tripo adapter tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.cs | EditMode tests for Sketchfab adapter search/resolve behavior and auth scheme. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SketchfabAdapterTests.cs.meta | Unity meta for Sketchfab adapter tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.cs | EditMode tests for encrypted key-store roundtrip/deletion/encryption-at-rest. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecureKeyStoreTests.cs.meta | Unity meta for SecureKeyStore tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecretRedactorTests.cs | EditMode tests ensuring auth header/token redaction. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SecretRedactorTests.cs.meta | Unity meta for SecretRedactor tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.cs | EditMode tests for allowlist extraction + zip-slip rejection. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/SafeZipExtractorTests.cs.meta | Unity meta for SafeZipExtractor tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/OpenRouterAdapterTests.cs | EditMode tests for OpenRouter inline image return + reference-image body shaping. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/OpenRouterAdapterTests.cs.meta | Unity meta for OpenRouter adapter tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ModelImportPipelineTests.cs | EditMode tests for model import pipeline guard/error paths. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ModelImportPipelineTests.cs.meta | Unity meta for ModelImportPipeline tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/MeshyAdapterTests.cs.meta | Unity meta for Meshy adapter tests (script not included in provided diff). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelHandlerTests.cs | EditMode tests driving import_model handler with fake transports + temp key store. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelHandlerTests.cs.meta | Unity meta for ImportModel handler tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelFileHandlerTests.cs | EditMode tests for import_model_file handler including traversal rejection. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/ImportModelFileHandlerTests.cs.meta | Unity meta for ImportModelFile handler tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateModelHandlerTests.cs | EditMode tests for generate_model handler (key checks, local image rules, provider list). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateModelHandlerTests.cs.meta | Unity meta for GenerateModel handler tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/GenerateImageTests.cs.meta | Unity meta for GenerateImage tests (script not included in provided diff). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/FalAdapterTests.cs | EditMode tests for fal adapter submit/poll + edit endpoint behavior. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/FalAdapterTests.cs.meta | Unity meta for fal adapter tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/EnvKeyOverrideTests.cs | EditMode tests for env-var override naming + resolution. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/EnvKeyOverrideTests.cs.meta | Unity meta for EnvKeyOverride tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/BlenderDetectionTests.cs | EditMode tests for deterministic Blender install detection core. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/BlenderDetectionTests.cs.meta | Unity meta for BlenderDetection tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenProvidersTests.cs | EditMode tests for provider registry + “Configured” bool (no key exposure). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenProvidersTests.cs.meta | Unity meta for AssetGenProviders tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenPrefsTests.cs | EditMode tests for non-secret AssetGen prefs stored in EditorPrefs. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenPrefsTests.cs.meta | Unity meta for AssetGenPrefs tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.cs | EditMode tests for job state machine + URL scheme restrictions + key non-leakage. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/AssetGen/AssetGenJobManagerTests.cs.meta | Unity meta for AssetGenJobManager tests. |
| Server/tests/test_asset_gen_scaffold.py | Python test ensuring asset_gen tool group exists and is disabled by default. |
| Server/tests/test_asset_gen_model.py | Python tests for generate_model tool routing, param mapping, and no-secret payload shaping. |
| Server/tests/test_asset_gen_import.py | Python tests for import_model tool routing, param mapping, and no-secret payload shaping. |
| Server/tests/test_asset_gen_import_file.py | Python tests for import_model_file tool routing, param mapping, and no-secret payload shaping. |
| Server/tests/test_asset_gen_image.py | Python tests for generate_image tool routing, param mapping, and no-secret payload shaping. |
| Server/src/services/tools/import_model.py | Adds import_model Python tool (Sketchfab import) as thin pass-through. |
| Server/src/services/tools/import_model_file.py | Adds import_model_file Python tool (local model import) as thin pass-through. |
| Server/src/services/tools/generate_model.py | Adds generate_model Python tool (Tripo/Meshy generation) as thin pass-through. |
| Server/src/services/tools/generate_image.py | Adds generate_image Python tool (fal/OpenRouter image gen) as thin pass-through. |
| Server/src/services/registry/tool_registry.py | Registers asset_gen tool group with description; remains disabled by default. |
| Server/src/cli/main.py | Registers asset_gen CLI command group. |
| README.md | Documents AI Asset Generation feature, setup, usage, and limitations. |
| MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml | Adds “Asset Gen” tab and panel containers to the editor window UXML. |
| MCPForUnity/Editor/Windows/Components/AssetGen.meta | Folder metadata for new editor UI AssetGen components. |
| MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxml | UXML for Asset Gen settings/key-management section UI. |
| MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.uxml.meta | Unity meta for AssetGen section UXML. |
| MCPForUnity/Editor/Windows/Components/AssetGen/McpAssetGenSection.cs.meta | Unity meta for AssetGen section script (script not included in provided diff). |
| MCPForUnity/Editor/Tools/AssetGen.meta | Folder metadata for AssetGen tool handlers. |
| MCPForUnity/Editor/Tools/AssetGen/GenerateModel.cs | Unity-side MCP tool handler for 3D generation job start/status/cancel/provider listing. |
| MCPForUnity/Editor/Tools/AssetGen/GenerateModel.cs.meta | Unity meta for GenerateModel handler. |
| MCPForUnity/Editor/Tools/AssetGen/GenerateImage.cs | Unity-side MCP tool handler for 2D image generation job start/status/cancel/provider listing. |
| MCPForUnity/Editor/Tools/AssetGen/GenerateImage.cs.meta | Unity meta for GenerateImage handler. |
| MCPForUnity/Editor/Tools/AssetGen/ImportModel.cs | Unity-side MCP tool handler for Sketchfab search/preview/import jobs. |
| MCPForUnity/Editor/Tools/AssetGen/ImportModel.cs.meta | Unity meta for ImportModel handler. |
| MCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs | Unity-side MCP tool handler for importing local model files into Assets and running pipeline. |
| MCPForUnity/Editor/Tools/AssetGen/ImportModelFile.cs.meta | Unity meta for ImportModelFile handler. |
| MCPForUnity/Editor/Services/AssetGen.meta | Folder metadata for AssetGen services. |
| MCPForUnity/Editor/Services/AssetGen/Providers.meta | Folder metadata for provider adapters. |
| MCPForUnity/Editor/Services/AssetGen/Providers/IProviderAdapters.cs | Defines provider adapter interfaces for model/image/marketplace. |
| MCPForUnity/Editor/Services/AssetGen/Providers/IProviderAdapters.cs.meta | Unity meta for provider adapter interfaces. |
| MCPForUnity/Editor/Services/AssetGen/Providers/ProviderModels.cs | Shared request/result models and provider info DTOs. |
| MCPForUnity/Editor/Services/AssetGen/Providers/ProviderModels.cs.meta | Unity meta for ProviderModels. |
| MCPForUnity/Editor/Services/AssetGen/Providers/ProviderHttp.cs | Shared helpers for extracting/truncating HTTP response text. |
| MCPForUnity/Editor/Services/AssetGen/Providers/ProviderHttp.cs.meta | Unity meta for ProviderHttp. |
| MCPForUnity/Editor/Services/AssetGen/Providers/AssetGenProviders.cs | Provider registry/factory + key-free provider listing. |
| MCPForUnity/Editor/Services/AssetGen/Providers/AssetGenProviders.cs.meta | Unity meta for AssetGenProviders. |
| MCPForUnity/Editor/Services/AssetGen/Providers/SketchfabAdapter.cs | Sketchfab adapter for search/preview/download URL resolution with token auth. |
| MCPForUnity/Editor/Services/AssetGen/Providers/SketchfabAdapter.cs.meta | Unity meta for SketchfabAdapter. |
| MCPForUnity/Editor/Services/AssetGen/Providers/OpenRouterAdapter.cs | OpenRouter adapter for synchronous inline-image generation via chat completions. |
| MCPForUnity/Editor/Services/AssetGen/Providers/OpenRouterAdapter.cs.meta | Unity meta for OpenRouterAdapter. |
| MCPForUnity/Editor/Services/AssetGen/Providers/FalAdapter.cs | fal adapter via queue API with submit/status/result retrieval; edit endpoint support. |
| MCPForUnity/Editor/Services/AssetGen/Providers/FalAdapter.cs.meta | Unity meta for FalAdapter. |
| MCPForUnity/Editor/Services/AssetGen/Providers/LocalImage.cs | Local image path validation + data-URI encoding for provider image inputs. |
| MCPForUnity/Editor/Services/AssetGen/Providers/LocalImage.cs.meta | Unity meta for LocalImage. |
| MCPForUnity/Editor/Services/AssetGen/Providers/TripoAdapter.cs.meta | Unity meta for Tripo adapter (adapter source not included in provided diff). |
| MCPForUnity/Editor/Services/AssetGen/Providers/MeshyAdapter.cs.meta | Unity meta for Meshy adapter (adapter source not included in provided diff). |
| MCPForUnity/Editor/Services/AssetGen/Http.meta | Folder metadata for HTTP transport seam. |
| MCPForUnity/Editor/Services/AssetGen/Http/IHttpTransport.cs | Defines HTTP transport interface for adapters. |
| MCPForUnity/Editor/Services/AssetGen/Http/IHttpTransport.cs.meta | Unity meta for IHttpTransport. |
| MCPForUnity/Editor/Services/AssetGen/Http/HttpRequestSpec.cs | Transport-agnostic request spec model. |
| MCPForUnity/Editor/Services/AssetGen/Http/HttpRequestSpec.cs.meta | Unity meta for HttpRequestSpec. |
| MCPForUnity/Editor/Services/AssetGen/Http/HttpResult.cs | Transport-agnostic response model + Ok helper. |
| MCPForUnity/Editor/Services/AssetGen/Http/HttpResult.cs.meta | Unity meta for HttpResult. |
| MCPForUnity/Editor/Services/AssetGen/Http/UnityWebRequestTransport.cs | Production transport using UnityWebRequest with async completion. |
| MCPForUnity/Editor/Services/AssetGen/Http/UnityWebRequestTransport.cs.meta | Unity meta for UnityWebRequestTransport. |
| MCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.cs | Test transport capturing requests and returning canned responses. |
| MCPForUnity/Editor/Services/AssetGen/Http/FakeHttpTransport.cs.meta | Unity meta for FakeHttpTransport. |
| MCPForUnity/Editor/Services/AssetGen/Import.meta | Folder metadata for import pipelines. |
| MCPForUnity/Editor/Services/AssetGen/Import/SafeZipExtractor.cs | Safe zip extraction with zip-slip prevention and extension allowlist. |
| MCPForUnity/Editor/Services/AssetGen/Import/SafeZipExtractor.cs.meta | Unity meta for SafeZipExtractor. |
| MCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs | Imports generated images and applies TextureImporter settings. |
| MCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs.meta | Unity meta for ImageImportPipeline. |
| MCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.cs.meta | Unity meta for ModelImportPipeline (pipeline source not included in provided diff). |
| MCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.cs.meta | Unity meta for AssetGenJobManager (manager source not included in provided diff). |
| MCPForUnity/Editor/Security.meta | Folder metadata for security components. |
| MCPForUnity/Editor/Security/SecureKeyStore.meta | Folder metadata for secure key store implementation. |
| MCPForUnity/Editor/Security/SecureKeyStore/ISecureKeyStore.cs | Interface contract for provider key storage backends. |
| MCPForUnity/Editor/Security/SecureKeyStore/ISecureKeyStore.cs.meta | Unity meta for ISecureKeyStore. |
| MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStoreConstants.cs | Shared constants including provider IDs and service namespace. |
| MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStoreConstants.cs.meta | Unity meta for SecureKeyStoreConstants. |
| MCPForUnity/Editor/Security/SecureKeyStore/EnvKeyOverride.cs | Env-var override for provider keys (read-only). |
| MCPForUnity/Editor/Security/SecureKeyStore/EnvKeyOverride.cs.meta | Unity meta for EnvKeyOverride. |
| MCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs | Regex-based redaction for auth scheme tokens + explicit secret replacement. |
| MCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs.meta | Unity meta for SecretRedactor. |
| MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStore.cs | Platform selection + env-overlay wrapper for secure key store access. |
| MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStore.cs.meta | Unity meta for SecureKeyStore. |
| MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs | macOS Keychain backend via security CLI. |
| MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs.meta | Unity meta for MacKeychainKeyStore. |
| MCPForUnity/Editor/Security/SecureKeyStore/WindowsCredentialKeyStore.cs | Windows Credential Manager backend via Cred* APIs. |
| MCPForUnity/Editor/Security/SecureKeyStore/WindowsCredentialKeyStore.cs.meta | Unity meta for WindowsCredentialKeyStore. |
| MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs | Linux libsecret backend via secret-tool CLI (stdin for secrets). |
| MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs.meta | Unity meta for LinuxSecretToolKeyStore. |
| MCPForUnity/Editor/Security/SecureKeyStore/EncryptedFileKeyStore.cs.meta | Unity meta for encrypted-file fallback backend (source not included in provided diff). |
| MCPForUnity/Editor/Helpers/BlenderDetection.cs | Adds best-effort Blender install detection for UI hinting. |
| MCPForUnity/Editor/Helpers/BlenderDetection.cs.meta | Unity meta for BlenderDetection helper. |
| MCPForUnity/Editor/Helpers/AssetGenPrefs.cs | Adds non-secret per-user AssetGen preferences (EditorPrefs). |
| MCPForUnity/Editor/Helpers/AssetGenPrefs.cs.meta | Unity meta for AssetGenPrefs. |
| MCPForUnity/Editor/Helpers/AssetGenPaths.cs | Adds safe Assets-relative/absolute path normalization helpers. |
| MCPForUnity/Editor/Helpers/AssetGenPaths.cs.meta | Unity meta for AssetGenPaths. |
| MCPForUnity/Editor/Constants/EditorPrefKeys.cs | Adds EditorPref keys for non-secret AssetGen settings. |
| docs/superpowers/specs/2026-06-28-blender-to-unity-handoff-design.md | Design doc for Blender→Unity handoff and import_model_file primitive. |
| docs/asset-gen-manual-verification.md | Manual verification checklist for live provider testing and security spot-checks. |
| .claude/skills/blender-to-unity/SKILL.md | Skill documentation for orchestrating Blender export → Unity import & placement. |
| .claude/skills/blender-to-unity/manual-verify.md | Manual verification checklist for the blender-to-unity skill. |
Files not reviewed (15)
- MCPForUnity/Editor/Helpers/AssetGenPaths.cs.meta: Generated file
- MCPForUnity/Editor/Helpers/AssetGenPrefs.cs.meta: Generated file
- MCPForUnity/Editor/Helpers/BlenderDetection.cs.meta: Generated file
- MCPForUnity/Editor/Security.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/EncryptedFileKeyStore.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/EnvKeyOverride.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/ISecureKeyStore.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/LinuxSecretToolKeyStore.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/MacKeychainKeyStore.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/SecretRedactor.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStore.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/SecureKeyStoreConstants.cs.meta: Generated file
- MCPForUnity/Editor/Security/SecureKeyStore/WindowsCredentialKeyStore.cs.meta: Generated file
- MCPForUnity/Editor/Services/AssetGen.meta: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
+43
| action: Annotated[Literal["generate", "remove_background", "status", "cancel", "list_providers"], | ||
| "Action to perform."], |
Comment on lines
+28
to
+31
| case "generate": return Generate(p); | ||
| case "remove_background": | ||
| return new ErrorResponse("remove_background is not implemented in this version. Generate with transparency-capable models instead."); | ||
| case "status": return Status(p); |
Comment on lines
+65
to
+70
| private static string ResolveSource(string source) | ||
| { | ||
| string s = source.Replace('\\', '/'); | ||
| if (s == "Assets" || s.StartsWith("Assets/")) return AssetGenPaths.ToAbsolute(s); | ||
| return s; // absolute path on disk | ||
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/docs/reference/tools/asset_gen/generate_image.md`:
- Line 32: The `image_path` description in `generate_image.md` incorrectly
mentions `remove_background`, which is not supported. Update the table entry for
`image_path` to describe only the valid image-to-image use case, using the
`generate_image` documentation text and removing any reference to
`remove_background`.
- Line 18: Remove the misleading “sync providers resolve immediately”
parenthetical from the generate_image documentation entry, since generate_image
always returns a pending response with job_id. Update the description in the
generate_image docs to match the actual Generate() flow that calls
AssetGenJobManager.StartImageGeneration() and returns PendingResponse, and keep
the parameter list unchanged.
- Line 19: Update the `generate_image.md` tool reference so the
`remove_background` entry does not describe a successful job flow; instead, mark
`remove_background` as unsupported/unimplemented in this version and note that
it returns an error rather than a `job_id`. Use the `remove_background` action
name in the docs to clearly match the C# handler behavior and avoid implying the
feature is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87e56c54-7a87-4ec0-ba9f-d34254cac30f
📒 Files selected for processing (8)
website/docs/reference/tools/asset_gen/_category_.jsonwebsite/docs/reference/tools/asset_gen/generate_image.mdwebsite/docs/reference/tools/asset_gen/generate_model.mdwebsite/docs/reference/tools/asset_gen/import_model.mdwebsite/docs/reference/tools/asset_gen/import_model_file.mdwebsite/docs/reference/tools/asset_gen/index.mdwebsite/docs/reference/tools/core/manage_tools.mdwebsite/docs/reference/tools/index.md
✅ Files skipped from review due to trivial changes (7)
- website/docs/reference/tools/asset_gen/category.json
- website/docs/reference/tools/core/manage_tools.md
- website/docs/reference/tools/asset_gen/index.md
- website/docs/reference/tools/asset_gen/import_model.md
- website/docs/reference/tools/index.md
- website/docs/reference/tools/asset_gen/import_model_file.md
- website/docs/reference/tools/asset_gen/generate_model.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI Asset Generation (3D model gen/import + 2D image)
Bring-your-own-key AI asset generation for MCP for Unity: 3D model generation (Tripo/Meshy), Sketchfab marketplace import, and 2D image generation (fal.ai/OpenRouter) — results import straight into
Assets/Generated/. Provider keys live in the OS secure store and never cross the bridge. (~21 commits; draft for review.)Architecture
EditorPrefs, logs, tool output, job records, or git.Recent hardening (
2efb7860) — security + correctness + localimage_pathSurfaced by a security audit + code review of the feature. NB: everything here is on the unreleased feature branch — no shipped version is affected; this lands the fixes before the feature merges.
Security
.cs/.dllunderAssets/and have the Editor compile/load it (code execution on import).file://SSRF / local-file read into the project.Provider correctness (each was silent wrong-output)
texture=trueactually returns a textured model./editendpoint +image_urlsarray; fal honorswidth/height.categories/count/cursor/downloadable;previewdoc corrected (returns metadata, not a base64 thumbnail).AssetDatabase.Refresh()before importing a freshly written file.Local image input —
image_pathis read and sent inline as a base64 data URI for Meshy / fal / OpenRouter; Tripo image→3D needs a hostedimage_url(returns a clear error for local files).Tests / verification
tools/local_harness.py): 968/1014 passed, 0 failures (46 pre-existing skips).docs/asset-gen-manual-verification.md.Known follow-ups
/edit, and theimage_pathdata-URI paths are not live-verified).transparentis import-flag-only (fal/FLUX has no transparent-background generation).https://claude.ai/code/session_015DAUrMR5UaSEzEn2wNPrEP
Summary by CodeRabbit
New Features
Bug Fixes
Tests