Merge PR #6484: fix(coding-agent): resume agent after /tree ask re-answer (@roboomp)
This commit is contained in:
@@ -115,6 +115,7 @@
|
||||
- Fixed the first submitted prompt stalling while the local tiny-title worker started: the interactive submit handler now paints the pending user row before starting title generation, and startup prewarms an idle, unref'd worker so the first submit reuses a live subprocess instead of paying spawn latency ahead of the first frame ([#6462](https://github.com/can1357/oh-my-pi/issues/6462)).
|
||||
- Fixed legacy Pi extensions failing validation when importing the upstream `keyText` keybinding helper ([#6470](https://github.com/can1357/oh-my-pi/issues/6470)).
|
||||
- Fixed Escape waiting for an in-flight `session_stop` extension handler to exhaust its timeout; abort now cancels the active stop pass without reporting a false timeout or applying stale continuation context ([#6489](https://github.com/can1357/oh-my-pi/issues/6489)).
|
||||
- Fixed the agent not resuming after re-answering a past `ask` from the session tree. Committing a new answer via `/tree` branched a fresh sibling `toolResult` and rebuilt context, but nothing ever continued the agent — unlike a live `ask`, whose continuation is intrinsic to the streaming run loop — so the model never consumed the new answer and the session sat idle until a manual prompt. `navigateTree` now reports the commit (`askReanswerCommitted`) and the interactive `/tree` handler resumes the agent via `resumeAfterAskReanswer()` *after* its transcript rebuild, so the resumed turn never renders against the stale pre-rebuild UI. Plain leaf moves and the read-only `reopenAsk` probe stay idle ([#6483](https://github.com/can1357/oh-my-pi/issues/6483)).
|
||||
|
||||
## [17.1.0] - 2026-07-24
|
||||
|
||||
|
||||
@@ -1275,6 +1275,15 @@ export class SelectorController {
|
||||
this.ctx.editor.setText(result.editorText);
|
||||
}
|
||||
this.ctx.showStatus("Navigated to selected point");
|
||||
|
||||
// Re-answering a past `ask` commits a new sibling answer but,
|
||||
// unlike a live `ask`, leaves the agent idle. Resume it now —
|
||||
// after the transcript rebuild above — so the model consumes the
|
||||
// new answer without the resumed turn rendering against the stale
|
||||
// pre-rebuild UI (issue #6483).
|
||||
if (result.askReanswerCommitted) {
|
||||
this.ctx.session.resumeAfterAskReanswer();
|
||||
}
|
||||
} catch (error) {
|
||||
this.ctx.showError(error instanceof Error ? error.message : String(error));
|
||||
} finally {
|
||||
|
||||
@@ -7339,6 +7339,14 @@ export class AgentSession {
|
||||
* (issue #5642).
|
||||
*/
|
||||
reopenAsk?: { toolCallId: string; questions: AskToolInput["questions"] };
|
||||
/**
|
||||
* `true` when this call committed a new sibling answer for an `ask`
|
||||
* re-answer (`reanswerAskResult` was applied). The interactive caller
|
||||
* resumes the agent via {@link resumeAfterAskReanswer} *after* rebuilding
|
||||
* its transcript, so the resumed turn never renders against the stale
|
||||
* pre-rebuild UI (issue #6483).
|
||||
*/
|
||||
askReanswerCommitted?: boolean;
|
||||
}> {
|
||||
await this.#bash.flushPending();
|
||||
const oldLeafId = this.sessionManager.getLeafId();
|
||||
@@ -7489,6 +7497,10 @@ export class AgentSession {
|
||||
// Determine the new leaf position based on target type
|
||||
let newLeafId: string | null;
|
||||
let editorText: string | undefined;
|
||||
// Set when the second-pass `ask` re-answer branch below actually commits a
|
||||
// new sibling answer — the trigger for resuming the agent afterwards so the
|
||||
// model consumes it, mirroring a live `ask` completion (issue #6483).
|
||||
let isAskReanswerCompletion = false;
|
||||
|
||||
if (targetEntry.type === "message" && targetEntry.message.role === "user") {
|
||||
// User message: leaf = parent (null if root), text goes to editor
|
||||
@@ -7525,6 +7537,7 @@ export class AgentSession {
|
||||
timestamp: Date.now(),
|
||||
};
|
||||
newLeafId = this.sessionManager.appendMessageToBranch(toolResultMessage, targetEntry.parentId);
|
||||
isAskReanswerCompletion = true;
|
||||
} else {
|
||||
// Non-user message (or a user-invoked skill-prompt injection): land the
|
||||
// leaf on the selected node so it stays on the active branch. Skill
|
||||
@@ -7570,6 +7583,14 @@ export class AgentSession {
|
||||
|
||||
this.#branchSummaryAbortController = undefined;
|
||||
|
||||
// Report a committed `ask` re-answer so the interactive caller can resume
|
||||
// the agent via `resumeAfterAskReanswer()` *after* rebuilding its
|
||||
// transcript. Scheduling the continue here instead would start a fresh
|
||||
// streaming turn whose `agent_start`/`turn_start` events could render
|
||||
// against the stale pre-rebuild UI and then be clobbered by the caller's
|
||||
// `renderInitialMessages(...)` (issue #6483). Plain leaf moves and the
|
||||
// read-only `reopenAsk` probe leave the flag unset.
|
||||
|
||||
// Emit session_tree event; only handlers can mutate session entries, so skip
|
||||
// the emit and the context rebuild when no handlers are registered (mirrors
|
||||
// the session_before_tree guard above).
|
||||
@@ -7582,9 +7603,34 @@ export class AgentSession {
|
||||
fromExtension: summaryText ? fromExtension : undefined,
|
||||
});
|
||||
const rawContext = this.sessionManager.buildSessionContext();
|
||||
return { editorText, cancelled: false, summaryEntry, sessionContext: rawContext };
|
||||
return {
|
||||
editorText,
|
||||
cancelled: false,
|
||||
summaryEntry,
|
||||
sessionContext: rawContext,
|
||||
askReanswerCommitted: isAskReanswerCompletion,
|
||||
};
|
||||
}
|
||||
return { editorText, cancelled: false, summaryEntry, sessionContext: stateContext };
|
||||
return {
|
||||
editorText,
|
||||
cancelled: false,
|
||||
summaryEntry,
|
||||
sessionContext: stateContext,
|
||||
askReanswerCommitted: isAskReanswerCompletion,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Resume the agent after the interactive `/tree` caller has committed an
|
||||
* `ask` re-answer (`navigateTree` returned `askReanswerCommitted`) and
|
||||
* rebuilt its transcript. Mirrors how a live `ask` completion drives a
|
||||
* follow-up turn, but is deferred to the caller so the resumed turn renders
|
||||
* against the rebuilt UI rather than the stale pre-navigation transcript
|
||||
* (issue #6483). The scheduled continue honors the same disposed/compacting
|
||||
* guards as every other post-prompt continuation.
|
||||
*/
|
||||
resumeAfterAskReanswer(): void {
|
||||
this.#scheduleAgentContinue();
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -458,6 +458,75 @@ describe("AgentSession tree navigation onto an ask toolResult", () => {
|
||||
await ctx.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it("(k) reports a committed re-answer and resumes the agent only via resumeAfterAskReanswer", async () => {
|
||||
const ctx = await createTestSession({ inMemory: true });
|
||||
try {
|
||||
const { session, sessionManager } = ctx;
|
||||
|
||||
// u1 -> a1(ask toolCall) -> tr1(stale answer) -> a2(next reply, leaf)
|
||||
sessionManager.appendMessage(userMsg("please deploy"));
|
||||
const askCallId = "ask-call-1";
|
||||
sessionManager.appendMessage(toolCallMsg(askCallId, "ask", { questions: ORIGINAL_QUESTIONS }));
|
||||
const tr1Id = sessionManager.appendMessage(
|
||||
toolResultMsg(askCallId, "ask", "User selected: staging", staleAnswerResult().details),
|
||||
);
|
||||
sessionManager.appendMessage(assistantMsg("deploying to staging"));
|
||||
|
||||
const continueSpy = vi.spyOn(session.agent, "continue").mockResolvedValue(undefined);
|
||||
|
||||
const probe = await session.navigateTree(tr1Id, { allowAskReopen: true });
|
||||
expect(probe.reopenAsk).toBeDefined();
|
||||
// The read-only probe commits nothing, so it must not report a re-answer.
|
||||
expect(probe.askReanswerCommitted).toBeFalsy();
|
||||
|
||||
const result = await session.navigateTree(tr1Id, {
|
||||
allowAskReopen: true,
|
||||
reanswerAskResult: newAnswerResult(),
|
||||
});
|
||||
expect(result.cancelled).toBe(false);
|
||||
// navigateTree reports the commit but does NOT resume on its own — the
|
||||
// interactive caller owns the timing so the resumed turn renders against
|
||||
// the rebuilt transcript (issue #6483).
|
||||
expect(result.askReanswerCommitted).toBe(true);
|
||||
await session.waitForIdle();
|
||||
expect(continueSpy).not.toHaveBeenCalled();
|
||||
// The tail the resume will continue from is the new answer toolResult.
|
||||
const messages = session.messages;
|
||||
expect(messages[messages.length - 1]?.role).toBe("toolResult");
|
||||
|
||||
// The caller resumes explicitly after rebuilding its UI.
|
||||
session.resumeAfterAskReanswer();
|
||||
await session.waitForIdle();
|
||||
expect(continueSpy).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
await ctx.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it("(l) does not report a committed re-answer for a plain non-ask leaf move", async () => {
|
||||
const ctx = await createTestSession({ inMemory: true });
|
||||
try {
|
||||
const { session, sessionManager } = ctx;
|
||||
|
||||
sessionManager.appendMessage(userMsg("read the config"));
|
||||
sessionManager.appendMessage(toolCallMsg("read-call-1", "read", { path: "config.txt" }));
|
||||
const tr1Id = sessionManager.appendMessage(toolResultMsg("read-call-1", "read", "file body"));
|
||||
sessionManager.appendMessage(assistantMsg("done reading"));
|
||||
|
||||
const continueSpy = vi.spyOn(session.agent, "continue").mockResolvedValue(undefined);
|
||||
|
||||
const result = await session.navigateTree(tr1Id, { allowAskReopen: true });
|
||||
expect(result.cancelled).toBe(false);
|
||||
expect(result.reopenAsk).toBeUndefined();
|
||||
expect(result.askReanswerCommitted).toBeFalsy();
|
||||
|
||||
await session.waitForIdle();
|
||||
expect(continueSpy).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
await ctx.cleanup();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("AgentSession.buildAskReanswerContext", () => {
|
||||
|
||||
+50
-3
@@ -85,20 +85,35 @@ function createCtx(leafEntry: SessionEntry, navigateTreeResult: unknown = { canc
|
||||
const showStatus = vi.fn();
|
||||
const showError = vi.fn();
|
||||
const editorContainer = createEditorSlot();
|
||||
// Records the order of UI-rebuild vs agent-resume so a test can prove the
|
||||
// re-answer continuation is deferred until after the transcript rebuild
|
||||
// (issue #6483).
|
||||
const order: string[] = [];
|
||||
const renderInitialMessages = vi.fn(() => {
|
||||
order.push("render");
|
||||
});
|
||||
const reloadTodos = vi.fn(async () => {
|
||||
order.push("reloadTodos");
|
||||
});
|
||||
const resumeAfterAskReanswer = vi.fn(() => {
|
||||
order.push("resume");
|
||||
});
|
||||
const ctx = {
|
||||
editor: { id: "editor" },
|
||||
editor: { id: "editor", getText: () => "", setText: vi.fn() },
|
||||
editorContainer,
|
||||
sessionManager: {
|
||||
getTree: () => tree,
|
||||
getLeafId: () => leafEntry.id,
|
||||
getEntry: (id: string) => (id === leafEntry.id ? leafEntry : undefined),
|
||||
},
|
||||
session: { navigateTree },
|
||||
session: { navigateTree, resumeAfterAskReanswer },
|
||||
ui: {
|
||||
setFocus: vi.fn(),
|
||||
requestRender: vi.fn(),
|
||||
terminal: { rows: 24 },
|
||||
},
|
||||
renderInitialMessages,
|
||||
reloadTodos,
|
||||
showStatus,
|
||||
showError,
|
||||
// No UI context available in this unit test — forces `#reanswerAsk` to
|
||||
@@ -109,7 +124,7 @@ function createCtx(leafEntry: SessionEntry, navigateTreeResult: unknown = { canc
|
||||
// itself (already covered at the session level).
|
||||
getToolUIContext: () => undefined,
|
||||
} as unknown as InteractiveModeContext;
|
||||
return { ctx, editorContainer, navigateTree, showStatus, showError };
|
||||
return { ctx, editorContainer, navigateTree, showStatus, showError, resumeAfterAskReanswer, order };
|
||||
}
|
||||
|
||||
/** Grabs the `TreeSelectorComponent` mounted by the most recent `showTreeSelector()` call and fires its onSelect as if the user pressed Enter on `entryId`. */
|
||||
@@ -160,4 +175,36 @@ describe("SelectorController.showTreeSelector re-answering the active ask leaf",
|
||||
expect(showError).toHaveBeenCalledWith("Ask tool UI is not ready");
|
||||
expect(showStatus).toHaveBeenCalledWith("Re-answer cancelled");
|
||||
});
|
||||
|
||||
it("resumes the agent only after rebuilding the transcript when navigateTree reports a committed re-answer", async () => {
|
||||
const entry = plainUserEntry("leaf-user");
|
||||
const { ctx, editorContainer, showStatus, resumeAfterAskReanswer, order } = createCtx(entry, {
|
||||
cancelled: false,
|
||||
askReanswerCommitted: true,
|
||||
});
|
||||
const controller = new SelectorController(ctx);
|
||||
|
||||
controller.showTreeSelector();
|
||||
// A non-current target skips the no-op short-circuit and lands straight on
|
||||
// the success path (navigateTree here returns a committed re-answer).
|
||||
await pickEntry(editorContainer, "some-other-entry");
|
||||
|
||||
expect(showStatus).toHaveBeenCalledWith("Navigated to selected point");
|
||||
expect(resumeAfterAskReanswer).toHaveBeenCalledTimes(1);
|
||||
// The resume must be deferred until after the transcript rebuild so the
|
||||
// resumed turn never renders against the stale pre-rebuild UI (issue #6483).
|
||||
expect(order.indexOf("render")).toBeGreaterThanOrEqual(0);
|
||||
expect(order.indexOf("resume")).toBeGreaterThan(order.indexOf("render"));
|
||||
});
|
||||
|
||||
it("does not resume the agent for a plain navigation without a committed re-answer", async () => {
|
||||
const entry = plainUserEntry("leaf-user");
|
||||
const { ctx, editorContainer, resumeAfterAskReanswer } = createCtx(entry, { cancelled: false });
|
||||
const controller = new SelectorController(ctx);
|
||||
|
||||
controller.showTreeSelector();
|
||||
await pickEntry(editorContainer, "some-other-entry");
|
||||
|
||||
expect(resumeAfterAskReanswer).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user