Skip to content

Fix/7220 preserve eval case order - #7221

Open
boopathi-376 wants to merge 4 commits into
google:mainfrom
boopathi-376:fix/7220-preserve-eval-case-order
Open

boopathi-376 wants to merge 4 commits into
google:mainfrom
boopathi-376:fix/7220-preserve-eval-case-order

Conversation

@boopathi-376

Copy link
Copy Markdown

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

Problem:

Updating an existing eval case moves it to the end of EvalSet.eval_cases in LocalEvalSetsManager.

Solution:

Replace the existing eval case in place instead of removing and appending it, preserving its original position.

Added a regression test to verify that updating a case does not change the order of eval cases.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

pytest tests/unittests/evaluation/874 passed

Manual End-to-End (E2E) Tests:

Not applicable. The change is covered by unit tests.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective.
  • New and existing unit tests pass locally.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

Small, focused bug fix. The change is limited to the eval-set update helper and its regression test.

…ble return

cast(bool, ...) is a static-only type hint and has no effect at runtime, so a require_confirmation callable that returns None was silently treated as falsy, letting the tool run without confirmation. Replace the cast with an isinstance(result, bool) runtime check that fails closed on any non-bool return. Fixes google#7010
- Detect un-awaited awaitables returned by require_confirmation predicates and treat them as requiring confirmation, with a logger.warning explaining why. - Log a warning whenever a non-bool return is coerced to True, so a silent behavior change (e.g. numpy.bool_ failing isinstance(bool)) is diagnosable instead of invisible. - Add regression tests asserting 0 and empty string (falsy non-bool) also fail closed, making the full scope of the behavior change explicit. Addresses feedback from mahirhir and tonydzi on PR 7012.
Update require_confirmation docstrings in FunctionTool, McpTool, and McpToolset to state that a non-bool return (including None) is treated as requiring confirmation, matching the runtime behavior fixed in the prior commits.
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.

[Bug]: Updating an eval case moves it to the end in Local/GCS eval sets

2 participants