fix(ci): harden trusted OpenShell release upgrades - #6744
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change derives OpenShell release versions from trusted installer and Brev pin tables, validates versioned release-manifest digests and asset mappings, and rejects unsafe tar archives before extraction. Tests cover allowlist drift, template mutations, archive layouts, and extraction behavior. ChangesRelease integrity controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InstallerAndBrev
participant ExtractInstallerPins
participant CheckInstallerHash
participant ReleaseManifests
participant Blueprint
InstallerAndBrev->>ExtractInstallerPins: provide pin tables
ExtractInstallerPins->>Blueprint: compare releaseVersion with max_openshell_version
ExtractInstallerPins-->>CheckInstallerHash: return releaseVersion and pinned assets
CheckInstallerHash->>ReleaseManifests: download checksum manifests
ReleaseManifests-->>CheckInstallerHash: return manifest contents
CheckInstallerHash->>CheckInstallerHash: verify manifest digests and pinned assets
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings 1 warning · 0 optional suggestionsWarningsThese merit maintainer attention but do not block by themselves.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/checks/extract-installer-pins.mts (1)
389-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared brace-matching walk.
functionDefinitionSourceRanges(389-430) duplicates the token/brace-depth traversal offunctionBodyRanges(351-387); they differ only in whether they return token index ranges or source-charSourceEdits. Since both drive security-critical decisions (pin extraction vs. template normalization), keeping two independent copies risks them silently disagreeing on what counts as a function definition. A single helper returning{definitionStart, bodyStart, bodyCursor}that both wrap would eliminate that drift risk.🤖 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 `@scripts/checks/extract-installer-pins.mts` around lines 389 - 430, Extract the shared function-definition and brace-matching traversal from functionBodyRanges and functionDefinitionSourceRanges into one helper returning definitionStart, bodyStart, and bodyCursor token indices. Update both callers to derive their existing token ranges or SourceEdit values from that helper, preserving their current outputs and validation for unmatched or unavailable ranges.test/brev-launchable-ci-cpu-checksum.test.ts (1)
381-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a filesystem existence check over parsing
sudoLog.This test confirms non-installation by regex-matching
fake.sudoLogfor aninstall -m 755 ...openshellline. Since the fakesudo installcopies the binary topath.join(fake.fakeBin, "openshell")(lines 172-177), a directfs.existsSync(path.join(fake.fakeBin, "openshell"))check would be a more robust observable-outcome assertion than parsing the logged command string, and is less coupled to the exactinstallinvocation syntax.♻️ Proposed diff
const tarCalls = fs.readFileSync(fake.tarLog, "utf-8"); expect(tarCalls).not.toMatch(/^xzf /m); - expect(fs.existsSync(fake.sudoLog) ? fs.readFileSync(fake.sudoLog, "utf-8") : "").not.toMatch( - /^install -m 755 .*openshell/m, - ); + expect(fs.existsSync(path.join(fake.fakeBin, "openshell"))).toBe(false);As per path instructions, tests should "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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/brev-launchable-ci-cpu-checksum.test.ts` around lines 381 - 404, Update the unsafe archive test around runLaunchable to verify non-installation by checking fs.existsSync(path.join(fake.fakeBin, "openshell")) is false. Remove the sudoLog regex assertion while preserving the existing status, output, and tar extraction checks.Source: Path instructions
test/runner.test.ts (1)
744-772: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated tar-mock case-statement into a shared helper.
The exact same ~29-line
tarmock (basename classification +-tzf/-tvzf/xzfhandling) is repeated verbatim in both the curl-direct and curl-fallback test setups. Consolidating into one shared shell-fragment/helper (e.g., a template string constant interpolated into both heredocs) would remove the risk of the two copies silently drifting apart as the production mapping evolves.Also applies to: 846-874
🤖 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/runner.test.ts` around lines 744 - 772, Extract the duplicated tar mock case statement, including archive basename classification and -tzf, -tvzf, and xzf handling, into one shared shell fragment or helper. Reuse that shared definition in both the curl-direct and curl-fallback test setups, preserving the existing behavior and avoiding separate copies that can drift.
🤖 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 `@scripts/checks/extract-installer-pins.mts`:
- Around line 389-430: Extract the shared function-definition and brace-matching
traversal from functionBodyRanges and functionDefinitionSourceRanges into one
helper returning definitionStart, bodyStart, and bodyCursor token indices.
Update both callers to derive their existing token ranges or SourceEdit values
from that helper, preserving their current outputs and validation for unmatched
or unavailable ranges.
In `@test/brev-launchable-ci-cpu-checksum.test.ts`:
- Around line 381-404: Update the unsafe archive test around runLaunchable to
verify non-installation by checking fs.existsSync(path.join(fake.fakeBin,
"openshell")) is false. Remove the sudoLog regex assertion while preserving the
existing status, output, and tar extraction checks.
In `@test/runner.test.ts`:
- Around line 744-772: Extract the duplicated tar mock case statement, including
archive basename classification and -tzf, -tvzf, and xzf handling, into one
shared shell fragment or helper. Reuse that shared definition in both the
curl-direct and curl-fallback test setups, preserving the existing behavior and
avoiding separate copies that can drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8822097e-4aed-4b26-a41d-796362ea3d88
📒 Files selected for processing (11)
.github/workflows/installer-hash-check.yamlscripts/brev-launchable-ci-cpu.shscripts/check-installer-hash.shscripts/checks/dependency-pins.tsscripts/checks/extract-installer-pins.mtsscripts/install-openshell.shtest/brev-launchable-ci-cpu-checksum.test.tstest/dependency-pins-check.test.tstest/install-openshell-version-check.test.tstest/installer-hash-check.test.tstest/runner.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/install-openshell-version-check.test.ts (1)
568-573: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoolean-short-circuit assertions are only meaningful for the unsafe branch.
expect(expected.unsafe && fs.existsSync(...)).toBe(false)and the similartar.logcheck reduce to a no-op tautology (expect(false).toBe(false)) wheneverexpected.unsafeisfalse, so these two lines assert nothing for thesafecase (presumably covered by the later download-content checks, per the line-range summary). The pattern works but is easy to misread as verifying both branches.♻️ Suggested clarity improvement
- expect(expected.unsafe && fs.existsSync(path.join(tmp, "local-bin", "openshell"))).toBe( - false, - ); - expect(expected.unsafe && /^xzf /m.test(fs.readFileSync(tarLog, "utf8"))).toBe(false); + if (expected.unsafe) { + expect(fs.existsSync(path.join(tmp, "local-bin", "openshell"))).toBe(false); + expect(/^xzf /m.test(fs.readFileSync(tarLog, "utf8"))).toBe(false); + }🤖 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-openshell-version-check.test.ts` around lines 568 - 573, Clarify the unsafe-branch assertions in the test around the result status checks by guarding the filesystem and tar-log validations with an explicit expected.unsafe conditional. Ensure the local OpenShell binary and extraction command are checked only when the archive is unsafe, while safe cases continue through their existing download-content assertions without tautological expect(false).toBe(false) checks.
🤖 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-openshell-version-check.test.ts`:
- Around line 568-573: Clarify the unsafe-branch assertions in the test around
the result status checks by guarding the filesystem and tar-log validations with
an explicit expected.unsafe conditional. Ensure the local OpenShell binary and
extraction command are checked only when the archive is unsafe, while safe cases
continue through their existing download-content assertions without tautological
expect(false).toBe(false) checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 448ca3c6-2213-497e-9177-c6e6ec847853
📒 Files selected for processing (3)
scripts/checks/extract-installer-pins.mtstest/install-openshell-version-check.test.tstest/installer-hash-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/checks/extract-installer-pins.mts
38 tasks
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
approved these changes
Jul 13, 2026
21 tasks
cv
pushed a commit
that referenced
this pull request
Jul 14, 2026
<!-- markdownlint-disable MD041 --> ## Summary Addresses the five release/nightly E2E blockers according to the evidence each failure left behind. The OpenShell version-pin fixture now supports the installer's archive validation calls, the Shields target restores lockdown after its intentional failure assertion, MCP patch failures preserve their real sandbox phase, and the Hermes rebuild target emits secret-safe phase and host-resource heartbeats. The Hermes failure was not the fixture's 45-minute rebuild timeout. GitHub ended the job after about 50 minutes because the hosted runner lost communication; the rebuild command began later in the scenario and could not yet have reached its own timeout. No final logs or artifacts survived, so the underlying memory, disk, CPU, network, or runner-termination trigger cannot be recovered from that run. Follow-up to #6744 and #6724. The source nightly had seven red jobs. A same-SHA rerun cleared the two accepted flakes, `hermes-shields-config` and `gateway-guard-recovery`, leaving five release blockers: `openshell-version-pin`, `shields-config`, `mcp-bridge`, `rebuild-hermes`, and `channels-stop-start (hermes)`. This PR fixes the two reproducible fixture defects, repairs the MCP phase evidence, and instruments the Hermes rebuild runner loss. `mcp-bridge` failed when Docker `stop` exceeded its 30-second client timeout after a successful image build, while `channels-stop-start (hermes)` was externally canceled before any assertion or artifact; neither has evidence for a causal retry or timeout change, so both still require a fresh green candidate run or a reproducible product defect. ## Changes - Model `tar -tzf`, `tar -tvzf`, and extraction in the hermetic OpenShell version-pin fixture so the installer can validate fake release archives before extracting them. - Restore Shields lockdown after the duplicate-down rejection assertion and register a strict relock cleanup that reports a failed command or missing lockdown confirmation before continuing destruction. - Parse modern `NAME CREATED PHASE` OpenShell rows with the shared sandbox-list parser so MCP Docker-patch diagnostics report `Provisioning` instead of the creation date, while retaining the canonical terminal `Evicted` phase. - Emit a one-minute Hermes rebuild heartbeat from setup through cleanup with the active phase, child-output age, memory, process RSS, workspace disk, and load average. The output observer records timestamps only and never forwards command output or credentials. - Map the changed OpenShell and Hermes live targets to their fast PR coverage in the mock/live parity manifest. - Keep installer recovery behavior, the 30-second destructive Docker-operation timeout, Shields behavior, sandbox destruction, and Hermes rebuild behavior unchanged; the 90-minute workflow limit and 45-minute rebuild-command limit are unchanged. - Keep the underlying `mcp-bridge` Docker timeout and `channels-stop-start (hermes)` cancellation as explicit green-evidence blockers instead of masking them with retries. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: all changes are internal E2E fixture, diagnostic, and parity corrections; no user-facing CLI, policy, lifecycle, installer, or Hermes contract changes. - [x] 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: the Shields security E2E target and secret-safe Hermes diagnostics changed; review is pending on the current revision and no waiver is requested. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: no waiver is requested; required PR CI is pending. ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — OpenShell version-pin live fixture (3/3); focused archive-safety installer tests (9/9); MCP Docker-patch and shared phase parsing (29/29); Hermes progress and cleanup tests (15/15); mock/live parity guard; Vitest project membership; source-shape check; CLI typecheck and build; Shields and Hermes live target collection. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable to narrow live-target fixture and diagnostic changes. An exploratory local E2E-support run passed 979 tests and hit 15 environment-specific failures (macOS Bash behavior, Node 26 warning output, and nested-run timeouts); required Linux CI remains authoritative. - [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) - [ ] 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) --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved sandbox status detection across modern list output formats, including evicted sandboxes. - Improved cleanup behavior so shield restoration failures are reported while subsequent cleanup continues. - Strengthened shield-state handling during test teardown. - **User Experience** - Added clearer progress and activity reporting during lengthy Hermes rebuild operations. - **Tests** - Expanded coverage for installer version pinning, sandbox lifecycle states, cleanup ordering, and rebuild progress reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
wscurran
added
area: ci
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
This prerequisite makes future OpenShell release bumps verifiable by code owned by the pull request base, while NemoClaw remains pinned to OpenShell
0.0.72. It also rejects unsafe release archives before extraction so the eventual0.0.82pin can change only reviewed release data rather than installer behavior.Related Issue
Related to #6379.
Changes
0.0.72; this PR does not perform the dependency upgrade.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablev0.0.72release-manifest check passed all 13 entries.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: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
Security Improvements
Bug Fixes
Tests