test(e2e): on-device multi-model mmproj invariants (quant-dedup + family separation)#611
Conversation
…ily separation)
Adds a device e2e that proves, from un-fakeable disk state + the app's own debug
log (no UI assertions), the two properties the A1 mmproj fix guarantees when more
than one vision model coexists on a device:
A) QUANT DEDUP — a 2nd quantization of the SAME model reuses the one on-disk
mmproj (mmProjLocalName is quant-independent; checkMmProjExists skips the
sidecar). Asserts: weights>=2, mmproj==1, and the reuse shows in the log.
B) FAMILY SEPARATION — two models of the same family (Gemma 4 E2B vs E4B) each
keep their OWN mmproj and never mispair. Asserts: two DISTINCT mmproj files
on disk; E2B weights pair the e2b mmproj AND E4B weights pair the e4b mmproj;
ZERO cross-pairing (e2b->e4b / e4b->e2b == 0); no "multimodal support not
enabled". The pairing signal is the app's own decision, logged either at boot
(linkOrphanMmProj "<weights> -- linking <mmproj>") or at load (WIRE-VISION) --
accept either, since the boot line is deterministic and needs no message-send.
PLATFORM=ios uses devicectl; PLATFORM=android uses adb. Phases baseline|dedup|family.
Verified on a real iPhone (all three phases green): 5 model families coexisting --
Qwen 0.8B, Qwen 2B, SmolVLM, Gemma E2B (two quants sharing one mmproj), Gemma E4B --
each paired to its OWN correctly-named mmproj, no mispairing. Android verified for
baseline (distinct e2b + e4b mmproj on disk) and dedup; the E4B family-pairing cell
needs the 5.6GB weights, blocked on that device by crippled WiFi (~39 KB/s), not code.
The shared naming/pairing logic (modelIdentityStem / mmProjLocalName /
mmProjBelongsToModel) is identical on both platforms and is already guarded by the
committed unit + live-HF-fixture tests; this script is the operator-run device gate.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughAdds device E2E coverage for vision model downloads and initialization. The tests inspect Android/iOS model artifacts and logs, validate mmproj deduplication and family pairing, and confirm Android vision initialization. ChangesVision device E2E tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/e2e/device/visionMmprojMultiModel.e2e.sh`:
- Around line 39-41: Update the iOS log retrieval flow around the devicectl copy
command to use a unique mktemp output file instead of /tmp/mm-e2e-ios.log, stop
the phase immediately when the copy fails, and remove the temporary file
afterward. Keep the log-reading behavior for successful pulls while ensuring no
stale or symlinked file can influence the result.
- Around line 75-93: Update pair_ok and the final pairing assertions so success
requires a correct-family [WIRE-VISION] entry containing initialized:true for
both E2B and E4B. Do not treat linkOrphanMmProj output as proof of
initialization; retain those logs only for diagnostic output, and preserve the
existing forbidden-family mispair checks and error conditions.
- Around line 62-63: Update the deduplication assertion using REUSE so the phase
only passes when the reuse log is non-empty and matches the expected
model-specific projector reuse record. Keep the existing N and WEIGHTS checks,
but require REUSE in the predicate to reject cases where the projector was
re-downloaded or replaced.
- Around line 51-98: Update the phase dispatch case around baseline, dedup, and
family to add an explicit wildcard branch that reports the invalid phase and
exits nonzero. Preserve the existing behavior and success status for the three
supported phases while ensuring unknown arguments cannot fall through
successfully.
In `@__tests__/e2e/device/visionModelOnDevice.android.e2e.sh`:
- Line 32: Update the SHOT function to capture screenshots at a uniquely
generated temporary path using mktemp, rather than the predictable
/tmp/e2e-android.png path. Add cleanup for the generated file through an
appropriate shell trap while preserving the existing adb screencap behavior.
- Around line 7-10: Update the vision model E2E assertion to truncate or
baseline offgrid-debug.log before loading, then poll only newly appended content
until a deadline for the fresh “[WIRE-VISION]” initialized event, accommodating
slow devices instead of relying on a fixed sleep. Assert the fresh log window
contains vision support and does not contain “Multimodal support not enabled,”
using the existing test flow and log variables.
- Around line 48-50: Update the vision model E2E flow around the download,
picker selection, and [WIRE-VISION] assertion to require a completed .gguf file,
preserve its exact basename, and verify/select that same model before inference;
match the exact basename in the logged model field rather than only the query
stem. Validate that the normalized query stem is non-empty before proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9d365b3-3688-4f62-af6e-0ed57abd1ff2
📒 Files selected for processing (2)
__tests__/e2e/device/visionMmprojMultiModel.e2e.sh__tests__/e2e/device/visionModelOnDevice.android.e2e.sh
| xcrun devicectl device copy from --device "$IOS_UDID" --domain-type appDataContainer \ | ||
| --domain-identifier "$PKG" --source Documents/offgrid-debug.log --destination /tmp/mm-e2e-ios.log >/dev/null 2>&1 | ||
| cat /tmp/mm-e2e-ios.log 2>/dev/null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid reading a stale iOS log after a failed pull.
copy from failures are suppressed, then the script reads a persistent /tmp/mm-e2e-ios.log. A failed current pull can therefore reuse a prior run’s pairing/initialization evidence and falsely pass. Use a unique mktemp file, fail the phase when the copy fails, and remove it afterward; this also removes the predictable-path symlink risk.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 39-39: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/mm-e2e-ios.log
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 40-40: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/mm-e2e-ios.log
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionMmprojMultiModel.e2e.sh` around lines 39 - 41,
Update the iOS log retrieval flow around the devicectl copy command to use a
unique mktemp output file instead of /tmp/mm-e2e-ios.log, stop the phase
immediately when the copy fails, and remove the temporary file afterward. Keep
the log-reading behavior for successful pulls while ensuring no stale or
symlinked file can influence the result.
Source: Linters/SAST tools
| case "${1:?phase: baseline|dedup|family}" in | ||
| baseline) | ||
| echo "[$PLATFORM] gemma-4-e2b mmproj files: $(mmproj_count gemma-4-e2b)" | ||
| echo "[$PLATFORM] gemma-4-e4b mmproj files: $(mmproj_count gemma-4-e4b)" | ||
| models_ls | tr '\n' ' '; echo ;; | ||
|
|
||
| dedup) # run AFTER downloading a 2nd quant of gemma-4-E2B | ||
| N=$(mmproj_count gemma-4-e2b) | ||
| WEIGHTS=$(models_ls | grep -vi mmproj | grep -ic gemma-4-e2b) | ||
| echo "gemma-4-e2b: weights=$WEIGHTS mmproj=$N" | ||
| # the 2nd quant's finalization must report the mmproj already present (not re-downloaded) | ||
| REUSE=$(read_log | grep -aiE "mmproj already on disk|mmProjFileExists\":true.*gemma-4-e2b|mmproj.*skip" | tail -1) | ||
| if [ "$N" -eq 1 ] && [ "$WEIGHTS" -ge 2 ]; then | ||
| echo "PASS(A): 2 gemma-4-E2B quants share ONE mmproj (no re-download). reuse-log: ${REUSE:0:80}" | ||
| else | ||
| echo "FAIL(A): expected 1 mmproj + >=2 weights, got mmproj=$N weights=$WEIGHTS"; exit 1 | ||
| fi ;; | ||
|
|
||
| family) # run AFTER downloading gemma-4-E4B alongside E2B | ||
| E2B=$(mmproj_count gemma-4-e2b); E4B=$(mmproj_count gemma-4-e4b) | ||
| echo "mmproj files: e2b=$E2B e4b=$E4B" | ||
| DUMP=$(read_log) | ||
| ERR=$(echo "$DUMP" | grep -aic "multimodal support not enabled") | ||
|
|
||
| # PAIRING proof — the un-fakeable signal is the app's own pairing decision, logged two ways: | ||
| # 1. [linkOrphanMmProj] (fires at boot / finalization) prints "<weights> — linking <mmproj>". | ||
| # 2. [WIRE-VISION] (fires at model LOAD) prints the model + mmproj it initialized with. | ||
| # Family separation holds iff E2B weights pair the e2b mmproj AND E4B weights pair the e4b mmproj | ||
| # (NEVER the other family's). A load-triggered WIRE-VISION is nice-to-have but needs a manual | ||
| # message-send; the boot-time linkOrphanMmProj line is deterministic, so accept EITHER. | ||
| pair_ok() { # $1=weights-family stem $2=required mmproj stem | ||
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -i "$2" | tail -1 | ||
| } | ||
| mispair() { # $1=weights-family stem $2=FORBIDDEN mmproj stem (the other family's) | ||
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -ic "$2" | ||
| } | ||
| P_E2B=$(pair_ok "gemma-4-e2b-it-q" "gemma-4-e2b-it-mmproj") | ||
| P_E4B=$(pair_ok "gemma-4-e4b-it-q" "gemma-4-e4b-it-mmproj") | ||
| X_E2B=$(mispair "gemma-4-e2b-it-q" "gemma-4-e4b-it-mmproj") # E2B must NOT pair e4b mmproj | ||
| X_E4B=$(mispair "gemma-4-e4b-it-q" "gemma-4-e2b-it-mmproj") # E4B must NOT pair e2b mmproj | ||
| echo "pair e2b->e2b: ${P_E2B:+yes} | pair e4b->e4b: ${P_E4B:+yes} | mispairs: e2b->e4b=$X_E2B e4b->e2b=$X_E4B" | ||
| if [ "$E2B" -ge 1 ] && [ "$E4B" -ge 1 ] && [ -n "$P_E2B" ] && [ -n "$P_E4B" ] \ | ||
| && [ "$X_E2B" -eq 0 ] && [ "$X_E4B" -eq 0 ] && [ "$ERR" -eq 0 ]; then | ||
| echo "PASS(B): distinct mmproj on disk; E2B pairs its OWN mmproj and E4B pairs its OWN mmproj; no cross-pairing, no error" | ||
| else | ||
| echo "FAIL(B): e2b_files=$E2B e4b_files=$E4B e2b_paired=${P_E2B:+1} e4b_paired=${P_E4B:+1} mispair_e2b=$X_E2B mispair_e4b=$X_E4B errors=$ERR"; exit 1 | ||
| fi ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unknown phases explicitly.
An invalid phase falls through the case and exits successfully, allowing a typo in CI to report a false green result.
Proposed fix
family)
# ...
fi ;;
+ *)
+ echo "Unknown phase: $1 (expected baseline, dedup, or family)" >&2
+ exit 2 ;;
esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "${1:?phase: baseline|dedup|family}" in | |
| baseline) | |
| echo "[$PLATFORM] gemma-4-e2b mmproj files: $(mmproj_count gemma-4-e2b)" | |
| echo "[$PLATFORM] gemma-4-e4b mmproj files: $(mmproj_count gemma-4-e4b)" | |
| models_ls | tr '\n' ' '; echo ;; | |
| dedup) # run AFTER downloading a 2nd quant of gemma-4-E2B | |
| N=$(mmproj_count gemma-4-e2b) | |
| WEIGHTS=$(models_ls | grep -vi mmproj | grep -ic gemma-4-e2b) | |
| echo "gemma-4-e2b: weights=$WEIGHTS mmproj=$N" | |
| # the 2nd quant's finalization must report the mmproj already present (not re-downloaded) | |
| REUSE=$(read_log | grep -aiE "mmproj already on disk|mmProjFileExists\":true.*gemma-4-e2b|mmproj.*skip" | tail -1) | |
| if [ "$N" -eq 1 ] && [ "$WEIGHTS" -ge 2 ]; then | |
| echo "PASS(A): 2 gemma-4-E2B quants share ONE mmproj (no re-download). reuse-log: ${REUSE:0:80}" | |
| else | |
| echo "FAIL(A): expected 1 mmproj + >=2 weights, got mmproj=$N weights=$WEIGHTS"; exit 1 | |
| fi ;; | |
| family) # run AFTER downloading gemma-4-E4B alongside E2B | |
| E2B=$(mmproj_count gemma-4-e2b); E4B=$(mmproj_count gemma-4-e4b) | |
| echo "mmproj files: e2b=$E2B e4b=$E4B" | |
| DUMP=$(read_log) | |
| ERR=$(echo "$DUMP" | grep -aic "multimodal support not enabled") | |
| # PAIRING proof — the un-fakeable signal is the app's own pairing decision, logged two ways: | |
| # 1. [linkOrphanMmProj] (fires at boot / finalization) prints "<weights> — linking <mmproj>". | |
| # 2. [WIRE-VISION] (fires at model LOAD) prints the model + mmproj it initialized with. | |
| # Family separation holds iff E2B weights pair the e2b mmproj AND E4B weights pair the e4b mmproj | |
| # (NEVER the other family's). A load-triggered WIRE-VISION is nice-to-have but needs a manual | |
| # message-send; the boot-time linkOrphanMmProj line is deterministic, so accept EITHER. | |
| pair_ok() { # $1=weights-family stem $2=required mmproj stem | |
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -i "$2" | tail -1 | |
| } | |
| mispair() { # $1=weights-family stem $2=FORBIDDEN mmproj stem (the other family's) | |
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -ic "$2" | |
| } | |
| P_E2B=$(pair_ok "gemma-4-e2b-it-q" "gemma-4-e2b-it-mmproj") | |
| P_E4B=$(pair_ok "gemma-4-e4b-it-q" "gemma-4-e4b-it-mmproj") | |
| X_E2B=$(mispair "gemma-4-e2b-it-q" "gemma-4-e4b-it-mmproj") # E2B must NOT pair e4b mmproj | |
| X_E4B=$(mispair "gemma-4-e4b-it-q" "gemma-4-e2b-it-mmproj") # E4B must NOT pair e2b mmproj | |
| echo "pair e2b->e2b: ${P_E2B:+yes} | pair e4b->e4b: ${P_E4B:+yes} | mispairs: e2b->e4b=$X_E2B e4b->e2b=$X_E4B" | |
| if [ "$E2B" -ge 1 ] && [ "$E4B" -ge 1 ] && [ -n "$P_E2B" ] && [ -n "$P_E4B" ] \ | |
| && [ "$X_E2B" -eq 0 ] && [ "$X_E4B" -eq 0 ] && [ "$ERR" -eq 0 ]; then | |
| echo "PASS(B): distinct mmproj on disk; E2B pairs its OWN mmproj and E4B pairs its OWN mmproj; no cross-pairing, no error" | |
| else | |
| echo "FAIL(B): e2b_files=$E2B e4b_files=$E4B e2b_paired=${P_E2B:+1} e4b_paired=${P_E4B:+1} mispair_e2b=$X_E2B mispair_e4b=$X_E4B errors=$ERR"; exit 1 | |
| fi ;; | |
| esac | |
| case "${1:?phase: baseline|dedup|family}" in | |
| baseline) | |
| echo "[$PLATFORM] gemma-4-e2b mmproj files: $(mmproj_count gemma-4-e2b)" | |
| echo "[$PLATFORM] gemma-4-e4b mmproj files: $(mmproj_count gemma-4-e4b)" | |
| models_ls | tr '\n' ' '; echo ;; | |
| dedup) # run AFTER downloading a 2nd quant of gemma-4-E2B | |
| N=$(mmproj_count gemma-4-e2b) | |
| WEIGHTS=$(models_ls | grep -vi mmproj | grep -ic gemma-4-e2b) | |
| echo "gemma-4-e2b: weights=$WEIGHTS mmproj=$N" | |
| # the 2nd quant's finalization must report the mmproj already present (not re-downloaded) | |
| REUSE=$(read_log | grep -aiE "mmproj already on disk|mmProjFileExists\":true.*gemma-4-e2b|mmproj.*skip" | tail -1) | |
| if [ "$N" -eq 1 ] && [ "$WEIGHTS" -ge 2 ]; then | |
| echo "PASS(A): 2 gemma-4-E2B quants share ONE mmproj (no re-download). reuse-log: ${REUSE:0:80}" | |
| else | |
| echo "FAIL(A): expected 1 mmproj + >=2 weights, got mmproj=$N weights=$WEIGHTS"; exit 1 | |
| fi ;; | |
| family) # run AFTER downloading gemma-4-E4B alongside E2B | |
| E2B=$(mmproj_count gemma-4-e2b); E4B=$(mmproj_count gemma-4-e4b) | |
| echo "mmproj files: e2b=$E2B e4b=$E4B" | |
| DUMP=$(read_log) | |
| ERR=$(echo "$DUMP" | grep -aic "multimodal support not enabled") | |
| # PAIRING proof — the un-fakeable signal is the app's own pairing decision, logged two ways: | |
| # 1. [linkOrphanMmProj] (fires at boot / finalization) prints "<weights> — linking <mmproj>". | |
| # 2. [WIRE-VISION] (fires at model LOAD) prints the model + mmproj it initialized with. | |
| # Family separation holds iff E2B weights pair the e2b mmproj AND E4B weights pair the e4b mmproj | |
| # (NEVER the other family's). A load-triggered WIRE-VISION is nice-to-have but needs a manual | |
| # message-send; the boot-time linkOrphanMmProj line is deterministic, so accept EITHER. | |
| pair_ok() { # $1=weights-family stem $2=required mmproj stem | |
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -i "$2" | tail -1 | |
| } | |
| mispair() { # $1=weights-family stem $2=FORBIDDEN mmproj stem (the other family's) | |
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -ic "$2" | |
| } | |
| P_E2B=$(pair_ok "gemma-4-e2b-it-q" "gemma-4-e2b-it-mmproj") | |
| P_E4B=$(pair_ok "gemma-4-e4b-it-q" "gemma-4-e4b-it-mmproj") | |
| X_E2B=$(mispair "gemma-4-e2b-it-q" "gemma-4-e2b-it-mmproj") # E2B must NOT pair e4b mmproj | |
| X_E4B=$(mispair "gemma-4-e4b-it-q" "gemma-4-e2b-it-mmproj") # E4B must NOT pair e2b mmproj | |
| echo "pair e2b->e2b: ${P_E2B:+yes} | pair e4b->e4b: ${P_E4B:+yes} | mispairs: e2b->e4b=$X_E2B e4b->e2b=$X_E4B" | |
| if [ "$E2B" -ge 1 ] && [ "$E4B" -ge 1 ] && [ -n "$P_E2B" ] && [ -n "$P_E4B" ] \ | |
| && [ "$X_E2B" -eq 0 ] && [ "$X_E4B" -eq 0 ] && [ "$ERR" -eq 0 ]; then | |
| echo "PASS(B): distinct mmproj on disk; E2B pairs its OWN mmproj and E4B pairs its OWN mmproj; no cross-pairing, no error" | |
| else | |
| echo "FAIL(B): e2b_files=$E2B e4b_files=$E4B e2b_paired=${P_E2B:+1} e4b_paired=${P_E4B:+1} mispair_e2b=$X_E2B mispair_e4b=$X_E4B errors=$ERR"; exit 1 | |
| fi ;; | |
| *) | |
| echo "Unknown phase: $1 (expected baseline, dedup, or family)" >&2 | |
| exit 2 ;; | |
| esac |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 93-93: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 93-93: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 51-51: Add a default case (*) to handle unexpected values.
[failure] 63-63: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 93-93: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 82-82: Assign this positional parameter to a local variable.
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 84-84: Add an explicit return statement at the end of the function.
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 63-63: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 82-82: Assign this positional parameter to a local variable.
[warning] 94-94: Redirect this error message to stderr (>&2).
[warning] 85-85: Assign this positional parameter to a local variable.
[warning] 85-85: Assign this positional parameter to a local variable.
[warning] 81-81: Add an explicit return statement at the end of the function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionMmprojMultiModel.e2e.sh` around lines 51 - 98,
Update the phase dispatch case around baseline, dedup, and family to add an
explicit wildcard branch that reports the invalid phase and exits nonzero.
Preserve the existing behavior and success status for the three supported phases
while ensuring unknown arguments cannot fall through successfully.
Source: Linters/SAST tools
| REUSE=$(read_log | grep -aiE "mmproj already on disk|mmProjFileExists\":true.*gemma-4-e2b|mmproj.*skip" | tail -1) | ||
| if [ "$N" -eq 1 ] && [ "$WEIGHTS" -ge 2 ]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the reuse log for the dedup assertion.
REUSE is only printed. The phase passes whenever disk state is 1 mmproj and >=2 weights, even if the second quant re-downloaded or replaced the projector. Include a non-empty, model-specific reuse record in the predicate.
Proposed fix
- if [ "$N" -eq 1 ] && [ "$WEIGHTS" -ge 2 ]; then
+ if [[ "$N" -eq 1 && "$WEIGHTS" -ge 2 && -n "$REUSE" ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| REUSE=$(read_log | grep -aiE "mmproj already on disk|mmProjFileExists\":true.*gemma-4-e2b|mmproj.*skip" | tail -1) | |
| if [ "$N" -eq 1 ] && [ "$WEIGHTS" -ge 2 ]; then | |
| REUSE=$(read_log | grep -aiE "mmproj already on disk|mmProjFileExists\":true.*gemma-4-e2b|mmproj.*skip" | tail -1) | |
| if [[ "$N" -eq 1 && "$WEIGHTS" -ge 2 && -n "$REUSE" ]]; then |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 63-63: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 63-63: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionMmprojMultiModel.e2e.sh` around lines 62 - 63,
Update the deduplication assertion using REUSE so the phase only passes when the
reuse log is non-empty and matches the expected model-specific projector reuse
record. Keep the existing N and WEIGHTS checks, but require REUSE in the
predicate to reject cases where the projector was re-downloaded or replaced.
| # PAIRING proof — the un-fakeable signal is the app's own pairing decision, logged two ways: | ||
| # 1. [linkOrphanMmProj] (fires at boot / finalization) prints "<weights> — linking <mmproj>". | ||
| # 2. [WIRE-VISION] (fires at model LOAD) prints the model + mmproj it initialized with. | ||
| # Family separation holds iff E2B weights pair the e2b mmproj AND E4B weights pair the e4b mmproj | ||
| # (NEVER the other family's). A load-triggered WIRE-VISION is nice-to-have but needs a manual | ||
| # message-send; the boot-time linkOrphanMmProj line is deterministic, so accept EITHER. | ||
| pair_ok() { # $1=weights-family stem $2=required mmproj stem | ||
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -i "$2" | tail -1 | ||
| } | ||
| mispair() { # $1=weights-family stem $2=FORBIDDEN mmproj stem (the other family's) | ||
| echo "$DUMP" | grep -aiE "linkOrphanMmProj|WIRE-VISION" | grep -i "$1" | grep -ic "$2" | ||
| } | ||
| P_E2B=$(pair_ok "gemma-4-e2b-it-q" "gemma-4-e2b-it-mmproj") | ||
| P_E4B=$(pair_ok "gemma-4-e4b-it-q" "gemma-4-e4b-it-mmproj") | ||
| X_E2B=$(mispair "gemma-4-e2b-it-q" "gemma-4-e4b-it-mmproj") # E2B must NOT pair e4b mmproj | ||
| X_E4B=$(mispair "gemma-4-e4b-it-q" "gemma-4-e2b-it-mmproj") # E4B must NOT pair e2b mmproj | ||
| echo "pair e2b->e2b: ${P_E2B:+yes} | pair e4b->e4b: ${P_E4B:+yes} | mispairs: e2b->e4b=$X_E2B e4b->e2b=$X_E4B" | ||
| if [ "$E2B" -ge 1 ] && [ "$E4B" -ge 1 ] && [ -n "$P_E2B" ] && [ -n "$P_E4B" ] \ | ||
| && [ "$X_E2B" -eq 0 ] && [ "$X_E4B" -eq 0 ] && [ "$ERR" -eq 0 ]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require successful vision initialization for both families.
linkOrphanMmProj only proves an association decision; it does not prove either model loaded or vision initialized. The fallback lets this phase pass with no [WIRE-VISION] event—or with initialized:false—despite the stated E2E invariant. Require a correct-family [WIRE-VISION] entry with initialized:true for E2B and E4B; retain orphan-link logs only as diagnostics.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 93-93: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 93-93: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 93-93: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 82-82: Assign this positional parameter to a local variable.
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 84-84: Add an explicit return statement at the end of the function.
[failure] 92-92: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 82-82: Assign this positional parameter to a local variable.
[warning] 85-85: Assign this positional parameter to a local variable.
[warning] 85-85: Assign this positional parameter to a local variable.
[warning] 81-81: Add an explicit return statement at the end of the function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionMmprojMultiModel.e2e.sh` around lines 75 - 93,
Update pair_ok and the final pairing assertions so success requires a
correct-family [WIRE-VISION] entry containing initialized:true for both E2B and
E4B. Do not treat linkOrphanMmProj output as proof of initialization; retain
those logs only for diagnostic output, and preserve the existing
forbidden-family mispair checks and error conditions.
| # Assertion (the mmproj-fix proof): after the model loads, offgrid-debug.log must contain | ||
| # [WIRE-VISION] {... "initialized":true, "support":{"vision":true ...}} | ||
| # and MUST NOT contain "Multimodal support not enabled". The load-time init is the exact thing the | ||
| # fix restores; attaching an image is a bonus, not required to prove the fix. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Wait for a fresh initialization event instead of sampling the historical log once.
initMultimodal is asynchronous and [WIRE-VISION] is logged only afterward (src/services/llm.ts:218-229), so a fixed 20-second sleep can miss slow devices. Conversely, DUMP=$(LOG) includes previous runs, allowing a stale successful line to pass. Capture/truncate a baseline, poll only newly written log content until a deadline, and assert that the fresh window contains no "Multimodal support not enabled" error—the documented negative assertion is currently missing.
Also applies to: 74-74, 76-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionModelOnDevice.android.e2e.sh` around lines 7 - 10,
Update the vision model E2E assertion to truncate or baseline offgrid-debug.log
before loading, then poll only newly appended content until a deadline for the
fresh “[WIRE-VISION]” initialized event, accommodating slow devices instead of
relying on a fixed sleep. Assert the fresh log window contains vision support
and does not contain “Multimodal support not enabled,” using the existing test
flow and log variables.
| QUERY="${MODEL_QUERY:?set MODEL_QUERY, e.g. Qwen3.5-0.8B or gemma-4-E2B}" | ||
| LOG() { adb exec-out run-as "$PKG" cat files/offgrid-debug.log 2>/dev/null; } | ||
| TAP() { adb shell input tap "$1" "$2"; sleep "${3:-2}"; } | ||
| SHOT() { adb exec-out screencap -p > /tmp/e2e-android.png 2>/dev/null; } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use a unique temporary screenshot path.
SHOT writes to predictable /tmp/e2e-android.png; a local process can pre-create a symlink and redirect the screenshot contents to another file. Use mktemp and clean it up with a trap.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 32-32: Function 'SHOT' should be named in snake case.
[warning] 32-32: Add an explicit return statement at the end of the function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionModelOnDevice.android.e2e.sh` at line 32, Update
the SHOT function to capture screenshots at a uniquely generated temporary path
using mktemp, rather than the predictable /tmp/e2e-android.png path. Add cleanup
for the generated file through an appropriate shell trap while preserving the
existing adb screencap behavior.
Source: Linters/SAST tools
| echo "== download the smallest (topmost) file ==" | ||
| SHOT | ||
| TAP 949 975 4 # download icon on the first (smallest) file row |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Carry the exact downloaded GGUF identity through the test.
The script only proves that some filename contains the query stem, then loads the first local model and accepts any [WIRE-VISION] line containing that stem. Multiple local quantizations can therefore make this test validate the wrong artifact. Require a completed .gguf, preserve its exact basename, select or verify that model in the picker, and match the exact filename in the model field logged by src/services/llm.ts:218-229. Also reject a query whose normalized stem is empty.
Also applies to: 54-60, 69-70, 79-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/e2e/device/visionModelOnDevice.android.e2e.sh` around lines 48 -
50, Update the vision model E2E flow around the download, picker selection, and
[WIRE-VISION] assertion to require a completed .gguf file, preserve its exact
basename, and verify/select that same model before inference; match the exact
basename in the logged model field rather than only the query stem. Validate
that the normalized query stem is non-empty before proceeding.
Update — Android family cell now GREEN too (full matrix complete)Android's E4B download finished (device network recovered), E4B loaded with vision, and all three phases now pass on Android as well: Android E4B load-time pairing (its OWN mmproj, vision initialised): Full matrix — every cell verified on a real device:
|
1 participant
What
Adds a device e2e (
__tests__/e2e/device/visionMmprojMultiModel.e2e.sh, plus the single-modelvisionModelOnDevice.android.e2e.sh) that proves the two mmproj invariants that only surface when more than one vision model lives on a device at once — the scenario behind the A1 vision regression:weights>=2 && mmproj==1on disk + reuse in the log.The pairing signal is the app's own decision, read from the debug log either at boot (
linkOrphanMmProj "<weights> — linking <mmproj>") or at model load (WIRE-VISION). The script accepts either; the boot line is deterministic and needs no message-send, which makes the assertion automatable and un-fakeable (disk + log, no UI).PLATFORM=iosusesdevicectl;PLATFORM=androidusesadb. Phases:baseline | dedup | family.Verified on real devices
iPhone — all three phases green. Five model families coexisting (Qwen 0.8B, Qwen 2B, SmolVLM, Gemma E2B with two quants sharing one mmproj, Gemma E4B), each paired to its OWN correctly-named mmproj:
Device pairing log (the literal resolution output):
Android — baseline + dedup green (two distinct mmproj on disk: e2b + e4b; 2 E2B quants share one mmproj). The E4B family-pairing cell needs the 5.6 GB E4B weights to land so the pairing log fires; that download is blocked on this device by crippled WiFi (~39 KB/s), not by code.
Why this is safe as a test-only change
The shared naming/pairing logic (
modelIdentityStem/mmProjLocalName/mmProjBelongsToModel) is identical on both platforms and is already guarded by the committed unit + live-HF-fixture tests. This script is the operator-run device gate on top of that. No product code changes.Summary by CodeRabbit