fix(images): replay MCP discovery permissions - #8754
Merged
jyaunches merged 9 commits intoAug 10, 2026
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
…untime-permissions Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe change normalizes MCP tool-discovery runtime ownership and permissions in managed images. PR workflows reproduce permission drift and validate runtime behavior. Staging QA rebuilds a historical base and verifies the candidate Deep Agents Code image contract. ChangesDiscovery runtime hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StagingQA
participant HistoricalBase
participant CandidateImage
participant DiscoveryRuntime
StagingQA->>HistoricalBase: rebuild pinned historical base
StagingQA->>CandidateImage: build candidate image against rebuilt base
CandidateImage->>DiscoveryRuntime: execute discovery runtime
DiscoveryRuntime-->>StagingQA: return protocol and invalid-arguments error
StagingQA->>CandidateImage: inspect image ID, base layers, ownership, and modes
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 |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit cef961c in the TypeScript / code-coverage/cliThe overall coverage in commit cef961c in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 4 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
Blockers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/managed-images.yaml:
- Around line 189-200: Update the layer inheritance jq invocation in the managed
image workflow to use null-input mode with -n, since its filter relies
exclusively on --argjson variables and should not read stdin. Preserve the
existing filter and failure behavior, and extend the relevant workflow contract
assertion in managed-image-publication-workflow.test.ts to verify the jq command
includes -n.
- Line 166: Update the Docker build arguments in the managed-images workflow to
use the declared NEMOCLAW_INFERENCE_PROVIDER_ID argument with value inference,
or remove the argument if relying on the Dockerfile default; do not pass the
undeclared NEMOCLAW_PROVIDER_KEY argument.
🪄 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: 81e24c25-a508-40bc-8c51-4af626244c0a
📒 Files selected for processing (6)
.github/workflows/managed-images.yamlDockerfileagents/hermes/Dockerfileagents/langchain-deepagents-code/Dockerfiletest/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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/managed-image-publication-workflow.test.ts (1)
682-684: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire checkout before staging permission drift.
The staging assertions only require
driftto run beforebaseBuild. They do not requiredriftto run afterprCheckout, although the step uses thecandidatedirectory. Add the lower-bound ordering assertion. (raw.githubusercontent.com)As per path instructions, verify both boundaries of the observable workflow ordering.
Proposed assertion
expect(drift["working-directory"]).toBe("candidate"); + expect(steps.indexOf(drift)).toBeGreaterThan(steps.indexOf(prCheckout)); expect(steps.indexOf(drift)).toBeLessThan(steps.indexOf(baseBuild));🤖 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 682 - 684, Update the staging workflow ordering assertions around finalBuild, contract, and baseBuild to also require the drift step to occur after prCheckout, since drift uses the candidate directory. Verify both ordering boundaries: prCheckout before drift and drift before baseBuild, while preserving the existing assertions.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.
Inline comments:
In `@test/managed-image-publication-workflow.test.ts`:
- Around line 132-135: Update the inline validator extraction helper around
required(...) to trim the regex capture before validating it, then reject both
missing and whitespace-only captures. Preserve returning the trimmed validator
for non-empty matches, and add a regression case covering a whitespace-only
extracted validator.
---
Outside diff comments:
In `@test/managed-image-publication-workflow.test.ts`:
- Around line 682-684: Update the staging workflow ordering assertions around
finalBuild, contract, and baseBuild to also require the drift step to occur
after prCheckout, since drift uses the candidate directory. Verify both ordering
boundaries: prCheckout before drift and drift before baseBuild, while preserving
the existing assertions.
🪄 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: 05e2df69-6ae7-406a-838a-5dceac6e5a87
📒 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
+132
to
+135
| return required( | ||
| source.match(/if ! node -e '([\s\S]+?)' <<< "\$actual_discovery_contract"/u)?.[1], | ||
| "managed-image workflow is missing or has an incomplete inline Node validator", | ||
| ).trim(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject an empty extracted validator.
required(...) rejects only an absent match. A whitespace-only match survives that call, then .trim() returns "". node -e "" exits successfully, so this helper can accept a workflow with no effective validator. Trim before the presence check and add a whitespace-only regression case. (raw.githubusercontent.com)
Proposed fix
function inlineNodeStdinValidator(source: string): string {
+ const validator = source.match(
+ /if ! node -e '([\s\S]+?)' <<< "\$actual_discovery_contract"/u,
+ )?.[1]?.trim();
return required(
- source.match(/if ! node -e '([\s\S]+?)' <<< "\$actual_discovery_contract"/u)?.[1],
+ validator || undefined,
"managed-image workflow is missing or has an incomplete inline Node validator",
- ).trim();
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return required( | |
| source.match(/if ! node -e '([\s\S]+?)' <<< "\$actual_discovery_contract"/u)?.[1], | |
| "managed-image workflow is missing or has an incomplete inline Node validator", | |
| ).trim(); | |
| const validator = source.match( | |
| /if ! node -e '([\s\S]+?)' <<< "\$actual_discovery_contract"/u, | |
| )?.[1]?.trim(); | |
| return required( | |
| validator || undefined, | |
| "managed-image workflow is missing or has an incomplete inline Node validator", | |
| ); |
🤖 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 132 - 135,
Update the inline validator extraction helper around required(...) to trim the
regex capture before validating it, then reject both missing and whitespace-only
captures. Preserve returning the trimmed validator for non-empty matches, and
add a regression case covering a whitespace-only extracted validator.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
Author
|
Review disposition for
No code change is warranted for these comments. The post-merge Brev/Launchable handoff remains intentionally deferred, as requested in #8665 and this PR description. |
jyaunches
deleted the
codex/mcp-discovery-runtime-permissions-replacement
branch
August 10, 2026 21:02
23 tasks
jyaunches
added a commit
that referenced
this pull request
Aug 11, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR updates the August 10, 2026 v0.0.106 release entry with gateway readiness fixes that merged after PR #8756. PRs #8765, #8767, and #8768 remain outside this entry because they are open and do not carry the `v0.0.106` release label. ## Changes - Document acceptance of OpenShell v0.0.101 `Server:` endpoint output and target-bound process tags when trusted listener evidence matches the configured gateway. - Document preservation of selected-gateway stale state so onboarding can reconcile a registered gateway when a gateway-scoped OpenShell status check cannot connect. - Record evidence-backed exclusions for internal image, startup, qualification, proxy-environment, CI, and test-harness changes in PRs #8754, #8609, #8762, #8432, #8766, and #8581. - Exclude PRs #8765, #8767, and #8768 because their changes are absent from `main` and the PRs do not carry the `v0.0.106` release label. The release entry must be updated after any of those PRs merges for v0.0.106. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] 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: `test/changelog-docs.test.ts` validates dated changelog SPDX placement, version headings, forbidden terms, and link form. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] 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: `docs-updated` - Evidence: `docs/changelog/2026-08-10.mdx`; an independent Codex Desktop subagent reviewed the writing rules and documentation style, terminology, structure, voice, code-sample presentation, links, source and test accuracy, release meaning, product scope, and evidence-backed exclusions at commit `190bf882c`. - Agent: Codex Desktop <!-- docs-review-head-sha: 190bf88 --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## 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 — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6 tests. - [ ] 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 a documentation-only release-entry update. - [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) — result: passed with 0 errors and 2 existing warnings. - [x] 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) — no page was added. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway readiness detection for OpenShell v0.0.101 endpoint output. * Process tags are now accepted only when they match trusted listener information for the configured gateway. * Preserved stale gateway status during connection failures to support accurate onboarding reconciliation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
23 tasks
This was referenced Aug 11, 2026
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
2 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 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
cef961c6ba411682a60819b02a9096b161374e8don a fresh branch.Independently supersedes the permission-fix scope of #8702. Its latest PR commit 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 staging run's effective final-image configuration through the declared 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
cef961c6bacross 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 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.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Security
Bug Fixes
Quality