feat: add sticky review comments and post-review CLI#383
Conversation
Site previewPreview: https://5abd04da-site.fullsend-ai.workers.dev Commit: |
There was a problem hiding this comment.
Review: #383
Head SHA: 26aea0f
Timestamp: 2026-04-23T00:00:00Z
Outcome: comment-only
Summary
This PR adds a well-structured post-review CLI subcommand and the supporting forge.Client methods for sticky review comment lifecycle management. The implementation correctly addresses the core problem described in #381 — preventing review comment flooding on re-runs. No critical or high findings were identified. The medium findings below relate to a marker-spoofing vector (missing author check when identifying the bot's comment), potential UTF-8 corruption during comment truncation, and unchecked GraphQL errors in the MinimizeComment implementation.
Findings
Medium
-
[Content security]
internal/cli/postreview.go:103-107— The marker-based comment search does not verify the comment author. Any user (or bot) can post a comment containing<!-- fullsend:review-agent -->, causingpostReviewto update the decoy comment instead of creating its own. This could be used to suppress the bot's review or inject content into what appears to be a bot-authored review.
Remediation: After finding a comment with the marker, verifyexisting.Authormatches the bot's authenticated identity (callclient.GetAuthenticatedUser()or accept the expected author as a parameter). -
[Correctness]
internal/cli/postreview.go:183—truncateBodyslices the body string at a raw byte offset (body[:budget]), which can split multi-byte UTF-8 characters mid-codepoint, producing invalid UTF-8 in the posted comment.
Remediation: Useutf8.ValidString(body[:budget])or walk backward withutf8.DecodeLastRuneInStringto find a valid rune boundary before slicing. -
[Correctness]
internal/forge/github/github.go:1045-1060—MinimizeCommentcloses the GraphQL response body without checking for GraphQL-level errors. GitHub's GraphQL API returns HTTP 200 even when the mutation fails; errors are in theerrorsarray of the JSON response. A failed minimize will be silently ignored.
Remediation: Decode the GraphQL response and check for a non-emptyerrorsfield before returning nil.
Low
-
[Correctness]
internal/cli/postreview.go:199-212—parseReviewResultis defined but never called in any production code path. ThepostReviewfunction receives the body as a plain string; the JSON-parsing logic is unused dead code.
Remediation: Either wireparseReviewResultinto the command'sRunEto handle structured input (usingActionto drivegh pr reviewflags), or remove it until needed. -
[Style]
internal/cli/postreview_test.go:12— The"io"stdlib import is placed between the third-party (testify) and internal (forge,ui) import groups instead of in the stdlib group. This will likely failgoimports/ linter checks.
Remediation: Move"io"into the first (stdlib) import group alongside"context","strings", and"testing".
Info
- [Style]
internal/forge/github/github.go:1260-1263— Unrelated whitespace-only change (alignment of map literal keys inCreateOrgSecret). Cosmetic, no functional impact, but adds noise to the diff.
Footer
Outcome: comment-only
This review applies to SHA 26aea0f0ed7aeb96ee731b0328fe3d031e618842. Any push to the PR head clears this review and requires a new evaluation.
|
/code This PR implements phase 1 of #381 (sticky review comments). The following items from the plan are missing and should be addressed in this PR or explicitly deferred to a follow-up: Missing from Plan
Top Findings (by severity)
|
|
/review check again |
There was a problem hiding this comment.
Review: #383
Head SHA: 1b9f57b
Timestamp: 2026-04-27T12:00:00Z
Outcome: request-changes
Summary
This PR adds a well-structured sticky comment system (internal/sticky) and two new CLI subcommands (post-comment, post-review) to prevent comment flooding on PR re-runs, along with forge interface additions (CreateIssueComment, UpdateIssueComment, MinimizeComment). The core sticky comment lifecycle logic is solid, with good test coverage of the collapse/history/truncation mechanics. However, the post-review command parses the review result JSON but ignores the action field entirely, and MinimizeComment — which the issue specifically calls for — is implemented but never invoked. Additionally, the post-triage.sh migration is incomplete (the feature-request case still uses gh issue comment), and readCommentBody/readReviewBody are identical functions that should be deduplicated.
Findings
High
-
[Correctness]
internal/cli/postreview.go:206-216— TheReviewResult.Actionfield is parsed but completely ignored. Thepost-reviewcommand always posts a sticky issue comment regardless of whether the action isapprove,request-changes, orcomment. Per the issue (#381), the command should use the action to post a formalgh pr review(approve/request-changes) and minimize stale formal reviews. As implemented, this command is functionally identical topost-commentwith a hardcoded marker.
Remediation: Useparsed.Actionto dispatch between sticky comment posting and formal review submission. At minimum, document that formal review posting is deferred to a follow-up if intentional. -
[Correctness]
internal/forge/github/github.go(MinimizeComment, lines ~555-598) —MinimizeCommentis fully implemented and tested but never called by any code path. The linked issue (#381) specifically requires minimizing stale formal reviews before posting new ones. Dead code in a new feature suggests incomplete implementation.
Remediation: WireMinimizeCommentinto thepost-reviewlifecycle, or remove it and track as a follow-up issue to avoid shipping dead code.
Medium
-
[Correctness]
internal/scaffold/fullsend-repo/scripts/post-triage.sh:127— Thefeature-requestcase still usesgh issue commentdirectly instead offullsend post-comment. The other three cases (insufficient,duplicate,sufficient) were all migrated. This means re-runs of the triage agent on feature-request issues will still flood comments.
Remediation: Update thefeature-requestcase to usefullsend post-comment --repo "${REPO}" --number "${ISSUE_NUMBER}" --marker "<!-- fullsend:triage-agent -->" --token "${GH_TOKEN}" --result -. -
[Correctness]
internal/cli/postreview.go:256-268—parseReviewResultwith{"action": "approve"}(empty body) falls back to using the entire raw JSON string as the comment body. This means a valid JSON result with nobodyfield would post raw JSON as a GitHub comment.
Remediation: Return an error whenbodyis empty in a successfully parsed JSON result, or use a sensible default message instead of the raw JSON. -
[Style/conventions]
internal/cli/postcomment.go:90-104andinternal/cli/postreview.go:232-246—readCommentBodyandreadReviewBodyare byte-for-byte identical functions duplicated across two files.
Remediation: Extract a sharedreadBody(path string) (string, error)helper (e.g., in a shared file or thestickypackage) and call it from both commands.
Info
-
[Intent alignment] The PR description states it "Closes #381" and the issue calls for three capabilities: (1) sticky issue comments, (2) minimize old formal reviews, (3) simplify post-review.sh. This PR delivers (1) fully and (3) partially, but (2) is implemented at the API layer without any caller. The scope is reasonable as a first step, but the PR should clarify that formal review lifecycle management is deferred.
-
[Style/conventions]
internal/cli/root.go— The new subcommands are registered at the top level (fullsend post-review,fullsend post-comment). This is consistent withfullsend scanandfullsend run, but as more agent-infrastructure commands accumulate, consider grouping them under a parent (e.g.,fullsend agent post-review).
Footer
Outcome: request-changes
This review applies to SHA 1b9f57b053d22cf83bcf68cc75711f34b908403d. Any push to the PR head clears this review and requires a new evaluation.
There was a problem hiding this comment.
Review: #383
Head SHA: 7e3a547
Timestamp: 2026-04-27T12:00:00Z
Outcome: comment-only
Summary
This PR delivers a well-structured sticky comment system that addresses the duplicate review comment flooding described in #381. The implementation cleanly separates concerns: a reusable internal/sticky package for comment lifecycle management, a post-comment CLI for general sticky comments, and a post-review CLI that adds formal PR review handling. The code is well-tested with good edge case coverage. A few medium/low findings are noted below around regex robustness, truncation precision, and missing input validation, but none block merge.
Findings
Medium
-
[Correctness]
internal/sticky/sticky.go:89— ThedetailsReregex uses(?s)with lazy.*?to match<details>...<summary>Previous ...</summary>...(content)...</details>. While the flattening logic prevents nested<details>blocks from accumulating naturally, user-authored review content could contain</details>tags (e.g., in code blocks or markdown), causing the regex to match prematurely and truncate history content. Consider using a more robust extraction approach (e.g., tracking open/close tags) or at minimum documenting this limitation. -
[Correctness]
internal/sticky/sticky.go:150-166—TruncateBodycan produce output slightly overmaxSize. The truncation message is 60 bytes but the function only subtracts its length from the budget; after UTF-8 boundary adjustment the totalbody[:budget] + truncationMsgcould exceedmaxSizeby a few bytes if budget walked backward. The test at line 131 usesLessOrEqual(t, len(result), defaultMaxSize+100)which masks this — the tolerance should be tighter or the function should guarantee the exact bound. -
[Correctness]
internal/cli/postreview.go:182-193—parseReviewResultsilently treats invalid JSON as plain-text. If a result file contains malformed JSON (e.g., truncated write), this falls through to posting garbled content as a comment. Consider validating that the input is either well-formed JSON or clearly not JSON (e.g., doesn't start with{).
Low
-
[Correctness]
internal/cli/postcomment.go/internal/cli/postreview.go— Neither command validates that the--marker/ hardcoded marker follows the HTML comment format (<!-- ... -->). A malformed marker would silently break the find-and-update lifecycle without any error. A simple prefix/suffix check would catch typos. -
[Style/conventions]
internal/forge/fake.go:595—CreateIssueCommentalways returnsID: 42regardless of how many comments are created. This makes it impossible to test scenarios where multiple comments are created and then individually updated by ID. Consider using an incrementing counter (theproposalCounterpattern already exists in FakeClient). -
[Correctness]
internal/sticky/sticky.go:96-97— The doublestrings.Replacefor marker stripping (firstmarker+" ", then baremarker) could strip the marker from content that legitimately contains the marker string elsewhere in the body (e.g., in documentation about the marker). Usingstrings.TrimPrefixon the first line would be more precise since the marker is always the first line of a marked comment.
Info
-
[Intent alignment] The PR scope matches #381 well. The addition of
post-commentas a general-purpose sticky comment command (used by triage agent inpost-triage.sh) goes slightly beyond the issue scope but is a natural generalization requested in issue comments by @ralphbean. -
[Platform security] The
--tokenflag andGITHUB_TOKENenv var handling is appropriate. No tokens are logged or included in comment bodies. GraphQL mutation forMinimizeCommentcorrectly uses parameterized variables rather than string interpolation, preventing injection. -
[Injection defense] No prompt injection patterns, non-rendering Unicode, or bidirectional overrides detected in PR body, commit messages, or code strings. The HTML comment markers are fixed constants, not user-controlled.
Footer
Outcome: comment-only
This review applies to SHA 7e3a5470bba32515816d3102f3cda2d83740bb6a. Any push to the PR head clears this review and requires a new evaluation.
ralphbean
left a comment
There was a problem hiding this comment.
Review Summary
The sticky comment infrastructure (internal/sticky, forge interface extensions, CLI commands) is well-designed and well-tested. The core lifecycle — find by marker, collapse history, edit in-place — is solid.
However, the sticky comment is not wired into the existing review posting flow, so this PR doesn't yet solve #381. See inline comments for details.
There was a problem hiding this comment.
Review: #383
Head SHA: c914027
Timestamp: 2026-04-27T12:00:00Z
Outcome: approve
Summary
This PR adds sticky comment lifecycle management to the fullsend CLI, directly addressing the comment-flooding problem described in #381. The implementation is well-structured: a reusable internal/sticky package handles marker detection, history collapsing, and truncation, consumed by two new CLI commands (post-review and post-comment). The forge interface and both implementations (LiveClient, FakeClient) are extended cleanly with CreateIssueComment, UpdateIssueComment, MinimizeComment, CreatePullRequestReview, and ListPullRequestReviews. The post-triage.sh scaffold is updated to use the generic post-comment command, addressing community feedback about triage agent flooding. Test coverage is thorough across all new code paths. No critical or high findings.
Findings
Critical
None.
High
None.
Medium
None.
Low
- [Correctness]
internal/cli/readbody.go/internal/cli/postcomment.go— No validation for empty body content. If stdin is empty or the file contains no data,post-commentwill create a comment with only the HTML marker and no meaningful content.post-reviewpartially handles this for JSON input (rejects emptybodyfield) but not for plain-text input. Consider adding an empty-body check inreadBodyor at the command level.
Remediation: Add a check afterreadBody()returns: if the trimmed result is empty, return an error.
Info
-
[Correctness]
internal/sticky/sticky.go— ThelegacyDetailsRefallback regex matches<details><summary>Previous [^<]*</summary>blocks without sentinel delimiters. If review content happens to contain a<details>block with a summary starting with "Previous", it could be incorrectly extracted as a history block on the first collapse cycle. Subsequent cycles use sentinel-delimited blocks and are unaffected. This is a narrow edge case unlikely to occur in practice. -
[Style]
internal/cli/postreview.go— TheReviewResulttype is exported but appears to be used only within theclipackage. Consider making it unexported (reviewResult) unless external consumption is planned. -
[Correctness]
internal/forge/github/github.go— TheListIssueCommentspagination loop caps at 100 pages (10,000 comments). This is a reasonable safety bound but worth documenting in a code comment for future maintainers. Same applies toListPullRequestReviews.
Footer
Outcome: approve
This review applies to SHA c914027af01b8fffca57fc2df05359507ab47031. Any push to the PR head clears this review and requires a new evaluation.
There was a problem hiding this comment.
Review: #383
Head SHA: c795baa
Timestamp: 2026-04-27T12:00:00Z
Outcome: approve
Summary
This PR delivers well-structured sticky comment infrastructure that closely matches issue #381's requirements. The implementation adds CreateIssueComment, UpdateIssueComment, and MinimizeComment to the forge.Client interface with correct GitHub REST/GraphQL implementations, a reusable internal/sticky package for comment lifecycle management, and two CLI subcommands (post-review and post-comment). The code is clean, consistent with existing patterns, and has thorough test coverage across all new functionality including edge cases (spoofed markers, UTF-8 truncation, history overflow, nested details in content, error tolerance). No blocking findings.
Findings
Info
-
[correctness]
internal/cli/readbody.go:8—maxBodyBytes = 2 << 20is correct (2 MiB) but the bit-shift style (2 << 20vs the more conventional2 * 1024 * 1024or2_097_152) may cause a double-take on review. Consider a comment or the idiomatic2 * (1 << 20)for clarity. -
[correctness]
internal/cli/postreview.go—parseReviewResultsilently treats invalid JSON as plain-text input. This is by design (fallback for non-JSON review bodies), but means a malformed JSON file with a typo would be posted as-is rather than flagged. The current behavior is documented and tested, so this is informational only. -
[correctness]
internal/sticky/sticky.go— TheBuildUpdatedBodyhistory-drop loop drops the oldest entries first when overmaxSize, which is the right policy. However, ifnewBodyalone exceedsmaxSize, the fallbackTruncateBodywill cut it. This is an edge case that would only occur with a >65KB review body, which is unlikely in practice. -
[intent-alignment]
internal/scaffold/fullsend-repo/scripts/post-triage.sh— The triage script now usesfullsend post-commentwith--token "${GH_TOKEN}", which requiresfullsendto be onPATHin the triage agent's runtime environment. This is a deployment dependency worth noting in rollout.
Footer
Outcome: approve
This review applies to SHA c795baae7398db83538049df7e7416c1949ccbc6. Any push to the PR head clears this review and requires a new evaluation.
ralphbean
left a comment
There was a problem hiding this comment.
Solid implementation that directly addresses #381. The sticky comment lifecycle, spoofing protection, sentinel-delimited history, and smart truncation are all well-designed. CI is green. A few notes below for future improvement — none are blocking.
There was a problem hiding this comment.
Review: #383
Head SHA: 0da0820
Timestamp: 2026-04-28T00:00:00Z
Outcome: comment-only
Summary
This PR delivers the sticky comment infrastructure requested in #381 — a well-structured internal/sticky package, new forge.Client methods for comment lifecycle management, and two CLI subcommands (post-review and post-comment). The code is clean, well-tested, and handles important edge cases (marker spoofing protection, UTF-8-safe truncation, structured history collapse without nesting). One case in post-triage.sh was missed during the migration, and a minor style nit on a constant expression.
Findings
Medium
- [Correctness]
internal/scaffold/fullsend-repo/scripts/post-triage.sh:126— Thefeature-requestcase still usesgh issue commentinstead offullsend post-comment. The other three cases (insufficient,duplicate,sufficient) were all migrated to the new sticky comment CLI, but this one was missed. Feature-request comments will continue to flood on re-triage runs.
Remediation: Update line 126 to usefullsend post-commentwith the<!-- fullsend:triage-agent -->marker, matching the pattern used in the other cases.
Low
- [Style]
internal/cli/readbody.go:8— The constant2 << 20is correct (2 MiB) but non-idiomatic Go. The conventional expression is2 * 1024 * 1024or1 << 21. This is a readability nit only.
Info
-
[Intent alignment] The linked issue (#381) proposes simplifying
post-review.shto delegate to the new CLI. The PR adds the CLI commands but does not modifypost-review.shitself. If this is intentional (follow-up work), no action needed; if it was overlooked, consider noting it in the PR description. -
[Correctness]
parseReviewResultsilently falls back to treating input as plain text when JSON parsing fails. This is documented and tested behavior, but callers should be aware that a malformed JSON file (e.g., truncated write) will be posted verbatim rather than rejected. -
[Injection defense] No prompt injection patterns, non-rendering Unicode, or bidirectional overrides detected in the PR body, commit messages, or code content. The HTML marker approach includes spoofing protection via
botUserfiltering inFindMarkedComment.
Footer
Outcome: comment-only
This review applies to SHA 0da08208f2c79c61a890be5f5323e85b2a407dd3. Any push to the PR head clears this review and requires a new evaluation.
There was a problem hiding this comment.
Review: #383
Head SHA: 033c598
Timestamp: 2026-04-28T00:00:00Z
Outcome: comment-only
Summary
This is a well-structured PR that implements the sticky comment lifecycle requested in #381. The new internal/sticky package is cleanly separated, the forge interface extensions are consistent with existing patterns, and test coverage is thorough — including spoofing protection, history collapse across multiple re-runs, UTF-8 truncation safety, and edge cases. The post-review and post-comment CLI subcommands are well-designed with proper input validation, size limits, and dry-run support. Two medium-severity findings warrant attention: undocumented behavioral changes in post-triage.sh that go beyond the scope authorized by #381, and a redundant API call with a silenced error in the stale-head notice path.
Findings
Medium
-
[Intent alignment / Scope]
internal/scaffold/fullsend-repo/scripts/post-triage.sh— The diff removes thefeature-requestaction handler (lines 104–117 in old file) and theinformation_gapsguard (lines 104–112 in old file) that rejectedsufficientresults containing open questions. These are behavioral changes to the triage workflow that are not authorized by #381 (which is scoped to review comment flooding and sticky comments). Thegh issue comment→fullsend post-commentreplacement is in-scope, but the guard and case removals are not mentioned in the PR description or linked issue. If these removals are intentional, they should be split into a separate PR with their own justification. If accidental, they should be restored.
Remediation: Either restore the removedfeature-requestcase andinformation_gapsguard, or split them into a separate PR with an explicit rationale. -
[Correctness]
internal/cli/postreview.go:182—postStaleHeadNoticecallsclient.GetPullRequestHeadSHA()a second time (line 182) aftercheckStaleHeadalready fetched the same value. The error is silently discarded (currentSHA, _ := ...), so if this second call fails, the stale-head notice will contain an empty string for the current SHA, producing a confusing message like "HEAD is now `` ". Consider passing the current SHA fromcheckStaleHeadto `postStaleHeadNotice` instead of re-fetching it.
Remediation: Refactor `checkStaleHead` to return the current SHA alongside the stale boolean, and pass it to `postStaleHeadNotice` to avoid the redundant call and silent error.
Low
- [Style]
internal/cli/postreview.go:172-177— TheminLenhelper duplicates Go's built-inmin(available since Go 1.21). If the module targets Go 1.21+, replaceminLen(len(s), 12)withmin(len(s), 12)and remove the helper.
Remediation: Checkgo.modfor the Go version; if ≥1.21, use the built-inmin.
Info
-
[Correctness]
internal/sticky/sticky.go:43-46— WhenGetAuthenticatedUserfails, the code logs a warning and continues with an emptybotUser, which meansFindMarkedCommentwill match any author's comment containing the marker. This is documented behavior and the degradation is logged, but it's worth noting that marker spoofing protection is disabled in this path. The current approach is reasonable for resilience. -
[Correctness] Test coverage is strong: the
stickypackage has 14 tests covering create/update lifecycle, history flattening across 5 re-runs, nested<details>in content, footer preservation, overflow truncation with oldest-first eviction, UTF-8 boundary safety, spoofing prevention, empty/whitespace body rejection, and dry-run modes. Thepostreviewpackage covers JSON/plaintext parsing, stale-head detection, failure notices, formal review submission with stale review minimization, and error tolerance. The GitHub client tests usehttptestservers for realistic API simulation including pagination and GraphQL error handling.
Footer
Outcome: comment-only
This review applies to SHA 033c598117a4c3f939a18eaa034f95eaaebfe816. Any push to the PR head clears this review and requires a new evaluation.
The run-fullsend step used `echo fullsend entrypoint` which prints the literal string instead of executing the CLI, making artifact upload non-functional. Remove `echo` so the command actually runs. Also add path validation (must be absolute) and use a heredoc delimiter for GITHUB_OUTPUT to prevent injection via newlines in CLI output. Signed-off-by: Wayne Sun <gsun@redhat.com>
The echo is intentional — fullsend CLI does not have an entrypoint subcommand yet. Revert the echo removal while keeping the path validation and GITHUB_OUTPUT hardening fixes. Signed-off-by: Wayne Sun <gsun@redhat.com>
Add forge.Client methods for comment lifecycle management (CreateIssueComment, UpdateIssueComment, MinimizeComment) and a new `fullsend post-review` CLI subcommand that prevents review comment flooding on PR re-runs. The post-review command uses a hidden HTML marker (<!-- fullsend:review-agent -->) to identify its comment. On re-runs it finds the existing comment, collapses old content into a <details> block, and edits in-place. History is truncated at GitHub's 65k character limit. Changes: - forge.Client interface: add CreateIssueComment, UpdateIssueComment, MinimizeComment methods - GitHub LiveClient: implement all three (MinimizeComment uses GraphQL minimizeComment mutation via REST bridge) - FakeClient: implement all three with call recorders - internal/cli/postreview.go: new post-review subcommand with sticky comment lifecycle, dry-run mode, and stdin/file input - Unit tests for comment collapse logic, marker detection, create/update/minimize flows, truncation, and edge cases Note: pre-commit could not run (network error in sandbox). The post-script runs authoritative pre-commit on the runner. Closes #381
Extract the sticky comment lifecycle (marker detection, flat history accumulation, rune-safe truncation) from post-review into a reusable internal/sticky package. Both the review and triage agents now share one codepath for find-and-update-in-place commenting. Add a generic `fullsend post-comment` CLI command that any agent script can call with --repo, --number, and --marker flags. Update post-triage.sh to use this instead of `gh issue comment`, preventing comment flooding on triage re-runs. Fix ListIssueComments to paginate (was single-page, 100 max), add GraphQL error handling to MinimizeComment, fix readReviewBody to use io.ReadAll, and revert accidental whitespace in CreateOrgSecret. Addresses #381 code quality findings and triage sticky comment request. Signed-off-by: Wayne Sun <gsun@redhat.com>
…nt error handling Add 5 new tests covering the two zero-coverage code paths: - ListIssueComments: single page, multi-page pagination, API error - MinimizeComment: GraphQL error response, GET node_id failure Signed-off-by: Wayne Sun <gsun@redhat.com>
- Wire ReviewResult.Action to submit formal GitHub PR reviews (APPROVE, REQUEST_CHANGES, COMMENT) after posting sticky comment - Minimize stale reviews by the same user on re-runs - Fix parseReviewResult to return error on empty body instead of silently posting raw JSON as comment text - Deduplicate readBody helper into shared readbody.go - Add CreatePullRequestReview and ListPullRequestReviews to forge.Client interface with GitHub and FakeClient implementations - Add comprehensive tests for review dispatch, stale minimization, action-to-event mapping, and parseReviewResult error handling Signed-off-by: Wayne Sun <gsun@redhat.com>
MinimizeComment previously took an integer comment ID and looked it up via the issue comments REST endpoint, but minimizeStaleReviews passed PR review IDs which live on a different endpoint (wrong ID space). Change MinimizeComment to accept a GraphQL node_id string directly, which eliminates the wrong-endpoint bug and removes an extra REST round-trip. Add NodeID fields to IssueComment and PullRequestReview types so callers have the value available. Reorder submitFormalReview to minimize stale reviews BEFORE creating the new one, eliminating a TOCTOU race where the just-created review might not appear in the list due to API eventual consistency. Fix FakeClient.CreatePullRequestReview to also append to the PRReviews map so ListPullRequestReviews reflects created reviews in tests. Signed-off-by: Wayne Sun <gsun@redhat.com>
… validation - Wrap sticky history blocks with sentinel comment delimiters to prevent nested <details> in review content from corrupting history extraction - Add legacy regex fallback for backward compatibility with existing comments - Add client-side reason validation to MinimizeComment before GraphQL call - FakeClient.CreateIssueComment now uses monotonic counter and sets NodeID - FakeClient.UpdateIssueComment now mutates backing IssueComments state - Add httptest coverage for CreatePullRequestReview and ListPullRequestReviews - Add error tolerance test for minimizeStaleReviews Signed-off-by: Wayne Sun <gsun@redhat.com>
…tory truncation Harden sticky comment lifecycle against resource exhaustion (2MB input cap), marker spoofing by untrusted users (author-based filtering), and sentinel corruption on overflow (drop oldest history blocks before byte-level truncation). Add validation for empty body/marker inputs. Signed-off-by: Wayne Sun <gsun@redhat.com>
- Use strings.CutPrefix for marker stripping to avoid matching markers embedded in review content body - Log a warning when GetAuthenticatedUser fails so degraded spoofing protection is observable - Extract readBodyFrom for testability and add unit tests covering stdin, file, oversize, and not-found paths Signed-off-by: Wayne Sun <gsun@redhat.com>
…view.sh Add GetPullRequestHeadSHA to forge interface for stale-head detection. The post-review CLI now validates the PR HEAD against the reviewed SHA before posting, handles action=failure with sticky failure notices, and accepts head_sha/reason fields from the JSON result schema. Simplify post-review.sh to delegate all review posting to the CLI, retaining only result file discovery and missing-result fallback. Signed-off-by: Wayne Sun <gsun@redhat.com>
…ew API - Pass currentSHA from checkStaleHead to postStaleHeadNotice instead of making a redundant API call that silently discards errors - Add positive integer validation for --pr and --number flags - Add event allowlist validation to CreatePullRequestReview, matching the pattern already used by MinimizeComment - Route shell fallback through fullsend post-review to get sticky comment deduplication on repeated no-output failures Signed-off-by: Wayne Sun <gsun@redhat.com>
033c598 to
6fae501
Compare
There was a problem hiding this comment.
Review: #383
Head SHA: 6fae501
Timestamp: 2026-04-28T00:00:00Z
Outcome: request-changes
Summary
The core sticky-comment library (internal/sticky/) and CLI commands (post-review, post-comment) are well-structured with thorough tests, proper anti-spoofing, and correct history management. However, the action.yml scaffold contains a critical command-execution bug that would prevent fullsend from running in any repo that uses it, and there are a few correctness and style issues that should be addressed.
Findings
Critical
- [Correctness]
internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml:97— TheRun fullsendstep usesecho fullsend entrypoint "${AGENT}"instead of executing the command. Theechoprefix meansARTIFACT_DIRis set to the literal string"fullsend entrypoint <agent>"rather than the output of running the command. This breaks the entire action for all consumers — fullsend never actually runs.
Remediation: Remove theechoprefix:ARTIFACT_DIR="$(fullsend entrypoint "${AGENT}" | tail -n1)"
High
-
[Correctness]
internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml— The diff removes the vendored binary check (bin/fullsend), thePrint fullsend versionstep, theInstall OpenShellstep, and theInstall validation dependenciesstep. It also downgradesactions/upload-artifactfromv7tov4, and removes theif: always()and__install_only__guards. These are significant behavioral changes that go well beyond the scope of the sticky-comment feature (issue #381). If these removals are intentional, they should be in a separate PR with their own justification.
Remediation: Split the action.yml refactor into a separate PR, or explain/justify the scope expansion in the PR description. -
[Correctness]
internal/scaffold/fullsend-repo/scripts/post-review.sh:94— The lineACTION="comment"was removed from the protected-path downgrade block, but thejqrewrite correctly sets.action = "comment"in the modified result file. However, theRESULT_FILEvariable is reassigned to point to the modified temp file, and the subsequentfullsend post-review --result "${RESULT_FILE}"reads from it. This works, but thetrap 'rm -f "${MODIFIED_RESULT}"' EXIToverwrites the earlier trap — if there was a prior trap, it is silently lost. Minor correctness risk in edge cases.
Remediation: Use a function-based trap pattern, or consolidate temp file cleanup.
Medium
-
[Style/conventions]
internal/forge/github/github.go(GetPullRequestHeadSHA) — This method usesjson.NewDecoder(resp.Body).Decode(&pr)directly instead of the existingdecodeJSON(resp, &raw)helper used by every other method in the file. This means it also manually callsdefer resp.Body.Close(), which is inconsistent. While functionally correct, it breaks the established pattern.
Remediation: UsedecodeJSON(resp, &pr)and remove the manualdefer resp.Body.Close(). -
[Style/conventions]
internal/forge/github/github.go(CreatePullRequestReview) — Afterc.post(...),resp.Body.Close()is called withoutdeferand without checking for errors on close. This is inconsistent withUpdateIssueCommentwhich does the same thing. Both should either usedecodeJSON(which closes internally) or usedefer.
Remediation: Usedefer resp.Body.Close()for consistency, or usedecodeJSONwith a discard target.
Low
-
[Correctness]
internal/forge/fake.go—NewFakeClient()does not initialize the newIssueCommentsandPRReviewsmaps. The implementations handle nil maps gracefully (lazy init inCreateIssueComment/CreatePullRequestReview, nil check inListIssueComments/ListPullRequestReviews), so this is not a bug, but it's inconsistent with howFileContents,Secrets, etc. are initialized eagerly.
Remediation: InitializeIssueCommentsandPRReviewsinNewFakeClient()for consistency. -
[Intent alignment]
internal/scaffold/fullsend-repo/scripts/post-triage.sh— The changes to usefullsend post-commentfor triage are a nice extension of the sticky-comment pattern (as requested in issue #381 comment by @ralphbean), but the triage-agent marker is hardcoded in three separate places. Consider extracting it to a variable at the top of the script.
Remediation: DefineTRIAGE_MARKER="<!-- fullsend:triage-agent -->"at the top and reference it.
Info
-
[Injection defense] PR body and commit messages were inspected for prompt injection patterns and non-rendering Unicode. None found. The HTML marker pattern (
<!-- fullsend:review-agent -->) is correctly protected against spoofing via thebotUsercheck inFindMarkedComment. -
[Content security] The
readBodyfunction correctly caps input at 2 MB (maxBodyBytes) to prevent resource exhaustion, and the sticky comment system respects GitHub's 65k character limit with graceful history eviction.
Footer
Outcome: request-changes
This review applies to SHA 6fae50105e61bb481b5df375b894d562e1e7afa1. Any push to the PR head clears this review and requires a new evaluation.
The rebase incorrectly took the branch's stale echo-stub version of action.yml instead of main's working fullsend-run version. Signed-off-by: Wayne Sun <gsun@redhat.com>
There was a problem hiding this comment.
Review: #383
Head SHA: 28c8e7a
Timestamp: 2026-04-28T20:23:54Z
Outcome: approve
Summary
This PR delivers a well-structured implementation of sticky review comments and the post-review / post-comment CLI subcommands, directly addressing the comment-flooding problem described in #381. The code follows existing codebase patterns, has thorough test coverage across all new packages, and correctly migrates stale-head checking and comment lifecycle management from fragile shell logic into typed Go code. The internal/sticky package is cleanly separated with sentinel-delimited history blocks that survive nested <details> in review content. The forge interface extensions (CreateIssueComment, UpdateIssueComment, MinimizeComment, GetPullRequestHeadSHA, CreatePullRequestReview, ListPullRequestReviews) are properly implemented in both LiveClient and FakeClient with appropriate thread safety. No critical or high findings.
Findings
Medium
- [Correctness]
internal/cli/postreview.go:227-235— Behavioral change in stale-head handling for failure results. The originalpost-review.shskipped the stale-head check whenACTION=failure, but the Go code runscheckStaleHeadbefore the failure check. If a failure result includes ahead_shaand the HEAD has moved, the stale-head notice replaces the original failure notice, losing the failure reason. In practice this is unlikely (failure results typically lackhead_sha), but it is a subtle behavioral change from the original script.
Remediation: Consider moving the failure check beforecheckStaleHead, or skip the stale-head check whenparsed.Action == "failure"to preserve the original semantics.
Low
-
[Correctness]
internal/sticky/sticky.go:116— ThelegacyDetailsReregex uses(?s)with.*?non-greedy matching, which will stop at the first</details>tag it encounters. If old review content contains<details>blocks with a summary starting with "Previous", this could produce false matches. The sentinel-delimiteddetailsRehandles this correctly. SincelegacyDetailsReis a migration path for pre-existing comments and the sentinel approach is used for all new history blocks, this is low risk. -
[Style]
internal/cli/postreview.go:311-316— TheminLenhelper function duplicates the built-inminfunction available since Go 1.21. Consider usingmin(len(reviewedSHA), 12)directly.
Info
-
[Correctness]
internal/sticky/sticky.go:206-223—TruncateBodycan return a string slightly longer thanmaxSizewhenmaxSizeis smaller than the truncation message length (~67 bytes), sincebudgetis floored to 0 but the truncation message is still appended. This is inconsequential at the configureddefaultMaxSizeof 65000 but worth noting for correctness of the contract. -
[Intent alignment] The
post-triage.shintegration is a nice addition that addresses feedback on #381 about the triage agent having the same flooding problem. This is appropriate scope expansion. -
[Correctness]
internal/forge/github/github.go— Pagination loops inListIssueCommentsandListPullRequestReviewscap at 100 pages (10,000 items). This is a reasonable safety bound for PR contexts.
Footer
Outcome: approve
This review applies to SHA 28c8e7a6584fe61eebad143f028dd1f97fba1451. Any push to the PR head clears this review and requires a new evaluation.
Pass commit_id to CreatePullRequestReview API call so GitHub rejects the review if the PR HEAD has moved between the stale-head check and review submission, closing the TOCTOU gap. Add hex regex validation for head_sha to prevent injection of arbitrary content into stale-head notices. Replace custom minLen helper with Go 1.21+ builtin min. Signed-off-by: Wayne Sun <gsun@redhat.com>
There was a problem hiding this comment.
Review: #383
Head SHA: 2f24b52
Timestamp: 2026-04-28T00:00:00Z
Outcome: approve
Summary
This PR delivers a well-structured sticky comment system that solves the review-comment flooding problem described in #381. The implementation adds three new forge.Client methods (CreateIssueComment, UpdateIssueComment, MinimizeComment), a reusable internal/sticky package for comment lifecycle management, and two CLI subcommands (post-review, post-comment). The code is clean, follows existing patterns, has comprehensive test coverage across all new packages, and correctly handles edge cases (marker spoofing protection, stale-head detection with commit pinning, history truncation with UTF-8 safety, size overflow via oldest-entry eviction). The shell script simplification in post-review.sh and post-triage.sh properly delegates to the new CLI while preserving the protected-path enforcement logic. No critical or high findings.
Findings
Critical
None.
High
None.
Medium
- [Correctness]
internal/cli/postreview.go:308—checkStaleHeadperforms an exact string comparison betweenreviewedSHAandcurrentSHA. If any upstream producer ever provides a short SHA (7-char prefix) whileGetPullRequestHeadSHAreturns the full 40-char SHA, the comparison would incorrectly report staleness. In practice this is safe today (the review agent writes full SHAs fromheadRefOid), but the contract is fragile.
Remediation: Consider usingstrings.HasPrefixfor prefix-based matching, or document the contract thathead_shamust always be the full 40-character SHA.
Low
-
[Correctness]
internal/sticky/sticky.go:157-165—BuildUpdatedBodynumbers collapsed history entries as "Previous run", "Previous run (2)", etc. When oldest entries are evicted due to size limits, the remaining entries retain their original numbering, which may produce gaps (e.g., "Previous run", "Previous run (2)" with run (3) dropped). This is cosmetic only. -
[Correctness]
internal/cli/postreview.go:388-395—minimizeStaleReviewssilently swallows errors fromGetAuthenticatedUserandListPullRequestReviews, returning nil. This is reasonable for robustness (don't block the review over cleanup failures), but makes debugging harder when minimize consistently fails. Consider logging at a higher verbosity level. -
[Style]
internal/sticky/sticky.go:93-95— ThelegacyDetailsReregex is defined for backward compatibility with an old format that has never shipped in production (this is the first version of the sticky comment system). If no migration path exists, this dead code could be removed to simplify the regex matching logic.
Info
-
[Correctness]
internal/forge/github/github.go—ListIssueCommentsandListPullRequestReviewsboth cap pagination at 100 pages (10,000 items). This is a safe upper bound for any realistic PR. Good defensive coding. -
[Intent alignment] The PR scope matches #381 precisely: sticky comment lifecycle,
post-reviewCLI, stale review minimization, and shell script simplification. The addition ofpost-comment(for triage agent reuse) and thepost-triage.shchanges are justified by issue comment #4 (ralphbean's request for a common codepath). Scope is appropriate. -
[Platform security]
FindMarkedCommentfilters bybotUserwhen available, preventing untrusted users from spoofing HTML markers. SHA validation viahexSHAReprevents injection into commit IDs.readBodycaps input at 2 MB.MinimizeCommentvalidates the reason enum server-side. All security-relevant patterns look correct. -
[Injection defense] PR body, commit messages, and code comments contain no prompt injection patterns, non-rendering Unicode, or bidirectional overrides. The HTML marker (
<!-- fullsend:review-agent -->) is a standard HTML comment, not an injection vector.
Footer
Outcome: approve
This review applies to SHA 2f24b5207a44f6077bdd7af366e3d89a86a08fe4. Any push to the PR head clears this review and requires a new evaluation.
Tighten hexSHARe regex from {7,64} to exactly 40 (SHA-1) or 64 (SHA-256)
characters. Short SHAs would always mismatch the 40-char API response,
causing every review to be falsely treated as stale. Validate the failure
reason field against an alphanumeric+hyphen allowlist to prevent markdown
injection via crafted agent result JSON.
Signed-off-by: Wayne Sun <gsun@redhat.com>
There was a problem hiding this comment.
Review: #383
Head SHA: 19f50cdfbcecd7b92088c3e104442f669a112f77
Timestamp: 2026-04-28T00:00:00Z
Outcome: comment-only
Summary
This PR implements the sticky comment pattern requested in #381, adding post-review and post-comment CLI subcommands, forge interface extensions for comment lifecycle management, and the internal/sticky package for comment deduplication. The code is well-structured, follows existing codebase conventions, and includes thorough test coverage across all new functionality. One low-severity accuracy issue in an error message and two informational observations are noted below, but none block merge.
Findings
Critical
None
High
None
Medium
None
Low
- [Correctness]
internal/cli/postreview.go:221— The SHA validation error message says "must be a hex string (7-64 chars)" but the regex (hexSHARe) only matches exactly 40 or 64 characters. A user who provides a 7-char abbreviated SHA will get this confusing error. Update the message to "must be a 40 or 64 character hex string" or update the regex to accept short SHAs.
Info
-
[Correctness]
internal/sticky/sticky.go:196-198— TheTruncateBodyfallback could theoretically produce output slightly larger thanmaxSizeifmaxSizeis smaller than the truncation message itself (~60 chars). Not a real concern at the 65K default, but worth noting for future maintainers. -
[Style/conventions]
internal/forge/github/github.go— TheListIssueCommentsandListPullRequestReviewspagination functions share identical structure. A generic paginator helper could reduce duplication, though the current approach is consistent with the existing codebase style.
Footer
Outcome: comment-only
This review applies to SHA 19f50cdfbcecd7b92088c3e104442f669a112f77. Any push to the PR head clears this review and requires a new evaluation.
…sage accuracy - Use strings.EqualFold for SHA comparison (defense-in-depth against mixed-case SHAs from different sources) - Default empty failure reason to "unknown" instead of passing through empty string to the notice template - Fix error message to say "40 or 64 character" matching actual regex Signed-off-by: Wayne Sun <gsun@redhat.com>
There was a problem hiding this comment.
Review: #383
Head SHA: 6c4a81a
Timestamp: 2026-04-28T00:00:00Z
Outcome: approve
Summary
This PR adds a well-designed sticky comment system and post-review/post-comment CLI subcommands that solve the duplicate-comment flooding problem described in #381. The implementation is correct, well-tested (comprehensive unit tests across all new packages), and demonstrates good security hygiene — SHA validation prevents injection, bot-user verification prevents marker spoofing, GraphQL minimize reason allowlisting prevents arbitrary input, and the commit_id parameter on formal reviews closes the TOCTOU gap between stale-head checks and review submission. The internal/sticky package is cleanly separated for reuse by both review and triage agents. The post-review.sh simplification correctly delegates comment lifecycle management to Go while preserving the protected-path enforcement as the sole shell-side gate.
Findings
Critical
None.
High
None.
Medium
- [Correctness]
internal/scaffold/fullsend-repo/scripts/post-triage.sh:126— Thefeature-requestaction still usesgh issue commentdirectly instead offullsend post-comment, making it inconsistent with theinsufficient,duplicate, andsufficientactions updated in this PR. Re-triage on feature-request issues will continue to produce duplicate comments. This is a pre-existing gap partially addressed by this PR.
Remediation: Update thefeature-requestcase to usefullsend post-commentwith the triage marker, or file a follow-up issue.
Low
- [Correctness]
internal/sticky/sticky.go:116— ThelegacyDetailsReregex uses a non-greedy(.*?)with(?s)dotall mode, which could incorrectly match against content containing</details>tags in review output. The sentinel-baseddetailsReis tried first, so this only affects truly legacy comments on a one-time migration path.
Remediation: No action needed for now; the sentinel pattern ensures correct behavior going forward.
Info
- [Style]
internal/cli/postreview.go:193— The stale-head notice body uses string concatenation with backtick-escaped%sformat verbs. Consider using a template or raw string for readability. - [Correctness] The
post-triage.shfeature-requestpath (line 126) does not add the<!-- fullsend:triage-agent -->marker, so even if converted later, there will be no existing marked comments to find. This is fine since it's a separate concern.
Footer
Outcome: approve
This review applies to SHA 6c4a81a3f7f093b35e8f067b7201ed8f93416803. Any push to the PR head clears this review and requires a new evaluation.
Add forge.Client methods for comment lifecycle management (CreateIssueComment, UpdateIssueComment, MinimizeComment) and a new
fullsend post-reviewCLI subcommand that prevents review comment flooding on PR re-runs.The post-review command uses a hidden HTML marker () to identify its comment. On re-runs it finds the existing comment, collapses old content into a
Details
block, and edits in-place. History is truncated at GitHub's 65k character limit.Changes:
UpdateIssueComment, MinimizeComment methods
uses GraphQL minimizeComment mutation via REST bridge)
with sticky comment lifecycle, dry-run mode, and
stdin/file input
create/update/minimize flows, truncation, and edge cases
Note: pre-commit could not run (network error in sandbox). The post-script runs authoritative pre-commit on the runner.
Closes #381
Changed files
internal/cli/postreview.gointernal/cli/postreview_test.gointernal/cli/root.gointernal/forge/fake.gointernal/forge/forge.gointernal/forge/github/github.gointernal/forge/github/github_comment_test.goCloses #381
Post-script verification
agent/381-sticky-review-comments)027d63ade7cba9c6dc203f0ab7265f167131dce7..HEAD)Created by fullsend code agent