fix(hotel_receptionist): don't pass simulation runs the grading never checked - #6797
Open
u9g wants to merge 3 commits into
Open
fix(hotel_receptionist): don't pass simulation runs the grading never checked#6797u9g wants to merge 3 commits into
u9g wants to merge 3 commits into
Conversation
on_session_end runs at job-process shutdown, after on_simulation_end has already recorded the simulator's verdict, and Tagger.success()/fail() are last-write-wins on a single outcome slot. Its heuristic ladder therefore overwrote the real verdict: a conversation-only scenario that correctly ended without a tool call or state change was tagged fail for being correct, and the same ladder minted lk.fail on production calls that have no simulator at all. Move the outcome to on_simulation_end, where the simulator's judgment is available and the DB check can veto it. The seed diff and the read-tool allowlist fed nothing but that ladder, so they go with it, along with the expected-state diff duplicated there.
`ctx.userdata().get("expected_state") or []` collapsed three distinct
scenario configurations into one: an absent key, an explicit null, and an
explicit []. Only the first means "do not grade state"; the other two
assert the seed DB comes back untouched. Falsiness skipped the diff for
all three, so a scenario written to catch a write that must not happen
graded nothing and passed regardless of what the agent wrote.
Parse the field instead of testing it for truth, and reject a malformed
value. build_expected() iterates the statements, so a string left
undashed in YAML was previously iterated character by character and each
character executed as SQL.
Nothing guarded the grading itself, so a scenario with unparseable expected_state or SQL that will not execute raised out of the callback into the framework's handler, which logs and reports an error but leaves the tagger with neither lk.success nor lk.fail. A scenario whose grading cannot run is not a scenario that passed; the distinct reason prefix keeps a broken scenario separable from a failing agent. Also tag state:graded / state:ungraded. Whether the DB was checked is not recoverable from the outcome, and most scenarios carry no expected_state by design - without the tag a forgotten assertion is indistinguishable from a deliberately omitted one.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The hotel receptionist's simulation grading now has one contract, enforced in one place.
What a scenario declares
userdata.expected_statesays how the final DB should be judged:nullor[]An omitted key and an empty list are different declarations: one opts out of state grading, the other asserts that nothing was written. Scenarios that assert an absence depend on it — the restaurant booking a guest can't complete without a phone number is graded entirely on no reservation appearing.
The comparison is by state, not by statement. It reads the facts the agent decided — room type, dates, extras, status — so confirmation codes, row order, and which particular room of a type don't matter, and the agent never has to reproduce the SQL. Writes nobody asked for still surface.
How a run is graded
on_simulation_endis the only place that sets an outcome. A run passes when both the conversation and the DB check pass:expected_state, SQL that won't execute) → the run fails, behind a prefix that tells a broken scenario from a failing agentEvery run is tagged
state:gradedorstate:ungraded. Most scenarios skip the DB check deliberately, and the outcome alone can't distinguish that from a forgotten assertion.on_session_endruns the judges, logs tags, dumps artifacts, and closes the DB. It sets no outcome: it fires afteron_simulation_end, and the tagger holds one outcome slot.What this replaces
The field was read as
get("expected_state") or []and tested for truth, so[]andnullskipped grading instead of asserting an unchanged DB.on_session_endderived its own outcome from a heuristic and, running later, overwrote the simulator's — failing conversation-only scenarios for correctly doing nothing, and marking production calls failed with no simulator involved. Grading that raised left a run with no verdict at all.Net 50 insertions, 90 deletions: the seed diff and the read-tool allowlist in
on_session_endfed nothing but the deleted heuristic, and the expected-state diff was duplicated there.Verification
examples/is excluded from collection (--ignore=examples,testpaths = ["tests"]), so this is verified out of tree against a real seeded DB: the parser contract, the stray-write veto, both broken-scenario paths, and conversation-only pass and fail.mypy --stricton this file goes from 2 pre-existing errors to 0.Overlaps #6567, which reworks the same two functions. Whichever lands second needs a manual resolution.