mirror of
https://github.com/google/adk-go.git
synced 2026-10-28 16:44:38 +08:00
João Westerberg
362e5297b5
* ci: guard the exported API against accidental breaking changes
The build and the tests do not catch every break. Adding a trailing
variadic parameter to an exported function changes that function's type:
every ordinary call site still compiles, so nothing goes red, while any
caller that held the function as a value stops compiling. That reached a
release, and a test written specifically to pin backward compatibility
did not catch it either, because it was an ordinary call expression.
The check runs apidiff over every exported package and fails on
incompatible changes. Adding to the API is always allowed.
Two details that matter. It compares against the merge base rather than
the target branch tip, because a branch cut before a recent change to the
target would otherwise be reported as removing whatever the target gained
in the meantime; the first draft did exactly that and produced two
convincing false positives. And internal packages are excluded, since
they are not public API.
Verified end to end rather than assumed: run against the commit that
introduced the variadics it reports both constructors and exits 1, and
against the commit that restores them it exits 0 with no findings.
* ci: pass the base ref to apidiff through the environment
zizmor's template-injection audit flagged the run step, correctly. A
${{ }} expansion inside run: is substituted into the shell text before
the shell sees it, so whatever the value holds becomes code rather than
an argument. Passing it as an environment variable and quoting it in the
script keeps it data.
Caught by CI on the pull request that added the workflow, which is a
reasonable advertisement for the check that job performs.
* ci: allow a deliberate API break behind a label
The check shipped with no way to land an intended break. The failure
message said a maintainer could override it, but the only mechanism was
disabling a required check, which is how checks get removed rather than
overridden.
A pull request labelled breaking-change is still compared and still
prints what changed, so the break is on the record instead of waved
through invisibly. It just stops failing.
* ci: compare whole modules instead of one package at a time
The check took eleven minutes warm and over fifteen cold. It invoked
apidiff once per exported package, and every invocation reloads and
type-checks the entire dependency graph. apidiff -m reads a whole module
in one pass and reports the same findings in seven seconds.
Module mode also reaches plugin/agentanalytics, which go list ./... never
saw. That module is public API and was going unchecked. And it skips
internal/ on its own, so the grep filtering is gone with it.
Separately, a module that cannot be read now fails. Every apidiff call
carried a trailing || true, so a base revision that failed to build wrote
no snapshots, every package was then skipped for want of one, and the job
went green having compared nothing at all. apidiff exits non-zero only
when it could not do its job, so that case is easy to separate from a
run that found a break, and the breaking-change label does not cover it:
a comparison that did not happen is not a break anyone decided to make.
Verified against the same cases as before. A trailing variadic added to
runner.New and to NewBatchProcessor is reported and exits 1, a clean tree
exits 0, and a module that does not compile fails even with the label set.