Skip to content

Implement find builtin command#36

Open
matt-dz wants to merge 24 commits intomainfrom
matt-dz/implement-find-builtin
Open

Implement find builtin command#36
matt-dz wants to merge 24 commits intomainfrom
matt-dz/implement-find-builtin

Conversation

@matt-dz
Copy link
Collaborator

@matt-dz matt-dz commented Mar 10, 2026

Summary

  • Add a sandboxed find builtin for searching directory trees with expression-based predicates
  • Supports -name, -iname, -path, -ipath, -type, -size, -empty, -newer, -mtime, -mmin, -maxdepth, -mindepth, -print, -print0, -prune, -true, -false, and logical operators (!, -a, -o, ())
  • Blocks unsafe predicates (-exec, -delete, -regex, -fprint, etc.) for sandbox safety
  • Uses recursive-descent expression parser with bounded AST (max 64 depth, 256 nodes) and explicit-stack DFS traversal (capped at 256 depth)

Implementation

  • interp/builtins/find/find.go — Command handler, arg parsing, DFS walker
  • interp/builtins/find/expr.go — AST types + recursive-descent parser
  • interp/builtins/find/eval.go — Expression evaluator
  • interp/builtins/find/match.go — Glob/type/size matching helpers
  • 24 YAML test scenarios covering basic, predicates, logic, depth, size, output, prune, errors, and sandbox categories
  • Import allowlist updated with path.Match and strings.ToLower

Test plan

  • go test ./interp/... ./tests/... — all pass
  • RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash — all pass (ordering-sensitive tests marked skip_assert_against_bash due to ReadDir returning sorted entries vs GNU find's filesystem order)
  • Pentest scenarios for resource exhaustion, symlink loops, blocked predicates
  • Code review cycle

🤖 Generated with Claude Code

@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 10, 2026

@codex review

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Priority File Finding
1 P1 Badge interp/builtins/find/find.go:116 -maxdepth/-mindepth extraction steals arguments from predicates
2 P2 Badge interp/builtins/find/eval.go:31 Dead field followLink in evalContext
3 P2 Badge interp/builtins/find/ No Go unit tests for parser, evaluator, or matcher
4 P2 Badge tests/scenarios/cmd/find/ Missing test coverage for -mtime, -mmin, -newer, -path, -ipath, -L flag, -size with various units
5 P2 Badge interp/builtins/find/expr.go:308 -type accepts empty string argument without error
6 P3 Badge interp/builtins/find/match.go:124 baseName only handles / separator — add comment noting shell normalizes paths
7 P3 Badge tests/scenarios/cmd/find/logic/or.yaml skip_assert_against_bash scenarios should document the reason for divergence

Positive Observations

  • All filesystem access uses sandbox APIs — no direct os.Open/os.Stat/os.ReadDir calls
  • 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
  • -newer file access goes through sandbox — no information leak outside AllowedPaths

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Priority File Finding
1 P3 Badge tests/scenarios/cmd/find/errors/nonexistent.yaml:14 skip_assert_against_bash nested under expect (silently ignored)
2 P3 Badge interp/builtins/find/eval.go:131 -newer stats reference file once per tree entry — could cache

Positive Observations

  • P1 fix is clean — leading-only parse with break at 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
  • -L symlink loop bounded by maxTraversalDepth=256 (consistent with ls -R approach)

@datadog-prod-us1-3

This comment has been minimized.

@AlexandreYang
Copy link
Member

@codex @DataDog make a comprehensive code and security review

@datadog-prod-us1-6
Copy link

datadog-prod-us1-6 bot commented Mar 10, 2026

View session in Datadog

Bits Dev status: ✅ Done

CI Auto-fix: Disabled | Enable

Comment @DataDog to request changes

@datadog-official
Copy link

Great PR overall — I did a full pass on the new find builtin implementation and tests with both code-quality and security lenses.

What I reviewed

  • interp/builtins/find/find.go
  • interp/builtins/find/expr.go
  • interp/builtins/find/eval.go
  • interp/builtins/find/match.go
  • Integration points:
    • interp/register_builtins.go
    • interp/builtins/builtins.go
    • tests/scenarios/cmd/find/**
    • tests/import_allowlist_test.go

I also ran:

  • go test ./tests -count=1
  • go test ./tests -run 'TestShellScenarios/cmd/find' -count=1
  • manual behavior repros with go run ./cmd/rshell ...

Findings

1) P1 – Bash compatibility bug: -newer missing reference is silently ignored

Location: interp/builtins/find/eval.go:130-134

evalNewer does:

refInfo, err := ec.callCtx.StatFile(ec.ctx, refPath)
if err != nil {
    return false
}

So a nonexistent/inaccessible reference file just makes predicate false per-entry, with no error and exit code 0.

Why this is a problem

  • GNU find errors out and exits non-zero when the -newer reference cannot be stat’d.
  • Silent failure can hide user mistakes and produce misleading “no matches” results.

Observed repro

  • rshell: exit 0, no stderr for missing ref.
  • GNU find: exit 1, explicit “No such file or directory”.

Recommended fix

  • Resolve/stat -newer reference once during parse/setup (or first use), cache mtime in expression node, and fail command immediately on error.

2) P2 – Input validation bug: malformed -type values accepted

Location:

  • Parser validation: interp/builtins/find/expr.go:310-317
  • Matcher behavior: interp/builtins/find/match.go:37-46

Current parser allows commas anywhere, so -type , is accepted. Matcher skips commas and returns false for all files, so command succeeds with zero output.

Why this is a problem

  • GNU find treats malformed -type arguments as an error (non-zero exit).
  • Accepting malformed predicates reduces compatibility and can mask mistakes.

Recommended fix

  • Parse as comma-separated tokens and require each token to be exactly one valid type char (f,d,l,p,s), no empty elements, no leading/trailing/consecutive commas.

3) P2 – Performance/security hardening: -newer does repeated stat per entry

Location: interp/builtins/find/eval.go:130-132

The reference path is stat’d for every visited file. On large trees this is avoidable overhead and can become expensive.

Recommended fix


Security review summary

Good security controls already present

  • Denylist blocks dangerous predicates (-exec, -delete, -fprint*, -regex, etc.) in expr.go.
  • AST complexity protections (maxExprDepth, maxExprNodes).
  • Traversal depth cap and explicit-stack DFS in find.go.
  • Uses sandboxed CallContext filesystem APIs only.

Hardening gap (non-blocking but worth adding)

  • No explicit test scenarios for -L symlink cycles/fanout exhaustion behavior (depth cap helps, but dedicated regression tests would be valuable).

Recommendation on merge readiness

I would not approve as-is due to finding #1 (-newer error handling).
After fixing #1 and #2, this looks strong and close to merge quality.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

matt-dz and others added 4 commits March 11, 2026 08:58
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>
@matt-dz matt-dz force-pushed the matt-dz/implement-find-builtin branch from d0c155c to b3d04ee Compare March 11, 2026 12:59
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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@codex @DataDog make a comprehensive code and security review

@datadog-datadog-prod-us1-2
Copy link

Absolutely — I completed a comprehensive code + security review of this PR’s find builtin.

Scope reviewed

  • interp/builtins/find/find.go
  • interp/builtins/find/expr.go
  • interp/builtins/find/eval.go
  • interp/builtins/find/match.go
  • Related wiring/docs/tests:
    • interp/register_builtins.go
    • interp/builtins/builtins.go
    • tests/scenarios/cmd/find/**
    • tests/import_allowlist_test.go
    • SHELL_FEATURES.md

I also validated behavior with:

  • go test ./tests -count=1
  • go test ./tests -run 'TestShellScenarios/cmd/find' -count=1
  • manual GNU find vs rshell repros.

Findings

P1 — -newer missing reference file is silently ignored (bash compatibility + correctness)

Where: interp/builtins/find/eval.go (evalNewer)

Current behavior:

  • If StatFile(refPath) fails, code returns false for that entry.
  • Command can finish with exit 0 and no stderr.

Expected (GNU find):

  • Missing/inaccessible -newer reference should be an error with non-zero exit.

Impact

  • User mistakes are silently masked as “no matches”.
  • Diverges from expected find semantics.

Fix recommendation

  • Validate/stat -newer reference once during parse/setup.
  • If stat fails, print error and exit 1.
  • Cache reference mtime for evaluation (also improves performance).

P2 — malformed -type arguments accepted (compatibility/input validation)

Where:

  • Validation: interp/builtins/find/expr.go (parseTypePredicate)
  • Matching: interp/builtins/find/match.go (matchType)

Current parser accepts comma anywhere, so -type , is accepted and just matches nothing (exit 0).

Expected (GNU find):

  • malformed type list should error out (exit 1).

Fix recommendation

  • Parse -type as comma-separated tokens.
  • Require each token to be exactly one valid type char (f,d,l,p,s).
  • Reject empty segments (leading/trailing/double commas).

P2 — avoid repeated stat of -newer reference per file (perf/security hardening)

Where: interp/builtins/find/eval.go (evalNewer)

Reference file is stat’d for every visited path.

Impact

  • Unnecessary overhead on large trees.
  • Can amplify cost under heavy workload.

Fix recommendation

  • Resolve reference once and reuse cached modtime.

P3 — hardening gap: no dedicated tests for -L symlink cycles / traversal exhaustion

Where: behavior in interp/builtins/find/find.go; test coverage gap in tests/scenarios/cmd/find/**

The implementation has good depth caps (maxTraversalDepth, AST limits), but there are no explicit regression scenarios for:

  • symlink loops with -L
  • large/fanout traversal stress

Fix recommendation

  • Add pentest scenarios for loop/fanout limits and confirm deterministic non-hanging behavior.

Security posture (positive)

Strong design choices already present:

  • Dangerous predicates blocked (-exec, -delete, -fprint*, -regex, etc.).
  • AST complexity bounded (maxExprDepth, maxExprNodes).
  • Explicit-stack DFS with traversal depth cap.
  • All file operations go through sandboxed CallContext APIs.
  • Import allowlist remains strict.

Overall assessment

  • Do not merge yet due to P1 (-newer error handling).
  • After fixing P1 + P2, this is close to merge-ready.
  • Security model is solid, with remaining work mainly in hardening tests and one perf/correctness cleanup.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

matt-dz and others added 3 commits March 11, 2026 09:29
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>
Copy link
Collaborator

@julesmcrt julesmcrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Priority File Finding
1 P2 Badge interp/builtins/find/match.go:96 Integer overflow in compareSize for file sizes near MaxInt64
2 P2 Badge tests/scenarios/cmd/find/predicates/false.yaml skip_assert_against_bash: true on tests whose output is order-independent
3 P2 Badge tests/scenarios/cmd/find/ Missing sandbox coverage: no test for -newer with a path outside allowed_paths
4 P2 Badge interp/builtins/find/find.go:111–117 Intentional -maxdepth-after-predicates limitation has no scenario test
5 P3 Badge interp/builtins/find/ Missing pentest Go tests (acknowledged in PR checklist)
6 P3 Badge tests/scenarios/cmd/find/ No stdout_windows overrides for path-separator output on Windows

…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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@codex @DataDog Ready for another review — this latest commit replaces path-based symlink loop detection with canonical file identity (dev+inode) tracking, addressing the P1 DoS concern. Would appreciate a look at the FileIdentity callback pattern and the Windows fallback path. Thanks!

@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@codex review

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Priority File Finding
1 P1 Badge interp/builtins/find/match.go:183-192 pathGlobMatch doesn't treat malformed brackets as literals — diverges from bash -path behavior
2 P2 Badge tests/scenarios/cmd/find/predicates/newer_dedup.yaml stderr_contains doesn't verify dedup — only checks presence, not uniqueness
3 P2 Badge tests/scenarios/cmd/find/predicates/newer_symlink_ref.yaml Test doesn't distinguish lstat vs stat — all files created within ms
4 P3 Badge interp/builtins/find/match.go:28 Redundant strings.ToLower in matchGlobFold error fallback
5 P3 Badge interp/builtins/find/match_test.go Missing test for pathGlobMatch with malformed bracket patterns

Positive Observations

  • All filesystem access goes through CallContext — no direct os.* 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=256 as ultimate safety bound
  • Overflow-safe ceiling division in compareSize

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@DataDog @codex make a comprehensive code and security reviews

@datadog-prod-us1-6
Copy link

I can only run on private repositories.

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Priority File Finding
1 P3 Badge interp/builtins/find/expr.go:230-244 Empty parentheses () produce different error message than GNU find
2 P3 Badge interp/builtins/find/match.go:184-198 pathGlobMatch malformed bracket + star backtracking interaction not tested

Security Assessment

Sandbox integrity: PASS

  • All filesystem access goes through callCtx.StatFile, callCtx.LstatFile, callCtx.ReadDir — zero direct os.* 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=256 caps 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 matchGlob and pathGlobMatch
  • -newer with lstat/stat respects -L flag correctly

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@DataDog @codex make a comprehensive code and security reviews

@datadog-prod-us1-5
Copy link

I can only run on private repositories.

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Empty parentheses detection in parseUnary with GNU find-compatible error message — correct
  2. Star + malformed bracket backtracking test cases — correct
  3. empty_parens.yaml scenario 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.yaml covers the new code path
  • Star + malformed bracket test covers the interaction

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@DataDog @codex make a comprehensive code and security reviews

@datadog-prod-us1-6
Copy link

I can only run on private repositories.

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. -H rejected with explicit error instead of silently ignoring — correct
  2. baseName strips trailing slashes — correctly preserves len(p) > 1 guard for root / — correct
  3. parseDepthOption rejects +/- prefixed values — correct

All 3 have scenario tests. No regressions introduced.

Security: PASS

  • No new filesystem access patterns
  • -H rejection is more restrictive (safer) than silent acceptance

Test Coverage: Adequate

  • unsupported_H.yaml, trailing_slash.yaml, maxdepth_plus_sign.yaml cover all new paths

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@DataDog @codex make a comprehensive code and security reviews

@datadog-prod-us1-5
Copy link

I can only run on private repositories.

Copy link
Collaborator Author

@matt-dz matt-dz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. -P now sets followLinks = false — verified against GNU find that -L -P disables symlink following
  2. baseName("/") returns "/" — the len(p) > 1 guard correctly preserves root, and the new len(tail) == 0 check returns "/" instead of ""
  3. TestBaseNameEdgeCases covers root, trailing slashes, and normal paths
  4. P_overrides_L.yaml covers the -L -P override 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

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@codex make a comprehensive code and security reviews

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

matt-dz and others added 2 commits March 11, 2026 17:42
- 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>
@matt-dz
Copy link
Collaborator Author

matt-dz commented Mar 11, 2026

@codex make a comprehensive code and security reviews

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +127 to +128
case -1: // -n: strictly less
return actual < target

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +183 to +188
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')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants