Repository navigation
feat(scanner)!: modernize core detection - #210
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 407 files, which is 107 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (407)
You can disable this status message by setting the 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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
vineethsai7
changed the title
vineethsai7
marked this pull request as ready for review
September 3, 2026 09:26
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
Author
|
@coderabbitai review |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b4fae1122
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a6192fee7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c59737681
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf56124c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41b835c66d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52afd4fd76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5455144e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5cc6a6433
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52796e1ac7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vineethsai7
changed the title
Habler-code
approved these changes
Sep 3, 2026
chinskiy
added a commit
to chinskiy/skill-scanner
that referenced
this pull request
Sep 25, 2026
… path from cisco-ai-defense#210 main added an eager, OrcaRouter-only ProviderConfig in __init__ (cisco-ai-defense#210) while this branch resolved credentials lazily for every provider. Rebased naively, both paths coexisted: the trailing request.update() from cisco-ai-defense#210 ran after this branch's spread-first request, so a future max_tokens/timeout key from get_request_params() would have silently overridden the adjudicator's own. Dropping the cisco-ai-defense#210 block alone broke OrcaRouter instead -- the lazy path re-derived ProviderConfig from an already-normalised model and lost the orcarouter/ detection, and it read the environment at call time rather than at construction, which test_adjudicator_uses_orcarouter_adapter_and_endpoint requires. Now one ProviderConfig is built eagerly in __init__ for every provider, with `provider` passed through and base_url/api_version from the two-tier lookup; _provider_params() adopts the normalised model for OrcaRouter as well as for OpenAI-compatible routes. model_id in the adjudication audit is now the configured model (orcarouter/...) rather than the normalised openai/...; the request LiteLLM receives is unchanged. Also documents SKILL_SCANNER_ADJUDICATOR_LLM_BASE_URL / _API_VERSION and the warn-once behaviour in docs/architecture/analyzers/adjudicator.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
22 of 32 tasks
vineethsai7
added a commit
that referenced
this pull request
Oct 2, 2026
…providers (#233) * fix(adjudicator): send provider credentials with the LiteLLM request The adjudicator builds its LiteLLM request by hand and omits `api_key` and `api_base`, so `--adjudicate` cannot authenticate in any deployment configured through `SKILL_SCANNER_LLM_API_KEY` / `SKILL_SCANNER_LLM_BASE_URL`. Every adjudication call fails and the pass silently does nothing. Reproduced with only the documented env vars set: SKILL_SCANNER_LLM_MODEL=anthropic/claude-sonnet-4-5 SKILL_SCANNER_LLM_API_KEY=<key> # ANTHROPIC_API_KEY unset litellm.completion kwargs: [drop_params, max_tokens, messages, model, temperature, timeout] litellm.AuthenticationError: Missing Anthropic API Key - A call is being made to anthropic but no key is set either in the environment variables or via params. Please set `ANTHROPIC_API_KEY` or `ANTHROPIC_AUTH_TOKEN` Observed in CI on 2.0.13 as 21 candidate findings and 21 failed adjudication calls, in a run where the LLM analyzer authenticated and produced findings normally against the same environment — which isolates the cause to this call path rather than the configuration. `adjudicator.py` is the only production LiteLLM call site that does not resolve its request through `ProviderConfig`. `LLMAnalyzer` and `MetaAnalyzer` both go through `LLMRequestHandler`, which merges `ProviderConfig.get_request_params()` (llm_provider_config.py:286) into the outbound request; that is where `SKILL_SCANNER_LLM_API_KEY` becomes `api_key` and `SKILL_SCANNER_LLM_BASE_URL` becomes `api_base`. The adjudicator re-implements model and temperature resolution but not credential resolution, so nothing supplies them and LiteLLM falls back to provider-native discovery. Delegating to the same resolver also picks up the provider mechanisms it already handles — Azure Entra ID tokens, Bedrock bearer/IAM, Vertex ADC, and Gemini's `GEMINI_API_KEY` handoff — instead of re-deriving any of them here. Notes on scope: - `self.model` is passed through unchanged rather than adopting `ProviderConfig.model`. For Gemini with `google-genai` installed, that attribute is normalised for the Google SDK, which this LiteLLM-only path cannot use. Keeping the resolved adjudicator model preserves the existing `SKILL_SCANNER_ADJUDICATOR_LLM_MODEL` override precedence. - A resolver failure degrades to an empty dict rather than raising. It must not be louder than the credential it is trying to supply, and the request then goes out exactly as it does today, with the existing error path keeping the finding at its original severity. The demote-only invariant is untouched. - Key collisions are not possible: `get_request_params` only ever returns `api_key` / `azure_ad_token` / `api_base` / `api_version` / `user` / `aws_*`. - `is_available()` deliberately still checks only the model. Requiring a key would break Ollama, Vertex ADC and Bedrock IAM, which authenticate ambiently. Four regression tests cover the credential fields. The existing 19 tests all patch `litellm.completion` and assert on the request the adjudicator *builds*, never on whether it can authenticate, which is why a request that fails against every real provider passed the suite. The first new test fails with `KeyError: 'api_key'` without this change. * fix(adjudicator): resolve base_url/api_version, and keep gateway traffic on the gateway Follow-up to the credential fix in the previous commit, which supplied `api_key` but not routing. `ProviderConfig` takes `base_url` and `api_version` as constructor arguments and never reads the environment for them — `analyzer_factory.py:152` and `meta_analyzer.py:340-349` do that for their own call sites — so `ProviderConfig(model=...)` alone left both unset. That left three of the documented provider families still broken, and made one of them worse than before. With the documented gateway setup: SKILL_SCANNER_LLM_MODEL=gpt-4o SKILL_SCANNER_LLM_BASE_URL=https://internal-gw.corp/v1 SKILL_SCANNER_LLM_API_KEY=<gateway key> the adjudicator sent `{api_key: <gateway key>}` with no `api_base`, and LiteLLM routes a bare `gpt-4o` to provider `openai` — so the gateway key and up to 600 lines of the scanned file would have gone to `api.openai.com`, an endpoint the operator deliberately routed away from. Before the previous commit the same call failed locally with no socket opened, so this was a new egress path, not a pre-existing one. Supplying half the routing information is worse than supplying none; both parts belong in the same change. Azure was simply non-functional: no `api_base` and no `api_version` produced `APIError: Must provide one of base_url or azure_endpoint`. Remote Ollama has the same dependency on `base_url`. Changes: - Read `base_url` and `api_version` and pass them in, two-tier (`SKILL_SCANNER_ADJUDICATOR_LLM_*` then scanner-wide) mirroring `meta_analyzer`. The adjudicator-specific tier exists because `SKILL_SCANNER_ADJUDICATOR_LLM_MODEL` already lets this pass run a different model, which may sit behind a different endpoint. - Adopt `ProviderConfig.model` for OpenAI-compatible providers only. LiteLLM cannot route a bare custom model name — `BadRequestError: LLM Provider NOT provided` — and `_normalize_openai_compatible_model_name` adds the required `openai/` prefix. Deliberately not generalised: the Gemini normalisation targets the Google SDK, which this LiteLLM-only path cannot use, and `ProviderConfig`'s separate bare-Gemini rewrite (`gemini-2.5-flash` → `gemini/2.5-flash`) drops the stem, which is not a behaviour to inherit here. - Drop `aws_region_name` unless `AWS_REGION` is explicitly set. It defaults to `us-east-1` and is therefore always present, and an explicit region short-circuits LiteLLM's entire resolution chain — model-ARN inference, `AWS_REGION_NAME`, `AWS_DEFAULT_REGION`, and the boto3 session that reads `~/.aws/config`. Forwarding it would silently relocate requests for a Bedrock operator who sets their region by profile or inference-profile ARN, which is a regression against the no-parameters behaviour that preceded this work. - Spread the provider params FIRST and set `model`/`messages`/`max_tokens`/ `timeout` after, so this pass's deliberate 200-token cap and timeout win by construction rather than by a promise about what `get_request_params` returns today. `model` is taken explicitly since it is the one value the resolver may legitimately override. - Report the first failure to obtain a verdict at WARNING, once per scan. Individually every adjudication failure is harmless — the finding keeps its severity and the gate only gets stricter — which is precisely why a wholly broken pass is invisible: DEBUG-only logs while `adjudicator` still appears in `analyzers_used`, so a misconfiguration reads as "found no false positives" rather than "never ran". That is how the bug in the previous commit survived a full CI run. Later failures stay at DEBUG so a flaky provider cannot flood the log. The docstring claims from the previous commit are corrected: it asserted `SKILL_SCANNER_LLM_BASE_URL` support that did not exist, and credited a Gemini `GEMINI_API_KEY` handoff that only works for the fully-prefixed model form. The `get_request_params` environment side effect and the `or ""` type-checker fallback are now noted where they live. Six of the nine tests in `TestAdjudicatorProviderCredentials` fail against unmodified upstream, verified in a clean worktree at the base commit. The previous `api_base` test was replaced: it patched `ProviderConfig.get_request_params` — the method under test — so it asserted its own mock, never set `SKILL_SCANNER_LLM_BASE_URL`, and passed whether or not the resolution it claimed to cover existed. The matrix now covers OpenAI-compatible gateway, Azure `api_version`, the adjudicator-specific override tier, Bedrock region in both directions, and the warn-once behaviour, all against a real `ProviderConfig`. * fix(adjudicator): reconcile credential resolution with the OrcaRouter path from #210 main added an eager, OrcaRouter-only ProviderConfig in __init__ (#210) while this branch resolved credentials lazily for every provider. Rebased naively, both paths coexisted: the trailing request.update() from #210 ran after this branch's spread-first request, so a future max_tokens/timeout key from get_request_params() would have silently overridden the adjudicator's own. Dropping the #210 block alone broke OrcaRouter instead -- the lazy path re-derived ProviderConfig from an already-normalised model and lost the orcarouter/ detection, and it read the environment at call time rather than at construction, which test_adjudicator_uses_orcarouter_adapter_and_endpoint requires. Now one ProviderConfig is built eagerly in __init__ for every provider, with `provider` passed through and base_url/api_version from the two-tier lookup; _provider_params() adopts the normalised model for OrcaRouter as well as for OpenAI-compatible routes. model_id in the adjudication audit is now the configured model (orcarouter/...) rather than the normalised openai/...; the request LiteLLM receives is unchanged. Also documents SKILL_SCANNER_ADJUDICATOR_LLM_BASE_URL / _API_VERSION and the warn-once behaviour in docs/architecture/analyzers/adjudicator.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(adjudicator): drop the adjudicator-specific base_url/api_version tier Keep this change to the bug: the adjudicator now resolves credentials and routing from the existing SKILL_SCANNER_LLM_* variables only. An adjudicator-specific SKILL_SCANNER_ADJUDICATOR_LLM_BASE_URL / _API_VERSION tier was half a feature -- there is no matching _API_KEY tier, so pointing the adjudicator at a different gateway would have forced the same key. If a separate adjudicator endpoint is wanted it should arrive as a complete tier, ideally via a ProviderConfig.from_env() that owns the resolution for every call site. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(adjudicator): send nothing when provider resolution fails Addresses the CodeRabbit finding on #233. Swallowing a ProviderConfig failure and sending the request anyway shipped the scanned file to whatever LiteLLM resolved from ambient environment variables: the provider's public endpoint instead of the configured gateway, or a remote Ollama host that validate_ollama_base_url had just rejected. For OrcaRouter it also undid the fail-closed behaviour on main, where the constructor error propagated to the scanner and no request was made. _provider_params() now returns None when resolution is unavailable and _call_llm() stops before litellm.completion(), reporting the cause through the once-per-scan WARNING. An empty dict stays a valid result, since Vertex ADC and Bedrock IAM authenticate ambiently and may legitimately need no parameters. Findings keep their original severity either way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Vineeth Sai Narajala <vnarajal@cisco.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
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
This PR modernizes Skill Scanner's deterministic core and adds a bounded CEL decision layer backed by the official
cel-gov0.32.0 runtime.CEL correlates typed analyzer facts after deterministic detection and before optional LLM/Meta analysis. It gates existing candidates only: it cannot create findings or change rule IDs, categories, or severities. All eight bundled CEL rules remain individually
shadow, so this PR does not enable suppression.Headline metrics are core-only. ATR, LLM, Meta, VirusTotal, and administrator-local packs are excluded.
Changelog
Added
skill_scanner.semantic.v1.ScanFacts.cel-gov0.32.0 helper, immutable compiled generations, runtime attestation, expression hashes, fail-open telemetry, and bounded fact projection.off,shadow, andenforce.validate-rules, and repeatable administrator-trusted local packs without arbitrary Python loading.Changed
Security
eval/exec/os/subprocessaliases and JavaScripteval/child_processaliases, including bounded function, arrow, block, catch, loop, reassignment, semicolonless CommonJS, and template-substitution handling, close review-identified masking and leakage paths without treating inert text or shadowed calls as executable code.Breaking changes
>=3.11,<3.15; Python 3.10 consumers must pin the previous release.--custom-rules.Architecture and rollout
true: retain.falsein shadow: retain and recordwould_suppress.falsein enforce: suppress only if that rule is also individually promoted.All bundled CEL rules remain
shadow. The measured quality gain in this PR comes from deterministic core-analyzer improvements, not CEL suppression.Benchmark results
Dataset:
ProtectSkills/MaliciousSkillBenchat revisiond4b42ce5766a6e0359c987cf59c1007cb3795a90. Configuration: deterministic core only, CEL off/shadow, no ATR/LLM/Meta.Development train/validation population
origin/mainThis split is development evidence and was used during tuning.
Locked source-disjoint population
origin/mainThis result fails the no-FPR-regression promotion criterion. It does not justify CEL enforcement or a release-pass claim. The split has been observed and is frozen against further tuning.
Compatibility and supplemental evidence
Official-skill off/shadow finding identities were exact. CEL evaluated 36 candidates, recorded 28 would-suppress decisions, and had zero fallbacks/errors; CEL used 0.239% of scan time. The remaining HIGH+ official findings are contextual hard negatives, not proof that the packages are malicious.
Validation
go test ./...andgo vet ./...passed for the v0.32.0 helper.pip-audit: no known vulnerabilities.validate-rulespassed on CPython 3.11, 3.12, 3.13, and 3.14; sdist install passed; Python 3.10 rejection verified.Known limitations
origin/main; more extractor/context work belongs in follow-up PRs.Review notes