diff --git a/docs/advisor-watchdog.md b/docs/advisor-watchdog.md index d716833e1..448646179 100644 --- a/docs/advisor-watchdog.md +++ b/docs/advisor-watchdog.md @@ -8,6 +8,7 @@ The advisor is not a second executor. It cannot edit files, run commands, approv - [`src/advisor/runtime.ts`](../packages/coding-agent/src/advisor/runtime.ts) - [`src/advisor/advise-tool.ts`](../packages/coding-agent/src/advisor/advise-tool.ts) +- [`src/advisor/emission-guard.ts`](../packages/coding-agent/src/advisor/emission-guard.ts) - [`src/advisor/watchdog.ts`](../packages/coding-agent/src/advisor/watchdog.ts) - [`src/advisor/transcript-recorder.ts`](../packages/coding-agent/src/advisor/transcript-recorder.ts) - [`src/prompts/advisor/system.md`](../packages/coding-agent/src/prompts/advisor/system.md) @@ -100,6 +101,19 @@ When you deliberately interrupt the agent (Esc, or a cancel from collab, ACP, RP `advisor.immuneTurns` limits interruption frequency. After the advisor successfully delivers a `concern` or `blocker` through the steering channel, later concerns/blockers are routed as non-interrupting asides until the configured number of primary turns has completed. The default is `3`. `nit` notes are unchanged, and advice raised while user-interrupt auto-resume suppression is active is still preserved instead of restarting a stopped run. +### Emission guard + +`AdvisorEmissionGuard` (in `src/advisor/emission-guard.ts`) sits on the `enqueueAdvice` boundary in `AgentSession` and enforces — in code — the advisor system prompt's "at most one `advise` per update" and "NEVER send the same advice twice" rules. Each call to the advisor's `advise` tool runs through the guard before it routes to the YieldQueue / steer channel: + +1. **Normalization.** Lowercase, NFKC, collapse every run of non-alphanumeric characters to a single space, trim. `"Stop."`, `"*Stop*"`, and `" stop "` all key to `stop`. +2. **Content-free phrase filter.** A small allowlist of normalized phrases the advisor occasionally emits but that carry no concrete reason — `stop`, `done`, `complete`, `no issue continue`, `lgtm`, `nothing to add`, `no further input`, and similar — is suppressed silently. Silence is the correct expression of "no concerns". +3. **Exact-text dedupe.** Any normalized note already accepted in this session is dropped. The dedupe history is bounded by a FIFO ring (default 4096 entries). +4. **Per-update rate limit.** At most one note per advisor model `prompt()` cycle is accepted; the runtime calls `host.beginAdvisorUpdate?.()` before each cycle to reset the gate. Suppressed calls never consume the budget — a noise call doesn't displace a real concern that follows in the same update. + +Suppression is invisible to the advisor model: `AdviseTool` still returns `Recorded.` for a dropped call. Surfacing "suppressed" back into advisor context risks the model rephrasing the same useless note to bypass the dedupe. + +The guard's full state — dedupe history and per-update gate — clears on every advisor reset (compaction, session switch, `/new`), so a re-primed reviewer can re-raise issues it already raised against the rewritten transcript. + ## Bounded catch-up with `advisor.syncBacklog` `advisor.syncBacklog` is not lockstep turn execution. It is a bounded catch-up delay for the primary agent when the advisor falls behind. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 79022b068..a17865705 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -66,6 +66,10 @@ - Fixed repeated todo updates in one TUI turn stacking full todo panels; superseded todo snapshots now stay live until the next todo update replaces them or the turn ends. ([#3516](https://github.com/can1357/oh-my-pi/issues/3516)) - Fixed MCP OAuth authorization failing with `Authorization failed: An unexpected error occurred` against authorization servers (Plane is the live example) that reject redundant fallback `resource` indicators. OMP now drops same-origin resources only when it synthesized them from the server URL fallback (e.g. `https://mcp.plane.so/http/mcp`). Provider-advertised resources from OAuth/protected-resource discovery or an embedded authorization-URL `resource` query parameter are preserved even when they are same-origin or origin-only, so gateway-hosted MCP services can still request the audience they advertised. The refresh-token path uses the same policy, filtered against the authorization-server origin persisted on the credential as `authorizationUrl`, with `tokenUrl`'s origin as the legacy fallback when that field is absent. ([#3502](https://github.com/can1357/oh-my-pi/issues/3502)) +### Fixed + +- Fixed the advisor entering a spam loop in which it emitted hundreds of repeated `Stop.`, `Done.`, and `No issue; continue.` `` injections, polluting the primary transcript and destabilizing the watched agent after the task was already complete. The advisor system prompt's rules ("at most one `advise` per update", "NEVER send the same advice twice") are now enforced in code by a new `AdvisorEmissionGuard` on the `enqueueAdvice` boundary in `AgentSession`: it normalizes each note (case-insensitive, punctuation-folded), drops content-free self-talk filler (`stop`/`done`/`no issue continue`/`lgtm`/etc.), dedupes by exact normalized text across the session (bounded FIFO history), and rate-limits to one accepted note per advisor model prompt cycle. Reset on advisor reset (compaction, session switch, `/new`) so a re-primed reviewer can re-raise old issues. ([#3520](https://github.com/can1357/oh-my-pi/issues/3520)) + ## [16.1.20] - 2026-06-25 ### Fixed diff --git a/packages/coding-agent/src/advisor/__tests__/emission-guard.test.ts b/packages/coding-agent/src/advisor/__tests__/emission-guard.test.ts new file mode 100644 index 000000000..5c6928263 --- /dev/null +++ b/packages/coding-agent/src/advisor/__tests__/emission-guard.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it } from "bun:test"; +import { AdvisorEmissionGuard, normalizeAdvisorNote } from "../emission-guard"; + +describe("normalizeAdvisorNote", () => { + it("collapses punctuation, casing, and surrounding whitespace into one canonical key", () => { + // The reporter's three top duplicates all key to the same canonical form + // regardless of trailing punctuation or casing — that's what makes the + // dedupe + suppression checks single-membership. + expect(normalizeAdvisorNote("Stop.")).toBe("stop"); + expect(normalizeAdvisorNote(" STOP! ")).toBe("stop"); + expect(normalizeAdvisorNote("*Stop*")).toBe("stop"); + expect(normalizeAdvisorNote("Done.")).toBe("done"); + expect(normalizeAdvisorNote("No issue; continue.")).toBe("no issue continue"); + }); + + it("returns empty string for whitespace-only input so callers can short-circuit", () => { + expect(normalizeAdvisorNote("")).toBe(""); + expect(normalizeAdvisorNote(" ")).toBe(""); + expect(normalizeAdvisorNote("...")).toBe(""); + }); + + it("preserves internal letters/digits but folds non-alphanumeric runs to one space", () => { + expect(normalizeAdvisorNote("Refactor `auth-flow.ts`: drop legacy branch.")).toBe( + "refactor auth flow ts drop legacy branch", + ); + }); +}); + +describe("AdvisorEmissionGuard", () => { + it("drops the exact content-free filler the reporter observed flooding the chat", () => { + // Issue #3520: 114× "Stop.", 52× "No issue; continue.", 41× "Done." — + // none of these carry a concrete reason and they cannot be acted on, so + // the guard suppresses them regardless of severity. + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("Stop.")).toBe(false); + expect(guard.accept("Done.")).toBe(false); + expect(guard.accept("No issue; continue.")).toBe(false); + expect(guard.accept("LGTM")).toBe(false); + expect(guard.accept("No further watcher input needed.")).toBe(false); + }); + + it("dedupes by normalized text across the session, ignoring casing and trailing punctuation", () => { + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("Move retries into the queue, not the request path.")).toBe(true); + // Same advice with different casing and trailing punctuation must NOT + // land twice in the primary transcript. + expect(guard.accept("move retries into the queue, not the request path")).toBe(false); + expect(guard.accept("Move retries into the queue, not the request path!")).toBe(false); + }); + + it("rate-limits to one accepted advise per advisor update cycle", () => { + // The advisor system prompt says "at most one `advise` per update". Real + // models violate this; the guard enforces it at the boundary so the + // primary transcript never receives two advisories from one model cycle. + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("First concern: missing await in #handleRetry.")).toBe(true); + expect(guard.accept("Second concern: wrong env var name.")).toBe(false); + guard.beginUpdate(); + // New cycle: budget reset. + expect(guard.accept("Second concern: wrong env var name.")).toBe(true); + }); + + it("does not let a suppressed call consume the per-update budget", () => { + // A noise call like "Stop." must never displace a real concern that + // follows in the same advisor model cycle. + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("Stop.")).toBe(false); + expect(guard.accept("Concrete: read race in #handleRetry.")).toBe(true); + }); + + it("does not let a deduped call consume the per-update budget", () => { + // A repeat of a prior session note is dropped, but the model can still + // follow it with a fresh concrete concern in the same cycle. + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("Concrete: read race in #handleRetry.")).toBe(true); + guard.beginUpdate(); + expect(guard.accept("Concrete: read race in #handleRetry.")).toBe(false); + expect(guard.accept("New concern: cache eviction never fires.")).toBe(true); + }); + + it("reset clears dedupe and the per-update gate so a re-primed advisor can re-raise old issues", () => { + // Compaction / session-switch rewrites the primary transcript. The + // advisor is re-primed from scratch and may legitimately re-raise the + // same concerns — they're new context for a freshly-primed reviewer. + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("Race in #handleRetry.")).toBe(true); + expect(guard.accept("Race in #handleRetry.")).toBe(false); + guard.reset(); + expect(guard.accept("Race in #handleRetry.")).toBe(true); + }); + + it("evicts oldest entries when dedupe history exceeds capacity", () => { + // Bounded so very long sessions cannot grow the dedupe state without + // bound. Pre-eviction unique notes are remembered; post-eviction the + // oldest one is forgotten and can resurface. + const guard = new AdvisorEmissionGuard({ capacity: 3 }); + expect(guard.accept("first")).toBe(true); + guard.beginUpdate(); + expect(guard.accept("second")).toBe(true); + guard.beginUpdate(); + expect(guard.accept("third")).toBe(true); + guard.beginUpdate(); + // "first" still in history. + expect(guard.accept("first")).toBe(false); + guard.beginUpdate(); + // Fourth unique entry evicts "first". + expect(guard.accept("fourth")).toBe(true); + guard.beginUpdate(); + expect(guard.accept("first")).toBe(true); + }); + + it("rejects empty / whitespace-only notes without consuming the budget", () => { + const guard = new AdvisorEmissionGuard(); + expect(guard.accept("")).toBe(false); + expect(guard.accept(" ")).toBe(false); + expect(guard.accept("Concrete advice.")).toBe(true); + }); + + it("end-to-end: the reporter's 309-call spam log produces ≤1 accepted note across many updates", () => { + // Mimic the issue's distribution: 114× "Stop.", 52× "No issue; continue.", + // 41× "Done.", plus 102 copies of one concrete-but-repeated nit. Spread + // the calls across 50 advisor update cycles. Each cycle is allowed at + // most one accepted note, and identical-text repeats never escape the + // guard. After all calls, exactly the concrete nit has been accepted + // — and only once. + const guard = new AdvisorEmissionGuard(); + const accepted: string[] = []; + const stream: string[] = [ + ...Array(114).fill("Stop."), + ...Array(52).fill("No issue; continue."), + ...Array(41).fill("Done."), + ...Array(102).fill("Concrete-but-repeated nit: x"), + ]; + // Interleave across 50 update cycles. + const cycles = 50; + const perCycle = Math.ceil(stream.length / cycles); + for (let c = 0; c < cycles; c++) { + guard.beginUpdate(); + for (let i = 0; i < perCycle; i++) { + const note = stream[c * perCycle + i]; + if (note === undefined) break; + if (guard.accept(note)) accepted.push(note); + } + } + expect(accepted).toEqual(["Concrete-but-repeated nit: x"]); + }); +}); diff --git a/packages/coding-agent/src/advisor/emission-guard.ts b/packages/coding-agent/src/advisor/emission-guard.ts new file mode 100644 index 000000000..f54dabe3e --- /dev/null +++ b/packages/coding-agent/src/advisor/emission-guard.ts @@ -0,0 +1,172 @@ +/** + * Per-session policy gate for advisor `advise()` calls. + * + * The advisor system prompt tells the watcher model: + * + * > at most one `advise` per update + * > NEVER repeat advice you already gave, and NEVER send the same advice twice + * + * Real advisor models violate this. Issue #3520 captured a session where + * `__advisor.jsonl` recorded 309 `advise` calls covering 92 unique notes — + * 114× `Stop.`, 52× `No issue; continue.`, 41× `Done.` — flooding the primary + * transcript with `Stop.` after the + * task was already complete. The fix is to make the rules load-bearing in code + * instead of prose: silently drop duplicates, content-free self-talk, and + * over-budget calls at the `enqueueAdvice` boundary so the primary stays + * clean even when the advisor misbehaves. + * + * The gate is intentionally invisible to the advisor model — `AdviseTool` + * still returns `Recorded.` for a suppressed call. Surfacing "suppressed" + * back into advisor context risks the model rephrasing the same useless note + * to bypass the dedupe ("Stop.", then "Halt." then "Stop now."). + */ + +/** + * Case-insensitive, punctuation-folded normalization. Collapses every run of + * non-letter / non-digit characters into a single space and trims, so + * `"Stop."`, `"*Stop*"`, and `" stop "` all key to `stop`, while + * `"No issue; continue."` keys to `no issue continue`. + * + * Exported for tests. + */ +export function normalizeAdvisorNote(note: string): string { + return note + .toLowerCase() + .normalize("NFKC") + .replace(/[^\p{L}\p{N}]+/gu, " ") + .trim(); +} + +/** + * Normalized phrases the advisor occasionally emits that carry no concrete + * actionable content. Each must be the output of {@link normalizeAdvisorNote} + * so a single membership check covers every punctuation/casing variant + * (`"Stop."`, `"stop"`, `"STOP!"`). + * + * The list is conservative — only short, content-free filler the reporter + * observed driving primary-transcript pollution. A genuine `blocker` like + * `"Stop: 'await' missing on writeStream.end() will lose buffered writes."` + * does not match. + */ +const SUPPRESSED_NORMALIZED_PHRASES: Record = { + // Self-stop noise — telling the agent to "stop" without a reason is useless. + stop: true, + "stop here": true, + "stop now": true, + halt: true, + abort: true, + // Completion self-talk — the agent already finished the task. + done: true, + "task done": true, + "task complete": true, + complete: true, + finished: true, + ok: true, + okay: true, + "ok done": true, + // "Nothing to flag" — silence is the correct expression of "no concerns". + "no issue": true, + "no issues": true, + "no issue continue": true, + "no concerns": true, + "no concern": true, + "nothing to add": true, + "nothing to flag": true, + "nothing to report": true, + "no notes": true, + "no further input": true, + "no further input needed": true, + "no further input required": true, + "no further watcher input": true, + "no further watcher input needed": true, + "no further advice": true, + "no further advice needed": true, + // Endorsements — equivalent to silence. + lgtm: true, + "looks good": true, + "all good": true, + "agent is on track": true, + "agent on track": true, + "on track": true, + continue: true, + "carry on": true, +}; + +/** + * Bounds the dedupe history. Sessions with very long advisor activity could + * otherwise grow the set without bound. The reporter's pathological session + * had 92 unique notes; 4096 leaves headroom while staying tiny (≤ ~256 KB of + * normalized strings even at long max). + */ +const DEFAULT_HISTORY_CAPACITY = 4096; + +/** + * Decides whether an advisor `advise()` call should reach the primary agent. + * + * Enforces — in this order — the noise filter, session-scoped exact-text + * dedupe (FIFO-evicted at {@link DEFAULT_HISTORY_CAPACITY}), and a per-update + * rate limit of one accepted note per advisor model prompt. Suppressed calls + * never consume the per-update budget — a noise call doesn't burn the slot + * for a real concern that follows in the same update. + * + * Reset on advisor reset (compaction, session switch, `/new`) via + * {@link reset}. Per-update gate is cleared at the start of every advisor + * `agent.prompt()` cycle via {@link beginUpdate}. + */ +export class AdvisorEmissionGuard { + #seen = new Set(); + /** Insertion-order log to drive FIFO eviction without an extra Map. */ + #seenOrder: string[] = []; + #consumedThisUpdate = false; + readonly #capacity: number; + + constructor(opts: { capacity?: number } = {}) { + this.#capacity = opts.capacity ?? DEFAULT_HISTORY_CAPACITY; + } + + /** + * Drop all dedupe and per-update state. Called from + * `AgentSession#resetAdvisorSessionState()` whenever the advisor runtime is + * reset — same boundary as `yieldQueue.clear("advisor")`, so a re-primed + * advisor can re-raise old issues (the primary transcript was rewritten). + */ + reset(): void { + this.#seen.clear(); + this.#seenOrder.length = 0; + this.#consumedThisUpdate = false; + } + + /** + * Clear the per-update rate-limit gate. Called by `AdvisorRuntime` right + * before each `agent.prompt(batch)` invocation so the next advisor model + * cycle starts with a fresh budget of one advise. + */ + beginUpdate(): void { + this.#consumedThisUpdate = false; + } + + /** + * Whether the proposed note should reach the primary. On `true` the gate + * has already recorded the note (consumed the per-update budget and added + * it to the dedupe history) — caller delivers the note. On `false` the + * caller drops it. + * + * Empty / whitespace-only notes are suppressed; the model's + * tool-args contract still requires a non-empty string but defense-in-depth. + */ + accept(note: string): boolean { + const key = normalizeAdvisorNote(note); + if (!key) return false; + if (SUPPRESSED_NORMALIZED_PHRASES[key]) return false; + if (this.#seen.has(key)) return false; + if (this.#consumedThisUpdate) return false; + this.#consumedThisUpdate = true; + this.#seen.add(key); + this.#seenOrder.push(key); + if (this.#seenOrder.length > this.#capacity) { + const stale = this.#seenOrder.shift(); + if (stale !== undefined) this.#seen.delete(stale); + } + return true; + } +} diff --git a/packages/coding-agent/src/advisor/index.ts b/packages/coding-agent/src/advisor/index.ts index 3a7362afb..fd6eea093 100644 --- a/packages/coding-agent/src/advisor/index.ts +++ b/packages/coding-agent/src/advisor/index.ts @@ -1,4 +1,5 @@ export * from "./advise-tool"; +export * from "./emission-guard"; export * from "./runtime"; export * from "./transcript-recorder"; export * from "./watchdog"; diff --git a/packages/coding-agent/src/advisor/runtime.ts b/packages/coding-agent/src/advisor/runtime.ts index 794c4bd00..d9901220b 100644 --- a/packages/coding-agent/src/advisor/runtime.ts +++ b/packages/coding-agent/src/advisor/runtime.ts @@ -30,6 +30,13 @@ export interface AdvisorRuntimeHost { * the primary's next compaction triggers {@link AdvisorRuntime.reset}). */ maintainContext?(incomingTokens: number): 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. + */ + beginAdvisorUpdate?(): void; } interface PendingDelta { @@ -275,6 +282,10 @@ export class AdvisorRuntime { let success = false; try { + // Reset the host's per-update advisor state (one-advise-per-update + // gate) before each model cycle, so the new batch starts with a + // fresh budget. Dedupe history persists across cycles. + this.host.beginAdvisorUpdate?.(); await this.agent.prompt(batch); success = true; this.#consecutiveFailures = 0; diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 7839fb365..5acab9e0e 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -130,6 +130,7 @@ import * as snapcompact from "@oh-my-pi/snapcompact"; import { AdviseTool, type AdvisorAgent, + AdvisorEmissionGuard, type AdvisorMessageDetails, type AdvisorNote, AdvisorRuntime, @@ -1172,6 +1173,11 @@ export class AgentSession { #advisorAutoResumeSuppressed = false; #advisorPrimaryTurnsCompleted = 0; #advisorInterruptImmuneTurnStart: number | undefined; + /** Dedupe + per-update rate-limit + content-free-phrase filter applied to + * every accepted advisor `advise()` call. Owned by the session because the + * session is what routes accepted notes back to the primary transcript. + * Reset on advisor reset (compaction, session switch, `/new`). */ + readonly #advisorEmissionGuard = new AdvisorEmissionGuard(); #planModeState: PlanModeState | undefined; #goalModeState: GoalModeState | undefined; #goalRuntime: GoalRuntime; @@ -1817,6 +1823,7 @@ export class AgentSession { this.#advisorAgentUnsubscribe = undefined; this.#advisorRuntime?.reset(); this.#advisorAdviseTool?.resetDeliveredNotes(); + this.#advisorEmissionGuard.reset(); this.#attachAdvisorRecorderFeed(); this.#advisorPrimaryTurnsCompleted = 0; this.#advisorInterruptImmuneTurnStart = undefined; @@ -1856,7 +1863,17 @@ export class AgentSession { // since steering an active run auto-resumes nothing; parking it there would // strand the advice and dump the backlog as one burst at the next prompt. A // plain nit always rides the non-interrupting YieldQueue aside. + // Apply the per-session emission policy (one-advise-per-update gate, + // exact-text dedupe, content-free phrase filter) before any routing. + // Suppression here means the advisor model called `advise()` but the call + // is dropped silently — the model still sees `Recorded.` from the tool, so + // telling it "suppressed" doesn't tempt it into rephrasing the same useless + // note to bypass the dedupe. const enqueueAdvice = (note: string, severity?: AdvisorSeverity) => { + if (!this.#advisorEmissionGuard.accept(note)) { + logger.debug("advisor advice suppressed by emission guard", { severity }); + return; + } const interrupting = isInterruptingSeverity(severity); const channel = resolveAdvisorDeliveryChannel({ severity, @@ -1980,6 +1997,7 @@ export class AgentSession { enqueueAdvice, maintainContext: incomingTokens => this.#maintainAdvisorContext(incomingTokens), obfuscator: this.#obfuscator, + beginAdvisorUpdate: () => this.#advisorEmissionGuard.beginUpdate(), }); if (seedToCurrent) { this.#advisorRuntime.seedTo(this.agent.state.messages.length);