chore(openshell): trust v0.0.101 release identities - #8616
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughThe installer trust checks now use allowlisted Homebrew formulas and trusted sandbox-build identities. Tests cover OpenShell 0.0.101, malformed trust data, mismatched digests, duplicate entries, alternate releases, and parser mutations. ChangesOpenShell release trust
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer as Installer
participant Checker as check-installer-hash.sh
participant Formula as Homebrew formula
participant Parser as extract-installer-pins.mts
Installer->>Checker: Submit release and installer pins
Checker->>Formula: Download allowlisted formula
Formula-->>Checker: Return formula content
Checker->>Checker: Verify upstream digest and formula pin
Checker->>Parser: Validate extracted sandbox-build pins
Parser-->>Checker: Return trusted identity result
Checker-->>Installer: Accept or reject release identities
Possibly related issues
Possibly related PRs
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 |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 1aa6c2b in the TypeScript / code-coverage/cliThe overall coverage in commit 1aa6c2b in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/installer-hash-check.test.ts (1)
639-691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that anchored mutations actually change the source.
CHECKER_MUTATIONSandPARSER_MUTATIONSuseString.prototype.replacewith long literal anchors, for example"readonly -a OPENSHELL_RELEASE_FORMULA_ALLOWLIST=(\n"at line 651 and"const TRUSTED_SANDBOX_BUILD_PINS: readonly TrustedSandboxBuildPin[] = [\n"at line 682. If a later refactor changes those anchors,replacereturns the source unchanged and the mutation becomes a silent no-op.A silent no-op does not make the negative tests error. It makes them pass for the wrong reason. For example,
trusted-sandbox-alternate-versionat line 1146 expects failure. IftrustAlternateSandboxBuildsno-ops, the run still fails, but because the sandbox identities were never trusted, not because the release lacks an allowlist entry.
test/installer-sandbox-build-trust.test.tsalready guards this at lines 49-53. Apply the same guard here.♻️ Proposed guard for anchored replacements
+const replaceAnchored = (source: string, anchor: string, replacement: string): string => { + const mutated = source.replace(anchor, replacement); + if (mutated === source) { + throw new Error(`fixture mutation anchor no longer matches: ${anchor}`); + } + return mutated; +};Then route the anchored mutations through it, for example:
-const trustAlternateSandboxBuilds = (source: string): string => { - const digests = SYNTHETIC_SANDBOX_BUILD_DIGESTS; - return source.replace( - "const TRUSTED_SANDBOX_BUILD_PINS: readonly TrustedSandboxBuildPin[] = [\n", - `const TRUSTED_SANDBOX_BUILD_PINS: readonly TrustedSandboxBuildPin[] = [ - { required: false, sha256: "${digests[0]}", version: "9.9.9" }, - { required: false, sha256: "${digests[1]}", version: "9.9.9" }, -`, - ); -}; +const trustAlternateSandboxBuilds = (source: string): string => { + const digests = SYNTHETIC_SANDBOX_BUILD_DIGESTS; + return replaceAnchored( + source, + "const TRUSTED_SANDBOX_BUILD_PINS: readonly TrustedSandboxBuildPin[] = [\n", + `const TRUSTED_SANDBOX_BUILD_PINS: readonly TrustedSandboxBuildPin[] = [ + { required: false, sha256: "${digests[0]}", version: "9.9.9" }, + { required: false, sha256: "${digests[1]}", version: "9.9.9" }, +`, + ); +};Apply the same treatment to the
allowlisted-alternate-version,duplicate-trusted-formula,malformed-trusted-formula,mismatched-trusted-formula-url, andtrusted-formula-mismatchentries.🤖 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/installer-hash-check.test.ts` around lines 639 - 691, Add a shared checked-replacement helper in the installer hash-check tests that asserts each anchor is found and the resulting source changes, then use it for the anchored replacements in CHECKER_MUTATIONS and trustAlternateSandboxBuilds, including the allowlisted, duplicate, malformed, URL-mismatch, and digest-mismatch formula mutations. Preserve each mutation’s existing replacement behavior while preventing silent no-ops.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/installer-hash-check.test.ts`:
- Around line 639-691: Add a shared checked-replacement helper in the installer
hash-check tests that asserts each anchor is found and the resulting source
changes, then use it for the anchored replacements in CHECKER_MUTATIONS and
trustAlternateSandboxBuilds, including the allowlisted, duplicate, malformed,
URL-mismatch, and digest-mismatch formula mutations. Preserve each mutation’s
existing replacement behavior while preventing silent no-ops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 856b1b40-dbb4-438c-9b26-ee3384dc64dc
📒 Files selected for processing (4)
scripts/check-installer-hash.shscripts/checks/extract-installer-pins.mtstest/installer-hash-check.test.tstest/installer-sandbox-build-trust.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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Restore the required root-owned, immutable `0444` mode for the reviewed managed-startup runtime bundle in every managed agent image. The repair uses each final image's existing permission replay so legacy builders remain supported and promotion continues to fail closed. This repair blocks clean trusted main and therefore blocks publication of the draft-only trust-root bootstrap. It does not directly qualify #8616 or #8617. ## Changes - Require the managed-startup runtime bundle to be a regular, non-link file in the OpenClaw, Hermes, and Deep Agents Code final images. - Replay `root:root` ownership and mode `0444`, then require the final numeric stat to be `0:0:444`. - Preserve the reviewed bundle bytes, original builder-stage copy provenance, legacy-builder compatibility, six-variant validation, and aggregate promotion barrier. ## 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 - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: The supported managed-image workflow validator checks regular-file, non-link, and `0:0:444` invariants for all six agent/architecture candidates before aggregate promotion. Focused image-layout and workflow contracts passed for this commit. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This restores an existing internal image artifact invariant and changes no command, configuration, default, documented workflow, or supported surface. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Fresh independent correctness/security review of commit `591550ad54cc43b7b18982712b16d40902f41a29` passed all nine security categories with no findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Commit 591550a restores the existing root-owned, read-only managed-image runtime artifact contract in three Dockerfiles. It changes no command, configuration, default, documented workflow, or supported surface. - Agent: Codex Desktop <!-- docs-review-head-sha: 591550a --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## 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 — `47` focused image/layout/workflow tests passed; the targeted Deep Agents Code compatibility contract passed. - [ ] Applicable broad gate passed — Not applicable to this three-Dockerfile permission repair; `npm run checks:repository`, Dockerfile lint, reviewed-bundle verification, and the source-shape audit passed. - [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) Final resolution still requires the first automatic exact-main managed-image run after external merge to pass all six validation jobs and artifact receipts. No manual E2E run was dispatched. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Strengthened container image validation to ensure the managed startup runtime is present as a regular, non-symlink file. * Confirmed the runtime file is root-owned and has read-only permissions before startup checks complete. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/installer-sandbox-build-trust.test.ts (1)
129-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winA partial mutation can make this test pass for the wrong reason.
mutateSandboxBuildFunctionasserts only that the combined result differs from the original. The two chained.replacecalls hold hardcoded production digests. If the installer rotates the second digest, the first replace still matches, the combined assert still passes, and the assertion at Line 146 checks only the first digest. The test then reports success while it no longer exercises full self-authorization.Assert both replacements, and assert both unexpected identities.
As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
♻️ Proposed change to assert each replacement
it("prevents a selected release from self-authorizing replacement binaries", () => { const result = runParser((source) => - mutateSandboxBuildFunction(source, (functionSource) => - functionSource - .replace( - "a4b0c38ed90a6dd4b4f312ad3727824a25ec478d88d4e65d22a82377b18e6214", - ARBITRARY_SANDBOX_BUILD_DIGESTS[0], - ) - .replace( - "f60ce5b76e4dbd645f690c8519852d261c8cf6a70b5fc56db329a23d68bc7b2e", - ARBITRARY_SANDBOX_BUILD_DIGESTS[1], - ), - ), + mutateSandboxBuildFunction(source, (functionSource) => { + const first = functionSource.replace( + "a4b0c38ed90a6dd4b4f312ad3727824a25ec478d88d4e65d22a82377b18e6214", + ARBITRARY_SANDBOX_BUILD_DIGESTS[0], + ); + assert.notEqual(first, functionSource, "x86_64 digest must be replaced"); + const second = first.replace( + "f60ce5b76e4dbd645f690c8519852d261c8cf6a70b5fc56db329a23d68bc7b2e", + ARBITRARY_SANDBOX_BUILD_DIGESTS[1], + ); + assert.notEqual(second, first, "arm64 digest must be replaced"); + return second; + }), ); expect(result.status).toBe(1); expect(result.stderr).toContain("must use only base-trusted binary identities"); expect(result.stderr).toContain(`unexpected=[0.0.99:${"a".repeat(64)}`); + expect(result.stderr).toContain("b".repeat(64)); });🤖 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/installer-sandbox-build-trust.test.ts` around lines 129 - 147, Update the test mutation in “prevents a selected release from self-authorizing replacement binaries” so it verifies that each chained replacement actually changed the source, rather than relying on mutateSandboxBuildFunction’s combined-difference assertion. Also assert that stderr reports both mutated unexpected identities, ensuring the test exercises full self-authorization.Source: Path instructions
test/installer-hash-check.test.ts (2)
679-679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnchor the digest replacement to the v0.0.72 tuple.
source.replace(FORMULA_DIGEST, "0".repeat(64))replaces only the first occurrence of the digest string in the checker. The test at Line 1221 asserts a v0.0.72 mismatch, so the mutation must target that tuple. If a second allowlist tuple ever reuses this digest, or if the v0.0.72 tuple stops being first, the mutation silently hits the wrong entry and the assertion at Line 980 still passes. Use the same anchored form asmalformed-trusted-formula.♻️ Proposed anchored replacement
- "trusted-formula-mismatch": (source) => source.replace(FORMULA_DIGEST, "0".repeat(64)), + "trusted-formula-mismatch": (source) => + source.replace( + `v0.0.72/openshell.rb|${FORMULA_DIGEST}`, + `v0.0.72/openshell.rb|${"0".repeat(64)}`, + ),🤖 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/installer-hash-check.test.ts` at line 679, Update the “trusted-formula-mismatch” mutation to replace the digest only within the v0.0.72 tuple, using the same anchored replacement pattern as “malformed-trusted-formula” rather than replacing the first standalone FORMULA_DIGEST occurrence. Preserve the existing zero-digest value and mismatch assertion behavior.
236-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated sandbox-build fixture helpers in two test files. Both files define their own copies of
mutateSandboxBuildFunction,addSandboxBuildPins, and the v0.0.101 digest constants. The helpers encode thepinned_sandbox_build_versionshell grammar. Two copies can drift when that grammar changes, and one suite can then stop exercising its claim while still passing.
test/installer-hash-check.test.ts#L236-L251: moveaddSandboxBuildPins,mutateSandboxBuildFunction, andV00101_SANDBOX_BUILD_DIGESTSinto a shared test helper module, and import them here.test/installer-sandbox-build-trust.test.ts#L39-L72: delete the localmutateSandboxBuildFunctionandaddSandboxBuildPinsdefinitions and the duplicated digest constants at Lines 26-29, then import them from the shared helper module.🤖 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/installer-hash-check.test.ts` around lines 236 - 251, Move mutateSandboxBuildFunction, addSandboxBuildPins, and V00101_SANDBOX_BUILD_DIGESTS from test/installer-hash-check.test.ts lines 236-251 into a shared test helper module, then import them there. In test/installer-sandbox-build-trust.test.ts lines 39-72, remove the local helper implementations and duplicated digest constants and import the shared symbols instead; both sites must use the same helper implementation.
🤖 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/installer-hash-check.test.ts`:
- Line 679: Update the “trusted-formula-mismatch” mutation to replace the digest
only within the v0.0.72 tuple, using the same anchored replacement pattern as
“malformed-trusted-formula” rather than replacing the first standalone
FORMULA_DIGEST occurrence. Preserve the existing zero-digest value and mismatch
assertion behavior.
- Around line 236-251: Move mutateSandboxBuildFunction, addSandboxBuildPins, and
V00101_SANDBOX_BUILD_DIGESTS from test/installer-hash-check.test.ts lines
236-251 into a shared test helper module, then import them there. In
test/installer-sandbox-build-trust.test.ts lines 39-72, remove the local helper
implementations and duplicated digest constants and import the shared symbols
instead; both sites must use the same helper implementation.
In `@test/installer-sandbox-build-trust.test.ts`:
- Around line 129-147: Update the test mutation in “prevents a selected release
from self-authorizing replacement binaries” so it verifies that each chained
replacement actually changed the source, rather than relying on
mutateSandboxBuildFunction’s combined-difference assertion. Also assert that
stderr reports both mutated unexpected identities, ensuring the test exercises
full self-authorization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b06065e9-a9c6-4285-9c6c-3d72c5d2d29e
📒 Files selected for processing (4)
scripts/check-installer-hash.shscripts/checks/extract-installer-pins.mtstest/installer-hash-check.test.tstest/installer-sandbox-build-trust.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/checks/extract-installer-pins.mts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Accepted issues #8598 and #8590 authorize the release-trust work. The delta from the previously approved b8b9b94 contains two verified main commits and verified merge commit bb3c7ce; none changes the four PR files, whose blob identities remain unchanged. The combined change preserves active v0.0.99 selectors, keeps v0.0.101 identities dormant, binds formulas and standalone sandbox binaries to base-owned identities, and fails closed. The nine-category security review found no blocker. No documentation update is required because supported runtime behavior, selectors, configuration, defaults, and errors remain unchanged. No competing PR, unresolved review thread, current automated finding, compatibility issue, or regression blocker was found.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
2 participants
Summary
Establish base-trusted identities for the published OpenShell v0.0.101 release without changing NemoClaw's active v0.0.99 selection. The checker now binds checksum manifests, the Homebrew formula, and standalone sandbox fallback binaries to reviewed release identities so a later selector PR cannot authorize its own artifacts.
Related Issue
Fixes #8598
Changes
Type of Change
Quality Gates
6ace3b8d657d3d65589d9ce61fe4672400f30955returned PASS with no blocker. It independently recomputed the v0.0.101 artifacts and confirmed the base-owned formula tuple and standalone sandbox version/digest sets close both candidate self-authorization paths.Documentation Writer Review
no-docs-neededDGX 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 unavailablescripts/check-installer-hash.shpassed on the current v0.0.99 selection.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to dormant trust data and its verifier boundary.npm run validate:pr,npm run checks:repository,npm run typecheck:cli,npm run test:changed, test-size/source-shape checks, shfmt, and ShellCheck passed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Security
Release Support
Testing