Skip to content

feat(security): add basic security reviewer agent with owasp skills#1008

Open
JasonTheDeveloper wants to merge 47 commits intomicrosoft:mainfrom
JasonTheDeveloper:feat/794
Open

feat(security): add basic security reviewer agent with owasp skills#1008
JasonTheDeveloper wants to merge 47 commits intomicrosoft:mainfrom
JasonTheDeveloper:feat/794

Conversation

@JasonTheDeveloper
Copy link

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:

Mode Description Scenario
audit Full audit of target code base for common vulnerabilities For use on existing code base. Will take time to complete if repository is large
diff Only analyse changes on current branch For use during validation stage after code base has been modified. Could also be used within a PR
plan Must provide an implementation plan. Analyse implementation plan to highlight potental vulnerabilities to look out for with mitigation suggestions For use at planning stage to guide the agent before the implementation stage to mitigate the risk of vulnerabilities from entering code base

There are also two additional inputs you can pass:

  • targetSkill: Run a particular skill - skips codebase profiling step
  • scope: Restricts agent to a particular directory/file

Detail 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 status
Loading

Related Issue(s)

Type of Change

Select all that apply:

Code & Documentation:

  • 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:

  • 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.
  • 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 and Model Version Requirements.

Other:

  • Script/automation (.ps1, .sh, .py)
  • Other (please describe):

Sample Prompts (for AI Artifact Contributions)

User Request:

Analyse the code base and reproduce a detailed security report containing common vulnerabilities

Execution Flow:

  1. The user switches to the Security Reviewer agent with prompt Analyse the code base and reproduce a detailed security report. By default the agent will run in auditmode. This will do a full audit of the current codebase.
  2. The Security Reviewer agent will then proceed with the following execution steps via subagents: Analyse codebase and select relevant owasp skills via Codebase Profiler agent -> Create subagents for each identified owasp skill to analyse codebase against owasp skill's knowledge base via Skill Assessor agent -> New subagents are created for each owasp skill to verify and challenge the agent's findings via Finding Deep Verifier agent -> Collate results and generate a report via Report Generator agent
  3. Report contains the results of the assessment, including links to offending files with details explanation of the findings, and remediation suggestions

Output Artifacts:

  • audit mode: .copilot-tracking/security/{date}/security-report-001.md
  • diff mode: .copilot-tracking/security/{date}/security-report-diff-001.md
  • plan mode: .copilot-tracking/security/{date}/plan-risk-assessment-001.md

Success Indicators:

  • A details report is generated and saved under .copilot-tracking/security/{date}/
  • Report should contain the following:
    • Summary count
    • Serverity breakdown
    • Verification summary
    • Findings by framework
    • Detailed remediation guidance
    • Disproved findings

Testing

Check Command Status
Markdown linting npm run lint:md ✅ Pass
Spell checking npm run spell-check ✅ Pass
Frontmatter validation npm run lint:frontmatter ✅ Pass
Skill structure validation npm run validate:skills ✅ Pass
Link validation npm run lint:md-links ✅ Pass
PowerShell analysis npm run lint:ps ✅ Pass
Plugin freshness npm run plugin:generate ✅ Pass

I had to modify CollectionHelpers.psm1 for npm run plugin:generate to work. My owasp skills contained a handful of .md used for reference. CollectionHelpers.psm1 would automatically add these .mds to the hev-core-all.collection.yaml with kind: "0". kind: "0" is not a recongised kind and would cause an error and updating kind to skill would just get overridden when you run npm run plugin:generate again. To resolve this I updated the script to ignore .md under the skills folder

Checklist

Required Checks

  • Documentation is updated (if applicable)
  • Files follow existing naming conventions
  • 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:

  • Markdown linting: npm run lint:md
  • Spell checking: npm run spell-check
  • Frontmatter validation: npm run lint:frontmatter
  • Skill structure validation: npm run validate:skills
  • Link validation: npm run lint:md-links
  • PowerShell analysis: npm run lint:ps
  • Plugin freshness: npm run plugin:generate

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues
  • Security-related scripts follow the principle of least privilege

Additional Notes

@JasonTheDeveloper JasonTheDeveloper requested a review from a team as a code owner March 13, 2026 09:32
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.04%. Comparing base (27fbd33) to head (c9db04f).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1008   +/-   ##
=======================================
  Coverage   88.04%   88.04%           
=======================================
  Files          45       45           
  Lines        7885     7885           
=======================================
  Hits         6942     6942           
  Misses        943      943           
Flag Coverage Δ
pester 86.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

katriendg added a commit that referenced this pull request Mar 17, 2026
…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.
Copy link
Contributor

@katriendg katriendg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-formats skill
  • 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`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - yes absolutely we need to include this dependency in the collection level because it will not automatically be deduced and added.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


## Constants

Skill resolution: Locate a skill's entry file by searching for `{skill-name}/SKILL.md`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 186f26b2 as 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 description frontmatter 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-10 skill to access vulnerability references"
  • "Read the security-reviewer-formats skill 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@katriendg
Copy link
Contributor

Forgot one thing, I believe I did not see an update to the file .github/CUSTOM-AGENTS.md which lists all available agents. Could you add it there as well?

@JasonTheDeveloper
Copy link
Author

Since everything is marked maturity: experimental, consumers know to expect further iteration with this version.

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 (owasp-*) probably won't change after this PR but I can see the agents and potentially the security-reviewer-formats skill being tweaked while we implement the other phases. I've updated the collections to reflect this in revert(collection): remove experimental tag from owasp skills.

That's my thinking though. Happy to take your lead if you see things differently 😊

Forgot one thing, I believe I did not see an update to the file .github/CUSTOM-AGENTS.md which lists all available agents. Could you add it there as well?

Done in feat(agent): update CUSTOM-AGENTS.md with security review agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants