Skip to content

jev_vl: process inline images with the shared native vision backend - #139

Open
linear3735 wants to merge 26 commits into
ThinkFlowLab:mainfrom
linear3735:codex/jev-vl-online-upstream
Open

linear3735 wants to merge 26 commits into
ThinkFlowLab:mainfrom
linear3735:codex/jev-vl-online-upstream

Conversation

@linear3735

@linear3735 linear3735 commented Oct 12, 2026 •

Copy link
Copy Markdown

Required human contributor answers

Reused from the contributor's explicit instructions supplied during PR preparation.

  1. What outcome are you trying to achieve? Validate online-vision correctness and complete-request latency, then integrate with Agents.
  2. What is the highest-leverage variable for that outcome? Inference speed, prioritizing single-batch performance while validating correctness.
  3. How will this work move that variable rather than create busywork? Reuse qwen: share vision execution for 4B and 27B models #117, compare all three official decision types, changed images and cache invalidation, then measure complete latency for fresh images and multiple questions on a retained image. Optimize measured hotspots after validation. This PR provides the online-image path; bounded current-head validation is recorded below.
  4. Who is the customer or user, and what do they actually need? System1 community developers working on inference and evaluation, with an online-image path and reproducible correctness and latency measurements.

Is a PR needed? Yes. The contributor explicitly requested submitting the current Omni integration before proceeding with Agents integration.

Observed bug evidence: The online-image path is a feature addition. Its HTTP transport fixes are covered by the raw-HTTP regressions: bodies above the old 4 MiB cap reach the worker's structured handler; oversized bodies return a structured 413; a truncated upload returns a structured 400. The truncated-upload probe previously returned 500 at 4ea23c6; the regression now passes at this head.

Purpose

JEV-27B-VL currently requires images to be encoded offline before a native request. This change accepts inline PNG/JPEG images and runs decoding, preprocessing and the shared vision tower inside the Rust/CUDA worker, retaining prepared-image compatibility and the existing choice/noul/score contract.

POST /v1/systemone → existing frontend
  → JEV processor: bounded decode, preprocessing and positions
  → existing serial admission → shared Qwen vision + JEV language/readout
  → existing decision response

The integration adds pinned vision-export validation, startup image warmup, online/prepared input handling and cache-boundary tests. The HTTP body cap is 8 MiB so a 4 MiB decoded image can fit after base64 encoding. Oversized and interrupted uploads retain the worker's structured 413/400 errors. Export pin-checking runs on Tokio's blocking pool before executor initialization. Per-image limits remain unchanged.

Depends on #117, currently 18094f16b342d29c5442838983f8a8dd60783f2f and still open. Its shared vision backend is included in this diff. The named float_repr provenance commit is now included in #117 without changing its source tree or rewriting history. Bounded current-head GPU and browser validation has completed locally; reviewer-accessible publication of the raw evidence and maintainer review remain outstanding.

Test Plan

System1-Omni Version / Commit: 26dc76bf65c544815e7b285866791da1e703816d, based on main 3e9cc33a61ddf110d97512fb6f296cf540377c61.

Current-head GitHub CI runs the repository Rust checks:

cargo fmt --all --check
cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings
cargo test --workspace --locked --features omni-laya/serve
cargo build --workspace --release --locked --features omni-laya/serve

The latest repair was also checked locally with:

cargo clippy --locked -p omni-jev-vl-native -p omni-cua-s1-native -p omni-qwen3-5-native --all-targets -- -D warnings
cargo test --locked -p omni-jev-vl-native --bin omni-jev-vl-native
cargo fmt --all -- --check

Current-head single-H800 validation rebuilt the ABI 6 CUDA library from this source revision, compared all three decision types against pinned autotrust/JEV-27B-VL@f34b598d4ef4bcefd337bee8d8e7ddd3b7733ccc, checked changed images/cache boundaries, and measured complete frontend HTTP requests for fresh images, retained images with new questions, and exact repeats. Checks retained the per-probability absolute tolerance of 0.025 and matching decision fields. Results and limitations are recorded below. Opt-in vision Graph remains outside this default-path validation.

Test Result

  • Current head: CI passed: Rust, benchmarks and CUDA contract checks. Docs passed; PR deployment was skipped. Both runs match 26dc76b. GitHub reports no merge conflict.
  • Local current-head repair checks: strict Clippy passed for all targets of JEV-VL, Cua-S1 and shared Qwen; the JEV worker's five HTTP/CPU tests passed, including the new truncated-upload 400 envelope regression. Formatting passed. Two independent Codex reviewers checked the frozen repair diff and found no unresolved findings; this is a scoped repair review, not full-PR maintainer approval or GPU acceptance.
  • Earlier supporting CPU evidence: at 4ea23c6, the same three crates had 91 passed / 21 ignored tests, and the vision-export rejection suite passed 8/8. The subsequent repair changes only the body-error mapping/type, its HTTP regression and the model README cap. Ignored checkpoint/GPU tests are not validation passes.
  • Current-head single-H800 evidence: native primitive 2/2; correctness 114 blocks / 612 requests, maximum probability delta 0.00263093220749; cache eviction 12 blocks / 36 requests, maximum delta 0.00231216420586. A separate fresh performance run completed 60 episodes / 1,680 raw / 1,080 timed requests. These are local audited results, not GitHub GPU CI. Broad model quality, every task/frontend, and end-to-end browser acceleration remain unverified.

Demo / evidence

The pinned recipe provides export and launch commands; the model contract documents request, memory and cache boundaries. The HTTP regressions exercise the production fallback and shared body mapper without requiring a model or GPU. They do not establish real-model inference.

Current-head GPU responses, repeated timing records, and the related same-trial Agents/browser recording have been produced and independently checked. They are retained outside Git and do not yet have public reviewer-accessible URLs. The separate Agents integration is tracked in ThinkFlowLab/system1-agents#78 and remains draft pending evidence publication; its source changes are not part of this PR.

Bounded runtime evidence (2026-10-14)

The measurements below use this exact Omni head 26dc76bf65c544815e7b285866791da1e703816d; the browser execution uses Agents 0def92771746ec94adc38bf41be9720bccfbf19d. Its submitted Agents head changes only three documentation files. Local code reviews and raw-runtime checks are contributor evidence, not maintainer approval.

  • Single-H800, concurrency 1, same-round official/native paired full HTTP requests: each workload/arm has 180 timed samples, 540 pairs total. P50 official → native: fresh image 214.79 → 165.93 ms (1.29×); retained image/new question 100.06 → 25.97 ms (3.85×); exact repeat 99.15 → 25.84 ms (3.84×). Preserve the negative subgroup: cold score/JPEG native P95 was about 5.33% slower. These figures cover complete HTTP requests, not end-to-end browser task speed.
  • Correctness/cache validation and timing are separate runs: jobs 419100 and 419125 respectively. The first timing wrapper failed while writing its recording; that failure is retained, and no old timing sample was imported into the fresh 60-episode run.
  • Related browser R3 used the existing RECOVERABLE fixture with recovery off: three executed actions, verified hello world POST, four non-forced operation choices, seven visual HTTP 200 responses; six forced targets do not count as learned choices. Chat/value provided text and summary; its backend checkpoint and full cost were not independently established.
  • Preserve all browser attempts: R1 transport timeout; R2 task passed but recording was empty; R3 complete evidence passed. Oracle verified 2/3 attempts, complete evidence 1/3; this is single-fixture smoke evidence, not a task-quality estimate.
  • Same-trial recorded replay: 294 frames / 11.76 s, original application plus static same-run evidence panel, no cuts/speed changes. SHA-256 58550ee293bbfb9c5b1f563c03e8cbdb6f6ac2a3ef23080f73ec3435b932a9e4. No exact frame/UTC mapping or per-request server logs are claimed.

Audit/media publication remains a review gap. Run-local records retain the complete inputs, failures, sources, binary identities and raw results under run 20261012T114800Z-online-vision-agents, revision 26dc76b-agents-0def927; no inaccessible local filesystem link is presented as public evidence. Related client: ThinkFlowLab/system1-agents#78.

Review scope

Against main 3e9cc33a61ddf110d97512fb6f296cf540377c61, the head diff is 46 files, +4329/−1229. Source/test/script changed-line accounting is 5,404 (+4208/−1196), including the shared-backend dependency and code moves; documentation, lockfiles and static fixtures are excluded from that code count. Review the #117 backend separately, then image decoding/export validation, JEV processing/execution and their root-level tests. After #117 merges, refresh the base and review the remaining integration diff.

The path is opt-in through JEV_VL_VISION. Source checkpoint and processor settings remain pinned, and prepared-image semantics are retained. The shared decoder, vision implementation and release DeviceBuffer bounds affect Cua/shared-Qwen consumers; rebuild the CUDA library and consumers together for ABI 6. Further frontend orchestration, schedulers and Agents integration are outside this change.

Disclosed provenance exception: the float_repr rewrite in src/models/qwen3_5/native/src/json.rs (+18/−12) and its tests (+36) originated in #117's merge 20b480a and enter this PR through b7442fd. This changes shared-Qwen prompt serialization and is outside the vision scope; #117's baseline approval at 6480b6b predates it. The unchanged patch has been extracted into named fork commit 900ad7a. The contributor selected #117 as the upstream home. Its current head 18094f1 now includes named commit 900ad7a through a normal merge; the complete source tree is unchanged and published history was not rewritten. The provenance handoff is complete. #117 remains open, and its approvals do not substitute for maintainer approval of #139.

Self-review

Remaining acceptance items: merge the #117 dependency; publish reviewer-accessible raw GPU/application evidence; obtain maintainer review for this PR. Named float_repr ancestry and bounded local current-head GPU/parity/HTTP validation are complete. Exact-head CI and conflict status are refreshed in the submission report; an older green run is not evidence for another head. The contributor's four answers and the four self-review declarations authorized on 2026-10-13 are retained. Runtime limitations remain explicit above.

  • I have reviewed the full diff and addressed the issues I found.
  • I have checked that the change follows the project's architecture and stays focused on the stated purpose.
  • I have run the checks appropriate to this change and reported commands, results, and anything I could not verify above.
  • I have checked that the PR description, documentation, and any accuracy or performance claims match the implementation and available evidence.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope note (head moved mid-review): findings below were verified at pinned head 953b6914fb; the remote head moved to f88d918221 (merge of current main 71857e0) during review. I verified every reviewed PR file is blob-identical at both heads except .github/workflows/ci.yml (which takes main's version plus this PR's one authored line), so all findings and evidence apply to the live head f88d918221. Spot-verified independently: the P2's blob arithmetic (merge 20b480a — both parents d9f3355, result 9557810) and the P3-3 readiness gate (main.rs ensure!(decide(&engine, ONLINE_VISION_WARMUP)…) under JEV_VL_VISION).


Independent review — PR #139 "jev_vl: process inline images with the shared native vision backend"

Reviewer: independent code review (ZCode agent), 2026-10-12. Not posted to GitHub; local report only.
This review grants no approval. Missing GPU/tooling results are evidence gaps, not passes.

Verdict

4 actionable findings: 1 × P2, 3 × P3. No P0/P1. The authored integration itself (bounded decode → CPU
preprocess → serialized vision forward → language prefill → CPU head) is well-scoped, ownership-clean,
and defensively tested; the P2 is a provenance problem in the dependency stack, not a code defect in the
authored commits.

Provenance

Item Value
Base (live, fetched this review) main @ 71857e033b2d2d0587742c1c1a5260d683ab17cd (71857e0, unchanged from last check)
Pinned review head 953b6914fbb2a667a7939dd2ad832136750a11ad (matches GitHub headRefOid at review start)
Remote head after recheck f88d9182219b243a523850783289bb9b26c0d794 — branch moved during review: one new commit, Merge upstream main and retain online vision export CI, merging main 71857e0. Verified the merge preserves every reviewed PR file blob-identical (processing.rs 317c75f9, json.rs 95578103, recipe README 96264686); only .github/workflows/ci.yml resolves to main's version plus the single authored unittest line, plus main-side files (src/backends/cuda/qwen3_5/README.md, qwen3_5.backend.json) taken from main. Findings below apply verbatim to both heads.
Merge base (pinned head vs main) 6b51652b69c761994f9c944505d2527c92b5d254 (PR #108 merge). GitHub's 3-dot diff = 6b51652..953b691.
PR state OPEN, DRAFT, author linear3735, GitHub mergeable = CONFLICTING (cleared in-tree by f88d918's main merge; GitHub marker needs refresh)
Skill load record .agents/skills/system1-omni-review/SKILL.md read from local working tree (repo HEAD ca5f69b5c104); SHA-256 of bytes read 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108 (matches pinned value); blob 7f245253cd identical across worktree / main / PR head. Unpublished local skill (repository-committed, content hash recorded).
References actually read references/repository-contracts.md (blob b6ace00bf8, identical in all three revisions); docs/architecture.md — initially worktree version 7710c734, then the PR-head/main version 9fb69e1165 (they differ; findings use the PR-head text); CONTRIBUTING.md at worktree/PR-head 1aa9639a2e (main differs, b092bbaacc); .agents/skills/self-review/SKILL.md artifact-hygiene section (worktree b6dcf04a5d); post-#138 .github/PULL_REQUEST_TEMPLATE.md at main 71857e0.

Findings

P2-1 — Unattributed rewrite of shared Python-compatible float rendering (float_repr) enters via dependency merge commits, owned by no named commit

  • Files: src/models/qwen3_5/native/src/json.rs:132-167 (head), tests tests/qwen3_5/json.rs:37-71.
  • Trigger: In the #117 line, merge 20b480a ("Merge main and preserve shared vision graph regressions") has
    both parents at json.rs blob d9f3355 (6480b6b and 6b51652) but the merge result is blob
    9557810 — a rewritten float_repr plus two new tie-to-even test functions that exist in no parent and
    no named commit
    (git log --all --find-object=9557810… returns nothing). PR #139 pulls this in through
    its dependency merge b7442fd (merge of #117 head c15a989), so the change appears in this PR's GitHub
    diff (json.rs +18/−12, tests/qwen3_5/json.rs +36) although none of the five authored commits touches
    json.rs.
  • Consequence: float_repr/dumps() render structured state into the prompt for every shared-Qwen
    consumer (Cua-S1, Open-Jev, JEV-VL). If this PR (or #117) merges, main gains a cross-model serialization
    semantics change that no review ever examined and no commit owns — it is invisible to blame, bisect, and
    the #117 review that baseline-approved 6480b6b. The change itself looks correct and is an improvement:
    the old code's own comment admitted it did not implement Python's tie-to-even ("Python breaks the rare
    exact ties … to even; this does not"); the new code delegates shortest-digit selection to
    serde_json::to_string (ryu) and fixes both the tie rule and the doc comment. The oracles are genuine:
    I verified all 15 expected strings in float_repr_matches_python_examples and the five bit-exact tie
    cases match local Python 3.12.8 repr() exactly.
  • Action: give the change a named home (a commit in #117, or a separate small PR) and disclose it in this
    PR's review scope; "the shared vision implementation belongs to #117" is inaccurate for json.rs, which is
    not vision. Alternatively exclude it from this stack.

P3-2 — docs/architecture.md left stale: "Image features must be prepared offline"

  • File: docs/architecture.md:64-69 at head (identical to main's): "The experimental JEV-VL worker …
    Image features must be prepared offline."
  • Trigger: any reader of the architecture contract after this PR merges. The PR's entire purpose is inline
    images encoded inside the worker (JEV_VL_VISION), and src/models/jev_vl/README.md +
    recipe/jev_vl/README.md are updated, but the architecture paragraph — added when the worker landed
    offline-only — is not. mkdocs build --strict passes because it cannot catch semantic staleness.
  • Consequence: the repository's own integration contract contradicts the shipped behavior. CONTRIBUTING's
    documentation rule ("Update affected shared-component documentation") applies. One sentence fix.

P3-3 — Recipe readiness claim stale in online mode: /health gate now includes an image warmup that the recipe doesn't document

  • Files: recipe/jev_vl/README.md ("Build and launch" section) vs src/models/jev_vl/native/src/main.rs:145-150.
  • Trigger: the recipe says "/health becomes available after model loading and a successful text warmup;
    that health check does not validate image assets." With JEV_VL_VISION set, main now additionally runs
    ONLINE_VISION_WARMUP — a full inline-image decision (1×1 PNG → decode → preprocess → vision forward) —
    and ensure!s on it before binding the port. The "Online images" section documents neither the
    warmup nor its readiness effect (the PR body does mention "startup image warmup").
  • Consequence: an operator debugging a slow/failed startup in online mode reads the recipe and concludes
    image health is never exercised; readiness semantics are part of the operator contract this recipe owns.
  • Note: the warmup itself is correct and desirable (real warmup before readiness, per the backend
    lifetime contract); only the documentation lags.

P3-4 — No per-request bound on in-flight online-image buffers (transient ~0.7 GB worst case)

  • Files: src/models/jev_vl/native/src/processing.rs:127-147 (load_images), processing.rs:83-108
    (load_image), executor.rs (resolve_images/pending_images).
  • Trigger: one request with ~250 distinct minimal inline images. Each 1×1 PNG is resized to 256×256
    (min_pixels 65,536) → 256 patches → ~1.57 MB of pixel_values (256×1536 f32) held in
    ImageInput::Inline for every image of the request until the serialized forward finishes; post-vision
    embeddings add ~1.3 MB per image (64 tokens × 5120 × f32) held in pending_images until request end.
    4 MiB body + 16,384-token budget admit ~250 such images (≥64 tokens each) ≈ 0.4 GB pixels + 0.3 GB
    embeddings, simultaneous, on the worker process that also hosts the model. The L2 budget deliberately
    "excludes in-flight request inputs" (recipe cache-controls), so nothing caps this.
  • Consequence: memory-pressure/OOM risk under a hostile or careless multi-image request; bounded and
    serial (one request at a time), so degradation not corruption. Partially pre-existing (prepared mode
    also retains all of a request's assets at once); inline mode roughly doubles the footprint and makes
    tiny-but-expensive images trivially scriptable.
  • Action (cheap): document the per-request image bound next to the other limits, or cap images per
    request (e.g., a small constant) pending the GPU validation.

Notes (not findings)

  • Ownership is clean. Shared bounded decoding (image_decode.rs), shared preprocessing
    (image_preprocess.rs), and vision execution stay in the shared layer (src/models/qwen3_5/native,
    src/backends/cuda/qwen3_5); jev_vl keeps policy (image-source selection, pinned-processor validation,
    cache policy, warmup, response assembly) in src/models/jev_vl. Reusing ImageLimits::cua() as the
    shared default is safe here only because validate_export pins the processor metadata to exactly those
    values at startup (min 65,536 / max 16,777,216 pixels, patch 16, merge 2, mean/std 0.5) — a mismatch
    cannot start the worker.
  • Wire contract preserved. Same POST /v1/systemone single-question shape, same kinds
    choice/noul/score, same 4 MiB DefaultBodyLimit, same response fields; inline data URIs were already
    the image addressing scheme (prepared mode required the exact URI), so online mode changes where
    embeddings come from, not the envelope. Prepared mode remains the default; JEV_VL_VISION and
    JEV_VL_IMGCACHE are mutually exclusive with a startup error; ImageInput keeps the two worlds typed
    apart, and vision forward errors ("online vision is not loaded") cannot occur for prepared inputs.
  • Empty-suffix skip cannot silently drop an image. The skip (suffix_pads == 0 → suffix_asset = None) happens only in the L3-state Continue arm, where the image's contribution already lives in the
    cached prefix device state and the continuation ids still carry <|vision_end|> + tail. When no live
    state exists, the rebuild arm reloads the image (match suffix_asset { None => self.load_image(...) }),
    and tests/jev_vl/processing.rs::aligned_image_end_still_loads_images_without_a_live_prefix pins exactly
    that case. structure_key hashes every state part including the image URL, so a warm record can never
    bypass validation of a different image, and a malformed image can never reach a warm record (its cold
    pass would have failed). Suffix image placeholders are rejected.
  • Bounded decoding verified line-by-line. MIME prefix whitelist + guess_format cross-check; base64
    and decoded 4 MiB caps; 2048-side / 1,048,576-pixel / 200-aspect checks matching the HF processor's
    rejection rule; image-crate Limits duplicated defensively; APNG rejected via is_apng(); the JPEG
    header-segment MPF/MPO walk is correct (fill-byte skipping, standalone markers, SOS/EOI break, APP2
    MPF\0 sniff only inside real segment bounds); overflow-checked geometry (checked_mul throughout,
    tokens ≤ 32768, gt == 1).
  • Pillow parity of the 16-bit conversions personally verified. Reproducing the test's exact cases with
    Pillow 11.3.0: 16-bit grayscale (I;16) converts with clamp-to-255 ([128,256,65535] →
    128/255/255 triples — matching min(255)), gray+tRNS loads as I;16 (matching the png_grayscale
    color-type-0 branch), 16-bit gray+alpha / RGB / RGBA take the high byte (matching >> 8), alpha
    dropped. The pillow_rgb emulation matches local Pillow exactly for every mode the tests exercise.
  • Executor lifetime/ownership contract respected. Vision forward resolves under the same model mutex
    inside the scheduler admission (one question at a time); in-request duplicate images share one forward
    via pending_images; vision output validated (len == image_tokens * 5120, all finite) before use;
    on error both language and vision drains are still attempted (language_drain?; vision_drain?; with
    both futures materialized first); L2 and L3 publishes happen only after success; admission retained
    until drains complete.
  • Bounds fix is real and shared. DeviceBuffer::at debug_assert! → assert! (cuda.rs:266) makes
    the offset bound release-effective for all consumers (Cua included), with a registered weight-free test
    (tests/qwen3_5/device_buffer.rs). Vision-export bounds (0a3f38e) add the 27B dimension checks to
    export_vision.py (before any torch/safetensors import) and mirror them in Rust validate_config
    (hidden 1152 / out 5120 / 64 layers / 24 heads / 4 KV heads, vision==language config, exact frozen
    processor metadata), plus file-hash pins; the manifest is written last so a partial export is not
    loadable.
  • ABI: no bump. CS1_ABI_VERSION stays 6 (ops.h:16 identical at merge-base and head). The three v2
    vision symbols (cs1_vision_position_v2/rope_v2/attention_v2) are declared additive/optional and
    resolved with lib.get(...).ok(); the error surfaces only when online vision is actually used
    ("27B vision needs additive v2 CUDA symbols; rebuild libqwen3_5_cuda.so"). The recipe's "rebuilt ABI 6"
    requirement is therefore: same ABI version, library must be rebuilt from this revision to include the
    new symbols. Rust declarations and ops.h agree.
  • Warmup. Text warmup unchanged; the new online warmup is a genuine end-to-end inline-image decision
    before bind, pinned by tests/jev_vl/http.rs::online_vision_startup_warmup_is_inline_image_decision
    (grid [1,16,16], 64 tokens) — but see P3-3 for the doc gap.
  • Artifact hygiene over the full 39-file inventory: clean. No generated run output, no binary
    fixtures (all image fixtures are inline base64 in test bodies), no validation artifacts; Cargo.lock in
    lockstep; new deps minimal (image with default-features = false, features = ["png","jpeg"],
    base64, serde derive); new Rust test bodies all live under root tests/ with explicit [[test]]
    registrations or #[cfg(test)] #[path] wiring, per CONTRIBUTING.
  • PR-body numbers reproduce exactly (personally recomputed): GitHub 3-dot 39 files +3105/−1031;
    authored-code changed lines (excl. .md/Cargo.lock) 4050 vs #117-merged base and 1675
    (27 files +1498/−239) vs fixed #117 head c15a989 — both match the body's "Review scope" section.

Stacked accounting

  • Total (GitHub, 6b51652..953b691): 39 files, +3105/−1031; code-only 3043+/1007− = 4050 lines.
  • Authored (this PR's five commits: 2a41f7d, c47a909, 635c389, 0a3f38e, 953b691): raw per-commit
    sum 26 file-touches, +1569/−310 (code-only +1519/−298); net increment vs #117 head c15a989: 27 files,
    +1498/−239 (code-only 1675 lines).
  • Inherited:
    • #117's reviewed content (6480b6b "qwen: share vision execution for 4B and 27B models", 12 files
      +1327/−653) and its stack merges below the merge base.
    • #117's post-review advance (6480b6b..c15a989): the FIFO-eviction regression, doc tweaks, and —
      carried inside merge 20b480a with no named commit — the float_repr rewrite and its tests (see
      P2-1). This advance is approved-in-baseline only up to 6480b6b; c15a989 content beyond it has no
      review trail.
    • Main-side content below the merge base flows through the stack's merges.
  • 3k-rule state (draft): 4050 authored-code lines > 3000. The body provides split rationale ("The
    dependency inflates the main diff; rebase after #117 merges"), a component map + review order ("Review
    the #117 shared backend separately, then image decoding/export validation, JEV processing/execution,
    and their root-level tests"), and per-area validation with unverified items disclosed. The four
    self-review checkboxes are unchecked — consistent with the stated intentional draft. Preparation is
    substantively present; completed self-review remains outstanding.
  • PR body vs the current post-#138 template: the four "Required human contributor answers", the
    PR-necessity decision, and the bug-fix evidence section are absent (the body uses the older
    Purpose/Test Plan/Test Result/Demo/Self-review structure). DRAFT, so reported as state, not gated.

Commands run and results

Source inspected: all five authored commits' full diffs; head-state reads of image_decode.rs,
image_preprocess.rs, vision.rs, processing.rs, executor.rs, images.rs, caches.rs, engine.rs,
main.rs, lib.rs, ops.h, cuda.rs (diff), export_vision.py, preencode.py, recipe/model READMEs,
tests/{jev_vl,qwen3_5}/*, PR body, current template. Merge/blob forensics for json.rs provenance
(git rev-parse <rev>:<path>, --find-object, parent blob comparison).

Command Result Class
git fetch origin main 71857e033b2d personally run
shasum -a 256 review SKILL.md 58be3bc6…dde108 (match) personally run
git fetch origin pull/139/head (twice) 953b6914fbb2 pinned; later f88d9182219b personally run
gh pr view 139 OPEN/DRAFT, CONFLICTING, 39 files, body text observed
gh pr checks 139 / gh run view first query: no checks; ~30 min later: 4 pass + deploy skip at f88d918 (rust 1m38s, build 19s, cuda-contract 5s, benchmarks 13s); aggregated rust tests 212 passed / 0 failed / 28 ignored observed
python -m unittest discover -s tests/jev_vl -p 'test_export_vision.py' -v (worktree @ 953b691) 1 test, OK personally run
mkdocs build --strict (worktree @ 953b691) success (0.27 s, no warnings) personally run
Pillow 11.3.0 conversion probes for the 16-bit PNG modes match Rust test expectations (see Notes) personally run
Python 3.12.8 repr() oracle check for the 15 float_repr expectations 15/15 match personally run
Code-line accounting (git diff --numstat, per scope) 4050 / 1675 — matches PR body personally run
Author-reported local Rust checks at 953b691 fmt/clippy/test/build pass, 157 tests / 23 ignored (--offline --features omni-laya/serve) author-reported
CUDA/GPU correctness + latency for online vision vs autotrust/JEV-27B-VL@f34b598… pending per author author-reported (pending)

CI observed

At the pinned head 953b691: no runs existed. During the review the branch moved to f88d918 (merge of
main 71857e0; every reviewed PR file blob-identical, ci.yml = main's + the one authored line), where
GitHub CI is green: rust, build, cuda-contract, benchmarks pass; deploy skipped (runs
38247490258/38247490263). The 28 ignored tests are the GPU/checkpoint-gated ones; ignore ≠ GPU evidence.
The author's local command set (--features omni-laya/serve) matches main's current CI flags.

Material unverified checks

  1. No Rust toolchain on this host — cargo fmt/clippy/test/build not independently run at either
    head; author's local results and CI at f88d918 are the only executions of the Rust suite.
  2. No GPU — the online vision path's numerical parity (shared v2 kernels on the 27B layout) and
    latency vs the pinned checkpoint are unverified; the previously validated H800/2×2-corpus evidence
    covers the offline preencoded path only, exactly as the PR body concedes. The cuda-contract job is
    file-text checking, not kernel execution.
  3. Pillow parity beyond the tested modes (Adobe CMYK JPEG inversion, palette PNGs, exotic chunks)
    not verified; low likelihood, no evidence either way.
  4. The f88d918 merge of main introduces main's Laya/LFM/Valen/OmniJev content into the branch tree;
    those are main-reviewed contents and were not re-reviewed here, but the combined tree's integration
    (e.g., workspace builds across new crates) has only CI evidence at f88d918, not at the pinned head.
  5. Self-review checklist and human-intent template sections incomplete (draft state, disclosed).

…bounds

Address PR ThinkFlowLab#139 review: the architecture page still claimed image
features must be prepared offline; the recipe documented neither the
online vision warmup that now gates readiness nor the transient host
memory a many-image online request holds outside the cache budgets.

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

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head d5246e22d: one new commit since my review at f88d918221 — "docs: record online image support, warmup readiness, and per-request bounds" (3 files, docs-only); the eight reviewed commits are patch-identical (git range-diff, named refs, cat-file verified).

All four findings from my review are addressed, verified at the new head:

  • P2 (stealth shared-contract change) — addressed by disclosure: the PR body now carries a "Disclosed provenance exception" section that states the float_repr rewrite (+18/−12 in src/models/qwen3_5/native/src/json.rs with its tests) enters via #117's merge 20b480a with the result blob in neither parent and no named home, that it is shared by every Qwen consumer and not covered by #117's baseline approval, and commits to either giving it a named commit or excluding it before this stack merges. That is the attribution action the P2 required; the provenance item stays open until that named home exists, but it is now tracked in the PR itself rather than invisible.
  • P3 (architecture doc) — fixed: docs/architecture.md now reads "Images are either prepared offline by Transformers or supplied inline and encoded inside the worker by the shared native vision backend."
  • P3 (undocumented readiness gate) — fixed: recipe/jev_vl/README.md documents that with JEV_VL_VISION the worker binds only after a successful inline-image warmup (1×1 PNG through decode/preprocess/forward), so /health implies the online path ran once at startup, while still not validating per-user images.
  • P3 (unbounded in-flight buffers) — documented as suggested: the recipe states the ~1.5 MiB pixels + ~1.25 MiB embeddings per minimum image, the ~250-image/4 MiB-body worst case at ~0.7 GiB transient, that cache budgets exclude in-flight inputs, and sizes workloads accordingly.

mkdocs build --strict passes on the full tree at this head; fork CI (rust/build/benchmarks/cuda-contract) is green at this exact head. Zero actionable findings remain. Verdict: COMMENTED — still a draft, so no approval. Unverified (unchanged): no GPU on this host — online-vision numerical parity/latency vs the pinned reference remains author-run and pending (prior H800 evidence covers the offline path only); Rust suite covered by the exact-head fork CI; body template state (human answers/checklist) remains to be completed before leaving draft per the post-#138 template.

@linear3735
linear3735 marked this pull request as ready for review October 12, 2026 15:25
cua_s1 consumes the shared TensorStore header/offset/path checks
instead of a private duplicate (27B layouts stay rejected for the 4B
worker); the JEV cache controls document l2_hit/l2_miss as lookup
counters and pin the LRU-refresh on admission rechecks; the recipe
pins explicit eager vision (CUA_S1_VISION_GRAPH=0) for the documented
path. Adds the byte-level vision checkpoint regression suite, the
non-UTF8 JEV_VL_IMGCACHE fallback test, and GPU-gated 27B-layout v2
position/rope/attention kernel checks.

(Prepared as tree 60a8fbe by the codex jev session; landed after
independent review.)

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

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head ceadfd7d3b: one new commit since my review at d5246e22d — "docs: correct online image memory and cache semantics" (docs-only); the nine reviewed commits are patch-identical (git range-diff, named refs, cat-file verified).

The correction improves its own documentation's accuracy, reviewed and confirmed: embeddings are BF16 and 0.625 MiB (not 1.25 MiB as my earlier note echoed), pixel buffers are released as image inputs are consumed during vision resolution rather than held to request completion, cache hits may reuse validated assets or retained image-prefix state (refining the warmup wording), and the memory statement is now explicitly labeled "not a measured peak-memory bound". Zero actionable findings at this head.

Verdict: COMMENTED — still a draft. Unverified (unchanged): online-vision numerical parity/latency vs the pinned reference remains author-run and pending (no GPU on this host); the float_repr provenance exception tracked in the body stays open until the change has a named home; body template state to be completed before leaving draft.

linear3735 and others added 3 commits October 13, 2026 00:27
The sticky drain checks ran after `language_drain?; vision_drain?` would
have been fine only when the request succeeded; on a failed request the
first `?` on drain results replaced the root-cause error with a drain
error and dropped the original context entirely. Merge drain failures
into the existing result instead: the request error wins, and a drain
failure on an already-failed request is logged rather than substituted.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Re-listing all thirteen VisionConfig fields in a json! literal could
silently drop any member added later; serialize the parsed struct
instead so the shared model configuration always sees the full set.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The single existing test covered only three of the four backbone
dimension checks. Extend it to the top-level model_type and cover the
remaining gates in execution order: pre-existing output, the exact
eight-key processor contract (drifted value, extra key, missing key),
non-language adapter tensors, an empty vision tensor set, shard paths
escaping the model directory, and non-BF16 or non-finite weights; each
asserts weights are never saved and no partial output is left behind.
A happy-path test pins the manifest: exact format and model id, three
output file hashes and the three checkpoint pin hashes recomputed from
disk.

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

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-review #4 — PR #139 at 08a13924

Independent incremental re-review of four new commits since my last check at ceadfd7d3b. Local, no GitHub posting; draft state and evidence gaps reported as-is.

Incremental scope (pinned)

  • Live head re-fetched and pinned: git fetch origin "pull/139/head:refs/remotes/origin/pr/139" --force → 08a13924d03e70bc089b3866e79956bce1303e3e (matches GitHub headRefOid; branch codex/jev-vl-online-upstream). Review baseline pinned: git fetch origin ceadfd7d3...:refs/remotes/pin/139 → ceadfd7d3bc2d4189a44c0a43280a20cd2959f08, which is the direct parent of the first new commit (git rev-parse 08a13924~4 = ceadfd7d3), so all eleven previously reviewed commits are unchanged by construction; the four-commit range-diff shows the new commits only. Delta: 15 files, +1051/−200.
  • New commits: f0febb0 (tests: cache counting + shared checkpoint regressions), ea48a42 (fix: error precedence), 79c3b17 (fix: config derivation), 08a1392 (test: exporter rejection gates).
  • Body Review-scope claim re-verified exactly: after git fetch origin main, merge-base(head, origin/main=3e9cc33a) = 71857e033b2d2d0587742c1c1a5260d683ab17cd, and git diff 71857e0..08a13924 = 45 files, +4154/−1211 — matches the body's stated counts.
  • Status change: the PR is no longer draft (isDraft: false, OPEN). The body discloses this ("left draft for review intake on 2026-10-12") with the #117 dependency and current-head GPU validation still tracked as open, but all four self-review checkboxes remain unchecked.

Per-commit review

ea48a42 — keep the original request error when post-error drains also fail (src/models/jev_vl/native/src/executor.rs, +20/−3)

Before: language_drain?; vision_drain?; let probabilities = result?; — both drains always ran, but on a failed request the first failing drain error replaced the root-cause error via ?, dropping the failing stage's context. After: the request result is carried first; a drain error only becomes the returned error when the request otherwise succeeded (now with "language drain"/"vision drain" context, which the old raw ? also lacked); a drain failure on an already-failed request is logged (eprintln!, the established convention in this file at line 361) instead of substituted. Both drains are still always attempted, preserving the sticky-drain/admission contract, and the success path (L2 insert, prefix publish) is unchanged. Ordering is deterministic (language before vision). Sound.

Test coverage: none in the commit, and none exists — the drain path requires an initialized device model (model.synchronize()), so it is not CPU-testable on this host; consistent with repo routing of device paths to #[ignore]-gated tests, but no such test was added here. Noted as a residual gap, not a defect.

79c3b17 — derive the shared vision config from the parsed struct (src/models/cua_s1/native/src/vision/{mod,model}.rs, +13/−13)

Before, VisionModel::load re-listed all 13 VisionConfig fields into a json! literal before handing them to the shared omni_qwen3_5_native::vision::VisionConfig::from_value — a duplicated schema where any later field addition would be silently dropped. After: serde_json::to_value(c) serializes the parsed struct (Serialize derived). Verified non-divergent today: the struct has plain field names with no serde(rename)/defaults, so the serialized key set is byte-identical to the old literal, and both paths funnel through the same from_value (parse + pinned-layout validate). Behavior-identical; removes only the silent-drop risk, exactly as claimed. No dedicated test pins the round-trip; the new checkpoint suite exercises VisionConfig::load on valid configs indirectly. Acceptable.

f0febb0 — cache counting pins, shared checkpoint regressions, non-UTF8 fallback, GPU-gated v2 kernel checks (11 files, +796/−184)

  • tests/jev_vl/caches.rs (new, +73, registered [[test]] jev_vl_caches): three weight-free tests. Mutation check: l2_cold_rechecks_count_each_lookup catches removing the miss counter on cold lookups or on the disabled/over-budget early-return (an undercount that would silently corrupt /v1/cache/stats-based sizing decisions); l2_recheck_sees_a_queued_publication_and_refreshes_lru catches loss of LRU refresh on hit — without the refresh, the newer insert would evict queued instead of older and the hit/miss assertions flip; l2_disabled_or_over_budget_rechecks_remain_counted_misses catches counter loss on all three disabled modes and confirms no record/byte accounting leaks. These pin real, breakable behavior.
  • Source riding in a "tests:" commit: (1) cua_s1's private TensorStore/validate_header/inside/is_visual/UniqueMap (−182) deleted; the shared qwen3_5 implementations are made pub and consumed. I compared the deleted private code against the shared code side-by-side: textually identical logic (same signature, index/shard check, 100 MB header cap, checked offset arithmetic, duplicate/missing/unexpected rejections), so the dedup is behavior-preserving; and (2) JEV_VL_IMGCACHE is now carried as OsString and a non-UTF8 value is treated as unset (falling back to the default prepared cache dir), matching the prepared worker's env::var parity; the mutual-exclusion check consequently no longer fires for non-UTF8 imgcache + online vision. Both changes are disclosed in the commit message and the body. See findings for the labeling issue.
  • tests/cua_s1/vision_checkpoint.rs (new, +404): 8 weight-free regression tests building synthetic checkpoints (sparse zero payloads, hard links, no weights). assert_error panics on unexpected Ok and checks the message — fail-loud. The both_reject pattern runs each malformed input against both the shared QwenCheckpoint and cua's VisionCheckpoint, which is precisely the regression pin for the dedup above; coverage includes header length bounds, duplicate JSON keys, usize::MAX offsets on ignored language tensors, dtype/shape/missing/unexpected inventories (base BF16 and LoRA F32), index duplicates/mismatch/escaping shards, unix symlink escapes, and qwen_27b_layout_does_not_expand_cua_4b_support (shared validate accepts both layouts; cua's local pin stays 4B). Also pins inventory sizes (297 base / 100 adapter names) and marker bytes at exact offsets.
  • tests/qwen3_5/kernels.rs (+234): two new tests, both #[ignore = "needs a GPU and CUA_S1_CUDA_LIB with vision v2 symbols"] inside the already-registered [[test]] kernels target — matches repo precedent exactly and stays unexecuted here (no GPU). Bit-exact (to_bits) pins for v2 position-gather and RoPE at dh=72 over n ∈ {1, 3, 65} with pinned bf16 rounding points, and a padding-attention test whose workspace is pre-poisoned with 9.0 to catch missing zero-writes to padded columns, checked bit-exact against an unpack reference plus the existing FlashAttention relative-error metric and 1.6e-2 budget including tail rows. Good mutation-sensitive design; none of it has ever run.
  • tests/jev_vl/vision.rs (+29): non_utf8_imgcache_preserves_default_selection_and_online_mode (unix-only) pins the source semantics change above: non-UTF8 imgcache retains the default directory in prepared mode and does not conflict with online mode, while a non-UTF8 vision path still errors.
  • Docs: caches.rs doc comments define l2_hit/l2_miss as lookup counters ("not unique images or encoding attempts"; cold single image = 1 lookup prepared, 2 online), mirrored in recipe/jev_vl/README.md (counters, retained-prefix skip, same-path comparison advice) and a cross-reference in src/models/jev_vl/README.md. The recipe additionally pins CUA_S1_VISION_GRAPH=0 in the launch command and documents CUA_S1_VISION_GRAPH/CUA_S1_GRAPH_TRACE as == "1"-enabled, default-off — verified against code (src/models/qwen3_5/native/src/vision/model.rs:130,576), so the eager-path claim is accurate.

08a1392 — pin every vision exporter rejection gate with mocks (tests/jev_vl/test_export_vision.py, +228/−4)

All eight rejection gates of recipe/jev_vl/export_vision.py are now pinned, in execution order: (1) top-level model_type + the four-way backbone gate (the old test covered three of the four; the drift values are the 4B numbers against the 27B VALID_CONFIG, so they are non-vacuous — I verified tests/qwen3_5/data/config.json is the 1152/5120/5120 layout the exporter requires); (2) pre-existing output, asserted before any weights/dependencies load (the test does not even patch sys.modules, so it also pins the lazy-import ordering — any reordering fails loudly); (3) the exact eight-key processor contract with drifted-value, extra-key and missing-key variants plus read_weights.assert_not_called(), pinning whole-dict comparison; (4) non-language adapter tensors; (5) empty vision tensor set; (6) shard path escaping the model directory; (7) non-BF16 and non-finite weights (sentinel-object dtype identity, faked isfinite); each rejection additionally asserts save_weights.assert_not_called() and no partial output directory; (8) a happy-path manifest pin: format jev-vl-vision/1, model id autotrust/JEV-27B-VL, three output file hashes and three checkpoint pin hashes recomputed from disk with real hashlib. The mocks are minimal but sufficient and fail-loud: if the exporter changed the tensor API it uses or stopped raising, the tests error rather than pass. Commit message's "four backbone dimension checks" resolves as model_type + three sizes, all four now covered.

Still-open items re-verified at 08a13924

  1. P2 float_repr provenance exception — STAYS OPEN (tracked). The delta does not touch src/models/qwen3_5/native/src/json.rs or tests/qwen3_5/json.rs (0 changed lines in ceadfd7d3..08a13924), so the +18/−12 rewrite remains with no named home. The body's "Disclosed provenance exception" section is still present and still commits to "a named home (a commit in #117 or a separate small PR) or … excluded" before this stack merges. The commitment is intact but unfulfilled; this remains the blocking provenance item for merge.
  2. P3 architecture offline wording — still fixed. docs/architecture.md has zero delta lines since ceadfd7d3 (the corrected "supplied inline and encoded inside the worker by the shared native vision backend" wording stands).
  3. P3 readiness gate documentation — still fixed. recipe/jev_vl/README.md lines 56–57 and 100–102 still document that with JEV_VL_VISION the worker binds only after a successful 1×1-PNG inline warmup.
  4. P3 in-flight memory documentation — still fixed. The "Cache budgets exclude in-flight inputs … not a measured peak-memory bound" passage is present (recipe lines ~117–121), still with the corrected BF16/0.625-MiB embedding semantics from the ceadfd7d3 docs commit.
  5. Draft/template state — changed. Not draft anymore (disclosed "review intake" exit on 2026-10-12). The human-contributor answers section is filled; the four self-review checkboxes are all unchecked and the body itself says current-head GPU evidence and contributor/maintainer review remain outstanding. Per the post-#138 template expectation I recorded earlier (checkboxes completed before leaving draft), this is incomplete: the PR is now reviewable/approvable in principle while its own self-review section declares the outstanding items.

New findings

  • P3 (process, no code defect): f0febb0 is labeled tests: but carries two source changes — the shared-TensorStore dedup (−182 lines of cua_s1 checkpoint-validation code, now consuming newly-pub shared code) and the non-UTF8 JEV_VL_IMGCACHE semantics change (engine.rs, vision.rs). Both are disclosed in the commit message and the body, and both reviewed sound (textual-identity dedup; documented prepared-worker parity), but a conventional-commit type that understates src changes misleads changelog/bisect tooling. Splitting or relabeling (e.g. refactor: + fix:) would have been correct.
  • Observation (no action required): test-registration deviation. tests/cua_s1/vision_checkpoint.rs is wired via #[cfg(test)] #[path = ...] mod tests; inside vision/mod.rs rather than the repo's [[test]] precedent. This is technically necessary — the suite exercises the private VisionConfig::load via use super::*, which a root-level [[test]] target cannot — and it runs under cargo test -p omni-cua-s1-native/workspace runs, but it is a second registration idiom in one tree.
  • No new P0–P2 findings. Both fixes are correct and minimal; the tests genuinely pin (fail-loud) the behaviors they claim.

Evidence

Commands I ran personally (host: macOS arm64, no Rust toolchain, no GPU):

  • shasum -a 256 .agents/skills/system1-omni-review/SKILL.md → 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108 (matches pinned value; skill read from the working tree at /Users/hsliu/thinkflowlab/system1-omni, unpublished local copy, content-hash recorded).
  • Read docs/architecture.md and .agents/skills/system1-omni-review/references/repository-contracts.md (audited-baseline map; target code re-checked at head for all touched components).
  • Ref pinning and structure: fetch + rev-parse as above; git log/diff --stat/merge-base/range-diff for the delta; per-commit git show for all four commits; side-by-side read of deleted vs shared TensorStore code.
  • Weight-free Python tests (the files the commits touch), run against the exact head tree: working tree is not at head, so I extracted tests/jev_vl/test_export_vision.py, recipe/jev_vl/export_vision.py, tests/qwen3_5/data/config.json from refs/remotes/origin/pr/139 via git archive into /tmp/pr-review-2026-10-10/p30-py/ (repo untouched) and ran /Users/hsliu/miniconda3/bin/python …/test_export_vision.py -v → Ran 8 tests … OK (8/8 pass, 0.009 s), matching the body's claim. Host Python has no usable torch (import fails), which the suite is designed around (lazy exporter imports + sys.modules mocks).
  • mkdocs build --strict: not re-run — the delta contains zero changes under docs/ or mkdocs.yml (verified by empty diff); the touched READMEs are not part of that build's input delta.
  • CI at the live head: gh pr checks 139 → "no checks reported on the 'codex/jev-vl-online-upstream' branch"; API confirms 0 check runs and commit status pending with 0 statuses for 08a13924. The body discloses the same. Green fork CI evidence ends at ceadfd7d3 (body-linked runs 38265875700/38265875936).

Material unverified checks

  1. No CI at the live head. The four new commits have zero GitHub check runs. The author reports local cargo check --locked --tests clean for omni-jev-vl-native/omni-cua-s1-native, 19 Cua CPU tests, 8/8 exporter tests, and the f0febb0 tree's 40 CPU tests — all author-run, none corroborated by CI or by me (no Rust toolchain on this host). The Rust compile/test claims for f0febb0/ea48a42/79c3b17/08a1392 therefore rest solely on author-reported evidence.
  2. The two new v2 GPU kernel tests have never executed (GPU-gated #[ignore], no GPU here or in CI); they pin dh=72 27B-layout kernels whose parity remains author-claimed via the pending H800 run.
  3. Current-head GPU numerical parity and latency remain pending (unchanged): no online-vision comparison against pinned autotrust/JEV-27B-VL@f34b598d has been reported; historical prepared-image H800 evidence does not cover this path.
  4. The float_repr provenance item stays open until executed: the body's commitment (named home or exclusion before merge) is intact but not yet actioned; json.rs remains home-less in the diff.
  5. Self-review checkboxes unchecked; #117 dependency still open (the PR includes #117's shared backend in its displayed diff by the author's own accounting).

Verdict: COMMENTED. The four new commits are sound and their tests are genuine pins; no new P0–P2 findings. The PR leaving draft with unchecked self-review boxes, no CI at the head, the still-unexecuted provenance commitment, and pending current-head GPU evidence keep this at comment level — none of these imply approval or completed validation.

linear3735 and others added 7 commits October 13, 2026 10:56
Resolve native_vision_graph.md in favor of upstream's CUA_S1_GRAPH
semantics (text and multimodal language execution) while keeping this
branch's FIFO scratch/Graph residency and the shared-crate regression
commands.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
The 4 MiB axum DefaultBodyLimit rejected every request carrying even one
maximum-size image (4 MiB raw ≈ 5.33 MiB base64) before the structured
validators ran, so the advertised per-image caps were unreachable and
their BadRequest rejections dead code. Over-limit clients also got
axum's plain-text 413 instead of the worker's error envelope.

Raise the transport cap to 8 MiB (the number the Cua-S1 vision worker
already uses) so at least one maximum-size image fits, and map transport
rejections into the official envelope with status 413. Pin both halves
with raw-HTTP tests through the production limit layer: an over-limit
body returns the 413 envelope, and a 5 MiB body now reaches request
validation instead of dying in transport.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Engine::load streamed the full vision.safetensors through sha256 on a
Tokio worker thread. Move validate_export into spawn_blocking so the
GB-scale startup pass no longer occupies the async runtime; the
executor's device upload already re-reads the same file once.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…ring

ImageLimits and preprocess_rgb8_with_limits had public visibility with a
single consumer (the cua() preset behind preprocess_rgb8); keeping them
crate-private keeps the limit surface single-sourced so a future caller
cannot compose caps the model side never validated. url_key and the
export pin check each hand-rolled digest-to-lowercase-hex formatting;
route both through contract::hex_digest so the L2 keys, imgcache
directories and manifest pin hashes cannot drift apart.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The body-mapping fallback mapped every non-413 transport rejection to
400; a mid-read connection failure is axum's 500, not a client error.
Add Reject::internal and route that branch through it.

Co-Authored-By: Claude Code <noreply@anthropic.com>
A truncated upload surfaces as axum's UnknownBodyError, which is 400;
mapping it to 500 relabeled an interrupted client delivery as a server
fault (and broke the status the un-mapped route already returned). Keep
the envelope but mirror axum's statuses: 413 over the cap, 400 for
other buffer errors, so Reject::internal is removed again. Returning
Reject from the mapper, with each handler rendering the response, also
keeps the helper's error type small enough for clippy::result_large_err
under -D warnings. Pin the 400 path with a truncated-upload regression.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The recipe README moved to 8 MiB with the transport fix; the model
contract page still said 4 MiB.

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

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-review #5 — PR #139 at 26dc76b

Independent incremental re-review of one upstream merge plus seven new commits since my last check at 08a13924. Local only, no GitHub posting; state and evidence gaps reported as-is.

Incremental scope (pinned)

  • Live head re-fetched and pinned: git fetch origin "pull/139/head:refs/remotes/origin/pr/139" --force → 26dc76bf65c544815e7b285866791da1e703816d (matches GitHub headRefOid; branch codex/jev-vl-online-upstream; state OPEN, not draft, MERGEABLE). Review baseline pinned at 08a13924d03e70bc089b3866e79956bce1303e3e (prior review head), which is e6a6934^1, so all fifteen previously reviewed commits are unchanged ancestors by construction.
  • New content: merge e6a6934 ("Merge upstream main into the online-vision branch", parents 08a13924 + main tip 3e9cc33a) and seven commits adf7ffc, a5be750, 1cde60f, 89559ad, 4ea23c6, 0f834a9, 26dc76b. Main has not advanced past 3e9cc33a since the merge.
  • Delta accounting (measured): git diff 08a13924..26dc76b = 58 files, +4125/−111 (merge-inclusive; most of it main-absorbed). Authored post-merge content git diff e6a6934..26dc76b = 10 files, +189/−33. Whole-PR diff vs main = 46 files, +4329/−1229 — matches the refreshed body's stated counts exactly.

Per-commit review

a5be750 — let the documented inline-image caps past the HTTP body limit

What was broken: the axum DefaultBodyLimit::max(4 << 20) applied to the whole POST body while the documented per-image cap is 4 MiB of decoded bytes; base64 inflates 4/3, so one maximum-size image is ~5.33 MiB encoded — every request carrying even one such image died in transport with axum's plain-text 413 before the structured validators ran, making the advertised caps (4 MiB decoded / 2048 px / 1,048,576 px / 4608 patches) unreachable and their rejection paths dead code.

The fix is correct on both halves: (1) MAX_BODY_BYTES: usize = 8 << 20 in contract.rs gives 5.33 MiB + JSON framing/text comfortable headroom, and (2) both body-consuming handlers (/v1/systemone, fallback) now take Result<Bytes, BytesRejection> and route through a shared body_bytes mapper that converts axum's rejection into the worker's envelope (413 → Reject::payload_too_large), so clients get structured JSON, not axum's plain text. GET-only routes take no body extractor, so no plain-text 413 path remains. Tests: over_limit_body_returns_envelope_413 (8 MiB+ body → 413 + envelope type/param/code/message containing "8 MiB") and body_within_transport_limit_reaches_request_validation (5 MiB body — above the old 4 MiB layer — now reaches validation → 404 unknown-model envelope), both raw-HTTP/1.0 through the production DefaultBodyLimit layer. Note the tests exercise the mapping via the fallback route (an Engine is not CPU-constructible); the mapper is the same shared code both handlers use.

4ea23c6 — keep the true status for non-limit body rejections (superseded one commit later)

Mapped every non-413 BytesRejection to a new Reject::internal (500), reasoning that a mid-read connection failure is a server fault. As applied to this extractor that was wrong: axum-core's BytesRejection has exactly two failure statuses (see 0f834a9), so a truncated client upload (400) was relabeled 500. Superseded by 0f834a9; Reject::internal was added here and removed there. Disclosed honestly in the body ("the truncated-upload probe previously returned 500 at 4ea23c6").

0f834a9 — mirror axum's 400 for non-limit body rejections (final state; correct)

I verified the claim against the pinned axum generation (workspace Cargo.lock: axum 0.8.9 / axum-core 0.5.6; monorepo tag axum-v0.8.9): axum-core's FailedToBufferBody composes LengthLimitError with #[status = PAYLOAD_TOO_LARGE] and UnknownBodyError with #[status = BAD_REQUEST], and BytesRejection composes exactly FailedToBufferBody — so 413-over-cap / 400-otherwise are axum's own statuses for this extractor, and there is no 500 variant to preserve. The mapper now returns Reject (not Response), each handler rendering it — the commit's clippy::result_large_err rationale — with statuses mirroring axum: 413 envelope over the cap, 400 envelope otherwise. The new truncated_body_keeps_axums_400_status regression pins the real failure mode end-to-end (Content-Length 100, 2 bytes delivered, EOF → 400 + BadRequestError/400 envelope), and it runs in CI. This matches the frontend transport contract: no frontend code changed; the worker's structured 400/413 passes through the proxy unmodified (no retries, no redirects, buffering preserved).

1cde60f — pin-check the vision export off the async runtime

vision::validate_export (streams the full GB-scale vision.safetensors through sha256) moved into tokio::task::spawn_blocking, awaited in Engine::load. Error propagation preserved: join failures get .context("vision export pin check failed to run"), the inner validate error propagates unchanged (??). The manifest.clone() is a one-time small-JSON startup cost. Cancellation semantics: if load is dropped mid-await the blocking task detaches and completes wastefully but harmlessly; load runs once before binding. Remaining load-path blocking I/O (fs::read of the manifest, LabelHead::load, Processor::load) is MB-scale and outside the commit's claim; no remaining GB-scale blocking call on the async runtime.

89559ad — crate-scope the image limits, share the hex digest rendering

Behavior-identical, verified. ImageLimits and preprocess_rgb8_with_limits in src/models/qwen3_5/native/src/image_preprocess.rs go pub → pub(crate): I grepped the whole head tree — the only external consumers use the still-pub preprocess_rgb8 (cua_s1 image_preprocess.rs/vision_processing.rs, jev_vl processing.rs, tests/jev_vl/http.rs), and the two now-private items are used only inside their own module, confirming the single-consumer claim. contract::hex_digest ({b:02x} per byte) replaces two hand-rolled identical formatters (url_key in jev_vl processing.rs, sha256 in jev_vl vision.rs) — byte-identical output; the doc's rationale (L2 keys / imgcache dirs / manifest pins cannot drift) is accurate.

adf7ffc — rustfmt the shared vision config derivation

Pure line re-wrapping of the VisionConfig::from_value(serde_json::to_value(c)?) call in cua_s1 vision/model.rs (2+/3−). No semantic delta.

26dc76b — sync the model contract body cap to 8 MiB

One line in src/models/jev_vl/README.md ("4 MiB" → "8 MiB"). Verified code and docs now agree everywhere: MAX_BODY_BYTES = 8 << 20 (contract.rs:83), recipe line 110 ("full HTTP body is limited to 8 MiB, including base64 and text, so at least one maximum-size image fits; ... status 413 in the same error envelope"), model README line 64. Remaining "4 MiB" strings are the per-image decoded cap (recipe:109) and an unrelated read-block size in export_vision.py — both still accurate.

Merge e6a6934 — resolutions reviewed

git diff-tree --cc --name-only shows exactly two files differing from both parents:

  • Cargo.lock — dependency union of both sides; consistency corroborated by the CI build/rust jobs passing with --locked at the merge descendant.
  • recipe/cua_s1/native_vision_graph.md — a genuine union: main's updated wording ("text and multimodal language execution", "Enable each switch separately", CUA_S1_GRAPH=0 bench prefixes from #103/#106) combined with the branch's newer shared-executor text (shared Qwen vision executor, at most four exact-grid scratch/Graph pairs FIFO, "Enabling vision Graph alone does not enable language Graph replay"). Both sides' contributions present; consistent with the retained test (below).

The #109-class risk (merges silently dropping tests) checked explicitly: git ls-tree inventories of tests/ across both parents, the merge, and the head — nothing from the branch side was lost, and the only file absent vs main is tests/cua_s1/vision_graph.rs. That deletion is compile-forced, not silent: at the merge, omni-cua-s1-native no longer contains any graph_enabled/scratch vision-graph machinery (the branch's #117 refactor moved vision graph execution into the shared omni-qwen3-5-native::vision::VisionModel), so main's test references a type that no longer exists. Equivalent coverage survives as the branch's tests/qwen3_5/vision_graph.rs (same changed-pixels bit-exact oracle, 29-stage trace pin, capture assertions, adapted to the four-grid scratch), and main's new tests/qwen3_5/multimodal_graph.rs passes through byte-identical (not in the --cc set).

Open items re-verified at 26dc76b

  1. float_repr provenance — OPEN, with real progress. src/models/qwen3_5/native/src/json.rs has zero delta in 08a13924..26dc76b (no commits touch it; the merge brought none). The refreshed body's provenance section now cites fork commit 900ad7a as a named home; I verified it exists, touches exactly json.rs + its tests, says it was extracted "as requested by the #139 review", and its json.rs blob SHA (95578103d6ce589d5fba0e14a3dc121ef379cd34) is byte-identical to the PR head's. However, it is still only in the author's fork — #117 remains at c15a9893 (OPEN, non-draft, matching the body's dependency pin) without it, and no separate upstream PR exists. The body's own bar ("included as a named commit in #117 or submitted as a separate upstream PR ... before this stack merges") is intact and still unmet.
  2. f0febb0 commit-type P3 — still present. The series was not restructured; f0febb0 ("tests:") remains in history unchanged (tree 60a8fbe9) and still carries the TensorStore dedup + non-UTF8 JEV_VL_IMGCACHE source changes.
  3. P3 architecture offline wording — still fixed. docs/architecture.md has zero delta since 08a13924; the "Images are either prepared offline by Transformers or supplied inline and encoded inside the worker by the shared native vision backend" wording stands (lines 66–68 at head). The merge did not regress it.
  4. P3 readiness-gate docs — still fixed (recipe lines ~56–59: readiness also requires a successful inline-image warmup with JEV_VL_VISION).
  5. P3 in-flight memory docs — still fixed (recipe lines ~116–122: cache budgets exclude in-flight inputs; "not a measured peak-memory bound").
  6. State/template. OPEN, not draft (unchanged from round 4). The body was substantially refreshed for the merge: head SHA, dependency pin, diff-vs-main counts (verified exact), CI links (verified to match run IDs and head SHA), and an honest account of the 400→500→400 repair. The four self-review declarations remain unchecked, explicitly framed as requiring the contributor's own confirmation; the listed acceptance items are the #117 dependency, the provenance upstream path, current-head GPU/parity/latency, human self-review, maintainer review.
  7. CI — gap closed. gh pr checks 139 at the live head: rust, build, benchmarks, cuda-contract = success; deploy = skipped, all five check runs with head_sha = 26dc76b... (runs 38344236367 / 38344236333 — the same runs the body links). The rust job covers fmt, strict clippy (--features omni-laya/serve), workspace tests, and release build; the benchmarks job also runs unittest discover -s tests/jev_vl -p 'test_export_vision.py'. The legacy combined status shows pending with 0 statuses (check-runs API only), same as prior rounds.

New findings

  • No new P0–P2. The transport chain's final state is correct and each behavior is test-pinned; the refactor and docs commits are behavior-identical; the merge resolutions are sound and test-complete.
  • Observation (process, no head defect): intermediate-commit hygiene in this push. Three of the seven commits are individually unclean: 4ea23c6 introduced a substantively wrong mapping (truncated uploads → 500) reverted one commit later; a5be750 introduced contract::hex_digest with zero consumers, so that commit alone would fail strict clippy's dead_code under -D warnings until 89559ad wired it up. All disclosed or self-corrected at head; noted as bisect/series-hygiene noise in a draft-review-intake PR, consistent with the earlier f0febb0 type-mislabel P3.
  • Observation (test scoping): the over-limit and within-limit tests drive the production limit layer through the fallback route (no Engine needed); the /v1/systemone handler shares the identical body_bytes mapper, so the mapping is covered, but no test drives the real route's rejection path end-to-end (needs a loaded model).

Evidence

Commands I ran personally (host: macOS arm64, no Rust toolchain, no GPU; repo untouched — head content extracted via git archive into /tmp/pr-review-2026-10-10/p34-py/):

  • shasum -a 256 .agents/skills/system1-omni-review/SKILL.md → 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108 (matches pinned value; skill read from the working tree at /Users/hsliu/thinkflowlab/system1-omni, unpublished local copy, content-hash recorded).
  • Read docs/architecture.md (working tree + head revision via git show), .agents/skills/system1-omni-review/references/repository-contracts.md, prior review bodies (body-139-post.md, body-139-p27.md, body-139-p30.md).
  • Ref pinning: fetch + rev-parse as above; git log/show --stat per commit; git diff-tree --cc for the merge; ls-tree test inventories across e6a6934^1, e6a6934^2, e6a6934, head; blob-SHA comparison for the provenance fork commit; grep sweeps for MiB claims, hex_digest, ImageLimits/preprocess_rgb8* consumers, cua_s1 graph symbols.
  • axum semantics: Cargo.lock pins axum 0.8.9 / axum-core 0.5.6; direct network is sandboxed (curl/WebFetch to raw.githubusercontent.com failed), so I read the rejection/extractor sources via gh api at tag axum-v0.8.9 (where axum-core is 0.5.5; 0.5.6 is a patch of the same extraction code). LengthLimitError → 413, UnknownBodyError → 400, BytesRejection → FailedToBufferBody only. The CI-passing truncated-upload test at the actually-pinned 0.5.6 corroborates the behavior at head.
  • Weight-free Python suite (tests/jev_vl), run at the exact head tree: /Users/hsliu/miniconda3/bin/python tests/jev_vl/test_export_vision.py -v → Ran 8 tests … OK (8/8, 0.009 s). Suite is unchanged since 08a1392 (empty diff) and is additionally pinned in CI's benchmarks job.
  • CI at live head: gh pr view 139 (OPEN, not draft, MERGEABLE, head 26dc76bf…); gh pr checks 139 and gh api .../commits/26dc76bf…/check-runs → rust/build/benchmarks/cuda-contract success, deploy skipped, all on the head SHA.
  • mkdocs build --strict: not run — the authored delta touches no docs/ or mkdocs.yml input; the two README edits are outside the docs build. CI's docs workflow passed at the merge (run 38344236333 per the body; deploy skipped).

Material unverified checks

  1. Rust execution is CI-corroborated, not personally run. No Rust toolchain on this host: the five worker HTTP/CPU tests (including the truncated-upload 400 regression), strict clippy, fmt, and the release build are verified only through the green CI runs at 26dc76b, which I confirmed are pinned to the head SHA. The author's local clippy/test claims for the intermediate commits rest on author evidence.
  2. Current-head GPU numerical parity and latency remain pending (unchanged): no online-vision comparison against pinned autotrust/JEV-27B-VL@f34b598d at this head; the H800 plan is documented in the body but not executed. The GPU-gated v2 kernel tests from f0febb0 and tests/qwen3_5/{vision_graph,multimodal_graph}.rs remain #[ignore]-gated and never executed here or in CI.
  3. The float_repr upstream home does not exist yet: fork commit 900ad7a is verified real and byte-identical, but #117 (c15a9893) does not contain it and no upstream PR carries it — the body's pre-merge commitment stays open until one of the two paths is completed or the change is excluded.
  4. Cargo.lock merge resolution is consistent-with-CI (--locked builds green) but I could not run cargo metadata/vendor verification locally.
  5. Self-review declarations unchecked; #117 dependency still open (its shared backend remains inside this displayed diff).

Verdict: COMMENTED. The merge and the seven commits are sound — the transport cap raise fixes a real dead-code/unreachable-caps defect, the 413/400 mapping now exactly mirrors axum's own rejection statuses with fail-loud regressions, and the merge dropped nothing that still compiled. No new P0–P2. The provenance commitment (now with a verified fork home but no upstream path), the unchecked self-review declarations, the pending current-head GPU evidence, and the open #117 dependency keep this at comment level — none of these imply approval or completed validation.

Levius-Fubuki pushed a commit that referenced this pull request Oct 13, 2026
… parity

float_repr renders structured state into the prompt of every
shared-Qwen consumer (Cua-S1, Open-Jev, JEV-VL). Rust's {:e} picks the
upper digit at some exact shortest-decimal ties, while Python repr()
breaks them to even — a divergence the old doc comment admitted.
Select digits via serde_json's default formatter (ryu) instead, and
pin 15 Python repr() examples and five bit-exact tie cases in tests.

Provenance: the change arrived inside #117's merge 20b480a ("Merge
main and preserve shared vision graph regressions", authored by
levius); its result blobs match neither parent and belong to no named
commit. Extracted unchanged here to give it a named, bisectable home,
as requested by the #139 review.

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants