Skip to content

fix: a C# verbatim identifier is not a duplicate-qn marker (#2017) - #2113

Merged
vitali87 merged 4 commits into
mainfrom
fix/2017-verbatim-identifier-marker
Sep 22, 2026
Merged

vitali87 merged 4 commits into
mainfrom
fix/2017-verbatim-identifier-marker

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Closes #2017.

The defect

function_registry disambiguates definitions that share a qualified name by appending @<line>, and _<col> after that when a same-named twin already holds that line. C# spells a keyword-colliding name with a leading @ — @event, @lock, @class — so it is the same character in both roles.

Every reader that recovered the written name with qn.split(cs.DUP_QN_MARKER, 1)[0] cut at the first @ without checking what followed:

input before after
Box@12 Box Box
Box@12_5 Box Box
@event '' @event
pkg.@event pkg. pkg.@event
@event@12 '' @event

Scope

The issue names _natural_qn; the grep found eight production readers sharing the spelling:

graph_updater._natural_qn, dead_code, call_processor, call_resolver (two sites), class_ingest/method_override, class_ingest/mixin, function_ingest.

Plus two in the eval oracle (evals/cgr_graph.py), which grades by "the written name" — precisely what the bug destroys, so a C# fixture with a verbatim-named base class would mis-grade. One of those was spelled across three lines, so a line-anchored grep would not have found it.

Meanwhile three sites already had the correct form — duplicates._DUP_QN_MARKER_RE, trace/resolution._natural_qualified_name, and the mixin partial key that #2014 fixed. The codebase held both answers at once, which is what this consolidates.

method_override.py is worth singling out: its comment already named this exact failure — "would empty out a C# verbatim identifier like @event" — directly above the line that committed it. The hazard was documented and not avoided, so grepping for the concept found the site and it read as handled.

The fix

codebase_rag/utils/qn_markers.py holds the single definition. Two properties matter:

  • The grammar is built from the producer's own constants (DUP_QN_MARKER, DUP_QN_COLUMN_MARKER), so a change to either cannot silently stop the stripping.
  • Only a trailing numeric run is a marker. That is what lets a duplicate verbatim type (@event@12) keep its name and lose its marker. A last-@ reader without the digit check gets this one wrong too.

@1 still strips to '', and that is correct: no language here permits an identifier starting with a digit, so it cannot be a verbatim name.

A behavioural change beyond the split

call_processor and call_resolver used if cs.DUP_QN_MARKER not in last: return ... as an early exit before splitting. That guard is true for @event — it carries the character without a marker — so those sites would have gone on to add a second candidate identical to the first. They now compare against the stripped form and skip the redundant probe.

Evidence

Tests were written against the unfixed readers first and confirmed red:

FAILED ...::test_a_verbatim_leaf_survives            assert '' == '@event'
FAILED ...::test_a_bare_verbatim_name_survives
FAILED ...::test_a_duplicate_verbatim_type_keeps_its_name_and_loses_its_marker
FAILED ...::test_the_reader_delegates_to_the_shared_helper
4 failed, 11 passed

The failing set matched what I predicted before running, which is the check that catches a test failing for an unrelated reason. After the fix: 15/15.

Regression: 642 passed, 3 skipped across the marker-adjacent areas (dead code, overrides, duplicates, partials, call resolution). The 2 errors are the documented Docker/Memgraph baseline on a machine without Docker, identical on main.

Every test drives a shipped reader rather than the helper. A helper that is correct in isolation says nothing about a caller that never invokes it, which is the failure mode that let this bug survive #2014.

Reader coverage

greptile-local returned 4/5 with one P2: only one of the routed readers was tested. Fixed — three are now driven behaviourally, chosen because they are reachable without graph fixtures:

  • graph_updater._natural_qn
  • call_processor._scope_qn_candidates — pkg.@event now yields ['pkg.@event'], where before it yielded ['pkg.@event', 'pkg.'] and the second candidate was the corrupted name
  • dead_code._is_root — the issue's provenance case, a second Go init@51 staying a root

The remaining five are methods on parser classes whose fixtures would cost more than the assertion is worth. Those get a parametrised structural backstop asserting each module reaches the shared grammar and no longer carries the broken spelling. It is deliberately the weakest thing in the file and the docstring says so.

Mutation-verified rather than assumed. Reverting call_processor to the pre-fix form reddens 4 tests (2 behavioural, 2 structural); reverting dead_code reddens 2 (structural only — honest evidence that its coverage rests on the backstop). Both restores were verified identical to HEAD afterwards.

38 tests, up from 15.

@vitali87

Copy link
Copy Markdown
Owner Author

@greptileai review

@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 1 second.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 09bfb4a3-9047-4e08-9679-b74728273ca8

📥 Commits

Reviewing files that changed from the base of the PR and between e47f911 and a0de8bd.

📒 Files selected for processing (10)
  • codebase_rag/dead_code.py
  • codebase_rag/graph_updater.py
  • codebase_rag/parsers/call_processor.py
  • codebase_rag/parsers/call_resolver.py
  • codebase_rag/parsers/class_ingest/method_override.py
  • codebase_rag/parsers/class_ingest/mixin.py
  • codebase_rag/parsers/function_ingest.py
  • codebase_rag/tests/test_verbatim_identifier_markers.py
  • codebase_rag/utils/qn_markers.py
  • evals/cgr_graph.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@vitali87

Copy link
Copy Markdown
Owner Author

Scope note, so the boundary is explicit rather than an oversight.

Three sites already had a CORRECT marker-stripping implementation before this PR and are left untouched: duplicates.py:251 (unanchored regex), trace/resolution.py:83 and class_ingest/mixin.py:1303 (both rpartition + a digit test). The local review verified all three agree with the new helper on @event, @event@12, @event@12_5 and pkg.M@12_5, so nothing here is broken.

They are still three more spellings of one grammar, which is the property #2017 is about — so I have filed #2114 rather than widening this PR into working code. Each needs a different entry point (duplicates strips from anywhere in a dotted qn, the other two are end-anchored), so it is not a find-and-replace and does not belong in a fix PR.

What this PR does change is every reader that was WRONG: eight in production plus two in evals/cgr_graph.py. After it, git grep "split(cs.DUP_QN_MARKER, 1)[0]" returns nothing outside tests.

@vitali87

Copy link
Copy Markdown
Owner Author

@coderabbitai review

CodeRabbit is rate limited on this head and has not reviewed it. gh pr checks 2113 reports CodeRabbit: pass, while the artifact (updated 20:58:32Z, same minute the PR opened) carries:

Review limit reached — Next included review available in 5 minutes.

Third time today across #2106, #2110 and this PR, so recording the shape rather than just re-triggering: the check bucket reads pass in every case, and on #2106 the check description ALSO read "Review completed". No field of gh pr checks separates "reviewed" from "declined to review" for this bot — only the comment body does.

The test I am using per bot, for anyone auditing behind me:

  • the head SHA appears in the artifact body in a review context, not only in boilerplate, AND
  • no rate limited / Action not completed / couldn't run / timeout phrase in it.

Greptile and Copilot are still outstanding at ea2ccf9e; I have requested both.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

@vitali87 I will request a new review for #2113. The check status alone does not establish that a review completed. The review artifact must contain head-SHA-specific review content and no failure or rate-limit message.

🐇


✏️ Learnings added
Learnt from: vitali87
URL: https://github.com/vitali87/code-graph-rag/pull/2113

Timestamp: 2026-09-21T21:01:18.136Z
Learning: For CodeRabbit reviews on this repository, a passing GitHub check or a "Review completed" check description does not prove that CodeRabbit reviewed the pull request. Treat the review artifact body as the source of truth: it must contain head-SHA-specific review content and must not contain phrases such as "rate limited", "Action not completed", "couldn't run", or "timeout".

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

Safe to merge; no blocking issues remain.

Summary

This update completes duplicate-qualified-name marker handling by extracting duplicate line suffixes from the final marker, preserving leading @ characters in C# verbatim identifiers. Regression coverage includes marked and unmarked verbatim names and structural checks against first-marker splitting.

Reviews (2) · Last reviewed commit: "fix: the marker suffix reader takes the ..."

@vitali87

Copy link
Copy Markdown
Owner Author

Pushed a0de8bda: an eleventh reader of this defect, found by strengthening the structural test rather than by grep.

The peer session on #1912 hit a test that passed for the wrong mechanism, which prompted me to re-examine my own. I found my structural check was keyed to one literal spelling — split(cs.DUP_QN_MARKER, 1)[0] as a substring — so it could not see either a bare "@" literal or, as it turned out, a site taking index [1].

Replaced it with an AST matcher over any .split(<marker>, 1) where the argument is DUP_QN_MARKER or a bare "@" constant, validated with a known-positive probe (hits both spellings, ignores a "." split). It immediately failed on a site my original grep had missed:

call_processor._go_variant_spans, which reads the marker's SUFFIX to recover a line number:

suffix = marker.split(cs.DUP_QN_MARKER, 1)[1]   # [1], not [0]
line_text = suffix.split(cs.DUP_QN_COLUMN_MARKER, 1)[0]

For @event@12 that yields line_text = "event@12", which fails isdigit() and returns None — dropping the span correspondence for the whole variant set. It failed safe (lost precision, no corruption), which is why it outlived the others and why I am calling it a lesser defect than the original eight.

Fixed with rpartition (last marker) and a stripped-form test for the no-marker branch. Verified: @event@12 -> line 12, @event -> no-marker branch, Box@12_5 -> line 12 unchanged.

Mutation-verified: reverting it reddens 3 tests (2 behavioural + the AST check). The marked-non-verbatim test correctly stays green — that path works either way, so it is a no-regression pin, not a defect test.

41 tests now. Re-triggering the bots at this head since the push invalidated the 5/5 anchored to ea2ccf9e.

@greptileai review

@vitali87

Copy link
Copy Markdown
Owner Author

Gate status at a0de8bda, and an open question about Copilot I am not going to keep retrying.

Satisfied: Greptile 5/5 anchored to a0de8bda, no red-flag phrases, summary naming this diff specifically ("extracting duplicate line suffixes from the final marker, preserving leading @ characters in C# verbatim identifiers"). Zero unresolved threads. Zero failing checks.

Outstanding: Copilot has produced no review on this PR at all, and no review_requested event of ANY kind appears on the timeline, despite both documented forms returning success:

gh pr edit 2113 --add-reviewer @copilot                      -> exit 0
gh api .../pulls/2113/requested_reviewers -X POST \
  -f reviewers[]=copilot-pull-request-reviewer[bot]          -> HTTP 200

Timeline review_requested count after both: 0.

Two things keep this from being a conclusion:

  1. The CLAUDE.md records that the timeline event "can be absent, coalesced or delayed on a PR that is genuinely queued, so its absence proves nothing either" — measured across 88 PRs, where four PRs with no event had in fact been requested and were reviewed minutes later.
  2. The copilot-code-review.yml runner workflow has run successfully on my other two branches today (21:03, both completed/success) but has no run for this branch, which is consistent with the request genuinely not registering — or with it simply not having been scheduled yet.

So: request status unknown, review absent. I am not retrying a third time — the CLAUDE.md is explicit that impatience here produced three retries and a false failure report on a previous occasion. The only signal that settles it is a review whose commit_id equals the head, and I will report when one appears rather than inferring from the request side.

Not blocking on it either: I will say plainly that Copilot has not reviewed this PR rather than treat its silence as approval.

@vitali87

Copy link
Copy Markdown
Owner Author

Pre-checked the two SonarCloud rules that failed on my other PRs today, so the queue wait is not also a surprise wait.

python:S3776 (cognitive complexity, limit 15). This bit #2106 earlier — my own fix there added a nesting level to a function already near the limit. Compared every changed file against its main baseline rather than counting in isolation, since Sonar gates on NEW issues:

dead_code.py         main=1 head=1
graph_updater.py     main=6 head=6
call_processor.py    main=8 head=8
call_resolver.py     main=1 head=1
mixin.py             main=2 head=2
function_ingest.py   main=1 head=1
evals/cgr_graph.py   main=2 head=2

Identical on every file. That is the expected result for this change — it replaces inline logic with helper calls, which can only reduce branching in the caller — but the absolute counts look alarming on their own, and the baseline comparison is what makes them readable. qn_markers.py and the test file are 0.

Caveat stated rather than glossed: ruff's C901 is CYCLOMATIC complexity while S3776 is COGNITIVE. Related metrics, not the same one, so this is corroboration and Sonar remains the authority.

python:S9073 (composite assertions). This bit #2110. AST sweep over every changed Python file: 0, with a known-positive probe returning 1 on the same run so a zero cannot mean "the matcher is dead".

Neither is a substitute for the gate. Both are cheap, and the failure mode they prevent — discovering a Sonar failure after a long queue wait, fixing it, moving the head, and discarding the review anchors — costs a full cycle each time.

Current state: Greptile 5/5 genuine at a0de8bda, zero unresolved threads, zero failures, required CI queued behind ~36 runs and not yet started. check_pr_gated.py reports NOT gated for that reason and specifically warns against an empty commit here.

@vitali87

Copy link
Copy Markdown
Owner Author

score went red here on the same cause as Analyze (python) on #2110: the repo installation's API rate limit, not this change.

##[error]API rate limit exceeded for installation.
  request ID A400:3C8DED:5C3A64:12ED94A:6AB1B222   2026-09-21 22:39:30 UTC

Sharper than the CodeQL case: this one died in setup-uv, the first step, so every step after it was skipped —

Set up Python      outcome=skipped
Install pr-split   outcome=skipped
Score PR           outcome=skipped
Post comment       outcome=skipped

The pr-split scoring never executed. Nothing about the diff was measured, so the red says nothing about it. Note score had already PASSED on this same head earlier — a check flipping from pass to fail without the head moving is itself the signal that the cause is environmental.

Two failures now, on two PRs, from the same exhausted installation quota, hitting unrelated jobs at unrelated stages. Around 35 workflow runs have been queued concurrently all evening, which is the obvious source.

Not acting on either. gh run rerun is refused for pull_request-triggered runs, and a new head would discard the Greptile 5/5 anchored here and send the queued required jobs back to the end of the queue — an expensive trade for a transient quota error that will clear on its own at the hourly reset.

I have also stopped polling the API from this session while the quota recovers: more calls against a saturated installation make the condition worse for every session working this repo, not better.

Standing position, unchanged on the merits: Greptile 5/5 genuine at a0de8bda, zero unresolved threads, both SonarCloud rules pre-cleared locally, required CI queued. The only red marks are rate-limit artifacts.

@sonarqubecloud

Copy link
Copy Markdown

@vitali87
vitali87 merged commit 57e077d into main Sep 22, 2026
26 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

C#: every split(DUP_QN_MARKER, 1)[0] reader mistakes a verbatim identifier (@event) for a duplicate marker

1 participant