Skip to content

Commit 2404e3a

Browse files
committed
fix: address PR review findings
1 parent 9e9ac0f commit 2404e3a

12 files changed

Lines changed: 234 additions & 100 deletions

File tree

apps/pythinker-code/src/tui/commands/registry.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,10 @@ export const BUILTIN_SLASH_COMMANDS = [
200200
description: 'Show or control the second-opinion advisor',
201201
priority: 95,
202202
completeArgs: advisorArgumentCompletions,
203-
availability: (args) => args.trim().toLowerCase() === 'status' ? 'always' : 'idle-only',
203+
availability: (args) => {
204+
const verb = args.trim().toLowerCase();
205+
return verb === '' || verb === 'status' ? 'always' : 'idle-only';
206+
},
204207
},
205208
{
206209
name: 'provider',

apps/pythinker-code/src/tui/components/chrome/transcript-container.ts

Lines changed: 28 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,6 @@ import {
1010

1111
export type { TranscriptChildMetadata, TranscriptChildRole } from '../../utils/transcript-component-metadata';
1212

13-
interface RenderedChild {
14-
readonly child: Component;
15-
readonly metadata: TranscriptChildMetadata;
16-
readonly rows: readonly string[];
17-
}
18-
1913
export class TranscriptContainer extends GutterContainer {
2014
private readonly leftGutter: number;
2115
private readonly rightGutter: number;
@@ -66,78 +60,58 @@ export class TranscriptContainer extends GutterContainer {
6660
throw new Error('Transcript child was added without metadata');
6761
}
6862
const inner = Math.max(1, width - this.leftGutter - this.rightGutter);
69-
const following = this.children.slice(index + 1).map((followingChild) => {
63+
let rows = 0;
64+
let previousDurable = isDurable(metadata.role);
65+
for (let childIndex = index + 1; childIndex < this.children.length; childIndex += 1) {
66+
const followingChild = this.children[childIndex]!;
7067
const followingMetadata = getTranscriptChildMetadata(followingChild);
7168
if (followingMetadata === undefined) {
7269
throw new Error('Transcript child was added without metadata');
7370
}
74-
return {
75-
child: followingChild,
76-
metadata: followingMetadata,
77-
rows: this.normalizeRows(followingChild.render(inner), followingMetadata),
78-
};
79-
});
80-
const firstVisible = following.find((segment) => segment.rows.length > 0);
81-
const separator =
82-
firstVisible !== undefined &&
83-
isDurable(metadata.role) &&
84-
isDurable(firstVisible.metadata.role)
85-
? 1
86-
: 0;
87-
return separator + this.rowsForSegments(following).length;
71+
const followingRows = this.normalizeRows(
72+
followingChild.render(inner),
73+
followingMetadata,
74+
);
75+
if (followingRows.length === 0) continue;
76+
if (previousDurable && isDurable(followingMetadata.role)) rows += 1;
77+
rows += followingRows.length;
78+
previousDurable = isDurable(followingMetadata.role);
79+
}
80+
return rows;
8881
}
8982

9083
override render(width: number): string[] {
91-
return this.rowsForSegments(this.renderedChildren(width)).map((row) => {
92-
return ' '.repeat(this.leftGutter) + row;
93-
});
94-
}
95-
96-
97-
private renderedChildren(width: number): RenderedChild[] {
9884
const inner = Math.max(1, width - this.leftGutter - this.rightGutter);
99-
return this.children.map((child) => {
85+
const lead = ' '.repeat(this.leftGutter);
86+
const rows: string[] = [];
87+
let hasVisible = false;
88+
let previousDurable = false;
89+
for (const child of this.children) {
10090
const metadata = getTranscriptChildMetadata(child);
10191
if (metadata === undefined) {
10292
throw new Error('Transcript child was added without metadata');
10393
}
104-
return {
105-
child,
106-
metadata,
107-
rows: this.normalizeRows(child.render(inner), metadata),
108-
};
109-
});
94+
const childRows = this.normalizeRows(child.render(inner), metadata);
95+
if (childRows.length === 0) continue;
96+
if (hasVisible && previousDurable && isDurable(metadata.role)) rows.push(lead);
97+
for (const row of childRows) rows.push(lead + row);
98+
hasVisible = true;
99+
previousDurable = isDurable(metadata.role);
100+
}
101+
return rows;
110102
}
111103

112104
private normalizeRows(
113105
rows: readonly string[],
114106
metadata: TranscriptChildMetadata,
115107
): readonly string[] {
116-
if (metadata.edgeBlankPolicy === 'preserve') return [...rows];
108+
if (metadata.edgeBlankPolicy === 'preserve') return rows;
117109
let start = 0;
118110
let end = rows.length;
119111
while (start < end && isPlainBlank(rows[start]!)) start += 1;
120112
while (end > start && isPlainBlank(rows[end - 1]!)) end -= 1;
121113
return rows.slice(start, end);
122114
}
123-
124-
private rowsForSegments(segments: readonly RenderedChild[]): string[] {
125-
const rows: string[] = [];
126-
const visibleSegments = segments.filter((segment) => segment.rows.length > 0);
127-
for (let index = 0; index < visibleSegments.length; index += 1) {
128-
const segment = visibleSegments[index]!;
129-
rows.push(...segment.rows);
130-
const next = visibleSegments[index + 1];
131-
if (
132-
next !== undefined &&
133-
isDurable(segment.metadata.role) &&
134-
isDurable(next.metadata.role)
135-
) {
136-
rows.push('');
137-
}
138-
}
139-
return rows;
140-
}
141115
}
142116

143117
function isPlainBlank(value: string): boolean {

apps/pythinker-code/test/tui/commands/advisor.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ describe('handleAdvisorCommand', () => {
7878
const { host, advisor, securityStatus } = makeHost();
7979
advisor.setEnabled.mockResolvedValueOnce([{ ...securityStatus, enabled: false }]);
8080

81-
await handleAdvisorCommand(host, 'off security');
81+
await handleAdvisorCommand(host, 'toggle security');
8282

8383
expect(advisor.setEnabled).toHaveBeenCalledWith(false, 'security');
8484
expect(host.showStatus).toHaveBeenCalledWith('Advisor security disabled.');

apps/pythinker-code/test/tui/commands/registry.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,15 @@ describe('built-in slash command registry', () => {
100100
{ value: 'status', label: 'status', description: 'Show Fast mode status' },
101101
]);
102102
});
103+
it('keeps advisor status and the omitted verb available while busy', () => {
104+
const advisor = findBuiltInSlashCommand('advisor');
105+
expect(advisor).toBeDefined();
106+
expect(resolveSlashCommandAvailability(advisor!, '')).toBe('always');
107+
expect(resolveSlashCommandAvailability(advisor!, 'status')).toBe('always');
108+
expect(resolveSlashCommandAvailability(advisor!, 'on')).toBe('idle-only');
109+
expect(resolveSlashCommandAvailability(advisor!, 'off')).toBe('idle-only');
110+
});
111+
103112

104113
it('marks plan clear as idle-only while normal plan toggles are always available', () => {
105114
const plan = findBuiltInSlashCommand('plan');

apps/pythinker-code/test/tui/components/chrome/transcript-container.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,15 +42,15 @@ describe('TranscriptContainer', () => {
4242

4343
it('preserves ANSI blank rows and does not invent gaps around ephemeral children', () => {
4444
const container = new TranscriptContainer(1, 1);
45-
const first = new StubLines(['', '\u001b[48;5;1m \u001b[0m', 'first', '']);
45+
const first = new StubLines(['', '\u001B[48;5;1m \u001B[0m', 'first', '']);
4646
const status = new StubLines(['status']);
4747
const second = new StubLines(['', 'second']);
4848

4949
container.addTranscriptChild(first, durable);
5050
container.addTranscriptChild(status, ephemeral);
5151
container.addTranscriptChild(second, durable);
5252
expect(container.render(20)).toEqual([
53-
' \u001b[48;5;1m \u001b[0m',
53+
' \u001B[48;5;1m \u001B[0m',
5454
' first',
5555
' status',
5656
' second',

apps/pythinker-code/test/tui/components/messages/dynamic-workflow-mission-control.test.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from '#/tui/components/messages/dynamic-workflow-mission-control';
1010
import {
1111
BRAILLE_SPINNER_FRAMES,
12+
DYNAMIC_WORKFLOW_RENDERING,
1213
BRAILLE_SPINNER_INTERVAL_MS,
1314
} from '#/tui/constant/rendering';
1415
import { currentTheme, darkColors } from '#/tui/theme';
@@ -674,12 +675,8 @@ describe('DynamicWorkflowMissionControlComponent', () => {
674675
component.registerSubagent({ agentId: 'agent-1' });
675676
component.markStarted('agent-1');
676677

677-
for (const [time, glyph] of [
678-
[0, BRAILLE_SPINNER_FRAMES[0]],
679-
[300, BRAILLE_SPINNER_FRAMES[1]],
680-
[600, BRAILLE_SPINNER_FRAMES[2]],
681-
[900, BRAILLE_SPINNER_FRAMES[3]],
682-
] as const) {
678+
for (const [index, glyph] of BRAILLE_SPINNER_FRAMES.slice(0, 4).entries()) {
679+
const time = index * DYNAMIC_WORKFLOW_RENDERING.progressFrameMs;
683680
vi.setSystemTime(time);
684681
const line = component.render(100).find((candidate) => strip(candidate).includes('001'));
685682
expect(line).toContain(chalk.hex(darkColors.primary)(glyph));

packages/agent-core/src/agent/compaction/micro.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,15 +94,18 @@ export class MicroCompaction {
9494
}
9595
}
9696

97-
compact(messages: readonly ContextMessage[]): readonly ContextMessage[] {
97+
compact(
98+
messages: readonly ContextMessage[],
99+
offset = 0,
100+
): readonly ContextMessage[] {
98101
if (!this.agent.experimentalFlags.enabled('micro_compaction')) return messages;
99102

100103
const config = this.config;
101104
const result: ContextMessage[] = [];
102105
let i = 0;
103106
for (const msg of messages) {
104107
if (
105-
i < this.cutoff &&
108+
i + offset < this.cutoff &&
106109
msg.role === 'tool' &&
107110
msg.toolCallId !== undefined &&
108111
estimateTokensForContentParts(msg.content) >= config.minContentTokens

packages/agent-core/src/agent/context/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,8 +290,8 @@ export class ContextMemory {
290290
return this._historyRevision;
291291
}
292292

293-
project(messages: readonly ContextMessage[]): Message[] {
294-
return project(this.agent.microCompaction.compact(messages));
293+
project(messages: readonly ContextMessage[], offset = 0): Message[] {
294+
return project(this.agent.microCompaction.compact(messages, offset));
295295
}
296296

297297
get messages(): Message[] {

packages/agent-core/src/session/advisor-config.ts

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { homedir } from 'node:os';
33
import path from 'node:path';
44
import { load as loadYaml } from 'js-yaml';
55
import { isPlainRecord } from '../agent/turn/canonical-args';
6+
import { findProjectRoot } from '../skill/scanner';
67

78
export interface AdvisorConfigEntry {
89
readonly name: string;
@@ -57,8 +58,8 @@ interface AdvisorConfigDocumentEntry {
5758
export function slugifyAdvisorName(name: string): string {
5859
const slug = name
5960
.toLowerCase()
60-
.replace(/[^a-z0-9]+/gu, '-')
61-
.replace(/^-+|-+$/gu, '');
61+
.replaceAll(/[^a-z0-9]+/gu, '-')
62+
.replaceAll(/^-+|-+$/gu, '');
6263
return slug.length === 0 ? 'advisor' : slug;
6364
}
6465

@@ -189,41 +190,55 @@ async function collectConfigCandidates(
189190
}
190191
}
191192

193+
const projectRoot = await findProjectRoot(resolvedCwd);
192194
const projectDirs: string[] = [];
193195
let current = resolvedCwd;
194196
while (true) {
195197
projectDirs.push(current);
198+
if (current === projectRoot) break;
196199
const parent = path.dirname(current);
197200
if (parent === current) break;
198201
current = parent;
199202
}
200203
projectDirs.reverse();
201204
for (const [depth, directory] of projectDirs.entries()) {
202205
for (const fileName of fileNames) {
203-
candidates.push({ path: path.join(directory, fileName), user: false, depth });
204-
candidates.push({ path: path.join(directory, '.omp', fileName), user: false, depth });
206+
candidates.push(
207+
{ path: path.join(directory, fileName), user: false, depth },
208+
{ path: path.join(directory, '.omp', fileName), user: false, depth },
209+
);
205210
}
206211
}
207212

208213
const unique = new Map<string, (typeof candidates)[number]>();
209214
for (const candidate of candidates) unique.set(path.resolve(candidate.path), candidate);
215+
const results = await Promise.all(
216+
[...unique.values()].map(async (candidate) => {
217+
try {
218+
const content = await readFile(candidate.path, 'utf8');
219+
return { candidate, content };
220+
} catch (error) {
221+
if (isMissingFile(error)) return undefined;
222+
return { candidate, error };
223+
}
224+
}),
225+
);
210226
const readable: ConfigCandidate[] = [];
211-
for (const candidate of unique.values()) {
212-
try {
213-
const content = await readFile(candidate.path, 'utf8');
214-
readable.push({
215-
...candidate,
216-
path: path.resolve(candidate.path),
217-
fileName: path.basename(candidate.path),
218-
content,
219-
});
220-
} catch (error) {
221-
if (isMissingFile(error)) continue;
227+
for (const result of results) {
228+
if (result === undefined) continue;
229+
if ('error' in result) {
222230
onWarning('Advisor config could not be read', {
223-
path: candidate.path,
224-
error: error instanceof Error ? error.message : String(error),
231+
path: result.candidate.path,
232+
error: result.error instanceof Error ? result.error.message : String(result.error),
225233
});
234+
continue;
226235
}
236+
readable.push({
237+
...result.candidate,
238+
path: path.resolve(result.candidate.path),
239+
fileName: path.basename(result.candidate.path),
240+
content: result.content,
241+
});
227242
}
228243
readable.sort((left, right) => {
229244
if (left.user !== right.user) return left.user ? -1 : 1;

0 commit comments

Comments
 (0)