Add grep cmd and docs#32
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 12 commits intomainfrom Mar 11, 2026
Merged
Conversation
astuyve
requested review from
AlexandreYang,
matt-dz and
thieman
as code owners
matt-dz
reviewed
Mar 10, 2026
Collaborator
There was a problem hiding this comment.
Security Review Summary
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
interp/builtins/grep/grep.go:129 |
MaxContextLines (1<<20) allows excessive memory allocation via -A/-B/-C |
Scope: Reviewed 41 changed files from PR #32.
Verdict: Well-engineered, security-conscious implementation. RE2 regex engine prevents ReDoS, file access is sandboxed via callCtx.OpenFile(), import allowlist is enforced at the AST level, and unknown flags are properly rejected. One defense-in-depth improvement recommended for the context line cap.
thieman
approved these changes
Mar 10, 2026
Co-authored-by: Matthew DeGuzman <91019033+matt-dz@users.noreply.github.com>
Member
|
@codex review |
Member
|
@DataDog review |
|
I pushed the changes you requested: Summary
Root Cause & Fix (ONLY include this section if the task involves fixing a bug, error, or issue)
Testing
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 743b5df328
ℹ️ 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".
AlexandreYang
previously requested changes
Mar 10, 2026
Member
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the grep builtin implementation: ~605 lines of Go in grep.go, ~880 lines of tests (unit + pentest), 27 YAML scenarios, documentation updates, and import allowlist additions.
Overall Assessment: needs fixes
The implementation is well-structured with strong security foundations:
- All file access through
callCtx.OpenFilesandbox ✓ - No direct
os.Open/os.Stat/os.ReadFilecalls ✓ - RE2 engine prevents ReDoS ✓
- 1 MiB line cap prevents unbounded allocation ✓
- Context cancellation checked every iteration ✓
- Thorough pentest coverage (path traversal, long lines, resource exhaustion) ✓
- Import allowlist properly updated ✓
However, there are two bash compatibility issues, one of which (P1) produces incorrect output:
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
interp/builtins/grep/grep.go:522 |
-o -v prints full lines instead of nothing |
| 2 | |
interp/builtins/grep/grep.go:221-230 |
Conflicting matchers (-E -G) not detected |
Positive Observations
- Clean separation of concerns: flag parsing, pattern compilation, file iteration, and line matching are well-isolated
- BRE→ERE conversion is correct for the supported metacharacters
- Context line tracking (before/after) with group separators matches GNU grep behavior
- The
MaxContextLinescap (10k) is a good safety measure - Streaming design — no file is loaded entirely into memory
- Comprehensive test suite with both unit tests and security-focused pentest tests
- YAML scenarios correctly use
skip_assert_against_bash: truewhere behavior intentionally diverges
Member
Contributor
|
I pushed the changes you requested: Completed a comprehensive review of the existing grep PR and found one additional GNU-compatibility gap, then fixed it. ✅ What I reviewedI audited the
🔎 Issue found (and fixed)I identified a behavior mismatch with GNU grep:
In GNU grep:
🛠️ Changes made
🧪 Verification run
I also updated PR metadata (title/body/commit message) with cumulative changes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33d119eb06
ℹ️ 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".
Member
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the new grep builtin implementation: grep.go (666 lines), grep_test.go (57 tests), builtin_grep_pentest_test.go (22 pentest tests), 27 YAML scenarios, plus registration, import allowlist, and documentation updates.
Overall Assessment: Safe to merge (with one hardening suggestion)
This is a well-implemented, security-conscious grep builtin. The key security invariants are all in place.
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | |
interp/builtins/grep/grep.go:130 |
Before-context buffer has no total byte cap (unlike tail's MaxRingBytes) |
| 2 |  |
interp/builtins/grep/grep.go:130 |
MaxContextLines (10k) × MaxLineBytes (1 MiB) = ~10 GiB theoretical max |
Positive Observations
- Sandbox integrity: All file access goes through
callCtx.OpenFile— no directos.Open/os.Stat/os.ReadFilecalls. The onlyosimport is for theos.O_RDONLYconstant. - ReDoS prevention: Uses Go's
regexppackage (RE2 engine) guaranteeing linear-time matching. Excellent choice for a shell used by untrusted scripts. - Streaming processing: Input is read line-by-line with a 1 MiB per-line cap. No full-file buffering.
- Context cancellation: Every scan loop iteration checks
ctx.Err(), honoring execution timeouts. - Import allowlist compliance: Only
regexp.Compile,regexp.QuoteMeta, andregexp.Regexpare added — all pure functions/types with no I/O side effects. - BRE→ERE conversion: Correctly swaps escaping for
( ) { } + ? |characters between BRE and ERE semantics. - Flag injection prevention: Unknown flags are rejected.
--separator is handled correctly. Pentest tests cover flag-via-expansion. - Comprehensive testing: 57 unit tests + 22 pentest tests + 27 YAML scenarios covering basic matching, all flags, context lines, error cases, and hardening.
- Bash compatibility: YAML scenarios are asserted against bash by default.
skip_assert_against_bash: trueis only used where error messages intentionally diverge (conflicting matchers message format, invalid regex error format, missing file error from sandbox). - Ordered
-l/-Lresolution: TheorderedBoolFlagpattern correctly implements GNU grep's "last one wins" semantics for conflicting output modes. - Quiet mode semantics: Correctly returns exit 0 immediately on first match even with preceding file errors, matching GNU grep behavior.
astuyve
commented
Mar 11, 2026
Collaborator
Author
astuyve
left a comment
astuyve
left a comment
There was a problem hiding this comment.
Code Review — PR #32: Add grep builtin
Reviewer: Self-review (automated)
Overall Assessment:
Review Summary
The grep builtin implementation is well-structured with good security properties: all file access goes through the sandbox, input is streamed line-by-line with a 1 MiB cap, context cancellation is checked every loop iteration, and the RE2 engine prevents ReDoS. The test coverage is comprehensive with 60+ unit tests and dedicated pentest tests.
However, several bash compatibility issues flagged in prior reviews remain unresolved. I've verified each against the current code on the branch.
Findings
P1 — Critical (Bash Compatibility)
1. `-h`/`-H` last-option precedence not implemented
- Location: `grep.go:256-258`
- Issue: GNU grep resolves `-h`/`-H` by option order (`grep -H -h` hides filenames, `grep -h -H` shows filenames). The current code always forces `showFilename = false` whenever `-h` is present, regardless of order.
- Evidence: `showFilename := len(files) > 1 || *withFilename; if *noFilename { showFilename = false }`
- Fix: Track the order of `-h` and `-H` flags (similar to the `orderedBoolFlag` approach from a prior version, or use `fs.Changed()` with a sequence counter).
2. `-c`/`-l`/`-L` interaction not implemented
- Location: `grep.go:266-268` and `grep.go:576-589`
- Issue: GNU grep treats `-l` as overriding `-c` (i.e., `grep -cl` behaves like `grep -l`). The current code emits both count AND filename output when `-c` and `-l` are both set.
- Evidence: The `grepOpts` struct stores `count`, `filesWithMatches`, and `filesWithoutMatch` independently with no precedence resolution. The `grepFile` function at lines 576-589 will print count output AND then check `filesWithMatches`.
- Fix: Resolve precedence: `-l`/`-L` should override `-c`. In GNU grep, the last of `-c`, `-l`, `-L` wins.
3. `-o` should suppress context lines
- Location: `grep.go:487-569` (the scan loop)
- Issue: When `-o` is combined with `-A`/`-B`/`-C`, GNU grep outputs only the matched fragments without context lines. The current code still prints context lines because it does not gate context output on `onlyMatching`.
- Evidence: The after-context printing at line 556 and before-context at line 523 have no `onlyMatching` guard.
- Fix: When `opts.onlyMatching` is true, skip before-context and after-context printing (set `afterRemaining = 0` and don't fill `beforeBuf`).
P2 — Warning
4. Newline-delimited patterns not split
- Location: `grep.go:208-217`
- Issue: GNU grep documents that PATTERNS may contain multiple patterns separated by newlines. For example, `grep "foo\nbar"` should match lines containing "foo" OR "bar". The current code treats each `-e` argument and the positional pattern as a single regex atom without splitting on newlines.
- Fix: Split each pattern string on `\n` before passing to `compilePatterns`.
5. `MaxContextLines` allows excessive memory for before-context buffer
- Location: `grep.go:130`, `grep.go:562-568`
- Issue: `MaxContextLines = 10,000` combined with `MaxLineBytes = 1 MiB` means the `beforeBuf` ring buffer could theoretically hold ~10 GB of data. While this requires adversarial input (10k lines each near 1 MiB), it could be a DoS vector.
- Fix: Consider adding a total byte cap on `beforeBuf` (e.g., 10 MiB total), or reducing `MaxContextLines` to a more reasonable value like 1,000.
6. Comment on line 307 is misleading
- Location: `grep.go:307`
- Issue: The comment says "GNU grep: error takes precedence over match (exit 2 > exit 0)" — this is actually correct behavior per POSIX (without `-q`, errors produce exit 2 regardless of matches). The comment is fine but could be clearer. Note: the codex comment suggesting the opposite is incorrect.
- Fix: Minor — consider expanding the comment to cite the POSIX spec or GNU grep man page for clarity.
P3 — Info
7. Several YAML error scenarios use `skip_assert_against_bash: true` unnecessarily
- Location: `nonexistent_file.yaml`, `conflicting_matchers.yaml`, `no_pattern.yaml`, `invalid_regex.yaml`, `unknown_flag.yaml`
- Issue: Some of these scenarios could potentially be tested against bash if the error message format is adjusted. For example, `nonexistent_file.yaml` uses `stderr_contains` which should be flexible enough. However, some (like `conflicting_matchers.yaml` and `unknown_flag.yaml`) genuinely diverge since the shell's flag parsing differs from GNU grep's.
- Fix: Review each `skip_assert_against_bash: true` scenario to determine if it can be tested against bash.
Positive Observations
- Sandbox integrity maintained: All file access goes through `callCtx.OpenFile()` — no direct `os.Open`/`os.Stat` calls.
- ReDoS protection: RE2 engine guarantees linear-time matching.
- Context cancellation: Every scan loop iteration checks `ctx.Err()`.
- Line cap enforced: `MaxLineBytes = 1 MiB` prevents unbounded allocation from long lines.
- Comprehensive pentest tests: Path traversal, large files, many files, ReDoS patterns, null bytes, etc.
- BRE-to-ERE conversion: Well-implemented with proper escaping swap.
- Import allowlist properly updated with safety justifications for all new symbols.
Summary
| Severity | Count |
|---|---|
| P1 (Critical) | 3 |
| P2 (Warning) | 3 |
| P3 (Info) | 1 |
Recommendation: Address P1 items (bash compat: -h/-H precedence, -c/-l/-L interaction, -o context suppression) before merging. P2 items should be addressed but are less urgent.
Collaborator
Author
|
To use Codex here, create a Codex account and connect to github. |
Contributor
|
Great request — I performed a full pass on the current Scope reviewed
Validation run
Findings1) High — Potential memory exhaustion with large
|
- Implement last-option-wins for -h/-H using orderedBoolFlag (matches GNU grep) - Suppress context lines (-A/-B/-C) when -o is active (GNU grep behavior) - Split newline-delimited patterns before compilation (GNU grep behavior) - Reduce MaxContextLines from 10k to 1k to limit memory exposure - Add strings.Split and strconv.ParseBool to import allowlist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
To use Codex here, create a Codex account and connect to github. |
Contributor
|
Re-review complete — I checked the new fixes specifically and re-ran targeted validation/security checks. ✅ What I re-reviewedRecent changes in
✅ Verification resultsBehavior spot-check against GNU grepI ran side-by-side checks with system
Result: rshell output/exit behavior matched GNU grep on these cases. Targeted automated checks run
Security review updateImproved
Remaining risk (still present)
Recommendation: add a byte-based cap for buffered context (evict oldest lines when total buffered bytes exceeds threshold), in addition to line-count cap. Final verdictThe recent fixes are correct and materially improve GNU compatibility. |
# Conflicts: # tests/import_allowlist_test.go
AlexandreYang
approved these changes
Mar 11, 2026
AlexandreYang
pushed a commit
that referenced
this pull request
Mar 11, 2026
Add grep cmd and docs Merge branch 'main' into aj/add-grep-skill Update interp/builtins/grep/grep.go Co-authored-by: Matthew DeGuzman <91019033+matt-dz@users.noreply.github.com> Merge branch 'main' into aj/add-grep-skill Merge branch 'aj/add-grep-skill' of github.com:DataDog/rshell into aj/add-grep-skill Fix grep exit code on mixed file errors more tests, fix -o, exit with conflicting matchers Merge branch 'aj/add-grep-skill' of github.com:DataDog/rshell into aj/add-grep-skill Fix grep -l/-L precedence and -c behavior Fix grep -h/-H precedence, -o context suppression, newline patterns - Implement last-option-wins for -h/-H using orderedBoolFlag (matches GNU grep) - Suppress context lines (-A/-B/-C) when -o is active (GNU grep behavior) - Split newline-delimited patterns before compilation (GNU grep behavior) - Reduce MaxContextLines from 10k to 1k to limit memory exposure - Add strings.Split and strconv.ParseBool to import allowlist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Merge remote-tracking branch 'origin/main' into aj/add-grep-skill # Conflicts: # tests/import_allowlist_test.go Merge branch 'main' into aj/add-grep-skill Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> Co-authored-by: datadog-prod-us1-5[bot] <266081015+datadog-prod-us1-5[bot]@users.noreply.github.com> Co-authored-by: aj.stuyvenberg <aj.stuyvenberg@datadoghq.com>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Mar 11, 2026
Implement if/elif/else clause support Add support for POSIX if/elif/else conditional constructs in the shell interpreter. This removes the validation block on *syntax.IfClause and adds execution logic following the same pattern as mvdan/sh's reference implementation. Key changes: - Remove IfClause block from validate.go (child node validation still applies, so blocked features like subshells in conditions are rejected) - Add case *syntax.IfClause in cmd() dispatcher in runner_exec.go - Fix stmts() to check break/continue/exit between statements, ensuring break inside if bodies within loops propagates correctly - Clamp excess break/continue levels at outermost loop (matches bash) - Comprehensive YAML scenario tests and Go tests covering basic if/else, elif chains, nested ifs, conditions (pipes, &&/||, test builtin, negation), exit code propagation, and loop interaction - Pentest tests for deeply nested ifs, context cancellation, blocked features in conditions, and break/continue edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Add test coverage combining if/elif with test/[ builtin Add 10 YAML scenario tests and 9 Go tests covering: - String equality/inequality with [ = ] and [ != ] - Integer comparisons: -eq, -lt, -le, -gt, -ge, -ne - Elif chains with integer range classification - Empty/non-empty string checks with -z and -n - File and directory existence with -f and -d - Combined [ ] conditions with && and || - Negated [ ] conditions with ! - Elif + test builtin inside for loops Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Replace Go tests with YAML scenario tests where possible Move all if/elif/else tests that can be expressed as YAML scenarios out of if_clause_test.go into scenario files. Delete if_clause_test.go entirely — every test it contained already has a YAML scenario equivalent. Keep only the pentest Go tests (deeply nested ifs, context cancellation, programmatic script generation) which require Go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Address review comments: fix YAML scenario metadata - if_in_pipeline.yaml: fix misleading description (said "blocked" but test expects success) - exit_in_then.yaml: remove unnecessary skip_assert_against_bash (bash produces same output) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Update CODEOWNERS Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Implement cut builtin command (#47) Add review-fix-loop skill for iterative self-review and fix coordination Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Update review-fix-loop: auto-detect current branch PR, request external reviews in parallel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Add cut builtin implementation with byte/field/complement modes and scenario tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Add cut builtin tests, pentest, GNU compat tests, and scenario tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Improve review-fix-loop skill reliability with structured task gating and gate checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Fix stripNewline to preserve \r bytes, matching GNU cut behavior GNU cut treats only \n as the line terminator. The \r byte in a \r\n sequence is a regular content byte at a regular position. The previous implementation incorrectly stripped \r after \n, which: - Lost \r bytes from line content - Shifted byte positions (e.g. for "ab\r\n", byte 3 should be \r) - Corrupted field content (e.g. for "a:b:c\r\n" -d: -f3, lost the \r) Add tests for CRLF last-field and byte-mode to verify \r preservation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Add sub-steps and parallel annotations to review-fix-loop task list Expand the task list from 4 to 9 items (including 2A–2E sub-steps), add parallelism indicators, and improve the execution order diagram. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Split review-fix-loop parallel sub-steps into distinct tasks Split 2A into 2A1 (self-review) and 2A2 (external reviews) as parallel sub-steps. Split 2B into 2B1 (address-pr-comments) and 2B2 (fix-ci-tests) as parallel sub-steps. Update all cross-references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Make 2B1 (address-pr-comments) and 2B2 (fix-ci-tests) sequential Run address-pr-comments before fix-ci-tests so CI fixes operate on code that already incorporates review feedback. Simplify 2C since parallel conflict resolution is no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> seq Rename review-fix-loop sequential sub-steps 2B1/2B2 to 2B/2C and shift subsequent steps Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Add Step 3 fallback to Step 2 when verification fails Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Address review comments: bash compatibility fixes for cut builtin - Remove -h short flag (GNU cut doesn't support it, exits 1 for cut -h) - Make -c (character mode) behave byte-wise matching GNU cut behavior - Remove processChars function (now unused since -c routes to processBytes) - Replace sort.Slice with slices.SortFunc to reduce import allowlist surface - Remove sort.Slice from import allowlist, remove unused unicode/utf8 import - Clean up noSplitMultibyte field (stored but never read, -n is a no-op) - Update test expectations for -c multibyte to match GNU byte-wise behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Update CODEOWNERS Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> improve .claude/skills/code-review/SKILL.md tests coverage Merge branch 'main' into alex/builtin-cut Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Prefer scenario tests over Go tests in AGENTS.md testing guidelines (#46) Prefer scenario tests over Go tests in AGENTS.md testing guidelines Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Fail import allowlist test on unused symbols (#43) Fail import allowlist test on unused symbols Add a check that every symbol in builtinAllowedSymbols is actually referenced by at least one builtin file. This keeps the allowlist minimal and prevents stale entries from accumulating. Remove io/fs.DirEntry which was the one unused symbol caught. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Rename import allowlist test to allowed symbols test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Fix Windows CI: ls sandbox test expects wrong error for /etc On Windows, /etc is not an absolute path (no drive letter), so it resolves to <cwd>/etc inside the sandbox, yielding "no such file or directory" instead of "permission denied". Add stderr_windows override. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Merge branch 'main' into alex/allowed_symbols_fail_unused Merge branch 'main' into alex/allowed_symbols_fail_unused Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Add grep cmd and docs (#32) Add grep cmd and docs Merge branch 'main' into aj/add-grep-skill Update interp/builtins/grep/grep.go Co-authored-by: Matthew DeGuzman <91019033+matt-dz@users.noreply.github.com> Merge branch 'main' into aj/add-grep-skill Merge branch 'aj/add-grep-skill' of github.com:DataDog/rshell into aj/add-grep-skill Fix grep exit code on mixed file errors more tests, fix -o, exit with conflicting matchers Merge branch 'aj/add-grep-skill' of github.com:DataDog/rshell into aj/add-grep-skill Fix grep -l/-L precedence and -c behavior Fix grep -h/-H precedence, -o context suppression, newline patterns - Implement last-option-wins for -h/-H using orderedBoolFlag (matches GNU grep) - Suppress context lines (-A/-B/-C) when -o is active (GNU grep behavior) - Split newline-delimited patterns before compilation (GNU grep behavior) - Reduce MaxContextLines from 10k to 1k to limit memory exposure - Add strings.Split and strconv.ParseBool to import allowlist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Merge remote-tracking branch 'origin/main' into aj/add-grep-skill # Conflicts: # tests/import_allowlist_test.go Merge branch 'main' into aj/add-grep-skill Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> Co-authored-by: datadog-prod-us1-5[bot] <266081015+datadog-prod-us1-5[bot]@users.noreply.github.com> Co-authored-by: aj.stuyvenberg <aj.stuyvenberg@datadoghq.com> Add dependabot.yml for weekly GitHub Actions and Go module updates (#48) Add dependabot.yml for weekly GitHub Actions and Go module updates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Bump the go-dependencies group with 2 updates (#50) Bump the go-dependencies group with 2 updates Bumps the go-dependencies group with 2 updates: [github.com/spf13/pflag](https://github.com/spf13/pflag) and [mvdan.cc/sh/v3](https://github.com/mvdan/sh). Updates `github.com/spf13/pflag` from 1.0.9 to 1.0.10 - [Release notes](https://github.com/spf13/pflag/releases) - [Commits](spf13/pflag@v1.0.9...v1.0.10) Updates `mvdan.cc/sh/v3` from 3.12.0 to 3.13.0 - [Release notes](https://github.com/mvdan/sh/releases) - [Changelog](https://github.com/mvdan/sh/blob/master/CHANGELOG.md) - [Commits](mvdan/sh@v3.12.0...v3.13.0) --- updated-dependencies: - dependency-name: github.com/spf13/pflag dependency-version: 1.0.10 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies - dependency-name: mvdan.cc/sh/v3 dependency-version: 3.13.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Bump the gh-actions-packages group with 2 updates (#51) Bump the gh-actions-packages group with 2 updates Bumps the gh-actions-packages group with 2 updates: [actions/checkout](https://github.com/actions/checkout) and [actions/setup-go](https://github.com/actions/setup-go). Updates `actions/checkout` from 4.2.2 to 6.0.2 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@11bd719...de0fac2) Updates `actions/setup-go` from 5.6.0 to 6.3.0 - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@40f1582...4b73464) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 6.0.2 dependency-type: direct:production update-type: version-update:semver-major dependency-group: gh-actions-packages - dependency-name: actions/setup-go dependency-version: 6.3.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: gh-actions-packages ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Implement strings builtin command (#49) Add builtins strings Fix strings -o/-t flag ordering and empty radix validation Implement last-flag-wins semantics for -o and -t by replacing the StringP/BoolP pair with custom pflag.Value types (radixFlagVal, octalFlagVal) that share the same *radixFormat pointer. pflag calls Set() in parse order, so whichever flag appears last on the command line naturally wins — matching GNU strings behavior where -o is just an alias for -t o. Also fix empty radix validation: the old `if *radix != ""` guard silently treated --radix= / -t '' as "no radix set". With Set() validation, empty values hit the default case and pflag reports an error before the handler runs. Add test scenarios: - errors/empty_radix: --radix= is rejected as invalid - offset/flag_order_t_then_o: -t d -o → octal (last wins) - offset/flag_order_o_then_t: -o -t d → decimal (last wins) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com> Fix continue with excess depth breaking loop instead of continuing When `continue N` exceeded the loop nesting depth (e.g. `continue 99` in a single loop), the shell incorrectly treated it like `break` and exited the loop after the first iteration. Bash discards excess levels and continues iterating. The fix checks for excess continue levels after `loopStmtsBroken` returns and clamps them at the outermost loop, allowing iteration to continue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Merge branch 'main' into alex/if-clause Add GitHub PR comment posting for review-fix-loop final summary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Post self-review result as PR comment in review-fix-loop skill Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: julesmcrt <110237980+julesmcrt@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: astuyve <astuyve@gmail.com> Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Implements the
grepbuiltin command as a newgreppackage. This command searches for pattern matches in files and stdin, supporting BRE, ERE, and fixed-string modes with 20 flags including context lines (-A/-B/-C).Uses Go's
regexppackage (RE2 engine) which guarantees linear-time matching, preventing ReDoS attacks. All file access goes through the existingcallCtx.OpenFilesandbox.Motivation
grepis a core text-processing utility that AI agents will use frequently for searching file contents, filtering pipeline output, and extracting matching lines. It is essential for common patterns likegrep -r pattern,cat file | grep error, and conditional checks likegrep -q pattern file && echo found.Testing
grep_test.go)builtin_grep_pentest_test.go)basic/,flags/,context/,errors/, andhardening/categoriestests/scenarios/cmd/unknown_cmd/common_progs/grep.yamlthat expected grep to be "not found"go test ./interp/... ./tests/...go vetcleanChecklist
SHELL_FEATURES.md,README.md)