diff --git a/.claude/skills/review-fix-loop/SKILL.md b/.claude/skills/review-fix-loop/SKILL.md index 37746c9e..a0bc821a 100644 --- a/.claude/skills/review-fix-loop/SKILL.md +++ b/.claude/skills/review-fix-loop/SKILL.md @@ -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!) { @@ -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). diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2dec643f..6ec5b00e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -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 diff --git a/AGENTS.md b/AGENTS.md index c6e5ca7d..d34973f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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. diff --git a/interp/builtins/printf/printf.go b/interp/builtins/printf/printf.go index 54b6cb76..c7538cee 100644 --- a/interp/builtins/printf/printf.go +++ b/interp/builtins/printf/printf.go @@ -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. diff --git a/interp/builtins/strings_cmd/strings.go b/interp/builtins/strings_cmd/strings.go index b884e59a..bdba91f8 100644 --- a/interp/builtins/strings_cmd/strings.go +++ b/interp/builtins/strings_cmd/strings.go @@ -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 { diff --git a/interp/builtins/tail/tail.go b/interp/builtins/tail/tail.go index 35d231e3..b7f227f6 100644 --- a/interp/builtins/tail/tail.go +++ b/interp/builtins/tail/tail.go @@ -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") @@ -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" } diff --git a/interp/builtins/wc/wc.go b/interp/builtins/wc/wc.go index 7fbf50ee..89ce4062 100644 --- a/interp/builtins/wc/wc.go +++ b/interp/builtins/wc/wc.go @@ -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") @@ -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 { width = stdinMinWidth } @@ -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 } 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++ inWord = false + } else if r == '\v' { + // vertical tab: zero display width, breaks words + inWord = false } else { - if !inWord { + isControl := unicode.Is(unicode.Cc, r) + if !inWord && !isControl { c.words++ inWord = true } @@ -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 + {0x20000, 0x2FFFD, 1}, // CJK Extension B..F + {0x30000, 0x3FFFD, 1}, // CJK Extension G+ }, } diff --git a/interp/builtins/wc/wc_gnu_compat_test.go b/interp/builtins/wc/wc_gnu_compat_test.go index 90966364..74e29322 100644 --- a/interp/builtins/wc/wc_gnu_compat_test.go +++ b/interp/builtins/wc/wc_gnu_compat_test.go @@ -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") @@ -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. diff --git a/interp/builtins/wc/wc_test.go b/interp/builtins/wc/wc_test.go index dd2e3d20..1cc02994 100644 --- a/interp/builtins/wc/wc_test.go +++ b/interp/builtins/wc/wc_test.go @@ -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 --- @@ -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 --- @@ -406,6 +406,14 @@ func TestWcMaxLineLenCR(t *testing.T) { assert.Equal(t, "5 file.txt\n", stdout) } +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") @@ -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) } + diff --git a/interp/register_builtins.go b/interp/register_builtins.go index 03608922..c494ca39 100644 --- a/interp/register_builtins.go +++ b/interp/register_builtins.go @@ -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" diff --git a/tests/compliance_test.go b/tests/compliance_test.go index e2e62683..037a0008 100644 --- a/tests/compliance_test.go +++ b/tests/compliance_test.go @@ -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, } diff --git a/tests/scenarios/cmd/ls/pipes/pipe_to_wc.yaml b/tests/scenarios/cmd/ls/pipes/pipe_to_wc.yaml index 9c687821..eab32a95 100644 --- a/tests/scenarios/cmd/ls/pipes/pipe_to_wc.yaml +++ b/tests/scenarios/cmd/ls/pipes/pipe_to_wc.yaml @@ -16,7 +16,6 @@ input: script: |+ ls | wc -l expect: - stdout: |2+ - 3 + stdout: "3\n" stderr: "" exit_code: 0 diff --git a/tests/scenarios/cmd/wc/errors/files0_from_rejected.yaml b/tests/scenarios/cmd/wc/errors/files0_from_rejected.yaml index d74a82d9..cda15c61 100644 --- a/tests/scenarios/cmd/wc/errors/files0_from_rejected.yaml +++ b/tests/scenarios/cmd/wc/errors/files0_from_rejected.yaml @@ -4,6 +4,7 @@ input: allowed_paths: ["$DIR"] script: |+ wc --files0-from=foo +skip_assert_against_bash: true expect: stdout: "" stderr_contains: ["wc:"] diff --git a/tests/scenarios_test.go b/tests/scenarios_test.go index 58652141..e3240c32 100644 --- a/tests/scenarios_test.go +++ b/tests/scenarios_test.go @@ -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"`