feat(core): Skills PR-B — wire loader into generation#79
Merged
Conversation
#33 added the skills loader and four builtin skills, but generate() never called the loader or injected skill bodies into the system prompt, so data-viz-recharts (and the other three) were dead code at runtime. Changes: - packages/core: generate() now calls loadBuiltinSkills() + matchSkillsToPrompt() per request and passes matched skill bodies through composeSystemPrompt({ skills }) (the parameter already existed). Loader failure is best-effort: log and continue. - packages/core/src/skills/loader.ts: export loadBuiltinSkills() that resolves the bundled builtin/ dir via import.meta.url. - packages/providers/src/skill-injector.ts: add matchSkillsToPrompt(skills, prompt) using a curated keyword set extracted from the four builtin descriptions. - vitest: added three skills-injection tests (match → injected, no match → not injected, loader failure → graceful fallback).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Findings
- [Blocker] Silent fallback on skill-load failure violates project constraint —
generate()catches builtin skill loading errors and continues without surfacing the error, which conflicts with “No silent fallbacks — every error must surface in UI or throw with context”. Evidencepackages/core/src/index.ts:359,packages/core/src/index.ts:367,packages/core/src/index.ts:371
Suggested fix:// packages/core/src/index.ts } catch (err) { throw new CodesignError( `Failed to load builtin skills: ${err instanceof Error ? err.message : String(err)}`, 'SKILLS_LOAD_FAILED', ); }
Summary
- Review mode: initial
- 1 issue found: silent fallback was introduced in
generate()for skill loading failures.
Testing
- Not run (automation)
open-codesign Bot
| errorClass: err instanceof Error ? err.constructor.name : typeof err, | ||
| message: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| return []; |
Contributor
There was a problem hiding this comment.
[Blocker] Silent fallback: this catch returns [] and suppresses the error path, violating the repo rule that errors must surface in UI or throw with context.
Suggested fix:
} catch (err) {
throw new CodesignError(
`Failed to load builtin skills: ${err instanceof Error ? err.message : String(err)}`,
'SKILLS_LOAD_FAILED',
);
}Codex flagged the silent fallback in collectMatchedSkillBlobs as a violation of "no silent fallbacks". Generation must still succeed when the builtin skills directory is unreadable (UX), but the failure now surfaces three ways: - console.warn with errorClass + message (visible in dev/main process) - logger.error step=load_skills.fail (structured event for desktop logs) - GenerateOutput.warnings[] so the renderer/UI can display it Test updated to assert the warning is returned and the error log fires.
Contributor
There was a problem hiding this comment.
Findings
- [Minor] Substring keyword matching can trigger unrelated skills —
extractKeywordsusesincludes, so short triggers (for exampleslide) also match inside unrelated words (for examplelandslide), which can inject the wrong skill and drift generation behavior. Evidencepackages/providers/src/skill-injector.ts:166.
Suggested fix:function normalizeForMatch(text: string): string { return ` ${text.toLowerCase().replace(/[^a-z0-9]+/g, ' ').trim()} `; } function extractKeywords(text: string): Set<string> { const normalized = normalizeForMatch(text); const hits = new Set<string>(); for (const kw of SKILL_TRIGGER_KEYWORDS) { if (normalized.includes(normalizeForMatch(kw))) hits.add(kw); } return hits; }
Summary
- Review mode: follow-up after new commits
- 1 issue found in the updated diff.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| const lower = text.toLowerCase(); | ||
| const hits = new Set<string>(); | ||
| for (const kw of SKILL_TRIGGER_KEYWORDS) { | ||
| if (lower.includes(kw)) hits.add(kw); |
Contributor
There was a problem hiding this comment.
[Minor] includes does substring matching, so triggers like slide can fire on unrelated words (e.g. landslide) and inject an unintended skill.
Suggested fix:
const normalized = ` ${text.toLowerCase().replace(/[^a-z0-9]+/g, ' ').trim()} `;
if (normalized.includes(` ${kw} `)) hits.add(kw);
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#33 added the skills loader and four builtin skills (data-viz-recharts, frontend-design-anti-slop, mobile-mock, pitch-deck) but the generation pipeline never called the loader nor injected skill bodies into the system prompt — so a user asking for a dashboard never got the data-viz-recharts skill applied.
This PR wires the loader into
generate():loadBuiltinSkills()resolves the bundledbuiltin/dir viaimport.meta.url.matchSkillsToPrompt(skills, userPrompt)(new, inpackages/providers/src/skill-injector.ts) does a lexical keyword match between the prompt and each skill's frontmatter description. Excludes the word "design" on purpose — it's in nearly every prompt.generate()calls the loader + matcher each request, then passes matched skill bodies through the existingcomposeSystemPrompt({ skills })slot. Loader failure is best-effort: log and continue with no skills.apply_commentandsystemPromptoverrides remain skill-free (no behaviour change there).No new dependencies. Does not touch the #43 craft-directives section.
Compatibility / upgradeability / no bloat / elegance
skills: <count>onstep=build_request.ok.as constarray, easy to extend per future skill.Test plan
pnpm --filter @open-codesign/core test— 41 generate tests pass, including 3 new skill-injection cases (matched, unmatched, loader failure)pnpm --filter @open-codesign/providers test— 36 tests passpnpm typecheck— greenpnpm lint— no new errors (10 pre-existing complexity warnings unchanged)pnpm dev, prompt "make a dashboard", confirm data-viz-recharts body appears in the request log