test(e2e): migrate security policy credential suites#3905
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a reusable Bash validation library, new e2e validation scripts (credentials, injection, policy, shields) that use it, updates suites.yaml to wire those steps, refactors credentials-present to call the library, adds Vitest tests, and updates parity-map assertion IDs for credential checks. ChangesE2E Security Policy & Credential Coverage Migration
Sequence DiagramsequenceDiagram
participant ValidationScript as validation script
participant SharedLib as lib/security_policy_credentials.sh
participant HostFS as Host FS (~/.nemoclaw/)
participant Stdout as Test Output
ValidationScript->>SharedLib: source library (guard check)
ValidationScript->>Stdout: emit assertion ID
ValidationScript->>SharedLib: call assertion (e.g., spc_assert_no_plaintext_host_store)
SharedLib->>HostFS: check ~/.nemoclaw/credentials.json (exists/read)
SharedLib->>Stdout: emit redacted assertion result
participant InjectionScript as injection script
participant Context as $E2E_CONTEXT_DIR/context.env
InjectionScript->>SharedLib: source library
InjectionScript->>SharedLib: spc_context_get E2E_TELEGRAM_PAYLOAD_FIXTURE
SharedLib->>Context: read context variable
InjectionScript->>Stdout: log payload size and dry-run notice
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used the provided trusted metadata and diff; no scripts, package-manager commands, or tests were executed.; The parity-map diff is truncated, so full classification of every legacy assertion could not be independently verified.; Issue #3815 has no comments in the provided context; acceptance coverage is based on the issue body only.; E2E Advisor content was found, but the latest current-head E2E recommendation check is still in progress, so current-head E2E advisor status is ambiguous.; CI, mergeability, and review-thread state are point-in-time from the provided GitHub context and may change after this review. Full advisor summaryPR Review AdvisorBase: Blocked by current hard gates: CI is still pending, GitHub mergeStateStatus is BLOCKED, and 2 review threads remain unresolved; the current patch improves several security suite assertions but still needs human review of migrated E2E semantics. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Contributor
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/validation_suites/suites.yaml (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd SPDX license header to this YAML file.
This file is missing the required SPDX copyright/license header.
As per coding guidelines, `Every source file must include an SPDX license header for copyright and Apache-2.0 license`.Suggested patch
+ # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + # SPDX-License-Identifier: Apache-2.0 + suites:🤖 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 `@test/e2e/validation_suites/suites.yaml` at line 1, This YAML file is missing the required SPDX license header; add the standard SPDX header comment at the top of the file (before the top-level key "suites:") containing the copyright owner and the Apache-2.0 license identifier (e.g., "SPDX-FileCopyrightText: <copyright holder>" and "SPDX-License-Identifier: Apache-2.0") so the header precedes the existing "suites:" entry.
🤖 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.
Inline comments:
In `@test/e2e/validation_suites/lib/security_policy_credentials.sh`:
- Around line 42-63: spc_assert_credentials_expected currently only runs
"nemoclaw credentials list" and succeeds on exit code rather than asserting that
at least one gateway credential is present; modify
spc_assert_credentials_expected to capture the command output (after piping
through spc_redact_secret_text), and if the expected state is "present" verify
the output contains at least one credential entry (e.g., non-empty output or
matches the gateway credential entry pattern) and return non-zero with a clear
error message if none found; keep the dry-run path and existing context checks
(spc_require_context, spc_log_provider_metadata) unchanged and use the existing
assertion id "post-onboard.credentials.gateway-list-redacts-values" to maintain
semantics.
- Around line 76-84: spc_assert_policy_preset_present is currently a no-op in
non-dry runs; update it so after calling spc_assertion_id and
spc_require_context it actually verifies the preset and fails the test if
missing: when e2e_env_is_dry_run is true keep the current echo, otherwise query
the system for the applied policy preset (using the same CLI/API the suite uses
for other assertions), compare the result to the expected "${preset}", and if
they differ call the test failure helper (or exit non‑zero) so the suite fails;
keep function name spc_assert_policy_preset_present and existing calls to
spc_assertion_id and e2e_env_is_dry_run.
In
`@test/e2e/validation_suites/security/injection/00-telegram-message-not-shell-executed.sh`:
- Around line 8-12: The test currently only logs the payload and exits; update
the script so that when e2e_env_is_dry_run returns false it actually submits the
payload (use the existing payload variable) to the system under test (e.g. via
your standard test helper or an HTTP POST) and then assert expected safe
behavior instead of just printing length: call the appropriate assertion helper
(e.g. spc_assert_* or a new assertion) to fail if the response or side-effect
shows shell-evaluated output or unexpected command execution; keep
spc_assertion_id and spc_require_context intact and ensure failures produce a
non-zero exit so regressions are enforced.
In
`@test/e2e/validation_suites/security/policy/01-openshell-version-supports-credential-rewrite.sh`:
- Around line 8-10: The script currently only echoes a message in
e2e_env_is_dry_run and never verifies OpenShell's capability in real runs;
update the block after spc_require_context to perform an actual capability check
when e2e_env_is_dry_run is false: call the appropriate gateway-capability
lookup/validation routine (e.g., the project helper that queries gateway
metadata or a function you add such as verify_openshell_capability_support) to
assert that the OpenShell gateway advertises "credential-rewrite" support and
exit non‑zero (failing the test) if the capability or required version is
absent; keep the dry-run echo for e2e_env_is_dry_run, but replace the no-op path
with the real check referenced above.
In `@test/e2e/validation_suites/security/shields/00-config-consistent.sh`:
- Around line 8-10: The test is non-enforcing: it only prints a dry-run message
and never compares shield configuration, so update the script to perform an
actual consistency check instead of just echoing when using spc_assertion_id and
spc_require_context; call the runtime helper e2e_env_is_dry_run to skip real
verification only in dry-run mode, otherwise load the expected shields config
(e.g. from the test fixture or canonical source), fetch the current deployed
shields config (via the appropriate CLI/helper used elsewhere in the suite),
perform a deterministic comparison, and fail the script (non-zero exit and/or
use the suite's assert helper) when differences are detected so regressions are
caught.
---
Outside diff comments:
In `@test/e2e/validation_suites/suites.yaml`:
- Line 1: This YAML file is missing the required SPDX license header; add the
standard SPDX header comment at the top of the file (before the top-level key
"suites:") containing the copyright owner and the Apache-2.0 license identifier
(e.g., "SPDX-FileCopyrightText: <copyright holder>" and
"SPDX-License-Identifier: Apache-2.0") so the header precedes the existing
"suites:" entry.
🪄 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: Enterprise
Run ID: d438b12c-d741-4656-a877-5690f9003a87
📒 Files selected for processing (11)
test/e2e/docs/parity-map.yamltest/e2e/scenario-framework-tests/e2e-lib-helpers.test.tstest/e2e/scenario-framework-tests/e2e-suite-runner.test.tstest/e2e/validation_suites/lib/security_policy_credentials.shtest/e2e/validation_suites/security/credentials/00-credentials-present.shtest/e2e/validation_suites/security/credentials/01-no-plaintext-host-store.shtest/e2e/validation_suites/security/injection/00-telegram-message-not-shell-executed.shtest/e2e/validation_suites/security/policy/00-telegram-preset-applied.shtest/e2e/validation_suites/security/policy/01-openshell-version-supports-credential-rewrite.shtest/e2e/validation_suites/security/shields/00-config-consistent.shtest/e2e/validation_suites/suites.yaml
wscurran
added
E2E
Contributor
|
✨ Related open issues: |
cv
approved these changes
May 20, 2026
This was referenced May 20, 2026
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
Migrates the security policy and credential E2E checks into focused validation suites. Adds shared security assertion helpers, suite wiring, parity-map coverage, and framework tests for the new security credential, policy, shields, and injection suites.
Related Issue
Fixes #3815
Changes
test/e2e/validation_suites/lib/security_policy_credentials.shfor shared credential redaction, context, and policy assertion primitives.test/e2e/validation_suites/security/{credentials,policy,shields,injection}/.security-credentials,security-policy,security-shields, andsecurity-injectionintest/e2e/validation_suites/suites.yaml.test/e2e/docs/parity-map.yamlto map migrated legacy security assertions to the new suites.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Verification run:
E2E_CONTEXT_DIR=<tmp> E2E_DRY_RUN=1 HOME=<tmp> test/e2e/runtime/run-suites.sh security-credentials security-policy security-shields security-injectionpasses with seeded context.npm test -- test/e2e/scenario-framework-tests/e2e-lib-helpers.test.tspasses, 26 tests.npm test -- test/e2e/scenario-framework-tests/e2e-suite-runner.test.ts test/e2e/scenario-framework-tests/e2e-parity-map.test.ts test/e2e/scenario-framework-tests/e2e-coverage-report.test.ts test/e2e/scenario-framework-tests/e2e-convention-lint.test.ts test/e2e/scenario-framework-tests/e2e-metadata-final-hygiene.test.tspasses, 32 tests.npx prek run --all-fileswas attempted after hook-compliance fix but failed on unrelated repo/environment issues: missingnemoclaw/node_modules/json5/ plugin build artifact and multiple unrelated CLI timeout failures.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit