Skip to content

fix: merge all system/developer messages into one leading message (SGLang #377) - #379

Closed
ranxianglei wants to merge 2 commits into
masterfrom
2026-08-31_merge-system-messages
Closed

ranxianglei wants to merge 2 commits into
masterfrom
2026-08-31_merge-system-messages

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Summary

Fixes #377 — with 2+ active ACP compressed blocks, bili rendered each block's summary as its own role:"system" message, so the OpenAI wire carried 2+ system messages. SGLang (and vLLM with the Qwen3-family "system-first" chat template) reject that with a hard 400:

{"object":"error","message":"System message must be at the beginning.","type":"BadRequest","code":400}

The earlier #355 fix (hoistMidSystemMessages) hoisted mid-conversation systems to a contiguous leading prefix but kept them separate — which SGLang still rejects, because its Qwen3 template guard is if message.role == 'system' and not loop.first: raise, i.e. it requires exactly one system message at index 0, not merely a leading prefix.

Change

Replace hoistMidSystemMessages with mergeSystemMessages (src/util.ts): it collapses all system/developer messages into a single leading system message, preserving the relative order of every other message. sysParts (the client's original system + the static compress prompt) is placed first so the stable prefix-cache anchor stays at the head across compress turns; the folded block summaries follow in their original anchor order.

  • src/server.ts (prepareOpenai) and src/loop/adapter-openai.ts (buildRequest) now call coreToOpenai(...) then mergeSystemMessages(msgs, sysParts); the now-unused injectOpenaiSystem import is dropped from both.
  • The Responses API path is unaffected (it already injects a single joined developer message).

Tests

Pre-flight

@github-actions

github-actions Bot commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-08-31_merge-system-messages (2d44dd4)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-379

Each push to this PR publishes a new version under the pr-379 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr379.tgz
npm install -g package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Reviewing this PR now. Diff reviewed: mergeSystemMessages in src/util.ts replaces hoistMidSystemMessages, both call sites (prepareOpenai in src/server.ts, buildRequest in src/loop/adapter-openai.ts) updated, unused injectOpenaiSystem imports dropped, tests rewritten + e2e assertion tightened to "exactly one system at index 0". Running the full pre-flight (typecheck / test / build) on the branch now — will report results here.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Review complete — verified independently on branch 2026-08-31_merge-system-messages (57cd966). All pre-flight claims check out:

Pre-flight (re-run locally)

  • npm run typecheck — clean
  • npm test — 863 pass, 0 fail (matches the PR claim)
  • npm run build — success
  • tests/e2e-mid-system.test.ts (real-proxy e2e) — passes standalone with the new "exactly one system/developer at index 0" assertion on every upstream request

Code review

  • mergeSystemMessages (src/util.ts:194): correct — collects all system/developer messages, sysParts first (stable prefix-cache anchor), block summaries in original anchor order, non-system relative order preserved, no-op (same array ref) when there's nothing to merge, non-string content JSON-stringified. The "\n\n---\n\n" separator and the sysParts-first ordering match the stated cache-stability rationale.
  • Both call sites updated (prepareOpenai in src/server.ts:1442, buildRequest in src/loop/adapter-openai.ts:176); unused injectOpenaiSystem imports dropped from both. No lingering references to hoistMidSystemMessages anywhere. Remaining injectOpenaiSystem hits are only kernel-API unit tests in tests/basic.test.ts / tests/proxy-basic.test.ts, which are legitimate.
  • Tests: tests/merge-system.test.ts covers no-op, single/mid/multiple systems, sysParts-first, the [Bug] ACP: ≥2 compressed blocks render as multiple system messages → SGLang (vLLM-compatible) upstreams return 400 #377 wire shape collapsing to exactly one system, and non-string content. E2e assertion correctly tightened from "contiguous leading prefix" to "exactly one at index 0" — that is sglang's actual guard.
  • Responses API path untouched, as claimed.

One caveat

  • The full codex e2e regression (tests/e2e/e2e-codex.test.ts) could not be run in this environment: no codex binary and no reachable upstream (127.0.0.1:8199 refused, E2E_UPSTREAM_URL unset). Per AGENTS.md it should be exercised before merging request-pipeline changes — the PR's own real-proxy e2e covers the OpenAI wire path through the actual server code, but if you have a codex-capable box, a run of the 4-phase core there would be the last mile.

LGTM from my side — merge is yours to make (I can't merge PRs).

@ranxianglei ranxianglei closed this Sep 1, 2026
ranxianglei added a commit that referenced this pull request Sep 24, 2026
fix: pin acp-kernel 0.0.86 — count-triggered distillation default-off (#379)
@github-actions github-actions Bot mentioned this pull request Sep 24, 2026
antclt pushed a commit to antclt/billion-context that referenced this pull request Sep 25, 2026
…ranxianglei#379)

Bundles kernel ranxianglei#379/ranxianglei#380: tiers.tier2Trigger 5->1000 / tier3Trigger 10->2000
and the ranxianglei#238 usage gate deleted. Count-triggered distillations reclaimed
almost nothing while rewriting the wire from the fold anchor onward — the
prefix-cache sawtooth root cause of ranxianglei#1249. Growth (>=growthFloor) and
token-mass (>=1.5x) paths unchanged and ungated.

Adds tests/kernel-tier-defaults.test.ts pinning the contract into bili's
suite (kernel defaults 1000/2000; applyCompressSettings never overrides
triggers — boolean tiers toggle only flips enabled).

Fixes ranxianglei#1249 (with acp-kernel#379)
antclt pushed a commit to antclt/billion-context that referenced this pull request Oct 6, 2026
…ix from ranxianglei#2225)

The Soft-target section still described the pre-ranxianglei#379/ranxianglei#380 shape: adaptive
step clamp(round(window*5%), 20k, 50k) with a window-dependent growth floor.
Current kernel constants (golden-pinned in tests/golden/wire-contract/kernel.json):
nudge.growthFloor == nudge.growthCap == 50000, so the step is flat 50k at every
window size and the growth gate is max(20k, 0.45*step) ~= 22.5k with the T1
path additionally requiring >= one step of compressible mass. The old text
contradicted the new large-window section added alongside ranxianglei#2225.
ranxianglei pushed a commit that referenced this pull request Oct 6, 2026
KDD #8 still stated the pre-#2110 contract (flat 50k pinned at every
window size, 'Do NOT re-scale the interval with the context window'), which
this PR deliberately reverses for sub-1M windows. Rewrite the entry to the
new contract: clamp(5% x window, 20k, 50k), >=1M byte-identical, and keep
the surviving #379/#380 settlement (growth-driven, no usage/count proxy
gates) plus the escape-hatch wording.
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.

[Bug] ACP: ≥2 compressed blocks render as multiple system messages → SGLang (vLLM-compatible) upstreams return 400

1 participant