From 630735084650ff50de6b40673206da4f2c338198 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 13 Aug 2026 10:27:28 +0300 Subject: [PATCH] mcpsrv: answer a real NULL as null --- mcpsrv/browse.go | 58 +++++++++++++++---- mcpsrv/browse_nulls_test.go | 112 ++++++++++++++++++++++++++++++++++++ mcpsrv/mcpsrv_test.go | 25 ++++++++ 3 files changed, 185 insertions(+), 10 deletions(-) create mode 100644 mcpsrv/browse_nulls_test.go diff --git a/mcpsrv/browse.go b/mcpsrv/browse.go index 43b81ff2ced0d8a2f427716425cdd60e78bce79e..ee1e9d33b4d7a0813cfce3d19295e3d4a792f0f5 100644 --- a/mcpsrv/browse.go +++ b/mcpsrv/browse.go @@ -159,10 +159,20 @@ type readRowsOutput struct { // Rows are rendered strings and not typed values: this surface reads a bare // store without a SQL engine, so what it has is the stored value formatted. - // A NULL cell reads as "NULL" and an unprintable one as "", neither - // of which is distinguishable from a row that stores those very strings — - // which is a limit worth knowing rather than a bug to work around. - Rows [][]string `json:"rows"` + // + // A cell that holds no value is null rather than a string. browse renders a + // real NULL as the text "NULL", which is exactly what a row storing those + // four characters renders as — harmless on a page, where a reader sees the + // same thing either way, and not harmless here: this tool hands rows to a + // machine with no schema beside them, and "is there a value in this cell?" + // is not a question an agent can answer from the string afterwards. The mask + // browse carries beside its rows is what makes the two distinguishable, and + // spending it on a JSON null is the shape that needs no explaining. + // + // An unprintable value still reads as "" and is still not + // distinguishable from a row that stores that text. That one stays: it + // answers what a value *is*, and the caller can see it is there. + Rows [][]*string `json:"rows"` Offset int `json:"offset"` @@ -298,8 +308,8 @@ func (s *Server) registerBrowseTools() { Description: "Read one page of rows from one table of a hosted Dolt database.\n\n" + "`columns` names the cells of every row in order (primary-key columns first for a keyed " + "table). Cells are rendered strings, because this surface reads a bare store without a SQL " + - "engine: a NULL cell reads as `NULL` and an unprintable one as ``, neither " + - "distinguishable from a row that stores those strings literally.\n\n" + + "engine: a cell that holds no value is `null`, and an unprintable one is the string " + + "``.\n\n" + "`limit` defaults to 100 and is capped at 500; the `limit` in the answer is the one actually " + "applied, so a larger request is visibly answered at the cap. `total` is the number of rows " + "in the whole table, and `truncated` says rows remain after this page — read them by raising " + @@ -471,7 +481,7 @@ func (s *Server) readRows(ctx context.Context, in readRowsInput) (readRowsOutput Ref: ref, Table: table, Columns: page.Columns, - Rows: page.Rows, + Rows: nullableCells(page), Offset: page.Offset, Limit: limit, Total: page.Total, @@ -484,12 +494,40 @@ func (s *Server) readRows(ctx context.Context, in readRowsInput) (readRowsOutput if out.Columns == nil { out.Columns = []string{} } - if out.Rows == nil { - out.Rows = [][]string{} - } return out, nil } +// nullableCells reads a page's rows through the NULL mask beside them: a cell +// the mask reports as holding no value becomes a nil pointer, which is the JSON +// null a caller decodes, and every other cell is the string browse rendered. +// +// It never returns nil, so an empty page answers an empty array rather than a +// null one — the rule read.go states for every list on this surface. +// +// A page whose mask is missing, or shorter than its rows, reports values and +// never nulls. browse builds a mask parallel to the rows for every page it +// returns, so that arm is about a page this package did not get from browse; +// claiming an absence for a cell nothing was observed about would be inventing +// the very answer the mask exists to carry. +func nullableCells(page *browse.RowPage) [][]*string { + out := make([][]*string, 0, len(page.Rows)) + for i, row := range page.Rows { + var mask []bool + if i < len(page.Nulls) { + mask = page.Nulls[i] + } + cells := make([]*string, len(row)) + for j, v := range row { + if j < len(mask) && mask[j] { + continue // nil: this cell holds no value at all + } + cells[j] = &v + } + out = append(out, cells) + } + return out +} + // getCommitLog answers get_commit_log: a page of history with the cursor that // continues it. func (s *Server) getCommitLog(ctx context.Context, in getCommitLogInput) (getCommitLogOutput, error) { diff --git a/mcpsrv/browse_nulls_test.go b/mcpsrv/browse_nulls_test.go new file mode 100644 index 0000000000000000000000000000000000000000..f35dc4d6d26e4c3e3783938ddbca5b5cadaabb81 --- /dev/null +++ b/mcpsrv/browse_nulls_test.go @@ -0,0 +1,112 @@ +package mcpsrv_test + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-dolt/browse" + "sourcecraft.dev/bigbes/sr-ht-dolt/core" + "sourcecraft.dev/bigbes/sr-ht-dolt/mcpsrv" +) + +// read_rows and the one question a rendered string cannot answer: is there a +// value in this cell at all? +// +// browse renders a real NULL as the text "NULL", which is what a row storing +// those four characters renders as too. On a page the two are interchangeable — +// a reader sees the same thing either way — but this tool hands rows to a +// machine with no schema beside them, and an agent cannot recover the +// distinction afterwards. So the mask browse carries beside its rows is spent +// here: an absent value is JSON null, a stored "NULL" is the string. + +// nullsTable is a table holding both halves of the ambiguity in one column: +// row 2 stores no value, row 3 stores the text "NULL", and browse renders both +// as "NULL". Only the mask tells them apart. +func nullsTable() fakeTable { + return fakeTable{ + name: "cells", + cols: []browse.ColumnInfo{ + {Name: "id", Type: "int", PrimaryKey: true}, + {Name: "body", Type: "text", Nullable: true}, + }, + rows: [][]string{ + {"1", "plain"}, + {"2", "NULL"}, // no value: browse rendered the placeholder + {"3", "NULL"}, // the four characters, actually stored + }, + nulls: [][]bool{ + {false, false}, + {false, true}, + {false, false}, + }, + } +} + +// nullableRowsResult is read_rows decoded into cells that can be null, which is +// the shape this tool answers: [][]*string, where a nil cell is a JSON null. +type nullableRowsResult struct { + Columns []string `json:"columns"` + Rows [][]*string `json:"rows"` +} + +func TestReadRowsAnswersARealNullAsNullAndAStoredNullAsAString(t *testing.T) { + res := call(t, connect(t, nullsSurface(t), nil), "read_rows", args("cells", "table", "cells")) + require.False(t, res.IsError, "%s", errorText(res)) + + var out nullableRowsResult + decode(t, res, &out) + + require.Equal(t, []string{"id", "body"}, out.Columns) + require.Len(t, out.Rows, 3) + + require.NotNil(t, out.Rows[0][1]) + assert.Equal(t, "plain", *out.Rows[0][1]) + + assert.Nil(t, out.Rows[1][1], "a cell that holds no value is null, not the string \"NULL\"") + + require.NotNil(t, out.Rows[2][1], "a row that stores the text \"NULL\" still has a value") + assert.Equal(t, "NULL", *out.Rows[2][1]) + + // A key is never null, and neither is a cell that holds an ordinary value: + // the mask is spent on the one column that needed it and nowhere else. + for i, row := range out.Rows { + require.NotNilf(t, row[0], "row %d: the primary key holds a value", i) + } +} + +// The distinction has to survive the wire, not merely the Go struct: what an +// agent reads is the JSON, so the null is asserted there too. +func TestReadRowsPutsANullOnTheWire(t *testing.T) { + res := call(t, connect(t, nullsSurface(t), nil), "read_rows", args("cells", "table", "cells")) + require.False(t, res.IsError, "%s", errorText(res)) + require.NotNil(t, res.StructuredContent) + + raw, err := json.Marshal(res.StructuredContent) + require.NoError(t, err) + assert.Contains(t, string(raw), `[["1","plain"],["2",null],["3","NULL"]]`, + "the absent value is a JSON null and the stored text is a string") +} + +// nullsSurface is the server over the cells database. +func nullsSurface(t *testing.T) *mcpsrv.Server { + t.Helper() + + repos := &fakeRepos{acl: map[int]map[int]core.AccessMode{}} + repos.repos = append(repos.repos, &core.Repo{ + ID: 1, + Name: "cells", + OwnerID: aliceID, + OwnerName: "alice", + Path: storePath("alice", "cells"), + Visibility: core.VisibilityPublic, + }) + + opener := &fakeOpener{sessions: map[string]*fakeSession{ + storePath("alice", "cells"): smallStore("main", "cccc", []fakeTable{nullsTable()}), + }} + + return newServer(t, repos, opener) +} diff --git a/mcpsrv/mcpsrv_test.go b/mcpsrv/mcpsrv_test.go index d6fde66467675d4abdf8adfa728057b320b1a291..01c28df26a3cd31cf63b16b4c4eb1306a6db2911 100644 --- a/mcpsrv/mcpsrv_test.go +++ b/mcpsrv/mcpsrv_test.go @@ -200,6 +200,12 @@ type fakeTable struct { name string cols []browse.ColumnInfo rows [][]string + + // nulls is the NULL mask browse carries beside its rows, parallel to rows. + // A table that leaves it unset holds a value in every cell, and pageOf builds + // the all-false mask browse would have returned for it — a fixture never + // answers a page without a mask, because the store never does either. + nulls [][]bool } func (t fakeTable) info() browse.TableInfo { @@ -596,16 +602,35 @@ func pageOf(t fakeTable, offset, limit int) *browse.RowPage { page := &browse.RowPage{ Columns: t.columns(), Rows: [][]string{}, + Nulls: [][]bool{}, Offset: offset, Total: len(t.rows), } if offset < len(t.rows) { end := min(offset+limit, len(t.rows)) page.Rows = append(page.Rows, t.rows[offset:end]...) + page.Nulls = append(page.Nulls, t.mask(offset, end)...) } return page } +// mask is the NULL mask of rows [lo, hi), sliced in lockstep with them. A table +// that declared none holds no NULL anywhere, so the mask is all false — the +// shape browse returns for such a table, rather than an absent one. +func (t fakeTable) mask(lo, hi int) [][]bool { + if t.nulls != nil { + if len(t.nulls) != len(t.rows) { + panic("fake: the NULL mask of " + t.name + " is not parallel to its rows") + } + return t.nulls[lo:hi] + } + out := make([][]bool, 0, hi-lo) + for _, r := range t.rows[lo:hi] { + out = append(out, make([]bool, len(r))) + } + return out +} + func (s *fakeSession) CommitSummary(_ context.Context, hashStr string) (*browse.CommitDiff, error) { hash := s.resolve(hashStr) if hash == "" {