fix(go): report a declaration group's doc comment as the group's, and a struct field's at all

[vlsi]

Why

Go attaches doc comments in two places the mapper had no model for, and both failures land on an agent reading sb map output.

const ( … ), var ( … ) and type ( … ) take a comment of their own, which go doc renders as the documentation of the whole block. The adapter handed that comment to whichever member came first, or dropped it when that member had a comment already — and nothing marked the result as inherited. Asked to review the doc comment of MaxBytes, an agent was handed a sentence written about the group. (#46)

Struct fields and interface methods had the opposite failure: their comments were never read at all, so docs: null was indistinguishable from a genuinely undocumented field. For a Kubernetes operator that is the whole API surface — controller-gen builds every CRD description from those comments — so a package of documented types mapped as undocumented. (#47)

What

• A block's comment travels as group on every member of the block and as no member's docs. DeclarationGroup is {docs, start_line, end_line}; the range is the block's own, const ( through ), with the comment ending on the line above it — the anchor issue Go: a grouped declaration's doc comment is dropped, or silently attributed to its first member #46 asked for, since the comment above const ( matches no declaration's line.
• map prints the comment once per block, marked [group] on every line. Printed bare above the first member it reads as that member's own, which is the bug. Two adjacent blocks are told apart by the block's range, not by its prose: // Deprecated: … and generator banners repeat verbatim.
• A member's own comment is no longer displaced — // Circle is round. inside a type ( … ) block used to lose to the group's.
• Struct fields and interface methods report docs and doc_start_byte the way types and functions already do.
• A comment sharing a line with the declaration before it is that declaration's trailing comment. Without this, every field below Alpha int // in bytes would have inherited it, which would have traded one misattribution for a denser one.
• --no-docs sheds the group in both JSON projection layers; --no-lines keeps its range, which is the only thing a block can be identified by.
• The 92 one-line group: None, additions across the other adapters are the new IR field reaching every Declaration literal.

I took the issue's second suggested fix rather than the first. Emitting the group as a declaration whose children are its members matches go doc more literally, but it changes the tree shape for every Go consumer: digest's flatteners would hide the members, the header counts would shift, and _walk_top resolves receiver types by their index among the package's direct children, so types declared inside a type ( … ) block would stop collecting their methods. group gets the same information to consumers without that blast radius.

One thing the issues suggest that this deliberately does not do: a parenthesised block holding a single spec is still a block. Measured with go1.26.4, go doc pkg.Solo on a one-member block and go doc pkg.Red on a two-member one both print the whole block followed by the block's comment — go doc draws no distinction by arity, so neither does this.

How to verify

cargo test --test go_adapter

tests/go_adapter.rs covers each case against tests/fixtures/go_adapter/docs.go: the group comment reaching no member's docs, a member keeping its own, struct-field and interface-method docs, undocumented members staying undocumented, trailing comments, two adjacent blocks sharing a comment, block comments, and both projections. Full suite green.

Fixes #46
Fixes #47

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 78661d4b-5c04-4232-a80b-e53dfa7688ad

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[aeroxy]

Why

Go attaches doc comments in two places the mapper had no model for, and both failures land on an agent reading sb map output.

const ( … ), var ( … ) and type ( … ) take a comment of their own, which go doc renders as the documentation of the whole block. The adapter handed that comment to whichever member came first, or dropped it when that member had a comment already — and nothing marked the result as inherited. Asked to review the doc comment of MaxBytes, an agent was handed a sentence written about the group. (#46)

Struct fields and interface methods had the opposite failure: their comments were never read at all, so docs: null was indistinguishable from a genuinely undocumented field. For a Kubernetes operator that is the whole API surface — controller-gen builds every CRD description from those comments — so a package of documented types mapped as undocumented. (#47)

What

• A block's comment travels as group on every member of the block and as no member's docs. DeclarationGroup is {docs, start_line, end_line}; the range is the block's own, const ( through ), with the comment ending on the line above it — the anchor issue Go: a grouped declaration's doc comment is dropped, or silently attributed to its first member #46 asked for, since the comment above const ( matches no declaration's line.
• map prints the comment once per block, marked [group] on every line. Printed bare above the first member it reads as that member's own, which is the bug. Two adjacent blocks are told apart by the block's range, not by its prose: // Deprecated: … and generator banners repeat verbatim.
• A member's own comment is no longer displaced — // Circle is round. inside a type ( … ) block used to lose to the group's.
• Struct fields and interface methods report docs and doc_start_byte the way types and functions already do.
• A comment sharing a line with the declaration before it is that declaration's trailing comment. Without this, every field below Alpha int // in bytes would have inherited it, which would have traded one misattribution for a denser one.
• --no-docs sheds the group in both JSON projection layers; --no-lines keeps its range, which is the only thing a block can be identified by.
• The 92 one-line group: None, additions across the other adapters are the new IR field reaching every Declaration literal.

I took the issue's second suggested fix rather than the first. Emitting the group as a declaration whose children are its members matches go doc more literally, but it changes the tree shape for every Go consumer: digest's flatteners would hide the members, the header counts would shift, and _walk_top resolves receiver types by their index among the package's direct children, so types declared inside a type ( … ) block would stop collecting their methods. group gets the same information to consumers without that blast radius.

One thing the issues suggest that this deliberately does not do: a parenthesised block holding a single spec is still a block. Measured with go1.26.4, go doc pkg.Solo on a one-member block and go doc pkg.Red on a two-member one both print the whole block followed by the block's comment — go doc draws no distinction by arity, so neither does this.

How to verify

cargo test --test go_adapter

tests/go_adapter.rs covers each case against tests/fixtures/go_adapter/docs.go: the group comment reaching no member's docs, a member keeping its own, struct-field and interface-method docs, undocumented members staying undocumented, trailing comments, two adjacent blocks sharing a comment, block comments, and both projections. Full suite green.

Fixes #46 Fixes #47

🤖 Generated with Claude Code

Xceptional!

🛠️ Minor Suggestions & Nits

These are mostly stylistic or minor optimizations.

1. Simplify Iterator Chaining in _deprecated
In src/core.rs, the current implementation captures an iterator and clones it inside the closure:

let group_docs = d.group.iter().flat_map(|g| &g.docs);
let doc_has = |needle: &str| {
    d.docs.iter().chain(group_docs.clone()).any(|x| x.contains(needle))
};

You can make this cleaner and slightly more efficient by chaining directly. This avoids the need to clone the iterator state and is more idiomatic Rust:

let doc_has = |needle: &str| {
    d.docs
        .iter()
        .chain(d.group.iter().flat_map(|g| &g.docs))
        .any(|x| x.contains(needle))
};

2. Idiomatic Option Mapping in _spec_docs
In src/adapters/go.rs, you have:

let doc_start = comments
    .first()
    .map(|c| c.range().start)
    .unwrap_or(node.range().start);

This is perfectly fine, but map_or is a bit more concise:

let doc_start = comments.first().map_or(node.range().start, |c| c.range().start);

3. Hardcoded Tree-Sitter Grammar Strings
In _group_context, you use:

matches!(c.kind().as_ref(), "(" | "const_spec_list" | "var_spec_list" | "type_spec_list")

While this is correct for the current tree-sitter-go grammar, these node names are subject to change if the grammar is updated. Consider extracting these strings into constants at the top of the file (e.g., const GROUP_INDICATORS: &[&str] = &["(", "const_spec_list", ...];) and adding a brief comment noting their origin. This makes future grammar maintenance slightly easier.

4. CRLF Handling in Rendering
In _push_group_docs, your CRLF handling is robust:

for line in d.trim_end_matches(&['\r', '\n'][..]).split('\n') {
    out.push(format!("{}{} {}", prefix, "[group]".dimmed(), line.trim_end_matches('\r')));
}

This correctly handles both \n and \r\n line endings. No changes needed, just a tip of the hat for handling Windows line endings properly in source code.

[vlsi]

All three addressed in 0d1e9e4:

1. _deprecated chains d.group.iter().flat_map(…) inline — the captured binding was only ever cloned, never advanced, so this is a straight simplification.
2. map_or in _spec_docs.
3. GROUP_KINDS extracted. One addition while I was there: var_spec_list is its own VAR_SPEC_LIST constant, because that name has a second reader 600 lines away in the traversal that descends into a var block. Spelled twice, a grammar rename leaves detection working via the ( child while the traversal stops descending — every member of every parenthesised var block would vanish from map output with no error.

Nit 4 needs no change, as you said.

Want these squashed into the one commit before merge, or is a follow-up commit fine?

[aeroxy]

@vlsi Ran a deeper pass over the branch and turned up three things, one of which lands directly on the constant from nit 3.

surface --json loses first-member docs. src/surface/fallback.rs copies decl.docs into SurfaceEntry, which has no group field. A documented const ( … ) block used to hand its comment to whichever member came first — the bug this PR fixes — but surface is a flat projection with nowhere to put a group as a group, so the members now come back undocumented instead of one-of-them-wrongly-documented. Falling back to group.docs when docs is empty would cover it: on a flat projection, "the block's comment" is the closest true thing to say about every member of the block.

Worth noting deprecated survives this, and got better: _deprecated's doc_has reads d.group.docs alongside d.docs, so // Deprecated: … on a block now marks every member rather than only the first.

Trailing comments are dropped rather than re-attributed. _leading_comments is right to refuse Alpha int // in bytes as Beta's doc, but nothing then hands it to Alpha. map go/token/token.go loses ~90 per-const annotations (// main, // +, …) that main reported on the wrong symbol, and go doc treats them as the symbol's own doc. The PR description says the comment "is that declaration's trailing comment" while the code only excludes it and the new test encodes the empty result as expected — so either the prose is overclaiming or the attribution is missing, and I think it's the latter. My preference is to read it onto the declaration it trails in the same pass, which makes this PR strictly additive on Go docs instead of trading a misattribution for a deletion. Say so if you'd rather it were its own PR — it widens the diff into every spec list, and that's a fair reason to split it.

The GROUP_KINDS hedge is half-applied. const_spec_list and type_spec_list are in the detection list, but only VAR_SPEC_LIST is descended into in the traversal. If a grammar ever did interpose a spec list for const or type, detection would call the block a group while its members went unreachable — zero members, silently, which is precisely the failure VAR_SPEC_LIST exists to prevent. A hedge covering only half the path is worse than no hedge, because the shape reads as handled. Worth dropping the two speculative names and keeping the comment that says ( is the tell for const and type.

Verification behind all of the above, for what it's worth: I diffed map output between main and this branch over the Go 1.25 stdlib (6,230 files). All 19,459 emitted groups anchor on a const (/var (/type ( line with every member inside the block range, zero own-line doc comments lost, and zero comments landing on a declaration that had none above it. The one degenerate case I found — two comment tokens sharing the last line above a declaration, /* a */ // b, which drops the whole doc — occurs zero times in those 6,230 files, so I'm not asking you to chase it.

On your squash question: separate commits are fine, and I'd rather have the review trail while this is still moving. I'll squash at merge.

[vlsi]

Review fixes pushed, and the branch is rebased onto aae4c86.

Commit id mapping:

before	after
05f2a57	b7cfd60	fix(go): report a group's doc comment as the group's, and a field's at all
0d1e9e4	a5d925e	refactor(go): address review nits on the group fix
—	56d41f2	fix(go): keep a block's documentation reachable, and its members together

56d41f2 is new: four cross-review rounds squashed into one commit. The two that matter:

• surface --json reported every member of a documented Go block as undocumented. SurfaceEntry copied only docs, so moving a block's comment into group left it unreachable — this PR's own bug, one surface over. SurfaceEntry now carries group, and all five resolvers copy it beside docs.
• A type ( … ) block's members each push a trailing blank line, which separated every member after the first from the [group] line documenting it, so Square read as an undocumented top-level type. Members of one block now render as one block — and that gap closes only when the marker is printed, since closing it unconditionally told the reader by spacing alone which block carried a comment they had hidden with --no-docs.

Also: the map MCP tool description carries the marker explanation (README and SKILL.md do not reach an MCP client), with a test that fails if the two stop stating the same facts; and GROUP_KINDS lost const_spec_list / type_spec_list, which read as insurance against a grammar change but were not — the traversal reaches members through var_spec_list alone, so such a block would have been classified as a group and then yielded nothing at exit 0. The fixture is what covers that upgrade.

Rebase conflict was one file, tests/mcp_e2e.rs, where c3dbb03's frontier tests and mine were both appended at the end; both kept. Suite green on the new base (27 binaries).

[aeroxy]

@vlsi Thanks for picking these up rather than handing them back — and for the two things you caught that I didn't.

The type ( … ) blank line is the better find of the two. I diffed 6,230 stdlib files and never saw it, because I was comparing doc attribution and that bug lives entirely in spacing: Square was attributed correctly and still read as an undocumented top-level type. Gating the gap-close on the marker actually being printed is the right call too — closing it unconditionally would have leaked, through spacing alone, which block carried the comment --no-docs was asked to hide.

And your read on GROUP_KINDS is stronger than mine. I said the hedge was half-applied. It was worse than that: _type_declaration_to_decls descends no spec list at all, so a grammar that interposed one would have yielded zero members with or without the two names. It bought nothing in either direction, and the fixture failing on such an upgrade is the real coverage.

On surface: copying group unconditionally is better than the docs-empty fallback I proposed. Mine would have erased a block's comment from exactly the members that document themselves, which is a strictly worse answer than carrying both facts. Verified all ten SurfaceEntry construction sites across the five resolvers set it, and surface tests/fixtures/go_adapter --json now carries the block comment on all nine block members.

Rebuilt and re-ran everything on the new base — suite green, and I re-checked the gap-close under default, --no-docs, --detail signatures, digest, --no-lines, --no-private with a private member both first and mid-block, and --max-members 1. The prev.is_none() guard is load-bearing in the last of those: without it two adjacent ungrouped declarations both hash to None and get glued together.

Three nits left, all prose or test rather than behaviour:

SurfaceEntry.group's doc comment describes a conditional the code doesn't implement. It says the group is carried "when its own docs are empty because the documentation belongs to the block it lives in". The resolvers copy it unconditionally, and Circle has both. That comment is the spec for whoever writes the sixth resolver, and read literally it reopens #46 on the surface side. Worth stating the invariant rather than the case that motivated it.

The test that would catch that omits the deciding member. surface_carries_a_blocks_documentation asserts "every block member must be exercised" over a list of 8, but the fixture has 9 members in documented blocks — the missing one is Circle, the only member with both docs and group, and so the only one whose result differs between unconditional copying and the conditional the comment describes. Adding it makes the assertion true and pins the behaviour.

The map tool description promises group "on every member of the block" without the caveat that it sheds with docs. _strip_projected_keys drops it under the docs projection deliberately and says why, so projected.docs: false covers it correctly and the behaviour is fine — it's only that the sentence an MCP client reads is unconditional while the field isn't.

Two more I'm not asking you to change: assert_eq!(checked, 3) is tautological over a 3-element literal, and only the type-group iteration can exercise the fix since const and var members never push a trailing blank; and in the --no-docs half, Undocumented/AlsoNone has group: None, so it would be separated in any mode — Circle/Square is the pair that actually tests the gate.

That leaves the trailing-comment question from my last comment unanswered, which is the only thing between this and merge. map go/token/token.go still drops ~90 per-const annotations that main reported on the wrong symbol, and go doc treats them as the symbol's own. I'd rather land it here so the PR is strictly additive on Go docs instead of trading a misattribution for a deletion — but it reaches into every spec list, and you've now got four rounds in this branch. Happy to take it as a follow-up if you'd rather draw the line here; say which and I'll merge or write it.

[vlsi]

All three nits addressed, plus the two test observations you weren't asking about. Five commits on top of 56d41f2.

SurfaceEntry.group states the invariant now. You were right that it was the spec for the sixth resolver — and read literally it prescribed the docs-empty fallback you'd already talked yourself out of.

The test that would have caught it derives its expected set from the fixture instead of hand-keeping a list, so Circle is covered and a member the fixture gains can't go uncovered.

The --no-docs caveat went to README, SKILL.md, the MCP tool description and wiki/architecture.md — the last one is the only text that already reasoned about projections, so it was the one most obviously missing it.

Both test observations: assert_eq!(checked, 3) is replaced by an assertion that the type-group iteration ran over a member with children, since that is the only shape that pushes the blank line; and the Undocumented/AlsoNone pair is labelled a control rather than reading as a second test of the gate.

Comments were also swept under our rustdoc rubric: seven summaries were carrying a second sentence into the item table, and two references named a position rather than an item.

Worth flagging honestly, since it is most of the diff above: cross-review spent four rounds not on the fix but on the test that guards it. The join key was wrong three times — bare name, then name and line (a struct and its own field share both: Square struct{ Square int }), then a basename filter that let a nested sub/docs.go through, because surface walks the directory recursively while map reads one file. Each time I had reproduced the previous case and believed it closed. It is now keyed by kind, name, line and whole path, with five axes checked by reverting each into the fixture — and the defect it exists for, group only when docs is empty, still fails it.

Two things I found while doing that, neither touched here:

• Multi-name specs lose every name but the first. const ( A, B = 1, 2 ) reports only A; B appears nowhere in map --json. Predates this PR and is orthogonal to grouping — say the word and I'll open an issue.
• src/search/chunker.rs:1558 trips clippy no_effect_replace (.replace("//", "//")). Came in with main, untouched here.

On the trailing-comment question: I'd rather it were its own PR. Not because it's hard — because every defect in this branch for four rounds has hidden in the checks around a change rather than in the change, and reading a trailing comment onto the declaration it trails adds an axis in every spec list. Its own fixture and its own rounds will do better by it than a fifth round here. This PR closes #46 and #47 as scoped; happy to be overruled if you'd rather land it together.

[aeroxy]

@vlsi All three landed, and the two corrections in 3ce0829 are the part I'd single out — catching that your own previous rewrite had the var tree shape backwards, and that _const_var_to_decls never sees a type_declaration, is the kind of thing nobody else was going to find. I checked both against the bundled grammar: const_declaration and type_declaration do carry ( as a direct child and var_declaration doesn't, so the rewritten comments are right on both halves. The projection caveat measures out consistent across all four texts, and the surface test's join is sound — I couldn't construct a false pass, and seen ⊆ in_a_documented_block holds unconditionally, so the assert_eq! means what it says.

And thank you for writing the four-rounds-on-the-test paragraph rather than letting the commit list imply a smooth ride. That's the more useful artifact than the fix.

Three left, all in the test, and one of them is a direct correction to a rationale in that paragraph:

The Radius int guard rests on a premise that isn't true. You replaced assert_eq!(checked, 3) with it "since that is the only shape that pushes the blank line" — but _render_decl pushes the trailing blank for any Struct/Interface whatever its children, so Circle struct{} still pushes one. I checked: with the fields removed the gap-closing logic is still exercised, --no-docs still separates and docs-on still joins, and the guard fails anyway asserting the opposite. So it converts a harmless fixture simplification into a failure that misnames its own cause — which is the failure mode you were guarding against one line up.

The Circle anti-vacuity guard doesn't guard what it names. It checks that Circle has a group, never that its own docs is non-empty. Delete // Circle is round. and the docs-plus-group case — the whole reason Circle is the deciding member — disappears while the assertion and its message still pass. Wants both fields.

Put the three collision cases in the fixture rather than in the comment. I had this down as speculative hardening until your note; reverting each one in is real verification and better than I'd assumed. But the comment states them present-tense as fixture contents (type Board struct { Square int }, Square struct{ Square int }, a nested sub/docs.go) and none are there, so the next reader can neither see them nor re-run them, and the key silently weakens the first time someone simplifies it. Five axes verified once by hand is worth less than three of them sitting in the fixture failing on their own — and that also retires the comment.

Yes to the multi-name spec issue, please open it: const ( A, B = 1, 2 ) reporting only A is a real hole and orthogonal to this, exactly as you say. The clippy no_effect_replace came in with main and I'll take it separately.

On trailing comments — agreed, and your reason is better than my instinct to land it here. Every defect in this branch for four rounds hid in the checks rather than the change, and adding an axis to every spec list under that track record earns its own fixture and its own rounds. Filing it; this PR closes #46 and #47 as scoped.

None of the three block anything, so leave them where they are — they'll keep until you're back, and they're yours rather than something I patch over the top of your branch while you're out. The third one is the only one with any shape to it: making those collision cases real means new fixture lines, which shifts the hardcoded L46 and start_line assertions and puts a second file under a directory surface walks recursively — your call whether that's worth it or whether the comment just stops claiming them.

Have a good week off — and thanks for four rounds of this, the sharpest findings in it being the ones you turned back on your own previous commits.