fix(policies): deduplicate preset entries on re-apply to prevent inva…

[Junior00619]

Summary

Prevent policy-add from generating invalid YAML when a preset is applied more than once. The previous merge path appended preset entries into network_policies without removing existing keys, which could produce duplicate mapping keys; this change gives repeated preset application update semantics by replacing overlapping entries before insertion.

Related Issue

Fixes #1010

Changes

• add extractPolicyKeyNames() to identify top-level policy keys contributed by an incoming preset within network_policies
• add stripPolicyKeys() to remove overlapping existing entries before merging the updated preset content
• update mergePresetIntoPolicy() so re-applying a preset replaces prior entries instead of appending duplicate keys
• preserve the existing lightweight text-based merge approach and avoid introducing new YAML parsing dependencies
• maintain compatibility with the current preset file structure and indentation conventions used in the repository

Type of Change

• Code change for a new feature, bug fix, or refactor.
• Code change with doc updates.
• Doc only. Prose changes without code sample modifications.
• Doc only. Includes code sample changes.

Testing

• npx prek run --all-files passes (or equivalently make check).
• npm test passes.
• make docs builds without warnings. (for doc-only changes)

Additional validation:

• Verified re-applying the same preset no longer produces duplicate YAML mapping keys
• Confirmed merged output remains syntactically valid YAML
• Confirmed updated preset entries replace prior values as intended
• Confirmed existing one-time preset application behavior remains unchanged

Checklist

General

• I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md).
• I have read and followed the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). (for doc-only changes)

Code Changes

• Formatters applied — npx prek run --all-files auto-fixes formatting (or make format for targeted runs).
• Tests added or updated for new or changed behavior.
• No secrets, API keys, or credentials committed.
• Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs).

Doc Changes

• Follows the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). Try running the update-docs agent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docs catch up the docs for the new changes I made in this PR."
• New pages include SPDX license header and frontmatter, if creating a new page.
• Cross-references and links verified.

Summary by CodeRabbit

• Bug Fixes

  • Reapplying presets in policy files now replaces matching entries under network_policies instead of creating duplicates, preserves unrelated sections, and keeps same-named keys outside network_policies intact.

• Tests

  • Added tests verifying deduplication when re-merging presets, replacement of stale entries, scoping to network_policies, and preservation of same-named keys in other sections.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dd4cad45-c5bf-41c7-bfc5-6879b0fd0ef8

📥 Commits

Reviewing files that changed from the base of the PR and between 6de0b3e and 51d1e46.

📒 Files selected for processing (2)

• bin/lib/policies.js
• test/policies.test.js

✅ Files skipped from review due to trivial changes (1)

• test/policies.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• bin/lib/policies.js

---

📝 Walkthrough

Walkthrough

Adds text-based deduplication for merging network policy presets: introduces helpers to extract 2-space-indented mapping keys under network_policies: and to strip matching entries from an existing policy before inserting preset entries, so re-applying a preset replaces matching mapping entries instead of duplicating them. (≤50 words)

Changes

Cohort / File(s)	Summary
Policy Merge Deduplication bin/lib/policies.js	Added extractPolicyKeyNames(text) to collect 2-space-indented mapping keys under network_policies: and stripPolicyKeys(policyText, keysToRemove) to remove matching network_policies entries. Updated mergePresetIntoPolicy() to compute incoming preset keys, strip overlapping entries from the current policy, then insert preset entries; exported both new helpers.
Deduplication Test Coverage test/policies.test.js	Added tests verifying deduplication/update semantics: repeated preset application yields a single telegram_bot: key; stale network_policies.telegram_bot blocks are replaced (old endpoints removed, new endpoints present); deduplication scoped to network_policies and preserves same-named keys in other sections.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 I nibble keys and tidy stacks,
I chase away duplicate pacts,
I strip the old and sew the new,
One tidy YAML stitched for you,
Hooray — no duplicate tracks!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: deduplicating preset entries when re-applying presets to prevent invalid YAML output.
Linked Issues check	✅ Passed	The implementation fully addresses issue #1010: deduplicates preset entries by key within network_policies, prevents duplicate YAML mapping keys, and maintains the lightweight text-based approach without introducing new YAML parsing dependencies.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the preset merge deduplication issue identified in #1010; no unrelated modifications are present in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/lib/policies.js`:
- Around line 113-135: stripPolicyKeys currently deletes 2-space-indented keys
globally; restrict it to only remove keys inside the top-level network_policies
block by detecting when we're within that block and only applying the removeSet
there: when you see a top-level "network_policies:" key (e.g.
/^\s*network_policies:/ with no indent) set a flag (e.g. inNetworkPolicies) and
clear it when any other top-level key (/^\S/) appears, then only evaluate
2-space-indented keys against removeSet (keysToRemove) while inNetworkPolicies;
update/remove the existing skipping logic accordingly and keep returning the
joined result.

🪄 Autofix (Beta)

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: Pro

Run ID: b47836ad-e3c3-4d32-8e8c-95d000343b2b

📥 Commits

Reviewing files that changed from the base of the PR and between 6a7f438 and d9c7648.

📒 Files selected for processing (2)

• bin/lib/policies.js
• test/policies.test.js

[HagegeR]

Maybe you can use yq to simplify this?

[Junior00619]

Thanks for the suggestion! I considered a structured YAML approach, but the existing codebase intentionally uses text-based manipulation throughout policies.js (no YAML parsing dependencies). Introducing yq would add an external binary dependency that isn't used elsewhere in the project. The current approach stays consistent with the codebase conventions and keeps the fix small. Happy to revisit if the team decides to adopt yq more broadly.

[HagegeR]

Thanks for the suggestion! I considered a structured YAML approach, but the existing codebase intentionally uses text-based manipulation throughout policies.js (no YAML parsing dependencies). Introducing yq would add an external binary dependency that isn't used elsewhere in the project. The current approach stays consistent with the codebase conventions and keeps the fix small. Happy to revisit if the team decides to adopt yq more broadly.

@cv @ericksoa WDYT about a simpler, more maintenable yq approach here? Won't need a special case per policy going forward

[wscurran]

Thanks for the preset deduplication fix. The codebase has changed significantly since March 30 — including a full TypeScript migration and rework of the policy preset merge logic — so this will need a rebase on origin/main before we can review it. Please rebase and resolve any conflicts, and we'll take a look.

[wscurran]

Just a friendly nudge: we're still waiting on the rebase against the TypeScript migration. If we don't hear back within 7 days, we'll close this to keep the queue clean. Feel free to reopen any time if you're able to update.

[ericksoa]

Thanks @Junior00619 for the contribution here. This PR correctly targeted the duplicate policy preset merge problem and helped identify the right behavior: re-applying a preset should update existing policy entries instead of producing duplicate YAML keys.

We have since landed the broader fix in #1055, which resolved #1010 by moving the merge path to structured YAML parsing and preserving non-network policy sections while overriding matching network policy entries. Since that implementation supersedes this earlier text-based approach, I am closing this PR as superseded rather than rejected on merit.