Skip to content

feat(templates): starter library + skill templates wiring#86

Closed
hqhq1025 wants to merge 3 commits intomainfrom
wt/loop-feat-starter-library
Closed

feat(templates): starter library + skill templates wiring#86
hqhq1025 wants to merge 3 commits intomainfrom
wt/loop-feat-starter-library

Conversation

@hqhq1025
Copy link
Copy Markdown
Collaborator

落地 4 个 starter 文件(ios-frame, design-canvas, meditation-app-mobile, meditation-app-ipad)+ 扩展 skill loader 支持 templates frontmatter,自动附加 starter 内容到 skill body。详见 docs/research/30-claude-design-extracted-architecture.md。

…frontmatter

Drop four reference starters (ios-frame.jsx, design-canvas.jsx,
meditation-app-mobile.html, meditation-app-ipad.html) into
packages/templates/starters/ with attribution headers, then teach the
skill loader to read frontmatter `templates: [...]` and append the
referenced starter contents to the skill body. mobile-mock now ships
with ios-frame.jsx baked in.
Copy link
Copy Markdown

@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] Template path traversal allows arbitrary file read during skill loading — templates values are used with join(startersDir, file) but never constrained to stay under startersDir, so ../ paths can read unintended files and inject them into skill body context. Evidence: packages/core/src/skills/loader.ts:214
    Suggested fix:

    import { extname, join, normalize, resolve, sep } from 'node:path';
    
    const root = resolve(startersDir);
    const candidate = resolve(root, normalize(file));
    if (!candidate.startsWith(root + sep) && candidate !== root) {
      throw new CodesignError(
        `Invalid starter template path "${file}" in ${skillFile}`,
        'SKILL_LOAD_FAILED',
      );
    }
    const content = await readFile(candidate, 'utf-8');
  • [Major] Missing starter templates are silently ignored, violating project error-surfacing constraint — current behavior logs console.warn and continues, which can hide broken skill config and produce incomplete prompts. Evidence: packages/core/src/skills/loader.ts:218 and packages/core/src/skills/loader.ts:223
    Suggested fix:

    } catch (err) {
      const msg = err instanceof Error ? err.message : String(err);
      throw new CodesignError(
        `Starter template "${file}" referenced by ${skillFile} not found: ${msg}`,
        'SKILL_LOAD_FAILED',
      );
    }

Summary
Review mode: initial.
Found 2 issues introduced by the diff: one security blocker (path traversal / arbitrary file read) and one major constraint violation (silent fallback on missing template files). docs/VISION.md and docs/PRINCIPLES.md were referenced in instructions but are not present in this checkout (Not found in repo/docs).

Testing
Not run (automation). Suggested additions: regression tests for ../ traversal rejection and missing-template hard failure path.

open-codesign Bot

const blocks: string[] = [];
for (const file of templates) {
const path = join(startersDir, file);
let content: string;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

join(startersDir, file) accepts ../ segments from frontmatter and can escape startersDir. Please resolve+normalize and enforce the candidate path stays within startersDir, then throw CodesignError on violation.

} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
console.warn(
`[skills] starter template "${file}" referenced by ${skillFile} not found: ${msg}`,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This catch path logs and continues, which silently degrades skill output when a referenced template is missing. Per project constraints, surface this as an error (CodesignError) instead of warning-and-skip.

`appendStarterTemplates` previously joined user-controlled `templates[]`
frontmatter strings onto `STARTERS_DIR` without verifying the resolved
path stayed inside it. A malicious skill with `templates: ['../../etc/passwd']`
or an absolute path could trigger arbitrary local file reads.

Now resolve the candidate path and reject any entry whose path escapes
the starters root (via `..`) or is absolute. Logged via console.warn,
matches existing missing-file behavior.

Adds vitest coverage for both traversal and absolute-path rejection,
plus regression coverage that legitimate starters still attach.
Copy link
Copy Markdown

@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] STARTERS_DIR uses URL .pathname, which breaks Windows file paths — new URL(...).pathname can produce /C:/... and percent-encoded segments, causing starter files to fail loading on Windows. Evidence packages/core/src/skills/loader.ts:193
    Suggested fix:

    import { fileURLToPath } from 'node:url';
    
    export const STARTERS_DIR = fileURLToPath(new URL('../../../templates/starters/', import.meta.url));
  • [Major] Missing/invalid starter templates are still silently skipped via console.warn + continue — this violates the "No silent fallbacks" constraint and can hide broken skill configuration. Evidence packages/core/src/skills/loader.ts:220 and packages/core/src/skills/loader.ts:223
    Suggested fix:

    throw new CodesignError(
      `Starter template "${file}" referenced by ${skillFile} is invalid or missing`,
      'SKILL_LOAD_FAILED',
    );

Summary
Review mode: follow-up after new commits.
Found 2 issues introduced/retained in the current diff: Windows path resolution risk for starter loading, and silent-fallback behavior that conflicts with project constraints. docs/VISION.md and docs/PRINCIPLES.md are not present in this checkout (Not found in repo/docs).

Testing
Not run (automation). Suggested additions: one Windows-path test for STARTERS_DIR, and one test asserting missing/invalid templates raise CodesignError.

open-codesign Bot


/** Default location of bundled starter templates referenced by skill frontmatter. */
export const STARTERS_DIR = new URL('../../../templates/starters/', import.meta.url).pathname;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using new URL(...).pathname for filesystem access is not cross-platform (notably Windows). Prefer fileURLToPath(new URL(...)) so STARTERS_DIR resolves correctly on all OSes.

);
continue;
}
let content: string;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This console.warn+continue path silently degrades behavior when a template is invalid/missing. Per project constraints, fail with CodesignError(SKILL_LOAD_FAILED) instead of skipping.

new URL(...).pathname returns '/C:/...' on Windows which path.resolve
rejects. Use fileURLToPath + dirname + resolve to get a proper absolute
path on every platform.
Copy link
Copy Markdown

@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] templates 加载存在静默降级,违反项目硬约束(No silent fallbacks)— 当模板路径越界或模板文件缺失时仅 console.warncontinue,调用方不会得到失败信号,配置错误会被悄悄吞掉。证据:packages/core/src/skills/loader.ts:222packages/core/src/skills/loader.ts:232
    Suggested fix:
if (rel === '' || rel.startsWith('..') || isAbsolute(rel)) {
  throw new CodesignError(
    `[skills] Refusing starter template outside starters dir: "${file}" (referenced by ${skillFile})`,
    'SKILL_LOAD_FAILED',
  );
}

try {
  content = await readFile(candidate, 'utf-8');
} catch (err) {
  const msg = err instanceof Error ? err.message : String(err);
  throw new CodesignError(
    `[skills] starter template "${file}" referenced by ${skillFile} not found: ${msg}`,
    'SKILL_LOAD_FAILED',
  );
}

Summary

  • Review mode: follow-up after new commits
  • 本次增量已修复上轮提出的 Windows 路径问题(STARTERS_DIR 改为 fileURLToPath + resolve),但仍保留 1 个阻断项:starter 模板异常路径仍被 warn+skip。
  • docs/VISION.mddocs/PRINCIPLES.md:Not found in repo/docs。

Testing

  • Not run (automation)
  • 建议补测:当 templates 指向缺失文件或越界路径时,loadSkillsFromDir 抛出 CodesignError(SKILL_LOAD_FAILED)(替换现有 warn+skip 断言)。

open-codesign Bot

const candidate = resolve(startersRoot, file);
const rel = relative(startersRoot, candidate);
if (rel === '' || rel.startsWith('..') || isAbsolute(rel)) {
console.warn(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

这里使用 console.warn + continue 会静默降级,违反项目硬约束(No silent fallbacks)。建议改为抛出 CodesignError('SKILL_LOAD_FAILED'),让模板配置错误对调用方可见。

@hqhq1025
Copy link
Copy Markdown
Collaborator Author

Closing this PR. 1751-line + [Blocker] templates silent fallback + DIRTY for 9h. The starter library concept is good but the patch is too large to land safely. Re-doing in v0.2 as 3 smaller PRs: (1) skill template loader infra, (2) starter directory wiring, (3) UI surface. Tracking: TBD.

@hqhq1025 hqhq1025 closed this Apr 19, 2026
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