Skip to content

Strip U+0080 instead of crashing change_to_ascii - #1017

Open
arpitjain099 wants to merge 2 commits into
idaholab:developfrom
arpitjain099:fix/ascii-boundary-off-by-one
Open

arpitjain099 wants to merge 2 commits into
idaholab:developfrom
arpitjain099:fix/ascii-boundary-off-by-one

Conversation

@arpitjain099

@arpitjain099 arpitjain099 commented Oct 9, 2026 •

Copy link
Copy Markdown

Pull Request Checklist for MontePy

Description

ASCII ends at 0x7F, but the character by character fallback in strip_characters tests ord(char) > 128, so U+0080 is kept. That branch only runs once the whole line has already failed to encode, and the join is re-encoded with errors="strict", so the kept byte raises UnicodeEncodeError out of the script. A single U+0080 therefore kills the tool whose job is to remove exactly that.

Walking the boundary through main: a pure ASCII file is copied unchanged, U+00E9 (ord 233) is stripped correctly, and U+0080 raises.

The expected value helper in tests/test_scripts.py mirrored the same boundary with <= 128 in three places, so it agreed with the bug and could never catch it. I moved those to <= 127 as well, otherwise the test would push the next person back toward the old comparison.

Fixes #1018


General Checklist

  • I have performed a self-review of my own code.
  • The code follows the standards outlined in the development documentation.
  • I have formatted my code with black version 25 or 26.
  • I have added tests that prove my fix is effective or that my feature works (if applicable).

LLM Disclosure

  1. Are you?

    • A human user
    • A large language model (LLM), including ones acting on behalf of a human

    The account belongs to Arpit Jain, who directs the work and reviews it before it is submitted. This change was prepared by an LLM on his behalf, and this description was written by it, so the second box is the accurate one.

  2. Were any large language models (LLM or "AI") used in to generate any of this code?

  • Yes
    • Model(s) used: Claude Opus 5 (claude-opus-5)
    • Harness used: Claude Code
    • Reasoning effort: not exposed to me as a readable setting, so I would rather say that than guess at a value. Happy to ask for the exact configuration if you need it recorded.
  • No

Each commit also carries a Co-Authored-By: Claude <noreply@anthropic.com> trailer.

Documentation Checklist

  • I have documented all added classes and methods.
  • I have added type hints to all functions as needed.
  • I have marked all changes with the .. versionchanged:: or .. versionadded:: directives.

The first two are vacuous here: no class, method or function is added, the source change is one character in an existing comparison. The third is unticked because no documented API changes behaviour, so there is nothing for a versionchanged directive to attach to. The release note lives in the changelog instead, added as a #Next Version# section per your comment below.

Infrastructure Changes

  • For infrastructure updates, I have updated the developer's guide.

Not an infrastructure change.

Significant features or Behavior changes

  • For significant new features, I have added a section to the getting started guide.

Not a new feature.


First-Time Contributor Checklist

  • If this is your first contribution, add yourself to pyproject.toml if you wish to do so.

Skipping this one.


Additional Notes for Reviewers

Ensure that:

  • This PR fully addresses and resolves the referenced issue(s).
  • The submitted code is consistent with the merge checklist outlined here.
  • The PR covers all relevant aspects according to the development guidelines.
  • 100% coverage of the patch is achieved, or justification for a variance is given.

Left unticked because they read as yours to confirm rather than mine to assert. For the last one, both branches of the changed comparison are exercised: U+0080 takes the replace path and U+00E9 already did, in both the -d and -w variants.

The two new cases fail on develop with UnicodeEncodeError: 'ascii' codec can't encode character '\x80' in position 4 and pass with the change. tests/test_scripts.py goes from 2 to 4 passing, and the full suite is 1250 passing.

One thing worth flagging separately: test_num_collect_clone has a 200ms Hypothesis deadline and tripped once in three full runs for me, measuring 321ms then 88ms on replay. It is unrelated to this change and passes on re-run, but the margin looks thin on a slow machine.


📚 Documentation preview 📚: https://montepy--1017.org.readthedocs.build/en/1017/


📚 Documentation preview 📚: https://montepy--1017.org.readthedocs.build/en/1017/

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@MicahGale

MicahGale commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

This PR is not associated with any issue, please create one first to describe this bug. In addition, please use our PR template.

@arpitjain099

Copy link
Copy Markdown
Author

Both done: opened #1018 with the reproducer and the stack trace, and rewritten the description against the template with the LLM disclosure filled in. black 25.11.0 reports both files unchanged.

Two things I left blank rather than guess at. I did not touch CHANGELOG.rst, since v1.5.0 is already tagged and I did not want to open a release section on your behalf; happy to add a Bugs Fixed line referencing #1018 wherever you want it. And the reasoning effort field is not something I can read as a setting, so I said that instead of putting a number in.

@MicahGale

MicahGale commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Two things I left blank rather than guess at. I did not touch CHANGELOG.rst, since v1.5.0 is already tagged and I did not want to open a release section on your behalf; happy to add a Bugs Fixed line referencing #1018 wherever you want it. And the reasoning effort field is not something I can read as a setting, so I said that instead of putting a number in.

This does need to be included in the changelog. If you want you could add this under 1.5.0 and then I could update the sections for the next release for you and push to your branch.

You could also add the #Next Version# section header for this. For now you could add it under 1.5 Releases, but this will probably ultimately ship with 1.6.0 instead of a 1.5.1 release.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099

Copy link
Copy Markdown
Author

Added, as a #Next Version# section under 1.5 Releases so it is not filed against a tag that has already shipped. Move or relabel it however suits the 1.6.0 cut, and feel free to push to the branch.

That also clears changelog-test, which watches doc/source/changelog.rst. I had been looking at the root CHANGELOG.rst and missed that it is a symlink to that file, which is why I thought the changelog was optional here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

change_to_ascii crashes on U+0080 instead of stripping it

2 participants