fix(sse): inject global system prompt once, post-translation, across all target shapes - #12904
Merged
diegosouzapw merged 8 commits intoSep 17, 2026
Conversation
…onses path codex/Responses requests carry input[]+instructions, not messages[]. The existing injectSystemPrompt runs PRE-translation (chatCore.ts) and only handles messages[]/system fields, so the Global System Prompt (After Prompt = suffixPrompt) never reached the provider for codex — verified 0/84 call logs while the catalog base_instructions reached 84/84. Add injectSystemPromptPostTranslation() and call it after prepareUpstreamBody on the resolved messages[]. With multiple system/developer messages (codex normalises its per-item developer roles to system), prefix goes on the FIRST and suffix on the LAST so the After Prompt retains the highest recency position — the semantics injectSystemPrompt's single-findIndex buries. Also wire OMNIROUTE_SYSTEM_INSTRUCTION_APPEND on the /v1/messages (Claude Messages -> OpenAI Chat Completions) translation path. The directive was previously only wired on the Responses API path, so DeepSeek-V4 kept leaking English planning/chain-of-thought into the content field on Claude Code sessions that route through /v1/messages. Mirror the openai-responses.ts pattern: append to string system, append a text block for array content, or unshift a new system message when none exists. Tests: 23/23 (19 system-prompt incl. 6 postTranslation + codex regression; 4 claude-to-openai directive append). typecheck:core clean.
…esponses target shapes
… gated pre-translation pass
Owner
|
Solid, well-tested rework of the injection point (52/52 tests pass at your head, including
|
This was referenced Sep 15, 2026
…dd changelog fragment Points the #reasoning-bilingual comment at the real companion file (translator/response/openai-to-claude.ts's directivePreambleStripper.ts from diegosouzapw#12905) instead of the nonexistent "openai-responses.ts", and adds the changelog fragment referenced in the PR body but missing from the diff. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
diegosouzapw
added a commit
that referenced
this pull request
Sep 17, 2026
Third pass on the release/v3.8.51 base-reds. Declaring `vite` in the previous commit was correct but incomplete: check-deps is a human review point against typosquatting, so a newly declared package has to be vouched for by name. Recorded in dependency-allowlist.json with why it is needed — the official Vite build tool, already pinned through overrides, and a required peer of both vitest 5 and @vitejs/plugin-react. That also turns check-deps.test.ts green. check-file-size went red on nine files. One is mine: sse-auth.test.ts grew when the #12080 assertion was rewritten. Three of the four assertions I had added were redundant with the strict deepEqual that follows them, so they are gone and the file grows by 4 lines instead of 8; the cap absorbs the rest. The other eight are production and test files this PR does not touch, grown by other work and never rebaselined — which is the whole reason a base-red drain exists. Each is attributed to the commit that grew it: #12906 (chat.ts, chatHelpers.ts, proxyFetch.ts, stream.ts), #12904 + #12910 (chatCore.ts), and batch_api.test.ts from the same wave. Two of them predate the wave entirely and were already over cap on 3d5baf1 — imageGeneration.ts (#13748) and roundRobinCombo.ts (#13776) — so they were base-reds hiding behind a gate that only surfaced them once the tip was merged in. Both are recorded separately from the wave so the history stays honest about when each cap actually moved. Note for whoever reads the gate next: it counts one line more than `wc -l`, since it measures split length rather than newlines. Finally, #13290 replaced rmSync with cleanupTempDataDir in zcode-executor.test.ts but left the import behind, which the frozen-warning ESLint gate rejects. Removed. Refs #13866
diegosouzapw
added a commit
that referenced
this pull request
Sep 17, 2026
* fix(quality): clear the release/v3.8.51 base-reds 19 failing unit tests plus the API Route Typecheck and mutation-test-coverage gates, all reproduced on the clean tip before touching anything. Ten of the failures share one cause. #13452/#13798 made `*-compatible-*` buildUrl() refuse a connection with no baseUrl instead of quietly defaulting to the real OpenAI/Anthropic API — which would ship the operator's stored key to a public third party. The guard is right; three fixtures still built those connections unhydrated, and one of them put baseUrl at the top level of credentials, where the chat path never reads it. The rest: - modelDiscovery.ts missed the VertexModelMetadataProvenance cast that its read-path twin in db/models/synced.ts already had — both written by #12471. - A provider-test regexp carried raw 0x00/0x1f bytes, which makes git, GitHub and ripgrep treat the file as binary. Same character class, written with escapes instead of the bytes themselves. - #13399 (Agnes AI China) adds "agnes-cn" + "agnescn": the only two provider prefixes since the count was last set (412 -> 414). Everything else added in that range is model ids. - The free-tier budget card SVG was stale (443 -> 452 models); regenerated by its own script. - Three new tests were missing from stryker.conf.json tap.testFiles, so the mutants they kill did not count. Three guards asserted syntax rather than the invariant they protect, and broke when the source legitimately changed. Each was re-expressed and then verified by mutating the source back: - #2331 required modelEffort to head the rawEffort chain; #13556 deliberately put the server-selected force rule first. The real invariant is relative — modelEffort outranks the defaults a client injects — and it still trips when explicitReasoning is moved ahead of it. - The OAuth loopback guard matched the isLocalhost arm literally; #9944 added `&& !opts?.manualLoopback`. It now matches the arm whatever guards it, and still fails when the hint stops being built. - The i18n scanner flagged dynamically-built keys — t("effort." + mode) reaches it as a literal prefix, never a string. It now accepts a prefix that resolves to a namespace holding messages, and still fails when the namespace is gone. tests/unit/sse-auth.test.ts (#12080) expected a bare null where #13879 now returns the key-policy diagnostic — the same sentinel shape the terminal-state path has used since #12441. The assertion was rewritten to the constraint #12080 actually protects: nothing usable comes back and neither connection leaks. The contract risk that remains — those sentinels are truthy, and executeWebSearch treats any truthy value as a credential — is filed as #13945 rather than widened into this PR. Refs #13866 * fix(quality): clear the second wave of release/v3.8.51 base-reds The tip moved 13 commits while the first pass was running and brought its own reds. All reproduced locally on the merged tree first. vitest 4.1.11 -> 5.0.0 in the #13661 development-group bump is a major, and vitest 5 moved `vite` from a dependency to a peerDependency. This repo only ever declared `vite` under `overrides`, which pins a version but installs nothing, so `npm ci` stopped providing it and the Vitest job died at startup with ERR_MODULE_NOT_FOUND. Declared as the devDependency it actually is — the same ^8.0.16 the override already pinned, and what @vitejs/plugin-react asks for as a peer — and regenerated the lockfile: 684 lines added, none changed. #12909 filtered a mapped array with `toolCall is JsonRecord`, but the element type is the tool-call literal or null, and a predicate's type has to be assignable to the parameter's (TS2677). Narrowed by the element's own type instead; the literal still satisfies JsonRecord at the return. #12906 added `|| result.errorCode === "empty_response"` to the stream-failure condition and Prettier rewrapped it, so the #8928 probe — which located the branch by an exact four-line string — stopped finding it. It now matches on what the branch tests rather than how it is typeset, and still fails when the eviction call is removed. probe-7293 is the visible half of a real conflict, filed as #13948. #7293 merges a mid-array system into index 0; #12908, landed later, demotes it to "user" in place instead. Both target the same constraint and only one can win, and the combination also reorders: the pre-translation hoist moves the turn forward expecting it to stay a system message, then the demotion converts it where it now sits, ahead of the conversation. Choosing between the two strategies is a product call, not a base-red one, so the test was realigned to assert the half that protects the caller — the instruction survives, as a user turn — and pins the current ordering with a pointer to the issue, so the eventual decision shows up as a deliberate test change instead of a silent regression. Refs #13866, #13948 * fix(quality): allowlist vite, rebaseline tip growth, drop a dead import Third pass on the release/v3.8.51 base-reds. Declaring `vite` in the previous commit was correct but incomplete: check-deps is a human review point against typosquatting, so a newly declared package has to be vouched for by name. Recorded in dependency-allowlist.json with why it is needed — the official Vite build tool, already pinned through overrides, and a required peer of both vitest 5 and @vitejs/plugin-react. That also turns check-deps.test.ts green. check-file-size went red on nine files. One is mine: sse-auth.test.ts grew when the #12080 assertion was rewritten. Three of the four assertions I had added were redundant with the strict deepEqual that follows them, so they are gone and the file grows by 4 lines instead of 8; the cap absorbs the rest. The other eight are production and test files this PR does not touch, grown by other work and never rebaselined — which is the whole reason a base-red drain exists. Each is attributed to the commit that grew it: #12906 (chat.ts, chatHelpers.ts, proxyFetch.ts, stream.ts), #12904 + #12910 (chatCore.ts), and batch_api.test.ts from the same wave. Two of them predate the wave entirely and were already over cap on 3d5baf1 — imageGeneration.ts (#13748) and roundRobinCombo.ts (#13776) — so they were base-reds hiding behind a gate that only surfaced them once the tip was merged in. Both are recorded separately from the wave so the history stays honest about when each cap actually moved. Note for whoever reads the gate next: it counts one line more than `wc -l`, since it measures split length rather than newlines. Finally, #13290 replaced rmSync with cleanupTempDataDir in zcode-executor.test.ts but left the import behind, which the frozen-warning ESLint gate rejects. Removed. Refs #13866
diegosouzapw
added a commit
that referenced
this pull request
Sep 17, 2026
…e moved (#14002) Measured on the clean tip (83fa432), not on a branch. Each ceiling was attributed to the PR that moved it before being raised — no blanket rebaseline: - src/sse/handlers/chatHelpers.ts 1245 -> 1246 (#13551, combo scope on the fail-closed proxy guard) - open-sse/handlers/chatCore.ts 6203 -> 6219 (#12905 DSML/preamble, #13910 disguised-2xx classification, #12904 single post-translation system prompt) - open-sse/utils/stream.ts 3123 -> 3140 (#12905, and #12906 empty_response 502 retry + reasoning-aware timeout) - tests/integration/chat-pipeline.test.ts 1736 -> 1740 (#13419, the two exact header assertions updated for charset=utf-8) Other gates re-measured on the same tip and already green: typecheck:core, check:open-sse-typecheck, check:docs-counts, and the #2331-adjacent chatcore-translation-paths xhigh-effort case that was red before this wave.
This was referenced Sep 18, 2026
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…all target shapes (diegosouzapw#12904) * fix(sse): inject global system prompt post-translation for codex/Responses path codex/Responses requests carry input[]+instructions, not messages[]. The existing injectSystemPrompt runs PRE-translation (chatCore.ts) and only handles messages[]/system fields, so the Global System Prompt (After Prompt = suffixPrompt) never reached the provider for codex — verified 0/84 call logs while the catalog base_instructions reached 84/84. Add injectSystemPromptPostTranslation() and call it after prepareUpstreamBody on the resolved messages[]. With multiple system/developer messages (codex normalises its per-item developer roles to system), prefix goes on the FIRST and suffix on the LAST so the After Prompt retains the highest recency position — the semantics injectSystemPrompt's single-findIndex buries. Also wire OMNIROUTE_SYSTEM_INSTRUCTION_APPEND on the /v1/messages (Claude Messages -> OpenAI Chat Completions) translation path. The directive was previously only wired on the Responses API path, so DeepSeek-V4 kept leaking English planning/chain-of-thought into the content field on Claude Code sessions that route through /v1/messages. Mirror the openai-responses.ts pattern: append to string system, append a text block for array content, or unshift a new system message when none exists. Tests: 23/23 (19 system-prompt incl. 6 postTranslation + codex regression; 4 claude-to-openai directive append). typecheck:core clean. * test(sse): reproduce global prompt double injection — single-injection contract tests * fix(sse): unify global prompt injection to single post-translation pass * fix(sse): carry single global-prompt injection across claude/gemini/responses target shapes * fix(sse): restore global-prompt coverage for carrier-less targets via gated pre-translation pass * fix(sse): cover codex/gemini source shapes in the carrier-less pre-translation gate * fix(types): preserve generic system prompt return * fix(sse): correct file reference in claude-to-openai.ts comment and add changelog fragment Points the #reasoning-bilingual comment at the real companion file (translator/response/openai-to-claude.ts's directivePreambleStripper.ts from diegosouzapw#12905) instead of the nonexistent "openai-responses.ts", and adds the changelog fragment referenced in the PR body but missing from the diff. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: Jihyun Son <jihyun.son@sk.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
* fix(quality): clear the release/v3.8.51 base-reds 19 failing unit tests plus the API Route Typecheck and mutation-test-coverage gates, all reproduced on the clean tip before touching anything. Ten of the failures share one cause. diegosouzapw#13452/diegosouzapw#13798 made `*-compatible-*` buildUrl() refuse a connection with no baseUrl instead of quietly defaulting to the real OpenAI/Anthropic API — which would ship the operator's stored key to a public third party. The guard is right; three fixtures still built those connections unhydrated, and one of them put baseUrl at the top level of credentials, where the chat path never reads it. The rest: - modelDiscovery.ts missed the VertexModelMetadataProvenance cast that its read-path twin in db/models/synced.ts already had — both written by diegosouzapw#12471. - A provider-test regexp carried raw 0x00/0x1f bytes, which makes git, GitHub and ripgrep treat the file as binary. Same character class, written with escapes instead of the bytes themselves. - diegosouzapw#13399 (Agnes AI China) adds "agnes-cn" + "agnescn": the only two provider prefixes since the count was last set (412 -> 414). Everything else added in that range is model ids. - The free-tier budget card SVG was stale (443 -> 452 models); regenerated by its own script. - Three new tests were missing from stryker.conf.json tap.testFiles, so the mutants they kill did not count. Three guards asserted syntax rather than the invariant they protect, and broke when the source legitimately changed. Each was re-expressed and then verified by mutating the source back: - diegosouzapw#2331 required modelEffort to head the rawEffort chain; diegosouzapw#13556 deliberately put the server-selected force rule first. The real invariant is relative — modelEffort outranks the defaults a client injects — and it still trips when explicitReasoning is moved ahead of it. - The OAuth loopback guard matched the isLocalhost arm literally; diegosouzapw#9944 added `&& !opts?.manualLoopback`. It now matches the arm whatever guards it, and still fails when the hint stops being built. - The i18n scanner flagged dynamically-built keys — t("effort." + mode) reaches it as a literal prefix, never a string. It now accepts a prefix that resolves to a namespace holding messages, and still fails when the namespace is gone. tests/unit/sse-auth.test.ts (diegosouzapw#12080) expected a bare null where diegosouzapw#13879 now returns the key-policy diagnostic — the same sentinel shape the terminal-state path has used since diegosouzapw#12441. The assertion was rewritten to the constraint diegosouzapw#12080 actually protects: nothing usable comes back and neither connection leaks. The contract risk that remains — those sentinels are truthy, and executeWebSearch treats any truthy value as a credential — is filed as diegosouzapw#13945 rather than widened into this PR. Refs diegosouzapw#13866 * fix(quality): clear the second wave of release/v3.8.51 base-reds The tip moved 13 commits while the first pass was running and brought its own reds. All reproduced locally on the merged tree first. vitest 4.1.11 -> 5.0.0 in the diegosouzapw#13661 development-group bump is a major, and vitest 5 moved `vite` from a dependency to a peerDependency. This repo only ever declared `vite` under `overrides`, which pins a version but installs nothing, so `npm ci` stopped providing it and the Vitest job died at startup with ERR_MODULE_NOT_FOUND. Declared as the devDependency it actually is — the same ^8.0.16 the override already pinned, and what @vitejs/plugin-react asks for as a peer — and regenerated the lockfile: 684 lines added, none changed. diegosouzapw#12909 filtered a mapped array with `toolCall is JsonRecord`, but the element type is the tool-call literal or null, and a predicate's type has to be assignable to the parameter's (TS2677). Narrowed by the element's own type instead; the literal still satisfies JsonRecord at the return. diegosouzapw#12906 added `|| result.errorCode === "empty_response"` to the stream-failure condition and Prettier rewrapped it, so the diegosouzapw#8928 probe — which located the branch by an exact four-line string — stopped finding it. It now matches on what the branch tests rather than how it is typeset, and still fails when the eviction call is removed. probe-7293 is the visible half of a real conflict, filed as diegosouzapw#13948. diegosouzapw#7293 merges a mid-array system into index 0; diegosouzapw#12908, landed later, demotes it to "user" in place instead. Both target the same constraint and only one can win, and the combination also reorders: the pre-translation hoist moves the turn forward expecting it to stay a system message, then the demotion converts it where it now sits, ahead of the conversation. Choosing between the two strategies is a product call, not a base-red one, so the test was realigned to assert the half that protects the caller — the instruction survives, as a user turn — and pins the current ordering with a pointer to the issue, so the eventual decision shows up as a deliberate test change instead of a silent regression. Refs diegosouzapw#13866, diegosouzapw#13948 * fix(quality): allowlist vite, rebaseline tip growth, drop a dead import Third pass on the release/v3.8.51 base-reds. Declaring `vite` in the previous commit was correct but incomplete: check-deps is a human review point against typosquatting, so a newly declared package has to be vouched for by name. Recorded in dependency-allowlist.json with why it is needed — the official Vite build tool, already pinned through overrides, and a required peer of both vitest 5 and @vitejs/plugin-react. That also turns check-deps.test.ts green. check-file-size went red on nine files. One is mine: sse-auth.test.ts grew when the diegosouzapw#12080 assertion was rewritten. Three of the four assertions I had added were redundant with the strict deepEqual that follows them, so they are gone and the file grows by 4 lines instead of 8; the cap absorbs the rest. The other eight are production and test files this PR does not touch, grown by other work and never rebaselined — which is the whole reason a base-red drain exists. Each is attributed to the commit that grew it: diegosouzapw#12906 (chat.ts, chatHelpers.ts, proxyFetch.ts, stream.ts), diegosouzapw#12904 + diegosouzapw#12910 (chatCore.ts), and batch_api.test.ts from the same wave. Two of them predate the wave entirely and were already over cap on 8a95ffa — imageGeneration.ts (diegosouzapw#13748) and roundRobinCombo.ts (diegosouzapw#13776) — so they were base-reds hiding behind a gate that only surfaced them once the tip was merged in. Both are recorded separately from the wave so the history stays honest about when each cap actually moved. Note for whoever reads the gate next: it counts one line more than `wc -l`, since it measures split length rather than newlines. Finally, diegosouzapw#13290 replaced rmSync with cleanupTempDataDir in zcode-executor.test.ts but left the import behind, which the frozen-warning ESLint gate rejects. Removed. Refs diegosouzapw#13866
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…e moved (diegosouzapw#14002) Measured on the clean tip (bebb827), not on a branch. Each ceiling was attributed to the PR that moved it before being raised — no blanket rebaseline: - src/sse/handlers/chatHelpers.ts 1245 -> 1246 (diegosouzapw#13551, combo scope on the fail-closed proxy guard) - open-sse/handlers/chatCore.ts 6203 -> 6219 (diegosouzapw#12905 DSML/preamble, diegosouzapw#13910 disguised-2xx classification, diegosouzapw#12904 single post-translation system prompt) - open-sse/utils/stream.ts 3123 -> 3140 (diegosouzapw#12905, and diegosouzapw#12906 empty_response 502 retry + reasoning-aware timeout) - tests/integration/chat-pipeline.test.ts 1736 -> 1740 (diegosouzapw#13419, the two exact header assertions updated for charset=utf-8) Other gates re-measured on the same tip and already green: typecheck:core, check:open-sse-typecheck, check:docs-counts, and the diegosouzapw#2331-adjacent chatcore-translation-paths xhigh-effort case that was red before this wave.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(sse): inject global system prompt once, post-translation, across all target shapes
Summary
define global system prompt (
OMNIROUTE_SYSTEM_INSTRUCTION_APPEND) injection at a single post-translation point. This resolves HCP-Vision vLLM's "System message must be at the beginning" 400.injectSystemPrompt()modified the pre-translation body → the prompt was lost/repositioned during translation and injected 2–3 times depending on the format. The Responses path was not reached at all (0004 partially resolved this by adding a post-translation injection point, but pre-translation injection remained, causing double injection).injectSystemPromptPostTranslation(body, {targetFormat})point (claude/gemini/openai-responses/openai/codex branches) + a gated pre-translation passinjectSystemPromptPreTranslationonly for carrier-less targets (kiro, antigravity envelopes)._systemPromptInjectedflag (Object.defineProperty, not exposed in JSON) so injection occurs only once regardless of the path taken.!result.requestenvelope, wrap Responsesinstructions, use first/last system for Codex, gate carrier-less kiro/antigravity, and no-op for Cursor.Related Issues
Validation
node --import tsx/esm --test tests/unit/system-prompt.test.ts tests/unit/system-instruction-append-claude-to-openai.test.ts tests/unit/global-prompt-single-injection.test.ts(26/26 + existing GREEN)npm run typecheck:core— 0 errorsnpm run lint— 0 errorsrelease/v3.8.51base; focused checks rerun afterwardTests Added Or Updated
tests/unit/global-prompt-single-injection.test.ts(new, 26 tests) — single-injection contract: idempotence, non-enumerable flag absent from JSON, Claude string/array, Gemini present/absent, Responses present/absent, Codex first/last, skip flag, 9 gates, dual-write negative case, 4 source-shape branchestests/unit/system-prompt.test.ts(updated)tests/unit/system-instruction-append-claude-to-openai.test.tsCoverage Notes
global-prompt-single-injection.test.ts. typecheck:core clean.Reviewer Notes
systemPrompt.tslacksinjectSystemPromptPostTranslation/injectSystemPromptPreTranslation;chatCore.tshas only one PRE-translationinjectSystemPrompt(body)at line 627; 0 occurrences ofOMNIROUTE_SYSTEM_INSTRUCTION_APPENDin the canonical tree.inputis not injected when passed to a carrier-less target (kiro/antigravity). Record as follow-up work./v1/messagesrequest, confirmed one system in upstreamproviderRequest(one prefix/one suffix).changelog.d/fixes/12904-system-prompt-single-injection.md