Validate composed reviews against the expected leaf worklist - #204
Conversation
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed 11fbd227f4a90e962e3370e60753a71a76b6fc69. Three contract gaps remain:
- The trusted composition artifact contains expected identities but no host-owned accepted leaf outputs. The validator trusts nested
sub-results, so a composer can fabricate a schema-valid missing leaf (not-applicable,no-knowledge, or evencompleted) and still produce a completed report. Bind every nested leaf to a host-captured accepted result (for example a canonical hash or trusted report path), and add negative tests for fabricated/altered leaves. - Missing selected leaf IDs are not required in
outcome-reason; a generic reason such asBudget expiredpasses despite the contract requiring every unfinished leaf to be named. Validate the reason against the complete missing-ID list and add a regression test. - Incomplete composition still accepts top-level
from-sub-skill: "agent"findings even though super-skill self-review is forbidden until selected leaves finish. Reject Agent findings whenever expected selected leaves are missing, with partial and failed regression cases.
These gaps preserve success-shaped undercoverage or forbidden findings, so the validator is not yet safe to merge.
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed aecd081778e911b4e24ef46e5ea4e72c9facbc89 against the previous reviewed head. The three prior gaps are fixed: fabricated leaves lacking trusted captures are rejected, unfinished leaf IDs must be named, and incomplete compositions cannot contain Agent self-review findings. The existing acceptance suite and adversarial outcome matrix pass.
One exact-return binding defect remains at tools/Validate-FindingsReport.ps1:176: PowerShell -ceq is case-sensitive but culture-sensitive, not ordinal, and treats NUL/soft-hyphen/several zero-width characters as ignorable. A host-captured accepted correction exit(1); can be changed in both the nested and rolled-up report to ex\u0000it(1);, yet the completed composition is accepted with normalized=False and returns the altered literal correction. The immutable capture remains unchanged. This violates the core guarantee that captured leaf corrections are preserved exactly.
Please compare string values ordinally and add end-to-end regressions that alter captured corrections with ignorable characters. Also preserve JSON string types during parsing: the ConvertFrom-Json calls at lines 32/117 coerce timestamp-shaped strings to DateTime, and an altered timestamp-shaped outcome-reason was likewise accepted by the content comparison. Exact-content comparison must preserve literal strings, not only their culture/date interpretation. No broader review expansion is needed.
The four substantive GitHub validation workflows remain action_required and should be allowed to run before merge.
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed 5d1399f9fb2287100d8bd092c256c44dc7456777 against aecd081778e911b4e24ef46e5ea4e72c9facbc89, limited to the promised final exact-return blocker and its coupled paths. The fix is sound: report/capture/composition parsing and bounded-normalization cloning preserve timestamp-shaped text with -DateKind String; literal corrections/messages are compared ordinally; and the PowerShell 7.5 requirement is explicit and documented. End-to-end regressions reject altered captured and rolled-up corrections while preserving immutable captures, reject alternate timestamp spellings, and preserve literal text during permitted normalization. The seven previously demonstrated ignorable-character probes are rejected, identical text remains accepted, and contract/fixture/skill-schema integration passes. No remaining merge-critical issue found in this rereview. The four substantive GitHub workflows remain awaiting approval and should run before merge.
206c5fe
into
microsoft:main
Why
A composed report can be internally consistent without covering the review selected by the host. Validating only returned
sub-resultscannot establish that a selected domain was not omitted. The previous executable success fixture even accepted two reports from the same leaf producer.Existing guidance already requires discrete, ordered execution of selected leaves. This change adds deterministic acceptance enforcement rather than a new review or scheduling policy. It also reconciles the coordinator's budget-exhaustion guidance with outcome derivation: completed returned leaves do not make an unfinished composition
completed.Changes
-ExpectedCompositionPathto the findings-report validator. The private, host-owned artifact is preserved before dispatch, after layer resolution and input-compatibility checks; it is never derived from model output.partialif a non-failed report is available, otherwisefailed. Do not fabricate leaf results or turn budget exhaustion into a configuration/input-compatibility skip.Regression evidence
The executable acceptance matrix covers selected
[A, B]with only[A]returned (completedrejected, honestpartialaccepted), duplicate producers, unexpected IDs, wrong outer/leaf versions, reordered results, fabricated/missing/conflicting skips, no returned results, all-failed results, mixed success/failure, and all-skipped compositions.Integration tests use the existing index builder and resolver with a temporary Custom override, fallback to Microsoft when that override is disabled, and configuration exclusion when all implementations of the slot are disabled. Existing normalization, reference-integrity and rollup checks remain in place.
Compatibility And Limits
The findings-report JSON schema and fields are unchanged. Manifest-free callers remain supported, but cannot certify composition completeness, selected versions/order or legitimate exclusions. Previously accepted duplicate producers are rejected as violations of the discrete-leaf contract.
Immutable raw Task payloads and the existing bounded-normalization policy are preserved. Optional path/layer metadata stays private; reports expose ID/version, not implementation-path provenance. This is deterministic contract hardening, not a claim of a production incident or of semantic model correctness.
No schema or skill frontmatter version changes are proposed. As this clarifies stable protocol semantics, review by both maintainers is requested under the contributing policy.
Validation
Passed locally:
tools/Test-ReviewContract.ps1 -Root ..github/scripts/Test-SkillIndex.ps1 -Root .(all 19 review leaves preserved in order)tools/Test-ReviewFixtures.ps1 -Root .(216 cases; 108/330 paired articles across 20 domains)git diff --checkEditor diagnostics found no errors in the five touched files. Python/PyYAML frontmatter validation was not run locally because no usable Python interpreter was available; it remains for CI. No real-model evaluation or AL runtime tests are claimed.
Knowledge additions, model evaluation changes, index-schema work and runner redesign are intentionally outside this PR.