-
Notifications
You must be signed in to change notification settings - Fork 175
feat(llma): Support Gemini System Instructions #2254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(llma): Support Gemini System Instructions #2254
Conversation
@cameronaziz is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 4 comments
describe('parseGeminiSystemInstruction', () => { | ||
describe('parseGeminiSystemInstruction', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Duplicate nested describe blocks - the outer one is redundant
describe('parseGeminiSystemInstruction', () => { | |
describe('parseGeminiSystemInstruction', () => { | |
describe('parseGeminiSystemInstruction', () => { |
it('should filter out empty strings but keep non-empty ones', () => { | ||
const instructions = ['', 'Valid instruction', ' ', 'Another valid'] | ||
const result = parseGeminiSystemInstruction(instructions) | ||
expect(result).toEqual([ | ||
{ role: 'system', content: 'Valid instruction' }, | ||
{ role: 'system', content: ' ' }, | ||
{ role: 'system', content: 'Another valid' }, | ||
]) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Test behavior differs from description - whitespace-only strings (' ') are kept, contradicting 'filter out empty strings'
packages/ai/src/sanitization.ts
Outdated
const result = [] | ||
for (const instruction of systemInstruction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider declaring explicit type for result
array as Array<{ role: string; content: string }>
to improve type safety
const result = [] | |
for (const instruction of systemInstruction) { | |
const result: Array<{ role: string; content: string }> = [] | |
for (const instruction of systemInstruction) { |
packages/ai/src/sanitization.ts
Outdated
const result = [] | ||
for (const part of parts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider declaring explicit type for result
array as Array<{ role: string; content: string }>
to improve type safety
const result = [] | |
for (const part of parts) { | |
const result: Array<{ role: string; content: string }> = [] | |
for (const part of parts) { |
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Problem
Currently, when sending events to PostHog for Gemini API calls, the system instructions are not included in the input data that gets tracked.
Changes
parseGeminiSystemInstruction
function to parse various system instruction formats into standardized PostHog event formatgetPartText
helper function to safely extract text content from various part formatssendEventToPostHog
calls in Gemini implementation to pass system instructionsformatInputForPostHog
to accept and include system instructions in the input data sent to PostHogRelease info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changeset
to generate a changeset file