From 8d597276331cde4e1e198744eac7157b27a556d0 Mon Sep 17 00:00:00 2001 From: pablolozano0216-art Date: Mon, 21 Sep 2026 13:32:21 -0500 Subject: [PATCH 1/7] fix(skills): confine frontend-slides export server to the deck directory and bind loopback (#3206) The temporary HTTP server that export-pdf.sh spins up to render a deck joined the decoded request path onto SERVE_DIR without checking the resolved path, and listened on all interfaces. A request such as `/..%2f..%2f..%2fetc%2fpasswd` (raw, or issued by a malicious deck via fetch() while it renders) returned files outside the deck directory, and any host on the network could hit the port while an export was running. - resolve the decoded path against SERVE_ROOT and answer 403 when the result is not inside it (handles ../, %2e%2e, %2f and query strings) - answer 400 on malformed percent-encoding instead of throwing - listen on 127.0.0.1 only; the only client is the local headless browser Verified with raw HTTP requests against the handler: `/` and `/index.html` -> 200, `/../x`, `/..%2f..%2fx`, `/%2e%2e/x`, `/deck/../../x` -> 403, unknown file -> 404, `/%zz` -> 400, and server.address() reports 127.0.0.1. Fixes #3101 --- skills/frontend-slides/scripts/export-pdf.sh | 25 ++++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/skills/frontend-slides/scripts/export-pdf.sh b/skills/frontend-slides/scripts/export-pdf.sh index ee446b793..2ab289f89 100755 --- a/skills/frontend-slides/scripts/export-pdf.sh +++ b/skills/frontend-slides/scripts/export-pdf.sh @@ -136,7 +136,7 @@ cat > "$TEMP_SCRIPT" << 'EXPORT_SCRIPT' import { chromium } from 'playwright'; import { createServer } from 'http'; import { readFileSync, existsSync, mkdirSync, unlinkSync, writeFileSync } from 'fs'; -import { join, extname, resolve } from 'path'; +import { join, extname, resolve, sep } from 'path'; import { execSync } from 'child_process'; const SERVE_DIR = process.argv[2]; @@ -166,10 +166,25 @@ const MIME_TYPES = { '.eot': 'application/vnd.ms-fontobject', }; +// Every request is confined to the deck directory: resolve the decoded path +// against SERVE_ROOT and refuse anything that escapes it (../, %2e%2e, %2f). +const SERVE_ROOT = resolve(SERVE_DIR); const server = createServer((req, res) => { // Decode URL-encoded characters (e.g., %20 -> space) so filenames with spaces resolve correctly - const decodedUrl = decodeURIComponent(req.url); - let filePath = join(SERVE_DIR, decodedUrl === '/' ? HTML_FILE : decodedUrl); + let decodedUrl; + try { + decodedUrl = decodeURIComponent((req.url || '/').split('?')[0]); + } catch { + res.writeHead(400); + res.end('Bad request'); + return; + } + const filePath = resolve(SERVE_ROOT, '.' + (decodedUrl === '/' ? '/' + HTML_FILE : decodedUrl)); + if (filePath !== SERVE_ROOT && !filePath.startsWith(SERVE_ROOT + sep)) { + res.writeHead(403); + res.end('Forbidden'); + return; + } try { const content = readFileSync(filePath); const ext = extname(filePath).toLowerCase(); @@ -181,9 +196,9 @@ const server = createServer((req, res) => { } }); -// Find a free port +// Find a free port on loopback only; the deck is rendered by the local headless browser const port = await new Promise((resolve) => { - server.listen(0, () => resolve(server.address().port)); + server.listen(0, '127.0.0.1', () => resolve(server.address().port)); }); console.log(` Local server on port ${port}`); From d7cf4584cfe8499464b9e765b6e500158eaa6e94 Mon Sep 17 00:00:00 2001 From: asp <128797936+ASP-SuperExplorer@users.noreply.github.com> Date: Tue, 22 Sep 2026 02:32:26 +0800 Subject: [PATCH 2/7] fix: restore Claude plugin setup compatibility (#3205) * fix: stop passing removed Claude plugin config flags * test: cover current Claude plugin install arguments * docs: add beginner Windows install walkthrough --- README.md | 25 +++++++++++++++++++++++++ scripts/lib/claude-plugin-setup.js | 3 --- tests/lib/claude-plugin-setup.test.js | 6 +----- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index f9d202b28..9156c729a 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,31 @@ For Claude Code plugin setup, updates, scope changes, and hook-profile changes: npx ecc-universal@2.2.2 setup ``` +#### Windows first-time walkthrough + +If you are new to command-line tools, use this copy-and-paste path: + +1. Install Node.js 18 or newer, Git, and Claude Code. +2. Open **PowerShell** from the Windows Start menu. +3. Confirm that each prerequisite is available: + + ```powershell + node --version + git --version + claude --version + ``` + +4. Run the guided installer: + + ```powershell + npx ecc-universal@2.2.2 setup + ``` + +5. For a typical personal setup, choose **Global user**, choose **Standard** hooks, and confirm. +6. Start a new Claude Code session and run `/plugin list` to verify that `ecc@ecc` is enabled. + +This path does not require cloning the repository. If any prerequisite command is not found, install or repair that prerequisite before rerunning ECC setup. + If npm reports a version or cache error, confirm the registry version before retrying: ```bash diff --git a/scripts/lib/claude-plugin-setup.js b/scripts/lib/claude-plugin-setup.js index 0672b505e..63c116759 100644 --- a/scripts/lib/claude-plugin-setup.js +++ b/scripts/lib/claude-plugin-setup.js @@ -570,7 +570,6 @@ function verifyPluginAtScope(options) { function ensurePluginAtScope(options) { const run = options.run || runClaude; - const configuredHooks = options.hookConfiguration || hookOptions(options.hooks); if (options.installed) { run( ['plugin', 'update', CURRENT_PLUGIN_ID, '--scope', options.scope], @@ -582,8 +581,6 @@ function ensurePluginAtScope(options) { [ 'plugin', 'install', CURRENT_PLUGIN_ID, '--scope', options.scope, - '--config', `hooks_enabled=${configuredHooks.hooks_enabled}`, - '--config', `hook_profile=${configuredHooks.hook_profile}`, ], { cwd: options.projectRoot, phase: 'plugin-install' } ); diff --git a/tests/lib/claude-plugin-setup.test.js b/tests/lib/claude-plugin-setup.test.js index ba82d3870..cdda9f5f0 100644 --- a/tests/lib/claude-plugin-setup.test.js +++ b/tests/lib/claude-plugin-setup.test.js @@ -278,7 +278,7 @@ test('requesting another scope fails without the PR 2 move-scope operation', () }); }); -test('fresh install follows the exact inventory, marketplace, install, and verification sequence', () => { +test('fresh install uses supported Claude arguments and follows the verification sequence', () => { withFixture({}, fixture => { const result = setupClaudePlugin(setupOptions(fixture, { scope: 'project', @@ -294,8 +294,6 @@ test('fresh install follows the exact inventory, marketplace, install, and verif [ 'plugin', 'install', 'ecc@ecc', '--scope', 'project', - '--config', 'hooks_enabled=true', - '--config', 'hook_profile=strict', ], ['plugin', 'list', '--json'], ]); @@ -724,8 +722,6 @@ test('provider failures stop later operations and leave settings untouched', () const installArgv = [ 'plugin', 'install', 'ecc@ecc', '--scope', 'user', - '--config', 'hooks_enabled=true', - '--config', 'hook_profile=standard', ]; withFixture({ failures: [{ From c056ae7df2f110e77134526fe88d9bb4b7dacadd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sedat=20Da=C4=9F?= <83247545+sedatdagg@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:33:19 +0300 Subject: [PATCH 3/7] fix(hooks): support python style comments in pre-commit quality checks (#3194) * fix(hooks): support python style comments in pre-commit quality checks fix(hooks): support python style comments in pre-commit quality checks * fix: reject whitespace-only TODO messages * fix: reject whitespace-only TODO messages * test: add cases for hash-prefixed comments and string limitations --- scripts/hooks/pre-bash-commit-quality.js | 6 ++--- tests/hooks/pre-bash-commit-quality.test.js | 28 ++++++++++++++++++++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/scripts/hooks/pre-bash-commit-quality.js b/scripts/hooks/pre-bash-commit-quality.js index 400497055..6504a1b56 100644 --- a/scripts/hooks/pre-bash-commit-quality.js +++ b/scripts/hooks/pre-bash-commit-quality.js @@ -99,7 +99,7 @@ function findFileIssues(filePath) { const lineNum = index + 1; // Check for console.log - if (line.includes('console.log') && !line.trim().startsWith('//') && !line.trim().startsWith('*')) { + if (line.includes('console.log') && !line.trim().startsWith('//') && !line.trim().startsWith('*') && !line.trim().startsWith('#')) { issues.push({ type: 'console.log', message: `console.log found at line ${lineNum}`, @@ -109,7 +109,7 @@ function findFileIssues(filePath) { } // Check for debugger statements - if (/\bdebugger\b/.test(line) && !line.trim().startsWith('//')) { + if (/\bdebugger\b/.test(line) && !line.trim().startsWith('//') && !line.trim().startsWith('#')) { issues.push({ type: 'debugger', message: `debugger statement at line ${lineNum}`, @@ -119,7 +119,7 @@ function findFileIssues(filePath) { } // Check for TODO/FIXME without issue reference - const todoMatch = line.match(/\/\/\s*(TODO|FIXME):?\s*(.+)/); + const todoMatch = line.match(/(?:\/\/|#)\s*(TODO|FIXME):?\s*(\S.*)/); if (todoMatch && !todoMatch[2].match(/#\d+|issue/i)) { issues.push({ type: 'todo', diff --git a/tests/hooks/pre-bash-commit-quality.test.js b/tests/hooks/pre-bash-commit-quality.test.js index 778fc2990..85a76ee9f 100644 --- a/tests/hooks/pre-bash-commit-quality.test.js +++ b/tests/hooks/pre-bash-commit-quality.test.js @@ -504,6 +504,32 @@ if (test('measures length of the full message past an apostrophe (not the trunca assert.strictEqual(res.message, subject); assert.ok(res.issues.some(i => i.type === 'length'), 'full (>72) message should trigger a length issue'); })) passed++; else failed++; - +if (test('handles hash-prefixed python comments and string false-positives', () => { + inTempRepo(repoDir => { + writeAndStage(repoDir, 'script.py', [ + '# console.log("commented out");', + '# debugger', + '# TODO: python unreferenced', + '# TODO: python issue #456', + 'const str = "# TODO: string false positive";', // Greptile string limitation + '' + ].join('\n')); + + const input = JSON.stringify({ tool_input: { command: 'git commit -m "fix(hooks): python hash comments"' } }); + + const origConsoleError = console.error; + let stderr = ''; + console.error = msg => { stderr += msg + '\n'; }; + + const result = hook.evaluate(input); + console.error = origConsoleError; + + assert.strictEqual(result.exitCode, 0, 'warning-only issues should not block'); + assert.ok(stderr.includes('INFO Line 3:'), `expected python TODO warning`); + assert.ok(!stderr.includes('INFO Line 4'), 'referenced python TODO should not warn'); + assert.ok(!stderr.includes('ERROR Line 2'), 'commented debugger should not error'); + assert.ok(stderr.includes('INFO Line 5:'), `expected string limitation warning`); + }); +})) passed++; else failed++; console.log(`\nResults: Passed: ${passed}, Failed: ${failed}, Skipped: ${skipped}`); process.exit(failed > 0 ? 1 : 0); From 43b3a01ee000c9df73094c2fd9dfe4762261692b Mon Sep 17 00:00:00 2001 From: Ram Singh Date: Tue, 22 Sep 2026 00:03:21 +0530 Subject: [PATCH 4/7] Update README with install health check instructions (#3196) Added instructions for checking install health with npx. --- .adal/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.adal/README.md b/.adal/README.md index 95fb30b5b..f53ce7741 100644 --- a/.adal/README.md +++ b/.adal/README.md @@ -20,3 +20,4 @@ bash ./install.sh --target adal --profile minimal - The `adal` target installs into the project-level `./.adal/` directory. - AdaL's own config (`~/.adal/settings.json`, MCP servers, plugins) is **not** touched by ECC install. - Use `npx ecc-universal doctor --target adal` to check install health. +- use an installed From 7b7dfc6412c1dd2d21d1dd4b198ea559d2b63525 Mon Sep 17 00:00:00 2001 From: Varun Gore Date: Tue, 22 Sep 2026 00:09:17 +0530 Subject: [PATCH 5/7] fix(gateguard): detect destructive SQL passed quoted to SQL clients (#3107) * fix(gateguard): detect destructive SQL passed quoted to SQL clients Quoted SQL like psql -c "drop table users" was stripped before the DESTRUCTIVE_SQL_DD regex ran, making it unreachable. Check dequoted tokens for known SQL clients (psql, mysql, sqlite3, ...) so real invocations are gated while git commit -m / echo mentions stay allowed. Fixes affaan-m/ECC#3024. * fix(gateguard): unwrap sudo/env flags and ignore SQL string literals Address review: sudo -u/env VAR= wrappers now resolve to the real SQL client; SQL single/double/dollar-quoted literals are stripped before the destructive regex so SELECT 'drop table' stays allowed. Adds deny/allow regression tests for quoted SQL, wrappers, literals, and non-SQL clients. * test(gateguard): cover env PGPASSWORD wrapper variant * fix(gateguard): close wrapper sh-c, env dir, tagged literal gaps Review 5189686662: recurse sh -c from unwrapped wrapper index (sudo/doas/env + sh -c now denied); consume env -C/--chdir separate values; strip tagged dollar-quoted literals ($tag$...\baseline). Tests: 200 passed. --- scripts/hooks/gateguard-fact-force.js | 155 ++++++++++++++++++++++- tests/hooks/gateguard-fact-force.test.js | 42 ++++++ 2 files changed, 195 insertions(+), 2 deletions(-) diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index bb800d28b..53d8424fe 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -359,6 +359,155 @@ function quoteAwareSegments(input) { const SHELL_WRAPPERS = new Set(['sh', 'bash', 'zsh', 'dash', 'ksh']); +/** + * SQL clients whose `-c`/`-e`/positional arguments carry SQL statements. + * Quoted SQL (e.g. `psql -c "drop table users"`) is invisible to the + * quote-stripping SQL regex, so it is re-checked here against dequoted + * tokens where quoted content is preserved (issue #3024). Restricted to + * known clients so `git commit -m "drop table"` and `echo "drop table"` + * stay allowed. + */ +const SQL_CLIENT_COMMANDS = new Set([ + 'psql', + 'postgres', + 'mysql', + 'mariadb', + 'sqlite3', + 'sqlite', + 'sqlcmd', + 'isql', + 'pgcli', + 'mycli', + 'duckdb', + 'bq', +]); + +/** + * Strip SQL string literals so phrases inside query data do not trigger + * the destructive detector (e.g. `SELECT 'drop table' ...` is a read). + * Handles single-quoted literals with '' escapes, double-quoted + * identifiers, and dollar-quoted blocks ($$...$$ and $tag$...$tag$). + * + * @param {string} input + * @returns {string} + */ +function stripSqlLiterals(input) { + return String(input || '') + .replace(/'(?:[^']|'')*'/g, "''") + .replace(/"(?:[^"\\]|\\.)*"/g, '""') + .replace(/(\$[A-Za-z_][A-Za-z0-9_]*\$|\$\$)[\s\S]*?\1/g, '$$$$'); +} + +const SUDO_VALUE_FLAGS = new Set([ + '-u', + '--user', + '-g', + '--group', + '-U', + '--other-user', + '-p', + '--prompt', + '-C', + '--close-from', + '-D', + '--chdir', + '-h', + '--host', + '-r', + '--role', + '-t', + '--type', + '-T', + '--command-timeout', +]); + +/** + * Advance past `sudo`/`doas`/`env` wrappers including their flags and + * `VAR=value` assignments, so `sudo -u postgres psql ...` and + * `env PGUSER=postgres psql ...` still resolve to the real command. + * + * @param {string[]} tokens dequoted tokens for one segment + * @returns {number} index of the real command token + */ +function unwrapLeadWrappers(tokens) { + let index = 0; + for (let guard = 0; guard < 4; guard += 1) { + if (index >= tokens.length) return index; + const base = commandBasename(tokens[index]); + if (base === 'sudo' || base === 'doas') { + index += 1; + while (index < tokens.length) { + const flag = tokens[index]; + if (flag === '--') { + index += 1; + break; + } + if (flag === '-' || !flag.startsWith('-')) break; + if (SUDO_VALUE_FLAGS.has(flag)) { + index += 2; + continue; + } + if (/^--[^=]+=.*$/.test(flag)) { + index += 1; + continue; + } + index += 1; + } + continue; + } + if (base === 'env') { + index += 1; + while (index < tokens.length) { + const arg = tokens[index]; + if (arg === '--' || arg === '-' || arg === '-i' || arg === '--ignore-environment') { + index += 1; + continue; + } + if (arg === '-u' || arg === '--unset') { + index += 2; + continue; + } + if (arg === '-C' || arg === '--chdir') { + index += 2; + continue; + } + if (/^--unset=.*$/.test(arg) || /^--chdir=.*$/.test(arg) || /^--argv0=.*$/.test(arg)) { + index += 1; + continue; + } + if (arg.startsWith('-') && !/^[A-Za-z_][A-Za-z0-9_]*=/.test(arg)) { + index += 1; + continue; + } + if (/^[A-Za-z_][A-Za-z0-9_]*=/.test(arg)) { + index += 1; + continue; + } + break; + } + continue; + } + break; + } + return index; +} + +/** + * Detect destructive SQL passed as (possibly quoted) arguments to a known + * SQL client. Operates on dequoted tokens from `quoteAwareSegments`, so + * `psql -c "drop table users"` joins back to matchable text. + * + * @param {string[]} tokens dequoted tokens for one segment + * @returns {boolean} + */ +function isDestructiveSqlClient(tokens) { + if (!tokens || tokens.length === 0) return false; + const start = unwrapLeadWrappers(tokens); + if (start >= tokens.length) return false; + if (!SQL_CLIENT_COMMANDS.has(commandBasename(tokens[start]))) return false; + return DESTRUCTIVE_SQL_DD.test(stripSqlLiterals(tokens.slice(start).join(' '))); +} + /** * Quote-aware destructive check: catches quoted command words, newline * separators, quoted `find -exec`, and `sh -c`/`bash -c` wrappers that evade @@ -374,10 +523,12 @@ function isDestructiveQuoteAware(raw, depth = 0) { if (tokens.length === 0) continue; if (isDestructiveRm(tokens)) return true; if (isDestructiveGit(tokens)) return true; + if (isDestructiveSqlClient(tokens)) return true; if (isDestructiveFindExec(tokens.join(' '))) return true; - const base = commandBasename(tokens[0]); + const wi = unwrapLeadWrappers(tokens); + const base = wi < tokens.length ? commandBasename(tokens[wi]) : ''; if (SHELL_WRAPPERS.has(base)) { - const ci = tokens.indexOf('-c'); + const ci = tokens.indexOf('-c', wi); if (ci !== -1 && tokens[ci + 1] && isDestructiveQuoteAware(tokens[ci + 1], depth + 1)) { return true; } diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index fc3ab3106..399cca83b 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -1507,6 +1507,48 @@ function runTests() { passed++; else failed++; + if ( + test('denies quoted destructive SQL passed to SQL clients (issue #3024)', () => { + expectDestructiveDeny('psql -c "drop table users"', 'psql quoted drop table'); + expectDestructiveDeny("psql -c 'truncate audit_log'", 'psql quoted truncate'); + expectDestructiveDeny('mysql -e "delete from sessions"', 'mysql quoted delete'); + expectDestructiveDeny('sqlite3 app.db "DROP TABLE users"', 'sqlite3 quoted drop'); + }) + ) + passed++; + else failed++; + + if ( + test('denies quoted destructive SQL through sudo/env wrappers', () => { + expectDestructiveDeny('sudo -u postgres psql -c "drop table users"', 'sudo -u psql'); + expectDestructiveDeny('env PGUSER=postgres psql -c "drop table users"', 'env psql'); + expectDestructiveDeny('env PGPASSWORD=value psql -c "drop table users"', 'env PGPASSWORD psql'); + expectDestructiveDeny('env -C /tmp psql -c "drop table users"', 'env -C psql'); + expectDestructiveDeny('env --chdir /tmp psql -c "drop table users"', 'env --chdir psql'); + }) + ) + passed++; + else failed++; + + if ( + test('denies destructive SQL through wrapper sh -c chains', () => { + expectDestructiveDeny('sudo sh -c \'psql -c "drop table users"\'', 'sudo sh -c psql'); + expectDestructiveDeny('env sh -c \'psql -c "drop table users"\'', 'env sh -c psql'); + }) + ) + passed++; + else failed++; + + if ( + test('allows SQL string literals and non-SQL clients mentioning SQL', () => { + expectAllow('psql -c "SELECT \'drop table\' FROM audit_log"', 'SQL string literal'); + expectAllow('psql -c "SELECT $tag$drop table users$tag$ FROM t"', 'tagged dollar-quote literal'); + expectAllow('echo "drop table users"', 'echo SQL mention'); + }) + ) + passed++; + else failed++; + if ( test('allows destructive SQL prose inside a quoted heredoc', () => { expectAllow( From 8b951d3bf63b5f3dcdac3f255c8a15c23fbf35f3 Mon Sep 17 00:00:00 2001 From: Lukas <103962359+L4XB@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:39:20 +0200 Subject: [PATCH 6/7] fix(skill-comply): stop a failed step supplying evidence downstream (#3109) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skill-comply): stop a failed step supplying evidence downstream `_check_temporal_order` fell back to the raw classifier output whenever the referenced step was absent from `resolved`. A step only enters `resolved` once it passes, so "failed" and "not graded yet" were the same thing to that lookup, and a dependant could pass on an event belonging to a prerequisite that had failed its own ordering check. With three steps C, A (before C) and B (after A) and events C@T0, A@T1, B@T2, A fails and B passed on A's classified event: 2/3 instead of 1/3. It compounds down a chain, so one failed prerequisite could leave a five-step workflow reading 4/5. The grader now tracks which steps have been graded at all. A referenced step that was graded and is missing from `resolved` failed, and its events are refused with a reason that says so. A step not graded yet is a forward reference to a step declared later, and the fallback stays as it was: that is what makes an out-of-order declaration work, and the existing regression for it goes red if the fallback is removed instead. `before_step` deliberately keeps the old fallback. The two fail in opposite directions: an `after_step` fallback can only turn a failure into a pass, a `before_step` one can only turn a pass into a failure, so dropping it would relax a constraint because some other step failed. * fix(skill-comply): revoke a pass that rested on a later-failing prerequisite Review of #3109 found the mirror image of the case that PR fixes. `graded` only catches a prerequisite that had already failed when its dependant was graded. A step declared *before* its `after_step` is graded against the classifier's raw events for a step that has not run yet — the fallback that makes an out-of-order declaration work — and nothing revisited it once that step went on to fail its own checks. Add a pass after grading that demotes any detected step whose `after_step` ended up failing, repeated to a fixed point: one demotion can invalidate whatever depended on it, in either declaration order. Demotion only removes passes, so it terminates. `compliance_rate` is computed from the demoted results. Also from review: build `graded` and the chain test's step list as new objects rather than mutating (AGENTS.md immutability rule), and annotate the injected mocks in the tests this PR owns. --- skills/skill-comply/scripts/grader.py | 88 +++++++- skills/skill-comply/tests/test_grader.py | 249 ++++++++++++++++++++++- 2 files changed, 326 insertions(+), 11 deletions(-) diff --git a/skills/skill-comply/scripts/grader.py b/skills/skill-comply/scripts/grader.py index 516beb18b..1209d042a 100644 --- a/skills/skill-comply/scripts/grader.py +++ b/skills/skill-comply/scripts/grader.py @@ -2,7 +2,7 @@ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, replace from scripts.classifier import classify_events from scripts.parser import ComplianceSpec, ObservationEvent, Step @@ -30,23 +30,38 @@ def _check_temporal_order( event: ObservationEvent, resolved: dict[str, list[ObservationEvent]], classified: dict[str, list[ObservationEvent]], + graded: set[str], ) -> str | None: """Check before_step/after_step constraints. Returns failure reason or None.""" if step.detector.after_step is not None: - after_events = resolved.get(step.detector.after_step) + after_step = step.detector.after_step + after_events = resolved.get(after_step) if after_events is None: - after_events = classified.get(step.detector.after_step, []) + if after_step in graded: + # Graded and missing from `resolved` means it failed its own checks. + # Its classified events are not evidence for anything: reusing them + # here let a dependant pass on the strength of a failed prerequisite, + # and the overstatement carried down the whole chain. + return f"after_step '{after_step}' did not pass its own checks" + # Not graded yet, so this is a forward reference to a step declared later. + # The classifier's own output is the only thing available, and using it is + # what makes an out-of-order declaration work. + after_events = classified.get(after_step, []) if not after_events: - return f"after_step '{step.detector.after_step}' not yet detected" + return f"after_step '{after_step}' not yet detected" latest_after = max(e.timestamp for e in after_events) if event.timestamp <= latest_after: return ( - f"must occur after '{step.detector.after_step}' " + f"must occur after '{after_step}' " f"(last at {latest_after}), but found at {event.timestamp}" ) if step.detector.before_step is not None: - # Look ahead using LLM classification results + # Look ahead using LLM classification results. A failed reference step is NOT + # excluded here the way it is above: the two fall in opposite directions. An + # `after_step` fallback can only turn a failure into a pass, while a + # `before_step` one can only turn a pass into a failure, so dropping it would + # relax a constraint because some other step failed. before_events = resolved.get(step.detector.before_step) if before_events is None: before_events = classified.get(step.detector.before_step, []) @@ -61,6 +76,46 @@ def _check_temporal_order( return None +def _demote_steps_resting_on_failures( + step_results: tuple[StepResult, ...], + after_steps: dict[str, str], +) -> tuple[StepResult, ...]: + """Undo passes that rest on an `after_step` which ended up failing. + + A step declared before its prerequisite is graded against the classifier's raw + events for that step, because `resolved` has nothing for it yet. That fallback is + what makes an out-of-order declaration work, but the prerequisite can go on to + fail its own checks, and then the dependant is left passing on evidence that + never held. Repeat until nothing changes: demoting one step can invalidate + whatever depended on it, whichever order the two were declared in. Demotion only + ever removes passes, so the loop terminates. + """ + results = step_results + while True: + failed = {result.step_id for result in results if not result.detected} + demote = { + result.step_id + for result in results + if result.detected and after_steps.get(result.step_id) in failed + } + if not demote: + return results + results = tuple( + replace( + result, + detected=False, + evidence=(), + failure_reason=( + f"after_step '{after_steps[result.step_id]}' " + "did not pass its own checks" + ), + ) + if result.step_id in demote + else result + for result in results + ) + + def grade( spec: ComplianceSpec, trace: list[ObservationEvent], @@ -80,6 +135,9 @@ def grade( # Step 2: Check temporal ordering (deterministic) resolved: dict[str, list[ObservationEvent]] = {} + # Steps already graded, pass or fail. `resolved` alone cannot tell "failed" from + # "not reached yet", and those need opposite answers in `_check_temporal_order`. + graded: set[str] = set() step_results: list[StepResult] = [] for step in spec.steps: @@ -88,7 +146,7 @@ def grade( failure_reason: str | None = None for event in candidates: - temporal_fail = _check_temporal_order(step, event, resolved, classified) + temporal_fail = _check_temporal_order(step, event, resolved, classified, graded) if temporal_fail is None: matched.append(event) break @@ -101,6 +159,7 @@ def grade( elif failure_reason is None: failure_reason = f"no matching event classified for step '{step.id}'" + graded = graded | {step.id} step_results.append(StepResult( step_id=step.id, detected=detected, @@ -108,8 +167,19 @@ def grade( failure_reason=failure_reason if not detected else None, )) + # `graded` catches a prerequisite that had already failed when its dependant was + # graded. The other direction needs a second pass: a dependant declared first is + # graded against the classifier's raw events for a prerequisite that has not run + # yet, and only later does that prerequisite fail. + after_steps = { + step.id: step.detector.after_step + for step in spec.steps + if step.detector.after_step is not None + } + resolved_results = _demote_steps_resting_on_failures(tuple(step_results), after_steps) + required_ids = {s.id for s in spec.steps if s.required} - required_steps = [s for s in step_results if s.step_id in required_ids] + required_steps = [s for s in resolved_results if s.step_id in required_ids] detected_required = sum(1 for s in required_steps if s.detected) total_required = len(required_steps) @@ -117,7 +187,7 @@ def grade( return ComplianceResult( spec_id=spec.id, - steps=tuple(step_results), + steps=resolved_results, compliance_rate=compliance_rate, recommend_hook_promotion=compliance_rate < spec.threshold_promote_to_hook, classification=classification, diff --git a/skills/skill-comply/tests/test_grader.py b/skills/skill-comply/tests/test_grader.py index a95825266..8ca056452 100644 --- a/skills/skill-comply/tests/test_grader.py +++ b/skills/skill-comply/tests/test_grader.py @@ -1,7 +1,7 @@ """Tests for grader module — compliance scoring with LLM classification.""" from pathlib import Path -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest @@ -137,7 +137,7 @@ class TestGradeEdgeCases: assert result.spec_id == "tdd-workflow" @patch("scripts.grader.classify_events") - def test_after_step_can_reference_later_declared_spec_step(self, mock_cls) -> None: + def test_after_step_can_reference_later_declared_spec_step(self, mock_cls: MagicMock) -> None: spec = ComplianceSpec( id="out-of-order-after-step", name="Out of order after_step", @@ -195,3 +195,248 @@ class TestGradeEdgeCases: assert step_a.failure_reason is None assert step_b.detected is True assert result.compliance_rate == 1.0 + + @patch("scripts.grader.classify_events") + def test_after_step_does_not_reuse_a_step_that_failed_its_own_constraint( + self, mock_cls: MagicMock + ) -> None: + """A step that failed may not supply evidence to a step that depends on it (#3108). + + `resolved` only receives a step once it passes, so a dependant fell back to the + raw classifier output and could pass on an event belonging to a failed parent. + The fallback exists for forward references, which the case above covers; a + parent that has already been graded and failed is a different thing. + """ + spec = ComplianceSpec( + id="failed-parent-evidence", + name="Failed parent evidence", + source_rule="rules/common/testing.md", + version="1.0", + steps=( + Step( + id="C", + description="Reference step with no constraint of its own", + required=True, + detector=Detector(description="Event C"), + ), + Step( + id="A", + description="Must occur before C, and does not", + required=True, + detector=Detector(description="Event A", before_step="C"), + ), + Step( + id="B", + description="Depends on A", + required=True, + detector=Detector(description="Event B", after_step="A"), + ), + ), + threshold_promote_to_hook=0.5, + ) + trace = [ + ObservationEvent( + timestamp=f"2026-03-20T10:00:0{index}Z", + event="tool_complete", + tool="Write", + session="sess-evidence", + input=f'{{"file_path":"src/{name}.py"}}', + output=f"step {name}", + ) + for index, name in enumerate(("c", "a", "b")) + ] + mock_cls.return_value = {"C": [0], "A": [1], "B": [2]} + + result = grade(spec, trace) + + detected = {step.step_id: step.detected for step in result.steps} + assert detected["C"] is True + assert detected["A"] is False + assert detected["B"] is False + assert result.compliance_rate == pytest.approx(1 / 3) + step_b = next(step for step in result.steps if step.step_id == "B") + assert "A" in (step_b.failure_reason or "") + + @patch("scripts.grader.classify_events") + def test_a_failed_prerequisite_does_not_carry_a_chain_of_passes( + self, mock_cls: MagicMock + ) -> None: + """The overstatement compounds: B on A, C on B, D on C (#3108). + + Each link used to pass on the classifier's raw output for the link above it, so + one failed prerequisite could leave a four-step workflow reading 3/4 compliant. + """ + steps = ( + Step( + id="Z", + description="Reference step with no constraint of its own", + required=True, + detector=Detector(description="Event Z"), + ), + Step( + id="A", + description="Must occur before Z, and does not", + required=True, + detector=Detector(description="Event A", before_step="Z"), + ), + *( + Step( + id=later, + description=f"Depends on {earlier}", + required=True, + detector=Detector(description=f"Event {later}", after_step=earlier), + ) + for earlier, later in (("A", "B"), ("B", "C"), ("C", "D")) + ), + ) + spec = ComplianceSpec( + id="failed-prerequisite-chain", + name="Failed prerequisite chain", + source_rule="rules/common/testing.md", + version="1.0", + steps=steps, + threshold_promote_to_hook=0.5, + ) + names = ("z", "a", "b", "c", "d") + trace = [ + ObservationEvent( + timestamp=f"2026-03-20T10:00:0{index}Z", + event="tool_complete", + tool="Write", + session="sess-chain", + input=f'{{"file_path":"src/{name}.py"}}', + output=f"step {name}", + ) + for index, name in enumerate(names) + ] + mock_cls.return_value = {"Z": [0], "A": [1], "B": [2], "C": [3], "D": [4]} + + result = grade(spec, trace) + + detected = {step.step_id: step.detected for step in result.steps} + assert detected == {"Z": True, "A": False, "B": False, "C": False, "D": False} + assert result.compliance_rate == pytest.approx(1 / 5) + + @patch("scripts.grader.classify_events") + def test_forward_reference_is_revoked_when_the_prerequisite_later_fails( + self, mock_cls: MagicMock + ) -> None: + """The mirror image of the case above, raised in review of #3109. + + `graded` only catches a prerequisite that had already failed. A step declared + *before* its prerequisite is graded against the classifier's raw events for a + step that has not run yet — the fallback that makes an out-of-order + declaration work — and nothing revisited it once that step failed. + """ + spec = ComplianceSpec( + id="forward-reference-revoked", + name="Forward reference revoked", + source_rule="rules/common/testing.md", + version="1.0", + steps=( + Step( + id="Z", + description="Reference step with no constraint of its own", + required=True, + detector=Detector(description="Event Z"), + ), + Step( + id="B", + description="Depends on A, which is declared after it", + required=True, + detector=Detector(description="Event B", after_step="A"), + ), + Step( + id="A", + description="Must occur before Z, and does not", + required=True, + detector=Detector(description="Event A", before_step="Z"), + ), + ), + threshold_promote_to_hook=0.5, + ) + trace = [ + ObservationEvent( + timestamp=f"2026-03-20T10:00:0{index}Z", + event="tool_complete", + tool="Write", + session="sess-forward", + input=f'{{"file_path":"src/{name}.py"}}', + output=f"step {name}", + ) + for index, name in enumerate(("z", "a", "b")) + ] + mock_cls.return_value = {"Z": [0], "A": [1], "B": [2]} + + result = grade(spec, trace) + + detected = {step.step_id: step.detected for step in result.steps} + assert detected == {"Z": True, "A": False, "B": False} + assert result.compliance_rate == pytest.approx(1 / 3) + step_b = next(step for step in result.steps if step.step_id == "B") + assert step_b.evidence == () + assert "A" in (step_b.failure_reason or "") + + @patch("scripts.grader.classify_events") + def test_revocation_reaches_a_step_that_referenced_the_dependant_backwards( + self, mock_cls: MagicMock + ) -> None: + """One demotion invalidates the next, in either declaration order. + + C is declared after B and passes on `resolved`, the ordinary backward + reference — B was still detected at the time. B is the forward-reference case + above and comes down with A, so C has to follow, which takes a second pass. + """ + spec = ComplianceSpec( + id="revocation-propagates", + name="Revocation propagates", + source_rule="rules/common/testing.md", + version="1.0", + steps=( + Step( + id="Z", + description="Reference step with no constraint of its own", + required=True, + detector=Detector(description="Event Z"), + ), + Step( + id="B", + description="Depends on A, which is declared after it", + required=True, + detector=Detector(description="Event B", after_step="A"), + ), + Step( + id="A", + description="Must occur before Z, and does not", + required=True, + detector=Detector(description="Event A", before_step="Z"), + ), + Step( + id="C", + description="Depends on B, which is declared before it", + required=True, + detector=Detector(description="Event C", after_step="B"), + ), + ), + threshold_promote_to_hook=0.5, + ) + trace = [ + ObservationEvent( + timestamp=f"2026-03-20T10:00:0{index}Z", + event="tool_complete", + tool="Write", + session="sess-propagate", + input=f'{{"file_path":"src/{name}.py"}}', + output=f"step {name}", + ) + for index, name in enumerate(("z", "a", "b", "c")) + ] + mock_cls.return_value = {"Z": [0], "A": [1], "B": [2], "C": [3]} + + result = grade(spec, trace) + + detected = {step.step_id: step.detected for step in result.steps} + assert detected == {"Z": True, "A": False, "B": False, "C": False} + assert result.compliance_rate == pytest.approx(1 / 4) + step_c = next(step for step in result.steps if step.step_id == "C") + assert "B" in (step_c.failure_reason or "") From bf70150eb2df8070024e5bdf08e4aa08959e2735 Mon Sep 17 00:00:00 2001 From: xsf <45911614+xsfX20@users.noreply.github.com> Date: Tue, 22 Sep 2026 02:40:16 +0800 Subject: [PATCH 7/7] fix(gateguard): sanitize dangerous invisible unicode in denial paths (#3103) * fix(gateguard): sanitize dangerous invisible unicode in denial paths sanitizePath only stripped control characters and bidi overrides, so the 394 codepoints that the repo-wide unicode safety policy classifies as dangerous-invisible (zero-width U+200B-200D, U+2060, U+2061-2064, variation selectors U+FE00-FE0F, tag block U+E0000-E007F, Hangul fillers, U+FEFF, U+180E, U+3164) plus the U+2028/U+2029 line and paragraph separators passed verbatim into Edit/Write denial messages. Invisible characters in a quoted file path let a malicious path look clean to a human reviewer. Align sanitizePath with the CI policy list and cover it with a hook-output regression test that asserts no dangerous invisible character survives into the denial reason while visible path text stays intact. * refactor(gateguard): name the sanitizePath unicode policy constants Address review feedback: move the code points and ranges out of sanitizePath into descriptive constants mirroring scripts/ci/check-unicode-safety.js, so the policy is auditable in one place. Also seed the regression test input with U+2029 so the paragraph-separator assertion is exercised rather than vacuous. * fix(gateguard): sanitize C1 control characters in denial paths Follow-up on the sanitizePath policy alignment: the Unicode C1 control block (U+0080..U+009F) is invisible in every renderer like the ASCII controls below 0x20, but the strip only covered the ASCII range. Add C1_CONTROLS to the policy constants and seed U+0091 into the regression test input. --- scripts/hooks/gateguard-fact-force.js | 56 +++++++++++++++++++++--- tests/hooks/gateguard-fact-force.test.js | 28 ++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index 53d8424fe..6756a0b79 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -1262,16 +1262,62 @@ function isChecked(key) { // --- Sanitize file path against injection --- +// Unicode policy for sanitizePath, mirroring the repo-wide dangerous set in +// scripts/ci/check-unicode-safety.js. Named so the ranges stay auditable and +// drift against the CI policy is visible in one place. +const ASCII_CONTROL_MAX = 0x1f; +const ASCII_DELETE = 0x7f; +const C1_CONTROLS = [0x80, 0x9f]; // Unicode C1 control block (U+0080..U+009F) +const BIDI_MARKS = [0x200e, 0x200f]; // LRM/RLM +const BIDI_EMBEDDINGS = [0x202a, 0x202e]; // LRE..PDF +const BIDI_ISOLATES = [0x2066, 0x2069]; // LRI..PDI +const ZERO_WIDTHS = [0x200b, 0x200d]; // ZWSP..ZWJ +const WORD_JOINER = 0x2060; +const BYTE_ORDER_MARK = 0xfeff; +const VARIATION_SELECTORS = [0xfe00, 0xfe0f]; +const VARIATION_SUPPLEMENTS = [0xe0100, 0xe01ef]; // MONGOLIAN..TAGS (VS17..VS256) +const TAG_BLOCK = [0xe0000, 0xe007f]; // ASCII-smuggling tag characters +const MONGOLIAN_VOWEL_SEPARATOR = 0x180e; +const HANGUL_CHOSEONG_FILLER = 0x115f; +const HANGUL_JUNGSEONG_FILLER = 0x1160; +const HANGUL_FILLER = 0x3164; +const INVISIBLE_MATH_OPERATORS = [0x2061, 0x2064]; // FUNCTION APPLICATION..INVISIBLE PLUS +const LINE_SEPARATOR = 0x2028; +const PARAGRAPH_SEPARATOR = 0x2029; +const SANITIZED_PATH_MAX_LENGTH = 500; + +function inRange(code, [lo, hi]) { + return code >= lo && code <= hi; +} + function sanitizePath(filePath) { - // Strip control chars (including null), bidi overrides, and newlines + // Strip control chars (including null), bidi overrides, separators, + // and the dangerous invisible characters defined by the constants + // above (mirroring scripts/ci/check-unicode-safety.js), so a denial + // message cannot carry content a human reviewer cannot see. let sanitized = ''; for (const char of String(filePath || '')) { const code = char.codePointAt(0); - const isAsciiControl = code <= 0x1f || code === 0x7f; - const isBidiOverride = (code >= 0x200e && code <= 0x200f) || (code >= 0x202a && code <= 0x202e) || (code >= 0x2066 && code <= 0x2069); - sanitized += isAsciiControl || isBidiOverride ? ' ' : char; + const isAsciiControl = + code <= ASCII_CONTROL_MAX || code === ASCII_DELETE || inRange(code, C1_CONTROLS); + const isBidiOverride = + inRange(code, BIDI_MARKS) || inRange(code, BIDI_EMBEDDINGS) || inRange(code, BIDI_ISOLATES); + const isUnicodeSeparator = code === LINE_SEPARATOR || code === PARAGRAPH_SEPARATOR; + const isDangerousInvisible = + inRange(code, ZERO_WIDTHS) || + code === WORD_JOINER || + code === BYTE_ORDER_MARK || + inRange(code, VARIATION_SELECTORS) || + inRange(code, VARIATION_SUPPLEMENTS) || + inRange(code, TAG_BLOCK) || + code === MONGOLIAN_VOWEL_SEPARATOR || + code === HANGUL_CHOSEONG_FILLER || + code === HANGUL_JUNGSEONG_FILLER || + code === HANGUL_FILLER || + inRange(code, INVISIBLE_MATH_OPERATORS); + sanitized += isAsciiControl || isBidiOverride || isUnicodeSeparator || isDangerousInvisible ? ' ' : char; } - return sanitized.trim().slice(0, 500); + return sanitized.trim().slice(0, SANITIZED_PATH_MAX_LENGTH); } function normalizeForMatch(value) { diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index 399cca83b..9e6a18334 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -3369,6 +3369,34 @@ function runTests() { console.error(` [cleanup] failed to remove ${stateDir}: ${err.message}`); } + // --- sanitizePath dangerous invisible unicode regression --- + clearState(); + if ( + test('sanitizePath strips CI-defined dangerous invisible unicode from denial paths', () => { + const file_path = + '/src/eu2028\u2028eu2029\u2029app.js\u200bhidden\u2060name\ufefftail\u3164x\u0091c1.js'; + const input = { + tool_name: 'Edit', + tool_input: { file_path, old_string: 'foo', new_string: 'bar' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + const reason = String( + output && output.hookSpecificOutput + ? output.hookSpecificOutput.permissionDecisionReason + : '' + ); + for (const bad of ['\u2028', '\u2029', '\u200b', '\u2060', '\ufeff', '\u3164', '\u0091']) { + assert.ok(!reason.includes(bad), `denial reason must not carry U+${bad.codePointAt(0).toString(16)} (${bad})`); + } + assert.ok(reason.includes('app.js'), 'visible path text must remain'); + }) + ) { + passed++; + } else { + failed++; + } + console.log(`\n ${passed} passed, ${failed} failed\n`); process.exit(failed > 0 ? 1 : 0); }