Skip to content

fix: APM skill.ts — YAML escaping, type safety, and test coverage #924

Description

@tamirdresher

Follow-up from PR #876 post-merge review

Reviewer: River (TypeScript Architect) via @diberry
Original PR: #876

Critical

  1. No YAML value escaping — skill names/descriptions containing colons, hashes, quotes, or newlines produce invalid YAML in publish() and generateApmYml()
  2. Hand-rolled YAML parser in installFromGitHub — line-splitting and regex will break on multi-line strings, quoted values with colons. Consider yaml npm package (0 deps, 50KB).

Medium

  1. catch (err: any) x2 should be catch (err: unknown) with narrowing (skill.ts:1057, 1171)
  2. as { name: string; path: string } x3 should use type guards (skill.ts:1117, 1125, 1131)
  3. Non-null assertions (!) x6 — regex groups and array indices not narrowed by TS
  4. parseFrontMatter returns Record<string, string> — no compile-time contract (skill.ts:819)
  5. ApmManifest declared but never validated at runtime

Low

  1. generateApmYml uses sync fs (writeFileSync) while skill.ts is fully async
  2. Silent error swallowing in collectSkills, readProjectName, listSkills
  3. 568 lines in one file — needs splitting

Action Items (priority order)

  1. Add YAML value escaping for all publish/init output paths
  2. Add unit tests for YAML parsing — at least 5 cases
  3. Replace catch (err: any) with catch (err: unknown)
  4. Add parseApmManifest() runtime validator
  5. Consider yaml npm package vs maintaining regex parser

Also noted by @Omzig

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions