Implementation of covenant-setup, a deterministic Windows installer engine.
Implementation of covenant-setup, a deterministic Windows installer engine.
copilot-pull-request-reviewer[bot]
(Migrated from github.com)
reviewed 2026-04-29 01:36:54 +00:00
copilot-pull-request-reviewer[bot]
(Migrated from github.com)
left a comment
Copy Link
Copy Source
Pull request overview
Implements the MVP for covenant-setup: a deterministic Windows installer/uninstaller engine in Rust with a bundled single-file packager, a C# WinForms UI over named-pipe IPC, and Windows VM harnesses/docs to validate real Win32 boundaries.
The README links use absolute local Windows paths (e.g., C:\Users\...\workspace\...) which will be broken for other contributors and on GitHub. Replace these with repo-relative links (e.g., src/win.rs, src/main.rs, etc.) throughout the README so they render correctly in the repository.
- Uses Win32 APIs through the `windows` crate with unsafe isolated in [`src/win.rs`](src/win.rs)
The README links use absolute local Windows paths (e.g., `C:\Users\...\workspace\...`) which will be broken for other contributors and on GitHub. Replace these with repo-relative links (e.g., `src/win.rs`, `src/main.rs`, etc.) throughout the README so they render correctly in the repository.
```suggestion
- Uses Win32 APIs through the `windows` crate with unsafe isolated in [`src/win.rs`](src/win.rs)
```
The README’s early bullets say the engine is “observing” everything a program does during install and post-install scripts, but later sections (and the codebase) indicate journaling is currently based on declared actions (and script execution is logged, not its internal mutations). Consider rewording these bullets to match current behavior so users don’t overestimate what gets rolled back.
- Recording the install actions it applies (files, directories, registry entries, shortcuts, and script execution) into a `journal.json` written alongside the installed application. This journal is then used during uninstall to reverse those recorded actions and clean up associated state.
- Take a "leave the campground better than you found it" approach - this Eagle Scout practices Leave No Trace.
- Taking a "trust but verify model" to program installs and uninstalls by journaling engine-applied mutations and logging script execution in order to respect the user.
- Using the `journal.json` as a manifest of the actions the installer performed during install and post-install processing.
The README’s early bullets say the engine is “observing” everything a program does during install and post-install scripts, but later sections (and the codebase) indicate journaling is currently based on declared actions (and script execution is logged, not its internal mutations). Consider rewording these bullets to match current behavior so users don’t overestimate what gets rolled back.
```suggestion
- Recording the install actions it applies (files, directories, registry entries, shortcuts, and script execution) into a `journal.json` written alongside the installed application. This journal is then used during uninstall to reverse those recorded actions and clean up associated state.
- Take a "leave the campground better than you found it" approach - this Eagle Scout practices Leave No Trace.
- Taking a "trust but verify model" to program installs and uninstalls by journaling engine-applied mutations and logging script execution in order to respect the user.
- Using the `journal.json` as a manifest of the actions the installer performed during install and post-install processing.
```
* **CLI Framework:** Utilize `clap` for robust argument parsing with standard subcommands (e.g., `covenant-setup install manifest.toml`, `covenant-setup uninstall journal.json`).
* **Structured Output Protocol:** The engine must accept a `--json` flag. When active, all standard text logs, progress percentages, and error stack traces must be suppressed and replaced with single-line serialized JSON objects emitted to `stdout`.
This doc uses glassbox as the example CLI name, but the actual tool/repo is covenant-setup (and the rest of the docs/examples use that). Consider updating the command examples here to match the real binary name to avoid confusion for readers.
This doc uses `glassbox` as the example CLI name, but the actual tool/repo is `covenant-setup` (and the rest of the docs/examples use that). Consider updating the command examples here to match the real binary name to avoid confusion for readers.
is_elevated leaks the process token handle if GetTokenInformation returns an error, because the ? exits before close_handle(token, ...) runs. Consider wrapping HANDLE in a small RAII guard (or using a scope guard) so the handle is always closed on all paths, including error returns.
`is_elevated` leaks the process token handle if `GetTokenInformation` returns an error, because the `?` exits before `close_handle(token, ...)` runs. Consider wrapping `HANDLE` in a small RAII guard (or using a scope guard) so the handle is always closed on all paths, including error returns.
known_folder only calls CoTaskMemFree after pwstr_to_path(raw, ...) succeeds. If UTF-16 decoding fails (or any future error is added before the free), the raw allocation from SHGetKnownFolderPath will be leaked. Consider freeing raw via a guard/defer so it is always released even when pwstr_to_path returns an error.
`known_folder` only calls `CoTaskMemFree` after `pwstr_to_path(raw, ...)` succeeds. If UTF-16 decoding fails (or any future error is added before the free), the `raw` allocation from `SHGetKnownFolderPath` will be leaked. Consider freeing `raw` via a guard/defer so it is always released even when `pwstr_to_path` returns an error.
This test uses a full pipe path (\\.\pipe\foo) as the value following --pipe, but the UI process currently passes that string directly into NamedPipeServerStream, which generally expects a pipe name (e.g., foo) rather than a full path. Consider changing the test input to a plain name, and (if you want to accept full paths) add a dedicated test that verifies the normalization/stripping behavior.
This test uses a full pipe path (`\\.\pipe\foo`) as the value following `--pipe`, but the UI process currently passes that string directly into `NamedPipeServerStream`, which generally expects a *pipe name* (e.g., `foo`) rather than a full path. Consider changing the test input to a plain name, and (if you want to accept full paths) add a dedicated test that verifies the normalization/stripping behavior.
ReadPipeName returns the raw token following --pipe with no validation/normalization. If the caller passes a full named-pipe path like \\.\pipe\foo, NamedPipeServerStream expects just the pipe name and will typically reject names containing path separators. Consider stripping the \\.\pipe\ prefix (or validating and showing a clear error) to make the UI process more robust to how the argument is passed.
`ReadPipeName` returns the raw token following `--pipe` with no validation/normalization. If the caller passes a full named-pipe path like `\\.\pipe\foo`, `NamedPipeServerStream` expects just the pipe name and will typically reject names containing path separators. Consider stripping the `\\.\pipe\` prefix (or validating and showing a clear error) to make the UI process more robust to how the argument is passed.
@copilot+claude-sonnet-4.6 apply changes based on the comments in this thread
@copilot+claude-sonnet-4.6 apply changes based on the comments in [this thread](https://github.com/JMR-dev/covenant-setup/pull/1#pullrequestreview-4193569389)
@copilot+claude-sonnet-4.6 apply changes based on the comments in this thread
Applied all changes from the review thread in commit 74e37e8:
README.md: Reworded the "observing" bullets to accurately describe declared-action journaling (not ETW observation); replaced the broken absolute Windows path with the repo-relative link src/win.rs.
project_mvp.md: Replaced glassbox CLI name with covenant-setup in the example subcommands.
Program.cs ReadPipeName: Now strips the \\.\pipe\ prefix if present, so NamedPipeServerStream always receives a plain pipe name regardless of how the argument is passed.
ProgramTests.cs: Updated the existing test to pass a plain name ("foo"); added a new ReadPipeName_strips_full_pipe_path_prefix test that verifies \\.\pipe\foo is normalized to foo.
src/win.rsis_elevated: Restructured to capture the GetTokenInformation result without ?, always call close_handle, then propagate the error — eliminating the token handle leak on error paths.
src/win.rsknown_folder: Restructured to call pwstr_to_path and store the result, always call CoTaskMemFree, then return the stored result — eliminating the PWSTR memory leak when UTF-16 decoding fails.
> @copilot+claude-sonnet-4.6 apply changes based on the comments in [this thread](https://github.com/JMR-dev/covenant-setup/pull/1#pullrequestreview-4193569389)
Applied all changes from the review thread in commit `74e37e8`:
- **README.md**: Reworded the "observing" bullets to accurately describe declared-action journaling (not ETW observation); replaced the broken absolute Windows path with the repo-relative link `src/win.rs`.
- **project_mvp.md**: Replaced `glassbox` CLI name with `covenant-setup` in the example subcommands.
- **Program.cs `ReadPipeName`**: Now strips the `\\.\pipe\` prefix if present, so `NamedPipeServerStream` always receives a plain pipe name regardless of how the argument is passed.
- **ProgramTests.cs**: Updated the existing test to pass a plain name (`"foo"`); added a new `ReadPipeName_strips_full_pipe_path_prefix` test that verifies `\\.\pipe\foo` is normalized to `foo`.
- **`src/win.rs` `is_elevated`**: Restructured to capture the `GetTokenInformation` result without `?`, always call `close_handle`, then propagate the error — eliminating the token handle leak on error paths.
- **`src/win.rs` `known_folder`**: Restructured to call `pwstr_to_path` and store the result, always call `CoTaskMemFree`, then return the stored result — eliminating the `PWSTR` memory leak when UTF-16 decoding fails.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implementation of covenant-setup, a deterministic Windows installer engine.
Pull request overview
Implements the MVP for
covenant-setup: a deterministic Windows installer/uninstaller engine in Rust with a bundled single-file packager, a C# WinForms UI over named-pipe IPC, and Windows VM harnesses/docs to validate real Win32 boundaries.Changes:
Reviewed changes
Copilot reviewed 31 out of 39 changed files in this pull request and generated 7 comments.
Show a summary per file
--pipe).Systrait boundary abstraction andWinSysproduction implementation.dotnet publish+ embeds application manifest.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@@ -0,0 +1,212 @@# covenant-setupThe README links use absolute local Windows paths (e.g.,
C:\Users\...\workspace\...) which will be broken for other contributors and on GitHub. Replace these with repo-relative links (e.g.,src/win.rs,src/main.rs, etc.) throughout the README so they render correctly in the repository.The README’s early bullets say the engine is “observing” everything a program does during install and post-install scripts, but later sections (and the codebase) indicate journaling is currently based on declared actions (and script execution is logged, not its internal mutations). Consider rewording these bullets to match current behavior so users don’t overestimate what gets rolled back.
@@ -0,0 +9,4 @@### 1. The Rust CLI Interface & IPC Readiness* **CLI Framework:** Utilize `clap` for robust argument parsing with standard subcommands (e.g., `covenant-setup install manifest.toml`, `covenant-setup uninstall journal.json`).* **Structured Output Protocol:** The engine must accept a `--json` flag. When active, all standard text logs, progress percentages, and error stack traces must be suppressed and replaced with single-line serialized JSON objects emitted to `stdout`.This doc uses
glassboxas the example CLI name, but the actual tool/repo iscovenant-setup(and the rest of the docs/examples use that). Consider updating the command examples here to match the real binary name to avoid confusion for readers.@@ -0,0 +1,949 @@use crate::{AppError, Logger, RegistryRoot};is_elevatedleaks the process token handle ifGetTokenInformationreturns an error, because the?exits beforeclose_handle(token, ...)runs. Consider wrappingHANDLEin a small RAII guard (or using a scope guard) so the handle is always closed on all paths, including error returns.known_folderonly callsCoTaskMemFreeafterpwstr_to_path(raw, ...)succeeds. If UTF-16 decoding fails (or any future error is added before the free), therawallocation fromSHGetKnownFolderPathwill be leaked. Consider freeingrawvia a guard/defer so it is always released even whenpwstr_to_pathreturns an error.@@ -0,0 +18,4 @@{var name = Program.ReadPipeName(new[] { "--pipe", @"\\.\pipe\foo" });Assert.Equal("foo", name);}This test uses a full pipe path (
\\.\pipe\foo) as the value following--pipe, but the UI process currently passes that string directly intoNamedPipeServerStream, which generally expects a pipe name (e.g.,foo) rather than a full path. Consider changing the test input to a plain name, and (if you want to accept full paths) add a dedicated test that verifies the normalization/stripping behavior.@@ -0,0 +38,4 @@value = value[pipePrefix.Length..];}return value;}ReadPipeNamereturns the raw token following--pipewith no validation/normalization. If the caller passes a full named-pipe path like\\.\pipe\foo,NamedPipeServerStreamexpects just the pipe name and will typically reject names containing path separators. Consider stripping the\\.\pipe\prefix (or validating and showing a clear error) to make the UI process more robust to how the argument is passed.@copilot+claude-sonnet-4.6 apply changes based on the comments in this thread
Applied all changes from the review thread in commit
74e37e8:src/win.rs.glassboxCLI name withcovenant-setupin the example subcommands.ReadPipeName: Now strips the\\.\pipe\prefix if present, soNamedPipeServerStreamalways receives a plain pipe name regardless of how the argument is passed."foo"); added a newReadPipeName_strips_full_pipe_path_prefixtest that verifies\\.\pipe\foois normalized tofoo.src/win.rsis_elevated: Restructured to capture theGetTokenInformationresult without?, always callclose_handle, then propagate the error — eliminating the token handle leak on error paths.src/win.rsknown_folder: Restructured to callpwstr_to_pathand store the result, always callCoTaskMemFree, then return the stored result — eliminating thePWSTRmemory leak when UTF-16 decoding fails.