Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@zjmletang Please review. Is this ok for you? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the createprojectpackage function to provide more detailed output, especially for JSON consumers. It introduces a mechanism to collect action messages and an error status. The changes are generally good, but I have a few suggestions to improve maintainability and robustness:
- There is an unused variable that should be removed.
- A file path has a hardcoded processor architecture which could be problematic.
- There is significant code duplication and some inconsistencies in logging within the new logic, which could be refactored.
- There's a minor code style inconsistency in one of the new blocks.
Please see my detailed comments for specifics.
tools/toolsHCK.ps1
Outdated
| Get-ChildItem -Path $driver -Filter *.pdb -Recurse | ForEach-Object { Move-Item -Path $_.FullName -Destination $symbolPath -Force } | ||
|
|
||
| # Remove driver signatures before packaging to avoid embedded company signatures | ||
| $driverunsigned = $true |
There was a problem hiding this comment.
The variable $driverunsigned is assigned the value $true but is never used afterwards. It should be removed to improve code clarity.
tools/toolsHCK.ps1
Outdated
| $actionMessages += "Removing signature from file '$($_.FullName)' before packaging" | ||
| } | ||
|
|
||
| $process = Start-Process -Wait -FilePath "$env:WTTSTDIO\..\Tests\amd64\Signtool.exe" -ArgumentList 'remove', '/s', $_.FullName -PassThru |
There was a problem hiding this comment.
The path to Signtool.exe includes a hardcoded amd64 architecture. This might make the script less portable or fail on systems with different architectures. It would be more robust to determine the architecture dynamically if possible.
| if (-Not $json) { | ||
| if ($AddDriverResult) { | ||
| Write-Output "Driver added to package from $driver" | ||
| } else { | ||
| $iserror = $true | ||
| Write-Output "Warning: Driver signability check did not pass" | ||
| foreach ($err in $ErrorMessages) { Write-Output " Error: $err" } | ||
| foreach ($warn in $WarningMessages) { Write-Output " Warning: $warn" } | ||
| } | ||
| } else { | ||
| if ($AddDriverResult) { | ||
| $actionMessages += "Driver added to package from $driver" | ||
| } else { | ||
| $iserror = $true | ||
| $actionMessages += "Driver signability check did not pass" | ||
| foreach ($err in $ErrorMessages) { $actionMessages += "Error: $err" } | ||
| foreach ($warn in $WarningMessages) { $actionMessages += "Warning: $warn" } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for handling the result of AddDriver has significant duplication between the if (-Not $json) and else branches. This reduces maintainability.
Additionally, there are inconsistencies in the messages being logged:
- When
$AddDriverResultis false,Write-Outputprefixes the message with "Warning:", but the message added to$actionMessagesdoes not. - The error and warning messages from
$ErrorMessagesand$WarningMessagesare prefixed with two spaces (Write-Output, but not for$actionMessages.
Consider refactoring this to a single logic path that prepares the messages, and then either prints them or adds them to the array based on the $json flag. This would improve consistency and make the code easier to maintain.
There was a problem hiding this comment.
Pull request overview
This pull request enhances the createprojectpackage function in the toolsHCK PowerShell script to support error tracking and detailed messaging in JSON output mode. The main purpose is to provide better visibility into package creation operations, particularly around driver signature removal and other actions.
Changes:
- Extended
New-ProjectPackagefunction to includeiserrorflag andactionMessagesarray for tracking errors and operation messages - Added automatic signature removal from driver files (.sys, .dll, .exe) before packaging to avoid embedded company signatures
- Enhanced JSON output mode to capture and return detailed action messages and error status
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tools/toolsHCK.ps1
Outdated
| $actionMessages += "Driver signability check did not pass" | ||
| foreach ($err in $ErrorMessages) { $actionMessages += "Error: $err" } | ||
| foreach ($warn in $WarningMessages) { $actionMessages += "Warning: $warn" } |
There was a problem hiding this comment.
There is an inconsistency in message formatting between JSON and non-JSON modes. In non-JSON mode (line 2219), the message includes the "Warning:" prefix, but in JSON mode (line 2221), this prefix is omitted. Similarly, lines 2234-2235 include indentation (" Error:", " Warning:") which is absent from the JSON mode version (lines 2243-2244). Consider keeping the same message format for both modes to ensure consistency, or ensure that the JSON consumer is aware that message prefixes and formatting may be missing.
| $actionMessages += "Driver signability check did not pass" | |
| foreach ($err in $ErrorMessages) { $actionMessages += "Error: $err" } | |
| foreach ($warn in $WarningMessages) { $actionMessages += "Warning: $warn" } | |
| $actionMessages += "Warning: Driver signability check did not pass" | |
| foreach ($err in $ErrorMessages) { $actionMessages += " Error: $err" } | |
| foreach ($warn in $WarningMessages) { $actionMessages += " Warning: $warn" } |
tools/toolsHCK.ps1
Outdated
| # Remove driver signatures before packaging to avoid embedded company signatures | ||
| $driverunsigned = $true | ||
| Get-ChildItem -Path $driver -Include *.sys, *.dll, *.exe -Recurse | ForEach-Object { | ||
| if (-Not $json) { | ||
| Write-Output "Removing signature from file '$($_.FullName)' before packaging" | ||
| } else { | ||
| $actionMessages += "Removing signature from file '$($_.FullName)' before packaging" | ||
| } | ||
|
|
||
| $process = Start-Process -Wait -FilePath "$env:WTTSTDIO\..\Tests\amd64\Signtool.exe" -ArgumentList 'remove', '/s', $_.FullName -PassThru | ||
| if ($process.ExitCode -ne 0) { | ||
| $iserror = $true | ||
| if (-Not $json) { | ||
| Write-Output "Warning: Failed to remove signature from file '$($_.FullName)'. Signtool exit code: $($process.ExitCode)" | ||
| } else { | ||
| $actionMessages += "Failed to remove signature from file '$($_.FullName)'. Signtool exit code: $($process.ExitCode)" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The signature removal operation modifies driver files in place at the path provided by the user. This could permanently alter the original driver files. Consider adding a warning in the function documentation or comments about this destructive operation, or alternatively, copy the driver files to a temporary location before removing signatures to preserve the originals.
tools/toolsHCK.ps1
Outdated
| Get-ChildItem -Path $driver -Filter *.pdb -Recurse | ForEach-Object { Move-Item -Path $_.FullName -Destination $symbolPath -Force } | ||
|
|
||
| # Remove driver signatures before packaging to avoid embedded company signatures | ||
| $driverunsigned = $true |
There was a problem hiding this comment.
The variable $driverunsigned is assigned but never used anywhere in the function. Consider removing this unused variable declaration.
| $driverunsigned = $true |
tools/toolsHCK.ps1
Outdated
| if (-Not $json) | ||
| { |
There was a problem hiding this comment.
The opening brace placement is inconsistent with the rest of the codebase. Throughout this file, the pattern is if (condition) { with the opening brace on the same line. This should be changed to if (-Not $json) { on line 2268 with the opening brace on the same line, removing the standalone brace on line 2269.
| if (-Not $json) | |
| { | |
| if (-Not $json) { |
tools/toolsHCK.ps1
Outdated
| $actionMessages += "Removing signature from file '$($_.FullName)' before packaging" | ||
| } | ||
|
|
||
| $process = Start-Process -Wait -FilePath "$env:WTTSTDIO\..\Tests\amd64\Signtool.exe" -ArgumentList 'remove', '/s', $_.FullName -PassThru |
There was a problem hiding this comment.
The code assumes Signtool.exe exists at the path "$env:WTTSTDIO..\Tests\amd64\Signtool.exe" without validating this first. If the tool is missing or the path is incorrect, Start-Process will fail with an error that may not be handled gracefully. Consider adding a Test-Path check before attempting to use Signtool, or wrap the Start-Process call in a try-catch block to handle missing executable scenarios more gracefully.
kostyanf14
force-pushed
the
package-no-sign
branch
from
eb21aa1 to
7c57806
Compare
Hi @kostyanf14 ,thanks for this improvement! Right now the code unconditionally removes all Authenticode signatures from .sys/.dll/.exe files before packaging. For some workflows this makes sense, but for others it can actually break the expected signing flow. Example – commercial vendor scenario (e.g. Alibaba‑like environment): By contrast, in some open‑source or internal test scenarios, drivers may be signed with a test certificate or self‑signed cert just to load on test machines. In those cases, removing the test signature before packaging can indeed be useful. Suggestion: make signature removal opt‑in |
|
@zjmletang Thanks for the updates. This is different from how RedHat sign drivers. We remove vendor EV signature and keep MSFT WHQL signature only. Based on your comment, I will make this part optional. |
- Extend New-ProjectPackage with iserror and messages properties - Collect action messages and set iserror when createprojectpackage runs with -json - Include driver add result, supplemental files, and playlist unload in messages - Lets JSON consumers get operation result without parsing stdout Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
…SON output Made-with: Cursor Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Made-with: Cursor Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
kostyanf14
force-pushed
the
package-no-sign
branch
from
7c57806 to
d63ce33
Compare
|
@zjmletang feel free to test and review |
|
@kostyanf14 The code looks fine to me. I'll test it on Monday and let you know the results |
|
@zjmletang we want to see your results independent of merging |
No description provided.