Repository navigation
Fix #47 review findings: custom XML decoding, decision record, dashboard denominators - #48
Merged
Merged
Conversation
_canonical_xml decoded every part as UTF-8 before parsing, so a valid UTF-16 custom XML item fell into the raw-byte hash path and an equivalent UTF-8 to UTF-16 rewrite was scored as a custom_xml change. Pass the bytes to the parser so the BOM and XML declaration control decoding. Rescoring the stored 2026-10-02 mutation outputs with the fixed scorer leaves every engine's verdict and feature preservation unchanged.
Add DEC-027: JSON-model external helpers (sheetjs, exceljs, libreoffice) are registered and scored like any other adapter so the comparison covers non-Python libraries, and mutation preservation compares parsed content so equivalent rewrites score the same as in-place edits. Update architecture.md, which still said external helpers are excluded from get_all_adapters().
The overview's Features Scored card, the score-3 total, the WolfXL hero fraction and the radar's feature coverage counted features whose read and write scores are null for every adapter, so the unscored pivot_tables row inflated the 2026-10-02 denominators from 21 to 22. Filter those rows with one shared helper before computing the aggregates.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The three referenced findings are correctly addressed with aligned regression coverage and documentation.
Review effort: Balanced
Findings: None
What changed in this PR
Addresses PR #47 review findings across mutation scoring, architecture records, and dashboard denominators.
Changes:
- Corrects encoding-aware custom XML canonicalization.
- Excludes globally unscored features from dashboard metrics.
- Documents external-helper adapters and content-model scoring.
| File | Description |
|---|---|
src/excelbench/harness/mutation.py |
Passes raw XML bytes to canonicalization. |
src/excelbench/results/html_dashboard.py |
Filters globally unscored features from aggregates. |
tests/test_mutation_suite.py |
Adds UTF-16 XML regression coverage. |
tests/test_results_html_dashboard.py |
Tests corrected dashboard denominators. |
decisions.md |
Adds DEC-027 and supersession notes. |
architecture.md |
Updates adapter and mutation-scoring architecture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Follow-up to #47, addressing its three review threads.
_canonical_xmlnow passes raw bytes toET.canonicalize, so the BOM and XML declaration select the decoding. An equivalent UTF-8 to UTF-16 rewrite of custom XML no longer counts as acustom_xmlchange. Regression test added. Rescoring every stored output inresults-2026-10-02/mutation/with the fixed scorer gives the same verdict and feature-preservation count for every engine, so the published results are unchanged.get_all_adapters()and the move to content-model preservation scoring.architecture.mdnow matches the code._scored_resultshelper drops features that no adapter scored (pivot_tablesin the 2026-10-02 snapshot) from Features Scored, the score-3 total, radar coverage and the WolfXL hero. WolfXL now renders 20/21 for that snapshot. Test added.Gate:
ruff check,mypy src,pytestwithOPENPYXL_LXML=False(the Linux CI writer path).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.