Skip to content

Commit 8604d36

Browse files
hamr0claude
andcommitted
fix(eval-assist F2): validate skill/tool names against provider charset
/diff-review finding: register() and the constructor accepted skill names, tool names, and metaToolName outside `^[a-zA-Z0-9_-]+$`. Since the skill name prefixes its tools (name_tool), an operator name with a disallowed char (e.g. a dot → my.skill_checkpoint) passed registration but would be REJECTED by OpenAI/Anthropic at generate time — the exact defect class the F2 POC caught for the separator, reintroducible via operator-supplied names. Now fails fast at registration/construction with a ValidationError. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3352afd commit 8604d36

2 files changed

Lines changed: 28 additions & 0 deletions

File tree

src/skills.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,12 @@ const { ValidationError, ToolError } = require('./errors');
3838

3939
const META_NAME = 'skill_use';
4040

41+
// Provider-safe identifier charset. OpenAI enforces `^[a-zA-Z0-9_-]+$` on tool names and Anthropic the
42+
// same shape (the POC, poc/f2-skill-thunk.mjs, proved a dot is rejected at generate time). Skill names and
43+
// bare tool names must satisfy this so the prefixed result — joined by `_`, itself in the set — is accepted
44+
// by every provider. Validating at register() fails fast, instead of surfacing as a mid-run provider error.
45+
const SAFE_NAME = /^[a-zA-Z0-9_-]+$/;
46+
4147
class SkillRegistry {
4248
/**
4349
* @param {Object} [options]
@@ -56,6 +62,9 @@ class SkillRegistry {
5662
/** @type {Set<string>} externally-reserved names (native/MCP) for collision detection */
5763
this._reserved = new Set(options.reserved || []);
5864
this.metaToolName = options.metaToolName || META_NAME;
65+
if (!SAFE_NAME.test(this.metaToolName)) {
66+
throw new ValidationError(`[SkillRegistry] meta-tool name "${this.metaToolName}" must match ${SAFE_NAME} (provider tool-name charset).`);
67+
}
5968
if (this._reserved.has(this.metaToolName)) {
6069
throw new ValidationError(`[SkillRegistry] meta-tool name "${this.metaToolName}" collides with a reserved tool name.`);
6170
}
@@ -79,6 +88,9 @@ class SkillRegistry {
7988
if (typeof name !== 'string' || !name) {
8089
throw new ValidationError(`[SkillRegistry] skill name must be a non-empty string (got ${JSON.stringify(name)}).`);
8190
}
91+
if (!SAFE_NAME.test(name)) {
92+
throw new ValidationError(`[SkillRegistry] skill name "${name}" must match ${SAFE_NAME} — it prefixes tool names, which providers reject outside that charset (e.g. a dot breaks OpenAI/Anthropic).`);
93+
}
8294
if (this._skills.has(name)) {
8395
throw new ValidationError(`[SkillRegistry] duplicate skill name "${name}".`);
8496
}
@@ -104,6 +116,9 @@ class SkillRegistry {
104116
if (typeof tool.execute !== 'function') {
105117
throw new ValidationError(`[SkillRegistry] skill "${name}" tool "${tool.name}" is missing an execute() function.`);
106118
}
119+
if (!SAFE_NAME.test(tool.name)) {
120+
throw new ValidationError(`[SkillRegistry] skill "${name}" tool name "${tool.name}" must match ${SAFE_NAME} (provider tool-name charset).`);
121+
}
107122
const fullName = `${name}_${tool.name}`;
108123
if (seenThisSkill.has(fullName)) {
109124
throw new ValidationError(`[SkillRegistry] skill "${name}" defines "${tool.name}" twice.`);

test/skills.test.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,19 @@ describe('SkillRegistry — registration & catalog', () => {
7474
);
7575
});
7676

77+
it('rejects a skill name or tool name outside the provider-safe charset (fail-fast, not mid-run)', () => {
78+
const skills = new SkillRegistry();
79+
// A dot in the skill name → prefixed tool name breaks OpenAI/Anthropic at generate time; reject early.
80+
assert.throws(() => skills.register({ name: 'my.skill', description: 'd', instructions: 'i', tools: [] }), ValidationError);
81+
// A dot in a bare tool name has the same effect.
82+
assert.throws(
83+
() => skills.register({ name: 'ok', description: 'd', instructions: 'i', tools: [mkTool('bad.name')] }),
84+
ValidationError,
85+
);
86+
// An unsafe meta-tool name is rejected at construction.
87+
assert.throws(() => new SkillRegistry({ metaToolName: 'skill.use' }), ValidationError);
88+
});
89+
7790
it('rejects a missing description or instructions', () => {
7891
const skills = new SkillRegistry();
7992
assert.throws(() => skills.register({ name: 's', instructions: 'i', tools: [] }), ValidationError);

0 commit comments

Comments
 (0)