Improve handling of config and rules errors for app server clients #9182
Conversation
When an invalid config.toml key or value is detected, the CLI currently just quits. This leaves the VSCE in a dead state. This PR changes the behavior to not quit and bubble up the config error to users to make it actionable. It also surfaces errors related to "rules" parsing.
etraut-openai
changed the title
| .with(otel_logger_layer) | ||
| .with(otel_tracing_layer) | ||
| .try_init(); | ||
| for warning in &config_warnings { |
There was a problem hiding this comment.
If warnings is non-empty, should we start up or should we just exit?
Admittedly, this would be more palatable if we were able to help the user find the errant config.toml and fix it...
There was a problem hiding this comment.
We should definitely start up. The current code just exits, and that leaves the user with a broken experience in the VSCE (just a spinner with no indication of a failure).
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct ConfigWarningNotification { |
There was a problem hiding this comment.
I realize it is not easy to do, but it would probably be most helpful if we could include the file (or files) that caused the issue and then helped the user open them in VS Code so they can make the necessary fixes and then reload?
There was a problem hiding this comment.
Yeah, I thought about that. I didn't want to make this PR more complicated. Could be a follow-on PR.
|
|
||
| match load_exec_policy(config_stack).await { | ||
| Ok(policy) => Ok((policy, None)), | ||
| Err(err @ ExecPolicyError::ParsePolicy { .. }) => Ok((Policy::empty(), Some(err))), |
There was a problem hiding this comment.
Similar to falling back to a default config, falling back to an empty could be particularly harmful/confusing for a user.
There was a problem hiding this comment.
Agree — that's why it's important to surface the warning to them.
There was a problem hiding this comment.
#9187 should help a bit so that at least all ExecPolicyError variants have a PathBuf associated with them.
Maybe we should have a sources: Vec<PathBuf> on ConfigWarningNotification now, even if it's just an empty array, so we can start experimenting with building UI against it?
There was a problem hiding this comment.
Can you think of a case where we need multiple paths? Shouldn't we always be able to map an error to a specific file?
Also, do you think we should go so far as to provide a line number and character offset? Or even a character range?
This is a slippery slope which is why I kind of didn't want to go there right now since my main objective is to just keep the VSCE from hanging on startup.
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
I like the UI in your screenshots, but in VS Code, we might want to use vscode.window.showErrorMessage() as well, just in case our sidebar is closed?
When an invalid config.toml key or value is detected, the CLI currently just quits. This leaves the VSCE in a dead state.
This PR changes the behavior to not quit and bubble up the config error to users to make it actionable. It also surfaces errors related to "rules" parsing.
This allows us to surface these errors to users in the VSCE, like this: