fix(ci): PR #187 머지 후 남은 리뷰 5건 — 가드의 사각지대 4부류 - #190
Conversation
…ast commit CodeRabbit's review (5 threads) and Codex's second review arrived at 06:22 and 06:23, five minutes after the final commit at 06:17, and #187 merged the next day with none of them addressed. All five reproduce against current main; this is the follow-up. The guard was blind to four whole classes, each verified by re-injecting it into a worktree and watching the guard report OK: - GNU long options. `date --date=`, `stat --format=`, `grep --perl-regexp` and `sed --in-place` are the same GNU features the short flags name, and every rule only matched single-dash spellings. `grep --perl-regexp` is `grep -oP` wearing a different hat, and this repo leads its BSD notes with that trap. - Extensionless shell entry points. Selection was `*.sh`, `*.bash`, `*.md`, so `.githooks/pre-commit` -- bash, no suffix, and the hook that RUNS this guard -- was never scanned. Tracked files are now also discovered by shebang. - Fake fallbacks. A bare `||` cleared a finding, so `md5sum "$f" || exit 1` read as portable while still being broken on macOS. A lone probe cleared it too, so `sed --version >/dev/null` followed by an unbranched `sed -i` passed. A `||` now counts only when the BSD counterpart is on its right side, and a probe only when the counterpart is actually nearby. Rules with no counterpart at all (grep -P, timeout, bash 4 syntax) cannot be cleared by either -- there is no portable alternative to point at, so only an explicit exemption applies. - An exemption nobody had to justify. Both docs specify `# portability-ok: <reason>`; ESCAPE matched the bare prefix, so a reasonless marker passed. The colon and a non-empty reason are now required. No existing comment used the marker, so tightening it breaks nothing. Re-injection now reports 8/8 across all four classes, and the control file -- `stat -c || stat -f`, `sha256sum || shasum`, a `date -d` / `date -j` pair, `nproc || sysctl`, and a properly-justified exemption -- still passes clean. Separately, the macOS diagnostic step printed through `echo "$(...)"`, which discards the inner exit status. That is the "종료 상태가 사라지는 자리" P1 this repo added in #183, and a workflow breaking its own rule is a poor exemplar. The commands print directly now; under the step's `bash -e` shell a genuinely missing bash fails the step instead of emitting a blank line. The tool loop keeps its `|| echo '(absent)'`, since absence is exactly what it is asking about. AGENTS.md and README.md describe the narrowed acceptance rules and the widened scan surface.
📝 WalkthroughWalkthroughThe shell portability checker now uses token-based GNU construct detection, stricter fallback and exemption rules, and shell-shebang file discovery. A standalone regression suite runs in pre-commit and CI. Documentation reflects the rules, and macOS diagnostics use direct Bash commands. ChangesShell portability enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 255774f9ac
ℹ️ 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".
| { id: 'stat -c', tool: 'stat', token: /\bstat\s+(-[a-zA-Z]+\s+)*-[a-zA-Z]*c[a-zA-Z]*(\s|=)/, bsd: /\bstat\s+(-[a-zA-Z]+\s+)*-f\b/, alt: '`stat -c … || stat -f …`' }, | ||
| { id: 'sed -i', tool: 'sed', token: /\bsed\s+(-[a-zA-Z]*\s+)*-i(\s|$)(?!\s*(''|""))|\bsed\s+(--[a-z-]+\s+)*--in-place(\s|=|$)/, bsd: /sed\s+-i\s+''/, alt: "a sed_inplace() helper branching on `sed --version` (GNU takes `-i`, BSD takes `-i ''`)" }, | ||
| { id: 'sed -r', tool: 'sed', token: /\bsed\s+(-[a-zA-Z]*\s+)*-[a-zA-Z]*r[a-zA-Z]*(\s|$)|\bsed\s+(--[a-z-]+\s+)*--regexp-extended(\s|$)/, alt: 'sed -E (accepted by both GNU and BSD)' }, | ||
| { id: 'grep -P', tool: 'grep', token: /\bgrep\s+(-[a-zA-Z]*\s+)*-[a-zA-Z]*P[a-zA-Z]*(\s|$)|\bgrep\s+(--[a-z-]+\s+)*--perl-regexp(\s|$)/, alt: "sed -n 's/…/\\1/p' or awk (BSD grep has no PCRE at all)" }, |
There was a problem hiding this comment.
GNU grep --help가 각각 -n, --line-number와 -P, --perl-regexp를 지원하며 grep -n --perl-regexp 'x'도 정상 실행되지만, 이 정규식은 grep과 --perl-regexp 사이에 인수 없는 긴 옵션만 허용하므로 해당 호출을 전혀 검출하지 않습니다. tracked 셸 픽스처에서 이 호출을 넣어도 가드가 OK를 반환했으며, 같은 구조 때문에 date -u --date, stat -L --format, sed -e ... --in-place도 빠집니다. 앞선 short/argument-bearing 옵션을 처리하도록 파싱하거나 각 긴 옵션 자체를 더 독립적으로 매칭해야 합니다.
AGENTS.md reference: AGENTS.md:L265-L265
Useful? React with 👍 / 👎.
| echo "PATH bash:" | ||
| bash --version | head -1 | ||
| echo "/bin/bash:" | ||
| /bin/bash --version | head -1 |
There was a problem hiding this comment.
이 macOS 진단 스텝의 기본 bash -e에는 pipefail이 없으므로, PATH의 bash는 존재하지만 /bin/bash가 없거나 실행에 실패하는 환경에서 /bin/bash --version의 실패가 head의 성공 상태로 덮여 스텝이 계속 통과합니다. 실제로 bash -e -c '/missing/bash --version | head -1; echo survived'가 0으로 끝나므로, 이번 변경이 제거하려던 false-green이 그대로 남습니다. 파이프 전에 set -o pipefail을 적용하거나 버전 명령의 상태를 별도로 확인해야 합니다.
AGENTS.md reference: AGENTS.md:L293-L293
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-shell-portability.mjs`:
- Around line 116-131: Limit shebang detection in shebangShellFiles to sh/bash
interpreters only by narrowing the regular expression to match sh and bash,
excluding dash, ksh, and zsh. Keep the existing tracked-file scanning and
deduplication flow unchanged.
- Around line 170-171: Update the GENERIC_PROBE handling in the shell
portability checker so a standalone uname -s execution, including logging
commands, cannot mark nearby GNU constructs as handled. Require uname usage to
be part of a verifiable case or conditional branch that provides a
macOS-specific path or alternative, or require an explicit exemption; preserve
handling for genuinely validated branching constructs.
- Around line 36-45: Update the command-detection regexes in the portability
rule definitions, especially the `sed -i` and GNU-option entries such as `grep
-P`, so preceding long options with `=value` are consumed and attached empty
`sed` suffixes (`-i''` and `-i""`) are detected. Add regression coverage for
these command forms and ensure GNU-specific invocations cannot bypass the macOS
portability guard.
- Around line 168-180: Update the portability checks around ESCAPE,
GENERIC_PROBE, and rule.bsd so matching examines only unquoted shell tokens,
preventing BSD command names inside string literals from satisfying a fallback.
Restrict the portability-ok exemption lookup to the unquoted comment portion
rather than the raw line, while preserving valid executable fallback and
exception detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cae8a4cf-fe2b-473b-ab5d-93126203b571
📒 Files selected for processing (4)
.github/workflows/validate-codex.ymlAGENTS.mdREADME.mdscripts/check-shell-portability.mjs
Six more findings on this guard, and the pattern behind them is the point. Each
review round produced a spelling the regexes had not anticipated -- `grep -oP`,
then `-Pio`, then `--perl-regexp`, then `--color=always -P`, then
`-n --perl-regexp`. Emulating shell option syntax with regexes is a losing game.
The matcher now tokenizes into shell words, which ends that class and brings
quote awareness with it.
What the tokenizer closed, all verified by re-injection:
- Every option spelling. Long and short flags are matched as words, so preceding
flags -- with or without `=value` -- no longer hide the one that matters.
- Quoted text is no longer evidence. `md5sum "$f" || printf '%s\n' 'md5'` read as
portable because the STRING `'md5'` satisfied the BSD counterpart, and
`printf '# portability-ok: …'` read as an exemption. Strings are tokens now.
- A logging `uname -s` is not a branch. `printf '%s\n' "$(uname -s)"` cleared
every construct within four lines; only a real `case "$(uname …)"` counts.
- Naming a command is not running it. Command position is tracked, so the
`timeout` in `command -v timeout` is an argument, and a `case` pattern listing
`iteration_cap|timeout|x)` is a pattern, not a call. The regex version only
tolerated the first of those by accident, reading `command -v` as a probe.
- A construct no longer clears itself. Counterpart matching was `startsWith`, so
`'md5sum'.startsWith('md5')` made any md5sum line the BSD fallback for the
md5sum line beside it. Exact tokens only.
Also: the macOS diagnostic step sets pipefail. `bash -e` alone does not, so a
failing `/bin/bash --version` was masked by `head` succeeding -- the same false
green the step had just been rewritten to remove, one layer down. Codex
demonstrated it: `bash -e -c '/missing/bash --version | head -1; echo survived'`
exits 0.
And the guard gets the coverage it should have had from the start. Nine holes
across three rounds, each found by hand-injecting defects into a throwaway
worktree and reading the output; check-shell-portability.test.mjs is that
verification kept, 30 cases in both directions -- constructs that must be
flagged, and correct fallbacks that must not be. The second half is what keeps
the guard usable: one false positive on `stat -c … || stat -f …` and someone
switches it off. Wired into pre-commit and CI beside the guard itself, because a
detector without coverage reports "nothing wrong" when it means "could not look"
-- which is the lore page this repo has been filling in all week.
The file grew from 170 to 336 lines. Earlier in this work I claimed tokenizing
would shrink it; that was wrong, and the trade is worth stating plainly: more
code, one closed class instead of an open-ended regex patch queue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d2727553
ℹ️ 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".
| if (t.op) { atCmd = true; continue } | ||
| if (!atCmd) continue | ||
| if (!t.hadQuote && /^[A-Za-z_][A-Za-z0-9_]*=/.test(t.bare)) continue // VAR=val prefix | ||
| pos.add(i) | ||
| atCmd = false |
There was a problem hiding this comment.
tracked 셸 픽스처에 if md5sum f; then echo x; fi를 넣으면 이 가드는 exit 0을 반환합니다. commandPositions()가 if를 명령으로 확정한 뒤 공백에서는 atCmd를 되살리지 않으므로 then·do·!·{ 뒤의 실제 명령과 LC_ALL="C" md5sum f 같은 따옴표 포함 assignment prefix도 검사에서 빠지며, 기존 정규식 검사가 잡던 GNU 전용 호출에 CI와 pre-commit이 false-green을 냅니다. 셸 예약어·그룹 시작과 유효한 assignment prefix를 명령 앞 문법으로 처리하거나 실제 셸 파서를 사용해야 합니다.
AGENTS.md reference: AGENTS.md:L265-L265
Useful? React with 👍 / 👎.
| function isCasePattern(code) { | ||
| const opens = (code.match(/\(/g) || []).length | ||
| const closes = (code.match(/\)/g) || []).length | ||
| return closes > opens && /\)\s*($|[^(]*;;)/.test(code) |
There was a problem hiding this comment.
case arm의 패턴과 본문이 같은 줄에 있는 case "$x" in a) md5sum f;; esac를 tracked 픽스처로 검사하면 exit 0입니다. isCasePattern()이 닫는 )를 발견하는 즉시 줄 전체를 건너뛰어 패턴 이름뿐 아니라 그 뒤에서 실제 실행되는 GNU 전용 명령까지 숨기기 때문입니다. ) 이전의 패턴 토큰만 호출 후보에서 제외하고 arm 본문은 계속 스캔해야 합니다.
AGENTS.md reference: AGENTS.md:L265-L265
Useful? React with 👍 / 👎.
| function hasBsdToken(tokens, bsd) { | ||
| return tokens.some((t) => !t.hadQuote && bsd.includes(t.bare)) |
There was a problem hiding this comment.
대응물 탐색이 명령 위치와 필수 옵션을 확인하지 않아, 예를 들어 tail -n 1 file 근처의 무가드 tac file, sysctl kern.ostype 근처의 nproc, 또는 printf '%s\n' -j 근처의 date -d가 모두 exit 0으로 통과합니다. 이 호출들은 각각 필요한 tail -r, sysctl -n hw.ncpu, date -j 폴백이 아니므로 macOS에서는 여전히 깨집니다. hasBsdToken()은 단순 토큰 존재가 아니라 rule별 전체 호출 형태와 명령 위치를 검증해야 합니다.
AGENTS.md reference: AGENTS.md:L265-L265
Useful? React with 👍 / 👎.
| const attached = a.bare.slice(a.bare.indexOf('i') + 1) | ||
| if (attached) return false | ||
| const next = tokens[j + 1] | ||
| if (next && next.hadQuote && next.bare === '' && next.text === '') return false |
There was a problem hiding this comment.
한 줄에 sed -i.bak 's/a/b/' f; sed -i 's/c/d/' g가 있으면 첫 번째 포터블 호출에서 cmdUsed()가 곧바로 false를 반환해 뒤의 GNU 전용 호출을 검사하지 않으며, tracked 픽스처에서도 가드가 exit 0을 냅니다. sed -i ''가 먼저 오는 경우도 같습니다. 포터블한 개별 호출은 해당 후보만 건너뛰고, 같은 줄의 나머지 sed 호출을 끝까지 순회해야 합니다.
AGENTS.md reference: AGENTS.md:L265-L265
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
scripts/check-shell-portability.mjs (5)
239-248: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value전체 추적 파일을 통째로 메모리에 읽습니다.
Line 244는 첫 120바이트만 필요한데
readFileSync로 파일 전체를 읽습니다. 바이너리 자산이 큰 저장소에서는 커밋 훅 지연이 눈에 띌 수 있으니openSync+readSync로 헤더만 읽거나, 최소한 널 바이트가 보이면 조기 반환하세요.♻️ 제안
- let head - try { head = readFileSync(f, 'utf8').slice(0, 120).split('\n')[0] } catch { continue } + let head = '' + let fd + try { + fd = openSync(f, 'r') + const buf = Buffer.alloc(120) + const read = readSync(fd, buf, 0, 120, 0) + head = buf.subarray(0, read).toString('utf8').split('\n')[0] + } catch { continue } finally { if (fd !== undefined) closeSync(fd) }import도
import { openSync, readSync, closeSync } from 'node:fs'로 조정이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.mjs` around lines 239 - 248, Update shebangShellFiles to read only the first 120 bytes of each candidate file using openSync, readSync, and closeSync instead of readFileSync; adjust the node:fs imports accordingly, preserve the existing shebang detection, and ensure opened descriptors are closed even when reading fails.
84-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
if [ "$(uname -s)" = Darwin ]형태의 분기는 인정되지 않습니다.
UNAME_BRANCH는case "$(uname만 매칭하므로, 동등하게 정당한if/[[ ]]기반 OS 분기 안의 GNU 구문은 그대로 차단됩니다. 헤더가 경고한 "cries wolf" 사례에 해당하니 조건문 형태도 허용하는 편이 좋습니다. (참고:(uname|\s*uname)대체 분기는 사실상 중복입니다.)♻️ 제안
-const UNAME_BRANCH = /\bcase\s+"?\$\(\s*uname/ +// `case "$(uname …)"` 또는 `if [ "$(uname -s)" = Darwin ]` 형태의 실제 분기만 인정. +const UNAME_BRANCH = /(\bcase\s+"?\$\(\s*uname)|((\bif|\[\[|\[)\s.*\$\(\s*uname)/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.mjs` around lines 84 - 87, Update UNAME_BRANCH to recognize genuine OS-branch conditions using if or [[ ]] comparisons around uname, including forms such as `if [ "$(uname -s)" = Darwin ]`, while continuing to reject bare uname logging. Remove the redundant uname alternatives and preserve the existing case-branch matching behavior.
71-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value주석과 실제 매칭 대상이 어긋납니다.
Line 71은 "unquoted text"라고 하지만 Line 292는
here.code(주석만 제거된 원문, 따옴표 내부 포함)에 매칭합니다. 그래서printf '${x,,}'처럼 리터럴 문자열도 bash4 위반으로 잡힙니다. 주석을 실제 동작(주석 제거된 줄)에 맞추거나, 토큰의 unquoted 부분(bare)만 이어붙여 매칭하세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.mjs` around lines 71 - 81, Align the SYNTAX_RULES comment and matching behavior: either describe matching against the comment-stripped source including quoted text, or update the checker to tokenize and match only unquoted content such as bare. Ensure literals like printf '${x,,}' are not reported as Bash 4 violations.
160-164: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
case패턴 판정이 한 줄짜리 분기 본문 전체를 스캔에서 제외합니다.Line 287은
isCasePattern이 참이면 줄 전체를 건너뜁니다. 그런데linux) timeout 5 foo;;처럼 패턴과 명령이 한 줄에 있는 흔한 형태도 이 조건을 만족하므로 실제 호출이 조용히 누락됩니다. 패턴 목록 부분()앞)만 잘라내고 나머지는 계속 검사하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.mjs` around lines 160 - 164, Update isCasePattern and its caller so case-pattern detection only excludes the pattern-list portion before the closing “)” rather than skipping the entire line. Preserve scanning of any inline case body, including commands such as “linux) timeout 5 foo;;”, so actual calls are still detected.
61-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
-j/-f를 단독 토큰으로 인정하면 무관한 플래그가 가드를 해제합니다.
hasBsdToken은 토큰 전체 일치만 볼 뿐 어떤 명령의 플래그인지 보지 않습니다. 따라서stat -c %Y "$f"근처 ±4줄에rm -f "$tmp"나grep -f pat이 있으면 BSD 대안이 있는 것으로 판정되어 조용히 통과합니다(date -d+ 인접한sort -j유사).sedBsdForm처럼 명령+플래그 쌍을 확인하는bsdFn으로 바꾸는 편이 정확합니다.♻️ 명령+플래그 쌍 검사 예시
+function cmdFlagForm(cmd, flag) { + return (tokens) => { + const pos = commandPositions(tokens) + for (let i = 0; i < tokens.length; i++) { + if (tokens[i].op || tokens[i].hadQuote || tokens[i].bare !== cmd || !pos.has(i)) continue + for (let j = i + 1; j < tokens.length; j++) { + if (tokens[j].op) break + if (tokens[j].bare === flag) return true + } + } + return false + } +}
date -d/stat -c규칙의bsd: ['-j'] / ['-f']를bsdFn: cmdFlagForm('date', '-j')/cmdFlagForm('stat', '-f')로 교체하면 됩니다. 단, 현재bsdFn은k !== 0일 때만 호출되므로 같은 줄||폴백(stat -c … || stat -f …)도 커버하려면 Line 302 조건을 함께 조정해야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.mjs` around lines 61 - 64, Replace the standalone BSD token checks in the date and stat portability rules with command-specific bsdFn predicates using cmdFlagForm, matching date -j and stat -f only when paired with their respective commands. Update the bsdFn invocation guard near the rule evaluation logic so same-line || fallback forms are also checked, including tokens at the initial position when applicable; preserve the existing sedBsdForm behavior..githooks/pre-commit (1)
23-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win커밋마다 30여 개의 임시 git 저장소 + node 프로세스를 생성합니다.
check-shell-portability.test.mjs의 각 케이스는mkdtemp→git init→git add→node실행을 반복합니다(대략 30회). 다른 훅 단계들이 "sub-second"를 기준으로 삼는 것과 달리 이 단계는 수 초가 걸릴 수 있어 훅 자체가 우회 대상이 되기 쉽습니다. 가드 본체(Line 22)는 훅에 두되 가드의 자체 테스트는 CI 전용으로 옮기거나, 케이스들을 하나의 임시 저장소에서 배치 실행하도록 바꾸는 것을 권합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.githooks/pre-commit around lines 23 - 27, Update the pre-commit hook’s invocation of check-shell-portability.test.mjs so it does not create roughly 30 temporary repositories and Node processes on every commit. Keep the portability guard itself in the hook, but move this self-test to CI or refactor the test cases to run in a shared temporary repository with batched execution.scripts/check-shell-portability.test.mjs (2)
34-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win임시 저장소를 사용자 전역 git 설정으로부터 격리하세요.
git init/git add -A가 사용자 전역 설정을 그대로 상속합니다. 전역core.excludesFile에*.txt나 유사 패턴이 있으면 Line 105/109의notes.txt케이스가 추적되지 않아 테스트가 환경에 따라 실패합니다.init.defaultBranch경고도 같이 사라집니다.♻️ 제안
- const git = (...a) => execFileSync('git', ['-C', dir, ...a], { stdio: 'pipe' }) - git('init', '-q') + const git = (...a) => execFileSync('git', [ + '-C', dir, + '-c', 'core.excludesFile=', + '-c', 'init.defaultBranch=main', + ...a, + ], { stdio: 'pipe', env: { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_SYSTEM: '/dev/null' } }) + git('init', '-q')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.test.mjs` around lines 34 - 52, Update the git helper inside scan so temporary-repository commands ignore the user’s global configuration, especially core.excludesFile, and configure git init with a deterministic default branch to suppress environment-dependent warnings. Preserve the existing git init, config, and add flow while ensuring notes.txt is always included.
63-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win케이스 구성은 명확하고 양방향 커버리지가 충실합니다. 위에서 지적한 큰따옴표 명령 치환(
H="$(md5sum f)"), 단일 파이프 폴백, 인접 줄 exemption 케이스도 함께 추가해 두면 회귀가 다시 열리지 않습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-shell-portability.test.mjs` around lines 63 - 112, Extend the portability test cases using the existing flags() and clean() helpers to cover a double-quoted command substitution such as H="$(md5sum f)", a single-pipe fallback that should still be flagged, and an exemption comment placed on the adjacent line rather than the command line. Ensure each case asserts the intended scanner result and preserves the existing exemption-with-reason behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-shell-portability.mjs`:
- Around line 300-301: Update the exemption handling in the surrounding
portability-check logic so only a `# portability-ok:` comment on the same line
as the GNU construct is accepted, matching the existing same-line handling near
line 284. Remove the nearby-comment exemption check represented by
`ESCAPE.test(near.comment)` and retain only the relevant `UNAME_BRANCH`
handling.
- Around line 111-119: Update the quoted-token parsing logic around the
tokenizer’s quote-handling branch to recognize `$(` inside double-quoted text,
track nested command-substitution depth through matching parentheses, and
tokenize the substitution contents so commands such as `md5sum`, `nproc`, and
`timeout` remain detectable. Preserve existing escaped-character handling and
single-quote behavior, and ensure nested quotes within the substitution are
handled correctly.
- Around line 302-310: Update the same-line handling in the rule-processing loop
to recognize only two adjacent `|` operator tokens as the `||` fallback,
preferably via a shared helper such as `orIndex`, so a single pipe is not
accepted. Also allow `rule.bsdFn` checks when `k === 0` by applying the function
to tokens after the detected fallback operator, preserving existing behavior for
later lines.
---
Nitpick comments:
In @.githooks/pre-commit:
- Around line 23-27: Update the pre-commit hook’s invocation of
check-shell-portability.test.mjs so it does not create roughly 30 temporary
repositories and Node processes on every commit. Keep the portability guard
itself in the hook, but move this self-test to CI or refactor the test cases to
run in a shared temporary repository with batched execution.
In `@scripts/check-shell-portability.mjs`:
- Around line 239-248: Update shebangShellFiles to read only the first 120 bytes
of each candidate file using openSync, readSync, and closeSync instead of
readFileSync; adjust the node:fs imports accordingly, preserve the existing
shebang detection, and ensure opened descriptors are closed even when reading
fails.
- Around line 84-87: Update UNAME_BRANCH to recognize genuine OS-branch
conditions using if or [[ ]] comparisons around uname, including forms such as
`if [ "$(uname -s)" = Darwin ]`, while continuing to reject bare uname logging.
Remove the redundant uname alternatives and preserve the existing case-branch
matching behavior.
- Around line 71-81: Align the SYNTAX_RULES comment and matching behavior:
either describe matching against the comment-stripped source including quoted
text, or update the checker to tokenize and match only unquoted content such as
bare. Ensure literals like printf '${x,,}' are not reported as Bash 4
violations.
- Around line 160-164: Update isCasePattern and its caller so case-pattern
detection only excludes the pattern-list portion before the closing “)” rather
than skipping the entire line. Preserve scanning of any inline case body,
including commands such as “linux) timeout 5 foo;;”, so actual calls are still
detected.
- Around line 61-64: Replace the standalone BSD token checks in the date and
stat portability rules with command-specific bsdFn predicates using cmdFlagForm,
matching date -j and stat -f only when paired with their respective commands.
Update the bsdFn invocation guard near the rule evaluation logic so same-line ||
fallback forms are also checked, including tokens at the initial position when
applicable; preserve the existing sedBsdForm behavior.
In `@scripts/check-shell-portability.test.mjs`:
- Around line 34-52: Update the git helper inside scan so temporary-repository
commands ignore the user’s global configuration, especially core.excludesFile,
and configure git init with a deterministic default branch to suppress
environment-dependent warnings. Preserve the existing git init, config, and add
flow while ensuring notes.txt is always included.
- Around line 63-112: Extend the portability test cases using the existing
flags() and clean() helpers to cover a double-quoted command substitution such
as H="$(md5sum f)", a single-pipe fallback that should still be flagged, and an
exemption comment placed on the adjacent line rather than the command line.
Ensure each case asserts the intended scanner result and preserves the existing
exemption-with-reason behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08ce7f70-632c-4f9c-a0c2-b8cf161ddb03
📒 Files selected for processing (6)
.githooks/pre-commit.github/workflows/validate-codex.ymlAGENTS.mdREADME.mdscripts/check-shell-portability.mjsscripts/check-shell-portability.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/validate-codex.yml
| if (c === '"' || c === "'") { | ||
| open = true; hadQuote = true | ||
| const q = c | ||
| for (i++; i < line.length && line[i] !== q; i++) { | ||
| if (q === '"' && line[i] === '\\' && i + 1 < line.length) i++ | ||
| text += line[i] | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
큰따옴표 안의 명령 치환("$( … )")이 토큰화되지 않아 탐지를 우회합니다.
Line 111-118은 큰따옴표 내부를 통째로 하나의 quoted 토큰 텍스트로 흡수합니다. 그래서 echo "$(md5sum "$f")", n="$(nproc)", printf '%s' "$(timeout 5 x)" 같은 매우 흔한 형태에서 GNU 전용 명령이 command position 토큰으로 나타나지 않아 그냥 통과합니다(테스트는 unquoted H=$(md5sum "$a") 형태만 커버). 큰따옴표 안에서는 $( … )를 만나면 중첩 깊이를 세며 내부를 다시 토큰화해야 합니다.
🔍 재현
node -e '
const {execFileSync}=require("node:child_process");
' # 또는 테스트 러너에 케이스 추가:
# flags("md5sum inside a quoted command substitution", "H=\"$(md5sum \"$f\")\"")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-shell-portability.mjs` around lines 111 - 119, Update the
quoted-token parsing logic around the tokenizer’s quote-handling branch to
recognize `$(` inside double-quoted text, track nested command-substitution
depth through matching parentheses, and tokenize the substitution contents so
commands such as `md5sum`, `nproc`, and `timeout` remain detectable. Preserve
existing escaped-character handling and single-quote behavior, and ensure nested
quotes within the substitution are handled correctly.
| if (ESCAPE.test(near.comment)) { handled = true; break } | ||
| if (UNAME_BRANCH.test(near.code)) { handled = true; break } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
주변 줄의 exemption이 무관한 구문까지 해제합니다.
파일 상단 문서(Line 30)는 exemption을 "the line's comment"로 한정하지만, Line 300은 ±4줄 안의 어떤 # portability-ok: 주석이든 인정합니다. 즉 한 줄에 붙인 면제가 위아래 4줄의 서로 다른 GNU 구문까지 조용히 통과시킵니다. 같은 줄(Line 284에서 이미 처리)로 제한하고 Line 300은 제거하는 편이 문서와 일치합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-shell-portability.mjs` around lines 300 - 301, Update the
exemption handling in the surrounding portability-check logic so only a `#
portability-ok:` comment on the same line as the GNU construct is accepted,
matching the existing same-line handling near line 284. Remove the
nearby-comment exemption check represented by `ESCAPE.test(near.comment)` and
retain only the relevant `UNAME_BRANCH` handling.
| if (rule.bsdFn && k !== 0 && rule.bsdFn(near.tokens)) { handled = true; break } | ||
| if (!rule.bsd) continue | ||
| if (k === 0) { | ||
| // same line: the counterpart must sit past the `||` | ||
| const idx = near.tokens.findIndex((t) => t.op === true && t.bare === '|') | ||
| if (idx !== -1 && hasBsdToken(near.tokens.slice(idx), rule.bsd)) handled = true | ||
| continue | ||
| } | ||
| if (hasBsdToken(near.tokens, rule.bsd)) handled = true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
같은 줄 처리 두 가지가 어긋납니다.
- Line 306:
lex는||를|op 토큰 두 개로 만들고 여기서는 첫 번째|만 찾으므로 단순 파이프md5sum f | md5도 폴백으로 인정됩니다. 연속된|두 개인지 확인하세요. - Line 302:
k !== 0때문에bsdFn규칙(sed -i)은 같은 줄 폴백을 전혀 인정하지 못해,sed -i … || sed -i '' …한 줄 관용구가 오탐으로 잡힙니다(rule.bsd가 없어 Line 303에서 탈락).
🐛 제안
- if (rule.bsdFn && k !== 0 && rule.bsdFn(near.tokens)) { handled = true; break }
+ if (rule.bsdFn) {
+ if (k !== 0 && rule.bsdFn(near.tokens)) { handled = true; break }
+ if (k === 0) {
+ const or = orIndex(near.tokens)
+ if (or !== -1 && rule.bsdFn(near.tokens.slice(or))) { handled = true; break }
+ }
+ }
if (!rule.bsd) continue
if (k === 0) {
// same line: the counterpart must sit past the `||`
- const idx = near.tokens.findIndex((t) => t.op === true && t.bare === '|')
+ const idx = orIndex(near.tokens)
if (idx !== -1 && hasBsdToken(near.tokens.slice(idx), rule.bsd)) handled = true
continue
}// `||` is two adjacent `|` op tokens; a single pipe is not a fallback.
function orIndex(tokens) {
for (let i = 0; i + 1 < tokens.length; i++) {
if (tokens[i].op && tokens[i].bare === '|' && tokens[i + 1].op && tokens[i + 1].bare === '|') return i + 2
}
return -1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (rule.bsdFn && k !== 0 && rule.bsdFn(near.tokens)) { handled = true; break } | |
| if (!rule.bsd) continue | |
| if (k === 0) { | |
| // same line: the counterpart must sit past the `||` | |
| const idx = near.tokens.findIndex((t) => t.op === true && t.bare === '|') | |
| if (idx !== -1 && hasBsdToken(near.tokens.slice(idx), rule.bsd)) handled = true | |
| continue | |
| } | |
| if (hasBsdToken(near.tokens, rule.bsd)) handled = true | |
| if (rule.bsdFn) { | |
| if (k !== 0 && rule.bsdFn(near.tokens)) { handled = true; break } | |
| if (k === 0) { | |
| const or = orIndex(near.tokens) | |
| if (or !== -1 && rule.bsdFn(near.tokens.slice(or))) { handled = true; break } | |
| } | |
| } | |
| if (!rule.bsd) continue | |
| if (k === 0) { | |
| // same line: the counterpart must sit past the `||` | |
| const idx = orIndex(near.tokens) | |
| if (idx !== -1 && hasBsdToken(near.tokens.slice(idx), rule.bsd)) handled = true | |
| continue | |
| } | |
| if (hasBsdToken(near.tokens, rule.bsd)) handled = true | |
| // `||` is two adjacent `|` op tokens; a single pipe is not a fallback. | |
| function orIndex(tokens) { | |
| for (let i = 0; i + 1 < tokens.length; i++) { | |
| if (tokens[i].op && tokens[i].bare === '|' && tokens[i + 1].op && tokens[i + 1].bare === '|') return i + 2 | |
| } | |
| return -1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-shell-portability.mjs` around lines 302 - 310, Update the
same-line handling in the rule-processing loop to recognize only two adjacent
`|` operator tokens as the `||` fallback, preferably via a shared helper such as
`orIndex`, so a single pipe is not accepted. Also allow `rule.bsdFn` checks when
`k === 0` by applying the function to tokens after the detected fallback
operator, preserving existing behavior for later lines.
1 participant
PR #187 의 마지막 커밋(06:17) 이후 5분 뒤 CodeRabbit 5건과 Codex 리뷰가 도착했고, PR 은 다음 날 그 6건이 미처리인 채 머지됐다. 이 PR 이 그 후속이다.
5건 전부 현재 main 에 대해 재현했다. 판정은 현재 코드 기준이다 — #187 이후 #189(council) 가 들어와 파일이 바뀌었을 수 있어서, 리뷰 당시 스냅샷이 아니라 지금 트리에 대해 다시 확인했다.
가드가 통째로 못 보던 네 부류
각 항목은 워크트리에 재주입해 가드가
OK를 내는 것을 눈으로 확인한 뒤 고쳤다.1. GNU 긴 옵션 (CR 🟠 Major + Codex P1 — 동일 건)
date --date=·stat --format=·grep --perl-regexp·sed --in-place는 짧은 플래그와 같은 GNU 기능인데, 모든 규칙이 단일 대시 철자만 매치하고 있었다.grep --perl-regexp는grep -oP가 옷만 바꾼 것이고, 이 저장소는 BSD 노트를 그 함정으로 시작한다(plugins/llm-wiki/CLAUDE.md).2. 확장자 없는 셸 진입점 (CR 🟠 Major)
선택이
*.sh·*.bash·*.md였으므로.githooks/pre-commit— bash 이고, 확장자가 없고, 이 가드를 실행하는 훅 — 이 한 번도 스캔되지 않았다. tracked 파일 중 shebang 이 sh/bash 인 것을 추가로 발견하도록 했다.가드를 실행하는 파일이 가드의 사각지대였다는 게 이 건의 핵심이다.
3. 가짜 폴백 (CR 🟠 Major, "Heavy lift")
||가 finding 을 지웠다 →md5sum "$f" || exit 1이 포터블로 읽혔다. macOS 에서 여전히 깨지는데도.sed --version >/dev/null뒤의 분기 없는sed -i가 통과했다.||는 우변에 BSD 대응물이 있을 때만 폴백으로 인정하고, probe 는 대응물이 실제로 근처에 있을 때만 인정하도록 좁혔다. BSD 대응물이 아예 없는 규칙(grep -P·timeout· bash 4 문법)은 가리킬 포터블 대안이 없으므로||나 probe 로 지워지지 않고 명시적 예외만 받는다.CR 은 이걸 "Heavy lift" 로 분류했지만 실제로는 수용 조건을 좁히는 작업이라 로직이 단순해졌다.
4. 사유 없이 통과하는 예외 (CR 🟡 Minor)
두 문서 모두 계약을
# portability-ok: <사유>로 명시하는데ESCAPE는 접두사만 매치했다. 즉 불일치는 문서가 아니라 코드 쪽이었다. 콜론과 비어 있지 않은 사유를 요구하도록 했다. 저장소에 이 마커를 쓰는 코드가 없어서 강화가 기존 코드를 깨지 않는다.정당화를 요구하지 않는 예외 구멍은 가장 먼저 썩는다.
검증 — 양방향
재주입 결과 8/8 검출:
같은 실행에서 대조군은 전부 통과했다 —
stat -c … || stat -f …,sha256sum || shasum -a 256,date -d/date -j쌍,nproc || sysctl -n hw.ncpu, 사유가 붙은 정당한 예외. 좁힌 뒤에도 현재 저장소 249 파일에서 오탐 0.별건: 진단 스텝의 종료 상태 마스킹 (CR 🟡 Minor)
macOS 진단 스텝이
echo "$(...)"로 출력하고 있었다. command substitution 안의 상태는 버려지고echo는 언제나 성공하므로, 이 저장소가 #183 에서 추가한 "종료 상태가 사라지는 자리" P1 을 워크플로 자신이 위반하고 있었다. 규칙을 강제하는 PR 이 그 규칙을 어기는 예시를 싣고 있으면 곤란하다.직접 출력으로 바꿨다. 스텝 셸이
bash -e라 정말로 bash 가 없으면 빈 줄 대신 스텝이 실패한다. 도구 루프의|| echo '(absent)'는 그대로 뒀다 — 거기서는 부재가 곧 답이다.검증 결과
sync-codex-manifests.mjs --checksync-hermes-manifests.mjs --checkcheck-doc-consistency.mjscheck-skill-tool-portability.mjs --checkcheck-shell-portability.mjsmock-load-hermes.pycr-fix/tests/run-tests.shcouncil/convene/tests/run-tests.shbash -e)(absent)플러그인 버전 범프 없음 — 루트 문서 · CI ·
scripts/만 건드리므로AGENTS.md규칙상 플러그인 콘텐츠가 아니다.Summary by CodeRabbit
sh/bashshebangs.