fix(changelog): source pointer + phantom-marker guard for canonical reconciliation - #3937
Merged
Conversation
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 <noreply@anthropic.com>
…d other non-link fields 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 <noreply@anthropic.com>
…read (#3931) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ly short-circuit pure markers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent failures now throw instead of returning null Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…o longer probe elasticsearch pool Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Step 8
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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ly short-circuit pure markers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…crubResult return type change Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
…econciliation 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: <canonical-pr>) 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) <noreply@anthropic.com>
reakaleek
approved these changes
Aug 26, 2026
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Requesting changes: there are high-severity delete-path regressions that can either orphan canonical objects (.yml numeric source keys) or delete unrelated canonical entries by misclassifying generic marker-shaped files as source pointers.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
…ic 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) <noreply@anthropic.com>
There was a problem hiding this comment.
Requesting changes: there are still high-severity delete-path correctness gaps that can cause unintended canonical deletions or leave canonical entries orphaned.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
…it before filename heuristics 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PR #3930's review identified two correctness regressions in canonical key/marker reconciliation. First, a private marker derived from raw (pre-allowlist) PR numbers can race with the scrubber writing canonical public content at the same key — if the marker is processed last it overwrites the canonical entry. Second, when a private source file is non-canonical (e.g.
12345-fix.yamlwhose public canonical is12345.yaml), a delete event only removes the source key from the public bucket, leaving the canonical and its secondary-PR markers stranded indefinitely.What
Issue 1 — phantom-marker overwrite guard:
ScrubResultgains anIsMarkerflag, set when the scrubber returns a pass-through private marker.ScrubberProcessorchecks this flag before writing: if canonical content (non-marker) already occupies the target public key, the marker write is skipped so it cannot overwrite the primary entry.Issue 2 — source pointer + delete-path tracing: When the write path routes content to a non-source canonical key (non-null
CanonicalKey), it also writes a source pointer — alink: <pr>object — at the source key in the public bucket. This gives the delete path a recoverable breadcrumb. On delete, non-numeric-filename keys are checked for a source pointer; if found, the processor derives the canonical key, callsDeleteStaleMarkersAsyncto clean up secondary-PR markers, deletes the canonical, then removes the pointer. Numeric-filename keys (already-canonical entries and secondary-PR markers) also receive marker cleanup before deletion.Three new
ScrubberProcessorTestscover the phantom-marker guard, source-pointer delete tracing, and canonical-with-markers delete.