Fix chat to auto-create proposals on actionable intent#567
Conversation
When the LLM classifies a message as actionable (e.g. "create card", "move card"), the chat service now automatically creates a review-first proposal via AutomationPlannerService without requiring the user to explicitly check RequestProposal. The RequestProposal flag is repurposed as a force-try mechanism for non-actionable messages. Fixes the core issue: chat surface responded with prose but never created proposals or tasks. Fixes #512
Add patterns like "create a task", "new card", "make a task", "remove card", "rename board" so the mock provider detects actionable intent from natural language, not just exact command syntax.
Cover: auto-proposal without RequestProposal, status hint when no board scope, graceful fallback when planner cannot parse, and explicit RequestProposal for non-actionable messages.
The test previously expected "status" for actionable messages without RequestProposal; update to expect "proposal-reference" now that chat auto-creates proposals.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-reviewGP-06 complianceVerified: all actionable intents route through Behavior changes
Traceability improvementNow passes Pre-existing issue noted
No issues found requiring action
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the chat service by automatically creating task proposals when actionable intent is detected by the LLM, even without an explicit user request. It also expands the LlmIntentClassifier with more natural language patterns and adds comprehensive unit and API tests for these new flows. Feedback was provided regarding significant code duplication in the ChatService proposal handling logic and the use of non-deterministic time functions in unit tests.
| if (llmResult.IsActionable) | ||
| { | ||
| if (!session.BoardId.HasValue) | ||
| { | ||
| messageType = "error"; | ||
| assistantContent = "Actionable instructions require a board-scoped chat session. Create a session with BoardId and retry."; | ||
| // Surface a hint so the user knows why no proposal was created | ||
| assistantContent = $"{llmResult.Content}\n\n(To act on this, open a board-scoped chat session.)"; | ||
| messageType = "status"; | ||
| } | ||
| else | ||
| { | ||
| var proposalResult = await _automationPlanner.ParseInstructionAsync( | ||
| dto.Content, | ||
| userId, | ||
| session.BoardId, | ||
| ct); | ||
| ct, | ||
| sourceType: ProposalSourceType.Chat, | ||
| sourceReferenceId: session.Id.ToString()); | ||
|
|
||
| if (proposalResult.IsSuccess) | ||
| { | ||
| messageType = "proposal-reference"; | ||
| proposalId = proposalResult.Value.Id; | ||
| assistantContent = $"{llmResult.Content}\n\nProposal created: {proposalResult.Value.Id}"; | ||
| assistantContent = $"{llmResult.Content}\n\nProposal created for review: {proposalResult.Value.Id}"; | ||
| } | ||
| else | ||
| { | ||
| messageType = "error"; | ||
| assistantContent = $"I could not create a proposal: {proposalResult.ErrorMessage}"; | ||
| // Planner could not parse — return the LLM prose with a hint | ||
| assistantContent = $"{llmResult.Content}\n\n(I detected a task request but could not parse it into a proposal: {proposalResult.ErrorMessage})"; | ||
| messageType = "status"; | ||
| } | ||
| } | ||
| } | ||
| else if (llmResult.IsActionable) | ||
| else if (dto.RequestProposal && session.BoardId.HasValue) | ||
| { | ||
| messageType = "status"; | ||
| // User explicitly requested proposal even though LLM did not | ||
| // detect actionable intent — try the planner anyway. | ||
| var proposalResult = await _automationPlanner.ParseInstructionAsync( | ||
| dto.Content, | ||
| userId, | ||
| session.BoardId, | ||
| ct, | ||
| sourceType: ProposalSourceType.Chat, | ||
| sourceReferenceId: session.Id.ToString()); | ||
|
|
||
| if (proposalResult.IsSuccess) | ||
| { | ||
| messageType = "proposal-reference"; | ||
| proposalId = proposalResult.Value.Id; | ||
| assistantContent = $"{llmResult.Content}\n\nProposal created for review: {proposalResult.Value.Id}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication in handling proposal creation. The logic for calling _automationPlanner.ParseInstructionAsync and handling the success case is repeated. This can be refactored to improve maintainability and reduce redundancy. By combining the conditions, you can have a single path for proposal creation that handles all cases correctly.
var shouldAttemptProposal = llmResult.IsActionable || (dto.RequestProposal && session.BoardId.HasValue);
if (shouldAttemptProposal)
{
if (!session.BoardId.HasValue)
{
// This case is only hit when llmResult.IsActionable is true, due to the outer condition.
assistantContent = $"{llmResult.Content}\n\n(To act on this, open a board-scoped chat session.)";
messageType = "status";
}
else
{
var proposalResult = await _automationPlanner.ParseInstructionAsync(
dto.Content,
userId,
session.BoardId,
ct,
sourceType: ProposalSourceType.Chat,
sourceReferenceId: session.Id.ToString());
if (proposalResult.IsSuccess)
{
messageType = "proposal-reference";
proposalId = proposalResult.Value.Id;
assistantContent = $"{llmResult.Content}\n\nProposal created for review: {proposalResult.Value.Id}";
}
else if (llmResult.IsActionable) // Only give a hint if we were confident it was actionable
{
assistantContent = $"{llmResult.Content}\n\n(I detected a task request but could not parse it into a proposal: {proposalResult.ErrorMessage})";
messageType = "status";
}
// If it was just a user-forced proposal that failed, we silently fall through and return the original LLM content.
}
}| DateTimeOffset.UtcNow, | ||
| DateTimeOffset.UtcNow, | ||
| DateTime.UtcNow.AddHours(1), |
There was a problem hiding this comment.
Using DateTimeOffset.UtcNow and DateTime.UtcNow directly in tests can lead to non-deterministic behavior, especially if tests are run near midnight or across time zone changes. While these specific values aren't asserted on here, it's a best practice to use a fixed date/time or a time provider abstraction (like TimeProvider in .NET 8) to ensure tests are fully deterministic and repeatable.
Adversarial Code Review — PR #567Reviewer focus: GP-06 (Review-First Automation Safety), security, robustness, error handling, test coverage, layer compliance. All 1,489 backend tests pass. GP-06 Compliance: PASSThe core change correctly maintains the review-first invariant. Chat now auto-creates proposals (not direct board mutations) when actionable intent is detected. Proposals still require explicit user approval before any board state changes. This is compliant with GP-06. FindingsMedium-1:
|
| Severity | Count | Action Needed |
|---|---|---|
| Critical | 0 | - |
| High | 0 | - |
| Medium | 2 | Recommended fixes |
| Low | 3 | Nice to have |
| Info | 1 | Future consideration |
Overall assessment: The PR is structurally sound and maintains GP-06 compliance. The two Medium findings (false positive expansion in classifier, silent failure on explicit proposal request) are the most impactful. Neither is a blocker, but Medium-2 (silent failure swallowing) should ideally be addressed before merge since it violates the repo's "handle error cases explicitly; do not swallow failures" Definition of Done.
Tests are thorough and all pass. Layer boundaries are respected — ChatService correctly uses Application-layer abstractions (IAutomationPlannerService, IAutomationProposalService) without touching Infrastructure.
- Surface planner error when RequestProposal is explicit but planner fails (Medium-2: do not swallow explicit user intent failures) - Narrow 'sort'/'reorder' classifier matches to require board/card/column context to reduce false positives now that proposals auto-create - Add 'make card'/'make task' (without article) to classifier - Add test for RequestProposal + !IsActionable + planner failure path
Final Assessment — Post-FixI pushed commit Fixes applied:
Verification: All 1,490 backend tests pass (0 failures, 0 skipped). Overall: The PR is ready to merge. GP-06 (review-first) is fully maintained — chat creates proposals, never direct board mutations. Layer boundaries are clean. Test coverage is comprehensive. |
Consolidates the auto-detected and explicit RequestProposal code paths into a single shouldAttemptProposal branch, eliminating duplicated ParseInstructionAsync call and response handling. Addresses Gemini review.
1 participant
Summary
ChatService.SendMessageAsynconly created proposals whendto.RequestProposalwas explicitlytrue, which defaults tofalse. Users had to manually check a hidden checkbox to get proposals — otherwise actionable messages like "create card" returned prose withmessageType: "status"and no proposal.IsActionable = true), the chat service now automatically routes throughAutomationPlannerService.ParseInstructionAsyncto create a review-first proposal (GP-06 compliant). TheRequestProposalflag is repurposed as a force-try for non-actionable messages.LlmIntentClassifierto recognize natural language like "create a task", "new card", "make a task", "remove card", "rename board" — not just exact command patterns.Closes #512
Test plan
proposal-referenceinstead ofstatus)