refactor(skills): derive implementation guidance from source - #8159
Conversation
jyaunches
added
refactor
|
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:
📝 WalkthroughWalkthroughThe PR adds shared implementation-discovery guidance and rewrites contributor skills for messaging channels, dependency updates, Hermes updates, and documentation. It updates the skills catalog and tests to enforce current, discovery-based guidance. ChangesContributor guidance modernization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 4da0be8 in the TypeScript / code-coverage/cliThe overall coverage in commit 4da0be8 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.agents/skills/nemoclaw-contributor-onboard-messaging-channel/SKILL.md:
- Around line 16-18: Update the accepted scope decision guidance near “Confirm
that an accepted issue or design decision” to explicitly require runtime and
version compatibility expectations alongside supported channel, agent runtimes,
ownership, lifecycle, security, and validation boundaries before implementation.
Preserve the existing Community Solutions routing guidance for integrations
without established product scope.
🪄 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: 5e0e72eb-bdbd-4aec-bdb8-ae69bf1e2de8
📒 Files selected for processing (14)
.agents/skills/_shared/documentation-writing-review.md.agents/skills/_shared/implementation-discovery.md.agents/skills/nemoclaw-contributor-onboard-messaging-channel/SKILL.md.agents/skills/nemoclaw-contributor-update-dependencies/SKILL.md.agents/skills/nemoclaw-contributor-update-dependencies/references/contract-audit.md.agents/skills/nemoclaw-contributor-update-dependencies/references/release-ledger.md.agents/skills/nemoclaw-contributor-update-docs/SKILL.md.agents/skills/nemoclaw-contributor-update-hermes/SKILL.md.agents/skills/nemoclaw-contributor-update-hermes/references/hermes-contract-map.md.agents/skills/nemoclaw-skills-guide/SKILL.mdtest/dependency-upgrade-skill.test.tstest/hermes-upgrade-skill.test.tstest/maintainer-skills-policy.test.tstest/skills-frontmatter.test.ts
💤 Files with no reviewable changes (1)
- .agents/skills/nemoclaw-contributor-update-hermes/references/hermes-contract-map.md
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head 4314199. The source-first skill refactor keeps repository-owned source and tests authoritative, preserves contributor security and provenance gates, and updates the contract tests consistently. The final delta only strengthens durable gate assertions. I found no blocking correctness, security, compatibility, test, or regression defect. The macOS failure is unrelated to this skills-only diff: the runtime test observed an unverified runner process PID.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the current head (4662389), including the follow-up commit that replaces ambiguous guidance with repository-owned policy links and adds focused assertions. I found no blocking correctness, security, compatibility, or regression issue in the updated diff.
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 @.agents/skills/_shared/implementation-discovery.md:
- Around line 17-18: Update the behavior-verification guidance in
implementation-discovery.md to limit current source and tests to runtime
behavior. Treat current AGENTS.md and repository policy documents as
authoritative for contributor-process and documentation claims, while retaining
history, issues, PRs, and other documentation as rationale sources.
🪄 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: 0264c9f4-5ebb-47ba-bd57-d2bf4cf3f2dc
📒 Files selected for processing (3)
.agents/skills/_shared/implementation-discovery.mdtest/dependency-upgrade-skill.test.tstest/skills-frontmatter.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/dependency-upgrade-skill.test.ts
- test/skills-frontmatter.test.ts
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed current head bc14e7f after the new implementation-discovery rewrite. The added commit removes duplicated generic mechanics while retaining the repository-source-of-truth rule, active AGENTS guidance, product-scope and trust-boundary gates, current-source verification, and task-specific evidence requirements. The paired tests correctly enforce that smaller contract. I found no blocking correctness, security, compatibility, or regression issue. The two image-build failures are from newly surfaced npm audit findings in the unchanged MCP tool-discovery dependency tree on both architectures, not from this skill-only delta; remaining exact-head checks are still running.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e2e113f after the main merge. Every effective PR blob remains identical to the previously approved revision. The two newer Hermes base edits are intentionally superseded by the source-first workflow: the current skill requires contributors to derive wrapper, adapter, compatibility, security, and validation surfaces from current source rather than a stale maintained map. Focused Hermes and policy contract validation passes 52 of 52 tests. The broader local failures are confined to an unchanged release-ledger helper running under the host Python 3.9, not this PR delta. No blocking defect found.
<!-- markdownlint-disable MD041 --> ## Summary Remove the dedicated Hermes implementation skill and route Hermes release work through the general dependency-upgrade workflow. Keep only the Hermes-specific CalVer collection and base-image publication gates in a conditional reference so implementation details continue to come from source and tests. This PR is stacked on #8159 and should be reviewed and merged after it. Once #8159 merges, this PR can target `main` without carrying duplicate skill-refactor changes. ## Changes - Remove the dedicated Hermes skill, agent metadata, catalog entry, and obsolete skill test while preserving narrow update, review, and publication triggers in the dependency skill. - Move the Hermes CalVer helper beneath the dependency skill and use its ordered release endpoints as adjacent audit boundaries only when the generic collector does not cover the selected range. - Keep concise, process-level Hermes base-image publication guidance without maintaining code-derived path or contract inventories. - Bring the moved helper under the parent collector's private-output contract with descriptor-anchored mode-`0600` writes, no replacement or symlink following, unsafe-directory rejection, and failure cleanup. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: No user-facing product behavior changes. Contributor-agent routing is documented by the updated checked-in skills catalog, and the exact-head documentation review passed. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop independently reviewed exact head `89e75241b` with the repository's nine-category security checklist. All categories passed with no remaining findings; evidence covers descriptor anchoring, ownership and mode checks, files and symlinks, hostile directories, permissive umask, and injected fsync failure cleanup. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed the full PR diff and final descriptor-anchored output-security follow-up against current writing and documentation rules. Affected tests passed 103/103; final focused tests passed 10/10; repository checks, Biome, git diff checks, forward testing, pre-commit, commit-msg, skill YAML, markdownlint, and gitleaks passed. Independent nine-category security review passed. No user-facing `docs/` change is needed because this changes contributor-agent routing, documented in the skills catalog. - Agent: Codex Desktop <!-- docs-review-head-sha: 89e7524 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: Affected integration selection passed 103/103; final variant and helper tests passed 10/10. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this narrow agent-skill and standalone-helper refactor; `npm run checks:repository` passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> --------- Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if created and not complete: | ||
| try: | ||
| os.unlink(output.name, dir_fd=directory) | ||
| except FileNotFoundError: |
5 participants
Summary
Refactor the checked-in contributor implementation skills so they describe durable process and priorities while deriving implementation details from the current checkout. This reduces the chance that skill guidance silently drifts from the code it is meant to help change.
Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedDGX Station Hardware Evidence
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 unavailabletest/dependency-upgrade-skill-security.test.tspassed 18/18 after the CI correction;test/skills-frontmatter.test.tspassed 30/30 after the messaging correction; the durable-gate follow-up passed 35/35 focused tests; the authority-link follow-up passed 49/49 focused tests; the final implementation-guidance simplification passed 63/63 focused tests and a clean-context forward test.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; this refactor does not change the runtime or test harness.npm run checks:repositorypassed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Documentation
Tests