Skip to content

fix(skills): resolve the full block-scalar grammar and real chomping - #7648

Merged
bolichen97 merged 1 commit into
mainfrom
fix/skill-frontmatter-yaml-7097
Sep 2, 2026
Merged

fix(skills): resolve the full block-scalar grammar and real chomping#7648
bolichen97 merged 1 commit into
mainfrom
fix/skill-frontmatter-yaml-7097

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

SKILL_LOADER in src/kiro_crew/frontmatter.py reads block scalars with a
hand-rolled scanner that implements less of YAML than the files on disk use. Two
separate defects, both silent:

frontmatter reader returned correct
description: |2- + indented body |2- (the header) the body
description: |- # note + body |- # note (the header) the body
description: | + one one one\n (clip keeps one break)
description: |+ + body + blanks body, blanks dropped blanks kept
a leading blank line inside a block dropped kept (no chomping mode drops it)

The 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 the
module understood |2- and the other did not.

The rest is one line: fold_block_scalar ended in .strip(), which removes a
LEADING 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 -- was
absorbed into the value (|- + body + # note read back as body\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_LOADER a real YAML parse, which is what #7097
originally 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.md
  • src/kiro_crew/builtin_skills/web-verify/SKILL.md

Both carry an unquoted ": " inside description (... Three capture backends: playwright-cli ...), which YAML rejects DOCUMENT-wide with "mapping values are not
allowed 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 |- # note rows are the worst: the value becomes the literal string
"|2-", so a skill's entire description is replaced by two characters of YAML
syntax.

It also pushed cost outward. website/src/components/SkillForm.tsx mirrors this
reader 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 its
header, 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. always and pinned are decided by an
exact meta.get(...).lower() == "true", so honouring keep-chomping would make
always: |+ followed by blank lines read "true\n\n", which is not "true" -- a
skill silently flipping from always-on to off on a file nobody edited.

What changed (motivation -> approach -> change)

  1. One header matcher for every site that recognizes a block scalar.
    _BLOCK_SCALAR_HEADER_RE now 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 the
    onboarding activation gate. An explicit 0 is refused, as YAML forbids it. The
    old six-element BLOCK_SCALAR_INDICATORS frozenset is DELETED -- it was the
    second recognizer, and keeping it is what let the module disagree with itself.
  2. The onboarding activation gate reads that same grammar. It is fail-closed
    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 a true continuation went
    undetected by _column0_activation_declared, was installed verbatim, and was
    then read by SkillsLoader as always == "true" -- external skill-package
    content self-activating into every session, the exact hazard
    automatic_activation_excluded exists to reject. Caught by Design Review, Opus
    and First Principles; measured before and after (three spellings fail-open
    before, none after) and pinned by
    TestTheActivationGateCoversEverythingTheLoaderResolves. An earlier revision of
    this description claimed the gate "must not move" -- that was wrong in the
    safety-relevant direction, and this is the correction.
  3. Collection stops at the block's indentation boundary, on BOTH paths. 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. Collecting on
    "is it indented" absorbed one: |- + body + # note read back as
    body\n# note. Caught by GPT on the explicit-indent path; measurement showed it
    was 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.
  4. Trailing breaks are classified AFTER dedent. A line holding only whitespace
    looks blank -- " ".strip() is empty -- but whitespace BEYOND the block's indent
    is content, so it is not a break and chomping must not eat it: |- over body
    then three spaces is body\n , not body. Found by GPT; the same correction was
    needed in the folded branch, which tested .strip() too and folded such a line
    away entirely.
  5. Explicit indentation indicators are honoured, which is what makes a
    leading-space first line survive -- inferring the indent from the first non-blank
    line cannot express it.
  6. Real chomping. - drops every trailing break, + keeps them all, clip keeps
    exactly one; a leading break is content and is preserved. The .strip() is gone.
  7. The break count includes the last line's own terminator. The fence extractor
    drops the newline before the closing ---, but that newline terminates the last
    content line, so every line inside a fence is newline-terminated. Missing this
    made a value depend on WHICH FIELD CAME NEXT (one when followed by another key,
    one\n at the end of the block) -- caught by the differential matrix, pinned by
    its 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.
  8. Activation flags normalise, case included. The six comparisons reading
    always / pinned with a bare .lower() now .strip().lower(), matching the
    five that already stripped (pinned was read BOTH ways in the same module). The
    frontend's two always comparisons use .trim().toLowerCase(), so the form and
    the loader agree on always: |+ with trailing blanks AND on always: "TRUE"
    (the case half was pre-existing; it is fixed here because this PR is what makes
    the two comparisons claim to agree).
  9. The TypeScript mirror moves with the fold, as test_frontmatter.py's own
    docstring requires. It no longer trims, counts the terminator, and compares in
    the same normalised space as scalarText (which drops one trailing break for a
    block 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.
  10. The corpus test Skill frontmatter reader implements a subset of YAML, forcing the writer to avoid valid constructs #7097 asked for, over every repo-tracked SKILL.md, asserting
    the not-valid-YAML set EXACTLY in both directions so a third such skill is
    visible rather than absorbed.

Tests

  • Differential vs PyYAML, 544 cases, 0 divergences. 13 headers x 25 bodies x 2
    positions (end-of-block and followed-by-a-field). The oracle is
    yaml.BaseLoader, not safe_load, so every scalar stays a str and
    always: true keeps 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.
  • Red on base: 9 of 10 new expectations. Verified by loading origin/main's
    frontmatter.py as a module and running the new expectations against it -- base
    returns '|2-' and '|- # note' as values. The tenth (|- strip with trailing
    blanks) already passed on base, since .strip() and strip-chomping agree there.
  • Chomping gets its own test, as asked. Two of them: the three modes are
    distinguishable on one body, and TestChompingCannotFlipAnActivationFlag pins
    that every block-scalar spelling of true still reads as always-on, with a
    guard assertion so it cannot go vacuous if chomping were reverted.
  • The fail-open gate is measured, not argued. Before: always: |2-, |-2 and
    |- # note each activate the skill (loader reads true) while
    _column0_activation_declared returns False. After: all detected.
    TestTheActivationGateCoversEverythingTheLoaderResolves asserts the invariant
    one-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 indentation boundary has its own tests for all four indicator families on
    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 a
    replaced block-scalar field leaves the author's less-indented comment behind.
  • Named blast radius. Exactly ONE repo-tracked SKILL.md uses a block scalar --
    skills/goal-loop/SKILL.md (description: |) -- and it gains exactly one
    trailing newline (562 -> 563 chars, measured base vs new). Eight snapshot-corpus
    rows move for the same reason; block_scalar_chomped (>-) does not, which is
    the 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.
  • Suites: 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. Frontend
    SkillFormFrontmatter.test.tsx + SkillsTab.test.tsx -- 121 passed.
  • Gates: flake8, isort, mypy, 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 description or
always with a leading blank line or keep-chomping) now opens in the EXISTING
structured editor instead of the EXISTING raw editor, both pixel-identical to their
current selves, and which one is selected is asserted by
SkillFormFrontmatter.test.tsx rather 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, and
that issue is claimed by another owner.

Refs #7187 -- its backendFoldsLiteral mirror moves here. The block-scalar decline
STRING 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 BOTH
spellings -- five comparisons stripped, six did not, and pinned was read both ways
in 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 generalizable is the wrong answer here -- the smell is a hand-maintained set of
literals 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() in fold_block_scalar itself -- that was
one 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
yaml library returned different values for the same block, which looked like a
library quirk to route around; both were right, and the captured text was missing
the newline that the closing --- implies. Reconstituting it made them agree and
exposed a real bug -- a value that changed depending on which field followed it.

@chenmingwei23
chenmingwei23 requested a review from a team September 1, 2026 15:02
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 1, 2026 15:02
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 0fab2b9790f6cdf0fde7a064a56dcd947b64e745 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 always: "TRUE" or a chomping-preserved newline showed the skill as off while the backend activated it — a lying control, now fixed).

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 0fab2b9790f6cdf0fde7a064a56dcd947b64e745 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All mechanical checks done. The sweep claims hold: zero unstripped always/pinned frontmatter comparisons remain (the leftover .lower() == "true" hits are HTTP headers/query params/git output), inject_on_trigger was already stripped at its 3 sites, BLOCK_SCALAR_INDICATORS is gone with no dangling references, and parse_block_scalar_header has 3 real production consumers (read path, write path, import gate). The one candidate sibling — the unstripped triggers truthiness guard at skills.py:4044 — is consequence-free: the per-phrase comma split strips each trigger, so a whitespace-only value scores nothing either way.

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 ships

Intent: make skill frontmatter block scalars read what the file actually says, per YAML — a FIX.

  1. description: |2- / |- # note now reads the body, not the header text — justified
  2. Chomping is real: |+ keeps trailing blanks, clip keeps one break, leading blank kept — justified
  3. A less-indented # comment is no longer absorbed into a block value, read and write — justified
  4. always/pinned compare normalized everywhere (6 sites), backend and form — justified companion; without it item 2 flips activation
  5. Whitespace-only repo_scope no longer suppresses an unscoped skill (3 sites) — justified, declared
  6. Import gate detects every header the loader resolves, staying fail-closed — justified (named security boundary)
  7. Gate now catches always: " true " inside quotes — rides along, declared; closes a fail-open spelling
  8. CRLF documents read like their LF twins inside blocks — justified companion to the spaces-only rule
  9. Editor now edits leading-blank / keep-chomped literals it refused; folded/explicit-indent still refused — justified, divergence removed at source
  10. always: "TRUE" reads as on in the form — pre-existing case defect, rides along, declared

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 BLOCK_SCALAR_INDICATORS is the cause-level move: the defect was two recognizers disagreeing, and one of them no longer exists to diverge.

[FIRST-PRINCIPLES-REVIEWED] 0fab2b9

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 0fab2b9790f6cdf0fde7a064a56dcd947b64e745 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0fab2b9790f6cdf0fde7a064a56dcd947b64e745 — this comment is updated in place on each push.

Review details

I've traced the candidate against the actual read path (_parse_block_lines, lines 837–859) and write path (set_frontmatter_fields, lines 623–689).

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 nxt_indent == 0 (line 848), while the write path, seeing a tab-led column-0 line, sets block_indent = 0 (line 680, since 0 < blank_floor is false) and consumes it rather than breaking. So "\torphan" is dropped when the description field is rewritten but ignored by the reader.

But bar (c) — an observable wrong outcome — does not hold:

  • A column-0 tab-led line ("\torphan") has no colon, so the reader's main loop skips it (line 795); it is not stored as any field and is not a comment any consumer decodes. It is "left standing" only as dead bytes.
  • A real YAML parser rejects such a document outright (tab as block-scalar indentation), so the differential oracle skips these shapes — there is no "correct" reading to diverge from.
  • _verify_round_trip re-parses to the same field dict (the reader ignores the line either way), so no crash.

The only column-0 lines that reach line 675 without breaking at line 652 are tab-led ones (a space-led line has nxt_indent ≥ 1; a non-space column-0 line breaks at 652), and those are precisely the malformed inputs no consumer reads. The candidate's own note concedes it could not construct a case where a well-formed field or authored comment is lost. (a)/(b) hold; (c) fails. Dropped.

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 0fab2b9790f6cdf0fde7a064a56dcd947b64e745.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 0fab2b9790f6cdf0fde7a064a56dcd947b64e745: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0fab2b9790f6cdf0fde7a064a56dcd947b64e745 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/frontmatter.py:87 -- [ \t]+ accepts tab-separated block-scalar comments that YAML rejects, contradicting the documented YAML grammar -> Fix: allow spaces only before #. (origin: validation)
[GPT-REVIEWED] 0fab2b9

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 0fab2b9790f6cdf0fde7a064a56dcd947b64e745: <one-sentence reason>

@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from 9cc2cfa to b1c0c5c Compare September 1, 2026 15:32
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 1 dispositions on b1c0c5c8c. Every finding accepted -- three lanes found the
same real regression I introduced, and my PR body's claim about it was wrong.

BLOCKING (Design, Opus, First Principles): the import gate went fail-OPEN -- FIXED

Accepted in full, and the mechanism is exactly as described. Reproduced before the
fix, over the loader's now-resolvable spellings:

always: gate detects loader reads activates
|2- False true yes -> FAIL-OPEN
|-2 False true yes -> FAIL-OPEN
|- # note False true yes -> FAIL-OPEN
|, |-, >+, true True true yes (ok)

Design put the invariant better than my PR body did: the gate is fail-closed only
while its detected set is a SUPERSET of the loader's resolvable set, so widening the
loader IS a change to the gate even though I never opened its file. My description
asserted the opposite ("that gate must not move") -- withdrawn, and the PR body now
carries the correction rather than quietly dropping it.

Fix: _column0_activation_declared reads the grammar from
parse_block_scalar_header instead of its own list, so anything the loader resolves
is detected. All three rows above now read True, with none of the "ok" rows moving.
BLOCK_SCALAR_INDICATORS is DELETED -- it was the second recognizer and its only
non-test consumer was that gate, which is First Principles' point about it. Pinned by
TestTheActivationGateCoversEverythingTheLoaderResolves, asserted one-directionally
(the gate may be stricter, never looser) over eleven spellings, plus tests that it
stays stricter where it should and does not start refusing false / no / 0 /
|0 / maybe.

BLOCKING (GPT): explicit-indent scalars absorb a less-indented comment -- FIXED, and it was wider than reported

Accepted -- and measurement says the finding under-states it. GPT scoped this to
explicit indents and prescribed reverting that support. Both readings, measured:

input base round 1 now yaml
|2- + body + # note (header) body\n# note body body
|- + body + # note body\n# note body\n# note body body

The second row is the same defect with NO explicit indicator, wrong on base already.
So reverting explicit-indent support would have left the bug in place -- the cause is
the collection loop taking any indented line as content, not the indicator. Fixed the
way GPT's own sentence names ("until collection enforces the declared indent
boundary"): collection stops at the first non-blank line indented less than the
block's content, which is the declared indent when the header gives one and otherwise
the first non-blank line's own indent. A comment indented PAST the content is still
content, as YAML has it, and that has its own test.

FINDING (GPT): quoted always: "TRUE" -- FIXED

Correct. The loader does .strip().lower(); the form did .trim() only, so
always: "TRUE" showed as off while the loader injected the skill. Pre-existing in
the case dimension, fixed here because this PR is what makes the two comparisons
claim to agree -- leaving it would have made my own code comment false. Both sites now
.trim().toLowerCase(), with a test over "TRUE" / True / true and a
negative set (false / FALSE / no / 0 / empty) so the normalisation did not
turn the check into "any non-empty value activates".

UX (Fable 5): PASS -- noted

No change requested. Its reading of the user-visible effect matches what the PR body
now says.

Also fixed this round, found by CI rather than a lane

test_skills_frontmatter.py and test_skill_update_flow.py carried block-scalar
expectations my local run missed (Backend Tests shard 3.10/4 caught them). All 15
updated, and each new expectation was verified against yaml.BaseLoader on the
reconstituted block rather than copied from the implementation -- 17 fixtures, 0
mismatches. Two of them, test_folded_with_chomping_keep and
test_literal_with_chomping_keep, had expectations IDENTICAL to their strip
counterparts, which is precisely the defect this PR fixes; they now differ.

Suites re-run: 1885 backend passed, 121 frontend passed, all gates clean.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from b1c0c5c to 2eedae3 Compare September 1, 2026 15:50
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 dispositions on 2eedae371. Both findings accepted; one of them exposed a
gap in my own test matrix.

BLOCKING (GPT): trailing whitespace content silently discarded -- FIXED

Correct, and the prescribed fix ("dedent before classifying trailing breaks") is the
right one. Measured before the fix -- a line holding only whitespace looks blank
(" ".strip() is empty), but the whitespace BEYOND the block's indent is content, so
it is not a trailing break and chomping must not eat it:

input (indent 2) before yaml
|- + body + (3 sp) body body\n
| + body + body\n body\n \n
|+ + body + body\n\n body\n \n
>- + body + body body\n
|- + body + (2 sp, == indent) body body (already right)
|- + body + (1 sp, < indent) body body (already right)

Classification now happens on the DEDENTED line: an empty string is a break, a
whitespace-only line is the content line it actually is. The folded branch needed the
same correction -- it tested .strip() too, so it folded such a line away entirely.

The more useful part of this finding is what it says about my test matrix. The
differential-vs-PyYAML matrix I added used only truly EMPTY strings for blank lines,
never whitespace-only ones, so 350/350 agreement was measured over a body space that
could not express this case. Twelve whitespace-only bodies are now in BODIES with a
comment naming why they are not interchangeable with empty ones, plus a direct test of
each row above. The matrix is 544 cases, 0 divergences, and its floor assertion moved
from 250 to 450 so the rows cannot quietly disappear.

Two regressions my own refactor introduced on the way, both caught by the widened
matrix before pushing rather than by a reviewer: an off-by-one in the all-blank-block
break count (|+ over one blank line), and the folded branch dropping a
whitespace-only line. Named because "the matrix caught it" is the only reason this
round is one push and not three.

Opus: no findings -- and its self-dropped candidate is FIXED anyway

Opus raised the write path (set_frontmatter_fields) over-consuming a block scalar's
less-indented trailing comment, then dropped it on reachability: the only production
caller is steering's _apply_declaration, whose writable fields are inclusion and
fileMatchPattern (enum tokens and globs), so the triggering document is contrived.

I verified it rather than taking the drop, and fixed it. It reproduces through that one
caller:

---                          set_frontmatter_fields(inclusion="always")
inclusion: |-        ->      ---
  manual                     inclusion: always
 # why                       ---          <- ` # why` gone

Reachability is as narrow as Opus says. I fixed it because it is an asymmetry THIS
change introduced: I tightened the read path's collection boundary and left the write
path on the old rule, so the writer treated a less-indented comment as block content.
Deleting an author's note is exactly the loss the walk's plain-value branch already
guards against ("a mode edit silently deleting the rationale beside a declaration is
exactly the kind of quiet loss this writer must not cause") -- shipping the block-scalar
half of that guard broken, on the grounds that no one would hit it, is not a trade I
wanted to make in a PR whose whole subject is the reader and writer agreeing. Both
paths now stop at the same boundary; a comment indented INSIDE the block is still the
field's content and still travels with it, which has its own assertion.

Held the fix rather than pushing it when I found it, so it costs one review round
together with the GPT finding instead of two.

Design, First Principles, UX: PASS on the previous head, nothing outstanding

No advisories to disposition. First Principles independently re-derived the consumer
counts and the flag-site grep, which is a stronger check than my own claim.

Suites re-run: 1887 backend passed, 121 frontend passed; flake8, isort, mypy, black
baseline, tsc, eslint clean.

@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from 2eedae3 to 8dcd6ef Compare September 1, 2026 16:29
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3 on 8dcd6ef26. One CI failure, no reviewer findings.

test_yaml_safe_loading.py::test_no_yaml_load_call_sites -- FIXED

Not a Windows problem despite surfacing on Backend Tests (Windows) (4): that shard
simply reached the guard first while the other shards were still queued. It fails on
every platform.

The guard forbids yaml.load( anywhere under src/ or test/, and its reasoning is
the point -- yaml.load is only as safe as its Loader= argument, and that safety is
invisible at the call site to a reviewer and to every scanner matching on the call
name. My four differential-oracle calls passed Loader=yaml.BaseLoader, which IS safe,
and that is exactly the case the guard says not to make a reader chase.

The sanctioned path is load_with(loader_cls, stream) from test/yaml_helpers.py,
which refuses any loader_cls that does not subclass yaml.SafeLoader. BaseLoader
cannot be passed: it is a SIBLING of SafeLoader, not a subclass. And I do need
BaseLoader's behaviour rather than safe_load's -- the reader's contract is
dict[str, str], so always: true must read the string "true"; under safe_load the
oracle would return True and disagree with the reader about a value neither of them
mis-parsed.

Resolved without weakening either constraint: a yaml.SafeLoader subclass with its
implicit-resolver table emptied. With no implicit resolver to match, every plain scalar
falls through to DEFAULT_SCALAR_TAG (str) while collections still resolve normally
-- BaseLoader's semantics from a safe base class, driven through load_with.

Verified equivalent rather than assumed: measured against BaseLoader over 348
documents, 0 differences, including every shape where the distinction bites --
true, yes, 123, 1.5, null, ~, a date, a flow sequence and a flow mapping.
The 544-case differential matrix is unchanged at 0 divergences, so the oracle swap did
not move what the tests assert.

The guard now passes (3 passed), and the only remaining yaml.load( string in the tree
is the allowlisted file that asserts its absence as string data.

Suites: 1890 backend passed. flake8, isort, mypy, black baseline clean; frontend
untouched this round.

Reviewer lanes on the previous head 2eedae371: all clean, nothing outstanding

UX PASS, First Principles PASS, Design PASS, Opus no findings -- and Opus raised no
candidate this round, the write-path one from round 2 having been fixed. GPT had not
reported on 2eedae371 before this push, so its next verdict will be its first on the
current code.

First Principles independently re-derived the counts I claimed (the remaining bare
.lower() == "true" sites read HTTP/CLI/git values, not frontmatter; trigger tokens
are already stripped per token), which is a stronger check than my own assertion of
the same thing.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

CI status note on 8dcd6ef26 -- two reds, neither owned by this PR. Recording the
attribution so nobody has to re-derive it.

Frontend Lint & Type Check -- MAIN-OWNED, pre-existing ratchet breach

The job reports 604 problems (0 errors, 604 warnings) and exits 1 against
npx eslint src/ --max-warnings 603. Zero errors: this is purely the warning ratchet,
one over the line.

This PR contributes zero warnings, and the arithmetic is checkable rather than
asserted:

  • The two frontend files it touches -- src/components/SkillForm.tsx and
    src/test/SkillFormFrontmatter.test.tsx -- report 0 warnings. Measured at BOTH
    revisions: at this head, and again after checking out the origin/main version of
    those same two files.
  • eslint warnings are per-file and independent, so a diff whose files contribute 0
    before and 0 after cannot move the total.
  • Therefore the base commit this branch was cut from, 1ee69f225, already computes 604
    in CI's environment -- the ratchet was breached before this branch existed.
  • --max-warnings 603 is unchanged on main since that base (git diff on ci.yml
    shows no edit to the threshold).

Locally the same tree counts 603, i.e. exactly at the limit. The one-warning gap
between CI and local is environmental, not this diff: CI sees one extra warning in
src/apps/design-tweak/DesignTweakPage.tsx and one in src/pages/ArtifactsPage.tsx
(neither touched here), and does not lint
src/apps/issue-radar/components/crew-ghost-sprite.gen.mjs, which the local run does.
Net +1.

Per the repo's own convention for main-owned frontend reds I am NOT folding a burn-down
into this PR: it would put unrelated files in a diff about the frontmatter reader, and
the fix belongs to whoever is tracking the ratchet. This clears either when a warning is
burned down on main or when the threshold moves, and I will rebase onto settled main
rather than touch it here.

Backend Tests (Windows) (2) -- infrastructure flake

PermissionError: [Errno 13] Permission denied on
...\pytest-0\popen-gw1\test_start_registered_kind_ret0\app-data\jobs\<uuid>.json.
A Windows file-locking error on a pytest tmpdir, in job-runner tests. Not an assertion
failure -- no behavioural claim broke -- and this PR touches no job-runner or app-data
code (git diff --name-only is limited to frontmatter.py, onboarding_import.py,
skills.py, skill_budget.py, three test files and two frontend files).

Windows shards 1, 3 and 4 pass on this head -- shard 4 being the one that caught the
yaml.load guard last round, so that fix is confirmed by the same matrix. Of the Linux
shards only 3.10/4 has finished so far (green); the rest are still running, so I am
not yet claiming the whole matrix. Re-running shard 2 once the workflow finishes; no
code change for it.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from 8dcd6ef to c24bf7f Compare September 1, 2026 21:39
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3 dispositions on c24bf7f53. Both findings accepted -- and they were only
readable because the gate's incomplete verdict hides them.

Where these came from

GPT's lane has posted "review incomplete" three times on 8dcd6ef26
(pass(es) 1 2 twice, then pass(es) 2). The third attempt is the useful one: pass 1
COMPLETED and emitted two BLOCKING candidates plus a [BLOCK-MERGE] marker, but pass
2 -- whose job is to falsify them -- did not, so the gate failed closed and the PR
comment shows no verdict at all. The findings exist only in the job log, between the
DISCOVERY_1_BEGIN/END markers, where the harness itself labels them "UNTRUSTED
EVIDENCE -- a prior model's guesses".

So I ran the falsification pass 2 never got to. Both candidates survive it, measured.

BLOCKING 1: padded quoted always bypasses the import activation gate -- FIXED

Real, and it is the SAME fail-open class as round 1, from the same cause on the other
side of the expression. _column0_activation_declared stripped whitespace only
OUTSIDE the quotes:

raw.strip().strip("\"'").casefold()

so always: " true " unquotes to true (padding intact) and matches no truthy
word -- while the loader's consumers compare .strip().lower() == "true" and DO
activate it. Measured before the fix:

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 override at any point.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from c24bf7f to 7e4d86e Compare September 1, 2026 22:46
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 4 disposition on 7e4d86e52. GPT's finding accepted, and chasing it properly
turned up three more defects of the same family plus the reason my own matrix kept
missing them.

The finding: explicit-indent whitespace content is erased -- FIXED

Real. Under an explicit indicator the HEADER fixes where content starts, so |2- over a
line of three spaces is one space of CONTENT. fold_block_scalar decided the block was
empty BEFORE dedenting, judging each raw line with .strip(), so the line looked blank
and the whole scalar came back "".

Measured against the oracle, the finding was broader than the one shape it named --
10 divergences of 20 in that family:

header + body before yaml
|2- + three spaces '' ' '
|2 + three spaces '' ' \n'
|2+ + three spaces '\n' ' \n'
|1- + three spaces '' ' '
|2- + four spaces '' ' '
>2- / >2 + three spaces '' ' ' / ' \n'
|2- + tab past indent '' '\t'
|2- + two whitespace lines '' ' \n '
|2+ + two whitespace lines '\n\n' ' \n \n'

Every implicit-indent case agreed, which is what localises it: the early exit is correct
when the indent comes from the content and wrong only when the header declares one. The
emptiness decision now happens after the dedent. All 20 agree.

One row in my first measurement was a PROBE artifact, not a defect -- I had built the
document with block.rstrip("\n"), which deletes the trailing blank lines that ARE the
input for the |+ cases, so the reader and the oracle were being shown different
documents. Corrected before fixing anything; |+ over two blank lines was always right.

Three more defects the same investigation found

Indentation is spaces, never tabs. Under |, two spaces then a tab is two columns
of indent followed by a TAB OF CONTENT ('\t\n'). Both the folder and the read path
judged content with .strip(), which counts a tab as whitespace, so such a line looked
blank. Two consequences, the second worse than the first: the folder found no content
line and returned ""; and in the READ path's boundary walk the "blank" tab line let a
following more-indented line set a deeper boundary, so | + \t + deep +
one silently dropped its last line ('\t\n deep\n' for '\t\n deep\none\n').
Both walks now measure in spaces.

The write path had the same walk. Left alone it would have disagreed with the reader
about where the block ends, so replacing an unrelated field would move the author's
lines. Fixed symmetrically and verified: over 13,320 documents, rewriting name
leaves the block's lines byte-identical and its value unchanged (0 moved, 0 changed).

The TypeScript mirror had drifted. backendFoldsLiteral predicts the reader so the
skill form knows when an edit is safe. It still classified trailing breaks BEFORE dedent
(round 2's bug), detected indent with \s (the tab bug), and used a boundary rule the
reader no longer has. Direction was conservative -- it refused edits the backend could
take, never corrupted one -- but a mirror that lags the thing it mirrors is exactly the
asymmetry round 1 flagged. Now checked mechanically rather than by eye: the function is
EXTRACTED from the .tsx and run in node against the Python read path over the same
documents -- 3330 compared, 0 divergences.

Why my matrix kept missing these, and what changed

Three rounds running, a reviewer found a line shape my differential matrix could not
express, because BODIES was a hand-enumerated list: round 2 whitespace-only trailing
lines, round 3 content followed by trailing spaces, round 4 whitespace past an explicit
indent. Each round I appended the shape just found -- which never anticipates the next
one.

The body space is now GENERATED from the line kinds that behave differently (empty,
whitespace short of / at / past the indent, tab past the indent, bare tab, content,
content with trailing spaces, more-indented, more-indented with trailing spaces), over
all 1- and 2-line combinations plus 3-line combinations of the discriminating subset.
5328 oracle-checked cases, up from 738, floor assertion 450 -> 5200.

That change earned itself immediately: on its first run it failed on | + \t, the
tab defect above, which neither my list nor the review had found. A local sweep at
20,880 cases (3-line bodies over all kinds) is also clean.

The honest lesson is that "0 divergences" was never a statement about the reader -- it
was a statement about the generator, and the generator was the weakest part of the
evidence.

Verification

1635 backend passed (3 new tests), 121 frontend passed (5 new cases), flake8, isort,
mypy, black baseline, tsc, eslint clean. The 2 mypy errors reported locally are
pre-existing in transcribe.py via follow-imports, untouched here, and CI's Backend
Lint is green on 3.10 and 3.12.

Still outstanding, not owned by this PR

Frontend Lint & Type Check -- main-owned ratchet (604 warnings vs --max-warnings 603, 0 errors; this PR adds zero). #7696 restores the threshold and clears it. No
/ai-review override has been used at any point on this PR.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

CI attribution for 7e4d86e52. Both reds are MAIN-OWNED, and neither touches a file this
PR changes. Evidence for each, not inference.

Backend Tests (Windows) shard 3 -- census ratchet on slack/gateway.py

test_security_posture.py::TestGateSideLogRedactorSpelling
  ::test_no_new_gate_side_log_line_reads_the_baseline_redactor
AssertionError: New gate-side log/audit line(s) reading the BASELINE redactor
slack/gateway.py: 7 sites, census says 6.

Proven main-owned by running that single test against a pristine origin/main
checkout
(a detached worktree at main tip, nothing of this branch in it): it fails
there with the byte-identical message. _BASELINE_LOG_SITE_CENSUS on main tip carries
"slack/gateway.py": 6 while the scan counts 7.

This branch contains no slack/ change at all (git diff --name-only origin/main...HEAD | grep -i slack is empty). It reaches this PR because a
pull_request run builds a MERGE REF of the head and main's tip, so a main-side ratchet
break lands on every open PR without anyone rebasing -- the same mechanism as the
frontend-lint ratchet below.

Fix already in flight as #7758 ("fix(ci): use context redactor for Slack log"), open and
mergeable. Not folding it in.

Backend Tests (Windows) shard 2 -- known tmpdir flake

test_job_routes.py::test_cancel_live_cancellable_run_is_200_cancelling
PermissionError: [Errno 13] Permission denied:
  '...\\pytest-0\\popen-gw1\\test_cancel_live_cancellable_r0\\app-data\\jobs\\....json'

A Windows-only teardown race on the pytest temp directory, in the job-routes suite --
nothing this PR imports or touches. The shard is otherwise fully green: 18853 passed,
1 failed
. The same class of failure on an earlier head passed on re-run against
byte-identical code, which is what makes it a flake rather than a finding. Re-running the
job alone (not the run) so the review lanes are not re-rolled.

Worth filing separately from this PR, and deliberately not filed as part of it.

Frontend Lint & Type Check -- resolved on main, by a different PR than expected

I had this attributed to #7696 ("restore the eslint ratchet to the count the tree
actually measures"), which is still open. It was actually fixed the other way: #7722
merged at 23:13:37Z
and memoized the artifacts fallback, burning the count back under
--max-warnings 603 instead of raising the threshold. So the lane clears from main's
side and #7696 is now redundant to this PR's needs.

Why I am not rebasing yet

Main's tip carries the eslint fix (#7722, merged) but ALSO still carries the census break
(#7758, open). A rebase now would absorb the first and leave the second red, so it would
have to be repeated -- and every push re-rolls the non-deterministic GPT lane. Waiting
for #7758 to merge and then taking BOTH in one rebase costs one re-roll instead of two.

Nothing in this PR's own code is implicated by either red: 1635 targeted backend tests
and 121 frontend tests pass locally, with flake8, isort, mypy, the black baseline, tsc
and eslint all clean.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

All five review lanes are clean on 7e4d86e52, so this dispositions the two advisories
rather than any blocking finding.

Verified by reading each lane's marker comment BODY, not its check conclusion: GPT 5.6
"no blocking findings", Opus 4.8 "no blocking findings", Design PASS, UX PASS, First
Principles PASS. Opus posts under <!-- claude-ai-review --> rather than an
opus-review marker -- worth stating because grepping the name in the check title
returns an empty body and looks like a lane that never ran.

First Principles: byte-identical duplicated list comprehension -- FIXED

Correct, and it was mine from the round-4 fix: adding the post-dedent emptiness check
introduced its own dedented = [...] and left the pre-existing identical one below it,
so fold_block_scalar computed the same list from the same inputs twice. The second is
removed.

Behaviour-neutral by construction, and measured as such: 557 targeted tests pass, the
20,880-document local reader sweep still reports 0 divergences, and the cross-language
mirror check still reports 0 over 3330 documents.

Held locally as e5810dadc rather than pushed. The branch needs one rebase anyway once
#7758 lands (see the attribution above), and every push re-rolls the non-deterministic
GPT lane -- which has just gone clean here. Pushing a dead-code removal on its own would
spend that re-roll for no merge benefit, so it rides along with the rebase and I will
name the final SHA then.

Design: nothing in the build enforces the Python/TypeScript mirror parity -- ACCEPTED, DEFERRED to #7763

The gap is real and this PR is the evidence for it: the mirror was found three separate
ways out of step with the reader during review (trailing breaks classified before dedent,
indentation measured with \s so a tab counted as indent, and a collection boundary
missing both the column-0 and less-indented rules). All three were conservative --
refusing edits the backend could take -- but a mirror that drifts the other way would let
the form edit a field it corrupts on save.

I did build the mechanism Design is asking for, as a throwaway probe: it EXTRACTS
backendFoldsLiteral from the .tsx at test time (so the harness cannot drift from what it
validates), generates documents from named line kinds, and compares against the READ PATH
rather than the fold alone -- which is what makes a collection mismatch visible. 3330
documents, 0 divergences, and it caught the tab defect on its first run.

Filed as #7763 with that design written down, deliberately not folded in here: this PR
had just converged with all five lanes clean, and adding a cross-language test harness to
it would restart the review cycle for a change that fixes nothing this PR broke. One
correction to Design's phrasing carried into the issue -- a frozen JSON fixture blob is
the wrong shape. A hand-enumerated case list is exactly what failed three rounds running
in this PR, each round finding a line shape the list could not express; the corpus needs
to be GENERATED from line kinds so new shapes come from naming a kind rather than from a
reviewer finding one.

Remaining reds, all main-owned and unchanged

The census break (slack/gateway.py: 7 sites, census says 6) now shows on three shards
-- Windows 3, 3.10 shard 3, 3.12 shard 3 -- which is the same single test landing in
whichever shard holds it, not three problems. Proven against a pristine origin/main
worktree; fix is #7758. Windows shard 2 remains the test_job_routes.py tmpdir flake
(18853 passed, 1 failed); re-running that job alone is still refused while the run is in
progress. Frontend Lint is red on a merge ref cut before #7722 merged and clears with
the rebase.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch 3 times, most recently from e5c989b to e847966 Compare September 2, 2026 00:51
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 5 on e847966a7. GPT 5.6, Opus 4.8, Design and UX were all clean on the previous
head e5c989b4a; this dispositions First Principles' CONCERNS and attributes the one
remaining shard red.

First Principles: repo_scope left unswept -- FIXED

Accepted, and it is exactly right about the root cause. My normalisation sweep was
justified as "exact use of a value that can now carry a break" and then stopped at the
BOOLEAN flags -- but whether the value is a boolean or a path makes no difference to that
argument. repo_scope is consumed by exact path containment at both gating reads, so a
repo_scope: | block resolving src/x\n fails containment and silently suppresses the
skill inside its own repo. That is a regression THIS PR introduces for that spelling:
base's .strip() in the fold happened to hide it.

The inconsistency was visible on adjacent lines, which is the part I should have caught:

if meta.get("always", "").strip().lower() == "true":   # normalised
    scope = meta.get("repo_scope", "")                 # not

Both gating reads now strip. Placed at the READ rather than inside
_repo_scope_satisfied on purpose: the callers test if scope before calling, so
stripping at the read keeps a whitespace-only value falsy and meaning "unscoped", where
normalising inside the gate would hand it an empty path that matches nothing while the
caller still believed the skill was scoped.

The two remaining unstripped repo_scope reads are deliberate -- one is redacted for a
listing, one populates an API dict. Neither compares.

I also re-ran the sweep the finding implies rather than fixing only the named sites, and
repo_scope was the whole gap: pinned and inject_on_trigger are already normalised at
every site as str(...).strip().lower(), triggers strips per token, and name /
description are free text. repo_scope in cron.py and learn.py is the lessons
subsystem, not skill frontmatter.

Pinned by test_a_block_scalar_repo_scope_still_gates_to_the_same_path in the existing
chomping/flag class, which asserts five spellings resolve to one path, asserts the raw
value really does carry the break (so the test cannot go vacuous if chomping were
reverted), and asserts a whitespace-only value stays falsy.

1637 targeted backend tests pass; flake8, isort, mypy, black baseline clean. The 2 mypy
errors reported locally are pre-existing in transcribe.py via follow-imports.

Backend Tests (Windows) shard 3 -- not this PR

FAILED test/test_session_control.py::test_the_created_agent_name_is_sanitized_before_storage
FAILED test/test_session_control.py::test_the_audit_write_does_not_run_on_the_event_loop
SessionControlError: too many sessions created
2 failed, 17993 passed, 1428 skipped

Neither session_control.py nor its test is in this branch's diff -- the diff is nine
files, all frontmatter/skills/onboarding plus the skill form and its test. Both tests pass
locally. too many sessions created is a session-creation quota guard, so the failure is
a count carried across tests in one parallel worker rather than anything a frontmatter
parser can reach; it is order- and timing-dependent, which is why it appears on one
Windows shard and not on the Linux matrix running the same tests.

The census break that was failing this shard earlier is GONE: #7761 merged, and I verified
the census test now passes both on a pristine origin/main worktree and on this branch.
#7758 and #7762 are redundant to it.

Lane state

GPT 5.6 and Opus 4.8 both reported "no blocking findings" with markers naming
e5c989b4a, Design PASS, UX PASS. UX and First Principles had each come back "could not
complete" on the first roll of that head -- their own logs say "errored / overloaded /
incomplete -- NOT blocking" with the review action conclusion=skipped, and neither log
contained a verdict line, so there was nothing stranded in them; re-running those two
workflows alone produced the real verdicts without disturbing the blocking lanes. No
/ai-review override has been used anywhere on this PR.

@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from e847966 to a337c81 Compare September 2, 2026 01:10
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 6 on a337c8182. First Principles was right on both counts, and one of them
corrects something I asserted on the record last round.

The third gate call site -- FIXED

grep self._repo_scope_satisfied( gives three call sites, not two. I stripped at
skills.py:4000 and :4048 and then wrote here that the remaining repo_scope reads
"are deliberate -- one is redacted for a listing, one populates an API dict. Neither
compares." That was wrong: the row built at skills.py:1931 IS consumed by the gate at
skills.py:4283.

The consequence is worse than the original defect, and it was mine: each site guards on
the value's TRUTHINESS before calling the gate, so after a partial sweep one document was
read as unscoped at two sites and handed to the gate at the third. The same file behaved
two different ways depending on which path reached it. Fixed by normalising where the row
is built, so all three agree.

My stated harm was overstated -- CORRECTED

First Principles also caught that the rationale I wrote into the code, and repeated in my
last comment, does not hold: I claimed a trailing break would suppress a skill inside its
own repo via exact path containment. It would not. project_scope_satisfied strips its own
fragment (project_scope.py:59 and :149, both pre-existing), so src/x\n was always
gated as src/x.

That claim came from the round-5 finding's own wording, and I accepted it without opening
the gate it named. Verifying the mechanism is not the same as verifying the harm, and I
checked only the first. The comments at both read sites, the test class docstring and the
test's rationale are all rewritten to state what is actually true: only the
WHITESPACE-ONLY value was ever a defect, because it is truthy while meaning nothing.

Which spellings actually reach the gate

Writing the regression test I asserted that repo_scope: | over three spaces is truthy.
It is not -- it resolves to "", because with the indent taken from the content a
whitespace-only block has no content line at all. My own test caught that before it
reached CI. Measured, the truthy-but-blank spellings are keep-chomping (|+, >+), an
explicit indent (|2, |2-, |2+), and a tab past the indent (| over \t, which is
truthy only because of the tab fix earlier in this PR):

spelling resolves to truthy blank after strip
| + three spaces '' no --
|+ + one blank '\n' yes yes
|2 + three spaces ' \n' yes yes
|2- + three spaces ' ' yes yes
>+ + one blank '\n' yes yes
| + tab past indent '\t\n' yes yes

The test now covers all five truthy spellings plus the falsy one for contrast, and asserts
truthiness-as-parsed explicitly, so it cannot silently stop exercising the inconsistency.

Not taking the prescribed subtraction as written

The finding suggests replacing the two consumer strips with one at the row build, on the
grounds that it "also covers the third caller". It does cover the third caller, but it
cannot replace the other two: skills.py:4000 and :4048 read meta.get("repo_scope")
from _cached_frontmatter directly and never touch the row built at 1931, so dropping
their strips would reopen the defect at both. All three reads are normalised instead --
one more strip, not one fewer.

A second test pins that mechanically rather than by eye: it greps every repo_scope read
in skills.py, asserts each carries .strip(), and asserts the gate is only ever called
with one of those normalised values. A fourth site added later fails the test rather than
shipping.

1638 targeted backend tests pass; flake8, isort, black baseline clean, and the 2 mypy
errors are the pre-existing transcribe.py pair via follow-imports.

Lane state on the previous head

GPT 5.6 and Opus 4.8 both "no blocking findings", Design PASS, UX PASS, all with markers
naming e847966a7. No /ai-review override has been used anywhere on this PR.

@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from a337c81 to ac5b794 Compare September 2, 2026 01:31
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 7 on ac5b7940e. GPT's finding accepted -- correct, reachable, and its prescribed
remedy was right too. First Principles went to PASS on a337c8182, so both of its earlier
concerns are closed.

BLOCKING: a leading blank line let an outer comment be consumed -- FIXED

Real, and it is the third variant of the note-deletion class this PR exists to close. A
leading all-space line takes part in detecting a block scalar's indentation, so a
SHALLOWER line after it ends the scalar rather than becoming its first content line.
Neither collection loop applied that, so the walk took the note as content -- and
rewriting the field then replaced it away.

I did not take the finding on its wording. My first reproduction attempt FAILED: rewriting
name on five candidate documents preserved the comment every time, because the walk that
over-consumes is the one following the REPLACED key, not any earlier field. Widening to
every dialect x every target found it: SKILL_LOADER and SKILL_UPDATE, body
description: | + + # outer comment, target description -- the comment is gone
from the output.

Then the oracle settled what the right answer IS, which mattered because two neighbouring
shapes disagree:

document yaml reader before
| + + # note '' ' \n# note\n' <- over-consumed
| + # note (no blank) '# note\n' '# note\n' <- already correct
|2 + + # note ' \n' ' \n' <- already correct

So the fix cannot be "never consume an indented comment": with no leading blank the
comment genuinely IS the first content line and scalar content, and an explicit indicator
fixes the indent itself. What was missing is precisely a FLOOR from the leading blanks --
which is what the finding prescribed. Applied to the read walk, the write walk, and the
TypeScript mirror, all three kept identical.

Verified: all six oracle shapes agree; the write path loses nothing across 22,608
rewrite documents (0 lines moved, 0 values changed); and the mirror agrees with the reader
across 5652 documents after taking the same floor -- it had 408 divergences the moment the
reader moved, which is what checking it mechanically each round is for.

The generated matrix now covers the class

The corpus gained two line kinds -- # note and shallow, content at depth 1 -- because
the shape needs a line SHALLOWER than a leading blank and every existing kind was either
blank, at the body indent, or deeper. Committed matrix: 5928 oracle-checked cases, up
from 5328, floor 5200 -> 5800. A wider local sweep over 3-line bodies runs 29,676 cases,
0 divergences.

This is the fourth round where a reviewer found a line shape the corpus could not express,
and the second where adding the kind was the whole fix. The generator is now the thing
being maintained rather than a case list, which is why each of these has been a one-line
addition rather than a redesign.

Verification

1639 targeted backend tests pass, 121 frontend, and flake8 / isort / mypy / black baseline
/ tsc / npx eslint src/ --max-warnings 604 are clean. The 2 mypy errors reported locally
are the pre-existing transcribe.py pair via follow-imports.

No /ai-review override has been used anywhere on this PR.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
…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.
@chenmingwei23
chenmingwei23 force-pushed the fix/skill-frontmatter-yaml-7097 branch from ac5b794 to 0fab2b9 Compare September 2, 2026 02:06
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 8 on 0fab2b979. GPT's CRLF finding accepted -- real, a regression from this PR, and
its prescribed fix was the right one. Opus 4.8 was clean on ac5b7940e, and Design, UX and
First Principles all PASS there.

BLOCKING: CRLF blank lines terminated block scalars early -- FIXED

Real. Judging "blank" in SPACES (which the tab fix earlier in this PR introduced) means a
CRLF blank line -- "\r", not "" -- is no longer empty, so it counted as a content line
at indent 0 and ENDED the scalar.

Measured against base under STEERING_LOADER, the dialect whose extraction mode actually
parses a CRLF fence:

document base this branch before the fix
| + one + blank + two 'one\r\n\ntwo' 'one\r\n' -- two LOST
| + blank + one 'one' '' -- value lost whole

95 of 108 read cases differed from base; most are this PR's intended chomping change, but
those two rows are data loss and they are mine.

I nearly filed this as a rebuttal, and the way I got there is worth recording. My first
measurement used SKILL_LOADER and reported ZERO delta over 240 CRLF documents -- true,
but irrelevant: SKILL_LOADER's extraction is column0_fence, which is LF-only, so it
returns no keys at all for a CRLF document on base and on this branch alike. The finding
said "steering", and STEERING_LOADER is the one dialect using column0_fence_crlf.
Reading the finding's own scenario more carefully would have pointed at the right dialect
immediately instead of at a measurement that could only ever come back clean.

The fix strips the trailing carriage return before classification on both walks, which is what the
finding prescribed. It also fixes the VALUE, which base had wrong too: YAML normalises line
breaks, so a CRLF document must resolve to exactly what its LF twin does. Base returned
'one\r\n\ntwo' with a stray carriage return embedded; the reader now returns 'one\n\ntwo\n', which is
what the parser gives.

Verified after the fix: across 99 steering documents, CRLF equals LF in every case and
equals the oracle in every case, and no resolved value contains a carriage return.

The write half is NOT a regression -- zero delta

The finding also named the write path. Measured per dialect over 36 CRLF documents each:

  • STEERING_LOADER: nothing left behind, on base or branch (0 and 0).
  • SKILL_UPDATE: the whole block is left behind on BASE and branch equally (36 and 36).
    Its extraction is leading_ws_fence, which does not match a CRLF fence, so
    set_frontmatter_fields finds no frontmatter and PREPENDS a new block ahead of the
    original -- byte-identical output before and after this PR.

So the write-side symptom is a pre-existing CRLF blindness in that dialect's fence, not
something this change caused and not something the prescribed line touches. I have not
folded a fix for it in: making leading_ws_fence CRLF-aware is a separate change with its
own test surface, and this PR is a block-scalar fix. Filed separately rather than expanded
here.

Verification

1640 targeted backend tests pass, plus 189 in the steering/consolidation suites that
exercise this dialect, and 121 frontend. The three standing sweeps are unchanged: reader
vs oracle 29,676 documents / 0 divergences, TypeScript mirror vs reader 5652 / 0, write
symmetry 22,608 / 0 lines moved. flake8, isort, mypy, black baseline, tsc and
npx eslint src/ --max-warnings 604 all clean.

No /ai-review override has been used anywhere on this PR.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Body supplement -- five items the description does not yet cover

Posted as a comment rather than a body edit on purpose: editing the body re-triggers the
GPT lane on this same commit, and that lane is non-deterministic, so it can flip a SHA all
five lanes have already cleared with no code change. Maintainer: this is the material to
fold into the squash message alongside the existing body.
Everything below is on
0fab2b979; the body is accurate about what it does describe.

1. Indentation is counted in SPACES, never tabs

Not in the body at all. YAML indentation is spaces, so under description: | a line of two
spaces then a tab is two columns of indent followed by a TAB OF CONTENT, resolving to
"\t". Judging content with strip() counted the tab as indentation and found no content
line, and in the collection walk that "blank" tab line let a following more-indented line
set a deeper boundary -- so | + <tab> + deep + one silently dropped its
last line
. Both walks now measure in spaces.

Found by the generated matrix on its first run, not by review.

2. A leading blank line sets the block's boundary

The body covers a leading break as CONTENT under chomping; this is a different rule. A
leading all-space line takes part in detecting the block's indent, so a SHALLOWER line
after it ENDS the scalar rather than becoming its first content line:

document yaml before
| + + # note '' ' \n# note\n' -- note consumed
| + # note (no leading blank) '# note\n' same -- correct already

The second row is why this is a floor and not a blanket refusal: with no leading blank the
comment genuinely IS the scalar's first content line. Without the floor, rewriting the
field deleted a line that belonged to the surrounding document.

3. repo_scope is normalised at all three gate call sites

The body's Pattern harvest describes the flag-normalisation sweep and says it "was a latent
bug before this PR rather than one this PR introduced". That is right for the boolean
flags and wrong for repo_scope, which the sweep originally missed.

repo_scope reaches _repo_scope_satisfied from three places, each guarding on the
value's truthiness first. repo_scope: |+ over a blank resolves to a break -- truthy,
blank -- so an unstripped site hands the gate whitespace and it refuses, suppressing a
skill nobody scoped. A PARTIAL sweep was worse than none: with two sites stripped and the
third reading an unstripped row, one document was unscoped at two sites and gated at the
third. All three now normalise, and a source-level test asserts every repo_scope read
carries .strip() so a fourth site fails the test rather than shipping.

One correction to the record while here: an earlier comment of mine claimed a trailing
break would suppress a skill via exact path containment. It would not --
project_scope_satisfied strips its own fragment, pre-existing. Only the whitespace-only
value was ever the defect.

4. A CRLF document resolves exactly like its LF twin

Not in the body. Judging "blank" in spaces (item 1) made a CRLF blank line -- "\r", not
"" -- look like content at indent 0, which ended the scalar. Under STEERING_LOADER,
the dialect whose extraction actually parses a CRLF fence:

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

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.

@bolichen97
bolichen97 merged commit 63a043a into main Sep 2, 2026
69 checks passed
@bolichen97
bolichen97 deleted the fix/skill-frontmatter-yaml-7097 branch September 2, 2026 04:36
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Skill frontmatter reader implements a subset of YAML, forcing the writer to avoid valid constructs

2 participants