[Wael (WaelAbuSeada)]

Summary

This prototype uptakes BCQuality as the AL review knowledge plugin while keeping trusted GitHub orchestration in AL-Go.

• BCQuality owns AL expertise: review skills, guidance, knowledge articles, report schemas, and product-specific review behavior.
• AL-Go owns execution: GitHub workflows, settings, permissions, Copilot CLI setup, review orchestration, diagnostics, artifacts, and comment publication.
• Generated Per Tenant Extension and AppSource App repositories receive the same two-workflow integration.

Architecture

SetupBCQuality

Actions/SetupBCQuality is the sole BCQuality installation boundary. It:

• installs the configured BCQuality release and plugin from the marketplace repository
• validates and applies enabled layers, disabled skills, knowledge allow/deny rules, and task context
• filters the installed plugin deterministically
• produces the prepared pluginRoot and filter report consumed by the reviewer

ReviewAgent does not clone, install, or filter BCQuality.

ReviewAgent

Actions/ReviewAgent consumes an already prepared BCQuality plugin and exposes four phases:

1. context validates trusted settings and pull-request metadata.
2. prepare installs and verifies the exact Copilot CLI version.
3. generate runs the read-only AL review against the validated PR head.
4. post publishes previously generated artifacts using the separate write-capable job.

The review engine is split into focused helper scripts for Git operations, execution, schema/report validation, telemetry, artifact handling, finding placement, deduplication, and publication.

Version selection follows the AL-Go action contract:

• Copilot CLI uses the exact version from the action's pinned @github/copilot dependency by default.
• The workflow supplies review agent version 10.0.7.
• Generated templates intentionally reference AL-Go actions through mutable @main refs, following the existing AL-Go template convention; they do not guarantee one immutable action revision across jobs.

Run diagnostics record the action repository and requested ref. An immutable action commit is recorded only for a SHA-pinned action reference; mutable refs record a null commit, rather than claiming the reviewed repository's SHA is the action source. Diagnostics also include the agent version, BCQuality version, requested models, and Copilot usage telemetry.

Trusted workflow boundary

The generated integration uses two workflows:

• CopilotPRReview.yaml performs unprivileged PR intake and uploads validated event metadata.
• CopilotPRReviewRunner.yaml starts through workflow_run, reads trusted settings from the base repository, prepares BCQuality, generates the review with read-only permissions, and hands artifacts to a separate publishing job with write permissions.

Review generation is bound to the validated pull-request head SHA. Untrusted PR code is never used as workflow configuration or executable review infrastructure.

Settings

The settings contract is intentionally separated:

• BCQuality selects the marketplace repository, BCQuality release, plugin version, layers, skills, knowledge filters, and AL task context.
• CodeReviewAgent controls AL-Go orchestration policy such as models, eligible owners, and review behavior.

This keeps AL product knowledge in BCQuality and repository/workflow policy in AL-Go.

Validation

• Focused review engine, metrics, action wrapper, and workflow suites: 414 passed under PowerShell 7, with no skipped or unrun cases.
• Action wrapper and workflow suites: 47 passed under Windows PowerShell 5.
• Prepare, generate, and post orchestration is exercised with mocked npm, Copilot, and engine entry points, including environment inputs and failure propagation.
• Marketplace setup suite: 23 passed under both PowerShell 5 and 7, including installed-plugin configuration and CLI failure paths.
• PowerShell ScriptAnalyzer: no findings in changed scripts.
• The engine suite is enforced by PowerShell 7 CI in both matrix jobs; the previous stale diagnostic assertions have been corrected.
• Workflow templates remain identical for PTE and AppSource projects.
• Real BCQuality v1.7 installation, filtering, and plugin consumption succeeded in the canary below.

End-to-end canary

Validated with WaelAbuSeada/al-go-copilot-review-canary#3:

• trusted intake, context resolution, and settings loading succeeded
• Copilot CLI 1.0.88 and standalone SetupBCQuality succeeded
• all 19 leaf reviews plus root consolidation completed
• root model gpt-5.6-sol and leaf model gpt-5.6-luna were observed with complete usage telemetry
• run manifest recorded agent 10.0.7, BCQuality 1.7, and the exact AL-Go source commit
• generation/upload/download/publication artifact handoff succeeded
• inline AL findings were published on the intended PR lines
• rerun publication skipped repeated and near-duplicate findings; materially new model findings can still be added, and obsolete findings are not currently removed

Successful runs:

• Initial full canary: https://github.com/WaelAbuSeada/al-go-copilot-review-canary/actions/runs/38654794137
• Rerun/deduplication canary: https://github.com/WaelAbuSeada/al-go-copilot-review-canary/actions/runs/38656028877

Follow-up work

• authenticate existing-comment deduplication metadata against the expected bot/app identity
• preserve canonical accepted leaf reports during root consolidation
• enforce captured source path and line bounds on root findings

[Alexander Holstrup (aholstrup1)]

Intermediate review

[Copilot]

🟡 Changes recommended

Review orchestration can proceed after filtering removes its required super-skill contract.

1 open finding

• Validate disabled review super-skill before launching reviews · New

3 resolved since last review

• Use ordinal comparison for case-sensitive suggested fixes
• Update stale diagnostics to satisfy the validation gate
• Prevent duplicate findings with PR-scoped publish concurrency

Files not reviewed (1)

• Actions/package-lock.json: Generated file

Previously missed (1)

In code that hasn't changed since last review

Fix PowerShell regex escaping for BCQUALITY_CONSUME

Tests/​WorkflowSanitation/​WorkflowFileContent.Test.ps1:139

Backslashes do not escape $ in PowerShell strings. This double-quoted pattern interpolates the process's BCQUALITY_CONSUME value instead of matching the source variable: when unset it only checks = 'plugin', and when set to plugin it creates an invalid \p regex escape. Use a literal string escaped for regex matching so the assertion is independent of the environment.

🧠 Review effort: Balanced

[Wael (WaelAbuSeada)]

Also fixed the overview-only BCQUALITY_CONSUME assertion: it now uses regex escaping on a literal PowerShell source string rather than interpolating the environment. Workflow tests pass with BCQUALITY_CONSUME=plugin under PS5, and the 377 engine/metrics/workflow PS7 cases all pass. The super-skill finding is fixed with a pre-execution configuration error, not by weakening or bypassing the review contract.

[Copilot]

🔵 Needs a closer look

Shared output files can mask eligibility-test failures, and mutable action references conflict with the stated immutable-source guarantee.

0 open findings

1 resolved since last review

• Validate disabled review super-skill before launching reviews

Files not reviewed (1)

• Actions/package-lock.json: Generated file

Previously missed (2)

In code that hasn't changed since last review

Use immutable action SHAs in runner templates

Templates/​Per Tenant Extension/​.github/​workflows/​CopilotPRReviewRunner.yaml:63

Both the PTE and AppSource runner templates use mutable @main references for the AL-Go actions, contrary to the PR description's immutable action-source guarantee. Separate jobs can resolve different revisions; the scenario documentation explicitly says diagnostics record a null commit for mutable refs. Either generate all phases and setup with one resolved action SHA and update the reference assertions, or correct the PR description to match the intended mutable-reference behavior.

Reset shared output files between parameterized tests

Tests/​ReviewAgent.Action.Test.ps1:25

The parameterized opt-in and authoritative-eligibility tests reuse opt-in-output.txt and eligibility-output.txt. Write-ReviewOutput appends, and TestDrive cleanup occurs at Describe/Context boundaries, not between these cases. Once the first case writes eligible=false, later cases can incorrectly write eligible=true and still pass Should -Contain 'eligible=false'. Reset both files in BeforeEach so each case checks only its own output.

🧠 Review effort: Balanced

[Wael (WaelAbuSeada)]

Addressed both overview-only follow-ups from the latest review (which reported zero open findings). Eligibility tests now truncate their output file at the start of each parameterized case; all 36 action tests pass under both PS5 and PS7. For action pinning, I chose the explicitly offered documentation correction rather than changing AL-Go template behavior: the PR description now accurately states that AL-Go actions use mutable Vlad Vannov (@main) references and diagnostics record a null action commit for those refs. It no longer promises immutable source across jobs.

[Copilot]

🔵 Needs a closer look

Per-file Git diffs can expand filename patterns and produce incorrect inline-comment coordinates.

0 open findings

Files not reviewed (1)

• Actions/package-lock.json: Generated file

Previously missed (2)

In code that hasn't changed since last review

Treat per-file Git diff paths as literal pathspecs

Actions/​ReviewAgent/​scripts/​Git.ps1:155

-- ends option parsing but still treats $FilePath as a Git pathspec. A path such as src/Code[1].al can also match src/Code1.al, mixing multiple files' patches into a line map that has no file boundaries. This can place findings on incorrect lines or cause inline publication to fail. Make this per-file diff literal with --literal-pathspecs, while preserving intentional glob support in changed-file discovery.

Add coverage for marketplace registration and plugin configuration paths

Actions/​SetupBCQuality/​SetupBCQuality.ps1:107

Every test in Tests/SetupBCQuality.Action.Test.ps1 sets SETUP_BCQUALITY_PLUGIN_ROOT, so none exercises marketplace registration, installation, or installed-plugin configuration loading. Add Pester cases with that override unset, a mocked Copilot CLI, and realistic configuration fixtures. Cover successful installation, nonzero CLI exits, missing or malformed configuration, ambiguous plugin selection, and manifest-version mismatches. Filtering tests and the reported canary do not provide regression coverage for these paths.

🧠 Review effort: Balanced

[Wael (WaelAbuSeada)]

Addressed the two overview-only follow-ups. Per-file Git diffs now use --literal-pathspecs; a real-Git regression with Code[1].al and Code1.al verifies one-file patches while changed-file discovery retains glob support. Added nine marketplace-installation cases without SETUP_BCQUALITY_PLUGIN_ROOT, covering registration/install arguments, loaded configuration/source metadata, CLI errors, missing/malformed JSON, absent/ambiguous plugin selection, and manifest-version mismatch. All 23 setup tests pass under PS5 and PS7, and all 367 engine/metrics cases pass under PS7.

[Copilot]

🔵 Needs a closer look

The credential and model-artifact boundaries feeding privileged publication require final human security review.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Misclassify deleted dashed separators as file headers in hunks

Actions/​ReviewAgent/​scripts/​FindingPlacement.ps1:31

A deleted dashed separator inside an AL block comment produces a patch line beginning with ---. This condition treats it as a file header, so it falls through and increments both coordinates instead of only the old-file coordinate. Subsequent additions in the same hunk are then mapped one line too far down, which can misplace inline findings. Skip metadata before the first hunk, then treat every leading - or + within a hunk as a deletion or addition. Add a regression case with a deleted dashed separator followed by a changed statement.

1 file not reviewed

• Actions/package-lock.json: Generated file

_{Review effort: Balanced}

[Jesper Schulz-Wedde (JesperSchulz)]

Wael (@WaelAbuSeada) BCQuality v1.8 is now released. It adds the read-only plan guidance preview (al-development-plan), SARIF export, the Community Copilot domain, and additional knowledge, and its plugin manifest now reports 1.8.0.

Could you update this PR's default BCQuality version from 1.7 to 1.8 before it ships? I found it in:

• Actions/.Modules/ReadSettings.psm1: the default "version" = "1.7"
• Scenarios/settings.md: the documented default version 1.7

Test inputs that merely use 1.7 as an example value can stay unchanged unless a test asserts the default.