fix: preserve original casing in identify_main_topic#2885
fix: preserve original casing in identify_main_topic#2885praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
Run the capitalized-phrase regex on original-case text instead of a lowercased copy so acronyms/proper nouns like "REST API" are preserved. Action/focus patterns still run on lowercase text. Capitalized candidates are preferred over lowercase action-object fragments. Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
1 similar comment
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesTopic extraction
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR updates conversation topic detection to preserve readable casing. The main changes are:
Confidence Score: 4/5This is close, but the topic ranking should be fixed before merging.
src/praisonai-agents/praisonaiagents/context/conversation.py Important Files Changed
Reviews (2): Last reviewed commit: "fix: rank conversation topics by frequen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2885 ✅
Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Fix implementedGreptile P1 (valid): repeated greetings override real topics. Confirmed by repro — Fix in
Verification
Skipped: Other Phase 3 — Verdict✅ Approve. The original case-preservation fix is sound and correctly scoped; the one valid reviewer-flagged bug (greeting override) is now resolved and pushed to this branch. No blocking issues remain.
Pushed commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/context/conversation.py (1)
88-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCore logic correctly fixes the casing bug; one design concern with sentence-initial words.
The two-set strategy and frequency/length-based selection are sound and produce the expected
"REST API"result for the test fixture. The docstring clearly documents the casing contract. Approving the implementation.One concern:
cap_patternmatches any word starting with an uppercase letter, including common sentence-initial words like "The", "Help", "What". Since frequency is counted viacombined_lower.count(candidate.lower())— a substring search without word boundaries — a sentence-initial word like "Help" can accumulate a high frequency from its lowercase occurrences elsewhere in the conversation, potentially outranking meaningful proper nouns. In the current test fixture this works because "REST API" ties on frequency and wins on length, but conversations heavy with sentence-initial "The"/"This"/"That" could produce unexpected topics.Consider adding a small stopword filter for common sentence-initial words, or using word-boundary-aware counting (e.g.,
len(re.findall(r'\b' + re.escape(candidate.lower()) + r'\b', combined_lower))). This is deferable since the current change is a net improvement over the previous fully-lowercased approach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/context/conversation.py` around lines 88 - 153, Prevent common sentence-initial words from being selected as topics in identify_main_topic: add a small stopword filter for words such as “The”, “This”, and “That”, and replace combined_lower.count(candidate.lower()) with word-boundary-aware matching using re.escape so only complete candidate occurrences contribute to frequency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/context/conversation.py`:
- Around line 88-153: Prevent common sentence-initial words from being selected
as topics in identify_main_topic: add a small stopword filter for words such as
“The”, “This”, and “That”, and replace combined_lower.count(candidate.lower())
with word-boundary-aware matching using re.escape so only complete candidate
occurrences contribute to frequency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d3fea16-771c-41fe-92a5-bdec7384d3b4
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/context/conversation.py
…e real topics Greptile P1: capitalized candidates were ranked entirely before lowercase action candidates, so a repeated greeting (e.g. 'Hello') could win over a more-frequently-discussed lowercase topic. Now all repeated candidates are scored by frequency, with capitalization used only as a tie-breaker. Also bound the action/focus object capture to a short noun phrase so real topics survive the length filter instead of being swallowed by a greedy match. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
| # Prefer higher frequency, then capitalized phrases, then longer. | ||
| return max( | ||
| scored.values(), | ||
| key=lambda item: (item[1], item[2], len(item[0])), |
There was a problem hiding this comment.
When a repeated greeting ties the actual topic, this tie-breaker still returns the greeting. For example, two messages like Hello, please implement a REST API and Hello, I am working on a REST API give Hello and rest api the same frequency, so is_cap makes Hello win. The compacted summary can then show the greeting as the conversation topic instead of the repeated action topic.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #2882
Summary
HybridConversationAnalyzer.identify_main_topic()lowercased all message text before running a regex designed to match capitalized phrases ([A-Z][a-z]+), so that pattern could never match. The function returned"rest api"and the case-sensitive testtest_identify_main_topicfailed on all platforms.Changes
REST API) are preserved.implement,working on, ...) on the lowercase copy.REST APIovera rest api); fall back to action/focus candidates only when no capitalized phrase repeats.Now
identify_main_topic()returns'REST API'for the standard fixture.Test plan
test_identify_main_topicpassestest_conversation_compaction.pypass (no regression intest_llm_analysis_fallback, etc.)Generated with Claude Code
Summary by CodeRabbit