enablement: make the recipe's replay sufficiency fail closed on what a consumer actually receives - #1414
Open
zoroyihan7 wants to merge 33 commits into
Open
enablement: make the recipe's replay sufficiency fail closed on what a consumer actually receives#1414zoroyihan7 wants to merge 33 commits into
zoroyihan7 wants to merge 33 commits into
Conversation
CI E2E report — ❌ Timeout
|
zoroyihan7
force-pushed
the
feature/r1-recipe-impl
branch
from
September 8, 2026 15:48
34771dd to
2b560b9
Compare
zoroyihan7
added a commit
that referenced
this pull request
Sep 14, 2026
PR #1414 is titled "make the recipe's replay sufficiency fail closed on what a consumer actually receives", and its B43 section describes the rule in detail. The rule is not in the branch. Head shipped the exact fail-open the PR claims to close: a recipe whose referenced bytes never reached the bundle still reported ``status: "sufficient"`` -- the one claim an independent consumer cannot check for itself. It was not lost to a merge. ``delivered_paths`` arrived with 0f38ecd, was renamed and strengthened to ``(path, sha256)`` pairs by 2facf08, and was deleted whole by 1a3f17f, whose one-line subject mentions none of it. 9358a52 closed a different fail-open -- a multi-round stack judged over one round -- and left this one open. Ported back from 08f10b7, the last commit carrying it, in the stronger pair form: * ``_payload_references`` names the bytes behind the recipe's manifests and digests; ``referenced_payloads`` reduces them to what a delivery is asked about; ``_delivery_reasons`` refuses a bundle that does not carry them. * ``session_package.deliverable`` answers per candidate: matched by the curated selection, held as a regular file inside the session, within the cap on its own, and still hashing to any digest its recorder took. * The collector asks the packager and hands the answer to the verdict, so neither side restates the other's rules. ``None`` when the recipe references nothing, which is the one case with nothing to deliver. No adaptation was needed for the reason shape: head's ``{code, blocks, scope}`` and both reason codes are unchanged. The package glob and the capture-overlay clearing were never lost and are untouched. The setup-ledger identity producers 1a3f17f removed stay removed; the two PORT NOTE comments mark the reads that are inert until a producer records ``config_digest`` or a step's ``input_identity`` again. Judging stays per payload. 2facf08 moved it off the cumulative budget because spending it in selection order refused a recipe over content it does not name, and two tests now hold that boundary open: an unrelated file sorted ahead does not refuse a referenced payload, while a payload larger than the cap on its own still does. What a truncated bundle actually dropped remains the packager manifest's to report. ``test_a_closed_lane_carries_a_replay_verdict`` began failing honestly: it declared a snapshot manifest and never wrote the captured bytes, which is the state this rule exists to refuse. Its fixture now writes them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| def test_patching_the_client_is_idempotent(tmp_path): | ||
| from hyperloom.orchestrator.actions.executors import _magpie_patcher as mp | ||
|
|
||
| _applied, text = _patch_client(tmp_path) |
| def test_patching_the_client_is_idempotent(tmp_path): | ||
| from hyperloom.orchestrator.actions.executors import _magpie_patcher as mp | ||
|
|
||
| _applied, text = _patch_client(tmp_path) |
zoroyihan7
force-pushed
the
feature/r1-recipe-impl
branch
2 times, most recently
from
September 15, 2026 07:35
5d1cf77 to
df7ba3e
Compare
Comment on lines
+1165
to
+1166
| "(EngineCore pid=2) ERROR RuntimeError: Engine core initialization failed. " | ||
| "See root cause above. Failed core proc(s): {}", |
Comment on lines
+1165
to
+1166
| "(EngineCore pid=2) ERROR RuntimeError: Engine core initialization failed. " | ||
| "See root cause above. Failed core proc(s): {}", |
An enablement session's durable state said WHAT was kept and nothing about how to replay it. This is the machinery that turns a session into a recipe a consumer can act on, a verdict on whether it may, and the delivery that carries it. recipe_steps projects the state onto an ordered array -- setup, build, patch -- each step carrying the root it applies to, the targets its own diff declares, and the identity of what an install consumed. The projection reads durable state only: it runs long after the session that wrote it, so a step that cannot name its own inputs is a step nothing can replay. replay_sufficiency judges that array against a closed vocabulary of reasons, each naming what it blocks -- replay, assertion validation, or both. The rules refuse rather than assume: a patch whose targets nobody recorded, a build nothing replayed, a root with no base commit, a credential the recipe cannot supply. None stays distinct from empty throughout, because "no producer observed this" and "this observation came back clean" certify opposite things. The KEEP records what those two need: per-root identity and base commit taken before the round's first mutation, byte-exact snapshots of every declared target, the environment closure and installed versions read through the interpreter the accepted bench launched, and an append-only ledger of the setup commands as they ran. Credentials are classified and sanitised on emission, and the recipe is judged against the bundle it travels in. The patch stack is proven rather than assumed. The capture used to compare each patch against the FINAL tree, which every patch satisfies by construction, so a stack that could not replay from its base passed anyway. The stack is now replayed in an isolated tree from each root's recorded base, in order, under a git environment of its own -- host config, attributes, the ambient umask and the safe-directory trust list all reached in and changed what the replay produced. A base-less root, the ordinary wheel-installed shape, declares its overlay inventory instead, which is how such a root is actually replayed. Finally the B43 delivery contract this PR is named after is restored, and an enablement round inherits the launch configuration earlier rounds established, instead of re-deriving it at the seam every round crosses. Squashed from 72 commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`environment_closure` was observed only when a runtime override existed, and
an override has exactly two sources: the round's own provisioning result, or
the `runtime_override` it was dispatched with. An enablement that patches the
framework tree in place has neither, so the probe never ran and the recipe
carried no record of what the KEEP depended on. An image rebuilt from such a
recipe failed to boot on a missing transitive dependency; `replay_sufficiency`
had been reporting `environment_closure_absent` correctly all along.
The probe needs an interpreter, not an override, so resolve one:
- `resolve_keep_interpreter` falls back to the interpreter the caller
resolved on every backend, not only on bypass. A backend that can name
none still resolves to "" rather than to a guess.
- Off bypass that fallback is `_resolve_probe_python`, the serving
framework's interpreter and not the benchmark backend's -- on a split-venv
host Magpie runs from one venv and the server it launches from another,
and a confidently wrong closure is worse than an absent one.
- Both the framework and the environment come from the accepted round's own
materialized config overlaid with the override, since `benchmark.envs` can
set `PATH` and decide which executable the graded server was.
- The probe itself runs under that same composed environment, so a
`PYTHONPATH` the launch imported from is visible to the closure too.
`probe_environment_closure` now takes that environment explicitly in place
of the override it used to rebuild internally.
`_resolve_probe_python` and `_resolve_magpie_python` accept an explicit
environment, defaulting to the ambient one. An ambient resolution keeps its
exact original call shape, so existing callers and their doubles are untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nineteen consecutive enablement rounds classified as `failure_kind: unknown` while the same fault recurred: the server reached "Application startup complete", served, and then its engine died of `HIP out of memory`. The specialist kept patching kernels, because a resource constraint that reaches it as "unknown" looks like one more thing to fix in source. `classify_failure` had a `resource_constraint` rule matching that text all along. It was never handed it. `server_log_death_excerpt` returned None for three compounding reasons, and the evidence then fell back to a raw log tail that carriage-return progress bars had entirely filled. `_SERVER_DEAD_MARKERS` names only failures that happen before the server serves. A post-startup engine death matches none of them, so `_FATAL_ENGINE_MARKERS` now names that shape. It is kept out of the legacy tuple on purpose: that one also drives the live readiness waiter, and widening it would change when a running server is torn down. The search read the last 64 KiB. An engine that dies mid-serving logs one downstream error per rejected request afterwards, thousands of them, so the cause is at the head of the cascade and no tail window reaches it. Bounding a read from the head instead would leave the middle of a long log unsearched, so the file is streamed a line at a time, with two lines of context in hand and a bounded read-ahead once a marker is found. Memory stays flat and no region goes unsearched. The excerpt was a tail slice of a fixed window, which dropped the marker line itself whenever the line above it was large -- vLLM dumps its whole scheduler state directly above its fatal-error line. A post-startup death now yields the marker followed by the exceptions below it, nearest first: the frames between carry no rule the classifier can use, and the window runs past this traceback into the errors it caused, so the last exception in it names the consequence while the cause sits above. Legacy markers keep the extraction they had, since those are wrappers whose root cause is the line above -- one of them says so. Marker classification is ordered, legacy first: every `_FATAL_ENGINE_MARKERS` entry is a substring of some legacy entry, `EngineDeadError` of `AsyncEngineDeadError`, and a legacy line must keep legacy handling. On the 3.0 MB log this was found in, the excerpt goes from None to the fatal line plus its `HIP out of memory`, and the round classifies as `resource_constraint` -- which the taxonomy marks `requires_code_acquisition= False`, the signal that no source change will help. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An enablement kept twelve patches and emitted an ordered recipe of twelve patch
steps. An image built from that recipe booted, reported "Application startup
complete", served seventy-five requests, and then lost its engine:
RuntimeError: Worker failed with error
''_OpNamespace' '_C' object has no attribute ...'
That enablement had rebuilt the framework's compiled extension partway through.
The run's host carried a 91 MB `vllm/_C.abi3.so`; the base image carried the
870 MB one it shipped with. The recipe replayed the Python patches onto the
original binary, and the patched Python called an op that binary does not
export.
`build_manifest` held the build as an attempt row with `ok: True`, but
`recipe_steps` carried no build step: the link runs through
`last_specialist_task_id`, which the final state no longer had. Two facts that
have to travel together do not have the same lifetime -- `kept_rounds` survives
and the linkage key does not -- and when they disagree the recipe is patch-only
and says nothing about it.
`replay_sufficiency` did answer `insufficient` for that recipe, on closure,
assertions, roots and targets. None of its twenty-eight reasons named the
missing build, which is the one that actually broke the replay, because
`_build_reasons` returned early whenever the steps held no build step -- so a
build that ran and went unreplayed was never judged at all.
It is named now. The manifest's attempt rows are what say a build ran; a routing
sentinel only says one was asked for, and a failed attempt is not something a
replay owes. Each executed build the steps do not replay is reported once, in
manifest order, scoped to the build it names.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`select_linked_build` decided which build belongs to a recipe by matching a routing sentinel's `probe_task_id` against `last_specialist_task_id`. That field is a one-shot marker: `_maybe_enqueue_specialist_requested_build` clears it through `_consume_marker()` as soon as a specialist-requested build is enqueued. Requesting the build is what erases the key that says whose build it is. By the time a recipe is emitted the marker is empty, the lookup returns nothing, and the recipe carries patches and no build -- without saying so. An image built from such a recipe puts patched Python on whatever binary the base image shipped. One did: it booted, served seventy-five requests, and lost its engine to `'_OpNamespace' '_C' object has no attribute`, the patches calling an op the original extension does not export. The linkage was never missing from that session's state, only from the marker. The sentinel's `probe_task_id` is the task id of the fourth of its eight kept rounds. So the marker is tried first while it is set, and the kept rounds after it, latest first. The sentinel match and the attempt-row join are the same equalities as before, so a build no round asked for still links to nothing. What changes is that the key now lives as long as the recipe does: kept rounds are what the recipe replays, and a build a kept round asked for is part of the stack it accepted whether or not it was the last one. On the session above the recipe goes from twelve patch steps to twelve patches and the build that produced their binary, carrying the source, ref and resolved sha a replay needs to rebuild it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A build installs nothing of its own. Each compiled extension it produces travels only as an artifact a specialist declares, one file at a time, and declaring some of them is indistinguishable from declaring all of them: the round boots, benchmarks, is kept, and the recipe goes out. One run built four extensions for the framework package and carried one. The recipe replayed into an image that reached "Application startup complete" and served a hundred and fifty-five requests before a code path reached `_moe_C.topk_softplus_sqrt` -- an op the built extension exports and the one actually loaded does not. The host that produced the recipe has the same gap, so the replay was faithful; it is the enablement that was incomplete, and nothing in the contract said so. At the KEEP the linked build's outputs are now compared against the framework root and the difference recorded. Scanned inside the trees the build itself names as its output, because an attempt root also holds cloned dependencies and, where one was provisioned, a virtual environment with its own copy of this same package. Every shared object in those trees counts, at its path relative to the package: an extension built without the stable-ABI tag carries an interpreter-specific suffix, and one belonging to a subpackage is not at the top. The observation is three-valued and stays that way through persistence and collection, because the states mean opposite things. Names say which files were left behind. Empty says a tree was scanned and nothing was. None says the build's outputs could not be read, which is not evidence that anything was carried -- an absent worktree, a cleaned-up candidate, a file that would not compare. Absent says a session that predates the observation, which is not an unreadable build. `build_extensions_not_carried` and `build_carry_unverified` block a replay on the first two of those. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An enabled stack booted under `--enablement off`, served, and lost its engine:
RuntimeError: Worker failed with error 'out of resource: shared memory,
Required: 98304, Hardware limit: 65536.
Reducing block sizes or `num_stages` may help.'
`classify_failure` returned `unknown`, and unknown is what an enablement
specialist has been working from for this model -- nineteen rounds of it in one
session -- which is why the rounds kept editing boot paths instead of the thing
that was failing.
The taxonomy had a place that looked right and was not. `resource_constraint`
means the host lacks what the run asked for; it sits in `_ENV_FAULT_KINDS` and
sets `requires_code_acquisition=False`, so filing this there would have told the
specialist that no source change helps. A kernel asking the device for more
shared memory than a launch can have is the enablement's own configuration, and
the tool that reports it names the fix.
`KERNEL_RESOURCE_LIMIT` says that instead: same `rocm_hip` bridge as the other
kernel work, code acquisition still required, matched on the resource wording,
the required-versus-limit pair, the remediation sentence, and the CUDA-side
phrasing of the same fault.
It is ordered ahead of `HIP_KERNEL_MISSING` in `_RULES`, which is what decides:
that rule matches any `hipError` token and `hipErrorLaunchOutOfResources`
carries this exact text, so being more specific in `FAILURE_KINDS` alone would
not have kept it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A recipe exported four env levers. One of them existed nowhere: not in the twelve patches it replays, and not anywhere in the framework tree the replay produces. `VLLM_HL_` is a prefix this project's own patches introduce, so the knob came from a round whose patch was later superseded -- the patch went, the lever stayed in `accepted_config`, and the recipe now exports an instruction nothing consults. A replay sets it and reproduces nothing by it, without saying so. A lever is accepted because the round that set it advanced. Never because a reader was shown to exist, and nothing looked afterwards. At the KEEP the accepted envs in the framework's own namespace are now checked against the framework tree. The namespace matters: `AMD_SERIALIZE_KERNEL` is the HIP runtime's and `NCCL_*` the collective library's, and their absence from this tree says nothing about them -- naming those would refuse a replay over working configuration. The scan reads every regular file and matches bytes. A reader can sit in a kernel that calls `getenv`, a launch script that expands it, a `Dockerfile`, a `Makefile`; a suffix list would not be evidence of absence, only of where the scan looked. A match inside a compiled artifact counts as well, which can make this miss a dangling lever but never invent one. The KEEP's own effective config is scanned alongside the standing accepted one, because the lane does not replace the latter until it re-arms on this result -- checking only shared state would cover every round's levers except the one that decided the recipe. Three-valued, like the build-carry observation beside it: names with no reader, empty for a completed scan, and none when the tree could not be read, which includes a tree that is not there -- an empty walk would otherwise report every lever as unread. `lever_has_no_reader` and `levers_unverified` block a replay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A replay script listed fifteen patches and named the wrong package to apply
them to:
export FRAMEWORK_ROOT=.../dist-packages/aiter
--- a/vllm/platforms/rocm.py
Every patch failed, at every strip level, and the script said nothing about why.
`patches_span_multiple_roots` was false, so nothing else said anything either.
`enablement.framework_root` is whichever root the last round to set it was
working in. A run that touches a second tree at any point leaves it naming that
tree, while the script is written from all the accumulated rounds -- so the two
drift apart and the recipe becomes unusable without ever looking wrong.
The patches know their own tree. The script's root is taken from them now: when
every classifiable section agrees on one top-level name and the recorded root's
own name differs, the recorded root's siblings are searched for it. The VCS kind
follows the same value, since correcting a git checkout to an installed package
while still announcing "git" would emit `git -C "$FRAMEWORK_ROOT" apply` against
a tree with no repository -- unusable in a new way.
What the patches do not settle, they do not change. Two trees in one recipe is
not something a single root can express, so a disagreement keeps the recorded
root rather than dropping half the replay. So does a patch that names no tree at
all -- a binary one, say -- because it may be the one that targets elsewhere.
So does a sibling that does not exist, and any read that fails. A guessed path
is worse than a wrong one that at least came from the run.
Reading them takes some care. Headers are recognised only outside hunks, whose
two side-counts are tracked separately: summing them double-counts context lines
and the hunk then swallows the next file section. A creation's old side and a
deletion's new side are `/dev/null`, so the other side is the one that names the
tree, and a rename-only patch names it in `diff --git` alone.
On the recipe this was found in, the fifteen patches move the root from aiter to
vllm, and an image built from the corrected script applies all fifteen.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Session Breakdown used to be rebuilt by walking state and disk; #1455 retired that read side in favour of recording at author time, and deleted the collectors it replaced. This PR had grown the enablement section inside those collectors, and then called back into them from the recorder -- from the side that survived into the side that had not. So the assembly moves to where its consumer is. The enablement section is built in breakdown/recorder/enablement_section.py, next to the recorder that writes it, and reaches into no retired module. collectors/sessions.py returns to exactly what upstream carries. build_attempt_summary moves further: to the recipe package, whose build_recipe_steps is now its only caller. It was injected rather than imported precisely so the recipe reused the shipped projection instead of re-reading the manifest; with the shipping module gone, the projection belongs with the machinery that depends on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sweep that proves the enablement relocation left nothing behind matches `package.module` and `package/module`. It never matches `from <package> import <module>`, which writes neither -- the most ordinary way to import a module is the one spelling the guard cannot see. This is not theoretical. Rebasing this branch onto the relocation produced three stale imports; the sweep caught one. Of the two it passed, one raised ImportError in a single test and the other -- `from hyperloom.orchestrator.framework import targeted_build`, reached through the recipe's build inputs -- broke collection in 53 files. A guard whose whole purpose is "nothing was missed" reported clean through both. So add the alternative, for each package the move emptied, and pin the spellings: seven that must be caught, four surviving ones that must not. The character class carries an absolute prefix and a relative `..` alike, and the optional group ahead of each name covers a multi-name import that lists it second -- the shape the third stale import happened to take. The sweep's first catch under the new alternative is in this branch's own recipe projection, which still reached for the pre-move stack_actions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Upstream made every enablement KEEP open a revalidation window: the rearm holds `validation_pending` and leaves `succeeded` to the promote, so the round that lands a KEEP no longer closes the lane. This branch's test still pinned the shape that preceded it -- a KEEP closing the lane and the close carrying the replay verdict -- and read the absent close as a missing verdict. The verdict is still computed at the lane's terminals; there are simply two of them now, and neither is the KEEP. `test_the_writeback_close_also_carries_ a_replay_verdict` holds the guard this test was written for, and it passes. So this one is retargeted at what the new lifecycle actually promises, which is the stronger claim of the two: a provisional KEEP must publish no terminal verdict at all. A verdict recorded there would describe a stack no measurement had yet confirmed -- exactly the certification this PR exists to withhold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The projection resolved a patch's own root -- ``patch_roots`` for the stack that spans trees, the framework root otherwise -- used it to look up ``root_id``, and then serialized the framework root into ``root`` regardless. On a single-root stack the two agree and nothing shows. On the multi-root stack this projection exists to describe they contradict each other inside the same step: ``root_id`` names the tree the patch was written against while ``root`` names the tree the round settled on, and a consumer reading ``root`` applies the patch to the wrong one. The recipe's whole claim is that a replay can act on it without guessing. A step that answers the same question two ways answers neither, so ``root`` carries the resolved value and a test pins a two-root stack, asserting the pair agrees on each step rather than only that the ids are right. Also records the recipe and the bounded ray.init in the changelog, which AGENTS.md requires for anything an operator can observe and which this branch had not done. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways the recipe said more than it should. `_setup_step` reconstructed the verbatim setup command into the step. That step lands in `session_breakdown.json`, which is written to disk and shipped in the session package, so an index token, an authenticated VCS URL or a secret environment assignment travelled with it -- beside a `credential_class` field computed from the same text, and beside a ledger row that had sanitised its own copy all along. Nothing is lost by withholding it: a step carrying a `credential_class` already raises `credential_required`, which refuses the replay, so the verbatim text serves no consumer the sanitised form does not. A clean command still travels verbatim, because that one the replay can run. `deliverable` judged each payload against the per-file ceiling alone. The argument was that charging it for unrelated files sorted ahead would refuse a recipe over content it does not name -- but the packer spends its budget in selection order and those files do consume it, so a payload behind an exhausted cap is one the consumer never receives. Reporting it deliverable is how a `sufficient` recipe came to ship with its own evidence missing. It now runs the packer's own selection and caps and answers from what would actually be written, which also stops the two sides from keeping separate copies of the same rules. The refusal that follows is not over content the recipe does not name; it is over bytes it names and will not get. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rule exists to tell one thing from another: a kernel whose own launch configuration exceeds the hardware, which the source can fix, from a host that simply lacks what the run asked for, which no source change reaches. It was given `Required: <n>, Hardware limit: <n>` as a pattern of its own, and each pattern matches independently -- so it claimed anything worded that way. That is how every capacity diagnostic words itself. `insufficient GPUs for tensor parallel. Required: 8, Hardware limit: 4` classified as `kernel_resource_limit`, and so did a KV cache larger than the device. The rule runs ahead of the resource-constraint one and carries `bridge_layer="rocm_hip"`, so each of those became a code-acquisition candidate: the enablement sent off to rebuild source that was never wrong, for a machine that was never going to be big enough. The one distinction this rule was added to draw, it erased. The pair is now anchored to the launch that reports it, and `hipErrorLaunchOutOfResources` joins it as a first-class spelling. Two negative cases are pinned alongside the three positive ones, because the positives passed throughout and said nothing about this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The recipe's setting script is executed by a shell; the launch args were
stored as a command line. Interpolated raw, `--compilation-config
{"max_cudagraph_capture_size":8,"cudagraph_mode":"NONE"}` is brace-expanded
and quote-stripped into three words -- the flag, a value that is no longer
JSON, and a stray operand -- so the server never receives the setting the
file plainly shows it receiving.
That is not hypothetical. Running this branch's own e2e from a setting script
without the compilation config, the engine booted with
`max_cudagraph_capture_size: 512`, a worker segfaulted mid-generation, and the
baseline failed; the round that produced the recipe had measured cap 8 with
`cudagraph_mode=NONE` as the one configuration that serves this model without
segfaulting. A recipe a consumer cannot run is the one thing this PR is for.
The patch and artifact lines above it were already shell-quoted; this line was
not. It is now tokenized by the splitter the launch path itself uses -- so the
script hands the server the argv every other consumer got, rather than a
second opinion about where the tokens are -- and each token is quoted. The
test asserts on the rendered script rather than the helper, because the defect
was a call site that did not use it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The routing lookup matched a manifest row on `task_id` alone. Nothing carries that key except the sentinel `_note_build_routed` writes, so the lookup was correct -- by an invariant of `BuildResult.to_state`, two packages away, which writes no `task_id`. If it ever gained one, every completed build would read as already routed and its launch probe would never be enqueued; a build lane that silently stops routing is the kind of failure that is found in a run, not in a diff. So the reader names what it is looking for. That exposed the writer: the append path stamped `routed`, the merge path took only the caller's fields, and two of the three call sites pass none -- leaving a row that named the build and no longer said it had been routed. Both paths stamp it now, and the hypothetical attempt row is pinned as a test rather than left as a property of a serializer nobody editing this file would think to check. Raised in review as a live defect on the claim that the executor's attempt row already shadows the sentinel. It does not, and the test that says so passes; what the review found was that the invariant was implicit, which was worth closing on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_RECIPE_KEYS` is documented as the verdict plus "the evidence that verdict was
reached over, kept beside it so the decision can be re-derived rather than
merely trusted". Two of the inputs were missing from it.
`build_extensions_not_carried` and `levers_without_readers` decide the
`build_extensions_not_carried` and `levers_unverified` reasons; the collector
computes them, the rules read them, and the recorder then dropped them before
writing the event. A consumer reading either reason in `session_breakdown.json`
could not see what it was decided over.
Their three readings are not interchangeable -- `None` says the scan could not
be made, `[]` that it came back clean, a list what it found -- and the
collector already separates them with a sentinel, omitting the key entirely
for the first. The straight key copy carries all three, so the fix is to name
them; the test pins each reading end to end from `EnablementRound` into
`enablement.recipe`, because the two that matter most are the ones an `or {}`
anywhere on the path would quietly merge.
Raised in review together with `accepted_config`, which is not missing: the
event records it and its archived path directly, one level above the recipe
block.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…p dropped Two things that survive past the moment they were supposed to end. `ray.init` cannot be cancelled, so a timed-out attempt is abandoned and the caller is told the cluster is unusable. The abandoned thread was still connecting. If it succeeded, this process -- a long-lived coordinator, where "the next attempt" is a later leg of the same run -- was holding a Ray session nobody asked for, racing whatever came next. The call still cannot be stopped; its effect can be undone, so a runner that finishes after being abandoned shuts the session back down on its way out. The flag is set before the TimeoutError is raised, so a connect landing in that window still sees it. `ProvisionResult.to_state` wrote `resolved_ref` and `resolved_packages` and `from_state` restored neither. Those two ARE the acquisition's identity -- the commit an editable clone actually landed on, and the version and digest of each package it installed -- so every serialize/rehydrate cycle, which is how this crosses a round boundary, handed the recipe a pinned runtime it could no longer tell from an unpinned one. A recipe that cannot say what it installed is the failure this PR exists to prevent, arriving through the one path that looked like plumbing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he hook to its workload The abandonment fix had the race it was written to close. The runner checked the flag and the caller set it, on two threads with nothing between them: a connect returning a microsecond before the join expired read "not abandoned", returned, and the caller then declared abandonment -- a connected process with nobody responsible for it, which is the state the cleanup exists to prevent. One lock now decides which side got it, and the timeout is read from that verdict rather than from `thread.is_alive()`, which answers about a thread that may already have connected. The test pins the interleaving instead of the happy path it had been pinning. Separately, `client_tokenizer_ok` was folded into `MagpiePatchStatus.ok`, the install-time contract. The hook is workload-specific and a Magpie layout carries siblings it does not fit -- the multimodal `*_mm.sh` among them, which this very machine reports as unpatchable on every run. Requiring them failed installation over a script the run would never execute. It stays reported, out of `ok`; the hard failure belongs where the materialized config names both the model that needs the hook and the one script that will run it, which is `BaselineExecutor`, and it is unchanged there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eft it The abandoned runner was made to shut its own session down. It cannot: with `ignore_reinit_error=True`, a late `ray.init` on an already-connected process is a no-op that attaches to whatever session is current, so by the time that thread runs, "its" session may be a LATER leg's working connection. The cleanup would tear down the wrong one -- a cross-leg teardown in a long-lived coordinator, worse than the stale session it was meant to clear. Responsibility moves to the thread that owns the process from then on. A timeout records that a runner may yet connect; the next attempt clears whatever it left, before taking the session for itself. Legs here are sequential, so that thread is the only claimant, and a connect landing after its shutdown attaches to the session it is about to create anyway. The flag is process-wide by design -- that is the coupling, not a leak -- so the tests isolate it explicitly rather than inheriting a timeout from whatever ran before them. This is the third pass over these few lines: bound the hang, then the abandoned thread's own race, then the cleanup's reach. Each was found by asking what the thread that outlives the call can still touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…isabled Two places where a string was taken for something it is not. `_resolved_packages` handed `action.packages` straight to `importlib.metadata.distribution()`. Those are requirements -- `vllm>=0.10`, `vllm[all]`, `aiter @ https://...` -- and only a bare name resolves; every other spelling raised PackageNotFoundError, was skipped, and left the map empty. A wheel acquisition is judged pinned by that map carrying digests, so a valid pinned acquisition reported `runtime_rebuild_required` and the recipe asked the consumer to rebuild what it already had. The probe now takes the distribution name off the front of the requirement, in the attempt interpreter, with no dependency to install for it. Credential-channel detection read any nonempty value as a live channel. `GIT_TERMINAL_PROMPT=0` is the standard way to forbid interactive credentials and `PIP_KEYRING_PROVIDER=disabled` is pip's own value for the same, so the two spellings that say "no credentials here" marked every setup and build in that environment `credential_required` and the recipe permanently insufficient. Those names are switches, not locations; a switch that is off is read as off, and the values that do mean a channel is live still do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three passes tried to make an abandoned connect safe alongside a new one. Each closed a window and opened another, because the thing being guarded is process-global and uncancellable: whatever the abandoned runner does, it does to the same Ray state the next attempt is using. The interleaving that survived: the next attempt cleared the stale marker and shut down, the abandoned runner connected in the gap, and that attempt's own `ray.init` -- `ignore_reinit_error=True` -- silently attached to the OLD cluster. That is precisely what the version-mismatch retry exists to escape, and it would have escaped nothing. So attempts are serialized instead of reconciled. A gate is held for the life of a connect and released by the thread that made it, abandoned or not; a second attempt waits out an unresolved first and, if it stays unresolved, reports the cluster unusable rather than racing it. The stale marker is set under the same lock the runner reports through, so it is always in place before the gate frees -- the next attempt cannot start and find it missing. The test starts the second attempt while the first runner is still inside `ray.init`, which is the ordering none of the three previous fixes covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`etc/apt/auth.conf.d` is a directory, and a stock Debian or Ubuntu image ships it empty. Detected by `Path.exists()`, it marked `apt_auth` a live credential channel on every such host -- including the machine this branch was developed and measured on, where `detect_credential_channels` returned `['apt_auth']` against an empty directory. Every setup and build there raised `credential_required`, and every recipe produced there was permanently insufficient, for a credential nobody had configured. This is the third reading of the same kind in this file: a switch set to off, a requirement string taken for a name, and now presence taken for contents. A drop-in directory is evidence when something has been dropped in. An unreadable path still counts, because a store this process cannot stat is one it cannot rule out, and the refusal that follows is the safe direction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`select_linked_build` falls back from `last_specialist_task_id` to `kept_rounds` for one reason: the marker is one-shot and is normally already consumed by the time a build is linked. That fallback was added to this branch so a build reachable only through a round's durable identity still joins. It could never fire in the recorded recipe, because neither the collector's export nor the recorder's key list carried `kept_rounds` -- the logic shipped and the data it reads did not. Both lists carry it now, and the test drives the case the fallback exists for: the marker cleared, the build reachable only through the round. A projection that omits what its own rules consult is a verdict reached over evidence the consumer never receives. This is the third time on this branch that a fix landed in the runtime and stopped at a field list -- the two tri-state scans before it. The lists are the contract; the logic is only what reads them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Carrying `kept_rounds` into the recipe copied the rows verbatim, and those rows are internal: `_push_kept_round` stores authoring-workspace patch paths and raw artifact dicts holding source and target. A recipe exists to be replayed somewhere else, and `kept_patches` beside it is relativized while `kept_artifacts` is reduced to its normalized fields -- this one arrived with absolute paths from the machine that produced it. Normalized the same way now: the task id, the patch reference, and each artifact's relative target. The replay sources a round's patches from the archive by name, so the name is the whole linkage and the directory they sat in here is not part of it. The first attempt at this wrote `_rel(...) or Path(...).name` and changed nothing, because `_rel` falls back to `str(path)` rather than to `None`: the `or` never fired and the absolute path travelled exactly as before, with the code reading as though it had been handled. The check is explicit now, in a helper that says what it guarantees. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Normalizing `kept_rounds` left `kept_patches` beside it untouched, so one recipe described the same patch twice and disagreed with itself: once by name, once by an absolute path into a directory on the authoring host. Of the two, only one can be acted on anywhere else. Both go through the same rule now -- session-relative where the patch is in the session, the bare name where it is not. `kept_patches` carried the same dead `_rel(...) or str(p)` shape the round export did, for the same reason: `_rel` falls back to `str(path)`, so the `or` never fired and the fallback that looked like the fix was never reached. Found while checking whether the observation handed to review applied here too. It did, and a fix that leaves two fields contradicting each other is worse than the leak it closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more places the authoring host escaped, both reported by review after the kept-round fix: `recipe_steps[*].path` -- the recipe's own product, the array a consumer replays in order -- still carried the raw `kept_patches` value, and the kept-round artifact export fell back from `rel_target` to a verbatim `target`, which is an install path on this machine. This leak has now been closed at four surfaces in a row, each time in front of the last. The test that goes with this one asserts over the serialized recipe as a whole rather than field by field, because per-field assertions are what let it keep reappearing somewhere else. Normalizing on the way OUT is the other half. The first attempt rewrote `kept_patches` in the state handed to `build_recipe_steps`, and that state is what `patch_targets` and `patch_roots` are keyed by: every lookup missed and the recipe reported `patch_targets_unknown` for every patch -- a portability fix that silently cost the steps their targets. The inputs stay raw so the joins hold; only the published value is rewritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A patch step kept the absolute `root` it was resolved against, and a kept artifact kept its absolute install `target`. Neither is read: the rules join on `root_id` and `rel_target`, and `project_roots` already drops the same path from the root records for the same reason. Shipped, they describe a directory layout the consumer does not have. `framework_root` stays. It is the recipe's declared subject and the setting script exports it, so it is the one host path with a job. The setting script also reads artifact targets from the durable `kept_rounds`, not from this projection, so dropping the published `target` costs the replay nothing. The whole-recipe test now populates every field this leak has surfaced in -- framework root, patch roots, kept artifacts, roots -- because each earlier fixture left one of them empty, which is how the next surface kept going unnoticed. It asserts the absence of the fields that may not carry a host path rather than the absence of a substring, which is the form that survives a field being added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zoroyihan7
force-pushed
the
feature/r1-recipe-impl
branch
from
September 17, 2026 16:06
ccde021 to
e1cf0ad
Compare
zoroyihan7
marked this pull request as ready for review
September 17, 2026 16:10
Comment on lines
+585
to
+586
| "triton.runtime.errors.OutOfResources: out of resource: shared memory, " | ||
| "Required: 98304, Hardware limit: 65536. Reducing block sizes may help.", |
| try: | ||
| _connect(address) | ||
| landed = True | ||
| except BaseException as exc: # noqa: BLE001 - re-raised on the caller's thread |
| try: | ||
| ray_runtime.quiet_ray_init(num_gpus=1) | ||
| second["ok"] = True | ||
| except BaseException as exc: # noqa: BLE001 |
Comment on lines
+585
to
+586
| "triton.runtime.errors.OutOfResources: out of resource: shared memory, " | ||
| "Required: 98304, Hardware limit: 65536. Reducing block sizes may help.", |
| await asyncio.to_thread(reached_second.wait, 30) | ||
| pending.cancel() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await pending |
| try: | ||
| _connect(address) | ||
| landed = True | ||
| except BaseException as exc: # noqa: BLE001 - re-raised on the caller's thread |
| try: | ||
| ray_runtime.quiet_ray_init(num_gpus=1) | ||
| second["ok"] = True | ||
| except BaseException as exc: # noqa: BLE001 |
Two things CI caught that the suite could not. The enablement section was lifted out of the retired collector with its import block intact, and the functions that needed `os`, `re`, `datetime`, `to_unix`, `iso_z`, `now_iso` and `_to_int` stayed behind -- eight imports naming nothing. `credentials.py` used `Any` in a signature it never imported. `from __future__ import annotations` makes every annotation a string, so the name is never resolved at runtime and no test could have failed on it; it is undefined all the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two removals, both of code nothing depends on. `recipe/attempts.py` carried verbatim copies of `_RECIPE_STATE_FIELDS`, `_LEGACY_ROUND_COUNTERS` and `_CLOSURE_DENYING_CODES`, module-private and read by nobody -- the file's only symbol with a caller is `build_attempt_summary`. They arrived when the enablement section was lifted out of the retired collector, and the proof they are dead is that they have already diverged: the live `_RECIPE_STATE_FIELDS` lists `kept_rounds` and this copy does not, and `kept_rounds` is exactly what `select_linked_build`'s fallback reads. Two definitions drifted apart with nobody noticing, which only happens to one nobody reads -- and a stale duplicate of a live constant is worse than no duplicate, because the next reader cannot tell which is the authority. `_magpie_patcher` walked the same two script directories at four sites, each repeating the resolve, the dedupe, and the skip of `benchmark_lib.sh` -- a skip that has to hold at every one of them or a shared library gets patched as if it were a caller. Two generators now say it once. One of those sites also computed `seen_target`, assigned it, and then `del`-ed it without reading it. Net -55 lines, no behaviour change: 8 failed / 17663 passed, the same eight failures origin/main has on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| def test_patching_the_client_is_idempotent(tmp_path): | ||
| from hyperloom.orchestrator.actions.executors import _magpie_patcher as mp | ||
|
|
||
| _applied, text = _patch_client(tmp_path) |
| def test_patching_the_client_is_idempotent(tmp_path): | ||
| from hyperloom.orchestrator.actions.executors import _magpie_patcher as mp | ||
|
|
||
| _applied, text = _patch_client(tmp_path) |
| await asyncio.to_thread(reached_second.wait, 30) | ||
| pending.cancel() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await pending |
Lint is two gates and this branch had only ever been held to one. `ruff check` was clean throughout; `ruff format --check` had never been run, and eighteen files had drifted -- every one of them a file this branch touched, none of them upstream's. The drift is what a long series of hand-applied edits leaves behind. Formatted with `ruff==0.16.2`, the version lint.yml pins, over the whole tree as CI runs it rather than over `src/`: a different version reformats differently and a narrower path answers a different question, which is how the first two attempts at this reported clean. Formatting only -- 8 failed / 17663 passed, the same eight origin/main has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Continues
feature/r1-recipe-impl. The branch already carried R1a'srecipe_stepsprojection and R1b's
replay_sufficiencycontract; this run closes the findings the R1bdesign was approved into implementation still carrying, and the acceptance clauses an
audit of both designs found named but unreached.
Carried findings
B42 — the design document exceeds a 900-line cap. Informational; the designs are frozen
and no code follows from it. Not addressed.
B43 — a recipe could be replay-sufficient while its referenced payloads were not
deliverable.
evaluate_replay_sufficiencyalready took adelivered_pathsset, but noproducer passed one, so every session took the branch that judges a recipe on its content
alone.
(
referenced_payloads), andsession_package.deliverable()answers per candidate:matched by the curated selection, held as a regular file inside the session, and small
enough that a bundle could carry it. Absent for any reason is absent.
content of every patch and artifact target the recipe declares was never shipped.
optimization_stack/enablement/**/files/**is now selected — the captured bytes only,since each capture's own manifest records the absolute framework root it was taken
under, and the portable projection of it already travels in the breakdown.
into one directory and the mechanism never cleared the files beside the manifest it
overwrote. Each capture now starts from an empty overlay, so what ships is the accepted
stack rather than the union of every round.
Patch steps are judged through their root's snapshot rather than their own
path, whichnames the authoring workspace no bundle ships.
B44 —
base_sharecorded the tree after the KEEP commit. Already closed on the branch:_git_head_shais read before the stash and before any apply, andtest_a_kept_patch_applies_exactly_once_to_the_recorded_baseapplies a real unified diffto the recorded base and is refused a second time. This run closes the per-root half —
is_git/base_shawere read offframework_rootand stamped onto every record, so asecond contributing checkout was asserted non-git with no base commit — and refuses a git
root whose commit the record does not name, which a failed
rev-parseused to leavestanding unjudged.
B45 — successful setup steps could depend on inputs neither captured nor rejected.
Capture and refusal were already on the branch. The missing half was delivery: the identity
stopped at durable state, so the emitted step handed a consumer a command string and
nothing else. The
setupelement now carriesinput_identityandunresolved_inputsandthe projection states the replay rule; the identity names the file where a delivery would
carry it, and a payload the bundle does not carry is refused rather than advertised.
Also in this change
Five rules were reporting more than the evidence supported: an absent
launch_evidencejudged only when a configuration had also projected;
setup_ledger_truncatedfiring for acommand the accepted round ran and failed;
closure_scope_incompletegated on an executionsucceeding;
dependency_closure_statusreading"verified"when the ledger that decidesscope was absent or capped; and
present_at_final_launchnever taken back from an earlierKEEP round, which let a stale row answer for a command the validated launch never ran and
made truncation unable to fire at all.
Test additions cover the audit's unreached clauses: the build element's key set pinned
closed, declarativeness walked to the leaves of a nested
build_inputs, the publishedrecipe key set actually inspected, multi-root rounds in both the git and non-git direction,
the KEEP probe's fail-closed half paired with the bypass backend that can resolve an
interpreter, a build inheriting a credentialed index, a runtime with neither rebuild
source, and the env values that must not travel beside their digest.
Deviations from the frozen designs, stated rather than hidden
setup_inputs_incompleteis not in R1b §3.1's closed code table, and the file read thatproduces its evidence is not in §2.1's authorized-collection list. B45 asks for exactly
this — an immutable identity for file and VCS inputs, or an explicit replay-blocking
reason — and neither is reachable without them. Both designs are frozen, so the extension
is recorded here rather than in the design.
It is the only point at which the decision can be made, since the breakdown is itself one
of the packaged files.
cannot be evaluated without enumerating the session tree, and that walk sits on the path
the coordinator drives at shutdown.
Verification
pytestfull suite: 20712 passed, 54 failed, 40 skipped, 8 xfailed (23m). All 54failures are environmental and pre-existing: 47 reproduce byte-identically on a clean
origin/mainworktree in the same environment (credential/preflight/kernelforge suitesthat read ambient provider keys), and the other 7 were an artefact of my own run having
unset
OPENAI_API_KEY. None is in any module this branch touches. Two suitesadditionally need the package importable in a child interpreter (
pip install -e .).ruff check .andruff format --check .— clean.mypy src/hyperloomis advisory and reports 1084 pre-existing errors across 188 files,none in any module this branch touches.