fix(images): report MCP discovery assertion failures - #8742
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe change hardens managed-image validation across three Dockerfiles and its contract test. It adds dereferenced symlink checks, explicit command-status diagnostics, defensive discovery JSON validation, secret redaction, and coverage for discovery and runtime-tree failures. ChangesManaged image validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedImageAssertions
participant MCPDiscovery
participant JSONContractValidator
participant RuntimeTreeScanner
ManagedImageAssertions->>MCPDiscovery: Execute discovery bundle
MCPDiscovery-->>JSONContractValidator: Return JSON output
JSONContractValidator-->>ManagedImageAssertions: Validate contract and sanitize diagnostics
ManagedImageAssertions->>RuntimeTreeScanner: Scan runtime tree
RuntimeTreeScanner-->>ManagedImageAssertions: Return command status and dereferenced metadata
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
![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: 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 `@Dockerfile`:
- Line 1518: Update the named-secret redaction matchers in Dockerfile:1518-1518,
agents/hermes/Dockerfile:380-380, and
agents/langchain-deepagents-code/Dockerfile:130-130 to redact any non-empty
value, removing the ten-character minimum while preserving the existing
field-name matching. In
test/support/managed-bootstrap-image-contract.ts:223-239, add negative-path
regression cases for short TOKEN and PASSWORD values and verify the resulting
JSON-contract errors do not expose those secrets.
🪄 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: c9805239-6371-47fd-a85f-a946e1426b9d
📒 Files selected for processing (4)
Dockerfileagents/hermes/Dockerfileagents/langchain-deepagents-code/Dockerfiletest/support/managed-bootstrap-image-contract.ts
| && discovery_unsafe="$(find -L /usr/local/lib/nemoclaw/mcp-tool-discovery-runtime \( ! -user root -o -perm /022 \) -print -quit)" \ | ||
| && test -z "$discovery_unsafe" \ | ||
| || managed_image_command_failed mcp-tool-discovery-bundle-execution "$?"; \ | ||
| node -e 'const expected = { protocol: 1, ok: false, detail: "tool discovery received invalid runtime arguments" }; const standaloneSecretPatterns = [/(?:nvapi-|nvcf-|gh[pousr]_|sk-proj-|sk-ant-|hf_|glpat-|gsk_|pypi-|tvly-)[A-Za-z0-9_-]{10,}/gu, /github_pat_[A-Za-z0-9_]{30,}/gu, /sk-[A-Za-z0-9_-]{20,}/gu, /(?:xox[bpas]|xapp)-[A-Za-z0-9-]{10,}/gu, /A(?:K|S)IA[A-Z0-9]{16}/gu, /\bbot\d{8,10}:[A-Za-z0-9_-]{35}\b/gu, /\b\d{8,10}:[A-Za-z0-9_-]{35}\b/gu, /\b[A-Za-z0-9]{24}\.[A-Za-z0-9_-]{6}\.[A-Za-z0-9_-]{27,}\b/gu, /lsv2_(?:pt|sk)_[A-Za-z0-9]{10,}(?:_[A-Za-z0-9]+)*/gu, /\beyJ[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]{2,}\.[A-Za-z0-9_-]{10,}\b/gu, /\b[A-Za-z0-9_=-]{32,}\b/gu]; const redactContextSecrets = (value) => value.replace(/\b(?:Bearer|Basic)\s+\S+/giu, "<REDACTED>").replace(/((?:^|[^A-Za-z0-9])(?:[A-Za-z0-9]{1,128}_(?:KEY|TOKEN|SECRET|CREDENTIAL|PASSWORD|PASSWD|PASS)|(?:X[-_])?API[-_]KEY|TOKEN|SECRET|CREDENTIAL|PASSWORD|PASSWD|PASS)["\x27]?(?:[ \t]{0,32}[=:][ \t]{0,32}|[ \t]{1,32})["\x27]?)[^\s"\x27]{10,}/giu, (_match, prefix) => prefix + "<REDACTED>").replace(/((?:^|[^A-Za-z0-9])(?:[A-Za-z0-9]{1,128}(?:Token|Secret|Credential)|[A-Za-z0-9]{0,128}(?:[Aa]ccess|[Rr]efresh|[Cc]lient|[Bb]earer|[Aa]uth|[Aa][Pp][Ii]|[Pp]rivate|[Ss]igning|[Ss]ession|[Bb]ot|[Aa]pp|[Rr]esolved)Key|[A-Za-z0-9]{1,128}(?:Password|Passwd|Pass))["\x27]?(?:[ \t]{0,32}[=:][ \t]{0,32}|[ \t]{1,32})["\x27]?)[^\s"\x27]{10,}/gu, (_match, prefix) => prefix + "<REDACTED>").replace(/((?:^|[^A-Za-z0-9])KEY["\x27]?(?:[ \t]{0,32}[=:][ \t]{0,32}|[ \t]{1,32})["\x27]?)[^\s"\x27]{10,}/gu, (_match, prefix) => prefix + "<REDACTED>"); const sanitize = (value) => { if (value === undefined) return "<missing>"; if (value === null || typeof value === "boolean" || typeof value === "number") return value; if (typeof value !== "string") return "<" + (Array.isArray(value) ? "array" : typeof value) + ">"; let printable = value.replace(/-----BEGIN (?:[A-Z0-9]+ )?PRIVATE KEY-----[\s\S]*/gu, "<REDACTED>").replace(/[^\x20-\x7e]/gu, "?"); for (const pattern of standaloneSecretPatterns) printable = printable.replace(pattern, "<REDACTED>"); printable = redactContextSecrets(printable); return printable.length <= 240 ? printable : printable.slice(0, 237) + "..."; }; let result; let parsed = true; try { result = JSON.parse(process.argv[1]); } catch { parsed = false; } const record = parsed && result !== null && typeof result === "object" && !Array.isArray(result) ? result : undefined; if (record && record.protocol === expected.protocol && record.ok === expected.ok && record.detail === expected.detail) process.exit(0); const actual = record ? { protocol: sanitize(record.protocol), ok: sanitize(record.ok), detail: sanitize(record.detail) } : parsed ? { type: result === null ? "null" : Array.isArray(result) ? "array" : typeof result, value: sanitize(result) } : { type: "invalid-json", preview: sanitize(process.argv[1]) }; console.error("ERROR: managed image assertion failed: mcp-tool-discovery-json-contract actual=%s expected=%s", JSON.stringify(actual), JSON.stringify(expected)); process.exit(1);' "$discovery_contract" \ |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact short values for named secret fields.
TOKEN=short and PASSWORD=secret do not match any current redaction pattern. The JSON-contract failure then writes those values to the Docker build log. Remove the {10,} minimum from each named-secret value matcher, while still requiring a non-empty value. Add regression cases for short TOKEN and PASSWORD values.
Dockerfile#L1518-L1518: redact non-empty short named secret values.agents/hermes/Dockerfile#L380-L380: apply the same redaction change.agents/langchain-deepagents-code/Dockerfile#L130-L130: apply the same redaction change.test/support/managed-bootstrap-image-contract.ts#L223-L239: add short named-secret regression cases.
As per path instructions, require negative-path tests that prove the boundary does not leak secrets in errors.
📍 Affects 4 files
Dockerfile#L1518-L1518(this comment)agents/hermes/Dockerfile#L380-L380agents/langchain-deepagents-code/Dockerfile#L130-L130test/support/managed-bootstrap-image-contract.ts#L223-L239
🤖 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 `@Dockerfile` at line 1518, Update the named-secret redaction matchers in
Dockerfile:1518-1518, agents/hermes/Dockerfile:380-380, and
agents/langchain-deepagents-code/Dockerfile:130-130 to redact any non-empty
value, removing the ten-character minimum while preserving the existing
field-name matching. In
test/support/managed-bootstrap-image-contract.ts:223-239, add negative-path
regression cases for short TOKEN and PASSWORD values and verify the resulting
JSON-contract errors do not expose those secrets.
Source: Path instructions
Contributor
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. 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. |
23 tasks
This was referenced Aug 10, 2026
wscurran
added
chore
prekshivyas
added a commit
that referenced
this pull request
Aug 11, 2026
## Summary The discovery-contract diagnostic added in #8742 redacts named secret values only when they contain at least 10 characters, so shorter values can reach CI stderr. This change redacts every matched nonempty named value while preserving the existing JSON contract, fail-closed behavior, and successful image build path. ## Related Issue Follow-up to #8742. Addresses the unresolved [short named-secret review finding](#8742 (comment)). ## Changes - Redact one-character and longer named credential values in each of the three managed-image Dockerfiles. - Execute one-character token and password contract failures through the shared image-contract helper and verify that stderr contains only the redacted diagnostic for all three Dockerfiles. ## 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 is an internal build-diagnostic redaction correction with no supported user-facing behavior change. - [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: Independent Codex security re-review of `d14fab714` passed with no publication blocker; it confirmed identical fail-closed behavior across all three Dockerfiles and executable redaction coverage through all three contract callers. - [ ] 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 only tightens internal diagnostic redaction and adds contract-test inputs; no public API, CLI, configuration, workflow, or documented behavior changes. - Agent: Codex Desktop <!-- docs-review-head-sha: d14fab7 --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - 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: `npm exec -- vitest run --project integration test/mcp-tool-discovery-image-contract.test.ts test/openclaw-final-image-layout.test.ts test/hermes-final-image-layout.test.ts test/langchain-deepagents-code-image.test.ts` passed (49 tests in 4 files). - [x] 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 this three-regex correction and two shared contract inputs; `npm run validate:pr` 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) --- 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 diagnostic redaction to hide context-based secrets regardless of value length. - Short credential values, including tokens and passwords, are now protected from appearing in managed-image diagnostic output. - Existing standalone secret detection and diagnostic validation remain unchanged. - **Tests** - Added coverage confirming that token and password assignments are properly redacted. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
23 tasks
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
Brev image run 31352614414 failed the shared managed-image
RUNwithout emitting a concrete assertion name. This change adds named, fail-closed diagnostics for MCP discovery bundle execution, JSON-contract validation, discovery-tree traversal, and the first unsafe path while preserving every existing predicate, assertion, and successful behavior. Contract mismatches report bounded, credential-redacted actual fields beside the expected contract.Related Issue
Related to #8665. PR #8702 addresses the image-metadata root cause and overlaps the same shared block; this PR does not close the issue.
Changes
findexit status before another command can overwrite it.Type of Change
Quality Gates
f67df5af4against the security rubric. The review found and verified fixes for canonical credential redaction andfind -Lreferent metadata, then reported PASS with no remaining publication blocker.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 exec -- vitest run --project integration test/mcp-tool-discovery-image-contract.test.ts test/openclaw-final-image-layout.test.ts test/hermes-final-image-layout.test.ts test/langchain-deepagents-code-image.test.ts— 4 files and 49 tests passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable because the change is limited to one managed-image contract helper and its three focused consumers.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit