Phases 2-5: application shell, safety, network profiles and distribution - #2
Merged
Merged
Conversation
Turns gui-framework.ps1 from an unused demo into the real shell, replacing the two bespoke WinForms UIs. The shell owns presentation only; every action comes from PCTools and runs on a background runspace, so the window stays responsive during a DISM run that the original tools froze for the duration. Pages: Maintenance (profile picker, preview, results grid), Network (verdict banner, adapter list, fixes), Preferences (reads real state, applies in both directions), Log. Structural problems fixed relative to the original framework: - Invoke-UI binds its arguments into a closure instead of relying on scope capture. A block queued with BeginInvoke runs after its creating scope is gone, and the captured locals are unresolvable by then - the callback fails outright rather than doing anything. The original only worked because its task pump was a WinForms Timer already on the UI thread, so InvokeRequired was false and the block ran synchronously; anything that completed a task off the UI thread would have broken it. - Every control key is declared up front. Under Set-StrictMode -Version Latest, reading a not-yet-assigned hashtable key is a terminating error, so a guard such as `if ($sync.Controls.StatusLabel)` threw rather than returning false. - Nav click handlers no longer wrap $this in GetNewClosure(), which would capture $null and shadow the sender WinForms supplies at click time. - The task list is a synchronized ArrayList, matching the rest of $sync. - Per-monitor DPI awareness, so layouts stop blurring and clipping at 150% scaling. - Docked TableLayoutPanel throughout, replacing absolute pixel coordinates and the disabled maximize button. - Closing while a task runs prompts instead of silently orphaning it, and the runspace pool is disposed on exit. FreedDisplay became a plain property rather than a ScriptProperty: these objects cross a runspace boundary, and a lazily evaluated property would reach back into a runspace that has since returned to the pool. Format-PCByteSize is now public. The shell needs to format a total across several results, and reaching into the module's private scope for that is not an interface. Tests: static analysis of the shell over its AST - control keys declared, no $this inside GetNewClosure, every PCTools command it calls actually exported, no long-running action invoked from a click handler, argument binding, pool disposal, and the Winsock confirmation prompt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ugbD2a2kVkzXGagAw9zzf
The preflight, restore-point gating and result summary landed with the module and shell. This is the distribution half. - Release workflow: builds from a v* tag, refuses to publish if the tag and PCTools.psd1 ModuleVersion disagree, runs the full suite first, stages artifacts, and publishes them with SHA256SUMS. Authenticode signing runs when a SIGNING_CERTIFICATE secret is configured, and the checksums are regenerated afterwards - signing rewrites the files, so checksums taken before it would not match what a user downloads. - Checksum generation split out of the Release task for that reason. - pc-tools.ps1: a real entry point, with -NoGui to load the module into a console session. It fails with an explanatory message when piped through iex, because the GUI needs the module beside it. - Release archives ship a layout (pc-tools.ps1 + src/) rather than loose scripts, alongside the module on its own and the legacy single-file tools so existing README URLs keep working. - README rewritten: verified download first, with a hash check that runs before anything executes. The irm | iex commands are kept, pinned to a tag, and documented with their trade-off rather than recommended - main is whatever was pushed to it last, run as Administrator. - MIGRATION.md maps every old function to its replacement, and lists the behaviour changes worth knowing. ROADMAP.md corrected. The async callback problem I described in the original plan was wrong in both mechanism and severity: the queued block does not see stale values, it loses $Task entirely and fails outright, and it is latent rather than live because the task pump is a WinForms Timer already on the UI thread. The corrected entry records the StrictMode hashtable-key issue found during the port as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ugbD2a2kVkzXGagAw9zzf
The features from the original README roadmap, cheap now that actions and profiles are data rather than click handlers. Network profiles (the Home/Work presets): - Export-PCNetworkProfile captures an adapter's full configuration - DHCP or static, address, prefix, gateway, DNS - as JSON. - Import-PCNetworkProfile applies it. The adapter is a parameter rather than being read from the profile, so a configuration captured on one machine can be applied to a differently named adapter on another. - Get-PCNetworkProfile lists what has been saved. JSON configuration: - Import-PCConfiguration loads user-defined maintenance profiles. They appear in Get-PCMaintenanceProfile and run through Invoke-PCMaintenance with no further plumbing, because a profile was already just data. One named the same as a built-in overrides it; re-importing replaces rather than accumulates. - Every action name is validated at import against the module's exported commands, so a typo fails there instead of partway through a run. New diagnostics: - Test-PCRoute: per-hop traceroute over Ping with an increasing TTL rather than by shelling out to tracert, so hops honour the timeout and come back as objects. Answers where connectivity stops, not just that it has. - Test-PCMtu: binary-searches the path MTU with the don't-fragment flag. This is the diagnostic for the confusing case where DNS resolves and small requests succeed but large transfers and TLS handshakes stall. - Get-PCWirelessStatus: parses and grades the Wi-Fi association. Net Diag captured the same netsh output as raw text in its full report. All five are wired into the shell's Network page, plus save/apply adapter profile. Every field read from user-supplied JSON is now probed before it is read. Under Set-StrictMode -Version Latest an absent property is a terminating error, so a config file missing "name" or "actions" produced a PowerShell property error instead of the message explaining what was wrong with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ugbD2a2kVkzXGagAw9zzf
- Release workflow publishes the module to the PowerShell Gallery on a tagged build, gated on a PSGALLERY_API_KEY secret so a release without one still ships as an archive. It refuses to republish an existing version rather than letting the Gallery reject it with a less useful error - Gallery versions are immutable. - Manifest carries the metadata the Gallery surfaces: CompatiblePSEditions, edition tags, inline release notes. - README documents Install-Module as the least-ceremony path, and says plainly why it is better than irm | iex: signed-package verification, versioning and Update-Module. On the roadmap's "portable executable": ps2exe output is unsigned and reliably trips SmartScreen and antivirus heuristics, which is worse for trust than the current script. Recorded as such rather than built. winget is deferred until releases are actually signed. Verified the release pipeline end to end: staged artifacts, expanded the archive, confirmed the layout is complete, ran the extracted entry point, and checked the archive against its published SHA256SUMS line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ugbD2a2kVkzXGagAw9zzf
The first CI run reported 1114 warnings, which is the same as reporting
none: nobody reads a thousand findings, and a real one cannot be seen
among them.
- Excluded the rules producing false positives by construction.
PSReviewUnusedParameter was the worst: nearly every public action wraps
its work in Invoke-PCAction -Body { ... }, and the analyzer does not
follow parameter use into that scriptblock, so it flagged parameters
used two lines further down.
- Dropped the purely cosmetic formatting rules. They accounted for most of
the volume, against a style the existing scripts already used. Worth a
deliberate one-off reformat someday; not worth a gate that hides real
findings today.
- Kept PSUseCompatibleSyntax, which catches syntax valid in PowerShell 7
but not in 5.1 - the runtime the GUI actually requires.
- Analyze now summarises findings by rule before listing them, because the
useful question is always which rule and how often.
- Added a warning budget. Errors always fail the build; warnings fail once
they exceed what the repository has agreed to carry, so the count can
only go down. Set provisionally pending the real number from CI.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ugbD2a2kVkzXGagAw9zzf
The retuned analyzer dropped from 1114 warnings to 147, which made the findings readable - and one of them was real. The FormClosing handler declared param($eventSender, $eventArgs), and $eventArgs is a PowerShell automatic variable; shadowing it in a param block is a bug waiting for whoever edits that handler next. Renamed to $closingSender/$closingArgs. Warning budget set to the measured 146. Errors always fail the build; warnings now fail once they exceed what the repository has agreed to carry, so the count can only go down. Most of the remainder is PSUseShouldProcessForStateChangingFunctions firing on the shell's internal UI helpers, where the rule does not apply but is worth keeping enabled for the module's actual cmdlets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ugbD2a2kVkzXGagAw9zzf
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfa2ea1990
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+106
to
+110
| - name: Publish release | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| files: | | ||
| out/* |
There was a problem hiding this comment.
When this workflow is run through workflow_dispatch, GITHUB_REF is the selected branch rather than v<version>, and the resolved version is used only for artifact names and the manifest check. Because the release action receives no tag_name, a manual run cannot publish the requested version tag (and may attempt to release the branch ref instead), leaving the documented pinned vX.Y.Z install path without a corresponding release. Pass v${{ steps.version.outputs.version }} as tag_name for this action.
Useful? React with 👍 / 👎.
Comment on lines
+75
to
+77
| Get-ChildItem ./out -Include *.ps1, *.psm1, *.psd1 -Recurse | ForEach-Object { | ||
| Set-AuthenticodeSignature -FilePath $_.FullName -Certificate $cert ` | ||
| -TimestampServer 'http://timestamp.digicert.com' -HashAlgorithm SHA256 |
There was a problem hiding this comment.
When a signing certificate is configured, this only finds loose script files under out; the actual pc-tools-<version>.zip and PCTools-<version>.zip were already compressed by the build and are never unpacked or signed. Consequently, users of the primary archive still receive unsigned pc-tools.ps1, .psm1, and .psd1 files despite the signing-enabled release path. Sign the staging tree before compression, or repackage the archives after signing.
Useful? React with 👍 / 👎.
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
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.
Continues the roadmap merged in #1, which covered Phase 0 (repo foundation) and Phase 1 (the
PCToolsmodule). This is Phases 2 through 5, rebased onto the currentmain.What this adds
Phase 2 - the application shell.
gui-framework.ps1was the best-architected file in the repo and the only one nothing used. It is now the real shell (src/Shell/), replacing both bespoke WinForms UIs. It owns presentation only; every action comes fromPCToolsand runs on a background runspace, so the window stays responsive during a DISM run that the original tools froze for the duration. Pages: Maintenance, Network, Preferences, Log.Phase 3 - safety and distribution. Release workflow builds from a
v*tag, refuses to publish when the tag andModuleVersiondisagree, runs the full suite first, and publishes checksummed artifacts. Authenticode signing runs when a certificate secret is configured, with checksums regenerated afterwards — signing rewrites the files, so checksums taken before it would not match what a user downloads. README rewritten around a verified download; theirm | iexcommands are kept, pinned to a tag, and documented with their trade-off rather than recommended.Phase 4 - the features from the original README roadmap. Network profiles (
Export-/Import-/Get-PCNetworkProfile), user-defined maintenance profiles from JSON (Import-PCConfiguration), and three new diagnostics:Test-PCRoute,Test-PCMtu,Get-PCWirelessStatus. These were cheap because profiles and actions were already data rather than click handlers.Phase 5 - PowerShell Gallery. Publishing wired into the release workflow, gated on an API key and refusing to republish an existing version.
PCToolsis now 0.4.0 with 36 public functions.MIGRATION.mdmaps every old function to its replacement.Verification
CI is green on the head commit (run 33048647036):
The release pipeline was also exercised end to end: artifacts staged, archive expanded, layout confirmed complete, the extracted entry point run, and the archive checked against its published
SHA256SUMSline.Bugs found and fixed along the way
Several were caught by tests written for other reasons:
Set-StrictMode -Version Latest, reading a not-yet-assigned key is a terminating error, so a guard likeif ($sync.Controls.StatusLabel)throws instead of returning false. This bit in the shell and twice more in user-JSON parsing, where a config file missingnameoractionsproduced a PowerShell property error instead of the message explaining what was wrong with it.$eventArgsis a PowerShell automatic variable, and theFormClosinghandler shadowed it in itsparamblock.ConvertTo-PCPrefixLength '0.0.0.0'threw on a null pipeline under StrictMode.PSReviewUnusedParameterwas the worst, since it does not follow parameter use into anInvoke-PCAction -Body { ... }scriptblock. Added a warning budget so the count can only go down.A correction to the roadmap in #1
The async-callback bug described in
ROADMAP.mdwas wrong in both mechanism and severity, and I have corrected it in this branch. Tested rather than assumed: the queued block does not see stale values —$Taskis gone entirely and the callback fails outright. It is also latent, not live, because the task pump is a WinFormsTimeralready on the UI thread, soInvokeRequiredis false and the block runs synchronously while the scope still exists. It only breaks if anything ever completes a task off the UI thread. The shell binds arguments withGetNewClosure(), which sidesteps the issue and does not depend on how WinForms marshals a delegate's parameter array.Note on review
The GUI cannot be exercised headlessly, so the shell is covered by static analysis over its AST instead: control keys declared, no
$thiscaptured insideGetNewClosure, everyPCToolscommand it calls actually exported, and no long-running action invoked from a click handler. Someone should still run.\pc-tools.ps1on a real desktop before tagging a release — that is the one thing CI here cannot tell you.Generated by Claude Code