Simplify manual Apply rollback flow

Centralize repeated rollback result construction, repair context, and entry verification without changing the live Apply contract.

AI-assisted: prepared by Codex under pbakaus's scheduled architecture-refactor authorization.

cli/bin/commands/skills.mjs

@@ -699,7 +699,7 @@ function deduplicateProviders(root, providers, scope) {
  * SKILL.md, so script-only fixes and removed files are detected.
  * Returns true if every bundle skill matches the local copy.
  */
-function isUpToDate(root, providers, bundleDir, scope, agentScope = scope) {
+function isUpToDate(root, providers, bundleDir, scope) {
   const unique = deduplicateProviders(root, providers, scope);
   if (unique.length === 0) return false;
 
@@ -724,8 +724,6 @@ function isUpToDate(root, providers, bundleDir, scope, agentScope = scope) {
         if (bundleHash !== localHash) return false;
       }
     }
-
-    if (!providerAgentsUpToDate(bundleDir, root, provider, agentScope)) return false;
   }
   return true;
 }
@@ -747,8 +745,7 @@ async function check() {
   console.log('Checking for updates...\n');
   try {
     const bundleDir = await downloadAndExtractBundle();
-    const agentScope = isHomeDir(root) ? 'user' : undefined;
-    const upToDate = isUpToDate(root, providers, bundleDir, undefined, agentScope);
+    const upToDate = isUpToDate(root, providers, bundleDir);
     rmSync(bundleDir, { recursive: true, force: true });
 
     if (upToDate) {
@@ -1254,9 +1251,7 @@ function copyProviderSkills(bundleDir, root, targets, { scope } = {}) {
 }
 
 // Native subagent definitions that ship in the bundle next to a provider's
-// skills. Claude Code's live at `.claude/agents/impeccable-*.md`; project
-// agents take precedence over user agents. GitHub Copilot's live at
-// `.github/agents/impeccable-*.agent.md`:
+// skills. GitHub Copilot's live at `.github/agents/impeccable-*.agent.md`:
 // project installs commit them at `<repo>/.github/agents/`, user-level
 // installs go to `~/.copilot/agents/` (Copilot's user-scope dir, NOT
 // `~/.github/`). On a name conflict Copilot lets the user-level file shadow
@@ -1266,11 +1261,6 @@ function copyProviderSkills(bundleDir, root, targets, { scope } = {}) {
 // `~/.cursor/agents/`; project agents take precedence there, so no shadow
 // warning is needed.
 const PROVIDER_AGENT_ARTIFACTS = {
-  '.claude': {
-    ext: '.md',
-    userDir: home => join(home, '.claude', 'agents'),
-    userShadowsProject: false,
-  },
   '.github': {
     ext: '.agent.md',
     userDir: home => join(home, '.copilot', 'agents'),
@@ -1283,23 +1273,6 @@ const PROVIDER_AGENT_ARTIFACTS = {
   },
 };
 
-function providerAgentsUpToDate(bundleDir, root, provider, scope) {
-  const artifact = PROVIDER_AGENT_ARTIFACTS[provider];
-  if (!artifact) return true;
-  const srcDir = join(bundleDir, provider, 'agents');
-  if (!existsSync(srcDir)) return true;
-
-  const destDir = scope === 'user'
-    ? artifact.userDir(root)
-    : join(root, provider, 'agents');
-  const agentFiles = readdirSync(srcDir).filter(name => name.endsWith(artifact.ext));
-  return agentFiles.every(name => {
-    const localPath = join(destDir, name);
-    return existsSync(localPath)
-      && hashSkillFile(join(srcDir, name)) === hashSkillFile(localPath);
-  });
-}
-
 function copyProviderAgents(bundleDir, root, providers, { scope, home = homedir() } = {}) {
   const targets = Array.isArray(providers) ? providers : [providers];
   const results = [];
@@ -2099,9 +2072,7 @@ function resolveUpdateTarget({ projectRoot, home, explicitScope }) {
   const homeRooted = isHomeDir(projectRoot);
   if (homeRooted && !explicitScope) {
     const providers = findInstalledProviders(home);
-    return providers.length
-      ? { root: home, scope: undefined, agentScope: 'user', providers, scopeLabel: 'user level' }
-      : null;
+    return providers.length ? { root: home, scope: undefined, providers, scopeLabel: 'user level' } : null;
   }
 
   const projectProviders = homeRooted ? [] : findImpeccableProviders(projectRoot, 'project');
@@ -2232,7 +2203,7 @@ async function update(flags = []) {
       : { root: projectRoot, scope: 'project', providers: target.projectProviders, scopeLabel: 'this project' };
   }
 
-  const { root, scope, agentScope = scope } = target;
+  const { root, scope } = target;
   console.log(`Updating the ${target.scopeLabel} install: ${formatPathForDisplay(root)} (${target.providers.join(', ')})`);
   const providers = target.providers;
   const linkedProviders = findLinkedProviders(root, providers, scope);
@@ -2256,7 +2227,7 @@ async function update(flags = []) {
   }
 
   // Compare local vs remote -- skip if already up to date
-  if (isUpToDate(root, copyProviders, tmpDir, scope, agentScope)) {
+  if (isUpToDate(root, copyProviders, tmpDir, scope)) {
     try {
       const wantHooks = installHooks && await decideHookInstall(root, copyProviders, { yes });
       const hookTargets = wantHooks ? copyProviderHooks(tmpDir, root, copyProviders, { force }) : [];
@@ -2292,7 +2263,7 @@ async function update(flags = []) {
     if (migrated > 0) console.log('Migrated a prefixed install back to /impeccable (the i- prefix is no longer used).');
 
     const updated = refreshProviderSkills(tmpDir, root, copyProviders, scope);
-    reportProviderAgents(copyProviderAgents(tmpDir, root, copyProviders, { scope: agentScope }));
+    reportProviderAgents(copyProviderAgents(tmpDir, root, copyProviders, { scope }));
     const wantHooks = installHooks && await decideHookInstall(root, providers, { yes });
     const hookTargets = wantHooks ? copyProviderHooks(tmpDir, root, providers, { force }) : [];
 

skill/scripts/live-commit-manual-edits.mjs

@@ -944,8 +944,42 @@ export async function commitManualEdits({
     };
   }
 
+  const repairContext = {
+    batch,
+    cwd,
+    pageUrl,
+    count,
+    provider,
+    env,
+    timeoutMs,
+    applyBatchToSource,
+    chatAvailable,
+    transactionId,
+  };
+
   const baseRollbackScope = collectApplyOwnedFiles(batch, cwd);
   const rollbackSnapshot = snapshotRollbackFiles(cwd, baseRollbackScope);
+  const failWithRollback = ({
+    scope = baseRollbackScope,
+    extraFiles = [],
+    failed,
+    files = [],
+    details = {},
+  }) => {
+    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, extraFiles, scope);
+    return {
+      applied: [],
+      failed,
+      files,
+      cleared: 0,
+      count,
+      pageUrl,
+      ...details,
+      rolledBackFiles: rollback.rolledBackFiles,
+      rollbackFailures: rollback.rollbackFailures,
+      ...countByPage(cwd),
+    };
+  };
   let result;
   try {
     result = repairOnly
@@ -965,42 +999,27 @@ export async function commitManualEdits({
           chatAvailable,
         });
   } catch (err) {
-    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, [], baseRollbackScope);
-    return {
-      applied: [],
+    return failWithRollback({
       failed: batch.entries.map((entry) => ({
         id: entry.id,
         reason: err.message || String(err),
         candidates: candidatesForEntry(batch, entry.id),
       })),
-      files: [],
-      cleared: 0,
-      count,
-      pageUrl,
-      rolledBackFiles: rollback.rolledBackFiles,
-      rollbackFailures: rollback.rollbackFailures,
-      ...countByPage(cwd),
-    };
+    });
   }
 
   if (result.status === 'error') {
     const rollbackScope = collectApplyOwnedFiles(batch, cwd, result.files || []);
-    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, result.files || [], rollbackScope);
     const failed = normalizeFailedEntries(batch, result, result.message || 'AI copy edit failed');
-    return {
-      applied: [],
+    return failWithRollback({
+      scope: rollbackScope,
+      extraFiles: result.files || [],
       failed: failed.length > 0
         ? failed
         : verificationFailuresForEntries(batch, batch.entries, result.message || 'AI copy edit failed'),
       files: result.files || [],
-      cleared: 0,
-      count,
-      pageUrl,
-      notes: result.notes || [],
-      rolledBackFiles: rollback.rolledBackFiles,
-      rollbackFailures: rollback.rollbackFailures,
-      ...countByPage(cwd),
-    };
+      details: { notes: result.notes || [] },
+    });
   }
 
   const reportedAppliedIds = uniqueStrings(result.appliedEntryIds || []);
@@ -1013,72 +1032,44 @@ export async function commitManualEdits({
   const conflictingAppliedIds = reportedAppliedIds.filter((id) => failedIds.has(id));
 
   if (conflictingAppliedIds.length > 0) {
-    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, result.files || [], rollbackScope);
     const conflictingEntries = batch.entries.filter((entry) => conflictingAppliedIds.includes(entry.id));
-    return {
-      applied: [],
+    return failWithRollback({
+      scope: rollbackScope,
+      extraFiles: result.files || [],
       failed: [
         ...verificationFailuresForEntries(batch, conflictingEntries, 'conflicting_apply_result'),
         ...aiFailed.filter((item) => !conflictingAppliedIds.includes(item.id)),
       ],
       files: result.files || [],
-      cleared: 0,
-      count,
-      pageUrl,
-      notes: result.notes || [],
-      rolledBackFiles: rollback.rolledBackFiles,
-      rollbackFailures: rollback.rollbackFailures,
-      ...countByPage(cwd),
-    };
+      details: { notes: result.notes || [] },
+    });
   }
 
   const unreportedFiles = unreportedChangedFiles(cwd, rollbackSnapshot, result.files || [], rollbackScope);
   if (unreportedFiles.length > 0) {
-    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, result.files || [], [...rollbackScope, ...unreportedFiles]);
-    return {
-      applied: [],
+    return failWithRollback({
+      scope: [...rollbackScope, ...unreportedFiles],
+      extraFiles: result.files || [],
       failed: verificationFailuresForEntries(batch, batch.entries, 'unreported_source_changes', { files: unreportedFiles }),
       files: result.files || [],
-      unreportedFiles,
-      cleared: 0,
-      count,
-      pageUrl,
-      notes: result.notes || [],
-      rolledBackFiles: rollback.rolledBackFiles,
-      rollbackFailures: rollback.rollbackFailures,
-      ...countByPage(cwd),
-    };
+      details: { unreportedFiles, notes: result.notes || [] },
+    });
   }
 
   if (result.status === 'done' && reportedAppliedIds.length === 0) {
-    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, result.files || [], rollbackScope);
-    return {
-      applied: [],
+    return failWithRollback({
+      scope: rollbackScope,
+      extraFiles: result.files || [],
       failed: verificationFailuresForEntries(batch, batch.entries, 'missing_applied_entry_ids'),
       files: result.files || [],
-      cleared: 0,
-      count,
-      pageUrl,
-      notes: result.notes || [],
-      rolledBackFiles: rollback.rolledBackFiles,
-      rollbackFailures: rollback.rollbackFailures,
-      ...countByPage(cwd),
-    };
+      details: { notes: result.notes || [] },
+    });
   }
 
   const reportedAppliedEntries = batch.entries.filter((entry) => reportedAppliedIds.includes(entry.id));
   if (reportedAppliedIds.length > 0 && reportedFiles.length === 0) {
     return repairPostApplyValidation({
-      batch,
-      cwd,
-      pageUrl,
-      count,
-      provider,
-      env,
-      timeoutMs,
-      applyBatchToSource,
-      chatAvailable,
-      transactionId,
+      ...repairContext,
       appliedEntryIds: reportedAppliedIds,
       files: result.files || [],
       failed: aiFailed,
@@ -1089,21 +1080,10 @@ export async function commitManualEdits({
     });
   }
 
-  const verifiedAppliedIds = [];
-  const verificationFailed = [];
-  for (const entry of reportedAppliedEntries) {
-    const failures = verifyAppliedEntry({ batch, entry, reportedFiles, cwd });
-    if (failures.length === 0) {
-      verifiedAppliedIds.push(entry.id);
-    } else {
-      verificationFailed.push({
-        id: entry.id,
-        reason: 'source_verification_failed',
-        failures,
-        candidates: candidatesForEntry(batch, entry.id),
-      });
-    }
-  }
+  const {
+    verifiedIds: verifiedAppliedIds,
+    failed: verificationFailed,
+  } = verifyEntriesAfterRepair({ batch, appliedEntryIds: reportedAppliedIds, files: reportedFiles, cwd });
   const unreportedEntries = result.status === 'done' || result.status === 'partial'
     ? batch.entries.filter((entry) => !reportedAppliedIds.includes(entry.id) && !aiFailed.some((item) => item.id === entry.id))
     : [];
@@ -1133,37 +1113,22 @@ export async function commitManualEdits({
         reason: 'rolled_back_due_to_failed_entry_source_changed',
         candidates: candidatesForEntry(batch, entry.id),
       }));
-    const rollback = rollbackChangedFiles(cwd, rollbackSnapshot, result.files || [], rollbackScope);
-    return {
-      applied: [],
+    return failWithRollback({
+      scope: rollbackScope,
+      extraFiles: result.files || [],
       failed: [
         ...leakedUnapplied,
         ...failed.filter((item) => !leakedIds.has(item.id)),
         ...rolledBackVerified,
       ],
       files: result.files || [],
-      cleared: 0,
-      count,
-      pageUrl,
-      rolledBackFiles: rollback.rolledBackFiles,
-      rollbackFailures: rollback.rollbackFailures,
-      notes: result.notes || [],
-      ...countByPage(cwd),
-    };
+      details: { notes: result.notes || [] },
+    });
   }
 
   if (verificationFailed.length > 0) {
     return repairPostApplyValidation({
-      batch,
-      cwd,
-      pageUrl,
-      count,
-      provider,
-      env,
-      timeoutMs,
-      applyBatchToSource,
-      chatAvailable,
-      transactionId,
+      ...repairContext,
       appliedEntryIds: reportedAppliedIds,
       files: result.files || [],
       failed: nonRepairFailed,
@@ -1180,16 +1145,7 @@ export async function commitManualEdits({
       ? reportedAppliedEntries.filter((entry) => verifiedAppliedIds.includes(entry.id))
       : batch.entries;
     return repairPostApplyValidation({
-      batch,
-      cwd,
-      pageUrl,
-      count,
-      provider,
-      env,
-      timeoutMs,
-      applyBatchToSource,
-      chatAvailable,
-      transactionId,
+      ...repairContext,
       appliedEntryIds: verifiedAppliedIds.length > 0
         ? verifiedAppliedIds
         : postCheckEntries.map((entry) => entry.id).filter(Boolean),

tests/skills-cli.test.js

@@ -84,13 +84,11 @@ function createFakeUniversalBundle(root, providers = ['.claude', '.agents', '.cu
     writeFileSync(join(skillDir, 'scripts', 'context.mjs'), 'console.log("local bundle context");\n');
   }
   if (providers.includes('.claude')) {
-    mkdirSync(join(bundleRoot, '.claude', 'agents'), { recursive: true });
+    mkdirSync(join(bundleRoot, '.claude'), { recursive: true });
     writeFileSync(join(bundleRoot, '.claude', 'settings.json'), JSON.stringify({
       description: 'fresh claude hook',
       hooks: { PostToolUse: [{ matcher: 'Edit', hooks: [{ type: 'command', command: 'node ".claude/skills/impeccable/scripts/hook.mjs"' }] }] },
     }, null, 2));
-    writeFileSync(join(bundleRoot, '.claude', 'agents', 'impeccable-finish-reviewer.md'),
-      '---\nname: impeccable-finish-reviewer\ndescription: Reviews a finished build.\n---\nClaude reviewer body.\n');
   }
   if (providers.includes('.cursor')) {
     mkdirSync(join(bundleRoot, '.cursor'), { recursive: true });
@@ -230,51 +228,7 @@ describe('copyProviderSkills: symlink handling', () => {
   });
 });
 
-describe('copyProviderAgents: Claude, Copilot, and Cursor subagents', () => {
-  test('Claude project and user scopes use .claude/agents, with project copies taking precedence', () => {
-    const tmp = mkdtempSync(join(tmpdir(), 'imp-agents-claude-'));
-    const home = mkdtempSync(join(tmpdir(), 'imp-agents-claude-home-'));
-    const bundle = createFakeUniversalBundle(tmp, ['.claude']);
-    mkdirSync(join(home, '.claude', 'agents'), { recursive: true });
-    writeFileSync(join(home, '.claude', 'agents', 'impeccable-finish-reviewer.md'), 'stale copy\n');
-
-    const projectResults = copyProviderAgents(bundle, tmp, ['.claude'], { scope: 'project', home });
-    const userResults = copyProviderAgents(bundle, home, ['.claude'], { scope: 'user' });
-
-    expect(projectResults).toHaveLength(1);
-    expect(projectResults[0].shadowed).toEqual([]);
-    expect(userResults).toHaveLength(1);
-    expect(readFileSync(join(tmp, '.claude', 'agents', 'impeccable-finish-reviewer.md'), 'utf8'))
-      .toContain('Claude reviewer body.');
-    expect(readFileSync(join(home, '.claude', 'agents', 'impeccable-finish-reviewer.md'), 'utf8'))
-      .toContain('Claude reviewer body.');
-
-    rmSync(tmp, { recursive: true, force: true });
-    rmSync(home, { recursive: true, force: true });
-  });
-
-  test('Claude install and update backfill bundled agents beside an unchanged skill', () => {
-    const tmp = mkdtempSync(join(tmpdir(), 'imp-agents-claude-install-'));
-    const home = mkdtempSync(join(tmpdir(), 'imp-agents-claude-install-home-'));
-    execSync('git init', { cwd: tmp });
-    const bundleRoot = createFakeUniversalBundle(tmp, ['.claude']);
-    const env = { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot };
-    const agentPath = join(tmp, '.claude', 'agents', 'impeccable-finish-reviewer.md');
-
-    const installOutput = run('skills install -y --no-hooks --providers=claude', { cwd: tmp, env });
-    expect(installOutput).toContain('Installed Claude Code agents into:');
-    expect(existsSync(agentPath)).toBe(true);
-
-    rmSync(agentPath);
-    const updateOutput = run('skills update -y --no-hooks', { cwd: tmp, env });
-    expect(updateOutput).toContain('Updated');
-    expect(updateOutput).toContain('Installed Claude Code agents into:');
-    expect(existsSync(agentPath)).toBe(true);
-
-    rmSync(tmp, { recursive: true, force: true });
-    rmSync(home, { recursive: true, force: true });
-  }, 15000);
-
+describe('copyProviderAgents: Copilot and Cursor subagents', () => {
   test('project scope places agents at .github/agents/ and .cursor/agents/', () => {
     const tmp = mkdtempSync(join(tmpdir(), 'imp-agents-project-'));
     const bundle = createFakeUniversalBundle(tmp, ['.github', '.cursor']);
@@ -308,46 +262,6 @@ describe('copyProviderAgents: Claude, Copilot, and Cursor subagents', () => {
     rmSync(home, { recursive: true, force: true });
   });
 
-  test('skills check accepts current Copilot user agents in a home-rooted checkout', () => {
-    const home = mkdtempSync(join(tmpdir(), 'imp-agents-check-home-'));
-    execSync('git init', { cwd: home });
-    const bundleRoot = createFakeUniversalBundle(home, ['.github']);
-    const env = { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot };
-
-    run('skills install -y --scope=global --no-hooks --providers=github', { cwd: home, env });
-    expect(existsSync(join(home, '.copilot', 'agents', 'impeccable-finish-reviewer.agent.md'))).toBe(true);
-    expect(existsSync(join(home, '.github', 'agents'))).toBe(false);
-
-    const output = run('skills check', { cwd: home, env });
-    expect(output).toContain('Skills are up to date');
-    expect(output).not.toContain('Updates available');
-
-    rmSync(home, { recursive: true, force: true });
-  }, 15000);
-
-  test('inferred home-rooted updates refresh stale or missing Copilot user agents', () => {
-    const home = mkdtempSync(join(tmpdir(), 'imp-agents-update-home-'));
-    execSync('git init', { cwd: home });
-    const bundleRoot = createFakeUniversalBundle(home, ['.github']);
-    const env = { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot };
-    const userAgent = join(home, '.copilot', 'agents', 'impeccable-finish-reviewer.agent.md');
-    const projectAgent = join(home, '.github', 'agents', 'impeccable-finish-reviewer.agent.md');
-
-    run('skills install -y --scope=global --no-hooks --providers=github', { cwd: home, env });
-    writeFileSync(userAgent, 'stale copy\n');
-
-    run('skills update -y --no-hooks', { cwd: home, env });
-    expect(readFileSync(userAgent, 'utf8')).toContain('Copilot reviewer body.');
-    expect(existsSync(projectAgent)).toBe(false);
-
-    rmSync(userAgent);
-    run('skills update -y --no-hooks', { cwd: home, env });
-    expect(readFileSync(userAgent, 'utf8')).toContain('Copilot reviewer body.');
-    expect(existsSync(projectAgent)).toBe(false);
-
-    rmSync(home, { recursive: true, force: true });
-  }, 20000);
-
   test('project scope reports user-level Copilot agents that shadow the installed ones; Cursor never does (project wins there)', () => {
     const tmp = mkdtempSync(join(tmpdir(), 'imp-agents-shadow-'));
     const home = mkdtempSync(join(tmpdir(), 'imp-agents-shadow-home-'));