From 910c61e52de69da7047f1ab3695a1206c3789c27 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 25 Aug 2026 17:04:25 +0200 Subject: [PATCH 01/17] =?UTF-8?q?feat(changelog):=20add=20link:=20field=20?= =?UTF-8?q?to=20changelog=20entries=20=E2=80=94=20marker=20read=20side?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `link:` to `ChangelogEntryDto`, `ChangelogEntry` (domain type), `BundledEntry`, and `BundledEntryDto` so the field survives the full YAML → domain → YAML round-trip and the scrub pass. `ChangelogEntry.IsMarker` returns true when `Link` is non-null. A marker is a machine-written object containing only `link: {prNumber}` that redirects a non-primary PR to the parent entry's key; no markers exist in S3 today (the writer lands in Step 8), so this is inert on merge. `ChangelogContentScrubber.ScrubChangelog` explicitly preserves `link:` in both the `BundledEntry` passed to `LinkAllowlistSanitizer` and the `with` expression applied after sanitization. `link:` is never a URL, so no allowlist rule touches it. Tests: serialization round-trip for marker entries; scrub preserves `link:` when prs: are stripped; `IsMarker` false on normal entries. Co-Authored-By: Claude Sonnet 4.6 --- .../ReleaseNotes/Bundle.cs | 1 + .../ReleaseNotes/ChangelogEntry.cs | 6 +- .../ReleaseNotes/ReleaseNotesSerialization.cs | 6 +- .../ReleaseNotes/BundledEntry.cs | 5 +- .../ReleaseNotes/ChangelogEntry.cs | 10 +++ .../Scrubbing/ChangelogContentScrubber.cs | 6 +- .../ChangelogContentScrubberTests.cs | 67 +++++++++++++++++++ .../ReleaseNotesSerializationTests.cs | 38 +++++++++++ 8 files changed, 133 insertions(+), 6 deletions(-) create mode 100644 tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/Bundle.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/Bundle.cs index 7cf3977e62..cbfe9b0116 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/Bundle.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/Bundle.cs @@ -83,6 +83,7 @@ public sealed record BundledEntryDto public string? Pr { get; set; } public List? Prs { get; set; } public List? Issues { get; set; } + public string? Link { get; set; } } /// diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs index 1fa079929c..f1f0571b66 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs @@ -28,7 +28,11 @@ public record ChangelogEntryDto public string? FeatureId { get; set; } public bool? Highlight { get; set; } - /// Bare PR number referencing the canonical entry. Marks this as a pipeline-written marker; must be the only field present. + /// + /// Marker reference: a bare PR number pointing to the authoritative entry in the same pool. + /// A marker carries link: and nothing else; any other field alongside it is invalid. + /// Written by the pipeline for non-primary PRs in a multi-PR entry; never hand-authored. + /// public string? Link { get; set; } } diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs index b780517ac8..bcdde16809 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs @@ -227,7 +227,8 @@ private static string ToYamlDoubleQuotedString(string s) Subtype = ParseEntrySubtype(dto.Subtype), Areas = dto.Areas, Prs = dto.Prs ?? (dto.Pr != null ? [dto.Pr] : null), - Issues = dto.Issues + Issues = dto.Issues, + Link = dto.Link }; private static BundledFile ToBundledFile(BundledFileDto dto) => new() @@ -341,7 +342,8 @@ private static ChangelogEntryType ParseEntryType(string? value) Subtype = EntrySubtypeToString(entry.Subtype), Areas = entry.Areas?.ToList(), Prs = entry.Prs?.ToList(), - Issues = entry.Issues?.ToList() + Issues = entry.Issues?.ToList(), + Link = entry.Link }; private static BundledFileDto ToDto(BundledFile file) => new() diff --git a/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs b/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs index 6801ae8f3d..ffce23ff10 100644 --- a/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs +++ b/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs @@ -48,6 +48,9 @@ public record BundledEntry /// Related issue URLs or references. public IReadOnlyList? Issues { get; init; } - /// Bare PR number referencing the canonical entry. Present only on pipeline-written markers. + /// + /// Marker reference: a bare PR number pointing to the authoritative entry in the same pool. + /// Preserved through the scrub round-trip so the public pool can resolve markers end-to-end. + /// public string? Link { get; init; } } diff --git a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs index 53fd67810e..ada693a49d 100644 --- a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs @@ -49,6 +49,16 @@ public record ChangelogEntry /// Bare PR number referencing the canonical entry. Marks this as a pipeline-written marker; must be the only field present. public string? Link { get; init; } + /// + /// Marker reference: a bare PR number pointing to the authoritative entry in the same pool. + /// Non-null only on machine-written marker objects emitted by the pipeline for non-primary PRs + /// in a multi-PR entry. A marker carries link: and nothing else. + /// + public string? Link { get; init; } + + /// True when this entry is a pipeline-written marker that redirects to another PR's entry. + public bool IsMarker => Link is not null; + /// /// Converts this ChangelogEntry to a BundledEntry for embedding in bundles. /// File property is set to null; set it separately using a 'with' expression. diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 8838724222..6d55964bff 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -98,7 +98,8 @@ private async Task ScrubChangelog(string content, Cancel ctx) Issues = entry.Issues, Areas = entry.Areas, Highlight = entry.Highlight, - Subtype = entry.Subtype + Subtype = entry.Subtype, + Link = entry.Link }; await using var collector = new DiagnosticsCollector([]); @@ -120,7 +121,8 @@ private async Task ScrubChangelog(string content, Cancel ctx) Impact = sanitized.Impact, Action = sanitized.Action, Prs = sanitized.Prs, - Issues = sanitized.Issues + Issues = sanitized.Issues, + Link = entry.Link }; var result = ReleaseNotesSerialization.SerializeEntry(scrubEntry); diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs new file mode 100644 index 0000000000..4a341c0a45 --- /dev/null +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs @@ -0,0 +1,67 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// Elasticsearch B.V licenses this file to you under the Apache 2.0 License. +// See the LICENSE file in the project root for more information + +using AwesomeAssertions; +using Elastic.Changelog.Scrubbing; +using Elastic.Documentation.Configuration.ReleaseNotes; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Elastic.Changelog.Tests.Scrubbing; + +public class ChangelogContentScrubberTests +{ + private static readonly IReadOnlyList AllowAll = ["elastic/elasticsearch"]; + + private static ChangelogContentScrubber Scrubber(IReadOnlyList? allowRepos = null) => + new(NullLoggerFactory.Instance, allowRepos ?? AllowAll); + + [Fact] + public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved() + { + // A marker is link: only — the scrubber must pass it through unchanged. + var yaml = "link: \"12345\"\n"; + var scrubber = Scrubber(); + + var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12346.yaml", yaml, CancellationToken.None); + + var entry = ReleaseNotesSerialization.DeserializeEntry(result); + entry.Link.Should().Be("12345", "link: must survive the scrub round-trip"); + entry.IsMarker.Should().BeTrue(); + } + + [Fact] + public async Task ScrubAsync_EntryWithLinkAndPrivatePrs_LinkPreservedPrsStripped() + { + // A hypothetical entry with both link: and prs: pointing to a private repo. + // link: must survive scrubbing; private prs: must be stripped. + var yaml = + "link: \"12345\"\n" + + "prs:\n" + + " - https://github.com/elastic/private-repo/pull/99\n"; + var scrubber = Scrubber(["elastic/elasticsearch"]); // private-repo not allowed + + var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12346.yaml", yaml, CancellationToken.None); + + var entry = ReleaseNotesSerialization.DeserializeEntry(result); + entry.Link.Should().Be("12345", "link: must survive the scrub even when prs: is stripped"); + entry.Prs.Should().BeNullOrEmpty("private PR reference must be scrubbed"); + } + + [Fact] + public async Task ScrubAsync_NormalEntry_LinkIsNull() + { + var yaml = + "title: Fix search performance\n" + + "type: bug-fix\n" + + "products:\n" + + " - product: elasticsearch\n"; + var scrubber = Scrubber(); + + var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12345.yaml", yaml, CancellationToken.None); + + var entry = ReleaseNotesSerialization.DeserializeEntry(result); + entry.Link.Should().BeNull(); + entry.IsMarker.Should().BeFalse(); + } +} diff --git a/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs b/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs index 91f038a0db..2a1a3352fc 100644 --- a/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs +++ b/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs @@ -150,4 +150,42 @@ public void SerializeEntry_InjectedFieldInTitle_DoesNotPolluteOtherFields() roundTrip.Impact.Should().BeNull(); roundTrip.Action.Should().BeNull(); } + + [Fact] + public void SerializeDeserialize_MarkerEntry_LinkRoundTrips() + { + // A marker is link: only — no title, type, products. + var yaml = "link: \"12345\"\n"; + + var entry = ReleaseNotesSerialization.DeserializeEntry(yaml); + + entry.Link.Should().Be("12345"); + entry.IsMarker.Should().BeTrue(); + entry.Title.Should().Be("", "title is empty when absent"); + entry.Type.Should().Be(ChangelogEntryType.Invalid); + } + + [Fact] + public void SerializeEntry_WithLink_LinkRoundTrips() + { + var entry = new ChangelogEntry { Link = "99999" }; + + var yaml = ReleaseNotesSerialization.SerializeEntry(entry); + var roundTrip = ReleaseNotesSerialization.DeserializeEntry(yaml); + + roundTrip.Link.Should().Be("99999"); + roundTrip.IsMarker.Should().BeTrue(); + } + + [Fact] + public void IsMarker_NullLink_ReturnsFalse() + { + var entry = new ChangelogEntry + { + Title = "A real entry", + Type = ChangelogEntryType.Feature + }; + + entry.IsMarker.Should().BeFalse(); + } } From 7d6f32e2a752ef33c44561f1da760e1dab35f766 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 25 Aug 2026 19:04:01 +0200 Subject: [PATCH 02/17] =?UTF-8?q?fix(changelog):=20marker=20entries=20seri?= =?UTF-8?q?alize=20as=20link-only=20=E2=80=94=20omit=20title=20and=20other?= =?UTF-8?q?=20non-link=20fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ToDto(ChangelogEntry) now short-circuits on IsMarker: only Link is emitted. This enforces the marker contract (link: is the sole field) and prevents 'title: ''' from appearing in round-tripped marker YAML. New test: SerializeEntry_MarkerEntry_YamlContainsOnlyLinkField. Co-Authored-By: Claude Sonnet 4.6 --- .../ReleaseNotes/ReleaseNotesSerialization.cs | 37 +++++++++++-------- .../ReleaseNotes/ChangelogEntry.cs | 3 -- .../ReleaseNotesSerializationTests.cs | 14 +++++++ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs index bcdde16809..236a9cafbd 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs @@ -284,22 +284,29 @@ private static ChangelogEntryType ParseEntryType(string? value) // Reverse mappings (Domain → DTO) for serialization - private static ChangelogEntryDto ToDto(ChangelogEntry entry) => new() + private static ChangelogEntryDto ToDto(ChangelogEntry entry) { - Prs = entry.Prs?.ToList(), - Issues = entry.Issues?.ToList(), - Type = EntryTypeToString(entry.Type), - Subtype = EntrySubtypeToString(entry.Subtype), - Products = entry.Products?.Select(ToDto).ToList(), - Areas = entry.Areas?.ToList(), - Title = entry.Title, - Description = entry.Description, - Impact = entry.Impact, - Action = entry.Action, - FeatureId = entry.FeatureId, - Highlight = entry.Highlight, - Link = entry.Link - }; + // Marker entries are link-only; emitting any other field would violate the marker contract. + if (entry.IsMarker) + return new ChangelogEntryDto { Link = entry.Link }; + + return new ChangelogEntryDto + { + Prs = entry.Prs?.ToList(), + Issues = entry.Issues?.ToList(), + Type = EntryTypeToString(entry.Type), + Subtype = EntrySubtypeToString(entry.Subtype), + Products = entry.Products?.Select(ToDto).ToList(), + Areas = entry.Areas?.ToList(), + Title = entry.Title, + Description = entry.Description, + Impact = entry.Impact, + Action = entry.Action, + FeatureId = entry.FeatureId, + Highlight = entry.Highlight, + Link = entry.Link + }; + } private static ProductInfoDto ToDto(ProductReference product) => new() { diff --git a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs index ada693a49d..25062776c6 100644 --- a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs @@ -46,9 +46,6 @@ public record ChangelogEntry /// Whether this entry should be highlighted. public bool? Highlight { get; init; } - /// Bare PR number referencing the canonical entry. Marks this as a pipeline-written marker; must be the only field present. - public string? Link { get; init; } - /// /// Marker reference: a bare PR number pointing to the authoritative entry in the same pool. /// Non-null only on machine-written marker objects emitted by the pipeline for non-primary PRs diff --git a/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs b/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs index 2a1a3352fc..f8a88914c1 100644 --- a/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs +++ b/tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs @@ -177,6 +177,20 @@ public void SerializeEntry_WithLink_LinkRoundTrips() roundTrip.IsMarker.Should().BeTrue(); } + [Fact] + public void SerializeEntry_MarkerEntry_YamlContainsOnlyLinkField() + { + // Marker entries must serialize as link: only — no title, type, products, etc. + var entry = new ChangelogEntry { Link = "12345" }; + + var yaml = ReleaseNotesSerialization.SerializeEntry(entry); + + yaml.Should().Contain("link:"); + yaml.Should().NotContain("title:"); + yaml.Should().NotContain("type:"); + yaml.Should().NotContain("products:"); + } + [Fact] public void IsMarker_NullLink_ReturnsFalse() { From bcec2619707bbd1eadd3517461134c76ca1e28eb Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 11:25:31 +0200 Subject: [PATCH 03/17] fix(changelog): probe-based CDN sourcing replaces dead pool-registry read (#3931) Co-authored-by: Claude Sonnet 4.6 --- .../ReleaseNotes/CdnChangelogEntryFetcher.cs | 48 ++++ .../Bundling/ChangelogBundlingService.cs | 133 +++++++++- .../Changelogs/BundleCdnSourcingTests.cs | 247 ++++++++++++++---- 3 files changed, 367 insertions(+), 61 deletions(-) diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs index 9e69933f21..329bf936dd 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs @@ -291,6 +291,54 @@ public async Task> FetchAsync( return (false, await response.Content.ReadAsStringAsync(ctx).ConfigureAwait(false)); } + /// + /// Probes for a PR's changelog entry by number. Returns null when the entry does not exist (404); + /// this is the normal "no changelog for this PR" case and is not an error. Transient failures + /// (5xx, timeouts, transport errors) are retried with the same budget as other fetches. + /// Unlike , a 404 here is authoritative-null, not an error. + /// + public async Task FetchPrEntryAsync( + Uri baseUri, string org, string repo, string branch, int prNumber, Cancel ctx) + { + var fileName = $"{prNumber}.yaml"; + var poolSegments = ChangelogKeys.PoolSegments(org, repo, branch); + var uri = CombineSegments(baseUri, [.. poolSegments, fileName]); + var poolLabel = $"{org}/{repo}/{branch}"; + var (fetched, content, _) = await TryProbeEntryAsync(uri, fileName, poolLabel, ctx).ConfigureAwait(false); + return fetched ? new CdnChangelogEntry(fileName, content) : null; + } + + private async Task<(bool Fetched, string Content, string? LastError)> TryProbeEntryAsync(Uri uri, string fileName, string poolLabel, Cancel ctx) + { + string? lastError = null; + for (var attempt = 1; attempt <= _maxAttempts; attempt++) + { + ctx.ThrowIfCancellationRequested(); + try + { + var (notFound, content) = await FetchTextOrNotFoundAsync(uri, attempt, ctx).ConfigureAwait(false); + if (notFound) + return (false, string.Empty, null); // 404 = probe miss, not an error + if (attempt > 1) + _logger.LogInformation("Probed changelog entry '{File}' for {Pool} on attempt {Attempt}/{Max}", fileName, poolLabel, attempt, _maxAttempts); + return (true, content, null); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + if (ex is HttpRequestException { StatusCode: >= HttpStatusCode.BadRequest and < HttpStatusCode.InternalServerError }) + return (false, string.Empty, ex.Message); + lastError = ex.Message; + if (attempt >= _maxAttempts) + break; + var delay = RetryDelay(attempt); + _logger.LogDebug("Probe for changelog entry '{File}' for {Pool} failed (attempt {Attempt}/{Max}: {Error}); retrying in {Delay}", + fileName, poolLabel, attempt, _maxAttempts, ex.Message, delay); + await _sleep(delay, ctx).ConfigureAwait(false); + } + } + return (false, string.Empty, lastError); + } + /// /// Fetches a single entry, retrying transient failures (most importantly a not-yet-propagated 404) /// up to times with exponential backoff. Retry requests are cache-busted diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs index 3e9dcea447..48503454cb 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs @@ -289,11 +289,11 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle if (!ValidateInput(collector, input, requireDirectoryExists: !useCdn)) return false; - // --all and --input-products require reading every entry body; that is only possible locally. - // On the CDN path entries are probed by key, so there is nothing to enumerate without a PR list. - if (useCdn && (input.All || input.InputProducts is { Count: > 0 })) + // --all, --input-products, and --issues require reading every entry body; that is only possible locally. + // On the CDN path entries are probed by key (one per PR), so there is nothing to enumerate without a PR list. + if (useCdn && (input.All || input.InputProducts is { Count: > 0 } || input.Issues is { Length: > 0 })) { - var flag = input.All ? "--all" : "--input-products"; + var flag = input.All ? "--all" : input.Issues is { Length: > 0 } ? "--issues" : "--input-products"; collector.EmitError(string.Empty, $"{flag} is not supported when sourcing changelog entries from the CDN, because entries are fetched by key (one per PR) and there is no pool enumeration. " + "Pass --force-local or --directory to bundle from a local checkout instead."); @@ -380,12 +380,16 @@ public async Task BundleChangelogs(IDiagnosticsCollector collector, Bundle } else { - // --prs / --issues on the CDN path: still uses the pool registry for now (Step 9 will - // switch this to per-PR probing once canonical keys and markers are in place). - var contents = await FetchCdnEntriesAsync(collector, authoringOwner, authoringRepo, authoringBranch, ctx); - if (contents == null) + // --prs / --report / --release-version on the CDN path: probe one key per PR number. + // Each entry lives at changelog/{org}/{repo}/{branch}/{pr}.yaml; 404 = no entry for that PR. + var probed = await FetchCdnProbedEntriesAsync(collector, authoringOwner, authoringRepo, authoringBranch, prsToMatch, ctx); + if (probed == null) return false; - matchResult = entryMatcher.MatchChangelogContents(collector, contents, filterCriteria, ctx); + _logger.LogInformation("Probed {Count} changelog entry(ies) for {Pool} from CDN", + probed.Count, $"{authoringOwner}/{authoringRepo}/{authoringBranch}"); + // Entries are already selected by probe; pass IncludeAll so the matcher skips content re-filtering. + var probeCriteria = filterCriteria with { IncludeAll = true }; + matchResult = entryMatcher.MatchChangelogContents(collector, probed, probeCriteria, ctx); } } else @@ -1324,6 +1328,117 @@ private BundleChangelogsArguments ApplyConfigDefaults(BundleChangelogsArguments return combined; } + private async Task?> FetchCdnProbedEntriesAsync( + IDiagnosticsCollector collector, + string? org, + string? repo, + string? branch, + HashSet prsToMatch, + Cancel ctx) + { + if (string.IsNullOrWhiteSpace(repo)) + { + collector.EmitError(string.Empty, + "Sourcing changelog entries from the CDN requires a resolvable authoring repository. " + + "Set bundle.repo in changelog.yml (or pass --repo), or pass --force-local / --directory to bundle local changelog files."); + return null; + } + + var resolvedOrg = string.IsNullOrWhiteSpace(org) ? DefaultOwner : org; + var resolvedBranch = string.IsNullOrWhiteSpace(branch) ? DefaultBranch : branch; + var poolLabel = $"{resolvedOrg}/{repo}/{resolvedBranch}"; + + var baseUri = ChangelogCdn.ResolveBaseUri(); + if (baseUri is null) + { + collector.EmitError(string.Empty, + $"No valid changelog CDN base URL is configured. Set the {ChangelogCdn.BaseUrlEnvironmentVariable} environment variable to an absolute http(s) URL."); + return null; + } + + var prNumbers = new List(); + foreach (var pr in prsToMatch) + { + if (TryExtractPrNumber(pr, resolvedOrg, repo, out var prNumber)) + prNumbers.Add(prNumber); + } + + if (prNumbers.Count == 0) + { + _logger.LogInformation("No PR numbers for {Pool} found in filter; returning empty probe result", poolLabel); + return []; + } + + var tasks = prNumbers.Select(pr => + _entryFetcher.FetchPrEntryAsync(baseUri, resolvedOrg, repo, resolvedBranch, pr, ctx)).ToArray(); + var results = await Task.WhenAll(tasks).ConfigureAwait(false); + + var entries = new Dictionary(StringComparer.OrdinalIgnoreCase); + var hasError = false; + + for (var i = 0; i < prNumbers.Count; i++) + { + var prNumber = prNumbers[i]; + var cdnEntry = results[i]; + + if (cdnEntry is null) + { + _logger.LogWarning("No changelog entry found for PR {PrNumber} in {Pool}", prNumber, poolLabel); + continue; + } + + var entry = ReleaseNotesSerialization.DeserializeEntry(cdnEntry.Value.Content); + if (!entry.IsMarker) + { + _ = entries.TryAdd(cdnEntry.Value.FileName, cdnEntry.Value.Content); + continue; + } + + if (!int.TryParse(entry.Link, out var parentPr) || parentPr <= 0) + { + collector.EmitError(string.Empty, + $"Changelog entry '{cdnEntry.Value.FileName}' contains an invalid link: '{entry.Link}'. Expected a positive PR number."); + hasError = true; + continue; + } + + var parentCdnEntry = await _entryFetcher.FetchPrEntryAsync(baseUri, resolvedOrg, repo, resolvedBranch, parentPr, ctx).ConfigureAwait(false); + if (parentCdnEntry is null) + { + collector.EmitError(string.Empty, + $"Changelog entry '{cdnEntry.Value.FileName}' is a marker pointing to PR {parentPr}, but that entry does not exist in {poolLabel}."); + hasError = true; + continue; + } + + var parentEntry = ReleaseNotesSerialization.DeserializeEntry(parentCdnEntry.Value.Content); + if (parentEntry.IsMarker) + { + collector.EmitError(string.Empty, + $"Marker chain detected: '{cdnEntry.Value.FileName}' → '{parentCdnEntry.Value.FileName}' is also a marker. Marker chains are not allowed."); + hasError = true; + continue; + } + + _ = entries.TryAdd(parentCdnEntry.Value.FileName, parentCdnEntry.Value.Content); + } + + return hasError ? null : entries.Select(kv => (kv.Key, kv.Value)).ToList(); + } + + private static bool TryExtractPrNumber(string pr, string authoringOwner, string authoringRepo, out int prNumber) + { + prNumber = 0; + // Normalize to {owner}/{repo}#{number} form; the org/repo may differ from the CDN pool + // (e.g. --owner override renames the pool but the PR URLs still carry the GitHub org). + // We only need the number to build the probe key. + var normalized = NormalizePrForComparison(pr, authoringOwner, authoringRepo); + var hashIndex = normalized.LastIndexOf('#'); + if (hashIndex < 0) + return false; + return int.TryParse(normalized[(hashIndex + 1)..], out prNumber) && prNumber > 0; + } + /// Gate for repo-scoped CDN entry sourcing: true when the authoring repo resolves, local sourcing is not forced (bundle.use_local_changelogs/--force-local/--directory), and a CDN base is configured. private static bool ShouldSourceFromCdn(string? authoringRepo, bool useLocalChangelogs, bool explicitDirectory) { diff --git a/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs index 1c7b77633b..cdac69fbed 100644 --- a/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs +++ b/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs @@ -16,6 +16,7 @@ namespace Elastic.Changelog.Tests.Changelogs; /// /// Tests for the changelog bundle command sourcing its individual changelog entries from the /// public CDN (the default when no --directory is passed and bundle.use_local_changelogs is false). +/// Probe-based: one GET per PR number keyed as {pr}.yaml; no pool registry consulted. /// public class BundleCdnSourcingTests(ITestOutputHelper output) : ChangelogTestBase(output) { @@ -43,18 +44,15 @@ public class BundleCdnSourcingTests(ITestOutputHelper output) : ChangelogTestBas - https://github.com/elastic/elasticsearch/pull/999 """; - // language=json - private const string RegistryJson = - """{ "schema_version": 1, "product": "elasticsearch", "bundles": [ { "file": "1-alpha.yaml" }, { "file": "2-bravo.yaml" } ] }"""; + // language=yaml + private static string MarkerFor(int parentPr) => $"link: \"{parentPr}\"\n"; - private static StubHandler RegistryHandler() => new(req => + private static StubHandler ProbeHandler() => new(req => { var path = req.RequestUri!.AbsolutePath; - if (path.EndsWith("/registry.json", StringComparison.Ordinal)) - return Json(RegistryJson); - if (path.EndsWith("1-alpha.yaml", StringComparison.Ordinal)) + if (path.EndsWith("/100.yaml", StringComparison.Ordinal)) return Yaml(EntryAlpha); - if (path.EndsWith("2-bravo.yaml", StringComparison.Ordinal)) + if (path.EndsWith("/999.yaml", StringComparison.Ordinal)) return Yaml(EntryBravo); return new HttpResponseMessage(HttpStatusCode.NotFound); }); @@ -63,17 +61,16 @@ public class BundleCdnSourcingTests(ITestOutputHelper output) : ChangelogTestBas private static CdnChangelogEntryFetcher Fetcher(ITestOutputHelper output, StubHandler handler) => new(new TestLoggerFactory(output), handler, sleep: (_, _) => Task.CompletedTask); - private CdnChangelogEntryFetcher Fetcher() => Fetcher(Output, RegistryHandler()); + private CdnChangelogEntryFetcher Fetcher() => Fetcher(Output, ProbeHandler()); private string OutputPath() => FileSystem.Path.Join(Paths.WorkingDirectoryRoot.FullName, Guid.NewGuid().ToString(), "bundle.yaml"); [Fact] - public async Task OptionMode_RepoResolvable_SourcesAllEntriesFromRepoPoolOnCdn() + public async Task OptionMode_RepoResolvable_ProbesEntriesByPrNumber() { - // Under the artifact-root layout the CDN entry pool is keyed by the authoring repo, not the - // target product. A resolvable repo (here via --repo) is what enables CDN sourcing. - var handler = RegistryHandler(); + // Probe-based: each PR URL is probed as {pr}.yaml directly; no registry.json is read. + var handler = ProbeHandler(); var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, Fetcher(Output, handler)); var output = OutputPath(); @@ -89,26 +86,26 @@ public async Task OptionMode_RepoResolvable_SourcesAllEntriesFromRepoPoolOnCdn() result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Where(d => d.Severity == Severity.Error).Select(d => d.Message))}"); Collector.Errors.Should().Be(0); - // Entries are sourced from the authoring pool, with org/branch defaulting: changelog/{org}/{repo}/{branch}/... - handler.RequestedPaths.Should().Contain("/changelog/elastic/elasticsearch/main/registry.json"); + // Entries are probed by PR number from the authoring pool. + handler.RequestedPaths.Should().Contain(p => p.EndsWith("/100.yaml", StringComparison.Ordinal)); + handler.RequestedPaths.Should().Contain(p => p.EndsWith("/999.yaml", StringComparison.Ordinal)); + handler.RequestedPaths.Should().NotContain(p => p.Contains("registry.json", StringComparison.Ordinal)); var bundle = await FileSystem.File.ReadAllTextAsync(output, TestContext.Current.CancellationToken); bundle.Should().Contain("Alpha"); bundle.Should().Contain("Bravo"); - bundle.Should().Contain("name: 1-alpha.yaml"); } [Fact] - public async Task OptionMode_OwnerAndBranchOverride_SourcesFromThatPoolOnCdn() + public async Task OptionMode_OwnerAndBranchOverride_ProbesFromThatPool() { - // Explicit owner/branch select a specific pool; the branch is stored verbatim (dots kept). - var handler = RegistryHandler(); + var handler = ProbeHandler(); var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, Fetcher(Output, handler)); var output = OutputPath(); var input = new BundleChangelogsArguments { - Prs = ["https://github.com/elastic/elasticsearch/pull/100", "https://github.com/elastic/elasticsearch/pull/999"], + Prs = ["https://github.com/elastic/elasticsearch/pull/100"], Output = output, Owner = "acme-corp", Repo = "elasticsearch", @@ -119,21 +116,21 @@ public async Task OptionMode_OwnerAndBranchOverride_SourcesFromThatPoolOnCdn() result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Where(d => d.Severity == Severity.Error).Select(d => d.Message))}"); Collector.Errors.Should().Be(0); - handler.RequestedPaths.Should().Contain("/changelog/acme-corp/elasticsearch/8.x/registry.json"); + handler.RequestedPaths.Should().Contain(p => + p.Contains("/acme-corp/elasticsearch/8.x/", StringComparison.Ordinal) && + p.EndsWith("/100.yaml", StringComparison.Ordinal)); } [Fact] - public async Task OptionMode_OwnerFromCombinedRepo_SourcesFromThatPool() + public async Task OptionMode_OwnerFromCombinedRepo_ProbesFromThatPool() { - // When --repo is given in owner/repo form and no explicit owner is set, the owner segment must be - // taken from the repo prefix (not defaulted to elastic), so the CDN pool path stays correct. - var handler = RegistryHandler(); + var handler = ProbeHandler(); var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, Fetcher(Output, handler)); var output = OutputPath(); var input = new BundleChangelogsArguments { - Prs = ["https://github.com/elastic/elasticsearch/pull/100", "https://github.com/elastic/elasticsearch/pull/999"], + Prs = ["https://github.com/elastic/elasticsearch/pull/100"], Output = output, Repo = "acme-corp/widget" }; @@ -142,15 +139,13 @@ public async Task OptionMode_OwnerFromCombinedRepo_SourcesFromThatPool() result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Where(d => d.Severity == Severity.Error).Select(d => d.Message))}"); Collector.Errors.Should().Be(0); - handler.RequestedPaths.Should().Contain("/changelog/acme-corp/widget/main/registry.json"); + handler.RequestedPaths.Should().Contain(p => + p.Contains("/acme-corp/widget/main/", StringComparison.Ordinal)); } [Fact] public async Task OptionMode_NoResolvableRepo_FallsBackToLocal() { - // With no --repo, no bundle.repo in config, and no git-remote resolution at the service layer, the - // authoring repo is unresolvable. With no --directory and no use_local_changelogs, the bundler - // still falls back to local folder sourcing rather than hitting the CDN. var localDir = FileSystem.Path.Join(Paths.WorkingDirectoryRoot.FullName, Guid.NewGuid().ToString(), "changelog"); FileSystem.Directory.CreateDirectory(localDir); await FileSystem.File.WriteAllTextAsync( @@ -165,7 +160,7 @@ await FileSystem.File.WriteAllTextAsync( FileSystem.Directory.CreateDirectory(FileSystem.Path.GetDirectoryName(configPath)!); await FileSystem.File.WriteAllTextAsync(configPath, configContent, TestContext.Current.CancellationToken); - var handler = RegistryHandler(); + var handler = ProbeHandler(); var service = new ChangelogBundlingService(LoggerFactory, FileSystem, ConfigurationContext, null, Fetcher(Output, handler)); var output = OutputPath(); @@ -183,8 +178,6 @@ await FileSystem.File.WriteAllTextAsync( [Fact] public async Task OptionMode_UseLocalChangelogs_ForcesLocalEvenWithResolvableRepo() { - // use_local_changelogs is the explicit opt-out: even when the authoring repo resolves (bundle.repo), - // entries are read from the local folder and the CDN is never touched. var localDir = FileSystem.Path.Join(Paths.WorkingDirectoryRoot.FullName, Guid.NewGuid().ToString(), "changelog"); FileSystem.Directory.CreateDirectory(localDir); await FileSystem.File.WriteAllTextAsync( @@ -201,7 +194,7 @@ await FileSystem.File.WriteAllTextAsync( FileSystem.Directory.CreateDirectory(FileSystem.Path.GetDirectoryName(configPath)!); await FileSystem.File.WriteAllTextAsync(configPath, configContent, TestContext.Current.CancellationToken); - var handler = RegistryHandler(); + var handler = ProbeHandler(); var service = new ChangelogBundlingService(LoggerFactory, FileSystem, ConfigurationContext, null, Fetcher(Output, handler)); var output = OutputPath(); @@ -255,15 +248,13 @@ public async Task CdnInputProducts_ReturnsError() } [Fact] - public async Task RegistryFailure_FailsBundle() + public async Task CdnIssues_ReturnsError() { - var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), - new StubHandler(_ => new HttpResponseMessage(HttpStatusCode.NotFound)), sleep: (_, _) => Task.CompletedTask); - var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, Fetcher()); var input = new BundleChangelogsArguments { - Prs = ["https://github.com/elastic/elasticsearch/pull/100"], + Issues = ["https://github.com/elastic/elasticsearch/issues/42"], Output = OutputPath(), Repo = "elasticsearch" }; @@ -271,37 +262,189 @@ public async Task RegistryFailure_FailsBundle() var result = await service.BundleChangelogs(Collector, input, TestContext.Current.CancellationToken); result.Should().BeFalse(); - Collector.Diagnostics.Should().Contain(d => d.Severity == Severity.Error && d.Message.Contains("registry")); + Collector.Diagnostics.Should().Contain(d => + d.Severity == Severity.Error && d.Message.Contains("--issues") && d.Message.Contains("--force-local")); } [Fact] - public async Task EntryMissingAfterRetries_FailsBundle() + public async Task ProbeMiss_WarnsAndSkips() { - // The registry lists two entries but the CDN never serves one of them. After the retry budget is - // spent the bundle must fail rather than silently omit the missing release entry. + // A 404 probe for a PR with no changelog entry warns and skips; the other entry is still included. + // One request per PR (no retries on 404). var handler = new StubHandler(req => { var path = req.RequestUri!.AbsolutePath; - if (path.EndsWith("/registry.json", StringComparison.Ordinal)) - return Json(RegistryJson); - if (path.EndsWith("1-alpha.yaml", StringComparison.Ordinal)) + if (path.EndsWith("/100.yaml", StringComparison.Ordinal)) return Yaml(EntryAlpha); - return new HttpResponseMessage(HttpStatusCode.NotFound); // 2-bravo.yaml never propagates + return new HttpResponseMessage(HttpStatusCode.NotFound); // 999 has no entry }); - var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, maxAttempts: 2, sleep: (_, _) => Task.CompletedTask); + var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, sleep: (_, _) => Task.CompletedTask); var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + var output = OutputPath(); - var input = new BundleChangelogsArguments + var result = await service.BundleChangelogs(Collector, new BundleChangelogsArguments { Prs = ["https://github.com/elastic/elasticsearch/pull/100", "https://github.com/elastic/elasticsearch/pull/999"], + Output = output, + Repo = "elasticsearch" + }, TestContext.Current.CancellationToken); + + result.Should().BeTrue("probe miss is a warn, not a fatal error"); + Collector.Errors.Should().Be(0); + // 404 is authoritative on the probe path — not retried. + handler.RequestedPaths.Count(p => p.EndsWith("/999.yaml", StringComparison.Ordinal)) + .Should().Be(1, "404 must not be retried on the probe path"); + var bundle = await FileSystem.File.ReadAllTextAsync(output, TestContext.Current.CancellationToken); + bundle.Should().Contain("Alpha", "the entry found by probe must appear in the bundle"); + } + + [Fact] + public async Task ProbedEntry_5xxTransient_Retried() + { + // A transient 5xx on the probe path uses the retry budget; the final content is returned. + var callCount = 0; + var handler = new StubHandler(req => + { + var path = req.RequestUri!.AbsolutePath; + if (path.Contains("/100.yaml", StringComparison.Ordinal)) + { + callCount++; + if (callCount < 3) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable); + return Yaml(EntryAlpha); + } + return new HttpResponseMessage(HttpStatusCode.NotFound); + }); + var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, maxAttempts: 4, sleep: (_, _) => Task.CompletedTask); + var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + + var result = await service.BundleChangelogs(Collector, new BundleChangelogsArguments + { + Prs = ["https://github.com/elastic/elasticsearch/pull/100"], Output = OutputPath(), Repo = "elasticsearch" - }; + }, TestContext.Current.CancellationToken); - var result = await service.BundleChangelogs(Collector, input, TestContext.Current.CancellationToken); + result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Where(d => d.Severity == Severity.Error).Select(d => d.Message))}"); + callCount.Should().BeGreaterThan(1, "5xx must be retried"); + } + + [Fact] + public async Task MarkerEntry_ResolvesDepthOne() + { + // PR 200 is a marker pointing to PR 100 (the primary). Bundling PR 200 should resolve to the Alpha entry. + var handler = new StubHandler(req => + { + var path = req.RequestUri!.AbsolutePath; + if (path.EndsWith("/100.yaml", StringComparison.Ordinal)) + return Yaml(EntryAlpha); + if (path.EndsWith("/200.yaml", StringComparison.Ordinal)) + return Yaml(MarkerFor(100)); + return new HttpResponseMessage(HttpStatusCode.NotFound); + }); + var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, sleep: (_, _) => Task.CompletedTask); + var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + + var output = OutputPath(); + var result = await service.BundleChangelogs(Collector, new BundleChangelogsArguments + { + Prs = ["https://github.com/elastic/elasticsearch/pull/200"], + Output = output, + Repo = "elasticsearch" + }, TestContext.Current.CancellationToken); + + result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Where(d => d.Severity == Severity.Error).Select(d => d.Message))}"); + Collector.Errors.Should().Be(0); + var bundle = await FileSystem.File.ReadAllTextAsync(output, TestContext.Current.CancellationToken); + bundle.Should().Contain("Alpha", "the marker must resolve to its parent entry"); + bundle.Should().NotContain("link:", "the marker itself must not appear in the output"); + } + + [Fact] + public async Task MarkerEntry_DuplicateMarkersToSameParent_OneEntryInBundle() + { + // PRs 200 and 201 are both markers for PR 100. Bundling both should yield one Alpha entry. + var handler = new StubHandler(req => + { + var path = req.RequestUri!.AbsolutePath; + if (path.EndsWith("/100.yaml", StringComparison.Ordinal)) + return Yaml(EntryAlpha); + if (path.EndsWith("/200.yaml", StringComparison.Ordinal) || path.EndsWith("/201.yaml", StringComparison.Ordinal)) + return Yaml(MarkerFor(100)); + return new HttpResponseMessage(HttpStatusCode.NotFound); + }); + var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, sleep: (_, _) => Task.CompletedTask); + var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + + var output = OutputPath(); + var result = await service.BundleChangelogs(Collector, new BundleChangelogsArguments + { + Prs = [ + "https://github.com/elastic/elasticsearch/pull/200", + "https://github.com/elastic/elasticsearch/pull/201" + ], + Output = output, + Repo = "elasticsearch" + }, TestContext.Current.CancellationToken); + + result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Where(d => d.Severity == Severity.Error).Select(d => d.Message))}"); + Collector.Errors.Should().Be(0); + var bundle = await FileSystem.File.ReadAllTextAsync(output, TestContext.Current.CancellationToken); + bundle.Split("title: Alpha").Length.Should().Be(2, "exactly one Alpha entry must appear"); + } + + [Fact] + public async Task MarkerEntry_Chain_FailsBundle() + { + // Marker chains (marker → marker) are hard errors. + var handler = new StubHandler(req => + { + var path = req.RequestUri!.AbsolutePath; + if (path.EndsWith("/200.yaml", StringComparison.Ordinal)) + return Yaml(MarkerFor(100)); + if (path.EndsWith("/100.yaml", StringComparison.Ordinal)) + return Yaml(MarkerFor(99)); // 100 is also a marker + return new HttpResponseMessage(HttpStatusCode.NotFound); + }); + var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, sleep: (_, _) => Task.CompletedTask); + var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + + var result = await service.BundleChangelogs(Collector, new BundleChangelogsArguments + { + Prs = ["https://github.com/elastic/elasticsearch/pull/200"], + Output = OutputPath(), + Repo = "elasticsearch" + }, TestContext.Current.CancellationToken); result.Should().BeFalse(); - Collector.Diagnostics.Should().Contain(d => d.Severity == Severity.Error && d.Message.Contains("2-bravo.yaml")); + Collector.Diagnostics.Should().Contain(d => + d.Severity == Severity.Error && d.Message.Contains("Marker chain")); + } + + [Fact] + public async Task MarkerEntry_ParentMissing_FailsBundle() + { + // A marker whose parent doesn't exist is a hard error (the pipeline promises the parent exists). + var handler = new StubHandler(req => + { + var path = req.RequestUri!.AbsolutePath; + if (path.EndsWith("/200.yaml", StringComparison.Ordinal)) + return Yaml(MarkerFor(100)); + return new HttpResponseMessage(HttpStatusCode.NotFound); // 100.yaml missing + }); + var fetcher = new CdnChangelogEntryFetcher(new TestLoggerFactory(Output), handler, sleep: (_, _) => Task.CompletedTask); + var service = new ChangelogBundlingService(LoggerFactory, FileSystem, null, null, fetcher); + + var result = await service.BundleChangelogs(Collector, new BundleChangelogsArguments + { + Prs = ["https://github.com/elastic/elasticsearch/pull/200"], + Output = OutputPath(), + Repo = "elasticsearch" + }, TestContext.Current.CancellationToken); + + result.Should().BeFalse(); + Collector.Diagnostics.Should().Contain(d => + d.Severity == Severity.Error && d.Message.Contains("marker") && d.Message.Contains("100")); } [Fact] From 2122b44e79e24fe6ca14a18d11927ea2eff3185e Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 11:36:58 +0200 Subject: [PATCH 04/17] =?UTF-8?q?fix(changelog):=20marker=20guard=20?= =?UTF-8?q?=E2=80=94=20allow=20link:=20alongside=20other=20fields,=20only?= =?UTF-8?q?=20short-circuit=20pure=20markers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- .../Scrubbing/ChangelogContentScrubber.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 6d55964bff..4583d05e19 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -74,17 +74,16 @@ private async Task ScrubChangelog(string content, Cancel ctx) var normalized = ReleaseNotesSerialization.NormalizeYaml(content); var entry = ReleaseNotesSerialization.DeserializeEntry(normalized); - // Marker: link: with no other content. Return unchanged — there is no URL to scrub. + // Pure marker: link: with no other content. Return unchanged — there is no URL to scrub. if (entry.Link != null) { var hasContent = !string.IsNullOrEmpty(entry.Title) || entry.Type != ChangelogEntryType.Invalid || entry.Products is { Count: > 0 } || entry.Prs is { Count: > 0 }; - if (hasContent) - throw new InvalidOperationException( - "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); - return content; + if (!hasContent) + return content; + // Has link: alongside other fields — fall through to normal scrubbing (link is preserved below). } var bundledEntry = new BundledEntry From 82c1e25d723d1b85211f1527253738302c2aba46 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 11:48:43 +0200 Subject: [PATCH 05/17] fix(changelog): restore marker-only guard; remove contradictory scrub test Co-Authored-By: Claude Sonnet 4.6 --- .../Scrubbing/ChangelogContentScrubber.cs | 9 +++++---- .../Scrubbing/ChangelogContentScrubberTests.cs | 18 ------------------ 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 4583d05e19..6d55964bff 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -74,16 +74,17 @@ private async Task ScrubChangelog(string content, Cancel ctx) var normalized = ReleaseNotesSerialization.NormalizeYaml(content); var entry = ReleaseNotesSerialization.DeserializeEntry(normalized); - // Pure marker: link: with no other content. Return unchanged — there is no URL to scrub. + // Marker: link: with no other content. Return unchanged — there is no URL to scrub. if (entry.Link != null) { var hasContent = !string.IsNullOrEmpty(entry.Title) || entry.Type != ChangelogEntryType.Invalid || entry.Products is { Count: > 0 } || entry.Prs is { Count: > 0 }; - if (!hasContent) - return content; - // Has link: alongside other fields — fall through to normal scrubbing (link is preserved below). + if (hasContent) + throw new InvalidOperationException( + "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); + return content; } var bundledEntry = new BundledEntry diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs index 4a341c0a45..6918a8dc57 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs @@ -30,24 +30,6 @@ public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved() entry.IsMarker.Should().BeTrue(); } - [Fact] - public async Task ScrubAsync_EntryWithLinkAndPrivatePrs_LinkPreservedPrsStripped() - { - // A hypothetical entry with both link: and prs: pointing to a private repo. - // link: must survive scrubbing; private prs: must be stripped. - var yaml = - "link: \"12345\"\n" + - "prs:\n" + - " - https://github.com/elastic/private-repo/pull/99\n"; - var scrubber = Scrubber(["elastic/elasticsearch"]); // private-repo not allowed - - var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12346.yaml", yaml, CancellationToken.None); - - var entry = ReleaseNotesSerialization.DeserializeEntry(result); - entry.Link.Should().Be("12345", "link: must survive the scrub even when prs: is stripped"); - entry.Prs.Should().BeNullOrEmpty("private PR reference must be scrubbed"); - } - [Fact] public async Task ScrubAsync_NormalEntry_LinkIsNull() { From b25f62eee3376cb5903fc60192802573d0642530 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 12:02:31 +0200 Subject: [PATCH 06/17] =?UTF-8?q?fix(changelog):=20propagate=20probe=20err?= =?UTF-8?q?or=20from=20FetchPrEntryAsync=20=E2=80=94=20transient=20failure?= =?UTF-8?q?s=20now=20throw=20instead=20of=20returning=20null?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- .../ReleaseNotes/CdnChangelogEntryFetcher.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs index 329bf936dd..175b0f9cf3 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogEntryFetcher.cs @@ -304,7 +304,9 @@ public async Task> FetchAsync( var poolSegments = ChangelogKeys.PoolSegments(org, repo, branch); var uri = CombineSegments(baseUri, [.. poolSegments, fileName]); var poolLabel = $"{org}/{repo}/{branch}"; - var (fetched, content, _) = await TryProbeEntryAsync(uri, fileName, poolLabel, ctx).ConfigureAwait(false); + var (fetched, content, lastError) = await TryProbeEntryAsync(uri, fileName, poolLabel, ctx).ConfigureAwait(false); + if (lastError != null) + throw new InvalidOperationException($"Transient failure probing changelog entry '{fileName}' for {poolLabel}: {lastError}"); return fetched ? new CdnChangelogEntry(fileName, content) : null; } From 3c68594eb90aba246a2f5241f4a7ef3e476e3d36 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 12:46:30 +0200 Subject: [PATCH 07/17] =?UTF-8?q?fix(changelog):=20TryExtractPrNumber=20va?= =?UTF-8?q?lidates=20repo=20name=20=E2=80=94=20kibana=20PRs=20no=20longer?= =?UTF-8?q?=20probe=20elasticsearch=20pool?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- .../Bundling/ChangelogBundlingService.cs | 11 ++++++++--- .../Changelogs/BundleCdnSourcingTests.cs | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs index 48503454cb..e47b31d05a 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs @@ -1429,13 +1429,18 @@ private BundleChangelogsArguments ApplyConfigDefaults(BundleChangelogsArguments private static bool TryExtractPrNumber(string pr, string authoringOwner, string authoringRepo, out int prNumber) { prNumber = 0; - // Normalize to {owner}/{repo}#{number} form; the org/repo may differ from the CDN pool - // (e.g. --owner override renames the pool but the PR URLs still carry the GitHub org). - // We only need the number to build the probe key. + // Normalize to {owner}/{repo}#{number} form. var normalized = NormalizePrForComparison(pr, authoringOwner, authoringRepo); var hashIndex = normalized.LastIndexOf('#'); if (hashIndex < 0) return false; + // Reject PRs whose repo does not match the authoring repo — a kibana PR number must not + // probe the elasticsearch pool even when --owner is overridden (owner may differ, repo must not). + var ownerRepo = normalized[..hashIndex]; + var slashIndex = ownerRepo.LastIndexOf('/'); + var prRepo = slashIndex >= 0 ? ownerRepo[(slashIndex + 1)..] : ownerRepo; + if (!string.Equals(prRepo, authoringRepo, StringComparison.OrdinalIgnoreCase)) + return false; return int.TryParse(normalized[(hashIndex + 1)..], out prNumber) && prNumber > 0; } diff --git a/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs index cdac69fbed..18a7460aa5 100644 --- a/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs +++ b/tests/Elastic.Changelog.Tests/Changelogs/BundleCdnSourcingTests.cs @@ -130,7 +130,7 @@ public async Task OptionMode_OwnerFromCombinedRepo_ProbesFromThatPool() var input = new BundleChangelogsArguments { - Prs = ["https://github.com/elastic/elasticsearch/pull/100"], + Prs = ["https://github.com/elastic/widget/pull/100"], Output = output, Repo = "acme-corp/widget" }; From ec766b14cae770bf262d2ff31d4a39755ceed194 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 25 Aug 2026 18:42:44 +0200 Subject: [PATCH 08/17] =?UTF-8?q?feat(changelog):=20canonical=20upload=20k?= =?UTF-8?q?eys=20and=20scrubber=20marker=20writing=20=E2=80=94=20Step=208?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upload service derives the S3 key from the entry's min PR number rather than the authored filename; entries with multiple PRs get a full-body primary at {min-pr}.yaml and an inline link: marker for each non-primary PR. note-* files pass through verbatim. Scrubber now returns ScrubResult with Content, CanonicalKey, and Markers: - CanonicalKey is non-null when the private-bucket key is not already {pr}.yaml (handles the 5 legacy non-canonical objects); ScrubberProcessor writes to the canonical public key instead of mirroring the source key verbatim. - Markers are additional link:{min-pr} objects the processor writes to the public bucket for each non-primary PR in a multi-PR entry. S3IncrementalUploader.UploadTarget gains an optional InlineContent property so machine-generated markers can be uploaded without a temporary on-disk file. Co-Authored-By: Claude Sonnet 4.6 --- .../Scrubbing/ChangelogContentScrubber.cs | 89 +++++++++++-- .../Scrubbing/ScrubberProcessor.cs | 16 ++- .../Uploading/ChangelogUploadService.cs | 65 +++++++++- .../S3/S3IncrementalUploader.cs | 27 +++- .../ChangelogContentScrubberTests.cs | 86 ++++++++++++- .../Scrubbing/ScrubberProcessorTests.cs | 51 +++++++- .../Uploading/ChangelogUploadServiceTests.cs | 118 ++++++++++++++++++ 7 files changed, 433 insertions(+), 19 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 6d55964bff..c492656565 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -10,6 +10,29 @@ namespace Elastic.Changelog.Scrubbing; +/// +/// The result of scrubbing a changelog artifact for public publication. +/// +public record ScrubResult +{ + /// The scrubbed YAML content to write to the public bucket. + public required string Content { get; init; } + + /// + /// Canonical public key for this object. Null when the source key is already canonical + /// and no rename is needed; non-null when the source was named non-canonically + /// (e.g. 12345-fix.yaml) and must be written at 12345.yaml instead. + /// + public string? CanonicalKey { get; init; } + + /// + /// Additional marker objects to write to the public bucket for non-primary PRs + /// in a multi-PR entry. Each marker's entire content is link: {parentPr}. + /// Empty for single-PR entries and bundles. + /// + public IReadOnlyList<(string Key, string Content)> Markers { get; init; } = []; +} + /// Rewrites private-bucket changelog YAML into its public, allowlist-scrubbed form. public interface IChangelogContentScrubber { @@ -18,7 +41,7 @@ public interface IChangelogContentScrubber /// shape: bundle/{product}/… is a bundle, everything else a changelog entry. Throws /// when the content cannot be proven free of private references. /// - Task ScrubAsync(string key, string content, Cancel ctx); + Task ScrubAsync(string key, string content, Cancel ctx); } /// @@ -31,7 +54,7 @@ public sealed class ChangelogContentScrubber(ILoggerFactory logFactory, IReadOnl private readonly ILogger _logger = logFactory.CreateLogger(); /// - public async Task ScrubAsync(string key, string content, Cancel ctx) + public async Task ScrubAsync(string key, string content, Cancel ctx) { // Artifact-root layout: bundles live under "bundle/{product}/…", entries under // "changelog/{org}/{repo}/{branch}/…". Match the bundle prefix (not a "/bundle/" substring, @@ -40,10 +63,10 @@ public async Task ScrubAsync(string key, string content, Cancel ctx) return isBundlePath ? await ScrubBundle(content, ctx) - : await ScrubChangelog(content, ctx); + : await ScrubChangelog(key, content, ctx); } - private async Task ScrubBundle(string content, Cancel ctx) + private async Task ScrubBundle(string content, Cancel ctx) { ctx.ThrowIfCancellationRequested(); @@ -59,15 +82,15 @@ private async Task ScrubBundle(string content, Cancel ctx) { _logger.LogInformation("Bundle had no private references, writing unchanged"); LinkAllowlistSanitizer.ValidateNoPrivateReferences(content, allowRepos); - return content; + return new ScrubResult { Content = content }; } var result = ReleaseNotesSerialization.SerializeBundle(sanitized); LinkAllowlistSanitizer.ValidateNoPrivateReferences(result, allowRepos); - return result; + return new ScrubResult { Content = result }; } - private async Task ScrubChangelog(string content, Cancel ctx) + private async Task ScrubChangelog(string key, string content, Cancel ctx) { ctx.ThrowIfCancellationRequested(); @@ -84,9 +107,12 @@ private async Task ScrubChangelog(string content, Cancel ctx) if (hasContent) throw new InvalidOperationException( "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); - return content; + return new ScrubResult { Content = content }; } + // Derive canonical key and markers from the pre-scrub entry while prs: URLs are still intact. + var (canonicalKey, markers) = BuildCanonicalKeyAndMarkers(key, entry); + var bundledEntry = new BundledEntry { Type = entry.Type, @@ -112,7 +138,7 @@ private async Task ScrubChangelog(string content, Cancel ctx) { _logger.LogInformation("Changelog entry had no private references, writing unchanged"); LinkAllowlistSanitizer.ValidateNoPrivateReferences(content, allowRepos); - return content; + return new ScrubResult { Content = content, CanonicalKey = canonicalKey, Markers = markers }; } var scrubEntry = entry with @@ -127,6 +153,49 @@ private async Task ScrubChangelog(string content, Cancel ctx) var result = ReleaseNotesSerialization.SerializeEntry(scrubEntry); LinkAllowlistSanitizer.ValidateNoPrivateReferences(result, allowRepos); - return result; + return new ScrubResult { Content = result, CanonicalKey = canonicalKey, Markers = markers }; + } + + private static (string? CanonicalKey, IReadOnlyList<(string Key, string Content)> Markers) + BuildCanonicalKeyAndMarkers(string sourceKey, ChangelogEntry entry) + { + // note-* files are their own anchor; markers have link: already set as their identity. + var lastSlash = sourceKey.LastIndexOf('/'); + if (lastSlash < 0) + return (null, []); + + var fileName = sourceKey[(lastSlash + 1)..]; + var keyPrefix = sourceKey[..(lastSlash + 1)]; + + if (fileName.StartsWith("note-", StringComparison.OrdinalIgnoreCase) || entry.IsMarker) + return (null, []); + + var prNumbers = entry.Prs? + .Select(pr => ChangelogTextUtilities.ExtractPrNumber(pr)) + .Where(n => n.HasValue) + .Select(n => n!.Value) + .Distinct() + .OrderBy(n => n) + .ToList(); + + if (prNumbers is null or { Count: 0 }) + return (null, []); + + var primaryPr = prNumbers[0]; + var canonicalFileName = $"{primaryPr}.yaml"; + var canonicalKey = string.Equals(fileName, canonicalFileName, StringComparison.OrdinalIgnoreCase) + ? null + : keyPrefix + canonicalFileName; + + if (prNumbers.Count == 1) + return (canonicalKey, []); + + var markerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = primaryPr.ToString() }); + var markers = prNumbers + .Skip(1) + .Select(pr => (keyPrefix + $"{pr}.yaml", markerContent)) + .ToList<(string, string)>(); + + return (canonicalKey, markers); } } diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index 4a65c51e2d..845a92d6fb 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -313,9 +313,19 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa } else { - var scrubbed = await scrubber.ScrubAsync(key, source.Content, ctx); - await PutPublicObject(key, scrubbed, "application/yaml", ctx); - _logger.LogInformation("Scrubbed and wrote {Key} to public bucket", key); + var scrubResult = await scrubber.ScrubAsync(key, source.Content, ctx); + var publicKey = scrubResult.CanonicalKey ?? key; + await PutPublicObject(publicKey, scrubResult.Content, "application/yaml", ctx); + if (scrubResult.CanonicalKey is not null) + _logger.LogInformation("Scrubbed {Key} → canonical public key {CanonicalKey}", key, scrubResult.CanonicalKey); + else + _logger.LogInformation("Scrubbed and wrote {Key} to public bucket", key); + + foreach (var (markerKey, markerContent) in scrubResult.Markers) + { + await PutPublicObject(markerKey, markerContent, "application/yaml", ctx); + _logger.LogInformation("Wrote marker {MarkerKey} → {PrimaryKey}", markerKey, publicKey); + } } } else diff --git a/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs b/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs index 33b3f149b8..77a4c6bcbf 100644 --- a/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs +++ b/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs @@ -11,6 +11,7 @@ using Elastic.Documentation.Diagnostics; using Elastic.Documentation.FileSystems; using Elastic.Documentation.Integrations.S3; +using Elastic.Documentation.ReleaseNotes; using Elastic.Documentation.Services; using Microsoft.Extensions.Logging; @@ -174,13 +175,73 @@ internal IReadOnlyList DiscoverUploadTargets(IDiagnosticsCollector } var fileName = _fileSystem.Path.GetFileName(filePath); - var s3Key = ChangelogKeys.ChangelogFileKey(org, repo, branch, fileName); - targets.Add(new UploadTarget(filePath, s3Key)); + + if (fileName.StartsWith("note-", StringComparison.OrdinalIgnoreCase)) + { + targets.Add(new UploadTarget(filePath, ChangelogKeys.ChangelogFileKey(org, repo, branch, fileName))); + continue; + } + + ChangelogEntry? entry = null; + try + { + var content = _fileSystem.File.ReadAllText(filePath); + entry = ReleaseNotesSerialization.DeserializeEntry(content); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Could not read entry from {File}; using filename for key", filePath); + } + + var (canonicalFileName, markerEntries) = DeriveCanonicalFileNameAndMarkers(fileName, entry, _logger); + var primaryKey = ChangelogKeys.ChangelogFileKey(org, repo, branch, canonicalFileName); + targets.Add(new UploadTarget(filePath, primaryKey)); + + foreach (var (markerFileName, markerContent) in markerEntries) + { + var markerKey = ChangelogKeys.ChangelogFileKey(org, repo, branch, markerFileName); + targets.Add(new UploadTarget(string.Empty, markerKey, markerContent)); + } } return targets; } + internal static (string CanonicalFileName, IReadOnlyList<(string FileName, string Content)> Markers) + DeriveCanonicalFileNameAndMarkers(string fileName, ChangelogEntry? entry, ILogger? logger = null) + { + if (entry is null) + return (fileName, []); + + var prNumbers = entry.Prs? + .Select(pr => ChangelogTextUtilities.ExtractPrNumber(pr)) + .Where(n => n.HasValue) + .Select(n => n!.Value) + .Distinct() + .OrderBy(n => n) + .ToList(); + + if (prNumbers is null or { Count: 0 }) + { + logger?.LogWarning("Entry {File} has no PR references; using filename as-is for key", fileName); + return (fileName, []); + } + + var primaryPr = prNumbers[0]; // already sorted ascending, min is first + var canonicalFileName = $"{primaryPr}.yaml"; + + if (prNumbers.Count == 1) + return (canonicalFileName, []); + + var markerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = primaryPr.ToString() }); + var markers = prNumbers + .Skip(1) + .Select(pr => ($"{pr}.yaml", markerContent)) + .ToList(); + + return (canonicalFileName, markers); + } + internal IReadOnlyList DiscoverBundleUploadTargets(IDiagnosticsCollector collector, string bundleDir) { var rootDir = _fileSystem.DirectoryInfo.New(bundleDir); diff --git a/src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs b/src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs index f774d71e7a..527d4cb3d8 100644 --- a/src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs +++ b/src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs @@ -10,7 +10,12 @@ namespace Elastic.Documentation.Integrations.S3; /// Describes a file to upload: its local path and intended S3 key. -public record UploadTarget(string LocalPath, string S3Key); +/// +/// When is non-null the object is uploaded from that string +/// directly (skipping ETag comparison) and is ignored. Used for +/// machine-generated marker objects that have no on-disk counterpart. +/// +public record UploadTarget(string LocalPath, string S3Key, string? InlineContent = null); /// Result of an incremental upload run. public record UploadResult(int Uploaded, int Skipped, int Failed); @@ -41,6 +46,14 @@ public async Task Upload(IReadOnlyList targets, bool try { + if (target.InlineContent is { } inlineContent) + { + _logger.LogInformation("Uploading inline marker → s3://{Bucket}/{S3Key}", bucketName, target.S3Key); + await PutInlineObject(target.S3Key, inlineContent, ctx); + uploaded++; + continue; + } + if (!skipEtagCheck) { var remoteEtag = await GetRemoteEtag(target.S3Key, ctx); @@ -101,4 +114,16 @@ private async Task PutObject(UploadTarget target, Cancel ctx) }; _ = await s3Client.PutObjectAsync(request, ctx); } + + private async Task PutInlineObject(string key, string content, Cancel ctx) + { + var request = new PutObjectRequest + { + BucketName = bucketName, + Key = key, + ContentBody = content, + ContentType = "application/yaml" + }; + _ = await s3Client.PutObjectAsync(request, ctx); + } } diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs index 6918a8dc57..ad2d993252 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs @@ -25,9 +25,11 @@ public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved() var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12346.yaml", yaml, CancellationToken.None); - var entry = ReleaseNotesSerialization.DeserializeEntry(result); + var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content); entry.Link.Should().Be("12345", "link: must survive the scrub round-trip"); entry.IsMarker.Should().BeTrue(); + result.CanonicalKey.Should().BeNull("markers are already canonical"); + result.Markers.Should().BeEmpty(); } [Fact] @@ -42,8 +44,88 @@ public async Task ScrubAsync_NormalEntry_LinkIsNull() var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12345.yaml", yaml, CancellationToken.None); - var entry = ReleaseNotesSerialization.DeserializeEntry(result); + var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content); entry.Link.Should().BeNull(); entry.IsMarker.Should().BeFalse(); } + + [Fact] + public async Task ScrubAsync_NonCanonicalKey_ReturnsCanonicalKey() + { + var yaml = + "title: Fix search performance\n" + + "type: bug-fix\n" + + "prs:\n" + + " - https://github.com/elastic/elasticsearch/pull/12345\n"; + var scrubber = Scrubber(["elastic/elasticsearch"]); + + var result = await scrubber.ScrubAsync( + "changelog/elastic/elasticsearch/main/12345-fix.yaml", yaml, CancellationToken.None); + + result.CanonicalKey.Should().Be("changelog/elastic/elasticsearch/main/12345.yaml"); + result.Markers.Should().BeEmpty(); + } + + [Fact] + public async Task ScrubAsync_AlreadyCanonicalKey_CanonicalKeyIsNull() + { + var yaml = + "title: Fix search performance\n" + + "type: bug-fix\n" + + "prs:\n" + + " - https://github.com/elastic/elasticsearch/pull/12345\n"; + var scrubber = Scrubber(["elastic/elasticsearch"]); + + var result = await scrubber.ScrubAsync( + "changelog/elastic/elasticsearch/main/12345.yaml", yaml, CancellationToken.None); + + result.CanonicalKey.Should().BeNull("source key is already canonical"); + } + + [Fact] + public async Task ScrubAsync_MultiPrEntry_WritesMarkersForNonPrimaryPrs() + { + // prs [100, 200, 300] → primary is 100 (min), markers for 200 and 300 + var yaml = + "title: Multi-PR feature\n" + + "type: feature\n" + + "prs:\n" + + " - https://github.com/elastic/elasticsearch/pull/300\n" + + " - https://github.com/elastic/elasticsearch/pull/100\n" + + " - https://github.com/elastic/elasticsearch/pull/200\n"; + var scrubber = Scrubber(["elastic/elasticsearch"]); + + var result = await scrubber.ScrubAsync( + "changelog/elastic/elasticsearch/main/100.yaml", yaml, CancellationToken.None); + + result.CanonicalKey.Should().BeNull("source key already matches the min PR"); + result.Markers.Should().HaveCount(2); + result.Markers.Should().Contain(m => m.Key == "changelog/elastic/elasticsearch/main/200.yaml"); + result.Markers.Should().Contain(m => m.Key == "changelog/elastic/elasticsearch/main/300.yaml"); + + foreach (var (_, markerContent) in result.Markers) + { + var markerEntry = ReleaseNotesSerialization.DeserializeEntry(markerContent); + markerEntry.Link.Should().Be("100"); + markerEntry.IsMarker.Should().BeTrue(); + } + } + + [Fact] + public async Task ScrubAsync_NoteFile_PassesThroughWithNoCanonicalKey() + { + var yaml = + "title: Known issue with rollover\n" + + "type: known-issue\n" + + "products:\n" + + " - product: elasticsearch\n" + + " target: 9.2.0\n"; + var scrubber = Scrubber(["elastic/elasticsearch"]); + + var result = await scrubber.ScrubAsync( + "changelog/elastic/elasticsearch/main/note-slow-rollover.yaml", yaml, CancellationToken.None); + + result.CanonicalKey.Should().BeNull("note-* files are already canonical and need no rename"); + result.Markers.Should().BeEmpty(); + } } diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs index bfb339b767..b70e763430 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs @@ -28,7 +28,8 @@ public ScrubberProcessorTests() // The real scrub pass has its own tests; here it just marks content so assertions can // tell a scrubbed write from a raw copy. _ = A.CallTo(() => _scrubber.ScrubAsync(A._, A._, A._)) - .ReturnsLazily((string _, string content, Cancel _) => Task.FromResult("scrubbed: " + content)); + .ReturnsLazily((string _, string content, Cancel _) => + Task.FromResult(new ScrubResult { Content = "scrubbed: " + content })); var reconciler = new BundleRegistryReconciler( NullLoggerFactory.Instance, _s3.Client, PublicBucket, retryBaseDelay: TimeSpan.Zero, metrics: _metrics); @@ -273,6 +274,54 @@ public async Task Process_SourceChangingMidFlight_IsDetectedByPostWriteValidatio _metrics.ObjectReconcileRetries.Should().Be(1); } + [Fact] + public async Task Process_CanonicalKeyEntry_WritesToCanonicalPublicKey() + { + // Scrubber says the private key 12345-fix.yaml should be written to public as 12345.yaml. + const string privateKey = "changelog/elastic/elasticsearch/main/12345-fix.yaml"; + const string canonicalKey = "changelog/elastic/elasticsearch/main/12345.yaml"; + _ = _s3.Seed(PrivateBucket, privateKey, "entry-content"); + _ = A.CallTo(() => _scrubber.ScrubAsync(privateKey, A._, A._)) + .ReturnsLazily((string _, string content, Cancel _) => + Task.FromResult(new ScrubResult { Content = "scrubbed: " + content, CanonicalKey = canonicalKey })); + + var failed = await _processor.ProcessAsync([Message("ObjectCreated:Put", privateKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.ContentOf(PublicBucket, canonicalKey).Should().Be("scrubbed: entry-content", + "canonical key must receive the scrubbed content"); + _s3.Exists(PublicBucket, privateKey).Should().BeFalse( + "private (non-canonical) key must not appear in the public bucket"); + } + + [Fact] + public async Task Process_MultiPrEntry_WritesMarkersForNonPrimaryPrs() + { + // Scrubber returns markers for PRs 200 and 300 pointing to the primary PR 100. + const string privateKey = "changelog/elastic/elasticsearch/main/100.yaml"; + _ = _s3.Seed(PrivateBucket, privateKey, "multi-pr-content"); + _ = A.CallTo(() => _scrubber.ScrubAsync(privateKey, A._, A._)) + .ReturnsLazily((string _, string content, Cancel _) => + Task.FromResult(new ScrubResult + { + Content = "scrubbed: " + content, + Markers = + [ + ("changelog/elastic/elasticsearch/main/200.yaml", "link: \"100\"\n"), + ("changelog/elastic/elasticsearch/main/300.yaml", "link: \"100\"\n") + ] + })); + + var failed = await _processor.ProcessAsync([Message("ObjectCreated:Put", privateKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.ContentOf(PublicBucket, privateKey).Should().Be("scrubbed: multi-pr-content"); + _s3.ContentOf(PublicBucket, "changelog/elastic/elasticsearch/main/200.yaml").Should().Be("link: \"100\"\n", + "marker for PR 200 must be written to public bucket"); + _s3.ContentOf(PublicBucket, "changelog/elastic/elasticsearch/main/300.yaml").Should().Be("link: \"100\"\n", + "marker for PR 300 must be written to public bucket"); + } + [Fact] public async Task Process_FailedObjectReconcile_FailsOnlyItsOwnMessages() { diff --git a/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs b/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs index 975b0ba88f..81dca91781 100644 --- a/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs +++ b/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs @@ -11,7 +11,9 @@ using Elastic.Changelog.Tests.Changelogs; using Elastic.Changelog.Uploading; using Elastic.Documentation.Configuration; +using Elastic.Documentation.Configuration.ReleaseNotes; using Elastic.Documentation.FileSystems; +using Elastic.Documentation.ReleaseNotes; using FakeItEasy; using Microsoft.Extensions.Logging.Abstractions; @@ -807,4 +809,120 @@ public async Task Upload_ChangelogArtifactType_DoesNotWriteRegistry() A._ )).MustNotHaveHappened(); } + + // --- Canonical key derivation tests + + [Fact] + public void DeriveCanonicalFileNameAndMarkers_FullPrUrl_UsesMinPrAsCanonicalKey() + { + var entry = new ChangelogEntry + { + Prs = ["https://github.com/elastic/elasticsearch/pull/12345"] + }; + + var (canonicalFileName, markers) = ChangelogUploadService.DeriveCanonicalFileNameAndMarkers("12345-fix.yaml", entry); + + canonicalFileName.Should().Be("12345.yaml"); + markers.Should().BeEmpty(); + } + + [Fact] + public void DeriveCanonicalFileNameAndMarkers_MultiPrEntry_ReturnsMinAndMarkersForRest() + { + var entry = new ChangelogEntry + { + Prs = + [ + "https://github.com/elastic/elasticsearch/pull/300", + "https://github.com/elastic/elasticsearch/pull/100", + "https://github.com/elastic/elasticsearch/pull/200" + ] + }; + + var (canonicalFileName, markers) = ChangelogUploadService.DeriveCanonicalFileNameAndMarkers("100.yaml", entry); + + canonicalFileName.Should().Be("100.yaml", "min PR is 100"); + markers.Should().HaveCount(2); + markers.Should().Contain(m => m.FileName == "200.yaml"); + markers.Should().Contain(m => m.FileName == "300.yaml"); + + foreach (var (_, markerContent) in markers) + { + var markerEntry = ReleaseNotesSerialization.DeserializeEntry(markerContent); + markerEntry.Link.Should().Be("100"); + } + } + + [Fact] + public void DeriveCanonicalFileNameAndMarkers_NoPrs_FallsBackToFileName() + { + var entry = new ChangelogEntry { Title = "No PRs" }; + + var (canonicalFileName, markers) = ChangelogUploadService.DeriveCanonicalFileNameAndMarkers("some-note.yaml", entry); + + canonicalFileName.Should().Be("some-note.yaml"); + markers.Should().BeEmpty(); + } + + [Fact] + public void DiscoverUploadTargets_EntryWithFullPrUrl_UsesCanonicalKey() + { + // language=yaml + AddChangelog("12345-fix.yaml", """ + title: Fix search performance + type: bug-fix + prs: + - https://github.com/elastic/elasticsearch/pull/12345 + """); + + var targets = _service.DiscoverUploadTargets(_collector, _changelogDir, "elastic", "elasticsearch", "main"); + + targets.Should().ContainSingle(); + targets[0].S3Key.Should().Be("changelog/elastic/elasticsearch/main/12345.yaml", + "canonical key is derived from PR number, not the authored filename"); + _collector.Warnings.Should().Be(0); + } + + [Fact] + public void DiscoverUploadTargets_NoteFile_PassesThroughVerbatim() + { + // language=yaml + AddChangelog("note-slow-rollover.yaml", """ + title: Known issue with rollover + type: known-issue + products: + - product: elasticsearch + target: 9.2.0 + """); + + var targets = _service.DiscoverUploadTargets(_collector, _changelogDir, "elastic", "elasticsearch", "main"); + + targets.Should().ContainSingle(); + targets[0].S3Key.Should().Be("changelog/elastic/elasticsearch/main/note-slow-rollover.yaml", + "note-* files are their own anchor and use verbatim filenames"); + } + + [Fact] + public void DiscoverUploadTargets_MultiPrEntry_AddsMarkerTargets() + { + // language=yaml + AddChangelog("100.yaml", """ + title: Multi-PR feature + type: feature + prs: + - https://github.com/elastic/elasticsearch/pull/100 + - https://github.com/elastic/elasticsearch/pull/200 + """); + + var targets = _service.DiscoverUploadTargets(_collector, _changelogDir, "elastic", "elasticsearch", "main"); + + targets.Should().HaveCount(2, "one primary + one marker"); + targets.Should().Contain(t => t.S3Key == "changelog/elastic/elasticsearch/main/100.yaml" && t.InlineContent == null, + "primary entry has a local file"); + var marker = targets.SingleOrDefault(t => t.S3Key == "changelog/elastic/elasticsearch/main/200.yaml"); + marker.Should().NotBeNull(); + marker!.InlineContent.Should().NotBeNullOrEmpty("marker has inline content, no local file"); + var markerEntry = ReleaseNotesSerialization.DeserializeEntry(marker.InlineContent!); + markerEntry.Link.Should().Be("100"); + } } From f94c26f6ad64487a3d389bee912ff798df7e7bac Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 25 Aug 2026 18:45:52 +0200 Subject: [PATCH 09/17] style: dotnet format Co-Authored-By: Claude Sonnet 4.6 --- .../Uploading/ChangelogUploadServiceTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs b/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs index 81dca91781..aa9b40c649 100644 --- a/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs +++ b/tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs @@ -921,8 +921,8 @@ public void DiscoverUploadTargets_MultiPrEntry_AddsMarkerTargets() "primary entry has a local file"); var marker = targets.SingleOrDefault(t => t.S3Key == "changelog/elastic/elasticsearch/main/200.yaml"); marker.Should().NotBeNull(); - marker!.InlineContent.Should().NotBeNullOrEmpty("marker has inline content, no local file"); - var markerEntry = ReleaseNotesSerialization.DeserializeEntry(marker.InlineContent!); + marker.InlineContent.Should().NotBeNullOrEmpty("marker has inline content, no local file"); + var markerEntry = ReleaseNotesSerialization.DeserializeEntry(marker.InlineContent); markerEntry.Link.Should().Be("100"); } } From 1715905eb7b5b9ede5e9f15464b180f82956a91c Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 25 Aug 2026 18:48:48 +0200 Subject: [PATCH 10/17] fix: use InvariantCulture for int.ToString in canonical key derivation Co-Authored-By: Claude Sonnet 4.6 --- .../Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs | 2 +- .../Elastic.Changelog/Uploading/ChangelogUploadService.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index c492656565..c97fb973ba 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -190,7 +190,7 @@ private static (string? CanonicalKey, IReadOnlyList<(string Key, string Content) if (prNumbers.Count == 1) return (canonicalKey, []); - var markerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = primaryPr.ToString() }); + var markerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = primaryPr.ToString(System.Globalization.CultureInfo.InvariantCulture) }); var markers = prNumbers .Skip(1) .Select(pr => (keyPrefix + $"{pr}.yaml", markerContent)) diff --git a/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs b/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs index 77a4c6bcbf..9d8a177156 100644 --- a/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs +++ b/src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs @@ -233,7 +233,7 @@ internal static (string CanonicalFileName, IReadOnlyList<(string FileName, strin if (prNumbers.Count == 1) return (canonicalFileName, []); - var markerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = primaryPr.ToString() }); + var markerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = primaryPr.ToString(System.Globalization.CultureInfo.InvariantCulture) }); var markers = prNumbers .Skip(1) .Select(pr => ($"{pr}.yaml", markerContent)) From ad25cbe80faa290958330b67c3808288b88d3525 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 11:38:23 +0200 Subject: [PATCH 11/17] =?UTF-8?q?fix(changelog):=20marker=20guard=20?= =?UTF-8?q?=E2=80=94=20allow=20link:=20alongside=20other=20fields,=20only?= =?UTF-8?q?=20short-circuit=20pure=20markers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- .../Scrubbing/ChangelogContentScrubber.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index c97fb973ba..e927b3c54a 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -97,17 +97,16 @@ private async Task ScrubChangelog(string key, string content, Cance var normalized = ReleaseNotesSerialization.NormalizeYaml(content); var entry = ReleaseNotesSerialization.DeserializeEntry(normalized); - // Marker: link: with no other content. Return unchanged — there is no URL to scrub. + // Pure marker: link: with no other content. Return unchanged — there is no URL to scrub. if (entry.Link != null) { var hasContent = !string.IsNullOrEmpty(entry.Title) || entry.Type != ChangelogEntryType.Invalid || entry.Products is { Count: > 0 } || entry.Prs is { Count: > 0 }; - if (hasContent) - throw new InvalidOperationException( - "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); - return new ScrubResult { Content = content }; + if (!hasContent) + return new ScrubResult { Content = content }; + // Has link: alongside other fields — fall through to normal scrubbing (link is preserved below). } // Derive canonical key and markers from the pre-scrub entry while prs: URLs are still intact. From 939ce0f1bf958acf51f0f704b50682b95cc9edd3 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 11:50:04 +0200 Subject: [PATCH 12/17] fix(changelog): restore marker-only guard in ScrubResult path Co-Authored-By: Claude Sonnet 4.6 --- .../Scrubbing/ChangelogContentScrubber.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index e927b3c54a..c97fb973ba 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -97,16 +97,17 @@ private async Task ScrubChangelog(string key, string content, Cance var normalized = ReleaseNotesSerialization.NormalizeYaml(content); var entry = ReleaseNotesSerialization.DeserializeEntry(normalized); - // Pure marker: link: with no other content. Return unchanged — there is no URL to scrub. + // Marker: link: with no other content. Return unchanged — there is no URL to scrub. if (entry.Link != null) { var hasContent = !string.IsNullOrEmpty(entry.Title) || entry.Type != ChangelogEntryType.Invalid || entry.Products is { Count: > 0 } || entry.Prs is { Count: > 0 }; - if (!hasContent) - return new ScrubResult { Content = content }; - // Has link: alongside other fields — fall through to normal scrubbing (link is preserved below). + if (hasContent) + throw new InvalidOperationException( + "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); + return new ScrubResult { Content = content }; } // Derive canonical key and markers from the pre-scrub entry while prs: URLs are still intact. From 06ff6c5e6e13c289d8f1d95e44cc5447bee382e6 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 12:50:10 +0200 Subject: [PATCH 13/17] fix(changelog): update MarkerScrubTests to use result.Content after ScrubResult return type change Co-Authored-By: Claude Sonnet 4.6 --- tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs index 47c20149b0..966232ad06 100644 --- a/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs +++ b/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs @@ -27,7 +27,7 @@ public async Task Marker_OnlyLink_PassesThroughUnchanged() var result = await _scrubber.ScrubAsync(key, content, Ctx); - result.Should().Be(content); + result.Content.Should().Be(content); } [Fact] @@ -38,7 +38,7 @@ public async Task Marker_OnlyLink_NoTrailingNewline_PassesThroughUnchanged() var result = await _scrubber.ScrubAsync(key, content, Ctx); - result.Should().Be(content); + result.Content.Should().Be(content); } [Fact] From e4d287b4d594b8156e2e22dec3fde4a3308ba7c8 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 13:11:00 +0200 Subject: [PATCH 14/17] fix(changelog): address PR 3930 review comments - Move BuildCanonicalKeyAndMarkers to after TryApplyChangelogEntry so private-only PRs stripped by the allowlist don't become the canonical key anchor or generate phantom markers - Wrap new Uri(prUrl) in ExtractPrNumber with try-catch so malformed PR URLs degrade to null rather than aborting upload/scrub discovery - Delete stale public marker objects when a multi-PR entry shrinks; reads the pre-existing canonical entry before overwriting and removes any marker keys no longer produced by the updated scrub result Co-Authored-By: Claude Sonnet 4.6 --- .../ReleaseNotes/ChangelogTextUtilities.cs | 21 +++-- .../Scrubbing/ChangelogContentScrubber.cs | 8 +- .../Scrubbing/ScrubberProcessor.cs | 92 +++++++++++++++++++ 3 files changed, 111 insertions(+), 10 deletions(-) diff --git a/src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs b/src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs index 0991e13171..c0026fe809 100644 --- a/src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs +++ b/src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs @@ -171,13 +171,20 @@ public static bool TitleNeedsDefensiveYamlQuoting(string? title) if (prUrl.StartsWith("https://github.com/", StringComparison.OrdinalIgnoreCase) || prUrl.StartsWith("http://github.com/", StringComparison.OrdinalIgnoreCase)) { - var uri = new Uri(prUrl); - var segments = uri.Segments; - // segments[0] is "/", segments[1] is "owner/", segments[2] is "repo/", segments[3] is "pull/", segments[4] is "123" - if (segments.Length >= 5 && - segments[3].Equals("pull/", StringComparison.OrdinalIgnoreCase) && - int.TryParse(segments[4].TrimEnd('/'), out var prNum)) - return prNum; + try + { + var uri = new Uri(prUrl); + var segments = uri.Segments; + // segments[0] is "/", segments[1] is "owner/", segments[2] is "repo/", segments[3] is "pull/", segments[4] is "123" + if (segments.Length >= 5 && + segments[3].Equals("pull/", StringComparison.OrdinalIgnoreCase) && + int.TryParse(segments[4].TrimEnd('/'), out var prNum)) + return prNum; + } + catch (UriFormatException) + { + // Malformed URL; fall through to return null. + } } // Handle short format: owner/repo#123 diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index c97fb973ba..73152bbac5 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -110,9 +110,6 @@ private async Task ScrubChangelog(string key, string content, Cance return new ScrubResult { Content = content }; } - // Derive canonical key and markers from the pre-scrub entry while prs: URLs are still intact. - var (canonicalKey, markers) = BuildCanonicalKeyAndMarkers(key, entry); - var bundledEntry = new BundledEntry { Type = entry.Type, @@ -134,6 +131,11 @@ private async Task ScrubChangelog(string key, string content, Cance out var sanitized, out var changed)) throw new InvalidOperationException($"Failed to apply allowlist to changelog entry; errors: {collector.Errors}"); + // Derive canonical key and markers AFTER allowlist filtering so private-only PRs that are + // stripped from public output don't become the primary anchor or generate stale markers. + var publicEntry = changed ? entry with { Prs = sanitized.Prs } : entry; + var (canonicalKey, markers) = BuildCanonicalKeyAndMarkers(key, publicEntry); + if (!changed) { _logger.LogInformation("Changelog entry had no private references, writing unchanged"); diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index 845a92d6fb..4568a91882 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -8,6 +8,7 @@ using Amazon.S3.Util; using Elastic.Changelog.Reconciliation; using Elastic.Documentation.Configuration.ReleaseNotes; +using Elastic.Documentation.ReleaseNotes; using Microsoft.Extensions.Logging; namespace Elastic.Changelog.Scrubbing; @@ -315,6 +316,11 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa { var scrubResult = await scrubber.ScrubAsync(key, source.Content, ctx); var publicKey = scrubResult.CanonicalKey ?? key; + + // Read the current public entry before overwriting so we can derive which + // marker objects it previously produced and delete any that are no longer needed. + var oldPublicContent = await TryGetPublicObject(publicKey, ctx); + await PutPublicObject(publicKey, scrubResult.Content, "application/yaml", ctx); if (scrubResult.CanonicalKey is not null) _logger.LogInformation("Scrubbed {Key} → canonical public key {CanonicalKey}", key, scrubResult.CanonicalKey); @@ -326,6 +332,10 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa await PutPublicObject(markerKey, markerContent, "application/yaml", ctx); _logger.LogInformation("Wrote marker {MarkerKey} → {PrimaryKey}", markerKey, publicKey); } + + // Remove marker objects that existed in the previous scrub result but are no + // longer produced (e.g. an entry shrank from 3 PRs to 1). + await DeleteStaleMarkersAsync(publicKey, oldPublicContent, scrubResult.Markers, ctx); } } else @@ -394,6 +404,88 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa } } + private async Task TryGetPublicObject(string key, Cancel ctx) + { + try + { + using var response = await s3Client.GetObjectAsync(new GetObjectRequest + { + BucketName = publicBucketName, + Key = key + }, ctx); + await using var stream = response.ResponseStream; + using var reader = new StreamReader(stream); + return await reader.ReadToEndAsync(ctx); + } + catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) + { + return null; + } + } + + private async Task DeleteStaleMarkersAsync( + string publicKey, + string? oldPublicContent, + IReadOnlyList<(string Key, string Content)> newMarkers, + Cancel ctx) + { + if (oldPublicContent is null) + return; + + IReadOnlyList oldMarkerKeys; + try + { + oldMarkerKeys = DeriveMarkerKeys(publicKey, oldPublicContent); + } + catch + { + // If the old content can't be parsed, we can't derive old markers — skip cleanup. + return; + } + + if (oldMarkerKeys.Count == 0) + return; + + var newMarkerKeySet = newMarkers.Select(m => m.Key).ToHashSet(StringComparer.Ordinal); + foreach (var staleKey in oldMarkerKeys) + { + if (newMarkerKeySet.Contains(staleKey)) + continue; + await DeletePublicObject(staleKey, ctx); + _logger.LogInformation("Deleted stale marker {StaleKey} (no longer referenced by {PrimaryKey})", staleKey, publicKey); + } + } + + private static IReadOnlyList DeriveMarkerKeys(string publicKey, string content) + { + var entry = ReleaseNotesSerialization.DeserializeEntry(content); + if (entry.IsMarker || entry.Prs is not { Count: > 0 }) + return []; + + var lastSlash = publicKey.LastIndexOf('/'); + if (lastSlash < 0) + return []; + var keyPrefix = publicKey[..(lastSlash + 1)]; + + var prNumbers = entry.Prs + .Select(pr => ChangelogTextUtilities.ExtractPrNumber(pr)) + .Where(n => n.HasValue) + .Select(n => n!.Value) + .Distinct() + .OrderBy(n => n) + .ToList(); + + if (prNumbers.Count <= 1) + return []; + + var primaryPr = prNumbers[0]; + return prNumbers + .Skip(1) + .Select(pr => $"{keyPrefix}{pr}.yaml") + .Where(k => !string.Equals(k, $"{keyPrefix}{primaryPr}.yaml", StringComparison.OrdinalIgnoreCase)) + .ToList(); + } + private async Task PutPublicObject(string key, string content, string contentType, Cancel ctx) => _ = await s3Client.PutObjectAsync(new PutObjectRequest { From 3e72879c47d081ed32acccb9bca9866b2fb93d1c Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 14:25:41 +0200 Subject: [PATCH 15/17] fix(changelog): source pointer + phantom-marker guard for canonical reconciliation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness issues raised in PR #3930 review: Issue 1 — phantom marker overwrite: a private marker derived from raw (pre-allowlist) PRs can race with the scrubber writing canonical public content at the same key, overwriting it. ScrubResult now carries IsMarker; the processor skips a pass-through marker write when canonical content already occupies the target public key. Issue 2 — stale canonical on delete: deleting the private source of a non-canonical entry (e.g. 12345-fix.yaml whose canonical public key is 12345.yaml) previously deleted only the non-existent source key from public, leaving the canonical and its markers stranded. The write path now places a source pointer (link: ) at the source key in the public bucket. The delete path reads that pointer, derives the canonical key, deletes its secondary-PR markers via DeleteStaleMarkersAsync, then deletes the canonical itself before removing the source pointer. Numeric-filename keys (pure PR markers or already-canonical entries) follow the existing path augmented with marker cleanup. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../Scrubbing/ChangelogContentScrubber.cs | 10 +- .../Scrubbing/ScrubberProcessor.cs | 120 ++++++++++++++++-- .../Scrubbing/ScrubberProcessorTests.cs | 92 +++++++++++++- 3 files changed, 206 insertions(+), 16 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 73152bbac5..257625688e 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -31,6 +31,14 @@ public record ScrubResult /// Empty for single-PR entries and bundles. /// public IReadOnlyList<(string Key, string Content)> Markers { get; init; } = []; + + /// + /// True when the content is a pass-through private marker (link: only). + /// The processor must not overwrite existing canonical public content at the same key + /// with a marker — private markers derived from raw (pre-allowlist) PR lists can be + /// stale and their canonical target may already be written to a different public key. + /// + public bool IsMarker { get; init; } } /// Rewrites private-bucket changelog YAML into its public, allowlist-scrubbed form. @@ -107,7 +115,7 @@ private async Task ScrubChangelog(string key, string content, Cance if (hasContent) throw new InvalidOperationException( "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); - return new ScrubResult { Content = content }; + return new ScrubResult { Content = content, IsMarker = true }; } var bundledEntry = new BundledEntry diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index 4568a91882..1f94e818d0 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -2,6 +2,7 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System.Globalization; using System.Net; using Amazon.S3; using Amazon.S3.Model; @@ -321,25 +322,91 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa // marker objects it previously produced and delete any that are no longer needed. var oldPublicContent = await TryGetPublicObject(publicKey, ctx); - await PutPublicObject(publicKey, scrubResult.Content, "application/yaml", ctx); - if (scrubResult.CanonicalKey is not null) - _logger.LogInformation("Scrubbed {Key} → canonical public key {CanonicalKey}", key, scrubResult.CanonicalKey); + // Issue 1 guard: a private marker derived from raw (pre-allowlist) PRs can race + // with the scrubber writing canonical public content at the same key. If canonical + // content already occupies the public key, skip the marker write so it cannot + // overwrite the canonical entry that the primary object's scrub already produced. + // A null return from TryDeserializeEntry (unparseable content) is treated as + // canonical — the safest assumption when we cannot classify the existing object. + var skipWrite = scrubResult.IsMarker + && oldPublicContent is not null + && TryDeserializeEntry(oldPublicContent)?.IsMarker != true; + + if (skipWrite) + { + _logger.LogInformation( + "Skipped pass-through marker {Key}: canonical content in public bucket takes precedence", key); + } else - _logger.LogInformation("Scrubbed and wrote {Key} to public bucket", key); - - foreach (var (markerKey, markerContent) in scrubResult.Markers) { - await PutPublicObject(markerKey, markerContent, "application/yaml", ctx); - _logger.LogInformation("Wrote marker {MarkerKey} → {PrimaryKey}", markerKey, publicKey); + await PutPublicObject(publicKey, scrubResult.Content, "application/yaml", ctx); + if (scrubResult.CanonicalKey is not null) + { + _logger.LogInformation("Scrubbed {Key} → canonical public key {CanonicalKey}", key, scrubResult.CanonicalKey); + // Issue 2: write a source pointer at the source key in the public bucket so + // that a delete event for 'key' can trace back to the canonical key and clean + // it up (see delete path below). The pointer uses the same link: format as + // secondary-PR markers; the non-canonical filename makes it distinguishable. + await WriteSourcePointerAsync(key, scrubResult.CanonicalKey, ctx); + } + else + _logger.LogInformation("Scrubbed and wrote {Key} to public bucket", key); + + foreach (var (markerKey, markerContent) in scrubResult.Markers) + { + await PutPublicObject(markerKey, markerContent, "application/yaml", ctx); + _logger.LogInformation("Wrote marker {MarkerKey} → {PrimaryKey}", markerKey, publicKey); + } + + // Remove marker objects that existed in the previous scrub result but are no + // longer produced (e.g. an entry shrank from 3 PRs to 1). + await DeleteStaleMarkersAsync(publicKey, oldPublicContent, scrubResult.Markers, ctx); } - - // Remove marker objects that existed in the previous scrub result but are no - // longer produced (e.g. an entry shrank from 3 PRs to 1). - await DeleteStaleMarkersAsync(publicKey, oldPublicContent, scrubResult.Markers, ctx); } } else { + // Issue 2: the private object is gone. Read the public bucket to discover what + // was previously written for this source key so we can clean it up completely. + // + // Two sub-cases: + // (a) Source key was non-canonical: the scrubber wrote canonical content to a + // different public key and left a source pointer (link: ) at 'key'. Read + // the pointer, derive the canonical key, delete it and all its markers, then + // delete the pointer. Non-canonical source keys always have a non-integer + // filename (e.g. "12345-fix.yaml"), which differentiates them from PR markers. + // (b) Source key was canonical or a secondary PR marker: delete its markers (if + // any), then delete the key itself. + var publicContent = await TryGetPublicObject(key, ctx); + if (publicContent is not null && !IsNumericYamlKey(key)) + { + // Non-canonical source key — check for a source pointer. + var entry = TryDeserializeEntry(publicContent); + if (entry?.IsMarker == true) + { + var lastSlash = key.LastIndexOf('/'); + var keyPrefix = lastSlash >= 0 ? key[..(lastSlash + 1)] : string.Empty; + var canonicalKey = $"{keyPrefix}{entry.Link}.yaml"; + var canonicalContent = await TryGetPublicObject(canonicalKey, ctx); + await DeleteStaleMarkersAsync(canonicalKey, canonicalContent, [], ctx); + await DeletePublicObject(canonicalKey, ctx); + _logger.LogInformation( + "Source pointer {Key} traced to canonical {CanonicalKey}; deleted canonical and its markers", + key, canonicalKey); + } + else + { + // Non-canonical key but not a pointer (e.g. a note-* file). + await DeleteStaleMarkersAsync(key, publicContent, [], ctx); + } + } + else if (publicContent is not null) + { + // Numeric key: canonical entry or secondary-PR marker. Either way, clean up any + // markers the canonical may have emitted before deleting the entry itself. + await DeleteStaleMarkersAsync(key, publicContent, [], ctx); + } + await DeletePublicObject(key, ctx); _logger.LogInformation("Private {Key} is gone; removed its public copy", key); } @@ -404,6 +471,35 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa } } + private static ChangelogEntry? TryDeserializeEntry(string? content) + { + if (content is null) + return null; + try + { return ReleaseNotesSerialization.DeserializeEntry(content); } + catch { return null; } + } + + private static bool IsNumericYamlKey(string key) + { + var lastSlash = key.LastIndexOf('/'); + var fileName = lastSlash >= 0 ? key[(lastSlash + 1)..] : key; + var stem = Path.GetFileNameWithoutExtension(fileName); + return int.TryParse(stem, NumberStyles.None, CultureInfo.InvariantCulture, out _); + } + + private async Task WriteSourcePointerAsync(string sourceKey, string canonicalKey, Cancel ctx) + { + var lastSlash = canonicalKey.LastIndexOf('/'); + var canonicalFileName = lastSlash >= 0 ? canonicalKey[(lastSlash + 1)..] : canonicalKey; + var stem = Path.GetFileNameWithoutExtension(canonicalFileName); + if (!int.TryParse(stem, NumberStyles.None, CultureInfo.InvariantCulture, out _)) + return; // Not a PR-based canonical key; nothing to point to. + var pointerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = stem }); + await PutPublicObject(sourceKey, pointerContent, "application/yaml", ctx); + _logger.LogInformation("Wrote source pointer {SourceKey} → canonical {CanonicalKey}", sourceKey, canonicalKey); + } + private async Task TryGetPublicObject(string key, Cancel ctx) { try diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs index b70e763430..0ade7aceca 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs @@ -275,7 +275,7 @@ public async Task Process_SourceChangingMidFlight_IsDetectedByPostWriteValidatio } [Fact] - public async Task Process_CanonicalKeyEntry_WritesToCanonicalPublicKey() + public async Task Process_CanonicalKeyEntry_WritesToCanonicalPublicKeyAndSourcePointer() { // Scrubber says the private key 12345-fix.yaml should be written to public as 12345.yaml. const string privateKey = "changelog/elastic/elasticsearch/main/12345-fix.yaml"; @@ -290,8 +290,12 @@ public async Task Process_CanonicalKeyEntry_WritesToCanonicalPublicKey() failed.Should().BeEmpty(); _s3.ContentOf(PublicBucket, canonicalKey).Should().Be("scrubbed: entry-content", "canonical key must receive the scrubbed content"); - _s3.Exists(PublicBucket, privateKey).Should().BeFalse( - "private (non-canonical) key must not appear in the public bucket"); + // A source pointer is written at the source key so the delete path can trace back to the + // canonical key when the private object is eventually removed. + _s3.Exists(PublicBucket, privateKey).Should().BeTrue( + "source pointer must exist so delete events can trace to the canonical key"); + _s3.ContentOf(PublicBucket, privateKey).Should().Contain("link:", + "source pointer must be a link marker pointing to the canonical PR number"); } [Fact] @@ -441,6 +445,88 @@ public async Task Process_NoteFile_ScrubbedAndNotesReconcileTriggered() _s3.Exists(PublicBucket, "changelog/elastic/elasticsearch/notes-9.0.0.json").Should().BeTrue(); } + [Fact] + public async Task Process_PassThroughMarker_DoesNotOverwriteExistingCanonicalContent() + { + // Issue 1: a private marker derived from raw (pre-allowlist) PRs can arrive after the + // canonical public entry has already been written. The marker write must be skipped so + // it cannot overwrite canonical content. + const string markerKey = "changelog/elastic/elasticsearch/main/20.yaml"; + _ = _s3.Seed(PrivateBucket, markerKey, "link: \"10\"\n"); + _ = _s3.Seed(PublicBucket, markerKey, "scrubbed canonical content at 20"); + _ = A.CallTo(() => _scrubber.ScrubAsync(markerKey, A._, A._)) + .ReturnsLazily((string _, string content, Cancel _) => + Task.FromResult(new ScrubResult { Content = content, IsMarker = true })); + + var failed = await _processor.ProcessAsync([Message("ObjectCreated:Put", markerKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.ContentOf(PublicBucket, markerKey).Should().Be( + "scrubbed canonical content at 20", + "pass-through marker must not overwrite existing canonical content"); + } + + [Fact] + public async Task Process_DeleteOfNonCanonicalSource_DeletesCanonicalAndMarkersThroughSourcePointer() + { + // Issue 2: when the private source key is non-canonical (e.g. 12345-fix.yaml), the + // scrubber writes canonical content to a different public key (12345.yaml) and leaves a + // source pointer at the source key. On delete, the processor must trace that pointer and + // remove the canonical and all its markers. + const string privateKey = "changelog/elastic/elasticsearch/main/12345-fix.yaml"; + const string canonicalKey = "changelog/elastic/elasticsearch/main/12345.yaml"; + const string markerKey = "changelog/elastic/elasticsearch/main/67890.yaml"; + // Simulate state left by the prior write: source pointer at privateKey, canonical at + // canonicalKey, and a secondary-PR marker at markerKey. + _ = _s3.Seed(PublicBucket, privateKey, "link: \"12345\"\n"); + _ = _s3.Seed(PublicBucket, canonicalKey, + // language=yaml + """ + type: enhancement + title: "Example" + prs: + - https://github.com/elastic/elasticsearch/pull/12345 + - https://github.com/elastic/elasticsearch/pull/67890 + """); + _ = _s3.Seed(PublicBucket, markerKey, "link: \"12345\"\n"); + + var failed = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", privateKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.Exists(PublicBucket, privateKey).Should().BeFalse("source pointer must be deleted"); + _s3.Exists(PublicBucket, canonicalKey).Should().BeFalse("canonical entry must be deleted"); + _s3.Exists(PublicBucket, markerKey).Should().BeFalse("secondary-PR marker must be deleted via DeleteStaleMarkersAsync"); + } + + [Fact] + public async Task Process_DeleteOfCanonicalEntryWithMarkers_DeletesMarkersBeforeCanonical() + { + // When the canonical entry's source key is itself the canonical key (numeric filename), + // the delete path must also clean up its secondary-PR markers in the public bucket. + const string canonicalKey = "changelog/elastic/elasticsearch/main/100.yaml"; + const string marker200 = "changelog/elastic/elasticsearch/main/200.yaml"; + const string marker300 = "changelog/elastic/elasticsearch/main/300.yaml"; + _ = _s3.Seed(PublicBucket, canonicalKey, + // language=yaml + """ + type: enhancement + title: "Multi-PR entry" + prs: + - https://github.com/elastic/elasticsearch/pull/100 + - https://github.com/elastic/elasticsearch/pull/200 + - https://github.com/elastic/elasticsearch/pull/300 + """); + _ = _s3.Seed(PublicBucket, marker200, "link: \"100\"\n"); + _ = _s3.Seed(PublicBucket, marker300, "link: \"100\"\n"); + + var failed = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", canonicalKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.Exists(PublicBucket, canonicalKey).Should().BeFalse("canonical entry must be deleted"); + _s3.Exists(PublicBucket, marker200).Should().BeFalse("PR-200 marker must be deleted"); + _s3.Exists(PublicBucket, marker300).Should().BeFalse("PR-300 marker must be deleted"); + } + // language=yaml private static string BundleYaml() => """ products: From 23228a0adb10d1708a0f83a0b1630ab0e40498e0 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 15:15:40 +0200 Subject: [PATCH 16/17] fix(changelog): add source-redirect discriminator and yaml-only numeric key guard Two correctness fixes flagged in the PR #3937 bot review: 1. Source-pointer ambiguity: plain link:-only markers at non-numeric keys were indistinguishable from scrubber-written source pointers, so the delete path could follow a legitimate PR marker and spuriously delete canonical content. Added SourceRedirect=true (serialized as source-redirect: true) written by WriteSourcePointerAsync; the delete path now checks SourceRedirect, not IsMarker. 2. .yml extension bypass: IsNumericYamlKey accepted any numeric stem regardless of extension, so a .yml source file (e.g. 12345.yml) was treated as canonical and bypassed source-pointer tracing on delete, leaving its canonical stranded. Fixed by also requiring a .yaml extension. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../ReleaseNotes/ChangelogEntry.cs | 8 ++++ .../ReleaseNotes/ReleaseNotesSerialization.cs | 7 ++- .../ReleaseNotes/ChangelogEntry.cs | 6 +++ .../Scrubbing/ScrubberProcessor.cs | 12 +++-- .../Scrubbing/ScrubberProcessorTests.cs | 45 ++++++++++++++++++- 5 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs index f1f0571b66..f0358a50aa 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs @@ -34,6 +34,14 @@ public record ChangelogEntryDto /// Written by the pipeline for non-primary PRs in a multi-PR entry; never hand-authored. /// public string? Link { get; set; } + + /// + /// When true, this public-bucket object is a scrubber-written source pointer that traces back + /// to a canonical public key. Distinguishes source pointers from ordinary link-only PR markers + /// so the delete path does not spuriously follow a regular marker to its canonical target. + /// + [YamlMember(Alias = "source-redirect", ApplyNamingConventions = false)] + public bool? SourceRedirect { get; set; } } /// diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs index 236a9cafbd..0aae497c7f 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs @@ -166,7 +166,8 @@ private static string ToYamlDoubleQuotedString(string s) Action = dto.Action, FeatureId = dto.FeatureId, Highlight = dto.Highlight, - Link = dto.Link + Link = dto.Link, + SourceRedirect = dto.SourceRedirect ?? false }; private static ChangelogEntry ToEntry(BundledEntry entry) => new() @@ -287,8 +288,10 @@ private static ChangelogEntryType ParseEntryType(string? value) private static ChangelogEntryDto ToDto(ChangelogEntry entry) { // Marker entries are link-only; emitting any other field would violate the marker contract. + // Source pointers also carry source-redirect: true so the delete path can distinguish them + // from ordinary PR markers without ambiguity. if (entry.IsMarker) - return new ChangelogEntryDto { Link = entry.Link }; + return new ChangelogEntryDto { Link = entry.Link, SourceRedirect = entry.SourceRedirect ? true : null }; return new ChangelogEntryDto { diff --git a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs index 25062776c6..ec1a5a549f 100644 --- a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs @@ -56,6 +56,12 @@ public record ChangelogEntry /// True when this entry is a pipeline-written marker that redirects to another PR's entry. public bool IsMarker => Link is not null; + /// + /// When true, this entry is a scrubber-written source pointer in the public bucket that traces + /// to a canonical key. Distinguishes source pointers from ordinary link-only PR markers. + /// + public bool SourceRedirect { get; init; } + /// /// Converts this ChangelogEntry to a BundledEntry for embedding in bundles. /// File property is set to null; set it separately using a 'with' expression. diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index fc659b47e0..ad114bc53b 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -380,9 +380,11 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa var publicContent = await TryGetPublicObject(key, ctx); if (publicContent is not null && !IsNumericYamlKey(key)) { - // Non-canonical source key — check for a source pointer. + // Non-canonical source key — check for a scrubber-written source pointer. + // Only entries with SourceRedirect=true are source pointers; a plain link: field + // alone is an ordinary PR marker that must not trigger canonical deletion. var entry = TryDeserializeEntry(publicContent); - if (entry?.IsMarker == true) + if (entry?.SourceRedirect == true) { var lastSlash = key.LastIndexOf('/'); var keyPrefix = lastSlash >= 0 ? key[..(lastSlash + 1)] : string.Empty; @@ -484,6 +486,10 @@ private static bool IsNumericYamlKey(string key) { var lastSlash = key.LastIndexOf('/'); var fileName = lastSlash >= 0 ? key[(lastSlash + 1)..] : key; + // Only .yaml (not .yml) files are written as canonical PR keys by the pipeline. + // A .yml source file is always non-canonical; treat it as needing pointer tracing. + if (!fileName.EndsWith(".yaml", StringComparison.OrdinalIgnoreCase)) + return false; var stem = Path.GetFileNameWithoutExtension(fileName); return int.TryParse(stem, NumberStyles.None, CultureInfo.InvariantCulture, out _); } @@ -495,7 +501,7 @@ private async Task WriteSourcePointerAsync(string sourceKey, string canonicalKey var stem = Path.GetFileNameWithoutExtension(canonicalFileName); if (!int.TryParse(stem, NumberStyles.None, CultureInfo.InvariantCulture, out _)) return; // Not a PR-based canonical key; nothing to point to. - var pointerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = stem }); + var pointerContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = stem, SourceRedirect = true }); await PutPublicObject(sourceKey, pointerContent, "application/yaml", ctx); _logger.LogInformation("Wrote source pointer {SourceKey} → canonical {CanonicalKey}", sourceKey, canonicalKey); } diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs index 0ade7aceca..4aacab7689 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs @@ -8,6 +8,8 @@ using Elastic.Changelog.Scrubbing; using Elastic.Changelog.Tests.Reconciliation; using Elastic.Changelog.Uploading; +using Elastic.Documentation.Configuration.ReleaseNotes; +using Elastic.Documentation.ReleaseNotes; using FakeItEasy; using Microsoft.Extensions.Logging.Abstractions; @@ -478,7 +480,9 @@ public async Task Process_DeleteOfNonCanonicalSource_DeletesCanonicalAndMarkersT const string markerKey = "changelog/elastic/elasticsearch/main/67890.yaml"; // Simulate state left by the prior write: source pointer at privateKey, canonical at // canonicalKey, and a secondary-PR marker at markerKey. - _ = _s3.Seed(PublicBucket, privateKey, "link: \"12345\"\n"); + // The source pointer must carry source-redirect: true to be distinguishable from a plain marker. + _ = _s3.Seed(PublicBucket, privateKey, + ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = "12345", SourceRedirect = true })); _ = _s3.Seed(PublicBucket, canonicalKey, // language=yaml """ @@ -527,6 +531,45 @@ public async Task Process_DeleteOfCanonicalEntryWithMarkers_DeletesMarkersBefore _s3.Exists(PublicBucket, marker300).Should().BeFalse("PR-300 marker must be deleted"); } + [Fact] + public async Task Process_DeleteOfNonCanonicalSourceWithPlainMarker_DoesNotDeleteCanonical() + { + // Regression guard for source-pointer ambiguity: a plain link: marker at a non-numeric key + // must NOT be treated as a source pointer. Only objects with source-redirect: true trigger + // canonical deletion; otherwise any migrated marker could accidentally nuke live content. + const string privateKey = "changelog/elastic/elasticsearch/main/12345-fix.yaml"; + const string canonicalKey = "changelog/elastic/elasticsearch/main/12345.yaml"; + // Public object has link: but no source-redirect: true — it's a plain marker, not a pointer. + _ = _s3.Seed(PublicBucket, privateKey, "link: \"12345\"\n"); + _ = _s3.Seed(PublicBucket, canonicalKey, "type: enhancement\ntitle: Real\n"); + + var failed = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", privateKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.Exists(PublicBucket, privateKey).Should().BeFalse("the source-key public object is deleted"); + _s3.Exists(PublicBucket, canonicalKey).Should().BeTrue( + "a plain link: marker must not trigger canonical deletion — only source-redirect: true does"); + } + + [Fact] + public async Task Process_DeleteOfYmlSourceKey_TracesSourcePointerToCanonical() + { + // .yml files are never canonical PR keys; IsNumericYamlKey must return false for them + // even when the stem is numeric, so source-pointer tracing runs correctly. + const string privateKey = "changelog/elastic/elasticsearch/main/12345.yml"; + const string canonicalKey = "changelog/elastic/elasticsearch/main/12345.yaml"; + _ = _s3.Seed(PublicBucket, privateKey, + ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = "12345", SourceRedirect = true })); + _ = _s3.Seed(PublicBucket, canonicalKey, "type: enhancement\ntitle: Real\n"); + + var failed = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", privateKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.Exists(PublicBucket, privateKey).Should().BeFalse("source pointer is cleaned up"); + _s3.Exists(PublicBucket, canonicalKey).Should().BeFalse( + "canonical must be deleted via pointer tracing — .yml stem being numeric must not block this"); + } + // language=yaml private static string BundleYaml() => """ products: From 418f5915680b799b3680c6ce6497eadb1514437a Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Wed, 26 Aug 2026 15:41:09 +0200 Subject: [PATCH 17/17] fix(changelog): strip source-redirect from private markers and check it before filename heuristics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more correctness fixes from the PR #3937 bot review: 1. source-redirect forgery: private-authored markers with source-redirect: true would pass through unchanged and impersonate scrubber-written source pointers on delete, triggering spurious canonical deletion. Fix: re-serialize pass-through markers in ChangelogContentScrubber so only the link: value survives — source-redirect is processor-owned and never emitted from private input. 2. Numeric .yaml source pointer bypassed by filename gate: a source key like 12345.yaml can carry a source pointer (e.g. when PRs [100, 12345] make 100.yaml canonical). The old !IsNumericYamlKey guard would skip pointer tracing, orphaning 100.yaml. Fix: remove the filename gate; check entry.SourceRedirect first regardless of shape, then fall through to marker/canonical cleanup. IsNumericYamlKey removed as dead code. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../Scrubbing/ChangelogContentScrubber.cs | 9 ++- .../Scrubbing/ScrubberProcessor.cs | 66 ++++++------------- .../Changelogs/MarkerScrubTests.cs | 18 +++-- .../ChangelogContentScrubberTests.cs | 20 +++++- .../Scrubbing/ScrubberProcessorTests.cs | 25 ++++++- 5 files changed, 83 insertions(+), 55 deletions(-) diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 257625688e..ad1ccc22a6 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -105,7 +105,11 @@ private async Task ScrubChangelog(string key, string content, Cance var normalized = ReleaseNotesSerialization.NormalizeYaml(content); var entry = ReleaseNotesSerialization.DeserializeEntry(normalized); - // Marker: link: with no other content. Return unchanged — there is no URL to scrub. + // Marker: link: with no other content. + // Re-serialize rather than passing raw content through so private-authored fields + // (e.g. source-redirect: true) are stripped. source-redirect is processor-owned and + // must never be settable by private input — if it were passed through, a forged marker + // could impersonate a scrubber-written source pointer and trigger canonical deletion. if (entry.Link != null) { var hasContent = !string.IsNullOrEmpty(entry.Title) @@ -115,7 +119,8 @@ private async Task ScrubChangelog(string key, string content, Cance if (hasContent) throw new InvalidOperationException( "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); - return new ScrubResult { Content = content, IsMarker = true }; + var safeContent = ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = entry.Link }); + return new ScrubResult { Content = safeContent, IsMarker = true }; } var bundledEntry = new BundledEntry diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index ad114bc53b..1c4c5bcfcb 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -366,46 +366,34 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa } else { - // Issue 2: the private object is gone. Read the public bucket to discover what - // was previously written for this source key so we can clean it up completely. + // Private object is gone. Read the public bucket to discover what was previously + // written for this source key so we can clean it up completely. // // Two sub-cases: - // (a) Source key was non-canonical: the scrubber wrote canonical content to a - // different public key and left a source pointer (link: ) at 'key'. Read - // the pointer, derive the canonical key, delete it and all its markers, then - // delete the pointer. Non-canonical source keys always have a non-integer - // filename (e.g. "12345-fix.yaml"), which differentiates them from PR markers. - // (b) Source key was canonical or a secondary PR marker: delete its markers (if - // any), then delete the key itself. + // (a) Source key carries a scrubber-written source pointer (SourceRedirect == true): + // the scrubber routed canonical content to a different public key and left a + // breadcrumb here. Trace the pointer to the canonical key, delete it and all its + // markers, then delete the pointer itself. SourceRedirect is processor-owned — + // the scrubber strips it from private-authored markers on the way through. + // (b) Any other key (canonical entry, secondary-PR marker, note-* file): clean up + // any markers the canonical may have emitted and delete the key itself. var publicContent = await TryGetPublicObject(key, ctx); - if (publicContent is not null && !IsNumericYamlKey(key)) + var publicEntry = publicContent is not null ? TryDeserializeEntry(publicContent) : null; + + if (publicEntry?.SourceRedirect == true) { - // Non-canonical source key — check for a scrubber-written source pointer. - // Only entries with SourceRedirect=true are source pointers; a plain link: field - // alone is an ordinary PR marker that must not trigger canonical deletion. - var entry = TryDeserializeEntry(publicContent); - if (entry?.SourceRedirect == true) - { - var lastSlash = key.LastIndexOf('/'); - var keyPrefix = lastSlash >= 0 ? key[..(lastSlash + 1)] : string.Empty; - var canonicalKey = $"{keyPrefix}{entry.Link}.yaml"; - var canonicalContent = await TryGetPublicObject(canonicalKey, ctx); - await DeleteStaleMarkersAsync(canonicalKey, canonicalContent, [], ctx); - await DeletePublicObject(canonicalKey, ctx); - _logger.LogInformation( - "Source pointer {Key} traced to canonical {CanonicalKey}; deleted canonical and its markers", - key, canonicalKey); - } - else - { - // Non-canonical key but not a pointer (e.g. a note-* file). - await DeleteStaleMarkersAsync(key, publicContent, [], ctx); - } + var lastSlash = key.LastIndexOf('/'); + var keyPrefix = lastSlash >= 0 ? key[..(lastSlash + 1)] : string.Empty; + var canonicalKey = $"{keyPrefix}{publicEntry.Link}.yaml"; + var canonicalContent = await TryGetPublicObject(canonicalKey, ctx); + await DeleteStaleMarkersAsync(canonicalKey, canonicalContent, [], ctx); + await DeletePublicObject(canonicalKey, ctx); + _logger.LogInformation( + "Source pointer {Key} traced to canonical {CanonicalKey}; deleted canonical and its markers", + key, canonicalKey); } else if (publicContent is not null) { - // Numeric key: canonical entry or secondary-PR marker. Either way, clean up any - // markers the canonical may have emitted before deleting the entry itself. await DeleteStaleMarkersAsync(key, publicContent, [], ctx); } @@ -482,18 +470,6 @@ private async Task ReconcileObjectAsync(string sourceBucket, string key, bool pa catch { return null; } } - private static bool IsNumericYamlKey(string key) - { - var lastSlash = key.LastIndexOf('/'); - var fileName = lastSlash >= 0 ? key[(lastSlash + 1)..] : key; - // Only .yaml (not .yml) files are written as canonical PR keys by the pipeline. - // A .yml source file is always non-canonical; treat it as needing pointer tracing. - if (!fileName.EndsWith(".yaml", StringComparison.OrdinalIgnoreCase)) - return false; - var stem = Path.GetFileNameWithoutExtension(fileName); - return int.TryParse(stem, NumberStyles.None, CultureInfo.InvariantCulture, out _); - } - private async Task WriteSourcePointerAsync(string sourceKey, string canonicalKey, Cancel ctx) { var lastSlash = canonicalKey.LastIndexOf('/'); diff --git a/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs index 966232ad06..cbf7f16064 100644 --- a/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs +++ b/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs @@ -4,13 +4,16 @@ using AwesomeAssertions; using Elastic.Changelog.Scrubbing; +using Elastic.Documentation.Configuration.ReleaseNotes; using Microsoft.Extensions.Logging.Abstractions; namespace Elastic.Changelog.Tests.Changelogs; /// /// Verifies that handles link: markers correctly: -/// a bare marker passes through unchanged; a marker with content fields throws. +/// a bare marker is re-serialized with the link value preserved; a marker with content fields throws. +/// Re-serialization (rather than pass-through) strips private-authored fields such as +/// source-redirect: true so forged markers cannot impersonate scrubber-written source pointers. /// public class MarkerScrubTests { @@ -20,25 +23,30 @@ public class MarkerScrubTests private Cancel Ctx => TestContext.Current.CancellationToken; [Fact] - public async Task Marker_OnlyLink_PassesThroughUnchanged() + public async Task Marker_OnlyLink_PreservesLinkValue() { const string key = "changelog/elastic/elasticsearch/main/200.yaml"; const string content = "link: 100\n"; var result = await _scrubber.ScrubAsync(key, content, Ctx); - result.Content.Should().Be(content); + var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content); + entry.Link.Should().Be("100", "link: value must survive the scrub"); + entry.SourceRedirect.Should().BeFalse("source-redirect must not appear in scrubbed markers"); + result.IsMarker.Should().BeTrue(); } [Fact] - public async Task Marker_OnlyLink_NoTrailingNewline_PassesThroughUnchanged() + public async Task Marker_OnlyLink_NoTrailingNewline_PreservesLinkValue() { const string key = "changelog/elastic/elasticsearch/main/200.yaml"; const string content = "link: 100"; var result = await _scrubber.ScrubAsync(key, content, Ctx); - result.Content.Should().Be(content); + var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content); + entry.Link.Should().Be("100", "link: value must survive the scrub even without a trailing newline"); + result.IsMarker.Should().BeTrue(); } [Fact] diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs index ad2d993252..224bc1ab8c 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ChangelogContentScrubberTests.cs @@ -19,7 +19,8 @@ private static ChangelogContentScrubber Scrubber(IReadOnlyList? allowRep [Fact] public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved() { - // A marker is link: only — the scrubber must pass it through unchanged. + // A marker is link: only — the scrubber must pass it through (re-serialized to strip + // any private-authored fields, but link: itself is preserved). var yaml = "link: \"12345\"\n"; var scrubber = Scrubber(); @@ -32,6 +33,23 @@ public async Task ScrubAsync_MarkerEntry_LinkFieldPreserved() result.Markers.Should().BeEmpty(); } + [Fact] + public async Task ScrubAsync_MarkerEntryWithSourceRedirect_StripsSourceRedirectFromOutput() + { + // source-redirect: true is processor-owned metadata. If a private author adds it to a + // marker file, the scrubber must strip it so forged markers cannot impersonate + // scrubber-written source pointers and trigger spurious canonical deletion on delete. + var yaml = "link: \"12345\"\nsource-redirect: true\n"; + var scrubber = Scrubber(); + + var result = await scrubber.ScrubAsync("changelog/elastic/elasticsearch/main/12346.yaml", yaml, CancellationToken.None); + + var entry = ReleaseNotesSerialization.DeserializeEntry(result.Content); + entry.Link.Should().Be("12345", "link: must survive the scrub round-trip"); + entry.SourceRedirect.Should().BeFalse("source-redirect must be stripped from private-authored markers"); + result.IsMarker.Should().BeTrue(); + } + [Fact] public async Task ScrubAsync_NormalEntry_LinkIsNull() { diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs index 4aacab7689..88485b469b 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs @@ -554,8 +554,8 @@ public async Task Process_DeleteOfNonCanonicalSourceWithPlainMarker_DoesNotDelet [Fact] public async Task Process_DeleteOfYmlSourceKey_TracesSourcePointerToCanonical() { - // .yml files are never canonical PR keys; IsNumericYamlKey must return false for them - // even when the stem is numeric, so source-pointer tracing runs correctly. + // A .yml source key can have a source pointer even though the stem looks numeric. + // The delete path must check SourceRedirect first, not the filename shape. const string privateKey = "changelog/elastic/elasticsearch/main/12345.yml"; const string canonicalKey = "changelog/elastic/elasticsearch/main/12345.yaml"; _ = _s3.Seed(PublicBucket, privateKey, @@ -570,6 +570,27 @@ public async Task Process_DeleteOfYmlSourceKey_TracesSourcePointerToCanonical() "canonical must be deleted via pointer tracing — .yml stem being numeric must not block this"); } + [Fact] + public async Task Process_DeleteOfNumericYamlSourceKeyWithSourcePointer_TracesPointerToCanonical() + { + // Comment 4 regression: a numeric .yaml source key (e.g. 12345.yaml) can itself have a + // source pointer when the entry's min PR is smaller than 12345 (e.g. PRs [100, 12345] → + // canonical 100.yaml). The delete path must check SourceRedirect first — before any + // filename-shape heuristic — so canonical 100.yaml is not orphaned. + const string privateKey = "changelog/elastic/elasticsearch/main/12345.yaml"; + const string canonicalKey = "changelog/elastic/elasticsearch/main/100.yaml"; + _ = _s3.Seed(PublicBucket, privateKey, + ReleaseNotesSerialization.SerializeEntry(new ChangelogEntry { Link = "100", SourceRedirect = true })); + _ = _s3.Seed(PublicBucket, canonicalKey, "type: enhancement\ntitle: Real\n"); + + var failed = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", privateKey)], Ctx); + + failed.Should().BeEmpty(); + _s3.Exists(PublicBucket, privateKey).Should().BeFalse("source pointer is cleaned up"); + _s3.Exists(PublicBucket, canonicalKey).Should().BeFalse( + "canonical must be deleted via pointer tracing even when the source key is numeric"); + } + // language=yaml private static string BundleYaml() => """ products: