diff --git a/bin/multiagent-safety.js b/bin/multiagent-safety.js index 6f57748..2d1e140 100755 --- a/bin/multiagent-safety.js +++ b/bin/multiagent-safety.js @@ -109,18 +109,20 @@ const TEMPLATE_FILES = [ 'vscode/guardex-active-agents/README.md', ]; -const LEGACY_WORKFLOW_SHIMS = [ - 'scripts/agent-branch-start.sh', - 'scripts/agent-branch-finish.sh', - 'scripts/agent-branch-merge.sh', - 'scripts/codex-agent.sh', - 'scripts/review-bot-watch.sh', - 'scripts/agent-worktree-prune.sh', - 'scripts/agent-file-locks.py', - 'scripts/openspec/init-plan-workspace.sh', - 'scripts/openspec/init-change-workspace.sh', +const LEGACY_WORKFLOW_SHIM_SPECS = [ + { relativePath: 'scripts/agent-branch-start.sh', kind: 'shell', command: ['branch', 'start'] }, + { relativePath: 'scripts/agent-branch-finish.sh', kind: 'shell', command: ['branch', 'finish'] }, + { relativePath: 'scripts/agent-branch-merge.sh', kind: 'shell', command: ['branch', 'merge'] }, + { relativePath: 'scripts/codex-agent.sh', kind: 'shell', command: ['internal', 'run-shell', 'codexAgent'] }, + { relativePath: 'scripts/review-bot-watch.sh', kind: 'shell', command: ['internal', 'run-shell', 'reviewBot'] }, + { relativePath: 'scripts/agent-worktree-prune.sh', kind: 'shell', command: ['worktree', 'prune'] }, + { relativePath: 'scripts/agent-file-locks.py', kind: 'python', command: ['locks'] }, + { relativePath: 'scripts/openspec/init-plan-workspace.sh', kind: 'shell', command: ['internal', 'run-shell', 'planInit'] }, + { relativePath: 'scripts/openspec/init-change-workspace.sh', kind: 'shell', command: ['internal', 'run-shell', 'changeInit'] }, ]; +const LEGACY_WORKFLOW_SHIMS = LEGACY_WORKFLOW_SHIM_SPECS.map((entry) => entry.relativePath); + const MANAGED_TEMPLATE_DESTINATIONS = TEMPLATE_FILES.map((entry) => toDestinationPath(entry)); const MANAGED_TEMPLATE_SCRIPT_FILES = MANAGED_TEMPLATE_DESTINATIONS.filter((entry) => entry.startsWith('scripts/'), @@ -248,6 +250,13 @@ const OMX_SCAFFOLD_FILES = new Map([ ['.omx/notepad.md', '\n\n## WORKING MEMORY\n'], ['.omx/project-memory.json', '{}\n'], ]); +const TARGETED_FORCEABLE_MANAGED_PATHS = new Set([ + 'AGENTS.md', + '.gitignore', + ...Array.from(OMX_SCAFFOLD_FILES.keys()), + ...REQUIRED_MANAGED_REPO_FILES, + ...LEGACY_WORKFLOW_SHIMS, +]); const COMMAND_TYPO_ALIASES = new Map([ ['relaese', 'release'], ['realaese', 'release'], @@ -1037,6 +1046,13 @@ function renderPythonDispatchShim(commandParts) { ); } +function managedForceConflictMessage(relativePath) { + return ( + `Refusing to overwrite existing file without --force: ${relativePath}\n` + + `Use '--force ${relativePath}' to rewrite only this managed file, or '--force' to rewrite all managed files.` + ); +} + function renderManagedFile(repoRoot, relativePath, content, options = {}) { const destinationPath = path.join(repoRoot, relativePath); const destinationExists = fs.existsSync(destinationPath); @@ -1050,7 +1066,7 @@ function renderManagedFile(repoRoot, relativePath, content, options = {}) { return { status: 'unchanged', file: relativePath }; } if (!force && !isCriticalGuardrailPath(relativePath)) { - throw new Error(`Refusing to overwrite existing file without --force: ${relativePath}`); + throw new Error(managedForceConflictMessage(relativePath)); } } @@ -1098,9 +1114,7 @@ function copyTemplateFile(repoRoot, relativeTemplatePath, force, dryRun) { return { status: 'unchanged', file: destinationRelativePath }; } if (!force && !isCriticalGuardrailPath(destinationRelativePath)) { - throw new Error( - `Refusing to overwrite existing file without --force: ${destinationRelativePath}`, - ); + throw new Error(managedForceConflictMessage(destinationRelativePath)); } } @@ -1151,6 +1165,22 @@ function ensureTemplateFilePresent(repoRoot, relativeTemplatePath, dryRun) { return { status: 'created', file: destinationRelativePath }; } +function ensureTargetedLegacyWorkflowShims(repoRoot, options) { + const targetedPaths = Array.isArray(options.forceManagedPaths) ? options.forceManagedPaths : []; + if (targetedPaths.length === 0) { + return []; + } + + const operations = []; + for (const shim of LEGACY_WORKFLOW_SHIM_SPECS) { + if (!shouldForceManagedPath(options, shim.relativePath)) { + continue; + } + operations.push(ensureGeneratedScriptShim(repoRoot, shim, { dryRun: options.dryRun, force: true })); + } + return operations; +} + function lockFilePath(repoRoot) { return path.join(repoRoot, LOCK_FILE_RELATIVE); } @@ -1457,8 +1487,65 @@ function requireValue(rawArgs, index, flagName) { return value; } +function normalizeManagedForcePath(rawPath) { + if (typeof rawPath !== 'string') { + return null; + } + const normalized = path.posix.normalize(rawPath.replace(/\\/g, '/')); + if (!normalized || normalized === '.' || normalized.startsWith('../') || path.posix.isAbsolute(normalized)) { + return null; + } + return normalized.startsWith('./') ? normalized.slice(2) : normalized; +} + +function collectForceManagedPaths(rawArgs, startIndex) { + const forceManagedPaths = []; + let nextIndex = startIndex; + + while (nextIndex + 1 < rawArgs.length) { + const candidate = rawArgs[nextIndex + 1]; + if (!candidate || candidate.startsWith('-')) { + break; + } + const normalized = normalizeManagedForcePath(candidate); + if (!normalized || !TARGETED_FORCEABLE_MANAGED_PATHS.has(normalized)) { + throw new Error(`Unknown managed path after --force: ${candidate}`); + } + forceManagedPaths.push(normalized); + nextIndex += 1; + } + + return { forceManagedPaths, nextIndex }; +} + +function appendForceArgs(args, options) { + if (!options.force) { + return; + } + args.push('--force'); + for (const managedPath of options.forceManagedPaths || []) { + args.push(managedPath); + } +} + +function shouldForceManagedPath(options, relativePath) { + if (!options.force) { + return false; + } + const targetedPaths = Array.isArray(options.forceManagedPaths) ? options.forceManagedPaths : []; + if (targetedPaths.length === 0) { + return true; + } + const normalized = normalizeManagedForcePath(relativePath); + return normalized !== null && targetedPaths.includes(normalized); +} + function parseCommonArgs(rawArgs, defaults) { const options = { ...defaults }; + const supportsForce = Object.prototype.hasOwnProperty.call(options, 'force'); + if (supportsForce && !Array.isArray(options.forceManagedPaths)) { + options.forceManagedPaths = []; + } for (let index = 0; index < rawArgs.length; index += 1) { const arg = rawArgs[index]; @@ -1480,7 +1567,17 @@ function parseCommonArgs(rawArgs, defaults) { continue; } if (arg === '--force') { + if (!supportsForce) { + throw new Error(`Unknown option: ${arg}`); + } options.force = true; + const parsed = collectForceManagedPaths(rawArgs, index); + if (parsed.forceManagedPaths.length > 0) { + options.forceManagedPaths = Array.from( + new Set([...(options.forceManagedPaths || []), ...parsed.forceManagedPaths]), + ); + } + index = parsed.nextIndex; continue; } if (arg === '--keep-stale-locks') { @@ -1598,6 +1695,7 @@ function parseSetupArgs(rawArgs, defaults) { function parseDoctorArgs(rawArgs) { const doctorDefaults = { target: process.cwd(), + force: false, dropStaleLocks: true, skipAgents: false, skipPackageJson: false, @@ -1746,6 +1844,7 @@ function runSetupBootstrapInternal(options) { target: installPayload.repoRoot, dryRun: options.dryRun, force: options.force, + forceManagedPaths: options.forceManagedPaths, dropStaleLocks: true, skipAgents: options.skipAgents, skipPackageJson: options.skipPackageJson, @@ -1783,7 +1882,7 @@ function resolveSandboxTarget(repoRoot, worktreePath, targetPath) { function buildSandboxSetupArgs(options, sandboxTarget) { const args = ['setup', '--target', sandboxTarget, '--no-global-install', '--no-recursive']; - if (options.force) args.push('--force'); + appendForceArgs(args, options); if (options.skipAgents) args.push('--skip-agents'); if (options.skipPackageJson) args.push('--skip-package-json'); if (options.skipGitignore) args.push('--no-gitignore'); @@ -1794,7 +1893,7 @@ function buildSandboxSetupArgs(options, sandboxTarget) { function buildSandboxDoctorArgs(options, sandboxTarget) { const args = ['doctor', '--target', sandboxTarget]; if (options.dryRun) args.push('--dry-run'); - if (options.force) args.push('--force'); + appendForceArgs(args, options); if (options.skipAgents) args.push('--skip-agents'); if (options.skipPackageJson) args.push('--skip-package-json'); if (options.skipGitignore) args.push('--no-gitignore'); @@ -5154,10 +5253,24 @@ function runInstallInternal(options) { operations.push(...ensureOmxScaffold(repoRoot, Boolean(options.dryRun))); for (const templateFile of TEMPLATE_FILES) { - operations.push(copyTemplateFile(repoRoot, templateFile, Boolean(options.force), Boolean(options.dryRun))); + operations.push( + copyTemplateFile( + repoRoot, + templateFile, + shouldForceManagedPath(options, toDestinationPath(templateFile)), + Boolean(options.dryRun), + ), + ); } + operations.push(...ensureTargetedLegacyWorkflowShims(repoRoot, options)); for (const hookName of HOOK_NAMES) { - operations.push(ensureHookShim(repoRoot, hookName, options)); + const hookRelativePath = path.posix.join('.githooks', hookName); + operations.push( + ensureHookShim(repoRoot, hookName, { + dryRun: options.dryRun, + force: shouldForceManagedPath(options, hookRelativePath), + }), + ); } operations.push(ensureLockRegistry(repoRoot, Boolean(options.dryRun))); @@ -5198,10 +5311,21 @@ function runFixInternal(options) { operations.push(...ensureOmxScaffold(repoRoot, Boolean(options.dryRun))); for (const templateFile of TEMPLATE_FILES) { + if (shouldForceManagedPath(options, toDestinationPath(templateFile))) { + operations.push(copyTemplateFile(repoRoot, templateFile, true, Boolean(options.dryRun))); + continue; + } operations.push(ensureTemplateFilePresent(repoRoot, templateFile, Boolean(options.dryRun))); } + operations.push(...ensureTargetedLegacyWorkflowShims(repoRoot, options)); for (const hookName of HOOK_NAMES) { - operations.push(ensureHookShim(repoRoot, hookName, options)); + const hookRelativePath = path.posix.join('.githooks', hookName); + operations.push( + ensureHookShim(repoRoot, hookName, { + dryRun: options.dryRun, + force: shouldForceManagedPath(options, hookRelativePath), + }), + ); } operations.push(ensureLockRegistry(repoRoot, Boolean(options.dryRun))); @@ -5715,6 +5839,7 @@ function doctor(rawArgs) { '--single-repo', '--target', repoPath, + ...(options.force ? ['--force', ...(options.forceManagedPaths || [])] : []), ...(options.dropStaleLocks ? [] : ['--keep-stale-locks']), ...(options.skipAgents ? ['--skip-agents'] : []), ...(options.skipPackageJson ? ['--skip-package-json'] : []), diff --git a/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/proposal.md b/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/proposal.md new file mode 100644 index 0000000..5cbb074 --- /dev/null +++ b/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/proposal.md @@ -0,0 +1,24 @@ +## Why + +- `gx doctor` currently surfaces managed-file conflicts like `scripts/review-bot-watch.sh`, but the recovery hint is incomplete: users can reasonably try `gx doctor --force scripts/review-bot-watch.sh` and hit `Unknown option`. +- `gx setup` has the same gap for managed template conflicts like `.github/workflows/cr.yml`. +- The CLI already distinguishes managed files from repo-owned package scripts and AGENTS content, so the remaining missing piece is a safe, explicit way to force only the named managed path instead of all managed files. + +## What Changes + +- Allow `gx setup`, `gx doctor`, and the shared repair/install aliases to accept managed relative paths after `--force`. +- Keep plain `--force` as the whole-surface rewrite path. +- Update managed-file conflict errors to explain both recovery options: + - `--force ` to rewrite only that file + - `--force` to rewrite all managed files +- Add install regressions for targeted doctor/setup force-path recovery. + +## Scope + +- `bin/multiagent-safety.js` +- `test/install.test.js` + +## Risks + +- Path matching must stay relative and deterministic so targeted force rewrites only the named managed file. +- The parser change must not accidentally relax other commands into accepting stray positional arguments. diff --git a/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/specs/doctor-setup-force-targets/spec.md b/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/specs/doctor-setup-force-targets/spec.md new file mode 100644 index 0000000..4da395b --- /dev/null +++ b/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/specs/doctor-setup-force-targets/spec.md @@ -0,0 +1,32 @@ +## ADDED Requirements + +### Requirement: setup and doctor accept targeted managed-file force paths + +`gx setup` and `gx doctor` SHALL accept one or more managed relative paths after `--force` so users can repair only the named managed files instead of rewriting the entire managed surface. + +#### Scenario: doctor rewrites one named managed shim + +- **GIVEN** a repo has a conflicting managed `scripts/review-bot-watch.sh` +- **WHEN** the user runs `gx doctor --force scripts/review-bot-watch.sh` +- **THEN** the command succeeds +- **AND** `scripts/review-bot-watch.sh` is rewritten to the current managed shim +- **AND** the path selector is not treated as an unknown option + +#### Scenario: setup rewrites one named managed template + +- **GIVEN** a repo has a conflicting managed `.github/workflows/cr.yml` +- **WHEN** the user runs `gx setup --force .github/workflows/cr.yml` +- **THEN** the command succeeds +- **AND** `.github/workflows/cr.yml` is rewritten to the current managed template + +### Requirement: conflict output teaches targeted and global force recovery + +When a managed file conflict blocks `gx setup` or `gx doctor`, the CLI SHALL tell the user how to recover with either a targeted `--force ` or a full-surface `--force`. + +#### Scenario: conflict message names both force paths + +- **GIVEN** a managed file differs from the current Guardex output +- **WHEN** `gx setup` or `gx doctor` hits that conflict without `--force` +- **THEN** the error names the conflicting managed path +- **AND** the error teaches `--force ` for one-file recovery +- **AND** the error teaches plain `--force` for rewriting all managed files diff --git a/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/tasks.md b/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/tasks.md new file mode 100644 index 0000000..23e3d20 --- /dev/null +++ b/openspec/changes/agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58/tasks.md @@ -0,0 +1,29 @@ +## 1. Spec + +- [x] 1.1 Define the targeted managed-file `--force` behavior in `specs/doctor-setup-force-targets/spec.md`. +- [x] 1.2 Capture the recovery UX problem and bounded scope in `proposal.md`. + +## 2. Tests + +- [x] 2.1 Add a regression that `gx doctor --force scripts/review-bot-watch.sh` rewrites the named managed shim instead of throwing `Unknown option`. +- [x] 2.2 Add a regression that `gx setup --force .github/workflows/cr.yml` rewrites the named managed template. +- [x] 2.3 Lock the conflict message so it teaches both targeted `--force ` and global `--force`. + +## 3. Implementation + +- [x] 3.1 Extend the shared setup/doctor/install/fix arg parsing to accept managed path selectors only after `--force`. +- [x] 3.2 Route targeted force-path matching through the managed file/template rewrite helpers. +- [x] 3.3 Preserve the existing plain `--force` behavior for whole-surface rewrites. + +## 4. Verification + +- [x] 4.1 Run `node --check bin/multiagent-safety.js`. +- [x] 4.2 Run targeted install regressions in `test/install.test.js`. +- [x] 4.3 Run `openspec validate agent-codex-fix-doctor-setup-force-conflict-ux-2026-04-22-08-58 --type change --strict`. +- [x] 4.4 Run `openspec validate --specs`. + +## 5. Cleanup + +- [x] 5.1 Confirm the OpenSpec tasks reflect the shipped behavior and note any residual risk. Residual risk: targeted `--force` selectors intentionally fail fast for unlisted paths, and this worktree currently has no main specs for `openspec validate --specs` beyond the clean `No items found to validate.` result. +- [ ] 5.2 Finish the agent branch via PR merge + cleanup (`gx finish --via-pr --wait-for-merge --cleanup` or `bash scripts/agent-branch-finish.sh --branch --base --via-pr --wait-for-merge --cleanup`). +- [ ] 5.3 Record PR URL + final `MERGED` evidence in the completion handoff. diff --git a/test/install.test.js b/test/install.test.js index 58e7ab5..b3a5931 100644 --- a/test/install.test.js +++ b/test/install.test.js @@ -651,6 +651,69 @@ test('setup and doctor explain .githooks file conflicts and still write managed assertZeroCopyManagedGitignore(gitignoreContent); }); +test('doctor --force rewrites only the named managed shim', () => { + const repoDir = initRepo(); + + let result = runNode(['setup', '--target', repoDir, '--no-global-install'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + + const reviewScriptPath = path.join(repoDir, 'scripts', 'review-bot-watch.sh'); + const workflowPath = path.join(repoDir, '.github', 'workflows', 'cr.yml'); + fs.writeFileSync(reviewScriptPath, '#!/usr/bin/env bash\nprintf "custom review shim\\n"\n', 'utf8'); + fs.chmodSync(reviewScriptPath, 0o755); + fs.writeFileSync(workflowPath, '# custom workflow\n', 'utf8'); + + result = runNode( + ['doctor', '--target', repoDir, '--force', 'scripts/review-bot-watch.sh'], + repoDir, + ); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.doesNotMatch(`${result.stdout}\n${result.stderr}`, /Unknown option:/); + const managedReviewShim = fs.readFileSync(reviewScriptPath, 'utf8'); + assert.match(managedReviewShim, /exec "\$node_bin" "\$GUARDEX_CLI_ENTRY" 'internal' 'run-shell' 'reviewBot' "\$@"/); + assert.match(managedReviewShim, /exec "\$cli_bin" 'internal' 'run-shell' 'reviewBot' "\$@"/); + assert.equal(fs.readFileSync(workflowPath, 'utf8'), '# custom workflow\n'); + assert.match(result.stdout, /skipped-conflict\s+\.github\/workflows\/cr\.yml/); +}); + +test('setup --force rewrites the named managed template', () => { + const repoDir = initRepo(); + + let result = runNode(['setup', '--target', repoDir, '--no-global-install'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + + const workflowPath = path.join(repoDir, '.github', 'workflows', 'cr.yml'); + const managedWorkflow = fs.readFileSync(workflowPath, 'utf8'); + fs.writeFileSync(workflowPath, '# custom workflow\n', 'utf8'); + + result = runNode( + ['setup', '--target', repoDir, '--force', '.github/workflows/cr.yml', '--no-global-install'], + repoDir, + ); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.doesNotMatch(`${result.stdout}\n${result.stderr}`, /Unknown option:/); + assert.equal(fs.readFileSync(workflowPath, 'utf8'), managedWorkflow); +}); + +test('setup conflict message teaches targeted and global managed --force recovery', () => { + const repoDir = initRepo(); + + let result = runNode(['setup', '--target', repoDir, '--no-global-install'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + + const dockerLoaderPath = path.join(repoDir, 'scripts', 'guardex-docker-loader.sh'); + fs.writeFileSync(dockerLoaderPath, '#!/usr/bin/env bash\nprintf "custom docker loader\\n"\n', 'utf8'); + fs.chmodSync(dockerLoaderPath, 0o755); + + result = runNode(['setup', '--target', repoDir, '--no-global-install'], repoDir); + assert.notEqual(result.status, 0, 'setup should fail on non-critical managed conflicts without --force'); + + const combined = `${result.stdout}\n${result.stderr}`; + assert.match(combined, /Refusing to overwrite existing file without --force: scripts\/guardex-docker-loader\.sh/); + assert.match(combined, /--force scripts\/guardex-docker-loader\.sh/); + assert.match(combined, /--force' to rewrite all managed files/); +}); + test('setup and doctor skip repo bootstrap when repo .env disables Guardex', () => { const repoDir = initRepo(); fs.writeFileSync(path.join(repoDir, '.env'), 'GUARDEX_ON=0\n', 'utf8');