Skip to content

fix(dispatch): strip every _omniroute* internal marker before upstream send - #13355

Merged
diegosouzapw merged 7 commits into
cheaper-inference:release/v3.8.51from
patrykkopycinski:pr-fix-strip-internal-omniroute-markers
Sep 16, 2026
Merged

diegosouzapw merged 7 commits into
cheaper-inference:release/v3.8.51from
patrykkopycinski:pr-fix-strip-internal-omniroute-markers

Conversation

@patrykkopycinski

Copy link
Copy Markdown
Contributor

Live 400 on the claude/claude-opus-5 hop of best-reasoning-paid:
[400]: _omnirouteSkipContextRelay: Extra inputs are not permitted

contextHandoff.ts stamps _omnirouteSkipContextRelay / _omnirouteInternalRequest
onto internal handoff-summary bodies so the context-relay injection in chat.ts
skips its own request. stripInternalBodyFields() -- the single pre-serialization
chokepoint in base.ts -- was a hand-maintained allowlist of 5 unrelated markers,
so those keys survived into the serialized payload and strict Anthropic-compatible
gateways rejected the whole request.

Strip by prefix instead of per-key: every _omniroute* marker is internal by
construction and consumed by routing before dispatch, so no future marker can
become the next upstream 400. Client fields that merely start with _ are
untouched.

Tests: helper-level cases plus an end-to-end DefaultExecutor.execute() test that
asserts on the actual serialized upstream bytes. Mutation-verified: reverting the
fix turns 4 of 7 red, including the e2e case.


Complements #12735: that PR deletes the two currently-known handoff flags key-by-key; this one strips the whole _omniroute* prefix so future internal markers can't become the next upstream 400 (e.g. _omnirouteResponsesStore, already on this base, is caught here).

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Verified the bug independently on the current tip and your prefix-based approach is the
right shape for this chokepoint — it also automatically covers _omnirouteResponsesStore
and any future _omniroute* marker, which the old allowlist would have missed again. Ran
your test file (7/7 pass, including the e2e byte-level assertion). Since you already flagged
this complements #12735: we're planning to land this one for the shared
cliFingerprints.ts chokepoint and ask #12735's author to rebase down to just their
4-executor defense-in-depth calls (dario/gitlab/glm/ninerouter bypass this chokepoint
entirely). One ask before merge: please add a changelog.d/fixes/ fragment referencing
#12729.

@patrykkopycinski

Copy link
Copy Markdown
Contributor Author

Fragment added in fbb690ba9: changelog.d/fixes/13355-strip-internal-omniroute-markers.md, referencing #12729. Passes npm run check:changelog-integrity locally — the only fragment that gate flags is fixes/reset-aware-model-family.md (missing leading - , landed via #12637), i.e. pre-existing base debt untouched by this PR.

@patrykkopycinski

Copy link
Copy Markdown
Contributor Author

CI attribution for the red fast-path on fbb690ba9 (and on be31d7233 before it) — pre-existing base heat, not this diff:

  • fbb690ba9 adds only changelog.d/fixes/13355-*.md. The only CI consumer of changelog.d/ is the changelog-integrity gate, which passes on fbb690ba9 (it failed on be31d7233 — the fragment fixed that). Unit shards never read changelog.d/.
  • be31d7233 (code only, no fragment) already failed Fast Quality Gates + unit shards on Sep 11 — with a different failing-test set (chatcore-hierarchical-admission, injection-guard-scan-bound-3932, glm-sse-transform-arity, tunnel-routes-error-sanitization) than today’s (paid-model-target-routes-6540 ×3, 200 !== 400). Same tree, disjoint failures ⇒ runner/base flakiness.
  • Fast Quality Gates fails inside check-forgotten-sibling-tests.mjs (Invalid string length while tracing agyModels.ts → shared.ts → ~every call-log test) — the sibling-graph blowup also aborted the be31 run’s step-summary upload (1065k > 1024k limit). The last green release/v3.8.51 run predates the current base tip.

Vitest fast-path, Docs Gates, ESLint, Merge integrity, semgrep all pass on fbb690ba9. Happy to re-trigger if you want a fresh roll of the shards — repo precedent 226e6ee54.

@patrykkopycinski

Copy link
Copy Markdown
Contributor Author

CI attribution (cross-posted to #13355 #13448 #13617 #13627 #13359): the red Fast Quality Gates on these heads are pre-existing base debt, visible on the base branch's own runs with no PR diff applied:

  • mutation-test-coverage: run 34970965604 (base head 1e1af28) fails "2 covering unit test(s) across 2 module(s) are missing from stryker.conf.json" (incl. tests/unit/noauth-model-lockout.test.ts) — same signature on our PR runs.
  • secrets ratchet: "REGRESSÃO — 1 secret findings > baseline 0" reproduces across PRs with disjoint diffs.
  • forgotten-sibling-tests: "Invalid string length" (RangeError, string overflow) crashes the checker on the agyModels→shared→tests graph — hit 13355 and 13627 hours apart on unrelated diffs.
    Base tip FQG itself: last completed run 34970965604 = failure. Happy to PR the stryker testFiles additions if useful.

…m send

Live 400 on the claude/claude-opus-5 hop of best-reasoning-paid:
  [400]: _omnirouteSkipContextRelay: Extra inputs are not permitted

contextHandoff.ts stamps _omnirouteSkipContextRelay / _omnirouteInternalRequest
onto internal handoff-summary bodies so the context-relay injection in chat.ts
skips its own request. stripInternalBodyFields() -- the single pre-serialization
chokepoint in base.ts -- was a hand-maintained allowlist of 5 unrelated markers,
so those keys survived into the serialized payload and strict Anthropic-compatible
gateways rejected the whole request.

Strip by prefix instead of per-key: every _omniroute* marker is internal by
construction and consumed by routing before dispatch, so no future marker can
become the next upstream 400. Client fields that merely start with _ are
untouched.

Tests: helper-level cases plus an end-to-end DefaultExecutor.execute() test that
asserts on the actual serialized upstream bytes. Mutation-verified: reverting the
fix turns 4 of 7 red, including the e2e case.
@patrykkopycinski
patrykkopycinski force-pushed the pr-fix-strip-internal-omniroute-markers branch from 9507621 to 1b59bd0 Compare September 16, 2026 17:06
hartmark and others added 4 commits September 16, 2026 15:09
…ount (cheaper-inference#13079)

Merged after a maintainer rework that kept every one of @hartmark's commits intact.

**What the rework added:** the reclaimable-space gate for the auto-cleanup VACUUM sits behind a default-off feature flag so the release default is unchanged, with the flag documented in `docs/reference/FEATURE_FLAGS.md` and described in all 66 locales; the rest is your change as submitted.

Validated as a combined board first (this PR merged with the 21 siblings of the same wave on the release tip): eslint with the frozen suppressions, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-counts, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 176 passing / 0 failing focused node:test cases across the 25 test files the wave touches and the dashboard test under Vitest (2/0). Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run.

Thank you — gating VACUUM on reclaimable pages instead of row count is the right signal.
…rmat key (cheaper-inference#13617)

Merged after a maintainer rework that kept every one of @patrykkopycinski's commits intact, including the changelog fragment you added afterwards.

**What the rework added:** the `eslint-suppressions.json` diff was corrected (the PR had dropped live entries) and a test now proves the CLI-probe fallback path is actually taken when the HTTP API rejects a CLI-format key — before, the fallback existed but nothing exercised it.

Validated as a combined board first (this PR merged with the 21 siblings of the same wave on the release tip): eslint with the frozen suppressions, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-counts, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 176 passing / 0 failing focused node:test cases across the 25 test files the wave touches and the dashboard test under Vitest (2/0). Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run.

Thank you.
…inned, all harnesses (cheaper-inference#13448)

Merged after a maintainer rework that kept every one of @patrykkopycinski's commits intact — including the two refactors you pushed later (extracting the adaptive-effort wiring out of `chatCore.ts` and reading `x-omniroute-effort` inside the wiring module), which were merged into the rework rather than overwritten.

**What the rework added:** the adaptive-effort wiring is scoped to OpenAI-dispatch requests only (the claim in `docs/routing` was corrected to match), and `defaultReasoningEffort` was widened to accept `auto` explicitly instead of relying on a loose string.

Validated as a combined board first (this PR merged with the 21 siblings of the same wave on the release tip): eslint with the frozen suppressions, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-counts, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 176 passing / 0 failing focused node:test cases across the 25 test files the wave touches and the dashboard test under Vitest (2/0). Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run.

Thank you — gateway-resolved, per-turn pinned effort is a real feature, and the header contract makes it usable from every harness.
@diegosouzapw
diegosouzapw merged commit f653a6f into cheaper-inference:release/v3.8.51 Sep 16, 2026
0 of 3 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…m send (cheaper-inference#13355)

Merged. Internal `_omniroute*` markers must never reach an upstream: at best they are noise in someone else's logs, at worst they change the upstream's parse. Stripping every one of them before the send is the right invariant.

Validated as a combined board first (this PR merged with the 21 siblings of the same wave on the release tip): eslint with the frozen suppressions, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-counts, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 176 passing / 0 failing focused node:test cases across the 25 test files the wave touches and the dashboard test under Vitest (2/0). Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run.

Thank you.
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