fix: auto-resolve two code-review bugs — plugin reattach guard (#496) + truncated tool-call corruption (#494) - #503
Draft
TYRMars wants to merge 2 commits into
Draft
fix: auto-resolve two code-review bugs — plugin reattach guard (#496) + truncated tool-call corruption (#494)#503TYRMars wants to merge 2 commits into
TYRMars wants to merge 2 commits into
Conversation
…l sibling skills + MCP servers (#496) `PluginManager.#reattachOne` iterated a plugin's skills with no per-skill error guard, and the MCP-server loop ran after it. A single unreadable or unparseable SKILL.md (e.g. a frontmatter typo introduced after install) threw, aborting the rest of that plugin's reattach — every sibling skill and every MCP server it declares. `reattachInstalled`'s bare `.catch(() => {})` then swallowed the rejection, so the failure was completely silent while `GET /v1/plugins` kept reporting the plugin as fully installed. Wrap the skills-loop body in a per-skill try/catch that skips the bad skill and continues, matching `SkillCatalog.scanRoot` and the MCP loop's own best-effort handling directly below it. A corrupt skill now drops out in isolation; its siblings and the plugin's MCP servers still come up. Adds a regression test asserting that after one skill's frontmatter is corrupted, `reattachInstalled` still registers the sibling skill and the MCP server.
…ated turn can't brick a conversation (#494) `isToolCallTurn` gated tool dispatch on `finish_reason === "tool_calls"`, but the assistant message was pushed into `conversation.messages` unconditionally. When a provider reported `finish_reason: "length"` (OpenAI) / `stop_reason: "max_tokens"` (Anthropic) alongside a non-empty, well-formed `tool_calls` array — reachable when the model hits the output-token cap mid-turn — the loop skipped dispatch and terminated, persisting an assistant message carrying `tool_calls` with zero matching `role:"tool"` replies. Both providers 400 on that history shape, so every later turn and every resume on the conversation failed permanently, with no in-product way to repair it. Rename the guard to `messageHasToolCalls` and drop the finish_reason conjunct: dispatch whenever the assistant turn carries tool calls. A truncated turn (`length`) still ends the loop — but only after its tool calls are answered, so the persisted history is always a valid shape. Any other finish_reason continues so the model can react to the results. Applied to both the blocking (`runWithUsage`) and streaming (`runStream`) paths. Adds regression tests on both paths asserting a length-tagged tool-call turn dispatches the tools, appends the role:"tool" reply, and terminates as length_limited with no dangling tool_calls.
TYRMars
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-resolved by the scheduled issue-triage routine. Two independent, self-contained bug fixes; each is one commit with its own regression tests.
#494 — core
agent.ts: truncated tool-call turn bricks a persisted conversationisToolCallTurngated tool dispatch onfinish_reason === "tool_calls", but the assistant message was pushed intoconversation.messagesunconditionally. When a provider reportedfinish_reason: "length"(OpenAI) /stop_reason: "max_tokens"(Anthropic) alongside a non-empty, well-formedtool_callsarray — reachable when the model hits the output-token cap mid-turn — the loop skipped dispatch and terminated, persisting an assistant message carryingtool_callswith zero matchingrole:"tool"replies. Both providers 400 on that history shape, so every later turn and every resume on the conversation failed permanently, with no in-product way to repair it.Fix: rename the guard to
messageHasToolCallsand drop thefinish_reasonconjunct — dispatch whenever the assistant turn carries tool calls. A truncated turn (length) still ends the loop, but only after its tool calls are answered, so the persisted history is always a valid shape. Any other finish_reason continues so the model can react to the results. Applied to both the blocking (runWithUsage) and streaming (runStream) paths, with regression tests on each.#496 — plugin
manager.ts: one badSKILL.mdsilently kills a plugin's MCP servers + sibling skillsPluginManager.#reattachOneiterated a plugin's skills with no per-skill error guard, and the MCP-server loop ran after it. A single unreadable / unparseableSKILL.md(e.g. a frontmatter typo introduced after install) threw, aborting the rest of that plugin's reattach — every sibling skill and every MCP server it declares.reattachInstalled's bare.catch(() => {})swallowed it, so the failure was completely silent whileGET /v1/pluginskept reporting the plugin as fully installed.Fix: wrap the skills-loop body in a per-skill
try/catchthat skips the bad skill and continues — matchingSkillCatalog.scanRootand the MCP loop's own best-effort handling directly below it. A corrupt skill now drops out in isolation; its siblings and the plugin's MCP servers still come up. Regression test included.Testing
@jarvis/core: 15/15 tests pass (2 new), typecheck clean, eslint clean.@jarvis/plugin: 15/15 tests pass (1 new), typecheck clean, eslint clean.🤖 Generated with Claude Code