enhance(#4032): apply configured agent tool grants (#4238)

* test(4032): add failing installed-agent grants contract

Cover global and project agent_tools precedence at the real Claude installer seam before adding implementation.

* feat(4032): apply configured agent tool grants during staging

Resolve selector-level global and project config once per staging call, then append validated grants before runtime conversion.

* test(4032): cover host grant and quoted MCP contracts

Exercise installed host artifacts and prove ZCode must treat quoted MCP scalars like plain MCP grants.

* feat(4032): apply configured agent tool grants across runtimes

Move augmentation and scalar identity into the converter seam so every staged artifact preserves host policy.

* fix(4032): register agent tool grants in configuration

Accept documented agent_tools config without unknown-key warnings.\n\nKeep installer fixtures on the shared temporary-directory helper.

* fix(4032): translate configured MCP grants for Kilo

Reuse the converter-owned scalar decoder so quoted canonical grants reach Kilo's native permission keys without altering other host policies.

* fix(4032): decode YAML-escaped tool grants

* fix(4032): emit valid inline agent tool grants

* fix(4032): reject invalid trailing-colon grants

* test(#4032): cover cross-review remediation gaps

* fix(#4032): close cross-runtime grant gaps

* test(#4032): expose Kimi global project context

* fix(#4032): preserve Kimi project config context

* chore(#4032): add release note

* test(#4032): expose fork review regressions

* fix(#4032): address fork review findings

* test(#4032): make byte-stability assertion portable

Compare repeat installs at one root so platform-specific path rendering cannot
masquerade as an agent_tools behavior change.

* chore(#4032): bind changeset to upstream PR 4238

* fix(#4032): address trek-e review findings (2,3,4,5,6,7,8)

Fixes fail-closed decode-failure handling in ZCode's mcp__ stripper,
a comment-only `tools:` header mis-parse that silently dropped
configured grants, and a naive comma-split that could tear a quoted
scalar containing a literal comma. Documents Kilo's inherent
`{server}_{tool}` MCP-permission-key collision (external, fixed
format — not ours to widen) and locks the existing first-seen-wins
resolution in with a regression test.

Opts kimi/kimi-code out of the ADR-1235 pre-converter path-rewrite
step: routing Kimi through that pipeline (needed so project-scoped
agent_tools selectors reach it) was short-circuiting Kimi's own
neutralizeKimiAgentPrompt, which expects the original ~/.claude/gsd-core
text rather than a pre-rewritten Kimi path.

Extends the fast-check token pool and per-runtime install coverage
with the missing comment/comma/broad-runtime cases the prior review
flagged as untested.

* docs(#4032): add CONTEXT.md glossary entries for agent_tools resolver + pre-converter step

Documents readGsdEffectiveAgentTools (Install Model Override Resolver
Module) and the appendAgentTools pre-converter pipeline step (Runtime
Artifact Conversion Module), per contributor-standards.md's
new-seam glossary requirement (finding 1).

* fix(#4032): address agy adversarial review findings

An agy (gemini-3.8-flash-high) adversarial pass over the prior review-fix
commit found the fixes for findings 3, 4, 6 and 8 had unfixed sibling gaps,
plus a genuine new regression and two CONTEXT.md inaccuracies:

- ZCode's comment-only `tools: # note` header matched the inline-value
  branch instead of falling through to the block-list scan, so a following
  mcp__* item leaked through unstripped — the exact defect finding 4 fixed
  in appendAgentTools, unfixed in this sibling function.
- Reverted capabilities/kimi-code/capability.json's noPathRewrite: true.
  kimi-code uses the standard 'agents' kind with converter: null (not
  kimi-agents — confirmed by reading the descriptor, not its prose
  description), so it never went through the pipeline change finding 5
  fixed, and disabling its path rewrite broke every ~/.claude/ embed in
  its shipped agents instead.
- decodeToolScalar never stripped a trailing ` # comment` from a bare
  (unquoted) scalar, so a comment after a block-list item, or after an
  appended grant on an inline line, became part of the "tool name" —
  fixed at the source (one call site fixes every consumer).
- appendAgentTools's comment-index scan wasn't quote-aware, so a `#`
  inside a quoted scalar (`"mcp__server #1"`) was mistaken for a comment
  start and corrupted the quote.
- parseFrontmatterTools (Kimi/Qwen's tool-list reader, downstream of
  appendAgentTools's own output) had the same naive comma-split and
  comment-only-header gaps as findings 4 and 6, unpatched.
- The all-runtime smoke test's presence assertion was built on a guessed
  omit-list; empirically only 7 of 17 runtimes keep an arbitrary mcp__
  grant recognizable, replaced with a verified allowlist.
- CONTEXT.md claimed a `project:<agent>` selector prefix that does not
  exist (project override is a same-key merge across two config files)
  and mislabeled stageAgentsForRuntimeWithConverter's module.

* fix(#4032): address full-PR review (Opus critical/ponytail + agy)

A whole-PR pass (critical-code-reviewer + ponytail-review on Opus, plus a
second agy full-source adversarial pass) surfaced defects the earlier
finding-scoped passes couldn't reach:

- appendAgentTools corrupted a `tools:` line whose ENTIRE value is a
  leading quoted scalar (`tools: "Read"` -> `tools: "Read", Write`,
  invalid YAML) — there is no safe line-surgical rewrite here, so it now
  refuses to touch that shape instead of emitting broken frontmatter.
- decodeToolScalar's malformed-trailing-quote check ran BEFORE comment
  stripping, so a bare tool name with a quote inside its own trailing
  comment (`Bash # note: "internal"`) was wrongly rejected. Reordered.
- findUnquotedCommentIndex (added in the prior remediation commit) was
  built on a wrong model of YAML: a `#` after whitespace starts a real
  comment in a plain scalar regardless of nearby quote characters —
  verified against the actual parser. The one case that DOES need
  protection (a leading quoted scalar) is now refused outright above, so
  the quote-tracking scan was dead weight solving a problem that no
  longer reaches it. Removed; reverted to the plain `[ \t]#` scan.
- Kilo has a SEPARATE agent-frontmatter parser (convertClaudeToKiloFrontmatter,
  distinct from the buildKiloAgentPermissionBlock fixed earlier) with the
  same comment-only-header and naive-comma-split gaps as findings 4 and 6
  — unfixed in both its src/ and bin/install.js copies. Fixed in both,
  exporting splitToolScalars for bin/install.js to reuse rather than
  reimplementing it.
- Pipeline docstring in stageAgentsForRuntimeWithConverter still listed 5
  steps, omitting appendAgentTools (now step 3 of 6).
- docs/CONFIGURATION.md didn't state that a --global install still
  discovers agent_tools from the cwd's .planning/config.json (confirmed
  intentional and already covered by a dedicated test, not a bug).
- Removed install-engine.cts's deps.cwd injection seam: zero callers or
  tests ever populated it.

Two claims from this round were verified and rejected, not fixed:
prototype pollution via a `__proto__` selector key (empirically confirmed
`Object.prototype` is never touched — only reassigns the resolver's own
local object's prototype, with no observable effect), and a `*` grant
value crashing YAML parsing as an alias reference (empirically confirmed
it parses as plain scalar text, no crash). A pre-existing, unrelated
defect (extractFrontmatterField returns null for block-list `tools:` on
Copilot/Antigravity/Cursor/Codex/Qwen, affecting two shipped agents
today) was filed as a follow-up rather than fixed here — it predates
#4032 and isn't caused or worsened by this PR.

* fix(#4032): update stale slug-derivation-drift-guard fixture line

normalizeKimiSkillName's real closing brace moved from line 616 to 635 as a
side effect of this PR's edits to runtime-artifact-conversion.cts; the
MAJOR-1 fixture's hardcoded realEndLine had gone stale.

* fix(#4032): address CodeRabbit findings on projectDir threading and flow-sequence tools

bin/install.js's installAgentsKindStandalone call site omitted the projectDir
argument the function already supports, so a global install through this
legacy branch silently fell back to the runtime config dir instead of
process.cwd() when resolving project-scoped agent_tools grants — inconsistent
with the sibling installOpencodeFamilyArtifacts call site, which already
threads it correctly.

appendAgentTools' leading-quoted-scalar bailout did not cover a YAML flow
sequence (`tools: [Bash, Read]`): splitToolScalars tore it apart on the
in-sequence commas and appended past its closing bracket, producing invalid
frontmatter. Extended the bailout regex to also refuse a value starting with
`[`, matching the same "whole node, nothing may follow" reasoning already
applied to quoted scalars.

---------

Co-authored-by: CI Rebase Check <ci@gsd-redux>
Co-authored-by: Test <test@test.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Dennis Alexis Valin Dittrich
2026-09-05 10:52:45 +02:00
committed by GitHub
parent e8800287d5
commit 925a363879
19 changed files with 976 additions and 55 deletions

View File

@@ -288,12 +288,27 @@ const kiloAgentPermissionOrder = [
'lsp',
];
const kiloMcpPermissionPattern = /^mcp__([A-Za-z0-9_-]+)__((?:[A-Za-z0-9_-]+)|\*)$/;
/**
* Derive Kilo's native `{server}_{tool}` MCP permission key (kilo.ai/docs —
* external, fixed format we don't control). Not injective: both capture
* groups allow `_`, so e.g. `mcp__a_b__c` and `mcp__a__b_c` derive the same
* key. `buildKiloAgentPermissionBlock`'s `Set` resolves any such collision
* deterministically to first-seen-wins — see its regression test. Changing
* the derivation to avoid the collision isn't an option: Kilo's own runtime
* only recognizes this exact key shape.
*/
function convertClaudeToKiloPermissionTool(claudeTool) {
return claudeToKiloAgentPermissions[claudeTool] || null;
const builtinPermission = claudeToKiloAgentPermissions[claudeTool];
if (builtinPermission) return builtinPermission;
const mcpPermission = kiloMcpPermissionPattern.exec(claudeTool);
return mcpPermission ? `${mcpPermission[1]}_${mcpPermission[2]}` : null;
}
function buildKiloAgentPermissionBlock(claudeTools) {
const allowedPermissions = new Set();
const allowedPermissions = new Set<string>();
for (const tool of claudeTools) {
const mapped = convertClaudeToKiloPermissionTool(tool);
@@ -306,6 +321,10 @@ function buildKiloAgentPermissionBlock(claudeTools) {
for (const permission of kiloAgentPermissionOrder) {
lines.push(` ${permission}: ${allowedPermissions.has(permission) ? 'allow' : 'deny'}`);
}
for (const permission of allowedPermissions) {
if (kiloAgentPermissionOrder.includes(permission)) continue;
lines.push(` ${permission}: allow`);
}
return lines;
}
@@ -675,6 +694,122 @@ function convertClaudeCommandToKimiCodeSkill(content, skillName, _runtime = null
const KIMI_CANONICAL_GSD_AGENT_RE = /^gsd-[a-z0-9-]+$/;
/** Split a `tools:` comma list without tearing a quoted scalar that contains a literal comma. */
function splitToolScalars(text: string): string[] {
const parts: string[] = [];
let current = '';
let quote: string | null = null;
for (const ch of text) {
if (quote) {
current += ch;
if (ch === quote) quote = null;
} else if (ch === '"' || ch === "'") {
quote = ch;
current += ch;
} else if (ch === ',') {
parts.push(current);
current = '';
} else {
current += ch;
}
}
parts.push(current);
return parts;
}
/** Normalize one complete frontmatter tool scalar through the shared YAML parser. */
function decodeToolScalar(raw: unknown): string | null {
if (typeof raw !== 'string') return null;
const value = raw.trim();
if (!value) return null;
if (value.startsWith('"') || value.startsWith("'")) {
try {
const decoded = frontmatterModule.parseFrontmatter('---\ntool: ' + value + '\n---\n').tool;
return typeof decoded === 'string' && decoded.trim() ? decoded.trim() : null;
} catch {
return null;
}
}
// A bare (unquoted) scalar may carry a trailing ` # comment` (YAML requires
// whitespace before `#` to start a comment) — every caller here passes a
// single already-comma-split token, never the whole `tools:` line, so this
// is safe to strip unconditionally rather than pushing the concern onto
// each call site. Strip BEFORE the malformed-quote check below: a comment
// may itself contain a `"`/`'` (e.g. `Bash # note: "internal"`), which is
// not a real YAML quote delimiter and must not cause a false rejection.
const commentIndex = value.search(/[ \t]#/);
const stripped = commentIndex === -1 ? value : value.slice(0, commentIndex).trimEnd();
if (!stripped) return null;
if (stripped.endsWith('"') || stripped.endsWith("'")) return null;
return stripped;
}
/** Append validated canonical grants without reserializing unrelated frontmatter. */
function appendAgentTools(content: string, grants: string[]): string {
if (grants.length === 0) return content;
const eol = content.includes('\r\n') ? '\r\n' : '\n';
const lines = content.split(eol);
if (lines[0] !== '---') return content;
const frontmatterEnd = lines.indexOf('---', 1);
if (frontmatterEnd === -1) return content;
const toolsIndex = lines.findIndex((line, index) => index < frontmatterEnd && /^tools:[ \t]*(.*)$/.test(line));
if (toolsIndex === -1) return content;
const toolsMatch = /^tools:[ \t]*(.*)$/.exec(lines[toolsIndex]);
if (!toolsMatch) return content;
// Plain (non-leading-quote) YAML scalars have no internal quoting — a `#`
// after whitespace is a real comment start regardless of nearby quote
// characters (verified: real YAML truncates `Read, "x #1"` at the space
// before `#` too), so this scan does not need to be quote-aware. The one
// case that DOES need protection — a value that IS a leading quoted scalar
// — is refused outright below rather than parsed past.
const commentIndex = toolsMatch[1].search(/[ \t]#/);
let inlineValue = commentIndex === -1 ? toolsMatch[1] : toolsMatch[1].slice(0, commentIndex);
let inlineComment = commentIndex === -1 ? '' : toolsMatch[1].slice(commentIndex);
// A header that is ONLY a comment (`tools: # note`) has no leading space in
// the captured group — the outer regex's `[ \t]*` already consumed it — so
// the `[ \t]#` scan above never fires. Reclassify as no inline value so the
// block-list scan below runs instead of swallowing the comment as content.
if (inlineValue.trim().startsWith('#')) {
inlineComment = toolsMatch[1];
inlineValue = '';
}
// A value that STARTS with a quote is a YAML quoted scalar occupying the
// whole node — nothing may follow it on the same line except a comment
// (`tools: "Bash"` is valid; `tools: "Bash", Read` is not, even before this
// function touches it). Appending in place would corrupt otherwise-valid
// frontmatter, so refuse rather than emit invalid YAML. A value that STARTS
// with `[` is a YAML flow sequence (`tools: [Bash, Read]`) — its own commas
// are node-internal, not scalar separators, and content cannot follow its
// closing `]` on the same line either, so the same refusal applies.
if (/^["'[]/.test(inlineValue.trim())) return content;
const existing: string[] = [];
let insertAt = toolsIndex + 1;
if (inlineValue.trim()) {
existing.push(...splitToolScalars(inlineValue).map(decodeToolScalar).filter((value): value is string => value !== null));
} else {
while (insertAt < frontmatterEnd) {
const item = /^([ \t]+)-[ \t]*(\S.*)$/.exec(lines[insertAt]);
if (!item) break;
const decoded = decodeToolScalar(item[2]);
if (decoded !== null) existing.push(decoded);
insertAt += 1;
}
}
const present = new Set(existing);
const additions = grants.filter((grant) => !present.has(grant) && (present.add(grant), true));
if (additions.length === 0) return content;
if (inlineValue.trim()) {
lines[toolsIndex] = `tools: ${inlineValue.trimEnd()}, ${additions.join(', ')}${inlineComment}`;
} else {
const firstItem = /^([ \t]+)-/.exec(lines[toolsIndex + 1]);
const indent = firstItem ? firstItem[1] : ' ';
lines.splice(insertAt, 0, ...additions.map((grant) => `${indent}- ${JSON.stringify(grant)}`));
}
return lines.join(eol);
}
function parseKimiAgentSource(source) {
if (typeof source === 'string') {
return {
@@ -703,7 +838,8 @@ function parseFrontmatterTools(frontmatter) {
if (collecting) {
if (trimmed.startsWith('- ')) {
tools.push(trimmed.slice(2).trim());
const tool = decodeToolScalar(trimmed.slice(2));
if (tool !== null) tools.push(tool);
continue;
}
collecting = false;
@@ -716,10 +852,13 @@ function parseFrontmatterTools(frontmatter) {
if (trimmed.startsWith('tools:') || trimmed.startsWith('allowed-tools:')) {
const value = trimmed.slice(trimmed.indexOf(':') + 1).trim();
if (value) {
for (const tool of value.split(',')) {
const name = tool.trim();
if (name) tools.push(name);
// A comment-only value (`tools: # note`) has no real inline content —
// fall through to the block-list scan below instead of decoding the
// comment text as a bogus tool name and silently dropping the list.
if (value && !value.startsWith('#')) {
for (const tool of splitToolScalars(value)) {
const name = decodeToolScalar(tool);
if (name !== null) tools.push(name);
}
} else {
collecting = true;
@@ -2100,7 +2239,8 @@ function convertClaudeToKiloFrontmatter(content, { isAgent = false, modelOverrid
if (isAgent && inAgentTools) {
if (trimmed.startsWith('- ')) {
agentTools.push(trimmed.substring(2).trim());
const tool = decodeToolScalar(trimmed.substring(2));
if (tool !== null) agentTools.push(tool);
continue;
}
if (trimmed && !trimmed.startsWith('-')) {
@@ -2112,8 +2252,11 @@ function convertClaudeToKiloFrontmatter(content, { isAgent = false, modelOverrid
if (trimmed.startsWith('tools:')) {
if (isAgent) {
const toolsValue = trimmed.substring(6).trim();
if (toolsValue) {
const tools = toolsValue.split(',').map(t => t.trim()).filter(t => t);
// A comment-only value (`tools: # note`) is not real inline content —
// fall through to the block-list scan (`inAgentTools`) instead of
// decoding the comment as a bogus tool name and dropping the list.
if (toolsValue && !toolsValue.startsWith('#')) {
const tools = splitToolScalars(toolsValue).map(decodeToolScalar).filter((tool): tool is string => tool !== null);
agentTools.push(...tools);
} else {
inAgentTools = true;
@@ -2179,7 +2322,8 @@ function convertClaudeToKiloFrontmatter(content, { isAgent = false, modelOverrid
if (trimmed.startsWith('- ')) {
const tool = trimmed.substring(2).trim();
if (isAgent) {
agentTools.push(tool);
const decoded = decodeToolScalar(tool);
if (decoded !== null) agentTools.push(decoded);
} else {
allowedTools.push(tool);
}
@@ -2545,11 +2689,17 @@ function convertClaudeAgentToQwenAgent(content) {
* Byte-identical for an agent with no `mcp__*` grants (the common case) and
* for an agent with no frontmatter at all.
*/
/** Fail-closed: an undecodable scalar is dropped, never kept (ZCode's contract is "never emit mcp__*"). */
function zcodeKeepsGrant(rawTool: string): boolean {
const decoded = decodeToolScalar(rawTool);
return decoded !== null && !decoded.startsWith('mcp__');
}
function convertClaudeAgentToZcodeAgent(content) {
// Fast path: no MCP grant token anywhere means nothing to strip. (A body
// mention alone is not a grant — the line scan below finds no tools-line
// change and returns `content` unchanged anyway; this just skips the scan.)
if (!content.includes('mcp__')) return content;
// A double-quoted YAML scalar may encode the leading "m" in mcp__ as an
// escape, so only skip the shared scalar-decoder scan when neither form is
// present. The unchanged scan below still preserves byte-identical content.
if (!content.includes('mcp__') && !content.includes('\\')) return content;
const lines = content.split('\n');
if (lines[0] !== '---') return content;
@@ -2568,9 +2718,14 @@ function convertClaudeAgentToZcodeAgent(content) {
while (i < fmEnd) {
const line = lines[i];
const inlineTools = /^tools:[ \t]*(.+)$/.exec(line);
if (inlineTools) {
const grants = inlineTools[1].split(',').map((tool) => tool.trim()).filter((tool) => tool !== '');
const kept = grants.filter((tool) => !tool.startsWith('mcp__'));
// A header that is ONLY a comment (`tools: # note`) is not real inline
// content — fall through to the block-list scan below instead of
// matching here, or a following block list's mcp__* items never get
// scanned and leak through unstripped.
const commentOnlyHeader = inlineTools !== null && inlineTools[1].trim().startsWith('#');
if (inlineTools && !commentOnlyHeader) {
const grants = splitToolScalars(inlineTools[1]).map((tool) => tool.trim()).filter((tool) => tool !== '');
const kept = grants.filter((tool) => zcodeKeepsGrant(tool));
if (kept.length === grants.length) {
out.push(line); // no mcp__* grants — keep the line byte-identical
} else if (kept.length > 0) {
@@ -2582,7 +2737,7 @@ function convertClaudeAgentToZcodeAgent(content) {
i++;
continue;
}
if (/^tools:[ \t]*$/.test(line)) {
if (/^tools:[ \t]*$/.test(line) || commentOnlyHeader) {
// Block-list form: collect the following `- item` lines.
const items = [];
let j = i + 1;
@@ -2592,7 +2747,7 @@ function convertClaudeAgentToZcodeAgent(content) {
}
const kept = items.filter((item) => {
const name = /^([ \t]*)-[ \t]*(\S.*)$/.exec(item)[2].trim();
return !name.startsWith('mcp__');
return zcodeKeepsGrant(name);
});
if (kept.length !== items.length) {
changed = true;
@@ -3644,6 +3799,7 @@ function processAttribution(
export = {
processAttribution,
appendAgentTools,
// #2103: public accessor for hostBehaviors.agentFileExtension, exported so
// surface.cts's _syncGsdDir can derive the .agent.md rename from the SAME
// descriptor read as install-engine.cts (folds a duplicated hardcoded
@@ -3700,6 +3856,8 @@ export = {
// opencode/kilo command install through the engine instead of the bespoke path).
convertClaudeToOpencodeFrontmatter,
convertClaudeToKiloFrontmatter,
_decodeToolScalar: decodeToolScalar,
_splitToolScalars: splitToolScalars,
readGsdCommandNames,
transformContentToHyphen,
// #1383: version resolver (exported for regression test of the Codex