From a41949952f01c47f58cc35fbba02205afd713d41 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 13 Aug 2026 10:50:48 +0300 Subject: [PATCH] mcpsrv: report the projection's clip on get_issue and list_milestones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Max clips every table read at 2000 rows. list_issues has always said so (table_truncated, table_total); the other two answers computed over the same read said nothing, so a rollup over a prefix of the tracker read as arithmetic about the whole of it, and an id in the tail of a big tracker was answered "no such issue" — a claim neither read can support. get_issue now carries the two fields on every answer and splits the miss: a complete read keeps the plain refusal, a clipped one says the id was not among the rows read, names the cap and the true total, and ships the payload (issue: null beside table_truncated) so the two misses are one field apart rather than one adjective apart. list_milestones carries the same two fields. --- mcpsrv/beads.go | 186 ++++++++++++++++++++++++++++++++----------- mcpsrv/beads_test.go | 162 +++++++++++++++++++++++++++++++++---- 2 files changed, 286 insertions(+), 62 deletions(-) diff --git a/mcpsrv/beads.go b/mcpsrv/beads.go index 3260f10b81e5e39f362219fdbf5478c294b6ca20..1728e1c419d223406e30dbd7b89ab46ce49d55c0 100644 --- a/mcpsrv/beads.go +++ b/mcpsrv/beads.go @@ -41,15 +41,23 @@ import ( // not-found of a database it may not. // // What the projection reads and what it therefore cannot say: it reads up to -// beads.Max (2000) rows per table in one pass. list_issues reports that clip -// (table_truncated, table_total), because a board computed over a clipped table -// is a count a caller cannot otherwise check. get_issue, list_milestones and -// list_memories get no such signal from the projection and so make no claim -// about one: an id that is not in the first beads.Max rows of a large tracker -// reads as absent there, and list_issues is where a caller learns that the -// tracker is that large. list_memories has a clip of an entirely different kind -// and does report it — the revision walk's, which is about the history rather -// than about a table (see memoryJSON.Revision). +// beads.Max (2000) rows per table in one pass, and every tool here whose answer +// is computed over such a read reports the clip in the same two fields — +// table_truncated and table_total, the vocabulary list_issues established. A +// board, a milestone rollup or an issue read out of a clipped table describes a +// prefix of the tracker, and a caller has no other way to check that. +// +// get_issue carries one further consequence of the same fact: an id that is not +// among the rows read is not thereby known to be absent. On a complete read the +// miss is the ordinary one ("no such issue"); on a clipped read the answer says +// the id was not in the first beads.Max rows and names the tracker's true total, +// because "there is no such issue" is a claim this projection cannot make there. +// Both are error results, and the clipped one also carries the structured +// payload — so the two are told apart by a field and not only by a sentence. +// +// list_memories has a clip of an entirely different kind and does report it — +// the revision walk's, which is about the history rather than about a table (see +// memoryJSON.Revision). // The caps of docs/DESIGN.mcp.md §9.3 for the issue listing. They are applied // *after* filtering — the limit clips a result set, not a table read — and, like @@ -281,8 +289,14 @@ type activityJSON struct { } type getIssueOutput struct { - Ref string `json:"ref"` - Issue issueJSON `json:"issue"` + Ref string `json:"ref"` + + // Issue is the issue asked for, and **null** when it was not among the rows + // read. Null occurs only together with table_truncated: a complete read that + // does not carry the id is the ordinary miss and has no payload at all. Read + // the two together — null here means "not in the first table_total rows this + // projection read", never "no such issue". + Issue *issueJSON `json:"issue"` // IsEpic reports that this issue is a parent of subtasks, which is what makes // the two rollup counts below meaningful. @@ -311,6 +325,18 @@ type getIssueOutput struct { // History is the comments and the audit trail merged and sorted oldest // first, which is the one place the *story* of an issue is readable. History []activityJSON `json:"history"` + + // TableTruncated is list_issues' flag under list_issues' name, and it is the + // same fact: some table this answer was assembled from exceeded the 2000-row + // cap and came back clipped. Here it covers more tables than a board does — + // the issue's labels, its comments and its events are read for this pane — + // so a true one says the edges, the thread or the history below may be short. + TableTruncated bool `json:"table_truncated"` + + // TableTotal is the number of rows in the issues table at this ref: what + // exists, against the first 2000 that were read. It is what makes both the + // flag above and a null issue checkable rather than a bare warning. + TableTotal int `json:"table_total"` } type listMilestonesInput struct { @@ -374,6 +400,20 @@ type listMilestonesOutput struct { // list with Unlabeled == Total, which is an answer and not an error. Unlabeled int `json:"unlabeled"` Total int `json:"total"` + + // TableTruncated is list_issues' flag under list_issues' name: one of the + // tables this rollup is computed from — issues, labels, dependencies, + // custom_statuses — exceeded the 2000-row cap and came back clipped, so every + // count above is arithmetic over a prefix of the tracker rather than over it. + // A clipped labels table is the quietest of the four: membership itself goes + // missing, so a milestone can lose members rather than merely undercount them. + TableTruncated bool `json:"table_truncated"` + + // TableTotal is the number of rows in the issues table at this ref — what + // exists, against Total, which is what was read. They differ exactly when the + // issues read was clipped, and that difference is the size of what this + // rollup did not see. + TableTotal int `json:"table_total"` } type listMemoriesInput struct { @@ -513,7 +553,11 @@ func (s *Server) registerBeadsTools() { "with the titles and statuses of the other ends, the transitive dependency trees, the " + "comments, and the merged history of comments and audit events oldest first.\n\n" + "`id` is the issue id list_issues reports, e.g. \"bd-42\". An id the tracker does not carry " + - "is answered as such — about this database, which you are looking straight at.\n\n" + + "is answered as such — about this database, which you are looking straight at. On a tracker " + + "of more than 2000 issues that answer changes, because it has to: only the first 2000 rows " + + "were read, so an id that is not among them is reported as *not read* rather than as absent, " + + "with `table_truncated: true` and the tracker's real `table_total` in the payload beside a " + + "null `issue`. read_rows pages past the cap when the tail is really wanted.\n\n" + "When the issue is an epic, `is_epic` is true and `subtasks` carries its children with " + "`subtask_done`/`subtask_total` as the rollup. `depends_on` is what this issue waits on and " + "`depended_on_by` is what waits on it; only an open `blocks` edge actually blocks, while " + @@ -522,8 +566,7 @@ func (s *Server) registerBeadsTools() { "This is the only tool on this surface that carries issue bodies. Use list_issues to find " + "the id, then this one for the issue you actually need.", }, func(ctx context.Context, _ *mcp.CallToolRequest, in getIssueInput) (*mcp.CallToolResult, getIssueOutput, error) { - out, err := s.getIssue(ctx, in) - return nil, out, err + return s.getIssue(ctx, in) }) mcp.AddTool(s.mcp, &mcp.Tool{ @@ -541,7 +584,12 @@ func (s *Server) registerBeadsTools() { "how many issues carry no milestone label at all.\n\n" + "A tracker that uses no milestone labels answers an empty list — that is an answer, not an " + "error. For the full cards of one milestone's members, call list_issues with " + - "`filter.label` set to the label.", + "`filter.label` set to the label.\n\n" + + "`table_truncated` says the rollup was computed over a clipped read — the projection reads " + + "at most 2000 rows of a table in one pass — so every count above describes that prefix and " + + "not the whole tracker; `table_total` is how many issues really exist, against `total`, " + + "which is how many were read. A clipped `labels` table is the quiet one: membership itself " + + "goes missing, so a milestone can lose members rather than merely undercount them.", }, func(ctx context.Context, _ *mcp.CallToolRequest, in listMilestonesInput) (*mcp.CallToolResult, listMilestonesOutput, error) { out, err := s.listMilestones(ctx, in) return nil, out, err @@ -658,46 +706,62 @@ func (s *Server) listIssues(ctx context.Context, in listIssuesInput) (listIssues } // getIssue answers get_issue: one issue whole, bodies included. -func (s *Server) getIssue(ctx context.Context, in getIssueInput) (getIssueOutput, error) { +// +// It is the one handler here that builds its own *mcp.CallToolResult, and only +// on one path: the miss over a clipped read, which is an error result that also +// carries the structured payload (a null issue beside table_truncated and +// table_total). An agent that reads only the sentence learns the same thing, and +// one that decodes the payload can tell that miss from the ordinary one without +// parsing prose. Every other path returns a nil result and lets the SDK build it. +func (s *Server) getIssue(ctx context.Context, in getIssueInput) (*mcp.CallToolResult, getIssueOutput, error) { const tool = "get_issue" var out getIssueOutput id := strings.TrimSpace(in.ID) if id == "" { - return out, errors.New("name the issue to read; list_issues reports the ids of a tracker") + return nil, out, errors.New("name the issue to read; list_issues reports the ids of a tracker") } sess, ref, err := s.openTracker(ctx, tool, in.databaseRef, in.Ref) if err != nil { - return out, err + return nil, out, err } defer sess.Close() data, err := beads.Build(ctx, sess, ref, url.Values{"issue": {id}}) if err != nil { - return out, internalError(err, tool) - } - if data.Issue == nil { - // An ordinary answer about a database the caller can see, in refMiss's - // sense: naming the id back is not a leak, it is what the caller asked - // with. - return out, errors.New(noSuchIssue(in.databaseRef, ref, id)) + return nil, out, internalError(err, tool) } out = getIssueOutput{ - Ref: ref, - Issue: issueOf(data.Issue), - IsEpic: data.Mode == "epic", - DependsOn: edgesOf(data.DependsOn), - DependedOnBy: edgesOf(data.DependedOnBy), - DependsTree: treeOf(data.DependsTree), - DependentTree: treeOf(data.DependentTree), - Subtasks: make([]subtaskJSON, 0, len(data.Subtasks)), - SubtaskDone: data.SubtaskDone, - SubtaskTotal: data.SubtaskTotal, - Comments: make([]commentJSON, 0, len(data.Comments)), - History: make([]activityJSON, 0, len(data.History)), + Ref: ref, + DependsOn: edgesOf(data.DependsOn), + DependedOnBy: edgesOf(data.DependedOnBy), + DependsTree: treeOf(data.DependsTree), + DependentTree: treeOf(data.DependentTree), + Subtasks: make([]subtaskJSON, 0, len(data.Subtasks)), + SubtaskDone: data.SubtaskDone, + SubtaskTotal: data.SubtaskTotal, + Comments: make([]commentJSON, 0, len(data.Comments)), + History: make([]activityJSON, 0, len(data.History)), + TableTruncated: data.Truncated, + TableTotal: data.ShownOf, + } + if data.Issue == nil { + // Two misses, and which one this is belongs to the projection: an issues + // table clipped at beads.Max means the id may sit in the tail nobody read, + // and answering "no such issue" there would state something this service + // does not know (beads.Data.MissingBeyondCap). + if data.MissingBeyondCap() { + return toolMiss(notAmongTheIssuesRead(in.databaseRef, ref, id, data.ShownOf)), out, nil + } + // The read was complete, so the id is genuinely not there. An ordinary + // answer about a database the caller can see, in refMiss's sense: naming + // the id back is not a leak, it is what the caller asked with. + return nil, getIssueOutput{}, errors.New(noSuchIssue(in.databaseRef, ref, id)) } + out.Issue = issueOf(data.Issue) + out.IsEpic = data.Mode == "epic" for _, st := range data.Subtasks { out.Subtasks = append(out.Subtasks, subtaskJSON{ ID: st.ID, @@ -722,7 +786,7 @@ func (s *Server) getIssue(ctx context.Context, in getIssueInput) (getIssueOutput CreatedAt: a.CreatedAt, }) } - return out, nil + return nil, out, nil } // listMilestones answers list_milestones: the milestone: labels rolled up. @@ -742,10 +806,12 @@ func (s *Server) listMilestones(ctx context.Context, in listMilestonesInput) (li } out = listMilestonesOutput{ - Ref: ref, - Milestones: make([]milestoneJSON, 0, len(view.Milestones)), - Unlabeled: view.Unlabeled, - Total: view.Total, + Ref: ref, + Milestones: make([]milestoneJSON, 0, len(view.Milestones)), + Unlabeled: view.Unlabeled, + Total: view.Total, + TableTruncated: view.Truncated, + TableTotal: view.ShownOf, } for _, m := range view.Milestones { entry := milestoneJSON{ @@ -980,12 +1046,15 @@ func laneCategory(slug string) (string, bool) { // --- the projections -------------------------------------------------------- -func issueOf(i *beads.Issue) issueJSON { +// issueOf renders the issue the projection found. It answers a pointer because +// get_issue's field is one: the shape has to be able to say "not read", and only +// a projection that found an issue ever reaches here. +func issueOf(i *beads.Issue) *issueJSON { labels := i.Labels if labels == nil { labels = []string{} } - return issueJSON{ + return &issueJSON{ ID: i.ID, Title: i.Title, Status: i.Status, @@ -1073,7 +1142,34 @@ func notATracker(ref databaseRef, at string) string { ref, at) } -// noSuchIssue is an ordinary answer about a tracker the caller can see. +// noSuchIssue is an ordinary answer about a tracker the caller can see, and it +// is only ever said about a *complete* read: the projection saw every issue +// there is, and this id is not one of them. func noSuchIssue(ref databaseRef, at, id string) string { return fmt.Sprintf("%s has no issue %q at %q; list_issues names the issues there", ref, id, at) } + +// notAmongTheIssuesRead is the other miss: the issues table exceeded the cap, so +// what this service knows is that the id is not in the rows it read — not that +// it does not exist. The sentence carries both numbers the claim rests on (the +// cap and the tracker's true total) so that a caller can check it, and it names +// the way past the cap rather than leaving an agent with a dead end. +func notAmongTheIssuesRead(ref databaseRef, at, id string, total int) string { + return fmt.Sprintf("%s carries %d issues at %q and this projection reads the first %d of them "+ + "in one pass: %q is not among the rows read, which is not the same as saying it does not "+ + "exist (table_truncated: true, table_total: %d). read_rows pages through the whole issues "+ + "table; list_issues with a filter narrows the tracker to a board that fits under the cap.", + ref, total, at, beads.Max, id, total) +} + +// toolMiss is an error result whose text this package wrote — the shape the SDK +// builds for a returned error, built here instead so that the structured payload +// can travel with it. It is used by the one answer that is both a refusal and a +// fact worth decoding (get_issue past the cap); everything else returns an error +// and lets the SDK pack it. +func toolMiss(text string) *mcp.CallToolResult { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: text}}, + } +} diff --git a/mcpsrv/beads_test.go b/mcpsrv/beads_test.go index 8508a1502292374fef3ae3d2f6f6ac288b705755..691381f63a04b9631860a21c6100c49425cbc43b 100644 --- a/mcpsrv/beads_test.go +++ b/mcpsrv/beads_test.go @@ -1,6 +1,7 @@ package mcpsrv_test import ( + "encoding/json" "strings" "testing" @@ -201,7 +202,14 @@ func plainTables(prefix, title string) []fakeTable { } // bulkTables is a tracker larger than beads.Max (2000), which is the only way to -// see the projection's own clip reported. +// see the projection's own clip reported. The ids run bulk-0 upwards in table +// order, so everything from bulk-2000 on is the tail a capped read never sees — +// bulkTail names it. +// +// Two issues carry milestone:m1: bulk-3, which is read, and the last one, which +// is not. The rollup over a clipped read therefore reports half a milestone, and +// the labels table itself stays far under the cap, so the shortfall can only +// have come from the issues read. func bulkTables(n int) []fakeTable { rows := make([][]string, 0, n) for i := range n { @@ -212,9 +220,17 @@ func bulkTables(n int) []fakeTable { return []fakeTable{ beadsTable("issues", issueColumns, rows), beadsTable("dependencies", []string{"issue_id", "depends_on_issue_id", "type"}, nil), + beadsTable("labels", []string{"issue_id", "label"}, [][]string{ + {"bulk-3", "milestone:m1"}, + {bulkTail(n), "milestone:m1"}, + }), } } +// bulkTail is the id of the last issue of a bulk tracker of n: it exists, and it +// is past the first beads.Max rows any projection here reads. +func bulkTail(n int) string { return "bulk-" + itoa(n-1) } + func itoa(n int) string { if n == 0 { return "0" @@ -429,18 +445,32 @@ type ( } getIssueResult struct { - Ref string `json:"ref"` - Issue issueResult `json:"issue"` - IsEpic bool `json:"is_epic"` - DependsOn []edgeResult `json:"depends_on"` - DependedOnBy []edgeResult `json:"depended_on_by"` - DependsTree []treeNodeResult `json:"depends_tree"` - DependentTree []treeNodeResult `json:"dependent_tree"` - Subtasks []subtaskResult `json:"subtasks"` - SubtaskDone int `json:"subtask_done"` - SubtaskTotal int `json:"subtask_total"` - Comments []commentResult `json:"comments"` - History []activityResult `json:"history"` + Ref string `json:"ref"` + Issue issueResult `json:"issue"` + IsEpic bool `json:"is_epic"` + DependsOn []edgeResult `json:"depends_on"` + DependedOnBy []edgeResult `json:"depended_on_by"` + DependsTree []treeNodeResult `json:"depends_tree"` + DependentTree []treeNodeResult `json:"dependent_tree"` + Subtasks []subtaskResult `json:"subtasks"` + SubtaskDone int `json:"subtask_done"` + SubtaskTotal int `json:"subtask_total"` + Comments []commentResult `json:"comments"` + History []activityResult `json:"history"` + TableTruncated bool `json:"table_truncated"` + TableTotal int `json:"table_total"` + } + + // getIssueMissResult is the payload that travels with the one refusal on this + // surface that carries one: get_issue over a clipped read. Issue is a pointer + // here precisely because the assertion is that it arrives as null — an empty + // issue object would be a placeholder a client could not tell from an issue + // with no fields set. + getIssueMissResult struct { + Ref string `json:"ref"` + Issue *issueResult `json:"issue"` + TableTruncated bool `json:"table_truncated"` + TableTotal int `json:"table_total"` } milestoneMemberResult struct { @@ -472,10 +502,12 @@ type ( } listMilestonesResult struct { - Ref string `json:"ref"` - Milestones []milestoneResult `json:"milestones"` - Unlabeled int `json:"unlabeled"` - Total int `json:"total"` + Ref string `json:"ref"` + Milestones []milestoneResult `json:"milestones"` + Unlabeled int `json:"unlabeled"` + Total int `json:"total"` + TableTruncated bool `json:"table_truncated"` + TableTotal int `json:"table_total"` } ) @@ -500,6 +532,19 @@ func listMilestones(t *testing.T, s *mcp.ClientSession, a map[string]any) listMi return out } +// decodeMiss reads the structured payload of an *error* result, which decode +// refuses on purpose. One answer here carries one — get_issue over a read that +// was clipped, where "not found" is a fact about the read rather than about the +// tracker — and this is how a client would pick it up. +func decodeMiss(t *testing.T, res *mcp.CallToolResult, out any) { + t.Helper() + require.True(t, res.IsError, "not an error result: %s", resultJSON(t, res)) + require.NotNil(t, res.StructuredContent, "an error result with no payload to decode") + raw, err := json.Marshal(res.StructuredContent) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(raw, out)) +} + // filter builds the nested filter argument, so a call in a test reads as the one // constraint it is about. func filter(kv ...any) map[string]any { @@ -822,6 +867,68 @@ func TestGetIssueOfAnUnknownIDIsAPlainMiss(t *testing.T) { assert.NotContains(t, text, "no database", "this is not the masked not-found") } +// An issue read out of a tracker larger than the cap is a whole issue, and the +// answer still says what it was read from: the edges, the thread and the history +// below it were assembled from a prefix of the tables. +func TestGetIssueReportsTheProjectionsOwnClip(t *testing.T) { + session := beadsServer(t) + + clipped := getIssue(t, session, args("bulk", "id", "bulk-7")) + assert.Equal(t, "bulk-7", clipped.Issue.ID, "an issue inside the rows read is answered whole") + assert.True(t, clipped.TableTruncated, "and the read it came out of is reported") + assert.Equal(t, bulkIssues, clipped.TableTotal, "with the tracker's true row count beside it") + + whole := getIssue(t, session, args("board", "id", "bd-3")) + assert.False(t, whole.TableTruncated, "a tracker read whole claims no clip") + assert.Equal(t, 8, whole.TableTotal, "and reports its size anyway, so the flag is checkable") +} + +// The id is in the tracker and past the rows the projection read. "No such +// issue" would be a claim this service cannot support, so it says what it +// actually knows — naming the cap, the true total, and the way past both. +func TestGetIssueOfAnIDPastTheCapIsNotAbsence(t *testing.T) { + tail := bulkTail(bulkIssues) + res := call(t, beadsServer(t), "get_issue", args("bulk", "id", tail)) + require.True(t, res.IsError, "the issue was not read, so this is not an answer about the issue") + + text := errorText(res) + assert.Contains(t, text, tail, "the id is named back") + assert.Contains(t, text, "2000", "the cap the read stopped at") + assert.Contains(t, text, itoa(bulkIssues), "and the tracker's true total") + assert.NotContains(t, text, "has no issue", "which is the claim a clipped read cannot make") + assert.Contains(t, text, "read_rows", "and the way past the cap is named") + + var out getIssueMissResult + decodeMiss(t, res, &out) + assert.Nil(t, out.Issue, "not read is a null issue, never an empty one") + assert.True(t, out.TableTruncated) + assert.Equal(t, bulkIssues, out.TableTotal) + assert.Equal(t, "main", out.Ref) +} + +// The two ways an issue can be missing are one field apart, not one adjective +// apart: a caller decides between "does not exist" and "was not read" without +// reading the sentence. +func TestTheTwoMissesAreToldApartByThePayload(t *testing.T) { + session := beadsServer(t) + + complete := call(t, session, "get_issue", args("board", "id", "bd-999")) + require.True(t, complete.IsError) + assert.Nil(t, complete.StructuredContent, + "a complete read's miss is the plain refusal it always was") + assert.Contains(t, errorText(complete), "has no issue") + + clipped := call(t, session, "get_issue", args("bulk", "id", bulkTail(bulkIssues))) + require.True(t, clipped.IsError) + require.NotNil(t, clipped.StructuredContent, + "and the clipped read's miss carries what makes it checkable") + + var out getIssueMissResult + decodeMiss(t, clipped, &out) + assert.True(t, out.TableTruncated, "the one field the two answers differ in") + assert.Equal(t, bulkIssues, out.TableTotal) +} + func TestGetIssueNeedsAnID(t *testing.T) { res := call(t, beadsServer(t), "get_issue", args("board", "id", " ")) require.True(t, res.IsError) @@ -888,6 +995,27 @@ func TestListMilestonesOnATrackerWithNoMilestones(t *testing.T) { assert.Equal(t, 2, out.Unlabeled) } +// A rollup computed over a clipped read is not arithmetic about the whole +// tracker, and the answer says so in list_issues' own two fields rather than +// presenting short counts as the tracker's. +func TestListMilestonesReportsTheProjectionsOwnClip(t *testing.T) { + session := beadsServer(t) + + got := listMilestones(t, session, args("bulk")) + assert.True(t, got.TableTruncated, "the rollup below is arithmetic over a partial read") + assert.Equal(t, bulkIssues, got.TableTotal, "issues that exist") + assert.Equal(t, 2000, got.Total, "issues that were read") + assert.Equal(t, 1999, got.Unlabeled, "every read issue but bulk-3 carries no milestone label") + + require.Len(t, got.Milestones, 1) + assert.Equal(t, 1, got.Milestones[0].Total, + "m1's other member is the tail issue, which sits past the cap — the count is short and the answer says why") + + whole := listMilestones(t, session, args("board")) + assert.False(t, whole.TableTruncated, "a tracker read whole claims no clip") + assert.Equal(t, whole.Total, whole.TableTotal, "nothing was left behind, so the two agree") +} + // A milestone member carries only what the rollup computes. The dependency // counts and the ready flag are absent rather than zero, because a 0 and a false // an agent cannot check are four lies per member.