Drop stale PLACEHOLDER annotations that shadow upstream descriptions#5588
Draft
janniklasrose wants to merge 2 commits into
Draft
Drop stale PLACEHOLDER annotations that shadow upstream descriptions#5588janniklasrose wants to merge 2 commits into
janniklasrose wants to merge 2 commits into
Conversation
A `description: PLACEHOLDER` marks a field that has no documentation anywhere. It is added when a field first appears undocumented, but was never removed when cli.json later gained a description for the field. Because the annotations file overrides upstream docs in the merge, the stale marker shadowed the real description, and since PLACEHOLDER is skipped when assigning annotations, the schema emitted no description at all (e.g. `vector_search_endpoints.*.target_qps`). Strip such placeholders in newAnnotationHandler before the merge: the upstream description shows through, and because the pruned map is what syncWithMissingAnnotations writes back, the annotations file sheds the stale markers in the same run. Entries that carry other hand-authored fields (e.g. deprecation_message) lose only the placeholder. Fields that are genuinely undocumented keep their TODO marker, so the result is idempotent. TestRequiredAnnotationsForNewFields now also records deletes and updates during regeneration instead of nil-panicking on them, so any future non-idempotent rewrite of annotations.yml fails with a message instead of a panic. Co-authored-by: Isaac
Co-authored-by: Isaac
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
1 participant
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.
Changes
description: PLACEHOLDERmarkers are now dropped when cli.json documents the field:newAnnotationHandlerstrips them before the merge, so the upstream description flows into the schema, and the sync step rewritesannotations.ymlwithout them in the same run.deprecation_message) lose only the placeholder.annotations.yml−487 lines (deletions only), both schema artifacts gain the same 128 description lines.Stacked on #5574, which stays byte-identical; this is the behavior change on top.
Why
A PLACEHOLDER means "no documentation anywhere". It is added when a field first appears undocumented, but nothing removed it once upstream docs arrived. The annotations file overrides upstream docs in the merge and PLACEHOLDER is skipped when assigning descriptions, so the schema ended up with no description at all — the stale marker hides the real text and then discards itself. 154 fields were affected (e.g.
vector_search_endpoints.*.target_qps). The file cannot self-heal: a shadowed field never registers as missing, so the backlog only grows.This also fixes the bare
[Public Preview]prefixes seen in the launch-stage work (#5443): the prefix was there, but the description it should prefix was swallowed by this bug.Tests
TestRequiredAnnotationsForNewFieldsnow also records deletes/updates during regeneration (previously a nil-visitor panic) and asserts regeneration of the committed file is a no-op, enforcing idempotency.task fmt,checks, fulllint, and fulltest(unit + acceptance) pass locally.This pull request and its description were written by Isaac, an AI coding agent.