Repository navigation
Knowledge: isolated events, FilterGroup scope, BLOB CalcFields and stream text encoding - #228
Conversation
…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>
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed 6eccb86db249e01092be1833390880478780439d. Two merge-critical issues remain:
set-text-encoding-on-streams-exchanged-externally.md:26and 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.- Eight samples in four new pairs reference undeclared demo tables:
Integration Log Demo,Shipment Header Demo,Expense Claim Demo, andExport 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>
|
Addressed both blockers from Jesper Schulz-Wedde (@JesperSchulz)'s review in commit 36a5bf3.
Validation passed:
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. |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
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>
|
Addressed Jesper Schulz-Wedde (@JesperSchulz)'s latest review and integrated current Microsoft main in merge commit 871b515.
Validation on the merged tree passed:
No AL compilation/runtime execution or model-score result is claimed. |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
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
|
Stefano Demiliani (@demiliani) The conflicts are resolved and validated at 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. |
Resolve BCQuality microsoft#228 merge conflicts
|
Jesper Schulz-Wedde (@JesperSchulz) Merged demiliani#2 using Create a merge commit, without squash or rebase. The source branch now points to merge commit Verified that all six approved articles and twelve companions are unchanged from the previous approved source head. GitHub now reports this PR as Thanks for preparing the synchronization PR. |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
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.
f820aa1
into
microsoft:main
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.alsamples. All platform claims link Microsoft Learn.isolated-events-are-isolated-only-outside-a-write-transactionIsolated = trueevent raised after uncommitted writes runs as a normal event, so a subscriber error rolls back the caller. The opposite mistake is addingCommit()just to obtain isolation.[20..]isolated-event-rollback-does-not-undo-var-parametersIsHandled := true, so the publisher skips its default logic. Changing the flag on a shipped event stays with the existingdo-not-change-shipped-event-attribute-flags.[20..]page-scope-filters-must-not-be-in-filtergroup-zerorestore-the-filtergroup-after-setting-hidden-filtersSubPageLink.calcfields-blob-before-reading-its-streamGet/Finddo not load BLOB content, soCreateInStreamreturns an empty stream with no error.set-text-encoding-on-streams-exchanged-externallyTextEncoding, 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-servicesnot-applicablenow 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-ChangedPathsFileand-PrepareDirectory, so deterministic ranking selects every new article): PASSEDtools/Test-ReviewContract.ps1,.github/scripts/Test-SkillIndex.ps1,.github/scripts/Test-KnowledgeIndex.ps1(420 articles): PASSEDvalidate_frontmatter.pywas not run locally (no Python on this machine). Its rules were checked by hand; CI will confirm.docs/contributing.mddescribes.