Resolve settings path deterministically and stop reads from clobbering settings.json

[Copilot]

On Linux, a successful reconfigure writes a valid settings.json, yet every subsequent msstore invocation fails with SellerId is not set. and no exception logged — the CLI is looking at (and silently overwriting) a different location than the one it wrote.

Root cause

ConfigurationManager<T> used Environment.GetFolderPath(SpecialFolder.LocalApplicationData) unvalidated:

// LocalApplicationData is "" on Unix when XDG_DATA_HOME, HOME and the passwd entry
// are all unavailable, making the settings path relative to the current directory.
Path.Combine("", "Microsoft", "MSStore.CLI")  // => "Microsoft/MSStore.CLI"

• The resolved path could be relative, so the settings file moved with the current working directory. The same mismatch occurs whenever HOME differs between invocations — e.g. the ephemeral HOME commonly used to unlock a fresh gnome-keyring on headless Linux: reconfigure writes under the ephemeral HOME, every later call reads the real one.
• On a miss, LoadAsync called ClearAsync, writing an empty settings.json as a side effect of a read, and logged nothing — hence a "successful" load with a null SellerId and no exception.
• The probed path was never surfaced on failure: InitAsync bails before info --verbose gets to print Settings File Path.

Changes

• ConfigurationManager
  • GetSettingsDirectory() always returns a rooted path, with a rooted fallback when the OS cannot resolve local application data.
  • New MSSTORE_SETTINGS_DIRECTORY environment variable to pin the settings folder, for containers/CI where local application data is unresolvable or unstable.
  • LoadAsync returns defaults and logs the probed path instead of writing an empty config.
  • SaveAsync ensures the settings directory exists.
• MicrosoftStoreCLI.InitAsync — SellerId is not set. now reports the resolved settings file path and points at reconfigure.
• Tests — ConfigurationManagerUnitTests covers the override, rootedness, no-write-on-missing-file, save/load round-trip, and invalid-config handling with and without clearInvalidConfig.
• README — documents the settings location and the new environment variable.

• Fixes Linux: settings.json (SellerId/TenantId/ClientId) not visible to any subsequent msstore invocation after reconfigure, despite file being correctly written #181

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

A relative MSSTORE_SETTINGS_DIRECTORY can reintroduce CWD-dependent behavior, LoadAsync still has write-side effects that can fail in read-only environments, and the updated critical message can be misleading when a config file exists but is incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The MSSTORE_SETTINGS_DIRECTORY override currently accepts relative values (reintroducing CWD-dependent resolution) and the updated “SellerId is not set” message can be misleading when a config file exists but lacks SellerId.

Review details

Suppressed comments (2)

MSStore.CLI/Services/ConfigurationManager.cs:41

• MSSTORE_SETTINGS_DIRECTORY is documented as requiring an absolute path, but the code currently accepts a relative value and normalizes it with Path.GetFullPath(...), which makes the resolved settings directory depend on the current working directory and can reintroduce the cross-invocation mismatch this PR is fixing. Consider validating that the override is rooted and failing fast (or ignoring it) when it is not.

            var settingsDirectoryOverride = Environment.GetEnvironmentVariable(SettingsDirectoryEnvironmentVariable);
            if (!string.IsNullOrWhiteSpace(settingsDirectoryOverride))
            {
                return Path.GetFullPath(settingsDirectoryOverride);
            }

MSStore.CLI/MicrosoftStoreCLI.cs:122

• This log message now states "No configuration was found" when SellerId is null, but SellerId can also be null when a settings file exists but is missing/cleared/partially-populated. To avoid misleading users, consider wording that reflects what you actually know here (you checked the resolved settings path, and SellerId is not set).

            if (config.SellerId == null)
            {
                logger.LogCritical("SellerId is not set. No configuration was found at '{SettingsPath}'. Please, run the 'reconfigure' command.", configurationManager.ConfigPath);
                return false;

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new MSSTORE_SETTINGS_DIRECTORY override currently allows relative paths, which can reintroduce CWD-dependent (non-deterministic) settings resolution.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

MSStore.CLI/MicrosoftStoreCLI.cs:121

• The critical error message contains a grammatical comma (“Please, run ...”). This reads like a typo and will be user-visible in logs; consider changing it to “Please run ...”.

MSStore.CLI/Services/ConfigurationManager.cs:41

• MSSTORE_SETTINGS_DIRECTORY is documented as requiring an absolute path, but the implementation accepts relative values and normalizes them with Path.GetFullPath(...), which makes the settings directory depend on the current working directory again (reintroducing the original non-determinism). Consider rejecting non-rooted overrides with a clear exception message (or other explicit handling) so the override cannot silently become CWD-relative.

            var settingsDirectoryOverride = Environment.GetEnvironmentVariable(SettingsDirectoryEnvironmentVariable);
            if (!string.IsNullOrWhiteSpace(settingsDirectoryOverride))
            {
                return Path.GetFullPath(settingsDirectoryOverride);
            }

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

LoadAsync currently opens the config file with read/write access (File.Open(..., FileMode.Open) default), which can break read-only loading scenarios and undermines the PR’s goal of making configuration loads non-writing/non-requiring write permissions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

MSStore.CLI/Services/ConfigurationManager.cs:133

• LoadAsync opens the settings file with File.Open(path, FileMode.Open), which defaults to read/write access. This can make a read-only config load fail on read-only filesystems or when the settings file is not writable, contradicting the goal that loading should not require write access.

                }

                using var file = File.Open(_settingsPath, FileMode.Open);

                return await JsonSerializer.DeserializeAsync(file, _jsonTypeInfo, ct) ?? new T();

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

LoadAsync still opens the settings file with read-write access, which can fail on read-only settings files/mounts and undermines the intended “read-only load” behavior.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

MSStore.CLI/MicrosoftStoreCLI.cs:121

• The critical log message has an unnecessary comma after "Please" ("Please, run...") which reads awkwardly in English.
  README.md:13
• In README, "MacOS" should be spelled "macOS" (Apple’s official styling) for consistency with common platform naming.

MSStore.CLI/Services/ConfigurationManager.cs:132

• LoadAsync still opens the settings file with File.Open(path, FileMode.Open), which defaults to FileAccess.ReadWrite and FileShare.None. That can fail when the settings file is readable but not writable (or on read-only mounts), contradicting the goal that loading should not require write access.

                    return new T();
                }

                using var file = File.Open(_settingsPath, FileMode.Open);

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite