refactor(spec): add lossless structural parser and tree API

[Thien Trung Vuong (trungams)]

Summary

RPM spec files are only loosely structured, and the existing line-oriented editor has to infer section boundaries after the fact. That becomes fragile around nested conditionals, multiline macros, and sections that begin or end inside %if blocks.

This PR adds the private foundation for a structural editor. It parses a spec into a lossless tree of sections, conditionals, text, and macro definitions, and provides transactional query and mutation primitives over that tree. Parsing and serializing a valid spec preserves its contents byte for byte.

Nothing selects this parser in production yet, and the public overlay configuration is unchanged.

Motivation

Issue #214 collects several cases where line ranges are not enough to preserve section and conditional boundaries safely. A structural representation lets later changes reason about those boundaries directly without trying to evaluate RPM conditions or expand macros.

Changes

• Parse %if/%elif/%else/%endif, including nested and empty branches.
• Keep directive-shaped text inside multiline %define and %global bodies opaque.
• Preserve backslash continuations, brace-delimited macros, Lua bodies, shell ${...} expressions, comments, blank lines, and original ordering.
• Serialize valid input byte-for-byte.
• Reject malformed structures instead of panicking or silently truncating them.
• Add the private structuralSpec, specTree, and section handles under their final filenames and types.
• Add transactional parse/inspect/mutate helpers and section query primitives.

Validation

• mage build
• mage unit

The cumulative stack was mechanically rebased onto eb9fb3f and revalidated after the rebase.

Known limitations

The parser deliberately does not evaluate RPM macros or conditional expressions. Sections created dynamically through macro expansion are therefore not visible to it. Public editor selection and actual overlay integration arrive in later PRs in the stack.

[Copilot]

🔵 Needs a closer look

The PR adds a substantial new parsing subsystem with complex lexical/state handling, so it warrants final human review despite strong test coverage and only minor review findings.

Pull request overview

Introduces a private, lossless structural RPM spec parser that builds a block tree (sections, conditionals, text, macro definitions) and provides a transactional tree API for querying and performing safe structural edits while preserving byte-for-byte serialization.

Changes:

• Added a two-pass structural parser and serializer to round-trip spec input without altering whitespace/comments.
• Added internal specTree / sectionHandle APIs for section queries and transactional mutation with post-mutation validation.
• Added unit tests covering conditional nesting/branches, macro continuation opacity (including Lua/expand bodies), and removal validation invariants.

File summaries

File	Description
internal/rpm/spec/tree.go	Implements the structural parse/serialize logic and macro/conditional/section scanning helpers.
internal/rpm/spec/tree_test.go	Adds round-trip and malformed-input tests for the structural parser, plus macro edge cases.
internal/rpm/spec/tree_raw_braces_test.go	Adds regression coverage for raw-brace shell fragments inside %{expand: ...} macro bodies.
internal/rpm/spec/structural_tree_api.go	Adds internal tree query/mutation primitives (sections, append/prepend lines, remove sections).
internal/rpm/spec/structural_tree_api_internal_test.go	Adds transactional semantics tests and removal-safety validation tests for the tree API.
internal/rpm/spec/structural_spec.go	Introduces the private structuralSpec wrapper holding raw spec lines.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• 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]

🔵 Needs a closer look

The change introduces a substantial new parsing subsystem whose correctness depends on many subtle edge cases, warranting careful human review despite good test coverage.

Review details

Suppressed comments (2)

internal/rpm/spec/structural_tree_api.go:232

• Same as above: this can refer to a '%elif…' header as an “%if block”. Using a neutral term like “conditional block” would make the error accurate for both %if and %elif nodes.

		if wouldEmptySectionWrapper(child, removeSet) && index+1 < len(children) {
			next := children[index+1]
			if next.Kind == conditionalBlock && !containsSectionBlocks(next) && conditionalHasTextOrMacroContent(next) {
				return fmt.Errorf("content in %%if block at %#q would be orphaned after removing the preceding section:\n%w",
					next.Header, ErrConditionalSpansSections)
			}

internal/rpm/spec/tree.go:232

• The comment says this function respects general backslash continuations, but the implementation only skips directive-shaped lines inside multi-line '%define'/'%global' bodies (and intentionally does not treat ordinary '\' continuations as structural suppression per parseTree's doc comment). This is misleading for future maintainers.

// findSectionHeaderLines returns the 0-indexed line numbers of all section headers,
// respecting line continuations (backslash-terminated lines suppress the next line).

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

[Copilot]

🟡 Changes recommended

Conditional branch validation and section-removal safety issues remain unresolved.

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

Review details

Suppressed comments (2)

internal/rpm/spec/structural_tree_api.go:235

• When the last section in a wrapper is removed, this safety check only examines a following conditional block. For input such as %if 1, %package one, %endif, followed by a non-empty text or macro block before the next section, removal succeeds and leaves content whose RPM section ownership is ambiguous (and can leak the removed section's tags or commands). Reject any non-empty non-section siblings in the straddling area, not only conditional siblings, or otherwise preserve them transactionally.

		if wouldEmptySectionWrapper(child, removeSet) && index+1 < len(children) {
			next := children[index+1]
			if next.Kind == conditionalBlock && !containsSectionBlocks(next) && conditionalHasTextOrMacroContent(next) {
				return fmt.Errorf("content in conditional block at %#q would be orphaned after removing the preceding section:\n%w",
					next.Header, ErrConditionalSpansSections)

internal/rpm/spec/tree.go:338

• This parser delegates opener detection to conditionalDepthChange, which recognizes only %if, %ifarch, %ifnarch, %ifos, and %ifnos. Valid RPM conditionals such as %ifmacro, %ifnmacro, %ifexist, and %ifnot are therefore treated as ordinary text; a section inside one is lifted to the surrounding tree, so later section edits can move content outside its conditional. Extend the shared conditional matcher (and branch matcher as needed) before using it to build the tree.

		if conditionalDepthChange(line) == 1 {

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

[Copilot]

🟡 Changes recommended

Three moderate parser/API issues and one test-package lint issue remain unresolved.

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

Review details

Suppressed comments (2)

internal/rpm/spec/structural_tree_api_internal_test.go:4

• This same-package *_internal_test.go file accesses unexported parser types but lacks the repository-required //nolint:testpackage directive. testpackage is enabled in .golangci.yml, and the other internal tests in this PR already opt out, so the quality checks will flag this file.

package spec

internal/rpm/spec/tree.go:815

• When a nested RPM expansion such as %{version} is encountered, this advances only past %, so the following { is processed again as a Lua raw brace. The lua raw braces strings and expansions fixture in tree_test.go then leaves one brace outstanding and parseTree reports the otherwise valid macro as unterminated. Advance past both opener bytes here.

		case text[idx] == '%' && idx+1 < len(text) && text[idx+1] == '{':
			state.nestedRPM++
			idx++

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

[Copilot]

🟡 Changes recommended

Two moderate correctness issues and one required test-lint fix remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

internal/rpm/spec/structural_tree_api.go:102

• The synthetic preamble is represented as a sectionBlock with an empty Name and Package (tree.go:213-225). Because this predicate does not exclude that block, SectionsByPackage("") returns the preamble along with main-package sections, and passing that result to RemoveSections always fails with cannot remove the global/preamble section; filter out the synthetic preamble (callers can still request it explicitly with Sections("", "")).

	walkBlocks(t.root, func(blk *block) bool {
		if blk.Kind == sectionBlock && blk.Package == pkg {
			matches = append(matches, &sectionHandle{block: blk, tree: t})

internal/rpm/spec/tree.go:819

• The nested RPM expansion increments nestedRPM but advances only past %, so the following { is also counted as a Lua raw brace. Its closing } decrements only nestedRPM, leaving an extra brace open; inputs such as the new local value = { nested = %{version}, ... } fixture therefore keep the outer Lua macro unterminated and make parseTree return an error. Consume the complete %{ opener (or otherwise balance the two counters).

		case text[idx] == '%' && idx+1 < len(text) && text[idx+1] == '{':
			state.nestedRPM++
			idx++

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

[Copilot]

🟡 Changes recommended

Four unresolved moderate findings remain in the parser and section query API.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

internal/rpm/spec/tree.go:819

• When a Lua body contains a nested RPM expansion such as %{version}, this branch advances only past %, so the following { is counted as a raw Lua brace. The matching } decrements nestedRPM but leaves that extra brace counted, preventing the outer Lua expansion from closing; the existing lua raw braces strings and expansions round-trip test therefore reaches parseMacroDefBlock's unterminated-macro error. Skip both opener bytes here so the nested expansion is tracked only by nestedRPM.

		case text[idx] == '%' && idx+1 < len(text) && text[idx+1] == '{':
			state.nestedRPM++
			idx++

internal/rpm/spec/tree.go:634

• shellBraces is tracked by macroStateAfter, but open() ignores it. If a %define/%global body contains an unclosed ${...} across physical lines, parsing resumes on the next line and can interpret %if/%package there as real structure, creating phantom conditionals or sections. Include this state when deciding whether the macro body continues (and add a multiline shell-expression regression test).

	return state.depth > 0 || state.escapedBraces > 0 || state.lua != nil

internal/rpm/spec/tree.go:819

• This treats every %{ in a Lua body as a nested RPM expansion, including escaped %%{. The parser already treats %%{ as a literal opener in regular expand bodies; with a valid body such as %global helper %{lua:\nprint("%%{")\n}\n%build, nestedRPM consumes the real outer } and the macro is reported as unterminated. Count the percent run here and only enter nestedRPM for an odd, live %{ opener.

		case text[idx] == '%' && idx+1 < len(text) && text[idx+1] == '{':
			state.nestedRPM++
			idx++

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

[Antonio Salinas (Tonisal-byte)]

Some comments that might be helpful to touch upon but not required

[Antonio Salinas (Tonisal-byte)]

Non-blocking: HasSection and Sections compare section names exactly, while isSectionHeaderLine recognizes them case-insensitively and getSectionNameAndPackageFromHeader stores the original spelling.

[Antonio Salinas (Tonisal-byte)]

Non-blocking: The parser treats directive-shaped lines as structure unless they occur inside %define or %global. Consequently, valid but uncommon standalone multiline %{lua:...} or %{expand:...} blocks containing %if or section-shaped text can be rejected or misclassified.