Skip to content

fix(compression): enable OmniGlyph for Claude Fable 5 - #7863

Merged
diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.49from
enjoyer-hub:fix/fable5-omniglyph
Jul 20, 2026
Merged

diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.49from
enjoyer-hub:fix/fable5-omniglyph

Conversation

@enjoyer-hub

Copy link
Copy Markdown
Contributor

Summary

  • classify the claude OAuth provider as a direct Anthropic transport for OmniGlyph
  • recognize Claude Fable model IDs as vision-capable in the shared heuristic
  • add focused regression coverage for both integration points

Why

OmniGlyph requires both direct transport and vision support. claude/claude-fable-5 was incorrectly treated as an aggregator and was absent from the shared vision heuristic, so compression was selected but skipped before transforming context.

Verification

  • npx tsx --test tests/unit/vision-detection-consistency.test.ts tests/unit/compression/omniglyph-chatcore-plumbing.test.ts (14 passed)
  • npx eslint open-sse/handlers/chatCore.ts src/shared/constants/visionModels.ts tests/unit/vision-detection-consistency.test.ts tests/unit/compression/omniglyph-chatcore-plumbing.test.ts --no-cache
  • repository pre-commit checks passed

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request updates the provider transport logic in chatCore.ts to treat both 'anthropic' and 'claude' providers as direct OmniGlyph transports, and adds 'claude-fable' to the vision model list. Feedback was provided regarding the newly added unit test, which is highly fragile because it uses regular expressions to perform static analysis on the raw source code of chatCore.ts instead of testing runtime behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +5 to +12
test("chatCore treats both Anthropic providers as direct OmniGlyph transports", () => {
const chatCore = readFileSync("open-sse/handlers/chatCore.ts", "utf8");

assert.match(
chatCore,
/providerTransport:\s*provider === "anthropic" \|\| provider === "claude" \? "direct" : "aggregator"/
);
});

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.

medium

This unit test is highly fragile because it performs static analysis via regular expressions on the raw source code of 'chatCore.ts'. Any minor formatting change (such as those applied by Prettier, or refactoring the ternary operator into a helper function/variable) will break this test even if the runtime behavior remains completely correct. A more robust approach is to write a behavioral test that mocks the compression module or calls 'handleChatCore' with mocked dependencies to verify that the correct 'providerTransport' is passed.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Verified this end-to-end in an isolated worktree off release/v3.8.49: the root cause is correctly diagnosed as two independent fail-closed gates in the OmniGlyph pipeline (providerTransport !== "direct" and supportsVision !== true), and the fix for both is minimal and semantically correct — I confirmed the claude OAuth provider does hit api.anthropic.com directly, same as the anthropic API-key provider, so reclassifying it as "direct" is accurate, not a workaround.

All 16 relevant tests pass locally, including the existing end-to-end omniglyph-plumbing.test.ts suite that exercises applyCompressionAsync() and confirms the engine actually runs (not skipped) once both gates pass.

The 3 red CI checks (Integration Tests, Quality Gates Extended, Quality Ratchet) are not caused by this diff — I cross-checked against another unrelated open PR (#7300, usage/token-counting only) and it shows the exact same failures (batch-e2e-rate-limit server-startup timeout, coverage.functions ratchet drift, unrelated shellcheck findings in workflow YAML), confirming these are pre-existing base-reds on the current tip.

One non-blocking note already raised by the gemini-code-assist bot review: tests/unit/compression/omniglyph-chatcore-plumbing.test.ts asserts via a regex match against chatCore.ts's raw source text rather than exercising runtime behavior, which is fragile against future reformatting. Consider folding that assertion into the existing behavioral omniglyph-plumbing.test.ts suite instead (or alongside it) when convenient — not required for this PR.

Thanks for tracking down both gates here — nice fix.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.49 July 20, 2026 14:03
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Reconciled this against the other two compression PRs in flight (#7869 CCR-skip-tool-outputs and #7855 session-dedup memory bound): complementary, no semantic collision — the three touch different engines, and none of them changes the pipeline's fixed stackPriority order (session-dedup=3 → ccr=4 → omniglyph=90), so enabling OmniGlyph for Fable 5 composes cleanly with both. On a Fable 5 request with tool outputs the stack behaves correctly end-to-end: dedup runs → CCR skips the tool output (post-#7869) → OmniGlyph runs after.

One mechanical item: the branch now shows as conflicting against the base — it needs a rebase onto the current release/v3.8.49 tip. Suggested land order within the trio, purely for rollback hygiene: #7855 → #7869 → this one.

Rebuilt clean on release/v3.8.49 (branch forked from an old main and carried
~68 files of base drift). Reconciled with cheaper-inference#7237 on the tip: supportsVision
keeps the authoritative getResolvedModelCapabilities() resolution (the PR's
isVisionModelId heuristic predates that fix); the PR's real change lands —
OAuth 'claude' now counts as a direct Anthropic transport for OmniGlyph, and
claude-fable-5 joins the vision model ids. Plumbing test now pins the
'|| provider === "claude"' invariant instead of exact formatting.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw
diegosouzapw force-pushed the fix/fable5-omniglyph branch from d0b1af4 to 6fd8a37 Compare July 20, 2026 16:29
@diegosouzapw
diegosouzapw merged commit 3fe61cd into cheaper-inference:release/v3.8.49 Jul 20, 2026
5 checks passed
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Merged — thanks @enjoyer-hub / @quanturbo! We rebuilt the branch clean on release/v3.8.49 (it carried ~68 files of old-main drift) and kept your real change — OAuth claude as a direct OmniGlyph transport + claude-fable-5 in the vision ids — while preserving the tip's #7237 authoritative supportsVision resolution. Validated in the local merge-train (14-PR combined tree, full unit suite + vitest green) on tomni-proxmox-113 @ 0fda37bb4fdb26ba1345fd7b4cac44fbe235cda4, then squash-merged.

@diegosouzapw diegosouzapw mentioned this pull request Jul 23, 2026
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…nce#7863)

Rebuilt clean on release/v3.8.49 (branch forked from an old main and carried
~68 files of base drift). Reconciled with cheaper-inference#7237 on the tip: supportsVision
keeps the authoritative getResolvedModelCapabilities() resolution (the PR's
isVisionModelId heuristic predates that fix); the PR's real change lands —
OAuth 'claude' now counts as a direct Anthropic transport for OmniGlyph, and
claude-fable-5 joins the vision model ids. Plumbing test now pins the
'|| provider === "claude"' invariant instead of exact formatting.

Co-authored-by: quanturbo <168349709+quanturbo@users.noreply.github.com>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…nce#7863)

Rebuilt clean on release/v3.8.49 (branch forked from an old main and carried
~68 files of base drift). Reconciled with cheaper-inference#7237 on the tip: supportsVision
keeps the authoritative getResolvedModelCapabilities() resolution (the PR's
isVisionModelId heuristic predates that fix); the PR's real change lands —
OAuth 'claude' now counts as a direct Anthropic transport for OmniGlyph, and
claude-fable-5 joins the vision model ids. Plumbing test now pins the
'|| provider === "claude"' invariant instead of exact formatting.

Co-authored-by: quanturbo <168349709+quanturbo@users.noreply.github.com>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
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