From 459dd90f826800620baea831bdc2f8a2ac385e1e Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:00:09 +0800 Subject: [PATCH] fix(daemon): stop the coalesced-comment fallback from pulling the whole issue history (MUL-5442) (#6309) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(daemon): stop the coalesced-comment fallback from pulling the whole issue history (MUL-5442) When a run covers comments that arrived before it started and the server sent only their ids (no bodies), the prompt told the agent to find them by running `multica issue comment list --recent 30`. `--recent N` caps THREADS, not comments, and every returned thread carries all of its descendants. On an issue with fewer than 30 root threads that is the entire comment history — measured on a live 3-thread issue: 88,301 bytes (~22k tokens) to locate two ids. It also contradicted the runtime brief's own catch-up step, which tells the agent to read in two bounded steps and never make one bulk pull (MUL-5372): the platform was recommending the exact shape it forbids elsewhere. Those comments all arrived between the agent's previous run and this one, so when the server supplied that anchor `--since` returns precisely them in one bounded read — 17,376 bytes on the same issue. When there is no anchor (no prior run on this issue, so the server sends none) the fallback now uses the same scan-then-expand pair the brief teaches instead of a bulk pull. Also drops the "--full if a thread is folded" note: per `comment list --help`, `--since`, `--tail` and `--roots-only` reads are never folded, so it was advice that could not apply to either replacement read. This was the last place the platform steered agents onto `--recent`. The flag and its saturation warning stay documented in Available Commands, since an agent can still choose it. MUL-5442 Co-authored-by: multica-agent * fix(daemon): make the coalesced-id lookup deterministic, not window-dependent (MUL-5442) Review found the previous version could still drop a user instruction. `--since` is not a reliable lower bound for these ids. A retry inherits the previous attempt's coalesced_comment_ids verbatim (queries/agent.sql RetryTask) while the anchor is recomputed from the last STARTED task's started_at (GetLastTaskStartedAtForIssueAndAgent), so an inherited id can predate the anchor. If any unrelated comment lands after the anchor, NewCommentsSince is populated, the prompt sent the agent at the window, and the inherited id was simply not in the result. It was not a precise fetch in the other direction either -- the window also carries the trigger comment and unrelated comments. Replace the window-or-heuristic pair with one deterministic contract: `--thread` accepts ANY comment id, reply or root, and the server resolves it to the containing thread. So every id is fetchable directly and bounded, with `--before`/`--before-id` paging when it is older than the tail window. That pass now runs unconditionally; `--since` is demoted to an optional prefetch, described as a candidate window rather than an exact fetch. This also removes the anchorless branch's "expand the threads whose last_activity_at is recent" heuristic. Completeness of user instructions is not a good place for the agent to guess. Tests assert the contract rather than the command spelling: the per-id lookup, the reply-id capability, cursor paging and the "account for every id" rule are required in both shapes; the anchored shape must not overpromise the window; the anchorless shape must carry no --since and no recency heuristic. MUL-5442 Co-authored-by: multica-agent --------- Co-authored-by: Bohan-J Co-authored-by: multica-agent --- server/internal/daemon/prompt.go | 36 ++++++++++++- server/internal/daemon/prompt_test.go | 76 +++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 7 deletions(-) diff --git a/server/internal/daemon/prompt.go b/server/internal/daemon/prompt.go index dd63a23976..36d67cabdc 100644 --- a/server/internal/daemon/prompt.go +++ b/server/internal/daemon/prompt.go @@ -275,8 +275,40 @@ func buildCommentPrompt(task Task, provider string) string { } fmt.Fprintf(&b, "\nIf you need the surrounding discussion for any of them, fetch its thread with `multica issue comment list %s --thread --tail 30 --output json` using the thread id shown above.\n\n", task.IssueID) } else if len(task.CoalescedCommentIDs) > 0 { - fmt.Fprintf(&b, "This run also covers %d earlier comment(s) posted before it started — you must read and address them too, not just the one above: %s. These may be in DIFFERENT threads, so do not assume they share the triggering thread; fetch each by pulling the issue-wide discussion with `multica issue comment list %s --recent 30 --output json` (expand with `--full` if a thread is folded) and locate the ids above.\n\n", - len(task.CoalescedCommentIDs), strings.Join(task.CoalescedCommentIDs, ", "), task.IssueID) + // MUL-5442: this fallback used to send the agent at `--recent 30`. + // That flag caps THREADS, not comments, and every returned thread + // carries all of its descendants — so on an issue with fewer than 30 + // root threads it returned the entire comment history to locate a + // handful of ids. It also contradicted the brief's own catch-up step, + // which tells the agent to read in two bounded steps and never make + // one bulk pull (MUL-5372): the platform was recommending exactly the + // shape it forbids elsewhere. + // + // The replacement is a per-id lookup, which is what makes it + // deterministic: `--thread` accepts ANY comment id, reply or root, and + // the server resolves it to the containing thread. So each id can be + // fetched directly and bounded, without knowing its thread and without + // guessing which threads look recent. + // + // `--since` is only a prefetch, never the guarantee. Two ways it can + // miss an id, so the per-id pass below is unconditional: + // - A retry inherits the previous attempt's coalesced_comment_ids + // verbatim (queries/agent.sql RetryTask), while the anchor is + // recomputed from the last STARTED task's started_at + // (GetLastTaskStartedAtForIssueAndAgent). An inherited id can + // therefore predate the anchor. + // - The anchor is only populated when some comment landed after it, + // which is independent of where these ids sit. + // It is also not a precise fetch in the other direction: the window + // carries the trigger comment and unrelated comments too. + fmt.Fprintf(&b, "This run also covers %d earlier comment(s) posted before it started — you must read and address every one of them, not just the one above: %s. They may be in DIFFERENT threads, so do not assume they share the triggering thread.\n\n", + len(task.CoalescedCommentIDs), strings.Join(task.CoalescedCommentIDs, ", ")) + if task.NewCommentsSince != "" { + fmt.Fprintf(&b, "Start with `multica issue comment list %s --since %s --output json`. Treat that as a candidate window, not a guarantee — it also carries unrelated comments, and a retried run can carry ids older than the window. Check every id above against the result.\n\n", + task.IssueID, task.NewCommentsSince) + } + fmt.Fprintf(&b, "Fetch each id you still need directly: `multica issue comment list %s --thread --tail 30 --output json`. `--thread` accepts a reply id, not just a thread root, so you do not need to know which thread the comment lives in. If it is older than those 30 replies, page back with the `Next reply cursor` values (`--before` / `--before-id`) until it appears. Do not finish this turn until every id above is accounted for.\n\n", + task.IssueID) } if task.TriggerAuthorType == "agent" { b.WriteString("⚠️ The triggering comment was posted by another agent. Decide whether a reply is warranted. If you produced actual work this turn (investigated, fixed something, answered a real question), post the result as a normal reply — that is NOT a noise comment, and the standard rule that final results must be delivered via comment still applies. If the triggering comment was a pure acknowledgment, thanks, or sign-off AND you produced no work this turn, do NOT reply — and do NOT post a comment saying 'No reply needed' or similar. Simply exit with no output. Silence is the preferred way to end agent-to-agent threads. If you do reply, do not @mention the other agent as a sign-off (that re-triggers them and starts a loop).\n\n") diff --git a/server/internal/daemon/prompt_test.go b/server/internal/daemon/prompt_test.go index f2e91cfa7c..2efde9f749 100644 --- a/server/internal/daemon/prompt_test.go +++ b/server/internal/daemon/prompt_test.go @@ -841,9 +841,14 @@ func TestBuildCommentPromptCoalescedCrossThread(t *testing.T) { // TestBuildCommentPromptCoalescedIDsOnlyFallback pins the old-server fallback: // when only coalesced ids are shipped (no embedded detail), the prompt must -// still NOT assume a shared thread and must point at an issue-wide fetch. +// still NOT assume a shared thread, and must reach the ids through a BOUNDED +// read rather than an issue-wide bulk pull (MUL-5442). +// +// The bulk pull is the regression this guards: `--recent N` caps threads, not +// comments, so on a small issue it returns the whole history — and the brief's +// own catch-up step forbids exactly that shape. func TestBuildCommentPromptCoalescedIDsOnlyFallback(t *testing.T) { - task := Task{ + base := Task{ IssueID: "issue-fallback-1", TriggerCommentID: "trigger-newest", TriggerThreadID: "thread-root-A", @@ -851,13 +856,74 @@ func TestBuildCommentPromptCoalescedIDsOnlyFallback(t *testing.T) { TriggerAuthorType: "member", CoalescedCommentIDs: []string{"c-old-1", "c-old-2"}, } - out := BuildPrompt(task, "claude") + t.Run("with since anchor", func(t *testing.T) { + task := base + task.NewCommentsSince = "2026-08-03T06:00:00Z" + out := BuildPrompt(task, "claude") + + want := "multica issue comment list issue-fallback-1 --since 2026-08-03T06:00:00Z --output json" + if !strings.Contains(out, want) { + t.Errorf("id-only fallback should prefetch the window with %q, got:\n%s", want, out) + } + // The window is a prefetch, never the guarantee: a retry inherits the + // prior attempt's coalesced ids verbatim while the anchor is recomputed + // from the last started task, so an inherited id can predate the window. + // The prompt must say so and must not promise an exact fetch. + for _, want := range []string{"candidate window, not a guarantee", "can carry ids older than the window"} { + if !strings.Contains(out, want) { + t.Errorf("anchored fallback must not present --since as complete, missing %q, got:\n%s", want, out) + } + } + for _, banned := range []string{"returns exactly the comments", "precisely"} { + if strings.Contains(out, banned) { + t.Errorf("anchored fallback must not overpromise the window (%q), got:\n%s", banned, out) + } + } + assertBoundedIDOnlyFallback(t, out) + }) + + t.Run("without since anchor", func(t *testing.T) { + // No prior run on this issue, so the server sent no anchor. The per-id + // lookup below is the whole contract here. + out := BuildPrompt(base, "claude") + + if strings.Contains(out, "--since") { + t.Errorf("anchorless fallback must not emit a --since read, got:\n%s", out) + } + // No heuristics: the agent must not be asked to guess which threads look + // recent enough to hold the ids (MUL-5442 review). + if strings.Contains(out, "last_activity_at") { + t.Errorf("anchorless fallback must not rely on a recency heuristic, got:\n%s", out) + } + assertBoundedIDOnlyFallback(t, out) + }) +} + +// assertBoundedIDOnlyFallback holds the completeness contract both fallback +// shapes must satisfy: every listed id is reachable deterministically, through +// bounded reads, without a bulk pull. +func assertBoundedIDOnlyFallback(t *testing.T, out string) { + t.Helper() if strings.Contains(out, "they are in the triggering thread") { t.Errorf("id-only fallback must not assume a shared thread, got:\n%s", out) } - if !strings.Contains(out, "--recent 30") { - t.Errorf("id-only fallback must point at an issue-wide fetch (--recent 30), got:\n%s", out) + if strings.Contains(out, "--recent") { + t.Errorf("id-only fallback must not send the agent at an issue-wide --recent pull (MUL-5442), got:\n%s", out) + } + // The deterministic per-id lookup. `--thread` resolves ANY comment id, so an + // id is reachable without knowing its thread; paging keeps it reachable even + // when it is older than the tail window. + for _, want := range []string{ + "multica issue comment list issue-fallback-1 --thread --tail 30 --output json", + "accepts a reply id", + "Next reply cursor", + "--before-id", + "Do not finish this turn until every id above is accounted for", + } { + if !strings.Contains(out, want) { + t.Errorf("id-only fallback missing per-id completeness guarantee %q, got:\n%s", want, out) + } } for _, id := range []string{"c-old-1", "c-old-2"} { if !strings.Contains(out, id) {