fix(skills): enforce stale verification controls - #8309
Merged
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. |
Contributor
|
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 |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit fba6e64 in the TypeScript / code-coverage/cliThe overall coverage in commit fba6e64 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review 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. 4 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 for the commit under review. Recommended E2E: None Manual-only E2E: 1 optional E2E recommendation
Blockers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
23 tasks
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>
Contributor
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
previously requested changes
Aug 11, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
.agents/skills/nemoclaw-maintainer-verify-stale/scripts/redact-evidence.py:40matches authentication headers only when an unquoted header name starts the line. Structured log forms such as{"Cookie":"session=topsecret"}remain unchanged. Because Step 10 permits inspection and public quotation after this helper runs, a session credential can cross the evidence boundary and enter a GitHub comment. Redact quoted and structuredCookieandSet-Cookieforms, and add negative regression cases intest/maintainer-skills-policy.test.ts:612for JSON and spaced assignment forms. Regression evidence: the current pattern returned{"Cookie":"session=topsecret"}unchanged; existing tests cover only canonical line-oriented headers..agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md:222uses the non-POSIX\bextension withgrep -E, although this command is documented for macOS system tools. BSD grep can return no tool names, which skips architecture-drift and multi-axis checks and can produce a falsefixed-on-latestverdict. Use a portable boundary expression or Python extraction, and add an executable compatibility regression test. Existing policy tests do not execute this extraction path.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Collaborator
Author
|
Addressed both blocking findings in 8fcf0d0: structured/quoted Cookie and Set-Cookie evidence is redacted, and architecture-drift extraction uses a portable POSIX boundary with an executable regression. Focused policy regressions, full pre-commit hooks, and the pre-push TypeScript CLI check passed. @cv please re-review the exact head. |
cv
dismissed
their stale review
August 11, 2026 21:13
The latest PR commit redacts structured Cookie forms and replaces the non-portable grep boundary with executable regression coverage.
cv
previously requested changes
Aug 11, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
.agents/skills/nemoclaw-maintainer-verify-stale/scripts/redact-evidence.py:40-47still permits authentication and session credentials to cross the public-evidence boundary. Quoted JSONAuthorizationandProxy-Authorizationfields are not matched, and escaped quotes terminate the new cookie-value match early. The helper returned{"Authorization":"Basic dXNlcjp0b3BzZWNyZXQ="}unchanged and transformed{"Cookie":"session=\"opaquevalue\""}into{"Cookie":[REDACTED]opaquevalue\""}. Step 10 permits inspection and public quotation after this helper runs, so these values can enter a GitHub comment. Redact quoted, structured, spaced-assignment, and escape-containing forms for all four authentication/session headers. Add negative regression cases attest/maintainer-skills-policy.test.ts:612for structured Basic authorization and escaped Cookie/Set-Cookie values. Synchronize the claimed pattern at.agents/skills/nemoclaw-maintainer-verify-stale/reference/scoring-comments-and-logging.md:143with the implemented behavior..agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md:222extracts onlyopenshellandnemoclawcommands. An approvedopenclaw channels add telegramreproducer produces no drift-check entry, although Step 6 explicitly accepts OpenClaw reproducers. This silently skips architecture-drift and multi-axis checks and can produce a falsefixed-on-latestverdict after an OpenClaw surface changes. Includeopenclawin extraction and add an executable regression case attest/maintainer-skills-policy.test.ts:661; the current pipeline returned empty output for that command.
Collaborator
Author
|
Addressed both fresh review findings at exact head
Validation: both focused executable regressions pass; full staged pre-commit gate passes (including gitleaks/markdown/repository checks/test budgets/skills YAML); pre-push gate passes. The full policy file also has 27 passing tests and one unrelated pre-existing local failure in the supersession parser case; the current remote head passed that required CI test before this focused change, and new CI is running now. |
cv
dismissed
their stale review
August 12, 2026 06:11
Commits after this review cover structured and escaped credential redaction, idempotence, and the OpenClaw architecture-drift command. The submitted blockers no longer apply to the latest PR commit.
cv
previously requested changes
Aug 12, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
.agents/skills/nemoclaw-maintainer-verify-stale/reference/reproduction-rubrics.md:227limits architecture-drift history tosrc/,bin/, andnemoclaw/src/. The acceptedopenclaw channels add telegramreproducer is implemented by the guard atscripts/nemoclaw-start.sh:3669-3925, outside that pathspec. Extraction now emitsopenclaw channels, but a release that changes this guard can still produce an empty drift result. The workflow can then skip multi-axis verification and publish a falsefixed-on-latestverdict. Include each owning OpenClaw command surface in the drift check, at leastscripts/nemoclaw-start.shand applicable plugin or wrapper paths, or map extracted commands to authoritative paths. Extendtest/maintainer-skills-policy.test.ts:676-698with an executable history fixture that changes the OpenClaw guard outside the current pathspec and asserts that drift is detected. The current regression test checks extraction only, so it passes while this bypass remains.
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8309.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
|
Security review — latest PR commit
This review replaces the prior container-ownership and architecture-path blockers. Fresh GitHub checks and commit verification remain required before merge. |
cv
approved these changes
Aug 13, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed latest PR commit fba6e64fce3f00621cee65f9818c598ebceb3c92. The final branch binds reset cleanup to post-acquisition container ownership evidence, verifies pre-existing containers remain, and includes the host startup script in architecture drift detection. Focused tests and the nine-category security review pass with no remaining findings. Approval remains subject to current required checks and GitHub commit verification.
cv
dismissed
their stale review
August 13, 2026 07:03
Dismissed as stale: the latest commit includes the host startup and agent wrapper paths in drift detection and adds an executable Git history regression fixture.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-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 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