Skip to content

[TEST – DO NOT MERGE] Validate BCQuality suggested inline fixes (re-test of #8772)#8816

Closed
JesperSchulz wants to merge 1 commit into
mainfrom
jesperschulz-bcquality-suggested-fixes-test
Closed

[TEST – DO NOT MERGE] Validate BCQuality suggested inline fixes (re-test of #8772)#8816
JesperSchulz wants to merge 1 commit into
mainfrom
jesperschulz-bcquality-suggested-fixes-test

Conversation

@JesperSchulz

@JesperSchulz JesperSchulz commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

⚠️ THIS IS A TEST PR — DO NOT MERGE

This PR exists solely to re-validate the BCQuality Copilot PR-review wiring, specifically that the review agent's suggested inline fixes (GitHub ```suggestion blocks) now render and can be committed with one click.

Why a re-test?

In the first test PR (#8772) the BCQuality review agent posted review comments, but its suggested fixes did not work as one-click GitHub suggestions. A fix has since been deployed, and this PR re-runs the exact byte-identical sample diff from #8772 so we have an apples-to-apples comparison.

Intentional reviewable issues

The sample code is deliberately crafted with issues so the review agent has clear things to flag and attach suggestion blocks to: magic number 500, Commit() inside loops, GET inside loop, discarded Count()/FindFirst() results, PII fields without DataClassification, plain-text credentials, uppercase reserved keywords, spaces before parentheses, an off-by-one loop, and PII in telemetry.

What we are verifying

References: #8772, microsoft/BCAppsBCQuality#47

Again: this is a throwaway test PR and must NOT be merged or marked ready for review.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Replicates the byte-identical sample diff from #8772 / microsoft/BCAppsBCQuality#47
to confirm that the BCQuality review agent's suggested inline fixes (```suggestion
blocks) now render and are committable. In #8772 the agent's review comments did
not work as one-click GitHub suggestions; a fix has since been deployed and this
PR re-tests it with an apples-to-apples, byte-identical diff (350 insertions, 0
deletions across 5 files).

The sample code intentionally contains reviewable issues (magic number 500,
Commit() inside loops, GET inside loop, discarded Count()/FindFirst() results,
PII fields without DataClassification, plain-text credentials, uppercase reserved
keywords, spaces before parentheses, off-by-one loop, PII in telemetry) so the
review agent has clear things to flag and attach suggestion blocks to.

This is a throwaway TEST branch and is NOT meant to be merged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234'

@JesperSchulz JesperSchulz marked this pull request as ready for review June 25, 2026 09:47
@JesperSchulz JesperSchulz requested a review from a team June 25, 2026 09:47
if not PromptAndImportIntoInStream(FilterString, FileInStream, ServerFileName) then
exit(ImportedCount);

TempBlob.CreateInStream(FileInStream);

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.

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

In ImportBatch, TempBlob.CreateInStream(FileInStream) is called on a freshly-initialised, empty Temp Blob (line 55).

This replaces FileInStream — which at this point holds the file content returned by PromptAndImportIntoInStream — with an empty stream derived from the empty blob. The uploaded file data is silently discarded; the procedure then increments the counter and calls Commit() around an empty operation, returning an inaccurate count of successfully imported files. This is a material correctness defect. If buffering in a TempBlob is required, first copy the stream via TempBlob.CreateOutStream and CopyStream, then call TempBlob.CreateInStream. If no buffering is needed, remove the TempBlob.CreateInStream call entirely and consume FileInStream directly. Note that this concern is non-gating as an agent finding; it warrants promotion to a knowledge-backed rule before it can gate a build.

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

if ResultBuffer.FindSet() then
repeat
// GET inside the loop re-reads the same customer on every iteration.
Customer.Get(CustomerNo);

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.

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Customer.Get(CustomerNo) is called inside the repeat…until loop in ImportResults, issuing one database round-trip against the Customer table per result buffer entry.

Because the same CustomerNo is passed on every iteration, this is N identical reads of the same production-scale table row. Lift Customer.Get(CustomerNo) before the FindSet call; a single Get before the loop is sufficient.

Knowledge:

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

ImportLogEntry.Insert();

// Commit inside the loop defeats the implicit transaction boundary.
Commit();

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.

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Commit() is called inside a repeat…until loop in ImportResults.

Each row iteration starts a new write transaction, preventing atomic rollback of the whole import operation. If a later ImportLogEntry.Insert() fails, the already-committed rows cannot be unwound. Per the referenced guidance, move the commit to a clearly-defined safe boundary outside the loop, or use bounded checkpoints that each process N rows.

Knowledge:

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

ImportedCount += 1;

// Commit after every imported file, inside the loop.
Commit();

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.

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Commit() is called inside a for loop in ImportBatch, issuing one Commit per file iteration.

The referenced guidance flags Commit inside any per-item loop as almost always a mistake. Remove Commit() from inside the loop; the implicit commit at successful procedure exit is sufficient unless an explicit checkpoint strategy is required.

Knowledge:

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

begin
ImportLogEntry.SetRange("Customer No.", CustomerNo);
// Count() materializes a number the caller does not need.
if ImportLogEntry.Count() > 0 then

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.

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

HasImportedResults uses Count() > 0 to test for existence (line 74), then immediately calls FindFirst() as a second existence check (line 77).

Both are the anti-patterns flagged by the referenced guidance: Count() materialises a full count the caller discards, and FindFirst() materialises a row the caller discards. Additionally, because Count() = 0 implies no rows, the FindFirst() branch (line 77) is unreachable. Replace both checks with a single not ImportLogEntry.IsEmpty().

Suggested change
if ImportLogEntry.Count() > 0 then
exit(not ImportLogEntry.IsEmpty());

Knowledge:

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn't look right. It should have suggested to replace both line 74 and 75 with the new suggestion, which now is a one-liner.

begin
// PII (customer name) placed directly in the telemetry message string,
// and a placeholder event id that was never registered.
Session.LogMessage(

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.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Session.LogMessage is called with StrSubstNo('Imported results for customer %1', Customer.Name) as the message argument (line 143).

Customer.Name is a personal name (CustomerContent) and is baked into the plain-text message string before Session.LogMessage runs. The DataClassification argument on the call does not protect the message text; the platform ships whatever is in that string verbatim to telemetry. Per the referenced guidance, the message must be a static non-personal string; move structured context to CustomDimensions. Note: the event ID '0000' is also a placeholder and should be replaced with a registered telemetry event ID.

Knowledge:

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

TelemetryScope::All,
'Category', 'QualityImport');

Dimensions.Add('CustomerName', Customer.Name);

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.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Dimensions.Add('CustomerName', Customer.Name) places the customer's personal name — CustomerContent / EUII — directly into the FeatureTelemetry CustomDimensions dictionary.

Per the referenced guidance, CustomDimensions values are sent to telemetry as-is with no platform classification applied. Remove Customer.Name from the dictionary and replace with a non-personal signal (for example, a count or a status enum) to preserve the telemetry's usefulness without exposing PII.

Knowledge:

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

Comment on lines +29 to +32
field(3; "Customer Name"; Text[100])
{
Caption = 'Customer Name';
}

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.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Field 'Customer Name' (Text[100]) stores a customer's personal name and has no DataClassification property.

Omitting DataClassification defaults to ToBeClassified, which tells the platform the data has not been reviewed. For a field storing a personal name, DataClassification = CustomerContent is required so GDPR data-subject requests, telemetry, and audit surfaces treat the field correctly.

Suggested change
field(3; "Customer Name"; Text[100])
{
Caption = 'Customer Name';
}
field(3; "Customer Name"; Text[100])
{
Caption = 'Customer Name';
DataClassification = CustomerContent;
}

Knowledge:

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

Comment on lines +33 to +36
field(4; "Contact Email"; Text[80])
{
Caption = 'Contact Email';
}

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.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Field 'Contact Email' (Text[80]) stores an email address and has no DataClassification property.

An email address is EndUserIdentifiableInformation or CustomerContent (depending on whether it belongs to the tenant's end-user or their customer). Either way DataClassification must be set explicitly. Omitting it leaves the field as ToBeClassified and prevents the platform from correctly handling GDPR requests and telemetry classification.

Suggested change
field(4; "Contact Email"; Text[80])
{
Caption = 'Contact Email';
}
field(4; "Contact Email"; Text[80])
{
Caption = 'Contact Email';
DataClassification = CustomerContent;
}

Knowledge:

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

/// subscribers can override them.
/// </summary>
[IntegrationEvent(false, false)]
procedure OnBeforeCallLabApi(Endpoint: Text; ApiKey: Text; var BearerToken: Text)

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.

$\textbf{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

OnBeforeCallLabApi exposes ApiKey (Text) and BearerToken (var Text) as integration event parameters.

Any extension on the tenant can subscribe to this event and read or overwrite both credentials with no permission check. Per the referenced guidance, authentication is handled by the publisher — credentials must never appear in an event signature. Remove ApiKey and BearerToken from the event parameters; the publisher should apply credentials internally before or after raising the event.

Knowledge:

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


local procedure FetchResultsIntoBuffer(CustomerNo: Code[20]; var ResultBuffer: Record "Qlty. Import Log Entry" temporary)
var
ApiKey: Text;

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.

$\textbf{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

ApiKey and BearerToken are declared as Text in FetchResultsIntoBuffer (lines 84–85) and passed through DownloadResults as Text parameters (line 97).

The referenced guidance requires credential-carrying variables to be SecretText end-to-end so the compiler prevents accidental plain-text exposure and the debugger cannot read them. Change the type of ApiKey and BearerToken to SecretText in both the local variable declarations and the DownloadResults signature, and update GetApiKey() / GetAccessToken() to return SecretText.

Knowledge:

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

begin
// Error message composed with StrSubstNo instead of passing the
// parameter directly to Error().
Error(StrSubstNo('Could not import results for customer %1', CustomerNo));

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.

$\textbf{🟠\ High\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

RaiseImportError calls Error(StrSubstNo('Could not import results for customer %1', CustomerNo)).

StrSubstNo appearing as a direct argument to Error() is the anti-pattern the referenced guidance flags unconditionally: it hides the format-string identity from analyzers, breaks the translation pipeline's ability to link the call to its label, and removes the platform's ability to classify substituted parameters individually. Declare a Label with an Err suffix and a Comment for the placeholder, then call Error(YourErr, CustomerNo). Note also that the format string is a hardcoded literal rather than a Label, which prevents translation.

Knowledge:

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

Comment on lines +113 to +122
VAR
ValidCount: Integer;
BEGIN
IF ImportLogEntry.FindSet() THEN
REPEAT
IF ImportLogEntry."Result Value" > 0 THEN
ValidCount += 1;
UNTIL ImportLogEntry.Next() = 0;
EXIT(ValidCount);
END;

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.

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

The entire body of CountValidEntries (lines 113–122) uses uppercase reserved keywords: VAR, BEGIN, IF, THEN, REPEAT, UNTIL, EXIT.

CodeCop AA0241 requires keywords to be lowercase in new AL code. The standard AL formatter normalises casing automatically; applying it to this procedure body resolves all instances.

Suggested change
VAR
ValidCount: Integer;
BEGIN
IF ImportLogEntry.FindSet() THEN
REPEAT
IF ImportLogEntry."Result Value" > 0 THEN
ValidCount += 1;
UNTIL ImportLogEntry.Next() = 0;
EXIT(ValidCount);
END;
var
ValidCount: Integer;
begin
if ImportLogEntry.FindSet() then
repeat
if ImportLogEntry."Result Value" > 0 then
ValidCount += 1;
until ImportLogEntry.Next() = 0;
exit(ValidCount);
end;

Knowledge:

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

Comment on lines +129 to +131
if ImportLogEntry.Get (EntryNo) then
exit (ImportLogEntry."Customer Name");
exit ('');

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.

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

DescribeEntry contains three calls with a space before the opening parenthesis: 'ImportLogEntry.Get (EntryNo)' (line 129), 'exit (ImportLogEntry."Customer Name")' (line 130), and 'exit ('')' (line 131).

CodeCop AA0002 forbids whitespace between a name and its opening parenthesis. The standard AL formatter removes these spaces automatically.

Suggested change
if ImportLogEntry.Get (EntryNo) then
exit (ImportLogEntry."Customer Name");
exit ('');
if ImportLogEntry.Get(EntryNo) then
exit(ImportLogEntry."Customer Name");
exit('');

Knowledge:

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

Comment on lines +29 to +31
field("Customer No."; Rec."Customer No.")
{
}

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.

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

Field 'Customer No.' on page 'Qlty.

Import Log' has no ToolTip property. CodeCop AA0218 requires a non-empty ToolTip on every page field control. Missing tooltips suppress the hover text and screen-reader announcement for this field.

Suggested change
field("Customer No."; Rec."Customer No.")
{
}
field("Customer No."; Rec."Customer No.")
{
ToolTip = 'Specifies the customer number for the import log entry.';
}

Knowledge:

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

Comment on lines +32 to +34
field("Customer Name"; Rec."Customer Name")
{
}

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.

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

Field 'Customer Name' on page 'Qlty.

Import Log' has no ToolTip property. CodeCop AA0218 requires a non-empty ToolTip on every page field control.

Suggested change
field("Customer Name"; Rec."Customer Name")
{
}
field("Customer Name"; Rec."Customer Name")
{
ToolTip = 'Specifies the name of the customer for the import log entry.';
}

Knowledge:

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

Comment on lines +35 to +37
field("Contact Email"; Rec."Contact Email")
{
}

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.

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

Field 'Contact Email' on page 'Qlty.

Import Log' has no ToolTip property. CodeCop AA0218 requires a non-empty ToolTip on every page field control.

Suggested change
field("Contact Email"; Rec."Contact Email")
{
}
field("Contact Email"; Rec."Contact Email")
{
ToolTip = 'Specifies the contact email address for the import log entry.';
}

Knowledge:

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

Comment on lines +42 to +44
field("Imported At"; Rec."Imported At")
{
}

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.

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

Field 'Imported At' on page 'Qlty.

Import Log' has no ToolTip property. CodeCop AA0218 requires a non-empty ToolTip on every page field control.

Suggested change
field("Imported At"; Rec."Imported At")
{
}
field("Imported At"; Rec."Imported At")
{
ToolTip = 'Specifies the date and time when the results were imported.';
}

Knowledge:

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

/// </summary>
procedure GetDialogTitle(): Text
begin
exit (ImportFromLbl);

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.

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

GetDialogTitle contains 'exit (ImportFromLbl)' with a space before the opening parenthesis (line 70).

CodeCop AA0002 forbids whitespace between a name and its opening parenthesis.

Suggested change
exit (ImportFromLbl);
exit(ImportFromLbl);

Knowledge:

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

@github-actions

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 1 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

Findings by domain

Findings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).

Domain Findings Knowledge-backed Agent Inline Fallback
Agent 1 0 1 1 0
Performance 4 4 0 4 0
Privacy 4 4 0 4 0
Security 2 2 0 2 0
Style 8 8 0 8 0

Totals: 18 knowledge-backed · 1 agent findings.

Orchestrator pre-filter (13 file(s) excluded)

  • layer-disabled (knowledge) : 13 file(s)

Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.

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

Labels

AL: Apps (W1) Add-on apps for W1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant