fix(skills): resolve the full block-scalar grammar and real chomping - #7648
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of All evidence gathered. This PR is a backend YAML block-scalar parser fix with a matching update to the SkillForm's parser mirror — no new UI surfaces, no changed user-facing strings, no screenshots. The user-visible effects are strictly positive: the structured skill editor now accepts block-scalar shapes it previously bounced into the raw editor, and the "Always on" checkbox now agrees with the loader (previously UX-Verdict: PASS Invisible-by-design parser fix; the one user-facing effect is the Always-on checkbox now telling the truth about skills the loader actually activates. [UX-REVIEWED] 0fab2b9 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All mechanical checks done. The sweep claims hold: zero unstripped First-Principles-Verdict: PASS The fix deletes the second recognizer instead of patching call sites, and the YAML-parser alternative was disqualified by a counted measurement, not preference. What this change shipsIntent: make skill frontmatter block scalars read what the file actually says, per YAML — a FIX.
The obvious existing mechanism (PyYAML, already a test dependency) was measured over all 56 shipped SKILL.md files and rejected on evidence — 2 shipped skills are unparseable YAML — and that measurement is pinned as a test, so the decision stays falsifiable. The riders (items 7, 10) are one-line fixes to the exact comparisons the fix already edits; their zero option leaves a fail-open import spelling and a form/loader disagreement, so the rider-deletion exception does not apply. Deleting [FIRST-PRINCIPLES-REVIEWED] 0fab2b9 |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS One grammar matcher shared by read, write, and the fail-closed activation gate — the root cause (dual recognizers) is deleted, the rejected alternative is measured, and the blast radius is named and pinned. [DESIGN-REVIEWED] 0fab2b9 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've traced the candidate against the actual read path ( The asymmetry the candidate describes is real in mechanism: for a block scalar with an inferred indent, the read path breaks on the first content line at But bar (c) — an observable wrong outcome — does not hold:
The only column-0 lines that reach line 675 without breaking at line 652 are tab-led ones (a space-led line has No new grounded defect surfaced while falsifying it — the change is pinned by a differential YAML oracle, a repo-corpus test, and the activation-gate superset test. No findings. [OPUS-REVIEWED] 0fab2b9 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/frontmatter.py:87 -- False positive or not applicable? A repository writer can comment: |
9cc2cfa to
b1c0c5c
Compare
|
Round 1 dispositions on BLOCKING (Design, Opus, First Principles): the import gate went fail-OPEN -- FIXEDAccepted in full, and the mechanism is exactly as described. Reproduced before the
Design put the invariant better than my PR body did: the gate is fail-closed only Fix: BLOCKING (GPT): explicit-indent scalars absorb a less-indented comment -- FIXED, and it was wider than reportedAccepted -- and measurement says the finding under-states it. GPT scoped this to
The second row is the same defect with NO explicit indicator, wrong on base already. FINDING (GPT): quoted
|
b1c0c5c to
2eedae3
Compare
|
Round 2 dispositions on BLOCKING (GPT): trailing whitespace content silently discarded -- FIXEDCorrect, and the prescribed fix ("dedent before classifying trailing breaks") is the
Classification now happens on the DEDENTED line: an empty string is a break, a The more useful part of this finding is what it says about my test matrix. The Two regressions my own refactor introduced on the way, both caught by the widened Opus: no findings -- and its self-dropped candidate is FIXED anywayOpus raised the write path ( I verified it rather than taking the drop, and fixed it. It reproduces through that one Reachability is as narrow as Opus says. I fixed it because it is an asymmetry THIS Held the fix rather than pushing it when I found it, so it costs one review round Design, First Principles, UX: PASS on the previous head, nothing outstandingNo advisories to disposition. First Principles independently re-derived the consumer Suites re-run: 1887 backend passed, 121 frontend passed; flake8, isort, mypy, black |
2eedae3 to
8dcd6ef
Compare
|
Round 3 on
|
|
CI status note on
|
8dcd6ef to
c24bf7f
Compare
|
Round 3 dispositions on Where these came fromGPT's lane has posted "review incomplete" three times on So I ran the falsification pass 2 never got to. Both candidates survive it, measured. BLOCKING 1: padded quoted
|
always: |
loader reads | activates | gate detects | |
|---|---|---|---|---|
" true " |
' true ' |
yes | False | FAIL-OPEN |
' true ' |
' true ' |
yes | False | FAIL-OPEN |
" TRUE " |
' TRUE ' |
yes | False | FAIL-OPEN |
"true" / true |
'true' |
yes | True | ok |
" yes " / " false " |
-- | no | False | ok (correctly not activating) |
An imported skill package spelled that way self-activates into every session past the
screen. Fixed exactly as the finding prescribed -- strip whitespace after the quotes
come off, so the gate normalises the way the consumers do. All three rows now detected,
with none of the "ok" rows moving, pinned by
test_a_padded_quoted_value_is_detected (which asserts BOTH halves: that the loader
activates the value AND that the gate sees it, so the two cannot drift apart again).
BLOCKING 2: folded scalars discard authored trailing whitespace -- FIXED
Real, 4 divergences. A folded scalar's trailing spaces are CONTENT, and the break folds
to a space AFTER them, so > over a then b is a b -- three spaces. The fold
was calling .strip() on each plain line and .rstrip() on each more-indented one:
| input | before | yaml |
|---|---|---|
> + a + b |
a b\n |
a b\n |
>1 + a + b |
a b\n |
a b\n |
> + a |
a\n |
a \n |
> + a + b |
a\n b\n |
a\n b \n |
The literal family already agreed in all four shapes, which is what made this
folded-only. The dedent already removes the block's indentation, so there was nothing
to trim on the left either; the line is now appended as-is.
The part worth recording: my matrix missed this too, again
The differential matrix did NOT catch either shape, and for the same structural reason
as last round. Round 2 added whitespace-ONLY lines after GPT found the trailing-break
bug; this round's bug needed a content line FOLLOWED BY spaces (" one "), which the
body set still could not express. Twelve such bodies are now in BODIES with a comment
naming why they are a third distinct case, the matrix runs 738 cases, 0 divergences
(up from 544), and its floor assertion moved 450 -> 620.
Two rounds in a row where the reviewer found something my own oracle could not see is
worth stating plainly: the matrix is only as good as its input space, and "0
divergences" means nothing about shapes the generator never emits.
Suites: 1632 backend passed, 121 frontend passed; flake8, isort, mypy, black baseline,
tsc, eslint clean.
Still outstanding, neither owned by this PR
Frontend Lint & Type Check-- main-owned ratchet (604 warnings vs
--max-warnings 603, 0 errors; this PR adds zero). ci: restore the eslint ratchet to the count the tree actually measures #7696 is open to restore the
threshold to what the tree measures; this clears when it merges.- GPT's lane will re-run on this head. Its three incompletes so far are infrastructure,
not verdicts, and I have not used/ai-review overrideat any point.
c24bf7f to
7e4d86e
Compare
|
Round 4 disposition on The finding: explicit-indent whitespace content is erased -- FIXEDReal. Under an explicit indicator the HEADER fixes where content starts, so Measured against the oracle, the finding was broader than the one shape it named --
Every implicit-indent case agreed, which is what localises it: the early exit is correct One row in my first measurement was a PROBE artifact, not a defect -- I had built the Three more defects the same investigation foundIndentation is spaces, never tabs. Under The write path had the same walk. Left alone it would have disagreed with the reader The TypeScript mirror had drifted. Why my matrix kept missing these, and what changedThree rounds running, a reviewer found a line shape my differential matrix could not The body space is now GENERATED from the line kinds that behave differently (empty, That change earned itself immediately: on its first run it failed on The honest lesson is that "0 divergences" was never a statement about the reader -- it Verification1635 backend passed (3 new tests), 121 frontend passed (5 new cases), flake8, isort, Still outstanding, not owned by this PR
|
|
CI attribution for Backend Tests (Windows) shard 3 -- census ratchet on
|
|
All five review lanes are clean on Verified by reading each lane's marker comment BODY, not its check conclusion: GPT 5.6 First Principles: byte-identical duplicated list comprehension -- FIXEDCorrect, and it was mine from the round-4 fix: adding the post-dedent emptiness check Behaviour-neutral by construction, and measured as such: 557 targeted tests pass, the Held locally as Design: nothing in the build enforces the Python/TypeScript mirror parity -- ACCEPTED, DEFERRED to #7763The gap is real and this PR is the evidence for it: the mirror was found three separate I did build the mechanism Design is asking for, as a throwaway probe: it EXTRACTS Filed as #7763 with that design written down, deliberately not folded in here: this PR Remaining reds, all main-owned and unchangedThe census break ( |
e5c989b to
e847966
Compare
|
Round 5 on First Principles:
|
e847966 to
a337c81
Compare
|
Round 6 on The third gate call site -- FIXED
The consequence is worse than the original defect, and it was mine: each site guards on My stated harm was overstated -- CORRECTEDFirst Principles also caught that the rationale I wrote into the code, and repeated in my That claim came from the round-5 finding's own wording, and I accepted it without opening Which spellings actually reach the gateWriting the regression test I asserted that
The test now covers all five truthy spellings plus the falsy one for contrast, and asserts Not taking the prescribed subtraction as writtenThe finding suggests replacing the two consumer strips with one at the row build, on the A second test pins that mechanically rather than by eye: it greps every 1638 targeted backend tests pass; flake8, isort, black baseline clean, and the 2 mypy Lane state on the previous headGPT 5.6 and Opus 4.8 both "no blocking findings", Design PASS, UX PASS, all with markers |
a337c81 to
ac5b794
Compare
|
Round 7 on BLOCKING: a leading blank line let an outer comment be consumed -- FIXEDReal, and it is the third variant of the note-deletion class this PR exists to close. A I did not take the finding on its wording. My first reproduction attempt FAILED: rewriting Then the oracle settled what the right answer IS, which mattered because two neighbouring
So the fix cannot be "never consume an indented comment": with no leading blank the Verified: all six oracle shapes agree; the write path loses nothing across 22,608 The generated matrix now covers the classThe corpus gained two line kinds -- This is the fourth round where a reviewer found a line shape the corpus could not express, Verification1639 targeted backend tests pass, 121 frontend, and flake8 / isort / mypy / black baseline No |
…7097) The SKILL_LOADER read path matched only the six BARE block-scalar indicators, so `description: |2-` and `description: |- # note` stored the HEADER as the value; and fold_block_scalar ended in `.strip()`, which ate a leading break and every trailing one, which no YAML chomping mode does. One header matcher now serves every site that recognizes a block scalar -- the read path, the write path, and the onboarding activation gate -- so the module no longer disagrees with itself about what a block scalar is. Explicit indentation indicators are honoured, which is what preserves a leading-space first line, and chomping is real: strip drops every trailing break, keep preserves them, clip keeps one. Whitespace is content, in three places the fold used to discard it. Trailing breaks are classified AFTER dedenting, so a line holding only whitespace beyond the block's indent stays content rather than counting as a break. A folded line's TRAILING spaces are kept, because the break folds to a space after them -- `>` over `a ` then `b` is `a b`. And a leading break is kept under every chomping mode. Collection stops at the block's indentation boundary, on both the read and the write path. YAML ends a block scalar at the first non-blank line indented less than its content, so a less-indented `#` line is a comment in the surrounding document; taking any indented line as content read `|- ` + ` body` + ` # note` back as `body\n# note`, and let a rewrite of that field delete the author's note. The onboarding activation gate reads the same grammar, and normalises the same way. It is fail-closed only while its detected set covers what the loader resolves, so widening the loader while it kept its own six-spelling list turned it fail-OPEN for `always: |2-`; and stripping whitespace only OUTSIDE the quotes let `always: " true "` through the screen while the loader's consumers activated it. `always` and `pinned` are decided by an exact `== "true"`, so a chomping -preserved break would have flipped a skill from always-on to off. Both sides normalise, case included. The plain-scalar half stays wider than YAML on purpose: two shipped builtin skills carry an unquoted ": " in `description`, which a YAML parser refuses document-wide. The new corpus test pins that set exactly.
ac5b794 to
0fab2b9
Compare
|
Round 8 on BLOCKING: CRLF blank lines terminated block scalars early -- FIXEDReal. Judging "blank" in SPACES (which the tab fix earlier in this PR introduced) means a Measured against base under
95 of 108 read cases differed from base; most are this PR's intended chomping change, but I nearly filed this as a rebuttal, and the way I got there is worth recording. My first The fix strips the trailing carriage return before classification on both walks, which is what the Verified after the fix: across 99 steering documents, CRLF equals LF in every case and The write half is NOT a regression -- zero deltaThe finding also named the write path. Measured per dialect over 36 CRLF documents each:
So the write-side symptom is a pre-existing CRLF blindness in that dialect's fence, not Verification1640 targeted backend tests pass, plus 189 in the steering/consolidation suites that No |
Body supplement -- five items the description does not yet coverPosted as a comment rather than a body edit on purpose: editing the body re-triggers the 1. Indentation is counted in SPACES, never tabsNot in the body at all. YAML indentation is spaces, so under Found by the generated matrix on its first run, not by review. 2. A leading blank line sets the block's boundaryThe body covers a leading break as CONTENT under chomping; this is a different rule. A
The second row is why this is a floor and not a blanket refusal: with no leading blank the 3.
|
| document | base | before the fix |
|---|---|---|
| + one + blank + two |
'one\r\n\ntwo' |
'one\r\n' -- two lost |
| + blank + one |
'one' |
'' -- value lost whole |
The trailing carriage return is now dropped before classification, which also corrects the
VALUE base had wrong: a parser normalises line breaks, so the reader returns 'one\n\ntwo\n'
where base returned a stray carriage return inside the string. Verified over 99 steering
documents -- CRLF equals LF and equals the oracle in every one, and no resolved value
contains a carriage return.
Two dialects still cannot see a CRLF fence at all (SKILL_LOADER, SKILL_UPDATE), which
is pre-existing and byte-identical before and after this PR. Filed as #7786 rather than
folded in.
5. Corrected test figures
The body's numbers are from an earlier head:
| claim in body | current |
|---|---|
| differential matrix, 544 cases | 5928 oracle-checked cases in CI |
| suite list, 1887 passed | 1897 passed, 11 skipped (same list, re-run on this head) |
| frontend, 121 passed | 121 passed (unchanged) |
The matrix grew because its body set is now GENERATED from named line kinds rather than
hand-enumerated -- four review rounds each found a line shape a hand-written list could not
express, so the generator became the thing being maintained. Three standing local sweeps
back it: reader vs oracle 29,676 documents, TypeScript mirror vs reader 5652, write
symmetry 22,608 rewrites -- 0 divergences, 0 lines moved.
Follow-ups filed, not folded in
- test(skills): enforce the TS mirror / Python reader parity in CI, not by docstring #7763 -- make the TypeScript-mirror/reader parity a CI test instead of a docstring.
- fix(skills): two frontmatter dialects cannot see a CRLF fence, so a Windows-saved SKILL.md parses as empty #7786 -- two frontmatter dialects cannot see a CRLF fence.
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.
Problem / Motivation
SKILL_LOADERinsrc/kiro_crew/frontmatter.pyreads block scalars with ahand-rolled scanner that implements less of YAML than the files on disk use. Two
separate defects, both silent:
description: |2-+ indented body|2-(the header)description: |- # note+ body|- # note(the header)description: |+oneoneone\n(clip keeps one break)description: |++ body + blanksThe first two are a matcher gap: the READ path tested membership of a six-element
frozenset of bare indicators, while the WRITE path in the same module already
matched the full header grammar with
_BLOCK_SCALAR_HEADER_RE. So one half of themodule understood
|2-and the other did not.The rest is one line:
fold_block_scalarended in.strip(), which removes aLEADING break and every trailing one. No YAML chomping mode does either.
A third defect surfaced while reviewing this PR and is fixed here because it is the
same collection loop: the reader took ANY indented line as scalar content, so a
less-indented
#line -- a comment in the surrounding document to YAML -- wasabsorbed into the value (
|-+body+# noteread back asbody\n# note).That was already wrong on base for a bare indicator; supporting explicit indents
would have widened it.
This PR does NOT give
SKILL_LOADERa real YAML parse, which is what #7097originally asked for. I measured the corpus the issue asked for first, and it
disqualifies that fix: see the comment on the issue
(#7097 (comment)). Over
the 56 fenced repo-tracked SKILL.md files, 2 make a YAML parser RAISE outright,
and both are skills this repo ships:
src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/SKILL.mdsrc/kiro_crew/builtin_skills/web-verify/SKILL.mdBoth carry an unquoted
": "insidedescription(... Three capture backends: playwright-cli ...), which YAML rejects DOCUMENT-wide with "mapping values are notallowed here". The scanner splits on the first colon and reads them correctly. So
the two accepted-input surfaces CROSS rather than nest, and the swap is not a
strict improvement. The quoting and unescaping rows of #7097 are #7063's scope and
are deliberately untouched here.
Why it matters
Every row above is a value the agent is handed that is not what the file says. The
|2-and|- # noterows are the worst: the value becomes the literal string"|2-", so a skill's entire description is replaced by two characters of YAMLsyntax.
It also pushed cost outward.
website/src/components/SkillForm.tsxmirrors thisreader in TypeScript and refuses to edit a field the two readers disagree about.
Because
.strip()made agreement depend on a block's CONTENT rather than itsheader, that mirror could not decide from the indicator -- three attempts tried and
each was wrong -- and a leading-space first line was deliberately degraded in the
editor because no YAML form of it survived the reader.
There was also a live activation hazard.
alwaysandpinnedare decided by anexact
meta.get(...).lower() == "true", so honouring keep-chomping would makealways: |+followed by blank lines read"true\n\n", which is not"true"-- askill silently flipping from always-on to off on a file nobody edited.
What changed (motivation -> approach -> change)
_BLOCK_SCALAR_HEADER_REnow carries the full grammar (both modifier orderings,an optional trailing comment) and is parsed by a new
parse_block_scalar_header()used by the read path, the write path, AND theonboarding activation gate. An explicit
0is refused, as YAML forbids it. Theold six-element
BLOCK_SCALAR_INDICATORSfrozenset is DELETED -- it was thesecond recognizer, and keeping it is what let the module disagree with itself.
only while its detected set is a SUPERSET of what the loader can resolve.
Round 1 of this PR widened the loader while the gate kept its own list, which
turned the gate fail-OPEN:
always: |2-over atruecontinuation wentundetected by
_column0_activation_declared, was installed verbatim, and wasthen read by
SkillsLoaderasalways == "true"-- external skill-packagecontent self-activating into every session, the exact hazard
automatic_activation_excludedexists to reject. Caught by Design Review, Opusand First Principles; measured before and after (three spellings fail-open
before, none after) and pinned by
TestTheActivationGateCoversEverythingTheLoaderResolves. An earlier revision ofthis description claimed the gate "must not move" -- that was wrong in the
safety-relevant direction, and this is the correction.
a block scalar at the first non-blank line indented less than its content, so a
less-indented
#line is a comment in the surrounding document. Collecting on"is it indented" absorbed one:
|-+body+# noteread back asbody\n# note. Caught by GPT on the explicit-indent path; measurement showed itwas ALREADY wrong on base for a bare indicator, and one boundary rule fixes
both. The WRITE path needed the same rule -- tightening read alone left the writer
treating a less-indented comment as block content, so replacing that field deleted
the author's note (raised and self-dropped as unreachable by Opus; fixed anyway,
because it is an asymmetry this change introduced and it does reproduce through
steering's mode edit). A comment indented PAST the content is still content on
both paths, as YAML has it.
looks blank --
" ".strip()is empty -- but whitespace BEYOND the block's indentis content, so it is not a break and chomping must not eat it:
|-overbodythen three spaces is
body\n, notbody. Found by GPT; the same correction wasneeded in the folded branch, which tested
.strip()too and folded such a lineaway entirely.
leading-space first line survive -- inferring the indent from the first non-blank
line cannot express it.
-drops every trailing break,+keeps them all, clip keepsexactly one; a leading break is content and is preserved. The
.strip()is gone.drops the newline before the closing
---, but that newline terminates the lastcontent line, so every line inside a fence is newline-terminated. Missing this
made a value depend on WHICH FIELD CAME NEXT (
onewhen followed by another key,one\nat the end of the block) -- caught by the differential matrix, pinned byits own test, and it is also why PyYAML and the editor's JavaScript parser looked
like they disagreed: feeding a parser the captured text verbatim asks about a
document one break short.
always/pinnedwith a bare.lower()now.strip().lower(), matching thefive that already stripped (
pinnedwas read BOTH ways in the same module). Thefrontend's two
alwayscomparisons use.trim().toLowerCase(), so the form andthe loader agree on
always: |+with trailing blanks AND onalways: "TRUE"(the case half was pre-existing; it is fixed here because this PR is what makes
the two comparisons claim to agree).
test_frontmatter.py's owndocstring requires. It no longer trims, counts the terminator, and compares in
the same normalised space as
scalarText(which drops one trailing break for ablock node). Consequence: the leading-blank and keep-chomped shapes it used to
refuse are editable again, because both sides genuinely agree now. A FOLDED form
and an explicit indicator stay refused -- that is fix(skills): refuse explicit-indicator block scalars, and fix two locales #7187's deliberate trade, and
refusing only declines an edit, it cannot corrupt.
the not-valid-YAML set EXACTLY in both directions so a third such skill is
visible rather than absorbed.
Tests
positions (end-of-block and followed-by-a-field). The oracle is
yaml.BaseLoader, notsafe_load, so every scalar stays astrandalways: truekeeps reading"true". Scoped to block scalars on purpose:asserting agreement wholesale would assert the parser swap the corpus rejected.
The body set includes WHITESPACE-ONLY lines, which are not interchangeable with
empty ones -- the first version of this matrix used only empty strings and so could
not express the case GPT found, which is why those rows carry a comment saying so.
The floor assertion is 450 checked cases, so they cannot quietly disappear.
origin/main'sfrontmatter.pyas a module and running the new expectations against it -- basereturns
'|2-'and'|- # note'as values. The tenth (|-strip with trailingblanks) already passed on base, since
.strip()and strip-chomping agree there.distinguishable on one body, and
TestChompingCannotFlipAnActivationFlagpinsthat every block-scalar spelling of
truestill reads as always-on, with aguard assertion so it cannot go vacuous if chomping were reverted.
always: |2-,|-2and|- # noteeach activate the skill (loader readstrue) while_column0_activation_declaredreturns False. After: all detected.TestTheActivationGateCoversEverythingTheLoaderResolvesasserts the invariantone-directionally over eleven header spellings (the gate may be stricter, never
looser), plus that it stays stricter where it should be and does not start
refusing ordinary values (
false,no,0,|0,maybe).the read path, one asserting a comment indented PAST the content is still content
(the boundary is the indent, not the
#), and one on the WRITE path asserting areplaced block-scalar field leaves the author's less-indented comment behind.
skills/goal-loop/SKILL.md(description: |) -- and it gains exactly onetrailing newline (562 -> 563 chars, measured base vs new). Eight snapshot-corpus
rows move for the same reason;
block_scalar_chomped(>-) does not, which isthe case that proves the modifier is now load-bearing. NOTE: this corrects the
"3 affected files" figure from my earlier issue comment -- those three were
installed-only skills, measured against an oracle missing the terminating break.
test_frontmatter.py,test_skills.py,test_skills_frontmatter.py,test_skill_discover.py,test_skill_update_flow.py,test_onboarding_import.py,test_onboarding_import_coverage.py,test_skill_budget.py,test_skill_listing_cost.py,test_history.py,test_check_feed_advance.py,test_pod_e2e_video_guard.py,test_vector_memory.py,test_ai_review_workflows.py-- 1887 passed. FrontendSkillFormFrontmatter.test.tsx+SkillsTab.test.tsx-- 121 passed.scripts/check_black_formatting.py,tsc --noEmit,eslint on changed files -- all clean.
Manual verification
Measured on this machine over 274 markdown files (repo builtins, apps, and two
installed-skill trees, 170 with a column-0 fence) comparing the reader against a
real YAML parse field by field. Results are in the issue comment linked above; the
repo-scoped slice of it is now the corpus test rather than a one-off script.
Screenshots / video
Why no screenshot: nothing rendered changes -- no component, layout, style, copy
or state was touched, and no locale string was added; the only user-facing
consequence is that a narrow class of skill files (a block-scalar
descriptionoralwayswith a leading blank line or keep-chomping) now opens in the EXISTINGstructured editor instead of the EXISTING raw editor, both pixel-identical to their
current selves, and which one is selected is asserted by
SkillFormFrontmatter.test.tsxrather than by pixels.Related Issues
Closes #7097
Refs #7063 -- the quoting and unescaping rows. Left alone deliberately: decoding
"a\tb"requires parsing the quoted scalar properly, which IS #7063's change, andthat issue is claimed by another owner.
Refs #7187 -- its
backendFoldsLiteralmirror moves here. The block-scalar declineSTRING it deferred (and its twelve locale catalogs) is still deferred; this PR
shrinks the refusal class instead of rewording it.
Pattern harvest
Rule candidate: semgrep
Pattern: a frontmatter/metadata value read into an exact string comparison
(
meta.get(...).lower() == "true") without.strip(). This repo held BOTHspellings -- five comparisons stripped, six did not, and
pinnedwas read both waysin the same module -- so any reader change that alters trailing whitespace flips a
behaviour flag at exactly the six unstripped sites. Mechanically detectable, and it
was a latent bug before this PR rather than one this PR introduced.
Second candidate, and the one that actually bit: one module recognising one grammar
with TWO different matchers. The read path tested membership of a six-element
frozenset while the write path, forty lines away, matched the full header regex, and
a fail-closed security gate in another module imported the frozenset. Widening one
recognizer and not the others is what turned that gate fail-OPEN. Rule candidate:
Not generalizableis the wrong answer here -- the smell is a hand-maintained set ofliterals sitting beside a regex for the same construct, and the fix is to delete the
set. Corollary worth stating because three reviewers had to find it for me: when a
gate is fail-closed BECAUSE its detected set covers a parser's, widening the parser
is a change to the gate, whether or not you touch its file.
Not generalizable: the trailing-
.strip()infold_block_scalaritself -- that wasone wrong line in one folder, and the corpus test is the guard, not a pattern.
Measure before adopting an issue's prescribed fix. #7097 asked for a real YAML
parse and the request looked obviously right; running its own suggested corpus
first showed the reader is deliberately WIDER than YAML in a way two shipped files
depend on, which inverted the fix. The corpus is now a test, so the next person
does not have to rediscover it.
Two parsers disagreeing is a sign the INPUT is wrong. PyYAML and the JavaScript
yamllibrary returned different values for the same block, which looked like alibrary quirk to route around; both were right, and the captured text was missing
the newline that the closing
---implies. Reconstituting it made them agree andexposed a real bug -- a value that changed depending on which field followed it.