Skip to content

apply(): refuse tagged keys, NaN equals NaN, refuse NFD look-alikes (#749) - #759

Merged
markramm merged 2 commits into
devfrom
fix/749-apply-tagged-keys
Oct 8, 2026
Merged

markramm merged 2 commits into
devfrom
fix/749-apply-tagged-keys

Conversation

@markramm

@markramm markramm commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #749 (items 1, 2, 4, 5; item 3 stated exactly in Limits; item 6 left).

apply() raises only OperationRefusedError, with a true reason, on any input. No caller yet.

Groom check, riskiest assumption first. The ticket said every tagged key raises TypeError/KeyError. Confirmed on dev, plus a third case it did not name: !!float 1: a loads as float 1.0 in ruamel but reads as int 1 plain (KeyError on path 1). !!int 1: a is not a problem (ruamel loads int 1). Item 5 (a list's last item drops the comments between items) did not reproduce on ~16000 generated layouts; it is pinned as a control, not fixed.

Choices (stated in the module's Limits)

  • Tagged key anywhere: refused for every operation, reason "tagged key". Two guards, each with a case the other cannot see: _has_tagged_key (node tag vs plain reading: !!float 1) and _loaded_tagged_key (what ruamel loaded: !!str x).
  • NaN equals NaN (_scalar_eq), as value, key or list item; and the old "cannot write NaN" refusal went with it (.nan is now emitted).
  • No Unicode normalisation: a path or sub-key that differs from an existing key only by NFC/NFD is refused (_lookup); the file's spelling, and a file holding both spellings, are edited exactly.

Covers (every operation x every place a key sits): _parse is the one entry for all ops, so tagged keys are refused there for Set, Append, Remove, Unset, AddSubkey and ReplaceBody (_EVERY_OP); at top level, nested, in a list item, in a flow map. NFD refusal is in _lookup, which _model_apply calls first for every segment of every op. NaN is in values_equal, _exact, _key_matches and _same_keys.

Evidence

  • verify-red: 16 red - 0 import-only - 0 unexpected pass - 0 n/a - 12 control.
  • Guard deletion (each alone, targeted tests): tagged node guard, loaded guard, NFD guard, _scalar_eq, .nan emit each turn a named test red.
  • Fuzz test_malformed_operations_raise_only_operation_refused now has tagged, complex, .nan and NFD texts and path pieces (6000 iterations); TypeError on dev, green here.
  • Local test-affected --run on the tree before the last docs/test-mark commit: 667 passed. An earlier local pre-push run on the final tree went over 2h under load and was stopped; the push that landed was not preceded by a completed local run. CI is the authority.
  • Slowest test in the file: the fuzz, 2.15 s alone (not the cause of the slowness).

Hallway notes: feedback/2026-10-05-749-apply-tagged-keys.md.

🤖 Generated with Claude Code

Worker report (2026-10-06)

Pushed: fad35de == origin/fix/749-apply-tagged-keys. 1 commit, rebased on dev 7d75837 (#744 splitter; no private span helper).
Closes: #749 items 1, 2, 4, 5. Item 3 is stated as a measured limit. Item 6 (independent oracle in CI) is left.
Evidence: verify-red 16 red · 0 import-only · 0 unexpected pass · 0 n/a · 12 control. test-affected -n 4: 667 passed, on the tree before the last commit (controls and docs only). No completed local run on the pushed SHA; CI is the check.
Guards (each removed alone turns its named test red): _has_tagged_key, _loaded_tagged_key, the Unicode-normalisation guard in _lookup, NaN in _scalar_eq, .nan emission in _emit_float.
Learned: the groom's tagged-key list was incomplete. !!float 1: loads as 1.0 but reads plain as int 1, a KeyError. !!str x is indistinguishable from x at node level, hence two guards. Item 5 (comment loss) did not reproduce over about 16k generated layouts, so it is pinned as a control. Item 3's widen loss is narrower than the ticket says.
Captured in: the test_a_tagged_key_*, test_a_nan_* and normalisation tests; the Limits section of the module docstring; feedback/2026-10-05-749-apply-tagged-keys.md.
Unsure: (1) refusing EVERY operation, ReplaceBody included, on a file with a tagged key; (2) the conservative node guard also refuses !!int "1"; (3) the NaN wording in Limits.
Left: item 6; no local run on the final SHA.
Tokens: 199k (subagent total).

Fix round 1 (2026-10-06): every unnameable key, any container, a true reason

Pushed: 8f7b1c5 (one commit on fad35de). test_file_operations.py at -n 4: 265 passed. verify-red not run for this round; CI is the authority.

Conductor review (2026-10-08)

The delta cold read of round 1 (8f7b1c5) says land. !!set and !!pairs tagged keys are now refused as "tagged key" for every operation, ReplaceBody included. Plain sets, omaps and pairs stay editable. It found no false refusals across 26 common frontmatter forms and the 17 SHAPES files, and no bytes changed outside the named spans. CI is green: test (3.12), verify-red, gate.
Known limits, all fail-safe, tracked in #769: a tagged key next to its plain twin is reported as a duplicate key; a tag inside a set member's value is invisible (ruamel discards it); the body-write wording on the other refusals; a test assertion pinned to old wording. Body-only writes on frontmatter Pyrite cannot model are tracked in #760.

…lisation look-alikes

A tagged key (!foo a: 1, !!str a: 1, !!float 1: a) made every operation raise
TypeError/KeyError; it is now refused with a reason. .nan blocked every edit
of its file with a misleading reason; NaN now equals NaN and can be written.
An NFD key was shadowed by an added NFC look-alike; it is now refused. The
fuzz covers tagged, complex, .nan and NFD keys. Limits section states each
choice; unsetting a list's last item is pinned (not reproducible).

Fixes #749

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ith a true reason

Fix round 1 of #759. _loaded_tagged_key checked isinstance(x, set), but ruamel
loads a CommentedSet (an abc.Set), and !!pairs loads as (key, value) tuples, so
a tagged key there reached the post-check and was refused with an untrue
reason. Walk every container a key can load into; one parametrised test lists
them. The reason no longer says 'no path can name it' (untrue for ReplaceBody).
Limits state the normalisation guard's scope (NFC canonical equivalence, keys).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@markramm
markramm marked this pull request as ready for review October 8, 2026 00:44
@markramm
markramm added this pull request to the merge queue Oct 8, 2026
Merged via the queue into dev with commit f30f4b3 Oct 8, 2026
11 checks passed
@markramm
markramm deleted the fix/749-apply-tagged-keys branch October 8, 2026 01:02
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.

B6 P1 apply(): limits to close before P3 wires it in (tagged keys, .nan, widen scope, NFD keys)

1 participant