Skip to content

fix(AIC-3614)!: hide SDK-only entry points in internal/sdkapi - #8

Merged
jeffdupont merged 1 commit into
mainfrom
fix/aic-3614-unexport-internal
Oct 19, 2026
Merged

jeffdupont merged 1 commit into
mainfrom
fix/aic-3614-unexport-internal

Conversation

@andrewklatzke

@andrewklatzke andrewklatzke commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Suggested fix — merge only if the team agrees

This is a proposal from AIC-3614, not an agreed change. Please do not merge it unless the team wants the public API break.

Summary

ConfigInternal, GraphInternal, and CreateHandlerInternal were exported so handler packages could skip $ld:ai:sdk:usage. They could not move into aiserver/internal: handler packages are siblings of aiserver, and Go would reject that import.

  • Handler packages call internal/sdkapi instead. Modules outside this repo cannot import internal/.
  • Config and Graph skip usage reporting only when SDKCall is a token from internal/sdkcall. External code cannot construct one.
  • CreateHandler, Config, and Graph stay the application API and still report usage.

Test plan

  • go test ./...
  • Confirm godoc for ConfigArgs.SDKCall and GraphOptions.SDKCall is acceptable on the public structs
  • Confirm handler packages still emit one usage event per helper, not a second client.config.* event

Made with Cursor

Handler packages were calling exported ConfigInternal, GraphInternal, and CreateHandlerInternal. Those now live under internal/, which modules outside this repo cannot import.

Co-authored-by: Cursor <cursoragent@cursor.com>
@andrewklatzke

Copy link
Copy Markdown
Contributor Author

Human message:

Go's internals are generally in an internals package. This just moves as much as we can over into that pattern. This is a departure from our other repos so If there's not a strong sense that we should do this, lets not.

@jeffdupont

Copy link
Copy Markdown
Contributor

Reviewed 737a54c: the change is correct. The token can't be built outside the module (unexported field, internal/ import, zero value = unmarked), no public API hands back a marked ConfigArgs/GraphOptions, every former *Internal caller moved to sdkapi one-for-one, and go test -race -shuffle=on ./... passes.

One gap before merging: no test covers the token on the graph path. TestUsageReporting checks Config, and openaiagents.TestSdkUsage builds OpenAIGraph but never invokes it. client.graph.invoke only fires on Invoke/Stream, so a wrapper that dropped SDKCall, or a broken check in AgentGraph.Invoke, would double-report with no failing test. Suggest adding at the end of TestUsageReporting: Graph("g", GraphOptions{SDKCall: sdkcall.Mark()}) with .Invoke and .Stream, asserting zero usage events. The report fires before resolution, so no graph flag is needed.

Timing: if the team takes this, it should land before the 0.1.0 release PR (#2). Removing exported functions only costs nothing until v0.1.0 exists. Heads-up that this conflicts with #4 in langchainagents/graph.go, langchainagents/handler.go and langchainmessages/handler.go; whichever lands second will rebase.

@apucacao apucacao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

@jeffdupont
jeffdupont merged commit acca582 into main Oct 19, 2026
5 checks passed
@jeffdupont
jeffdupont deleted the fix/aic-3614-unexport-internal branch October 19, 2026 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants