From 30c41a9bde3614d92fcc2ed5331d6198b6f613d6 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Sat, 29 Aug 2026 14:42:59 -0400 Subject: [PATCH] fix: close truth and portability review gaps --- scripts/ci/validate-skills.js | 92 +++++++++++++------ scripts/lib/transcript-context.js | 11 ++- skills/skill-comply/pyproject.toml | 1 + skills/skill-comply/scripts/runner.py | 18 +++- skills/skill-comply/tests/test_runner.py | 22 +++++ skills/skill-stocktake/scripts/quick-diff.sh | 25 ++++- skills/skill-stocktake/scripts/scan.sh | 25 ++++- tests/ci/gateguard-env-documented.test.js | 16 +++- tests/ci/validators.test.js | 25 +++++ tests/hooks/suggest-compact.test.js | 5 +- tests/lib/transcript-context.test.js | 16 +++- .../scripts/skill-stocktake-discovery.test.js | 7 +- 12 files changed, 216 insertions(+), 47 deletions(-) diff --git a/scripts/ci/validate-skills.js b/scripts/ci/validate-skills.js index 1ae0e67ce..6e7c3a64b 100644 --- a/scripts/ci/validate-skills.js +++ b/scripts/ci/validate-skills.js @@ -68,9 +68,41 @@ function extractFrontmatter(content) { * @param {string[]} lines * @returns {{values: Record, descriptionIndicator: string|null}} */ +function stripUnquotedYamlComment(rawValue) { + let inSingleQuote = false; + let inDoubleQuote = false; + + for (let index = 0; index < rawValue.length; index++) { + const character = rawValue[index]; + + if (inDoubleQuote && character === '\\') { + index += 1; + continue; + } + if (!inDoubleQuote && character === "'") { + if (inSingleQuote && rawValue[index + 1] === "'") { + index += 1; + } else { + inSingleQuote = !inSingleQuote; + } + continue; + } + if (!inSingleQuote && character === '"') { + inDoubleQuote = !inDoubleQuote; + continue; + } + if (!inSingleQuote && !inDoubleQuote && character === '#' + && (index === 0 || /\s/.test(rawValue[index - 1]))) { + return rawValue.slice(0, index).trim(); + } + } + + return rawValue.trim(); +} + function inspectFrontmatter(lines) { const values = Object.create(null); - const syntaxErrors = []; + let syntaxErrors = []; let descriptionIndicator = null; let inBlockScalar = false; let blockScalarIndent = -1; @@ -92,13 +124,8 @@ function inspectFrontmatter(lines) { 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(); + // Strip YAML comments only when # appears outside a quoted scalar. + const valueNoComment = stripUnquotedYamlComment(rawValue); values[key] = valueNoComment; const isQuoted = /^"(?:[^"\\]|\\.)*"$/.test(valueNoComment) || /^'(?:[^']|'')*'$/.test(valueNoComment); @@ -109,16 +136,19 @@ function inspectFrontmatter(lines) { // drops a value's quoting, or glues the next frontmatter key onto // the end of a value, this is exactly what shows up (see #2630). if (valueNoComment.includes(': ')) { - syntaxErrors.push( + syntaxErrors = [...syntaxErrors, `${key}: unquoted value contains ': ' — invalid YAML; ` + `quote the value or the next key was likely glued onto this line` - ); + ]; } // '@' and '`' are reserved YAML indicators and cannot start a // plain scalar (see #2630 — a reordering during translation moved // '@' into the first column of an unquoted description). if (/^[@`]/.test(valueNoComment)) { - syntaxErrors.push(`${key}: unquoted value starts with reserved character '${valueNoComment[0]}' — quote the value`); + syntaxErrors = [ + ...syntaxErrors, + `${key}: unquoted value starts with reserved character '${valueNoComment[0]}' — quote the value` + ]; } } @@ -246,30 +276,31 @@ function validateSkillFile(skillMd, label, reportFrontmatterFinding, opts = {}) function findDocsSkillFiles(docsDir) { if (!fs.existsSync(docsDir)) return []; - const files = []; - const locales = fs - .readdirSync(docsDir, { withFileTypes: true }) + const readDirectories = (directory, label) => { + try { + return fs.readdirSync(directory, { withFileTypes: true }); + } catch { + throw new Error(`unable to read ${label}`); + } + }; + + const locales = readDirectories(docsDir, 'docs directory') .filter(e => e.isDirectory() && !e.name.startsWith('.')) .map(e => e.name); - for (const locale of locales) { + return locales.flatMap(locale => { const localeSkillsDir = path.join(docsDir, locale, 'skills'); - if (!fs.existsSync(localeSkillsDir)) continue; + if (!fs.existsSync(localeSkillsDir)) return []; - const skillDirs = fs - .readdirSync(localeSkillsDir, { withFileTypes: true }) + const skillDirs = readDirectories(localeSkillsDir, `docs/${locale}/skills directory`) .filter(e => e.isDirectory() && !e.name.startsWith('.')) .map(e => e.name); - for (const skillDir of skillDirs) { - files.push({ - skillMd: path.join(localeSkillsDir, skillDir, 'SKILL.md'), - label: `docs/${locale}/skills/${skillDir}/SKILL.md` - }); - } - } - - return files; + return skillDirs.map(skillDir => ({ + skillMd: path.join(localeSkillsDir, skillDir, 'SKILL.md'), + label: `docs/${locale}/skills/${skillDir}/SKILL.md` + })); + }); } function validateSkills() { @@ -329,4 +360,9 @@ function validateSkills() { console.log(msg); } -validateSkills(); +try { + validateSkills(); +} catch (error) { + console.error(`ERROR: ${error.message}`); + process.exit(1); +} diff --git a/scripts/lib/transcript-context.js b/scripts/lib/transcript-context.js index d0a944330..853a35b2a 100644 --- a/scripts/lib/transcript-context.js +++ b/scripts/lib/transcript-context.js @@ -162,10 +162,11 @@ function readLatestContextTokens(transcriptPath, options = {}) { * positively detected or merely assumed. * * `inferred: false` means the size came from evidence — an explicit env - * override, the `[1m]` marker, a known large-window family, or an observed - * token count that already exceeds the standard window. `inferred: true` means - * every check fell through and the standard 200k default was assumed; the - * window may actually be larger and callers must not present it as fact. + * override, the `[1m]` marker, or a known large-window family. An observed + * token count above the standard window selects the safer large-window + * thresholds, but remains inferred because the true denominator could be an + * unmarked intermediate size such as 400k. Callers must not present inferred + * windows as fact. * * @returns {{ windowTokens: number, inferred: boolean }} */ @@ -193,7 +194,7 @@ function resolveContextWindow(tokens, model) { } if (Number.isFinite(tokens) && tokens > STANDARD_CONTEXT_WINDOW_TOKENS) { - return { windowTokens: LARGE_CONTEXT_WINDOW_TOKENS, inferred: false }; + return { windowTokens: LARGE_CONTEXT_WINDOW_TOKENS, inferred: true }; } return { windowTokens: STANDARD_CONTEXT_WINDOW_TOKENS, inferred: true }; diff --git a/skills/skill-comply/pyproject.toml b/skills/skill-comply/pyproject.toml index 323185cef..3584f8262 100644 --- a/skills/skill-comply/pyproject.toml +++ b/skills/skill-comply/pyproject.toml @@ -8,6 +8,7 @@ dependencies = ["pyyaml>=6.0"] [tool.pytest.ini_options] testpaths = ["tests"] pythonpath = ["."] +markers = ["unit: isolated tests without external services"] [dependency-groups] dev = [ diff --git a/skills/skill-comply/scripts/runner.py b/skills/skill-comply/scripts/runner.py index 4a852a57e..ffad8447f 100644 --- a/skills/skill-comply/scripts/runner.py +++ b/skills/skill-comply/scripts/runner.py @@ -149,11 +149,25 @@ def _redact_home_path(text: str) -> str: def _redact_home_paths(value: object) -> object: - """Return a copy with home paths redacted from every string leaf.""" + """Return a copy with home paths redacted from string keys and leaves. + + Redacted mapping keys receive a stable numeric suffix when two original + keys collapse to the same portable value. This preserves every observation + without leaking the original home path or silently dropping data. + """ if isinstance(value, str): return _redact_home_path(value) if isinstance(value, dict): - return {key: _redact_home_paths(item) for key, item in value.items()} + redacted: dict[object, object] = {} + for key, item in value.items(): + redacted_key = _redact_home_path(key) if isinstance(key, str) else key + candidate = redacted_key + suffix = 2 + while candidate in redacted: + candidate = f"{redacted_key}#{suffix}" + suffix += 1 + redacted[candidate] = _redact_home_paths(item) + return redacted if isinstance(value, list): return [_redact_home_paths(item) for item in value] return value diff --git a/skills/skill-comply/tests/test_runner.py b/skills/skill-comply/tests/test_runner.py index a45270fc1..f8141d184 100644 --- a/skills/skill-comply/tests/test_runner.py +++ b/skills/skill-comply/tests/test_runner.py @@ -144,6 +144,7 @@ class TestRunScenarioMaxTurnsTermination: run_scenario(scenario, model="haiku") +@pytest.mark.unit class TestParseStreamJsonRedactsHomePath: """Observations feed grade() and then a written report (results/.md) — a raw absolute path bakes the operator's username into every tool call @@ -209,6 +210,27 @@ class TestParseStreamJsonRedactsHomePath: "~/資料.txt", ] + def test_mapping_keys_are_redacted_without_silent_collision( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + home = r"C:\Users\Zoë" + self._set_home(monkeypatch, home) + stdout = self._stream_json_for( + { + home + r"\private.txt": "first", + r"c:\users\zoë\private.txt": "second", + }, + "irrelevant output", + ) + events = _parse_stream_json(stdout) + + parsed_input = json.loads(events[0].input) + assert home not in events[0].input + assert parsed_input == { + r"~\private.txt": "first", + r"~\private.txt#2": "second", + } + def test_sibling_and_embedded_prefix_paths_untouched( self, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/skills/skill-stocktake/scripts/quick-diff.sh b/skills/skill-stocktake/scripts/quick-diff.sh index f929b2432..418b02558 100755 --- a/skills/skill-stocktake/scripts/quick-diff.sh +++ b/skills/skill-stocktake/scripts/quick-diff.sh @@ -13,6 +13,27 @@ set -euo pipefail +sort_nul_file() { + local input_file="$1" + local sorted_file="${input_file}.sorted" + node -e ' + const fs = require("fs"); + const input = fs.readFileSync(0); + const records = []; + let start = 0; + for (let index = 0; index < input.length; index += 1) { + if (input[index] === 0) { + records.push(input.subarray(start, index + 1)); + start = index + 1; + } + } + if (start < input.length) records.push(input.subarray(start)); + records.sort(Buffer.compare); + process.stdout.write(Buffer.concat(records)); + ' <"$input_file" >"$sorted_file" + mv "$sorted_file" "$input_file" +} + RESULTS_JSON="${1:-}" CWD_SKILLS_DIR="${SKILL_STOCKTAKE_PROJECT_DIR:-${2:-$PWD/.claude/skills}}" GLOBAL_DIR="${SKILL_STOCKTAKE_GLOBAL_DIR:-$HOME/.claude/skills}" @@ -56,13 +77,13 @@ process_dir() { # Capture find's exit status and stderr instead of discarding them: with -L, # a broken symlink or unreadable directory makes find skip that entry AND # exit non-zero, which would otherwise silently under-count skills. - # NUL-delimited (-print0 / sort -z / read -d '') so a path containing a + # NUL-delimited (-print0 / sort_nul_file / read -d '') so a path containing a # literal newline can't desync record boundaries — paths here are untrusted. if ! find -L "$dir" -name "SKILL.md" -type f -print0 >"$find_out" 2>"$find_err"; then echo "Warning: find encountered errors while scanning $dir (broken symlinks or permission issues may cause skills to be missed):" >&2 cat "$find_err" >&2 fi - sort -z -o "$find_out" "$find_out" + sort_nul_file "$find_out" while IFS= read -r -d '' file; do local mtime dp is_new diff --git a/skills/skill-stocktake/scripts/scan.sh b/skills/skill-stocktake/scripts/scan.sh index 76f5523dc..e43509edc 100755 --- a/skills/skill-stocktake/scripts/scan.sh +++ b/skills/skill-stocktake/scripts/scan.sh @@ -13,6 +13,27 @@ set -euo pipefail +sort_nul_file() { + local input_file="$1" + local sorted_file="${input_file}.sorted" + node -e ' + const fs = require("fs"); + const input = fs.readFileSync(0); + const records = []; + let start = 0; + for (let index = 0; index < input.length; index += 1) { + if (input[index] === 0) { + records.push(input.subarray(start, index + 1)); + start = index + 1; + } + } + if (start < input.length) records.push(input.subarray(start)); + records.sort(Buffer.compare); + process.stdout.write(Buffer.concat(records)); + ' <"$input_file" >"$sorted_file" + mv "$sorted_file" "$input_file" +} + GLOBAL_DIR="${SKILL_STOCKTAKE_GLOBAL_DIR:-$HOME/.claude/skills}" CWD_SKILLS_DIR="${SKILL_STOCKTAKE_PROJECT_DIR:-${1:-$PWD/.claude/skills}}" # Path to JSONL file containing tool-use observations (optional; used for usage frequency counts). @@ -100,13 +121,13 @@ scan_dir_to_json() { # Capture find's exit status and stderr instead of discarding them: with -L, # a broken symlink or unreadable directory makes find skip that entry AND # exit non-zero, which would otherwise silently under-count skills. - # NUL-delimited (-print0 / sort -z / read -d '') so a path containing a + # NUL-delimited (-print0 / sort_nul_file / read -d '') so a path containing a # literal newline can't desync record boundaries — paths here are untrusted. if ! find -L "$dir" -name "SKILL.md" -type f -print0 >"$find_out" 2>"$find_err"; then echo "Warning: find encountered errors while scanning $dir (broken symlinks or permission issues may cause skills to be missed):" >&2 cat "$find_err" >&2 fi - sort -z -o "$find_out" "$find_out" + sort_nul_file "$find_out" while IFS= read -r -d '' file; do local name desc mtime u7 u30 dp diff --git a/tests/ci/gateguard-env-documented.test.js b/tests/ci/gateguard-env-documented.test.js index 6ee96754c..28400cbea 100644 --- a/tests/ci/gateguard-env-documented.test.js +++ b/tests/ci/gateguard-env-documented.test.js @@ -52,6 +52,15 @@ function test(name, fn) { const REGEX_CAN_FOLLOW = new Set([ '', '(', ',', '=', ':', '[', '!', '&', '|', '?', '{', '}', ';', '+', '-', '*', '%', '~', '^', '<', '>', ]); +const REGEX_CAN_FOLLOW_KEYWORD = new Set([ + 'await', 'case', 'delete', 'do', 'else', 'in', 'instanceof', 'new', 'of', + 'return', 'throw', 'typeof', 'void', 'yield', +]); + +function regexFollowsKeyword(source, slashIndex) { + const match = source.slice(0, slashIndex).match(/([A-Za-z_$][\w$]*)\s*$/); + return Boolean(match && REGEX_CAN_FOLLOW_KEYWORD.has(match[1])); +} /** * Blank out comments and literal text, preserving length and line breaks so @@ -102,7 +111,7 @@ function blankCommentsAndLiterals(source) { continue; } - if (ch === '/' && REGEX_CAN_FOLLOW.has(prev)) { + if (ch === '/' && (REGEX_CAN_FOLLOW.has(prev) || regexFollowsKeyword(source, i))) { emit(ch); i += 1; let inClass = false; while (i < source.length) { @@ -291,6 +300,11 @@ if (test('a regex literal containing a slash does not swallow the code after it' assert.deepStrictEqual([...readGateguardEnvNames(fixture)], ['GATEGUARD_AFTER_REGEX']); })) passed++; else failed++; +if (test('a regex literal after a statement keyword is ignored', () => { + const fixture = 'function matches() { return /process\\.env\\.GATEGUARD_IN_RETURN_REGEX/; }'; + assert.deepStrictEqual([...readGateguardEnvNames(fixture)], []); +})) passed++; else failed++; + if (test('the access guard rejects every form the parser cannot follow', () => { const cases = [ ['destructuring', 'const { GATEGUARD_HIDDEN } = process.env;'], diff --git a/tests/ci/validators.test.js b/tests/ci/validators.test.js index e6d950161..4bcb9452a 100644 --- a/tests/ci/validators.test.js +++ b/tests/ci/validators.test.js @@ -2856,6 +2856,31 @@ function runTests() { cleanupTestDir(testDir); })) passed++; else failed++; + if (test('preserves # inside a quoted frontmatter value', () => { + const testDir = createTestDir(); + const docsDir = path.join(testDir, 'docs-root'); + const skillDir = path.join(docsDir, 'ja-JP', 'skills', 'example'); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\nname: example\ndescription: "Fix: details #tag" # translation note\n---\n# Example'); + + const result = runSkillsValidator('/nonexistent/skills-dir', ['--strict'], {}, docsDir); + assert.strictEqual(result.code, 0, + `Quoted # content must remain valid, got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('reports an unreadable docs root deterministically', () => { + const testDir = createTestDir(); + const docsPath = path.join(testDir, 'docs-file'); + fs.writeFileSync(docsPath, 'not a directory'); + + const result = runSkillsValidator('/nonexistent/skills-dir', ['--strict'], {}, docsPath); + assert.strictEqual(result.code, 1, 'Should fail when the docs root cannot be read'); + assert.strictEqual(result.stderr.trim(), 'ERROR: unable to read docs directory'); + cleanupTestDir(testDir); + })) passed++; else failed++; + if (test('flags a docs mirror SKILL.md with no frontmatter block at all', () => { const testDir = createTestDir(); const docsDir = path.join(testDir, 'docs-root'); diff --git a/tests/hooks/suggest-compact.test.js b/tests/hooks/suggest-compact.test.js index 6036442d3..5b9d2324d 100644 --- a/tests/hooks/suggest-compact.test.js +++ b/tests/hooks/suggest-compact.test.js @@ -698,7 +698,10 @@ function runTests() { const ctx = createContextContext(); const transcript = writeTranscriptFixture(170000); try { - const result = runCompactWithInput({ session_id: ctx.sessionId, transcript_path: transcript }); + const result = runCompactWithInput( + { session_id: ctx.sessionId, transcript_path: transcript }, + { ECC_CONTEXT_WINDOW_TOKENS: '', CLAUDE_CODE_AUTO_COMPACT_WINDOW: '' }, + ); assert.strictEqual(result.code, 0, 'Should exit 0'); assert.ok(result.stdout.trim().length > 0, `Expected stdout payload. Got: "${result.stdout}"`); const parsed = JSON.parse(result.stdout); diff --git a/tests/lib/transcript-context.test.js b/tests/lib/transcript-context.test.js index f10f62c76..b5a3addf6 100644 --- a/tests/lib/transcript-context.test.js +++ b/tests/lib/transcript-context.test.js @@ -139,6 +139,10 @@ console.log('\nresolveContextWindowTokens:'); // Isolation: an env-set window override (either knob) otherwise leaks into the // default-window assertions below and fails them (#2290). +const originalContextWindowEnv = { + ECC_CONTEXT_WINDOW_TOKENS: process.env.ECC_CONTEXT_WINDOW_TOKENS, + CLAUDE_CODE_AUTO_COMPACT_WINDOW: process.env.CLAUDE_CODE_AUTO_COMPACT_WINDOW, +}; delete process.env.ECC_CONTEXT_WINDOW_TOKENS; delete process.env.CLAUDE_CODE_AUTO_COMPACT_WINDOW; @@ -222,9 +226,6 @@ test('treats an empty model id as standard window', () => { // ── isContextWindowInferred ── console.log('\nisContextWindowInferred:'); -delete process.env.ECC_CONTEXT_WINDOW_TOKENS; -delete process.env.CLAUDE_CODE_AUTO_COMPACT_WINDOW; - test('flags the assumed 200k default as inferred', () => { assert.strictEqual(isContextWindowInferred(187000, 'claude-opus-9'), true); }); @@ -246,10 +247,15 @@ test('a known large-window family is a detected window, not inferred', () => { assert.strictEqual(isContextWindowInferred(187000, 'claude-fable-5'), false); }); -test('tokens above the standard window make the size detected, not inferred', () => { - assert.strictEqual(isContextWindowInferred(220000, 'claude-opus-9'), false); +test('tokens above the standard window still leave the exact size inferred', () => { + assert.strictEqual(isContextWindowInferred(220000, 'claude-opus-9'), true); }); +for (const [name, value] of Object.entries(originalContextWindowEnv)) { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; +} + // ── resolveContextThreshold ── console.log('\nresolveContextThreshold:'); diff --git a/tests/scripts/skill-stocktake-discovery.test.js b/tests/scripts/skill-stocktake-discovery.test.js index d1d36c0d5..498ed43fa 100644 --- a/tests/scripts/skill-stocktake-discovery.test.js +++ b/tests/scripts/skill-stocktake-discovery.test.js @@ -47,7 +47,9 @@ test('both scanners use canonical, error-visible, NUL-delimited discovery', () = for (const scriptPath of [scanScript, quickDiffScript]) { const source = fs.readFileSync(scriptPath, 'utf8'); assert.match(source, /find -L "\$dir" -name "SKILL\.md" -type f -print0/); - assert.match(source, /sort -z -o "\$find_out" "\$find_out"/); + assert.match(source, /sort_nul_file "\$find_out"/); + assert.match(source, /records\.sort\(Buffer\.compare\)/); + assert.doesNotMatch(source, /sort -z/, `${path.basename(scriptPath)} still requires GNU sort`); assert.match(source, /read -r -d '' file/); assert.doesNotMatch(source, /find [^\n]*2>\/dev\/null/, `${path.basename(scriptPath)} still hides find errors`); } @@ -103,6 +105,9 @@ if (process.platform === 'win32') { ); assert.ok(output.every(entry => entry.is_new === true)); }); + } catch (error) { + console.log(` ✗ fixture setup: ${error.message}`); + failed++; } finally { fs.rmSync(tempRoot, { recursive: true, force: true }); }