fix(blueprint): use allowlist for plan.json persist and log - #3649
Closed
MayankSharmaCSE wants to merge 1 commit into
Closed
fix(blueprint): use allowlist for plan.json persist and log#3649MayankSharmaCSE wants to merge 1 commit into
MayankSharmaCSE wants to merge 1 commit into
Conversation
Signed-off-by: mayanksharmaCSE <mayanksharmacse1@gmail.com>
Contributor
📝 WalkthroughWalkthroughAdded ChangesPlan Data Sanitization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces ad-hoc field exclusion for plan persistence and logging with an explicit allowlist, so that secret-bearing fields on RunPlan (currently credential_env, plus future additions) can never leak into stdout via actionPlan, into state/plan.json via actionApply, or be echoed back by actionStatus.
Changes:
- Introduces a
SanitizedPlaninterface andsanitizePlan()helper enumerating the fields safe to persist/log. actionPlannow logssanitizePlan(plan)instead of the rawRunPlan.actionApplyconstructs aSanitizedPlanforplan.json, andactionStatusdefensively re-filters the file contents through a key allowlist before logging.
Comments suppressed due to low confidence (1)
nemoclaw/src/blueprint/runner.ts:758
- The allowlist of safe keys is hard-coded here as a string literal array, separate from the
SanitizedPlaninterface and fromsanitizePlan. If a new safe field is added toSanitizedPlanin the future, this list will silently drop it fromstatusoutput. Consider deriving this list from a single source of truth (e.g., a sharedconst SAFE_PLAN_KEYS = [...] as constused both for the type and the runtime filter), so all three call sites stay in sync.
const safe: Record<string, unknown> = {};
for (const key of ["run_id", "profile", "sandbox_name", "inference", "policy_additions", "timestamp"]) {
if (key in raw) safe[key] = raw[key];
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+692
to
+705
| const safeState: SanitizedPlan = { | ||
| run_id: rid, | ||
| profile, | ||
| sandbox_name: sandboxName, | ||
| inference: { | ||
| provider_type: inferenceCfg.provider_type, | ||
| provider_name: inferenceCfg.provider_name, | ||
| endpoint: inferenceCfg.endpoint, | ||
| model: inferenceCfg.model, | ||
| }, | ||
| policy_additions: policyAdditions, | ||
| timestamp: new Date().toISOString(), | ||
| }; | ||
| writeFileSync(join(stateDir, "plan.json"), JSON.stringify(safeState, null, 2)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nemoclaw/src/blueprint/runner.ts (2)
692-705: ⚡ Quick winConsider using
sanitizePlan()helper for consistency.The manual construction of
safeStateduplicates the sanitization logic fromsanitizePlan(). IfSanitizedPlanis extended in the future, both locations would need updates. Since all the required data (sandboxImage,forwardPorts, router config, etc.) is available in scope, you could construct aRunPlanobject and callsanitizePlan(plan, timestamp)to centralize the sanitization logic.♻️ Refactor to use sanitizePlan() helper
progress(85, "Saving run state"); - const safeState: SanitizedPlan = { - run_id: rid, - profile, - sandbox_name: sandboxName, - inference: { - provider_type: inferenceCfg.provider_type, - provider_name: inferenceCfg.provider_name, - endpoint: inferenceCfg.endpoint, - model: inferenceCfg.model, - }, - policy_additions: policyAdditions, - timestamp: new Date().toISOString(), - }; + const plan: RunPlan = { + run_id: rid, + profile, + sandbox: { + image: sandboxImage, + name: sandboxName, + forward_ports: forwardPorts, + }, + inference: { + provider_type: inferenceCfg.provider_type, + provider_name: inferenceCfg.provider_name, + endpoint: inferenceCfg.endpoint, + model: inferenceCfg.model, + credential_env: inferenceCfg.credential_env, + }, + router: { + enabled: blueprint.components?.router?.enabled === true, + port: blueprint.components?.router?.port ?? DEFAULT_ROUTER_PORT, + pool_config_path: blueprint.components?.router?.pool_config_path, + }, + policy_additions: policyAdditions, + dry_run: false, + }; + const safeState = sanitizePlan(plan, new Date().toISOString()); writeFileSync(join(stateDir, "plan.json"), JSON.stringify(safeState, null, 2));🤖 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 `@nemoclaw/src/blueprint/runner.ts` around lines 692 - 705, The manual construction of safeState duplicates sanitization logic; instead build a RunPlan object with the existing scope values (e.g. run_id rid, profile, sandbox_name sandboxName, inferenceCfg fields, policyAdditions, plus sandboxImage, forwardPorts, router config, etc.), then call sanitizePlan(plan, new Date().toISOString()) to produce the SanitizedPlan and write that JSON to join(stateDir, "plan.json") using writeFileSync; replace the safeState literal with the sanitizedPlan result and ensure you import/ensure availability of sanitizePlan and the RunPlan type where needed.
756-758: ⚡ Quick winLeverage type system for the field whitelist.
The hardcoded field list should be tied to the
SanitizedPlaninterface for type safety. IfSanitizedPlanis extended or modified, the current approach requires manually remembering to update this array. A type-annotated constant would catch mismatches at compile time.♻️ Add type annotation to the whitelist
try { const raw: Record<string, unknown> = JSON.parse( readFileSync(join(runDir, "plan.json"), "utf-8"), ); + const SAFE_KEYS: (keyof SanitizedPlan)[] = [ + "run_id", + "profile", + "sandbox_name", + "inference", + "policy_additions", + "timestamp", + ]; const safe: Record<string, unknown> = {}; - for (const key of ["run_id", "profile", "sandbox_name", "inference", "policy_additions", "timestamp"]) { + for (const key of SAFE_KEYS) { if (key in raw) safe[key] = raw[key]; } log(JSON.stringify(safe, null, 2));🤖 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 `@nemoclaw/src/blueprint/runner.ts` around lines 756 - 758, Replace the hardcoded string array with a typed whitelist so the compiler verifies it against the SanitizedPlan shape: declare a constant like const WHITELIST: Array<keyof SanitizedPlan> = ["run_id", "profile", "sandbox_name", "inference", "policy_additions", "timestamp"]; then iterate for (const key of WHITELIST) { if (key in raw) safe[key] = raw[key]; } referencing SanitizedPlan and the new WHITELIST constant in runner.ts to ensure future changes to SanitizedPlan surface type errors.
🤖 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 `@nemoclaw/src/blueprint/runner.ts`:
- Around line 692-705: The manual construction of safeState duplicates
sanitization logic; instead build a RunPlan object with the existing scope
values (e.g. run_id rid, profile, sandbox_name sandboxName, inferenceCfg fields,
policyAdditions, plus sandboxImage, forwardPorts, router config, etc.), then
call sanitizePlan(plan, new Date().toISOString()) to produce the SanitizedPlan
and write that JSON to join(stateDir, "plan.json") using writeFileSync; replace
the safeState literal with the sanitizedPlan result and ensure you import/ensure
availability of sanitizePlan and the RunPlan type where needed.
- Around line 756-758: Replace the hardcoded string array with a typed whitelist
so the compiler verifies it against the SanitizedPlan shape: declare a constant
like const WHITELIST: Array<keyof SanitizedPlan> = ["run_id", "profile",
"sandbox_name", "inference", "policy_additions", "timestamp"]; then iterate for
(const key of WHITELIST) { if (key in raw) safe[key] = raw[key]; } referencing
SanitizedPlan and the new WHITELIST constant in runner.ts to ensure future
changes to SanitizedPlan surface type errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59cdea0b-211b-41d6-bb2a-5eb2b0247ed3
📒 Files selected for processing (1)
nemoclaw/src/blueprint/runner.ts
Contributor
Collaborator
|
Closing in favor of #3674, which lands the same allowlist direction (and credits this PR in the body) but additionally:
|
ericksoa
added a commit
that referenced
this pull request
May 19, 2026
## Summary - remove `credential_env` from public blueprint plan output - persist `plan.json` through an explicit allowlist builder instead of ad-hoc credential exclusions - make `status` parse `plan.json` and re-render only safe allowlisted fields - add regression coverage for credential env names, defaults, secret values, token/auth fields, and future unallowlisted fields Fixes #3648 ## Related contributor work - Related PR #3649 by @MayankSharmaCSE independently proposed the explicit allowlist direction for plan logging and persisted `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 build` - `cd nemoclaw && npm run check` - `git diff --check` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Plan outputs and stored run plans no longer expose credential-related fields or secret values * **Tests** * Added comprehensive tests validating that sensitive credential information and secret values are not exposed in plan output, persisted plans, or status reports <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3674?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3674?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
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
actionPlan was logging credential_env to stdout and actionStatus was dumping the raw plan.json contents without any field filtering. Both paths relied on manually remembering to exclude sensitive fields — a pattern that breaks silently when new fields get added to RunPlan. This PR replaces that with an explicit allowlist so only safe fields ever get persisted or logged.
Related Issue
Closes #3648
Changes
SanitizedPlaninterface andsanitizePlan()helper that defines exactly which fields are safe to write to disk or print to stdoutactionPlannow logs the sanitized view instead of the full plan object (previously includedcredential_env)actionApplynow constructs aSanitizedPlandirectly instead of an inline object with a "don't forget to exclude secrets" comment — the type system enforces the boundaryactionStatusnow parses plan.json and filters to allowlisted keys before logging, instead of dumping raw file contentsType of Change
Verification
npm testpassesSigned-off-by: MayankSharmaCSE mayanksharmaCSE1@gmail.com