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}
…control characters
|
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.34%
==========================================================================
Files ? 223
Lines ? 17912
Branches ? 3643
==========================================================================
Hits ? 10629
Misses ? 6765
Partials ? 518
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.
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