Implement find builtin command

[matt-dz]

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]

@codex review

[matt-dz]

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		interp/builtins/find/find.go:116	-maxdepth/-mindepth extraction steals arguments from predicates
2		interp/builtins/find/eval.go:31	Dead field followLink in evalContext
3		interp/builtins/find/	No Go unit tests for parser, evaluator, or matcher
4		tests/scenarios/cmd/find/	Missing test coverage for -mtime, -mmin, -newer, -path, -ipath, -L flag, -size with various units
5		interp/builtins/find/expr.go:308	-type accepts empty string argument without error
6		interp/builtins/find/match.go:124	baseName only handles / separator — add comment noting shell normalizes paths
7		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

[matt-dz]

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		tests/scenarios/cmd/find/errors/nonexistent.yaml:14	skip_assert_against_bash nested under expect (silently ignored)
2		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)

[AlexandreYang]

@codex @DataDog make a comprehensive code and security review

[datadog-prod-us1-6]

View session in Datadog

Bits Dev status: ✅ Done

CI Auto-fix: Disabled | Enable

Comment @DataDog to request changes

[datadog-official]

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

• Same as finding initial commit #1: precompute once and cache in AST/eval state.

---

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.

[chatgpt-codex-connector]

💡 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]

@codex @DataDog make a comprehensive code and security review

[datadog-datadog-prod-us1-2]

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.

[chatgpt-codex-connector]

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

[julesmcrt]

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

[matt-dz]

@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!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7375410f46

ℹ️ 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".

[chatgpt-codex-connector]

Detect symlink cycles when file identity lookup fails

The fallback cycle check uses the raw traversed path string, so loops like a/loop -> . become a/loop, a/loop/loop, a/loop/loop/loop, etc. and never hit ancestorPaths[path]; this means if FileIdentity is unavailable for a subtree (e.g., platform/ACL-specific identity lookup failure), loop detection is effectively disabled and traversal repeats until the hard depth cap, causing avoidable CPU and output amplification.

Useful? React with 👍 / 👎.

[matt-dz]

[Claude Opus 4.6] Valid observation. The path-based fallback won't detect cycles because the traversed path keeps growing (a/loop, a/loop/loop, ...) and never matches an ancestor entry.

However, I believe this is acceptable as-is for the following reasons:

1. FileIdentity works on all supported platforms (Linux/macOS via dev+inode, Windows via volume serial+file index). The path-based fallback only activates when the syscall fails, which is rare (exotic filesystems, ACL edge cases).

2. maxTraversalDepth=256 provides a hard safety cap — even if the fallback fails, traversal stops at depth 256. For an AI agent sandbox, this is the ultimate bound.

3. A proper fix would require symlink resolution (os.EvalSymlinks or equivalent), which isn't available within the builtin sandbox and would add syscall overhead per directory entry.

The existing code comments (lines 267-274) explicitly acknowledge the depth cap as the ultimate safety bound. If we want tighter cycle detection in the fallback, we could resolve canonical paths via StatFile + comparing dev/inode from the info, but that would duplicate what FileIdentity already does.

Leaving this for the reviewer to decide — not resolving the thread.

[matt-dz]

Hey @thieman — addressed your *os.File feedback (narrowed to fs.ReadDirFile) and picked up the remaining bot comments:

• Context cancellation: find now returns exit 1 when cancelled mid-traversal instead of silently succeeding with partial output.
• Future-dated file mtime: Removed the negative delta clamp so -mtime 0 no longer incorrectly matches future-dated files, matching GNU find.
• Symlink cycle fallback (path-based detection): Acknowledged the limitation but we think the tradeoff is acceptable here — FileIdentity works on all supported platforms, and the maxTraversalDepth=256 hard cap provides defense-in-depth for the rare case where it doesn't. Happy to revisit if you feel differently.

Ready for another look when you have a moment!

[matt-dz]

@chatgpt-codex-connector — addressed your review comments:

• Context cancellation: find now returns exit 1 when cancelled mid-traversal instead of silently succeeding with partial output.
• Future-dated file mtime: Removed the negative delta clamp so -mtime 0 no longer incorrectly matches future-dated files, matching GNU find.
• Symlink cycle fallback (path-based detection): Acknowledged the limitation but the tradeoff is acceptable here — FileIdentity works on all supported platforms, and the maxTraversalDepth=256 hard cap provides defense-in-depth for the rare case where it doesn't.

Could you take another look?

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9fbd9aa44

ℹ️ 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]

@chatgpt-codex-connector — addressed your latest feedback:

• Capture evaluation time once (cced11b): Moved callCtx.Now() from walkPath to run() so all root paths evaluate age predicates against a single invocation timestamp, matching GNU find.
• Added regression test (4a43391): TestNowCalledOnce asserts Now() is called exactly once per find invocation, regardless of how many root paths are provided.

Ready for another look!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a4339116e

ℹ️ 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]

@chatgpt-codex-connector — addressed your latest comment:

• Dangling -newer refs under -L (f4b613a): Both eager validation and evalNewer now fall back to LstatFile when StatFile fails with ErrNotExist under -L, matching GNU find. Added scenario test newer_dangling_symlink_L.

Ready for another look!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4b613a5e9

ℹ️ 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]

@chatgpt-codex-connector — merged main and resolved conflicts. All functionality intact, CI green on Linux/macOS/Docker. The only remaining failure is the verified/allowed_symbols label check (needs human approval since allowedsymbols/ was updated during the merge).

Ready for another look!

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Breezy!

ℹ️ 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]

@codex make a comprehensive code and security review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 32c6ee0d18

ℹ️ 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".

[chatgpt-codex-connector]

Reject escaped -newer references when -L cannot follow target

In -L mode this fallback treats every StatFile failure as if it were a dangling symlink and silently switches to LstatFile. If a -newer reference is a symlink inside allowed_paths that points outside the sandbox, StatFile returns a permission/path-escape error but LstatFile succeeds, so find -L ... -newer ref proceeds and exits 0 instead of enforcing the sandbox denial (while direct outside refs fail). This weakens the path restriction model and changes matching behavior; the fallback should only apply to dangling-target errors.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Surface symlink escape errors during -L directory walk

The -L walker also falls back from StatFile to LstatFile on any error, so symlinks that resolve outside allowed_paths are silently treated as regular links instead of reporting a sandbox violation. In practice, an in-sandbox link like out -> /etc makes find -L . succeed and print ./out with exit code 0, even though following the link is disallowed. This masks policy violations and makes -L ineffective for enforcing follow-link access checks.

Useful? React with 👍 / 👎.