yaml: clear remaining 3 non-cyclic test.todo entries (BOM strip, 2-anchor e-node key, flow cmi gap) - #31569
Merged
Merged
Claude / Claude Code Review
completed
May 29, 2026 in 39m 19s
Code review found 3 potential issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/parsers/yaml.rs:1666-1672 |
string_is_number doc comment now stale after is_core_schema_number gate |
| 🟡 Nit | test/js/bun/yaml/yaml.test.ts:1217 |
Stale comment on un-todo'd two-anchor e-node test |
| 🟣 Pre-existing | src/parsers/yaml.rs:2447-2453 |
BOM strip is stream-start only, not per-document ([211]) |
Annotations
Check warning on line 1672 in src/parsers/yaml.rs
claude / Claude Code Review
string_is_number doc comment now stale after is_core_schema_number gate
Nit: the new `is_core_schema_number` gate changes `try_resolve_number`'s contract, but its hand-written mirror `string_is_number` (src/runtime/api/YAMLObject.rs:903-912) still documents the old behavior — line 909-910 says `"1+5"` round-trips to `1`, which this PR's own test now asserts is false (`["1+1", "1+1"]` resolves as string). No functional bug (`string_is_number` is now over-conservative, which is round-trip-safe); just worth dropping/correcting the stale `"1+5" → 1` example and embedded
Check warning on line 1217 in test/js/bun/yaml/yaml.test.ts
claude / Claude Code Review
Stale comment on un-todo'd two-anchor e-node test
🟡 Nit: the test was un-todo'd and the title dropped "— pre-existing over-reject", but the comment body at 1219-1222 still describes the old bug — "the e-node arm reaches it. Pre-existing on main." After this PR the e-node arm clears `has_mapping_anchor` (yaml.rs:4345), so the guard no longer over-rejects; drop the last sentence-and-a-half (keep just the [200]/[193] rationale on 1218-1219), or update to reflect the fix.
Check notice on line 2453 in src/parsers/yaml.rs
claude / Claude Code Review
BOM strip is stream-start only, not per-document ([211])
🟣 Pre-existing, non-blocking: this strips the BOM at stream-start only, but [211] `l-yaml-stream` admits `l-document-prefix` (with `c-byte-order-mark?`) before *each* document — and lists `c-byte-order-mark` directly in its trailing alternation — so per §5.2 "byte order marks may appear at the start of any document". `a: 1\n...\n---\nb: 2` (concatenated UTF-8-with-BOM files) won't have the second-document BOM stripped; the U+FEFF reaches `scan_plain_scalar` and `---` parses as content instead
Loading