benchmarks: realworld_llm_benchmark exits 0 when the model is missing (last site of the issue #205 class)

[jaylfc]

CARD TITLE (intent, not commit subject): benchmarks: realworld_llm_benchmark exits 0 when the model is missing (last site of the issue #205 class)

Autonomous build of board card tsk-xykcf2.

• benchmarks/realworld_llm_benchmark.py:300: change bare return to sys.exit(1)
• tests/test_realworld_llm_benchmark_exit.py: add exit-code non-zero test
• changelog.d/tsk-xykcf2-realworld-llm-benchmark.md: record the fix

Files:
benchmarks/realworld_llm_benchmark.py | 2 +-
changelog.d/tsk-xykcf2-realworld-llm-benchmark.md | 3 +++
tests/test_realworld_llm_benchmark_exit.py | 19 +++++++++++++++++++
3 files changed, 23 insertions(+), 1 deletion(-)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jaylfc]

Review: the production fix is right, the test and the scope claim are not

The one-line change is correct and I want it on master. sys is already imported at line 22, so sys.exit(1) raises cleanly, and I confirmed the fix is genuinely covered:

Red witness, clean. Reverting sys.exit(1) back to return makes the new test fail; restoring it makes it pass. I ran ast.parse on the edited file first so a red could not be a syntax error wearing the right costume.

Two things stop me merging it as it stands.

---

1. The test only passes because Ollama is absent from CI

test_exit_code_non_zero_when_model_missing calls run_benchmark(limit=1, top_k=1) against the real check_ollama(). check_ollama swallows every exception and returns (False, []), so on a box with no Ollama the model-missing branch runs and the test passes. That is the CI condition, and it is the only condition it passes under.

Measured, with the positive control in the same run so this is not an ambiguous probe:

test_shipped_assertion_when_model_present  FAILED
test_CONTROL_model_absent                  PASSED

I monkeypatched check_ollama to report the model available, then ran the shipped test's body verbatim. It fails, because run_benchmark proceeds past the branch under test:

Ollama OK — qwen3.5:9b available
>       with open(DATA_PATH) as f:
E       FileNotFoundError: .../benchmarks/data/longmemeval_s_full.json

So on any machine that actually serves qwen3.5:9b, this test fails. On the bench host, where both the model and that dataset are present, it would not even fail fast: it would run a real one-question LLM benchmark, with model loading and network calls, inside the unit suite.

Worth saying plainly, because it is the same trap the PR is fixing: this change exists because a benchmark reported success when its environment was wrong, and its test currently reports success for the same reason. The environment has to be pinned, not inherited.

The fix is three lines. Pin the dependency instead of hoping for it:

async def test_exit_code_non_zero_when_model_missing(monkeypatch):
    async def fake_check_ollama():
        return False, []
    monkeypatch.setattr(rw, "check_ollama", fake_check_ollama)
    with pytest.raises(SystemExit) as exc:
        await rw.run_benchmark(limit=1, top_k=1)
    assert exc.value.code != 0

That still red-witnesses correctly (I checked) and it passes everywhere rather than only where Ollama is missing.

2. "last site of the issue #205 class" is not accurate

benchmarks/eventqa_runner.py still has two of them, both inside async def run():

line 723: bare return after an ERROR print  (graph_expansion unsupported)
line 728: bare return after an ERROR print  (no rows loaded for tier)

and the caller has no exit-code plumbing at all:

line 863: asyncio.run(run(args))     # no sys.exit, no return value consumed
main(): contains no sys.exit

Both paths print an error and exit 0, which is exactly the #205 shape. PR #290 touched that file with +2/-2, so it fixed one site and left these two.

My scan found the site this PR fixes when run against origin/master and stops finding it on this branch, so the scan works; these two are real, not an artifact.

Please do not close issue #205 on this PR.

3. Nits

Both new files are missing a trailing newline (changelog.d/tsk-xykcf2-realworld-llm-benchmark.md and the test).

---

I am closing this in favour of a revision card rather than leaving it open, for the same reason as #298: a blocked PR has no revision path here, and closing frees the throttle slot and clears the one-PR-per-task guard. The revision card branches from exec/tsk-xykcf2, so the one-line production fix and the red witness behind it are preserved exactly. It also picks up the two eventqa_runner.py sites, which need doing regardless and belong with this work rather than in a separate pass.

For the record on process: Kilo Code Review has been in_progress on this PR since 14:50:50Z, now over three hours against a same-day completed band of 8 to 10 minutes. It is not a required check on this repo (required_status_checks is absent from branch protection and rulesets is empty), so it is advisory and I reviewed without it rather than waiting on a run that is not coming back.

[jaylfc]

Closing in favour of the revision card tsk-2umdob, which carries the two blockers from the review above.

The production fix on this branch is correct and I am not asking for it back. sys is imported at line 22, sys.exit(1) is the right call, and the red witness is clean. tsk-2umdob branches from exec/tsk-xykcf2 so that one-liner survives untouched, and the branch is deliberately NOT deleted. Please leave it in place.

What the revision adds is pinning the test so it stops depending on Ollama being absent, and fixing the two eventqa_runner.py sites that make the "last site" claim inaccurate. Issue #205 should stay open until those land.