Add experimental AskAnotherUser tool with disclosure and cancel controls - #945
Add experimental AskAnotherUser tool with disclosure and cancel controls#945crspeller wants to merge 20 commits into
Conversation
…swer endpoint Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
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:
📝 WalkthroughWalkthroughThe change adds the deferred ChangesAsk Another User deferred workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ToolRunner
participant Conversations
participant MattermostAPI
participant AskUserPost
participant TargetUser
ToolRunner->>Conversations: Dispatch deferred AskAnotherUser call
Conversations->>MattermostAPI: Create pending question card
MattermostAPI->>AskUserPost: Deliver custom_llm_ask_user post
AskUserPost->>TargetUser: Display question and answer controls
TargetUser->>AskUserPost: Submit answer or decline
AskUserPost->>MattermostAPI: POST ask_user_response
MattermostAPI->>Conversations: Validate and persist response
Conversations->>ToolRunner: Resume when unresolved tool calls are resolved
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
conversations/ask_another_user.go (1)
89-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the span-derived context for the rest of the function.
telemetry.Tracer().Startreturns a context that carries the new span. The code discards it with_. Downstream calls in this function do not take a context today, so no trace is lost yet. A later ctx-aware call would attach to the parent span instead of this one.♻️ Proposed change
- _, span := telemetry.Tracer().Start(ctx, "dispatch ask another user", + ctx, span := telemetry.Tracer().Start(ctx, "dispatch ask another user",
ctxis then used by any context-aware call added later. If no call uses it now, keep_ = ctxout and add the assignment together with the first consumer.As per coding guidelines: "Thread
ctx context.Contextas the first parameter through every entry point in the LLM call path, avoidcontext.Background()in production code, and add OpenTelemetry spans with the repo's telemetry helpers and attribute keys."🤖 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 `@conversations/ask_another_user.go` around lines 89 - 95, Update dispatchAskAnotherUser to retain the context returned by telemetry.Tracer().Start instead of discarding it, and use that span-derived context for subsequent function operations and any context-aware calls. Preserve the existing span attributes and avoid introducing a separate context or context.Background().Source: Coding guidelines
webapp/src/index.tsx (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the exported
AskUserPostTypeconstant.
webapp/src/components/ask_user_post/ask_user_post.tsxline 23 exportsAskUserPostType = 'custom_llm_ask_user'and documents it as the server contract. Importing the constant here removes the duplicated literal and prevents drift.♻️ Proposed change
-import {AskUserPost} from './components/ask_user_post/ask_user_post'; +import {AskUserPost, AskUserPostType} from './components/ask_user_post/ask_user_post';- registry.registerPostTypeComponent('custom_llm_ask_user', AskUserPost); + registry.registerPostTypeComponent(AskUserPostType, AskUserPost);Also applies to: 235-235
🤖 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 `@webapp/src/index.tsx` at line 34, Update the usage in webapp/src/index.tsx associated with AskUserPost to import and use the exported AskUserPostType constant from ask_user_post.tsx instead of duplicating the 'custom_llm_ask_user' literal, preserving the existing server-contract behavior.webapp/src/components/tool_card.test.tsx (1)
188-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the unknown-decliner render branch.
webapp/src/components/tool_card.tsxlines 682-687 renderDeclined to answerwhendeclinedByis''. The parse tests cover the''return value, but no test renders that branch. Add a rejectedAskAnotherUsertool whose result is{"status":"declined"}with no arguments.🤖 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 `@webapp/src/components/tool_card.test.tsx` around lines 188 - 205, Add a test to the “ToolCard declined rendering” suite that renders a rejected AskAnotherUser tool with result {"status":"declined"} and no target username, then assert the UI displays “Declined to answer”. Keep the existing named-decliner and ordinary rejected-tool cases unchanged.webapp/src/components/question_card.tsx (1)
218-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the interactivity guard and the submit rule into the shared hook.
handleToggleOption,handleToggleFreeForm, and thecanSubmitexpression are identical toguardedToggleOption,guardedToggleFreeForm, and the options branch ofcanSubmitinwebapp/src/components/ask_user_post/ask_user_post.tsx(lines 384-414). IfuseOptionSelectionaccepted aninteractiveflag and exposedcanSubmit, both callers would drop this duplicated logic and stay in sync.🤖 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 `@webapp/src/components/question_card.tsx` around lines 218 - 234, Move the shared interactivity guards and submission eligibility into useOptionSelection by accepting an interactive flag and exposing canSubmit. Update question_card.tsx and ask_user_post.tsx to use the hook’s guardedToggleOption, guardedToggleFreeForm, and canSubmit, removing their duplicated handlers and local canSubmit logic while preserving the existing free-form and predefined-option rules.webapp/src/components/question_options.tsx (1)
318-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFree-form answer fields have no accessible name. Each free-form field uses a placeholder as its only name. Screen readers announce no field name, and the placeholder disappears once the user types. Add an
aria-labelwith the same localized text at each site.
webapp/src/components/question_options.tsx#L318-L333: passaria-label={freeFormPlaceholder}to bothFreeFormTextareaandFreeFormInput.webapp/src/components/ask_user_post/ask_user_post.tsx#L484-L495: passaria-labelwith theai.ask_user.free_form_placeholdermessage to the standaloneFreeFormTextarea.🤖 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 `@webapp/src/components/question_options.tsx` around lines 318 - 333, Give both free-form controls in webapp/src/components/question_options.tsx lines 318-333, FreeFormTextarea and FreeFormInput, an aria-label using freeFormPlaceholder. Also add an aria-label with the localized ai.ask_user.free_form_placeholder text to the standalone FreeFormTextarea in webapp/src/components/ask_user_post/ask_user_post.tsx lines 484-495.mcp/vetted_tools.go (1)
79-87: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid duplicating the built-in tool name.
mcp/vetted_tools.gohardcodes"AskAnotherUser"while the registered tool exposesmmtools.AskAnotherUserToolName. If either value changes, the policy seed will no longer match the tool. Use a shared dependency-neutral constant, or reuse the existing constant when the import graph permits. Add a test that checks the seed name against the registered tool name.🤖 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 `@mcp/vetted_tools.go` around lines 79 - 87, Update SeedBuiltInToolConfigs to source the AskAnotherUser policy name from a shared dependency-neutral constant, or reuse mmtools.AskAnotherUserToolName if the import graph allows, instead of hardcoding the string. Add a test that verifies the seeded name matches the registered AskAnotherUser tool name.conversations/tool_approval.go (1)
402-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
persistBlocksfor the remaining inline marshal-and-update.The new helper duplicates the marshal-then-
UpdateTurnContentsequence that still appears inline later inHandleToolCall. Routing that call throughpersistBlockskeeps one error-wrapping path for turn writes.🤖 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 `@conversations/tool_approval.go` around lines 402 - 409, Update the inline marshal-and-UpdateTurnContent sequence in HandleToolCall to call persistBlocks instead, passing the same turn ID and blocks. Remove the duplicated JSON marshaling and preserve persistBlocks’s existing error-wrapping behavior.toolrunner/toolrunner_deferred_test.go (1)
46-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a table for the deferred dispatch cases.
TestRunLoopDeferredDispatchcontains six cases as separatet.Runclosures. The repository guideline requires table-driven tests when a test contains more than one case. The cases differ in the scripted LLM responses, the tool store, and the approval function, so a table would need those as fields.conversations/ask_another_user_test.goin this same change uses that shape already.As per coding guidelines: "Go tests must be table-driven when they contain more than one case."
🤖 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 `@toolrunner/toolrunner_deferred_test.go` around lines 46 - 246, Refactor TestRunLoopDeferredDispatch into a table-driven test with one case entry per deferred-dispatch scenario, including fields for scripted responses, tool configuration, approval function, dispatcher error, and case-specific assertions. Iterate with t.Run using the existing testLLM, dispatchRecorder, and collectToolCallEvents helpers, preserving each case’s expected statuses, call counts, tool turns, and resolver behavior.Source: Coding guidelines
mcp/tool_policy_lookup_test.go (1)
226-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert these five cases to a table.
All five subtests share one shape: a
Config, a tool name, an expected policy, and an expected enabled flag. A table with those four fields covers them without losing the explanatory comment on theWebSearchcase.As per coding guidelines: "Go tests must be table-driven when they contain more than one case."
♻️ Proposed table shape
func TestLookupToolPolicyBuiltIn(t *testing.T) { const builtInToolName = "AskAnotherUser" tests := []struct { name string cfg Config toolName string wantPolicy string wantEnabled bool }{ { name: "unconfigured AskAnotherUser gets the seed", toolName: builtInToolName, wantPolicy: ToolPolicyAsk, wantEnabled: true, }, { name: "admin auto_run_in_dm override wins over the seed", cfg: Config{BuiltInTools: []ToolConfig{{ Name: builtInToolName, Policy: ToolPolicyAutoRunInDM, Enabled: true, }}}, toolName: builtInToolName, wantPolicy: ToolPolicyAutoRunInDM, wantEnabled: true, }, { name: "admin disable wins over the seed", cfg: Config{BuiltInTools: []ToolConfig{{ Name: builtInToolName, Policy: ToolPolicyAsk, Enabled: false, }}}, toolName: builtInToolName, wantPolicy: ToolPolicyAsk, }, { // ask+enabled still cannot auto-run: enabled only gates auto-run. name: "unconfigured WebSearch stays at ask", toolName: "WebSearch", wantPolicy: ToolPolicyAsk, wantEnabled: true, }, { name: "empty tool name stays closed", wantPolicy: ToolPolicyAsk, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { policy, enabled := LookupToolPolicy(tt.cfg, "", tt.toolName) require.Equal(t, tt.wantPolicy, policy) require.Equal(t, tt.wantEnabled, enabled) require.False(t, IsToolPolicyAutoRunInDM(policy) && !tt.wantEnabled) }) } }🤖 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 `@mcp/tool_policy_lookup_test.go` around lines 226 - 284, Convert TestLookupToolPolicyBuiltIn into a table-driven test with fields for the case name, Config, tool name, expected policy, and expected enabled state. Iterate through the cases with t.Run, preserving the WebSearch explanatory comment and its auto-run assertion while retaining each existing expected result.Source: Coding guidelines
🤖 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 `@conversations/ask_another_user.go`:
- Around line 258-266: The waiting-state transition in the flow around
findToolUseBlock must be atomic: replace the unconditional
persistBlocks/UpdateTurnContent operation with a conditional store update that
modifies the block only when its status is still conversation.StatusWaiting.
Check the affected-row count and return ErrAskNotPending when another request
has already transitioned it, before creating the tool_result turn or starting
the follow-up stream.
- Around line 398-404: The asynchronous follow-up path in HandleAskUserResponse
must detach the request context before invoking streamToolFollowUp. Apply
telemetry.DetachContext at this API boundary and pass the detached context
through the existing follow-up flow so streamContinuationToExistingPost is not
cancelled when the handler returns.
- Around line 129-131: Update AskAnotherUser around dispatchAskAnotherUser to
check source-channel permissions before sending the DM card: when conv.ChannelID
identifies a non-open channel, require the target user to have
model.PermissionReadChannel and return an access error if not. Preserve the
existing usage-restriction check and behavior for open channels.
In `@conversations/tool_approval.go`:
- Around line 184-210: Make approval handling in findPendingToolTurn and
UpdateTurnContent atomic so only one request can claim a pending block per
conversation before dispatchAskAnotherUser, using the existing locking or
transactional mechanism where available. Ensure persistBlocks and
CreateTurnAutoSequence commit the terminal block status and matching tool_result
together, or add recovery that reconciles any partial failure so no unresolved
persisted state remains.
In `@mmtools/ask_another_user.go`:
- Around line 90-92: Update the too-many-options error message in the options
validation near args.Options to match the enforced range: zero through five
options, while leaving the existing len(args.Options) > 5 check unchanged.
Ensure the tool error no longer claims a minimum of one option or conflicts with
the accepted behavior.
In `@toolrunner/toolrunner.go`:
- Around line 349-396: The mixed deferred/non-deferred batch path currently
discards failedResults, leaving failed deferred tool calls without persisted
tool_result entries. Update the handling around deliverToolTurns and the
resume/finalization flow to retain and persist each failed deferred call’s
ToolResult alongside the batch, while preserving pending non-deferred calls and
waiting deferred calls for resume; ensure StatusError calls are not skipped when
constructing the follow-up request.
In `@webapp/src/components/ask_user_post/ask_user_post.tsx`:
- Around line 135-137: Update buildAnswerPreview to join selected labels and
trimmed free-form text with " — " instead of ", ", while preserving empty-value
filtering. Replace the UTF-16 slice(0, 200) truncation with Unicode
code-point-aware truncation so the preview is limited to 200 runes, matching the
server rule.
In `@webapp/src/components/question_options.tsx`:
- Around line 278-284: Set the native disabled attribute on each button rendered
by OptionRow and FreeFormToggle when interactive is false, including the option
row, FreeFormToggle, and free-form OptionRow instances. Keep the existing
$disabled styling prop and click handlers unchanged.
- Around line 275-298: Update the options rendering in the options map to use an
index-qualified React key instead of key={opt.label}, and make selection state
row-specific so duplicate labels do not cause both rows to appear selected.
Apply the corresponding toggle/state handling consistently with the existing
onToggleOption flow; alternatively, if duplicate labels are intended to be
invalid, add duplicate-label validation to both parseQuestionArgs and
parseAskUserProps.
---
Nitpick comments:
In `@conversations/ask_another_user.go`:
- Around line 89-95: Update dispatchAskAnotherUser to retain the context
returned by telemetry.Tracer().Start instead of discarding it, and use that
span-derived context for subsequent function operations and any context-aware
calls. Preserve the existing span attributes and avoid introducing a separate
context or context.Background().
In `@conversations/tool_approval.go`:
- Around line 402-409: Update the inline marshal-and-UpdateTurnContent sequence
in HandleToolCall to call persistBlocks instead, passing the same turn ID and
blocks. Remove the duplicated JSON marshaling and preserve persistBlocks’s
existing error-wrapping behavior.
In `@mcp/tool_policy_lookup_test.go`:
- Around line 226-284: Convert TestLookupToolPolicyBuiltIn into a table-driven
test with fields for the case name, Config, tool name, expected policy, and
expected enabled state. Iterate through the cases with t.Run, preserving the
WebSearch explanatory comment and its auto-run assertion while retaining each
existing expected result.
In `@mcp/vetted_tools.go`:
- Around line 79-87: Update SeedBuiltInToolConfigs to source the AskAnotherUser
policy name from a shared dependency-neutral constant, or reuse
mmtools.AskAnotherUserToolName if the import graph allows, instead of hardcoding
the string. Add a test that verifies the seeded name matches the registered
AskAnotherUser tool name.
In `@toolrunner/toolrunner_deferred_test.go`:
- Around line 46-246: Refactor TestRunLoopDeferredDispatch into a table-driven
test with one case entry per deferred-dispatch scenario, including fields for
scripted responses, tool configuration, approval function, dispatcher error, and
case-specific assertions. Iterate with t.Run using the existing testLLM,
dispatchRecorder, and collectToolCallEvents helpers, preserving each case’s
expected statuses, call counts, tool turns, and resolver behavior.
In `@webapp/src/components/question_card.tsx`:
- Around line 218-234: Move the shared interactivity guards and submission
eligibility into useOptionSelection by accepting an interactive flag and
exposing canSubmit. Update question_card.tsx and ask_user_post.tsx to use the
hook’s guardedToggleOption, guardedToggleFreeForm, and canSubmit, removing their
duplicated handlers and local canSubmit logic while preserving the existing
free-form and predefined-option rules.
In `@webapp/src/components/question_options.tsx`:
- Around line 318-333: Give both free-form controls in
webapp/src/components/question_options.tsx lines 318-333, FreeFormTextarea and
FreeFormInput, an aria-label using freeFormPlaceholder. Also add an aria-label
with the localized ai.ask_user.free_form_placeholder text to the standalone
FreeFormTextarea in webapp/src/components/ask_user_post/ask_user_post.tsx lines
484-495.
In `@webapp/src/components/tool_card.test.tsx`:
- Around line 188-205: Add a test to the “ToolCard declined rendering” suite
that renders a rejected AskAnotherUser tool with result {"status":"declined"}
and no target username, then assert the UI displays “Declined to answer”. Keep
the existing named-decliner and ordinary rejected-tool cases unchanged.
In `@webapp/src/index.tsx`:
- Line 34: Update the usage in webapp/src/index.tsx associated with AskUserPost
to import and use the exported AskUserPostType constant from ask_user_post.tsx
instead of duplicating the 'custom_llm_ask_user' literal, preserving the
existing server-contract behavior.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dd7512ce-958b-4db8-bf69-2a633b5099fe
📒 Files selected for processing (47)
api/api.goapi/api_post.goapi/api_post_test.goconfig/mcp_config.goconversation/approval_state.goconversation/approval_state_test.goconversation/content_block.goconversation/convert.goconversation/convert_test.goconversation/service_test.goconversations/ask_another_user.goconversations/ask_another_user_test.goconversations/bot_channel_tool_filter.goconversations/conversations.goconversations/handle_messages.goconversations/regeneration.goconversations/tool_approval.goconversations/tool_policy_test.goi18n/en.jsonllm/tool_retry.gollm/tool_retry_test.gollm/tools.gomcp/tool_policy.gomcp/tool_policy_lookup_test.gomcp/vetted_tools.gomcp/vetted_tools_test.gommtools/ask_another_user.gommtools/ask_another_user_test.gommtools/provider.gostreaming/streaming.gostreaming/turn_persistence_test.gotoolrunner/toolrunner.gotoolrunner/toolrunner_deferred_test.gowebapp/src/client.test.tswebapp/src/client.tsxwebapp/src/components/ask_user_post/ask_user_post.test.tsxwebapp/src/components/ask_user_post/ask_user_post.tsxwebapp/src/components/llmbot_post/turn_content_utils.test.tswebapp/src/components/llmbot_post/turn_content_utils.tswebapp/src/components/question_card.tsxwebapp/src/components/question_options.tsxwebapp/src/components/tool_card.test.tsxwebapp/src/components/tool_card.tsxwebapp/src/components/tool_types.tswebapp/src/i18n/en.jsonwebapp/src/index.tsxwebapp/src/types/conversation.ts
… param, preview parity Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…arg lengths Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
conversations/ask_another_user_test.go (1)
218-232: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover requester lookup failures in the attribution test.
The dispatcher leaves the plaintext fallback without attribution when
GetUser(conv.UserID)fails. This table covers a human requester and an autonomous bot requester, but not a lookup failure. Add a case that verifies the intended behaviour, or make dispatch fail before sending when attribution is required.🤖 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 `@conversations/ask_another_user_test.go` around lines 218 - 232, Extend the table-driven attribution test around the existing wantRequesterProp assertions to cover a GetUser(conv.UserID) lookup failure, verifying the intended no-attribution plaintext fallback behavior (or the required pre-send dispatch failure). Configure the case using the test’s existing user-lookup/mock symbols and assert the corresponding sent message or error outcome.
🤖 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 `@mmtools/ask_another_user.go`:
- Around line 90-96: Canonicalize args.Username in ValidateAskAnotherUserArgs by
trimming surrounding whitespace and removing the optional leading @, then reject
the canonical value when empty. Reuse that same canonical username in
conversations.dispatchAskAnotherUser when calling GetUserByUsername, rather than
passing the original input.
---
Nitpick comments:
In `@conversations/ask_another_user_test.go`:
- Around line 218-232: Extend the table-driven attribution test around the
existing wantRequesterProp assertions to cover a GetUser(conv.UserID) lookup
failure, verifying the intended no-attribution plaintext fallback behavior (or
the required pre-send dispatch failure). Configure the case using the test’s
existing user-lookup/mock symbols and assert the corresponding sent message or
error outcome.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 005c3897-69f4-4117-b473-8ef5568bdfea
📒 Files selected for processing (5)
conversations/ask_another_user.goconversations/ask_another_user_test.goi18n/en.jsonmmtools/ask_another_user.gommtools/ask_another_user_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- mmtools/ask_another_user_test.go
- conversations/ask_another_user.go
![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: 67aefc1525
ℹ️ 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".
… username Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ype constant) Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: christopher <christopher@mattermost.com>
…nflicts Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ops, sanitize, cancel endpoint Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…t separation), experimental toggle, cancel control Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…laim, cancel button hydration, identity-line alignment Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…er-tool-5d96 Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
cursor
Bot
changed the title
2 participants
Summary
Adds an experimental, default-off
AskAnotherUsercapability that lets an Agent ask another Mattermost user a clarifying question, wait for the response, and continue the originating task.Admins enable it in System Console → Agents → AI Functions → Enable Agents to Ask Other Users. The setting explains the implications: bot-authored DMs to users who did not initiate them, answers flowing back to the originating DM/channel, and the additional prompt-injection/social-engineering surface.
Key behavior:
ask/auto_run_in_dm/auto_run_everywherepolicy; default behavior requires initiator approval.Your answer will be shared with @requester.Your answer may be shared with the N members of ~channel.Asked by the <agent> agent running unattended (no human requester).answer_received:false,canceled_by:"requester"); the target card becomesThis question is no longer needed.{"status":"canceled"}): no second tool result or continuation. Duplicate normal answers remain 409.LLM_TurnsJSON + post props; no schema migration.Security/reliability:
Testing:
make checkpasses with Go 1.26.5: 4,499 Go tests (85 skipped), 500 Jest tests, lint/type-check/shard/i18n/lock/go-mod checks clean.QA steps:
AskAnotherUser.Ticket Link
NONE
Screenshots
Experimental toggle and implications:
Experimental AskAnotherUser system-console toggle and implications
Target card with attribution, AI-generated-content boundary, DM disclosure, and requester access context:
Target card showing Alice Anderson and Incident Commander access context
Channel destination disclosure:
Question card disclosing three Town Square recipients
Anti-impersonation rendering (attempted forged system text is absent from the AI region):
AI-generated question visually separated from system attribution
Cancel walkthrough (approval → waiting → cancel → explicit no-answer continuation):
ask_user_v2_cancel_walkthrough_final_20260819.mp4
Release Note
To show artifacts inline, enable in settings.
Summary by CodeRabbit