[Alexander Yakunin (Alexander-Ya)]

What & why

Linked work

Fixes #

How I validated this

• I read the full diff and it contains only changes I intended.
• I built the affected app(s) locally with no new analyzer warnings.
• I ran the change in Business Central and confirmed it behaves as expected.
• I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The source file name d365icsetup.permissionset.al does not follow the PascalCase ..al convention. Rename it to a PascalCase object name with the PermissionSet type segment, while preserving the declared permission-set object identity.

Knowledge:

• microsoft/knowledge/style/file-name-object-type-pattern.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The source file name d365icview.permissionset.al does not follow the PascalCase ..al convention. Rename it to a PascalCase object name with the PermissionSet type segment, while preserving the declared permission-set object identity.

Knowledge:

• microsoft/knowledge/style/file-name-object-type-pattern.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The ordinary procedure parameter ChildrenSetupDiagnostic is declared as a temporary record without the Temp prefix. Rename it to TempChildrenSetupDiagnostic and update its uses so the temporary nature is visible at call sites.

Knowledge:

• microsoft/knowledge/style/temporary-variable-temp-prefix.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

The existing local Purchase Line events OnBeforeUpdateICPartner and OnAfterUpdateICPartner were removed from the base table and replaced by a different publisher, OnAfterUpdateItemReference, while the old publishers were recreated in a table extension. This removes the original base-table event contracts for dependent subscribers; preserve the original publishers and their parameters while moving the implementation.

Knowledge:

• microsoft/knowledge/events/treat-local-and-internal-events-as-subscriber-contracts.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

GetPurchaseInvoiceLine no longer guards its item-ledger and Value Entry lookups with Rec.Type = Rec.Type::Item and ItemLedgerEntry."Entry No." <> 0. Non-item lines and lines without an item ledger entry therefore execute an unnecessary database lookup; restore both guards before querying Value Entry. The same regression is present in the corresponding country-layer copies.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if Rec.Type = Rec.Type::Item then begin
            CheckApplFromItemLedgEntry(ItemLedgerEntry);

            if ItemLedgerEntry."Entry No." = 0 then
                FindItemLedgerEntryFromItemApplicationEntry(ItemLedgerEntry);

            if ItemLedgerEntry."Entry No." <> 0 then begin
                ValueEntry.SetLoadFields("Item Ledger Entry No.", "Item Ledger Entry Type", "Document Type", "Document No.", "Document Line No.");
                ValueEntry.SetRange("Item Ledger Entry No.", ItemLedgerEntry."Entry No.");
                ValueEntry.SetRange("Item Ledger Entry Type", ItemLedgerEntry."Entry Type");
                ValueEntry.SetRange("Document Type", ValueEntry."Document Type"::"Purchase Invoice");
                if ValueEntry.FindFirst() then begin
                    PurchInvLine.Get(ValueEntry."Document No.", ValueEntry."Document Line No.");
                    exit;
                end;
            end;
        end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Privacy}$

The cross-environment intercompany connector posts record JSON to an external Business Central API without checking approval for a dedicated privacy notice for this integration. Register a stable custom notice and require its approval before the request is issued.

Knowledge:

• microsoft/knowledge/privacy/privacy-notice-consent-for-external-data-transfer.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The ordinary procedure parameter xPurchLine is declared as a temporary Record but does not start with Temp, making it look like a database-backed record. Rename the parameter to a Temp-prefixed name and update its local call sites.

Knowledge:

• microsoft/knowledge/style/temporary-variable-temp-prefix.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ AppSource}$

The Intercompany app's assignable role permission sets add tabledata grants but do not grant execute permission for the app-owned codeunits used by setup and normal workflows. A user assigned only the app's Intercompany roles can therefore reach the app's pages but fail when those workflows invoke the moved codeunits; add the required codeunit execute grants to the assignable roles (or included focused permission sets) and validate representative setup and usage paths without SUPER.

Knowledge:

• microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Breaking\ Changes}$

The moved IC Gen. Journal Line table extension exposes implementation helpers such as CheckICPartner and GetDefaultICPartnerGLAccNo as public procedures, widening the former protected/local surface into a supported external API without an indicated API reason. Keep these helpers internal (or local where they are used by only one object) and reserve public access for deliberate extension points.

Knowledge:

• microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Breaking\ Changes}$

The moved Standard General Journal Line table extension exposes implementation helpers such as CheckICPartner and GetDefaultICPartnerGLAccNo as public procedures, widening the former protected/local surface into a supported external API without an indicated API reason. Keep these helpers internal (or local where they are used by only one object) and reserve public access for deliberate extension points.

Knowledge:

• microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

The OnBeforePostICPartner IsHandled guard can exit before the intercompany partner posting routine creates the balancing G/L entry and performs its posting validations. Do not make this critical ledger operation suppressible; limit IsHandled to a safe calculation or run the ledger-entry creation unconditionally.

Knowledge:

• microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

OnUpdateICStatusOnAfterLoopIteration is raised inside the repeat/until loop, so every processed outbox transaction invokes all subscribers separately and scales event overhead with the number of rows. Move the extensibility point outside the loop or expose a set-level operation instead of publishing once per iteration.

Knowledge:

• microsoft/knowledge/events/do-not-publish-events-inside-loops.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

The OnBeforePostGLAccICLine IsHandled guard can exit before an intercompany purchase posting line is validated and its intercompany journal line is created. Do not allow this OnBefore event to suppress the critical posting work; make only a safe calculation overridable or execute the posting unconditionally.

Knowledge:

• microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

The OnBeforeInsertICGenJnlLine IsHandled guard can exit before intercompany purchase journal-line validation, account lookup, and journal-line creation. This lets a subscriber bypass critical posting work; restrict the override to a safe calculation and keep the posting operation unconditional.

Knowledge:

• microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

The OnBeforePostGLAccICLine IsHandled guard can exit before an intercompany sales posting line is validated and its intercompany journal line is created. Do not allow this OnBefore event to suppress the critical posting work; make only a safe calculation overridable or execute the posting unconditionally.

Knowledge:

• microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Events}$

The OnBeforeInsertICGenJnlLine IsHandled guard can exit before intercompany sales journal-line validation, account lookup, and journal-line creation. This lets a subscriber bypass critical posting work; restrict the override to a safe calculation and keep the posting operation unconditional.

Knowledge:

• microsoft/knowledge/events/do-not-bypass-critical-operations-with-ishandled.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance}$

The persistent buffer tables are checked with IsEmpty() and then read with FindSet(), causing an unnecessary existence-query round trip before each iteration. Replace each guard in AssignOperationIDToBufferObjects with FindSet() as the loop guard; the same redundant pattern is repeated for the buffer collections through this procedure.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if BufferICInboxJnlLine.FindSet() then begin

Knowledge:

• microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Privacy}$

The new IC Bank Account field "Bank Account No." stores a financial account identifier but inherits the table's broad CustomerContent classification. Add a field-level DataClassification = AccountData so the platform classifies the identifier according to its actual data.

Knowledge:

• microsoft/knowledge/privacy/data-classification-required-on-pii-fields.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Privacy}$

The new IC Bank Account field "IBAN" stores a financial account identifier but inherits the table's broad CustomerContent classification. Add a field-level DataClassification = AccountData so the platform classifies the identifier according to its actual data.

Knowledge:

• microsoft/knowledge/privacy/data-classification-required-on-pii-fields.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Security}$

The permission check for changing sensitive IC Partner fields returns immediately when GuiAllowed() is false. Table event subscribers also run for API, background, and job-queue sessions, so those sessions can modify the sensitive endpoint, token, client-secret, and connection-key fields without the required D365 IC Partner Edit or SUPER permission. Remove the noninteractive bypass and enforce authorization for every session type, with an explicit separate policy only if a trusted automation path is intended.

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The payment-journal test declares labels with one or more placeholders, but the comments only describe every value generically as a field, value, or account information. Replace those descriptions with the actual meaning of each argument so translators can distinguish the placeholders.

Knowledge:

• microsoft/knowledge/style/label-comment-explains-placeholders.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The posting-batch labels Text012 through Text020 and Text026 through Text027 contain multiple placeholders without comments explaining what each argument represents. Add one translator-facing description for every placeholder.

Knowledge:

• microsoft/knowledge/style/label-comment-explains-placeholders.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Data\ Modeling}$

The custom master table "IC G/L Account" uses a Code[20] "No." as its primary key, but its OnInsert trigger does not assign a blank number from a configured number series or record the originating "No. Series". Inserts therefore depend on callers supplying every business identifier manually and cannot follow the standard master numbering and manual-entry flow. Add a setup-configured number series and assign it in OnInsert when "No." is blank.

Knowledge:

• microsoft/knowledge/data-modeling/master-table-no-from-number-series-in-oninsert.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The payment-journal validation labels contain multiple placeholders, but their Comments only say 'field, value, or account information' and do not identify each argument. Replace those generic descriptions with the actual semantic meaning of every placeholder.

Knowledge:

• microsoft/knowledge/style/label-comment-explains-placeholders.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

These labels use multiple placeholders without a Comment that identifies each placeholder's meaning. Add one semantic description per placeholder so translators can distinguish the transaction, document type, code, and table values.

Knowledge:

• microsoft/knowledge/style/label-comment-explains-placeholders.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

This four-placeholder label has no Comment describing the document type, document number, partner, and window placeholders. Add a Comment with one named description for each placeholder.

Knowledge:

• microsoft/knowledge/style/label-comment-explains-placeholders.md

Posting this finding as an issue comment because inline comment placement failed.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6}