fix(overlays): allocate macro source tag to avoid collisions

[Nan Liu (liunan-ms)]

Fixes #235

The problem

The macro-load overlay hard-coded its source as Source9999. When a spec already defines that tag (e.g. texlive, whose sources go up to Source10133), the injected tag collided:

spectool failed: error: source 9999 defined multiple times

The fix

Allocate the macros source tag dynamically: keep the conventional Source9999 when that number is free (so already-rendered specs stay byte-identical), and fall back to the next number after the highest existing source tag only when Source9999 is already taken.

Why prefer Source9999 instead of always highest+1?

azldev has historically emitted the macros file as Source9999, and the rendered specs committed in microsoft/azurelinux reflect that. Always allocating highest+1 would change the tag number for every macros-bearing spec (e.g. a spec whose highest source is Source1 would get Source2), making render non-idempotent against the committed trees and breaking the end-to-end idempotency test. Preferring Source9999 keeps existing output stable while still resolving the collision for the rare specs that already use Source9999.

Example: texlive

texlive's spec already uses Source9999 (and sources up to Source10133):

Before: injected a second Source9999: texlive.azl.macros → collides → build fails.

After: AddSourceEntry sees Source9999 is occupied, finds highest = 10133, and inserts:

Source10134: texlive.azl.macros

For an ordinary spec where Source9999 is free, it stays Source9999 (unchanged from today's output).

Notes

• Adds Spec.AddSourceEntry and ParseSourceTagNumber (rejecting negative suffixes), mirroring the existing AddPatchEntry / GetHighestPatchTagNumber logic. Bare Source: tags count as auto-numbered slots.
• The macros load directive and source registration are returned as separate overlays so the source registration is applied after user overlays, ensuring it accounts for any sources the user added before choosing a number.
• The synthesized internal-source-add overlay is applied at source-prep time, so it has no component fingerprint impact.

[Copilot]

🟡 Changes recommended

The updated source-prep tests currently assert a specific Source0 tag, which is brittle and contradicts the new “next free SourceN” allocation behavior.

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

Pull request overview

This PR fixes a spec-overlay collision where the macros overlay previously injected a hard-coded Source9999 entry that could already exist in some specs (e.g., texlive). It introduces dynamic allocation of the next available SourceN tag and adjusts overlay ordering so source registration happens after user overlays.

Changes:

• Add Spec.AddSourceEntry and ParseSourceTagNumber to allocate the next free SourceN based on existing Source* and bare Source: tags.
• Split synthesized macros overlays into (1) load directive and (2) source registration, and apply source registration after user overlays.
• Update/extend unit tests to validate collision avoidance and new allocation behavior.

File summaries

File	Description
internal/rpm/spec/edit.go	Adds ParseSourceTagNumber and Spec.AddSourceEntry to allocate a collision-free SourceN tag.
internal/rpm/spec/edit_test.go	Adds unit tests covering AddSourceEntry and ParseSourceTagNumber, including large/boundary cases.
internal/app/azldev/core/sources/sourceprep.go	Reorders overlays and splits macros overlays so source registration is applied after user overlays.
internal/app/azldev/core/sources/sourceprep_test.go	Updates tests and adds a collision-focused test for the new source allocation behavior.
internal/app/azldev/core/sources/overlays.go	Introduces an internal overlay type (internal-source-add) and wires it to Spec.AddSourceEntry.
internal/app/azldev/core/sources/overlays_test.go	Adds a test case validating internal-source-add avoids an occupied Source9999.

Review details

Suppressed comments (1)

internal/app/azldev/core/sources/sourceprep_test.go:434

• This test hard-codes the expected source tag as "Source0", but the new allocation logic intentionally chooses the next free SourceN based on what the spec already contains. To avoid a brittle test (and to better match the behavior being validated), assert that the macros filename is present as a Source value rather than expecting a specific tag number.

	// Verify spec has macro load directive and a collision-free source tag.
	specPath := filepath.Join(testOutputDir, "my-package.spec")
	specContents, err := fileutils.ReadFile(ctx.FS(), specPath)
	require.NoError(t, err)

	specStr := string(specContents)
	assert.Contains(t, specStr, "%{load:%{_sourcedir}/my-package"+sources.MacrosFileExtension+"}")
	assert.Contains(t, specStr, "Source0: my-package"+sources.MacrosFileExtension)
}

• Files reviewed: 6/6 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]

🟢 Approval recommended

The implementation matches the stated collision-avoidance strategy, preserves overlay ordering requirements, and includes targeted regression tests for the texlive-style collision scenario.

Review details

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

[Copilot]

🟡 Changes recommended

ParseSourceTagNumber currently treats negative suffixes (e.g. Source-1) as valid, which is not a valid RPM SourceN tag and should be rejected to avoid surprising downstream behavior.

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

Review details

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

[Copilot]

🟢 Approval recommended

The change replaces a fixed, collision-prone tag with deterministic “next free SourceN” allocation and updates overlay ordering accordingly, with targeted tests covering key cases.

Review details

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

[Copilot]

🟡 Changes recommended

One updated test assertion no longer validates the intended behavior (no macros source injection), and the implementation/PR description disagree on whether Source9999 is still preferred when free.

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

Review details

Suppressed comments (1)

internal/app/azldev/core/sources/sourceprep_test.go:103

• The no-macros path is supposed to assert that no macros source entry is injected into the spec, but the current check assert.NotContains(..., "Source0") is unrelated (a real spec can legitimately contain Source0 even when no macros are generated). This makes the test unable to catch regressions where a macros SourceN line is mistakenly added.

	specContents, err := fileutils.ReadFile(ctx.FS(), outputSpecPath)
	require.NoError(t, err)
	assert.NotContains(t, string(specContents), "%{load:%{_sourcedir}/"+macrosFileName+"}")
	assert.NotContains(t, string(specContents), "Source0")
}

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

[Copilot]

🔵 Needs a closer look

The implemented AddSourceEntry behavior and tests conflict with the PR description/examples, and one updated test assertion is too weak to validate the intended “no source injected” behavior.

Review details

Suppressed comments (2)

internal/rpm/spec/edit.go:656

• The new [Spec.AddSourceEntry] behavior and its comment prefer allocating 'Source9999' when free, but the PR description and examples say the macros source tag should be allocated as the next number after the highest existing source tag (e.g. a spec with only 'Source0' should get 'Source1', and a spec with no sources should get 'Source0'). As-is, this will produce 'Source9999' in those cases, so either the implementation/tests need to change to always allocate highest+1, or the PR description needs to be updated to match the intended (backward-compatible) behavior.

// AddSourceEntry registers a source in the spec. It prefers the conventional high slot
// (Source9999) when that number is free, matching azldev's historical output so already-rendered
// specs stay byte-identical (idempotent). When Source9999 is already taken, it falls back to the
// next number after the highest existing source tag. Automatically numbered bare Source tags are
// included when determining occupancy and the highest number.

internal/app/azldev/core/sources/sourceprep_test.go:103

• This assertion only checks that the spec does not contain the literal string 'Source0', which doesn’t actually validate the intended behavior (no source entry should be injected when no macros file is generated). It would still pass even if a different SourceN tag were accidentally added.

	specContents, err := fileutils.ReadFile(ctx.FS(), outputSpecPath)
	require.NoError(t, err)
	assert.NotContains(t, string(specContents), "%{load:%{_sourcedir}/"+macrosFileName+"}")
	assert.NotContains(t, string(specContents), "Source0")
}

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

[Copilot]

🟢 Approval recommended

The allocation logic is consistent with existing tag insertion behavior, addresses the reported collision deterministically, and is covered by targeted unit tests including the texlive-style collision case.

Review details

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