Skip to content

fix(workflows): guard a non-string overlay edit operation - #3881

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/overlay-nonstring-operation
Open

fix(workflows): guard a non-string overlay edit operation#3881
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/overlay-nonstring-operation

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

_parse_edit in src/specify_cli/workflows/overlays/schema.py reads operation straight from hand-edited YAML, then membership-tests it:

if operation not in VALID_OPERATIONS:

VALID_OPERATIONS is a frozenset, so that test hashes the value. An unhashable one raises instead of being reported.

Reproduction on current main (81bf741)

op={'insert_after': 'a'}  -> RAISED TypeError: unhashable type: 'dict'
op=['insert_after']       -> RAISED TypeError: unhashable type: 'list'

Why this is a contract violation, not just an edge case

validate_overlay_yaml's own docstring states:

Errors are returned as a list of strings; validation never raises.

And nothing upstream catches TypeErrorlayer_sources wraps only yaml.YAMLError / OSError / UnicodeDecodeError, and _commands.py catches only ValueError. So the raw traceback reaches the user.

The trigger is an ordinary authoring mistake — nesting the recommended shorthand form under the explicit key:

edits:
  - operation:
      insert_after: build

Every other field in the same function is isinstance-guarded before use:

Field Guard
anchor schema.py:93if not isinstance(anchor, str) or not anchor:
step schema.py:102if not isinstance(step, dict):
step["id"] schema.py:105if not isinstance(step_id, str) or not step_id:
_validate_safe_id schema.py:49if not isinstance(value, str) or not value:
operation none

Fix

One code line, matching the sibling guards:

if not isinstance(operation, str) or operation not in VALID_OPERATIONS:

No breaking change. The only inputs whose behaviour changes are the ones that raise TypeError today; they now return the same "Edit at index N has invalid operation ..." message the function already produces for operation: None or operation: 7. Every currently-valid and currently-cleanly-rejected input is byte-identical.

Scope note

merge.py:384 has the same edit.operation not in VALID_OPERATIONS pattern, but an OverlayEdit can only be constructed by _parse_edit, so once this guard is in place that path is unreachable. I left it alone to keep the PR to one file — happy to add a defensive guard there too if you'd prefer.

Verification

  • Fail-before / pass-after: both new parametrized cases fail on unpatched src (TypeError) and pass with the fix. File: 2 failed → 33 passed.
  • Scoped regression over tests/workflows: failure set identical to the clean-main baseline I captured on 81bf741 (10 pre-existing in scope, all Windows symlink-privilege).
  • uvx ruff@0.15.0 check src tests → clean

Test goes into the existing TestShorthandEdits, beside test_invalid_operation_field_rejected.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

`_parse_edit` reads `operation` straight from hand-edited YAML and then
does `if operation not in VALID_OPERATIONS`. `VALID_OPERATIONS` is a
frozenset, so that membership test hashes the value — and an unhashable
one raises:

  operation={'insert_after': 'a'} -> TypeError: unhashable type: 'dict'
  operation=['insert_after']      -> TypeError: unhashable type: 'list'

`validate_overlay_yaml`'s docstring promises "validation never raises",
and nothing upstream catches TypeError (layer_sources wraps only
YAMLError/OSError/UnicodeDecodeError; _commands catches only ValueError),
so the CLI dies with a raw traceback instead of reporting the error.

The trigger is an ordinary authoring mistake: nesting the recommended
shorthand form under the explicit key.

Every other field in the same function is isinstance-guarded first
(`anchor`, `step`, `step["id"]`); `operation` was the outlier. Check the
type first and return the message the function already uses for
`operation: None` / `operation: 7`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents malformed YAML overlay operations from raising TypeError, preserving the validator’s never-raise contract.

Changes:

  • Type-checks operation before set membership.
  • Adds regression tests for mapping and sequence values.
Show a summary per file
File Description
src/specify_cli/workflows/overlays/schema.py Safely rejects non-string operations.
tests/workflows/test_overlay_schema.py Covers previously unhashable operation values.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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.

3 participants