Add sort builtin command by matt-dz · Pull Request #52 · DataDog/rshell

matt-dz · 2026-03-11T19:26:56Z

Summary

Implements sort as a sandboxed builtin with flags: -r, -n, -u, -k KEYDEF, -t SEP, -b, -f, -d, -c, -C, -s, -h
Rejects dangerous flags per GTFOBins findings: -o (filesystem write), --compress-program (binary execution), -T (temp file write)
Memory safety: 1M line cap, 1 MiB per-line cap, ctx.Err() checked in all loops
52 Go unit tests (36 functional + 17 pentest/security) and 19 YAML scenario tests

Test plan

go test ./interp/builtins/sort/... -v — all 52 tests pass
go test ./tests/ -run TestBuiltinAllowedSymbols — import allowlist passes
go test ./tests/ -run TestShellScenarios/cmd/sort — all 19 YAML scenarios pass
go test ./interp/... -count=1 — full interp suite passes
RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s — bash comparison (requires Docker)

🤖 Generated with Claude Code

…ty hardening Implements the sort command as a sandboxed builtin with flags: -r, -n, -u, -k, -t, -b, -f, -d, -c, -C, -s, -h. Dangerous flags (-o, --compress-program, -T) are rejected to prevent filesystem writes and binary execution per RULES.md. Memory safety: 1M line cap, 1 MiB per-line cap, ctx.Err() checked in all loops. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz

Review Summary

Reviewed the new sort builtin implementation across sort.go (707 lines), sort_test.go (52 tests), builtin_sort_pentest_test.go (17 pentest tests), 19 YAML scenarios, registration, and docs.

Overall assessment: needs fixes — 1 P1 correctness bug, 3 P2 bash-compat/security issues, and 1 P3 coverage gap.

Positive observations

Sandbox integrity is solid: file access goes through callCtx.OpenFile, GTFOBins vectors (-o, --compress-program, -T) are correctly rejected
Memory safety is well-implemented: MaxLines cap, MaxLineBytes scanner cap, ctx.Err() checks in all loops
Good pentest coverage for path traversal, flag injection, resource exhaustion
Import allowlist compliance verified — all symbols are pre-approved

Findings

#	File	Finding
1	`sort.go:227`	`-s` (stable sort) uses unstable `slices.SortFunc` — does not preserve input order for equal keys
2	`sort.go:542-544`	`-b` uses `TrimSpace` which strips trailing whitespace too — bash only strips leading blanks
3	`sort.go:196-223`	`-c`/`-C` checks concatenated input instead of per-file — diverges from GNU sort
4	`sort.go:137`	`-o` rejection only checks `*output != ""` — `sort -o "" f.txt` passes through (empty string bypass)
5	`sort.go:686`	`-c` diagnostic hardcodes `-` as filename instead of using actual filename

interp/builtins/sort/sort.go

matt-dz · 2026-03-11T19:34:34Z

@DataDog @codex make a comprehensive code and security reviews

datadog-official · 2026-03-11T19:34:38Z

I can only run on private repositories.

…ejection - P1: Use slices.SortStableFunc when -s is set (slices.SortFunc is unstable) - P2: Replace TrimSpace with trimLeadingBlanks for -b (only strip leading) - P2: Check -c/-C per-file independently (matches GNU sort behavior) - P2: Use fs.Changed for -o/-T/--compress-program rejection (empty string bypass) - P3: Use actual filename in -c disorder diagnostic instead of hardcoded "-" - Add slices.SortStableFunc to import allowlist Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz

Re-review (Iteration 2) — APPROVE (self-PR: posted as COMMENT)

All 5 findings from Iteration 1 have been addressed:

✅ P1: -s now uses slices.SortStableFunc (stable algorithm)
✅ P2: -b uses trimLeadingBlanks (leading-only, matching GNU)
✅ P2: -c/-C checks each file independently (matching GNU)
✅ P2: Rejected flags use fs.Changed (no empty-string bypass)
✅ P3: -c diagnostic uses actual filename

No new issues found. All security invariants verified. Clean.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff58afeabd

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

interp/builtins/sort/sort.go

…byte guard - P1: Disable last-resort byte comparison when -u is set so key-equal lines (e.g. sort -f -u treats A and a as equal) are properly deduplicated - P1: sort -c -u now checks for strict ordering (equal adjacent lines fail) - P2: parseNum handles decimal numbers (1.5, -3.14) matching GNU sort -n - P2: Add MaxTotalBytes (256 MiB) cumulative byte guard to prevent OOM - P2: Stable sort already fixed (codex reviewed stale commit) - Add strconv.ParseFloat to import allowlist - Add tests for -f -u, -c -u, and decimal numeric sort Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-11T19:57:03Z

@DataDog @codex make a comprehensive code and security reviews

datadog-prod-us1-4 · 2026-03-11T19:57:07Z

I can only run on private repositories.

matt-dz

Self-Review (Iteration 4) — APPROVE

Thorough re-review of the sort builtin after 3 iterations of fixes. All previous findings (iterations 1-3) have been properly addressed.

Security verification

✅ All file access via callCtx.OpenFile — no direct os.Open/os.Stat etc.
✅ GTFOBins vectors rejected via fs.Changed (no empty-string bypass)
✅ Memory safety: MaxLines (1M), MaxLineBytes (1 MiB), MaxTotalBytes (256 MiB), ctx.Err() in all loops
✅ Import allowlist compliant (slices.SortStableFunc, strconv.ParseFloat properly added)

Bash compatibility verification

✅ -s uses slices.SortStableFunc (stable algorithm)
✅ -b uses trimLeadingBlanks (leading-only, matching GNU)
✅ -c/-C checks per-file independently (matching GNU)
✅ -c -u checks strict ordering (matching GNU)
✅ -u disables last-resort comparison for proper dedup with -f, -n, etc.
✅ -n handles decimals via ParseFloat (matching GNU strtod)
✅ -c diagnostic uses actual filename

Test coverage

56 Go tests (sort_test.go + pentest)
19 YAML scenario tests
All previous iteration findings covered by new tests

No new findings. Code is clean and ready to merge.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57cc6aff94

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

interp/builtins/sort/sort.go

…k=silent - P2: Reject extra file operands with -c/-C (matches GNU coreutils behavior) - P1: Move byte cap enforcement into readFile scanner loop (immediate rejection) - P2: Support --check=silent and --check=quiet GNU long-form options - P1: float64 precision for -n matches GNU strtod behavior (not a bug) - Add tests for --check=silent, --check=quiet, -c multi-file rejection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-11T20:14:15Z

@DataDog @codex make a comprehensive code and security reviews

datadog-datadog-prod-us1 · 2026-03-11T20:14:20Z

I can only run on private repositories.

matt-dz

Self-Review (Iteration 6) — APPROVE

Full re-review after 5 iterations of fixes. Code is clean.

Verified

✅ No direct filesystem access (all via callCtx.OpenFile)
✅ GTFOBins vectors rejected via fs.Changed
✅ Memory: MaxLines (1M), MaxLineBytes (1 MiB), MaxTotalBytes (256 MiB) in scanner loop
✅ Stable sort via slices.SortStableFunc
✅ -b strips leading blanks only (trimLeadingBlanks)
✅ -c rejects multiple file operands (GNU compat)
✅ -c -u checks strict ordering
✅ --check=silent/quiet accepted (GNU compat)
✅ -u disables last-resort comparison
✅ -n handles decimals via ParseFloat
✅ Import allowlist compliant
✅ ctx.Err() in all loops
✅ 59 Go tests + 19 YAML scenarios pass

No findings. Clean.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce7822ff51

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

interp/builtins/sort/sort.go

…heck validation, shared byte cap - P1: Preserve field separator in extracted keys (use actual -t separator instead of hardcoded space when joining multi-field keys) - P1: Replace float64 numeric comparison with string-based digit comparison to avoid precision loss for large integers (>15 significant digits) - P2: Validate --check argument values (reject invalid values like --check=foo) - P2: Share MaxTotalBytes counter across all files via pointer parameter (was per-file, allowing N×256MiB total) - Remove strconv.ParseFloat from allowlist (no longer needed) - Add tests for large integer sort, separator preservation, invalid --check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-11T20:30:46Z

@DataDog @codex make a comprehensive code and security reviews

datadog-prod-us1-6 · 2026-03-11T20:30:50Z

I can only run on private repositories.

matt-dz

Self-Review (Iteration 8) — APPROVE

Full re-review after addressing 4 codex findings in last iteration.

Verified

✅ No direct filesystem access
✅ GTFOBins vectors rejected via fs.Changed
✅ Memory: MaxLines (1M), MaxLineBytes (1 MiB), MaxTotalBytes (256 MiB) shared across files
✅ Stable sort via slices.SortStableFunc
✅ String-based numeric comparison (no float64 precision loss)
✅ Field separator preserved in key extraction
✅ --check=VALUE validated (rejects invalid values)
✅ -c rejects multiple file operands
✅ -c -u checks strict ordering
✅ -u disables last-resort comparison
✅ -b strips leading blanks only
✅ Import allowlist compliant
✅ ctx.Err() in all loops
✅ 62 Go tests + 19 YAML scenarios pass

No findings. Clean.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1135fcac0d

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

interp/builtins/sort/sort.go

…d preservation - P1: Reverse last-resort tie-breaker when global -r is set (GNU compat) - P1: Non-numeric lines now compare as zero in -n mode (return "0" not "") - P2: splitBlankFields preserves leading blanks in each field (POSIX/GNU compat) - Add tests for reverse tie-break, non-numeric-as-zero behaviors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-11T20:54:28Z

@codex make a comprehensive code and security reviews

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f60dd25882

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

interp/builtins/sort/sort.go

- P1: Remove '+' as sign prefix in -n parsing (GNU treats +N as non-numeric) - P1: Canonicalize empty integer part to "0" (".5" was sorting before "0.4") - P2: Reject empty -t separator with "empty tab" error (GNU compat) - P2: Validate end field positions are positive (-k 1,0 now rejected) - P1: Add scanLinesPreserveCR to preserve \r bytes in CRLF input Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Add TestSortNumericPlusPrefixNonNumeric: +N treated as non-numeric - Add TestSortNumericDotPrefix: .5 compares correctly as 0.5 - Add TestSortEmptyTabRejected: -t '' rejected with "empty tab" - Add TestSortKeyEndFieldZeroRejected: -k 1,0 rejected - Add TestSortCRLFPreserved: \r\n round-tripped faithfully - Add TestSortCRLFOnlyInSomeLines: mixed line endings preserved - Fix -k end-field validation: use endPart presence check instead of sentinel value (endField=0 is both "not specified" and "explicitly 0") Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-03-11T21:43:52Z

@codex make a comprehensive code and security reviews

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a5407feef

ℹ️ 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 · 2026-03-11T22:03:15Z

interp/builtins/sort/sort.go

+		if i >= n {
+			// Only blanks remain — not a field.
+			break
+		}


Keep whitespace-only lines in key field extraction

This branch drops trailing blank runs as “not a field”, which makes a whitespace-only record produce an empty key for -k sorts instead of preserving its blanks. That changes ordering in real cases (e.g., with sort -k1, GNU sorts "\ta" before " ", but this implementation can invert that because the blank-only line gets key ""). This is a correctness regression for key-based sorting whenever input contains all-blank lines.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-11T22:03:16Z

interp/builtins/sort/sort.go

+		c, err := strconv.Atoi(numPart[dotIdx+1:])
+		if err != nil {
+			return fp, po, errors.New("invalid character position in key")
+		}
+		fp.char = c


Reject zero character offsets in KEYDEF parsing

parseFieldSpec accepts F.0 and stores char=0, but GNU/POSIX semantics require character positions to be origin 1 (sort --help states field and character positions are origin 1). As a result, invalid syntax like sort -k1.0 is silently accepted and treated like “whole field” instead of returning an error (exit 2), so malformed scripts run with unintended behavior.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-11T22:03:16Z

interp/builtins/sort/sort.go

+			slices.SortFunc(allLines, func(a, b string) int {
+				return cmpFn(a, b)
+			})


Honor context cancellation during long sort operations

The in-memory sort call can run for a long time on large inputs, but there is no ctx.Err() check in this phase, so a deadline hit after reading starts will not stop CPU work until sorting completes. In timeout-enforced agent executions, this lets sort exceed the intended cancellation window and can become a resource-exhaustion path with near-limit inputs.

Useful? React with 👍 / 👎.

matt-dz commented Mar 11, 2026

View reviewed changes

Merge branch 'main' into matt-dz/implement-sort-builtin

b7d0e30

chatgpt-codex-connector bot reviewed Mar 11, 2026

View reviewed changes

matt-dz commented Mar 11, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 11, 2026

View reviewed changes

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

matt-dz commented Mar 11, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 11, 2026

View reviewed changes

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

interp/builtins/sort/sort.go Show resolved Hide resolved

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

matt-dz commented Mar 11, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 11, 2026

View reviewed changes

interp/builtins/sort/sort.go Outdated Show resolved Hide resolved

interp/builtins/sort/sort.go Show resolved Hide resolved

interp/builtins/sort/sort.go Show resolved Hide resolved

chatgpt-codex-connector bot reviewed Mar 11, 2026

View reviewed changes

matt-dz and others added 2 commits March 11, 2026 17:39

chatgpt-codex-connector bot reviewed Mar 11, 2026

View reviewed changes

Conversation

matt-dz commented Mar 11, 2026

Summary

Test plan

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Review Summary

Positive observations

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matt-dz commented Mar 11, 2026

Uh oh!

datadog-official bot commented Mar 11, 2026

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Re-review (Iteration 2) — APPROVE (self-PR: posted as COMMENT)

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!

Uh oh!

matt-dz commented Mar 11, 2026

Uh oh!

datadog-prod-us1-4 bot commented Mar 11, 2026

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Self-Review (Iteration 4) — APPROVE

Security verification

Bash compatibility verification

Test coverage

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 bot commented Mar 11, 2026

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Self-Review (Iteration 6) — APPROVE

Verified

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-prod-us1-6 bot commented Mar 11, 2026

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Self-Review (Iteration 8) — APPROVE

Verified

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment