feat: expose CLI config options in Settings UI (fixes #698)#806
Conversation
…bleAllHooks) in Settings UI Add an Advanced section to the Settings page with toggles for three Copilot CLI configuration options: - **Compact Paste**: collapse large pasted content to save context tokens - **Respect .gitignore**: exclude gitignored files from working-tree context - **Disable All Hooks**: globally disable all CLI hooks Implementation: - Add CompactPaste, RespectGitignore, DisableAllHooks bool properties to ConnectionSettings with JSON round-trip support - Add SyncCliConfig() method that merges these values into ~/.copilot/config.json (preserving existing keys) - Register three new SettingDescriptors under 'Advanced' category in SettingsRegistry (hidden in Remote/Demo modes) - Add Advanced nav item, section heading, and SettingEditor rendering in Settings.razor - Wire OnSettingChanged to call SyncCliConfig() for advanced.* settings - Add unit tests: defaults, round-trip, backward compat, SyncCliConfig write + merge, registry get/set/visibility/search - Add integration test for Advanced section rendering Fixes #698 Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-Platform Verification — PR #806Build Results
✅ All platforms verified Triggered by: verify-build run |
🧪 Integration Test Report — PR #806
✅ All platforms passed |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review-Fix Loop — Round 1 of 3Findings addressed: 5 of 6
Tests✅ All 3662 tests pass (including new test for parse-failure abort behavior) Next steps
Warning
|
Cross-Platform Verification — PR #806Build Results
✅ All platforms verified Triggered by: verify-build run |
Expert Code Review — PR #806Methodology: 3 independent reviewers with adversarial consensus 5 findings posted as inline comments (4 🟡 moderate, 1 🟢 minor) Findings Summary
Discarded Findings
CI StatusNo CI check runs detected on this PR. Test CoverageThe PR includes good unit test coverage: defaults, JSON round-trip, backward compatibility, merge preservation, registry get/set, visibility by mode, and search keywords. Integration tests exist but lack assertions (Finding #5). No tests cover the startup sync gap (Finding #2) or corrupt file handling (Finding #3).
|
There was a problem hiding this comment.
Review Summary
The PR is well-structured: model properties, registry descriptors, UI wiring, config sync, and tests are all consistent. One moderate UX bug found; the rest is clean.
Findings
🟡 MODERATE — Advanced nav item visible when content section is hidden (Remote/Demo mode on desktop)
File: PolyPilot/Components/Pages/Settings.razor, line 70 (within diff)
The "Advanced" sidebar nav button is guarded only by PlatformHelper.IsDesktop, but the Advanced content section is guarded by advancedSettings.Any() — which returns false in Remote/Demo mode because all three descriptors have IsVisible = ctx => ctx.Settings.Mode != ConnectionMode.Remote && ctx.Settings.Mode != ConnectionMode.Demo. A desktop user in Remote mode sees a clickable "Advanced" nav entry that scrolls to nothing.
Scenario: Desktop user selects Remote mode → sidebar shows "Advanced" → clicking it does nothing (no #settings-advanced element exists to scroll to).
Fix: Add a mode guard to the nav button:
`@if` (PlatformHelper.IsDesktop
&& settings.Mode != ConnectionMode.Remote
&& settings.Mode != ConnectionMode.Demo)
{
<button class="settings-nav-item ..." ...>Advanced</button>
}Or compute advancedSettings earlier (e.g., as a field in RebuildSettingsContext) and use @if (advancedSettings.Any()) around the nav item.
🟢 MINOR — No test-isolation hook for ~/.copilot/config.json path
File: PolyPilot/Models/ConnectionSettings.cs, line ~378 (within diff, SyncCliConfig default path)
The no-arg SyncCliConfig() resolves to the real ~/.copilot/config.json via Environment.GetFolderPath(UserProfile). Unlike SettingsPath (which has SetSettingsFilePathForTesting), there's no override mechanism for the copilot config dir. Current tests pass explicit dirs so this is safe today, but if a future integration test exercises OnSettingChanged("advanced.compactPaste") end-to-end, it would silently mutate the user's real CLI config.
Fix: Add a static override field (matching the _settingsPath pattern):
private static string? _copilotConfigDirOverride;
internal static void SetCopilotConfigDirForTesting(string? dir) => _copilotConfigDirOverride = dir;Then in SyncCliConfig: copilotConfigDir ??= _copilotConfigDirOverride ?? Path.Combine(home, ".copilot");
No regressions, race conditions, data-loss risks, or security issues found. The SyncCliConfig merge logic correctly preserves unrelated keys, the catch {} is appropriate for best-effort config writes, and the OnSettingChanged handler correctly calls both SyncCliConfig() and SaveSettingsQuietly() to keep both config files in sync.
Generated by Expert Code Review · ● 31.7M
Review-Fix Loop — Round 2 of 3Findings addressed: 5 of 5
Additional improvements
Tests✅ All 3666 tests pass (5 new tests added) Next steps
Warning
|
Cross-Platform Verification — PR #806Build Results
✅ All platforms verified Previous Review HistoryFound 2 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
Review-Fix Loop — Round 3 of 3Findings addressed: 6 of 6
Tests✅ All 3667 tests pass (6 new unit tests added) Round statusWarning
|
Cross-Platform Verification — PR #806Build Results
✅ All platforms verified Previous Review HistoryFound 2 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
Summary
Adds an Advanced section to the Settings page with toggles for three Copilot CLI configuration options requested in #698:
Changes
Model (
ConnectionSettings.cs)CompactPaste,RespectGitignore,DisableAllHooksboolean properties (defaultfalse)SyncCliConfig()method that merges these values into~/.copilot/config.json, preserving any existing keys in the fileRegistry (
SettingsRegistry.cs)SettingDescriptorentries under a new Advanced categoryUI (
Settings.razor)SettingEditorOnSettingChangedto callSyncCliConfig()when anyadvanced.*setting changesGroupVisible("advanced")search supportTests
SyncCliConfigwrite + merge preservationTest Results
All 3661 unit tests pass. Integration tests build successfully.
Warning
The following domain was blocked by the firewall during workflow execution:
192.0.2.1To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.