feat: unify review finding workflow - #139
Conversation
Rename label-review to review and add a provider-neutral review CLI. Separate finding verdicts from thread resolution so confirmed findings cannot be closed before their fixes are published.
| p_reply = sub.add_parser("reply", help="reply in a comment's thread") | ||
| p_reply.add_argument("number", metavar="number|url") | ||
| p_reply.add_argument("comment_id", metavar="comment-id", help="from `pr findings`") | ||
| p_reply.add_argument("comment_id", metavar="comment-id", help="from `pr show`") |
There was a problem hiding this comment.
🤖 devloop code-review · seed-2.1-turbo · ready in 2m 1s
The pr reply subcommand's comment_id help text was changed from from 'pr findings' to from 'pr show'. However, pr show (the cmd_show function at lines 68-91) does not print comment IDs — it only displays author and body for each comment. A user running pr show to find a comment ID to pass to pr reply would be unable to do so, since the IDs are not visible in the output.
The pr findings command was removed and moved to review findings (in devloop/scripts/review.py), which does print comment IDs. The help text should either reference review findings or pr show should be updated to display comment IDs.
ccr:fp=f05d65515830
| forge.reply(number, finding.comment, body) | ||
| except ForgeError as exc: | ||
| print(f"review label: {exc}", file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
🤖 devloop code-review · seed-2.1-pro · ready in 4m 46s
Unhandled ValueError in cmd_label when --reason is empty
cmd_label calls review_feedback.verdict_reply(ns.verdict, ns.reason) at line 130, which raises ValueError("a review verdict requires a reason") when the stripped reason is empty. However, only ForgeError is caught (lines 133-135). An empty --reason "" passes argparse (which only checks presence, not content), causing an unhandled crash.
Compare with cmd_missed (lines 185-188), which explicitly validates --reason before proceeding, showing the author intended this guard but missed it in cmd_label.
Suggested fix: Add an explicit empty-reason check before calling verdict_reply, consistent with the cmd_missed pattern, or wrap the verdict_reply call in a try/except ValueError.
ccr:fp=4062168f79f0
|
🤖 devloop code-review · 2 finding(s)（2 条已作为独立 review thread 发布） |
2 participants
Rename label-review to review and add a provider-neutral review CLI.
Separate finding verdicts from thread resolution so confirmed findings
cannot be closed before their fixes are published.