Skip to content

[API tests] Adopt shared authentication and re-enable scoped AL tests - #11860

Open
Prangshuman Das (t-prda) wants to merge 211 commits into
features/646383-api-test-workflowfrom
features/646383-api-test-uptake
Open

Prangshuman Das (t-prda) wants to merge 211 commits into
features/646383-api-test-workflowfrom
features/646383-api-test-uptake

Conversation

@t-prda

@t-prda Prangshuman Das (t-prda) commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Scope

AB#646383

AL shared-auth adoption and scoped test re-enablement above #11891.

  • Preserve the existing 160 caller files selecting the Microsoft provider during retained-instance initialization.
  • Preserve the agreed 117 work-date caller files / 128 invocation sites and 73 new Disabled-isolation declarations; no broader date/lifecycle rollout.
  • Retain four reviewed exclusion-manifest changes and remove the old Expense helper with its caller migration.
  • Activate enableCleanTestCodeunitExecution: true in .github/AL-Go-Settings.json alongside authentication adoption. This single activation setting intentionally belongs to uptake; the scheduling/discovery implementation remains workflow-owned.
  • Add the single real HTTP regression below in separate CU139496. Total auth/URL/HTTP methods here: 12 across two codeunits; all prior six contracts and five URL methods remain in CU139494.

This review diff changes 168 AL files, four exclusion manifests and one activation-settings file (173 files). Workflow credential/discovery implementation remains in #11891; only explicit activation is owned here.

Authentication request regression

Separate uptake-owned CU139496 API Test Auth HTTP Tests contains MicrosoftAuthenticationRespectsServerAuthMode: the same three real OData service-document requests must return 401/200/401 for UserPassword or 200/200/200 for authorized ambient Windows authentication, in default None / Microsoft / deselected None order. A helper selects only the expected ambient status; the test body executes all three requests with unconditional assertions. The OnPrem constraint remains; no environment is silently skipped. Windows runtime is unverified until tested in an actual Windows-authenticated environment. The existing HTTP request helper executes the requests; no credentials are printed and no public production test event is added. The scenario reads the existing endpoint without fixture writes or new isolation metadata. Its own codeunit boundary prevents the three URL metadata-writing tests in CU139494 from retaining transaction state into the HTTP request; all 11 auth/URL tests remain unchanged. The HTTP scenario uses response assertions only, without mock-event recording assertions. It belongs here because #11891 provisions the container password; core remains runnable before that infrastructure. The Key Vault fallback alone cannot supply AL-Go's random container password.

Review boundaries

Uptake retains 48 Expense exclusions. The four existing Expense layers still remove only their owned 4 + 5 + 1 + 2 methods, leaving 36 at full tip. All upstream exclusion metadata is preserved. The business-fix patches are unchanged relative to their respective refreshed parents.

Native stack #11893: #10085 auth core -> #11862 URL/fixture prerequisites -> #11891 workflow infrastructure -> #11860 AL uptake -> #11224–#11230 -> #11322 -> #11451–#11454.

Validation limits

The previous UserPassword HTTP 401/200/401 result is historical after this reconciliation. CU139496 MicrosoftAuthenticationRespectsServerAuthMode still executes all three requests; Windows200/200/200 runtime remains unverified. The workflow clean-codeunit gate stays default-off and uptake stays explicitly enabled. No provider, authentication contract, new public event, NAV selector or foreign NST change was introduced. Excluded PDF cases and absent/excluded country/native cases remain unverified; prior tolerated-native results are not universal passes. The 59 owned re-enabled methods cover the reviewed fixes, not59 distinct product defects. Validation drafts remain Do Not Merge, outside native stack #11893.

Deferred upstream exclusion

Keep CU139806 APIV2 - Company Info. E2E.TestGetCompanyAndEnvironmentDescriptions disabled. PR11741, commit 5d287d0c253487563e3fd12624ba86ee3c889263, introduced both the test and its exclusion. This preserves the upstream exclusion independently of the authentication rollout, as requested. The original technical disable reason is unconfirmed. This is not a claim of NAV disablement, instability or a rollback defect. Follow up with the original owner to establish the rationale and verify current NAV behavior before considering re-enablement. No new bug is filed.

Runtime verification from direct uptake36779875744: CU139806 was present in21 of22 Integration countries; TestGetCompanyAndEnvironmentDescriptions was skipped in all21, with zero executions. The IN suite was absent—not passing. This confirms the retained exclusion, not its original technical rationale or current NAV status; both still require follow-up.

Current checkpoint

Head da513cc2559be4c95b29f6321ca2c481f7feee4f; captured main eaddd6b3c518414f372154ea6cedd6c12b287359.

Validation BLOCKED; no whole-run or all-country green claim. Current exact-head AL runs: workflow, workflow validation, uptake, uptake validation, full. Remaining jobs are not cancelled.

Platform 30.0.55429.0 reproducibly throws AcquireSqlConnectionFromPool NullReference on the first API GET after tenant reset: uptake IS Integration job111493435528 (CapabilitiesProjectsEnabledViaAPI) and full MX Default job111493443618 (TestGetCurrencyExchangeRates). Both occur56–64 seconds after tenant3 reset; later independent requests pass, but the failed methods do not recover. No scheduler reset/worker overlap was found. Async runtime/pool lifecycle coupling remains possible, unproven—not infrastructure-only. Exact implementation is unavailable in the exposed NAV tree; runtime-owner source/PDB analysis of pool lifetime across dismount/copy/remount is required. Separately, uptake-validation BE job111494278233 fails ordinary SLS installation with a duplicate datasearch sequence before clean execution. Runner/network outages are separate failures. No AL retries, arbitrary waits or classifier broadening will mask these failures.

Verified partial evidence: direct uptake CU148343 passes 153/198 cases across17 countries, including all9 methods and the repaired policy snapshot method. CA/CZ/ES/NL/NO artifacts were missing at2026-10-04T19:58Z; absence is not failure or success. Original8 methods, all assertions, four response clears and five query-safe URLs are preserved alongside upstream's ninth method. Local Pester remains117/117 at workflow/uptake/full. All4 exact-head PowerShell runs passed; workflow-validation37215974877 required attempt2 after one analyzer-tool crash; the other3 passed attempt1. Uptake PowerShell provenance is validation37215973669 at the same uptake SHA, not a separate direct-branch run.

The supported finalizer ran after normal teardown in direct workflow BE Default job111495104917 at2026-10-04T19:30:13.2943924Z and direct uptake DE Integration job111493433096 at2026-10-04T19:37:30.6075547Z. These markers prove hook invocation, not explicit deletion when teardown already removed the file. The23 committed wrappers/shared finalizer and absent generator remain unchanged. Accepted limitation: explicit cleanup is success-only; failed/cancelled runs rely on normal container teardown, with no hard-runner-loss guarantee. Credential ACL protections remain; per-run disposable CI credentials limit risk, not eliminate it.

No CU139496 runtime evidence yet; Legacy1 remains unverified and W1 Default does not cover it. Windows authentication, excluded CompanyInfo (TestGetCompanyAndEnvironmentDescriptions, PR11741), Travel, PDF/native coverage remain unverified/excluded as applicable. Existing review fixes/resolutions, native stack #11893, merged prefix, approval/queue/draft states and upstream exclusions remain unchanged. Permission cleanup from PR11561 is retained. No new source change, baseline update, push, merge or AL retry accompanies this checkpoint. Validation drafts remain Do Not Merge.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Restore exclusions unrelated to the authentication bridge and retain representative GET, POST, PATCH, and E-Document coverage.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Honor RequiredTestIsolation=Disabled for Integration tests and re-enable the Expense Agent API coverage.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Run the read-only API scenarios under the existing Codeunit-isolated Integration pass.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Retain the 22 Expense API scenarios proven under Codeunit isolation while deferring two setup-visibility cases.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Limit the pull-request matrix while iterating on API test coverage. Remove before merge.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Avoid treating unrelated disabled capabilities as project capability failures.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Keep the temporary W1 project filter without disabling incremental baseline resolution.

AB#646383

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept with Suggestions

What this PR does

