fix(coding-agent): suppress WIP advisor non-blockers

This commit is contained in:
Wolfgang Schoenberger
2026-07-30 15:18:06 -07:00
parent 4df68d6043
commit 26e422a00a
5 changed files with 56 additions and 7 deletions
+3
View File
@@ -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
@@ -180,12 +180,23 @@ export class AdviseTool implements AgentTool<typeof adviseSchema, AdviseDetails>
* escalation: nit → concern → blocker), so an advisor cannot bypass dedupe
* by retagging the same text at a lower or equal severity. */
#deliveredNoteSeverities = new Map<string, number>();
#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<typeof adviseSchema, AdviseDetails>
_onUpdate?: AgentToolUpdateCallback<AdviseDetails>,
_context?: AgentToolContext,
): Promise<AgentToolResult<AdviseDetails>> {
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;
+6 -6
View File
@@ -51,11 +51,11 @@ export interface AdvisorRuntimeHost {
maintainContext?(incomingTokens: number, signal: AbortSignal): Promise<boolean>;
/**
* 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 {
@@ -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 () => {
@@ -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<void>();
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<void>();
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 () => {