Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit a36cf67. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
some cases return data and errors that can be ignored
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Type assertions on results panic on errors and recovery
- Replaced all
results[0]type assertions in filesystem/file wrappers with closure-captured typed return variables so error and panic-recovery paths no longer panic on nil results or nil interfaces.
- Replaced all
Or push these changes by commenting:
@cursor push 592b19da69
Preview (592b19da69)
diff --git a/packages/orchestrator/pkg/nfsproxy/middleware/file.go b/packages/orchestrator/pkg/nfsproxy/middleware/file.go
--- a/packages/orchestrator/pkg/nfsproxy/middleware/file.go
+++ b/packages/orchestrator/pkg/nfsproxy/middleware/file.go
@@ -28,47 +28,55 @@
}
func (w *wrappedFile) Write(p []byte) (int, error) {
- results, err := w.chain.Exec(w.ctx, "File.Write", []any{p},
+ var n int
+ _, err := w.chain.Exec(w.ctx, "File.Write", []any{p},
func(_ context.Context) ([]any, error) {
- n, err := w.inner.Write(p)
+ var err error
+ n, err = w.inner.Write(p)
return []any{n}, err
})
- return results[0].(int), err
+ return n, err
}
func (w *wrappedFile) Read(p []byte) (int, error) {
- results, err := w.chain.Exec(w.ctx, "File.Read", []any{p},
+ var n int
+ _, err := w.chain.Exec(w.ctx, "File.Read", []any{p},
func(_ context.Context) ([]any, error) {
- n, err := w.inner.Read(p)
+ var err error
+ n, err = w.inner.Read(p)
return []any{n}, err
})
- return results[0].(int), err
+ return n, err
}
func (w *wrappedFile) ReadAt(p []byte, off int64) (int, error) {
- results, err := w.chain.Exec(w.ctx, "File.ReadAt", []any{p, off},
+ var n int
+ _, err := w.chain.Exec(w.ctx, "File.ReadAt", []any{p, off},
func(_ context.Context) ([]any, error) {
- n, err := w.inner.ReadAt(p, off)
+ var err error
+ n, err = w.inner.ReadAt(p, off)
return []any{n}, err
})
- return results[0].(int), err
+ return n, err
}
func (w *wrappedFile) Seek(offset int64, whence int) (int64, error) {
- results, err := w.chain.Exec(w.ctx, "File.Seek", []any{offset, whence},
+ var n int64
+ _, err := w.chain.Exec(w.ctx, "File.Seek", []any{offset, whence},
func(_ context.Context) ([]any, error) {
- n, err := w.inner.Seek(offset, whence)
+ var err error
+ n, err = w.inner.Seek(offset, whence)
return []any{n}, err
})
- return results[0].(int64), err
+ return n, err
}
func (w *wrappedFile) Close() error {
diff --git a/packages/orchestrator/pkg/nfsproxy/middleware/fs.go b/packages/orchestrator/pkg/nfsproxy/middleware/fs.go
--- a/packages/orchestrator/pkg/nfsproxy/middleware/fs.go
+++ b/packages/orchestrator/pkg/nfsproxy/middleware/fs.go
@@ -25,47 +25,55 @@
}
func (w *wrappedFS) Create(filename string) (billy.File, error) {
- results, err := w.chain.Exec(w.ctx, "FS.Create", []any{filename},
+ var file billy.File
+ _, err := w.chain.Exec(w.ctx, "FS.Create", []any{filename},
func(_ context.Context) ([]any, error) {
- f, err := w.inner.Create(filename)
+ var err error
+ file, err = w.inner.Create(filename)
- return []any{f}, err
+ return []any{file}, err
})
- return WrapFile(w.ctx, results[0].(billy.File), w.chain), err
+ return WrapFile(w.ctx, file, w.chain), err
}
func (w *wrappedFS) Open(filename string) (billy.File, error) {
- results, err := w.chain.Exec(w.ctx, "FS.Open", []any{filename},
+ var file billy.File
+ _, err := w.chain.Exec(w.ctx, "FS.Open", []any{filename},
func(_ context.Context) ([]any, error) {
- f, err := w.inner.Open(filename)
+ var err error
+ file, err = w.inner.Open(filename)
- return []any{f}, err
+ return []any{file}, err
})
- return WrapFile(w.ctx, results[0].(billy.File), w.chain), err
+ return WrapFile(w.ctx, file, w.chain), err
}
func (w *wrappedFS) OpenFile(filename string, flag int, perm os.FileMode) (billy.File, error) {
- results, err := w.chain.Exec(w.ctx, "FS.OpenFile", []any{filename, flag, perm},
+ var file billy.File
+ _, err := w.chain.Exec(w.ctx, "FS.OpenFile", []any{filename, flag, perm},
func(_ context.Context) ([]any, error) {
- f, err := w.inner.OpenFile(filename, flag, perm)
+ var err error
+ file, err = w.inner.OpenFile(filename, flag, perm)
- return []any{f}, err
+ return []any{file}, err
})
- return WrapFile(w.ctx, results[0].(billy.File), w.chain), err
+ return WrapFile(w.ctx, file, w.chain), err
}
func (w *wrappedFS) Stat(filename string) (os.FileInfo, error) {
- results, err := w.chain.Exec(w.ctx, "FS.Stat", []any{filename},
+ var info os.FileInfo
+ _, err := w.chain.Exec(w.ctx, "FS.Stat", []any{filename},
func(_ context.Context) ([]any, error) {
- info, err := w.inner.Stat(filename)
+ var err error
+ info, err = w.inner.Stat(filename)
return []any{info}, err
})
- return results[0].(os.FileInfo), err
+ return info, err
}
func (w *wrappedFS) Rename(oldpath, newpath string) error {
@@ -91,25 +99,29 @@
}
func (w *wrappedFS) TempFile(dir, prefix string) (billy.File, error) {
- results, err := w.chain.Exec(w.ctx, "FS.TempFile", []any{dir, prefix},
+ var file billy.File
+ _, err := w.chain.Exec(w.ctx, "FS.TempFile", []any{dir, prefix},
func(_ context.Context) ([]any, error) {
- f, err := w.inner.TempFile(dir, prefix)
+ var err error
+ file, err = w.inner.TempFile(dir, prefix)
- return []any{f}, err
+ return []any{file}, err
})
- return WrapFile(w.ctx, results[0].(billy.File), w.chain), err
+ return WrapFile(w.ctx, file, w.chain), err
}
func (w *wrappedFS) ReadDir(path string) ([]os.FileInfo, error) {
- results, err := w.chain.Exec(w.ctx, "FS.ReadDir", []any{path},
+ var infos []os.FileInfo
+ _, err := w.chain.Exec(w.ctx, "FS.ReadDir", []any{path},
func(_ context.Context) ([]any, error) {
- infos, err := w.inner.ReadDir(path)
+ var err error
+ infos, err = w.inner.ReadDir(path)
return []any{infos}, err
})
- return results[0].([]os.FileInfo), err
+ return infos, err
}
func (w *wrappedFS) MkdirAll(filename string, perm os.FileMode) error {
@@ -122,14 +134,16 @@
}
func (w *wrappedFS) Lstat(filename string) (os.FileInfo, error) {
- results, err := w.chain.Exec(w.ctx, "FS.Lstat", []any{filename},
+ var info os.FileInfo
+ _, err := w.chain.Exec(w.ctx, "FS.Lstat", []any{filename},
func(_ context.Context) ([]any, error) {
- info, err := w.inner.Lstat(filename)
+ var err error
+ info, err = w.inner.Lstat(filename)
return []any{info}, err
})
- return results[0].(os.FileInfo), err
+ return info, err
}
func (w *wrappedFS) Symlink(target, link string) error {
@@ -142,25 +156,29 @@
}
func (w *wrappedFS) Readlink(link string) (string, error) {
- results, err := w.chain.Exec(w.ctx, "FS.Readlink", []any{link},
+ var target string
+ _, err := w.chain.Exec(w.ctx, "FS.Readlink", []any{link},
func(_ context.Context) ([]any, error) {
- target, err := w.inner.Readlink(link)
+ var err error
+ target, err = w.inner.Readlink(link)
return []any{target}, err
})
- return results[0].(string), err
+ return target, err
}
func (w *wrappedFS) Chroot(path string) (billy.Filesystem, error) {
- results, err := w.chain.Exec(w.ctx, "FS.Chroot", []any{path},
+ var fs billy.Filesystem
+ _, err := w.chain.Exec(w.ctx, "FS.Chroot", []any{path},
func(_ context.Context) ([]any, error) {
- fs, err := w.inner.Chroot(path)
+ var err error
+ fs, err = w.inner.Chroot(path)
return []any{fs}, err
})
- return WrapFilesystem(w.ctx, results[0].(billy.Filesystem), w.chain), err
+ return WrapFilesystem(w.ctx, fs, w.chain), err
}
func (w *wrappedFS) Root() string {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
also return a mount error status
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 865484f. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 865484f85a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merge Score: 95/100🟢 This PR successfully refactors the NFS proxy observability and recovery wrappers into a generic middleware/interceptor pattern, drastically reducing boilerplate code. The author also caught and fixed a double-wrapping bug in Code Suggestions (2)Medium Priority (1)
Reasoning: When Suggested Code: Low Priority (1)
Reasoning: The new middleware pattern creates a 📊 Review Metadata
|
| // Exec runs the operation through all interceptors. | ||
| func (c *Chain) Exec(ctx context.Context, req Request, fn func(context.Context) error) error { | ||
| wrapped := fn | ||
| for i := len(c.interceptors) - 1; i >= 0; i-- { |
There was a problem hiding this comment.
Chain.Exec builds N closures on every NFS operation (every Read, Stat, Lstat), adding GC pressure proportional to len(interceptors) * NFS ops/sec. Consider composing the function chain once and caching it, so Exec is a single function call with zero allocations.
| interceptor := c.interceptors[i] | ||
| inner := wrapped | ||
| wrapped = func(ctx context.Context) error { | ||
| return interceptor(ctx, req, inner) |
There was a problem hiding this comment.
This means that if the "core" operation succeeds, but an interceptor layer fails we would return an error to the caller; 2/5 we should ensure that non-critical interceptors log their errors, and only "real" failures are passed up.
| return err | ||
| }) | ||
|
|
||
| return WrapFile(w.ctx, f, w.chain), err |
There was a problem hiding this comment.
Nit: Is this intentional that we return a wrapped object on exec error? If so, maybe a comment?
|
|
||
| // extractArgFields extracts the relevant fields from a typed request. | ||
| // This is the single source of truth for which args to extract for each operation. | ||
| func extractArgFields(req middleware.Request) []argField { |
There was a problem hiding this comment.
2/5 This is a lot of avoidable allocations in the NFS proxy hot path. The following prompt to Dr. Claude seemed to produce a reasonable result,
Please check chunker.go for an example of how we prebuild OTEL attributes to avoid more and more allocations. Suggest a similarly optimal allocation avoidance strategy for OTEL in this PR
3 participants
This should simplify adding and maintaining these; the amount of boilerplate was intense.