This change moves API test codeunits to shared Microsoft test-environment authentication, applies a license-safe work date and scoped isolation metadata, re-enables API tests, and enables clean codeunit execution.

All current request-bearing callers select the provider during initialization. The retained-instance ordering is enforced, the app-specific authentication subscriber is removed, and the real HTTP test covers provider selection and deselection under UserPassword while preserving ambient Windows authentication.

Problem-solution fit

Fit: Strong

The bug is caused by Graph requests not receiving the test credential. The shared provider is selected before fixture creation across all current callers, and the reduced exclusion set activates the affected suites.

Suggestions

S1 (🟠 Moderate): Cover binary Graph request callers
Add BinaryUpdateToWebServiceAndCheckResponseCode to this request-method regex. A codeunit using only this public method could omit SetAuthenticationProvider without failing the assertion.

Risk assessment and necessity

Risk: The change touches 160 Graph callers, date-sensitive fixtures, isolation metadata, and four exclusion manifests. Current callers are covered and the exact head passed its complete test matrix, but the Windows-authentication branch remains runtime-unverified.

Necessity: The change is required because the API suites otherwise cannot use the generated UserPassword credential. The broad scope is proportional to the shared-library usage and is guarded by focused metadata checks and a real HTTP regression test.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11860 round=1 by=alexei-dobriansky at=2026-09-28T22:05:15.453Z lastSha=f846c7b9704decd4b01d17a4ca4e2a28943b1b85 reviewKey=ad6a8863f67632e2a75c246e284bb7113bd0f4902cd6fb8b12e2a74d7ae6fc52 suggestions=S1@b343e854

pull Bot pushed a commit to CarstenMertes/BCApps that referenced this pull request Sep 29, 2026
## Purpose


[AB#646383](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_workitems/edit/646383)

Add opt-in authentication to `Library - Graph Mgt.` without API
re-enablement or pipeline changes.

## Design

- Default `None` preserves existing behavior. The public enum/interface
and public `API Test Auth Context` retain external provider
extensibility.
- The Microsoft provider stays `Internal`; context `Apply` stays
internal. Internal visibility is API encapsulation, not authorization.
- Credential precedence remains container file, then existing Key Vault
only if the file is absent. Invalid configured credentials fail;
successful passwords remain instance-scoped `SecretText`.
- File credentials now use a private file-mapping provider directly,
without replacing or clearing the session-wide Azure Key Vault
provider/cache. No extra AL plaintext copy is introduced.
- `OnAfterInitializeWebRequestWithURL` remains last. The pre-existing
empty Graph `OnRun` is restored.

The trust boundary is admitted OnPrem test code and environment
credential access, not `Internal` visibility or a destination-URL
restriction.

## Tests and scope

All six AL contracts remain: default None, event ordering, provider
reuse, instance scoping, same-provider reselection and deselection.
The dedicated recorder is removed. The non-SingleInstance internal mock
publishes the **test-only** `OnAfterConfigureAuthentication` event; the
same manually bound test-codeunit instance owns `Library - Variable
Storage` for both recording and verification. Initialization clears that
instance's queue after prior failures; each contract drains it and calls
`AssertEmpty`.

The six source-pattern PowerShell checks are removed, not replaced by
other AL-text assertions. The real 401/200/401 HTTP scenario is owned by
uptake microsoft#11860 after credential provisioning; core has no dependency on
that future workflow. This is not a claim of a complete provider-branch
matrix.

No caller migration, URL repair, credential provisioning, scheduling,
exclusion or work-date rollout belongs to this core.

Native stack #11893: **microsoft#10085 auth core -> microsoft#11862 URL/fixture
prerequisites -> microsoft#11891 workflow infrastructure -> microsoft#11860 AL uptake ->
microsoft#11224–microsoft#11230 -> microsoft#11322 -> microsoft#11451–microsoft#11454**.

## Current checkpoint

Head `4a76bb71851c158083dbe4d22e84bf40a0577842`, tree
`12739039e502a3feeea9e98694fb360ea959c606`.

Merged main baseline remains `0a602a2481c93ebfe7b1d27d6016fb9926c42111`
(permission cleanup from PR 11561). No additional main merge or code
changes were made during finalization. Old `b195c7a`/`4eef8ac`
tree-equality claims remain obsolete after the intentional baseline/auth
changes.

[Verified AL compile/publish/test run
36131334134](https://github.com/microsoft/BCApps/actions/runs/36131334134)
**succeeded, attempt 2**, at exact validation head
`4a76bb71851c158083dbe4d22e84bf40a0577842`. **113/113 test jobs and
113/113 cleanup steps succeeded.** Core has no credential-file
provisioning/removal step (expected).

W1 artifacts verify all **6 auth contracts**.

Local Pester at the applicable checkpoint: **28 passed, zero
failed/skipped**. Core local Pester: **28 passed**; no unsupported claim
about a separate root PowerShell workflow.

## Validation limits

For uptake/full, CU139496
`MicrosoftAuthenticationRespectsServerAuthMode` is verified in the
actual UserPassword fixture (**401/200/401**). The Windows
**200/200/200** expectations are implemented but **Windows runtime
remains unverified**; no foreign local NST was used. Successful GitHub
runs do not establish universal native NAV coverage. Excluded PDF cases,
country-specific absent/excluded cases and tolerated-native distinctions
are not claimed passing. No PR has been merged or auto-merged;
validation drafts remain Do Not Merge outside stack #11893.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
pull Bot pushed a commit to CarstenMertes/BCApps that referenced this pull request Sep 29, 2026
## Scope


[AB#646383](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_workitems/edit/646383)

Prerequisite layer above microsoft#10085. Move existing **non-authentication**
repairs out of the auth-only base so reviewers can assess them
separately.

- Query-safe Graph URL path/query helpers and
`CreateTargetURLWithTwoSubpages`, with five existing URL regressions.
- Corresponding URL composition changes in API callers.
- Existing APIV2 RapidStart fixture/polling, journal-handler and
Foundation-setup corrections.
- Existing Expense Activity/Policy API fixture and assertion
corrections.
- Existing Czech inventory-posting fixture corrections, also tracked by
microsoft#11370.
- Three existing fixture-variable relocations, retained without claiming
separate product bugs.

No provider-adoption calls, work-date rollout, `RequiredTestIsolation`
changes, pipeline configuration or exclusion changes belong to this
layer. The old Expense auth helper and bindings remain until uptake.

## Dependency and validation

This PR remains the separate 18-file prerequisite above microsoft#10085; its
owned patch is unchanged by the main refresh. The five URL tests remain
alongside the six core contracts in CU139494 (11 methods). Workflow
microsoft#11891 follows it without changing AL source; AL adoption remains
microsoft#11860. No exclusions or pipeline files change here.

The root core's queue/KV fixes are inherited, not duplicated. Standalone
feature-base checks are distinct from main-targeted workflow/uptake/full
validation; none of the previous successful cumulative runs is new-head
proof.

## Current checkpoint

Head `768a6395f186f4db4981631dbcacaa01677c5d40`, tree
`0bea3b816388c79fb950a8ad22bb11ff0f2598be`.

Merged main baseline remains `0a602a2481c93ebfe7b1d27d6016fb9926c42111`
(permission cleanup from PR 11561). No additional main merge or code
changes were made during finalization. Old `b195c7a`/`4eef8ac`
tree-equality claims remain obsolete after the intentional baseline/auth
changes.

[Verified AL compile/publish/test run
36157073629](https://github.com/microsoft/BCApps/actions/runs/36157073629)
**succeeded, attempt 1**, at exact validation head
`9269e3df582704881a898e19029308e9296cbbf1`. **113/113 test jobs and
113/113 cleanup steps succeeded.** **113/113 credential-removal steps**
also succeeded.

W1 artifacts verify all **11 auth/URL methods**. Clean-codeunit
activation remains absent/off; ordinary execution, Legacy and the Unit
Disabled fallback are preserved. This prerequisite is validated as part
of the successful workflow cumulative head, not by a claimed standalone
prerequisite run.

Local Pester at the applicable checkpoint: **28 passed, zero
failed/skipped**. [PowerShell run
36157073348](https://github.com/microsoft/BCApps/actions/runs/36157073348)
**succeeded, attempt 1**.

## Validation limits

For uptake/full, CU139496
`MicrosoftAuthenticationRespectsServerAuthMode` is verified in the
actual UserPassword fixture (**401/200/401**). The Windows
**200/200/200** expectations are implemented but **Windows runtime
remains unverified**; no foreign local NST was used. Successful GitHub
runs do not establish universal native NAV coverage. Excluded PDF cases,
country-specific absent/excluded cases and tolerated-native distinctions
are not claimed passing. No PR has been merged or auto-merged;
validation drafts remain Do Not Merge outside stack #11893.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Keep all workflow behavior tests and the clean-codeunit activation setting. The test file now matches the workflow parent.

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
@github-actions github-actions Bot removed the Build: scripts & configs Build scripts and configuration files label Sep 30, 2026
Keep the activity-log fixture independent of approval-limit defaults; leave production defaults and shared test-user helpers unchanged.

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

Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
…exclusion

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept

What this PR does

Since round 1, the net PR change gives the expense activity-log E2E approver explicit unlimited approval. The validation runs after Can Approve is enabled, clears any approval limit, and makes the re-enabled approval flow independent of record defaults.

Status of previous suggestions
ID Title Status Author response
S1 Cover binary Graph request callers Addressed The source-inspection assertion was removed, so the incomplete request-method regex is no longer part of this PR.
New observations (commits since round 1)

None - the new AL fixture change is valid and only removes dependence on approval-limit defaults.

Risk assessment and necessity

Risk: The PR still spans 168 AL files, authentication setup, work dates, isolation metadata, and exclusion manifests. The changed AL files pass diff integrity checks, completed app builds compile successfully, and the current failed jobs are platform-index network failures rather than AL compiler failures; the full fresh runtime matrix and Windows-authentication branch remain pending.

Necessity: Shared authentication is required so the re-enabled API suites can use the generated UserPassword credential. The new approval fixture value is required to keep the activity-log E2E flow independent of approval-limit defaults.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11860 round=2 by=alexei-dobriansky at=2026-09-30T23:45:22.6751691Z lastSha=875726d09d211ec374ca4dbd3a600a1403340ce4 reviewKey=fc935a8c8ed961ad30e050b80e6e9d917fedaa290da9d65832e016b1afec0cf7 suggestions=S1@b343e854:addressed parentRound=1

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 3

Recommendation: Accept

What this PR does

Since round 2, one non-merge commit replaces five direct URL concatenations in the policy-snapshot API test with the shared path and query helpers. The path helper inserts additions before an existing query string, and the query helper uses & when a tenant query is already present, so all requests keep their tenant context.

Status of previous suggestions
ID Title Status Author response
S1 Cover binary Graph request callers Addressed The removed source-inspection assertion was not reintroduced.
New observations (commits since round 2)

None - both sequential review samples found the incremental URL changes correct. All five changed spans are also present in the net PR diff; the large merge commit only integrates the updated base.

Risk assessment and necessity

Risk: The incremental change is limited to test URL construction and follows existing helper usage. It changes no production API, event, BaseApp dependency, or report layout. The live checks currently show 121 successes and 226 queued jobs; the cancelled review-intake job is unrelated to AL correctness.

Necessity: The change is required because appending paths after ?tenant=... would create invalid request URLs, while adding another query with ? would discard the existing separator contract. The shared helpers preserve the intended tenant for every request in this test.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11860 round=3 by=alexei-dobriansky at=2026-10-02T22:21:40.2609575Z lastSha=ef20ad38eae6769b76516051e5f0c53a2596de8d reviewKey=ec80445f3659b37ced55b5020aafcda02cdd517dbc1f3f26e5c0cbb5ecbd6695 suggestions=none parentRound=2

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
…I test fixes

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91

This branch was successfully deployed

1 active (outdated) deployment
triage — f846c7b9 Deployed Sep 28, 2026 by t-prda via Classify team ownership #5932
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 Build: Automation Workflows and other setup in .github folder Team: Integrations GitHub request for Integrations area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants