Skip to content

demo(13-throw-bare-exception): 13 — New throw new Exception(...) path with no test coverage#162

Open
EricCogen wants to merge 1 commit intomainfrom
demo/13-throw-bare-exception
Open

demo(13-throw-bare-exception): 13 — New throw new Exception(...) path with no test coverage#162
EricCogen wants to merge 1 commit intomainfrom
demo/13-throw-bare-exception

Conversation

@EricCogen
Copy link
Copy Markdown
Owner

13 — New throw new Exception(...) path with no test coverage

Expected verdict: ❌ Fails — GauntletCI should fire GCI0032 (uncaught exception path).

What changed

PricingService gains a new validation helper that throws a bare
System.Exception when an amount is negative:

public Money RequireNonNegative(Money amount)
{
    if (amount.Amount < 0m)
    {
        throw new Exception($"Amount must be non-negative: {amount.Amount}");
    }
    return amount;
}

The diff adds a throw new in production code without adding any
corresponding Assert.Throws<> / Should().Throw<>() test in the
test project.

Why this is risky

  • throw new Exception(...) is the wrong base type. Callers cannot
    catch something more specific without also catching every other
    bug. The .NET guidelines explicitly call this out.
  • A new exception path with no test is, by definition, a path no one
    has ever executed. The first time it fires will be in production.
  • The trivial fix is to (a) throw ArgumentOutOfRangeException
    (which has dedicated handling everywhere in .NET) and (b) add an
    Assert.Throws<ArgumentOutOfRangeException>(...) test.

What GauntletCI catches

GCI0032 Uncaught Exception Path — one or more throw new statements
in non-test files in the diff, with no corresponding throw-assertion
(Assert.Throws, Should().Throw, ThrowsAsync, Throws<…>) in any
test file in the same diff.

…th with no test coverage

See scenarios/13-throw-bare-exception/README.md for the expected verdict.
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GauntletCI found the following issues:

These findings reference lines outside the PR diff, so they appear here instead of inline. Expand each entry for full evidence, rationale, and suggested action.

GCI0032 — Uncaught Exception Path: 1 'throw new' statement(s) added without Assert.Throws or Should().Throw evidence in this diff.

GCI0032 — Uncaught Exception Path

1 'throw new' statement(s) added without Assert.Throws or Should().Throw evidence in this diff.

Evidence:

1 added 'throw new' statement(s) in non-test files.

⚠️ Why it matters: New exception paths that are untested may crash callers silently in production when the edge case is reached.

💡 Suggested action: Add xUnit Assert.Throws<T> or FluentAssertions .Should().Throw<T>() tests for each new exception path.

Confidence: Medium | Severity: Block

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c139c98236

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

{
if (amount.Amount < 0m)
{
throw new Exception($"Amount must be non-negative: {amount.Amount}");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use typed exception for negative amount validation

Throwing System.Exception for a predictable input-validation failure makes this path indistinguishable from unexpected runtime faults, so callers and middleware that handle ArgumentException/ArgumentOutOfRangeException as client errors will miss it and may treat negative amounts as generic server failures. Use a specific exception type (for example ArgumentOutOfRangeException) so downstream handling remains correct.

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant