Skip to content

fix(daemon): keep quoted YAML triggers with colons as strings - #6286

Closed
saidyildiz wants to merge 2 commits into
nexu-io:mainfrom
saidyildiz:fix/frontmatter-quoted-colon-triggers
Closed

fix(daemon): keep quoted YAML triggers with colons as strings#6286
saidyildiz wants to merge 2 commits into
nexu-io:mainfrom
saidyildiz:fix/frontmatter-quoted-colon-triggers

Conversation

@saidyildiz

@saidyildiz saidyildiz commented Jul 30, 2026

Copy link
Copy Markdown

Why

While browsing skills in the home composer + menu, skill previews crashed for catalogue entries whose frontmatter triggers include quoted phrases with a colon — for example "1:1 figma".

The lightweight YAML frontmatter parser treated any sequence item containing : as a single-line mapping (key: value). That turned free-text trigger phrases into objects. React then failed when rendering those objects as children in the skill preview.

Pain: users cannot open the skill preview for Figma-related (and similar) skills that use ratio-style trigger wording.

What users will see

  • Skill previews in the composer + menu open normally for skills whose triggers include quoted phrases like "1:1 figma".
  • Trigger chips still show the intended free-text phrases instead of breaking the panel.

Surface area

  • UI — composer skill preview path in apps/web (defensive filter only; no new UI surface)
  • Keyboard shortcut — new or changed
  • CLI / env var — new od subcommand or flag, new tools-dev / tools-pack flag, or new OD_* env var
  • API / contract — new /api/* endpoint, new SSE event, or changed shape in packages/contracts
  • Extension point — new entry under skills/, design-systems/, design-templates/, or craft/, or change to the skills protocol
  • i18n keys — added new translation keys
  • New top-level dependency — adding any new entry to the root package.json
  • Default behavior change — changes what existing users experience without opting in
  • None — internal refactor, docs, tests, or translation update only

Screenshots

UI change is a crash fix (preview now renders). No visual redesign; happy to attach a before/after capture if maintainers want one for the review record.

Bug fix verification

  • Test path that reproduces the bug:
    • apps/daemon/tests/frontmatter.test.ts — keeps quoted sequence items with colons as strings; still parses unquoted key: value sequence items as objects
    • packages/plugin-runtime/tests/parsers.test.ts — same coverage in the shared parser
  • Did the test go red on main and green on this branch? yes (new assertions encode the quoted-colon invariant; focused suites pass on this branch)
  • Additional hardening:
    • normalizeTriggers() in the daemon skills listing drops non-string items from bad YAML parses
    • Composer skill preview filters triggers to strings before React render

Validation

  • pnpm --filter @open-design/daemon exec vitest run tests/frontmatter.test.ts — 21 passed
  • pnpm --filter @open-design/plugin-runtime exec vitest run tests/parsers.test.ts — 29 passed

Implementation notes

  • Extract isQuotedScalar() and skip the key: value mapping branch for fully quoted scalars ("..." / '...').
  • Apply the same parser fix in both apps/daemon and packages/plugin-runtime so design-system and plugin frontmatter stay aligned.

Quoted skill trigger phrases like "1:1 figma" were parsed as mapping
objects, which crashed the home composer skill preview. Treat fully
quoted sequence items as scalars, normalize triggers to strings, and
guard the UI so non-string triggers never render as children.
@lefarcen

Copy link
Copy Markdown
Contributor

Thanks @saidyildiz — nice catch on the quoted-colon frontmatter edge case in skill previews. I've routed this to @PerishCode and marked it for a manual QA pass before merge, so please hold off self-merging for now; we'll loop QA in once it's merge-ready.

@lefarcen lefarcen added the size/S PR changes 20-100 lines label Jul 30, 2026
@lefarcen
lefarcen requested a review from PerishCode July 30, 2026 15:18
@lefarcen lefarcen added risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 30, 2026

@PerishCode PerishCode left a comment

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.

The quoted-trigger crash is addressed defensively, but the parser change introduces a mapping compatibility regression that needs correction before merge.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

if (value.includes(':')) {
// Quoted scalars keep their colons (e.g. `- "1:1 figma"`). Only unquoted
// `key: value` items are single-line mapping entries in a sequence.
if (value.includes(':') && !isQuotedScalar(value)) {

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.

Preserve quoted keys in sequence mappings. The new whole-value check also matches a valid single-line mapping such as - "name": "foo", because that complete string both starts and ends with a double quote. This branch therefore sends it to coerce() and produces the scalar name": "foo instead of the prior mapping object. That is a backward-incompatible parse change for the documented dash-prefixed single-line-object subset, and the same condition is mirrored in packages/plugin-runtime/src/parsers/frontmatter.ts. Locate a mapping separator only when the colon is outside quotes (and, for YAML mapping syntax, followed by whitespace or the end), otherwise treat the item as a scalar; then add the quoted-key mapping case alongside the quoted-colon scalar fixtures in both parser suites.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated in the latest commit: sequence mappings now key off an unquoted colon (with trailing whitespace / EOL), so - "name": "foo" remains a mapping while - "1:1 figma" stays a scalar. Added the quoted-key fixture in both parser test suites.

Detect single-line sequence mappings only when the colon is outside
quotes and followed by whitespace or end-of-string. This keeps quoted
trigger phrases like "1:1 figma" as scalars while preserving quoted-key
mappings such as - "name": "foo".
@saidyildiz

saidyildiz commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks for the review ,good catch on the quoted-key mapping regression.

I pushed a follow-up that stops using a whole-value isQuotedScalar gate for sequence items. Mapping separators are now found only when : is outside quotes and followed by whitespace or end-of-string, so:

  • - "1:1 figma" / - '1:1 notes' stay scalars
  • - "name": "foo" / - 'id': 1 still parse as mappings
  • unquoted - k: v keeps working as before

Both parser suites cover the quoted-key case alongside the quoted-colon scalar fixtures (apps/daemon + packages/plugin-runtime). Focused tests pass locally.

@PerishCode PerishCode left a comment

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.

The quoted-key case is repaired, but the replacement separator scanner still changes the supported single-line sequence-mapping behavior for plain keys containing apostrophes. Please preserve that existing parse path in both mirrored parsers before merge.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/design-systems/frontmatter.ts
@saidyildiz saidyildiz closed this Jul 30, 2026
@saidyildiz
saidyildiz deleted the fix/frontmatter-quoted-colon-triggers branch July 30, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/S PR changes 20-100 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants