refactor(v1): share the dialects' content walker, mediation and preamble - #2505
Open
hallerite wants to merge 1 commit into
Open
refactor(v1): share the dialects' content walker, mediation and preamble#2505hallerite wants to merge 1 commit into
hallerite wants to merge 1 commit into
Conversation
hallerite
force-pushed
the
chore/declare-direct-deps
branch
from
September 2, 2026 14:47
6671218 to
6913e02
Compare
hallerite
force-pushed
the
refactor/dedup-dialects
branch
from
September 2, 2026 14:47
de7bfdd to
bc295b5
Compare
hallerite
force-pushed
the
chore/declare-direct-deps
branch
from
September 2, 2026 17:22
6913e02 to
e333aad
Compare
hallerite
force-pushed
the
refactor/dedup-dialects
branch
from
September 2, 2026 17:22
bc295b5 to
66bbca4
Compare
An error occurred while trying to automatically change base from
chore/declare-direct-deps
to
main
September 2, 2026 17:34
Behaviour-preserving: the same inputs produce identical output before and after (81 cases across mediate_content, blocked_content_path, rewrite_request, message_to_wire and serialize_completion). - dialects/base.py gains blocked_path (list walk + caller gate, the prefix both anthropic and responses had verbatim), mediate_parts (the list mediation both had, with anthropic's wrapper recursion as a parameter) and user_and_tool_messages (the rewrite_request preamble both had) - anthropic/responses: blocked_content_path and mediate_content become thin wrappers over the shared functions; rewrite_request uses the shared preamble - responses: the content -> input_text/input_image conversion appeared twice in rewrite_request; now _content_to_input - chat: tool_calls_to_wire is the tool-call comprehension message_to_wire had inline; clients/train.serialize_completion had the same one and now calls it (its content/reasoning handling is unchanged) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hallerite
force-pushed
the
refactor/dedup-dialects
branch
from
September 2, 2026 19:21
66bbca4 to
dfa7ec3
Compare
hallerite
marked this pull request as ready for review
September 2, 2026 19:24
Contributor
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This PR consolidates duplicated content-walking, mediation, message-selection, and serialization logic without changing the resulting provider payloads or network-policy decisions. It introduces no new capability, schema, configuration, or deployment behavior. You can add or adjust custom eligibility rules. Learn more. |
xeophon
pushed a commit
that referenced
this pull request
Sep 2, 2026
Stacked on #2505 → #2503 → #2496. Every third-party harness installed its program with the same routine: `mkdir -p <dir>`, take a lock on `<dir>/install.lock`, run `[ ready ] || ( install )`, raise `RuntimeError("<name> install failed: " + stderr[-500:])`. Nine copies, and they had drifted into three different locks: | lock | harnesses | |---|---| | `"$(command -v flock \|\| command -v lockf)"` | claude_code, codex, kimi_code, openclaw, pool, prime_agent | | hand-rolled symlink spinlock with dead-owner check + `EXIT` trap | pi, `node.py` (shared Node install) | | bare `flock` | rlm | Around it: the `version: str = Field(default=..., pattern=...)` field nine times, and an `rm -rf` + raise cleanup six times. **After this PR** - `harnesses/utils/install.py`: `ensure_installed(runtime, directory=, install=, env=, label=, ready=None, lock=None, shell=("sh","-c"))` and `remove_dir(runtime, path, label)`. `ready` is the optional skip test (openclaw passes none: its `SETUP` script from #2485 self-guards and must always run the transcript patch), `lock` lets the Node installer keep its lock beside the directory it replaces, `shell` lets pool and openclaw keep `bash -o pipefail`. Directory and lock paths are `shlex.quote`d, as #2485 started doing for openclaw. - `configs/harness.py`: `PinnedVersion = Annotated[str, Field(pattern=r"^[A-Za-z0-9._+-]+$")]`; the nine configs declare `version: PinnedVersion = "<default>"` and keep their own docstrings. rlm keeps its git-ref field. - claude_code, codex, kimi_code, pool, openclaw, prime_agent, pi, rlm and `ensure_node` install through the helper; claude_code, codex, openclaw, prime_agent and hermes_agent clean up through it. hermes_agent, terminus_2 and mini_swe_agent install via `prepare_uv_script` and only pick up `PinnedVersion`. **Intended behaviour changes** (all in the lock and error path, none in install scripts): 1. Every harness locks with `flock || lockf` when one is present (they release on holder death natively; every common base image ships one, Alpine via busybox) and falls back to the symlink spinlock that pi and the Node installer used before when neither is. The fallback records its owner as `pid:starttime` (from `/proc`, pid alone where unreadable) so a reused pid is not mistaken for the live holder, and reaps a lock that is a regular file or whose owner is gone; the original spinlock spun forever on a regular lock file because `kill -0 ""` succeeds under busybox ash, and would wait on a reused pid until that process exited (e19f2c8). 2. rlm gains the lockf fallback it lacked. 3. Install failures report stdout when stderr is empty or whitespace-only (three harnesses already did the former). Cleanup failures share one phrasing, `failed to clean up <label>: …`. 4. `mkdir -p /var/tmp/vf-node` now precedes the Node install; its script creates the directory itself anyway. Rebased onto main after #2496 merged; the OpenClaw 2.0 (#2485), RLM (#2507) and prime-agent (#2502) changes on main are preserved. **Verification.** I replayed `setup()` and `cleanup()` for all nine harnesses plus `ensure_node` against a recording fake runtime on the base branch and this branch and diffed every command and environment. Against the rebased base, 17 of 23 replays are byte-identical (openclaw included, since #2485 already uses `flock || lockf`); the six that differ (the four Node installs, pi, rlm) differ only in the lock prefix and are identical from `sh -c` onward. `PinnedVersion` rejects `""`, `"a/b"` and `"bad version!"` and accepts `"0.147.0"`. Also `ruff check`, `ruff format --check`, `ty check verifiers`, `pytest tests/v1 -m "not e2e"` (82 passed). Both lock branches were exercised on `alpine:latest`: two concurrent installs serialize, the install's exit code propagates, a dead-owner symlink, a leftover regular lock file and a live process holding a lock with a stale identity (pid reuse) are reaped, a live owner with its true identity is waited on (a4f4ea5). The docker e2e job runs on this PR as well. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- Macroscope's pull request summary starts here --> <!-- Macroscope will only edit the content between these invisible markers, and the markers themselves will not be visible in the GitHub rendered markdown. --> <!-- If you delete either of the start / end markers from your PR's description, Macroscope will append its summary at the bottom of the description. --> > [!NOTE] > ### Consolidate harness install-lock and cleanup into shared helpers > - Adds `ensure_installed` and `remove_dir` to [install.py](https://github.com/PrimeIntellect-ai/verifiers/pull/2506/files#diff-5ddaf01b8cbb0ded4d3d42b42549b45ad9866136f071e72e14cde15c105d8417); `ensure_installed` handles directory creation, optional readiness checks, `flock`/`lockf` serialization with a PID/start-time symlink fallback, stale-owner cleanup, and label-specific errors. > - Adds a shared `PinnedVersion` type alias in [harness.py](https://github.com/PrimeIntellect-ai/verifiers/pull/2506/files#diff-c5b409466f272f39cbab9228a91d9b6de6c8a0cdd33ad73f05e7fd690966e478) and replaces inline version field declarations across all harness config classes. > - Migrates `setup` installers and `cleanup` handlers in the Claude Code, Codex, Hermes, Kimi Code, Node, OpenClaw, Pi, Pool, Prime Agent, and RLM harnesses to the shared helpers, passing through their existing scripts, environments, and labels. > - Risk: all harnesses now share one locking and removal implementation; verify per-harness lock paths and readiness conditions passed to `ensure_installed`, especially `ensure_node` in [node.py](https://github.com/PrimeIntellect-ai/verifiers/pull/2506/files#diff-4117aae0a7b4023c32f09206611f0bf2149b2a51e264157bd24339df2166dfa8) where the lock lives outside the installed directory. > > <!-- Macroscope's review summary starts here --> > > <sup><a href="https://app.macroscope.com">Macroscope</a> summarized 93a2100.</sup> > <!-- Macroscope's review summary ends here --> > <!-- Macroscope's pull request summary ends here --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > All harness program installs and several cleanups now share one locking and error path; lock location and readiness checks (especially Node’s external lock) affect every concurrent rollout on a shared runtime. > > **Overview** > Introduces **`ensure_installed`** and **`remove_dir`** in `harnesses/utils/install.py` and routes harness `setup`/`cleanup` through them instead of nine copy-pasted `mkdir`, lock, `[ ready ] || install`, and `rm -rf` blocks. > > **`ensure_installed`** centralizes concurrent install serialization (`flock` / `lockf`, with an improved symlink spinlock fallback using `pid:starttime`), optional readiness skips, configurable shell (`bash -o pipefail` where needed), and consistent install failure messages (stderr or stdout). **`remove_dir`** standardizes cleanup errors as `failed to clean up <label>: …`. > > Harness configs that pin npm/release versions now use shared **`PinnedVersion`** in `configs/harness.py` instead of repeated `Field(..., pattern=...)` declarations (RLM keeps its git-ref field). > > Migrated installers: Claude Code, Codex, Kimi Code, Pool, OpenClaw, Prime Agent, Pi, RLM, and shared **`ensure_node`**. Cleanup via **`remove_dir`**: Claude Code, Codex, Hermes, OpenClaw, Prime Agent (plus OpenClaw staged-skills clear). Hermes, mini-swe-agent, and Terminus 2 only pick up **`PinnedVersion`**. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 93a2100. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Fable 5.1 <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.
Stacked on #2503 (which is stacked on #2496). Behaviour-preserving; see the equivalence check at the bottom.
The Anthropic and Responses dialects each carried their own copy of the same three pieces, and two other exact twins sat in
responses.pyand betweenchat.pyandclients/train.py.Now shared in
dialects/base.pyblocked_path(value, path, policy, blocked_item): the list walk, non-dict pass-through and non-directcallergate that bothblocked_content_pathfunctions started with verbatim. Each dialect keeps itsblocked_content_pathname and its own per-item rules (_blocked_block/_blocked_item), so every call site and the recursion through nested content are unchanged.mediate_parts(value, path, policy, blocked, wrappers=()): the list mediation bothmediate_contentfunctions implemented. Anthropic's extra branch (check a wrapper block without its content, then mediate its content in place) is thewrappersparameter; Responses passes none, which is exactly its old body.user_and_tool_messages(request): the two list comprehensions bothrewrite_requestmethods opened with.Exact twins removed
responses.py: the content →input_text/input_imageconversion appeared twice insiderewrite_request; it is_content_to_inputnow.chat.message_to_wireandclients/train.serialize_completionhad the same tool-call comprehension. It istool_calls_to_wireinchat.py, whichtrain.pyalready imported from. Only that comprehension is shared:serialize_completionkeeps its owncontentandreasoning_contenthandling, because it deliberately differs frommessage_to_wire(content: nullstaysnull,provider_stateis not consulted).Net lines are roughly flat (+121 / −126): the shared functions carry docstrings. The point is one implementation of each rule instead of two.
Equivalence check. With no dedicated dialect tests, I ran the same 81 inputs through the base branch and this branch and diffed the JSON output:
mediate_contentandblocked_content_pathfor both dialects across three network policies and nested/wrapper/caller/file/url cases,rewrite_requestfor both dialects with string and list inputs (changed and unchanged),message_to_wirefor assistant/tool/user messages includingcontent=Nonewith and without tool calls, andserialize_completionwith and without usage. Identical. Alsoruff check,ruff format --check,ty check verifiers,pytest tests/v1 -m "not e2e"(82 passed).🤖 Generated with Claude Code
Note
Extract shared content walker, mediation, and message selectors into
basedialectblocked_pathfor recursive policy-path traversal,mediate_partsfor content mediation, anduser_and_tool_messagesfor selectingUserMessage/ToolMessageinstancestool_calls_to_wireserializer_content_to_inputto remove duplication in request rewriting_blocked_block,_blocked_item) must preserve their original checks exactly; any missed condition changes what content gets blocked or removed at runtimeMacroscope summarized dfa7ec3.
Note
Medium Risk
Changes only move network-policy blocking and content mediation into shared code; a regression in the dialect-specific block callbacks could alter what provider capabilities are stripped at runtime.
Overview
Behavior-preserving deduplication across Anthropic, Responses, chat, and the train client—no intended runtime changes.
New shared helpers in
dialects/base.py:blocked_path(recursive policy traversal plus non-directcallergate),mediate_parts(strip blocked content parts; Anthropic passes_CONTENT_WRAPPERS), anduser_and_tool_messagesforrewrite_requestalignment.Anthropic and Responses keep their own
blocked_content_path/mediate_contententry points but delegate to those helpers via_blocked_block/_blocked_item. Responses also adds_content_to_inputso typed content →input_text/input_imageisn’t duplicated inrewrite_request.tool_calls_to_wireinchat.pyreplaces identical comprehensions inmessage_to_wireandtrain.serialize_completion.Reviewed by Cursor Bugbot for commit dfa7ec3. Bugbot is set up for automated code reviews on this repo. Configure here.