ADORSYS-GIS / ADORSYS-GIS/lightbridge-code-intelligence
[Ticket]: Deep-tier review flips severity (P1↔P2) on the same finding across same-commit re-reviews
- 主要语言
- 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 还没有评估数据。