spike: evaluate and improve the PR review path

Open
  • Is something wrong?
#432 2 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
20/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Active
Tech stack
csharp, github
Domain
devtools

Research direction

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.

Written by the indexing model from the issue text.

Description

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.
Dominant language
C#
Stars
1
Forks
0
Avg merge
2h 2m
Merged PRs (30d)
98

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from nevridge/TaskFlow

All issues in nevridge/TaskFlow

Similar issues

More C# issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.