Skip to content

Commit e6778dc

Browse files
authored
fix: harden journal repair, task detach, subagent prompts, and login cancel (#221)
## Related Issue No linked issue — maintainer-authored reliability and API work. ## Problem Five separate gaps: - A wire journal repair that fails (disk full, backup write refused) left the journal corrupted, and the next `appendRecord` wrote behind the corrupted tail. Callers of `flush()` saw success. - Subagent turns opened in the transcript with no prompt, so the reader could not see what the subagent was asked to do. - A running foreground task could only be cancelled, never moved to the background, over the server API. - In the VS Code extension, two concurrent opens of the same session for one view each wrapped the SDK session, so every streamed event was delivered twice. - Denying the OpenAI Codex consent page ended as `OpenAI Codex login failed` after first prompting for the redirect URL, and the web sign-in page waited out its full TTL. ## What changed - `repairWireJournal` reports `repaired` / `failed`. `WireService` keeps the failed repair pending, retries it before the next append, and rethrows a `RECORDS_WRITE_FAILED` `WireError` from `flush()` when it still fails, so nothing is appended behind a corrupted tail. - `turn.started` now carries the prompt for `system_trigger` / `subagent` origins, and the cold-path grouping folds the opening input onto that turn. Search extraction still filters those messages out. - New `POST /api/v1/sessions/{session_id}/tasks/{task_id}:detach` action, answering `{ detached, status }`. The bash and subagent tools report a user-initiated detach distinctly from a timeout detach, and process tasks carry their parent tool call id. - `PythinkerRuntime` serializes open / attach / detach per webview id, so concurrent callers cannot both miss the session map. - The OpenAI Codex callback distinguishes `error=access_denied` from every other dead end. The CLI reports it as cancelled, and `CodexLoginFlow` moves the attempt to `cancelled` with a reason instead of waiting for expiry — the web sign-in page already renders that state. ## Checklist - [x] I have read the [CONTRIBUTING](https://github.com/PyModel/pythinker-code/blob/main/CONTRIBUTING.md) document. - [ ] I have linked a related issue (external PRs: the issue must have a maintainer's `/approve`). - [x] I have added tests that prove my feature works. - [x] Ran `gen-changesets` skill, or this PR needs no changeset. - [x] Ran `gen-docs` skill, or this PR needs no doc update. ## Risk - Risk level: medium. Boundaries touched: session journal persistence, the tasks REST surface, the VS Code session runtime, and the OAuth callback. - The journal change makes a previously silent failure loud: `flush()` can now reject where it used to resolve. That is the intent — the alternative is losing the record. - The detach action is additive; `:cancel` keeps its behavior and its error codes. - No new dependency and no lockfile change. ## Verification All exited 0: - `pnpm run typecheck` - `pnpm run lint` (0 errors, pre-existing warnings only) - `pnpm test` — 1,218 files passed, 12 skipped; 20,522 tests passed, 3 expected fail, 79 skipped, 2 todo - `pnpm run build`, `pnpm run build:web`, `pnpm run check:web` - `pnpm sherif`, `node scripts/check-nix-workspace.mjs` (18 workspaces) - `pnpm -C apps/vscode run typecheck`, `pnpm -C apps/vscode test` (18 files / 369 tests) Each new guard was proven by mutation — reverting it fails the test that covers it: - dropping the `system_trigger`/`subagent` case from `isDisplayablePromptOrigin` → 1 failure in `loop.test.ts` - collapsing the per-view serialization → 3 failures in `pythinker-runtime.test.ts` - settling the callback with `null` instead of the denial → 1 failure in `openai-codex-oauth.test.ts` - folding the denial back into the timeout branch → 1 failure in `codexLogin.test.ts` `packages/transcript` and `packages/protocol` are web build inputs, so `dist-web` was rebuilt; only `.web-bundle-manifest.json` changed. ## Rollback and review - Rollback: revert the single commit; every change is additive or behind a new branch. - Residual risk: the journal `flush()` rejection is a new failure surface for callers that previously ignored a silent loss. - Human review wanted on the journal repair retry and the tasks route split. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Foreground tasks can now be detached into the background. - Background tasks and pending questions appear in the client’s background-task panel. - Subagent prompts and attachments display correctly in transcripts. - Task details can retain parent tool-call information. - **Bug Fixes** - Prevented duplicated streaming output during concurrent session opens. - Improved recovery from session journal write failures. - Codex sign-in denials now appear as cancelled. - Improved task detachment behavior and status reporting. - **Documentation** - Documented task detachment and updated background-task guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 6fb46b0 commit e6778dc

44 files changed

Lines changed: 957 additions & 67 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Report a denied OpenAI Codex sign-in as cancelled instead of asking for the redirect URL.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Show the prompt that started a subagent turn in the transcript.

.changeset/task-detach-action.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": minor
3+
---
4+
5+
Add a task detach action to the server API. Call `POST /api/v1/sessions/{session_id}/tasks/{task_id}:detach` to move a running foreground task to the background.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"pythinker": patch
3+
---
4+
5+
Fix duplicated streaming output when a session is opened twice at the same time.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Retry a failed session journal repair before writing new records, so no message is appended behind a corrupted tail.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
{
2-
"sourceHash": "d84e17f04092f5fb9afa9f4d323b614d3945ec8e33abbbc92f5793ad2c30959e",
2+
"sourceHash": "aa9ad464f74ddd042d5435d29ed27bac364c25274ede0eade5d39cc7593d0eb6",
33
"sourceFileCount": 404
44
}

apps/vscode/src/runtime/pythinker-runtime.ts

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export class PythinkerRuntime {
4848
private readonly log: PythinkerRuntimeOptions["log"];
4949
private readonly sessions = new Map<string, SessionRuntime>();
5050
private readonly sessionByView = new Map<string, string>();
51+
private readonly viewChains = new Map<string, Promise<void>>();
5152
private readonly pendingPermissionByView = new Map<string, PermissionMode>();
5253
private closed = false;
5354

@@ -101,6 +102,10 @@ export class PythinkerRuntime {
101102
}
102103

103104
async openSession(options: OpenSessionOptions): Promise<SessionRuntime> {
105+
return this.serializeView(options.webviewId, () => this.openSessionInner(options));
106+
}
107+
108+
private async openSessionInner(options: OpenSessionOptions): Promise<SessionRuntime> {
104109
this.ensureOpen();
105110
const current = this.getSessionForView(options.webviewId);
106111
const requestedId = options.sessionId ?? current?.id;
@@ -119,7 +124,7 @@ export class PythinkerRuntime {
119124
if (runtime !== undefined) {
120125
assertSessionWorkDir(runtime.session, options.workDir);
121126
await applySessionPermission(runtime.session, runtime.permissionMode);
122-
await this.detachView(options.webviewId);
127+
await this.detachViewInner(options.webviewId);
123128
} else {
124129
const seedMode = defaultPermissionMode(options.yoloMode);
125130
const session =
@@ -135,7 +140,7 @@ export class PythinkerRuntime {
135140
try {
136141
assertSessionWorkDir(session, options.workDir);
137142
const mode = await restorePermissionMode(session, seedMode);
138-
await this.detachView(options.webviewId);
143+
await this.detachViewInner(options.webviewId);
139144
runtime = this.wrapSession(session, mode);
140145
} catch (error) {
141146
await session.close().catch((closeError: unknown) => {
@@ -156,14 +161,24 @@ export class PythinkerRuntime {
156161
webviewId: string,
157162
session: Session,
158163
yoloModeSetting = false,
164+
): Promise<SessionRuntime> {
165+
return this.serializeView(webviewId, () =>
166+
this.attachResumedSessionInner(webviewId, session, yoloModeSetting),
167+
);
168+
}
169+
170+
private async attachResumedSessionInner(
171+
webviewId: string,
172+
session: Session,
173+
yoloModeSetting: boolean,
159174
): Promise<SessionRuntime> {
160175
const existing = this.sessions.get(session.id);
161176
if (existing !== undefined && this.sessionByView.get(webviewId) === session.id) {
162177
existing.subscribe(webviewId);
163178
await existing.announceStatus(webviewId);
164179
return existing;
165180
}
166-
await this.detachView(webviewId);
181+
await this.detachViewInner(webviewId);
167182
let runtime = existing ?? this.sessions.get(session.id);
168183
if (runtime === undefined) {
169184
try {
@@ -184,6 +199,10 @@ export class PythinkerRuntime {
184199
}
185200

186201
async detachView(webviewId: string): Promise<void> {
202+
return this.serializeView(webviewId, () => this.detachViewInner(webviewId));
203+
}
204+
205+
private async detachViewInner(webviewId: string): Promise<void> {
187206
const id = this.sessionByView.get(webviewId);
188207
if (id === undefined) return;
189208
this.sessionByView.delete(webviewId);
@@ -196,6 +215,23 @@ export class PythinkerRuntime {
196215
}
197216
}
198217

218+
// A view attaches to at most one session, so opens/detaches for one view
219+
// must never overlap: concurrent callers that both miss `this.sessions`
220+
// would wrap the same SDK session twice and double every streamed event.
221+
private serializeView<T>(webviewId: string, work: () => Promise<T>): Promise<T> {
222+
const prev = this.viewChains.get(webviewId) ?? Promise.resolve();
223+
const run = prev.then(work, work);
224+
const next = run.then(
225+
() => undefined,
226+
() => undefined,
227+
);
228+
this.viewChains.set(webviewId, next);
229+
void next.finally(() => {
230+
if (this.viewChains.get(webviewId) === next) this.viewChains.delete(webviewId);
231+
});
232+
return run;
233+
}
234+
199235
async closeSession(id: string): Promise<void> {
200236
const runtime = this.sessions.get(id);
201237
if (runtime === undefined) {

apps/vscode/test/pythinker-runtime.test.ts

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,91 @@ describe("Pythinker runtime (owns shared SDK sessions for Webviews)", () => {
382382
expect(boundary.handlerInstallations).toEqual({ approval: 1, question: 1 });
383383
});
384384

385+
it("does not double-wrap the SDK session when two opens race for it", async () => {
386+
const sdk = createFakeHarness();
387+
const broadcasts: { event: string; data: unknown; webviewId?: string }[] = [];
388+
const runtime = new PythinkerRuntime({
389+
version: "0.6.0",
390+
harness: sdk.harness,
391+
broadcast: (event, data, webviewId) => {
392+
broadcasts.push({ event, data, webviewId });
393+
},
394+
captureBaseline: () => undefined,
395+
log: () => undefined,
396+
});
397+
const boundary = sdk.addSession("saved-1", "/workspace");
398+
399+
const [first, second] = await Promise.all([
400+
runtime.openSession(openOptions({ sessionId: "saved-1" })),
401+
runtime.openSession(openOptions({ sessionId: "saved-1" })),
402+
]);
403+
404+
expect(second).toBe(first);
405+
expect(boundary.subscriptionCount()).toBe(1);
406+
407+
boundary.emit({
408+
type: "assistant.delta",
409+
sessionId: "saved-1",
410+
agentId: "main",
411+
turnId: 1,
412+
delta: "Hello",
413+
});
414+
415+
const parts = broadcasts.filter(
416+
({ data }) => (data as { type?: string }).type === "ContentPart",
417+
);
418+
expect(parts).toHaveLength(1);
419+
});
420+
421+
it("coalesces two concurrent new-session opens for one view onto one session", async () => {
422+
const { runtime, sdk } = createRuntime();
423+
424+
const [first, second] = await Promise.all([
425+
runtime.openSession(openOptions()),
426+
runtime.openSession(openOptions()),
427+
]);
428+
429+
expect(second).toBe(first);
430+
expect(sdk.createInputs).toHaveLength(1);
431+
expect(first.subscribers).toEqual(["view-1"]);
432+
});
433+
434+
it("does not double-wrap the SDK session when two attaches race for it", async () => {
435+
const sdk = createFakeHarness();
436+
const broadcasts: { event: string; data: unknown; webviewId?: string }[] = [];
437+
const runtime = new PythinkerRuntime({
438+
version: "0.6.0",
439+
harness: sdk.harness,
440+
broadcast: (event, data, webviewId) => {
441+
broadcasts.push({ event, data, webviewId });
442+
},
443+
captureBaseline: () => undefined,
444+
log: () => undefined,
445+
});
446+
const boundary = sdk.addSession("saved-1", "/workspace");
447+
448+
const [first, second] = await Promise.all([
449+
runtime.attachResumedSession("view-1", boundary.session),
450+
runtime.attachResumedSession("view-1", boundary.session),
451+
]);
452+
453+
expect(second).toBe(first);
454+
expect(boundary.subscriptionCount()).toBe(1);
455+
456+
boundary.emit({
457+
type: "assistant.delta",
458+
sessionId: "saved-1",
459+
agentId: "main",
460+
turnId: 1,
461+
delta: "Hello",
462+
});
463+
464+
const parts = broadcasts.filter(
465+
({ data }) => (data as { type?: string }).type === "ContentPart",
466+
);
467+
expect(parts).toHaveLength(1);
468+
});
469+
385470
it("preserves the resumed session's model instead of reapplying the configured default", async () => {
386471
const { runtime, sdk } = createRuntime();
387472
const session = sdk.addSession("saved-1", "/workspace", { model: "old-model" });

docs/reference/server-api.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ Endpoints are grouped by resource below. A `:{action}` suffix in a path is the a
177177
| `GET /api/v1/sessions/{session_id}/tasks` | List background tasks |
178178
| `GET /api/v1/sessions/{session_id}/tasks/{task_id}` | Read a task (optional output preview) |
179179
| `POST /api/v1/sessions/{session_id}/tasks/{task_id}:cancel` | Cancel a task |
180+
| `POST /api/v1/sessions/{session_id}/tasks/{task_id}:detach` | Move a foreground task to the background |
180181

181182
### Skills, tools, and MCP
182183

packages/agent-core-v2/docs/state-manifest.d.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1353,6 +1353,7 @@ export interface AgentStateSnapshot {
13531353
readonly command: string;
13541354
readonly pid: number;
13551355
readonly exitCode: number | null;
1356+
readonly parentToolCallId?: string;
13561357
readonly taskId: string;
13571358
readonly description: string;
13581359
readonly status: /* AgentTaskStatus — packages/agent-core-v2/src/agent/task/types.ts */ 'completed' | 'failed' | 'running' | 'timed_out' | 'killed' | 'lost';
@@ -1400,6 +1401,7 @@ export interface AgentStateSnapshot {
14001401
readonly command: string;
14011402
readonly pid: number;
14021403
readonly exitCode: number | null;
1404+
readonly parentToolCallId?: string;
14031405
readonly taskId: string;
14041406
readonly description: string;
14051407
readonly status: /* AgentTaskStatus — packages/agent-core-v2/src/agent/task/types.ts */ 'completed' | 'failed' | 'running' | 'timed_out' | 'killed' | 'lost';

0 commit comments

Comments
 (0)