Repository navigation
feat(cli)!: verify the comment author before issues post-comment edits - #7916
Merged
Merged
Conversation
issues post-comment found its earlier sticky comment by marker alone. Anyone who could comment could plant the marker (as body text, or on Jira as a property on their own comment), and every later run then wrote into the planted comment, or failed if the edit was refused. An existing comment is now edited only when it carries the marker and its author is exactly the identity the command posts as: the login from forge.Client.GetAuthenticatedUser on GitHub and GitLab (which also resolves App installation tokens and Actions' GITHUB_TOKEN through GraphQL viewer), the account ID from GET /myself on Jira. Login shapes and display names are never trusted. A planted comment is ignored and a new comment posted. The identity lookup retries twice (context-aware). If it still fails, the command returns an error naming the cause, posts or edits nothing, and says to rerun if the failure was transient or otherwise check that the token can read its own identity. Posting a new comment instead would orphan the earlier one. --dry-run resolves the identity too. Help text and the user guide's trust model state the rule, drop the "acceptable in trusted CI" caveat, and note that whoever can edit the command's own comments is still trusted. BREAKING CHANGE: issues post-comment fails, posting nothing, when it cannot resolve the identity it posts as, including under --dry-run; and it no longer edits a marker comment written by a different identity, so after a token rotation or App migration the next run posts a new comment instead of updating the old one. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
changed the title
PR Summary by QodoVerify comment authors before updating sticky issue comments
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 2:52 PM UTC · Completed 3:30 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.66 |
Site previewPreview: https://5625e0f8-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsModerate file and line churn with no protected paths or CI changes, balanced by an established author, zero history of reverts, and strong issue acceptance criteria alignment despite modifying existing CLI behavior without a feature flag; anchored to the prior review's score of 2 (moderate) since Tier 1 signals (file/line counts, protected paths, CI/dependency surface) are unchanged — only test and doc wording changed since the prior assessment. Previous runRisk Assessment: moderate (2/5) DetailsModerate file and line churn with no protected paths or CI changes, balanced by an established author, zero history of reverts, and strong issue acceptance criteria alignment despite modifying existing CLI behavior without a feature flag. |
ReviewFindingsHigh
Low
Other dimensions (clean)
This is a re-review. Since the prior review (SHA Next steps:
Previous runReviewFindingsHigh
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ggallen
approved these changes
Oct 1, 2026
ggallen
left a comment
ggallen
left a comment
Member
There was a problem hiding this comment.
Reviewed the changes and found no actionable issues.
…ery steps TestForgeClient_AuthenticatedUser and TestJiraClient_AuthenticatedUser follow the Test<Receiver>_<Method> convention, and the Jira one moves to jira_client_test.go with the other JiraClient tests. The user guide's identity-failure guidance becomes numbered steps. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 3:46 PM UTC · Completed 4:25 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.87 |
waynesun09
dismissed stale reviews from fullsend-ai-review[bot] and fullsend-ai-review[bot]
October 1, 2026 16:38
No substantive findings: this round's bot review consists only of sub-agent timeouts (900s); 0 unresolved threads, CI green, ggallen approved.
3 tasks done
jctanner
pushed a commit
to jctanner/fullsend
that referenced
this pull request
Oct 1, 2026
…y is unresolved 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. fullsend-ai#7243/fullsend-ai#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 fullsend-ai#7916, and the issue asks to keep the retry local to internal/sticky. Closes fullsend-ai#7884
This branch was successfully 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.
What
fullsend issues post-commentnow edits an existing sticky comment only when the comment carries the marker and its author is exactly the identity the command posts as:forge.Client.GetAuthenticatedUser. It also resolves App installation tokens and Actions'GITHUB_TOKENthrough GraphQLviewer.GET /myself.Login shapes (such as a
[bot]suffix) and display names are never trusted. A marker comment written by anyone else is ignored, and a new comment is posted.Why
The command found its earlier comment by marker alone. Anyone who could comment could plant the marker: as body text on GitHub or GitLab, or on Jira as a marker property (or legacy body text) on their own comment. Every later run then wrote into the planted comment, or failed if the edit was refused. On Jira, comments are now filtered by author before both the property lookup and the legacy body-text fallback.
If the identity can't be resolved, the lookup retries twice and respects context cancellation. If it still fails, the command returns an error naming the cause and asking for a rerun, and posts or edits nothing. Posting a new comment instead would orphan the earlier one, leaving content that no later run updates.
BREAKING CHANGE
fullsend issues post-comment(GitHub, GitLab, Jira):--dry-runresolves the identity too. A dry run now fails if it can't resolve the posting identity. Before, it needed no identity.The deprecated
fullsend post-comment(sticky.Post) is unchanged here; #7884 tracks it.Tests
internal/cli/issues_test.go:mallory,evil-review[bot],fullsend-ai-revieworx-fullsend-ai-review[bot]gets a new comment.ownJiraCommentsmatches by account ID and returns nothing for an empty identity.Each of these mutations fails a test:
The fakes support the change:
FakeJiraClientgainsGetMyself,MyselfandMyselfError, and stamps its own account on new comments.Split out of #7243. Closes #7886. Closes #6236.
🤖 Generated with Claude Code