Repository navigation
fix(modal): read the eval journal after a nonzero eval run exit - #347
Merged
Merged
Conversation
`ultrafuzz eval run` reports `ok: false` for any failed or incomplete row, `emitCommandResult` turns that into exit 1, and `runCommand` turns exit 1 into a throw. Gating the `model_work_started` corroboration hook on `runError === undefined` therefore gated it on exactly the rows it was written to account for: the failed row that would earn a `none` verdict is the same row that skipped the read, so the only production-reachable `none` was a zero-record journal and the pre-model retry #320 set out to restore was never delivered. Gate on what actually matters instead -- whether the command returned. A command that chose its own exit code has finished writing its journal, however it exited; one this worker timed out or aborted may have launched work it never recorded, so `runCommand` and `throwIfAborted` now raise `PublicWorkerCommandInterruptedError` and only that shape keeps the raised flag on the strength of the interruption alone. The flag is still only ever lowered, and only on a journal that positively accounts for every record. Also from the same review: - `launchEvalRow` read the terminal report path between a successful launch and the `runs.jsonl` append. With the gate relaxed a dropped launched row would read as `none` for a run that did spend tokens, so that read can no longer throw. - the `catch` around the hook was silent, so a persistently throwing `assertSafeEvalId` would have been invisible; it now records a redacted line in the worker log without becoming an outcome itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… read `runCommand` folded a child's termination signal into `code ?? 1`, so an eval command reclaimed by the sandbox or taken by the OOM killer arrived at `publicEvalCommandLeftFinalJournal` as a command that returned. With #332's gate relaxed that is newly load-bearing: corroboration then reads a `runs.jsonl` holding only the rows that had already appended, answers `none`, and lowers `model_work_started` for a run whose in-flight rows may have spent their budget. A child that never chose an exit code never reached the end of its writing, so it is an interruption like any other. Both of `runCommand`'s interruption paths were also untested -- reverting the throw left the suite green, because the one existing case is pre-aborted and rejects at `materializeBakedCandidate`'s own `throwIfAborted` without reaching `runCommand`. Cover the branch that actually fires in production with a child that is alive when the interruption lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Closes #332.
The corroboration hook added by #320 was gated on
runError === undefined, but any failed or incomplete row makesultrafuzz eval runexit 1 (eval/run.ts:63→command-shared.ts:117-119), whichrunCommandturns into a throw. So the hook could never run for the case it was written for.Proven on the production path, by driving a real 2-row matrix with rows failing pre-model:
origin/maineval runEXITS NONZEROmodel_work_startedafter settletruefalserunCommandandthrowIfAbortedalready knew the distinction that matters but discarded it: a killed command and a self-chosen nonzero exit both surfaced as a bareOperationalDispositionError("unreachable"). The timeout/abort branch now raisesPublicWorkerCommandInterruptedError, and the gate became "not an interruption" — a command that chose its exit code has finished writing; one that was killed may have launched work it never journaled and keeps its raised flag.The flag is still only ever lowered, and only on a journal that positively accounts for every record.
Also:
resolveTerminalReportPathbetween a successful launch andappendJsonLineis now total, and the previously silentcatchwrites one redacted line throughsanitizeWorkerDiagnosticMessage.Residual, flagged not fixed: with the gate relaxed, a genuinely partial journal becomes reachable — if
runEvalSuitethrows mid-matrix, rows still in flight never append. The principled close is reader-side (require the journal to account for everymatrix.jsonrow before answeringnone), but that flips a deliberately documented assertion and is a call for the maintainer, not the implementer.Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Greptile Summary
The PR allows the public Modal worker to corroborate model-work state after an eval command completes with a nonzero exit while preserving the raised checkpoint when the command is interrupted.
Confidence Score: 5/5
The PR appears safe to merge, with no unacknowledged actionable defects established in the changed behavior.
Completed nonzero eval exits now reach journal corroboration, while timeout, abort, and signal termination preserve the conservative model-work checkpoint; downstream report consumers remain able to resolve report paths from each run root.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Start eval command] --> B{How did child terminate?} B -->|Timeout, abort, or signal| C[PublicWorkerCommandInterruptedError] C --> D[Keep model_work_started raised] B -->|Exit code 0 or nonzero| E[Read final eval journal] E --> F{Journal evidence} F -->|Launched or unknown| D F -->|No model work| G[Lower model_work_started] D --> H[Persist diagnostics and terminal status] G --> HReviews (1): Last reviewed commit: "fix(modal): keep an externally killed ev..." | Re-trigger Greptile
Context used: