[codex] fix(policy): remove deprecated tls termination directives#1821
Open
coder999999999 wants to merge 2 commits intoNVIDIA:mainfrom
Open
[codex] fix(policy): remove deprecated tls termination directives#1821coder999999999 wants to merge 2 commits intoNVIDIA:mainfrom
coder999999999 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved the deprecated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR completes the cleanup of deprecated tls: terminate directives from shipped policy YAML, aligning the repo’s policy blueprints with the current OpenShell policy schema and preventing WARN-level log noise (Fixes #1686).
Changes:
- Removed remaining
tls: terminateentries from blueprint preset policies and sandbox/agent policy YAML. - Updated Hermes agent policy additions to match the deprecation removal.
- Added a regression test that scans policy YAML directories to ensure
tls: terminatedoes not reappear.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/policies.test.ts | Adds a regression test that walks policy YAML trees and fails if tls: terminate is present. |
| nemoclaw-blueprint/policies/presets/pypi.yaml | Removes deprecated TLS termination directive from endpoints. |
| nemoclaw-blueprint/policies/presets/outlook.yaml | Removes deprecated TLS termination directive from endpoints. |
| nemoclaw-blueprint/policies/presets/npm.yaml | Removes deprecated TLS termination directive from endpoints. |
| nemoclaw-blueprint/policies/presets/jira.yaml | Removes deprecated TLS termination directive from endpoints. |
| nemoclaw-blueprint/policies/presets/huggingface.yaml | Removes deprecated TLS termination directive from endpoints. |
| nemoclaw-blueprint/policies/presets/brave.yaml | Removes deprecated TLS termination directive from endpoint. |
| nemoclaw-blueprint/policies/openclaw-sandbox.yaml | Removes deprecated TLS termination directive from sandbox policy endpoints. |
| agents/hermes/policy-additions.yaml | Removes deprecated TLS termination directive from Hermes-specific policy endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/policies.test.ts (1)
626-629: Harden the deprecated TLS check to catch YAML-equivalent variants.Line 627 currently checks only the exact literal
tls: terminate, so variants liketls: "terminate"(or spacing differences) can bypass this regression test.Diff suggestion
- for (const file of yamlFiles) { + const deprecatedTlsPattern = /^\s*tls\s*:\s*["']?terminate["']?(?:\s+#.*)?\s*$/m; + for (const file of yamlFiles) { const content = fs.readFileSync(file, "utf-8"); assert.equal( - content.includes("tls: terminate"), + deprecatedTlsPattern.test(content), false, `${path.relative(REPO_ROOT, file)} still contains tls: terminate`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.ts` around lines 626 - 629, The test currently checks only for the exact substring "tls: terminate" using content.includes; change it to detect YAML-equivalent variants by replacing that includes check with a regex test against content (for example use /tls:\s*(['"])?terminate\1/ to match optional quotes and varying whitespace) so the assertion becomes something like assert.equal(/tls:\s*(['"])?terminate\1/.test(content), false, `${path.relative(REPO_ROOT, file)} still contains tls: terminate`), referencing the existing assert.equal, content and file variables shown in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/policies.test.ts`:
- Around line 626-629: The test currently checks only for the exact substring
"tls: terminate" using content.includes; change it to detect YAML-equivalent
variants by replacing that includes check with a regex test against content (for
example use /tls:\s*(['"])?terminate\1/ to match optional quotes and varying
whitespace) so the assertion becomes something like
assert.equal(/tls:\s*(['"])?terminate\1/.test(content), false,
`${path.relative(REPO_ROOT, file)} still contains tls: terminate`), referencing
the existing assert.equal, content and file variables shown in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83ba0d7c-f624-4493-90b8-2b6d39d1a0c6
📒 Files selected for processing (9)
agents/hermes/policy-additions.yamlnemoclaw-blueprint/policies/openclaw-sandbox.yamlnemoclaw-blueprint/policies/presets/brave.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/npm.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/pypi.yamltest/policies.test.ts
💤 Files with no reviewable changes (8)
- nemoclaw-blueprint/policies/presets/jira.yaml
- nemoclaw-blueprint/policies/presets/npm.yaml
- nemoclaw-blueprint/policies/presets/outlook.yaml
- nemoclaw-blueprint/policies/presets/brave.yaml
- nemoclaw-blueprint/policies/presets/pypi.yaml
- nemoclaw-blueprint/policies/presets/huggingface.yaml
- nemoclaw-blueprint/policies/openclaw-sandbox.yaml
- agents/hermes/policy-additions.yaml
wscurran
added
bug
Contributor
|
✨ Thanks for submitting this PR, which proposes a fix for a CI/CD issue and may improve the overall reliability of the test suite. Possibly related open issues: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/policies.test.ts (1)
608-634: Add a non-empty scan assertion to avoid a vacuous pass.If traversal finds zero YAML files, this regression still passes without validating deprecation usage. Add a guard assertion before running the regex checks.
Proposed patch
while (stack.length > 0) { const current = stack.pop(); for (const entry of fs.readdirSync(current, { withFileTypes: true })) { @@ } } } + expect(yamlFiles.length).toBeGreaterThan(0); + const deprecatedTlsPattern = /^\s*tls\s*:\s*["']?terminate["']?(?:\s+#.*)?\s*$/m;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.ts` around lines 608 - 634, The test currently collects YAML files into yamlFiles and then asserts none match deprecatedTlsPattern but can vacuously pass when yamlFiles is empty; add a guard assertion asserting yamlFiles.length > 0 (with a clear message e.g. "no YAML files found under REPO_ROOT to scan") immediately after the file discovery loop (after stack traversal and before iterating over yamlFiles) so the test fails if nothing was scanned; reference the variables yamlFiles, deprecatedTlsPattern and REPO_ROOT when adding this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/policies.test.ts`:
- Around line 608-634: The test currently collects YAML files into yamlFiles and
then asserts none match deprecatedTlsPattern but can vacuously pass when
yamlFiles is empty; add a guard assertion asserting yamlFiles.length > 0 (with a
clear message e.g. "no YAML files found under REPO_ROOT to scan") immediately
after the file discovery loop (after stack traversal and before iterating over
yamlFiles) so the test fails if nothing was scanned; reference the variables
yamlFiles, deprecatedTlsPattern and REPO_ROOT when adding this assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 151ce792-160b-4820-80f4-f21e6e1af398
📒 Files selected for processing (1)
test/policies.test.ts
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
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.
Fixes #1686.
What changed
tls: terminatedirectives from shipped policy YAMLnemoclaw-blueprint/policiesandagentsto keep the deprecated directive from returningWhy
tls: terminateis deprecated, and the repository still had several remaining occurrences outside the earlier messaging-preset cleanup. This finishes the cleanup so the shipped policy blueprints no longer pin the deprecated setting.Validation
npm run validate:configsnpm run lintnpm run typechecknpm run typecheck:clinpx vitest run test/policies.test.ts test/validate-blueprint.test.ts test/onboard.test.tsnpm run build:clinpm testSummary by CodeRabbit
Chores
Tests