spike: evaluate and improve the PR review path

Đang mở
  • Có gì không ổn?
#432 2 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức phù hợp với người mới
20/100
Loại issue
Tính năng
Độ rõ ràng
Khá rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
csharp, github
Lĩnh vực
devtools

Hướng nghiên cứu

Start by reading the user-level pr-triage, pr-review, and pr-scoped-reviewer.md files named in the issue, along with the relevant PR diffs and follow-up fixes. Build the back-test table and record whether each bug was caught, dropped, or never raised, then use those results to assess the proposed changes. Done means the table and findings are posted, any adopted changes are re-tested against it, and the stated skill changes are complete.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

tech-debt

Found while auditing the pipeline for #400 (piece 7). The PR review path (pr-triage, which runs pr-review and /code-review medium and then triages the findings) often returns a clean review, and the owner suspects it is missing real bugs. There is no data either way. This spike measures it and, if the data says so, improves it.

All of the review skills and agents involved are user-level files on the owner's machine (~/.claude/skills/pr-triage, ~/.claude/skills/pr-review, ~/.claude/agents/pr-scoped-reviewer.md), so there is no repo change unless the spike produces one.

Background

  • The built-in /code-review was too critical about small issues and kept producing new changes on each pass, so a PR almost never got a clean review.
  • pr-review (a thin wrapper around pr-scoped-reviewer) fixed the noise but seemed to miss bugs.
  • pr-triage combines both and adds a verify-and-rank step with a human walkthrough. It is the only one still in use.

Hypotheses to test (from reading the skill text, not from run data)

  1. pr-scoped-reviewer may only cite lines the diff adds or changes and is told to omit anything in doubt, so it cannot report an omission (a caller, validator, migration or mapping that should have changed and did not).
  2. The filters stack: the reviewer drops what it cannot prove, then the triage step drops what is speculative or pre-existing, so a real bug can be lost as "speculative".
  3. Neither reviewer sees the issue's acceptance criteria, so "does it do what was asked" is checked only by implementation-validator on pipeline PRs.
  4. Neither reviewer is told this repo's known traps (enum casing, journal carry-forward, timezone, compose volumes).
  5. The two reviewers are the same model on the same diff, so agreement between them adds little confidence about what both missed.

Scope

  1. Build a back-test set: merged PRs that were later followed by a fix (a bugfix/ branch, a fix: commit or a bug issue that traces to the earlier PR). Record for each one the bug, the PR it came from, and the category (omission, logic, project-specific trap, other).
  2. Run the current pr-triage reviewers (pr-scoped-reviewer and /code-review medium) on each original PR diff, read-only. Record whether the bug was caught, found but dropped in verification, or never raised, and which layer lost it.
  3. Over the next several live pr-triage runs, note whether anything on the dropped list later proves real.
  4. From the results, decide which of these to apply, if any:
    • allow omission findings that cite the changed line and the unchanged code that should have changed
    • a "needs a human look" bucket for speculative critical or important candidates, in place of silent drops
    • pass the issue's acceptance criteria and the CLAUDE.md Safe Change Rules to the reviewers
    • drop one of the two reviewers if it finds nothing the other does not
  5. Change pr-triage Step 2 to call pr-scoped-reviewer directly with the diff it already fetched, then retire the pr-review skill. Do these two together: deleting pr-review first would break pr-triage, which invokes it. This also removes the duplicate diff fetch.
  6. Remove the "shell quoting mangles" sentences in pr-triage (Step 5b item 4 and Step 5c item 5). The --input <file> mechanism for the review JSON stays. #400 decided this cut in piece 2.

Acceptance criteria

  1. A back-test table is posted on this issue: each known bug, the PR it came from, and the result (caught, dropped in verification, or never raised) for the current review path.
  2. The findings say which layer lost the misses: reviewer scope, the verification filter, or missing context.
  3. Each change from scope item 4 is either applied or rejected, with a reason that points at the table.
  4. After any change, the back-test is re-run and the result is posted. The miss rate is no worse, and the noise on a few recent clean PRs has not clearly grown.
  5. pr-triage no longer invokes pr-review, and the pr-review skill is gone (scope item 5).

Out of scope

  • The validator's role in the feature-factory pipeline. #400 decided it checks the diff against the approved brief only; revisit if this spike finds omissions need catching before the PR.
  • Running pr-triage from the pipeline. #400 decided it stays manual; the pipeline prints /pr-triage <N> after the PR is created.
  • Any change to the built-in /code-review.
Ngôn ngữ chính
C#
Star
1
Fork
0
Merge trung bình
2 giờ 2 phút
Pull request đã merge (30 ngày)
98

Chuẩn bị môi trường

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của nevridge/TaskFlow

Tất cả issue của nevridge/TaskFlow

Issue tương tự

Thêm issue về C#

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.