spike: evaluate and improve the PR review path

Aperta
  • Qualcosa non va?
#432 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
5/5
Tempo stimato
Più di una settimana
Idoneità per principianti
20/100
Tipo di issue
Funzionalità
Chiarezza
Abbastanza chiara
Stato di attività
Attiva
Stack tecnologico
csharp, github
Ambito
devtools

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.
Lingua principale
C#
Stelle
1
Fork
0
Merge medio
2h 2m
PR unite (30g)
98

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di nevridge/TaskFlow

Tutte le issue di nevridge/TaskFlow

Issue simili

Altre issue su C#

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.