Skip to content

Add Phase 2 package policy editor - #5335

Open
Benoît Cortier (CBenoit) wants to merge 25 commits into
cbenoit-plan-policy-inspector-uifrom
cbenoit-phase-2-policy-editor
Open

Add Phase 2 package policy editor#5335
Benoît Cortier (CBenoit) wants to merge 25 commits into
cbenoit-plan-policy-inspector-uifrom
cbenoit-phase-2-policy-editor

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Aug 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Add a Windows-only Agent policy management dashboard and full structured/raw policy editor while preserving the Phase 1 read-only inspector.
  • Add authoritative Agent validation, warning acknowledgement, exact-token conflict handling, in-memory dirty-state protection, and accessibility/localization coverage.
  • Add a separately built NativeAOT elevated helper with authenticated, bounded, single-use named-pipe IPC; only the helper performs ReplacePolicy.
  • Wire the helper into Windows build, signing, integrity, installer, and uninstall cleanup paths.

Stack and dependencies

Validation

Validated at head a15e9c703a3abaf6a181543156e39905bc1453dd:

  • NuGet.org-only restore with <clear/> and no unpublished/local feed.
  • Focused policy-editor/restart tests: 265 passed; privileged dispatcher tests: 8 portable and 8 Windows passed.
  • Full Windows solution tests passed, including Avalonia 311, PackageEngine 501 portable / 735 Windows, and all supporting test projects.
  • Windows x64 solution build and forced portable net10.0 Avalonia build succeeded with zero errors.
  • Fresh win-x64 NativeAOT publish produced metadata-free AMD64 PE executables with no CLR header and no CoreCLR/hostfxr/hostpolicy files:
    • UniGetUI.exe: 62,453,248 bytes, SHA-256 63AEF015EE02FAAD7A11DBBD806634418C39CABBC23B5A4C0E3F5328F38F7C46
    • UniGetUI.PolicyElevator.exe: 5,870,592 bytes, SHA-256 EB64BB65DE56B2DACA8158B83E8EE5A17ED5A9F13DBFC1A82841C49390BCCA7F
  • Integrity tree validated 79 files.
  • Translation placeholders validated across 59 catalogs; source extraction found 1,097 keys, zero missing keys, and zero warnings. The check-only source-sync command retains the known repository baseline of 190 dynamic/legacy keys because no legacy boundary is configured.
  • Changed-file whitespace formatting and git diff --check pass; the worktree is clean.

Review state

This PR remains draft pending exact-head CI and the required repeated Copilot review loop. It must not be marked ready until Copilot reports an explicit no-findings approval and all review threads are resolved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate correctness, performance, contract-boundary, and post-commit protocol issues remain unresolved.

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

Pull request overview

Adds a Windows-only Agent policy editor with structured/raw editing, validation, stale-token handling, and secure elevated writes.

Changes:

  • Adds policy-management UI, lifecycle guards, localization, and search integration.
  • Introduces authenticated NativeAOT elevation and bounded IPC.
  • Expands packaging, build automation, and test coverage.
File summaries
File Description
UniGetUI.iss Handles helper installation cleanup.
src/UniGetUI.Windows.slnx Adds the elevated-helper project.
src/UniGetUI.Tests/SettingsSearchIndexTests.cs Tests policy search indexing.
src/UniGetUI.Tests/PolicyEditor/PolicyRuleOperationsTests.cs Tests rule mutations.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorViewModelFakes.cs Provides editor test doubles.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorTestFixtures.cs Provides policy test fixtures.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorTemplatesTests.cs Tests draft templates.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorSessionCloseGuardTests.cs Tests dirty-session protection.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorRetryResolverTests.cs Tests conflict retry decisions.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorLocalizationTests.cs Tests localization-key coverage.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorFindingIndexTests.cs Tests bounded finding indexing.
src/UniGetUI.Tests/PolicyEditor/PolicyEditorDraftFingerprintTests.cs Tests canonical fingerprints.
src/UniGetUI.Tests/AgentPolicyInspectorViewModelTests.cs Tests policy-management states.
src/UniGetUI.PackageEngine.Tests/PolicyWriteElevation/PolicyElevationProtocolTests.cs Tests elevation protocol constants.
src/UniGetUI.PackageEngine.Tests/PolicyWriteElevation/PolicyElevationPathsTests.cs Tests authenticated paths.
src/UniGetUI.PackageEngine.Tests/PolicyWriteElevation/PolicyElevationLaunchArgumentsTests.cs Tests launch-argument validation.
src/UniGetUI.PackageEngine.Tests/PolicyWriteElevation/PolicyElevationHelperLocatorTests.cs Tests helper discovery.
src/UniGetUI.PackageEngine.Tests/PolicyWriteElevation/PolicyElevationFrameTests.cs Tests bounded framing.
src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs Updates broker-operation regressions.
src/UniGetUI.PackageEngine.Tests/BrokerPolicyInspectorTests.cs Tests policy validation.
src/UniGetUI.PackageEngine.AgentBroker/UniGetUI.PackageEngine.AgentBroker.csproj Updates policy dependencies.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Protocol/PolicyElevationPaths.cs Defines protocol paths.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Protocol/PolicyElevationMessages.cs Defines IPC messages.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Protocol/PolicyElevationJsonContext.cs Supplies generated JSON metadata.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Protocol/PolicyElevationEnumConverters.cs Serializes protocol enums.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Protocol/PolicyElevationArguments.cs Parses helper arguments.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/PolicyElevationPipeServer.cs Creates secured IPC pipes.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/PolicyElevationOutcome.cs Models elevation outcomes.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/PolicyElevationHelperLocator.cs Locates the packaged helper.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Interop/WindowsProcessInspector.cs Inspects process identity.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Interop/WindowsPeerAuthenticator.cs Authenticates IPC peers.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Interop/WindowsAuthenticodeTrustVerifier.cs Verifies executable signatures.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Interop/PolicyElevationTrustPolicy.cs Defines signer trust decisions.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/Interop/PolicyElevationAccessPolicy.cs Defines elevation access checks.
src/UniGetUI.PackageEngine.AgentBroker/PolicyManagement/BrokerPolicyManagementModels.cs Defines management outcomes and limits.
src/UniGetUI.PackageEngine.AgentBroker/BrokerPolicyInspector.cs Strengthens policy validation.
src/UniGetUI.Avalonia/Views/SoftwarePages/Interfaces/PageInterfaces.cs Defines asynchronous leave guards.
src/UniGetUI.Avalonia/Views/Pages/SettingsPages/SettingsBasePage.axaml.cs Guards settings navigation.
src/UniGetUI.Avalonia/Views/Pages/SettingsPages/PolicyEditor/PolicyEditorDialog.axaml.cs Handles editor dialog interactions.
src/UniGetUI.Avalonia/Views/Pages/SettingsPages/PolicyEditor/PolicyEditorConfirmationPrompt.cs Presents save confirmations.
src/UniGetUI.Avalonia/Views/Pages/SettingsPages/AgentPolicyInspector.axaml.cs Opens editor dialogs.
src/UniGetUI.Avalonia/Views/Pages/SettingsPages/AgentPolicyInspector.axaml Adds policy-management controls.
src/UniGetUI.Avalonia/Views/MainWindow.axaml.cs Guards window closure.
src/UniGetUI.Avalonia/Views/Controls/PolicyJsonEditor.cs Provides the raw JSON editor.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyRuleOperations.cs Implements rule-list mutations.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorTemplates.cs Creates operation-specific drafts.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorSessionCloseGuard.cs Protects unsaved changes.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorRetry.cs Resolves stale-token retries.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorRawSyntax.cs Implements strict raw parsing.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorProductionAdapters.cs Connects UI services to Agent APIs.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorPolicyContract.cs Centralizes policy contract values.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorEnums.cs Defines editor states and operations.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDependencies.cs Defines editor service seams.
src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyDraftFingerprint.cs Computes canonical draft fingerprints.
src/UniGetUI.Avalonia/UniGetUI.Avalonia.csproj Publishes and stages the helper.
src/UniGetUI.Avalonia/Infrastructure/SettingsSearchIndex.cs Adds policy-management search actions.
src/UniGetUI.AgentPolicy.ElevatedHelper/UniGetUI.AgentPolicy.ElevatedHelper.csproj Configures NativeAOT publication.
src/UniGetUI.AgentPolicy.ElevatedHelper/PolicyReplacementExecutor.cs Executes elevated replacements.
src/UniGetUI.AgentPolicy.ElevatedHelper/app.manifest Requires administrator elevation.
scripts/build.ps1 Builds and stages the helper.
.github/workflows/build-release.yml Signs, verifies, and packages the helper.
Review details

Suppressed comments (8)

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs:182

  • This displays parser-generated English text verbatim. PolicyEditorRawSyntax supplies hard-coded/interpolated messages and raw JsonException.Message values, so non-English users see untranslated error details despite neighboring status text being localized. Represent syntax failures with localizable kinds/arguments and translate a bounded message here.
                syntaxError.Message,

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs:181

  • SyntaxError also represents valid JSON that violates the draft contract (wrong schema, policy type, or fixed precedence), so this title incorrectly tells users those documents are not JSON. Use a title such as “The document is not a valid policy draft,” or distinguish JSON parse errors from contract errors.
                CoreTools.Translate("The document is not valid JSON"),

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorRawSyntax.cs:113

  • The raw document uses the PascalCase Enforcement property, so this case-sensitive JSON Pointer cannot identify the missing block in the editor. Use /Enforcement.
            error = new PolicyEditorSyntaxError("Missing enforcement block.", "/enforcement");

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorRawSyntax.cs:121

  • Canonical raw JSON names this path Enforcement/RulePrecedence. JSON Pointer matching is case-sensitive, so the lowercase pointer cannot locate or highlight the rejected field.
                "/enforcement/rulePrecedence");

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorRawSyntax.cs:127

  • The expected raw property is Metadata; JSON Pointer member names are case-sensitive. Use /Metadata so this error points to the property the editor accepts/emits.
            error = new PolicyEditorSyntaxError("Missing metadata block.", "/metadata");

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorRawSyntax.cs:9

  • Message is populated with hard-coded English strings and raw runtime exception messages, then bound directly to the dialog. Non-English users therefore receive untranslated, runtime-dependent syntax details, and translation-source tooling cannot discover them. Carry a stable syntax-error kind plus bounded arguments/pointer and localize it at the presentation layer with literal translation keys instead of exposing Exception.Message.
public sealed record PolicyEditorSyntaxError(string Message, string Pointer);

src/UniGetUI.Avalonia/Views/Pages/SettingsPages/PolicyEditor/PolicyEditorConfirmationPrompt.cs:113

  • This count is computed from the bounded display list, whose final item is a synthetic warning whenever findings are omitted. It can therefore overcount (omitted errors) or severely undercount (omitted warnings) the warnings being acknowledged. Carry the authoritative warning count separately from the bounded findings and display that value.
    src/UniGetUI.Tests/PolicyEditor/PolicyEditorLocalizationTests.cs:108
  • This regex only discovers literal CoreTools.Translate("...") calls, but the changed editor also translates dynamic enum/severity/argument labels (for example CoreTools.Translate(Severity.ToString()) and CoreTools.Translate(label)). Removing one of those keys from lang_en.json would still leave this test green, so the claimed policy-editor localization coverage is incomplete. Enumerate those bounded dynamic key sets explicitly or refactor them to stable literal translation keys that this check can validate.
  • Files reviewed: 91/91 changed files
  • Comments generated: 4
  • Review effort level: Balanced

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

Implement transactional policy management, authoritative validation, the structured/raw editor, and the authenticated elevated write helper with packaging and test coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Consume the final now-libraries Phase 2 policy artifacts and retain exact version parity between the main broker integration and elevated helper.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Refresh to the authoritative now-libraries Phase 2 packages and migrate PolicyJson and BrokerJson callers to their clarified serializer APIs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Consume the authoritative now-libraries policy artifacts with tightened serialization boundaries and revision validation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Consume the authoritative policy artifacts and keep malformed-wire fixtures compatible with stricter standalone serializer and revision invariants.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address policy editing, validation, elevation protocol, cancellation, and NativeAOT safety findings while retaining authoritative Agent behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical policy-rule behavior and multiple unresolved correctness, accessibility, lifecycle, and dependency issues block approval.

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

Review details
  • Files reviewed: 84/88 changed files
  • Comments generated: 9
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Moderate issues remain in conflict messaging, UI-thread dirty-state computation, and structured description round-tripping.

Review details

Suppressed comments (6)

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

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs:212

  • A stale-token response sets both LastWriteFailureKind and Session.HasConflict, so this earlier branch always wins and the conflict-specific “Review … then choose Overwrite” status below is unreachable. Users see only a generic broker rejection even though the UI is waiting for an exact-token overwrite decision. Prioritize HasConflict before the generic write-failure branch.
    src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorSession.cs:68
  • IsDirty synchronously serializes and compares the entire structured draft on every getter call. Every structured field edit raises IsDirty, so typing can repeatedly serialize a policy up to the 16 MiB request limit on the UI thread; cache/debounce the structured fingerprint or track dirty generations and perform the full comparison off the input path.
    src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorStructuredUi.cs:175
  • This silently converts schema-valid empty or whitespace-only descriptions to null. That changes user-authored policy data when editing in structured mode, so switching modes or saving no longer round-trips the draft exactly; preserve the value and let authoritative validation handle its length constraints.

This issue also appears in the following locations of the same file:

  • line 425
  • line 477
  • line 483

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorStructuredUi.cs:425

  • This silently converts schema-valid empty or whitespace-only rule reasons to null. Structured editing therefore mutates valid policy content instead of preserving the exact reason; assign the nullable value directly and leave validation to the Agent.
    src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorStructuredUi.cs:477
  • Whitespace is nonempty policy text, but this setter silently converts it to an omitted lower bound instead of submitting the exact value for authoritative validation. Only an empty textbox should clear MinVersion.
    src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorStructuredUi.cs:483
  • Like MinVersion, a whitespace-only upper bound is silently rewritten to null. Preserve nonempty input so structured editing does not change the draft before the Agent validates it.
  • Files reviewed: 83/89 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit

Copy link
Copy Markdown
Member Author

Resolved every concern from review #5335 (review) at exact head 02a3e6f4ae5af52196416296e3cfbbfad05085f0:

  1. Conflict status precedence: Session.HasConflict now wins before generic write failures, so exact-token overwrite guidance is reachable. Tests cover both flags set and generic rejection without conflict.
  2. Constant-time structured dirty state: synchronous setters and IsDirty only update/read cached state. Debounced analysis captures a deep snapshot on the UI context, serializes it off-thread, and stale-suppresses by mutation generation plus baseline version. Tests cover dirty, exact revert, rapid stale analyses, live-draft isolation, large drafts, boundary reconciliation, and edit-during-save ownership.
  3. Description presence/text: HasDescription explicitly distinguishes omission from present empty text; null survives initial binding and empty, whitespace, normal text, explicit omission, and raw/structured round-trips are exact.
  4. Rule Reason presence/text: HasReason supplies the same omission versus present-empty semantics, with exact null/empty/whitespace/normal-text and mode-round-trip coverage.
  5. MinVersion: exactly empty clears the optional bound; whitespace and nonempty authored text are preserved for authoritative Agent validation, with exact round-trip tests.
  6. MaxVersion: exactly empty clears the optional bound; whitespace and nonempty authored text are preserved for authoritative Agent validation, with exact round-trip tests.

Additional race hardening rechecks raw-mode edits after an in-flight authenticated save against the new authoritative baseline and clears stale SavedWithNewerChanges when structured or raw exact reconciliation proves the draft clean. Focused policy-editor tests pass 236/236; the full Windows solution tests, Windows x64 and forced-portable builds, fresh app/helper NativeAOT publish, and 79-file integrity verification pass. Official NuGet.org-only policy packages remain 2026.9.3 from release commit 790d7c63.

@CBenoit

Copy link
Copy Markdown
Member Author

Copilot review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Three moderate issues remain in publisher handling, finding severity mapping, and restart cancellation behavior.

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

Review details

Suppressed comments (3)

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

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorTemplates.cs:26

  • Publisher is a schema-valid required string even when it contains only whitespace (the existing preservation regression covers this at AgentPolicyInspectorViewModelTests.cs:39-64). ReplaceIdentity() passes the active policy's publisher into this method, so such a valid policy throws here and the editor never opens. Reject only null/empty publishers, and update the whitespace test expectation accordingly.
    src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyValidationFinding.cs:30
  • Info findings fall through to Warning here. Warning findings require acknowledgement before saving (PolicyEditorSession.cs:689-696), so an informational Agent result is incorrectly shown as a warning and adds a confirmation step. Preserve all three severities explicitly.

This issue also appears on line 39 of the same file.

src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyValidationFinding.cs:41

  • The sanitized-finding path also maps Info to Warning. These findings feed the same warning acknowledgement logic, so informational results from the bounded broker adapter unnecessarily block saving behind a warning confirmation. Preserve the informational severity.
  • Files reviewed: 82/89 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/UniGetUI.Avalonia/Views/MainWindow.axaml.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The initiating-user identity and duplicate discard-prompt issues must be resolved before approval.

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

Review details

Suppressed comments (1)

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

src/UniGetUI.Avalonia/Views/Pages/SettingsPages/AgentPolicyInspector.axaml.cs:110

  • Concurrent window-close and page-leave/shutdown requests can prompt twice for the same dirty editor. _closePromptPending only serializes the dialog’s local path, while this external path calls ConfirmDiscardAsync independently; for example, shutdown can enter here while the dialog’s close confirmation is still open. Coalesce discard confirmation through one session-level in-flight task (used by both paths) so all callers await the same decision.
  • Files reviewed: 89/98 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/UniGetUI.AgentPolicy.ElevatedHelper/PolicyReplacementExecutor.cs Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces a broad, security-sensitive privileged policy-writing path requiring final human review.

Review details
  • Files reviewed: 91/100 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Model BY_HANDLE_FILE_INFORMATION with the exact Win32 FILETIME layout and cover field offsets plus real handle identity stability.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive elevation flow is broad, and a moderate accessibility issue remains unresolved.

Review details

Suppressed comments (1)

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

src/UniGetUI.Avalonia/Views/Pages/SettingsPages/PolicyEditor/PolicyEditorDialog.axaml:91

  • This live region hard-codes every status as Polite, including asynchronous save failures. Unlike validation findings, changes to LastWriteFailureKind/LastErrorCode only refresh this InfoBar, so those errors are never announced assertively. Please make the live setting severity-aware or explicitly announce error statuses as Assertive; this matches the established pattern in MainWindowViewModel.cs:333-344 and AvaloniaOperationRegistry.cs:240-255.
  • Files reviewed: 93/102 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Track authoritative write completions independently from visual status precedence, announce each outcome with severity-aware live settings, and avoid duplicate validation and raw-syntax summaries.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit

Copy link
Copy Markdown
Member Author

Resolved the suppressed accessibility finding from review 5116912308 in commit 23522d6. Policy write completions now carry monotonic outcome identity and are announced independently with assertive error or polite non-error severity, while validation/local/raw detail regions avoid duplicate summaries. The finding had no inline review thread to resolve.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical executable-verification race must be fixed before approval.

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

Review details
  • Files reviewed: 92/102 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Four moderate issues remain in timeout handling, cancellation propagation, accessibility announcements, and UI-thread preflight work.

Review details

Suppressed comments (5)

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

src/UniGetUI.AgentPolicy.ElevatedHelper/Program.cs:43

  • The exchange deadline starts before protected-layout checks, both peer authentications, pipe connection, identity resolution, and request reading. Those stages consume the same two-minute budget intended for the broker exchange, while the host grants a separate connect budget and resets its exchange timeout after authentication. A valid slow broker write can therefore be canceled by the helper first and surface as an unknown result. Use separate stage-scoped cancellation sources, starting the exchange budget only after connection/authentication succeeds.
    src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorSessionCloseGuard.cs:63
  • If the caller cancels while the operation is still running, the delay task completes in the canceled state and this method returns false. The leave guard then reports a five-second timeout instead of propagating the navigation/shutdown cancellation. Distinguish cancellation from an elapsed timeout before returning false.
    src/UniGetUI.Avalonia/Views/Pages/SettingsPages/AgentPolicyInspector.axaml:62
  • This status is already announced explicitly by AnnounceManagementStatus, which selects Assertive for errors. Making the bound InfoBar Polite as well causes duplicate announcements and can announce an error at the wrong priority; leave the visual status out of the live region and keep the centralized severity-aware announcement.

This issue also appears on line 173 of the same file.
src/UniGetUI.PackageEngine.AgentBroker/PolicyWriteElevation/WindowsPolicyWriteElevator.cs:181

  • Verify performs synchronous install-tree/ACL and Authenticode checks before this method reaches an await. Saves originate on the Avalonia command/UI path, so this can freeze the editor while Windows performs disk and certificate work. Please move the preflight to a worker, as PackagedPolicyWriteElevationEligibility.EvaluateAsync already does.

src/UniGetUI.Avalonia/Views/Pages/SettingsPages/AgentPolicyInspector.axaml:173

  • AnnounceStatus already emits this status through the accessibility announcement service and uses Assertive for errors. This additional fixed Polite live region can announce every update twice and downgrade the urgency of errors; rely on the explicit severity-aware announcement only.
  • Files reviewed: 92/102 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit

Copy link
Copy Markdown
Member Author

Resolved suppressed findings from Copilot review 5117833320 in 4c6d2c2.

  • Split the helper's pre-exchange and broker-exchange deadlines, and bounded synchronous protected-layout, peer-authentication, and initiating-user stages with late-work cleanup ownership.
  • Added process-wide single-flight authoritative preflight execution with cancellable queuing, fresh save verification, and deterministic abandoned lease/fault handling.
  • Propagated policy-editor close-guard caller cancellation distinctly from an elapsed internal timeout.
  • Removed duplicate fixed-Polite inspector status live regions in favor of centralized severity-aware announcements.

The review produced only suppressed summary findings and no resolvable inline threads, so this comment records their disposition.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The privileged write path and broad security-sensitive release integration require final human review.

Review details
  • Files reviewed: 94/107 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants