Repository navigation
Fix review findings in the ambiguity reporting feature - #1462
Merged
frankslin merged 4 commits intoJul 23, 2026
Merged
Conversation
Address the code-review findings on the merged --ambiguities feature (BYVoid#1461) without changing the record schema or any output downstream consumers already depend on. Detailed Changes: - **Correctness**: - Route the --ambiguities branch of ConvertFileStreams through the shared fclose epilogue instead of early-returning; the early return leaked both streams and broke --in-place, which replaces the output file after this function and requires it to be closed. Covered by a new in-place CLI test. - Apply PrefixMatch's group policy semantics in the walk via a recursive helper instead of calling the virtual Dict::MatchPrefix on groups: a DictGroup constructed directly with the Union policy reports Union (honored by Convert()'s PrefixMatch tables) while its inherited MatchPrefix still short-circuits, so the walk could emit different output than Convert() for directly constructed groups. - Mirror SingleStageConverter::Convert's no-segmentation branch by walking the whole text as one string_view, preserving embedded NUL bytes that the Segments/strlen round-trip silently truncated. - Propagate AnnotatedConversion::analyzed into AmbiguityStream::Chunk so stream consumers can distinguish "no ambiguities" from "converter not analyzable". - Detect mid-stream read errors (ferror) in ConvertAmbiguitiesStream and fail without emitting the end record, which is the stream's integrity signal and previously marked truncated streams complete. - **Streaming windowing**: - Extract the flush-window computation shared verbatim by ConverterStream and AmbiguityStream into the private StreamWindow.hpp helper so a windowing fix reaches both wrappers; neither installed header changes. - **CLI measurement**: - Count the JSONL bytes actually written (and the end record) in measurement.outputBytes, consistent with every other output mode, and time the end-record write; the end record's own output_bytes field keeps its converted-text semantics. - **npm CLI parity**: - Reject --ambiguities with the same native-CLI guidance as --inspect/--segmentation, list it in the help text, and cover both in node/test.js, per the documented-and-tested rule for CLI behavior differences. - **Cleanup**: - Replace Compose's unreachable trailing-run folding with asserts that both alignment sides exhaust together. - Add unit tests for the Union-policy group, NUL preservation, and the streamed analyzed flag.
Plain conversion is byte-transparent and --inspect is human-oriented, but the ambiguity record stream is a machine-readable contract, so emitting broken JSON is worse than failing. Length-based walking tolerates a multi-byte lead byte followed by invalid continuation bytes, and without validation such input flowed verbatim into the records and broke every strict JSON consumer. Enable rapidjson's kWriteValidateEncodingFlag for the def/lit/amb writers and abort the stream loudly on failure -- no end record, error message, non-zero exit -- consistent with the end record's role as the stream's integrity signal. Covered by a CLI test and documented in README along with the missing-end-record semantics.
The two new throw paths were verified safe with --in-place (ConvertFile
removes the temp file and rethrows before any replacement), and
UnionDictGroup::MatchPrefix was verified to do no cross-child value
merging, so the recursive matcher's semantics and NumValues counts are
exact -- both guarantees are now pinned by tests instead of resting on
inspection.
Detailed Changes:
- **Compose zero-width trailing runs**:
- Restore the a-side trailing fold: an empty-value dictionary entry
(constructible by library users) matching at the end of a segment
leaves a zero-width run on the middle coordinate, which the previous
round wrongly removed as unreachable; the b-side cannot trail (every
run consumes at least one intermediate byte) and keeps its assert.
Covered by a regression test with an empty-value entry.
- **Matcher semantics**:
- Document the verified equivalence of the recursive matcher with both
DictGroup::MatchPrefix (first child wins) and
UnionDictGroup::MatchPrefix (strict-> longest, earlier child wins
ties), and add a same-length-tie test against Convert().
- **CLI tests**:
- Add an --ambiguities --in-place test with invalid UTF-8 asserting a
non-zero exit and an intact original file.
- Pin the invalid-UTF-8 failure to the record writer by asserting the
same input converts fine in plain mode; use RunCommand consistently;
type-check amb record fields; replace dictionary-content-pinned
counts with invariant and contains-style assertions so unrelated
dictionary updates cannot break CI.
- **Python CLI parity**:
- Reject the three native-only modes with the same guidance message as
the npm CLI instead of argparse's generic error, with a pytest.
- **Polish**:
- Move the shared keep-tail default into StreamWindow.hpp, the
windowing logic's home; take string_view in FlushableByteCount; drop
Converter.cpp's stale UTF8Util include; correct the encoding-failure
message to name converted output (input or dictionary) as the
origin; note why fputs is NUL-safe in the record writer; document
--ambiguities in the diagnostic-modes list and the mutual-exclusion
rule in README.
Two review nits promoted from inspection-backed to test-backed: the
ambiguity flag must reflect NumValues() of exactly the entry the union
picked rather than values merged across children, and a failed
--ambiguities --in-place run must leave no temporary file behind.
Detailed Changes:
- **UnionNumValuesComesFromWinningChild**:
- Construct the UnionDictGroup subclass directly (covering the
override path alongside the plain Union-policy DictGroup tests) with
two children sharing a key, multi-value versus single-value; assert
the span is flagged when the multi-value child wins the tie and NOT
flagged when the single-value child does, in both child orders.
- **AmbiguitiesInPlaceInvalidUtf8KeepsOriginal**:
- Run in a dedicated directory and assert exactly one entry remains
after the failed run, so temp-file cleanup is verified alongside the
intact original.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Address the code-review findings on the merged --ambiguities feature
(#1461) without changing the record schema or any output downstream
consumers already depend on.
Detailed Changes:
shared fclose epilogue instead of early-returning; the early return
leaked both streams and broke --in-place, which replaces the output
file after this function and requires it to be closed. Covered by a
new in-place CLI test.
recursive helper instead of calling the virtual Dict::MatchPrefix on
groups: a DictGroup constructed directly with the Union policy
reports Union (honored by Convert()'s PrefixMatch tables) while its
inherited MatchPrefix still short-circuits, so the walk could emit
different output than Convert() for directly constructed groups.
walking the whole text as one string_view, preserving embedded NUL
bytes that the Segments/strlen round-trip silently truncated.
so stream consumers can distinguish "no ambiguities" from
"converter not analyzable".
and fail without emitting the end record, which is the stream's
integrity signal and previously marked truncated streams complete.
ConverterStream and AmbiguityStream into the private
StreamWindow.hpp helper so a windowing fix reaches both wrappers;
neither installed header changes.
measurement.outputBytes, consistent with every other output mode,
and time the end-record write; the end record's own output_bytes
field keeps its converted-text semantics.
--inspect/--segmentation, list it in the help text, and cover both
in node/test.js, per the documented-and-tested rule for CLI
behavior differences.
that both alignment sides exhaust together.
the streamed analyzed flag.