Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions .claude/skills/review-fix-loop/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ Check **all three** review sources for remaining issues:

1. **Self-review** — Was the latest `/code-review` result **APPROVE** (no findings)?

2. **External reviews** — Are there unresolved PR comment threads from @datadog, @codex, or @chatgpt-codex-connector[bot]?
2. **External reviews** — Are there unresolved PR comment threads from @datadog or @codex?
```bash
gh api graphql -f query='
query($owner: String!, $repo: String!, $pr: Int!) {
Expand Down Expand Up @@ -297,37 +297,32 @@ Run a final verification regardless of how the loop exited:

4. **Confirm @codex has replied to the LATEST review request (with polling):**

The review request comment posted in Step 2A2 triggers @codex asynchronously. The bot may respond as either `codex` or `chatgpt-codex-connector[bot]` (the GitHub App). It can take **15+ minutes** to respond. You must verify that the bot has actually responded to **the most recent** request, not a previous iteration's request. Replies from earlier iterations do NOT count.
The review request comment posted in Step 2A2 triggers @codex asynchronously. @codex can take **15+ minutes** to respond. You must verify that @codex has actually responded to **the most recent** request, not a previous iteration's request. Replies from earlier iterations do NOT count.

**How to check:**
- Find the timestamp of the **last** `@codex` review request comment (the one posted in Step 2A2 of the final iteration). You can identify it by looking for comments authored by the current user containing "@codex" in the body:
```bash
gh api repos/{owner}/{repo}/issues/{pr-number}/comments --paginate --jq '
[.[] | select(.body | test("@codex")) | select(.user.login != "codex") | select(.user.login != "chatgpt-codex-connector[bot]")] | last | .created_at'
gh api repos/{owner}/{repo}/issues/{pr-number}/comments --jq '
[.[] | select(.body | test("@codex")) | select(.user.login != "codex")] | last | .created_at'
```
- Then check whether the codex bot has posted a review **after** that timestamp. Check both possible bot logins (`codex` and `chatgpt-codex-connector[bot]`):
- Then check whether @codex has posted a review **after** that timestamp:
```bash
gh api repos/{owner}/{repo}/pulls/{pr-number}/reviews --paginate --jq '
[.[] | select(.user.login == "codex" or .user.login == "chatgpt-codex-connector[bot]")] | last | {submitted_at, state, user: .user.login}'
gh api repos/{owner}/{repo}/pulls/{pr-number}/reviews --jq '
[.[] | select(.user.login == "codex")] | last | {submitted_at, state}'
```
- Also check issue comments (the bot may reply as a comment instead of a review):
```bash
gh api repos/{owner}/{repo}/issues/{pr-number}/comments --paginate --jq '
[.[] | select(.user.login == "codex" or .user.login == "chatgpt-codex-connector[bot]")] | last | {created_at, user: .user.login}'
```
- Compare timestamps. If the bot's latest review `submitted_at` (or comment `created_at`) is **after** the latest request's `created_at`, the bot has replied — **verification passes**. Use whichever response (review or comment) has the most recent timestamp.
- Compare the two timestamps. If @codex's latest review `submitted_at` is **after** the latest request's `created_at`, @codex has replied — **verification passes**.

**Polling wait if the bot hasn't replied yet:**
**Polling wait if @codex hasn't replied yet:**

Do NOT immediately fail. Instead, poll and wait:
- **Poll interval:** 1 minute (use `sleep 60` between checks)
- **Maximum wait:** 10 minutes (up to 10 poll attempts)
- On each poll iteration, re-run the `gh api` commands above and compare timestamps
- On each poll iteration, re-run both `gh api` commands above and compare timestamps
- Log each poll attempt: `"Waiting for @codex reply... (attempt N/10, elapsed Xm)"`

**Only fail this verification** if the bot has still not replied after the full 10-minute wait. Then go back to **Step 2: Run the review-fix loop**.
**Only fail this verification** if @codex has still not replied after the full 10-minute wait. Then go back to **Step 2: Run the review-fix loop**.

**If the bot has no reviews or comments at all** after the 10-minute wait, the verification also fails.
**If @codex has no reviews at all** after the 20-minute wait, the verification also fails.

Record the final state of each dimension (self-review, external reviews, CI, @codex response).

Expand Down
17 changes: 0 additions & 17 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,6 @@ jobs:
- name: Run tests with race detector
run: go test -race -v ./...

gofmt:
name: gofmt
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
- uses: actions/setup-go@4b73464bb391d4059bd26b0524d20df3927bd417 # v6.3.0
with:
go-version-file: .go-version
- name: Check gofmt
run: |
unformatted=$(gofmt -l .)
if [ -n "$unformatted" ]; then
echo "The following files are not gofmt'd:"
echo "$unformatted"
exit 1
fi

test-against-bash:
name: Test against Bash (Docker)
runs-on: ubuntu-latest
Expand Down
8 changes: 0 additions & 8 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,6 @@ The shell is supported on Linux, Windows and macOS.

- `README.md` and `SHELL_FEATURES.md` must be kept up to date with the implementation.

## Code Style

- **All Go files must be formatted with `gofmt` before committing.** Run `gofmt -w .` from the repo root and verify with `gofmt -l .` (no output means clean). CI enforces this and will fail if any file is not properly formatted.

## Pull Requests

- **Always open pull requests in draft mode.** Use `gh pr create --draft` (or the GitHub UI's "Draft pull request" option). Only mark a PR ready for review once all CI checks pass and the work is complete.

## CRITICAL: Bug Fixes and Bash Compatibility

- **ALWAYS prioritise fixing the shell implementation to match bash behaviour over changing tests to match the current (incorrect) shell output.** Never "fix" a failing test by updating its expected output to match broken shell behaviour — fix the shell instead.
Expand Down
1 change: 1 addition & 0 deletions interp/builtins/printf/printf.go
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,7 @@ func parseFloatArg(s string) (floatArg, error) {
return floatArg{f: val}, nil
}


// processBEscapes handles backslash escapes for %b (like echo -e).
// Returns the processed string, whether \c was seen (stop all output),
// and any warning messages to emit to stderr.
Expand Down
2 changes: 1 addition & 1 deletion interp/builtins/strings_cmd/strings.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ const (

// radixFlagVal implements pflag.Value for the -t / --radix flag.
// Validation happens in Set so pflag reports errors during parsing, which also
// correctly rejects empty values (e.g. --radix= or -t ).
// correctly rejects empty values (e.g. --radix= or -t '').
type radixFlagVal struct{ target *radixFormat }

func (r *radixFlagVal) String() string {
Expand Down
10 changes: 5 additions & 5 deletions interp/builtins/tail/tail.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,15 @@ type countMode struct {
// returns a bound handler whose flag variables are captured by closure. The
// framework calls Parse and passes positional arguments to the handler.
func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc {
help := fs.BoolP("help", "h", false, "print usage and exit")
help := fs.BoolP("help", "h", false, "print usage and exit")
zeroTerminated := fs.BoolP("zero-terminated", "z", false, "use NUL as line delimiter")

// quietFlag, silentFlag, and verboseFlag share a sequence counter so that
// after parsing we can tell which appeared last on the command line and
// apply last-flag-wins semantics (e.g. "-q -v" should show headers).
var headerSeq int
quietFlag := newHeaderFlag(&headerSeq)
silentFlag := newHeaderFlag(&headerSeq)
quietFlag := newHeaderFlag(&headerSeq)
silentFlag := newHeaderFlag(&headerSeq)
verboseFlag := newHeaderFlag(&headerSeq)
fs.VarP(quietFlag, "quiet", "q", "never print file name headers")
fs.Var(silentFlag, "silent", "alias for --quiet")
Expand Down Expand Up @@ -162,10 +162,10 @@ func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc {
// Bytes mode wins if -c/--bytes was parsed after -n/--lines.
useBytesMode := bytesFlag.pos > linesFlag.pos

countStr := linesFlag.val
countStr := linesFlag.val
modeLabel := "lines"
if useBytesMode {
countStr = bytesFlag.val
countStr = bytesFlag.val
modeLabel = "bytes"
}

Expand Down
74 changes: 54 additions & 20 deletions interp/builtins/wc/wc.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,26 @@ type options struct {
showMaxLineLen bool
}

func (o options) columnCount() int {
n := 0
if o.showLines {
n++
}
if o.showWords {
n++
}
if o.showChars {
n++
}
if o.showBytes {
n++
}
if o.showMaxLineLen {
n++
}
return n
}

func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc {
help := fs.BoolP("help", "h", false, "print usage and exit")
lines := fs.BoolP("lines", "l", false, "print the newline counts")
Expand Down Expand Up @@ -175,7 +195,8 @@ func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc {
}

width := fieldWidth(total, opts)
if hasStdin && width < stdinMinWidth {
nCols := opts.columnCount()
if hasStdin && nCols >= 2 && width < stdinMinWidth {
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 Apply stdin width padding for single-column multi-file output

The new hasStdin && nCols >= 2 guard drops GNU-compatible padding for cases that still require the 7-character stdin field width when only one count column is selected but multiple inputs are printed. For example, with GNU wc, printf 'a\n' | wc -l - file produces padded counts (" 1 -", " 1 file", " 2 total"), but this branch leaves width at 1 and emits unpadded output. This is a user-visible formatting regression for mixed stdin+file invocations in single-column mode.

Useful? React with 👍 / 👎.

width = stdinMinWidth
}

Expand Down Expand Up @@ -273,16 +294,29 @@ func countReader(ctx context.Context, r io.Reader) (counts, error) {
lineLen = 0
inWord = false
} else if r == '\r' {
if lineLen > c.maxLineLen {
c.maxLineLen = lineLen
}
lineLen = 0
inWord = false
Comment thread
AlexandreYang marked this conversation as resolved.
} else if r == '\t' {
lineLen = (lineLen/8 + 1) * 8
inWord = false
} else if r == ' ' || r == '\v' || r == '\f' {
} else if r == '\f' {
if lineLen > c.maxLineLen {
c.maxLineLen = lineLen
}
lineLen = 0
inWord = false
} else if r == ' ' {
lineLen++
Comment thread
AlexandreYang marked this conversation as resolved.
inWord = false
} else if r == '\v' {
Comment thread
AlexandreYang marked this conversation as resolved.
Comment thread
AlexandreYang marked this conversation as resolved.
// vertical tab: zero display width, breaks words
inWord = false
} else {
if !inWord {
isControl := unicode.Is(unicode.Cc, r)
if !inWord && !isControl {
c.words++
Comment thread
AlexandreYang marked this conversation as resolved.
Comment thread
AlexandreYang marked this conversation as resolved.
inWord = true
Comment thread
AlexandreYang marked this conversation as resolved.
}
Expand Down Expand Up @@ -353,25 +387,25 @@ func runeWidth(r rune) int {
// codepoints per UAX #11, matching the ranges used by wcwidth(3).
var eastAsianWide = &unicode.RangeTable{
R16: []unicode.Range16{
{0x1100, 0x115F, 1}, // Hangul Jamo initials
{0x2329, 0x232A, 1}, // CJK angle brackets
{0x2E80, 0x303E, 1}, // CJK Radicals Supplement .. CJK Symbols
{0x3040, 0x33BF, 1}, // Hiragana .. CJK Compatibility
{0x33C0, 0x33FF, 1}, // CJK Compatibility (cont.)
{0x3400, 0x4DBF, 1}, // CJK Unified Ideographs Extension A
{0x4E00, 0xA4CF, 1}, // CJK Unified Ideographs .. Yi
{0xAC00, 0xD7A3, 1}, // Hangul Syllables
{0xF900, 0xFAFF, 1}, // CJK Compatibility Ideographs
{0xFE10, 0xFE19, 1}, // Vertical Forms
{0xFE30, 0xFE6F, 1}, // CJK Compatibility Forms + Small Form Variants
{0xFF01, 0xFF60, 1}, // Fullwidth Forms
{0xFFE0, 0xFFE6, 1}, // Fullwidth Signs
{0x1100, 0x115F, 1}, // Hangul Jamo initials
{0x2329, 0x232A, 1}, // CJK angle brackets
{0x2E80, 0x303E, 1}, // CJK Radicals Supplement .. CJK Symbols
{0x3040, 0x33BF, 1}, // Hiragana .. CJK Compatibility
{0x33C0, 0x33FF, 1}, // CJK Compatibility (cont.)
{0x3400, 0x4DBF, 1}, // CJK Unified Ideographs Extension A
{0x4E00, 0xA4CF, 1}, // CJK Unified Ideographs .. Yi
{0xAC00, 0xD7A3, 1}, // Hangul Syllables
{0xF900, 0xFAFF, 1}, // CJK Compatibility Ideographs
{0xFE10, 0xFE19, 1}, // Vertical Forms
{0xFE30, 0xFE6F, 1}, // CJK Compatibility Forms + Small Form Variants
{0xFF01, 0xFF60, 1}, // Fullwidth Forms
{0xFFE0, 0xFFE6, 1}, // Fullwidth Signs
},
R32: []unicode.Range32{
{0x1F300, 0x1F64F, 1}, // Misc Symbols/Pictographs + Emoticons
{0x1F900, 0x1F9FF, 1}, // Supplemental Symbols and Pictographs
{0x20000, 0x2FFFD, 1}, // CJK Extension B..F
{0x30000, 0x3FFFD, 1}, // CJK Extension G+
{0x1F300, 0x1F64F, 1}, // Misc Symbols/Pictographs + Emoticons
{0x1F900, 0x1F9FF, 1}, // Supplemental Symbols and Pictographs
Comment thread
AlexandreYang marked this conversation as resolved.
Comment thread
AlexandreYang marked this conversation as resolved.
Comment thread
AlexandreYang marked this conversation as resolved.
Comment thread
AlexandreYang marked this conversation as resolved.
{0x20000, 0x2FFFD, 1}, // CJK Extension B..F
{0x30000, 0x3FFFD, 1}, // CJK Extension G+
},
}

Expand Down
8 changes: 4 additions & 4 deletions interp/builtins/wc/wc_gnu_compat_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestGNUCompatWordsMulti(t *testing.T) {
// TestGNUCompatBytesCount — -c on "x".
//
// GNU command: printf 'x' | gwc -c
// Expected: "1\n"
// Expected: "0\n"
func TestGNUCompatBytesCount(t *testing.T) {
dir := t.TempDir()
writeFile(t, dir, "file.txt", "x")
Expand Down Expand Up @@ -148,16 +148,16 @@ func TestGNUCompatCharsMultibyte(t *testing.T) {
assert.Equal(t, "5 file.txt\n", stdout)
}

// TestGNUCompatControlCharIsWord — control byte \x01 counts as a word.
// TestGNUCompatControlCharIsWord — control byte \x01 does NOT count as a word.
//
// GNU command: printf '\x01\n' | gwc -w
// Expected: "1\n"
// Expected: "0\n"
func TestGNUCompatControlCharIsWord(t *testing.T) {
dir := t.TempDir()
writeFile(t, dir, "file.txt", "\x01\n")
stdout, _, code := cmdRun(t, "wc -w file.txt", dir)
assert.Equal(t, 0, code)
assert.Equal(t, "1 file.txt\n", stdout)
assert.Equal(t, "0 file.txt\n", stdout)
}

// TestGNUCompatRejectedFlag — unknown flag exits 1.
Expand Down
13 changes: 11 additions & 2 deletions interp/builtins/wc/wc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func TestWcWordsControlChar(t *testing.T) {
writeFile(t, dir, "file.txt", "\x01\n")
stdout, _, code := cmdRun(t, "wc -w file.txt", dir)
assert.Equal(t, 0, code)
assert.Equal(t, "1 file.txt\n", stdout)
assert.Equal(t, "0 file.txt\n", stdout)
}

// --- Bytes ---
Expand Down Expand Up @@ -354,7 +354,7 @@ func TestWcPipeInput(t *testing.T) {
writeFile(t, dir, "file.txt", "alpha\nbeta\ngamma\n")
stdout, _, code := cmdRun(t, "cat file.txt | wc -l", dir)
assert.Equal(t, 0, code)
assert.Equal(t, " 3\n", stdout)
assert.Equal(t, "3\n", stdout)
}

// --- Combined flags ---
Expand Down Expand Up @@ -406,6 +406,14 @@ func TestWcMaxLineLenCR(t *testing.T) {
assert.Equal(t, "5 file.txt\n", stdout)
}
Comment thread
AlexandreYang marked this conversation as resolved.

func TestWcMaxLineLenCRLongerPrefix(t *testing.T) {
dir := t.TempDir()
writeFile(t, dir, "file.txt", "abcdef\rxy\n")
stdout, _, code := cmdRun(t, "wc -L file.txt", dir)
assert.Equal(t, 0, code)
assert.Equal(t, "6 file.txt\n", stdout)
}

func TestWcCRLFLineCount(t *testing.T) {
dir := t.TempDir()
writeFile(t, dir, "file.txt", "a\r\nb\r\n")
Expand Down Expand Up @@ -451,3 +459,4 @@ func TestWcChunkBoundaryMultibyte(t *testing.T) {
// max line length: 32766 + 2 (emoji display width) = 32768
assert.Equal(t, "32768 32768 file.txt\n", stdout)
}

2 changes: 1 addition & 1 deletion interp/register_builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import (
"github.com/DataDog/rshell/interp/builtins"
breakcmd "github.com/DataDog/rshell/interp/builtins/break"
"github.com/DataDog/rshell/interp/builtins/cat"
continuecmd "github.com/DataDog/rshell/interp/builtins/continue"
"github.com/DataDog/rshell/interp/builtins/cut"
continuecmd "github.com/DataDog/rshell/interp/builtins/continue"
"github.com/DataDog/rshell/interp/builtins/echo"
"github.com/DataDog/rshell/interp/builtins/exit"
falsecmd "github.com/DataDog/rshell/interp/builtins/false"
Expand Down
10 changes: 5 additions & 5 deletions tests/compliance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,11 @@ func TestComplianceLicense3rdPartyFormat(t *testing.T) {
// Known SPDX identifiers (extend as needed).
knownSPDX := map[string]bool{
"MIT": true,
"Apache-2.0": true,
"BSD-2-Clause": true,
"BSD-3-Clause": true,
"ISC": true,
"MPL-2.0": true,
"Apache-2.0": true,
"BSD-2-Clause": true,
"BSD-3-Clause": true,
"ISC": true,
"MPL-2.0": true,
"MIT AND Apache-2.0": true,
}

Expand Down
3 changes: 1 addition & 2 deletions tests/scenarios/cmd/ls/pipes/pipe_to_wc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ input:
script: |+
ls | wc -l
expect:
stdout: |2+
3
stdout: "3\n"
stderr: ""
exit_code: 0
1 change: 1 addition & 0 deletions tests/scenarios/cmd/wc/errors/files0_from_rejected.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ input:
allowed_paths: ["$DIR"]
script: |+
wc --files0-from=foo
skip_assert_against_bash: true
expect:
stdout: ""
stderr_contains: ["wc:"]
Expand Down
2 changes: 1 addition & 1 deletion tests/scenarios_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const dockerBashImage = "debian:bookworm-slim"
// scenario represents a single test scenario.
type scenario struct {
Description string `yaml:"description"`
SkipAssertAgainstBash bool `yaml:"skip_assert_against_bash"` // true = skip bash comparison
SkipAssertAgainstBash bool `yaml:"skip_assert_against_bash"` // true = skip bash comparison
Setup setup `yaml:"setup"`
Input input `yaml:"input"`
Expect expected `yaml:"expect"`
Expand Down
Loading