fix(install): keep the managed checkout clean after dependency install - #8316
Conversation
The installer's own `npm install` normalizes the committed lockfile while it builds the tree, so the checkout it owns was dirty by the time the reuse check ran. Every re-run then took the destructive branch and re-cloned instead of reusing the installed revision. Restoring the lockfile keeps the checkout at that revision, and the reuse test now models a dirty tree instead of reporting a clean one unconditionally. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughThe installer restores ChangesManaged checkout reuse
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit c4a2d38 in the TypeScript / code-coverage/cliThe overall coverage in commit c4a2d38 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
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 `@scripts/install.sh`:
- Around line 2062-2066: Update restore_managed_source_lockfile to return the
git checkout failure instead of suppressing it with || true, and ensure its
caller at the managed installation flow around line 2150 propagates the failure
and stops installation. Add a regression test covering a failed
package-lock.json restoration and verify the installation does not continue or
remove and reclone the checkout.
🪄 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: e9cb02a0-310c-45a4-9909-5ce7a29a1130
📒 Files selected for processing (2)
scripts/install.shtest/install-managed-cli-reuse.test.ts
The restore discarded the checkout status, so a failed restore looked like a success and the next run silently fell back to removing and recloning the checkout. The failure now reaches the caller, which warns and names that fallback, and the reuse test covers the failing restore. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/install-managed-cli-reuse.test.ts (3)
246-258: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the fallback warning.
The test checks
Could not restore package-lock.json, the absence of the reuse message, and a secondgit init. It does not check that the warning identifies the removal-and-reclone fallback.Add an assertion for a stable fallback-specific warning substring. This prevents the test from passing when the user receives no recovery guidance.
Based on the PR objective, the warning must identify the removal-and-reclone fallback.
🤖 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/install-managed-cli-reuse.test.ts` around lines 246 - 258, The test case run by runManagedCliInstallTwice should also assert that result.stdout contains the stable warning substring identifying the removal-and-reclone fallback, alongside the existing lockfile, reuse-message, and git-init assertions.
136-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject unsupported build commands in the fake npm.
The
case "$*"patterns match any command containingbuild. Therunbranch has no failure path for unknown commands. A mistyped build script can return success and create artifacts, so the test can miss a broken installer command.Match the exact
run build:cliandrun buildcommands. Exit with a failure for unsupportednpm runcommands.As per path instructions, do not let a broad mock bypass the behavior under test.
🤖 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/install-managed-cli-reuse.test.ts` around lines 136 - 149, Update the fake npm command handling around the run-command case so it matches only the exact supported commands “run build:cli” and “run build”, rather than any argument containing “build”. Add a failure path for unsupported npm run commands, ensuring mistyped build scripts exit nonzero without creating artifacts.Source: Path instructions
103-109: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the lockfile fixture verify the managed file.
The fake
npm installonly creates.fixture-lockfile-dirtyat Line 135. The fake Git path clears that marker for anycheckoutcommand at Lines 103 and 107-109. It does not validatepackage-lock.jsonor restore its contents.The pre-existing
writeManagedSourcefixture also has no lockfile. The test can therefore pass with a wrong checkout target or without verifying restored content. Create a baselinepackage-lock.json, mutate it during fake installation, requirecheckout -- package-lock.json, and assert the baseline content before cleanup.As per path instructions, review this test through observable lockfile behavior instead of a private dirty marker.
Also applies to: 135-135
🤖 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/install-managed-cli-reuse.test.ts` around lines 103 - 109, Update the lockfile fixture and fake Git/npm flows around writeManagedSource and the checkout handling so a baseline package-lock.json is created, fake installation mutates its contents, and only checkout -- package-lock.json restores the baseline. Remove reliance on the private .fixture-lockfile-dirty marker, and assert the restored package-lock.json content before cleanup to verify observable lockfile behavior.Source: 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.
Nitpick comments:
In `@test/install-managed-cli-reuse.test.ts`:
- Around line 246-258: The test case run by runManagedCliInstallTwice should
also assert that result.stdout contains the stable warning substring identifying
the removal-and-reclone fallback, alongside the existing lockfile,
reuse-message, and git-init assertions.
- Around line 136-149: Update the fake npm command handling around the
run-command case so it matches only the exact supported commands “run build:cli”
and “run build”, rather than any argument containing “build”. Add a failure path
for unsupported npm run commands, ensuring mistyped build scripts exit nonzero
without creating artifacts.
- Around line 103-109: Update the lockfile fixture and fake Git/npm flows around
writeManagedSource and the checkout handling so a baseline package-lock.json is
created, fake installation mutates its contents, and only checkout --
package-lock.json restores the baseline. Remove reliance on the private
.fixture-lockfile-dirty marker, and assert the restored package-lock.json
content before cleanup to verify observable lockfile behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d817c9f-3b9e-4666-ac24-c21ee88e3375
📒 Files selected for processing (2)
scripts/install.shtest/install-managed-cli-reuse.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.sh
The reuse fixtures tracked a private marker file, so a wrong checkout target or an unrestored lockfile still passed. The fake Git and npm commands now read and write package-lock.json, the assertions cover its restored content and the warning that names the re-clone fallback, and an unsupported npm run command fails instead of reporting success. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/install-managed-cli-reuse.test.ts (1)
251-262: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the no-rebuild outcome.
The test proves one initialization, one install, and one link operation. It does not prove that
npm run --if-present build:cliornpm run buildruns only during the first installer run. A regression that rebuilds the reused checkout can still pass this test.Add a build sentinel or an assertion that each build command occurs once across both runs.
As per path instructions, make the test prove the no-rebuild behavior instead of only checking the current command sequence.
🤖 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/install-managed-cli-reuse.test.ts` around lines 251 - 262, Add coverage in the managed CLI reuse test around runManagedCliInstallTwice to verify build:cli and build commands each execute exactly once across both installer runs. Use the existing command log or a build sentinel, while preserving the current initialization, install, link, lockfile, and reuse assertions.Source: 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.
Nitpick comments:
In `@test/install-managed-cli-reuse.test.ts`:
- Around line 251-262: Add coverage in the managed CLI reuse test around
runManagedCliInstallTwice to verify build:cli and build commands each execute
exactly once across both installer runs. Use the existing command log or a build
sentinel, while preserving the current initialization, install, link, lockfile,
and reuse assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e37349f-2f2e-4c0d-81d1-7b4f85c3c3f6
📒 Files selected for processing (1)
test/install-managed-cli-reuse.test.ts
|
The current branch has 43 passing checks, no pending or failed checks, and no unresolved review threads. The addressed review finding now also has a written disposition in its thread. The remaining merge blocker is independent approval. |
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry for the planned NemoClaw v0.0.103 release. The new `docs/changelog/2026-08-05.mdx` entry uses the exact `## v0.0.103` heading and summarizes supported user-visible changes merged since v0.0.102. ## Changes - Add the parser-safe MDX SPDX header, three-paragraph release summary, and detailed grouped bullets to `docs/changelog/2026-08-05.mdx`. - Link each release-note group to the most specific published OpenClaw, Hermes, or Deep Agents documentation routes. - Exclude dormant MXC and Podman foundations, internal managed-inference adapters, test-only changes, and maintainer tooling from the supported product narrative. ### Source summary - [#8082](#8082) -> `docs/changelog/2026-08-05.mdx`: Document the new one-command agent launch flow. - [#8314](#8314) -> `docs/changelog/2026-08-05.mdx`: Document managed vLLM host capability validation and restart handling. - [#8248](#8248) -> `docs/changelog/2026-08-05.mdx`: Record the DGX Spark Qwen profile MTP default change. - [#8223](#8223) -> `docs/changelog/2026-08-05.mdx`: Record explicit model preservation across provider switches. - [#8209](#8209) -> `docs/changelog/2026-08-05.mdx`: Document corrected Windows WSL provider selection. - [#8316](#8316) -> `docs/changelog/2026-08-05.mdx`: Record clean managed-checkout reuse after installation. - [#8239](#8239) -> `docs/changelog/2026-08-05.mdx`: Record the packaged-service teardown fallback. - [#8247](#8247) -> `docs/changelog/2026-08-05.mdx`: Document uninstall behavior for an already-removed sandbox. - [#7998](#7998) -> `docs/changelog/2026-08-05.mdx`: Record preserved container-start diagnostics. - [#8027](#8027) -> `docs/changelog/2026-08-05.mdx`: Record journal-backed not-ready repair authority. - [#7812](#7812) -> `docs/changelog/2026-08-05.mdx`: Document actionable rebuild preflight diagnostics. - [#8222](#8222) -> `docs/changelog/2026-08-05.mdx`: Record redacted top-level CLI failures. - [#8313](#8313) -> `docs/changelog/2026-08-05.mdx`: Record structured MCP bridge destruction failures. - [#8211](#8211) -> `docs/changelog/2026-08-05.mdx`: Document cleanup of incomplete snapshot captures. - [#8212](#8212) -> `docs/changelog/2026-08-05.mdx`: Document best-effort post-restore policy reconciliation. - [#8245](#8245) -> `docs/changelog/2026-08-05.mdx`: Clarify manifest-defined OpenClaw workspace persistence. - [#8254](#8254) -> `docs/changelog/2026-08-05.mdx`: Include corrected snapshot restore selection guidance. - [#8238](#8238) -> `docs/changelog/2026-08-05.mdx`: Document preservation of managed MCP policy entries. - [#7568](#7568) -> `docs/changelog/2026-08-05.mdx`: Record mutable-default Shields rollback preservation. - [#8200](#8200) -> `docs/changelog/2026-08-05.mdx`: Record truthful Shields state after a rejected transition. - [#7895](#7895) -> `docs/changelog/2026-08-05.mdx`: Record descriptor-bound Shields lock inspection. - [#7892](#7892) -> `docs/changelog/2026-08-05.mdx`: Document the canonical Hermes dashboard profile and migration. - [#7871](#7871) -> `docs/changelog/2026-08-05.mdx`: Document fail-closed Hermes cron restore. - [#7894](#7894) -> `docs/changelog/2026-08-05.mdx`: Record the reset Hermes health budget after recovery. - [#8228](#8228) -> `docs/changelog/2026-08-05.mdx`: Document Hermes build-time corporate CA trust. - [#8206](#8206) -> `docs/changelog/2026-08-05.mdx`: Document bounded Deep Agents Code failure classification. - [#8297](#8297) -> `docs/changelog/2026-08-05.mdx`: Record reuse of the published Deep Agents Code base image. - [#8321](#8321) -> `docs/changelog/2026-08-05.mdx`: Document aligned endpoint SSRF protections and userinfo rejection. - [#8299](#8299) -> `docs/changelog/2026-08-05.mdx`: Document the fail-closed `setpriv` transition in managed images. - [#7603](#7603) -> `docs/changelog/2026-08-05.mdx`: Record corrected confidentiality-root traversal. - [#8334](#8334) -> `docs/changelog/2026-08-05.mdx`: Record removal of the unsupported logs audit example. - [#8256](#8256) -> `docs/changelog/2026-08-05.mdx`: Record reordered network-policy walkthrough prerequisites. - [#7767](#7767) -> `docs/changelog/2026-08-05.mdx`: Record platform runtime shape validation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `npx vitest run test/changelog-docs.test.ts` passed all 6 tests. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [ ] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-08-05.mdx` follows the release-prep and documentation writing rules. The changelog contract tests passed 6/6, and `npm run docs` completed with 0 errors and the repository's 2 existing Fern warnings. - Agent: Codex Desktop <!-- docs-review-head-sha: 66fcd80 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## 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 — `npx vitest run test/changelog-docs.test.ts`: 1 file and 6 tests passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not run for this doc-only change. - [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) — completed with 0 errors and 2 existing Fern warnings. - [x] 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) — the native changelog uses the required parser-safe MDX SPDX comment and does not use page frontmatter. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.103. * Documented the new `nemoclaw launch` command. * Included updates covering onboarding, inference, installation, recovery, snapshots, security, integrations, endpoint validation, sandbox hardening, and related guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
The installer's own
npm installnormalizes the committed lockfile while it builds the tree, so the checkout at~/.nemoclaw/sourcewas dirty by the time the reuse check ran. Before this change every installer re-run failed that check and took the destructive branch, removing the checkout and cloning and rebuilding it again; after it, a re-run at the installed revision reuses the checkout as intended.Related Issue
Fixes #8305
Changes
scripts/install.sh: addrestore_managed_source_lockfileand call it after the managed branch finishes its npm steps, so the checkout the installer owns stays at the installed revision. The source-checkout branch is untouched, because that tree belongs to the contributor.test/install-managed-cli-reuse.test.ts: the git stub answereddiffwith an unconditionalexit 0, so the reuse check passed in tests while it could never pass on a real install. The stub now reports a dirty tree from state the npm stub writes duringinstalland thecheckoutcase clears, and the build stubs write the artifacts the reuse check inspects.test/install-managed-cli-reuse.test.ts: add a case covering a later installer run, which reuses the checkout instead of cloning again.Type of Change
Quality Gates
docs/reference/commands.mdxalready documents--freshas the clean re-clone escape hatch for a broken-but-current install, which describes the behavior this change restores.Documentation Writer Review
no-docs-neededdocs/reference/commands.mdxalready describes through the--freshre-clone escape hatch, so no page needs an edit.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 unavailablenpx vitest run --project installer-integration test/install-managed-cli-reuse.test.ts— 4 passed. With the newrestore_managed_source_lockfilecall removed, the added case fails and the three existing cases still pass.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