This repository was archived by the owner on May 24, 2026. It is now read-only.
fix: address review findings round 1 (PR #786)#796
Closed
github-actions[bot] wants to merge 2 commits into
Closed
Conversation
Three follow-up items from the PR #377 consensus review: 1. Save() now returns bool (true on success, false on failure). The Keychain cleanup in RecoverSecretsFromSecureStorage is gated on Save()'s return value instead of File.Exists(SettingsPath), which was unsafe: a prior settings file could exist even if the current Save() failed (e.g., disk full), causing Keychain entries to be deleted without the recovered values persisted. 2. The outer catch {} in RecoverSecretsFromSecureStorage now logs to Debug.WriteLine and appends to ~/.polypilot/crash.log, matching the existing crash-log convention. 3. ReadSecureStorage gets a doc comment explaining the intentional sync-over-async Task.Run pattern (one-time migration, not hot path). Unit tests added for Save() return value behavior. Integration test added for Settings page navigation. Fixes #379 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>
- Fix CSS selectors in SettingsPersistenceTests (#settings-page -> .settings-page) - Add missing assertion for hasModeSection in SettingsPage_ShowsConnectionMode - Add diagnostic logging when Save() fails in RecoverSecretsFromSecureStorage - Fix tautological assertion and use chmod for real POSIX permission enforcement in Save_ReturnsFalse_WhenPathIsReadOnly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
|
Stale fix-round PR — fixes were pushed to the main PR branch. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Addresses expert review findings from PR #786 round 1.
Findings Fixed (4)
SettingsPersistenceTests.cs:27,41#settings-page→.settings-page(ID→class selector)SettingsPersistenceTests.cs:44Assert.True(hasModeSection, ...)ConnectionSettings.cs:390elsebranch withDebug.WriteLinewhenSave()failsConnectionSettingsTests.cs:654,666chmod 555instead of no-opFileAttributes.ReadOnly;Record.Exception+Assert.Nullinstead of tautologicalresult || !resultFindings Skipped (2)
ConnectionSettings.cs:403!cosmetic; protected by nestedcatch {}Settings.razor:~904SaveSettingsQuietly()outside diff; harmless defensive codingTests
All 3,579 unit tests passed ✅
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.