diff --git a/.changeset/happy-deer-howl.md b/.changeset/happy-deer-howl.md new file mode 100644 index 000000000..ee5783142 --- /dev/null +++ b/.changeset/happy-deer-howl.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4149 +--- +**`gsd-tools commit`, `commit-to-subrepo`, and `pr-subrepo` no longer silently refuse to commit a moved submodule pointer under `diff.ignoreSubmodules=all`** — on git 2.39.x, `git commit` itself (pathspec-scoped or whole-index) consults that config the same way `git diff` does and drops the change, and `pr-subrepo`'s own change-detection probe hid the same submodule bump before it ever reached the commit step. All three commit sites, plus the `pr-subrepo` probe, now pin `diff.ignoreSubmodules=dirty` the same way the pre-existing empty-diff probe in `commit` already did. diff --git a/.changeset/mellow-pumas-zip.md b/.changeset/mellow-pumas-zip.md new file mode 100644 index 000000000..6fc53d4eb --- /dev/null +++ b/.changeset/mellow-pumas-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4149 +--- +**`/gsd-pause-work` phase/spike/sketch detection now works on macOS** — the #4112 fix removed a shell-syntax bug but left a GNU-only `grep -oP` that macOS's BSD grep silently fails on, so detection resolved to empty. A new lint (`lint-portable-grep`) now catches this class of GNU-only-grep-flag defect in workflow markdown before it merges. (#4112) diff --git a/gsd-core/workflows/pause-work.md b/gsd-core/workflows/pause-work.md index e44283e86..07305e412 100644 --- a/gsd-core/workflows/pause-work.md +++ b/gsd-core/workflows/pause-work.md @@ -15,13 +15,16 @@ Determine what kind of work is being paused and set the handoff destination acco ```bash # Check for active phase -phase=$(ls -lt .planning/phases/*/PLAN.md 2>/dev/null | head -1 | grep -oP 'phases/\K[^/]+' || true) +phase=$(ls -t .planning/phases/*/PLAN.md 2>/dev/null | head -1 || true) +phase=${phase:+$(basename "$(dirname "$phase")")} # Check for active spike -spike=$(ls -lt .planning/spikes/*/SPIKE.md .planning/spikes/*/DESIGN.md .planning/spikes/*/README.md 2>/dev/null | head -1 | grep -oP 'spikes/\K[^/]+' || true) +spike=$(ls -t .planning/spikes/*/SPIKE.md .planning/spikes/*/DESIGN.md .planning/spikes/*/README.md 2>/dev/null | head -1 || true) +spike=${spike:+$(basename "$(dirname "$spike")")} # Check for active sketch -sketch=$(ls -lt .planning/sketches/*/README.md .planning/sketches/*/index.html 2>/dev/null | head -1 | grep -oP 'sketches/\K[^/]+' || true) +sketch=$(ls -t .planning/sketches/*/README.md .planning/sketches/*/index.html 2>/dev/null | head -1 || true) +sketch=${sketch:+$(basename "$(dirname "$sketch")")} # Check for active deliberation deliberation=$(ls .planning/deliberations/*.md 2>/dev/null | head -1 || true) diff --git a/gsd-core/workflows/sync-skills.md b/gsd-core/workflows/sync-skills.md index 9ed2baf05..cf3e17368 100644 --- a/gsd-core/workflows/sync-skills.md +++ b/gsd-core/workflows/sync-skills.md @@ -30,14 +30,14 @@ IS_APPLY=false # Parse --from if [[ "$@" == *"--from"* ]]; then - FROM_RUNTIME=$(echo "$@" | grep -oP '(?<=--from )\S+') + FROM_RUNTIME=$(echo "$@" | sed -E 's/.*--from[[:space:]]+([^[:space:]]+).*/\1/') fi # Parse --to if [[ "$@" == *"--to all"* ]]; then TO_RUNTIMES=(antigravity augment claude cline codebuddy codex copilot cursor grok hermes kilo kimi kimi-code opencode pi qwen trae windsurf zcode) elif [[ "$@" == *"--to"* ]]; then - TO_RUNTIMES=( $(echo "$@" | grep -oP '(?<=--to )\S+') ) + TO_RUNTIMES=( $(echo "$@" | sed -E 's/.*--to[[:space:]]+([^[:space:]]+).*/\1/') ) fi # Parse --apply diff --git a/package.json b/package.json index f1bf7e7a1..d59f9b059 100644 --- a/package.json +++ b/package.json @@ -121,7 +121,7 @@ "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", "lint:frontmatter-scalar-broad-grep": "node scripts/lint-frontmatter-scalar-broad-grep.cjs", "lint:removed-but-needed": "node scripts/lint-removed-but-needed.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.cjs && node scripts/lint-workflow-shellcheck.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/lint-portable-grep.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.cjs && node scripts/lint-workflow-shellcheck.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lint-portable-grep.cjs b/scripts/lint-portable-grep.cjs new file mode 100644 index 000000000..42ad9bc2a --- /dev/null +++ b/scripts/lint-portable-grep.cjs @@ -0,0 +1,176 @@ +#!/usr/bin/env node +'use strict'; + +/** + * lint-portable-grep.cjs — ban GNU-only `grep -P`/`--perl-regexp` in gsd + * workflow / agent / reference / command markdown (#4112 macOS regression). + * + * ## Why + * + * `-P`/`--perl-regexp` selects GNU grep's PCRE engine. Stock macOS ships BSD + * grep, which does not implement `-P` at all — the call errors out ("grep: + * invalid option -- P" or "unknown option") on that host, and a pipeline like + * `... | grep -oP '...' || true` swallows that error and silently resolves to + * an empty string instead of failing loudly. + * + * This is exactly what happened in `gsd-core/workflows/pause-work.md`'s + * Context Detection step (#4112, merged as #4140): the fix for the `$((` + * shell-syntax bug left `grep -oP 'phases/\K[^/]+'` in place. That construct + * had never been exercised on macOS before (the syntax error masked it), so + * fixing the syntax bug newly exposed a second, pre-existing macOS-only + * defect — phase/spike/sketch detection silently resolving to "" on macOS — + * which the PR's own new regression test then caught, but only in the + * post-merge `full test (macos-latest, ...)` matrix. That lane runs on push + * to the base branch, not on the pull_request event, so nothing pre-merge + * (gsd-test's Linux-only benches included) could have caught it before `next` + * went red. + * + * This is the same class of defect `lint-portable-timeout.cjs` (#2351) exists + * for: a GNU-coreutils assumption baked into workflow markdown that every + * pre-merge gate is blind to because none of them run on macOS. The fix here + * is the same shape — a static ratchet that catches the assumption in the + * markdown itself, before a shell ever executes it on the platform that lacks + * the flag. + * + * ## What PASSES + * + * - `grep -o '...'`, `grep -E '...'`, `egrep`/`fgrep` without `-P`. + * - `-P` appearing as part of an unrelated long option (`--path`, `--perl`) — + * only a `-P`/short-cluster-containing-`P` token or the literal + * `--perl-regexp` counts. + * - Prose that merely mentions "grep -P" outside of a fenced command context + * is still flagged (this lint does not parse fences) — the roots below are + * restricted to directive markdown where a `grep -P` mention is always + * either a real invocation or a specimen to fix, never neutral prose. + * + * ## What FAILS + * + * A `grep`/`egrep`/`fgrep` invocation whose flag cluster includes `P` + * (`-P`, `-oP`, `-Po`, `-iP`, ...) or the long form `--perl-regexp`. + * + * ## Portable replacements + * + * - Extracting a path component: `basename "$(dirname "$path")")` instead of + * `grep -oP 'seam/\K[^/]+'`. + * - Extracting a token after a flag: `sed -E 's#.*--flag[[:space:]]+([^[:space:]]+).*#\1#'` + * instead of `grep -oP '(?<=--flag )\S+'` — POSIX ERE (`-E`) has no + * lookbehind, so capture the group and let `sed` emit just it. `-E` (not + * GNU-only `-r`) is supported by both BSD and GNU `sed`. (A `/`-delimited + * sed command ending in a star-quantifier would place a star immediately + * before a slash, closing this very JSDoc block comment early — hence the + * `#`-delimited form used above.) + */ + +const fs = require('fs'); +const path = require('path'); +const { ExitError, runMain } = require('./lib/cli-exit.cjs'); + +const ROOT = path.join(__dirname, '..'); + +// Surfaces whose markdown carries agent-executed bash. Mirrors +// lint-portable-timeout.cjs's roots — the same files that can ship shell +// snippets which a bash/zsh host on macOS actually executes. +const DEFAULT_ROOTS = ['gsd-core/workflows', 'gsd-core/references', 'agents', 'commands']; + +// A `grep`/`egrep`/`fgrep` token, anchored to a command position (line start, +// right after `| & ; ( \` {`, or right after a shell keyword that opens a new +// command — `then`/`do`/`else`/`elif`) so prose mentioning "grep -P" in +// running text is still caught in these directive-markdown roots (see file +// header) without also matching an unrelated word ending in "grep". The +// keyword alternative is `\b`-bounded on both sides, so a bare mention of the +// word "then"/"do"/etc. with no following grep never matches on its own. +const GREP_INVOCATION_RE = /(?:^|[|&;(`{]|\b(?:then|do|else|elif)\b)[ \t]*(?:e|f)?grep\b/g; + +// A `-P` short-flag cluster (e.g. `-P`, `-oP`, `-Po`, `-iP`) or the long form +// `--perl-regexp`, as a standalone token. +const PERL_FLAG_RE = /(?:^|\s)-[a-zA-Z]*P[a-zA-Z]*(?=\s|$)|--perl-regexp\b/; + +/** + * Locate `grep -P`/`--perl-regexp` invocations in a block of text. + * + * Pure (no I/O): callers pass file contents; the caller reads files. For each + * `grep`-family invocation on a line, inspects only the segment from that + * invocation to the next `|`/`;`/end-of-line, so a `-P`-bearing token in a + * later, unrelated command on the same line is never mistaken for a grep flag. + * + * @param {string} text file contents + * @returns {{ line: number, snippet: string }[]} findings (empty array = clean) + */ +function findPerlGrepInvocations(text) { + const findings = []; + const lines = String(text).split(/\r?\n/); + for (let i = 0; i < lines.length; i += 1) { + const line = lines[i]; + GREP_INVOCATION_RE.lastIndex = 0; + let match; + while ((match = GREP_INVOCATION_RE.exec(line)) !== null) { + const startAt = match.index + match[0].length; + const rest = line.slice(startAt); + const nextPipe = rest.search(/[|;`]/); + const segment = nextPipe === -1 ? rest : rest.slice(0, nextPipe); + if (PERL_FLAG_RE.test(segment)) { + findings.push({ line: i + 1, snippet: line.trim() }); + break; // one finding per line is enough context to fix it + } + } + } + return findings; +} + +function walkMarkdown(dir) { + const out = []; + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return out; // a missing root is not an error — some surfaces are optional + } + for (const entry of entries) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...walkMarkdown(full)); + else if (entry.isFile() && entry.name.endsWith('.md')) out.push(full); + } + return out; +} + +/** + * Scan the given roots (repo-relative) for `grep -P`/`--perl-regexp` invocations. + * @param {string[]} roots + * @returns {{ file: string, line: number, snippet: string }[]} + */ +function scan(roots = DEFAULT_ROOTS) { + const offenders = []; + for (const rel of roots) { + const abs = path.isAbsolute(rel) ? rel : path.join(ROOT, rel); + for (const file of walkMarkdown(abs)) { + const findings = findPerlGrepInvocations(fs.readFileSync(file, 'utf8')); + for (const f of findings) { + offenders.push({ file: path.relative(ROOT, file), line: f.line, snippet: f.snippet }); + } + } + } + return offenders; +} + +function main() { + const rootsEnv = process.env.GSD_LINT_PORTABLE_GREP_ROOTS; + const roots = rootsEnv ? rootsEnv.split(path.delimiter).filter(Boolean) : DEFAULT_ROOTS; + const offenders = scan(roots); + if (offenders.length > 0) { + const detail = offenders.map((o) => ` ${o.file}:${o.line} ${o.snippet}`).join('\n'); + throw new ExitError( + 1, + 'lint-portable-grep: `grep -P`/`--perl-regexp` is not portable — stock macOS ships\n' + + 'BSD grep, which has no `-P`, so the call errors and a `|| true`/`$(...)` fallback\n' + + 'silently resolves to empty output instead of failing loudly (#4112). Use\n' + + '`basename "$(dirname "$path")")` for path-component extraction, or\n' + + '`sed -E \'s/.../\\1/\'` (POSIX ERE, no lookbehind needed) for token extraction:\n' + + detail, + ); + } + console.log(`ok lint-portable-grep: no GNU-only grep -P invocations in ${roots.length} root(s)`); +} + +module.exports = { findPerlGrepInvocations, scan, DEFAULT_ROOTS }; + +if (require.main === module) runMain(main); diff --git a/src/commands.cts b/src/commands.cts index 6b72d9335..357d0863b 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -2046,10 +2046,45 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u if (canScope) { commitArgs.push('--', ...stagedPaths); } + // #3859 follow-up: on git 2.39.5 (confirmed on the CI Linux bench image, + // ghcr.io/open-gsd/gsd-tester-linux:v1.8.0-node24; NOT reproducible on git + // 2.50.1) `git commit` itself — not just `git diff` — consults + // `diff.ignoreSubmodules` when deciding whether there is anything to + // record. With a local `diff.ignoreSubmodules=all` and a submodule gitlink + // genuinely bumped, that git version silently REFUSES the commit (prints a + // `git status`-style "Changes to be committed" dump and exits 1, having + // written nothing) even though the diff probe above (already pinned with + // its own `--ignore-submodules=dirty`) correctly reported the change as + // present. The result was misclassified as generic `commit_failed` because + // git's refusal text does not contain "nothing to commit". + // Originally scoped to `canScope` on the assumption that only a + // PATHSPEC-LIMITED `git commit -- ` exercises this git internal + // path. That assumption was wrong: reproduced directly against the pinned + // v1.8.0-node24 tester image, a bare WHOLE-INDEX `git commit -m ...` (no + // pathspec at all) is refused identically when the only staged change is a + // submodule gitlink and `diff.ignoreSubmodules=all` — git's "nothing to + // commit" check is a real diff (HEAD vs. index) honouring + // `diff.ignoreSubmodules` regardless of whether a pathspec narrows it. + // `--amend` is the one shape confirmed NOT to hit this: it always + // recreates the commit from the current index and never runs the + // empty-diff refusal a plain `git commit` does, override or not. The + // override is therefore applied unconditionally here (not gated on + // `canScope`) — it is a documented no-op everywhere it is not needed + // (dry-run, git 2.50.1, and `--amend` already behave this way with or + // without it; see `#3859 follow-up (canScope gap)` regression tests). + // The override rides in via `GIT_CONFIG_*` env vars rather than a `-c` + // argv flag so `commitArgs[0]` stays `'commit'` — several #3859 regression + // tests assert on the raw argv captured at the `execGit` seam (e.g. + // `gitCalls.some((a) => a[0] === 'commit')`), and a leading `-c` would shift + // every element and break that pinning. Same override the probe already + // carries, so the two can never disagree again. + const commitEnv: Record = { + GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.ignoreSubmodules', GIT_CONFIG_VALUE_0: 'dirty', + }; // #3886: `git commit` runs pre-commit hooks (husky/lint-staged routinely // idles ~4s on Windows before any task) — 10s is too tight, and a timeout // kill is NOT an ordinary failure. Same band as the push call below. - const commitResult = execGit(commitArgs, { cwd, timeout: COMMIT_TIMEOUT_MS }); + const commitResult = execGit(commitArgs, { cwd, timeout: COMMIT_TIMEOUT_MS, env: commitEnv }); if (commitResult.exitCode !== 0) { // #3886: a SIGTERM'd git commit is a timeout, not commit_failed — the // partial stderr it flushed (often incidental CRLF warnings) is noise, @@ -2213,7 +2248,12 @@ function cmdCommitToSubrepo(cwd: string, message: string | undefined, files: str const commitArgs = canScopeSub ? ['commit', '-m', message as string, '--', ...stagedRelPaths] : ['commit', '-m', message as string]; - const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS }); + // #3859 follow-up fix as cmdCommit above (line ~2081) — git 2.39.5 needs + // this override for pathspec-scoped AND whole-index commits alike. + const commitEnvSub: Record = { + GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.ignoreSubmodules', GIT_CONFIG_VALUE_0: 'dirty', + }; + const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS, env: commitEnvSub }); if (commitResult.exitCode !== 0) { if (isSpawnTimeout(commitResult)) { // #3886 (subrepo counterpart): timeout ≠ error; surface the stale-lock @@ -2301,7 +2341,19 @@ function cmdPrSubrepo( // 1. Collect changed files via porcelain status — explicit, never git add -A. // ?? (untracked) lines are excluded — only stage tracked modifications. - const statusResult = execGit(['-c', 'core.quotePath=false', 'status', '--porcelain'], { cwd: repoCwd }); + // #3859 follow-up: `git status --porcelain` honors `diff.ignoreSubmodules` + // the same way the empty-diff probe fixed for cmdCommit did — under a local + // `diff.ignoreSubmodules=all`, a genuinely bumped submodule gitlink is + // invisible here too, so `changedFiles` comes back empty and the function + // reports `nothing_to_commit` before ever reaching the (now-fixed) commit + // call. `--ignore-submodules=dirty` pins this the same way, reported + // verbatim: `git -C repo status --porcelain` (no flag) shows nothing for a + // pure gitlink bump under `diff.ignoreSubmodules=all`, while + // `--ignore-submodules=dirty` reports ` M nested` (reproduced directly). + const statusResult = execGit( + ['-c', 'core.quotePath=false', 'status', '--porcelain', '--ignore-submodules=dirty'], + { cwd: repoCwd }, + ); if (statusResult.exitCode !== 0) { error(`git status failed in ${repo}: ${statusResult.stderr}`); } @@ -2381,7 +2433,12 @@ function cmdPrSubrepo( const commitArgs = canScopePr ? ['commit', '-m', commitMessage as string, '--', ...changedFiles] : ['commit', '-m', commitMessage as string]; - const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS }); + // #3859 follow-up fix as cmdCommit above (line ~2081) — git 2.39.5 needs + // this override for pathspec-scoped AND whole-index commits alike. + const commitEnvPr: Record = { + GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.ignoreSubmodules', GIT_CONFIG_VALUE_0: 'dirty', + }; + const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS, env: commitEnvPr }); if (commitResult.exitCode !== 0) { rollback(); if (isSpawnTimeout(commitResult)) { diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 942242683..c8184d768 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -5737,8 +5737,180 @@ describe('#3859: the empty-diff probe is pinned against diff-only configuration' gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), 'B\n', 'the new content must actually be recorded'); }); + + // #3859 follow-up (e935694fc/b3d37b929, widened after a canScope gap found + // reproducing live against the pinned CI tester image, + // ghcr.io/open-gsd/gsd-tester-linux:v1.8.0-node24, which runs git 2.39.5): + // the actual `git commit` call carries a `commitEnv` GIT_CONFIG_* override + // forcing `diff.ignoreSubmodules=dirty`. This was originally scoped to only + // fire when `canScope` was true (a pathspec-limited `git commit -- + // `), on the assumption that only a pathspec-limited commit + // consults `diff.ignoreSubmodules` when deciding whether a bumped + // submodule gitlink is a real change to record. That assumption was wrong: + // on git 2.39.5 a bare WHOLE-INDEX `git commit -m ...` (no pathspec at + // all, canScope=false) is refused identically when the only staged change + // is a submodule gitlink under `diff.ignoreSubmodules=all` — git's + // "nothing to commit" check is a real diff (HEAD vs. index) that honours + // `diff.ignoreSubmodules` regardless of pathspec. The override is now + // applied unconditionally (no `canScope` gate) to cover this shape too. + // `--amend` is the one shape confirmed NOT to hit the refusal at all + // (reproduced directly: it succeeds identically with or without the + // override, since amend never runs the empty-diff check a plain `git + // commit` does) — its test below pins that the override being applied + // unconditionally is still harmless there. + test('a whole-index commit (no --files, canScope=false) still records a bumped submodule under diff.ignoreSubmodules=all', () => { + bumpedSubmodule(); + gitOrThrow(['config', 'diff.ignoreSubmodules', 'all'], { cwd: tmpDir }); + gitOrThrow(['add', '--', 'sub'], { cwd: tmpDir }); + const before = gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(); + + const result = runGsdTools('commit "m"', tmpDir); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + const output = JSON.parse(payload); + + assert.strictEqual(output.committed, true, + 'a whole-index commit hits the same git 2.39.5 refusal as a pathspec-limited one, so it needs the ' + + 'GIT_CONFIG_* override applied unconditionally, not gated on canScope, to record the bumped gitlink'); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(), before, + 'the recorded gitlink must actually advance'); + }); + + test('an --amend commit (canScope=false) still records a bumped submodule under diff.ignoreSubmodules=all', () => { + bumpedSubmodule(); + gitOrThrow(['config', 'diff.ignoreSubmodules', 'all'], { cwd: tmpDir }); + gitOrThrow(['add', '--', 'sub'], { cwd: tmpDir }); + const before = gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(); + + const result = runGsdTools('commit "m" --amend', tmpDir); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + const output = JSON.parse(payload); + + assert.strictEqual(output.committed, true, + '--amend never hits the empty-diff refusal, so the now-unconditional GIT_CONFIG_* override must remain ' + + 'a harmless no-op here'); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(), before, + 'the recorded gitlink must actually advance'); + }); }); +// #3859 follow-up: `cmdCommitToSubrepo` and `cmdPrSubrepo` carry the identical +// structurally-shaped `canScope*`-branched `git commit` call as `cmdCommit` +// above (see `COMMIT_TIMEOUT_MS`'s "three commit sites" comment in +// src/commands.cts) and were missing the same `diff.ignoreSubmodules=dirty` +// GIT_CONFIG_* override, applied unconditionally for the same reason. +describe('#3859 follow-up: commit-to-subrepo and pr-subrepo also need the diff.ignoreSubmodules override', () => { + const { createTempGitProject } = require('./helpers.cjs'); + let rootDir; + let nestedSubmoduleSrc; + + afterEach(() => { + if (rootDir) cleanup(rootDir); + if (nestedSubmoduleSrc) cleanup(nestedSubmoduleSrc); + rootDir = undefined; + nestedSubmoduleSrc = undefined; + }); + + // A submodule nested inside `repoDir` whose recorded gitlink is AHEAD of + // what `repoDir` has committed — same shape as `bumpedSubmodule()` above, + // scoped to an arbitrary sub-repo directory instead of the project root. + function bumpedSubmoduleIn(repoDir) { + const subSrc = path.join(repoDir, '..', path.basename(repoDir) + '-nested-sub'); + nestedSubmoduleSrc = subSrc; + fs.mkdirSync(subSrc, { recursive: true }); + gitOrThrow(['init', '-q', '.'], { cwd: subSrc }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: subSrc }); + gitOrThrow(['config', 'user.name', 't'], { cwd: subSrc }); + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v1\n'); + gitOrThrow(['add', 'f.txt'], { cwd: subSrc }); + gitOrThrow(['commit', '-m', 'v1'], { cwd: subSrc }); + + gitOrThrow(['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', subSrc, 'nested'], { cwd: repoDir }); + gitOrThrow(['commit', '-m', 'add nested submodule'], { cwd: repoDir }); + + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v2\n'); + gitOrThrow(['add', 'f.txt'], { cwd: subSrc }); + gitOrThrow(['commit', '-m', 'v2'], { cwd: subSrc }); + gitOrThrow(['-c', 'protocol.file.allow=always', 'submodule', 'update', '--remote', '--', 'nested'], { cwd: repoDir }); + } + + test('commit-to-subrepo records a bumped nested submodule under diff.ignoreSubmodules=all', () => { + rootDir = createTempGitProject(); + fs.writeFileSync( + path.join(rootDir, '.planning', 'config.json'), + JSON.stringify({ planning: { sub_repos: ['backend'] } }, null, 2), + ); + const subDir = path.join(rootDir, 'backend'); + fs.mkdirSync(subDir, { recursive: true }); + gitOrThrow(['init', '-q', '.'], { cwd: subDir }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: subDir }); + gitOrThrow(['config', 'user.name', 't'], { cwd: subDir }); + fs.writeFileSync(path.join(subDir, 'seed.js'), '// seed\n'); + gitOrThrow(['add', 'seed.js'], { cwd: subDir }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: subDir }); + + bumpedSubmoduleIn(subDir); + gitOrThrow(['config', 'diff.ignoreSubmodules', 'all'], { cwd: subDir }); + const before = gitOrThrow(['rev-parse', 'HEAD:nested'], { cwd: subDir }).trim(); + + const res = runGsdTools( + ['commit-to-subrepo', 'chore: bump nested submodule', '--files', 'backend/nested'], + rootDir, + ); + assert.ok(res.success, `commit-to-subrepo failed: ${res.error}`); + const result = JSON.parse(res.output); + + assert.strictEqual(result.repos.backend.committed, true, + `the gitlink moved and \`git commit -- nested\` records it on git 2.39.5 only with the ` + + `GIT_CONFIG_* override applied, got ${JSON.stringify(result.repos.backend)}`); + assert.notStrictEqual(result.repos.backend.reason, 'error'); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:nested'], { cwd: subDir }).trim(), before, + 'the recorded gitlink must actually advance'); + }); + + test('pr-subrepo records a bumped nested submodule under diff.ignoreSubmodules=all', () => { + rootDir = createTempGitProject(); + fs.writeFileSync( + path.join(rootDir, '.planning', 'config.json'), + JSON.stringify({ planning: { sub_repos: ['backend'] } }, null, 2), + ); + const subDir = path.join(rootDir, 'backend'); + const bareDir = path.join(rootDir, '_bare-backend.git'); + fs.mkdirSync(subDir, { recursive: true }); + gitOrThrow(['init', '-q', '.'], { cwd: subDir }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: subDir }); + gitOrThrow(['config', 'user.name', 't'], { cwd: subDir }); + fs.writeFileSync(path.join(subDir, 'seed.js'), '// seed\n'); + gitOrThrow(['add', 'seed.js'], { cwd: subDir }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: subDir }); + fs.mkdirSync(bareDir, { recursive: true }); + gitOrThrow(['init', '--bare', '-q'], { cwd: bareDir }); + gitOrThrow(['remote', 'add', 'origin', bareDir], { cwd: subDir }); + const branch = gitOrThrow(['branch', '--show-current'], { cwd: subDir }).trim(); + gitOrThrow(['push', 'origin', branch], { cwd: subDir }); + + bumpedSubmoduleIn(subDir); + gitOrThrow(['config', 'diff.ignoreSubmodules', 'all'], { cwd: subDir }); + const before = gitOrThrow(['rev-parse', 'HEAD:nested'], { cwd: subDir }).trim(); + + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): bump nested submodule', + '--repo', 'backend', '--branch', 'fix-3859-nested-submodule-pr'], + rootDir, + ); + assert.ok(res.success, `pr-subrepo failed: ${res.error}`); + const result = JSON.parse(res.output); + + assert.strictEqual(result.committed, true, + `the gitlink moved and \`git commit -- nested\` records it on git 2.39.5 only with the ` + + `GIT_CONFIG_* override applied, got ${JSON.stringify(result)}`); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:nested'], { cwd: subDir }).trim(), before, + 'the recorded gitlink must actually advance'); + }); +}); describe('#2279: map-codebase date stamp instructions overwrite existing dates', () => { const REPO_ROOT = path.join(__dirname, '..'); diff --git a/tests/lint-portable-grep.test.cjs b/tests/lint-portable-grep.test.cjs new file mode 100644 index 000000000..b761e07ae --- /dev/null +++ b/tests/lint-portable-grep.test.cjs @@ -0,0 +1,95 @@ +'use strict'; + +/** + * Tests for scripts/lint-portable-grep.cjs — the ratchet that bans GNU-only + * `grep -P`/`--perl-regexp` in gsd workflow / agent / reference / command + * markdown (#4112 macOS regression: a `grep -oP` left behind after the + * `pause-work.md` `$((` shell-syntax fix silently resolved phase/spike/sketch + * detection to "" on stock macOS's BSD grep). + * + * Tests the PURE check logic (findPerlGrepInvocations) directly, on in-memory + * string fixtures, so the suite is fast, hermetic, and never reads real repo + * files (that integration concern is already covered by `npm run lint:ci`). + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +const { findPerlGrepInvocations } = require('../scripts/lint-portable-grep.cjs'); + +describe('lint-portable-grep: findPerlGrepInvocations pure logic', () => { + test('flags `grep -oP` with a lookahead pattern', () => { + const findings = findPerlGrepInvocations(String.raw`grep -oP 'x\K[^/]+'`); + assert.strictEqual(findings.length, 1); + assert.strictEqual(findings[0].line, 1); + }); + + test('flags bare `grep -P`', () => { + const findings = findPerlGrepInvocations(`grep -P 'x'`); + assert.strictEqual(findings.length, 1); + }); + + test('flags `grep --perl-regexp` long form', () => { + const findings = findPerlGrepInvocations(`grep --perl-regexp 'x'`); + assert.strictEqual(findings.length, 1); + }); + + test('flags `egrep -P`', () => { + const findings = findPerlGrepInvocations(`egrep -P 'x'`); + assert.strictEqual(findings.length, 1); + }); + + test('flags `fgrep -P`', () => { + const findings = findPerlGrepInvocations(`fgrep -P 'x'`); + assert.strictEqual(findings.length, 1); + }); + + test('passes `grep -o` (no P flag)', () => { + const findings = findPerlGrepInvocations(`grep -o 'x'`); + assert.strictEqual(findings.length, 0); + }); + + test('passes `grep -E` (POSIX ERE, no perl flag)', () => { + const findings = findPerlGrepInvocations(`grep -E 'x'`); + assert.strictEqual(findings.length, 0); + }); + + test('passes a line containing `--path` (lowercase p, not a -P cluster)', () => { + const findings = findPerlGrepInvocations(`some-cmd --path /foo | grep -o 'x'`); + assert.strictEqual(findings.length, 0); + }); + + test('segment-scoping: an unrelated earlier -P-bearing command does not taint a later plain grep', () => { + const findings = findPerlGrepInvocations(`foo -P | grep -o 'x'`); + assert.strictEqual(findings.length, 0, 'the -P belongs to `foo`, not to the grep invocation after the pipe'); + }); + + test('multi-line input: finding reports the correct 1-indexed line number', () => { + const text = ['line one is clean', 'line two is also clean', `grep -oP 'x\\K.*'`, 'line four is clean'].join( + '\n', + ); + const findings = findPerlGrepInvocations(text); + assert.strictEqual(findings.length, 1); + assert.strictEqual(findings[0].line, 3); + }); + + test('empty string input produces no findings', () => { + const findings = findPerlGrepInvocations(''); + assert.strictEqual(findings.length, 0); + }); + + test('flags `grep -oP` right after a `then` shell keyword', () => { + const findings = findPerlGrepInvocations(`if [ -n "$x" ]; then grep -oP 'x' ; fi`); + assert.strictEqual(findings.length, 1); + }); + + test('flags `grep -oP` right after a `do` shell keyword', () => { + const findings = findPerlGrepInvocations(`for f in *.md; do grep -oP 'x' "$f"; done`); + assert.strictEqual(findings.length, 1); + }); + + test('a bare mention of the word "then" with no following grep is not flagged', () => { + const findings = findPerlGrepInvocations(`this sentence mentions the word then but nothing else`); + assert.strictEqual(findings.length, 0); + }); +});