Skip to content

fix(schematic): validate mutations before commit - #527

Merged
neusse merged 1 commit into
mixelpixx:mainfrom
neusse:fix/499-precommit-schematic-validation
Sep 11, 2026
Merged

neusse merged 1 commit into
mixelpixx:mainfrom
neusse:fix/499-precommit-schematic-validation

Conversation

@neusse

@neusse neusse commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Closes #499

Problem

The component edit, annotation, grouping, and shared hierarchy mutation paths could commit a schematic and only then discover that the saved result did not match the bound request. They returned a refusal even though the file had already changed, which made a retry unsafe.

Change

  • Parse and semantically validate the exact prospective command result before any write.
  • Bind component UUID/reference/unit/library/hierarchy/placement/property intent for edit_schematic_component, add_component_annotation, and group_components.
  • Apply the same contract to hierarchy sheet edits, sheet-pin edits, and sheet deletion.
  • Keep committed-file readback as an independent backstop.
  • Return additive structured mutation_outcome_uncertain evidence when a completed write cannot be proven, explicitly warning that the file may have changed.
  • Add Schematic::from_source so prospective text can be parsed without disk I/O.
  • Document the public response change and atomicity contract.

Atomicity evidence

Regression tests invoke the real handlers against KiCad-authored fixtures and inject a one-shot prospective writer mismatch. Every refusal is stale_target, the original target remains byte-identical, and an absent component target is not created.

Required negative controls were also run locally:

  1. Temporarily removed the component prospective-validation guard. component_mutation_handlers_refuse_bad_prospective_results_without_writing failed because the handler no longer returned the pre-commit refusal.
  2. Restored it, temporarily removed the hierarchy prospective-validation guard. hierarchy_handlers_refuse_bad_prospective_results_without_writing failed for the same reason.
  3. Restored both guards; both focused tests and their complete module suites passed.

Validation

  • cargo fmt --all -- --check
  • cargo clippy --workspace --locked --all-targets -- -D warnings
  • cargo test --workspace --locked --lib --tests
  • cargo test --workspace --locked --doc
  • cargo xtask fix-doc-counts --check
  • Independent standards review: no findings remaining
  • Independent issue-specification review: no gaps remaining

Tool counts remain current: 21 toolsets, 226 registered tools, 233 total tools.

Sequencing and compatibility

This was reconstructed on current main after #489 and #485 merged, as requested. PR #524 overlaps the hierarchy helper and should reconstruct/rebase on the resulting main after this lands.

No tool or argument is removed or renamed. mutation_outcome_uncertain is an additive structured error response documented for the next minor release.

Rollback

Reverting this commit restores the previous write-then-verify behavior; no persisted schema or migration is introduced.

@neusse
neusse requested a review from mixelpixx as a code owner September 11, 2026 16:28
@neusse neusse added P0 Correctness or data-safety: fix before the next release area:schematic Schematic capture and analysis status:waiting-on-review Next actor: maintainer labels Sep 11, 2026
@neusse
neusse merged commit 8ed319a into mixelpixx:main Sep 11, 2026
10 checks passed
@neusse neusse removed the status:waiting-on-review Next actor: maintainer label Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:schematic Schematic capture and analysis P0 Correctness or data-safety: fix before the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Schematic mutation handlers can return a stale_target refusal after the file has already been changed

1 participant