mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-19 07:54:38 +00:00
fix(cli): reconcile registry-missing snapshots as just-finished + address review nits
Hot-fix: the previous round's "drop the row when registry.get()
returns undefined" was too aggressive. `unregisterForeground` calls
`emitStatusChange(entry)` BEFORE it deletes the entry, so the
snapshot useBackgroundTaskView captures still says "running" while
the very next render's registry.get sees nothing. Dropping the row
outright made foreground subagents disappear from the panel the
instant they finished — users saw "SubAgents 不显示了" on tasks that
ran-and-immediately-completed.
Reconciliation now has three branches:
1. live found → use live (newest recentActivities).
2. snap says still-live but registry forgot → synthesize a
terminal version with endTime pinned to the FIRST observation
so the 8s visibility window gives the user a "the agent
finished" beat then evicts cleanly. The first-seen-missing
timestamp is held in a useRef map (without it, each tick
resets endTime to `now` and the row never expires).
3. snap is already terminal but registry forgot → drop (no
useful state to keep showing).
Also addresses three smaller review notes (deepseek-v4-pro via
/review):
- DEFAULT_SUBAGENT_TYPE is now imported from
@qwen-code/qwen-code-core (a new DEFAULT_BUILTIN_SUBAGENT_TYPE
export referenced by both BuiltinAgentRegistry's seed entry and
the panel's default-type elision). A backend rename now propagates
instead of silently re-introducing the redundant
`general-purpose:` prefix on every row.
- The useMemo body now reads `now` (as `reconcileAt`) so the
dependency is semantically honest — a future "remove dead dep"
cleanup can no longer silently freeze the panel on the first
tool-call after a snapshot refresh.
Coverage delta: +5 cases on LiveAgentPanel.test.tsx — token rendering
on completed entries, status-icon routing for paused / failed /
cancelled (parametrized), case-insensitive prefix stripping in
descriptionWithoutPrefix, plus the rewritten ghost-row case
(synthesized terminal lingers 8s then evicts) and a sibling case
asserting already-terminal snapshots with empty registry still drop.
17 LiveAgentPanel tests pass.
This commit is contained in:
parent
1bb1326904
commit
dbad3f9449
4 changed files with 191 additions and 33 deletions
|
|
@ -156,6 +156,73 @@ describe('<LiveAgentPanel />', () => {
|
|||
expect(frame).toContain('5s');
|
||||
});
|
||||
|
||||
it('renders elapsed + token count for completed agents with stats', () => {
|
||||
// Locks in the cost-visibility win the panel is partly motivated
|
||||
// by — completed entries should surface `▶ Ns · Nk tokens`. Using
|
||||
// a completed entry (rather than running) so the assertion is
|
||||
// stable against the running-tally heuristic.
|
||||
const { lastFrame } = renderPanel({
|
||||
entries: [
|
||||
agentEntry({
|
||||
agentId: 'done-1',
|
||||
subagentType: 'researcher',
|
||||
description: 'researcher: investigate',
|
||||
status: 'completed',
|
||||
startTime: -12_000,
|
||||
endTime: 0,
|
||||
stats: { totalTokens: 2400, toolUses: 5, durationMs: 12_000 },
|
||||
}),
|
||||
],
|
||||
});
|
||||
const frame = lastFrame() ?? '';
|
||||
expect(frame).toContain('12s');
|
||||
expect(frame).toContain('2.4k tokens');
|
||||
});
|
||||
|
||||
it.each([
|
||||
['paused', '⏸'],
|
||||
['failed', '✖'],
|
||||
['cancelled', '✖'],
|
||||
] as const)('renders the %s status with the %s glyph', (status, glyph) => {
|
||||
// Status routing is otherwise uncovered for paused / failed /
|
||||
// cancelled — a future regression that flattened the switch
|
||||
// would slip past the existing running / completed cases.
|
||||
const { lastFrame } = renderPanel({
|
||||
entries: [
|
||||
agentEntry({
|
||||
agentId: `${status}-1`,
|
||||
subagentType: 'researcher',
|
||||
description: 'researcher: status routing fixture',
|
||||
status,
|
||||
// paused entries don't carry an endTime; failed / cancelled do.
|
||||
endTime: status === 'paused' ? undefined : 0,
|
||||
}),
|
||||
],
|
||||
});
|
||||
expect(lastFrame() ?? '').toContain(glyph);
|
||||
});
|
||||
|
||||
it('strips the subagentType: prefix from the description case-insensitively', () => {
|
||||
// `descriptionWithoutPrefix` lowercases both sides — the existing
|
||||
// tests only feed lowercase prefixes, so a future revert to
|
||||
// strict `startsWith` would silently re-introduce
|
||||
// `Researcher: Researcher: …` double-prefix on capitalised inputs.
|
||||
const { lastFrame } = renderPanel({
|
||||
entries: [
|
||||
agentEntry({
|
||||
agentId: 'cap-1',
|
||||
subagentType: 'researcher',
|
||||
description: 'Researcher: cap-mismatch description',
|
||||
}),
|
||||
],
|
||||
});
|
||||
const frame = lastFrame() ?? '';
|
||||
// The prefix MUST have been stripped — the descriptive tail
|
||||
// should appear exactly once, with no leading "Researcher: ".
|
||||
expect(frame).toContain('cap-mismatch description');
|
||||
expect(frame).not.toContain('Researcher: cap-mismatch');
|
||||
});
|
||||
|
||||
it('does NOT surface a flavor marker on foreground agents', () => {
|
||||
// Foreground vs background distinction stays with BackgroundTasksDialog
|
||||
// (where cancel semantics differ); the panel reads as a glance roster
|
||||
|
|
@ -275,23 +342,53 @@ describe('<LiveAgentPanel />', () => {
|
|||
expect(lastFrame() ?? '').toBe('');
|
||||
});
|
||||
|
||||
it('drops snapshot rows the live registry no longer knows about', () => {
|
||||
// Foreground subagents unregister silently after the status-change
|
||||
// callback fires (`unregisterForeground` deletes from the registry
|
||||
// without emitting another transition). The snapshot still lists the
|
||||
// entry as `running`, so a naive `live ?? snap` fallback would leave
|
||||
// a ghost row that never clears. The panel must trust the registry
|
||||
// and drop the row when `registry.get()` returns undefined.
|
||||
it('reconciles snapshots the live registry no longer knows about as just-finished', () => {
|
||||
// `unregisterForeground` calls `emitStatusChange(entry)` BEFORE
|
||||
// it deletes the entry, so a snapshot taken on that callback
|
||||
// captures the agent as "still running" while the very next
|
||||
// render's `registry.get()` returns undefined. Naively falling
|
||||
// back to the snap leaves a ghost-running row that never clears;
|
||||
// dropping the row outright makes the agent disappear instantly
|
||||
// and the user loses the "what just finished?" beat. Synthesize
|
||||
// a terminal version so the 8s visibility window gives feedback
|
||||
// and then evicts the row cleanly.
|
||||
const ghost = agentEntry({
|
||||
agentId: 'ghost-1',
|
||||
subagentType: 'editor',
|
||||
description: 'editor: long-gone foreground task',
|
||||
status: 'running',
|
||||
});
|
||||
// Stub registry knows nothing about ghost-1 (simulates the
|
||||
// post-unregister state).
|
||||
const { config } = makeRegistryConfig([]);
|
||||
const { lastFrame } = renderPanel({ entries: [ghost], config });
|
||||
// First frame: synthesized completion, row still on screen.
|
||||
let frame = lastFrame() ?? '';
|
||||
expect(frame).toContain('editor');
|
||||
expect(frame).toContain('long-gone foreground task');
|
||||
// The synthesis sets status='completed', so the running tally
|
||||
// should be 0/1 and the bullet should switch to the success ✔.
|
||||
expect(frame).toContain('(0/1)');
|
||||
expect(frame).toContain('✔');
|
||||
// After the visibility window the row evicts and the panel hides.
|
||||
act(() => {
|
||||
vi.advanceTimersByTime(9000);
|
||||
});
|
||||
frame = lastFrame() ?? '';
|
||||
expect(frame).toBe('');
|
||||
});
|
||||
|
||||
it('still drops rows where snapshot is already terminal AND registry is empty', () => {
|
||||
// No useful state to keep showing — the snap is already terminal
|
||||
// (so the user already saw the result) and the registry forgot
|
||||
// about it (so we can't update activity / stats). Drop.
|
||||
const stale = agentEntry({
|
||||
agentId: 'stale-1',
|
||||
subagentType: 'researcher',
|
||||
description: 'researcher: stale terminal entry',
|
||||
status: 'completed',
|
||||
endTime: 0,
|
||||
});
|
||||
const { config } = makeRegistryConfig([]);
|
||||
const { lastFrame } = renderPanel({ entries: [stale], config });
|
||||
expect(lastFrame() ?? '').toBe('');
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -27,8 +27,9 @@
|
|||
*/
|
||||
|
||||
import type React from 'react';
|
||||
import { useContext, useEffect, useMemo, useState } from 'react';
|
||||
import { useContext, useEffect, useMemo, useRef, useState } from 'react';
|
||||
import { Box, Text } from 'ink';
|
||||
import { DEFAULT_BUILTIN_SUBAGENT_TYPE as CORE_DEFAULT_SUBAGENT_TYPE } from '@qwen-code/qwen-code-core';
|
||||
import { useBackgroundTaskViewState } from '../../contexts/BackgroundTaskViewContext.js';
|
||||
import { ConfigContext } from '../../contexts/ConfigContext.js';
|
||||
import { theme } from '../../semantic-colors.js';
|
||||
|
|
@ -61,12 +62,13 @@ const DEFAULT_MAX_ROWS = 5;
|
|||
// because the panel is denser and we have the dialog as the long-term
|
||||
// review surface.
|
||||
const TERMINAL_VISIBLE_MS = 8000;
|
||||
// `general-purpose` is the default builtin subagent; printing the type
|
||||
// every row when it's the default just clutters the line — the
|
||||
// description carries all the meaningful identity. Specialized
|
||||
// subagents (named in `subagents/builtin-agents.ts` or user-authored)
|
||||
// still get their type rendered as a bold anchor.
|
||||
const DEFAULT_SUBAGENT_TYPE = 'general-purpose';
|
||||
// Re-export under a panel-local alias so the source of truth stays
|
||||
// in `subagents/builtin-agents.ts` (a backend rename of the default
|
||||
// type would otherwise silently re-introduce the redundant
|
||||
// `general-purpose:` prefix on every row). Specialized subagents
|
||||
// (other builtins or user-authored types) still get their type
|
||||
// rendered as a bold anchor.
|
||||
const DEFAULT_SUBAGENT_TYPE = CORE_DEFAULT_SUBAGENT_TYPE;
|
||||
|
||||
type LivePanelEntry = AgentDialogEntry & {
|
||||
/** True when the row is past its terminal-visibility window. */
|
||||
|
|
@ -191,37 +193,84 @@ export const LiveAgentPanel: React.FC<LiveAgentPanelProps> = ({
|
|||
// BackgroundTasksDialog's detail body, which re-reads the registry
|
||||
// on its own activity tick.
|
||||
//
|
||||
// When `registry.get()` returns undefined the entry is gone (the
|
||||
// canonical case is a foreground subagent that unregistered after
|
||||
// its statusChange fired but before the next snapshot refresh —
|
||||
// `unregisterForeground` deletes silently). Drop the row instead
|
||||
// of falling back to `snap`: the snapshot still says `running`,
|
||||
// and that ghost would never clear because the registry has no
|
||||
// record left to flip it to terminal.
|
||||
// Three reconciliation paths between the snapshot and the registry:
|
||||
// 1. Both agree → use live (newest `recentActivities`).
|
||||
// 2. Snap says still-live (running / paused) but registry forgot
|
||||
// → most commonly a foreground subagent that finished:
|
||||
// `unregisterForeground` fires `emitStatusChange(entry)` BEFORE
|
||||
// it deletes the entry, so the snapshot captures the old
|
||||
// "still running" state and the next render's `registry.get`
|
||||
// returns undefined. Synthesize a terminal version with
|
||||
// `endTime = now` so the 8s visibility window gives the user
|
||||
// a "the agent finished" beat instead of either a ghost-running
|
||||
// row that never clears OR an instant disappearance the moment
|
||||
// the tool returns.
|
||||
// 3. Snap is already terminal but registry forgot → nothing useful
|
||||
// to keep showing; drop.
|
||||
//
|
||||
// When `config` itself is undefined (test fixtures that render
|
||||
// without ConfigContext) the panel degrades to snapshot-only —
|
||||
// there's no live source of truth to drop against, so the
|
||||
// snapshot is the best we have.
|
||||
// there's no live source of truth to reconcile against.
|
||||
//
|
||||
// NOTE: this useMemo MUST come before the `if (dialogOpen) return null`
|
||||
// early-return below — React's rules of hooks require hook calls in
|
||||
// identical order each render, so a conditional early-return that
|
||||
// skips a subsequent hook is a violation.
|
||||
// First-seen-missing timestamps for synthesized terminal entries.
|
||||
// We need this to survive across useMemo recomputes — without it,
|
||||
// each tick would re-synthesize the entry with a fresh `now` as
|
||||
// `endTime`, the visibility-window check (`now - endTime > 8000`)
|
||||
// would always evaluate to 0, and the row would never expire. The
|
||||
// ref outlives both the snapshot and the tick state.
|
||||
const missingSinceRef = useRef<Map<string, number>>(new Map());
|
||||
|
||||
const liveAgentSnapshots: AgentDialogEntry[] = useMemo(() => {
|
||||
const snapshots = entries.filter(isAgentEntry);
|
||||
if (!config) return snapshots;
|
||||
const registry = config.getBackgroundTaskRegistry();
|
||||
return snapshots
|
||||
// `now` participates in the dependency array so the memo recomputes
|
||||
// each tick and picks up `recentActivities` the registry mutated in
|
||||
// place via appendActivity. Reading it here makes the dependency
|
||||
// semantically honest — without this read a future "remove dead
|
||||
// dep" cleanup would silently freeze the panel on the first
|
||||
// tool-call after a snapshot refresh.
|
||||
const reconcileAt = now;
|
||||
const seenIds = new Set<string>();
|
||||
const next = snapshots
|
||||
.map((snap) => {
|
||||
seenIds.add(snap.agentId);
|
||||
const live = registry.get(snap.agentId);
|
||||
return live ? ({ ...live, kind: 'agent' as const } as const) : null;
|
||||
if (live) {
|
||||
// Recovered (or never went missing) — drop any stale
|
||||
// missing-since record so a future re-disappearance
|
||||
// gets a fresh timestamp.
|
||||
missingSinceRef.current.delete(snap.agentId);
|
||||
return { ...live, kind: 'agent' as const };
|
||||
}
|
||||
if (snap.status === 'running' || snap.status === 'paused') {
|
||||
// Pin the disappearance time on first observation so
|
||||
// subsequent ticks don't keep resetting endTime to `now`.
|
||||
let missingSince = missingSinceRef.current.get(snap.agentId);
|
||||
if (missingSince === undefined) {
|
||||
missingSince = reconcileAt;
|
||||
missingSinceRef.current.set(snap.agentId, missingSince);
|
||||
}
|
||||
return {
|
||||
...snap,
|
||||
status: 'completed' as const,
|
||||
endTime: snap.endTime ?? missingSince,
|
||||
};
|
||||
}
|
||||
return null;
|
||||
})
|
||||
.filter((e): e is AgentDialogEntry => e !== null);
|
||||
// `now` is a deliberate dep so the memo recomputes each tick and
|
||||
// captures the latest `recentActivities` mutated in place by the
|
||||
// registry's appendActivity path.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
// GC: drop missing-since records for agents that are no longer
|
||||
// even in the snapshot (e.g. statusChange refreshed and the
|
||||
// entry left useBackgroundTaskView's view entirely).
|
||||
for (const id of missingSinceRef.current.keys()) {
|
||||
if (!seenIds.has(id)) missingSinceRef.current.delete(id);
|
||||
}
|
||||
return next;
|
||||
}, [entries, config, now]);
|
||||
|
||||
// Defense in depth: don't compete with the dialog. Under
|
||||
|
|
|
|||
|
|
@ -7,6 +7,15 @@
|
|||
import { ToolDisplayNames, ToolNames } from '../tools/tool-names.js';
|
||||
import type { SubagentConfig } from './types.js';
|
||||
|
||||
/**
|
||||
* Canonical name of the default builtin subagent. Exported so UI
|
||||
* surfaces (e.g. `LiveAgentPanel`'s default-type elision) can compare
|
||||
* against the same source of truth instead of redeclaring the literal
|
||||
* — a rename here would otherwise silently break "skip the type
|
||||
* prefix when it's the default" logic.
|
||||
*/
|
||||
export const DEFAULT_BUILTIN_SUBAGENT_TYPE = 'general-purpose';
|
||||
|
||||
/**
|
||||
* Registry of built-in subagents that are always available to all users.
|
||||
* These agents are embedded in the codebase and cannot be modified or deleted.
|
||||
|
|
@ -16,7 +25,7 @@ export class BuiltinAgentRegistry {
|
|||
Omit<SubagentConfig, 'level' | 'filePath'>
|
||||
> = [
|
||||
{
|
||||
name: 'general-purpose',
|
||||
name: DEFAULT_BUILTIN_SUBAGENT_TYPE,
|
||||
description:
|
||||
'General-purpose agent for researching complex questions, searching for code, and executing multi-step tasks. When you are searching for a keyword or file and are not confident that you will find the right match in the first few tries use this agent to perform the search for you.',
|
||||
systemPrompt: `You are a general-purpose agent. Given the user's message, you should use the tools available to complete the task. Do what has been asked; nothing more, nothing less. When you complete the task, respond with a concise report covering what was done and any key findings — the caller will relay this to the user, so it only needs the essentials.
|
||||
|
|
|
|||
|
|
@ -26,7 +26,10 @@ export type {
|
|||
export { SubagentError } from './types.js';
|
||||
|
||||
// Built-in agents registry
|
||||
export { BuiltinAgentRegistry } from './builtin-agents.js';
|
||||
export {
|
||||
BuiltinAgentRegistry,
|
||||
DEFAULT_BUILTIN_SUBAGENT_TYPE,
|
||||
} from './builtin-agents.js';
|
||||
|
||||
// Validation system
|
||||
export { SubagentValidator } from './validation.js';
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue