fix(images): replay MCP discovery permissions - #8745
Closed
jyaunches wants to merge 4 commits into
Closed
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe Dockerfiles normalize ownership and permissions for the MCP tool-discovery runtime tree. The workflow reproduces permission drift and validates the built image. Contract tests verify failure handling, modes, ownership, content preservation, and symlink boundaries. ChangesDiscovery runtime permissions
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ImageBuild
participant DiscoveryRuntime
Workflow->>Workflow: Reproduce permission drift
Workflow->>ImageBuild: Build managed image
ImageBuild->>DiscoveryRuntime: Normalize ownership and modes
Workflow->>DiscoveryRuntime: Run discovery contract probe
DiscoveryRuntime-->>Workflow: Return permissions and invalid-argument results
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
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: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/support/managed-bootstrap-image-contract.ts (1)
95-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce exact Dockerfile text assertions.
These assertions require one specific command spelling and ordering. Equivalent safe implementations can fail this test without changing the image contract.
Keep the fixture execution checks for failure reporting, ownership, modes, content, and symlink scope. Move remaining structural requirements to built-image behavior checks where possible.
As per path instructions, “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/support/managed-bootstrap-image-contract.ts` around lines 95 - 110, Reduce the exact Dockerfile instruction assertions in the test covering mcp-tool-discovery and managed startup metadata, retaining only checks needed for fixture failure reporting and observable ownership, mode, content, and symlink behavior. Remove assertions that depend on a specific command spelling or ordering, and move structural requirements to built-image behavior checks where feasible, following the public-boundary path guidance.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/support/managed-bootstrap-image-contract.ts`:
- Around line 95-110: Reduce the exact Dockerfile instruction assertions in the
test covering mcp-tool-discovery and managed startup metadata, retaining only
checks needed for fixture failure reporting and observable ownership, mode,
content, and symlink behavior. Remove assertions that depend on a specific
command spelling or ordering, and move structural requirements to built-image
behavior checks where feasible, following the public-boundary path guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8fb8dd1d-980d-42df-a320-8ccfb8982d82
📒 Files selected for processing (3)
.github/workflows/managed-images.yamltest/managed-image-publication-workflow.test.tstest/support/managed-bootstrap-image-contract.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
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 `@test/managed-image-publication-workflow.test.ts`:
- Around line 491-494: Update the workflow response validator tested by the
assertions around contractSource to reject any keys outside the complete allowed
response shape, while preserving validation of protocol, ok, and detail. Replace
the invalid NaN-based extra-field fixture near the relevant test cases with
valid JSON such as an extra boolean property, and assert that the validator
rejects it so the test proves unknown-field behavior rather than JSON parsing
failure.
🪄 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: 6db83399-4fca-4268-9af4-747336b0ba5c
📒 Files selected for processing (2)
.github/workflows/managed-images.yamltest/managed-image-publication-workflow.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/managed-images.yaml
Comment on lines
+491
to
+494
| expect(contractSource).toContain('result = JSON.parse(require("node:fs").readFileSync(0'); | ||
| expect(contractSource).toContain("record.protocol !== expected.protocol"); | ||
| expect(contractSource).toContain("record.ok !== expected.ok"); | ||
| expect(contractSource).toContain("record.detail !== expected.detail"); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject valid extra fields in the contract test.
Line 577 uses NaN, which is invalid JSON. JSON.parse rejects it before the field validator runs. A valid extra property, such as "extra":true, currently passes because the validator checks only three required fields.
Define the complete allowed response shape. Reject unknown keys in the workflow validator. Add a valid extra-field fixture to this test.
Proposed test fixture change
- '{"protocol":1,"ok":false,"detail":"tool discovery received invalid runtime arguments","extra":NaN}\n',
+ '{"protocol":1,"ok":false,"detail":"tool discovery received invalid runtime arguments","extra":true}\n',As per path instructions, tests must prove observable behavior rather than a parser failure.
Also applies to: 562-569, 575-578
🤖 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/managed-image-publication-workflow.test.ts` around lines 491 - 494,
Update the workflow response validator tested by the assertions around
contractSource to reject any keys outside the complete allowed response shape,
while preserving validation of protocol, ok, and detail. Replace the invalid
NaN-based extra-field fixture near the relevant test cases with valid JSON such
as an extra boolean property, and assert that the validator rejects it so the
test proves unknown-field behavior rather than JSON parsing failure.
Source: Path instructions
wscurran
added
area: packaging
23 tasks
jyaunches
added a commit
that referenced
this pull request
Aug 10, 2026
<!-- markdownlint-disable MD041 --> ## Summary [Exact staging run 31396519688](https://github.com/brevdev/nemoclaw-image/actions/runs/31396519688) showed that Docker preserved mode `0664` from the build context when it copied the reviewed MCP discovery runtime. Replay root ownership and read-only modes in each final image before bundle execution, while keeping the existing fail-closed tree assertion unchanged. Add a PR-only Deep Agents Code regression that rebuilds the exact staging source and effective Docker configuration without claiming that the unavailable historical image was reproduced byte-for-byte. ## Related Issue Fixes #8665. Supersedes #8745. GitHub accepted that PR's final fast-forward branch update but did not synchronize its pull ref, commit list, or PR checks. This replacement carries that GitHub-Verified history plus the review repairs at commit `cef961c6ba411682a60819b02a9096b161374e8d` on a fresh branch. Independently supersedes the permission-fix scope of #8702. Its latest PR commit lost the permission-normalizing `COPY` options during conflict resolution and all three managed-image builds reproduce the `0664` failure. This PR uses a distinct legacy-builder-compatible final-image replay and does not transfer that PR's code, tests, managed-startup copy changes, or runtime-directory diagnostics. ## Changes - Replay `root:root` ownership, directory mode `0555`, and regular-file mode `0444` under `/usr/local/lib/nemoclaw/mcp-tool-discovery-runtime` in the OpenClaw, Hermes, and Deep Agents Code final images. - Use non-dereferencing traversal so the replay cannot change an out-of-tree link target, then run the unchanged `find -L` tree assertion after bundle validation. - Report a named permission-replay failure without changing the existing bundle, JSON, tree, or managed-startup diagnostics. - Execute the shared Dockerfile replay against the real reviewed bundle at mode `0664`, run the bundle afterward, and verify ownership, modes, bytes, failure behavior, and link scope for all three Dockerfiles. - Make the PR image workflow reproduce mode `0664` before each real managed-image build, then inspect the immutable final image for root ownership, `0555`/`0444` modes, and the exact bundle contract. - Rebuild the staging base from exact NemoClaw source `d097a22145859102c0495b0310de264b7a27624f`, apply the staging run's effective final-image configuration through the declared build arguments, prove layer ancestry, and run the final-image contract. The recorded `ceaa94...` index was local-only and unavailable, so the job treats it as provenance rather than an executable image identity. - Keep classic Docker builder compatibility by applying permissions in the final `RUN` layer instead of using `COPY --chmod` or `COPY --chown`. - Do not dispatch a Brev image build; exact staging validation remains a post-merge step. ## 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: This restores an internal managed-image file-permission invariant. It changes no public command, configuration, default, API, operator workflow, or successful user behavior. - [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: Codex Desktop independently reviewed commit `cef961c6b` across all nine security categories, including shell status handling, symlink scope, ownership and mode enforcement, output privacy, workflow authorization, and the historical-image limitation. It found no actionable security issue. - [ ] 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: The change repairs internal managed-image MCP tool-discovery filesystem metadata and adds CI-only regression coverage. It does not change public commands, configuration, defaults, APIs, or operator workflows; existing MCP tool-discovery documentation remains accurate. - Agent: Codex Desktop <!-- docs-review-head-sha: cef961c --> <!-- 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 — 68 tests passed across the managed-image workflow, MCP discovery image contract, and all three final-image layout contracts; the 19-test workflow contract was rerun after the final ordering assertion. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this scoped image-permission repair. `npm run validate:pr` passed for the review repair, including repository checks, source-shape budgets, Biome, hadolint, secret scanning, and CLI typecheck; the final assertion also passed normal commit and push hooks. - [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: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Security** - Managed images now enforce secure ownership and read-only permissions for the tool-discovery runtime. - Directories remain executable while files remain readable without being writable, and symlinks are handled safely. - **Bug Fixes** - Improved consistency and reliability when building managed images, including protection against permission drift. - **Quality** - Added automated validation for image security properties, runtime execution, and discovery response format. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Author
|
Closing as superseded by #8754, which merged the complete fix for v0.0.106. |
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
4 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
Exact staging run 31396519688 showed that Docker preserved mode
0664from the build context when it copied the reviewed MCP discovery runtime. Replay root ownership and read-only modes in each final image before bundle execution, while keeping the existing fail-closed tree assertion unchanged. Add a PR-only Deep Agents Code regression that rebuilds the exact staging source and recorded Docker recipe without claiming that the unavailable historical image was reproduced byte-for-byte.Related Issue
Fixes #8665.
PR #8702 independently overlaps this defect. Its current head lost the permission-normalizing
COPYoptions during conflict resolution and all three managed-image builds reproduce the0664failure. This PR uses a distinct legacy-builder-compatible final-image replay and does not transfer that PR's code, tests, managed-startup copy changes, or runtime-directory diagnostics.Changes
root:rootownership, directory mode0555, and regular-file mode0444under/usr/local/lib/nemoclaw/mcp-tool-discovery-runtimein the OpenClaw, Hermes, and Deep Agents Code final images.find -Ltree assertion after bundle validation.0664, run the bundle afterward, and verify ownership, modes, bytes, failure behavior, and link scope for all three Dockerfiles.0664before each real managed-image build, then inspect the immutable final image for root ownership,0555/0444modes, and the exact bundle contract.d097a22145859102c0495b0310de264b7a27624f, apply the recorded final-image build arguments, prove layer ancestry, and run the final-image contract. The recordedceaa94...index was local-only and unavailable, so the job treats it as provenance rather than an executable image identity.RUNlayer instead of usingCOPY --chmodorCOPY --chown.Type of Change
Quality Gates
7eeaefa24across all nine security categories, including shell status handling, symlink scope, ownership and mode enforcement, output privacy, workflow authorization, and the historical-image limitation. It found no actionable security issue.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 unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable to this scoped image-permission repair.npm run validate:prpassed on exact head7eeaefa24, including repository checks, source-shape budgets, Biome, hadolint, secret scanning, and CLI typecheck.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com