Skip to content

Recover more of an application and report what cannot be recovered - #2408

Closed
woksin wants to merge 21 commits into
mainfrom
feat/screenplay-phase2
Closed

Recover more of an application and report what cannot be recovered#2408
woksin wants to merge 21 commits into
mainfrom
feat/screenplay-phase2

Conversation

@woksin

@woksin woksin commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Added

Changed

Fixed

woksin and others added 19 commits July 28, 2026 20:40
…angling

A reactor observing what a sibling bounded context publishes refers to an
event a package declares. The document had nothing to say about it beyond
SP0021, and then referred to a name it never introduced - which the official
Screenplay compiler reports as an unknown event.

Screenplay already has the construct for this: the compiler reads the last
segment of an 'import' as the name of an event that is known. Every undeclared
name is now looked for in the assemblies the compilation references, and the
one that is found is imported rather than reported. SP0021 is left for what
is really unresolvable - a name nothing at all declares an event under, where
importing it would state that a package declares something it does not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A compilation loaded from a project carries what every source generator
emitted to the intermediate folder. Those files declare real members of the
slice, so the folders they sit in were counted among the folders the slice
is written across - which reported a slice sitting in one folder as spread
over two, claimed the folder was shared with the next slice to be handed the
same generator output, and widened the screen scan to a build folder.

Generated source now answers neither where a slice is written nor where the
project is. Where a build wrote every last file the whole set is kept, since
answering with nothing writes every path against the machine that generated it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Kebab case is idiomatic for the runtime name of a Chronicle constraint and a
Screenplay identifier cannot carry a hyphen, so a transformation is forced.
Deleting the separators was the least readable one available - every one of
Ada's thirty one constraints came out as a run-together lower case word, and
the word boundaries the source stated were thrown away with them.

Each character an identifier cannot hold now ends a word instead, and the
words are joined in Pascal case: 'unique-timesheet-start' reads as
'UniqueTimesheetStart'. This is every identifier position rather than only a
constraint. A name carrying no separator is left exactly as it is, so nothing
the application already writes as one word changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolving a type strips the collection it sits in and then strips the
optionality of the element, while marking one as personal data stripped only
the collection. A collection of an optional value therefore registered the
mark under 'Nullable' - a name no concept is declared with - so the document
said the value was not sensitive while the runtime encrypted it.

Both now strip the same wrappers through one path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The statement pattern anchors on a line starting with 'import', which a
commented out one still does, so a component carrying the leftovers of a
change bound its screen to queries it never calls.

Comments are now removed before statements are read, and the line breaks a
block comment spanned are kept - joining the line after one to the line
before would hide the real import that follows it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only an interface parameter was read as a collaborator rather than as input,
which is not the whole of what a host hands a query. A cancellation token,
the page asked for and the order asked for are all filled in from the request
and none of them is an interface, so each was stated as caller input - a
parameter no caller sends, typed by a name the document never declares.

They are named explicitly, because there is no property of a type that tells
infrastructure from a value a caller really sends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sequence jumped from SP0028 to SP0030 with nothing saying why. A code is
what a consumer suppresses and groups on, so a number handed to something
else later would silently change what an existing suppression means. The gap
is now stated as deliberate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An accented letter can be written as one character or as a letter followed by
a combining mark, and Unicode calls the two canonically equal. A combining
mark is not a letter, so stripping first kept the accent of one spelling and
quietly removed it from the other - two documents for one application,
decided by how an editor happened to save the file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
C# says a value may be absent in two ways - a nullable annotation on a
reference type and a Nullable wrapper on a value type - and Screenplay says
it in one, a trailing question mark. Unwrapping distinguished the two all
along and nothing asserted that either survived as far as the printed text,
where getting it wrong describes a shape the application does not have or
names a type the document never declares.

The specification is end to end and covers a command property, an event
property, an enumeration, a collection of optional values and the parameter
of a query.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generator resolved them against the assembly it was reading and the
syntax builder resolved them again against the domain of the model, so one
generation ran two resolutions with two different fallbacks and let whichever
came last decide. They agree today, which is the only reason nothing showed.

The emitter is now the single place the emission half resolves, and the
builder is handed options that are already resolved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every Screenplay specification builds its compilation from source strings,
which is what makes them hermetic and what puts a class of defect out of
their reach by construction: a compilation built that way has no intermediate
folder, no source generator output on disk and no referenced package
declaring an event, so a generator that mis-attributes one of those produces
a wrong document that every specification still passes. Two shipped that way.

A console project reads a real project through MSBuild, generates from it and
reads the document back with the compiler the language ships. It has to come
back with nothing to say, warnings included - a warning there is the document
referring to something it never introduces, which is this generator being
wrong rather than the application.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Any compilation error at all made SP0024 an error saying "nothing recovered
from it describes the application reliably". That claim is false whenever
recovery succeeded, and it usually did: a host handing over a compilation
assembled without the compile items a build generates leaves every reference
to a generated type unresolved, and the errors then sit in code declaring no
artifact while every command, event and reactor is read exactly as written.
One real application produced 607 such errors alongside a complete and
accurate document, which the host then discarded on the error alone.

The severity now follows how many artifacts were recovered from a declaration
no compilation error sits inside. None - nothing recovered at all, or every
declaration something came out of being one the compiler could not make sense
of - stays an error, because no part of that document can be trusted. Any at
all is a warning stating how many errors there were, how many artifacts came
through anyway and what usually causes it, so the result is successful and
the document is kept.

A count is used rather than a proportion because any threshold would make the
same recovery pass for a large application and fail for a small one. Zero is
the only number that means recovery was prevented rather than dented.

Suppressing "declares nothing" is unchanged and, in the warning branch, can
never apply - a warning is only reached when something was recovered.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SP0024 suppressed the document check on the code alone, which was right while
it was always an error: a model recovered from symbols the compiler never
accepted describes an application that does not exist, so a document made from
it being poor is the broken build rather than a second defect.

Now that it can be a warning, that reasoning inverts. A warning says what was
recovered stands, and a document built from a model that stands is exactly
what the check exists for. Suppressing it there would hand back a document the
language rejects with nothing wrong reported and a successful result, which is
the one outcome the check was added to make impossible.

The suppression therefore follows the severity rather than the code.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generator takes a Compilation and nothing else, so assembling one the way
a real build would is the caller's job - and nothing said so. The part hosts
get wrong is source generators: no workspace loading mode runs them, and an
Arc application leans on generation heavily enough that a compilation loaded
without them is missing every type they emit.

Documents the contract, how to run the generators before handing the
compilation over, and what the generator does when a host does not - which is
now two different things depending on how much survived.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A message reaches a rule through a lambda wherever an application declares
its messages once and names them from every validator rather than repeating
the text. The lambda computes nothing there - it names a constant the
compiler already substituted - but only the direct form was read, so those
applications ended up with a document carrying no message on any rule at all.

The lambda that really does build its text while the request runs stays
reported, and the report now names the expression it could not write down
rather than only saying that there was one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A rule holding an end date to the start date it was sent with names that
start date with a lambda, which is the same shape a chain names the property
it is declared on. Only a constant was read, so every one of those rules
looked like a comparison given nothing to compare against and was dropped -
taking the layer a domain is really written in with it. A rule operand is a
host expression, so the path is written as one.

A rule held to a condition is stated as though it always holds, because a
rule carries no condition of its own yet (Cratis/Screenplay#32). The report
now carries the condition, so a reader can tell a rule that hardly ever
applies from one that nearly always does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A record is referred to by name and never declared, so nothing inside one was
ever named on its own and every concept reached only through a child record or
a read model stayed out of the document. Where such a concept is marked as
personal data, the document then understated what the application holds about
people - which is the opposite of what declaring concepts is for.

The walk is cycle safe and ordered by name, so a record referring to itself
ends and the same source always yields the same document. The shape of the
record itself still has nowhere to go (Cratis/Screenplay#29), so a property
carrying one now says so rather than leaving the reader to notice.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An import naming something a slice does not declare says nothing, which holds
for a component, a command and a package and was taken to hold for everything.
It does not hold for a query another slice declares - a screen aggregating
several read models is what an Event Modeling screen routinely is, and those
bindings were dropped without a word.

Writing them down is what the language cannot do: a binding names a query by
the bare name its own slice declares it under, and an application declares
All once per read model (Cratis/Screenplay#28). Where the import was written
does say which slice was meant, so the screen, the query and that slice are
reported instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Where components are written against a view model, the component imports the
view model and the view model imports the query - so reading the component
alone found the name of a file and nothing about what the screen reads, and
those screens ended up bound to nothing at all.

The view model is written down rather than guessed at: the component names
the module, it sits in the slice's own folder, and what it imports is read
exactly the way the component's own imports are. One hop and no further, and
only within that folder, because a chain across folders would be inferring an
architecture rather than reading one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@woksin woksin added the patch label Jul 28, 2026
@woksin

woksin commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Reviewer context — deliberately kept out of the description.

This is agent-authored code that no human has read yet. Green CI is not sign-off. Do not merge without explicit review. Stacked on #2407, which is stacked on #2406; it must merge last of the three.

Three of the issues' premises turned out to be wrong

This is the most important part of this PR. The acceptance criteria could not be met as written, because the evidence they rest on does not hold against the current source.

#2397 — the message recovery does not fire on the real application, and the 643 figure is not what it looks like. All 510 WithMessage calls there are the lambda form, but every one points at a resx-generated designer propertyinternal static string X => ResourceManager.GetString(…) — which is a runtime, culture-dependent lookup, not a constant. HasConstantValue cannot reach it, and "keep SP0016 for genuinely computed messages" applies. SP0016 therefore stays at 643. The 643 is also not all message drops: the real split is 268 RuleFor(c => c) whole-command rules, 247 messages, 59 Must, 28 NotEqual, 20 MustAsync, 12 When, 6 InclusiveBetween, 3 IsInEnum. Recovering those messages needs $strings. localization-key emission from the resx key — a different change, not attempted here. What did improve: those 247 now name the expression they could not write down, and the 12 When drops name their condition.

#2403 — 6 → 2, not 6 → 0. Only four of the six SP0011s were the property-reference shape the issue describes. The other two are .GreaterThan(businessClock.Today()) (a method call) and .GreaterThan(DateOnly.MinValue) (static readonly, not a constant); both correctly stay dropped.

#2400 — only 7 of the 15 named concepts are recoverable, and the three PII ones are not among them. FirstName, LastName, PhoneNumber, WebsiteUrl, MagicLinkRecipientId and the three Tripletex ids are not referenced by any artifact in the current tree. FirstName and LastName appear nowhere outside their own declaration files; PhoneNumber appears only in an interface method signature and its implementation; WebsiteUrl only in specs; the Tripletex ids only as handler-returned event source ids, already reported as SP0013. That application's PersonalDetails carries FullName/DisplayName/Bio, not the three names the issue's table lists. Declaring an unreferenced concept would contradict the registry's "declare only what is referenced" rule, so it was not done. This was independently re-checked against the source before writing this.

Personal data did improve where it was reachable: @pii went 27 → 29, with AnswerText and CriterionNote both emitting as String @pii with their validate blocks intact. Concepts overall went 105 → 130.

#2396 — the diagnostic works; the optional ViewModel hop is inert here. SP0036 reports 29 cross-slice bindings, resolving each import through its module path so the correct slice is named; one screen reports five. The one-hop follow through sibling *ViewModel.ts files is implemented and specified but adds 0 bindings on that application — its 17 ViewModels import commands, prop types and helpers, never a query proxy; the query imports sit in the .tsx directly. Which query a screen binds when a bare name collides across slices was deliberately left alone, as changing it would remove bindings.

Diagnostics, before and after, same commit lineage

Code Before After
SP0011 6 2 four property-reference operands now emitted
SP0016 643 643 text improved, count unchanged — see above
SP0031 0 1 genuine and newly reachable: two TimelineEntryKind enums were both unreachable before; now both are reached and the collision is reported, first wins deterministically
SP0035 0 13 new — one per record an event or command carries whose shape is undeclarable (upstream Cratis/Screenplay#29)
SP0036 0 29 new — cross-slice query bindings previously dropped in silence
all others unchanged unchanged
Total 1061 1100

Verification

  • Screenplay.csproj Release after dotnet clean: 0 errors, 0 warnings.
  • 973 specs passing (928 before, +45); the report lists exactly which of them were confirmed to fail against pre-fix code, and it is a subset — the rest are perimeter guards that cannot fail by construction.
  • End-to-end gate on TestApps/AspNetCore: exit 0, reads back clean, diagnostics unchanged.
  • Real application: exit 0, 5530 lines, reads back clean under the Screenplay compiler.
  • Determinism: two runs, document and diagnostics both byte-identical.

Caveat

TypeRegistry.cs is now 251 lines, over the repository's ~200-line guideline (244 before). A split into a separate concept registry is available; it was left rather than churn the area for size alone. Worth a reviewer's opinion.

@woksin woksin changed the title Recover concepts, comparisons and messages the document was dropping Recover more of an application and report what cannot be recovered Jul 28, 2026
@woksin
woksin changed the base branch from feat/screenplay-sp0024 to main July 28, 2026 22:28
@woksin

woksin commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated reviewer context for the whole change — deliberately kept out of the description. This PR now supersedes #2406 and #2407; all 19 commits are here.

This is agent-authored code that no human has read yet. Green CI is not sign-off. Do not merge without explicit review.

Where I would look first

The suppression interaction between #2393 and the document check. SP0024 previously suppressed SP0034 (the generator reading its own printed document back) unconditionally. That is now narrowed to the Error branch. As an Error, SP0024 says nothing recovered can be trusted, so a poor document is the broken build rather than a second defect. As a Warning it says the opposite — and suppressing SP0034 there would hand back a document the Screenplay compiler rejects while reporting IsSuccess == true, the one outcome SP0034 exists to make impossible. Two changes interacting in a way neither issue anticipated.

The SP0024 rule itself. Error exactly when the count of artifacts recovered from a declaration no compilation error sits inside is zero; Warning otherwise. A threshold was deliberately rejected — the same recovery would pass for a large application and fail for a small one. "Inside a declaration" is measured against every DeclaringSyntaxReference, so an error in a build-generated partial counts as inside the artifact.

The end-to-end CI gate (Source/DotNET/Screenplay.EndToEnd/, wired into dotnet-build.yml) is the most valuable thing here and is not in the release notes. It generates from TestApps/AspNetCore and compiles the result with the Cratis.Screenplay compiler, failing on any compiler diagnostic including warnings. Two of the defects found before this package first shipped were invisible to hermetic specs by construction.

Worth knowing: because the workflow triggers on pull_request_target, a job added in a PR runs from the base branch and cannot execute on its own PR. I proved the gate works by dispatching the workflow against the branch directly — it generated 53 lines and reported "reads back clean" on a runner. Its first pull_request_target run will be after this merges.

Three of the issues' premises did not hold

Checked against the source, not taken from the issues:

Diagnostics on the reference application, across the whole change

Code Before After
SP0011 6 2 property-reference operands now emitted
SP0016 643 643 text improved, count unchanged — see above
SP0021 1 0 the external event is imported
SP0024 1 0 resolved upstream in the host; calibration makes it a warning regardless
SP0025 3 0 an obj/ generator folder no longer counts as slice source
SP0031 0 1 genuine and newly reachable: two same-named enums were both unreachable before
SP0035 0 13 new — records whose shape is undeclarable (upstream Cratis/Screenplay#29)
SP0036 0 29 new — cross-slice query bindings previously dropped in silence

Verification

  • Screenplay.csproj Release after dotnet clean: 0 errors, 0 warnings.
  • 973 specs passing, up from 804 when this line of work started. Each phase's new specs were confirmed to fail against pre-fix code by reverting and re-running; the reports name exactly which, and which are perimeter guards that cannot fail by construction.
  • End-to-end gate: exit 0, "reads back clean", both locally and on a CI runner.
  • Reference application: 5530 lines, reads back clean under the official Screenplay compiler, byte-identical across two runs.

Caveats

  • Analysis/Types/TypeRegistry.cs is 251 lines, over the ~200-line guideline. A split into a separate concept registry is available; left rather than churn the area for size alone.
  • The gate does not carry the resx PrepareResources workaround the CLI has, so it would trip on an application whose resource classes are build-generated. TestApps/AspNetCore has none, so CI is unaffected.
  • Screenplay generation: separator-carrying constraint names are sanitized into unreadable identifiers #2398's "prefer the declaring type name minus a Constraint suffix" was not adopted: one IConstraint may declare several rules in a single Define, and naming them all after the type collapses them into duplicate constraint names in a slice.
  • Screenplay generation: cross-slice query imports in screens are dropped silently #2396's optional one-hop follow through sibling *ViewModel.ts files is implemented and specified but adds nothing on the reference application, whose view models import commands and prop types rather than query proxies.

@woksin

woksin commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Repository-wide verification, which should have run before this was pushed rather than after.

  • dotnet clean Arc.slnx -c Release then dotnet build Arc.slnx -c Release: 0 errors, 1 warning — the pre-existing CS0436 in TestApps/Chronicle, which reproduces on main without this branch.
  • Full suite, dotnet test Arc.slnx -c Release: every project passes. Two runs reported a single failure each in Arc.Core.Specs on net9.0 and net10.0; re-running that project in isolation gives 1552/1552 on both, so it is the known parallel timing flake in that suite rather than anything from this change.
  • Screenplay.Specs: 973 across net8.0/net9.0/net10.0.
  • Working tree clean afterwards, including TestAppsCommands.ts did not regenerate dirty.

woksin and others added 2 commits July 29, 2026 01:05
SP0013, SP0015, SP0016 and SP0020 each report something the application
really declares that the document cannot yet hold. Which of them is a
permanent gap and which is waiting on a specific change to the language
was carried nowhere, so a reader had no way to tell a report that will
always stand from one that disappears the moment an issue is resolved.

Each reference is written into the explanation rather than appended to
it, so it reads as the reason the gap exists, matching how SP0035 and
SP0036 already cite theirs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TypeRegistry had grown past the size a file is meant to stay under by
answering two questions at once: what name to write for a type, and what
the application's concepts are. The second is a registry with rules of
its own - what counts as a concept, which declaration wins when two share
a name, how a personal-data mark and a validation rule arriving later
still find the concept they belong to - and none of that has anything to
do with unwrapping a collection or reporting what a name loses.

ConceptRegistry now owns the concepts, and TypeRegistry keeps type
resolution and the two things that fall out of writing a name. The
enumeration and ConceptAs branches that were repeated between naming a
type and walking what it carries collapse into one TryRegister, which is
the same decision stated once.

Behavior is unchanged: the document generated for a real application is
byte-identical before and after, and all 973 specs pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@woksin

woksin commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Three follow-ups pushed (a8422af0, 0569cb8c). None changes behaviour, so the release notes are unchanged.

Diagnostics now name the upstream language issue they wait on. SP0013Cratis/Screenplay#33, SP0015#30, SP0016#32, SP0020#39, written into the explanation rather than appended as a tag.

Five candidates were considered and rejected, which is the more useful half:

Also verified as having no corresponding diagnostic at all: #31 (a CLI concern), #34 (no side-effect concept exists — a returning reactor is silently recorded as translating), #35, #36, #37 (ISubject<T> is silently stripped as a wrapper), #38 (IsPii is a bare bool with no reason field). Several of those silences may deserve diagnostics of their own; not in scope here.

TypeRegistry split, 251 → 160, with a new ConceptRegistry at 163 — both under the guideline. The seam is real rather than a cut: the type was answering what name do I write and what are the application's concepts at once, and the second has its own rules about what counts, which declaration wins on a name collision, and how a later @pii mark finds its concept. A duplicated enum/ConceptAs branch pair collapsed out of it. Public surface unchanged.

Comparison against the reference document. Fresh output 5,526 lines against the 5,348-line reference: 52 removed, 230 added, 61 hunks, and every line classified — nothing unplaced.

  • Intended fixes (188 added, 31 removed): 24 recovered concept declarations including two carrying @pii; 31 constraint names becoming readable; the one import for the externally declared event; 4 recovered comparisons.
  • Source drift in the application itself (42 added, 21 removed): a new EmailReservations slice with its event, and two reducers replaced by real projections — the C# type names changed, which only the application can do.

One name looked wrong and was chased down rather than waved past: UniqueTimesheetMonthendReminder. The application's own source declares "unique-timesheet-monthend-reminder"; the generator reproduced the misspelling faithfully.

Determinism and refactor safety in one check: the document generated before the split, after it, and on a second run afterwards all hash d142fcca….

Caveat on that comparison: the reference application's working tree has 87 uncommitted modifications and was being edited concurrently during the run, so the drift classification is against a live tree rather than a commit. It does not weaken the byte-identity guard above, and /Users/sindrewilting/Ada.play was not modified.

Verification: Release 0 errors / 0 warnings; 973 specs, count unchanged as a refactor requires; end-to-end gate exit 0, reads back clean; worktree clean.

Comment on lines +97 to +104
foreach (var assembly in compilation.SourceModule.ReferencedAssemblySymbols
.OrderBy(_ => _.Identity.GetDisplayName(), StringComparer.Ordinal))
{
if (names.Any(assembly.TypeNames.Contains))
{
Collect(assembly.GlobalNamespace, names, found);
}
}
Comment on lines +67 to +78
foreach (var property in type.DeclaredProperties())
{
var carried = UnderlyingTypes.Of(property.Type);
var name = carried.ToDisplayString();

found.TryAdd(name, carried);

if (IsRecord(carried) && walked.Add(name))
{
Walk(carried, found, walked);
}
}
@woksin

woksin commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #2413, which now targets main directly and carries all 34 commits of this stack as one change with combined release notes covering #2393, #2394, #2395, #2396, #2397, #2398, #2399, #2400, #2401, #2403, #2404 and #2409.

Nothing is lost — every commit here is an ancestor of that branch, and the branch is kept. Reviewer context for all of it is consolidated into a single comment there.

Closing so there is one PR to review and one set of release notes rather than four that have to be merged in order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant