Issue #3620618: Tests whose assertions do not pin the behaviour they name

Fixes all twenty-five findings in #3620618.

What "green" was worth here

The suite passed before this branch and it passes after, so nothing here is a bug fix. Each finding is an assertion that would have kept holding while the behaviour it names broke, and the work is to make each one able to fail. Sixteen were then seen to fail against deliberately broken production code, with the break named each time; the rest are listed at the end with the reason there is no break behind them.

The ones that were pinning nothing at all

A timeout join's configured hour was never compared to anything. The test asserted a deadline existed and then overwrote it with a past timestamp to fire the sweep. Arming one second instead would have stayed green here while every timeout join tore down live branches on the first cron run after the first arrival. Now the armed value is asserted before the backdating: replacing the configured duration with '1' fails it by 3,599 seconds.

"Milestones are ordered oldest first" compared two timestamps that are always equal. Every token in one kernel request shares a request time, so the comparison held whatever order the query returned. The accumulating test now asserts the rendered time comes from the token rather than the instance or the request, which is all three copies of one number can show; a second test reverses the clock against the token order — end node oldest, review node newest — and asserts the timeline follows the clock with three times that differ. Dropping ->sort('created') fails that one and leaves the rest of the class green.

The non-timer filter in the current-step resolver was named in two docblocks and asserted nowhere. A timer parks a token of its own beside the one doing the waiting, so a resolver that asks only for parked tokens reports the timer's node as a step no person is standing at, in the current-step column of every list surface. The test parks a timer token on the end node, where a leak cannot be mistaken for the real step, and asserts both the single and the batch resolver drop it.

Equality "compares loosely" and every assertion was a same-type string. Hardening == to === — a standard suggestion — passed all four. A configured value is always a string because config is where it was typed, while the variable carries whatever a step wrote, so that change stops a flow condition written as 1 from matching a step that stored TRUE. That case now fails against ===, with the FALSE case asserted beside it so the fix cannot be a coercion that matches anything.

The failed leg of InstanceEndedEvent had never fired under any assertion. A listener that releases what a run reserved runs on the failed path as much as on the other two. Reached the way EngineFailurePathTest reaches it, with a token pointed at a node the definition no longer holds; suppressing that one dispatch fails the new test and nothing else.

Neither deletion test could tell a cancel from a purge. Both asserted every row was gone, which is true whether or not the running instances were canceled first. What the cancel adds is in the hook's own comment — the ending is announced while the run still exists — so both tests now capture InstanceEndedEvent across the delete and assert the running instance, and only it, ended as canceled. Removing cancelInstance() from either hook fails its test.

The purge command's deleting branch was never entered. One test keeps its instance because retention is off, the other because it asked for a dry run. Pinning the command to dry-run for ever would have left both green while nothing was purged on any site. There is now a run that deletes, and the dry-run test asserts the count it reports rather than only that nothing was deleted — the count is what an operator reads to decide whether to run it for real.

The dead-end opt-out was asserted to be quiet, never to be logged, which the production comment insists on: "opted out of the incident, but never silent". Removing that warning now fails the test.

The instance-delete cascade covers four tables and two were asserted. It seeds an incident and a work item and asserts all four, so dropping either from the hook fails instead of leaving rows pointing at an instance that is gone.

All three retention previews were seeded to report the same number, so no form's scope wiring was pinned: swapping the tenant form's scope for the workflow's would have stayed green while a tenant page reported a cross-tenant total. The fixture now makes the three scopes report 6, 5 and 3.

A negative duration was rejected by both docblocks and asserted in neither spelling. Relaxing the digit check to ltrim($duration, '-') — which is exactly what toTimestamp() does one method away — then parks a task already overdue by the amount configured. Seen to fail that way, with -5 answering the anchor plus five. The same docblocks also listed zero among the rejections, which #3620608 settled the other way and left the prose behind.

Docblocks that described a different test

Four said more than their assertions pinned, and in each case the assertion was the honest one and the prose was corrected, or the prose was right and the assertion was added:

• the variable-set audit event "carries the name and scope but not the value" — it carries the value, deliberately, and recordVariable()'s own docblock says why;
• the resource and correlation id promised for three lifecycle events and asserted on one — an id present only on the opening event groups nothing, so completed, canceled and the canceled token assert it too;
• a join-cancellation docblock naming the parked branch as asserted — it now is, because a live token left behind in a canceled instance is what would resume a run nobody is waiting for;
• a helper called "the open incidents" whose query has no state condition — unfiltered is what its caller wants, since it asserts none were raised at all.

And two named things that do not exist: CurrentStepsFor() (it is getCurrentStepsByInstance()) and, in the engine index test, a claim that removing any declared index fails the build.

Every declared index, counted

That claim was false in both index tests. Comparing every name the storage schemas declare against every name any test asserts found seven unasserted, not the three the issue names: orchestra_token__instance_created, orchestra_instance__initiator, orchestra_instance__initiator_status, orchestra_instance__tenant_id, orchestra_incident__instance_state, orchestra_work_item__completer_completed, and both of orchestra_delegation's. The runtime test now lists the whole of what the src/ schemas declare and says so, since a chosen few is how the gap opened; renaming one declared index fails it. Delegation had no index test at all and now has one, following the pattern the payment, work-item and attachment tests already set.

Assertions that could pass on nothing

• A bounded drain asserted "none parked", which also holds for a drain that processed nothing. It now counts what moved: exactly one run's token reached the wait node. Removing the bound from the loop fails it.
• A teardown query-count budget compared two counts that would both be zero if the query log observed nothing. The non-zero guard is asserted first.
• One half of the resume-outcome test looped over a list that could be empty, which is the one way a test of a declaration agrees with anything; and that list was two hardcoded ids, so a silent action added later was skipped in silence. It is read from the definitions now, with the exclusions named and reasoned, so a new one arrives in the list and fails until somebody classifies it.
• "Every token consumed" asserted only "none active, none parked", which a token left waiting at a join, canceled or errored satisfies while the run finished around it.
• A variables test asserted a token key existed, so the wrong token was invisible; the id is asserted.
• A tenant-scoped workflow's dependency on its tenant was named in the class docblock and never asserted, because every fixture was untenanted. Removing that dependency now fails.
• Two of the retention preview's three bands were asserted nowhere: the wording for nothing eligible, and the wording that tells an operator the count is a floor rather than a total because counting stopped at the cap. A unit test drives all three; collapsing the zero band into the count fails it.

What has no break behind it

Nine, and the reason for each, since "strengthened" is easy to claim and a break is the only thing that shows it:

• RetentionUiTest is functional, so CI validates its new seeding rather than a local break. The phpunit job is green, which is what confirms the 6 / 5 / 3 arithmetic.
• The consumed-token, parked-branch and seen_token assertions are stronger by construction: each asserts a specific value or state where the old one asserted a key's presence or the absence of two other states. No break for them is anything but contrived.
• The two non-empty guards (the teardown query budget, the silent-action list) exist to catch a harness that observed nothing, so breaking production is the wrong instrument — the thing they guard against is the test's own fixture going empty.
• The work-item and delegation index assertions rest on the same mechanism proven for the runtime schema, where renaming a declared index fails the test. The delegation class is new, and its passing is itself the evidence that the module's storage schema handler is wired at all.
• The accumulating status-history assertion pins where the timestamp comes from, not which token each milestone read it from: every token in one request shares a created time. The ordering test is what distinguishes them.

The five prose corrections are not assertions and have nothing to prove: three docblocks that said the opposite of what production does (established from recordVariable()'s own docblock, from the single caller of the incident helper, and by running Duration), and two references to things that do not exist.

This merge request was prepared with the assistance of an AI agent (Claude). The analysis and the wording were reviewed by me before posting, and accountability for the content is mine.