Skip to content

Fix the findings of the code review of #10 (security, correctness, performance, docs) - #11

Merged
dominikpeter merged 3 commits into
mainfrom
review-fixes-dominik
Oct 7, 2026
Merged

dominikpeter merged 3 commits into
mainfrom
review-fixes-dominik

Conversation

@dominikpeter

@dominikpeter dominikpeter commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Refs #3 (Test Embeddings): fixes the embedding-related findings (credentials, uploads, cache) of the review of #10, which delivered the embedding support and its evaluation.

Related issues: #1 (ablation data posted there), #2, #4 (release flow used to ship 0.3.1). Issue comments: #1, #2, #3, #4.

Summary

Follow-up to #10. #10 was merged while a full code review of that branch was still being fixed, so these fixes
never reached main. This PR contains exactly the difference between main and the reviewed, fixed tree
(19 files, +1,242 / -388, no files removed) plus the version bump to 0.3.1.

The review (architecture gate, security, two correctness reviewers, agent docs, performance, Python scripts,
duplication, then a verification pass on every blocking/should-fix candidate) found real defects; candidates
the verifiers could not reproduce were dropped. Everything below is fixed here, each with a regression test where
code is involved.

Release

Merging #10 bumped the version to 0.3.0 and published v0.3.0 from main, which contains the defects fixed
here. This PR bumps the version to 0.3.1, so merging it publishes the fixed release (the Release workflow runs
on merge to main).

What is fixed

Credentials and uploads (security)

  • An Azure Entra token was attached to any keyless endpoint (e.g. OPENAI_BASE_URL or a local server); a keyless
    OpenAI-compatible or Cohere endpoint now gets no credential. Reproduced with a fake endpoint and a fake az.
  • The API key was the first set variable regardless of the endpoint (an OpenAI key could be sent to a Cohere
    endpoint, an Azure key to a non-Azure one); it is now chosen from the endpoint's own provider.
  • Redirects are no longer followed (a redirect could replay an api-key header to another host); the key is
    redacted from Debug.
  • The local --embed presets (minilm|qwen3|potion) honored any DUPLICATECODE_EMBED_ENDPOINT, so a leftover
    remote value would receive all source text with Bearer local. They now accept loopback hosts only, and every
    --embed* run prints where the text is sent before sending it (plus a warning for plain http to a remote host).
  • scan/review/diff embedded every unit before applying --min-tokens/--skip-tests/boilerplate filters, so
    filtered units were uploaded for nothing; they are filtered first when embeddings are enabled (default output is
    unchanged).

Embedding cache

  • After a truncated or corrupt tail, every later append was unreadable, so the same texts were re-fetched on every
    run (a permanent re-billing loop for a paid API). The bad tail is now cut off before appending.
  • An embedding row that was missing, empty or non-numeric was stored as a valid zero-length vector and served as a
    cache hit forever; such rows are now rejected.
  • Entries over the u16 length fields desynced the file; they now stay in memory only. The cache is owner-only on
    unix, keys are 128-bit, and identifier-name phrases are capped.

Correctness

  • The k-gram prefilter claimed to be exact but dropped any pair sharing no 4-gram, even when the other features
    alone reach the threshold (so embedding-only matches never appeared in scan).
  • --embed-code forced renormalize on for every profile, changing copies scores beyond the embedding term;
    the embedding term is now dropped whenever a unit has no vector, independent of the profile.
  • Plain SQL queries were all named query, so any two queries matched by name; in the default profile two
    unrelated SELECTs scored 0.42 (above the diff threshold) and one shared filter pushed them to 0.65. They are now
    named like their file.
  • .js/.mjs/.cjs were parsed with the grammar that has no JSX, so React components in plain JS files produced no
    units; they now use the TSX grammar (JS benchmark scores are unchanged).
  • fragments and find with several roots dropped same-named files from different roots as "nested"; they now
    keep the root in file names like scan. --explain paired units by file:line, which mixed up units starting on
    the same line; it uses the full place.
  • Whole-file normalization: x = obj.attr inside functions was dropped as "script noise"; main() inlining
    deleted an else branch of the guard and corrupted multi-line strings; compound statements reported their whole
    body as differing lines in --explain and fragments.
  • Deeply nested input (about 17,000 levels) aborted the process with a stack overflow; extraction now runs on a
    large-stack thread (pre-existing for Python, newly reachable for SQL).

Performance (11k-unit repository, best of two runs, original main vs this tree): default scan 4.7 s to
4.0 s; scan --min-name 0 13.3 s to 6.9 s; --profile reimpl --min-name 0 4.0 s to 12.2 s. The review measured the
default scan 28% slower and reimpl 4.7x slower before these changes. reimpl stays about 3x slower than the
original because at its weights the k-gram prefilter cannot prune.

Benchmarks, tooling and docs

  • The CodeNet dev and holdout sets were described as disjoint but only differ by random seed: 13 of 60 Python and
    4 of 60 JavaScript problems appear in both. On holdout problems not in dev, Python scores 0.648 (0.659 on the full
    holdout) and JavaScript 0.497 (unchanged). Docs corrected; fetch_codenet.py --exclude-from builds a disjoint
    holdout.
  • Eval scripts hardened: argument parsing, a byte-offset bug in the TypeScript fragment generator, path filters
    that matched the user's own directory name, remote data used in file paths, a validated and allowlisted local
    embedding server, pinned dependency versions.
  • The repo's agent skill (skills/duplicate-code-review/SKILL.md), --help text and crate descriptions still
    described Python/TypeScript/C# only and no new commands; they now cover SQL/JS, fragments, find, --explain,
    --embed, the profile defaults and the fact that hosted embedding presets upload source text.

Tested

  • cargo fmt --check, cargo test --workspace --locked (52 tests, up from 34), clippy clean for new code.
  • Benchmark headline unchanged (dev 0.7827 / holdout 0.7607); original function-level bench top-1 rows unchanged
    against the earlier results; default-profile scan of a real Python repository still identical to the pre-Detection quality: SQL and JS/JSX support, optional embeddings, fragments/find/explain, eval harness #10
    main (209 pairs, 0 score differences); fragment recall unchanged (Python 0.93 / 0.97, TypeScript 0.975 / 0.97 for
    blocks of 6 / 8).
  • The CI wheel smoke test (build wheel, install, scan dataset) was run locally earlier. macOS and Windows are
    covered only by CI, which caught a Windows-only bug in the cache fix (an append handle cannot be truncated);
    it is fixed in the last commit, which also never cuts away records a concurrent writer appended meanwhile.

Not fixed here

--profile reimpl is slower than the original; SQL scoring compares every same-family unit; per-language rules are
still inlined in units.rs and whole-file pruning rewrites source text instead of the token stream; Unit.text
copies source for every unit. All are recorded in eval/REPORT.md.

History note

#10 was merged with its full commit history, which includes early versions of eval/REPORT.md that named an
internal repository and some of its folder, class and function names (no credentials or source code). The tip no
longer contains them; the history of main does. Rewriting main is a decision for the owner and is not part of
this PR.

🤖 Generated with Claude Code

This was referenced Oct 7, 2026
@dominikpeter
dominikpeter merged commit 68a89d1 into main Oct 7, 2026
5 checks passed
dominikpeter and others added 3 commits October 7, 2026 09:47
Security and robustness
- an Azure token is no longer sent to non-Azure embedding endpoints; keys are chosen per provider; redirects
  are not followed; the API key is redacted from Debug
- local --embed presets accept only loopback endpoints; the upload destination is printed before any request;
  units that cannot match are not embedded
- embedding cache: a corrupt tail no longer blocks later appends (re-fetch loop), empty or non-numeric vectors
  are rejected, oversized entries cannot desync the file, owner-only permissions, 128-bit keys

Correctness and performance
- the k-gram prefilter no longer loses pairs that share no 4-gram; --embed-code no longer changes the copies
  renormalization; the default scan is faster than before (zero-weight features are not computed)
- plain SQL queries are named like their file; .js/.mjs/.cjs use the TSX grammar; fragments and find keep
  the root in file names; --explain pairs units by full place
- script noise is limited to module level; main() inlining keeps else branches and multi-line strings;
  compound statements report header lines; extraction runs on a large stack

Tooling and docs
- hardened eval scripts, corrected the dev/holdout description of the CodeNet sets, updated the agent skill,
  --help text and crate descriptions

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s cannot be truncated on Windows)

CI caught this: the tail cut used set_len on the append handle, which fails on Windows. The tail is now
cut only if nobody appended since the file was read, so a concurrent writer's records are never lost.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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.

1 participant