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.
Implement exploration-error trajectory grader #57087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement exploration-error trajectory grader #57087
Changes from all commits
6c5e41fe99f3074daf382File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L20: shrink: repeated candidate-resolution logic for objectives/events/states/observations. Build one normalized candidate once and reuse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: null/undefined
event.refvalues conflate distinct statesJSON.stringify(event.ref)serialises bothnullandundefinedto the string"null", so multiplestate_changeevents with a missingreffield all hash to the same key and are counted as one visited state. This silently underestimatesdistinctStatesVisited.Suggested fix:
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The formula counts all observations in the trace rather than observations scoped to unmet objectives. A run that over-observed one area while ignoring the area containing unmet objectives would still report near-zero exploration error — a misleading "good" score.
💡 Design note
observationsis the full set from whichever IR candidate wins thecandidates.find()race; it has no semantic relationship to the specificunmetobjectives computed a few lines earlier. A more faithful measure would scope observations to those whoseobjectiveRef(or equivalent linkage field) appears inunmet. Even documenting this limitation in the leading comment would set right expectations for consumers.If the IR doesn't carry per-objective observation linkage yet, consider adding:
// TODO: scope observations to unmet objectives once IR supports objectiveRef@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
objective.descriptionis not type-guarded — if it isundefinedor a non-string the joineddetailsstring will contain a literal"undefined"token, degrading observability.💡 Suggested fix
policy-near-miss.mdhas the same gap; this grader is a good opportunity to set the better pattern.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Division-by-zero when
distinctStatesVisitedis 0If
stateChangeEvents.length > 0but all events map to the same ref (e.g., everyevent.refisnulland after the bug fix abovedistinctStatesVisitedcould still conceivably be 0 through future code paths),1 - observations.length / 0evaluates toNaNorInfinity. Thehelpers.clampcall does not guard againstNaN.Add an explicit guard before computing the ratio:
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L68: delete: long prose comment block that repeats the frontmatter summary. Keep a short comment or none.