-
Notifications
You must be signed in to change notification settings - Fork 126
🤖 feat: show task kind and spawn intent in single-task task_await summary #3793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5d442c4
aaaa785
b7e0530
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ import { GlobalWindow } from "happy-dom"; | |
|
|
||
| import { TooltipProvider } from "@/browser/components/Tooltip/Tooltip"; | ||
|
|
||
| import type { DisplayedMessage } from "@/common/types/message"; | ||
| import type { FrontendWorkspaceMetadata } from "@/common/types/workspace"; | ||
| import { computeTaskReportLinking } from "@/browser/utils/messages/taskReportLinking"; | ||
|
|
||
| let workspaceContextMock: { | ||
| workspaceMetadata: Map<string, FrontendWorkspaceMetadata>; | ||
|
|
@@ -69,6 +71,23 @@ function createWorkspaceMetadata( | |
| const taskAwaitArgs = { task_ids: ["task-1"], timeout_secs: 70 }; | ||
| const TaskAwaitToolCall = getToolComponent("task_await", taskAwaitArgs); | ||
|
|
||
| function createToolMessage(overrides: { | ||
| toolName: string; | ||
| args: unknown; | ||
| result?: unknown; | ||
| }): DisplayedMessage { | ||
| return { | ||
| type: "tool", | ||
| id: "tool-msg-1", | ||
| historyId: "hist-1", | ||
| toolCallId: "call-1", | ||
| status: "completed", | ||
| isPartial: false, | ||
| historySequence: 1, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| function renderTaskAwaitToolCall(props: Record<string, unknown> = {}) { | ||
| return render( | ||
| <TooltipProvider> | ||
|
|
@@ -363,6 +382,182 @@ describe("TaskAwaitToolCall", () => { | |
| expect(view.getByText("Task service unavailable")).toBeDefined(); | ||
| }); | ||
|
|
||
| test("shows bash kind and spawn model_intent for a single completed bash task", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [CRF-22] No viewport-pinned story covers the new collapsed detail, so the 375px claim rests on a session nobody can rerun. (Melody, Chopper, Mafu-san)
The overflow risk itself was checked and is structurally absent (the detail renders inside
|
||
| const bashSpawn = createToolMessage({ | ||
| toolName: "bash", | ||
| args: { | ||
| script: "./scripts/wait_pr_ready.sh 27330", | ||
| display_name: "PR ready watcher", | ||
| model_intent: "watching PR 27330 until it is ready", | ||
| timeout_secs: 3600, | ||
| run_in_background: true, | ||
| }, | ||
| result: { | ||
| success: true, | ||
| output: "Started", | ||
| exitCode: 0, | ||
| wall_duration_ms: 10, | ||
| taskId: "bash:pr-ready-watcher-a1b2", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [CRF-30] The fixture taskId is a shape the backend never produces, which is what hides CRF-8 from anyone reading these tests. (Nami)
The test still exercises the right path; the invented suffix just makes the id look unique when it is not.
|
||
| backgroundProcessId: "pr-ready-watcher-a1b2", | ||
| }, | ||
| }); | ||
|
|
||
| const view = renderTaskAwaitToolCall({ | ||
| status: "completed", | ||
| args: { task_ids: ["bash:pr-ready-watcher-a1b2"] }, | ||
| result: { | ||
| results: [ | ||
| { | ||
| status: "completed", | ||
| taskId: "bash:pr-ready-watcher-a1b2", | ||
| title: "PR ready watcher", | ||
| reportMarkdown: "exit 0", | ||
| }, | ||
| ], | ||
| }, | ||
| taskReportLinking: computeTaskReportLinking([bashSpawn]), | ||
| }); | ||
|
|
||
| expect(view.getByText("1 task completed")).toBeDefined(); | ||
| expect(view.getByText(/bash · Watching PR 27330 until it is ready/)).toBeDefined(); | ||
| }); | ||
|
|
||
| test("falls back to the task title when the spawn intent merely restates the command", () => { | ||
| const bashSpawn = createToolMessage({ | ||
| toolName: "bash", | ||
| args: { | ||
| script: "git status", | ||
| display_name: "Repo State", | ||
| model_intent: "git status", | ||
| timeout_secs: 30, | ||
| run_in_background: true, | ||
| }, | ||
| result: { | ||
| success: true, | ||
| output: "Started", | ||
| exitCode: 0, | ||
| wall_duration_ms: 10, | ||
| taskId: "bash:repo-state-a1b2", | ||
| backgroundProcessId: "repo-state-a1b2", | ||
| }, | ||
| }); | ||
|
|
||
| const view = renderTaskAwaitToolCall({ | ||
| status: "completed", | ||
| args: { task_ids: ["bash:repo-state-a1b2"] }, | ||
| result: { | ||
| results: [ | ||
| { | ||
| status: "completed", | ||
| taskId: "bash:repo-state-a1b2", | ||
| title: "Repo State", | ||
| reportMarkdown: "exit 0", | ||
| }, | ||
| ], | ||
| }, | ||
| taskReportLinking: computeTaskReportLinking([bashSpawn]), | ||
| }); | ||
|
|
||
| expect(view.getByText(/bash · Repo State/)).toBeDefined(); | ||
| expect(view.queryByText(/bash · Git status/)).toBeNull(); | ||
| }); | ||
|
|
||
| test("falls back to the completed task title when no spawn intent is linked", () => { | ||
| const view = renderTaskAwaitToolCall({ | ||
| status: "completed", | ||
| args: { task_ids: ["bash:pr-ready-watcher-a1b2"] }, | ||
| result: { | ||
| results: [ | ||
| { | ||
| status: "completed", | ||
| taskId: "bash:pr-ready-watcher-a1b2", | ||
| title: "PR ready watcher", | ||
| reportMarkdown: "exit 0", | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| expect(view.getByText(/bash · PR ready watcher/)).toBeDefined(); | ||
| }); | ||
|
|
||
| test("shows agent type and title for a single completed sub-agent task", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 [CRF-16] The sub-agent test asserts a string that both candidate title sources produce, so it cannot fail for the precedence it appears to cover. (Mafu-san)
The
|
||
| const taskSpawn = createToolMessage({ | ||
| toolName: "task", | ||
| args: { | ||
| agentId: "explore", | ||
| prompt: "Find pagination helpers.", | ||
| title: "Pagination exploration", | ||
| run_in_background: true, | ||
| }, | ||
| result: { status: "queued", taskId: "task-1" }, | ||
| }); | ||
|
|
||
| const view = renderTaskAwaitToolCall({ | ||
| status: "completed", | ||
| result: { | ||
| results: [ | ||
| { | ||
| status: "completed", | ||
| taskId: "task-1", | ||
| title: "Pagination exploration", | ||
| reportMarkdown: "Report", | ||
| }, | ||
| ], | ||
| }, | ||
| taskReportLinking: computeTaskReportLinking([taskSpawn]), | ||
| }); | ||
|
|
||
| expect(view.getByText(/explore · Pagination exploration/)).toBeDefined(); | ||
| }); | ||
|
|
||
| test("prefers the spawn title over the sub-agent's own report title", () => { | ||
| const taskSpawn = createToolMessage({ | ||
| toolName: "task", | ||
| args: { | ||
| agentId: "explore", | ||
| prompt: "Find pagination helpers.", | ||
| title: "Pagination exploration", | ||
| run_in_background: true, | ||
| }, | ||
| result: { status: "queued", taskId: "task-1" }, | ||
| }); | ||
|
|
||
| const view = renderTaskAwaitToolCall({ | ||
| status: "completed", | ||
| result: { | ||
| results: [ | ||
| { | ||
| status: "completed", | ||
| taskId: "task-1", | ||
| title: "Pagination Helpers Investigation Complete", | ||
| reportMarkdown: "Report", | ||
| }, | ||
| ], | ||
| }, | ||
| taskReportLinking: computeTaskReportLinking([taskSpawn]), | ||
| }); | ||
|
|
||
| expect(view.getByText(/explore · Pagination exploration/)).toBeDefined(); | ||
| expect(view.queryByText(/Investigation Complete/)).toBeNull(); | ||
| }); | ||
|
|
||
| test("keeps multi-task completion summaries count-only", () => { | ||
| const view = renderTaskAwaitToolCall({ | ||
| status: "completed", | ||
| args: { task_ids: ["task-1", "task-2"] }, | ||
| result: { | ||
| results: [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note [CRF-5]
It is a boundary guard for the deliberate "multi stays count-only" decision, not a proof of this diff, so it carries no red-green value today. Worth knowing when reading the "5 new tests ... red-green verified" claim in the PR body: four were verified red, one cannot be, and a fifth (CRF-16) cannot fail for the behavior it names.
|
||
| { status: "completed", taskId: "task-1", title: "First task", reportMarkdown: "a" }, | ||
| { status: "completed", taskId: "task-2", title: "Second task", reportMarkdown: "b" }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| expect(view.getByText("2 tasks completed")).toBeDefined(); | ||
| expect(view.queryByText(/First task/)).toBeNull(); | ||
| }); | ||
|
|
||
| test("uses valid legacy agentType for task_await rows when agentId is invalid", () => { | ||
| workspaceContextMock = { | ||
| workspaceMetadata: new Map([ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ import type { | |
| } from "@/common/types/tools"; | ||
| import type { TaskReportLinking } from "@/browser/utils/messages/taskReportLinking"; | ||
| import { formatGitPatchArtifactSummary } from "./taskPatchSummary"; | ||
| import { sanitizeDisplayableModelIntent } from "./bashCollapsedSummary"; | ||
| import { | ||
| formatTaskGroupCreationLabel, | ||
| formatTaskGroupHeader, | ||
|
|
@@ -460,6 +461,10 @@ function isWorkspaceTurnTaskHandleId(taskId: string): boolean { | |
| return /^wst_[a-z0-9][a-z0-9_-]*$/.test(taskId); | ||
| } | ||
|
|
||
| function isWorkflowRunTaskHandleId(taskId: string): boolean { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-3]
The "single source of truth" claim in
|
||
| return taskId.startsWith("wfr_"); | ||
| } | ||
|
|
||
| function fromBashTaskId(taskId: string): string | null { | ||
| const prefix = "bash:"; | ||
| if (!taskId.startsWith(prefix)) { | ||
|
|
@@ -1334,6 +1339,33 @@ export const TaskAwaitToolCall: React.FC<TaskAwaitToolCallProps> = ({ | |
| const targetCount = totalCount > 0 ? totalCount : taskIds?.length; | ||
| const formatTasks = (count: number) => `${count} ${count === 1 ? "task" : "tasks"}`; | ||
|
|
||
| // "1 task completed" alone says nothing about what finished; for single-task awaits, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [CRF-23] The second clause of the new comment narrates the code below it. (Gon P2, adjusted to Nit)
// "1 task completed" alone says nothing about what finished.Severity adjusted down from Gon's P2: the review vocabulary reserves Nit for convention violations where the code works, and nothing depends on the second clause. Gon's wider point stands and is worth acting on as a set: three of this diff's comment sites need edits (CRF-4, CRF-24, and this one). Leorio's counterpoint, recorded because it is the more useful half: the first clause is exactly what a comment is for, quoting the string it replaces and naming who was hurt by it. More of that.
|
||
| // surface the task's kind plus its spawn intent/title in the collapsed row. | ||
| const firstResult = results[0]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-18]
The block also runs on the error, interrupted, executing and waiting renders, whose branches discard const only = results.length === 1 ? results[0] : undefined;
if (only?.status === "completed") { ... }The optional chain then expresses the guard instead of depending on statement order. Nami's alternative is worth weighing: a sibling pure module (
|
||
| let singleTaskDetail: string | undefined; | ||
| if (results.length === 1 && firstResult.status === "completed") { | ||
| const completedTaskId = firstResult.taskId; | ||
| const bashSpawn = taskReportLinking?.bashSpawnByTaskId.get(completedTaskId); | ||
| const kind = fromBashTaskId(completedTaskId) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-17] This is the fifth in-file spelling of "what kind is this task", and the new one already disagrees with the one 30 lines above. (Mafu-san P3, Zoro P3, Knov Nit, Chopper Nit)
They already disagree for the same task: the waiting path derives kind from the handle id and labels bash tasks with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [CRF-25]
|
||
| ? "bash" | ||
| : isWorkflowRunTaskHandleId(completedTaskId) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-2] Two of the four
Both branches work: temporary tests rendered
|
||
| ? "workflow" | ||
| : isWorkspaceTurnTaskHandleId(completedTaskId) || | ||
| firstResult.handleKind === "workspace_turn" | ||
| ? "workspace" | ||
| : taskReportLinking?.spawnAgentTypeByTaskId.get(completedTaskId); | ||
| // Spawn-side intent first (bash model_intent, task spawn title); the result's own | ||
| // title (report heading, bash display_name) is only a fallback. | ||
| const description = | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 [CRF-11] The collapsed row builds a weaker title chain than the expanded row, so the exact case this PR targets can still read
Three reviewers rendered it independently:
|
||
| (bashSpawn | ||
| ? sanitizeDisplayableModelIntent(bashSpawn.modelIntent, bashSpawn.script) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note [CRF-29] The bash detail ignores the user's "Collapsed bash summaries" setting. (Nami)
Held at Note deliberately: the setting's own copy scopes itself to "collapsed bash tools", and a
|
||
| : undefined) ?? | ||
| trimToNonEmptyString(taskReportLinking?.spawnTitleByTaskId.get(completedTaskId)) ?? | ||
| trimToNonEmptyString(firstResult.title); | ||
| const detail = [kind, description].filter((part): part is string => part != null).join(" · "); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note [CRF-31] Model-controlled text is spliced into a
No security finding: the sink is a React text child inside a truncating span, no
|
||
| singleTaskDetail = detail.length > 0 ? detail : undefined; | ||
| } | ||
|
|
||
| let summaryTitle: string; | ||
| let summaryDetail: string | undefined; | ||
| let summaryTone: "active" | "danger" | "interrupted" | "success" | "waiting"; | ||
|
|
@@ -1369,6 +1401,7 @@ export const TaskAwaitToolCall: React.FC<TaskAwaitToolCallProps> = ({ | |
| summaryTone = "waiting"; | ||
| } else if (completedCount > 0) { | ||
| summaryTitle = `${formatTasks(completedCount)} completed`; | ||
| summaryDetail = singleTaskDetail; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 [CRF-7] A background bash that exited nonzero or was killed renders as
Verified by render, twice independently: a completed result When
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 [CRF-10] A run of two or more adjacent
Orchestrator verified the mechanism: for a cleanly settled group Either give the
|
||
| summaryTone = "success"; | ||
| } else { | ||
| summaryTitle = "Checked task status"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,11 +37,7 @@ export function buildBashCollapsedSummary( | |
| return { kind: "command", command }; | ||
| } | ||
|
|
||
| const intent = sanitizeModelIntent(options.args.model_intent, command); | ||
| const displayIntent = | ||
| intent && normalizeForComparison(intent) !== normalizeForComparison(command) | ||
| ? intent | ||
| : undefined; | ||
| const displayIntent = sanitizeDisplayableModelIntent(options.args.model_intent, command); | ||
| if (mode === "intent") { | ||
| return { | ||
| kind: "intent", | ||
|
|
@@ -91,6 +87,17 @@ export function sanitizeModelIntent(rawIntent: unknown, command: string): string | |
| return capitalize(intent); | ||
| } | ||
|
|
||
| /** Sanitized intent, or undefined when it merely restates the command. */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [CRF-28] "or undefined when it merely restates the command" names one of the two ways this returns undefined. (Leorio)
/** Sanitized intent, or undefined when there is no usable intent or it merely restates the command. */
|
||
| export function sanitizeDisplayableModelIntent( | ||
| rawIntent: unknown, | ||
| command: string | ||
| ): string | undefined { | ||
| const intent = sanitizeModelIntent(rawIntent, command); | ||
| return intent && normalizeForComparison(intent) !== normalizeForComparison(command) | ||
| ? intent | ||
| : undefined; | ||
| } | ||
|
|
||
| function getIntentOnlyFallback(args: BashToolArgs, command: string): string { | ||
| const displayName = typeof args.display_name === "string" ? args.display_name.trim() : ""; | ||
| if (displayName && normalizeForComparison(displayName) !== normalizeForComparison(command)) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit [CRF-33]
createToolMessageis the second local factory for a toolDisplayedMessage, and it hardcodes every id. (Robin, Bisky)Fine today because every test passes a single-message array; a future test with two spawns gets duplicate ids and silently ambiguous linking, which is exactly the fixture shape CRF-8 needs. One exported fixture next to the
DisplayedMessageconsumers would serve both, and a third copy is the likely next event.