Skip to content

fix(runtime): start dependency replay after the prefix the worktree already holds - #314

Merged
aviggiano merged 4 commits into
mainfrom
fix/312-chain-superseded-replay
Aug 7, 2026
Merged

aviggiano merged 4 commits into
mainfrom
fix/312-chain-superseded-replay

Conversation

@aviggiano

@aviggiano aviggiano commented Aug 7, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #312.

What breaks

prepare:stateful-invariant-implement-properties fans in every dependency's workspace patch and replays them. That is correct on a fresh run: the task worktree starts at the pinned baseline, so each patch's declared base_tree is satisfied in turn down the chain. A resumed run breaks the precondition, because the worktree lives on a durable volume and still holds the previous attempt's state.

Two production runs died at this node with the same expected tree:

run image error
R48 62aaef1 expected 2dd4efef…, got bf324c39…
R49 51e6a7b expected 2dd4efef…, got b8d46f13…

R49 is the sharper evidence: it ran 24 nodes fresh without touching this bug, its sandbox exited, the overseer recovered it with action: launch, a new worker came up 90 seconds later with resume=true, restoredTaskCount=48, and it was dead within 90 seconds of resuming. Its overseer then exhausted its whole recovery budget — three further resumes, three identical deaths.

R48's dependency manifests, dumped from its volume:

node base_tree result_tree files
setup-foundry 2dd4efef 4fefa8df 1
base-test-setup 4fefa8df 4fefa8df 0
stateful-invariant-setup 4fefa8df fd4e3c2c 7
stateful-invariant-handlers fd4e3c2c bf324c39 7
stateful-invariant-coverage bf324c39 bf324c39 0

The worktree sat at the end of that chain. Every dependency's content was already present, folded in by the later links; applyWorkspacePatch threw only because it compares the worktree against one patch's own base_tree in isolation. R49's manifests were never dumped (the volume walk timed out), so its worktree being at the end of its chain is a hypothesis, not a measurement.

The rule

Replay starts after the prefix the worktree already holds. Two conditions are required, and only the first is a hash identity — an earlier revision of this PR claimed both were, and review falsified it:

  1. The worktree's tree equals dependency i's declared result_tree. A tree id is a content hash, so the workspace is identical to that dependency's output over the snapshot the hash covers. Content outside that snapshot is content no patch can carry either, because captureWorkspacePatch diffs the same staged index — so nothing patch-delivered is missed.
  2. The dependencies before i chain into it, each one's result_tree being the next one's base_tree. Without this, "everything before i is materialized" is a claim about topology, not content.

Why condition 2 is not theoretical

Review reproduced the corruption end to end against real git, on a fresh run — no resume required:

setup-foundry-0:  base 32263f5d -> result e1507a29   (writes the scaffold)
setup-foundry-1:  base 32263f5d -> result 32263f5d   (agent changed nothing)
worktree:         32263f5d                            (the pinned baseline)

origin/main : 2 patches applied, loud base-tree mismatch
first revision of this PR : 0 patches applied, NO error, empty workspace

A partial skip is self-validating — the next applyWorkspacePatch re-checks base_tree and throws. A total skip validates nothing, and a sibling fan-in produces exactly that. It is one line of topology away: the strategies group defaults to loops: 3, loop_mode: parallel, and every patch publisher is held to a single copy only by an explicit loops: 1, with nothing validating that a workspace-patch@1 publisher may not fan out.

When the prefix is not a chain, replay starts at 0 and applyWorkspacePatch raises its mismatch exactly as today. Failing the way we already fail is the safe direction; silent garbage findings are far worse than a crash.

Validation no longer depends on applying

validateWorkspacePatchCapture is split out of applyWorkspacePatch and called for every capture, skipped or not. The manifest schema, object ids, patch digest, symlink/submodule rejection, sensitive-path rejection and the base-commit check all used to live behind the apply, so a skipped patch went entirely unvalidated — while the skip decision read result_tree from a manifest nothing had checked was even well formed. A re-pinned target between attempts would also have slipped past a skip.

Scope

