The original DO contract pinned all agent reasoning to the super-skill:
'Agent findings are emitted only by super-skills... Leaf sub-skills MUST
NOT emit agent findings'. This funnels all agent reasoning across all 6
domains through a single super-skill pass, which is the root structural
cause of the attention dilution we have been chasing in BCAppsBCQuality
PRs #28 and #30:
- T1 standalone run showed al-security-review finds rimd-on-read-only
cleanly when run alone, but emits zero agent findings because the
contract forbids it. So obvious things like case-without-else (no
matching KB article yet) get dropped on the floor.
- The al-code-review self-review pass keeps producing 0-1 agent findings
per PR because it is asked to reason across 6 domains in one pass.
The fix is to move agent reasoning into the leaves, bounded by each
leaf's domain. Each leaf now has both knowledge-backed and agent-finding
permissions within its own scope; the super-skill self-review pass
becomes a smaller, cross-cutting role.
skills/do.md
- Replace the 'only by super-skills' / 'MUST NOT' clause with a
two-tier model: leaf sub-skills MAY emit agent findings strictly
within their declared domain; super-skills MAY emit agent findings
for cross-cutting concerns that span domains.
- Update the encoding rules: leaf agent findings have references:[]
and an agent:-prefixed id, no from-sub-skill (the leaf's own report
carries the finding under its own skill.id). Super-skills set
from-sub-skill='agent' for their own self-review findings; when
rolling up leaf agent findings, they set from-sub-skill=<leaf-id>.
- Clarify that 'MUST validate against knowledge' applies to super-
skill self-review candidates only - leaves already validated within
their domain when they decided to emit.
microsoft/skills/review/al-{security,performance,privacy,style,upgrade,
ui}-review.md
- New paragraph after the confidence rules instructing each leaf to
surface domain-specific agent findings when no knowledge file
covers a defect the agent recognises from general AL knowledge.
- Bound the scope: 'The scope is strictly <domain>; defects outside
this domain belong to other leaves and MUST NOT be emitted here.'
- Same validation requirement: check the worklist for a matching
knowledge file first; if one exists, upgrade to a knowledge-backed
finding instead.
microsoft/skills/review/al-code-review.md
- Rewrite the 'Agent self-review pass' subsection. The pass is now
explicitly for cross-cutting concerns that no single leaf could
have surfaced because they span multiple domains. Domain-specific
reasoning belongs in the leaves, not duplicated here.
- Update the rollup behavior to acknowledge leaf-emitted agent
findings: they are rolled up like any other sub-skill finding, with
from-sub-skill set to the leaf id, and are not re-validated by the
super-skill (the leaf already validated within its own domain).
- Drop the 'Leaf sub-skills MUST NOT emit agent findings' line.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop the specific pattern checklist (architecture-level smells, error-
handling gaps, magic constants, privacy/telemetry surface, resource
lifecycle - with concrete code patterns under each). It baked case-
study findings into the skill contract and aged badly. Replace with a
domain-level framing: walk by the domains the sub-skills already cover
(performance, security, privacy, style, upgrade, UI) plus the cross-
cutting concerns (architecture, error handling, resource lifecycle).
The domains are anchors for completeness, not a script.
The structural rule - do an explicit self-review pass after the leaves
- is what carries the value. The content of the pass belongs in the
agent's general AL judgement and in the knowledge layer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Execution-discipline change + suggested-code propagation in the
skills is the structural fix. The two knowledge articles
(case-must-handle-unknown-enum-values, instream-length-unreliable-for-bc-streams)
were T4 follow-ups derived from a single parity case study; they need
broader review before landing as canonical BCQuality knowledge and are
out of scope for this PR.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Driven by a parity comparison between BCAppsBCQuality PR #27 and
BCAppsCampAIRHack PR #162 on byte-identical content:
| | BCQuality | AIRHack |
|--|--|--|
| Total findings | 6 | 10 |
| Performance | 0 | 4 |
| Security | 0 | 5 |
Standalone runs of al-security-review and al-performance-review against
the SAME diff produced the expected matches (rimd-on-read-only via
inherent-permissions-minimal-grant; redundant-Get via
avoid-redundant-get-when-record-already-loaded). The miss in the live
run is therefore not a knowledge-coverage gap and not a worklist
filtering issue. It is attention dilution inside the al-code-review
super-skill, which the model collapses into one rolled-up generation
pass on real-size PRs.
Changes:
microsoft/skills/review/al-code-review.md
- New 'Execution discipline (mandatory)' subsection in the Action step
that explicitly forbids collapsing leaves into one shared reasoning
pass and requires each sub-skill to walk its Source -> Relevance ->
Worklist -> Action steps as its own iteration before the next leaf
starts.
- Self-review pass is now described as the final, mandatory iteration
with a concrete candidate-category checklist (architecture-level
smells, error-handling gaps, magic constants, privacy/telemetry,
resource lifecycle). Returning zero agent findings on a real-size
diff is explicitly defined as a defect.
microsoft/skills/review/al-{security,performance,privacy,style,
upgrade,ui}-review.md
- Each leaf skill now states that when an unambiguous .good.al
companion exists, findings[].suggested-code should carry the
literal replacement for the source lines. Closes the
one-click-suggestion gap created when BCQ#19 only updated
al-code-review.
microsoft/knowledge/security/case-must-handle-unknown-enum-values.{md,
bad.al,
good.al}
- New article: case over a security-sensitive enum (Authentication
Type, Authorization Mode, Identity Provider, Permission Scope,
Encryption Algorithm) MUST have an else arm. Without it, an unknown
enum value silently falls through and the security context never
initialises. The bad sample is lifted from the SharePoint Graph
helper that triggered the parity finding.
microsoft/knowledge/performance/instream-length-unreliable-for-bc-
streams.{md,bad,good}
- New article: InStream.Length returns 0 / partial for HTTP-response
streams and some file-API streams, breaking size-threshold branching
in upload code. Bad sample is the simple-vs-chunked Graph upload
pattern; good sample materialises into a Temp Blob first.
Companion change: microsoft/BCAppsBCQuality#28 extends the
orchestrator's bootstrap prompt with the same per-iteration execution
discipline and adds a CI warning when a >5-file PR returns zero agent
findings (regression signal).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the contract field, skill instructions, and two knowledge articles
that BCAppsBCQuality's PR-review agent needs to match (and exceed) the
coverage of the embedded review agent in BCAppsCampAIRHack:
skills/do.md
- New optional findings[].suggested-code field. Documents what it MUST
contain (a literal line-replacement payload) and when to emit it.
microsoft/skills/review/al-code-review.md
- Instructs both the agent self-review pass and rolled-up sub-skill
findings to populate suggested-code when the fix is mechanical.
- Lists examples (dead code removal, Count > 0 -> IsEmpty, object-scope
Label) that map to issues observed in the parity comparison.
microsoft/knowledge/style/telemetry-event-id-stable-unique.{md,bad.al,good.al}
- New knowledge article: telemetry event IDs must be stable, unique,
and non-placeholder. Closes a gap surfaced by the parity comparison.
microsoft/knowledge/style/labels-declared-at-object-scope.{md,bad.al,good.al}
- New knowledge article: Labels must live in the object-level var
block, not in procedure-local var blocks. Closes the second gap.
microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.md
- Adds an explicit note that changing DataClassification alone does not
make embedding PII into the message string acceptable, plus links to
the two adjacent privacy articles. Resolves the privacy advice the
parity comparison flagged as ambiguous.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The article claimed SetLoadFields must be called before filters, but
call order has no impact on the resulting query plan. Removing the
article along with its good/bad AL samples rather than rewriting,
since the premise itself is incorrect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous LLM-generated knowledge files contained factual
hallucinations. The most visible was the claim that `FindFirst` /
`FindLast` "forces a full-table scan" on an unfiltered record - it does
not; those APIs return a single row via the current key.
Other inaccuracies the audit found and fixed:
* `FindSet(true)` was described as "taking a LockTable". The correct
upstream phrasing is that `FindSet(true)` sets
`ReadIsolation::UpdLock` on the read. UpdLock and LockTable are
related but distinct mechanisms.
* The list of production-scale tables had been invented beyond the
upstream source (e.g. "Detailed Cust. Ledg. Entry") without a
citation. The regenerated list matches the ten tables upstream lists
with their P95 row counts.
* `SetLoadFields` guidance had been augmented with an extra mechanism
claim ("the database resolves the filter using the index without
hydrating the value") not present in upstream.
Approach: full regeneration of `microsoft/knowledge/` from the six
upstream BCApps Code Review instruction files, with Microsoft Learn /
the AL language reference as a secondary source. Every claim in every
regenerated file is anchored to a verbatim upstream quote (or a Learn
URL); the audit trail lives in artifacts/trace-<domain>.json on the
session workspace.
The PR #11 transaction/error-handling cluster is preserved verbatim:
* performance/understand-implicit-transaction-boundary.md
* performance/codeunit-run-as-atomic-sub-operation.{md,good.al,bad.al}
* performance/codeunit-run-requires-prior-commit-inside-transaction.{md,good.al,bad.al}
* performance/use-tryfunction-for-error-catching-not-rollback.{md,good.al,bad.al}
* performance/avoid-commit-inside-loops.{md,good.al,bad.al}
* security/commitbehavior-attribute-scopes-explicit-commits.{md,good.al,bad.al}
* testing/transactionmodel-attribute-governs-test-transactions.{md,good.al,bad.al}
These articles already cite Microsoft Learn and were carefully
cross-referenced; the regeneration skips their topics rather than
duplicating them.
File counts after regeneration:
performance 35 .md (5 preserved + 30 new)
privacy 17 .md
security 18 .md (1 preserved + 17 new)
style 33 .md
testing 1 .md (preserved)
ui 19 .md
upgrade 18 .md
Total 141 atomic knowledge files, each strictly one rule. All pass
.github/scripts/validate_frontmatter.py with 0 errors and 0 warnings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Let super-skills surface findings the agent identifies on its own,
clearly tagged so consumers can render them differently from
knowledge-backed ones.
- skills/do.md: permit references:[] when from-sub-skill='agent';
define the agent-finding encoding (id 'agent:<slug>', confidence
capped at medium, self-contained message); restrict agent findings
to super-skills only.
- microsoft/skills/review/al-code-review.md: add a self-review pass
to Action that validates agent-identified candidates against
BCQuality (cite if matched, suppress if contradicted, surface as
agent finding otherwise). Add example finding.
- agent-consumption.md, README.md: describe the additive model and
the from-sub-skill: 'agent' marker so consumer orchestrators know
to render unbacked findings.
Strictly additive: existing knowledge-backed flow is unchanged and
backward compatible.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cover non-obvious platform behaviors a capable LLM reliably gets wrong:
hidden FlowFields still calculate, LockTable scopes to the whole table,
query objects bypass the primary-key cache, table-event subscribers
disable bulk ModifyAll/DeleteAll, Blob fields are uncached, OnCompanyOpen
subscribers block every session creation, and the test framework
disables bulk insert mode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- performance/use-setloadfields-for-partial-records: Clarify that
filter-only fields (SetRange/SetFilter) do not need to be listed in
SetLoadFields — the DB resolves them via the index without hydrating
the value into AL memory.
- performance/avoid-calcfields-in-loops: Add explicit exception for
OnAfterGetRecord and OnValidate triggers, which are platform-managed
and not developer-authored loops.
- performance/split-read-only-and-write-paths-to-avoid-locktable: Add
ReadIsolation as the primary recommendation for read-only paths;
LockTable reserved for confirmed write paths only.
- performance/prefer-direct-record-over-recordref: Scope the finding to
hot unbounded loops (10k+ rows) over ledger-entry-scale tables;
RecordRef in bounded/admin/setup contexts is not a concern.
- upgrade/enum-changes-must-be-additive-at-the-end: Replace direct
ObsoleteState = Removed guidance with the two-stage workflow (Pending
first, Removed later); reference use-obsolete-pending-before-removed.
- upgrade/use-datatransfer-for-large-dataset-initialization: Add the
>300,000 records threshold as the concrete trigger for requiring
DataTransfer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 35 articles still in their seed form previously carried a banner
reading "Seed article. ... Domain stewards should expand, restructure,
and refine as needed." For a community preview, that phrasing reads as
"TODO left in production" to first-time visitors.
Replace all three banner variants (performance-seeded, security-seeded,
community-ported) with a single positive invitation:
> Contributions welcome — open a PR to refine or extend this article.
Content and structure of the articles are unchanged; only the leading
quote block differs. Articles that had their banner fully stripped in
the earlier triage pass (the showcase-grade ten) are unaffected.
The knowledge corpus now covers performance, security, privacy, upgrade,
style, and UI. Previously only two leaf reviewer skills existed
(al-performance-review, al-security-review), so four of the six domains
had knowledge with no skill sourcing from them. A community reader
landing in privacy/, upgrade/, style/, or ui/ would see articles with
no apparent consumer.
Three changes:
1. Move existing review skills into `microsoft/skills/review/`. The
`review/` subfolder groups all review-kind skills together and leaves
room for future non-review action skills at the `microsoft/skills/`
level. Updates references in README.md, agent-consumption.md, and
skills/entry.md to the new paths.
2. Add four new leaf reviewer skills — al-privacy-review,
al-upgrade-review, al-style-review, al-ui-review — each following
the same DO template as al-performance-review/al-security-review but
sourcing from the corresponding knowledge domain. al-upgrade-review
and al-ui-review return `not-applicable` when the diff contains no
upgrade surface or no page files, respectively.
3. Update al-code-review to compose all six leaf skills and retarget
the dangling references in every populated JSON example
(`use-setloadfields.md`, `no-plaintext-secrets-in-telemetry.md`,
`avoid-implicit-commit.md` — none of which exist in the corpus) to
real knowledge files: `call-setloadfields-before-filters.md`,
`use-secrettext-for-credentials.md`, `never-hardcode-secrets-in-al.md`.
Validator passes with 0 errors / 0 warnings.
Adds 55 articles (plus 76 code samples) spanning four new domains and
two existing domains, extracted from the internal Business Central
review-agent prompt. Content was filtered against BCQuality's
remedial-knowledge premise: each article encodes BC-specific behaviour,
a CodeCop rule, a platform API semantic, or an anti-false-positive
guideline that a capable LLM would otherwise get wrong.
New domains:
- privacy (11 articles): DataClassification inheritance semantics, the
StrSubstNo-defeats-Error-telemetry-classification pitfall, Privacy
Notice consent for outgoing requests, anti-false-positives for pages
and in-memory data.
- upgrade (11 articles): upgrade-codeunit structure, upgrade-tag
lifecycle and registration, protected DB reads, DataTransfer for
large datasets, InitValue semantics, enum-ordinal preservation,
obsolete-workflow, first-install detection.
- ui (9 articles): caption capitalization by phrase type, tooltip voice,
teaching-tip vs tooltip, tour-tip conventions, character limits,
banned terms, ampersand handling, title punctuation.
- style (11 articles): label-suffix convention, API page naming,
temporary-variable prefix, label properties (Comment/Locked), named
invocations, FieldCaption in user messages, OptionCaption pairing,
Error-parameter passing, `this` keyword, required parentheses, file
naming.
Gaps in existing domains:
- performance (11 articles): production-scale table catalog (no row
counts, per internal-data concern), anti-false-positive for bounded
tables, guard-before-Get ordering, redundant-Get-in-OnAfterGetRecord,
LockTable in read-only helpers, combined ModifyAll passes, writes in
OnAfterGetRecord, SetLoadFields heuristics, temporary-table
regressions, FlowField source-table widening, MaintainSQLIndex
disabling SIFT.
- security (2 articles): environment-specific hardcoded GUIDs,
ValidateTableRelation=false on user input.
Intentionally excluded: specific production P95 row-count numbers
(aggregated internal telemetry); rewritten as categorical guidance on
which tables to treat as production-scale without publishing sizes.
All articles use `bc-version: [all]` (applies to every BC version, per
the new schema sentinel). Validator passes with 0 errors / 0 warnings.
Most of the corpus — FindSet/SetLoadFields/CalcFields patterns, permission
sets, SingleInstance codeunits, DataClassification, IsolatedStorage,
transaction scope, SecretText — describes BC platform behaviour that is
identical across supported versions. The seed [26..28] range on every
file implied a version-specificity the content does not actually have,
and there was no way to express "applies to every version" in the
schema the way [w1] and [all] already do for countries and
application-area.
Extend the v1 schema with a universal sentinel for bc-version, parallel
to the sentinels already defined for the other dimensions:
bc-version: [all] # applies to every BC version
[all] is mutually exclusive with explicit versions. Range shorthand
([26..28]) and explicit lists ([26, 27, 28]) continue to work for files
genuinely tied to a version-gated API or deprecation.
Update read.md (field definition, matching semantics, partial-context
rule), write.md (default to [all], use ranges only with a concrete
reason), README.md (frontmatter example), and the CI validator. All
forty existing knowledge files and the three action skills convert to
[all]; none of the current content is version-gated. Validator passes.
Remove seven knowledge files whose content is generic software-engineering
guidance that a capable LLM already applies without BCQuality present
(HTTPS-only, secret-leakage-in-errors, no-credentials-in-URLs, silent
security-error swallowing, short transaction scope, HTTP timeouts,
StrSubstNo-vs-concatenation). These fail the remedial-knowledge premise
and dilute the signal of the preview corpus.
Strip the "Seed article — domain stewards should expand" banner from ten
files that are ready to showcase (AA0232/AA0233 rules, FindSet read-only
semantics, SetLoadFields ordering and usage, CalcFields-in-loops,
SecretText end-to-end, DataClassification). The banner remains on files
that still need domain-steward refinement.
Add a "What belongs here" section to the README stating the admission
test: a file exists only if a modern LLM would get something wrong or
miss something without it. Gives contributors a concrete yes/no filter
before they open a PR.
- Introduced unit tests for the bc-domain-context implementation, covering various scenarios including filtering by application area, technology mismatches, layer precedence, and conditional applicability.
- Added knowledge files related to finance, including topics such as Chart of Accounts, Codeunit 12, Dimension Management, and VAT on prepayment chains, among others.
- Each knowledge file includes structured metadata and best practices to enhance the domain knowledge available for Business Central tasks.
Ports 14 concern-sized articles (8 performance, 6 security) and 25
AL samples from BC Code Intelligence, restructured to BCQuality's v1
schema and layered under /community/knowledge/. Each article is
atomic, under 100 lines, and ships <slug>.good.al and (where the
pattern has a clear anti-example) <slug>.bad.al siblings.
Jesper's microsoft-layer leaves (al-performance-review and
al-security-review) source across every enabled layer via
*/knowledge/<domain>/**, so these additions are picked up by the
existing action skills without any new skill definitions.
Performance (8):
- use-deleteall-for-filtered-bulk-deletion
- call-setloadfields-before-filters
- load-common-fields-before-branching-on-case
- load-only-primary-key-fields-for-reference-work
- omit-filter-only-fields-from-setloadfields
- choose-maintainsiftindex-by-read-write-ratio
- avoid-growing-globals-in-singleinstance-subscribers
- order-case-branches-by-frequency
Security (6):
- classify-every-field-with-dataclassification
- protect-sensitive-data-in-temporary-tables
- guard-bulk-operations-with-istemporary
- compose-permission-sets-with-included-sets
- do-not-grant-rights-beyond-a-users-entitlement
- prefer-oauth2-over-api-keys-for-external-http-calls
Graveyard-bound items (not ported; to be captured in a later
/docs/triage-graveyard.md):
- testfield-performance (soft guidance, low actionability)
- table-event-batch-operation-impact (keep-event-subscribers-lightweight
already carries the core insight)
- Most of /roger-reviewer (AL formatting - frontier-model territory)
- sift-technology-fundamentals (descriptive, not a citable concern)
- bc-telemetry-buddy-* (tooling promotion, not guidance)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Python validator derived from READ, WRITE, DO, and Entry. Enforces
frontmatter shape, required sections, knowledge-file length and
no-code-blocks rule, sample-sibling naming (<slug>.good.al /
<slug>.bad.al), action-skill section ordering, and unique skill ids
per kind. Runs in GitHub Actions on PRs and pushes to main; emits
GitHub annotations when GITHUB_ACTIONS is set, plain text otherwise.
Warnings do not fail the build.
Passes cleanly against the existing microsoft-layer corpus.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a new skill kind, 'entry-point', and its sole instance at
skills/entry.md. When an orchestrator points an agent at BCQuality,
the agent's first call is Entry: it receives a task context and
returns a dispatch record naming the action skill(s) to invoke.
Routing logic lives in Entry, not in the orchestrator.
Entry structurally follows DO's Source -> Relevance -> Worklist ->
Action pattern but the units it selects are action skills (not
knowledge files) and its output is a dispatch record (not a
findings-report).
Contract highlights:
- Inputs semantics in DO clarified as any-of: orchestrator supplies
whichever listed input types it has; skill must return
'not-applicable' if the subset is insufficient. This matches
the existing al-* canonical skills which declare
[pr-diff, file-path] as alternatives.
- Relevance admits candidates whose inputs intersect
inputs-available, not whose inputs are a subset.
- Dispatched inputs are the intersection, not the full
inputs-available set, to avoid leakage between skills.
- Super-skill precedence in Worklist supersedes a sub-skill only
when the goal is a broader match for the super than the sub.
When the goal specifically names a concern the sub handles
(e.g., 'performance review'), the sub wins and the super is
dropped with reason 'narrower-sub-skill-selected'.
- Skill layer precedence is defined here as custom > community >
microsoft, matching READ's rule for knowledge files.
- skipped[] carries 'superseded-by' for layer-precedence,
sub-skill, and super-skill drops, for traceability.
Propagate the concept through:
- skills/README.md: distinguish the runtime entry-point skill from
the three meta-skill contracts.
- skills/do.md: acknowledge entry-point alongside meta-skills as
the only kinds that live outside a layer.
- README.md: rewrite the Skills and Agent bootstrapping sections
so the bootstrap instruction is 'invoke /skills/entry.md first'.
- agent-consumption.md: update the Mermaid flow and step narrative
to show Entry dispatch, with READ and DO read on demand.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Anyone (human or agent) browsing to /skills/ on GitHub previously saw a
bare directory listing. This file names the three meta-skills, states
the reading order, and describes who each is for. Replaces the
placeholder .gitkeep.
No contract changes -- this is a navigation aid.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sample code in knowledge articles is demonstration-only and never
imported into a BC app, so object-ID uniqueness or ranges are not a
property the READ contract needs to enforce. The Sample files section
now just says samples are self-contained and demonstration-only, with
no guidance about IDs.
Existing samples keep whatever IDs they happen to have; future authors
are free to use anything readable.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The /samples/ top-level tree is replaced with sibling files in each
knowledge-layer folder. An article and its demonstrations now live
side-by-side:
microsoft/knowledge/<domain>/<slug>.md
microsoft/knowledge/<domain>/<slug>.good.al
microsoft/knowledge/<domain>/<slug>.bad.al
Rationale:
- Proximity. An article and its paired samples are one unit; the
filesystem now reflects that.
- Layer ownership. Samples inherit layer precedence for free -- a
/custom/ fork can override an article and its samples atomically,
which the shared /samples/ tree previously made awkward.
- Trivial migration path. Action-skill source globs
(*/knowledge/<domain>/**/*.md) are unchanged; sample discovery is a
sibling-filename lookup.
Changes:
- git mv of all 65 sample files from samples/<domain>/<slug>/{bad,good}.al
to microsoft/knowledge/<domain>/<slug>.{bad,good}.al (history preserved).
- Update See-sample references in all 37 articles that ship samples.
- skills/read.md: replace the no-code-blocks bullet with a pointer to a
new Sample files section that fully specifies the sibling convention,
the kinds (good/bad + forward-compatible), multi-technology rules,
demonstration-only status, and layer-precedence behaviour.
- skills/write.md: update the samples pointer to match.
- README.md: annotate the knowledge tree with the sample sibling shape.
- samples/README.md deleted; content lifted into skills/read.md.
- Both generators (C:\temp\gen_performance_knowledge.py,
C:\temp\gen_security_knowledge.py) updated to emit at the new paths
and to stop writing samples/README.md. Re-running them is idempotent
against the committed layout.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Converts Jesper's existing AL security-review prompt into BCQuality seed
knowledge articles so the al-security-review leaf has a real corpus to
match against. Mirrors the performance seed phase.
Articles under microsoft/knowledge/security/ (16):
- Permission model: follow-least-privilege-in-permission-sets,
use-indirect-permissions-for-elevated-access,
use-inherent-permissions-to-grant-minimal-access
- Secrets: never-hardcode-secrets-in-al,
use-isolated-storage-for-module-and-company-secrets,
prefer-azure-key-vault-for-production-secrets,
use-secrettext-for-credentials, use-secrettext-with-httpclient,
compose-secrets-with-secretstrsubstno,
use-nondebuggable-when-parsing-secrets
- External calls: require-https-for-external-calls,
set-timeouts-for-external-calls, do-not-put-credentials-in-urls
- Error handling: avoid-sensitive-data-in-error-messages,
do-not-swallow-security-errors-silently
- Extensibility: do-not-expose-sensitive-data-in-event-publishers
Paired AL samples under samples/security/<slug>/{bad,good}.al, object
IDs 50200-50231 (no overlap with performance 50100-50140).
Rubber-duck findings addressed:
- HttpClient secret-URI: SetSecretRequestUri is on HttpRequestMessage
(not HttpClient). Rewrote use-secrettext-with-httpclient and its
good sample to use HttpRequestMessage + HttpClient.Send.
- InherentPermissions only grants access to same-extension objects;
the sample now defines its own table 50230 "Sec Sample Lookup" and
grants 'r' on that, not on Database::Customer.
- Reworked compose-secrets-with-secretstrsubstno bad.al away from
Format(SecretText) (unreliable) to a plain Text+StrSubstNo anti-
pattern.
- Moved normative guidance out of Description in three articles
(compose-secrets-..., prefer-azure-key-vault-..., use-inherent-...)
so it sits in Best Practice / Anti Pattern per READ contract.
- Added a companion helper codeunit (50231) to the indirect-permissions
good sample so it actually demonstrates the controlled write path.
- Rebuilt the event-publisher good/bad pair on the same ExportCustomer
scenario so the contrast is the shape of the event signature, not a
different event.
Also: broaden samples/README.md object-ID range note to 50100-50299.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Converts an existing performance-review prompt into 22 atomic
knowledge articles under microsoft/knowledge/performance/, each
paired with AL samples under samples/performance/<slug>/ demonstrating
the anti-pattern and/or the best practice. The full set seeds the
corpus the microsoft/skills/al-performance-review leaf skill matches
against and validates the READ knowledge-file format end-to-end.
Every article conforms to the READ contract: six required frontmatter
fields, Description always present, no fenced code blocks, sample
code referenced by repo-relative path. Each article is marked with a
blockquote 'Seed article' note so domain stewards can extend or
restructure them freely.
Articles (ordered by concern area):
Database query efficiency
- use-findset-with-next (AA0181)
- avoid-findfirst-with-next (AA0233)
- only-fetch-records-you-use (AA0175)
- use-findset-readonly-by-default
- use-setloadfields-for-partial-records
- use-addloadfields-in-report-layouts
- use-calcsums-to-aggregate-filtered-sets (file: use-calcsums-for-flowfield-totals.md)
- avoid-calcfields-in-loops
- add-sift-keys-for-flowfields (AA0232)
- use-isempty-for-existence-checks
Filter and key optimization
- filter-before-find
- set-current-key-to-match-filters
Temporary tables and transactions
- use-temporary-tables-for-intermediate-data
- keep-transaction-scope-short
- avoid-user-interaction-in-transactions
- avoid-commit-inside-loops
Record operations
- prefer-get-for-primary-key-lookups
- use-insert-false-when-skipping-triggers
- prefer-direct-record-over-recordref
Strings, codeunits, events
- use-strsubstno-for-message-formatting
- use-single-instance-codeunits-for-caching
- keep-event-subscribers-lightweight
samples/README.md documents the sample-folder convention and makes
clear the samples are demonstration-only, not derived from BC base
application source, with unique object IDs in the 50100-50199 range.
Rubber-duck pass caught: a misleading good.al in avoid-calcfields-in-loops
(fixed by switching to a hoistable CalcFields scenario), an invalid
event subscriber signature in keep-event-subscribers-lightweight
(fixed by adding var xRec), normative guidance leaked into the
Description of use-findset-readonly-by-default (moved to Anti Pattern),
a missing sample pair for keep-transaction-scope-short (added), and
muddy FlowField/CalcSums framing (retitled and clarified).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DO contract (skills/do.md)
- New 'sub-skills' optional frontmatter field on action skills: when
present and non-empty, the skill is a super-skill that composes
other action skills.
- New 'Composition (super-skills)' section covering section
interpretation, outcome rollup, summary aggregation, and suppression
scope.
- Output schema gains three optional fields: 'from-sub-skill' on each
finding, top-level 'sub-results[]' carrying nested findings-reports,
and top-level 'skipped-sub-skills[]'.
- Super-skills MUST NOT filter sub-skills by task content; leaves own
task-level applicability and signal via outcome.
- Findings from a failed sub-skill MUST NOT flow into the parent's
findings[] or counts, consistent with DO's rule that consumers
ignore a failed skill's findings. Reports are still preserved in
sub-results[] for traceability.
- Rolled-up non-citation finding ids MUST be prefixed with the sub-
skill id to prevent collisions across sub-skills. Citation-based
ids are already unique via repo path and are not rewritten.
- Outcome rollup rules updated: 'partial' covers S = {partial},
{partial, partial}, and {partial, failed}. Empty worklist rolls up
to 'not-applicable' with outcome-reason.
- Nested super-skills are not permitted in v1.
Reference skills (microsoft/skills/)
- al-code-review.md rewritten as the canonical super-skill: lists
al-performance-review and al-security-review as sub-skills, orch-
estrates invocation, aggregates output, and includes a worked
rolled-up JSON example plus the empty-corpus rollup.
- al-performance-review.md added as a leaf reference skill for the
performance knowledge domain.
- al-security-review.md added as a leaf reference skill for the
security knowledge domain.
- Both leaves retain the leaf-level rules validated in the prior
pass: partial-context message requirement, worklist-scoped
suppression, application-area semantics, and the platform-guarantee
threshold for blocker severity.
README updated to describe leaf vs super-skill and link all three
reference skills.
Two rubber-duck passes tightened the contract and caught schema
violations in the worked examples before commit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
