Surface actionable Nix error above show-trace frames#2849
Open
domenkozar wants to merge 2 commits into
Open
Conversation
Deploying devenv with
|
| Latest commit: |
28d78b9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f91f322.devenv.pages.dev |
| Branch Preview URL: | https://fix-2820-actionable-error.devenv.pages.dev |
A Nix eval error with `show-trace=true` (always on in devenv) rendered as a single line headline `Failed to get shell attribute from devenv:` followed by ~120 lines of `lib/modules.nix` frames, with the actionable text (the thrown assertion, e.g. `$ devenv inputs add git-hooks ...`) buried at the very bottom. The follow up comments on #2820 reported the suggestion as "swallowed". `format_eval_error` now extracts the trailing `error: ...` paragraph from the FFI return value and uses it as the diagnostic headline; the preceding frames move to miette's `help:` section. When no `error:` paragraph is present (malformed input, future format change), the original text is rendered as one block (graceful fallback). Vestigial code removed: `select_raw_error`, `looks_like_warning`, `peek_pre_repl_errors`. The C bindings now return the full error via FFI rather than the log callback, so the log buffer merge no longer contributes anything. `take_pre_repl_errors` stays because `launch_repl` uses it to replay TUI swallowed log lines. Tests: * 5 new unit tests on `format_eval_error` covering the happy path, the bare syntax error shape, warning shadow regression, graceful degradation, and indent stripping (no FFI required, all under 40ms). * `tests/missing-input-error/` new integration test asserting the `devenv inputs add ...` suggestion appears alongside the headline. * `tests/syntax-error/.test.sh` gains a warning shadow assertion on the headline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
e063888 to
d02361d
Compare
Contributor
🔍 Suggested ReviewersBased on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:
Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏 This comment was automatically generated by git-blame-auto-reviewer Last updated: 2026-05-20T15:44:40.320Z |
`split_trailing_error` matched any line whose stripped prefix was
`error:`. That misfired on user-authored multi-line `throw`s whose
message body contained a line like `error: my note`: my parser picked
the user's literal text as the boundary, leaving the real Nix `error:`
keyword and the leading lines of the throw in the demoted trace.
Reproduction:
env.GREET = throw ''
Step 1: do this
error: but this is the actual problem
Step 3: then this
'';
Before: headline `× ... error: but this is the actual problem` plus
`Step 3: then this`, with `Step 1: do this` buried in `help:`.
After: headline starts with Nix's real `error:` keyword and carries the
full user throw body.
Discriminator: Nix's real `error:` paragraphs are always separated from
preceding content by at least one blank line. User content inside a
`throw` sits on consecutive lines. Require the boundary line to be
preceded by a blank line (or start of input).
Tests:
* New `split_trailing_error_ignores_error_keyword_inside_user_throw_body`
locks in the regression.
* `split_trailing_error_picks_last_error_when_multiple_present` and
`split_trailing_error_handles_ansi_prefixed_error_line` get an
explicit blank line in their fixtures so they model what Nix actually
emits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d02361d to
28d78b9
Compare
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.
Summary
Addresses the follow-up comments on #2820: the actionable text in a Nix evaluation error (the thrown assertion, the syntax error keyword, the "use
devenv inputs add …" suggestion) is no longer buried at the bottom of ~120 lines of--show-traceframes. It now appears as the diagnostic headline, with the trace demoted tomiette'shelp:field.Before (missing-input case):
After:
Commits
290d57dc— extractformat_eval_errorpure helper, use the trailingerror: …paragraph as the headline and demote the frames tohelp:. Also dropsselect_raw_error,looks_like_warning,peek_pre_repl_errors: empirical probing showed they were no-ops with the currentcachix/nix-bindings-rust(the log buffer is empty during eval failures because the C bindings return the full error via FFI).e0638880— require the boundaryerror:line to be preceded by a blank line. Without this, a user-authored multi-linethrowwhose body containederror: fooas a literal line would split the user's message across headline andhelp:.Verified on five distinct error shapes
error: Failed assertions: - To use 'git-hooks'…error: syntax error, unexpected '}'…1 + "string")error: cannot add a string to an integererror: undefined variable 'foo'Tests
format_eval_errorcovering: happy path, bare syntax error, warning-shadow regression, graceful degradation when noerror:line is found, and indent stripping.split_trailing_error_ignores_error_keyword_inside_user_throw_body) locking in the multi-line throw fix frome0638880.tests/missing-input-error/integration test asserting thedevenv inputs addsuggestion lands within 5 lines of the headline.tests/syntax-error/.test.shgains a headline assertion that the staleIgnoring the client-specified setting 'system'warning does not become the diagnostic title.Known limitation (not fixed here)
For syntax errors specifically, Nix puts the
at /path:N:M:location block and source-code caret before theerror:line, separated by a blank line. The split keys on theerror:line, so the location ends up inhelp:rather than alongside the headline. The user still sees the location, just one section down. A separate, more invasive parsing pass would be needed to pull a precedingat <user-file>:N:M:block into the tail when it's not part of a… while …frame — left as follow-up to keep this PR focused on the buried-suggestion symptom.Why parsing at all
cachix/nix-bindings-rust(branch2.34) exposes evaluation errors as a single formatted string via the Cerr_msgaccessor; the C++EvalError's frames vector is not surfaced. I instrumented every callback (on_start/on_stop/on_result/on_log) and confirmed that during an eval failure they emit only progress-level log messages — zero structured trace events. So extracting the actionable bit requires textual parsing today. The truly principled fix is an upstream binding change to expose structured error data; this PR is the user-facing improvement we can land without that.Test plan
cargo nextest run -p devenv-nix-backend error::— 13 unit tests passdevenv-run-tests run --only syntax-error --only missing-input-error tests— both pass🤖 Generated with Claude Code