feat(spec): preserve referenced macros during removal

[Thien Trung Vuong (trungams)]

Summary

Removing a section or subpackage can also remove %define or %global declarations that happen to live inside it. When surviving content still references one of those macros, the rendered spec is left with a dangling reference and often fails much later during the build.

This PR teaches the structural editor to preserve the complete declarations that surviving content still needs. It follows transitive dependencies and keeps declaration ordering intact. When a relocation is ambiguous or clearly unsafe, the removal fails without modifying the spec.

Motivation

Issue #203 documents this with QEMU: %define testsdir lives in the tests subpackage, but %install continues to use it after that subpackage is removed. The macro declaration must survive even though the package sections do not.

Changes

• Select removed macro declarations referenced by surviving content.
• Follow transitive dependencies at the relevant use order.
• Preserve complete multiline declarations and original declaration order.
• Scan section-header arguments while excluding the section marker itself.
• Avoid duplicate hoists when a safe surviving definition already supplies the binding.
• Log automatic macro preservation.
• Reject unsafe cases with ErrUnsafeMacroHoist without modifying the spec, including:
  • ambiguous conditional declarations;
  • unsafe eager %global dependencies or redefinitions;
  • selected dynamic macro names;
  • self-referential selected %global declarations;
  • incompatible same-name bindings.
• Add QEMU-shaped spec-remove-subpackage drops %define macros inside the removed subpackage that are referenced elsewhere #203 coverage and focused rejection tests.

Validation

• mage build
• mage unit

The completed structural editor also rendered the full Azure Linux evaluation corpus without parser or overlay errors, including automatic preservation of QEMU's testsdir macro.

Known limitations

This is conservative structural and lexical analysis, not RPM macro evaluation. If azldev cannot establish a safe binding, it rejects the removal instead of guessing. Conditional same-name bindings that would require evaluating the condition remain best-effort.

[Copilot]

🟡 Changes recommended

Unresolved integration and macro-analysis correctness issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds structural RPM macro hoisting when sections or subpackages are removed, preserving referenced declarations and rejecting unsafe relocations.

Changes:

• Tracks transitive macro dependencies and preserves declaration order.
• Rejects ambiguous or unsafe hoists without modifying the spec.
• Adds focused tests and regression fixtures.

File summaries

File	Summary
internal/rpm/spec/tree_hoist.go	Implements macro analysis, validation, and hoisting.
internal/rpm/spec/tree_hoist_internal_test.go	Adds focused hoisting and rejection tests.
internal/rpm/spec/testdata/specs/subpackage-define-transitive.spec	Covers transitive macro preservation.
internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec	Covers shadowed definitions.
internal/rpm/spec/testdata/specs/subpackage-define-referenced.spec	Covers direct macro references.
internal/rpm/spec/testdata_test.go	Adds fixture-based integration coverage.
internal/rpm/spec/structural_tree_api.go	Integrates hoisting with section removal.

Review details

Suppressed comments (7)

internal/rpm/spec/structural_tree_api.go:188

• This changes the user-facing behavior of both spec-remove-section and spec-remove-subpackage, but docs/user/reference/config/overlays.md still only documents section deletion. Please document that referenced macro declarations may be hoisted and that conservative ErrUnsafeMacroHoist cases abort without changing the spec.

	if err := t.hoistReferencedMacros(sections); err != nil {
		return err
	}

internal/rpm/spec/structural_tree_api.go:188

• An empty handle slice now reaches hoistReferencedMacros, where no removed root ancestor exists and ErrUnsafeMacroHoist is returned. Before this hook, validateSectionRemoval followed by removeSections made RemoveSections(nil) a no-op; retain that behavior for callers that build a selection dynamically and can have no matches.

	if err := t.hoistReferencedMacros(sections); err != nil {
		return err
	}

internal/rpm/spec/tree_hoist.go:466

• Dynamic invocations are only checked against removed declaration names here; this never resolves a dynamic target to a surviving lazy definition and traverses that definition's dependencies. For example, with surviving %define suffix name and %define rootname %{dep}, a removed %define dep value, and echo %{root%{suffix}}, the dynamic target is rootname so dep is never selected and removal leaves a dangling %{dep} when rootname expands. Resolve or conservatively reject matching surviving definitions at the invocation order.

	for _, reference := range facts.references {
		visit(effectiveMacroDefinition(facts.all[reference.name], reference.order), reference.order)
	}

internal/rpm/spec/tree_hoist.go:616

• This treats macro-looking text in comments as a live reference. RPM does not expand a # comment, so a removed parameterized or conditional declaration plus a surviving line such as # %{helper} will select that declaration and then fail with ErrUnsafeMacroHoist, even though no surviving content needs it. Skip comment-only lines before collecting references.

				if !removed {
					addReferences(line, order)
				}

internal/rpm/spec/tree_hoist.go:361

• A selected %global can depend on a selected lazy %define, but this early return prevents validation from traversing that dependency. If %define base %{dep} is removed, %global root %{base} is later in the removed sections, and surviving %define dep new lies between the first removed section and root, the original eager root sees new while the hoisted root expands before it and sees a different binding; this shape is currently accepted and silently changes the spec. Validate the complete eager dependency closure at both the original and insertion orders, or reject this case.

		if definition == nil || definition.global {
			return nil

internal/rpm/spec/tree_hoist.go:532

• This rejects every surviving dependency of a selected %global, even when that dependency is already defined before the hoist insertion point. For example, a preamble %define dep /usr/lib followed by a removed %global root %{dep} and a surviving echo %{root} is safe—the hoisted global still follows dep—but this returns ErrUnsafeMacroHoist. Compare the candidate's order with insertionOrder instead of treating all surviving candidates as unsafe.

			if !candidate.removed || candidate.order >= definition.order {
				return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q before dependency %#q:\n%w",
					definition.block.Name, dependency, ErrUnsafeMacroHoist)
			}

internal/rpm/spec/tree_hoist.go:225

• facts.undefined records only whether a name was ever undefined, so any %undefine causes a selected declaration to be rejected. A preamble %undefine foo followed by a removed %define foo value is safe: the hoisted definition remains after the preamble and still follows the undefine, but this returns ErrUnsafeMacroHoist. Track directive order/context and reject only when the relocation crosses a relevant %undefine.

		if facts.undefined[definition.block.Name] {
			return fmt.Errorf("cannot safely hoist macro %#q across '%%undefine':\n%w",
				definition.block.Name, ErrUnsafeMacroHoist)
		}

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain, including unsafe dependency handling and missing production-path integration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

internal/rpm/spec/structural_tree_api.go:188

• RemoveSections previously treated an empty handle list as a no-op, but this hook now calls hoistReferencedMacros, which returns ErrUnsafeMacroHoist because there is no removed root ancestor. Any caller that computes an empty selection now fails instead of leaving the spec unchanged. Return early when len(sections) == 0 before invoking the hoister.

	if err := t.hoistReferencedMacros(sections); err != nil {
		return err
	}

internal/rpm/spec/structural_tree_api.go:188

• This hook only affects EditorStructural; the production render/prepare path initializes sourcePreparerImpl.specEditor to EditorLegacy and no caller passes WithSpecEditor(EditorStructural). Therefore the issue #203 azldev comp render -p qemu path still uses the legacy removal and can drop testsdir, while these tests exercise direct structural opens. Wire the overlay path to the structural editor or implement the same preservation in the legacy editor before treating the issue as fixed.

	if err := t.hoistReferencedMacros(sections); err != nil {
		return err
	}

internal/rpm/spec/tree_hoist.go:616

• Macro references in spec comments are treated as live references here. RPM ignores lines whose trimmed text starts with # (the tree's own content check does the same at structural_tree_api.go:556-560), so a surviving comment such as # %{helper} can hoist a removed %global and change later expansion—or make an unsafe removal fail—even though no surviving content uses it. Skip comment lines before calling addReferences and cover this with a regression test.

				if !removed {
					addReferences(line, order)
				}

internal/rpm/spec/tree_hoist.go:437

• Dynamic references inside a selected %define are recorded with definition.order rather than each surviving invocation's order. Because %define is lazy, a later removed binding can be the target at use time but is skipped by (definition.order <= dynamic.order || selected[definition]); for example, root %{dir%{suffix}} followed by removed dirname and a surviving %{root} leaves dirname undefined after hoisting. Propagate the actual use orders through closure/dynamic validation.

			dynamics = append(dynamics, macroDynamicReference{pattern: pattern, order: definition.order})

internal/rpm/spec/tree_hoist.go:531

• validateGlobalDependency treats every surviving dependency definition as unsafe, even when it is before insertionOrder and remains in the same relative position after the selected %global is hoisted. A removed %global path %{base}/bin after a surviving preamble %global base /usr/lib is safe to move, but this branch rejects it and prevents the promised preservation. Only surviving candidates that the move crosses (after the insertion point) should cause rejection; the single-binding “ambiguous” test should be adjusted accordingly.

			if !candidate.removed || candidate.order >= definition.order {
				return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q before dependency %#q:\n%w",
					definition.block.Name, dependency, ErrUnsafeMacroHoist)

internal/rpm/spec/tree_hoist.go:785

• bracedMacroEnd decrements the outer macro depth for every raw }. %{expand:...} and %{lua:...} bodies legitimately contain shell/Lua braces, and the parser already treats those braces as opaque; here an inner } can terminate the scan before later %{dependency} references. A removed macro such as %define root %{expand: ... ${x} ... %{dep} ... } can therefore be hoisted without dep, leaving a dangling reference. Track the body syntax/raw-brace state, or reuse the parser’s macro scanner, when finding the outer close.

		if content[index] == '}' {
			depth--
			if depth == 0 {
				return index, true

internal/rpm/spec/tree_hoist.go:66

• This behavior is user-visible for both spec-removal overlays, but the checked-in overlay reference and agent guidance still only say that these overlays delete sections. Please document that referenced declarations may be hoisted in order and that conservative analysis can fail with ErrUnsafeMacroHoist; otherwise users cannot predict the new output or failure mode.

// hoistReferencedMacros preserves only the exact declarations used by surviving
// references. RPM macro evaluation is contextual, so any ambiguous relocation
// is rejected rather than guessed.

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Thien Trung Vuong (trungams)]

Closing this PR for now. Review surfaced unresolved ambiguity around eager %global, conditional %define/%undefine events, and when automatic declaration relocation can be proven semantics-preserving. We would rather define the intended TOML overlay behavior and safety contract in detail before implementing this feature.