Skip to content

Implement strings builtin command#49

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 2 commits intomainfrom
jules.macret/builtins/strings
Mar 11, 2026
Merged

Implement strings builtin command#49
gh-worker-dd-mergequeue-cf854d[bot] merged 2 commits intomainfrom
jules.macret/builtins/strings

Conversation

@julesmcrt
Copy link
Copy Markdown
Collaborator

@julesmcrt julesmcrt commented Mar 11, 2026

Summary

  • Implements the strings builtin command (GNU binutils strings equivalent) as a sandboxed builtin, enabling AI agents to extract printable sequences from binary files without executing host binaries.
  • Registers strings in interp/register_builtins.go and documents it in SHELL_FEATURES.md.
  • Adds 27 YAML scenario tests and a pentest test suite (strings_pentest_test.go).

Flags implemented

Flag Description
-a / --all Scan entire file (default; accepted for POSIX compat)
-n MIN / --bytes=MIN Minimum sequence length (default 4; ≥ 1)
-t o|d|x / --radix= Print byte offset before each string in octal/decimal/hex (7-char right-justified field matching GNU strings)
-o Legacy alias for -t o
-f / --print-file-name Prefix each string with its source filename
-s SEP / --output-separator=SEP Custom separator between strings (default \n)
-h / --help Print usage to stdout

Algorithm

Reads in 32 KiB chunks. A byte is printable if 0x20 ≤ b ≤ 0x7e or b == '\t'. Sequences shorter than the minimum length are discarded. Individual strings are capped at 1 MiB to prevent memory exhaustion. Context cancellation is checked at every read boundary.

Test plan

  • 27 YAML scenario tests covering: basic output, min-length, offsets (o/d/x), -o legacy flag, filename prefix, -f -t combination, custom separator, long-form flags (--bytes, --output-separator), stdin, nil stdin, and all error paths (missing file, directory, invalid radix, invalid min-len, partial error).
  • strings_pentest_test.go with 5 Go tests:
    • TestStringsCap1MiB — verifies the 1 MiB cap is enforced exactly
    • TestStringsCapOffsetAfterCap — verifies offset tracking is correct after a capped sequence
    • TestStringsAllNullBytes — verifies no hang / no output on all-null input
    • TestStringsAllPrintable — verifies correct EOF flush for all-printable files
    • TestStringsLargeOffsetDecimal — verifies offsets > 7 digits are not truncated

All scenarios use skip_assert_against_bash: true because strings (GNU binutils) is not available in the debian:bookworm-slim Docker image used for bash comparison tests.

🤖 Generated with Claude Code

@julesmcrt julesmcrt changed the title Add builtins strings Implement strings builtin command Mar 11, 2026
@julesmcrt
Copy link
Copy Markdown
Collaborator Author

@codex review

Copy link
Copy Markdown

@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: 24d5209b1d

ℹ️ 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 thread interp/builtins/strings_cmd/strings.go Outdated
Comment on lines +119 to +123
if *offsetLegacy {
format = radixOctal
}
if *radix != "" {
switch *radix {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Respect last radix-related flag in option order

GNU strings --help defines -o as an alias for --radix=o, so mixed usage should behave as if each option were processed in sequence (e.g. -t d -o should end in octal, while -o -t d should end in decimal). This implementation hard-codes --radix to always override -o, which makes strings -t d -o file produce decimal offsets instead of octal and breaks compatibility for scripts that rely on flag order.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Sonnet 4.6] Fixed. Replaced the StringP/BoolP pair with custom pflag.Value types (radixFlagVal, octalFlagVal) that share the same *radixFormat pointer. pflag calls Set() in left-to-right parse order, so whichever of -o/-t appears last naturally wins — matching GNU strings semantics where -o is just an alias for -t o. Added test scenarios offset/flag_order_t_then_o and offset/flag_order_o_then_t to verify both orderings.

Comment thread interp/builtins/strings_cmd/strings.go Outdated
if *offsetLegacy {
format = radixOctal
}
if *radix != "" {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject empty radix values instead of treating them as unset

--radix is documented as --radix={o,d,x}, but this guard skips validation when the parsed value is empty, so invocations like strings --radix= file (or -t '') are treated as if no radix flag was passed. That silently accepts invalid CLI input instead of returning an error, which can mask caller mistakes and diverges from standard strings option validation.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Sonnet 4.6] Fixed. Moved radix validation into radixFlagVal.Set(), which returns an error for any value not in {"o", "d", "x"} — including the empty string. pflag then reports the error before the handler runs, so --radix= / -t '' are now rejected with invalid argument "" for "-t, --radix" flag: invalid radix. Added errors/empty_radix scenario to cover this case.

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>
@julesmcrt julesmcrt marked this pull request as ready for review March 11, 2026 15:14
Copy link
Copy Markdown
Collaborator

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

Security Review Summary

# Priority File Finding
No security issues found

Scope: Reviewed 4 changed source files (+ 18 test scenario YAMLs) from PR #49.

Verdict: Clean implementation with strong security posture.

Detailed Assessment

File access control — All file I/O is correctly delegated to callCtx.OpenFile, which enforces the shell's allowed-paths sandbox. No direct os.Open or os.ReadFile calls that could bypass path restrictions. Files are opened read-only (os.O_RDONLY).

Memory safety — Input is read in bounded 32 KiB chunks. Individual strings are capped at 1 MiB (maxStringLen) to prevent memory exhaustion from arbitrarily long printable runs. The cap is correctly enforced: bytes beyond the limit are silently dropped while scanning continues.

Cancellationctx.Err() is checked at every read loop iteration, ensuring the command respects the shell's execution timeout and supports graceful cancellation.

Input validation — The -n/--bytes flag is validated to be in range [1, maxInt32]. The -t/--radix flag is validated against an explicit allowlist (o, d, x). Invalid values produce clear error messages and exit code 1.

No injection vectors — No shell execution, no exec calls, no format-string vulnerabilities (all Outf/Errf calls use static format strings with %s/%d verbs). The custom separator (-s) is written as raw output with no interpretation.

Offset tracking correctnessfileOffset is int64, safe for files up to 2⁶³ bytes. Offset calculation across chunk boundaries is correct. The offset reported for capped strings correctly reflects the start of the original sequence.

Test coverage — Good security-relevant test coverage including: 1 MiB cap enforcement, offset correctness after cap, all-null input (no hangs), large offsets (no truncation), missing files, invalid flags, and partial errors with multiple files.

Copy link
Copy Markdown
Collaborator

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

Security Review — Approved

No security issues found. Clean implementation with sandboxed file access, bounded memory (1 MiB cap per string, 32 KiB read chunks), proper context cancellation, and thorough input validation.

Approving from a security perspective. ✅

@gh-worker-dd-mergequeue-cf854d gh-worker-dd-mergequeue-cf854d Bot merged commit 70c5996 into main Mar 11, 2026
9 checks passed
@gh-worker-dd-mergequeue-cf854d gh-worker-dd-mergequeue-cf854d Bot deleted the jules.macret/builtins/strings branch March 11, 2026 19:33
AlexandreYang added a commit that referenced this pull request Mar 11, 2026
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>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants