Skip to content

test(package): install packed CLI as a consumer - #10986

Merged
rsliter merged 1 commit into
mainfrom
codex/fix-package-contract-npm-install
Sep 3, 2026
Merged

test(package): install packed CLI as a consumer#10986
rsliter merged 1 commit into
mainfrom
codex/fix-package-contract-npm-install

Conversation

@rsliter

@rsliter rsliter commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Outcome

The managed-image registry transport package contract now installs the packed CLI through a consumer project. The test still verifies the production undici dependency and transport load without asking npm to resolve NemoClaw's development graph.

Reason

Main's build-typecheck job fails in this contract because npm 10.9.8 crashes with Cannot read properties of null (reading 'edgesOut'). The test caused the crash by unpacking the tarball and treating it as the root project, which makes npm resolve development-only optional peers despite --omit=dev. A consumer install models the published package boundary that the test intends to verify.

Failure: https://github.com/NVIDIA/NemoClaw/actions/runs/33777143937/job/100722012206

Changes

  • Install the packed CLI archive into a minimal consumer project with --omit=dev and --no-save.
  • Inspect undici from the consumer's production dependency tree.
  • Load the registry transport from the installed nemoclaw package.

Verification

  • npx vitest run --project package-contract test/package-contract/managed-image-registry-transport.test.ts --reporter=verbose - passed on commit 16a7cedd8a7103f914c14335a17688c0dae5daeb after refresh to main.
  • npm run test:titles:check - passed.
  • npm run validate:pr - passed.
  • npm run review:local - unavailable because the temporary OpenShell gateway refused connections during Advisor configuration.
  • The package-contract aggregate executed the changed test successfully. Other tests had existing macOS environment failures from the unavailable private SDK artifact and five-second timeouts.
  • The validated diff contains no secrets, API keys, or credentials.

Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com

Summary by CodeRabbit

  • Tests
    • Improved package installation verification by testing installation from a packaged archive in a separate consumer project.
    • Added validation of dependency integrity and runtime behavior from the installed package location.
    • Enhanced cleanup of temporary test artifacts.

Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
@rsliter rsliter self-assigned this Sep 3, 2026
@copy-pr-bot

copy-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 037b7712-7764-4091-9665-b509f1ed7ec1

📥 Commits

Reviewing files that changed from the base of the PR and between f7c1d83 and 16a7ced.

📒 Files selected for processing (1)
  • test/package-contract/managed-image-registry-transport.test.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The package-contract test now installs the packed fixture tarball into a temporary consumer project. It verifies dependencies from that project, runs the probe from the installed package path, and removes temporary directories during cleanup.

Changes

Managed image registry transport contract test

Layer / File(s) Summary
Archive and consumer setup
test/package-contract/managed-image-registry-transport.test.ts
The test creates separate archive and consumer directories, retains the packed tarball, creates a minimal consumer package.json, and removes the unused import.
Install and validate the packaged module
test/package-contract/managed-image-registry-transport.test.ts
The test installs the tarball with --no-save, checks dependencies from the consumer project, runs the probe from node_modules/nemoclaw, and cleans up both temporary directories.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 16a7c

The contract test now installs the packed CLI archive through a minimal consumer project and validates the installed package without development dependencies. No current merge-blocking risk remains.

Suggested reviewers: aasthajh, afourniernv, ahunnargikar-nvidia

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: installing the packed CLI in a consumer project for package-contract testing.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-package-contract-npm-install

Comment @coderabbitai help to get the list of available commands.

@rsliter

rsliter commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-code-quality

github-code-quality Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 16a7ced in the codex/fix-package-co... branch remains at 96%, unchanged from commit f7c1d83 in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit 16a7ced in the codex/fix-package-co... branch remains at 83%, unchanged from commit f7c1d83 in the main branch.


Updated September 03, 2026 18:01 UTC

@rsliter
rsliter marked this pull request as ready for review September 3, 2026 18:01
@rsliter

rsliter commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Ready for review. This PR unblocks the recurring main build-typecheck failure by installing the packed CLI into a minimal consumer project, matching the real package boundary. The focused package-contract test, npm run validate:pr, and the PR build-typecheck job passed. An unrelated CLI shard timeout passed on a single-job retry, and that focused test passed locally in 205 ms.

PR Review Advisor is currently broken upstream for this commit: eight specialist jobs ended in model 429s and one failed while pulling the shared sandbox image, so the workflow produced no findings. I could not complete npm run review:local because the temporary OpenShell gateway refused connections. CodeRabbit completed against the exact commit with no actionable findings.

@rsliter
rsliter requested a review from prekshivyas September 3, 2026 18:01
@ericksoa

ericksoa commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This is the exact deterministic base failure currently blocking PR #10810 at head d219206d0710d19ed340e5c238ccef8660dd44ab. Two independent build-typecheck attempts passed 1,238/1,239 package-contract assertions and then npm 10.9.8 crashed in managed-image-registry-transport.test.ts with Cannot read properties of null (reading edgesOut). #10810 does not touch package metadata or this harness, so I am keeping the repair isolated here rather than duplicating it into the policy PR. Once this lands, I will refresh #10810 from the corrected base and rerun its exact-head gates.

@rsliter
rsliter merged commit ee8b43f into main Sep 3, 2026
96 of 108 checks passed
@rsliter
rsliter deleted the codex/fix-package-contract-npm-install branch September 3, 2026 18:10
gaveezy added a commit that referenced this pull request Sep 3, 2026
Picks up #10986, which fixes the build-typecheck packed-CLI install contract.

Signed-off-by: Hai Nguyen <haingu@nvidia.com>
prekshivyas pushed a commit that referenced this pull request Sep 3, 2026
## Outcome

Messaging onboarding now routes checked-in profile inspection and
import, provider lookup and update, and post-update verification through
typed `OpenShellProviderAdapter` results. Named gateway targets fail
closed when an ambient `OPENSHELL_GATEWAY_ENDPOINT` could redirect the
operation, while lookup, update, identity-mismatch, and operational
failures retain distinct redacted diagnostics.

## Reason

The accepted #9806 slice requires the adapter to own the OpenShell CLI
boundary instead of leaving profile and provider lifecycle commands in
onboarding. This is a fresh implementation from current `main`; it does
not merge or cherry-pick the closed #10719 attempt.

### Related issues

- Part of #9806
- Replaces the adapter foundation attempted in #10719
- Relates to #9813, which owns migration of the remaining raw CLI
consumers
- Provides a fresh base on which the separate #10724 and #10726 consumer
slices can be restacked

## Changes

- Add typed provider metadata, lookup, update, profile-import, and
verification results to `OpenShellProviderAdapter`.
- Move checked-in profile parsing, validation, import, export, and exact
contract verification behind the CLI adapter.
- Move provider metadata and diagnostic parsing to the adapter layer,
retaining only the narrow exact-not-found classification needed at the
CLI boundary.
- Route messaging provider publication and managed-clone reconciliation
through exact adapter calls.
- Preserve safe redacted lookup and update failure details while keeping
identity mismatch distinct from operational failure.
- Preserve both provider and temporary-source cleanup failures when
preparation aborts.
- Reject named-target operations when ambient
`OPENSHELL_GATEWAY_ENDPOINT` is present.
- Bind both ordinary and Hermes portable sandbox creation to the same
provider-effect boundary, including deferred post-identity effects and
resume replay protection.
- Add tests for exact `getProvider`, `updateProvider`, profile-import,
and verification calls, including raw-command exclusion and repeatable
desired-state recovery after partial publication.

## Verification

- On exact candidate tree `7e3490e69`, the focused CLI suite passed 253
tests across seven files, covering adapter get/update/profile results,
provider publication, ordinary creation, Hermes portable creation, and
the real superseded portable transaction. The published signed candidate
`364aa89d4` has that exact tree.
- `npx vitest run --project integration
test/onboarding/onboard-hermes-portable-provider-publication.test.ts`:
four public-boundary cases passed on `364aa89d4`, covering pre-create
publication, deferred post-verification publication, verified-resume
suppression, superseded-path exclusion, exact named-gateway adapter
calls, and isolation from ambient XDG paths.
- `npm run typecheck:cli`: passed on `364aa89d4` after the final
canonical `main` refresh.
- Targeted Oxlint for the changed TypeScript files: passed.
- `npm run validate:pr`: passed on exact signed candidate `364aa89d4`
against canonical `main` `d836ccb44` in a clean isolated checkout,
including repository checks, secret scanning, source-shape checks,
growth guardrails, commit policy, and the CLI pre-push type check.
- The full manual-stage coverage pass completed successfully. The
broader all-files pre-commit sweep passed every check except existing
Hadolint warnings in unchanged Dockerfiles; the identical Hadolint
failure was reproduced on canonical `main` `3509b5a43` before the
subsequent `main` refresh.
- `git diff --check origin/main...HEAD`: passed.
- `npm run review:local`: previously failed before analysis because its
desktop bootstrap did not forward the active Colima `DOCKER_HOST`; no
artifacts were produced. Per maintainer direction, repairing that
unrelated local-review tooling is deferred.
- PR Review Advisor: three full runs on `eb2be67fd` produced no
specialist artifacts and were deferred as unavailable infrastructure
evidence. On final head `c7217b54b`, run `33759103328` succeeded for all
nine specialists. Every specialist summary and full JSONL session was
read; no specialist reported a change-required finding.
- Diff inspection: no secrets, API keys, or credentials are present.

## Review notes

- CodeRabbit's final incremental review covered `eb2be67fd..c7217b5`
and produced no actionable comments. Its generated summary retained an
older Hermes portable risk sentence, but CodeRabbit rechecked commits
`1abb93872` and `49bc39aef`, verified all four portable lifecycle cases,
and explicitly classified that sentence as stale. All review threads are
resolved.
- CodeRabbit's docstring-coverage warning is a generic advisory, not a
repository gate. Adding docstrings across 51 touched existing functions
would broaden this migration without protecting the requested adapter
behavior, so no change is required.
- The remaining raw provider consumers are intentionally unchanged for
#9813.
- The diagnostic parser could not be removed entirely because the CLI
adapter still needs exact absence classification. It is no longer an
onboarding shim and now lives at the adapter boundary.
- The branch includes canonical `main` through `d836ccb44`, including
the remediation for the inherited `fast-uri` advisories, the
package-contract npm fix from #10986, the stricter existing-profile
validation protocol from #10884, and the Advisor on-demand diff-reading
fix from #10952.
- #10724 and #10726 are broader drafts stacked on the closed #10719
branch. This PR does not close or modify them.

---
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>

---------

Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants