Repository navigation
fix(runtime): give the invariant-suite handoff a deletion channel and durable, supersedable provenance - #287
Merged
Conversation
…urable The invariant chain is linear but `dependencyArtifactDirs` is the full transitive ancestor closure, so every stage sees every earlier stage's artifact. The published manifest had no deletion channel and tombstones never left process memory, so a harness file deleted at one stage was re-selected from an earlier ancestor and re-copied into coverage, implement-properties and the campaign workspace Recon fuzzes. A resurrected duplicate breaks the suite build; a resurrected stale Properties or handler file gets fuzzed instead of the intended one. Represent deletions in the manifest as a typed, additive `tombstones` field, accumulate them down the chain, and suppress a tombstoned path during dependency selection unless a declared predecessor still carries it, so a deleted-then-re-added source stays alive. Also tombstone an emptied test-tree source on the Git fallback discovery path, which previously skipped it without recording the deletion. The selection, tombstone and publication state were plain module-level Maps with no persistence. `verifyArtifacts` is its own retries:0 node, so any restart between an invariant agent task finishing and its verifier running reached an empty map and hard-threw "invariant suite publication snapshot is unavailable", killing the run at a stateful-invariant stage. On a post-restart post-agent preparation pass the rebuild also re-derived a fresh selection and copied ancestor bytes over the harness the stage had just authored. Persist the dependency handoff as a schema-validated record under `<runRoot>/invariant-suite-handoffs/<attemptId>/handoff.json` carrying attempt, producer, path, size, digest, directness and tombstones, re-verify every recorded byte before materialization, and fail closed on mismatch. Recovery now reads the durable manifest rather than a module-level Map, and an absent record is a diagnosable typed failure naming the missing file instead of an unconditional throw. Closes #217 Closes #219 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remediates the three independent adversarial reviews of the #217/#219 change. Five findings were major. The durable handoff record now binds itself to a fingerprint of every ancestor invariant-suite manifest it was derived from. A legitimately re-executed ancestor (operator retry-task/timetravel, or a new Modal execution generation republishing the directory) therefore supersedes the record and materialization re-derives, instead of throwing `invariant suite dependency changed` forever on a retries=0 preparation node. A byte mismatch against a byte-identical ancestor manifest still fails closed, which is the actual tampering case. Materialization no longer trusts a durable record blindly. It re-checks the ancestors' implemented-property expectations on the recovered path, and reconciles the workspace from the record when — and only when — no durable workspace snapshot exists yet, per missing path. That repopulates a worktree lost inside the write-record-then-capture-snapshot window without ever overwriting a source the attempt authored or resurrecting one it deleted. `resetTaskArtifactsForRetry` drops the attempt's in-process tombstones. The restored workspace snapshot puts a deleted source physically back, so a stale tombstone would otherwise be published as a durable, chain-wide deletion that every descendant honours. The record's `tombstones` field is now consumed rather than merely written: it is the durable statement of what the handoff suppressed, and `materializeInvariantSuiteCompanions` prefers it over recomputing from the ancestor manifests. The handoff-record diagnostic path is computed without touching the filesystem, so "the record is unavailable, here is the file" can no longer be replaced by an unrelated mkdir failure. Tests: the suite grows from 7 to 15 cases. 13 of 15 fail against the pre-#217/#219 template. Every one of the 10 safety-critical clauses was mutated in turn and each mutation killed exactly one test, including the two cases that are green on the pristine tree — the over-suppression guard the review correctly identified as vacuous now goes red when the declared-predecessor rule is removed, and the git-fallback emptied-source tombstone is driven against a real temporary git repository. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines
+3019
to
+3029
| const previousSnapshot = invariantSuiteDependencySnapshots.get(task.attemptId); | ||
| if (previousSnapshot !== undefined) { | ||
| for (const [relativePath, entry] of previousSnapshot) { | ||
| const sourceRoot = path.join(realpathSync(entry.dependency), "invariant-suite"); | ||
| const current = readInvariantSuiteSourceBytes(sourceRoot, relativePath, "artifact handoff invariant suite"); | ||
| if (!current.equals(entry.bytes)) { | ||
| throw new Error(`artifact-contract failure: invariant suite dependency changed ${relativePath}`); | ||
| } | ||
| } | ||
| reconcileInvariantSuiteWorkspace(task, workspaceRoot, previousSnapshot, new Set()); | ||
| return; |
There was a problem hiding this comment.
In-memory snapshot bypasses supersession
When an upstream invariant stage republishes during a same-process retry or timetravel, this branch reuses the retained dependency snapshot without comparing manifest fingerprints. Changed bytes terminate downstream preparation, while added paths or tombstones are silently missed, leaving the downstream suite stale or resurrecting a deleted source.
Knowledge Base Used: Runtime: Run Orchestration Lifecycle
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runtime/src/templates/smithers/workflows/workflow.tsx
Line: 3019-3029
Comment:
**In-memory snapshot bypasses supersession**
When an upstream invariant stage republishes during a same-process retry or timetravel, this branch reuses the retained dependency snapshot without comparing manifest fingerprints. Changed bytes terminate downstream preparation, while added paths or tombstones are silently missed, leaving the downstream suite stale or resurrecting a deleted source.
**Knowledge Base Used:** [Runtime: Run Orchestration Lifecycle](https://app.greptile.com/monad-foudnation/-/custom-context/knowledge-base/monad-developers/ultrafuzz/-/docs/runtime.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
aviggiano
added a commit
that referenced
this pull request
Aug 7, 2026
Integrates origin/main (fa8be64): PR #287's invariant-suite handoff deletion channel and durable provenance, plus #282, #284, #290, #294, #296, #298, #300, #303, #305 and #308. CHANGELOG keeps both Unreleased sections; the modal runner test import list keeps both new symbols. Fixes the review finding on #165 (issue #212). Compiled task specs can only name singular `test/...` output roots, because they are built before the worktree exists. Targets are not consistent: the Aave v4 target at 6959e321 has no `test/` at all, only plural `tests/`. The generated workflow created the singular tree regardless, so such a repository grew empty `test/recon`, `test/chimera` and `test/invariants` directories, which setup.md tells the model to read as the repository's convention, and retry cleanup then scrubbed that invented tree while leaving the real `tests/` output behind. Workspace output roots are now re-anchored at execution time onto the test root the repository actually uses, restoring the semantics of main's `invariantTestRoots` helper that this branch dropped in merge 2bbecba, at the layer this branch's retry preparation uses. Three review findings on the first cut of that fix are addressed here: - Exactly ONE root is selected, never the union of those present. Anchoring under every existing root would have recreated the same defect from the other side: once an ancestor stage introduces the other root, each invariant stage would create an empty duplicate tree beside the real one. Preference follows INVARIANT_TEST_ROOT_NAMES, so a repository that has `test/` behaves exactly as it did before this helper existed and only a repository without it is redirected. - The resolution is pinned per attempt at preparation time. Rediscovering per call was unsafe: prepareTaskWorkspaceOutputRoots finishes by replaying ancestor workspace patches, and an ancestor stage can introduce the other test root as a side effect, so a second independent discovery could select a root whose subdirectories were never created. taskWorkspaceOutputRoots would then lstat a missing path and throw a bare ENOENT instead of the typed artifact-contract failure, killing a retries:0 node on exactly the restart and resume paths this machinery exists to survive. - Only ENOENT and ENOTDIR mean "this root is absent". Swallowing EACCES or ELOOP would silently fall back to the singular default against a root that does exist. Test changes, also from review: - The non-directory case was vacuous: with only a regular file named `test` present, "rejected it" and "fell back to the default" both read `test`. It now runs against a repository that also has a real `tests/`, so the outputs differ; verified by mutation (dropping the isDirectory() guard fails the test). - taskWorkspaceOutputRoots' use of the helper is now pinned in the generated-source assertions. Reverting only that call site previously left every test green while every task on a `tests/`-only target died on a bare ENOENT. - Adds coverage for the pinning behaviour, for per-attempt isolation of the pin, and for surfacing an unreadable candidate root. Also repairs a merge-integration gap the new #217 harness could not have anticipated: it lifts resetTaskArtifactsForRetry, which on this branch calls prepareAnchoredDirectory (branch-only, absent from main), so the harness now supplies that collaborator and drops the dead invariantTestRoots stub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Two defects in the generated Smithers workflow template, in the same mechanism.
#217 — an ancestor resurrects a source a later stage deleted.
artifactAncestorNodeIds(packages/runtime/src/smithers.ts) walksdependsOntransitively, so every invariant stage'sdependencyArtifactDirsis the full transitive ancestor closure, not just its declared parents.materializeInvariantSuiteFromDependenciesiterated all of those dirs and selected every suite path present. The published manifest carried onlyschema_version,producer_node_id,producer_attempt_idandfiles— there was no deletion channel.recordInvariantSuiteTombstoneonly mutated a module-levelMapkeyed by the deleting stage's workspace root, so a descendant's lookup was always empty for a path a predecessor deleted. Net effect:handlerscorrectly omits a deleted file from its own artifact, andcoveragere-selects it straight out of thesetupartifact and copies it back into the workspace.#219 — a restart between an invariant agent task and its verifier killed the run, or silently reverted it.
invariantSuiteTombstones,invariantSuiteDependencySnapshotsandinvariantSuitePublicationSnapshotswere plain module-levelMaps with no persistence anywhere. The verifier is its own<Task retries={0}>depending on the agent task, while publication and the firstverifyArtifactsrun inside the agent-generate wrapper. So once the agent node is durably complete, a restart runs only the verifier, andrememberInvariantSuitePublicationshard-threwinvariant suite publication snapshot is unavailableon the emptyMap. Withretries: 0that killed the run — the R31/R43 signature. Symmetrically, with the in-process snapshot lost, the post-agent preparation pass fell through to a fresh selection and re-copied ancestor bytes over the harness the agent had just authored.A correction worth recording: the "mutated ancestor bytes" framing in the original triage is wrong.
assertTaskInputs→assertVerifiedDependency→rememberExpectedInvariantSuitePublicationsalready validates every ancestor manifest, digest-checks every ancestor suite file and cross-checks the verification marker under<runRoot>/.ultrafuzz-verification/on every preparation. Ancestor byte integrity was never the gap, and this PR does not touch that path.The fix
#217 — a typed, additive deletion channel. The published manifest gains
tombstones, validated per entry throughassertSafeInvariantSuitePathand bounded byassertInvariantSuiteTombstoneBudget. It is an additive v1 field, so manifests already on disk from in-flight runs stay readable (absent = empty set).inheritedInvariantSuiteTombstonesreads the manifests of the task's declared dependencies and returns(union of their tombstones) minus (union of their files); a tombstoned path is skipped during selection, so it is neither republished to this stage's artifact nor copied into the workspace, and the effective set is republished so the channel accumulates down the chain.Restricting tombstone authority to declared predecessors is deliberate: it is exactly right for the linear invariant chain in
.ultrafuzz/topology.yml, and theminus filesterm is what keeps a deleted-then-re-added source alive.dependencyArtifactDirshas no reliable topological order, so an ordering-sensitive rule was rejected.#219 — durable handoff provenance that can be superseded. The exact materialized handoff is recorded at
<runRoot>/invariant-suite-handoffs/<attemptId>/handoff.json(attempt, producer, path, size, digest, directness, tombstones, and a fingerprint of every ancestor invariant-suite manifest it was derived from), written after the copy loop.loadInvariantSuiteDependencyHandoffrebuilds it, re-reads each recorded byte from the ancestor artifact and fails closed on mismatch, so a post-restart pass keeps the handoff it used instead of reverting authored sources.rememberInvariantSuitePublicationsrebuilds its expectation from the durable manifest the attempt already wrote rather than hard-throwing, andresolveInvariantSuiteDependencySnapshottreats an empty selection as the legitimate recorded outcome it is for the first invariant stage.Per the design constraint: every recovery path reads disk. No recovery path reads an in-process
Map(the PR #278 mistake), and the empty case is a diagnosable typed failure naming the missing file rather than an unconditional throw.What the review round changed
The first three findings below were major and are the reason this PR has a second commit.
The record can now go stale, and staleness is not tampering. Two reviewers independently reproduced the same terminal path:
setuppublishes v1,handlersrecords its handoff, an operator legitimately re-executessetup(retry-task/timetravel/ a new Modal execution generation callingreplacePublishedDirectory) and it republishes v2 with a fresh, internally consistent manifest and verification marker.assertVerifiedDependencypasses. Then the stale record's digest check threwartifact-contract failure: invariant suite dependency changed …— from aretries={0}preparation node, on every subsequent resume, because the record is durable andattemptId === stableBaseAttemptId(node.id)is stable for the life of the run root. Onmainthe same situation self-healed by re-deriving. That is precisely the "converts a stall into TERMINAL loss" shape this PR exists to remove, reached from disk instead of from aMap, and the operator saw the tamper message for a legitimate upstream retry.The record now binds itself to
producers: [{attempt_id, manifest_sha256}]. A fingerprint mismatch means "this record describes a superseded handoff": it is discarded and materialization re-derives against the ancestor's new, already-verified bytes. The hard failure is retained for exactly the tampering case — ancestor manifest byte-identical, recorded suite bytes not. Ancestor byte integrity is already guaranteed byassertVerifiedDependency, so failing closed here was buying no integrity.The early return no longer trusts that the copies are still on disk.
returning as soon as a record loaded rested on "a record implies the copies landed", which is an assumption about worktree lifetime, not a checked invariant —loadInvariantSuiteDependencyHandoffverifies ancestor bytes and never looks at the workspace. If the process died between writing the record andcaptureInvariantSuiteWorkspaceSnapshot, and the worktree was then reclaimed or reconstructed, resume found no snapshot to restore, returned without copying, and the stage's agent ran against a workspace with no inherited harness and no error. Worse,captureInvariantSuiteWorkspaceSnapshotis write-once, so the empty workspace was immediately persisted as the authoritative snapshot and every later retry restored the loss.reconcileInvariantSuiteWorkspacenow repopulates from the record — gated on the durable workspace snapshot being absent (the only window in which nothing else can restore the worktree) and on each individual path being absent (so the post-agent pass never overwrites a source this attempt authored and never resurrects one it deleted). The early return also no longer skips the ancestors' implemented-property expectation check: a record binds the suite bytes, not the ancestors' ledgers, so that fail-closed check is re-run on the recovered path too.A retry no longer publishes the previous attempt's deletions.
invariantSuiteTombstonesis keyed byrealpathSync(workspaceRoot)and was never cleared.resetTaskArtifactsForRetryrestores the durable workspace snapshot — putting a file the attempt-1 agent deleted physically back — but left the tombstone. On attempt 2 the file is unchanged, so it is not re-discovered, yet the manifest still carried it as a tombstone, andimplement-propertiesandcampaignthen suppressed it permanently: the campaign workspace Recon fuzzes loses a harness file, the suite fails to build, and it reads as model quality. Before this PR the same stale drop was accidentally healed by the #217 resurrection; the fix removed the healing while leaving the staleness.resetTaskArtifactsForRetrynow drops the attempt's tombstone set. Nothing is lost: tombstones are fully re-derived from the protected baseline on every companions pass. (Precondition:max_attempts > 1, which the shipped topology leaves at 1 butbenchmarks/smoke-benchmark.ymlandpackages/topology/src/expand.tssupport — latent, not live.)The record's
tombstonesare now consumed, not merely written. All three reviewers noted the field was parsed, path-checked and budget-checked and then discarded by every caller, advertising durability the code did not provide.materializeInvariantSuiteCompanionsnow prefers the record's deletion channel over recomputing from the ancestor manifests: the record is the durable statement of what the handoff actually suppressed, and recomputation is the fallback for an absent or superseded record, where re-derivation is the correct answer anyway.The "record is unavailable" diagnostic is side-effect free. It formatted its path by calling a helper that
mkdirSyncs two directories and can throwunsafe invariant workspace snapshot root …— so in exactly the run-root states where diagnosability matters most, the operator got an unrelated path-safety error instead of "the record is missing, here is the file". The message now uses a pure path computation.Regression tests and red/green evidence
packages/runtime/test/invariant-suite-handoff-durability.test.ts, 15 cases. All 15 pass on this branch. The tests drive the extracted template helpers with injected collaborators against a real filesystem, becauseworkflow.tsxis a generated template with a__ULTRAFUZZ_TASK_SPECS__substitution point and cannot be imported directly.Against the pre-#217/#219 template (
77da78a): 13 of 15 fail.Failure 5 is the exact
invariant suite publication snapshot is unavailablemessage that killed R31/R43. Failure 7 is the silent revert. Failure 1 is the #217 resurrection.Cases 2 and 10 are green on the pristine tree, and that is expected, not a gap. Case 2 asserts a live source is not suppressed — on a tree with no deletion channel there is nothing to suppress. Case 10 asserts a lost worktree is repopulated — on a tree that always re-derives, it always was. Both pin rules that only exist here, so both are proved by mutation instead.
Every safety-critical clause was mutated in turn, and each mutation killed exactly one test — no mutation was silent, and no mutation killed a second test:
#217 … not resurrected from an indirect ancestorpresent-path subtraction removed frominheritedInvariantSuiteTombstones#217 a declared predecessor that still carries a tombstoned path outranks the tombstone#217 a path this stage deletes and re-creates in the same attempt …#219 a legitimately republished ancestor supersedes the record …#219 a worktree lost before the durable workspace snapshot exists …#219 the record never resurrects a source the agent deleted …#219 the recovered path still fails closed on an ancestor property …#219 the durable record's tombstones drive the deletion channel …resetTaskArtifactsForRetrytombstone clear removed#217 a retry drops the previous attempt's tombstones …#217 an emptied test-tree source is tombstoned on both discovery pathsThe first review round was right that the over-suppression guard was vacuous as originally written: it put the tombstone on
setup's manifest, butsetupis an indirect ancestor ofcoverage, soinheritedInvariantSuiteTombstonesskipped it at thedirectDependenciesfilter and thepresentsubtraction was never reached — deleting either rule left the suite green. The tombstone now sits onhandlers, the direct dependency, which is the only shape that reaches the subtraction, and the mutation table above is the proof. The emptied-source tombstone is now driven against a real temporary git repository, which is the one path that needs one.Review findings
14 actionable findings across three lenses (correctness-regression, blast-radius-compatibility, durability-crash-restart); 5 major. Several were raised by more than one reviewer.
Addressed in code (13):
retries=0node → ancestor manifest fingerprints; the record is superseded, not treated as tampering.reconcileInvariantSuiteWorkspace, gated on snapshot absence and per-path absence.resetTaskArtifactsForRetry.handoff.jsontombstoneswritten and validated but never read back → now consumed bymaterializeInvariantSuiteCompanions.mkdirs and can throw a different error → pure path computation.Accepted as a documentation correction rather than a code change (1):
changedTestTreePathshas exactly one caller,materializeInvariantSuiteCompanions, which always passesinvariantSuiteProtectedBaselinePath(task);captureInvariantSuiteBaselinewrites that file on every preparation and it lives outside the model-controlled workspace, so the baseline branch always wins and the Git-fallback block is not reached for the five invariant nodes. The truncation case was already handled on the baseline branch before this PR. The line is kept as defence-in-depth for a caller that has no baseline, and it now has coverage, but it closes no gap that was open. Corrected here rather than by deleting the line, because one reviewer explicitly offered that resolution and the branch is otherwise silently wrong if it is ever reached.Rejected: none.
Residual risk
workflow.tsxis covered by neitherpnpm -w typecheck(packages/runtime/tsconfig.jsonincludes onlysrc/**/*.ts) norpnpm -w lint(the glob is.ts/.js/.mjs, not.tsx). Compensated by a standalonetsc --noEmit --strict --jsx react-jsx --types nodeover the pristine, first-commit and final versions of the template: all three produce a byte-identical 51-error profile, i.e. zero new type errors from either commit. The template's real type surface is still only fully bound at run time once__ULTRAFUZZ_TASK_SPECS__is substituted.handoff.jsonhas noproducersarray and is discarded as unbindable, which re-derives (safe) rather than failing.loadInvariantSuiteDependencyHandoffnow re-digests the ancestor manifests on each call and is called from more than one site per pass. Bounded by the existing 512-file / 64 MiB suite budgets, but it is more I/O per preparation than before.inheritedInvariantSuiteTombstonesthrows rather than skipping on a structurally invalid direct-dependency manifest.assertVerifiedDependencyfails closed on the same manifest earlier inprepareArtifactMirror, so this should be unreachable — but it is a stricter posture than the oldcontinue.retries: 0verifier and the separate preparation node were verified in source.typescriptto erase type annotations from the extracted helpers. It is a root devDependency, resolving via the rootnode_modulesexactly astsc,prettier,eslintand@types/nodealready do for this package. A change to the install layout would break the test harness, not the product code.Validation
Ran from
/home/ubuntu/uf-fix-217-219;df -h /checked before every heavy gate (166–176G free on/, 66% used).pnpm -w run ci→ exit 0 (format:check"All matched files use Prettier code style!",lintclean,buildacross all 13 projects,validate:releasewrote its report).pnpm -w typecheck→ exit 0, all 13 workspace projects.git diff --check→ clean.77da78a).node scripts/run-tests.mjs, all 14 files) → 306 tests, 0 fail, 15 skipped.packages/runtime/test/generated-workflow-verifier.test.ts→ green. This matters: that suite extracts template helpers by source-text offsets, so all new code was placed outside its slice ranges.Merge order
This PR should merge before #272 (
fix/prepare-verify-wrapper-failure-surface). Both are runtime fixes; #272 is the narrower change and touchespackages/runtime/src/workflow-sync.tsrather than the template, so it rebases cleanly onto this.Fixes #217
Fixes #219
Greptile Summary
The PR adds a durable deletion channel and disk-backed provenance for invariant-suite handoffs, including restart recovery and supersession when ancestors republish. One same-process retry path still bypasses supersession.
Confidence Score: 4/5
The PR should not merge until same-process retries revalidate ancestor manifest fingerprints and re-derive superseded dependency handoffs.
A retained module-level dependency snapshot causes legitimate upstream retries to either terminate downstream preparation when bytes change or silently omit new paths and tombstones, bypassing the recovery behavior this change introduces.
Files Needing Attention: packages/runtime/src/templates/smithers/workflows/workflow.tsx
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Materialize dependency suite] --> B{In-memory snapshot exists?} B -- Yes --> C[Check only previously selected bytes] C --> D[Return without fingerprint comparison] B -- No --> E[Load durable handoff] E --> F{Ancestor manifest fingerprints match?} F -- Yes --> G[Reuse recorded handoff] F -- No --> H[Re-derive selection and tombstones] D -. Upstream retry is missed .-> I[Stale suite or terminal mismatch]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(runtime): make the durable invariant..." | Re-trigger Greptile
Context used: