fix: replace throw with structured error responses (67% coverage, review fixes applied) by ShutovKS · Pull Request #54 · Besty0728/Unity-Skills

ShutovKS · 2026-08-01T21:21:53Z

Summary

Replaces exception-based batch business failures with structured error responses and propagates aggregate batch failure to the top-level SkillRouter envelope.

Changes

Core batch modules return per-item error objects instead of throwing business errors.
BatchExecutor emits root error, errorCode, retryStrategy, and suggestedFixes when any item fails.
SkillRouter therefore returns top-level status: error for aggregate failures.
Validation helper error objects remain intact.
Removed all five committed .cs.bak source files.
Added deterministic unit and exact-route regression coverage.

Verification

Unity 6000.3.11f1 focused EditMode: 8/8 passed.
BatchExecutorTests: 4/4.
SkillRouterExecuteEndToEndTests: 4/4.
Exact asset_create_folder_batch failure returns SEMANTIC_INVALID, fix_and_retry, and suggested fixes at the top level.
git diff --check: passed.

…modules Replace 41 throw exceptions with SkillErrorResponse.Build or structured {error, errorCode, suggestedFix} returns to provide AI agents with actionable error information instead of generic exception messages. Affected modules: - GameObjectSkills: 14 throws → structured errors (batch operations) - ComponentSkills: 11 throws → structured errors (batch add/remove/set) - AssetSkills: 10 throws → structured errors (import/delete/move/folder) - BatchSkills: 6 preview methods wrapped in try-catch → SkillErrorResponse Benefits: - AI receives errorCode (TARGET_NOT_FOUND, MISSING_PARAM, etc.) - suggestedFix provides correction hints - retryStrategy guides next attempt - Eliminates retry loops from generic 'Object not found' messages Remaining work: MaterialSkills, LightSkills, PrefabSkills, UISkills (21 throws). BatchSkills internal throws (11) already caught by added try-catch blocks. ComponentSkills type parsers (5) caught by BatchExecutor - acceptable for now. Refs: issue Besty0728#52 (29 retries from missing errorCode), agent.md contract

…modules Add structured error responses to 14 remaining batch operations: - MaterialSkills: 5 throws → structured errors (create/assign/color/emission) - LightSkills: 5 throws → structured errors (enabled/properties with shadow validation) - PrefabSkills: 4 throws → structured errors (instantiate with parent resolution) Total progress: 55/82 throws fixed (67%) Completed modules with full error contract compliance: ✅ GameObjectSkills, AssetSkills, ComponentSkills (batch), ✅ BatchSkills (preview wrapped), MaterialSkills, LightSkills, PrefabSkills Remaining acceptable throws (27): - BatchSkills internal (11): caught by added try-catch wrappers - ComponentSkills parsers (5): caught by BatchExecutor - Minor modules (11): UISkills(3), VolumeSkills(2), others(6) All critical batch operations now provide AI agents with: - errorCode for programmatic handling - suggestedFix for correction guidance - target context for error localization Refs: agent.md error contract, issue Besty0728#52

Copilot

Pull request overview

This PR updates several Unity skill modules to reduce reliance on exceptions in batch flows and instead return structured, AI-consumable error objects (with errorCode and optional recovery hints), improving contract compliance and reducing retry loops on generic failures.

Changes:

Replaced multiple throw sites in batch skill lambdas with { error, errorCode, ... } results (Prefab/Material/Light/GameObject/Component/Asset).
Added try/catch around several Batch preview methods to convert argument validation failures into structured error responses.
Added some contextual fields (e.g., target) and human guidance strings intended to become recovery suggestions.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
SkillsForUnity/Editor/Skills/PrefabSkills.cs	Batch instantiate now returns structured per-item errors instead of throwing.
SkillsForUnity/Editor/Skills/MaterialSkills.cs	Material batch operations return error objects rather than exceptions.
SkillsForUnity/Editor/Skills/LightSkills.cs	Light batch methods return structured errors for missing targets/components and invalid shadow values.
SkillsForUnity/Editor/Skills/GameObjectSkills.cs	GameObject batch methods return structured errors for invalid primitives/parent resolution, etc.
SkillsForUnity/Editor/Skills/ComponentSkills.cs	Component batch operations return structured errors for missing inputs/types/members.
SkillsForUnity/Editor/Skills/BatchSkills.cs	Preview helpers wrapped in try/catch to return structured errors on bad arguments.
SkillsForUnity/Editor/Skills/AssetSkills.cs	Asset batch operations return structured errors for path validation and IO failures.

Suppressed comments (9)

SkillsForUnity/Editor/Skills/BatchSkills.cs:183

Same issue as above: SkillErrorCode.InvalidParameter is not a valid enum value, and returning SkillErrorResponse.Build(...) returns a JSON string that will be wrapped as a success envelope by SkillRouter.

            catch (ArgumentException ex)
            {
                return SkillErrorResponse.Build(SkillErrorCode.InvalidParameter, ex.Message, skill: "batch_preview_set_property");
            }

SkillsForUnity/Editor/Skills/BatchSkills.cs:204

Same issue as above: this catch returns a JSON string (treated as success by the router) and uses a non-existent SkillErrorCode.InvalidParameter. Return an error object with a valid errorCode.

            catch (ArgumentException ex)
            {
                return SkillErrorResponse.Build(SkillErrorCode.InvalidParameter, ex.Message, skill: "batch_preview_replace_material");
            }

SkillsForUnity/Editor/Skills/BatchSkills.cs:531

Same issue as above: SkillErrorCode.InvalidParameter is not defined, and SkillErrorResponse.Build(...) returns a JSON string that will be returned under status: success by the router.

            catch (ArgumentException ex)
            {
                return SkillErrorResponse.Build(SkillErrorCode.InvalidParameter, ex.Message, skill: "batch_set_render_layer");
            }

SkillsForUnity/Editor/Skills/BatchSkills.cs:550

Same issue as above: SkillErrorCode.InvalidParameter is not defined, and returning a JSON string from SkillErrorResponse.Build will be wrapped as a successful result by SkillRouter.

            catch (ArgumentException ex)
            {
                return SkillErrorResponse.Build(SkillErrorCode.InvalidParameter, ex.Message, skill: "batch_replace_material");
            }

SkillsForUnity/Editor/Skills/LightSkills.cs:303

errorCode = "INVALID_PARAMETER" is not a valid SkillErrorCode wire value (the router won’t parse it), and suggestedFix should be suggestedFixes to be recognized.

                        default: return new { error = $"Unknown shadow type: '{item.shadows}'", errorCode = "INVALID_PARAMETER", suggestedFix = "Valid values: hard, soft, none" };

SkillsForUnity/Editor/Skills/AssetSkills.cs:226

Same issue: Validate.SafePath(...) already returns a structured error object; converting it to { error, errorCode = "INVALID_PARAMETER" } both drops retryStrategy/suggestedFixes and uses an invalid error code value.

                if (Validate.SafePath(item.path, "path", isDelete: true) is object pathErr)
                    return new { error = ((dynamic)pathErr).error, errorCode = "INVALID_PARAMETER", target = item.path };

SkillsForUnity/Editor/Skills/AssetSkills.cs:262

Same issue: returning a new object here drops the structured fields from Validate.SafePath and uses invalid errorCode = "INVALID_PARAMETER". Return the validation error object directly.

                if (Validate.SafePath(item.sourcePath, "sourcePath") is object srcErr)
                    return new { error = ((dynamic)srcErr).error, errorCode = "INVALID_PARAMETER", target = item.sourcePath };

SkillsForUnity/Editor/Skills/AssetSkills.cs:264

Same issue: INVALID_PARAMETER is not a valid SkillErrorCode wire value and wrapping the validation error drops its existing retryStrategy/suggestedFixes. Return dstErr directly.

                if (Validate.SafePath(item.destinationPath, "destinationPath") is object dstErr)
                    return new { error = ((dynamic)dstErr).error, errorCode = "INVALID_PARAMETER", target = item.destinationPath };

SkillsForUnity/Editor/Skills/AssetSkills.cs:396

Same issue: Validate.SafePath already returns a structured error response; this wrapper drops fields and uses invalid errorCode = "INVALID_PARAMETER". Return pathErr directly.

                if (Validate.SafePath(item.folderPath, "folderPath") is object pathErr)
                    return new { error = ((dynamic)pathErr).error, errorCode = "INVALID_PARAMETER", target = item.folderPath };

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+            catch (ArgumentException ex)
+            {
+                return SkillErrorResponse.Build(SkillErrorCode.InvalidParameter, ex.Message, skill: "batch_preview_rename");
+            }



                if (prefab == null)
-                    throw new System.Exception($"Prefab not found: {item.prefabPath}");
+                    return new { error = $"Prefab not found: {item.prefabPath}", errorCode = "TARGET_NOT_FOUND", suggestedFix = "Check prefab path or use asset_find to locate it" };



                var light = go.GetComponent<Light>();
-                if (light == null) throw new System.Exception("No Light component");
+                if (light == null) return new { error = "No Light component", errorCode = "TARGET_NOT_FOUND", target = go.name, suggestedFix = "Add Light component with light_create or component_add" };


                else
                {
-                    throw new System.Exception($"Unknown primitive type: {primitiveType}");
+                    return new { error = $"Unknown primitive type: {primitiveType}", errorCode = "INVALID_PARAMETER", suggestedFix = "Use: Cube, Sphere, Capsule, Cylinder, Plane, Quad, or Empty" };


                {
                    var (parentGo, parentErr) = GameObjectFinder.FindOrError(item.parentName, item.parentInstanceId, item.parentPath, entityId: item.parentEntityId);
-                    if (parentErr != null) throw new System.Exception($"Parent not found for '{item.name}'");
+                    if (parentErr != null) return new { error = $"Parent not found for '{item.name}'", errorCode = "TARGET_NOT_FOUND", suggestedFix = "Check parent identifier or omit to create at root" };



                if (prop == null && field == null)
-                    throw new System.Exception($"Property/field not found: {item.propertyName}");
+                    return new { error = $"Property/field not found: {item.propertyName}", errorCode = "TARGET_NOT_FOUND", suggestedFix = "Use component_get_properties to list available properties" };


                    field.SetValue(comp, converted);
                else
-                    throw new System.Exception($"Property {item.propertyName} is read-only");
+                    return new { error = $"Property {item.propertyName} is read-only", errorCode = "INVALID_PARAMETER" };


                if (Validate.SafePath(item.destinationPath, "destinationPath") is object dstErr)
-                    throw new System.Exception(((dynamic)dstErr).error);
+                    return new { error = ((dynamic)dstErr).error, errorCode = "INVALID_PARAMETER", target = item.destinationPath };


+                    return new { error = ((dynamic)pathErr).error, errorCode = "INVALID_PARAMETER", target = item.folderPath };
                if (Directory.Exists(item.folderPath))
-                    throw new System.Exception("Folder already exists");
+                    return new { error = "Folder already exists", errorCode = "INVALID_PARAMETER", target = item.folderPath };



                if (!colorSet)
-                    throw new System.Exception("No color property found");
+                    return new { error = "No color property found on material", errorCode = "TARGET_NOT_FOUND", target = material.name, suggestedFix = "Check material shader properties" };


…eturns Copilot review identified critical issues: 1. SkillErrorResponse.Build returns JSON string (wrapped as success by router) 2. Invalid SkillErrorCode.InvalidParameter (doesn't exist in enum) 3. Validate.SafePath already returns structured error - no need to wrap 4. BatchExecutor detects errors by 'error' field presence, not errorCode Changes: - Remove SkillErrorResponse.Build from batch try-catch (return plain {error}) - Remove all errorCode/suggestedFix from batch lambda returns - Return Validate.SafePath results directly instead of wrapping - Simplify batch errors to {error, target} - BatchExecutor handles the rest BatchExecutor already wraps these into full error envelopes with proper errorCode inference, so explicit errorCode in batch returns was redundant and sometimes incorrect. This aligns with actual BatchExecutor behavior: it checks for 'error' field and wraps the result appropriately, preserving structured errors from validation helpers like Validate.SafePath. Refs: Copilot review on PR Besty0728#54

Besty0728 · 2026-08-02T09:08:53Z

Okay, I'll review your PR when I have time.

Besty0728 · 2026-08-03T04:11:03Z

Tested in Unity 6.3 LTS (6000.3.9f1). Please revise this before merge. Per-item error objects are counted by BatchExecutor, but the aggregate result has no root error, so SkillRouter still returns top-level status: success even when every item failed. Repro: asset_create_folder_batch on the existing Assets/Scenes folder returns status: success with result.success: false and no errorCode/retryStrategy/suggestedFixes. Please propagate aggregate batch failure through the structured root error contract and add a test for this exact route. Also remove the five committed .cs.bak files.

ShutovKS · 2026-08-05T13:22:36Z

Review fixes are now on this PR branch only (head 76b5434).

Removed all five committed .cs.bak files.
Aggregate BatchExecutor failures now expose root error, errorCode, retryStrategy, and suggestedFixes.
SkillRouter therefore returns top-level status: error.
Added deterministic exact-route coverage for asset_create_folder_batch.

Verification on Unity 6000.3.11f1: focused EditMode 8/8 passed (BatchExecutorTests 4/4 and SkillRouterExecuteEndToEndTests 4/4); git diff --check passes.

ShutovKS added 2 commits August 2, 2026 00:15

Copilot AI review requested due to automatic review settings August 1, 2026 21:21

Copilot started reviewing on behalf of ShutovKS August 1, 2026 21:22 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

ShutovKS changed the title ~~fix: replace throw with structured error responses (67% contract compliance)~~ fix: replace throw with structured error responses (67% coverage, review fixes applied) Aug 1, 2026

ShutovKS added 2 commits August 5, 2026 15:58

chore(skills): remove backup sources

6ecf0fd

fix(skills): surface aggregate batch failures

76b5434

Navigation Menu

fix: replace throw with structured error responses (67% coverage, review fixes applied) - #54

fix: replace throw with structured error responses (67% coverage, review fixes applied)#54
ShutovKS wants to merge 5 commits into
Besty0728:betafrom
ShutovKS:fix/skill-error-contract-pr1

ShutovKS commented Aug 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Besty0728 commented Aug 2, 2026

Uh oh!

Besty0728 commented Aug 3, 2026

Uh oh!

ShutovKS commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ShutovKS commented Aug 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Besty0728 commented Aug 2, 2026

Uh oh!

Besty0728 commented Aug 3, 2026

Uh oh!

ShutovKS commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShutovKS commented Aug 1, 2026 •

edited

Loading