Skip to content

feat(core): Skills system foundation — loader + 4 starter skills#33

Merged
hqhq1025 merged 6 commits intomainfrom
wt/feat-skills
Apr 19, 2026
Merged

feat(core): Skills system foundation — loader + 4 starter skills#33
hqhq1025 merged 6 commits intomainfrom
wt/feat-skills

Conversation

@hqhq1025
Copy link
Copy Markdown
Collaborator

Summary

First PR of 3 implementing the Skills system (research 12).

This PR delivers the backend foundation only:

  • SKILL.md loader (file-based, no new deps; tiny inline frontmatter parser)
  • Schema + types (zod, schemaVersion 1)
  • 4 built-in starter skills (Apache-2.0 self-written, no Anthropic SKILL.md text copied)
  • Provider-agnostic skill injector (packages/providers)

Not in scope (follow-up PRs):

  • Settings → Skills tab UI (PR-B)
  • Wiring loaded skills into the actual generation pipeline (PR-C)

Architecture note

SkillFrontmatterV1 and LoadedSkill are defined in packages/shared (not packages/core) to avoid a circular dependency (core depends on providers; providers now depends on shared for the skill types). packages/core/src/skills/types.ts re-exports from shared for convenience.

Built-in starters

  • frontend-design-anti-slop — design quality guard (oklch / no Inter / asymmetry)
  • pitch-deck — slide ratio, whitespace, one-claim-per-slide
  • data-viz-recharts — chart palette + recharts patterns
  • mobile-mock — viewport ratio + 44pt touch targets

No new dependencies

Inline YAML subset parser (~100 lines) handles: folded/literal block scalars, nested block mappings, inline + block sequences. No gray-matter added.

Test plan

  • loader: 4 starters load with correct frontmatter
  • loader: project overrides user overrides builtin (same id)
  • loader: missing frontmatter fields get zod defaults
  • loader: description > 1536 chars → skill silently skipped
  • loader: malformed .md doesn't crash; other skills still load
  • injector: produces single system message block; correct order
  • injector: wildcard provider matches any provider
  • injector: prefix scope prepends to first user message
  • injector: returns original array when no active skills
  • injector: does not mutate the original messages array

Compatibility

  • Compat: builds on existing providers/shared/core packages, no breaking changes
  • Upgradeability: schemaVersion 1 on frontmatter + LoadedSkill for forward migration
  • No bloat: zero new runtime deps
  • Elegant: pure functions throughout, no global state

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Invalid/malformed skills are silently dropped, violating the no-silent-fallback constraint and making configuration errors hard to diagnose in UI flows. Evidence packages/core/src/skills/loader.ts:226 (same pattern at :190, :214).
    Suggested fix:

    const errors: string[] = [];
    // ...on parse/validation/read failures
    errors.push(`[skills] Invalid frontmatter in ${filePath}: ${result.error.message}`);
    // after loop
    if (errors.length > 0) {
      throw new CodesignError(
        `Skill loading failed:\n${errors.join('\n')}`,
        'SKILL_LOAD_FAILED',
      );
    }
  • [Major] Injection scope is chosen from only the first active skill, so mixed-scope active skills can be injected into the wrong channel depending on array order. Evidence packages/providers/src/skill-injector.ts:85.
    Suggested fix:

    const scopes = new Set(active.map((s) => s.frontmatter.trigger?.scope ?? 'system'));
    if (scopes.size > 1) {
      throw new CodesignError(
        'Mixed skill scopes are not supported in one request',
        'SKILL_SCOPE_CONFLICT',
      );
    }
    const scope = [...scopes][0] ?? 'system';

Summary

  • Review mode: initial
  • Not found in repo/docs: docs/VISION.md and docs/PRINCIPLES.md (mentioned by process docs), so policy-conformance checks were limited to CLAUDE.md + changed code.

Testing

  • Not run (automation)
  • Suggested additions: Vitest for mixed active scopes conflict behavior, and Vitest for YAML literal block scalar (|) newline-preservation behavior.

open-codesign Bot

Comment thread packages/core/src/skills/loader.ts Outdated

const result = SkillFrontmatterV1.safeParse(raw_fm);
if (!result.success) {
console.warn(`[skills] Invalid frontmatter in ${filePath}:`, result.error.issues);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently drops invalid skills with only a console warning (continue). In this project, silent fallbacks are disallowed. Please return/throw structured load errors so the caller can surface them in UI.

const errors: string[] = [];
errors.push(`[skills] Invalid frontmatter in ${filePath}: ${result.error.message}`);
if (errors.length > 0) throw new CodesignError('Skill loading failed', 'SKILL_LOAD_FAILED');

// Use the scope of the first active skill as the injection strategy.
// All skills in a single generation share one scope; mixing scopes is not
// supported — the system setting wins.
const scope = active[0]?.frontmatter.trigger?.scope ?? 'system';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

active[0] makes injection behavior order-dependent when active skills mix system and prefix. Please make this deterministic by rejecting mixed scopes (or explicitly prioritizing one scope).

const scopes = new Set(active.map((s) => s.frontmatter.trigger?.scope ?? 'system'));
if (scopes.size > 1) throw new CodesignError('Mixed skill scopes are not supported', 'SKILL_SCOPE_CONFLICT');
const scope = [...scopes][0] ?? 'system';

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Non-ENOENT directory read failures are silently ignored, which violates the no-silent-fallback constraint and can hide real filesystem/config errors. Evidence packages/core/src/skills/loader.ts:188 and packages/core/src/skills/loader.ts:192.
    Suggested fix:

    let entries: string[];
    try {
      entries = await readdir(dir);
    } catch (err) {
      if ((err as NodeJS.ErrnoException)?.code === 'ENOENT') return [];
      throw new CodesignError(
        `Could not read skills directory ${dir}: ${err instanceof Error ? err.message : String(err)}`,
        'SKILL_LOAD_FAILED',
        { cause: err },
      );
    }
  • [Major] Injection scope is still order-dependent (active[0]), so mixed-scope active skills can be injected into the wrong channel depending on array order. Evidence packages/providers/src/skill-injector.ts:85.
    Suggested fix:

    const scopes = new Set(active.map((s) => s.frontmatter.trigger?.scope ?? 'system'));
    if (scopes.size > 1) {
      throw new CodesignError(
        'Mixed skill scopes are not supported in one request',
        'SKILL_SCOPE_CONFLICT',
      );
    }
    const scope = [...scopes][0] ?? 'system';
  • [Minor] Literal block scalars (|) are flattened into one line, so YAML semantics do not match the parser contract and multiline content is lost. Evidence packages/core/src/skills/loader.ts:60.
    Suggested fix:

    function parseBlockScalar(
      lines: string[],
      start: number,
      baseIndent: number,
      style: 'folded' | 'literal',
    ): [string, number] {
      const blockLines: string[] = [];
      let i = start;
      while (i < lines.length) {
        const next = lines[i] ?? '';
        if (next.trim() === '') {
          blockLines.push('');
          i++;
          continue;
        }
        if (indentOf(next) <= baseIndent) break;
        blockLines.push(next.trim());
        i++;
      }
      return [style === 'literal' ? blockLines.join('\n') : blockLines.join(' '), i];
    }

Summary

  • Review mode: follow-up after new commits
  • Prior no-silent-fallback finding in loader validation is fixed, but silent fallback still exists for non-ENOENT directory read errors.
  • Not found in repo/docs: docs/VISION.md and docs/PRINCIPLES.md.

Testing

  • Not run (automation)
  • Suggested tests: (1) loadSkillsFromDir throws on EACCES/non-ENOENT readdir errors, (2) mixed system+prefix active skills throw deterministic conflict error, (3) | block scalar preserves newlines.

open-codesign Bot

Comment thread packages/core/src/skills/loader.ts Outdated
let entries: string[];
try {
entries = await readdir(dir);
} catch {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch currently converts any readdir failure (including permission/IO errors) into an empty skill set. That is a silent fallback and can mask real configuration failures. Please treat only ENOENT as optional-missing and throw SKILL_LOAD_FAILED for all other error codes.

// Use the scope of the first active skill as the injection strategy.
// All skills in a single generation share one scope; mixing scopes is not
// supported — the system setting wins.
const scope = active[0]?.frontmatter.trigger?.scope ?? 'system';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

active[0] still makes scope selection order-dependent. If one active skill is system and another is prefix, behavior changes based on array order. Please detect mixed scopes and throw a deterministic conflict error (or explicitly resolve with a fixed precedence).

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBlockScalar always uses join(' '), so literal style (|) loses newlines. Since the parser contract says literal/folded are supported, this should preserve line breaks for | and only fold for >.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] Missing DCO sign-off on latest commit — this violates the repo hard constraint and can block merge. Evidence: commit 1a21df570e740f4cd3d4a4cf76b27a5de4b5ba77 message has no Signed-off-by: trailer.
    Suggested fix:

    git commit --amend -s --no-edit
    git push --force-with-lease
  • [Major] Mixed-scope skill injection is order-dependent, so behavior can change based on array order when both system and prefix skills are active, causing nondeterministic prompt shaping. Evidence packages/providers/src/skill-injector.ts:85.
    Suggested fix:

    const scopes = new Set(active.map((s) => s.frontmatter.trigger?.scope ?? 'system'));
    if (scopes.size > 1) {
      throw new CodesignError(
        'Mixed skill scopes are not supported in one request',
        'SKILL_SCOPE_CONFLICT',
      );
    }
    const scope = [...scopes][0] ?? 'system';
  • [Minor] Literal block scalar semantics are lost (| is flattened), which contradicts the parser contract and can corrupt multiline skill frontmatter values. Evidence packages/core/src/skills/loader.ts:60.
    Suggested fix:

    const isLiteral = afterTrimmed.startsWith('|');
    // for '|' keep newlines, for '>' fold to spaces
    return [isLiteral ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];

Summary

  • Review mode: follow-up after new commits
  • Two prior issues remain unresolved in current head (scope conflict handling and | scalar preservation).
  • Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.

Testing

  • Not run (automation)
  • Suggested tests: mixed system+prefix active skills should throw deterministic conflict; literal | scalar should preserve line breaks.

open-codesign Bot

// Use the scope of the first active skill as the injection strategy.
// All skills in a single generation share one scope; mixing scopes is not
// supported — the system setting wins.
const scope = active[0]?.frontmatter.trigger?.scope ?? 'system';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using active[0] makes scope resolution order-dependent when both system and prefix skills are active. Please detect mixed scopes and throw a deterministic conflict error, e.g.

const scopes = new Set(active.map((s) => s.frontmatter.trigger?.scope ?? 'system'));
if (scopes.size > 1) throw new CodesignError('Mixed skill scopes are not supported in one request', 'SKILL_SCOPE_CONFLICT');

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBlockScalar always flattens with spaces, so literal style (|) loses newlines. Please preserve line breaks for | and only fold for >, for example:

const isLiteral = afterTrimmed.startsWith('|');
return [isLiteral ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Mixed-scope skill injection is order-dependent, which can change prompt-shaping behavior based only on array order when both system and prefix skills are active. This makes behavior nondeterministic and can silently route instructions to the wrong channel. Evidence packages/providers/src/skill-injector.ts:85.
    Suggested fix:

    const scopes = new Set(active.map((s) => s.frontmatter.trigger?.scope ?? 'system'));
    if (scopes.size > 1) {
      throw new CodesignError(
        'Mixed skill scopes are not supported in one request',
        'SKILL_SCOPE_CONFLICT',
      );
    }
    const scope = [...scopes][0] ?? 'system';
  • [Minor] Literal block scalar (|) semantics are not preserved in frontmatter parsing; multiline values are always flattened with spaces. This can corrupt user-authored skill metadata that depends on newline preservation. Evidence packages/core/src/skills/loader.ts:60.
    Suggested fix:

    function parseBlockScalar(
      lines: string[],
      start: number,
      baseIndent: number,
      mode: 'folded' | 'literal',
    ): [string, number] {
      // ... collect blockLines
      return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];
    }

Summary

  • Review mode: follow-up after new commits
  • Two issues remain in current head: mixed-scope conflict handling and | literal scalar preservation.
  • Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.

Testing

  • Not run (automation)
  • Suggested tests: mixed system + prefix active skills should throw SKILL_SCOPE_CONFLICT; literal | block scalar should preserve line breaks.

open-codesign Bot

// Use the scope of the first active skill as the injection strategy.
// All skills in a single generation share one scope; mixing scopes is not
// supported — the system setting wins.
const scope = active[0]?.frontmatter.trigger?.scope ?? 'system';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

active[0] makes scope resolution order-dependent when active skills contain both system and prefix scopes. Please detect mixed scopes and throw a deterministic SKILL_SCOPE_CONFLICT error before choosing the injection strategy.

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBlockScalar() always flattens with spaces, so YAML literal style (|) loses newlines. Please preserve line breaks for | and only fold whitespace for > style scalars.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Mixed-scope skills are merged into one block and injected using only the first skill's scope, so skills declaring the other scope are silently injected into the wrong channel. This can materially change model behavior and violates each skill's trigger.scope contract. Evidence packages/providers/src/skill-injector.ts:105, packages/providers/src/skill-injector.ts:110.
    Suggested fix:

    const systemSkills = active.filter((s) => (s.frontmatter.trigger?.scope ?? 'system') === 'system');
    const prefixSkills = active.filter((s) => (s.frontmatter.trigger?.scope ?? 'system') === 'prefix');
    
    let out = baseMessages;
    if (systemSkills.length > 0) out = prependSystemContent(out, buildSkillBlock(systemSkills));
    if (prefixSkills.length > 0) out = prependUserContent(out, buildSkillBlock(prefixSkills));
    return out;
  • [Minor] YAML literal block style (|) is not preserved: parseBlockScalar always joins with spaces, collapsing line breaks. This corrupts valid frontmatter values that intentionally use literal multiline text. Evidence packages/core/src/skills/loader.ts:60.
    Suggested fix:

    function parseBlockScalar(
      lines: string[],
      start: number,
      baseIndent: number,
      mode: 'folded' | 'literal',
    ): [string, number] {
      // ...collect blockLines
      return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];
    }

Summary

  • Review mode: follow-up after new commits
  • Two issues remain in current head: mixed-scope injection collapses per-skill scope semantics; YAML literal block style is not preserved.

Testing

  • Not run (automation)
  • Suggested tests: mixed system + prefix skills should inject into both channels deterministically; | scalar parsing should preserve embedded newlines.

open-codesign Bot

// Mixed `system`/`prefix` scopes resolve to the highest-precedence skill
// (project > user > builtin, then alphabetical), so the chosen channel is a
// function of the skill set rather than caller array order.
const scope = active[0]?.frontmatter.trigger?.scope ?? 'system';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

active[0] chooses a single scope for the whole merged block, so any skill with the other scope is injected into the wrong channel. Please split active by trigger.scope and inject each block to its own channel (system and/or prefix) instead of collapsing to one scope.

const systemSkills = active.filter((s) => (s.frontmatter.trigger?.scope ?? 'system') === 'system');
const prefixSkills = active.filter((s) => (s.frontmatter.trigger?.scope ?? 'system') === 'prefix');

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBlockScalar() always flattens with spaces, so YAML literal style (|) loses line breaks. Please preserve newlines for | and fold only > style scalars.

return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] YAML literal block style (|) is not preserved: parseBlockScalar() always joins lines with spaces, which collapses intended newlines and changes valid frontmatter semantics. Evidence packages/core/src/skills/loader.ts:60, packages/core/src/skills/loader.ts:145.
    Suggested fix:
    function parseBlockScalar(
      lines: string[],
      start: number,
      baseIndent: number,
      mode: 'folded' | 'literal',
    ): [string, number] {
      // ... collect blockLines
      return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];
    }
    
    // call site
    const mode = afterTrimmed.trim().startsWith('|') ? 'literal' : 'folded';
    const [value, nextI] = parseBlockScalar(lines, i, baseIndent, mode);

Summary

  • Review mode: follow-up after new commits
  • 1 issue remains in the current head: literal YAML block (|) handling is lossy.
  • Context files docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)
  • Suggested tests: add a loader test asserting description: | preserves embedded \n between lines.

open-codesign Bot

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBlockScalar() always returns blockLines.join(' '), which collapses YAML literal scalars (|) into a single line. This is lossy for valid frontmatter content.

Suggested fix:

function parseBlockScalar(lines: string[], start: number, baseIndent: number, mode: 'folded' | 'literal'): [string, number] {
  // ...
  return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];
}

const mode = afterTrimmed.trim().startsWith('|') ? 'literal' : 'folded';
const [value, nextI] = parseBlockScalar(lines, i, baseIndent, mode);

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] YAML literal block (|) semantics are still lost in frontmatter parsing — parseBlockScalar() always joins lines with spaces, so literal blocks no longer preserve line breaks, which can change skill descriptions/instructions unexpectedly. Evidence packages/core/src/skills/loader.ts:60, packages/core/src/skills/loader.ts:145.
    Suggested fix:
    function parseBlockScalar(
      lines: string[],
      start: number,
      baseIndent: number,
      mode: 'folded' | 'literal',
    ): [string, number] {
      const blockLines: string[] = [];
      let i = start;
      while (i < lines.length) {
        const next = lines[i] ?? '';
        if (next.trim() === '') {
          blockLines.push('');
          i++;
          continue;
        }
        if (indentOf(next) <= baseIndent) break;
        blockLines.push(next.trim());
        i++;
      }
      return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];
    }
    
    // call site
    const mode = afterTrimmed.startsWith('|') ? 'literal' : 'folded';
    const [value, nextI] = parseBlockScalar(lines, i, baseIndent, mode);

Summary

  • Review mode: follow-up after new commits
  • 1 issue found in added code: literal-block frontmatter parsing is lossy.
  • Context files docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBlockScalar() currently collapses literal (|) block scalars by joining with spaces. Please preserve | blocks with \n joins while keeping folded (>) as space-joined.

- packages/shared/src/skills.ts: SkillFrontmatterV1 zod schema + LoadedSkill
  interface (canonical location; avoids core→providers circular dep)
- packages/core/src/skills/: inline YAML frontmatter parser, loadSkillsFromDir,
  loadAllSkills with 3-tier priority (project > user > builtin), loader tests
- packages/core/src/skills/builtin/: 4 starter skills (frontend-design-anti-slop,
  pitch-deck, data-viz-recharts, mobile-mock) — self-written, Apache-2.0, no
  Anthropic SKILL.md text copied
- packages/providers/src/skill-injector.ts: injectSkillsIntoMessages, pure,
  supports system and prefix scope, provider-wildcard matching, injector tests

No new runtime deps added. Inline YAML parser (~100 lines) handles folded
scalars, nested mappings, inline + block sequences.

Signed-off-by: hqhq1025 <[email protected]>
… drop

Replace the three silent continue/console.warn paths in loadSkillsFromDir
with error collection; after processing all files, throw CodesignError
'SKILL_LOAD_FAILED' if any errors were accumulated. Update tests to
expect the throw, and add a new loadAllSkills test for a broken skill
(missing description field).

Signed-off-by: hqhq1025 <[email protected]>
Mixed-scope skill injection was order-dependent: whichever skill
appeared first in the caller's array decided whether the block went
into the system prompt or the first user message. That made
prompt-shaping behaviour a function of loader iteration order rather
than the active skill set.

Sort skills by source precedence (project > user > builtin) and then
alphabetical name before building the block. The chosen scope, the
concatenated body, and the resulting prompt blob are now byte-identical
across runs regardless of input order, which also stabilises prompt
caching and snapshot tests.

Adds a vitest case that feeds three random permutations of a five-skill
fixture and asserts the resulting system content is byte-identical to
the canonical sort, plus a mixed system/prefix scope test that confirms
the higher-precedence skill picks the channel.

Signed-off-by: hqhq1025 <[email protected]>
Previously a mixed system/prefix skill set was concatenated into a single
block and injected via the highest-precedence skill's channel, silently
routing the rest into the wrong channel and violating each skill's
trigger.scope contract. Now we partition active skills by scope and
inject system-scope skills into the system message and prefix-scope
skills into the first user message, preserving canonical order within
each channel.

Signed-off-by: hqhq1025 <[email protected]>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] YAML literal block (|) semantics are not preserved — parseBlockScalar() always joins block lines with spaces, so literal blocks lose intended newlines and can alter skill instructions before prompt injection. Evidence packages/core/src/skills/loader.ts:46, packages/core/src/skills/loader.ts:60, packages/core/src/skills/loader.ts:145.
    Suggested fix:
    function parseBlockScalar(
      lines: string[],
      start: number,
      baseIndent: number,
      mode: 'folded' | 'literal',
    ): [string, number] {
      const blockLines: string[] = [];
      let i = start;
      while (i < lines.length) {
        const next = lines[i] ?? '';
        if (next.trim() === '') {
          blockLines.push('');
          i++;
          continue;
        }
        if (indentOf(next) <= baseIndent) break;
        blockLines.push(next.trim());
        i++;
      }
      return [mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim(), i];
    }
    
    const mode = afterTrimmed.startsWith('|') ? 'literal' : 'folded';
    const [value, nextI] = parseBlockScalar(lines, i, baseIndent, mode);

Summary
Review mode: follow-up after new commits

  • 1 issue found in changed lines.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread packages/core/src/skills/loader.ts Outdated
blockLines.push(next.trim());
i++;
}
return [blockLines.join(' ').trim(), i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literal (|) YAML blocks should preserve newlines, but this parser path currently collapses lines with spaces.

Suggested minimal change:

const mode = afterTrimmed.startsWith('|') ? 'literal' : 'folded';
const [value, nextI] = parseBlockScalar(lines, i, baseIndent, mode);
// ...inside parseBlockScalar return mode === 'literal' ? blockLines.join('\n').trim() : blockLines.join(' ').trim();

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • No high-confidence issues found in added/modified lines of the current diff.

Summary

  • Review mode: follow-up after new commits
  • No new actionable issues detected in this revision.
  • Residual risk/testing gap: unable to execute Vitest/Playwright in this runner (pnpm unavailable), so runtime behavior was validated by static analysis only.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

@hqhq1025 hqhq1025 merged commit 44556d0 into main Apr 19, 2026
6 checks passed
@hqhq1025 hqhq1025 deleted the wt/feat-skills branch April 19, 2026 04:07
hqhq1025 added a commit that referenced this pull request Apr 19, 2026
* feat(core): skills loader + 4 starter skills + provider injector (PR-A)

- packages/shared/src/skills.ts: SkillFrontmatterV1 zod schema + LoadedSkill
  interface (canonical location; avoids core→providers circular dep)
- packages/core/src/skills/: inline YAML frontmatter parser, loadSkillsFromDir,
  loadAllSkills with 3-tier priority (project > user > builtin), loader tests
- packages/core/src/skills/builtin/: 4 starter skills (frontend-design-anti-slop,
  pitch-deck, data-viz-recharts, mobile-mock) — self-written, Apache-2.0, no
  Anthropic SKILL.md text copied
- packages/providers/src/skill-injector.ts: injectSkillsIntoMessages, pure,
  supports system and prefix scope, provider-wildcard matching, injector tests

No new runtime deps added. Inline YAML parser (~100 lines) handles folded
scalars, nested mappings, inline + block sequences.

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): skills loader collects errors and throws instead of silent drop

Replace the three silent continue/console.warn paths in loadSkillsFromDir
with error collection; after processing all files, throw CodesignError
'SKILL_LOAD_FAILED' if any errors were accumulated. Update tests to
expect the throw, and add a new loadAllSkills test for a broken skill
(missing description field).

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): propagate non-ENOENT errors in skills loader

Signed-off-by: hqhq1025 <[email protected]>

* fix(providers): sort skills into canonical order before injection

Mixed-scope skill injection was order-dependent: whichever skill
appeared first in the caller's array decided whether the block went
into the system prompt or the first user message. That made
prompt-shaping behaviour a function of loader iteration order rather
than the active skill set.

Sort skills by source precedence (project > user > builtin) and then
alphabetical name before building the block. The chosen scope, the
concatenated body, and the resulting prompt blob are now byte-identical
across runs regardless of input order, which also stabilises prompt
caching and snapshot tests.

Adds a vitest case that feeds three random permutations of a five-skill
fixture and asserts the resulting system content is byte-identical to
the canonical sort, plus a mixed system/prefix scope test that confirms
the higher-precedence skill picks the channel.

Signed-off-by: hqhq1025 <[email protected]>

* fix(providers): inject mixed-scope skills into separate channels

Previously a mixed system/prefix skill set was concatenated into a single
block and injected via the highest-precedence skill's channel, silently
routing the rest into the wrong channel and violating each skill's
trigger.scope contract. Now we partition active skills by scope and
inject system-scope skills into the system message and prefix-scope
skills into the first user message, preserving canonical order within
each channel.

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): preserve newlines in YAML literal block scalar (PR #33 codex Minor)

---------

Signed-off-by: hqhq1025 <[email protected]>
hqhq1025 added a commit that referenced this pull request Apr 19, 2026
* feat(core): system prompt — adopt Claude Design patterns

Adds a new prompt section that embeds high-leverage craft directives
paraphrased from patterns observed in the publicly leaked Claude Design
system prompt. Direct response to feedback that generated designs are
sparse, generic, and monotone.

The new section sits between tweaks-protocol and anti-slop in the
composer, applies to all create/revise/tweak modes, and codifies:

- silent artifact-type classification (landing / dashboard / case study /
  pricing / deck / etc.) controlling section ladder and density target
- density floor — default to "rich", drop only on explicit "minimal"
- real, specific content — concrete bans on common placeholder strings
- before/after side-by-side rendering when comparison is implied
- big-number visual blocks (display weight + label + delta + sparkline)
- three-family typography ladder (display + sans + mono)
- dark-theme warmth requirements (accent + gradient + transparent borders)
- SVG monogram/wordmark for logos (no emoji, no flat circles)
- distinguished customer-quote treatment
- single-page structure ladder (hero → trust → 3-5 sections → focal →
  closing CTA), with dashboard and slide deck substitutions

Directives are independently authored — no original Claude Design text is
reproduced verbatim. Attribution and the underlying structural analysis
live in docs/research/15-claude-design-prompts.md (gitignored, internal).

Vitest coverage:
- new test asserts all ten directive headers appear in the create-mode
  composed prompt
- existing drift test ensures the new .txt and the inlined TS constant
  stay byte-identical

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): rename 'Claude-Design-style' → 'Craft directives' to remove provenance comment per codex review

Signed-off-by: hqhq1025 <[email protected]>

* test(core): cover revise mode for craft directives (PR #43 codex Minor)

* fix(desktop): replace hardcoded text-[Npx] with --text-* tokens in preview/inline-comment (#57)

Codex has flagged this exact pattern as a Blocker on PRs #50 and #51 ("hardcoded
pixel font size violates token-only UI constraint"). Sweeps the chat-adjacent
surface that is not covered by any in-flight worktree.

Mappings:
- text-[11px] -> text-[var(--text-xs)] (12px, closest token)
- text-[12px] -> text-[var(--text-xs)]
- text-[13px] -> text-[var(--text-sm)]

Files touched: InlineCommentComposer.tsx, PreviewToolbar.tsx, PreviewPane.tsx.
The bg-[rgba(255,255,255,0.88)] frosted pill in PreviewPane is left for a
follow-up because it requires a new translucent surface token.

* fix(desktop): localize Toast dismiss button aria-label (#56)

Codex flagged a recurring i18n violation pattern across PRs #48 and #49: hardcoded
English ARIA strings bypass the i18n layer and ship inconsistent screen-reader
output to localized users. The toast close button was the last remaining
hardcoded aria-label outside the onboarding flow.

- Add common.dismissNotification key (en + zh-CN)
- Wire useT() into ToastItem and consume the new key

* fix(desktop): tokenize ConnectionStatusDot tooltip values (#58)

* chore(desktop): biome auto-format InlineCommentComposer (unblock pre-push)

* fix(desktop): tokenize ConnectionStatusDot tooltip hardcoded values

* fix(desktop): tokenize LanguageToggle hardcoded sizes/spacing (#60)

* fix(desktop): tokenize LanguageToggle hardcoded sizes/spacing

* chore(desktop): biome format InlineCommentComposer (drive-by, unblocks pre-push)

* [Claude Design adoption] PR-B: examples gallery (#50)

* feat(hub): examples gallery as first-class section

PR-B from doc 28 (Claude Design adoption). Ships eight curated examples
(cosmic animation, organic loaders, landing page, case study, dashboard,
pitch slide, welcome email, mobile habit tracker) with stylised inline
SVG thumbnails, an `ExamplesTab` view component, and full en + zh-CN
translations for every title, description, category label, and surrounding
chrome.

The tab is self-contained: a single `onUsePrompt` prop hands the chosen
example back to the host so PR-A can wire it into the hub without further
plumbing. No App.tsx changes here — independent of PR-A landing first.

Compatibility ✅  Upgradeability ✅  No bloat ✅  Elegance ✅

Signed-off-by: hqhq1025 <[email protected]>

* fix(hub): use --font-size-body-xs token for example card category badge

Replaces hardcoded text-[10px] with the existing --font-size-body-xs
typography token (11px). Resolves Codex token-only UI constraint blocker
on PR #50.

Signed-off-by: hqhq1025 <[email protected]>

* fix(templates): throw on missing locale content for examples (no silent fallback)

Codex blocker on PR #50: getExamples returned `{ title: id, description: '' }`
when both the requested locale and the en fallback lacked an entry, shipping
degraded UI without surfacing the bug.

- Throw a descriptive Error when no registry has the example id
- Add vitest case asserting the throw path
- Drive-by: biome format apps/desktop/src/renderer/src/components/InlineCommentComposer.tsx
  (pre-existing format drift on main blocking pre-push)

Signed-off-by: hqhq1025 <[email protected]>

---------

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): tokenize PreviewToolbar values missed by #57 (#62)

* fix(desktop): tokenize remaining PreviewToolbar hardcoded values missed by #57

Replaces three remaining hardcoded px values in PreviewToolbar with design
tokens to keep the export menu aligned with the token system:
- h-[30px] → h-[var(--size-control-sm)]
- w-[14px] h-[14px] → w-[var(--size-icon-sm)] h-[var(--size-icon-sm)]
- min-w-[200px] → min-w-[var(--size-stage-min)]

Adds a new --size-stage-min (200px) token to packages/ui for menu/popover
minimum widths. The other three font-size values noted in the original
audit were already tokenized by #57.

* chore: format Download icon and silence pre-existing core complexity lint

- Wrap PreviewToolbar Download icon across multiple lines per Biome formatter
- Add biome-ignore for pre-existing noExcessiveCognitiveComplexity in
  packages/core/src/index.ts runModel (blocks pre-push hook; refactor
  tracked separately, mirrors the precedent set in 9051fae)

* fix(desktop): drop unused fileURLToPath import in main entry (#63)

* fix(desktop): PreviewPane hint pill + iframe respect dark mode (#69)

Co-authored-by: Claude <[email protected]>

* fix(desktop): i18n ThemeToggle aria/tooltip strings (#70)

Replace hardcoded English with t() calls; add theme.{toggleAria,switchToLight,switchToDark} to en.json and zh-CN.json.

Co-authored-by: Claude <[email protected]>

* fix(desktop): tokenize raw utilities in preview Loading/Error states (#64)

* fix(desktop): tokenize raw utilities in preview Loading/Error states

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): tokenize space-y-3 in LoadingState skeleton header

Signed-off-by: hqhq1025 <[email protected]>

---------

Signed-off-by: hqhq1025 <[email protected]>

* feat(desktop): Snapshots SQLite schema + IPC handlers (PR-A) (#29)

* feat(desktop): snapshots SQLite + IPC foundation (PR-A of version history)

- packages/shared: DesignSnapshotV1 + DesignV1 Zod schemas, SnapshotCreateInput interface
- apps/desktop: better-sqlite3 persistence (designs + design_snapshots tables, WAL, FK cascade)
  initSnapshotsDb(path) for production, initInMemoryDb() for tests
- snapshots:v1:* IPC handlers: list-designs, create-design, list, get, create, delete
  All reject malformed payloads with CodesignError('IPC_BAD_INPUT')
- preload: window.codesign.snapshots namespace bridged to renderer
- Vitest: 26 new tests across snapshots-db + snapshots-ipc (111 pass total)

New dep: better-sqlite3@^11 (MIT, native, already in CLAUDE.md stack)
No ulid added — using crypto.randomUUID() instead.

PR-B will wire auto-snapshot-on-sendPrompt + history sidebar UI.

Signed-off-by: hqhq1025 <[email protected]>

* style(desktop): fix biome formatting in snapshots-db.test.ts

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): address codex snapshots cascade/parent/sort findings

- Enable PRAGMA foreign_keys = ON in applySchema so ON DELETE CASCADE
  / SET NULL fire in production (previously only the test enabled it
  manually, hiding the regression).
- Constrain design_snapshots.parent_id with a self-referential FK
  (ON DELETE SET NULL) and validate in the IPC layer that parentId
  resolves to a snapshot in the same design — prevents silent history
  corruption from stale or cross-design ids.
- Sort listDesigns by updated_at DESC, created_at DESC so designs
  bubble after new snapshots are added (createSnapshot already bumps
  updated_at).
- Drop the now-redundant manual pragma in the cascade test and add
  coverage for parent SET NULL, cross-design parent rejection, and
  the activity-based design sort.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): reject invalid create-design input instead of silent default

The snapshots:v1:create-design IPC handler coerced empty/non-string payloads to 'Untitled design', hiding caller bugs and violating the no-silent-fallback rule. Reject with IPC_BAD_INPUT instead, drop the matching preload coercion, and update tests to cover the new contract.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): translate SQLite errors to typed IPC contract in snapshots-ipc

Wraps every snapshot DB call in a translateSqliteError helper that maps
better-sqlite3 SqliteError codes to typed CodesignError instances:

- SQLITE_CONSTRAINT_FOREIGNKEY -> IPC_BAD_INPUT
- SQLITE_BUSY / SQLITE_LOCKED  -> IPC_DB_BUSY
- SQLITE_FULL                  -> IPC_DB_FULL
- other                        -> IPC_DB_ERROR (full details logged server-side)

Renderer no longer sees raw provider error strings. Adds vitest cases that
stub better-sqlite3 prepare() to throw each code and assert the right
CodesignError is surfaced.

Signed-off-by: hqhq1025 <[email protected]>

* fix(snapshots-ipc): map SQLite constraint subcodes individually

Bare SQLITE_CONSTRAINT also covers UNIQUE / NOT NULL / CHECK violations,
so translating it as "Parent snapshot does not exist" misled the UI on
unrelated failures. Match each subcode explicitly:

- SQLITE_CONSTRAINT_FOREIGNKEY  -> IPC_BAD_INPUT, parent-snapshot message
- SQLITE_CONSTRAINT_UNIQUE/PK   -> IPC_CONFLICT, "Snapshot already exists"
- SQLITE_CONSTRAINT_NOTNULL/CHECK -> IPC_BAD_INPUT, neutral constraint message
- bare SQLITE_CONSTRAINT (no suffix) -> generic IPC_DB_ERROR

Adds vitest coverage for each new branch.

Signed-off-by: hqhq1025 <[email protected]>

* fix(snapshots-ipc): clarify FK error covers design and parent

SQLITE_CONSTRAINT_FOREIGNKEY fires for both a missing design_id and a
missing parent_id, but the previous translation always reported "Parent
snapshot does not exist" — leading contributors to look for the wrong
cause. Key the FK message by call-site context and use a message that
names both columns ("Referenced design or parent snapshot does not
exist"); fall back to a generic "Referenced item does not exist" for
unmapped contexts.

Also extracts the static SQLite-code lookup into a small helper to keep
translateSqliteError below the cognitive-complexity threshold.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): schema-version snapshots:v1 IPC payloads

Every snapshots:v1:* object payload now carries `schemaVersion: 1`, both
on the wire (preload bridge) and in the main-process parser. Mismatched
or missing versions throw IPC_BAD_INPUT so future handler revisions can
break cleanly instead of silently mis-parsing legacy callers.

- snapshots-ipc.ts: requireSchemaV1() helper applied to list-designs,
  list, get, create, delete, create-design (now object-shaped).
- preload/index.ts: every snapshots invoke wraps the payload with
  { schemaVersion: 1, ... }.
- snapshots-ipc.test.ts: updated existing fixtures via a v1() helper and
  added a parameterised gating suite that asserts every channel rejects
  missing and mismatched schemaVersion values.

Addresses Codex Major review on PR #29.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): degrade gracefully when snapshots DB init fails

Previously initSnapshotsDb() ran inside app.whenReady() without a guard,
so any open/migration/native-binding failure rejected the boot promise
and prevented createWindow() from ever firing — the user got a silent
no-window app.

Add safeInitSnapshotsDb() wrapper that captures the error, then in main
boot: log it with full stack via electron-log, surface it through
dialog.showErrorBox, skip registerSnapshotsIpc, and continue to open
the window. Snapshot-dependent renderer features will fail loudly via
their IPC channels, but the rest of the app stays usable.

Also reset the singleton if applySchema throws so a retry can recover
instead of returning a half-open DB handle.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): typed SNAPSHOTS_UNAVAILABLE stub when snapshots DB init fails

When safeInitSnapshotsDb fails at boot, main/index.ts previously skipped
registerSnapshotsIpc entirely. Renderer calls to window.codesign.snapshots.*
then surfaced as Electron's opaque "No handler registered" rejection,
violating the no-silent-fallback / error-context requirement (PR #29 codex
Major).

Install registerSnapshotsUnavailableIpc stubs for the same channel set so
every renderer call rejects with a typed CodesignError carrying code
SNAPSHOTS_UNAVAILABLE and a message pointing the user at Settings → Storage.
The channel list is exported (SNAPSHOTS_CHANNELS_V1) so the test can pin
it to the live registration set and prevent drift.

Adds vitest coverage that asserts SNAPSHOTS_UNAVAILABLE is thrown for every
channel and that the stub set matches registerSnapshotsIpc exactly.

Signed-off-by: hqhq1025 <[email protected]>

---------

Signed-off-by: hqhq1025 <[email protected]>

* feat(exporters): Markdown export of generated artifact (#66)

* feat(exporters): add Markdown export with simple HTML→MD conversion

Adds a tier-1 Markdown exporter (.md with YAML frontmatter carrying
schemaVersion: 1). Conversion is a small set of regex passes covering
h1..h6, p, a, img, ul/ol, strong/em, code/pre — anything else is
stripped. Zero new runtime deps.

Wired through the existing exporter IPC + Preview toolbar Export menu,
with en + zh-CN i18n strings.

Compatibility ✅  Upgradeability ✅  No bloat ✅  Elegance ✅

Signed-off-by: hqhq1025 <[email protected]>

* fix(exporters): allowlist URL schemes in markdown export

Sanitize <a href> and <img src> during htmlToMarkdown so unsafe schemes
(javascript:, vbscript:, file:, non-image data:, etc.) cannot ride into
the exported .md and execute via downstream renderers. Allow http(s),
mailto, relative URLs, fragments; permit data:image/* only on <img>.
Unsafe links collapse to plain text, unsafe images are dropped.

Also fix the IPC default extension for the markdown format (`design.md`
instead of `design.markdown`) when no defaultFilename is provided.

Addresses codex review on PR #66.

Signed-off-by: hqhq1025 <[email protected]>

* fix(exporters): close encoded-scheme bypass in markdown sanitizeUrl

Decode HTML entities (named, hex, decimal), URL %escapes, and strip
control characters (TAB/CR/LF/etc.) before scheme allowlisting so
payloads like &#x6A;avascript:, %6Aavascript:, JavaScript:, and tab-
prefixed schemes can no longer slip through the link/image sanitizer.

Adds regression tests covering each bypass vector.

Signed-off-by: hqhq1025 <[email protected]>

* fix(exporters): only decode scheme prefix in sanitizeUrl

The previous follow-up ran decodeURIComponent on the entire URL which
both threw on legitimate inputs containing literal `%` (dropping the
link entirely) and rewrote percent-escaped path/query characters such
as %2F or %C3%A9. Restrict entity + percent decoding to the scheme
portion (before the first colon) used solely for the safety check, and
emit the original (control-stripped, trimmed) URL when the scheme is
allowlisted.

Adds regression tests for %2F in query, UTF-8 percent-escapes in path,
literal trailing %, and confirms the existing entity / %-encoded
javascript bypass guards still strip dangerous schemes.

Signed-off-by: hqhq1025 <[email protected]>

---------

Signed-off-by: hqhq1025 <[email protected]>

* [Claude Design adoption] PR-A: designs hub + multi-type create wizard (#51)

* feat(desktop): designs hub + multi-type create wizard (Claude Design adoption PR-A)

Replaces the straight-to-composer launch flow with a designs hub modeled on
Claude Design's Recent / Your designs / Examples / Design systems navigation
plus a typed create wizard (Prototype / Slide deck / From template / Other).

- Hub view with four sibling tabs and a primary "New design" CTA
- Modal create flow; CTA stays disabled until a project name is provided
- Per-type forms; PR-F will fill in the wireframe vs high-fidelity cards
- Examples and Design systems tabs ship as placeholders for PR-B / PR-C
- Project schema lives in shared with schemaVersion=1; persisted to
  localStorage for now (SQLite migration arrives with PR-C)
- Full en + zh-CN coverage; tokens from packages/ui (no hardcoded values)
- Vitest covering the create-draft logic; existing 144-test suite untouched

Compatibility: green - no IPC/main changes, no schema breaks.
Upgradeability: green - schemaVersion field on every Project payload.
No bloat: green - no new dependencies; reuses lucide-react + zustand.
Elegance: green - single store action per intent; per-type forms < 40 LOC.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): surface project storage errors via toast + warn (no silent fallback)

- readStoredProjects/persistProjects now console.warn and return an error
  string instead of swallowing exceptions; createProject pushes an error
  toast on persist failure while keeping in-memory state consistent.
- Validate stored projects with the Project zod schema (safeParse) and
  count rejected records so corrupted entries surface a toast instead of
  silently disappearing.
- openProject resets project-scoped workspace state (messages, preview,
  inputs, generation flags) to prevent cross-project state leakage.
- Add errors.projectStorageFailed i18n key (en + zh-CN).
- Vitest: mock localStorage.setItem to throw, assert toast pushed and the
  new project is still added to in-memory state.

Addresses Codex blocker on PR #51.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): replace hardcoded checkbox sizing with --size-icon-md token

SlideDeckForm checkbox used `w-4 h-4` literals which violate the
token-only UI constraint. Swap to `var(--size-icon-md)` (16px) so the
control scales with centralized theming.

Addresses Codex blocker on PR #51.

Signed-off-by: hqhq1025 <[email protected]>

* chore: clear pre-existing biome errors blocking pre-push hook

- tailwindExtractor: replace non-null assertion with safe cast
- InlineCommentComposer: apply formatter

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): wire ExamplesTab onUsePrompt after rebase onto PR-B

PR-B merged ExamplesTab as a real component requiring an onUsePrompt
callback. After rebase, surface that prop through HubView and have App
prefill the workspace prompt + switch view.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): reset project-scoped workspace state in createProject

createProject was only clearing messages/previewHtml/generationStage,
leaving inputFiles, referenceUrl, selectedElement, lastPromptInput,
and generation/error flags inherited from the previously open project.
Mirror openProject's full reset so a freshly created project starts
with a clean workspace and prompt context.

Adds vitest coverage for the reset.

Refs codex review on PR #51.

Signed-off-by: hqhq1025 <[email protected]>

* fix(ui): replace hardcoded sizing in hub/create with new tokens

PR-A introduced raw arbitrary-value Tailwind classes (360px sidebar,
560px modal, 60ch prose, 240px card minimum, 1px hover lift) that
violate the token-only constraint in CLAUDE.md. Add five tokens to
packages/ui/src/tokens.css and route every call site through them so
themes and density tweaks stay centralized.

---------

Signed-off-by: hqhq1025 <[email protected]>

* feat(core): Skills system foundation — loader + 4 starter skills (#33)

* feat(core): skills loader + 4 starter skills + provider injector (PR-A)

- packages/shared/src/skills.ts: SkillFrontmatterV1 zod schema + LoadedSkill
  interface (canonical location; avoids core→providers circular dep)
- packages/core/src/skills/: inline YAML frontmatter parser, loadSkillsFromDir,
  loadAllSkills with 3-tier priority (project > user > builtin), loader tests
- packages/core/src/skills/builtin/: 4 starter skills (frontend-design-anti-slop,
  pitch-deck, data-viz-recharts, mobile-mock) — self-written, Apache-2.0, no
  Anthropic SKILL.md text copied
- packages/providers/src/skill-injector.ts: injectSkillsIntoMessages, pure,
  supports system and prefix scope, provider-wildcard matching, injector tests

No new runtime deps added. Inline YAML parser (~100 lines) handles folded
scalars, nested mappings, inline + block sequences.

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): skills loader collects errors and throws instead of silent drop

Replace the three silent continue/console.warn paths in loadSkillsFromDir
with error collection; after processing all files, throw CodesignError
'SKILL_LOAD_FAILED' if any errors were accumulated. Update tests to
expect the throw, and add a new loadAllSkills test for a broken skill
(missing description field).

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): propagate non-ENOENT errors in skills loader

Signed-off-by: hqhq1025 <[email protected]>

* fix(providers): sort skills into canonical order before injection

Mixed-scope skill injection was order-dependent: whichever skill
appeared first in the caller's array decided whether the block went
into the system prompt or the first user message. That made
prompt-shaping behaviour a function of loader iteration order rather
than the active skill set.

Sort skills by source precedence (project > user > builtin) and then
alphabetical name before building the block. The chosen scope, the
concatenated body, and the resulting prompt blob are now byte-identical
across runs regardless of input order, which also stabilises prompt
caching and snapshot tests.

Adds a vitest case that feeds three random permutations of a five-skill
fixture and asserts the resulting system content is byte-identical to
the canonical sort, plus a mixed system/prefix scope test that confirms
the higher-precedence skill picks the channel.

Signed-off-by: hqhq1025 <[email protected]>

* fix(providers): inject mixed-scope skills into separate channels

Previously a mixed system/prefix skill set was concatenated into a single
block and injected via the highest-precedence skill's channel, silently
routing the rest into the wrong channel and violating each skill's
trigger.scope contract. Now we partition active skills by scope and
inject system-scope skills into the system message and prefix-scope
skills into the first user message, preserving canonical order within
each channel.

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): preserve newlines in YAML literal block scalar (PR #33 codex Minor)

---------

Signed-off-by: hqhq1025 <[email protected]>

* feat(desktop): viewport switcher (desktop / tablet / mobile with phone bezel) (#32)

* feat(desktop): mobile/tablet/desktop viewport preview with phone bezel

- Add PreviewViewport type and previewViewport/setPreviewViewport to store
- Add PhoneFrame component (CSS-only iPhone bezel, fully tokenised)
- Add viewport switcher (Desktop/Tablet/Mobile) to PreviewToolbar
- PreviewPane renders iframe inside PhoneFrame at 375x812 for mobile,
  centred 768px container for tablet, and full-width for desktop
- Add preview.viewport i18n keys (en + zh-CN)
- Add 4 unit tests for setPreviewViewport in store.test.ts

Signed-off-by: hqhq1025 <[email protected]>

* fix(ui): tokenize phone frame dimensions + give tablet wrapper a height

Replace hard-coded px values in PhoneFrame with CSS custom properties from
packages/ui tokens.css. Add --size-preview-mobile-*, --size-preview-tablet-width,
--radius-phone, and --border-width-strong tokens. Fix tablet branch in
PreviewPane to propagate h-full so the iframe is no longer zero-height.

Signed-off-by: hqhq1025 <[email protected]>

* fix(ui): tokenize preview viewport sizes (mobile/tablet/desktop)

Signed-off-by: hqhq1025 <[email protected]>

* fix(ui): tokenize preview hint badge color/size

Address Codex blocker re-flagged on PR #32:
- Replace hardcoded values in PreviewPane hint badge (left-5/top-5,
  bg-[rgba(255,255,255,0.88)], px-3/py-1, text-[11px]) with
  --space-5/--space-3/--space-1, --color-surface-elevated, --text-xs.
- Replace hardcoded sizes/motion in PreviewToolbar viewport controls
  and Download button (w-[14px], w-[30px], h-[30px], duration-150,
  literal cubic-bezier) with --size-icon-sm, --size-control-xs,
  --duration-fast, --ease-out.
- Add tokens: --color-surface-elevated (light + dark),
  --size-control-xs (30px), --size-icon-sm (14px).

Signed-off-by: hqhq1025 <[email protected]>

* fix(ui): tokenize PhoneFrame border + remaining hardcoded values

Replace hard-coded `outline: '1px solid ...'` with the existing `--shadow-inset-soft` token composed into boxShadow. Removes the last non-tokenized value in PhoneFrame so the component is fully driven by packages/ui tokens.

Signed-off-by: hqhq1025 <[email protected]>

* fix(ui): make PhoneFrame notch overlay click-through (pointer-events: none)

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): add aria-label to viewport switcher buttons

title alone is not a reliable accessible name for screen readers;
add aria-label using the same i18n string so assistive tech announces
Mobile/Tablet/Desktop instead of a generic button.

Signed-off-by: hqhq1025 <[email protected]>

* fix(desktop): tokenize iframe background in mobile preview (PR #32 codex Major)

* fix(desktop): tokenize desktop preview iframe bg + dedup icon size tokens (PR #32 codex follow-up)

---------

Signed-off-by: hqhq1025 <[email protected]>

* fix(core): exclude craft-directives from tweak mode (PR #43 codex Major)

---------

Signed-off-by: hqhq1025 <[email protected]>
Co-authored-by: Claude <[email protected]>
hqhq1025 added a commit that referenced this pull request Apr 19, 2026
#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).
hqhq1025 added a commit that referenced this pull request Apr 19, 2026
* feat(core): wire skills loader into generation pipeline (Skills PR-B)

#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).

* fix(core): surface skill-load failures via warnings + console.warn

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant