Skip to content

Consume BCQuality knowledge base for Copilot PR review#8700

Merged
aholstrup1 merged 10 commits into
mainfrom
private/waabusea/bcquality-codereview
Jun 24, 2026
Merged

Consume BCQuality knowledge base for Copilot PR review#8700
aholstrup1 merged 10 commits into
mainfrom
private/waabusea/bcquality-codereview

Conversation

@WaelAbuSeada

@WaelAbuSeada WaelAbuSeada commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

Migrates the Copilot PR reviewer from a self-maintained instructions/skill model to consuming the proven, centrally-maintained BCQuality knowledge base through a small, config-driven clone + filter integration layer.

The reviewer now pulls its review knowledge (performance, privacy, security, style, upgrade) from microsoft/BCQuality at runtime, instead of carrying a duplicated copy of that guidance in this repo.

What changed

Added — BCQuality integration layer (tools/BCQuality/)

  • bcquality.config.yaml — single source of truth for which BCQuality content this repo consumes (repo/ref, enabled layers, disabled skills, knowledge allow/deny, task-context). Every value is overridable at runtime via Actions variables.
  • scripts/Get-BCQualityConfig.ps1 — loads the YAML and applies BCQUALITY_* env overrides.
  • scripts/Invoke-BCQualityFilter.ps1 — prunes the BCQuality clone to policy and emits a _filter-report.json artifact for auditability.
  • README.md — documents the layer.

Updated — orchestrator (tools/Code Review/scripts/Invoke-CopilotPRReview.ps1)

  • Consumes the filtered BCQuality clone (BCQUALITY_ROOT / BCQUALITY_SHA) and reads BCQuality's entry.md rather than the in-repo instruction files.
  • Emits structured findings, maps BCQuality severities/domains, and supports both knowledge-backed and agent findings.

Updated — runner workflow (.github/workflows/CopilotPRReviewRunner.yaml)

  • Clones + filters BCQuality on the privileged runner, then runs the review.
  • Uses the built-in GITHUB_TOKEN + copilot-requests: write to bill Copilot inference to the org (no PAT secret required).
  • Clones via init + fetch + checkout so the pinned ref may be a branch, tag, or commit SHA.

Config

  • BCQuality ref is pinned to a specific main commit (822cae1b2771ac25f665f73369f69093bd4fd630) for reproducible reviews; bump deliberately as BCQuality advances.

Removed — now sourced from BCQuality

  • tools/Code Review/instructions/*.md (accessibility, performance, privacy, security, style, upgrade)
  • tools/Code Review/skills/al-code-review/SKILL.md

Fixes AB#637778

Replace the local al-code-review skill and instructions with the BCQuality consumption model: the workflow now checks out microsoft/BCQuality at main, and the review script copies that clone into the runner workspace and instructs Copilot CLI to start from BCQuality skills/entry.md. The script's flat JSON output contract is preserved so the comment-posting pipeline is unchanged.
Replace the naive direct-checkout bridge with the config-driven clone+filter pattern from the tested reference implementation:

- Add tools/BCQuality integration layer (config YAML, Get-BCQualityConfig, Invoke-BCQualityFilter, README)

- Replace orchestrator with the version that consumes BCQuality native structured DO output (agent findings, references, confidence, interrupted-JSON repair, filter report in PR summary)

- Runner workflow now clones the configured BCQuality repo/ref, filters it to policy, and passes BCQUALITY_ROOT/SHA + BCQUALITY_* + AGENT_MINIMUM_SEVERITY; uploads the filter report
@github-actions github-actions Bot added the Build: Automation Workflows and other setup in .github folder label Jun 20, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jun 20, 2026
@JesperSchulz JesperSchulz requested a review from Copilot June 20, 2026 21:17
JesperSchulz
JesperSchulz previously approved these changes Jun 20, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the Copilot PR review orchestration to consume centrally maintained review skills/knowledge from microsoft/BCQuality at runtime, replacing the duplicated in-repo instruction/skill content with a config-driven clone + filter layer.

Changes:

  • Added tools/BCQuality/ integration layer (config + config loader + deterministic filter with audit report artifact).
  • Updated the PR review runner workflow to fetch/filter BCQuality and run the orchestrator with BCQuality as the working directory (pinned ref for reproducibility).
  • Removed the legacy in-repo AL review skill and domain instruction markdown files now sourced from BCQuality.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
.github/workflows/CopilotPRReviewRunner.yaml Adds BCQuality fetch/filter, copilot-requests: write, and switches Copilot CLI auth to built-in token.
tools/BCQuality/bcquality.config.yaml Pins BCQuality ref and defines enabled layers + knowledge allow/deny + task-context defaults.
tools/BCQuality/scripts/Get-BCQualityConfig.ps1 Loads YAML config and applies env overrides for Actions variables.
tools/BCQuality/scripts/Invoke-BCQualityFilter.ps1 Prunes cloned BCQuality content and emits _filter-report.json.
tools/BCQuality/README.md Documents the shared BCQuality integration layer and override schema.
tools/Code Review/scripts/Invoke-CopilotPRReview.ps1 Orchestrator now boots via BCQuality skills/entry.md and parses BCQuality DO-contract output.
tools/Code Review/README.md Documents the updated two-workflow pattern, severity mapping, and BCQuality-backed findings model.
tools/Code Review/skills/al-code-review/SKILL.md Removed (now sourced from BCQuality).
tools/Code Review/instructions/accessibility.md Removed (now sourced from BCQuality).
tools/Code Review/instructions/performance.md Removed (now sourced from BCQuality).
tools/Code Review/instructions/privacy.md Removed (now sourced from BCQuality).
tools/Code Review/instructions/security.md Removed (now sourced from BCQuality).
tools/Code Review/instructions/style.md Removed (now sourced from BCQuality).
tools/Code Review/instructions/upgrade.md Removed (now sourced from BCQuality).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tools/BCQuality/scripts/Invoke-BCQualityFilter.ps1 Outdated
Comment thread tools/Code Review/README.md Outdated
Comment thread tools/Code Review/README.md Outdated
Comment thread .github/workflows/CopilotPRReviewRunner.yaml
Comment thread tools/BCQuality/scripts/Get-BCQualityConfig.ps1 Outdated
@gggdttt gggdttt marked this pull request as ready for review June 23, 2026 09:05
@gggdttt gggdttt requested review from a team June 23, 2026 09:05
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1 Outdated
Comment thread .github/workflows/CopilotPRReviewRunner.yaml Outdated
Comment thread tools/Code Review/README.md Outdated
Comment thread tools/BCQuality/scripts/Invoke-BCQualityFilter.ps1 Outdated
Comment thread tools/Code Review/README.md Outdated
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
gggdttt
gggdttt previously approved these changes Jun 23, 2026
JesperSchulz
JesperSchulz previously approved these changes Jun 23, 2026
…ish jobs

Isolate the tool-enabled Copilot CLI (which processes untrusted PR diff
content with --allow-all-tools) in a read-only 'review' job and move all
comment/issue writes into a separate 'publish' job that never runs the model.
Only the raw agent output crosses the job boundary as an artifact.

Also:
- Fix Test-GlobMatch regex-escape bug (-contains against a single string
  never matched metacharacters, so globs were not escaped).
- Expand the Get-BCQualityConfig defaults block for readability.
- Update Code Review README: drop the obsolete COPILOT_GH_TOKEN secret and
  document GH_TOKEN/GITHUB_TOKEN authentication and the two-job security model.
@gggdttt gggdttt dismissed stale reviews from JesperSchulz and themself via f73df2e June 23, 2026 09:36
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Full agent output persisted as world-readable artifact

The raw Copilot CLI output (al-code-review-raw.txt) and the CLI transcript (agent-transcript.log) are uploaded as workflow artifacts without scrubbing. Because the agent runs with --allow-all-tools, it may have read and reproduced content from arbitrary files in the PR worktree (including secrets, tokens, or PII) to support its review. Any user with download access to workflow artifacts — including collaborators on forks — can then retrieve that content.

Recommendation:

  • Redact known secret patterns from the raw output before persisting it, or limit artifact retention to a short window and restrict download access to repository admins via the actions: write scope check, or remove al-code-review-raw.txt from uploaded artifacts and keep only the structured findings JSON.

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1 Outdated
Comment thread .github/workflows/CopilotPRReviewRunner.yaml
Comment thread .github/workflows/CopilotPRReviewRunner.yaml Outdated
Comment thread tools/BCQuality/scripts/Get-BCQualityConfig.ps1
Comment thread tools/BCQuality/scripts/Invoke-BCQualityFilter.ps1
Comment thread tools/BCQuality/scripts/Invoke-BCQualityFilter.ps1
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

$BaseUrl built from unvalidated GITHUB_REPOSITORY

$BaseUrl is constructed as "https://api.github.com/repos/$Repository" where $Repository = $env:GITHUB_REPOSITORY. While GitHub Actions sets this automatically, any caller running the script locally with a crafted environment variable (e.g., GITHUB_REPOSITORY=../../admin) could redirect all API calls to unintended endpoints. No format validation or URL-encoding of the value is performed before it is embedded in the base URL.

Recommendation:

  • Validate $Repository against a strict owner/repo pattern (e.g., ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$) before constructing $BaseUrl, and throw an informative error if the value does not match.
if ($Repository -notmatch '^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$') {
    throw "GITHUB_REPOSITORY must be in 'owner/repo' format. Got: $Repository"
}
$BaseUrl = "https://api.github.com/repos/$Repository"

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Query string built via string concatenation, not encoding

In Invoke-GitHubApi, query parameters are assembled as "$($_.Key)=$($_.Value)" strings joined with &, without any URL-encoding of key or value. If a value (e.g., a PR title passed as a query parameter in a future call) contained & or =, it would silently corrupt the query string. While current callers only pass integer-valued parameters (per_page, page), this is a latent correctness and potential injection risk.

Recommendation:

  • URL-encode both key and value using [System.Uri]::EscapeDataString() before concatenation, so the function is safe by construction for any future caller.
$qs = ($Query.GetEnumerator() | ForEach-Object {
    "$([System.Uri]::EscapeDataString($_.Key))=$([System.Uri]::EscapeDataString([string]$_.Value))"
}) -join '&'

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
Comment thread tools/Code Review/scripts/Invoke-CopilotPRReview.ps1
Comment thread .github/workflows/CopilotPRReviewRunner.yaml
Comment thread .github/workflows/CopilotPRReviewRunner.yaml
Comment thread .github/workflows/CopilotPRReviewRunner.yaml
Comment thread tools/BCQuality/scripts/Get-BCQualityConfig.ps1
- Format-AnnotationMessage: encode '%' to '%25' before replacing newlines so
  agent output containing literal %0A/%0D cannot be replayed as injected
  workflow-command terminators.
- Assert-Config: validate BASE_BRANCH against a strict git ref-name pattern
  before it is interpolated into git refspecs.
- Skip Build-TaskContext/Save-TaskContext entirely in the post phase; the
  result is unused there and re-parsing the BCQuality config wastes work and
  can fail spuriously when the clone is absent.
- Invoke-BCQualityFilter: make the path-traversal root-containment check
  case-insensitive (OrdinalIgnoreCase) so it holds on Windows runners.
- publish job: also require needs.review.outputs.head_sha != '' so findings are
  never posted against an empty/wrong commit SHA.
@gggdttt gggdttt closed this Jun 23, 2026
@gggdttt gggdttt reopened this Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build: Automation Workflows and other setup in .github folder

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants