Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewer's GuideIntroduces an embedded binary distribution mechanism that extracts binaries to ~/.codeactor/bin, wires main to use those embedded binaries (including codeactor-codebase and fzf), and ensures the codebase server process is tracked and cleanly killed on exit, while adjusting module deps accordingly. Sequence diagram for embedded binary extraction and codebase server lifecyclesequenceDiagram
actor User
participant Main
participant distBinFS
participant embedbin
participant OS
participant execCmd as exec.Cmd
User->>Main: main()
Main->>embedbin: ExtractBinaries(distBinFS, dist/bin)
embedbin->>OS: UserHomeDir()
OS-->>embedbin: homeDir
embedbin->>OS: MkdirAll(~/.codeactor/bin)
embedbin->>distBinFS: ReadDir(dist/bin)
distBinFS-->>embedbin: entries
loop each entry
embedbin->>distBinFS: ReadFile(dist/bin/<name>)
distBinFS-->>embedbin: data
embedbin->>OS: WriteFile(~/.codeactor/bin/<name>, data, 0755)
end
embedbin-->>Main: binDir or error
Main->>Main: handle extract error (warn only)
Main->>Main: startCodebaseServer(port, repoPath)
Main->>embedbin: BinPath(codeactor-codebase)
embedbin->>OS: UserHomeDir()
OS-->>embedbin: homeDir
embedbin-->>Main: ~/.codeactor/bin/codeactor-codebase
Main->>OS: Stat(binPath)
OS-->>Main: ok or not exist
alt binary exists
Main->>OS: MkdirAll(~/.codeactor/logs/codeactor-codebase)
Main->>OS: OpenFile(logPath)
OS-->>Main: logFile
Main->>execCmd: exec.Command(binPath, args)
execCmd-->>Main: cmd
Main->>execCmd: Start()
execCmd-->>Main: started
Main-->>User: application running
User->>Main: exit
Main->>execCmd: Process.Kill()
execCmd-->>Main: killed
else binary missing or error
Main-->>User: skip codebase server startup
end
Sequence diagram for ExecuteFileSearch using embedded fzf binarysequenceDiagram
participant Tool as SearchOperationsTool
participant fzfPath
participant embedbin
participant OS
participant execCmd as exec.Cmd
Tool->>Tool: ExecuteFileSearch(ctx, params)
Tool->>Tool: build findOutput
Tool->>fzfPath: fzfPath()
fzfPath->>embedbin: BinPath(fzf)
embedbin->>OS: UserHomeDir()
OS-->>embedbin: homeDir
embedbin-->>fzfPath: ~/.codeactor/bin/fzf or error
alt embedded fzf path ok
fzfPath->>OS: Stat(path)
OS-->>fzfPath: exists
fzfPath-->>Tool: embedded fzf path
else embedded fzf missing or error
fzfPath-->>Tool: fzf
end
Tool->>execCmd: exec.CommandContext(ctx, resolvedPath, ...)
execCmd-->>Tool: cmd
Tool->>execCmd: CombinedOutput()
execCmd-->>Tool: output or error
Tool->>Tool: parse fzf result
Tool-->>Tool: return search result
Class diagram for embedbin utilities and updated startup codeclassDiagram
class embedbin {
+string ExtractBinaries(binFS embed.FS, subDir string)
+string BinPath(name string)
-bool isExecutableName(name string)
}
class MainEmbed {
+distBinFS embed.FS
}
class MainApp {
+main()
+startCodebaseServer(port int, repoPath string) *exec.Cmd
}
class SearchOperationsTool {
-string workingDir
+ExecuteFileSearch(ctx context.Context, params map[string]string) (string, error)
}
class FzfHelper {
+string fzfPath()
}
MainEmbed <.. MainApp : uses
MainApp ..> embedbin : calls ExtractBinaries
MainApp ..> embedbin : calls BinPath
SearchOperationsTool ..> FzfHelper : uses fzfPath
FzfHelper ..> embedbin : calls BinPath
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- In
startCodebaseServer, you now callos.UserHomeDirfor the log directory andembedbin.BinPath(which callsos.UserHomeDiragain) for the binary path; consider deriving the log directory from the bin path or lettingembedbinexpose a helper for the~/.codeactorroot to avoid redundant home-dir resolution and potential divergence. ExtractBinariesstops on the first failure while iterating embedded entries, which can leave the bin directory partially updated; if you want more robustness, consider logging per-file failures and continuing, returning a combined error only if nothing succeeded.ExtractBinariescurrently only iterates the direct children ofsubDirand skips subdirectories; if you plan to support platform- or architecture-specific subfolders underdist/bin, you may want to make this walk recursive or explicitly validate that the layout is flat.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `startCodebaseServer`, you now call `os.UserHomeDir` for the log directory and `embedbin.BinPath` (which calls `os.UserHomeDir` again) for the binary path; consider deriving the log directory from the bin path or letting `embedbin` expose a helper for the `~/.codeactor` root to avoid redundant home-dir resolution and potential divergence.
- `ExtractBinaries` stops on the first failure while iterating embedded entries, which can leave the bin directory partially updated; if you want more robustness, consider logging per-file failures and continuing, returning a combined error only if nothing succeeded.
- `ExtractBinaries` currently only iterates the direct children of `subDir` and skips subdirectories; if you plan to support platform- or architecture-specific subfolders under `dist/bin`, you may want to make this walk recursive or explicitly validate that the layout is flat.
## Individual Comments
### Comment 1
<location path="main.go" line_range="105-107" />
<code_context>
+
// Start codebase server
- startCodebaseServer(codebasePort, repoPath)
+ codebaseCmd := startCodebaseServer(codebasePort, repoPath)
+ if codebaseCmd != nil {
+ defer func() {
+ if err := codebaseCmd.Process.Kill(); err != nil {
+ slog.Warn("Failed to kill codebase process", "error", err)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling already-exited processes and using a gentler shutdown before Kill.
The deferred `Process.Kill()` will run even when the child has already exited, leading to `os.ErrProcessDone`/`ESRCH` and spurious warnings during normal shutdown. Please guard this with a check (e.g., `ProcessState`, `Wait()`, or equivalent) so you only kill a still-running process, and consider sending a softer signal (like `os.Interrupt`/`SIGTERM`) before a hard kill where supported.
Suggested implementation:
```golang
// Start codebase server
codebaseCmd := startCodebaseServer(codebasePort, repoPath)
if codebaseCmd != nil && codebaseCmd.Process != nil {
defer func() {
// Try graceful shutdown first where supported.
if err := codebaseCmd.Process.Signal(os.Interrupt); err != nil {
// If the process is already done, there's nothing to do and no need to warn.
if !errors.Is(err, os.ErrProcessDone) {
slog.Warn("Failed to send interrupt to codebase process", "error", err)
}
}
// Best-effort hard kill if the process is still running.
if err := codebaseCmd.Process.Kill(); err != nil {
// Ignore already-exited processes to avoid spurious warnings.
if !errors.Is(err, os.ErrProcessDone) {
slog.Warn("Failed to kill codebase process", "error", err)
}
} else {
slog.Info("Codebase process killed on exit", "pid", codebaseCmd.Process.Pid)
}
}()
}
```
1. Ensure `main.go` imports `os` and `errors`:
- Add `import "os"` and `import "errors"` to the import block if they are not already present.
2. If you prefer not to always attempt a hard kill after a successful interrupt, you can add a short platform-appropriate check (e.g. a build-tagged helper) to determine whether the interrupt likely stopped the process and skip the `Kill` in that case.
</issue_to_address>
### Comment 2
<location path="internal/tools/search_operations.go" line_range="128" />
<code_context>
fzfCmd := exec.CommandContext(ctx, fzfPath(), "-f", query, "--print-query", "--no-sort", "--tac")
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+105
to
+107
| codebaseCmd := startCodebaseServer(codebasePort, repoPath) | ||
| if codebaseCmd != nil { | ||
| defer func() { |
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling already-exited processes and using a gentler shutdown before Kill.
The deferred Process.Kill() will run even when the child has already exited, leading to os.ErrProcessDone/ESRCH and spurious warnings during normal shutdown. Please guard this with a check (e.g., ProcessState, Wait(), or equivalent) so you only kill a still-running process, and consider sending a softer signal (like os.Interrupt/SIGTERM) before a hard kill where supported.
Suggested implementation:
// Start codebase server
codebaseCmd := startCodebaseServer(codebasePort, repoPath)
if codebaseCmd != nil && codebaseCmd.Process != nil {
defer func() {
// Try graceful shutdown first where supported.
if err := codebaseCmd.Process.Signal(os.Interrupt); err != nil {
// If the process is already done, there's nothing to do and no need to warn.
if !errors.Is(err, os.ErrProcessDone) {
slog.Warn("Failed to send interrupt to codebase process", "error", err)
}
}
// Best-effort hard kill if the process is still running.
if err := codebaseCmd.Process.Kill(); err != nil {
// Ignore already-exited processes to avoid spurious warnings.
if !errors.Is(err, os.ErrProcessDone) {
slog.Warn("Failed to kill codebase process", "error", err)
}
} else {
slog.Info("Codebase process killed on exit", "pid", codebaseCmd.Process.Pid)
}
}()
}- Ensure
main.goimportsosanderrors:- Add
import "os"andimport "errors"to the import block if they are not already present.
- Add
- If you prefer not to always attempt a hard kill after a successful interrupt, you can add a short platform-appropriate check (e.g. a build-tagged helper) to determine whether the interrupt likely stopped the process and skip the
Killin that case.
|
|
||
| // 使用fzf进行模糊搜索 | ||
| fzfCmd := exec.CommandContext(ctx, "fzf", "-f", query, "--print-query", "--no-sort", "--tac") | ||
| fzfCmd := exec.CommandContext(ctx, fzfPath(), "-f", query, "--print-query", "--no-sort", "--tac") |
There was a problem hiding this comment.
security (go.lang.security.audit.dangerous-exec-command): Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
Source: opengrep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Embed and extract distributed binaries at runtime and use them for internal tools and the codebase server, while ensuring spawned processes are cleaned up on exit.
New Features:
Enhancements: