ADORSYS-GIS / ADORSYS-GIS/lightbridge-code-intelligence

[Ticket]: Deep-tier review flips severity (P1↔P2) on the same finding across same-commit re-reviews

Offen
#285 3 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
ticket
Vorherrschende Sprache
Rust
Sterne
0
Forks
0
Ø Merge
14 Std. 13 Min.
Gemergte PRs (30 T.)
16

Beschreibung

### Type

Bug (with an investigation/spike first step — the observable defect is clear; the stabilisation approach needs measurement).

### Summary

We need **deep-tier review severity (P0/P1/P2) to be stable for the same finding across re-reviews of an unchanged commit**, because today the same underlying issue flips between **P1 (blocking)** and **P2 (non-blocking)** from one run to the next with no code change — which destroys trust in the blocking signal (an author can't rely on "this is a merge blocker").

Expected result:
> Re-reviewing the same commit yields the same *classification* for the same finding. Coverage may legitimately differ (more files read → more findings); the **severity of a finding both runs actually make** should not.

### Intent

The P1↔P2 line is the whole product contract: P1 = "fix before merge", P2 = "nit". If that classification is non-deterministic on identical code, the review's core signal is unreliable — worse than a missed nit, because it teaches authors to distrust every severity. This ticket is about **classification stability**, not about finding *more* (that's coverage, already addressed by the diff-budget/ADR-0069 work).

### Source of truth (links)

- Epic [#252](https://github.com/adorsys-gis/lightbridge-code-intelligence/issues/252) — review quality & reliability.
- [ADR-0065](https://github.com/adorsys-gis/lightbridge-code-intelligence/blob/main/docs/adr/0065-re-review-dedup-and-reconciliation.md) — same-commit re-reviews deliberately run fully and *can* differ; this ticket argues **severity flips are outside the intended tolerance** and reconciliation stabilises *findings* but not their *severity*.
- Evidence — two deep re-reviews on the **same commit** (`29aa25e2`), **no commit in between**, `lightbridge-assistant` on webank-mobile #145:
- Review [4643335072](https://github.com/ADORSYS-GIS/webank-mobile/pull/145#pullrequestreview-4643335072) (08:55Z)
- Review [4644473735](https://github.com/ADORSYS-GIS/webank-mobile/pull/145#pullrequestreview-4644473735) (11:16Z)

### Current Behavior

Both reviews above ran on commit `29aa25e2` (deep tier, `glm-5p2`) with **no code change between them**. Same underlying issues got different classifications/anchors:

| Underlying issue | Review 4643335072 | Review 4644473735 |
|---|---|---|
| Cancel refund failure leaves orphaned row | **P1** (`repository.go:312`) | **P2** (`service.go:691`) |
| `ReconcilePendingIntents` false-FAILED | P1 `service.go:367` | P1 `service.go:175` (anchor drift, same symbol) |
| Idempotency-middleware rejects a pending route | P1 "GET `/payments/pending`" (`main.go:543`) | P1 "Cancel POST" (`main.go:544` + dart client) — reframed target |

**Stable across both (good, for contrast):** reflected XSS on `/claim/`, `hashPhone` no normalisation, `GetByClaimToken` missing status filter, `ReconcilePendingIntents` existence — the high-severity backend signal is consistent; the instability is in *borderline* severity and anchoring.

**Explicitly NOT the bug:** coverage also differed (18/37 vs 32/37 files), but that is **fully explained** by the diff-budget increase ([ai-helm-values#48](https://github.com/ADORSYS-GIS/ai-helm-values/pull/48)) landing between the two runs (live ~09:47Z) — expected and desirable. This ticket is the **residual severity/anchor non-determinism** that remains after accounting for coverage.

### Expected Behavior

- A finding both runs make gets the **same P0/P1/P2** classification (no P1↔P2 flips on the same issue).
- Anchors for the same symbol don't drift by hundreds of lines between runs.
- Genuinely borderline findings are classified *consistently*, not coin-flipped.

### Acceptance Criteria

- [ ] Given the same commit re-reviewed N times (deep tier), when a finding is produced in ≥2 runs, then its severity classification is identical across those runs (target: 0 P1↔P2 flips on the same issue over a golden set).
- [ ] Anchor stability: the same symbol/finding anchors to the same (or adjacent) line across runs.
- [ ] A measurement exists (variance metric per finding over K repeats) so regressions are catchable — ties into the fast/deep eval harness (ADR-0049).
- [ ] No regression: legitimate *escalation* when a later run genuinely learns more (e.g. reads a file the first couldn't) is still allowed — only same-evidence flips are the target.
- [ ] Verification evidence provided.

### Out of Scope

- Coverage differences between runs (owned by the diff-budget / ADR-0069 coverage-gate work).
- The correctness of the specific webank-mobile #145 findings themselves.
- Fast-tier severity (fast is diff-only, single-pass; this is about deep re-review stability).

### Technical Context

- Deep-tier native agent: `services/agent-runner/src/review/native/` (agent loop + finish/verdict).
- Severity comes from model judgement against the system-prompt rubric (P0/P1/P2), with no per-issue-class anchoring — a likely source of run-to-run variance.
- [ADR-0065](https://github.com/adorsys-gis/lightbridge-code-intelligence/blob/main/docs/adr/0065-re-review-dedup-and-reconciliation.md) reconciliation dedups *findings* against priors but does not appear to stabilise their *severity*; the prompt's re-review stanza tells the model to re-derive, which re-rolls severity each time.
- Root-cause hypotheses to investigate: (1) severity is unanchored model judgement → variance; (2) reconciliation could carry a prior finding's severity as a prior/anchor unless the current evidence clearly warrants a change; (3) a deterministic severity rubric (concrete P1-vs-P2 tests per category) in the prompt.

### Risks

- Over-stabilising could suppress a legitimate escalation when a later run genuinely has more evidence → mitigate by targeting only same-evidence flips, and by letting reconciliation *raise* on new evidence but not silently *drop*.
- Anchoring severity to priors risks perpetuating a mis-severity → any carry-over must be overridable by clear current evidence.

### Test Plan

- Re-run deep review on a fixed golden commit K times (K≥5); compute per-finding severity variance and anchor drift; assert 0 P1↔P2 flips on findings common to ≥2 runs.
- Regression: a case where run 2 legitimately reads a file run 1 couldn't still allows escalation.
- Manual: reproduce with the two linked webank-mobile #145 runs as the seed cases.

### Verification evidence

Two live runs on the same commit, captured from runner telemetry (`lightbridge-agents` ns, hetzner-prod):
- `task 63c220de` = review 4644473735, 112 turns, diff not truncated, coverage 32/37.
- The 08:55Z run = review 4643335072, coverage 18/37 (pre-diff-budget-bump).
- The flips/anchor drift tabulated under **Current Behavior** are quoted verbatim from the two review bodies.

### Human accountable owner

@stephane-segning

### AI Usage Declaration

Drafting the ticket · Understanding code · Reviewing the diff.

### Human verification completed

_Left for the accountable owner._
- [ ] I understood the intent
- [ ] I checked the source of truth
- [ ] I reviewed all AI-generated text/code
- [ ] I verified the implementation manually
- [ ] I checked for hallucinated assumptions
- [ ] I documented remaining risks
- [ ] I am the accountable owner and accept responsibility for this ticket.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.