feat: add bundled Codex provider parity#8
Open
bnema wants to merge 5 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a bundled Codex review provider plugin with a new JSON-RPC 2.0 app-server client over WebSocket/Unix socket, publish message formatting, and a ChangesBundled Codex Plugin
Sequence Diagram(s)sequenceDiagram
participant codexProvider
participant SendCallback
participant DialLiveSession
participant AppServerClient
participant CodexAppServer
codexProvider->>SendCallback: formattedMessage, cfg(SocketPath, ThreadID)
SendCallback->>SendCallback: ValidateCallbackTarget()
SendCallback->>DialLiveSession: cfg
DialLiveSession->>CodexAppServer: dial Unix socket + WebSocket upgrade
DialLiveSession-->>SendCallback: AppServerClient
SendCallback->>AppServerClient: Initialize(ctx)
AppServerClient->>CodexAppServer: initialize request
CodexAppServer-->>AppServerClient: initialize response
AppServerClient->>CodexAppServer: initialized notification
SendCallback->>AppServerClient: PublishMessage(ctx, ThreadID, formattedMessage)
AppServerClient->>CodexAppServer: turn/start {threadID, message}
CodexAppServer-->>AppServerClient: turn.id
AppServerClient-->>SendCallback: turnID
SendCallback-->>codexProvider: PublishResult{ThreadID, TurnID}
SendCallback->>AppServerClient: Close()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@internal/adapters/out/codex/client_internal_test.go`:
- Around line 206-299: The test TestInitializeAppendsNewline claims to verify
that the initialized notification includes a trailing newline for JSONL
transport, but it never actually asserts the presence of that newline in the
message. After calling client.Initialize() and verifying the handshake is ready,
add an explicit assertion that captures and checks the actual initialized
notification message sent to the server to confirm it ends with a newline
character. This will ensure that the trailing newline required for JSONL framing
is actually present in the initialized notification.
In `@internal/adapters/out/codex/client_test.go`:
- Around line 152-167: The TestConfigFromEnvDefaults test does not isolate the
environment and will fail nondeterministically if ERO_CODEX_* environment
variables are already set. Before calling ConfigFromEnv(), save the current
values of the relevant environment variables (ExecPath, ThreadID, SessionKey,
CodexHome), unset them, and then use a defer statement to restore them after the
test completes. This ensures the test always runs against a clean environment
state regardless of what values may be pre-set in the test runner's environment.
In `@internal/adapters/out/codex/client.go`:
- Around line 362-377: The ListStoredThreads function parses createdAt and
updatedAt timestamp fields from the wire response but fails to include them in
the ThreadCandidate struct being constructed within the loop. Add the missing
CreatedAt and UpdatedAt fields to the ThreadCandidate struct initialization and
map the corresponding parsed timestamp values from the raw wire response data
into these fields to preserve the recency metadata needed for downstream
selection and tie-break behavior.
In `@internal/adapters/out/codex/config.go`:
- Around line 163-170: The ResolveExecPath method currently uses os.Stat to
verify the ExecPath exists, but this does not validate that the path is actually
executable, allowing non-executable files or directories to pass validation and
cause failures later during subprocess startup. Modify the validation logic
after calling os.Stat to also check that the file has executable permissions
set. Examine the FileInfo mode returned by os.Stat using its Mode method and
verify the executable permission bits are set before returning the path.
In `@internal/adapters/out/codex/jsonrpc.go`:
- Around line 423-426: The MsgResponse case in the ValidateMessage function at
lines 423-426 only checks for missing or null IDs, but does not validate the
shape/type of the ID itself. Add validation to ensure response IDs conform to
the expected type (string or number) in addition to the existing missing/null
checks, so that ValidateMessage rejects invalid ID shapes like objects, arrays,
or booleans before they reach VerifyResponse, ensuring consistent protocol
validation across both functions.
In `@internal/adapters/out/codex/thread.go`:
- Around line 276-281: In internal/adapters/out/codex/thread.go at lines 279-281
(anchor) and 305-307 (sibling), the code incorrectly returns
ThreadDecisionInvalidOverride when collectStoredThreads fails due to I/O errors.
Both locations should instead return ThreadDecisionIOError to properly
distinguish between I/O failures and invalid overrides. Change the Decision
field from ThreadDecisionInvalidOverride to ThreadDecisionIOError in both the
error handling block that checks the result of collectStoredThreads at the
anchor location and the sibling location, ensuring I/O failures are correctly
classified rather than being misrepresented as invalid overrides.
In `@internal/adapters/out/codex/transport_test.go`:
- Around line 14-33: The TestProbeSocketExists test creates a regular file but
never asserts that ProbeSocket returns false for it, and it completely lacks a
positive test case for actual socket files. Replace the if-only-log pattern with
proper assertions: first assert that ProbeSocket returns false for the regular
file case, then create an actual Unix domain socket file (by creating a listener
with net.Listen("unix", sockPath)) and assert that ProbeSocket returns true for
it. This ensures the test verifies both the negative case (regular files) and
the positive case (real sockets) so that socket detection regressions will cause
the test to fail.
In `@internal/adapters/out/plugin/bundled.go`:
- Around line 35-36: Change the deduplication logic to use the plugin's logical
source identifier instead of the filesystem path as the key in the seen map. In
internal/adapters/out/plugin/bundled.go at lines 35-36 where the seen map is
initialized and used, replace the path-based key with the plugin Source
identifier (for example, bundled:plugin-name). Apply the same change at lines
50-56 and lines 71-77 where deduplication checks occur, ensuring all three
locations consistently use the Source value as the key to properly deduplicate
plugins by their logical source rather than their filesystem location.
In `@internal/adapters/out/plugin/provider_loader.go`:
- Around line 182-187: The pluginCanBuildLocally function at lines 182-187
incorrectly allows absolute host executables to pass validation because the
check on line 183 only verifies that buildCommandName contains a forward slash,
not that it is a relative path. This causes absolute paths like /usr/bin/make to
be accepted and found by runtimeCommandPath on the host system, incorrectly
classifying binary-only or release plugins as locally buildable. Modify the
condition to reject absolute paths (those starting with /) and only accept
relative paths, ensuring that runtimeCommandPath will only find executables
within the plugin directory rather than from the host system.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 520c10ce-e3b3-4044-876c-ce30fd411983
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
.gitignore.goreleaser.yamlREADME.mddocs/plugins.mdgo.modinternal/adapters/in/cli/plugin.gointernal/adapters/in/cli/plugin_test.gointernal/adapters/in/cli/render/plugin.gointernal/adapters/out/codex/client.gointernal/adapters/out/codex/client_internal_test.gointernal/adapters/out/codex/client_test.gointernal/adapters/out/codex/config.gointernal/adapters/out/codex/jsonrpc.gointernal/adapters/out/codex/jsonrpc_test.gointernal/adapters/out/codex/publish.gointernal/adapters/out/codex/publish_test.gointernal/adapters/out/codex/publish_workflow.gointernal/adapters/out/codex/setpgid_linux.gointernal/adapters/out/codex/setpgid_other.gointernal/adapters/out/codex/thread.gointernal/adapters/out/codex/thread_test.gointernal/adapters/out/codex/transport.gointernal/adapters/out/codex/transport_live.gointernal/adapters/out/codex/transport_test.gointernal/adapters/out/codex/websocket_test.gointernal/adapters/out/plugin/bundled.gointernal/adapters/out/plugin/bundled_test.gointernal/adapters/out/plugin/manager.gointernal/adapters/out/plugin/manager_test.gointernal/adapters/out/plugin/provider_loader.gointernal/adapters/out/plugin/provider_loader_test.gointernal/app/active_provider_service_test.gointernal/app/app.gointernal/ports/mocks/review_provider_loader_mock.gointernal/ports/plugin.goplugins/codex/cmd/ero-plugin-codex/main.goplugins/codex/cmd/ero-plugin-codex/main_test.goplugins/codex/ero-plugin.toml
💤 Files with no reviewable changes (1)
- internal/ports/mocks/review_provider_loader_mock.go
Comment on lines
+206
to
+299
| // TestInitializeAppendsNewline verifies that the initialized notification | ||
| // includes a trailing newline (required for JSONL transport). | ||
| // It also verifies that Initialize + ReadThread work correctly with the | ||
| // proper response shapes. | ||
| func TestInitializeAppendsNewline(t *testing.T) { | ||
| serverStdinR, clientStdoutW := io.Pipe() | ||
| clientStdinR, serverStdoutW := io.Pipe() | ||
|
|
||
| serverErr := make(chan error, 1) | ||
|
|
||
| go pipeFakeServer(t, serverStdinR, serverStdoutW, serverErr) | ||
| time.Sleep(10 * time.Millisecond) | ||
|
|
||
| // Construct AppServerClient directly using the pipes. | ||
| scanner := bufio.NewScanner(clientStdinR) | ||
| scanner.Buffer(make([]byte, 2*1024*1024), 2*1024*1024) | ||
|
|
||
| client := &AppServerClient{ | ||
| cfg: Config{CommandTimeout: 5 * time.Second}, | ||
| stdin: clientStdoutW, | ||
| scanner: scanner, | ||
| timeout: 5 * time.Second, | ||
| maxMsgSize: 2 * 1024 * 1024, | ||
| } | ||
|
|
||
| // Initialize handshake. | ||
| if err := client.Initialize(context.Background()); err != nil { | ||
| t.Fatalf("Initialize: %v", err) | ||
| } | ||
| if !client.hs.IsReady() { | ||
| t.Fatal("Initialize should leave handshake in Ready state") | ||
| } | ||
|
|
||
| // readThread should parse the correct wrapped response shape. | ||
| candidate, err := client.readThread(context.Background(), "thr_loaded_1") | ||
| if err != nil { | ||
| t.Fatalf("ReadThread: %v", err) | ||
| } | ||
| if candidate.ID != "thr_loaded_1" { | ||
| t.Errorf("ReadThread ID = %q, want %q", candidate.ID, "thr_loaded_1") | ||
| } | ||
| if candidate.CWD != "/home/user/project" { | ||
| t.Errorf("ReadThread CWD = %q, want %q", candidate.CWD, "/home/user/project") | ||
| } | ||
| if candidate.SessionKey != "sess_loaded_1" { | ||
| t.Errorf("ReadThread SessionKey = %q, want %q", candidate.SessionKey, "sess_loaded_1") | ||
| } | ||
| if candidate.Preview != "Loaded review thread" { | ||
| t.Errorf("ReadThread Preview = %q, want %q", candidate.Preview, "Loaded review thread") | ||
| } | ||
| if candidate.Status != ThreadStatusIdle { | ||
| t.Errorf("ReadThread Status = %q, want %q", candidate.Status, ThreadStatusIdle) | ||
| } | ||
|
|
||
| // ListLoadedThreads should enrich loaded threads with CWD via ReadThread. | ||
| loaded, err := client.ListLoadedThreads(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("ListLoadedThreads: %v", err) | ||
| } | ||
| if len(loaded) != 1 { | ||
| t.Fatalf("expected 1 loaded thread, got %d", len(loaded)) | ||
| } | ||
| if loaded[0].CWD != "/home/user/project" { | ||
| t.Errorf("enriched loaded thread CWD = %q, want %q", loaded[0].CWD, "/home/user/project") | ||
| } | ||
| if loaded[0].SessionKey != "sess_loaded_1" { | ||
| t.Errorf("enriched loaded thread SessionKey = %q, want %q", loaded[0].SessionKey, "sess_loaded_1") | ||
| } | ||
|
|
||
| // Resume + PublishMessage returns the turn ID. | ||
| if err := client.ResumeThread(context.Background(), "thr_loaded_1"); err != nil { | ||
| t.Fatalf("ResumeThread: %v", err) | ||
| } | ||
|
|
||
| turnID, err := client.PublishMessage(context.Background(), "thr_loaded_1", "Test review") | ||
| if err != nil { | ||
| t.Fatalf("PublishMessage: %v", err) | ||
| } | ||
| if turnID != "turn_1" { | ||
| t.Errorf("PublishMessage turn ID = %q, want %q", turnID, "turn_1") | ||
| } | ||
|
|
||
| // Clean up. | ||
| _ = clientStdinR.Close() | ||
| _ = clientStdoutW.Close() | ||
|
|
||
| select { | ||
| case err := <-serverErr: | ||
| if err != nil { | ||
| t.Fatalf("fake server error: %v", err) | ||
| } | ||
| default: | ||
| } | ||
| } |
There was a problem hiding this comment.
TestInitializeAppendsNewline does not explicitly verify newline framing.
The test exercises handshake flow, but it never asserts that initialized was observed as its own JSONL message. A missing trailing newline on that notification could still pass here.
🤖 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 `@internal/adapters/out/codex/client_internal_test.go` around lines 206 - 299,
The test TestInitializeAppendsNewline claims to verify that the initialized
notification includes a trailing newline for JSONL transport, but it never
actually asserts the presence of that newline in the message. After calling
client.Initialize() and verifying the handshake is ready, add an explicit
assertion that captures and checks the actual initialized notification message
sent to the server to confirm it ends with a newline character. This will ensure
that the trailing newline required for JSONL framing is actually present in the
initialized notification.
Comment on lines
+152
to
+167
| func TestConfigFromEnvDefaults(t *testing.T) { | ||
| cfg := codex.ConfigFromEnv() | ||
|
|
||
| if cfg.ExecPath != "" { | ||
| t.Errorf("expected empty ExecPath, got %q", cfg.ExecPath) | ||
| } | ||
| if cfg.ThreadID != "" { | ||
| t.Errorf("expected empty ThreadID, got %q", cfg.ThreadID) | ||
| } | ||
| if cfg.SessionKey != "" { | ||
| t.Errorf("expected empty SessionKey, got %q", cfg.SessionKey) | ||
| } | ||
| if cfg.CodexHome != "" { | ||
| t.Errorf("expected empty CodexHome, got %q", cfg.CodexHome) | ||
| } | ||
| } |
There was a problem hiding this comment.
Isolate environment in TestConfigFromEnvDefaults to avoid flaky failures.
This test depends on ERO_CODEX_* being unset, but it doesn’t clear them. Pre-set env values can make it fail nondeterministically.
🔧 Suggested fix
func TestConfigFromEnvDefaults(t *testing.T) {
+ t.Setenv(codex.EnvCodexExecPath, "")
+ t.Setenv(codex.EnvCodexThreadID, "")
+ t.Setenv(codex.EnvCodexSessionKey, "")
+ t.Setenv(codex.EnvCodexHome, "")
+ t.Setenv(codex.EnvCodexTransport, "")
+ t.Setenv(codex.EnvCodexSocketPath, "")
+ t.Setenv(codex.EnvCodexTimeout, "")
+
cfg := codex.ConfigFromEnv()🤖 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 `@internal/adapters/out/codex/client_test.go` around lines 152 - 167, The
TestConfigFromEnvDefaults test does not isolate the environment and will fail
nondeterministically if ERO_CODEX_* environment variables are already set.
Before calling ConfigFromEnv(), save the current values of the relevant
environment variables (ExecPath, ThreadID, SessionKey, CodexHome), unset them,
and then use a defer statement to restore them after the test completes. This
ensures the test always runs against a clean environment state regardless of
what values may be pre-set in the test runner's environment.
Comment on lines
+423
to
+426
| case MsgResponse: | ||
| if len(msg.ID) == 0 || string(msg.ID) == "null" { | ||
| return fmt.Errorf("%w: response missing id", ErrInvalidMessage) | ||
| } |
There was a problem hiding this comment.
Validate response ID shape in ValidateMessage.
Line [424] only checks missing/null IDs for responses. Object/array/bool IDs currently pass ValidateMessage but fail VerifyResponse, creating inconsistent protocol validation.
🔧 Proposed fix
case MsgResponse:
if len(msg.ID) == 0 || string(msg.ID) == "null" {
return fmt.Errorf("%w: response missing id", ErrInvalidMessage)
}
+ if !isValidID(msg.ID) {
+ return fmt.Errorf("%w: response id must be a string or number", ErrInvalidMessage)
+ }
if msg.Error != nil && len(msg.Result) > 0 {
return fmt.Errorf("%w: response has both result and error", ErrInvalidMessage)
}📝 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
| case MsgResponse: | |
| if len(msg.ID) == 0 || string(msg.ID) == "null" { | |
| return fmt.Errorf("%w: response missing id", ErrInvalidMessage) | |
| } | |
| case MsgResponse: | |
| if len(msg.ID) == 0 || string(msg.ID) == "null" { | |
| return fmt.Errorf("%w: response missing id", ErrInvalidMessage) | |
| } | |
| if !isValidID(msg.ID) { | |
| return fmt.Errorf("%w: response id must be a string or number", ErrInvalidMessage) | |
| } |
🤖 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 `@internal/adapters/out/codex/jsonrpc.go` around lines 423 - 426, The
MsgResponse case in the ValidateMessage function at lines 423-426 only checks
for missing or null IDs, but does not validate the shape/type of the ID itself.
Add validation to ensure response IDs conform to the expected type (string or
number) in addition to the existing missing/null checks, so that ValidateMessage
rejects invalid ID shapes like objects, arrays, or booleans before they reach
VerifyResponse, ensuring consistent protocol validation across both functions.
Comment on lines
+276
to
+281
| stored, err := collectStoredThreads(ctx, lister) | ||
| if err != nil { | ||
| return ThreadSelectionResult{ | ||
| Decision: ThreadDecisionInvalidOverride, | ||
| Reason: fmt.Sprintf("listing stored threads failed while searching for explicit thread %q: %v", threadID, err), | ||
| } |
There was a problem hiding this comment.
Don’t classify explicit-override lister failures as InvalidOverride.
Lines [279] and [305] convert stored-thread I/O failures into ThreadDecisionInvalidOverride, even though the override may be valid. This conflicts with the ThreadDecisionIOError contract and can surface misleading user errors.
🔧 Proposed fix
stored, err := collectStoredThreads(ctx, lister)
if err != nil {
return ThreadSelectionResult{
- Decision: ThreadDecisionInvalidOverride,
- Reason: fmt.Sprintf("listing stored threads failed while searching for explicit thread %q: %v", threadID, err),
+ Decision: ThreadDecisionIOError,
+ Reason: fmt.Sprintf("listing stored threads failed while resolving explicit thread %q: %v", threadID, err),
}
}
@@
stored, err := collectStoredThreads(ctx, lister)
if err != nil {
return ThreadSelectionResult{
- Decision: ThreadDecisionInvalidOverride,
- Reason: fmt.Sprintf("listing stored threads failed while searching for session key %q: %v", sessionKey, err),
+ Decision: ThreadDecisionIOError,
+ Reason: fmt.Sprintf("listing stored threads failed while resolving session key %q: %v", sessionKey, err),
}
}Also applies to: 302-307
🤖 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 `@internal/adapters/out/codex/thread.go` around lines 276 - 281, In
internal/adapters/out/codex/thread.go at lines 279-281 (anchor) and 305-307
(sibling), the code incorrectly returns ThreadDecisionInvalidOverride when
collectStoredThreads fails due to I/O errors. Both locations should instead
return ThreadDecisionIOError to properly distinguish between I/O failures and
invalid overrides. Change the Decision field from ThreadDecisionInvalidOverride
to ThreadDecisionIOError in both the error handling block that checks the result
of collectStoredThreads at the anchor location and the sibling location,
ensuring I/O failures are correctly classified rather than being misrepresented
as invalid overrides.
Comment on lines
+14
to
+33
| func TestProbeSocketExists(t *testing.T) { | ||
| // Create a temp socket file and verify ProbeSocket finds it. | ||
| dir := t.TempDir() | ||
| sockPath := filepath.Join(dir, "test.sock") | ||
|
|
||
| // Create the socket file (can't actually listen without a real server, | ||
| // but ProbeSocket only stats and checks the socket mode bit). | ||
| f, err := os.Create(sockPath) | ||
| if err != nil { | ||
| t.Fatalf("create temp socket: %v", err) | ||
| } | ||
| if err := f.Close(); err != nil { | ||
| t.Fatalf("close temp socket: %v", err) | ||
| } | ||
|
|
||
| // ProbeSocket checks the mode bit which requires ModeSocket. A regular | ||
| // file won't have that bit, so we expect false. | ||
| if got := ProbeSocket(sockPath); got { | ||
| t.Log("ProbeSocket returns true for a regular file (expected on some platforms)") | ||
| } |
There was a problem hiding this comment.
TestProbeSocketExists never asserts the positive socket case.
The test currently creates a regular file and only logs if ProbeSocket returns true, so it won’t fail when real socket detection regresses.
🔧 Suggested fix
func TestProbeSocketExists(t *testing.T) {
- // Create a temp socket file and verify ProbeSocket finds it.
+ // Create a real unix socket and verify ProbeSocket detects it.
dir := t.TempDir()
sockPath := filepath.Join(dir, "test.sock")
- // Create the socket file (can't actually listen without a real server,
- // but ProbeSocket only stats and checks the socket mode bit).
- f, err := os.Create(sockPath)
+ ln, err := net.Listen("unix", sockPath)
if err != nil {
- t.Fatalf("create temp socket: %v", err)
+ t.Fatalf("listen unix: %v", err)
}
- if err := f.Close(); err != nil {
- t.Fatalf("close temp socket: %v", err)
- }
+ t.Cleanup(func() { _ = ln.Close() })
- // ProbeSocket checks the mode bit which requires ModeSocket. A regular
- // file won't have that bit, so we expect false.
- if got := ProbeSocket(sockPath); got {
- t.Log("ProbeSocket returns true for a regular file (expected on some platforms)")
+ if got := ProbeSocket(sockPath); !got {
+ t.Fatal("ProbeSocket should return true for existing unix socket")
}🤖 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 `@internal/adapters/out/codex/transport_test.go` around lines 14 - 33, The
TestProbeSocketExists test creates a regular file but never asserts that
ProbeSocket returns false for it, and it completely lacks a positive test case
for actual socket files. Replace the if-only-log pattern with proper assertions:
first assert that ProbeSocket returns false for the regular file case, then
create an actual Unix domain socket file (by creating a listener with
net.Listen("unix", sockPath)) and assert that ProbeSocket returns true for it.
This ensures the test verifies both the negative case (regular files) and the
positive case (real sockets) so that socket detection regressions will cause the
test to fail.
Comment on lines
+35
to
+36
| seen := map[string]struct{}{} | ||
| for _, dir := range searchDirs { |
There was a problem hiding this comment.
Deduplicate bundled plugins by logical source, not only filesystem path.
Line 55 and Line 76 only guard duplicates by path key, so the same bundled plugin discovered in multiple roots can be returned multiple times with the same Source (for example bundled:ero-plugin-codex). That creates duplicate provider descriptors and duplicate lifecycle/update entries downstream.
Suggested fix
func discoverBundledPluginRoots(searchDirs []string) []bundledPluginRoot {
roots := make([]bundledPluginRoot, 0)
- seen := map[string]struct{}{}
+ seenPaths := map[string]struct{}{}
+ seenSources := map[string]struct{}{}
for _, dir := range searchDirs {
entries, err := os.ReadDir(dir)
if err != nil {
continue
}
@@
manifest, err := LoadManifest(pluginDir)
if err != nil {
continue
}
- pathKey := cleanPathKey(pluginDir)
- if _, ok := seen[pathKey]; ok {
+ pathKey := cleanPathKey(pluginDir)
+ source := strings.ToLower(strings.TrimSpace(bundledSource(manifest.Name)))
+ if _, ok := seenPaths[pathKey]; ok {
continue
}
- seen[pathKey] = struct{}{}
- roots = append(roots, bundledPluginRoot{Source: bundledSource(manifest.Name), Path: pluginDir})
+ if _, ok := seenSources[source]; ok {
+ continue
+ }
+ seenPaths[pathKey] = struct{}{}
+ seenSources[source] = struct{}{}
+ roots = append(roots, bundledPluginRoot{Source: bundledSource(manifest.Name), Path: pluginDir})
}
}
sort.Slice(roots, func(i, j int) bool { return roots[i].Source < roots[j].Source })
return roots
}Also applies to: 50-56, 71-77
🤖 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 `@internal/adapters/out/plugin/bundled.go` around lines 35 - 36, Change the
deduplication logic to use the plugin's logical source identifier instead of the
filesystem path as the key in the seen map. In
internal/adapters/out/plugin/bundled.go at lines 35-36 where the seen map is
initialized and used, replace the path-based key with the plugin Source
identifier (for example, bundled:plugin-name). Apply the same change at lines
50-56 and lines 71-77 where deduplication checks occur, ensuring all three
locations consistently use the Source value as the key to properly deduplicate
plugins by their logical source rather than their filesystem location.
Comment on lines
+182
to
+187
| buildCommandName, _ := splitRuntimeCommand(buildCommand) | ||
| if buildCommandName == "" || !strings.Contains(buildCommandName, "/") { | ||
| return false | ||
| } | ||
| info, err := os.Stat(runtimeCommandPath(buildCommandName, pluginDir)) | ||
| return err == nil && !info.IsDir() |
There was a problem hiding this comment.
pluginCanBuildLocally incorrectly allows absolute host executables.
Line 183 only checks for /, so commands like /usr/bin/make pass, and Line 187 returns true if present. That can incorrectly classify binary-only/release plugins as locally buildable and trigger unwanted build attempts.
Suggested fix
func pluginCanBuildLocally(pluginDir, buildCommand string) bool {
if pluginHasGoSource(pluginDir) {
return true
}
buildCommandName, _ := splitRuntimeCommand(buildCommand)
- if buildCommandName == "" || !strings.Contains(buildCommandName, "/") {
+ if buildCommandName == "" || filepath.IsAbs(buildCommandName) || !strings.Contains(buildCommandName, "/") {
return false
}
- info, err := os.Stat(runtimeCommandPath(buildCommandName, pluginDir))
+ buildPath := runtimeCommandPath(buildCommandName, pluginDir)
+ rel, err := filepath.Rel(filepath.Clean(pluginDir), buildPath)
+ if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+ return false
+ }
+ info, err := os.Stat(buildPath)
return err == nil && !info.IsDir()
}📝 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
| buildCommandName, _ := splitRuntimeCommand(buildCommand) | |
| if buildCommandName == "" || !strings.Contains(buildCommandName, "/") { | |
| return false | |
| } | |
| info, err := os.Stat(runtimeCommandPath(buildCommandName, pluginDir)) | |
| return err == nil && !info.IsDir() | |
| func pluginCanBuildLocally(pluginDir, buildCommand string) bool { | |
| if pluginHasGoSource(pluginDir) { | |
| return true | |
| } | |
| buildCommandName, _ := splitRuntimeCommand(buildCommand) | |
| if buildCommandName == "" || filepath.IsAbs(buildCommandName) || !strings.Contains(buildCommandName, "/") { | |
| return false | |
| } | |
| buildPath := runtimeCommandPath(buildCommandName, pluginDir) | |
| rel, err := filepath.Rel(filepath.Clean(pluginDir), buildPath) | |
| if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | |
| return false | |
| } | |
| info, err := os.Stat(buildPath) | |
| return err == nil && !info.IsDir() | |
| } |
🤖 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 `@internal/adapters/out/plugin/provider_loader.go` around lines 182 - 187, The
pluginCanBuildLocally function at lines 182-187 incorrectly allows absolute host
executables to pass validation because the check on line 183 only verifies that
buildCommandName contains a forward slash, not that it is a relative path. This
causes absolute paths like /usr/bin/make to be accepted and found by
runtimeCommandPath on the host system, incorrectly classifying binary-only or
release plugins as locally buildable. Modify the condition to reject absolute
paths (those starting with /) and only accept relative paths, ensuring that
runtimeCommandPath will only find executables within the plugin directory rather
than from the host system.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/adapters/out/codex/client.go (1)
419-442:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential nil pointer dereference in
writeLockedifc.connis nil.At line 422,
conn := c.conncaptures the connection, but ifc.connis already nil (e.g., client was closed), the subsequent type assertion and goroutine will panic or behave incorrectly.The mutex
c.muis held by the caller (writeat line 409-410, orsendRequestRawat line 196-197), butsendRequestRawchecksc.closedat line 199-201 before callingwriteLocked. However,writedoes not checkc.closedbefore callingwriteLocked.Looking at callers:
sendRequestRaw(line 211) - checksc.closedfirst ✓write(line 411) - does NOT checkc.closed✗If
writeis called after the client is closed,c.conncould be nil, leading to a nil pointer dereference when the goroutine tries to useconn.Proposed fix
func (c *AppServerClient) writeLocked(ctx context.Context, data []byte) error { + if c.closed || c.conn == nil { + return fmt.Errorf("codex: client is closed") + } ch := make(chan error, 1) conn := c.conn🤖 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 `@internal/adapters/out/codex/client.go` around lines 419 - 442, The writeLocked function can dereference a nil c.conn if the client is closed, but the write function (which calls writeLocked) does not check c.closed before calling it, while sendRequestRaw does perform this check before calling writeLocked. Add a c.closed check in the write function before calling writeLocked to ensure the connection is still valid, matching the pattern already established in sendRequestRaw.plugins/codex/cmd/ero-plugin-codex/main.go (1)
117-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against nil publish results before dereference.
result.ThreadIDis dereferenced on the success path without checkingresult != nil. If a publish implementation returns(nil, nil), this panics at runtime.Suggested fix
result, err := publish(publishCtx, cfg, codexadapter.FormatPublishMessage(msg)) if err != nil { return plugin.PublishReviewResultData{}, classifyCallbackPublishError(result, err) } + if result == nil { + return plugin.PublishReviewResultData{}, plugin.NewError(plugin.ErrorInternal, "codex callback publish returned nil result") + } refs := make([]plugin.PublishedReviewCommentRef, 0, len(payload.Draft.Comments)) if result.ThreadID != "" {🤖 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 `@plugins/codex/cmd/ero-plugin-codex/main.go` around lines 117 - 124, The code dereferences result.ThreadID without checking if result is nil. After the error check for the publish function call, add a nil guard for the result variable before accessing result.ThreadID. This nil check should protect the code block that uses result.ThreadID to ensure the function handles the case where publish returns (nil, nil) without panicking.
♻️ Duplicate comments (1)
internal/adapters/out/codex/client_test.go (1)
45-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIsolate env state in
TestConfigFromEnvDefaultsto avoid flaky assertions.Line 46 assumes all
ERO_CODEX_*vars are unset; pre-set runner env can make this fail nondeterministically.🔧 Proposed fix
func TestConfigFromEnvDefaults(t *testing.T) { + t.Setenv(codex.EnvCodexThreadID, "") + t.Setenv(codex.EnvCodexSocketPath, "") + t.Setenv(codex.EnvCodexTimeout, "") + cfg := codex.ConfigFromEnv() if cfg.ThreadID != "" {🤖 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 `@internal/adapters/out/codex/client_test.go` around lines 45 - 54, The TestConfigFromEnvDefaults test is flaky because it assumes environment variables starting with ERO_CODEX_ are unset, but the test runner environment may have them already set. Save the current state of relevant environment variables (those that ConfigFromEnv reads, such as ERO_CODEX_THREAD_ID and ERO_CODEX_SOCKET_PATH) at the start of the test, unset them to ensure clean test conditions, run the ConfigFromEnv() assertions, and then restore the original environment variable values at the end of the test (using defer or explicit cleanup) to avoid affecting other tests.
🤖 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 `@docs/plugins.md`:
- Around line 26-33: The documentation currently contradicts itself by listing
plugins/github and plugins/pi-coding-agent as shipped plugins in one section
while also listing them as installable (non-shipped) plugins in another section.
Choose one consistent classification for each of these plugins and update both
the shipped plugins section and the installable plugins section to reflect this
single, unified designation. Ensure that plugins/github and
plugins/pi-coding-agent appear in only one category with consistent language
describing their lifecycle status throughout the documentation.
In `@internal/adapters/out/codex/client.go`:
- Around line 277-300: The non-WebSocket fallback in the readMessage method
assumes that a single Read call captures the complete JSON-RPC message, which
fails for larger messages or data arriving in chunks. Modify the fallback path
(after the WebSocket check) to use buffered reading with bufio.Reader wrapping
t.conn, then use the reader's ReadLine or similar method to read complete lines
or messages, ensuring that partial reads are accumulated until the full message
is received. Alternatively, if test messages are guaranteed to be small and
atomic, document this assumption in a comment above the fallback section
explaining the limitation and when it applies.
In `@internal/adapters/out/plugin/bundled.go`:
- Around line 196-200: The bundledDescriptorForPath function compares
canonicalized paths but does not resolve symlinks on the input path parameter,
allowing symlinks pointing to shipped plugin directories to bypass the
shipped-plugin guard. Resolve symlinks on the path parameter before applying
cleanPathKey in the comparison, so that symlinked targets are properly matched
against bundled plugin descriptor paths and cannot circumvent the security
check.
In `@plugins/codex/cmd/ero-plugin-codex/main_test.go`:
- Around line 57-66: The test in the TestDetectContextMissingSocket function
uses a hardcoded `/tmp/ero-test-nonexistent.sock` path which can cause flaky
tests in shared environments due to potential collisions. Replace the hardcoded
path with an isolated temporary directory created using t.TempDir() and
construct the socket path from that directory. Store the generated path in a
variable and use it both when setting the EnvCodexSocketPath environment
variable and in the assertion that checks the result.Result.Reason contains the
expected path.
---
Outside diff comments:
In `@internal/adapters/out/codex/client.go`:
- Around line 419-442: The writeLocked function can dereference a nil c.conn if
the client is closed, but the write function (which calls writeLocked) does not
check c.closed before calling it, while sendRequestRaw does perform this check
before calling writeLocked. Add a c.closed check in the write function before
calling writeLocked to ensure the connection is still valid, matching the
pattern already established in sendRequestRaw.
In `@plugins/codex/cmd/ero-plugin-codex/main.go`:
- Around line 117-124: The code dereferences result.ThreadID without checking if
result is nil. After the error check for the publish function call, add a nil
guard for the result variable before accessing result.ThreadID. This nil check
should protect the code block that uses result.ThreadID to ensure the function
handles the case where publish returns (nil, nil) without panicking.
---
Duplicate comments:
In `@internal/adapters/out/codex/client_test.go`:
- Around line 45-54: The TestConfigFromEnvDefaults test is flaky because it
assumes environment variables starting with ERO_CODEX_ are unset, but the test
runner environment may have them already set. Save the current state of relevant
environment variables (those that ConfigFromEnv reads, such as
ERO_CODEX_THREAD_ID and ERO_CODEX_SOCKET_PATH) at the start of the test, unset
them to ensure clean test conditions, run the ConfigFromEnv() assertions, and
then restore the original environment variable values at the end of the test
(using defer or explicit cleanup) to avoid affecting other tests.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cf1b0e7-7d2c-4e67-85da-5bca9360c967
📒 Files selected for processing (21)
.goreleaser.yamldocs/plugins.mdinternal/adapters/in/cli/plugin.gointernal/adapters/in/cli/plugin_test.gointernal/adapters/in/cli/render/plugin.gointernal/adapters/out/codex/client.gointernal/adapters/out/codex/client_internal_test.gointernal/adapters/out/codex/client_test.gointernal/adapters/out/codex/config.gointernal/adapters/out/codex/publish_workflow.gointernal/adapters/out/codex/transport.gointernal/adapters/out/codex/transport_live.gointernal/adapters/out/codex/transport_test.gointernal/adapters/out/plugin/bundled.gointernal/adapters/out/plugin/bundled_test.gointernal/adapters/out/plugin/manager.gointernal/adapters/out/plugin/manager_test.goplugins/codex/cmd/ero-plugin-codex/main.goplugins/codex/cmd/ero-plugin-codex/main_test.goplugins/github/ero-plugin.tomlplugins/pi-coding-agent/ero-plugin.toml
💤 Files with no reviewable changes (1)
- internal/adapters/out/codex/transport_test.go
Comment on lines
+26
to
+33
| ### Shipped plugins | ||
|
|
||
| Ero ships maintained provider plugins under `plugins/`: | ||
|
|
||
| - `plugins/codex` | ||
| - `plugins/github` | ||
| - `plugins/pi-coding-agent` | ||
|
|
There was a problem hiding this comment.
Resolve the shipped-vs-installable contradiction for GitHub and pi-coding-agent.
This doc currently classifies plugins/github and plugins/pi-coding-agent as shipped (Line 26-Line 33) and later as not shipped (Line 196-Line 197). Please make these sections consistent so plugin lifecycle guidance is unambiguous.
Suggested doc fix (pick one model and apply consistently)
- - `plugins/github`
- - `plugins/pi-coding-agent`
+ - `plugins/github` (if shipped) / remove from this list (if installable-only)
+ - `plugins/pi-coding-agent` (if shipped) / remove from this list (if installable-only)
...
- - `plugins/github`: GitHub review provider. It is a maintained installable plugin, not a shipped provider.
- - `plugins/pi-coding-agent`: pi-coding-agent destination. It is a maintained installable plugin, not a shipped provider.
+ - `plugins/github`: GitHub review provider. [align shipped/installable wording with the section above]
+ - `plugins/pi-coding-agent`: pi-coding-agent destination. [align shipped/installable wording with the section above]Also applies to: 196-197
🤖 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 `@docs/plugins.md` around lines 26 - 33, The documentation currently
contradicts itself by listing plugins/github and plugins/pi-coding-agent as
shipped plugins in one section while also listing them as installable
(non-shipped) plugins in another section. Choose one consistent classification
for each of these plugins and update both the shipped plugins section and the
installable plugins section to reflect this single, unified designation. Ensure
that plugins/github and plugins/pi-coding-agent appear in only one category with
consistent language describing their lifecycle status throughout the
documentation.
Comment on lines
+277
to
+300
| func (t readTransport) readMessage() ([]byte, error) { | ||
| if ws, ok := t.conn.(*websocket.Conn); ok { | ||
| var msgStr string | ||
| if err := websocket.Message.Receive(ws, &msgStr); err != nil { | ||
| return nil, fmt.Errorf("codex: receive websocket message: %w", err) | ||
| } | ||
| if len(msgStr) == 0 { | ||
| return nil, io.ErrUnexpectedEOF | ||
| } | ||
| return []byte(msgStr), nil | ||
| } | ||
| // Fallback for non-websocket conn (test pipes). | ||
| buf := make([]byte, t.maxMsgSize) | ||
| n, err := t.conn.Read(buf) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("codex: read from conn: %w", err) | ||
| } | ||
| if n == 0 { | ||
| return nil, io.ErrUnexpectedEOF | ||
| } | ||
| line := make([]byte, n) | ||
| copy(line, buf[:n]) | ||
| return line, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Non-WebSocket fallback in readMessage assumes single-read message completeness.
For non-WebSocket connections (test pipes), lines 288-299 perform a single Read call and return whatever bytes were read. This assumes the entire JSON-RPC message fits in one read operation, which may not hold for large messages or if the pipe delivers data in chunks.
While this is documented as a "test pipes" fallback, if a test message exceeds a single read's worth of data, the JSON unmarshaling will fail with a partial message. This could cause flaky tests.
Consider buffered reading for test pipes
If test messages can be large, consider using a buffered reader or requiring test pipes to deliver complete messages atomically. For now, this is acceptable if tests guarantee small, atomic writes.
🤖 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 `@internal/adapters/out/codex/client.go` around lines 277 - 300, The
non-WebSocket fallback in the readMessage method assumes that a single Read call
captures the complete JSON-RPC message, which fails for larger messages or data
arriving in chunks. Modify the fallback path (after the WebSocket check) to use
buffered reading with bufio.Reader wrapping t.conn, then use the reader's
ReadLine or similar method to read complete lines or messages, ensuring that
partial reads are accumulated until the full message is received. Alternatively,
if test messages are guaranteed to be small and atomic, document this assumption
in a comment above the fallback section explaining the limitation and when it
applies.
Comment on lines
+196
to
+200
| func bundledDescriptorForPath(path string) (ports.PluginDescriptor, bool) { | ||
| pathKey := cleanPathKey(path) | ||
| for _, descriptor := range bundledPlugins() { | ||
| if descriptor.Path != "" && cleanPathKey(descriptor.Path) == pathKey { | ||
| return descriptor, true |
There was a problem hiding this comment.
Resolve symlinks when matching shipped plugin paths.
Line 197 compares canonicalized strings but not real filesystem targets. A symlink pointing to a shipped plugin directory will not match descriptor.Path, so Install can bypass the shipped-plugin guard via symlinked local paths.
Suggested fix
func cleanPathKey(path string) string {
path = filepath.Clean(path)
+ if resolved, err := filepath.EvalSymlinks(path); err == nil {
+ path = resolved
+ }
if abs, err := filepath.Abs(path); err == nil {
return abs
}
return path
}🤖 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 `@internal/adapters/out/plugin/bundled.go` around lines 196 - 200, The
bundledDescriptorForPath function compares canonicalized paths but does not
resolve symlinks on the input path parameter, allowing symlinks pointing to
shipped plugin directories to bypass the shipped-plugin guard. Resolve symlinks
on the path parameter before applying cleanPathKey in the comparison, so that
symlinked targets are properly matched against bundled plugin descriptor paths
and cannot circumvent the security check.
Comment on lines
+57
to
+66
| t.Setenv(codexadapter.EnvCodexSocketPath, "/tmp/ero-test-nonexistent.sock") | ||
| t.Setenv(codexadapter.EnvCodexThreadID, "thr_test") | ||
|
|
||
| provider := codexProvider{} | ||
| result, err := provider.DetectContext(context.Background(), plugin.DetectContextRequest{}) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.False(t, result.Result.Applicable) | ||
| assert.Contains(t, result.Result.Reason, "/tmp/ero-test-nonexistent.sock") | ||
| } |
There was a problem hiding this comment.
Use an isolated temp path to avoid flaky /tmp collisions.
This test assumes a global hardcoded /tmp path does not exist. On shared environments, that can intermittently fail. Build the missing socket path from t.TempDir() instead.
Suggested fix
func TestDetectContext_Unavailable_SocketNotReachable(t *testing.T) {
- t.Setenv(codexadapter.EnvCodexSocketPath, "/tmp/ero-test-nonexistent.sock")
+ socketPath := filepath.Join(t.TempDir(), "missing.sock")
+ t.Setenv(codexadapter.EnvCodexSocketPath, socketPath)
t.Setenv(codexadapter.EnvCodexThreadID, "thr_test")
@@
- assert.Contains(t, result.Result.Reason, "/tmp/ero-test-nonexistent.sock")
+ assert.Contains(t, result.Result.Reason, socketPath)
}📝 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
| t.Setenv(codexadapter.EnvCodexSocketPath, "/tmp/ero-test-nonexistent.sock") | |
| t.Setenv(codexadapter.EnvCodexThreadID, "thr_test") | |
| provider := codexProvider{} | |
| result, err := provider.DetectContext(context.Background(), plugin.DetectContextRequest{}) | |
| require.NoError(t, err) | |
| assert.False(t, result.Result.Applicable) | |
| assert.Contains(t, result.Result.Reason, "/tmp/ero-test-nonexistent.sock") | |
| } | |
| socketPath := filepath.Join(t.TempDir(), "missing.sock") | |
| t.Setenv(codexadapter.EnvCodexSocketPath, socketPath) | |
| t.Setenv(codexadapter.EnvCodexThreadID, "thr_test") | |
| provider := codexProvider{} | |
| result, err := provider.DetectContext(context.Background(), plugin.DetectContextRequest{}) | |
| require.NoError(t, err) | |
| assert.False(t, result.Result.Applicable) | |
| assert.Contains(t, result.Result.Reason, socketPath) | |
| } |
🤖 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 `@plugins/codex/cmd/ero-plugin-codex/main_test.go` around lines 57 - 66, The
test in the TestDetectContextMissingSocket function uses a hardcoded
`/tmp/ero-test-nonexistent.sock` path which can cause flaky tests in shared
environments due to potential collisions. Replace the hardcoded path with an
isolated temporary directory created using t.TempDir() and construct the socket
path from that directory. Store the generated path in a variable and use it both
when setting the EnvCodexSocketPath environment variable and in the assertion
that checks the result.Result.Reason contains the expected path.
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
Test Plan
go test ./...golangci-lint run ./..../cmd/eroand confirmplugin list --jsonshows bundled Codex from a source checkoutSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation