Conversation
tools/exportcheck (its own module, so x/tools stays out of the SDK's go.mod) reports exported names in pkg/dotc1z, pkg/synccompactor and pkg/sync that no non-test code outside their package references; interface satisfaction counts. .exportcheck-baseline holds the 406 accepted on main, and `make exportcheck` in ci.yaml fails on additions. Run against kans/ledger-storage's head it names the test-only ledger surface that was unexported by hand. .claude/skills/ci-review.md gets a design pass ahead of line comments (external callers, one interface per capability, one name per operation, prose-justified abstractions, test instrumentation in production paths) and a tighter comment budget: one comment finding per review, delete not reword, stale-but-harmless is silent. .cursor/rules/comments.mdc and CLAUDE.md add the writing-side rules: name the kind before writing, delete anything longer than the code under it, delete half before finishing. Co-authored-by: Cursor <cursoragent@cursor.com>
A struct over 20 methods or 15 fields, or an interface over 10 methods, is recorded in the baseline at its current size and may only shrink; types under the limits may grow up to them. Growing a pinned type takes `make exportcheck-update` and a reason in the PR. Fourteen types are pinned on main, Engine at 305 methods. Run against kans/ledger-storage it reports Engine +14, pebbleStore +12, RecordBatch +8 and four new types over the limits, which is the review that branch did not get. Co-authored-by: Cursor <cursoragent@cursor.com>
A record's field count tracks its schema; a generated type tracks its proto. Neither is the accretion the pin is for. Twelve types pinned on main. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,10 @@ | |||
| module github.com/conductorone/baton-sdk/tools/exportcheck | |||
|
|
|||
| go 1.26.0 | |||
There was a problem hiding this comment.
🟠 Bug: this go directive is above the root go.mod's go 1.25.2, and setup-go exports GOTOOLCHAIN: local, so the new Exported surface check step cannot switch toolchains. The go-lint job on this very SHA fails here: go: go.mod requires go >= 1.26.0 (running go 1.25.2; GOTOOLCHAIN=local) → make: *** [Makefile:381: exportcheck] Error 1. Drop this to go 1.25.2 (and pin golang.org/x/tools to a release whose own go directive is ≤ 1.25.2), or set GOTOOLCHAIN=auto on that CI step.
| scoped, err := packages.Load(&packages.Config{Dir: *dir, Mode: packages.NeedName, Tests: false}, strings.Split(*scope, ",")...) | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, "exportcheck:", err) | ||
| os.Exit(2) | ||
| } | ||
| inScope := map[string]bool{} | ||
| for _, p := range scoped { | ||
| inScope[p.PkgPath] = true | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: a ./... pattern that matches nothing is not a packages.Load error — go list just warns. If pkg/synccompactor is ever renamed or a EXPORTCHECK_SCOPE entry is mistyped, scoped comes back empty, inScope is empty, and the run prints exportcheck: ok (0 accepted) and exits 0 with nothing checked. Since this is a guard whose whole value is failing, fail loudly instead: call packages.PrintErrors(scoped) and exit non-zero if any scope pattern resolved to zero packages.
| methods = u.NumMethods() | ||
| over = methods > maxInterfaceMethods | ||
| case *types.Struct: | ||
| methods = named.NumMethods() |
There was a problem hiding this comment.
🟡 Suggestion: types.Named.NumMethods counts only explicitly declared methods, while the interface branch above uses Interface.NumMethods, which includes embedded ones. So a struct pin is evadable by embedding: moving 100 of Engine's methods onto an embedded *engineCore drops the counted total without shrinking the surface the pin exists to bound, and the check reports a shrink. Either count the full method set (types.NewMethodSet on the type and its pointer) or say in the doc comment that only declared methods are pinned.
| for _, it := range ifaces { | ||
| if it.Method(0) == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: ifaces is only appended to under NumMethods() > 0 in both collection sites, so it.Method(0) == nil is never true. Delete the guard.
| func parseSizeLine(l string) (typeSize, bool) { | ||
| var s typeSize | ||
| if _, err := fmt.Sscanf(l, "size %s methods=%d fields=%d", &s.name, &s.methods, &s.fields); err != nil { | ||
| return typeSize{}, false | ||
| } | ||
| return s, true | ||
| } | ||
|
|
||
| // sizeShrank reports whether the baseline pins name at a size no smaller | ||
| // than the current one on both axes. A shrink passes without a baseline | ||
| // update; -update lowers the pin. | ||
| func sizeShrank(accepted map[string]bool, cur typeSize) bool { | ||
| for l := range accepted { | ||
| if base, ok := parseSizeLine(l); ok && base.name == cur.name { | ||
| return cur.methods <= base.methods && cur.fields <= base.fields | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: tools/exportcheck has no tests, and because it is a separate module it is outside ./... for both go test and the root golangci-lint run — 295 new lines with no automated coverage at all. The sizeLine → parseSizeLine → sizeShrank round trip is the part worth a table test: it is the only path that lets a pinned type shrink, and a format drift between the writer and the parser turns every legitimate shrink into a CI failure with no other signal.
General PR Review: review: design pass, exported-surface and type-size check, fewer commentsBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit is documentation only: it rewrites the comment-review rule in Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
The writing test is one question: is this genuinely surprising and not gleanable from the code. The first move is to change the code so it is not; the comment is for what remains. One block for a sizeable, novel algorithm or protocol is the exception. Reviewers never request a comment; a comment finding is false, should-have-been-code, or delete. Co-authored-by: Cursor <cursoragent@cursor.com>
| - Code comments: a comment finding is one of three things. The comment is false. It | ||
| documents a surprise the code should have removed, and the finding is the rename, type, | ||
| or split, not the comment. It documents nothing a reader could not glean from the code, | ||
| and the finding is delete. Anything else about a comment is silent: stale, imprecise, | ||
| too long, restating the diff. Never request a comment, and never propose replacement |
There was a problem hiding this comment.
🟡 Suggestion: "Never request a comment" contradicts this file's own Deprecation section at line 127 ("Go symbols need // Deprecated: comments"), and docs/COMMENTS.md §Reviewing, which says deleting a comment that carried a contract, an invariant, or a Deprecated marker "is a change to that thing and is reviewed as one." A reviewer following the absolute would stay silent when a PR strips a // Deprecated: marker. docs/COMMENTS.md uses the qualified form — "Do not request a comment where a rename or a split would do" — which keeps the deprecation case reviewable.
Separately, this bullet drops both the severity rule it replaced ("A comment finding blocks only when the comment is false in a way that would lead a reader to write wrong code") and the pointer to docs/COMMENTS.md, "Reviewing". Since only this file is appended into the CI review prompt, comment findings now have no stated severity and no route to the authoritative page. (high confidence)
CI runs Go 1.25.2 with GOTOOLCHAIN=local, so this module cannot require Go 1.26. x/tools v0.49.0 is the newest release whose own module graph stays on 1.25. A scope pattern that matches nothing now fails; go list only warns, and an empty scope was exiting 0. Struct pins count the method set of *T, so moving methods onto an embedded field does not shrink the pin. pebbleStore embeds *Engine, so its pin moves from 49 declared methods to 324 in that set. The comment rule no longer says never to request a comment. A // Deprecated: marker and a false contract stay reviewable, per docs/COMMENTS.md. Co-authored-by: Cursor <cursoragent@cursor.com>
| if p.TypesInfo == nil { | ||
| return | ||
| } | ||
| for _, obj := range p.TypesInfo.Uses { |
There was a problem hiding this comment.
🟡 Suggestion: a call like x.M() on an instantiated generic type records the instantiated *types.Func in TypesInfo.Uses, not the declared method, so used[m] at line 304 misses it and the method gets reported as unreferenced. Key the map on obj.Origin() (for *types.Func and *types.Var). No generic type in scope has exported methods today, so this doesn't fire yet. (medium confidence)
| } | ||
| } | ||
|
|
||
| func TestLoadScope(t *testing.T) { |
There was a problem hiding this comment.
🟡 Suggestion: the tests cover parseSizeLine, sizeShrank, and loadScope, but not findUnreferenced or oversizedTypes, which decide what fails CI. A small fixture module run through packages.Load would pin down the cases: used only by tests, satisfies an interface, promoted methods, generated-file skip, and a method-less record over the field limit. (high confidence)
1 participant
Three things that reviews were not catching, made mechanical or made first.
tools/exportcheckIts own module, so
x/toolsstays out of the SDK'sgo.mod. Run withmake exportcheck;make exportcheck-updaterewrites the baseline. In CI on pull requests, in thego-lintjob. Scope:pkg/dotc1z/...,pkg/synccompactor/...,pkg/sync/....kans/ledger-storage's head before its cleanup, it named the whole test-only ledger surface that was then unexported by hand.main;Engineat 305 methods..exportcheck-baselineholds 418 lines: the accepted exports (mostly connector-facing API with no in-repo caller) and the 12 pins. A PR that adds to it says why in the PR. That is the only path to growing a pinned type.Review skill
.claude/skills/ci-review.mdgets a design pass ahead of line comments, on the whole new surface rather than the diff: external callers per exported name, one interface per capability, one name per operation, type size, abstractions justified by a comment rather than a second caller, test instrumentation in production paths. Design findings outrank everything else.Comment budget tightened: one comment finding per review, only when the comment is false in a way that would change what a reader writes; delete, not reword; stale-but-harmless is silent.
Writing side
.cursor/rules/comments.mdcandCLAUDE.md: before adding a comment, name which of the six kinds indocs/COMMENTS.mdit is, or don't write it; a comment longer than the code under it is deleted or moved todocs/; before finishing, delete half the comments you added.docs/COMMENTS.mditself is unchanged.Landing order
This first.
kans/ledger-storagethen rebases andmake exportcheckreports its growth (Engine+14,pebbleStore+12,RecordBatch+8,Ledgernew at 34, two interfaces over 10), which is the review that branch did not get; it accepts with a reason or shrinks.