Skip to content

fix(maintainer): keep advisor checks advisory - #8563

Merged
cv merged 6 commits into
mainfrom
codex/advisor-check-advisory
Aug 7, 2026
Merged

fix(maintainer): keep advisor checks advisory#8563
cv merged 6 commits into
mainfrom
codex/advisor-check-advisory

Conversation

@apurvvkumaria

@apurvvkumaria apurvvkumaria commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

The maintainer gate checker currently treats a failed PR Review / Advisor analysis job as merge-blocking even though the repository policy makes advisor findings non-authoritative. This change treats only authenticated Terra and Nemotron analysis jobs as advisory; missing or contradictory evidence, publication jobs, future advisor jobs, and required-context substitution remain blocking.

Changes

  • Authenticate the advisory classification with the canonical GitHub Actions run and job URL, repository, workflow path, event, current PR branch and base revisions, and matching job identity and state.
  • Restrict advisory treatment to the two current analysis jobs. Keep the publication job and unrecognized future jobs merge-relevant.
  • Re-read run metadata after enumerating jobs so a changing run fails closed.
  • Add regression coverage for spoofed display names, malformed or incomplete URLs, workflow and event mismatches, stale PR association, missing run or job evidence, changing runs, hostile origins and repositories, check/job state mismatches, publication and future jobs, and required-context substitution.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: This aligns the internal maintainer checker with the existing MERGE-GATE.md policy and changes no user-facing command, configuration, or documentation surface.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: An independent nine-category review of the current branch revision 286757a81 passed. Advisory treatment requires authenticated run, job, workflow, event, repository, and PR association evidence; missing or contradictory evidence continues to block.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: Existing maintainer guidance already states that PR Review / Advisor input does not determine allPass; this change makes the checker enforce that policy. The test-only feedback follow-up adds no user-facing command, configuration, or support-surface change.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: The directly changed gate-evidence suite passes 27/27; npm run check:diff, CLI typecheck, and Biome pass on the current branch revision.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Not run; the two-file checker change is covered by focused tests and npm run validate:pr.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Summary by CodeRabbit

  • New Features

    • Added support for recognizing and authenticating PR Review Advisor workflow checks.
    • Valid advisor checks are now correctly associated with the relevant pull request and workflow run.
  • Bug Fixes

    • Advisory checks that are pending or unsuccessful no longer incorrectly block merging.
    • Invalid, mismatched, or incomplete check results remain merge-relevant.
    • Advisor jobs cannot replace required status checks.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria apurvvkumaria self-assigned this Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 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: c9c03087-6dd6-4e15-a80f-ffa7ed0d0cc7

📥 Commits

Reviewing files that changed from the base of the PR and between ab2fc32 and 286757a.

📒 Files selected for processing (1)
  • test/skills/check-gates-actions-evidence.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/skills/check-gates-actions-evidence.test.ts

📝 Walkthrough

Walkthrough

The gate checker authenticates PR Review Advisor checks against GitHub Actions evidence. Valid advisor checks remain advisory. Invalid or incomplete checks remain merge-relevant. Tests cover both outcomes and required-check substitution.

Changes

PR Review Advisor gate validation

Layer / File(s) Summary
Authenticate advisor workflow evidence
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts, test/skills/check-gates-actions-evidence.test.ts
The checker validates approved advisor jobs, URLs, workflows, events, pull requests, runs, statuses, and conclusions. Test helpers construct configurable advisor evidence.
Apply advisory classification to gate rollups
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts, test/skills/check-gates-actions-evidence.test.ts
Authenticated advisor checks are excluded from merge-relevant processing. Invalid evidence remains merge-relevant. Tests cover failed and pending checks and prevent advisor jobs from satisfying the required checks context.

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

Sequence Diagram(s)

sequenceDiagram
  participant PullRequest
  participant CheckGates
  participant GitHubActions
  PullRequest->>CheckGates: provide status checks
  CheckGates->>GitHubActions: validate advisor workflow and job evidence
  GitHubActions-->>CheckGates: return matching run and job metadata
  CheckGates-->>PullRequest: produce merge-relevant gate result
Loading

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: keeping recognized advisor checks advisory in the maintainer gate checker.
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 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/advisor-check-advisory

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

@github-code-quality

github-code-quality Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 286757a in the codex/advisor-check-... branch remains at 96%, unchanged from commit 47542dd in the main branch.


Updated August 07, 2026 19:02 UTC

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

2 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • justified — merge-relevant at test/skills/check-gates-actions-evidence.test.ts:214: Keep `merge-relevant` for this merge-gate classification because it names the behavioral contrast.
  • justified — advisor-like at test/skills/check-gates-actions-evidence.test.ts:214: Keep `advisor-like`; the modifier prevents display identity alone from implying advisory treatment.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

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.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

CI follow-up: cli-test-shards (6) initially timed out in two untouched tests. Both tests passed in the focused local rerun (32/32), and the failed-job rerun has now completed successfully. All required CI, security, documentation, DCO, and aggregate gates are green. The remaining Nemotron advisor-analysis failure is advisory and is not a branch test failure. This PR is awaiting independent human review.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

PRA-1 is addressed in the current branch revision.

The gate-evidence table now covers both independently mismatched states while keeping the advisor-like check merge-relevant:

  • CheckRun IN_PROGRESS with no conclusion versus the authenticated REST job QUEUED with no conclusion.
  • CheckRun FAILURE versus the authenticated REST job SUCCESS.

Both cases assert allPass: false and a failed CI gate. The directly changed suite passes 25/25; npm run check:diff, CLI typecheck, Biome, DCO, and commit verification also pass. The follow-up changes tests only.

@apurvvkumaria

apurvvkumaria commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Security review — refreshed

Verdict: PASS. Independent re-review found no security findings in the current branch revision.

Category Result Evidence
Secrets and credentials PASS No credential, token, or secret-bearing data was added; the repository secret scan passes.
Input validation PASS Classification requires HTTPS github.com, no user information or port, this repository's seven-segment run/job path, positive numeric IDs, and server-fetched matching state.
Authentication and authorization PASS The checker authenticates the run, job, workflow path, event, current pull request association, branch revision, and base revision. Missing or contradictory evidence fails closed.
Dependencies PASS No dependency or lockfile changes.
Errors and logging PASS Malformed, hostile-origin, cross-repository, missing, changing, or mismatched evidence remains merge-relevant; no sensitive values are logged.
Cryptography and data protection PASS No cryptographic or protected-data behavior changed.
Configuration and security defaults PASS Only the two current analysis jobs are advisory. Publication jobs, future jobs, required-context substitution, and unauthenticated lookalikes remain blocking.
Security testing PASS Twenty-seven focused cases cover valid failed/pending lanes plus hostile origin, another repository, malformed identity, path/event/association/state mismatches, missing evidence, and required-context impersonation.
Holistic posture PASS Latest-attempt REST evidence, stable run re-read, state agreement, and the final rollup preserve fail-closed and TOCTOU-resistant behavior without weakening required CI or review gates.

Verification: focused suite 27/27, npm run check:diff, CLI typecheck, Biome, test-title policy, and diff checks pass. All six PR commits are GitHub Verified with DCO sign-offs. Documentation review remains no-docs-needed; existing maintainer policy already defines Advisor output as review input rather than merge authority.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

The current PRA-1 warning is addressed in the current branch revision.

The advisor-like negative-case table now includes two otherwise valid checks whose job URLs point to:

  • https://attacker.example/NVIDIA/NemoClaw/...
  • https://github.com/NVIDIA/OtherRepo/...

Both cases retain the failed check in merge evaluation and assert allPass: false with a failed CI gate. The focused suite passes 27/27, full differential gates pass, and an independent re-review confirmed that each case reaches the intended fail-closed path rather than failing on unrelated fixture metadata. All six PR commits are GitHub Verified and DCO-signed.

@apurvvkumaria

apurvvkumaria commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI classification for the current branch revision:

  • Attempt 1: CLI shard 6 failed because the unrelated shields policy-transition setup exceeded the 10-second hook limit under full-shard load.
  • Attempt 2: that test passed, while a different unrelated test, host-local-vllm-selection.test.ts, exceeded its 5-second Vitest limit. This is the same transient timeout signature previously seen on fix(messaging): let the operator choose the Hermes WhatsApp mode #8423.
  • This PR changes only the maintainer gate checker and its focused tests; it does not touch either failing implementation, harness, or test.
  • Focused local validation passes: policy-transition 29 of 29 tests, and host-local-vLLM selection 3 of 3 tests.
  • I reran only the failed workflow jobs again because the non-repeating failures isolate this to runner contention rather than the PR.

The third attempt is being monitored. No PR code change is indicated by either failure.

@cv
cv merged commit c886cdb into main Aug 7, 2026
70 of 79 checks passed
@cv
cv deleted the codex/advisor-check-advisory branch August 7, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants