Skip to content

[Bank Acc. Reconciliation with Copilot] Read the functional part of the prompt from .resources and safety clause from Key Vault - #10090

Open
dcenic wants to merge 1 commit into
mainfrom
features/636129BankAccRecWithCopilotSplitPrompt
Open

[Bank Acc. Reconciliation with Copilot] Read the functional part of the prompt from .resources and safety clause from Key Vault#10090
dcenic wants to merge 1 commit into
mainfrom
features/636129BankAccRecWithCopilotSplitPrompt

Conversation

@dcenic

@dcenic dcenic commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

[Bank Acc. Reconciliation with Copilot] Read the functional part of the prompt from .resources and safety clause from Key Vault

What & why

When constructing the prompts for this feature, read the functional part from .resources, and only the safety clause from the Key Vault.

Linked work

Fixes AB#636129

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)

New tests are not needed, because the current prompt and harms tests are invariant to this change. They let the app build the prompt, and the prompts are updated in the navenlistmentkeyvault too, where the tests run.

Risk & compatibility

Low

@dcenic
dcenic requested a review from a team August 10, 2026 13:09
@dcenic
dcenic requested review from a team as code owners August 10, 2026 13:09
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Finance GitHub request for Finance area labels Aug 10, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 10, 2026
Error(ConstructingPromptFailedErr);
end;

CompletionTaskTxt := AddCompletionPromptLine(CompletionTaskTxt, NavApp.GetResourceAsText('BankAccRecAIMatchingTask.md', TextEncoding::UTF8));

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\ —\ error-handling}$

In both BuildBankRecCompletionTask (codeunit 7250) and BuildMostAppropriateGLAccountPromptTask (codeunit 7251), the newly added NavApp.GetResourceAsText('...Task.md', TextEncoding::UTF8) call is concatenated straight into the SecretText prompt with no check on its result, right after a Key Vault secret read that is explicitly guarded (CompletionTaskBuildingFromKeyVaultFailed -> Session.LogMessage + Error(ConstructingPromptFailedErr)). If GetResourceAsText throws its own runtime error when the packaged resource is missing, this is only a minor inconsistency (an unhandled generic platform error instead of the app's structured error+telemetry path); if it can instead return empty text without erroring, an incomplete/degraded AI prompt could silently be sent. Recommend validating the returned resource text is non-empty and routing failures through the same ConstructingPromptFailedErr + telemetry path used for the Key Vault read, for a consistent, structured failure mode either way.

Agent judgement — not directly backed by a BCQuality knowledge article.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4

CompletionTaskPartTxt: SecretText;
SafetyClauseTxt: SecretText;
CompletionTaskBuildingFromKeyVaultFailed: Boolean;
ConcatSubstrTok: Label '%1%2', Locked = true;

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}$

BuildMostAppropriateGLAccountPromptTask in codeunit 7251 declares a new procedure-local ConcatSubstrTok: Label '%1%2', Locked = true; and calls SecretStrSubstNo directly, duplicating the exact %1%2 concatenation pattern already implemented as the private helper AddCompletionPromptLine in codeunit 7250 "Bank Rec. AI Matching Impl." (which itself also declares this same label locally, inside that helper). BCQuality's Label-scope guidance requires Labels to live in the object's top-level var block rather than procedure-local blocks, and explicitly calls out not duplicating a shared literal across procedure-scoped declarations in multiple places. Codeunit 7251 already has a BankRecAIMatchingImpl: Codeunit "Bank Rec. AI Matching Impl." variable in scope in this procedure — expose/reuse a shared concatenation helper instead of re-declaring the label and logic here.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4

begin
if GetAzureKeyVaultSecret(CompletionTaskPartTxt, 'BankAccRecAIMatching1') then
CompletionTaskTxt := CompletionTaskPartTxt
if GetAzureKeyVaultSecret(SafetyClauseTxt, 'BankAccRecAIMatchingSft') 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\ —\ upgrade}$

Both codeunits rename the Azure Key Vault secret identifiers they look up via GetAzureKeyVaultSecret: 'BankAccRecAIMatching1' -> 'BankAccRecAIMatchingSft' (BankRecAIMatchingImpl.Codeunit.al) and 'BankAccRecAITransToGLAccount1' -> 'BankAccRecAITransToGLAccountSft' (BankAccRecTransToAcc.Codeunit.al), with no fallback to the old name. These codeunits already exist pre-diff (this is a modification, not new code), so the old identifiers were presumably already the ones provisioned in the platform's Key Vault for released environments. If the Key Vault entry rename is not coordinated with this code deployment, GetAzureKeyVaultSecret returns false and the feature now hits ConstructingPromptFailedErr for tenants/environments still serving the old secret name. Note this is Microsoft's own platform-managed Azure Key Vault (via the Azure Key Vault system codeunit), not a customer-provisioned secret, so the risk is mitigated if Microsoft's own deployment pipeline provisions the new secret name before/alongside this code ships — but the PR diff gives no visible evidence of that coordination (e.g. no dual-name fallback, no rollout note). Confirm the Key Vault secret has been (or will be) provisioned under the new name before this code is deployed, or add a fallback read of the old name during the rollout window.

Agent judgement — not directly backed by a BCQuality knowledge article.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4

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 Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant