Strengthen review contracts and add AL reliability guidance

[Stefano Demiliani (demiliani)]

Summary

This PR combines two related workstreams developed and reviewed separately on the fork: framework hardening for deterministic review execution, and new source-backed AL reliability rules with paired evaluation fixtures.

Workstream 1: review framework hardening and outbound HTTP guidance

• Adds two outbound HttpClient rules:
  • distinguish platform/transport failure from an HTTP response before accessing response data;
  • require a successful or explicitly expected HTTP status before consuming a response body as a success payload.
• Adds good/bad AL sample pairs and routes both rules through the web-services review skill.
• Makes layered action-skill overrides coherent with the documented custom > community > microsoft precedence:
  • same IDs are allowed across layers but rejected within one layer;
  • super-skill leaf slots resolve deterministically by ID;
  • disabling a higher-layer implementation falls back to the next enabled layer while preserving declared order.
• Adds Resolve-SkillWorklist.ps1 and focused precedence, fallback, and ambiguity tests.
• Adds Validate-FindingsReport.ps1 to enforce the findings-report schema and semantic acceptance rules, including counts, coverage, references, retrieved article membership, source locations, agent-finding limits, and bounded range normalization.
• Makes evaluation coverage measurable and requires changed paired articles to be selected or explicitly waived.
• Updates the composition, standalone runner, customization, evaluation, and CI documentation/contracts.

Workstream 2: AL data handling and test isolation guidance

Adds five source-backed rules, each with focused good/bad AL samples, routing keywords/checks, and deterministic evaluation coverage:

• derive base quantities through the line unit-of-measure validation path instead of assigning base fields directly;
• do not concatenate external text into SetFilter; pass it through filter placeholders;
• reset per-test state before an IsInitialized guard to preserve test isolation;
• check JSON values for null before converting them to scalar AL values;
• use standard format 9 for locale-independent values exchanged with external systems.

Validation

• Test-SkillIndex.ps1: passed, including layered override resolution.
• Test-ReviewContract.ps1: passed, including executable findings-report acceptance cases.
• Test-ReviewFixtures.ps1: passed with 122 cases covering 61 selected paired articles across 20 leaf domains; every changed paired article is selected.
• Knowledge retrieval validation passed from a non-linked temporary checkout: 335 articles and 561 samples round-tripped.
• Editor diagnostics: clean.
• git diff --check: clean.

[Jesper Schulz-Wedde (JesperSchulz)]

One merge-critical contract gap remains in tools/Validate-FindingsReport.ps1. The validator only rejects items-evaluated > worklist-size and recursively validates sub-results. It does not enforce completed/partial coverage consistency, leaf-versus-super-skill constraints, derived super-skill outcomes, or aggregate coverage rollups.

As written, a report with outcome: completed, worklist-size: 7, and items-evaluated: 0 passes, as can a completed super-skill whose sub-results all failed. This contradicts the mandatory acceptance semantics in skills/do.md and makes the advertised deterministic gate success-shaped for incomplete reviews.

Please enforce the full leaf/super invariants and add negative regression cases for completed undercoverage, leaf sub-results, incorrect derived outcomes, and incorrect rollups.

[Jesper Schulz-Wedde (JesperSchulz)]

The original coverage, leaf/super composition, derived-outcome, and coverage-rollup gaps are fixed. One merge-critical rollup invariant remains: a super report can silently omit findings emitted by a completed/non-failed leaf and still pass validation.

The validator requires from-sub-skill only for top-level findings, but never reconciles top-level findings with non-failed sub-results or verifies that the claimed producer is an invoked, non-failed leaf. A completed leaf containing a unique blocker can therefore roll up to findings: [] and zero counts while the super report remains accepted, contradicting the mandatory finding-preservation/deduplication contract in skills/do.md.

Please validate leaf-to-top finding rollup while accounting for documented deduplication, reject nonexistent/failed producer attribution, and add negative tests for an omitted non-failed leaf finding and failed-leaf finding leakage.

[Jesper Schulz-Wedde (JesperSchulz)]

The new rollup validation addresses omission and producer attribution in the simple case, but three merge-critical holes remain:

1. Representation is tracked only by finding ID. Two distinct violations of the same rule at different locations share the same citation ID; retaining one marks the ID valid and silently permits omission of the other. Track distinct finding identity/coverage, not only ID, and add a regression with two unique same-ID findings where one is omitted.
2. Exact one-leaf equality rejects the documented coordinator deduplication contract. al-code-review.md permits merging overlapping findings with different knowledge IDs, appending supporting references, choosing the specific owner, and retaining the highest justified severity/confidence. Validate that every eligible leaf finding is represented by a permitted merged finding rather than requiring exact references/location/message equality to one leaf. Add a positive regression for the documented A+B -> A merged form.
3. from-sub-skill: agent skips producer/leakage checks, so a citation-backed failed-leaf finding can be relabeled as agent and pass. Enforce the agent-finding contract here (references: [], agent: ID, Agent domain and caps) and add a failed-leaf leakage regression using the agent label.

[Jesper Schulz-Wedde (JesperSchulz)]

The new commit fixes explicit-location multiplicity and closes the from-sub-skill: agent bypass. Two merge-critical deduplication gaps remain:

1. Cross-rule merges are recognized only when suggested-code is exactly equal, or—when absent—when message is exactly equal. The coordinator contract permits merging findings that prescribe materially the same correction while retaining one self-contained message; messages need not be identical. Add support for the documented A+B -> A merge without exact text equality, plus a positive regression with different messages and no suggested code.
2. Test-LocationsOverlap treats two missing locations as overlapping. Because location is optional, one top-level finding can then represent multiple locationless same-ID leaf occurrences, losing multiplicity. Missing locations do not establish the same file/overlapping range required for deduplication; use multiplicity-aware matching unless overlap is proven, and add a locationless same-ID omission regression.

[Jesper Schulz-Wedde (JesperSchulz)]

The locationless multiplicity fix and different-message/no-suggested-code merge case are now correct. One merge-critical guardrail remains: the supporting-reference fallback permits a cross-rule merge whenever locations overlap and references are appended, even when both findings provide contradictory suggested-code.

That exceeds the documented “materially the same correction” contract. Please reject a reference-based merge when explicit correction evidence conflicts (at minimum, when both leaf and rolled findings have unequal suggested code), and add a negative regression showing overlapping A/B findings with different references and contradictory replacements cannot collapse into one.

[Jesper Schulz-Wedde (JesperSchulz)]

The new guard rejects rolled-versus-leaf suggested-code conflicts, but one merge-critical bypass remains: if the rolled finding omits suggested-code, two overlapping leaves with different explicit replacements are still accepted through the supporting-reference fallback.

For reference-based merges, compare explicit corrections across all represented leaf findings—not only each leaf against the rolled finding—and reject unequal suggested-code values even when the rollup omits the field. Please add the omitted-rollup variant as a negative regression while retaining the valid text-only and locationless cases.

[Jesper Schulz-Wedde (JesperSchulz)]

Thanks for working through the validator edge cases. The omitted-rollup correction conflict is the final known blocker from this review. Once that case is fixed with the requested regression, the next pass will be limited to verifying that exact guard and confirming no new merge-critical defect was introduced; no broader review expansion is planned.

[Jesper Schulz-Wedde (JesperSchulz)]

Approved. The final known blocker is resolved: merged findings now compare explicit suggested replacements across all represented leaves, including when the rolled finding omits suggested-code. The requested negative regression is present, and the valid text-only and locationless multiplicity cases remain intact.

[Wenjie Fan (gggdttt)]

Thanks for adding the outbound HTTP guidance. We found five concrete false-alarm examples that support the optional-return clarification in this PR. We had started a separate knowledge candidate, but have paused it to avoid duplicating your canonical article.

This is evidence and a focused routing follow-up, not a request to reopen the validator review or add a new merge blocker.

Five observed transport-failure misreadings

These come from BC-Bench run 35608133170 on September 21: Bench ce21d50a, engine 75d67cd, BCQuality b74967bc, CLI 1.0.83, Sol root / Luna domain leaves. gN below is the zero-based generated-comment index. The rows belong to successfully audited executions, although the full run was incomplete.

Finding / retained job	Bare call	Incorrect premise in the generated comment
privacy-008:g1	CustomerSyncDispatcher.Codeunit.al:10, Post	Ignoring the Boolean lets a failed request return without surfacing an error.
privacy-010:g1	ExternalCRMSync.Codeunit.al:27, Post	Transport failure can fall through to the later generic response-status error.
privacy-015:g2	AIContextBuilder.Codeunit.al:26, Post	The existing status check cannot expose transport failure unless the Boolean is captured.
privacy-015:g3	CustomerDataExporter.Codeunit.al:24, Post	A transport failure is treated as successful export.
security-clean-02:g0	PlainTokenHeaderClient.Codeunit.al:11, Get	A failed request can silently return because its Boolean is ignored.

The full case IDs have the synthetic__ prefix. The supplied source has no visible enclosing error-catching boundary. Both the official Post return contract and Get return contract state that unsuccessful execution raises a runtime error when the optional result is omitted. Your transport article's clarification addresses exactly this mistaken inference.

This does not establish that all five HTTP paths are otherwise correct. The CRM and AI examples already check HTTP status; the other three do not. A completed non-2xx response, a consumed false that is ignored, or an outer TryFunction failure converted into success remain separate concerns. The Get comment also includes status-handling advice; that does not make its incorrect transport premise true.

Discovery across the two domains

All five claims originated as reference-less agent findings in al-error-handling-review; root retained their original wording. At this PR's current head:

• Error Handling Source still requests only -Domain error-handling.
• Search-Knowledge filters domain equality, so that catalog does not include the new web-services guard.
• Root rollup does not repeat knowledge validation for leaf-level agent findings.

Could we coordinate a minimal finder/source route that lets the Error Handling leaf discover this existing HTTP owner guidance, while keeping one canonical article? That can be a separate follow-up rather than expanding this PR. We should avoid copying the AL fact into another article, the leaf instructions, or the engine.

Useful regression boundaries are: bare Get/Post with fail-fast propagation, consumed false followed by success-shaped processing, completed non-2xx responses, and actually consumed versus unconsumed TryFunction results. The guard should suppress only the first mistaken inference, not the genuine failures.

The domain-catalog boundary above is statically verified. It does not prove which articles the historical model read, and we have not run this PR's candidate or measured whether it removes the five findings. Original gold, scores and run artifacts remain unchanged.

[Wenjie Fan (gggdttt)]

demiliani#1
Stefano Demiliani (@demiliani) Here is the PR, if you feel it's okay we can merge it to current PR first, and then merge current PR to master
(We can also do it separately, but need your approval)

[Stefano Demiliani (demiliani)]

Wenjie Fan (@gggdttt) > demiliani#1 Stefano Demiliani (Stefano Demiliani (@demiliani)) Here is the PR, if you feel it's okay we can merge it to current PR first, and then merge current PR to master (We can also do it separately, but need your approval)

For me it's good to go. Should I approve the PR from my side?

[Jesper Schulz-Wedde (JesperSchulz)]

Wenjie Fan (Wenjie Fan (@gggdttt)) > demiliani#1 Stefano Demiliani (Stefano Demiliani (Stefano Demiliani (@demiliani))) Here is the PR, if you feel it's okay we can merge it to current PR first, and then merge current PR to master (We can also do it separately, but need your approval)

For me it's good to go. Should I approve the PR from my side?

Nope! I'll hit the merge button and it's another one for the books from you 🥳

[Wenjie Fan (gggdttt)]

Stefano Demiliani (@demiliani) Yes need your approval, then we can merge it to current PR

[Jesper Schulz-Wedde (JesperSchulz)]

Stefano Demiliani (@demiliani), turns out I have no powers over your fork 😉 You actually need to approve and merge it, and then I can merge this one.

[Stefano Demiliani (demiliani)]

Stefano Demiliani (Stefano Demiliani (@demiliani)), turns out I have no powers over your fork 😉 You actually need to approve and merge it, and then I can merge this one.

Jesper Schulz-Wedde (@JesperSchulz) PR #1 has been approved and merged.

[Jesper Schulz-Wedde (JesperSchulz)]

Reviewed the update at 13a3d50b803822bfdfe6601d6ee09792b76f3ce6 against the previously approved de62a818dff867ed7dd6198eb07cd0aced8f3d97. The new HTTP-error routing is consistent with the canonical web-services knowledge, and I found no regressions in the findings-report rollup contract or the newly added reliability guidance. The relevant retrieval, contract, fixture, index, and frontmatter validations pass.

[Jesper Schulz-Wedde (JesperSchulz)]

Reviewed merge head 008d458e75673c5aac9dc78f0b4484d2f0eaf99f. The conflict resolutions preserve the previously approved testing and web-services routing while incorporating current main; contract, fixture, skill/index, knowledge retrieval, frontmatter, and diff validations pass. No new merge-critical issue found.