Adopt typed compare engine output and remove legacy violation contract by corymhall · Pull Request #105 · pulumi/schema-tools

corymhall · 2026-02-27T10:33:56Z

Summary

This PR establishes typed Change items as the canonical output of the internal compare engine.

It replaces violation plumbing with a typed-first contract that downstream renderers can consume directly.

What Changed

Reworked internal/compare model and engine output to emit typed change records.
Removed legacy violation-oriented report fields and bridging logic.
Strengthened engine/requiredness tests around deterministic typed output and removed-output edge cases.
Kept category semantics explicit in one place so later PRs can project/render from a stable contract.

Why

The stack needs a single canonical change model before projection/rendering and lookup normalization can be simplified safely.

This is the foundation for follow-up projection and lookup integration work in #106 and above.

corymhall · 2026-02-27T10:34:15Z

This change is part of the following stack:

Define normalization metadata contract and loader #94
- Adopt typed compare engine output and remove legacy violation contract #105 ◀
  - Rebuild compare projections and renderers around typed changes #106
    - Add metadata-driven token lookup resolution for normalization #107
      - Implement field-path and type-equivalence lookup helpers #108
        
        Integrate token lookup decisions into compare change generation #109
        
        Integrate field and type lookups in compare and simplify command flow #110
        
        Normalize rename-aware maxItems transitions using real AWS fixtures #111
        
        Finalize structured goldens, harness coverage, and compare output semantics #112

_{Change managed by git-spice.}

guineveresaenger

I've got a few comments but they may be because I haven't worked my way all the way down the PR stack yet.

guineveresaenger · 2026-02-27T22:32:45Z

internal/compare/engine.go

+		*changes = append(*changes, newChange(category, name, path, ChangeKindTypeChanged, fmt.Sprintf("type changed from %q to %q", oldType, newType)))
+	}
+
+	appendTypeChanges(changes, category, name, append(slicesClone(path), "items"), old.Items, new.Items)


is slicesClone just for safety?

Yep, exactly for path-slice safety during recursive append(...) calls. I’ve now switched this over to stdlib slices.Clone so we keep the safety behavior without a custom helper.

guineveresaenger · 2026-02-27T22:33:20Z

internal/compare/engine.go

+	appendTypeChanges(changes, category, name, append(slicesClone(path), "additional properties"), old.AdditionalProperties, new.AdditionalProperties)
+}
+
+func slicesClone(xs []string) []string {


Any reason not to use the stdlib https://pkg.go.dev/slices#Clone?

Good catch, I thought I replaced all usages of this type of thing, but missed one!

guineveresaenger · 2026-02-27T22:34:54Z

internal/compare/engine.go

+		Changes:      changes,
+		NewResources: newResources,
+		NewFunctions: newFunctions,
+		Violations:   BreakingChanges(oldSchema, newSchema),


Are we looking at getting rid of this further down the PR stack? I'd think that breaking changes would ultimately be part of Changes? I'm asking because buildChanges is effectively the same walk that BreankingChanges does.

Yes, that’s the direction. This PR introduces typed Changes while still keeping Violations to make this PR diff smaller. The downstream stack PRs remove the legacy path.

guineveresaenger · 2026-02-27T22:36:07Z

internal/compare/engine.go

 			for _, req := range f.Outputs.Required {
-				_, stillExists := f.Outputs.Properties[req]
+				stillExists := false
+				if newFunc.Outputs != nil {


Should this be fixed separately?

I kept this here because it’s a correctness fix in the same compare logic path touched by this refactor. Without it, we can incorrectly report “required -> optional” when the output is actually removed. I added regression coverage for this case in engine_test.go.

guineveresaenger · 2026-02-27T22:36:43Z

internal/compare/engine.go

 		newRequired := set.FromSlice(newTyp.Required)
 		for _, r := range typ.Required {
-			_, stillExists := typ.Properties[r]
+			_, stillExists := newTyp.Properties[r]


same - isn't this an unrelated change?

Similar reasoning here: this is a bug fix in requiredness gating (newTyp.Properties is the right source of truth for “still exists”). Keeping it with this change avoided carrying known-bad behavior into the typed engine transition, and it now has test coverage.

Squash details: - Branch: st-ac3.17.3.1-typed-change-engine - Base: st-ac3.17-011-normalize-metadata-contract - Squashed commits: 5 Original commits: - d437c23 internal/compare: emit typed change report - b1c2126 internal/compare: make analyze typed-first contract - b92c677 internal/compare: remove violations shim from report - cc68ad5 internal/compare: restore compatibility and fix requiredness gating - e576846 Fix requiredness gating for removed outputs and properties

## Summary This PR rebuilds compare projections and renderers to consume typed change data directly. It removes legacy rendering assumptions and aligns JSON/text output contracts with the typed engine introduced in #105. ## What Changed - Reworked compare projection logic to build structured `changes` and `grouped` views from typed events. - Updated JSON and text renderers to use the structured model consistently. - Removed dead legacy-engine artifacts from compare internals. - Added/updated golden fixtures and renderer tests for deterministic contract coverage. - Added compare-level structured golden coverage for fixture parity. ## Why Renderer/projection behavior should be derived from one typed source of truth. Doing this here makes later normalization lookups additive, instead of requiring command-layer patchups. ## Context - Builds directly on #105. - Provides the projection baseline that later lookup-focused PRs rely on.

…antics (#112) ## Summary This PR finalizes the compare structured-output wave by aligning contracts, goldens, and harness coverage across compare and cmd paths. It also captures finalized output semantics for capped text rendering and full structured JSON output. ## What Changed - Finalized structured compare/cmd golden harness coverage for AWS mini fixtures. - Updated compare/cmd tests and fixture expectations to the finalized contracts. - Kept JSON structured output uncapped while keeping text output capped. - Refined requiredness/remap severity/breaking classification to match the final policy. - Updated README/docs and supporting command/test plumbing required for end-to-end parity. ## Why This closes the loop on behavior, tests, and documentation so reviewers can validate a stable end-state at the stack tip. It ensures the same semantics are exercised by both compare package tests and command harness tests. ## Context - Final stack tip on top of #111. - Consolidates contract/golden alignment after upstream engine and lookup integrations (#105–#111). closes #83 closes #84

corymhall changed the title ~~st-ac3.17.3.1-typed-change-engine: squash branch history~~ Adopt typed compare engine output and remove legacy violation contract Feb 27, 2026

corymhall marked this pull request as ready for review February 27, 2026 13:39

corymhall requested a review from a team February 27, 2026 13:42

guineveresaenger approved these changes Feb 27, 2026

View reviewed changes

Base automatically changed from st-ac3.17-011-normalize-metadata-contract to master March 2, 2026 11:13

corymhall added 2 commits March 2, 2026 06:15

Use slices.Clone in typed compare engine

cf2f789

corymhall force-pushed the st-ac3.17.3.1-typed-change-engine branch from 5048446 to cf2f789 Compare March 2, 2026 11:16

corymhall merged commit 05e962f into master Mar 2, 2026
5 checks passed

corymhall deleted the st-ac3.17.3.1-typed-change-engine branch March 2, 2026 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt typed compare engine output and remove legacy violation contract#105

Adopt typed compare engine output and remove legacy violation contract#105
corymhall merged 2 commits intomasterfrom
st-ac3.17.3.1-typed-change-engine

corymhall commented Feb 27, 2026 •

edited

Loading

Uh oh!

corymhall commented Feb 27, 2026 •

edited

Loading

Uh oh!

guineveresaenger left a comment

Uh oh!

guineveresaenger Feb 27, 2026

Uh oh!

corymhall Mar 2, 2026

Uh oh!

guineveresaenger Feb 27, 2026

Uh oh!

corymhall Mar 2, 2026

Uh oh!

guineveresaenger Feb 27, 2026

Uh oh!

corymhall Mar 2, 2026

Uh oh!

guineveresaenger Feb 27, 2026

Uh oh!

corymhall Mar 2, 2026

Uh oh!

guineveresaenger Feb 27, 2026

Uh oh!

corymhall Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

corymhall commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Changed

Why

Uh oh!

corymhall commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guineveresaenger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

corymhall commented Feb 27, 2026 •

edited

Loading

corymhall commented Feb 27, 2026 •

edited

Loading