fix(policies): preset application for versionless policies (Fixes #35)

[deepujain]

Summary

Preset application now correctly handles existing policies that have content but no version: header (versionless policies). The merged policy always includes a version header so OpenShell accepts it.

Fixes #35.

Changes

• bin/lib/policies.js
  • Extracted merge logic into a pure function mergePresetIntoPolicy(currentPolicy, presetEntries) so it can be unit-tested.
  • When the current policy has content but no version header, the merged result is prefixed with version: 1\n.
  • When the current policy has a network_policies: section but no version (e.g. from an older or external source), the merged result is given a version header.
  • Exported extractPresetEntries, parseCurrentPolicy, and mergePresetIntoPolicy for tests.
• test/policies.test.js
  • Added mergePresetIntoPolicy describe block with 4 regression tests: versionless content-only policy, versionless policy with network_policies, existing version preserved, empty current policy.

Testing

• npm test — 43 tests pass (39 existing + 4 new).

Summary by CodeRabbit

• Bug Fixes

  • Improved policy merge behavior: missing version headers are normalized (defaults to "version: 1"), existing keys and explicit versions are preserved, and preset network entries are consistently appended even when policies are empty or previously lacked a network section.

• Tests

  • Added coverage for merge scenarios: empty policies, versionless policies, existing network policies, and explicit version preservation.

[wscurran]

I've examined the changes and see that this PR addresses a bug with versionless policies and extracts the merge logic into a testable function, which should improve the overall policy handling.

[deepujain]

Rebased onto latest main to resolve conflicts in bin/lib/policies.js and test/policies.test.js. Upstream added buildPolicySetCommand and buildPolicyGetCommand helpers; kept both alongside mergePresetIntoPolicy. All policy tests pass (buildPolicySetCommand, buildPolicyGetCommand, mergePresetIntoPolicy).

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Added exported helper mergePresetIntoPolicy(currentPolicy, presetEntries) to merge network_policies into existing YAML and ensure a version: header when missing. Refactored applyPreset to delegate merging to this helper and adjusted its behavior. Added tests covering multiple merge scenarios.

Changes

Cohort / File(s)	Summary
Policy logic bin/lib/policies.js	Added and exported mergePresetIntoPolicy(currentPolicy, presetEntries) which injects/merges network_policies: and prepends version: 1 when the merged output lacks a version: header. Refactored applyPreset to load/validate preset content, fetch current policy (runCapture(..., { ignoreError: true })), parse it, and delegate YAML composition to mergePresetIntoPolicy. Removed prior inline merging/version-insertion branches and adjusted error/return behavior when no preset entries are present.
Tests test/policies.test.js	Added describe("mergePresetIntoPolicy") tests covering: non-empty policy without version: (prepends version: 1 and adds presets), policy with existing network_policies but no version: (preserves existing entries and appends presets), policy with existing version: (preserved), and empty policy input (creates version: 1 and network_policies: with presets).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through YAML lines at night,
I tucked in presets neat and tight,
When version: vanished from sight,
I placed version: 1 just right,
Policies now bound and light.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fixing preset application for versionless policies, and directly references the linked issue #35.
Linked Issues check	✅ Passed	The PR fully addresses all requirements from #35: handles versionless policies with content [#35], adds mergePresetIntoPolicy function for proper YAML merging with version headers, includes regression tests for versionless policies, and all tests pass via npm test.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing preset application for versionless policies: extraction of merge logic into mergePresetIntoPolicy, refactoring of applyPreset, and comprehensive test coverage for the new function.

_{✏️ 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]

🧹 Nitpick comments (1)

bin/lib/policies.js (1)

166-172: Consider logging or commenting on the swallowed exception.

The empty catch {} block silently swallows any error from runCapture. While this is intentional (treating policy-get failures as "no existing policy"), an inline comment explaining the intent or a debug log would improve clarity for future maintainers.

📝 Suggested improvement

   let rawPolicy = "";
   try {
     rawPolicy = runCapture(
       buildPolicyGetCommand(sandboxName),
       { ignoreError: true }
     );
-  } catch {}
+  } catch {
+    // Policy may not exist yet — treat as empty policy
+  }

🤖 Prompt for AI Agents

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

In `@bin/lib/policies.js` around lines 166 - 172, The empty catch around the
runCapture call that sets rawPolicy silently swallows errors; add either a brief
inline comment clarifying this is intentional (treating policy-get failures as
"no existing policy") or log the caught error at debug/trace level using the
same logger used elsewhere, referencing the
runCapture(buildPolicyGetCommand(sandboxName)) call and the rawPolicy variable
so maintainers understand the intent and can diagnose unexpected failures.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@bin/lib/policies.js`:
- Around line 166-172: The empty catch around the runCapture call that sets
rawPolicy silently swallows errors; add either a brief inline comment clarifying
this is intentional (treating policy-get failures as "no existing policy") or
log the caught error at debug/trace level using the same logger used elsewhere,
referencing the runCapture(buildPolicyGetCommand(sandboxName)) call and the
rawPolicy variable so maintainers understand the intent and can diagnose
unexpected failures.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 615337ac-38a7-4bf9-8028-0dbca6657634

📥 Commits

Reviewing files that changed from the base of the PR and between dbfd78c and be97a12.

📒 Files selected for processing (2)

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

[deepujain]

Addressed CodeRabbit review comment

[coderabbitai]

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)

bin/lib/policies.js (1)

101-111: ⚠️ Potential issue | 🟡 Minor

Match network_policies as a YAML key, not a substring.

Line 101 and Line 110 use broad string checks; these can mis-detect comments/scalars and route merge logic incorrectly. Prefer top-level-key detection to avoid malformed output.

Suggested hardening

-  if (currentPolicy.includes("network_policies:")) {
+  if (/^network_policies\s*:/m.test(currentPolicy)) {
...
-      if (line.trim() === "network_policies:" || line.trim().startsWith("network_policies:")) {
+      if (/^network_policies\s*:/.test(line)) {

🤖 Prompt for AI Agents

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

In `@bin/lib/policies.js` around lines 101 - 111, currentPolicy is being detected
with broad string checks which can match substrings in comments or scalars;
replace those checks with top-level YAML key detection by using a regex that
matches a line that starts with optional indentation then the literal
"network_policies:" as a key (e.g. /^\s*network_policies:\s*(#.*)?$/ or similar)
and use that same test inside the loop instead of line.trim().startsWith or
currentPolicy.includes; update the logic around inNetworkPolicies, isTopLevel
and the line-match condition so you only enter/exit the network_policies block
when a true top-level key is seen (respecting indentation via isTopLevel) to
avoid false positives.

🤖 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 167-174: The catch block around
runCapture(buildPolicyGetCommand(sandboxName)) is too broad and masks real
errors; update the handler so that runCapture errors are inspected and only
treated as "policy missing" when the command indicates a not-found condition
(e.g., specific exit code or stderr message), while other errors are rethrown or
logged and cause failure. Locate the call to runCapture and
buildPolicyGetCommand and change the catch to check the thrown error's
properties (error.code, error.exitCode, error.stderr/message) and only set
rawPolicy to empty for the not-found case; for all other errors rethrow or call
processLogger.error and exit.

---

Outside diff comments:
In `@bin/lib/policies.js`:
- Around line 101-111: currentPolicy is being detected with broad string checks
which can match substrings in comments or scalars; replace those checks with
top-level YAML key detection by using a regex that matches a line that starts
with optional indentation then the literal "network_policies:" as a key (e.g.
/^\s*network_policies:\s*(#.*)?$/ or similar) and use that same test inside the
loop instead of line.trim().startsWith or currentPolicy.includes; update the
logic around inNetworkPolicies, isTopLevel and the line-match condition so you
only enter/exit the network_policies block when a true top-level key is seen
(respecting indentation via isTopLevel) to avoid false positives.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4a6afca8-ecc3-4978-9843-cce971a01164

📥 Commits

Reviewing files that changed from the base of the PR and between be97a12 and 0feaea6.

📒 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

[deepujain]

Addressed second round of CodeRabbit review

[coderabbitai]

🧹 Nitpick comments (2)

bin/lib/policies.js (2)

96-98: Edge case discards currentPolicy when presetEntries is falsy.

If currentPolicy has content but presetEntries is falsy, this returns a fresh template and silently loses the existing policy. The caller guards against this (lines 161-164), but as a "pure function for testing," it should handle this case defensively.

Suggested defensive fix

 function mergePresetIntoPolicy(currentPolicy, presetEntries) {
-  if (!currentPolicy || !presetEntries) {
-    return "version: 1\n\nnetwork_policies:\n" + (presetEntries || "");
+  if (!presetEntries) {
+    // Nothing to merge — return current policy unchanged (or empty template)
+    return currentPolicy || "version: 1\n\nnetwork_policies:\n";
+  }
+  if (!currentPolicy) {
+    return "version: 1\n\nnetwork_policies:\n" + presetEntries;
   }

🤖 Prompt for AI Agents

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

In `@bin/lib/policies.js` around lines 96 - 98, The current conditional returns a
fresh template whenever either currentPolicy or presetEntries is falsy,
discarding a valid currentPolicy; update the logic in the function that contains
currentPolicy and presetEntries so it only returns the template when both are
missing, otherwise preserve and return currentPolicy when present, or return the
template plus presetEntries when only presetEntries exists—i.e., change the
guard around currentPolicy/presetEntries to handle three cases (both missing ->
template, currentPolicy present -> return currentPolicy, only presetEntries ->
template + presetEntries) and retain the existing return formatting using the
same string construction.

---

131-139: Potential duplicate version: keys when version exists but not at the document start.

The includes("version:") check at line 131 detects version anywhere in the document, but the final guard at line 137 only checks if the merged result starts with version:. If the original policy has a non-leading version: field, both checks can pass/fail in opposite ways, resulting in two version: keys at the root level.

This is an edge case since well-formed policies have version: at the top, but worth noting for robustness.

Suggested fix: unify detection logic

   } else {
-    const withVersion = currentPolicy.includes("version:")
-      ? currentPolicy
-      : "version: 1\n" + currentPolicy;
-    merged = withVersion + "\n\nnetwork_policies:\n" + presetEntries;
+    merged = currentPolicy + "\n\nnetwork_policies:\n" + presetEntries;
   }
 
   if (!merged.trimStart().startsWith("version:")) {
     merged = "version: 1\n" + merged;
   }

This delegates all version-prefixing to the final guard (lines 137-139), ensuring consistent behavior.

🤖 Prompt for AI Agents

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

In `@bin/lib/policies.js` around lines 131 - 139, The code currently uses
currentPolicy.includes("version:") to decide whether to prepend a version, then
later checks merged.trimStart().startsWith("version:") which can produce
duplicate top-level version keys if "version:" appears mid-document; update the
logic so detection is unified: stop using includes("version:") (or change it to
a startsWith-style check) and build merged from currentPolicy (and
presetEntries) without conditionally prepending there, then rely solely on the
final guard that uses merged.trimStart().startsWith("version:") to prepend
"version: 1\n" when missing; adjust the variables referenced (currentPolicy,
withVersion, merged, presetEntries) accordingly and remove the redundant branch
that used includes("version:").

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@bin/lib/policies.js`:
- Around line 96-98: The current conditional returns a fresh template whenever
either currentPolicy or presetEntries is falsy, discarding a valid
currentPolicy; update the logic in the function that contains currentPolicy and
presetEntries so it only returns the template when both are missing, otherwise
preserve and return currentPolicy when present, or return the template plus
presetEntries when only presetEntries exists—i.e., change the guard around
currentPolicy/presetEntries to handle three cases (both missing -> template,
currentPolicy present -> return currentPolicy, only presetEntries -> template +
presetEntries) and retain the existing return formatting using the same string
construction.
- Around line 131-139: The code currently uses
currentPolicy.includes("version:") to decide whether to prepend a version, then
later checks merged.trimStart().startsWith("version:") which can produce
duplicate top-level version keys if "version:" appears mid-document; update the
logic so detection is unified: stop using includes("version:") (or change it to
a startsWith-style check) and build merged from currentPolicy (and
presetEntries) without conditionally prepending there, then rely solely on the
final guard that uses merged.trimStart().startsWith("version:") to prepend
"version: 1\n" when missing; adjust the variables referenced (currentPolicy,
withVersion, merged, presetEntries) accordingly and remove the redundant branch
that used includes("version:").

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7a2ecb62-6468-4c67-9893-bed5a0f00a4c

📥 Commits

Reviewing files that changed from the base of the PR and between 0feaea6 and e2755e6.

📒 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

[deepujain]

Rebased onto latest main. Resolved merge conflict

[coderabbitai]

🧹 Nitpick comments (1)

bin/lib/policies.js (1)

138-138: Minor: potential extra blank lines when appending new section.

If currentPolicy already ends with one or more newlines, the concatenation produces extra blank lines. This is cosmetic and won't break functionality.

♻️ Optional: normalize trailing newlines

-    merged = currentPolicy + "\n\nnetwork_policies:\n" + presetEntries;
+    merged = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries;

🤖 Prompt for AI Agents

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

In `@bin/lib/policies.js` at line 138, The concatenation that builds merged uses
currentPolicy + "\n\nnetwork_policies:\n" + presetEntries and can create extra
blank lines if currentPolicy ends with newlines; trim or normalize trailing
newlines from currentPolicy (e.g., call a trimEnd/newline-normalizing helper)
before constructing merged so you always insert exactly one blank line before
"network_policies:"; update the merged assignment that references currentPolicy
and presetEntries to use the normalized currentPolicy.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@bin/lib/policies.js`:
- Line 138: The concatenation that builds merged uses currentPolicy +
"\n\nnetwork_policies:\n" + presetEntries and can create extra blank lines if
currentPolicy ends with newlines; trim or normalize trailing newlines from
currentPolicy (e.g., call a trimEnd/newline-normalizing helper) before
constructing merged so you always insert exactly one blank line before
"network_policies:"; update the merged assignment that references currentPolicy
and presetEntries to use the normalized currentPolicy.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 612384b9-9936-4eb8-8bb8-f6f619f8718d

📥 Commits

Reviewing files that changed from the base of the PR and between e2755e6 and 227ff0e.

📒 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

[deepujain]

Rebased on latest main, addressed the trailing-newline nitpick (trimEnd before concatenation), and converted the mergePresetIntoPolicy tests from assert to vitest expect. All tests pass. Should be good to go!

[coderabbitai]

🧹 Nitpick comments (1)

bin/lib/policies.js (1)

141-143: Edge case: leading comments could cause duplicate version headers.

If currentPolicy has content before the version: line (e.g., a leading comment), trimStart() removes only whitespace, so "# comment\nversion: 1\n..." still fails the check and gets version: 1\n prepended—resulting in duplicate version: keys.

Consider using a regex that searches for version: anywhere at the start of a line:

💡 Suggested fix

-  if (!merged.trimStart().startsWith("version:")) {
+  if (!/^version\s*:/m.test(merged)) {
     merged = "version: 1\n" + merged;
   }

🤖 Prompt for AI Agents

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

In `@bin/lib/policies.js` around lines 141 - 143, The current check uses
merged.trimStart().startsWith("version:") which fails when there are leading
comments or other non-whitespace characters before a version header (causing
duplicate version: lines); update the check around the merged variable (where
currentPolicy is combined) to test for a version header at the start of any line
using a multiline-aware regex (e.g., match /^\s*version:/m) or by scanning lines
for one that starts with "version:" and only prepend "version: 1\n" when no such
header is found; adjust the if condition that currently prepends the header so
it relies on this regex/line-scan check instead of
trimStart().startsWith("version:").

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@bin/lib/policies.js`:
- Around line 141-143: The current check uses
merged.trimStart().startsWith("version:") which fails when there are leading
comments or other non-whitespace characters before a version header (causing
duplicate version: lines); update the check around the merged variable (where
currentPolicy is combined) to test for a version header at the start of any line
using a multiline-aware regex (e.g., match /^\s*version:/m) or by scanning lines
for one that starts with "version:" and only prepend "version: 1\n" when no such
header is found; adjust the if condition that currently prepends the header so
it relies on this regex/line-scan check instead of
trimStart().startsWith("version:").

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f5e6b7fc-53f5-4aa0-bd94-1b5a5a493c1d

📥 Commits

Reviewing files that changed from the base of the PR and between 227ff0e and fc4e389.

📒 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

[senthilr-nv]

Tested on DGX Spark (ARM64, Ubuntu 24.04) against current main (c051cbb).

Bug reproduced on main: When the existing policy has network_policies: but no version: field, the merge path (lines 133–166 of applyPreset) produces YAML without version:, and openshell policy set rejects it with missing field 'version'. This matches the report in #991.

Fix verified: mergePresetIntoPolicy() correctly adds version: 1 as a safety net after all merge paths. Tested four scenarios:

• Versionless policy with network_policies: — version: 1 prepended ✅
• Empty/no policy — version: 1 added ✅
• Existing version: 2 preserved, not overwritten ✅
• Policy with content but no network_policies: and no version: — both added ✅

Full test suite passes: 497 tests green (33 suites, 2 skipped).

This also fixes #991.