Repository navigation
fix: compat.roles wire role rewrite for role-rejecting upstreams (#552) - #579
Conversation
New opt-in compat.roles map (global + per-provider, provider wins per key)
rewrites message roles on the FINAL forwarded openai/responses body at the
forward boundary — covering client-sent roles and bili's own injected
developer-role prompt alike — and the compress-retry loops apply the same
transform to every re-sent body via RequestOptions.wireTransform. Default {}
forwards byte-for-byte unchanged; no kernel changes (kernel already
normalizes developer internally, so session/ACP state is unaffected).
📦 Built Package ArtifactBranch: Option A — Install from npm PR tag (recommended)npm install -g billion-context@pr-579Each push to this PR publishes a new version under the Option B — Download artifact
tar xzf billion-context-pr579.tgz
npm install -g packageThis comment is automatically updated on each push. |
[bot] 🏷 Review of PR #579 ( Verdict: implementation verified end-to-end, no functional blockers. Three non-blocking findings below — two are description/comment accuracy, one is scope disclosure. Independently verifiedRan on the checked-out head (clean tree): Design conformance, point by point:
Findings (non-blocking)1. PR body misstates the chat-path mechanism. It says the chat path "already normalizes developer→system during the rebuild (kernel coreToOpenai)". That is not what happens: acp-kernel 0.0.54's 2. Test B's comment overclaims what it proves ( 3. Unmentioned bundled change: acp_status unification in both compress loops. This commit also deletes the stale local handlers — 4. Nit (optional): LGTM pending the description fixes (#1 required, #3 required, #2/#4 optional). Merging stays with you — I don't merge PRs. |
#552) Learn-on-failure for compat roles: an upstream 400 'Invalid role: ...' triggers ONE auto-retry with the offending role rewritten to 'system'. On success the mapping is remembered session-scoped (in-memory on the session, never written to config) so later requests skip the 400 round-trip; the info log carries the permanent per-provider snippet. Retry failures pass the original 400 through verbatim. Detection is conservative (two body patterns + stopword guard).
A response body's pending pull can resolve AFTER the consumer called clearTimer (abandoned body — a failed retry whose response is never read, or a client abort racing the last chunk). The old code re-armed a fresh UPSTREAM_TIMEOUT_MS (10 min) timer in that window, pinning the event loop for ten minutes per leaked fetch; the new e2e #552 F suite hung exactly this way. Guard rearm() with a cleared flag set by cleanup().
Follow-up per the maintainer's UX feedback on this PR (thank you!) — the amendment is now implemented on top of
9603e42asd2b15b3+9b6efc2.Auto-fix: no config needed for the common case
compatblock — it prints a per-provider snippet with the real upstream origin (bili knows exactly which provider rejected), ready to copy-paste into the config file only if the user wants the fix to be permanent.{role: "system"}, retries once, and on success records the mapping insession.metadata.learnedCompatRoles(same precedent aslearnedContextLimits). Later requests in the same session apply the learned map up-front and skip the 400 round-trip entirely. The config file is never touched.New e2e coverage: E proves the learn (3 upstream hits: rejected → retried → pre-rewritten), F proves the verbatim 400 + single retry.
Bonus bug found & fixed while testing (
9b6efc2)The new F test exposed a fetch-util timer leak that pins the event loop for 10 minutes: a response body's pending
pullcan resolve after the consumer already calledclearTimer(abandoned body — exactly F's failed retry, but also any client abort racing the last chunk). The late chunk re-armed a freshUPSTREAM_TIMEOUT_MStimer that nothing would ever clear.rearm()is now a no-op oncecleanup()has run, and F asserts_liveUpstreamTimersForTest() === 0as a regression guard. This class was previously documented at the#411call site — it could plausibly explain stray long-lived idle procs in the wild.Pre-flight: typecheck ✓ · targeted 13/13 ✓ · full suite 1080 pass / 2 known plugin-agent env fails (baseline) ✓ · build ✓.