diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 25c809467..0fa5a101f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -230,6 +230,9 @@ - Fixed MiMo models using hashline edit mode by default despite needing the same replace-mode fallback as Kimi. ([#3772](https://github.com/can1357/oh-my-pi/issues/3772)) - Fixed `omp` refusing to start on Windows when no `bash.exe` is discoverable — most visibly with scoop-installed Git, whose manifest shims `sh.exe`/`git.exe` but never `bash.exe`, so PATH lookup missed it. Startup threw `No bash shell found` while merely building the bash tool description, even though bash tool commands always execute in the embedded brush-core shell and need no host bash. Shell discovery now also checks `GIT_INSTALL_ROOT`, scoop and per-user Git for Windows install roots, and `sh.exe` on PATH, then falls back to `cmd.exe` for the spawn-only paths (interactive PTY, ACP client terminals) instead of failing; the cmd fallback is never used to wrap user-shell commands — brush runs the POSIX line directly. - Added a selectable voice setting for `/live` realtime sessions ([#6566](https://github.com/can1357/oh-my-pi/issues/6566)). +### Fixed + +- Withheld advisor nits and concerns while the primary turn is explicitly marked in progress, while still allowing blockers for unrecoverable active side effects. ## [17.1.4] - 2026-07-26 diff --git a/packages/coding-agent/src/advisor/advise-tool.ts b/packages/coding-agent/src/advisor/advise-tool.ts index d2b00fea7..91e5e328d 100644 --- a/packages/coding-agent/src/advisor/advise-tool.ts +++ b/packages/coding-agent/src/advisor/advise-tool.ts @@ -180,12 +180,23 @@ export class AdviseTool implements AgentTool * escalation: nit → concern → blocker), so an advisor cannot bypass dedupe * by retagging the same text at a lower or equal severity. */ #deliveredNoteSeverities = new Map(); + #inProgressUpdate = false; constructor(private readonly onAdvice: (note: string, severity?: AdviseDetails["severity"]) => void) {} + /** + * Mark whether the next advisor prompt reviews an in-progress primary turn. + * Non-blockers are withheld until a completed update so partial work does + * not interrupt the primary before it can finish its planned steps. + */ + beginUpdate(inProgress: boolean): void { + this.#inProgressUpdate = inProgress; + } + /** Clear delivered-note memory when the advisor starts a fresh conversation. */ resetDeliveredNotes(): void { this.#deliveredNoteSeverities.clear(); + this.#inProgressUpdate = false; } async execute( @@ -195,6 +206,13 @@ export class AdviseTool implements AgentTool _onUpdate?: AgentToolUpdateCallback, _context?: AgentToolContext, ): Promise> { + if (this.#inProgressUpdate && args.severity !== "blocker") { + return { + content: [{ type: "text", text: "Recorded." }], + details: { note: args.note, severity: args.severity }, + useless: true, + }; + } const key = advisorNoteDedupeKey(args.note); const rank = advisorSeverityRank(args.severity); const previousRank = this.#deliveredNoteSeverities.get(key) ?? 0; diff --git a/packages/coding-agent/src/advisor/runtime.ts b/packages/coding-agent/src/advisor/runtime.ts index 245e0abba..071a6c014 100644 --- a/packages/coding-agent/src/advisor/runtime.ts +++ b/packages/coding-agent/src/advisor/runtime.ts @@ -51,11 +51,11 @@ export interface AdvisorRuntimeHost { maintainContext?(incomingTokens: number, signal: AbortSignal): Promise; /** * Called immediately before each `agent.prompt(batch)` cycle. Lets the host - * clear per-update advisor state — currently the one-advise-per-update gate - * in {@link AdvisorEmissionGuard}, which the host owns because it is the - * one that routes `advise()` results back to the primary. + * clear per-update advisor state and apply the in-progress delivery policy. + * The host owns these gates because it routes `advise()` results back to the + * primary. */ - beginAdvisorUpdate?(): void; + beginAdvisorUpdate?(inProgress: boolean): void; /** * Called with the error of every failed advisor turn, before the retry sleep * or the dropped-after-3 path. Lets the host apply credential-level remedies @@ -909,8 +909,8 @@ export class AdvisorRuntime { const contextWasFresh = resetContext || recoveringOverflow || messageSnapshot === 0; try { // Reset the host's per-update advisor state (one-advise-per-update - // gate) before each model cycle so the new batch starts fresh. - this.host.beginAdvisorUpdate?.(); + // gate) and pass through whether this batch reviews partial work. + this.host.beginAdvisorUpdate?.(wip); const prompt = this.agent.prompt(batch); this.#promptInFlight = prompt; try { diff --git a/packages/coding-agent/src/session/session-advisors.ts b/packages/coding-agent/src/session/session-advisors.ts index 97e6751ed..9e61a6024 100644 --- a/packages/coding-agent/src/session/session-advisors.ts +++ b/packages/coding-agent/src/session/session-advisors.ts @@ -877,7 +877,10 @@ export class SessionAdvisors { this.#maintainAdvisorContext(advisorRef, incomingTokens, signal), obfuscator: this.#host.obfuscator, getModelIdentity: () => formatModelString(advisorRef.agent.state.model), - beginAdvisorUpdate: () => advisorRef.emissionGuard.beginUpdate(), + beginAdvisorUpdate: inProgress => { + advisorRef.adviseTool.beginUpdate(inProgress); + advisorRef.emissionGuard.beginUpdate(); + }, onTurnError: (error, failedMessages, signal) => this.#recoverAdvisorTurn(advisorRef, error, failedMessages, signal), onTurnSuccess: async () => { diff --git a/packages/coding-agent/test/advisor/advisor.test.ts b/packages/coding-agent/test/advisor/advisor.test.ts index 66d76114a..678192697 100644 --- a/packages/coding-agent/test/advisor/advisor.test.ts +++ b/packages/coding-agent/test/advisor/advisor.test.ts @@ -432,6 +432,25 @@ describe("advisor", () => { expect(onAdvice).toHaveBeenNthCalledWith(3, note, "blocker"); }); + it("withholds non-blockers for in-progress updates without consuming dedupe state", async () => { + const onAdvice = vi.fn(); + const tool = new AdviseTool(onAdvice); + const note = "The result still needs a focused regression test."; + + tool.beginUpdate(true); + await tool.execute("tc-1", { note, severity: "concern" }); + await tool.execute("tc-2", { note: "Minor naming cleanup.", severity: "nit" }); + await tool.execute("tc-3", { note: "A destructive command is running.", severity: "blocker" }); + + expect(onAdvice).toHaveBeenCalledTimes(1); + expect(onAdvice).toHaveBeenCalledWith("A destructive command is running.", "blocker"); + + tool.beginUpdate(false); + await tool.execute("tc-4", { note, severity: "concern" }); + expect(onAdvice).toHaveBeenCalledTimes(2); + expect(onAdvice).toHaveBeenLastCalledWith(note, "concern"); + }); + it("validates parameters using ArkType", () => { const onAdvice = vi.fn(); const tool = new AdviseTool(onAdvice); @@ -1275,6 +1294,7 @@ describe("advisor", () => { it("tags in-progress turns with [in progress] heading", async () => { const promptInputs: string[] = []; + const updateStates: boolean[] = []; const { promise: promptStarted, resolve: startPrompt } = Promise.withResolvers(); const agent: AdvisorAgent = { prompt: async input => { @@ -1289,6 +1309,7 @@ describe("advisor", () => { const host: AdvisorRuntimeHost = { snapshotMessages: () => messages, enqueueAdvice: () => {}, + beginAdvisorUpdate: inProgress => updateStates.push(inProgress), }; const runtime = new AdvisorRuntime(agent, host); @@ -1297,10 +1318,12 @@ describe("advisor", () => { expect(promptInputs).toHaveLength(1); expect(promptInputs[0]).toContain("[in progress — more steps follow]"); + expect(updateStates).toEqual([true]); }); it("uses plain heading when willContinue is false or absent", async () => { const promptInputs: string[] = []; + const updateStates: boolean[] = []; const { promise: promptStarted, resolve: startPrompt } = Promise.withResolvers(); const agent: AdvisorAgent = { prompt: async input => { @@ -1315,6 +1338,7 @@ describe("advisor", () => { const host: AdvisorRuntimeHost = { snapshotMessages: () => messages, enqueueAdvice: () => {}, + beginAdvisorUpdate: inProgress => updateStates.push(inProgress), }; const runtime = new AdvisorRuntime(agent, host); @@ -1324,6 +1348,7 @@ describe("advisor", () => { expect(promptInputs).toHaveLength(1); expect(promptInputs[0]).toContain("### Session update\n"); expect(promptInputs[0]).not.toContain("[in progress"); + expect(updateStates).toEqual([false]); }); it("sends the batch when context maintenance fails", async () => {