Skip to content

Knowledge: isolated events, FilterGroup scope, BLOB CalcFields and stream text encoding - #228

Merged
Jesper Schulz-Wedde (JesperSchulz) merged 5 commits into
microsoft:mainfrom
demiliani:knowledge/core-al-gaps
Oct 20, 2026
Merged

Jesper Schulz-Wedde (JesperSchulz) merged 5 commits into
microsoft:mainfrom
demiliani:knowledge/core-al-gaps

Conversation

@demiliani

@demiliani Stefano Demiliani (demiliani) commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Why

The corpus had no guidance on four AL mechanics that agents get wrong: grep finds 0 articles on FilterGroup, 0 on isolated-event runtime behaviour, and only incidental mentions of BLOB stream reading and text encoding. Each concern below either causes a silent defect, which the compiler and analyzers do not catch, or leads to a misleading review comment.

What

Six Microsoft-layer articles, each with .good.al / .bad.al samples. All platform claims link Microsoft Learn.

Domain Article Mistake prevented
events isolated-events-are-isolated-only-outside-a-write-transaction An Isolated = true event raised after uncommitted writes runs as a normal event, so a subscriber error rolls back the caller. The opposite mistake is adding Commit() just to obtain isolation. [20..]
events isolated-event-rollback-does-not-undo-var-parameters Rollback undoes only normal-table writes. A failing subscriber can leave IsHandled := true, so the publisher skips its default logic. Changing the flag on a shipped event stays with the existing do-not-change-shipped-event-attribute-flags. [20..]
security page-scope-filters-must-not-be-in-filtergroup-zero A scope filter set in group 0 can be removed from the filter pane. The article also says a hidden filter group is not a security boundary (use security filters).
security restore-the-filtergroup-after-setting-hidden-filters When the filter group is not restored, a later filter replaces the hidden one. Do not use platform groups 1/3/4/6/7, for example group 4 replaces SubPageLink.
data-modeling calcfields-blob-before-reading-its-stream Get/Find do not load BLOB content, so CreateInStream returns an empty stream with no error.
web-services set-text-encoding-on-streams-exchanged-externally Without a TextEncoding, BLOB streams default to MSDos, which depends on the server locale. Non-ASCII characters break in downloaded files and HTTP payloads. Round trips inside BC are explicitly not flagged.

Every article includes false-positive carve-outs: group 0 used as a removable default, reading a BLOB just written on the same variable, binary content, isolated events raised from read-only contexts.

Wiring

  • al-events-review, al-security-review, al-data-modeling-review, al-web-services-review: diff tokens plus a targeted cue per article. Web-services not-applicable now also covers AL file/stream text exchange, and the data-modeling scope sentences now include BLOB stream reads.
  • evaluation/review-fixtures.json: the six paired articles are added to the existing domain overrides.

Validation

  • tools/Test-ReviewFixtures.ps1 (including -ChangedPathsFile and -PrepareDirectory, so deterministic ranking selects every new article): PASSED
  • tools/Test-ReviewContract.ps1, .github/scripts/Test-SkillIndex.ps1, .github/scripts/Test-KnowledgeIndex.ps1 (420 articles): PASSED
  • validate_frontmatter.py was not run locally (no Python on this machine). Its rules were checked by hand; CI will confirm.
  • AL samples are demonstrations, not compiled, as docs/contributing.md describes.

…ream text encoding

Add six Microsoft-layer articles with good/bad samples for concerns the
corpus did not cover:

- events: isolated events are isolated only outside a write transaction;
  an isolated event's rollback does not undo var parameters (IsHandled).
- security: page scope filters must not be set in filter group 0;
  restore the filter group and avoid platform groups 1/3/4/6/7.
- data-modeling: calculate a BLOB field before reading its stream.
- web-services: set an explicit TextEncoding on BLOB streams whose text
  leaves or enters Business Central (default is MSDos).

Wire each article into its leaf review skill (tokens and targeted cue)
and register the paired articles in the review-fixtures overrides.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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.

Reviewed 6eccb86db249e01092be1833390880478780439d. Two merge-critical issues remain:

  1. set-text-encoding-on-streams-exchanged-externally.md:26 and its leaf routing flag an omitted stream encoding when the stream is passed to download/HTTP, even when no text conversion happens. An explicitly UTF-8 writer followed by a default-encoding InStream used only for DownloadFromStream still transfers the original UTF-8 bytes: creating the InStream does not transcode them. Raw uploads/CopyStream paths have the same distinction. Require an encoding-sensitive text read/write with a missing or incompatible encoding, explicitly exclude byte-preserving transfers of already encoded data, and add that boundary as a clean control.
  2. Eight samples in four new pairs reference undeclared demo tables: Integration Log Demo, Shipment Header Demo, Expense Claim Demo, and Export Buffer Demo. No declarations are supplied in the exact-head tree, and preparation presents each companion independently. In particular the BLOB examples do not establish that Payload/Content are Blob fields at all. Include the necessary minimal tables/fields in each independent sample or use suitable shipped objects; preserve the actual Blob types and status definitions in executable AL.

The broad platform facts and deterministic retrieval were reviewed. Frontmatter, index/retrieval, skill/schema and the standalone contract suite passed in the isolated exact-head harness; all 23 actual changed paths were used and 264 cases prepared with the six article pairs within the retrieval cutoff. No AL compile/runtime or model-score result is claimed. The sample dependencies and stream conversion boundary need fixing despite structural validation.

Exclude byte-preserving transfers from encoding findings and exercise UTF-8 download through a default InStream as a clean control. Include independent demo table declarations with Blob fields and shipment status definitions in all eight affected samples.

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

Copy link
Copy Markdown
Contributor Author

Addressed both blockers from Jesper Schulz-Wedde (@JesperSchulz)'s review in commit 36a5bf3.

  1. Narrowed the stream-encoding article and leaf routing to encoding-sensitive text conversions (WriteText/ReadText) with missing or incompatible encoding, rather than stream creation or transport alone. Explicitly excluded byte-preserving download, stream-based HTTP transfer, raw upload, and CopyStream paths. The existing clean companion now deliberately writes UTF-8 and downloads through a default-encoding InStream, retaining the original bytes.
  2. Added minimal independent table declarations to all eight affected companions: Integration Log Demo (Payload: Blob), Export Buffer Demo (Content: Blob), Shipment Header Demo (key, carrier, and executable Open/Confirmed status members), and Expense Claim Demo (key, employee user ID, amount). Every companion includes its own dependencies. Updated evaluation documentation accordingly.

Validation passed:

  • 264 neutral fixtures across 20 leaf domains, including coverage for the actual PR paths plus the new changes and executable review-contract tests.
  • Explicit checks on prepared neutral inputs: all eight independent demo tables retain the required Blob/status definitions, and the byte-preserving UTF-8 download clean control remains present.
  • Knowledge-index determinism/coverage and retrieval round-trip (420 articles, 727 samples), using an isolated local copy outside OneDrive.
  • Skill-index/schema validation, preserving the existing 19 Microsoft review leaves.
  • git diff --check.

No AL compilation/runtime execution or model-score result is claimed. The Python frontmatter validator could not be run locally because neither Python nor the Python launcher is available; CI can run that check.

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.

Reviewed 36a5bf338b11b23320ddea404c7ce0514464881d. The encoding fix is correct: article/routing now require an actual incompatible text conversion and preserve byte-only transfers; the good control writes UTF-8 and downloads through a default InStream without transcoding. Most missing table declarations are also supplied correctly.

The expense-claim pair still cannot compile: in both microsoft/knowledge/security/page-scope-filters-must-not-be-in-filtergroup-zero.good.al:22-37 and .bad.al:22-37, the new table ... "Expense Claim Demo" declaration is nested inside page.layout.area(Content). AL application objects must be top-level, not layout children. Move each table declaration outside the complete page object; keep the page's OnOpenPage trigger inside the page. Brace balance alone does not establish valid object boundaries.

No further change to the encoding guidance is needed. The exact-head structural/fixture/index contracts pass but do not parse or compile AL, which is why this malformed clean sample remains a blocker. GitHub also still reports merge conflicts against main; those need resolving before merge.

Resolve data-modeling review routing conflicts by retaining both upstream price-rounding and branch BLOB signals. Move expense demo tables to top level while preserving page triggers.

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

Copy link
Copy Markdown
Contributor Author

Addressed Jesper Schulz-Wedde (@JesperSchulz)'s latest review and integrated current Microsoft main in merge commit 871b515.

  • Moved each Expense Claim Demo table outside the complete page object in both companions. OnOpenPage remains inside its page; field definitions and filter behavior are unchanged.
  • Merged Microsoft main at 7619cb3. Resolved the data-modeling routing conflict by retaining both upstream price-rounding signals and this PR's BLOB signals. Preserved the combined evaluation coverage.
  • Left the approved stream-encoding guidance unchanged.

Validation on the merged tree passed:

  • 276 prepared neutral fixtures across 20 leaf domains, including changed-path coverage and executable review-contract checks.
  • Explicit lexical boundary checks on both original source companions and their neutralized inputs: page/table declarations are at depth zero and OnOpenPage is at page depth one. The same check detects the previous table nesting at depth three; this is more than a brace-balance check, but not an AL compiler.
  • Knowledge-index determinism/coverage and retrieval round-trip (428 articles, 743 samples).
  • Skill-index/schema checks preserving the existing 19 Microsoft review leaves.
  • git diff --cached --check and an index with no unresolved conflict entries before commit.

No AL compilation/runtime execution or model-score result is claimed.

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.

Reviewed 871b5155cf28d9a6f0c2f589c1a21ac3c9dcdb28 against the previous reviewed head. The remaining sample-integrity blocker is resolved: both Expense Claim Demo tables now sit outside the complete page object, with each OnOpenPage trigger retained inside its page. All twelve companions have balanced top-level object boundaries and valid names after neutralization. The accepted encoding text/routing and byte-preserving clean control are unchanged.

The main merge retains upstream price-rounding guidance alongside the branch's BLOB signals and preserves every merged evaluation selection. Exact-head actual-changed-path preparation passes 276 cases; contract, skill/schema, frontmatter and knowledge-index/retrieval checks pass locally. No remaining merge-critical content issue found; no new AL compiler/runtime execution is claimed. GitHub still reports conflicts against the newer main and the substantive validation workflows await approval, so the branch needs a refresh and those checks before merge.

Preserve BLOB stream and business-state routing, both fixture selections, and approved knowledge samples.

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

Copilot-Session: c1acef1d-dec9-4c8b-b119-dcfad2f33246
@JesperSchulz

Copy link
Copy Markdown
Contributor

Stefano Demiliani (@demiliani) The conflicts are resolved and validated at daed4d43772d8268199ac17d87fd327f3a1e0c69, preserving the approved article/sample bodies and all main routing/evaluation selections. Our direct push was rejected because the OAuth token lacks workflow scope for a workflow inherited from main, so I opened demiliani/BCQuality#2 against this source branch.

Please merge that sync PR using Create a merge commit (not squash/rebase) so main's ancestry is retained and this PR becomes conflict-free. We are ready to finish merging #228 after the synchronization and checks.

@demiliani

Copy link
Copy Markdown
Contributor Author

Jesper Schulz-Wedde (@JesperSchulz) Merged demiliani#2 using Create a merge commit, without squash or rebase.

The source branch now points to merge commit 21973c338433ae486a5d9e7fd111f486e0bf667a, retaining the validated sync commit daed4d43772d8268199ac17d87fd327f3a1e0c69 and current Microsoft main 843199341e41b99f4b462707f05398112d6677da in its ancestry.

Verified that all six approved articles and twelve companions are unchanged from the previous approved source head. GitHub now reports this PR as MERGEABLE (no conflicts). Checks are running; flag and CLA have passed at the time of this update.

Thanks for preparing the synchronization PR.

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.

Reviewed synced head 21973c338433ae486a5d9e7fd111f486e0bf667a. The contributor merged the sync PR with main ancestry intact. The resulting entire tree is identical to validated conflict-resolution commit daed4d43772d8268199ac17d87fd327f3a1e0c69: all six approved canonical articles and twelve corrected companions are unchanged, both sides of the routing/evaluation conflicts are preserved, and workflow/tool/schema files equal current main. Native local validation of that resolution passed frontmatter, actual 24-path fixture preparation (290 cases), report contract, skill/schema and knowledge index/retrieval. No merge-critical issue remains. I have authorized the exact-head GitHub validation runs and will wait for them before merging.

@JesperSchulz
Jesper Schulz-Wedde (JesperSchulz) merged commit f820aa1 into microsoft:main Oct 20, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants