Conversation
|
@codex review |
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the full find builtin implementation across 4 new Go files, 24 YAML test scenarios, and supporting changes (registration, import allowlist, documentation).
Overall assessment: needs fixes
The architecture is sound — expression parser with bounded AST, explicit-stack DFS traversal, all filesystem access through sandbox APIs (callCtx.ReadDir, callCtx.LstatFile, callCtx.StatFile). The blocked-predicates list correctly prevents -exec, -delete, -regex, and friends. No direct os filesystem calls.
However, there is one P1 correctness bug and several P2 gaps that need attention before merge.
Positive Observations
- All filesystem access uses sandbox APIs — no direct
os.Open/os.Stat/os.ReadDircalls - Expression parser has proper AST bounds (64 depth, 256 nodes) preventing resource exhaustion
- DFS traversal uses explicit stack capped at 256 depth
- Context cancellation checked before every ReadDir and on each stack iteration
- Blocked predicates list is comprehensive and well-justified
path.Match(Go stdlib) is immune to ReDoS-newerfile access goes through sandbox — no information leak outside AllowedPaths
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary (Round 2)
Re-reviewed after fixes for round 1 findings.
Overall assessment: safe to merge (with minor P3 nits)
The P1 from round 1 (-maxdepth/-mindepth extraction corrupting predicate arguments) has been properly fixed — depth options are now only parsed from leading positions before the expression starts. The dead followLink field has been removed, -type "" validation added, baseName documented, and 6 new test scenarios added for -path, -ipath, -mmin, -mtime, -size variants, and -type "" error.
No P0 or P1 findings remain. Two minor P3 items noted below.
Positive Observations
- P1 fix is clean — leading-only parse with
breakat first non-depth-option, passes remaining args as a slice without mutation - All round 1 P2 findings addressed: dead field removed,
-type ""validated, new test scenarios cover previously-untested predicates - Sandbox integrity remains solid — all filesystem access exclusively through
callCtx.ReadDir,callCtx.StatFile,callCtx.LstatFile - Expression parser bounds (64 depth, 256 nodes) prevent resource exhaustion
- Blocked predicate list is comprehensive
-Lsymlink loop bounded bymaxTraversalDepth=256(consistent withls -Rapproach)
This comment has been minimized.
This comment has been minimized.
|
Great PR overall — I did a full pass on the new What I reviewed
I also ran:
Findings1) P1 – Bash compatibility bug:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0c155cb1c
ℹ️ 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".
Add a sandboxed find builtin for searching directory trees by name, type, size, and time predicates. Uses a recursive-descent expression parser with bounded AST (max 64 depth, 256 nodes) and explicit-stack DFS traversal (capped at 256 depth). Blocks -exec, -delete, -regex and other unsafe predicates for sandbox safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P1: Fix -maxdepth/-mindepth extraction to only parse leading positions, preventing corruption of predicate arguments (e.g. find . -name -maxdepth). P2: Remove dead followLink field from evalContext, add empty-string validation for -type predicate, add test scenarios for -path, -ipath, -mmin, -mtime, -size variants, and -type empty error. P3: Document baseName forward-slash-only convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The field was nested under expect: where it was silently ignored by the YAML decoder. Removed since the test passes against bash anyway. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- builtins.go: Expand ReadDir comment to explain that sorted entries cause builtins (ls -R, find) to produce deterministic but different output ordering than GNU coreutils/findutils - find.go: Add NOTE explaining the ordering divergence at the walker - empty_file.yaml: Add skip_assert_against_bash (ordering divergence) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d0c155c to
b3d04ee
Compare
P1: Surface -newer reference file errors — report to stderr and set exit code 1 instead of silently returning false. Cache reference file modtime to avoid redundant stat calls per entry. P1: Detect symlink loops with -L — track visited directory paths to break cycles, preventing CPU/memory DoS on attacker-controlled trees. P2: Reject malformed -type arguments — validate comma-separated format properly, rejecting leading/trailing/consecutive commas and adjacent type chars without separators (e.g. ",", "f,", ",d", "fd"). Low: Add comment explaining why errorf uses errors.New(fmt.Sprintf()) instead of fmt.Errorf (fmt.Errorf is not in the import allowlist). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add fmt.Errorf to the import allowlist (pure function, no I/O) and replace all errorf() calls with fmt.Errorf() directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Absolutely — I completed a comprehensive code + security review of this PR’s Scope reviewed
I also validated behavior with:
FindingsP1 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10e4148c02
ℹ️ 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".
Cover all major code paths including symlinks (-L, loop detection, broken links), -newer, -true/-false, comma-separated -type, size units (c/w/k/M/b), exact numeric comparisons, logical operator aliases (-not/-and/-or), parser error paths, sandbox-blocked predicates, paths with spaces, pipe integration, and explicit -print. Brings total find test scenarios from 29 to 81. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix P1 bug where -newer with a nonexistent reference file would cache
a zero-time sentinel, causing subsequent entries (with fresh
evalContexts) to match against time.Time{} instead of returning false.
Replace per-evalContext newerErr flag with a shared newerErrors map
that persists across all entries in the traversal, consistent with
newerCache.
Also improve documentation:
- Clarify -maxdepth/-mindepth leading-only parse is intentional
- Document symlink loop detection path-text limitation and
maxTraversalDepth=256 safety bound
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move depth option parsing from a leading-only extraction loop into the recursive-descent expression parser itself. This allows commands like `find . -name '*.go' -maxdepth 1` to work correctly, while the parser's natural token ownership prevents the argument-stealing bug that the previous naive scan-and-extract approach suffered from. Add 9 test scenarios covering parser integration, anti-stealing, depth band selection, last-wins semantics, and edge cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
julesmcrt
left a comment
There was a problem hiding this comment.
Review Summary
This PR implements a find builtin with good overall design: sandbox enforcement goes through os.Root (immune to .. traversal), blocked predicates (-exec, -delete, -regex, etc.) are enforced at parse time, expression AST size is bounded (64 depth / 256 nodes), and the explicit-stack DFS avoids deep Go recursion. The import allowlist is correctly updated. No P0/P1 issues found.
Verdict: needs minor fixes — four P2 findings (one correctness bug, three coverage gaps) and two P3 issues.
…ings Path-based cycle detection missed loops that re-enter the same directory under different textual paths (e.g. dir/a/link_to_dir → dir), allowing expansion until maxTraversalDepth=256. Now track visited directories by canonical file identity (device + inode) on Unix via FileInfo.Sys(), with path-based fallback on Windows. Zero additional I/O — reuses FileInfo already in hand. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary — Round 8
Reviewed commit 4fc005f (round 7 changes) plus full find implementation.
Overall Assessment: Needs minor fixes
Security posture is excellent — zero direct filesystem access, all operations go through sandboxed CallContext callbacks, and the import allowlist enforces this at test time. The round 7 changes correctly address the -newer lstat/stat distinction and malformed bracket literal fallback for -name.
However, one bash compatibility bug was found in the -path predicate, and the new test scenarios have verification gaps.
Positive Observations
- All filesystem access goes through
CallContext— no directos.*calls - Import allowlist (
allowed_symbols_test.go) enforces sandbox at compile time - Expression parser has depth (64) and node (256) limits against resource exhaustion
- All dangerous predicates (
-exec,-delete,-regex) blocked at parse time - Symlink loop detection uses dual-layer approach (file identity + path fallback)
maxTraversalDepth=256as ultimate safety bound- Overflow-safe ceiling division in
compareSize
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fc005f9df
ℹ️ 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".
- Fix pathGlobMatch to treat malformed brackets as literals (bash compat) - Remove redundant strings.ToLower in matchGlobFold error fallback - Add pathGlobMatch malformed bracket unit tests and scenario test - Rewrite newer_symlink_ref test to use broken symlink (verifies lstat) - Update newer_dedup test description for accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I can only run on private repositories. |
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary — Round 9 (Self-Review)
Reviewed full find builtin implementation across find.go, eval.go, expr.go, match.go, match_test.go, portable_unix.go, portable_windows.go, runner_exec.go, and all 98 scenario tests.
Overall Assessment: Safe to merge ✓
No P0 or P1 findings. The implementation is security-sound with excellent sandbox enforcement.
Security Assessment
Sandbox integrity: PASS
- All filesystem access goes through
callCtx.StatFile,callCtx.LstatFile,callCtx.ReadDir— zero directos.*calls - Import allowlist (
allowed_symbols_test.go) enforces this at compile time - Blocked predicates (
-exec,-delete,-regex, etc.) rejected at parse time - Expression parser limits: depth=64, nodes=256 prevent resource exhaustion
maxTraversalDepth=256caps directory recursion- Symlink loop detection via dual-layer file identity + path tracking
No command/path injection vectors found.
Bash Compatibility
All 98 scenario tests pass. Key intentional divergences (with skip_assert_against_bash: true):
- Sorted output (vs filesystem-dependent order)
- Blocked predicates
- Error message formatting
Positive Observations
- Excellent test coverage with 98 scenario files covering basic, depth, errors, logic, output, pipe, predicates, prune, sandbox, size, and symlinks
- Overflow-safe ceiling division in
compareSize - Proper context cancellation checks throughout traversal
- Clean separation of parsing (
expr.go), evaluation (eval.go), matching (match.go), and traversal (find.go) - Malformed bracket fallback to literal matching in both
matchGlobandpathGlobMatch -newerwith lstat/stat respects-Lflag correctly
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d1151a1d7
ℹ️ 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".
- Detect and reject empty parentheses with GNU find-compatible error message - Add star + malformed bracket backtracking test cases to pathGlobMatch - Add empty_parens.yaml scenario test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I can only run on private repositories. |
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary — Round 10 (Self-Review)
Reviewed commit 4afc9fb (round 9 changes) on top of the previously-reviewed 5d1151a.
Overall Assessment: Safe to merge ✓
No findings. The delta is 3 files / 20 lines:
- Empty parentheses detection in
parseUnarywith GNU find-compatible error message — correct - Star + malformed bracket backtracking test cases — correct
empty_parens.yamlscenario test — correct
All prior findings from rounds 7–9 have been addressed. All review threads are resolved. 0 unresolved comments remain.
Security: PASS
- No new filesystem access patterns
- No new imports
- Expression parser change is purely additive (early return on empty parens)
Bash Compatibility: PASS
- Empty parens error message now matches GNU find format
- Scenario test validates the behavior
Test Coverage: Adequate
empty_parens.yamlcovers the new code path- Star + malformed bracket test covers the interaction
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4afc9fb208
ℹ️ 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".
- Reject unsupported -H flag with explicit error instead of silently ignoring - Fix baseName to strip trailing slashes for correct -name matching on dir/ - Reject +N and -N forms in -maxdepth/-mindepth to match GNU find - Add scenario tests for all three fixes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I can only run on private repositories. |
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary — Round 11 (Self-Review)
Reviewed commit b187f82 (round 10 changes).
Overall Assessment: Safe to merge ✓
No findings. The delta addresses 3 valid codex findings:
-Hrejected with explicit error instead of silently ignoring — correctbaseNamestrips trailing slashes — correctly preserveslen(p) > 1guard for root/— correctparseDepthOptionrejects+/-prefixed values — correct
All 3 have scenario tests. No regressions introduced.
Security: PASS
- No new filesystem access patterns
-Hrejection is more restrictive (safer) than silent acceptance
Test Coverage: Adequate
unsupported_H.yaml,trailing_slash.yaml,maxdepth_plus_sign.yamlcover all new paths
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b187f829ee
ℹ️ 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".
- Make -P override earlier -L (last global option wins, matching GNU find) - Fix baseName to return "/" for root path instead of empty string - Add P_overrides_L.yaml scenario test and TestBaseNameEdgeCases unit test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I can only run on private repositories. |
matt-dz
left a comment
There was a problem hiding this comment.
Review Summary — Round 12 (Self-Review)
Reviewed commit f87d171 (round 11 changes: -P overrides -L, root baseName fix).
Overall Assessment: Safe to merge ✓
No findings. Both changes are correct:
-Pnow setsfollowLinks = false— verified against GNU find that-L -Pdisables symlink followingbaseName("/")returns"/"— thelen(p) > 1guard correctly preserves root, and the newlen(tail) == 0check returns"/"instead of""TestBaseNameEdgeCasescovers root, trailing slashes, and normal pathsP_overrides_L.yamlcovers the-L -Poverride behavior
State
- Unresolved threads: 0
- CI: All 8 checks SUCCESS
- Security: No new filesystem access patterns
- Bash compatibility: Both fixes align with GNU find behavior
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f87d1714d6
ℹ️ 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".
- Fix symlink loop detection to use ancestor-chain tracking instead of global visited set. Multiple symlinks to the same target directory are now traversed correctly with -L, matching GNU find behavior. Only actual ancestor cycles (directory is its own ancestor) are flagged. - Add multiple_links_same_target.yaml scenario test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 793f6da4ba
ℹ️ 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".
- Fix -mmin to use math.Ceil for minute bucketing (matches GNU find's rounding-up behavior for fractional minutes) - Emit warning when -maxdepth exceeds safety limit of 256 instead of silently truncating - Update mmin_exact test to match corrected ceiling behavior - Add math.Ceil to import allowlist Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- eval_test.go: TestEvalMminCeiling (21 cases) verifies ceiling rounding for -mmin — prevents regression to math.Floor. TestEvalMtimeFloor (10 cases) verifies floor rounding for -mtime stays correct. TestCompareSizeOverflow covers edge cases including MaxInt64. - expr_test.go: TestParseDepthRejectsSignedValues (11 cases) ensures +N/-N forms are rejected. TestParseEmptyParens, TestParseSizeEdgeCases, TestParseBlockedPredicates, TestParseExpressionLimits cover parser correctness and security invariants. - match_test.go: Enhanced TestBaseNameEdgeCases with //, ///, ./, and additional path forms. Added TestMatchClassEdgeCases for bracket expressions and TestCompareNumeric for comparison operators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92d809da01
ℹ️ 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".
| case -1: // -n: strictly less | ||
| return actual < target |
There was a problem hiding this comment.
Use inclusive comparison for negative time predicates
compareNumeric uses a strict < check for -N, which makes -mmin/-mtime under-select files at GNU find boundary cases. In practice, files that GNU findutils 4.9.0 matches with negative predicates are dropped here (for example, a file ~30 seconds old with -mmin -1, and files on the 1-day boundary with -mtime -1), so scripts that depend on GNU-compatible age ranges can silently miss expected results.
Useful? React with 👍 / 👎.
| if strings.HasPrefix(arg, "-") && len(arg) > 1 { | ||
| // Distinguish expression predicates from paths like "-" or paths | ||
| // that happen to start with "-" (unlikely but possible). | ||
| // All find predicates start with a letter after the dash. | ||
| c := arg[1] | ||
| return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') |
There was a problem hiding this comment.
Parse all dash-prefixed tokens as expression starts
The expression/path split only treats -<letter> as an expression start, but GNU find switches to expression parsing for any dash-prefixed token once path parsing ends. This means malformed predicates like -1 are interpreted as paths instead of producing an unknown predicate error; if a file named -1 exists, find -1 succeeds and walks it, turning invalid filter input into unintended traversal behavior.
Useful? React with 👍 / 👎.
Summary
findbuiltin for searching directory trees with expression-based predicates-name,-iname,-path,-ipath,-type,-size,-empty,-newer,-mtime,-mmin,-maxdepth,-mindepth,-print,-print0,-prune,-true,-false, and logical operators (!,-a,-o,())-exec,-delete,-regex,-fprint, etc.) for sandbox safetyImplementation
interp/builtins/find/find.go— Command handler, arg parsing, DFS walkerinterp/builtins/find/expr.go— AST types + recursive-descent parserinterp/builtins/find/eval.go— Expression evaluatorinterp/builtins/find/match.go— Glob/type/size matching helperspath.Matchandstrings.ToLowerTest plan
go test ./interp/... ./tests/...— all passRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash— all pass (ordering-sensitive tests markedskip_assert_against_bashdue to ReadDir returning sorted entries vs GNU find's filesystem order)🤖 Generated with Claude Code