fix: stop AI from logging habits the user didn't mention - #166
Conversation
Reported: when the user wrote "Eu meditei hoje à noite e já fiz meu
yoga" the AI logged Yoga noturno (correct) AND Fazer anotações no
diário (NOT mentioned). The chat prompt currently says "act
immediately when the user's intent is clear" plus "Use those IDs
directly for actions whenever possible," but never explicitly forbids
substituting a semantically-related habit when no exact-name match
exists. Combined with the bulk_log_habits tool's loose description
("Use this when the user mentions completing several activities at
once"), the model fills in extra habits that share a tag, routine,
parent, or theme with the one the user actually named.
- Add Rule 18 (STRICT MATCH FOR LOG / COMPLETE / SKIP) to
GlobalRulesSection. Only act on habits whose title clearly
corresponds to the words the user used. Obvious translations are
allowed ("meditei" -> "Meditate"); related-but-different habits
are not. If no match exists, tell the user and offer to create
one - never substitute.
- Tighten BulkLogHabitsTool.Description to forbid bundling habits
that share a tag, parent, routine, or theme but weren't named.
Existing ChatToolMetadataTests assertions (contains "multiple" and
"habit_ids") still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 793758c7e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review SummaryThe fix is correct and well-targeted. Root cause analysis is accurate — the model was substituting tangentially-related habits because neither the tool description nor the system prompt explicitly prohibited it. Both the What looks good
Issues
CaveatsPrompt-only mitigation is inherently probabilistic — the LLM may still occasionally substitute habits in complex multi-language contexts. No server-side guard is practical here (you can't verify "did the user say this title" at the API layer), so the prompt approach is the right call; just worth noting in the PR for future reference. Verdict: Approve after adding the |
Codex P1: Rule 18 as written required user's words to match habit
titles, which would regress valid flows like "log that one" or "mark
the first one done" after a list/query. Softened the rule to clearly
restrict SUBSTITUTION ONLY, and added an explicit carve-out: indirect
references resolve to whatever habit the AI was just discussing.
Claude: added a Build_ContainsNoSubstitutionRule test in
GlobalRulesSectionTests so future refactors can't silently drop the
rule. Asserts the key phrases ("NO HABIT SUBSTITUTION FOR LOG /
COMPLETE / SKIP", "bulk_log_habits", "no more, no fewer",
"Indirect references").
Claude: AssertTool in ChatToolMetadataTests took descriptionFragment
and schemaFragment parameters but never asserted on them - so the PR
description's claim that the test guarded those words was wrong.
Wired both parameters into proper Should().Contain assertions (case-
insensitive for the description, exact for the schema). This caught
two pre-existing gaps:
- duplicate_habit description didn't mention "duplicate" at all
("Create an exact copy..."). Updated to "Duplicate an existing
habit, creating an exact copy...". Improves AI tool discoverability
when the user says "duplicate".
- update_goal_progress test asserted on schema fragment "delta", but
the schema uses "current_value" / "goal_name". Updated the fragment.
Full Application.Tests suite: 1642/1642 passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summaryRoot cause analysis is correct and the fix is well-scoped. Rule 18 is clearly written, explicitly carves out indirect references ("log that one"), and avoids over-constraining the model. One gap: Incidental improvements (all good):
No API contract, auth, database, or security concerns — all changes are confined to AI prompt wording and test assertions. |
Round 2 of PR review feedback (Claude bot):
Rule 18 covered log_habit, bulk_log_habits, and skip_habit but
forgot bulk_skip_habits, and BulkSkipHabitsTool.Description was
not tightened the same way as BulkLogHabitsTool. So the same
substitution problem (model skipping habits the user didn't
mention) could occur on bulk skip flows.
- Add bulk_skip_habits to Rule 18's tool list and an example
("pulei o treino") so it's clear the rule applies to skip flows
in any language.
- Mirror BulkLogHabitsTool's tightened description on
BulkSkipHabitsTool: "Use this only for habits the user EXPLICITLY
mentioned skipping - never include extra habits that share a tag,
parent, routine, or theme but were not named."
- Extend Build_ContainsNoSubstitutionRule to assert all four tool
names (log_habit, bulk_log_habits, skip_habit, bulk_skip_habits)
are present in the rule, so future refactors can't silently drop
one.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summaryThe fix is correct and well-targeted. Root cause is properly identified, the two-layer defence (system prompt rule + tool description) is the right approach, and the indirect-reference carve-out in Rule 18 prevents a regression for flows like "log that one". What's good
Two issues noted inline
Both are minor; the core fix is solid. LGTM with those addressed. |
Round 3 of PR review feedback (Claude bot): - Drop "or status-change tools" from Rule 18. The phrase was undefined - for habits there is no separate status-change tool (logging IS the state change). Goal status changes are out of scope for the no-substitution rule. The four habit tools (log_habit, bulk_log_habits, skip_habit, bulk_skip_habits) are the relevant set. - Fix the N+1 in BulkSkipHabitsTool.ExecuteAsync. Previously each habit ID triggered its own FindOneTrackedAsync call inside the loop; BulkLogHabitsTool was already updated to batch-load via FindTrackedAsync with a Contains predicate. Aligning the two tools now. - Update BulkSkipHabitsToolTests SetupHabitLookup helper and the AllNotFound_ReturnsError test to mock FindTrackedAsync instead of FindOneTrackedAsync, since the production call changed. Full Application.Tests suite: 1642/1642 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClarifies tool descriptions to require explicit habit matches, refactors BulkSkipHabitsTool to batch-load tracked habits, adds a global no-substitution prompt rule, and updates tests to validate batch lookup behavior and ownership filtering. ChangesHabit Tool No-substitution Enforcement
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ReviewCorrectness — the root cause analysis is accurate and the fixes address it properly. The N+1 fix in Prompt engineering — Rule 18 in Tests — the infrastructure test No API contract changes, no migration, no auth/authz impact. Two inline comments posted (one actionable test suggestion, one soft style note). Otherwise good to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Orbit.Application.Tests/Chat/Tools/BulkSkipHabitsToolTests.cs (1)
126-130: ⚡ Quick winMake the repository mock predicate-aware to avoid false positives.
SetupHabitLookupcurrently returns all provided habits regardless of the expression passed by the tool, so tests can still pass if filtering logic regresses.Suggested test-mock adjustment
_habitRepo.FindTrackedAsync( Arg.Any<Expression<Func<Habit, bool>>>(), Arg.Any<Func<IQueryable<Habit>, IQueryable<Habit>>?>(), Arg.Any<CancellationToken>() - ).Returns(habits.ToList()); + ).Returns(callInfo => + { + var predicate = callInfo.ArgAt<Expression<Func<Habit, bool>>>(0).Compile(); + return habits.Where(predicate).ToList(); + });🤖 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/Orbit.Application.Tests/Chat/Tools/BulkSkipHabitsToolTests.cs` around lines 126 - 130, The mock for _habitRepo.FindTrackedAsync is returning the entire habits list unconditionally; update the test setup (e.g., SetupHabitLookup or the Arrange where _habitRepo.FindTrackedAsync is configured) to capture the passed Expression<Func<Habit,bool>> predicate and the optional Func<IQueryable<Habit>,IQueryable<Habit>> queryTransform, compile and apply the predicate to the in-memory habits collection (and then apply queryTransform if not null) so the mock returns only matching items; use NSubstitute's Returns(callInfo => ...) or Arg.Do to access callInfo.ArgAt<Expression...>(0) and callInfo.ArgAt<Func<IQueryable<Habit>,IQueryable<Habit>>?>(1) to implement the filtering so tests fail if tool filtering regresses.
🤖 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 `@tests/Orbit.Application.Tests/Chat/Tools/BulkSkipHabitsToolTests.cs`:
- Around line 126-130: The mock for _habitRepo.FindTrackedAsync is returning the
entire habits list unconditionally; update the test setup (e.g.,
SetupHabitLookup or the Arrange where _habitRepo.FindTrackedAsync is configured)
to capture the passed Expression<Func<Habit,bool>> predicate and the optional
Func<IQueryable<Habit>,IQueryable<Habit>> queryTransform, compile and apply the
predicate to the in-memory habits collection (and then apply queryTransform if
not null) so the mock returns only matching items; use NSubstitute's
Returns(callInfo => ...) or Arg.Do to access callInfo.ArgAt<Expression...>(0)
and callInfo.ArgAt<Func<IQueryable<Habit>,IQueryable<Habit>>?>(1) to implement
the filtering so tests fail if tool filtering regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 338c0f51-bbb8-4805-ac1b-dcc3357e9f53
📒 Files selected for processing (7)
src/Orbit.Application/Chat/Tools/Implementations/BulkLogHabitsTool.cssrc/Orbit.Application/Chat/Tools/Implementations/BulkSkipHabitsTool.cssrc/Orbit.Application/Chat/Tools/Implementations/DuplicateHabitTool.cssrc/Orbit.Infrastructure/Services/Prompts/Sections/Static/GlobalRulesSection.cstests/Orbit.Application.Tests/Chat/Tools/BulkSkipHabitsToolTests.cstests/Orbit.Application.Tests/Chat/Tools/ChatToolMetadataTests.cstests/Orbit.Infrastructure.Tests/Services/PromptSectionTests.cs
Round 4 of PR review feedback (Claude bot): - Restructure Rule 18 from a single ~230-word paragraph into a lead sentence + sub-bullets. The dense paragraph could dilute model attention to the key constraint; bulleted structure makes each sub-clause scannable. All assertion phrases in the existing GlobalRulesSectionTests test stay present (key phrase, all four tool names, "no more, no fewer", "Indirect references"). - Restore predicate filtering in BulkSkipHabitsToolTests SetupHabitLookup. The simplified mock from the previous round returned all habits regardless of predicate, which meant the production query's `h.UserId == userId` ownership check was no longer exercised by unit tests. Compile the predicate and apply it to the seeded habits so the ownership scoping is verified the same way it was before the N+1 refactor. - Add DifferentUserHabit_IsNotSkipped regression test that seeds a habit owned by a different user and asserts it is filtered out (result.Success false, "No habits were skipped"). Locks in the ownership scoping explicitly. Application.Tests: 1643/1643 passing (was 1642 - the new test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review SummaryCorrectness & logic — solid. The N+1 fix in Test coverage — good. One gap worth a follow-up: No API contract changes, no auth/authz changes, no migration required. Approve once the manual chat test passes. |
…est coverage
Round 5 of PR review feedback (Claude bot):
- Rule 18 heading: "NO HABIT SUBSTITUTION FOR LOG / COMPLETE / SKIP"
-> "NO HABIT SUBSTITUTION FOR LOG / SKIP". The body already lists
the four real tool names; "COMPLETE" in the heading suggested a
separate complete tool that doesn't exist (logging IS completing
for habits). Heading now matches the actual tool set. Test
assertion updated accordingly.
- BulkLogHabitsToolTests symmetry with BulkSkipHabitsToolTests:
- SetupHabitsFound now compiles + applies the predicate to the
seeded habits, so the production query's `h.UserId == userId`
ownership check is exercised by these tests too (previously
the mock returned all habits regardless of predicate).
- Add DifferentUserHabit_IsNotLogged regression test mirroring
DifferentUserHabit_IsNotSkipped, asserting that a habit owned
by another user is filtered out before the log loop sees it.
Application.Tests: 1644/1644 passing (was 1643 - the new test).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| var habits = await habitRepository.FindTrackedAsync( | ||
| h => habitIds.Contains(h.Id) && h.UserId == userId, | ||
| q => q.Include(h => h.Logs), | ||
| ct); |
There was a problem hiding this comment.
Good fix — the batch load exactly mirrors the existing pattern in BulkLogHabitsTool and eliminates N round-trips to the DB. One detail worth noting: because the ownership filter (h.UserId == userId) lives entirely in this query, TrySkipHabit never needs its own userId guard, which keeps it clean. The approach is correct.
| AssertTool(skipHabitTool, "skip_habit", "Skip", "date"); | ||
| AssertTool(suggestBreakdownTool, "suggest_breakdown", "Suggest", "suggested_sub_habits"); | ||
| AssertTool(updateGoalProgressTool, "update_goal_progress", "goal", "delta"); | ||
| AssertTool(updateGoalProgressTool, "update_goal_progress", "goal", "current_value"); |
There was a problem hiding this comment.
This was a silent pre-existing bug: the old AssertTool helper only checked that the serialised schema contained "type", so the stale "delta" fragment never triggered a failure even though the actual parameter is current_value. The new helper now validates both the description fragment and the schema fragment, which is what caught it. Fix is correct.
| 18. NO HABIT SUBSTITUTION FOR LOG / SKIP. When the user describes an activity ("I meditated", "fiz yoga", "log my workout", "pulei o treino"): | ||
| - Only call log_habit, bulk_log_habits, skip_habit, or bulk_skip_habits on habits whose title clearly matches the described activity. Obvious translations are fine ("meditei" -> "Meditate" / "Meditar"). | ||
| - Do NOT log or skip a habit just because it shares a tag, parent, time-of-day, routine, or general theme with the described activity. | ||
| - If NO habit in the index clearly matches, do NOT substitute a related habit. Tell the user briefly that you don't see a matching habit and ask if they want to create one. | ||
| - When the user describes multiple activities, log exactly the habits they described - no more, no fewer. | ||
| - This rule restricts SUBSTITUTION ONLY. Indirect references like "log that one", "mark the first one done", "skip it", or "complete it" after you have already named a specific habit are still valid - resolve them to the habit you were just discussing, then act. |
There was a problem hiding this comment.
The indirect-reference carve-out on the last bullet is important — without it the rule would break conversational flows like "now log it" after the assistant has already named a habit. Well-targeted.
One heads-up for later: this rule is enforced entirely through the system prompt and tool descriptions; there's no server-side guard that prevents the model from calling these tools with unrelated IDs. That's the right trade-off for a prompt-engineering fix, but if the mismatch recurs it would be worth considering whether a post-call audit log (e.g., logging which IDs were submitted vs. what the user said) could help diagnose future regressions.
| { | ||
| _habitRepo.FindOneTrackedAsync( | ||
| // Apply the predicate so the production query's `h.UserId == userId` | ||
| // ownership check is still exercised by the unit tests. | ||
| _habitRepo.FindTrackedAsync( | ||
| Arg.Any<Expression<Func<Habit, bool>>>(), | ||
| Arg.Any<Func<IQueryable<Habit>, IQueryable<Habit>>?>(), | ||
| Arg.Any<CancellationToken>() | ||
| ).Returns(callInfo => | ||
| { | ||
| var predicate = callInfo.ArgAt<Expression<Func<Habit, bool>>>(0).Compile(); | ||
| return habits.FirstOrDefault(predicate); | ||
| return habits.Where(predicate).ToList(); | ||
| }); |
There was a problem hiding this comment.
Compiling and applying the predicate in-memory is the right approach for a unit test — it exercises the exact ownership expression (h.UserId == userId) that the production code passes to EF Core. The caveat (documented by the comment) is that this doesn't prove EF Core can translate the expression to SQL; the integration test suite covers that path via a real database, which is the correct division of responsibility.
| @@ -9,7 +9,7 @@ public class DuplicateHabitTool( | |||
| public string Name => "duplicate_habit"; | |||
|
|
|||
| public string Description => | |||
There was a problem hiding this comment.
This description change was required to satisfy the newly-strengthened AssertTool check in ChatToolMetadataTests (which now validates that the description contains the expected fragment). The old text "Create an exact copy…" didn't contain "duplicate", so without this tweak the test would fail. Wording reads naturally.
Review summaryChanges reviewed: What's good
No blocking issuesNo API contract changes, no auth regressions, no new N+1 queries, no migration needed. All changes are either prompt engineering, tool descriptions, or test hardening. One note for follow-upRule 18 is enforced purely through the prompt and tool descriptions — the server accepts any valid habit IDs regardless of what the user said. That's the right trade-off here, but if substitution recurs it may be worth adding structured logging of which IDs each AI tool call receives so future regressions can be diagnosed from logs rather than reproduced manually. Verdict: approved, no changes required. |
1 participant
Summary
Reported case: user wrote "Eu meditei hoje à noite e já fiz meu yoga". The AI logged `Yoga noturno` (correct) and `Fazer anotações no diário` (NOT mentioned). It also missed the meditation habit. The model is substituting related-but-different habits when no exact-name match exists.
Root cause
The system prompt currently emphasizes acting immediately when intent is clear (`CoreIdentitySection`) and "Use those IDs directly for actions whenever possible" (`GlobalRulesSection` Rule 13), but it never explicitly forbids picking a habit that's merely tangentially related to what the user said. Combined with `BulkLogHabitsTool`'s loose description — "Use this when the user mentions completing several activities at once" — the model freely fills in extra habits that share a tag, parent, routine, or theme with the one the user named.
Changes
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests