Implement find builtin command by matt-dz · Pull Request #36 · DataDog/rshell

matt-dz · 2026-03-10T20:43:32Z

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 · 2026-03-10T20:45:54Z

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

#	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 · 2026-03-10T21:53:40Z

@codex @DataDog make a comprehensive code and security review

datadog-prod-us1-6 · 2026-03-10T21:53:57Z

View session in Datadog

Bits Dev status: ✅ Done

CI Auto-fix: Disabled | Enable

Comment @DataDog to request changes

datadog-official · 2026-03-10T21:58:27Z

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

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>

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 · 2026-03-11T13:12:58Z

@codex @DataDog make a comprehensive code and security review

datadog-datadog-prod-us1-2 · 2026-03-11T13:14:03Z

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

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

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.

#	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

…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 · 2026-03-11T14:23:51Z

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

The lstat fallback for dangling symlinks under -L was catching ALL stat errors, silently masking permission denied and sandbox escape errors. Now only falls back for "not found" errors (dangling symlinks), letting other errors propagate correctly. Added isNotExist() helper that checks both errors.Is and the portable error string, since PortablePathError strips the fs.ErrNotExist sentinel. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-16T12:58:10Z

@codex make a comprehensive code and security review

GNU find always uses lstat for -newer reference files regardless of -L/-P mode. Remove the followLinks branch and lstat fallback in both eager validation and evalNewer, and tighten isNotExist by removing the overly broad strings.Contains fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d61bb92cf

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

All 9 -newer test scenarios are now verified against GNU find (debian:bookworm-slim) via the Docker-based bash comparison suite. The skip_assert_against_bash flags were overly cautious — our stderr_contains assertions already handle format differences. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-16T13:30:24Z

[Claude Opus 4.6] Addressing Codex review comments from 2026-03-16:

1. isNotExist substring check (find.go:85) — Already fixed in commit 36324e4. The strings.Contains fallback has been replaced with return false. The two remaining checks (os.IsNotExist and errors.As with PathError) cover all sandbox error paths without substring matching.

2. Windows path separators (match.go:135) — Not a bug. The shell normalises all paths to forward slashes on all platforms (including Windows) before they reach find. This is documented in the comment on line 127-128: "The shell normalises all paths to forward slashes on all platforms, so hardcoding / is correct even on Windows."

matt-dz · 2026-03-16T13:38:44Z

[Claude Opus 4.6] Correction to my earlier reply about Windows path separators (match.go:135):

My previous reply was inaccurate — the shell does NOT globally normalize paths to forward slashes. I've now fixed this properly:

Start paths are normalized with filepath.ToSlash in run() (find.go)
-path, -ipath, -newer arguments are normalized with filepath.ToSlash via a new parsePathPredicate method (expr.go)
Internal paths from directory traversal already use / via joinPath
baseName comment updated to accurately describe why /-only checks are correct

Added filepath.ToSlash to the allowed symbols list and added Go unit tests verifying the normalization.

Commit: forthcoming.

Add filepath.ToSlash normalization for all path-like arguments: - Start paths normalized in run() - -path, -ipath, -newer arguments normalized via parsePathPredicate - baseName comment updated to accurately describe the invariant On Unix this is a no-op. On Windows it converts '\' to '/' so that internal path matching (baseName, joinPath, pathGlobMatch) works correctly with native path separators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-16T13:40:29Z

@codex make a comprehensive code and security review

The test only passes forward-slash paths, so it doesn't actually verify backslash normalization (filepath.ToSlash is a no-op on Unix). Renamed to accurately describe what it tests and added a note that actual backslash conversion is only exercised on Windows CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

filepath.ToSlash is a no-op on Unix, so the backslash-to-slash normalization in parsePathPredicate and run() can only be tested on Windows. Add a go:build windows test file that verifies -path, -ipath, -newer, -wholename, and -iwholename all convert '\' to '/', and that -name/-iname do NOT normalize (they match basenames only). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3485a2b864

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

GNU find with -L uses stat (not lstat) for -newer reference files, so the comparison uses the target file's mtime. Only when stat fails on a dangling symlink does it fall back to lstat. Our earlier change to always use lstat was incorrect — verified against GNU find: find -L dir -newer symlink_ref → uses target mtime (stat) find dir -newer symlink_ref → uses symlink mtime (lstat) Restore the followLinks → StatFile branch with isNotExist fallback in both eager validation and evalNewer. Add two scenario tests verified against GNU find via Docker: - newer_symlink_L_follows_target: -L uses target mtime - newer_symlink_P_uses_lstat: -P uses symlink mtime Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Lock in correct -L behavior for all metadata-reading predicates: - -L -type f follows symlink to regular file target - -L -type d follows symlink to directory target - -L -type l matches only dangling symlinks (stat fails) - -L -empty follows symlink to empty target file All verified against GNU find (debian:bookworm-slim) via Docker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Add scenario tests verified against GNU find (debian:bookworm-slim): - L_mtime_follows_target: -L -mtime uses target file mtime - P_mtime_uses_symlink: -P -mtime uses symlink mtime - L_mmin_follows_target: -L -mmin uses target file mtime - P_mmin_uses_symlink: -P -mmin uses symlink mtime Move Windows path normalization tests from a separate go:build windows file into the main expr_test.go behind a runtime.GOOS check. This matches the project's existing pattern (e.g. builtin_ls_pentest_test.go) and ensures the test is compiled on all platforms (catching syntax errors) while only running on Windows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-16T14:03:06Z

@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: 37e50e7a15

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

pathGlobMatch treated a dangling trailing backslash (escape with no following character) as a literal backslash match, producing false positives for -name/-path filters. GNU find's fnmatch treats this as non-matching. Verified against GNU find 4.9.0 via Docker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-16T14:41:49Z

@codex make a comprehensive code and security review

The trailing backslash test only needs to verify the pattern matches nothing — a file literally named \ is not required. Removing it fixes the Windows CI failure since \ is an illegal filename character there. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector · 2026-03-16T15:08:11Z

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ 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 requested review from AlexandreYang and thieman as code owners March 10, 2026 20:43

matt-dz commented Mar 10, 2026

View reviewed changes

Comment thread interp/builtins/find/find.go Outdated

Comment thread interp/builtins/find/eval.go Outdated

Comment thread builtins/find/expr.go

Comment thread builtins/find/match.go

matt-dz commented Mar 10, 2026

View reviewed changes

Comment thread tests/scenarios/cmd/find/errors/nonexistent.yaml Outdated

Comment thread interp/builtins/find/eval.go Outdated

This comment has been minimized.

Sign in to view

datadog-datadog-prod-us1-2 Bot reviewed Mar 10, 2026

View reviewed changes

Comment thread interp/builtins/find/expr.go Outdated

chatgpt-codex-connector Bot reviewed Mar 10, 2026

View reviewed changes

Comment thread interp/builtins/find/eval.go Outdated

Comment thread builtins/find/expr.go

Comment thread interp/builtins/find/find.go Outdated

matt-dz and others added 4 commits March 11, 2026 08:58

Fix misplaced skip_assert_against_bash in nonexistent.yaml

45331b5

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>

matt-dz force-pushed the matt-dz/implement-find-builtin branch from d0c155c to b3d04ee Compare March 11, 2026 12:59

matt-dz requested review from astuyve and julesmcrt as code owners March 11, 2026 12:59

AlexandreYang reviewed Mar 11, 2026

View reviewed changes

Comment thread tests/scenarios/cmd/find/basic/explicit_path.yaml

Use fmt.Errorf directly instead of errors.New(fmt.Sprintf())

10e4148

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>

chatgpt-codex-connector Bot reviewed Mar 11, 2026

View reviewed changes

Comment thread interp/builtins/find/eval.go Outdated

Comment thread interp/builtins/find/find.go Outdated

Comment thread interp/builtins/find/find.go Outdated

matt-dz and others added 3 commits March 11, 2026 09:29

julesmcrt reviewed Mar 11, 2026

View reviewed changes

Comment thread interp/builtins/find/match.go Outdated

Comment thread tests/scenarios/cmd/find/predicates/false.yaml Outdated

Comment thread interp/builtins/find/eval.go Outdated

Comment thread interp/builtins/find/find.go Outdated

Comment thread builtins/find/find.go

chatgpt-codex-connector Bot reviewed Mar 16, 2026

View reviewed changes

Comment thread builtins/find/match.go

Comment thread builtins/find/find.go Outdated

matt-dz and others added 3 commits March 16, 2026 09:44

format

92aa8c4

chatgpt-codex-connector Bot reviewed Mar 16, 2026

View reviewed changes

Comment thread builtins/find/eval.go Outdated

matt-dz and others added 3 commits March 16, 2026 09:56

chatgpt-codex-connector Bot reviewed Mar 16, 2026

View reviewed changes

Comment thread builtins/find/match.go Outdated

gh-worker-dd-devflow-36fce6 Bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Mar 16, 2026

gh-worker-dd-mergequeue-cf854d Bot merged commit 1e81597 into main Mar 16, 2026
30 checks passed

gh-worker-dd-devflow-36fce6 Bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Mar 16, 2026

gh-worker-dd-mergequeue-cf854d Bot deleted the matt-dz/implement-find-builtin branch March 16, 2026 15:10

matt-dz mentioned this pull request Mar 16, 2026

feat(find): add --help, -type b/c, -perm, -quit; fix access TOCTOU #104

Merged

6 tasks

Conversation

matt-dz commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation

Test plan

Uh oh!

matt-dz commented Mar 10, 2026

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Review Summary

Positive Observations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Review Summary (Round 2)

Positive Observations

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

AlexandreYang commented Mar 10, 2026

Uh oh!

datadog-prod-us1-6 Bot commented Mar 10, 2026 • edited by datadog-official Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

datadog-official Bot commented Mar 10, 2026

What I reviewed

Findings

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

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

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

Security review summary

Good security controls already present

Hardening gap (non-blocking but worth adding)

Recommendation on merge readiness

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matt-dz commented Mar 11, 2026

Uh oh!

datadog-datadog-prod-us1-2 Bot commented Mar 11, 2026

Scope reviewed

Findings

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

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

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

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

Security posture (positive)

Overall assessment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julesmcrt left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matt-dz commented Mar 11, 2026

matt-dz commented Mar 10, 2026 •

edited

Loading

datadog-prod-us1-6 Bot commented Mar 10, 2026 •

edited by datadog-official Bot

Loading

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

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

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

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

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

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

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