From 137c496d74f6e9f71b3b0cebb14b2e25f0558d0b Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 20 Aug 2026 17:46:01 +0200 Subject: [PATCH] feat(agent): say which forced source a queued run did not get MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-175 settled that a source switched off between enqueue and start does not enter the run. Nothing said so. The run proceeded without it and the only way to notice was to compare the queued request against the transcript. The dequeue now reports what it asked for and did not get, and why: deactivated for a record that is switched off, gone for one that no longer resolves. Telling those apart is the point — one is reversible, the other is not — and it needs the SECOND lookup: a uid the enabled-only lookup skipped but the existence lookup finds is switched off. Without both, the two reasons are one. It travels on RunAugmentation, is written through RunTrace as its own step kind before assembly, and reaches the run view through RunStep's payload and the timeline's allow-list. Recorded on the resolve side rather than inferred from the assembled messages, because a source that never arrived leaves no mark in them — that absence is what an operator cannot see today. Flattened to "kind#uid reason" strings, not a count: "2 dropped" would flatten exactly the distinction the record exists to make. A run that dropped nothing records no step, so the step's presence is the signal. The resume path is untouched: ADR-166 and ADR-175 keep a deactivated source resolving there, so nothing is dropped and a step would imply otherwise. An existing test caught a real defect in the first version. ToolLoopServiceAssemblyOrderTest pins that the enabled-only lookup runs once per dequeue; my detection called it a second time. The records that did arrive are now passed in rather than re-queried — one extra query per kind instead of two. Two controls, each observed: resolving both sides with the enabled-only lookup makes the two reasons indistinguishable and fails two tests; removing the empty-list guard makes a clean run record a step and fails the test that forbids it. ADR-179 claimed RunAugmentation was @internal. It is on the frozen surface, and that sentence is corrected in the record rather than left for the next reader. The growth is additive: a parameter with a default plus one method, announced under ### Added. ADR-176 said five builtins write through the DataHandler. ADR-180 added a sixth within a day, so the number is replaced by the command that answers it — the trait every writer uses, not a search for the class name, which also matches the two traits and overcounts by two. Closes #809 Signed-off-by: Sebastian Mendel --- CHANGELOG.md | 21 +++ Classes/Domain/Enum/DroppedSourceReason.php | 42 ++++++ Classes/Domain/ValueObject/DroppedSource.php | 30 ++++ Classes/Domain/ValueObject/RunStep.php | 23 +++ .../Service/Agent/AgentRunRequestCodec.php | 103 ++++++++++++- .../Agent/Timeline/RunTimelineFactory.php | 5 + Classes/Service/Tool/RunAugmentation.php | 16 +++ Classes/Service/Tool/RunTrace.php | 24 ++++ Classes/Service/Tool/ToolLoopService.php | 13 ++ ...r176PerCallOutcomeSeparateFromApproval.rst | 16 ++- ...ADroppedForcedSourceIsRecordedOnTheRun.rst | 13 +- .../DroppedForcedSourceDetectionTest.php | 136 ++++++++++++++++++ Tests/Unit/Api/api-surface.txt | 4 +- .../Service/Agent/DroppedForcedSourceTest.php | 88 ++++++++++++ 14 files changed, 525 insertions(+), 9 deletions(-) create mode 100644 Classes/Domain/Enum/DroppedSourceReason.php create mode 100644 Classes/Domain/ValueObject/DroppedSource.php create mode 100644 Tests/Functional/Service/Agent/DroppedForcedSourceDetectionTest.php create mode 100644 Tests/Unit/Service/Agent/DroppedForcedSourceTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 2323d580a..4228f0a88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,27 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). reasons that say nothing about the model, so folding it in would measure the gate instead of the answer. +- A forced source a queued run asked for and did not get is now recorded on the + run (ADR-179). + + Switching a snippet or skill off between enqueue and start drops it — that is + ADR-175's rule and stays — but until now nothing said so, and the only way to + notice was to compare the queued request against the transcript. + + The run now carries a `dropped` step naming each source and why: `deactivated` + for a record that is switched off, `gone` for one that no longer resolves. + Those stay apart on purpose — a deactivated record can be switched back on, a + removed one cannot, so a single "dropped" would send the reader looking. + + A run that dropped nothing records no step, so the step's presence is the + signal. The resume path is untouched: ADR-166 and ADR-175 keep a deactivated + source resolving there, so nothing is dropped and a report would imply + otherwise. + + `RunAugmentation` gains a `droppedSources` parameter, appended with a default; + `RunTrace` gains `recordDroppedSources()`. Both are additive. + + ### Changed - **AGENTS.md files synchronized with the repository state.** Root slimmed from 356 to 119 lines by moving content into the scoped files it belongs to; stale inventories refreshed (TCA files, database tables, backend templates, JS modules, `Services.Dashboard.php`); phantom `TCA/Overrides/` and dead `MEMORY.md` references removed; generic workflow boilerplate in `.github/workflows/AGENTS.md` replaced with this repository's actual conventions (no local jobs, release flow, dependency automation). diff --git a/Classes/Domain/Enum/DroppedSourceReason.php b/Classes/Domain/Enum/DroppedSourceReason.php new file mode 100644 index 000000000..f758c7aa9 --- /dev/null +++ b/Classes/Domain/Enum/DroppedSourceReason.php @@ -0,0 +1,42 @@ +>|null $messagesSent Snapshot of the messages sent this round (REQUEST/assembled). * @param list|null $toolSpecs Names of the tools offered this round (REQUEST). @@ -78,6 +87,11 @@ public function __construct( public ?bool $toolIsError = null, public ?array $toolArtifacts = null, public ?ContextBudgetBreakdown $contextBudget = null, + /** + * @var list|null the sources this run asked for and did + * not get; null on every other kind + */ + public ?array $droppedSources = null, ) {} /** @@ -106,6 +120,15 @@ public function toArray(): array 'estimatedCost' => $this->estimatedCost, 'requestedToolCalls' => $this->requestedToolCalls, 'raw' => $this->raw, + // Flattened to "kind#uid reason" strings rather than nested objects: + // the timeline's allow-list renders scalars and simple lists, and a + // count alone would flatten the two reasons ADR-179 keeps apart. + // uid and reason are metadata, not content — the privacy filter's + // concern is the transcript, and nothing here carries prose. + 'droppedSources' => $this->droppedSources === null ? null : array_map( + static fn(DroppedSource $d): string => sprintf('%s#%d %s', $d->kind, $d->uid, $d->reason->value), + $this->droppedSources, + ), 'toolName' => $this->toolName, 'toolArguments' => $this->toolArguments, 'toolResult' => $this->toolResult, diff --git a/Classes/Service/Agent/AgentRunRequestCodec.php b/Classes/Service/Agent/AgentRunRequestCodec.php index a7a68e3d0..582265416 100644 --- a/Classes/Service/Agent/AgentRunRequestCodec.php +++ b/Classes/Service/Agent/AgentRunRequestCodec.php @@ -9,6 +9,7 @@ namespace Netresearch\NrLlm\Service\Agent; +use Netresearch\NrLlm\Domain\Enum\DroppedSourceReason; use Netresearch\NrLlm\Domain\Model\PromptSnippet; use Netresearch\NrLlm\Domain\Model\Skill; use Netresearch\NrLlm\Domain\Repository\LlmConfigurationRepository; @@ -17,6 +18,7 @@ use Netresearch\NrLlm\Domain\ValueObject\AgentRun; use Netresearch\NrLlm\Domain\ValueObject\AiActorContext; use Netresearch\NrLlm\Domain\ValueObject\ChatMessage; +use Netresearch\NrLlm\Domain\ValueObject\DroppedSource; use Netresearch\NrLlm\Service\Agent\Exception\RunConfigurationGoneException; use Netresearch\NrLlm\Service\Option\ToolOptions; use Netresearch\NrLlm\Service\Tool\RunAugmentation; @@ -154,10 +156,15 @@ public function rehydrate(AgentRun $run): AgentRunRequest $augmentation = null; if (is_array($data['augmentation'] ?? null)) { $augmentationData = $data['augmentation']; + $skillUids = $this->uidList($augmentationData['forcedSkillUids'] ?? null); + $snippetUids = $this->uidList($augmentationData['forcedSnippetUids'] ?? null); + $forcedSkills = $this->skillsByUids($skillUids); + $forcedSnippets = $this->snippetsByUids($snippetUids); $augmentation = new RunAugmentation( - forcedSkills: $this->skillsByUids($this->uidList($augmentationData['forcedSkillUids'] ?? null)), - forcedSnippets: $this->snippetsByUids($this->uidList($augmentationData['forcedSnippetUids'] ?? null)), + forcedSkills: $forcedSkills, + forcedSnippets: $forcedSnippets, dryRun: ($augmentationData['dryRun'] ?? false) === true, + droppedSources: $this->droppedSources($skillUids, $forcedSkills, $snippetUids, $forcedSnippets), ); } @@ -241,4 +248,96 @@ private function snippetsByUids(array $uids): array return $this->promptSnippetRepository->findByUids($uids); } + + /** + * The forced sources this run asked for and did not get (ADR-179). + * + * Resolves each kind a SECOND time through its existence lookup. That is + * the only way to tell the two reasons apart: a uid the enabled-only + * lookup skipped but the existence lookup finds is switched off; one + * neither finds is gone. Without the second call both look identical, and + * a reader would be told "dropped" without being told what to do about it. + * + * ONE extra query per kind, not two: the records that DID arrive are passed + * in, because the rehydration has just resolved them. Re-querying them here + * would double every enabled-only lookup on the dequeue path — which is + * what `ToolLoopServiceAssemblyOrderTest` pins, and it caught exactly that + * in the first version of this method. + * + * @param list $skillUids uids the run was queued with + * @param list $arrivedSkills what the enabled-only lookup returned + * @param list $snippetUids + * @param list $arrivedSnippets + * + * @return list + */ + private function droppedSources(array $skillUids, array $arrivedSkills, array $snippetUids, array $arrivedSnippets): array + { + $dropped = []; + + if ($skillUids !== [] && $this->skillRepository instanceof SkillRepository) { + $dropped = [...$dropped, ...$this->missing( + 'skill', + $skillUids, + $this->uidsOf($arrivedSkills), + $this->uidsOf($this->skillRepository->findExistingByUids($skillUids)), + )]; + } + + if ($snippetUids !== [] && $this->promptSnippetRepository instanceof PromptSnippetRepository) { + return [...$dropped, ...$this->missing( + 'snippet', + $snippetUids, + $this->uidsOf($arrivedSnippets), + $this->uidsOf($this->promptSnippetRepository->findExistingByUids($snippetUids)), + )]; + } + + return $dropped; + } + + /** + * @param list $requested + * @param list $arrived uids the enabled-only lookup returned + * @param list $existing uids the existence lookup returned + * + * @return list + */ + private function missing(string $kind, array $requested, array $arrived, array $existing): array + { + $dropped = []; + foreach ($requested as $uid) { + if (in_array($uid, $arrived, true)) { + continue; + } + + $dropped[] = new DroppedSource( + $kind, + $uid, + in_array($uid, $existing, true) + ? DroppedSourceReason::DEACTIVATED + : DroppedSourceReason::GONE, + ); + } + + return $dropped; + } + + /** + * @param list|list $records + * + * @return list + */ + private function uidsOf(array $records): array + { + $uids = []; + foreach ($records as $record) { + $uid = $record->getUid(); + if ($uid !== null) { + $uids[] = $uid; + } + } + + return $uids; + } } diff --git a/Classes/Service/Agent/Timeline/RunTimelineFactory.php b/Classes/Service/Agent/Timeline/RunTimelineFactory.php index 6c71dae2e..634180b77 100644 --- a/Classes/Service/Agent/Timeline/RunTimelineFactory.php +++ b/Classes/Service/Agent/Timeline/RunTimelineFactory.php @@ -69,6 +69,11 @@ 'contentRedacted', 'approved', 'decidedBy', + // The forced sources a run asked for and did not get (ADR-179). On the + // list because the absence is invisible everywhere else: a source that + // never arrived leaves no mark in the transcript, which is exactly why + // the operator cannot see it today. + 'droppedSources', ]; public function __construct( diff --git a/Classes/Service/Tool/RunAugmentation.php b/Classes/Service/Tool/RunAugmentation.php index 39f152475..1757a41df 100644 --- a/Classes/Service/Tool/RunAugmentation.php +++ b/Classes/Service/Tool/RunAugmentation.php @@ -11,6 +11,7 @@ use Netresearch\NrLlm\Domain\Model\PromptSnippet; use Netresearch\NrLlm\Domain\Model\Skill; +use Netresearch\NrLlm\Domain\ValueObject\DroppedSource; use Netresearch\NrLlm\Domain\ValueObject\InjectedContext; /** @@ -33,11 +34,26 @@ /** * @param list $forcedSkills * @param list $forcedSnippets + * @param list $droppedSources sources asked for that did not arrive */ public function __construct( public array $forcedSkills = [], public array $forcedSnippets = [], public bool $dryRun = false, + /** + * Forced sources this run asked for and did not get (ADR-179). + * + * Empty is the normal case and means "nothing was dropped", never + * "not checked": a caller that builds the augmentation by hand has + * nothing to compare against, and a run whose sources all resolved + * is indistinguishable from it — deliberately, because both are the + * same statement about what reached the model. + * + * Appended last with a default, so every existing caller keeps + * working. The class is on the frozen surface, so the growth is + * announced rather than silent. + */ + public array $droppedSources = [], ) {} /** diff --git a/Classes/Service/Tool/RunTrace.php b/Classes/Service/Tool/RunTrace.php index 8f7c5d5ae..a068666d2 100644 --- a/Classes/Service/Tool/RunTrace.php +++ b/Classes/Service/Tool/RunTrace.php @@ -13,6 +13,7 @@ use Netresearch\NrLlm\Domain\Model\CompletionResponse; use Netresearch\NrLlm\Domain\ValueObject\ChatMessage; use Netresearch\NrLlm\Domain\ValueObject\ContextBudgetBreakdown; +use Netresearch\NrLlm\Domain\ValueObject\DroppedSource; use Netresearch\NrLlm\Domain\ValueObject\RunStep; use Netresearch\NrLlm\Domain\ValueObject\ToolArtifact; use Netresearch\NrLlm\Domain\ValueObject\ToolCall; @@ -108,6 +109,29 @@ public function recordRequest(int $round, array $messagesSent, array $toolSpecs) * the question this answers, and a run only reaches the interesting answer * after the boring ones. */ + /** + * Record the forced sources this run asked for and did not get (ADR-179). + * + * Called once, before the first round, and only with a non-empty list: a + * run whose sources all resolved records nothing, so the step's presence + * is the signal and an empty step would be noise on every other run. + * + * @param list $dropped + */ + public function recordDroppedSources(array $dropped): void + { + if ($dropped === []) { + return; + } + + $this->add(new RunStep( + kind: RunStep::KIND_DROPPED, + round: 0, + durationMs: 0.0, + droppedSources: $dropped, + )); + } + public function recordContextBudget(int $round, ContextBudgetBreakdown $breakdown): void { $this->add(new RunStep( diff --git a/Classes/Service/Tool/ToolLoopService.php b/Classes/Service/Tool/ToolLoopService.php index 80f2f84d2..e3c327146 100644 --- a/Classes/Service/Tool/ToolLoopService.php +++ b/Classes/Service/Tool/ToolLoopService.php @@ -252,6 +252,19 @@ public function runLoop( if ($skipAssembly) { $dryRun = false; } else { + // Before anything is assembled: what the run asked for and did not + // get (ADR-179). Recorded on the resolve side rather than inferred + // from the assembled messages, because a source that never arrived + // leaves no trace in them — that absence is exactly what an + // operator cannot see today. + // + // Not on the resume branch above: ADR-166 and ADR-175 keep a + // deactivated source resolving there on purpose, so nothing is + // dropped and a step would imply otherwise. + if ($augmentation instanceof RunAugmentation) { + $runTrace?->recordDroppedSources($augmentation->droppedSources); + } + [$messages, $dryRun] = $this->assemble($messages, $configuration, $options, $augmentation); } diff --git a/Documentation/Adr/Adr176PerCallOutcomeSeparateFromApproval.rst b/Documentation/Adr/Adr176PerCallOutcomeSeparateFromApproval.rst index a938b7d30..990442f62 100644 --- a/Documentation/Adr/Adr176PerCallOutcomeSeparateFromApproval.rst +++ b/Documentation/Adr/Adr176PerCallOutcomeSeparateFromApproval.rst @@ -108,10 +108,18 @@ not an identity. Without a queryable write target there is no join to That is :ref:`ADR-122 `'s deferred territory. It declined to build a side-effecting tool contract because no tool had side effects, and its status -already records that premise as expired. Five builtins write through the -DataHandler today. So the prerequisite is not new work invented here; it is the -part of ADR-122 whose reason for waiting has gone, and the outcome signal is -the first reader it lacked. +already records that premise as expired. Builtins write through the DataHandler +today, and the number is deliberately not repeated here: this record first said +five, and :ref:`ADR-180 ` added a sixth within a day. What the argument +needs is that the count is no longer zero, which is what expired ADR-122's +premise. Where a number is wanted, +``grep -l 'use WritesThroughDataHandlerTrait' Classes/Service/Tool/Builtin/*.php`` +answers it — the trait every writer uses, rather than a search for the class +name, which also matches the two traits themselves and overcounts by two. + +So the prerequisite is not new work invented here; it is the part of ADR-122 +whose reason for waiting has gone, and the outcome signal is the first reader +it lacked. What this does not do ===================== diff --git a/Documentation/Adr/Adr179ADroppedForcedSourceIsRecordedOnTheRun.rst b/Documentation/Adr/Adr179ADroppedForcedSourceIsRecordedOnTheRun.rst index 8876eea0e..3fc1be3d2 100644 --- a/Documentation/Adr/Adr179ADroppedForcedSourceIsRecordedOnTheRun.rst +++ b/Documentation/Adr/Adr179ADroppedForcedSourceIsRecordedOnTheRun.rst @@ -83,8 +83,17 @@ An operator reading a run sees, next to the sources that were injected, the ones that were asked for and did not arrive, and which of the two things happened to each. -:php:`RunAugmentation` gains a field. It is ``@internal``, so no frozen surface -moves. +:php:`RunAugmentation` gains a field, and this record first claimed the class was +``@internal`` so nothing frozen would move. That was wrong and is corrected +here rather than left for the implementation to discover: +``Tests/Unit/Api/api-surface.txt`` lists ``RunAugmentation (class)`` with its +full constructor, so it is ``@api`` and frozen. + +The consequence is not that the field cannot be added — a parameter with a +default is an additive change, which is what the snapshot's own failure message +distinguishes from a breaking one. The consequence is that adding it is not +free: the snapshot is regenerated and the change is announced under +``### Added``, like any other growth of the public surface. The playground's synchronous send composes the same way and gains the same record. The queued path is where the gap between request and start is wide diff --git a/Tests/Functional/Service/Agent/DroppedForcedSourceDetectionTest.php b/Tests/Functional/Service/Agent/DroppedForcedSourceDetectionTest.php new file mode 100644 index 000000000..52b5341bb --- /dev/null +++ b/Tests/Functional/Service/Agent/DroppedForcedSourceDetectionTest.php @@ -0,0 +1,136 @@ +importFixture('SkillsByUid.csv'); + $this->codec = new AgentRunRequestCodec( + $this->get(LlmConfigurationRepository::class), + $this->get(SkillRepository::class), + $this->get(PromptSnippetRepository::class), + ); + } + + /** + * @param list $skillUids + * + * @return list + */ + private function droppedFor(array $skillUids): array + { + // Resolved the way the rehydration resolves them, then handed over: + // the production path passes the records it already has rather than + // re-querying. A test that re-queried would exercise a comparison + // production does not make — and the duplicate query is what + // ToolLoopServiceAssemblyOrderTest caught in the first version. + $arrived = $skillUids === [] ? [] : $this->get(SkillRepository::class)->findByUids($skillUids); + + $method = new ReflectionMethod(AgentRunRequestCodec::class, 'droppedSources'); + + /** @var list $dropped */ + $dropped = $method->invoke($this->codec, $skillUids, $arrived, [], []); + + return $dropped; + } + + #[Test] + public function aDeactivatedSourceIsReportedAsDeactivated(): void + { + // uid 23 exists and is disabled in the fixture. + $dropped = $this->droppedFor([23]); + + self::assertCount(1, $dropped); + self::assertSame(23, $dropped[0]->uid); + self::assertSame('skill', $dropped[0]->kind); + self::assertSame(DroppedSourceReason::DEACTIVATED, $dropped[0]->reason); + } + + #[Test] + public function aVanishedSourceIsReportedAsGone(): void + { + // Nothing with this uid exists. From the run's side a deleted record + // and a uid that never existed are one event: it asked for something + // it did not get, and nothing remains to say which. + $dropped = $this->droppedFor([999999]); + + self::assertCount(1, $dropped); + self::assertSame(DroppedSourceReason::GONE, $dropped[0]->reason); + } + + #[Test] + public function aDeletedSourceIsGoneAndNotDeactivated(): void + { + // uid 24 is deleted=1. Both lookups keep the deleted restriction, so + // it resolves through neither — which is the only reason the two + // reasons stay distinguishable at all. + $dropped = $this->droppedFor([24]); + + self::assertSame(DroppedSourceReason::GONE, $dropped[0]->reason); + } + + #[Test] + public function anEnabledSourceIsNotReported(): void + { + // uid 22 is enabled: it arrives, so there is nothing to say about it. + self::assertSame([], $this->droppedFor([22])); + } + + #[Test] + public function aHiddenButEnabledSourceArrivesAndIsNotReported(): void + { + // uid 25 carries hidden=1. The repositories ignore enable fields, so + // `hidden` is FormEngine visibility and not a runtime gate — reporting + // it as dropped would tell an operator to fix something that is not + // broken. + self::assertSame([], $this->droppedFor([25])); + } + + #[Test] + public function theOrderFollowsTheRequestAndMixedCasesStaySeparate(): void + { + $dropped = $this->droppedFor([23, 22, 999999]); + + self::assertCount(2, $dropped, 'the enabled uid 22 must not appear'); + self::assertSame([23, 999999], array_map(static fn(DroppedSource $d): int => $d->uid, $dropped)); + self::assertSame(DroppedSourceReason::DEACTIVATED, $dropped[0]->reason); + self::assertSame(DroppedSourceReason::GONE, $dropped[1]->reason); + } + + #[Test] + public function anEmptyRequestCostsNoQuery(): void + { + self::assertSame([], $this->droppedFor([])); + } +} diff --git a/Tests/Unit/Api/api-surface.txt b/Tests/Unit/Api/api-surface.txt index fa4917325..87363b4a8 100644 --- a/Tests/Unit/Api/api-surface.txt +++ b/Tests/Unit/Api/api-surface.txt @@ -1613,8 +1613,9 @@ Netresearch\NrLlm\Service\Retrieval\SourceReference (class) property readonly parts: array Netresearch\NrLlm\Service\Tool\RunAugmentation (class) - constructor(array $forcedSkills = …, array $forcedSnippets = …, bool $dryRun = …) + constructor(array $forcedSkills = …, array $forcedSnippets = …, bool $dryRun = …, array $droppedSources = …) method injectedContext(): Netresearch\NrLlm\Domain\ValueObject\InjectedContext + property readonly droppedSources: array property readonly dryRun: bool property readonly forcedSkills: array property readonly forcedSnippets: array @@ -1626,6 +1627,7 @@ Netresearch\NrLlm\Service\Tool\RunTrace (class) method getSteps(): array method recordAssembledMessages(array $messages): void method recordContextBudget(int $round, Netresearch\NrLlm\Domain\ValueObject\ContextBudgetBreakdown $breakdown): void + method recordDroppedSources(array $dropped): void method recordLlmCall(int $round, float $durationMs, Netresearch\NrLlm\Domain\Model\CompletionResponse $response): void method recordRequest(int $round, array $messagesSent, array $toolSpecs): void method recordToolExecution(int $round, float $durationMs, string $name, array $arguments, string $result, bool $isError, array $artifacts = …): void diff --git a/Tests/Unit/Service/Agent/DroppedForcedSourceTest.php b/Tests/Unit/Service/Agent/DroppedForcedSourceTest.php new file mode 100644 index 000000000..54b623411 --- /dev/null +++ b/Tests/Unit/Service/Agent/DroppedForcedSourceTest.php @@ -0,0 +1,88 @@ +recordDroppedSources([ + new DroppedSource('snippet', 41, DroppedSourceReason::DEACTIVATED), + ]); + + $steps = $trace->getSteps(); + self::assertCount(1, $steps); + self::assertSame(RunStep::KIND_DROPPED, $steps[0]->kind); + self::assertNotNull($steps[0]->droppedSources); + self::assertSame(41, $steps[0]->droppedSources[0]->uid); + } + + #[Test] + public function aRunThatDroppedNothingRecordsNoStep(): void + { + // The step's PRESENCE is the signal. An empty step on every run would + // make the readout noise, and a reader would stop looking at it — + // which is the state ADR-179 exists to end. + $trace = new RunTrace(); + $trace->recordDroppedSources([]); + + self::assertSame([], $trace->getSteps()); + } + + #[Test] + public function theTwoReasonsStayDistinguishable(): void + { + // ADR-179 decides this: a deactivated record can be switched back on, + // a removed one cannot. Folding both into "dropped" would send the + // reader looking for which it was. + $trace = new RunTrace(); + $trace->recordDroppedSources([ + new DroppedSource('snippet', 41, DroppedSourceReason::DEACTIVATED), + new DroppedSource('skill', 77, DroppedSourceReason::GONE), + ]); + + $dropped = $trace->getSteps()[0]->droppedSources; + self::assertNotNull($dropped); + self::assertSame(DroppedSourceReason::DEACTIVATED, $dropped[0]->reason); + self::assertSame(DroppedSourceReason::GONE, $dropped[1]->reason); + self::assertNotSame($dropped[0]->reason, $dropped[1]->reason); + } + + #[Test] + public function bothKindsAreCarriedAndTellApart(): void + { + $trace = new RunTrace(); + $trace->recordDroppedSources([ + new DroppedSource('snippet', 41, DroppedSourceReason::GONE), + new DroppedSource('skill', 41, DroppedSourceReason::GONE), + ]); + + $dropped = $trace->getSteps()[0]->droppedSources; + self::assertNotNull($dropped); + // Same uid, different kind: the uid alone does not identify a source, + // so a readout keyed on it would merge two unrelated records. + self::assertSame('snippet', $dropped[0]->kind); + self::assertSame('skill', $dropped[1]->kind); + } +}