Skip to content

Commit bb9b112

Browse files
committed
fix: address review findings on fork, session list, tower, wire, and transcript
- fork: route session lookup and confirmation errors through the command error path; drop redundant undefined unions - session list: reject malformed --limit values such as 2x and 1.5 - git status parser: consume the origin path for unstaged renames/copies - tower: validate the persisted base branch; treat completed missions as not needing a replacement worker; document /tower <base> activation - wire: reject dot segments as legacy plan ids before migrating the key - session fork: stamp updatedAt with the fork time so new forks sort first - transcript: register the interruption marker; correct the plan directory and MCP forwarding docs - tests: make acp success-path exit assertions discriminating; assert debounce timer cleanup directly; type telemetry stubs without casts
1 parent cfec5ed commit bb9b112

16 files changed

Lines changed: 102 additions & 52 deletions

File tree

‎apps/pythinker-code/src/cli/sub/fork.ts‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ interface WritableLike {
2626

2727
export interface ForkedSessionResult {
2828
readonly id: string;
29-
readonly title?: string | undefined;
29+
readonly title?: string;
3030
}
3131

3232
export interface ForkDeps {
@@ -41,34 +41,34 @@ export interface ForkDeps {
4141

4242
export interface ForkOptions {
4343
readonly yes: boolean;
44-
readonly cwd?: string | undefined;
44+
readonly cwd?: string;
4545
}
4646

4747
export async function handleFork(
4848
deps: ForkDeps,
4949
sessionId: string | undefined,
5050
opts: ForkOptions,
5151
): Promise<void> {
52-
let resolvedId = normalizeOptionalSessionId(sessionId);
53-
if (resolvedId === undefined) {
54-
const sessions = await deps.listSessions(opts.cwd ?? deps.cwd());
55-
const latest = sessions[0];
56-
if (latest === undefined) {
57-
deps.stderr.write('No previous session found to fork.\n');
58-
return deps.exit(1);
59-
}
60-
if (!opts.yes) {
61-
const confirmed = await deps.confirmPreviousSession(latest);
62-
if (!confirmed) {
63-
deps.stdout.write('Fork cancelled.\n');
64-
return;
52+
try {
53+
let resolvedId = normalizeOptionalSessionId(sessionId);
54+
if (resolvedId === undefined) {
55+
const sessions = await deps.listSessions(opts.cwd ?? deps.cwd());
56+
const latest = sessions[0];
57+
if (latest === undefined) {
58+
deps.stderr.write('No previous session found to fork.\n');
59+
return deps.exit(1);
60+
}
61+
if (!opts.yes) {
62+
const confirmed = await deps.confirmPreviousSession(latest);
63+
if (!confirmed) {
64+
deps.stdout.write('Fork cancelled.\n');
65+
return;
66+
}
6567
}
68+
resolvedId = latest.id;
6669
}
67-
resolvedId = latest.id;
68-
}
6970

70-
const startedAt = Date.now();
71-
try {
71+
const startedAt = Date.now();
7272
const forked = await deps.forkSession(resolvedId);
7373
const elapsedMs = Date.now() - startedAt;
7474
const title = forked.title === undefined ? '' : ` ("${forked.title}")`;

‎apps/pythinker-code/src/cli/sub/session.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ function formatRow(summary: SessionSummary, showWorkDir: boolean): string {
135135
}
136136

137137
function sanitizeField(value: string): string {
138-
return value.replaceAll(/[\x00-\x1F\x7F]+/g, ' ').trim();
138+
return value.replaceAll(/[\u0000-\u001F\u007F]+/g, ' ').trim();
139139
}
140140

141141
function formatTimestamp(epochMs: number): string {
@@ -145,8 +145,8 @@ function formatTimestamp(epochMs: number): string {
145145
}
146146

147147
function parseLimitOption(value: string): number {
148-
const parsed = Number.parseInt(value, 10);
149-
if (!Number.isFinite(parsed) || parsed <= 0) {
148+
const parsed = /^\d+$/.test(value) ? Number(value) : Number.NaN;
149+
if (!Number.isSafeInteger(parsed) || parsed <= 0) {
150150
throw new Error(`--limit must be a positive integer, got "${value}"`);
151151
}
152152
return parsed;

‎apps/pythinker-code/test/cli/acp.test.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,20 +57,22 @@ describe('pythinker acp', () => {
5757
const program = new Command('pythinker').exitOverride();
5858
registerAcpCommand(program);
5959

60-
await expect(program.parseAsync(['node', 'pythinker', 'acp'])).rejects.toThrow(ExitCalled);
60+
exitSpy.mockImplementation((() => undefined) as never);
61+
await program.parseAsync(['node', 'pythinker', 'acp']);
6162

6263
expect(runAcpServer).toHaveBeenCalledTimes(1);
6364
expect(vi.mocked(runAcpServer).mock.calls[0]?.[0]).toEqual(
6465
expect.objectContaining({ homeDir: getDataDir() }),
6566
);
66-
expect(exitSpy).toHaveBeenCalledWith(0);
67+
expect(exitSpy.mock.calls).toEqual([[0]]);
6768
});
6869

6970
it('invokes runAcpServer with the v2 host options and exits 0 on success', async () => {
7071
const program = new Command('pythinker').exitOverride();
7172
registerAcpCommand(program);
7273

73-
await expect(program.parseAsync(['node', 'pythinker', 'acp'])).rejects.toThrow(ExitCalled);
74+
exitSpy.mockImplementation((() => undefined) as never);
75+
await program.parseAsync(['node', 'pythinker', 'acp']);
7476

7577
expect(runAcpServer).toHaveBeenCalledTimes(1);
7678
const optsArg = vi.mocked(runAcpServer).mock.calls[0]?.[0];
@@ -80,7 +82,7 @@ describe('pythinker acp', () => {
8082
agentInfo: { name: 'Pythinker Code CLI', version: expect.any(String) },
8183
}),
8284
);
83-
expect(exitSpy).toHaveBeenCalledWith(0);
85+
expect(exitSpy.mock.calls).toEqual([[0]]);
8486
});
8587

8688
it('uses PYTHINKER_CODE_HOME as homeDir when set', async () => {
@@ -90,7 +92,8 @@ describe('pythinker acp', () => {
9092
const program = new Command('pythinker').exitOverride();
9193
registerAcpCommand(program);
9294

93-
await expect(program.parseAsync(['node', 'pythinker', 'acp'])).rejects.toThrow(ExitCalled);
95+
exitSpy.mockImplementation((() => undefined) as never);
96+
await program.parseAsync(['node', 'pythinker', 'acp']);
9497

9598
const optsArg = vi.mocked(runAcpServer).mock.calls[0]?.[0];
9699
expect(optsArg).toEqual(

‎apps/pythinker-code/test/cli/session.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,14 +171,14 @@ describe('registerSessionCommand', () => {
171171
expect(captured.exitCode).toBeUndefined();
172172
});
173173

174-
it('rejects a non-numeric --limit', async () => {
174+
it.each(['abc', '2x', '1.5', '0', '-1'])('rejects a malformed --limit %s', async (limit) => {
175175
const { deps } = stubDeps([]);
176176
const program = new Command('pythinker');
177177
program.exitOverride();
178178
registerSessionCommand(program, deps);
179179

180180
await expect(
181-
program.parseAsync(['node', 'pythinker', 'session', 'list', '--limit', 'abc']),
181+
program.parseAsync(['node', 'pythinker', 'session', 'list', '--limit', limit]),
182182
).rejects.toThrow(/positive integer/);
183183
});
184184
});

‎apps/vscode/test/webview/useDebouncedValue.test.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
import { renderHook, waitFor } from '@testing-library/react';
2-
import { describe, expect, it } from 'vitest';
2+
import { afterEach, describe, expect, it, vi } from 'vitest';
33
import { useDebouncedValue } from '@/hooks/useDebouncedValue';
44

55
describe('useDebouncedValue', () => {
6+
afterEach(() => {
7+
vi.restoreAllMocks();
8+
vi.useRealTimers();
9+
});
10+
611
it('returns the initial value immediately without debounce', () => {
712
const { result } = renderHook(() => useDebouncedValue('a', 100));
813
expect(result.current).toBe('a');
@@ -28,11 +33,17 @@ describe('useDebouncedValue', () => {
2833
await waitFor(() => { expect(result.current).toBe('abc'); });
2934
});
3035

31-
it('does not update state after unmount', async () => {
32-
const { result, rerender, unmount } = renderHook(({ value }) => useDebouncedValue(value, 100), { initialProps: { value: 'a' } });
36+
it('clears the pending timer on unmount', () => {
37+
vi.useFakeTimers();
38+
const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout');
39+
const { rerender, unmount } = renderHook(({ value }) => useDebouncedValue(value, 100), { initialProps: { value: 'a' } });
3340
rerender({ value: 'ab' });
41+
const pending = vi.getTimerCount();
42+
expect(pending).toBeGreaterThan(0);
43+
3444
unmount();
35-
await new Promise((r) => setTimeout(r, 150));
36-
expect(result.current).toBe('a');
45+
46+
expect(clearTimeoutSpy).toHaveBeenCalled();
47+
expect(vi.getTimerCount()).toBe(pending - 1);
3748
});
3849
});

‎docs/reference/pythinker-acp.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ All methods not listed above return `methodNotFound`.
8383

8484
## MCP forwarding
8585

86-
When an ACP client provides `mcpServers` in `session/new` or `session/load`, the ACP server performs the following conversions:
86+
When an ACP client provides `mcpServers` in `session/new`, `session/load`, or `session/resume`, the ACP server performs the following conversions:
8787

8888
- `http` → Pythinker's `transport: 'http'` configuration
8989
- `stdio` → Pythinker's `transport: 'stdio'` configuration

‎packages/agent-core-v2/src/app/git/gitParsers.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export function parsePorcelain(
2525
const xy = record.slice(0, 2);
2626
const wirePath = record.slice(3);
2727

28-
if (xy.startsWith('R') || xy.startsWith('C')) {
28+
if (xy.includes('R') || xy.includes('C')) {
2929
i++;
3030
}
3131
if (filter !== undefined && !filter.has(wirePath)) continue;

‎packages/agent-core-v2/src/features/tower/tools/init/init.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ Initialize a tower multi-agent workspace in the current repository.
22

33
Creates the .tower/ directory (comms state, inbox, findings, reviews, missions, activity log, worktree slots) that the full tower tool set operates on (TowerPlan/TowerSpawn/TowerMerge/TowerTeardown plus the shared TowerSend/TowerInbox/TowerFinding/TowerReview/TowerMission/TowerStatus).
44

5-
Tower mode must already be active before you call this — only the user can enable it, with `/tower on`; the agent can never enter tower mode by itself. While the mode is off this tool refuses: ask the user to turn tower mode on first. Use tower only when a task is large enough to split across multiple parallel agents with isolated git worktrees and a review-gated merge protocol.
5+
Tower mode must already be active before you call this — only the user can enable it, with `/tower on` or `/tower <base>`; the agent can never enter tower mode by itself. While the mode is off this tool refuses: ask the user to turn tower mode on first. Use tower only when a task is large enough to split across multiple parallel agents with isolated git worktrees and a review-gated merge protocol.
66

77
Safe to call again — an existing workspace is reported, never reset. Re-entering from a new CLI session adopts the workspace: roster entries the previous session spawned are retired (their agent ids cannot be resumed across sessions), while missions, worktrees, and the activity log carry over.
88

‎packages/agent-core-v2/src/features/tower/tools/status/statusTool.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ function renderDeathWarnings(state: TowerState): string[] {
183183
const lines: string[] = [];
184184
for (const mission of state.missions) {
185185
if (mission.owner === undefined) continue;
186-
if (mission.status === 'merged' || mission.status === 'abandoned') continue;
186+
if (mission.status === 'completed' || mission.status === 'merged' || mission.status === 'abandoned') continue;
187187
const entry = deadByName.get(mission.owner);
188188
if (entry === undefined) continue;
189189
lines.push(

‎packages/agent-core-v2/src/features/tower/towerOps.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ export const towerOwnerKey = defineState('tower.owner', () => undefined as strin
5454

5555
export const towerBaseKey = defineState('tower.base', (): string | null => null)
5656
.replayable({
57-
schema: z.custom<string | null>(),
57+
schema: z.string().nullable(),
5858
})
5959
.on(TowerModeEnter, (_s, e) => e.base ?? null)
6060
.on(TowerModeExit, () => null);

0 commit comments

Comments
 (0)