chore(dev→main): routine PR cadence for CodeRabbit review

[Snider]

Routine dev→main PR opened by Hephaestus PR-cadence task.

This PR exists to:

1. Trigger CodeRabbit auto-review of accumulated dev work
2. Surface any blocking review feedback before merge
3. Keep main current with dev once approved

ahead_by: 77 commit(s) (per gh api compare)

If CodeRabbit clears with no blocking comments, this PR is eligible for gh pr merge --merge (real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.

Co-authored-by: Hephaestus hephaestus@cladius

Summary by CodeRabbit

Release Notes

• New Features

  • Added sentence and paragraph-based text chunking strategies alongside existing markdown chunking.
  • Implemented TF-IDF keyword search as a fallback method for query results.
  • Expanded file ingestion to support PDF documents alongside Markdown and plain text formats.
  • Added --keywords flag to enable keyword-based filtering during queries.
  • Introduced new convenience APIs for ingestion, queries, and collection management.

• Documentation

  • Updated architecture documentation and CLI examples to reflect enhanced capabilities.

[coderabbitai]

Warning

Rate limit exceeded

@Snider has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 1 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0fe8e10e-7d3c-449e-bd3b-425dcbdb672a

📥 Commits

Reviewing files that changed from the base of the PR and between 12b06a6 and bb63643.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (24)

• CLAUDE.md
• README.md
• chunk.go
• cmd/rag/cmd_ingest.go
• cmd/rag/cmd_query.go
• cmd/rag/cmd_rag.go
• docs/architecture.md
• docs/index.md
• endpoint.go
• go.mod
• helpers.go
• ingest.go
• integration_test.go
• keyword.go
• ollama.go
• qdrant.go
• qdrant_test.go
• query.go
• query_test.go
• rfc_features_test.go
• specs/forge.lthn.ai/core/go-rag.md
• specs/forge.lthn.ai/core/go-rag/cmd/rag.md
• test_helpers_test.go
• tests/cli/rag/main.go

📝 Walkthrough

Walkthrough

This pull request enhances the RAG system with sentence- and paragraph-based chunking, adds TF-IDF keyword search fallback, implements PDF ingestion support, introduces batched embedding, improves query result ranking, and consolidates utility imports to dappco.re/go/core whilst removing the testify dependency.

Changes

Cohort / File(s)	Summary
Chunking Primitives chunk.go, chunk_test.go	Added ChunkBySentences, ChunkByParagraphs and corresponding sequential variants. Refactored markdown chunking to use rune-safe length checks, improved section title parsing with parent heading context, and replaced sentence-only splitting with word-fallback logic. Extended FileExtensions to include .pdf and updated ShouldProcess to use core.PathExt/core.Lower.
Keyword Search keyword.go, keyword_test.go	Implemented KeywordIndex with TF-IDF scoring and SearchKeywords/SearchKeywordsSeq functions for local keyword search fallback. Updated KeywordFilter to use core.Lower, added keyword deduplication, and extended sorting with additional tie-breakers. Added comprehensive test coverage for indexing, ranking, and edge cases.
Ingestion Pipeline ingest.go, ingest_test.go	Refactored filesystem handling to use core.Fs, added PDF text extraction via readPDFDocument with plaintext fallback, introduced batched embedding via embedChunkBatch and embedBatchConcurrent, and improved error tracking per-file/chunk. Updated error handling to use core.E and replaced direct os/filepath calls with core equivalents.
Query & Results query.go, query_test.go, vectorstore.go	Added Index field and accessor methods to QueryResult. Introduced generic Rank function for deterministic result sorting with tie-breakers and deduplication. Updated output formatting to use core.NewBuilder/core.Sprintf and JSON marshalling via core.JSONMarshalString. Expanded Vector struct and CollectionInfo with metadata fields.
Qdrant Integration qdrant.go, qdrant_test.go, qdrant_integration_test.go	Added NewQdrantStore(endpoint) constructor with automatic URL parsing and scheme-based TLS handling. Enhanced SearchResult with denormalised fields (Text, Source, Section, ChunkIndex, Index) and accessor methods. Added CollectionInfo metadata fields and improved index type inference. Implemented Add convenience method.
Ollama Integration ollama.go, ollama_test.go, ollama_integration_test.go, endpoint.go, endpoint_test.go	Added NewOllamaEmbedder(endpoint, model) constructor with endpoint parsing. Extended OllamaConfig with URL scheme/host/port fields. Refactored NewOllamaClient to use configurable scheme. Added parseEndpointURL helper and integration tests for endpoint configuration parsing. Updated error handling to use core.E.
Helper Functions helpers.go, helpers_test.go	Added JoinResults generic function for concatenating results, embedBatchConcurrent for parallel per-text embedding, and clamped negative topK in QueryWith. Updated error handling to use core.E and replaced log.E across connection failures. Expanded test coverage for new helpers.
Collections API collections.go, collections_test.go	Updated documentation comments for ListCollections, ListCollectionsSeq, DeleteCollection, and CollectionStats with usage examples. Adjusted test expectations to validate new CollectionStats fields (Count, Vectors, Index).
CLI Commands cmd/rag/cmd_ingest.go, cmd/rag/cmd_query.go, cmd/rag/cmd_collections.go, cmd/rag/cmd_commands.go, cmd/rag/cmd_rag.go	Refactored output handling to use cmd.OutOrStdout() and core.Print/core.Sprintf. Added sync.Once guards for flag and subcommand registration to ensure idempotency. Added --keywords flag to query command. Updated import paths to use dappco.re/go/core umbrella and separate dappco.re/go/cli/dappco.re/go/i18n packages. Switched error creation from log.E to core.E.
CLI Test Harness cmd/rag/cmd_commands_test.go, tests/cli/rag/main.go, tests/cli/rag/Taskfile.yaml	Added idempotency test for AddRAGSubcommands. Introduced new CLI test entrypoint with command registration and help validation. Created Taskfile for isolated build/execution testing.
Test Utilities test_helpers_test.go, mock_test.go, benchmark_gpu_test.go, benchmark_test.go	Introduced comprehensive assertion helper suite (assertLen, assertEqual, assertContains, assertInDelta, etc.) to replace testify dependency. Refactored benchmarks to use dappco.re/go/core utilities instead of standard library functions. Updated mock implementations to use core.E and new stats fields. Renamed benchmark test functions with _Ugly suffix.
Integration Tests integration_test.go, qdrant_integration_test.go, ollama_integration_test.go	Renamed test entrypoints with _Ugly suffix. Removed testify dependency and switched to custom assertion helpers. Updated string/path construction to use core.Sprintf/core.JoinPath. Consolidated error assertion patterns.
Documentation & Specifications README.md, docs/architecture.md, docs/index.md, specs/forge.lthn.ai/core/go-rag.md, specs/forge.lthn.ai/core/go-rag/cmd/rag.md, specs/rag/RFC.md, embedder.go, vectorstore.go	Updated README with enhanced chunking capabilities and TF-IDF fallback. Updated architecture documentation to describe PDF extraction, FileExtensions API, and Ollama order preservation. Added CLI example with --keywords flag. Created comprehensive package specification documenting chunking, ingest, query, and backend implementations. Updated interface documentation examples from assertions to commented usage patterns.
Dependencies go.mod	Updated module path from dappco.re/go/core/rag to dappco.re/go/rag. Refactored direct dependencies: replaced granular dappco.re/go/core/{cli,i18n,io,log} imports with umbrella dappco.re/go/core, added separate dappco.re/go/cli, dappco.re/go/i18n, and new github.com/ledongthuc/pdf. Updated indirect dependencies to use consolidated dappco.re/go/inference. Removed testify dependency.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.35% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title describes a routine dev→main PR for CodeRabbit review, which partially relates to the changeset; however, it does not clearly summarise the primary functional changes (chunking enhancements, keyword features, RFC alignment, testify removal, and dependency updates).	Consider a more specific title that highlights the main change—for example: 'feat: enhance chunking, add keyword search, align with RAG RFC, and migrate dependencies' or similar, to better communicate the scope of 77 commits to reviewers scanning history.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

docs/index.md (1)

79-79: ⚠️ Potential issue | 🟡 Minor

Documentation lists testify but it has been removed from dependencies.

The dependencies table still lists github.com/stretchr/testify as a test-only dependency, but the go.mod changes show testify has been removed from the project. This documentation is now stale.

📝 Proposed fix

-| `github.com/stretchr/testify` | Test assertions (test-only) |
+| `github.com/ledongthuc/pdf` | PDF text extraction for ingestion pipeline |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/index.md` at line 79, The dependencies table in docs/index.md still
lists `github.com/stretchr/testify` but it was removed from go.mod; update the
docs by removing the table row that references `github.com/stretchr/testify`
(the line containing "`github.com/stretchr/testify` | Test assertions
(test-only)") or replace it with the current test dependency if applicable so
the documentation matches the repository dependencies.

docs/architecture.md (1)

149-156: ⚠️ Potential issue | 🟡 Minor

Update the ingestion step to match the batched embedding path.

Step 5 still says ingestion embeds each chunk individually, but this PR adds batch/Ollama embedding support. Please describe the actual batching/concurrency behaviour here so the architecture doc does not drift from the implementation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/architecture.md` around lines 149 - 156, The doc text for Ingest(...) is
out of date: instead of embedding each chunk individually, the implementation
batches chunks and sends them to the embedder in batches (controlled by
BatchSize) and may embed batches concurrently; update step 5 to state that after
ChunkMarkdown is called you accumulate chunks (across files as implemented),
group them into batches of up to BatchSize, call the embedder's batch embed API
(preserving chunk order so returned embeddings map back to chunks), construct
Point structs from the batch of embeddings, and then send those Points to the
batch upsert path (which also operates in BatchSize slices); mention any
concurrency/workers used by the implementation when embedding batches and that
embedding and upsert are done per-batch rather than per-chunk.

cmd/rag/cmd_ingest.go (1)

37-74: ⚠️ Potential issue | 🟠 Major

Validate local chunk flags before touching Qdrant or Ollama.

--chunk-size / --chunk-overlap are only rejected after both clients are created and the model check has run, so a bad local flag can be masked by unrelated connectivity failures and still burn external calls.

Suggested change

 	ctx := context.Background()
 	out := cmd.OutOrStdout()
 
+	// Validate local config first.
+	if chunkSize <= 0 {
+		return core.E("rag.cmd.ingest", "chunk-size must be > 0", nil)
+	}
+	if chunkOverlap < 0 || chunkOverlap >= chunkSize {
+		return core.E("rag.cmd.ingest", "chunk-overlap must be >= 0 and < chunk-size", nil)
+	}
+
 	// Connect to Qdrant
 	core.Print(out, "Connecting to Qdrant at %s:%d...", qdrantHost, qdrantPort)
@@
-	// Configure ingestion
-	if chunkSize <= 0 {
-		return core.E("rag.cmd.ingest", "chunk-size must be > 0", nil)
-	}
-	if chunkOverlap < 0 || chunkOverlap >= chunkSize {
-		return core.E("rag.cmd.ingest", "chunk-overlap must be >= 0 and < chunk-size", nil)
-	}
-
 	cfg := rag.IngestConfig{

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/rag/cmd_ingest.go` around lines 37 - 74, Validate the local ingestion
flags chunkSize and chunkOverlap before any network calls: move the checks for
chunkSize <= 0 and chunkOverlap < 0 || chunkOverlap >= chunkSize to run before
creating the Qdrant and Ollama clients (before calls to rag.NewQdrantClient,
rag.NewOllamaClient, qdrantClient.HealthCheck, and ollamaClient.VerifyModel).
Keep the same error returns (core.E with "rag.cmd.ingest" and the existing
messages) so invalid local flags short-circuit the command without attempting
NewQdrantClient, NewOllamaClient, HealthCheck, or VerifyModel.

ingest_test.go (1)

25-520: ⚠️ Potential issue | 🟠 Major

Use testify assertions in this _test.go file.

The new helper-based assertions (assertNoError, assertEqual, assertLen, etc.) violate the repository test standard and make this suite inconsistent with the expected assertion style.

Suggested pattern

- assertNoError(t, err)
+ require.NoError(t, err)

- assertEqual(t, 1, stats.Files)
+ assert.Equal(t, 1, stats.Files)

As per coding guidelines **/*_test.go: Use testify assert/require for writing tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ingest_test.go` around lines 25 - 520, Replace all custom test helper
assertions (assertNoError, assertEqual, assertLen, assertContains,
assertGreater, assertNotEmpty, assertEmpty, assertError, assertTruef, etc.) in
this file with testify Assert/Require calls (e.g., assert.NoError(t, err),
assert.Equal(t, expected, actual), assert.Len(t, obj, n), assert.Contains(t, s,
substr), assert.Greater(t, a, b), assert.NotEmpty(t, v), assert.Empty(t, v),
assert.Error(t, err), assert.Truef(t, cond, format, args...)); add the testify
import ("github.com/stretchr/testify/assert") at the top of the test file (and
require if you prefer fatal assertions for setup), and update the writeFile
helper to use assert.Truef (or require.Truef) instead of assertTruef; ensure all
assertion argument order matches testify (expected before actual where
applicable) and remove or stop using the old helper names so tests conform to
the repository standard (references: Ingest, IngestFile, writeFile, and all test
cases in this file).

🧹 Nitpick comments (5)

endpoint.go (1)

9-11: Use UK English spelling in comments.

The comment uses "normalizes" which is US English spelling. As per coding guidelines, UK English should be used.

📝 Proposed fix

-// parseEndpointURL normalizes host-style endpoints into a parsed URL.
+// parseEndpointURL normalises host-style endpoints into a parsed URL.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@endpoint.go` around lines 9 - 11, Update the comment(s) in endpoint.go to use
UK English spelling: change "normalizes" to "normalises" in the parseEndpointURL
comment (and any other occurrences in nearby comments), e.g. the header sentence
describing parseEndpointURL and the inline description "are treated as HTTP
URLs" if it contains US spellings; retain original meaning and punctuation while
only adjusting spelling to UK conventions.

go.mod (1)

6-6: Duplicate dependency entry for dappco.re/go/core.

dappco.re/go/core v0.8.0-alpha.1 appears in both the direct require block (line 6) and indirect require block (line 15). While Go modules will dedupe this, having the same package in both blocks is redundant. Consider removing the indirect entry.

📝 Proposed fix

 require (
-	dappco.re/go/core v0.8.0-alpha.1 // indirect
 	dappco.re/go/inference v0.8.0-alpha.1 // indirect

Also applies to: 15-15

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@go.mod` at line 6, Remove the redundant indirect require for the module
dappco.re/go/core v0.8.0-alpha.1 so it only appears once in the go.mod require
block; locate the duplicate entry named "dappco.re/go/core v0.8.0-alpha.1" and
delete the indirect occurrence (the one under the indirect/require block)
leaving the direct require intact to avoid duplication.

test_helpers_test.go (1)

366-377: lengthOf(nil) returns (0, true), which differs from testify semantics.

This causes assertLen(t, nil, 0) to pass silently, whereas testify's assert.Len would fail on a nil value. If intentional, this is fine, but it may mask bugs where code incorrectly returns nil instead of an empty slice.

🔧 Consider aligning with testify behaviour

 func lengthOf(value any) (int, bool) {
 	if value == nil {
-		return 0, true
+		return 0, false
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test_helpers_test.go` around lines 366 - 377, The helper lengthOf currently
returns (0, true) for value == nil which lets assertLen(t, nil, 0) pass; change
lengthOf so nil values are treated as non-collections: return (0, false) when
value == nil, and for reflected values that can be nil (Slice, Map, Chan, Ptr,
Interface, Func) return (0, false) if v.IsNil(); otherwise keep the existing
Kind-based v.Len() behavior for non-nil collections. Update function lengthOf
accordingly to match testify semantics.

integration_test.go (1)

418-422: writeTestFile is now a thin wrapper.

The function correctly delegates to writeFile(t, path, content). Consider removing this wrapper entirely and using writeFile directly if no additional logic is planned.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@integration_test.go` around lines 418 - 422, The writeTestFile wrapper is
redundant—remove the writeTestFile function and update all call sites to call
writeFile(t, path, content) directly (search for usages of writeTestFile and
replace them), then run the test suite to ensure nothing else depends on the
wrapper; if any call sites relied on helper semantics, migrate them to writeFile
or reintroduce needed behavior inside writeFile instead.

tests/cli/rag/main.go (1)

12-13: Mirror the production command tree in this smoke driver.

Mounting rag directly under a synthetic root means this helper never exercises the real core ai rag path, so wiring regressions under the ai parent can slip through.

Suggested change

 func main() {
-	root := cli.NewGroup("core-rag", "RAG CLI artifact test driver", "")
-	ragcmd.AddRAGSubcommands(root)
+	root := cli.NewGroup("core", "RAG CLI artifact test driver", "")
+	ai := cli.NewGroup("ai", "", "")
+	root.AddCommand(ai)
+	ragcmd.AddRAGSubcommands(ai)
 	root.SetArgs(os.Args[1:])

Based on learnings, AddRAGSubcommands() is mounted under core ai rag.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/cli/rag/main.go` around lines 12 - 13, The test driver currently mounts
RAG under a synthetic root ("core-rag") so it doesn't exercise the real command
path; change the setup to mirror production by creating the parent groups and
mounting AddRAGSubcommands under the "core" → "ai" → "rag" hierarchy instead of
directly on the synthetic root (use cli.NewGroup to create the "core" group,
then an "ai" subgroup, then call ragcmd.AddRAGSubcommands on the "ai" subgroup
or its "rag" child) so the test exercises the same command tree as production.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@chunk_test.go`:
- Around line 9-17: The test TestChunk_ChunkMarkdown_Good_SmallSection uses
custom helpers assertLen and assertContains instead of the repo standard testify
assertions; update this test to import github.com/stretchr/testify/require (or
assert) and replace assertLen(t, chunks, 1) with require.Len(t, chunks, 1) (or
assert.Len) and replace assertContains(t, chunks[0].Text, "small section") with
require.Contains(t, chunks[0].Text, "small section"); keep the tested functions
ChunkMarkdown and DefaultChunkConfig unchanged.

In `@cmd/rag/cmd_commands_test.go`:
- Around line 15-24: Replace the manual t.Fatalf checks with testify requires:
import "github.com/stretchr/testify/require" in cmd_commands_test.go, call
children := parent.Commands() then use require.Len(t, children, 1) instead of
the first length check, use require.Equal(t, "rag", children[0].Name()) instead
of the Name() t.Fatalf, and use require.Len(t, children[0].Commands(), 3)
instead of the final length check so failures are reported using testify's
require helpers.

In `@cmd/rag/cmd_query.go`:
- Around line 73-79: Replace direct calls to core.Println in the result
formatting switch with writes to the command writer returned by
cmd.OutOrStdout() so redirected callers capture output; specifically, for the
cases that call rag.FormatResultsJSON(results),
rag.FormatResultsContext(results), and rag.FormatResultsText(results) write
their returned strings to cmd.OutOrStdout() (e.g., using fmt.Fprintln or
io.WriteString) instead of calling core.Println.

In `@cmd/rag/cmd_rag.go`:
- Around line 39-45: The current init code silently ignores non-numeric
QDRANT_PORT (and similarly OLLAMA_PORT) by falling back to defaults; change the
logic in the rag command setup so that when core.Env("QDRANT_PORT") (and
core.Env("OLLAMA_PORT")) returns a non-empty string and strconv.Atoi fails, you
fail fast (return an error or call a fatal/exit path) instead of using the
default. Locate the qPort variable and its parsing block (where strconv.Atoi is
called) and the analogous OLLAMA_PORT parsing, and replace the silent-ignore
behavior with an explicit error message that includes the invalid value and stop
startup (e.g., processLogger.Fatalf / return fmt.Errorf) before calling
ragCmd.PersistentFlags().IntVar with qdrantPort/ollamaPort.

In `@helpers.go`:
- Around line 160-185: The embedBatchConcurrent function launches an unbounded
goroutine per text which can overload the embed service; limit concurrency by
adding a semaphore or worker-pool (e.g., a buffered channel or worker
goroutines) to cap simultaneous embed calls, acquire before calling embed(ctx,
text) and release after; keep using vectors and errs slices indexed by i and the
existing wait group wg (or replace wg with worker pool coordination), ensure
embed errors are written into errs[i] and successful vectors[i] set, and return
the same [][]float32 and []error shape.

In `@ingest.go`:
- Around line 62-69: The code does an unchecked type assertion on
infoResult.Value in the ingest flow: after calling localFS.Stat(scanRoot) check
infoResult.OK and then assert the type safely by using the comma-ok form (e.g.,
info, ok := infoResult.Value.(fs.FileInfo)); if ok is false return a wrapped
error (using core.Wrap or core.E) indicating an unexpected type from Stat and
include infoResult.Value or its type in the message; keep the subsequent IsDir()
check only when the assertion succeeds.
- Around line 243-258: In collectMarkdownFiles, the code does a direct type
assertion listResult.Value.([]fs.DirEntry) which can panic if the type is
unexpected; change this to a safe assertion (val, ok :=
listResult.Value.([]fs.DirEntry)) and handle the !ok case by returning a
suitable error (e.g., via resultError or a new error describing unexpected List
value) before proceeding to sort; ensure you reference listResult.Value and
update any downstream uses (entries) to use the safely asserted val.

In `@keyword_test.go`:
- Around line 19-21: The test uses local helper assertions (assertLen,
assertEqual) but must use testify's assert/require; replace calls to
assertLen(t, filtered, 2) and assertEqual(t, float32(0.9), filtered[0].Score) /
assertEqual(t, float32(0.8), filtered[1].Score) with the testify equivalents
(e.g., assert.Len(t, filtered, 2) and assert.Equal(t, float32(0.9),
filtered[0].Score), assert.Equal(t, float32(0.8), filtered[1].Score)) and ensure
the test imports the "github.com/stretchr/testify/assert" package (or require if
appropriate).

In `@ollama.go`:
- Around line 95-99: The code silently ignores strconv.Atoi errors when parsing
parsed.Port(), which can hide invalid endpoint configs; update the function that
sets cfg.Port to handle the error from strconv.Atoi(portText) instead of
ignoring it—either return the parse error upstream or log it with context
(including portText and the relevant config/endpoint) and avoid silently keeping
the default cfg.Port; locate the block using parsed.Port(), strconv.Atoi, and
cfg.Port and change the if err == nil branch to handle the err case (return an
error or call the package logger with a clear message and the invalid portText).

In `@qdrant.go`:
- Around line 285-302: GetChunkIndex uses 0 as a sentinel so a real chunk index
of 0 is treated as "unset" and falls back to Payload; change the data model and
accessor to distinguish "unset" from 0 by either making SearchResult.ChunkIndex
a pointer (*int) or adding an explicit presence flag (e.g., ChunkIndexPresent
bool), then update SearchResult.GetChunkIndex to check the presence flag or
nil-pointer before falling back to Index or Payload; keep the same Payload
parsing logic (reading "chunk_index" from Payload) but only consult it when the
explicit presence check indicates ChunkIndex is unset.
- Around line 66-69: When parsing the endpoint port, don’t silently keep the
default on parse errors: if parsed.Port() != "" then attempt strconv.Atoi on it
and if Atoi returns an error return that error (or wrap it) instead of leaving
cfg.Port unchanged; additionally validate the parsed port is within 1–65535 and
reject out-of-range values (again returning an error), otherwise set cfg.Port to
the validated integer. Update the code paths handling parsed.Port(),
strconv.Atoi, and cfg.Port to implement this fail-fast validation.

In `@query.go`:
- Around line 55-63: GetChunkIndex() conflates a real first chunk (0) with "no
chunk metadata" causing rankKey/deduplication to collapse all chunk-less hits to
source:0; change the behavior so missing chunk metadata is represented by a
sentinel (e.g. -1) instead of 0 or make rankKey detect absence explicitly:
update QueryResult.GetChunkIndex to return a distinct sentinel like -1 when
neither ChunkIndex nor Index is set, and update any callers (notably rankKey) to
use that sentinel when building dedupe keys (or include the sentinel in the key)
so chunk-less hits from the same source are not merged into source:0.

In `@README.md`:
- Around line 38-42: The README example block is inconsistent: the tagged test
command lacks the workspace override; update the line containing "go test -tags
rag ./..." to include the workspace flag so it reads "GOWORK=off go test -tags
rag ./..." ensuring all commands in the block consistently use GOWORK=off.

In `@specs/forge.lthn.ai/core/go-rag.md`:
- Line 10: Update US English spellings to UK English across the spec: change
occurrences of "normalizes"/"normalized"/"serializes" (and any other US forms
like "colorize"/"organization"/"initialize"/"behavior") to
"normalises"/"normalised"/"serialises" (and
"colourise"/"organisation"/"initialise"/"behaviour") throughout the document,
including in descriptions for ChunkMarkdownSeq, Size and Overlap and any related
headings or examples; ensure all .md and .go references follow the project's UK
English convention.
- Line 107: The documentation is inconsistent: FileExtensions() lists `.pdf` as
ingestible but the Ingest description omits `.pdf`; update the Ingest docs to
include `.pdf` (or, if intended otherwise, remove `.pdf` from FileExtensions())
so both statements match—specifically edit the Ingest section text to mention
`.md`, `.markdown`, `.pdf`, and `.txt` to align with the FileExtensions()
symbol.

In `@specs/forge.lthn.ai/core/go-rag/cmd/rag.md`:
- Around line 13-14: Update the spec text for the AddRAGSubcommands entry to use
UK English: change the word "Initializes" to "Initialises" in the description
for `func AddRAGSubcommands(parent *cli.Command)` so the doc string matches the
repo's UK spelling convention (e.g., "Initialises the package flag set through
`initFlags`, adds ...").

In `@test_helpers_test.go`:
- Around line 1-14: The test introduces custom assertion helpers
(assertionHelper and testAssert) but the project guideline CLAUDE.md mandates
using testify's assert/require; resolve by either restoring testify-based
assertions (replace usage of assertionHelper/testAssert with testify's
assert/require in test_helpers_test.go and re-add testify import) or update
CLAUDE.md to explicitly allow and document the new custom testing patterns
(mentioning assertionHelper/testAssert semantics). Make the change consistently
across tests so the codebase and CLAUDE.md policy match.

---

Outside diff comments:
In `@cmd/rag/cmd_ingest.go`:
- Around line 37-74: Validate the local ingestion flags chunkSize and
chunkOverlap before any network calls: move the checks for chunkSize <= 0 and
chunkOverlap < 0 || chunkOverlap >= chunkSize to run before creating the Qdrant
and Ollama clients (before calls to rag.NewQdrantClient, rag.NewOllamaClient,
qdrantClient.HealthCheck, and ollamaClient.VerifyModel). Keep the same error
returns (core.E with "rag.cmd.ingest" and the existing messages) so invalid
local flags short-circuit the command without attempting NewQdrantClient,
NewOllamaClient, HealthCheck, or VerifyModel.

In `@docs/architecture.md`:
- Around line 149-156: The doc text for Ingest(...) is out of date: instead of
embedding each chunk individually, the implementation batches chunks and sends
them to the embedder in batches (controlled by BatchSize) and may embed batches
concurrently; update step 5 to state that after ChunkMarkdown is called you
accumulate chunks (across files as implemented), group them into batches of up
to BatchSize, call the embedder's batch embed API (preserving chunk order so
returned embeddings map back to chunks), construct Point structs from the batch
of embeddings, and then send those Points to the batch upsert path (which also
operates in BatchSize slices); mention any concurrency/workers used by the
implementation when embedding batches and that embedding and upsert are done
per-batch rather than per-chunk.

In `@docs/index.md`:
- Line 79: The dependencies table in docs/index.md still lists
`github.com/stretchr/testify` but it was removed from go.mod; update the docs by
removing the table row that references `github.com/stretchr/testify` (the line
containing "`github.com/stretchr/testify` | Test assertions (test-only)") or
replace it with the current test dependency if applicable so the documentation
matches the repository dependencies.

In `@ingest_test.go`:
- Around line 25-520: Replace all custom test helper assertions (assertNoError,
assertEqual, assertLen, assertContains, assertGreater, assertNotEmpty,
assertEmpty, assertError, assertTruef, etc.) in this file with testify
Assert/Require calls (e.g., assert.NoError(t, err), assert.Equal(t, expected,
actual), assert.Len(t, obj, n), assert.Contains(t, s, substr), assert.Greater(t,
a, b), assert.NotEmpty(t, v), assert.Empty(t, v), assert.Error(t, err),
assert.Truef(t, cond, format, args...)); add the testify import
("github.com/stretchr/testify/assert") at the top of the test file (and require
if you prefer fatal assertions for setup), and update the writeFile helper to
use assert.Truef (or require.Truef) instead of assertTruef; ensure all assertion
argument order matches testify (expected before actual where applicable) and
remove or stop using the old helper names so tests conform to the repository
standard (references: Ingest, IngestFile, writeFile, and all test cases in this
file).

---

Nitpick comments:
In `@endpoint.go`:
- Around line 9-11: Update the comment(s) in endpoint.go to use UK English
spelling: change "normalizes" to "normalises" in the parseEndpointURL comment
(and any other occurrences in nearby comments), e.g. the header sentence
describing parseEndpointURL and the inline description "are treated as HTTP
URLs" if it contains US spellings; retain original meaning and punctuation while
only adjusting spelling to UK conventions.

In `@go.mod`:
- Line 6: Remove the redundant indirect require for the module dappco.re/go/core
v0.8.0-alpha.1 so it only appears once in the go.mod require block; locate the
duplicate entry named "dappco.re/go/core v0.8.0-alpha.1" and delete the indirect
occurrence (the one under the indirect/require block) leaving the direct require
intact to avoid duplication.

In `@integration_test.go`:
- Around line 418-422: The writeTestFile wrapper is redundant—remove the
writeTestFile function and update all call sites to call writeFile(t, path,
content) directly (search for usages of writeTestFile and replace them), then
run the test suite to ensure nothing else depends on the wrapper; if any call
sites relied on helper semantics, migrate them to writeFile or reintroduce
needed behavior inside writeFile instead.

In `@test_helpers_test.go`:
- Around line 366-377: The helper lengthOf currently returns (0, true) for value
== nil which lets assertLen(t, nil, 0) pass; change lengthOf so nil values are
treated as non-collections: return (0, false) when value == nil, and for
reflected values that can be nil (Slice, Map, Chan, Ptr, Interface, Func) return
(0, false) if v.IsNil(); otherwise keep the existing Kind-based v.Len() behavior
for non-nil collections. Update function lengthOf accordingly to match testify
semantics.

In `@tests/cli/rag/main.go`:
- Around line 12-13: The test driver currently mounts RAG under a synthetic root
("core-rag") so it doesn't exercise the real command path; change the setup to
mirror production by creating the parent groups and mounting AddRAGSubcommands
under the "core" → "ai" → "rag" hierarchy instead of directly on the synthetic
root (use cli.NewGroup to create the "core" group, then an "ai" subgroup, then
call ragcmd.AddRAGSubcommands on the "ai" subgroup or its "rag" child) so the
test exercises the same command tree as production.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7b01dfcb-a14c-4b3f-942a-66fb0674d7f6

📥 Commits

Reviewing files that changed from the base of the PR and between c60fd0b and 12b06a6.

⛔ Files ignored due to path filters (3)

• go.sum is excluded by !**/*.sum
• go.work is excluded by !**/*.work
• go.work.sum is excluded by !**/*.sum

📒 Files selected for processing (42)

• README.md
• benchmark_gpu_test.go
• benchmark_test.go
• chunk.go
• chunk_test.go
• cmd/rag/cmd_collections.go
• cmd/rag/cmd_commands.go
• cmd/rag/cmd_commands_test.go
• cmd/rag/cmd_ingest.go
• cmd/rag/cmd_query.go
• cmd/rag/cmd_rag.go
• collections.go
• collections_test.go
• docs/architecture.md
• docs/index.md
• embedder.go
• endpoint.go
• endpoint_test.go
• go.mod
• helpers.go
• helpers_test.go
• ingest.go
• ingest_test.go
• integration_test.go
• keyword.go
• keyword_test.go
• mock_test.go
• ollama.go
• ollama_integration_test.go
• ollama_test.go
• qdrant.go
• qdrant_integration_test.go
• qdrant_test.go
• query.go
• query_test.go
• specs/forge.lthn.ai/core/go-rag.md
• specs/forge.lthn.ai/core/go-rag/cmd/rag.md
• specs/rag/RFC.md
• test_helpers_test.go
• tests/cli/rag/Taskfile.yaml
• tests/cli/rag/main.go
• vectorstore.go

[Snider]

RESOLVED-COMMENT: for the outside-diff ingest_test.go/testify recommendation, I am not reintroducing testify. AX-6 shifted this repo to stdlib testing plus local helpers; adding github.com/stretchr/testify back would violate the active convention. I removed the remaining testify import/checksum from the branch and updated CLAUDE.md so the documented policy matches the code.

[Snider]

Submitting AX-6/testify disposition replies: these CodeRabbit suggestions are resolved by convention documentation and dependency removal, not by reintroducing testify.

[Snider]

Submitting AX-6/testify disposition replies: these CodeRabbit suggestions are resolved by convention documentation and dependency removal, not by reintroducing testify.