👀🤏 Doctor tool PR review#121
Merged
Merged
Conversation
Adding a return value moved `click.echo(render(...))` after the try/except, so a failure while rendering or echoing a successful result (e.g. BrokenPipeError from `aqua ... | head`, or a TypeError from a non-serializable result under --format json) escaped as an uncaught traceback with the wrong exit code instead of the formatted render_error + exit 1 that every CLI command relied on. Move the echo and return back inside the try and add a regression guard.
Two safety touch-ups from PR review: - `_is_prunable_default` now uses `.get(name, True)`, matching `features.is_tool_enabled` exactly, so doctor's notion of "matches the default" can't drift from the value the runtime applies for an absent key. Documents that pruning default-matching entries is sound only while an existing tool's shipped default never changes. - Orphan-tool findings now include the value (`'foo'=False ...`) so `--fix` never *silently* drops a deliberate disable — relevant under version skew, where a tool a newer binary added and the user disabled looks like an orphan to an older binary and would fall back to its default once re-recognized.
coelhogonzalo
changed the title
coelhogonzalo
changed the title
3 tasks
coelhogonzalo
marked this pull request as ready for review
July 15, 2026 18:11
andycreed0x
approved these changes
Jul 17, 2026
andycreed0x
left a comment
andycreed0x
left a comment
Collaborator
There was a problem hiding this comment.
good job. Thanks Gon
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
2 participants
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.
PR review fixes for
aqua doctorFeel free to edit this PR, no issue is big enough to block the original PR being merged.
Addresses findings from the review of the doctor tool. Two focused commits:
9509fcb— Fix run_tool: keep output rendering inside the error handlerAdding a return value moved
click.echo(render(...))after the try/except, so a failure while rendering or echoing a successful result (e.g.BrokenPipeErrorfromaqua ... | head, or aTypeErrorfrom a non-serializable result under--format json) escaped as an uncaught traceback with the wrong exit code instead of the formattedrender_error+ exit 1 that every CLI command relied on. Move the echo and return back inside the try and add a regression guard.4cc0926— doctor: surface orphan values and align default lookup with runtimeTwo safety touch-ups from PR review:
_is_prunable_defaultnow uses.get(name, True), matchingfeatures.is_tool_enabledexactly, so doctor's notion of "matches the default" can't drift from the value the runtime applies for an absent key. Documents that pruning default-matching entries is sound only while an existing tool's shipped default never changes.'foo'=False ...) so--fixnever silently drops a deliberate disable — relevant under version skew, where a tool a newer binary added and the user disabled looks like an orphan to an older binary and would fall back to its default once re-recognized.