Skip to content

fix(config): report an unparsable user config instead of panicking - #4002

Merged
max-sixty merged 4 commits into
mainfrom
fix/user-config-load-error-context
Sep 3, 2026
Merged

fix(config): report an unparsable user config instead of panicking#4002
max-sixty merged 4 commits into
mainfrom
fix/user-config-load-error-context

Conversation

@worktrunk-bot

@worktrunk-bot worktrunk-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

With a ~/.config/worktrunk/config.toml that doesn't parse, wt config show --format json, wt config show --full, wt step prune, wt step relocate, wt step eval, and wt step for-each exit 101 with a Rust panic on a debug build. A release build doesn't panic, but its header becomes a bare ✗ Command failed naming neither the config nor the file — the parse detail still reaches the gutter. The text form of that same config show renders a full, correctly-labelled diagnosis of the same file, so the JSON path breaks on exactly the condition you would run it to diagnose.

UserConfig::load() flattens LoadError::File's multi-line Display — the header plus the TOML parser's caret diagram — into a ConfigError string. Six call sites propagated it bare with ?, so it reached anyhow with no context and no cause chain: the one shape render_error in src/main.rs has no arm for, where it trips that function's own debug_assert! and falls back to the unlabelled header. Adding .context("Failed to load config") — what 7 of the 22 UserConfig::load() call sites already did, and what all 13 propagating ones do after this (the other 9 swallow the error deliberately) — gives the renderer its header back.

Verified by hand against a malformed config before and after, and by cargo test --test integration (2045 passed; the one failure, test_copy_ignored_preserves_file_executable_permissions, reproduces identically with these changes stashed — it asserts 0644 and this sandbox's umask is 002, so it is unrelated to this PR).

Before and after

Before, on a debug build:

thread 'main' (25707) panicked at src/main.rs:1052:21:
Multiline error without CommandError or context: User config @ ~/.config/worktrunk/config.toml failed to parse:
TOML parse error at line 1, column 16

After:

✗ Failed to load config
  User config @ ~/.config/worktrunk/config.toml failed to parse:
  TOML parse error at line 1, column 16
    |
  1 | invalid = [toml
    |                ^
  unclosed array, expected `]`

test_unparsable_user_config_errors_legibly is the regression test, one case per fixed call site. Reverting .context(...) on any single site fails that site's case with left: Some(101) while the others keep passing — checked for all six.

Two things I deliberately left alone

The structural fix. A call site can reintroduce this by writing ? instead of .context(...), and nothing catches it — the debug_assert! only fires once the path is actually exercised, which is why the test covers every fixed site rather than a sample. Closing that would mean LoadError exposing the parse error through Error::source() rather than in its own Display, and ConfigError growing a cause to carry it — and ConfigError is a pub struct ConfigError(pub String) constructed at 63 sites, so that is a wider change than this defect justifies on its own. Worth doing if you want it; I did not want to fold that decision into a bug fix.

Whether --format json should fail here at all. The text path warns and renders; the JSON path errors. Failing is arguably the better machine-readable answer — handing back a config object that is not the user's config is worse than a non-zero exit — but making it report the parse error in the payload instead is a real alternative, and it is a JSON-schema decision rather than a bug. This PR only makes the existing failure legible.

One pre-existing wrinkle this PR surfaces but doesn't fix

These commands now print the same six-line diagnosis twice: ▲ User config @ … failed to parse, skipping from emit_config_load_warning on the degrading path (via Repository::prewarm_user_config), then ✗ Failed to load config with an identical gutter. The warning's "skipping" also promises a degradation that doesn't happen on these paths. It predates this PR — the second block used to say Command failed — but the two blocks now read as contradictory verdicts on one file, so it's worth a look separately.

Found by the nightly sweep, from the wt config show file in today's rolling survey.

@worktrunk-bot worktrunk-bot added the nightly-cleanup Issues found by nightly code quality sweep label Sep 3, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself is right and the sweep is complete — after this change every UserConfig::load() site that propagates now carries .context("Failed to load config"), and the nine that don't (output/handlers.rs, completion.rs, picker/pager.rs, alias.rs) all swallow the error deliberately. I verified the three tested commands plus step eval and step for-each against a malformed config with the built binary: all five exit 1 with the header and the parser's caret diagram in the gutter.

Two things worth acting on, both inline: the test covers three of the six call sites it fixes, and the docstring overstates what the pre-fix release build lost.

The regression test leaves step eval and step for-each uncovered. Those are exactly the two sites where a future ? would regress silently — the debug_assert! only fires when the path is exercised, and nothing exercises it. I checked this rather than assuming: adding the two #[case] lines suggested below passes with this PR's changes, and with .context(...) reverted on just those two files the new cases fail with left: Some(101) and the Multiline error without CommandError or context panic, while the existing three keep passing.

The release-build claim is wrong in two places. render_error writes error_message("Command failed") and then format_with_gutter(&normalized, None) — the parse detail survives; only the header is unlabelled. multiline_error_helpers_normalize_line_endings in tests/integration_tests/git_error_display.rs pins that exact shape. So the release output wasn't contentless, it was misattributed, and the real gain is the header naming the file. The "other two thirds of the call sites" figure is off too: 7 of 22 carried .context before this change, 13 of 22 after — the PR body's "other seven" is the accurate count.

One observation, not a request. Running these commands against a broken config prints the same six-line diagnosis twice: ▲ User config @ … failed to parse, skipping from emit_config_load_warning (the degrading path, via Repository::prewarm_user_config), then ✗ Failed to load config with the identical gutter. The warning's "skipping" also promises a degradation that doesn't happen here. That predates this PR — the second block used to say Command failed — so it isn't a regression, but it's the surface this PR is improving and it now reads as two contradictory verdicts on the same file.

Not approving: this is a bot-authored PR, so I can't approve it in any case. Separately, src/commands/step/prune.rs is on the deletion surface the repo's review guidance holds for, so @max-sixty should take the merge call. The diff line there is UserConfig::load().context(…)? at command entry — .context maps Err to Err and Ok to Ok, so it provably cannot change what prune removes — but the hold is the maintainer's to lift, not mine.

Comment thread tests/integration_tests/config_show.rs Outdated
Comment thread tests/integration_tests/config_show.rs
Comment on lines +593 to +594
// The header the renderer needs, and the parse detail it would otherwise
// have replaced with "Command failed".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The header the renderer needs, and the parse detail it would otherwise
// have replaced with "Command failed".
// The header the renderer needs, and the parse detail that previously sat
// under an unlabelled "Command failed".

Same correction as the docstring above — the detail wasn't replaced, only the header was wrong.

Comment on lines +5 to 7
use anyhow::Context;

use std::path::PathBuf;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use anyhow::Context;
use std::path::PathBuf;
use std::path::PathBuf;
use anyhow::Context;

std before external crates, matching the two sibling files this PR touches (for_each.rs, eval.rs) and the rest of src/commands/. Nothing enforces it — there's no rustfmt.toml, and group_imports is nightly-only — so it only stays consistent by hand.

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two things from the last pass are handled: step eval and step for-each now have cases, and the release-build claim in the docstring is corrected — render_error does write format_with_gutter(&normalized, None) after the unlabelled header, so "what it loses is the header" is the accurate description.

The rewritten docstring introduces two new inaccuracies, both inline.

"One case per fixed call site" is one case short. The PR fixes six sites; five have cases. The uncovered one is the UserConfig::load() under // Test commit generation - use effective config for current project in render_diagnostics (src/commands/config/show.rs), which is reached only from wt config show --fullhandle_config_show calls it behind if full. That makes it exactly the site the docstring's own rationale is about: the debug_assert! fires only on an exercised path. I checked rather than assuming — adding the case suggested below passes with this PR as-is, and with .context(...) reverted on just that one line it fails with left: Some(101) and the Multiline error without CommandError or context panic while the other five keep passing. With the case added the sentence becomes true as written, so no edit to that paragraph.

The count moved the word "propagating" onto the wrong number. There are 22 call sites, of which 13 propagate and 9 swallow; 7 of the 22 carried .context before this change. As written — "7 of the 22 propagating … call sites already did, and all 13 do after this" — the two halves disagree with each other. The PR body's phrasing is the correct one.

Two notes, neither a request. The PR body lists five affected commands; wt config show --full is a sixth, for the same render_diagnostics site. And release / plan is red on curl: (35) Recv failure: Connection reset by peer fetching the cargo-dist installer — transient, unrelated to the diff, and green on the other open PRs.

Also worth knowing for merge order rather than as a review point: #3999 rewrites handle_config_show and touches both src/commands/config/show.rs and tests/integration_tests/config_show.rs off the same merge base. Different change, not a duplicate, but the two will conflict.

Not approving — this is a bot-authored PR, so I can't in any case. The src/commands/step/prune.rs hold from the last pass still stands and @max-sixty is already on the review request.

Comment thread tests/integration_tests/config_show.rs
Comment thread tests/integration_tests/config_show.rs Outdated
@max-sixty
max-sixty merged commit bc2ec12 into main Sep 3, 2026
43 checks passed
@max-sixty
max-sixty deleted the fix/user-config-load-error-context branch September 3, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nightly-cleanup Issues found by nightly code quality sweep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants