Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions coderd/x/chatd/chatadvisor/guidance.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@ call with no tools, and returns concise guidance for the parent agent rather
than the end user. Provide a brief question, no more than 2000 runes. Summarize
context instead of pasting long logs or transcripts.
</advisor-guidance>`
// LimitReachedAdvice is returned when the per-run advisor budget is
// exhausted, telling the parent agent to stop calling the advisor.
LimitReachedAdvice = "The advisor budget for this turn is exhausted. Do not call the advisor again this turn; proceed with your own judgment."
)
1 change: 1 addition & 0 deletions coderd/x/chatd/chatadvisor/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func (rt *Runtime) RunAdvisor(
if !rt.tryAcquire() {
return AdvisorResult{
Type: ResultTypeLimitReached,
Advice: LimitReachedAdvice,
RemainingUses: 0,
}, nil
}
Expand Down
2 changes: 1 addition & 1 deletion coderd/x/chatd/chatadvisor/tool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ func TestAdvisorToolReportsLimitReached(t *testing.T) {
require.NoError(t, json.Unmarshal([]byte(second.Content), &result))
require.Equal(t, chatadvisor.ResultTypeLimitReached, result.Type)
require.Equal(t, 0, result.RemainingUses)
require.Empty(t, result.Advice)
require.Equal(t, chatadvisor.LimitReachedAdvice, result.Advice)
require.Empty(t, result.Error)
require.Empty(t, result.AdvisorModel)
}
Expand Down
4 changes: 2 additions & 2 deletions coderd/x/chatd/chatadvisor/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ const (
ResultTypeError ResultType = "error"
)

// AdvisorArgs contains the tool-visible advisor question.
type AdvisorArgs struct {
Question string `json:"question" description:"A brief question for the advisor. Must be 2000 runes or fewer. Summarize context instead of pasting long logs or transcripts."`
Question string `json:"question" description:"A brief question for the advisor. Must be 2000 runes or fewer. Summarize context instead of pasting long logs or transcripts."`
ModelIntent *string `json:"model_intent,omitempty" description:"A short, natural-language, present-participle phrase describing why you are consulting the advisor. This is shown to the user as the tool row title, so write it as a standalone action. Use plain English with no underscores or technical jargon. Do not restate the question. Keep it under 100 characters. Good examples: \"Weighing a refactor tradeoff\", \"Checking a migration strategy\", \"Planning a safe rollout\"."`
}

// AdvisorResult is the structured result returned by the advisor runtime.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ const sampleQuestion =
"Should we extract a shared helper for tool result parsing before refactoring the agents page tool cards?";

const longQuestion = [
"We are planning a risky refactor of the advisor tool UI after several rounds of feedback from designers, frontend engineers, and dogfood users. The goal is to keep the card readable when the advisor includes a long prompt, a model name, a remaining-use count, and an expanded body with long markdown guidance.",
"Before changing the layout further, I want advice on whether the metadata should remain inline with the title, move into compact chips, wrap onto a second line, or disappear behind a details affordance when horizontal space is tight. Please weigh readability, scanability, accessibility, and consistency with adjacent tool cards.",
"The edge case I care about most is a real agent asking a verbose strategic question that includes implementation history, user feedback, test expectations, and design constraints in one tool call. The card should still make the question easy to read, avoid truncating important context, and keep the advisor identity, model, and usage details visually distinct.",
"Assume the answer may contain multiple markdown sections, bullets, and code references. The UI should not become visually heavy, the header should not look like one blended text block, the question should wrap naturally, and the body should remain scrollable without pushing nearby chat messages too far away.",
"Please recommend the safest layout and interaction behavior for this peak state, including where the metadata belongs, how much emphasis the long question should receive, whether the expanded state should stay open by default, and which details should be visible to users versus only useful for debugging.",
"Also call out any accessibility risks from nested buttons, long labels, dense metadata, color-only separators, or scroll regions, and suggest a practical test plan that Storybook can cover without adding brittle assertions about exact Tailwind class names.",
"We are planning a risky refactor of the advisor tool UI after several rounds of feedback from designers, frontend engineers, and dogfood users. The goal is to keep the row readable when the advisor includes a long prompt, a remaining-use count, and an expanded body with long markdown guidance.",
"Before changing the layout further, I want advice on whether the metadata should remain inline with the title, move into compact trailing text, or disappear when horizontal space is tight. Please weigh readability, scanability, accessibility, and consistency with adjacent tool cards.",
"The edge case I care about most is a real agent asking a verbose strategic question that includes implementation history, user feedback, test expectations, and design constraints in one tool call. The row should still make the question easy to scan, truncate gracefully, and keep the advisor identity visually distinct from the answer.",
].join(" ");

const sampleAdvice = [
Expand Down Expand Up @@ -51,11 +48,11 @@ const longAdvice = [
"- Use markdown rendering for prose and code examples.",
"- Preserve a subtle metadata footer for debugging and support.",
"",
"The dedicated card should still behave like the existing tool cards, including collapse, expansion, and overflow handling for long guidance.",
"The dedicated row should still behave like the existing tool rows, including collapse, expansion, and overflow handling for long guidance.",
"",
]).flat(),
"## Follow-up questions",
"1. Should the card stay expanded by default?",
"1. Should the row stay expanded by default while running?",
"2. Should limit states include remaining uses when the backend provides them?",
"3. Should the error state surface the raw provider message or a friendlier summary?",
]
Expand All @@ -77,25 +74,26 @@ export const SuccessfulAdvice: Story = {
result: {
type: "advice",
advice: sampleAdvice,
advisor_model: "GPT-5 Advisor",
advisor_model: "openai/gpt-5.1",
remaining_uses: 3,
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText(sampleQuestion)).toBeInTheDocument();
const toggle = canvas.getByRole("button");

expect(toggle).toHaveAttribute("aria-expanded", "false");
expect(canvas.getByText("Consulted the advisor")).toBeInTheDocument();
expect(canvas.queryByText(sampleQuestion)).not.toBeInTheDocument();
expect(canvas.queryByText("Quick summary")).not.toBeInTheDocument();

expect(canvas.queryByText("openai/gpt-5.1")).not.toBeInTheDocument();
expect(canvas.queryByText("3 left")).not.toBeInTheDocument();

await userEvent.click(toggle);
expect(toggle).toHaveAttribute("aria-expanded", "true");
expect(await canvas.findByText(sampleQuestion)).toBeInTheDocument();
expect(await canvas.findByText("Quick summary")).toBeInTheDocument();
expect(canvas.getByText("Advice")).toBeInTheDocument();
expect(canvas.queryByText("Guidance ready")).not.toBeInTheDocument();
expect(canvas.getByText("GPT-5 Advisor")).toBeInTheDocument();
expect(canvas.getByText("3 uses left")).toBeInTheDocument();
expect(
canvas.queryByText(
(_, element) =>
element?.textContent?.replace(/\s+/g, " ").trim() ===
"Advisor model: GPT-5 Advisor",
),
).not.toBeInTheDocument();
},
};

Expand All @@ -106,14 +104,47 @@ export const Running: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByRole("button")).toHaveAttribute("aria-expanded", "true");
expect(canvas.getByText("Consulting the advisor")).toBeInTheDocument();
expect(canvas.getByText(sampleQuestion)).toBeInTheDocument();
expect(canvas.getAllByText("Consulting advisor…")).toHaveLength(1);
expect(
canvas.getByText("Reviewing context and preparing guidance."),
).toBeInTheDocument();
},
};

// When the model supplies a model_intent, it is the whole header label,
// matching how the exec tool renders its intent.
export const WithModelIntent: Story = {
args: {
status: "completed",
args: {
question: sampleQuestion,
model_intent: "Weighing a refactor tradeoff",
},
// The backend surfaces model_intent as a top-level tool field, so the
// story passes it the same way the timeline does.
modelIntent: "Weighing a refactor tradeoff",
result: {
type: "advice",
advice: sampleAdvice,
remaining_uses: 2,
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const toggle = canvas.getByRole("button", {
name: /Weighing a refactor tradeoff/,
});
expect(toggle).toHaveAttribute("aria-expanded", "false");
expect(canvas.queryByText(/Consulted the advisor/)).not.toBeInTheDocument();
expect(canvas.queryByText("2 left")).not.toBeInTheDocument();

await userEvent.click(toggle);
expect(await canvas.findByText(sampleQuestion)).toBeInTheDocument();
},
};

export const RunningWithStreamedAdvice: Story = {
args: {
status: "running",
Expand All @@ -123,7 +154,7 @@ export const RunningWithStreamedAdvice: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText(sampleQuestion)).toBeInTheDocument();
expect(canvas.getByText("Consulting advisor")).toBeInTheDocument();
expect(canvas.getByText("Consulting the advisor")).toBeInTheDocument();
expect(
await canvas.findByText(
"Use the smaller diff while the advisor is still responding.",
Expand All @@ -149,14 +180,21 @@ export const LimitReached: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText("Advisor limit reached.")).toBeInTheDocument();
const toggle = canvas.getByRole("button", {
name: /Advisor limit reached/,
});
expect(toggle).toHaveAttribute("aria-expanded", "false");

await userEvent.click(toggle);
expect(
await canvas.findByText("Advisor limit reached."),
).toBeInTheDocument();
expect(
canvas.getByText(
"You have reached the advisor limit for this conversation.",
),
).toBeInTheDocument();
// Assert the semantic role screen readers rely on to announce the
// limit state. A refactor that drops role="status" should fail here.
// Screen readers announce the limit state via role="status".
expect(canvas.getByRole("status")).toBeInTheDocument();
},
};
Expand All @@ -173,12 +211,17 @@ export const ErrorState: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText("Advisor request failed.")).toBeInTheDocument();
const toggle = canvas.getByRole("button");
expect(toggle).toHaveAttribute("aria-expanded", "false");

await userEvent.click(toggle);
expect(
await canvas.findByText("Advisor request failed."),
).toBeInTheDocument();
expect(
canvas.getByText("The advisor service is temporarily unavailable."),
).toBeInTheDocument();
// Assert the semantic role screen readers rely on to announce the
// error state. A refactor that drops role="alert" should fail here.
// Screen readers announce the error state via role="alert".
expect(canvas.getByRole("alert")).toBeInTheDocument();
},
};
Expand All @@ -194,10 +237,13 @@ export const EmptyQuestion: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText("No question provided.")).toBeInTheDocument();
// Confirm the advice body still renders alongside the blank-question
// fallback, so a future refactor that suppresses the body for empty
// questions cannot pass silently.
expect(canvas.queryByText("No question provided.")).not.toBeInTheDocument();
await userEvent.click(canvas.getByRole("button"));
expect(
await canvas.findByText("No question provided."),
).toBeInTheDocument();
// The advice body still renders after expanding, so a refactor that
// suppresses the body for empty questions cannot pass silently.
expect(await canvas.findByText("Quick summary")).toBeInTheDocument();
},
};
Expand All @@ -212,8 +258,9 @@ export const EmptyAdvice: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(canvas.getByRole("button"));
expect(
canvas.getByText("Advisor returned no guidance."),
await canvas.findByText("Advisor returned no guidance."),
).toBeInTheDocument();
expect(canvas.queryByText("No guidance")).not.toBeInTheDocument();
},
Expand All @@ -230,7 +277,10 @@ export const BlankError: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText("Advisor request failed.")).toBeInTheDocument();
await userEvent.click(canvas.getByRole("button"));
expect(
await canvas.findByText("Advisor request failed."),
).toBeInTheDocument();
expect(
canvas.getByText("Advisor could not return guidance."),
).toBeInTheDocument();
Expand All @@ -240,7 +290,7 @@ export const BlankError: Story = {

// Mirrors the backend path where a tool call is marked execution-failed
// (status === "error") without a structured result payload. The renderer
// must fold the error status into the error signal so the card surfaces
// must fold the error status into the error signal so the row surfaces
// the failure instead of falling through to "Advisor returned no guidance".
export const StatusErrorWithoutResult: Story = {
args: {
Expand All @@ -249,7 +299,10 @@ export const StatusErrorWithoutResult: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText("Advisor request failed.")).toBeInTheDocument();
await userEvent.click(canvas.getByRole("button"));
expect(
await canvas.findByText("Advisor request failed."),
).toBeInTheDocument();
expect(
canvas.getByText("Advisor could not return guidance."),
).toBeInTheDocument();
Expand All @@ -260,8 +313,8 @@ export const StatusErrorWithoutResult: Story = {
// Mirrors the backend path where a tool call is marked execution-failed
// (status === "error") and the result payload is a raw string instead of
// a structured object. AdvisorRenderer must route the string through the
// `errorMessage` branch so the failure surfaces in the error card rather
// than being rendered as advice text.
// `errorMessage` branch so the failure surfaces rather than being rendered
// as advice text.
export const StatusErrorWithStringResult: Story = {
args: {
status: "error",
Expand All @@ -270,7 +323,10 @@ export const StatusErrorWithStringResult: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText("Advisor request failed.")).toBeInTheDocument();
await userEvent.click(canvas.getByRole("button"));
expect(
await canvas.findByText("Advisor request failed."),
).toBeInTheDocument();
expect(canvas.getByText("Connection timed out")).toBeInTheDocument();
expect(canvas.getByRole("alert")).toBeInTheDocument();
},
Expand All @@ -287,50 +343,14 @@ export const PlainStringResult: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText(sampleQuestion)).toBeInTheDocument();
expect(canvas.queryByText(sampleQuestion)).not.toBeInTheDocument();
await userEvent.click(canvas.getByRole("button"));
expect(
await canvas.findByText(
"Prefer extracting a shared helper once two renderers need it.",
),
).toBeInTheDocument();
},
};

export const LongAdvice: Story = {
args: {
status: "completed",
args: { question: sampleQuestion },
result: {
type: "advice",
advice: longAdvice,
advisor_model: "GPT-5 Advisor",
remaining_uses: 12,
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const toggle = canvas.getByRole("button");

expect(toggle).toHaveAttribute("aria-expanded", "true");
await userEvent.click(toggle);
expect(toggle).toHaveAttribute("aria-expanded", "false");
expect(canvas.queryByText("Follow-up questions")).not.toBeInTheDocument();

await userEvent.click(toggle);
expect(toggle).toHaveAttribute("aria-expanded", "true");
expect(await canvas.findByText("Follow-up questions")).toBeInTheDocument();

const scrollArea = canvas.getByTestId("advisor-tool-scroll-area");
const viewport = scrollArea.querySelector(
"[data-radix-scroll-area-viewport]",
);
if (!(viewport instanceof HTMLElement)) {
throw new globalThis.Error("Expected advisor scroll viewport.");
}

viewport.scrollTop = viewport.scrollHeight;
viewport.dispatchEvent(new Event("scroll"));
expect(viewport.scrollTop).toBeGreaterThan(0);
expect(await canvas.findByText(sampleQuestion)).toBeInTheDocument();
},
};

Expand All @@ -342,33 +362,22 @@ export const LongAdviceLongQuestion: Story = {
result: {
type: "advice",
advice: longAdvice,
advisor_model: "GPT-5 Advisor",
advisor_model: "openai/gpt-5.1",
remaining_uses: 12,
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const toggle = canvas.getByRole("button");
const question = canvas.getByText(longQuestion);

expect(question).toBeInTheDocument();
const expandedQuestionHeight = question.getBoundingClientRect().height;
expect(expandedQuestionHeight).toBeGreaterThan(40);
expect(await canvas.findByText("Follow-up questions")).toBeInTheDocument();
expect(canvas.getByText("Advice")).toBeInTheDocument();
expect(canvas.getByText("GPT-5 Advisor")).toBeInTheDocument();
expect(canvas.getByText("12 uses left")).toBeInTheDocument();

await userEvent.click(toggle);
expect(toggle).toHaveAttribute("aria-expanded", "false");
expect(question.getBoundingClientRect().height).toBeLessThan(
expandedQuestionHeight,
);
expect(canvas.queryByText(longQuestion)).not.toBeInTheDocument();
expect(canvas.queryByText("12 left")).not.toBeInTheDocument();
expect(canvas.queryByText("Follow-up questions")).not.toBeInTheDocument();

await userEvent.click(toggle);
expect(toggle).toHaveAttribute("aria-expanded", "true");
expect(question.getBoundingClientRect().height).toBeGreaterThan(40);
expect(await canvas.findByText(longQuestion)).toBeInTheDocument();
expect(await canvas.findByText("Follow-up questions")).toBeInTheDocument();
},
};
Loading
Loading