Let step names keep their diacritics and their script - #673
Open
elias-ba wants to merge 4 commits into
Open
Conversation
Apollo folded every name to ASCII, so a Spanish name lost its accents and a Japanese or Russian one became the empty string. Two steps in different scripts then collapsed onto each other, and a lookup for one matched the first of them. Names now go through name_rules, which normalises to NFC, counts graphemes the way Elixir does, and refuses only control characters. It is behind APOLLO_UNICODE_STEP_NAMES and defaults to off, so nothing changes until Lightning ships the matching rule. Keys and names are rewritten together rather than separately, which is what left edges pointing at nothing before, and the YAML dump keeps unicode instead of escaping it. Split out of #660 so the leak-hardening question can be settled on its own.
2 tasks
Review found leak-hardening residue in this branch. A comment about swap tokens had ended up above _reference_key, where it read as that function's doc. _section was left with no caller once its tests went to the other branch. unicodedata was imported for a function that no longer exists. Three test fixtures and five section banners had nothing left pointing at them.
The rebuild took main's copy of test_yaml_utils.py, so the tests covering non-Latin name lookup, page parsing and fold resolution never came across. Putting them back caught a second thing the rebuild had missed: get_page_view was still main's, which reads only the third path segment, so a step whose name contains a slash lost its focus entirely.
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
1 participant
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.
Short Description
Apollo folded every job and step name to ASCII, so a Spanish name lost its accents and a Japanese or Russian one became the empty string. This lets names keep their diacritics and their script.
Part of #446
Implementation Details
Two names in different scripts both folded to nothing, so they collided, and a lookup for one matched the first of them. Names now go through
name_rules, which normalises to NFC, counts graphemes the way Elixir does, and refuses only control characters.It sits behind
APOLLO_UNICODE_STEP_NAMESand defaults to off. With the flag off the new rule is ASCII like the old one, but it is not byte-for-byte what was there before: a newline or tab inside a name becomes a space rather than being kept, names are trimmed and capped at 100 graphemes, andstraßefolds tostrasserather thanstrae. Job and trigger keys are now sanitised too, which they never were. Each of those moves in the safe direction, but the off path is not a no-op and I would rather say so than have someone find it.Two other things came with it. Keys and names are now rewritten together rather than separately, which is what used to leave edges pointing at nothing. And the YAML dump keeps unicode rather than escaping it.
This is split out of #660 so the question about job code in Sentry can be settled on its own. Nothing here depends on how that goes.
AI Usage
You can read more details in our
Responsible AI Policy