Replay path only. Post-agent preparation (replayWorkspacePatches: false) keeps its own rule and is not second-guessed, with a test that discriminates the two.

Tests

13 tests. Five cover the rule; the rest lift materializeWorkspacePatchDependencies out of the template and run it against stubbed collaborators, because the rule alone was not enough — deleting the wiring left the rule tests green.

The stubbed worktree tree advances as patches land, as it does on a real run. A frozen stub had hidden a surviving mutation (hoisting the per-iteration tree read out of the post-agent loop) and had also made me assert a post-agent result of two patches where the real path applies four.

Mutations, each failing at least one test: replayFrom forced to 0; the replay-path guard dropped; chain scanned forward; return matched index instead of the one after; compare base_tree instead of result_tree; the chain-linearity check removed; the validate-all loop removed; the dependency sort comparator neutered; the per-iteration tree read hoisted.

Full runtime suite: 371 tests, 356 pass, 0 fail.

Greptile Summary

The PR makes workspace-patch replay resume after the validated linear dependency prefix already represented by a durable worktree.

  • Extracts capture-only validation so skipped patches remain subject to manifest, digest, object-ID, path-safety, and base-commit checks.
  • Preserves the separate post-agent materialization behavior.
  • Adds focused tests for fresh, resumed, partial, repeated-tree, fan-in, validation, ordering, and post-agent scenarios.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/runtime/src/templates/smithers/workflows/workflow.tsx Adds validated, chain-aware replay-prefix selection while retaining the distinct post-agent preparation path.
packages/runtime/src/workspace-handoff.ts Extracts capture-only validation and keeps worktree-dependent checks inside patch application.
packages/runtime/test/workspace-patch-replay.test.ts Adds focused regression and wiring coverage for replay selection, fan-in safety, validation, ordering, and state advancement.
packages/runtime/test/generated-workflow-verifier.test.ts Verifies that the generated workflow retains the replay guard, chain check, validation pass, and replay slicing.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Read and sort dependency captures] --> B[Validate every capture]
  B --> C{Replay path?}
  C -- No --> D[Use post-agent materialization rule]
  C -- Yes --> E[Capture current worktree tree]
  E --> F[Scan matching result trees backward]
  F --> G{Preceding manifests form a linear chain?}
  G -- No --> H[Replay from dependency zero]
  G -- Yes --> I[Replay after matched prefix]
  H --> J[applyWorkspacePatch validates base tree]
  I --> J
Loading

Reviews (3): Last reviewed commit: "docs(runtime): scope the skipped-capture..." | Re-trigger Greptile

Context used:

aviggiano and others added 3 commits August 7, 2026 05:32
…lready holds

`prepare:stateful-invariant-implement-properties` replays every dependency's
workspace patch. That is correct on a fresh run -- the task worktree starts
at the pinned baseline, so each patch's declared `base_tree` is satisfied in
turn down the chain. A RESUMED run breaks the precondition: the worktree
lives on a durable volume and still holds the previous attempt's state.

Two production runs died on it, at the same node, with the same expected
tree:

  R48  expected 2dd4efef..., got bf324c39...
  R49  expected 2dd4efef..., got b8d46f13...

`2dd4efef` is `setup-foundry`'s pristine base. The chain, dumped from the
durable volume, is linear:

  setup-foundry             2dd4efef -> 4fefa8df   1 file
  base-test-setup           4fefa8df -> 4fefa8df   0 files
  stateful-invariant-setup  4fefa8df -> fd4e3c2c   7 files
  handlers                  fd4e3c2c -> bf324c39   7 files
  coverage                  bf324c39 -> bf324c39   0 files

The worktree sat at the END of that chain. Every dependency's content was
already present, folded in by later links; `applyWorkspacePatch` threw only
because it compares the worktree against one patch's own `base_tree` in
isolation and cannot see the chain that patch belongs to.

Replay now starts after the prefix the worktree already equals. The rule is
sound for one reason: a tree id is a content hash of the WHOLE snapshot, so
equality with a dependency's declared `result_tree` means the workspace is
byte-identical to that dependency's output, and everything at or before it
in replay order is already materialized. That is an identity, not an
inference about intent -- which is what the two earlier attempts lacked. One
relaxed `applyWorkspacePatch` (from inside that function a legitimate fan-in
is indistinguishable from real drift); one consulted recorded provenance (a
task that throws inside the replay loop never persists a preparation record,
so the evidence does not exist when the decision is made). Both were
rejected by review.

It never suppresses an error. A worktree matching no declared output replays
from the start and `applyWorkspacePatch` raises its base-tree mismatch
exactly as before, so genuine drift stays loud instead of quietly producing
a corrupt workspace. Manifests are now read before any patch is applied,
because the decision is about the chain as a whole.

Scoped to the replay path only. Post-agent preparation
(`replayWorkspacePatches: false`) keeps its own long-standing rule and is not
second-guessed; a test discriminates the two, since a worktree at
`4fefa8df` is the declared base of two different dependencies there.

Nine tests, and each of five mutations fails at least one of them: forcing
`replayFrom` to 0, dropping the replay-path guard, scanning the chain
forward instead of backward, returning the matched index instead of the one
after it, and comparing base trees instead of result trees. The rule alone
was not enough -- deleting the wiring left the first five tests green -- so
`materializeWorkspacePatchDependencies` is lifted out of the template and
run against stubbed collaborators too.

Fixes #312.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both reviews of ea0085c returned NO-MERGE on the same defect, and they were
right. My soundness argument had two steps and I only justified the first.

  1. worktree tree == dependency i's result_tree  =>  the workspace equals
     that dependency's output. This IS a hash identity.
  2. => everything before i is already materialized. This is NOT. It holds
     only if the sorted dependency list is a chain, and nothing checked that.

The adversarial reviewer reproduced the consequence end to end against real
git, and it is worse than the bug being fixed -- it corrupts a FRESH run:

  setup-foundry-0:  base 32263f5d -> result e1507a29   (writes the scaffold)
  setup-foundry-1:  base 32263f5d -> result 32263f5d   (agent changed nothing)
  worktree:         32263f5d                            (the pinned baseline)

  origin/main : 2 patches applied, loud base-tree mismatch
  ea0085c     : 0 patches applied, NO error, empty workspace

The backward scan matched the no-op sibling at the last index and skipped the
entire list. A partial skip is self-validating -- the next applyWorkspacePatch
re-checks base_tree and throws -- but a TOTAL skip validates nothing, which is
exactly what a sibling fan-in produces. Reachable in one line of topology: the
`strategies` group defaults to `loops: 3, loop_mode: parallel` and every patch
publisher is held to one copy only by an explicit `loops: 1`, with nothing
validating that a workspace-patch publisher may not fan out.

The prefix is now checked link by link and replay falls back to 0 when it is
not a chain, so we fail the way we already fail. Silent garbage findings are
far worse than a crash.

Also from review:

- "It never suppresses an error" was false as written. The manifest schema,
  object ids, patch digest, symlink/submodule rejection and sensitive-path
  rejection all live inside applyWorkspacePatch, so a skipped patch was never
  validated at all -- and the skip decision reads result_tree, so an unchecked
  field was steering it. Split out validateWorkspacePatchCapture, called for
  EVERY capture regardless of the skip. The base-commit check moved there too:
  a re-pinned target between attempts would otherwise go unnoticed on a skip.
- The R49 half of the causal claim is withdrawn. Only R48's manifests were
  ever dumped; b8d46f13 appears in no manifest anywhere because the R49 volume
  walk timed out. R49 failing at the same node with the same expected tree is
  measured; its worktree being at the end of ITS chain was an inference stated
  as fact, in a comment, which is the specific habit this project has already
  paid for twice.
- "byte-identical" overstated what captureWorkspaceTree measures: it hashes
  the staged snapshot, excluding untracked paths under the runtime and
  generated roots. The decision is still sound -- captureWorkspacePatch stages
  through the same function, so no patch can carry a path outside the hash --
  but the comment now says what is actually compared.
- captureWorkspaceTree no longer runs for tasks with no patch dependencies.
- The claim that a fresh run never matches was too strong; a leading zero-file
  publisher declares base == result and does match.

Tests 9 -> 13. New: the reviewer's fresh-run sibling fixture; a non-chain
fan-in; validation of skipped captures; and replay order, which the sort
alone determines and which nothing pinned -- `.sort(() => 0)` passed the
entire suite before.

The stubbed worktree tree now ADVANCES as patches land, as it does on a real
run. A frozen stub hid a surviving mutation (hoisting the per-iteration tree
read out of the post-agent loop) and had also made me assert a post-agent
result of two patches where the real path applies four.

Full runtime suite: 371 tests, 356 pass, 0 fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The source comment was fixed when review falsified the one-step soundness
argument; this file's header still asserted it. It claimed the worktree
matching a dependency's result_tree meant 'that dependency and every one
before it in replay order are already materialized. That is an identity'
-- which is exactly the step that is NOT an identity, and exactly what a
sibling fan-in breaks on a fresh run.

Fixing the code and leaving the prose is how a file ends up documenting a
claim its own tests disprove.

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

Copy link
Copy Markdown
Collaborator Author

Independent verification of the fresh-run corruption fix

I rebuilt the reported reproduction from scratch — real git repo, real captureWorkspacePatch / captureWorkspaceTree / applyWorkspacePatch, and the production materializeWorkspacePatchDependencies lifted from each revision's own template — and ran the same input against three revisions.

The fixture is the reported one, built rather than asserted: sibling A writes test/recon/Setup.sol, sibling B's agent changes nothing so it publishes a zero-file patch, both declare the same base, and the worktree starts at the pinned baseline — a fresh run, no resume.

A: base cdad1e09 -> result d8be5348 (1 file)
B: base cdad1e09 -> result cdad1e09 (0 files)
revision error test/recon/Setup.sol afterwards verdict
origin/main workspace patch base tree mismatch present safe
ea0085c (first revision) none absent silent corruption
dd9df13 (this head) workspace patch base tree mismatch present safe

Two things this establishes that a passing test alone would not:

  1. The harness can fail. It reproduces the corruption on ea0085c, so the clean result on dd9df13 is evidence rather than an absence of evidence.
  2. The fix restores origin/main's behaviour exactly on this input — sibling A's content is applied and sibling B raises the pre-existing loud mismatch. The change does not trade the crash for a quieter, worse failure; it leaves the non-chain case exactly as loud as it was.

Verification script kept at /tmp/verify312.mjs.

…y checked

Review pointed out that the validator's doc comment implied more coverage
than it delivers. The checks that need only the capture and HEAD moved into
`validateWorkspacePatchCapture`; `assertPatchPathsMatchManifest`, the
empty-patch consistency check and the result-tree verification stayed in
`applyWorkspacePatch`, because they are meaningful only against a worktree
the patch is being applied to.

So a SKIPPED capture is validated less thoroughly than an applied one: its
manifest cannot name a sensitive path, but nothing cross-checks the paths in
its patch BODY against that manifest. Acceptable, because the body is never
applied -- but "sensitive-path rejection" without that qualifier reads as a
guarantee the code does not make.

Also records that validating every capture up front and then applying some of
them double-validates those, and why that is the deliberate trade: the skip
decision reads `result_tree`, so validation has to precede the decision, and
re-running inside `applyWorkspacePatch` keeps that function safe for any
caller. Every check is pure and idempotent.

Full runtime suite: 371 tests, 356 pass, 0 fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aviggiano
aviggiano merged commit b2c3571 into main Aug 7, 2026
9 checks passed
@aviggiano
aviggiano deleted the fix/312-chain-superseded-replay branch August 7, 2026 06:50
aviggiano added a commit that referenced this pull request Aug 7, 2026
…template

Integrates b2c3571, "start dependency replay after the prefix the worktree already
holds" (#314). One conflict, in the generated workflow template's runtime-module
destructure: main added `validateWorkspacePatchCapture` while this branch had added
ten symbols and a postflight type. Resolved as the union.

Because this is exactly the shape of merge that produced issue #212 on this branch --
main hardening a file the branch also edits, and the branch silently dropping it -- I
verified both sides landed rather than trusting the resolution: main's
`validateWorkspacePatchCapture(workspaceRoot, capture)` call and its
`firstDependencyRequiringReplay`/`replayFrom` prefix-skipping are present, and this
branch's `invariantTestRootName`, `repositoryAwareWorkspaceOutputRoots`,
`resolvedWorkspaceOutputRoots` and `preparedWorkspaceOutputRoots` are all still there.

Separately, that verification exposed a defect of my own: the per-attempt pin key I
added embedded a RAW NUL byte in the source rather than an escape sequence:

    return `${workspaceRoot}<0x00>${task.attemptId}`;

A NUL separator is the right choice -- it cannot occur in a path or an attempt id, so
the key is unambiguous -- but writing it as a literal byte makes the file binary to
grep and similar tools, which silently return NOTHING instead of matching. That misled
my own verification of this very merge: greps for both main's and the branch's symbols
came back empty and looked like evidence of loss. Git still rendered diffs as text
only because its binary heuristic scans the first 8KB and the byte sits at ~160KB, so
review was not blinded, but that is luck rather than design. Now written as `\0`,
which produces the identical runtime string with an all-ASCII source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aviggiano added a commit that referenced this pull request Aug 7, 2026
…ce it rewrote (#316)

* fix(runtime): let a later ancestor supersede the invariant-suite source it rewrote

R50 died at `prepare:stateful-invariant-implement-properties` with 25 nodes
succeeded and zero failed -- the cleanest run of this investigation, one node
from the Recon campaign no run has ever entered:

  artifact handoff ancestor invariant suite sources conflict for
  tests/recon/Properties.sol:
    .../artifacts/stateful-invariant-handlers vs
    .../artifacts/stateful-invariant-setup

It is not a conflict. `stateful-invariant-handlers` depends on
`stateful-invariant-setup` (handlers <- setup <- property-specification-fanin
in the shipped topology), runs after it, and legitimately rewrites
Properties.sol. One ancestor superseded the other.

The selection could not see that. `orderedInvariantSuiteDependencies` sorts by
directness and then ALPHABETICALLY, and `implement-properties` depends
directly only on `stateful-invariant-coverage`, so both of these are indirect
and the tie-break is `localeCompare` -- under which `handlers` sorts BEFORE
`setup`, the reverse of causal order. Sort position is not causality, and the
directness tie-break supplies no ordering at all in the case that arises.

This is the same blind spot as #312 one subsystem over. There
`applyWorkspacePatch` read a SUPERSEDED dependency patch as drift because it
compared one patch's base_tree in isolation and could not see the chain; here
the handoff reads a SUPERSEDED source as a conflict because it compares two
ancestors' bytes in isolation and cannot see which ran first.

`invariantSuiteAncestorSupersedes` asks the dependency graph whether one
ancestor transitively depends on the other. Reachability is deliberately the
question, not ordering: two ancestors unordered with respect to each other are
a GENUINE conflict and still fail closed. Answering "whichever sorts later
wins" would silently drop a sibling's work, which is exactly what made the
first revision of #314 unmergeable.

Confined to the equal-directness branch, which is the only case that used to
throw unconditionally. A first attempt consulted the graph BEFORE the
directness rule and inverted the precedence a direct predecessor holds over an
indirect ancestor, breaking two #217 tombstone tests -- while the comment
above it asserted that no working selection could change. The suite disproved
the comment on its first run, which is the cheapest place for that to happen.

Nine tests. Six pin the relation directly: the R50 pair, its directionality,
transitivity across three edges, unordered siblings resolving to neither, the
unknown and self-referential cases, and termination on a malformed cyclic
graph -- because a helper that hangs on bad input turns a validation bug into
a run that never fails and never finishes.

Three more pin the WIRING through the real materializer, because the helper
alone proves nothing: R50's exact shape, where setup and handlers are BOTH
indirect ancestors of implement-properties. The existing #217 fixtures never
reach that branch -- `coverage` has handlers direct and setup indirect -- so
the whole suite passed while the new code was unreachable.

Mutations, each failing at least one test: reverting to the unconditional
throw; deleting the throw; swapping the supersedes arguments; the helper
always false; always true; direct edges only. Removing the visited set does
not fail a test -- it HANGS, which is what the cyclic test is named for.

Full runtime suite: 379 tests, 364 pass, 0 fail.

Fixes #315.

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

* docs(runtime): record the two limitations review measured in the ancestor rule

Neither is fixed here, and both are written down rather than left to be
rediscovered.

1. The caller folds over ancestors in sort order, so on a FAN-IN MERGE shape
   the outcome depends on node naming: unordered siblings L and N plus a merge
   node M depending on both resolve cleanly when M is visited first and throw
   when it is visited last, decided by localeCompare on directory names. The
   failing direction is fail-closed and identical to the behaviour before this
   change, and the shipped topology is a pure chain, so it does not arise
   today. Making it order-independent means reducing each path's publishers to
   their maximal elements before comparing -- a restructure of the fold, not a
   guard inside it, and filed as #317 rather than bolted on here. This same
   area has already produced one fix that had to be redone after review found
   it silently corrupted a fresh run.

2. Attempt ids are stable across re-runs, so an OUT-OF-ORDER re-run loses
   loudness: retrying stateful-invariant-setup after stateful-invariant-handlers
   has already succeeded leaves setup's bytes newer in wall-clock time while
   supersedes(handlers, setup) is still true, so they are silently discarded
   where the old code raised a conflict. That is the deliberate trade --
   always throwing is what killed R50 -- but it is a real loss of loudness.

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

* fix(runtime): resolve invariant-suite publishers per path, not pairwise in visit order

Adversarial review found the previous revision's headline test could not fail,
and that the fix had introduced a silent-selection arm. Both are real, and the
second changed what "defer the restructure" was worth.

The masked test. Its fixture had `coverage` -- a DIRECT dependency --
republish handlers' bytes, so the last comparison hit the different-directness
path where a direct dependency wins unconditionally, and `coverage` decided
the assertion. Inverting the two `invariantSuiteAncestorSupersedes` calls,
which exactly reverses supersession and selects the stale ancestor's Solidity,
passed all 384 tests. `coverage` now publishes nothing and the inversion fails
two tests.

The silent arm. The fold compared only against the LAST writer, so any
publisher whose entry was replaced -- including replacement by IDENTICAL bytes
-- was forgotten and never reachability-checked. With unordered siblings `sib`
and `mid` and a descendant of `mid` publishing different bytes, sorting `sib`
first selected silently while sorting `mid` first threw. Same graph, opposite
outcomes, decided by localeCompare. Before this change both orderings threw,
so the silent arm was mine.

Publishers are now collected per path and resolved as a SET, which removes the
order dependence rather than patching one symptom of it:

  1. drop every publisher another publisher transitively depends on;
  2. among what remains, a DIRECT dependency still outranks an indirect one;
  3. if the survivors still disagree, throw -- in every visit order.

Doing reachability BEFORE directness also fixes a case review found where a
stale DIRECT ancestor outranked the INDIRECT descendant that rewrote it,
silently selecting older sources. That was unreachable in the shipped topology
only by luck: `property-specification-fanin` is a direct dependency of
`stateful-invariant-coverage` and an ancestor of the indirect
`stateful-invariant-setup` -- exactly that shape -- and is harmless solely
because it publishes no suite sources.

This is the restructure #317 asked for, so it closes that too. I had deferred
it as a follow-up on the grounds that the non-confluence was fail-closed and
absent from the shipped topology. The silent arm falsified the first half of
that reasoning, and deferring a limitation I had introduced myself is not the
same as deferring one I inherited.

Every fixture published exactly one file per dependency, so `continue` could
become `break` and drop every later path unnoticed. The R50 fixture now
publishes two.

Mutations, each failing at least one test: supersession inverted (4);
the maximal filter removed (2); the conflict throw removed (2); the directness
preference removed (1); the tombstone skip removed (1).

Full runtime suite: 382 tests, 367 pass, 0 fail.

Closes #317.

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

* fix(runtime): never let an empty maximal set drop an invariant-suite path silently

Found by asking the question I keep failing to ask of my own code: can the
guard clause I wrote for safety be reached, and what does it do if it is?

`if (first === undefined) continue;` was written as an impossible-case guard.
It is reachable. A CYCLE in the dependency graph leaves every publisher
superseded by another, so the unsuperseded set is empty, `preferred` is empty,
and the path is dropped from the harness with no error whatsoever -- the exact
silent-loss class this whole change exists to remove, reintroduced by the
defensive line meant to prevent it.

An empty unsuperseded set now falls back to the full publisher set, which
hands the decision to the conflict check and fails closed. The topology
validator rejects cycles so this should be unreachable, but "unreachable" is a
property of today's validator, not of this function -- the same reasoning that
put a bounds guard in `truncateUtf8`.

Removing the fallback fails the new test.

Full runtime suite: 383 tests, 368 pass, 0 fail.

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

* docs(runtime): scope the fail-closed claim to where it actually holds

Review found the comment on the conflict throw asserts a blanket guarantee the
code does not provide: "genuinely unordered publishers disagreeing about the
same file ... stays fail-closed in every visit order."

It is fail-closed WITHIN `preferred`, and not before it. The directness filter
immediately above can already have dropped an unordered INDIRECT publisher
whose bytes differ, silently, in favour of a direct one. That arm is
pre-existing behaviour the #217 tombstone tests depend on and is deliberately
preserved -- but it is not fail-closed, and the comment claimed otherwise.

The behaviour itself is unchanged and already pinned by "#315 a DIRECT
publisher still outranks an unordered indirect one", which uses differing
bytes precisely so the silent resolution is visible rather than incidental.

This is the same defect as several before it in this branch: prose asserting a
property one line above code that contradicts it. It is worth noting that the
reviewer found it by running the algorithm over every permutation of ten graph
shapes rather than by reading the comment -- the claim was only checkable
because the behaviour was executable.

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

* fix(runtime): detect same-directness conflicts before preferring a direct publisher

Both reviews of the restructure independently found the same latent defect,
and it is a strict loss of fail-closed behaviour versus main rather than the
stale comment I had treated it as one commit earlier.

Filtering to the direct publishers BEFORE checking for disagreement hides a
conflict between the ones discarded. Review's probe, run against three
revisions: `root` fans out to unordered `i1`, `i2` and `zd`, all publishing
the same path with different bytes, and only `zd` is direct.

  main      throws  conflict ... i1 vs i2
  rev 1     throws  conflict ... i1 vs i2
  rev 2     NO ERROR, selects zd's bytes

Two unordered ancestors disagreeing about the same Solidity file is precisely
the case this change is about, and I had made it silent.

main's pairwise rule is "same directness disagreeing throws, otherwise the
direct publisher wins" -- and main reaches that outcome only for some arrival
orders, because whichever publisher lands in the selection first decides what
the next one is compared against. Checking each directness GROUP for internal
disagreement makes it the outcome for every order, which is the property the
restructure exists to provide, applied to the rule main already had rather
than to a rule I invented.

The previous commit corrected the comment to say the guarantee held only
within `preferred`. That was accurate but was the wrong remedy: the honest
description of a regression is not a substitute for not having one. Reverting
the comment is unnecessary -- it now describes code that no longer needs the
caveat -- but the caveat is gone because the behaviour changed, not because
the sentence did.

Five mutations, each failing at least one test: the group conflict check
removed (4); only the direct group checked (4); supersession inverted (4); the
directness preference removed (1); the maximal filter removed (2).

Full runtime suite: 384 tests, 369 pass, 0 fail.

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

---------

Co-authored-by: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

applyWorkspacePatch base tree mismatch blocks stateful-invariant-implement-properties, with maxAttempts=1 so it kills the run

1 participant