From 5a2d87c9f46ac9bdcdb7e8b5173e48e08dd11aab Mon Sep 17 00:00:00 2001 From: Shree Bohara Date: Thu, 13 Aug 2026 08:57:37 -0700 Subject: [PATCH] Add Go CI coverage and a cross-language proto drift check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #11 merged with no CI protection at all: ci.yml had Python 3.11 and Node 20 only, so nothing compiled the Go service, ran go vet, or checked that the two languages' generated stubs still agreed. Its automated review also never ran -- the Codex bot hit its usage limit -- so that code landed unguarded twice over. indexer-go: gofmt (excluding gen/, which the drift job owns), go vet, go test -race, and a cgo build. CGO_ENABLED=1 throughout and an explicit `cc --version` step, because every tree-sitter grammar is a cgo package with its own generated parser.c -- if the runner ever loses its C toolchain the failure should name that rather than surfacing as a link error. proto-codegen-drift: regenerates both the Go and Python stubs from proto/indexer.proto and fails if either differs from what is committed. Without it, editing the proto and regenerating only one side produces a server and client that disagree, and nothing catches it until runtime. Generators are PINNED (protoc 29.3, protoc-gen-go v1.36.12, protoc-gen-go-grpc v1.6.2, grpcio-tools 1.83.0) rather than @latest. Generated files embed the generator version ("protoc-gen-go v1.36.12", "Protobuf Python Version: 7.35.1"), so an unpinned plugin would eventually change the output and fail the check for a reason unrelated to the contract. I verified the round-trip is byte-identical on both sides with these versions before relying on a strict whole-file diff, and corrected a comment that had claimed the comparison was looser than it actually is. Go tests, which did not exist before, cover the properties that were only verified by hand: - .tsx parses under the tsx grammar with no parse error (the plain TypeScript grammar cannot parse JSX -- the bug that shipped in the Python parser) - chunks following a line containing non-ASCII text still slice as 'class Café:' and 'def método(self):' rather than misaligned fragments, because tree-sitter offsets are byte offsets - HadParseError is set on a syntax error rather than swallowed, since tree-sitter returns a partial tree instead of failing - MaxFiles actually stops the walk and sets Truncated -- the Python original's `break` left only the inner filename loop, so os.walk continued into the next directory - extension mapping, unlinked extensions, and the module-chunk fallback Also ran gofmt over the two source files it flagged, and validated the workflow YAML locally, which caught an unquoted colon in a step name that would have failed the run. Verified: gofmt clean, go vet clean, go test ./... passes both packages, cgo build succeeds, proto round-trip byte-identical, workflow parses with 4 jobs. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 107 +++++++++++ services/indexer/README.md | 31 +++- services/indexer/internal/parse/parse.go | 4 +- services/indexer/internal/parse/parse_test.go | 174 ++++++++++++++++++ services/indexer/internal/walk/walk.go | 2 +- services/indexer/internal/walk/walk_test.go | 134 ++++++++++++++ 6 files changed, 447 insertions(+), 5 deletions(-) create mode 100644 services/indexer/internal/parse/parse_test.go create mode 100644 services/indexer/internal/walk/walk_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d8112c1..75a16b8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -86,3 +86,110 @@ jobs: - name: Run Tests run: pnpm --filter web test + + indexer-go: + runs-on: ubuntu-latest + defaults: + run: + working-directory: ./services/indexer + + steps: + - uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.23' + cache-dependency-path: services/indexer/go.sum + + # Every tree-sitter grammar is a cgo package with its own generated parser.c, so this + # job cannot use CGO_ENABLED=0 and needs a C toolchain. ubuntu-latest ships gcc, but + # pinning the expectation here makes the failure obvious if that ever changes. + - name: Verify C toolchain is present (grammars are cgo) + run: cc --version + + - name: Formatting + run: | + # gen/ is protoc output and is checked by the codegen-drift step instead. + unformatted=$(gofmt -l . | grep -v '^gen/' || true) + if [ -n "$unformatted" ]; then + echo "::error::gofmt needed for: $unformatted" + gofmt -d $unformatted + exit 1 + fi + + - name: Vet + run: CGO_ENABLED=1 go vet ./... + + - name: Test + run: CGO_ENABLED=1 go test -race ./... + + - name: Build + run: CGO_ENABLED=1 go build -o /tmp/indexer . + + # The contract is shared between two languages that generate stubs independently. Without + # this, editing indexer.proto and regenerating only one side produces a mismatch that + # nothing catches until runtime -- the Go server and Python client would simply disagree. + proto-codegen-drift: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.23' + cache-dependency-path: services/indexer/go.sum + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.11" + + - name: Install protoc and plugins + run: | + PROTOC_VERSION=29.3 + curl -fsSL -o /tmp/protoc.zip \ + "https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-x86_64.zip" + unzip -q /tmp/protoc.zip -d /tmp/protoc + echo "/tmp/protoc/bin" >> "$GITHUB_PATH" + # Pinned, not @latest. Generated files embed the generator version + # ("protoc-gen-go v1.36.12", "Protobuf Python Version: 7.35.1"), so an unpinned + # plugin would eventually change the output and fail this check for a reason that + # has nothing to do with the proto. These are the exact versions that produced the + # committed stubs; bump them and the stubs together. + go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.12 + go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2 + echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH" + python -m pip install --quiet "grpcio-tools==1.83.0" + + - name: Regenerate Go stubs + working-directory: ./services/indexer + run: | + protoc --proto_path=proto --go_out=gen --go_opt=paths=source_relative \ + --go-grpc_out=gen --go-grpc_opt=paths=source_relative proto/indexer.proto + + - name: Regenerate Python stubs + working-directory: ./apps/api + run: | + python -m grpc_tools.protoc \ + --proto_path=../../services/indexer/proto \ + --python_out=src/core/indexer --grpc_python_out=src/core/indexer \ + ../../services/indexer/proto/indexer.proto + # The generated grpc stub uses a flat import; the committed copy is package-relative. + sed -i 's/^import indexer_pb2 as indexer__pb2$/from src.core.indexer import indexer_pb2 as indexer__pb2/' \ + src/core/indexer/indexer_pb2_grpc.py + + - name: Fail if committed stubs differ from the proto + run: | + # A strict whole-file diff is safe because protoc, both Go plugins and grpcio-tools + # are all pinned above; the round-trip was verified byte-identical. + if ! git diff --quiet -- services/indexer/gen apps/api/src/core/indexer; then + echo "::error::Generated stubs are out of date with proto/indexer.proto." + echo "Regenerate both sides and commit; see services/indexer/README.md." + git diff --stat -- services/indexer/gen apps/api/src/core/indexer + git diff -- services/indexer/gen apps/api/src/core/indexer | head -60 + exit 1 + fi + echo "Generated stubs match the proto on both sides." diff --git a/services/indexer/README.md b/services/indexer/README.md index 7b21a25..0639128 100644 --- a/services/indexer/README.md +++ b/services/indexer/README.md @@ -61,6 +61,32 @@ CGO_ENABLED=1 go build -o bin/indexer . `CGO_ENABLED=1` is mandatory: every grammar is cgo. That means a C toolchain in any builder image, no `scratch` base, and a slower uncached build than the all-wheels Python image. +## Regenerating the stubs + +Both sides are generated from `proto/indexer.proto` and both are committed. CI regenerates +them and fails if they differ, so an edit to the proto that updates only one language +cannot merge. + +```bash +# Go +cd services/indexer +protoc --proto_path=proto --go_out=gen --go_opt=paths=source_relative \ + --go-grpc_out=gen --go-grpc_opt=paths=source_relative proto/indexer.proto + +# Python +cd apps/api +python -m grpc_tools.protoc --proto_path=../../services/indexer/proto \ + --python_out=src/core/indexer --grpc_python_out=src/core/indexer \ + ../../services/indexer/proto/indexer.proto +sed -i 's/^import indexer_pb2 as indexer__pb2$/from src.core.indexer import indexer_pb2 as indexer__pb2/' \ + src/core/indexer/indexer_pb2_grpc.py +``` + +Generator versions are **pinned in CI** (protoc 29.3, protoc-gen-go v1.36.12, +protoc-gen-go-grpc v1.6.2, grpcio-tools 1.83.0) because generated files embed the +generator version — an unpinned plugin would change the output and fail the drift check +for a reason unrelated to the contract. Bump the pins and the committed stubs together. + ## Grammar scope, stated rather than implied This build links **5** grammars (python, javascript, typescript, tsx, go) against the @@ -109,5 +135,6 @@ The service is **not wired into the indexing pipeline** — `IndexingService` st Python parser, and `IndexerClient` is opt-in. Doing that swap needs a decision this PR does not make: the service must be co-located with the API on the shared volume, because clones live under `./data/repos//` and a Fly/EBS-style volume attaches to exactly one -machine. There is also no CI job for Go yet (`ci.yml` has Python 3.11 and Node 20 only, no -cgo toolchain, no grammar-compile caching, no cross-language contract test). +machine. CI now covers this service: `indexer-go` runs gofmt, `go vet`, `go test -race` and a cgo +build, and `proto-codegen-drift` regenerates both languages' stubs and fails if they differ +from what is committed. diff --git a/services/indexer/internal/parse/parse.go b/services/indexer/internal/parse/parse.go index 60ae5a4..887cf99 100644 --- a/services/indexer/internal/parse/parse.go +++ b/services/indexer/internal/parse/parse.go @@ -97,9 +97,9 @@ var configs = map[string]langConfig{ var extToLang = map[string]string{ ".py": "python", ".js": "javascript", ".jsx": "javascript", - ".ts": "typescript", + ".ts": "typescript", ".tsx": "tsx", // never "typescript" - ".go": "go", + ".go": "go", } // LanguageFor reports the grammar for an extension, and whether one exists. diff --git a/services/indexer/internal/parse/parse_test.go b/services/indexer/internal/parse/parse_test.go new file mode 100644 index 0000000..3918ff6 --- /dev/null +++ b/services/indexer/internal/parse/parse_test.go @@ -0,0 +1,174 @@ +package parse + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func write(t *testing.T, dir, name, content string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// The plain TypeScript grammar cannot parse JSX. Mapping .tsx to it produces a tree full +// of ERROR nodes for every .tsx file -- the exact bug that shipped in the Python parser. +func TestTSXUsesTheTSXGrammar(t *testing.T) { + dir := t.TempDir() + src := "export function Widget({ label }: { label: string }) {\n" + + " return
{label}
;\n}\n" + abs := write(t, dir, "Widget.tsx", src) + + chunks, err := File(abs, "Widget.tsx") + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(chunks) == 0 { + t.Fatal("expected at least one chunk") + } + for _, c := range chunks { + if c.Language != "tsx" { + t.Errorf("language = %q, want tsx (the plain TS grammar cannot parse JSX)", c.Language) + } + if c.HadParseError { + t.Errorf("chunk %q has a parse error; JSX should parse cleanly under tsx", c.Name) + } + } +} + +// tree-sitter offsets are BYTE offsets. Slicing a decoded string with them corrupts every +// chunk after the first multi-byte character. +func TestChunksAfterNonASCIIAreNotMisaligned(t *testing.T) { + dir := t.TempDir() + src := "# héllo — wörld 日本語 🎉\n" + + "def after_unicode(x):\n return x * 2\n" + + "class Café:\n def método(self):\n return \"ok\"\n" + abs := write(t, dir, "unicode.py", src) + + chunks, err := File(abs, "unicode.py") + if err != nil { + t.Fatalf("parse: %v", err) + } + + byName := map[string]Chunk{} + for _, c := range chunks { + if c.Name != "" { + byName[c.Name] = c + } + } + + for _, want := range []struct{ name, prefix string }{ + {"after_unicode", "def after_unicode"}, + {"Café", "class Café:"}, + {"método", "def método(self):"}, + } { + got, ok := byName[want.name] + if !ok { + t.Errorf("missing chunk %q (got %v)", want.name, keys(byName)) + continue + } + if !strings.HasPrefix(got.Content, want.prefix) { + t.Errorf("chunk %q content = %q, want prefix %q -- byte/char offset mismatch", + want.name, truncateForMsg(got.Content), want.prefix) + } + } +} + +// tree-sitter returns a partial tree rather than failing on a syntax error, so the flag is +// the only way a caller can decide to fall back to raw indexing. +func TestSyntaxErrorIsReportedNotSwallowed(t *testing.T) { + dir := t.TempDir() + abs := write(t, dir, "broken.py", "def ok():\n return 1\n\nclass ((( :\n") + + chunks, err := File(abs, "broken.py") + if err != nil { + t.Fatalf("parse should not fail on a syntax error: %v", err) + } + if len(chunks) == 0 { + t.Fatal("expected chunks even from a file with errors") + } + found := false + for _, c := range chunks { + if c.HadParseError { + found = true + } + } + if !found { + t.Error("HadParseError was never set on a file with a syntax error") + } +} + +func TestExtensionMapping(t *testing.T) { + cases := map[string]string{ + ".py": "python", ".ts": "typescript", ".tsx": "tsx", + ".js": "javascript", ".jsx": "javascript", ".go": "go", + } + for ext, want := range cases { + got, ok := LanguageFor(ext) + if !ok || got != want { + t.Errorf("LanguageFor(%q) = %q,%v; want %q", ext, got, ok, want) + } + } + if _, ok := LanguageFor(".rs"); ok { + t.Error("LanguageFor(.rs) should report unlinked: this build has 5 grammars, not 9") + } +} + +func TestUnknownExtensionYieldsNoChunks(t *testing.T) { + dir := t.TempDir() + abs := write(t, dir, "notes.txt", "plain text") + chunks, err := File(abs, "notes.txt") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(chunks) != 0 { + t.Errorf("expected no chunks for an unlinked extension, got %d", len(chunks)) + } +} + +// A file with no extractable declarations still carries meaning; dropping it would lose +// content the Python side keeps. +func TestFileWithNoDeclarationsFallsBackToModuleChunk(t *testing.T) { + dir := t.TempDir() + abs := write(t, dir, "consts.py", "A = 1\nB = 2\n") + chunks, err := File(abs, "consts.py") + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(chunks) != 1 || chunks[0].ChunkType != "module" { + t.Fatalf("want one module chunk, got %+v", chunks) + } +} + +func TestLinkedReportsEveryMappedExtension(t *testing.T) { + linked := Linked() + if len(linked) == 0 { + t.Fatal("Linked() must report grammars so callers never assume parity with Python") + } + if exts, ok := linked["tsx"]; !ok || len(exts) == 0 { + t.Error("tsx must be reported separately from typescript") + } +} + +func keys(m map[string]Chunk) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} + +func truncateForMsg(s string) string { + if len(s) > 60 { + return s[:60] + "..." + } + return s +} diff --git a/services/indexer/internal/walk/walk.go b/services/indexer/internal/walk/walk.go index 6a179b0..372a664 100644 --- a/services/indexer/internal/walk/walk.go +++ b/services/indexer/internal/walk/walk.go @@ -30,7 +30,7 @@ var indexedExts = map[string]struct{}{ ".cs": {}, ".csx": {}, ".rb": {}, ".rake": {}, ".gemspec": {}, ".php": {}, ".swift": {}, ".kt": {}, ".erb": {}, - ".md": {}, ".json": {}, + ".md": {}, ".json": {}, } // Mirrors INDEXED_FILENAMES: extensionless files worth indexing. diff --git a/services/indexer/internal/walk/walk_test.go b/services/indexer/internal/walk/walk_test.go new file mode 100644 index 0000000..d0ec1d5 --- /dev/null +++ b/services/indexer/internal/walk/walk_test.go @@ -0,0 +1,134 @@ +package walk + +import ( + "fmt" + "os" + "path/filepath" + "testing" +) + +func seed(t *testing.T, dir string, files map[string]string) { + t.Helper() + for name, body := range files { + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } +} + +func TestSkipsExcludedDirectories(t *testing.T) { + dir := t.TempDir() + seed(t, dir, map[string]string{ + "src/app.ts": "x", + "node_modules/pkg/index.js": "x", + ".git/config": "x", + "dist/bundle.js": "x", + ".venv/lib/mod.py": "x", + }) + + got, err := Find(dir, Options{}) + if err != nil { + t.Fatal(err) + } + if len(got.Paths) != 1 || got.Paths[0] != "src/app.ts" { + t.Errorf("paths = %v, want only src/app.ts", got.Paths) + } +} + +// The Python original's cap did not work: its `break` left only the inner filename loop, +// so os.walk continued into the next directory and kept appending. This is the regression +// test for that class of bug. +func TestMaxFilesActuallyStopsTheWalk(t *testing.T) { + dir := t.TempDir() + files := map[string]string{} + for d := 0; d < 5; d++ { + for f := 0; f < 10; f++ { + files[fmt.Sprintf("d%d/f%d.py", d, f)] = "x" + } + } + seed(t, dir, files) + + got, err := Find(dir, Options{MaxFiles: 12}) + if err != nil { + t.Fatal(err) + } + if len(got.Paths) > 12 { + t.Errorf("found %d files with MaxFiles=12: the cap leaked", len(got.Paths)) + } + if !got.Truncated { + t.Error("Truncated must be set, or a partial index looks like full coverage") + } +} + +func TestTruncatedIsFalseWhenEverythingFits(t *testing.T) { + dir := t.TempDir() + seed(t, dir, map[string]string{"a.py": "x", "b.py": "x"}) + got, err := Find(dir, Options{MaxFiles: 100}) + if err != nil { + t.Fatal(err) + } + if got.Truncated { + t.Error("Truncated set despite the walk completing") + } +} + +func TestSkipsOversizeFiles(t *testing.T) { + dir := t.TempDir() + big := make([]byte, 3*1024) + for i := range big { + big[i] = 'x' + } + seed(t, dir, map[string]string{"small.py": "x"}) + if err := os.WriteFile(filepath.Join(dir, "big.py"), big, 0o644); err != nil { + t.Fatal(err) + } + + got, err := Find(dir, Options{MaxFileSizeKB: 2}) + if err != nil { + t.Fatal(err) + } + if len(got.Paths) != 1 || got.Paths[0] != "small.py" { + t.Errorf("paths = %v, want only small.py", got.Paths) + } + if got.Skipped != 1 { + t.Errorf("Skipped = %d, want 1", got.Skipped) + } +} + +func TestIndexesExtensionlessKnownFilenames(t *testing.T) { + dir := t.TempDir() + seed(t, dir, map[string]string{ + "Gemfile": "x", "Rakefile": "x", "config.ru": "x", "LICENSE": "x", + }) + got, err := Find(dir, Options{}) + if err != nil { + t.Fatal(err) + } + if len(got.Paths) != 3 { + t.Errorf("paths = %v, want the three known Ruby filenames and not LICENSE", got.Paths) + } +} + +func TestPathsAreRelativeAndSlashSeparated(t *testing.T) { + dir := t.TempDir() + seed(t, dir, map[string]string{"a/b/c.py": "x"}) + got, err := Find(dir, Options{}) + if err != nil { + t.Fatal(err) + } + // Must match CodeFile.path on the Python side, which is repo-relative with forward + // slashes regardless of platform. + if len(got.Paths) != 1 || got.Paths[0] != "a/b/c.py" { + t.Errorf("paths = %v, want [a/b/c.py]", got.Paths) + } +} + +func TestMissingRootIsNotAPanic(t *testing.T) { + if _, err := Find(filepath.Join(t.TempDir(), "nope"), Options{}); err != nil { + t.Errorf("a missing root should return empty, not error: %v", err) + } +}