Skip to content

fix: make Remove patch op sound for flow collections and empty parents#2121

Open
mostafa wants to merge 1 commit into
zizmorcore:mainfrom
mostafa:fix/remove-op-soundness
Open

fix: make Remove patch op sound for flow collections and empty parents#2121
mostafa wants to merge 1 commit into
zizmorcore:mainfrom
mostafa:fix/remove-op-soundness

Conversation

@mostafa

@mostafa mostafa commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Pre-submission checks

Please check these boxes:

  • Mandatory: This PR corresponds to an issue (if not, please create
    one first).

  • Having read the AI policy, I hereby disclose the use of an LLM or other
    AI coding assistant in the creation of this PR. PRs will not be rejected
    for using AI tools, but will be rejected for undisclosed use or
    use that violates the policy.

If a checkbox is not applicable, you can leave it unchecked.

Summary

Closes #2122. Reworks the Remove patch op so it is sound for flow collections and nested structures, addressing the soundness concern raised in #1020 and following the "re-think within yamlpath" direction of #1000. Relates to META #876.

Previously Remove deleted whole lines, which:

  • corrupted flow mappings and sequences (removing b from {a: 1, b: 2} deleted the whole line), and
  • left dangling null parents (removing the only key under env: left env: with no value).

The earlier flow handling in #1020 detected the container by sniffing for {/[ tokens, which is brittle because it cannot distinguish nesting like {[ ... ]} from [{ ... }].

Removal span computation now lives in yamlpath as Document::removal_span, which uses the parse tree rather than token heuristics:

  • Dispatches on the value's actual container kind (block vs flow) as reported by tree-sitter, so nested mixtures like [{a: 1}, {b: 2}] and {x: [1, 2]} resolve correctly.
  • For flow members, removes the element plus a single adjacent comma and the whitespace up to its neighbor, and collapses a sole member cleanly to {} or [].
  • Resolves the element at its own lexical site, so a terminal alias is not followed: removing b: *x deletes b: *x rather than the anchor definition a: &x 1 it points to.
  • Collapses dangling empty block parents upward to the first ancestor that still has other content. Flow containers are left as valid {}/[], and block containers only nest inside block containers, so collapse never crosses styles.

yamlpatch's Op::Remove is now a thin call into Document::removal_span.

Verified outputs match the (draft) expectations in #1000: foo: { a: a, b: b } with /foo/b removed gives foo: { a: a }; an empty key foo/baz removal preserves the rest; and foo: [1, 2, 3: {abc: def}] with /foo/2 removed gives foo: [1, 2].

Design note: #1000 leaned toward extending QueryMode::Pretty in query_node and deleting that span. I added a dedicated removal_span instead, because removal has needs that pretty extraction does not (separator handling, sole-member collapse, parent collapse) and query_pretty is shared with Op::Replace. Happy to fold this into query_pretty if you prefer.

Test Plan

  • cargo test -p yamlpath -p yamlpatch (added snapshot coverage for block/flow mappings and sequences, nested flow collections, multiline flow, alias safety, and block parent collapse)
  • cargo test -p zizmor --bins insecure_commands (the only current Op::Remove consumer)
  • cargo fmt --check
  • cargo clippy -- --deny warnings

The Remove op previously deleted whole lines, which corrupted flow
mappings and sequences (e.g. removing `b` from `{a: 1, b: 2}` deleted the
entire line) and left dangling null parents (e.g. removing the only key
under `env:` left `env:` with no value).

Resolve removal spans in yamlpath via the parse tree instead:

- Dispatch on the value's actual container kind (block vs flow), so
  nested mixtures like `[{a: 1}, {b: 2}]` and `{x: [1, 2]}` resolve
  correctly rather than by sniffing for `{`/`[` tokens.
- Remove flow members with a single adjacent comma and surrounding
  whitespace, collapsing a sole member cleanly to `{}`/`[]`.
- Resolve the element at its own lexical site so a terminal alias is not
  followed (removing `b: *x` no longer deletes the anchor `a: &x 1`).
- Collapse dangling empty block parents upward to the first ancestor that
  retains other content; flow containers stay as valid `{}`/`[]`.

yamlpatch's Op::Remove now delegates to Document::removal_span.
@woodruffw

Copy link
Copy Markdown
Member

Thanks @mostafa! I'm hoping to give this a review in the next day or so.

@woodruffw woodruffw added enhancement New feature or request autofix Auto-fix functionality bugfix Fixes a known bug and removed enhancement New feature or request labels Jun 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix Auto-fix functionality bugfix Fixes a known bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Remove patch op corrupts flow collections and leaves dangling parents in fix mode

2 participants