fix(cli): validate/recipe UX papercuts (#1383 items 1-7)#1391
Merged
Conversation
mchmarny
added
the
theme/validation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR delivers several CLI output quality improvements. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/logging/cli_test.go`:
- Around line 96-127: Add a test case to the
TestCLIHandler_FailureStatusColoredRed function that validates handler-bound
status attributes. Create a test variant that uses logger.With("status",
"...").Info(...) to log a record, ensuring the colorization logic that checks
h.attrs (handler-bound attributes) is covered. This should verify that status
values set via logger.With() are correctly colored red or green just like status
values passed directly to logger.Info().
In `@pkg/snapshotter/agent.go`:
- Around line 209-210: The bare string values being passed to slog.Warn() at
lines 209-210 for the "namespace" and "jobName" fields do not follow the
established logging pattern used elsewhere in this file. Wrap both string field
values with the slog.String() helper function to match the consistent structured
logging pattern used at lines 161, 168, and 182. This ensures all string fields
are properly wrapped with their respective type helpers when passed to
slog.Warn().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 23100763-1a37-4218-862e-587dd211bebf
📒 Files selected for processing (9)
pkg/cli/recipe.gopkg/cli/recipe_list.gopkg/cli/recipe_list_test.gopkg/cli/validate.gopkg/logging/cli.gopkg/logging/cli_test.gopkg/serializer/writer.gopkg/serializer/writer_test.gopkg/snapshotter/agent.go
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
mchmarny
force-pushed
the
fix/cli-ux-papercuts
branch
from
57012ca to
4976133
Compare
lalitadithya
previously approved these changes
Jun 16, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/serializer/writer.go`:
- Around line 285-291: The string escape check in the condition around line 287
only detects newline characters with strings.Contains(s, "\n"), but tab and
carriage return characters can also distort tabwriter alignment. Expand the
condition to also check for the presence of `\t` and `\r` characters in the
string value, so that compactJSONLeaf is called whenever any of these
problematic characters are detected, ensuring consistent table formatting
regardless of which whitespace characters are present in the string.
In `@pkg/snapshotter/agent.go`:
- Around line 208-211: The warning log in the agent.go file uses the field key
"jobName" in the slog.String call for agentConfig.JobName, but this value is
logged with the field key "job" elsewhere in the function. Change the field key
from "jobName" to "job" in the slog.String call to maintain consistent field
naming across all logs, which ensures log queries and correlation are not
fragmented across different field names.
- Around line 231-234: In the error message construction for the GPU scheduling
failure (the msg variable), the example node selectors provided
(kubernetes.io/os=linux and node-role=worker) are generic and don't address the
actual GPU scheduling problem. Replace these generic selector examples with
GPU-specific guidance that remains focused on the nvidia.com/gpu.present=true
selector or the --require-gpu option, so users are directed toward solutions
that actually solve GPU node scheduling instead of generic selectors that won't
fix the problem.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d3dc1639-4917-4acd-817a-26674403a642
📒 Files selected for processing (9)
pkg/cli/recipe.gopkg/cli/recipe_list.gopkg/cli/recipe_list_test.gopkg/cli/validate.gopkg/logging/cli.gopkg/logging/cli_test.gopkg/serializer/writer.gopkg/serializer/writer_test.gopkg/snapshotter/agent.go
mchmarny
force-pushed
the
fix/cli-ux-papercuts
branch
from
4976133 to
26dc7a0
Compare
Address the remaining CLI UX items split out from #1361 (the chainsaw "no such file" symptom was already fixed by the in-process executor): 1. Failed validators no longer print green. The CLI slog handler now colors a record red when it carries status=failed/error, even at Info level (CTRF logs "validator completed status=failed" at Info). 2. `recipe` completion now lists the resolved component names, not just the count. 3. `recipe` and `validate` print a one-line mode banner so it's clear whether the run touches a live cluster (recipe is offline; validate distinguishes live-cluster vs --no-cluster dry-run). 4. `-o table` no longer garbles on nested values: the flattener renders maps, scalar slices, and multi-line strings as single-line compact JSON, keeping FIELD/VALUE columns intact while still recursing structs and struct slices. 5. `recipe list` table prints a legend explaining that a bare `any` criteria value is an intentional wildcard (human-readable surfaces only; structured output is unchanged). 6. The agent "pod did not become ready" warning now names the namespace and jobName. 7. The "job failed" GPU-scheduling hint now shows concrete, repeatable --node-selector key=value syntax. Closes #1383
mchmarny
force-pushed
the
fix/cli-ux-papercuts
branch
from
26dc7a0 to
d6b824d
Compare
dims
approved these changes
Jun 16, 2026
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
3 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.
Summary
Resolves items 1–7 of the #1383 tracker — the remaining
validate/recipeCLI UX papercuts split out from #1361. (The big symptom, the repeatedchainsaw: no such file, was already fixed by the in-process chainsaw executor; item 8 came along with that move and is left to confirm/close separately.)Motivation / Context
These are small, independent, same-area UX fixes that make the CLI legible to a confused user. Bundled per the tracker's own framing ("fixable in one or two PRs").
Fixes: #1383
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/collector,pkg/snapshotter)pkg/errors,pkg/k8s) —pkg/logging,pkg/serializerImplementation Notes
pkg/logging/cli.gocolored strictly by log level, so aslog.Info("validator completed", "status", "failed")rendered green. The handler now also reds a record carryingstatus=failed/error(hasFailureStatus). No log-level reclassification, soAICR_LOG_LEVELfiltering is untouched.recipelists component names, not just the count, on the completion line.recipelogs that it runs offline (reads inputs, never deploys/modifies a cluster);validatedistinguishes live-cluster vs--no-clusterdry-run.-o tablenested rendering. The table flattener now renders maps, scalar slices, and multi-line strings as single-line compact JSON (struct and struct-slice recursion preserved), so nestedvalues.yamlfragments no longer shatter the FIELD/VALUE columns. Empty top-level collections still print<empty>.anylegend.recipe listtable output appends a one-line legend thatany= wildcard/unconstrained. Human-readable surface only — the recipe YAML/JSON body still emits the literalany.namespaceandjobName.--node-selector key=valuesyntax.Testing
New tests: status-attr coloring (failed/error→red, passed/skipped→green); table compact-JSON leaves (nested map, scalar slice, multi-line string) + empty-data still
<empty>;recipe listwildcard legend.Risk Assessment
-o tablenested rendering, covered by new + existing tests.Rollout notes:
-o tablenow collapses nested values into compact JSON cells instead of exploding/garbling them — more readable, but anyone scraping the old per-key table rows should use-o json/-o yaml(the machine-readable formats, unchanged).Checklist
make testwith-race)make lint)git commit -S)