refactor(spec): add selectable legacy and structural editors

[Thien Trung Vuong (trungams)]

Summary

This PR puts the existing line-oriented editor and the new structural editor behind the same public Spec API. The legacy implementation remains intact, while the structural implementation uses the parser and tree API introduced in the previous PR.

Editor selection happens once when a spec is opened. There are no mode checks scattered through individual operations and no automatic fallback from one editor to the other. This PR also adds a curated set of real-world spec fixtures to exercise the structural implementation. Production continues to use the legacy editor until the rollout configuration is introduced at the top of the stack.

Motivation

Issue #214 calls for structural awareness when editing sections and conditionals. At the same time, azldev is stabilizing and we want an escape hatch during adoption. Keeping both implementations behind one facade gives projects a controlled migration path without changing the public editing API.

Changes

• Add a private editor interface and constructor.
• Keep Spec as the unchanged public facade.
• Preserve the legacy visitor and line-oriented implementation.
• Add the structural implementation for section, tag, search, patch, and changelog operations.
• Preserve current-main release and provenance behavior.
• Keep structural visitors fully structural; macro bodies remain opaque to tag traversal.
• Preserve sequential search/replace overlays that temporarily pass through an unbalanced conditional state.
• Add curated real-world fixtures and deterministic parser/edit stress coverage.

Validation

• mage build
• mage unit

The cumulative stack was mechanically rebased onto eb9fb3f and revalidated. The complete rollout was also exercised against the full Azure Linux structural render corpus and the default-legacy E2E suite.

Known limitations

Section and subpackage removal does not automatically relocate %define or %global declarations from removed content. Projects must preserve any declarations still needed by surviving content explicitly. The structural editor does not evaluate conditional expressions; when ownership cannot be established statically, removal is rejected rather than guessed. There is intentionally no automatic fallback between editors.

[Copilot]

🟡 Changes recommended

Unresolved moderate findings affect release handling, structural edits, tag matching, and legacy mutation coverage.

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

Pull request overview

This PR unifies legacy and structural RPM spec editing behind the existing Spec API while preserving legacy behavior by default.

Changes:

• Adds selectable editor implementations and facade delegation.
• Implements structural section, tag, search, patch, and changelog operations.
• Propagates editor selection through source preparation, overlays, release, and provenance handling.
• Adds fixtures and regression tests for macro- and conditional-heavy specs.

File summaries

File	Summary / review note
internal/rpm/spec/tree_fixture_internal_test.go	Structural fixture stress tests.
internal/rpm/spec/testdata/specs/subpackage-define-unreferenced.spec	Fixture for unreferenced subpackage definitions.
internal/rpm/spec/testdata/specs/straddling-wrapper.spec	Fixture for sections straddling wrappers.
internal/rpm/spec/testdata/specs/script-section-tag-shaped.spec	Fixture for script and tag-shaped sections.
internal/rpm/spec/testdata/specs/nested-wrappers.spec	Fixture for nested conditional wrappers.
internal/rpm/spec/testdata/specs/multi-package-mixed.spec	Fixture for mixed multi-package content.
internal/rpm/spec/testdata/specs/macro-with-parameters.spec	Fixture for parameterized macros.
internal/rpm/spec/testdata/specs/macro-continuation.spec	Fixture for macro continuations.
internal/rpm/spec/testdata/specs/macro-conditional.spec	Fixture for macro conditionals.
internal/rpm/spec/testdata/specs/if-with-continuation.spec	Fixture for conditional continuations.
internal/rpm/spec/testdata/specs/elif-with-sections.spec	Fixture for sections within elif branches.
internal/rpm/spec/testdata/specs/elif-chain.spec	Fixture for elif chains.
internal/rpm/spec/testdata/specs/comment-only-conditional.spec	Fixture for comment-only conditionals.
internal/rpm/spec/testdata_test.go	Structural fixture edit tests.
internal/rpm/spec/structural_tree_api.go	Tree accessors and mutations; section-name matching needs to remain case-insensitive (moderate, 2 votes).
internal/rpm/spec/structural_tree_api_internal_test.go	Structural tree API tests.
internal/rpm/spec/structural_spec.go	Structural spec representation.
internal/rpm/spec/structural_edit.go	Structural editing operations; replacement-driven continuation changes can leave classification stale (moderate, 1 vote).
internal/rpm/spec/spec_test.go	Spec behavior tests.
internal/rpm/spec/legacy_spec.go	Preserved legacy spec implementation.
internal/rpm/spec/legacy_edit.go	Preserved legacy editing operations.
internal/rpm/spec/editor.go	Public facade and editor selection; exported facade methods need documentation (nit, 1 vote).
internal/rpm/spec/edit_test.go	Editor behavior tests; retain default-mode mutation coverage (moderate, 1 vote).
internal/app/azldev/core/sources/upstream_provenance.go	Editor propagation for provenance handling.
internal/app/azldev/core/sources/upstream_provenance_internal_test.go	Provenance parser tests.
internal/app/azldev/core/sources/sourceprep.go	Spec editor preparer option; option documentation is misplaced (nit, 2 votes).
internal/app/azldev/core/sources/release.go	Release tag editor integration; preserve empty-value handling (moderate, 2 votes).
internal/app/azldev/core/sources/release_test.go	Release parsing tests.
internal/app/azldev/core/sources/release_internal_test.go	Release bump tests.
internal/app/azldev/core/sources/overlays.go	Overlay editor propagation.
internal/app/azldev/core/sources/overlays_test.go	Sequential overlay tests.

Review details

Suppressed comments (3)

internal/rpm/spec/edit_test.go:283

• These cases now explicitly open EditorStructural, so the edit tables that previously exercised the default editor no longer verify the preserved legacy implementation. Production still defaults to legacy, and the remaining default-mode tests cover visitors/read-only behavior rather than these mutations; retain default-mode coverage or run the mutation tables against both editor modes.

			specFile, err := spec.OpenSpec(strings.NewReader(test.input), spec.WithEditor(spec.EditorStructural))

internal/rpm/spec/editor.go:116

• The new public Spec facade methods (starting with ReplaceLine/RemoveLine) have no name-prefixed documentation; the comments on the old legacy implementations no longer document these declarations. Because the repository enables revive's exported rule (.golangci.yml:147-152), this file will fail the configured lint and the public API loses its method contracts. Add concise doc comments to each exported facade method.

func (s *Spec) ReplaceLine(lineNumber int, replacement string) {
	s.editor.ReplaceLine(lineNumber, replacement)
}
func (s *Spec) RemoveLine(lineNumber int)  { s.editor.RemoveLine(lineNumber) }
func (s *Spec) RemoveLines(start, end int) { s.editor.RemoveLines(start, end) }
func (s *Spec) InsertLinesAt(lines []string, lineNumber int) {
	s.editor.InsertLinesAt(lines, lineNumber)
}

internal/rpm/spec/structural_edit.go:604

• headerAt and the macro/conditional state are derived from the original lines, even though replacements are written back to lines before the next iteration. If a replacement adds or removes the trailing \\ on a %define/%global line, subsequent %package or %if lines change between macro text and structural content, but this walk still uses the old classification; section-scoped replacements can then target the wrong section or be skipped. Recompute classification from the updated lines (or explicitly reject replacements that change continuation structure).

	headers := findSectionHeaderLines(lines)
	headerAt := make(map[int]bool, len(headers))

• Files reviewed: 31/31 changed files
• Comments generated: 3
• 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 lint failures and behavior inconsistencies.

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

Review details

Suppressed comments (7)

internal/app/azldev/core/sources/release.go:48

• GetLastTag reports success for a syntactically present but empty Release: line, whereas this helper previously rejected empty values. That now lets an invalid empty release pass this API and changes callers' error classification; retain an explicit empty-value check after the lookup.

	releaseValue, err := openedSpec.GetLastTag("", "Release")

internal/app/azldev/core/sources/sourceprep.go:109

• Please move the WithSkipLookaside documentation above WithSkipLookaside and add a WithSpecEditor comment here. As written, Go documentation attributes the lookaside behavior to WithSpecEditor, while WithSkipLookaside has no doc comment, so the new editor option is undocumented and the existing option is misdocumented.

func WithSpecEditor(mode spec.EditorMode) PreparerOption {

internal/rpm/spec/structural_edit.go:37

• The public UpdateExistingTag contract still says the first matching tag is updated (and the legacy implementation does that), but structural mode replaces every match. The same Spec.UpdateExistingTag/SetTag call therefore has different results solely based on WithEditor, for example when a tag appears in conditional branches; either preserve first-match semantics or explicitly change and document the facade contract and legacy behavior together.

// UpdateExistingTag replaces every instance of the named tag in the given
// package with the provided value. If no such tag exists, it returns an error.
func (s *structuralSpec) UpdateExistingTag(packageName string, tag string, value string) (err error) {

internal/rpm/spec/structural_edit.go:147

• When VisitAllLines returns a visitor error, it has already flushed buffered mutations into root, but this early return skips serializing root back into s.rawLines; mutations made before the error are silently lost, unlike legacy VisitTags. Serialize and commit the tree before returning err.

	if err != nil {
		return err
	}

	lines := serializeTree(root)
	if _, err := parseTree(lines); err != nil {
		return fmt.Errorf("validating mutated spec tree:\n%w", err)

internal/rpm/spec/structural_edit.go:137

• line.lineNumber is captured before callbacks run, but structural Context.InsertLinesBefore, InsertLinesAfter, and RemoveLine only buffer changes and do not adjust later handles' line numbers. After a visitor inserts or removes a tag, subsequent callbacks receive an incorrect CurrentLineNum (and callers using it can target the wrong physical line), unlike the legacy visitor contract. Maintain the offset during the structural walk.

		return visitor(&TagLine{Tag: tag, Value: value}, &Context{
			Target: VisitTarget{
				TargetType: SectionLineTarget,
				Line:       &Line{Text: line.Text, Parsed: &TagLine{Tag: tag, Value: value}},
			},
			RawLine:        &rawLine,
			CurrentLineNum: line.lineNumber,
			CurrentSection: SectionTarget{
				SectName: sectionName,
				SectType: PackageSection,
				Package:  packageName,
			},

internal/rpm/spec/structural_edit.go:268

• These comment fragments are detached from any declaration and sit immediately before InsertTag, so the tag-family and conditional helper documentation is misleadingly separated from its helpers and interrupts the public method documentation. Move each fragment to the corresponding helper or remove it.

// For example, "Source9999" returns "source", "Patch100" returns "patch", and
// "BuildRequires" returns "buildrequires". The result is always lowercased.

// -1 for %endif, and 0 for everything else. Comments are ignored.
//

internal/rpm/spec/structural_tree_api.go:420

• isSectionHeaderLine and getSectionNameAndPackageFromHeader accept section directives case-insensitively, but this exact comparison makes tags under an uppercase %PACKAGE header invisible to structural tag operations such as VisitTags, GetTag, and patch scans. Use a case-insensitive comparison here so the structural editor matches its own parser and the legacy editor.

	return secName == "" || secName == packageSectionName

• Files reviewed: 31/31 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Four unresolved review findings remain, including a critical testpackage lint issue and structural mutation/changelog correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

internal/rpm/spec/legacy_spec.go:126

• The structural mutation branches only queue the edit, but Context documents that insert/remove methods update traversal state and line numbers. In a structural VisitTags walk, removing or inserting before an earlier tag leaves later callbacks with stale CurrentLineNum values, unlike legacy mode; account for queued edits in the walker or explicitly preserve and test a different contract.
  internal/rpm/spec/structural_edit.go:705
• This uses only the first %changelog block, while the legacy visitor inserts the entry into every matching section and the other structural section edits handle repeated conditional sections. A %if/%else spec with changelog blocks would leave one branch without the new entry; iterate over tree.Sections("%changelog", "") instead.

		sect := tree.Section("%changelog", "")
		if sect == nil {
			return errors.New("existing changelog section could not be found")
		}

• Files reviewed: 31/31 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

internal/rpm/spec/structural_edit.go:582

• ReplaceAllLiteralString can return replacement text containing \n (the overlay tests already use a multiline replacement), but this keeps that text as one rawLines element. A later structural operation reparses each element as one physical line, so a balanced replacement such as %if 0\n...\n%endif is seen as an unmatched %if in the same opened Spec, even though the serialized/reopened file parses correctly. Split multiline replacements into physical-line entries before storing them and add a regression test for a balanced multiline replacement followed by a tree edit.

	updatedLines := slices.Clone(s.rawLines)
	updated := searchReplaceLines(updatedLines, sectionName, packageName, compiledRegex, replacement)

• Files reviewed: 31/31 changed files
• Comments generated: 2
• Review effort level: Lite