Skip to content

fix: reload legacy session rows#1365

Merged
chenjw merged 2 commits intovolcengine:mainfrom
chethanuk:fix/1358-session-add-message-crash
Apr 13, 2026
Merged

fix: reload legacy session rows#1365
chenjw merged 2 commits intovolcengine:mainfrom
chethanuk:fix/1358-session-add-message-crash

Conversation

@chethanuk
Copy link
Copy Markdown
Contributor

Summary

  • normalize datetime created_at values when serializing messages
  • tolerate legacy session rows that only stored content and may omit created_at
  • add regression coverage for legacy message reload/append behavior

Validation

  • uv run pytest tests/unit/test_message.py -v
  • uv run pytest tests/unit/test_message.py -k "legacy_content_only or legacy_message_can_be_reloaded_and_extended or to_dict_timestamp_format" -v
  • uv run ruff check openviking/message/message.py tests/unit/test_message.py

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1358 - Partially compliant

Compliant requirements:

  • Handle legacy session rows with only 'content' field
  • Tolerate missing 'created_at' in legacy rows
  • Normalize datetime 'created_at' values when serializing

Non-compliant requirements:

  • Fix RuntimeError: not found when appending to session messages file (requires integration testing to verify)

Requires further human verification:

  • Fix RuntimeError: not found when appending to session messages file
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🏅 Score: 90
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle naive datetime objects safely

Add handling for naive datetime objects (without timezone info) to prevent
astimezone() errors. Assume naive datetimes are in UTC and explicitly set their
timezone before conversion.

openviking/message/message.py [66-72]

 created_at_val = self.created_at or datetime.now(timezone.utc).isoformat()
 if isinstance(created_at_val, datetime):
+    if created_at_val.tzinfo is None:
+        # Naive datetime, assume UTC
+        created_at_val = created_at_val.replace(tzinfo=timezone.utc)
     created_at_val = (
         created_at_val.astimezone(timezone.utc)
         .isoformat(timespec="milliseconds")
         .replace("+00:00", "Z")
     )
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential issue with naive datetime objects (without timezone info) that could cause astimezone() to fail. Adding this check improves robustness, and the change is accurate within the PR's context.

Low

@qin-ctx qin-ctx requested a review from chenjw April 13, 2026 06:22
@chenjw chenjw merged commit e6628f9 into volcengine:main Apr 13, 2026
6 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants