~bigbes/sr-ht-dolt

c82f106f9e36a8ce14539fdc64d0083b98a90773 — Eugene Blikh 5 days ago 6307350
beads: stop flattening a stored NULL string
M beads/build.go => beads/build.go +11 -11
@@ 77,7 77,7 @@ func Build(ctx context.Context, sess BrowseSession, ref string, query url.Values

	// Bucket every matching issue into exactly one lane.
	var rolling, linedUp, stalled, pastStand []Card
	for _, r := range issues.Rows {
	for _, r := range rowsOf(issues) {
		id := cell(issueCols, r, "id")
		if !filter.matches(id, r, issueCols, labelsByIssue[id]) {
			continue


@@ 182,9 182,9 @@ func buildDetail(
	// id → (title, status, whole row) for edge labels and the subtask rollup.
	titleByIssue := map[string]string{}
	statusByIssue := map[string]string{}
	rowByID := make(map[string][]string, len(issues.Rows))
	var row []string
	for _, r := range issues.Rows {
	rowByID := make(map[string]rowCells, len(issues.Rows))
	var row rowCells
	for _, r := range rowsOf(issues) {
		id := cell(issueCols, r, "id")
		titleByIssue[id] = cell(issueCols, r, "title")
		statusByIssue[id] = cell(issueCols, r, "status")


@@ 195,7 195,7 @@ func buildDetail(
	}

	data := &Data{Mode: "detail", Truncated: clip.truncated, ShownOf: clip.issuesTotal}
	if row == nil {
	if row.values == nil {
		// Unknown id: a detail pane with a nil Issue. Whether that is an answer or
		// an admission is Data.MissingBeyondCap's to tell — when the issues table
		// was clipped at Max the id may simply live in the tail that was never


@@ 247,7 247,7 @@ func buildDetail(
			Closed:  statusCategory(st, catByStatus) == "closed",
		}
	}
	for _, r := range deps.Rows {
	for _, r := range rowsOf(deps) {
		from := cell(depCols, r, "issue_id")
		to := cell(depCols, r, "depends_on_issue_id")
		typ := cell(depCols, r, "type")


@@ 292,7 292,7 @@ func buildDetail(
	// flat Depends-on / Depended-on-by lists can't show, without duplicating them.
	outAdj := map[string][]depLink{} // id → things it depends on
	inAdj := map[string][]depLink{}  // id → things that depend on it
	for _, r := range deps.Rows {
	for _, r := range rowsOf(deps) {
		from := cell(depCols, r, "issue_id")
		to := cell(depCols, r, "depends_on_issue_id")
		if from == "" || to == "" {


@@ 316,7 316,7 @@ func buildDetail(
	if comments, commentsTotal, err := readRowsOptional(ctx, sess, ref, "comments"); err == nil && comments != nil {
		data.Truncated = data.Truncated || commentsTotal > Max
		ccols := indexCols(comments.Columns)
		for _, r := range comments.Rows {
		for _, r := range rowsOf(comments) {
			if cell(ccols, r, "issue_id") != want {
				continue
			}


@@ 339,7 339,7 @@ func buildDetail(
	if events, eventsTotal, err := readRowsOptional(ctx, sess, ref, "events"); err == nil && events != nil {
		data.Truncated = data.Truncated || eventsTotal > Max
		ecols := indexCols(events.Columns)
		for _, r := range events.Rows {
		for _, r := range rowsOf(events) {
			if cell(ecols, r, "issue_id") != want {
				continue
			}


@@ 365,7 365,7 @@ func buildDetail(
// values and label names across all issues, sorted, for the filter dropdowns.
func collectFilterOptions(issues *browse.RowPage, cols map[string]int, labelsByIssue map[string][]string) FilterOptions {
	types, prios, assignees, labels := map[string]bool{}, map[string]bool{}, map[string]bool{}, map[string]bool{}
	for _, r := range issues.Rows {
	for _, r := range rowsOf(issues) {
		if t := cell(cols, r, "issue_type"); t != "" {
			types[t] = true
		}


@@ 392,7 392,7 @@ func collectFilterOptions(issues *browse.RowPage, cols map[string]int, labelsByI
// issueCreatedAt maps issue id → created_at string, for lane sorting.
func issueCreatedAt(issues *browse.RowPage, cols map[string]int) map[string]string {
	m := make(map[string]string, len(issues.Rows))
	for _, r := range issues.Rows {
	for _, r := range rowsOf(issues) {
		m[cell(cols, r, "id")] = cell(cols, r, "created_at")
	}
	return m

M beads/events.go => beads/events.go +8 -3
@@ 97,11 97,16 @@ func jsonPairs(raw string) string {
	return strings.Join(parts, ", ")
}

// decodeJSONObject parses raw into a map, tolerating the browse NULL placeholder
// and non-object payloads (returns nil rather than erroring).
// decodeJSONObject parses raw into a map, tolerating an empty value and
// non-object payloads (returns nil rather than erroring).
//
// It used to special-case the string "NULL" as well, back when that string was
// how an absent value reached it. cell answers an absent value as "" now, so a
// "NULL" arriving here is four characters a row actually stores — which is not a
// JSON object, and takes the same nil the parse error gives it.
func decodeJSONObject(raw string) map[string]any {
	raw = strings.TrimSpace(raw)
	if raw == "" || raw == "NULL" {
	if raw == "" {
		return nil
	}
	var m map[string]any

M beads/memory.go => beads/memory.go +2 -2
@@ 158,7 158,7 @@ func BuildMemories(ctx context.Context, sess MemorySession, ref string, query ur
	cols := indexCols(rows.Columns)
	raw := map[string]string{}
	texts := map[string]string{}
	for _, r := range rows.Rows {
	for _, r := range rowsOf(rows) {
		key := cell(cols, r, "key")
		if !strings.HasPrefix(key, memoryPrefix) {
			continue


@@ 346,7 346,7 @@ func memoryValuesAt(ctx context.Context, sess MemorySession, at string) (map[str
		return out, nil
	}
	cols := indexCols(rows.Columns)
	for _, r := range rows.Rows {
	for _, r := range rowsOf(rows) {
		if key := cell(cols, r, "key"); key != "" {
			out[key] = cell(cols, r, "value")
		}

M beads/milestones.go => beads/milestones.go +2 -2
@@ 85,7 85,7 @@ func BuildMilestones(ctx context.Context, sess BrowseSession, ref string) (*Mile
	parentsByChild := map[string][]string{}
	if deps != nil {
		cols := indexCols(deps.Columns)
		for _, r := range deps.Rows {
		for _, r := range rowsOf(deps) {
			if !strings.EqualFold(cell(cols, r, "type"), "parent-child") {
				continue
			}


@@ 104,7 104,7 @@ func BuildMilestones(ctx context.Context, sess BrowseSession, ref string) (*Mile
	byLabel := map[string]*MilestoneDetail{}
	cardsByLabel := map[string][]Card{}
	unlabeled := 0
	for _, r := range issues.Rows {
	for _, r := range rowsOf(issues) {
		id := cell(issueCols, r, "id")
		cat := statusCategory(cell(issueCols, r, "status"), catByStatus)
		card := Card{

M beads/model.go => beads/model.go +1 -1
@@ 108,7 108,7 @@ func (f Filter) Active() bool {
}

// matches reports whether one issue row passes every set filter.
func (f Filter) matches(id string, row []string, cols map[string]int, labels []string) bool {
func (f Filter) matches(id string, row rowCells, cols map[string]int, labels []string) bool {
	if f.Type != "" && cell(cols, row, "issue_type") != f.Type {
		return false
	}

M beads/prefixes.go => beads/prefixes.go +1 -1
@@ 187,7 187,7 @@ func readPrefix(ctx context.Context, sess BrowseSession, ref string) (string, er
		return "", nil
	}
	cols := indexCols(rows.Columns)
	for _, r := range rows.Rows {
	for _, r := range rowsOf(rows) {
		if cell(cols, r, "key") != prefixKey {
			continue
		}

M beads/ready.go => beads/ready.go +1 -1
@@ 415,7 415,7 @@ func readyCards(ctx context.Context, sess BrowseSession, ref string) ([]Card, er
	labelsByIssue := indexLabels(labels)

	var cards []Card
	for _, r := range issues.Rows {
	for _, r := range rowsOf(issues) {
		id := cell(issueCols, r, "id")
		cat := catByIssue[id]
		blocked := truthy(cell(issueCols, r, "is_blocked")) || depIdx.blockedOpen[id]

M beads/rows.go => beads/rows.go +67 -9
@@ 43,14 43,72 @@ func indexCols(cols []string) map[string]int {
	return m
}

// rowCells is one row of a page together with the NULL mask browse returned
// beside it: the rendered strings, and the answer to "does this cell hold a
// value at all?" that the strings cannot carry.
//
// It is what every projection here iterates and what cell reads, so a row and
// its mask travel together and cannot be paired up wrongly at a call site.
type rowCells struct {
	values []string

	// nulls is browse's mask for this row, or nil for a page that carried none —
	// which is a hand-built page, since browse fills one for every page it
	// returns. See cell for what an absent mask means.
	nulls []bool
}

// rowsOf pairs each row of a page with its own mask. A nil page — an optional
// table that is absent — has no rows, which is what the callers of
// readRowsOptional already treat it as.
//
// A row the mask does not cover gets a nil one rather than an all-false one: not
// knowing whether a cell holds a value is a different answer from knowing that
// it does, and cell reads the two differently.
func rowsOf(page *browse.RowPage) []rowCells {
	if page == nil {
		return nil
	}
	out := make([]rowCells, 0, len(page.Rows))
	for i, r := range page.Rows {
		var mask []bool
		if i < len(page.Nulls) {
			mask = page.Nulls[i]
		}
		out = append(out, rowCells{values: r, nulls: mask})
	}
	return out
}

// cell returns the named column's value for a row, or "" when the column is
// absent, out of range, or the literal browse NULL placeholder.
func cell(cols map[string]int, row []string, name string) string {
// absent, out of range, or holds no value at all.
//
// A cell that holds no value reads as "": every projection renders a missing
// timestamp, assignee or close reason as nothing, and that must not change. What
// the mask changes is the other reading of the same string — browse renders a
// real NULL as the text "NULL", so a row that *stores* those four characters
// rendered identically and was flattened to "" too, which turned a stored title
// into an empty one. The mask beside the row answers which of the two it is, so
// it decides here rather than the string.
//
// A cell no mask covers is read the way this package read every cell before the
// mask existed: "NULL" is absent. That is not a guess dressed up as an answer —
// it is the older reading, kept for the only pages that lack a mask, which are
// the ones built by hand rather than read from a store. browse fills a mask
// parallel to the rows for every page it returns, so no read of a database
// arrives here without one.
func cell(cols map[string]int, row rowCells, name string) string {
	i, ok := cols[name]
	if !ok || i < 0 || i >= len(row) {
	if !ok || i < 0 || i >= len(row.values) {
		return ""
	}
	v := row[i]
	if i < len(row.nulls) {
		if row.nulls[i] {
			return ""
		}
		return row.values[i]
	}
	v := row.values[i]
	if v == "NULL" {
		return ""
	}


@@ 66,7 124,7 @@ func indexStatusCategories(statuses *browse.RowPage) map[string]string {
		return out
	}
	cols := indexCols(statuses.Columns)
	for _, r := range statuses.Rows {
	for _, r := range rowsOf(statuses) {
		if name := cell(cols, r, "name"); name != "" {
			out[strings.ToLower(name)] = strings.ToLower(cell(cols, r, "category"))
		}


@@ 82,7 140,7 @@ func indexLabels(labels *browse.RowPage) map[string][]string {
		return out
	}
	cols := indexCols(labels.Columns)
	for _, r := range labels.Rows {
	for _, r := range rowsOf(labels) {
		id := cell(cols, r, "issue_id")
		lb := cell(cols, r, "label")
		if id != "" && lb != "" {


@@ 96,7 154,7 @@ func indexLabels(labels *browse.RowPage) map[string][]string {
// closed), which is what decides whether a blocking target still blocks.
func indexIssueCategories(issues *browse.RowPage, cols map[string]int, catByStatus map[string]string) map[string]string {
	out := make(map[string]string, len(issues.Rows))
	for _, r := range issues.Rows {
	for _, r := range rowsOf(issues) {
		out[cell(cols, r, "id")] = statusCategory(cell(cols, r, "status"), catByStatus)
	}
	return out


@@ 125,7 183,7 @@ func indexDeps(deps *browse.RowPage, catByIssue map[string]string) depIndex {
		return idx
	}
	cols := indexCols(deps.Columns)
	for _, r := range deps.Rows {
	for _, r := range rowsOf(deps) {
		from := cell(cols, r, "issue_id")
		to := cell(cols, r, "depends_on_issue_id")
		typ := strings.ToLower(cell(cols, r, "type"))


@@ 151,7 209,7 @@ func indexDeps(deps *browse.RowPage, catByIssue map[string]string) depIndex {
// filter, the cross-database /ready page, the MCP ready_work tool — comes
// through here, because two surfaces that each spelled the rule out would
// disagree the first time it changed.
func readyRow(cat string, blocked bool, row []string, cols map[string]int) bool {
func readyRow(cat string, blocked bool, row rowCells, cols map[string]int) bool {
	return cat == "open" && !blocked &&
		!truthy(cell(cols, row, "is_template")) && !truthy(cell(cols, row, "ephemeral"))
}

A beads/rows_null_test.go => beads/rows_null_test.go +132 -0
@@ 0,0 1,132 @@
package beads

import (
	"context"
	"net/url"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"

	"sourcecraft.dev/bigbes/sr-ht-dolt/browse"
)

// The two readings of the string "NULL", and the mask that tells them apart.
//
// browse renders a cell that holds no value as the text "NULL", which is what a
// row storing those four characters renders as too. Flattening both to "" — as
// this package did before the mask existed — is right for the first and wrong
// for the second: an issue titled "NULL" arrived with an empty title, and no
// projection could tell that it had one.

// nullMaskFixture is one issues table carrying both readings in the same
// columns: i-absent holds no value in title or closed_at, i-stored stores the
// text "NULL" in both.
func nullMaskFixture() *fakeSession {
	issues := &browse.RowPage{
		Columns: []string{"id", "title", "status", "priority", "issue_type", "assignee", "created_at", "closed_at", "is_blocked"},
		Rows: [][]string{
			{"i-absent", "NULL", "open", "1", "task", "alice", "2024-01-01", "NULL", "0"},
			{"i-stored", "NULL", "open", "1", "task", "alice", "2024-01-02", "NULL", "0"},
		},
		Nulls: [][]bool{
			// title and closed_at hold no value at all.
			{false, true, false, false, false, false, false, true, false},
			// every cell holds a value; the two "NULL"s are stored text.
			{false, false, false, false, false, false, false, false, false},
		},
		Total: 2,
	}
	deps := &browse.RowPage{
		Columns: []string{"id", "issue_id", "depends_on_issue_id", "type"},
		Rows:    [][]string{},
		Nulls:   [][]bool{},
	}
	return &fakeSession{rowsByTable: map[string]*browse.RowPage{
		"issues":       issues,
		"dependencies": deps,
	}}
}

// cell is where the distinction is actually made, so it is asked directly first:
// the same string, read two ways, decided by the mask beside it.
func TestCellReadsTheNullMaskAndNotTheString(t *testing.T) {
	cols := indexCols([]string{"id", "title"})

	absent := rowCells{values: []string{"i-absent", "NULL"}, nulls: []bool{false, true}}
	stored := rowCells{values: []string{"i-stored", "NULL"}, nulls: []bool{false, false}}

	assert.Equal(t, "", cell(cols, absent, "title"),
		"a cell that holds no value reads as empty, which every projection depends on")
	assert.Equal(t, "NULL", cell(cols, stored, "title"),
		"a cell that stores the text \"NULL\" has a value, and it is that text")

	assert.Equal(t, "", cell(cols, stored, "nosuchcolumn"), "an absent column is still empty")
}

// A page that carries no mask at all cannot answer the question, and the reading
// that predates the mask is all there is: "NULL" reads as absent. Every page
// browse returns carries one, so this is about a page built by hand.
func TestCellWithoutAMaskKeepsTheOlderReading(t *testing.T) {
	cols := indexCols([]string{"id", "title"})
	unmasked := rowCells{values: []string{"i-1", "NULL"}}

	assert.Equal(t, "", cell(cols, unmasked, "title"))
	assert.Equal(t, "i-1", cell(cols, unmasked, "id"))
}

// rowsOf pairs each row with its own mask, and a page with none hands out rows
// that answer "unknown" rather than rows that answer "not null".
func TestRowsOfPairsEveryRowWithItsMask(t *testing.T) {
	page := &browse.RowPage{
		Columns: []string{"id", "title"},
		Rows:    [][]string{{"a", "NULL"}, {"b", "NULL"}},
		Nulls:   [][]bool{{false, true}, {false, false}},
	}
	rows := rowsOf(page)
	require.Len(t, rows, 2)
	assert.Equal(t, []bool{false, true}, rows[0].nulls)
	assert.Equal(t, []bool{false, false}, rows[1].nulls)

	unmasked := rowsOf(&browse.RowPage{Columns: []string{"id"}, Rows: [][]string{{"a"}}})
	require.Len(t, unmasked, 1)
	assert.Nil(t, unmasked[0].nulls, "a page with no mask answers no mask, not an all-false one")

	assert.Empty(t, rowsOf(nil), "a table that is absent has no rows to pair")
}

// The projections are where it is felt: a title that is literally "NULL" must
// survive to the detail pane, and a closed_at that holds no value must keep
// reading as empty.
func TestProjectionsKeepAStoredNullAndDropARealOne(t *testing.T) {
	ctx := context.Background()

	stored, err := Build(ctx, nullMaskFixture(), "main", url.Values{"issue": {"i-stored"}})
	require.NoError(t, err)
	require.NotNil(t, stored.Issue)
	assert.Equal(t, "NULL", stored.Issue.Title,
		"the row stores those four characters: an empty title would lose them")
	assert.Equal(t, "NULL", stored.Issue.ClosedAt)

	absent, err := Build(ctx, nullMaskFixture(), "main", url.Values{"issue": {"i-absent"}})
	require.NoError(t, err)
	require.NotNil(t, absent.Issue)
	assert.Equal(t, "", absent.Issue.Title, "no value is no text, as every projection reads it")
	assert.Equal(t, "", absent.Issue.ClosedAt)
}

// The board goes through the same rows, and the same two readings have to reach
// the cards.
func TestBoardCardsKeepAStoredNullTitle(t *testing.T) {
	d, err := Build(context.Background(), nullMaskFixture(), "main", url.Values{})
	require.NoError(t, err)

	titles := map[string]string{}
	for _, lane := range d.Lanes {
		for _, c := range lane.Issues {
			titles[c.ID] = c.Title
		}
	}
	assert.Equal(t, "NULL", titles["i-stored"])
	assert.Equal(t, "", titles["i-absent"])
}