diff --git a/cmd/gc/cmd_order.go b/cmd/gc/cmd_order.go index 2716641d59..5f3155a7de 100644 --- a/cmd/gc/cmd_order.go +++ b/cmd/gc/cmd_order.go @@ -1414,6 +1414,24 @@ func routeOrderHistory(cityPath string, cfg *config.City, name, rig string, aa [ return doOrderHistoryBounded(name, rig, aa, cachedOrderHistoryStoresResolver(cityPath, cfg, stderr), bounds, jsonOutput, stdout, stderr) } + // A rig-scoped order is registered once per importing rig, so a bare name + // with no --rig names several registrations at once. The API request + // carries a single scoped_name, so routing an ambiguous name there answers + // for whichever registration findOrder returned first and silently drops + // every other rig's runs -- while the response still renders a RIG column, + // which makes a one-rig answer indistinguishable from a city-wide one. That + // under-report already cost a P1 on the refinery merge cadence (gc-6a6vz). + // + // The local iterator has no such limit: it walks every matching + // registration, merges the runs newest-first and only then applies the + // bound, so --limit N means "the N most recent runs in the city". Stay on + // it whenever the name resolves to more than one registration. A + // rig-qualified read still names exactly one, so it keeps the API route. + if matches := countOrderRegistrations(aa, name, rig); matches > 1 { + logRoute(stderr, "order history", "fallback", "multi-rig") + return doOrderHistoryBounded(name, rig, aa, cachedOrderHistoryStoresResolver(cityPath, cfg, stderr), bounds, jsonOutput, stdout, stderr) + } + var cr api.CachedRead[[]api.OrderHistoryView] return routeRead(c, "order history", nilReason, stderr, func() error { @@ -1431,6 +1449,24 @@ func routeOrderHistory(cityPath string, cfg *config.City, name, rig string, aa [ ) } +// countOrderRegistrations reports how many loaded order registrations match +// (name, rig). A rig-scoped order is registered once per importing rig, so an +// unqualified name matches one registration per rig that imported it; an empty +// rig therefore means "any rig", matching findOrder's own filter. +func countOrderRegistrations(aa []orders.Order, name, rig string) int { + matches := 0 + for _, a := range aa { + if a.Name != name { + continue + } + if rig != "" && a.Rig != rig { + continue + } + matches++ + } + return matches +} + // orderScopedName returns the rig-qualified key for the server's // /orders/history lookup. When a matching order is loaded locally, its // ScopedName() is authoritative (handles the city-level vs rig-level diff --git a/cmd/gc/cmd_order_history_store_completeness_test.go b/cmd/gc/cmd_order_history_store_completeness_test.go new file mode 100644 index 0000000000..59cc33c559 --- /dev/null +++ b/cmd/gc/cmd_order_history_store_completeness_test.go @@ -0,0 +1,247 @@ +package main + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/gastownhall/gascity/internal/beads" + "github.com/gastownhall/gascity/internal/orders" +) + +// A rig-scoped order is registered once per importing rig, so one bare `name` +// names several order registrations. These tests pin that a bounded read +// (`--limit N`) answers for every one of them, rather than for whichever +// registration happened to be found first (gc-6a6vz). + +// orderRunsStoreForScoped builds a store holding count order-run beads for a +// single scoped order name, one hour apart going back from newest. It mirrors +// orderHistoryRunsStore but takes the scoped name so each rig's registration +// gets its own rows. +func orderRunsStoreForScoped(t *testing.T, scoped string, newest time.Time, count int) beads.Store { + t.Helper() + rows := make([]string, 0, count) + for i := 0; i < count; i++ { + rows = append(rows, fmt.Sprintf( + `{"id":%q,"title":"run %d","status":"closed","issue_type":"task","created_at":%q,"labels":["order-run:%s"]}`, + fmt.Sprintf("%s-%d", strings.ReplaceAll(scoped, ":", "-"), i), + i, + newest.Add(-time.Duration(i)*time.Hour).Format(time.RFC3339Nano), + scoped, + )) + } + payload := []byte("[" + strings.Join(rows, ",") + "]") + return beads.NewBdStore(t.TempDir(), func(_, _ string, args ...string) ([]byte, error) { + if len(args) > 0 && args[0] == "list" { + return payload, nil + } + return []byte("[]"), nil + }) +} + +// twoRigOrderRegistrations is the shape at the heart of gc-6a6vz: one order +// name, one registration per importing rig. +func twoRigOrderRegistrations() []orders.Order { + return []orders.Order{ + {Name: "digest", Rig: "rig-a", Formula: "mol-digest"}, + {Name: "digest", Rig: "rig-b", Formula: "mol-digest"}, + } +} + +// TestOrderHistoryBoundedReadIsStoreCompleteAcrossRigs is the bead's stated +// acceptance test: with a limit larger than the total number of retained runs, +// a bare `gc order history ` must return rows from EVERY rig, not just +// one. An answer drawn from a single store still renders a RIG column, so a +// partial read is indistinguishable from a complete one at the terminal. +func TestOrderHistoryBoundedReadIsStoreCompleteAcrossRigs(t *testing.T) { + newest := time.Date(2026, 5, 17, 12, 0, 0, 0, time.UTC) + storeA := orderRunsStoreForScoped(t, "digest:rig:rig-a", newest, 3) + storeB := orderRunsStoreForScoped(t, "digest:rig:rig-b", newest.Add(-30*time.Minute), 3) + + resolver := func(a orders.Order) ([]beads.OrdersStore, error) { + switch a.Rig { + case "rig-a": + return []beads.OrdersStore{{Store: storeA}}, nil + case "rig-b": + return []beads.OrdersStore{{Store: storeB}}, nil + } + return nil, fmt.Errorf("unexpected rig %q", a.Rig) + } + + var stdout, stderr bytes.Buffer + // 50 is far more than the 6 rows that exist, so nothing here is a budget + // exhaustion: any missing rig is a store the read never opened. + code := doOrderHistoryBounded("digest", "", twoRigOrderRegistrations(), resolver, + orderHistoryBounds{Limit: 50}, true, &stdout, &stderr) + if code != 0 { + t.Fatalf("doOrderHistoryBounded = %d, want 0; stderr: %s", code, stderr.String()) + } + + payload := orderHistoryEntries(t, &stdout) + if len(payload.Entries) != 6 { + t.Fatalf("entries = %d, want 6 (3 per rig across 2 rigs); a short count means a store was not read", len(payload.Entries)) + } + seen := map[string]int{} + for _, e := range payload.Entries { + seen[e.Rig]++ + } + if seen["rig-a"] != 3 || seen["rig-b"] != 3 { + t.Fatalf("per-rig counts = %v, want 3 each; a bounded read must be store-complete", seen) + } +} + +// TestOrderHistoryBoundedReadKeepsNewestAcrossRigs pins the ordering half of +// the contract: the limit is applied AFTER merging every store newest-first, so +// `--limit N` means "the N most recent runs in the city", not "the N most recent +// runs in whichever store was read first". +func TestOrderHistoryBoundedReadKeepsNewestAcrossRigs(t *testing.T) { + newest := time.Date(2026, 5, 17, 12, 0, 0, 0, time.UTC) + // rig-b's newest run is 30 minutes older than rig-a's, so a correct merge + // interleaves them: a-0, b-0, a-1, b-1, ... + storeA := orderRunsStoreForScoped(t, "digest:rig:rig-a", newest, 3) + storeB := orderRunsStoreForScoped(t, "digest:rig:rig-b", newest.Add(-30*time.Minute), 3) + + resolver := func(a orders.Order) ([]beads.OrdersStore, error) { + switch a.Rig { + case "rig-a": + return []beads.OrdersStore{{Store: storeA}}, nil + case "rig-b": + return []beads.OrdersStore{{Store: storeB}}, nil + } + return nil, fmt.Errorf("unexpected rig %q", a.Rig) + } + + var stdout, stderr bytes.Buffer + code := doOrderHistoryBounded("digest", "", twoRigOrderRegistrations(), resolver, + orderHistoryBounds{Limit: 2}, true, &stdout, &stderr) + if code != 0 { + t.Fatalf("doOrderHistoryBounded = %d, want 0; stderr: %s", code, stderr.String()) + } + + payload := orderHistoryEntries(t, &stdout) + if len(payload.Entries) != 2 { + t.Fatalf("entries = %d, want 2", len(payload.Entries)) + } + // The two newest overall are rig-a's newest and rig-b's newest. Keeping + // only rig-a's two newest would mean the limit was applied per store. + gotRigs := []string{payload.Entries[0].Rig, payload.Entries[1].Rig} + if gotRigs[0] != "rig-a" || gotRigs[1] != "rig-b" { + t.Fatalf("rigs = %v, want [rig-a rig-b]; the bound must be applied after merging every store", gotRigs) + } +} + +// writeOrderHistoryMultiRigCity builds a city whose site config declares two +// rigs, so a rig-scoped order registration resolves to a real per-rig store. +func writeOrderHistoryMultiRigCity(t *testing.T) string { + t.Helper() + cityPath := t.TempDir() + for _, dir := range []string{".gc", "orders", "formulas", "rig-a", "rig-b"} { + if err := os.MkdirAll(filepath.Join(cityPath, dir), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(cityPath, "city.toml"), []byte(`[workspace] +name = "test-city" + +[[agent]] +name = "mayor" +`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cityPath, ".gc", "site.toml"), []byte(`workspace_name = "test-city" + +[[rig]] +name = "rig-a" +path = "./rig-a" + +[[rig]] +name = "rig-b" +path = "./rig-b" +`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cityPath, "orders", "digest.toml"), []byte(`[order] +trigger = "manual" +formula = "mol-digest" +scope = "rig" +`), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("GC_DOLT", "skip") + t.Setenv("GC_BEADS", "file") + t.Setenv("GC_BEADS_SCOPE_ROOT", "") + return cityPath +} + +// orderHistoryNoClientReason is the nilReason handed to routeOrderHistory when +// these tests pass a nil API client. routeRead logs it verbatim on the nil-client +// path, so it doubles as a sentinel: seeing it proves the call reached the API +// route and fell back only for want of a controller, and NOT seeing it proves an +// earlier branch claimed the read. +// +// Passing nil rather than an httptest server is deliberate. The routing decision +// is fully observable from the emitted route= line, which logRoute writes before +// any request is built, so a real listener would prove nothing extra -- and the +// untagged http_test_server census is a "cannot grow" ratchet (TESTING.md, Small +// and Source debt ratchets, ga-80po0c.2.2). Spending two call sites and a file +// against that ceiling to observe what stderr already reports would be a poor +// trade. +const orderHistoryNoClientReason = "no-controller-sentinel" + +// TestRouteOrderHistoryBoundedStaysLocalWhenNameSpansRigs is the routing +// regression. The supervisor API request carries a single scoped_name, so it can +// only ever answer for ONE registration; routing a bare, ambiguous name there +// silently drops every other rig's runs while still printing a RIG column. The +// bounded read must therefore stay on the local iterator, which fans out across +// each matching registration's store. +func TestRouteOrderHistoryBoundedStaysLocalWhenNameSpansRigs(t *testing.T) { + t.Setenv("GC_DEBUG", "1") + + cityPath := writeOrderHistoryMultiRigCity(t) + cfg, err := loadCityConfig(cityPath, &bytes.Buffer{}) + if err != nil { + t.Fatalf("loadCityConfig: %v", err) + } + + var stdout, stderr bytes.Buffer + // A positive limit is the whole point: this is the path that used to route + // to the API. --rig is deliberately empty, so "digest" is ambiguous. + routeOrderHistory(cityPath, cfg, "digest", "", twoRigOrderRegistrations(), nil, orderHistoryNoClientReason, + orderHistoryBounds{Limit: defaultOrderHistoryLimit}, false, &stdout, &stderr) + + if !strings.Contains(stderr.String(), "route=fallback reason=multi-rig") { + t.Errorf("stderr missing multi-rig fallback: a bounded read of a name spanning %d rigs reached the API route, which answers for one scoped_name only:\n%s", + len(twoRigOrderRegistrations()), stderr.String()) + } + if strings.Contains(stderr.String(), "reason="+orderHistoryNoClientReason) { + t.Errorf("read reached the API route before the multi-rig guard:\n%s", stderr.String()) + } +} + +// TestRouteOrderHistoryBoundedUsesAPIWhenRigQualified is the guard on the other +// side: --rig names exactly one registration, so the API route stays available +// and this fix does not push every bounded read back onto the slower local scan. +func TestRouteOrderHistoryBoundedUsesAPIWhenRigQualified(t *testing.T) { + t.Setenv("GC_DEBUG", "1") + + cityPath := writeOrderHistoryMultiRigCity(t) + cfg, err := loadCityConfig(cityPath, &bytes.Buffer{}) + if err != nil { + t.Fatalf("loadCityConfig: %v", err) + } + + var stdout, stderr bytes.Buffer + routeOrderHistory(cityPath, cfg, "digest", "rig-a", twoRigOrderRegistrations(), nil, orderHistoryNoClientReason, + orderHistoryBounds{Limit: defaultOrderHistoryLimit}, false, &stdout, &stderr) + + if strings.Contains(stderr.String(), "reason=multi-rig") { + t.Errorf("rig-qualified read names exactly one registration and must keep the API route:\n%s", stderr.String()) + } + if !strings.Contains(stderr.String(), "reason="+orderHistoryNoClientReason) { + t.Errorf("rig-qualified read did not reach the API route:\n%s", stderr.String()) + } +}