From 021dae3cd89370c08f17ed1a7be200a449b86d51 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Tue, 12 May 2026 16:03:29 +0900 Subject: [PATCH 1/3] port: hook + validator fixes from ECC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second port PR from the 2026-05-12 upstream sync round (audit log: upstream/sync-rounds/2026-05-12.md). scripts/hooks/block-no-verify.js (0dcde13) Rewrite flag detection to use shell-words tokenization instead of string-stripping regex. EGC's previous stripQuotedStrings approach had two real bypass holes: (1) "git -c \"core.hooksPath=/dev/null\" commit -m \"msg\"" stripped the quoted -c value to "" and silently allowed the hook override; (2) "git commit \"--no-verify\" -m \"msg\"" similarly stripped the quoted flag to "". The tokenizer preserves shell quoting structure, so quoted args occupying the flag position are still detected as flags while flags MENTIONED inside a commit message body remain false-positive-safe. Also gains: combined short options like `git commit -am` are now parsed correctly (-am with a message arg does not consume the next token as a value for -n), and chained git commands like "git commit -m 'msg' && git push --no-verify" detect the push bypass instead of stopping at the first git invocation. tests/hooks/block-no-verify.test.js (0dcde13) +6 new tests: quoted core.hooksPath override; -am combined short opt + quoted --no-verify / -n message; git bypass phrase in quoted message; real quoted --no-verify flag still blocked; later chained git commands still blocked. 23/23 pass. scripts/ci/validate-skills.js (e196f8a) Extend the validator to inspect SKILL.md frontmatter: - require name: field (non-empty after trim + comment-strip) - reject literal block-scalar description: (|, |-, |+, |2, |-2, |- # note, etc.) — preserves internal newlines and breaks flat-table renderers keyed off description. Frontmatter findings default to WARN (exit 0) and promote to ERROR with --strict or CI_STRICT_SKILLS=1. Structural findings (missing or empty SKILL.md) remain hard errors. skills/openclaw-persona-forge/SKILL.md (data fix, paired with e196f8a) description: was a literal block scalar (|-). Inlined to a quoted string preserving the original Chinese text verbatim. skills/skill-stocktake/SKILL.md (data fix, paired with e196f8a) Frontmatter was missing name: entirely. Added "name: skill-stocktake". .github/workflows/reusable-validate.yml Enable --strict on validate-skills now that both data defects are fixed. Future SKILL.md additions cannot reintroduce the same class of frontmatter defect without failing CI. Four ECC commits evaluated and not ported (architecture mismatch): - d26d66f / b1456bd / 03108be: ECC's session-start hook emits an additional_context block carrying learned-instinct content. EGC's session-start.js only manages session/learned-skills directories and command shims — there is no additional_context surface to inject into. These are session-start design changes, not bug fixes. - 3fadc37: ECC moves continuous-learning observe from a bash hook to a Node observe-runner registered in hooks/hooks.json. EGC keeps the observe hook inside the skill bundle at skills/continuous-learning-v2/hooks/observe.sh. Porting would require restructuring EGC's skill-local hook layout. Lint clean. 270/264 -> 270 total tests pass (+6 new in block-no-verify). validate-skills reports 0 warnings against the current skills/ tree under --strict. --- .github/workflows/reusable-validate.yml | 2 +- scripts/ci/validate-skills.js | 213 ++++++++++++++- scripts/hooks/block-no-verify.js | 329 +++++++++++++++++++++--- skills/openclaw-persona-forge/SKILL.md | 10 +- skills/skill-stocktake/SKILL.md | 1 + tests/hooks/block-no-verify.test.js | 35 +++ 6 files changed, 527 insertions(+), 63 deletions(-) diff --git a/.github/workflows/reusable-validate.yml b/.github/workflows/reusable-validate.yml index e255d1f..70b2510 100644 --- a/.github/workflows/reusable-validate.yml +++ b/.github/workflows/reusable-validate.yml @@ -34,7 +34,7 @@ jobs: run: node scripts/ci/validate-commands.js - name: Validate skills - run: node scripts/ci/validate-skills.js + run: node scripts/ci/validate-skills.js --strict - name: Validate workflow security run: node scripts/ci/validate-workflow-security.js diff --git a/scripts/ci/validate-skills.js b/scripts/ci/validate-skills.js index 066045d..6ffc853 100644 --- a/scripts/ci/validate-skills.js +++ b/scripts/ci/validate-skills.js @@ -1,21 +1,210 @@ #!/usr/bin/env node /** - * Validate skill directories have SKILL.md with required structure + * Validate curated skill directories (skills/ in repo). + * + * Checks: + * 1. Each sub-directory of skills/ contains a SKILL.md file. + * 2. SKILL.md is non-empty. + * 3. SKILL.md frontmatter (if present) declares a `name:` field. + * 4. SKILL.md frontmatter `description:` uses an inline scalar — not a + * literal block scalar (`|` / `|-` / `|+`), which preserves internal + * newlines and breaks flat-table renderers keyed off `description`. + * + * Frontmatter findings default to WARN so CI does not break while + * pre-existing data defects are being cleaned up out of band (see #1663). + * Pass `--strict` or set `CI_STRICT_SKILLS=1` to promote frontmatter + * findings to errors (exit 1). + * + * Structural findings (missing/empty SKILL.md) are always errors. + * + * Scope: curated only. Learned/imported/evolved roots are out of scope. + * If skills/ does not exist, exit 0 (no curated skills to validate). */ const fs = require('fs'); const path = require('path'); -const { validateDirs } = require('../lib/validator'); -validateDirs({ - dir: path.join(__dirname, '../../skills'), - label: 'skill', - validate(dirPath) { - const skillMd = path.join(dirPath, 'SKILL.md'); - if (!fs.existsSync(skillMd)) return ['Missing SKILL.md']; +const SKILLS_DIR = path.join(__dirname, '../../skills'); - const content = fs.readFileSync(skillMd, 'utf-8'); - if (content.trim().length === 0) return ['SKILL.md is empty']; - return null; +const STRICT = process.argv.includes('--strict') || process.env.CI_STRICT_SKILLS === '1'; + +/** + * Parse the leading YAML frontmatter of a markdown document. + * + * Returns `{ present, lines }` so callers can inspect raw lines + * (needed to detect block-scalar `description:` values). + * + * Tolerant of UTF-8 BOM and CRLF line endings, matching the other + * validators in this directory. + * + * @param {string} content + * @returns {{present: boolean, lines: string[]}} + */ +function extractFrontmatter(content) { + // Strip BOM if present (UTF-8 BOM: U+FEFF). + const clean = content.replace(/^\uFEFF/, ''); + const match = clean.match(/^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)/); + if (!match) return { present: false, lines: [] }; + return { + present: true, + lines: match[1].split(/\r?\n/) + }; +} + +/** + * Extract top-level keys (with trimmed values) and flag block-scalar + * `description:` values. + * + * Lines that continue a block scalar (`|` or `>`) are skipped — we only + * care about the top-level key set and the raw indicator on the + * `description:` line. Block-scalar indicators accept YAML chomp and + * indent modifiers and trailing comments, e.g. `|`, `|-`, `|+`, `|2`, + * `|-2`, `>- # note`. + * + * @param {string[]} lines + * @returns {{values: Record, descriptionIndicator: string|null}} + */ +function inspectFrontmatter(lines) { + const values = Object.create(null); + let descriptionIndicator = null; + let inBlockScalar = false; + let blockScalarIndent = -1; + + for (const rawLine of lines) { + if (inBlockScalar) { + // Stay inside the block until a line with indent <= the opener's + // indent (or an empty continuation). + const leadingSpaces = rawLine.match(/^(\s*)/)[1].length; + if (rawLine.trim() === '' || leadingSpaces > blockScalarIndent) { + continue; + } + inBlockScalar = false; + blockScalarIndent = -1; + } + + const match = rawLine.match(/^([A-Za-z0-9_-]+):\s*(.*)$/); + if (!match) continue; + + const key = match[1]; + const rawValue = match[2]; + // Strip unquoted comments for value/indicator inspection. Handles both + // trailing comments (`foo: bar # note`) and comment-only values + // (`foo: # todo`) so the latter is treated as empty. + const valueNoComment = rawValue + .replace(/^\s*#.*$/, '') + .replace(/\s+#.*$/, '') + .trim(); + values[key] = valueNoComment; + + // Detect literal / folded block-scalar indicators. Accept chomp + // modifiers (`-` / `+`) and optional indent-indicator digits in + // either order, per YAML 1.2. + if (/^[|>](?:[+-]?\d+|\d+[+-]?|[+-])?$/.test(valueNoComment)) { + if (key === 'description') { + descriptionIndicator = valueNoComment; + } + inBlockScalar = true; + blockScalarIndent = rawLine.match(/^(\s*)/)[1].length; + } + } + + return { values, descriptionIndicator }; +} + +/** + * Validate a single skill directory. + * + * Returns `{ fatal }` where `fatal` indicates a structural error that + * should be surfaced via `console.error` and abort CI (missing/empty + * SKILL.md). Frontmatter findings are routed through + * `reportFrontmatterFinding`, which owns the WARN/ERROR decision based + * on strict mode. + * + * @param {string} dir + * @param {string} skillsDir + * @param {(msg: string) => void} reportFrontmatterFinding + * @returns {{fatal: boolean}} + */ +function validateSkillDir(dir, skillsDir, reportFrontmatterFinding) { + const skillMd = path.join(skillsDir, dir, 'SKILL.md'); + if (!fs.existsSync(skillMd)) { + console.error(`ERROR: ${dir}/ - Missing SKILL.md`); + return { fatal: true }; + } + + let content; + try { + content = fs.readFileSync(skillMd, 'utf-8'); + } catch (err) { + console.error(`ERROR: ${dir}/SKILL.md - ${err.message}`); + return { fatal: true }; + } + if (content.trim().length === 0) { + console.error(`ERROR: ${dir}/SKILL.md - Empty file`); + return { fatal: true }; } -}); + + const fm = extractFrontmatter(content); + if (fm.present) { + const { values, descriptionIndicator } = inspectFrontmatter(fm.lines); + + if (!Object.prototype.hasOwnProperty.call(values, 'name')) { + reportFrontmatterFinding(`${dir}/SKILL.md - frontmatter missing required field: name`); + } else if (values.name === '') { + reportFrontmatterFinding(`${dir}/SKILL.md - frontmatter 'name' is empty`); + } + + if (descriptionIndicator && descriptionIndicator.startsWith('|')) { + reportFrontmatterFinding( + `${dir}/SKILL.md - frontmatter description uses literal block scalar ` + `'${descriptionIndicator}' which preserves internal newlines; ` + `use an inline string or folded '>' scalar instead` + ); + } + } + + return { fatal: false }; +} + +function validateSkills() { + if (!fs.existsSync(SKILLS_DIR)) { + console.log('No curated skills directory (skills/), skipping'); + process.exit(0); + } + + const entries = fs.readdirSync(SKILLS_DIR, { withFileTypes: true }); + const dirs = entries.filter(e => e.isDirectory() && !e.name.startsWith('.')).map(e => e.name); + + let hasErrors = false; + let warnCount = 0; + let validCount = 0; + + const reportFrontmatterFinding = msg => { + if (STRICT) { + console.error(`ERROR: ${msg}`); + hasErrors = true; + } else { + console.warn(`WARN: ${msg}`); + warnCount++; + } + }; + + for (const dir of dirs) { + const { fatal } = validateSkillDir(dir, SKILLS_DIR, reportFrontmatterFinding); + if (fatal) { + hasErrors = true; + continue; + } + validCount++; + } + + if (hasErrors) { + process.exit(1); + } + + let msg = `Validated ${validCount} skill directories`; + if (warnCount > 0) { + msg += ` (${warnCount} warning${warnCount === 1 ? '' : 's'})`; + } + console.log(msg); +} + +validateSkills(); diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 4562630..84979b9 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -35,6 +35,212 @@ const GIT_COMMANDS_WITH_NO_VERIFY = [ */ const VALID_BEFORE_GIT = ' \t\n\r;&|$`(<{!"\']/.~\\'; +const GIT_CONFIG_KEY_PREFIX = 'core.hooksPath='; + +const COMMIT_OPTIONS_WITH_VALUE = new Set([ + '-m', + '--message', + '-F', + '--file', + '-C', + '--reuse-message', + '-c', + '--reedit-message', + '--author', + '--date', + '--template', + '--fixup', + '--squash', + '--pathspec-from-file', +]); + +const COMMIT_OPTIONS_WITH_INLINE_VALUE = [ + '--message=', + '--file=', + '--reuse-message=', + '--reedit-message=', + '--author=', + '--date=', + '--template=', + '--fixup=', + '--squash=', + '--pathspec-from-file=', +]; + +const COMMIT_SHORT_OPTIONS_WITH_VALUE = new Set(['m', 'F', 'C', 'c']); + +function tokenizeShellWords(input, start = 0, end = input.length) { + const tokens = []; + let value = ''; + let tokenStart = null; + let quote = null; + let escaped = false; + + function beginToken(index) { + if (tokenStart === null) { + tokenStart = index; + } + } + + function pushToken(index) { + if (tokenStart === null) { + return; + } + + tokens.push({ + value, + start: tokenStart, + end: index, + }); + value = ''; + tokenStart = null; + } + + for (let i = start; i < end; i++) { + const char = input.charAt(i); + + if (escaped) { + beginToken(i - 1); + value += char; + escaped = false; + continue; + } + + if (quote) { + if (char === quote) { + quote = null; + continue; + } + + if (quote === '"' && char === '\\') { + beginToken(i); + escaped = true; + continue; + } + + beginToken(i); + value += char; + continue; + } + + if (char === '"' || char === "'") { + beginToken(i); + quote = char; + continue; + } + + if (char === '\\') { + beginToken(i); + escaped = true; + continue; + } + + if (/\s/.test(char)) { + pushToken(i); + continue; + } + + beginToken(i); + value += char; + } + + if (escaped) { + value += '\\'; + } + pushToken(end); + + return tokens; +} + +function findCommandSegmentEnd(input, start) { + let quote = null; + let escaped = false; + + for (let i = start; i < input.length; i++) { + const char = input.charAt(i); + + if (escaped) { + escaped = false; + continue; + } + + if (quote) { + if (quote === '"' && char === '\\') { + escaped = true; + continue; + } + if (char === quote) { + quote = null; + } + continue; + } + + if (char === '"' || char === "'") { + quote = char; + continue; + } + + if (char === '\\') { + escaped = true; + continue; + } + + if (char === ';' || char === '|' || char === '&' || char === '\n') { + return i; + } + } + + return input.length; +} + +function commitOptionConsumesNextValue(value) { + if (isCommitNoVerifyShortFlag(value)) { + return false; + } + + if (COMMIT_OPTIONS_WITH_VALUE.has(value)) { + return true; + } + + const shortValueOption = getCommitShortValueOption(value); + return Boolean(shortValueOption && shortValueOption.consumesNextValue); +} + +function commitOptionContainsInlineValue(value) { + if (isCommitNoVerifyShortFlag(value)) { + return false; + } + + if (COMMIT_OPTIONS_WITH_INLINE_VALUE.some(prefix => value.startsWith(prefix))) { + return true; + } + + const shortValueOption = getCommitShortValueOption(value); + return Boolean(shortValueOption && shortValueOption.containsInlineValue); +} + +function getCommitShortValueOption(value) { + if (!value.startsWith('-') || value.startsWith('--') || value === '-') { + return null; + } + + const options = value.slice(1); + for (let i = 0; i < options.length; i++) { + if (COMMIT_SHORT_OPTIONS_WITH_VALUE.has(options.charAt(i))) { + return { + consumesNextValue: i === options.length - 1, + containsInlineValue: i < options.length - 1, + }; + } + } + + return null; +} + +function isCommitNoVerifyShortFlag(value) { + return value === '-n' || /^-n[a-zA-Z]/.test(value); +} + /** * Check if a position in the input is inside a shell comment. */ @@ -79,8 +285,7 @@ function findGit(input, start) { * Returns { command, offset } where offset is the position right after the * subcommand keyword, so callers can scope flag checks to only that portion. */ -function detectGitCommand(input) { - let start = 0; +function detectGitCommand(input, start = 0) { while (start < input.length) { const git = findGit(input, start); if (!git) return null; @@ -106,7 +311,7 @@ function detectGitCommand(input) { const after = input[cmdIdx + cmd.length] || ' '; if (!/\s/.test(before)) { searchPos = cmdIdx + 1; continue; } if (!/[\s;&#|>)\]}"']/.test(after) && after !== '') { searchPos = cmdIdx + 1; continue; } - if (/[;|&]/.test(input.slice(git.idx + git.len, cmdIdx))) break; + if (/[;|]/.test(input.slice(git.idx + git.len, cmdIdx))) break; if (isInComment(input, cmdIdx)) { searchPos = cmdIdx + 1; continue; } // Verify this token is the first non-flag word after "git" — i.e. the @@ -141,7 +346,13 @@ function detectGitCommand(input) { } if (bestCmd) { - return { command: bestCmd, offset: bestIdx + bestCmd.length }; + return { + command: bestCmd, + offset: bestIdx + bestCmd.length, + gitStart: git.idx, + gitEnd: git.idx + git.len, + commandStart: bestIdx, + }; } start = git.idx + git.len; @@ -149,20 +360,6 @@ function detectGitCommand(input) { return null; } -/** - * Replace the contents of single- and double-quoted strings with empty - * quotes. Used to neutralize commit messages and other quoted args before - * we run the flag-detection regexes — without this, a benign command like - * git commit -m "fix: --no-verify edge case" - * would falsely match `--no-verify` inside the message and block the - * commit. Backslash-escaped quotes inside the string are honored. - */ -function stripQuotedStrings(input) { - return input - .replace(/'(?:[^'\\]|\\.)*'/g, "''") - .replace(/"(?:[^"\\]|\\.)*"/g, '""'); -} - /** * Check if the input contains a --no-verify flag for a specific git command. * Only inspects the portion of the input starting at `offset` (the position @@ -170,12 +367,39 @@ function stripQuotedStrings(input) { * earlier commands in a chain are not falsely matched. */ function hasNoVerifyFlag(input, command, offset) { - const region = stripQuotedStrings(input.slice(offset)); - if (/--no-verify\b/.test(region)) return true; + const segmentEnd = findCommandSegmentEnd(input, offset); + const tokens = tokenizeShellWords(input, offset, segmentEnd); + let skipNext = false; - // For commit, -n is shorthand for --no-verify - if (command === 'commit') { - if (/\s-n(?:\s|$)/.test(region) || /\s-n[a-zA-Z]/.test(region)) return true; + for (const token of tokens) { + const value = token.value; + + if (skipNext) { + skipNext = false; + continue; + } + + if (value === '--') { + break; + } + + if (command === 'commit') { + if (commitOptionConsumesNextValue(value)) { + skipNext = true; + continue; + } + + if (commitOptionContainsInlineValue(value)) { + continue; + } + } + + if (value === '--no-verify') return true; + + // For commit, -n is shorthand for --no-verify. + if (command === 'commit' && isCommitNoVerifyShortFlag(value)) { + return true; + } } return false; @@ -183,34 +407,57 @@ function hasNoVerifyFlag(input, command, offset) { /** * Check if the input contains a -c core.hooksPath= override. - * Quoted strings are stripped first to avoid false positives on commit - * messages or argument values that happen to contain the phrase. */ -function hasHooksPathOverride(input) { - return /-c\s+["']?core\.hooksPath\s*=/.test(stripQuotedStrings(input)); +function hasHooksPathOverride(input, detected) { + const tokens = tokenizeShellWords(input, detected.gitEnd, detected.commandStart); + + for (let i = 0; i < tokens.length; i++) { + const value = tokens[i].value; + + if (value === '-c') { + const next = tokens[i + 1] && tokens[i + 1].value; + if (typeof next === 'string' && next.startsWith(GIT_CONFIG_KEY_PREFIX)) { + return true; + } + i++; + continue; + } + + if (value.startsWith(`-c${GIT_CONFIG_KEY_PREFIX}`)) { + return true; + } + } + + return false; } /** * Check a command string for git hook bypass attempts. */ function checkCommand(input) { - const detected = detectGitCommand(input); - if (!detected) return { blocked: false }; + let start = 0; - const { command: gitCommand, offset } = detected; + while (start < input.length) { + const detected = detectGitCommand(input, start); + if (!detected) return { blocked: false }; - if (hasNoVerifyFlag(input, gitCommand, offset)) { - return { - blocked: true, - reason: `BLOCKED: --no-verify flag is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`, - }; - } + const { command: gitCommand, offset } = detected; - if (hasHooksPathOverride(input)) { - return { - blocked: true, - reason: `BLOCKED: Overriding core.hooksPath is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`, - }; + if (hasHooksPathOverride(input, detected)) { + return { + blocked: true, + reason: `BLOCKED: Overriding core.hooksPath is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`, + }; + } + + if (hasNoVerifyFlag(input, gitCommand, offset)) { + return { + blocked: true, + reason: `BLOCKED: --no-verify flag is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`, + }; + } + + start = findCommandSegmentEnd(input, offset) + 1; } return { blocked: false }; diff --git a/skills/openclaw-persona-forge/SKILL.md b/skills/openclaw-persona-forge/SKILL.md index 9e718cb..949cb06 100644 --- a/skills/openclaw-persona-forge/SKILL.md +++ b/skills/openclaw-persona-forge/SKILL.md @@ -1,14 +1,6 @@ --- name: openclaw-persona-forge -description: |- - 为 OpenClaw AI Agent 锻造完整的龙虾灵魂方案。根据用户偏好或随机抽卡， - 输出身份定位、灵魂描述(SOUL.md)、角色化底线规则、名字和头像生图提示词。 - 如当前环境提供已审核的生图 skill，可自动生成统一风格头像图片。 - 当用户需要创建、设计或定制 OpenClaw 龙虾灵魂时使用。 - 不适用于：微调已有 SOUL.md、非 OpenClaw 平台的角色设计、纯工具型无性格 Agent。 - 触发词：龙虾灵魂、虾魂、OpenClaw 灵魂、养虾灵魂、龙虾角色、龙虾定位、 - 龙虾剧本杀角色、龙虾游戏角色、龙虾 NPC、龙虾性格、龙虾背景故事、 - lobster soul、lobster character、抽卡、随机龙虾、龙虾 SOUL、gacha。 +description: "为 OpenClaw AI Agent 锻造完整的龙虾灵魂方案。根据用户偏好或随机抽卡， 输出身份定位、灵魂描述(SOUL.md)、角色化底线规则、名字和头像生图提示词。 如当前环境提供已审核的生图 skill，可自动生成统一风格头像图片。 当用户需要创建、设计或定制 OpenClaw 龙虾灵魂时使用。 不适用于：微调已有 SOUL.md、非 OpenClaw 平台的角色设计、纯工具型无性格 Agent。 触发词：龙虾灵魂、虾魂、OpenClaw 灵魂、养虾灵魂、龙虾角色、龙虾定位、 龙虾剧本杀角色、龙虾游戏角色、龙虾 NPC、龙虾性格、龙虾背景故事、 lobster soul、lobster character、抽卡、随机龙虾、龙虾 SOUL、gacha。" origin: community --- diff --git a/skills/skill-stocktake/SKILL.md b/skills/skill-stocktake/SKILL.md index d5b7047..b036ce8 100644 --- a/skills/skill-stocktake/SKILL.md +++ b/skills/skill-stocktake/SKILL.md @@ -1,4 +1,5 @@ --- +name: skill-stocktake description: "Use when auditing Gemini skills and commands for quality. Supports Quick Scan (changed skills only) and Full Stocktake modes with sequential subagent batch evaluation." origin: ECC --- diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index de3ec2b..a45a319 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -141,6 +141,41 @@ if (test('still blocks --no-verify when both quoted message and real flag are pr assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); })) passed++; else failed++; +// --- Shell-words parsing fixes (ECC 0dcde13) --- + +if (test('blocks quoted core.hooksPath override argument', () => { + const r = runHook({ tool_input: { command: 'git -c "core.hooksPath=/dev/null" commit -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('core.hooksPath'), `stderr should mention core.hooksPath: ${r.stderr}`); +})) passed++; else failed++; + +if (test('allows --no-verify after combined -am message option', () => { + const r = runHook({ tool_input: { command: 'git commit -am "--no-verify"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('allows -n after combined -am message option', () => { + const r = runHook({ tool_input: { command: 'git commit -am "-n"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('allows git bypass phrase discussed in a quoted commit message', () => { + const r = runHook({ tool_input: { command: 'git commit -m "doc: explain git push --no-verify risk"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('still blocks a real quoted --no-verify flag', () => { + const r = runHook({ tool_input: { command: 'git commit "--no-verify" -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('BLOCKED'), `stderr should contain BLOCKED: ${r.stderr}`); +})) passed++; else failed++; + +if (test('still blocks bypass flags in later chained git commands', () => { + const r = runHook({ tool_input: { command: 'git commit -m "msg" && git push --no-verify' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('git push'), `stderr should mention git push: ${r.stderr}`); +})) passed++; else failed++; + console.log('─'.repeat(50)); console.log(`Passed: ${passed} Failed: ${failed}`); From e2d73888d3255f4e2439ffceef417ae965b49c43 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Tue, 12 May 2026 16:19:50 +0900 Subject: [PATCH 2/3] fix: address PR #68 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0 — combined short option bypass in block-no-verify (CodeRabbit): isCommitNoVerifyShortFlag only matched when 'n' was the first character after the dash. Tokens like '-an' (== '-a -n' in git semantics) or '-na' slipped through because the regex was /^-n[a-zA-Z]/. Rewritten to scan every character of a combined short-option token until either an 'n' is found (bypass) or a value-consuming flag like -m / -F / -C / -c is hit (which ends the option chain, the rest becomes the inline value). So '-mn' still allows ('n' is part of the -m message), '-am' still allows (no 'n' before the message-consuming 'm'), but '-an' / '-na' / '-abcn' are now caught. P1 — quoted-empty / YAML-null name bypass (CodeRabbit): inspectFrontmatter leaves raw scalar values as literal strings, so `name: ""` produced values.name === '""' (length 2, not 0) and slipped past the `values.name === ''` check. Same for `''`, `null`, and `~`. Added normalizeYamlScalar() that strips matched outer quotes and treats `null` / `~` as empty before the check. Tests (+3 in block-no-verify, 26 total): - blocks -n hidden inside combined short option (-an) - blocks -n hidden inside combined short option (-na) - still allows -mn (n is inside the -m message, not a flag) Lint clean. 273/270 -> 273 total tests pass. --- scripts/ci/validate-skills.js | 24 +++++++++++++++++++++++- scripts/hooks/block-no-verify.js | 16 +++++++++++++++- tests/hooks/block-no-verify.test.js | 15 +++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/scripts/ci/validate-skills.js b/scripts/ci/validate-skills.js index 6ffc853..c253ce9 100644 --- a/scripts/ci/validate-skills.js +++ b/scripts/ci/validate-skills.js @@ -111,6 +111,28 @@ function inspectFrontmatter(lines) { return { values, descriptionIndicator }; } +/** + * Normalize a YAML scalar value before treating it as empty. Catches + * quoted-empty (`""`, `''`) and YAML null forms (`~`, `null`) that + * `inspectFrontmatter` leaves as literal strings — without this, a + * frontmatter line like `name: ""` or `name: null` would pass the + * non-empty check even though both encode the absence of a value. + * + * @param {string} value + * @returns {string} + */ +function normalizeYamlScalar(value) { + const v = value.trim(); + if (/^(?:~|null)$/i.test(v)) return ''; + if ( + (v.startsWith('"') && v.endsWith('"') && v.length >= 2) || + (v.startsWith("'") && v.endsWith("'") && v.length >= 2) + ) { + return v.slice(1, -1).trim(); + } + return v; +} + /** * Validate a single skill directory. * @@ -150,7 +172,7 @@ function validateSkillDir(dir, skillsDir, reportFrontmatterFinding) { if (!Object.prototype.hasOwnProperty.call(values, 'name')) { reportFrontmatterFinding(`${dir}/SKILL.md - frontmatter missing required field: name`); - } else if (values.name === '') { + } else if (normalizeYamlScalar(values.name) === '') { reportFrontmatterFinding(`${dir}/SKILL.md - frontmatter 'name' is empty`); } diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 84979b9..71f9b59 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -238,7 +238,21 @@ function getCommitShortValueOption(value) { } function isCommitNoVerifyShortFlag(value) { - return value === '-n' || /^-n[a-zA-Z]/.test(value); + if (value === '-n') return true; + if (!value.startsWith('-') || value.startsWith('--') || value === '-') { + return false; + } + // Scan every character of a combined short-option token (e.g. -an, -na). + // A value-consuming flag like -m or -F (case-sensitive: 'm', 'F', 'C', 'c') + // ends the option chain — characters after it are the inline value, not + // further flags, so we must stop scanning there. + const options = value.slice(1); + for (let i = 0; i < options.length; i++) { + const char = options.charAt(i); + if (char === 'n') return true; + if (COMMIT_SHORT_OPTIONS_WITH_VALUE.has(char)) return false; + } + return false; } /** diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index a45a319..cd67f8d 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -176,6 +176,21 @@ if (test('still blocks bypass flags in later chained git commands', () => { assert.ok(r.stderr.includes('git push'), `stderr should mention git push: ${r.stderr}`); })) passed++; else failed++; +if (test('blocks -n hidden inside combined short option (-an)', () => { + const r = runHook({ tool_input: { command: 'git commit -an -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); +})) passed++; else failed++; + +if (test('blocks -n hidden inside combined short option (-na)', () => { + const r = runHook({ tool_input: { command: 'git commit -na -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); +})) passed++; else failed++; + +if (test('still allows -mn (n is inside -m message, not a flag)', () => { + const r = runHook({ tool_input: { command: 'git commit -mn' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + console.log('─'.repeat(50)); console.log(`Passed: ${passed} Failed: ${failed}`); From fbf7908b743d95b4d11d9976ef3de63c91abbeb0 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Tue, 12 May 2026 16:43:25 +0900 Subject: [PATCH 3/3] fix: address round-3 review on block-no-verify (case + -t) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0 — core.hooksPath case-insensitivity bypass (CodeRabbit): Git config section and variable names are case-insensitive per git-scm.com/docs/git-config, so 'git -c core.hookspath=/dev/null commit ...' or 'core.HOOKSPATH=...' bypassed the previous guard which compared against the literal 'core.hooksPath=' constant. Switched GIT_CONFIG_KEY_PREFIX to the lowercase form and lowercase the candidate tokens in hasHooksPathOverride before matching, in both the '-c ' and '-c' code paths. P1 — -t template short option missing (CodeRabbit): COMMIT_SHORT_OPTIONS_WITH_VALUE was missing 't'. Git parses 'git commit -tn' as -t with template path 'n' (stuck-arg form), not '-t -n'. Without 't' in the set, the scanner reached the 'n' check and falsely flagged the command as a --no-verify bypass. Added 't' to the set with a comment explaining the stuck-arg semantics. Tests (+3, 29 total in this file): - still allows -tn (n is the -t template path, not a flag) - blocks case-variant core.hooksPath (lowercase) - blocks case-variant core.hooksPath (uppercase) Lint clean. 276/276 across the suite. --- scripts/hooks/block-no-verify.js | 22 ++++++++++++++++++---- tests/hooks/block-no-verify.test.js | 16 ++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 71f9b59..3129467 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -35,7 +35,12 @@ const GIT_COMMANDS_WITH_NO_VERIFY = [ */ const VALID_BEFORE_GIT = ' \t\n\r;&|$`(<{!"\']/.~\\'; -const GIT_CONFIG_KEY_PREFIX = 'core.hooksPath='; +// Git config section and variable names are case-insensitive +// (subsection names are case-sensitive but core.hooksPath has none), +// so we normalize the candidate token to lowercase before matching. +// See https://git-scm.com/docs/git-config — "The variable names are +// case-insensitive." +const GIT_CONFIG_KEY_PREFIX = 'core.hookspath='; const COMMIT_OPTIONS_WITH_VALUE = new Set([ '-m', @@ -67,7 +72,12 @@ const COMMIT_OPTIONS_WITH_INLINE_VALUE = [ '--pathspec-from-file=', ]; -const COMMIT_SHORT_OPTIONS_WITH_VALUE = new Set(['m', 'F', 'C', 'c']); +// Short options that take a value. When seen as part of a combined +// short-option token (e.g. -tn), git's parser treats the rest of the +// token as the option's value (template path 'n' here), so the scanner +// must stop at this character — anything after it is the inline value, +// not another flag. +const COMMIT_SHORT_OPTIONS_WITH_VALUE = new Set(['m', 'F', 'C', 'c', 't']); function tokenizeShellWords(input, start = 0, end = input.length) { const tokens = []; @@ -427,17 +437,21 @@ function hasHooksPathOverride(input, detected) { for (let i = 0; i < tokens.length; i++) { const value = tokens[i].value; + // Git config section + variable names are case-insensitive, so a + // bypass attempt like `core.HOOKSPATH=...` or `core.hookspath=...` + // must compare against the lowercased token. + const lowered = value.toLowerCase(); if (value === '-c') { const next = tokens[i + 1] && tokens[i + 1].value; - if (typeof next === 'string' && next.startsWith(GIT_CONFIG_KEY_PREFIX)) { + if (typeof next === 'string' && next.toLowerCase().startsWith(GIT_CONFIG_KEY_PREFIX)) { return true; } i++; continue; } - if (value.startsWith(`-c${GIT_CONFIG_KEY_PREFIX}`)) { + if (lowered.startsWith(`-c${GIT_CONFIG_KEY_PREFIX}`)) { return true; } } diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index cd67f8d..2b5c3d2 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -191,6 +191,22 @@ if (test('still allows -mn (n is inside -m message, not a flag)', () => { assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); })) passed++; else failed++; +if (test('still allows -tn (n is the -t template path, not a flag)', () => { + const r = runHook({ tool_input: { command: 'git commit -tn -m "msg"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks case-variant core.hooksPath (lowercase)', () => { + const r = runHook({ tool_input: { command: 'git -c core.hookspath=/dev/null commit -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(/core\.hooksPath/i.test(r.stderr), `stderr should mention core.hooksPath: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks case-variant core.hooksPath (uppercase)', () => { + const r = runHook({ tool_input: { command: 'git -c core.HOOKSPATH=/dev/null commit -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); +})) passed++; else failed++; + console.log('─'.repeat(50)); console.log(`Passed: ${passed} Failed: ${failed}`);