knowledge(upgrade): upgrade code must not use ChangeCompany

[Jesper Schulz-Wedde (JesperSchulz)]

Addresses ADO bug 651092. Upgrade code should run in the context of the company being upgraded, but nothing in the upgrade corpus told the review agent to flag ChangeCompany there. A model will happily loop the Company table in OnUpgradePerDatabase and write each company's data through ChangeCompany, which compiles and passes analyzers.

What this adds

• microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md (+ .good.al / .bad.al)
  • Why: PerCompany upgrade triggers already run once per company in that company's own system session (Learn). Reaching into another company duplicates or races its own upgrade, sees data in an unknown pre/post-upgrade shape, records per-company upgrade tags in the wrong company, and leaves triggers and trigger-event subscribers running in the calling company.
  • Covers Feature Data Update implementations too; Feature Management already schedules Update Feature Data per company via TaskScheduler.CreateTask(..., CompanyName, ...) (BCApps FeatureManagementImpl.CreateTask).
  • Best practice: per-company work in OnUpgradePerCompany guarded by a per-company tag; reserve PerDatabase for non-per-company data; schedule per-company tasks instead of ChangeCompany when work must start in other companies.
  • Detection: ChangeCompany(<name>) on a Record/RecordRef in an upgrade codeunit, a helper reachable from its triggers, or a Feature Data Update implementation. Reads are not exempt. The parameterless ChangeCompany() and runtime code are excluded.
• al-upgrade-review skill: adds ChangeCompany, Feature Data Update, UpdateData as worklist tokens and a worklist bullet routing to the new article.
• Events article changecompany-runs-triggers-in-the-calling-company.md: cross-reference to the upgrade rule.

Notes for reviewers

• The rule is intentionally strict (reads included), matching the bug's "upgrade code must not contain any ChangeCompany statements". The justification for reads is that company upgrade order is not documented, so another company's data shape is unknown.
• bc-version: [all], countries: [w1], application-area: [all].

Validation

• validate_frontmatter.py: 0 errors (2 pre-existing warnings)
• Test-ReviewFixtures.ps1, Test-ReviewContract.ps1, Test-KnowledgeIndex.ps1, Test-SkillIndex.ps1: pass

Fixes: AB#651092

[Jesper Schulz-Wedde (JesperSchulz)]

Reviewed 4e33ca87e2e69979747dfbe090ed5bc798fd043c. The central company-context guidance is well founded: PerCompany upgrade triggers execute in company-specific system sessions, PerDatabase triggers have no open company, ChangeCompany does not move trigger execution context, and Feature Management schedules migration tasks for a named company. Three merge-critical issues remain:

1. Both companions are not self-contained. no-changecompany-in-upgrade.good.al:13 and .bad.al:8 reference Record "Sales Order Ext", but neither declares it, and the reviewed tree contains no definition. Include the sample table and both required fields in each independent companion, or use an existing table with a complete declared extension. Otherwise the canonical good example cannot compile.
2. The changed-path evaluation contract fails. evaluation/review-fixtures.json selects neither the new no-changecompany-in-upgrade pair nor the modified changecompany-runs-triggers-in-the-calling-company pair. The exact-head GitHub job and a local run with the PR's actual changed paths both report these missing registrations. Add both pairs to their upgrade/events article overrides and verify bounded positive/clean retrieval; preserve existing selections.
3. Feature routing is broader than the migration path the article justifies. Article line 26 and al-upgrade-review.md:47 match any method in a codeunit implementing Feature Data Update, including IsDataUpdateRequired/ReviewData and their read-only helpers. These are distinct interface phases, not necessarily reachable from scheduled UpdateData. A concrete first-party example is VATSettlActCodeFeatDataUpd.Codeunit.al: IsDataUpdateRequired calls CountRecords, which loops companies with ChangeCompany and Count, while UpdateData migrates only the current company. The present route would condemn that preflight solely because it shares the implementation codeunit. Bind feature detection to the actual UpdateData/AfterUpdate execution paths and resolved helpers, rather than the whole codeunit; include a read-only preflight clean control. Keep the intended strict prohibition within the migration execution path. Also include feature-update execution in the skill's applicability/not-applicable scope so an ordinary implementation is not discarded before that route runs.

No source files were changed by this review. As this is my own PR, the blockers are recorded in an exact-head comment rather than a formal change request.

[Jesper Schulz-Wedde (JesperSchulz)]

Reviewed 48bd2e2e1be4300004fb97e6bebc26f5fe6af366 against the previous reviewed head. All three blockers are resolved: both companions independently declare the sample table and required fields; both the new upgrade pair and modified events pair are registered without losing existing selections; and feature detection follows UpdateData/AfterUpdate and resolved reachable helpers rather than treating independent IsDataUpdateRequired/ReviewData preflight as migration execution. The expanded applicability scope and added read-only preflight clean control agree with that boundary. The feature execution/preflight distinction is confirmed against FeatureManagementImpl, and changed-path fixture validation passes 226 cases with the contract/skill checks intact. All GitHub checks pass. No remaining merge-critical issue found; content is ready. Formal self-approval is unavailable.