Outcome sync can count rejected developer replies as accepted findings

[mountainowl]

Summary

Bubo outcome sync can mark a disputed developer reply as an accepted/resolved finding.

The concrete failure seen on ...dev/commonservice!93:

• Bubo posted a blocking correctness finding.
• A developer later replied that the reported behavior is expected: This is fine since we do expect cabins to be correct even for old paths.
• The GitLab discussion is resolved.
• After running bubo-poller --sync-outcomes, Bubo correctly updates resolved=true, but still keeps disputed=false, false_positive=false, and the project dispute aggregate remains correctness: total=1, rejected=0, dispute_rate=0.0.

That makes a rejected finding look accepted in ROI/quality metrics.

Observed Environment

• Bubo version: 0.22.0
• Host root: /usr/local/bubo
• Command run:

BUBO_ROOT=/usr/local/bubo \
PATH=/home/user/.local/bin:/usr/local/bin:/usr/bin:/bin \
flock -n /var/run/bubo/outcome-sync.lock \
/usr/local/bubo/bin/bubo-poller --sync-outcomes --sync-limit 500

• Sync result:

{"classified": 4, "event": "outcome_sync_done", "synced": 63, "ts": "2026-06-19T00:49:02+00:00"}

Repro Facts

1. Original Bubo finding

Stored in review_findings:

{
  "project": ".../commonservice",
  "iid": 93,
  "sha": "1f10edeac316285bc44c0864077765cbf1e644c5",
  "fingerprint": "f79a8d06f47c2bf3e1826f4083ef3c4216701dd9f905dee5d51b0d5ed45b6dee",
  "file": "src/main/java/com/.../multisource/sentinel/checks/FareCheck.java",
  "line": 68,
  "status": "posted",
  "type": "issue",
  "severity": "blocking",
  "category": "correctness",
  "confidence": 0.91,
  "discussion_id": "09e640179bbd57791767435386eae0d6dd6a3ef6",
  "updated_at": "2026-06-12T01:02:12+00:00"
}

Finding body:

Issue (blocking, correctness): Disabled cabin thresholds still require parsing the cabin

Impact: When MIN_BASE_FARE_BY_CABIN_ENABLED is false, fares with any cabin string not accepted by the new Cabin enum are marked ERROR_GETTING_MIN_BASE_FARE instead of being checked against the legacy MIN_BASE_FARE threshold.

Evidence: validateBaseFare computes minBaseFareByCabinEnabled, but then unconditionally calls Cabin.fromConfigurationValue(filingOutput.getCabin()) before the ternary chooses between per-cabin and global MIN_BASE_FARE. The disabled-path tests only cover Cabin.BUSINESS, so they do not catch legacy/custom cabin values.

Fix: Only parse filingOutput.getCabin() inside the minBaseFareByCabinEnabled branch; when disabled, read MIN_BASE_FARE directly without depending on cabin normalization.

Confidence: 0.91

2. GitLab discussion state

GitLab discussion 09e640179bbd57791767435386eae0d6dd6a3ef6 currently has:

• Bot/finding note: resolvable and resolved.
• Developer reply:

This is fine since we do expect cabins to be correct even for old paths.

• GitLab reports both notes as resolved=true.
• The thread was resolved at 2026-06-18T23:11:55.611Z.

This reply is a rejection of the finding premise. It should be classified as disputed=true; possibly false_positive=true depending on how strict Bubo wants to be with "expected behavior" rebuttals.

3. Bubo state after outcome sync

Stored in finding_outcomes after sync:

{
  "project": ".../commonservice",
  "iid": 93,
  "sha": "1f10edeac316285bc44c0864077765cbf1e644c5",
  "fingerprint": "f79a8d06f47c2bf3e1826f4083ef3c4216701dd9f905dee5d51b0d5ed45b6dee",
  "discussion_id": "09e640179bbd57791767435386eae0d6dd6a3ef6",
  "resolved": 1,
  "deleted": 0,
  "developer_replied": 1,
  "disputed": 0,
  "false_positive": 0,
  "duplicate": 0,
  "resolved_at": null,
  "merged_unresolved": 0,
  "reply_classified": 1,
  "last_checked_at": "2026-06-19T00:48:11+00:00"
}

Project-level dispute stats:

{
  "classes": [
    {
      "category": "correctness",
      "total": 1,
      "rejected": 0,
      "dispute_rate": 0.0,
      "would_suppress": false
    }
  ]
}

Expected Behavior

For a resolved thread with a developer reply like:

This is fine since we do expect cabins to be correct even for old paths.

Bubo should record:

developer_replied=true
resolved=true
disputed=true
reply_classified=true

And the dispute aggregate should include this rejected finding:

correctness: total=1, rejected=1, dispute_rate=1.0

Actual Behavior

Bubo records:

developer_replied=true
resolved=true
disputed=false
false_positive=false
reply_classified=true

This miscounts the finding as accepted/resolved.

Likely Root Causes

There appear to be two related issues.

1. reply_classified is cached forever

In bubo.poller.sync_outcomes, classification is skipped when the DB row already has reply_classified=true:

already_classified = bool(finding.get("reply_classified"))
...
and not already_classified

That is unsafe when new developer replies are added after the first classification.

For this MR, outcome-sync.log shows Bubo classified commonservice!93 on 2026-06-12T02:17:31Z:

{"event": "reply_classified", "false_positive": false, "iid": 93, "project": "...dev/commonservice", "ts": "2026-06-12T02:17:31+00:00", "verdict": "accepted"}

The actual developer rebuttal was created later, on 2026-06-18T23:11:55Z.

Because reply_classified=true, later syncs do not reclassify the new reply text.

2. GitLab discussion classification depends on current bot_username

In bubo.gitlab.classify_discussion_outcome, bot notes are identified by comparing each note's author username to the current configured bot_username:

reply_notes = [
    note
    for note in active_notes
    if ((note.get("author") or {}).get("username") or "") != bot_username
]
bot_notes = [
    note
    for note in active_notes
    if ((note.get("author") or {}).get("username") or "") == bot_username
]

This can misclassify historical bot comments if the posting identity changes or if the stored finding lacks the posted note_id.

In this repro, the original finding note author in GitLab is v-user, while current production should use the llm-reviewer identity. If bot_username no longer matches the historical note author, the bot finding itself can be treated as a developer reply, producing an early wrong verdict and setting reply_classified=true.

Why This Matters

Bubo's ROI and quality metrics depend on outcome sync:

• accepted findings
• disputed findings
• false-positive rate
• cost per accepted actionable finding
• category suppression via dispute rate

This bug biases all of those metrics toward false success:

• rejected findings become accepted findings
• dispute rate stays artificially low
• suppression does not trigger for noisy categories
• resolved-but-rejected threads inflate ROI

Proposed Fix (dev can override this suggestion)

Make reply classification idempotent over the actual reply content, not a one-time boolean.

Possible implementation:

1. Store a stable fingerprint of the developer reply material used for classification:

reply_text_hash
reply_last_note_id
reply_last_note_updated_at
classifier_verdict
classifier_version

2. Reclassify when any of these change:

developer_replied=true
and reply_text_hash != stored_reply_text_hash

3. Identify the bot's original finding by stored note_id when available. If note_id is missing, fall back to the stored discussion_id + finding body fingerprint, not only current bot_username.

4. Treat explicit rebuttal phrases as rejected even without magic markers:

this is fine since ...
expected behavior
working as intended
not a bug
not an issue
false positive
we expect ...

The LLM classifier prompt already says:

"rejected": the developer disagrees — says it is working as intended, not a real problem, or a false positive.

The problem is that the classifier is not rerun against the new reply.

Acceptance Criteria

• Add a unit test where:
  • initial sync sees a bot finding with no real developer rebuttal,
  • later sync sees a new developer reply: This is fine since we do expect cabins to be correct even for old paths.,
  • final outcome is resolved=true, developer_replied=true, disputed=true, reply_classified=true.
• Add a regression test where the current configured bot username differs from the historical note author, but the stored finding/discussion still identifies the original bot note correctly.
• disputed_class_stats() should report the finding's category as rejected after sync.
• Existing resolved accepted replies still remain accepted.

[mountainowl]

Related but separate MR #xx93 learning: Bubo also missed an accepted human review suggestion about throwing on invalid configuration instead of returning/adding a local failure code. Filed as #128 so outcome-sync classification and review-recall quality stay separate.