Skip to content

fix: bump initial commit #3

Open
kszinhu wants to merge 3 commits intosaleszera:mainfrom
kszinhu:main
Open

fix: bump initial commit #3
kszinhu wants to merge 3 commits intosaleszera:mainfrom
kszinhu:main

Conversation

@kszinhu
Copy link
Copy Markdown

@kszinhu kszinhu commented Nov 27, 2025

This PR focuses on fixes ChatKit codebase. The most significant updates include enforcing required attributes for thread items, fixing a typo in the mime_type parameter.

Changes:

  • Fix CI "setup ruby" stage
  • Fix a typo
  • Fix some method access

Changes: * Add x86_64-linux platform to Gemfile.lock
Changes:

* Added a check for required attributes (`id`, `thread_id`, `created_at`) in `Thread::Item.from_event`, raising an `ArgumentError` if any are missing. 
* Made the `parse_workflow_data` method in `Thread::Item` a class method and updated its usage accordingly for clarity and correctness. 
* Added a `client` reader attribute to `Session` for easier access to the `ChatKit` client instance.
Copy link
Copy Markdown
Owner

@saleszera saleszera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your fix is great overall, but there are some points that we need to discuss

Comment on lines +81 to +84
missing_attrs = REQUIRED_ATTRIBUTES.reject { |attr| data.key?(attr) }
raise ArgumentError, "Missing required attributes: #{missing_attrs.join(', ')}" unless missing_attrs.empty?


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation is unnecessary, and you might break the response stream, and it's just a response. I don't think it makes sense to have a validation here

Comment thread Gemfile.lock
Comment thread lib/chatkit/conversation/response/thread/item.rb
Comment thread lib/chatkit/files/response.rb
Comment thread lib/chatkit/session.rb
Comment thread spec/chatkit/files/response_spec.rb
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