fix: auto-resolve two bug reports — plugin reattach isolation (#496) + routed-model usage attribution (#495)

apps/jarvis-web/src/services/plugins.ts

@@ -15,6 +15,10 @@ export interface InstalledPlugin {
   installed_at: string;
   skill_names: string[];
   mcp_prefixes: string[];
+  /** Set when the last reattach couldn't fully restore the plugin. Absent = healthy. */
+  degraded?: boolean;
+  /** Human-readable summary of the reattach failure, present only when `degraded`. */
+  last_error?: string;
 }
 
 export interface PluginInstallReport {

packages/core/src/agent.ts

@@ -155,7 +155,11 @@ export class Agent {
           if (chunk.type === "content_delta") {
             yield { type: "delta", content: chunk.content };
           } else if (chunk.type === "usage") {
-            yield { type: "usage", model: this.config.model, ...chunk.usage };
+            // Prefer the model the request actually ran on (set by wrappers that
+            // rewrite the model mid-flight, e.g. RoutingProvider) so usage is
+            // attributed and priced against the right model — falling back to
+            // the static request model for plain providers.
+            yield { type: "usage", model: chunk.model ?? this.config.model, ...chunk.usage };
           } else if (chunk.type === "finish") {
             finish = {
               message: chunk.message,

packages/core/src/llm.ts

@@ -71,6 +71,14 @@ export interface ChatResponse {
   finish_reason: FinishReason;
   response_id?: string | null;
   usage?: Usage;
+  /**
+   * Concrete model that produced `usage`, when it differs from the request's
+   * `model`. Set by wrappers that rewrite the model mid-flight (e.g.
+   * `RoutingProvider`) so usage is attributed to the model that actually ran,
+   * not the pre-routing name. Absent for plain providers — callers fall back to
+   * the request model.
+   */
+  model?: string;
 }
 
 /**
@@ -87,7 +95,7 @@ export type LlmChunk =
       name?: string;
       arguments_fragment?: string;
     }
-  | { type: "usage"; usage: Usage }
+  | { type: "usage"; usage: Usage; model?: string }
   | { type: "finish"; message: Message; finish_reason: FinishReason; response_id?: string | null };
 
 export interface LlmProvider {

packages/plugin/src/manager.test.ts

@@ -325,6 +325,80 @@ test("reattachInstalled re-registers skills and mcp from the ledger", async (t)
   assert.ok(mcp.slots.has("gh"));
 });
 
+test("reattach isolates one bad SKILL.md — sibling skills + MCP still come up, plugin marked degraded", async (t) => {
+  const staging = await makeTmp();
+  t.after(() => rm(staging, { recursive: true, force: true }));
+
+  const pluginSrc = path.join(staging, "src");
+  await writePlugin(
+    pluginSrc,
+    `{
+      "name": "demo", "version": "0.1.0", "description": "x",
+      "skills": ["skills/a", "skills/b"],
+      "mcp_servers": { "gh": { "transport": { "type": "stdio", "command": "uvx" } } }
+    }`,
+  );
+  await writeSkill(path.join(pluginSrc, "skills"), "a", "name: a\ndescription: y\n", "A.");
+  await writeSkill(path.join(pluginSrc, "skills"), "b", "name: b\ndescription: y\n", "B.");
+
+  const installRoot = path.join(staging, "plugins");
+  await (await PluginManager.open(installRoot, SkillCatalog.empty(), new FakeMcp())).installFromPath(
+    pluginSrc,
+  );
+
+  // The operator corrupts the *installed* copy of skill `a` (unknown frontmatter
+  // field → parseSkill throws) — exactly the trigger from the bug report.
+  const badSkillMd = path.join(installRoot, "demo", "skills", "a", "SKILL.md");
+  await writeFile(badSkillMd, "---\nname: a\ndescription: y\nbogus: true\n---\nA.");
+
+  const cat = SkillCatalog.empty();
+  const mcp = new FakeMcp();
+  const mgr = await PluginManager.open(installRoot, cat, mcp);
+  await mgr.reattachInstalled();
+
+  // The bad skill must NOT take down its siblings or the MCP server.
+  assert.equal(cat.get("a"), undefined, "corrupt skill a is skipped");
+  assert.ok(cat.get("b"), "sibling skill b still registered");
+  assert.ok(mcp.slots.has("gh"), "MCP server still connected despite the bad skill");
+
+  // The failure is surfaced, not silent: the ledger entry reports degraded.
+  const listed = await mgr.list();
+  assert.equal(listed[0]?.degraded, true);
+  assert.match(listed[0]?.last_error ?? "", /skill skills\/a/);
+});
+
+test("reattach clears a stale degraded marker once the skill is fixed", async (t) => {
+  const staging = await makeTmp();
+  t.after(() => rm(staging, { recursive: true, force: true }));
+
+  const pluginSrc = path.join(staging, "src");
+  await writePlugin(
+    pluginSrc,
+    `{ "name": "demo", "version": "0.1.0", "description": "x", "skills": ["skills/a"] }`,
+  );
+  await writeSkill(path.join(pluginSrc, "skills"), "a", "name: a\ndescription: y\n", "A.");
+
+  const installRoot = path.join(staging, "plugins");
+  await (await PluginManager.open(installRoot, SkillCatalog.empty(), new FakeMcp())).installFromPath(
+    pluginSrc,
+  );
+
+  const badSkillMd = path.join(installRoot, "demo", "skills", "a", "SKILL.md");
+  const good = "---\nname: a\ndescription: y\n---\nA.";
+  await writeFile(badSkillMd, "---\nname: a\ndescription: y\nbogus: true\n---\nA.");
+
+  const mgr = await PluginManager.open(installRoot, SkillCatalog.empty(), new FakeMcp());
+  await mgr.reattachInstalled();
+  assert.equal((await mgr.list())[0]?.degraded, true);
+
+  // Operator repairs the file; a subsequent reattach clears the marker.
+  await writeFile(badSkillMd, good);
+  await mgr.reattachInstalled();
+  const listed = await mgr.list();
+  assert.notEqual(listed[0]?.degraded, true);
+  assert.equal(listed[0]?.last_error, undefined);
+});
+
 test("install rejects a directory without plugin.json", async (t) => {
   const staging = await makeTmp();
   t.after(() => rm(staging, { recursive: true, force: true }));

packages/plugin/src/manager.ts

@@ -105,6 +105,19 @@ export interface InstalledPlugin {
   skill_names: string[];
   /** MCP prefixes the plugin owns, so uninstall can drop them. */
   mcp_prefixes: string[];
+  /**
+   * True when the last {@link PluginManager.reattachInstalled} could not fully
+   * restore this plugin (a bad `SKILL.md`, an unreadable `plugin.json`, or a
+   * failed MCP add). Set/cleared at reattach time so `GET /v1/plugins` stops
+   * reporting a half-attached plugin as fully healthy. Absent = healthy.
+   */
+  degraded?: boolean;
+  /**
+   * Human-readable summary of what went wrong during the last reattach, present
+   * only when {@link degraded} is true. Semicolon-joined per-skill / per-MCP
+   * failures, or the whole-plugin error (e.g. a corrupt manifest).
+   */
+  last_error?: string;
 }
 
 /**
@@ -359,12 +372,27 @@ export class PluginManager {
     return this.#runExclusive(async () => {
       const entries = [...this.#ledger.values()];
       for (const entry of entries) {
-        await this.#reattachOne(entry).catch(() => {});
+        try {
+          await this.#reattachOne(entry);
+        } catch (e) {
+          // Whole-plugin failure that even the per-skill / per-MCP guards
+          // inside #reattachOne can't localise — an unreadable or unparseable
+          // `plugin.json`. Record it on the ledger entry so `GET /v1/plugins`
+          // surfaces the degradation instead of reporting the plugin as
+          // healthy (its skills / MCP servers are all absent from live state).
+          entry.degraded = true;
+          entry.last_error = errText(e);
+        }
       }
     });
   }
 
   async #reattachOne(entry: InstalledPlugin): Promise<void> {
+    // A clean reattach makes the plugin healthy again — clear any stale marker
+    // from a previous run (e.g. the operator fixed the broken SKILL.md).
+    entry.degraded = false;
+    delete entry.last_error;
+
     const manifestPath = path.join(entry.install_dir, "plugin.json");
     let text: string;
     try {
@@ -374,24 +402,45 @@ export class PluginManager {
     }
     const manifest: PluginManifest = parsePluginManifest(text);
 
-    // Skills: re-load each one and insert.
+    // Accumulate localised failures so a single bad skill / MCP server can't
+    // abort the rest of the plugin, yet the degradation is still reported.
+    const failures: string[] = [];
+
+    // Skills: re-load each one and insert. A per-skill guard (matching
+    // `SkillCatalog.scanRoot` and the MCP loop below) keeps one unreadable /
+    // unparseable `SKILL.md` from aborting the plugin's sibling skills AND its
+    // MCP servers, which used to vanish silently.
     for (const rel of manifest.skills) {
-      const abs = path.join(entry.install_dir, rel);
-      const skillMd = (await isDir(abs)) ? path.join(abs, "SKILL.md") : abs;
-      const raw = await readFile(skillMd, "utf8");
-      const parsed = parseSkill(raw);
-      this.#skills.insert({
-        manifest: parsed.manifest,
-        body: parsed.body,
-        path: skillMd,
-        source: "plugin",
-      });
+      try {
+        const abs = path.join(entry.install_dir, rel);
+        const skillMd = (await isDir(abs)) ? path.join(abs, "SKILL.md") : abs;
+        const raw = await readFile(skillMd, "utf8");
+        const parsed = parseSkill(raw);
+        this.#skills.insert({
+          manifest: parsed.manifest,
+          body: parsed.body,
+          path: skillMd,
+          source: "plugin",
+        });
+      } catch (e) {
+        failures.push(`skill ${rel}: ${errText(e)}`);
+      }
     }
 
-    // MCP: re-add each server. Failures are swallowed so the rest come up.
+    // MCP: re-add each server. Failures are best-effort (so the rest come up)
+    // but now recorded rather than silently swallowed.
     for (const [prefix, cfg] of Object.entries(manifest.mcp_servers)) {
       const cloned: McpClientConfig = { ...cfg, prefix };
-      await this.#mcp.add(cloned).catch(() => {});
+      try {
+        await this.#mcp.add(cloned);
+      } catch (e) {
+        failures.push(`mcp ${prefix}: ${errText(e)}`);
+      }
+    }
+
+    if (failures.length > 0) {
+      entry.degraded = true;
+      entry.last_error = failures.join("; ");
     }
   }
 

packages/router/src/provider.ts

@@ -137,34 +137,43 @@ export class RoutingProvider implements LlmProvider {
     const { provider, name, model } = await this.#decide(req);
     const routed = this.#rewrite(req, name, model);
     const resp = await provider.complete(routed);
+    // Attribute usage to the model that actually ran (unless the downstream
+    // already reported one), so cost estimates aren't priced against the
+    // pre-routing model.
+    const withModel = resp.model != null ? resp : { ...resp, model };
     // Stamp the id we hand back so the next turn can detect a switch.
-    if (resp.response_id != null) {
-      return { ...resp, response_id: stampResponseId(name, resp.response_id) };
+    if (withModel.response_id != null) {
+      return { ...withModel, response_id: stampResponseId(name, withModel.response_id) };
     }
-    return resp;
+    return withModel;
   }
 
   async completeStream(req: ChatRequest): Promise<AsyncIterable<LlmChunk>> {
     const { provider, name, model } = await this.#decide(req);
     const routed = this.#rewrite(req, name, model);
     const inner = await provider.completeStream(routed);
     // Mirror the blocking path: re-stamp the terminal Finish's response_id with
-    // the routed provider so chaining stays correct.
-    return restampStream(inner, name);
+    // the routed provider so chaining stays correct, and tag the usage chunk
+    // with the routed model so token usage is attributed to the model that ran.
+    return restampStream(inner, name, model);
   }
 }
 
 /**
  * Re-stamp the terminal `finish` chunk's `response_id` with the routed
- * provider name, forwarding every other chunk untouched.
+ * provider name, tag the `usage` chunk with the routed `model` (unless the
+ * downstream already set one), and forward every other chunk untouched.
  */
 async function* restampStream(
   inner: AsyncIterable<LlmChunk>,
   name: string,
+  model: string,
 ): AsyncGenerator<LlmChunk> {
   for await (const chunk of inner) {
     if (chunk.type === "finish" && chunk.response_id != null) {
       yield { ...chunk, response_id: stampResponseId(name, chunk.response_id) };
+    } else if (chunk.type === "usage" && chunk.model == null) {
+      yield { ...chunk, model };
     } else {
       yield chunk;
     }

packages/router/src/router.test.ts

@@ -260,6 +260,88 @@ test("classification reads the last USER message, ignoring a trailing tool resul
   assert.equal(anthropic.lastModel, "claude-opus");
 });
 
+// ---------- RoutingProvider: usage model attribution (#495) ----------
+
+/** A provider whose stream emits a `usage` chunk, to exercise model tagging. */
+class UsageProvider implements LlmProvider {
+  readonly tag: string;
+  constructor(tag: string) {
+    this.tag = tag;
+  }
+  complete(req: ChatRequest): Promise<ChatResponse> {
+    return Promise.resolve({
+      message: assistantText(`${this.tag}:${req.model}`),
+      finish_reason: "stop",
+      usage: { prompt_tokens: 40000, completion_tokens: 6000 },
+    });
+  }
+  completeStream(req: ChatRequest): AsyncIterable<LlmChunk> {
+    const tag = this.tag;
+    const model = req.model;
+    return (async function* (): AsyncGenerator<LlmChunk> {
+      yield { type: "usage", usage: { prompt_tokens: 40000, completion_tokens: 6000 } };
+      yield { type: "finish", message: assistantText(`${tag}:${model}`), finish_reason: "stop" };
+    })();
+  }
+}
+
+test("streamed usage chunk is tagged with the routed model, not the request model (#495)", async () => {
+  const anthropic = new UsageProvider("anthropic");
+  const map = new Map<string, LlmProvider>([["anthropic", anthropic]]);
+  const cfg = new RouterConfig(modelRef("openai", "gpt-4o-mini")).withTier(
+    "complex",
+    modelRef("anthropic", "claude-opus-4"),
+  );
+  const rp = new RoutingProvider(new RecordingProvider("fb"), map, cfg, new FixedClassifier("complex"));
+
+  const stream = await rp.completeStream({ model: "gpt-4o-mini", messages: [] });
+  const usage = [];
+  for await (const chunk of stream) if (chunk.type === "usage") usage.push(chunk);
+  assert.equal(usage.length, 1);
+  // The tokens ran on Opus after routing — usage must carry that, so the web
+  // UsagePanel doesn't price 46k Opus tokens at gpt-4o-mini rates.
+  assert.equal(usage[0]?.model, "claude-opus-4");
+});
+
+test("blocking complete stamps the routed model onto the response (#495)", async () => {
+  const anthropic = new UsageProvider("anthropic");
+  const map = new Map<string, LlmProvider>([["anthropic", anthropic]]);
+  const cfg = new RouterConfig(modelRef("openai", "gpt-4o-mini")).withTier(
+    "complex",
+    modelRef("anthropic", "claude-opus-4"),
+  );
+  const rp = new RoutingProvider(new RecordingProvider("fb"), map, cfg, new FixedClassifier("complex"));
+
+  const resp = await rp.complete({ model: "gpt-4o-mini", messages: [] });
+  assert.equal(resp.model, "claude-opus-4");
+});
+
+test("router does not overwrite a model a downstream already reported on usage", async () => {
+  // A downstream that pre-sets usage.model must be respected (?? semantics).
+  const inner: LlmProvider = {
+    complete: (_req) =>
+      Promise.resolve({ message: assistantText("x"), finish_reason: "stop", model: "explicit-model" }),
+    completeStream: (_req) =>
+      (async function* (): AsyncGenerator<LlmChunk> {
+        yield { type: "usage", usage: { prompt_tokens: 1 }, model: "explicit-model" };
+        yield { type: "finish", message: assistantText("x"), finish_reason: "stop" };
+      })(),
+  };
+  const map = new Map<string, LlmProvider>([["anthropic", inner]]);
+  const cfg = new RouterConfig(modelRef("openai", "gpt-4o-mini")).withTier(
+    "complex",
+    modelRef("anthropic", "claude-opus-4"),
+  );
+  const rp = new RoutingProvider(new RecordingProvider("fb"), map, cfg, new FixedClassifier("complex"));
+
+  const resp = await rp.complete({ model: "gpt-4o-mini", messages: [] });
+  assert.equal(resp.model, "explicit-model");
+  const stream = await rp.completeStream({ model: "gpt-4o-mini", messages: [] });
+  for await (const chunk of stream) {
+    if (chunk.type === "usage") assert.equal(chunk.model, "explicit-model");
+  }
+});
+
 // ---------- RoutingProvider: chain-handle reconciliation ----------
 
 test("same provider preserves + unstamps the chain handle, re-stamps the reply", async () => {