Conversation
Add a safe printf builtin supporting all standard format specifiers (%s, %b, %c, %d, %i, %o, %u, %x, %X, %e, %E, %f, %F, %g, %G, %%), escape sequences, width/precision modifiers (including * from args), and format string reuse for excess arguments. Safety measures: %n rejected (security risk), -v rejected, format reuse bounded to 10,000 iterations with context cancellation checks, width/precision clamped to ±10,000 to prevent memory exhaustion. Includes 80+ Go unit tests, 25 GNU compatibility tests, 41 pentest tests, and 46 YAML scenario tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AlexandreYang
requested review from
astuyve,
julesmcrt,
matt-dz and
thieman
as code owners
![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: f34fd54c64
ℹ️ 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 40 YAML scenario tests and additional Go tests covering:
- Unsigned negative wrapping (%u, %o, %x with -1)
- Double-quote character constants ("A)
- Star (*) width/precision: zero, negative, empty, clamping, invalid
- Octal/hex escape boundary conditions and truncation
- %F uppercase for infinity/NaN
- %b with multiple escapes, \c stopping format reuse
- Conflicting flags (- vs 0, + vs space)
- Incomplete specifiers with flags/width
- Invalid octal digits (8, 9)
- Format reuse iteration limit and context cancellation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review: Implement printf builtin command
Overall Assessment: Needs fixes (submitted as COMMENT because reviewer is PR author — would be REQUEST_CHANGES otherwise)
The printf implementation is well-structured, thoroughly tested, and security-hardened. The %n rejection, width/precision clamping, iteration bounding, and context cancellation are all excellent safety measures. The import allowlist additions are correct and properly justified.
However, there are several bash compatibility bugs that will cause the TestShellScenariosAgainstBash suite to fail, and missing skip_assert_against_bash flags on intentionally-divergent scenarios.
Findings Summary
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
interp/builtins/printf/printf.go:133 |
printf with no args returns exit 1 instead of bash's exit 2 |
| 2 | |
interp/builtins/printf/printf.go:340-343 |
Incomplete format specifiers (%, %-, %10) silently succeed instead of erroring (exit 1) |
| 3 | |
interp/builtins/printf/printf.go:548-551 |
Unknown format specifiers (%z) silently succeed instead of erroring (exit 1) |
| 4 |  |
tests/scenarios/cmd/printf/errors/rejected_n_specifier.yaml |
Missing skip_assert_against_bash: true — bash accepts %n |
| 5 | |
tests/scenarios/cmd/printf/basic/no_args.yaml |
Missing skip_assert_against_bash: true — bash returns exit 2, not 1 |
| 6 | |
tests/scenarios/cmd/printf/errors/no_format.yaml |
Duplicate of basic/no_args.yaml; also missing bash skip flag |
| 7 | |
tests/scenarios/cmd/printf/specifiers/incomplete_with_flags.yaml |
Missing skip_assert_against_bash: true — bash errors on incomplete specifiers |
| 8 |  |
interp/builtins/printf/printf.go:217 |
Byte-at-a-time output for literal characters is inefficient |
skip_assert_against_bash Audit
| Scenario file | Bash matches? | Flag needed? | Status |
|---|---|---|---|
shell_features/command_substitution.yaml |
No (shell blocks cmd substitution) | Yes | OK |
escapes/octal_single_digit.yaml |
Yes (both produce 0x01) | Unclear | Verify YAML byte encoding |
escapes/hex_single_digit.yaml |
Yes (both produce 0x0f) | Unclear | Verify YAML byte encoding |
errors/rejected_v_flag.yaml |
No (bash accepts -v) | Yes | OK |
specifiers/float_f_upper_nan.yaml |
Yes (bash outputs NAN) | No | Remove flag |
specifiers/b_octal_no_digits.yaml |
Yes (both produce NUL + x) | Unclear | Verify YAML byte encoding |
specifiers/b_hex_one_digit.yaml |
Yes (both produce 0x0f) | Unclear | Verify YAML byte encoding |
Coverage Summary
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Basic format specifiers (%s, %d, etc.) | 10+ scenarios | 20+ tests | Adequate |
| Width/precision modifiers | 14 scenarios | 8 tests | Adequate |
| Escape sequences | 12 scenarios | 12 tests | Adequate |
| %b specifier with escapes | 6 scenarios | 10 tests | Adequate |
| Error handling (invalid numbers) | 6 scenarios | 12 tests | Adequate |
| Security: %n rejection | 1 scenario | 2 tests | Adequate |
| Security: -v rejection | 1 scenario | 2 tests | Adequate |
| Security: width/precision clamping | — | 4 tests | Adequate |
| Security: iteration limit | — | 1 test | Adequate |
| Security: context cancellation | — | 2 tests | Adequate |
| Pentest: integer edge cases | — | 12 tests | Adequate |
| Pentest: flag injection | — | 6 tests | Adequate |
| Incomplete specifier error path | 2 scenarios (wrong expect) | 1 test (wrong expect) | Needs fix |
| Unknown specifier error path | — | 1 test (wrong expect) | Needs fix |
Positive Observations
- Excellent security hardening:
%nrejection,-vrejection, width/precision clamping to 10,000, iteration cap at 10,000, context cancellation - Comprehensive test pyramid: 80+ unit tests, 25 GNU compat tests, 41 pentest tests, 46 YAML scenario tests
- Clean import allowlist: only
context,fmt,math,strconv,strings— all safe, pure functions NoFlagsmode is correct for printf since args can look like flagsprocessBEscapesproperly handles\cto stop all output- Good boundary testing: MaxInt64+1 overflow, huge widths, many-arg iteration limits
- No filesystem access, no stdin reading — purely computes output from args
tests/scenarios/cmd/printf/specifiers/incomplete_with_flags.yaml
Outdated
Show resolved
Hide resolved
|
I can only run on private repositories. |
|
Iteration 1 self-review result: COMMENT
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c6e39e4ec
ℹ️ 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".
- Exit code 2 for missing format (matches bash behavior) - Incomplete format specifiers now produce error + exit 1 (matches bash) - Unknown format specifiers now produce error + exit 1 (matches bash) - Negative * precision treated as omitted (matches bash) - Float NaN/Inf output lowercased for lowercase verbs (matches bash) - Batch consecutive literal characters into single write (optimization) - Add skip_assert_against_bash for %n rejection (intentional divergence) - Remove duplicate errors/no_format.yaml (identical to basic/no_args.yaml) - Fix scenarios using -- between format and args (not bash-compatible) - Update Go tests to match new bash-compatible behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review: printf builtin (PR #57)
Overview
New printf builtin command implementation with comprehensive test coverage (Go unit tests, GNU compat tests, pentest tests, and 80+ YAML scenario tests). The implementation covers format specifiers (%s, %b, %c, %d, %i, %o, %u, %x, %X, %e, %E, %f, %F, %g, %G), width/precision modifiers, escape sequences, format reuse, and several security hardening measures.
Overall Assessment: Needs Fixes
The implementation is well-structured with good security posture (bounded loops, width clamping, %n rejection, no file/stdin access). However, there are bash compatibility bugs and a CI-blocking issue that need to be addressed before merging.
Summary of Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | |
interp/builtins/printf/printf.go:153 |
-- after format string is wrongly stripped (bash compat bug) |
| 2 | |
interp/builtins/printf/printf.go:118 |
strings.ReplaceAll not in allowed symbols list — CI will fail |
| 3 | |
interp/builtins/printf/printf.go:133 |
-h is not a valid bash printf flag — should be treated as format string |
| 4 | |
interp/builtins/printf/printf.go:133 |
--help exits 0 but bash exits 2 for its help output |
| 5 | |
interp/builtins/printf/printf.go:410 |
Error message format differs from bash (missing quotes around value) |
Positive Observations
- Excellent security hardening: %n rejected, -v rejected, width/precision clamped to 10,000, format reuse bounded to 10,000 iterations, context cancellation respected
- Comprehensive test coverage across all test types (scenario, functional, GNU compat, pentest)
- No filesystem access, no stdin reading — pure format+args computation
- All new imports are safe pure functions (fmt.Sprintf, strconv, math, strings)
- Proper registration in
register_builtins.goandallowed_symbols_test.go
Test Coverage Summary
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Basic %s, %d, %f formatting | 20+ scenarios | printf_test.go, gnu_compat | Covered |
| Format reuse with excess args | basic/format_reuse.yaml | TestPrintfFormatReuse | Covered |
| Missing args (default 0/"") | basic/missing_arg_*.yaml | TestPrintfMissingArg* | Covered |
| %b escape sequences | specifiers/b_*.yaml | TestPrintfBEscape* | Covered |
| Width/precision modifiers | width_precision/*.yaml | TestPrintfWidth* | Covered |
| Star width/precision (*) | width_precision/star_*.yaml | TestPentestStarWidth* | Covered |
| Numeric input (hex/octal/char) | numeric/*.yaml | TestPrintfNumeric* | Covered |
| Unsigned negative wrapping | numeric/*_negative_wrap.yaml | TestPentestUnsigned* | Covered |
| Integer overflow/huge numbers | — | TestPentestIntMaxInt64PlusOne | Covered |
| %n rejection (security) | errors/rejected_n_specifier.yaml | TestPentestPercentN | Covered |
| -v rejection (security) | errors/rejected_v_flag.yaml | TestPentestVFlag | Covered |
| Width/precision clamping (DoS) | — | TestPentestHugeWidth/Precision | Covered |
| Iteration limit (10K bound) | — | TestPrintfFormatReuseIterationLimit | Covered |
| Context cancellation | — | TestPrintfContextCancellation* | Covered |
-- after format string (bug) |
— | — | Missing |
-h flag behavior vs bash |
— | TestPrintfHelpShort | Incorrect |
|
I can only run on private repositories. |
|
Iteration 1 self-review result: COMMENT (needs fixes)
Total: 5 findings. Proceeding to fix. |
- Remove incorrect `--` stripping after format string (P1): bash treats `--` after the format as a regular argument, not an option terminator - Add `strings.ReplaceAll` to allowed symbols list (P1): needed by bashFloat() for NaN/Inf case normalization - Remove `-h` flag (P2): not a valid bash printf flag; `-h` is now treated as a format string like bash does - Change `--help` to exit code 2 with stderr (P2): matches bash behavior where unrecognized options produce usage errors - Add single quotes around invalid values in error messages (P3): matches bash's `printf: 'abc': invalid number` format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 669df66a39
ℹ️ 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
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review (Iteration 2) — printf builtin
Scope: Full diff of PR #57 (alex/command_printf → main), re-reviewed after 5 fixes from iteration 1.
Overall assessment: Needs minor fixes — two P2 bash compatibility issues with uppercase float format verbs and unknown single-dash flag handling. No P0/P1 security or correctness issues found. The implementation is well-structured with good safety bounds.
Summary of findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | |
interp/builtins/printf/printf.go:478-544 |
Uppercase float verbs (%E, %F, %G) produce wrong Inf/NaN output |
| 2 | |
interp/builtins/printf/printf.go:131-142 |
Unknown single-dash flags treated as format strings instead of rejected |
Positive observations
- Security:
%nproperly rejected,-vblocked,maxFormatIterations(10k) andmaxWidthOrPrec(10k) prevent DoS, context cancellation honored in the loop. No filesystem access, noos/execimports. - Correctness: Format reuse, missing argument defaults, escape processing, octal/hex parsing, unsigned wrapping all match bash behavior.
- Test coverage: Excellent — 85+ scenario YAML tests covering all specifiers, escapes, width/precision, errors, shell integration, plus Go unit tests, GNU compat tests, and pentest tests.
- Code quality: Clean separation of concerns (processFormat → processSpecifier → type-specific handlers), helper functions well-documented.
|
I can only run on private repositories. |
|
Iteration 2 self-review result: COMMENT (needs fixes)
Total: 2 findings. Proceeding to fix. |
… getIntArg Address review comments from iteration 2: - Add bashFloatUpper() helper for %E, %F, %G to produce correct INF/NAN output matching bash (was using strings.ToUpper or raw Go output) - Emit raw bytes for format-string escape values >= 0x80 instead of UTF-8 encoding via string(rune(val)) - Fix %c to apply width/flags formatting even for empty arguments - Fix getIntArg to accept hex (0xff) and octal (077) forms for * width/precision operands, matching bash behavior - Update allowed_symbols_test.go: add strconv.IntSize, remove unused strings.ToUpper - Fix %F scenario tests to use exact stdout match and remove incorrect skip_assert_against_bash flag Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Iteration 2 review fix summary (commit e75142d) Resolved all 14 unresolved review threads. Here is the disposition of each: Fixed (4 findings)
Already fixed in previous iterations (4 findings)
Not valid / declined (4 findings)
Ancillary changes
All 85+ scenario tests and full test suite pass. |
Review-Fix Loop Summary
Iteration log
Final state
Remaining issuesNone — PR is clean and ready for human review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ce4c4b8ec
ℹ️ 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/printf/printf.go
Outdated
| if consumed == 0 { | ||
| return "\\x", 2 | ||
| } | ||
| return string(rune(val)), 2 + consumed |
There was a problem hiding this comment.
processFormatEscape converts octal/hex escapes with string(rune(val)), which UTF-8-encodes values above ASCII instead of emitting a single byte; for example, printf "\xFF" now writes 0xC3 0xBF while bash writes a single 0xFF. This corrupts binary output and breaks byte-for-byte compatibility for any \xNN/octal escape >= 128.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Handle hex integers used as float args (0xff etc). | ||
| if len(s) > 2 && s[0] == '0' && (s[1] == 'x' || s[1] == 'X') { |
There was a problem hiding this comment.
The float parser only recognizes hex integers when the argument starts with 0x/0X, so signed forms like -0xff fall through to strconv.ParseFloat and are treated as invalid (0.000000, exit 1). Bash accepts signed hex integer arguments for %f/%e/%g (e.g., printf '%f\n' -0xff -> -255.000000), so this regresses numeric compatibility.
Useful? React with 👍 / 👎.
| default: | ||
| // Unrecognized: output backslash and character. | ||
| b.WriteByte('\\') | ||
| b.WriteByte(s[i]) |
There was a problem hiding this comment.
In %b escape handling, only \0nnn is parsed as octal, and escapes like \101 are routed to the default branch and emitted literally (\101). Bash interprets both \0NNN and \NNN for %b, so commands such as printf '%b' '\101' should output A but currently do not.
Useful? React with 👍 / 👎.
interp/builtins/printf/printf.go
Outdated
| // Empty argument produces a NUL byte (bash behavior). | ||
| callCtx.Out("\x00") |
There was a problem hiding this comment.
The empty-argument %c path writes a bare NUL byte and skips the constructed format string, so width/alignment flags are lost. In bash, formatting still applies (e.g., printf '%3c' '' outputs two spaces followed by NUL), but this implementation emits only NUL.
Useful? React with 👍 / 👎.
1 participant
Summary
printfbuiltin with all standard format specifiers (%s,%b,%c,%d,%i,%o,%u,%x,%X,%e,%E,%f,%F,%g,%G,%%), escape sequences, width/precision modifiers (including*from args), and format reuse for excess arguments%nrejected (security risk),-vrejected, format reuse bounded to 10,000 iterations with context cancellation, width/precision clamped to ±10,000Test plan
go test ./interp/builtins/printf/...— all unit, GNU compat, and pentest tests passgo test ./tests/...— all YAML scenario tests passgo test ./interp/...— full interp test suite passes (no regressions)RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s— bash comparison🤖 Generated with Claude Code