Skip to content

Harden correlate package correctness - #44

Open
optaktlux wants to merge 2 commits into
masterfrom
fix/correlate-correctness
Open

Harden correlate package correctness#44
optaktlux wants to merge 2 commits into
masterfrom
fix/correlate-correctness

Conversation

@optaktlux

@optaktlux optaktlux commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

What changed

  • Embed: bounds-check the API index (including negative) and report missing embeddings instead of panicking on malformed responses. Made the endpoint a package var for testability.
  • cosineToEpistemicCorrelation: aligned the struct doc, function doc, and implementation to the same calibration-backed piecewise thresholds (0.53/0.65/0.82). Breakpoints are now spelled on the exported CorrelationResult struct so external readers can see them.
  • MostIsolated (cleanup, not a bug): on master, n=1 produces +Inf internally but already returns Sources[0] because +Inf < MaxFloat64 is false. The early return avoids the meaningless Inf and guards against future simplification of the loop.
  • New internal tests: pin the transform breakpoints and monotonicity, verify embedding bounds/completeness/ordering via httptest (positive index, negative index, missing, reordered), and cover single- and multi-source isolation.

Verification

  • go test -race ./...
  • go vet ./...
  • staticcheck ./correlate/
  • correlate coverage 48.9% → 62.0%

Fixes #43

@optaktlux

Copy link
Copy Markdown
Collaborator Author

Hostile review (own PR)

Two of three findings are real. One was oversold.

Embed panic: real, merge it

result[item.Index] = item.Embedding with no bounds check is a crash on a malformed Voyage body. Out-of-range, negative, or omitted index is a production path if the API ever returns garbage, a truncated body, or a schema change. Returning an error and refusing missing slots is the right fix. httptest coverage for out-of-range, missing, and reordered indices is the part of this PR that earns the "correctness hunt" label.

Making voyageEndpoint a package var to inject the test server is the usual ugly-but-fine trick. It is not concurrent-safe. The tests are serial. Leave it, or put the URL on an unexported client later. Don't pretend this is architecture.

Doc drift: real, but it is docs

Three descriptions of the same transform, code is the calibration-backed one. Aligning comments is correct. It is not a defect in behavior. Don't lead the PR with it.

Also: cosineToEpistemicCorrelation is unexported. The new struct comment says "see cosineToEpistemicCorrelation". External readers of CorrelationResult cannot see that function. Point at the package comment or spell the breakpoints on the struct.

MostIsolated n=1: not a user-visible bug

On master, n=1 does avg := 0 / float64(0) → +Inf in IEEE 754. +Inf < math.MaxFloat64 is false, so minIdx stays 0 and the function already returns m.Sources[0]. The NaN I claimed is wrong; the return value does not change. The early return is cleaner and avoids a meaningless Inf. It is not a correctness fix. The test is still worth having so nobody "simplifies" the loop later.

Empty Sources already returned the zero value. Untouched. Fine.

Tests

Transform breakpoint table + monotonicity: good, pins the thing the docs just started telling the truth about.

TestMostIsolatedPicksLowestAverage is new coverage of the actual selection rule, not of the n=1 guard. Keep it.

No test for negative index (only index 5 vs len 2). Cheap to add.

Verdict

Merge the Embed bounds/completeness work. Demote MostIsolated from "guard single-source isolation" to "avoid Inf in the n=1 loop." Fix the struct comment that points at an unexported name. Then this is a real, small correctness PR, not a dressed-up coverage bump.

…egative-index test, demote MostIsolated n=1 to cleanup
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.

Harden correlate package: fix embedding index bounds, transform doc drift, and single-source NaN

1 participant