Conversation
…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
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
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
| # | Priority | 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 |
|
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
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Re-review (Iteration 2) — APPROVE (self-PR: posted as COMMENT)
All 5 findings from Iteration 1 have been addressed:
- ✅ P1:
-snow usesslices.SortStableFunc(stable algorithm) - ✅ P2:
-busestrimLeadingBlanks(leading-only, matching GNU) - ✅ P2:
-c/-Cchecks each file independently (matching GNU) - ✅ P2: Rejected flags use
fs.Changed(no empty-string bypass) - ✅ P3:
-cdiagnostic uses actual filename
No new issues found. All security invariants verified. Clean.
![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: 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".
…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>
|
I can only run on private repositories. |
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
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 directos.Open/os.Statetc. - ✅ 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.ParseFloatproperly added)
Bash compatibility verification
- ✅
-susesslices.SortStableFunc(stable algorithm) - ✅
-busestrimLeadingBlanks(leading-only, matching GNU) - ✅
-c/-Cchecks per-file independently (matching GNU) - ✅
-c -uchecks strict ordering (matching GNU) - ✅
-udisables last-resort comparison for proper dedup with-f,-n, etc. - ✅
-nhandles decimals viaParseFloat(matching GNUstrtod) - ✅
-cdiagnostic 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.
There was a problem hiding this comment.
💡 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".
…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>
|
I can only run on private repositories. |
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 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".
…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>
|
I can only run on private repositories. |
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 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".
…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>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 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".
- 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>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 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".
GNU sort -n only skips leading blanks (space/tab) before reading the numeric prefix. strings.TrimSpace strips all Unicode whitespace including \v, \f, \r which GNU sort treats as non-numeric. This caused inputs like "\v2" to sort as 2 instead of 0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regression test for the TrimSpace → blank-only skip fix: ensures \v prefix is treated as non-numeric by sort -n, matching GNU sort. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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,
astuyve,
julesmcrt and
thieman
as code owners
- check_empty_value_reject: verifies sort --check= (empty value) returns exit 2 with "invalid argument" error - check_read_error_exit2: verifies sort -c returns exit 2 (not 1) for file read errors, since exit 1 is reserved for disorder Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests that checkSorted: - Returns exit 1 when context is cancelled (3000 lines, fires at 1024th iteration check) - Completes normally for sorted input without cancellation - Detects disorder before the cancellation check fires Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The field was incorrectly nested under expect:, causing the bash comparison test to run these scenarios and fail on intentional divergences (error message format, exit code differences). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Effective Go Review — sort builtin
Overall: has findings — the implementation is solid and well-structured. Good use of strings.Builder for string concatenation, proper context cancellation threading, and thorough error handling. A few non-idiomatic patterns to clean up.
| # | Priority | File | Finding | Rule |
|---|---|---|---|---|
| 1 | P2 | sort.go:527 |
errors.New(fmt.Sprintf(...)) → fmt.Errorf |
Effective Go: Errors |
| 2 | P3 | sort.go:693-775 |
extractKeyFromFields is dead code |
#16 Linters / dead code |
| 3 | P3 | sort.go:927 |
Repeated string reslicing in blank-skip loop | #39 String building |
Positive Observations
- Excellent doc comment on the package with full flag reference and memory safety guarantees
- Proper use of
strings.BuilderwithGrow()hints infoldCaseanddictFilter - Context cancellation is thoroughly threaded through all long-running loops
checkTrackercustompflag.Valueis a clean pattern for tracking conflicting flag states- Good separation of concerns: parsing (
parseKeyDef/parseFieldSpec), extraction (extractKey), comparison (compareStrings/compareNumeric), and orchestration (buildCompare)
- Remove extractKeyFromFields (lines 690-775) which was replaced by extractKey and had zero callers. - Replace per-byte string reslicing in parseNumParts blank-skip loop with an index-based approach for idiomatic Go. - Revert fmt.Errorf back to errors.New(fmt.Sprintf) as fmt.Errorf is not in the project's builtin symbol allowlist. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add fmt.Errorf to the builtin symbol allowlist — it's a pure function with no I/O, same as fmt.Sprintf. Replace the redundant errors.New(fmt.Sprintf(...)) pattern in parseFieldSpec. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402e7eeda5
ℹ️ 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".
- Thread context cancellation into dedup's comparison loop (every 1024 iterations) so sort -u respects timeouts during the dedup phase on large inputs. - Add +1 to scanner buffer cap so lines of exactly MaxLineBytes (1 MiB) are accepted. bufio.Scanner's cap includes the delimiter byte, so MaxLineBytes alone rejects boundary-sized lines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b85def43ff
ℹ️ 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 upfront ctx.Err() check before entering the checkSorted loop so that a pre-cancelled context is detected even for inputs under the 1024-iteration periodic check threshold. Added test for the small-input cancellation case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In the context of this shell, sort processes at most a few hundred lines — the i&1023 amortization added complexity for no practical benefit. Now sortCmp, checkSorted, and dedup all check ctx.Err() on every iteration for immediate cancellation response. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tested The runner's shouldStop method catches context cancellation before dispatching to builtins, so pre-cancelled context tests through the shell runner always return exit 0. Builtin-level cancellation (checkSorted, sortCmp, dedup) is properly tested via direct unit tests in cancellation_test.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
sortas a sandboxed builtin with flags:-r,-n,-u,-k KEYDEF,-t SEP,-b,-f,-d,-c,-C,-s,-h-o(filesystem write),--compress-program(binary execution),-T(temp file write)ctx.Err()checked in all loopsTest plan
go test ./interp/builtins/sort/... -v— all 52 tests passgo test ./tests/ -run TestBuiltinAllowedSymbols— import allowlist passesgo test ./tests/ -run TestShellScenarios/cmd/sort— all 19 YAML scenarios passgo test ./interp/... -count=1— full interp suite passesRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s— bash comparison (requires Docker)🤖 Generated with Claude Code