fix(skills): give root-cause and sensitive-state checks one stage-neutral owner - #8575
fix(skills): give root-cause and sensitive-state checks one stage-neutral owner#8575laitingsheng wants to merge 8 commits into
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
… routing Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…ment Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughThe PR adds shared root-cause and sensitive-workflow state checks. Planning, implementation, and PR follow-up workflows now use this guidance. Policy tests validate references, reporting fields, sibling-path checks, state coverage, and evidence handling. ChangesWorkflow check alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PlanningSkill
participant ImplementationSkill
participant PRFollowUp
participant SharedChecks
PlanningSkill->>SharedChecks: Apply root-cause and sensitive-workflow checks
PlanningSkill->>ImplementationSkill: Pass operation, failure class, paths, and states
ImplementationSkill->>SharedChecks: Re-check paths and workflow states
ImplementationSkill->>PRFollowUp: Provide completed-diff outcomes
PRFollowUp->>SharedChecks: Apply checks before grouped fixes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 14cb50e in the TypeScript / code-coverage/cliThe overall coverage in commit 14cb50e in the Show a code coverage summary of the most impacted files.
Updated |
laitingsheng
added
bug-fix
![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 @.agents/skills/_shared/root-cause-and-state-checks.md:
- Around line 22-39: Update the Sensitive-Workflow State Matrix guidance to
require an explicit authorization-boundary check for public or external writes.
Either add the check and required evidence to this matrix, or explicitly
delegate it to the Security Rubric and require that evidence there.
In @.agents/skills/nemoclaw-contributor-implement-issue/SKILL.md:
- Around line 80-83: Update
.agents/skills/nemoclaw-contributor-implement-issue/SKILL.md lines 80-83 to
require recording the explicit operation and failure class before editing,
alongside sibling paths and sensitive-workflow outcomes. Update lines 108-111 to
require rechecking and reporting both fields during self-review. Update
.agents/skills/_shared/pr-follow-up.md lines 93-97 to require recording the
operation and failure class for each finding group before applying fixes.
In @.agents/skills/nemoclaw-contributor-plan-issue/SKILL.md:
- Around line 130-132: Expand the sensitive-workflow evidence requirements in
the plan report fields at
.agents/skills/nemoclaw-contributor-plan-issue/SKILL.md:130-132 to capture
credential location, access, lifetime, removal, and separate result/action
classifications for every failure cell. Apply the same evidence requirements to
the implementation handoff fields at
.agents/skills/nemoclaw-contributor-implement-issue/SKILL.md:135-139, preserving
the shared lifecycle-report contract.
🪄 Autofix
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: 52fa9ea5-82a7-47f5-9000-eff6294dd98a
📒 Files selected for processing (6)
.agents/skills/_shared/pr-follow-up.md.agents/skills/_shared/root-cause-and-state-checks.md.agents/skills/nemoclaw-contributor-implement-issue/SKILL.md.agents/skills/nemoclaw-contributor-plan-issue/SKILL.mdtest/maintainer-skills-policy.test.tstest/skills-frontmatter.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
.agents/skills/nemoclaw-contributor-plan-issue/SKILL.md:143 and .agents/skills/nemoclaw-contributor-implement-issue/SKILL.md:139 require separate results and actions only for applicable phases, while the shared contract requires them for each failure cell. An agent can therefore collapse command failure, transport ambiguity, and verification failure into one phase-level result and omit security or recovery actions. Require a separate result and action for every applicable failure cell in both report templates. Update test/skills-frontmatter.test.ts to assert that exact contract; its current generic separate result and action assertion does not prevent this regression.
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 `@test/skills-frontmatter.test.ts`:
- Line 143: Expand the assertion in the checks test to reject all prohibited
command forms, including npm test, pnpm test, and npx vitest, alongside the
existing npm run, src/test paths, and workflow references. Keep the no-inventory
guard case-insensitive and anchored to command boundaries so unrelated text is
not rejected.
🪄 Autofix
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: d6ee3624-13ca-48cd-abff-2db5c7e4530e
📒 Files selected for processing (6)
.agents/skills/_shared/pr-follow-up.md.agents/skills/_shared/root-cause-and-state-checks.md.agents/skills/nemoclaw-contributor-implement-issue/SKILL.md.agents/skills/nemoclaw-contributor-plan-issue/SKILL.mdtest/maintainer-skills-policy.test.tstest/skills-frontmatter.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/maintainer-skills-policy.test.ts
- .agents/skills/_shared/root-cause-and-state-checks.md
- .agents/skills/nemoclaw-contributor-implement-issue/SKILL.md
- .agents/skills/nemoclaw-contributor-plan-issue/SKILL.md
- .agents/skills/_shared/pr-follow-up.md
| expect(checks).toContain("Assume a possible write and re-read external state"); | ||
| expect(checks).toContain("owns the authentication and authorization category"); | ||
| expect(checks).toContain("security-rubric.md"); | ||
| expect(checks).not.toMatch(/\bsrc\/|\btest\/|npm run|\.github\/workflows/u); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Expand the no-inventory guard for all command forms.
Line 143 rejects npm run only. Content such as npm test, pnpm test, or npx vitest passes this test even though the PR objective prohibits current commands in shared guidance. Add checks for the command forms that the shared document must exclude.
🤖 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 `@test/skills-frontmatter.test.ts` at line 143, Expand the assertion in the
checks test to reject all prohibited command forms, including npm test, pnpm
test, and npx vitest, alongside the existing npm run, src/test paths, and
workflow references. Keep the no-inventory guard case-insensitive and anchored
to command boundaries so unrelated text is not rejected.
2 participants
Summary
.agents/skills/_shared/pr-follow-up.mdwas the only stage owner of the root-cause sibling-path record and the sensitive-workflow state matrix, so planning and implementation could complete without either check. A stage-neutral resource,.agents/skills/_shared/root-cause-and-state-checks.md, now states both checks for plan-issue, implement-issue, and pr-follow-up. PR follow-up keeps its stage-specific mechanics..agents/skills/_shared/documentation-writing-review.mdkeeps a separate review-scope statement of adjacent-path inspection and sensitive-boundary dimensions, and this PR does not change it.Related Issue
Fixes #8555
Parent epic: #8240
Changes
.agents/skills/_shared/root-cause-and-state-checks.mdas the stage-neutral owner that plan-issue, implement-issue, and pr-follow-up all consume. The matrix rows, columns, and cells move verbatim; itsAuthoritysection keeps path, command, selector, test, and architecture details in the checkout rather than in the resource. It delegates the authentication and authorization category to the Security Rubric and requires that rubric's positive and negative evidence for a public or external write, rather than restating an authorization check in the matrix._shared/pr-follow-up.mdconsumes the shared resource instead of stating either check, and keeps review collection, pagination, head stability, finding grouping and disposition, evidence cleanup, and the push gate. Classification now precedes the shared checks in the review cycle, because the checks act on the valid groups that classification produces, and each group records the operation and failure class it belongs to.nemoclaw-contributor-plan-issuenames the operation and failure class during discovery and records the sibling paths checked and the applicable sensitive-workflow states in its plan report. The report requires a separate result and action per phase, plus each credential's location, access, lifetime, and removal.nemoclaw-contributor-implement-issuenames the operation and failure class before editing, re-checks it with the sibling paths and state outcomes against the completed diff during self-review, reports each sibling path that still needs the same change, and carries the same credential and result/action evidence in its handoff.origin/mainto keep the branch current. The merge was clean; main touched only.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tsand.agents/skills/nemoclaw-maintainer-e2e/SKILL.md, neither of which this PR changes.Type of Change
Quality Gates
.agents/skills/, which is absent from thefileslist inpackage.jsonand outside the sparse checkout thatdocs/resources/agent-skills.mdxdocuments; that recipe coversnemoclaw-user-guideonly.Documentation Writer Review
no-docs-neededWRITING.mdand the controlled word list across six heads. No documentation update is required: the change is confined to.agents/skills/**andtest/**, and.agents/is absent from thefileslist inpackage.json, so the shared resource ships in no published package;docs/resources/agent-skills.mdxsparse-checks out only/.agents/skills/nemoclaw-user-guide/**plus/.claude/**, whose sole entry is theskillssymlink resolving back into that same sparse set; no page underdocs/orfern/references the moved checks, and no dangling reference to the removedpr-follow-up.mdheading remains. No command, configuration, default, workflow, or other user-visible surface changed. The relocated authorization sentence was verified against the Security Rubric's own ownership statement and its authentication and authorization category, and now claims only what that rubric claims for itself. Five blocking findings raised across the review rounds were all applied before this head, the last being the single-ownership wording in this description; no blocking finding remains. Reconciling the overlapping adjacent-path and sensitive-boundary text in_shared/documentation-writing-review.md, which predates this branch, is left to a focused follow-up.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project integration test/skills-frontmatter.test.ts test/maintainer-skills-policy.test.ts test/code-change-considerations.test.ts— 3 files, 64 passed.npm run checks:repositorypassed, including the test title style and test registration boundary checks.npx markdownlint-cli2reported 0 errors on the changed Markdown.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Process Improvements
Tests