feat(cli): add structural-health columns to recipe list#1302
Conversation
Extend `aicr recipe list` with the ADR-009 §4 structural-health view, delegating computation to pkg/health so pkg/cli stays logic-free: - New facade method Client.ComputeHealth wraps health.Compute, binding the client's own DataProvider + version so --data overlays are scored against the same catalog the listing enumerates. - table: add STATUS (rolled-up verdict) and COVERAGE (compact per-phase named-check summary, R:n D:n P:n C:n) columns; non-leaf overlays render "-". - json/yaml: each entry gains an additive `health` block (per-dimension status map + full declared_coverage), omitted for non-leaf overlays. Embedded CatalogEntry carries yaml:",inline" so the NVIDIA#1208 fields stay top-level (yaml.v3 does not auto-inline anonymous fields). Rendering reads the dimension set generically, so an absent constraints_wellformed dimension (NVIDIA#1227) degrades gracefully — the status column still renders. Docs: extend the recipe list section of docs/user/cli-reference.md with the health columns, field descriptions, and real-catalog examples. Closes NVIDIA#1228
njhensley
added
the
theme/validation
📝 WalkthroughWalkthroughThis PR extends the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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 unit tests (beta)
Comment |
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Clean, additive recipe list health columns. The structural-health computation is correctly delegated to pkg/health via the new Client.ComputeHealth facade method, keeping pkg/cli logic-free, and the facade follows the sibling guard/inflight pattern. The leaf-name join with the catalog listing is correct, non-leaf overlays render the - placeholder, and the yaml:",inline" regression is explicitly guarded by a dedicated test. CI fully green; coverage up on both packages.
Two non-blocking nits inline (error-wrap symmetry, and the always-on health compute on every list). Approving.
| // entry can be paired with its verdict during rendering. | ||
| report, err := client.ComputeHealth(ctx, filter) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
nit: the ListCatalog sibling two lines up wraps with errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to list catalog"), while this returns err bare. Functionally fine since ComputeHealth already wraps internally with PropagateOrWrap, so re-wrapping here would double the message — flagging only for symmetry with the adjacent call. No change needed.
There was a problem hiding this comment.
Thanks — agreed, keeping the bare return. ComputeHealth already wraps with PropagateOrWrap(err, ErrCodeInternal, "failed to compute recipe health"), so re-wrapping here would double the message and is exactly the "don't double-wrap errors that already have proper codes" case in CLAUDE.md. (The adjacent ListCatalog call is arguably the asymmetric one, since the facade ListCatalog already wraps internally too — but that's pre-existing #1208 code and out of scope here.)
| // facade, which binds this Client's own provider so --data overlays | ||
| // are scored). Health is keyed by leaf overlay name so each catalog | ||
| // entry can be paired with its verdict during rendering. | ||
| report, err := client.ComputeHealth(ctx, filter) |
There was a problem hiding this comment.
nit: recipe list now resolves every leaf on every invocation, so the previously-cheap enumeration always pays the full health-compute cost with no way to opt out. Within ADR-009's sub-minute budget so not blocking — but if the added latency is noticeable interactively, a --no-health / --skip-health flag would be a reasonable follow-up.
There was a problem hiding this comment.
Good call-out. Health compute runs within ADR-009's sub-minute budget so it's fine for v1, but an opt-out (--no-health / --skip-health) is a sensible follow-up if interactive latency becomes noticeable. Tracking it as a follow-up rather than expanding this PR's scope.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/user/cli-reference.md (2)
545-549:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefine ADR acronym on first use.
Line 545 references "ADR-009 §4" but the acronym ADR (Architecture Decision Record) is not defined earlier in this document. As per coding guidelines, define acronyms on first use in documentation.
📝 Suggested expansion
-Each leaf overlay also carries a structural-health verdict (ADR-009 §4): a +Each leaf overlay also carries a structural-health verdict (Architecture Decision Record 009, §4): aOr if space is a concern:
-Each leaf overlay also carries a structural-health verdict (ADR-009 §4): a +Each leaf overlay also carries a structural-health verdict (per ADR-009 §4, the structural-health design): a🤖 Prompt for 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. In `@docs/user/cli-reference.md` around lines 545 - 549, Add a definition for the acronym ADR at first use where "ADR-009 §4" is referenced: replace or augment the first occurrence with "ADR (Architecture Decision Record)‑009 §4" or append "(ADR = Architecture Decision Record)" immediately after "ADR-009 §4" in the sentence about structural-health verdicts so readers see the full term on first mention; ensure subsequent mentions keep just "ADR-009" unchanged.Source: Coding guidelines
584-589: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify what the status column renders for unknown dimensions.
Line 589 states "and the status column still renders" but doesn't specify what value is rendered. Based on the rollup logic (context snippet 3), when a dimension cannot be confidently graded, the rolled-up status becomes
unknown. Make this explicit for clarity.📝 Suggested clarification
-Conformance). Non-leaf overlays render `-` in both columns. A dimension whose -grader cannot reach a confident verdict surfaces as `unknown`, and the status -column still renders. +Conformance). Non-leaf overlays render `-` in both columns. When a dimension's +grader cannot reach a confident verdict, that dimension surfaces as `unknown`, +and the STATUS column renders `unknown` (held rather than failing).🤖 Prompt for 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. In `@docs/user/cli-reference.md` around lines 584 - 589, Clarify that when a dimension's grader cannot reach a confident verdict the rolled-up STATUS column displays the literal value "unknown" (while COVERAGE still shows the per-phase summary); update the sentence that currently reads "and the status column still renders" to explicitly state that it renders "unknown" for such dimensions (keeping the existing note that non-leaf overlays render "-" in both columns and the COVERAGE format R:2 D:4 P:1 C:10).
🤖 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.
Outside diff comments:
In `@docs/user/cli-reference.md`:
- Around line 545-549: Add a definition for the acronym ADR at first use where
"ADR-009 §4" is referenced: replace or augment the first occurrence with "ADR
(Architecture Decision Record)‑009 §4" or append "(ADR = Architecture Decision
Record)" immediately after "ADR-009 §4" in the sentence about structural-health
verdicts so readers see the full term on first mention; ensure subsequent
mentions keep just "ADR-009" unchanged.
- Around line 584-589: Clarify that when a dimension's grader cannot reach a
confident verdict the rolled-up STATUS column displays the literal value
"unknown" (while COVERAGE still shows the per-phase summary); update the
sentence that currently reads "and the status column still renders" to
explicitly state that it renders "unknown" for such dimensions (keeping the
existing note that non-leaf overlays render "-" in both columns and the COVERAGE
format R:2 D:4 P:1 C:10).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 20b55fec-5599-4fb8-ba22-ff94aac36402
📒 Files selected for processing (1)
docs/user/cli-reference.md
2 participants
Summary
Extend
aicr recipe listwith the ADR-009 §4 structural-health view: a rolled-up status column and a compact per-phase coverage summary, delegating all computation topkg/healthsopkg/clistays logic-free.Motivation / Context
Part of the recipe health tracking epic (ADR-009 V1). #1208 delivered the
recipe listenumeration command (criteria/leaf/source columns +--format/filter flags); this adds the health-derived columns on top, without changing #1208's output shape.Fixes: #1228
Related: #1224 (epic), #1226/#1227 (health grading dependencies)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)docs/,examples/)pkg/client/v1facade (Client.ComputeHealth)Implementation Notes
Client.ComputeHealth(ctx, filter)wrapshealth.Compute, binding the client's ownDataProvider+ version so--dataoverlays are scored against the same catalog the listing enumerates (not the process-global embedded catalog). It follows the sibling facade guard pattern and deliberately omits the per-op timeout sincehealth.Computeapplies its ownHealthComputeTimeout.STATUS(rolled-up verdict) andCOVERAGE(compact per-phase named-check summary,R:n D:n P:n C:n); non-leaf overlays render-.healthblock (per-dimension status map + fulldeclared_coverage), omitted for non-leaf overlays. The embeddedCatalogEntrycarriesyaml:",inline"so feat(recipe): add aicr recipe list subcommand for catalog enumeration #1208's fields stay top-level —yaml.v3does not auto-inline anonymous struct fields the wayencoding/jsondoes.constraints_wellformeddimension (pkg/health: constraints_wellformed signal (parse-only, hermetic) #1227 not yet merged) doesn't break the status column.Testing
Coverage:
pkg/cli64.0% → 67.7%,pkg/client/v175.1% → 75.3% (newComputeHealth81.2%). New tests cover the table/json/yaml formats, the non-leaf-placeholder, filtered-CLI pass-through, the empty-result branch, and a YAML-inline regression guard.This change was reviewed by a multi-persona review pass (Go architecture, CLI/API-contract, test-quality), which caught and fixed a YAML serialization regression before push.
Risk Assessment
Rollout notes: Backwards compatible. The
healthblock is additive (omitted for non-leaf); existing json/yaml/table consumers see no field removals or renames.Checklist
make testwith-race)make lint)git commit -S)