-
Notifications
You must be signed in to change notification settings - Fork 0
fix: stop AI from logging habits the user didn't mention #166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
793758c
0e77c9c
796d96d
7e52ec0
e3cb864
dc63cfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ public class BulkSkipHabitsTool( | |
| public string Name => "bulk_skip_habits"; | ||
|
|
||
| public string Description => | ||
| "Skip multiple habits for today in a single operation. For recurring habits, advances due date to next scheduled occurrence. For one-time tasks, postpones to tomorrow. Does not log completion. Works on habits that are due today or overdue."; | ||
| "Skip multiple habits for today in a single operation. 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. For recurring habits, advances due date to next scheduled occurrence. For one-time tasks, postpones to tomorrow. Does not log completion. Works on habits that are due today or overdue."; | ||
|
thomasluizon marked this conversation as resolved.
|
||
|
|
||
| public object GetParameterSchema() => new | ||
| { | ||
|
|
@@ -49,13 +49,15 @@ public async Task<ToolResult> ExecuteAsync(JsonElement args, Guid userId, Cancel | |
| var today = await userDateService.GetUserTodayAsync(userId, ct); | ||
| var skippedNames = new List<string>(); | ||
|
|
||
| // Batch-load all requested habits in a single query instead of N+1 | ||
| var habits = await habitRepository.FindTrackedAsync( | ||
| h => habitIds.Contains(h.Id) && h.UserId == userId, | ||
| q => q.Include(h => h.Logs), | ||
| ct); | ||
|
Comment on lines
+53
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good fix — the batch load exactly mirrors the existing pattern in |
||
|
|
||
| foreach (var habitId in habitIds) | ||
| { | ||
| var habit = await habitRepository.FindOneTrackedAsync( | ||
| h => h.Id == habitId && h.UserId == userId, | ||
| q => q.Include(h => h.Logs), | ||
| ct); | ||
|
|
||
| var habit = habits.FirstOrDefault(h => h.Id == habitId); | ||
| if (habit is not null && await TrySkipHabit(habit, today, ct)) | ||
| skippedNames.Add(habit.Title); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ public class DuplicateHabitTool( | |
| public string Name => "duplicate_habit"; | ||
|
|
||
| public string Description => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This description change was required to satisfy the newly-strengthened |
||
| "Create an exact copy of an existing habit with all its properties."; | ||
| "Duplicate an existing habit, creating an exact copy with all its properties."; | ||
|
|
||
| public object GetParameterSchema() => new | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ public void ToolMetadata_ExposesExpectedNamesDescriptionsAndSchemas() | |
| AssertTool(queryHabitsTool, "query_habits", "habits", "include_metrics", expectReadOnly: true); | ||
| 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"); | ||
|
thomasluizon marked this conversation as resolved.
thomasluizon marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a silent pre-existing bug: the old |
||
| AssertTool(updateGoalStatusTool, "update_goal_status", "goal", "status"); | ||
| AssertTool(updateGoalTool, "update_goal", "goal", "target_value"); | ||
| AssertTool(updateHabitTool, "update_habit", "habit", "frequency_unit"); | ||
|
|
@@ -72,8 +72,13 @@ private static void AssertTool(Orbit.Application.Chat.Tools.IAiTool tool, string | |
| { | ||
| tool.Name.Should().Be(expectedName); | ||
| tool.Description.Should().NotBeNullOrWhiteSpace(); | ||
| tool.Description.ToLowerInvariant().Should().Contain(descriptionFragment.ToLowerInvariant(), | ||
|
thomasluizon marked this conversation as resolved.
|
||
| $"tool '{expectedName}' description should mention '{descriptionFragment}'"); | ||
| tool.IsReadOnly.Should().Be(expectReadOnly); | ||
| JsonSerializer.Serialize(tool.GetParameterSchema()).Should().Contain("\"type\""); | ||
| var schema = JsonSerializer.Serialize(tool.GetParameterSchema()); | ||
| schema.Should().Contain("\"type\""); | ||
| schema.Should().Contain(schemaFragment, | ||
| $"tool '{expectedName}' parameter schema should include '{schemaFragment}'"); | ||
| } | ||
|
|
||
| private static IGenericRepository<T> Repo<T>() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.