fix: NeedsClarification for habit-flavored titles without frequency

src/Orbit.Api/Controllers/AiController.cs

@@ -1,8 +1,13 @@
+using System.Text.Json;
+using System.Text.Json.Nodes;
 using System.Text.Json.Serialization;
+using FluentValidation;
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
 using Orbit.Api.Extensions;
 using Orbit.Api.RateLimiting;
+using Orbit.Application.Chat.Models;
+using Orbit.Application.Common;
 using Orbit.Domain.Interfaces;
 using Orbit.Domain.Models;
 
@@ -15,9 +20,11 @@ public class AiController(
     IAgentCatalogService catalogService,
     IAgentPolicyEvaluator policyEvaluator,
     IPendingAgentOperationStore pendingOperationStore,
+    IPendingClarificationStore pendingClarificationStore,
     IAgentStepUpService stepUpService,
     IAgentAuditService auditService,
-    IAgentOperationExecutor operationExecutor) : ControllerBase
+    IAgentOperationExecutor operationExecutor,
+    IValidator<ResolveClarificationRequest> resolveClarificationValidator) : ControllerBase
 {
     [HttpGet("capabilities")]
     public async Task<IActionResult> GetCapabilitiesMetadata(CancellationToken cancellationToken)
@@ -294,4 +301,202 @@ await auditService.RecordAsync(new AgentAuditEntry(
 
         return Ok(result);
     }
+
+    [HttpPost("clarifications/{operationId:guid}/resolve")]
+    [DistributedRateLimit("chat")]
+    public async Task<IActionResult> ResolveClarification(
+        Guid operationId,
+        [FromBody] ResolveClarificationRequest body,
+        CancellationToken cancellationToken)
+    {
+        // Clarification cards are a UI-only flow — API-key clients can't render or tap them.
+        // Mirrors the guard on ExecutePendingOperation and the step-up endpoints.
+        var authMethod = HttpContext.User.GetAgentAuthMethod();
+        if (authMethod == AgentAuthMethod.ApiKey)
+            return Forbid();
+
+        var userId = HttpContext.GetUserId();
+
+        var validation = await resolveClarificationValidator.ValidateAsync(body, cancellationToken);
+        if (!validation.IsValid)
+        {
+            var firstError = validation.Errors[0];
+            await RecordResolveAuditAsync(
+                userId,
+                authMethod,
+                operationId,
+                AgentPolicyDecisionStatus.Denied,
+                AgentOperationStatus.Failed,
+                $"invalid_clarification_value:{firstError.PropertyName}",
+                cancellationToken);
+            return BadRequest(new { error = firstError.ErrorMessage });
+        }
+
+        var pending = await pendingClarificationStore.GetForResolutionAsync(operationId, userId, cancellationToken);
+        if (pending is null)
+        {
+            await RecordResolveAuditAsync(
+                userId,
+                authMethod,
+                operationId,
+                AgentPolicyDecisionStatus.Denied,
+                AgentOperationStatus.Failed,
+                "clarification_not_found",
+                cancellationToken);
+            return NotFound(new { error = ErrorMessages.ClarificationNotFound });
+        }
+
+        // The patch must be one of the server-offered quick-action values. This is a
+        // defense-in-depth check: prevents a malicious client from hand-crafting a patch
+        // that overrides fields the contract never said could be changed.
+        if (!pending.AllowedValues.Contains(body.Value, StringComparer.Ordinal))
+        {
+            await RecordResolveAuditAsync(
+                userId,
+                authMethod,
+                operationId,
+                AgentPolicyDecisionStatus.Denied,
+                AgentOperationStatus.Failed,
+                "clarification_value_not_offered",
+                cancellationToken);
+            return BadRequest(new { error = ErrorMessages.ClarificationValueNotOffered });
+        }
+
+        JsonElement mergedArgs;
+        try
+        {
+            mergedArgs = MergeClarificationValue(pending.PartialArgumentsJson, body.Value);
+        }
+        catch (JsonException)
+        {
+            await RecordResolveAuditAsync(
+                userId,
+                authMethod,
+                operationId,
+                AgentPolicyDecisionStatus.Denied,
+                AgentOperationStatus.Failed,
+                "invalid_clarification_value",
+                cancellationToken);
+            return BadRequest(new { error = ErrorMessages.ClarificationValueNotJsonObject });
+        }
+
+        // Atomic one-shot claim: if this returns false, either another concurrent request
+        // already marked the row resolved OR the row expired in the (typically sub-ms)
+        // window between Get and MarkResolved. Bail before re-invoking.
+        //
+        // The clarification is intentionally consumed BEFORE ExecuteAsync runs. If the
+        // executor throws or the tool returns Failed/Denied, the clarification is gone —
+        // the user must re-initiate the request via chat. This is acceptable because:
+        //   (a) the alternative (un-claim on failure) reopens TOCTOU races on retry,
+        //   (b) tool Failed/Denied is surfaced in the response so the client can prompt
+        //       the user appropriately,
+        //   (c) re-asking in chat is a natural recovery path the user already understands.
+        var claimed = await pendingClarificationStore.MarkResolvedAsync(operationId, userId, cancellationToken);
+        if (!claimed)
+        {
+            var auditError = pending.ExpiresAtUtc <= DateTime.UtcNow
+                ? "clarification_expired_mid_flight"
+                : "clarification_already_resolved";
+            await RecordResolveAuditAsync(
+                userId,
+                authMethod,
+                operationId,
+                AgentPolicyDecisionStatus.Denied,
+                AgentOperationStatus.Failed,
+                auditError,
+                cancellationToken);
+            return Conflict(new { error = ErrorMessages.ClarificationAlreadyResolved });
+        }
+
+        var result = await operationExecutor.ExecuteAsync(new AgentExecuteOperationRequest(
+            userId,
+            pending.ToolName,
+            mergedArgs,
+            AgentExecutionSurface.Chat,
+            authMethod,
+            HttpContext.User.GetGrantedAgentScopes(),
+            HttpContext.User.IsReadOnlyCredential(),
+            ConfirmationToken: null,
+            HttpContext.TraceIdentifier), cancellationToken);
+
+        await RecordResolveAuditAsync(
+            userId,
+            authMethod,
+            operationId,
+            result.Operation.Status == AgentOperationStatus.Succeeded
+                ? AgentPolicyDecisionStatus.Allowed
+                : AgentPolicyDecisionStatus.Denied,
+            result.Operation.Status,
+            result.Operation.PolicyReason,
+            cancellationToken,
+            targetName: result.Operation.TargetName);
+
+        return Ok(result);
+    }
+
+    private Task RecordResolveAuditAsync(
+        Guid userId,
+        AgentAuthMethod authMethod,
+        Guid operationId,
+        AgentPolicyDecisionStatus policyDecision,
+        AgentOperationStatus outcome,
+        string? error,
+        CancellationToken cancellationToken,
+        string? targetName = null)
+    {
+        return auditService.RecordAsync(new AgentAuditEntry(
+            userId,
+            AgentCapabilityIds.ChatInteract,
+            nameof(ResolveClarification),
+            AgentExecutionSurface.Chat,
+            authMethod,
+            AgentRiskClass.Low,
+            policyDecision,
+            outcome,
+            HttpContext.TraceIdentifier,
+            "Resolve clarification",
+            TargetId: operationId.ToString(),
+            TargetName: targetName,
+            Error: error), cancellationToken);
+    }
+
+    private static JsonElement MergeClarificationValue(string baseJson, string value)
+    {
+        // Fail closed if the stored args aren't a JSON object — silently coercing to {}
+        // would drop the original tool arguments and replay the tool with only the patch.
+        if (JsonNode.Parse(baseJson) is not JsonObject baseNode)
+            throw new JsonException("Stored partial arguments are not a JSON object.");
+
+        if (!string.IsNullOrWhiteSpace(value))
+        {
+            if (JsonNode.Parse(value) is not JsonObject patchNode)
+                throw new JsonException("Clarification value must be a JSON object.");
+
+            DeepMerge(baseNode, patchNode);
+        }
+
+        return JsonDocument.Parse(baseNode.ToJsonString()).RootElement.Clone();
+    }
+
+    /// <summary>
+    /// Recursively merges <paramref name="patch"/> into <paramref name="target"/>. When a
+    /// key exists on both sides and both values are JsonObjects, the merge recurses into
+    /// the nested object instead of clobbering it. Other types (arrays, primitives) are
+    /// replaced as-is. Patches we currently emit are flat top-level keys, but the deep
+    /// merge keeps nested fields in <c>PartialArgumentsJson</c> safe under future patches.
+    /// </summary>
+    private static void DeepMerge(JsonObject target, JsonObject patch)
+    {
+        foreach (var kvp in patch.ToList())
+        {
+            if (target[kvp.Key] is JsonObject targetChild && kvp.Value is JsonObject patchChild)
+            {
+                DeepMerge(targetChild, patchChild);
+            }
+            else
+            {
+                target[kvp.Key] = kvp.Value?.DeepClone();
+            }
+        }
+    }
 }

src/Orbit.Api/Extensions/ServiceCollectionExtensions.cs

@@ -142,6 +142,7 @@ public static WebApplicationBuilder AddOrbitAiServices(this WebApplicationBuilde
         builder.Services.AddScoped<IGoalReviewService, AiGoalReviewService>();
         builder.Services.AddScoped<IAgentCatalogService, AgentCatalogService>();
         builder.Services.AddScoped<IPendingAgentOperationStore, PendingAgentOperationStore>();
+        builder.Services.AddScoped<IPendingClarificationStore, PendingClarificationStore>();
         builder.Services.AddScoped<IAgentStepUpService, AgentStepUpService>();
         builder.Services.AddScoped<IAgentPolicyEvaluator, AgentPolicyEvaluator>();
         builder.Services.AddScoped<IAgentAuditService, AgentAuditService>();
@@ -255,7 +256,8 @@ public static WebApplicationBuilder AddOrbitAiServices(this WebApplicationBuilde
                 sp.GetRequiredService<IPayGateService>(),
                 sp.GetRequiredService<IUnitOfWork>(),
                 sp.GetRequiredService<IServiceScopeFactory>(),
-                sp.GetRequiredService<IAgentOperationExecutor>()));
+                sp.GetRequiredService<IAgentOperationExecutor>(),
+                sp.GetRequiredService<IPendingClarificationStore>()));
 
         return builder;
     }

src/Orbit.Application/Chat/Commands/ProcessUserChatCommand.cs

@@ -3,6 +3,7 @@
 using MediatR;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
+using Orbit.Application.Chat.Models;
 using Orbit.Application.Chat.Tools;
 using Orbit.Application.Common;
 using Orbit.Domain.Common;
@@ -42,9 +43,10 @@ public record ActionResult(
     string? EntityName = null,
     string? Error = null,
     string? Field = null,
-    IReadOnlyList<AiAction>? SuggestedSubHabits = null);
+    IReadOnlyList<AiAction>? SuggestedSubHabits = null,
+    ClarificationRequest? ClarificationRequest = null);
 
-public enum ActionStatus { Success, Failed, Suggestion }
+public enum ActionStatus { Success, Failed, Suggestion, NeedsClarification }
 
 /// <summary>
 /// Groups AI-related dependencies to reduce constructor parameter count (S107).
@@ -76,7 +78,8 @@ public record ChatExecutionDependencies(
     IPayGateService PayGateService,
     IUnitOfWork UnitOfWork,
     IServiceScopeFactory ServiceScopeFactory,
-    IAgentOperationExecutor OperationExecutor);
+    IAgentOperationExecutor OperationExecutor,
+    IPendingClarificationStore PendingClarificationStore);
 
 public partial class ProcessUserChatCommandHandler(
     ChatDataDependencies data,
@@ -363,12 +366,49 @@ public async Task<Result<ChatResponse>> Handle(
 
         var operationResult = executionResponse.Operation;
         var toolResult = BuildToolCallResult(call, operationResult);
+        var isClarification = operationResult.Payload is NeedsClarificationPayload;
 
-        if (operationResult.Status == AgentOperationStatus.Succeeded)
+        if (operationResult.Status == AgentOperationStatus.Succeeded && !isClarification)
             LogToolSucceeded(logger, call.Name, operationResult.TargetName);
         else if (operationResult.Status is AgentOperationStatus.Failed or AgentOperationStatus.Denied)
             LogToolFailed(logger, call.Name, operationResult.PolicyReason);
 
+        if (operationResult.Status == AgentOperationStatus.Succeeded
+            && operationResult.Payload is NeedsClarificationPayload payload)
+        {
+            // Serialize null as an empty array literal — JsonSerializer.Serialize(null)
+            // returns the string "null" which bypasses PendingClarification.Create's
+            // "[]" default and would break ExtractQuickActionValues on read.
+            var quickActionsJson = payload.QuickActions is null
+                ? "[]"
+                : JsonSerializer.Serialize(payload.QuickActions);
+
+            var stashedId = await execution.PendingClarificationStore.CreateAsync(
+                request.UserId,
+                call.Name,
+                call.Args.GetRawText(),
+                payload.MissingArgumentKey,
+                payload.Question,
+                quickActionsJson,
+                cancellationToken);
+            LogClarificationRequested(logger, call.Name, stashedId, payload.MissingArgumentKey);
+            var clarification = new ClarificationRequest(
+                payload.Question,
+                stashedId,
+                payload.MissingArgumentKey,
+                payload.QuickActions);
+            return (
+                toolResult,
+                new ActionResult(
+                    ToolNameToPascalCase(call.Name),
+                    ActionStatus.NeedsClarification,
+                    EntityName: call.Name,
+                    ClarificationRequest: clarification),
+                operationResult,
+                executionResponse.PolicyDenial,
+                executionResponse.PendingOperation);
+        }
+
         return operationResult.Status switch
         {
             AgentOperationStatus.PendingConfirmation => (
@@ -718,6 +758,9 @@ private static async Task IncrementAiMessageCountAsync(
     [LoggerMessage(EventId = 8, Level = LogLevel.Warning, Message = "Tool {Name} failed: {Error}")]
     private static partial void LogToolFailed(ILogger logger, string name, string? error);
 
+    [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 = 9, Level = LogLevel.Error, Message = "Tool {Name} threw an exception")]
     private static partial void LogToolThrewException(ILogger logger, Exception ex, string name);
 

src/Orbit.Application/Chat/Models/ClarificationRequest.cs

@@ -0,0 +1,18 @@
+namespace Orbit.Application.Chat.Models;
+
+/// <summary>
+/// External-facing "I need to ask the user a question" payload. The frontend renders
+/// quick-action buttons; tapping one POSTs the chosen <c>QuickAction.Value</c> to
+/// <c>POST /api/ai/clarifications/{OperationId}/resolve</c>, which merges the value
+/// into the partial arguments stash and re-invokes the original tool deterministically.
+/// <para>
+/// Tools do not construct this directly — they return a <see cref="NeedsClarificationPayload"/>
+/// and the chat handler attaches the store-minted <c>OperationId</c> when building the
+/// outbound <c>ActionResult</c>.
+/// </para>
+/// </summary>
+public record ClarificationRequest(
+    string Question,
+    Guid OperationId,
+    string MissingArgumentKey,
+    IReadOnlyList<QuickAction> QuickActions);

src/Orbit.Application/Chat/Models/NeedsClarificationPayload.cs

@@ -0,0 +1,17 @@
+namespace Orbit.Application.Chat.Models;
+
+/// <summary>
+/// Internal payload returned by a tool that wants to ask the user a question instead
+/// of executing. The chat handler stashes this server-side, mints an <c>OperationId</c>
+/// from the store, and constructs the external-facing <see cref="ClarificationRequest"/>
+/// before surfacing it to the frontend.
+/// <para>
+/// Tools should not attempt to populate an <c>OperationId</c> themselves — the handler
+/// owns that field. Keeping the tool's output free of the id avoids a <c>Guid.Empty</c>
+/// sentinel leaking into the contract.
+/// </para>
+/// </summary>
+public record NeedsClarificationPayload(
+    string Question,
+    string MissingArgumentKey,
+    IReadOnlyList<QuickAction>? QuickActions);

src/Orbit.Application/Chat/Models/QuickAction.cs

@@ -0,0 +1,16 @@
+namespace Orbit.Application.Chat.Models;
+
+/// <summary>
+/// A choice the user can tap on a <see cref="ClarificationRequest"/> card.
+/// </summary>
+/// <param name="Label">i18n key (or literal text) shown on the button.</param>
+/// <param name="Value">
+/// Opaque token the client must echo back verbatim when resolving the clarification.
+/// On the server it carries a JSON merge patch (e.g. <c>{"frequency_unit":"Day","frequency_quantity":1}</c>)
+/// that gets applied to the stashed partial arguments. Compared with byte-for-byte
+/// equality (<c>StringComparer.Ordinal</c>) against the set of values offered when
+/// the clarification was issued — clients MUST NOT re-serialize, re-order keys, or
+/// trim whitespace, or the resolve endpoint will reject with 400.
+/// </param>
+/// <param name="Description">Optional secondary label shown below the button.</param>
+public record QuickAction(string Label, string Value, string? Description = null);