[Bank Acc. Reconciliation with Copilot] Read the functional part of the prompt from .resources and safety clause from Key Vault - #10090
Conversation
…he prompt from .resources
| Error(ConstructingPromptFailedErr); | ||
| end; | ||
|
|
||
| CompletionTaskTxt := AddCompletionPromptLine(CompletionTaskTxt, NavApp.GetResourceAsText('BankAccRecAIMatchingTask.md', TextEncoding::UTF8)); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
[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
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