feat: human-in-the-loop support with AskUserQuestion#897
Merged
Gkrumbach07 merged 1 commit intomainfrom Mar 13, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (24)
WalkthroughThis PR introduces agent status derivation from event logs and an interactive AskUserQuestion tool flow. It adds backend support to extract agent status from persisted AGUI events, enriches session responses with derived status, creates frontend UI components for status display, implements AskUserQuestion message handling with multi-question and freeform input support, and adds halt-tracking in the runner for frontend tool interactions. Changes
Sequence DiagramssequenceDiagram
participant Client as Frontend (Session Page)
participant Handler as Backend Handler
participant EventLog as AGUI Event Log
participant WebSocket as WebSocket Layer
Client->>Handler: GetSession(sessionName)
Handler->>EventLog: DeriveAgentStatusFromEvents(sessionName)
EventLog->>EventLog: Scan event log backwards
EventLog-->>Handler: Return derived status (working/idle/waiting_input)
Handler->>Handler: enrichAgentStatus(session)
Handler-->>Client: AgenticSession with agentStatus field
Client->>Client: useAgentStatus() hook
Client->>Client: Render AgentStatusIndicator
sequenceDiagram
participant User as User
participant UI as Frontend (Chat UI)
participant Stream as AG-UI Stream
participant Agent as Claude Agent
participant Runner as Runner/Bridge
Agent->>Runner: Tool call: AskUserQuestion
Runner->>Stream: Emit TOOL_CALL_START event
Stream-->>UI: Receive tool event
UI->>UI: Detect AskUserQuestion tool
UI->>UI: Render AskUserQuestionMessage
User->>UI: Select answers + submit
UI->>Stream: sendMessage(answer, metadata: {type: ask_user_question_response})
Stream-->>Runner: User answer via message stream
Runner->>Agent: Emit TOOL_CALL_RESULT with answer
Agent->>Agent: Continue execution
Runner->>Stream: Emit result events
Stream-->>UI: Update message state
UI->>UI: Filter and hide tool response message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/frontend/src/hooks/use-agui-stream.ts (1)
226-247:⚠️ Potential issue | 🟠 MajorSkip the optimistic append for hidden HITL answers.
ask_user_question_responseis supposed to advance the UI when the next run emits the matching tool result, but this still inserts a local user message before the POST succeeds. On 4xx/5xx, that leaves a hidden “answered” message instate.messagesthat never existed in the persisted event log, so retries can accumulate phantom answers and any message/status logic reading raw stream state can drift from the backend.Suggested fix
- // Add user message to state immediately for instant UI feedback. - const userMsgWithTimestamp = { - ...userMessage, - timestamp: new Date().toISOString(), - } as PlatformMessage - setState((prev) => ({ - ...prev, - status: 'connected', - error: null, - messages: [...prev.messages, userMsgWithTimestamp], - })) + const isHiddenToolAnswer = metadata?.type === 'ask_user_question_response' + if (!isHiddenToolAnswer) { + const userMsgWithTimestamp = { + ...userMessage, + timestamp: new Date().toISOString(), + } as PlatformMessage + setState((prev) => ({ + ...prev, + status: 'connected', + error: null, + messages: [...prev.messages, userMsgWithTimestamp], + })) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/hooks/use-agui-stream.ts` around lines 226 - 247, The code currently optimistically appends a local user message (userMessage / userMsgWithTimestamp) via setState inside the async run caller; for hidden HITL answers (metadata indicating the ask_user_question_response tool) skip this optimistic append to avoid phantom messages—detect this case by checking metadata (e.g., metadata?.tool === 'ask_user_question_response' or a similarly-named flag used by your backend) and only call setState to add the user message after the POST/SSE confirms success or when the matching tool result arrives; update the async sender logic that builds userMessage and calls setState so it conditionally bypasses adding userMsgWithTimestamp for ask_user_question_response and instead relies on the server-emitted event to insert the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/sessions.go`:
- Around line 44-46: DeriveAgentStatusFromEvents currently accepts only
sessionName which is not namespace-scoped; change its signature and all call
sites to accept a namespaced identifier (either project/namespace + sessionName
or a stable sessionUID) and use that namespaced key when deriving agent status;
update the function declaration DeriveAgentStatusFromEvents and callers in this
file (and the other affected locations referenced around lines 367-383, 456-459,
933-934) to pass the extra project/namespace or sessionUID parameter so lookups
are isolated per project/namespace.
In `@components/backend/websocket/agui_proxy.go`:
- Around line 439-446: The bug is that sessionProjectMap is being looked up by
sessionID alone (sessionProjectMap.Load(sessionID) in the activity branch) but
the map is populated using a project-scoped key (sessionName/projectName), so
entries from different projects can collide; fix by using the same
project-scoped key format everywhere: derive the composite key (the same key
format used in HandleAGUIRunProxy/session registration) when loading
sessionProjectMap and when updating lastActivityTime (keep
updateLastActivityTime(projectName, sessionID, ... ) as-is but ensure you
retrieve the correct projectName by loading sessionProjectMap with the composite
key rather than plain sessionID); update any registration code to use and
document that composite key consistently.
In `@components/backend/websocket/agui_store_test.go`:
- Around line 84-88: The test currently ignores errors from json.Marshal and
f.Write when writing events (the loop using variables events, data, and file
variable f); update the test to check and fail on errors: capture the error
returned by json.Marshal and by f.Write, and call t.Fatalf or t.Fatalf-like
helper if either returns an error, and defer f.Close() while also checking the
error returned by Close to surface disk I/O failures; ensure you reference the
existing variables/events loop and use the test helper (t) for deterministic
failures.
In `@components/backend/websocket/agui_store.go`:
- Around line 223-228: The code opens a file earlier but still calls
os.ReadFile(path) when fileSize <= maxTailBytes, causing a second open; replace
that call by reading from the already-open file handle (the variable used to
open the file earlier, e.g. f) — seek to the beginning if needed and use
io.ReadAll(f) (or equivalent) to populate data, handle errors the same way, and
avoid reopening the file; update the branch that currently calls
os.ReadFile(path) to use the existing handle and remove the redundant open.
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1401-1411: sendToolAnswer currently always calls aguiSendMessage
(hitting /agui/run) even when the session is not in Running state; mirror the
same phase guard used in sendChat by checking the session/phase (e.g.,
isSessionRunning or session.status/phase used by sendChat) before calling
aguiSendMessage in sendToolAnswer, and short-circuit with the same UI behavior
(toast/throw or resume/queue flow) when the session isn't Running; update
references in this function (sendToolAnswer, aguiSendMessage) to reuse the same
guard logic/path that sendChat uses so historical AskUserQuestion cards cannot
submit to stopped/completed sessions.
In `@components/frontend/src/components/session-status-dot.tsx`:
- Around line 37-56: The TooltipProvider wrapper inside the SessionStatusDot
component causes redundant providers when rendering many dots; remove the
TooltipProvider from SessionStatusDot (leaving Tooltip, TooltipTrigger,
TooltipContent intact) and document/ensure that a single TooltipProvider is
mounted at a higher level (e.g., the sessions list/table or app root) so
tooltips still function; update any usage or tests of SessionStatusDot to wrap
the parent with TooltipProvider if not already provided.
In `@components/frontend/src/components/session/ask-user-question.tsx`:
- Around line 112-131: The handleSubmit function currently only uses finally to
reset isSubmitting and doesn't report failures; update handleSubmit to catch
errors from onSubmitAnswer, set an error state (e.g., add local state like
submissionError) or call an existing error callback, and provide user feedback
before resetting isSubmitting and without blocking successful setSubmitted.
Specifically, wrap await onSubmitAnswer(response) in a try/catch, call
setSubmissionError or a provided onError handler with the caught error,
optionally log it, and only call setSubmitted(true) on success while ensuring
setIsSubmitting(false) still runs in finally; refer to handleSubmit,
onSubmitAnswer, setIsSubmitting and setSubmitted to locate and modify the logic.
- Around line 211-247: Replace the non-interactive <label> used for the "Other"
option with a proper interactive element: either a <button type="button"> or the
project's RadioGroupItem component so assistive tech gets correct semantics;
move the existing handlers to the new element (use onClick={ () => !disabled &&
handleOtherToggle(q.question) } and remove manual onKeyDown/tabIndex/role since
a button provides keyboard support), keep disabled state via the disabled prop
and aria-pressed or aria-checked (e.g., aria-pressed={isOther}) for
accessibility, ensure the inner Input still stops propagation on click and
continues to use value={otherText[q.question] || ""} and onChange={(e) =>
handleOtherTextChange(q.question, e.target.value)}, and preserve the visual
classes (the div showing the dot and container classes) so styling remains
unchanged.
In `@components/frontend/src/components/ui/tool-message.tsx`:
- Around line 311-319: The current AskUserQuestion branch reads
input.questions[0].question directly and can print "undefined" for malformed
entries; update the check in the block that handles toolName ===
"askuserquestion" (referencing toolName and input.questions) to validate that
questions is an array, questions.length > 0, and that questions[0].question is a
non-empty string (e.g., using questions[0]?.question && typeof
questions[0].question === "string"); if the first entry is missing or invalid,
fall back to the generic "Asking a question" (or a safe placeholder) instead of
interpolating undefined.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 126-140: The code captures adapter halt state into
self._halted_by_thread[thread_id] then immediately reads and pops it; replace
that unnecessary dict round-trip with a local variable: assign halted =
self._adapter.halted (using thread_id only for logging), check halted and call
await worker.interrupt() if true, and remove the set/pop operations on
self._halted_by_thread; if persistent per-thread state is actually required
elsewhere, do not pop here and expose the state via an accessor instead.
In `@components/runners/ambient-runner/ambient_runner/platform/prompts.py`:
- Around line 88-95: The helper resolve_workspace_prompt currently
unconditionally injects HUMAN_INPUT_INSTRUCTIONS which requires the
AskUserQuestion tool; add a boolean parameter supports_human_input (default
False) to resolve_workspace_prompt and use it to conditionally append
HUMAN_INPUT_INSTRUCTIONS only when True, update the docstring to remove the ADK
bridge claim, and ensure callers (e.g., the Claude bridge) pass
supports_human_input=True while other bridges omit or pass False so models are
not instructed to use an unsupported AskUserQuestion tool.
---
Outside diff comments:
In `@components/frontend/src/hooks/use-agui-stream.ts`:
- Around line 226-247: The code currently optimistically appends a local user
message (userMessage / userMsgWithTimestamp) via setState inside the async run
caller; for hidden HITL answers (metadata indicating the
ask_user_question_response tool) skip this optimistic append to avoid phantom
messages—detect this case by checking metadata (e.g., metadata?.tool ===
'ask_user_question_response' or a similarly-named flag used by your backend) and
only call setState to add the user message after the POST/SSE confirms success
or when the matching tool result arrives; update the async sender logic that
builds userMessage and calls setState so it conditionally bypasses adding
userMsgWithTimestamp for ask_user_question_response and instead relies on the
server-emitted event to insert the message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14ad42df-a6f8-433a-b999-72ef1d072b47
📒 Files selected for processing (24)
components/backend/handlers/sessions.gocomponents/backend/main.gocomponents/backend/types/agui.gocomponents/backend/types/session.gocomponents/backend/websocket/agui_proxy.gocomponents/backend/websocket/agui_store.gocomponents/backend/websocket/agui_store_test.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/components/agent-status-indicator.tsxcomponents/frontend/src/components/session-status-dot.tsxcomponents/frontend/src/components/session/MessagesTab.tsxcomponents/frontend/src/components/session/ask-user-question.tsxcomponents/frontend/src/components/ui/stream-message.tsxcomponents/frontend/src/components/ui/tool-message.tsxcomponents/frontend/src/components/workspace-sections/sessions-section.tsxcomponents/frontend/src/hooks/agui/types.tscomponents/frontend/src/hooks/use-agent-status.tscomponents/frontend/src/hooks/use-agui-stream.tscomponents/frontend/src/services/queries/use-sessions.tscomponents/frontend/src/types/agentic-session.tscomponents/frontend/src/types/api/sessions.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/platform/prompts.py
Comment on lines
+44
to
+46
| // DeriveAgentStatusFromEvents derives agentStatus from the persisted event log. | ||
| // Set by the websocket package at init to avoid circular imports. | ||
| DeriveAgentStatusFromEvents func(sessionName string) string |
There was a problem hiding this comment.
Use a namespaced key for derived agent status.
DeriveAgentStatusFromEvents only receives sessionName, but session names are namespace-scoped and this file generates them from session-<unix seconds>. Two projects can legitimately share the same name, so list/detail can pick up another project's waiting/working state. Please thread project/namespace or a stable session UID through this lookup.
Suggested fix
- DeriveAgentStatusFromEvents func(sessionName string) string
+ DeriveAgentStatusFromEvents func(project, sessionName string) string
...
-func enrichAgentStatus(session *types.AgenticSession) {
+func enrichAgentStatus(project string, session *types.AgenticSession) {
if session.Status == nil || session.Status.Phase != "Running" {
return
}
if DeriveAgentStatusFromEvents == nil {
return
}
name, _ := session.Metadata["name"].(string)
if name == "" {
return
}
- if derived := DeriveAgentStatusFromEvents(name); derived != "" {
+ if derived := DeriveAgentStatusFromEvents(project, name); derived != "" {
session.Status.AgentStatus = types.StringPtr(derived)
}
}
...
- enrichAgentStatus(&paginatedSessions[i])
+ enrichAgentStatus(project, &paginatedSessions[i])
...
- enrichAgentStatus(&session)
+ enrichAgentStatus(project, &session)Also applies to: 367-383, 456-459, 933-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/sessions.go` around lines 44 - 46,
DeriveAgentStatusFromEvents currently accepts only sessionName which is not
namespace-scoped; change its signature and all call sites to accept a namespaced
identifier (either project/namespace + sessionName or a stable sessionUID) and
use that namespaced key when deriving agent status; update the function
declaration DeriveAgentStatusFromEvents and callers in this file (and the other
affected locations referenced around lines 367-383, 456-459, 933-934) to pass
the extra project/namespace or sessionUID parameter so lookups are isolated per
project/namespace.
Comment on lines
+439
to
446
| // Extract event type; projectName is derived from the | ||
| // sessionID-to-project mapping populated by HandleAGUIRunProxy. | ||
| eventType, _ := event["type"].(string) | ||
|
|
||
| // Update lastActivityTime on CR for activity events (debounced). | ||
| if isActivityEvent(eventType) { | ||
| if projectName, ok := sessionProjectMap.Load(sessionID); ok { | ||
| updateLastActivityTime(projectName.(string), sessionID, eventType == types.EventTypeRunStarted) |
There was a problem hiding this comment.
Use a project-scoped key here, not sessionID alone.
Line 445 recovers projectName from sessionProjectMap using only sessionID, but that map is populated by sessionName. If two projects have the same session name, the later run overwrites the earlier entry and activity from one session can update lastActivityTime on the wrong CR. That will make the wrong session appear active while the real one goes stale.
Suggested direction
- go proxyRunnerStream(runnerURL, bodyBytes, sessionName, runID, threadID)
+ go proxyRunnerStream(runnerURL, bodyBytes, projectName, sessionName, runID, threadID)
- func proxyRunnerStream(runnerURL string, bodyBytes []byte, sessionName, runID, threadID string) {
+ func proxyRunnerStream(runnerURL string, bodyBytes []byte, projectName, sessionName, runID, threadID string) {
- persistStreamedEvent(sessionName, runID, threadID, jsonData)
+ persistStreamedEvent(projectName, sessionName, runID, threadID, jsonData)
- func persistStreamedEvent(sessionID, runID, threadID, jsonData string) {
+ func persistStreamedEvent(projectName, sessionID, runID, threadID, jsonData string) {
- if projectName, ok := sessionProjectMap.Load(sessionID); ok {
- updateLastActivityTime(projectName.(string), sessionID, eventType == types.EventTypeRunStarted)
- }
+ updateLastActivityTime(projectName, sessionID, eventType == types.EventTypeRunStarted)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_proxy.go` around lines 439 - 446, The bug
is that sessionProjectMap is being looked up by sessionID alone
(sessionProjectMap.Load(sessionID) in the activity branch) but the map is
populated using a project-scoped key (sessionName/projectName), so entries from
different projects can collide; fix by using the same project-scoped key format
everywhere: derive the composite key (the same key format used in
HandleAGUIRunProxy/session registration) when loading sessionProjectMap and when
updating lastActivityTime (keep updateLastActivityTime(projectName, sessionID,
... ) as-is but ensure you retrieve the correct projectName by loading
sessionProjectMap with the composite key rather than plain sessionID); update
any registration code to use and document that composite key consistently.
Comment on lines
+84
to
+88
| for _, evt := range events { | ||
| data, _ := json.Marshal(evt) | ||
| f.Write(append(data, '\n')) | ||
| } | ||
| f.Close() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider checking write errors in tests for reliability.
The f.Write() calls ignore errors (e.g., lines 86, 117, 150, 179, 211). While unlikely to fail in practice, checking these would make test failures more diagnosable if disk issues occur.
♻️ Example fix
for _, evt := range events {
data, _ := json.Marshal(evt)
- f.Write(append(data, '\n'))
+ if _, err := f.Write(append(data, '\n')); err != nil {
+ t.Fatalf("Failed to write event: %v", err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, evt := range events { | |
| data, _ := json.Marshal(evt) | |
| f.Write(append(data, '\n')) | |
| } | |
| f.Close() | |
| for _, evt := range events { | |
| data, _ := json.Marshal(evt) | |
| if _, err := f.Write(append(data, '\n')); err != nil { | |
| t.Fatalf("Failed to write event: %v", err) | |
| } | |
| } | |
| f.Close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_store_test.go` around lines 84 - 88, The
test currently ignores errors from json.Marshal and f.Write when writing events
(the loop using variables events, data, and file variable f); update the test to
check and fail on errors: capture the error returned by json.Marshal and by
f.Write, and call t.Fatalf or t.Fatalf-like helper if either returns an error,
and defer f.Close() while also checking the error returned by Close to surface
disk I/O failures; ensure you reference the existing variables/events loop and
use the test helper (t) for deterministic failures.
Comment on lines
+223
to
+228
| if fileSize <= maxTailBytes { | ||
| // File is small, read it all | ||
| data, err = os.ReadFile(path) | ||
| if err != nil { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inefficiency: file opened twice for small files.
The file is already open from line 209, but for small files, os.ReadFile is called which opens the file again. Consider reading from the already-open file handle instead.
♻️ Proposed fix to read from existing handle
if fileSize <= maxTailBytes {
- // File is small, read it all
- data, err = os.ReadFile(path)
- if err != nil {
- return ""
- }
+ // File is small, read it all from the open handle
+ data = make([]byte, fileSize)
+ _, err = file.Read(data)
+ if err != nil {
+ return ""
+ }
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_store.go` around lines 223 - 228, The code
opens a file earlier but still calls os.ReadFile(path) when fileSize <=
maxTailBytes, causing a second open; replace that call by reading from the
already-open file handle (the variable used to open the file earlier, e.g. f) —
seek to the beginning if needed and use io.ReadAll(f) (or equivalent) to
populate data, handle errors the same way, and avoid reopening the file; update
the branch that currently calls os.ReadFile(path) to use the existing handle and
remove the redundant open.
Comment on lines
+1401
to
+1411
| // Send an AskUserQuestion response (hidden from chat, properly formatted) | ||
| const sendToolAnswer = async (formattedAnswer: string) => { | ||
| try { | ||
| await aguiSendMessage(formattedAnswer, { | ||
| type: "ask_user_question_response", | ||
| }); | ||
| } catch (err) { | ||
| toast.error(err instanceof Error ? err.message : "Failed to send answer"); | ||
| throw err; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Mirror the sendChat() phase guard for tool answers.
This path always calls /agui/run, even when the session is no longer Running. Since the page still renders historical AskUserQuestion cards, users can submit into a stopped/completed session instead of getting a resume/queue flow.
Suggested fix
const sendToolAnswer = async (formattedAnswer: string) => {
+ if (session?.status?.phase !== "Running") {
+ toast.error("Resume the session before answering this question");
+ return;
+ }
try {
await aguiSendMessage(formattedAnswer, {
type: "ask_user_question_response",
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx
around lines 1401 - 1411, sendToolAnswer currently always calls aguiSendMessage
(hitting /agui/run) even when the session is not in Running state; mirror the
same phase guard used in sendChat by checking the session/phase (e.g.,
isSessionRunning or session.status/phase used by sendChat) before calling
aguiSendMessage in sendToolAnswer, and short-circuit with the same UI behavior
(toast/throw or resume/queue flow) when the session isn't Running; update
references in this function (sendToolAnswer, aguiSendMessage) to reuse the same
guard logic/path that sendChat uses so historical AskUserQuestion cards cannot
submit to stopped/completed sessions.
Comment on lines
+112
to
+131
| const handleSubmit = async () => { | ||
| if (!onSubmitAnswer || !allQuestionsAnswered || disabled) return; | ||
|
|
||
| // Build the structured response matching Claude SDK format | ||
| const answers: Record<string, string> = {}; | ||
| for (const q of questions) { | ||
| const sel = selections[q.question]; | ||
| answers[q.question] = Array.isArray(sel) ? sel.join(", ") : (sel as string); | ||
| } | ||
|
|
||
| // Send as JSON matching the AskUserQuestion response format | ||
| const response = JSON.stringify({ questions, answers }); | ||
| try { | ||
| setIsSubmitting(true); | ||
| await onSubmitAnswer(response); | ||
| setSubmitted(true); | ||
| } finally { | ||
| setIsSubmitting(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling for submission failures.
The handleSubmit function catches errors implicitly in the finally block but doesn't provide user feedback on failure. If onSubmitAnswer throws, isSubmitting is reset but users won't know the submission failed.
🛡️ Proposed improvement with error feedback
try {
setIsSubmitting(true);
await onSubmitAnswer(response);
setSubmitted(true);
+ } catch (error) {
+ console.error("Failed to submit answer:", error);
+ // Optionally show user feedback via toast or inline error
} finally {
setIsSubmitting(false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleSubmit = async () => { | |
| if (!onSubmitAnswer || !allQuestionsAnswered || disabled) return; | |
| // Build the structured response matching Claude SDK format | |
| const answers: Record<string, string> = {}; | |
| for (const q of questions) { | |
| const sel = selections[q.question]; | |
| answers[q.question] = Array.isArray(sel) ? sel.join(", ") : (sel as string); | |
| } | |
| // Send as JSON matching the AskUserQuestion response format | |
| const response = JSON.stringify({ questions, answers }); | |
| try { | |
| setIsSubmitting(true); | |
| await onSubmitAnswer(response); | |
| setSubmitted(true); | |
| } finally { | |
| setIsSubmitting(false); | |
| } | |
| }; | |
| const handleSubmit = async () => { | |
| if (!onSubmitAnswer || !allQuestionsAnswered || disabled) return; | |
| // Build the structured response matching Claude SDK format | |
| const answers: Record<string, string> = {}; | |
| for (const q of questions) { | |
| const sel = selections[q.question]; | |
| answers[q.question] = Array.isArray(sel) ? sel.join(", ") : (sel as string); | |
| } | |
| // Send as JSON matching the AskUserQuestion response format | |
| const response = JSON.stringify({ questions, answers }); | |
| try { | |
| setIsSubmitting(true); | |
| await onSubmitAnswer(response); | |
| setSubmitted(true); | |
| } catch (error) { | |
| console.error("Failed to submit answer:", error); | |
| // Optionally show user feedback via toast or inline error | |
| } finally { | |
| setIsSubmitting(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/session/ask-user-question.tsx` around
lines 112 - 131, The handleSubmit function currently only uses finally to reset
isSubmitting and doesn't report failures; update handleSubmit to catch errors
from onSubmitAnswer, set an error state (e.g., add local state like
submissionError) or call an existing error callback, and provide user feedback
before resetting isSubmitting and without blocking successful setSubmitted.
Specifically, wrap await onSubmitAnswer(response) in a try/catch, call
setSubmissionError or a provided onError handler with the caught error,
optionally log it, and only call setSubmitted(true) on success while ensuring
setIsSubmitting(false) still runs in finally; refer to handleSubmit,
onSubmitAnswer, setIsSubmitting and setSubmitted to locate and modify the logic.
Comment on lines
+211
to
+247
| <label | ||
| className={cn( | ||
| "flex items-center gap-2.5 p-1.5 rounded cursor-pointer transition-colors", | ||
| isOther ? "bg-accent" : "hover:bg-muted/50", | ||
| disabled && "cursor-default opacity-60" | ||
| )} | ||
| onClick={() => !disabled && handleOtherToggle(q.question)} | ||
| onKeyDown={(e) => { | ||
| if (!disabled && (e.key === "Enter" || e.key === " ")) { | ||
| e.preventDefault(); | ||
| handleOtherToggle(q.question); | ||
| } | ||
| }} | ||
| tabIndex={disabled ? -1 : 0} | ||
| role="button" | ||
| > | ||
| <div className={cn( | ||
| "aspect-square h-4 w-4 rounded-full border border-primary flex items-center justify-center flex-shrink-0", | ||
| disabled && "opacity-50" | ||
| )}> | ||
| {isOther && <div className="h-2.5 w-2.5 rounded-full bg-current" />} | ||
| </div> | ||
| <div className="min-w-0 flex-1"> | ||
| <span className="text-sm">Other</span> | ||
| {isOther && ( | ||
| <Input | ||
| autoFocus | ||
| placeholder="Type your answer..." | ||
| value={otherText[q.question] || ""} | ||
| onChange={(e) => handleOtherTextChange(q.question, e.target.value)} | ||
| onClick={(e) => e.stopPropagation()} | ||
| disabled={disabled} | ||
| className="mt-1 h-7 text-sm" | ||
| /> | ||
| )} | ||
| </div> | ||
| </label> |
There was a problem hiding this comment.
Accessibility: "Other" option should use semantic elements instead of interactive label.
The "Other" option uses a <label> with onClick, onKeyDown, tabIndex, and role="button", which violates accessibility best practices. A <label> is non-interactive by definition, and assigning an interactive role creates confusion for assistive technologies.
Consider refactoring to use a proper <button> element or, for consistency with other options, add an actual RadioGroupItem for the "Other" selection:
♿ Suggested refactor using button element
- <label
+ <button
+ type="button"
className={cn(
- "flex items-center gap-2.5 p-1.5 rounded cursor-pointer transition-colors",
+ "flex items-center gap-2.5 p-1.5 rounded cursor-pointer transition-colors w-full text-left",
isOther ? "bg-accent" : "hover:bg-muted/50",
disabled && "cursor-default opacity-60"
)}
onClick={() => !disabled && handleOtherToggle(q.question)}
- onKeyDown={(e) => {
- if (!disabled && (e.key === "Enter" || e.key === " ")) {
- e.preventDefault();
- handleOtherToggle(q.question);
- }
- }}
- tabIndex={disabled ? -1 : 0}
- role="button"
+ disabled={disabled}
>🧰 Tools
🪛 Biome (2.4.6)
[error] 211-247: A form label must be associated with an input.
(lint/a11y/noLabelWithoutControl)
[error] 225-225: The elements with this role can be changed to the following elements:
(lint/a11y/useSemanticElements)
[error] 225-225: The HTML element label is non-interactive and should not have an interactive role.
(lint/a11y/noNoninteractiveElementToInteractiveRole)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/session/ask-user-question.tsx` around
lines 211 - 247, Replace the non-interactive <label> used for the "Other" option
with a proper interactive element: either a <button type="button"> or the
project's RadioGroupItem component so assistive tech gets correct semantics;
move the existing handlers to the new element (use onClick={ () => !disabled &&
handleOtherToggle(q.question) } and remove manual onKeyDown/tabIndex/role since
a button provides keyboard support), keep disabled state via the disabled prop
and aria-pressed or aria-checked (e.g., aria-pressed={isOther}) for
accessibility, ensure the inner Input still stops propagation on click and
continues to use value={otherText[q.question] || ""} and onChange={(e) =>
handleOtherTextChange(q.question, e.target.value)}, and preserve the visual
classes (the div showing the dot and container classes) so styling remains
unchanged.
Comment on lines
+311
to
+319
| // AskUserQuestion - show first question text | ||
| if (toolName.toLowerCase().replace(/[^a-z]/g, "") === "askuserquestion") { | ||
| const questions = input.questions as Array<{ question: string }> | undefined; | ||
| if (questions?.length) { | ||
| const suffix = questions.length > 1 ? ` (+${questions.length - 1} more)` : ""; | ||
| return `Asking: "${questions[0].question}"${suffix}`; | ||
| } | ||
| return "Asking a question"; | ||
| } |
There was a problem hiding this comment.
Add defensive check for malformed question entries.
The code assumes questions[0].question exists, but if the array contains a malformed entry (e.g., [{}] or [{question: undefined}]), this could display "Asking: "undefined"".
🛡️ Proposed fix
if (toolName.toLowerCase().replace(/[^a-z]/g, "") === "askuserquestion") {
const questions = input.questions as Array<{ question: string }> | undefined;
- if (questions?.length) {
+ if (questions?.length && questions[0]?.question) {
const suffix = questions.length > 1 ? ` (+${questions.length - 1} more)` : "";
return `Asking: "${questions[0].question}"${suffix}`;
}
return "Asking a question";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // AskUserQuestion - show first question text | |
| if (toolName.toLowerCase().replace(/[^a-z]/g, "") === "askuserquestion") { | |
| const questions = input.questions as Array<{ question: string }> | undefined; | |
| if (questions?.length) { | |
| const suffix = questions.length > 1 ? ` (+${questions.length - 1} more)` : ""; | |
| return `Asking: "${questions[0].question}"${suffix}`; | |
| } | |
| return "Asking a question"; | |
| } | |
| // AskUserQuestion - show first question text | |
| if (toolName.toLowerCase().replace(/[^a-z]/g, "") === "askuserquestion") { | |
| const questions = input.questions as Array<{ question: string }> | undefined; | |
| if (questions?.length && questions[0]?.question) { | |
| const suffix = questions.length > 1 ? ` (+${questions.length - 1} more)` : ""; | |
| return `Asking: "${questions[0].question}"${suffix}`; | |
| } | |
| return "Asking a question"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/ui/tool-message.tsx` around lines 311 -
319, The current AskUserQuestion branch reads input.questions[0].question
directly and can print "undefined" for malformed entries; update the check in
the block that handles toolName === "askuserquestion" (referencing toolName and
input.questions) to validate that questions is an array, questions.length > 0,
and that questions[0].question is a non-empty string (e.g., using
questions[0]?.question && typeof questions[0].question === "string"); if the
first entry is missing or invalid, fall back to the generic "Asking a question"
(or a safe placeholder) instead of interpolating undefined.
Comment on lines
+126
to
+140
| # Capture halt state for this thread to avoid race conditions | ||
| # with concurrent runs modifying the shared adapter's halted flag | ||
| self._halted_by_thread[thread_id] = self._adapter.halted | ||
|
|
||
| # If the adapter halted (frontend tool or built-in HITL tool like | ||
| # AskUserQuestion), interrupt the worker to prevent the SDK from | ||
| # auto-approving the tool call with a placeholder result. | ||
| if self._halted_by_thread.get(thread_id, False): | ||
| logger.info( | ||
| f"Adapter halted for thread={thread_id}, " | ||
| "interrupting worker to await user input" | ||
| ) | ||
| await worker.interrupt() | ||
| # Clear the halt flag for this thread | ||
| self._halted_by_thread.pop(thread_id, None) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify: use a local variable instead of storing/reading/popping from dict.
The halt state is captured into _halted_by_thread[thread_id], immediately read back, and then popped—all within the same code block. This pattern adds complexity without benefit since the value never persists beyond this block. A local variable achieves the same goal more clearly.
Proposed simplification
- # Capture halt state for this thread to avoid race conditions
- # with concurrent runs modifying the shared adapter's halted flag
- self._halted_by_thread[thread_id] = self._adapter.halted
-
# If the adapter halted (frontend tool or built-in HITL tool like
# AskUserQuestion), interrupt the worker to prevent the SDK from
# auto-approving the tool call with a placeholder result.
- if self._halted_by_thread.get(thread_id, False):
+ if self._adapter.halted:
logger.info(
- f"Adapter halted for thread={thread_id}, "
- "interrupting worker to await user input"
+ "Adapter halted for thread=%s, interrupting worker to await user input",
+ thread_id,
)
await worker.interrupt()
- # Clear the halt flag for this thread
- self._halted_by_thread.pop(thread_id, None)If per-thread halt tracking is intended for access outside this method (e.g., querying halt state from another component), the value shouldn't be popped immediately—consider exposing it via a method instead.
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 135-136: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`
around lines 126 - 140, The code captures adapter halt state into
self._halted_by_thread[thread_id] then immediately reads and pops it; replace
that unnecessary dict round-trip with a local variable: assign halted =
self._adapter.halted (using thread_id only for logging), check halted and call
await worker.interrupt() if true, and remove the set/pop operations on
self._halted_by_thread; if persistent per-thread state is actually required
elsewhere, do not pop here and expose the state via an accessor instead.
Comment on lines
+88
to
+95
| HUMAN_INPUT_INSTRUCTIONS = ( | ||
| "## Human-in-the-Loop\n" | ||
| "When you need user input, a decision, or confirmation before proceeding, " | ||
| "you MUST use the AskUserQuestion tool. Do not ask questions in plain text " | ||
| "and wait for a response — the AskUserQuestion tool triggers platform " | ||
| "notifications and status indicators that help users know you need their " | ||
| "attention.\n\n" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Prompt helper callers ==\n'
rg -n -C2 '\b(resolve_workspace_prompt|build_workspace_context_prompt)\s*\('
printf '\n== AskUserQuestion registrations / implementations ==\n'
rg -n -C3 '\bAskUserQuestion\b|\bask_user_question\b|\baskUserQuestion\b'
printf '\n== Bridge/runtime tool exposure near prompt usage ==\n'
rg -n -C3 'resolve_workspace_prompt|register.*tool|tool_registry|available_tools|AskUserQuestion' components/runnersRepository: ambient-code/platform
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the context around line 219 in prompts.py to see the exact injection
printf '\n== Context around line 219 in prompts.py ==\n'
sed -n '210,230p' components/runners/ambient-runner/ambient_runner/platform/prompts.py
# Check the complete function signature of build_workspace_context_prompt
printf '\n== Complete build_workspace_context_prompt function ==\n'
sed -n '124,160p' components/runners/ambient-runner/ambient_runner/platform/prompts.py
# Look for all bridge implementations
printf '\n== Bridge implementations ==\n'
find components/runners/ambient-runner/ambient_runner/bridges -name '*.py' -type f | head -20
# Search for any other calls to resolve_workspace_prompt or build_workspace_context_prompt outside tests
printf '\n== Non-test calls to resolve_workspace_prompt ==\n'
rg -l 'resolve_workspace_prompt|build_workspace_context_prompt' --glob '!test*.py' --glob '!*test.py' components/runners/ambient-runnerRepository: ambient-code/platform
Length of output: 4000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if gemini_cli bridge uses resolve_workspace_prompt or build_workspace_context_prompt
printf '\n== Gemini CLI system_prompt.py ==\n'
cat components/runners/ambient-runner/ambient_runner/bridges/gemini_cli/system_prompt.py | head -100
printf '\n\n== LangGraph bridge prompt/system prompt setup ==\n'
rg -A 5 -B 5 'system_prompt|SYSTEM|prompt' components/runners/ambient-runner/ambient_runner/bridges/langgraph/bridge.py | head -60
# Check if these bridges register AskUserQuestion
printf '\n\n== Tool registration in langgraph bridge ==\n'
rg -i 'tool|ask' components/runners/ambient-runner/ambient_runner/bridges/langgraph/bridge.py | head -30
printf '\n\n== Tool registration in gemini_cli bridge ==\n'
rg -i 'tool|ask' components/runners/ambient-runner/ambient_runner/bridges/gemini_cli/bridge.py | head -30Repository: ambient-code/platform
Length of output: 4474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for ADK bridge references
printf '\n== ADK bridge references ==\n'
rg -i 'adk.*bridge|bridge.*adk' components/runners/ambient-runner
printf '\n\n== Search for "adk" directory ==\n'
find components/runners/ambient-runner/ambient_runner/bridges -type d -name '*adk*'
printf '\n\n== List all directories in bridges ==\n'
ls -la components/runners/ambient-runner/ambient_runner/bridges/
# Double-check who actually calls resolve_workspace_prompt or build_workspace_context_prompt (non-test)
printf '\n\n== All non-test calls to the shared prompt helpers ==\n'
rg -n 'resolve_workspace_prompt|build_workspace_context_prompt' components/runners/ambient-runner --glob '!test*.py' --glob '!*test.py' --glob '!*.jsonl'
# Check the complete resolve_workspace_prompt function flow
printf '\n\n== Full resolve_workspace_prompt implementation ==\n'
sed -n '268,295p' components/runners/ambient-runner/ambient_runner/platform/prompts.pyRepository: ambient-code/platform
Length of output: 2705
Gate AskUserQuestion instructions on runtime support.
The shared resolve_workspace_prompt helper documents support for both Claude and "ADK bridge" prompt builders, but currently only Claude uses it. Line 219 unconditionally injects HUMAN_INPUT_INSTRUCTIONS requiring the AskUserQuestion tool. While Claude fixtures confirm it always provides this tool, any future bridge calling this helper without AskUserQuestion support will instruct the model to invoke a non-existent capability. Add a parameter to gate this section to defensive design principles.
Suggested direction
def build_workspace_context_prompt(
repos_cfg: list,
workflow_name: str | None,
artifacts_path: str,
ambient_config: dict,
workspace_path: str,
+ supports_human_input: bool = False,
) -> str:
@@
- # Human-in-the-loop instructions
- prompt += HUMAN_INPUT_INSTRUCTIONS
+ # Human-in-the-loop instructions
+ if supports_human_input:
+ prompt += HUMAN_INPUT_INSTRUCTIONSPass supports_human_input=True from Claude bridge; default to False for safety. Also update docstring to remove reference to non-existent ADK bridge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/platform/prompts.py` around
lines 88 - 95, The helper resolve_workspace_prompt currently unconditionally
injects HUMAN_INPUT_INSTRUCTIONS which requires the AskUserQuestion tool; add a
boolean parameter supports_human_input (default False) to
resolve_workspace_prompt and use it to conditionally append
HUMAN_INPUT_INSTRUCTIONS only when True, update the docstring to remove the ADK
bridge claim, and ensure callers (e.g., the Claude bridge) pass
supports_human_input=True while other bridges omit or pass False so models are
not instructed to use an unsupported AskUserQuestion tool.
- **AskUserQuestion UI**: Interactive question component with single/multi-select options, freeform "Other" input, and multi-question tabbed navigation - **Agent status from event log**: Derive `agentStatus` at query time from persisted AG-UI events instead of caching on the CR (eliminates goroutine race conditions) - **Frontend status fix**: `useAgentStatus` hook reads raw `PlatformMessage.toolCalls[]` format, correctly detecting unanswered AskUserQuestion tool calls - **TOOL_CALL_RESULT emission**: Adapter emits `TOOL_CALL_RESULT` on next run for halted tool calls so the frontend transitions questions to answered state - **Session status indicators**: `SessionStatusDot` and `AgentStatusIndicator` components for detail and table views - **CRD cleanup**: Removed `agentStatus` field from CRD schema — no longer stored, only derived Supersedes #725. - [ ] Create session, send prompt that triggers AskUserQuestion - [ ] Verify question UI appears with options - [ ] Verify session table shows `waiting_input` status - [ ] Verify detail page shows `waiting_input` status - [ ] Submit answer, verify question transitions to answered (green) state - [ ] Verify agent resumes after answer 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Ambient Code Bot <bot@ambient-code.local> Co-authored-by: ambient-code[bot] <ambient-code[bot]@users.noreply.github.com>
Gkrumbach07
force-pushed
the
feat/human-in-the-loop
branch
from
66c12a2 to
c493dec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agentStatusat query time from persisted AG-UI events instead of caching on the CR (eliminates goroutine race conditions)SessionStatusDotandAgentStatusIndicatorcomponents for detail and table views,useAgentStatushook reads rawPlatformMessage.toolCalls[]formatTOOL_CALL_RESULTon next run for halted tool calls so the frontend transitions questions to answered staterefetchOnMount: 'always'so navigating back to the sessions page fetches fresh data immediatelySupersedes #871 (which was reverted in #896 due to a namespace-qualification bug in the event store). This version removes the namespace-qualification refactor entirely, keeping only the HITL feature changes.
Changes from #871
Removed:
.gitignoreworktrees entryMakefileport-forward cleanupcomponents/package-lock.jsondeletionUnixMillisession timestamp changeAdded:
refetchOnMount: 'always'on session list query for immediate refresh on navigationTest plan
🤖 Generated with Claude Code