Skip to content

fix(selfhost): ignore YAML inline comments on review-skill name and when - #2724

Merged
JSONbored merged 3 commits into
JSONbored:mainfrom
ultrahighsuper:fix/review-skill-inline-comment-parity
Jul 3, 2026
Merged

fix(selfhost): ignore YAML inline comments on review-skill name and when#2724
JSONbored merged 3 commits into
JSONbored:mainfrom
ultrahighsuper:fix/review-skill-inline-comment-parity

Conversation

@ultrahighsuper

@ultrahighsuper ultrahighsuper commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

A self-host operator annotating their private review rubrics inline — the most common way to document YAML — silently breaks them today:

---
name: SQL Rubric   # our sql-index rubric
when: "**/*.sql"   # only sql files
---

parseReviewSkill captured the whole value tail and stripped only surrounding quotes, not a trailing comment:

  • name became "SQL Rubric # our sql-index rubric" — the comment leaked into the label rendered in the reviewer prompt (## skill: …).
  • when became '**/*.sql" # only sql files' — a stray quote + comment. reviewSkillApplies glob-matches that against changed paths, so it never matches and the rubric silently never fires, with no error (a container-private file with no CI lint over it).

Fix

Decode name/when through the YAML parser (yaml, already imported in this module) instead of hand-rolling scalar parsing. Each value is parsed as a standalone YAML scalar, so quoting, escaped \" / doubled '' quotes, and trailing inline comments are all handled correctly:

  • name: "SQL #1 Rubric"SQL #1 Rubric (internal # preserved)
  • name: "SQL \"Index\" Rubric"SQL "Index" Rubric; name: 'Owner''s Rubric'Owner's Rubric
  • name: SQL Rubric # noteSQL Rubric (comment dropped); name: a#ba#b

A value the parser rejects standalone — notably an unquoted *-leading glob (when: **/…, which is invalid standalone YAML) — falls back to a lenient inline-comment/quote strip so those globs keep working exactly as before. Each field is parsed independently, so a malformed value in one never affects the other.

Tests

Adds regression cases for inline comments, internal #, escaped/doubled quotes, an unquoted glob (no regression), a non-string scalar, and a malformed unterminated quote. Verified the pre-fix behavior FAILS these and the fix PASSES; the full private-config + focus-manifest suites stay green.

No linked issue: issue creation is unavailable for this account.

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.
@ultrahighsuper
ultrahighsuper requested a review from JSONbored as a code owner July 3, 2026 08:13
@loopover-orb

loopover-orb Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 08:47:54 UTC

2 files · 1 AI reviewer · no blockers · readiness 80/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This change fixes the self-host review-skill frontmatter path by decoding `name` and `when` with the existing YAML parser and preserving the legacy fallback for invalid standalone globs. The visible implementation covers the reported inline-comment failure, quoted internal `#`, escaped quotes, non-string scalars, and malformed fallback behavior without changing the reader wiring or public contract. I do not see a reachable break in the changed code.

Nits — 4 non-blocking
  • nit: `src/selfhost/private-config.ts:181` trims parsed quoted YAML strings, so intentionally quoted leading/trailing whitespace is no longer preserved; that is probably fine for labels/globs, but it is a small behavior change worth making explicit or testing if intentional.
  • nit: `src/selfhost/private-config.ts:185` keeps the fallback comment stripping non-quote-aware for parser failures, so malformed quoted values containing ` #` still lose everything after that marker; acceptable as a degradation path, but the behavior should stay documented as best-effort rather than YAML-equivalent.
  • At `src/selfhost/private-config.ts:181`, consider returning `parsed` directly for string scalars and letting the existing `||` fallback handle empty values, unless trimming quoted YAML content is an intentional normalization.
  • At `test/unit/private-config.test.ts:244`, add one focused assertion for an invalid standalone glob with a trailing inline comment, e.g. `when: **/*.ts # ts only`, since that exercises the fallback path this PR explicitly preserves.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 27 merged, 6 issue(s).
Contributor context ✅ Confirmed Gittensor contributor ultrahighsuper; Gittensor profile; 48 PR(s), 6 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: ultrahighsuper
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 6 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.00%. Comparing base (60c85c8) to head (63cd902).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2724      +/-   ##
==========================================
- Coverage   96.09%   96.00%   -0.09%     
==========================================
  Files         243      244       +1     
  Lines       27236    27284      +48     
  Branches     9897     9913      +16     
==========================================
+ Hits        26173    26195      +22     
- Misses        438      462      +24     
- Partials      625      627       +2     
Files with missing lines Coverage Δ
src/selfhost/private-config.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 3, 2026
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 JSONbored#1 Rubric" stays "SQL JSONbored#1 Rubric"),
while a trailing inline comment after the value is still dropped and an
unquoted a#b keeps its #.
…canner

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.
@JSONbored
JSONbored merged commit 7306f88 into JSONbored:main Jul 3, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants