mirror of
https://github.com/affaan-m/ECC.git
synced 2026-10-20 23:11:46 +02:00
fix: address CodeRabbit and Greptile review findings
- Reject missing values for --ecc-root/--out with exit code 2 (CodeRabbit) | - Preserve regex matcher tokens with metacharacters or mcp__ prefixes from lower-casing (CodeRabbit) | - Shell-quote eccRoot in the env prefix to close CWE-78 command substitution (CodeRabbit, Greptile P1) | - Fix last_verified_at future date in the deepseek-harness record (CodeRabbit) | - Guide and skill: clarify skill-name placeholders and make the compose check verify the entry instead of echoing OK on exit status alone (Greptile P2 x2) | - Tests: 11 passing including new coverage for quoting, regex preservation, and flag validation
This commit is contained in:
@@ -35,6 +35,10 @@ To expose the full curated set in every project, link the catalog once:
|
||||
ln -s /path/to/ECC/skills/<skill-name> ~/.dsh/skills/<skill-name>
|
||||
```
|
||||
|
||||
Run this per skill with real names substituted for both placeholders (for
|
||||
example `ln -s ~/ECC/skills/tdd-workflow ~/.dsh/skills/tdd-workflow`); the
|
||||
angle brackets are placeholders, not shell syntax.
|
||||
|
||||
DSH reads `SKILL.md` frontmatter (name/description), which matches the ECC skill
|
||||
format, so no conversion is needed. Name conflicts with pre-existing user skills
|
||||
are resolved by root priority; skip those skills when linking.
|
||||
@@ -96,14 +100,21 @@ Configuration notes from practice:
|
||||
### Verify
|
||||
|
||||
```bash
|
||||
# Compose check (no session boot)
|
||||
cd ~/.dsh && dsh --profile web --dump-config > /dev/null && echo OK
|
||||
# Compose check (no session boot). Keep stderr visible: silently dropped or
|
||||
# rejected patch entries are reported there while the exit status stays 0.
|
||||
cd ~/.dsh && dsh --profile web --dump-config > /tmp/dump.yml
|
||||
|
||||
grep ecc-hooks /tmp/dump.yml
|
||||
|
||||
# End-to-end: fire a SessionStart hook in a one-shot headless session
|
||||
# (uses a test config with a single marker-writing hook)
|
||||
dsh --profile headless --patch /tmp/ecc-patch-test.yml 'Reply with exactly: BOOT-OK'
|
||||
```
|
||||
|
||||
A successful compose does not prove the entry mounted: dropped entries and
|
||||
"is not a group" rejections only appear on stderr, never in the exit status.
|
||||
Confirm the entry id exists in the dump and that stderr was empty.
|
||||
|
||||
In a live web session, the GateGuard fact-forcing hook denies the first
|
||||
edit/write/bash attempt until facts are presented — a visible signal that the
|
||||
bridge is active.
|
||||
|
||||
@@ -45,8 +45,15 @@ function parseArgs(argv) {
|
||||
const opts = { eccRoot: null, out: null };
|
||||
for (let i = 2; i < argv.length; i++) {
|
||||
const a = argv[i];
|
||||
if (a === '--ecc-root') opts.eccRoot = argv[++i];
|
||||
else if (a === '--out') opts.out = argv[++i];
|
||||
if (a === '--ecc-root' || a === '--out') {
|
||||
const value = argv[++i];
|
||||
if (value === undefined || value.startsWith('--')) {
|
||||
process.stderr.write('Missing value for ' + a + '\n');
|
||||
process.exit(2);
|
||||
}
|
||||
if (a === '--ecc-root') opts.eccRoot = value;
|
||||
else opts.out = value;
|
||||
}
|
||||
else {
|
||||
process.stderr.write('Unknown argument: ' + a + '\n');
|
||||
process.exit(2);
|
||||
@@ -64,7 +71,7 @@ function mapMatcher(matcher) {
|
||||
if (typeof matcher !== 'string' || matcher.length === 0) return matcher;
|
||||
const parts = matcher.split('|').map(function (t) {
|
||||
const v = t.trim();
|
||||
if (v === '' || v === '.*' || v === '^mcp__') return v;
|
||||
if (v === '' || /^\^?mcp__/.test(v) || /[\\^$.*+?()[\]{}]/.test(v)) return v;
|
||||
const mapped = TOOL_NAME_MAP[v];
|
||||
return mapped !== undefined ? mapped : v.toLowerCase();
|
||||
});
|
||||
@@ -81,9 +88,13 @@ function mapMatcher(matcher) {
|
||||
* CLAUDE_PLUGIN_ROOT environment variable, so prefixing the variables
|
||||
* inline (env-prefix form) is the only required change.
|
||||
*/
|
||||
function shellQuote(value) {
|
||||
return "'" + String(value).replace(/'/g, "'\\''") + "'";
|
||||
}
|
||||
|
||||
function mapCommand(command, eccRoot) {
|
||||
const prefix = 'CLAUDE_PLUGIN_ROOT=' + JSON.stringify(eccRoot) +
|
||||
' ECC_PLUGIN_ROOT=' + JSON.stringify(eccRoot) + ' ';
|
||||
const prefix = 'CLAUDE_PLUGIN_ROOT=' + shellQuote(eccRoot) +
|
||||
' ECC_PLUGIN_ROOT=' + shellQuote(eccRoot) + ' ';
|
||||
return prefix + command;
|
||||
}
|
||||
|
||||
@@ -149,6 +160,6 @@ function main() {
|
||||
if (notes.length) process.stderr.write(notes.join('; ') + '\n');
|
||||
}
|
||||
|
||||
module.exports = { generateConfig, mapMatcher, mapCommand, SUPPORTED_EVENTS, TOOL_NAME_MAP };
|
||||
module.exports = { generateConfig, mapMatcher, mapCommand, shellQuote, SUPPORTED_EVENTS, TOOL_NAME_MAP };
|
||||
|
||||
if (require.main === module) main();
|
||||
|
||||
@@ -284,7 +284,7 @@ const ADAPTER_RECORDS = Object.freeze([
|
||||
'Keep the tool-name matcher mapping (Bash to bash, Edit/MultiEdit to edit) in sync with DSH tool names',
|
||||
'A plain top-level patch entry is silently dropped; additions require a bare insert entry',
|
||||
],
|
||||
last_verified_at: '2026-10-02',
|
||||
last_verified_at: '2026-10-01',
|
||||
owner: 'ECC maintainers',
|
||||
source_docs: [
|
||||
'docs/DEEPSEEK-HARNESS-GUIDE.md',
|
||||
|
||||
@@ -26,6 +26,8 @@ Run ECC on the DeepSeek Harness. Full guide: `docs/DEEPSEEK-HARNESS-GUIDE.md`.
|
||||
|
||||
Root priority (project `.dsh` and `.agents` roots beat `~/.dsh`): skip
|
||||
linking skills whose names already exist in a higher-priority root.
|
||||
Substitute real skill names for the placeholders; the angle brackets are
|
||||
not shell syntax.
|
||||
|
||||
2. **Hooks go through the first-party bridge.** Generate a DSH-format config
|
||||
from the canonical ECC hooks file:
|
||||
@@ -57,11 +59,15 @@ Run ECC on the DeepSeek Harness. Full guide: `docs/DEEPSEEK-HARNESS-GUIDE.md`.
|
||||
4. **Validate by composing, then verify live.**
|
||||
|
||||
```bash
|
||||
cd ~/.dsh && dsh --profile web --dump-config > /dev/null && echo OK
|
||||
cd ~/.dsh && dsh --profile web --dump-config > /tmp/dump.yml
|
||||
grep ecc-hooks /tmp/dump.yml
|
||||
```
|
||||
|
||||
A live GateGuard denial on the session's first edit/write/bash is the
|
||||
visible signal the bridge is active.
|
||||
Compose exit status stays 0 even when the loader drops or rejects an
|
||||
entry — those problems are only reported on stderr. Confirm the entry id
|
||||
appears in the dump and stderr is empty. A live GateGuard denial on the
|
||||
session's first edit/write/bash is the visible signal the bridge is
|
||||
active.
|
||||
|
||||
## Updating
|
||||
|
||||
|
||||
@@ -43,12 +43,12 @@ function runTests() {
|
||||
if (test('mapCommand prefixes plugin-root env vars and keeps the resolver intact', () => {
|
||||
const cmd = 'node -e "resolver" pretooluse:bash:check-console-log scripts/hooks/check-console-log.js standard';
|
||||
const out = gen.mapCommand(cmd, '/ecc');
|
||||
assert.strictEqual(out, 'CLAUDE_PLUGIN_ROOT="/ecc" ECC_PLUGIN_ROOT="/ecc" ' + cmd);
|
||||
assert.strictEqual(out, 'CLAUDE_PLUGIN_ROOT=\'/ecc\' ECC_PLUGIN_ROOT=\'/ecc\' ' + cmd);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('mapCommand passes through non-resolver commands with prefix only', () => {
|
||||
const out = gen.mapCommand('echo hi', '/ecc');
|
||||
assert.strictEqual(out, 'CLAUDE_PLUGIN_ROOT="/ecc" ECC_PLUGIN_ROOT="/ecc" echo hi');
|
||||
assert.strictEqual(out, 'CLAUDE_PLUGIN_ROOT=\'/ecc\' ECC_PLUGIN_ROOT=\'/ecc\' echo hi');
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('generateConfig skips unsupported events and reports them', () => {
|
||||
@@ -80,6 +80,28 @@ function runTests() {
|
||||
assert.ok(gen.SUPPORTED_EVENTS.has('SubagentStop'));
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('mapMatcher preserves case-sensitive regex tokens with metacharacters', () => {
|
||||
assert.strictEqual(gen.mapMatcher('^mcp__GitHub__.*'), '^mcp__GitHub__.*');
|
||||
assert.strictEqual(gen.mapMatcher('\\S'), '\\S');
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('mapCommand shell-quotes dangerous path characters', () => {
|
||||
const dollar = gen.mapCommand('echo hi', '/home/u/$proj');
|
||||
assert.ok(dollar.startsWith("CLAUDE_PLUGIN_ROOT='/home/u/$proj' "), 'literal inside single quotes: ' + dollar);
|
||||
const quoted = gen.mapCommand('echo hi', "/home/u'z");
|
||||
assert.ok(quoted.startsWith("CLAUDE_PLUGIN_ROOT='/home/u'\\''z' "), 'embedded quote escaped: ' + quoted);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('CLI rejects missing flag values with exit code 2', () => {
|
||||
const repoRoot = path.resolve(__dirname, '..', '..');
|
||||
const res = spawnSync('node', [
|
||||
path.join(repoRoot, 'scripts', 'dsh', 'generate-hooks-config.js'),
|
||||
'--ecc-root',
|
||||
], { encoding: 'utf8' });
|
||||
assert.strictEqual(res.status, 2, 'exit 2 on missing value, stderr: ' + res.stderr);
|
||||
assert.ok(res.stderr.includes('Missing value for --ecc-root'));
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('CLI end-to-end writes valid JSON to --out', () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-dsh-'));
|
||||
const outPath = path.join(tmp, 'hooks.dsh.json');
|
||||
|
||||
Reference in New Issue
Block a user