Skip to content

fix: preserve Bypass mode after domain reload - #57

Open
yany79 wants to merge 1 commit into
Besty0728:betafrom
yany79:fix/preserve-bypass-after-domain-reload
Open

fix: preserve Bypass mode after domain reload#57
yany79 wants to merge 1 commit into
Besty0728:betafrom
yany79:fix/preserve-bypass-after-domain-reload

Conversation

@yany79

@yany79 yany79 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

An explicitly chosen operating mode silently reverts to Auto after a domain reload. The user picks Bypass in the panel, keeps working, and some time later — typically after a domain reload following a test run — the package is back in Auto and starts gating operations that were previously running unattended.

Nothing in the UI indicates the mode was reset, and the audit log shows no mode_changed event for the reversion, because no code ever set the mode back to Auto. The stored preference simply stopped existing.

Root cause

SkillsModeManager.ResetForTests() gives each test a clean slate by deleting five machine-wide EditorPrefs keys — including UnitySkills_OperatingMode:

EditorPrefs.DeleteKey(PrefKeyMode);
EditorPrefs.DeleteKey(PrefKeyPanelApproval);
EditorPrefs.DeleteKey(PrefKeyAllowlist);
EditorPrefs.DeleteKey(PrefKeyMigrationDone);
EditorPrefs.DeleteKey(PrefKeyLegacyGranted);

The fixtures are careful about this: they snapshot the real values in [OneTimeSetUp] and put them back in [OneTimeTearDown]. But ResetForTests() runs in both [SetUp] and [TearDown], so the user's preferences are deleted for the entire duration of the fixture and are restored only by that single [OneTimeTearDown] at the very end.

That makes the recovery path fragile. If the run never reaches [OneTimeTearDown] — a domain reload triggered mid-run by a recompile or by a skill under test, a cancelled run, an aborted run — the key stays deleted, and the fixture's in-memory snapshot dies with the domain.

The reversion to Auto specifically comes from the no-key fallback in the CurrentMode getter:

if (EditorPrefs.HasKey(PrefKeyMode)) { /* parse and return */ }
return IsExistingInstall() ? SkillsOperatingMode.Bypass : SkillsOperatingMode.Auto;

IsExistingInstall() is true only when one of seven legacy UnitySkills_* keys is present (RequireConfirmation, PreferredPort, LogLevel, Language, RequestTimeoutMinutes, KeepAliveIntervalSeconds, AutoInstallPackagesOnStartup). Those keys are written when the user changes the corresponding setting. A user who installed the package and never touched any of them has none of them, so the fallback resolves to Auto — and their deleted Bypass reads back as Auto. On a machine that happens to have one of those keys, the same deletion falls back to Bypass and the bug is invisible. This is why it reproduces on some setups and not others.

Two things widen the blast radius:

  • ResetForTests() is the only production code path that deletes UnitySkills_OperatingMode, so this is the entire surface of the bug — but it is also entirely sufficient to cause it.
  • Any project that lists com.besty.unity-skills in its testables runs these fixtures as part of its own EditMode suite. The user does not have to be developing the package to lose their mode; running their own project's tests is enough.

The same reasoning applies to the other four keys deleted alongside it — panel-approval preference, allowlist, migration flag, and the legacy granted-skills list.

Fix

Add a recovery path that survives the domain reload that breaks the existing one.

  1. CaptureResetForTests() now writes the five preference values into SessionState before deleting them. SessionState survives domain reloads, which is precisely what the in-memory fixture snapshot does not do. The capture is guarded by an Active flag so only the first ResetForTests() of a session records values; the subsequent per-test calls do not overwrite the pristine snapshot with the already-wiped state.

  2. RestoreSkillsModeManager is marked [InitializeOnLoad] and its new static constructor calls RestorePreferencesAfterTestDomainReload(). On every domain load, if the recovery flag is set, the saved values are written back — including correctly re-deleting keys that genuinely did not exist before the run — and the recovery data is cleared.

  3. Disarm — each fixture's [OneTimeTearDown] calls CompleteTestPreferenceRecovery() after restoring its own snapshot. A clean run therefore clears the recovery data itself, so a later unrelated domain reload cannot resurrect stale values.

The net effect: on a clean run nothing changes, and on an interrupted run the user's preferences come back at the next domain reload instead of being lost.

RestorePreferencesAfterTestDomainReload() is exposed as internal so the regression test can drive the recovery step directly rather than having to provoke a real domain reload inside a test.

Cost outside of tests is one SessionState.GetBool per domain load.

Test coverage

ResetForTests_DomainReloadRecovery_RestoresExplicitBypassMode covers the exact reported scenario: set Bypass explicitly, call ResetForTests(), assert the key is gone, then run the recovery that the static constructor would run after a reload, and assert the mode is Bypass again.

Known limitations

  • Recovery is keyed on SessionState, which does not survive an Editor restart. If the Editor crashes or is killed mid-run rather than merely reloading the domain, the preferences are still lost. Covering that would need a disk-backed sidecar; that seemed disproportionate for the failure mode.
  • Recovery happens at the next domain reload. After a cancelled run, the mode reads as the fallback until then.

Alternatives considered

The deeper fix is for ResetForTests() never to touch machine-wide EditorPrefs at all — routing mode/allowlist state through an injectable preference store, or namespacing the keys under a test-scoped prefix. That removes the failure mode instead of recovering from it, but it changes what the existing fixtures are actually asserting and touches every consumer of those keys. This PR is deliberately the narrow, behavior-preserving safety net; the isolation refactor is worth doing separately if you want it.

Testing

  • New regression test added; not executed — the Unity Editor on this machine is in use by unrelated work and running EditMode tests forces a domain reload.
  • Verified statically on a live install instead: with the patch compiled into the Editor, UnitySkills_OperatingMode survives domain reloads intact, the new [InitializeOnLoad] static constructor raises no type-initialization error, and skill execution continues to run with Bypass semantics.
  • Note that the install used for that check predates this PR's base branch, so only the SkillsModeManager.cs change and the SkillsModeManagerTests.cs change were exercised there; the two other fixture teardowns were not.

CI on beta is the authoritative check for this one.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@yany79
yany79 changed the base branch from main to beta August 5, 2026 15:36
@Besty0728

Copy link
Copy Markdown
Owner

I'll review your PR when I have some time.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants