Skip to content

Review agent should apply elevated severity when findings co-occur with breaking-change indicators #5250

Description

@fullsend-ai-retro

What happened

On PR #4510, the review agent and human reviewer (waynesun09) independently identified 5 of the same issues. In all 5 cases, the bot rated the finding 1–2 severity levels lower than the human:

Finding Bot severity Human severity Gap
OTEL_SDK_DISABLED doc mismatch MEDIUM MEDIUM Same
parentSampledProcessor child-span leak MEDIUM HIGH 1 level
NaN/Inf float drops entire span LOW MEDIUM 1 level
Attribute rename agentfullsend.agent MEDIUM HIGH 1 level
No fsync (crash-safety regression) LOW MEDIUM 1 level
Missing TRACEPARENT adoption tests LOW MEDIUM 1 level

The PR carried multiple breaking-change signals: ! suffix in the title, tech-debt label, ~1,540 lines of deleted test code, 6 deleted source files, and a complete subsystem replacement. Despite these strong signals of high behavioral-contract exposure, the bot applied standard severity calibration.

The human reviewer recognized that in the context of a breaking refactor, issues like "NaN/Inf float edge case" are not LOW-severity curiosities — they represent forensic data loss in the replacement system (dropping entire spans from run-telemetry.jsonl). The severity depends on the stakes of the change, not just the technical likelihood of the edge case.

What could go better

The review agent should incorporate contextual severity modifiers based on PR-level signals. When a PR exhibits breaking-change indicators (title contains !, deletes >500 lines of test code, replaces an entire subsystem, carries breaking-change labels), findings related to behavioral contracts, API surfaces, and data integrity should receive an automatic severity boost.

This is distinct from the deleted-test invariant analysis (proposal 1). That proposal addresses which findings to generate; this proposal addresses how to rate findings the agent already generates. The bot correctly identified the NaN/Inf edge case and the attribute rename — it just under-weighted their impact.

Confidence: MEDIUM-HIGH. The pattern is consistent across all 5 overlapping findings on this PR. However, I have not verified whether this under-rating pattern holds across other PRs — it may be specific to the scale and breaking nature of this particular change.

Risk: Over-correction could lead to severity inflation on routine refactors, increasing review noise. The modifier should be calibrated against the degree of breaking-change exposure, not applied as a blanket boost.

Proposed change

Add contextual severity modifiers to the review agent's finding-rating logic. Specifically:

  1. Detect breaking-change exposure at the PR level before rating individual findings. Signals to check:

    • PR title contains ! (conventional commits breaking-change marker)
    • PR deletes test files or removes >30% of test coverage in touched packages
    • PR deletes source files that are replaced by new files (subsystem replacement pattern)
    • PR carries labels indicating high-impact change (e.g., tech-debt, breaking)
  2. Apply severity modifiers when breaking-change exposure is detected:

    • Findings about behavioral contract changes (attribute renames, API surface changes, format changes): boost by 1 level (LOW→MEDIUM, MEDIUM→HIGH)
    • Findings about data integrity in replacement systems (edge cases that cause data loss, silent failures): boost by 1 level
    • Findings about test-adequacy gaps in replacement code: boost by 1 level (these are more consequential when old tests are deleted)
  3. Annotate the severity boost in the finding description so reviewers understand why a finding was rated higher than usual (e.g., "Severity elevated from LOW to MEDIUM due to breaking-change context: PR deletes the test that previously guarded this invariant").

This is a change to the review skill or review agent's system prompt, likely at internal/scaffold/fullsend-repo/skills/ or the agent definition. The severity modifiers should be documented in the review agent's rating rubric.

Validation criteria

  1. Alignment with human severity: On PRs with breaking-change indicators, the review agent's severity ratings for overlapping findings should match human reviewer ratings within 1 level in >80% of cases (currently 17% exact match, 83% under-rated by 1 level on this PR).
  2. No severity inflation on non-breaking PRs: On routine PRs without breaking-change indicators, severity ratings should remain unchanged from current behavior.
  3. Boost annotation present: When a severity modifier is applied, the finding description should include a brief explanation of why the severity was elevated.
  4. Measurable on historical PRs: Re-run the review agent on PR refactor(telemetry)!: replace bespoke recorder with OTel Go SDK #4510's final commit with the severity modifiers enabled. At least 3 of the 5 overlapping findings should be rated at the same level as the human reviewer's rating.

Generated by retro agent from #4510

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent/reviewReview agentcomponent/skillsfeatureFeature-category issue awaiting human prioritizationpriority/mediumNormal priority, plan for next cycletriagedTriaged but awaiting human prioritizationtype/featureNew capability request

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions