fix(cli): align utils/update/ + utils/command/ error messages with 4-ingredient strategy#1257
Open
John-David Dalton (jdalton) wants to merge 2 commits intomainfrom
Open
fix(cli): align utils/update/ + utils/command/ error messages with 4-ingredient strategy#1257John-David Dalton (jdalton) wants to merge 2 commits intomainfrom
John-David Dalton (jdalton) wants to merge 2 commits intomainfrom
Conversation
…ingredient strategy Rewrites error messages across packages/cli/src/utils/update/ and packages/cli/src/utils/command/ to follow the What / Where / Saw vs. wanted / Fix strategy from CLAUDE.md. Sources: - utils/update/checker.mts: 8 messages (URL validation, package name / registry URL validation, version-response validation). Each now names the function, the received type, and what a valid value looks like. - utils/update/manager.mts: 3 messages (mirror guards for name / version / ttl). Still warn-and-return-false, but the text now tells the caller exactly which option was wrong. - utils/command/registry-core.mts: 6 messages (command / alias registration conflicts, middleware next() misuse, flag parsing failures). Each now names the offending command, flag name, or index so debuggers don't need to read source. Tests updated: - test/unit/utils/update/checker.test.mts: 6 assertions (switched to regex) - test/unit/utils/update/manager.test.mts: 3 assertions (switched to expect.stringContaining) - test/unit/utils/command/registry-core.test.mts: 5 assertions All 153 tests in the affected suites pass. Follows strategy from #1254. Part of the multi-PR series started by #1255 (commands/) and continued in #1256 (utils/dlx/).
Two issues flagged by Cursor bugbot on #1257: 1. (Medium) registry-core.mts middleware error reported the wrong offending middleware index. `index` tracks the highest dispatched position, not the caller; when dispatch(i) is re-entered after a double-next(), the offender held middleware[i - 1]'s next closure. Fixed to use `i - 1` with a comment explaining why. 2. (Low) checker.mts error referenced a non-existent `UpdateChecker.fetch()` — the object is actually exported as `NetworkUtils`. Renamed in both the error and its test regex. Caught by #1257 bugbot review.
3 tasks
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 952b914. Configure here.
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
PR 4 in the error-message cleanup series. Covers
packages/cli/src/utils/update/(the update-checking stack) andpackages/cli/src/utils/command/registry-core.mts(the custom command-registry layer). ~17 messages across 3 source files + 3 test files.What's fixed
utils/update/checker.mts(8 throws)Registry / network / input validation errors. Before, most said "Package name must be a non-empty string" or "Invalid registry URL: foo". After, each message names:
Example:
Before:
Invalid version data in registry responseAfter:
https://registry.npmjs.org/foo/latest responded without a .version string (got: {"name":"foo"}); the registry may be misconfigured or foo may not exist — verify the URL in a browserutils/update/manager.mts(3 warn guards)Mirror validation on the manager layer — same options, checked again before the checker call. Previously shared wording with checker.mts which was confusing when both fired. Now each message explicitly prefixes
checkForUpdates options.Xso you can tell which layer complained.These are
loggerLocal.warn + return false, not throws, so the tone stays soft ("skipping update check" rather than "throw").utils/command/registry-core.mts(6 throws)The custom CLI command-registry layer. Conflict errors now name both commands involved; middleware errors name the index; flag errors name the flag and the expected type. Examples:
Before:
Alias "test" conflicts with existing command "test"After:
cannot register command "other" alias "test": conflicts with command "test"; rename the alias or unregister the conflicting command firstBefore:
Missing value for flag --nameAfter:
flag --name requires a string value but none was provided; pass it as --name=<value> or --name <value>Tests updated
test/unit/utils/update/checker.test.mts— 6 assertions switched to regextest/unit/utils/update/manager.test.mts— 3 assertions switched toexpect.stringContainingtest/unit/utils/command/registry-core.test.mts— 5 assertions153/153 tests in the affected suites pass.
Test plan
--page fooor similar invalid flag into the custom registry's built-in parser (it's not wired to most commands, but the tests exercise it)Note
Low Risk
Low risk: changes are limited to error/warn message wording and corresponding test assertion updates, with no functional logic changes to command execution or update checking paths.
Overview
Improves CLI diagnostics by rewriting command registry and update-checking validation errors to include the calling function/context, the received value, and clear remediation guidance (e.g., duplicate command/alias conflicts, middleware
next()double-invocation, missing/invalid flag values, invalid registry inputs/registry responses).Updates unit tests to match the new, more descriptive messages (switching several assertions to regex/
stringContaining).Reviewed by Cursor Bugbot for commit 952b914. Configure here.