fix: require whole-word skip and a non-empty reason in example annotations - #767
Open
Ectsang wants to merge 2 commits into
Open
fix: require whole-word skip and a non-empty reason in example annotations#767Ectsang wants to merge 2 commits into
Ectsang wants to merge 2 commits into
Conversation
…tions parse_annotation() treated any text beginning with "skip" as a skip, so skiped, skip_the_check and skipping validation for now each disabled validation for that block silently. _KNOWN_ATTRS catches the same class of typo on the validating path; the skipping path had no guard. reason was also optional in the parse, so a bare skip returned an empty reason and the skip was not auditable. Match skip as a whole word and return the existing _error when reason is missing or empty. process_block already handles _error ahead of the skip branch, so nothing else changes. All 51 skip annotations currently in the docs still parse as skips.
Two CI checks failed on the whole-word skip guard: - spellcheck flagged `skiped`, a deliberate typo used as test input. Appended it to the existing `# cspell:ignore` directive in both files rather than adding a misspelling to .cspell/custom-words.txt, which would silence genuine typos repo-wide. - PYTHON_RUFF_FORMAT wanted the typo tuple wrapped its own way. No behavior change. Contract tests unchanged: 47 pass, and the 2 failures (`ucp-schema` binary absent locally) are identical with and without this commit.
damaz91
requested review from
carolinerg1 and
damaz91
and removed request for
DanielFalconGuedes and
jingyli
August 26, 2026 17:10
damaz91
approved these changes
Aug 27, 2026
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.
parse_annotation()treats any annotation text beginning withskipas a skip:So
skiped,skip_the_checkandskipping validation for noweach disable validation for that block, silently._KNOWN_ATTRScatches the same class of typo on the validating path (shema=,directon=), but the skipping path has no guard. A typo that accidentally validates is loud; one that accidentally skips is invisible.The
reasonfield looks like it's there so every skip stays auditable, but it's optional in the parse — a bareskipreturns{"skip": True, "reason": ""}.Nothing is broken today. All 51 skip annotations in the docs use the documented
skip reason="..."form. This is a guard against a future silent loss of coverage, not a live bug.The fix matches
skipas a whole word and returns the existing_errorwhenreasonis missing or empty.process_blockalready handles_errorahead of the skip branch, so nothing else changes.Tested: all 51 live skip annotations still parse as skips.
scripts/test_validate_examples.pygoes from 42 to 47 passing, with the same two pre-existing failures (ucp-schemanot on PATH).