Skip to content

fix(exceptions): preserve falsy-but-valid values in dataset.py and cli.py - #279

Merged
himanshu231204 merged 2 commits into
OpenAgentHQ:mainfrom
PrinceThummar011:fix/preserve-falsy-exception-details
Aug 13, 2026
Merged

fix(exceptions): preserve falsy-but-valid values in dataset.py and cli.py#279
himanshu231204 merged 2 commits into
OpenAgentHQ:mainfrom
PrinceThummar011:fix/preserve-falsy-exception-details

Conversation

@PrinceThummar011

Copy link
Copy Markdown
Contributor

Fixes #68

Summary of Changes

  • openagent_eval/exceptions/metric.py (around line 119):
    • Status: Already fixed on main in commit 59e5e65 (if timeout_seconds is not None:).
  • openagent_eval/exceptions/dataset.py (line 84):
    • Status: Fixed. Replaced if line_number: with if line_number is not None:. Preserves line_number=0.
  • openagent_eval/exceptions/cli.py (lines 90-93):
    • Status: Fixed. Replaced if field: and if value: with if field is not None: and if value is not None:. Preserves field="" and value="".

Contributor Claim Verification

  • Nithin00614's claim that the bug "already appears fixed on main" was only partially true. While metric.py had been updated in an earlier commit, dataset.py and cli.py still contained the exact same truthiness bug.

Regression Tests Added

  • test_invalid_dataset_error_preserves_zero_line_number (line_number=0)
  • test_invalid_dataset_error_omits_none_line_number (line_number=None)
  • test_timeout_error_preserves_zero_timeout (timeout_seconds=0.0)
  • test_timeout_error_omits_none_timeout (timeout_seconds=None)
  • test_validation_error_preserves_empty_string_field_and_value (field="", value="")
  • test_validation_error_omits_none_field_and_value (field=None, value=None)

…i.py

Extends the fix already applied to metric.py (59e5e65) to
InvalidDatasetError (line_number=0) and ValidationError
(field='', value=''), which had the same 'if x:' truthiness
bug instead of 'if x is not None:'.

Adds regression tests covering both the falsy-valid case and
the genuinely-absent (None) case for all three exception types.

Fixes OpenAgentHQ#68

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a common truthiness pitfall in exception constructors so that falsy-but-valid values (e.g., 0 and "") are preserved in exception details and therefore remain visible in str(error) output/logging.

Changes:

  • Update InvalidDatasetError to include line_number=0 in details by using is not None.
  • Update ValidationError to include empty-string field / value in details by using is not None.
  • Add targeted regression tests covering 0 vs None and "" vs None behavior for these exceptions (and for MetricTimeoutError as well).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/unit/test_exceptions.py Adds regression tests ensuring falsy-but-valid values are preserved in exception details and string rendering.
openagent_eval/exceptions/dataset.py Fixes line_number handling so 0 is retained (only None is omitted).
openagent_eval/exceptions/cli.py Fixes field/value handling so empty strings are retained (only None is omitted).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@himanshu231204

Copy link
Copy Markdown
Member

/oc review pr

@opencode-agent

Copy link
Copy Markdown
Contributor

PR Review: #279 — fix(exceptions): preserve falsy-but-valid values

Verdict: Approve. Small, correct, well-tested fix.

Summary

Replaces truthiness checks (if x:) with explicit is not None checks so falsy-but-valid values survive into error.details and str(error), while None is still omitted.

What I verified

  • dataset.py:84if line_number is not None: correctly keeps line_number=0 and drops None. ✓
  • cli.py:90-93field=""/value="" are now kept; None omitted. ✓
  • metric.py was indeed already fixed on main (if timeout_seconds is not None: at metric.py:119); PR correctly treats it as such rather than re-patching it.
  • Manually confirmed every new test assertion passes (both preserve and omit cases, plus str(error) rendering like 'V (field=, value=)').

Test quality

Good coverage — each fix gets a "preserve falsy" test and an "omit None" test, and the timeout tests also lock in the pre-existing fix's str() behavior. The "field" not in str(error) assertions are safe because str(error) is just the message when details are empty.

Notes (non-blocking)

  • Scope is appropriately limited. dataset_path (dataset.py:31), data_format (dataset.py:82), and command (cli.py:31) still use truthiness checks, but empty values there aren't genuinely valid, so leaving them is reasonable.
  • ruff check on the 3 changed files is clean. The one I001 import-sort finding in openagent_eval/exceptions/__init__.py is pre-existing on main and unrelated to this PR.

Nothing blocking merge.

New%20session%20-%202026-08-13T19%3A04%3A07.462Z
opencode session  |  github run

@himanshu231204
himanshu231204 merged commit 0e5f581 into OpenAgentHQ:main Aug 13, 2026
8 checks passed
@github-actions

Copy link
Copy Markdown

🎉 Congratulations @PrinceThummar011!

Your pull request has been successfully merged into main. 🚀

Thank you for contributing to OpenAgentHQ and helping improve the project.

We truly appreciate your contribution and hope to see you back with more amazing PRs!

Happy Open Sourcing! ❤️

himanshu231204 pushed a commit that referenced this pull request Aug 16, 2026
…dropping them (#280)

Detail-recording guards used truthiness tests, so an explicitly passed
"", 0, False or [] was silently dropped from .details. Only None means
"not supplied". Switch all 21 guards in the exceptions package to
`is not None`, matching the fix that landed for #68 in PR #279.

Message-text construction guards and __str__ formatting are unchanged.

Co-Authored-By: Kimi K3 <noreply@kimi.com>
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.

exceptions(metric): MetricTimeoutError drops timeout_seconds=0.0 due to falsy check

3 participants