feat(recipe-health): deep-link Evidence to dashboard + link-check bot#1809
Open
njhensley wants to merge 1 commit into
Open
feat(recipe-health): deep-link Evidence to dashboard + link-check bot#1809njhensley wants to merge 1 commit into
njhensley wants to merge 1 commit into
Conversation
njhensley
added
theme/recipes
Contributor
|
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 (14)
📝 WalkthroughWalkthroughAdds canonical TestGrid dashboard links, embedded presence data, and live dashboard path extraction. Recipe health output now renders Evidence deep-links for present coordinates and Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 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/testgrid/testgrid_test.go`:
- Around line 65-78: Refactor TestPresenceHas into a table-driven test with
cases containing a recipe.Coordinate and expected presence result. Iterate over
the cases and compare p.Has for each coordinate against its want value,
preserving the existing coordinates and expectations.
In `@tools/testgrid-link-check/main.go`:
- Around line 183-188: Update the CheckRedirect callback to require both the
expected origin host and an HTTPS request scheme, rejecting HTTP downgrade
redirects with the existing invalid-request error. Add a test covering an
HTTPS-origin request redirected to an HTTP URL and verify that the redirect is
rejected.
In `@tools/testgrid-link-check/workflow_test.go`:
- Around line 65-68: Update the workflow permission assertions around
wf.Permissions to iterate over every top-level permission entry and fail
whenever any value is "write", rather than checking only the "contents" key.
Retain validation that contents is read if required by the existing test.
🪄 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: f13489ff-6b4f-47c3-ae8d-ca8df2f4c79a
📒 Files selected for processing (14)
.github/workflows/testgrid-link-check.yamldocs/user/recipe-health.mdpkg/testgrid/doc.gopkg/testgrid/presence.yamlpkg/testgrid/testgrid.gopkg/testgrid/testgrid_test.gotools/health/main.gotools/health/main_test.gotools/health/markdown.gotools/testgrid-link-check/check.gotools/testgrid-link-check/check_test.gotools/testgrid-link-check/main.gotools/testgrid-link-check/main_test.gotools/testgrid-link-check/workflow_test.go
njhensley
force-pushed
the
feat/recipe-health-evidence-deeplink
branch
from
July 17, 2026 23:40
bb7777b to
ad76d03
Compare
Member
Author
|
Addressed all three CodeRabbit findings in the amended commit:
Tests + |
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/testgrid/testgrid.go`:
- Around line 60-89: Update LoadPresence to decode presenceManifest with a YAML
decoder configured with KnownFields(true), rejecting unknown manifest keys.
After decoding, validate that doc.Coordinates is present and non-empty,
returning an error before path processing when absent or empty, and add
regression coverage for both cases.
In `@tools/testgrid-link-check/main_test.go`:
- Around line 86-107: Convert the specified multi-case tests to table-driven
subtests: in tools/testgrid-link-check/main_test.go ranges 86-107, 109-128, and
177-201, cover the decoding, file-input, and HTTP-response cases respectively;
in tools/testgrid-link-check/check_test.go range 113-141, cover clean and
dead-link reports. Preserve each test’s existing inputs, expected outcomes, and
relevant assertions while using descriptive case names.
🪄 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: 133d94fe-8d16-4969-b00c-edcd144fe402
📒 Files selected for processing (14)
.github/workflows/testgrid-link-check.yamldocs/user/recipe-health.mdpkg/testgrid/doc.gopkg/testgrid/presence.yamlpkg/testgrid/testgrid.gopkg/testgrid/testgrid_test.gotools/health/main.gotools/health/main_test.gotools/health/markdown.gotools/testgrid-link-check/check.gotools/testgrid-link-check/check_test.gotools/testgrid-link-check/main.gotools/testgrid-link-check/main_test.gotools/testgrid-link-check/workflow_test.go
Member
Author
|
Second round addressed:
pkg/testgrid coverage now 100%; lint clean. |
njhensley
force-pushed
the
feat/recipe-health-evidence-deeplink
branch
from
July 17, 2026 23:49
ad76d03 to
a8c5ed7
Compare
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/testgrid/testgrid.go`:
- Around line 87-97: Update parsePresence in pkg/testgrid/testgrid.go to decode
once more after the first document and require io.EOF, returning the existing
parse error for any trailing YAML document or malformed content. Add a
multi-document regression case in pkg/testgrid/testgrid_test.go covering a valid
manifest followed by extra content.
In `@tools/testgrid-link-check/workflow_test.go`:
- Around line 100-115: The TestWorkflowRunsLinkCheckToStepSummary assertion must
verify that the tools/testgrid-link-check invocation passes $GITHUB_STEP_SUMMARY
specifically as the value for -report-out, rather than merely checking both
strings independently. Update the step.Run validation in
TestWorkflowRunsLinkCheckToStepSummary to match that argument pairing while
preserving the existing invocation check.
🪄 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: 45dcbfef-8ce2-40a6-8fe2-52ecea3bff36
📒 Files selected for processing (14)
.github/workflows/testgrid-link-check.yamldocs/user/recipe-health.mdpkg/testgrid/doc.gopkg/testgrid/presence.yamlpkg/testgrid/testgrid.gopkg/testgrid/testgrid_test.gotools/health/main.gotools/health/main_test.gotools/health/markdown.gotools/testgrid-link-check/check.gotools/testgrid-link-check/check_test.gotools/testgrid-link-check/main.gotools/testgrid-link-check/main_test.gotools/testgrid-link-check/workflow_test.go
RQ1 (NVIDIA#1283): render the recipe-health Evidence column as a deterministic, hermetic deep-link into the evidence dashboard (validation.aicr.run) for each recipe with a committed dashboard presence; keep an honest `pending` otherwise. Link construction reuses the shared pkg/recipe.CoordinateFor mapping and carries no k8s version, so links are stable across version rolls. New pkg/testgrid owns the dashboard origin, link scheme, and the committed presence manifest (presence.yaml). Evidence-column prose in docs/user/recipe-health.md is rewritten to explain the link, the author-asserted-provenance caveat, and its coexistence with ADR-009's deferred in-cell freshness state. RQ2 (NVIDIA#1284): add tools/testgrid-link-check and a weekly, warning-only workflow (clone of bom-refresh minus the PR step) that fetches the live dashboard data with an origin-pinned, redirect-hardened, bounded HTTP client and reports any linked coordinate the dashboard no longer serves. Never blocks merges, never edits the doc; a coordinate not yet linked is expected, not a warning. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
Member
Author
|
Third round (both Minor) addressed:
Tests + lint green; pkg/testgrid still 100%. |
njhensley
force-pushed
the
feat/recipe-health-evidence-deeplink
branch
from
July 17, 2026 23:56
a8c5ed7 to
a966aa9
Compare
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
1 participant
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
Turn the recipe-health matrix's
pendingEvidence column into deterministic deep-links into the evidence dashboard (validation.aicr.run) for recipes with a real dashboard presence (RQ1), and add a weekly, warning-only bot that verifies those links still resolve (RQ2).Motivation / Context
The Evidence column was a uniform literal
pending. RQ1 makes it a stable, hermetic deep-link to each recipe's dashboard coordinate; RQ2 keeps those links honest without ever blocking a merge.Fixes: #1283, #1284
Related: #1265 (parent), #1400 (dashboard)
Type of Change
Component(s) Affected
pkg/recipe) — consumes sharedCoordinateFor(no change to it)docs/)pkg/testgrid,tools/testgrid-link-check,tools/healthrendering, CI workflowImplementation Notes
Scope pivot (per the issue comments): links target the live GitHub Pages dashboard
validation.aicr.run, not the deferred live TestGrid. Because the dashboard is a hash-routed SPA (aGETon the/#/fragment can't reveal coordinate presence), the RQ2 bot checks presence in the publisheddata/index.json, not the fragment URL.pkg/testgrid(new): dashboard origin, hash-route link scheme (LinkForover the sharedpkg/recipe.CoordinateFor), the committed presence manifestpresence.yaml(the 6 UAT-covered coordinates today), andLivePathsto read present coordinates from a parsed index. The generator reads the embedded manifest, so link construction is fully offline/hermetic and byte-deterministic; the coordinate carries no k8s version, so links survive version rolls.tools/health: the Evidence cell renders[<path>](<link>)for a recipe whose resolved coordinate is present, else honestpending. No status/count token.docs/user/recipe-health.mdprose rewritten to explain the link, the author-asserted-provenance caveat, and coexistence with ADR-009's deferred in-cell freshness state; cross-links to the coverage matrix and dashboard.tools/testgrid-link-check+.github/workflows/testgrid-link-check.yaml: a clone ofbom-refreshminus the PR step — weekly cron + dispatch,contents: read, report to$GITHUB_STEP_SUMMARY, always exits 0. Bounded HTTP client pinned to the dashboard origin with off-origin redirects refused and aLimitReader-bounded body. Classifies each linked coordinateresolved/missing-but-present(warn: dead link) /not-yet-linked(expected, not a warning). A Go workflow-structure test enforces the read-only / no-create-pull-requestcontract.Testing
-race;golangci-lint,go vet,yamllint, andactionlintclean.pkg/testgrid94.6%,tools/testgrid-link-check87.9%,tools/health82.8% — all above the 75% floor, no new exported func at 0%.-index-filefixture and real fetch).Risk Assessment
pendingif the manifest is absent.Rollout notes: N/A. The bot is report-only and never blocks merges.
Checklist
make testwith-race)make lint)git commit -S)