fix(theme): warn on unknown theme id in config - #569
Conversation
/color rejects unknown names, but a typo in [ui].theme was silently remapped to one-dark. resolve_theme now pushes a user-facing warning (and tracing) so the fallback is visible; the id is still returned so from_name's fallback behaviour is unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a955d06fb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| agent_code_lib::services::warnings::warn(format!( | ||
| "unknown theme '{id}'; using one-dark. Run /color to list" | ||
| )); |
There was a problem hiding this comment.
For interactive TUI startup—the production path that calls resolve_theme—main.rs redirects tracing output to agent.log, while a repo-wide search shows that the warnings registry has no production reader: snapshot() is only called by tests, and /doctor in commands/mod.rs only prints diagnostic checks. Consequently, this push never reaches the claimed startup banner or /doctor, so users with a misspelled [ui].theme still receive no visible warning unless they inspect the log; wire the registry into the TUI/doctor or emit the warning through an existing UI event path.
Useful? React with 👍 / 👎.
| agent_code_lib::services::warnings::clear(); | ||
| let _ = resolve_theme("one-dark"); | ||
| assert!( | ||
| agent_code_lib::services::warnings::snapshot().is_empty(), |
There was a problem hiding this comment.
Under Rust's default parallel test runner, this test clears the process-wide warning registry without sharing a lock with other CLI tests that push and inspect warnings, such as the_announcement_reaches_the_operator in ui/modern/run.rs. An interleaving can erase that test's warning before its assertion, or let its warning appear before this test's is_empty() assertion, making the CI suite flaky; guard the entire test with a shared registry lock or avoid clearing/asserting global state.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a955d06fb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| agent_code_lib::services::warnings::warn(format!( | ||
| "unknown theme '{id}'; using one-dark. Run /color to list" | ||
| )); |
There was a problem hiding this comment.
In the interactive TUI—the production path that resolves the configured theme—tracing is redirected to agent.log, while a repo-wide search shows no production caller of services::warnings::snapshot() and /doctor only renders diagnostics checks. Consequently this registry entry is never shown in the claimed startup banner or /doctor, so users with a misspelled [ui].theme still receive no visible warning; wire the registry into the TUI transcript/banner or another user-visible startup path.
Useful? React with 👍 / 👎.
| fn unknown_theme_id_is_flagged_but_still_resolves() { | ||
| // resolve_theme must not panic or rewrite the id — from_name falls | ||
| // back to one-dark — but warn_if_unknown_theme must fire. | ||
| agent_code_lib::services::warnings::clear(); |
There was a problem hiding this comment.
When CLI tests run in parallel, these unconditional clear() calls race with other tests using the same process-wide registry, notably the_announcement_reaches_the_operator: this test can erase that test's warning before its snapshot, or that test can push between the second clear and this test's is_empty() assertion. Use a shared registry test lock or avoid asserting global emptiness so the default parallel cargo test run is not flaky.
Useful? React with 👍 / 👎.
Codex review (#569): interactive startup redirects tracing to agent.log, so a registry-only push never reached the user. Surface the first process warning on the status bar, and serialize registry tests via warnings::test_lock.
1 participant
Summary
[ui].themevalues no longer fail silently — a warnings-registry + tracing notice names the id and points at/color.Part of #561 (D8-12).
Test plan
unknown_theme_id_is_flagged_but_still_resolves