From 0574c714b4cd7ee0a955207172ff75470a5908d4 Mon Sep 17 00:00:00 2001 From: ultrahighsuper Date: Fri, 3 Jul 2026 17:12:52 +0900 Subject: [PATCH 1/3] fix(selfhost): ignore YAML inline comments on review-skill name and when MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseReviewSkill captured the whole value tail for name and when, so an inline comment leaked in: `name: SQL Rubric # note` became the literal label, and `when: "**/*.sql" # note` became a glob that never matches — silently disabling the rubric with no error. Strip a trailing ` # …` comment before the quote-strip, byte-identical to isReviewSkillEnabled; a `#` with no preceding whitespace (C# Rubric, a#b) stays part of the value. --- src/selfhost/private-config.ts | 11 +++++++---- test/unit/private-config.test.ts | 8 ++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index 7520a816c9..e390155e2e 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -174,11 +174,14 @@ export function parseReviewSkill(filename: string, text: string): RepoReviewSkil const fm = /^---\s*\n([\s\S]*?)\n---\s*\n?([\s\S]*)$/.exec(text); const head = fm?.[1] ?? ""; const body = (fm?.[2] ?? text).trim(); - // Strip surrounding quotes on `name` too, symmetric with `when` below — a quoted scalar - // (`name: "SQL Rubric"`) is ordinary YAML frontmatter, so the quotes must not survive into the label. - const nameRaw = /(?:^|\n)name:\s*(.+)/.exec(head)?.[1]?.trim(); + // Drop an unquoted YAML inline comment (` # …`) before stripping surrounding quotes — symmetric with + // isReviewSkillEnabled. A trailing comment (`when: "**/*.sql" # only sql`) is not part of the scalar; left in + // place it corrupts the label and, for `when`, produces a glob that never matches so the skill silently never + // fires. Also strip surrounding quotes on `name`, symmetric with `when` — a quoted scalar (`name: "SQL Rubric"`) + // is ordinary frontmatter, so neither the quotes nor the comment must survive into the label/glob. + const nameRaw = /(?:^|\n)name:\s*(.+)/.exec(head)?.[1]?.replace(/\s+#.*$/, "").trim(); const name = (nameRaw ?? "").replace(/^["']|["']$/g, "") || filename.replace(/\.md$/i, ""); - const whenRaw = /(?:^|\n)when:\s*(.+)/.exec(head)?.[1]?.trim(); + const whenRaw = /(?:^|\n)when:\s*(.+)/.exec(head)?.[1]?.replace(/\s+#.*$/, "").trim(); const when = (whenRaw ?? "always").replace(/^["']|["']$/g, "") || "always"; return { name, when, body }; } diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index cbd505be81..0466a6a344 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -226,6 +226,14 @@ describe("parseReviewSkill (#review-skills)", () => { expect(parseReviewSkill("y.md", "---\nname: 'Voice Guide'\n---\nb").name).toBe("Voice Guide"); expect(parseReviewSkill("fallback.md", '---\nname: ""\n---\nb').name).toBe("fallback"); }); + it("ignores a YAML inline comment on name and when, symmetric with enabled", () => { + // A trailing ` # …` is a YAML comment, not part of the scalar: left in, it corrupts the label and turns + // `when` into a glob that never matches, silently disabling the rubric. + expect(parseReviewSkill("sql.md", '---\nname: SQL Rubric # the sql one\nwhen: "**/*.sql" # only sql\n---\nBody.\n')).toEqual({ name: "SQL Rubric", when: "**/*.sql", body: "Body." }); + // A `#` with no preceding whitespace is part of the value (real YAML), not a comment — must be preserved. + expect(parseReviewSkill("cs.md", '---\nname: "C# Rubric"\n---\nb').name).toBe("C# Rubric"); + expect(parseReviewSkill("z.md", "---\nname: a#b\n---\nb").name).toBe("a#b"); + }); }); describe("isReviewSkillEnabled (#review-skills)", () => { From 8bcca4efef669b2e2c40761ae0b340ae051756eb Mon Sep 17 00:00:00 2001 From: ultrahighsuper Date: Fri, 3 Jul 2026 17:27:23 +0900 Subject: [PATCH 2/3] quote-aware scalar extraction for review-skill name/when MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review: the inline-comment strip must not fire inside a quoted scalar. Read name/when via a quote-aware extractor — a quoted value keeps its body verbatim (so name: "SQL #1 Rubric" stays "SQL #1 Rubric"), while a trailing inline comment after the value is still dropped and an unquoted a#b keeps its #. --- src/selfhost/private-config.ts | 31 +++++++++++++++++++++---------- test/unit/private-config.test.ts | 6 +++++- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index e390155e2e..9c355b53b6 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -168,21 +168,32 @@ function reviewContextFolders(repoFullName: string): string[] { return [join(`${owner}__${repo}`, "review"), join(repo, "review")]; } +/** Extract a YAML frontmatter scalar's value, quote-aware. A double/single-quoted scalar returns its quoted body + * verbatim (an inline comment or stray text after the closing quote is dropped, and a `#` INSIDE the quotes — e.g. + * `"SQL #1 Rubric"` — is preserved as part of the value). An unquoted scalar drops a trailing ` # …` inline comment + * (whitespace-before-hash; `a#b` keeps its `#`) and any stray surrounding quote. */ +function reviewSkillScalar(raw: string): string { + const s = raw.trim(); + const quote = s[0]; + if (quote === '"' || quote === "'") { + const close = s.indexOf(quote, 1); + if (close !== -1) return s.slice(1, close); // well-formed quoted scalar; a `#` within it is not a comment + } + return s.replace(/\s+#.*$/, "").replace(/^["']|["']$/g, "").trim(); +} + /** Parse a skill markdown file into {name, when, body}. YAML frontmatter (`---\nname:\nwhen:\n---`) is optional; name - * defaults to the filename and `when` to "always". */ + * defaults to the filename and `when` to "always". `name`/`when` are read as quote-aware scalars so a quoted value + * keeps its contents (incl. an internal `#`) while a trailing inline comment is dropped — an unstripped comment + * corrupts the label and turns `when` into a glob that never matches, silently disabling the rubric. */ export function parseReviewSkill(filename: string, text: string): RepoReviewSkill { const fm = /^---\s*\n([\s\S]*?)\n---\s*\n?([\s\S]*)$/.exec(text); const head = fm?.[1] ?? ""; const body = (fm?.[2] ?? text).trim(); - // Drop an unquoted YAML inline comment (` # …`) before stripping surrounding quotes — symmetric with - // isReviewSkillEnabled. A trailing comment (`when: "**/*.sql" # only sql`) is not part of the scalar; left in - // place it corrupts the label and, for `when`, produces a glob that never matches so the skill silently never - // fires. Also strip surrounding quotes on `name`, symmetric with `when` — a quoted scalar (`name: "SQL Rubric"`) - // is ordinary frontmatter, so neither the quotes nor the comment must survive into the label/glob. - const nameRaw = /(?:^|\n)name:\s*(.+)/.exec(head)?.[1]?.replace(/\s+#.*$/, "").trim(); - const name = (nameRaw ?? "").replace(/^["']|["']$/g, "") || filename.replace(/\.md$/i, ""); - const whenRaw = /(?:^|\n)when:\s*(.+)/.exec(head)?.[1]?.replace(/\s+#.*$/, "").trim(); - const when = (whenRaw ?? "always").replace(/^["']|["']$/g, "") || "always"; + const nameRaw = /(?:^|\n)name:\s*(.+)/.exec(head)?.[1]; + const name = (nameRaw !== undefined ? reviewSkillScalar(nameRaw) : "") || filename.replace(/\.md$/i, ""); + const whenRaw = /(?:^|\n)when:\s*(.+)/.exec(head)?.[1]; + const when = (whenRaw !== undefined ? reviewSkillScalar(whenRaw) : "always") || "always"; return { name, when, body }; } diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index 0466a6a344..784e2c8237 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -226,13 +226,17 @@ describe("parseReviewSkill (#review-skills)", () => { expect(parseReviewSkill("y.md", "---\nname: 'Voice Guide'\n---\nb").name).toBe("Voice Guide"); expect(parseReviewSkill("fallback.md", '---\nname: ""\n---\nb').name).toBe("fallback"); }); - it("ignores a YAML inline comment on name and when, symmetric with enabled", () => { + it("ignores a trailing YAML inline comment on name and when, quote-aware", () => { // A trailing ` # …` is a YAML comment, not part of the scalar: left in, it corrupts the label and turns // `when` into a glob that never matches, silently disabling the rubric. expect(parseReviewSkill("sql.md", '---\nname: SQL Rubric # the sql one\nwhen: "**/*.sql" # only sql\n---\nBody.\n')).toEqual({ name: "SQL Rubric", when: "**/*.sql", body: "Body." }); // A `#` with no preceding whitespace is part of the value (real YAML), not a comment — must be preserved. expect(parseReviewSkill("cs.md", '---\nname: "C# Rubric"\n---\nb').name).toBe("C# Rubric"); expect(parseReviewSkill("z.md", "---\nname: a#b\n---\nb").name).toBe("a#b"); + // A `#` INSIDE a quoted scalar — even with preceding whitespace — is part of the value, not a comment. + expect(parseReviewSkill("h.md", '---\nname: "SQL #1 Rubric"\nwhen: "src/#hot/**" # trailing note\n---\nb')).toEqual({ name: "SQL #1 Rubric", when: "src/#hot/**", body: "b" }); + // A malformed unterminated quote degrades to stripping the stray leading quote (back-compat, not a crash). + expect(parseReviewSkill("u.md", '---\nname: "unterminated\n---\nb').name).toBe("unterminated"); }); }); From 63cd902d774a10ed22fdfb837bf11553952f7265 Mon Sep 17 00:00:00 2001 From: ultrahighsuper Date: Fri, 3 Jul 2026 17:44:15 +0900 Subject: [PATCH 3/3] parse review-skill name/when via the YAML parser, not a hand-rolled scanner Address review: the hand-rolled quoted-scalar scanner mishandled YAML-escaped double quotes ("SQL \"Index\" Rubric") and doubled single quotes ('Owner''s Rubric'). Decode each value with the yaml parser (already imported) so quoting, escapes, and trailing inline comments are handled correctly; a value the parser rejects standalone (an unquoted *-leading glob) falls back to a lenient comment/quote strip so it still works. --- src/selfhost/private-config.ts | 30 ++++++++++++++++-------------- test/unit/private-config.test.ts | 7 +++++++ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index 9c355b53b6..75c9669e4e 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -168,24 +168,26 @@ function reviewContextFolders(repoFullName: string): string[] { return [join(`${owner}__${repo}`, "review"), join(repo, "review")]; } -/** Extract a YAML frontmatter scalar's value, quote-aware. A double/single-quoted scalar returns its quoted body - * verbatim (an inline comment or stray text after the closing quote is dropped, and a `#` INSIDE the quotes — e.g. - * `"SQL #1 Rubric"` — is preserved as part of the value). An unquoted scalar drops a trailing ` # …` inline comment - * (whitespace-before-hash; `a#b` keeps its `#`) and any stray surrounding quote. */ -function reviewSkillScalar(raw: string): string { - const s = raw.trim(); - const quote = s[0]; - if (quote === '"' || quote === "'") { - const close = s.indexOf(quote, 1); - if (close !== -1) return s.slice(1, close); // well-formed quoted scalar; a `#` within it is not a comment +/** Read a `name:` / `when:` frontmatter value robustly. The value text (everything after the key on its line) is + * parsed as a standalone YAML scalar, so the real parser handles quoting, escaped `\"` / doubled `''` quotes, and a + * trailing inline comment — `"SQL #1 Rubric"` keeps its internal `#`, `SQL Rubric # note` drops the comment. A + * value the YAML parser rejects standalone — notably an unquoted glob that begins with a `*` wildcard — falls back + * to a lenient strip (drop an inline comment and any surrounding quote) so those globs keep working. */ +function reviewSkillScalar(rawValue: string): string { + try { + const parsed = parseYaml(rawValue); + if (typeof parsed === "string") return parsed.trim(); + } catch { + // not a standalone-parseable scalar (e.g. an unquoted *-leading glob) — fall through to the lenient strip } - return s.replace(/\s+#.*$/, "").replace(/^["']|["']$/g, "").trim(); + return rawValue.replace(/\s+#.*$/, "").replace(/^["']|["']$/g, "").trim(); } /** Parse a skill markdown file into {name, when, body}. YAML frontmatter (`---\nname:\nwhen:\n---`) is optional; name - * defaults to the filename and `when` to "always". `name`/`when` are read as quote-aware scalars so a quoted value - * keeps its contents (incl. an internal `#`) while a trailing inline comment is dropped — an unstripped comment - * corrupts the label and turns `when` into a glob that never matches, silently disabling the rubric. */ + * defaults to the filename and `when` to "always". `name`/`when` are decoded through the YAML parser (see + * reviewSkillScalar) so a quoted value keeps its contents (incl. an internal `#`) while a trailing inline comment is + * dropped — an unstripped comment corrupts the label and turns `when` into a glob that never matches, silently + * disabling the rubric. */ export function parseReviewSkill(filename: string, text: string): RepoReviewSkill { const fm = /^---\s*\n([\s\S]*?)\n---\s*\n?([\s\S]*)$/.exec(text); const head = fm?.[1] ?? ""; diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index 784e2c8237..92f14eb082 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -235,6 +235,13 @@ describe("parseReviewSkill (#review-skills)", () => { expect(parseReviewSkill("z.md", "---\nname: a#b\n---\nb").name).toBe("a#b"); // A `#` INSIDE a quoted scalar — even with preceding whitespace — is part of the value, not a comment. expect(parseReviewSkill("h.md", '---\nname: "SQL #1 Rubric"\nwhen: "src/#hot/**" # trailing note\n---\nb')).toEqual({ name: "SQL #1 Rubric", when: "src/#hot/**", body: "b" }); + // YAML-escaped double quotes and doubled single quotes are decoded, not treated as the terminator. + expect(parseReviewSkill("e.md", '---\nname: "SQL \\"Index\\" Rubric"\n---\nb').name).toBe('SQL "Index" Rubric'); + expect(parseReviewSkill("o.md", "---\nname: 'Owner''s Rubric'\n---\nb").name).toBe("Owner's Rubric"); + // An unquoted *-leading glob is not valid standalone YAML; it must still survive as the literal when-glob. + expect(parseReviewSkill("g.md", "---\nwhen: **/*.ts\n---\nb").when).toBe("**/*.ts"); + // A non-string YAML scalar (e.g. a bare number) falls through to the literal text rather than a typed value. + expect(parseReviewSkill("n.md", "---\nname: 42\n---\nb").name).toBe("42"); // A malformed unterminated quote degrades to stripping the stray leading quote (back-compat, not a crash). expect(parseReviewSkill("u.md", '---\nname: "unterminated\n---\nb').name).toBe("unterminated"); });