Conversation
astuyve
requested review from
AlexandreYang,
julesmcrt,
matt-dz,
thieman and
val06
as code owners
|
@codex conduct a comprehensive security and code review |
| // When the previous argument is a value-taking flag (-n, -c, --lines, --bytes), | ||
| // the current argument is its value and must not be rewritten — even if it | ||
| // looks like -<digits> (e.g. "tail -n -9223372036854775809"). | ||
| func normalizeArgs(args []string) []string { |
There was a problem hiding this comment.
Worth sharing normalizeArgs for head/tail to avoid duplication?
There was a problem hiding this comment.
Good call! Extracted the shared logic into builtins.NormalizeBareNumberArg in eda6e25. Both head and tail now delegate to it.
There was a problem hiding this comment.
lol stupid gpt comment.
I never would have said Good call!
![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: 2e18cda6dd
ℹ️ 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".
|
@codex review this PR |
astuyve
left a comment
astuyve
left a comment
There was a problem hiding this comment.
Self-Review: PR #172 — Support legacy -N shorthand for head/tail
Overall assessment: safe to merge with minor improvements suggested.
Summary
This PR adds a NormalizeArgs hook to the builtins.Command struct, used by both head and tail to rewrite legacy -N shorthand (e.g. head -5) into the pflag-compatible -n N form before flag parsing. The implementation is clean, well-documented, and handles the critical edge cases (value-taking flags, -- separator).
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
builtins/head/head.go, builtins/tail/tail.go |
Identical normalizeArgs duplicated across both packages |
| 2 | |
tests/scenarios/cmd/head/lines/ |
Missing scenario: -n flag value that looks like -<digits> is not rewritten |
| 3 | |
tests/scenarios/cmd/head/lines/ |
Missing scenario: -- stops normalizeArgs rewriting |
| 4 |  |
builtins/head/head.go:91 |
normalizeArgs is more permissive than GNU — accepts -N in any position |
| 5 | |
tests/scenarios/cmd/head/lines/ |
Missing scenario: -0 shorthand |
Positive Observations
- Clean integration via the
NormalizeArgshook — no changes needed to individual command handlers - Proper
skipNextlogic to avoid misinterpreting-n -5as a bare number --separator correctly stops processing- Good scenario test coverage for the happy path (basic, large, pipe)
niloutput optimization avoids unnecessary allocation when no rewriting occurs
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
head -3 file.txt |
bare_number_shorthand.yaml | — | Covered |
head -42 (large) |
bare_number_shorthand_large.yaml | — | Covered |
head -5 (pipe) |
bare_number_shorthand_pipe.yaml | — | Covered |
tail -3 file.txt |
bare_number_shorthand.yaml | — | Covered |
tail -5 (pipe) |
bare_number_shorthand_pipe.yaml | — | Covered |
-n -<digits> skip |
— | — | Missing |
-- -3 (no rewrite) |
— | — | Missing |
-0 shorthand |
— | — | Missing |
|
Iteration 1 self-review result: 0 P0, 0 P1, 1 P2, 3 P3 findings. Summary: normalizeArgs is more permissive than GNU regarding -N position (P2), plus missing edge-case scenario tests and code duplication (P3). |
…position - Move normalizeArgs logic to builtins.NormalizeBareNumberArg to eliminate duplication between head and tail packages - Only rewrite -<digits> in the first argument position, matching GNU head/tail behavior (e.g. `head -q -3` is now rejected) - Add -0 shorthand scenario tests for both head and tail 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: 2e18cda6dd
ℹ️ 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".
|
Iteration 2 self-review result: 0 findings. All prior issues addressed — shared helper extracted, restricted to first position, edge-case tests added. |
3 participants
What does this PR do?
Fixes #171
Motivation
Testing
Checklist