Skip to content

Add changelog bundle support for hiding private links#3002

Merged
lcawl merged 13 commits intomainfrom
sanitize-links
Apr 1, 2026
Merged

Add changelog bundle support for hiding private links#3002
lcawl merged 13 commits intomainfrom
sanitize-links

Conversation

@lcawl
Copy link
Copy Markdown
Contributor

@lcawl lcawl commented Mar 30, 2026

Summary

Adds opt-in bundle-time private link sanitization for changelogs.
When enabled, PR and issue links that point at repositories marked private: true in assembler.yml are rewritten to quoted # PRIVATE: … sentinels in bundle YAML. Renderers skip those sentinels in output while keeping them for audit purposes.
Default behavior is unchanged (sanitize_private_links defaults to false).

A new link-visibility option is also added to the changelog directive (equivalent to the changelog render command's --input visibility options) so that teams working from private repos have the option of showing public repo links (if any) after sanitizing the private links. Previously the directive hid all links from bundles derived from private repos.

Configuration & CLI

  • changelog.yml: bundle.sanitize_private_links (default false); optional per-profile sanitize_private_links override.
  • changelog bundle command supports --sanitize-private-links / --no-sanitize-private-links options (or the new sanitize_private_links config settings for profile-based comands
  • bundle.resolve must be true ( or --resolve command option specified) when sanitization is on; otherwise the command errors.
  • Registry: Every referenced repo must appear under assembler.yml references (unknown repos fail the bundle). private defaults to false if omitted.

Implementation highlights

  • PrivateChangelogLinkSanitizer: Parses refs via ChangelogTextUtilities.TryGetGitHubRepo, applies assembler registry + privacy checks, quotes sentinels in YAML.
  • ChangelogBundlingService: Runs sanitization after bundle rules when enabled and resolved; loads assembler config for validation.
  • changelog gh-release: No sanitize CLI flags; respects bundle.sanitize_private_links and bundle.resolve from config when creating the bundle (same bundling pipeline as option-based bundle).
  • changelog bundle-amend: Applies the same sanitization when config enables it and the amend is resolved; no sanitize CLI flags.
  • ChangelogTextUtilities: FormatPrLink / FormatIssueLink omit output for # PRIVATE: prefixes; TryGetGitHubRepo for URLs, owner/repo#n, and bare numbers.

{changelog} directive

  • New :link-visibility: option: auto | keep-links | hide-links (aligns with render-time link hiding vs. bundle-time target sanitization).

Config & docs

  • config/assembler.yml: e.g. kibana-team entry for private registry / link filtering.
  • config/changelog.example.yml: bundle + profile examples for sanitize_private_links.
  • Docs: docs/contribute/changelog.md, docs/syntax/changelog.md, docs/cli/release/changelog-bundle.md, changelog-gh-release.md, changelog-bundle-amend.md (behavior, CLI matrix, limitations).

Tests

  • PrivateChangelogLinkSanitizerTests: parsing, sentinel formatting, sanitize/unknown-repo behavior.
  • AssemblerConfigurationYamlTests: repo config/assembler.yml deserializes (keeps registry YAML valid in CI).

Steps to test

  1. Create changelog configuration file and a bunch of changelogs. For example, check out Kibana release notes grouped by team label kibana#258941

  2. Update the changelog configuration file to include the new option (ensure that resolve: true is also set):

     # When true, PR/issue links to repos marked private in assembler.yml are rewritten in bundle output
     # (requires resolve: true). Option-based: --sanitize-private-links / --no-sanitize-private-links.
     sanitize_private_links: true
  3. Re-create the bundles, for example:

     /path/to/.artifacts/publish/docs-builder/release/docs-builder changelog bundle kibana-release 9.3.0 ./docs/9.3.0-kibana.txt 

    This command correctly fails because there's a repo mentioned in one of the changelogs that doesn't exist in assembler.yml:

    Error: Repository xxx referenced in a changelog issue is not listed in assembler.yml references. Add it under references with private: true or false. Ensure assembler configuration is up to date (local ./config, embedded binary, or --configuration-source).
  4. Add the appropriate repos to the assembler.yml then re-run the bundle command. NOTE: You must re-run the publishbinaries command to pick up these changes.

  5. Verify that the private links have been commented out. For example:

    - file:
        name: 241928.yaml
        ...
      type: enhancement
      ...
      prs:
        - https://github.com/elastic/kibana/pull/241928
      issues:
        - '# PRIVATE: https://github.com/elastic/kibana-team/issues/2083'
  6. Generate more test bundles, for example:

    /path/to/docs-builder/.artifacts/publish/docs-builder/release/docs-builder changelog bundle security-release 9.3.0 ./docs/9.3.0-security.txt
    /path/to/docs-builder/.artifacts/publish/docs-builder/release/docs-builder changelog bundle observability-release 9.3.0 ./docs/9.3.0-observability.txt
  7. Verify that the private links do not appear in the output via changelog directives. For example, the private link is omitted here:
    image

  8. Verify that the private links do not appear in the output via changelog render commands. For example:

    /path/to/docs-builder/.artifacts/publish/docs-builder/release/docs-builder changelog render --input "./docs/releases/kibana/9.3.0.yaml" --output ./docs/release-notes/_snippets/   

    When it generates markdown output (by default), the private link is successfully omitted:

    * Allow users to edit schedule config. [#241928](https://github.com/elastic/kibana/pull/241928)  

    When run with the --file-type asciidoc option, the same is also true:

    * Allow users to edit schedule config. {elastic-pull}241928[#241928]  

Generative AI disclosure

  1. Did you use a generative AI (GenAI) tool to assist in creating this contribution?
  • Yes
  • No
  1. If you answered "Yes" to the previous question, please specify the tool(s) and model(s) used (e.g., Google Gemini, OpenAI ChatGPT-4, etc.).

Tool(s) and model(s) used: composer-2-fast

Comment thread docs/cli/release/changelog-bundle-amend.md Outdated
Comment thread docs/cli/release/changelog-gh-release.md Outdated
@lcawl lcawl marked this pull request as ready for review March 30, 2026 23:33
@lcawl lcawl requested review from a team as code owners March 30, 2026 23:33
@lcawl lcawl requested a review from cotti March 30, 2026 23:33
@coderabbitai coderabbitai Bot added documentation Improvements or additions to documentation feature and removed enhancement labels Mar 30, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 30, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds optional private-link sanitization for changelog bundles. When enabled (via bundle.sanitize_private_links, per-profile override, or CLI flags) and only when bundle resolution is performed, bundling rewrites PR/issue references that resolve to repositories marked private: true in assembler.yml into quoted # PRIVATE: <reference> sentinels. Introduces PrivateChangelogLinkSanitizer and ChangelogTextUtilities parsing helpers, wires sanitization through bundling and amend services, exposes CLI flags, updates configuration parsing and directive :link-visibility:, adjusts renderers to treat sentinel-prefixed refs as hidden, and adds tests and docs.

Sequence Diagram(s)

sequenceDiagram
    participant User as CLI/User
    participant Bundler as ChangelogBundlingService
    participant ConfigLoader as ChangelogConfigurationLoader
    participant Assembler as ConfigurationContext (assembler.yml)
    participant Sanitizer as PrivateChangelogLinkSanitizer
    participant Amend as ChangelogBundleAmendService
    participant Renderer as Markdown/Asciidoc Renderer

    User->>Bundler: run "changelog bundle" (--sanitize-private-links / config)
    Bundler->>ConfigLoader: load changelog.yml
    ConfigLoader-->>Bundler: bundle.SanitizePrivateLinks / profiles
    Bundler->>Assembler: load assembler.yml references
    Assembler-->>Bundler: ReferenceRepositories

    alt sanitize enabled and resolve==true
        Bundler->>Sanitizer: TrySanitizeBundle(bundle, assembly, defaults)
        Sanitizer->>Sanitizer: parse each entry.Prs/Issues -> owner/repo
        Sanitizer->>Assembler: lookup repository by owner/repo
        Assembler-->>Sanitizer: Repository (Private? true/false)
        alt Repository.Private == true
            Sanitizer->>Sanitizer: replace ref -> "# PRIVATE: <ref>"
        else
            Sanitizer->>Sanitizer: keep original ref
        end
        Sanitizer-->>Bundler: sanitized bundle (or error)
    else
        Bundler->>Bundler: emit error / skip sanitization
    end

    User->>Amend: run "changelog bundle-amend" (may request sanitization)
    Amend->>ConfigLoader: load configuration & parent bundle (async)
    Amend->>Sanitizer: validate/sanitize parent and amend bundle if needed

    Bundler->>Renderer: render bundle entries (link-visibility considered)
    Renderer->>Renderer: FormatPr/IssueLink returns empty for "# PRIVATE:" sentinel
    Renderer-->>User: output with links hidden or shown per visibility
Loading

Suggested labels

documentation

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.31% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add changelog bundle support for hiding private links' clearly summarizes the main feature added: opt-in bundle-time sanitization of PR/issue links targeting private repositories. It directly reflects the primary change across the changeset.
Description check ✅ Passed The description comprehensively relates to the changeset, explaining the feature (private link sanitization in bundles), configuration options, implementation highlights, directive enhancement, tests, and testing steps. It clearly connects to all major file changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch sanitize-links

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/cli/release/changelog-bundle-amend.md`:
- Line 71: The markdown link uses a nonexistent anchor
"#changelog-bundle-private-link-sanitization"; update the link target in the
sentence "Private link sanitization at bundle time" to point to the correct
anchor or page — either remove the fragment and link to
/contribute/changelog.md, or replace the fragment with the correct anchor name
that exists in contribute/changelog.md (search for the actual heading for
"bundle private link sanitization" and use that heading's slug).

In `@docs/cli/release/changelog-gh-release.md`:
- Line 54: Update the broken markdown link on the line that currently reads
"/contribute/changelog.md#changelog-bundle-private-link-sanitization": change
the target file to "/cli/release/changelog-bundle.md" and the anchor to
"#private-link-sanitization" so the final link is
"/cli/release/changelog-bundle.md#private-link-sanitization".

In `@src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs`:
- Around line 233-245: The parsing logic in ChangelogTextUtilities that extracts
owner/repo from GitHub URLs returns true too eagerly; update it to validate
additional path segments and fragments before accepting a match: ensure the
AbsolutePath has exactly two segments to accept plain owner/repo, and if there
are extra segments require the pattern to be /owner/repo/issues/<number> or
/owner/repo/pull/<number> (i.e., the third segment must be "issues" or "pull"
and the fourth segment must parse as an integer); likewise, if the input uses a
fragment (owner/repo#123) validate the fragment is a numeric issue/PR id before
returning true. Apply the same stricter validation to the second similar block
in this file (the block around the later range) so malformed refs like
/pull/not-a-number or `#abc` return false instead of being treated as valid
owner/repo.
- Around line 279-280: The sentinel handling currently returns an empty string
which still makes renderers treat a reference as present; change the checks that
do if (pr.StartsWith(PrivateReferenceSentinelPrefix,
StringComparison.OrdinalIgnoreCase)) return string.Empty; (and the similar
checks at the other occurrences) to return null instead, so callers can
distinguish "no reference" vs "empty text"; then update the Markdown rendering
call sites (where entry.Prs / entry.Issues are enumerated in
MarkdownRendererBase and IndexMarkdownRenderer) to filter out nulls before
deciding whether to emit the surrounding prose. Ensure every occurrence of
PrivateReferenceSentinelPrefix handling (lines shown: the three pairs) is
updated consistently.

In `@src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs`:
- Around line 117-123: The current check uses sanitizePrivateLinks and
shouldResolve but only guards the new amend file, allowing --resolve to bypass
an unresolved parent bundle; update the logic in ChangelogBundleAmendService so
that when sanitizePrivateLinks is true you also verify the original/parent
bundle is resolved (e.g., check the existing/parent bundle resolved flag or
resolution state before proceeding) and emit the same collector.EmitError
message if the parent bundle is unresolved, preventing sanitize from running
when only the new amend is resolved; apply the same fix to the other occurrence
noted around the 151-170 block.

In `@src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs`:
- Around line 434-437: The ternary that computes sanitizePrivateLinks ignores
the CLI override when input.Profile is set; update the expression so that when
Profile is provided you still prefer a non-null input.SanitizePrivateLinksCli
before falling back to ProcessProfile value and config. Concretely, change the
branch that currently picks input.SanitizePrivateLinks when input.Profile is set
so it instead uses input.SanitizePrivateLinks ?? input.SanitizePrivateLinksCli
?? config.Bundle.SanitizePrivateLinks (and keep the other branch using
input.SanitizePrivateLinksCli ?? config.Bundle.SanitizePrivateLinks),
referencing the sanitizePrivateLinks local and the input.* properties
(NoSanitizePrivateLinks, Profile, SanitizePrivateLinks, SanitizePrivateLinksCli)
and config.Bundle.SanitizePrivateLinks.

In `@src/tooling/docs-builder/Commands/ChangelogCommand.cs`:
- Around line 504-505: Add a new changelog YAML entry under docs/changelog
describing the new bundle flags referenced in ChangelogCommand.cs: the
sanitizePrivateLinks and noSanitizePrivateLinks options (CLI flags
--sanitize-private-links / --no-sanitize-private-links) and that
sanitize_private_links behavior defaults to bundle.sanitize_private_links when
omitted; ensure the YAML includes a short title, date, PR/author reference, and
a clear description of user-visible behavior and which command/class it affects
(reference ChangelogCommand and the parameter names sanitizePrivateLinks /
noSanitizePrivateLinks) so assembler-preview stops failing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 06353714-34c7-48e9-bcf2-4e3e3e75228e

📥 Commits

Reviewing files that changed from the base of the PR and between dc4a854 and 85724d6.

📒 Files selected for processing (20)
  • config/assembler.yml
  • config/changelog.example.yml
  • docs/cli/release/changelog-bundle-amend.md
  • docs/cli/release/changelog-bundle.md
  • docs/cli/release/changelog-gh-release.md
  • docs/contribute/changelog.md
  • docs/syntax/changelog.md
  • src/Elastic.Documentation.Configuration/Changelog/BundleConfiguration.cs
  • src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogBlock.cs
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogInlineRenderer.cs
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogLinkVisibility.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
  • src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs
  • src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs
  • src/services/Elastic.Changelog/Serialization/ChangelogConfigurationYaml.cs
  • src/tooling/docs-builder/Commands/ChangelogCommand.cs
  • tests/Elastic.Changelog.Tests/Changelogs/AssemblerConfigurationYamlTests.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs

Comment thread docs/cli/release/changelog-bundle-amend.md Outdated
Comment thread docs/cli/release/changelog-gh-release.md Outdated
Comment thread src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
Comment thread src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
Comment thread src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs Outdated
Comment thread src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
Comment thread src/tooling/docs-builder/Commands/ChangelogCommand.cs
@lcawl lcawl removed the documentation Improvements or additions to documentation label Mar 31, 2026
@lcawl lcawl marked this pull request as ready for review March 31, 2026 02:32
@coderabbitai coderabbitai Bot added documentation Improvements or additions to documentation and removed feature labels Mar 31, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs`:
- Around line 132-138: The code currently only checks
parentBundleForSanitize.IsResolved; instead run the same private-link sanitizer
used for amendBundle against parentBundleForSanitize (using the sanitizer
method/class already used elsewhere in ChangelogBundleAmendService) and compare
the sanitized result to the original parent; if sanitization would change the
parent, call collector.EmitError (same message context) and return false,
instructing the user to rebuild the original bundle with resolve enabled or
disable bundle.sanitize_private_links; apply the same check to the second code
path that currently only inspects IsResolved (the block covering the later
amend/merge logic).

In `@src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs`:
- Around line 39-48: The current early-return that checks
assembly.ReferenceRepositories.Count and calls collector.EmitError causes
sanitize_private_links to fail before any entries are scanned; remove or bypass
this upfront check in PrivateChangelogLinkSanitizer and instead defer emitting
the "empty assembler.yml references" error until you actually encounter a
reference that requires privacy classification (i.e., when processing a specific
changelog entry or reference and you need to look up its repo but
assembly.ReferenceRepositories is empty). Update the logic in the method that
iterates references (the sanitizer's processing loop) to detect missing
reference repositories at the point of classification, and call
collector.EmitError with the same message only when a PR/issue link is being
evaluated and cannot be classified due to an empty ReferenceRepositories
collection.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 70b83c14-9303-4715-8b77-21eeb2e8eee5

📥 Commits

Reviewing files that changed from the base of the PR and between 85724d6 and 0aa19bf.

📒 Files selected for processing (10)
  • docs/changelog/changelog-private-link-sanitization.yaml
  • src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogBlock.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
  • src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs
  • src/services/Elastic.Changelog/Rendering/Asciidoc/AsciidocRendererBase.cs
  • src/services/Elastic.Changelog/Rendering/Markdown/IndexMarkdownRenderer.cs
  • src/services/Elastic.Changelog/Rendering/Markdown/MarkdownRendererBase.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs
✅ Files skipped from review due to trivial changes (1)
  • docs/changelog/changelog-private-link-sanitization.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogBlock.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
  • src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs

Comment thread src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs Outdated
Comment thread src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs Outdated
@coderabbitai coderabbitai Bot added feature and removed documentation Improvements or additions to documentation labels Mar 31, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs`:
- Around line 133-155: The repository resolution duplicates the same repo-only
check in the fallback loop, causing it to miss keys like "elastic/<repo>"; in
the method PrivateChangelogLinkSanitizer.ResolveRepository (or the surrounding
resolution block) change the second-loop comparison from comparing kvp.Key to
repo to comparing kvp.Key to the owner-prefixed form (e.g. $"{owner}/{repo}" or
specifically "elastic/{repo}" when owner == "elastic") using the same
StringComparison.OrdinalIgnoreCase, so the fallback will find entries stored
with the "owner/repo" key and return the repository correctly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7b9f2122-00e1-4940-91cf-54edb5722155

📥 Commits

Reviewing files that changed from the base of the PR and between 0aa19bf and afab163.

📒 Files selected for processing (3)
  • src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs
  • src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs

Comment thread src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs Outdated
Comment thread src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs (1)

189-190: Consider extracting duplicate owner/repo extraction.

The same logic appears twice:

var owner = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Owner ?? "elastic" : "elastic";
var repo = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Repo : null;
Extract to local function
+		(string Owner, string? Repo) GetBundleOwnerRepo(Bundle bundle) =>
+			bundle.Products.Count > 0
+				? (bundle.Products[0].Owner ?? "elastic", bundle.Products[0].Repo)
+				: ("elastic", null);
+
 		if (sanitizePrivateLinks)
 		{
 			// ... validation logic ...
 
-			var owner = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Owner ?? "elastic" : "elastic";
-			var repo = parentBundleForSanitize.Products.Count > 0 ? parentBundleForSanitize.Products[0].Repo : null;
+			var (owner, repo) = GetBundleOwnerRepo(parentBundleForSanitize);
 			if (!PrivateChangelogLinkSanitizer.TrySanitizeBundle(

And similarly for the second occurrence at lines 241-242.

Also applies to: 241-242

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs`
around lines 189 - 190, Duplicate owner/repo extraction logic from
parentBundleForSanitize.Products (using Products[0].Owner with fallback
"elastic" and Products[0].Repo) should be extracted into a small local helper
inside ChangelogBundleAmendService (e.g., a private local function or local
lambda inside the method that returns (owner, repo) given
parentBundleForSanitize) and both occurrences (the one using
parentBundleForSanitize.Products[0].Owner ?? "elastic" and the one using
Products[0].Repo) should call that helper; update all references so owner and
repo are assigned from the helper instead of repeating the ternary expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs`:
- Around line 189-190: Duplicate owner/repo extraction logic from
parentBundleForSanitize.Products (using Products[0].Owner with fallback
"elastic" and Products[0].Repo) should be extracted into a small local helper
inside ChangelogBundleAmendService (e.g., a private local function or local
lambda inside the method that returns (owner, repo) given
parentBundleForSanitize) and both occurrences (the one using
parentBundleForSanitize.Products[0].Owner ?? "elastic" and the one using
Products[0].Repo) should call that helper; update all references so owner and
repo are assigned from the helper instead of repeating the ternary expressions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d68b594d-96ea-4d58-9c23-33cce34bfac3

📥 Commits

Reviewing files that changed from the base of the PR and between afab163 and 6c78b62.

📒 Files selected for processing (3)
  • src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs
  • src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs

out var sanitizedParent))
return false;

if (sanitizedParent != parentBundleForSanitize)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if will always return true even if we don't want it to: sanitizedParent.Entries property is created as a List, so it's a reference being compared down the line. The comparison needs to take that into account.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the rest looks good, I'll get this fixed and add some tests.

Comment thread src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs Outdated
@coderabbitai coderabbitai Bot added documentation Improvements or additions to documentation and removed feature labels Apr 1, 2026
coderabbitai[bot]
coderabbitai Bot previously requested changes Apr 1, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs`:
- Around line 86-89: The early continue when encountering SentinelPrefix in
PrivateChangelogLinkSanitizer.cs (the code that checks
r.StartsWith(SentinelPrefix)) treats "# PRIVATE:" as trusted and skips
validation; change this so the underlying reference is re-parsed and validated
instead of blindly accepting it (e.g., strip SentinelPrefix from r, re-run the
repo/ref parsing and lookup logic used elsewhere, and only add to list if that
validation succeeds), or alternatively gate the existing list.Add(r); continue;
behind an explicit “already sanitized bundle” flag/path so sentinels are only
trusted when that flag is set. Ensure you update the handling around
SentinelPrefix, r, and the repo lookup/parsing functions used by this class so
no quoted "# PRIVATE: ..." bypasses validation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ca76d344-a61d-436a-804f-62fac9069955

📥 Commits

Reviewing files that changed from the base of the PR and between 6c78b62 and e5a3497.

📒 Files selected for processing (6)
  • src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs
  • src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
  • src/services/Elastic.Changelog/Bundling/PrivateChangelogLinkSanitizer.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs
  • tests/Elastic.Markdown.Tests/Directives/ChangelogHideLinksTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
  • src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
  • tests/Elastic.Changelog.Tests/Changelogs/PrivateChangelogLinkSanitizerTests.cs

@lcawl lcawl enabled auto-merge (squash) April 1, 2026 15:55
@coderabbitai coderabbitai Bot dismissed their stale review April 1, 2026 15:56

The flagged sentinel bypass issue has been fixed in commit 8bbf05a with proper validation of # PRIVATE: references.

@lcawl lcawl merged commit e578fac into main Apr 1, 2026
28 checks passed
@lcawl lcawl deleted the sanitize-links branch April 1, 2026 15:56
cotti added a commit that referenced this pull request Apr 2, 2026
Co-authored-by: Felipe Cotti <felipe.cotti@elastic.co>
cotti added a commit that referenced this pull request Apr 7, 2026
* Move S3 upload to a new integrations project and reuse it for changelog uploads. Add tests.

* Update tests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Ne'er forget using

* Adjust entities being used

* Update src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Add changelog bundle support for hiding private links (#3002)


Co-authored-by: Felipe Cotti <felipe.cotti@elastic.co>

* Fix ApplicationData path collision with workspace root in Docker/CI containers (#3012)

* Reapply "Wrap all IFileSystem usage in ScopedFileSystem (#3001)" (#3011)

This reverts commit 108282e.

* Fix ApplicationData scope-root collision when HOME is unset in Docker

In Docker/CI containers where neither XDG_DATA_HOME nor HOME is set,
Environment.GetFolderPath(LocalApplicationData) returns "". Path.Join("", "elastic",
"docs-builder") then produces a relative path that resolves to {CWD}/elastic/docs-builder
— a subdirectory of WorkingDirectoryRoot. ScopedFileSystem 0.4.0's ValidateRootsAreDisjoint
check then throws in FileSystemFactory's static constructor, crashing the process with
TypeInitializationException before anything runs.

Observed in elastic/elasticsearch (workspace /github/workspace) and
elastic/chainguard-image-sync where the resolved ApplicationData path fell inside
the runner workspace.

Fix: fall back to Path.GetTempPath() when LocalApplicationData is empty so the
ApplicationData path is always an absolute non-workspace path.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* Misceleanous fixes, one old pending coderabbit task

* Address code review feedback on ScopedFileSystem usage

- Use FileSystemFactory.AppData for default checkout dirs (CodexContext, AssembleContext) instead of ReadFileSystem to avoid scope mismatches
- Fix RealGitRootForPathWrite to use BuildWriteOptions so AllowedSpecialFolders.Temp is included
- Fix DetermineWorkingDirectoryRoot: *.slnx check now respects depth guard (was bypassing it entirely)
- Fix GitLinkIndexReader.CloneDirectory to use Paths.ApplicationData instead of raw LocalApplicationData (handles Docker/CI empty HOME)
- Normalize ExternalScopeRoots in DetectionRulesDocsBuilderExtension to absolute paths
- Replace static Directory.GetCurrentDirectory() with fileSystem.Directory.GetCurrentDirectory() in DeployUpdateRedirectsService
- Replace new FileStream / static File.ReadAllTextAsync with scoped fileSystem equivalents in IncrementalDeployService
- Fix StaticWebHost to pass _contentRoot to RealGitRootForPath for consistent scoping
- Split AssemblerBuildService.BuildAll fs param into readFs/writeFs; update AssemblerCommands callers
- Fix test paths in CodexNavigationTestBase to fall within WorkingDirectoryRoot/ApplicationData scopes
- Make changelog test reportPath unique to prevent parallel-test file collisions

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* Fix build errors: AssemblerIndexService and MockFileSystem constructor

- Pass fileSystem as both readFs/writeFs in AssemblerIndexService.Index()
  to match the updated BuildAll(readFs, writeFs) signature
- Fix MockFileSystem construction in CodexNavigationTestBase: use
  constructor argument for currentDirectory instead of missing property

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* Fix FindGitRoot file-path fallback and split AssemblerIndexService read/write scopes

- Paths.FindGitRoot: capture startDir before the loop so fallback returns
  always return a directory path, not the raw startPath which can be a file
- AssemblerIndexService.Index: replace single fileSystem param with separate
  readFs/writeFs to avoid forwarding the read scope into write operations;
  update AssemblerIndexCommand to pass RealRead and RealWrite respectively

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* Restore unlimited-depth *.slnx anchor in DetermineWorkingDirectoryRoot

The previous commit applied a depth guard to *.slnx in release builds,
but this broke both authoring tests and the assembler integration tests:
binaries run by Aspire and dotnet test start from the build output or
project directory (4+ levels below the solution root), so restricting
*.slnx to depth <= 1 caused WorkingDirectoryRoot to resolve to the
build output directory instead of the repo root, making docs/ and
all other repo paths fall outside the scoped filesystem roots.

*.slnx is intentionally a depth-unlimited anchor for DetermineWorkingDirectoryRoot
— it is the primary way to locate the solution root regardless of where
the binary is launched from. The .git depth guard is kept unchanged.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* Fix docs-builder redirect tests (#3008)

* Fix redirect tests

* Remove changelog redirects implemented elsewhere

* Search: Use default semantic_text inference, remove Jina mappings (#3014)

Elasticsearch Serverless now defaults semantic_text to Jina, making
the explicit Jina sub-fields redundant and the ELSER inference ID
unnecessary. This removes both inference ID constants, all .jina
field mappings, and lets semantic_text fields use the platform default.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore: Update config/versions.yml eck 3.3.2 (#3019)

Made with ❤️️ by updatecli

Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com>

* Deploy: Use write-scoped filesystem for apply command (#3021)

The deploy apply command used RealRead which lacks AllowedSpecialFolder.Temp,
causing ScopedFileSystemException when AwsS3SyncApplyStrategy stages files
in /tmp/ for S3 upload. Switch to RealWrite which permits temp directory access.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Use ScopedFileSystem and inject interfaces.

* Fix exception filtering

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Lisa Cawley <lcawley@elastic.co>
Co-authored-by: Martijn Laarman <Mpdreamz@gmail.com>
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Jan Calanog <jan.calanog@elastic.co>
Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants