Reviewer missed accepted human feedback about throwing on invalid config

[mountainowl]

Summary

Bubo missed a high-signal human review suggestion on long.../commonservice!93.

The human reviewer flagged that invalid min-base-fare-by-cabin configuration should throw/fail through the caller path instead of returning a boolean / adding request notes locally. Bubo did not produce any equivalent suggestion or question; it only posted a different blocking finding about cabin parsing when the feature flag is disabled.

This is a review-quality / recall bug, separate from outcome-sync issue #127.

Observed Bubo Output

Bubo posted exactly one finding for the earlier successful review of commonservice!93:

{
  "project": "long.../commonservice",
  "iid": 93,
  "sha": "1f10edeac316285bc44c0864077765cbf1e644c5",
  "file": "src/main/java/com/long.../multisource/sentinel/checks/FareCheck.java",
  "line": 68,
  "status": "posted",
  "type": "issue",
  "severity": "blocking",
  "category": "correctness",
  "confidence": 0.91,
  "discussion_id": "09e640179bbd57791767435386eae0d6dd6a3ef6"
}

Finding title:

Disabled cabin thresholds still require parsing the cabin

Bubo did not flag the separate design concern below.

Human Review Signal Bubo Missed

Human reviewer comment on src/main/java/com/long.../multisource/sentinel/model/MinBaseFareConfiguration.java:

Instead of returning a boolean, I suggest renaming to `requireValidMinBaseFareByCabinConfiguration` and throw an exception if the parsing goes wrong. The good thing about this is that this requestNotes thing that you are doing will be done in the try catch from the caller class, plus it will add an error log in ELK and portainer, plus it will mark the request as FAILED, for free

The author accepted the feedback:

True. I think I was trying to mark it unsuccessful so they would know it's a data issue, but I think it's fine to mark it as a failure as well.

The thread was resolved after the author pushed follow-up commits.

Why This Should Have Been A Bubo Finding

The diff introduced a new configuration parser:

public static SortedMap<Cabin, Double> parseMinBaseFareByCabin(@NotNull Map<String, String> configMap) {
    ...
    if (StringUtils.isBlank(rawMinBaseFareByCabin)) {
        throw new IllegalArgumentException("Missing MIN_BASE_FARE_BY_CABIN job configuration");
    }
    ...
}

And changed FareCheck.validateBaseFare to catch parsing errors locally:

try {
    Cabin cabin = Cabin.fromConfigurationValue(filingOutput.getCabin());

    minBaseFare = (minBaseFareByCabinEnabled) ?
            MinBaseFareConfiguration.parseMinBaseFareByCabin(configMap).getOrDefault(cabin, null) :
            Double.valueOf(configMap.get(JobConfigurationKeys.MIN_BASE_FARE.name()));
} catch (IllegalArgumentException e) {
    failureCodes.add(ERROR_GETTING_MIN_BASE_FARE);
    return;
}

The human reviewer identified the system-behavior issue: invalid job configuration is not just a fare validation failure. It should fail through the caller/job failure path so the request is marked failed and operational logs are emitted.

This is exactly the kind of item Bubo should surface as a suggestion or question, because it is:

• introduced by the diff,
• visible from the changed code,
• tied to failure behavior and operations,
• accepted by the author,
• not a style nit.

Expected Bubo Finding Shape

Example acceptable output:

{
  "type": "question",
  "severity": "non-blocking",
  "category": "failure",
  "title": "invalid config is handled as fare failure",
  "file": "src/main/java/com/long.../multisource/sentinel/checks/FareCheck.java",
  "line": 66,
  "impact": "Bad MIN_BASE_FARE_BY_CABIN config is recorded as ERROR_GETTING_MIN_BASE_FARE instead of failing the request through the caller path.",
  "evidence": "validateBaseFare catches IllegalArgumentException from cabin/config parsing, adds a failure code, and returns. The parser errors are configuration errors, not fare data failures.",
  "fix": "Should invalid min-base-fare-by-cabin configuration throw to the caller so the request is marked failed and logged operationally?",
  "confidence": 0.9
}

Or as a suggestion:

{
  "type": "suggestion",
  "severity": "non-blocking",
  "category": "failure",
  "title": "throw on invalid config",
  "file": "src/main/java/com/long.../multisource/sentinel/checks/FareCheck.java",
  "line": 66,
  "impact": "Configuration errors can be hidden inside fare-check failure codes instead of using the existing job failure path and operational logging.",
  "evidence": "The new code catches IllegalArgumentException from MinBaseFareConfiguration.parseMinBaseFareByCabin and returns after adding ERROR_GETTING_MIN_BASE_FARE.",
  "fix": "Let configuration parse errors throw to the caller, or split data validation failures from configuration failures explicitly.",
  "confidence": 0.9
}

Related Human Comments Also Missed Or Worth Comparing

The same MR had other human comments that can be useful as evaluation labels:

1. Remove an accidental/leftover line.
2. Prefer SortedMap for the variable declaration instead of TreeMap.
3. Add a test that protects the enum/order assumption so a future random change does not break the ordering logic.

Bubo should not necessarily report every nit. But comment 3 is also a legitimate review-quality signal because the new TreeMap<Cabin, Double> ordering depends on Cabin enum order for validation. A concise test-gap suggestion would have been reasonable.

Impact

This bug means Bubo can miss actionable failure-behavior concerns that senior human reviewers catch and authors accept. It weakens:

• review recall,
• cost-per-accepted-finding metrics,
• trust in "no additional findings" when Bubo posts only one issue,
• training/eval loops that compare Bubo to human review outcomes.

Suggested Fix

Add a review-quality evaluation case for this MR shape:

• new configuration parser,
• caller catches parse/config exceptions locally,
• caller converts them into validation/failure-code return,
• surrounding system has a higher-level failure path that should handle operational failure.

The reviewer should ask or suggest whether invalid configuration should throw to the caller rather than be modeled as domain validation failure.

Implementation ideas:

• Add "configuration error vs domain validation error" to failure-behavior review heuristics.
• In the prompt or verifier, emphasize that config/schema/contract parse failures should be checked against caller-level failure handling.
• Add a labelled sample from this MR to the regression/evaluation set so future model/prompt changes preserve this recall.

Acceptance Criteria

• A regression/eval test for this diff expects at least one finding or question about throw-vs-return / configuration errors being handled as fare validation failures.
• The finding must be non-style and tied to failure behavior or operational correctness.
• The test should not require hardcoding repository names, class names, or MR IDs.
• Bubo should still avoid reporting low-value style-only comments such as generic interface preference unless explicitly configured for style review.