fix(blueprint): allowlist safe run plan output - #3674
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRunPlan's public interface is tightened to exclude credential-bearing inference fields. New safe plan builders explicitly allowlist non-secret fields for public output, persistence, and status rendering. actionPlan, actionApply, and actionStatus are updated to use these builders instead of inline or ad-hoc credential exclusions. Comprehensive tests validate that no credential field names or secret values leak to stdout or persisted files. ChangesSecret field sanitization in run plans
Sequence DiagramsequenceDiagram
participant User
participant actionPlan
participant buildSafePublicRunPlan
participant stdout
participant actionApply
participant buildPersistedRunPlan
participant plan.json
participant actionStatus
participant buildStatusRunPlan
User->>actionPlan: nemoclaw plan
actionPlan->>buildSafePublicRunPlan: inferenceCfg, routerCfg, ...
buildSafePublicRunPlan-->>actionPlan: RunPlan (no credentials)
actionPlan->>stdout: JSON.stringify(safe plan)
User->>actionApply: nemoclaw apply
actionApply->>buildPersistedRunPlan: plan, inferenceCfg, ...
buildPersistedRunPlan-->>actionApply: PersistedRunPlan (allowlisted fields)
actionApply->>plan.json: writeFileSync(safe persisted plan)
User->>actionStatus: nemoclaw status
actionStatus->>plan.json: readFileSync()
plan.json-->>actionStatus: raw JSON (unknown)
actionStatus->>buildStatusRunPlan: unknown object
buildStatusRunPlan-->>actionStatus: StatusRunPlan (safe fields only)
actionStatus->>stdout: JSON.stringify(safe status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26009769049
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26010189361
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26072002555
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26103843685
|
…apes # Conflicts: # test/e2e/docs/parity-inventory.generated.json
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26116463793
|
cjagwani
approved these changes
May 19, 2026
Collaborator
There was a problem hiding this comment.
APPROVE.
Same direction as #3649 (explicit allowlist) but executed more thoroughly:
- Tightens
RunPlan.inferencetype itself (nemoclaw/src/blueprint/runner.ts:461-466) —credential_envis removed from the public interface, so it can't reappear by accident. - Single source of truth via four builders (
buildSafeInferencePlan,buildSafePublicRunPlan,buildPersistedRunPlan,buildStatusRunPlan) — no duplicate allowlists. actionStatusrecursively allowlists nestedsandbox.*androuter.*fields and falls back to{status: "unknown"}on malformedplan.json. Catches credentials in plans written by older versions.- +179 lines of regression tests covering credential names, env-var values, future-secret fields (
token,authorization), persisted schema shape, and the corrupt-JSON path. credential-sanitization-e2ePASSED in nightly (run 26103843685). Self-hosted sandbox/gateway/non-root smoke all green.
4 tasks
wscurran
added
area: cli
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
credential_envfrom public blueprint plan outputplan.jsonthrough an explicit allowlist builder instead of ad-hoc credential exclusionsstatusparseplan.jsonand re-render only safe allowlisted fieldsFixes #3648
Related contributor work
plan.json. This PR keeps that core approach while adding nested status sanitization, preserving the public plan/status fields requested in the issue, and adding regression tests.Validation
cd nemoclaw && npm test -- src/blueprint/runner.test.ts(91 passed)cd nemoclaw && npm run buildcd nemoclaw && npm run checkgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests