From 7f6f7bca6e7b798e321d38cd3f99eac0afee4230 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 01:26:07 +0000 Subject: [PATCH 1/2] fix(plugin): guard per-skill reattach + surface degraded plugins (#496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PluginManager.#reattachOne` iterated a plugin's skills with no per-skill error guard, and the MCP-server loop ran after it. One unreadable or unparseable SKILL.md threw, aborting the rest of that plugin's reattach — including every MCP server it declared — and `reattachInstalled`'s bare `.catch(() => {})` swallowed the rejection, so an entire MCP integration plus all sibling skills vanished silently on restart while `GET /v1/plugins` kept reporting the plugin as fully installed. - Wrap each skill's load/parse/insert in a per-skill try/catch that records the failure and continues, matching `SkillCatalog.scanRoot` and the MCP loop below it. - Record (rather than silently swallow) per-MCP add failures too. - Add `degraded` / `last_error` fields to the ledger entry, set at reattach time (and cleared on a clean reattach), surfaced through `list()` / `get()` so `GET /v1/plugins` no longer reports a half-attached plugin as healthy. - `reattachInstalled` now records whole-plugin failures (e.g. a corrupt plugin.json) on the entry instead of dropping them on the floor. Adds regression tests: one bad SKILL.md no longer takes down sibling skills or the MCP server, the plugin is marked degraded, and a subsequent clean reattach clears the marker. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Pdj9CWcTPhxPMi559vJpDz --- apps/jarvis-web/src/services/plugins.ts | 4 ++ packages/plugin/src/manager.test.ts | 74 ++++++++++++++++++++++++ packages/plugin/src/manager.ts | 77 ++++++++++++++++++++----- 3 files changed, 141 insertions(+), 14 deletions(-) diff --git a/apps/jarvis-web/src/services/plugins.ts b/apps/jarvis-web/src/services/plugins.ts index d2e47044..59c57c60 100644 --- a/apps/jarvis-web/src/services/plugins.ts +++ b/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 { diff --git a/packages/plugin/src/manager.test.ts b/packages/plugin/src/manager.test.ts index 680df2af..c78dac94 100644 --- a/packages/plugin/src/manager.test.ts +++ b/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 })); diff --git a/packages/plugin/src/manager.ts b/packages/plugin/src/manager.ts index 59e315f6..a37957d0 100644 --- a/packages/plugin/src/manager.ts +++ b/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 { + // 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("; "); } } From e81da51243800e0b784f79ca1bc983ed20a79507 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 01:31:34 +0000 Subject: [PATCH 2/2] fix(router,core): attribute usage to the routed model, not the pre-routing one (#495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under JARVIS_ROUTER_ENABLED, RoutingProvider rewrites ChatRequest.model before delegating, but the agent loop stamped every `usage` event with the static `config.model`. So all token usage from routed turns was attributed to the pre-routing model, and the web UsagePanel priced it from a table keyed by that wrong name — under-reporting cost by up to two orders of magnitude with no anomaly visible (the expensive tier showed zero usage, and the "unknown model → partial estimate" safety valve never fired because the stale name is a valid known one). Propagate the effective model back through the provider seam: - Add an optional `model` to the `usage` LlmChunk variant and to ChatResponse (absent for plain providers). - RoutingProvider tags the streamed usage chunk and the blocking response with the resolved tier target — using `?? existing` so a downstream that reports its own model is respected. - agent.ts prefers `chunk.model ?? this.config.model`, so non-routing providers are unchanged while routed usage is attributed correctly. Adds regression tests: streamed + blocking routed usage carries the tier target model, and a downstream-set model is not overwritten. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Pdj9CWcTPhxPMi559vJpDz --- packages/core/src/agent.ts | 6 ++- packages/core/src/llm.ts | 10 +++- packages/router/src/provider.ts | 21 +++++--- packages/router/src/router.test.ts | 82 ++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 8 deletions(-) diff --git a/packages/core/src/agent.ts b/packages/core/src/agent.ts index 40f8d178..0c2360d5 100644 --- a/packages/core/src/agent.ts +++ b/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, diff --git a/packages/core/src/llm.ts b/packages/core/src/llm.ts index ba91ed49..249aa7b5 100644 --- a/packages/core/src/llm.ts +++ b/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 { diff --git a/packages/router/src/provider.ts b/packages/router/src/provider.ts index 9da5177c..c16f6b6a 100644 --- a/packages/router/src/provider.ts +++ b/packages/router/src/provider.ts @@ -137,11 +137,15 @@ 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> { @@ -149,22 +153,27 @@ export class RoutingProvider implements LlmProvider { 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, name: string, + model: string, ): AsyncGenerator { 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; } diff --git a/packages/router/src/router.test.ts b/packages/router/src/router.test.ts index d1720bf0..33d122af 100644 --- a/packages/router/src/router.test.ts +++ b/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 { + return Promise.resolve({ + message: assistantText(`${this.tag}:${req.model}`), + finish_reason: "stop", + usage: { prompt_tokens: 40000, completion_tokens: 6000 }, + }); + } + completeStream(req: ChatRequest): AsyncIterable { + const tag = this.tag; + const model = req.model; + return (async function* (): AsyncGenerator { + 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([["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([["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 { + yield { type: "usage", usage: { prompt_tokens: 1 }, model: "explicit-model" }; + yield { type: "finish", message: assistantText("x"), finish_reason: "stop" }; + })(), + }; + const map = new Map([["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 () => {