[andriytyurnikov]

CodeRabbit's post-merge finding on #139 (src/graph.h, outside the diff): the decl-to-def widening behind every file:name selector retains a definition on FILE evidence alone, so a translation unit that #includes api.h for its own reasons and defines an unrelated helper in an anonymous namespace is served as the header's implementation. Confirmed by reading and by a fixture. One correction to the finding's framing: a same-signature anonymous helper in an including TU makes every unqualified call ambiguous, so the shape that compiles is an overload — helper(double) in the anonymous namespace, or a namespace-scope static helper(long), beside the declared helper(int). Both were gathered by name and both were wrong.

The rule

Internal linkage makes a definition visible to its own translation unit alone, so no other file's declaration can stand for it. Symbol carried no linkage information at all, so the fix needs an extraction bit.

Change

• Ingest. Every C and C++ definition carries a syntactic internalLinkage bit (ingest_names.h::cppInternalLinkage): inside an anonymous namespace { } at any depth, or carrying a namespace-scope static. A class-scope static member has external linkage and is not marked. Rides RawDef and the per-file cache record as one u8 after recovered (kMinDefRecordBytesLean 78 → 79); Symbol takes it as a bit-field sharing testScope's byte, so sizeof( Symbol ) and its static_assert are unchanged.
• Graph. The widening's keep loop (keepProvenCandidates, split out of declToDefFollowThrough so the per-symbol test does not push it over the complexity bar, the same reason H1 split its scan out) keeps an internal-linkage candidate only for a declaration in its own file. A rejected one is COUNTED in unproven_defs= like any other drop: the reader still learns that same-named definitions exist which no row covers, and the bare-name selector still shows them, as the legend already says. This is clause 3 of the H1 note.
• Versions. kParserVer 95 → 96 and kCacheVersion 21 → 22 with quality.h's two mirrors in the same commit; test/qschemetrip.hash re-pinned with its log entry. Old caches are rejected and rebuilt through the existing degrade path.

Gate, red first

test/decltodefcheck.sh arm (B2): a header, its defining .cpp, one real caller, and two including TUs with an anonymous-namespace overload and a static overload. On main's binary:

FAIL  (B2) api.h:helper serves localUse statUse — an anonymous-namespace / static helper in a TU that includes api.h (internal linkage, gathered by name)

With the fix:

PASS  (B2) api.h:helper count="1" names useApi, unproven_defs="2"; controls: api.cpp:helper count="1", bare helper count="3"

Arm (B) already covered TUs that do NOT include the header; (B2) is the gap it left. The existing (E2) residue arms, which expect the two anonymous helpers of arm (B)'s corpus to be disclosed as dropped, are what decided that the linkage test lives in the proof loop rather than the gather: 226 of 226 arms pass.

Measurements

Corpus	Before	After
fixture, --callers=api.h:helper	count="3" (api.cpp:helper said count="1")	count="1" naming useApi, unproven_defs="2"
this repo at 1cf3086e (frozen worktree), default map	26,402 B	byte-identical
this repo, the 11 header-qualified --callers selectors over src/ingest.h's declarations	—	0 of 11 moved

The tree has no colliding internal-linkage overload, so the own-tree numbers are a no-movement control, not a demonstration. No larger C++ corpus is on this machine (the local bitcoin checkout is empty); a second number on llvm would be welcome.

Verification

• decltodefcheck 226/226, declinedlistcheck 30/30, cachefuzzcheck 104/104, cachehashcheck 7/7 — each also under ASan/UBSan (-fno-sanitize-recover=all), plus raw ASan runs of the default map, the fixture answer, and a cold+warm cache round trip: zero bytes on stderr.
• qschemetripcheck, qextractionkeycheck, cacheidentitycheck, gateexitcheck, manifestcheck, testscopecheck green.
• Determinism ×2 and xmllint --noout on the fixture and this tree.
• Full suite (test/pargates.py -j 3, 612 gates): running on my machine as this opens; verdict to follow in this thread.

Disclosed

--quality-delta exits 2 with one gating row: short-horizon-churn on declToDefFollowThrough, churn="self". The function was written by #63 (09-08), rewritten by H1 (09-11), and this is its third touch in a week — the finding is about exactly the function the fix has to live in. Not acked; the change shrinks the function (the keep loop moved out) rather than growing it.

Also on #139's review

CodeRabbit's other outside-diff finding there, the declined-candidate list growing with calls × candidates, is closed by #208 (one interned copy per distinct list, a uint32 ceiling behind DEGRADED_PATH_ALERT, verifyOffsetCsr, and test/declinedlistcheck.sh); verified green on main before starting this. Nothing to do here beyond a reply on that thread.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b1cdbc73-3679-4fac-8a27-fbf79d8686f2

📥 Commits

Reviewing files that changed from the base of the PR and between a2f90ba and 65b8244.

📒 Files selected for processing (11)

• CHANGELOG.md
• src/graph.h
• src/ingest_cache.h
• src/ingest_model.h
• src/ingest_names.h
• src/ingest_sidecap.h
• src/model.h
• src/quality.h
• test/decltodefcheck.sh
• test/qschemetrip.hash
• test/qschemetripcheck.sh

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes

  • Improved header-based code navigation and symbol matching by preventing unrelated file-local definitions from being treated as the declared symbol.
  • Corrected results for C and C++ symbols defined in anonymous namespaces or with namespace-scope static linkage.
  • Improved handling of file-local symbol information in analysis results.
  • Added coverage for header-qualified and file-qualified symbol lookups.

• Compatibility

  • Existing analysis caches are automatically rejected and rebuilt to ensure accurate results after this update.

Walkthrough

The change records C/C++ internal linkage on definitions, persists the flag in caches, and prevents header-qualified declaration-to-definition matching from selecting unrelated internal-linkage definitions. Regression tests cover anonymous-namespace and static overloads.

Changes

Internal linkage declaration-to-definition matching

Layer / File(s)	Summary
Capture and persist linkage metadata src/ingest_names.h, src/ingest_sidecap.h, src/model.h, src/ingest_cache.h, src/ingest_model.h, src/quality.h	C and C++ definitions detect anonymous-namespace and namespace-scope static linkage. The flag is stored in symbols and cache records. Cache versions increase from 21 to 22 and from 95 to 96.
Filter declaration-to-definition candidates src/graph.h	Decl-to-def follow-through keeps internal-linkage definitions only when a declaration is in the same file. Other proven candidates are counted in unproven_defs.
Validate matching and update test metadata test/decltodefcheck.sh, test/qschemetrip.hash, test/qschemetripcheck.sh, CHANGELOG.md	Regression fixtures verify header-qualified, defining-file, and bare-name selectors. Test metadata and the changelog record the cache and parser version changes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Low

Sequence Diagram(s)

sequenceDiagram
  participant Query
  participant declToDefFollowThrough
  participant Symbol
  participant Result
  Query->>declToDefFollowThrough: submit declaration-qualified query
  declToDefFollowThrough->>Symbol: read internalLinkage
  declToDefFollowThrough->>Result: retain valid definitions and count discarded ones
  Result-->>Query: callers and unproven_defs

Loading

Suggested reviewers: joyful-ii-v-i

Merge Risk: ⚪ Minimal · up to 65b82

The change prevents declarations from resolving to unrelated internal-linkage definitions while retaining valid same-file matches. No unresolved merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 9 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main graph fix and identifies the parser version update.
Description check	✅ Passed	The description directly explains the internal-linkage fix, cache and parser version changes, regression coverage, and verification results.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 9 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[andriytyurnikov]

Full battery, as promised. python3 test/pargates.py . ./build/ripwire -j 3 on 0986edfd:

gates=626 pass=621 skip=3 fail=2 wall=2854.7s jobs=3 tree_writes=0

Both reds are the runner's, not the change's, and each is green rerun alone on the same head:

• versioncheck — I committed while the suite was running, so the binary's stamp read 1cf3086e8+dirty against a HEAD of 0986edfd. Rebuilt at 0986edfd (no source change): ALL PASS.
• strkerncheck — TIMEOUT at its declared 300 s budget under the three-job load (it compiles and runs the SIMD mirrors under sanitizers). Alone: strkerncheck: PASS in 141 s.

So 626 of 626 on this head. Everything in the description stands: decltodefcheck 226/226 plain and under ASan, determinism ×2, xmllint, the frozen-HEAD default map byte-identical.

[joyful-ii-V-I]

Thank you, @andriytyurnikov — this is exemplary work, and the part I most want to call out is the correction you made to the finding itself. The automated review described a same-signature anonymous helper; you noticed that shape makes every unqualified call ambiguous and wouldn't compile, and rebuilt the repro as an overload — helper(double) in an anonymous namespace and a namespace-scope static helper(long) beside the declared helper(int). That's the version that actually ships in real code, and it's the difference between a gate that proves something and a gate that proves a shape nobody writes.

The red-first discipline is exactly right: arm (B2) lands in its own commit, and I reproduced it — on main's binary it is the only failure of 226 arms, with your exact message, and 227/227 pass on yours. I also like that you let the existing (E2) residue arms decide where the linkage test goes. Putting it in the gather would have silenced the disclosure; putting it in the proof loop keeps a rejected candidate counted in unproven_defs=, so the reader still learns those definitions exist. That reasoning is written down in the code, which is where the next person will need it.

Two things worth noting from review. Your fix reaches further than the PR claims: on the B2 fixture --uses goes from count="3" to count="1", and --impact and --safe-delete both pick up unproven_defs="2" — three surfaces corrected, not one. And --quality-delta=origin/main..HEAD comes back gating="0", so the churn row you disclosed doesn't fire against current main; thank you for flagging it conservatively anyway. ASan/UBSan, determinism and the full suite are green locally on your head.

Two small things before this lands. CHANGELOG.md now conflicts with main — please merge origin/main into your branch (we never rebase contributor branches); that will also give the PR its first real CI run, since a conflicting PR gets no workflow run at all. And in src/ingest_cache.h:48, the new field landed between recovered and its own comment continuation, so recovered's explanation now reads as if it documents internalLinkage — just move the new line below those three lines. Optional, same area: coercing the cached byte with ( r.u8() != 0 ) ? 1 : 0 for both 1-bit fields would keep testScope's old "any non-zero means test" reading on a corrupt blob.

Once CI is green this goes in for 0.6.1, and you'll be credited in the CHANGELOG and the release notes.

[joyful-ii-V-I]

Thanks for this — the fix is the right shape and the gate arm is the part I'd have asked for. A note on why it looks stalled, because the reason is not your code.

No CI has run on this branch, and none will until the conflict is resolved. GitHub creates no workflow run at all for a conflicting pull request — not a failed run, no run — so the checks list is empty rather than red. That is also why nothing here is waiting on a maintainer approval.

The conflict is one file and it is trivial. Your branch is 2 ahead and 32 behind main; intersecting your eleven files against everything main has touched since the merge base leaves only CHANGELOG.md. Both sides insert a brand-new ### … section immediately under ## [Unreleased] — yours is the ### Fixed for the internal-linkage widening, and main has since gained an ### Added in the same position. Keeping both sections resolves it. Every substantive file you changed is untouched by main in that range, and the parser and cache numbers you claim (96 and 22) are still the next free ones on main today, so nothing about the fix needs revisiting.

A plain git merge origin/main is what we want here — please don't rebase. Once that lands, CI will be created automatically and I'll take it from there.

Two things I confirmed while looking, in case they save you a round: the Symbol bit-field packing is the one thing I'd want an actual build to sign off, since internalLinkage shares testScope's byte and that struct's static_assert is load-bearing. And the disclosure choice you made — counting a rejected candidate into unproven_defs= instead of dropping it — is exactly the house rule, so that part needs no defence.

[joyful-ii-V-I]

Thanks for the merge — that was exactly what was needed. CI is running now (it did not need a maintainer approval this time), so those commits are getting their first real run across all six legs.

The review I owe you

I read this properly earlier and it holds up. The things I checked specifically:

The fix is shaped the way the finding deserves. Symbol::internalLinkage as a : 1 bit-field sharing testScope's byte, with testScope narrowed to : 1 in the same edit since it was already used as 0/1 throughout. Extraction is a syntactic walk that excludes class-scope static (correctly — that is external linkage) and walks past the nearest scope owner deliberately, so a named class inside an anonymous namespace still counts as internal. internalLinkageBit() returning 0 for every non-C/C++ grammar is the right failure direction: unmarked keeps today's behaviour.

And the disclosure is the part I liked most. A rejected candidate is counted into unproven_defs= rather than silently dropped. That is this codebase's central rule and you did not have to be told.

The gate arm is not vacuous, which is the thing I check hardest. It asserts controls first — the bare selector unioning all three translation units, then the .cpp selector naming the one real caller — then the fix, then the disclosure count. Both overload shapes are exercised, and both including translation units #include the header, which is precisely the gap the previous arm left open.

Pins moved in lockstep in one commit, which is the house rule: parser 95→96, cache 21→22, both mirrors, the lean-record byte minimum, and the trip hash re-pinned with a dated log entry. I verified against main that 96 and 22 are still the next free numbers.

The one thing I wanted an actual build to confirm is the Symbol bit-field packing against the existing static_assert — and the run in flight is what proves it. If it goes green, that question is answered.

On timing, and I want to be straight with you rather than vague

This lands after the 0.6.1 tag, not in it. Two specific reasons, neither about the quality of the work:

1. It bumps the parser version, which invalidates every cached index. That is a real behavioural change and it deserves its own clean cycle rather than being slotted into a release with three other branches still landing.
2. It changes Symbol's layout. This repository's CLAUDE.md devotes several paragraphs to sizeof( Symbol ) drift being the single worst diagnosis trap here — a sanitizer reporting a real overflow of a fake bug. There is even a pull request open right now whose whole purpose is detecting that drift. A layout change to that struct gets a careful landing, not a scramble.

Your merge was not wasted work: it got you out of the conflicted state, it got you CI, and it means you are ready to land first rather than last. You will be credited in the changelog when it does.

If CI comes back red on something real, I will bring you the actual log text rather than a guess — a shard failed on an unrelated branch earlier today and the log named a known flake on a leg the documentation does not mention, so I would rather read it than infer it.

[andriytyurnikov]

All three taken, on the branch as two commits — no rebase.

2c81fe15 — the two ingest_cache.h items. internalLinkage now sits below the two lines that continue recovered's explanation. For the coercion I put it one step later than suggested, at the RawDef → Symbol assignment in ingest_model.h rather than in readDef: that assignment is the point where the u8 is squeezed into the 1-bit field, so it is where a cached 2 would become 0, and it covers a hand-edited record and a fresh extraction alike. Both fields read ( byte != 0 ) ? 1 : 0 there, with the reason in the comment.

65b8244b — merge(main) at a2f90ba6. Main had moved to 82 ahead by the time I merged; still only CHANGELOG.md conflicted, and both sections are kept — main's under [Unreleased] in the order it had them, the internal-linkage section after them (line 649, still inside [Unreleased], which ends at 846). Every other file the branch touches is untouched by main in the range; quality.h and ingest_cache.h are unchanged there, so 96 and 22 are still the next free numbers and test/qschemetrip.hash needed no re-derivation.

On the merged head 65b8244b, clean rebuild of both trees (--clean-first, the branch-switch rule): decltodefcheck 226/226, cachefuzzcheck 104/104, cachehashcheck, cacheidentitycheck, qschemetripcheck, qextractionkeycheck, gatecountcheck, versioncheck, showcasecapturecheck, gateexitcheck, manifestcheck all green; --quality-delta gating="0"; determinism ×2 and xmllint on this tree. ASan/UBSan (-fno-sanitize-recover=all) on the merged head: the default map, a cold+warm cache round trip, decltodefcheck and cachefuzzcheck — zero bytes on stderr, so the bit-field packing has a sanitized build behind it as well as the static_assert; CI's plain and Release flavours are the independent sign-off.

Thank you for the --uses / --impact / --safe-delete measurement — three surfaces, not one; I had only counted the one I fixtured.

[joyful-ii-V-I]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[joyful-ii-V-I]

Merged — thank you. This closes the last of the decl→def family: #173 stopped a header selector answering with bodies it could not tie to that header, #190 made the focus verbs say what they dropped, and this one stops an internal-linkage definition standing for another file's declaration.

That was a wrong answer on every file:name selector, not a miss, which is the worst kind of bug this tool can have — and it needed a parser-version bump to fix properly, which you did rather than working around. It ships in 0.6.1, with credit in the CHANGELOG and release notes.