fix(graph2md): register disambiguated slugs and correct line_count for Function/Class/Type#95
Merged
greynewell merged 1 commit intomainfrom Apr 9, 2026
Merged
Conversation
…r Function/Class/Type Slug collision: when a base slug (e.g. "fn-handler-go-run") was already taken and a "-2" suffix was appended, the new slug was never recorded in usedSlugs. A later node naturally producing "fn-handler-go-run-2" would pass the uniqueness check and get the same slug, causing one output file to silently overwrite the other. Fixed by marking the resolved slug used. line_count: Function, Class, and Type frontmatter computed endLine - startLine + 1 without guarding against startLine == 0 (the sentinel for "not provided"). This produced line_count = endLine + 1 instead of endLine. File nodes already defaulted startLine to 1; the three symbol node types now do the same. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThis change adds collision-resistant slug generation and fixes line-count calculations for nodes with missing start-line values. Two new tests verify both the slug disambiguation strategy and the line-count defaulting behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/archdocs/graph2md/graph2md_test.go`:
- Around line 124-127: The test currently discards the error returned by
os.ReadDir when assigning entries; change the call in graph2md_test.go to
capture the error (e.g., entries, err := os.ReadDir(outDir)) and immediately
fail the test if err != nil (e.g., if err != nil { t.Fatalf("ReadDir(%q) failed:
%v", outDir, err) }) before asserting len(entries); update the t.Fatalf
assertion to run only after the error check so filesystem/setup errors are
surfaced instead of hidden.
In `@internal/archdocs/graph2md/graph2md.go`:
- Around line 373-377: The current logic that disambiguates duplicate slugs by
doing slug = fmt.Sprintf("%s-%d", slug, n+1) and immediately registering that
new slug in usedSlugs can still collide if a naturally-created slug already
equals that candidate; update the code in the block that manipulates
usedSlugs/slug (the disambiguation logic using fmt.Sprintf and the usedSlugs
map) to loop/increment until you find a truly unused candidate (e.g., compute
candidate := fmt.Sprintf("%s-%d", base, i) and check usedSlugs[candidate] in a
for loop), then set usedSlugs[base] = i and usedSlugs[candidate] = 1 for the
final unique slug; ensure you reference and update both the base count and the
final slug key rather than assuming a single step is sufficient.
🪄 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: c3360408-e6bc-48eb-90df-9fdda497876e
📒 Files selected for processing (2)
internal/archdocs/graph2md/graph2md.gointernal/archdocs/graph2md/graph2md_test.go
Comment on lines
+124
to
+127
| entries, _ := os.ReadDir(outDir) | ||
| if len(entries) != 1 { | ||
| t.Fatalf("expected 1 output file, got %d", len(entries)) | ||
| } |
There was a problem hiding this comment.
Don’t discard ReadDir errors in the test.
At Line 124, ignoring the error can hide filesystem/setup failures and make assertions noisy.
Suggested fix
- entries, _ := os.ReadDir(outDir)
+ entries, err := os.ReadDir(outDir)
+ if err != nil {
+ t.Fatalf("ReadDir: %v", err)
+ }
if len(entries) != 1 {
t.Fatalf("expected 1 output file, got %d", len(entries))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entries, _ := os.ReadDir(outDir) | |
| if len(entries) != 1 { | |
| t.Fatalf("expected 1 output file, got %d", len(entries)) | |
| } | |
| entries, err := os.ReadDir(outDir) | |
| if err != nil { | |
| t.Fatalf("ReadDir: %v", err) | |
| } | |
| if len(entries) != 1 { | |
| t.Fatalf("expected 1 output file, got %d", len(entries)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/archdocs/graph2md/graph2md_test.go` around lines 124 - 127, The test
currently discards the error returned by os.ReadDir when assigning entries;
change the call in graph2md_test.go to capture the error (e.g., entries, err :=
os.ReadDir(outDir)) and immediately fail the test if err != nil (e.g., if err !=
nil { t.Fatalf("ReadDir(%q) failed: %v", outDir, err) }) before asserting
len(entries); update the t.Fatalf assertion to run only after the error check so
filesystem/setup errors are surfaced instead of hidden.
Comment on lines
373
to
377
| if n, ok := usedSlugs[slug]; ok { | ||
| usedSlugs[slug] = n + 1 | ||
| slug = fmt.Sprintf("%s-%d", slug, n+1) | ||
| usedSlugs[slug] = 1 // register the disambiguated slug so a later node can't claim it | ||
| } else { |
There was a problem hiding this comment.
Handle secondary collisions when the -N candidate is already taken.
At Line 375, the code picks base-(n+1) directly. If that slug already exists (from a naturally-generated slug seen earlier), this still reuses the same filename and can overwrite output.
Suggested fix
- slug := generateSlug(node, primaryLabel)
+ baseSlug := generateSlug(node, primaryLabel)
+ slug := baseSlug
if slug == "" {
continue
}
// Handle slug collisions
- if n, ok := usedSlugs[slug]; ok {
- usedSlugs[slug] = n + 1
- slug = fmt.Sprintf("%s-%d", slug, n+1)
- usedSlugs[slug] = 1 // register the disambiguated slug so a later node can't claim it
+ if n, ok := usedSlugs[baseSlug]; ok {
+ i := n + 1
+ for {
+ candidate := fmt.Sprintf("%s-%d", baseSlug, i)
+ if _, exists := usedSlugs[candidate]; !exists {
+ slug = candidate
+ usedSlugs[baseSlug] = i
+ break
+ }
+ i++
+ }
} else {
- usedSlugs[slug] = 1
+ usedSlugs[baseSlug] = 1
}
+ usedSlugs[slug] = 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/archdocs/graph2md/graph2md.go` around lines 373 - 377, The current
logic that disambiguates duplicate slugs by doing slug = fmt.Sprintf("%s-%d",
slug, n+1) and immediately registering that new slug in usedSlugs can still
collide if a naturally-created slug already equals that candidate; update the
code in the block that manipulates usedSlugs/slug (the disambiguation logic
using fmt.Sprintf and the usedSlugs map) to loop/increment until you find a
truly unused candidate (e.g., compute candidate := fmt.Sprintf("%s-%d", base, i)
and check usedSlugs[candidate] in a for loop), then set usedSlugs[base] = i and
usedSlugs[candidate] = 1 for the final unique slug; ensure you reference and
update both the base count and the final slug key rather than assuming a single
step is sufficient.
This was referenced Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slug collision: when a base slug such as
fn-handler-go-runwas already taken and a-2suffix was appended to resolve the collision, the new slug was never recorded inusedSlugs. A later node that naturally generatedfn-handler-go-run-2would pass the uniqueness check, receive the same slug, and silently overwrite the first collision-resolved page on disk. The fix is a one-liner: register the disambiguated slug inusedSlugsimmediately after computing it.line_countoff-by-N for Function/Class/Type nodes:line_countwas computed asendLine - startLine + 1without guarding againststartLine == 0(the sentinel returned bygetNumwhen the property is absent). WithstartLine == 0andendLine == 50the output wasline_count: 51instead of50. File nodes already defaultedstartLineto1; the three symbol types now do the same via a localeffectiveStartvariable.Test plan
TestSlugCollisionResolution: three nodes where two naturally produce the same base slug and a third naturally produces the collision-resolved slug — verifies all three output files are distinct.TestLineCountMissingStartLine: function node withendLine=50and nostartLine— verifiesline_count: 50(not51) in the generated frontmatter.go test ./...— all existing tests pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests