fix: run unit tests in CI, repair unrunnable scripts, and correct false-pass connector evaluations - #216
Open
ethanolivertroy wants to merge 9 commits into
Open
Conversation
The contract-test and manifest workflows ran the schema validator and the shell validators, but nothing ever invoked the node:test suites. That left tests/wiz-inspector-collect.test.js green-by-default since it was added, and any test file a contributor adds under tests/ would be silently skipped. Discovery is by filename rather than a directory or glob argument because Node 20, the CI baseline, expands neither. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
…nalyzer runnable All three were written as CommonJS while the root package.json declares "type": "module", so every entry point threw ReferenceError: require is not defined before running a single line. npm run scan-iac and npm run test-control were both dead, as were the documented /grc-engineer:scan-iac and :test-control workflows. Converting scan-iac.js surfaced a second failure behind the first: the rules database binds this.checkK8sEncryption, which was never implemented, so the constructor threw on every invocation. Implemented it against the same signature as the sibling validators. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
…erdicts Each of these produced a confident wrong answer rather than an error, which is the worst failure mode for a compliance connector: - okta: filter(Boolean) dropped maxSession*Minutes values of 0, which is how Okta encodes 'unlimited'. The unlimited case was the one the code below it meant to fail, and it passed instead. Math.max also ranked 0 as the strictest setting. Policies declaring no session setting now report inconclusive rather than pass on no evidence. - splunk: retention fell back to maxGlobalDataSizeMB, a size cap, and divided it by 86400 as though it were seconds. Retention is now unknown when frozenTimePeriodInSecs is absent. - datadog: monitor priority is an integer 1-5, so comparing it to the string 'p1' never matched and the check silently degraded to a name regex. - tenable: vulnerabilities with an unparseable last-seen date aged as 0 and counted as fresh; they are now reported as undated. - tenable, crowdstrike: the flat YAML matcher only accepted unindented keys, so the nested 'defaults:' block written by setup.sh parsed as "" and every configured default was ignored. - snowflake: a config without 'account' emitted findings whose resource.id was undefined, which JSON.stringify drops, producing documents that violate the finding schema. Now fails at startup. - wiz: connection pagination had no page cap or cursor-repeat check. - gcp: the 'oldest key' named in the message was whichever key was found first. Connectors that called main() at import time now guard on CLI invocation so their helpers can be unit tested. The guard tolerates an undefined process.argv[1] instead of throwing inside pathToFileURL. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
CLAUDE.md and docs/ARCHITECTURE.md described a repo of 30 plugins, 21 frameworks, and 4 connectors. The marketplace registers 65, 35, and 15. The hand-maintained lists are what drifted, so they are replaced with pointers to marketplace.json and the generated FRAMEWORK-COVERAGE.md rather than refreshed in place. Also corrects claims contradicted by the tree: cis-controls is not MIT, Node scripts are not confined to persona plugins, fedramp-20x is not prompt-only, and grc-auditor/grc-internal/grc-tprm have neither scripts/ nor config/ despite being documented as script-backed personas. Drops references to a fedramp-docs plugin (the OSCAL plugin is fedramp-ssp) and to config/crosswalk-overrides.yaml; neither exists. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe PR fixes connector evaluation logic, makes collector scripts safe to import, converts GRC scripts to ES modules, adds Kubernetes encryption checks, introduces all-suite unit testing, and updates repository documentation. ChangesConnector and validation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github.com/VantaInc/vanta-mcp-plugin now 404s, which fails the link-check workflow on every pull request. The official Vanta plugin ships in anthropics/claude-plugins-official, as the vanta-bridge removal note in CHANGELOG.md already says. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
Greptile SummaryThis PR enables automatic Node unit-test discovery in CI, repairs ESM-incompatible GRC scripts, and tightens connector evaluation behavior.
Confidence Score: 4/5The PR is not yet safe to merge because the Kubernetes encryption check can still suppress a finding based on unrelated metadata. The Wiz truncation and Kubernetes dispatch issues are fixed, but Files Needing Attention: plugins/grc-engineer/scripts/scan-iac.js; tests/scan-iac-kubernetes.test.mjs
|
| Filename | Overview |
|---|---|
| plugins/connectors/wiz-inspector/scripts/collect.js | Truncated pagination now produces inconclusive findings, records partial errors, avoids conflicting empty-result passes, and exits with the partial status. |
| plugins/grc-engineer/scripts/scan-iac.js | Kubernetes dispatch is restored, but encryption evidence is still matched across the entire serialized resource, leaving the previously reported false-negative path reachable. |
| tests/scan-iac-kubernetes.test.mjs | Adds dispatch and encryption regression tests, but does not cover affirmative-looking encryption fields in unrelated metadata. |
| tests/wiz-inspector-collect.test.js | Verifies repeating-cursor truncation produces partial, inconclusive output without a coexisting blanket pass. |
| tests/run-unit-tests.sh | Adds automatic execution of top-level Node test suites for CI. |
| .github/workflows/contract-test.yml | Runs the newly added unit-test command as part of the contract-test workflow. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
CI["Contract-test workflow"] --> UNIT["Auto-discovered Node unit suites"]
UNIT --> CONN["Connector evaluation regressions"]
UNIT --> WIZ["Wiz pagination regressions"]
UNIT --> K8S["Kubernetes scanner regressions"]
CONN --> FIND["Schema-conforming findings"]
WIZ --> FIND
K8S --> REPORT["IaC compliance report"]
Reviews (5): Last reviewed commit: "fix(scan-iac): only affirmative declarat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Open
13 tasks
…d document test:unit Both notes are superseded by this PR: the ESM conversion makes the scripts runnable, and tests/run-unit-tests.sh (npm run test:unit) now discovers every tests/*.test.* suite. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
ethanolivertroy
marked this pull request as ready for review
August 7, 2026 12:41
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 38ef6f3. Configure here.
13 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@CLAUDE.md`:
- Around line 42-54: Update the script-backed plugin classification in the
two-pattern section to include fedramp-20x, dashboards/compliance-posture, and
knowledge-sources/gcp-docs, matching the script-bearing locations listed near
the end of the document. Alternatively, explicitly label both inventories as
examples, but ensure the classification and the later inventory remain
consistent.
In `@docs/ARCHITECTURE.md`:
- Around line 137-140: Update the plugin taxonomy section in the architecture
documentation to include the reporting/workflow, bridge, and knowledge-source
categories defined by CLAUDE.md and docs/ARCHITECTURE-V2-RFC.md. Keep the
existing categories intact and ensure the list represents the current plugin
inventory rather than a partial subset.
- Line 177: Update the crosswalk guidance near the “Don't: use SCF” text to
remove the prohibition against using SCF and align it with the document’s
canonical guidance: contributors should use SCF and submit a PR upstream when
the required control mapping is missing.
In `@plugins/connectors/datadog-inspector/scripts/collect.js`:
- Around line 248-250: Update isCriticalMonitor so critical, prod, and
production match only as complete word tokens rather than substrings, while
preserving the priority-based match. Add a regression test covering a
non-production monitor named “product” and ensure it is not classified as
critical.
In `@plugins/connectors/tenable-inspector/scripts/collect.js`:
- Around line 100-103: Update the scalar parsing logic before assigning
parent[key] so it strips inline comments only when they occur outside quoted
values, then recognizes and removes matching single or double quotes before
boolean and numeric conversion. Ensure values such as quoted URLs retain their
full content and numeric values followed by comments are converted to numbers.
In `@plugins/connectors/wiz-inspector/scripts/collect.js`:
- Around line 172-182: Update the callers of collectConnection for issues,
vulnerabilities, and configurationFindings to check res.truncated in addition to
!res.ok before creating findings. On truncation, call recordError, mark the
tenant metadata with truncated: true, and ensure the evaluation is inconclusive
so it exits via EXIT.PARTIAL.
In `@plugins/grc-engineer/scripts/scan-iac.js`:
- Around line 304-317: Update checkK8sEncryption so storageClassName alone does
not satisfy declaresEncryption; continue accepting explicit encryption or KMS
indicators, but report resources that only reference a StorageClass for manual
verification unless the referenced StorageClass is resolved and confirmed
encrypted. Preserve Kubernetes YAML manifest support and the existing issue
structure.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3b46d5b-be36-4e59-959e-2151e95f11ca
📒 Files selected for processing (21)
.github/workflows/contract-test.ymlAGENTS.mdCHANGELOG.mdCLAUDE.mdROADMAP.mddocs/ARCHITECTURE-V2-RFC.mddocs/ARCHITECTURE.mdpackage.jsonplugins/connectors/crowdstrike-inspector/scripts/collect.jsplugins/connectors/datadog-inspector/scripts/collect.jsplugins/connectors/gcp-inspector/scripts/collect.jsplugins/connectors/okta-inspector/scripts/collect.jsplugins/connectors/snowflake-inspector/scripts/collect.jsplugins/connectors/splunk-inspector/scripts/collect.jsplugins/connectors/tenable-inspector/scripts/collect.jsplugins/connectors/wiz-inspector/scripts/collect.jsplugins/grc-engineer/scripts/cross-framework-analyzer.jsplugins/grc-engineer/scripts/scan-iac.jsplugins/grc-engineer/scripts/test-control.jstests/connector-evaluation-logic.test.mjstests/run-unit-tests.sh
- scan-iac: implement the missing scanKubernetesResource dispatch and narrow scanYaml's try/catch to the parse, so Kubernetes documents are scanned instead of silently skipped (Greptile P1) - scan-iac: stop accepting a bare storageClassName reference as encryption evidence; unresolvable StorageClass references now get a manual-verification finding (CodeRabbit) - wiz-inspector: surface pagination truncation as an inconclusive tenant evaluation plus a partial error so runs exit PARTIAL, with a repeating-cursor fixture exercising it end to end (Bugbot, CodeRabbit) - datadog-inspector: add word boundaries to the critical-monitor name regex so 'product catalog' no longer matches 'prod' (CodeRabbit) - tenable/crowdstrike: flat YAML parser handles single-quoted scalars and inline comments (CodeRabbit) - grc-engineer scripts: guard CLI detection when process.argv[1] is undefined, matching the connectors (Bugbot) - docs: complete the script-backed plugin inventory in CLAUDE.md, add the missing plugin categories to ARCHITECTURE.md, and fix the ambiguous 'Don't: use SCF.' crosswalk wording (CodeRabbit) Unit suite grows 23 -> 32 tests; all pass. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
…sult passes A repeating cursor with zero collected rows previously produced both an inconclusive truncation finding and a confident 'no open findings' pass for the same control. The empty-result pass branch is now mutually exclusive with truncation. Fixture updated so configurationFindings exercises the truncated-and-empty case. Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
…evidence 'encryption: disabled', descriptions like 'kms encryption pending', or any value merely containing kms no longer suppress the finding. Evidence now requires encrypted: true, an affirmative encryption: value, or a KMS key reference (key name containing kms+key with a value). Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
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
2 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
A review of the repo surfaced one root cause and three families of defects sitting behind it.
Nothing ever ran the
node:testsuites. CI ran the schema validator and the shell validators only, sotests/wiz-inspector-collect.test.jshas been green-by-default since it landed, and any test file a contributor adds undertests/is silently skipped today. That is also why the defects below survived: none of them are subtle, they were simply never executed.With that fixed, the rest of this PR repairs what the gap was hiding.
Three
grc-engineerscripts could not run at all.scan-iac.js,test-control.js, andcross-framework-analyzer.jsare CommonJS while the rootpackage.jsondeclares"type": "module", so each threwReferenceError: require is not definedbefore executing a line.npm run scan-iacandnpm run test-controlwere both dead, as were the/grc-engineer:scan-iacand:test-controlworkflows documented inCLAUDE.md. Convertingscan-iac.jsexposed a second fault behind the first: the rules database bindsthis.checkK8sEncryption, which was never implemented, so the constructor threw on every invocation.Seven connectors returned confident wrong compliance verdicts. These are the worst failure mode for this repo, because an operator sees a green control rather than an error. Each one is now covered by a regression test in
tests/connector-evaluation-logic.test.mjs.okta-inspectorfilter(Boolean)dropped session values of00is how Okta encodes unlimited — the worst case passedokta-inspectorMath.maxover session minutessplunk-inspectormaxGlobalDataSizeMBfor retentiondatadog-inspectorpriorityto string'p1'tenable-inspector0tenable,crowdstrikedefaults:block their ownsetup.shwrites was ignoredsnowflake-inspectoraccountvalidationfinding.schema.jsonwiz-inspectorhasNextPageloops until OOMgcp-inspectorstaleKeys[0]Verified pre-fix behaviour, reproduced directly:
Docs described a different repo, and a link had rotted.
CLAUDE.mdanddocs/ARCHITECTURE.mdclaimed 30 plugins / 21 frameworks / 4 connectors; the marketplace registers 65 / 35 / 15. Separately,github.com/VantaInc/vanta-mcp-pluginbegan returning 404 after 2026-07-27 and was failing thelink-checkworkflow on every open PR, including this one before the fix.Type of change
Schema impact
snowflake-inspectornow fails closed instead of emitting documents that violate the existingresource.idrequirement, so this moves it into conformance rather than changing the contract.Test plan
Full suite locally:
The new step running in CI on Node 20, picking up the suite that had never executed:
The runner auto-discovers suites and fails the build correctly (verified with a deliberately failing canary, then removed):
Previously-dead scripts, now exercised end to end:
All 15 connectors still behave correctly as CLIs after the import guards (each reports missing config rather than crashing), and
snowflake-inspectornow exits 5 on a config with noaccount. Markdown lint is unchanged at the pre-existing 31 errors, none in files touched here. All GitHub checks pass on this branch.CHANGELOG
Notes for reviewers
The connector changes are worth the closest look. Connectors that called
main()at import time now guard on CLI invocation so their helpers can be unit tested; the guard tolerates an undefinedprocess.argv[1]rather than throwing insidepathToFileURL, which is a latent crash the existing pattern has whenever the module is imported outside a normal CLI entry point.Two judgement calls that change verdicts rather than just fixing mechanics, in case you disagree:
inconclusiveinstead ofpass. It was passing on no evidence.frozenTimePeriodInSecsnow reportsinconclusiveinstead of a fabricated day count.Both follow the repo's existing convention that absence of evidence is not evidence of compliance, but they will move numbers on existing dashboards.
Deliberately left alone as out of scope: the pre-existing
MD022markdown lint warnings, andtestssl-inspector's option-injection surface where a--targetbeginning with--is passed through totestssl.shas another flag.Summary by CodeRabbit
Bug Fixes
Documentation
Tests