feat(security): add basic security reviewer agent with owasp skills#1008
feat(security): add basic security reviewer agent with owasp skills#1008JasonTheDeveloper wants to merge 47 commits intomicrosoft:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1008 +/- ##
=======================================
Coverage 88.04% 88.04%
=======================================
Files 45 45
Lines 7885 7885
=======================================
Hits 6942 6942
Misses 943 943
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ndition (#1109) Add a fork-detection `if:` condition to the "Submit uv.lock dependencies" step in the dependency-review workflow. The Dependency Submission API requires `contents: write`, but GitHub automatically downgrades fork PR tokens to read-only, causing the step to fail with `HttpError: Resource not accessible by integration`. The existing code comment indicated the step should be skipped on fork PRs, but no condition enforced this. The fix adds `if: github.event.pull_request.head.repo.full_name == github.repository` so the step runs only for same-repo PRs. The Dependency Review step (which only needs read access) continues to run for all PRs including forks. ## Related Issue(s) Closes #1108 ## Type of Change Select all that apply: **Code & Documentation:** * [x] Bug fix (non-breaking change fixing an issue) * [ ] New feature (non-breaking change adding functionality) * [ ] Breaking change (fix or feature causing existing functionality to change) * [ ] Documentation update **Infrastructure & Configuration:** * [x] GitHub Actions workflow * [ ] Linting configuration (markdown, PowerShell, etc.) * [ ] Security configuration * [ ] DevContainer configuration * [ ] Dependency update **AI Artifacts:** * [ ] Reviewed contribution with `prompt-builder` agent and addressed all feedback * [ ] Copilot instructions (`.github/instructions/*.instructions.md`) * [ ] Copilot prompt (`.github/prompts/*.prompt.md`) * [ ] Copilot agent (`.github/agents/*.agent.md`) * [ ] Copilot skill (`.github/skills/*/SKILL.md`) > Note for AI Artifact Contributors: > > * Agents: Research, indexing/referencing other project (using standard VS Code GitHub Copilot/MCP tools), planning, and general implementation agents likely already exist. Review `.github/agents/` before creating new ones. > * Skills: Must include both bash and PowerShell scripts. See [Skills](../docs/contributing/skills.md). > * Model Versions: Only contributions targeting the **latest Anthropic and OpenAI models** will be accepted. Older model versions (e.g., GPT-3.5, Claude 3) will be rejected. > * See [Agents Not Accepted](../docs/contributing/custom-agents.md#agents-not-accepted) and [Model Version Requirements](../docs/contributing/ai-artifacts-common.md#model-version-requirements). **Other:** * [ ] Script/automation (`.ps1`, `.sh`, `.py`) * [ ] Other (please describe): ## Testing * YAML lint passes: `npm run lint:yaml` * Verified the condition syntax matches GitHub Actions expression documentation * Confirmed all 29 other checks pass on the affected fork PR (#1008); only "Review Dependencies" fails due to this missing condition ## Checklist ### Required Checks * [ ] Documentation is updated (if applicable) * [x] Files follow existing naming conventions * [x] Changes are backwards compatible (if applicable) * [ ] Tests added for new functionality (if applicable) ### AI Artifact Contributions * [ ] Used `/prompt-analyze` to review contribution * [ ] Addressed all feedback from `prompt-builder` review * [ ] Verified contribution follows common standards and type-specific requirements ### Required Automated Checks The following validation commands must pass before merging: * [x] Markdown linting: `npm run lint:md` * [x] Spell checking: `npm run spell-check` * [x] Frontmatter validation: `npm run lint:frontmatter` * [x] Skill structure validation: `npm run validate:skills` * [x] Link validation: `npm run lint:md-links` * [x] PowerShell analysis: `npm run lint:ps` * [x] Plugin freshness: `npm run plugin:generate` ## Security Considerations * [x] This PR does not contain any sensitive or NDA information * [ ] Any new dependencies have been reviewed for security issues * [x] Security-related scripts follow the principle of least privilege ## Additional Notes Single-line fix. The `if:` condition uses the standard GitHub Actions fork detection pattern comparing `github.event.pull_request.head.repo.full_name` against `github.repository`. When they differ, the PR originates from a fork and the Dependency Submission API call is skipped. The subsequent Dependency Review step runs unconditionally for all PRs.
katriendg
left a comment
There was a problem hiding this comment.
Great work on getting this PR in, especially with the rework you did on aligning with some of the specifics within HVE-Core.
I left a few comments to address, I believe after some additional review we are close to being able to approve this PR.
Merging it will allow us to test in some real end user scenarios, especially also when calling in the agents/skills from the extension in VS code and within Copilot CLI as plugins.
There will be some follow-up PRs, but it may be beneficial have to PR #979 merged in first.
Some high level ideas I believe for follow-up work:
- Align orchestrator step voice with established agents - how vs what, delegation voice in orchestrator vs imperative voice in subagents
- Move the remaining 3 format contracts out of subagent bodies and into the
security-reviewer-formatsskill - We need to add follow-up issue to create easy to use prompt files for users to discover the workflow via slash commands, like some of the other agents offer.
Since everything is marked maturity: experimental, consumers know to expect further iteration with this version.
| 4. Resolve mode-specific inputs before proceeding to the assessment pipeline. | ||
|
|
||
| * When mode is `audit`: no additional setup is required. Proceed to Step 1. | ||
| * When mode is `diff`: |
There was a problem hiding this comment.
Can you evaluate rewording this section to use the updated pr-reference skill, the tool list-changed-files should cover all of these commands by natural language. This is based on @obrocki's earlier PR that has been merged and should simplify this section.
There was a problem hiding this comment.
Resolved in refactor(agent): switch to using pr-reference skill to retrieve diff. One question though. Since the agent now needs the pr-reference skill, would I need to update the security.collection.yml to include that skill?
There was a problem hiding this comment.
Thanks - yes absolutely we need to include this dependency in the collection level because it will not automatically be deduced and added.
There was a problem hiding this comment.
|
|
||
| ## Constants | ||
|
|
||
| Skill resolution: Locate a skill's entry file by searching for `{skill-name}/SKILL.md`. |
There was a problem hiding this comment.
This is a general note for all skill searches, do we need to use this approach or can we switch the natural language - VS Code's progressive disclosure?
It will require manual testing to validate Copilot correctly discovers the required files, but this should align more with the expectations of how skills are enabled. Note it may require updating some of the skills' description fields to be correctly discoverable.
Inline skill resolution pattern diverges from progressive disclosure convention
- File: All 5 agent files
- Category: Convention Compliance (prompt-builder)
- Severity: MEDIUM
- Status: Not addressed (the pattern was added in commit
186f26b2as part of "remove hardcoded file path for portability")
Every agent file contains a Skill resolution: constant with procedural instructions:
"Locate a skill's entry file by searching for
{skill-name}/SKILL.md. Follow the entry file's normative reference links to access vulnerability reference documents..."
This appears ~12 times across the 5 files.
The prompt-builder.instructions.md "Skill Invocation from Callers" section states:
"When prompts, agents, or instructions need a skill's capability, describe the task intent rather than referencing script paths directly. Copilot matches the task description against each skill's
descriptionfrontmatter and loads the skill on-demand via progressive disclosure.""Avoid hardcoded script paths, platform detection logic, or extension fallback code in caller files."
No other agent in the codebase uses inline skill resolution. The PR Review agent says "use the pr-reference skill"; the PowerPoint subagent says "use the powerpoint skill". All rely on Copilot's name-matching discovery mechanism.
Suggestion: Replace all Skill resolution: constants and procedural path-searching instructions with by-name references. For example:
- "Read the
owasp-top-10skill to access vulnerability references" - "Read the
security-reviewer-formatsskill for format templates. Follow its normative reference links to load report, finding, completion, and severity format files."
This aligns with progressive disclosure, reduces per-agent token overhead, and makes agents portable across contexts (repo, plugin, extension).
Also applies to the Format Specifications section in the orchestrator:
# Before
Format templates used by subagents are defined in the `security-reviewer-formats`
skill. Locate the skill entry file by searching for `security-reviewer-formats/SKILL.md`,
then follow its normative reference links to load the required format files.
# After
Read the `security-reviewer-formats` skill for format templates used by subagents.
Follow its normative reference links to load report, finding, completion, and severity
format files.
There was a problem hiding this comment.
This is a general note for all skill searches, do we need to use this approach or can we switch the natural language - VS Code's progressive disclosure?
I tend to err on the side of caution and try to be super explicite as to which file I want the agent to look at to prevent the agent from going off on a tangent. Originally I had the full .github/skills/... folder path referenced but that approach is not super portable especially with users using non GitHub Copilot folder structures. You are probably right though referencing the skill by name is probably enough.
I'll take your lead and go with your approach. Changes applied in refactor(agents): refer to skill by name rather than {skill}/SKILL.md.
|
Forgot one thing, I believe I did not see an update to the file |
I did that more because I wasn't sure on the approach we want to take with this. Since this is phase 1 of 4 phases I figured we'd be building on top of this and making adjustments as we go. The skills themselves ( That's my thinking though. Happy to take your lead if you see things differently 😊
Done in feat(agent): update CUSTOM-AGENTS.md with security review agent. |
Pull Request
Description
This PR introduces a new Security Reviewer agent containing skills relating to the follow OWASP related content:
The agent contains 3 modes to fit a rang of different usecases and they are:
auditdiffplanThere are also two additional inputs you can pass:
targetSkill: Run a particular skill - skips codebase profiling stepscope: Restricts agent to a particular directory/fileDetail agent flow:
flowchart TD Start([User Invokes Security Reviewer]) --> SetDate["Pre-req: Set report date"] SetDate --> DetectMode{"Detect scanning mode"} DetectMode -->|"explicit or keywords:<br/>changes, branch, PR"| DiffMode["Mode = diff"] DetectMode -->|"explicit or keywords:<br/>plan, design, RFC"| PlanMode["Mode = plan"] DetectMode -->|"default / explicit"| AuditMode["Mode = audit"] DetectMode -->|"invalid mode"| InvalidStop([Stop: Invalid mode]) %% Step 0: Mode-specific setup AuditMode --> StatusSetup["Status: Starting in audit mode"] DiffMode --> GitDetect["Detect default branch<br/>git symbolic-ref"] PlanMode --> ResolvePlan["Resolve plan document<br/>from input / context / fallback"] GitDetect -->|"fail"| FallbackAudit["Fallback to audit mode"] FallbackAudit --> StatusSetup GitDetect -->|"ok"| MergeBase["Compute merge base<br/>git merge-base"] MergeBase -->|"fail"| FallbackAudit MergeBase -->|"ok"| ChangedFiles["Get changed files<br/>git diff --name-only"] ChangedFiles -->|"fail"| FallbackAudit ChangedFiles -->|"no files"| EmptyStop([Stop: No changed files]) ChangedFiles -->|"files found"| FilterFiles["Filter non-assessable<br/>.md .yml .json images etc."] FilterFiles -->|"empty after filter"| FilterStop([Stop: No assessable code files]) FilterFiles -->|"assessable files"| StatusSetup ResolvePlan -->|"no plan found"| AskUser["Ask user for plan path"] AskUser --> ResolvePlan ResolvePlan -->|"plan resolved"| ReadPlan["Read plan document"] --> StatusSetup %% Step 1: Profile Codebase StatusSetup --> TargetSkill{"targetSkill<br/>provided?"} TargetSkill -->|"yes"| ValidateSkill{"Skill in<br/>Available Skills?"} ValidateSkill -->|"no"| SkillStop([Stop: Show available skills]) ValidateSkill -->|"yes"| StubProfile["Build minimal profile stub<br/>skip Codebase Profiler"] StubProfile --> SetSkills1["Applicable skills = targetSkill only"] TargetSkill -->|"no"| RunProfiler[/"Subagent: Codebase Profiler<br/>mode-specific prompt"/] RunProfiler -->|"fail"| ProfileFail([Stop: Profiling failed]) RunProfiler -->|"ok"| IntersectSkills["Intersect profiler skills<br/>with Available Skills"] IntersectSkills --> SpecificOverride{"Specific skills<br/>list provided?"} SpecificOverride -->|"yes"| OverrideSkills["Override with provided list<br/>intersect with Available Skills"] SpecificOverride -->|"no"| CheckEmpty{"Any applicable<br/>skills?"} OverrideSkills --> CheckEmpty CheckEmpty -->|"none"| NoSkillStop([Stop: No applicable skills]) CheckEmpty -->|"skills found"| SetSkills2["Set applicable skills list"] SetSkills1 --> StatusProfile["Status: Profiling complete"] SetSkills2 --> StatusProfile %% Step 2: Assess Skills StatusProfile --> AssessLoop["Status: Beginning skill assessments"] AssessLoop --> ForEachSkill["For each applicable skill<br/>(parallel when supported)"] ForEachSkill --> RunAssessor[/"Subagent: Skill Assessor<br/>mode-specific prompt per skill"/] RunAssessor -->|"incomplete"| RetryAssessor[/"Retry Skill Assessor<br/>(once)"/] RetryAssessor -->|"still fails"| ExcludeSkill["Exclude skill from results"] RetryAssessor -->|"ok"| CollectFindings["Collect structured findings"] RunAssessor -->|"ok"| CollectFindings ExcludeSkill --> AllDone{"All skills<br/>processed?"} CollectFindings --> AllDone AllDone -->|"no"| ForEachSkill AllDone -->|"yes"| CheckAllFailed{"All assessments<br/>failed?"} CheckAllFailed -->|"yes"| AllFailStop([Stop: All assessments failed]) CheckAllFailed -->|"no"| StatusAssess["Status: All assessments complete"] %% Step 3: Verify Findings StatusAssess --> IsPlanMode{"Mode = plan?"} IsPlanMode -->|"yes"| SkipVerify["Skip verification<br/>pass findings through unchanged"] IsPlanMode -->|"no"| VerifyLoop["Status: Adversarial verification"] VerifyLoop --> ForEachSkillV["For each skill's findings<br/>(parallel when supported)"] ForEachSkillV --> Classify["Classify findings"] Classify --> PassThrough["PASS + NOT_ASSESSED<br/>verdict = UNCHANGED"] Classify --> Serialize["FAIL + PARTIAL<br/>serialize findings"] Serialize --> HasUnverified{"Any FAIL/PARTIAL<br/>findings?"} HasUnverified -->|"no"| MergeVerified["Merge into verified collection"] HasUnverified -->|"yes"| RunVerifier[/"Subagent: Finding Deep Verifier<br/>all FAIL+PARTIAL in single call"/] RunVerifier -->|"incomplete"| RetryVerifier[/"Retry Verifier (once)"/] RetryVerifier --> CaptureVerdicts["Capture deep verdicts"] RunVerifier -->|"ok"| CaptureVerdicts PassThrough --> MergeVerified CaptureVerdicts --> MergeVerified MergeVerified --> AllVerified{"All skills<br/>verified?"} AllVerified -->|"no"| ForEachSkillV AllVerified -->|"yes"| StatusVerify["Status: All findings verified"] SkipVerify --> StatusVerify %% Step 4: Generate Report StatusVerify --> RunReporter[/"Subagent: Report Generator<br/>mode-specific prompt + verified findings"/] RunReporter --> CaptureReport["Capture report path +<br/>summary counts + severity"] %% Step 5: Completion CaptureReport --> StatusReport["Status: Report generation complete"] StatusReport --> IsPlanReport{"Mode = plan?"} IsPlanReport -->|"yes"| PlanCompletion["Display plan completion format<br/>risk counts + report path"] IsPlanReport -->|"no"| AuditCompletion["Display audit/diff completion format<br/>severity + verification + finding counts"] PlanCompletion --> ExcludedNote{"Excluded skills?"} AuditCompletion --> ExcludedNote ExcludedNote -->|"yes"| AppendNote["Append excluded skills note"] ExcludedNote -->|"no"| Done([Scan Complete]) AppendNote --> Done %% Styling classDef subagent fill:#4a90d9,color:#fff,stroke:#2c5f8a classDef stop fill:#e74c3c,color:#fff,stroke:#c0392b classDef decision fill:#f5c542,color:#333,stroke:#d4a017 classDef status fill:#2ecc71,color:#fff,stroke:#27ae60 class RunProfiler,RunAssessor,RetryAssessor,RunVerifier,RetryVerifier,RunReporter subagent class InvalidStop,EmptyStop,FilterStop,ProfileFail,SkillStop,NoSkillStop,AllFailStop stop class DetectMode,TargetSkill,ValidateSkill,SpecificOverride,CheckEmpty,AllDone,CheckAllFailed,IsPlanMode,HasUnverified,AllVerified,IsPlanReport,ExcludedNote decision class StatusSetup,StatusProfile,StatusAssess,StatusVerify,StatusReport statusRelated Issue(s)
security-revieweragent with OWASP-aligned skill delegation #794owasp-agenticskill for OWASP Agentic Top 10 vulnerability assessment #793owasp-llmskill for OWASP LLM Top 10 vulnerability assessment #796owasp-top-10skill for OWASP Top 10 web vulnerability assessment #795Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderagent and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md)Other:
.ps1,.sh,.py)Sample Prompts (for AI Artifact Contributions)
User Request:
Execution Flow:
Security Revieweragent with promptAnalyse the code base and reproduce a detailed security report. By default the agent will run inauditmode. This will do a full audit of the current codebase.Codebase Profileragent -> Create subagents for each identified owasp skill to analyse codebase against owasp skill's knowledge base viaSkill Assessoragent -> New subagents are created for each owasp skill to verify and challenge the agent's findings viaFinding Deep Verifieragent -> Collate results and generate a report viaReport GeneratoragentOutput Artifacts:
auditmode:.copilot-tracking/security/{date}/security-report-001.mddiffmode:.copilot-tracking/security/{date}/security-report-diff-001.mdplanmode:.copilot-tracking/security/{date}/plan-risk-assessment-001.mdSuccess Indicators:
.copilot-tracking/security/{date}/Testing
I had to modify
CollectionHelpers.psm1fornpm run plugin:generateto work. My owasp skills contained a handful of.mdused for reference.CollectionHelpers.psm1would automatically add these.mds to thehev-core-all.collection.yamlwithkind: "0".kind: "0"is not a recongisedkindand would cause an error and updatingkindtoskillwould just get overridden when you runnpm run plugin:generateagain. To resolve this I updated the script to ignore.mdunder theskillsfolderChecklist
Required Checks
AI Artifact Contributions
/prompt-analyzeto review contributionprompt-builderreviewRequired Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run lint:psnpm run plugin:generateSecurity Considerations
Additional Notes