fix(schema-compiler): quote scaffolded YAML values that would not round-trip - #11495
Conversation
|
Claude finished @igorlukanin's task in 5m 46s —— View job Review: solid fix, one gap on the adjacent path — 1 medium-high, 1 medium, 3 lowThe diagnosis is right and the direction is right: replacing a character blocklist with a round-trip predicate is the correct shape for this, the negative cases ( Findings and review notes — 5 inline comments
Verified while reviewing
What I could not verify: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @igorlukanin's task in 3m 11s —— View job All five prior findings addressed;
|
| # | Prior finding | Status in 74407ec |
|---|---|---|
| 1 | MemberReference bypassed needsQuotes, so drill_members: [2024] stopped denoting name: "2024" |
Fixed — routed through escapedValue(value.member, flow) (YamlSchemaFormatter.ts:78), with a parametrised test that compares the parsed drill list to the parsed dimension names tagged by typeof, which is the only way the assertion can tell 2024 from "2024". That detail is the test doing real work. |
| 2 | Newline detected but not escaped; \r not detected mid-string |
Fixed, and better than suggested — JSON.stringify replaces the hand-rolled replace chain, and detection widened to [\x00-\x1f\x7f-\x9f] plus surrogates. |
| 3 | Empty alternative in YAML_TYPE_SHAPED was accidental |
Fixed — comment on line 18. |
| 4 | Comment over-claimed (sexagesimal, first-position set) | Fixed — both claims now scoped correctly, and the sexagesimal note is right: js-yaml 4's float resolver dropped the :-separated form. The YAML.load-derived alternative was declined; fine either way, and the hand-rolled version keeps the formatter free of a parse dependency. |
| 5 | yes asserted on text only |
Fixed — parse assertion added alongside. |
On JSON.stringify as the escaper
This is the right call and worth stating explicitly, since it looks like a shortcut and isn't. YAML's double-quoted style is a strict superset of JSON's: every escape JSON.stringify can emit (\" \\ \b \f \n \r \t \uXXXX) is a valid YAML escape, and ES2019 well-formed stringify escapes lone surrogates rather than emitting them raw — which is what makes the [\uD800-\uDFFF] branch actually deliver instead of just detecting.
Verified while reviewing
- No unescaped dynamic path left that I could find. Object keys are emitted raw in
render(), but every dynamic name reaches the output as a value: dimension and measure names via theentries.map(([ok]) => ({ name: ok, … }))conversion, andjoinsthe same way — so a cube named2024joined from another file renders- name: "2024"there too, consistent withrenderFile. The only raw keys are literals (cubes,dimensions,sql,type). - Cube-name quoting is self-consistent with
{}interpolation.cubeReferenceproduces{2024}.id, which the pre-existing{}rule already quotes, so the join SQL and the cube name agree on the string. - A pre-existing bug fell out of this too:
sqlForMemberon a column needing driver quoting produced{CUBE}."2024", which the old{}-only rule quoted without escaping —"{CUBE}."2024"", a parse error. Escaping fixes that path as a side effect. - Snapshots: no
__snapshots__changes in the diff, and no ordinary emitted value (public.orders,COUNT(*),Order Status) trips a new branch — consistent with the claim that all 18 are unchanged. - No security or performance surface: seven anchored regexes per scalar on a one-shot scaffolding path.
Nit (1 inline comment)
U+FFFE/U+FFFF are in js-yaml's non-printable set alongside the C1 range this PR now handles, so they're still emitted bare and fail the whole stream. One character to close; details inline. I also suggested pinning down why the C1 half works via quoting rather than escaping, since that's the kind of subtlety a later "simplification" removes.
What I could not verify
node_modules is absent in this checkout and dependency installation isn't available to me, so I did not run the suite, tsc, or eslint. The js-yaml behaviour above (the PATTERN_NON_PRINTABLE set, the JSON-character check in captureSegment, the 4.x int/float resolvers) is read from the 4.x loader source rather than observed in a run. The tests assert parsed documents, so CI is the real check here — in particular dimensionOf('2026-08-06').title depends on inflection.titleize leaving the dashes alone, which I couldn't confirm.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## igor/core-725-scaffolding-drill-members #11495 +/- ##
===========================================================================
+ Coverage 59.45% 59.46% +0.01%
===========================================================================
Files 223 223
Lines 17961 17963 +2
Branches 3643 3643
===========================================================================
+ Hits 10678 10682 +4
+ Misses 6766 6764 -2
Partials 517 517
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @igorlukanin's task in 1m 18s —— View job
|
| # | Prior finding | Status |
|---|---|---|
| 1 | MemberReference bypassed needsQuotes |
Fixed in 74407ec (YamlSchemaFormatter.ts:81) |
| 2 | Newline detected, not escaped; \r undetected |
Fixed in 74407ec via JSON.stringify |
| 3 | Empty alternative accidental | Fixed (:19) |
| 4 | Comment over-claimed | Fixed (:13, :181) |
| 5 | yes asserted on text only |
Fixed (:633) |
| 6 | U+FFFE/U+FFFF uncovered | Fixed here (:209) |
Verified while reviewing
- The detection set now matches js-yaml 4's
PATTERN_NON_PRINTABLEexactly, and is a strict superset.\x85(NEL) is excluded from js-yaml's set but included by\x7f-\x9fhere — over-quoting, which is the safe direction, and harmless sinceis_EOLin js-yaml 4 is0x0A/0x0Donly, so a quoted\x85stays literal. Astral noncharacters (U+1FFFE etc.) aren't in js-yaml's set either, so their absence here isn't a gap. - No
uflag on/[\uD800-\uDFFF]/is correct, not an oversight — without it the class matches individual code units, which is what unpaired-surrogate detection needs. - U+2028/U+2029 are safe unhandled.
JSON.stringifydoesn't escape them, but js-yaml 4 follows YAML 1.2 where they aren't line breaks, so they round-trip bare. (Would be a bug under a 1.1 loader — noting it only because the file's other decisions are pinned to js-yaml 4 as well.) - No unescaped dynamic path remains. Every dynamic name reaches the output as a value: members and joins via the
entries.map(([ok]) => ({ name: ok, … }))conversion, the cube name via the explicitescapedValueinrenderFile:56. The only raw keys inrender()are literals. - The drill-member test does real work. Tagging each parsed value with
typeofbefore comparing as sets is the only formulation that distinguishes2024from"2024"— a plain sorted comparison would pass on the broken output.
Nits, all optional and none blocking
needsQuotestouches no instance state; it could sit at module level next toYAML_TYPE_SHAPED.- The
��literals are invisible in the source; a�-style spelling would read closer to the comment above them. (The current form is already the escaped spelling — noting only that the uppercase-hex convention used for\uD800two characters later differs.)
What I could not verify
node_modules is absent in this checkout and dependency installation isn't available to me, so I did not run the suite, tsc, or eslint. The js-yaml behaviour cited above is read from the 4.x loader source, not observed in a run — CI is the real check, and the tests assert parsed documents, which is the right thing for it to check.
845ad08 to
aca6b3d
Compare
e92c9bc to
a387263
Compare
|
Claude finished @igorlukanin's task in 2m 7s —— View job Rebase only — content identical to the state I already reviewed; no new findingsReview of the synchronize push (
|
f2c2e04 |
quote scaffolded YAML values that would not round-trip |
be95f27 |
quote member references and cube names, escape control characters |
a387263 |
quote the U+FFFE/U+FFFF noncharacters too |
Same three subjects, same order as f2c2e04/74407ec/e92c9bc before the rebase, and the resulting file matches the state described in my last review character for character — MemberReference routed through escapedValue(value.member, flow) (:81), JSON.stringify as the escaper (:171), the deliberate-empty-alternative comment (:19), the noncharacters in the detection class (:209), and the split explanation at :200-207. The diff is +87/-6 and +169/-0, unchanged. (I can't diff against the pre-rebase SHAs directly — they aren't in this checkout — so this is an inspection of the resulting tree, not a machine comparison.)
Re-verified against the new base
The base version of the formatter I read this time is a clean subset: renderFile still interpolates ${cube} raw, MemberReference still returns value.member, and escapedValue is still the two-regex /[{}"]/ + flow /[{}",[\]]/ form with a "-only replace. So the rebase introduced no overlap or partial duplication with the flow-sequence commit the base now carries — this PR strictly extends it, and the flow threading it relies on is the base's, untouched.
Re-audited the predicate (no execution)
Walked the ordinary emitted values through needsQuotes by hand again against the current source; none trip a new branch, consistent with the unchanged snapshots:
Order Status,COUNT(*),public.orders,users.id,default,id— no indicator in first position, no:/#, no type shape.1.2.3— the decimal alternative anchors at$, so1.2+.3doesn't match. Stays a string, correctly unquoted.12:30:00— every colon is followed by a digit, so/:(?:\s|$)/misses it and it stays bare; correct under js-yaml 4, which resolves a timestamp only with a date part.2024-13-45and08are over-quoted (invalid month, leading-zero decimal). Safe direction, no behaviour change.- Astral characters (emoji, CJK ext) match
[\uD800-\uDFFF]as a well-formed pair and get quoted — already called out in the comment at:206as costing only a pair of quotes.
What I could not verify
node_modules is absent in this checkout and dependency installation isn't available to me, so I did not run the suite, tsc, or eslint — CI is the real check. The one assertion I still can't confirm statically is dimensionOf('2026-08-06').title depending on inflection.titleize leaving the dashes alone.
Nothing blocking from my side; the same two optional nits from last time stand (needsQuotes could sit at module level; the �� literals are invisible in source next to the \uD800 spelling two characters later).
1 participant
Summary
escapedValuedecided quoting from/[{}"]/(plus,[]inside a flow sequence), which leaves several characters that change how YAML parses a scalar emitted bare. This is reachable from ordinary warehouse data, not just from a cast: a dimension'stitleisinflection.titleize(column.name), so a column namedrevenue: usdrenderstitle: Revenue: Usdand the generated model file fails to parse at all, whileprice # netsilently truncates the title toPrice.needsQuotespredicate covering the cases that demonstrably do not round-trip: a leading indicator character,:or a trailing colon,#, surrounding whitespace, a tab or newline, and a scalar YAML would resolve to null / a boolean / a number / aDaterather than a string. The existing{}"rule and the flow-sequence,[]rule are kept.C:\pathused to be emitted as"C:\path", which is an invalid escape and a parse error;tab\therebecame a real tab.a#b,a:b,COUNT(*),users.id,Order Statusandyesstay unquoted, so ordinary member names, titles and SQL expressions are untouched. All 18 existing snapshots are unchanged.Stacked on the
drill_membersbranch, which introduced the flow-context flag this extends; it targets that branch rather thanmaster.Test plan
scaffolding-template.test.ts, asserting the parsed document rather than the rendered text — the defect is that the output parses as something other than the string that went in, which a text assertion cannot seetsc --noEmitandeslintclean on both changed files