Refactor caib main.go#139
Merged
Merged
Conversation
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
📝 WalkthroughWalkthroughAdds a comprehensive "image" CLI with build/disk/build-dev/download/logs/flash/sealed/query subcommands, API client with reauth, manifest/OCI helpers, log streaming and flash feedback, build-name validation, runtime wiring, and small Makefile build path tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "caib build"
participant Handler as "Build Handler"
participant API as "Build API"
participant Upload as "Upload Service"
participant Registry as "Container Registry"
CLI->>Handler: RunBuild(manifest, flags)
Handler->>Handler: Sanitize name, validate flags
Handler->>Handler: Parse manifest, find local file refs
Handler->>Upload: Poll & upload local artifacts
Handler->>Handler: Construct BuildRequest (apply registry creds)
Handler->>API: CreateBuild(request)
API-->>Handler: BuildResponse (name)
loop wait / follow
Handler->>API: GetBuild(name)
API-->>Handler: BuildStatus
alt active
Handler->>Handler: Try open log stream
Handler-->>Handler: Stream log lines via Logstream
end
end
opt registry artifact
Handler->>Registry: (optional) Pull artifact
end
Handler->>CLI: Display results & flash instructions
sequenceDiagram
participant CLI as "caib flash"
participant Handler as "Flash Handler"
participant API as "Build API"
participant JS as "Jumpstarter Config"
participant Logs as "Log Stream"
CLI->>Handler: RunFlash(target, client, flags)
Handler->>JS: Read & base64-encode client config
Handler->>Handler: Create FlashRequest
Handler->>API: CreateFlash(request)
API-->>Handler: FlashResponse (name)
loop wait / follow
Handler->>API: GetFlash(name)
API-->>Handler: FlashStatus
alt Running
Handler->>Logs: Open log stream
Logs-->>Handler: Stream lines
end
end
Handler->>CLI: Display flash instructions (placeholders substituted)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (9)
cmd/caib/common/api_client.go (1)
177-179: Add a timeout aroundoc whoami -tfallback.At Line 178, the external command runs without a timeout and can block indefinitely in some environments.
Proposed fix
import ( "context" "fmt" "os" "os/exec" "strings" + "time" ... ) ... if path, lookErr := exec.LookPath("oc"); lookErr == nil && path != "" { - out, cmdErr := exec.Command(path, "whoami", "-t").Output() + cmdCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + out, cmdErr := exec.CommandContext(cmdCtx, path, "whoami", "-t").Output() if cmdErr == nil { if t := strings.TrimSpace(string(out)); t != "" { return t, nil } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/api_client.go` around lines 177 - 179, The oc fallback uses exec.LookPath and exec.Command(path, "whoami", "-t").Output() with no timeout, which can hang; modify the branch that detects oc (the exec.LookPath(...) block) to use a context with timeout (e.g., context.WithTimeout) and exec.CommandContext instead of exec.Command so the whoami -t invocation is cancelled after a short timeout, handle the context deadline error and treat it like cmdErr so the code falls back cleanly.cmd/caib/common/manifest_artifact_helpers.go (1)
27-29: Use cleaned path-segment checks instead of rawstrings.Contains("..").At Line 27, this can reject benign names (for example
file..bak) and is not a robust traversal check.Proposed refactor
- if strings.Contains(path, "..") { - return fmt.Errorf("directory traversal detected in path: %s", path) - } + clean := filepath.Clean(path) + if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return fmt.Errorf("directory traversal detected in path: %s", path) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/manifest_artifact_helpers.go` around lines 27 - 29, Replace the brittle strings.Contains(path, "..") check with a segment-based check: normalize the separators (e.g. use filepath.ToSlash or string(os.PathSeparator)), split the path into segments and reject only if any segment equals ".." (e.g. for _, seg := range strings.Split(normalizedPath, "/") { if seg == ".." { return fmt.Errorf(...)} }), optionally also call filepath.Clean for other normalizations elsewhere; update the check around the strings.Contains(path, "..") usage so names like "file..bak" are allowed but actual directory-traversal segments are rejected.cmd/caib/buildcmd/build.go (1)
85-91:handleErrorcallspanicas fallback, which may not be appropriate for CLI tools.If
HandleErroris nil, the function panics. While this may be intentional to catch configuration errors during development, it could lead to poor user experience if triggered in production. Consider a more graceful fallback likeos.Exit(1)with an error message.♻️ Proposed fix
func (h *Handler) handleError(err error) { if h.opts.HandleError != nil { h.opts.HandleError(err) return } - panic(err) + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 85 - 91, The handleError function currently panics when h.opts.HandleError is nil; change this fallback to a graceful CLI exit by printing the error to stderr (e.g., fmt.Fprintln(os.Stderr, err) or a contextual message) and calling os.Exit(1) instead of panic; update the Handler.handleError implementation to detect nil h.opts.HandleError, output the error to stderr, and call os.Exit(1) so CLI runs fail gracefully.cmd/caib/flashcmd/flash.go (4)
124-146:parseLeaseDurationsilently returns default on parse failure.The function returns a 1-hour default for invalid input without any warning to the user. This could lead to unexpected behavior if the user provides a malformed duration string. Consider logging a warning or returning an error.
♻️ Proposed approach
-func parseLeaseDuration(duration string) time.Duration { +func parseLeaseDuration(duration string) (time.Duration, error) { parts := strings.Split(duration, ":") if len(parts) != 3 { - return time.Hour + return 0, fmt.Errorf("invalid duration format %q: expected HH:MM:SS", duration) } // ... rest of parsing with similar error returns }Alternatively, if keeping the silent default is intentional for backward compatibility, add a comment explaining this design choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/flashcmd/flash.go` around lines 124 - 146, The parseLeaseDuration function currently swallows parse errors and returns a 1-hour default; change it to return (time.Duration, error) instead of silently defaulting so callers can handle invalid input: update func parseLeaseDuration(duration string) to return (time.Duration, error), return a descriptive error when parts length != 3, when fmt.Sscanf fails for any component, or when values are out of range, and keep a successful time.Duration return on valid input; update any callers of parseLeaseDuration to handle the error (or, if you prefer silent fallback, explicitly log a warning inside parseLeaseDuration before returning the default and add a comment explaining the backward-compatibility decision).
253-313: Significant code duplication withlogs.go.The
logStreamStatestruct and its methods (canRetry,reset),streamLogsToStdout, andhandleLogStreamErrorare nearly identical to the implementations incmd/caib/buildcmd/logs.go. Consider extracting these into a shared package (e.g.,cmd/caib/commonor a newcmd/caib/logstreampackage) to reduce maintenance burden and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/flashcmd/flash.go` around lines 253 - 313, Extract the duplicated log streaming logic into a single shared package (e.g., logstream) and replace the copies in both flash.go and logs.go with imports/calls to that package: move the logStreamState type and its methods (canRetry, reset), plus the functions streamLogsToStdout and handleLogStreamError, into the new package, export any identifiers that need to be visible (e.g., LogStreamState, StreamLogsToStdout, HandleLogStreamError) or provide thin wrappers if you want to keep unexported names, update both flash.go and logs.go to import and call the shared functions/types, and run go fmt/build to ensure no breakage and consistent behavior across both callers.
161-168: SetMinVersionon TLS config to enforce modern TLS.Same issue as in
logs.go- the TLS configuration should include a minimum version for defense-in-depth.🔒 Proposed fix
flashLogTransport := &http.Transport{ ResponseHeaderTimeout: 30 * time.Second, IdleConnTimeout: 2 * time.Minute, } if *h.opts.InsecureSkipTLS { - flashLogTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + flashLogTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/flashcmd/flash.go` around lines 161 - 168, The TLS config for the http.Transport (flashLogTransport) must enforce a minimum TLS version; when setting flashLogTransport.TLSClientConfig in flash.go (inside the if *h.opts.InsecureSkipTLS block) ensure you initialize TLSClientConfig if nil and set TLSClientConfig.MinVersion = tls.VersionTLS12 (or a project-standard >= TLS1.2) in addition to InsecureSkipVerify so the transport enforces a modern minimum TLS version.
232-232: Ignoring error fromhttp.NewRequestWithContext.Same issue as in
logs.go- the error should be handled explicitly.♻️ Proposed fix
-req, _ := http.NewRequestWithContext(ctx, http.MethodGet, logURL, nil) +req, err := http.NewRequestWithContext(ctx, http.MethodGet, logURL, nil) +if err != nil { + return fmt.Errorf("failed to create log request: %w", err) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/flashcmd/flash.go` at line 232, The call to http.NewRequestWithContext is ignoring its error return; update the code around the req creation (the http.NewRequestWithContext(ctx, http.MethodGet, logURL, nil) invocation that assigns to req) to capture the error, check it, and handle it (e.g., log the error and return it or propagate it) instead of discarding it—ensure you use the same ctx, logURL and follow the existing error-handling pattern in this file (returning an error from the enclosing function or calling the same logger used elsewhere).cmd/caib/buildcmd/logs.go (2)
240-240: Ignoring error fromhttp.NewRequestWithContextcould mask issues.While the error is typically
nilfor valid HTTP methods and URLs, ignoring it entirely could mask bugs if the URL construction logic changes. Consider handling the error explicitly.♻️ Proposed fix
-req, _ := http.NewRequestWithContext(ctx, http.MethodGet, logURL, nil) +req, err := http.NewRequestWithContext(ctx, http.MethodGet, logURL, nil) +if err != nil { + return fmt.Errorf("failed to create log request: %w", err) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/logs.go` at line 240, The call to http.NewRequestWithContext currently ignores its returned error (req, _ := http.NewRequestWithContext(ctx, http.MethodGet, logURL, nil)); update this to capture the error (req, err := http.NewRequestWithContext(...)), handle it immediately (e.g., return the error upstream or log and wrap it) so URL/ctx construction failures aren't silently ignored, and keep using req on success; refer to the variables req, err, ctx and logURL to find and replace the current statement in the function where the request is created.
36-48: SetMinVersionon TLS config to enforce modern TLS.The static analysis correctly flagged that
MinVersionis missing from the TLS configuration. Even whenInsecureSkipVerifyis used (typically for development/testing), setting a minimum TLS version is a defense-in-depth measure. The same issue exists at line 362-363.🔒 Proposed fix to add MinVersion
logTransport := &http.Transport{ ResponseHeaderTimeout: 30 * time.Second, IdleConnTimeout: 2 * time.Minute, } if *h.opts.InsecureSkipTLS { - logTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + logTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + } }Apply the same fix at lines 359-364:
logTransport := &http.Transport{ ResponseHeaderTimeout: 30 * time.Second, } if *h.opts.InsecureSkipTLS { - logTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + logTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/logs.go` around lines 36 - 48, The TLSClientConfig created when *h.opts.InsecureSkipTLS is true should explicitly set a minimum TLS version for defense-in-depth; update the logTransport TLSClientConfig (created in the log setup where logTransport is defined) to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you require only 1.3) in addition to InsecureSkipVerify, and apply the identical change to the other TLSClientConfig instance that follows the same pattern later in the file (the other transport/TLSClientConfig block flagged by static analysis).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 639-657: The loop checks readyCtx.Err() but doesn't stop execution
on timeout; update the loop in buildReady wait logic so that when readyCtx.Err()
!= nil you call h.handleError(fmt.Errorf("timed out waiting for upload server to
be ready")) and immediately return (or break out of the surrounding function)
instead of continuing to create requests or sleeping; ensure you still call
reqCancel for each api.GetBuild call and keep using api.GetBuild(reqCtx,
buildName) and the existing checks on st.Phase (phaseUploading / phaseFailed and
st.Message) unchanged.
- Around line 164-166: The call to ResolveRegistryCredentials error is passed to
h.handleError(err) but execution may continue and leave req.RegistryCredentials
set from a failed resolution; update the surrounding function (in build.go) to
stop execution on error—either make handleError terminate or (preferably) change
the caller to return the error after calling h.handleError(err) (or refactor
ResolveRegistryCredentials to return an error up the stack) so that after
calling ResolveRegistryCredentials you either return the error immediately
instead of continuing and avoid using req.RegistryCredentials when resolution
failed.
- Around line 109-140: The validateBootcBuildFlags function currently calls
h.handleError(...) and continues execution; change its signature to return error
(func (h *Handler) validateBootcBuildFlags() error) and replace each
h.handleError(err) call with an immediate return of that error (e.g., return
fmt.Errorf(...) or return err) for all validation branches (server URL check,
internal registry/export conflict, output/push validation, container push
requirement). Update callers of validateBootcBuildFlags (the same context that
calls RunFlash) to check the returned error and propagate or handle it (similar
to RunFlash's explicit returns) instead of relying on h.handleError
side-effects. Ensure all unique symbols referenced are adjusted:
validateBootcBuildFlags, Handler, and any caller(s) that invoke
validateBootcBuildFlags.
In `@cmd/caib/buildcmd/logs.go`:
- Around line 338-350: RunLogs currently calls h.handleError on missing server
or after CreateBuildAPIClient error but does not return, which can let execution
continue and cause nil dereferences on variables like api (similar to RunFlash's
pattern); update RunLogs to immediately return after each h.handleError
invocation (both after the empty server check and after the CreateBuildAPIClient
error) so execution stops if handleError does not terminate the program.
In `@cmd/caib/cli_helpers.go`:
- Around line 36-38: The handleError function prints error messages to stdout;
change it to write to stderr by using os.Stderr as the writer for the error
output (replace fmt.Printf with fmt.Fprintf targeting os.Stderr) in the
handleError(err error) function so fatal errors go to stderr and then exit as
before.
- Around line 13-20: The switch currently falls back to archAMD64 for unknown
runtime.GOARCH which silently picks the wrong target; change the default case to
return runtime.GOARCH (or an explicit "unknown" value) and emit a warning/error
so callers know the host arch was unrecognized; update the switch that
references runtime.GOARCH and the constants archAMD64 and archARM64 to use
runtime.GOARCH in default (and/or return an error value) instead of returning
archAMD64, and add a log/process warning call so the unexpected architecture is
visible.
In `@cmd/caib/common/api_client.go`:
- Around line 17-19: The exported helper functions CreateBuildAPIClient and
CreateDeployAPIClient currently dereference the authToken pointer (e.g.
*authToken) without nil checks; add a guard to safely handle nil inputs by first
deriving a local token string (e.g. tokenStr := "" ; if authToken != nil {
tokenStr = strings.TrimSpace(*authToken) }) and then use tokenStr for subsequent
checks (instead of *authToken) and env fallback (os.Getenv("CAIB_TOKEN")),
ensuring no direct dereference occurs anywhere in CreateBuildAPIClient or
CreateDeployAPIClient.
- Around line 19-43: The code sets explicitToken based on presence of CAIB_TOKEN
but never assigns the env value to *authToken, so the env token is ignored;
modify the auth logic so when explicitToken is true and
strings.TrimSpace(*authToken) == "" you read os.Getenv("CAIB_TOKEN"), trim it
and assign it to *authToken (falling back to LoadTokenFromKubeconfig only if
CAIB_TOKEN is empty), and ensure this behavior complements the existing
GetTokenWithReauth and LoadTokenFromKubeconfig branches (reference
explicitToken, authToken, CAIB_TOKEN, GetTokenWithReauth,
LoadTokenFromKubeconfig, serverURL, insecureSkipTLS).
In `@cmd/caib/common/build_validation.go`:
- Around line 31-35: ValidateBuildName currently only checks that
SanitizeBuildName(name) is non-empty; add a max-length enforcement by computing
s := SanitizeBuildName(name) and returning an error if len(s) >
MAX_BUILD_NAME_LEN (introduce a clear constant like MAX_BUILD_NAME_LEN near
other constants). Update ValidateBuildName to return a descriptive fmt.Errorf
when the sanitized name exceeds the limit (include the actual length and the
allowed max) and keep the existing empty-after-sanitize check; reference the
function names ValidateBuildName and SanitizeBuildName when making the changes.
In `@cmd/caib/common/manifest_artifact_helpers.go`:
- Around line 31-42: The current safe-directory check in
manifest_artifact_helpers.go uses an empty hardcoded slice safeDirectories which
rejects all absolute paths; change the logic so that if safeDirectories is empty
it does not block absolute paths (i.e., skip the isInSafeDir validation) or wire
safeDirectories to a real source (CLI/config) before this check; update the
block around safeDirectories/isInSafeDir/path to either populate safeDirectories
from the existing config/flag (preferred) or treat an empty safeDirectories as
"no restriction" so absolute source_path values are allowed.
In `@cmd/caib/common/oci_artifact.go`:
- Around line 157-205: The code currently only treats manifests as multi-layer
when the annotation is present, but if manifest.Layers has more than one entry
and the annotation is missing the code falls back to extracting only
manifest.Layers[0], potentially dropping data; update the logic around
isMultiLayer to also detect len(manifest.Layers) > 1 (or explicitly error) and
route such manifests through the multi-layer extraction path that uses
sanitizeFilename, seenFilenames, copyFile, ociLayoutPath and destPath (or fail
fast with a clear error message) so all layers are handled consistently even
when the annotation is absent.
In `@cmd/caib/downloadcmd/download.go`:
- Around line 51-97: The RunDownload function calls h.handleError in several
failure branches but continues execution, so change those branches to return
immediately after invoking h.handleError to avoid proceeding in an invalid
state; specifically, after each h.handleError call in RunDownload (the checks
for empty ServerURL/OutputDir, the ExecuteWithReauth error handling, the
st.Phase check, the empty ociRef check, registry credential validation failure,
and the PullOCIArtifact error branch) add an immediate return to stop further
processing when the handler does not terminate the program.
In `@cmd/caib/login.go`:
- Around line 15-18: The code reads args[0] without verifying args length, which
can panic when no positional arguments are provided; update the command handler
(the function that processes args in cmd/caib/login.go) to first check len(args)
== 0 and call handleError(fmt.Errorf("server URL is required")) (or return an
appropriate error/usage) before accessing args[0], then proceed to use
strings.TrimSpace(raw) and the existing logic; locate the argument handling in
the login command entrypoint where raw := strings.TrimSpace(args[0]) is defined
and add the guard prior to that line.
- Around line 20-24: The code currently just prepends a scheme to server and
calls config.SaveServerURL(server), which can persist malformed URLs; before
calling SaveServerURL in cmd/caib/login.go, validate the normalized server
string using net/url parsing (e.g., url.Parse or url.ParseRequestURI) and ensure
the parsed URL has a non-empty Scheme and Host (and no illegal characters); if
validation fails, return or call handleError with a clear message instead of
saving; update the block that modifies server and calls
config.SaveServerURL(server) to perform this validation step and only persist
when the URL is valid.
In `@cmd/caib/querycmd/query.go`:
- Around line 49-147: The handlers RunList and RunShow call h.handleError in
multiple failure paths but continue execution; update both functions so each
place that invokes h.handleError is immediately followed by a return to stop
further processing (e.g., after the server URL empty checks, after the
ExecuteWithReauth error handling blocks, and after JSON/YAML marshal error
handling and the invalid output-format branch in RunShow), ensuring no code runs
after h.handleError is called.
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 278-311: The sealedStreamLogs method currently ignores NewRequest
errors, ignores the Handler TLS option, and drops scanner errors; fix it by (1)
checking and returning the error from http.NewRequestWithContext in
sealedStreamLogs, (2) constructing http.Client with a Transport whose
TLSClientConfig.InsecureSkipVerify is set from *h.opts.InsecureSkipTLS (same
pattern as buildcmd/container/flashcmd) and keeping the 10m Timeout, and (3)
after scanning, if scanner.Err() is non-nil return that error (rather than
discarding it); reference the sealedStreamLogs function, the Handler type and
its opts (ServerURL, AuthToken, InsecureSkipTLS), the httpClient
variable/transport creation, and the scanner.Err() call to locate and apply the
changes.
---
Nitpick comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 85-91: The handleError function currently panics when
h.opts.HandleError is nil; change this fallback to a graceful CLI exit by
printing the error to stderr (e.g., fmt.Fprintln(os.Stderr, err) or a contextual
message) and calling os.Exit(1) instead of panic; update the Handler.handleError
implementation to detect nil h.opts.HandleError, output the error to stderr, and
call os.Exit(1) so CLI runs fail gracefully.
In `@cmd/caib/buildcmd/logs.go`:
- Line 240: The call to http.NewRequestWithContext currently ignores its
returned error (req, _ := http.NewRequestWithContext(ctx, http.MethodGet,
logURL, nil)); update this to capture the error (req, err :=
http.NewRequestWithContext(...)), handle it immediately (e.g., return the error
upstream or log and wrap it) so URL/ctx construction failures aren't silently
ignored, and keep using req on success; refer to the variables req, err, ctx and
logURL to find and replace the current statement in the function where the
request is created.
- Around line 36-48: The TLSClientConfig created when *h.opts.InsecureSkipTLS is
true should explicitly set a minimum TLS version for defense-in-depth; update
the logTransport TLSClientConfig (created in the log setup where logTransport is
defined) to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you
require only 1.3) in addition to InsecureSkipVerify, and apply the identical
change to the other TLSClientConfig instance that follows the same pattern later
in the file (the other transport/TLSClientConfig block flagged by static
analysis).
In `@cmd/caib/common/api_client.go`:
- Around line 177-179: The oc fallback uses exec.LookPath and exec.Command(path,
"whoami", "-t").Output() with no timeout, which can hang; modify the branch that
detects oc (the exec.LookPath(...) block) to use a context with timeout (e.g.,
context.WithTimeout) and exec.CommandContext instead of exec.Command so the
whoami -t invocation is cancelled after a short timeout, handle the context
deadline error and treat it like cmdErr so the code falls back cleanly.
In `@cmd/caib/common/manifest_artifact_helpers.go`:
- Around line 27-29: Replace the brittle strings.Contains(path, "..") check with
a segment-based check: normalize the separators (e.g. use filepath.ToSlash or
string(os.PathSeparator)), split the path into segments and reject only if any
segment equals ".." (e.g. for _, seg := range strings.Split(normalizedPath, "/")
{ if seg == ".." { return fmt.Errorf(...)} }), optionally also call
filepath.Clean for other normalizations elsewhere; update the check around the
strings.Contains(path, "..") usage so names like "file..bak" are allowed but
actual directory-traversal segments are rejected.
In `@cmd/caib/flashcmd/flash.go`:
- Around line 124-146: The parseLeaseDuration function currently swallows parse
errors and returns a 1-hour default; change it to return (time.Duration, error)
instead of silently defaulting so callers can handle invalid input: update func
parseLeaseDuration(duration string) to return (time.Duration, error), return a
descriptive error when parts length != 3, when fmt.Sscanf fails for any
component, or when values are out of range, and keep a successful time.Duration
return on valid input; update any callers of parseLeaseDuration to handle the
error (or, if you prefer silent fallback, explicitly log a warning inside
parseLeaseDuration before returning the default and add a comment explaining the
backward-compatibility decision).
- Around line 253-313: Extract the duplicated log streaming logic into a single
shared package (e.g., logstream) and replace the copies in both flash.go and
logs.go with imports/calls to that package: move the logStreamState type and its
methods (canRetry, reset), plus the functions streamLogsToStdout and
handleLogStreamError, into the new package, export any identifiers that need to
be visible (e.g., LogStreamState, StreamLogsToStdout, HandleLogStreamError) or
provide thin wrappers if you want to keep unexported names, update both flash.go
and logs.go to import and call the shared functions/types, and run go fmt/build
to ensure no breakage and consistent behavior across both callers.
- Around line 161-168: The TLS config for the http.Transport (flashLogTransport)
must enforce a minimum TLS version; when setting
flashLogTransport.TLSClientConfig in flash.go (inside the if
*h.opts.InsecureSkipTLS block) ensure you initialize TLSClientConfig if nil and
set TLSClientConfig.MinVersion = tls.VersionTLS12 (or a project-standard >=
TLS1.2) in addition to InsecureSkipVerify so the transport enforces a modern
minimum TLS version.
- Line 232: The call to http.NewRequestWithContext is ignoring its error return;
update the code around the req creation (the http.NewRequestWithContext(ctx,
http.MethodGet, logURL, nil) invocation that assigns to req) to capture the
error, check it, and handle it (e.g., log the error and return it or propagate
it) instead of discarding it—ensure you use the same ctx, logURL and follow the
existing error-handling pattern in this file (returning an error from the
enclosing function or calling the same logger used elsewhere).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
Makefilecmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/login.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
cmd/caib/buildcmd/build.go (1)
182-190:⚠️ Potential issue | 🟠 MajorReturn immediately after flash-validation
handleErrorcalls infetchTargetDefaults.If
HandleErrorcallback returns normally,fetchTargetDefaultscontinues after fatal conditions. On Line 193, this can become a nil-dereference path whenconfigfetch failed.Proposed fix
config, err := api.GetOperatorConfig(ctx) if err != nil { // Non-fatal for defaults: if we can't reach the config endpoint, just skip defaults. if !validateFlash { fmt.Fprintf(os.Stderr, "Warning: could not fetch operator config for target defaults: %v\n", err) return nil } h.handleError(fmt.Errorf("failed to get operator configuration for Jumpstarter validation: %w", err)) + return nil } if validateFlash { if len(config.JumpstarterTargets) == 0 { h.handleError(fmt.Errorf("flash enabled but no Jumpstarter target mappings configured in operator")) + return nil } @@ h.handleError( fmt.Errorf( "flash enabled but no Jumpstarter target mapping found for target %q. Available targets: %v", target, availableTargets, ), ) + return nil } }Also applies to: 193-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 182 - 190, fetchTargetDefaults currently continues execution after calling h.handleError on fatal config fetch paths, risking nil dereferences (see the h.handleError(fmt.Errorf(...)) calls); immediately return from the function after each fatal h.handleError invocation (e.g., add "return nil" right after the h.handleError call) so execution does not proceed with a nil config; apply the same immediate return change for all h.handleError calls in the fetchTargetDefaults control paths (the ones handling operator config fetch failures and similar validation errors).
🧹 Nitpick comments (1)
cmd/caib/logstream/logstream.go (1)
24-29: Fix off-by-one in retry limit check.Line 28 uses
<=, which permitsmaxRetries + 1attempts whenRetryCountstarts at 0.🔧 Proposed fix
func (s *State) CanRetry(maxRetries int) bool { if s == nil { return false } - return s.RetryCount <= maxRetries && !s.Completed + return s.RetryCount < maxRetries && !s.Completed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/logstream/logstream.go` around lines 24 - 29, The CanRetry method currently allows one too many attempts because it uses <=; change the comparison in State.CanRetry to use < instead of <= so that RetryCount (which starts at 0) is strictly less than maxRetries, and keep the nil check and Completed check intact (i.e., return s.RetryCount < maxRetries && !s.Completed in the CanRetry method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/buildcmd/logs.go`:
- Around line 61-63: The wait loop is creating child request contexts from the
original ctx (reqCtx := context.WithTimeout(ctx, ...)) so in-flight requests
won't be canceled when the overall timeout (timeoutCtx) expires; update all
child context creations inside the wait loop and related requests to derive from
timeoutCtx (e.g., replace context.WithTimeout(ctx, ...) with
context.WithTimeout(timeoutCtx, ...) where reqCtx and cancelReq are created
before calling api.GetBuild and similar RPCs such as the other calls in the loop
and at the later request site), and ensure each cancelReq() is still called to
avoid leaks.
In `@cmd/caib/common/api_client.go`:
- Around line 38-39: Change the user-visible status/error prints from stdout to
stderr: replace fmt.Printf/ fmt.Println calls shown (e.g., fmt.Printf("Error:
OIDC authentication failed: %v\n", err) and fmt.Println("Attempting kubeconfig
fallback (this may use a different identity)")) with writes to os.Stderr (use
fmt.Fprintf(os.Stderr, "...%v\n", err) or fmt.Fprintln(os.Stderr, "...") as
appropriate) so all diagnostic messages in cmd/caib/common/api_client.go (the
occurrences at lines referenced: the fmt.Printf/Println calls at the six
locations) go to stderr and do not interfere with stdout JSON/YAML output.
In `@cmd/caib/common/oci_artifact.go`:
- Around line 47-49: The code currently unconditionally sets signature
verification to insecure by using signature.NewPRInsecureAcceptAnything();
modify the logic in the OCI artifact pull setup (where signature.Policy is
constructed) to select the policy based on a configurable flag instead of
hardcoding: add a boolean parameter (or read an env var such as SIGNATURE_VERIFY
or a passed-in policy path) and when verification is enabled build/load a strict
policy (e.g., load system signature policy or use the default/enforced signature
requirements) and when disabled fall back to
signature.NewPRInsecureAcceptAnything(); update the function that accepts
insecureSkipTLS to also accept or derive this signature-verification flag and
use signature.Policy only after branching on that flag (referencing
signature.Policy, signature.NewPRInsecureAcceptAnything(), and the
insecureSkipTLS parameter to locate the code to change).
In `@cmd/caib/flashcmd/flash.go`:
- Around line 185-186: The status/log requests are using the ambient ctx instead
of the timeout context, so replace usages of ctx with timeoutCtx when creating
per-request contexts and when calling API methods (e.g., in the reqCtx creation
and the api.GetFlash call and any subsequent status/log calls around lines where
api.GetFlash, GetFlashStatus/GetFlashLogs are invoked); ensure you use
timeoutCtx as the parent for context.WithTimeout (keeping reqCtx and cancelReq)
and defer cancelReq so the configured timeout actually cancels long-running
streams.
- Around line 66-71: RunFlash currently reads args[0] into imageRef without
checking args length which can panic; add a guard at the start of RunFlash to
verify len(args) > 0 and handle the error path (e.g., call cmd.Usage() or print
an error and return/exit) before accessing args[0]; update any callers or tests
if they rely on the previous behavior. Ensure the check is placed before the
line that sets imageRef and reference the RunFlash method and the args/imageRef
variables when making the change.
- Around line 131-138: The parsing of lease duration segments using fmt.Sscanf
on parts[0], parts[1], parts[2] (assigning to hours, mins, secs) accepts
malformed tokens like "1x"; replace each fmt.Sscanf(...) call with strict
integer parsing via strconv.Atoi on the corresponding parts[i], return an error
if Atoi fails, and keep the same error messages (e.g., "invalid lease duration
hours %q") so malformed inputs are rejected and errors remain clear; update
imports to include strconv if needed.
In `@cmd/caib/querycmd/query.go`:
- Around line 102-105: The RunShow method on Handler dereferences args[0]
without checking args length; add a guard at the start of Handler.RunShow that
verifies len(args) > 0 and handles the error case (e.g., print usage/error or
return) before accessing args[0]; update any early returns to use the same
behavior so RunShow never panics when args is empty.
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 99-105: The call sites invoke h.opts.HandleError directly which
can panic if the callback is nil; update RunPrepareReseal (and the other
affected methods) to guard before calling by checking if h.opts != nil &&
h.opts.HandleError != nil, and if present call h.opts.HandleError(err) otherwise
fall back to a safe alternative such as logging the error or returning/printing
it; apply this same nil-guard pattern to every occurrence of
h.opts.HandleError(...) in this file (e.g., in RunPrepareReseal, and the ranges
around lines handling resolveSealedTwoRefs, subsequent error branches, and the
other listed call sites).
- Around line 259-269: The bug is that logRetryWarningShown is set on the first
failed attempt so the "retries exhausted" branch never runs; update the logic so
the initial "Waiting for logs..." message does not set logRetryWarningShown, and
only set logRetryWarningShown when you actually fall back to status-only mode.
Specifically, in the block that prints "Waiting for logs... (attempt %d/%d)"
leave logRetryWarningShown false, and in the branch where you detect retries are
exhausted (the code that sets *h.opts.FollowLogs = false and prints "Log
streaming failed... Falling back...") set logRetryWarningShown = true and set
logStreaming = false as needed; use the existing symbols logRetryWarningShown,
logRetries, maxSealedLogRetries, logStreaming, and h.opts.FollowLogs to locate
and implement the change.
---
Duplicate comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 182-190: fetchTargetDefaults currently continues execution after
calling h.handleError on fatal config fetch paths, risking nil dereferences (see
the h.handleError(fmt.Errorf(...)) calls); immediately return from the function
after each fatal h.handleError invocation (e.g., add "return nil" right after
the h.handleError call) so execution does not proceed with a nil config; apply
the same immediate return change for all h.handleError calls in the
fetchTargetDefaults control paths (the ones handling operator config fetch
failures and similar validation errors).
---
Nitpick comments:
In `@cmd/caib/logstream/logstream.go`:
- Around line 24-29: The CanRetry method currently allows one too many attempts
because it uses <=; change the comparison in State.CanRetry to use < instead of
<= so that RetryCount (which starts at 0) is strictly less than maxRetries, and
keep the nil check and Completed check intact (i.e., return s.RetryCount <
maxRetries && !s.Completed in the CanRetry method).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/login.gocmd/caib/logstream/logstream.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/caib/root.go
- cmd/caib/build_validation.go
- cmd/caib/login.go
- cmd/caib/common/cli_helpers.go
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
cmd/caib/logstream/logstream.go (1)
93-113: Ensure caller closesresp.Bodybefore callingHandleLogStreamError.
HandleLogStreamErrorreads the entire response body (Line 95) but does not close it. While the caller typically manages the response lifecycle, this function consumes the body, so consider documenting that the caller must closeresp.Bodyafter this function returns, or close it here after reading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/logstream/logstream.go` around lines 93 - 113, HandleLogStreamError reads resp.Body but doesn't close it; update HandleLogStreamError to close the response body after reading (e.g., ensure resp.Body.Close() is called after io.ReadAll), handle any read error returned by io.ReadAll instead of ignoring it, and leave the State and logging behavior unchanged; reference resp.Body and the HandleLogStreamError function when making the change so callers no longer must close the body themselves.cmd/caib/common/api_client.go (1)
78-166: Consider simplifying the retry logic inExecuteWithReauth.The function has multiple client recreation paths and token refresh attempts that make the flow complex. While functional, the nested retry logic (Lines 120-133 and 152-163) could be consolidated. This is not blocking, but a future refactor could improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/api_client.go` around lines 78 - 166, The ExecuteWithReauth function's retry logic is duplicated and can be simplified by consolidating client recreation and token fallback into a single retry path: after initial fn(client) failure and an auth.IsAuthError, call auth.GetTokenWithReauth(ctx, serverURL, currentToken, insecureSkipTLS) once, setToken(newToken), then attempt a single unified retry sequence that (1) recreates the client via CreateBuildAPIClient(serverURL, authToken, insecureSkipTLS), (2) runs fn(client), and if still an auth error, performs one kubeconfig fallback by calling LoadTokenFromKubeconfig(), setToken(tok) if present, recreates the client and calls fn(client) one final time; remove the duplicated client recreation blocks and repeated currentToken assignments so ExecuteWithReauth, CreateBuildAPIClient, auth.GetTokenWithReauth, LoadTokenFromKubeconfig, setToken, and fn are only invoked in this single clear flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 425-427: The call to h.handleFileUploads currently ignores
failures because handleFileUploads swallows errors; change handleFileUploads to
return an error (func handleFileUploads(...) error) and update all call sites
(including the h.handleFileUploads(ctx, api, resp.Name, localRefs) occurrences
and the other places flagged in the diff) to check the returned error and
propagate or return it (or invoke HandleError in a terminating fashion) so that
upload failures halt further processing; ensure callers return or exit on
non-nil error and update any upstream functions to propagate the error as
needed.
- Around line 713-716: The upload entries are using the host-local source path
as the remote destination; update the construction of the uploads slice
(buildapiclient.Upload entries created in the loop over localRefs) to use the
manifest destination key (ref["path"]) for DestPath while keeping SourcePath as
ref["source_path"], so remote mapping uses the manifest path instead of the host
path.
- Around line 324-329: The entrypoints RunBuild, RunDisk, and RunBuildDev access
args[0] without checking args length, which will panic when no positional
arguments are provided; add an explicit guard at the start of each function
(e.g., in Handler.RunBuild, Handler.RunDisk, Handler.RunBuildDev) that checks if
len(args) == 0 and if so prints a clear error to the command (cmd.PrintErr or
cmd.PrintErrf), calls cmd.Usage() or cmd.Help() and returns early (or exits)
instead of accessing args[0]; update the code paths that set *h.opts.Manifest
from args[0] to only run after this validation.
In `@cmd/caib/flashcmd/flash.go`:
- Around line 44-45: Add a nil-safe wrapper around the optional callback so
calls won't panic: implement a handleError method on the flash handler (or a
small helper function) that checks whether h.opts is non-nil and
h.opts.HandleError is non-nil before invoking it, and update all sites that call
h.opts.HandleError(...) (including RunFlash and the other 9 call sites) to call
handleError(err) instead of calling h.opts.HandleError directly.
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 64-70: The default error path in Handler.handleError currently
only writes to stderr and returns, so command failures do not produce a non-zero
exit; update handleError to call os.Exit with a non-zero code (e.g., os.Exit(1))
after printing the error when h.opts.HandleError is nil so the process
terminates with failure; leave the behavior unchanged when h != nil &&
h.opts.HandleError != nil and invoke that callback as before.
- Around line 262-265: The log streamer is using context.Background() and can
outlive the wait loop; update sealedStreamLogs to accept a context.Context
(change signature of sealedStreamLogs to sealedStreamLogs(ctx context.Context,
op ..., name ...)) and use that ctx internally instead of context.Background(),
then in sealedWaitForCompletion pass the wait-loop context (the ctx variable) to
the call (replace h.sealedStreamLogs(op, name) with h.sealedStreamLogs(ctx, op,
name)); update any other callers of sealedStreamLogs accordingly so log
streaming is cancelled when the wait loop times out or is cancelled.
---
Nitpick comments:
In `@cmd/caib/common/api_client.go`:
- Around line 78-166: The ExecuteWithReauth function's retry logic is duplicated
and can be simplified by consolidating client recreation and token fallback into
a single retry path: after initial fn(client) failure and an auth.IsAuthError,
call auth.GetTokenWithReauth(ctx, serverURL, currentToken, insecureSkipTLS)
once, setToken(newToken), then attempt a single unified retry sequence that (1)
recreates the client via CreateBuildAPIClient(serverURL, authToken,
insecureSkipTLS), (2) runs fn(client), and if still an auth error, performs one
kubeconfig fallback by calling LoadTokenFromKubeconfig(), setToken(tok) if
present, recreates the client and calls fn(client) one final time; remove the
duplicated client recreation blocks and repeated currentToken assignments so
ExecuteWithReauth, CreateBuildAPIClient, auth.GetTokenWithReauth,
LoadTokenFromKubeconfig, setToken, and fn are only invoked in this single clear
flow.
In `@cmd/caib/logstream/logstream.go`:
- Around line 93-113: HandleLogStreamError reads resp.Body but doesn't close it;
update HandleLogStreamError to close the response body after reading (e.g.,
ensure resp.Body.Close() is called after io.ReadAll), handle any read error
returned by io.ReadAll instead of ignoring it, and leave the State and logging
behavior unchanged; reference resp.Body and the HandleLogStreamError function
when making the change so callers no longer must close the body themselves.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/login.gocmd/caib/logstream/logstream.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/caib/build_validation.go
- cmd/caib/common/build_validation.go
- cmd/caib/cli_helpers.go
- cmd/caib/runtime_wiring.go
- cmd/caib/common/manifest_artifact_helpers.go
- cmd/caib/buildcmd/flash_feedback.go
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
cmd/caib/buildcmd/build.go (1)
176-216:⚠️ Potential issue | 🟠 Major
fetchTargetDefaultsshould return errors instead of swallowing them viahandleError.At Line 189, Line 195, and Line 204, the function reports errors but returns
nilconfig, allowing callers to continue request construction. Return(*buildapitypes.OperatorConfigResponse, error)and abort in caller on non-nil error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 176 - 216, fetchTargetDefaults currently swallows errors via h.handleError and returns nil; change its signature to return (*buildapitypes.OperatorConfigResponse, error), propagate errors instead of calling h.handleError (wrap GetOperatorConfig errors returned from api.GetOperatorConfig, and return errors for missing/invalid JumpstarterTargets checks), and return (config, nil) on success; update any callers of fetchTargetDefaults to check the returned error and abort request construction when non-nil so callers stop using a nil config. Make sure to reference the function fetchTargetDefaults, the call to api.GetOperatorConfig, and the JumpstarterTargets existence checks when replacing h.handleError calls with returned errors.cmd/caib/buildcmd/logs.go (1)
23-212:⚠️ Potential issue | 🟠 MajorReturn an error from
waitForBuildCompletioninstead of only side-effectinghandleError.At Line 58 and Line 166, failures are handled internally, but callers cannot detect failure. If
Handler.opts.HandleErrorreturns, downstream code can still execute success-oriented flows (e.g., result display). Make this method returnerrorand let callers short-circuit explicitly.💡 Suggested refactor
-func (h *Handler) waitForBuildCompletion(ctx context.Context, api *buildapiclient.Client, name string) { +func (h *Handler) waitForBuildCompletion(ctx context.Context, api *buildapiclient.Client, name string) error { @@ - h.handleError(fmt.Errorf("timed out waiting for build")) - return + return fmt.Errorf("timed out waiting for build") @@ - h.handleFlashError(handleErr, st) + h.handleFlashError(handleErr, st) + return handleErr } else { - h.handleError(handleErr) + return handleErr } - return + @@ - return + return nil @@ -} +}Then in callers:
-h.waitForBuildCompletion(ctx, api, name) -h.displayBuildResults(ctx, api, name) +if err := h.waitForBuildCompletion(ctx, api, name); err != nil { + h.handleError(err) + return +} +h.displayBuildResults(ctx, api, name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/logs.go` around lines 23 - 212, The method waitForBuildCompletion currently returns nothing and only calls h.handleError on failures, so callers can't detect errors; change the signature of Handler.waitForBuildCompletion to return error and propagate failures by returning an appropriate error instead of only calling h.handleError (keep calling h.handleError for side-effects if you want but also return the error). Specifically, replace the timeout branch (where it calls h.handleError(fmt.Errorf("timed out waiting for build"))) to return that error, and in the build-failed branch (where it computes handleErr and calls h.handleFlashError or h.handleError) return handleErr (or the error returned by handleFlashError if it returns one); also return any error from tryLogStreaming up the call chain (or convert stream-related failures into returned errors where appropriate) so callers can short-circuit; update all call sites to check the returned error and act accordingly.cmd/caib/sealedcmd/sealed.go (1)
221-286:⚠️ Potential issue | 🟠 MajorSeal wait timeout is not enforced for in-flight API/log requests.
Line 239 and Line 265 use
ctxderived fromcontext.Background(). The loop deadline only gates iterations; it does not cancel blockedGetSealedor log-stream calls. Use a timeout-derived context for the whole wait operation and per-request subtimeouts.💡 Suggested fix
func (h *Handler) sealedWaitForCompletion( ctx context.Context, @@ ) { @@ - sealedTimeout := time.Duration(*h.opts.Timeout) * time.Minute - deadline := time.Now().Add(sealedTimeout) + sealedTimeout := time.Duration(*h.opts.Timeout) * time.Minute + timeoutCtx, cancel := context.WithTimeout(ctx, sealedTimeout) + defer cancel() @@ - for time.Now().Before(deadline) { - st, err := api.GetSealed(ctx, op, name) + for { + select { + case <-timeoutCtx.Done(): + h.handleError(fmt.Errorf("timed out after %v", sealedTimeout)) + return + default: + } + + reqCtx, reqCancel := context.WithTimeout(timeoutCtx, 15*time.Second) + st, err := api.GetSealed(reqCtx, op, name) + reqCancel() @@ - streamErr := h.sealedStreamLogs(ctx, op, name) + streamErr := h.sealedStreamLogs(timeoutCtx, op, name) @@ - h.handleError(fmt.Errorf("timed out after %v", sealedTimeout)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/sealedcmd/sealed.go` around lines 221 - 286, sealedWaitForCompletion currently only gates loop iterations with a deadline and lets long-running GetSealed and sealedStreamLogs calls block past that deadline; wrap the whole wait with a deadline/timeout context derived from the incoming ctx (e.g. ctx, cancel := context.WithDeadline(ctx, time.Now().Add(time.Duration(*h.opts.Timeout)*time.Minute))) and use that context for all API calls (api.GetSealed) and for log streaming; additionally, for each individual request create a short per-request sub-context with context.WithTimeout (and defer cancel) so single calls don’t hang indefinitely, and ensure sealedStreamLogs accepts/uses the passed ctx (call h.sealedStreamLogs(ctxReq, op, name)); cancel contexts properly to avoid leaks and check ctx.Done() before/after blocking calls to respect the overall timeout.
🧹 Nitpick comments (1)
cmd/caib/common/manifest_artifact_helpers.go (1)
35-38: Resolve the safe-directory TODO in this validation path.At Line [36], a TODO remains in the absolute-path safety branch. This policy should be explicitly wired (flag/config) or formally documented as unrestricted-by-default to avoid ambiguity.
If you want, I can draft a follow-up issue with a concrete safe-directories option shape and validation flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/manifest_artifact_helpers.go` around lines 35 - 38, The absolute-path validation branch currently has a TODO and an empty safeDirectories slice; update the logic so safe directories are sourced from configuration/CLI instead of being a hardcoded empty list: wire a safe-directories option into the manifest path validation (e.g., expose a flag or read from an injected config struct used by the code that calls filepath.IsAbs(path)), populate safeDirectories from that config, and ensure the branch explicitly documents/handles the default policy (either deny all absolute paths unless matched or allow-by-default with clear docs). Update the code references around filepath.IsAbs(path) and the safeDirectories variable so the validation uses the configured list and emits a clear error when an absolute path is not permitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 754-771: The retry loop currently calls api.UploadFiles(ctx,
buildName, uploads) with the parent ctx, allowing a stalled in-flight request to
outlive the 10-minute retry window; change the call so each attempt uses a
bounded child context (use context.WithTimeout) and ensure cancel() is called
after the attempt. Inside the loop create a per-attempt context (e.g.,
ctxAttempt, cancel := context.WithTimeout(ctx, perAttemptTimeout) or the
remaining time until uploadDeadline if smaller), pass ctxAttempt to
api.UploadFiles, call cancel() immediately after the call, and keep the existing
logic that checks uploadDeadline, isServiceUnavailable, and retries. Ensure you
reference uploadDeadline, api.UploadFiles, ctx, uploads, and buildName when
making this change.
In `@cmd/caib/buildcmd/logs.go`:
- Around line 93-95: The flash detection is case-sensitive: change the
assignment of flashWasExecuted (where it currently uses
strings.Contains(st.Message, "flash")) to a case-insensitive check by
normalizing st.Message (e.g., strings.Contains(strings.ToLower(st.Message),
"flash")) so messages like "Flash completed..." are detected; update only the
flashWasExecuted expression near the existing bannerColor closure in logs.go.
In `@cmd/caib/downloadcmd/download.go`:
- Around line 47-50: The RunDownload handler accesses args[0] without checking
length and can panic; update Handler.RunDownload to guard against empty args by
validating len(args)>0 before indexing (or using cobra's Args validation), and
if missing, return or log a user-friendly error/usage message; reference the
RunDownload method and the args slice so you add the check at the start of that
function and handle the missing build name gracefully instead of directly
indexing args[0].
In `@cmd/caib/flashcmd/flash.go`:
- Around line 202-207: The polling/logging requests (calls like api.GetFlash
inside the long-running loop using reqCtx and cancelReq) must be wrapped with
the existing re-auth helper so the token can refresh during long waits; replace
direct calls to api.GetFlash (and similar requests around the same area) with
common.ExecuteWithReauth that executes the API call closure using reqCtx and
returns the same st, err results, ensuring you still call cancelReq() after the
call and handle errors as before; reference the existing helper
common.ExecuteWithReauth and the variables reqCtx, cancelReq, api.GetFlash and
h.opts.AuthToken when locating and changing these call sites.
In `@cmd/caib/login.go`:
- Around line 31-38: Validate and sanitize the parsed URL before calling
config.SaveServerURL: after url.Parse(server) and the existing scheme/host
checks, reject URLs that contain embedded credentials (parsedURL.User != nil),
any query parameters (parsedURL.RawQuery != ""), any fragment
(parsedURL.Fragment != ""), or a non-root path (parsedURL.Path != "" &&
parsedURL.Path != "/") by calling handleError with an appropriate error; if the
URL passes these checks, reconstruct server to the base URL using only
parsedURL.Scheme + "://" + parsedURL.Host and then call
config.SaveServerURL(server) as before to persist the sanitized base URL.
In `@cmd/caib/logstream/logstream.go`:
- Around line 4-11: The error/warning messages currently printed to stdout
should be sent to stderr instead: update the print calls at the error paths (the
prints around the log stream loop noted at lines ~109-122) to write to os.Stderr
rather than the current stdout writer; specifically replace any
fmt.Fprintln/Printf/Print calls that emit errors or warnings in the
stream-handling function (the log stream loop in
cmd/caib/logstream/logstream.go) so they target os.Stderr (or use
fmt.Fprintln(os.Stderr, ...)) for the error cases at the three locations
mentioned.
- Around line 83-90: The code currently sets state.Completed = true on any clean
EOF which prevents future reconnects because CanRetry() checks !s.Completed;
change the logic in logstream.go so that when scanner.Err() == nil (a clean
EOF/interrupt) you do not set state.Completed = true—only set state.Active =
false and return nil; reserve setting state.Completed = true for
unrecoverable/terminal conditions (e.g., explicit "stream finished" signal or
non-recoverable error paths) so CanRetry() remains true after transient
disconnects (refer to state.Active, scanner.Err(), state.Completed, and
CanRetry()).
In `@cmd/caib/querycmd/query.go`:
- Around line 127-137: The fallback call to common.ExecuteWithReauth when
st.Parameters is nil currently ignores its returned error, so capture the error
result and propagate or report it instead of swallowing it; specifically, change
the block around common.ExecuteWithReauth (the closure that calls
api.GetBuildTemplate and buildParametersFromTemplate) to check the error
returned by ExecuteWithReauth and either return that error from the enclosing
function or log a clear message including the error and context (e.g., which
build template failed and h.opts.ServerURL) so callers/users see why parameters
are missing.
---
Duplicate comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 176-216: fetchTargetDefaults currently swallows errors via
h.handleError and returns nil; change its signature to return
(*buildapitypes.OperatorConfigResponse, error), propagate errors instead of
calling h.handleError (wrap GetOperatorConfig errors returned from
api.GetOperatorConfig, and return errors for missing/invalid JumpstarterTargets
checks), and return (config, nil) on success; update any callers of
fetchTargetDefaults to check the returned error and abort request construction
when non-nil so callers stop using a nil config. Make sure to reference the
function fetchTargetDefaults, the call to api.GetOperatorConfig, and the
JumpstarterTargets existence checks when replacing h.handleError calls with
returned errors.
In `@cmd/caib/buildcmd/logs.go`:
- Around line 23-212: The method waitForBuildCompletion currently returns
nothing and only calls h.handleError on failures, so callers can't detect
errors; change the signature of Handler.waitForBuildCompletion to return error
and propagate failures by returning an appropriate error instead of only calling
h.handleError (keep calling h.handleError for side-effects if you want but also
return the error). Specifically, replace the timeout branch (where it calls
h.handleError(fmt.Errorf("timed out waiting for build"))) to return that error,
and in the build-failed branch (where it computes handleErr and calls
h.handleFlashError or h.handleError) return handleErr (or the error returned by
handleFlashError if it returns one); also return any error from tryLogStreaming
up the call chain (or convert stream-related failures into returned errors where
appropriate) so callers can short-circuit; update all call sites to check the
returned error and act accordingly.
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 221-286: sealedWaitForCompletion currently only gates loop
iterations with a deadline and lets long-running GetSealed and sealedStreamLogs
calls block past that deadline; wrap the whole wait with a deadline/timeout
context derived from the incoming ctx (e.g. ctx, cancel :=
context.WithDeadline(ctx,
time.Now().Add(time.Duration(*h.opts.Timeout)*time.Minute))) and use that
context for all API calls (api.GetSealed) and for log streaming; additionally,
for each individual request create a short per-request sub-context with
context.WithTimeout (and defer cancel) so single calls don’t hang indefinitely,
and ensure sealedStreamLogs accepts/uses the passed ctx (call
h.sealedStreamLogs(ctxReq, op, name)); cancel contexts properly to avoid leaks
and check ctx.Done() before/after blocking calls to respect the overall timeout.
---
Nitpick comments:
In `@cmd/caib/common/manifest_artifact_helpers.go`:
- Around line 35-38: The absolute-path validation branch currently has a TODO
and an empty safeDirectories slice; update the logic so safe directories are
sourced from configuration/CLI instead of being a hardcoded empty list: wire a
safe-directories option into the manifest path validation (e.g., expose a flag
or read from an injected config struct used by the code that calls
filepath.IsAbs(path)), populate safeDirectories from that config, and ensure the
branch explicitly documents/handles the default policy (either deny all absolute
paths unless matched or allow-by-default with clear docs). Update the code
references around filepath.IsAbs(path) and the safeDirectories variable so the
validation uses the configured list and emits a clear error when an absolute
path is not permitted.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/login.gocmd/caib/logstream/logstream.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/caib/target_defaults_compat.go
- cmd/caib/common/api_client.go
- cmd/caib/common/build_validation.go
- cmd/caib/root.go
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 260-268: In displayBuildResults, after calling h.handleError(...)
for failed common.PullOCIArtifact calls, add an immediate return so execution
does not continue if handleError doesn't terminate; update both occurrences that
call common.PullOCIArtifact(...) with h.handleError(fmt.Errorf(...)) to be
followed by return to match the error-handling pattern used elsewhere.
In `@cmd/caib/buildcmd/logs.go`:
- Around line 258-260: The RunLogs handler reads args[0] without validation
causing a panic when no positional arg is supplied; add an explicit guard at the
start of Handler.RunLogs to check len(args) >= 1 and handle the missing-argument
case the same way as RunFlash/RunBuild/RunDisk/RunBuildDev (e.g., print an
error/usage message and return early). Update the function to return immediately
when args is empty so it never dereferences args[0]; keep the rest of the logic
unchanged and reference the Handler.RunLogs function and its args parameter when
making the change.
In `@cmd/caib/downloadcmd/download.go`:
- Around line 47-63: RunDownload currently dereferences option pointers like
h.opts.ServerURL and h.opts.OutputDir without nil-checking, which can panic if
h.opts or those pointer fields are nil; update RunDownload to first validate
h.opts is non-nil and then guard each pointer (h.opts.ServerURL,
h.opts.OutputDir and any others used later) before dereferencing, returning via
h.handleError if any are nil or empty, and use safe local variables (e.g.,
serverURL := h.opts.ServerURL) only after the nil checks to avoid panics in
RunDownload and any helper calls.
In `@cmd/caib/login.go`:
- Around line 31-35: The URL validation currently only checks parsedURL.Scheme
and Host for emptiness, allowing unsupported schemes to be saved; update the
validation in the login flow to ensure parsedURL.Scheme is either "http" or
"https" before persisting the server string (the parsedURL variable and the code
path that calls handleError should enforce this), and apply the same stricter
scheme check to the other validation site referenced (the similar check around
lines 52-53). If the scheme is not "http" or "https", call handleError with an
invalid server URL error and return so only http(s) URLs are saved.
In `@cmd/caib/querycmd/query.go`:
- Around line 49-52: Handlers are currently dereferencing option pointers (e.g.,
*h.opts.ServerURL) without nil guards which can panic if wiring misses values;
add explicit nil checks for h.opts and each required pointer before
dereferencing in the handlers (for example in Handler.RunList and the
corresponding list/show handlers referenced at the other ranges), and call
h.handleError with a descriptive error (same message already used) or return
early when a nil is detected so execution never dereferences a nil pointer;
update the checks wherever *h.opts.ServerURL, *h.opts.SomeOtherPtr are used (the
occurrences around lines 57-58, 111-112, 117-118, 148-149) to follow this
pattern.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/login.gocmd/caib/logstream/logstream.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/caib/common/oci_artifact.go
- cmd/caib/common/build_validation.go
- cmd/caib/common/manifest_artifact_helpers.go
- cmd/caib/common/cli_helpers.go
- cmd/caib/cli_helpers.go
Comment on lines
+277
to
+285
| if logRetries < maxSealedLogRetries { | ||
| logCtx, logCancel := context.WithTimeout(waitCtx, requestTimeout) | ||
| streamErr := h.sealedStreamLogs(logCtx, op, name) | ||
| logCancel() | ||
| if streamErr != nil { | ||
| logRetries++ | ||
| if logRetries == 1 { | ||
| fmt.Printf("Waiting for logs... (attempt %d/%d)\n", logRetries, maxSealedLogRetries) | ||
| } |
There was a problem hiding this comment.
Don’t count stream context timeouts as log-stream failures.
Line [278] imposes a 2-minute stream timeout, and Line [282] treats that as a failed attempt. For long builds, this can exhaust retries and disable follow mode even when streaming is otherwise healthy.
🔧 Proposed fix
import (
"bufio"
"context"
"crypto/tls"
+ "errors"
"fmt"
@@
if *h.opts.FollowLogs && !logStreaming && (st.Phase == phaseRunning || st.Phase == phasePending) {
if logRetries < maxSealedLogRetries {
logCtx, logCancel := context.WithTimeout(waitCtx, requestTimeout)
streamErr := h.sealedStreamLogs(logCtx, op, name)
logCancel()
if streamErr != nil {
+ if errors.Is(streamErr, context.DeadlineExceeded) || errors.Is(streamErr, context.Canceled) {
+ continue
+ }
logRetries++
if logRetries == 1 {
fmt.Printf("Waiting for logs... (attempt %d/%d)\n", logRetries, maxSealedLogRetries)
}
} else {Also applies to: 289-294
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
cmd/caib/querycmd/query.go (1)
109-112:⚠️ Potential issue | 🔴 CriticalPrevent panic by validating
argsbefore access.
RunShowdereferencesargs[0]without checkinglen(args), so empty invocations crash.🔧 Proposed fix
func (h *Handler) RunShow(_ *cobra.Command, args []string) { ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("build name is required")) + return + } showBuildName := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/querycmd/query.go` around lines 109 - 112, RunShow currently dereferences args[0] without validating args length which can panic; update RunShow (method on Handler) to validate the incoming args slice (e.g., if len(args) == 0) and return an error or print usage/exit early (or use cobra.ExactArgs/MinimumNArgs at command setup) before accessing args[0] so empty invocations do not crash.cmd/caib/sealedcmd/sealed.go (1)
4-14:⚠️ Potential issue | 🟠 MajorDon’t count context timeout/cancel as log-stream failure retries.
sealedStreamLogsis invoked with a 2-minute timeout. Deadline/cancel errors currently increment retry count, which can disable follow mode during long but healthy runs.🔧 Proposed fix
import ( "bufio" "context" "crypto/tls" + "errors" "fmt" "net/http"if *h.opts.FollowLogs && !logStreaming && (st.Phase == phaseRunning || st.Phase == phasePending) { if logRetries < maxSealedLogRetries { logCtx, logCancel := context.WithTimeout(waitCtx, requestTimeout) streamErr := h.sealedStreamLogs(logCtx, op, name) logCancel() if streamErr != nil { + if errors.Is(streamErr, context.DeadlineExceeded) || errors.Is(streamErr, context.Canceled) { + continue + } logRetries++ if logRetries == 1 { fmt.Printf("Waiting for logs... (attempt %d/%d)\n", logRetries, maxSealedLogRetries) } } else {Also applies to: 276-285, 289-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/sealedcmd/sealed.go` around lines 4 - 14, In sealedStreamLogs, avoid treating context.DeadlineExceeded or context.Canceled as a stream failure that increments the retry counter or disables follow mode; instead detect these with errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) and skip increasing the retry/failure count (or return/continue appropriately for a graceful exit) so deadline/cancel events don’t consume retry attempts; ensure you import the errors package and apply the same change to the other identical error-handling sites referenced (around the other ranges noted).cmd/caib/downloadcmd/download.go (1)
47-50:⚠️ Potential issue | 🔴 CriticalGuard
argsbefore readingargs[0].
RunDownloadreadsargs[0]before validating argument length, so invocation without a build name panics.🔧 Proposed fix
func (h *Handler) RunDownload(_ *cobra.Command, args []string) { ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("build name is required")) + return + } downloadBuildName := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/downloadcmd/download.go` around lines 47 - 50, RunDownload in type Handler reads args[0] without checking arguments; add a guard at the start of Handler.RunDownload to validate len(args) > 0 (or >=1) and handle the missing argument case by returning/printing an error or showing command usage before accessing args[0]; locate the RunDownload method and insert the conditional check for args length and safe early return or user-facing message to avoid a panic when args is empty.cmd/caib/buildcmd/logs.go (1)
257-261:⚠️ Potential issue | 🔴 CriticalAdd positional argument validation in
RunLogsbeforeargs[0].
RunLogscan panic when no build name argument is passed.Proposed fix
func (h *Handler) RunLogs(_ *cobra.Command, args []string) { ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("build name is required")) + return + } name := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/logs.go` around lines 257 - 261, RunLogs in type Handler dereferences args[0] without checking args length which can panic; add a guard at the start of Handler.RunLogs to check len(args) > 0 and handle the missing positional argument by printing a clear error and returning (or calling cmd.Help()/cmd.Usage() then returning) instead of accessing args[0]; update any tests or callers expecting a panic to expect a graceful error.cmd/caib/buildcmd/build.go (1)
321-326:⚠️ Potential issue | 🔴 CriticalGuard positional args before reading
args[0]in command entrypoints.These handlers panic when invoked without required positional args. Please add explicit length guards before dereferencing
args[0].Proposed fix
func (h *Handler) RunBuild(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, true, false) ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("manifest path is required")) + return + } manifestPath := args[0] *h.opts.Manifest = manifestPath @@ func (h *Handler) RunDisk(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, false, false) ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("container reference is required")) + return + } containerRef := args[0] *h.opts.ContainerRef = containerRef @@ func (h *Handler) RunBuildDev(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, false, false) ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("manifest path is required")) + return + } manifestPath := args[0] *h.opts.Manifest = manifestPathAlso applies to: 443-448, 553-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 321 - 326, The handler reads args[0] without checking args length (e.g., in Handler.RunBuild where h.applyWaitFollowDefaults is called and *h.opts.Manifest is set), causing a panic when no positional args are provided; add an explicit guard such as if len(args) == 0 { cmd.Usage(); os.Exit(1) } (or return after printing an error) before dereferencing args[0], and apply the same pattern to other command entrypoint handlers that dereference args[0].
🧹 Nitpick comments (1)
cmd/caib/common/manifest_artifact_helpers.go (1)
23-37: Cache safe-directory configuration once per manifest parse.Line 36 resolves
CAIB_SAFE_DIRECTORIESon everysource_path. This can be computed once before validation for lower overhead and deterministic behavior within a single call.Refactor sketch
+safeDirectories := configuredSafeDirectories() + isPathSafe := func(path string) error { if path == "" || path == "/" { return fmt.Errorf("empty or root path is not allowed") } @@ if filepath.IsAbs(path) { - safeDirectories := configuredSafeDirectories() if len(safeDirectories) > 0 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/manifest_artifact_helpers.go` around lines 23 - 37, The isPathSafe closure currently calls configuredSafeDirectories() inside validation for each path; compute configuredSafeDirectories() once before iterating the manifest and reuse that slice for all isPathSafe checks to avoid repeated environment lookups and ensure deterministic behavior within a single manifest parse—either capture the precomputed safeDirectories in the isPathSafe closure or change isPathSafe to accept a safeDirectories parameter and use that instead when validating source_path entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/buildcmd/logs.go`:
- Around line 65-67: Replace the stdout print used for transient status-check
failures with a stderr write: change the fmt.Printf("status check failed: %v\n",
err) call in cmd/caib/buildcmd/logs.go to use fmt.Fprintf(os.Stderr, "status
check failed: %v\n", err) so error messages go to stderr; if the file does not
already import "os", add that import. This targets the error path where
fmt.Printf is called and ensures consumers of normal output are not affected.
In `@cmd/caib/common/api_client.go`:
- Around line 97-105: The re-auth path in runWithFreshClient calls
CreateBuildAPIClient(serverURL, authToken, insecureSkipTLS) but when authToken
is nil it never substitutes the refreshed currentToken back into the
CreateBuildAPIClient call, so retries can keep using nil; update
runWithFreshClient (and the similar block around the 117-124 region) to pass a
pointer to the latest token when available (i.e., use a local tokenPtr that is
authToken if non-nil else ¤tToken when currentToken is non-empty) so
CreateBuildAPIClient always receives the refreshed credential before invoking
fn(client).
In `@cmd/caib/common/manifest_artifact_helpers.go`:
- Around line 62-77: The processAddFiles closure silently ignores malformed
add_files entries and never validates the destination "path"; update
processAddFiles to fail fast on invalid entries by returning an error when a
file item is not a map[string]any or when "path"/"source_path" are missing or
not strings, and validate both the source and destination by calling
isPathSafe(sourcePath) and isPathSafe(path) (or equivalent validation) before
appending to localFiles; reference the processAddFiles function, the addFiles
loop, localFiles slice, and isPathSafe helper when implementing these checks so
malformed manifests and unsafe destination paths are rejected immediately.
---
Duplicate comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 321-326: The handler reads args[0] without checking args length
(e.g., in Handler.RunBuild where h.applyWaitFollowDefaults is called and
*h.opts.Manifest is set), causing a panic when no positional args are provided;
add an explicit guard such as if len(args) == 0 { cmd.Usage(); os.Exit(1) } (or
return after printing an error) before dereferencing args[0], and apply the same
pattern to other command entrypoint handlers that dereference args[0].
In `@cmd/caib/buildcmd/logs.go`:
- Around line 257-261: RunLogs in type Handler dereferences args[0] without
checking args length which can panic; add a guard at the start of
Handler.RunLogs to check len(args) > 0 and handle the missing positional
argument by printing a clear error and returning (or calling
cmd.Help()/cmd.Usage() then returning) instead of accessing args[0]; update any
tests or callers expecting a panic to expect a graceful error.
In `@cmd/caib/downloadcmd/download.go`:
- Around line 47-50: RunDownload in type Handler reads args[0] without checking
arguments; add a guard at the start of Handler.RunDownload to validate len(args)
> 0 (or >=1) and handle the missing argument case by returning/printing an error
or showing command usage before accessing args[0]; locate the RunDownload method
and insert the conditional check for args length and safe early return or
user-facing message to avoid a panic when args is empty.
In `@cmd/caib/querycmd/query.go`:
- Around line 109-112: RunShow currently dereferences args[0] without validating
args length which can panic; update RunShow (method on Handler) to validate the
incoming args slice (e.g., if len(args) == 0) and return an error or print
usage/exit early (or use cobra.ExactArgs/MinimumNArgs at command setup) before
accessing args[0] so empty invocations do not crash.
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 4-14: In sealedStreamLogs, avoid treating context.DeadlineExceeded
or context.Canceled as a stream failure that increments the retry counter or
disables follow mode; instead detect these with errors.Is(err,
context.DeadlineExceeded) || errors.Is(err, context.Canceled) and skip
increasing the retry/failure count (or return/continue appropriately for a
graceful exit) so deadline/cancel events don’t consume retry attempts; ensure
you import the errors package and apply the same change to the other identical
error-handling sites referenced (around the other ranges noted).
---
Nitpick comments:
In `@cmd/caib/common/manifest_artifact_helpers.go`:
- Around line 23-37: The isPathSafe closure currently calls
configuredSafeDirectories() inside validation for each path; compute
configuredSafeDirectories() once before iterating the manifest and reuse that
slice for all isPathSafe checks to avoid repeated environment lookups and ensure
deterministic behavior within a single manifest parse—either capture the
precomputed safeDirectories in the isPathSafe closure or change isPathSafe to
accept a safeDirectories parameter and use that instead when validating
source_path entries.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/login.gocmd/caib/logstream/logstream.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/caib/build_validation.go
- cmd/caib/runtime_wiring.go
- cmd/caib/login.go
- cmd/caib/flashcmd/flash.go
- cmd/caib/buildcmd/flash_feedback.go
- cmd/caib/common/build_validation.go
- cmd/caib/root.go
Comment on lines
+65
to
+67
| if err != nil { | ||
| fmt.Printf("status check failed: %v\n", err) | ||
| continue |
There was a problem hiding this comment.
Send transient status-check failures to stderr, not stdout.
Line 66 is an error path; printing to stdout can interfere with consumers of normal output.
Proposed fix
- if err != nil {
- fmt.Printf("status check failed: %v\n", err)
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "status check failed: %v\n", err)
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/caib/buildcmd/logs.go` around lines 65 - 67, Replace the stdout print
used for transient status-check failures with a stderr write: change the
fmt.Printf("status check failed: %v\n", err) call in cmd/caib/buildcmd/logs.go
to use fmt.Fprintf(os.Stderr, "status check failed: %v\n", err) so error
messages go to stderr; if the file does not already import "os", add that
import. This targets the error path where fmt.Printf is called and ensures
consumers of normal output are not affected.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> assited-by: gpt-5.3
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
cmd/caib/buildcmd/logs.go (2)
258-261:⚠️ Potential issue | 🟠 MajorGuard positional arguments before
args[0]access.At Line 260, missing-arg invocation can panic.
🔧 Proposed fix
func (h *Handler) RunLogs(_ *cobra.Command, args []string) { ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("build name is required")) + return + } name := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/logs.go` around lines 258 - 261, The RunLogs handler currently accesses args[0] without checking args length which can panic; update the Handler.RunLogs function to validate that len(args) > 0 (and optionally check for unexpected extra args) before using args[0], return or call cmd.Usage()/print an error when missing, and ensure any subsequent code uses the validated variable (e.g., name) so the function never dereferences args[0] without the guard.
65-67:⚠️ Potential issue | 🟡 MinorRoute transient status-check failures to stderr.
At Line 66, this is an error path but currently writes to stdout.
🔧 Proposed fix
- if err != nil { - fmt.Printf("status check failed: %v\n", err) + if err != nil { + fmt.Fprintf(os.Stderr, "status check failed: %v\n", err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/logs.go` around lines 65 - 67, The error message "status check failed: %v" is being printed to stdout; change the error path in logs.go where the fmt.Printf call is used (the branch handling err after the status check) to write to stderr instead (use fmt.Fprintf with os.Stderr and include err), and ensure the os package is imported if not already present so the error output goes to stderr.cmd/caib/flashcmd/flash.go (1)
76-81:⚠️ Potential issue | 🟠 MajorAdd argument guard before reading
args[0].At Line 80, command invocation without an image ref will panic.
🔧 Proposed fix
func (h *Handler) RunFlash(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, true, false) + if len(args) == 0 { + h.handleError(fmt.Errorf("image reference is required")) + return + } ctx := context.Background() imageRef := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/flashcmd/flash.go` around lines 76 - 81, The RunFlash handler reads args[0] without validating arguments; add an argument guard at the start of Handler.RunFlash to check len(args) >= 1 and return or call cmd.Usage()/exit with a clear error if missing (before using imageRef), so that imageRef assignment and subsequent logic (e.g., imageRef := args[0], applyWaitFollowDefaults) never panic when no image ref is provided.cmd/caib/common/manifest_artifact_helpers.go (1)
62-77:⚠️ Potential issue | 🟠 MajorFail fast on malformed
add_filesitems instead of silently skipping.At Lines 63-76, invalid entries are ignored, which can mask manifest errors and drop needed references.
🔧 Proposed fix
processAddFiles := func(addFiles []any) error { - for _, file := range addFiles { - if fileMap, ok := file.(map[string]any); ok { - path, hasPath := fileMap["path"].(string) - sourcePath, hasSourcePath := fileMap["source_path"].(string) - if hasPath && hasSourcePath { - if err := isPathSafe(sourcePath); err != nil { - return err - } - localFiles = append(localFiles, map[string]string{ - "path": path, - "source_path": sourcePath, - }) - } - } + for i, file := range addFiles { + fileMap, ok := file.(map[string]any) + if !ok { + return fmt.Errorf("invalid add_files[%d]: expected mapping", i) + } + path, hasPath := fileMap["path"].(string) + sourcePath, hasSourcePath := fileMap["source_path"].(string) + if !hasPath || !hasSourcePath { + return fmt.Errorf("invalid add_files[%d]: path and source_path must be strings", i) + } + if strings.TrimSpace(path) == "" || strings.TrimSpace(sourcePath) == "" { + return fmt.Errorf("invalid add_files[%d]: empty path/source_path", i) + } + if err := isPathSafe(sourcePath); err != nil { + return err + } + localFiles = append(localFiles, map[string]string{ + "path": path, + "source_path": sourcePath, + }) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/manifest_artifact_helpers.go` around lines 62 - 77, The loop inside the processAddFiles closure currently skips invalid add_files entries; change it to fail fast by returning a descriptive error whenever an entry is not a map[string]any, or when "path" or "source_path" keys are missing or not strings (instead of silently continuing). Keep the existing isPathSafe(file) check and its error return, but add explicit error returns for malformed entries referencing the function/closure name processAddFiles and the variables addFiles, fileMap, path and sourcePath so callers can see which entry was invalid.cmd/caib/common/api_client.go (1)
97-104:⚠️ Potential issue | 🟠 MajorRefreshed token is still dropped when
authTokenis nil.At Line 98,
runWithFreshClientalways passesauthTokentoCreateBuildAPIClient. IfauthToken == nil, the refreshed token set at Line 121 only lives incurrentToken, so retry can still run without it.🔧 Proposed fix
runWithFreshClient := func() error { - client, err := CreateBuildAPIClient(serverURL, authToken, insecureSkipTLS) + tokenRef := authToken + if tokenRef == nil { + tokenRef = ¤tToken + } + + client, err := CreateBuildAPIClient(serverURL, tokenRef, insecureSkipTLS) if err != nil { return err } - if authToken != nil { - currentToken = strings.TrimSpace(*authToken) - } + currentToken = strings.TrimSpace(*tokenRef) return fn(client) }Also applies to: 121-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/common/api_client.go` around lines 97 - 104, The refreshed token stored in currentToken is never passed into CreateBuildAPIClient when authToken is nil, so update runWithFreshClient and the retry path to always pass the latest token pointer: ensure you create a token pointer variable (e.g., tokenPtr := ¤tToken) when authToken is nil and call CreateBuildAPIClient(serverURL, tokenPtr, insecureSkipTLS) (and likewise use tokenPtr instead of authToken in the retry logic around the refreshed-token handling at lines handling currentToken update) so the refreshed token is used for subsequent retries; reference functions/vars: runWithFreshClient, CreateBuildAPIClient, currentToken, authToken.cmd/caib/buildcmd/build.go (3)
321-326:⚠️ Potential issue | 🔴 CriticalMissing argument validation before accessing
args[0]inRunBuild.Line 325 accesses
args[0]without checking ifargshas any elements. This will panic if the command is invoked without a manifest path.🐛 Proposed fix
func (h *Handler) RunBuild(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, true, false) ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("manifest path is required")) + return + } manifestPath := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 321 - 326, RunBuild currently dereferences args[0] without validating args length; add a guard at the start of RunBuild that checks if len(args) < 1 and handle the missing manifest gracefully (e.g., call cmd.Usage() or print an error and return/exit) before proceeding. Specifically, in Handler.RunBuild validate args, return early on failure, and only then set manifestPath := args[0] and *h.opts.Manifest = manifestPath so you never access args[0] when it's absent.
443-448:⚠️ Potential issue | 🔴 CriticalMissing argument validation before accessing
args[0]inRunDisk.Line 447 accesses
args[0]without checking ifargshas any elements. This will panic if the command is invoked without a container reference.🐛 Proposed fix
func (h *Handler) RunDisk(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, false, false) ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("container reference is required")) + return + } containerRef := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 443 - 448, RunDisk currently reads args[0] without validation which will panic if no container reference is provided; add an args length check at the start of Handler.RunDisk (e.g., verify len(args) > 0) and return a user-facing error (or call cmd.Usage()/log and exit) if missing, then only assign containerRef and set *h.opts.ContainerRef when the argument is present; reference the Handler.RunDisk function, the args slice, containerRef variable, and h.opts.ContainerRef to locate and update the code.
553-558:⚠️ Potential issue | 🔴 CriticalMissing argument validation before accessing
args[0]inRunBuildDev.Line 557 accesses
args[0]without checking ifargshas any elements. This will panic if the command is invoked without a manifest path.🐛 Proposed fix
func (h *Handler) RunBuildDev(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, false, false) ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("manifest path is required")) + return + } manifestPath := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 553 - 558, In RunBuildDev, args[0] is accessed without validation which will panic when no manifest path is provided; add an argument check at the start of Handler.RunBuildDev to verify len(args) > 0 and, if not, return an error or print usage and exit (e.g., call cmd.Usage() or cmd.Help() and stop execution) before calling h.applyWaitFollowDefaults or dereferencing *h.opts.Manifest, ensuring the manifestPath assignment only happens after the check.cmd/caib/querycmd/query.go (1)
109-112:⚠️ Potential issue | 🔴 CriticalMissing argument validation before accessing
args[0]inRunShow.Line 111 accesses
args[0]without checking ifargshas any elements, which will panic if the command is invoked without a build name. The validation for options happens after this access.🐛 Proposed fix
func (h *Handler) RunShow(_ *cobra.Command, args []string) { ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("build name is required")) + return + } showBuildName := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/querycmd/query.go` around lines 109 - 112, The RunShow handler accesses args[0] without checking length; add a guard at the start of Handler.RunShow to validate that len(args) >= 1 (or check args != nil and len > 0) and if not present print a helpful error/usage message and return (or call cmd.Usage()/os.Exit as appropriate) before any use of args[0]; update any related early-return paths so subsequent option validation still runs only after this argument check.cmd/caib/sealedcmd/sealed.go (1)
311-317:⚠️ Potential issue | 🟡 MinorMissing nil guard for
InsecureSkipTLSoption pointer.Line 312 dereferences
*h.opts.InsecureSkipTLSwithout a nil check. If the option isn't wired, this will panic. Other handlers in this PR guard against nil option pointers.🛡️ Proposed fix
transport := &http.Transport{} - if *h.opts.InsecureSkipTLS { + if h.opts.InsecureSkipTLS != nil && *h.opts.InsecureSkipTLS { transport.TLSClientConfig = &tls.Config{ InsecureSkipVerify: true, MinVersion: tls.VersionTLS12, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/sealedcmd/sealed.go` around lines 311 - 317, The code dereferences h.opts.InsecureSkipTLS without a nil guard which can panic; update the condition around transport setup (the block creating transport := &http.Transport{}) to check that h.opts and h.opts.InsecureSkipTLS are non-nil before dereferencing (e.g., change the if to ensure h.opts != nil && h.opts.InsecureSkipTLS != nil && *h.opts.InsecureSkipTLS) so the TLSClientConfig is only set when the pointer exists and is true, matching the nil-guard pattern used in other handlers.cmd/caib/downloadcmd/download.go (1)
47-50:⚠️ Potential issue | 🔴 CriticalMissing argument validation before accessing
args[0]inRunDownload.Line 49 accesses
args[0]without checking ifargshas any elements, causing a panic if the command is invoked without a build name argument.🐛 Proposed fix
func (h *Handler) RunDownload(_ *cobra.Command, args []string) { ctx := context.Background() + if len(args) == 0 { + h.handleError(fmt.Errorf("build name is required")) + return + } downloadBuildName := args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/downloadcmd/download.go` around lines 47 - 50, RunDownload in type Handler currently accesses args[0] without validation causing a panic; add an argument check at the start of RunDownload (Handler.RunDownload) to ensure len(args) > 0 and handle the missing-argument case by printing/logging an error and returning a non-zero exit or invoking cobra's help/usage (or returning an error) instead of proceeding to use downloadBuildName; ensure the validation prevents nil/empty access to args before any further logic that uses args[0].
🧹 Nitpick comments (1)
cmd/caib/buildcmd/build.go (1)
394-411: Consider extracting duplicated flash configuration logic.The flash configuration block (reading Jumpstarter client config, setting FlashEnabled/FlashClientConfig/FlashLeaseDuration) is duplicated across
RunBuild,RunDisk, andRunBuildDev. This could be extracted into a helper method to reduce duplication.♻️ Suggested refactor
func (h *Handler) applyFlashConfiguration(req *buildapitypes.BuildRequest) error { if !*h.opts.FlashAfterBuild { return nil } if *h.opts.ExportOCI == "" && !*h.opts.UseInternalRegistry { return fmt.Errorf("cannot enable --flash without exporting a disk image") } if *h.opts.JumpstarterClient == "" { return fmt.Errorf("--flash requires --client to specify Jumpstarter client config file") } clientConfigBytes, err := os.ReadFile(*h.opts.JumpstarterClient) if err != nil { return fmt.Errorf("failed to read Jumpstarter client config: %w", err) } req.FlashEnabled = true req.FlashClientConfig = base64.StdEncoding.EncodeToString(clientConfigBytes) req.FlashLeaseDuration = *h.opts.LeaseDuration return nil }Also applies to lines 516-533 and 649-667.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 394 - 411, Extract the duplicated flash setup into a new Handler method (e.g., applyFlashConfiguration) and call it from RunBuild, RunDisk, and RunBuildDev: move the logic that checks h.opts.FlashAfterBuild, validates ExportOCI/UseInternalRegistry and JumpstarterClient, reads the Jumpstarter client file, base64-encodes it, and sets req.FlashEnabled, req.FlashClientConfig, and req.FlashLeaseDuration into that helper; have the helper return an error for any validation/file-read failures so callers (RunBuild/RunDisk/RunBuildDev) can propagate via h.handleError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 195-219: The function sealedRunViaAPI dereferences option pointers
without nil checks (e.g., *h.opts.ServerURL, *h.opts.InsecureSkipTLS,
*h.opts.WaitForBuild, *h.opts.FollowLogs); add guards at the start of
sealedRunViaAPI to validate those option pointers (returning via h.handleError
or a descriptive error) before dereferencing, then proceed to call
common.CreateBuildAPIClient, h.sealedBuildRequest and h.sealedWaitForCompletion
as before; ensure the same validation pattern used in other handlers is followed
so sealedRunViaAPI never dereferences nil option pointers.
- Around line 151-164: The sealedBuildRequest function on Handler dereferences
option pointers (AutomotiveImageBuilder, SealedBuilderImage, SealedArchitecture,
AIBExtraArgs) without nil checks which can panic; add validation at the start of
sealedBuildRequest to verify h.opts is non-nil and each of those pointer fields
is non-nil and return a descriptive error if any are missing (e.g., "missing
option: AutomotiveImageBuilder"); only dereference the pointers after these
guards so the function builds the SealedRequest safely.
---
Duplicate comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 321-326: RunBuild currently dereferences args[0] without
validating args length; add a guard at the start of RunBuild that checks if
len(args) < 1 and handle the missing manifest gracefully (e.g., call cmd.Usage()
or print an error and return/exit) before proceeding. Specifically, in
Handler.RunBuild validate args, return early on failure, and only then set
manifestPath := args[0] and *h.opts.Manifest = manifestPath so you never access
args[0] when it's absent.
- Around line 443-448: RunDisk currently reads args[0] without validation which
will panic if no container reference is provided; add an args length check at
the start of Handler.RunDisk (e.g., verify len(args) > 0) and return a
user-facing error (or call cmd.Usage()/log and exit) if missing, then only
assign containerRef and set *h.opts.ContainerRef when the argument is present;
reference the Handler.RunDisk function, the args slice, containerRef variable,
and h.opts.ContainerRef to locate and update the code.
- Around line 553-558: In RunBuildDev, args[0] is accessed without validation
which will panic when no manifest path is provided; add an argument check at the
start of Handler.RunBuildDev to verify len(args) > 0 and, if not, return an
error or print usage and exit (e.g., call cmd.Usage() or cmd.Help() and stop
execution) before calling h.applyWaitFollowDefaults or dereferencing
*h.opts.Manifest, ensuring the manifestPath assignment only happens after the
check.
In `@cmd/caib/buildcmd/logs.go`:
- Around line 258-261: The RunLogs handler currently accesses args[0] without
checking args length which can panic; update the Handler.RunLogs function to
validate that len(args) > 0 (and optionally check for unexpected extra args)
before using args[0], return or call cmd.Usage()/print an error when missing,
and ensure any subsequent code uses the validated variable (e.g., name) so the
function never dereferences args[0] without the guard.
- Around line 65-67: The error message "status check failed: %v" is being
printed to stdout; change the error path in logs.go where the fmt.Printf call is
used (the branch handling err after the status check) to write to stderr instead
(use fmt.Fprintf with os.Stderr and include err), and ensure the os package is
imported if not already present so the error output goes to stderr.
In `@cmd/caib/common/api_client.go`:
- Around line 97-104: The refreshed token stored in currentToken is never passed
into CreateBuildAPIClient when authToken is nil, so update runWithFreshClient
and the retry path to always pass the latest token pointer: ensure you create a
token pointer variable (e.g., tokenPtr := ¤tToken) when authToken is nil
and call CreateBuildAPIClient(serverURL, tokenPtr, insecureSkipTLS) (and
likewise use tokenPtr instead of authToken in the retry logic around the
refreshed-token handling at lines handling currentToken update) so the refreshed
token is used for subsequent retries; reference functions/vars:
runWithFreshClient, CreateBuildAPIClient, currentToken, authToken.
In `@cmd/caib/common/manifest_artifact_helpers.go`:
- Around line 62-77: The loop inside the processAddFiles closure currently skips
invalid add_files entries; change it to fail fast by returning a descriptive
error whenever an entry is not a map[string]any, or when "path" or "source_path"
keys are missing or not strings (instead of silently continuing). Keep the
existing isPathSafe(file) check and its error return, but add explicit error
returns for malformed entries referencing the function/closure name
processAddFiles and the variables addFiles, fileMap, path and sourcePath so
callers can see which entry was invalid.
In `@cmd/caib/downloadcmd/download.go`:
- Around line 47-50: RunDownload in type Handler currently accesses args[0]
without validation causing a panic; add an argument check at the start of
RunDownload (Handler.RunDownload) to ensure len(args) > 0 and handle the
missing-argument case by printing/logging an error and returning a non-zero exit
or invoking cobra's help/usage (or returning an error) instead of proceeding to
use downloadBuildName; ensure the validation prevents nil/empty access to args
before any further logic that uses args[0].
In `@cmd/caib/flashcmd/flash.go`:
- Around line 76-81: The RunFlash handler reads args[0] without validating
arguments; add an argument guard at the start of Handler.RunFlash to check
len(args) >= 1 and return or call cmd.Usage()/exit with a clear error if missing
(before using imageRef), so that imageRef assignment and subsequent logic (e.g.,
imageRef := args[0], applyWaitFollowDefaults) never panic when no image ref is
provided.
In `@cmd/caib/querycmd/query.go`:
- Around line 109-112: The RunShow handler accesses args[0] without checking
length; add a guard at the start of Handler.RunShow to validate that len(args)
>= 1 (or check args != nil and len > 0) and if not present print a helpful
error/usage message and return (or call cmd.Usage()/os.Exit as appropriate)
before any use of args[0]; update any related early-return paths so subsequent
option validation still runs only after this argument check.
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 311-317: The code dereferences h.opts.InsecureSkipTLS without a
nil guard which can panic; update the condition around transport setup (the
block creating transport := &http.Transport{}) to check that h.opts and
h.opts.InsecureSkipTLS are non-nil before dereferencing (e.g., change the if to
ensure h.opts != nil && h.opts.InsecureSkipTLS != nil &&
*h.opts.InsecureSkipTLS) so the TLSClientConfig is only set when the pointer
exists and is true, matching the nil-guard pattern used in other handlers.
---
Nitpick comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 394-411: Extract the duplicated flash setup into a new Handler
method (e.g., applyFlashConfiguration) and call it from RunBuild, RunDisk, and
RunBuildDev: move the logic that checks h.opts.FlashAfterBuild, validates
ExportOCI/UseInternalRegistry and JumpstarterClient, reads the Jumpstarter
client file, base64-encodes it, and sets req.FlashEnabled,
req.FlashClientConfig, and req.FlashLeaseDuration into that helper; have the
helper return an error for any validation/file-read failures so callers
(RunBuild/RunDisk/RunBuildDev) can propagate via h.handleError.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
cmd/caib/build_validation.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/flash_feedback.gocmd/caib/buildcmd/logs.gocmd/caib/cli_helpers.gocmd/caib/common/api_client.gocmd/caib/common/build_validation.gocmd/caib/common/cli_helpers.gocmd/caib/common/manifest_artifact_helpers.gocmd/caib/common/oci_artifact.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/image/image_test.gocmd/caib/login.gocmd/caib/logstream/logstream.gocmd/caib/main.gocmd/caib/querycmd/query.gocmd/caib/root.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gocmd/caib/target_defaults_compat.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/caib/target_defaults_compat.go
- cmd/caib/build_validation.go
- cmd/caib/cli_helpers.go
- cmd/caib/logstream/logstream.go
- cmd/caib/root.go
- cmd/caib/common/cli_helpers.go
- cmd/caib/common/build_validation.go
- cmd/caib/login.go
Comment on lines
+195
to
+219
| func (h *Handler) sealedRunViaAPI(op buildapitypes.SealedOperation, inputRef, outputRef, signedRef string) { | ||
| api, err := common.CreateBuildAPIClient(*h.opts.ServerURL, h.opts.AuthToken, *h.opts.InsecureSkipTLS) | ||
| if err != nil { | ||
| h.handleError(err) | ||
| return | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| req, err := h.sealedBuildRequest(op, inputRef, outputRef, signedRef) | ||
| if err != nil { | ||
| h.handleError(err) | ||
| return | ||
| } | ||
|
|
||
| resp, err := api.CreateSealed(ctx, req) | ||
| if err != nil { | ||
| h.handleError(err) | ||
| return | ||
| } | ||
| fmt.Printf("Job %s accepted: %s - %s\n", resp.Name, resp.Phase, resp.Message) | ||
|
|
||
| if *h.opts.WaitForBuild || *h.opts.FollowLogs { | ||
| h.sealedWaitForCompletion(ctx, api, op, resp.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing nil guards for option pointers in sealedRunViaAPI.
Lines 196 and 216 dereference option pointers (ServerURL, InsecureSkipTLS, WaitForBuild, FollowLogs) without nil checks. This is inconsistent with other handlers that validate required options before use.
🛡️ Proposed fix
func (h *Handler) sealedRunViaAPI(op buildapitypes.SealedOperation, inputRef, outputRef, signedRef string) {
+ if h.opts.ServerURL == nil || h.opts.InsecureSkipTLS == nil {
+ h.handleError(fmt.Errorf("internal error: sealed handler options are not fully initialized"))
+ return
+ }
api, err := common.CreateBuildAPIClient(*h.opts.ServerURL, h.opts.AuthToken, *h.opts.InsecureSkipTLS)And at line 216:
- if *h.opts.WaitForBuild || *h.opts.FollowLogs {
+ if (h.opts.WaitForBuild != nil && *h.opts.WaitForBuild) || (h.opts.FollowLogs != nil && *h.opts.FollowLogs) {
h.sealedWaitForCompletion(ctx, api, op, resp.Name)
}📝 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 (h *Handler) sealedRunViaAPI(op buildapitypes.SealedOperation, inputRef, outputRef, signedRef string) { | |
| api, err := common.CreateBuildAPIClient(*h.opts.ServerURL, h.opts.AuthToken, *h.opts.InsecureSkipTLS) | |
| if err != nil { | |
| h.handleError(err) | |
| return | |
| } | |
| ctx := context.Background() | |
| req, err := h.sealedBuildRequest(op, inputRef, outputRef, signedRef) | |
| if err != nil { | |
| h.handleError(err) | |
| return | |
| } | |
| resp, err := api.CreateSealed(ctx, req) | |
| if err != nil { | |
| h.handleError(err) | |
| return | |
| } | |
| fmt.Printf("Job %s accepted: %s - %s\n", resp.Name, resp.Phase, resp.Message) | |
| if *h.opts.WaitForBuild || *h.opts.FollowLogs { | |
| h.sealedWaitForCompletion(ctx, api, op, resp.Name) | |
| } | |
| } | |
| func (h *Handler) sealedRunViaAPI(op buildapitypes.SealedOperation, inputRef, outputRef, signedRef string) { | |
| if h.opts.ServerURL == nil || h.opts.InsecureSkipTLS == nil { | |
| h.handleError(fmt.Errorf("internal error: sealed handler options are not fully initialized")) | |
| return | |
| } | |
| api, err := common.CreateBuildAPIClient(*h.opts.ServerURL, h.opts.AuthToken, *h.opts.InsecureSkipTLS) | |
| if err != nil { | |
| h.handleError(err) | |
| return | |
| } | |
| ctx := context.Background() | |
| req, err := h.sealedBuildRequest(op, inputRef, outputRef, signedRef) | |
| if err != nil { | |
| h.handleError(err) | |
| return | |
| } | |
| resp, err := api.CreateSealed(ctx, req) | |
| if err != nil { | |
| h.handleError(err) | |
| return | |
| } | |
| fmt.Printf("Job %s accepted: %s - %s\n", resp.Name, resp.Phase, resp.Message) | |
| if (h.opts.WaitForBuild != nil && *h.opts.WaitForBuild) || (h.opts.FollowLogs != nil && *h.opts.FollowLogs) { | |
| h.sealedWaitForCompletion(ctx, api, op, resp.Name) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/caib/sealedcmd/sealed.go` around lines 195 - 219, The function
sealedRunViaAPI dereferences option pointers without nil checks (e.g.,
*h.opts.ServerURL, *h.opts.InsecureSkipTLS, *h.opts.WaitForBuild,
*h.opts.FollowLogs); add guards at the start of sealedRunViaAPI to validate
those option pointers (returning via h.handleError or a descriptive error)
before dereferencing, then proceed to call common.CreateBuildAPIClient,
h.sealedBuildRequest and h.sealedWaitForCompletion as before; ensure the same
validation pattern used in other handlers is followed so sealedRunViaAPI never
dereferences nil option pointers.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.5
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/caib/sealedcmd/sealed.go (1)
254-272:⚠️ Potential issue | 🟠 MajorDon’t count context timeout/cancel as a failed log-stream attempt.
When log streaming is intentionally bounded by
requestTimeout, timeout/cancel errors from that context are currently counted as retries. On long-running jobs this can disable--followeven though streaming is otherwise healthy.🔧 Suggested fix
import ( "bufio" "context" "crypto/tls" + "errors" "fmt" @@ streamErr := h.sealedStreamLogs(logCtx, op, name) logCancel() if streamErr != nil { + if errors.Is(streamErr, context.DeadlineExceeded) || errors.Is(streamErr, context.Canceled) { + continue + } logRetries++ if logRetries == 1 { fmt.Printf("Waiting for logs... (attempt %d/%d)\n", logRetries, maxSealedLogRetries) } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/sealedcmd/sealed.go` around lines 254 - 272, The log streaming retry logic treats context timeout/cancel as failures; update the block that handles streamErr from sealedStreamLogs so that it does not increment logRetries or mark a retry attempt when streamErr is context.DeadlineExceeded or context.Canceled (use errors.Is to check). Keep the existing behavior for other errors (increment logRetries and print the waiting message on first retry) and still set logStreaming = true when streamErr == nil; reference sealedStreamLogs, streamErr, logRetries, maxSealedLogRetries, requestTimeout, logCtx, and the FollowLogs flag in your change.
🧹 Nitpick comments (1)
cmd/caib/image/image.go (1)
97-230: Extract shared flag wiring helpers to reduce drift risk.
build,disk, andbuild-devduplicate a large amount of flag setup. This is already hard to keep consistent (defaults/help text can diverge over time).♻️ Suggested refactor sketch
+func addCommonAPITokenFlags(cmd *cobra.Command, opts Options) { + cmd.Flags().StringVar(opts.ServerURL, "server", config.DefaultServer(), "REST API server base URL") + cmd.Flags().StringVar(opts.AuthToken, "token", os.Getenv("CAIB_TOKEN"), "Bearer token for authentication") +} + +func addCommonBuildWaitLogFlags(cmd *cobra.Command, opts Options, waitDefault bool) { + cmd.Flags().IntVar(opts.Timeout, "timeout", 60, "timeout in minutes") + cmd.Flags().BoolVarP(opts.WaitForBuild, "wait", "w", waitDefault, "wait for build to complete") + cmd.Flags().BoolVarP(opts.FollowLogs, "follow", "f", false, "follow build logs (shows full log output instead of progress bar)") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/image/image.go` around lines 97 - 230, Several flag blocks (for buildCmd, diskCmd, buildDevCmd) are duplicated; extract shared wiring into helper functions to avoid drift. Create small functions such as addServerAuthFlags(cmd *cobra.Command, opts *optsType), addCommonBuildFlags(cmd *cobra.Command, opts *optsType), addFlashFlags(cmd *cobra.Command, opts *optsType), and addInternalRegistryFlags(cmd *cobra.Command, opts *optsType) (and a specialized addDiskFlags if needed) and call them from where buildCmd, diskCmd, and buildDevCmd are initialized; move all repeated StringVar/BoolVar/IntVar calls into these helpers and keep unique flags (e.g., Build-specific BuildName/Mode/OutputDir differences) in the command setup. Ensure helpers reference the same opts fields (ServerURL, AuthToken, Distro, Target, Architecture, Timeout, WaitForBuild, FollowLogs, AIBExtraArgs, AutomotiveImageBuilder, RegistryAuthFile, CompressionAlgo, ExportOCI/ContainerPush, DiskFormat, OutputDir, StorageClass, FlashAfterBuild, JumpstarterClient, LeaseDuration, UseInternalRegistry, InternalRegistryImageName, InternalRegistryTag, CustomDefs, BuilderImage, RebuildBuilder) so no flag semantics 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 `@cmd/caib/README.md`:
- Around line 254-273: The newly added Markdown tables (including the "**Shared
flags:**" table and the other recent table) are missing the required blank lines
before their start; insert a single blank line above each table and ensure at
least one blank line after the preceding paragraph so the tables comply with
markdownlint MD058.
In `@internal/common/tasks/scripts/sealed_operation.sh`:
- Line 87: The auth file is created under a permissive umask (umask 0022) so it
may get too-open permissions before chmod; change the writes that create
"$HOME/.custom_authjson" (and the other occurrences around lines 100-101) to run
under a restrictive umask, e.g. use a subshell or inline umask: (umask 077; echo
"$REGISTRY_AUTH_FILE_CONTENT" > "$HOME/.custom_authjson") or (umask 077; printf
'%s' "$REGISTRY_AUTH_FILE_CONTENT" > "$HOME/.custom_authjson") so the file is
created with safe permissions from the moment of creation.
- Line 95: The current assignment cfg.setdefault('auths', {})[sys.argv[2]] =
{'auth': sys.argv[3]} unconditionally overwrites any existing auth for the
registry from REGISTRY_AUTH_FILE_CONTENT; change it to only insert the merged
credentials when there is no existing explicit entry: check cfg.get('auths',
{}).get(sys.argv[2]) and if it is None then set cfg.setdefault('auths',
{})[sys.argv[2]] = {'auth': sys.argv[3]}; alternatively merge by preserving
existing value (e.g., only update missing keys) so you do not replace
user-provided creds during the SA merge.
---
Duplicate comments:
In `@cmd/caib/sealedcmd/sealed.go`:
- Around line 254-272: The log streaming retry logic treats context
timeout/cancel as failures; update the block that handles streamErr from
sealedStreamLogs so that it does not increment logRetries or mark a retry
attempt when streamErr is context.DeadlineExceeded or context.Canceled (use
errors.Is to check). Keep the existing behavior for other errors (increment
logRetries and print the waiting message on first retry) and still set
logStreaming = true when streamErr == nil; reference sealedStreamLogs,
streamErr, logRetries, maxSealedLogRetries, requestTimeout, logCtx, and the
FollowLogs flag in your change.
---
Nitpick comments:
In `@cmd/caib/image/image.go`:
- Around line 97-230: Several flag blocks (for buildCmd, diskCmd, buildDevCmd)
are duplicated; extract shared wiring into helper functions to avoid drift.
Create small functions such as addServerAuthFlags(cmd *cobra.Command, opts
*optsType), addCommonBuildFlags(cmd *cobra.Command, opts *optsType),
addFlashFlags(cmd *cobra.Command, opts *optsType), and
addInternalRegistryFlags(cmd *cobra.Command, opts *optsType) (and a specialized
addDiskFlags if needed) and call them from where buildCmd, diskCmd, and
buildDevCmd are initialized; move all repeated StringVar/BoolVar/IntVar calls
into these helpers and keep unique flags (e.g., Build-specific
BuildName/Mode/OutputDir differences) in the command setup. Ensure helpers
reference the same opts fields (ServerURL, AuthToken, Distro, Target,
Architecture, Timeout, WaitForBuild, FollowLogs, AIBExtraArgs,
AutomotiveImageBuilder, RegistryAuthFile, CompressionAlgo,
ExportOCI/ContainerPush, DiskFormat, OutputDir, StorageClass, FlashAfterBuild,
JumpstarterClient, LeaseDuration, UseInternalRegistry,
InternalRegistryImageName, InternalRegistryTag, CustomDefs, BuilderImage,
RebuildBuilder) so no flag semantics change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/caib/README.mdcmd/caib/image/image.gocmd/caib/runtime_wiring.gocmd/caib/sealedcmd/sealed.gointernal/common/tasks/scripts/sealed_operation.sh
Comment on lines
+254
to
+273
| **Shared flags:** | ||
| | Flag | Default | Description | | ||
| |------|---------|-------------| | ||
| | `--server` | `$CAIB_SERVER` | Build API server URL | | ||
| | `--token` | `$CAIB_TOKEN` | Bearer token | | ||
| | `--input` | | Input/source container ref (alternative to positional) | | ||
| | `--output` | | Output container ref (alternative to positional) | | ||
| | `--aib-image` | `quay.io/.../automotive-image-builder:latest` | AIB container image | | ||
| | `--builder-image` | | Builder container image (overrides `--arch` default) | | ||
| | `--arch` | (auto-detected) | Target architecture (`amd64`, `arm64`) | | ||
| | `--key` | | Path to local PEM key file | | ||
| | `--passwd` | | Password for encrypted key file | | ||
| | `--key-secret` | | Name of cluster secret containing sealing key | | ||
| | `--key-password-secret` | | Name of cluster secret containing key password | | ||
| | `--registry-auth-file` | | Path to Docker/Podman auth file for registry authentication | | ||
| | `--extra-args` | | Extra arguments to pass to AIB (repeatable) | | ||
| | `--timeout` | `120` | Timeout in minutes | | ||
| | `-w`, `--wait` | `false` | Wait for completion | | ||
| | `-f`, `--follow` | `true` | Stream task logs | | ||
|
|
There was a problem hiding this comment.
Add blank lines around new tables to satisfy markdownlint MD058.
Both newly added tables are missing the required blank line before table start.
📝 Suggested markdown fix
**Shared flags:**
+
| Flag | Default | Description |
|------|---------|-------------|
| `--server` | `$CAIB_SERVER` | Build API server URL |
@@
| `-w`, `--wait` | `false` | Wait for completion |
| `-f`, `--follow` | `true` | Stream task logs |
@@
Additional flag:
+
| Flag | Description |
|------|-------------|
| `--signed` | Signed artifact ref (alternative to positional) |Also applies to: 320-323
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 255-255: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/caib/README.md` around lines 254 - 273, The newly added Markdown tables
(including the "**Shared flags:**" table and the other recent table) are missing
the required blank lines before their start; insert a single blank line above
each table and ensure at least one blank line after the preceding paragraph so
the tables comply with markdownlint MD058.
Contributor
|
@bennyz the code changes look good. I didn’t see any substantial issues, but it looks like you only updated the sealed operation README with the new syntax, what about the other commands? |
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.5
Contributor
Author
updated |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/caib/README.md (1)
243-245:⚠️ Potential issue | 🟡 MinorAdd required blank lines before table blocks (MD058).
Line 244 and Line 310 start tables immediately after prose labels. Markdownlint requires a blank line before these tables.
Suggested markdown fix
**Shared flags:** + | Flag | Default | Description | |------|---------|-------------| | `--server` | `$CAIB_SERVER` | Build API server URL | @@ Additional flag: + | Flag | Description | |------|-------------| | `--signed` | Signed artifact ref (alternative to positional) |Also applies to: 309-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/README.md` around lines 243 - 245, The markdown linter error is caused by tables being placed immediately after prose labels; insert a single blank line before each table header that follows a paragraph label (e.g., before the line with "| Flag | Default | Description |" that follows the "**Shared flags:**" label and the other table starting at lines referenced around 309-311) so that each table is separated from the preceding text by an empty line; update every occurrence where a paragraph or label is immediately followed by a table to include that blank line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/README.md`:
- Around line 231-236: Update the README example heading to avoid overstating
authentication requirements: replace the phrase that says push "requires
REGISTRY_USERNAME/REGISTRY_PASSWORD" with a neutral sentence that notes multiple
authentication methods are supported (e.g., environment vars
REGISTRY_USERNAME/REGISTRY_PASSWORD, REGISTRY_AUTH_FILE/--registry-auth-file, or
discovered auth.json) and recommends setting an auth method if pushing to a
registry; ensure the example block (the image build/push command) remains
unchanged and briefly list the alternate auth methods in the same sentence for
clarity.
---
Duplicate comments:
In `@cmd/caib/README.md`:
- Around line 243-245: The markdown linter error is caused by tables being
placed immediately after prose labels; insert a single blank line before each
table header that follows a paragraph label (e.g., before the line with "| Flag
| Default | Description |" that follows the "**Shared flags:**" label and the
other table starting at lines referenced around 309-311) so that each table is
separated from the preceding text by an empty line; update every occurrence
where a paragraph or label is immediately followed by a table to include that
blank line.
Comment on lines
231
to
+236
| # Build and push to OCI registry (requires REGISTRY_USERNAME/REGISTRY_PASSWORD env vars) | ||
| bin/caib build-dev my-manifest.aib.yml \ | ||
| bin/caib image build-dev my-manifest.aib.yml \ | ||
| --arch arm64 \ | ||
| --mode image \ | ||
| --format qcow2 \ | ||
| --push quay.io/myorg/disk-image:v1.0 \ | ||
| --follow | ||
| --push quay.io/myorg/disk-image:v1.0 |
There was a problem hiding this comment.
Avoid overstating auth requirements in the build-dev push example.
Line 231 says push requires REGISTRY_USERNAME/REGISTRY_PASSWORD, but the same README now documents other supported methods (--registry-auth-file, REGISTRY_AUTH_FILE, discovered auth.json). Please relax wording to avoid user confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/caib/README.md` around lines 231 - 236, Update the README example heading
to avoid overstating authentication requirements: replace the phrase that says
push "requires REGISTRY_USERNAME/REGISTRY_PASSWORD" with a neutral sentence that
notes multiple authentication methods are supported (e.g., environment vars
REGISTRY_USERNAME/REGISTRY_PASSWORD, REGISTRY_AUTH_FILE/--registry-auth-file, or
discovered auth.json) and recommends setting an auth method if pushing to a
registry; ensure the example block (the image build/push command) remains
unchanged and briefly list the alternate auth methods in the same sentence for
clarity.
bkhizgiy
approved these changes
Mar 3, 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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation