mirror of
https://github.com/affaan-m/ECC.git
synced 2026-10-23 03:27:50 +09:00
fix: integrate strict issue targeting and Windows Python discovery
This commit is contained in:
6 files changed
+244
-8
No files matched your search
@@ -12,8 +12,9 @@ function normalizeRepo(repo) {
|
||||
}
|
||||
|
||||
function normalizeIssueNumber(value) {
|
||||
const parsed = Number.parseInt(String(value), 10);
|
||||
if (!Number.isFinite(parsed) || parsed <= 0) {
|
||||
const text = String(value).trim();
|
||||
const parsed = Number(text);
|
||||
if (!/^\+?\d+$/.test(text) || !Number.isSafeInteger(parsed) || parsed <= 0) {
|
||||
throw new Error(`Invalid issue number: ${value}`);
|
||||
}
|
||||
return parsed;
|
||||
|
||||
@@ -16,10 +16,15 @@ const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
const { spawnSync } = require('child_process');
|
||||
const { resolvePython } = require('./lib/helpers/windows-test-env');
|
||||
|
||||
const repoRoot = path.resolve(__dirname, '..');
|
||||
const GUARD = path.join(repoRoot, 'hooks', 'hermes', 'config-protection.py');
|
||||
const LOGGER = path.join(repoRoot, 'hooks', 'hermes', 'check-console-log.py');
|
||||
// A working Python 3, not the Windows Store `python3` stub; with none found the
|
||||
// spawns use python3 as before and fail loudly rather than skip.
|
||||
const PYTHON = resolvePython() || { command: 'python3', prefix: [] };
|
||||
const runPython = (script, options) => spawnSync(PYTHON.command, [...PYTHON.prefix, script], options);
|
||||
|
||||
function test(name, fn) {
|
||||
try {
|
||||
@@ -35,7 +40,7 @@ function test(name, fn) {
|
||||
|
||||
/** Run a hook with a payload; return {rc, stdout, stderr} and assert clean spawn. */
|
||||
function runHook(script, payload) {
|
||||
const res = spawnSync('python3', [script], {
|
||||
const res = runPython(script, {
|
||||
input: JSON.stringify(payload),
|
||||
encoding: 'utf8',
|
||||
timeout: 15000,
|
||||
@@ -181,7 +186,7 @@ run('allows non-editing tools (read/search)', () => {
|
||||
// ─── config-protection: fail-open + diagnostics ────────────────────────────
|
||||
|
||||
run('malformed payload: allows AND reports on stderr', () => {
|
||||
const res = spawnSync('python3', [GUARD], { input: 'not-json{{', encoding: 'utf8', timeout: 15000 });
|
||||
const res = runPython(GUARD, { input: 'not-json{{', encoding: 'utf8', timeout: 15000 });
|
||||
assert.strictEqual(res.status, 0, 'must exit 0 (fail-open)');
|
||||
assert.strictEqual(res.stdout.trim(), '', 'no decision emitted');
|
||||
assert.ok(res.stderr.includes('unparseable payload'), 'diagnostic required');
|
||||
@@ -250,13 +255,13 @@ run('hermes-hooks module installs via the real installer and the landed guard bl
|
||||
const t = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-guard-'));
|
||||
const target = path.join(t, 'ESLINT.CONFIG.MJS');
|
||||
fs.writeFileSync(target, 'export default {}\n');
|
||||
const g = spawnSync('python3', [landed], {
|
||||
const g = runPython(landed, {
|
||||
input: JSON.stringify({ hook_event_name: 'pre_tool_call', tool_name: 'patch', tool_input: { path: target, old_string: 'a', new_string: 'b' } }),
|
||||
encoding: 'utf8', timeout: 15000,
|
||||
});
|
||||
assert.ok((g.stdout || '').includes('"decision": "block"'), 'landed guard must block');
|
||||
fs.unlinkSync(target);
|
||||
const bootstrap = spawnSync('python3', [landed], {
|
||||
const bootstrap = runPython(landed, {
|
||||
input: JSON.stringify({ hook_event_name: 'pre_tool_call', tool_name: 'patch', tool_input: { path: target } }),
|
||||
encoding: 'utf8', timeout: 15000,
|
||||
});
|
||||
|
||||
@@ -0,0 +1,96 @@
|
||||
/**
|
||||
* Tests for scripts/lib/github-coordination/gh-api.js — normalizeIssueNumber
|
||||
*
|
||||
* Guards against silent truncation of malformed issue numbers
|
||||
* (https://github.com/affaan-m/ECC/issues/3514): parseInt-style prefix
|
||||
* parsing accepted "42oops" as 42, "42.9" as 42, and "4e2" as 4, while
|
||||
* integers beyond 2^53 lost precision (9007199254740993 became ...992).
|
||||
* The same helper also parses --limit, so malformed values there must be
|
||||
* rejected too. Whitespace, leading plus, and leading zero tolerance is
|
||||
* preserved.
|
||||
*
|
||||
* Run with: node tests/lib/github-coordination-gh-api.test.js
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const assert = require('assert');
|
||||
|
||||
const {
|
||||
normalizeIssueNumber,
|
||||
} = require('../../scripts/lib/github-coordination/gh-api');
|
||||
const { test, banner, section, summary } = require('./helpers/mini-test-runner');
|
||||
|
||||
let passed = 0;
|
||||
let failed = 0;
|
||||
|
||||
banner('normalizeIssueNumber — accepted forms:');
|
||||
|
||||
if (test('accepts a plain positive integer', () => {
|
||||
assert.strictEqual(normalizeIssueNumber('42'), 42);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('preserves surrounding whitespace tolerance', () => {
|
||||
assert.strictEqual(normalizeIssueNumber(' 7 '), 7);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('preserves leading plus tolerance', () => {
|
||||
assert.strictEqual(normalizeIssueNumber('+7'), 7);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('preserves leading zero tolerance', () => {
|
||||
assert.strictEqual(normalizeIssueNumber('007'), 7);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('accepts MAX_SAFE_INTEGER exactly', () => {
|
||||
assert.strictEqual(normalizeIssueNumber('9007199254740991'), 9007199254740991);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('accepts numeric (non-string) input', () => {
|
||||
assert.strictEqual(normalizeIssueNumber(42), 42);
|
||||
})) passed++; else failed++;
|
||||
|
||||
section('normalizeIssueNumber — malformed tokens are rejected, not truncated:');
|
||||
|
||||
for (const [value, label] of [
|
||||
['42oops', 'trailing garbage after digits'],
|
||||
['42.9', 'decimal fraction'],
|
||||
['4e2', 'exponent notation'],
|
||||
['0x10', 'hex notation'],
|
||||
['1e999', 'exponent beyond Number range'],
|
||||
['12 34', 'internal whitespace'],
|
||||
['-.', 'punctuation only'],
|
||||
]) {
|
||||
if (test(`rejects ${JSON.stringify(value)} (${label})`, () => {
|
||||
assert.throws(() => normalizeIssueNumber(value), /Invalid issue number/);
|
||||
})) passed++; else failed++;
|
||||
}
|
||||
|
||||
section('normalizeIssueNumber — unsafe integers are rejected:');
|
||||
|
||||
for (const value of [
|
||||
'9007199254740992', // 2^53
|
||||
'9007199254740993', // 2^53 + 1 (rounds to 2^53 as a Number)
|
||||
'999999999999999999999999',
|
||||
]) {
|
||||
if (test(`rejects ${value}`, () => {
|
||||
assert.throws(() => normalizeIssueNumber(value), /Invalid issue number/);
|
||||
})) passed++; else failed++;
|
||||
}
|
||||
|
||||
section('normalizeIssueNumber — non-positive and empty inputs:');
|
||||
|
||||
for (const value of ['0', '-3', '', ' ', 'abc']) {
|
||||
if (test(`rejects ${JSON.stringify(value)}`, () => {
|
||||
assert.throws(() => normalizeIssueNumber(value), /Invalid issue number/);
|
||||
})) passed++; else failed++;
|
||||
}
|
||||
|
||||
if (test('error message preserves the original input', () => {
|
||||
assert.throws(
|
||||
() => normalizeIssueNumber('42oops'),
|
||||
(error) => error.message === 'Invalid issue number: 42oops'
|
||||
);
|
||||
})) passed++; else failed++;
|
||||
|
||||
summary(passed, failed);
|
||||
@@ -8,6 +8,7 @@
|
||||
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { spawnSync } = require('child_process');
|
||||
|
||||
// On Windows, spawning a bare `claude` resolves only .com/.exe on PATH, never
|
||||
// .cmd. A natively installed claude.exe therefore wins over a test's fake
|
||||
@@ -35,4 +36,29 @@ function toRelativeTarPath(cwd, archivePath) {
|
||||
return path.relative(cwd, archivePath).split(path.sep).join('/');
|
||||
}
|
||||
|
||||
module.exports = { withoutShadowingClaude, toRelativeTarPath };
|
||||
// On Windows, `python3` is usually the Microsoft Store alias stub, which exits
|
||||
// 9009 instead of running Python; python.org installs provide `python` and the
|
||||
// `py` launcher instead.
|
||||
const PYTHON_CANDIDATES = Object.freeze({
|
||||
win32: [['python'], ['py', '-3'], ['python3']],
|
||||
default: [['python3'], ['python']]
|
||||
});
|
||||
const PYTHON3_CHECK = 'import sys; sys.exit(0 if sys.version_info[0] == 3 else 1)';
|
||||
|
||||
function runsPython3(command, args) {
|
||||
return spawnSync(command, args, { encoding: 'utf8', timeout: 15000 }).status === 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* The first candidate that actually runs Python 3, as `{ command, prefix }`
|
||||
* (spawn `command` with `[...prefix, script]`), or null when none does.
|
||||
*/
|
||||
function resolvePython({ platform = process.platform, probe = runsPython3 } = {}) {
|
||||
const candidates = platform === 'win32' ? PYTHON_CANDIDATES.win32 : PYTHON_CANDIDATES.default;
|
||||
for (const [command, ...prefix] of candidates) {
|
||||
if (probe(command, [...prefix, '-c', PYTHON3_CHECK])) return { command, prefix };
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
module.exports = { withoutShadowingClaude, toRelativeTarPath, resolvePython };
|
||||
@@ -10,9 +10,23 @@ const assert = require('assert');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
const vm = require('vm');
|
||||
|
||||
const { test, banner, summary } = require('./helpers/mini-test-runner');
|
||||
const { withoutShadowingClaude, toRelativeTarPath } = require('./helpers/windows-test-env');
|
||||
const { spawnSync } = require('child_process');
|
||||
const { withoutShadowingClaude, toRelativeTarPath, resolvePython } = require('./helpers/windows-test-env');
|
||||
|
||||
/** Load the unchanged helper with controlled process results, keeping its real probe. */
|
||||
function loadWithSpawn(spawn) {
|
||||
const filename = path.join(__dirname, 'helpers', 'windows-test-env.js');
|
||||
const module = { exports: {} };
|
||||
vm.runInNewContext(fs.readFileSync(filename, 'utf8'), {
|
||||
module,
|
||||
require: name => name === 'child_process' ? { spawnSync: spawn } : require(name),
|
||||
process,
|
||||
}, { filename });
|
||||
return module.exports;
|
||||
}
|
||||
|
||||
banner('windows-test-env helper');
|
||||
|
||||
@@ -55,6 +69,78 @@ try {
|
||||
assert.ok(!relative.includes('\\'), `expected forward slashes: ${relative}`);
|
||||
assert.strictEqual(path.resolve(cwd, relative), archive);
|
||||
})) passed++; else failed++;
|
||||
|
||||
// A probe that runs only the commands listed, as Python 3.
|
||||
const probeFor = working => (command, args) => working.includes([command, ...args.slice(0, args.indexOf('-c'))].join(' '));
|
||||
|
||||
if (test('skips a python3 that does not run Python, such as the Microsoft Store alias stub', () => {
|
||||
assert.deepStrictEqual(resolvePython({ platform: 'win32', probe: probeFor(['py -3']) }), { command: 'py', prefix: ['-3'] });
|
||||
assert.deepStrictEqual(resolvePython({ platform: 'win32', probe: probeFor(['python', 'py -3']) }), { command: 'python', prefix: [] });
|
||||
assert.deepStrictEqual(resolvePython({ platform: 'win32', probe: probeFor(['python3']) }), { command: 'python3', prefix: [] });
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('prefers python3 off Windows', () => {
|
||||
assert.deepStrictEqual(resolvePython({ platform: 'linux', probe: probeFor(['python3', 'python']) }), { command: 'python3', prefix: [] });
|
||||
assert.deepStrictEqual(resolvePython({ platform: 'darwin', probe: probeFor(['python']) }), { command: 'python', prefix: [] });
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('returns null when no candidate runs Python 3', () => {
|
||||
assert.strictEqual(resolvePython({ platform: 'win32', probe: () => false }), null);
|
||||
assert.strictEqual(resolvePython({ platform: 'linux', probe: () => false }), null);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('real probe accepts success and preserves Windows launcher arguments', () => {
|
||||
const calls = [];
|
||||
const helper = loadWithSpawn((command, args, options) => {
|
||||
calls.push({ command, args: Array.from(args), options: { ...options } });
|
||||
return { status: command === 'py' ? 0 : 9009 };
|
||||
});
|
||||
const python = helper.resolvePython({ platform: 'win32' });
|
||||
assert.strictEqual(python.command, 'py');
|
||||
assert.deepStrictEqual(Array.from(python.prefix), ['-3']);
|
||||
assert.deepStrictEqual(calls.map(call => call.command), ['python', 'py']);
|
||||
assert.deepStrictEqual(calls[1].args.slice(0, 2), ['-3', '-c']);
|
||||
assert.deepStrictEqual(calls[1].options, { encoding: 'utf8', timeout: 15000 });
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('real probe rejects nonzero, missing commands and timed out processes', () => {
|
||||
for (const result of [
|
||||
{ status: 1 },
|
||||
{ status: 9009 },
|
||||
{ status: null, error: Object.assign(new Error('not found'), { code: 'ENOENT' }) },
|
||||
{ status: null, error: Object.assign(new Error('timed out'), { code: 'ETIMEDOUT' }) },
|
||||
]) {
|
||||
const calls = [];
|
||||
const helper = loadWithSpawn(command => { calls.push(command); return result; });
|
||||
assert.strictEqual(helper.resolvePython({ platform: 'win32' }), null);
|
||||
assert.deepStrictEqual(calls, ['python', 'py', 'python3']);
|
||||
}
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('real probe code succeeds under known Python 3 and rejects Python 2', () => {
|
||||
// Select independently so broken discovery cannot silently skip this control.
|
||||
const known = [['python3'], ['python'], ['py', '-3']].find(([command, ...prefix]) => {
|
||||
const result = spawnSync(command, [...prefix, '-c', 'import sys; print(sys.version_info[0])'], { encoding: 'utf8', timeout: 15000 });
|
||||
return result.status === 0 && result.stdout.trim() === '3';
|
||||
});
|
||||
assert.ok(known, 'Python 3 is required for the Hermes test workflow');
|
||||
for (const [major, expected] of [[3, true], [2, false]]) {
|
||||
const helper = loadWithSpawn((_command, args, options) => {
|
||||
const code = args[args.indexOf('-c') + 1];
|
||||
const wrapped = `import sys; sys.version_info = (${major}, 0);\n${code}`;
|
||||
return spawnSync(known[0], [...known.slice(1), '-c', wrapped], options);
|
||||
});
|
||||
assert.strictEqual(helper.resolvePython({ platform: 'win32' }) !== null, expected);
|
||||
}
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('a resolved interpreter actually runs Python 3 here', () => {
|
||||
const python = resolvePython();
|
||||
if (!python) return;
|
||||
const run = spawnSync(python.command, [...python.prefix, '-c', 'import sys; print(sys.version_info[0])'], { encoding: 'utf8' });
|
||||
assert.strictEqual(run.status, 0, run.stderr);
|
||||
assert.strictEqual(run.stdout.trim(), '3');
|
||||
})) passed++; else failed++;
|
||||
} finally {
|
||||
fs.rmSync(root, { recursive: true, force: true });
|
||||
}
|
||||
|
||||
@@ -93,6 +93,28 @@ async function runTests() {
|
||||
let passed = 0;
|
||||
let failed = 0;
|
||||
|
||||
if (
|
||||
await test('rejects malformed issue numbers during argument parsing', async () => {
|
||||
const { parseArgs } = require(SCRIPT);
|
||||
|
||||
for (const argv of [
|
||||
['node', 'github-coordination.js', 'claim', '42oops', '--repo', 'example/project'],
|
||||
['node', 'github-coordination.js', 'claim', '--issue', '42.9', '--repo', 'example/project'],
|
||||
['node', 'github-coordination.js', 'claim', '--issue', '4e2', '--repo', 'example/project'],
|
||||
['node', 'github-coordination.js', 'claim', '--issue', '9007199254740993', '--repo', 'example/project'],
|
||||
['node', 'github-coordination.js', 'claim', '--limit', '42.9', '--repo', 'example/project'],
|
||||
]) {
|
||||
assert.throws(() => parseArgs(argv), /Invalid issue number/);
|
||||
}
|
||||
|
||||
const valid = parseArgs(['node', 'github-coordination.js', 'claim', '7', '--repo', 'example/project']);
|
||||
assert.strictEqual(valid.issueNumber, 7);
|
||||
assert.strictEqual(valid.limit, 100);
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
|
||||
if (
|
||||
await test('claims an epic issue, updates GitHub state, and caches a work item', async () => {
|
||||
const rootDir = createTempDir('github-coordination-claim-');
|
||||
|
||||
Reference in new issue
Block a user