fix(config): disambiguate PR comment markers across konflate instances - #511
Open
Stasky745 wants to merge 2 commits into
Open
fix(config): disambiguate PR comment markers across konflate instances#511Stasky745 wants to merge 2 commits into
Stasky745 wants to merge 2 commits into
Conversation
The hidden comment marker (<!-- konflate:pr-N -->) was keyed only by PR number. Multiple konflate deployments sharing one bot identity — a folder-per-cluster monorepo's normal shape, e.g. dev/app and production/app both authenticating as the same GitHub App — that render the same PR are indistinguishable to UpsertComment: whichever posts second finds the first's comment (same author, same marker) and overwrites it instead of creating its own. Add config.commentTag (KONFLATE_COMMENT_TAG), embedded in the marker. Falls back to StatusCheckName (already commonly set to a distinct per-instance name for branch protection), then to no tag — a single-instance setup with neither set is unaffected. Sanitized before embedding since it lands inside an HTML comment (see home-operations#314).
 The PR appears safe to merge; the previously reported marker collision, default compatibility, and stale-template-marker concerns are addressed.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| marker := konflateMarker(number) | ||
| func ensureMarker(number int, tag, body string) string { | ||
| marker := konflateMarker(number, tag) | ||
| if strings.Contains(body, marker) { |
There was a problem hiding this comment.
For a tagged instance, a custom template that already emits the previously documented <!-- konflate:pr-{{ .PR.Number }} --> marker receives the new tagged marker in addition to the legacy one. During a mixed-version rollout, an older untagged instance can then match and overwrite this tagged comment, depending on comment ordering. Please remove or replace the legacy marker when injecting a tagged marker and cover this compatibility case in TestEnsureMarker.
…mplate markers Three issues from review on home-operations#511: - CommentMarkerTag() falling back to StatusCheckName unconditionally broke every single-instance install: Load fills a blank StatusCheckName in with DefaultStatusCheckName before any caller sees it, so the fallback always fired and changed every install's marker on upgrade. Now excludes the default constant from the fallback. - konflateMarker's character-collapsing sanitizer let distinct tags collide ("dev/app", "dev app", "dev-app" all sanitized to "dev-app"), reintroducing the exact cross-instance overwrite this exists to prevent. Replaced with a sha256 hash of the raw tag: deterministic, collision-safe, and structurally can't break out of the HTML comment (hex-only output). - ensureMarker didn't strip a marker a custom template already embeds verbatim (the contract says a template needn't include one, not that it mustn't). A stale untagged marker surviving alongside the new tagged one would let an older or differently-tagged instance still Contains-match and overwrite the comment during a mixed rollout.
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.
Overview
The hidden comment marker (
<!-- konflate:pr-N -->) was keyed only by PR number. In a multi-cluster monorepo scenario two instances behind the same GitHub App rendering the same PR can't tell each other's comments apart, so the second one to post finds and overwrites the first's.Add
config.commentTag(KONFLATE_COMMENT_TAG), embedded in the marker. Falls back toStatusCheckName(already commonly set to a distinct per-instance name for branch protection), then to no tag. A single-instance setup with neither set is unaffected. Sanitized before embedding since it lands inside an HTML comment.Additional information
This might create a new extra comment on a PR that already existed before the update, since the newly-tagged marker doesn't match the old untagged comment already on the PR. This is one-time and only for comments generated before the update.
Requirements