Skip to content

repo_shape.py: a flow-mapping entry opening ? is refused as an explicit key, and an empty flow-mapping value reads as null, so {? a: b} no longer reads as the key ? a and {a: } no longer dies as not a flow collection: '' (#252) - #257

Merged
brettheap merged 1 commit into
mainfrom
fix/yaml-flow-mapping-keys
Oct 11, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openreposhape-5 (openRepoShape-5)
Claim: #252 (comment)

What

Two edges of the reader's flow-mapping entry, found by the adversarial review of #213 (its F3 and F4) and
filed as #252. Both made a flow mapping read differently from its block twin.

  1. An explicit key. The block reader refuses ? by name (_refuse_unsupported: explicit keys are not supported), and the reader's header lists explicit keys as REFUSED, because the meaning would have to be
    invented. That check runs on block lines only, so k: {? a: b} read as the literal key ? a. A flow
    mapping entry opening ? is now refused with the same phrase. A flow collection has no line to name, so it
    names the entry: explicit keys are not supported: '? a: b'.
  2. An empty value. _flow_value asked text[:1] in "[{", which is True for the empty string, so {a: },
    {a:} and {a: , b: 1} ran _flow('') and were refused as not a flow collection: '', a text nobody
    wrote. The question is whether the text OPENS a flow collection (startswith(("[", "{"))), which an empty
    one does not. It reaches _scalar(''), the null the block a: reads. A quoted empty ({a: ""}) stays the
    empty string.

Functions touched, and nothing else: _split_key_flow (the refusal, before return _split_key(text)),
_flow_value (the test), and one appended paragraph in the SUPPORTED/REFUSED header comment. _split_key,
_flow, _split_flow, _scalar, _strip_comment and _refuse_unsupported are untouched.

Why

"An invented meaning in a validator is a wrong answer with a confident tone." Row 1 was a wrong answer with no
refusal; rows 3 to 5 were refusals of valid YAML naming an empty string a person never wrote. Latent today: no
writer in the repository emits a flow mapping with a key or a value. It shows on a file a person edits, such as
naming: {form: } left half filled in project.yaml.

Evidence

  • New tests/test_yaml_flow_mappings.py, 46 tests: 34 fail and 12 pass on 150c41d (the 12 are controls),
    46 pass on head. Pins the issue's rows, the places each can stand (after another entry, nested in a
    flow mapping and in a flow sequence, as a block-sequence item, in a file read through load_yaml), the block
    twin equal wherever the block reader reads one, the spelled nulls (~, null, Null, NULL) equal to the
    empty value, quoted empties staying '', and what must NOT be refused ("? a", '? a', ?a, a?,
    b?).
  • Mutation probes, each reverts or bends one hunk: nine behavioural mutants each fail at least one new test
    (20, 14, 1, 3, 5, 20, 13, 1, 2); the comment hunk and docstrings are behaviour-free and survive.
  • Differential fuzz, 100,000 documents with PyYAML 6.0.3 as the oracle (the oracle only, never imported by
    shipped code): flow == expected == block twin == PyYAML on every one on head; on 150c41d 64,387 of them read
    wrong. 100,000 variants with one entry made an explicit key: all refused on head, none on 150c41d. Re-run of
    scripts/repo_shape.py _split_flow ignores a backslash inside a double-quoted flow item, so pins: ["x\",y"] reads back as two items x\ and y" where the same text as a block sequence item reads as the one item x",y #213's flow/block fuzz (100,000 plus 60,000 with the code-point escapes): 0 mismatches.
  • Reach: all 14 tracked YAML files read identically on 150c41d and on head. The writers that emit a flow
    collection (pins, allow_empty_legs, also_matches, referent_chain, members: [], paths: []) emit
    sequences or [], never a mapping.
  • Targeted, on head: test_yaml_flow_mappings, test_yaml_flow_quotes, test_load_yaml_refusals,
    test_adopt_plan_escapes, test_manifest_validator, test_pin_validator, test_neutral_pin_validator,
    test_repo_hygiene, test_cli_reference, test_adopt_plan: 321 passed, 1 skipped (SHAPE_MEDXEHR_CLONE
    unset). test_shape_doctor.py: 181 passed.
  • Not run here: the full suite, and the macOS and Windows jobs; the new tests pass bytes to git nowhere and
    write files as bytes, so LF on every platform.

Composition

git merge-tree --write-tree HEAD <ref>: clean against origin/main 39ac59c, and clean against
fix/yaml-block-key-nul (9be6287), where the combined tree also passes both writers' new tests together (125).
fix/yaml-quote-boundaries and fix/scaffold-referent-chain-links had no commit when this was written, so
nothing was merged against them.

Left out

Closes #252

🤖 Generated with Claude Code

Summary by Sourcery

Align flow-mapping edge cases with block-mapping behavior by refusing unsupported explicit keys and reading empty values as null.

Bug Fixes:

  • Make flow-mapping entries beginning with ? refuse unsupported explicit keys instead of interpreting them as literal keys.
  • Treat empty flow-mapping values as null, matching block mappings and spelled null values.

Enhancements:

  • Document flow-mapping behavior parity with block mappings and preserve handling of quoted empty strings and non-explicit question-mark keys.

Tests:

  • Add comprehensive coverage for explicit-key refusals, empty values, nested flow mappings, block-sequence contexts, file loading, and regression cases.

…licit key, as the block reader refuses it, and an empty flow-mapping value reads as null, as a block `a:` does, so `k: {? a: b}` no longer reads as the literal key `? a` and `k: {a: }` and `k: {a:}` no longer die as `not a flow collection: ''` (#252)

`_split_key_flow` raises `explicit keys are not supported` for an entry that
opens `? `, the phrase `_refuse_unsupported` uses on a block line; a flow
collection has no line to name, so it names the entry. `_flow_value` asks
whether the text OPENS a flow collection (`startswith(("[", "{"))`), which an
empty string does not, where `text[:1] in "[{"` is True for it. `_scalar('')`
is the null the block twin reads.

No writer in the repository emits either form.

Lane: openreposhape-5 (openRepoShape-5)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-5, 2026-10-11T22:24Z) @codex review

@sourcery-ai

sourcery-ai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 1 day and 1 hour by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Reviewer's Guide

The PR fixes two flow-mapping edge cases in repo_shape.py: explicit-key entries beginning with ? are now refused consistently with block mappings, and genuinely empty values are parsed as null rather than being misclassified as flow collections. A new focused test module validates parity across nesting, sequence and file-loading contexts while protecting related YAML behaviors.

File-Level Changes

Change Details Files
Align flow-mapping entry handling with block-mapping behavior for unsupported explicit keys and empty values.
  • Reject entries beginning with ? using the existing explicit-key refusal wording, including the raw flow entry in the error.
  • Treat only text that actually starts with [ or { as a nested flow collection; pass empty values to scalar parsing so they become null.
  • Document the flow-mapping/block-mapping parity contract and the two edge cases.
scripts/repo_shape.py
Add comprehensive regression coverage for flow-mapping parity, refusals, null values, and preserved distinctions.
  • Cover explicit-key refusals across nesting and placement contexts, while confirming quoted and non-indicator question marks remain valid.
  • Verify empty flow values match block nulls and spelled null forms, while quoted empty values remain empty strings.
  • Exercise nested collections, file loading, refusal propagation, and unchanged malformed-collection behavior.
tests/test_yaml_flow_mappings.py

Assessment against linked issues

Issue Objective Addressed Explanation
#252 Refuse explicit flow-mapping keys beginning with ? with the same unsupported-explicit-key behavior as the block reader, rather than interpreting ? a as a literal key. ✅
#252 Interpret empty flow-mapping values such as {a: }, {a:}, and {a: , b: 1} as YAML null values, matching block mappings, while preserving quoted empty strings and nested flow-collection handling. ✅
#252 Add regression coverage documenting and testing the explicit-key refusal and empty-value behavior in relevant nesting, file-loading, and compatibility cases. ✅

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-11T22:33:32.791793Z 2438190 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@sonarqubecloud

Copy link
Copy Markdown

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 2438190b40

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@brettheap
brettheap merged commit 4b2add8 into main Oct 11, 2026
5 checks passed
@brettheap
brettheap deleted the fix/yaml-flow-mapping-keys branch October 11, 2026 22:54
brettheap added a commit that referenced this pull request Oct 12, 2026
…, #252 via #257, #195 via #226, #210 via #219, #211 via #220, #248 via #265, #212 via #223) into fix/adopt-record-submodule-placement

Brings in 06b5204 (#214), 60d08a1 (#217), 2727b08 (#209), 4b2add8 (#252),
aaa40eb (#195), d89fffa (#210), eb6df75 (#211), c3f4987 (#248) and
5751460 (#212) on top of 39ac59c. No rebase. 5751460 landed while the
merge of c3f4987 was being checked, and it changes adopt-project.py,
scripts/shape_materialize.py and tests/test_adopt_submodules.py, as this
branch does, so this one merge takes it too.

No conflicts. Both sides change adopt-project.py, scripts/repo_shape.py,
scripts/shape_materialize.py, shape-doctor.py, tests/test_adopt_submodules.py
and tests/test_shape_doctor.py. Every top-level definition only one side
changed is that side's text in the merge, byte for byte. The one thing
both changed is shape_materialize.py's imports, and both are kept: main's
`codecs` and this branch's `path_spelling_fault` and `PATH_*` codes.
Checked:

- adopt-project.py: main calls `escape_what_stdout_cannot_encode()` first
  in `main` (#214). It makes a `.gitmodules` git's submodule reader cannot
  read a finding where the tip holds no submodule and the history does, or
  where `ignore`, `update` or `branch` has no value (#195). And every path
  a finding names is now `_spelled` (#212). This branch's
  `submodule_record`, `_resolution_text` and the record's preamble are
  unchanged by the merge, and so are `_answered_legs`, `_kept_in_a_leg`,
  `_commit_the_split`, `_refuse_what_check_finds`, `cmd_execute` and `Plan`.
  The record and #195's finding read the kept submodules the same way,
  `_kept_in_a_leg(_answered_legs(...))` over the tip's gitlinks. `execute`
  refuses #195's finding in `_refuse_what_check_finds`, before any leg
  exists and before `_commit_the_split` writes the record. So a source
  #195 refuses gets no record, and so does a source whose tip holds no
  submodule.
- scripts/repo_shape.py: main refuses an explicit key in a flow mapping
  and reads an empty flow-mapping value as null (#252). A block scalar's
  record is now a (key, body) pair, and `_BLOCK_SENTINEL` is gone (#248);
  nothing on this branch names it. The record is a block sequence of
  double-quoted scalars, and it reads back through the merged reader with
  0 mismatches (`submodule_record`, then `parse_yaml`, path and
  resolution):
  - 4,525 names, plus 35 more;
  - 240 aimed at #252 and #248 (`? a`, `{a: }`, `|`, `a: |`, a NUL, the
    old sentinel).
- scripts/shape_materialize.py: main adds `escape_what_stdout_cannot_encode`,
  `run()` reading UTF-8 (#217), and `first_and_more(paths, spell)` (#212).
  This branch's `LEG_SEGMENT_REASONS` and `_spelling_reason` are untouched.
- shape-doctor.py: main adds the `manifest kinds` finding for a
  `family.yaml` beside `project.yaml` (#211), and the stdout escaping.
  This branch's placement row and `recorded_submodules` are untouched.
- tests: #212 respells the finding text that the earlier tests in
  test_adopt_submodules.py assert. None of this branch's tests quotes a
  finding.
- No top-level name is bound twice in adopt-project.py,
  scaffold-project.py, scripts/repo_shape.py, scripts/shape_materialize.py,
  shape-doctor.py, validate-manifest.py or update-shape.py. Each merged
  module's names are the base's, plus what either side added, minus what
  either side removed. No side added a name the other side also added,
  in the tests either.
- AGENTS.md is untouched by main: 472 lines, under the cap of 472.
- The branch's diff against main is still #189 alone: 12 files, +1670
  -156, file by file the same as against 39ac59c.

Lane: openreposhape-1 (openRepoShape-1)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 12, 2026
…, #209, #252, #195, #210, #211, #248, #212, #251 and #206 merge with one hunk, the materializer import list, kept on both sides, and `_entry_findings` still hands `_leg_findings` the tree and ends with the root-symlink findings

origin/main moved 39ac59c -> 60588d5: 06b5204 (#214 via #246), 60d08a1
(#217 via #230), 2727b08 (#209 via #218), 4b2add8 (#252 via #257),
aaa40eb (#195 via #226), d89fffa (#210 via #219), eb6df75 (#211 via
#220), c3f4987 (#248 via #265), 5751460 (#212 via #223), 23e16ae (#251
via #271) and 60588d5 (#206; its subject names no pull request).

Conflict, resolved by keeping both sides:
- adopt-project.py, the `shape_materialize` import list. This branch
  adds BOOTSTRAP, SYMLINK_HOPS and template_files; #214 adds
  escape_what_stdout_cannot_encode. The resolution is the union.

Auto-merged: scripts/shape_materialize.py, where #214's escaping, #217's
UTF-8 `run()` and #212's `first_and_more(paths, spell=None)` sit apart
from this branch's #183 section, and adopt-project.py's other hunks.
#195 adds `import os` at the line where this branch added it, and the
merge keeps it once.

Checked on the merged tree:
- `_entry_findings` still calls `_leg_findings(plan, tree)` and ends
  with `_root_symlink_findings(plan, source, tree)`. `main` still calls
  `escape_what_stdout_cannot_encode()` first.
- No top-level name is bound twice in adopt-project.py,
  scaffold-project.py, scripts/repo_shape.py or
  scripts/shape_materialize.py, and none is loaded but bound nowhere
  (AST). `first_and_more` gives its new `spell` a default, so this
  branch's call is unchanged.
- Both sides are intact by +/- line sets: each side's change against the
  base reappears against the other side, apart from the import hunk and
  the shared `import os`.
- #212 says this branch's `plan-root-symlink` names its paths raw, and
  that whichever pull request lands second should spell them. That was
  04792d2. Since d11cf9f every path the finding names goes through
  `_spelled`.

One clash the textual merge does not show is fixed in the next commit:
#217's tests/test_git_output_decoding.py refuses a shipped text-mode
`subprocess` call that names no codec, and this branch's
`_checks_out_symlinks` probe names none.

Lane: openreposhape-1 (openRepoShape-1)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 12, 2026
…e-notice

It merged with no conflict. Main brings nine commits since 39ac59c:

- 06b5204, six tools escape a name their stdout cannot encode (#214
  via #246), with tests/test_stdout_encoding.py;
- 60d08a1, `shape_materialize.run()` and 26 other git and gh calls
  read the child's output as UTF-8 (#217 via #230), with
  tests/test_git_output_decoding.py;
- 2727b08, update-shape.py's `gh pr create --base` (#209 via #218),
  with tests/test_update_shape_pr_base.py;
- 4b2add8, scripts/repo_shape.py's flow mappings (#252 via #257), with
  tests/test_yaml_flow_mappings.py;
- aaa40eb, adopt-project.py refuses a `.gitmodules` the history's
  gitlinks make unreadable (#195 via #226), with
  tests/test_adopt_history_gitlinks.py;
- d89fffa, family.py `bump` to the current pin (#210 via #219);
- eb6df75, shape-doctor.py's family.yaml beside a project.yaml (#211
  via #220);
- c3f4987, scripts/repo_shape.py's block-scalar records (#248 via
  #265), with tests/test_yaml_block_scalar_keys.py;
- 5751460, adopt-project.py spells every path a FINDING or a refusal
  names (#212 via #223), with tests/test_adopt_finding_spelling.py.

Three files changed on both sides: adopt-project.py,
tests/test_adopt_ignored_shape_files.py and tests/test_adopt_verify.py.
In each, `git diff -U0` of this merge against 5a8b649 equals main's
diff against 39ac59c, and this merge against 5751460 equals the
branch's diff against 39ac59c, apart from the hunk headers. Neither
side's lines are changed.

How main's changes meet this branch's failure handling:

- Every definition the late failures go through is byte-identical to
  5a8b649's: LATE_FAILURES, the notice (`_nothing_rolled_back`),
  `_failed_loudly`, `_report_late`, the three builders' handlers,
  `_verify` and `cmd_execute` among them. Three definitions differ, each
  by main's hunk alone.
- `main()` gains `escape_what_stdout_cannot_encode()` as its first
  statement (06b5204). The branch writes every REFUSED block and notice
  to stderr, which Python opens with `backslashreplace` whatever the
  locale.
- `run()` names its codec (60d08a1). Output that is not UTF-8 now
  reaches the branch's handlers as `CommandFailed`, not as
  `UnicodeDecodeError`. LATE_FAILURES keeps `UnicodeError`, because
  `git_out` in scripts/repo_shape.py still decodes strictly.
- `Source.registered_submodules` reads the history (aaa40eb). Its one
  caller in `execute` is `_refuse_what_check_finds`, before
  `_create_leg_remotes`, so the new refusal comes before any leg
  repository exists and needs no notice.
- 5751460 spells the paths of the verification's findings. The
  branch's footer and notice follow those findings unchanged.

No top-level name is bound twice in adopt-project.py,
scaffold-project.py, scripts/repo_shape.py or
scripts/shape_materialize.py, and a `symtable` check finds no name
they read that nothing binds. On the merged tree, ten test files give
439 passed and 1 skipped (Windows' own stdout default): the two shared
ones, main's four new ones that read adopt-project.py
(finding_spelling, history_gitlinks, stdout_encoding,
git_output_decoding), and test_adopt_late_failure,
test_adopt_filter_repo_failure, test_adopt_execute_refusals and
test_adopt_plan_escapes.

Lane: openreposhape-1 (openRepoShape-1)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant