chore(skills): add nodewright-cross-review multi-agent review skill - #399
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded the Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5).claude/skills/nodewright-cross-review/scripts/workflow.mjsFile contains syntax errors that prevent linting: Line 802: Illegal return statement outside of a function; Line 969: Illegal return statement outside of a function; Line 1025: Illegal return statement outside of a function; Line 1112: Illegal return statement outside of a function; Line 1115: Illegal return statement outside of a function; Line 1116: Illegal return statement outside of a function; Line 1121: Illegal return statement outside of a function; Line 1153: Illegal return statement outside of a function; Line 1324: Illegal return statement outside of a function Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.claude/skills/nodewright-cross-review/scripts/workflow.mjs:
- Around line 161-184: Remove the direct PINNED_READS interpolation from all
five prompt builders—claudeReviewPrompt, coderabbitReviewPrompt,
integrationPrompt, crossPrompt, and refutePrompt—when they also compose
NO_EXECUTION. Retain the single PINNED_READS reference embedded by NO_EXECUTION;
where a builder needs the recipe mentioned earlier, refer to that shared block
without duplicating its contents.
- Around line 854-862: Update the listed-path matching in flagUnchecked so a
filesChecked entry must be at least as specific as the cited finding or consumer
path. Retain only the direction that allows a listed absolute path to match a
cited repo-relative suffix, and remove the reverse atBoundary(path, p) match
that lets bare basenames satisfy paths anywhere in the repository.
- Around line 1029-1033: When candidates is empty, record that the cross-review
round was skipped instead of allowing the report to imply a full two-pass
review. Track a crossRoundRan boolean around the cross-review block in the
workflow, include it in the returned result object, and state its value in the
Phase 4 report header alongside the existing consensus information.
- Around line 636-641: Expose the empty change-list condition in integration
results by adding the current changes.length as changeListSize to both the
incomplete() result and the normal return object in integrationPrompt’s
surrounding integration lane flow. Preserve existing status behavior while
ensuring consumers can distinguish an integration lane that analyzed no changed
items.
In @.claude/skills/nodewright-cross-review/SKILL.md:
- Around line 141-162: Move the DIFFPATH, PRREF, and BASEREF echo statements in
the remote fetch block to immediately after those variables are assigned and
before git fetch, so cleanup identifiers are recorded if fetch fails. Apply the
same ordering change to the corresponding local block, preserving the existing
fetch, head validation, diff, and cleanup behavior.
- Around line 90-95: Update the gh pr view invocation in the argument-handling
instructions to validate the shape of $ARGUMENTS before execution and pass it as
one safely quoted shell token, while preserving the no-argument current-branch
behavior. Use the existing shell contract’s zsh pattern rules or an equivalent
grep -E validation, and keep the command from allowing whitespace or shell
metacharacters to be interpreted.
🪄 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: 82e08d22-6cbf-4f67-9f98-c2f7af3c6559
📒 Files selected for processing (2)
.claude/skills/nodewright-cross-review/SKILL.md.claude/skills/nodewright-cross-review/scripts/workflow.mjs
Pull request was converted to draft
735a66c to
0e56157
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.claude/skills/nodewright-cross-review/scripts/workflow.mjs:
- Line 318: Add explicit validation for the unconstrained prType alongside the
existing required-argument checks in the workflow entrypoint, using the
supported review-type values (code-change and design-doc) and rejecting any
unknown value before review behavior is selected. Remove reliance on the
REVIEW_FOCUS fallback at the affected focus-selection sites while preserving the
existing prType-based clause gates.
- Around line 985-988: Update the integration finding drop paths in intake() and
the integDropped handling around the existing Integration lane log to also
append each discarded finding, or an equivalent aggregated entry, to
openQuestions. Ensure the returned result and Phase 4 report expose these drops
so reviewerStatus.integration cannot remain an unqualified ok when findings
lacking consumerPath/consumerLine were discarded.
- Line 1204: Update the verifier lane’s agent call in the workflow around
refutePrompt(c) to explicitly set agentType to 'general-purpose', matching the
configuration used by the other workflow lanes while preserving the existing
label, phase, and schema.
In @.claude/skills/nodewright-cross-review/SKILL.md:
- Line 58: Reword the instruction near line 58 to preserve validation while
stating that, after validation, the accepted number or URL should be passed to
gh unchanged rather than rewritten. Tighten the URL validation near the
accepted-argument rules so only URLs identifying NVIDIA/nodewright are accepted,
matching the repository used by subsequent gh commands.
🪄 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: 2ad2136d-90a8-4321-9db5-ee9c3485f76d
📒 Files selected for processing (2)
.claude/skills/nodewright-cross-review/SKILL.md.claude/skills/nodewright-cross-review/scripts/workflow.mjs
0e56157 to
c66e69c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.claude/skills/nodewright-cross-review/SKILL.md (1)
58-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLine 58 still conflicts with the Phase 0 validation rule.
Lines 91-106 require the agent to validate the shape of
$ARGUMENTSbefore it reaches a shell, and lines 100-102 require rejecting a URL for another repository. Line 58 tells the agent not to write a parser for the argument. A reader can apply line 58 to the Phase 0 check and skip it. State the narrow point instead: validate the shape in Phase 0, then pass the value toghunchanged.🛡️ Proposed reword
-Do not write a parser for the argument; `gh` accepts a number or a URL directly. +Validate the argument's shape in Phase 0 first. Then pass the validated value to `gh` +unchanged: `gh` accepts a bare number or a pull URL directly, so no rewriting is needed.🤖 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 @.claude/skills/nodewright-cross-review/SKILL.md at line 58, Update the guidance near the argument-handling instruction to clarify that Phase 0 must validate the shape of $ARGUMENTS, including rejecting URLs for another repository, and that the validated value is then passed to gh unchanged; only prohibit parsing or transforming it after validation.
🤖 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.
Duplicate comments:
In @.claude/skills/nodewright-cross-review/SKILL.md:
- Line 58: Update the guidance near the argument-handling instruction to clarify
that Phase 0 must validate the shape of $ARGUMENTS, including rejecting URLs for
another repository, and that the validated value is then passed to gh unchanged;
only prohibit parsing or transforming it after validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 29196f80-649c-4137-bda8-c548995baa5e
📒 Files selected for processing (2)
.claude/skills/nodewright-cross-review/SKILL.md.claude/skills/nodewright-cross-review/scripts/workflow.mjs
c66e69c to
82bd997
Compare
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 @.claude/skills/nodewright-cross-review/SKILL.md:
- Around line 694-700: Update the Contested Issues table in the cross-review
workflow to add an Adjudication column, documenting that entries use evaluated
or raised-late values. Ensure the table also represents the workflow’s why field
so reports distinguish entries that were cross-evaluated from those raised late.
🪄 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: b54b43ba-b4a1-4475-a883-5a17008e1c42
📒 Files selected for processing (2)
.claude/skills/nodewright-cross-review/SKILL.md.claude/skills/nodewright-cross-review/scripts/workflow.mjs
82bd997 to
0b379c4
Compare
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 @.claude/skills/nodewright-cross-review/SKILL.md:
- Around line 75-85: Update the Codex pre-flight check in the gh/git loop and
codex-companion validation block to avoid passing the unmatched plugin glob
directly to ls under zsh. Test the globbed path set safely, then print the
existing actionable Codex plugin installation message when no matching companion
exists; preserve the fail-fast behavior and missing-tool reporting.
🪄 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: fa257804-e815-4dd0-9f72-8279ee403963
📒 Files selected for processing (2)
.claude/skills/nodewright-cross-review/SKILL.md.claude/skills/nodewright-cross-review/scripts/workflow.mjs
Adds a repo-local skill that reviews a change with three independent reviewers (Claude Code, Codex, CodeRabbit) plus a targeted integration impact analysis, cross-reviews them to a 2-of-3 consensus, and sends every confirmed finding to a fresh adversarial refuter before reporting it. Ported from the aicr-cross-review skill in NVIDIA/aicr, which is where the consensus mechanics and the CLI operational notes were worked out. Claims marked "measured" in SKILL.md were measured there, against the same CLIs on the same machine, and have not been re-measured here. What is new for this repo: - A repository-context block handed to every lane. It names the three components, tells reviewers the partial Skyhook to NodeWright rename is expected so a mixed vocabulary is never reported as a defect, makes Status/State/Stage confusion an explicit finding class, lists the vendored and generated trees to skip along with the hand-written zz.migration.*.go exception, and supplies the pathspec that keeps repo-wide greps out of operator/vendor and agent/vendor. - A code-change review focus covering the operator invariants this codebase actually depends on (level-triggered, idempotent, no reconciler-owned state, status subresource, finalizer cleanup, no sleep or poll in reconcile) and the cross-surface contracts that break silently (operator/config to chart, manifests/generate drift, stale mockery output, the CLI annotation contract, docs in the same PR). - design-doc, config-change and documentation-only focuses aimed at docs/designs, docs/plans, chart/ and operator/config. - An integration lane that searches this repo's real consumer surfaces, chainsaw fixtures and chart templates first. The skill also runs without a PR argument: it resolves the PR for the current branch, and falls back to reviewing the branch's committed work against its merge-base when there is no PR yet. Both modes share one code path because both pin a commit. Uncommitted work is never reviewed, since every lane reads a commit-pinned diff. The skill never executes the reviewed commit's code and never posts to a PR unless explicitly asked. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
0b379c4 to
4a623d7
Compare
|
@CodeRabbit full review. |
|
✅ Action performedFull review finished. |
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 @.claude/skills/nodewright-cross-review/scripts/workflow.mjs:
- Around line 53-61: Validate mode explicitly in the workflow setup instead of
defaulting every non-local value to pr. Update the reviewMode logic near the
required-argument checks to accept only local or pr and throw a clear
invalid-mode error for any other value, matching the existing prType validation
behavior.
- Around line 203-210: Update the CODEX_DISPATCH step 1 instructions to use the
established find-based companion lookup instead of the bare glob, matching the
pre-flight recipe used by SHELL_CONTRACT and SKILL.md. Require an explicit
unavailable outcome when no codex-companion.mjs is found, before step 2 attempts
to run node with an empty path.
🪄 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: cb3f1ad2-7954-4f68-b54f-1ffb293ab9c9
📒 Files selected for processing (2)
.claude/skills/nodewright-cross-review/SKILL.md.claude/skills/nodewright-cross-review/scripts/workflow.mjs
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
What
Adds
.claude/skills/nodewright-cross-review/, a repo-local skill that reviews a change with three independent reviewers (Claude Code, Codex, CodeRabbit) plus a targeted integration impact analysis, cross-reviews them to a 2-of-3 consensus, and sends every confirmed finding to a fresh adversarial refuter before reporting it.Two files, both new, nothing else touched:
SKILL.md(764 lines) is the phase-by-phase procedure: pre-flight, mode resolution, pinned setup, classification, theWorkflowcall, CI status, report, output.scripts/workflow.mjs(1281 lines) is the orchestration and the single source of truth for the consensus mechanics.Why this shape
Ported from the
aicr-cross-reviewskill inNVIDIA/aicr, which is where the consensus mechanics and the CLI operational notes were worked out. That provenance is stated inSKILL.mdrather than left implicit: claims marked "measured" were measured there, against the same CLIs on the same machine, and have not been re-measured against this repo. Reviewers should read them as inherited rather than as evidence gathered here.The parts written for this repository:
zz.migration.*.goexception; and supplies the pathspec that keeps repo-wide greps out ofoperator/vendorandagent/vendor. Most of that block removes work rather than adding it, which is why it is affordable even in the deliberately context-starved Codex lane.code-changefocus covering the operator invariants this codebase actually depends on: level-triggered, idempotent, no reconciler-owned state, status subresource, finalizer cleanup, no sleep or poll inside reconcile. Plus the cross-surface contracts that break silently:operator/config/tochart/,make manifests generatedrift, stale mockery output, the CLI annotation contract, and docs landing in the same PR.design-doc,config-changeanddocumentation-onlyfocuses aimed atdocs/designs/,docs/plans/,chart/andoperator/config/.operator/config/, chainsaw fixtures (which assert on exact annotation keys, statuses and stage names), the CLI, mocks, agent schemas.Self-review mode
The skill runs without an argument: it resolves the PR for the current branch, and falls back to reviewing the branch's committed work against its merge-base when there is no PR yet. Both modes share one code path because both pin a commit, so there is one consensus path to keep correct rather than two. Uncommitted work is never reviewed in either mode, since every lane reads a commit-pinned diff and a dirty tree would hand each lane a different view of the code.
Safety properties
maketargets or repository scripts. Suspectedmake manifests generatedrift is reported as a finding against the diff, not settled by running the generator./code-review, whose final step posts its result back to the PR, anddisallowed-tools: Skillcloses the path by which the CodeRabbit skill could auto-trigger and post..claude/skills/nodewright-cross-review/, since the scripts that would execute are the ones under review. This PR is therefore deliberately not self-reviewed; it wants ordinary human review, and the first real exercise should be an unrelated PR.New pattern, called out per the working rules
This is the first
.claude/skills/directory in the repo, so it introduces a convention rather than following one. The layout (<skill-name>/SKILL.mdplusscripts/) matches the Claude Code skill convention and the siblingaicrrepo. It sits under.claude/alongside the existingCLAUDE.md, which is already the agent-configuration root here.Two consequences worth knowing:
.claude/skills/is on the local sandbox write deny-list, so editing the skill needs a sandbox bypass even though running it does not. That is a property of the local sandbox profile, not of the skill, and it is documented inSKILL.md..mdnor.mjsis in thelicense_filesglobs inoperator/Makefile, so the license-header gate does not cover these files.workflow.mjscarries the Apache-2.0 header anyway;SKILL.mddoes not, matching how other Markdown in the repo is treated.Testing
Not behavior-changing for the operator, CLI, agent or chart, so no
docs/update applies and no test suite covers it. What was verified:workflow.mjsparses. It cannot be checked directly because the Workflow runtime permits top-levelreturn, so it was validated by wrapping the source in an async function and runningnode --check.NVIDIA/nodewright. The remainingskyhookstrings were each checked against.claude/CLAUDE.mdand are all still-true:agent/skyhook-agent/,/skyhook-package/config.json, the legacy read-onlyskyhook.nvidia.comCRD group, and the namespace.The end-to-end run is the obvious gap. It is genuinely expensive (four parallel agents, with the Codex lane budgeted up to about eighteen minutes) and, per the self-review guard above, cannot be exercised against this PR. Reviewers should treat the consensus mechanics as inherited-and-unexercised-here rather than as verified in this repo.