Skip to content

SkillsModeManager fresh-install default: revert Auto→Approval (deferred from #1 PR#2 — needs test-env setup first) #4

Description

@Scaler0222

Background

Filing this so the change is tracked separately and the rest of #1 isn't held up by it.

SkillsModeManager.cs:143 currently returns SkillsOperatingMode.Auto for a fresh install (no UnitySkills_OperatingMode pref, no legacy install marker). The test contract — SkillsModeManagerTests.CurrentMode_FreshInstall_NoKeys_DefaultsToApproval (Test Matrix Besty0728#17) — says it should be Approval. A brand-new install should require explicit user consent before any AI-driven skill runs.

Decision D1 of the #1 plan was to revert that fallback to Approval in PR#2. It was attempted and rolled back during PR#2 verification because the change triggers exactly the regression Codex flagged at the Define→Develop debate gate (R2):

ForgeAI.Tests.EditMode.ContentAgents.ForgePrefabBuildSpecTests.BuildPrefab_AppliesMaterial_WhenProvided
ForgeAI.Tests.EditMode.ContentAgents.ForgePrefabBuildSpecTests.Parity_EnemyPrefab_HasExpectedComponentShape  (also flaked under D1)
UnitySkills.Tests.Core.EditorUndoRedoTests.EditorUndoRedo_RestoresLatestSkillMutation
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartAlignToGround_AlignsSelectedObjects
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartDistribute_SpreadsMiddleObjectsBetweenEndpoints
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartRandomizeTransform_ModifiesSelection
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartReplaceObjects_ReplacesSelectedObjectsWithPrefab
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartSceneLayout_ExecutesAgainstCurrentSelection
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartSelectByComponent_SelectsMatchingObjects
UnitySkills.Tests.Core.SelectionDrivenSkillTests.SmartSnapToGrid_SnapsSelection
UnitySkills.Tests.Core.SelectionDrivenSkillTests.UIAlignSelected_AlignsRectTransforms
UnitySkills.Tests.Core.SelectionDrivenSkillTests.UIDistributeSelected_RepositionsRectTransformsEvenly

All 8 SelectionDrivenSkillTests + EditorUndoRedoTests fixtures fail with the same shape: Expected: "success" / But was: "error" — the SkillRouter envelope returns NeedsGrant because the affected skills (FullAuto / MutatesScene = true) hit the Approval-mode gate without any grant set up in [SetUp]. These tests rode on the implicit Auto default; flipping it to Approval is a silent behavior break.

Why this needs its own PR

The fix is not the one-line revert. The test environment needs a deterministic mode set BEFORE any UnitySkills.Tests.Core test runs, so the default fall-through value doesn't matter for the tests at all.

Recommended shape:

// Tests/Editor/Core/UnitySkillsTestEnvironment.cs
using NUnit.Framework;
using UnityEditor;

namespace UnitySkills.Tests.Core
{
    [SetUpFixture]
    public class UnitySkillsTestEnvironment
    {
        private const string PrefKeyMode = "UnitySkills_OperatingMode";
        private string _savedMode;
        private bool _hadSavedMode;

        [OneTimeSetUp]
        public void OneTimeSetUp()
        {
            // Pin Bypass for the test suite — tests that need a specific mode
            // (SkillsModeManagerTests) explicitly set + reset their own state.
            _hadSavedMode = EditorPrefs.HasKey(PrefKeyMode);
            if (_hadSavedMode) _savedMode = EditorPrefs.GetString(PrefKeyMode);
            EditorPrefs.SetString(PrefKeyMode, "Bypass");
        }

        [OneTimeTearDown]
        public void OneTimeTearDown()
        {
            if (_hadSavedMode) EditorPrefs.SetString(PrefKeyMode, _savedMode);
            else EditorPrefs.DeleteKey(PrefKeyMode);
        }
    }
}

Once that lands, SkillsModeManager.cs:143 can flip from Auto to Approval without breaking anything — the test suite becomes default-agnostic.

Tests already in place for the eventual revert

PR#2 of #1 (the one that filed this issue) added the migration test matrix in SkillsModeManagerTests.cs as a forward-promise:

  • CurrentMode_ExplicitAutoPref_PersistsAcrossFreshInstallDefaultRevert — pins case (a): explicit pref wins.
  • CurrentMode_MigrationMatrix_AllThreeBranchesHonorIntent — pins cases (a)/(b)/(c) of the getter. Case (b) currently asserts Auto (the deferred state); the assertion message explicitly calls out it needs flipping when D1 lands.

Acceptance criteria

  • Tests/Editor/Core/UnitySkillsTestEnvironment.cs (or equivalent global setup) sets Bypass for the suite.
  • Editor/Skills/SkillsModeManager.cs:143 returns SkillsOperatingMode.Approval for the fresh-install path.
  • SkillsModeManagerTests.CurrentMode_FreshInstall_NoKeys_DefaultsToApproval passes.
  • SkillsModeManagerTests.CurrentMode_MigrationMatrix_AllThreeBranchesHonorIntent case-(b) assertion flips to Approval in lock-step.
  • No regression in the 8 fixtures listed above.
  • Forge baseline (host project, 761 tests) stays green.

Gate artifact

Adversarial Define→Develop debate transcript that originally flagged this is at the consumer-project path .claude/embrace-gate-define-develop-2026-05-28.md (3-way: Codex STOP / Gemini-fast PROCEED / Claude PROCEED-with-remediations). Codex's R2 finding is the one that ended up calling this exactly right.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions