From 08c7bf6fae967fa19b860dccb719bd0ee00e7d35 Mon Sep 17 00:00:00 2001 From: Jiang Bohan Date: Sat, 9 May 2026 15:57:19 +0800 Subject: [PATCH] fix(timeline): address review feedback on pagination removal Three issues caught in PR #2322 review: 1. /timeline broke for stale clients between #2128 and this PR. They send ?limit/?before/?after/?around and parse with the wrapped TimelinePageSchema; the new flat-array response was failing schema validation and falling back to an empty timeline. Restore the wrapped shape on those query params (DESC entries, null cursors, has_more_*=false), keeping the flat ASC array for bare requests. Around-mode now also fills target_index from the merged slice so legacy clients can still scroll-to-anchor without a follow-up. 2. The agent prompts in runtime_config.go and prompt.go still told agents that `multica issue comment list` accepts --limit/--offset and to use `--limit 30` on truncated output. With those flags removed in this PR, new agent runs would hit "unknown flag" or skip context. Update the prompt copy to "returns all comments, capped at 2000; --since for incremental polling". 3. useCreateComment's onSuccess was a bare append to the timeline cache with no id-dedupe, so a fast comment:created WS event firing before onSuccess produced a transient duplicate. Restore the id guard the old prependToLatestPage helper used to provide. Adds two new boundary tests: - TestListTimeline_LegacyWrappedShape_OnPaginationParams - TestListTimeline_LegacyWrappedShape_AroundFillsTargetIndex Co-authored-by: multica-agent --- packages/core/issues/mutations.ts | 11 ++- .../internal/daemon/execenv/runtime_config.go | 9 +- server/internal/daemon/prompt.go | 4 +- server/internal/handler/activity.go | 82 ++++++++++++++++--- server/internal/handler/activity_test.go | 73 +++++++++++++++++ 5 files changed, 156 insertions(+), 23 deletions(-) diff --git a/packages/core/issues/mutations.ts b/packages/core/issues/mutations.ts index 17f9b5019..e9098ae0f 100644 --- a/packages/core/issues/mutations.ts +++ b/packages/core/issues/mutations.ts @@ -327,9 +327,14 @@ export function useCreateComment(issueId: string) { created_at: comment.created_at, updated_at: comment.updated_at, }; - qc.setQueryData(issueKeys.timeline(issueId), (old) => - old ? [...old, entry] : [entry], - ); + // Dedupe by id: the `comment:created` WS event may have already added + // this entry from the broadcast path before this onSuccess fires. Skip + // the append if the entry is already in the cache. + qc.setQueryData(issueKeys.timeline(issueId), (old) => { + if (!old) return [entry]; + if (old.some((e) => e.id === entry.id)) return old; + return [...old, entry]; + }); }, onSettled: () => { qc.invalidateQueries({ queryKey: issueKeys.timeline(issueId) }); diff --git a/server/internal/daemon/execenv/runtime_config.go b/server/internal/daemon/execenv/runtime_config.go index 49c3cb440..eeebaff8b 100644 --- a/server/internal/daemon/execenv/runtime_config.go +++ b/server/internal/daemon/execenv/runtime_config.go @@ -106,7 +106,7 @@ func buildMetaSkillContent(provider string, ctx TaskContextForEnv) string { b.WriteString("### Read\n") b.WriteString("- `multica issue get --output json` — Get full issue details (title, description, status, priority, assignee)\n") b.WriteString("- `multica issue list [--status X] [--priority X] [--assignee X | --assignee-id ] [--limit N] [--offset N] [--full-id] [--output json]` — List issues in workspace (default limit: 50; table output uses routable issue keys; JSON output includes `total`, `has_more` — use offset to paginate when `has_more` is true). Prefer `--assignee-id ` when scripting from `multica workspace members --output json` / `multica agent list --output json`.\n") - b.WriteString("- `multica issue comment list [--limit N] [--offset N] [--since ] --output json` — List comments on an issue (supports pagination; includes id, parent_id for threading)\n") + b.WriteString("- `multica issue comment list [--since ] --output json` — List all comments on an issue (server caps at 2000 rows). Use `--since` for incremental polling.\n") b.WriteString("- `multica issue label list --output json` — List labels currently attached to an issue\n") b.WriteString("- `multica issue subscriber list --output json` — List members/agents subscribed to an issue\n") b.WriteString("- `multica label list --output json` — List all labels defined in the workspace (returns id + name + color)\n") @@ -246,8 +246,8 @@ func buildMetaSkillContent(provider string, ctx TaskContextForEnv) string { // Comment-triggered: focus on reading and replying b.WriteString("**This task was triggered by a NEW comment.** Your primary job is to respond to THIS specific comment, even if you have handled similar requests before in this session.\n\n") fmt.Fprintf(&b, "1. Run `multica issue get %s --output json` to understand the issue context\n", ctx.IssueID) - fmt.Fprintf(&b, "2. Run `multica issue comment list %s --output json` to read the conversation\n", ctx.IssueID) - b.WriteString(" - If the output is very large or truncated, use pagination: `--limit 30` to get the latest 30 comments, or `--since ` to fetch only recent ones\n") + fmt.Fprintf(&b, "2. Run `multica issue comment list %s --output json` to read the conversation (returns all comments, capped server-side at 2000)\n", ctx.IssueID) + b.WriteString(" - For incremental polling, use `--since ` to fetch only comments newer than a known cursor\n") fmt.Fprintf(&b, "3. Find the triggering comment (ID: `%s`) and understand what is being asked — do NOT confuse it with previous comments\n", ctx.TriggerCommentID) b.WriteString("4. **Decide whether a reply is warranted.** If you produced actual work this turn (investigated, fixed, answered a real question), post the result via step 6 — that is a normal reply, not a noise comment. If the triggering comment was a pure acknowledgment / thanks / sign-off from another agent AND you produced no work this turn, do NOT post a reply — and do NOT post a comment saying 'No reply needed' or similar. Simply exit with no output. Silence is a valid and preferred way to end agent-to-agent conversations.\n") b.WriteString("5. If a reply IS warranted: do any requested work first, then **decide whether to include any `@mention` link.** The default is NO mention. Only mention when you are escalating to a human owner who is not yet involved, delegating a concrete new sub-task to another agent for the first time, or the user explicitly asked you to loop someone in. Never @mention the agent you are replying to as a thank-you or sign-off.\n") @@ -258,8 +258,7 @@ func buildMetaSkillContent(provider string, ctx TaskContextForEnv) string { // Assignment-triggered: defer to agent Skills for workflow specifics. b.WriteString("You are responsible for managing the issue status throughout your work.\n\n") fmt.Fprintf(&b, "1. Run `multica issue get %s --output json` to understand your task\n", ctx.IssueID) - fmt.Fprintf(&b, "2. Run `multica issue comment list %s --output json` to read the full comment history — this is mandatory, not optional. Earlier comments often carry context the issue body lacks (e.g. which repo to work in, the prior agent's findings, the reason the issue was reassigned to you). Skipping this step is the most common cause of agents acting on stale or incomplete instructions.\n", ctx.IssueID) - fmt.Fprintf(&b, " - If the output is very large or truncated, use pagination: `--limit 30` to get the latest 30 comments, or `--since ` to fetch only recent ones\n") + fmt.Fprintf(&b, "2. Run `multica issue comment list %s --output json` to read the full comment history (returns all comments, capped server-side at 2000) — this is mandatory, not optional. Earlier comments often carry context the issue body lacks (e.g. which repo to work in, the prior agent's findings, the reason the issue was reassigned to you). Skipping this step is the most common cause of agents acting on stale or incomplete instructions.\n", ctx.IssueID) fmt.Fprintf(&b, "3. Run `multica issue status %s in_progress`\n", ctx.IssueID) b.WriteString("4. Follow your Skills and Agent Identity to complete the task (write code, investigate, etc.)\n") fmt.Fprintf(&b, "5. **Post your final results as a comment — this step is mandatory**: `multica issue comment add %s --content \"...\"`. Your results are only visible to the user if posted via this CLI call; text in your terminal or run logs is NOT delivered.\n", ctx.IssueID) diff --git a/server/internal/daemon/prompt.go b/server/internal/daemon/prompt.go index a199b17a3..92d1e7d93 100644 --- a/server/internal/daemon/prompt.go +++ b/server/internal/daemon/prompt.go @@ -27,7 +27,7 @@ func BuildPrompt(task Task) string { b.WriteString("You are running as a local coding agent for a Multica workspace.\n\n") fmt.Fprintf(&b, "Your assigned issue ID is: %s\n\n", task.IssueID) fmt.Fprintf(&b, "Start by running `multica issue get %s --output json` to understand your task, then complete it.\n", task.IssueID) - fmt.Fprintf(&b, "If you need comment history, `multica issue comment list %s` returns the latest 50 by default — pass --limit or --since to scope older windows. Long issues can have thousands of comments; do not fetch everything blindly.\n", task.IssueID) + fmt.Fprintf(&b, "If you need comment history, `multica issue comment list %s --output json` returns all comments for the issue (server caps at 2000). Pass `--since ` to fetch only comments newer than a known cursor.\n", task.IssueID) return b.String() } @@ -116,7 +116,7 @@ func buildCommentPrompt(task Task) string { } } fmt.Fprintf(&b, "Start by running `multica issue get %s --output json` to understand your task, then decide how to proceed.\n\n", task.IssueID) - fmt.Fprintf(&b, "If you need comment history, `multica issue comment list %s` returns the latest 50 by default — pass --limit or --since to scope older windows. Long issues can have thousands of comments; do not fetch everything blindly.\n\n", task.IssueID) + fmt.Fprintf(&b, "If you need comment history, `multica issue comment list %s --output json` returns all comments for the issue (server caps at 2000). Pass `--since ` to fetch only comments newer than a known cursor.\n\n", task.IssueID) b.WriteString(execenv.BuildCommentReplyInstructions(task.IssueID, task.TriggerCommentID)) return b.String() } diff --git a/server/internal/handler/activity.go b/server/internal/handler/activity.go index a3f4b355d..3800002d5 100644 --- a/server/internal/handler/activity.go +++ b/server/internal/handler/activity.go @@ -41,12 +41,37 @@ type TimelineEntry struct { // data-shape rationale (#1929). const timelineHardCap = 2000 -// ListTimeline returns the full issue timeline (comments + activities merged) -// in chronological order (oldest first). Comments and activities each carry -// their own hard cap to bound the response — paged delivery and cursors were -// removed in #1929 because time-based pagination splits reply threads at page -// boundaries, and the actual data shape (p99 ~30 comments per issue) made the -// cursor machinery pure overhead. +// timelinePaginatedResponse mirrors the wrapper shape produced by the prior +// cursor-paginated ListTimeline (#2128). It is preserved as a backward-compat +// surface for installed Desktop builds and stale Web bundles between #2128 and +// #1929 that send `?limit=`/`?before=`/`?after=`/`?around=` and parse the +// response with the old TimelinePageSchema (entries + cursors). Cursors are +// always nil and `has_more_*` are always false: the new server returns the +// whole timeline in one shot. +type timelinePaginatedResponse struct { + Entries []TimelineEntry `json:"entries"` + NextCursor *string `json:"next_cursor"` + PrevCursor *string `json:"prev_cursor"` + HasMoreBefore bool `json:"has_more_before"` + HasMoreAfter bool `json:"has_more_after"` + TargetIndex *int `json:"target_index,omitempty"` +} + +// ListTimeline returns the full issue timeline (comments + activities merged). +// Two response shapes coexist for boundary compatibility (#1929): +// +// - No pagination params → flat ASC `TimelineEntry[]`. Matches the legacy +// desktop contract (Multica.app ≤ v0.2.25) and the new client. +// - Any of `limit` / `before` / `after` / `around` present → wrapped object +// with DESC entries + null cursors + has_more_*=false. Matches what a +// stale v0.2.26+ build expects when it parses the response with +// TimelinePageSchema; cursor-walking is now a no-op so the client just +// sees a single full page. +// +// Both shapes carry the same set of entries — paging and ordering differ. +// Time-based pagination was removed because it split reply threads at page +// boundaries, and at observed data sizes (p99 ~30 comments per issue) the +// cursor machinery was pure overhead. func (h *Handler) ListTimeline(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "id") issue, ok := h.loadIssueForUser(w, r, id) @@ -73,17 +98,42 @@ func (h *Handler) ListTimeline(w http.ResponseWriter, r *http.Request) { return } - entries := h.mergeTimelineAsc(r, comments, activities) + q := r.URL.Query() + wantWrapped := q.Get("limit") != "" || q.Get("before") != "" || + q.Get("after") != "" || q.Get("around") != "" + + if wantWrapped { + entries := h.mergeTimeline(r, comments, activities, false) + if entries == nil { + entries = []TimelineEntry{} + } + resp := timelinePaginatedResponse{Entries: entries} + // `around=`: locate the anchor in the DESC slice so the legacy + // client can scroll-to-highlight without a follow-up request. + if anchor := q.Get("around"); anchor != "" { + for i, e := range entries { + if e.ID == anchor { + idx := i + resp.TargetIndex = &idx + break + } + } + } + writeJSON(w, http.StatusOK, resp) + return + } + + entries := h.mergeTimeline(r, comments, activities, true) if entries == nil { entries = []TimelineEntry{} } writeJSON(w, http.StatusOK, entries) } -// mergeTimelineAsc merges comments and activities into a single timeline -// ordered by (created_at, id) ascending — oldest first, matching the contract -// the desktop and web frontends both consume. -func (h *Handler) mergeTimelineAsc(r *http.Request, comments []db.Comment, activities []db.ActivityLog) []TimelineEntry { +// mergeTimeline merges comments and activities and returns them sorted by +// (created_at, id). When ascending=true, oldest first (the new flat-array +// contract); otherwise newest first (the wrapped legacy contract). +func (h *Handler) mergeTimeline(r *http.Request, comments []db.Comment, activities []db.ActivityLog, ascending bool) []TimelineEntry { out := make([]TimelineEntry, 0, len(comments)+len(activities)) out = append(out, h.commentsToEntries(r, comments)...) for _, a := range activities { @@ -91,9 +141,15 @@ func (h *Handler) mergeTimelineAsc(r *http.Request, comments []db.Comment, activ } sort.Slice(out, func(i, j int) bool { if out[i].CreatedAt != out[j].CreatedAt { - return out[i].CreatedAt < out[j].CreatedAt + if ascending { + return out[i].CreatedAt < out[j].CreatedAt + } + return out[i].CreatedAt > out[j].CreatedAt } - return out[i].ID < out[j].ID + if ascending { + return out[i].ID < out[j].ID + } + return out[i].ID > out[j].ID }) return out } diff --git a/server/internal/handler/activity_test.go b/server/internal/handler/activity_test.go index 27837f88d..1ef9542ff 100644 --- a/server/internal/handler/activity_test.go +++ b/server/internal/handler/activity_test.go @@ -134,6 +134,79 @@ func TestListTimeline_MergesCommentsAndActivities(t *testing.T) { } } +// fetchTimelineWrapped exercises the legacy wrapped response shape that +// stale Multica.app v0.2.26+ builds still expect — sending any of +// limit/before/after/around makes the server emit a TimelinePage-style +// object (entries DESC, null cursors, has_more_*=false) instead of the new +// flat array. Used to verify the boundary-compat path doesn't regress. +func fetchTimelineWrapped(t *testing.T, issueID, query string) (timelinePaginatedResponse, int) { + t.Helper() + w := httptest.NewRecorder() + req := newRequest("GET", "/api/issues/"+issueID+"/timeline?"+query, nil) + req = withURLParam(req, "id", issueID) + testHandler.ListTimeline(w, req) + var resp timelinePaginatedResponse + if w.Code == http.StatusOK { + json.NewDecoder(w.Body).Decode(&resp) + } + return resp, w.Code +} + +// Boundary-compat: a stale client between #2128 and #1929 sends ?limit=50 +// and parses the response with TimelinePageSchema. The handler must keep +// returning the wrapped object so that path doesn't fall back to an empty +// timeline. +func TestListTimeline_LegacyWrappedShape_OnPaginationParams(t *testing.T) { + issueID := createIssueForTimeline(t, "Legacy wrapped shape test") + commentIDs, _ := seedTimelineEntries(t, issueID, 3, 0) + + resp, status := fetchTimelineWrapped(t, issueID, "limit=50") + if status != http.StatusOK { + t.Fatalf("status = %d, want 200", status) + } + if resp.HasMoreBefore || resp.HasMoreAfter { + t.Errorf("has_more_*: want false/false, got before=%v after=%v", + resp.HasMoreBefore, resp.HasMoreAfter) + } + if resp.NextCursor != nil || resp.PrevCursor != nil { + t.Errorf("cursors: want nil/nil, got next=%v prev=%v", resp.NextCursor, resp.PrevCursor) + } + // DESC order: most recent comment first; activity from issue-creation + // sits at the bottom. + commentEntries := []TimelineEntry{} + for _, e := range resp.Entries { + if e.Type == "comment" { + commentEntries = append(commentEntries, e) + } + } + if got, want := len(commentEntries), len(commentIDs); got != want { + t.Fatalf("comment count = %d, want %d", got, want) + } + for i, e := range commentEntries { + want := commentIDs[len(commentIDs)-1-i] + if e.ID != want { + t.Errorf("DESC entry %d: id = %s, want %s", i, e.ID, want) + } + } +} + +func TestListTimeline_LegacyWrappedShape_AroundFillsTargetIndex(t *testing.T) { + issueID := createIssueForTimeline(t, "Around target index test") + commentIDs, _ := seedTimelineEntries(t, issueID, 5, 0) + anchor := commentIDs[2] // pick a middle comment + + resp, status := fetchTimelineWrapped(t, issueID, "around="+anchor) + if status != http.StatusOK { + t.Fatalf("status = %d, want 200", status) + } + if resp.TargetIndex == nil { + t.Fatalf("target_index: want non-nil for around mode") + } + if got := resp.Entries[*resp.TargetIndex].ID; got != anchor { + t.Errorf("target_index points at %s, want anchor %s", got, anchor) + } +} + func TestListTimeline_EmptyIssue(t *testing.T) { issueID := createIssueForTimeline(t, "Empty timeline test") entries, status := fetchTimeline(t, issueID)