diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 588debd..25c78d2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,18 +64,42 @@ jobs: - name: Lint run: make lint + test: + runs-on: ubuntu-24.04 + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + # Node 20 rather than the runner's default: the hook scripts run under + # whatever Node the user's harness carries, so the floor is the version + # worth testing on. No matrix -- CONTRIBUTING.md has why. + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: "20" + + # No pip cache, for the same reason the Verify job gives: the test suite is + # stdlib-only, so there is nothing to install and nothing to key a cache on. + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.12" + + - name: Test + run: make test + # The required status check. Do NOT add a `name:` -- the ruleset matches the # context by job id, so naming it silently stops the check from matching. check: if: always() # When you add jobs above, add them to BOTH the needs list and the results # array below. - needs: [verify, lint] + needs: [verify, lint, test] runs-on: ubuntu-24.04 timeout-minutes: 2 steps: - run: | - results=("${{ needs.verify.result }}" "${{ needs.lint.result }}") + results=("${{ needs.verify.result }}" "${{ needs.lint.result }}" "${{ needs.test.result }}") for r in "${results[@]}"; do if [[ "$r" != "success" && "$r" != "skipped" ]]; then echo "Check failed: $r" diff --git a/CLAUDE.md b/CLAUDE.md index f8a2e07..43d4f31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,10 +60,11 @@ done, confirm: ```bash make verify make lint +make test ``` -`make test` joins these two once the suite lands -- it is specified in -`CONTRIBUTING.md` and arrives with its first tests, not ahead of them. +`CONTRIBUTING.md` has what `make test` covers, what it deliberately does not, +and where a new test goes. CI runs the same checks. `AGENTS.md` is a symlink to this file, not a second source of truth -- edit `CLAUDE.md`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2501964..482fdb2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,10 +22,12 @@ error. ```bash make lint make verify +make test ``` -Both must pass -- CI runs the same checks. `make help` lists what each target -covers. `make test` joins them once the suite lands; see [Tests](#tests). +All three must pass -- CI runs the same checks. `make help` lists what each +target covers, and [Tests](#tests) has what the suite covers, what it +deliberately does not, and where a new test goes. ## Tests diff --git a/Makefile b/Makefile index b57ab07..0e5336b 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ # CI calls these targets directly, so this file is the single definition of the # checks. Recipes run under dash both here and on the runners -- no bashisms. -.PHONY: help install lint verify +.PHONY: help install lint verify test help: ## Show available targets @grep -E '^[a-zA-Z_-]+:.*?## ' $(MAKEFILE_LIST) | awk 'BEGIN{FS=":.*?## "}{printf " %-12s %s\n", $$1, $$2}' @@ -29,3 +29,21 @@ verify: ## Run the correctness gate (ASCII, JSON parses, Codex drift) @python3 -c "import json,subprocess; files=subprocess.run(['git','ls-files','*.json'],capture_output=True,text=True,check=True).stdout.split(); assert files, 'git ls-files matched no JSON -- gate would pass having checked nothing'; [json.load(open(f)) for f in files]" @echo "Checking generated Codex files against their sources..." @python3 scripts/generate-codex.py --check + +# Each half hands its runner an explicit file list from `git ls-files`, and +# asserts the list is non-empty for the same reason the JSON check above does: +# `git ls-files` exits 0 on no match and `node --test` with no arguments walks +# the whole tree instead of failing, so an unguarded list is a silent pass. +# Why a list rather than a directory or a glob: see CONTRIBUTING.md. +# +# Node runs first, and Make stops at the first failing line, so a Node failure +# hides the Python result -- the same trade `verify` makes above. +test: ## Run the unit tests and hook process contracts (Node, then Python) + @echo "Running the Node tests..." + @files=$$(git ls-files 'tests/*.test.js'); \ + if [ -z "$$files" ]; then echo "ERROR: git ls-files matched no Node tests -- gate would pass having checked nothing"; exit 1; fi; \ + node --test $$files + @echo "Running the Python tests..." + @files=$$(git ls-files 'tests/test_*.py' 'tests/*/test_*.py'); \ + if [ -z "$$files" ]; then echo "ERROR: git ls-files matched no Python tests -- gate would pass having checked nothing"; exit 1; fi; \ + python3 -m unittest $$files diff --git a/plugins/praxis/.claude-plugin/plugin.json b/plugins/praxis/.claude-plugin/plugin.json index 79f7da6..6435c66 100644 --- a/plugins/praxis/.claude-plugin/plugin.json +++ b/plugins/praxis/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "praxis", - "version": "1.4.0", + "version": "1.4.2", "description": "Development workflow -- issue planning, implementation, PR creation, code review with specialized reviewers, and project conventions", "author": { "name": "Jartan LLC", diff --git a/plugins/praxis/.codex-plugin/plugin.json b/plugins/praxis/.codex-plugin/plugin.json index b640740..e0e4484 100644 --- a/plugins/praxis/.codex-plugin/plugin.json +++ b/plugins/praxis/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "praxis", - "version": "1.4.0", + "version": "1.4.2", "description": "Development workflow -- issue planning, implementation, PR creation, code review with specialized reviewers, and project conventions", "interface": { "displayName": "Praxis", diff --git a/plugins/praxis/hooks/scripts/lib/transcript-context.js b/plugins/praxis/hooks/scripts/lib/transcript-context.js index cf82dbd..886177d 100644 --- a/plugins/praxis/hooks/scripts/lib/transcript-context.js +++ b/plugins/praxis/hooks/scripts/lib/transcript-context.js @@ -176,6 +176,8 @@ function resolveContextInterval(env) { module.exports = { MAX_TOKEN_SETTING, + readFileTail, + extractUsageTokens, readLatestContextTokens, resolveContextThreshold, resolveContextInterval diff --git a/plugins/praxis/hooks/scripts/lib/utils.js b/plugins/praxis/hooks/scripts/lib/utils.js index a5d7d65..9192466 100644 --- a/plugins/praxis/hooks/scripts/lib/utils.js +++ b/plugins/praxis/hooks/scripts/lib/utils.js @@ -499,6 +499,7 @@ module.exports = { getProjectName, // File operations + filterByPatterns, findFiles, readFile, writeFile, diff --git a/plugins/praxis/skills/code-hygiene/find-duplicate-comments.js b/plugins/praxis/skills/code-hygiene/find-duplicate-comments.js index df3b1d5..b769a39 100644 --- a/plugins/praxis/skills/code-hygiene/find-duplicate-comments.js +++ b/plugins/praxis/skills/code-hygiene/find-duplicate-comments.js @@ -318,6 +318,6 @@ function main() { report(dedupePairs(findings)); } -module.exports = { buildSkipMatcher, prose, buildCommentIndex, findRetoldInDiff, dedupePairs }; +module.exports = { parseArgs, buildSkipMatcher, prose, trackedPaths, buildCommentIndex, findRetoldInDiff, dedupePairs }; if (require.main === module) main(); diff --git a/tests/plugins/praxis/hooks/scripts/check-console-log.test.js b/tests/plugins/praxis/hooks/scripts/check-console-log.test.js new file mode 100644 index 0000000..98e21f0 --- /dev/null +++ b/tests/plugins/praxis/hooks/scripts/check-console-log.test.js @@ -0,0 +1,192 @@ +// Tests for the console.log hook: the report/carry-forward split, and the +// process contract every shipped hook entry point owes its caller. +// +// applyCooldown is where a wrong answer is silent -- a finding suppressed +// forever, or one repeated on every edit. Whether the advice is good advice, and +// what the default cooldown should be, are judgment calls and deliberately not +// asserted here. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const SCRIPT = path.resolve( + __dirname, + '../../../../../plugins/praxis/hooks/scripts/check-console-log.js' +); +const { applyCooldown } = require(SCRIPT); + +const COOLDOWN = 10; + +const finding = (file, text) => ({ file, lineNumber: 1, text, key: `${file}:${text}` }); +const A = finding('src/a.js', 'console.log(a)'); +const B = finding('src/b.js', 'console.log(b)'); + +const GIT_ENV = { + GIT_AUTHOR_NAME: 'Grimoire Fixture', + GIT_AUTHOR_EMAIL: 'fixture@example.invalid', + GIT_COMMITTER_NAME: 'Grimoire Fixture', + GIT_COMMITTER_EMAIL: 'fixture@example.invalid' +}; + +/** + * Spawn the hook and assert the contract it owes its caller: exit 0, and either + * nothing on stdout or exactly one well-formed JSON object. JSON.parse is the + * "exactly one" half -- two concatenated objects do not parse. + */ +function spawnHook({ stdin = '', cwd, env = {} } = {}) { + const r = spawnSync(process.execPath, [SCRIPT], { + input: stdin, + cwd, + encoding: 'utf8', + env: { ...process.env, ...env } + }); + + assert.equal(r.status, 0, `exited ${r.status}, signal ${r.signal}; stderr: ${r.stderr}`); + const out = r.stdout.trim(); + if (out === '') return null; + + const payload = JSON.parse(out); + assert.equal(typeof payload, 'object'); + assert.notEqual(payload, null); + return payload; +} + +/** A throwaway repository plus an isolated temp directory for the hook's state. */ +function withFixture(fn) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-console-')); + const repo = path.join(dir, 'repo'); + const state = path.join(dir, 'state'); + fs.mkdirSync(repo); + fs.mkdirSync(state); + + const git = args => { + const r = spawnSync('git', args, { cwd: repo, encoding: 'utf8', env: { ...process.env, ...GIT_ENV } }); + assert.equal(r.status, 0, `git ${args.join(' ')} failed: ${r.stderr}`); + }; + git(['init', '-q']); + git(['config', 'commit.gpgsign', 'false']); + fs.writeFileSync(path.join(repo, 'README.md'), 'fixture\n'); + git(['add', '-A']); + git(['commit', '-qm', 'fixture']); + + try { + return fn({ repo, env: { TMPDIR: state, TMP: state, TEMP: state } }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +test('applyCooldown reports a finding new to the session immediately', () => { + const { report, state } = applyCooldown([A], { seen: [], cooldown: 0 }, COOLDOWN); + assert.deepEqual(report, [A]); + assert.deepEqual(state, { seen: [A.key], cooldown: COOLDOWN }); +}); + +test('applyCooldown holds a repeat back until the cooldown elapses', () => { + const { report, state } = applyCooldown([A], { seen: [A.key], cooldown: 3 }, COOLDOWN); + assert.deepEqual(report, []); + assert.deepEqual(state, { seen: [A.key], cooldown: 2 }); +}); + +test('applyCooldown releases a repeat once the cooldown reaches zero', () => { + const { report, state } = applyCooldown([A], { seen: [A.key], cooldown: 0 }, COOLDOWN); + assert.deepEqual(report, [A]); + assert.deepEqual(state, { seen: [A.key], cooldown: COOLDOWN }); +}); + +test('applyCooldown restarts the cooldown whenever it reports something', () => { + // Otherwise a finding is announced and then immediately repeated on the next + // edit. + const { state } = applyCooldown([A], { seen: [], cooldown: 4 }, COOLDOWN); + assert.equal(state.cooldown, COOLDOWN); +}); + +test('applyCooldown reports the fresh finding while the repeat is still cooling', () => { + const { report, state } = applyCooldown([A, B], { seen: [B.key], cooldown: 5 }, COOLDOWN); + assert.deepEqual(report, [A]); + assert.deepEqual(state.seen.sort(), [A.key, B.key].sort()); + assert.equal(state.cooldown, COOLDOWN); +}); + +test('applyCooldown stops decrementing at zero', () => { + const { report, state } = applyCooldown([A], { seen: [A.key], cooldown: 0 }, COOLDOWN); + assert.deepEqual(report, [A]); + + const quiet = applyCooldown([], { seen: [], cooldown: 0 }, COOLDOWN); + assert.deepEqual(quiet.report, []); + assert.equal(quiet.state.cooldown, 0); +}); + +test('applyCooldown drops a finding that has disappeared out of seen', () => { + const { state } = applyCooldown([], { seen: [A.key, B.key], cooldown: 3 }, COOLDOWN); + assert.deepEqual(state.seen, []); + assert.equal(state.cooldown, 2); +}); + +test('applyCooldown counts a reintroduced statement as new again', () => { + // Keyed on the line's text rather than its number, so an edit above a debug + // statement is not a new finding -- but removing and re-adding it is. + let state = applyCooldown([A], { seen: [], cooldown: 0 }, COOLDOWN).state; + assert.deepEqual(state.seen, [A.key]); + + state = applyCooldown([], state, COOLDOWN).state; + assert.deepEqual(state.seen, []); + + const again = applyCooldown([A], state, COOLDOWN); + assert.deepEqual(again.report, [A]); + assert.equal(again.state.cooldown, COOLDOWN); +}); + +test('applyCooldown rebuilds seen from what is present, never growing it', () => { + const { state } = applyCooldown([A], { seen: [A.key, 'src/gone.js:console.log(gone)'], cooldown: 0 }, COOLDOWN); + assert.deepEqual(state.seen, [A.key]); +}); + +test('applyCooldown honours the cooldown length it is handed', () => { + assert.equal(applyCooldown([A], { seen: [], cooldown: 0 }, 3).state.cooldown, 3); +}); + +test('contract: malformed JSON on stdin', () => { + withFixture(({ repo, env }) => { + assert.equal(spawnHook({ stdin: '{not json', cwd: repo, env }), null); + }); +}); + +test('contract: empty stdin', () => { + withFixture(({ repo, env }) => { + assert.equal(spawnHook({ stdin: '', cwd: repo, env }), null); + }); +}); + +test('contract: a payload with no transcript_path and no session_id', () => { + withFixture(({ repo, env }) => { + assert.equal(spawnHook({ stdin: JSON.stringify({ hook_event_name: 'PostToolUse' }), cwd: repo, env }), null); + }); +}); + +test('contract: outside a git repository', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-console-bare-')); + try { + assert.equal( + spawnHook({ stdin: '{}', cwd: dir, env: { TMPDIR: dir, TMP: dir, TEMP: dir } }), + null + ); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('contract: a real finding travels as exactly one JSON object', () => { + withFixture(({ repo, env }) => { + fs.mkdirSync(path.join(repo, 'src')); + fs.writeFileSync(path.join(repo, 'src', 'app.js'), 'console.log("debug");\n'); + + const payload = spawnHook({ stdin: JSON.stringify({ session_id: 'contract-test' }), cwd: repo, env }); + assert.equal(payload.hookSpecificOutput.hookEventName, 'PostToolUse'); + assert.match(payload.hookSpecificOutput.additionalContext, /console\.log found in src\/app\.js/); + }); +}); diff --git a/tests/plugins/praxis/hooks/scripts/lib/session-state.test.js b/tests/plugins/praxis/hooks/scripts/lib/session-state.test.js new file mode 100644 index 0000000..e4e28d8 Binary files /dev/null and b/tests/plugins/praxis/hooks/scripts/lib/session-state.test.js differ diff --git a/tests/plugins/praxis/hooks/scripts/lib/transcript-context.test.js b/tests/plugins/praxis/hooks/scripts/lib/transcript-context.test.js new file mode 100644 index 0000000..a185340 --- /dev/null +++ b/tests/plugins/praxis/hooks/scripts/lib/transcript-context.test.js @@ -0,0 +1,227 @@ +// Tests for the strategic-compact hook's token accounting and setting resolvers. +// +// The silent-wrong failure modes here are a context size reported at twice its +// real value, and a threshold that reads as disabled when it is not. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { + MAX_TOKEN_SETTING, + readFileTail, + extractUsageTokens, + readLatestContextTokens, + resolveContextThreshold, + resolveContextInterval +} = require('../../../../../../plugins/praxis/hooks/scripts/lib/transcript-context'); + +const DEFAULT_THRESHOLD = 160000; +const DEFAULT_INTERVAL = 60000; + +/** Write `contents` into a throwaway directory and hand back the path. */ +function tempFile(contents, name = 'transcript.jsonl') { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-transcript-')); + const file = path.join(dir, name); + fs.writeFileSync(file, contents); + return { dir, file }; +} + +function withTempFile(contents, fn) { + const { dir, file } = tempFile(contents); + try { + return fn(file); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +const usageRecord = usage => JSON.stringify({ message: { usage } }); + +test('extractUsageTokens sums the fields that partition the prompt', () => { + const tokens = extractUsageTokens({ + message: { usage: { input_tokens: 1000, cache_read_input_tokens: 20000, cache_creation_input_tokens: 300 } } + }); + assert.equal(tokens, 21300); +}); + +test('extractUsageTokens treats an absent or non-numeric field as zero', () => { + assert.equal(extractUsageTokens({ message: { usage: { input_tokens: 500 } } }), 500); + assert.equal( + extractUsageTokens({ message: { usage: { input_tokens: 500, cache_read_input_tokens: 'lots' } } }), + 500 + ); +}); + +test('extractUsageTokens returns zero when there is no usable usage block', () => { + assert.equal(extractUsageTokens(null), 0); + assert.equal(extractUsageTokens({}), 0); + assert.equal(extractUsageTokens({ message: {} }), 0); + assert.equal(extractUsageTokens({ message: { usage: null } }), 0); + assert.equal(extractUsageTokens({ message: { usage: 'nope' } }), 0); + assert.equal(extractUsageTokens({ message: { usage: {} } }), 0); +}); + +test('extractUsageTokens takes the largest iteration, not the aggregate', () => { + // On a multi-iteration turn the top-level fields aggregate ACROSS iterations, + // so summing them reports a context that was never that large -- 2.00x here. + // Each iteration re-sends the prompt, so the largest single one is the truth. + const tokens = extractUsageTokens({ + message: { + usage: { + input_tokens: 200000, + iterations: [{ input_tokens: 100000 }, { input_tokens: 100000 }] + } + } + }); + assert.equal(tokens, 100000); +}); + +test('extractUsageTokens sums each iteration across its own prompt fields', () => { + const tokens = extractUsageTokens({ + message: { + usage: { + input_tokens: 999999, + iterations: [ + { input_tokens: 10, cache_read_input_tokens: 20 }, + { input_tokens: 100, cache_read_input_tokens: 200, cache_creation_input_tokens: 5 } + ] + } + } + }); + assert.equal(tokens, 305); +}); + +test('extractUsageTokens falls back to the top level when no iteration is usable', () => { + const usage = { input_tokens: 4200, iterations: [null, {}, 'junk'] }; + assert.equal(extractUsageTokens({ message: { usage } }), 4200); + assert.equal(extractUsageTokens({ message: { usage: { input_tokens: 4200, iterations: [] } } }), 4200); + assert.equal(extractUsageTokens({ message: { usage: { input_tokens: 4200, iterations: 'no' } } }), 4200); +}); + +test('readFileTail returns null for a file it cannot open', () => { + assert.equal(readFileTail(path.join(os.tmpdir(), 'grimoire-does-not-exist-98217'), 1024), null); +}); + +test('readFileTail reads a whole file that fits, and reports it untruncated', () => { + withTempFile('one\ntwo\n', file => { + assert.deepEqual(readFileTail(file, 1024), { text: 'one\ntwo\n', truncated: false }); + }); +}); + +test('readFileTail reads only the tail of a larger file, and says so', () => { + withTempFile('0123456789', file => { + assert.deepEqual(readFileTail(file, 4), { text: '6789', truncated: true }); + }); +}); + +test('readFileTail reports an empty file as empty and untruncated', () => { + withTempFile('', file => { + assert.deepEqual(readFileTail(file, 1024), { text: '', truncated: false }); + }); +}); + +test('readLatestContextTokens rejects a path that is not a usable string', () => { + assert.equal(readLatestContextTokens(''), null); + assert.equal(readLatestContextTokens(undefined), null); + assert.equal(readLatestContextTokens(42), null); +}); + +test('readLatestContextTokens returns null for a missing transcript', () => { + assert.equal(readLatestContextTokens(path.join(os.tmpdir(), 'grimoire-no-transcript-31337.jsonl')), null); +}); + +test('readLatestContextTokens takes the most recent usable record', () => { + const lines = [ + usageRecord({ input_tokens: 111 }), + usageRecord({ input_tokens: 222 }), + usageRecord({ input_tokens: 333 }) + ]; + withTempFile(lines.join('\n') + '\n', file => { + assert.deepEqual(readLatestContextTokens(file), { tokens: 333 }); + }); +}); + +test('readLatestContextTokens scans past blank, unparsable and usage-free records', () => { + const lines = [ + usageRecord({ input_tokens: 777 }), + '{"message":{"usage":{}}}', + 'not json at all', + '{"type":"user","message":{"content":"hi"}}', + '' + ]; + withTempFile(lines.join('\n') + '\n', file => { + assert.deepEqual(readLatestContextTokens(file), { tokens: 777 }); + }); +}); + +test('readLatestContextTokens returns null when no record carries usage', () => { + withTempFile('{"type":"user"}\nnot json\n\n', file => { + assert.equal(readLatestContextTokens(file), null); + }); +}); + +test('readLatestContextTokens distrusts the first line of a truncated tail', () => { + // The first line of a tail read is almost certainly partial JSON. The guard is + // unconditional, so a complete record sitting exactly on the cut is dropped + // too -- deliberately, since the read cannot tell the two apart. + const record = usageRecord({ input_tokens: 999999 }); + const contents = `noise\n${record}\nnot json\n`; + withTempFile(contents, file => { + const size = Buffer.byteLength(contents); + // Cut exactly after "noise\n", so the record is the tail's first line. + assert.equal(readLatestContextTokens(file, { tailBytes: size - 'noise\n'.length }), null); + // Read whole, the same record is trusted and found. + assert.deepEqual(readLatestContextTokens(file, { tailBytes: size }), { tokens: 999999 }); + }); +}); + +test('readLatestContextTokens falls back to the default tail for an unusable tailBytes', () => { + withTempFile(usageRecord({ input_tokens: 42 }) + '\n', file => { + for (const tailBytes of [0, -1, 1.5, 'big', null]) { + assert.deepEqual(readLatestContextTokens(file, { tailBytes }), { tokens: 42 }, `tailBytes=${tailBytes}`); + } + }); +}); + +test('resolveContextThreshold defaults when the setting is absent', () => { + assert.equal(resolveContextThreshold({}), DEFAULT_THRESHOLD); + assert.equal(resolveContextThreshold({ COMPACT_CONTEXT_THRESHOLD: '' }), DEFAULT_THRESHOLD); + assert.equal(resolveContextThreshold({ COMPACT_CONTEXT_THRESHOLD: null }), DEFAULT_THRESHOLD); + assert.equal(resolveContextThreshold(undefined), DEFAULT_THRESHOLD); +}); + +test('resolveContextThreshold treats zero as a disable switch', () => { + assert.equal(resolveContextThreshold({ COMPACT_CONTEXT_THRESHOLD: '0' }), 0); +}); + +test('resolveContextThreshold accepts an in-range value up to the maximum', () => { + assert.equal(resolveContextThreshold({ COMPACT_CONTEXT_THRESHOLD: '90000' }), 90000); + assert.equal(resolveContextThreshold({ COMPACT_CONTEXT_THRESHOLD: String(MAX_TOKEN_SETTING) }), MAX_TOKEN_SETTING); +}); + +test('resolveContextThreshold falls back for anything invalid or out of range', () => { + for (const raw of ['-1', 'lots', String(MAX_TOKEN_SETTING + 1)]) { + assert.equal(resolveContextThreshold({ COMPACT_CONTEXT_THRESHOLD: raw }), DEFAULT_THRESHOLD, raw); + } +}); + +test('resolveContextInterval defaults when the setting is absent or invalid', () => { + assert.equal(resolveContextInterval({}), DEFAULT_INTERVAL); + assert.equal(resolveContextInterval(undefined), DEFAULT_INTERVAL); + assert.equal(resolveContextInterval({ COMPACT_CONTEXT_INTERVAL: 'often' }), DEFAULT_INTERVAL); + assert.equal(resolveContextInterval({ COMPACT_CONTEXT_INTERVAL: '-5' }), DEFAULT_INTERVAL); + assert.equal(resolveContextInterval({ COMPACT_CONTEXT_INTERVAL: String(MAX_TOKEN_SETTING + 1) }), DEFAULT_INTERVAL); +}); + +test('resolveContextInterval has no disable switch, unlike the threshold', () => { + // The interval only spaces out repeats; zero is not an off state for it. + assert.equal(resolveContextInterval({ COMPACT_CONTEXT_INTERVAL: '0' }), DEFAULT_INTERVAL); +}); + +test('resolveContextInterval accepts an in-range value up to the maximum', () => { + assert.equal(resolveContextInterval({ COMPACT_CONTEXT_INTERVAL: '25000' }), 25000); + assert.equal(resolveContextInterval({ COMPACT_CONTEXT_INTERVAL: String(MAX_TOKEN_SETTING) }), MAX_TOKEN_SETTING); +}); diff --git a/tests/plugins/praxis/hooks/scripts/lib/utils.test.js b/tests/plugins/praxis/hooks/scripts/lib/utils.test.js new file mode 100644 index 0000000..122cf6e --- /dev/null +++ b/tests/plugins/praxis/hooks/scripts/lib/utils.test.js @@ -0,0 +1,239 @@ +// Tests for the shared hook helpers that do real work on their arguments: the +// pattern filter, the two regex-flag guards, and the glob-to-regex conversion. +// +// The thin wrappers over fs and child_process in the same module are deliberately +// untested -- the assertion would be that Node works. CONTRIBUTING.md has the +// full list and why. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { + filterByPatterns, + findFiles, + countInFile, + grepFile +} = require('../../../../../../plugins/praxis/hooks/scripts/lib/utils'); + +const FILES = ['src/app.ts', 'src/app.tsx', 'src/app.js', 'docs/readme.md']; + +function withTempDir(fn) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-utils-')); + try { + return fn(dir); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +function write(dir, relative, contents = 'x\n', ageMs = 0) { + const file = path.join(dir, relative); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, contents); + if (ageMs > 0) { + const when = (Date.now() - ageMs) / 1000; + fs.utimesSync(file, when, when); + } + return file; +} + +const missing = () => path.join(os.tmpdir(), 'grimoire-utils-absent-55123.txt'); + +test('filterByPatterns leaves the list alone when there are no patterns', () => { + assert.deepEqual(filterByPatterns(FILES, []), FILES); +}); + +test('filterByPatterns keeps only what a pattern matches', () => { + assert.deepEqual(filterByPatterns(FILES, ['\\.tsx?$']), ['src/app.ts', 'src/app.tsx']); +}); + +test('filterByPatterns unions its patterns', () => { + assert.deepEqual(filterByPatterns(FILES, ['\\.tsx?$', '\\.jsx?$']), ['src/app.ts', 'src/app.tsx', 'src/app.js']); +}); + +test('filterByPatterns skips an invalid pattern and applies the rest', () => { + assert.deepEqual(filterByPatterns(FILES, ['[unclosed', '\\.md$']), ['docs/readme.md']); +}); + +test('filterByPatterns leaves the list alone when every pattern is unusable', () => { + // Compiling nothing must not mean matching nothing: an all-invalid list that + // filtered everything away would report a clean tree to every caller. + assert.deepEqual(filterByPatterns(FILES, ['[unclosed', '(']), FILES); + assert.deepEqual(filterByPatterns(FILES, ['', null, undefined, 42]), FILES); +}); + +test('filterByPatterns matches anywhere in the path, not just the end', () => { + assert.deepEqual(filterByPatterns(FILES, ['^src/']), ['src/app.ts', 'src/app.tsx', 'src/app.js']); + assert.deepEqual(filterByPatterns(FILES, ['app']), ['src/app.ts', 'src/app.tsx', 'src/app.js']); +}); + +test('countInFile counts every match of a string pattern', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'console.log(1)\nconsole.log(2)\nconsole.log(3)\n'); + assert.equal(countInFile(file, 'console\\.log'), 3); + }); +}); + +test('countInFile counts every match of a RegExp that lacks the global flag', () => { + // Without the flag enforced, String.match returns the first match only and the + // count silently reads 1 however many there are. + withTempDir(dir => { + const file = write(dir, 'a.js', 'console.log(1)\nconsole.log(2)\nconsole.log(3)\n'); + assert.equal(countInFile(file, /console\.log/), 3); + }); +}); + +test('countInFile preserves the other flags on the pattern it is handed', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'CONSOLE.LOG(1)\nconsole.log(2)\n'); + assert.equal(countInFile(file, /console\.log/i), 2); + assert.equal(countInFile(file, /console\.log/), 1); + }); +}); + +test('countInFile is not disturbed by a global RegExp reused across calls', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'console.log(1)\nconsole.log(2)\n'); + const shared = /console\.log/g; + assert.equal(countInFile(file, shared), 2); + assert.equal(countInFile(file, shared), 2); + }); +}); + +test('countInFile returns zero for a missing file, a bad pattern or a bad type', () => { + assert.equal(countInFile(missing(), 'x'), 0); + withTempDir(dir => { + const file = write(dir, 'a.js', 'x\n'); + assert.equal(countInFile(file, '[unclosed'), 0); + assert.equal(countInFile(file, 42), 0); + assert.equal(countInFile(file, null), 0); + }); +}); + +test('grepFile reports matching lines with 1-based numbers', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'const x = 1;\nconsole.log(x);\nconst y = 2;\n'); + assert.deepEqual(grepFile(file, /console\.log/), [{ lineNumber: 2, content: 'console.log(x);' }]); + }); +}); + +test('grepFile matches consecutive lines despite a global RegExp', () => { + // The g flag makes .test() stateful: lastIndex carries between calls, so + // consecutive matching lines alternate match/miss and half the findings vanish. + withTempDir(dir => { + const file = write(dir, 'a.js', 'console.log(1)\nconsole.log(2)\nconsole.log(3)\nconsole.log(4)\n'); + assert.deepEqual(grepFile(file, /console\.log/g).map(m => m.lineNumber), [1, 2, 3, 4]); + }); +}); + +test('grepFile gives the same answer when a global RegExp is reused', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'console.log(1)\nconsole.log(2)\n'); + const shared = /console\.log/g; + assert.deepEqual(grepFile(file, shared), grepFile(file, shared)); + }); +}); + +test('grepFile keeps the flags that are not g', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'CONSOLE.LOG(1)\nconst x = 2;\n'); + assert.deepEqual(grepFile(file, /console\.log/i).map(m => m.lineNumber), [1]); + assert.deepEqual(grepFile(file, /console\.log/), []); + }); +}); + +test('grepFile accepts a string pattern', () => { + withTempDir(dir => { + const file = write(dir, 'a.js', 'const x = 1;\nconsole.log(x);\n'); + assert.deepEqual(grepFile(file, 'console\\.log').map(m => m.lineNumber), [2]); + }); +}); + +test('grepFile returns nothing for a missing file or a bad pattern', () => { + assert.deepEqual(grepFile(missing(), /x/), []); + withTempDir(dir => { + assert.deepEqual(grepFile(write(dir, 'a.js', 'x\n'), '[unclosed'), []); + }); +}); + +test('findFiles converts the glob wildcards', () => { + withTempDir(dir => { + write(dir, 'a.tmp'); + write(dir, 'b.tmp'); + write(dir, 'c.md'); + assert.deepEqual(findFiles(dir, '*.tmp').map(f => path.basename(f.path)).sort(), ['a.tmp', 'b.tmp']); + + write(dir, 'log1.txt'); + write(dir, 'log22.txt'); + assert.deepEqual(findFiles(dir, 'log?.txt').map(f => path.basename(f.path)), ['log1.txt']); + }); +}); + +test('findFiles escapes the regex specials in a pattern', () => { + // Unescaped, the dot in `a.txt` is a wildcard and the pattern also matches + // `axtxt` -- a file the caller never asked for, deleted by a caller that + // sweeps what this returns. + withTempDir(dir => { + write(dir, 'a.txt'); + write(dir, 'axtxt'); + write(dir, 'a+b.txt'); + assert.deepEqual(findFiles(dir, 'a.txt').map(f => path.basename(f.path)), ['a.txt']); + assert.deepEqual(findFiles(dir, 'a+b.txt').map(f => path.basename(f.path)), ['a+b.txt']); + }); +}); + +test('findFiles anchors the pattern to the whole name', () => { + withTempDir(dir => { + write(dir, 'notes.md'); + write(dir, 'notes.md.bak'); + assert.deepEqual(findFiles(dir, '*.md').map(f => path.basename(f.path)), ['notes.md']); + }); +}); + +test('findFiles descends only when asked to', () => { + withTempDir(dir => { + write(dir, 'top.md'); + write(dir, 'nested/deep.md'); + assert.deepEqual(findFiles(dir, '*.md').map(f => path.basename(f.path)), ['top.md']); + assert.deepEqual( + findFiles(dir, '*.md', { recursive: true }).map(f => path.basename(f.path)).sort(), + ['deep.md', 'top.md'] + ); + }); +}); + +test('findFiles drops anything older than maxAge', () => { + withTempDir(dir => { + const day = 24 * 60 * 60 * 1000; + write(dir, 'fresh.md', 'x\n'); + write(dir, 'stale.md', 'x\n', 10 * day); + assert.deepEqual(findFiles(dir, '*.md', { maxAge: 5 }).map(f => path.basename(f.path)), ['fresh.md']); + assert.equal(findFiles(dir, '*.md', { maxAge: 30 }).length, 2); + }); +}); + +test('findFiles sorts newest first', () => { + withTempDir(dir => { + const hour = 60 * 60 * 1000; + write(dir, 'oldest.md', 'x\n', 3 * hour); + write(dir, 'newest.md', 'x\n'); + write(dir, 'middle.md', 'x\n', hour); + assert.deepEqual( + findFiles(dir, '*.md').map(f => path.basename(f.path)), + ['newest.md', 'middle.md', 'oldest.md'] + ); + }); +}); + +test('findFiles returns nothing for a missing directory or a bad argument', () => { + assert.deepEqual(findFiles(path.join(os.tmpdir(), 'grimoire-utils-nodir-8812'), '*.md'), []); + withTempDir(dir => { + assert.deepEqual(findFiles(dir, ''), []); + assert.deepEqual(findFiles(dir, null), []); + assert.deepEqual(findFiles(null, '*.md'), []); + assert.deepEqual(findFiles(42, '*.md'), []); + }); +}); diff --git a/tests/plugins/praxis/hooks/scripts/suggest-compact.test.js b/tests/plugins/praxis/hooks/scripts/suggest-compact.test.js new file mode 100644 index 0000000..803af93 --- /dev/null +++ b/tests/plugins/praxis/hooks/scripts/suggest-compact.test.js @@ -0,0 +1,125 @@ +// Process contract for the strategic-compact hook. +// +// The resolvers it composes are unit-tested in lib/transcript-context.test.js. +// What is left here is the contract: exit 0 and at most one JSON object, whatever +// arrives on stdin, with both signals sharing the single payload a hook is +// allowed. Every spawn gets its own temp directory, since the hook writes +// per-session state there. +// +// The thresholds and the wording are tuning, not facts, so only the disable +// switch and the shape of the payload are asserted. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const SCRIPT = path.resolve(__dirname, '../../../../../plugins/praxis/hooks/scripts/suggest-compact.js'); + +/** Spawn the hook with isolated state, asserting exit 0 and at most one JSON object. */ +function spawnHook({ stdin = '', env = {} } = {}) { + const state = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-compact-')); + try { + const r = spawnSync(process.execPath, [SCRIPT], { + input: stdin, + encoding: 'utf8', + env: { ...process.env, TMPDIR: state, TMP: state, TEMP: state, ...env } + }); + + assert.equal(r.status, 0, `exited ${r.status}, signal ${r.signal}; stderr: ${r.stderr}`); + const out = r.stdout.trim(); + if (out === '') return null; + + const payload = JSON.parse(out); + assert.equal(typeof payload, 'object'); + assert.notEqual(payload, null); + return payload; + } finally { + fs.rmSync(state, { recursive: true, force: true }); + } +} + +/** A one-record transcript reporting `tokens` of context. */ +function transcript(tokens) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-compact-tx-')); + const file = path.join(dir, 'transcript.jsonl'); + fs.writeFileSync(file, JSON.stringify({ message: { usage: { input_tokens: tokens } } }) + '\n'); + return { dir, file }; +} + +function withTranscript(tokens, fn) { + const { dir, file } = transcript(tokens); + try { + return fn(file); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +test('contract: malformed JSON on stdin', () => { + assert.equal(spawnHook({ stdin: '{"session_id": ' }), null); +}); + +test('contract: empty stdin', () => { + assert.equal(spawnHook({ stdin: '' }), null); +}); + +test('contract: a payload with no transcript_path', () => { + assert.equal(spawnHook({ stdin: JSON.stringify({ session_id: 'contract-test' }) }), null); +}); + +test('contract: a transcript_path that is not a string', () => { + assert.equal(spawnHook({ stdin: JSON.stringify({ session_id: 'x', transcript_path: 42 }) }), null); +}); + +test('contract: a transcript_path pointing at nothing', () => { + const absent = path.join(os.tmpdir(), 'grimoire-compact-absent-4471.jsonl'); + assert.equal(spawnHook({ stdin: JSON.stringify({ session_id: 'x', transcript_path: absent }) }), null); +}); + +test('contract: the count signal emits exactly one JSON object', () => { + const payload = spawnHook({ + stdin: JSON.stringify({ session_id: 'contract-test' }), + env: { COMPACT_THRESHOLD: '1' } + }); + assert.equal(payload.hookSpecificOutput.hookEventName, 'PreToolUse'); + assert.match(payload.hookSpecificOutput.additionalContext, /1 tool calls reached/); +}); + +test('contract: both signals share the one payload a hook is allowed', () => { + withTranscript(400000, file => { + const payload = spawnHook({ + stdin: JSON.stringify({ session_id: 'contract-test', transcript_path: file }), + env: { COMPACT_THRESHOLD: '1', COMPACT_CONTEXT_THRESHOLD: '1000' } + }); + const context = payload.hookSpecificOutput.additionalContext; + assert.match(context, /Context ~400k tokens/); + assert.match(context, /1 tool calls reached/); + }); +}); + +test('contract: COMPACT_CONTEXT_THRESHOLD=0 silences the context signal', () => { + withTranscript(400000, file => { + assert.equal( + spawnHook({ + stdin: JSON.stringify({ session_id: 'contract-test', transcript_path: file }), + env: { COMPACT_CONTEXT_THRESHOLD: '0' } + }), + null + ); + }); +}); + +test('contract: a transcript below the threshold says nothing', () => { + withTranscript(1000, file => { + assert.equal( + spawnHook({ + stdin: JSON.stringify({ session_id: 'contract-test', transcript_path: file }), + env: { COMPACT_CONTEXT_THRESHOLD: '160000' } + }), + null + ); + }); +}); diff --git a/tests/plugins/praxis/hooks/scripts/suggest-skills.test.js b/tests/plugins/praxis/hooks/scripts/suggest-skills.test.js new file mode 100644 index 0000000..13dcbc5 --- /dev/null +++ b/tests/plugins/praxis/hooks/scripts/suggest-skills.test.js @@ -0,0 +1,59 @@ +// Process contract for the skill-activation hook. +// +// The whole script is a gate on the incoming event, so there is nothing to +// import: the contract IS the behaviour. A hook that throws breaks the user's +// turn, and no other check in this repository can see that. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const SCRIPT = path.resolve(__dirname, '../../../../../plugins/praxis/hooks/scripts/suggest-skills.js'); + +/** + * Spawn the hook and assert exit 0 plus either no stdout or exactly one + * well-formed JSON object. JSON.parse is the "exactly one" half -- two + * concatenated objects do not parse. + */ +function spawnHook(stdin) { + const r = spawnSync(process.execPath, [SCRIPT], { input: stdin, encoding: 'utf8' }); + + assert.equal(r.status, 0, `exited ${r.status}, signal ${r.signal}; stderr: ${r.stderr}`); + const out = r.stdout.trim(); + if (out === '') return null; + + const payload = JSON.parse(out); + assert.equal(typeof payload, 'object'); + assert.notEqual(payload, null); + return payload; +} + +test('contract: malformed JSON on stdin', () => { + assert.equal(spawnHook('{"hook_event_name": '), null); +}); + +test('contract: empty stdin', () => { + assert.equal(spawnHook(''), null); +}); + +test('contract: a payload with no event name', () => { + // An unread or unparsed payload leaves the event unknown, and guessing would + // label the output with an event that did not happen. + assert.equal(spawnHook(JSON.stringify({ transcript_path: '/nowhere' })), null); + assert.equal(spawnHook(JSON.stringify({ hook_event_name: '' })), null); +}); + +test('contract: a prompt submission emits exactly one JSON object', () => { + const payload = spawnHook(JSON.stringify({ hook_event_name: 'UserPromptSubmit' })); + assert.equal(payload.hookSpecificOutput.hookEventName, 'UserPromptSubmit'); + assert.match(payload.hookSpecificOutput.additionalContext, /load them with Skill/); +}); + +test('contract: a session start says nothing unless a compact caused it', () => { + assert.equal(spawnHook(JSON.stringify({ hook_event_name: 'SessionStart', source: 'startup' })), null); + assert.equal(spawnHook(JSON.stringify({ hook_event_name: 'SessionStart' })), null); + + const payload = spawnHook(JSON.stringify({ hook_event_name: 'SessionStart', source: 'compact' })); + assert.equal(payload.hookSpecificOutput.hookEventName, 'SessionStart'); +}); diff --git a/tests/plugins/praxis/hooks/scripts/sync-codex-agents.test.js b/tests/plugins/praxis/hooks/scripts/sync-codex-agents.test.js new file mode 100644 index 0000000..046e934 --- /dev/null +++ b/tests/plugins/praxis/hooks/scripts/sync-codex-agents.test.js @@ -0,0 +1,168 @@ +// Process contract for the Codex role installer. +// +// Spawned rather than imported, and not because spawning is tidier: hooks.json +// invokes this script as `require(root + '/hooks/scripts/sync-codex-agents.js')` +// and it does its work at require time, so the `require.main === module` guard +// that would make it importable would turn the hook into a no-op. +// +// Every plugin that ships a copy is covered from here, in one file rather than +// one per plugin: the copies must be byte-identical, which `make verify` asserts +// through generate-codex.py's divergence finder, so a second file would only +// duplicate this one. PLUGIN_ROOT and CODEX_HOME point at temporary directories +// throughout -- the script writes into the user's real Codex config otherwise. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const PLUGINS = path.resolve(__dirname, '../../../../../plugins'); +const RELATIVE = path.join('hooks', 'scripts', 'sync-codex-agents.js'); + +/** Every plugin shipping this hook, discovered so a new copy is covered too. */ +const COPIES = fs + .readdirSync(PLUGINS, { withFileTypes: true }) + .filter(entry => entry.isDirectory()) + .map(entry => ({ plugin: entry.name, script: path.join(PLUGINS, entry.name, RELATIVE) })) + .filter(copy => fs.existsSync(copy.script)); + +test('the hook is shipped by at least one plugin', () => { + // Discovery that matched nothing would leave every contract below vacuous. + assert.ok(COPIES.length > 0, `no sync-codex-agents.js found under ${PLUGINS}`); +}); + +/** Spawn one copy, asserting exit 0 and stdout empty or exactly one JSON object. */ +function spawnHook(script, { stdin = '', env = {} } = {}) { + const r = spawnSync(process.execPath, [script], { + input: stdin, + encoding: 'utf8', + env: { ...process.env, ...env } + }); + + assert.equal(r.status, 0, `exited ${r.status}, signal ${r.signal}; stderr: ${r.stderr}`); + const out = r.stdout.trim(); + if (out === '') return null; + + const payload = JSON.parse(out); + assert.equal(typeof payload, 'object'); + assert.notEqual(payload, null); + return payload; +} + +/** A fake plugin root holding `roles`, plus an empty Codex home. */ +function withDirs(roles, fn) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-codex-')); + const pluginRoot = path.join(dir, 'someplugin'); + const codexHome = path.join(dir, 'codex-home'); + fs.mkdirSync(codexHome); + + if (roles) { + const agents = path.join(pluginRoot, 'codex', 'agents'); + fs.mkdirSync(agents, { recursive: true }); + for (const [name, contents] of Object.entries(roles)) { + fs.writeFileSync(path.join(agents, name), contents); + } + } else { + fs.mkdirSync(pluginRoot); + } + + try { + return fn({ + env: { PLUGIN_ROOT: pluginRoot, CODEX_HOME: codexHome }, + codexHome, + installed: path.join(codexHome, 'agents', 'grimoire', 'someplugin') + }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +for (const { plugin, script } of COPIES) { + test(`${plugin}: contract with PLUGIN_ROOT unset -- a clean no-op`, () => { + // Claude Code sets only the prefixed name, so this is every non-Codex session. + const env = { PLUGIN_ROOT: undefined }; + const r = spawnSync(process.execPath, [script], { + input: '', + encoding: 'utf8', + env: Object.fromEntries(Object.entries({ ...process.env, ...env }).filter(([, v]) => v !== undefined)) + }); + assert.equal(r.status, 0, `exited ${r.status}; stderr: ${r.stderr}`); + assert.equal(r.stdout.trim(), ''); + }); + + test(`${plugin}: contract with malformed JSON on stdin`, () => { + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env }) => { + assert.equal(spawnHook(script, { stdin: '{"hook_event_name": ', env }), null); + }); + }); + + test(`${plugin}: contract with empty stdin`, () => { + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env }) => { + assert.equal(spawnHook(script, { stdin: '', env }), null); + }); + }); + + test(`${plugin}: contract with a payload carrying no transcript_path`, () => { + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env }) => { + assert.equal(spawnHook(script, { stdin: JSON.stringify({ hook_event_name: 'SessionStart' }), env }), null); + }); + }); + + test(`${plugin}: contract with no roles to install`, () => { + withDirs(null, ({ env }) => { + assert.equal(spawnHook(script, { env }), null); + }); + }); + + test(`${plugin}: installs the roles it ships under its own namespace`, () => { + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env, installed }) => { + spawnHook(script, { env }); + assert.equal(fs.readFileSync(path.join(installed, 'reviewer.toml'), 'utf8'), 'name = "reviewer"\n'); + }); + }); + + test(`${plugin}: removes a role it no longer ships and leaves other files alone`, () => { + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env, installed }) => { + fs.mkdirSync(installed, { recursive: true }); + fs.writeFileSync(path.join(installed, 'dropped.toml'), 'name = "dropped"\n'); + fs.writeFileSync(path.join(installed, 'notes.md'), 'not a role\n'); + + spawnHook(script, { env }); + + assert.equal(fs.existsSync(path.join(installed, 'dropped.toml')), false); + assert.equal(fs.existsSync(path.join(installed, 'notes.md')), true); + assert.equal(fs.existsSync(path.join(installed, 'reviewer.toml')), true); + }); + }); + + test(`${plugin}: leaves an unchanged role's mtime alone`, () => { + // Writing only on difference keeps mtimes stable across the many sessions + // that change nothing. + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env, installed }) => { + spawnHook(script, { env }); + const target = path.join(installed, 'reviewer.toml'); + const first = fs.statSync(target).mtimeMs; + + spawnHook(script, { env }); + assert.equal(fs.statSync(target).mtimeMs, first); + }); + }); + + test(`${plugin}: installs under the owned namespace even when PLUGIN_ROOT ends in ..`, () => { + // Resolving before taking the basename is what keeps the sweep inside a + // directory this plugin owns. Unresolved, the basename is ".." and the + // target collapses to the shared agents directory, putting roles this + // plugin does not own within reach of the sweep. + // + // Built by concatenation, not path.join: join normalizes the `..` away, + // which would make this test pass against the bug it exists to catch. + withDirs({ 'reviewer.toml': 'name = "reviewer"\n' }, ({ env, installed, codexHome }) => { + const trailing = { ...env, PLUGIN_ROOT: `${env.PLUGIN_ROOT}/codex/..` }; + assert.equal(spawnHook(script, { env: trailing }), null); + assert.equal(fs.existsSync(path.join(installed, 'reviewer.toml')), true); + assert.equal(fs.existsSync(path.join(codexHome, 'agents', 'reviewer.toml')), false); + }); + }); +} diff --git a/tests/plugins/praxis/skills/code-hygiene/find-duplicate-comments.test.js b/tests/plugins/praxis/skills/code-hygiene/find-duplicate-comments.test.js new file mode 100644 index 0000000..6d5b022 --- /dev/null +++ b/tests/plugins/praxis/skills/code-hygiene/find-duplicate-comments.test.js @@ -0,0 +1,332 @@ +// Tests for the duplicate-comment finder's parsing and bookkeeping. +// +// Four of the cases below are named `regression:` and each pins a defect the +// tool actually shipped, found by hand against a throwaway repository. Each is +// written to fail against the pre-fix behaviour, so removing the fix reddens the +// suite rather than only deleting a code comment. +// +// Fixtures are throwaway repositories under the OS temp directory, never this +// checkout: the tool runs against whatever repository is being reviewed, and a +// fixture that is this repository lets a rule which suppresses a finding here +// look correct. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const SOURCE = path.resolve( + __dirname, + '../../../../../plugins/praxis/skills/code-hygiene/find-duplicate-comments.js' +); +const { + parseArgs, + buildSkipMatcher, + prose, + trackedPaths, + buildCommentIndex, + findRetoldInDiff, + dedupePairs +} = require(SOURCE); + +// One comment body reused throughout, long enough to clear MIN_PROSE_CHARS, and +// the prose() output it normalizes to. +const COMMENT = 'The retold fact rule needs a whole repo view'; +const PROSE = 'the retold fact rule needs a whole repo view'; + +const GIT_ENV = { + GIT_AUTHOR_NAME: 'Grimoire Fixture', + GIT_AUTHOR_EMAIL: 'fixture@example.invalid', + GIT_COMMITTER_NAME: 'Grimoire Fixture', + GIT_COMMITTER_EMAIL: 'fixture@example.invalid' +}; + +function gitIn(cwd, args) { + const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: { ...process.env, ...GIT_ENV } }); + assert.equal(r.status, 0, `git ${args.join(' ')} failed: ${r.stderr}`); + return r.stdout; +} + +/** + * A throwaway repository holding `files` as one commit. Config is set rather + * than inherited: autocrlf on the developer's machine would rewrite the CRLF + * fixture's bytes, and a signing requirement would fail the commit. + */ +function makeRepo(files) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grimoire-dupes-')); + gitIn(dir, ['init', '-q']); + gitIn(dir, ['config', 'core.autocrlf', 'false']); + gitIn(dir, ['config', 'commit.gpgsign', 'false']); + + for (const [name, content] of Object.entries(files)) { + const full = path.join(dir, name); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); + } + + gitIn(dir, ['add', '-A']); + gitIn(dir, ['commit', '-qm', 'fixture']); + return dir; +} + +/** Run `fn` with the fixture as cwd -- git() in the tool inherits it -- then delete it. */ +function inRepo(dir, fn) { + const before = process.cwd(); + process.chdir(dir); + try { + return fn(); + } finally { + process.chdir(before); + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +test('parseArgs defaults the base ref and collects no skips', () => { + assert.deepEqual(parseArgs([]), { base: 'origin/main', skips: [] }); +}); + +test('parseArgs takes a positional base ref', () => { + assert.deepEqual(parseArgs(['HEAD~3']), { base: 'HEAD~3', skips: [] }); +}); + +test('parseArgs collects both --skip spellings, in any position', () => { + assert.deepEqual( + parseArgs(['--skip', 'dist', 'upstream/main', '--skip=build']), + { base: 'upstream/main', skips: ['dist', 'build'] } + ); +}); + +// die() exits the process, so the rejection paths are exercised as a process. +// Both die inside parseArgs, before the script touches git. +test('parseArgs rejects an unknown option with exit 2', () => { + const r = spawnSync(process.execPath, [SOURCE, '--nope'], { encoding: 'utf8' }); + assert.equal(r.status, 2); + assert.match(r.stderr, /unknown option '--nope'/); +}); + +test('parseArgs rejects a valueless --skip with exit 2', () => { + const r = spawnSync(process.execPath, [SOURCE, '--skip'], { encoding: 'utf8' }); + assert.equal(r.status, 2); + assert.match(r.stderr, /--skip needs a pattern/); +}); + +test('buildSkipMatcher always skips node_modules, at any depth', () => { + const matcher = buildSkipMatcher([]); + assert.ok(matcher.test('node_modules/dep/index.js')); + assert.ok(matcher.test('web/node_modules/dep/index.js')); + assert.ok(matcher.test('node_modules')); +}); + +test('buildSkipMatcher matches whole segments only', () => { + const matcher = buildSkipMatcher(['dist']); + assert.ok(matcher.test('dist/bundle.js')); + assert.ok(matcher.test('web/dist/bundle.js')); + assert.ok(!matcher.test('redistribute.js')); + assert.ok(!matcher.test('src/distance.js')); +}); + +test('regression: a --skip value is matched literally, not compiled as a pattern', () => { + // Taken as a regex, `.*` compiles happily, matches every path, and reports a + // clean tree -- the silent false-clean the tool exists to avoid producing. + const matcher = buildSkipMatcher(['.*']); + assert.ok(!matcher.test('src/index.js')); + assert.ok(!matcher.test('README.md')); + // It still skips a directory actually named `.*`. + assert.ok(matcher.test('src/.*/generated.js')); +}); + +test('prose reads every comment opener the tool covers', () => { + assert.equal(prose(`// ${COMMENT}`), PROSE); + assert.equal(prose(` # ${COMMENT}`), PROSE); + assert.equal(prose(` * ${COMMENT}`), PROSE); + assert.equal(prose(`/* ${COMMENT} */`), PROSE); +}); + +test('prose returns nothing for a line carrying no comment', () => { + assert.equal(prose('const answer = 42;'), ''); + assert.equal(prose(''), ''); +}); + +test('prose compares on words alone, so punctuation and case do not hide a twin', () => { + assert.equal(prose(`// ${COMMENT.toUpperCase()}!!!`), PROSE); + assert.equal(prose(`// The retold, fact "rule" -- needs a whole repo view.`), PROSE); +}); + +test('prose drops a comment whose prose is shorter than the floor', () => { + assert.equal(prose('// abcdefghij abcdefghij abc'), 'abcdefghij abcdefghij abc'); + assert.equal(prose('// abcdefghij abcdefghij ab'), ''); +}); + +test('prose exempts mandated and machine-read text', () => { + assert.equal(prose('// SPDX-License-Identifier: MIT, and enough words to clear the floor'), ''); + assert.equal(prose('# noqa: E501 -- and enough further words to clear the length floor'), ''); + assert.equal(prose('// eslint-disable-next-line no-console, plus words to clear the floor'), ''); +}); + +test('regression: a CRLF line still reads as a comment', () => { + // A CRLF-committed blob leaves a trailing \r that the `(.*)$` patterns reject, + // because \r is a line terminator to `.` -- silently hiding every // # and * + // comment in the file. + assert.equal(prose(`// ${COMMENT}\r`), PROSE); + assert.equal(prose(`# ${COMMENT}\r`), PROSE); + assert.equal(prose(` * ${COMMENT}\r`), PROSE); +}); + +test('trackedPaths lists tracked files minus the skipped trees', () => { + const dir = makeRepo({ + 'a.js': 'const x = 1;\n', + 'node_modules/dep/index.js': 'const y = 2;\n', + 'dist/bundle.js': 'const z = 3;\n', + 'redistribute.js': 'const w = 4;\n' + }); + + inRepo(dir, () => { + assert.deepEqual(trackedPaths(buildSkipMatcher(['dist'])).sort(), ['a.js', 'redistribute.js']); + }); +}); + +test('regression: trackedPaths drops a submodule gitlink', () => { + // A gitlink is a mode-160000 index entry with no blob behind it. + // `update-index --cacheinfo` creates one without a submodule checkout. + const dir = makeRepo({ 'a.js': `// ${COMMENT}\n` }); + gitIn(dir, ['update-index', '--add', '--cacheinfo', `160000,${'0'.repeat(39)}1,vendor/sub`]); + gitIn(dir, ['commit', '-qm', 'gitlink']); + + inRepo(dir, () => { + const paths = trackedPaths(buildSkipMatcher([])); + assert.deepEqual(paths, ['a.js']); + + // Why the entry must never reach the batch: what `cat-file --batch` answers + // for a gitlink is git-version dependent -- ` submodule`, which carries + // no size field and ends the whole batch, on the versions that emit it. The + // filter is the contract; the downstream reply is not ours to pin. + const index = buildCommentIndex(paths); + assert.ok(index instanceof Map); + assert.deepEqual(index.get(PROSE), ['a.js:1']); + }); +}); + +test('buildCommentIndex maps prose to every site carrying it, 1-based', () => { + const dir = makeRepo({ + 'a.js': `const x = 1;\n// ${COMMENT}\n`, + 'docs/b.md': `# heading\n# ${COMMENT}\n` + }); + + inRepo(dir, () => { + const index = buildCommentIndex(['a.js', 'docs/b.md']); + assert.deepEqual(index.get(PROSE), ['a.js:2', 'docs/b.md:2']); + // `# heading` normalizes below the floor, so it is not indexed at all. + assert.equal(index.get('heading'), undefined); + }); +}); + +test('regression: a CRLF-committed blob has its comments indexed', () => { + const dir = makeRepo({ 'crlf.js': `const x = 1;\r\n// ${COMMENT}\r\n` }); + + inRepo(dir, () => { + assert.deepEqual(buildCommentIndex(['crlf.js']).get(PROSE), ['crlf.js:2']); + }); +}); + +test('buildCommentIndex skips a file that is missing at HEAD', () => { + const dir = makeRepo({ 'a.js': `// ${COMMENT}\n` }); + fs.writeFileSync(path.join(dir, 'new.js'), `// ${COMMENT}\n`); + + inRepo(dir, () => { + // Newly added, so `HEAD:new.js` does not resolve -- normal, and nothing to + // index either way. It must not take the batch down with it. + const index = buildCommentIndex(['a.js', 'new.js']); + assert.deepEqual(index.get(PROSE), ['a.js:1']); + }); +}); + +test('buildCommentIndex short-circuits an empty file list', () => { + // No git call at all, so this needs no fixture. + assert.equal(buildCommentIndex([]).size, 0); +}); + +test('findRetoldInDiff numbers added lines against the new file', () => { + const index = new Map([[PROSE, ['other/file.js:7']]]); + const diff = [ + '@@ -1,2 +1,3 @@', + ' const first = 1;', + ' const second = 2;', + `+// ${COMMENT}`, + '' + ].join('\n'); + + assert.deepEqual(findRetoldInDiff(diff, 'sample.js', index), [ + { here: 'sample.js:3', text: PROSE, elsewhere: ['other/file.js:7'] } + ]); +}); + +test('findRetoldInDiff ignores a removed line and reports nothing told only here', () => { + const index = new Map([[PROSE, ['sample.js:1']]]); + const diff = [ + '@@ -1,2 +1,2 @@', + `-// ${COMMENT}`, + `+// ${COMMENT}`, + '' + ].join('\n'); + + // The one recorded site IS this line, so there is nothing told elsewhere. + assert.deepEqual(findRetoldInDiff(diff, 'sample.js', index), []); +}); + +test('regression: the no-newline marker does not advance the line counter', () => { + // `\ No newline at end of file` annotates the previous line rather than being + // one, so counting it shifts every later line number by one -- a finding that + // points a reviewer at the wrong line. + const index = new Map([[PROSE, ['other/file.js:7']]]); + const diff = [ + '@@ -1,3 +1,3 @@', + ' const shared = 0;', + '-const removed = 1;', + '\\ No newline at end of file', + '+const added = 1;', + `+// ${COMMENT}`, + '' + ].join('\n'); + + const findings = findRetoldInDiff(diff, 'sample.js', index); + assert.equal(findings.length, 1); + assert.equal(findings[0].here, 'sample.js:3'); +}); + +test('findRetoldInDiff restarts the counter at each hunk header', () => { + const index = new Map([[PROSE, ['other/file.js:7']]]); + const diff = [ + '@@ -1,1 +1,2 @@', + ' const first = 1;', + `+// ${COMMENT}`, + '@@ -40,1 +41,2 @@', + ' const later = 2;', + `+// ${COMMENT}`, + '' + ].join('\n'); + + assert.deepEqual( + findRetoldInDiff(diff, 'sample.js', index).map(f => f.here), + ['sample.js:2', 'sample.js:42'] + ); +}); + +test('dedupePairs collapses the mirror image of a pair added on both sides', () => { + const findings = [ + { here: 'a.js:1', text: PROSE, elsewhere: ['b.js:2'] }, + { here: 'b.js:2', text: PROSE, elsewhere: ['a.js:1'] } + ]; + + assert.deepEqual(dedupePairs(findings), [findings[0]]); +}); + +test('dedupePairs keeps distinct pairs', () => { + const findings = [ + { here: 'a.js:1', text: PROSE, elsewhere: ['b.js:2'] }, + { here: 'c.js:3', text: 'a different retelling of some other fact entirely', elsewhere: ['d.js:4'] } + ]; + + assert.deepEqual(dedupePairs(findings), findings); +}); diff --git a/tests/scripts/test_generate_codex.py b/tests/scripts/test_generate_codex.py new file mode 100644 index 0000000..6aba1bb --- /dev/null +++ b/tests/scripts/test_generate_codex.py @@ -0,0 +1,390 @@ +"""Tests for the Codex generator's parsing, rendering and drift finders. + +Everything here is a function that takes its input as an argument and whose wrong +answer would be silent: a frontmatter field dropped, a TOML string corrupted by +an unescaped backslash, a stale role file left installed in every Codex user's +config. The thin filesystem walk in main() is not covered -- see CONTRIBUTING.md. + +Fixtures are temporary directories with `ROOT` patched to point at them, never +this checkout: the drift finders glob the whole tree, so running them here would +assert against whatever the repository happens to hold today. +""" + +import importlib.util +import json +import pathlib +import tempfile +import unittest +from unittest import mock + +# The hyphen in the filename is not valid in a module name, so the module cannot +# be imported by name. +SOURCE = pathlib.Path(__file__).resolve().parents[2] / "scripts" / "generate-codex.py" +_spec = importlib.util.spec_from_file_location("generate_codex", SOURCE) +gc = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(gc) + + +class DisplayNameTests(unittest.TestCase): + def test_capitalises_the_first_character_only(self): + self.assertEqual(gc.display_name("praxis"), "Praxis") + self.assertEqual(gc.display_name("mcpTools"), "McpTools") + + def test_leaves_an_already_capitalised_name_alone(self): + self.assertEqual(gc.display_name("Grimoire"), "Grimoire") + + +class ShortDescriptionTests(unittest.TestCase): + def test_takes_the_lead_clause(self): + self.assertEqual( + gc.short_description("Development workflow -- issue planning, review"), + "Development workflow", + ) + + def test_passes_a_description_with_no_separator_through(self): + self.assertEqual(gc.short_description(" Development workflow "), "Development workflow") + + def test_splits_on_the_first_separator_only(self): + self.assertEqual(gc.short_description("A -- B -- C"), "A") + + +class QualifyTests(unittest.TestCase): + def test_namespaces_a_bare_skill_to_its_own_plugin(self): + self.assertEqual(gc.qualify("readable-code", "praxis"), "praxis:readable-code") + + def test_leaves_an_already_qualified_skill_alone(self): + self.assertEqual(gc.qualify("gitwise:github-conventions", "praxis"), "gitwise:github-conventions") + + +class ParseFrontmatterTests(unittest.TestCase): + def test_reads_scalars_a_list_and_the_body(self): + text = ( + "---\n" + "name: general-reviewer\n" + "description: Reviews code\n" + "skills:\n" + " - readable-code\n" + " - review-severity\n" + "---\n" + "\n" + "Body text.\n" + ) + fields, body = gc.parse_frontmatter(text, "general-reviewer.md") + self.assertEqual( + fields, + { + "name": "general-reviewer", + "description": "Reviews code", + "skills": ["readable-code", "review-severity"], + }, + ) + self.assertEqual(body, "Body text.") + + def test_keeps_a_colon_inside_a_scalar_value(self): + fields, _ = gc.parse_frontmatter("---\ndescription: Reviews code: carefully\n---\nx\n", "a.md") + self.assertEqual(fields["description"], "Reviews code: carefully") + + def test_a_valueless_key_becomes_an_empty_list(self): + fields, _ = gc.parse_frontmatter("---\nskills:\n---\nx\n", "a.md") + self.assertEqual(fields["skills"], []) + + def test_ignores_a_list_item_with_no_key_above_it(self): + fields, body = gc.parse_frontmatter("---\n - stray\nname: x\n---\nbody\n", "a.md") + self.assertEqual(fields, {"name": "x"}) + self.assertEqual(body, "body") + + def test_names_the_file_when_the_frontmatter_does_not_open(self): + with self.assertRaises(ValueError) as caught: + gc.parse_frontmatter("name: x\n---\nbody\n", "broken.md") + self.assertIn("broken.md", str(caught.exception)) + self.assertIn("open with ---", str(caught.exception)) + + def test_names_the_file_when_the_frontmatter_does_not_close(self): + with self.assertRaises(ValueError) as caught: + gc.parse_frontmatter("---\nname: x\nbody\n", "broken.md") + self.assertIn("broken.md", str(caught.exception)) + self.assertIn("no closing ---", str(caught.exception)) + + +class BasicStringTests(unittest.TestCase): + def test_quotes_a_plain_value(self): + self.assertEqual(gc.basic_string("Reviews code"), '"Reviews code"') + + def test_escapes_a_backslash(self): + # Basic strings process escapes, so an unescaped backslash silently + # corrupts the value rather than failing to parse. + self.assertEqual(gc.basic_string("a\\b"), '"a\\\\b"') + + def test_escapes_a_quote(self): + self.assertEqual(gc.basic_string('say "hi"'), '"say \\"hi\\""') + + def test_escapes_the_control_characters_with_a_shorthand(self): + self.assertEqual(gc.basic_string("a\nb"), '"a\\nb"') + self.assertEqual(gc.basic_string("a\rb"), '"a\\rb"') + self.assertEqual(gc.basic_string("a\tb"), '"a\\tb"') + self.assertEqual(gc.basic_string("a\x7fb"), '"a\\u007Fb"') + + def test_escapes_a_control_character_with_no_shorthand_as_a_codepoint(self): + self.assertEqual(gc.basic_string("a\x01b"), '"a\\u0001b"') + self.assertEqual(gc.basic_string("a\x1fb"), '"a\\u001Fb"') + + def test_escapes_backslashes_before_control_characters(self): + # A literal backslash followed by `n` must not come out as a newline + # escape, which is what escaping in the other order would produce. + self.assertEqual(gc.basic_string("\\n"), '"\\\\n"') + self.assertEqual(gc.basic_string("\\"), '"\\\\"') + # And a real newline next to a literal backslash keeps both. + self.assertEqual(gc.basic_string("\\\n"), '"\\\\\\n"') + + +class RenderAgentTomlTests(unittest.TestCase): + def render(self, fields, body="Do the review.", plugin="praxis"): + return gc.render_agent_toml(fields, body, plugin) + + def test_renders_name_description_and_body(self): + out = self.render({"name": "test-reviewer", "description": "Reviews tests"}) + self.assertEqual( + out, + 'name = "test-reviewer"\n' + 'description = "Reviews tests"\n' + "developer_instructions = '''\n" + "Do the review.\n" + "'''\n", + ) + + def test_names_the_preloaded_skills_in_the_instructions(self): + out = self.render( + { + "name": "test-reviewer", + "description": "Reviews tests", + "skills": ["testing-patterns", "gitwise:github-conventions"], + } + ) + self.assertIn( + "## Skills\n\nLoad these skills before starting: " + "`praxis:testing-patterns`, `gitwise:github-conventions`.", + out, + ) + + def test_omits_the_skills_section_when_there_are_none(self): + self.assertNotIn("## Skills", self.render({"name": "a", "description": "b"})) + self.assertNotIn("## Skills", self.render({"name": "a", "description": "b", "skills": []})) + + def test_maps_a_permission_mode_to_a_sandbox(self): + for mode, sandbox in ( + ("plan", "read-only"), + ("readOnly", "read-only"), + ("acceptEdits", "workspace-write"), + ): + with self.subTest(mode=mode): + out = self.render({"name": "a", "description": "b", "permissionMode": mode}) + self.assertIn(f'sandbox_mode = "{sandbox}"\n', out) + + def test_omits_the_sandbox_for_an_absent_or_unmapped_mode(self): + self.assertNotIn("sandbox_mode", self.render({"name": "a", "description": "b"})) + self.assertNotIn( + "sandbox_mode", + self.render({"name": "a", "description": "b", "permissionMode": "bypassPermissions"}), + ) + + def test_escapes_the_name_and_description_but_not_the_body(self): + out = self.render( + {"name": 'a"b', "description": "c\\d"}, + body="A path like C:\\Users stays verbatim.", + ) + self.assertIn('name = "a\\"b"\n', out) + self.assertIn('description = "c\\\\d"\n', out) + self.assertIn("A path like C:\\Users stays verbatim.", out) + + def test_rejects_a_body_that_would_end_the_literal_string(self): + with self.assertRaises(ValueError) as caught: + self.render({"name": "test-reviewer", "description": "b"}, body="A fence: '''") + self.assertIn("test-reviewer", str(caught.exception)) + self.assertIn("cannot be a TOML literal", str(caught.exception)) + + def test_rejects_a_triple_quote_introduced_by_the_skills_section(self): + # The check runs after the section is appended, so a `'''` arriving with + # the skill names is caught too. + with self.assertRaises(ValueError): + self.render({"name": "a", "description": "b", "skills": ["x'''y"]}) + + +class PluginManifestTests(unittest.TestCase): + def make_plugin(self, root, name, manifest): + source = root / "plugins" / name + (source / ".claude-plugin").mkdir(parents=True) + (source / ".claude-plugin" / "plugin.json").write_text(json.dumps(manifest)) + return source + + def test_builds_the_manifest_with_the_interface_block(self): + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) + self.make_plugin( + root, + "praxis", + {"name": "praxis", "version": "1.4.0", "description": "Workflow -- planning and review"}, + ) + entry = {"name": "praxis", "source": "./plugins/praxis", "category": "development", "keywords": ["review"]} + + with mock.patch.object(gc, "ROOT", root): + path, manifest = gc.plugin_manifest(entry) + + self.assertEqual(path, root / "plugins" / "praxis" / ".codex-plugin" / "plugin.json") + self.assertEqual( + manifest, + { + "name": "praxis", + "version": "1.4.0", + "description": "Workflow -- planning and review", + "keywords": ["review"], + "interface": { + "displayName": "Praxis", + "shortDescription": "Workflow", + "category": "Development", + }, + }, + ) + + def test_omits_keywords_when_the_entry_carries_none(self): + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) + self.make_plugin(root, "praxis", {"name": "praxis", "version": "1.0.0", "description": "d"}) + entry = {"name": "praxis", "source": "./plugins/praxis", "category": "development"} + + with mock.patch.object(gc, "ROOT", root): + _, manifest = gc.plugin_manifest(entry) + + self.assertNotIn("keywords", manifest) + + def test_raises_when_the_manifest_and_the_catalog_name_the_plugin_differently(self): + # A one-sided rename would otherwise leave the generated pair quietly + # disagreeing, which nothing downstream would notice. + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) + self.make_plugin(root, "praxis", {"name": "praxis-renamed", "version": "1.0.0", "description": "d"}) + entry = {"name": "praxis", "source": "./plugins/praxis", "category": "development"} + + with mock.patch.object(gc, "ROOT", root): + with self.assertRaises(ValueError) as caught: + gc.plugin_manifest(entry) + + message = str(caught.exception) + self.assertIn("praxis-renamed", message) + self.assertIn("praxis", message) + + def test_raises_on_a_category_less_entry(self): + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) + self.make_plugin(root, "praxis", {"name": "praxis", "version": "1.0.0", "description": "d"}) + entry = {"name": "praxis", "source": "./plugins/praxis"} + + with mock.patch.object(gc, "ROOT", root): + with self.assertRaises(KeyError): + gc.plugin_manifest(entry) + + +class FindOrphansTests(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.root = pathlib.Path(self.tmp.name) + self.codex_marketplace = self.root / ".agents" / "plugins" / "marketplace.json" + self.codex_marketplace.parent.mkdir(parents=True) + self.codex_marketplace.write_text("{}\n") + + def write(self, relative, contents="x\n"): + path = self.root / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(contents) + return path + + def find(self, targets): + with mock.patch.object(gc, "ROOT", self.root), mock.patch.object( + gc, "CODEX_MARKETPLACE", self.codex_marketplace + ): + return gc.find_orphans(targets) + + def test_reports_nothing_when_every_file_is_a_target(self): + kept = self.write("plugins/praxis/codex/agents/kept.toml") + manifest = self.write("plugins/praxis/.codex-plugin/plugin.json", "{}\n") + targets = [(self.codex_marketplace, "{}\n"), (kept, "x\n"), (manifest, "{}\n")] + self.assertEqual(self.find(targets), []) + + def test_reports_a_role_left_behind_by_a_deleted_agent(self): + kept = self.write("plugins/praxis/codex/agents/kept.toml") + self.write("plugins/praxis/codex/agents/deleted.toml") + self.assertEqual( + self.find([(self.codex_marketplace, "{}\n"), (kept, "x\n")]), + [pathlib.Path("plugins/praxis/codex/agents/deleted.toml")], + ) + + def test_reports_a_stale_manifest_alongside_a_stale_role(self): + self.write("plugins/praxis/codex/agents/deleted.toml") + self.write("plugins/praxis/.codex-plugin/plugin.json", "{}\n") + self.assertEqual( + self.find([(self.codex_marketplace, "{}\n")]), + [ + pathlib.Path("plugins/praxis/.codex-plugin/plugin.json"), + pathlib.Path("plugins/praxis/codex/agents/deleted.toml"), + ], + ) + + def test_reports_files_of_a_plugin_that_contributes_no_targets_at_all(self): + # The reason the directories to scan come from disk and not from the + # targets: a plugin that dropped to zero agents, or left the marketplace, + # would otherwise take its whole directory out of the scan along with the + # files left in it. + self.write("plugins/gone/codex/agents/old.toml") + self.assertEqual( + self.find([(self.codex_marketplace, "{}\n")]), + [pathlib.Path("plugins/gone/codex/agents/old.toml")], + ) + + def test_ignores_a_subdirectory_inside_a_scanned_directory(self): + (self.root / "plugins" / "praxis" / "codex" / "agents" / "nested").mkdir(parents=True) + self.assertEqual(self.find([(self.codex_marketplace, "{}\n")]), []) + + +class FindDivergentCopiesTests(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.root = pathlib.Path(self.tmp.name) + + def write_copy(self, plugin, contents): + path = self.root / "plugins" / plugin / gc.DUPLICATED_HOOK_SCRIPTS[0] + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(contents) + + def find(self): + with mock.patch.object(gc, "ROOT", self.root): + return gc.find_divergent_copies() + + def test_reports_nothing_when_the_copies_are_byte_identical(self): + self.write_copy("praxis", "console.error('sync');\n") + self.write_copy("recursio", "console.error('sync');\n") + self.assertEqual(self.find(), []) + + def test_reports_every_copy_once_they_differ(self): + # Plugins install independently, so they cannot share a module at hook + # runtime and nothing else stops a one-sided edit drifting. + self.write_copy("praxis", "console.error('sync');\n") + self.write_copy("recursio", "console.error('sync'); // edited here only\n") + self.assertEqual( + self.find(), + [ + pathlib.Path("plugins/praxis") / gc.DUPLICATED_HOOK_SCRIPTS[0], + pathlib.Path("plugins/recursio") / gc.DUPLICATED_HOOK_SCRIPTS[0], + ], + ) + + def test_reports_nothing_when_only_one_plugin_ships_the_script(self): + self.write_copy("praxis", "console.error('sync');\n") + self.assertEqual(self.find(), []) + + def test_reports_nothing_when_no_plugin_ships_it(self): + self.assertEqual(self.find(), []) + + +if __name__ == "__main__": + unittest.main()