Summary
A code review of Cratis.Arc.Screenplay (as merged in PR #2384) surfaced a set of smaller, high-confidence defects and gaps. Filing them together since each is a few lines; happy to split any of them out.
Findings
-
MarkAsPii does not unwrap collection elements the way Resolve does (Analysis/Types/TypeRegistry.cs). Resolve re-unwraps a collection's element type; MarkAsPii does not, so a [PII] IEnumerable<Name?>-shaped member registers PII under Nullable rather than under the concept. Share the unwrap path between the two.
-
Commented-out imports still produce screen-binding candidates (Analysis/Screens/ScreenImports.cs). StatementRegex anchors on ^[ \t]*import, so // import { AllAuthors } from './Listing'; matches, and when it names a query the slice declares it becomes a false data binding. Strip // and /* */ regions before matching.
-
Query parameter filtering excludes only interface-typed parameters (Analysis/Queries/QueryReader.cs, IsInput). Struct infrastructure parameters (CancellationToken, paging/sorting value types) would be emitted as caller input with an undeclared type name. Filter the known infrastructure types explicitly.
-
SP0029 is skipped in ScreenplayDiagnosticCodes with nothing documenting why (SP0028 jumps to SP0030). If it was retired pre-merge, reserve it with a comment so it is not reused with a different meaning later.
-
Unicode normalization order in ScreenplayNaming.Sanitize (Emission/Naming/ScreenplayNaming.cs): characters are stripped before normalizing to Form C, so a decomposed é (e + combining accent) loses its accent while a precomposed é survives — canonically equal source produces different documents. Normalize first, then strip.
-
No spec covers optional/nullable emission end-to-end. TypeRegistry.Unwrap distinguishes Nullable<T> and annotated reference types, but no spec asserts that a nullable property comes out as Type? in the printed document. (Observed to work in practice — Ada emits groupKey InvoiceGroupKey? — but it is unspecified.)
-
ScreenplayOptions.WithDefaults is resolved twice (ScreenplayGenerator.Generate and again in ApplicationSyntaxBuilder.Build). Harmless under the generator, but a standalone Emit call behaves subtly differently; one resolution point would be cleaner.
-
Mutation-testing follow-up: the PR self-reports 145 mutants / 40 surviving (37%), clustered in ordering and first-wins decisions, and two of the nine defects found adversarially were invisible to the hermetic specs by construction. A CI gate that generates from a real sample app (e.g. Samples/Library or TestApps/AspNetCore) and compiles the output with the official Screenplay tool would catch exactly that class of regression. This was recommended in the PR but not adopted.
Environment
Summary
A code review of
Cratis.Arc.Screenplay(as merged in PR #2384) surfaced a set of smaller, high-confidence defects and gaps. Filing them together since each is a few lines; happy to split any of them out.Findings
MarkAsPiidoes not unwrap collection elements the wayResolvedoes (Analysis/Types/TypeRegistry.cs).Resolvere-unwraps a collection's element type;MarkAsPiidoes not, so a[PII] IEnumerable<Name?>-shaped member registers PII underNullablerather than under the concept. Share the unwrap path between the two.Commented-out imports still produce screen-binding candidates (
Analysis/Screens/ScreenImports.cs).StatementRegexanchors on^[ \t]*import, so// import { AllAuthors } from './Listing';matches, and when it names a query the slice declares it becomes a falsedatabinding. Strip//and/* */regions before matching.Query parameter filtering excludes only interface-typed parameters (
Analysis/Queries/QueryReader.cs,IsInput). Struct infrastructure parameters (CancellationToken, paging/sorting value types) would be emitted as caller input with an undeclared type name. Filter the known infrastructure types explicitly.SP0029is skipped inScreenplayDiagnosticCodeswith nothing documenting why (SP0028 jumps to SP0030). If it was retired pre-merge, reserve it with a comment so it is not reused with a different meaning later.Unicode normalization order in
ScreenplayNaming.Sanitize(Emission/Naming/ScreenplayNaming.cs): characters are stripped before normalizing to Form C, so a decomposedé(e + combining accent) loses its accent while a precomposedésurvives — canonically equal source produces different documents. Normalize first, then strip.No spec covers optional/nullable emission end-to-end.
TypeRegistry.UnwrapdistinguishesNullable<T>and annotated reference types, but no spec asserts that a nullable property comes out asType?in the printed document. (Observed to work in practice — Ada emitsgroupKey InvoiceGroupKey?— but it is unspecified.)ScreenplayOptions.WithDefaultsis resolved twice (ScreenplayGenerator.Generateand again inApplicationSyntaxBuilder.Build). Harmless under the generator, but a standaloneEmitcall behaves subtly differently; one resolution point would be cleaner.Mutation-testing follow-up: the PR self-reports 145 mutants / 40 surviving (37%), clustered in ordering and first-wins decisions, and two of the nine defects found adversarially were invisible to the hermetic specs by construction. A CI gate that generates from a real sample app (e.g.
Samples/LibraryorTestApps/AspNetCore) and compiles the output with the official Screenplay tool would catch exactly that class of regression. This was recommended in the PR but not adopted.Environment
Cratis.Arc.Screenplay20.65.0 (PR Add Cratis.Arc.Screenplay package for generating Screenplay from Arc source #2384)