fix(skills): enforce stale verification controls - #8309
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 verify-stale workflow now validates exact release tags, treats issue reproducers as untrusted, requires approval before remote or GitHub effects, bounds local and Brev execution, redacts evidence, and routes installation or baseline failures to inconclusive outcomes. ChangesVerify-stale workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 5a8276a in the TypeScript / code-coverage/cliThe overall coverage in commit 5a8276a in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.agents/skills/nemoclaw-maintainer-verify-stale/reference/by-design.md:
- Around line 64-68: Escape issue-derived text before shell interpolation: in
by-design.md lines 64-68, assign the symbol to a variable and pass it to git
grep as a quoted -e argument; in reproduction-rubrics.md lines 53-66, assign the
redacted symptom phrase to a variable and pass it to grep -F -- with quotes.
Apply the same safe argument handling at both sites while preserving the
evidence-capture workflow.
In
@.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md:
- Around line 272-277: Update the performance-sample scoring flow around the
loop writing `${PERF_SIDE}-perf-exits.log` to read and validate all ten recorded
exit codes before calculating p50 or p90. Accept only ten expected successful
exit codes; if any sample fails or the count is incomplete, mark that side
inconclusive and ensure the final result is verify-inconclusive for both
baseline and latest rather than scoring recorded durations.
🪄 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: e6fa341b-342e-4175-82f9-4a4fa35c4f63
📒 Files selected for processing (9)
.agents/skills/nemoclaw-maintainer-verify-stale/SKILL.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/brev-provisioning.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/by-design.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/candidate-selection.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/environment-and-reproducer.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md.agents/skills/nemoclaw-maintainer-verify-stale/scripts/redact-evidence.pytest/maintainer-skills-policy.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
changed the title
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md (1)
358-400: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize shared activity-log writes.
Scheduled batch runs and manual runs can share
VERIFY_STALE_LOG_DIR. Per-issue appends and end-of-batch prepends are separate file operations. Concurrent runs can interleave entries or overwrite a session summary.Use a lock or atomic write protocol. Alternatively, write unique per-run files and merge them atomically.
🤖 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 @.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md around lines 358 - 400, Serialize all writes to the shared VERIFY_STALE_LOG_DIR/nemoclaw-verify-stale-log.md activity log, including per-issue appends and end-of-batch session-summary prepends. Update the verification logging flow to use a lock or atomic read-modify-write protocol so concurrent scheduled and manual runs cannot interleave entries or overwrite summaries; preserve the existing behavior when VERIFY_STALE_LOG_DIR is unset..agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md (2)
284-310:⚠️ Potential issue | 🟠 MajorReject failed performance samples before scoring.
The harness writes
${PERF_SIDE}-perf-exits.log, but the scoring path validates only ten numeric timing samples. It never validates the ten exit codes. Ten fast failures can therefore produce an in-SLA p50/p90 and a falsefixed-on-latestresult. Require ten expected successful exit codes before calculating percentiles. Otherwise selectverify-inconclusive.🤖 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 @.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md around lines 284 - 310, Update the percentile-scoring flow around PERF_SAMPLES to read and validate ${PERF_SIDE}-perf-exits.log before calculating P50_MS or P90_MS. Require exactly ten exit codes, all matching the reproducer’s expected successful code; on any missing, malformed, or non-success code, select verify-inconclusive and do not score performance. Preserve the existing numeric sample validation and percentile calculations for valid runs.
200-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a portable word-boundary check.
The drift command uses GNU-specific
\bsyntax withgrep -E. macOS BSD grep can skip all tool names. Use POSIX character classes or Python, then test the command with GNU and BSD grep.🤖 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 @.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md around lines 200 - 215, Update the tool-name extraction in the architecture-drift check to use a portable POSIX word-boundary approach instead of GNU-specific \b with grep -E. Preserve multi-word tool matching and macOS system Bash compatibility, and ensure the command works with both GNU and BSD grep.
🤖 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-maintainer-verify-stale/reference/candidate-selection.md:
- Around line 105-107: Update the batch-mode candidate processing instructions
to bind ISSUE_NUMBER to the current candidate before querying comments. Ensure
each verification plan uses that candidate’s issue number, avoiding
unset-variable failures and inherited values from other issues.
In
@.agents/skills/nemoclaw-maintainer-verify-stale/reference/environment-and-reproducer.md:
- Around line 250-255: Update the local version probe near NEMOCLAW_BIN and
LOCAL_VERSION to execute through the existing bounded Python helper instead of
invoking the binary directly. Preserve the captured version output, and treat
helper timeouts as an inconclusive local result so the skill cannot hang on a
broken nemoclaw binary.
- Around line 292-294: Update the local reproducer validator’s argument
validation to match the complete argv vector against explicit read-only command
forms, rather than checking only argv[1] or help flags anywhere. Reject extra
arguments and write flags, while preserving the permitted read-only commands and
help forms before execution.
In
@.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md:
- Around line 148-157: Update the resolved-version verification around
run_bounded and RESOLVED so its exit status is captured before any tail or
version parsing occurs. Store the command output in a temporary file, check the
run_bounded result—including timeouts—and set LATEST_INSTALL_FAILED=1
immediately on any non-zero status; only parse and compare RESOLVED_SEMVER when
the remote nemoclaw --version command succeeds.
In
@.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md:
- Around line 234-241: Update the evidence templates for baseline verification
and newest-release verification so fixed evidence is not emitted when baseline
installation or reproduction fails. In the scoring and logging guidance, make
the rows conditional or split them into separate templates: use
verify-inconclusive for baseline-stop paths, recording n/a and one redacted
failure line, while retaining newest-only results only for successful baseline
verification.
- Around line 26-27: Update both scoring rules in the scoring documentation so
the +50 newest-release signal requires the reviewed reproducer to match the
reported symptom on the exact $REPORTED_VERSION tag before confirming the
expected exit and absence of the symptom on the newest tag. Apply the same
reported-version symptom-match prerequisite to the +25 fallback rule, preserving
the existing mixed-results and newest-release conditions.
---
Outside diff comments:
In
@.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md:
- Around line 284-310: Update the percentile-scoring flow around PERF_SAMPLES to
read and validate ${PERF_SIDE}-perf-exits.log before calculating P50_MS or
P90_MS. Require exactly ten exit codes, all matching the reproducer’s expected
successful code; on any missing, malformed, or non-success code, select
verify-inconclusive and do not score performance. Preserve the existing numeric
sample validation and percentile calculations for valid runs.
- Around line 200-215: Update the tool-name extraction in the architecture-drift
check to use a portable POSIX word-boundary approach instead of GNU-specific \b
with grep -E. Preserve multi-word tool matching and macOS system Bash
compatibility, and ensure the command works with both GNU and BSD grep.
In
@.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md:
- Around line 358-400: Serialize all writes to the shared
VERIFY_STALE_LOG_DIR/nemoclaw-verify-stale-log.md activity log, including
per-issue appends and end-of-batch session-summary prepends. Update the
verification logging flow to use a lock or atomic read-modify-write protocol so
concurrent scheduled and manual runs cannot interleave entries or overwrite
summaries; preserve the existing behavior when VERIFY_STALE_LOG_DIR is unset.
🪄 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: 53611ae7-5b00-42a7-a347-ad83b49c636f
📒 Files selected for processing (9)
.agents/skills/nemoclaw-maintainer-verify-stale/SKILL.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/brev-provisioning.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/by-design.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/candidate-selection.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/environment-and-reproducer.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md.agents/skills/nemoclaw-maintainer-verify-stale/scripts/redact-evidence.pytest/maintainer-skills-policy.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/maintainer-skills-policy.test.ts
- .agents/skills/nemoclaw-maintainer-verify-stale/scripts/redact-evidence.py
- .agents/skills/nemoclaw-maintainer-verify-stale/SKILL.md
- .agents/skills/nemoclaw-maintainer-verify-stale/reference/by-design.md
- .agents/skills/nemoclaw-maintainer-verify-stale/reference/brev-provisioning.md
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Do not pipe an unverified installer response directly to Bash on a credential-bearing Brev instance. Download a release-tag-bound installer artifact, verify a repository-accepted checksum or signature, and execute only the verified file for both release passes. Add a policy test that rejects download-and-execute without integrity verification. Then refresh onto current main and rerun the skill policy and required checks.
2 participants
Summary
Prevents stale-issue verification from assigning a fixed verdict unless the same reviewed reproducer exposes the reported symptom on the reported release. The workflow installs requested release tags, treats issue content as untrusted, requires approval before execution or GitHub writes, and verifies cleanup before comparing releases.
Changes
vX.Y.Zrelease tag and require a matching reported-release result before fixed scoring.Type of Change
Quality Gates
Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-verify-stale/SKILL.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/brev-provisioning.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/by-design.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/candidate-selection.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/environment-and-reproducer.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md6234ac308.DGX 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 unavailablepre-commitandcommit-msghooks passed on the repair commits. The final push ran the normal pre-push checks; CLI TypeScript and package-version checks passed.npx vitest run --project integration test/maintainer-skills-policy.test.ts test/skills-frontmatter.test.ts(2 files, 51 tests passed)npm run checks:repositorypassed as a focused repository check.npm run docsbuilds without warnings (doc changes only)Additional focused checks:
npm run checks:repositorynpx vitest run --project integration test/maintainer-skills-policy.test.ts test/skills-frontmatter.test.tsnpx --yes markdownlint-cli2@0.22.0 '.agents/skills/nemoclaw-maintainer-verify-stale/**/*.md'git diff --check6234ac308Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests