fix: NeedsClarification for habit-flavored titles without frequency - #167
Conversation
Two-layer fix for AI silently creating one-time tasks:
- Prompt: tighten CreateHabitTool.Description; new structuring rule
for habit/rotina/hábito titles without a schedule
- Tool: returns ClarificationRequest payload when frequency_unit is
absent on a habit-keyword title
- Infra: new ActionStatus.NeedsClarification, PendingClarification
entity + store, POST /api/ai/clarifications/{id}/resolve endpoint
- Prompt: new ClarificationGuidanceSection teaching the model not
to over-ask after a clarification round-trip
- Tests: 14 new clarification-trigger unit tests
Refs thomasluizon/orbit-ui-mobile#98 thomasluizon/orbit-ui-mobile#99
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements an AI tool clarification feature allowing tools to return a ClarificationRequest, stash partial arguments server-side, present quick-action options to users, and resume the original operation via POST /api/ai/clarifications/{operationId}/resolve which merges the chosen JSON patch and executes the operation. ChangesAI Tool Clarification Request System
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant Chat as Chat Handler
participant Tool as CreateHabitTool
participant Store as PendingClarificationStore
participant API as ResolveClarification API
User->>Chat: Send message with habit mention, no frequency
Chat->>Tool: Execute create_habit with partial args
Tool->>Chat: Return ToolResult with NeedsClarificationPayload
Chat->>Store: CreateAsync(pending clarification, 30min TTL)
Store->>Chat: Return OperationId
Chat->>User: Display clarification card with quick actions
User->>API: POST /api/ai/clarifications/{id}/resolve with JSON patch
API->>Store: GetForResolutionAsync(id, userId)
Store->>API: Return partial args + allowed values
API->>API: Merge patch into partial args
API->>Store: MarkResolvedAsync(id, userId)
API->>Chat: Execute create_habit with merged args (ConfirmationToken: null)
Chat->>User: Create habit with selected frequency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 unit tests (beta)
Comment |
![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: 056f335b35
ℹ️ 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 architecture is clean — server-side stash + dedicated resolve endpoint is the right call over a synthetic chat turn. The two-layer fix (prompt + tool heuristic) is well-reasoned and test coverage is solid. A few issues need addressing before merge. Must-fix
Should-fix
Nit
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
tests/Orbit.Application.Tests/Chat/Tools/CreateHabitToolClarificationTests.cs (1)
111-122: ⚡ Quick winConsider validating QuickAction.Value structure beyond JSON parseability.
The test confirms each
QuickAction.Valueis valid JSON but doesn't verify the structure or content. Consider adding assertions to ensure the parsed JSON contains the expected fields (e.g.,frequency_unit,frequency_quantity) with correct values for each action type.🧪 Example enhancement
[Fact] public async Task ClarificationQuickActions_ContainValidJsonPatches() { var result = await Execute("""{"title": "Morning habit"}"""); var clarification = (ClarificationRequest)result.Payload!; foreach (var action in clarification.QuickActions) { var parsed = () => JsonDocument.Parse(action.Value); parsed.Should().NotThrow($"QuickAction '{action.Label}' value should be valid JSON"); + + using var doc = JsonDocument.Parse(action.Value); + doc.RootElement.TryGetProperty("frequency_unit", out _) + .Should().BeTrue($"QuickAction '{action.Label}' should contain frequency_unit"); } }🤖 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/CreateHabitToolClarificationTests.cs` around lines 111 - 122, Update the test ClarificationQuickActions_ContainValidJsonPatches to not only parse QuickAction.Value but also validate the JSON shape and expected fields: after parsing (using JsonDocument.Parse(action.Value)) assert the root contains expected properties like "frequency_unit" and "frequency_quantity" (and any other action-specific keys) and verify their types/values match the expected values for each QuickAction.Label; use the existing Execute(...) result cast to ClarificationRequest and iterate clarification.QuickActions to perform these assertions so malformed or semantically incorrect patches fail the test.src/Orbit.Infrastructure/Persistence/OrbitDbContext.cs (1)
183-185: ⚡ Quick winReplace new inline limits with shared constants.
The new max-length values are hardcoded in this mapping. Please route these through shared constants to keep limits centralized.
As per coding guidelines,
Use AppConstants for magic numbers (MaxSubHabits, MaxUserFacts, etc.), never inline limits.🤖 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/Orbit.Infrastructure/Persistence/OrbitDbContext.cs` around lines 183 - 185, Replace the hardcoded max-length integers in the EF mapping with centralized constants: change the calls that set HasMaxLength on ToolName, MissingArgumentKey, and Question inside OrbitDbContext's model configuration to use existing AppConstants (or add new constants like AppConstants.MaxToolNameLength, AppConstants.MaxMissingArgumentKeyLength, AppConstants.MaxQuestionLength) instead of 100/500; update or add those constants in the AppConstants class so the mapping uses the named values and remove magic numbers from the entity.Property(...) calls.src/Orbit.Infrastructure/Services/PendingClarificationStore.cs (1)
11-11: ⚡ Quick winUse shared constants for clarification TTL.
The 30-minute TTL is introduced as an inline numeric constant in this service. Move it to shared constants to keep policy values centralized.
As per coding guidelines,
Use AppConstants for magic numbers (MaxSubHabits, MaxUserFacts, etc.), never inline limits.Also applies to: 29-29
🤖 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/Orbit.Infrastructure/Services/PendingClarificationStore.cs` at line 11, The TTL value (private const int TtlMinutes = 30) in PendingClarificationStore should be moved to the centralized AppConstants to avoid inline magic numbers; add a suitably named constant (e.g. AppConstants.PendingClarificationTtlMinutes or AppConstants.ClarificationTtlMinutes), replace the local TtlMinutes usage in the PendingClarificationStore class with that AppConstants member, and remove the local constant declaration so all TTL policy values are sourced from AppConstants.src/Orbit.Api/Controllers/AiController.cs (2)
16-23: ⚡ Quick winAdd structured business-event logging for the clarification resolve path.
The controller still lacks
ILogger<AiController>injection and the new resolve operation does not log business events.As per coding guidelines,
All controllers must inject ILogger<T> and log business events in format: logger.LogInformation("Action {Property}", value) with structured properties in PascalCase.Also applies to: 301-336
🤖 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/Orbit.Api/Controllers/AiController.cs` around lines 16 - 23, The AiController constructor must inject ILogger<AiController> and the clarification resolve handler must emit structured business-event logs using PascalCase property names; add ILogger<AiController> to the AiController parameter list and assign to a private readonly field, then in the clarification resolve path (the resolve method referenced around lines 301-336) add logger.LogInformation calls such as logger.LogInformation("ClarificationResolved {ClarificationId} {AgentId} {UserId}", clarificationId, agentId, userId) and any additional context (operation status, timestamp) using PascalCase property keys to follow the project's logging guideline.
310-310: ⚡ Quick winUse shared API error constants instead of hardcoded strings.
Please replace the new literal error messages with
ErrorMessagesconstants.As per coding guidelines,
Use ErrorMessages constants from Common/ErrorMessages.cs, never hardcode error strings.Also applies to: 319-319
🤖 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/Orbit.Api/Controllers/AiController.cs` at line 310, Replace the hardcoded NotFound error strings in AiController (the NotFound(...) calls that return "Clarification not found or expired." and the similar literal at the other occurrence) with the shared ErrorMessages constants from Common/ErrorMessages.cs (e.g., ErrorMessages.ClarificationNotFound or the appropriate constant name); ensure the controller imports the Common namespace (or use the fully-qualified ErrorMessages) and update both NotFound(...) calls to pass the constant instead of the literal string.
🤖 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.
Inline comments:
In `@src/Orbit.Api/Controllers/AiController.cs`:
- Around line 340-350: MergeClarificationValue currently overwrites top-level
keys (baseNode[kvp.Key] = kvp.Value) which drops nested fields in
PartialArgumentsJson; change it to perform a recursive deep-merge: when
iterating patchNode entries, if both baseNode[kvp.Key] and kvp.Value are
JsonObject instances, merge their children recursively (e.g., via a helper
MergeJsonObjects(JsonObject baseObj, JsonObject patchObj)), otherwise replace as
now; ensure you clone nodes (DeepClone) when assigning to avoid shared
references and keep the existing function name MergeClarificationValue and
variables baseNode/patchNode for locating the logic.
- Line 335: The controller is returning Ok(result) directly; replace that with
the PayGate-aware adapter by calling result.ToPayGateAwareResult(v => Ok(v)) so
the endpoint uses Extensions/ResultActionResultExtensions.cs for PAY_GATE
handling; locate the return in AiController (the action that currently returns
Ok(result)) and change it to use ToPayGateAwareResult, and remove any manual
403/PAY_GATE response logic to comply with the guideline.
- Around line 308-335: The current flow (GetForResolutionAsync +
MarkResolvedAsync) can race and allow double execution; replace it with a single
atomic claim operation on pendingClarificationStore (e.g.,
ClaimForResolutionAsync or ClaimAndGetForResolutionAsync) that performs an
UPDATE setting ResolvedAtUtc (with a WHERE ResolvedAtUtc IS NULL AND
ExpiresAtUtc > now AND OperationId = operationId AND UserId = userId) and
returns the claimed record or null; call this new Claim method instead of
GetForResolutionAsync/MarkResolvedAsync, validate the returned claimed item
(return NotFound if null), run MergeClarificationValue on the claimed payload,
and only then call operationExecutor.ExecuteAsync so execution occurs only when
the claim succeeded.
- Around line 343-346: MergeClarificationValue currently calls
JsonNode.Parse(...)? .AsObject() which throws InvalidOperationException for
valid JSON that is not an object (array/primitive); update both occurrences (the
one around line 337 and the block creating patchNode at the AsObject call) to
validate the parsed JsonNode's Kind or use TryGetProperty-style checks instead
of calling AsObject directly, and if the parsed node is not a JsonObject return
a 400 Bad Request (or wrap the parse in a try/catch that catches
InvalidOperationException and responds 400) so non-object JSON patches are
rejected explicitly rather than causing a 500; reference MergeClarificationValue
and the patchNode creation where JsonNode.Parse(...)? .AsObject() is used.
In `@src/Orbit.Application/Chat/Tools/Implementations/CreateHabitTool.cs`:
- Around line 323-328: The IsHabitFlavoredTitle method currently checks for
"hábito" but misses the non-accented Portuguese form; update the OR conditions
in IsHabitFlavoredTitle to also check title.Contains("habito",
StringComparison.OrdinalIgnoreCase) (and optionally other variants like plural
if desired) so inputs like "meu habito matinal" are recognized as
habit-flavored.
In `@src/Orbit.Domain/Entities/PendingClarification.cs`:
- Around line 53-56: MarkResolved currently overwrites ResolvedAtUtc and uses
DateTime.UtcNow; change it to preserve one-shot semantics by only setting
ResolvedAtUtc when it's not already set (i.e., if ResolvedAtUtc is default/null)
and remove direct DateTime.UtcNow usage — instead accept the timestamp via a
parameter or obtain it from the shared time abstraction (e.g., an injected
IClock/IDateTimeProvider) so MarkResolved (and the ResolvedAtUtc property) are
set once and use a testable time source.
- Around line 27-46: The Create factory on PendingClarification must validate
inputs before constructing the instance: inside PendingClarification.Create,
check that userId != Guid.Empty (throw ArgumentException), toolName,
missingArgumentKey and question are not null/empty/whitespace (throw
ArgumentNullException or ArgumentException), and that expiresAtUtc is in the
future (expiresAtUtc > DateTime.UtcNow; throw ArgumentException if not). Keep
the existing defaulting for PartialArgumentsJson and QuickActionsJson but
perform these invariant checks at the top of the Create method and throw clear
exceptions if any validation fails.
---
Nitpick comments:
In `@src/Orbit.Api/Controllers/AiController.cs`:
- Around line 16-23: The AiController constructor must inject
ILogger<AiController> and the clarification resolve handler must emit structured
business-event logs using PascalCase property names; add ILogger<AiController>
to the AiController parameter list and assign to a private readonly field, then
in the clarification resolve path (the resolve method referenced around lines
301-336) add logger.LogInformation calls such as
logger.LogInformation("ClarificationResolved {ClarificationId} {AgentId}
{UserId}", clarificationId, agentId, userId) and any additional context
(operation status, timestamp) using PascalCase property keys to follow the
project's logging guideline.
- Line 310: Replace the hardcoded NotFound error strings in AiController (the
NotFound(...) calls that return "Clarification not found or expired." and the
similar literal at the other occurrence) with the shared ErrorMessages constants
from Common/ErrorMessages.cs (e.g., ErrorMessages.ClarificationNotFound or the
appropriate constant name); ensure the controller imports the Common namespace
(or use the fully-qualified ErrorMessages) and update both NotFound(...) calls
to pass the constant instead of the literal string.
In `@src/Orbit.Infrastructure/Persistence/OrbitDbContext.cs`:
- Around line 183-185: Replace the hardcoded max-length integers in the EF
mapping with centralized constants: change the calls that set HasMaxLength on
ToolName, MissingArgumentKey, and Question inside OrbitDbContext's model
configuration to use existing AppConstants (or add new constants like
AppConstants.MaxToolNameLength, AppConstants.MaxMissingArgumentKeyLength,
AppConstants.MaxQuestionLength) instead of 100/500; update or add those
constants in the AppConstants class so the mapping uses the named values and
remove magic numbers from the entity.Property(...) calls.
In `@src/Orbit.Infrastructure/Services/PendingClarificationStore.cs`:
- Line 11: The TTL value (private const int TtlMinutes = 30) in
PendingClarificationStore should be moved to the centralized AppConstants to
avoid inline magic numbers; add a suitably named constant (e.g.
AppConstants.PendingClarificationTtlMinutes or
AppConstants.ClarificationTtlMinutes), replace the local TtlMinutes usage in the
PendingClarificationStore class with that AppConstants member, and remove the
local constant declaration so all TTL policy values are sourced from
AppConstants.
In
`@tests/Orbit.Application.Tests/Chat/Tools/CreateHabitToolClarificationTests.cs`:
- Around line 111-122: Update the test
ClarificationQuickActions_ContainValidJsonPatches to not only parse
QuickAction.Value but also validate the JSON shape and expected fields: after
parsing (using JsonDocument.Parse(action.Value)) assert the root contains
expected properties like "frequency_unit" and "frequency_quantity" (and any
other action-specific keys) and verify their types/values match the expected
values for each QuickAction.Label; use the existing Execute(...) result cast to
ClarificationRequest and iterate clarification.QuickActions to perform these
assertions so malformed or semantically incorrect patches fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08e43719-784d-4fa6-9f46-5936669c21fb
📒 Files selected for processing (21)
src/Orbit.Api/Controllers/AiController.cssrc/Orbit.Api/Extensions/ServiceCollectionExtensions.cssrc/Orbit.Application/Chat/Commands/ProcessUserChatCommand.cssrc/Orbit.Application/Chat/Models/ClarificationRequest.cssrc/Orbit.Application/Chat/Models/QuickAction.cssrc/Orbit.Application/Chat/Tools/Implementations/CreateHabitTool.cssrc/Orbit.Domain/Entities/PendingClarification.cssrc/Orbit.Domain/Interfaces/IAgentPlatformServices.cssrc/Orbit.Domain/Models/PendingClarificationData.cssrc/Orbit.Infrastructure/Migrations/20260519165526_AddPendingClarifications.Designer.cssrc/Orbit.Infrastructure/Migrations/20260519165526_AddPendingClarifications.cssrc/Orbit.Infrastructure/Migrations/OrbitDbContextModelSnapshot.cssrc/Orbit.Infrastructure/Persistence/OrbitDbContext.cssrc/Orbit.Infrastructure/Services/PendingClarificationStore.cssrc/Orbit.Infrastructure/Services/Prompts/Sections/Static/ClarificationGuidanceSection.cssrc/Orbit.Infrastructure/Services/Prompts/Sections/Static/StructuringStrategySection.cssrc/Orbit.Infrastructure/Services/SystemPromptBuilder.cstests/Orbit.Application.Tests/Chat/Tools/CreateHabitToolClarificationTests.cstests/Orbit.Application.Tests/Chat/Tools/CreateHabitToolTests.cstests/Orbit.Application.Tests/Commands/Chat/ProcessUserChatCommandHandlerTests.cstests/Orbit.Infrastructure.Tests/Controllers/AiControllerTests.cs
Backend hardening from review:
- Atomic resolve: MarkResolvedAsync now uses ExecuteUpdateAsync with a
WHERE ResolvedAtUtc IS NULL guard, returning bool. Closes the TOCTOU
window where two concurrent resolves could both invoke the tool. The
controller returns 409 Conflict if it didn't claim the row.
- Audit trail: every branch of ResolveClarification now records an
AgentAuditEntry (not-found, invalid value, already-resolved, success,
tool-failure) — matches the ExecutePendingOperation pattern.
- Rate limiting: [DistributedRateLimit("chat")] on the resolve endpoint.
- FluentValidation: new ResolveClarificationRequestValidator
enforcing non-empty + 2048-char max on Value. Moved
ResolveClarificationRequest to Orbit.Application.Chat.Models so the
validator can target it. Auto-discovered via
AddValidatorsFromAssemblyContaining.
- Defensive merge: MergeClarificationValue now throws JsonException
(not InvalidOperationException → 500) when the patch is valid JSON
but not an object (e.g. "[]", "1").
- Tests: replaced fragile i18n-key label assertions with JSON-patch
Value assertions (the load-bearing contract); added validator tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/Orbit.Application/Chat/Validators/ResolveClarificationRequestValidator.cs (2)
8-8: 💤 Low valueMove
MaxValueLengthtoAppConstants.This limit should be centralized alongside other max-length constants. As per coding guidelines, "Use AppConstants for magic numbers (MaxSubHabits, MaxUserFacts, etc.), never inline limits."
🤖 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/Orbit.Application/Chat/Validators/ResolveClarificationRequestValidator.cs` at line 8, Remove the inline constant MaxValueLength from ResolveClarificationRequestValidator and replace its usages with a centralized constant on AppConstants (e.g., AppConstants.MaxValueLength); add the new MaxValueLength constant to AppConstants alongside other limits (matching value 2048), update ResolveClarificationRequestValidator to reference AppConstants.MaxValueLength, and delete the now-redundant MaxValueLength declaration from the validator class.
12-16: 💤 Low valueUse
ErrorMessagesconstants for validation messages.The hardcoded error strings should use constants from
Common/ErrorMessages.csfor consistency. As per coding guidelines, "Use ErrorMessages constants from Common/ErrorMessages.cs, never hardcode error strings."♻️ Proposed fix
Add to
ErrorMessages.cs:public const string ClarificationValueEmpty = "Clarification value cannot be empty."; public const string ClarificationValueTooLong = "Clarification value cannot exceed {0} characters.";Then update the validator:
RuleFor(x => x.Value) .NotEmpty() - .WithMessage("Clarification value cannot be empty.") + .WithMessage(ErrorMessages.ClarificationValueEmpty) .MaximumLength(MaxValueLength) - .WithMessage($"Clarification value cannot exceed {MaxValueLength} characters."); + .WithMessage(string.Format(ErrorMessages.ClarificationValueTooLong, MaxValueLength));🤖 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/Orbit.Application/Chat/Validators/ResolveClarificationRequestValidator.cs` around lines 12 - 16, The validator currently hardcodes messages in ResolveClarificationRequestValidator's RuleFor(x => x.Value) chain; replace those strings with ErrorMessages constants from Common/ErrorMessages.cs (add ClarificationValueEmpty and ClarificationValueTooLong constants as suggested) and use the ClarificationValueTooLong with string formatting to inject MaxValueLength so the MaximumLength rule uses the constant message; update the RuleFor call to reference ErrorMessages.ClarificationValueEmpty and string.Format(ErrorMessages.ClarificationValueTooLong, MaxValueLength) for the respective WithMessage invocations.src/Orbit.Infrastructure/Services/PendingClarificationStore.cs (1)
11-11: 💤 Low valueMove
TtlMinutestoAppConstants.The TTL value should be centralized in
AppConstantsrather than inlined here. As per coding guidelines, "Use AppConstants for magic numbers (MaxSubHabits, MaxUserFacts, etc.), never inline limits."♻️ Proposed fix
- private const int TtlMinutes = 30;Add to
AppConstants:public const int PendingClarificationTtlMinutes = 30;Then reference it here:
DateTime.UtcNow.AddMinutes(AppConstants.PendingClarificationTtlMinutes)🤖 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/Orbit.Infrastructure/Services/PendingClarificationStore.cs` at line 11, Move the inline TTL constant out of PendingClarificationStore by adding a new constant PendingClarificationTtlMinutes = 30 to AppConstants, replace references to the local TtlMinutes in PendingClarificationStore (e.g., the DateTime.UtcNow.AddMinutes(...) call and any other uses of TtlMinutes) with AppConstants.PendingClarificationTtlMinutes, and remove the private const int TtlMinutes from the PendingClarificationStore class.tests/Orbit.Application.Tests/Chat/Validators/ResolveClarificationRequestValidatorTests.cs (1)
26-31: ⚡ Quick winConsider verifying error details for consistency.
The other failure tests (
Empty_Value_FailsandTooLong_Value_Fails) verify error details (property name or message content), but this test only checksIsValid. Adding an assertion likeresult.Errors.Should().Contain(e => e.PropertyName == "Value")would improve robustness—if the validator later fails whitespace for an unintended reason, the test would catch it.✨ Suggested assertion for consistency
[Fact] public void Whitespace_Value_Fails() { var result = _validator.Validate(new ResolveClarificationRequest(" ")); result.IsValid.Should().BeFalse(); + result.Errors.Should().Contain(e => e.PropertyName == "Value"); }🤖 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/Validators/ResolveClarificationRequestValidatorTests.cs` around lines 26 - 31, Update the Whitespace_Value_Fails test to assert the validation error targets the expected property: after calling _validator.Validate(new ResolveClarificationRequest(" ")) and assigning to result, add an assertion like result.Errors.Should().Contain(e => e.PropertyName == "Value") (or match the same message/content used in Empty_Value_Fails/TooLong_Value_Fails) so the test verifies the failure is for the Value property specifically; reference the test method Whitespace_Value_Fails, the ResolveClarificationRequest instantiation, the _validator variable and the result.Errors collection when adding the assertion.
🤖 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/Orbit.Application/Chat/Validators/ResolveClarificationRequestValidator.cs`:
- Line 8: Remove the inline constant MaxValueLength from
ResolveClarificationRequestValidator and replace its usages with a centralized
constant on AppConstants (e.g., AppConstants.MaxValueLength); add the new
MaxValueLength constant to AppConstants alongside other limits (matching value
2048), update ResolveClarificationRequestValidator to reference
AppConstants.MaxValueLength, and delete the now-redundant MaxValueLength
declaration from the validator class.
- Around line 12-16: The validator currently hardcodes messages in
ResolveClarificationRequestValidator's RuleFor(x => x.Value) chain; replace
those strings with ErrorMessages constants from Common/ErrorMessages.cs (add
ClarificationValueEmpty and ClarificationValueTooLong constants as suggested)
and use the ClarificationValueTooLong with string formatting to inject
MaxValueLength so the MaximumLength rule uses the constant message; update the
RuleFor call to reference ErrorMessages.ClarificationValueEmpty and
string.Format(ErrorMessages.ClarificationValueTooLong, MaxValueLength) for the
respective WithMessage invocations.
In `@src/Orbit.Infrastructure/Services/PendingClarificationStore.cs`:
- Line 11: Move the inline TTL constant out of PendingClarificationStore by
adding a new constant PendingClarificationTtlMinutes = 30 to AppConstants,
replace references to the local TtlMinutes in PendingClarificationStore (e.g.,
the DateTime.UtcNow.AddMinutes(...) call and any other uses of TtlMinutes) with
AppConstants.PendingClarificationTtlMinutes, and remove the private const int
TtlMinutes from the PendingClarificationStore class.
In
`@tests/Orbit.Application.Tests/Chat/Validators/ResolveClarificationRequestValidatorTests.cs`:
- Around line 26-31: Update the Whitespace_Value_Fails test to assert the
validation error targets the expected property: after calling
_validator.Validate(new ResolveClarificationRequest(" ")) and assigning to
result, add an assertion like result.Errors.Should().Contain(e => e.PropertyName
== "Value") (or match the same message/content used in
Empty_Value_Fails/TooLong_Value_Fails) so the test verifies the failure is for
the Value property specifically; reference the test method
Whitespace_Value_Fails, the ResolveClarificationRequest instantiation, the
_validator variable and the result.Errors collection when adding the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cca93890-5642-4b1c-a6f9-58dc108a32b5
📒 Files selected for processing (8)
src/Orbit.Api/Controllers/AiController.cssrc/Orbit.Application/Chat/Models/ResolveClarificationRequest.cssrc/Orbit.Application/Chat/Validators/ResolveClarificationRequestValidator.cssrc/Orbit.Domain/Interfaces/IAgentPlatformServices.cssrc/Orbit.Infrastructure/Services/PendingClarificationStore.cstests/Orbit.Application.Tests/Chat/Tools/CreateHabitToolClarificationTests.cstests/Orbit.Application.Tests/Chat/Validators/ResolveClarificationRequestValidatorTests.cstests/Orbit.Infrastructure.Tests/Controllers/AiControllerTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/Orbit.Application/Chat/Models/ResolveClarificationRequest.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Orbit.Domain/Interfaces/IAgentPlatformServices.cs
- tests/Orbit.Infrastructure.Tests/Controllers/AiControllerTests.cs
- tests/Orbit.Application.Tests/Chat/Tools/CreateHabitToolClarificationTests.cs
- src/Orbit.Api/Controllers/AiController.cs
Review summaryWell-designed feature. The two-layer defence (prompt + tool heuristic) is the right approach, the atomic one-shot claim via Four issues flagged inline:
The red and orange items should be fixed before merge; the two yellow items are worth addressing but not blocking. |
Tightening the resolve endpoint per review: - API-key auth guard on ResolveClarification (mirrors ExecutePendingOperation and the step-up endpoints — clarification cards are a UI-only flow). - Expiry-aware atomic claim: MarkResolvedAsync's WHERE now also requires ExpiresAtUtc > now, so a clarification that ticks over the 30-min TTL between get and claim can't be executed. - Server-offered value allowlist: PendingClarificationData now exposes AllowedValues (extracted from the stored QuickActionsJson). The controller rejects any incoming Value that isn't one of the offered options, closing the "client crafts an arbitrary merge patch" hole. - Validator: Value must parse as a JSON object — unifies the error surface (400 with a clear message) rather than failing later inside MergeClarificationValue. - Centralized constants: MaxClarificationValueLength and PendingClarificationTtlMinutes moved to AppConstants; validation messages added to ErrorMessages. Removes the inline magic numbers and string literals per coding guidelines. - Tests: validator now covers JSON-object rejection (string/array/ number/null/bool), property-name assertion on the whitespace case, and a max-length-valid-JSON happy-path case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Trivial round-3 cleanup — store the auth method once and reuse it for both the ApiKey guard and the rest of the handler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Orbit.Api/Controllers/AiController.cs (1)
19-27: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winInject
ILogger<AiController>and log this flow.The new clarification endpoint adds a business operation, but this controller still doesn't inject
ILogger<AiController>or emit the required structured business-event logs for validation failures, claims, and execution outcomes.As per coding guidelines,
All controllers must inject ILogger<T> and log business events in format: logger.LogInformation("Action {Property}", value) with structured properties in PascalCase.🤖 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/Orbit.Api/Controllers/AiController.cs` around lines 19 - 27, Add an ILogger<AiController> parameter to the AiController constructor (alongside IAgentCatalogService, IAgentPolicyEvaluator, etc.) and store it in a private readonly field; then update the new clarification endpoint (the action that uses ResolveClarificationRequest and resolveClarificationValidator) to emit structured LogInformation calls for the key business events: validation failures (e.g., logger.LogInformation("ValidationFailed {Request}", request)), claims extraction (e.g., logger.LogInformation("Claims {UserId}", userId)), and execution outcomes (e.g., logger.LogInformation("ClarificationResult {Result}", result)); ensure all logged property names use PascalCase and reuse the injected ILogger<AiController> instance.
🤖 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.
Inline comments:
In `@src/Orbit.Api/Controllers/AiController.cs`:
- Around line 452-455: MergeClarificationValue currently coerces any non-object
parsed JsonNode into an empty JsonObject, which silently drops stored args
(e.g., when pending.PartialArgumentsJson is an array or primitive). Change
MergeClarificationValue to parse baseJson into a JsonNode, check if the result
is a JsonObject, and if it is not then fail closed (throw a clear exception like
InvalidOperationException or return the original baseJson as a JsonElement)
instead of creating new JsonObject; only perform the merge logic when the parsed
node is a JsonObject. Reference the MergeClarificationValue method and use the
JsonNode/JsonObject type check to locate and change the behavior.
---
Outside diff comments:
In `@src/Orbit.Api/Controllers/AiController.cs`:
- Around line 19-27: Add an ILogger<AiController> parameter to the AiController
constructor (alongside IAgentCatalogService, IAgentPolicyEvaluator, etc.) and
store it in a private readonly field; then update the new clarification endpoint
(the action that uses ResolveClarificationRequest and
resolveClarificationValidator) to emit structured LogInformation calls for the
key business events: validation failures (e.g.,
logger.LogInformation("ValidationFailed {Request}", request)), claims extraction
(e.g., logger.LogInformation("Claims {UserId}", userId)), and execution outcomes
(e.g., logger.LogInformation("ClarificationResult {Result}", result)); ensure
all logged property names use PascalCase and reuse the injected
ILogger<AiController> instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3332695f-3645-444f-b6e6-8de6e8938e66
📒 Files selected for processing (7)
src/Orbit.Api/Controllers/AiController.cssrc/Orbit.Application/Chat/Validators/ResolveClarificationRequestValidator.cssrc/Orbit.Application/Common/AppConstants.cssrc/Orbit.Application/Common/ErrorMessages.cssrc/Orbit.Domain/Models/PendingClarificationData.cssrc/Orbit.Infrastructure/Services/PendingClarificationStore.cstests/Orbit.Application.Tests/Chat/Validators/ResolveClarificationRequestValidatorTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Orbit.Application.Tests/Chat/Validators/ResolveClarificationRequestValidatorTests.cs
Review SummaryThe design is solid. Two-layer defence (prompt + tool-layer heuristic), atomic one-shot resolve, TOCTOU handled correctly via Blocking
Non-blocking
Everything else — entity factory validation, migration indexes, rate-limit guard, audit trail, deep-merge correctness (null patch value → one-time task confirmed via |
…, AiController test coverage - ProcessUserChatCommand: ClarificationRequest construction now null-coalesces payload.QuickActions to Array.Empty<QuickAction>(). Aligns the nullable upstream type with the non-nullable downstream contract so a future tool emitting a null list can't NPE. - New LogClarificationDroppedOnFailedTool (EventId 25): warns when a tool returns Failed/Denied with a NeedsClarificationPayload — the payload is dropped (we only stash for Succeeded), so the warning surfaces the bug class in production logs. - DeepMerge XML doc-block collapsed to a single-line comment per AGENTS.md "one short line max" convention. - AiControllerTests: 6 new tests covering the ResolveClarification endpoint — ApiKey 403, validator rejection 400, store-miss 404, value-not-offered 400, claim-race 409, and the happy path through to operationExecutor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five clarification-related comment improvements: - ProcessUserChatCommand: moved EventIds 24 and 25 to the end of the LoggerMessage declarations (after EventId 23) so the sequence reads monotonically. They were sandwiched between 8 and 9 and broke a visual scan. - CreateHabitTool: inline comment near the TryGetProperty check explaining the null-vs-missing semantic that lets the "one-time task" quick action bypass the guard on re-invocation. Easy to misregress if someone moves to a value-based check. - QuickAction docstring: corrected "applied" wording to "deep-merged" to match the actual DeepMerge behaviour in the controller. - PendingClarification.ResolvedAtUtc: comment documents why there's no domain Resolve() method — the store uses ExecuteUpdateAsync to keep the claim atomic and intentionally bypasses EF change tracking for that transition. - PendingClarificationStore.CreateAsync: comment explains the eager SaveChangesAsync trade-off (orphans an unused row on chat-command failure; TTL reclaims it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summaryThe clarification-card flow is well-architected: two-layer defence (prompt + tool-layer heuristic), atomic one-shot claim via Four items to address before merge:
|
- ResolveClarificationRequest XML doc: corrected "shallow-merged" to
"deep-merged" so it matches the actual DeepMerge implementation.
- CreateHabitTool.IsHabitFlavoredTitle: removed unaccented "habito"
match. It collides with the PT verb habitar ("Eu habito em Lisboa"
= "I live in Lisbon"), which would wrongly trigger the clarification
card. Users typing PT properly use the accent.
- ProcessUserChatCommand: cap call.Args.GetRawText() at
AppConstants.MaxClarificationArgsLength (16 KB) before stashing.
Runaway tool arguments can't bloat PendingClarifications. Returns
ActionStatus.Failed when exceeded.
- PendingClarificationStore.GetForResolutionAsync: expiry +
resolved-state filters are now in the SQL WHERE clause instead of
C# post-load. Single round-trip stays the same but unusable rows
no longer cross the wire.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review summaryOverall this is a well-designed feature — the two-layer defence (prompt rule + tool-layer heuristic), atomic one-shot claim, user-scoped lookup, Three issues flagged inline:
Everything else — auth, migration, indexes, TOCTOU handling, user scoping, DRY patterns — looks correct. |
…te rate-limit bucket
- New LogClarificationArgsTooLarge (EventId 26): the args-too-large
path was reusing LogClarificationDroppedOnFailedTool, whose
message claimed the tool was Failed/Denied. Misleading because the
tool actually Succeeded but we dropped the clarification because
the payload was too big. Two distinct log lines now.
- ResolveClarificationRequestValidator: Cascade(CascadeMode.Stop) on
the Value rule so an empty value emits one error ("cannot be
empty") instead of also tripping the JSON-object check. Controller
only surfaces Errors[0] anyway; redundant failures were noise.
- New "ai-resolve" rate-limit bucket (30/min) separate from "chat".
Resolving a clarification card no longer counts against the user's
chat quota — a user mid-flow can't get blocked from tapping a
quick action because they hit the chat throttle.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| - User describes a SINGLE weekly occurrence (e.g., "weekly study routine", "every week I want to clean") with no specific day named -> ask which day of the week | ||
| - User says a vague time like "morning" or "evening" without a specific hour | ||
| - Structure is genuinely ambiguous between checklist and sub-habits -> ask "do you want these as a single checklist or individually trackable steps?" | ||
| - User calls something a "habit" / "rotina" / "hábito" without stating daily / weekly / X times per week / a specific schedule -> ASK before calling create_habit. Offer: daily, weekly with specific days, X times per week, or one-time task. Prefer returning a NeedsClarification clarification card from create_habit (with the four quick actions) over a plain-text question. |
There was a problem hiding this comment.
This line contradicts ClarificationGuidanceSection (Order=260), which says:
"Prefer to ask the schedule INLINE in your text BEFORE calling
create_habit… The clarification card is the safety net; the ideal flow is for you to ask first."
But this line says:
"Prefer returning a NeedsClarification clarification card from create_habit (with the four quick actions) over a plain-text question."
The two sections give opposing guidance on which flow is preferred. The model will get contradictory instructions: StructuringStrategySection (lower order ≈ rendered first) says "prefer the card", then ClarificationGuidanceSection says "prefer inline text". Whichever the model anchors on, the other section will seem wrong.
Suggest aligning both to the same preferred flow. Based on the PR description ("the clarification card is the safety net"), the intent appears to be:
- Preferred: Ask inline in text before calling the tool.
- Safety net: If the model still calls the tool without a frequency, the tool returns the card.
| - User calls something a "habit" / "rotina" / "hábito" without stating daily / weekly / X times per week / a specific schedule -> ASK before calling create_habit. Offer: daily, weekly with specific days, X times per week, or one-time task. Prefer returning a NeedsClarification clarification card from create_habit (with the four quick actions) over a plain-text question. | |
| - User calls something a "habit" / "rotina" / "hábito" without stating daily / weekly / X times per week / a specific schedule -> ASK before calling create_habit. Offer: daily, weekly with specific days, X times per week, or one-time task. Prefer asking via plain text first; the NeedsClarification clarification card is the automatic safety net if you call the tool without a frequency. |
| [LoggerMessage(EventId = 26, Level = LogLevel.Warning, Message = "Tool {Name} requested clarification with oversized partial args ({Length} chars) — dropped without stashing")] | ||
| private static partial void LogClarificationArgsTooLarge(ILogger logger, string name, int length); | ||
|
|
||
| [LoggerMessage(EventId = 23, Level = LogLevel.Warning, Message = "Background post-response work failed")] |
There was a problem hiding this comment.
EventId 23 is declared here but EventIds 24, 25, and 26 are declared on lines 825–831 above it. The numeric sequence in the file reads 22 → 24 → 25 → 26 → 23, which makes it look like EventId 23 is missing when scanning the file.
Swap the declaration order so the sequence reads 22 → 23 → 24 → 25 → 26, or at minimum add a comment explaining the ordering.
| [LoggerMessage(EventId = 23, Level = LogLevel.Warning, Message = "Background post-response work failed")] | |
| [LoggerMessage(EventId = 23, Level = LogLevel.Warning, Message = "Background post-response work failed")] | |
| private static partial void LogBackgroundPostResponseFailed(ILogger logger, Exception ex); | |
| [LoggerMessage(EventId = 24, Level = LogLevel.Information, Message = "Tool {Name} requested clarification (operationId={OperationId}, missing={MissingKey})")] | |
| private static partial void LogClarificationRequested(ILogger logger, string name, Guid operationId, string missingKey); | |
| [LoggerMessage(EventId = 25, Level = LogLevel.Warning, Message = "Tool {Name} emitted a clarification payload on a Failed/Denied result and it was dropped: {Reason}")] | |
| private static partial void LogClarificationDroppedOnFailedTool(ILogger logger, string name, string? reason); | |
| [LoggerMessage(EventId = 26, Level = LogLevel.Warning, Message = "Tool {Name} requested clarification with oversized partial args ({Length} chars) — dropped without stashing")] | |
| private static partial void LogClarificationArgsTooLarge(ILogger logger, string name, int length); |
| private readonly IServiceScopeFactory _scopeFactory = Substitute.For<IServiceScopeFactory>(); | ||
| private readonly IAgentCatalogService _catalogService = Substitute.For<IAgentCatalogService>(); | ||
| private readonly IAgentOperationExecutor _operationExecutor = Substitute.For<IAgentOperationExecutor>(); | ||
| private readonly IPendingClarificationStore _pendingClarificationStore = Substitute.For<IPendingClarificationStore>(); |
There was a problem hiding this comment.
_pendingClarificationStore is wired into the handler but no test in this file exercises the NeedsClarification code path through ProcessUserChatCommand. The unit tests for CreateHabitTool confirm the tool emits the right payload, and AiControllerTests covers the resolve endpoint — but the critical in-between logic in ProcessUserChatCommandHandler (args-size cap, CreateAsync call, ActionResult.NeedsClarification construction) is untested here.
Minimum coverage gap:
- Tool returns
NeedsClarificationPayloadwithSuccess=true→ handler callspendingClarificationStore.CreateAsyncand returnsActionStatus.NeedsClarification+ClarificationRequestin the action result. - Tool returns
NeedsClarificationPayloadwith oversized args → handler logs and returnsActionStatus.Failedwithout callingCreateAsync.
_pendingClarificationStore.Received(1).CreateAsync(...) / DidNotReceive() patterns already work for the other stores in this file.
| var claimed = await pendingClarificationStore.MarkResolvedAsync(operationId, userId, cancellationToken); | ||
| if (!claimed) | ||
| { | ||
| var auditError = pending.ExpiresAtUtc <= DateTime.UtcNow |
There was a problem hiding this comment.
pending.ExpiresAtUtc here is the value fetched earlier by GetForResolutionAsync, not a fresh DB read. If MarkResolvedAsync returns false because a concurrent request won the claim race (not because the row expired), but the row's TTL then elapses in the sub-millisecond window between the two calls, this check mis-classifies the audit event as "clarification_expired_mid_flight" when the real cause was "clarification_already_resolved".
In practice this race is negligibly rare (two requests resolve the same clarification within 1 ms of the TTL boundary), so it doesn't affect correctness — only the audit label. Worth noting if audit data is ever used for metrics.
Review summarySolid, well-scoped implementation. The two-layer approach (prompt rule + tool-layer heuristic), server-side stash, atomic one-shot claim, and PayGate-before-clarification ordering are all correct. Security story is clean: Issues (4 inline comments)
What looks good
|
1 participant
Summary
Two-layer fix for the AI silently creating one-time tasks when the user describes something as a habit/rotina/hábito without naming a schedule.
CreateHabitTool.Description; new rule inStructuringStrategySectionso the model asks before callingcreate_habiton a habit-flavored titleCreateHabitToolreturns aClarificationRequestpayload whenfrequency_unitis absent and the title contains "habit"/"rotina"/"hábito"ActionStatus.NeedsClarificationvalue,PendingClarificationEF entity + store,POST /api/ai/clarifications/{operationId}/resolveendpoint that deep-merges the user's JSON-patch value into the stashed partial args and re-invokes the original tool deterministicallyClarificationGuidanceSectionteaching the model not to over-ask after a clarification round-tripLinked issues
Refs thomasluizon/orbit-ui-mobile#98
Refs thomasluizon/orbit-ui-mobile#99
(Issues live in the ui-mobile repo; the ui-mobile PR carries the
Closeskeywords.)Paired PR
Design decisions
PendingClarificationStore+ dedicated endpoint, not a synthetic chat message. Survives page reload, avoids LLM re-interpretation, no policy/confirmation/step-up lifecycle (separate concerns fromPendingAgentOperationStore).frequency_unitabsent AND title contains "habit"/"rotina"/"hábito" (case-insensitive). Verb-phrase cases handled by the prompt rule, not the tool.QuickAction.Valueis a JSON merge patch. Generalizes for multi-key patches (e.g. "3 times per week" setsfrequency_unit + frequency_quantity + is_flexible). Resolve handler does a shallow JSON merge.HabitsWritecapability; clarification is an intermediate state ofcreate_habit, not a new gated operation.Test plan
dotnet build(full solution): PASSdotnet test Orbit.Application.Tests: PASS (1658 passed, 14 new clarification tests)dotnet test Orbit.Infrastructure.Tests --filter AiControllerTests: PASS (17 passed)AddPendingClarificationsDeferred follow-ups
ClarificationFlowTests.csintegration test: existing chat integration tests hit the live OpenAI API, making deterministic clarification testing impractical. A focused integration test (seedPendingClarification→ POST resolve) is a follow-up PR.PendingClarificationrows (TTL already enforces logical expiry; physical cleanup deferred).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests