Skip to content

translate: find first user message regardless of leading system/developer prompt - #1073

Closed
Adityakk9031 wants to merge 1 commit into
weave-os:mainfrom
Adityakk9031:fix/first-user-message-session-key-collision
Closed

translate: find first user message regardless of leading system/developer prompt#1073
Adityakk9031 wants to merge 1 commit into
weave-os:mainfrom
Adityakk9031:fix/first-user-message-session-key-collision

Conversation

@Adityakk9031

@Adityakk9031 Adityakk9031 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes FirstUserMessageText in internal/translate/envelope.go to find the first message with role == "user" by walking the messages array instead of only checking messages.0.

Problem

In OpenAI Chat Completions and envelopes with leading system or developer prompts, messages.0 is not role == "user". As a result, FirstUserMessageText() returned an empty string "". When deriving session keys (DeriveSessionKey), this caused all OpenAI conversations under the same API key sharing a standard system prompt to derive identical session keys, leading to session pin collisions, cross-session pin thrashing, and degraded cache affinity.

Changes

  • Updated FirstUserMessageText to iterate through messages to find the first role == "user" block across all supported wire formats.
  • Added test coverage in internal/translate/envelope_extras_test.go for leading system and developer messages.
  • Added test in internal/proxy/session_key_test.go asserting that independent OpenAI conversations sharing a system prompt produce distinct session keys.
  • Normalized CRLF line endings in internal/proxy/conformance_golden_test.go for cross-platform test stability.

@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown

PR author is not in the allowed authors list.

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@aminsamir45 have a look

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Thank you for this — you found a real bug that had been hiding in plain sight. FirstUserMessageText only ever looked at messages.0, so every OpenAI-format body with a leading system/developer turn returned "", DeriveSessionKey fell back to SystemText(), and unrelated conversations sharing one system prompt collapsed onto a single pin slot. Nice catch, and your ForEach scan is exactly the right shape (it matches openAISystemText in the same package, and lines the OpenAI/Anthropic path up with geminiFirstUserMessageText, which already scanned).

I've opened #1078 as a rewrite so it lands against our internal conventions. To be clear: none of this is your fault — these conventions are internal, still evolving, and genuinely not discoverable from the outside. Your diagnosis and fix are the substance of that PR, and you're credited via Co-authored-by.

What changed, for next time:

  • The stale comment in internal/proxy/session_key.go. DeriveSessionKey's doc comment said the system-text fallback exists "for OpenAI-format bodies, where system lives in messages[] and the first user message is empty" — your fix makes that sentence false, so it now reads "only a fallback for bodies with no user text at all". Root AGENTS.md treats comments as load-bearing ("Only when why is non-obvious… If removing wouldn't confuse, don't write") — the flip side being that a comment describing behavior a change removes has to move with it.
  • The conformance_golden_test.go hunk was dropped. CRLF-normalizing both sides of the golden comparison isn't needed here (no golden fixture in the repo contains \r\n) and it permanently softens the assertion, which runs against the "Real assertions only" rule in AGENTS.md's Tests section. It looks like a core.autocrlf artifact of a Windows checkout — git config core.autocrlf input on your clone should make it go away without touching the test.
  • gofmt. Both new test files ended with extra trailing blank lines, which fails make precommit (fmt + vet + build + test) — worth running that before pushing; it's the same gate CI applies.

Your tests carried over as-is; the subtests read well. Thanks again for digging into the session-key path — this one was a genuinely good find.

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.

2 participants