fix(cli): prevent silent fresh downgrades - #8367
Conversation
Signed-off-by: Rui Luo <ruluo@nvidia.com>
|
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 update command adds ChangesUpdate fresh version guard
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant User
participant UpdateCommand
participant runUpdateAction
participant MaintainedTargetResolver
participant MaintainedInstaller
User->>UpdateCommand: run update --fresh
UpdateCommand->>runUpdateAction: pass fresh and allowDowngrade
runUpdateAction->>MaintainedTargetResolver: resolve maintained revision and version
MaintainedTargetResolver-->>runUpdateAction: return version relation
runUpdateAction->>runUpdateAction: reject unsafe replacement without allowDowngrade
runUpdateAction->>MaintainedInstaller: install reviewed maintained revision when permitted
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8367.docs.buildwithfern.com/nemoclaw |
![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 `@src/lib/actions/update.ts`:
- Around line 203-214: Update parsePublicVersion to split prerelease identifiers
only on dots, preserving hyphens within each identifier so SemVer comparison
distinguishes values such as rc-1 and rc.1. Add a regression test in the update
test suite verifying this ordering is treated as a downgrade and requires
--allow-downgrade rather than permitting --fresh.
🪄 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: 01960f8b-a041-4f2c-bbf0-f27b7bae8af1
📒 Files selected for processing (6)
docs/reference/commands.mdxsrc/commands/update.tssrc/lib/actions/update.test.tssrc/lib/actions/update.tssrc/lib/cli/public-display-defaults.tstest/update.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
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 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Rui Luo <ruluo@nvidia.com>
Signed-off-by: Rui Luo <ruluo@nvidia.com>
…e-guard' into fix/8306-update-fresh-downgrade-guard
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed PRA-1 in e52a3a3. The update action now resolves the maintained tag once as a revision and version, validates the revision format, removes inherited installer overrides, and passes that validated revision to the installer. Regression coverage changes the tag after comparison and confirms the installer retains the selected target; a malformed target is also rejected. Validation: focused update tests passed 35/35, affected integration tests passed 38/38, CLI type-checking passed, and the docs build completed with 0 errors. The independent documentation writer review passed after clarifying the conditional behavior. The maintainer security review passed all nine categories with no findings. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/update.ts (1)
378-382: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the unused
getLatestVersioncompatibility path.
UpdateCommanduses the built-in maintained-target resolver, so the fallback has no production callers. Replace the test fakes withgetMaintainedTargetvalues that includerevision, then removegetLatestVersionand its fallback. If external callers require it, define a bounded owner and retirement criteria.🤖 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 `@src/lib/actions/update.ts` around lines 378 - 382, Remove the getLatestVersion compatibility branch from the maintainedTarget selection in UpdateCommand, retaining only the injected getMaintainedTarget path and the getMaintainedNemoClawTargetFromGitTag fallback. Update test fakes and call sites to provide getMaintainedTarget results containing both revision and version, then remove the obsolete getLatestVersion dependency and references.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@src/lib/actions/update.ts`:
- Around line 378-382: Remove the getLatestVersion compatibility branch from the
maintainedTarget selection in UpdateCommand, retaining only the injected
getMaintainedTarget path and the getMaintainedNemoClawTargetFromGitTag fallback.
Update test fakes and call sites to provide getMaintainedTarget results
containing both revision and version, then remove the obsolete getLatestVersion
dependency and references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 46389c88-fb9c-48a1-a041-35cda8478e90
📒 Files selected for processing (3)
docs/reference/commands.mdxsrc/lib/actions/update.test.tssrc/lib/actions/update.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/actions/update.test.ts
- docs/reference/commands.mdx
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Follow-up after the branch update: 6b8f73a restores the independently reviewed conditional documentation wording. The preceding branch update correctly made ambiguous same-line The reference now states that the command passes the selected repository revision to the installer when the maintained tag resolves. Independent documentation review passed with no remaining findings. Focused update tests passed 36/36, affected integration tests passed 38/38, CLI type-checking passed, and the docs build completed with 0 errors. The security delta review passed all nine categories with no findings. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the latest CodeRabbit finding in 693a08b. The unused Validation: focused update tests passed 36/36, affected integration tests passed 38/38, CLI type-checking and test-title checks passed, and scoped hooks passed. The independent documentation-impact review confirmed that no additional docs change is needed and the current command reference remains accurate. The security review passed all nine categories with no findings. |
|
CI classification: the current Terra advisor job failed because the analysis service omitted its required terminology-analysis result. The job stopped before producing a branch finding, so this is an automated-review service failure rather than a PR-related test or code failure. No branch change is indicated. I will use the single bounded failed-job rerun after the paired advisor workflow completes. |
|
Second-opinion disposition for the current branch:
No additional branch change is indicated by these advisory findings. All non-advisor CI gates currently pass; the single bounded Terra retry is still running. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer refresh and review disposition for the current branch revision:
Security review under the current nine-category rubric:
Overall security verdict: PASS with no findings. The default path denies unproven fresh replacements, and the override is explicit and documented. No merge was performed. The release freeze remains in effect. After fresh required checks complete, this PR still needs independent approval before it can enter the merge queue. |
|
Current branch revision
Result: 42 checks pass. The only non-passing check is the external secondary advisor job described above. The branch is mergeable with no unresolved review threads; independent human approval is still required. Merge remains deferred under the active release-tag freeze. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review for exact head a872c39979aba83253166cf26587d1315ae9d2b1 against base db31c286129e878c3356eed49f76ab259561e47e: PASS.
- Secrets and credentials: PASS. No secret source or storage changes. The installer environment continues to remove
BASH_ENV,ENV, and caller-provided release override variables before launching the shell. - Input validation: PASS. The downgrade guard parses the complete supported public-version format, fails closed on invalid or incomparable values, and accepts a maintained revision only when
git ls-remotereturns a 40- to 64-character hexadecimal object ID. - Authentication and authorization: PASS. No authentication, authorization, ownership, or permission boundary changes. Destructive host replacement now requires explicit
--allow-downgradeconsent when safety cannot be proved;--yesonly skips confirmation. - Dependencies: PASS. No dependency or lockfile changes.
- Error handling and logging: PASS. Older, unresolved, and incomparable maintained versions produce actionable refusal messages without running the installer. Accepted downgrade output states the risk instead of claiming the install is current.
- Cryptography and data protection: PASS. No cryptographic or protected-data path changes. The exact reviewed Git object ID is handed to the installer to avoid a later maintained-tag movement changing the selected build.
- Configuration and security headers: PASS. No network policy, service exposure, debug mode, port, or security-header changes. Installer environment sanitization prevents inherited shell startup files and release overrides from changing the guarded handoff.
- Security testing: PASS. Regression tests cover older, unresolved, malformed, incomparable, prerelease, build-metadata, git-describe, explicit-consent, immutable-revision, source-checkout, non-interactive, and environment-sanitization paths.
- System security: PASS. The change reduces host integrity risk by refusing an unapproved downgrade, preserves the source-checkout guard, and runs the existing maintained installer only after the existing confirmation or explicit non-interactive consent path.
Local evidence at this exact head: 36/36 focused source tests and 38/38 integration tests passed; plugin and CLI builds, CLI type-check, the documentation build and agent-variant check, exact Vitest project membership (2177 files across 7 projects), and npm run check:diff passed. Documentation review returned docs-updated for docs/reference/commands.mdx at head marker a872c39979ab and AGENTS marker 3dd7c2425b70.
This review does not waive CI or independent human approval.
|
Current validation receipt for branch revision
GitHub reports the PR mergeable. Independent approval is still required, and the active release-tag merge freeze remains in force. No merge or auto-merge action was taken. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer update for current branch revision
Fresh repository checks and automated reviews are running. Independent approval remains the human merge gate. I have not merged the PR. |
|
CI classification: GitHub Actions is currently returning repeated 5xx errors while resolving standard actions. In the latest run, shards 3, 5, 7, and 8 failed during action setup before repository tests ran. Shard 4 reached tests and hit the known unchanged lifecycle-lock race; that full suite passes locally (49/49 in 2.79 seconds). I have queued a failed-jobs-only retry for those shards and downstream aggregates. No source change is indicated; required gates remain blocked on the external Actions service recovery. |
4 participants
Summary
Prevent
nemoclaw update --fresh --yesfrom silently replacing a newer installation with an older maintained build. Downgrades and versions that cannot be safely compared now require explicit confirmation with--allow-downgrade.Related Issue
Fixes #8306
Changes
--allow-downgradeis provided.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/reference/commands.mdxto document--allow-downgrade, the default--freshrefusal for older, unresolved, or incomparable maintained versions, the confirmation-only effect of--yes, and the validated maintained-target handoff to the installer. Independent review confirmed no remaining documentation findings;npm run docsandnpm run docs:check-agent-variantspassed.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 unavailablenpm run check:diffalso passed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rui Luo ruluo@nvidia.com
Summary by CodeRabbit
--allow-downgradeto theupdatecommand.--freshnow reinstalls only when the maintained build is the same version or newer by default.--allow-downgradeto explicitly permit reinstalls of older or unordered maintained builds.--yesskips confirmation but does not override downgrade protection.