Tighten people_map stoplist (Sprint 1 follow-up) - #124
Conversation
Removes residual common-noun false-positives from people extraction (Character, Dogs, Phase, Payment, Reservation, Command/Resort Command, Pandora Memory, Add, ...) by expanding the stoplist and rejecting a name if ANY token is a known non-name word (so multi-word phrases like "Resort Command" no longer register as people). - Keeps alias canonicalization, event_id dedup, and payload caps unchanged. - Real names (Janine Tan, Mang Jun, Joven Del Rosario) still pass. - Test proves nouns dropped, names kept, aliases/dedup/caps intact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe person-name detection heuristic in the memory distillation service was updated: the stopword set was expanded with additional domain nouns, and the filtering logic now rejects a candidate if any token matches a stopword, rather than only the first token. A corresponding unit test was added. ChangesPerson Name Detection Heuristic
Estimated code review effort: 1 (Trivial) | ~5 minutes Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/services/memory-distillation-service.ts (2)
50-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStopword list expansion looks reasonable; minor duplicate entry.
"core"is already present at line 56 and re-added at line 63. Harmless since it's aSet, but worth a quick cleanup pass.🤖 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 `@lib/services/memory-distillation-service.ts` around lines 50 - 64, Clean up the PERSON_NAME_STOPWORDS list in memory-distillation-service.ts by removing the duplicate "core" entry from the set initializer. The issue is harmless because it uses a Set, but the duplicate should be deleted to keep the stopword list tidy and avoid confusion when reviewing or extending the constants.
68-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAny-token stopword check confirmed correct and consistent with regex matching behavior.
Verified the greedy regex
\b[A-Z][a-z]+(?:\s+[A-Z][a-z]+){0,2}\bwill already capture leading stopwords like "The" as part of multi-word matches (e.g. "The Character"), so most of the new test's multi-word junk cases (Resort Command,Pandora Memory) were already excluded by the pre-existing first-token check once their leading word ("resort", "pandora") was in the stopword set. The any-token change genuinely matters for cases where the stopword is not the first token (e.g. a capitalized phrase like"John Payment"), which none of the current tests exercise. Logic itself looks correct.🤖 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 `@lib/services/memory-distillation-service.ts` around lines 68 - 74, The any-token stopword logic in isLikelyPersonName is already correct, so no code change is needed here; keep the existing tokens.some(...) check and ensure any future tests cover cases where a stopword appears after the first token (for example, capitalized phrases like “John Payment”) to preserve the intended behavior.tests/unit/context-stabilization.test.ts (1)
24-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't isolate the new "any-token" behavior.
Traced through the regex against the sample sentence: every multi-word junk phrase in this test (
"Resort Command","Pandora Memory") has a stopword as its first token (resort,pandora), so these cases would already be filtered out by the old first-token-only check once the stopword list was expanded — they don't actually exercise the newtokens.some(...)logic. Similarly,"Add"was already a stopword before this PR (line 53), so it isn't new coverage either.Consider adding a case where the stopword appears only in a non-first position (e.g. a phrase like
"John Command"or"Grace Payment"), which is the actual behavior this diff changes.🤖 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 `@tests/unit/context-stabilization.test.ts` around lines 24 - 35, The current test in extractPeopleMentions does not isolate the new any-token filtering path because the multi-word junk cases are already caught by first-token stopwords and “Add” is preexisting coverage. Update the context-stabilization test data in context-stabilization.test.ts to include at least one phrase where the stopword appears in a non-first position, using the extractPeopleMentions and ev helpers, so the assertion directly exercises the new tokens.some(...) behavior while still verifying real names are preserved.
🤖 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 `@lib/services/memory-distillation-service.ts`:
- Around line 50-64: Clean up the PERSON_NAME_STOPWORDS list in
memory-distillation-service.ts by removing the duplicate "core" entry from the
set initializer. The issue is harmless because it uses a Set, but the duplicate
should be deleted to keep the stopword list tidy and avoid confusion when
reviewing or extending the constants.
- Around line 68-74: The any-token stopword logic in isLikelyPersonName is
already correct, so no code change is needed here; keep the existing
tokens.some(...) check and ensure any future tests cover cases where a stopword
appears after the first token (for example, capitalized phrases like “John
Payment”) to preserve the intended behavior.
In `@tests/unit/context-stabilization.test.ts`:
- Around line 24-35: The current test in extractPeopleMentions does not isolate
the new any-token filtering path because the multi-word junk cases are already
caught by first-token stopwords and “Add” is preexisting coverage. Update the
context-stabilization test data in context-stabilization.test.ts to include at
least one phrase where the stopword appears in a non-first position, using the
extractPeopleMentions and ev helpers, so the assertion directly exercises the
new tokens.some(...) behavior while still verifying real names are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cf4a076-cf11-4aca-ad9c-9097d16370a5
📒 Files selected for processing (2)
lib/services/memory-distillation-service.tstests/unit/context-stabilization.test.ts
2 participants
What
Roadmap Sprint 1 follow-up: remove residual common-noun false-positives from
people_mapthat the initial stoplist (pronouns/imperatives/openers) didn't catch —Character,Dogs,Phase,Payment,Reservation,Command/Resort Command,Pandora Memory,Add, etc. These showed up live in AU + real_lifepeople_mapafter the Sprint 1 deploy.Change —
lib/services/memory-distillation-service.tsPERSON_NAME_STOPWORDSwith common domain nouns.isLikelyPersonNamenow rejects a name if any token is a stopword (so multi-word phrases likeResort Command/Pandora Memoryare no longer treated as people).Tests —
tests/unit/context-stabilization.test.ts(6/6)Added a case proving the residual nouns are dropped while real names (
Janine Tan,Mang Jun,Joven Del Rosario) still pass; existing dedup/alias/caps/slim tests still green.Verification
Scope / safety
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests