diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 91a9cd62..c00cebe0 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -20,6 +20,11 @@ if command -v node >/dev/null 2>&1; then # has forbidden these for a while with nothing enforcing it, which is how # md5sum reached translator and broke image downloads on macOS (issue #172). node "$repo_root/scripts/check-shell-portability.mjs" + # The portability guard is a detector, and this repo's own lore says a + # detector without coverage reports "nothing wrong" when it means "could + # not look". Nine holes across three review rounds, each found by hand; + # these cases are that verification kept. + node "$repo_root/scripts/check-shell-portability.test.mjs" # Skill-prose progressive-disclosure smell detector — INFORMATIONAL ONLY. It exits 0 # regardless, but the `|| true` keeps it non-blocking even if node itself errors. node "$repo_root/scripts/check-skill-prose.mjs" || true diff --git a/.github/workflows/validate-codex.yml b/.github/workflows/validate-codex.yml index 970e6f8d..e5f42c2c 100644 --- a/.github/workflows/validate-codex.yml +++ b/.github/workflows/validate-codex.yml @@ -51,6 +51,10 @@ jobs: # downloaded image to one filename on macOS (issue #172). Flags a construct # only when no fallback, capability probe, or `# portability-ok:` sits near it. - run: node scripts/check-shell-portability.mjs + # 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 it gets switched off. + - run: node scripts/check-shell-portability.test.mjs # The BSD fallbacks the suite is full of can only be proven on a BSD userland. # Without this leg every `stat -c || stat -f` and `date -d || date -j` branch is @@ -61,11 +65,26 @@ jobs: - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 with: persist-credentials: false + # Printed directly rather than through `echo "$(...)"`. Command substitution + # inside echo discards the inner status -- echo succeeds either way -- which + # is the "종료 상태가 사라지는 자리" P1 this repo added in #183, and a workflow + # that violates its own rule is a bad exemplar. The step's shell is `bash -e`, + # so a genuinely missing bash now fails here instead of printing a blank line. + # The loop keeps its `|| echo '(absent)'`: absence is the answer it is asking. - name: runner userland (informational) run: | - echo "bash on PATH: $(which -a bash | tr '\n' ' ')" - echo "PATH bash: $(bash --version | head -1)" - echo "/bin/bash: $(/bin/bash --version | head -1)" + # The step shell is `bash -e`, which does NOT set pipefail: a failing + # `/bin/bash --version` was masked by `head`'s success and the step + # still passed -- the same false green this block was rewritten to + # remove, one layer down. Verified: `bash -e -c '/missing/bash + # --version | head -1; echo survived'` exits 0. + set -o pipefail + echo "bash on PATH:" + which -a bash + echo "PATH bash:" + bash --version | head -1 + echo "/bin/bash:" + /bin/bash --version | head -1 for t in sed grep date stat jq md5sum timeout gsed gdate gstat; do printf '%-8s %s\n' "$t" "$(command -v "$t" || echo '(absent)')" done diff --git a/AGENTS.md b/AGENTS.md index 7318c7e8..a00ec919 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -262,7 +262,7 @@ node scripts/check-shell-portability.mjs `check-doc-consistency.mjs` 는 README 구조 트리·`## Plugins` 표·문서에 박힌 카운트 문자열을 marketplace.json 과 대조하고 `.githooks/pre-commit` 에서 차단한다. 문서 블록이 관례가 아니라 기계적으로 강제된다는 뜻이므로, 어떤 문서 내용을 "코드에서 재생성 가능하니 지워도 된다" 고 판단하기 전에 훅이 그 블록을 요구하는지 먼저 확인한다. 가드 6종 전체 설명은 `README.md` 의 "CI 가드가 지키는 것". -`check-shell-portability.mjs` 는 `code_review.md` P1 의 크로스플랫폼 규칙을 기계적으로 강제한다 — GNU 전용 구문(`md5sum`·`sed -i`·`grep -P`·`date -d`·`stat -c`·`timeout`·`${VAR,,}`·`mapfile`·`declare -A` 등)이 **폴백도 capability probe 도 없이** 쓰인 경우만 잡는다. 정상 폴백 쌍(`stat -c … || stat -f …`)과 probe 분기는 통과하며, 증거는 **코드여야 하고 주석은 인정하지 않는다** (대체재를 언급하는 주석이 실제 폴백으로 계수되면 이 가드가 존재하는 이유인 `md5sum` 자체가 새어나간다). 정말 예외인 줄은 `# portability-ok: <사유>` 로 표시한다. `.llmwiki/`·`.claude/spec/`·`code_review.md`·`AGENTS.md` 는 그 구문을 *설명*할 뿐이라 스캔에서 제외된다. +`check-shell-portability.mjs` 는 `code_review.md` P1 의 크로스플랫폼 규칙을 기계적으로 강제한다 — GNU 전용 구문(`md5sum`·`sed -i`·`grep -P`·`date -d`·`stat -c`·`timeout`·`${VAR,,}`·`mapfile`·`declare -A` 등)이 **폴백도 capability probe 도 없이** 쓰인 경우만 잡는다. 정상 폴백 쌍(`stat -c … || stat -f …`)은 통과한다 — 단 **`||` 는 우변에 BSD 대응물이 있을 때만** 폴백으로 인정한다 (`md5sum "$f" || exit 1` 은 여전히 macOS 에서 깨지므로 통과시키지 않는다). capability probe 도 **대응물과 짝일 때만** 인정한다 — 짝 없는 `sed --version` 뒤의 `sed -i` 는 분기가 없으므로 잡는다. BSD 대응물이 아예 없는 구문(`grep -P`·`timeout`·bash 4 문법 등)은 가리킬 포터블 대안이 없으므로 `||` 나 probe 로 지워지지 않고 명시적 예외만 받는다. GNU 긴 옵션 별칭(`--perl-regexp`·`--date`·`--format`·`--in-place`)도 같은 규칙으로 잡는다. 증거는 **코드여야 하고 주석은 인정하지 않는다** (대체재를 언급하는 주석이 실제 폴백으로 계수되면 이 가드가 존재하는 이유인 `md5sum` 자체가 새어나간다). 정말 예외인 줄은 `# portability-ok: <사유>` 로 표시한다 — 사유는 필수이고, 콜론 뒤가 비면 예외로 인정하지 않는다. 판정은 **정규식이 아니라 셸 워드 토크나이저**로 한다: 옵션 문법을 정규식으로 흉내내면 새 철자(`-oP` → `-Pio` → `--perl-regexp` → `--color=always -P` → `-n --perl-regexp`)가 계속 새어 나가고, 토크나이저는 따옴표 인식이 딸려 와 **인용 문자열이 증거로 계수되는 것**(`printf 'md5'` 를 BSD 대안으로, `printf '# portability-ok: …'` 를 예외로 읽는 것)까지 같이 막는다. 명령 위치도 구분하므로 `command -v timeout` 의 `timeout` 은 인자이지 호출이 아니고, `case` 패턴의 `iteration_cap|timeout|…)` 도 실행이 아니다. 회귀 케이스는 `scripts/check-shell-portability.test.mjs` 30건이며 pre-commit + CI 양쪽에서 돈다 — 가드 자체가 detector 라 커버리지 없이 두면 "볼 수 없었다" 를 "문제 없다" 로 보고한다. 스캔 대상은 `*.sh`/`*.bash`/`*.md` 의 fenced bash 블록에 더해 **shebang 이 sh/bash 인 확장자 없는 tracked 파일**까지다 (`.githooks/pre-commit` 이 그것이고, 이 가드를 실행하는 파일 자신이 사각지대였다). `.llmwiki/`·`.claude/spec/`·`code_review.md`·`AGENTS.md` 는 그 구문을 *설명*할 뿐이라 스캔에서 제외된다. macOS CI 레그(`validate-codex.yml` 의 `macos` job)는 BSD 폴백이 실제로 실행되는 유일한 지점이다. `env -i PATH=/usr/bin:/bin` 는 쓰지 않는다 — macOS 에서 `jq` 가 Homebrew 경로에 있어 스위트가 도구 부재로 죽는다. 대신 `sed`/`date`/`stat` 이 `--version` 을 거부하는지(=BSD 빌드인지) assert 하고, Homebrew coreutils 가 시스템 도구를 가리면 시끄럽게 실패시킨다. 스위트는 `bash` 가 아니라 `/bin/bash` 로 돌려 bash 3.2 를 강제한다 (러너 PATH 의 Homebrew bash 5 로 돌면 bash-4 전용 구문이 여기서 통과하고 사용자에게서 깨진다). diff --git a/README.md b/README.md index 53dfeb84..49f45594 100644 --- a/README.md +++ b/README.md @@ -759,7 +759,7 @@ shared-source 배선은 6개 가드가 매 PR 과 매 커밋(`.githooks/pre-comm - `sync-hermes-manifests.mjs --check` — Hermes 어댑터 drift + orphan. - `check-doc-consistency.mjs` — 플러그인 트리·표·카운트(총 25 / Codex-eligible 23 / Hermes 7)가 `manifest-eligibility.mjs` SoT 와 일치. - `check-skill-tool-portability.mjs --check` — 공유 스킬 본문의 `AskUserQuestion` 사용이 파일럿 표준 매핑 또는 baseline 에 등록됐는지(미등록 크로스런타임 상호작용 경로 차단). -- `check-shell-portability.mjs` — GNU 전용 셸 구문(`md5sum`·`sed -i`·`grep -P`·`date -d`·`stat -c`·`timeout`·`${VAR,,}`·`mapfile`·`declare -A` 등)이 **폴백도 capability probe 도 없이** 쓰인 경우 차단. 정상 폴백 쌍(`stat -c … || stat -f …`)과 probe 분기는 통과하고, 증거는 코드만 인정합니다(대체재를 언급하는 주석은 폴백이 아님). 예외는 `# portability-ok: <사유>`. +- `check-shell-portability.mjs` — GNU 전용 셸 구문(`md5sum`·`sed -i`·`grep -P`·`date -d`·`stat -c`·`timeout`·`${VAR,,}`·`mapfile`·`declare -A` 등)이 **폴백도 capability probe 도 없이** 쓰인 경우 차단. 정상 폴백 쌍(`stat -c … || stat -f …`)은 통과하되, `||` 는 **우변에 BSD 대응물이 있을 때만** 폴백으로 봅니다 (`md5sum "$f" || exit 1` 은 통과 못 함). probe 도 대응물과 짝일 때만 인정하고, BSD 대응물이 없는 구문(`grep -P`·`timeout`·bash 4 문법)은 명시적 예외만 받습니다. GNU 긴 옵션(`--perl-regexp`·`--date`·`--format`·`--in-place`)도 동일하게 잡습니다. 증거는 코드만 인정합니다(대체재를 언급하는 주석은 폴백이 아님). 예외는 `# portability-ok: <사유>` — 사유 필수. 판정은 정규식이 아니라 셸 워드 토크나이저로 하므로 옵션 철자(`-Pio`·`--perl-regexp`·`--color=always -P`·`-n --perl-regexp`)를 모두 잡고, 인용 문자열은 증거로 세지 않으며(`printf 'md5'` 는 BSD 대안이 아님), `command -v timeout` 처럼 이름만 언급한 자리와 `case` 패턴은 호출로 보지 않습니다. 스캔 대상에 shebang 이 sh/bash 인 확장자 없는 파일(`.githooks/pre-commit`)도 포함됩니다. 회귀 케이스 30건(`check-shell-portability.test.mjs`)이 pre-commit + CI 에서 함께 돕니다. - `check-skill-prose.mjs` — 500줄 초과·깊은 참조 경로에 대한 정보성 경고(비차단, 항상 exit 0). 가드와 별개로 두 개의 픽스처 스위트가 같은 자리에서 돕니다 — `plugins/github-dev/skills/cr-fix/tests/run-tests.sh` (1차 경로가 실패한 뒤에만 실행되는 CLI 폴백·CR 상태 경로)와 `plugins/council/skills/convene/tests/run-tests.sh` (스킬 본문에만 존재해 아무도 실행하지 않는 codex/agy 호출 계약 + 레지스트리 TTL 산술). 후자가 지키는 것은 이식성 가드가 볼 수 없는 종류입니다 — agy 호출에서 `< /dev/null` 이 빠지면 그 의석이 영원히 멈추는데, 그건 GNU 전용 구문이 아니라서 `check-shell-portability` 의 관심사가 아닙니다. diff --git a/scripts/check-shell-portability.mjs b/scripts/check-shell-portability.mjs index 3d756805..44e551fe 100644 --- a/scripts/check-shell-portability.mjs +++ b/scripts/check-shell-portability.mjs @@ -8,75 +8,245 @@ // This exists because code_review.md P1 has forbidden these constructs for a // while and nothing enforced it: `md5sum` shipped in translator's // download_image.sh and silently collapsed every image to one filename on -// macOS (issue #172). A grep denylist would have caught it in a second. +// macOS (issue #172). A denylist would have caught it in a second. // -// The hard part is NOT finding the tokens, it is not crying wolf. This repo is -// full of CORRECT uses -- `stat -c %Y f || stat -f %m f`, `sha256sum || shasum -// -a 256`, `sed -i` inside a `sed --version` probe branch -- and a guard that -// flags those gets switched off within a week. So a token only counts as a -// finding when nothing nearby makes it portable: +// It scans SHELL WORDS, not raw text. The first version matched regexes against +// the line, and every review round produced another spelling it had not +// anticipated -- `grep -oP`, then `-Pio`, then `--perl-regexp`, then +// `--color=always -P`, then `-n --perl-regexp`. Emulating option syntax with +// regexes is a losing game; tokenizing ends it. Quote awareness comes free with +// the tokenizer, which is what stops a STRING like `printf 'md5'` from counting +// as a BSD alternative. // -// - a `||` alternative on the same line -// - a capability probe (`command -v x`, `x --version`, `sort -V ` escape hatch +// The hard part is not finding the constructs, it is not crying wolf. This repo +// is full of correct uses -- `stat -c %Y f || stat -f %m f`, `sha256sum || +// shasum -a 256`, `sed -i` inside a `sed --version` probe branch -- and a guard +// that flags those gets switched off in a week. A construct counts only when +// nothing nearby makes it portable: +// +// - the BSD counterpart on the far side of a same-line `||` +// - the BSD counterpart on a nearby line (in code, never in a comment) +// - a `case "$(uname ...)"` branch, which covers any construct +// - an explicit `# portability-ok: ` in the line's comment +// +// A construct with no BSD counterpart at all (grep -P, timeout, bash 4 syntax) +// has no portable alternative to point at, so only the exemption clears it. // // Node 18 builtins only, per the repo's generator convention. import { execFileSync } from 'node:child_process' import { readFileSync } from 'node:fs' -// token: a regex matching the GNU-only construct -// alt: the POSIX / portable replacement named in the failure message -// probe: extra regex whose presence nearby proves the author branched on it -const RULES = [ - { id: 'md5sum', tool: 'md5sum', token: /(^|[\s|(`$])md5sum\b/, bsd: /\b(cksum|md5)\b/, alt: 'cksum (POSIX) or `md5sum || md5`' }, - { id: 'sha256sum', tool: 'sha256sum', token: /(^|[\s|(`$])sha256sum\b/, bsd: /\bshasum\b/, alt: '`sha256sum || shasum -a 256`' }, - { id: 'sed -i', tool: 'sed', token: /\bsed\s+(-[a-zA-Z]*\s+)*-i(\s|$)(?!\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|$)/, 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|$)/, alt: "sed -n 's/…/\\1/p' or awk (BSD grep has no PCRE at all)" }, - { id: 'date -d', tool: 'date', token: /\bdate\s+(-[a-zA-Z]+\s+)*-[a-zA-Z]*d[a-zA-Z]*(\s|=)/, bsd: /\bdate\s+-j\b/, alt: '`date -d … || date -j -f FMT …`' }, - { 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: 'timeout', tool: 'timeout', token: /(^|[\s|(`$])timeout\s+\d/, alt: 'a bash watchdog (see cr-fix tests run_capped) -- stock macOS has no timeout(1)' }, - { id: 'tac', tool: 'tac', token: /(^|[\s|(`$])tac(\s|$)/, bsd: /\btail\s+-r\b/, alt: 'tail -r' }, - { id: 'nproc', tool: 'nproc', token: /(^|[\s|(`$])nproc(\s|$)/, bsd: /sysctl\s+-n\s+hw\.ncpu/, alt: '`nproc || sysctl -n hw.ncpu`' }, - { id: 'realpath -m', tool: 'realpath', token: /\brealpath\s+(-[a-zA-Z]+\s+)*-m(\s|$)/, alt: 'cd + pwd -P on the parent, then re-append the basename (see cr-fix path-trust.sh)' }, - { id: 'readlink -f', tool: 'readlink', token: /\breadlink\s+(-[a-zA-Z]+\s+)*-f(\s|$)/, alt: 'a readlink loop, or guard on macOS >= 12.3' }, - { id: 'bash4-case', tool: 'bash', token: /\$\{[A-Za-z_][A-Za-z0-9_]*(,,|\^\^)\}/, alt: "tr '[:upper:]' '[:lower:]' -- macOS /bin/bash is 3.2" }, - { id: 'bash4-mapfile', tool: 'bash', token: /(^|[\s;&|(])(mapfile|readarray)(\s|$)/, alt: 'a while read -r loop -- mapfile is bash 4+' }, - { id: 'bash4-assoc', tool: 'bash', token: /\bdeclare\s+(-[a-zA-Z]+\s+)*-A(\s|$)/, alt: 'parallel indexed arrays -- associative arrays are bash 4+' }, - { id: 'bash4-nameref', tool: 'bash', token: /\b(declare|local)\s+(-[a-zA-Z]+\s+)*-n(\s|$)/, alt: 'pass the value, not the name -- namerefs are bash 4.3+' }, -].map((r) => ({ ...r, probe: toolProbe(r.tool) })) - -// A probe or an alternative anywhere in this window proves the author handled it. -// A GNU-first idiom often puts its BSD counterpart on the NEXT lines -// (`ts=$(date -d …) && return; ts=$(date -j -f …) && return`), so look both ways. -const WINDOW = 4 -// A probe must name the tool it is probing. A generic /command -v/ let an -// UNRELATED probe nearby clear a finding: `command -v node || exit` two lines -// above an `md5sum` call marked the md5sum handled. `uname` branching is the one -// probe that legitimately covers any tool, so it stays generic. -const GENERIC_PROBE = /\bcase\s+"?\$\(uname|\buname\s+-s\b/ -function toolProbe(tool) { - const t = tool.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') - return new RegExp(`(command\\s+-v\\s+${t}|type\\s+-P\\s+${t}|\\b${t}\\s+(-[a-zA-Z]+\\s+)*--version|${t}\\s+-V\\s*<\\s*/dev/null)`) -} -const ESCAPE = /#\s*portability-ok/ +// --- rules ----------------------------------------------------------------- +// Command rules name the command plus the GNU-only flag in both spellings. +// `bare: true` means the command itself is the problem, flags irrelevant. +// `bsd` lists the tokens that, when present nearby, prove a fallback exists. +const CMD_RULES = [ + { id: 'md5sum', cmd: 'md5sum', bare: true, bsd: ['cksum', 'md5'], + alt: 'cksum (POSIX) or `md5sum || md5`' }, + { id: 'sha256sum', cmd: 'sha256sum', bare: true, bsd: ['shasum'], + alt: '`sha256sum || shasum -a 256`' }, + { id: 'timeout', cmd: 'timeout', bare: true, + alt: 'a bash watchdog (see cr-fix tests run_capped) -- stock macOS has no timeout(1)' }, + { id: 'tac', cmd: 'tac', bare: true, bsd: ['tail'], + alt: 'tail -r' }, + { id: 'nproc', cmd: 'nproc', bare: true, bsd: ['sysctl'], + alt: '`nproc || sysctl -n hw.ncpu`' }, + { id: 'sed -i', cmd: 'sed', short: 'i', long: ['--in-place'], sedInPlace: true, bsdFn: sedBsdForm, + alt: "a sed_inplace() helper branching on `sed --version` (GNU takes `-i`, BSD takes `-i ''`)" }, + { id: 'sed -r', cmd: 'sed', short: 'r', long: ['--regexp-extended'], + alt: 'sed -E (accepted by both GNU and BSD)' }, + { id: 'grep -P', cmd: 'grep', short: 'P', long: ['--perl-regexp'], + alt: "sed -n 's/…/\\1/p' or awk (BSD grep has no PCRE at all)" }, + { id: 'date -d', cmd: 'date', short: 'd', long: ['--date'], bsd: ['-j'], + alt: '`date -d … || date -j -f FMT …`' }, + { id: 'stat -c', cmd: 'stat', short: 'c', long: ['--format', '--printf'], bsd: ['-f'], + alt: '`stat -c … || stat -f …`' }, + { id: 'realpath -m', cmd: 'realpath', short: 'm', long: ['--canonicalize-missing'], + alt: 'cd + pwd -P on the parent, then re-append the basename (see cr-fix path-trust.sh)' }, + { id: 'readlink -f', cmd: 'readlink', short: 'f', long: ['--canonicalize'], + alt: 'a readlink loop, or guard on macOS >= 12.3' }, +] -// Rule text and lore DESCRIBE these constructs; they never run them. -const SKIP_FILES = [ - 'code_review.md', - 'AGENTS.md', - 'scripts/check-shell-portability.mjs', +// Syntax rules are not commands; they match the unquoted text of a line. +const SYNTAX_RULES = [ + { id: 'bash4-case', re: /\$\{[A-Za-z_][A-Za-z0-9_]*(,,|\^\^)\}/, + alt: "tr '[:upper:]' '[:lower:]' -- macOS /bin/bash is 3.2" }, + { id: 'bash4-mapfile', re: /(^|[\s;&|(])(mapfile|readarray)(\s|$)/, + alt: 'a while read -r loop -- mapfile is bash 4+' }, + { id: 'bash4-assoc', re: /\bdeclare\s+(-[a-zA-Z]+\s+)*-A(\s|$)/, + alt: 'parallel indexed arrays -- associative arrays are bash 4+' }, + { id: 'bash4-nameref', re: /\b(declare|local)\s+(-[a-zA-Z]+\s+)*-n(\s|$)/, + alt: 'pass the value, not the name -- namerefs are bash 4.3+' }, ] + +const WINDOW = 4 +// Only a real branch counts. A bare `uname -s` is often just logging, and +// accepting it let `printf '%s\n' "$(uname -s)"` clear every construct within +// four lines of it. +const UNAME_BRANCH = /\bcase\s+"?\$\((uname|\s*uname)/ +const ESCAPE = /#\s*portability-ok:\s*\S/ + +const SKIP_FILES = ['code_review.md', 'AGENTS.md', 'scripts/check-shell-portability.mjs'] const SKIP_PREFIXES = ['.llmwiki/', '.claude/spec/', 'docs/superpowers/'] +// --- tokenizer ------------------------------------------------------------- +// Splits a shell line into words, tracking which characters were quoted. +// `code` is the unquoted-only text (what a rule may match against) and +// `comment` is the trailing unquoted `#` remainder (where an exemption lives). +function lex(line) { + const tokens = [] + let text = '', bare = '', hadQuote = false, open = false + let i = 0 + const flush = () => { + if (!open) return + tokens.push({ text, bare, hadQuote, empty: hadQuote && text === '' }) + text = ''; bare = ''; hadQuote = false; open = false + } + for (; i < line.length; i++) { + const c = line[i] + if (!open && /\s/.test(c)) continue + if (open && /\s/.test(c)) { flush(); continue } + if (c === '#' && !open) break // comment starts a new word + 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 + } + if (';&|()'.includes(c)) { flush(); tokens.push({ text: c, bare: c, op: true }); continue } + open = true; text += c; bare += c + } + flush() + const hash = unquotedHash(line) + return { tokens, code: line.slice(0, hash === -1 ? line.length : hash), comment: hash === -1 ? '' : line.slice(hash) } +} + +// Index of the `#` that opens a comment, skipping quoted ones. +function unquotedHash(line) { + let q = null + for (let i = 0; i < line.length; i++) { + const c = line[i] + if (q) { if (c === q) q = null; continue } + if (c === '"' || c === "'") { q = c; continue } + if (c === '#' && (i === 0 || /\s/.test(line[i - 1]))) return i + } + return -1 +} + +// `sed -i ''` -- the BSD spelling. Used as the nearby-counterpart test for the +// sed -i rule, whose counterpart is a call SHAPE rather than a distinct token. +function sedBsdForm(tokens) { + for (let i = 0; i < tokens.length; i++) { + if (tokens[i].op || tokens[i].hadQuote || tokens[i].bare !== 'sed') continue + for (let j = i + 1; j < tokens.length; j++) { + const a = tokens[j] + if (a.op) break + if (a.bare === '-i' || a.bare === '--in-place') { + const next = tokens[j + 1] + if (next && next.hadQuote && next.bare === '' && next.text === '') return true + } + } + } + return false +} + +// A `case` branch lists patterns, it does not run them: `iteration_cap|timeout|x)` +// names timeout, it never invokes it. The shape is an unmatched `)` closing a +// pattern list, optionally followed by `;;`. +function isCasePattern(code) { + const opens = (code.match(/\(/g) || []).length + const closes = (code.match(/\)/g) || []).length + return closes > opens && /\)\s*($|[^(]*;;)/.test(code) +} + +// Which token indexes sit in command position? A command name is the first word +// of a simple command -- after a separator, or after `VAR=value` prefixes. This +// is what separates INVOKING a command from NAMING it: in `command -v timeout`, +// timeout is an argument, and the old regex version only tolerated it because it +// happened to read `command -v` as a capability probe. +function commandPositions(tokens) { + const pos = new Set() + let atCmd = true + for (let i = 0; i < tokens.length; i++) { + const t = tokens[i] + 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 + } + return pos +} + +// Does `tokens` invoke rule.cmd with the GNU-only flag (or bare, for bare rules)? +function cmdUsed(tokens, rule, cmdPos) { + for (let i = 0; i < tokens.length; i++) { + const t = tokens[i] + if (t.op || t.hadQuote || t.bare !== rule.cmd) continue + if (!cmdPos.has(i)) continue + if (rule.bare) return true + for (let j = i + 1; j < tokens.length; j++) { + const a = tokens[j] + if (a.op) break // command boundary + if (a.bare === '--') break // end of options + if (a.hadQuote && a.bare === '') { // e.g. the `''` in BSD `sed -i ''` + continue + } + if (a.bare.startsWith('--')) { + if (rule.long?.includes(a.bare.split('=')[0])) return true + continue + } + if (a.bare.startsWith('-') && a.bare.length > 1) { + if (!rule.short || !a.bare.slice(1).includes(rule.short)) continue + if (!rule.sedInPlace) return true + // `sed -i ''` is the BSD spelling; `-i` alone (or `-i''`, which the shell + // collapses to `-i`) is the GNU one. An attached extension (`-i.bak`) + // is accepted by both. + 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 + return true + } + } + } + return false +} + +// Is a counterpart present as an unquoted token? Two things this must NOT do: +// a quoted `'md5'` is a string, not a call; and the match is exact, never a +// prefix -- `'md5sum'.startsWith('md5')` made every md5sum line count as the BSD +// fallback for the md5sum line next to it, so the construct cleared itself. +function hasBsdToken(tokens, bsd) { + return tokens.some((t) => !t.hadQuote && bsd.includes(t.bare)) +} + +// --- file discovery -------------------------------------------------------- function tracked(patterns) { return execFileSync('git', ['ls-files', ...patterns], { encoding: 'utf8' }) .split('\n').filter(Boolean) } +// Extension is not what makes a file shell. `.githooks/pre-commit` is bash with +// no suffix -- and it is the hook that RUNS this guard, so leaving it unscanned +// made the guard blind to the file that invokes it. Restricted to sh/bash to +// match what the rules actually encode; a ksh or zsh script would need its own +// dialect rules before it could be judged here. +function shebangShellFiles() { + const out = [] + for (const f of tracked([])) { + if (/\.(sh|bash|md)$/.test(f)) continue + let head + try { head = readFileSync(f, 'utf8').slice(0, 120).split('\n')[0] } catch { continue } + if (/^#!.*\b(ba)?sh\b/.test(head)) out.push(f) + } + return out +} + // For .md, only fenced bash/sh blocks are executable; the rest is prose. function executableLines(path, text) { const out = [] @@ -98,19 +268,8 @@ function executableLines(path, text) { return out } -function stripComment(line) { - // Good enough for shell: drop from an unquoted # to end of line. - let q = null - for (let i = 0; i < line.length; i++) { - const c = line[i] - if (q) { if (c === q && line[i - 1] !== '\\') q = null; continue } - if (c === '"' || c === "'") { q = c; continue } - if (c === '#' && (i === 0 || /\s/.test(line[i - 1]))) return line.slice(0, i) - } - return line -} - -const files = tracked(['*.sh', '*.md', '*.bash']) +// --- scan ------------------------------------------------------------------ +const files = [...tracked(['*.sh', '*.md', '*.bash']), ...shebangShellFiles()] .filter((f) => !SKIP_FILES.includes(f) && !SKIP_PREFIXES.some((p) => f.startsWith(p))) const findings = [] @@ -118,33 +277,40 @@ for (const file of files) { let text try { text = readFileSync(file, 'utf8') } catch { continue } const lines = executableLines(file, text) - const byIndex = new Map(lines.map((l) => [l.n, l.text])) - for (const { n, text: raw } of lines) { - if (ESCAPE.test(raw)) continue - const code = stripComment(raw) - if (!code.trim()) continue - for (const rule of RULES) { - if (!rule.token.test(code)) continue - // same-line alternative - if (code.includes('||')) continue - // probe on this line or just above (comments included -- a `sed --version` - // probe is often the line before, and its own comment names the tool) - // Evidence has to be CODE. A comment that names the portable alternative - // ("POSIX cksum, not md5sum: ...") is documentation, not a fallback -- and - // counting it let the exact md5sum bug this guard exists for slip through. - // The escape hatch is the one thing that IS a comment, by design. + const lexed = new Map(lines.map((l) => [l.n, lex(l.text)])) + + for (const { n } of lines) { + const here = lexed.get(n) + if (ESCAPE.test(here.comment)) continue + if (!here.code.trim()) continue + + if (isCasePattern(here.code)) continue + + const cmdPos = commandPositions(here.tokens) + const hits = [] + for (const rule of CMD_RULES) if (cmdUsed(here.tokens, rule, cmdPos)) hits.push(rule) + for (const rule of SYNTAX_RULES) if (rule.re.test(here.code)) hits.push(rule) + if (hits.length === 0) continue + + for (const rule of hits) { let handled = false - for (let k = -WINDOW; k <= WINDOW; k++) { - const near = byIndex.get(n + k) - if (near === undefined) continue - if (ESCAPE.test(near)) { handled = true; break } - const nearCode = stripComment(near) - if (GENERIC_PROBE.test(nearCode)) { handled = true; break } - if (rule.probe.test(nearCode)) { handled = true; break } - if (rule.bsd && k !== 0 && rule.bsd.test(nearCode)) { handled = true; break } + for (let k = -WINDOW; k <= WINDOW && !handled; k++) { + const near = lexed.get(n + k) + if (!near) continue + if (ESCAPE.test(near.comment)) { handled = true; break } + if (UNAME_BRANCH.test(near.code)) { handled = true; break } + 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 (handled) continue - findings.push({ file, n, id: rule.id, alt: rule.alt, code: code.trim() }) + findings.push({ file, n, id: rule.id, alt: rule.alt, code: here.code.trim() }) } } } diff --git a/scripts/check-shell-portability.test.mjs b/scripts/check-shell-portability.test.mjs new file mode 100644 index 00000000..a925b3c7 --- /dev/null +++ b/scripts/check-shell-portability.test.mjs @@ -0,0 +1,112 @@ +#!/usr/bin/env node +// Regression cases for check-shell-portability.mjs. +// +// The guard has had nine holes found across three review rounds -- `grep -oP` +// caught but `-Pio` missed, then `--perl-regexp`, then `--color=always -P`, then +// `-n --perl-regexp`; a bare `||` accepted as a fallback; a quoted `'md5'` +// accepted as one; a logging `uname -s` accepted as a branch; `.githooks/` +// unscanned because it has no extension; and `md5sum` satisfying its own +// counterpart through prefix matching. Each round I re-injected the defects into +// a throwaway worktree by hand and read the output. This file is that, kept. +// +// Two directions, both of which matter: a construct that SHOULD be flagged, and +// correct code that must NOT be. The second half is what keeps the guard usable +// -- a guard that cries wolf on `stat -c … || stat -f …` gets switched off. +// +// Run: node scripts/check-shell-portability.test.mjs +// Node 18 builtins only, per the repo's generator convention. + +import { execFileSync } from 'node:child_process' +import { mkdtempSync, writeFileSync, rmSync, mkdirSync, cpSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, dirname } from 'node:path' +import { fileURLToPath } from 'node:url' + +const HERE = dirname(fileURLToPath(import.meta.url)) +const GUARD = join(HERE, 'check-shell-portability.mjs') +let pass = 0, fail = 0 +const ok = (m) => { pass++; console.log(` ok ${m}`) } +const bad = (m, d) => { fail++; console.log(` FAIL ${m}\n ${d}`) } + +// The guard reads `git ls-files`, so a case is a throwaway git repo holding one +// shell file. Scanning a real checkout instead would make the cases depend on +// whatever else the repo happens to contain that day. +function scan(body, name = 'case.sh', mode = 0o644) { + const dir = mkdtempSync(join(tmpdir(), 'shport-')) + try { + mkdirSync(join(dir, 'scripts'), { recursive: true }) + mkdirSync(join(dir, dirname(name)), { recursive: true }) + cpSync(GUARD, join(dir, 'scripts/check-shell-portability.mjs')) + writeFileSync(join(dir, name), body, { mode }) + const git = (...a) => execFileSync('git', ['-C', dir, ...a], { stdio: 'pipe' }) + git('init', '-q') + git('config', 'user.email', 't@t'); git('config', 'user.name', 't') + git('add', '-A') + let out = '', code = 0 + try { + out = execFileSync('node', ['scripts/check-shell-portability.mjs'], + { cwd: dir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }) + } catch (e) { code = e.status; out = (e.stdout || '') + (e.stderr || '') } + return { out, code } + } finally { rmSync(dir, { recursive: true, force: true }) } +} + +const flags = (m, body, opts = {}) => { + const r = scan(`#!/usr/bin/env bash\n${body}\n`, opts.name, opts.mode) + r.code === 1 ? ok(m) : bad(m, `expected exit 1, got ${r.code}\n ${r.out.trim().split('\n')[0]}`) +} +const clean = (m, body, opts = {}) => { + const r = scan(`#!/usr/bin/env bash\n${body}\n`, opts.name, opts.mode) + r.code === 0 ? ok(m) : bad(m, `expected exit 0, got ${r.code}\n ${r.out.trim().split('\n').slice(1, 4).join('\n ')}`) +} + +console.log('option spellings — the same GNU flag, written every way') +flags('short flag', "grep -P 'x' f") +flags('clustered short flag', "grep -Pio 'x' f") +flags('long flag', "grep --perl-regexp 'x' f") +flags('long flag after a short one', "grep -n --perl-regexp 'x' f") +flags('long flag after --opt=value', "grep --color=always -P 'x' f") +flags('date --date=value', 'date -u --date=@1 +%s') +flags('stat --format=value', 'stat -L --format=%Y f') +flags('sed --in-place after -e', "sed -e 's/x/y/' --in-place f") +flags('sed -i with attached empty quote', "sed -i'' 's/a/b/' f") + +console.log('\nevidence must be executable, not text that looks like it') +flags('a quoted counterpart is a string', "md5sum \"$f\" || printf '%s\\n' 'md5'") +flags('a quoted exemption is a string', "printf '# portability-ok: nope'\nH=$(md5sum \"$g\")") +flags('a logging uname is not a branch', 'printf \'%s\\n\' "$(uname -s)"\nH=$(md5sum "$f")') +flags('a bare || is not a fallback', 'md5sum "$f" || exit 1') +flags('an unpaired probe is not a branch', "sed --version >/dev/null\nsed -i 's/a/b/' f") +flags('a construct does not clear itself', 'H=$(md5sum "$a")\nG=$(md5sum "$b")') + +console.log('\ncommand position — naming a command is not running it') +clean('command -v is a mention', 'TMO=""; command -v timeout >/dev/null 2>&1 && TMO="timeout 10"') +clean('a case pattern lists, never runs', 'case "$s" in iteration_cap|timeout|x) echo cap;; esac') +flags('but an actual call still counts', 'timeout 10 sleep 1') + +console.log('\nreal fallbacks must keep passing') +clean('|| with the counterpart', 'age=$(stat -c %Y "$f" 2>/dev/null || stat -f %m "$f")') +clean('sha256sum || shasum', 'h=$(sha256sum "$f" 2>/dev/null || shasum -a 256 "$f")') +clean('counterpart on a nearby line', 'ts=$(date -d "$x" +%s 2>/dev/null)\nts=$(date -j -u -f \'%Y\' "$x" +%s)') +clean('nproc || sysctl', 'n=$(nproc 2>/dev/null || sysctl -n hw.ncpu)') +clean('uname case branch covers any tool', 'case "$(uname -s)" in Darwin) t_() { tail -r "$@"; };; *) t_() { tac "$@"; };; esac') +clean('sed --version branch, both spellings', + "if sed --version >/dev/null 2>&1; then\n f_() { sed -i \"$@\"; }\nelse\n f_() { sed -i '' \"$@\"; }\nfi") +clean('sed -i.bak is accepted by both', "sed -i.bak 's/a/b/' f") +clean('an exemption with a reason', 'G=$(md5sum f) # portability-ok: fixture, never runs on a user machine') +flags('an exemption without one', 'G=$(md5sum f) # portability-ok') + +console.log('\nscan surface') +flags('extensionless shell file, by shebang', 'H=$(md5sum f)', { name: '.githooks/pre-commit', mode: 0o755 }) +// Discovery is by shebang, not by extension -- so `.txt` carrying `#!/bin/bash` +// IS scanned, and only a file with neither is skipped. Written as its own case +// because the first draft asserted the opposite and the guard was right. +{ + const r = scan('H=$(md5sum f)\n', 'notes.txt') + r.code === 0 ? ok('no shebang, no shell extension -> not scanned') + : bad('no shebang, no shell extension -> not scanned', `expected 0, got ${r.code}`) +} +flags('shebang wins over extension', 'H=$(md5sum f)', { name: 'notes.txt' }) + +console.log(`\n${pass} passed, ${fail} failed`) +process.exit(fail === 0 ? 0 : 1)