[codex] add Go read-only core tools - #47
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds an internal tools subsystem: core Tool types and arg helpers, workspace path safety, four read-only file tools (read_file, list_directory, glob, grep), a registry enforcing PermissionAllow, and unit tests validating functionality and metadata. ChangesTools Package
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/tools/file_tools_test.go`:
- Around line 161-167: The test currently only asserts the output string from
NewGrepTool(...).Run(...); also assert the returned result's status equals
StatusOK so execution errors are caught (e.g., add an assertion that the result
from NewGrepTool(root).Run(...) has result.Status == StatusOK). Reference the
Run call and the result variable (count) and use the StatusOK constant to
perform the status check before asserting the output string.
- Around line 95-100: The test currently uses strings.Count(result.Output,
".go") which is brittle; instead parse result.Output into lines and count only
the lines that represent matched file paths (e.g., use a regexp on result.Output
like regexp.MustCompile(`(?m)^[^\n]*\.go\b(:\d+)?`) to find path/line matches)
and assert that the number of those matches equals 1; update the assertion that
references result.Output to use this filtered count and keep the existing check
on result.Truncated unchanged.
In `@internal/tools/glob.go`:
- Around line 94-97: The WalkDir callback currently swallows filesystem errors
by returning nil when walkErr != nil; update the callback used in the
filepath.WalkDir call (inside the glob traversal function) to return the
incoming walkErr (or wrap it with context) instead of nil so WalkDir will
propagate the error to the caller; ensure the surrounding function (the glob
traversal function) checks and returns any error from filepath.WalkDir to the
caller so filesystem traversal failures are not hidden.
In `@internal/tools/grep.go`:
- Around line 169-172: The WalkDir callback passed to filepath.WalkDir currently
ignores the incoming walkErr, suppressing filesystem errors; in the anonymous
func used by filepath.WalkDir (parameters path, entry, walkErr) return the
walkErr (or wrap it with contextual info using fmt.Errorf) instead of nil so
upstream callers receive the error; alternatively, explicitly handle known
ignorable errors but log and propagate other errors from the WalkDir callback
(refer to the anonymous func around filepath.WalkDir in grep.go).
In `@internal/tools/read_file.go`:
- Around line 66-70: The current splitting logic inflates line counts for files
ending with a newline because strings.Split on "\n" yields a trailing empty
element; before computing lines and using variables like lines, total, startLine
and okResult/relativePath, trim a single trailing "\n" (after the CRLF
replacement) or otherwise remove a final empty line (e.g., via
strings.TrimSuffix or similar) so that splitting produces the true number of
lines; update the code that builds lines to trim the trailing newline before
splitting and keep the rest of the checks (startLine > total) unchanged.
In `@internal/tools/workspace.go`:
- Around line 51-66: The current check only resolves a single Readlink and can
miss symlinked parent segments; instead fully resolve symlinks before enforcing
the workspace boundary: compute absolute paths for both root and the requested
target (use filepath.Abs on target/root), then call filepath.EvalSymlinks (or
iteratively EvalSymlinks on path prefixes) on the absolute target (and root) to
get their real paths, and only then use filepath.Rel and the existing checks
(relative == ".." etc.) against the resolved paths; ensure errors from
EvalSymlinks are propagated and keep references to target, root,
filepath.EvalSymlinks, filepath.Abs, filepath.Rel and requestedPath to locate
the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e97ff2ca-0092-40df-919e-ddf7a7bb226c
📒 Files selected for processing (10)
internal/tools/args.gointernal/tools/file_tools_test.gointernal/tools/glob.gointernal/tools/grep.gointernal/tools/list_directory.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/types.gointernal/tools/workspace.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- Clean separation in
internal/tools/types.go:Toolinterface withbaseToolcovering the four boilerplate methods, JSON-schema-compatibleSchema/PropertySchema, and theSideEffect/Permissionenums.readOnlySafetyis a tidy factory. - Workspace boundary is enforced correctly:
resolveWorkspacePath(internal/tools/workspace.go:32-69) runsfilepath.EvalSymlinkson both the root and the target before the../ abs-path check, so symlinked parent segments can't smuggle a path out of the workspace. This was one of CodeRabbit's earlier findings and is now done. - All four tools share a single
ignoredDirectoriesset (.git,node_modules,dist,build,.next,.turbo,coverage,.cache,tmp,temp) viashouldSkipDirectory— used consistently byglob.scanGlob,grep.grepFiles, andlistDirectoryEntries. WalkDircallbacks now propagatewalkErr(bothinternal/tools/glob.go:96-98andinternal/tools/grep.go:171-173), so filesystem traversal failures reach the caller instead of being silently dropped.read_file.go:64-69trims a single trailing\nbefore splitting, so a file ending in\nreports the correct line count. Also normalises CRLF → LF and treatsstart_line > totalas a soft "past the end" result rather than an error.Registry.Run(internal/tools/registry.go:30-41) gates ontool.Safety().Permission == PermissionAllowbefore delegating — gives a single safe-run choke point for future tools.- Test coverage: metadata + schema invariants, workspace rejection, line ranges, recursive listing with junk-dir skipping, glob
**+ limit, glob include-dirs, grep content /files_with_matches/countmodes, and the glob test now usesregexp.MustCompileto count.gomatches instead ofstrings.Count(".go")(also a prior CodeRabbit concern, now addressed). - Follow-up commit
0c63a34 fix: harden go read-only toolsaddresses each of CodeRabbit's actionable items; CI is green on all four matrices (ubuntu, macos, windows, Performance Smoke) and CodeRabbit reports SUCCESS on the latest SHA.
Observations (non-blocking)
-
read_fileon a missing path leaks the absolute path via EvalSymlinks.resolveWorkspacePathrunsfilepath.EvalSymlinks(target)beforeos.ReadFile, so a missing file surfaces asError reading file <requested>: lstat /workspace/<requested>: no such file or directory(internal/tools/read_file.go:54-58). The message is technically informative but the EvalSymlinks failure mode is opaque to a non-engineer caller — catchingerrors.Is(err, fs.ErrNotExist)(or aENOENTcheck) and returning a dedicated "file not found" result would tidy this up. -
listDirectoryEntriesswallows subdirectory errors silently. When the recursiveos.ReadDiron a child fails, the child is dropped from the output with no diagnostic (internal/tools/list_directory.go:88-92). Graceful degradation is fine for a read tool, butlist_directory --recursivewill silently show a directory as empty when a permissions error blocks its children. A surface as a trailing! permission denied: <dir>line, or at least aMetafield, would help. -
head_limithas no upper bound in grep.intArg(args, "head_limit", 50, 1, 0)passesmax=0(meaning "no max" in the helper's convention), so a caller can ask for a million-line result. Glob caps at 1000; grep is unbounded. Picking a sensible cap (1000?) and clamping would mirror the glob behaviour. -
shouldSkipDirectoryis not unit-tested directly. It's exercised transitively through every other tool's tests, but aTestIgnoredDirectories_AreSkippedasserting the exact set and behaviour would document the policy and catch accidental additions. -
The
"-i"schema key is unusual.internal/tools/grep.go:37registers the case-insensitive flag under the property name"-i". Standard for CLI parity, but the tool interface's JSON-schema consumer (TUI / LLM-side) might prefer a separatecase_insensitive(already present alongside it) and treat-ias an alias. Worth a doc comment in theSchemadescription.
No blockers. Ship it.
2 participants
Summary
internal/toolspackage for Zero.read_file,list_directory,glob, andgrep.Why
This starts Vasanth's Go M0 tools/TUI path without jumping into Bubble Tea or mutation tools too early. The TUI and agent loop can now consume a clean Go tool contract and safe read/search tools.
Validation
Local:
git diff --cached --checkpassed before commit.git diff --checkpassed.bun run typecheckpassed.bun test ./tests --timeout 15000passed: 277 tests.bun run buildpassed.bun run smoke:buildpassed.Blocked locally:
go test ./...andgo build ./cmd/zerocould not run becausegois not installed on this Windows machine.CI:
go test ./...andgo build ./cmd/zeroon Ubuntu, macOS, and Windows.Summary by CodeRabbit
New Features
Tests
Chores