BED-6446: Add winres tool to generate Windows Resources#169
Open
StranDutton wants to merge 6 commits intomainfrom
Open
BED-6446: Add winres tool to generate Windows Resources#169StranDutton wants to merge 6 commits intomainfrom
StranDutton wants to merge 6 commits intomainfrom
Conversation
WalkthroughThe PR introduces Windows resource generation functionality to the project. It adds a new Go CLI utility that generates Windows resource configuration and invokes the go-winres tool, includes new workflow steps to execute this utility during build and publish processes, adds a Company constant, and updates Go dependencies. Changes
Sequence DiagramsequenceDiagram
participant WF as GitHub Workflow
participant CLI as generate-windows-resources CLI
participant FS as File System
participant Tool as go-winres Tool
WF->>CLI: Execute with version argument
CLI->>CLI: Parse and validate product version
CLI->>CLI: Build winres configuration map<br/>(icon paths, metadata, version info)
CLI->>FS: Create winres directory
CLI->>FS: Write winres.json with config
CLI->>Tool: Execute go-winres make command
Tool->>FS: Generate Windows resource files
Tool-->>CLI: Return status
CLI->>WF: Return success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/generate-windows-resources/generate-windows-resources.go (2)
67-76:parseProductVersionreads from the globalos.Argsdirectly.This works, but makes the function harder to test in isolation without mutating global state (which the tests do). Consider accepting
[]stringas a parameter for better testability. That said, it's a small build utility so this is purely a style suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/generate-windows-resources/generate-windows-resources.go` around lines 67 - 76, Change parseProductVersion to accept an argument slice (e.g., args []string) instead of reading os.Args directly so it becomes testable; update its signature (parseProductVersion(args []string) (string, error)), replace uses of os.Args within the function with the provided args slice (use args[0] for the program name and args[1] for the version), and update any call sites (main or tests) to pass os.Args or a custom slice when testing.
111-130: Deferredf.Close()error is silently discarded.On buffered/networked filesystems,
Closecan be the call that actually flushes and reports a write error. SinceEncodesucceeds doesn't guarantee the data hit disk. Consider checking the close error explicitly:Suggested fix
func writeWinresConfig(config map[string]interface{}) error { if err := os.MkdirAll(winresDir, 0755); err != nil { return fmt.Errorf("failed to create winres directory: %w", err) } configPath := filepath.Join(winresDir, winresJSONFile) f, err := os.Create(configPath) if err != nil { return fmt.Errorf("failed to create %s: %w", configPath, err) } - defer f.Close() enc := json.NewEncoder(f) enc.SetIndent("", " ") if err := enc.Encode(config); err != nil { + f.Close() return fmt.Errorf("failed to encode JSON: %w", err) } - return nil + return f.Close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/generate-windows-resources/generate-windows-resources.go` around lines 111 - 130, The deferred file Close in writeWinresConfig currently discards any error from f.Close; capture and handle the Close error so flush/write failures aren't ignored—update writeWinresConfig to check the error returned by f.Close (in the defer or after encoder.Encode) and return or wrap that error (e.g., if enc.Encode succeeded but f.Close returns non-nil, return fmt.Errorf("failed to close %s: %w", configPath, err)). Ensure you reference the same file handle (f) and configPath and propagate the Close error instead of silently dropping it..github/workflows/publish.yml (1)
35-40: Double error suppression:continue-on-error: trueand|| echoare redundant.The
continue-on-error: trueon line 37 already ensures the job won't fail if this step errors. The|| echofallback on line 40 provides a friendlier log message but is technically unnecessary for preventing failure. Not a bug—just noting the overlap. Consider keeping only one mechanism for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 35 - 40, The step named "Generate Windows Resources" uses both continue-on-error: true and a shell fallback "|| echo 'Failed to generate Windows Resources. Skipping...'", which is redundant; edit that step to remove one mechanism (preferably keep continue-on-error: true and delete the "|| echo ..." suffix in the run block) so error suppression is handled in one place and logs remain clean; look for the step name "Generate Windows Resources" and the run command that invokes go run cmd/generate-windows-resources/generate-windows-resources.go to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 54: The go.mod currently pulls golang.org/x/image at an old vulnerable
revision; add an explicit module override to bump it to v0.18.0 or later
(recommended v0.36.0) to mitigate CVE-2024-24792 and ensure the indirect
consumer (go-winres) uses the fixed version; update go.mod by adding a
require/replace for golang.org/x/image with the chosen safe version and then run
go mod tidy to update the dependency graph and lockfile.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 35-40: The step named "Generate Windows Resources" uses both
continue-on-error: true and a shell fallback "|| echo 'Failed to generate
Windows Resources. Skipping...'", which is redundant; edit that step to remove
one mechanism (preferably keep continue-on-error: true and delete the "|| echo
..." suffix in the run block) so error suppression is handled in one place and
logs remain clean; look for the step name "Generate Windows Resources" and the
run command that invokes go run
cmd/generate-windows-resources/generate-windows-resources.go to make the change.
In `@cmd/generate-windows-resources/generate-windows-resources.go`:
- Around line 67-76: Change parseProductVersion to accept an argument slice
(e.g., args []string) instead of reading os.Args directly so it becomes
testable; update its signature (parseProductVersion(args []string) (string,
error)), replace uses of os.Args within the function with the provided args
slice (use args[0] for the program name and args[1] for the version), and update
any call sites (main or tests) to pass os.Args or a custom slice when testing.
- Around line 111-130: The deferred file Close in writeWinresConfig currently
discards any error from f.Close; capture and handle the Close error so
flush/write failures aren't ignored—update writeWinresConfig to check the error
returned by f.Close (in the defer or after encoder.Encode) and return or wrap
that error (e.g., if enc.Encode succeeded but f.Close returns non-nil, return
fmt.Errorf("failed to close %s: %w", configPath, err)). Ensure you reference the
same file handle (f) and configPath and propagate the Close error instead of
silently dropping it.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumwinres/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (6)
.github/workflows/build.yml.github/workflows/publish.ymlcmd/generate-windows-resources/generate-windows-resources.gocmd/generate-windows-resources/generate-windows-resources_test.goconstants/misc.gogo.mod
StranDutton
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
winres/generate-windows-resources/generate-windows-resources.go (2)
99-99: Inject time source for deterministic metadata generation.Using
time.Now()inline makes output time-dependent and can cause edge-time test flakes (e.g., New Year rollover).⏱️ Proposed refactor
const ( @@ fileVersion = "0.0.0.0" // Windows PE file version; we will update 'productVersion' field instead of this one ) + +var now = time.Now @@ - "LegalCopyright": fmt.Sprintf("Copyright (C) %d %s", time.Now().Year(), constants.Company), + "LegalCopyright": fmt.Sprintf("Copyright (C) %d %s", now().Year(), constants.Company),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@winres/generate-windows-resources/generate-windows-resources.go` at line 99, The code uses time.Now() inline when building the "LegalCopyright" string causing non-deterministic output; refactor the function that constructs the metadata (the function that sets "LegalCopyright" in generate-windows-resources.go) to accept an injected time source or explicit year parameter (e.g., a time.Time or int year) instead of calling time.Now() inside the function, and replace fmt.Sprintf("Copyright (C) %d %s", time.Now().Year(), constants.Company) with a format that uses the injected value so tests can pass a fixed year for deterministic metadata generation.
47-47: Decouple argument parsing fromos.Argsand normalize input.
parseProductVersioncurrently depends on process-global state and accepts whitespace-only values. Passing args explicitly and trimming keeps behavior deterministic and test-friendly.♻️ Proposed refactor
import ( "encoding/json" "fmt" "os" "os/exec" "path/filepath" + "strings" "time" @@ func run() error { - productVersion, err := parseProductVersion() + productVersion, err := parseProductVersion(os.Args) if err != nil { return err } @@ -func parseProductVersion() (string, error) { - if len(os.Args) < 2 { - return "", fmt.Errorf("usage: %s <product-version>", filepath.Base(os.Args[0])) +func parseProductVersion(args []string) (string, error) { + if len(args) < 2 { + return "", fmt.Errorf("usage: %s <product-version>", filepath.Base(args[0])) } - version := os.Args[1] + version := strings.TrimSpace(args[1]) if version == "" { return "", fmt.Errorf("product version cannot be empty") } return version, nil }Also applies to: 67-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@winres/generate-windows-resources/generate-windows-resources.go` at line 47, Change parseProductVersion to stop reading os.Args directly: give it an explicit input (e.g., func parseProductVersion(raw string) (string, error) or func parseProductVersion(args []string) ...) and trim the received string (strings.TrimSpace) before validation; if the trimmed value is empty return a descriptive error instead of accepting whitespace-only input. Update the call site that does productVersion, err := parseProductVersion() to pass the appropriate argument (the specific CLI token or os.Args element) and apply the same refactor+trim/validation to the other parsing helpers in this file so all argument parsing is deterministic and testable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@winres/generate-windows-resources/generate-windows-resources.go`:
- Around line 111-127: The writeWinresConfig function currently defers f.Close()
without checking its error; update it to capture and handle Close() failures
(after encoding with enc.Encode) by replacing the simple defer f.Close() with a
deferred closure that checks the error returned by f.Close() and wraps/returns
it (or appends it to any existing error from enc.Encode) so the function fails
if the file close/fsync fails; reference the writeWinresConfig function, the
file handle f, configPath and the enc.Encode call when implementing this change.
---
Nitpick comments:
In `@winres/generate-windows-resources/generate-windows-resources.go`:
- Line 99: The code uses time.Now() inline when building the "LegalCopyright"
string causing non-deterministic output; refactor the function that constructs
the metadata (the function that sets "LegalCopyright" in
generate-windows-resources.go) to accept an injected time source or explicit
year parameter (e.g., a time.Time or int year) instead of calling time.Now()
inside the function, and replace fmt.Sprintf("Copyright (C) %d %s",
time.Now().Year(), constants.Company) with a format that uses the injected value
so tests can pass a fixed year for deterministic metadata generation.
- Line 47: Change parseProductVersion to stop reading os.Args directly: give it
an explicit input (e.g., func parseProductVersion(raw string) (string, error) or
func parseProductVersion(args []string) ...) and trim the received string
(strings.TrimSpace) before validation; if the trimmed value is empty return a
descriptive error instead of accepting whitespace-only input. Update the call
site that does productVersion, err := parseProductVersion() to pass the
appropriate argument (the specific CLI token or os.Args element) and apply the
same refactor+trim/validation to the other parsing helpers in this file so all
argument parsing is deterministic and testable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml.github/workflows/publish.ymlwinres/generate-windows-resources/generate-windows-resources.gowinres/generate-windows-resources/generate-windows-resources_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yml
- .github/workflows/publish.yml
Comment on lines
+111
to
+127
| func writeWinresConfig(config map[string]interface{}) error { | ||
| if err := os.MkdirAll(winresDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create winres directory: %w", err) | ||
| } | ||
|
|
||
| configPath := filepath.Join(winresDir, winresJSONFile) | ||
| f, err := os.Create(configPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create %s: %w", configPath, err) | ||
| } | ||
| defer f.Close() | ||
|
|
||
| enc := json.NewEncoder(f) | ||
| enc.SetIndent("", " ") | ||
| if err := enc.Encode(config); err != nil { | ||
| return fmt.Errorf("failed to encode JSON: %w", err) | ||
| } |
There was a problem hiding this comment.
Handle Close() errors when writing winres.json.
Ignoring Close failures can return success even when the file write is incomplete (Line 121).
🛠️ Proposed fix
-func writeWinresConfig(config map[string]interface{}) error {
+func writeWinresConfig(config map[string]interface{}) (err error) {
if err := os.MkdirAll(winresDir, 0755); err != nil {
return fmt.Errorf("failed to create winres directory: %w", err)
}
@@
- f, err := os.Create(configPath)
+ f, err := os.Create(configPath)
if err != nil {
return fmt.Errorf("failed to create %s: %w", configPath, err)
}
- defer f.Close()
+ defer func() {
+ if cerr := f.Close(); err == nil && cerr != nil {
+ err = fmt.Errorf("failed to close %s: %w", configPath, cerr)
+ }
+ }()
@@
if err := enc.Encode(config); err != nil {
return fmt.Errorf("failed to encode JSON: %w", err)
}
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@winres/generate-windows-resources/generate-windows-resources.go` around lines
111 - 127, The writeWinresConfig function currently defers f.Close() without
checking its error; update it to capture and handle Close() failures (after
encoding with enc.Encode) by replacing the simple defer f.Close() with a
deferred closure that checks the error returned by f.Close() and wraps/returns
it (or appends it to any existing error from enc.Encode) so the function fails
if the file close/fsync fails; reference the writeWinresConfig function, the
file handle f, configPath and the enc.Encode call when implementing this change.
definitelynotagoblin
approved these changes
Feb 27, 2026
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
2 participants
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.
Ticket
https://specterops.atlassian.net/browse/BED-6446
Summary
go-winresin order to apply manifest information to the.exefor Windows users.buildandpublishGitHub actions in order to generate these resources just before the application is built and published.go-winres.Go 1.23.0 -> 1.24.0- I reran unit tests and performed a smoke test to make sure everything still works as expected after updating.Testing
act(https://github.com/nektos/act?tab=readme-ov-file)Testing Instructions
Goal: run the script I created to generate the Windows resources, build the image locally, and then check the
.exeto make sure that file properties get populated.Install
go-winresonto the windows machine (not in the AzH project directory)go install http://github.com/tc-hib/go-winres@latestAfter
go-winresis installed, pull my branchBED-6446-include-version-in-exe-propsThen run
go run cmd/generate-windows-resources/generate-windows-resources.go "v0.0.0-rolling+8675".sysofile is generatedBuild AzureHound (on Windows) with this command:
Find the
.exeand inspect the properties to ensure everything looks correct.Demo
All information populated as expected when examining the properties of the .exe ✅
Summary by CodeRabbit
New Features
Chores