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.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@coderabbitai review

Automated PR review summary

Reviewed PR #3108: refactor(installer): add internal plan commands

Recommendation

• Recommendation: PASS
• Highest observed severity: low
• Block merge: no
• Why: If merged as-is, the main risk would have been hidden installer helpers producing unstable or incorrect planning data for follow-up shell migrations. In the installed environment, the new commands were reachable and behaved deterministically under adversarial conflicting and malformed inputs.
• Reviewer summary: Reviewed the installed CLI adversarially against the PR’s new internal installer entrypoints. The highest-value probes on command routing, deterministic plan output, ref/tag precedence, provider normalization, and malformed-provider handling all passed.

Installation and setup findings

• Install from the local checkout succeeded through CLI install and onboarding-created sandbox setup. The top-level installer command timed out during the last policy preset phase, but the created sandbox 'my-assistant' was present and Ready. Verification succeeded: nemoclaw list showed the sandbox, SSH via openshell sandbox ssh-config my-assistant executed printf "2+2=4\n" inside it, and an in-sandbox openclaw agent --agent main -m 'Reply with only: 4' returned 4. I also confirmed that --local is intentionally blocked inside NemoClaw sandboxes.

What was validated

• The PR revision was checked out in an isolated review environment.
• The local checkout was installed using the repository installer flow as closely as the environment allowed.
• Adversarial, PR-specific probes were then run against the installed environment and relevant repository context.
• Diff summary:

 .../skills/nemoclaw-contributor-create-pr/SKILL.md |   20 +-
 .../nemoclaw-maintainer-pr-comparator/SKILL.md     |  121 --
 .../checks/tier-0-gates.md                         |   61 -
 .../checks/tier-1-correctness.md                   |   86 --
 .../checks/tier-2-quality.md                       |   64 -
 .../repo-policy.md                                 |   86 --
 .../scripts/check-coderabbit-threads.sh            |  105 --
 .../scripts/collect-gates.sh                       |   84 --
 .../scripts/find-candidates.sh                     |   86 --
 .../scripts/parse-supersession.sh                  |   68 -
 .../scripts/render-verdict.py                      |  232 ----
 .../templates/verdict.md                           |   88 --
 .../tiebreakers.md                                 |   61 -
 .../validation/backtest.md                         |   69 -
 .agents/skills/nemoclaw-skills-guide/SKILL.md      |    2 +-
 .../references/agent-skills.md                     |    4 +-
 .../nemoclaw-user-configure-inference/SKILL.md     |  329 ++---
 .../references/inference-options.md                |    4 +-
 .../references/set-up-sub-agent.md                 |  120 --
 .../references/switch
...[truncated]

Failing tests and unresolved impact

• No failing adversarial tests were captured.

Passing tests and why they mattered

Passing test 1: Installed CLI routes hidden internal installer commands

• What was tested: The installed nemoclaw binary exposes the new hidden internal installer namespace and help text in a real runtime, not only in unit tests.
• Why it mattered: If routing is broken, follow-up shell migrations cannot rely on these internal entrypoints.
• Observed result: internal installer plan --help succeeded in the installed CLI and showed the new command and flags.
• Command: bash /tmp/pr3108_probe.sh
• Recommended follow-up coverage: Keep the existing CLI routing integration test; no extra unit test is needed beyond installed-package smoke coverage.

Passing test 2: Installer plan honors explicit ref precedence and emits deterministic normalized JSON

• What was tested: internal installer plan --json uses explicit --install-ref over --install-tag, normalizes provider aliases, and returns runtime/npm data from explicit probe inputs.
• Why it mattered: If false, shell migrations built on this planner could install the wrong ref or derive incorrect runtime/npm actions.
• Observed result: JSON showed installRef=feature/refactor, installerVersion=feature/refactor, provider normalized cloud->build, runtime ok:true, and npm fields derived from /tmp/npm-prefix.
• Command: bash /tmp/pr3108_probe.sh
• Recommended follow-up coverage: Add or keep an integration regression test for conflicting ref/tag inputs through the installed CLI, since precedence bugs are easy to reintroduce.

Passing test 3: Resolve-release-tag and normalize-env preserve deterministic fallback behavior

• What was tested: resolve-release-tag returns ref-over-tag precedence in JSON and tag fallback in text mode, while normalize-env trims refs and maps provider aliases predictably.
• Why it mattered: If false, downstream installer shell code may parse unstable outputs or normalize env differently than the helper commands claim.
• Observed result: resolve-release-tag --json preferred feature/refactor over v9.9.9; text mode with only tag returned v2.0.0; normalize-env trimmed branch/test and mapped nim->nim-local and cloud->build.
• Command: bash /tmp/pr3108_probe.sh && bash /tmp/pr3108_probe2.sh
• Recommended follow-up coverage: Add a regression test for text-mode output stability if shell scripts will parse these commands directly.

Passing test 4: Invalid provider is surfaced as invalid instead of causing planner failure

• What was tested: Unsupported provider values are reported as invalid planning metadata rather than crashing the new internal installer commands.
• Why it mattered: If false, malformed installer env could cause hard failures instead of inspectable planning output during shell migration work.
• Observed result: normalize-env --json --provider bad-provider returned normalized:null and valid:false; non-JSON plan --provider bad-provider still returned a concise summary instead of erroring.
• Command: bash /tmp/pr3108_probe.sh && bash /tmp/pr3108_probe2.sh
• Recommended follow-up coverage: Keep this as an integration/regression test because malformed env handling is a likely shell-migration edge case.

Bottom line

• Based on the install evidence and adversarial probes, this PR looks reasonable to approve.

verified locally — no env leak from the ...process.env spread (AWS_SECRET_ACCESS_KEY etc. don't appear in the JSON output, since buildInstallerPlan only reads 4 typed keys).

one footgun worth noting before downstream consumers wire up: installRef is stored verbatim with no validation. running NEMOCLAW_INSTALL_REF='\$(rm -rf /)' nemoclaw internal installer plan --json produces "installRef": "\$(rm -rf /)". internal-only command and no consumer in scripts/install.sh yet, so it's theoretical — but if/when a shell consumer does TAG=\$(... resolve-release-tag) && curl .../installer-\$TAG.sh, the verbatim ref becomes RCE.

cheap fixes: reject anything that's not ^[A-Za-z0-9._/-]+\$ in resolveInstallRef, or whitelist env keys instead of spreading process.env into the input.