feat(audit): allow SkillSpector --baseline through external args - #2936
Open
Mohammed Alkindi (MohammedAlkindi) wants to merge 2 commits into
Open
feat(audit): allow SkillSpector --baseline through external args#2936Mohammed Alkindi (MohammedAlkindi) wants to merge 2 commits into
Mohammed Alkindi (MohammedAlkindi) wants to merge 2 commits into
Conversation
The SkillSpector adapter's passthrough allowlist had no --baseline entry, so `apm audit --external skillspector --external-args "--baseline .skillspector-baseline.yaml"` was rejected fail-closed and reviewed suppression baselines could not be used through APM. Add --baseline to ALLOWED_ARG_PREFIXES. Path resolution and containment need no new code: validate_extra_args already resolves value tokens and rejects anything that leaves the scan directory. Add validate_value_arity, run after the allowlist gate, to require exactly one value for declared flags. extra_args are appended before the positional scan targets, so a bare --baseline would make SkillSpector read the first target as the baseline path and silently audit one path fewer; a stray second value would become an extra target. Both --flag=value and --flag value are handled. Closes microsoft#2331
Copilot started reviewing on behalf of
Mohammed Alkindi (MohammedAlkindi)
September 10, 2026 07:44
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved baseline path-containment and inline-argument validation issues remain, and packaged usage guidance is stale.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds SkillSpector --baseline support to apm audit --external-args, with allowlisting, arity validation, tests, and documentation.
Changes:
- Allowlists and forwards
--baseline. - Adds value-arity validation and related tests.
- Documents baseline usage and constraints.
File summaries
| File | Summary |
|---|---|
tests/unit/test_skillspector_llm.py |
Tests baseline forwarding and safety behavior. |
tests/unit/test_external_options.py |
Tests baseline value-arity validation. |
src/apm_cli/security/external/skillspector.py |
Adds SkillSpector baseline support. |
src/apm_cli/security/external/options.py |
Adds shared arity validation; inline values still need trailing-value enforcement. |
docs/src/content/docs/integrations/external-scanners.md |
Documents baseline usage; packaged usage guidance also needs updating. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "--language", | ||
| "--exclude", | ||
| "--include", | ||
| "--baseline", |
Comment on lines
+224
to
+227
| if separator: | ||
| if not inline_value: | ||
| raise ExternalScanError(missing) | ||
| continue |
Comment on lines
+122
to
+124
| `--baseline` points SkillSpector at a reviewed suppression baseline. It must | ||
| carry exactly one value, inside the working directory, so a bare `--baseline` | ||
| cannot swallow the first scan target: |
The arity gate was asymmetric. `--baseline value extra.yaml` was rejected, but `--baseline=value extra.yaml` was accepted: the inline branch returned to the loop without looking at the following token, so the stray value survived and became an extra positional scan target. Check the next token in both spellings. Containment did not cover the documented form. validate_extra_args treats a separator-free token as a non-path, which is right for `--model gpt-4o` but not for a path-valued flag: `--baseline .skillspector-baseline.yaml` never reached ensure_path_within, so a symlink of that name pointing outside the scan directory would have been read despite the containment claim. Resolve every declared path value through _reject_escaping_value, separators or not. This is the allowlist's first path-valued entry, which is why the gap appeared. validate_value_arity now takes base_dir, so each adapter states the scan root it validates against. Four regression tests cover the inline stray value and both escaping spellings. Also document the exactly-one-value and in-working-directory constraints in the packaged apm-usage guide, which describes the same --external-args contract as the Starlight page.
Contributor
Author
|
All three are addressed in 1adae94. Declared path values now resolve regardless of separators, so the documented Checked with a directory junction named |
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.
Closes #2331
Description
The SkillSpector adapter's argument allowlist had no
--baseline, soapm audit --external-args "--baseline ..."was rejected fail-closed and reviewed suppression baselines were unusable through APM.--baselineis now allowlisted. Path handling is unchanged:validate_extra_argsalready resolves value tokens and rejects anything leaving the scan directory, so the issue's resolve and reject-outside bullets needed no new code. The one new rule is arity.extra_argsare appended before the positional targets, so a bare--baselinemakes SkillSpector read the first scan target as the baseline path and silently audit one path fewer. A newvalidate_value_aritypass rejects zero values and a second value, for--flag=valueand--flag valuealike.Type of change
Testing
16 new tests; the four external-scanner test files go from 77 to 93 passed. Fail-before on the unfixed tree: 6 of 7 adapter tests raise "argument '--baseline' is not allowed", and the no-baseline case passes both ways. Full
tests/unit tests/test_console.pymatches the pre-change baseline.ruff checkandruff format --checkclean.Spec conformance (OpenAPM v0.1)
🤖 Generated with Claude Code