fix: address remaining PR review feedback from #53, #54, #57 (#58)
* perf(skill-loader): eliminate duplicate readFile in async discovery
Copilot flagged on PR #57 that discoverSkillsInDirAsync performed a
readFile(skillMdPath, 'utf-8') as an existence check immediately
before calling loadSkillFromPathAsync(skillMdPath, ...) which reads
the same file a second time. Two full file reads per directory with
no benefit.
loadSkillFromPathAsync already wraps a single readFile in try/catch
and returns null on any error (ENOENT, parse failure, etc.), so the
existence-check readFile is redundant. Call loadSkillFromPathAsync
directly, treat null as 'not a valid skill entrypoint', and fall
through to the {dirName}.md fallback or recursion.
Same refactor applied for the {dirName}.md fallback path. Halves the
filesystem reads during skill discovery.
* fix(skill-loader): resolve flat-name collisions across sources and hidden top-levels
Addresses two related flat-name slot-reservation gaps flagged on PR
#54 by Codex (P2) and Devin.
Before:
1. Each skill source (user, project, opencode, opencode-project,
agents-project, agents-global, config-source) called
skillsToCommandDefinitionRecord with its own fresh keyAssignedTo
map. Two nested skills with the same flatName in different sources
both claimed the flat slot, and the last source spread-merged won.
2. When a top-level skill had user-invocable: false it was skipped
entirely and never recorded in keyAssignedTo. A nested skill with
a matching flatName could then claim the flat slot, causing the
user's explicit 'do not expose this as /auth-patterns' to silently
leak through a nested skill that happened to share the basename.
After:
- SkillsToCommandDefinitionRecordOptions gains an optional
keyAssignedTo map. command-config-handler.ts allocates a single map
and threads it through all seven skill-source calls, so flat-name
ownership is global across the whole registration pass.
- Hidden top-level skills still record their name into keyAssignedTo
even though they are not registered as slash commands. Nested
skills with a matching flatName detect the ownership and fall back
to their path-prefixed unique name.
Two new regression tests:
- reserves flat-name slot when top-level is hidden via user-invocable
false
- shares keyAssignedTo across calls so cross-source collisions
resolve
* fix(config): tighten remaining disabled_* fields + clarify disabled_commands scope
Two follow-ups to PR #53 review feedback from Devin:
1. Inconsistent min(1) validation: PR #53 tightened disabled_skills
and disabled_commands to reject empty / whitespace-only strings,
but left disabled_agents, disabled_hooks, and disabled_tools on
plain z.array(z.string()). Apply the same NonBlankStringSchema
across all four so typos surface uniformly. disabled_mcps is left
as-is because AnyMcpNameSchema already enforces min(1).
2. disabled_commands JSDoc was misleading: Devin correctly pointed
out that loadBuiltinCommands() is the only loader that consults
disabled_commands. User-defined commands and skill-sourced
commands are loaded independently and spread on top without any
filter. The doc now states explicitly that only OmO builtin
commands pass through this filter, with examples.
New tests cover the three newly-tightened fields accepting valid
names and rejecting whitespace inputs.
* fix(skill-loader): address PR #58 review: race condition, clobbered reservations, disabled_mcps parity
Three critical findings from PR #58 reviews:
1. RACE CONDITION (Kilo, Devin, Codex, Copilot)
The shared keyAssignedTo map was passed into six parallel load*Skills
calls running under Promise.all. Flat-name ownership was decided by
whichever async loader finished first rather than by a deterministic
source order, so cross-source collisions could resolve differently
run-to-run and two sources could even both claim the same key in a
true race. Fix: swap load*Skills (Record-returning) for discover*Skills
(LoadedSkill[]-returning) in the Promise.all, then run
skillsToCommandDefinitionRecord sequentially in the documented
priority order with the shared map. File-system reads still happen
in parallel; only slash-key assignment is sequentialized.
2. CLOBBERED RESERVATIONS (Copilot)
The hidden-top-level slot reservation did keyAssignedTo.set(name, name)
unconditionally, which would overwrite an earlier nested skill's claim
on the same name and break 'first claimant wins'. Guard with has()
before reserving.
3. disabled_mcps PARITY (Kilo CRITICAL)
disabled_mcps was still using AnyMcpNameSchema (z.string().min(1))
which accepts whitespace-only strings, leaving a gap after the other
disabled_* fields tightened to NonBlankStringSchema. Switch
disabled_mcps to NonBlankStringSchema and drop the now-unused
AnyMcpNameSchema import.
Test updates:
- command-config-handler.test.ts: spies moved from load*Skills to
discover*Skills and now mock LoadedSkill[] instead of Record.
- agent-names.test.ts: added empty-string and whitespace-only rejection
tests for disabled_mcps, plus empty-string rejection tests for
disabled_agents/disabled_hooks/disabled_tools per Kilo's suggestion.
Verified with bun run script/run-ci-tests.ts (4779 pass, 0 fail).
* fix(plugin-config): reject blank disabled_* entries in the partial fallback
Cubic (P2) caught that parseConfigPartially's fast-path for string-array
keys only checked Array.isArray + typeof === string, so whitespace-only
values could silently load when another config section was invalid and
forced the partial fallback - bypassing the NonBlankStringSchema
validation on the main schema.
Fix: add PARTIAL_NON_BLANK_STRING_ARRAY_KEYS tracking disabled_mcps,
disabled_agents, disabled_skills, disabled_hooks, disabled_commands,
and disabled_tools. When any of those appear on the fast path, reject
the whole array if any entry is empty or whitespace-only and log an
invalid-section message (matching the semantics of rejecting a blank
entry under the main schema).
Two new regression tests cover the partial-fallback path:
- drops whitespace-only disabled_* entries even on the fast path
- keeps valid disabled_* entries when no blanks are present
Verified with bun run script/run-ci-tests.ts (4781 pass, 0 fail). V
Vacbo committed
afbf209739f39c60e42a39084b40a5f1199eb4ef
Parent: b02b4bb
Committed by GitHub <noreply@github.com>
on 4/21/2026, 11:34:58 PM