feat(health): constraints_wellformed signal (parse-only, hermetic)#1301
feat(health): constraints_wellformed signal (parse-only, hermetic)#1301njhensley wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThis PR implements the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/design/009-recipe-health-tracking.md`:
- Around line 208-218: Update the ADR so its tense/state is consistent: either
change the ADR status block from "Proposed — design-only; not implemented." to
reflect that behavior is "shipped" or reword the "Compute budget." paragraph
(the sentence beginning "As shipped, V1's `constraints_wellformed`...") to
clearly mark it as intended/design-only; for example prepend "As designed for
V1," or "Intended behavior (V1):" and keep `constraints_wellformed` referenced
as design intent; ensure the ADR status text and the `Compute budget.` paragraph
use matching language so readers can tell current behavior from future intent.
🪄 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: 597a3c00-741a-4217-89be-4baabe14662c
📒 Files selected for processing (5)
docs/design/009-recipe-health-tracking.mdpkg/health/doc.gopkg/health/health.gopkg/health/health_test.gopkg/recipe/builder.go
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 (1)
docs/design/009-recipe-health-tracking.md (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate ADR status to reflect implementation in this PR.
Line 5 marks this ADR as "Proposed — 2026-05-30 (design-only; not implemented)," but this PR implements the
constraints_wellformeddimension (per the PR summary: "Adds a third graded health signal,constraints_wellformed, topkg/health"). The body language at lines 208-220 and 176 describes the feature as current behavior using present tense ("is specified," "is therefore").Update the status block to reflect that V1 structural signals (including
constraints_wellformed) are now implemented, while the deferred validation axis remains design intent. This ensures readers can distinguish current behavior from future intent.📝 Suggested status block update
-**Proposed** — 2026-05-30 (design-only; not implemented). +**Accepted** — 2026-05-30. V1 structural signals (§2) implemented; validation axis (§3) deferred.Then update Line 7-12 to clarify what is implemented vs. deferred:
This Architecture Decision Record (ADR) specifies the V1 contract for computing, recording, and publishing -recipe health across every supported criteria combination. The package -layout, CLI surface, generator, and CI workflow described below are intent, -not current behavior. Implementation will be tracked under a follow-on epic -once this ADR is accepted. +recipe health across every supported criteria combination. V1 structural +soundness signals (§2) are implemented; the validation-posture axis (§3), +generator (§5), and publication workflow (§5) remain design intent pending +first evidence attestation.Based on learnings, documentation should "Clearly distinguish current behavior from future intent in documentation."
🤖 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/design/009-recipe-health-tracking.md` at line 5, Update the ADR status block to mark the V1 structural health signals as implemented: change the "Proposed — 2026-05-30" status to indicate "Implemented" (or "Implemented — 2026-05-30") and explicitly state that the new constraints_wellformed signal (implemented in pkg/health) and the other V1 structural signals are now live; also clarify that the separate "validation" axis remains a design intent/deferred item so readers can distinguish current behavior from future work. Reference the constraints_wellformed signal and pkg/health in the status text and adjust surrounding present-tense wording to consistently reflect implemented behavior versus deferred intent.Source: Coding guidelines
🤖 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/design/009-recipe-health-tracking.md`:
- Line 5: Update the ADR status block to mark the V1 structural health signals
as implemented: change the "Proposed — 2026-05-30" status to indicate
"Implemented" (or "Implemented — 2026-05-30") and explicitly state that the new
constraints_wellformed signal (implemented in pkg/health) and the other V1
structural signals are now live; also clarify that the separate "validation"
axis remains a design intent/deferred item so readers can distinguish current
behavior from future work. Reference the constraints_wellformed signal and
pkg/health in the status text and adjust surrounding present-tense wording to
consistently reflect implemented behavior versus deferred intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fa789684-6b08-44e0-b9cf-7e1e964caa22
📒 Files selected for processing (1)
docs/design/009-recipe-health-tracking.md
1 participant
Summary
Adds the third graded health signal,
constraints_wellformed, topkg/health. It grades every leaf recipe on parse-only constraint well-formedness — fully hermetic (no snapshot, no cluster, no network).Motivation / Context
ADR-009 (Recipe Health Tracking, V1) calls for a graded
constraints_wellformedsignal. The ADR's original phrasing ("inline replay under--no-clusteris clean") is not achievable offline: the only evaluator (constraints.Evaluate) extracts a snapshot value before parsing and rejects a nil snapshot, so with no snapshot there is no clean replay to run. The V1 signal is therefore parse-only well-formedness; snapshot-dependent constraint replay is explicitly deferred to the validation/evidence axis (coverage_declared_vs_run).Fixes: #1227
Related: #1224 (epic), #1225 / #1226 (deps), #1228 / #1229 (consumers)
Type of Change
Component(s) Affected
pkg/recipe)docs/,examples/)pkg/health(structural health scoring)Implementation Notes
fail): for every mergedConstraint,classifyConstraintsWellformedcalls the exported, snapshot-free parsers directly —constraints.ParseConstraintPath(c.Name)andconstraints.ParseConstraintExpression(c.Value). Any parse failure isfail, naming the offending constraint and parser error inDetail. A malformed constraint is never a silentpass.warn): the leaf is resolved throughBuildFromCriteriaWithEvaluatorwith an injected satisfied-stub evaluator (satisfiedEvaluator) so the constraint-aware path executes with no cluster data; any resultingConstraintWarnings/ExcludedOverlayssurface aswarn. NOTE: those fields are only populated on a non-passing verdict, so under the satisfied stub the warn branch does not fire today — composition problems surface instead as resolve errors. The branch is the correct reading of a resolved result, is forward-compatible with a future evaluator wired into this path, and is unit-tested via direct injection. This is documented honestly in the code/doc comments.computeCombonow resolves once viaBuildFromCriteriaWithEvaluator(satisfiedEvaluator). With every constraint satisfied this merges exactly the overlaysBuildFromCriteriawould (no cluster-dependent exclusions), so the existingresolves/chart_pinnedgrades are unchanged while the result additionally carries the merged constraints and any warnings the new signal reads.pkg/validatorimport — acyclic integration ispkg/health → pkg/constraints → pkg/recipe.pkg/recipe/builder.godoc comment that referenced a non-existentvalidator.EvaluateConstraint; the real mechanism isconstraints.Evaluate(seepkg/client/v1). Flagged in the issue.pkg/health/doc.goand synced the now-inaccurate ADR-009 spec-table row + compute-budget premise (the ADR still described the superseded--no-clusterreplay design).This change went through a four-persona agent review (architect/correctness, determinism, test/QA, docs/consistency); their convergent finding — that the
warnbranch is inert under the satisfied stub while the comments overclaimed it — was addressed by making every comment honest and keeping the (correct, forward-compatible, unit-tested) wiring.Testing
pkg/healthtests pass under-race(new: table-drivenTestClassifyConstraintsWellformed, end-to-endTestComputeConstraintsWellformedFailThroughBuilder, four-signalTestComputeAllDimensionsCoexistAndRollUpClean).pkg/health: 97.6% → 98.0% (+0.4%); all new functions at 100%.Risk Assessment
Rollout notes: N/A — no user-facing surface changes; hermetic and deterministic.
Checklist
make testwith-race)make lint)git commit -S)