Repository navigation
feat(#6966)!: flatten rejected status, replace stale findings on ok, mount review repo read-only - #7243
feat(#6966)!: flatten rejected status, replace stale findings on ok, mount review repo read-only#7243waynesun09 wants to merge 16 commits into
Conversation
PR Summary by QodoSanitize rejected post-script statuses before logging
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Finished Review · ✅ Success · Started 7:25 PM UTC · Completed 7:37 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.87 |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsAnchored at moderate risk as Tier 1 signals remain unchanged with zero protected, security, CI, or dependency modifications and 31% test ratio, where large change size (16 files, 898 lines) and elevated git churn are balanced by trusted authorship, complete issue acceptance criteria coverage, and now-explicit breaking-change flagging. Previous runRisk Assessment: moderate (2/5) DetailsAlthough PR growth crossed into larger size buckets (16 files, 898 lines), the change retains zero protected/security/CI/dependency modifications, trusted authorship, and solid test coverage (31%), anchoring the score at moderate (2) alongside elevated git history churn and unflagged comment-editing behavior modifications. Previous run (2)Risk Assessment: moderate (2/5) DetailsA well-tested, narrowly-scoped bug fix (33% test-file ratio, no protected/security/CI/dependency paths flagged, no author-risk signals) touching files with moderate churn and a history of fix/revert commits, referencing an already-closed tracking issue and modifying existing template/generator behavior without a rollback flag; these moderate git-history and rollback-safety signals offset the otherwise low metadata risk, yielding a moderate composite score. Previous run (3)Risk Assessment: moderate (2/5) DetailsAnchored re-review: despite the diff growing ~3.8x since the prior assessment (136->512 lines, 9->14 files) and crossing FILES_CHANGED/LINES_CHANGED buckets, BLAST_RADIUS was already maxed pre-growth so the Tier 1 composite is essentially unchanged; combined with stable Tier 2 churn/author signals and a well-tested, narrowly-scoped security-hardening fix with no protected paths, dependency changes, or CI changes, the weighted composite preserves the prior moderate score of 2/5. Previous run (4)Risk Assessment: moderate (2/5) DetailsWeighted composite (~1.54, rounded to 2): small, well-tested, non-security-flagged hardening fix in an actively-churning but single-author-dominated new package, addressing a narrow slice of a larger epic; consistent with the PR's own risk/moderate label. Previous run (5)Risk Assessment: moderate (2/5) DetailsTier 1 signals are unchanged from the prior three assessments on this PR (still a small/medium 9-file, 136-line change, no protected/security/CI/dependency paths, moderate test ratio, non-bot returning contributor), Tier 2 shows only moderate churn/fix-revert history concentrated in one docs file and the previously flagged render_test.go coupling gap is now closed by this PR, and Tier 3 remains unavailable (linked issue closed, no actionable scope), so per re-review anchoring the composite stays at the prior moderate score of 2/5. Previous run (6)Risk Assessment: moderate (2/5) DetailsRe-scoring the complete 8-file/130-line PR reproduces the same qualitative Tier 1 profile as prior reviews -- small-to-medium blast radius, no protected/security/CI/dependency paths, moderate 0.25 test ratio, non-bot returning contributor -- and Tier 2 shows only modest churn/fix-revert history on the touched golden/template files; with no acceptance-criteria-bearing linked issue to score (Tier 3 unavailable, weights redistributed 62/38 to Tier 1/Tier 2), the weighted composite rounds to a moderate score of 2, unchanged from prior assessments. Previous run (7)Risk Assessment: moderate (2/5) DetailsRe-review at head c2f4acd (103 lines vs. the prior 58-line commit, same 4-file set) reproduces the same Tier 1 profile as the prior assessment - small blast radius, no protected/security/CI/dependency paths, moderate test ratio, non-bot returning contributor - and Tier 2 shows only moderate churn/fix-revert history on the touched golden/template files with one coupled file (render_test.go) absent from the PR; with no acceptance-criteria-bearing linked issue to score (Tier 3 unavailable, weights redistributed 62/38 to Tier 1/Tier 2), the weighted composite again rounds to a moderate score of 2, unchanged from the prior review. Previous run (8)Risk Assessment: moderate (2/5) DetailsSmall, self-contained security-hardening fix (4 files, 58 lines, no protected/security-sensitive/CI/dependency paths, 0.25 test ratio, includes a targeted regression test) pulls Tier 1 low; moderate Tier 2 churn/fix-revert history on the touched golden/template files plus a coupled file not included in the PR, and a Tier 3 pull-up from lack of a rollback flag for this behavior change combine to a moderate composite score of 2. |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)Looks good to me Previous run (4)ReviewFindingsLow
Next steps:
Previous run (5)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (6)Looks good to me Previous run (7)ReviewFindingsLow
Next steps:
Previous run (8)Looks good to me Previous run (9)ReviewFindingsLow
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.
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 7:44 PM UTC · Completed 7:58 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.95 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Review · ❌ Terminated · Started 8:14 PM UTC · Ended 8:29 PM UTC Commit: |
Site previewPreview: https://6a06083c-site.fullsend-ai.workers.dev Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Superseded by updated review
fullsend-ai-review
Bot
removed
the
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 8:14 PM UTC · Completed 8:29 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.90 |
|
🤖 Finished Review · ✅ Success · Started 8:33 PM UTC · Completed 8:43 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.52 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…s its own issues post-comment (GitHub, GitLab) edited the first comment carrying the marker, whoever wrote it. A planted marker comment therefore received the findings, and the --only-if-exists all-clear, which checks the author, could never replace them. The poster's identity is now always resolved, and an existing comment is edited only on an exact author and marker match; a planted one is ignored and a new comment posted. When the identity cannot be resolved, no comment is edited: a new one is posted with a warning naming the reason, or nothing under --only-if-exists. An empty identity matches nothing by construction. Jira, whose markers are comment properties, is unchanged. Help text and the user guide's trust model drop the "acceptable in trusted CI" caveat for the new rule. BREAKING CHANGE: issues post-comment (GitHub, GitLab) no longer edits a marker comment written by a different identity; after a token rotation or App migration the next run posts a new comment instead. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…s comment A comment with no author and an unresolved (empty) identity must not match. The lookup's empty-identity guard was only covered indirectly. Help text, the user guide and the generated script's comment now say an existing comment is found by its marker and its own author. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…its identity Posting a new comment when the poster's login could not be resolved avoided editing an unverified comment, but left the earlier comment behind: later runs keep updating the oldest own comment, so a findings comment orphaned by one transient failure would never be cleared. The identity lookup now retries briefly (two retries, context-aware) and, if it still fails, the command returns an error naming the cause and asking for a rerun, posting and editing nothing, with or without --only-if-exists. The generated post-script runs under set -e, so the run then fails visibly; its comment says so. BREAKING CHANGE: issues post-comment now fails, posting nothing, when it cannot resolve the identity it posts as. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…y guard A cancelled context stops the retry without waiting out the backoff, and an empty identity matches no comment, not even an authorless one. The --dry-run help now says it posts or edits nothing rather than "makes no API calls": it lists comments and resolves the poster's identity. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
changed the title
e340bce to
069e284
Compare
|
🤖 Finished Review · ✅ Success · Started 2:07 AM UTC · Completed 2:47 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.57 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/cli/issues.go(file-level): Line 352 · [medium] fail-open
The new author-identity verification and fail-closed behavior this PR adds to fullsend issues post-comment covers the GitHub/GitLab path (postTrackerStickyComment, which calls resolveTrackerSelf and requires findMarkedTrackerComment's author match before treating a comment as existing) but not the Jira path (postJiraStickyComment). There, existing := jc.FindCommentByMarkerProperty(jiraComments, cfg.Marker) at line 352 still matches on the marker property alone, with no author/identity check, and the --only-if-exists branch for Jira (line 380) inherits the same marker-only lookup. This is exactly the comment-spoofing shape (a planted marker-bearing comment being treated as the bot's own) that this PR's GitHub/GitLab change was built to close, left open on the one tracker this PR did not update. The PR body states "Jira, whose markers are comment entity properties, is unchanged" as if comment-property markers made the identity question moot, but a marker property is set by whoever posts the comment, including a non-bot actor, so the same planted-comment risk applies.
Suggested fix: Apply the same identity gate on the Jira sticky path: resolve the authenticated Jira user (or document why Jira's comment-property markers are not forgeable the way GitHub/GitLab comment bodies are, if that is actually true), and require both the marker property and author to match before editing or before --only-if-exists treats a comment as existing.
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
PRs should contain only one change at a time.
fullsend-ai/fullsend#7243 was split into #7914 (status sanitiser), #7915 (readonly_repo), #7916 (comment-author check) and #7917 (--only-if-exists, stacked on #7916). Regenerating from all four merged onto fullsend main gives output byte-identical to #7243's last head, so only the reference changes. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
fullsend issues post-comment exits non-zero without posting when it cannot resolve the login it posts as (fullsend-ai/fullsend#7243). The post-script relies on set -e to surface that. A stub that lists --only-if-exists but fails every post now checks that both the ok and the findings path exit non-zero after attempting the post. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
The example's step 1 matched ISSUE_URL against a regex containing `https://github[.]com/` inside the bash block. Fullsend's SSRF PreToolUse hook, on by default for Bash, scans the command for URLs and refuses that one as malformed, so an agent that followed the prompt would hit the step's error branch and report an error on every pull request. Use the REPO_FULL_NAME and ISSUE_NUMBER the harness now passes (set for pull-request dispatches too) and drop the URL from the command. The README and the authoring skill now also tell users to replace the generated prompt's `gh issue view`, which goes through GraphQL that the generated profile does not allow (fullsend-ai/fullsend#7014), with the REST equivalent. post-link-check.sh is regenerated from fullsend-ai/fullsend#7243 at e340bcefa: a comment-only change saying post-comment fails the run when it cannot resolve its own login. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
fullsend-ai/fullsend#7243 was split into #7914 (status sanitiser), #7915 (readonly_repo), #7916 (comment-author check) and #7917 (--only-if-exists, stacked on #7916). Regenerating from all four merged onto fullsend main gives output byte-identical to #7243's last head, so only the reference changes. Assisted-by: Claude (fix), Claude (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…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
Follow-up to #6972, from a review finding on fullsend-ai/agents#1167.
This PR does three things
flatten a rejected status…throughlog the rejected status with printf….jq -rturns JSON\n/\rescapes into real line breaks. A craftedstatuscould therefore split the runner's log line and start a new one with a::workflow-command prefix.printf '%s\n'. Unlikeechounderxpg_echo,printfnever expands backslash escapes.agent newgeneratesreadonly_repo: truefor thereviewrole. Commitsgenerate readonly_repo…andpin readonly_repo….status: ok, replace stale findings with the all-clear. Commitson ok, replace an earlier findings comment…throughpin the identity lookup's cancellation….fullsend issues post-comment --only-if-existsflag. It's documented indocs/guides/user/issues-commands.mdanddocs/guides/dev/cli-internals.md.--only-if-exists, the earlier comment must carry the marker and have, as its author, exactly the login fullsend posts as. That login comes fromforge.Client.GetAuthenticatedUser, which also resolves App installation tokens through GraphQLviewer. Login shape (such as a[bot]suffix) is never used.post-comment --help) and keeps the old "post nothing" behaviour on a runner whose pinned fullsend predates it.issues post-commentcall on GitHub and GitLab (never edit a marker comment…). Before, the findings path edited whichever comment carried the marker first. A planted comment would receive the findings, and the all-clear could then never replace them.--only-if-exists. Posting a new comment instead would orphan the earlier one, whose stale findings no later run would update. The generated post-script runs underset -e, so the run fails visibly and a rerun recovers.sticky.Poststill falls back to marker-only matching when identity lookup fails; that is sticky.Post: never edit a marker comment when the poster's identity cannot be resolved #7884.fullsend-ai/agents#1167's
examples/link-check/already ships all three changes verbatim from this generator, so this PR must merge before agents#1167.Release note
fullsend issues post-comment --only-if-existsupdates the poster's own earlier marker comment and never creates one.issues post-commentno longer edits a marker comment it can't verify as its own; it posts a new one instead, and it fails instead of posting when it can't verify its own identity.fullsend agent new --role reviewnow generatesreadonly_repo: true.statusbefore logging it, and replace an earlier findings comment with the all-clear on an ok result.BREAKING CHANGE
fullsend issues post-comment(GitHub, GitLab):--dry-runresolves the identity too. A dry run now fails if it cannot resolve the posting identity. Before, it needed no identity at all.Jira is unchanged. The deprecated
fullsend post-comment(sticky.Post) is unchanged here; #7884 tracks bringing it to the same rule.Tests
internal/agentnew/postscript_exec_test.go:statuswith an LF, CR or CRLF line break followed by::error::injectedis rejected on a single log line.bash -O xpg_echo, a literal backslash-n stays on one line.internal/cli/issues_test.go(normal path and--only-if-exists):mallory,evil-review[bot],fullsend-ai-review,x-fullsend-ai-review[bot]). The normal path posts a new comment;--only-if-existsposts nothing.--only-if-existscreates nothing when no earlier comment exists, and skips a planted comment listed before the poster's own.--only-if-exists.GITHUB_TOKEN: GraphQLviewer.login== the posted comment's author ==github-actions[bot].roles_test.go:TestReviewRoleMountsTheRepositoryReadOnly.printf→echo[bot]-suffix author matchPart of #6966. Closes #7886.