fix(skill-loader): coerce numeric frontmatter command names to strings

[MoerAI]

Summary

• Prevent crash when YAML frontmatter name field is a number (e.g., name: 12306)

Problem

YAML parses numeric-looking name values as numbers, not strings. The existing code data.name || options.defaultName treats truthy numbers correctly but passes them as number type downstream, which causes type errors when the name is used as a string key in Maps or object properties.

Fix

Changed data.name || options.defaultName to data.name == null ? options.defaultName : String(data.name). This coerces any non-null frontmatter name to a string while still falling back to defaultName when name is absent.

Changes

File	Change
src/features/opencode-skill-loader/loaded-skill-from-path.ts	Null-check + String() coercion for frontmatter name
src/features/opencode-skill-loader/loader.test.ts	Add test for numeric frontmatter name (name: 12306)

---

Summary by cubic

Prevent crash in the skill loader when the YAML frontmatter name is numeric. Non-null names are coerced to strings while preserving the default-name fallback; a test covers numeric names.

^{Written for commit 3750265. Summary will update on new commits.}

[MoerAI]

I have read the CLA Document and I hereby sign the CLA

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3750265ff7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Treat empty frontmatter names as missing

The new null-only check changes behavior for name: "": baseName now becomes an empty string instead of falling back to defaultName. In practice this produces invalid skill identifiers ("" at top level or "prefix/" when nested), which are still inserted into the skill map/command record and become unreachable or collision-prone at invocation time. This is a regression from the previous fallback behavior; you can preserve the numeric-name fix while still treating empty strings as absent.

Useful? React with 👍 / 👎.

[cubic-dev-ai]

2 issues found across 2 files

Confidence score: 3/5

• There is a concrete regression risk in src/features/opencode-skill-loader/loaded-skill-from-path.ts: switching to == null allows name: "" to pass through instead of falling back to defaultName, which can produce invalid skill identifiers and user-facing loader issues.
• The test in src/features/opencode-skill-loader/loader.test.ts is currently weak for this case, because matching directory and frontmatter names can let the test pass even if frontmatter handling regresses.
• Given the medium severity and high confidence on both findings, this looks mergeable with caution after fixing/strengthening these spots.
• Pay close attention to src/features/opencode-skill-loader/loaded-skill-from-path.ts and src/features/opencode-skill-loader/loader.test.ts - identifier fallback behavior and regression coverage need to be aligned.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/features/opencode-skill-loader/loader.test.ts">

<violation number="1" location="src/features/opencode-skill-loader/loader.test.ts:111">
P2: The regression test is ineffective because the skill directory name matches the numeric frontmatter name, so it still passes if the loader ignores the frontmatter and falls back to the directory basename.</violation>
</file>

<file name="src/features/opencode-skill-loader/loaded-skill-from-path.ts">

<violation number="1" location="src/features/opencode-skill-loader/loaded-skill-from-path.ts:28">
P2: Using `== null` no longer treats `name: ""` as missing. The old `||` operator fell back to `defaultName` for empty strings, but now an empty-string name passes through, producing invalid skill identifiers (e.g., `""` or `"prefix/"`). Use a falsy-or-nullish check that also covers empty strings:
```ts
const baseName = data.name != null && data.name !== "" ? String(data.name) : options.defaultName
```</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: The regression test is ineffective because the skill directory name matches the numeric frontmatter name, so it still passes if the loader ignores the frontmatter and falls back to the directory basename.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/opencode-skill-loader/loader.test.ts, line 111:

<comment>The regression test is ineffective because the skill directory name matches the numeric frontmatter name, so it still passes if the loader ignores the frontmatter and falls back to the directory basename.</comment>

<file context>
@@ -100,6 +100,34 @@ This is a simple skill.
+---
+This skill name should still load.
+`
+      createTestSkill("12306", skillContent)
+
+      // when
</file context>

Suggested change

	createTestSkill("12306", skillContent)
	createTestSkill("numeric-name-skill", skillContent)

[cubic-dev-ai]

P2: Using == null no longer treats name: "" as missing. The old || operator fell back to defaultName for empty strings, but now an empty-string name passes through, producing invalid skill identifiers (e.g., "" or "prefix/"). Use a falsy-or-nullish check that also covers empty strings:

const baseName = data.name != null && data.name !== "" ? String(data.name) : options.defaultName

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/opencode-skill-loader/loaded-skill-from-path.ts, line 28:

<comment>Using `== null` no longer treats `name: ""` as missing. The old `||` operator fell back to `defaultName` for empty strings, but now an empty-string name passes through, producing invalid skill identifiers (e.g., `""` or `"prefix/"`). Use a falsy-or-nullish check that also covers empty strings:
```ts
const baseName = data.name != null && data.name !== "" ? String(data.name) : options.defaultName
```</comment>

<file context>
@@ -25,7 +25,7 @@ export async function loadSkillFromPath(options: {
     const mcpConfig = mcpJsonMcp || frontmatterMcp
 
-    const baseName = data.name || options.defaultName
+    const baseName = data.name == null ? options.defaultName : String(data.name)
     const skillName = namePrefix ? `${namePrefix}/${baseName}` : baseName
     const originalDescription = data.description || ""
</file context>

Suggested change

	const baseName = data.name == null ? options.defaultName : String(data.name)
	const baseName = data.name != null && data.name !== "" ? String(data.name) : options.defaultName