Test AI review comment resolution - #3898
Conversation
Test UCI branch that introduces AI comment resolution. See: sei-protocol/uci#58
PR SummaryLow Risk Overview The reusable AI Review workflow no longer pins UCI v0.0.13 ( Reviewed by Cursor Bugbot for commit 98cb269. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This test PR breaks the build: go.mod declares go 1.2x5.6, which is not a parseable Go version directive, so all Go tooling fails. It also pins the AI-review workflow to an unmerged UCI PR commit rather than a release tag.
Findings: 2 blocking | 3 non-blocking | 3 posted inline
Blockers
- Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output, so this review is based on Codex's findings plus my own analysis only. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- PR is described as a test branch for UCI comment resolution; confirm both the
go.modedit and the temporary workflow pin are reverted before merge, or close it without merging. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3898 +/- ##
==========================================
- Coverage 59.45% 58.46% -1.00%
==========================================
Files 2319 2225 -94
Lines 198379 187879 -10500
==========================================
- Hits 117946 109835 -8111
+ Misses 69235 67683 -1552
+ Partials 11198 10361 -837
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@seidroid review |
|
Claude finished @masih's task in 1m 24s —— View job ReviewNet diff against
✅ Previous blocker is resolvedThe earlier CHANGES_REQUESTED review flagged
🔴 Blocking before merge — unreleased ref pin
Revert both lines to a release ref (and restore the 🟡 Non-blockingPin drift across UCI callers. When you revert, worth landing on the newest tag rather than back to v0.0.13:
Not this PR's job to unify, but Missing the dual-ref warning comment. Notes
Verdict: the build break is gone; the only thing standing between this and mergeable is the unmerged-UCI pin, which the PR description says is intentional and temporary. |
There was a problem hiding this comment.
The go.mod build breakage from the previous review is fixed, but the AI-review workflow now resolves both the reusable workflow and uci-ref to a mutable branch on sei-protocol/uci while inheriting all secrets, pull-requests: write, and OIDC id-token: write — a strictly worse supply-chain posture than the SHA pin it replaced. Acceptable only as a throwaway test branch; it must not merge in this state.
Findings: 3 blocking | 3 non-blocking | 3 posted inline
Blockers
- Cursor's second-opinion review file (
cursor-review.md) is empty again — that pass produced no output, so this review combines Codex's findings with my own analysis only. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- This is self-described as a test PR for UCI comment resolution (sei-protocol/uci#58). Before merge, all three temporary edits must be reverted together: the workflow ref,
uci-ref, andre-review-on-push. Otherwise close it without merging. re-review-on-pushis a new input that likely only exists on the UCI test branch. Note the coupling: reverting the two refs to v0.0.13 without also removing this line will fail the reusable-workflow input validation, so the revert has to be done as one change.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
No changes since the last review: the AI-review workflow still resolves both the privileged reusable workflow and uci-ref to a mutable branch on sei-protocol/uci while inheriting all secrets, pull-requests: write, and OIDC id-token: write. This is a throwaway test branch and must not merge in this state.
Findings: 4 blocking | 3 non-blocking | 3 posted inline
Blockers
- Cursor's second-opinion review file (
cursor-review.md) is empty for the third consecutive review — that pass produced no output, so this review combines Codex's findings with my own analysis only. - No commits have landed since review 4907309715: the diff is byte-identical, so every finding from that review is still open.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- This is self-described as a test PR for sei-protocol/uci#58. Before merge, all three temporary edits must be reverted together: the
uses:ref,uci-ref, andre-review-on-push. Otherwise close it without merging. - The revert is coupled:
re-review-on-pushlikely only exists as an input on the UCI test branch, so restoring the two refs to a released SHA without also deleting that line will fail the reusable workflow's input validation. Do it as one change. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| # See: https://github.com/sei-protocol/uci/releases/tag/v0.0.13 | ||
| uses: sei-protocol/uci/.github/workflows/ai-review.yml@29a9c73301b2218e1940cf3f9a2c3d34c86fbd9a | ||
| # Test: https://github.com/sei-protocol/uci/pull/58 | ||
| uses: sei-protocol/uci/.github/workflows/ai-review.yml@masih/ai-review-comment-mark-as-resolved |
There was a problem hiding this comment.
[blocker] Still unaddressed from the previous two reviews. This replaces a commit SHA pin with a mutable branch ref (@masih/ai-review-comment-mark-as-resolved) on a job that runs with secrets: inherit, pull-requests: write, and id-token: write. Any later push to that branch — by anyone with write access to sei-protocol/uci, or via a compromised account — immediately executes new privileged code against this repo's inherited secrets and OIDC identity on every pull_request event, with no review here.
This is a regression from the prior state, which at least pinned an immutable commit. Restore a SHA pin (@29a9c73301b2218e1940cf3f9a2c3d34c86fbd9a for v0.0.13, or the audited head SHA of the test branch if you need to keep testing) along with the See: .../releases/tag/vX.Y.Z comment before this merges.
(Same point raised by Codex.)
| # See: https://github.com/sei-protocol/uci/releases/tag/v0.0.13 | ||
| uci-ref: 29a9c73301b2218e1940cf3f9a2c3d34c86fbd9a | ||
| # Test: https://github.com/sei-protocol/uci/pull/58 | ||
| uci-ref: 'masih/ai-review-comment-mark-as-resolved' |
There was a problem hiding this comment.
[blocker] Still unaddressed. Same issue as line 8: uci-ref is a mutable branch name. This is the ref the reusable workflow checks out to source the UCI tooling it runs, so it is a second, independent path to executing unreviewed code with this repo's inherited secrets. Pin it to the same audited commit SHA as the uses: above — the two should never point at different revisions.
| # Test: https://github.com/sei-protocol/uci/pull/58 | ||
| uci-ref: 'masih/ai-review-comment-mark-as-resolved' | ||
| enable-cursor: false # Disabled for now since there is a dedicated Bugbot flow built into Cursor currently enabled on repo. | ||
| re-review-on-push: true # Enabled for testing only No newline at end of file |
There was a problem hiding this comment.
[suggestion] Still unaddressed. Two things on this line:
re-review-on-push: true # Enabled for testing only— the comment states this is temporary, so it needs to be removed (or deliberately kept with a real justification) before merge. Left as-is, every push to every PR triggers a full AI re-review.- The file still does not end with a newline (
\ No newline at end of file). Restore the trailing newline.
|
Testing looks good. closing. |
1 participant
Test UCI branch that introduces AI comment resolution.
See: sei-protocol/uci#58