Skip to content

refactor(advisories): add structured registry foundation - #6945

Merged
apurvvkumaria merged 3 commits into
NVIDIA:mainfrom
HOYALIM:codex/issue-3213-advisory-foundation
Jul 18, 2026
Merged

apurvvkumaria merged 3 commits into
NVIDIA:mainfrom
HOYALIM:codex/issue-3213-advisory-foundation

Conversation

@HOYALIM

@HOYALIM HOYALIM commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Introduce the phase-1 structured advisory foundation from #3213 without changing existing CLI behavior. The new module defines typed advisory/check contracts, an explicit immutable registry, a resume-aware runner, and console/JSON presentation with a shared blocking gate.

Related Issue

Part of #3213.

Changes

  • Add typed severity, phase, advisory, and advisory-check contracts.
  • Add an explicit registry and a runner that handles phase filtering, suppression, skip predicates, and resume-safe cache behavior.
  • Add deterministic console/JSON presentation and non-suppressible blocking enforcement.
  • Add 11 focused tests covering registry immutability, resume behavior, suppression, ordering, formatting, and blocking semantics.

Verification

  • npx vitest run --project cli src/lib/advisories/presenter.test.ts src/lib/advisories/registry.test.ts src/lib/advisories/runner.test.ts
  • npm run build:cli
  • npm run typecheck
  • npm run docs:check-agent-variants
  • npm run check:diff
  • Every commit is DCO-signed and pushed with an SSH signature

Signed-off-by: Ho Lim subhoya@gmail.com

Summary by CodeRabbit

  • New Features
    • Introduced a structured advisory system with typed metadata (severity, phase, kind), including console and JSON presentation of findings.
    • Added an advisory runner that supports phase filtering, conditional skipping, optional resume caching for resume-safe results, and suppression of selected non-critical advisories.
    • Enforced blocking rules by collecting and failing on fatal/blocking advisories.
    • Added an advisory registry to define ordered, immutable advisory checks with validation for duplicate IDs.
  • Tests
    • Added coverage for deterministic output, blocking behavior, registry ordering/uniqueness, runner caching/suppression logic, and correctness checks.

Signed-off-by: Ho Lim <subhoya@gmail.com>
Copilot AI review requested due to automatic review settings July 15, 2026 15:12
@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7a32bbc1-7441-4f02-a67c-5e11cc5f37f6

📥 Commits

Reviewing files that changed from the base of the PR and between a8b37f7 and 6874d33.

📒 Files selected for processing (1)
  • src/lib/advisories/registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/advisories/registry.ts

📝 Walkthrough

Walkthrough

Adds shared advisory contracts, immutable registry validation, a runner supporting filtering, skipping, resume caching, suppression, and metadata checks, plus console/JSON presentation and blocking enforcement with Vitest coverage.

Changes

Advisory Pipeline

Layer / File(s) Summary
Advisory contracts and registry
src/lib/advisories/types.ts, src/lib/advisories/registry.ts, src/lib/advisories/registry.test.ts
Defines structured advisory and check types, validates unique check IDs, freezes registries, and tests order preservation, immutability, and duplicate rejection.
Check execution and resume flow
src/lib/advisories/runner.ts, src/lib/advisories/runner.test.ts
Runs checks by phase, applies skipping and suppression, reuses only resume-safe cached results, validates metadata, tracks execution state, and tests these behaviors.
Advisory formatting and blocking enforcement
src/lib/advisories/presenter.ts, src/lib/advisories/presenter.test.ts
Formats advisories as console or JSON output, classifies fatal and blocking findings, throws structured blocking errors, and tests deterministic output.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant runAdvisories
  participant AdvisoryCheck
  participant CachedResults
  Caller->>runAdvisories: provide checks, context, and options
  runAdvisories->>AdvisoryCheck: filter by phase and skipIf
  runAdvisories->>CachedResults: read resume-safe cached result
  CachedResults-->>runAdvisories: return cached advisory or null
  runAdvisories->>AdvisoryCheck: execute eligible check when cache is not reused
  AdvisoryCheck-->>runAdvisories: return advisory or null
  runAdvisories-->>Caller: return advisories, results, and execution IDs
Loading

Suggested labels: feature

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: a structured advisory registry foundation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized E2E selections match; severity counts match.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: None

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/lib/advisories/registry.ts`:
- Around line 18-19: Add a GitHub issue or pull-request reference alongside the
ADVISORY_CHECKS declaration in the registry, linking the unresolved advisory
registration migration work. Keep the existing registry initialization and
comment behavior unchanged.
🪄 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: CHILL

Plan: Enterprise

Run ID: 1fd35b93-35e9-4f56-95a2-a82b4502b276

📥 Commits

Reviewing files that changed from the base of the PR and between 88f2dd8 and a8b37f7.

📒 Files selected for processing (7)
  • src/lib/advisories/presenter.test.ts
  • src/lib/advisories/presenter.ts
  • src/lib/advisories/registry.test.ts
  • src/lib/advisories/registry.ts
  • src/lib/advisories/runner.test.ts
  • src/lib/advisories/runner.ts
  • src/lib/advisories/types.ts

Comment thread src/lib/advisories/registry.ts Outdated
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output refactor PR restructures code without intended behavior change labels Jul 15, 2026
@wscurran

Copy link
Copy Markdown
Contributor

✨ Thanks for the refactor. Adding a structured advisory registry with typed contracts, immutable registry, and resume-aware runner improves maintainability while preserving existing CLI behavior. Ready for maintainer review.


Related open issues:

Signed-off-by: Ho Lim <subhoya@gmail.com>
@apurvvkumaria apurvvkumaria self-assigned this Jul 18, 2026

@apurvvkumaria apurvvkumaria left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at exact head 6874d33 after independent correctness/security review. Phase-one registry contracts, ordered duplicate-safe registration, cache/suppression validation, presenter behavior, and fatal/blocking gates are fail-closed; there is no current CLI wiring or behavior change. Both commits are Verified and DCO-compliant; CodeRabbit is resolved. Non-blocking follow-up: add a malformed resume-cache metadata regression for runner.ts:83-89. The prior red E2E gate is a trusted-validation timeout on a stale base, not a test assertion failure; fresh exact-head/current-base validation is still required.

Co-authored-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria
apurvvkumaria merged commit f78b485 into NVIDIA:main Jul 18, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants