Skip to content

[codex] add Go wrapper for C ABI#103

Merged
project-navi-bot merged 2 commits into
mainfrom
codex/go-wrapper
May 29, 2026
Merged

[codex] add Go wrapper for C ABI#103
project-navi-bot merged 2 commits into
mainfrom
codex/go-wrapper

Conversation

@Fieldnote-Echo
Copy link
Copy Markdown
Owner

Summary

  • add repo-local ordvec-go module wrapping the committed C ABI with Load, Index.Close, Index.Info, and Index.Search
  • make Close idempotent, return ErrClosed after close, serialize Search/Info against Close, and copy C hits/stats into Go-owned values
  • add Go tests for load/info/search, subset search, stats, nil vs empty subset, typed status errors, idempotent close, ErrClosed, and race-test coverage

Validation

  • cargo fmt --all --check
  • cbindgen ordvec-ffi --config ordvec-ffi/cbindgen.toml --output ordvec-ffi/include/ordvec.h --verify
  • cargo test
  • cargo test -p ordvec-ffi
  • cargo clippy --all-targets -- -D warnings
  • cargo clippy -p ordvec-ffi --all-targets -- -D warnings
  • cargo build -p ordvec-ffi
  • GOCACHE=/tmp/go-build go test -count=1 ./... from ordvec-go
  • GOCACHE=/tmp/go-build go test -race -count=1 ./... from ordvec-go

Stack base: PR #102 / codex/c-api-docs.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces ordvec-go, a Go wrapper using cgo to interface with the local ordvec-ffi C ABI, along with comprehensive unit tests and documentation. The review feedback focuses on enhancing portability by adding macOS build flags, improving security by validating paths against null byte injection, and optimizing performance on the search hot path by caching immutable index metadata to avoid redundant, thread-locking CGO calls.

Comment thread ordvec-go/ordvec.go Outdated
Comment thread ordvec-go/ordvec.go
Comment thread ordvec-go/ordvec.go
Comment thread ordvec-go/ordvec.go
Comment thread ordvec-go/ordvec.go
Comment thread ordvec-go/ordvec.go Outdated
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Add Go wrapper for ordvec C ABI with comprehensive testing

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add Go wrapper module for C ABI with Load, Index.Close, Index.Info, Index.Search
• Implement thread-safe operations with mutex protection and OS thread locking
• Make Close idempotent, return ErrClosed after close, serialize Search/Info
• Copy C hits/stats into Go-owned values with comprehensive error handling
• Add extensive test coverage including subset search, stats, race conditions
Diagram
flowchart LR
  A["C ABI<br/>ordvec-ffi"] -->|cgo bindings| B["ordvec.go<br/>Core wrapper"]
  B -->|Load| C["Index struct<br/>Thread-safe"]
  C -->|Close| D["Resource cleanup<br/>Idempotent"]
  C -->|Info| E["Index metadata"]
  C -->|Search| F["Hit results<br/>+ Stats"]
  G["Test fixtures<br/>RankQuant/Bitmap"] -->|Load| H["ordvec_test.go<br/>14 test cases"]
  H -->|Validate| I["Correctness<br/>+ Race safety"]

Loading

Grey Divider

File Changes

1. ordvec-go/README.md 📝 Documentation +15/-0

Documentation for Go wrapper setup and usage

• Provides build and usage instructions for the Go wrapper
• Documents requirement to build Rust library before running tests
• Explains explicit Close() call and finalizer safety net behavior

ordvec-go/README.md


2. ordvec-go/go.mod ⚙️ Configuration changes +4/-0

Go module definition and dependencies

• Declares Go module with path github.com/Fieldnote-Echo/ordvec/ordvec-go
• Specifies Go version 1.22 requirement

ordvec-go/go.mod


3. ordvec-go/ordvec.go ✨ Enhancement +296/-0

Core Go wrapper implementation with thread safety

• Implements cgo bindings to ordvec-ffi C library with proper CFLAGS and LDFLAGS
• Defines Go types for Status, Kind, Info, Hit, Stats, SearchOptions, Index
• Implements Load() function with finalizer setup for resource cleanup
• Implements thread-safe Index.Close() with idempotent behavior and ErrClosed sentinel
• Implements Index.Info() with read lock and C struct conversion
• Implements Index.Search() with query validation, candidate filtering, memory management, and stats
 collection
• Uses runtime.LockOSThread for C FFI calls and runtime.KeepAlive for pointer safety

ordvec-go/ordvec.go


View more (1)
4. ordvec-go/ordvec_test.go 🧪 Tests +204/-0

Comprehensive test suite for Go wrapper

• Provides test fixtures for RankQuant and Bitmap index formats
• Tests Load, Info, Search operations on RankQuant and Bitmap indices
• Tests subset search with candidate filtering and row ID ordering
• Tests nil vs empty candidate distinction and error handling
• Tests typed StatusError handling for nonfinite queries and IO errors
• Tests idempotent Close behavior and ErrClosed sentinel
• Includes race condition testing via -race flag support

ordvec-go/ordvec_test.go


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 29, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Linux/debug-only cgo linking 🐞 Bug ☼ Reliability
Description
ordvec-go hardcodes Linux-only LDFLAGS to a debug static archive at ../target/debug/libordvec_ffi.a,
so building on macOS/Windows or building ordvec-ffi with the documented --release flow will fail at
link time. This makes the new Go wrapper brittle and inconsistent with the repo’s own C-API
build/link contract.
Code

ordvec-go/ordvec.go[R3-6]

Evidence
The Go wrapper currently links only on Linux and only against a debug build output, while the
repository’s C-API documentation specifies a release build and linking from target/release;
additionally the repo tests across macOS/Windows, so the current linux-only linking will break
builds on those platforms.

ordvec-go/ordvec.go[3-6]
docs/c-api.md[9-20]
.github/workflows/ci.yml[69-76]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ordvec-go/ordvec.go` links the Rust FFI library with a Linux-only, debug-only hardcoded path:
- Only `#cgo linux LDFLAGS: ...` is provided, so non-Linux builds will fail to link.
- It pins to `../target/debug/libordvec_ffi.a`, but the repo’s C-API docs instruct building `ordvec-ffi` with `--release` and linking from `target/release`.

## Issue Context
The repo documents that consumers should build the native library with `cargo build -p ordvec-ffi --release` and link from `target/release`. The Rust CI also runs across Linux/macOS/Windows, suggesting cross-OS support is expected.

## Fix Focus Areas
- ordvec-go/ordvec.go[3-6]

### Implementation directions
1. Decide the intended default: debug vs release (prefer aligning with `docs/c-api.md` and using release).
2. Make linking work across supported OSes, e.g.:
  - Provide per-OS `#cgo` directives (`linux`, `darwin`, `windows`) with appropriate library names/paths, or
  - Split into OS-specific Go files with build tags (e.g. `link_linux.go`, `link_darwin.go`, `link_windows.go`) containing only the cgo preamble.
3. Avoid hardcoding a single build output directory when possible. Options include:
  - Use `-L${SRCDIR}/../target/release -lordvec_ffi` (and platform equivalents), and/or
  - Allow overriding via environment (`CGO_LDFLAGS` or a documented build variable) so callers can point at a custom build directory.
4. Update `ordvec-go/README.md` to match the chosen build mode (debug vs release) so users don’t follow conflicting instructions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b7106a654

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ordvec-go/ordvec.go Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new repo-local Go module (ordvec-go) that wraps the committed ordvec-ffi C ABI via cgo, providing a Go-friendly Load, Index.Info, Index.Search, and idempotent Index.Close, plus a Go test suite covering basic functionality and edge cases.

Changes:

  • Introduce ordvec-go Go module with cgo bindings and typed status/error handling.
  • Implement Index lifecycle + concurrency control (serialize Close against Info/Search) and copy C results into Go-owned values.
  • Add Go tests that generate minimal fixture indexes and validate load/info/search behaviors and error semantics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
ordvec-go/README.md Adds build/test usage notes for the new Go wrapper.
ordvec-go/ordvec.go Implements the cgo wrapper types and API surface (Load, Info, Search, Close).
ordvec-go/ordvec_test.go Adds fixture-based tests covering search modes, stats, and error handling.
ordvec-go/go.mod Defines the new ordvec-go Go module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ordvec-go/ordvec.go Outdated
Comment thread ordvec-go/ordvec.go
Comment thread ordvec-go/ordvec.go Outdated
Comment thread ordvec-go/ordvec.go
Comment thread ordvec-go/ordvec.go Outdated
Comment thread ordvec-go/ordvec.go
Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
Copy link
Copy Markdown
Owner Author

Addressed the remaining bot findings for this PR:

  • cgo link flags now target target/release and include Linux, Darwin, and Windows directives; the README matches the release build requirement.
  • Load rejects paths containing NUL bytes before C.CString, with a Go test covering the behavior.
  • Info is cached during Load; Info() and Search() use the cached metadata while still returning ErrClosed after close.
  • Search no longer passes a Go ordvec_search_params_t containing Go pointers into C. A C helper builds params on the C stack, while Go passes pinned query/candidate pointers directly for the synchronous call.
  • Added a Go wrapper CI lane that builds ordvec-ffi --release and runs go test, go test -race, and GOEXPERIMENT=cgocheck2 go test.
  • README documents nil-vs-empty candidate behavior.

Validation run locally: cargo build -p ordvec-ffi --release, go test -count=1 ./..., go test -race -count=1 ./..., and GOEXPERIMENT=cgocheck2 go test -count=1 ./....

Base automatically changed from codex/c-api-docs to main May 29, 2026 03:39
@project-navi-bot project-navi-bot merged commit 1cb7002 into main May 29, 2026
27 checks passed
@project-navi-bot project-navi-bot deleted the codex/go-wrapper branch May 29, 2026 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants