Refactor/consolidated packages - #3
Conversation
- cmd/fix.go: agent.New() → ai.NewAgent() - cmd/ask.go: llm.* → ai.*, config → core - cmd/explain.go: storage.* → core.*, llm.* → ai.*
- Add Tab/Shift+Tab key handling for tab navigation - Fix getFocusLabel to return capitalized labels - Correct TestModel_ModeFromTab expectations (agent starts in normal mode)
These packages have been consolidated into core/ and ai/: - internal/textutil → core/config.go (text utilities) - internal/agent → ai/agent.go (autonomous agent)
📝 WalkthroughWalkthroughThis PR introduces a major architectural restructuring that consolidates AI/LLM functionality into Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Shell
participant CLI as CLI Command
participant Engine as Workflow Engine
participant Checkpoint as Checkpoint Store
participant EventBus as Event Bus
participant Executor as Step Executor
participant RollbackReg as Rollback Registry
User->>CLI: run workflow.yaml
CLI->>Engine: Run(ctx, workflow)
Engine->>Checkpoint: LoadRun(runID) or create new
Engine->>EventBus: Publish(WorkflowStart)
rect rgb(200, 220, 255)
Note over Engine,Executor: Execute Steps
loop For each step
Engine->>Engine: ShouldSkip(step)?
alt Step skipped
Engine->>EventBus: Publish(StepSkipped)
else Execute step
Engine->>Executor: Execute step with timeout
Executor-->>Engine: StepResult
Engine->>Checkpoint: SaveStepResult
alt Step failed
Engine->>Engine: DetermineFailureAction
alt Action = Rollback
Engine->>RollbackReg: ExecuteAll(ctx)
RollbackReg-->>Engine: RollbackResults[]
Engine->>EventBus: Publish(WorkflowRollback)
else Action = Abort
Engine->>EventBus: Publish(WorkflowComplete/Failed)
Engine-->>CLI: Return error
else Action = Continue
Engine->>EventBus: Publish(StepFailed)
end
else Step succeeded
Engine->>EventBus: Publish(StepSuccess)
end
end
end
end
Engine->>Checkpoint: SaveRun(finalState)
Engine->>EventBus: Publish(WorkflowComplete)
Engine-->>CLI: RunResult
CLI-->>User: Display summary
sequenceDiagram
participant Client as Hybrid Client
participant Perplexity as Perplexity API
participant Ollama as Ollama (Local)
participant Cache as Response Cache
Client->>Cache: Get(query)
alt Cache hit
Cache-->>Client: CachedResult
else Cache miss
Client->>Client: needsWebSearch(query)?
alt Web search likely
Client->>Perplexity: Research(query)
Perplexity-->>Client: ResearchResult
alt Success
Client->>Cache: Set(query, result)
Cache-->>Client: OK
else Perplexity unavailable/error
Client->>Ollama: Research(query)
Ollama-->>Client: ResearchResult
Client->>Cache: Set(query, result)
end
else Local sufficient
Client->>Ollama: Research(query)
Ollama-->>Client: ResearchResult
Client->>Cache: Set(query, result)
end
end
Client-->>Client: Return result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 23758428 | Triggered | Generic High Entropy Secret | 2257420 | internal/llm/sanitizer_test.go | View secret |
| 23758724 | Triggered | MongoDB Credentials | 2257420 | internal/llm/sanitizer_test.go | View secret |
| 23758725 | Triggered | JSON Web Token | 2257420 | internal/llm/sanitizer_test.go | View secret |
| 23758429 | Triggered | Generic Password | 2257420 | internal/llm/sanitizer_test.go | View secret |
| 23758726 | Triggered | GitHub Personal Access Token | 2257420 | internal/llm/sanitizer_test.go | View secret |
| 23758727 | Triggered | Redis Credentials | 2257420 | internal/llm/sanitizer_test.go | View secret |
| 23758432 | Triggered | Generic Password | 2257420 | internal/llm/sanitizer_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
internal/llm/cache.go (1)
1-202: Duplicate ofinternal/ai/cache.go.As noted in the review of
internal/ai/cache.go, this file contains an essentially identical implementation. Both should be consolidated into a shared package to eliminate duplication.The same suggestions apply here:
- Use
sort.Sliceinstead of manual bubble sort inGetTopHits(lines 162-168)- Consider
RWMutexusage patterns
🟠 Major comments (20)
internal/infra/services_test.go-34-42 (1)
34-42: Remove or completeTestCheckServices_LocalListener—test currently performs no assertions.The test creates a TCP listener and immediately closes it without any verification logic. This is functionally a no-op and serves no testing purpose. Either implement the intended local listener test case with proper assertions, or remove this stub entirely to avoid confusion.
internal/tools/git_tools.go-184-200 (1)
184-200: Multiple command injection points ingetDiff.The
refparameter is used in three different commands without validation.internal/tools/git_tools.go-140-156 (1)
140-156: Command injection risk withpathparameter in blame.Same issue as above -
pathis user-controlled and interpolated directly into the shell command.internal/tools/package_tools.go-131-164 (1)
131-164: Same command injection risk inoutdatedaction.Apply similar quoting/escaping for the path in this command as well.
internal/core/config.go-132-137 (1)
132-137: Potential panic ifmaxLenis less than 20.If
maxLen < 20andlen(output) > maxLen, the expressionoutput[:maxLen-20]will have a negative index, causing a runtime panic.🔎 Proposed fix
func TruncateOutput(output string, maxLen int) string { if len(output) <= maxLen { return output } + if maxLen < 20 { + return output[:maxLen] + } return output[:maxLen-20] + "\n...[truncated]..." }internal/tools/git_tools.go-109-138 (1)
109-138: Command injection risk withrefparameter.The
refparameter is directly interpolated into the git command. A malicious ref like"; rm -rf /; "could execute arbitrary commands.🔎 Proposed fix using validation
func (t *GitInfoTool) getLog(params map[string]any, start time.Time) ToolResult { count := GetInt(params, "count", 10) ref := GetString(params, "ref", "HEAD") + + // Validate ref contains only safe characters + if strings.ContainsAny(ref, ";|&$`\\\"'<>(){}[]!") { + return NewErrorResult("invalid ref: contains disallowed characters", time.Since(start)) + } cmd := "git log --pretty=format:'%h|%an|%ad|%s' --date=short -n " + strconv.Itoa(count) + " " + refSimilar validation should be applied to all git commands that use user-provided
reforpathparameters.internal/tools/package_tools.go-96-121 (1)
96-121: Command injection risk with unescaped path.
absPathis interpolated directly into the shell command. Paths containing spaces, semicolons, backticks, or other shell metacharacters could break execution or enable injection.🔎 Proposed fix using shell quoting
- result := executor.ExecuteSimple("cd " + absPath + " && go list -m -f '{{.Path}}@{{.Version}}' all 2>/dev/null | head -100") + result := executor.ExecuteSimple("cd '" + strings.ReplaceAll(absPath, "'", "'\\''") + "' && go list -m -f '{{.Path}}@{{.Version}}' all 2>/dev/null | head -100")Alternatively, consider changing the working directory programmatically before executing the command, or using a safer command execution API that doesn't involve shell interpolation.
internal/pipeline/graph_pager.go-112-114 (1)
112-114: NextCursor points to the last item of current page, causing duplicate on next fetch.When
HasMoreis true,NextCursoris set to the last item's ID in the current page. The nextGetPagecall will start at that ID (line 91-92), including it again. The cursor should point to the next item after the last one in the page.🔎 Proposed fix
if page.HasMore && len(pageNodes) > 0 { - page.NextCursor = pageNodes[len(pageNodes)-1].ID + // Point to the next item after the current page + if endIdx < len(nodesAtLevel) { + page.NextCursor = nodesAtLevel[endIdx].ID + } }Committable suggestion skipped: line range outside the PR's diff.
internal/core/events.go-206-211 (1)
206-211:LastErrorstores a pointer to a copy - safe but potentially confusing.Line 209 takes a pointer to the local copy
block(the function parameter), which is valid since it's stored ins.Blocksslice first. However, ifs.Blocksis reallocated (e.g., during eviction on line 200), the pointer may become stale.🔎 Proposed fix - store pointer to the slice element
s.Blocks = append(s.Blocks, block) s.blockIndex[block.ID] = len(s.Blocks) - 1 s.SelectedIdx = len(s.Blocks) - 1 if block.ExitCode != 0 { - s.LastError = &block + s.LastError = &s.Blocks[len(s.Blocks)-1] }internal/core/rca.go-245-265 (1)
245-265: Race condition inUpdateRunbookStats- read-modify-write is not atomic.This function reads the runbook, modifies it in memory, then writes back. Concurrent calls could lose updates. Consider using a single
UPDATEquery with SQL arithmetic.🔎 Proposed fix using atomic SQL update
func UpdateRunbookStats(db *sql.DB, id string, success bool) error { - rb, err := GetRunbookByID(db, id) - if err != nil { - return err - } - if rb == nil { - return fmt.Errorf("runbook not found: %s", id) - } - - rb.UsageCount++ - if success { - rb.SuccessRate = ((rb.SuccessRate * float64(rb.UsageCount-1)) + 1.0) / float64(rb.UsageCount) - } else { - rb.SuccessRate = (rb.SuccessRate * float64(rb.UsageCount-1)) / float64(rb.UsageCount) - } - rb.LastUsed = time.Now() - - query := `UPDATE runbooks SET success_rate = ?, last_used = ?, usage_count = ? WHERE id = ?` - _, err = db.Exec(query, rb.SuccessRate, rb.LastUsed.Unix(), rb.UsageCount, id) - return err + successVal := 0 + if success { + successVal = 1 + } + query := `UPDATE runbooks SET + usage_count = usage_count + 1, + success_rate = (success_rate * usage_count + ?) / (usage_count + 1), + last_used = ? + WHERE id = ?` + result, err := db.Exec(query, successVal, time.Now().Unix(), id) + if err != nil { + return err + } + rows, _ := result.RowsAffected() + if rows == 0 { + return fmt.Errorf("runbook not found: %s", id) + } + return nil }internal/hook/zsh.go-143-146 (1)
143-146: Background subshell won't update parent shell variables.
(sleep 0.1 && __devops_check_resolution) &!runs in a subshell, so changes to__DEVOPS_LAST_FAILURE_IDand__DEVOPS_LAST_FAILURE_CMDwon't propagate to the parent shell. The variables will remain empty until the next synchronous call to__devops_check_resolution.Consider calling
__devops_check_resolutionsynchronously or using a different mechanism.🔎 Proposed fix
if [[ $exit_code -ne 0 && $exit_code -ne 130 ]]; then - # Command failed - check for unresolved failure after a short delay - (sleep 0.1 && __devops_check_resolution) &! - + # Command failed - update failure tracking synchronously + __devops_check_resolution + # Try smart suggestion firstIf the delay is needed to ensure the failure is logged before checking, the log-event call should be synchronous or you need a different approach.
internal/tools/command_tools.go-35-53 (1)
35-53: Context parameter is ignored;cwdparameter has no effect.
- The
ctxparameter is passed toExecutebut never used.executor.ExecuteWithTimeoutcreates its own context internally, so caller-provided cancellation/deadline won't propagate.- The
cwdparameter is extracted on line 43 (not shown but implied byCommandResult.Cwd) but never passed to the executor—commands will always run in the current working directory.🔎 Proposed fix to honor context and working directory
func (t *RunCommandTool) Execute(ctx context.Context, params map[string]any) ToolResult { start := time.Now() command := GetString(params, "command", "") if command == "" { return NewErrorResult("command is required", time.Since(start)) } timeout := GetDuration(params, "timeout", 60*time.Second) + cwd := GetString(params, "cwd", "") - result := executor.ExecuteWithTimeout(command, timeout) + // Use context with timeout that respects the caller's context + execCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + result := executor.ExecuteWithContext(execCtx, command, cwd) return NewResult(CommandResult{ Command: result.Command, Output: result.Output, ExitCode: result.ExitCode, Duration: result.Duration.String(), - Cwd: result.Cwd, + Cwd: cwd, }, time.Since(start)) }Note: You'll need to update
ExecuteWithContext(or create a variant) to accept a working directory parameter if it doesn't already support one.Committable suggestion skipped: line range outside the PR's diff.
internal/storage/rca_models.go-73-81 (1)
73-81: Use standard librarystrings.IndexRuneinstead of customindexOf.Go's standard library provides
strings.IndexRune(s, r)which does exactly what this helper does.🔎 Proposed fix
+import "strings" func GenerateErrorSignature(command string, exitCode int, output string) string { firstLine := output - if idx := indexOf(output, '\n'); idx > 0 { + if idx := strings.IndexRune(output, '\n'); idx > 0 { firstLine = output[:idx] } ... } - -// indexOf finds the first occurrence of a rune in a string -func indexOf(s string, r rune) int { - for i, c := range s { - if c == r { - return i - } - } - return -1 -}Committable suggestion skipped: line range outside the PR's diff.
internal/storage/rca_models.go-57-71 (1)
57-71: DuplicateGenerateErrorSignaturefunction exists across two packages.This function is defined in both
internal/core/rca.go(line 52) andinternal/storage/rca_models.go(line 59) with identical logic. While they use different helper functions (indexOfRunevsindexOf), the core implementation is the same, creating maintenance burden and risk of divergence.Consolidate to a single implementation. Consider which package should own this function based on architectural boundaries.
internal/storage/rca_models.go-35-43 (1)
35-43: Consolidate duplicateRunbookStepstruct and related functions across packages.
RunbookStep,GenerateErrorSignature, and helper functions (indexOf/indexOfRune) are duplicated ininternal/core/rca.goandinternal/storage/rca_models.go. While these packages maintain separate namespaces and don't cause import conflicts, the code duplication creates maintenance burden and inconsistency. Define these types and functions in a shared location (e.g., a commoninternal/models/rca.go) and import them in both packages, or consolidate them in one authoritative location that both core and storage depend on.internal/workflow/checkpoint.go-35-48 (1)
35-48:INSERT OR REPLACEconflicts withAUTOINCREMENTprimary key.The
workflow_step_resultstable usesINTEGER PRIMARY KEY AUTOINCREMENT, butSaveStepResultusesINSERT OR REPLACE. Sinceidis auto-generated and not provided in the INSERT, this will always insert a new row rather than replace an existing one. To enable upsert behavior, add a unique constraint on(run_id, step_id)and useON CONFLICTinstead.🔎 Proposed fix
CREATE TABLE IF NOT EXISTS workflow_step_results ( id INTEGER PRIMARY KEY AUTOINCREMENT, run_id TEXT NOT NULL, step_id TEXT NOT NULL, status TEXT NOT NULL, exit_code INTEGER, output TEXT, error TEXT, retries INTEGER DEFAULT 0, started_at DATETIME, completed_at DATETIME, duration_ms INTEGER, - FOREIGN KEY (run_id) REFERENCES workflow_runs(id) + FOREIGN KEY (run_id) REFERENCES workflow_runs(id), + UNIQUE(run_id, step_id) );And update
SaveStepResult:- INSERT OR REPLACE INTO workflow_step_results + INSERT INTO workflow_step_results (run_id, step_id, status, exit_code, output, error, retries, started_at, completed_at, duration_ms) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(run_id, step_id) DO UPDATE SET + status = excluded.status, + exit_code = excluded.exit_code, + output = excluded.output, + error = excluded.error, + retries = excluded.retries, + started_at = excluded.started_at, + completed_at = excluded.completed_at, + duration_ms = excluded.duration_msinternal/workflow/engine.go-345-350 (1)
345-350: Samedefer cancel()issue in rollback loop.Apply the same fix as in
executeStepto avoid accumulating deferred cancel calls.🔎 Proposed fix
rollbackCtx := ctx + var cancel context.CancelFunc if step.Rollback.Timeout > 0 { - var cancel context.CancelFunc rollbackCtx, cancel = context.WithTimeout(ctx, step.Rollback.Timeout) - defer cancel() } result := executor.ExecuteWithContext(rollbackCtx, step.Rollback.Command) + + if cancel != nil { + cancel() + }internal/workflow/engine.go-286-291 (1)
286-291:defer cancel()inside loop causes deferred calls to accumulate.Each iteration defers a
cancel()call, but these won't execute until the function returns. This accumulates resources and the contexts won't be canceled promptly. Extract to a helper function or callcancel()explicitly after use.🔎 Proposed fix
stepCtx := ctx + var cancel context.CancelFunc if step.Timeout > 0 { - var cancel context.CancelFunc stepCtx, cancel = context.WithTimeout(ctx, step.Timeout) - defer cancel() } execResult := executor.ExecuteWithContext(stepCtx, step.Command) + + if cancel != nil { + cancel() + }Committable suggestion skipped: line range outside the PR's diff.
internal/storage/rca_repository.go-182-206 (1)
182-206:UpdateRunbookStatshas a read-modify-write race condition.Concurrent calls to
UpdateRunbookStatsfor the same runbook can cause lost updates. The read, modify, and write are not atomic. Consider using a single SQL UPDATE with arithmetic expressions.🔎 Proposed fix
func UpdateRunbookStats(db *sql.DB, id string, success bool) error { - rb, err := GetRunbookByID(db, id) - if err != nil { - return err - } - if rb == nil { - return fmt.Errorf("runbook not found: %s", id) - } - - rb.UsageCount++ - if success { - rb.SuccessRate = ((rb.SuccessRate * float64(rb.UsageCount-1)) + 1.0) / float64(rb.UsageCount) - } else { - rb.SuccessRate = (rb.SuccessRate * float64(rb.UsageCount-1)) / float64(rb.UsageCount) - } - rb.LastUsed = time.Now() - - query := `UPDATE runbooks SET success_rate = ?, last_used = ?, usage_count = ? WHERE id = ?` - _, err = db.Exec(query, rb.SuccessRate, rb.LastUsed.Unix(), rb.UsageCount, id) - return err + var query string + if success { + query = `UPDATE runbooks SET + success_rate = (success_rate * usage_count + 1.0) / (usage_count + 1), + usage_count = usage_count + 1, + last_used = ? + WHERE id = ?` + } else { + query = `UPDATE runbooks SET + success_rate = (success_rate * usage_count) / (usage_count + 1), + usage_count = usage_count + 1, + last_used = ? + WHERE id = ?` + } + result, err := db.Exec(query, time.Now().Unix(), id) + if err != nil { + return err + } + rows, _ := result.RowsAffected() + if rows == 0 { + return fmt.Errorf("runbook not found: %s", id) + } + return nil }internal/ai/cache.go-1-184 (1)
1-184: Consolidate duplicate cache implementation withinternal/llm/cache.go.This file is nearly identical to
internal/llm/cache.go—both define the same types and methods with identical logic. Only the package name, comments, and type definition order differ. This duplication violates DRY and creates maintenance burden.Move the shared cache logic to
internal/cache(or similar shared package) and have bothaiandllmpackages import from it.Additionally, consider these code quality improvements:
- Replace the O(n²) bubble sort in
GetTopHits()(lines 147–153) with a more efficient sorting algorithm.- Avoid repeated slice allocations in
moveToFront()when prepending to the order slice.
🟡 Minor comments (15)
.golangci.yml-33-33 (1)
33-33: Remove theexportloopreflinter; it is redundant.The project requires Go 1.25.4, which is well after Go 1.22. The loop variable scoping issue that
exportlooprefchecks for was fixed in Go 1.22, making this linter unnecessary.internal/tui/app_test.go-200-212 (1)
200-212: Test doesn't verify actual Escape key handling for Insert mode.This test expects Escape to switch from Insert to Normal mode, but because the agent tab is never actually placed in Insert mode, the Escape key handler in
internal/tui/tabs/agent/update.go(lines 98-100, which only executes whenInsertModeis true) never runs. The test passes becausegetModeFromTab()correctly returnsModeNormalwhen the agent is not in Insert mode—validating the fallback synchronization rather than the Escape key transition itself.To properly test Insert→Normal mode transitions, first put the agent in Insert mode (e.g., by sending the Insert key or directly calling
agent.SetInsertMode(true)), then send Escape and verify the mode changes.internal/core/config.go-46-51 (1)
46-51: Handleos.UserHomeDir()error to avoid writing to root filesystem.If
UserHomeDir()fails,homeis empty, resulting inLogDir = "/.devlogs"which could attempt to write to the root filesystem, causing permission errors or unintended behavior.🔎 Proposed fix
if val := os.Getenv("DEV_CLI_LOG_DIR"); val != "" { cfg.LogDir = val } else { - home, _ := os.UserHomeDir() - cfg.LogDir = filepath.Join(home, ".devlogs") + if home, err := os.UserHomeDir(); err == nil { + cfg.LogDir = filepath.Join(home, ".devlogs") + } else { + cfg.LogDir = ".devlogs" // fallback to current directory + } }internal/tools/package_tools.go-110-120 (1)
110-120: IncorrectDirectfield logic for Go modules.The first package (
i == 0) is the main module itself, not a dependency. All entries fromgo list -m allafter the first are dependencies, butgo list -m alldoesn't distinguish direct vs indirect dependencies.To accurately detect direct dependencies, use
go list -m -f '{{if not .Indirect}}...{{end}}' all.🔎 Proposed fix
- result := executor.ExecuteSimple("cd " + absPath + " && go list -m -f '{{.Path}}@{{.Version}}' all 2>/dev/null | head -100") + result := executor.ExecuteSimple("cd " + absPath + " && go list -m -f '{{.Path}}@{{.Version}}@{{.Indirect}}' all 2>/dev/null | head -100") packages := make([]PackageInfo, 0) directCount := 0 lines := strings.Split(result.Output, "\n") - for i, line := range lines { + for _, line := range lines { line = strings.Trim(line, "'") if line == "" { continue } - parts := strings.Split(line, "@") - if len(parts) == 2 { + parts := strings.Split(line, "@") + if len(parts) >= 2 { + isDirect := len(parts) < 3 || parts[2] != "true" pkg := PackageInfo{ Name: parts[0], Version: parts[1], - Direct: i == 0, + Direct: isDirect, }Committable suggestion skipped: line range outside the PR's diff.
internal/pipeline/graph_pager.go-88-96 (1)
88-96: Cursor not found silently starts from index 0.If the provided cursor ID doesn't exist in
nodesAtLevel, the loop completes without finding it andstartIdxremains 0. This could lead to unexpected pagination behavior. Consider returning an error or documenting this fallback.🔎 Proposed fix - return error for invalid cursor
startIdx := 0 if cursor != "" { + found := false for i, n := range nodesAtLevel { if n.ID == cursor { startIdx = i + found = true break } } + if !found { + return nil, fmt.Errorf("cursor not found: %s", cursor) + } }Note: This would require adding
"fmt"to imports.internal/tools/tools_test.go-123-148 (1)
123-148: Unchecked error from os.WriteFile.The error from
os.WriteFileat line 127 is ignored. While unlikely to fail in test scenarios, it's good practice to check for errors.🔎 Suggested fix
- os.WriteFile(testFile, []byte("original"), 0644) + if err := os.WriteFile(testFile, []byte("original"), 0644); err != nil { + t.Fatal(err) + }cmd/mark_resolved.go-55-75 (1)
55-75: Silent error exits may hinder debugging.The
check-last-failurecommand exits silently (code 1) on errors without any message to stderr. While this may be intentional for scripting purposes (hidden command), it makes troubleshooting difficult when things fail unexpectedly.🔎 Suggested improvement
Run: func(cmd *cobra.Command, args []string) { db, err := storage.InitDB() if err != nil { + fmt.Fprintln(os.Stderr, "error: failed to open database") os.Exit(1) } defer db.Close() failure, err := storage.GetLastUnresolvedFailure(db) if err != nil { + fmt.Fprintln(os.Stderr, "error: failed to query failures") os.Exit(1) } if failure == nil { os.Exit(1) }internal/workflow/condition.go-64-75 (1)
64-75:parseIntFromStringhas edge cases: empty string returns 0, negative numbers fail silently.
- Empty string
""returns(true, nil)with*result = 0, which may be unexpected.- Negative values like
"-1"silently fail (returnsfalse, nil).Consider using
strconv.Atoiwhich handles these cases properly.🔎 Proposed fix
+import "strconv" // matchExitCode checks if an exit code matches the condition value. func matchExitCode(exitCode int, value string) bool { if value == "!0" { return exitCode != 0 } - var expected int - if _, err := parseIntFromString(value, &expected); err != nil { - return false - } + expected, err := strconv.Atoi(value) + if err != nil { + return false + } return exitCode == expected } - -// parseIntFromString is a helper to parse int from string. -func parseIntFromString(s string, result *int) (bool, error) { - n := 0 - for _, ch := range s { - if ch < '0' || ch > '9' { - return false, nil - } - n = n*10 + int(ch-'0') - } - *result = n - return true, nil -}Committable suggestion skipped: line range outside the PR's diff.
cmd/doctor.go-163-166 (1)
163-166: Output ordering issue: summary header appears after per-check details.In non-JSON mode, the "🔍 dev-cli doctor" header and separator are printed after all individual check results have already been output (lines 119-141). This results in a confusing user experience where the header appears at the bottom rather than the top.
🔎 Proposed fix - move header before the check loop
func runDoctor(cmd *cobra.Command, args []string) { + if !doctorJSON { + fmt.Println("\033[1m🔍 dev-cli doctor\033[0m") + fmt.Println("\033[90m────────────────────────────────\033[0m") + fmt.Println() + } + checks := []func() CheckResult{ checkDocker, ...And remove lines 164-165 from the current location.
Committable suggestion skipped: line range outside the PR's diff.
internal/ai/sanitizer.go-103-107 (1)
103-107: Global sanitizer is not thread-safe for concurrentAddPatterncalls.
globalSanitizeris a package-level variable thatSanitizeForLLMreads from. IfAddPatternis called concurrently from multiple goroutines, it could cause a data race. Consider adding a mutex or documenting thatAddPatternis not safe for concurrent use.Also applies to: 120-131
internal/ai/sanitizer.go-137-143 (1)
137-143:TruncateForLLMcan panic or produce malformed output for smallmaxLen.Similar to the checkpoint's
truncateString, ifmaxLen <= 20,halfbecomes 0 or negative. Add a guard.🔎 Proposed fix
func TruncateForLLM(input string, maxLen int) string { if len(input) <= maxLen { return input } + if maxLen <= 20 { + return input[:maxLen] + } half := (maxLen - 20) / 2 return input[:half] + "\n...[truncated]...\n" + input[len(input)-half:] }internal/tools/search_tools.go-95-101 (1)
95-101:TotalCountreflects truncated count, not actual matches found.Setting
TotalCounttolen(matches)after truncation doesn't reflect the actual number of matches. Consider tracking the pre-truncation count for accurate reporting.🔎 Proposed fix
matches := parseRipgrepJSON(string(output)) + totalFound := len(matches) truncated := false if len(matches) > maxResults { matches = matches[:maxResults] truncated = true } return NewResult(SearchResult{ Pattern: pattern, Path: searchPath, Matches: matches, - TotalCount: len(matches), + TotalCount: totalFound, Truncated: truncated, }, time.Since(start))Committable suggestion skipped: line range outside the PR's diff.
internal/core/executor.go-329-341 (1)
329-341: Useerrors.Isfor context error comparison.Comparing errors with
==can fail if the error is wrapped. Useerrors.Isfor reliable comparison.🔎 Proposed fix
+import "errors" + if err != nil { if exitError, ok := err.(*exec.ExitError); ok { exitCode = exitError.ExitCode() - } else if err == context.DeadlineExceeded { + } else if errors.Is(err, context.DeadlineExceeded) { exitCode = 124 outputStr = "Command timed out" } else {Committable suggestion skipped: line range outside the PR's diff.
internal/tools/docker_tools.go-189-198 (1)
189-198: Potential panic on short container ID.The code slices
c.ID[:12]without verifying the ID length. If a container ID is unexpectedly shorter than 12 characters, this will panic.🔎 Proposed fix
containers := make([]DockerContainerInfo, 0, len(health.Containers)) for _, c := range health.Containers { + id := c.ID + if len(id) > 12 { + id = id[:12] + } containers = append(containers, DockerContainerInfo{ - ID: c.ID[:12], + ID: id, Name: strings.TrimPrefix(c.Name, "/"), Image: c.Image, State: c.State, Status: c.Status, }) }internal/core/executor.go-31-35 (1)
31-35: Global mutable state without synchronization.
globalDBis a package-level variable modified bySetDatabase()and read byExecuteAndLogWithTimeout(). If these are called from different goroutines, this could cause a data race.🔎 Proposed fix using atomic pointer or mutex
+import "sync/atomic" + -var globalDB *sql.DB +var globalDB atomic.Pointer[sql.DB] func SetDatabase(db *sql.DB) { - globalDB = db + globalDB.Store(db) }Then use
globalDB.Load()when accessing.Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (55)
.golangci.yml (2)
26-26: Remove or clarify the inline reference tag.The comment includes
[web:101]which appears to be a reference tag but is embedded directly in the YAML comment string, making it unclear. If this is meant to be a reference, consider either removing it or documenting what it refers to in a separate comment.🔎 Suggested clarification
- - godoclint # (New 2025) Validates comments against Go standards [web:101] + - godoclint # (New 2025) Validates comments against Go standardsOr if the reference is important, document it separately:
+ # godoclint reference: web:101 for latest documentation - godoclint # (New 2025) Validates comments against Go standards
61-62: Consider setting limits for max-issues.Both
max-issues-per-linterandmax-same-issuesare set to 0 (unlimited). While this may be intentional for this major refactoring PR to surface all issues at once, unlimited issues can be overwhelming in practice and may cause developers to miss critical problems in the noise.Consider setting reasonable limits once the initial refactoring is complete.
internal/tui/tabs/monitor/model.go (1)
210-212: Remove dead code.Line 212 assigns
6to the blank identifier, which has no effect. This appears to be leftover from refactoring and should be removed.🔎 Proposed fix
servicesHeight := (panelHeight - 8) / 2 imagesHeight := (panelHeight - 8) / 2 - _ = 6internal/core/config.go (1)
60-60: Consider lazy initialization forCurrentConfig.Package-level initialization at import time may complicate testing scenarios where environment variables need to be set before config load. Consider a
sync.Oncepattern or explicit initialization.internal/llm/ollama.go (1)
354-359: DuplicateToolCallResulttype definition.This struct is also defined in
internal/ai/client.go(lines 58-62) with identical fields. Consider consolidating into a shared package to avoid duplication and potential drift.internal/storage/db.go (1)
51-148: Consider enabling foreign key enforcement.SQLite does not enforce foreign key constraints by default. Without
PRAGMA foreign_keys = ON, the foreign key definitions inworkflow_step_resultsandroot_causesare purely documentary.🔎 Proposed fix
func migrate(db *sql.DB) error { + // Enable foreign key enforcement + if _, err := db.Exec("PRAGMA foreign_keys = ON"); err != nil { + return fmt.Errorf("enable foreign keys: %w", err) + } + schema := ` CREATE TABLE IF NOT EXISTS history (internal/tools/git_tools.go (1)
202-240: Status parsing duplicatesGitInspectorToollogic.The porcelain status parsing in
getStatusis nearly identical toGitInspectorTool.Execute. Consider extracting a shared helper function.internal/pipeline/graph_pager.go (1)
206-212:truncateStringcounts bytes, not runes - may split multi-byte characters.Using
len(s)counts bytes. For strings with multi-byte UTF-8 characters, this could truncate mid-character, producing invalid UTF-8.🔎 Proposed fix using runes
func truncateString(s string, maxLen int) string { - if len(s) <= maxLen { + runes := []rune(s) + if len(runes) <= maxLen { return s } - return s[:maxLen] + "..." + return string(runes[:maxLen]) + "..." }internal/core/rca.go (2)
65-72: Usestrings.IndexRuneinstead of reimplementing.The standard library provides
strings.IndexRunewith the same behavior, which is more idiomatic and avoids maintaining custom code.🔎 Proposed fix
+import "strings" ... func GenerateErrorSignature(command string, exitCode int, output string) string { firstLine := output - if idx := indexOfRune(output, '\n'); idx > 0 { + if idx := strings.IndexRune(output, '\n'); idx > 0 { firstLine = output[:idx] } ... - -func indexOfRune(s string, r rune) int { - for i, c := range s { - if c == r { - return i - } - } - return -1 -}
143-165: Silent fallback for JSON unmarshal errors may mask data corruption.When JSON unmarshalling fails, the code silently defaults to empty slices. Consider logging these errors for debugging data issues.
Also applies to: 167-186
internal/core/events.go (1)
80-84: Consider defining a constant for the wildcard subscriber key.Using
"*"as a magic string for wildcard subscriptions works but could be made more explicit with a constant.🔎 Proposed improvement
+const eventWildcard EventType = "*" + func (e *EventBus) SubscribeAll(handler EventHandler) { e.mu.Lock() defer e.mu.Unlock() - e.subscribers["*"] = append(e.subscribers["*"], handler) + e.subscribers[eventWildcard] = append(e.subscribers[eventWildcard], handler) }And update line 95 similarly.
internal/pipeline/state_test.go (1)
53-55: Rune conversion for IDs only works for limited range.Using
string(rune('a' + i))orstring(rune('0' + i))for block IDs only produces valid single characters for small ranges (26 letters, 10 digits). The concurrent test (line 283) usesid % 26which is safe, but this pattern can be fragile.🔎 Consider using fmt.Sprintf for clarity
- store.AddBlock(Block{ID: string(rune('a' + i))}) + store.AddBlock(Block{ID: fmt.Sprintf("block-%d", i)})Also applies to: 107-108, 155-156, 283-283
cmd/ask.go (1)
49-52: Consider handling the Ollama error more gracefully.When
EnsureOllamaRunning()fails, a warning is printed but execution continues (empty else block). The tool mode (fetchCommands) still requires Ollama and will fail later. Consider either returning early or making this clearer to users.internal/storage/db_test.go (1)
119-121: Consider testing with different resolution values.The test uses a generic
"solution"string. Consider adding a case with a more realistic resolution value (e.g., the actual fix command or JSON payload) to ensure special characters are handled correctly.internal/workflow/condition_test.go (1)
76-86: Consider adding a test for regex that doesn't match.There's a test for
output_matchessucceeding, but no test for when the regex doesn't match. This would improve confidence in the negative case handling.🔎 Suggested additional test case
{ name: "output_matches regex - no match", cond: &Condition{ Type: CondOutputMatches, Value: `version \d+\.\d+`, }, result: &StepResult{ Output: "no version info here", }, expected: false, },internal/pipeline/events_test.go (1)
89-107: Consider using a more readable BlockID generation approach.The BlockID generation using
string(rune('a' + i))works but could be clearer.🔎 Suggested improvement
for i := 0; i < 5; i++ { bus.Publish(Event{ Type: EventCommandOutput, - BlockID: string(rune('a' + i)), + BlockID: string('a' + byte(i)), }) }Or use
fmt.Sprintf("%c", 'a'+i)for better readability.cmd/workflow.go (3)
77-83: Consider cleaning up signal notification on completion.The signal channel is registered but never cleaned up with
signal.Stop(). While this is acceptable for short-lived CLI commands, it's good practice to clean up signal handlers.🔎 Suggested improvement
sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigCh) go func() { <-sigCh fmt.Println("\n⏸ Received interrupt, saving checkpoint...") cancel() }()
77-83: Duplicate signal handling code could be extracted.The signal handling setup is duplicated between
workflowRunCmdandworkflowResumeCmd. Consider extracting to a helper function.🔎 Suggested refactor
func setupInterruptHandler(ctx context.Context, cancel context.CancelFunc) { sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) go func() { select { case <-sigCh: fmt.Println("\n⏸ Received interrupt, saving checkpoint...") cancel() case <-ctx.Done(): } signal.Stop(sigCh) }() }Also applies to: 138-144
370-408: Consider logging when workflow file parsing fails during search.When
workflow.ParseFilefails at line 397, the error is silently ignored. This could make debugging difficult when a workflow file exists but has syntax errors.🔎 Suggested improvement
wf, err := workflow.ParseFile(fullPath) if err != nil { + // Could add verbose logging here if needed continue }Consider adding verbose logging when
workflowVerboseis enabled to help with troubleshooting.internal/workflow/parser_test.go (1)
171-182: Consider strengthening the uniqueness test.The current test only compares two consecutive IDs. While this is sufficient for basic validation, generating more IDs would increase confidence in the uniqueness guarantee.
🔎 Suggested improvement
func TestGenerateRunID(t *testing.T) { - id1 := GenerateRunID() - id2 := GenerateRunID() - - if id1 == id2 { - t.Error("GenerateRunID() should return unique IDs") - } - - if !strings.HasPrefix(id1, "run_") { - t.Errorf("GenerateRunID() = %q, want prefix 'run_'", id1) + seen := make(map[string]bool) + for i := 0; i < 100; i++ { + id := GenerateRunID() + if !strings.HasPrefix(id, "run_") { + t.Errorf("GenerateRunID() = %q, want prefix 'run_'", id) + } + if seen[id] { + t.Errorf("GenerateRunID() returned duplicate: %s", id) + } + seen[id] = true } }internal/tools/tools_test.go (1)
219-227: Magic number for expected default tools count.The test expects exactly 10 default tools. This could break if new tools are added or removed. Consider either:
- Using a constant from the production code
- Testing for a minimum count instead of exact match
- Adding a comment explaining the expected tools
🔎 Alternative approaches
t.Run("RegisterDefaults", func(t *testing.T) { reg := NewRegistry() reg.RegisterDefaults() - if reg.Count() != 10 { - t.Errorf("expected 10 default tools, got %d", reg.Count()) + // Verify at least the core tools are registered + if reg.Count() < 5 { + t.Errorf("expected at least 5 default tools, got %d", reg.Count()) } })Or document the expected tools in a comment for maintainability.
internal/workflow/safemode_test.go (1)
149-159: Use standard librarystrings.Containsinstead of custom implementation.The custom
containsandcontainsSubstrhelper functions duplicate functionality already available in the standard library.🔎 Suggested fix
+import ( + "strings" + "testing" +) + // In TestSafeMode_GetPreviewSummary: - if !contains(summary, "destructive") { + if !strings.Contains(summary, "destructive") { t.Error("summary should mention destructive actions") } -} - -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || containsSubstr(s, substr)) -} - -func containsSubstr(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false }internal/tools/tool.go (2)
26-33: Type comment is incomplete.The
Typefield comment lists"string, int, bool, []string, []int"but"duration"is also used (seecommand_tools.goline 21). Consider updating the comment to reflect all supported types.🔎 Suggested update
ToolParam struct { Name string `json:"name"` - Type string `json:"type"` // string, int, bool, []string, []int + Type string `json:"type"` // string, int, bool, duration, []string, []int Description string `json:"description"` Required bool `json:"required"` Default any `json:"default,omitempty"` }
78-91: Potential integer overflow when converting float64/int64 to int.When parsing JSON, numbers are often decoded as
float64. Large values could overflow when cast toint. This is unlikely to be a practical issue for typical tool parameters, but worth noting.internal/storage/repository.go (1)
201-217: Consider validating theresolutionparameter.The comment states valid values are
"solution","unrelated","skipped", but the function accepts any string. Invalid values could lead to inconsistent data.🔎 Proposed validation
// MarkResolution updates the resolution status of a history entry. // Valid values: "solution", "unrelated", "skipped" func MarkResolution(db *sql.DB, id int64, resolution string) error { + switch resolution { + case "solution", "unrelated", "skipped": + // valid + default: + return fmt.Errorf("invalid resolution value: %q", resolution) + } + query := `UPDATE history SET resolution = ? WHERE id = ?`internal/workflow/parser.go (2)
182-190: ID generation may collide under high concurrency.
generateID()andGenerateRunID()usetime.Now().UnixNano()which can produce duplicates if called within the same nanosecond (especially in concurrent scenarios or on systems with low-resolution clocks).Consider adding randomness or using a UUID library for guaranteed uniqueness.
🔎 Proposed fix using crypto/rand
+import ( + "crypto/rand" + "encoding/hex" +) // generateID creates a simple unique ID based on timestamp. func generateID() string { - return fmt.Sprintf("wf_%d", time.Now().UnixNano()) + b := make([]byte, 8) + rand.Read(b) + return fmt.Sprintf("wf_%d_%s", time.Now().UnixNano(), hex.EncodeToString(b)) } // GenerateRunID creates a unique run ID. func GenerateRunID() string { - return fmt.Sprintf("run_%d", time.Now().UnixNano()) + b := make([]byte, 8) + rand.Read(b) + return fmt.Sprintf("run_%d_%s", time.Now().UnixNano(), hex.EncodeToString(b)) }
147-180: Validation does not detect cycles in step references.The validation checks that
on_successandon_failurereference valid step IDs, but doesn't detect cycles (e.g., step A → step B → step A). This could cause infinite loops in the workflow engine.Would you like me to generate a cycle detection algorithm for the step graph?
internal/storage/rca_test.go (1)
131-140:UpdateRunbookStatstest only verifiesUsageCount, notSuccessRate.The test passes
truefor the success parameter but doesn't verify thatSuccessRatewas recalculated correctly. Consider adding an assertion for the success rate.🔎 Proposed enhancement
updated, _ := GetRunbookByID(db, "rb-001") if updated.UsageCount != 11 { t.Errorf("expected usage count 11, got %d", updated.UsageCount) } + // Initial: 10 uses at 0.9 success = 9 successes + // After: 11 uses with 1 more success = 10 successes + // Expected rate: 10/11 ≈ 0.909 + if updated.SuccessRate < 0.9 || updated.SuccessRate > 0.92 { + t.Errorf("expected success rate ~0.909, got %f", updated.SuccessRate) + }internal/workflow/condition.go (2)
86-93: Non-deterministic "latest result" whenStepRefis empty.When
c.StepRef == "", the code iterates overresultsmap to find the result with the latestCompletedAt. However, Go map iteration order is undefined, so if multiple results have the sameCompletedAt, the selected result is non-deterministic.This is unlikely to cause issues in practice if
CompletedAtvalues are unique, but worth noting.
29-34: Regex compilation error is silently ignored.If the regex pattern in
c.Valueis invalid,regexp.MatchStringreturns an error that's discarded. The condition will evaluate tofalse, which may confuse users debugging workflow execution.Consider logging or returning the error for better observability.
🔎 Example with error logging
case CondOutputMatches: if lastResult == nil { return false } - matched, _ := regexp.MatchString(c.Value, lastResult.Output) + matched, err := regexp.MatchString(c.Value, lastResult.Output) + if err != nil { + // Invalid regex pattern - log for debugging + // log.Printf("warning: invalid regex pattern %q: %v", c.Value, err) + return false + } return matchedinternal/hook/zsh.go (1)
98-99: Parsing assumesid|cmdformat without validation.If
dev-cli check-last-failurereturns malformed output (e.g., missing|),${failure_info%%|*}and${failure_info#*|}could produce unexpected results. Consider adding format validation.🔎 Example validation
+ if [[ "$failure_info" != *"|"* ]]; then + __DEVOPS_LAST_FAILURE_ID="" + __DEVOPS_LAST_FAILURE_CMD="" + return + fi __DEVOPS_LAST_FAILURE_ID="${failure_info%%|*}" __DEVOPS_LAST_FAILURE_CMD="${failure_info#*|}"internal/pipeline/events.go (1)
8-8:EventTypeis defined in bothpipeline/events.goandcore/events.go, but only the pipeline version is used.The
core/events.godefinitions are unused in the application. All references to event types in the codebase usepipeline.EventTypeandpipeline.EventBus. Consider removing the duplicate event type definitions fromcore/events.goto avoid confusion and clarify thatpipelineis the single source of truth for event handling.cmd/doctor.go (1)
155-157: Consider logging the JSON encoding error.The error from
enc.Encode(report)is silently discarded. While encoding to stdout is unlikely to fail, consider at least logging the error for debugging purposes.🔎 Proposed fix
- _ = enc.Encode(report) + if err := enc.Encode(report); err != nil { + fmt.Fprintf(os.Stderr, "error encoding JSON: %v\n", err) + }internal/tools/network_tools.go (1)
54-72: Consider validating port numbers are within valid range.Ports should be in the range 1-65535. Invalid port numbers could cause unexpected behavior when passed to
infra.CheckPortAvailable.🔎 Proposed fix
for _, port := range ports { + if port < 1 || port > 65535 { + return NewErrorResult(fmt.Sprintf("invalid port number: %d", port), time.Since(start)) + } status := PortStatus{Port: port}internal/tools/registry.go (1)
129-151: Potential for sorted/deterministic schema output.
GetSchemas()andGetSchemasJSON()iterate over a map which has non-deterministic order in Go. For LLM prompts, consider sorting the tools by name before generating schemas to ensure consistent output across runs.🔎 Proposed fix for deterministic ordering
func (r *Registry) GetSchemas() []ToolSchema { r.mu.RLock() defer r.mu.RUnlock() tools := make([]Tool, 0, len(r.tools)) for _, tool := range r.tools { tools = append(tools, tool) } + sort.Slice(tools, func(i, j int) bool { + return tools[i].Name() < tools[j].Name() + }) return GenerateToolsSchema(tools) }internal/workflow/safemode.go (1)
133-142: Simple substring matching may have false positives.The
isDestructivecheck usesstrings.Containswhich could match unintended commands. For example,"delete from_backup"would match the"delete from"pattern. Consider whether word boundaries or more precise matching is needed.For stricter matching, you could use regex with word boundaries, but the current approach is likely acceptable given the safety-first philosophy (false positives are better than false negatives for destructive operations).
internal/tools/schema.go (2)
117-124:ParseToolCallprovides no validation of tool existence or parameter types.The function only parses JSON without validating that the tool exists or that parameters match the schema. Consider whether validation should happen here or at the call site.
If validation is expected elsewhere, this is fine. Consider adding a comment clarifying that callers must validate the parsed request against the registry.
48-52: Array items schema only supports string and int types.The current implementation only handles
[]stringand[]intfor array items. If other array types are needed in the future (e.g.,[]bool,[]object), this will need extension.This is fine for current usage but worth documenting the limitation.
internal/core/db.go (1)
147-148: Defensive ALTER TABLE for migration - consider tracking schema versions.The
ALTER TABLE history ADD COLUMN resolution TEXTis executed every time with errors ignored. While this works, a proper schema migration system with version tracking would be more maintainable as the schema evolves.For now, this is acceptable for a simple CLI tool. Consider a migration library (like
golang-migrate) if schema changes become more frequent.cmd/explain.go (1)
163-176: Consider extracting dangerous patterns to a configurable constant.The hardcoded dangerous command patterns are reasonable, but extracting them to a package-level constant would improve maintainability and make it easier to extend or customize.
🔎 Suggested refactor
+var dangerousPatterns = []string{ + "rm -rf", "rm -r /", "dd if=", "mkfs", "> /dev/", "chmod 777", ":(){ :|:& };:", +} + func analyzeEntry(entry core.LogEntry, interactive bool) { // ... if interactive { - dangerousPatterns := []string{"rm -rf", "rm -r /", "dd if=", "mkfs", "> /dev/", "chmod 777", ":(){ :|:& };:"} for _, pattern := range dangerousPatterns {internal/workflow/checkpoint.go (1)
220-271:ListRunsdoes not populateStepResults.While
LoadRunpopulatesStepResults,ListRunscreates empty maps but never loads step results. This is likely intentional for performance, but consider adding a comment to clarify this design decision, or add an optional parameter to load step results when needed.internal/tools/file_tools.go (2)
215-230:copyFilemay leave destination file in inconsistent state on copy error.If
io.Copyfails, the destination file is created but may be incomplete. Also, the original file's permissions are not preserved. Consider syncing and preserving mode.🔎 Proposed fix
func copyFile(src, dst string) error { source, err := os.Open(src) if err != nil { return err } defer source.Close() + srcInfo, err := source.Stat() + if err != nil { + return err + } + dest, err := os.Create(dst) if err != nil { return err } - defer dest.Close() - _, err = io.Copy(dest, source) - return err + if _, err = io.Copy(dest, source); err != nil { + dest.Close() + os.Remove(dst) + return err + } + + if err := dest.Chmod(srcInfo.Mode()); err != nil { + dest.Close() + return err + } + + return dest.Close() }
86-94: Inefficient line filtering for large files.The entire file (up to
maxSize) is read into memory before line filtering is applied. For large files where only a few lines are needed, this is wasteful. Consider using a buffered scanner to read only the required lines.internal/ai/sanitizer.go (1)
109-112: Regex is compiled on every call toMaskEnvVars.The regex is recompiled each time
MaskEnvVarsis called. For a frequently-used function, precompile it as a package-level variable.🔎 Proposed fix
+var sensitiveVarsRegex = regexp.MustCompile(`(?i)(export\s+)?(API_KEY|SECRET|PASSWORD|TOKEN|PRIVATE_KEY|AWS_SECRET)[=]["']?([^\s"'\n]+)["']?`) + func MaskEnvVars(input string) string { - sensitiveVars := regexp.MustCompile(`(?i)(export\s+)?(API_KEY|SECRET|PASSWORD|TOKEN|PRIVATE_KEY|AWS_SECRET)[=]["']?([^\s"'\n]+)["']?`) - return sensitiveVars.ReplaceAllString(input, `$1$2=[REDACTED]`) + return sensitiveVarsRegex.ReplaceAllString(input, `$1$2=[REDACTED]`) }internal/workflow/workflow.go (1)
148-157:LastStepResultiterates all results on every call.This O(n) iteration could be avoided by caching the last result or tracking it during
SetStepResult. For workflows with many steps called frequently, this may impact performance.🔎 Suggested optimization
Track the last result in
SetStepResult:type RunState struct { // ... existing fields + lastResult *StepResult } func (r *RunState) SetStepResult(result *StepResult) { r.StepResults[result.StepID] = result r.UpdatedAt = time.Now() + r.lastResult = result } func (r *RunState) LastStepResult() *StepResult { - var last *StepResult - for _, result := range r.StepResults { - if last == nil || result.CompletedAt.After(last.CompletedAt) { - last = result - } - } - return last + return r.lastResult }internal/tools/search_tools.go (1)
82-86: Ignoring command execution errors may hide real failures.The error from
cmd.Output()is discarded. While ripgrep returns non-zero for no matches, it also returns non-zero for actual errors (e.g., invalid regex). Consider distinguishing between "no matches" and actual errors.🔎 Proposed fix
cmd := exec.CommandContext(ctx, "rg", args...) - output, _ := cmd.Output() + output, err := cmd.Output() + if err != nil { + // rg exits with 1 for no matches, 2 for errors + if exitErr, ok := err.(*exec.ExitError); ok { + if exitErr.ExitCode() == 2 { + return NewErrorResult(fmt.Sprintf("ripgrep error: %s", string(exitErr.Stderr)), time.Since(start)) + } + // Exit code 1 = no matches, continue with empty output + } + }internal/storage/rca_repository.go (2)
77-99: Consider reducing code duplication betweenscanRootCauseandscanRootCauseRow.The two scan functions have nearly identical logic. A common helper could reduce duplication. This pattern repeats for Runbook and ProjectFingerprint scanners as well.
Also applies to: 101-120
83-85: Returning(nil, nil)forsql.ErrNoRowsmay be confusing.Callers must check both return values to distinguish "not found" from success. Consider returning a sentinel error like
ErrNotFoundinstead, or documenting this behavior clearly.internal/ai/cache.go (1)
147-153: Usesort.Sliceinstead of manual bubble sort.The manual O(n²) sort can be replaced with the standard library's more efficient and idiomatic
sort.Slice.🔎 Proposed fix
- for i := 0; i < len(entries)-1; i++ { - for j := i + 1; j < len(entries); j++ { - if entries[j].HitCount > entries[i].HitCount { - entries[i], entries[j] = entries[j], entries[i] - } - } - } + sort.Slice(entries, func(i, j int) bool { + return entries[i].HitCount > entries[j].HitCount + })Add
"sort"to imports.internal/workflow/rollback.go (2)
56-60: UseRLockfor read-onlyCountmethod.
Count()only reads from the slice, so it should useRLock/RUnlockfor better concurrency.🔎 Proposed fix
func (r *RollbackRegistry) Count() int { - r.mu.Lock() - defer r.mu.Unlock() + r.mu.RLock() + defer r.mu.RUnlock() return len(r.hooks) }
160-167: UseRLockfor read-onlyGetHooksmethod.This method only reads from the hooks slice and should use
RLock/RUnlock.🔎 Proposed fix
func (r *RollbackRegistry) GetHooks() []RollbackHook { - r.mu.Lock() - defer r.mu.Unlock() + r.mu.RLock() + defer r.mu.RUnlock() hooks := make([]RollbackHook, len(r.hooks)) copy(hooks, r.hooks) return hooks }internal/core/executor.go (1)
313-316: Ignoring error fromio.Copy.The goroutine discards the error from
io.Copy. While often acceptable for PTY output, logging unexpected errors could help diagnose issues.internal/ai/client.go (3)
765-771: Silently swallowing Perplexity errors may hide issues.When Perplexity fails, the error is discarded and Ollama is used as fallback. Consider logging the error for debugging purposes.
🔎 Proposed fix
if h.perplexity != nil && needsWebSearch(query) { result, err = h.perplexity.Research(context.Background(), query) if err == nil { h.cache.Set(query, result) return result, nil } + // Log fallback for debugging (consider using a logger) + fmt.Fprintf(os.Stderr, "Perplexity fallback: %v\n", err) }
104-133:EnsureOllamaRunninguses hardcoded container configuration.The function hardcodes the container name, port, and volume. Consider making these configurable or at least documenting the assumptions. Also, if the container exists but was created with different settings, this could cause confusion.
678-693: Expired cache entries are not proactively cleaned.The
Getmethod returnsnilfor expired entries but doesn't remove them. Over time, this could leave stale entries consuming memory until evicted by size limit.This is acceptable for a small cache but worth noting.
| func (p *GraphPager) AddNode(node CausalNode) { | ||
| p.nodes[node.ID] = &node | ||
| } |
There was a problem hiding this comment.
Pointer to loop variable stored in map - potential aliasing bug in AddNodes.
In AddNode, you take a pointer to the node parameter. When called from AddNodes (lines 57-61), each iteration reuses the same node variable, so all stored pointers will point to the last node.
🔎 Proposed fix
func (p *GraphPager) AddNode(node CausalNode) {
- p.nodes[node.ID] = &node
+ n := node // create a copy
+ p.nodes[node.ID] = &n
}🤖 Prompt for AI Agents
In internal/pipeline/graph_pager.go around lines 52-54, AddNode currently takes
the address of the parameter `node` which, when called from a loop in AddNodes,
causes all map entries to point to the same reused loop variable; fix by storing
a pointer to a fresh copy or store the value instead: create a new local
variable copy (e.g. copy := node) and store © in the map, or change the map
to store CausalNode values (not *CausalNode) and assign directly, ensuring each
map entry gets its own distinct memory.
| Metadata: map[string]string{ | ||
| "exit_code": string(rune('0' + failure.ExitCode)), | ||
| "working_dir": failure.WorkingDir, |
There was a problem hiding this comment.
Exit code conversion is incorrect for values >= 10.
string(rune('0' + failure.ExitCode)) only works correctly for exit codes 0-9. Exit code 10 would produce : (ASCII 58), not "10".
🔎 Proposed fix
+ "strconv"
...
Metadata: map[string]string{
- "exit_code": string(rune('0' + failure.ExitCode)),
+ "exit_code": strconv.Itoa(failure.ExitCode),
"working_dir": failure.WorkingDir,
"timestamp": failure.Timestamp.Format(time.RFC3339),
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/pipeline/graph_pager.go around lines 197 to 199, the code converts
the exit code with string(rune('0' + failure.ExitCode)) which only works for 0–9
and yields incorrect characters for >=10; replace this with a proper
integer-to-string conversion like strconv.Itoa(failure.ExitCode) (or
fmt.Sprintf("%d", failure.ExitCode)) and add the strconv import if not present,
so Metadata["exit_code"] contains the correct numeric string for all exit codes.
1 participant
Summary by CodeRabbit
New Features
--jsonflag for structured diagnostics.Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.