From 9cc831d3ddf966f264889c01c9280a9d54c13cc3 Mon Sep 17 00:00:00 2001 From: ZhouChuange Date: Tue, 26 May 2026 16:16:27 +1000 Subject: [PATCH 1/6] fix(providers): enforce DeepSeek thinking-mode reasoning_content invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeepSeek's thinking-mode protocol requires that once ANY assistant message in history carries a non-empty 'reasoning_content', every subsequent assistant message must also carry one — otherwise the API returns: 400 'reasoning_content in the thinking mode must be passed back to the API' This hit users whenever: 1) A synthetic skill_invoke -> read_file assistant message was injected mid-turn after a real thinking-mode assistant message (agent-loop.js injectSyntheticSkillRead). 2) The force-final-summary fallback at iteration-limit produced an assistant message that lost its thoughts stream. Changes: - agent-loop.js: synthetic skill-read assistant now carries a short reasoning_content; force-final-summary captures tailThoughts and attaches them so reload/round-trip never lands on a reasoning-less assistant. - providers/index.js sanitizeMessages: defence-in-depth backfill. Once any assistant in history has reasoning_content, every other assistant without one gets a short placeholder. The pass is scoped to reasoning-capable models only and runs after the strip pass. --- src/chat/agent-loop.js | 21 ++++++++++++-- src/providers/index.js | 62 ++++++++++++++++++++++++++++++++---------- 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/src/chat/agent-loop.js b/src/chat/agent-loop.js index 6eaafb0..111a729 100644 --- a/src/chat/agent-loop.js +++ b/src/chat/agent-loop.js @@ -57,6 +57,15 @@ function injectSyntheticSkillRead(messages, skillName, body, skillPath) { messages.push({ role: 'assistant', content: null, + // DeepSeek thinking-mode quirk: once any assistant message in history + // carries reasoning_content, every subsequent assistant message MUST + // also carry it, or the API returns 400 ("reasoning_content in the + // thinking mode must be passed back to the API"). Skill injection + // happens mid-turn (right after a real thinking-mode assistant + // message), so we must include a non-empty reasoning_content here. + // Sanitization (sanitizeMessages in providers/index.js) preserves + // this field for reasoning-capable models, so it survives to the API. + reasoning_content: `Loading skill SOP "${safeName}" via read_file to obtain its instructions before proceeding.`, tool_calls: [{ id: callId, type: 'function', @@ -924,15 +933,23 @@ class AgentLoop { { role: 'user', content: '\nYou have reached the tool-call iteration limit without producing a user-facing answer. Stop calling tools. Write a concise plain-text reply that: (1) summarises what you tried, (2) states what you found or could not find, (3) suggests a concrete next step the user can take.\n' }, ]; let tail = ''; + let tailThoughts = ''; await streamChat( { provider, apiKey, baseUrl, messages: finalMsgs, model, noTools: true }, { onDelta: t => { tail += t; run.reply.asst += t; this._postToRun(run, { type: 'replyDelta', text: t }); }, - onThinking: t => { run.reply.thoughts += t; this._postToRun(run, { type: 'thinkingDelta', text: t }); }, + onThinking: t => { tailThoughts += t; run.reply.thoughts += t; this._postToRun(run, { type: 'thinkingDelta', text: t }); }, }, signal, ).catch(e => Logger.info('FORCE_FINAL_SUMMARY_ERROR', { message: e.message })); - if (tail) run.messages.push({ role: 'assistant', content: tail }); + if (tail) run.messages.push({ + role: 'assistant', + content: tail, + // Preserve thinking output so the persisted/reloaded history + // does not end with a reasoning-less assistant message, which + // would 400 on the next turn under DeepSeek thinking mode. + ...(tailThoughts ? { reasoning_content: tailThoughts } : {}), + }); } } catch (e) { // Restore the last known-good message state to prevent persisting a diff --git a/src/providers/index.js b/src/providers/index.js index b2fc189..3201728 100644 --- a/src/providers/index.js +++ b/src/providers/index.js @@ -153,27 +153,61 @@ function listModels(providerId) { * Exception: `reasoning_content` is never stripped for reasoning-capable models * (capabilities.reasoning === true) because DeepSeek's API requires it to be * passed back in subsequent turns — omitting it causes HTTP 400. + * + * Additionally, for reasoning-capable models we enforce an invariant required + * by DeepSeek's thinking-mode protocol: once any assistant message in history + * carries a non-empty `reasoning_content`, EVERY subsequent assistant message + * MUST also carry one — otherwise the API rejects the request with + * 400 "reasoning_content in the thinking mode must be passed back to the API". + * We backfill a short placeholder on any assistant message that is missing it. + * This is a defence-in-depth net: callers should still attach the real thought + * stream when they have one, but if they forget (or the model returned an empty + * thought, or a synthetic assistant was injected mid-turn) we won't 400. */ +const REASONING_PLACEHOLDER = '(no thoughts surfaced for this step)'; + function sanitizeMessages(providerId, messages, modelId) { if (!Array.isArray(messages) || !messages.length) return messages; const strip = getProvider(providerId)?.quirks?.stripInputFields; - if (!Array.isArray(strip) || !strip.length) return messages; - // Reasoning-capable models MUST have reasoning_content passed back to the - // API; strip every other declared field but skip reasoning_content for them. const isReasoning = !!(modelId && getModel(providerId, modelId)?.capabilities?.reasoning); - const effectiveStrip = isReasoning ? strip.filter(f => f !== 'reasoning_content') : strip; - if (!effectiveStrip.length) return messages; - return messages.map(m => { - if (!m || typeof m !== 'object') return m; - let copy = m; - for (const k of effectiveStrip) { - if (Object.prototype.hasOwnProperty.call(copy, k)) { - if (copy === m) copy = Object.assign({}, m); - delete copy[k]; + // For reasoning-capable models, strip every declared field except + // reasoning_content (which must round-trip). + const effectiveStrip = Array.isArray(strip) + ? (isReasoning ? strip.filter(f => f !== 'reasoning_content') : strip) + : []; + + let out = messages; + if (effectiveStrip.length) { + out = out.map(m => { + if (!m || typeof m !== 'object') return m; + let copy = m; + for (const k of effectiveStrip) { + if (Object.prototype.hasOwnProperty.call(copy, k)) { + if (copy === m) copy = Object.assign({}, m); + delete copy[k]; + } } + return copy; + }); + } + + // Reasoning-mode invariant backfill. Only kicks in once at least one + // assistant message already carries reasoning_content — that's the + // moment DeepSeek starts enforcing the "every assistant has it" rule. + if (isReasoning) { + const anyHasReasoning = out.some( + m => m && m.role === 'assistant' && typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0, + ); + if (anyHasReasoning) { + out = out.map(m => { + if (!m || m.role !== 'assistant') return m; + if (typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0) return m; + return Object.assign({}, m, { reasoning_content: REASONING_PLACEHOLDER }); + }); } - return copy; - }); + } + + return out; } /** From 9fb8be5ae57d300a58c38b7e2bfc27900cb06a9a Mon Sep 17 00:00:00 2001 From: ZhouChuange Date: Tue, 26 May 2026 16:16:38 +1000 Subject: [PATCH 2/6] fix(skills): suppress skill-creator hard gate when meta-skill not installed (#146 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #146 introduced a 'HARD GATE' requiring skill_invoke({name:'skill-creator'}) before any skill_create call. In environments where no skill-creator meta-skill is actually installed, the model still followed the instruction and fabricated a skill_invoke for the non-existent skill, producing a confusing: Error: skill 'skill-creator' not found. Available: ... Three coordinated changes silence the false positive: - prompts/system.js: only emit the gate paragraph if discoverSkills() reports a name in the {skill-creator, skill_creator, skillcreator} variant set is actually installed in the current workspace. - tools/schema.js: soften skill_create description from 'HARD GATE' to 'Quality gate — ONLY if a meta-skill named ... is listed in the Available skills index'; explicitly tells the model not to fabricate the invoke when the gate is inactive. - tools/skill-tools.js: when the model still fabricates a call to one of the variant names, return a soft Notice (gate inactive, you may proceed) instead of an error. Executor-side gate is already a no-op when no meta-skill is installed, so this is consistent end-to-end. --- src/prompts/system.js | 19 ++++++++++++++++--- src/tools/schema.js | 2 +- src/tools/skill-tools.js | 10 ++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/prompts/system.js b/src/prompts/system.js index 5672f81..ba6cd74 100644 --- a/src/prompts/system.js +++ b/src/prompts/system.js @@ -269,6 +269,21 @@ function readSkillIndex() { // Issue #61 — Step 6 + Step 7. function getProblemSolvingParadigm() { + // Issue #146 — only mention the skill-creator hard gate when a meta-skill + // is actually installed. Otherwise the model fabricates a skill_invoke + // call to a non-existent "skill-creator", producing a confusing error. + let skillCreatorInstalled = false; + try { + const { discoverSkills } = require('../skills'); + const all = discoverSkills(wsRoot()); + const variants = new Set(['skill-creator', 'skill_creator', 'skillcreator']); + skillCreatorInstalled = all.some(s => variants.has(String(s && s.name || '').toLowerCase())); + } catch { /* ignore — treat as not installed */ } + + const gateParagraph = skillCreatorInstalled + ? `\n\n**Issue #146 — skill_create quality gate**: A meta-skill matching \`skill-creator\` (or variant) is installed locally. You MUST call \`skill_invoke({ name: "" })\` in the SAME turn BEFORE you call \`skill_create\`. The meta-skill performs description tightening, body structure check, and dedup. Calling \`skill_create\` without it will be rejected by the tool layer.` + : ''; + return `# Problem-solving paradigm For any non-trivial task, follow this loop: @@ -284,9 +299,7 @@ For any non-trivial task, follow this loop: Skills (\`skill_invoke\` / \`skill_create\`) capture reasoned, on-demand playbooks. Hooks (\`hooks.json\`) capture deterministic reflexes. Do not conflate them. -Never call \`skill_create\` for one-off fixes, trivial tasks, or before the user has confirmed the solution works. - -**Issue #146 — skill_create quality gate**: if a skill matching the \`skill-creator\` meta-skill name (or common variants: \`skill_creator\`, \`skillcreator\`) appears in the Available skills index above, you MUST call \`skill_invoke({ name: "" })\` in the SAME turn BEFORE you call \`skill_create\`. The meta-skill performs description tightening, body structure check, and dedup. Calling \`skill_create\` without it will be rejected by the tool layer. Do NOT treat \`skill_create\` as "just a file write" — creation goes through review first.`; +Never call \`skill_create\` for one-off fixes, trivial tasks, or before the user has confirmed the solution works.${gateParagraph}`; } // ---------- workspace instructions (lazy, opt-in) ---------- diff --git a/src/tools/schema.js b/src/tools/schema.js index 5d3bc75..ce9c6ba 100644 --- a/src/tools/schema.js +++ b/src/tools/schema.js @@ -130,7 +130,7 @@ const TOOL_DEFS = [ type: 'function', function: { name: 'skill_create', - description: 'Persist a reusable multi-step workflow as a new skill on disk. Use ONLY when ALL of these are true: (1) you have just completed a non-trivial multi-step task, (2) the user has explicitly confirmed the result is correct, (3) the workflow has ≥3 concrete steps that span multiple tools and is likely to recur. Do NOT use for one-off fixes, trivial edits, single-step tasks, before user confirmation, or to record preferences (those belong in ~/.deepcopilot/memory.md) or project facts (those belong in /DEEPCOPILOT.md). When the SOP was synthesized from web research, set source to "web" or "hybrid" — the skill will be marked untrusted automatically. **HARD GATE (Issue #146)**: if a meta-skill whose name is one of `skill-creator`, `skill_creator`, or `skillcreator` is installed locally (see the "Available skills" index in the system prompt), you MUST call `skill_invoke` earlier in the SAME turn with `name` set to the EXACT installed spelling (whichever of the three appears in the index) before calling skill_create; invoking a variant that is NOT actually installed does NOT satisfy the gate. The meta-skill performs description tightening, body structure check, and dedup. If no such skill is installed the call is allowed through with a warning prepended to the result.', + description: 'Persist a reusable multi-step workflow as a new skill on disk. Use ONLY when ALL of these are true: (1) you have just completed a non-trivial multi-step task, (2) the user has explicitly confirmed the result is correct, (3) the workflow has ≥3 concrete steps that span multiple tools and is likely to recur. Do NOT use for one-off fixes, trivial edits, single-step tasks, before user confirmation, or to record preferences (those belong in ~/.deepcopilot/memory.md) or project facts (those belong in /DEEPCOPILOT.md). When the SOP was synthesized from web research, set source to "web" or "hybrid" — the skill will be marked untrusted automatically. **Quality gate (Issue #146)**: ONLY if a meta-skill named `skill-creator`, `skill_creator`, or `skillcreator` is listed in the "Available skills" index of the current system prompt, you MUST call `skill_invoke` earlier in the SAME turn with `name` set to that exact installed spelling before calling skill_create. If no such meta-skill appears in the index, do NOT fabricate a `skill_invoke` call for it — just call `skill_create` directly; the gate is inactive in that environment.', parameters: { type: 'object', properties: { diff --git a/src/tools/skill-tools.js b/src/tools/skill-tools.js index 54fd6ce..3bf02cc 100644 --- a/src/tools/skill-tools.js +++ b/src/tools/skill-tools.js @@ -30,6 +30,16 @@ function skillInvoke(args, run) { const all = discoverSkills(wsRoot()); const s = all.find(x => x.name === name); if (!s) { + // Issue #146 follow-up: when the model fabricates a call to a + // skill-creator meta-skill that isn't actually installed, return a + // soft notice instead of an error so the agent can simply proceed + // with `skill_create` (the executor-side gate is a no-op when no + // meta-skill is installed). This prevents the confusing + // "skill 'skill-creator' not found" failure users frequently see. + const variants = new Set(['skill-creator', 'skill_creator', 'skillcreator']); + if (variants.has(name.toLowerCase())) { + return 'Notice: no skill-creator meta-skill is installed in this environment. The skill_create quality gate is inactive here — you may proceed to call `skill_create` directly when appropriate.'; + } const known = all.map(x => x.name).join(', ') || '(none installed)'; return `Error: skill "${name}" not found. Available: ${known}`; } From 75a30c9853f07859c5796d88aa1ceb2b7a85d8cb Mon Sep 17 00:00:00 2001 From: ZhouChuange Date: Tue, 26 May 2026 16:30:10 +1000 Subject: [PATCH 3/6] fix(agent): address Copilot review feedback on PR #164 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four targeted refinements to the previous two commits, all from Copilot's PR #164 review: 1. agent-loop.js (injectSyntheticSkillRead): only attach reasoning_content when history already shows thinking mode is live. Previously the synthetic value was added unconditionally, which on non-DeepSeek providers (a) sent an unknown field to OpenAI-compatible endpoints, and (b) artificially activated the providers/index.js backfill. The new gate ('has any prior assistant carried non-empty reasoning_content?') exactly matches the moment DeepSeek's API starts enforcing the round-trip rule. 2. providers/index.js (sanitizeMessages): narrow the backfill gate from isReasoning alone to isReasoning AND stripInputFields.includes('reasoning_content'). OpenAI / Anthropic models may set capabilities.reasoning=true without honoring the round-trip protocol; without this tighter check we would have injected reasoning_content into requests bound for endpoints that reject unknown fields. 3. skill-tools.js (skillInvoke): when the requested name is a skill-creator variant but no exact match exists, scan the installed skills (case-insensitive) for a matching creator. If found, return an actionable 'did you mean X?' error so the gate stays effective under wrong casing/spelling. Only return the soft 'gate is inactive' notice when no creator variant is actually installed. 4. system.js (readSkillIndex + getProblemSolvingParadigm): introduce a per-buildSystemPrompt skills cache so discoverSkills() — which walks the filesystem and reads each SKILL.md body — is called once per prompt build instead of twice. Cache is reset at the start of every buildSystemPrompt() so freshly-installed skills appear without a process restart. --- src/chat/agent-loop.js | 28 +++++++++++++++++----------- src/prompts/system.js | 37 +++++++++++++++++++++++++++---------- src/providers/index.js | 18 ++++++++++++++---- src/tools/skill-tools.js | 24 ++++++++++++++++-------- 4 files changed, 74 insertions(+), 33 deletions(-) diff --git a/src/chat/agent-loop.js b/src/chat/agent-loop.js index 111a729..1cd032d 100644 --- a/src/chat/agent-loop.js +++ b/src/chat/agent-loop.js @@ -54,18 +54,20 @@ function injectSyntheticSkillRead(messages, skillName, body, skillPath) { const filePath = skillPath || path.join(DEEPCOPILOT_SKILLS_DIR, safeName, 'SKILL.md'); const callId = `synthetic_skill_read_${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 6)}`; - messages.push({ + // DeepSeek thinking-mode quirk: once any assistant message in history + // carries reasoning_content, every subsequent assistant message MUST + // also carry it, or the API returns 400 ("reasoning_content in the + // thinking mode must be passed back to the API"). Only attach a + // reasoning_content here when the history already shows thinking mode is + // live — that way non-DeepSeek providers (which don't round-trip this + // field) aren't poisoned by a synthetic value, and the providers/index.js + // backfill stays inactive until real thoughts have already appeared. + const thinkingLive = Array.isArray(messages) && messages.some( + m => m && m.role === 'assistant' && typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0, + ); + const asst = { role: 'assistant', content: null, - // DeepSeek thinking-mode quirk: once any assistant message in history - // carries reasoning_content, every subsequent assistant message MUST - // also carry it, or the API returns 400 ("reasoning_content in the - // thinking mode must be passed back to the API"). Skill injection - // happens mid-turn (right after a real thinking-mode assistant - // message), so we must include a non-empty reasoning_content here. - // Sanitization (sanitizeMessages in providers/index.js) preserves - // this field for reasoning-capable models, so it survives to the API. - reasoning_content: `Loading skill SOP "${safeName}" via read_file to obtain its instructions before proceeding.`, tool_calls: [{ id: callId, type: 'function', @@ -74,7 +76,11 @@ function injectSyntheticSkillRead(messages, skillName, body, skillPath) { arguments: JSON.stringify({ path: filePath }), }, }], - }); + }; + if (thinkingLive) { + asst.reasoning_content = `Loading skill SOP "${safeName}" via read_file to obtain its instructions before proceeding.`; + } + messages.push(asst); messages.push({ role: 'tool', tool_call_id: callId, diff --git a/src/prompts/system.js b/src/prompts/system.js index ba6cd74..021ee97 100644 --- a/src/prompts/system.js +++ b/src/prompts/system.js @@ -235,11 +235,25 @@ function readUserMemory() { // // Issue #61 — Step 2 (skill index), Step 8 (trust warning). -function readSkillIndex() { +// Cheap memoization across a single buildSystemPrompt() call. Both +// readSkillIndex() and getProblemSolvingParadigm() need the discovered +// skills, but discoverSkills() walks the filesystem and reads every +// SKILL.md body, so scanning twice per prompt build is wasteful. The cache +// is reset at the start of every buildSystemPrompt() invocation. +let _skillsCache = null; +function _getSkills() { + if (_skillsCache !== null) return _skillsCache; try { const { discoverSkills } = require('../skills'); - const root = wsRoot(); - const skills = discoverSkills(root); + _skillsCache = discoverSkills(wsRoot()); + } catch { _skillsCache = []; } + return _skillsCache; +} +function _resetSkillsCache() { _skillsCache = null; } + +function readSkillIndex() { + try { + const skills = _getSkills(); if (!skills.length) return null; const lines = ['# Available skills']; @@ -272,13 +286,12 @@ function getProblemSolvingParadigm() { // Issue #146 — only mention the skill-creator hard gate when a meta-skill // is actually installed. Otherwise the model fabricates a skill_invoke // call to a non-existent "skill-creator", producing a confusing error. - let skillCreatorInstalled = false; - try { - const { discoverSkills } = require('../skills'); - const all = discoverSkills(wsRoot()); - const variants = new Set(['skill-creator', 'skill_creator', 'skillcreator']); - skillCreatorInstalled = all.some(s => variants.has(String(s && s.name || '').toLowerCase())); - } catch { /* ignore — treat as not installed */ } + // Reuses the memoized skills list shared with readSkillIndex() so we + // don't walk the filesystem twice per prompt build. + const variants = new Set(['skill-creator', 'skill_creator', 'skillcreator']); + const skillCreatorInstalled = _getSkills().some( + s => variants.has(String(s && s.name || '').toLowerCase()), + ); const gateParagraph = skillCreatorInstalled ? `\n\n**Issue #146 — skill_create quality gate**: A meta-skill matching \`skill-creator\` (or variant) is installed locally. You MUST call \`skill_invoke({ name: "" })\` in the SAME turn BEFORE you call \`skill_create\`. The meta-skill performs description tightening, body structure check, and dedup. Calling \`skill_create\` without it will be rejected by the tool layer.` @@ -380,6 +393,10 @@ function buildSystemPrompt(opts = {}) { const staticPart = getStaticCore(); + // Reset the per-build skills cache so a freshly-installed skill becomes + // visible without a full process restart. + _resetSkillsCache(); + const dynamicParts = [getEnvironmentSection(osName)]; const mem = readUserMemory(); if (mem) dynamicParts.push(mem); diff --git a/src/providers/index.js b/src/providers/index.js index 3201728..5dafd2d 100644 --- a/src/providers/index.js +++ b/src/providers/index.js @@ -170,10 +170,18 @@ function sanitizeMessages(providerId, messages, modelId) { if (!Array.isArray(messages) || !messages.length) return messages; const strip = getProvider(providerId)?.quirks?.stripInputFields; const isReasoning = !!(modelId && getModel(providerId, modelId)?.capabilities?.reasoning); - // For reasoning-capable models, strip every declared field except - // reasoning_content (which must round-trip). + // The reasoning_content round-trip rule is specific to providers that + // BOTH (a) declare reasoning_content as a stripInputField (i.e. the + // provider's API is known to care about this field) AND (b) are being + // used in a reasoning-capable mode. Without this narrower gate, OpenAI / + // Anthropic models that happen to flip `capabilities.reasoning` would + // get unknown `reasoning_content` fields pushed onto their requests. + const stripsReasoning = Array.isArray(strip) && strip.includes('reasoning_content'); + const honorsReasoningRoundTrip = isReasoning && stripsReasoning; + // For models that honor the round-trip protocol, strip every declared + // field except reasoning_content. Otherwise strip everything declared. const effectiveStrip = Array.isArray(strip) - ? (isReasoning ? strip.filter(f => f !== 'reasoning_content') : strip) + ? (honorsReasoningRoundTrip ? strip.filter(f => f !== 'reasoning_content') : strip) : []; let out = messages; @@ -194,7 +202,9 @@ function sanitizeMessages(providerId, messages, modelId) { // Reasoning-mode invariant backfill. Only kicks in once at least one // assistant message already carries reasoning_content — that's the // moment DeepSeek starts enforcing the "every assistant has it" rule. - if (isReasoning) { + // Scoped to providers that actually honor the round-trip protocol so we + // never inject reasoning_content into requests bound for OpenAI/Anthropic. + if (honorsReasoningRoundTrip) { const anyHasReasoning = out.some( m => m && m.role === 'assistant' && typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0, ); diff --git a/src/tools/skill-tools.js b/src/tools/skill-tools.js index 3bf02cc..398dfd4 100644 --- a/src/tools/skill-tools.js +++ b/src/tools/skill-tools.js @@ -30,14 +30,22 @@ function skillInvoke(args, run) { const all = discoverSkills(wsRoot()); const s = all.find(x => x.name === name); if (!s) { - // Issue #146 follow-up: when the model fabricates a call to a - // skill-creator meta-skill that isn't actually installed, return a - // soft notice instead of an error so the agent can simply proceed - // with `skill_create` (the executor-side gate is a no-op when no - // meta-skill is installed). This prevents the confusing - // "skill 'skill-creator' not found" failure users frequently see. - const variants = new Set(['skill-creator', 'skill_creator', 'skillcreator']); - if (variants.has(name.toLowerCase())) { + // Issue #146 follow-up: handle a model that calls a skill-creator + // variant. Two distinct cases must be told apart: + // 1. A creator IS installed but the model used wrong casing/spelling. + // → return an actionable "did you mean X" error so the gate + // stays effective. + // 2. No creator is installed at all. + // → return a soft notice so the agent can proceed straight to + // skill_create (the executor-side gate is a no-op here). + const SKILL_CREATOR_VARIANTS = new Set(['skill-creator', 'skill_creator', 'skillcreator']); + if (SKILL_CREATOR_VARIANTS.has(name.toLowerCase())) { + const installedCreator = all.find( + x => SKILL_CREATOR_VARIANTS.has(String(x && x.name || '').toLowerCase()), + ); + if (installedCreator) { + return `Error: skill "${name}" not found. Did you mean "${installedCreator.name}"? Call \`skill_invoke({ name: "${installedCreator.name}" })\` with that exact spelling.`; + } return 'Notice: no skill-creator meta-skill is installed in this environment. The skill_create quality gate is inactive here — you may proceed to call `skill_create` directly when appropriate.'; } const known = all.map(x => x.name).join(', ') || '(none installed)'; From 367d04dd07f9060e0befb8048a3e7fda1a3e1ba7 Mon Sep 17 00:00:00 2001 From: ZhouChuange Date: Tue, 26 May 2026 16:41:17 +1000 Subject: [PATCH 4/6] fix(agent): tighten reasoning backfill scope + dedup skill-creator variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more refinements from Copilot's PR #164 follow-up review: 1. providers/index.js JSDoc: rewrite to clearly distinguish the two branches — non-thinking-mode (strip reasoning_content because DeepSeek 400s) vs. thinking-mode (preserve + backfill because the API requires round-trip). Previous doc conflated them. 2. providers/index.js sanitizeMessages: narrow backfill to messages AFTER the first assistant carrying non-empty reasoning_content. DeepSeek's documented rule is about *subsequent* assistants, not all of them; this keeps pre-thinking history untouched and reduces request payload. 3. skill-tools.js: hoist SKILL_CREATOR_NAMES to module scope above skillInvoke so the soft-notice path and the existing skill_create gate share the same canonical variant set. Removes the duplicated SKILL_CREATOR_VARIANTS literal that risked drifting out of sync. --- src/providers/index.js | 53 +++++++++++++++++++++++----------------- src/tools/skill-tools.js | 17 +++++++------ 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/src/providers/index.js b/src/providers/index.js index 5dafd2d..87e5487 100644 --- a/src/providers/index.js +++ b/src/providers/index.js @@ -146,23 +146,26 @@ function listModels(providerId) { * Returns a NEW array; never mutates the input. The strip rule lives in the * provider JSON so a vendor's protocol quirks stay encapsulated. * - * Example: DeepSeek 400s when the input contains `reasoning_content`. - * deepseek.json sets quirks.stripInputFields: ["reasoning_content"], and this - * function removes that field from each message before the API call. + * Two behaviours, switched on whether the call is in DeepSeek's thinking-mode + * round-trip protocol (provider declares `reasoning_content` in stripInputFields + * AND the chosen model is reasoning-capable): * - * Exception: `reasoning_content` is never stripped for reasoning-capable models - * (capabilities.reasoning === true) because DeepSeek's API requires it to be - * passed back in subsequent turns — omitting it causes HTTP 400. + * - **Non-thinking-mode call** (everything else, including non-reasoning + * DeepSeek models and all OpenAI/Anthropic models): every field declared in + * `quirks.stripInputFields` is removed. DeepSeek 400s when the input + * contains `reasoning_content` outside thinking mode, so deepseek.json sets + * `stripInputFields: ["reasoning_content"]` and we drop it here. * - * Additionally, for reasoning-capable models we enforce an invariant required - * by DeepSeek's thinking-mode protocol: once any assistant message in history - * carries a non-empty `reasoning_content`, EVERY subsequent assistant message - * MUST also carry one — otherwise the API rejects the request with - * 400 "reasoning_content in the thinking mode must be passed back to the API". - * We backfill a short placeholder on any assistant message that is missing it. - * This is a defence-in-depth net: callers should still attach the real thought - * stream when they have one, but if they forget (or the model returned an empty - * thought, or a synthetic assistant was injected mid-turn) we won't 400. + * - **Thinking-mode call** (DeepSeek reasoner family, etc.): `reasoning_content` + * is NOT stripped because the API requires it to be passed back in + * subsequent turns. On top of that we enforce the documented invariant + * "once any assistant in history carries non-empty `reasoning_content`, + * EVERY subsequent assistant message MUST also carry one" — otherwise the + * API rejects with `400 "reasoning_content in the thinking mode must be + * passed back to the API"`. We backfill a short placeholder on assistant + * messages that come after the first one with thoughts, leaving earlier + * pre-thinking messages alone. This is a defence-in-depth net: callers + * should still attach the real thought stream when they have one. */ const REASONING_PLACEHOLDER = '(no thoughts surfaced for this step)'; @@ -199,17 +202,21 @@ function sanitizeMessages(providerId, messages, modelId) { }); } - // Reasoning-mode invariant backfill. Only kicks in once at least one - // assistant message already carries reasoning_content — that's the - // moment DeepSeek starts enforcing the "every assistant has it" rule. - // Scoped to providers that actually honor the round-trip protocol so we - // never inject reasoning_content into requests bound for OpenAI/Anthropic. + // Reasoning-mode invariant backfill. The DeepSeek rule is specifically + // about messages that come AFTER the first assistant message with + // non-empty reasoning_content — earlier "pre-thinking" assistant + // messages don't need it. Locating the first thinking index and only + // backfilling from there onward keeps payload size minimal and matches + // the documented protocol more precisely. Scoped to providers that + // actually honor the round-trip protocol so we never inject + // reasoning_content into OpenAI/Anthropic-bound requests. if (honorsReasoningRoundTrip) { - const anyHasReasoning = out.some( + const firstThinkingIdx = out.findIndex( m => m && m.role === 'assistant' && typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0, ); - if (anyHasReasoning) { - out = out.map(m => { + if (firstThinkingIdx !== -1) { + out = out.map((m, i) => { + if (i <= firstThinkingIdx) return m; if (!m || m.role !== 'assistant') return m; if (typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0) return m; return Object.assign({}, m, { reasoning_content: REASONING_PLACEHOLDER }); diff --git a/src/tools/skill-tools.js b/src/tools/skill-tools.js index 398dfd4..c2cc0d5 100644 --- a/src/tools/skill-tools.js +++ b/src/tools/skill-tools.js @@ -15,6 +15,13 @@ const path = require('path'); const { discoverSkills, DEEPCOPILOT_SKILLS_DIR } = require('../skills'); const { wsRoot } = require('../utils/paths'); +// Issue #146 — Meta-skill that must review every skill_create call. +// Accept a couple of common spelling variants to be permissive about how +// the on-disk skill is named. Defined at module scope so both skill_invoke +// (for "did you mean X?" handling) and the skill_create gate share the same +// canonical variant set. +const SKILL_CREATOR_NAMES = new Set(['skill-creator', 'skill_creator', 'skillcreator']); + // ─── skill_invoke ──────────────────────────────────────────────────────────── /** @@ -38,10 +45,9 @@ function skillInvoke(args, run) { // 2. No creator is installed at all. // → return a soft notice so the agent can proceed straight to // skill_create (the executor-side gate is a no-op here). - const SKILL_CREATOR_VARIANTS = new Set(['skill-creator', 'skill_creator', 'skillcreator']); - if (SKILL_CREATOR_VARIANTS.has(name.toLowerCase())) { + if (SKILL_CREATOR_NAMES.has(name.toLowerCase())) { const installedCreator = all.find( - x => SKILL_CREATOR_VARIANTS.has(String(x && x.name || '').toLowerCase()), + x => SKILL_CREATOR_NAMES.has(String(x && x.name || '').toLowerCase()), ); if (installedCreator) { return `Error: skill "${name}" not found. Did you mean "${installedCreator.name}"? Call \`skill_invoke({ name: "${installedCreator.name}" })\` with that exact spelling.`; @@ -88,11 +94,6 @@ const VALID_SOURCES = new Set(['self', 'web', 'hybrid']); const VALID_TRUSTS = new Set(['trusted', 'untrusted']); const MAX_BODY_BYTES = 64 * 1024; // 64 KB hard ceiling -// Issue #146 — Meta-skill that must review every skill_create call. -// Accept a couple of common spelling variants to be permissive about how -// the on-disk skill is named. -const SKILL_CREATOR_NAMES = new Set(['skill-creator', 'skill_creator', 'skillcreator']); - /** * Issue #146 — Check whether `skill_invoke({name: 'skill-creator'})` was * called earlier in the *current* turn (i.e. after the most recent real user From 4ef5b6179aaae4b35077b09b6266ea0b9d75e2f2 Mon Sep 17 00:00:00 2001 From: ZhouChuange Date: Tue, 26 May 2026 16:53:18 +1000 Subject: [PATCH 5/6] docs(skill-tools): clarify SKILL_CREATOR_NAMES gate is conditional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3: the comment above SKILL_CREATOR_NAMES claimed the meta-skill 'must review every skill_create call', but the actual gate is conditional — it only fires when a creator skill is installed in the workspace, matching the conditional prompt paragraph in system.js and the schema wording in schema.js. Rephrase the comment to reflect the runtime behaviour and cross-reference the related call sites. --- src/tools/skill-tools.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/tools/skill-tools.js b/src/tools/skill-tools.js index c2cc0d5..b565760 100644 --- a/src/tools/skill-tools.js +++ b/src/tools/skill-tools.js @@ -15,11 +15,15 @@ const path = require('path'); const { discoverSkills, DEEPCOPILOT_SKILLS_DIR } = require('../skills'); const { wsRoot } = require('../utils/paths'); -// Issue #146 — Meta-skill that must review every skill_create call. -// Accept a couple of common spelling variants to be permissive about how -// the on-disk skill is named. Defined at module scope so both skill_invoke -// (for "did you mean X?" handling) and the skill_create gate share the same -// canonical variant set. +// Issue #146 — Canonical name variants for the optional `skill-creator` +// meta-skill. When this skill is installed in the workspace, the `skill_create` +// gate requires it to be invoked before any new skill can be created (a +// quality-review step). When it is NOT installed the gate is a no-op and +// `skill_create` proceeds directly — see `skillCreate` for the conditional +// check and `system.js` / `schema.js` for the matching prompt/schema wording. +// Accept a couple of common spellings to be permissive about how the on-disk +// skill is named. Defined at module scope so both skill_invoke (for the "did +// you mean X?" soft-notice path) and the skill_create gate share the same set. const SKILL_CREATOR_NAMES = new Set(['skill-creator', 'skill_creator', 'skillcreator']); // ─── skill_invoke ──────────────────────────────────────────────────────────── From be40b3284b19d41248dbc69b573705d4940e40d4 Mon Sep 17 00:00:00 2001 From: ZhouChuange Date: Tue, 26 May 2026 17:00:56 +1000 Subject: [PATCH 6/6] docs(providers): correct sanitizeMessages return-value contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4: JSDoc claimed 'Returns a NEW array', but the implementation may return the original messages reference when no fields are stripped and no backfill applies (the fast path for OpenAI/Anthropic calls with empty stripInputFields). Update the contract to describe both branches, and explicitly tell callers to treat the result as read-only. Behaviour unchanged — the original-reference fast path avoids an unnecessary shallow copy on the hot path. --- src/providers/index.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/providers/index.js b/src/providers/index.js index 87e5487..3fc0dda 100644 --- a/src/providers/index.js +++ b/src/providers/index.js @@ -143,8 +143,12 @@ function listModels(providerId) { /** * Apply provider-declared `stripInputFields` to a messages array. - * Returns a NEW array; never mutates the input. The strip rule lives in the - * provider JSON so a vendor's protocol quirks stay encapsulated. + * Never mutates the input. Returns either a NEW array (when at least one + * message was rewritten by stripping or backfill) or the ORIGINAL `messages` + * reference unchanged (fast path: no quirks apply, e.g. an OpenAI/Anthropic + * call with an empty `stripInputFields`). Callers must treat the result as + * read-only — do not mutate elements of the returned array. The strip rule + * lives in the provider JSON so a vendor's protocol quirks stay encapsulated. * * Two behaviours, switched on whether the call is in DeepSeek's thinking-mode * round-trip protocol (provider declares `reasoning_content` in stripInputFields