fix: preserve Bypass mode after domain reload - #57
Open
yany79 wants to merge 1 commit into
Open
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
|
I'll review your PR when I have some time. |
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.
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_changedevent 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-wideEditorPrefskeys — includingUnitySkills_OperatingMode:The fixtures are careful about this: they snapshot the real values in
[OneTimeSetUp]and put them back in[OneTimeTearDown]. ButResetForTests()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
CurrentModegetter:IsExistingInstall()is true only when one of seven legacyUnitySkills_*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 toAuto— 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 deletesUnitySkills_OperatingMode, so this is the entire surface of the bug — but it is also entirely sufficient to cause it.com.besty.unity-skillsin itstestablesruns 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.
Capture —
ResetForTests()now writes the five preference values intoSessionStatebefore deleting them.SessionStatesurvives domain reloads, which is precisely what the in-memory fixture snapshot does not do. The capture is guarded by anActiveflag so only the firstResetForTests()of a session records values; the subsequent per-test calls do not overwrite the pristine snapshot with the already-wiped state.Restore —
SkillsModeManageris marked[InitializeOnLoad]and its new static constructor callsRestorePreferencesAfterTestDomainReload(). 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.Disarm — each fixture's
[OneTimeTearDown]callsCompleteTestPreferenceRecovery()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 asinternalso 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.GetBoolper domain load.Test coverage
ResetForTests_DomainReloadRecovery_RestoresExplicitBypassModecovers the exact reported scenario: set Bypass explicitly, callResetForTests(), 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
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.Alternatives considered
The deeper fix is for
ResetForTests()never to touch machine-wideEditorPrefsat 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
UnitySkills_OperatingModesurvives domain reloads intact, the new[InitializeOnLoad]static constructor raises no type-initialization error, and skill execution continues to run with Bypass semantics.SkillsModeManager.cschange and theSkillsModeManagerTests.cschange were exercised there; the two other fixture teardowns were not.CI on
betais the authoritative check for this one.