[SREP-4791]: Add sre-agent integration to osdctl AI commands#890
Open
TheUndeadKing wants to merge 3 commits into
Open
[SREP-4791]: Add sre-agent integration to osdctl AI commands#890TheUndeadKing wants to merge 3 commits into
TheUndeadKing wants to merge 3 commits into
Conversation
openshift-ci
Bot
added
the
needs-rebase
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an ChangesAI Command Group and SRE Agent Subcommand
sequenceDiagram
actor User
participant CLI as osdctl CLI
participant Cmd as ai sre-agent Cmd
participant Validate as Validation Logic
participant FS as File System
participant Binary as External sre-agent Binary
User->>CLI: osdctl ai sre-agent --pd-url=<url>
CLI->>Cmd: execute subcommand
Cmd->>Validate: validateSreAgent()
Validate->>FS: stat <DataHome>/sre-agent/venv/bin/sre-agent
alt not present
Validate->>User: prompt for venv path
User->>Validate: provide path
Validate->>FS: copy venv -> <DataHome>/sre-agent/venv
end
Cmd->>Validate: checkSreAgentConfig()
Validate->>FS: read <ConfigHome>/sre-agent/config.yaml
alt ops-sop missing or path differs
Validate->>User: prompt for ops-sop path
User->>Validate: provide path
Validate->>FS: copy ops-sop -> <DataHome>/sre-agent/ops-sop
Validate->>FS: write updated config.yaml
end
Cmd->>Binary: exec <DataHome>/sre-agent/venv/bin/sre-agent (stdio wired)
Binary-->>CLI: exit code / output
CLI-->>User: display result
Estimated Code Review Effort 🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TheUndeadKing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
cmd/ai/sre_agent/ops_sop.go (1)
12-39: 💤 Low valueConsider renaming to reflect the function's full behavior.
The function name
opsSopIsPresentsuggests it only checks whether ops-sop is present, but it also updates an existing repository or clones a new one. A more descriptive name likeensureOpsSopReadyorsetupOpsSopRepowould better communicate that the function performs side effects beyond just checking presence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/ops_sop.go` around lines 12 - 39, Rename the function opsSopIsPresent to a name that reflects its side effects (e.g., ensureOpsSopReady or setupOpsSopRepo) and update all call sites accordingly; inside the function, keep the existing logic (checking utils.FolderExists(opsSopPath), calling gitPullAndValidate(opsSopPath) when present, and cloneRepository(opsSopRepoURL, opsSopPath) when missing) but change the function name and exported visibility if needed so callers clearly understand it performs cloning/updating, not a pure presence check.cmd/ai/sre_agent/validate_cluster.go (1)
37-41: ⚡ Quick winConsider defining organization IDs in a single location.
The allowed organization IDs are duplicated in the description (lines 23-25), here in code, and again in
sre_agent.go(lines 27-29). If these IDs ever need to change, you'll need to update them in multiple places.♻️ Suggestion: Define once and reference
Consider defining these in a shared constant or variable in one file and referencing it from the description and validation logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/validate_cluster.go` around lines 37 - 41, The hard-coded slice knownOrgIDs is duplicated; extract it into a single shared symbol (e.g., an exported package-level variable or constant named AllowedOrgIDs) and replace the inline knownOrgIDs in validate_cluster.go and the duplicate list in sre_agent.go and the description to reference that one symbol; ensure the new symbol has the appropriate visibility for where it’s referenced and update all uses (validateCluster logic that reads knownOrgIDs and the description generation that lists allowed org IDs) to use AllowedOrgIDs so changes are made only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ai/sre_agent/helper.go`:
- Around line 117-133: gitPullAndValidate currently only prints a warning and
ignores the error from running "git status --porcelain"; change it so after the
pull you capture the output and error from exec.Command("git", "-C", repoPath,
"status", "--porcelain").Output(), return a wrapped error if that call fails,
and if the output is non-empty return a descriptive error (e.g.
fmt.Errorf("repository has uncommitted changes: %s",
strings.TrimSpace(string(output)))); update references in the function
gitPullAndValidate (the second cmd variable / output handling) and ensure all
returned errors are wrapped with context.
- Around line 136-140: The promptUserInput function currently ignores the error
from reader.ReadString('\n'); update promptUserInput to return (string, error),
check the error from ReadString and if non-nil return "", err (or a wrapped
fmt.Errorf) after trimming and lowercasing only on success, and update all
callers of promptUserInput to handle the returned error appropriately
(propagate, log, or exit). Ensure you reference the promptUserInput function and
reader.ReadString in your changes so callers are adjusted to the new signature.
- Around line 48-79: The getLatestWheelURL function currently parses the curl
JSON output with string operations; replace that with proper JSON unmarshaling
using encoding/json: run the command to get output as before, define small
structs matching the GitLab release/asset structure (e.g., Release -> Assets ->
Links with fields Name and URL), json.Unmarshal the output into those structs,
iterate the Links slice to find the link with Name == "Python wheel" and return
its URL, and propagate clear errors if unmarshaling fails or no matching link is
found; update error messages in getLatestWheelURL accordingly and remove the
brittle string-index logic.
In `@cmd/ai/sre_agent/sre_agent_config.go`:
- Around line 96-99: The code in checkSreAgentConfig incorrectly treats
updateSopDirectory's false return as a failure; updateSopDirectory only signals
whether it performed an update (true) or no change was needed (false). Modify
checkSreAgentConfig so it does not return false when updateSopDirectory returns
false—either remove the conditional return and ignore the boolean result, or
call updateSopDirectory without treating its return as an error; ensure
references to updateSopDirectory and checkSreAgentConfig are used to locate the
change.
In `@cmd/ai/sre_agent/sre_agent_install.go`:
- Around line 27-30: The code currently calls cmdutil.CheckErr(fmt.Errorf(...))
after os.MkdirAll(baseDir, 0755) and then has a reachable-looking return false
which is actually unreachable because CheckErr exits; remove the redundant
return false so control flow is consistent (keep the CheckErr call and its error
message), or alternatively replace the CheckErr call with direct error handling
that returns false (choose one approach and apply it consistently); locate this
in the block containing os.MkdirAll, baseDir, cmdutil.CheckErr and adjust
accordingly.
In `@cmd/ai/sre_agent/sre_agent.go`:
- Line 97: sreAgentCmd.MarkFlagRequired("pd-url") currently ignores its error
return; capture and handle the error from MarkFlagRequired (called on
sreAgentCmd) by assigning it to a variable (e.g., err) and reacting
appropriately (returning the error from the initializer, logging and exiting, or
calling run-time error handling consistent with surrounding code) so the
function checks and handles any failure from MarkFlagRequired.
In `@cmd/ai/sre_agent/validate_cluster.go`:
- Line 60: The call to validateClusterCmd.MarkFlagRequired("cluster-id") ignores
the returned error; capture its error (e.g., err :=
validateClusterCmd.MarkFlagRequired("cluster-id")) and handle it per existing
command conventions (return the error, log and exit, or use the package's error
helper) so failures are not silently ignored; update the code around
validateClusterCmd.MarkFlagRequired to check err and handle/report it
appropriately.
---
Nitpick comments:
In `@cmd/ai/sre_agent/ops_sop.go`:
- Around line 12-39: Rename the function opsSopIsPresent to a name that reflects
its side effects (e.g., ensureOpsSopReady or setupOpsSopRepo) and update all
call sites accordingly; inside the function, keep the existing logic (checking
utils.FolderExists(opsSopPath), calling gitPullAndValidate(opsSopPath) when
present, and cloneRepository(opsSopRepoURL, opsSopPath) when missing) but change
the function name and exported visibility if needed so callers clearly
understand it performs cloning/updating, not a pure presence check.
In `@cmd/ai/sre_agent/validate_cluster.go`:
- Around line 37-41: The hard-coded slice knownOrgIDs is duplicated; extract it
into a single shared symbol (e.g., an exported package-level variable or
constant named AllowedOrgIDs) and replace the inline knownOrgIDs in
validate_cluster.go and the duplicate list in sre_agent.go and the description
to reference that one symbol; ensure the new symbol has the appropriate
visibility for where it’s referenced and update all uses (validateCluster logic
that reads knownOrgIDs and the description generation that lists allowed org
IDs) to use AllowedOrgIDs so changes are made only once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5b4b4bf-dd36-43ae-bf6a-9f478bb8407f
📒 Files selected for processing (8)
cmd/ai/cmd.gocmd/ai/sre_agent/helper.gocmd/ai/sre_agent/ops_sop.gocmd/ai/sre_agent/sre_agent.gocmd/ai/sre_agent/sre_agent_config.gocmd/ai/sre_agent/sre_agent_install.gocmd/ai/sre_agent/validate_cluster.gocmd/cmd.go
Comment on lines
+48
to
+79
| func getLatestWheelURL() (string, error) { | ||
| cmd := exec.Command("curl", "-s", gitlabReleasesURL) | ||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to fetch releases: %w", err) | ||
| } | ||
|
|
||
| // Parse JSON to find Python wheel URL | ||
| // Looking for: "name":"Python wheel","url":"<wheel_url>" | ||
| outputStr := string(output) | ||
|
|
||
| // Find first occurrence of "Python wheel" link | ||
| wheelIdx := strings.Index(outputStr, `"name":"Python wheel"`) | ||
| if wheelIdx == -1 { | ||
| return "", fmt.Errorf("no Python wheel found in latest release") | ||
| } | ||
|
|
||
| // Find the URL after "Python wheel" | ||
| urlStart := strings.Index(outputStr[wheelIdx:], `"url":"`) | ||
| if urlStart == -1 { | ||
| return "", fmt.Errorf("failed to parse wheel URL") | ||
| } | ||
| urlStart += wheelIdx + len(`"url":"`) | ||
|
|
||
| urlEnd := strings.Index(outputStr[urlStart:], `"`) | ||
| if urlEnd == -1 { | ||
| return "", fmt.Errorf("failed to parse wheel URL end") | ||
| } | ||
|
|
||
| wheelURL := outputStr[urlStart : urlStart+urlEnd] | ||
| return wheelURL, nil | ||
| } |
There was a problem hiding this comment.
Replace manual JSON string parsing with proper JSON unmarshaling.
Using string index operations to parse JSON is fragile and error-prone. The JSON structure could change, field order could vary, or there could be escaped characters that break the parsing. The Go standard library provides encoding/json for reliable JSON parsing.
📦 Proposed fix using encoding/json
+import (
+ "encoding/json"
+)
+
+type GitLabRelease struct {
+ Assets struct {
+ Links []struct {
+ Name string `json:"name"`
+ URL string `json:"url"`
+ } `json:"links"`
+ } `json:"assets"`
+}
+
func getLatestWheelURL() (string, error) {
cmd := exec.Command("curl", "-s", gitlabReleasesURL)
output, err := cmd.Output()
if err != nil {
return "", fmt.Errorf("failed to fetch releases: %w", err)
}
- // Parse JSON to find Python wheel URL
- // Looking for: "name":"Python wheel","url":"<wheel_url>"
- outputStr := string(output)
-
- // Find first occurrence of "Python wheel" link
- wheelIdx := strings.Index(outputStr, `"name":"Python wheel"`)
- if wheelIdx == -1 {
- return "", fmt.Errorf("no Python wheel found in latest release")
- }
-
- // Find the URL after "Python wheel"
- urlStart := strings.Index(outputStr[wheelIdx:], `"url":"`)
- if urlStart == -1 {
- return "", fmt.Errorf("failed to parse wheel URL")
- }
- urlStart += wheelIdx + len(`"url":"`)
-
- urlEnd := strings.Index(outputStr[urlStart:], `"`)
- if urlEnd == -1 {
- return "", fmt.Errorf("failed to parse wheel URL end")
+ var releases []GitLabRelease
+ if err := json.Unmarshal(output, &releases); err != nil {
+ return "", fmt.Errorf("failed to parse releases JSON: %w", err)
+ }
+
+ if len(releases) == 0 {
+ return "", fmt.Errorf("no releases found")
+ }
+
+ // Find Python wheel in the first (latest) release
+ for _, link := range releases[0].Assets.Links {
+ if link.Name == "Python wheel" {
+ return link.URL, nil
+ }
}
- wheelURL := outputStr[urlStart : urlStart+urlEnd]
- return wheelURL, nil
+ return "", fmt.Errorf("no Python wheel found in latest release")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/helper.go` around lines 48 - 79, The getLatestWheelURL
function currently parses the curl JSON output with string operations; replace
that with proper JSON unmarshaling using encoding/json: run the command to get
output as before, define small structs matching the GitLab release/asset
structure (e.g., Release -> Assets -> Links with fields Name and URL),
json.Unmarshal the output into those structs, iterate the Links slice to find
the link with Name == "Python wheel" and return its URL, and propagate clear
errors if unmarshaling fails or no matching link is found; update error messages
in getLatestWheelURL accordingly and remove the brittle string-index logic.
Comment on lines
+117
to
+133
| func gitPullAndValidate(repoPath string) error { | ||
| cmd := exec.Command("git", "-C", repoPath, "pull") | ||
| cmd.Stdout = os.Stderr | ||
| cmd.Stderr = os.Stderr | ||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("git pull failed: %w", err) | ||
| } | ||
|
|
||
| // Validate repository status | ||
| cmd = exec.Command("git", "-C", repoPath, "status", "--porcelain") | ||
| output, _ := cmd.Output() | ||
| if len(output) > 0 { | ||
| fmt.Fprintln(os.Stderr, "Warning: Repository has uncommitted changes") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Function doesn't validate as its name suggests.
The function gitPullAndValidate indicates it will validate the repository state, and the AI summary states it should "fail if git status --porcelain is non-empty after pulling." However, the implementation only prints a warning when uncommitted changes are detected (line 129) and never returns an error. Additionally, the error from git status is silently ignored on line 127.
🔧 Proposed fix to match the function's contract
func gitPullAndValidate(repoPath string) error {
cmd := exec.Command("git", "-C", repoPath, "pull")
cmd.Stdout = os.Stderr
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
return fmt.Errorf("git pull failed: %w", err)
}
// Validate repository status
cmd = exec.Command("git", "-C", repoPath, "status", "--porcelain")
- output, _ := cmd.Output()
+ output, err := cmd.Output()
+ if err != nil {
+ return fmt.Errorf("git status failed: %w", err)
+ }
if len(output) > 0 {
- fmt.Fprintln(os.Stderr, "Warning: Repository has uncommitted changes")
+ return fmt.Errorf("repository has uncommitted changes:\n%s", string(output))
}
return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/helper.go` around lines 117 - 133, gitPullAndValidate
currently only prints a warning and ignores the error from running "git status
--porcelain"; change it so after the pull you capture the output and error from
exec.Command("git", "-C", repoPath, "status", "--porcelain").Output(), return a
wrapped error if that call fails, and if the output is non-empty return a
descriptive error (e.g. fmt.Errorf("repository has uncommitted changes: %s",
strings.TrimSpace(string(output)))); update references in the function
gitPullAndValidate (the second cmd variable / output handling) and ensure all
returned errors are wrapped with context.
Comment on lines
+27
to
+30
| if err := os.MkdirAll(baseDir, 0755); err != nil { | ||
| cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err)) | ||
| return false | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling with cmdutil.CheckErr.
cmdutil.CheckErr typically calls os.Exit(1) when given a non-nil error, which means the return false on line 29 is unreachable. This creates confusion about the error handling flow. Either remove the CheckErr wrapper and handle the error directly, or remove the unreachable return statement.
🔧 Proposed fix with consistent error handling
// Create base directory
if err := os.MkdirAll(baseDir, 0755); err != nil {
- cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err))
+ fmt.Fprintf(os.Stderr, "Failed to create base directory: %v\n", err)
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := os.MkdirAll(baseDir, 0755); err != nil { | |
| cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err)) | |
| return false | |
| } | |
| if err := os.MkdirAll(baseDir, 0755); err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to create base directory: %v\n", err) | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/sre_agent_install.go` around lines 27 - 30, The code
currently calls cmdutil.CheckErr(fmt.Errorf(...)) after os.MkdirAll(baseDir,
0755) and then has a reachable-looking return false which is actually
unreachable because CheckErr exits; remove the redundant return false so control
flow is consistent (keep the CheckErr call and its error message), or
alternatively replace the CheckErr call with direct error handling that returns
false (choose one approach and apply it consistently); locate this in the block
containing os.MkdirAll, baseDir, cmdutil.CheckErr and adjust accordingly.
| } | ||
|
|
||
| validateClusterCmd.Flags().StringVarP(&clusterID, "cluster-id", "C", "", "Cluster ID (internal or external)") | ||
| validateClusterCmd.MarkFlagRequired("cluster-id") |
There was a problem hiding this comment.
Check the error return from MarkFlagRequired.
The error returned by MarkFlagRequired should be checked. While failures are rare (typically only if the flag doesn't exist), Go conventions and the static analysis tool both recommend checking all errors.
✅ Proposed fix
validateClusterCmd.Flags().StringVarP(&clusterID, "cluster-id", "C", "", "Cluster ID (internal or external)")
- validateClusterCmd.MarkFlagRequired("cluster-id")
+ if err := validateClusterCmd.MarkFlagRequired("cluster-id"); err != nil {
+ fmt.Fprintf(os.Stderr, "Error marking flag as required: %v\n", err)
+ os.Exit(1)
+ }📝 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
| validateClusterCmd.MarkFlagRequired("cluster-id") | |
| validateClusterCmd.Flags().StringVarP(&clusterID, "cluster-id", "C", "", "Cluster ID (internal or external)") | |
| if err := validateClusterCmd.MarkFlagRequired("cluster-id"); err != nil { | |
| fmt.Fprintf(os.Stderr, "Error marking flag as required: %v\n", err) | |
| os.Exit(1) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 60-60: Error return value of validateClusterCmd.MarkFlagRequired is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/validate_cluster.go` at line 60, The call to
validateClusterCmd.MarkFlagRequired("cluster-id") ignores the returned error;
capture its error (e.g., err :=
validateClusterCmd.MarkFlagRequired("cluster-id")) and handle it per existing
command conventions (return the error, log and exit, or use the package's error
helper) so failures are not silently ignored; update the code around
validateClusterCmd.MarkFlagRequired to check err and handle/report it
appropriately.
TheUndeadKing
force-pushed
the
SREP-4791
branch
2 times, most recently
from
f3bb554 to
7941c1d
Compare
openshift-ci
Bot
removed
the
needs-rebase
bergmannf
reviewed
May 13, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
cmd/ai/sre_agent/helper.go (2)
45-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the error from
ReadString.The error returned by
reader.ReadString('\n')is silently ignored. If reading from stdin fails (e.g., stdin is closed or encounters an error), the function will return an empty string with no indication that something went wrong, potentially causing unexpected behavior in callers.🛡️ Proposed fix to handle read errors
-func promptUserInput() string { +func promptUserInput() (string, error) { reader := bufio.NewReader(os.Stdin) - input, _ := reader.ReadString('\n') - return strings.ToLower(strings.TrimSpace(input)) + input, err := reader.ReadString('\n') + if err != nil { + return "", fmt.Errorf("failed to read input: %w", err) + } + return strings.ToLower(strings.TrimSpace(input)), nil }Note: This change will require updating all callers of
promptUserInput()to handle the returned error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/helper.go` around lines 45 - 49, The promptUserInput function currently ignores the error from reader.ReadString and should surface it: change promptUserInput to return (string, error), check the error returned by reader.ReadString('\n') and return an appropriate trimmed/lowercased string on success or the error on failure, and then update all callers to handle the new (string, error) signature (e.g., in any place that calls promptUserInput) so read failures are propagated instead of silently returning an empty string.
26-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFunction doesn't validate as its name suggests.
The function
gitPullAndValidateindicates it will validate the repository state. However, the implementation only prints a warning when uncommitted changes are detected (line 38) and never returns an error. Additionally, the error fromgit statusis silently ignored on line 36.🔧 Proposed fix to match the function's contract
func gitPullAndValidate(repoPath string) error { cmd := exec.Command("git", "-C", repoPath, "pull") cmd.Stdout = os.Stderr cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { return fmt.Errorf("git pull failed: %w", err) } // Validate repository status cmd = exec.Command("git", "-C", repoPath, "status", "--porcelain") - output, _ := cmd.Output() + output, err := cmd.Output() + if err != nil { + return fmt.Errorf("git status failed: %w", err) + } if len(output) > 0 { - fmt.Fprintln(os.Stderr, "Warning: Repository has uncommitted changes") + return fmt.Errorf("repository has uncommitted changes:\n%s", string(output)) } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/helper.go` around lines 26 - 42, The function gitPullAndValidate currently ignores errors from the "git status" call and only prints a warning for uncommitted changes; change it so gitPullAndValidate returns an error when repository is not clean and surfaces the git status error instead of ignoring it. Specifically, check and handle the error returned by cmd.Output() from the "git -C repoPath status --porcelain" invocation (return fmt.Errorf with context on failure), and if the output indicates uncommitted changes (non-empty), return a descriptive error rather than only printing to stderr; keep the existing behavior for git pull (exec.Command("git", "-C", repoPath, "pull")) but propagate any errors up through gitPullAndValidate.cmd/ai/sre_agent/sre_agent_config.go (1)
97-99:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncorrect handling of
updateSopDirectoryreturn value.The function
updateSopDirectoryreturnstruewhen the config was updated andfalsewhen no change was needed (see lines 200-223). However, line 97 treats afalsereturn as a failure condition by returningfalsefromcheckSreAgentConfig. This means the configuration setup will fail when the SOP directory is already correct, which is not the intended behavior.🐛 Proposed fix
// Update SOP directory path - if !updateSopDirectory(config, homeDir) { - return false - } + updateSopDirectory(config, homeDir)The return value can be ignored here since
updateSopDirectorynever fails—it only indicates whether an update was made.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/sre_agent_config.go` around lines 97 - 99, The check in checkSreAgentConfig incorrectly treats updateSopDirectory returning false as an error; updateSopDirectory only signals whether it modified the config (true) or no change was needed (false). Remove the conditional failure: call updateSopDirectory(config, homeDir) and ignore its boolean return (or only log the result) rather than returning false from checkSreAgentConfig when it returns false, keeping the rest of the function flow unchanged.cmd/ai/sre_agent/sre_agent_install.go (1)
26-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnreachable return after
cmdutil.CheckErr.
cmdutil.CheckErrcallsos.Exit(1)when given a non-nil error, which means thereturn falseon line 28 is unreachable. This creates confusion about the error handling flow. Either remove theCheckErrwrapper and handle the error directly, or remove the unreachable return statement.🔧 Proposed fix with consistent error handling
// Create base directory if err := os.MkdirAll(baseDir, 0755); err != nil { - cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err)) + fmt.Fprintf(os.Stderr, "Failed to create base directory: %v\n", err) return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/sre_agent_install.go` around lines 26 - 29, The call to cmdutil.CheckErr after os.MkdirAll makes the subsequent return false unreachable because CheckErr exits on error; update the error handling in the function containing this code (the os.MkdirAll call) by choosing one consistent flow: either remove cmdutil.CheckErr and handle the error locally (e.g., log or wrap the error and return false), or keep cmdutil.CheckErr and remove the unreachable return false; specifically edit the block around os.MkdirAll(baseDir, 0755) to use only one of these approaches and ensure the function's return behavior matches (no dead code).cmd/ai/sre_agent/sre_agent.go (1)
85-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck the error return from
MarkFlagRequired.The error returned by
MarkFlagRequiredshould be checked. While failures are rare, Go conventions and the static analysis tool both recommend checking all errors.✅ Proposed fix
// Mark pd-url as required - sreAgentCmd.MarkFlagRequired("pd-url") + if err := sreAgentCmd.MarkFlagRequired("pd-url"); err != nil { + // This should never happen with a valid flag name, but we check for completeness + panic(fmt.Sprintf("failed to mark flag as required: %v", err)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/sre_agent.go` at line 85, MarkFlagRequired call on sreAgentCmd (sreAgentCmd.MarkFlagRequired("pd-url")) ignores its error; change it to capture and handle the returned error according to project conventions: assign the error (e.g., err := sreAgentCmd.MarkFlagRequired("pd-url")), check if err != nil and return or log/exit appropriately within the enclosing init/command setup function. Ensure you reference sreAgentCmd and the "pd-url" flag when locating the call and use the same error handling style used elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ai/sre_agent/helper.go`:
- Around line 12-23: In copyRepository, validate sourcePath before invoking the
external cp: call os.Stat(sourcePath) (and use os.IsNotExist to distinguish
missing paths) and return a clear, contextual error if the path does not exist
or is inaccessible; only proceed to exec.Command("cp", "-r", sourcePath,
destPath) after the check (you can also optionally verify it's a directory via
FileInfo.IsDir()), so errors are deterministic and more informative than the raw
cp failure.
In `@cmd/ai/sre_agent/sre_agent_config.go`:
- Around line 64-67: The call to cmdutil.CheckErr makes the subsequent return
false unreachable; replace the cmdutil.CheckErr(fmt.Errorf(...)) inside the
os.MkdirAll error branch with direct error reporting and a normal return so
control flow matches the rest of the function: log or write the formatted error
message (e.g., via fmt.Fprintf(os.Stderr, "failed to create config directory:
%v\n", err) or the project’s standard error writer) and then return false;
update the branch that uses os.MkdirAll(configDir, 0755) to remove
cmdutil.CheckErr and perform explicit error output followed by return false.
In `@cmd/ai/sre_agent/sre_agent.go`:
- Around line 48-52: The explicit "return" after calling cmdutil.CheckErr in the
Run callback is unreachable because cmdutil.CheckErr exits on error; remove the
redundant "return" following the cmdutil.CheckErr call (the block around
os.UserHomeDir / err handling in sre_agent.go) so the function flow is
clear—keep the CheckErr call as-is and delete the subsequent "return".
---
Duplicate comments:
In `@cmd/ai/sre_agent/helper.go`:
- Around line 45-49: The promptUserInput function currently ignores the error
from reader.ReadString and should surface it: change promptUserInput to return
(string, error), check the error returned by reader.ReadString('\n') and return
an appropriate trimmed/lowercased string on success or the error on failure, and
then update all callers to handle the new (string, error) signature (e.g., in
any place that calls promptUserInput) so read failures are propagated instead of
silently returning an empty string.
- Around line 26-42: The function gitPullAndValidate currently ignores errors
from the "git status" call and only prints a warning for uncommitted changes;
change it so gitPullAndValidate returns an error when repository is not clean
and surfaces the git status error instead of ignoring it. Specifically, check
and handle the error returned by cmd.Output() from the "git -C repoPath status
--porcelain" invocation (return fmt.Errorf with context on failure), and if the
output indicates uncommitted changes (non-empty), return a descriptive error
rather than only printing to stderr; keep the existing behavior for git pull
(exec.Command("git", "-C", repoPath, "pull")) but propagate any errors up
through gitPullAndValidate.
In `@cmd/ai/sre_agent/sre_agent_config.go`:
- Around line 97-99: The check in checkSreAgentConfig incorrectly treats
updateSopDirectory returning false as an error; updateSopDirectory only signals
whether it modified the config (true) or no change was needed (false). Remove
the conditional failure: call updateSopDirectory(config, homeDir) and ignore its
boolean return (or only log the result) rather than returning false from
checkSreAgentConfig when it returns false, keeping the rest of the function flow
unchanged.
In `@cmd/ai/sre_agent/sre_agent_install.go`:
- Around line 26-29: The call to cmdutil.CheckErr after os.MkdirAll makes the
subsequent return false unreachable because CheckErr exits on error; update the
error handling in the function containing this code (the os.MkdirAll call) by
choosing one consistent flow: either remove cmdutil.CheckErr and handle the
error locally (e.g., log or wrap the error and return false), or keep
cmdutil.CheckErr and remove the unreachable return false; specifically edit the
block around os.MkdirAll(baseDir, 0755) to use only one of these approaches and
ensure the function's return behavior matches (no dead code).
In `@cmd/ai/sre_agent/sre_agent.go`:
- Line 85: MarkFlagRequired call on sreAgentCmd
(sreAgentCmd.MarkFlagRequired("pd-url")) ignores its error; change it to capture
and handle the returned error according to project conventions: assign the error
(e.g., err := sreAgentCmd.MarkFlagRequired("pd-url")), check if err != nil and
return or log/exit appropriately within the enclosing init/command setup
function. Ensure you reference sreAgentCmd and the "pd-url" flag when locating
the call and use the same error handling style used elsewhere in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e68ddeed-d9c5-45b1-a8c5-ea2610b47ee9
📒 Files selected for processing (7)
cmd/ai/cmd.gocmd/ai/sre_agent/helper.gocmd/ai/sre_agent/ops_sop.gocmd/ai/sre_agent/sre_agent.gocmd/ai/sre_agent/sre_agent_config.gocmd/ai/sre_agent/sre_agent_install.gocmd/cmd.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ai/cmd.go
- cmd/cmd.go
Comment on lines
+64
to
+67
| if err := os.MkdirAll(configDir, 0755); err != nil { | ||
| cmdutil.CheckErr(fmt.Errorf("failed to create config directory: %w", err)) | ||
| return false | ||
| } |
There was a problem hiding this comment.
Unreachable return after cmdutil.CheckErr.
cmdutil.CheckErr calls os.Exit(1) when given a non-nil error, making the return false on line 66 unreachable. For consistency with the rest of the function's error handling pattern, replace cmdutil.CheckErr with direct error reporting.
🔧 Proposed fix with consistent error handling
// Create config directory
if err := os.MkdirAll(configDir, 0755); err != nil {
- cmdutil.CheckErr(fmt.Errorf("failed to create config directory: %w", err))
+ fmt.Fprintf(os.Stderr, "Failed to create config directory: %v\n", err)
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := os.MkdirAll(configDir, 0755); err != nil { | |
| cmdutil.CheckErr(fmt.Errorf("failed to create config directory: %w", err)) | |
| return false | |
| } | |
| if err := os.MkdirAll(configDir, 0755); err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to create config directory: %v\n", err) | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/sre_agent_config.go` around lines 64 - 67, The call to
cmdutil.CheckErr makes the subsequent return false unreachable; replace the
cmdutil.CheckErr(fmt.Errorf(...)) inside the os.MkdirAll error branch with
direct error reporting and a normal return so control flow matches the rest of
the function: log or write the formatted error message (e.g., via
fmt.Fprintf(os.Stderr, "failed to create config directory: %v\n", err) or the
project’s standard error writer) and then return false; update the branch that
uses os.MkdirAll(configDir, 0755) to remove cmdutil.CheckErr and perform
explicit error output followed by return false.
4b3d4d8 to
1048df2
Compare
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
Author
|
/test all |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
cmd/ai/sre_agent/helper.go (2)
12-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate source path exists before attempting copy.
The function does not check whether
sourcePathexists before invokingcp -r. If the source is missing or inaccessible, thecpcommand will fail with a potentially unclear error message.🛡️ Proposed fix to add source validation
func copyRepository(sourcePath, destPath string) error { + // Validate source exists + if _, err := os.Stat(sourcePath); err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("source path does not exist: %s", sourcePath) + } + return fmt.Errorf("cannot access source path: %w", err) + } + fmt.Fprintf(os.Stderr, "Copying repository to %s...\n", destPath) cmd := exec.Command("cp", "-r", sourcePath, destPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/helper.go` around lines 12 - 23, In copyRepository, validate that sourcePath exists and is accessible before running exec.Command: use os.Stat on sourcePath and if it returns an error (or os.IsNotExist(err)) return a descriptive error mentioning the missing/unreadable sourcePath; only proceed to create and run the cp command (cmd := exec.Command("cp", "-r", sourcePath, destPath)) when the stat succeeds to avoid opaque cp failures.
26-30:⚠️ Potential issue | 🟡 Minor | ⚖️ Poor tradeoffHandle the error from
ReadString.The error returned by
reader.ReadString('\n')is silently ignored. If reading from stdin fails (e.g., stdin is closed or encounters an EOF/error), the function will return an empty string with no indication that something went wrong, potentially causing unexpected behavior in the validation logic.🛡️ Proposed fix to handle read errors
Note: This requires updating the function signature and all callers in
validate_sre_agent.go:26andvalidate_sre_agent_config.go:50.-func promptUserInput() string { +func promptUserInput() (string, error) { reader := bufio.NewReader(os.Stdin) - input, _ := reader.ReadString('\n') - return strings.ToLower(strings.TrimSpace(input)) + input, err := reader.ReadString('\n') + if err != nil { + return "", fmt.Errorf("failed to read input: %w", err) + } + return strings.ToLower(strings.TrimSpace(input)), nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/helper.go` around lines 26 - 30, The promptUserInput function currently ignores the error from reader.ReadString('\n'); change its signature to return (string, error), check and handle the error from ReadString('\n') (return "", err on failure), and only on success return the trimmed lowercase string; then update all callers (validate_sre_agent.go's call at line referenced and validate_sre_agent_config.go's call) to handle the error return (propagate or surface a user-facing error) and adjust any validation flow accordingly.cmd/ai/sre_agent/sre_agent.go (2)
50-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnreachable return after
cmdutil.CheckErr.
cmdutil.CheckErrcallsos.Exit(1)when given a non-nil error, making thereturnon line 51 unreachable. Since theRuncallback has no return value, the explicit return afterCheckErris unnecessary.🔧 Proposed fix
homeDir, err := os.UserHomeDir() if err != nil { cmdutil.CheckErr(fmt.Errorf("failed to get home directory: %w", err)) - return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/sre_agent.go` around lines 50 - 51, Remove the redundant unreachable return after cmdutil.CheckErr: locate the Run callback where cmdutil.CheckErr(fmt.Errorf("failed to get home directory: %w", err)) is called and delete the following explicit "return" statement since cmdutil.CheckErr exits on error and Run has no return value; keep the error check call but remove the dead return line.
80-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck the error return from
MarkFlagRequired.The error returned by
MarkFlagRequiredshould be checked. While failures are rare (typically only programming errors like marking a non-existent flag), Go conventions and the static analysis tool both recommend checking all errors.✅ Proposed fix
// Mark pd-url as required - sreAgentCmd.MarkFlagRequired("pd-url") + if err := sreAgentCmd.MarkFlagRequired("pd-url"); err != nil { + // This should never happen unless there's a programming error + panic(fmt.Sprintf("failed to mark pd-url as required: %v", err)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/sre_agent.go` at line 80, The call to sreAgentCmd.MarkFlagRequired("pd-url") currently ignores its error; capture and handle the returned error (e.g., err := sreAgentCmd.MarkFlagRequired("pd-url") and if err != nil { log.Fatalf(...) } or return the error from init/setup) so failures are not silently ignored. Update the code around sreAgentCmd.MarkFlagRequired to check the error and handle it appropriately (use your existing logger or return the error from the enclosing function) to satisfy Go error-handling conventions.
🧹 Nitpick comments (1)
cmd/ai/sre_agent/sre_agent.go (1)
38-83: Consider the installation approach feedback.A past review comment from bergmannf raised concerns about embedding installation logic in
osdctl, suggesting that tools should be packaged via Homebrew or RPM instead. While the current implementation prompts for an existing installation path rather than automatically downloading/installing, the team should consider whether this copy-based approach aligns with the project's packaging strategy and maintenance model.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/sre_agent.go` around lines 38 - 83, The command currently embeds installation/copy behavior via validateSreAgent and checkSreAgentConfig; instead remove any automatic install/copy attempts and make NewCmdSreAgent only validate presence of a user-managed installation: update validateSreAgent and checkSreAgentConfig to return false with a clear error message directing users to install the tool via supported packaging (Homebrew/RPM) or provide an existing path, ensure executeSreAgent only runs when the binary exists at sreAgentPath, and adjust command help/flags or docs to document the expected packaging/installation approach rather than performing installation in code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ai/sre_agent/validate_sre_agent_config.go`:
- Line 45: Check that sop["directory"] exists and is a string before using it:
replace the silent type assertion currentSopDir, _ := sop["directory"].(string)
with an explicit check that sop["directory"] is present and of type string
(e.g., v, ok := sop["directory"]; s, okType := v.(string)), and if not present
or not a string, return or append a validation error (so the config is rejected
or flagged) rather than proceeding with an empty currentSopDir; update the
validation flow that uses currentSopDir to rely on this guard.
In `@cmd/ai/sre_agent/validate_sre_agent.go`:
- Around line 36-39: The call to cmdutil.CheckErr after os.MkdirAll makes the
subsequent "return false" unreachable because CheckErr exits the process; update
the error handling in the block around os.MkdirAll (in validateSREAgent or the
function containing os.MkdirAll and cmdutil.CheckErr) by either removing the
unreachable "return false" or replacing cmdutil.CheckErr with non-fatal handling
that logs the error and returns false (so the function can continue to return a
boolean instead of exiting).
---
Duplicate comments:
In `@cmd/ai/sre_agent/helper.go`:
- Around line 12-23: In copyRepository, validate that sourcePath exists and is
accessible before running exec.Command: use os.Stat on sourcePath and if it
returns an error (or os.IsNotExist(err)) return a descriptive error mentioning
the missing/unreadable sourcePath; only proceed to create and run the cp command
(cmd := exec.Command("cp", "-r", sourcePath, destPath)) when the stat succeeds
to avoid opaque cp failures.
- Around line 26-30: The promptUserInput function currently ignores the error
from reader.ReadString('\n'); change its signature to return (string, error),
check and handle the error from ReadString('\n') (return "", err on failure),
and only on success return the trimmed lowercase string; then update all callers
(validate_sre_agent.go's call at line referenced and
validate_sre_agent_config.go's call) to handle the error return (propagate or
surface a user-facing error) and adjust any validation flow accordingly.
In `@cmd/ai/sre_agent/sre_agent.go`:
- Around line 50-51: Remove the redundant unreachable return after
cmdutil.CheckErr: locate the Run callback where
cmdutil.CheckErr(fmt.Errorf("failed to get home directory: %w", err)) is called
and delete the following explicit "return" statement since cmdutil.CheckErr
exits on error and Run has no return value; keep the error check call but remove
the dead return line.
- Line 80: The call to sreAgentCmd.MarkFlagRequired("pd-url") currently ignores
its error; capture and handle the returned error (e.g., err :=
sreAgentCmd.MarkFlagRequired("pd-url") and if err != nil { log.Fatalf(...) } or
return the error from init/setup) so failures are not silently ignored. Update
the code around sreAgentCmd.MarkFlagRequired to check the error and handle it
appropriately (use your existing logger or return the error from the enclosing
function) to satisfy Go error-handling conventions.
---
Nitpick comments:
In `@cmd/ai/sre_agent/sre_agent.go`:
- Around line 38-83: The command currently embeds installation/copy behavior via
validateSreAgent and checkSreAgentConfig; instead remove any automatic
install/copy attempts and make NewCmdSreAgent only validate presence of a
user-managed installation: update validateSreAgent and checkSreAgentConfig to
return false with a clear error message directing users to install the tool via
supported packaging (Homebrew/RPM) or provide an existing path, ensure
executeSreAgent only runs when the binary exists at sreAgentPath, and adjust
command help/flags or docs to document the expected packaging/installation
approach rather than performing installation in code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 370a85fd-2ec3-4694-8662-a588c6bc162c
📒 Files selected for processing (6)
cmd/ai/cmd.gocmd/ai/sre_agent/helper.gocmd/ai/sre_agent/sre_agent.gocmd/ai/sre_agent/validate_sre_agent.gocmd/ai/sre_agent/validate_sre_agent_config.gocmd/cmd.go
✅ Files skipped from review due to trivial changes (1)
- cmd/cmd.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ai/cmd.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/ai/sre_agent/validate_sre_agent.go (1)
40-42:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInconsistent error handling violates function contract.
While the unreachable
return falsestatement was removed from the previous version, the underlying issue remains:cmdutil.CheckErrcallsos.Exit(1)on error, which is inconsistent with the rest of this function's error handling pattern (lines 29, 36, 48 all returnfalse). This violates the function's boolean return contract and causes the process to exit instead of allowing the caller to handle the failure.🔧 Proposed fix
// Create base directory if err := os.MkdirAll(baseDir, 0755); err != nil { - cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err)) + fmt.Fprintf(os.Stderr, "Failed to create base directory: %v\n", err) + return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sre_agent/validate_sre_agent.go` around lines 40 - 42, The code calls cmdutil.CheckErr after os.MkdirAll which exits the process, breaking this function's boolean-return contract; replace the cmdutil.CheckErr call with non-fatal error handling that mirrors the other branches (e.g., log the error including context for baseDir and return false). Specifically, in the block that currently uses cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err)), remove the CheckErr call, emit an error via the same logging mechanism used elsewhere (or fmt.Fprintf(os.Stderr, ...)), and then return false so callers can handle the failure; reference os.MkdirAll, baseDir and cmdutil.CheckErr to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ai/sre_agent/validate_sre_agent.go`:
- Around line 24-30: The prompt accepts userVenvPath from promptUserInput but
doesn't validate it's absolute; update the validation after reading userVenvPath
(in the same block that currently checks err) to use
filepath.IsAbs(userVenvPath) and, if false, print a clear error to os.Stderr
(e.g., "path must be absolute") and return false (or re-prompt if desired) so
downstream file/copy operations use a guaranteed absolute path; ensure you
import "path/filepath" if not already present and reference userVenvPath and
promptUserInput in the change.
---
Duplicate comments:
In `@cmd/ai/sre_agent/validate_sre_agent.go`:
- Around line 40-42: The code calls cmdutil.CheckErr after os.MkdirAll which
exits the process, breaking this function's boolean-return contract; replace the
cmdutil.CheckErr call with non-fatal error handling that mirrors the other
branches (e.g., log the error including context for baseDir and return false).
Specifically, in the block that currently uses
cmdutil.CheckErr(fmt.Errorf("failed to create base directory: %w", err)), remove
the CheckErr call, emit an error via the same logging mechanism used elsewhere
(or fmt.Fprintf(os.Stderr, ...)), and then return false so callers can handle
the failure; reference os.MkdirAll, baseDir and cmdutil.CheckErr to locate and
update the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a66b92a9-e5b0-42cc-9680-b414e2d4e707
📒 Files selected for processing (8)
cmd/ai/sre_agent/helper.gocmd/ai/sre_agent/sre_agent.gocmd/ai/sre_agent/validate_sre_agent.gocmd/ai/sre_agent/validate_sre_agent_config.godocs/README.mddocs/osdctl.mddocs/osdctl_ai.mddocs/osdctl_ai_sre-agent.md
✅ Files skipped from review due to trivial changes (1)
- docs/osdctl.md
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/ai/sre_agent/sre_agent.go
- cmd/ai/sre_agent/helper.go
- cmd/ai/sre_agent/validate_sre_agent_config.go
Contributor
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
bergmannf
reviewed
May 18, 2026
| ) | ||
|
|
||
| // validateSreAgent checks if sre-agent is installed | ||
| func validateSreAgent(homeDir string) bool { |
Contributor
There was a problem hiding this comment.
I guess this should just be replaced with a call to a sre-agent binary and if this doesn't succeed, prompt the user to install it and make the binary available?
This would then leave it up to the user, if they want to use venvs + a script, or a aliased docker container.
bergmannf
reviewed
May 18, 2026
| // checkSreAgentConfig validates config.yaml and updates ops-sop path if needed | ||
| func checkSreAgentConfig(homeDir string) bool { | ||
| baseDir := filepath.Join(homeDir, ".local/share/sre-agent") | ||
| configPath := filepath.Join(homeDir, ".config/sre-agent/config.yaml") |
Contributor
There was a problem hiding this comment.
If we really depend on this - on Linux at least - we should not reinvent the wheel and use https://pkg.go.dev/github.com/adrg/xdg to use the XDG directories to find config and data files.
geowa4
suggested changes
May 18, 2026
Contributor
geowa4
left a comment
geowa4
left a comment
There was a problem hiding this comment.
IMO this kind of thing shouldn't be in osdctl. It makes another layer that is brittle by its nature. If sre-agent changes at all, this breaks. It also doesn't solve logistics like installation or versioning so it's a fraught abstraction. However, if this orchestrated some kind of workflow, I'd be open to hearing an argument for it.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ai/sre_agent/sre_agent.go`:
- Around line 48-57: Replace the Cobra command's Run handler with RunE so
validation failures become command errors: change the anonymous func assigned to
Run into RunE(func(cmd *cobra.Command, args []string) error { ... }), call
validateSreAgent() and checkSreAgentConfig(), and if either returns false return
a descriptive error (e.g., errors.New("sre-agent validation failed") or include
context like "sre-agent config check failed") instead of returning early; ensure
the function ultimately returns nil on success so Cobra propagates failures
correctly.
In `@cmd/ai/sre_agent/validate_sre_agent_config.go`:
- Around line 52-78: Currently the code always calls promptUserInput() to ask
for userOpsSopPath before checking if ops-sop exists; move the prompt and
subsequent validation (promptUserInput(), utils.FolderExists(userOpsSopPath)
checks and the copyRepository call) inside the branch that runs only when
opsSopPath := filepath.Join(baseDir, "ops-sop") does not exist (i.e., inside the
if !utils.FolderExists(opsSopPath) block). Keep the existing error handling
(fmt.Fprintf on read/copy failures and the success/found messages) but remove
the unconditional prompt so the code only asks for the source path when a copy
is actually needed; reference promptUserInput, utils.FolderExists and
copyRepository to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: da1ebb28-9fdf-4440-ae83-e7adb178b4c4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/ai/sre_agent/sre_agent.gocmd/ai/sre_agent/validate_sre_agent.gocmd/ai/sre_agent/validate_sre_agent_config.gogo.mod
Comment on lines
+48
to
+57
| Run: func(cmd *cobra.Command, args []string) { | ||
| // Step 1: Validate sre-agent installation | ||
| if !validateSreAgent() { | ||
| return | ||
| } | ||
|
|
||
| // Step 2: Check/Setup config (includes ops-sop setup) | ||
| if !checkSreAgentConfig() { | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmd/ai/sre_agent/sre_agent.go | head -100Repository: openshift/osdctl
Length of output: 3409
🏁 Script executed:
grep -n "func validateSreAgent\|func checkSreAgentConfig" cmd/ai/sre_agent/sre_agent.goRepository: openshift/osdctl
Length of output: 42
🏁 Script executed:
find cmd/ai/sre_agent -type f -name "*.go" | head -20Repository: openshift/osdctl
Length of output: 202
🏁 Script executed:
grep -n "func validateSreAgent\|func checkSreAgentConfig" cmd/ai/sre_agent/*.goRepository: openshift/osdctl
Length of output: 216
🏁 Script executed:
cat -n cmd/ai/sre_agent/validate_sre_agent.goRepository: openshift/osdctl
Length of output: 1896
🏁 Script executed:
cat -n cmd/ai/sre_agent/validate_sre_agent_config.goRepository: openshift/osdctl
Length of output: 3601
Use RunE to propagate validation failures as command errors.
When validateSreAgent() or checkSreAgentConfig() return false, the Run function returns early without signaling an error. This causes the command to exit with success (code 0) despite validation failures, breaking automation and scripting workflows. Migrate to RunE and return explicit errors.
Proposed fix
- Run: func(cmd *cobra.Command, args []string) {
+ RunE: func(cmd *cobra.Command, args []string) error {
// Step 1: Validate sre-agent installation
if !validateSreAgent() {
- return
+ return fmt.Errorf("sre-agent installation validation failed")
}
// Step 2: Check/Setup config (includes ops-sop setup)
if !checkSreAgentConfig() {
- return
+ return fmt.Errorf("sre-agent configuration validation failed")
}
// Step 3: Execute sre-agent
sreAgentPath := filepath.Join(xdg.DataHome, "sre-agent/venv/bin/sre-agent")
sreAgentArgs := buildSreAgentArgs(args)
- err := executeSreAgent(sreAgentPath, sreAgentArgs, outputDir)
- if err != nil {
- cmdutil.CheckErr(err)
- }
+ return executeSreAgent(sreAgentPath, sreAgentArgs, outputDir)
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/sre_agent.go` around lines 48 - 57, Replace the Cobra
command's Run handler with RunE so validation failures become command errors:
change the anonymous func assigned to Run into RunE(func(cmd *cobra.Command,
args []string) error { ... }), call validateSreAgent() and
checkSreAgentConfig(), and if either returns false return a descriptive error
(e.g., errors.New("sre-agent validation failed") or include context like
"sre-agent config check failed") instead of returning early; ensure the function
ultimately returns nil on success so Cobra propagates failures correctly.
Comment on lines
+52
to
+78
| // Ask user for ops-sop repository path | ||
| fmt.Fprintln(os.Stderr, "\nChecking ops-sop repository...") | ||
| fmt.Fprint(os.Stderr, "Enter the absolute path to ops-sop repository: ") | ||
| userOpsSopPath, err := promptUserInput() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to read input: %v\n", err) | ||
| return false | ||
| } | ||
|
|
||
| // Validate path exists | ||
| if !utils.FolderExists(userOpsSopPath) { | ||
| fmt.Fprintln(os.Stderr, "\nThe provided ops-sop path does not exist.") | ||
| return false | ||
| } | ||
|
|
||
| opsSopPath := filepath.Join(baseDir, "ops-sop") | ||
|
|
||
| // Copy ops-sop if not present | ||
| if !utils.FolderExists(opsSopPath) { | ||
| if err := copyRepository(userOpsSopPath, opsSopPath); err != nil { | ||
| fmt.Fprintf(os.Stderr, "\nCopy failed: %v\n", err) | ||
| return false | ||
| } | ||
| fmt.Fprintln(os.Stderr, "✓ ops-sop copied successfully") | ||
| } else { | ||
| fmt.Fprintln(os.Stderr, "✓ ops-sop repository found") | ||
| } |
There was a problem hiding this comment.
Avoid unconditional prompting; only ask for source path when copy is actually needed.
The command currently prompts every run, even when ops-sop is already present under XDG data. That makes routine execution unnecessarily interactive.
💡 Proposed fix
- // Ask user for ops-sop repository path
- fmt.Fprintln(os.Stderr, "\nChecking ops-sop repository...")
- fmt.Fprint(os.Stderr, "Enter the absolute path to ops-sop repository: ")
- userOpsSopPath, err := promptUserInput()
- if err != nil {
- fmt.Fprintf(os.Stderr, "Failed to read input: %v\n", err)
- return false
- }
-
- // Validate path exists
- if !utils.FolderExists(userOpsSopPath) {
- fmt.Fprintln(os.Stderr, "\nThe provided ops-sop path does not exist.")
- return false
- }
-
opsSopPath := filepath.Join(baseDir, "ops-sop")
// Copy ops-sop if not present
if !utils.FolderExists(opsSopPath) {
+ fmt.Fprintln(os.Stderr, "\nChecking ops-sop repository...")
+ fmt.Fprint(os.Stderr, "Enter the absolute path to ops-sop repository: ")
+ userOpsSopPath, err := promptUserInput()
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "Failed to read input: %v\n", err)
+ return false
+ }
+ if !utils.FolderExists(userOpsSopPath) {
+ fmt.Fprintln(os.Stderr, "\nThe provided ops-sop path does not exist.")
+ return false
+ }
if err := copyRepository(userOpsSopPath, opsSopPath); err != nil {
fmt.Fprintf(os.Stderr, "\nCopy failed: %v\n", err)
return false
}
fmt.Fprintln(os.Stderr, "✓ ops-sop copied successfully")📝 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
| // Ask user for ops-sop repository path | |
| fmt.Fprintln(os.Stderr, "\nChecking ops-sop repository...") | |
| fmt.Fprint(os.Stderr, "Enter the absolute path to ops-sop repository: ") | |
| userOpsSopPath, err := promptUserInput() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to read input: %v\n", err) | |
| return false | |
| } | |
| // Validate path exists | |
| if !utils.FolderExists(userOpsSopPath) { | |
| fmt.Fprintln(os.Stderr, "\nThe provided ops-sop path does not exist.") | |
| return false | |
| } | |
| opsSopPath := filepath.Join(baseDir, "ops-sop") | |
| // Copy ops-sop if not present | |
| if !utils.FolderExists(opsSopPath) { | |
| if err := copyRepository(userOpsSopPath, opsSopPath); err != nil { | |
| fmt.Fprintf(os.Stderr, "\nCopy failed: %v\n", err) | |
| return false | |
| } | |
| fmt.Fprintln(os.Stderr, "✓ ops-sop copied successfully") | |
| } else { | |
| fmt.Fprintln(os.Stderr, "✓ ops-sop repository found") | |
| } | |
| opsSopPath := filepath.Join(baseDir, "ops-sop") | |
| // Copy ops-sop if not present | |
| if !utils.FolderExists(opsSopPath) { | |
| fmt.Fprintln(os.Stderr, "\nChecking ops-sop repository...") | |
| fmt.Fprint(os.Stderr, "Enter the absolute path to ops-sop repository: ") | |
| userOpsSopPath, err := promptUserInput() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to read input: %v\n", err) | |
| return false | |
| } | |
| if !utils.FolderExists(userOpsSopPath) { | |
| fmt.Fprintln(os.Stderr, "\nThe provided ops-sop path does not exist.") | |
| return false | |
| } | |
| if err := copyRepository(userOpsSopPath, opsSopPath); err != nil { | |
| fmt.Fprintf(os.Stderr, "\nCopy failed: %v\n", err) | |
| return false | |
| } | |
| fmt.Fprintln(os.Stderr, "✓ ops-sop copied successfully") | |
| } else { | |
| fmt.Fprintln(os.Stderr, "✓ ops-sop repository found") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ai/sre_agent/validate_sre_agent_config.go` around lines 52 - 78,
Currently the code always calls promptUserInput() to ask for userOpsSopPath
before checking if ops-sop exists; move the prompt and subsequent validation
(promptUserInput(), utils.FolderExists(userOpsSopPath) checks and the
copyRepository call) inside the branch that runs only when opsSopPath :=
filepath.Join(baseDir, "ops-sop") does not exist (i.e., inside the if
!utils.FolderExists(opsSopPath) block). Keep the existing error handling
(fmt.Fprintf on read/copy failures and the success/found messages) but remove
the unconditional prompt so the code only asks for the source path when a copy
is actually needed; reference promptUserInput, utils.FolderExists and
copyRepository to locate the changes.
Contributor
|
/retest |
Contributor
Author
|
/test all |
Contributor
|
@TheUndeadKing: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
3 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
Integrates the sre-agent Python tool as a new
osdctl ai sre-agentcommand with automatic installation, configuration, and validation capabilities.Files Added
cmd/ai/sre_agent/sre_agent.go- Main command and orchestrationcmd/ai/sre_agent/sre_agent_install.go- Installation logiccmd/ai/sre_agent/sre_agent_config.go- Configuration managementcmd/ai/sre_agent/helper.go- Shared utilities (git, venv, prompts)Summary by CodeRabbit
New Features
Documentation