Kevin/improved errors (#61)

* feat: Improved apply validation errors

* feat: Refactored to share the same plugin error type. Use the reporter to determine how to present it.

* fix: Fixed display functions to be async

* feat: Improve the copy and formatting for apply validation error. Fix pretty print error for NOOP. Added to CLAUDE.md

* feat: Improved copy again

* feat: Added partial applys. If a resource fails to apply, skip it and all transitive dependent resources. Apply un-related resources.

* feat: Further refactored the error messages. Combined it with a final display message change. The final display message will now tell the user a list of all changes that were made.

* feat: refactored the code so that the final apply message is handled entirely inside the reporters.

* feat: Additional refactoring. Cleaned up dead APPLY_VALIDATION_ERROR dead code. Moved rendering logic to the ApplyComplete component instead.

* fix: Fix /etc/os-release not present

Author: kevinwang5658

CLAUDE.md

@@ -184,4 +184,27 @@ Parent Process              Plugin Process
 3. **Plugin IPC**: Plugins cannot directly read stdin (security isolation)
 4. **Sudo Caching**: Password cached in memory during session unless `--secure` flag used
 5. **File Watcher**: Use `persistent: false` option to prevent hanging processes
-6. **Linting**: ESLint enforces single quotes, specific import ordering, and strict type safety
+6. **Linting**: ESLint enforces single quotes, specific import ordering, and strict type safety
+7. **Reporter display methods are async**: All `Reporter` interface display methods (`displayPlan`, `displayImportResult`, `displayFileModifications`, `displayMessage`, `displayPluginError`) return `Promise<void>`. Always `await` them at call sites — `DefaultReporter.updateRenderState()` has a 50ms sleep, so unawaited calls cause `process.exit(1)` to fire before the UI renders.
+8. **Mock reporter async assertions**: Assertions inside `MockReporter` config callbacks (e.g. `displayFileModifications`) will silently pass if the call isn't awaited. Making display methods async surfaced latent bugs where expected file paths were wrong.
+
+## Plugin Error Handling Architecture
+
+Plugin errors flow as structured `PluginErrorData` over IPC and are caught as `PluginError` instances on the CLI side:
+
+**IPC envelope** (`@codifycli/schemas`):
+```typescript
+interface PluginErrorData {
+  errorType: string;   // 'apply_validation' | 'sudo_error' | 'unknown'
+  message: string;
+  data?: unknown;
+}
+```
+
+**CLI carrier** (`src/common/errors.ts`): `PluginError extends CodifyError` holds `pluginName`, `resourceType`, and `errorData: PluginErrorData`.
+
+**Reporter as view model**: Reporters (not components) decide how to render each `errorType`. `DefaultReporter.displayPluginError()` branches on `errorType` to set the appropriate `RenderStatus` (`APPLY_VALIDATION_ERROR` with a `ResourcePlan` for plan diffs, `PLUGIN_ERROR` with a message string for generic errors). The `DefaultComponent` is purely display.
+
+**Shared formatter**: `src/ui/plugin-error-formatter.ts` exports `formatApplyValidationError(error: PluginError): string` used by both `PlainReporter` and `DefaultComponent`.
+
+**Backward compat**: `plugin.ts#toErrorData()` validates IPC data against `ErrorResponseDataSchema` (AJV); falls back to `{ errorType: 'unknown', message: data }` for old plugins sending bare strings.

package-lock.json

@@ -5,7 +5,7 @@
   },
   "dependencies": {
     "@codifycli/ink-form": "0.0.12",
-    "@codifycli/schemas": "1.1.0-beta5",
+    "@codifycli/schemas": "1.1.0-beta8",
     "@homebridge/node-pty-prebuilt-multiarch": "^0.12.0-beta.5",
     "@mischnic/json-sourcemap": "^0.1.1",
     "@oclif/core": "^4.0.8",
@@ -43,7 +43,7 @@
   },
   "description": "Codify is a configuration-as-code tool that declaratively installs and manages developer tools and applications. Check out https://dashboard.codifycli.com for an editor.",
   "devDependencies": {
-    "@codifycli/plugin-core": "^1.1.0-beta13",
+    "@codifycli/plugin-core": "^1.1.0-beta19",
     "@oclif/prettier-config": "^0.2.1",
     "@types/chalk": "^2.2.0",
     "@types/cors": "^2.8.19",

package.json

@@ -11,7 +11,7 @@ import { DefaultReporter } from '../ui/reporters/default-reporter.js';
 import { Reporter, ReporterFactory, ReporterType } from '../ui/reporters/reporter.js';
 import { spawnSafe } from '../utils/spawn.js';
 import { SudoUtils } from '../utils/sudo.js';
-import { prettyPrintError } from './errors.js';
+import { PluginError, prettyPrintError } from './errors.js';
 
 export abstract class BaseCommand extends Command {
   static baseFlags = {
@@ -145,6 +145,11 @@ export abstract class BaseCommand extends Command {
   }
 
   protected async catch(err: Error): Promise<void> {
+    if (err instanceof PluginError && this.reporter) {
+      await this.reporter.displayPluginError(err);
+      process.exit(1);
+    }
+
     prettyPrintError(err);
     process.exit(1);
   }

src/common/base-command.ts

@@ -1,5 +1,6 @@
 import { ErrorObject } from 'ajv';
 import chalk from 'chalk';
+import { PluginErrorData } from '@codifycli/schemas';
 
 import { ResourceConfig } from '../entities/resource-config.js';
 import { SourceMapCache } from '../parser/source-maps.js';
@@ -231,6 +232,24 @@ export class SpawnError extends CodifyError {
   }
 }
 
+export class PluginError extends CodifyError {
+  name = 'PluginError';
+  pluginName: string;
+  resourceType: string;
+  errorData: PluginErrorData;
+
+  constructor(pluginName: string, resourceType: string, errorData: PluginErrorData) {
+    super(errorData.message);
+    this.pluginName = pluginName;
+    this.resourceType = resourceType;
+    this.errorData = errorData;
+  }
+
+  formattedMessage(): string {
+    return this.message;
+  }
+}
+
 export function prettyPrintError(error: unknown): void {
   if (error instanceof CodifyError) {
     return console.error(chalk.red(error.formattedMessage()));

src/common/errors.ts

@@ -0,0 +1,53 @@
+import { ResourceOperation } from '@codifycli/schemas';
+
+import { PluginError } from '../common/errors.js';
+import { ResourcePlan } from './plan.js';
+
+export interface ApplyResultEntry {
+  id: string;
+  operation: ResourceOperation;
+  status: 'success' | 'failed' | 'skipped';
+  error?: PluginError;
+}
+
+export interface ApplyResult {
+  entries: ApplyResultEntry[];
+  errors: PluginError[];
+
+  isPartialFailure(): boolean;
+}
+
+export function createApplyResult(
+  succeededPlans: ResourcePlan[],
+  failedErrors: PluginError[],
+  skippedIds: Set<string>,
+): ApplyResult {
+  const failedByType = new Map(failedErrors.map((e) => [e.resourceType, e]));
+
+  const entries: ApplyResultEntry[] = [
+    ...succeededPlans.map((p) => ({
+      id: p.id,
+      operation: p.operation,
+      status: 'success' as const,
+    })),
+    ...failedErrors.map((e) => ({
+      id: e.resourceType,
+      operation: ResourceOperation.NOOP,
+      status: 'failed' as const,
+      error: e,
+    })),
+    ...[...skippedIds].map((id) => ({
+      id,
+      operation: ResourceOperation.NOOP,
+      status: 'skipped' as const,
+    })),
+  ];
+
+  return {
+    entries,
+    errors: failedErrors,
+    isPartialFailure() {
+      return failedErrors.length > 0;
+    },
+  };
+}

src/entities/apply-result.ts

@@ -54,6 +54,30 @@ export class Plan {
     return this.raw.every((r) => r.operation === ResourceOperation.NOOP);
   }
 
+  computeTransitiveDependents(failedId: string): Set<string> {
+    const reverseDeps = new Map<string, Set<string>>();
+    for (const r of this.project.resourceConfigs) {
+      for (const depId of r.dependencyIds) {
+        if (!reverseDeps.has(depId)) reverseDeps.set(depId, new Set());
+        reverseDeps.get(depId)!.add(r.id);
+      }
+    }
+
+    const toSkip = new Set<string>();
+    const queue = [failedId];
+    while (queue.length > 0) {
+      const current = queue.shift()!;
+      const dependents = reverseDeps.get(current) ?? new Set();
+      for (const dep of dependents) {
+        if (!toSkip.has(dep)) {
+          toSkip.add(dep);
+          queue.push(dep);
+        }
+      }
+    }
+    return toSkip;
+  }
+
   *[Symbol.iterator](): Iterator<ResourcePlan> {
     for (const resource of this.resources) {
       yield resource;

src/entities/plan.ts

@@ -1,7 +1,6 @@
 import { ProcessName, ctx } from '../events/context.js';
 import { DefaultReporter } from '../ui/reporters/default-reporter.js';
 import { Reporter } from '../ui/reporters/reporter.js';
-import { sleep } from '../utils/index.js';
 import { VerbosityLevel } from '../utils/verbosity-level.js';
 import { PlanOrchestrator } from './plan.js';
 
@@ -29,7 +28,7 @@ export const ApplyOrchestrator = {
         return process.exit(0);
       }
     }
-    
+
     const { plan, pluginManager, project } = planResult;
     const filteredPlan = plan.filterNoopResources()
 
@@ -44,14 +43,14 @@ export const ApplyOrchestrator = {
     if (!args.noProgress) ctx.processStarted(ProcessName.APPLY);
     if (!args.noProgress) await reporter.displayProgress();
 
-    await pluginManager.apply(project, filteredPlan);
+    const applyResult = await pluginManager.apply(project, filteredPlan);
+
     if (!args.noProgress) ctx.processFinished(ProcessName.APPLY);
 
-    // Need to sleep to wait for the message to display before we exit
-    await sleep(100);
+    await reporter.displayApplyComplete(applyResult);
 
-    reporter.displayMessage(`
-🎉 Finished applying 🎉
-Open a new terminal or source '.zshrc' for the new changes to be reflected`);
+    if (applyResult.isPartialFailure()) {
+      process.exit(1);
+    }
   },
 };

src/orchestrators/apply.ts

@@ -42,7 +42,7 @@ export class DestroyOrchestrator {
     plan.sortByEvalOrder(project.evaluationOrder);
     destroyProject.removeNoopFromEvaluationOrder(plan);
 
-    reporter.displayPlan(plan);
+    await reporter.displayPlan(plan);
 
     // Short circuit and exit if every change is NOOP
     if (plan.isEmpty()) {
@@ -70,13 +70,15 @@ export class DestroyOrchestrator {
     }
 
     await reporter.displayProgress();
-    await ctx.process(ProcessName.DESTROY, () =>
+    const applyResult = await ctx.process(ProcessName.DESTROY, () =>
       pluginManager.apply(destroyProject, filteredPlan)
     )
 
-    await reporter.displayMessage(`
-🎉 Finished applying 🎉
-Open a new terminal or source '.zshrc' for the new changes to be reflected`);
+    await reporter.displayApplyComplete(applyResult);
+
+    if (applyResult.isPartialFailure()) {
+      process.exit(1);
+    }
   }
 
   /** This method is responsible for generating a plan for specific resources specified by the user */