[bawoodruff]

Problem

kg index/kg migrate logged context deadline exceeded warnings on HLS .ts video segments (MPEG transport stream) in depop-public-cloudfront-s3-assets. The tree-sitter indexer maps .ts → the TypeScript parser by extension alone, so a binary segment is fed to the parser, which chews on it until the 10s parse timeout — once per file, on every re-index.

The graph was never corrupted (the files are skipped, no entities written) — but each misidentified file burned the full deadline and logged noise.

Fix

A binary-content guard in processWithTreeSitter, right after os.ReadFile and before the parser runs / the file entity is written: if the file's head contains a NUL byte it's binary (valid source text never contains NUL), so skip it cleanly (return nil).

if bytes.IndexByte(src[:min(len(src), 8000)], 0) >= 0 {
    return nil
}

This matches git's own binary-detection heuristic and catches the whole class (any binary sharing a source extension), not just MPEG-TS. Returning nil is a clean skip — consistent with how the indexer already tolerates unparseable files.

Test

TestIndexer_BinaryTSSegment writes a synthetic MPEG-TS .ts (0x47 sync-byte packets + NUL padding), drives it through the real indexing path, and asserts no file: entity is created and stats.Errors == 0. Verified fail-before / pass-after. Existing positive .ts coverage (TestIndexer_TSFile) still passes. (The synthetic blob parses fast, so the deterministic unit signal is the written file entity, not the 10s stall the real segments cause — the test comment states this honestly.)

• go build, go vet, gofmt -l, go test ./... clean in both src/kg and src/kglib.

Generated with Claude Code

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: src/kg/internal/knowledge/indexer_treesitter.go, src/kg/internal/knowledge/indexer_treesitter_test.go (plus surrounding context in indexer.go for callers/stats)
• Overall verdict: APPROVE

This is a small, correctly-scoped fix. processWithTreeSitter has exactly one caller (indexer.go:471), so there's no parallel code path (CLI/API, sync/async, etc.) that needed the same guard and was missed. The guard sits after os.ReadFile and before both parser.ParseCtx and the file: entity write, which matches the stated goal (skip before burning the 10s deadline, no partial/garbage entity). The bytes.IndexByte(src[:min(len(src), 8000)], 0) bound is panic-safe for empty files (src[:0]), and min is a valid builtin under the module's go 1.26.2. The new test (TestIndexer_BinaryTSSegment) drives the real Index() path, asserts the correct entity name (relPath = "clip.ts") against the correct EntityTypeFile, and honestly documents in its comment that the synthetic blob exercises the "no entity written" signal rather than the real 10s stall — that's a legitimate, non-misleading substitute for a timing-based assertion. No other extension-routed path (.md, .yaml, .html, asm) shares the 10s tree-sitter timeout this PR targets, so narrowing the fix to processWithTreeSitter is correct.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

None.

Minor Issues (optional)

• src/kg/internal/knowledge/indexer_treesitter.go:582 — The NUL-in-head heuristic (like git's) will also silently and completely skip a legitimate source file sharing a tracked extension if it happens to be UTF-16/UTF-32 encoded (NUL bytes interleaved with ASCII). Before this change such a file would still get a file: entity (likely with poor symbol extraction); after, it gets nothing and no error/log. Given the tracked extensions (.go, .py, .rs, .cs, .ts, …) rarely appear as UTF-16 in practice, this is low-probability, but consider a one-line comment acknowledging the known tradeoff (you already cite git's heuristic as precedent, which has the same limitation) or a stats counter for skipped-binary files so this failure mode isn't completely invisible if it ever fires on a real repo.
• src/kg/internal/knowledge/indexer_treesitter.go:586 — 8000 is a bare magic number; a named constant (e.g. const binarySniffBytes = 8000) with a one-line "matches git's core.bigFileThreshold-independent binary-detection sniff length" note would make the git-parity claim self-documenting.
• CHANGELOG.md — The ### Fixed section under [Unreleased] documents comparable bug fixes with PR references (e.g. "Legacy PDF cleanup matches extensions case-insensitively."); this fix isn't recorded there despite matching that pattern exactly. Not required for correctness, but keeps the changelog convention intact.

Security Findings

None.

Positive Observations

• Single entry point for the fix (no divergent/parallel indexing path missed).
• Guard placed correctly before both the expensive parse and the entity write — matches the stated "clean skip" semantics and doesn't inflate stats.Errors.
• New test exercises the real Index() pipeline (not a unit-level call to the guard in isolation) and is explicit/honest in its doc comment about what it does and doesn't prove.
• Existing positive .ts coverage (TestIndexer_TSFile) is unaffected; no cross-module (kglib/markitdown) changes were needed or made, correctly recognizing this is purely a kg-indexer concern.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: CHANGELOG.md, src/kg/index.go, src/kg/internal/knowledge/indexer.go, src/kg/internal/knowledge/indexer_treesitter.go, src/kg/internal/knowledge/indexer_treesitter_test.go, plus src/kg/internal/knowledge/mcp_server.go for the second Index() call site.
• Overall verdict: REQUEST (comment-level) — this is a solid, correctly-scoped fix that also addresses all three minor nits from the prior review round (NUL-in-UTF-16 tradeoff documented, binarySniffBytes named constant, CHANGELOG entry added). One Major gap remains in the newly-added observability itself.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

• src/kg/internal/knowledge/mcp_server.go:271-279 — index_project (the MCP tool entry point to Indexer.Index()) builds its summary message from stats.FilesScanned, stats.EntitiesCreated, stats.RelationsCreated but never surfaces stats.BinarySkipped. This PR's whole stated purpose for adding BinarySkipped (per the second commit message: "so a legitimate but UTF-16/32-encoded source file wrongly skipped isn't invisible") is only realized on the CLI path (src/kg/index.go:171-173); the MCP path — the second and only other caller of Index() — remains exactly as invisible as before. Add a parallel if stats.BinarySkipped > 0 { ... } clause to the msg built at mcp_server.go:272, mirroring the CLI's conditional line, so both entry points honor the new observability guarantee.

Minor Issues (optional)

None beyond what the prior review round already raised (and this PR has since fixed: the UTF-16/32 caveat comment, the binarySniffBytes constant, and the CHANGELOG entry are all present).

Security Findings

None.

Positive Observations

• Correctly addresses all three minor items from the prior review pass (comment on the known UTF-16/32 tradeoff, named binarySniffBytes constant with git-parity rationale, CHANGELOG entry under [Unreleased] Fixed).
• BinarySkipped is counted alongside, not instead of, FilesScanned (indexer.go:479) and not counted as an error — consistent with the "clean skip" semantics and doesn't skew error-rate signals.
• processWithTreeSitter is called from a single sequential filepath.Walk (no goroutines over stats), so the new stats.BinarySkipped++ has no data-race exposure.
• The new test (TestIndexer_BinaryTSSegment) exercises the real Index() pipeline, asserts on BinarySkipped, entity absence, and Errors == 0 together, and its doc comment honestly states what the synthetic blob does and doesn't prove versus the real 10s-stall scenario.
• src[:min(len(src), binarySniffBytes)] is panic-safe for empty files, and min is valid under this module's go 1.26.2.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: CHANGELOG.md, src/kg/index.go, src/kg/internal/knowledge/indexer.go, src/kg/internal/knowledge/indexer_treesitter.go, src/kg/internal/knowledge/indexer_treesitter_test.go, src/kg/internal/knowledge/mcp_server.go, src/kg/internal/knowledge/mcp_binary_summary_test.go (new)
• Overall verdict: APPROVE

This push closes out the one Major gap from the prior review round: index_project (src/kg/internal/knowledge/mcp_server.go:280-282) now appends a Binary files skipped: %d line mirroring the CLI's conditional at src/kg/index.go:171-173, so both call sites of Indexer.Index() (confirmed via grep — these are the only two) honor the observability guarantee that motivated adding IndexStats.BinarySkipped in the first place. The new mcp_binary_summary_test.go pins the behavior in both directions (present when BinarySkipped > 0, absent when zero), driving the real index_project MCP handler rather than asserting on the counter in isolation. IndexStats is freshly allocated per Index() call (indexer.go:407), so there's no cross-call leakage of the counter to worry about.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

None.

Minor Issues (optional)

None — all minors from the first review round (UTF-16/32 caveat comment, binarySniffBytes named constant, CHANGELOG entry) remain fixed, and this push introduces no new ones.

Security Findings

None.

Positive Observations

• Both Indexer.Index() call sites (CLI index.go and MCP mcp_server.go) now surface BinarySkipped with matching conditional formatting, closing the observability gap flagged previously.
• New tests exercise the real index_project MCP tool handler end-to-end (via buildTools/mcpCall) rather than unit-testing the summary string construction in isolation, and cover both the present and omitted cases symmetrically.
• IndexStats is allocated fresh per Index() invocation, so the counter can't bleed across repeated indexing calls on the same Indexer/MCP session.
• No cross-module changes needed or made — this remains correctly scoped to the kg module.