Repository navigation
fix(#7884): sticky.Post never edits a comment when identity is unresolved - #7936
Merged
Merged
Conversation
…lved sticky.Post resolved the poster's login with GetAuthenticatedUser and, on failure, logged a warning and continued with an empty login. FindMarkedComment skipped its author filter whenever botUser was empty, so it matched the first comment carrying the marker regardless of who wrote it, letting an unverified (and potentially spoofed) comment get edited in-place. #7243/#7916 already closed this gap for postTrackerStickyComment; this brings sticky.Post (used by post-comment / post-review) to the same rule. Changes in internal/sticky/sticky.go: * Added resolveBotUser, which retries a failed or empty GetAuthenticatedUser lookup per a package-level botUserLookupBackoff (tests shorten it), honouring ctx cancellation. This mirrors resolveTrackerSelf in internal/cli/issues.go, but is kept local to this package since internal/cli imports sticky and sharing it would create an import cycle. * Post now calls resolveBotUser before listing or writing anything. If the login can't be resolved, it returns an error naming the cause (wrapping the lookup error) and calls neither UpdateIssueComment nor CreateIssueComment, including on dry run, so dry-run output never implies an edit would occur when none safely could. * FindMarkedComment now treats an empty botUser as "match nothing" instead of "match any author," and requires an exact author match plus the marker. Tests in internal/sticky/sticky_test.go: * Inverted TestFindMarkedComment_EmptyBotUser to assert nil instead of matching any author. * Added coverage for a persistent identity-lookup failure (errors, posts/edits nothing), a transient failure that succeeds on retry (edits the bot's own comment), a planted marker comment alongside the bot's own comment (only the bot's comment is edited), a lone planted comment (a new comment is created and the planted one is untouched), and dry run with an unresolved identity (errors, doesn't claim an edit would happen). * flakyAuthClient wraps forge.Client to fail GetAuthenticatedUser a configurable number of times before succeeding, since FakeClient's Errors map isn't a call-sequence. * Set AuthenticatedUser on the fake client in TestPost_DryRun and TestPost_DryRunExisting, which previously relied on the degraded fallback (an empty login) succeeding. Scope: internal/cli/issues.go (and its resolveTrackerSelf / postTrackerStickyComment) is untouched; that package already has its own equivalent, added by #7916, and the issue asks to keep the retry local to internal/sticky. Closes #7884
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 7:21 PM UTC · Completed 7:31 PM UTC Commit: Runtime: pi · Model: openai/gpt-6.1-sol → gpt-6.1-sol · Effort: high · Cost: $1.57 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Risk Assessment: low (1/5) DetailsScoped two-file bug fix by a bot author, with one test file, no protected-path changes, and low historical churn; the diff aligns with the linked security issue despite changing existing behavior without a feature flag. |
|
Looks good to me Labels: This fixes an existing sticky-comment identity-check defect that could allow edits to another author's marker comment. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
approved these changes
Oct 1, 2026
This branch was previously deployed
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
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.
Summary
sticky.Post(internal/sticky/sticky.go) resolved the poster's login viaforge.Client.GetAuthenticatedUserand, on failure, logged a warning and continued with an empty login.FindMarkedCommentskipped its author filter whenever the login was empty, so it matched the first comment carrying the marker regardless of author, meaning an unverified (potentially spoofed) comment could be edited in-place. #7243/#7916 already closed this gap forpostTrackerStickyComment; this bringssticky.Post(used bypost-comment/post-review) to the same rule.resolveBotUserresolves the login with a short retry (package-levelbotUserLookupBackoff, tests shorten it; honours context cancellation), mirroringresolveTrackerSelfininternal/cli/issues.go. The retry logic is kept local tointernal/sticky(not shared) sinceinternal/cliimports this package.Postnow resolves identity before listing or writing anything. If it can't be resolved, it returns an error naming the cause and calls neitherUpdateIssueCommentnorCreateIssueComment, including on dry run, so dry-run output never claims an edit would occur when none safely could.FindMarkedCommentnow treats an emptybotUseras "match nothing" (previously "match any author"), requiring an exact author match plus the marker.Testing
go test ./internal/sticky/...andgo test ./internal/cli/...(both pass; two pre-existing failures ininternal/cli,TestPostScriptEnv_NoSchemaAppendedWhenEmptyand...WhenNoValidationLoop, are unrelated to this change and reproduce onmainbefore this commit).go vet ./...,gofmt -lon changed files.pre-commit runcould not fetch its remote hook repos in this sandbox (network policy); the applicable local-equivalent checks (gofmt, go vet, trailing whitespace, EOF newline, merge-conflict markers, line endings, secret scan) were run directly instead.internal/sticky/sticky_test.go: invertedTestFindMarkedComment_EmptyBotUser; added persistent-failure, transient-failure-then-success, planted-marker-alongside-own-comment, only-planted-comment, and dry-run-with-unresolved-identity cases.Scope
internal/cli/issues.go(resolveTrackerSelf/postTrackerStickyComment) is intentionally untouched. It already has its own equivalent protection from #7916.Closes #7884
Post-script verification
agent/7884-sticky-identity-check)2d56b14f8b667ff2c922f0898126936b9fd4b41a..HEAD)