diff --git a/manifests/context-packs/skill-triggers@1.json b/manifests/context-packs/skill-triggers@1.json index 62c4f9adc..2b90f2703 100644 --- a/manifests/context-packs/skill-triggers@1.json +++ b/manifests/context-packs/skill-triggers@1.json @@ -10,7 +10,7 @@ "id": "hand-seeded", "source": "manual-curation-pending-regeneration" }, - "registryDigest": "72cdbf4ed32b9b08a536099ba7c621208b9b9b5aaae2210301d11224ee442a76", + "registryDigest": "18d816c6f7d8b3639edfe7b086210d12634d79b6a27b2b4ff0973f47ab22752c", "schemaVersion": 1, "triggers": { "skill:agent-runtime-gateway-smoke-test": [ diff --git a/pi/core/skills/agent-self-evaluation/scripts/evaluate.py b/pi/core/skills/agent-self-evaluation/scripts/evaluate.py index 2d129c407..afedd37a9 100755 --- a/pi/core/skills/agent-self-evaluation/scripts/evaluate.py +++ b/pi/core/skills/agent-self-evaluation/scripts/evaluate.py @@ -19,6 +19,7 @@ For production use, pair with an LLM judge for semantic understanding. """ import argparse +import errno import re import sys from dataclasses import dataclass, field @@ -368,7 +369,9 @@ def _read_file_or_text(path: Optional[str], *, required: bool = False) -> Option try: with open(path) as f: return f.read() - except FileNotFoundError: + except OSError as error: + if error.errno not in (errno.ENOENT, errno.ENAMETOOLONG): + raise if required: print(f"Error: output file '{path}' not found", file=sys.stderr) sys.exit(1) @@ -382,7 +385,10 @@ def _read_input(args: argparse.Namespace) -> tuple[Optional[str], str]: print("Paste agent output (Ctrl+D to finish):") return task, sys.stdin.read() if args.output: - return _read_file_or_text(args.task), _read_file_or_text(args.output, required=True) or "" + task = _read_file_or_text(args.task) + if task is None and not sys.stdin.isatty(): + task = sys.stdin.read() + return task, _read_file_or_text(args.output, required=True) or "" return _read_file_or_text(args.task), sys.stdin.read() diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 1d19e5c35..99589b299 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -579,13 +579,24 @@ function readSedExecute(script, i, budget) { return { end: j, parts: [], stdinExec: true }; } -// Text, label and filename operands run to end of line; all are data. +// Text and filename operands run to end of line; all are data. function readSedLineOperand(script, i, budget) { let end = i + 1; while (end < script.length && script[end] !== '\n') { budget.spend(); end++; } return { end, parts: [], stdinExec: false }; } +// Branch and label commands (b, t, T, :) take a label that ends at `;` or +// newline, verified against GNU sed 4.9: ':a;e cmd' and 'ba;e cmd' both +// parse the label up to `;` and still run the following `e`. Commands after +// the label therefore continue to be scanned, so return the label end and +// let the caller resume command parsing there. +function readSedLabel(script, i, budget) { + let end = i + 1; + while (end < script.length && script[end] !== ';' && script[end] !== '\n') { budget.spend(); end++; } + return { end, parts: [], stdinExec: false }; +} + function readSedCommand(script, i, budget) { const letter = script[i]; if (letter === 's') { @@ -602,7 +613,8 @@ function readSedCommand(script, i, budget) { return sub ? { end: sub.end, parts: [], stdinExec: false } : null; } if (letter === 'e') return readSedExecute(script, i, budget); - if ('acirwWb:tTqQ'.includes(letter)) return readSedLineOperand(script, i, budget); + if ('btT:'.includes(letter)) return readSedLabel(script, i, budget); + if ('acirwWqQ'.includes(letter)) return readSedLineOperand(script, i, budget); if ('dDgGhHlnNpPxFzv='.includes(letter)) return { end: i + 1, parts: [], stdinExec: false }; return null; // unmodeled command letter: keep the opaque default } diff --git a/scripts/lib/claude-scope-migration.js b/scripts/lib/claude-scope-migration.js index acb789f3b..c4e00bc18 100644 --- a/scripts/lib/claude-scope-migration.js +++ b/scripts/lib/claude-scope-migration.js @@ -1,5 +1,6 @@ 'use strict'; +const fs = require('fs'); const path = require('path'); const { @@ -45,6 +46,63 @@ function recoveryCommands(sourceScope, destinationScope) { return commands; } +function enableRecoveryCommands(destinationScope) { + return [ + `claude plugin enable ${CURRENT_PLUGIN_ID} --scope ${destinationScope}`, + ...recoveryCommands(null, destinationScope), + ]; +} + +function destinationSettingsPaths(paths, destinationScope) { + const projectSettingsDir = path.join(paths.projectRoot, '.claude'); + if (destinationScope === 'user') { + return [path.join(paths.configDir, 'settings.json')]; + } + if (destinationScope === 'project') { + return [path.join(projectSettingsDir, 'settings.json')]; + } + return [ + path.join(projectSettingsDir, 'settings.local.json'), + path.join(projectSettingsDir, 'settings.json'), + ]; +} + +function readExplicitlyDisabledSettings(settingsPath) { + try { + const settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); + return settings?.enabledPlugins?.[CURRENT_PLUGIN_ID] === false; + } catch { + // Missing or unreadable settings are not an explicit user choice. + return false; + } +} + +function destinationExplicitlyDisabledError(paths, plugins, destinationScope) { + const installed = currentEccPlugins(plugins); + const destination = installed.find(plugin => plugin.scope === destinationScope); + if (!destination || destination.enabled === true) { + return null; + } + const disabledSettingsPaths = destinationSettingsPaths(paths, destinationScope) + .filter(readExplicitlyDisabledSettings); + if (disabledSettingsPaths.length === 0) { + return null; + } + return migrationError( + 'DESTINATION_EXPLICITLY_DISABLED', + `${CURRENT_PLUGIN_ID} is installed at ${destinationScope} scope but explicitly ` + + `disabled by enabledPlugins["${CURRENT_PLUGIN_ID}"]: false in ` + + `${disabledSettingsPaths.join(', ')}. ECC keeps that choice and will not ` + + `rewrite the setting; enable the plugin to continue the migration.`, + { + phase: 'destination-verification', + observedScopes: installed.map(plugin => plugin.scope), + settingsPaths: disabledSettingsPaths, + recovery: enableRecoveryCommands(destinationScope), + } + ); +} + function readPluginInventory(run, projectRoot, phase) { return parsePluginList( run( @@ -95,7 +153,7 @@ function assertMigrationInventory(plugins, destinationScope) { { phase: 'destination-verification', observedScopes, - recovery: recoveryCommands(null, destinationScope), + recovery: enableRecoveryCommands(destinationScope), } ); } @@ -171,8 +229,10 @@ function plannedActions(migration, destinationScope, marketplaceAction) { function verifySourceAndDestination(run, paths, migration, destinationScope, phase) { const expectedScopes = [migration.sourceScope, destinationScope]; + const plugins = readPluginInventory(run, paths.projectRoot, phase); + const destination = currentEccPlugins(plugins).find(plugin => plugin.scope === destinationScope); return validateExpectedScopes( - readPluginInventory(run, paths.projectRoot, phase), + plugins, expectedScopes, { code: phase === 'concurrency-check' @@ -183,7 +243,9 @@ function verifySourceAndDestination(run, paths, migration, destinationScope, pha ? 'Claude plugin scopes changed during migration; the source was not removed.' : `Could not verify ${CURRENT_PLUGIN_ID} at the destination before source cleanup.`, phase, - recovery: recoveryCommands(null, destinationScope), + recovery: destination && destination.enabled !== true + ? enableRecoveryCommands(destinationScope) + : recoveryCommands(null, destinationScope), } ); } @@ -208,23 +270,32 @@ function uninstallSource(run, paths, migration, destinationScope) { assertNoConflictingEccPlugins(plugins); const installed = currentEccPlugins(plugins); observedScopes = installed.map(plugin => plugin.scope); + const destination = installed.find( + plugin => plugin.scope === destinationScope + ); if ( - installed.length === 1 - && installed[0].scope === destinationScope - && installed[0].enabled === true + destination + && destination.enabled !== false + && !installed.some(plugin => plugin.scope === migration.sourceScope) ) { return ['Claude reported an uninstall error, but destination-only state was verified.']; } } catch { // Preserve the safest known two-scope state in the structured recovery. } + const sourcePresent = observedScopes.includes(migration.sourceScope); throw migrationError( 'SOURCE_UNINSTALL_FAILED', `The destination is installed, but Claude could not remove the ${migration.sourceScope} source scope.`, { phase: 'source-uninstall', observedScopes, - recovery: recoveryCommands(migration.sourceScope, destinationScope), + recovery: [ + 'claude plugin list --json', + ...(observedScopes.length === 0 + ? [`ecc setup --mode claude-plugin --scope ${destinationScope} --yes`] + : recoveryCommands(sourcePresent ? migration.sourceScope : null, destinationScope)), + ], } ); } @@ -265,6 +336,14 @@ function migrateClaudePluginScope(options = {}, dependencies = {}) { ? createDryRunClaudeRunner(providerRun, paths, options) : providerRun; const plugins = readPluginInventory(run, paths.projectRoot, 'inventory'); + const disabledDestinationError = destinationExplicitlyDisabledError( + paths, + plugins, + options.scope + ); + if (disabledDestinationError) { + throw disabledDestinationError; + } const migration = assertMigrationInventory(plugins, options.scope); const hooks = options.hooks === undefined ? deriveHookMode(settings) diff --git a/scripts/lib/context-profiles.js b/scripts/lib/context-profiles.js index de80d1142..31b0a2266 100644 --- a/scripts/lib/context-profiles.js +++ b/scripts/lib/context-profiles.js @@ -86,13 +86,28 @@ function estimateMetadata(entries, target, profile) { }; } -function compileContextProfile({ +function validatePlanOptions({ selectionMode = 'manual', target = 'codex' }) { + validateTarget(target); + if (!MODES.includes(selectionMode)) throw new Error(`Unknown selection mode: ${selectionMode}`); +} + +function compileContextProfile(options = {}) { + return compileContextProfileWithRegistry(options).plan; +} + +// Keep registry reuse inside this call to the canonical loader. Callers can +// consume the snapshot, but cannot supply a cached or caller-built registry. +function compileContextProfileWithRegistry({ repoRoot = DEFAULT_REPO_ROOT, ...options } = {}) { + validatePlanOptions(options); + const registry = loadContextRegistry({ repoRoot }); + return { registry, plan: compileContextPlan(registry, { repoRoot, ...options }) }; +} + +function compileContextPlan(registry, { repoRoot = DEFAULT_REPO_ROOT, profileId = 'lean@1', selectionMode = 'manual', target = 'codex', include = [], exclude = [], } = {}) { - validateTarget(target); - if (!MODES.includes(selectionMode)) throw new Error(`Unknown selection mode: ${selectionMode}`); - const registry = loadContextRegistry({ repoRoot }); + validatePlanOptions({ selectionMode, target }); const profile = loadContextProfile(profileId, { repoRoot }); if (profile.registryId !== registry.id) throw new Error('Profile registry ID mismatch'); const selected = resolveSelection(registry, profile, include, exclude); @@ -130,4 +145,4 @@ function compileContextProfile({ return plan; } -module.exports = { compileContextProfile, explainContextEntry, loadContextProfile }; +module.exports = { compileContextProfile, compileContextProfileWithRegistry, explainContextEntry, loadContextProfile }; diff --git a/scripts/lib/context-selection.js b/scripts/lib/context-selection.js index 5a642d455..f732543ad 100644 --- a/scripts/lib/context-selection.js +++ b/scripts/lib/context-selection.js @@ -1,8 +1,8 @@ 'use strict'; const yaml = require('js-yaml'); -const { loadContextRegistry, loadSkillTriggers } = require('./context-pack-registry'); -const { compileContextProfile } = require('./context-profiles'); +const { loadSkillTriggers } = require('./context-pack-registry'); +const { compileContextProfileWithRegistry } = require('./context-profiles'); const { buildRetrievalIndex, searchRetrieval } = require('./context-retrieval'); const { DEFAULT_REPO_ROOT, createSourceReader, digestObject } = require('./context-profile-support'); @@ -310,10 +310,9 @@ function admissibility(byId, excluded, reader) { * without explicit selection. Metadata only; bodies are never returned. */ function routingEntries({ repoRoot = DEFAULT_REPO_ROOT, profileId = 'lean@1', target = 'codex', selectionMode = 'auto', include = [], exclude = [] } = {}) { - const plan = compileContextProfile({ repoRoot, profileId, target, selectionMode, include, exclude }); - const registry = loadContextRegistry({ repoRoot }); + // One registry load serves the plan and the routing entries. + const { registry, plan } = compileContextProfileWithRegistry({ repoRoot, profileId, target, selectionMode, include, exclude }); const { triggers } = loadSkillTriggers({ repoRoot }); - if (registry.registryDigest !== plan.registryDigest) throw new Error('Registry changed during routing index build'); const byId = new Map(registry.entries.map(entry => [entry.id, entry])); const excluded = new Set(plan.excludedIds); const admissible = admissibility(byId, excluded, createSourceReader(repoRoot)); @@ -335,10 +334,10 @@ function resolveTaskContext({ repoRoot = DEFAULT_REPO_ROOT, task, profileId = 'l selectionMode = 'auto', include = [], exclude = [], load = false, previous = null, expectedDigest = null } = {}) { validateTask(task); validatePrevious(previous); - const plan = compileContextProfile({ repoRoot, profileId, target, selectionMode, include, exclude }); - const registry = loadContextRegistry({ repoRoot }); + // One registry load serves the plan and the selection; skill bytes are + // still verified against it when they are read. + const { registry, plan } = compileContextProfileWithRegistry({ repoRoot, profileId, target, selectionMode, include, exclude }); const { triggers } = loadSkillTriggers({ repoRoot }); - if (registry.registryDigest !== plan.registryDigest) throw new Error('Registry changed during task selection'); const reader = createSourceReader(repoRoot); const byId = new Map(registry.entries.map(entry => [entry.id, entry])); const excluded = new Set(plan.excludedIds); diff --git a/scripts/lib/skill-evolution/tracker.js b/scripts/lib/skill-evolution/tracker.js index effa107eb..2964028d1 100644 --- a/scripts/lib/skill-evolution/tracker.js +++ b/scripts/lib/skill-evolution/tracker.js @@ -53,6 +53,7 @@ function normalizeExecutionRecord(input, options = {}) { const outcome = input.outcome; const recordedAt = input.recorded_at || options.now || new Date().toISOString(); const userFeedback = input.user_feedback || input.userFeedback || null; + const failureReasons = [input.failure_reason, input.failureReason]; if (typeof skillId !== 'string' || skillId.trim().length === 0) { throw new Error('skill_id is required'); @@ -74,6 +75,10 @@ function normalizeExecutionRecord(input, options = {}) { throw new Error('user_feedback must be accepted, corrected, rejected, or null'); } + if (failureReasons.some(value => value != null && typeof value !== 'string')) { + throw new Error('failure_reason must be a string or null'); + } + if (Number.isNaN(Date.parse(recordedAt))) { throw new Error('recorded_at must be an ISO timestamp'); } diff --git a/skills/agent-self-evaluation/scripts/evaluate.py b/skills/agent-self-evaluation/scripts/evaluate.py index 2d129c407..afedd37a9 100755 --- a/skills/agent-self-evaluation/scripts/evaluate.py +++ b/skills/agent-self-evaluation/scripts/evaluate.py @@ -19,6 +19,7 @@ For production use, pair with an LLM judge for semantic understanding. """ import argparse +import errno import re import sys from dataclasses import dataclass, field @@ -368,7 +369,9 @@ def _read_file_or_text(path: Optional[str], *, required: bool = False) -> Option try: with open(path) as f: return f.read() - except FileNotFoundError: + except OSError as error: + if error.errno not in (errno.ENOENT, errno.ENAMETOOLONG): + raise if required: print(f"Error: output file '{path}' not found", file=sys.stderr) sys.exit(1) @@ -382,7 +385,10 @@ def _read_input(args: argparse.Namespace) -> tuple[Optional[str], str]: print("Paste agent output (Ctrl+D to finish):") return task, sys.stdin.read() if args.output: - return _read_file_or_text(args.task), _read_file_or_text(args.output, required=True) or "" + task = _read_file_or_text(args.task) + if task is None and not sys.stdin.isatty(): + task = sys.stdin.read() + return task, _read_file_or_text(args.output, required=True) or "" return _read_file_or_text(args.task), sys.stdin.read() diff --git a/skills/skill-comply/scripts/runner.py b/skills/skill-comply/scripts/runner.py index ffad8447f..d638ad2e7 100644 --- a/skills/skill-comply/scripts/runner.py +++ b/skills/skill-comply/scripts/runner.py @@ -94,8 +94,12 @@ def _safe_sandbox_dir(scenario_id: str) -> Path: """Sanitize scenario ID and ensure path stays within sandbox base.""" safe_id = re.sub(r"[^a-zA-Z0-9\-_]", "_", scenario_id) path = SANDBOX_BASE / safe_id + resolved_path = path.resolve() + resolved_base = SANDBOX_BASE.resolve() + if not safe_id or resolved_path == resolved_base: + raise ValueError("Scenario sandbox must be a subdirectory of the sandbox base") # Validate path stays within sandbox base (raises ValueError on traversal) - path.resolve().relative_to(SANDBOX_BASE.resolve()) + resolved_path.relative_to(resolved_base) return path diff --git a/skills/skill-comply/tests/test_runner.py b/skills/skill-comply/tests/test_runner.py index f8141d184..448d990f4 100644 --- a/skills/skill-comply/tests/test_runner.py +++ b/skills/skill-comply/tests/test_runner.py @@ -9,7 +9,13 @@ from pathlib import Path from unittest.mock import patch import pytest -from scripts.runner import _parse_stream_json, _setup_sandbox, run_scenario +from scripts.runner import ( + SANDBOX_BASE, + _parse_stream_json, + _safe_sandbox_dir, + _setup_sandbox, + run_scenario, +) @dataclass(frozen=True) @@ -21,6 +27,37 @@ class _FakeScenario: setup_commands: tuple[str, ...] = () +class TestSafeSandboxDirectory: + def test_empty_id_cannot_delete_the_shared_base(self) -> None: + with ( + patch("scripts.runner.Path.exists", return_value=True), + patch("scripts.runner.shutil.rmtree") as remove, + patch("scripts.runner.Path.mkdir") as mkdir, + patch("scripts.runner.subprocess.run") as execute, + ): + execute.return_value = subprocess.CompletedProcess([], 0, stdout="", stderr="") + with pytest.raises(ValueError, match="sandbox"): + run_scenario(_FakeScenario(id="")) + remove.assert_not_called() + mkdir.assert_not_called() + execute.assert_not_called() + + def test_resolved_base_is_not_a_scenario_directory(self) -> None: + with patch("scripts.runner.Path.resolve", return_value=SANDBOX_BASE): + with pytest.raises(ValueError, match="sandbox"): + _safe_sandbox_dir("alias") + + def test_valid_id_keeps_existing_sanitization(self) -> None: + assert _safe_sandbox_dir("scenario/one") == SANDBOX_BASE / "scenario_one" + + def test_resolved_escape_is_still_rejected(self, tmp_path, monkeypatch) -> None: + base = tmp_path / "sandbox" + monkeypatch.setattr("scripts.runner.SANDBOX_BASE", base) + with patch.object(Path, "resolve", side_effect=[tmp_path / "outside", base]): + with pytest.raises(ValueError): + _safe_sandbox_dir("escape") + + class TestSetupSandboxSkipsShellBuiltins: """Setup commands containing shell builtins (cd/pushd/popd) must be skipped. diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index 56549e721..2cf25a1e1 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -2183,7 +2183,18 @@ const sedCases = [ ['unknown escape decoding bypass is conservatively blocked', "printf x | sed 's/x/\\git commit --no-verify/e'", 2], ['escaped-newline continued e operand is conservatively blocked', "printf x | sed -e 'e git commit \\\n--no-verify'", 2], ['octal escape decoding bypass is conservatively blocked', "printf x | sed 's/x/\\147it commit --no-verify/e'", 2], - ['backslash in a non-executing script stays data', "sed 's/a\\/b/c/' 'git commit --no-verify'", 0] + ['backslash in a non-executing script stays data', "sed 's/a\\/b/c/' 'git commit --no-verify'", 0], + // Labels end at `;` and following commands still execute (GNU sed 4.9 + // verified: ':a;e cmd' runs cmd). Reading label operands to end of line + // let an e command hide behind a label (CodeRabbit finding, landed via + // #3495). The label reader must stop at `;` so the rest is scanned. + ['e command after a label is scanned', "sed ':a;e git commit --no-verify'", 2], + ['e command after a b branch is scanned', "printf x | sed 'ba;e git commit --no-verify'", 2], + ['e command after a T branch is scanned', "printf x | sed 'TX;e git commit --no-verify'", 2], + ['e command after a newline label is scanned', "sed ':a\ne git commit --no-verify'", 2], + ['e command after a t branch is scanned', "printf x | sed 'tX;e git commit --no-verify'", 2], + ['plain label without following commands stays data', "sed ':a' 'git commit --no-verify'", 0], + ['b branch alone stays data', "sed 'bx' 'git commit --no-verify'", 0] ]; for (const [name, command, expected] of sedCases) { if (test(`sed model ${name}`, () => { diff --git a/tests/lib/claude-scope-migration.test.js b/tests/lib/claude-scope-migration.test.js index c3ab6606f..648b591b1 100644 --- a/tests/lib/claude-scope-migration.test.js +++ b/tests/lib/claude-scope-migration.test.js @@ -514,6 +514,7 @@ test('source uninstall failure reports both scopes and exact forward recovery', assert.strictEqual(error.phase, 'source-uninstall'); assert.deepStrictEqual([...error.observedScopes].sort(), ['project', 'user']); assert.deepStrictEqual(error.recovery, [ + 'claude plugin list --json', 'claude plugin uninstall ecc@ecc --scope user --keep-data', 'ecc setup --mode claude-plugin --scope project --move-scope --yes', ]); @@ -670,5 +671,170 @@ test('migration preserves an explicit includeCoAuthoredBy opt-in', () => { }); }); +function writeExplicitDisable(filePath, pluginId = 'ecc@ecc') { + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, `${JSON.stringify({ + enabledPlugins: { [pluginId]: false }, + }, null, 2)}\n`); +} + +test('an explicitly disabled resume destination names the setting and the enable recovery', () => { + withFixture({ + plugins: [plugin('user'), plugin('local', { enabled: false })], + marketplaces: [marketplace('user')], + }, fixture => { + const localSettings = path.join( + fixture.projectRoot, + '.claude', + 'settings.local.json' + ); + writeExplicitDisable(localSettings); + const error = captureError(() => ( + migrateClaudePluginScope(migrationOptions(fixture, 'local')) + )); + assert.strictEqual(error.code, 'DESTINATION_EXPLICITLY_DISABLED'); + assert.strictEqual(error.phase, 'destination-verification'); + assert.match(error.message, /enabledPlugins/); + assert.ok(error.message.includes(localSettings)); + assert.deepStrictEqual(error.observedScopes, ['user', 'local']); + assert.deepStrictEqual(error.recovery, [ + 'claude plugin enable ecc@ecc --scope local', + 'ecc setup --mode claude-plugin --scope local --move-scope --yes', + ]); + assert.deepStrictEqual(readCalls(fixture), [ + ['plugin', 'list', '--json'], + ]); + assert.ok(!readCalls(fixture).some(argv => argv[1] === 'uninstall')); + assert.deepStrictEqual( + readState(fixture).plugins.map(entry => entry.scope).sort(), + ['local', 'user'] + ); + assert.strictEqual(JSON.parse( + fs.readFileSync(localSettings, 'utf8') + ).enabledPlugins['ecc@ecc'], false); + }); +}); + +test('a single-scope explicitly disabled destination is not a generic verification failure', () => { + for (const { scope, settingsPath } of [ + { scope: 'user', settingsPath: 'config' }, + { scope: 'project', settingsPath: 'project-settings' }, + { scope: 'local', settingsPath: 'project-settings' }, + { scope: 'local', settingsPath: 'project-local-settings' }, + ]) { + withFixture({ plugins: [plugin(scope, { enabled: false })] }, fixture => { + const explicitPaths = { + config: fixture.settingsPath, + 'project-settings': path.join( + fixture.projectRoot, '.claude', 'settings.json' + ), + 'project-local-settings': path.join( + fixture.projectRoot, '.claude', 'settings.local.json' + ), + }; + const disabledPath = explicitPaths[settingsPath]; + writeExplicitDisable(disabledPath); + const error = captureError(() => ( + migrateClaudePluginScope(migrationOptions(fixture, scope)) + )); + assert.strictEqual(error.code, 'DESTINATION_EXPLICITLY_DISABLED'); + assert.ok(error.message.includes(disabledPath)); + assert.deepStrictEqual(error.recovery, [ + `claude plugin enable ecc@ecc --scope ${scope}`, + `ecc setup --mode claude-plugin --scope ${scope} --move-scope --yes`, + ]); + }); + } +}); + +test('a disabled destination without an explicit setting stays generic and gains the enable path', () => { + withFixture({ plugins: [plugin('local', { enabled: false })] }, fixture => { + const error = captureError(() => ( + migrateClaudePluginScope(migrationOptions(fixture, 'local')) + )); + assert.strictEqual(error.code, 'DESTINATION_VERIFICATION_FAILED'); + assert.match(error.message, /exists at local scope but is not enabled/); + assert.deepStrictEqual(error.recovery, [ + 'claude plugin enable ecc@ecc --scope local', + 'ecc setup --mode claude-plugin --scope local --move-scope --yes', + ]); + }); +}); + +test('a source uninstall that errors but removes the source accepts the verified destination', () => { + withFixture({ + plugins: [plugin('user'), plugin('project')], + marketplaces: [marketplace('user')], + failures: [{ + argv: uninstallArgv('user'), + status: 9, + stderr: 'uninstall failed', + times: 1, + }], + pluginListResponses: [ + [plugin('user'), plugin('project')], + [plugin('user'), plugin('project')], + [plugin('user'), plugin('project')], + [plugin('project'), plugin('local')], + [plugin('project')], + ], + }, fixture => { + const result = migrateClaudePluginScope( + migrationOptions(fixture, 'project') + ); + assert.strictEqual(result.action, 'resumed'); + assert.strictEqual(result.sourceScope, 'user'); + assert.deepStrictEqual(result.warnings, [ + 'Claude reported an uninstall error, but destination-only state was verified.', + ]); + assert.ok(!readCalls(fixture).slice(-2).some(argv => argv[1] === 'uninstall')); + }); +}); + +test('source uninstall failure recovery stops suggesting an already-absent source', () => { + withFixture({ + plugins: [plugin('user'), plugin('project')], + marketplaces: [marketplace('user')], + failures: [{ + argv: uninstallArgv('user'), + status: 9, + stderr: 'uninstall failed', + times: 1, + }], + pluginListResponses: [ + [plugin('user'), plugin('project')], + [plugin('user'), plugin('project')], + [plugin('user'), plugin('project')], + [], + ], + }, fixture => { + const error = captureError(() => ( + migrateClaudePluginScope(migrationOptions(fixture, 'project')) + )); + assert.strictEqual(error.code, 'SOURCE_UNINSTALL_FAILED'); + assert.strictEqual(error.phase, 'source-uninstall'); + assert.deepStrictEqual(error.observedScopes, []); + assert.deepStrictEqual(error.recovery, [ + 'claude plugin list --json', + 'ecc setup --mode claude-plugin --scope project --yes', + ]); + }); +}); + +test('a two-scope disabled destination offers enable recovery without removing the source', () => { + withFixture({ + plugins: [plugin('user'), plugin('project', { enabled: false })], + marketplaces: [marketplace('user')], + }, fixture => { + const error = captureError(() => migrateClaudePluginScope(migrationOptions(fixture, 'project'))); + assert.strictEqual(error.code, 'DESTINATION_VERIFICATION_FAILED'); + assert.deepStrictEqual(error.recovery, [ + 'claude plugin enable ecc@ecc --scope project', + 'ecc setup --mode claude-plugin --scope project --move-scope --yes', + ]); + assert.ok(!readCalls(fixture).some(argv => argv[1] === 'uninstall')); + }); +}); + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); diff --git a/tests/lib/context-selection.test.js b/tests/lib/context-selection.test.js index c04f943d0..53755c62e 100644 --- a/tests/lib/context-selection.test.js +++ b/tests/lib/context-selection.test.js @@ -5,7 +5,7 @@ const fs = require('node:fs'); const path = require('node:path'); const test = require('node:test'); const { withFixture, write, update } = require('./helpers/context-fixture'); -const { resolveTaskContext, resolveDeclinedFallback } = require('../../scripts/lib/context-selection'); +const { resolveTaskContext, resolveDeclinedFallback, routingEntries } = require('../../scripts/lib/context-selection'); const task = (values = {}) => ({ sessionId: 'session-1', taskId: 'task-1', revision: 1, phase: 'implement', query: '', explicitIds: [], proposedIds: [], ...values }); @@ -71,9 +71,9 @@ for (const load of [false, true]) { let alteredReads = 0; context.mock.method(fs, 'openSync', (filename, ...args) => { const descriptor = originalOpen(filename, ...args); - // First compile the profile, then reload the canonical registry. Only - // the subsequent policy read observes replacement bytes. - if (filename === policyPath && ++policyOpens === 3) changedDescriptor = descriptor; + // The canonical registry is loaded once and shared with the profile + // compiler. Only the subsequent policy read observes replacement bytes. + if (filename === policyPath && ++policyOpens === 2) changedDescriptor = descriptor; return descriptor; }); context.mock.method(fs, 'readSync', (descriptor, buffer, offset, length, position) => { @@ -90,12 +90,32 @@ for (const load of [false, true]) { try { assert.throws(() => resolve(repoRoot, { proposedIds: ['skill:feature'] }, { load }), /Context source changed during selection/); - assert.equal(policyOpens, 3); + assert.equal(policyOpens, 2); assert.equal(alteredReads, 1); } finally { context.mock.restoreAll(); } })); } +test('resolution and the routing index hash the registry sources once per call', context => withFixture(repoRoot => { + const relative = 'skills/feature/agents/openai.yaml'; + write(repoRoot, relative, 'policy:\n allow_implicit_invocation: true\n'); + const policyPath = path.join(fs.realpathSync(repoRoot), relative); + const originalOpen = fs.openSync; + let opens = 0; + context.mock.method(fs, 'openSync', (filename, ...args) => { + if (filename === policyPath) opens++; + return originalOpen(filename, ...args); + }); + try { + // One open hashes the policy into the registry; one reads it for admission. + resolve(repoRoot, { query: 'use the feature skill for this change' }); + assert.equal(opens, 2, 'resolveTaskContext'); + opens = 0; + routingEntries({ repoRoot }); + assert.equal(opens, 2, 'routingEntries'); + } finally { context.mock.restoreAll(); } +})); + test('authority-bearing metadata cannot become automatic invocation', () => withFixture(repoRoot => { write(repoRoot, 'skills/feature/SKILL.md', '---\nname: feature\ndescription: Feature work\nallowed-tools: Bash\n---\nRun !`touch /tmp/never-run`'); assert.throws(() => resolve(repoRoot, { proposedIds: ['skill:feature'] }, { load: true }), /authority|dynamic/); @@ -727,3 +747,11 @@ test('singular forms do not create exact directive admission', () => { assert.ok(result.candidates.some(candidate => candidate.id === 'skill:database-migrations')); assert.ok(!result.receipt.autoSelection || !result.receipt.autoSelection.exact); }); + +test('invalid resolution and routing options are rejected before reading a missing registry', () => { + const repoRoot = path.join(__dirname, 'missing-registry'); + const values = { repoRoot, target: 'unknown-host' }; + assert.throws(() => resolveTaskContext({ ...values, task: task() }), /target/i); + assert.throws(() => routingEntries(values), /target/i); + assert.throws(() => routingEntries({ repoRoot, selectionMode: 'invalid' }), /Unknown selection mode/); +}); diff --git a/tests/lib/skill-evolution.test.js b/tests/lib/skill-evolution.test.js index 1860de247..53ed517b2 100644 --- a/tests/lib/skill-evolution.test.js +++ b/tests/lib/skill-evolution.test.js @@ -445,11 +445,41 @@ function runTests() { [{ ...valid, recorded_at: 'not-a-date' }, /recorded_at must be/], [{ ...valid, tokens_used: 'lots' }, /tokens_used must be a number/], [{ ...valid, duration_ms: Number.POSITIVE_INFINITY }, /duration_ms must be a number/], + ...[42, 0, false, {}, []].flatMap(value => [ + [{ ...valid, failure_reason: value }, /failure_reason must be a string or null/], + [{ ...valid, failureReason: value }, /failure_reason must be a string or null/], + ]), ]; for (const [payload, expectedError] of cases) { assert.throws(() => tracker.normalizeExecutionRecord(payload), expectedError); } + for (const value of [null, '', 'Useful detail']) { + assert.strictEqual( + tracker.normalizeExecutionRecord({ ...valid, failureReason: value }).failure_reason, + value || null + ); + } + for (const snake of ['', null, undefined]) { + assert.strictEqual(tracker.normalizeExecutionRecord({ + ...valid, failure_reason: snake, failureReason: 'Useful detail', + }).failure_reason, 'Useful detail'); + } + assert.strictEqual(tracker.normalizeExecutionRecord({ + ...valid, failure_reason: 'Primary detail', failureReason: 'Other detail', + }).failure_reason, 'Primary detail'); + for (const value of [42, 0, false, {}, []]) { + for (const payload of [ + { ...valid, failure_reason: value, failureReason: 'Useful detail' }, + { ...valid, failure_reason: '', failureReason: value }, + ]) assert.throws(() => tracker.normalizeExecutionRecord(payload), /failure_reason must be a string or null/); + } + const invalidRunsFile = path.join(homeDir, '.claude', 'state', 'invalid-reason-runs.jsonl'); + assert.throws( + () => tracker.recordSkillExecution({ ...valid, failure_reason: 42 }, { runsFilePath: invalidRunsFile }), + /failure_reason must be a string or null/ + ); + assert.ok(!fs.existsSync(invalidRunsFile), 'invalid reasons must be rejected before writing a run'); })) passed++; else failed++; if (test('uses state-store adapters for recording and reading when available', () => { diff --git a/tests/scripts/setup.test.js b/tests/scripts/setup.test.js index 715d383c2..806e31e24 100644 --- a/tests/scripts/setup.test.js +++ b/tests/scripts/setup.test.js @@ -646,6 +646,7 @@ test('migration JSON failures retain phase, scopes, and exact recovery', () => { assert.strictEqual(payload.error.phase, 'source-uninstall'); assert.deepStrictEqual([...payload.error.observedScopes].sort(), ['project', 'user']); assert.deepStrictEqual(payload.error.recovery, [ + 'claude plugin list --json', 'claude plugin uninstall ecc@ecc --scope user --keep-data', 'ecc setup --mode claude-plugin --scope project --move-scope --yes', ]); diff --git a/tests/test_agent_self_evaluation.py b/tests/test_agent_self_evaluation.py new file mode 100644 index 000000000..515256f96 --- /dev/null +++ b/tests/test_agent_self_evaluation.py @@ -0,0 +1,107 @@ +"""Input-mode regressions for the standalone agent output evaluator.""" + +import argparse +import errno +import importlib.util +import io +import re +import subprocess +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "skills/agent-self-evaluation/scripts/evaluate.py" +SPEC = importlib.util.spec_from_file_location("agent_self_evaluation", SCRIPT) +evaluator = importlib.util.module_from_spec(SPEC) +sys.modules[SPEC.name] = evaluator +SPEC.loader.exec_module(evaluator) + + +def args(**overrides): + return argparse.Namespace(**{"interactive": False, "task": None, "output": None, **overrides}) + + +def test_output_file_uses_piped_task(tmp_path, monkeypatch): + output = tmp_path / "response.txt" + output.write_text("All tests passed.", encoding="utf-8") + monkeypatch.setattr(sys, "stdin", io.StringIO("Add retry logic")) + + assert evaluator._read_input(args(output=str(output))) == ("Add retry logic", "All tests passed.") + + +def test_explicit_task_takes_precedence_over_pipe(tmp_path, monkeypatch): + output = tmp_path / "response.txt" + output.write_text("Answer", encoding="utf-8") + stream = io.StringIO("Ignored pipe") + monkeypatch.setattr(sys, "stdin", stream) + + assert evaluator._read_input(args(task="Explicit task", output=str(output))) == ("Explicit task", "Answer") + assert stream.tell() == 0 + + +def test_output_file_without_task_does_not_read_terminal(tmp_path, monkeypatch): + output = tmp_path / "response.txt" + output.write_text("Answer", encoding="utf-8") + monkeypatch.setattr(sys, "stdin", SimpleNamespace(isatty=lambda: True)) + + assert evaluator._read_input(args(output=str(output))) == (None, "Answer") + + +def test_no_output_file_keeps_stdin_as_output(monkeypatch): + monkeypatch.setattr(sys, "stdin", io.StringIO("Piped answer")) + assert evaluator._read_input(args(task="Inline task")) == ("Inline task", "Piped answer") + + +def test_interactive_mode_keeps_prompt_and_stdin(monkeypatch): + monkeypatch.setattr("builtins.input", lambda _prompt: " Interactive task ") + monkeypatch.setattr(sys, "stdin", io.StringIO("Interactive answer")) + assert evaluator._read_input(args(interactive=True)) == ("Interactive task", "Interactive answer") + + +def test_task_can_still_be_read_from_file(tmp_path, monkeypatch): + task = tmp_path / "task.txt" + task.write_text("File task", encoding="utf-8") + monkeypatch.setattr(sys, "stdin", io.StringIO("Piped answer")) + assert evaluator._read_input(args(task=str(task))) == ("File task", "Piped answer") + + +def test_missing_output_file_still_fails(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(sys, "stdin", io.StringIO("Task")) + with pytest.raises(SystemExit, match="1"): + evaluator._read_input(args(output=str(tmp_path / "missing.txt"))) + assert "not found" in capsys.readouterr().err + + +def test_cli_uses_piped_task_for_conciseness_score(tmp_path): + output = tmp_path / "response.txt" + output.write_text("Verified with tests. " * 25, encoding="utf-8") + result = subprocess.run( + [sys.executable, "-X", "utf8", str(SCRIPT), "--output", str(output)], + input="Fix bug", + capture_output=True, + encoding="utf-8", + check=True, + timeout=10, + ) + assert re.search(r"Conciseness[^\n]*3/5", result.stdout) + + +def test_long_inline_task_remains_text(tmp_path, monkeypatch): + output = tmp_path / "response.txt" + output.write_text("Answer", encoding="utf-8") + task = "Long explicit task description. " * 60 + monkeypatch.setattr(sys, "stdin", io.StringIO("Ignored pipe")) + assert evaluator._read_input(args(task=task, output=str(output))) == (task, "Answer") + + +@pytest.mark.parametrize("error_code", [errno.EACCES, errno.EISDIR]) +def test_task_read_does_not_hide_other_file_errors(monkeypatch, error_code): + def fail_open(_path): + raise OSError(error_code, "Cannot read task") + + monkeypatch.setattr("builtins.open", fail_open) + with pytest.raises(OSError) as captured: + evaluator._read_file_or_text("task-file") + assert captured.value.errno == error_code