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

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

未关闭
#285 3 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
ticket
主要语言
Rust
星标
0
派生
0
平均合并
14 小时 13 分钟
30 天内合并 PR
16

描述

### 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.

贡献指南

打开贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。