Skip to content

fix(evaluations)!: reject boolean scorer scores - #92

Merged
donei003 merged 1 commit into
mainfrom
fix/scorer-rejects-boolean-scores
Oct 30, 2026
Merged

donei003 merged 1 commit into
mainfrom
fix/scorer-rejects-boolean-scores

Conversation

@donei003

@donei003 donei003 commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #63, now merged. Brings the scorer return contract in line with ai-sdks-monorepo TESTING.md §8.8.4, which was tightened after #63 was written.

Problem

A scorer's contract was "a bool or a finite number in 0–1", with True/False coerced to 1.0/0.0. Two problems with the coercion:

It put two score types on the wire. A threshold then meant different things depending on which type a scorer happened to return, even though LaunchDarkly rules on score-vs-threshold identically for both — so the comparison a caller reasoned about and the one ingest performed could differ.

It hid bugs, and the failure mode was a green run. Every non-empty value is truthy, so a scorer that returned "high", or a stray object, scored 1.0 and passed:

# Before: scored 1.0, row passed, no error anywhere.
Scorer(name="quality", fn=lambda row, output: grade(output))  # grade() returns "high"

A false pass is the one failure mode an evaluation harness must not have — the point of the gate is that a broken check fails loudly.

Change

The contract is now a finite number in 0–1, and nothing else. A scorer answering a yes/no question returns 1.0 or 0.0 itself:

def mentions_policy(row: DatasetRow, output: str | None) -> float:
    return 1.0 if "refund policy" in (output or "").lower() else 0.0

A bool, a non-finite number, or one out of range is an invalid_score result — a per-criterion ERROR event, never a raise, since the row's generation has already been paid for.

numeric_score() already excluded bool (Python's bool is an int subclass, which is exactly the trap), so the fix deletes the coercion branch rather than adding a check — the two error paths collapse into one whose message names the offending value.

ScorerFn drops bool from its signature. This documents the contract, but type checkers still accept a bool here because bool is a subclass of int. The runtime check in numeric_score() is the guard.

The first invalid_score from each scorer in a run also logs a warning that names the scorer and the returned value. A caller who upgrades with bool scorers sees the cause in their own logs.

⚠️ Breaking

A scorer returning a bool now produces an invalid_score result instead of 1.0/0.0. Callers return the number directly. This is deliberate per §8.8.4 ("there is no boolean shortcut and nothing is coerced on the caller's behalf") — but it is a behavior change and worth a release note.

Every scorer in the examples repo is converted to match in launchdarkly-labs/ai-sdk-evaluations-example#4.

Validation

  • uv run pytest -q — 1264 passed, 11 skipped
  • uv run mypy .../evaluations — clean; ruff check / ruff format --check — clean
  • New parametrized coverage: True, False, NaN, Infinity, 3, -1, "high", None all produce invalid_score with the value named and no score field, and the run still completes, flushes, and polls
  • Boundary coverage: 0, 1, 0.0, 1.0, 0.25 stay valid, ints included — only bool is excluded
  • The suite's own scorers were converted from bool to numeric, which is the change callers make

🤖 Generated with Claude Code


Note

Overview
Breaking: offline evaluation Scorer functions must return a finite number in 0–1; True/False are no longer coerced to 1.0/0.0 and instead produce a per-criterion invalid_score ERROR (the run still completes).

Validation goes through numeric_score() only, so bools, strings, NaN/Infinity, and out-of-range values share one error path with an updated message. ScorerFn is typed as float | Awaitable[float], and README/Scorer docs tell callers to return 1.0/0.0 for yes/no checks.

The runner logs a one-time WARNING per scorer name per run when an invalid score is seen. Tests and examples were updated, with new parametrized coverage for invalid and boundary returns.

Reviewed by Cursor Bugbot for commit ff5da45. Bugbot is set up for automated code reviews on this repo. Configure here.

@donei003
donei003 marked this pull request as ready for review September 21, 2026 22:55

@devin-ai-integration devin-ai-integration Bot 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.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@donei003
donei003 force-pushed the fix/scorer-rejects-boolean-scores branch from db9bfbc to 0bc9544 Compare September 30, 2026 19:08

@jeffdupont jeffdupont 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.

Reviewed with the 1.0 freeze in mind. Nothing blocking from me. This matches ai-sdks-monorepo TESTING.md §8.8.4 as tightened in 1402609 ("there is no boolean shortcut and nothing is coerced on the caller's behalf"), and the invalid_score row in the §8.8.4 failure table. Tested on 0bc9544: make test exit 0 (1410 passed, 11 skipped), make typecheck exit 0. That head is 2 commits behind main, and both are release commits.

On the contract question, reject is the right choice for 1.0. I went through the three options:

  • Coerce is the bug this PR fixes. return "high" scored a pass.
  • Drop doesn't work. The backend needs one result per (row, criterion) to finish the run, so a dropped result stalls it until the §8.9 timeout.
  • Raise throws away a generation that has already been paid for.

A per-criterion ERROR is what §8.8.3 already does for judge scores, so judges and scorers now behave the same way. It is also the right direction to freeze. If we want a boolean shortcut later, loosening the contract is additive. Tightening it after 1.0 would be a breaking change.

JS parity: there's nothing to port. JS has no evaluations on main (deferred to 1.1 in the GA plan), and its judge path already rejects booleans, because isFiniteScore checks typeof score === 'number' (packages/client/src/judges.ts:68). When evals land in JS, §8.8.4 covers it.

Smaller notes:

  • The PR body says dropping bool from ScorerFn means "a caller who annotates their scorer sees this at type-check time". That isn't true. I ran mypy --strict against this branch: a def f(...) -> bool scorer and lambda row, output: True both pass, and only a -> str scorer is flagged. bool is a subclass of int, and type checkers accept int where float is expected. The runtime check is the only guard. Narrowing the type is still worth doing, but the claim should come out of the PR body and the squash message. Details inline.
  • Please mark it as breaking when you squash: fix(evaluations)!: or a BREAKING CHANGE: footer. The coercion shipped in launchdarkly-ai-server 0.2.4. As titled, release-please will list this under Bug Fixes as a patch, and a caller with bool scorers will see every row go ERROR with no note telling them why.
  • Optional: on that upgrade path the only signal is passed=False and error_rows in the summary. _criterion_error_result (runner.py:818) doesn't log anything locally. A logger.warning once per scorer on invalid_score that names the returned value would make the bool case obvious from the caller's own output.

@donei003 donei003 changed the title fix(evaluations): reject boolean scorer scores fix(evaluations)!: reject boolean scorer scores Oct 5, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

@jeffdupont

Copy link
Copy Markdown
Contributor

@donei003 this is approved but conflicts with main, so it can't merge yet. The two conflicts are packages/client/tests/test_evaluations_run.py (this PR's tests still use the old run(project_key=) call) and packages/ai/README.md. A rebase onto main should be all it needs; I'll re-approve after.

@jeffdupont

Copy link
Copy Markdown
Contributor

Pushed bc596c0: merged origin/main, keeping both sides of the test_evaluations_run.py conflict. The new scorer tests now use tagged(handler) and a mode, which main's handler routing requires.

Not done: the invalid_score message and warning still use repr(). The JSON _describe_value helper lands with #209, which is not on main yet. Switch them once #209 merges.

The PR body makes no claim that mypy catches a bool scorer (it says the opposite), so it is unchanged. Needs a human reviewer.

@jeffdupont
jeffdupont requested a review from aknight-ld October 23, 2026 21:49
@jeffdupont

Copy link
Copy Markdown
Contributor

@aknight-ld, could you re-approve? There are new commits since your approval.

  • 52f69be: merged main in (it was conflicting after feat(evaluations): normalize text behaviors #209). The conflict was in the scorer validation in evaluations/runner.py. I kept this PR's rule that a bool is an invalid score, against main's 1.0/0.0 coercion. The invalid_score message and the one-time warning now name the value with feat(evaluations): normalize text behaviors #209's _describe_value (true, null, "high", NaN) instead of repr(), and test_scorer_must_return_a_finite_number_in_range asserts that form.
  • bc596c0 (earlier): merged main and added tagged(handler) and a mode to the new tests, as main requires.

Locally the full suite passes (3379 passed), along with lint, format, typecheck and the API report check. @donei003, FYI since it's your branch: I merged main in rather than rebasing, so nothing was force-pushed.

@jeffdupont jeffdupont 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.

Re-approving after the main merge in 52f69be: bools still reject, and the invalid_score message now uses _describe_value.

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 1 new potential issue.

Devin Review

if score is None or score < 0 or score > 1:
message = (
"scorer fn must return a finite number between 0 and 1, "
f"got {_describe_value(score_value)}"

@devin-ai-integration devin-ai-integration Bot Oct 23, 2026 •

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.

🔴 Invalid scorer values abort evaluation runs

When a scorer returns a mapping with tuple keys, _describe_value raises instead of recording invalid_score. The exception escapes _run_scorer_for_result, aborting the run after generation.

Learn more

The scorer's return value is supplied by customer code. An invalid return must become a per-criterion ERROR, leaving the run free to emit events for all rows. Here numeric_score rejects a mapping, but _describe_value calls canonical_json, whose json.dumps step rejects mappings with tuple keys. The exception occurs after the scorer invocation's exception handler and propagates through _run_criteria_for_results, preventing criterion event emission.

Example: A scorer returns {('precision', 'recall'): 0.8} instead of a score. The expected invalid_score event never appears; run() raises TypeError after generating the rows.

Recommended fix: Make _describe_value safely fall back to repr(value) when JSON serialization fails, while retaining canonical formatting for ordinary JSON-like values. Test a scorer returning a mapping with tuple keys and assert it produces an invalid_score event and completes the run.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

donei003 added a commit that referenced this pull request Oct 29, 2026
Promotes Evaluations from Code from
`launchdarkly_ai_server.experimental.evaluations` to the package root.
This follows the Agent Skills promotion in #210.

> **Draft.** This PR releases together with launchdarkly/js-ai-sdk#181.
A Jira ticket is not linked yet.

## Migration

Import Evaluations from Code names from `launchdarkly_ai_server` instead
of `launchdarkly_ai_server.experimental.evaluations`. The names,
signatures and behavior do not change.

```python
# before
from launchdarkly_ai_server.experimental.evaluations import EvalTool, init_evaluations
# after
from launchdarkly_ai_server import EvalTool, init_evaluations
```

This is a breaking change (`!`). It bumps the minor version while the
package is 0.x.

## What changed

- **Root exports.** The 14 names that `experimental/evaluations.py`
exported are now in `launchdarkly_ai_server.__all__`, with the same
spelling. No name collides with a current root export.
`launchdarkly-ai-python` gets the names through its existing `import *`.
- **Experimental module removed.** `experimental/evaluations.py` is
deleted. `experimental/__init__.py` stays as a docstring only, so a
future experimental feature has a place to go.
- **Usage ID.** `init_evaluations` now reports `client.initEvaluations`,
not `experimental.initEvaluations`. Queries that count the old ID must
also count the new one.
- **API reports.** Regenerated with `make api-report`. The names are in
`launchdarkly_ai_server.txt`, and
`launchdarkly_ai_server.experimental.evaluations.txt` is deleted.
- **Changelog.** The three `evaluations` rows are removed from
`changelog-sections`, so `feat(evaluations):` lands under Features. The
`AGENTS.md` scope examples now use a placeholder scope.
- **Docs.** The client README, the `launchdarkly-ai-python` README,
`packages/client/agents.md` and `AGENTS.md` no longer call Evaluations
from Code experimental. The README heading anchor changed from
`#run-an-evaluation-from-code-experimental` to
`#run-an-evaluation-from-code`.

## Tests

- `TestPackageExports` checks that every evaluations name is on the root
and that `experimental.evaluations` no longer imports.
- `test_public_surface.py` adds the 14 names to the root snapshot.
- `test_reexports.py` checks that the barrel re-exports the names.
- `test_core_does_not_import_experimental_code` now checks only
`launchdarkly_ai_server.experimental`, because the root loads
`evaluations`. A new test checks that `lifecycle` imports no evaluations
module.
- `test_sdk_usage.py` expects `client.initEvaluations`.

## Import time

`import launchdarkly_ai_server` now loads the `evaluations` package.
Measured with `python -X importtime`, it adds about 33 ms to a root
import of about 130 ms. The largest part is `evaluations.api`.

## Checks

- [x] `make lint`, `make format-check`, `make typecheck`, `make
api-report-check`: pass
- [x] `python -m scripts.api_breaking pr --base origin/main`: no
breaking changes to the public API
- [x] `make test`: 3640 passed, 18 skipped
- [ ] Run one evaluation end to end against staging with the root import

## Conflicts

The open evaluations PRs #228, #227, #143, #135 and #92 edit the same
test files. Merge them first, or rebase them after this PR.

## Related PRs

Releases together with:
- launchdarkly/js-ai-sdk#181

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> **Evaluations from Code** moves from
`launchdarkly_ai_server.experimental.evaluations` to the
**`launchdarkly_ai_server` package root** (and through
**`launchdarkly-ai-python`** re-exports). The experimental shim is
**removed**; callers should import `init_evaluations`, `EvalTool`,
`Judge`, `Scorer`, and the rest from the root instead.
> 
> The **public API contract** shifts accordingly: generated
**`launchdarkly_ai_server.txt`** now lists the evaluations surface, and
**`launchdarkly_ai_server.experimental.evaluations.txt`** is deleted.
**`init_evaluations`** usage telemetry changes from
**`experimental.initEvaluations`** to **`client.initEvaluations`**.
> 
> **Release/changelog policy** is updated: **`evaluations` is no longer
an Experimental scope** in `release-please-config.json`, and
**`AGENTS.md`** / READMEs describe Evaluations (and Skills) as core,
with docs and examples using root imports and a non-experimental README
anchor.
> 
> Tests and snapshots (**`test_public_surface`**,
**`TestPackageExports`**, **`test_reexports`**, lifecycle import guards)
enforce the new export layout and that **`experimental.evaluations`**
cannot be imported.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
3ee3612. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
A scorer must return a finite number from 0 to 1. The runner no longer
changes True or False to 1.0 or 0.0. A bool, a non-finite number, or a
number out of range gives an invalid_score criterion ERROR. The run
continues.

The first invalid score from each scorer in a run logs one warning. The
warning names the scorer and the value.

ScorerFn no longer includes bool in its return type.

BREAKING CHANGE: a scorer that returns a bool now gets an invalid_score
result. Return 1.0 or 0.0 instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@donei003
donei003 force-pushed the fix/scorer-rejects-boolean-scores branch from 52f69be to ff5da45 Compare October 29, 2026 23:48
@donei003
donei003 merged commit 678a605 into main Oct 30, 2026
11 checks passed
@donei003
donei003 deleted the fix/scorer-rejects-boolean-scores branch October 30, 2026 02:51
jeffdupont pushed a commit to launchdarkly/js-ai-sdk that referenced this pull request Oct 30, 2026
)

## Summary

This change matches launchdarkly/python-ai-sdk#92.

The first time a scorer returns an invalid score in a run, the SDK logs
one `console.warn`. The warning names the scorer and the value:

```
[LaunchDarkly] Scorer 'quality' returned an invalid score: scorer fn must return a finite number, got "high"
```

An invalid score is a bool, a non-finite number, a number out of range,
or a value that is not a number.

The criterion result does not change. It is still an `invalid_score`
ERROR event, and the run continues. The SDK already rejects a boolean
score, so this PR changes only the warning.

## Why

An invalid score is recorded as a per-criterion ERROR, so the problem is
easy to miss. A caller who upgrades with scorers that still return
booleans sees only ERROR results in LaunchDarkly. The warning shows the
cause in the caller's own logs. The SDK logs once for each scorer in
each run, so a large dataset does not flood the log.

## Tests

- Two invalid scorers over three rows log exactly two warnings, one for
each scorer.
- A second run on the same module logs the warning again.
- A valid score logs no warning.
- `vitest run` in `packages/client`: 2082 passed, 11 skipped. `yarn
typecheck` and `biome check .` pass.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- devin-review-badge-begin -->

---

<a href="https://app.devin.ai/review/launchdarkly/js-ai-sdk/pull/199"
target="_blank"><picture><source media="(prefers-color-scheme: dark)"
srcset="https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img
src="https://static.devin.ai/assets/gh-devin-review-light.svg?v=4"
alt="Devin Review"></picture></a>
<!-- devin-review-badge-end -->

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> When a custom scorer returns a non-numeric, out-of-range, or otherwise
invalid value, the evaluation phase now emits a **`console.warn`** the
first time each scorer does so **within a run**, naming the scorer and
the validation message (aligned with the Python SDK).
> 
> **Criterion behavior is unchanged:** each row still gets an
`invalid_score` ERROR event and the run continues. A per-run `Set`
dedupes warnings across rows so large datasets do not spam logs; a new
run can warn again.
> 
> Tests cover one warning per bad scorer across multiple rows, repeat
warnings on subsequent runs, and silence for valid scores.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
b716232. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.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.

3 participants