Skip to content

fix: handle None updated_at in DefaultSessionStore.list_sessions() (fixes #1445)#1446

Merged
MervinPraison merged 1 commit intomainfrom
claude/issue-1445-20260417-0719
Apr 17, 2026
Merged

fix: handle None updated_at in DefaultSessionStore.list_sessions() (fixes #1445)#1446
MervinPraison merged 1 commit intomainfrom
claude/issue-1445-20260417-0719

Conversation

@praisonai-triage-agent
Copy link
Copy Markdown
Contributor

@praisonai-triage-agent praisonai-triage-agent bot commented Apr 17, 2026

Fixes the bug where DefaultSessionStore.list_sessions() crashes with a TypeError when any session has updated_at=None.

Changes Made

  1. Fixed the sorting logic in praisonaiagents/session/store.py:493:

    • Changed from: sessions.sort(key=lambda x: x.get("updated_at", ""), reverse=True)
    • Changed to: sessions.sort(key=lambda x: x.get("updated_at") or "", reverse=True)
    • The 'or' operator coerces both missing-key (None from .get()) and explicit None to ""
  2. Added regression test to ensure None values are handled properly:

    • Creates sessions with updated_at=None in metadata
    • Verifies list_sessions() returns results without raising TypeError
    • Confirms None sessions appear last in sorted results

Root Cause

dict.get(key, default) only uses default when the key is missing, not when the stored value is None. Any session whose metadata contains "updated_at": None broke the sort comparison between NoneType and str.

Testing

  • Added comprehensive regression test covering None and missing updated_at scenarios
  • Manually tested with sessions containing None updated_at values
  • All existing tests continue to pass

Fixes #1445

Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where sessions with missing or null update timestamps could cause failures when retrieving the session list. Sessions are now properly sorted regardless of timestamp availability.

- Fix TypeError when sessions have updated_at=None by using 'or' operator
- Add regression test for None updated_at values in session store
- Ensures sessions with None updated_at sort last (as intended)

Fixes #1445
Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@MervinPraison
Copy link
Copy Markdown
Owner

@coderabbitai review

@MervinPraison
Copy link
Copy Markdown
Owner

/review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 17, 2026

βœ… Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 17, 2026

No actionable comments were generated in the recent review. πŸŽ‰

ℹ️ Recent review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96421242-9b5d-41d6-bf8b-b59ff531eeee

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 220d0d5 and ae687dd.

πŸ“’ Files selected for processing (2)
  • src/praisonai-agents/praisonaiagents/session/store.py
  • src/praisonai-agents/tests/unit/session/test_session_store.py

πŸ“ Walkthrough

Walkthrough

A single-line bug fix in DefaultSessionStore.list_sessions() to robustly handle missing or explicit None updated_at values using the or "" operator, accompanied by a regression test that validates the fix without crashes.

Changes

Cohort / File(s) Summary
Session store sorting fix
src/praisonai-agents/praisonaiagents/session/store.py
Updated sort key logic from x.get("updated_at", "") to x.get("updated_at") or "" to coerce both missing keys and explicit None values to empty string, preventing TypeError during sort.
Regression test
src/praisonai-agents/tests/unit/session/test_session_store.py
Added test_list_sessions_with_none_updated_at() to verify list_sessions() handles sessions with updated_at=None without crashing and returns them last in sorted order.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A sorting spell once broke with None in sight,
But or "" coerces values to the light!
Sessions now align without a crash,
Fixed in one lineβ€”a quick, clean dash. ✨

πŸš₯ Pre-merge checks | βœ… 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive The PR scope is limited to the sorting logic fix and test coverage; however, the issue's acceptance criteria also requested applying the same fix to list_sessions_by_agent and list_sessions_by_gateway_agent if they have the same pattern. Verify whether list_sessions_by_agent and list_sessions_by_gateway_agent require the same fix as mentioned in issue #1445 acceptance criteria, and apply if needed.
βœ… Passed checks (4 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title accurately describes the main fix: handling None updated_at in DefaultSessionStore.list_sessions(), and references the issue it resolves (#1445).
Linked Issues check βœ… Passed The PR addresses all primary objectives from issue #1445: fixes the TypeError by changing the sort logic, adds a regression test for None updated_at, and handles both missing and explicit None values.
Docstring Coverage βœ… Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
πŸ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/issue-1445-20260417-0719

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@MervinPraison
Copy link
Copy Markdown
Owner

@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first β€” incorporate their findings.

Review areas:

  1. Bloat check: Are changes minimal and focused? Any unnecessary code or scope creep?
  2. Security: Any hardcoded secrets, unsafe eval/exec, missing input validation?
  3. Performance: Any module-level heavy imports? Hot-path regressions?
  4. Tests: Are tests included? Do they cover the changes adequately?
  5. Backward compat: Any public API changes without deprecation?
  6. Code quality: DRY violations, naming conventions, error handling?
  7. Address reviewer feedback: If Qodo, Coderabbit, or Gemini flagged valid issues, include them in your review
  8. Suggest specific improvements with code examples where possible

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 17, 2026

@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first β€” incorporate their findings.

Review areas:...

Reviewed at commit ae687dd.

Findings:

  1. Bloat: Scope is minimal (1-line fix + 1 regression test).
  2. Security: No secrets, no eval/exec, no new input surface.
  3. Performance: No new imports; sort remains O(n log n) with same hot-path cost.
  4. Tests: Regression test is present and valid. I re-ran tests/unit/session/test_session_store.py locally: 30 passed.
  5. Backward compat: No API/signature changes.
  6. Code quality: Change is clear and idiomatic; no DRY or error-handling regressions.
  7. Reviewer feedback: I checked existing PR comments/reviews. No actionable findings from Qodo/Gemini were present; CodeRabbit comments were trigger/progress notes only (no code issues raised).
  8. Optional improvement (non-blocking): make ordering deterministic for equal/missing timestamps:
sessions.sort(
    key=lambda x: (x.get("updated_at") or "", x.get("session_id") or ""),
    reverse=True,
)

Also checked branch CI metadata: many workflow runs report failure with zero jobs started on this branch, which looks workflow/infrastructure-side rather than caused by this code diff.

@MervinPraison
Copy link
Copy Markdown
Owner

@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.

Phase 1: Review per AGENTS.md

  1. Protocol-driven: check heavy implementations vs core SDK
  2. Backward compatible: ensure zero feature regressions
  3. Performance: no hot-path regressions

Phase 2: FIX Valid Issues
4. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
5. Push all code fixes directly to THIS branch (do NOT create a new PR)
6. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
7. If all issues are resolved, approve the PR / close the Issue
8. If blocking issues remain, request changes / leave clear action items

@praisonai-triage-agent
Copy link
Copy Markdown
Contributor Author

praisonai-triage-agent bot commented Apr 17, 2026

Claude finished @MervinPraison's task β€”β€” View job


Architecture Review Complete βœ…

Phase 1: Architecture Review per AGENTS.md

  • Read AGENTS.md for architecture guidelines
  • Review changed files for protocol compliance
  • Check backward compatibility
  • Assess performance impact

Phase 2: Address Reviewer Feedback

  • Analyze CodeRabbit, Copilot, and other reviewer comments
  • Check if list_sessions_by_agent and list_sessions_by_gateway_agent need similar fixes
  • Implement any required fixes

Phase 3: Final Verdict

  • Approve PR or request changes
  • Update issue status

Architecture Compliance βœ…

The fix adheres to AGENTS.md principles:

  • Protocol-driven: Fixing implementation detail in existing protocol (SessionStoreProtocol)
  • No performance impact: Single-character change (or "") with identical O(n log n) complexity
  • Backward compatible: No API changes, only internal sorting logic
  • Minimal scope: Targeted fix for specific TypeError issue

Reviewer Analysis βœ…

Valid Issues Found:

  1. CodeRabbit flagged missing similar fixes in list_sessions_by_agent and list_sessions_by_gateway_agent
  2. Both methods iterate over session files but don't use sorting, so no similar fix needed
  3. Copilot's feedback was comprehensive - no additional issues

Verification Results:

  • list_sessions_by_agent:498: No sorting logic - only collects session IDs
  • list_sessions_by_gateway_agent:610: No sorting logic - only collects session IDs
  • Original fix at list_sessions:493 correctly handles the specific sorting TypeError
  • Regression test properly covers the bug scenario
  • Manual test confirms fix works βœ…

Final Verdict: APPROVED βœ…

This PR successfully addresses issue #1445 with:

  1. Minimal, targeted fix: One-character change (or "") that coerces both missing keys and explicit None to empty string
  2. Complete test coverage: Comprehensive regression test ensuring None values don't crash sorting
  3. Architecture compliance: Follows protocol-driven design principles
  4. No additional fixes needed: Other methods don't perform sorting operations
  5. Zero performance impact: Same O(n log n) complexity, no import-time overhead

Recommendation: This PR is ready to merge. The fix is minimal, well-tested, and architecturally sound.


Job run

Copilot AI requested a review from MervinPraison April 17, 2026 07:27
@MervinPraison MervinPraison merged commit f17de6e into main Apr 17, 2026
19 checks passed
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.

Bug: DefaultSessionStore.list_sessions() crashes with TypeError on None updated_at

2 participants