Conversation
Co-Authored-By: Virgil <virgil@lethean.io>
- aur.go: initialize → initialise in comment and error string - ci.go: already_initialized → already_initialised i18n key - diff.go: add comment noting GetUncolorizedText is upstream oasdiff API Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Replace all fmt.Errorf and errors.New calls across 32 production files
with coreerr.E("caller.Method", "message", err) from go-log, matching
the convention already established in publishers/scoop.go and
publishers/homebrew.go.
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
132 translation keys for build, ci, sdk commands. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
SDK diff.go used getkin/kin-openapi but oasdiff v1.12+ uses its own fork. Switched import to match. Restores full CLI binary compilation. Co-Authored-By: Virgil <virgil@lethean.io>
Move i18n.T() calls out of package-level var declarations (which run at import time, before i18n is initialised) into setXxxI18n() functions called from AddXxxCommands at registration time. Co-Authored-By: Virgil <virgil@lethean.io>
- CLAUDE.md: update error wrapping convention from fmt.Errorf to coreerr.E() - cmd/build: fix RegisterCommands API mismatch (cli v0.3.5 compat) - locales: self-register translations via i18n.RegisterLocales in init() - cmd/sdk: replace fmt.Printf with cli.Print for consistent output - cmd/build/cmd_pwa: replace os.Stat with coreio.Local.IsDir - pkg/build/builders: replace os.Stat with io.Local.IsFile for CLI checks - pkg/release/publishers/scoop: replace os.Stat with coreio.Local.IsDir - pkg/api: add tests for getBuilder, resolveDir, and medium initialisation Co-Authored-By: Virgil <virgil@lethean.io>
…LAUDE.md — update any outdate...' (#1) from agent/dx-audit-and-fix--1--review-claude-md into main
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis pull request systematically replaces standard error handling across multiple modules with a centralized core error wrapper (coreerr.E) for consistent error construction. It introduces internationalization infrastructure through locales filesystem embedding, wires i18n initialization into command setup functions, and upgrades multiple Go module dependencies. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
✅ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
pkg/release/publishers/chocolatey.go (1)
231-254:⚠️ Potential issue | 🟡 MinorConsider API key exposure in process listings.
The API key passed via
--api-keycommand-line argument (line 249) is visible in process listings (ps aux,/proc/<pid>/cmdline). Whilst this is the standardchocoCLI pattern and the key already exists in the environment, command-line arguments have broader visibility on multi-user systems.If this is a concern for your deployment environment, consider:
- Using
choco apikey --setduring CI setup to store the key in Chocolatey's config beforehand, then omitting--api-keyfrom the push command.- Documenting that builds should run in isolated environments.
This is informational rather than a blocker, as the current approach aligns with common CI/CD practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release/publishers/chocolatey.go` around lines 231 - 254, The pushToChocolatey function currently passes CHOCOLATEY_API_KEY on the choco push command line (visible in process listings); instead, set the API key in Chocolatey config prior to pushing and omit the --api-key argument: call choco apikey (e.g. the equivalent of "choco apikey --set"/"choco apikey --key") using the CHOCOLATEY_API_KEY env var (referencing CHOCOLATEY_API_KEY, pushToChocolatey, cmd and nupkgPath) and then change the choco push invocation to remove the "--api-key" flag so the key is not exposed in process arguments; optionally document that CI should run in isolated environments if you keep the current approach.pkg/release/publishers/npm.go (1)
240-245:⚠️ Potential issue | 🟡 MinorSilent error swallowing may hide custom template failures.
If
m.IsFile(customPath)returns true butm.Read(customPath)subsequently fails (e.g., permissions, encoding issues), the error is silently ignored and the embedded template is used instead. Users won't be aware their custom template wasn't applied.Consider logging a warning or returning an error when a custom template exists but cannot be read.
🛠️ Suggested fix
customPath := filepath.Join(".core", name) if m != nil && m.IsFile(customPath) { customContent, err := m.Read(customPath) - if err == nil { + if err != nil { + return "", coreerr.E("npm.renderTemplate", "failed to read custom template "+customPath, err) + } + if customContent != "" { content = []byte(customContent) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release/publishers/npm.go` around lines 240 - 245, The code silently ignores errors from m.Read(customPath), so when m.IsFile(customPath) is true but m.Read fails the custom template silently falls back to embedded content; update the branch that calls m.Read(customPath) to handle err != nil explicitly: either return the error up from the current function or emit a warning via the package logger including customPath and err, and only overwrite content on successful read (keep using content variable only when err == nil). Ensure you modify the snippet that calls m.IsFile and m.Read (and the surrounding function that uses content) so callers/users are informed when a custom template exists but cannot be read.cmd/build/cmd_release.go (1)
35-46:⚠️ Potential issue | 🟠 MajorGuard flag initialisation to prevent duplicate-flag panics.
Line 35-40 is called from
AddReleaseCommand(Line 44-45). IfAddReleaseCommandis invoked more than once in the same process, these flag registrations will panic due to duplicate names.Suggested fix
import ( "context" "os" + "sync" @@ ) @@ var ( releaseVersion string releaseDraft bool releasePrerelease bool releaseGoForLaunch bool + releaseFlagsOnce sync.Once ) @@ func initReleaseFlags() { - releaseCmd.Flags().BoolVar(&releaseGoForLaunch, "we-are-go-for-launch", false, i18n.T("cmd.build.release.flag.go_for_launch")) - releaseCmd.Flags().StringVar(&releaseVersion, "version", "", i18n.T("cmd.build.release.flag.version")) - releaseCmd.Flags().BoolVar(&releaseDraft, "draft", false, i18n.T("cmd.build.release.flag.draft")) - releaseCmd.Flags().BoolVar(&releasePrerelease, "prerelease", false, i18n.T("cmd.build.release.flag.prerelease")) + releaseFlagsOnce.Do(func() { + releaseCmd.Flags().BoolVar(&releaseGoForLaunch, "we-are-go-for-launch", false, i18n.T("cmd.build.release.flag.go_for_launch")) + releaseCmd.Flags().StringVar(&releaseVersion, "version", "", i18n.T("cmd.build.release.flag.version")) + releaseCmd.Flags().BoolVar(&releaseDraft, "draft", false, i18n.T("cmd.build.release.flag.draft")) + releaseCmd.Flags().BoolVar(&releasePrerelease, "prerelease", false, i18n.T("cmd.build.release.flag.prerelease")) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build/cmd_release.go` around lines 35 - 46, The flag registrations in initReleaseFlags() (releaseCmd.Flags().BoolVar / StringVar etc.) can panic if called more than once; make initialization idempotent by guarding it with a package-level sync.Once (e.g., releaseFlagsInitOnce) or a boolean guard and call that guard from AddReleaseCommand before calling initReleaseFlags() and setReleaseI18n(); ensure initReleaseFlags() only runs once so duplicate calls to AddReleaseCommand won't re-register flags on releaseCmd.cmd/build/cmd_project.go (1)
348-356:⚠️ Potential issue | 🟡 MinorReject empty OS/architecture tokens in target parsing.
Line 354 and Line 355 can still be empty after trimming. Inputs like
linux/or/amd64pass current validation and fail later with less actionable errors. Validate both fields before appending.💡 Proposed fix
osArch := strings.Split(part, "/") if len(osArch) != 2 { return nil, coreerr.E("build.parseTargets", "invalid target format (expected os/arch): "+part, nil) } + osVal := strings.TrimSpace(osArch[0]) + archVal := strings.TrimSpace(osArch[1]) + if osVal == "" || archVal == "" { + return nil, coreerr.E("build.parseTargets", "invalid target format (expected non-empty os/arch): "+part, nil) + } targets = append(targets, build.Target{ - OS: strings.TrimSpace(osArch[0]), - Arch: strings.TrimSpace(osArch[1]), + OS: osVal, + Arch: archVal, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build/cmd_project.go` around lines 348 - 356, When parsing targets into os/arch pairs (the osArch split used to build build.Target entries), the code currently only checks len(osArch) == 2 but does not reject empty tokens; trim both parts (os := strings.TrimSpace(osArch[0]); arch := strings.TrimSpace(osArch[1])) and validate they are non-empty before appending to targets. If either os or arch is empty, return a coreerr.E error (consistent with the existing "build.parseTargets" error style) describing the invalid target format so inputs like "linux/" or "/amd64" fail early with a clear message.pkg/build/builders/cpp.go (1)
205-213:⚠️ Potential issue | 🟠 MajorReplace
os.Stat()with the injected filesystem method.The function receives an
fs io.Mediumparameter and uses it forIsDir()andList()operations (lines 180 and 184), but line 206 switches toos.Stat(fullPath). This breaks the filesystem abstraction and causes silent data loss: entries returned byfs.List()will be silently skipped becauseos.Stat()cannot access paths on non-local mediums.Fix
- info, err := os.Stat(fullPath) + info, err := fs.Stat(fullPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/build/builders/cpp.go` around lines 205 - 213, Replace the direct call to os.Stat(fullPath) with the injected filesystem on the function's io.Medium parameter (use fs.Stat or the equivalent Stat method on the io.Medium) so the code uses the same abstracted medium used by fs.IsDir() and fs.List(); keep the existing logic that checks info.Mode()&0111 for executability and continue on error, but obtain info from fs.Stat(fullPath) (respecting the io.Medium API) instead of os.Stat.pkg/build/archive.go (1)
223-249:⚠️ Potential issue | 🔴 CriticalPropagate archive writer finalisation errors.
Ignoring
Close()errors on lines 225, 229, and 275 allows the function to return success whilst leaving a corrupt archive on disk. Thegzip.Writer.Close(),tar.Writer.Close(), andzip.Writer.Close()calls write critical metadata: GZIP footer (CRC and uncompressed size), tar footer (two 512-byte zero blocks), and ZIP central directory respectively.Check these errors before returning nil:
Example fix for
createTarGzArchive- gzWriter := gzip.NewWriter(dstFile) - defer func() { _ = gzWriter.Close() }() + gzWriter := gzip.NewWriter(dstFile) - tarWriter := tar.NewWriter(gzWriter) - defer func() { _ = tarWriter.Close() }() + tarWriter := tar.NewWriter(gzWriter) // ... file content operations ... - return nil + if err := tarWriter.Close(); err != nil { + return coreerr.E("build.createTarGzArchive", "failed to close tar writer", err) + } + if err := gzWriter.Close(); err != nil { + return coreerr.E("build.createTarGzArchive", "failed to close gzip writer", err) + } + return nilApply the same pattern to
createZipArchiveforzipWriter.Close()and the destination file close.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/build/archive.go` around lines 223 - 249, The Close() errors from archive writers and the destination file are currently ignored, which can leave corrupt archives; update createTarGzArchive and createZipArchive to capture and check the errors returned from gzWriter.Close(), tarWriter.Close(), zipWriter.Close() and dstFile.Close() (and any srcFile.Close() if deferred closures return errors) and return a coreerr.E with a clear message (e.g., "failed to finalize gzip/tar/zip archive" or "failed to close destination file") when those Close() calls fail instead of discarding the error; make sure to call Close() in the correct order (flush writers before closing the underlying file) and propagate the first non-nil error back to the caller.cmd/build/cmd_pwa.go (1)
206-214:⚠️ Potential issue | 🔴 CriticalValidate asset paths to prevent writing outside the destination directory.
The path construction using
filepath.Join(destDir, filepath.FromSlash(u.Path))is vulnerable to directory traversal. Sinceu.Pathis extracted from a remote manifest URL and can contain..segments (e.g., a manifest listing an asset with path../../etc/passwd), an attacker can craft a manifest that writes arbitrary files outsidedestDir. Go'sfilepath.Joinis not a security boundary—it normalises paths but does not prevent escaping.Recommended fix: For Go 1.21+, use
filepath.IsLocal(filepath.FromSlash(u.Path))to reject any path containing.., absolute components, or other escape sequences before joining. Alternatively, validate the final path explicitly:Path validation example
cleanPath := filepath.Clean(filepath.Join(destDir, filepath.FromSlash(u.Path))) cleanDestDir := filepath.Clean(destDir) if cleanPath != cleanDestDir && !strings.HasPrefix(cleanPath, cleanDestDir+string(filepath.Separator)) { return coreerr.E("pwa.downloadAsset", "asset path escapes destination directory", nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build/cmd_pwa.go` around lines 206 - 214, The code currently joins destDir and filepath.FromSlash(u.Path) (used before calling coreio.Local.EnsureDir and os.Create) which allows directory-traversal via `..`; validate and reject paths that escape destDir before creating directories or files. Replace the naive join with a check that either uses filepath.IsLocal(filepath.FromSlash(u.Path)) on Go 1.21+ to reject unsafe inputs, or compute a cleaned absolute path (cleanPath := filepath.Clean(filepath.Join(destDir, filepath.FromSlash(u.Path)))) and ensure it equals cleanDestDir or has cleanDestDir+separator as a prefix; if the check fails return a descriptive error (e.g., using the existing pwa download error path like coreerr.E("pwa.downloadAsset", "asset path escapes destination directory", nil)). Ensure the validation happens prior to calling coreio.Local.EnsureDir and os.Create.
🧹 Nitpick comments (6)
pkg/release/publishers/npm.go (1)
133-133: Unused parametercfg.The
cfg *NpmConfigparameter is not used withindryRunPublish. Consider removing it if it's not needed for API consistency, or prefix with_to indicate intentional non-use.♻️ Suggested fix
-func (p *NpmPublisher) dryRunPublish(m coreio.Medium, data npmTemplateData, cfg *NpmConfig) error { +func (p *NpmPublisher) dryRunPublish(m coreio.Medium, data npmTemplateData, _ *NpmConfig) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release/publishers/npm.go` at line 133, The method signature for NpmPublisher.dryRunPublish includes an unused parameter cfg *NpmConfig; remove this parameter from the function signature and all call sites if API compatibility allows, otherwise rename it to _cfg (or _ in the parameter list) to mark it intentionally unused; update references to the function (calls and any interface definitions) to match the new signature and run tests to ensure no call sites are left with the removed argument.pkg/build/signing/sign.go (1)
93-93: Include the checksum file path in the error message.Using
checksumFilein this message would make failures easier to pinpoint in multi-artifact runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/build/signing/sign.go` at line 93, Update the error returned from signing.SignChecksums so it includes the checksum file path (checksumFile) in the message; replace the current coreerr.E("signing.SignChecksums", "failed to sign checksums", err) with a message that embeds checksumFile (e.g. "failed to sign checksums for <checksumFile>") so failures clearly identify which checksum file failed to sign.pkg/build/signing/codesign.go (1)
45-45: Make the unavailable error reason explicit.
codesign not availablehides whether the blocker is OS, identity, or missing binary. Returning a specific reason here makes failures actionable.Proposed refactor
func (s *MacOSSigner) Sign(ctx context.Context, fs io.Medium, binary string) error { - if !s.Available() { - return coreerr.E("codesign.Sign", "codesign not available", nil) - } + if runtime.GOOS != "darwin" { + return coreerr.E("codesign.Sign", "codesign requires darwin", nil) + } + if s.config.Identity == "" { + return coreerr.E("codesign.Sign", "missing codesign identity", nil) + } + if _, err := exec.LookPath("codesign"); err != nil { + return coreerr.E("codesign.Sign", "codesign binary not found", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/build/signing/codesign.go` at line 45, The current return coreerr.E("codesign.Sign", "codesign not available", nil) hides the actual cause; update the codesign.Sign path to detect and include the specific reason (unsupported OS, missing codesign binary, or absent signing identity) in the error message and attach any underlying error/inspect details to coreerr.E so the returned error reads e.g. "codesign not available: missing binary" or "codesign not available: no signing identity" and preserves the original error value instead of nil; locate the call site in the Sign function and replace the generic message with a reasoned message and underlying error passed into coreerr.E.pkg/release/publishers/scoop.go (1)
256-261: Consider distinguishing file-not-found from other read errors.When
m.Read(customPath)fails, the error is silently discarded and the code falls back to the embedded template. This is reasonable for missing files, but could mask permission errors or I/O failures that the user should be aware of.💡 Optional: Log non-existent vs failed reads
customPath := filepath.Join(".core", name) if m != nil && m.IsFile(customPath) { customContent, err := m.Read(customPath) if err == nil { content = []byte(customContent) + } else { + // Log warning: custom template exists but couldn't be read + fmt.Fprintf(os.Stderr, "Warning: failed to read custom template %s, using default: %v\n", customPath, err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release/publishers/scoop.go` around lines 256 - 261, The code currently swallows all errors from m.Read(customPath) and silently falls back to the embedded template; change it so only a "file not found" error is treated as a fallback: call m.Read(customPath), and if err == nil assign customContent to content, but if err != nil check whether the error indicates the file is missing (e.g. os.IsNotExist(err) or the equivalent error from the filesystem implementation) and only ignore that case; for any other error (permission, I/O, etc.) return or propagate/log the error so the caller of the scoop publisher (the code path using m.Read, customPath, and content) can handle it appropriately. Ensure you reference m.Read and customPath in the updated logic.pkg/build/builders/wails.go (1)
145-151: Inconsistent error handling: raw errors returned withoutcoreerr.Ewrapping.These error paths return raw errors directly, unlike the other error cases in this file which use
coreerr.E. For consistency with the rest of the migration, consider wrapping these errors as well.♻️ Suggested fix for consistency
// Simple copy using the medium content, err := cfg.FS.Read(sourcePath) if err != nil { - return build.Artifact{}, err + return build.Artifact{}, coreerr.E("WailsBuilder.buildV2Target", "failed to read source artifact", err) } if err := cfg.FS.Write(destPath, content); err != nil { - return build.Artifact{}, err + return build.Artifact{}, coreerr.E("WailsBuilder.buildV2Target", "failed to write artifact to output", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/build/builders/wails.go` around lines 145 - 151, The Read/Write error paths currently return raw errors from cfg.FS.Read and cfg.FS.Write (inside the code that returns build.Artifact{}), which is inconsistent with other error handling in this file; update these returns to wrap the errors with coreerr.E (e.g., return build.Artifact{}, coreerr.E(err) or coreerr.E("failed to read source", err)) so they match the rest of the file's error-wrapping style and include contextual message for cfg.FS.Read and cfg.FS.Write calls.pkg/release/publishers/github.go (1)
150-159: Preserve underlyingghexecution errors.Line 151 and Line 158 currently pass
nilas the cause, which drops useful diagnostics fromexec.Suggested fix
if err := cmd.Run(); err != nil { - return coreerr.E("github.validateGhCli", "gh CLI not found. Install it from https://cli.github.com", nil) + return coreerr.E("github.validateGhCli", "gh CLI not found. Install it from https://cli.github.com", err) } @@ output, err := cmd.CombinedOutput() if err != nil { - return coreerr.E("github.validateGhCli", "not authenticated with gh CLI. Run 'gh auth login' first", nil) + return coreerr.E("github.validateGhCli", "not authenticated with gh CLI. Run 'gh auth login' first", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release/publishers/github.go` around lines 150 - 159, The checks that run the gh CLI use coreerr.E with a nil cause, dropping the underlying exec errors; update the error returns in the cmd.Run() failure and the cmd.CombinedOutput() failure to pass the actual exec error (and optionally the command output for richer context) as the cause so diagnostics are preserved—modify the branch that calls cmd.Run() and the branch that calls cmd.CombinedOutput() to return coreerr.E("github.validateGhCli", "<message>", err) (or include fmt.Errorf("%s: %w", string(output), err) as the cause) instead of nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/build/builders/docker.go`:
- Line 190: The wrapped errors in DockerBuilder.validateDockerCli currently drop
the underlying command failure by passing nil as the cause; update both
coreerr.E calls that return the "docker CLI not found..." and "docker daemon not
running..." errors to pass the actual error variable (err) returned from
cmd.Run() as the third argument so the original cmd.Run() failure is preserved
in the wrapped error (locate the coreerr.E calls inside validateDockerCli and
replace the nil cause with err).
In `@pkg/release/publishers/docker.go`:
- Around line 274-279: The docker CLI validation in validateDockerCli discards
the underlying error from cmd.Run(); capture the returned err and pass it into
the coreerr.E wrapper so diagnostics include the original error (i.e., when
cmd.Run() returns non-nil, return coreerr.E("docker.validateDockerCli", "docker
CLI not found. Install it from https://docs.docker.com/get-docker/", err)). This
preserves the original error context for troubleshooting while keeping the
existing message and error code.
- Around line 253-256: The error from exec.CommandContext(...).Run() is being
discarded; update the error return so the original err is passed into coreerr.E
instead of nil. Locate the docker buildx check (where cmd :=
exec.CommandContext(ctx, "docker", "buildx", "version") and if err := cmd.Run();
err != nil { ... }) and change the return to return
coreerr.E("docker.ensureBuildx", "buildx is not available. Install it from
https://docs.docker.com/buildx/working-with-buildx/", err) so the underlying
diagnostic is preserved.
In `@pkg/release/publishers/github.go`:
- Line 201: The error currently returned in github.parseGitHubRepo includes the
raw remote URL which may leak credentials; change the return in parseGitHubRepo
(the coreerr.E call) to avoid embedding the raw url by using a generic message
like "not a GitHub URL" or redacting the URL first (e.g., replace userinfo with
"<redacted>") before including it. Update the coreerr.E invocation in github.go
to either remove the "+url" concatenation or call a small helper (e.g.,
redactRemote(url)) to sanitize the value so no sensitive auth data is logged.
In `@pkg/release/publishers/linuxkit.go`:
- Around line 301-303: The linuxkit CLI validation currently discards the
underlying error returned by cmd.Run(); update the error wrap so the actual err
is passed into coreerr.E(...) instead of nil. Locate the block where cmd.Run()
is executed (the validation around linuxkit version) and change the coreerr.E
call to include the caught err as the third argument so the original failure is
preserved for diagnostics.
---
Outside diff comments:
In `@cmd/build/cmd_project.go`:
- Around line 348-356: When parsing targets into os/arch pairs (the osArch split
used to build build.Target entries), the code currently only checks len(osArch)
== 2 but does not reject empty tokens; trim both parts (os :=
strings.TrimSpace(osArch[0]); arch := strings.TrimSpace(osArch[1])) and validate
they are non-empty before appending to targets. If either os or arch is empty,
return a coreerr.E error (consistent with the existing "build.parseTargets"
error style) describing the invalid target format so inputs like "linux/" or
"/amd64" fail early with a clear message.
In `@cmd/build/cmd_pwa.go`:
- Around line 206-214: The code currently joins destDir and
filepath.FromSlash(u.Path) (used before calling coreio.Local.EnsureDir and
os.Create) which allows directory-traversal via `..`; validate and reject paths
that escape destDir before creating directories or files. Replace the naive join
with a check that either uses filepath.IsLocal(filepath.FromSlash(u.Path)) on Go
1.21+ to reject unsafe inputs, or compute a cleaned absolute path (cleanPath :=
filepath.Clean(filepath.Join(destDir, filepath.FromSlash(u.Path)))) and ensure
it equals cleanDestDir or has cleanDestDir+separator as a prefix; if the check
fails return a descriptive error (e.g., using the existing pwa download error
path like coreerr.E("pwa.downloadAsset", "asset path escapes destination
directory", nil)). Ensure the validation happens prior to calling
coreio.Local.EnsureDir and os.Create.
In `@cmd/build/cmd_release.go`:
- Around line 35-46: The flag registrations in initReleaseFlags()
(releaseCmd.Flags().BoolVar / StringVar etc.) can panic if called more than
once; make initialization idempotent by guarding it with a package-level
sync.Once (e.g., releaseFlagsInitOnce) or a boolean guard and call that guard
from AddReleaseCommand before calling initReleaseFlags() and setReleaseI18n();
ensure initReleaseFlags() only runs once so duplicate calls to AddReleaseCommand
won't re-register flags on releaseCmd.
In `@pkg/build/archive.go`:
- Around line 223-249: The Close() errors from archive writers and the
destination file are currently ignored, which can leave corrupt archives; update
createTarGzArchive and createZipArchive to capture and check the errors returned
from gzWriter.Close(), tarWriter.Close(), zipWriter.Close() and dstFile.Close()
(and any srcFile.Close() if deferred closures return errors) and return a
coreerr.E with a clear message (e.g., "failed to finalize gzip/tar/zip archive"
or "failed to close destination file") when those Close() calls fail instead of
discarding the error; make sure to call Close() in the correct order (flush
writers before closing the underlying file) and propagate the first non-nil
error back to the caller.
In `@pkg/build/builders/cpp.go`:
- Around line 205-213: Replace the direct call to os.Stat(fullPath) with the
injected filesystem on the function's io.Medium parameter (use fs.Stat or the
equivalent Stat method on the io.Medium) so the code uses the same abstracted
medium used by fs.IsDir() and fs.List(); keep the existing logic that checks
info.Mode()&0111 for executability and continue on error, but obtain info from
fs.Stat(fullPath) (respecting the io.Medium API) instead of os.Stat.
In `@pkg/release/publishers/chocolatey.go`:
- Around line 231-254: The pushToChocolatey function currently passes
CHOCOLATEY_API_KEY on the choco push command line (visible in process listings);
instead, set the API key in Chocolatey config prior to pushing and omit the
--api-key argument: call choco apikey (e.g. the equivalent of "choco apikey
--set"/"choco apikey --key") using the CHOCOLATEY_API_KEY env var (referencing
CHOCOLATEY_API_KEY, pushToChocolatey, cmd and nupkgPath) and then change the
choco push invocation to remove the "--api-key" flag so the key is not exposed
in process arguments; optionally document that CI should run in isolated
environments if you keep the current approach.
In `@pkg/release/publishers/npm.go`:
- Around line 240-245: The code silently ignores errors from m.Read(customPath),
so when m.IsFile(customPath) is true but m.Read fails the custom template
silently falls back to embedded content; update the branch that calls
m.Read(customPath) to handle err != nil explicitly: either return the error up
from the current function or emit a warning via the package logger including
customPath and err, and only overwrite content on successful read (keep using
content variable only when err == nil). Ensure you modify the snippet that calls
m.IsFile and m.Read (and the surrounding function that uses content) so
callers/users are informed when a custom template exists but cannot be read.
---
Nitpick comments:
In `@pkg/build/builders/wails.go`:
- Around line 145-151: The Read/Write error paths currently return raw errors
from cfg.FS.Read and cfg.FS.Write (inside the code that returns
build.Artifact{}), which is inconsistent with other error handling in this file;
update these returns to wrap the errors with coreerr.E (e.g., return
build.Artifact{}, coreerr.E(err) or coreerr.E("failed to read source", err)) so
they match the rest of the file's error-wrapping style and include contextual
message for cfg.FS.Read and cfg.FS.Write calls.
In `@pkg/build/signing/codesign.go`:
- Line 45: The current return coreerr.E("codesign.Sign", "codesign not
available", nil) hides the actual cause; update the codesign.Sign path to detect
and include the specific reason (unsupported OS, missing codesign binary, or
absent signing identity) in the error message and attach any underlying
error/inspect details to coreerr.E so the returned error reads e.g. "codesign
not available: missing binary" or "codesign not available: no signing identity"
and preserves the original error value instead of nil; locate the call site in
the Sign function and replace the generic message with a reasoned message and
underlying error passed into coreerr.E.
In `@pkg/build/signing/sign.go`:
- Line 93: Update the error returned from signing.SignChecksums so it includes
the checksum file path (checksumFile) in the message; replace the current
coreerr.E("signing.SignChecksums", "failed to sign checksums", err) with a
message that embeds checksumFile (e.g. "failed to sign checksums for
<checksumFile>") so failures clearly identify which checksum file failed to
sign.
In `@pkg/release/publishers/github.go`:
- Around line 150-159: The checks that run the gh CLI use coreerr.E with a nil
cause, dropping the underlying exec errors; update the error returns in the
cmd.Run() failure and the cmd.CombinedOutput() failure to pass the actual exec
error (and optionally the command output for richer context) as the cause so
diagnostics are preserved—modify the branch that calls cmd.Run() and the branch
that calls cmd.CombinedOutput() to return coreerr.E("github.validateGhCli",
"<message>", err) (or include fmt.Errorf("%s: %w", string(output), err) as the
cause) instead of nil.
In `@pkg/release/publishers/npm.go`:
- Line 133: The method signature for NpmPublisher.dryRunPublish includes an
unused parameter cfg *NpmConfig; remove this parameter from the function
signature and all call sites if API compatibility allows, otherwise rename it to
_cfg (or _ in the parameter list) to mark it intentionally unused; update
references to the function (calls and any interface definitions) to match the
new signature and run tests to ensure no call sites are left with the removed
argument.
In `@pkg/release/publishers/scoop.go`:
- Around line 256-261: The code currently swallows all errors from
m.Read(customPath) and silently falls back to the embedded template; change it
so only a "file not found" error is treated as a fallback: call
m.Read(customPath), and if err == nil assign customContent to content, but if
err != nil check whether the error indicates the file is missing (e.g.
os.IsNotExist(err) or the equivalent error from the filesystem implementation)
and only ignore that case; for any other error (permission, I/O, etc.) return or
propagate/log the error so the caller of the scoop publisher (the code path
using m.Read, customPath, and content) can handle it appropriately. Ensure you
reference m.Read and customPath in the updated logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22eb7003-8627-4296-a381-c3989219b74a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
cmd/build/cmd_build.gocmd/build/cmd_project.gocmd/build/cmd_pwa.gocmd/build/cmd_release.gocmd/build/cmd_sdk.gocmd/ci/ci.gocmd/ci/cmd.gocmd/sdk/cmd.gogo.modlocales/embed.golocales/en.jsonpkg/build/archive.gopkg/build/builders/cpp.gopkg/build/builders/docker.gopkg/build/builders/go.gopkg/build/builders/linuxkit.gopkg/build/builders/taskfile.gopkg/build/builders/wails.gopkg/build/checksum.gopkg/build/config.gopkg/build/signing/codesign.gopkg/build/signing/gpg.gopkg/build/signing/sign.gopkg/release/changelog.gopkg/release/config.gopkg/release/publishers/aur.gopkg/release/publishers/chocolatey.gopkg/release/publishers/docker.gopkg/release/publishers/github.gopkg/release/publishers/homebrew.gopkg/release/publishers/linuxkit.gopkg/release/publishers/npm.gopkg/release/publishers/scoop.gopkg/release/release.gopkg/release/sdk.gopkg/release/version.gopkg/sdk/detect.gopkg/sdk/diff.gopkg/sdk/generators/go.gopkg/sdk/generators/php.gopkg/sdk/generators/python.gopkg/sdk/generators/typescript.gopkg/sdk/sdk.go
Comment on lines
253
to
256
| cmd := exec.CommandContext(ctx, "docker", "buildx", "version") | ||
| if err := cmd.Run(); err != nil { | ||
| return errors.New("docker: buildx is not available. Install it from https://docs.docker.com/buildx/working-with-buildx/") | ||
| return coreerr.E("docker.ensureBuildx", "buildx is not available. Install it from https://docs.docker.com/buildx/working-with-buildx/", nil) | ||
| } |
There was a problem hiding this comment.
Consider preserving the original error for diagnostics.
The error from cmd.Run() is discarded by passing nil as the third argument. This loses potentially useful diagnostic information (e.g., permission issues, PATH problems). Line 266 in the same function correctly wraps its error.
🔧 Suggested fix
cmd := exec.CommandContext(ctx, "docker", "buildx", "version")
if err := cmd.Run(); err != nil {
- return coreerr.E("docker.ensureBuildx", "buildx is not available. Install it from https://docs.docker.com/buildx/working-with-buildx/", nil)
+ return coreerr.E("docker.ensureBuildx", "buildx is not available. Install it from https://docs.docker.com/buildx/working-with-buildx/", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/release/publishers/docker.go` around lines 253 - 256, The error from
exec.CommandContext(...).Run() is being discarded; update the error return so
the original err is passed into coreerr.E instead of nil. Locate the docker
buildx check (where cmd := exec.CommandContext(ctx, "docker", "buildx",
"version") and if err := cmd.Run(); err != nil { ... }) and change the return to
return coreerr.E("docker.ensureBuildx", "buildx is not available. Install it
from https://docs.docker.com/buildx/working-with-buildx/", err) so the
underlying diagnostic is preserved.
Comment on lines
274
to
279
| func validateDockerCli() error { | ||
| cmd := exec.Command("docker", "--version") | ||
| if err := cmd.Run(); err != nil { | ||
| return errors.New("docker: docker CLI not found. Install it from https://docs.docker.com/get-docker/") | ||
| return coreerr.E("docker.validateDockerCli", "docker CLI not found. Install it from https://docs.docker.com/get-docker/", nil) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Consider preserving the original error for diagnostics.
Same issue as in ensureBuildx: the error from cmd.Run() is discarded. Wrapping it would provide useful context when debugging why the docker CLI check failed.
🔧 Suggested fix
func validateDockerCli() error {
cmd := exec.Command("docker", "--version")
if err := cmd.Run(); err != nil {
- return coreerr.E("docker.validateDockerCli", "docker CLI not found. Install it from https://docs.docker.com/get-docker/", nil)
+ return coreerr.E("docker.validateDockerCli", "docker CLI not found. Install it from https://docs.docker.com/get-docker/", err)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func validateDockerCli() error { | |
| cmd := exec.Command("docker", "--version") | |
| if err := cmd.Run(); err != nil { | |
| return errors.New("docker: docker CLI not found. Install it from https://docs.docker.com/get-docker/") | |
| return coreerr.E("docker.validateDockerCli", "docker CLI not found. Install it from https://docs.docker.com/get-docker/", nil) | |
| } | |
| return nil | |
| func validateDockerCli() error { | |
| cmd := exec.Command("docker", "--version") | |
| if err := cmd.Run(); err != nil { | |
| return coreerr.E("docker.validateDockerCli", "docker CLI not found. Install it from https://docs.docker.com/get-docker/", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/release/publishers/docker.go` around lines 274 - 279, The docker CLI
validation in validateDockerCli discards the underlying error from cmd.Run();
capture the returned err and pass it into the coreerr.E wrapper so diagnostics
include the original error (i.e., when cmd.Run() returns non-nil, return
coreerr.E("docker.validateDockerCli", "docker CLI not found. Install it from
https://docs.docker.com/get-docker/", err)). This preserves the original error
context for troubleshooting while keeping the existing message and error code.
| } | ||
|
|
||
| return "", fmt.Errorf("not a GitHub URL: %s", url) | ||
| return "", coreerr.E("github.parseGitHubRepo", "not a GitHub URL: "+url, nil) |
There was a problem hiding this comment.
Do not include raw remote URLs in error text.
Line 201 can leak credentials if the remote is tokenised (e.g. HTTPS remotes with embedded auth). Keep the message generic or redact before logging.
Suggested fix
- return "", coreerr.E("github.parseGitHubRepo", "not a GitHub URL: "+url, nil)
+ return "", coreerr.E("github.parseGitHubRepo", "not a supported GitHub URL", nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return "", coreerr.E("github.parseGitHubRepo", "not a GitHub URL: "+url, nil) | |
| return "", coreerr.E("github.parseGitHubRepo", "not a supported GitHub URL", nil) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/release/publishers/github.go` at line 201, The error currently returned
in github.parseGitHubRepo includes the raw remote URL which may leak
credentials; change the return in parseGitHubRepo (the coreerr.E call) to avoid
embedding the raw url by using a generic message like "not a GitHub URL" or
redacting the URL first (e.g., replace userinfo with "<redacted>") before
including it. Update the coreerr.E invocation in github.go to either remove the
"+url" concatenation or call a small helper (e.g., redactRemote(url)) to
sanitize the value so no sensitive auth data is logged.
Comment on lines
301
to
303
| if err := cmd.Run(); err != nil { | ||
| return errors.New("linuxkit: linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit") | ||
| return coreerr.E("linuxkit.validateLinuxKitCli", "linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit", nil) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current implementation around validateLinuxKitCli and confirm whether the wrapped cause is nil.
awk 'NR>=299 && NR<=304 {print NR ":" $0}' pkg/release/publishers/linuxkit.goRepository: dAppCore/go-build
Length of output: 349
🏁 Script executed:
find . -type f -name "*.go" | xargs grep -l "package coreerr" | head -5Repository: dAppCore/go-build
Length of output: 43
🏁 Script executed:
rg "func \(E\|E\w+\)\(" --type go -A 3 | grep -A 3 "coreerr" | head -30Repository: dAppCore/go-build
Length of output: 43
🏁 Script executed:
rg "coreerr\.E\(" --type go -B 1 -A 1 | head -50Repository: dAppCore/go-build
Length of output: 2446
Preserve the underlying cmd.Run() error in CLI validation.
Line 302 currently wraps with nil, which drops the actual failure cause from linuxkit version and makes diagnosis harder. The established pattern in the codebase (e.g. typescript.Generate, go.generateNative) is to pass the caught error to coreerr.E() when available.
Suggested patch
func validateLinuxKitCli() error {
cmd := exec.Command("linuxkit", "version")
if err := cmd.Run(); err != nil {
- return coreerr.E("linuxkit.validateLinuxKitCli", "linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit", nil)
+ return coreerr.E("linuxkit.validateLinuxKitCli", "linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit", err)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := cmd.Run(); err != nil { | |
| return errors.New("linuxkit: linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit") | |
| return coreerr.E("linuxkit.validateLinuxKitCli", "linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit", nil) | |
| } | |
| func validateLinuxKitCli() error { | |
| cmd := exec.Command("linuxkit", "version") | |
| if err := cmd.Run(); err != nil { | |
| return coreerr.E("linuxkit.validateLinuxKitCli", "linuxkit CLI not found. Install it from https://github.com/linuxkit/linuxkit", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/release/publishers/linuxkit.go` around lines 301 - 303, The linuxkit CLI
validation currently discards the underlying error returned by cmd.Run(); update
the error wrap so the actual err is passed into coreerr.E(...) instead of nil.
Locate the block where cmd.Run() is executed (the validation around linuxkit
version) and change the coreerr.E call to include the caught err as the third
argument so the original failure is preserved for diagnostics.
- chocolatey: pass API key via env var instead of CLI arg - npm: remove unused cfg parameter from dryRunPublish - signing: include checksums file path in error message - codesign: provide specific reason when codesign unavailable - scoop: handle error from custom template read - wails: wrap Read/Write errors with coreerr.E - github: include actual error in validateGhCli coreerr.E calls - docker: include actual error in validate/buildx coreerr.E calls Co-Authored-By: Virgil <virgil@lethean.io>
…fy each against current code, f...' (#2) from agent/fix-coderabbit-findings--verify-each-aga into main
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Forge → GitHub Sync
Commits: 10
Files changed: 44
Automated sync from Forge (forge.lthn.ai) to GitHub mirror.
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Release Notes
New Features
prereleaseflag to the release command for managing pre-release versions.Improvements
Dependencies