License review - #13
Merged
Merged
Conversation
Third in the series, after aistudio-server (31 July) and aistudio-app.
LICENSE is already the complete Apache-2.0 text, so detection and
scanning work — unlike aistudio-server finding 1. But the appendix still
read:
Copyright [yyyy] [name of copyright owner]
the template text meant to be replaced. Same as aistudio-app. Inert
legally, since the grant does not depend on the appendix, but it was the
only place in the repository identifying the copyright holder and it
identified nobody.
NOTICE covers the copyright, the third-party software compiled into the
binary, the trademark statement, and one section specific to this repo:
the distinction between what the CLI DISTRIBUTES and what it merely
INSTALLS on the operator's behalf.
That distinction is worth writing down because it cuts in our favour.
ai-studio-cli provisions NVIDIA drivers, the CUDA userspace, Docker or
Podman and vLLM onto GPU nodes, but CoreSpan conveys none of it — the
operator downloads it from the publisher onto their own machine. So the
NVIDIA CUDA EULA question that is the largest open item for
aistudio-server, where we do build and ship images containing CUDA, has
no counterpart here. Absent a statement a reader could reasonably assume
our Apache grant covers the drivers this tool installs. It does not.
.gitattributes is scoped to the file types this change introduces rather
than a blanket `* text=auto eol=lf`. Nine of the 33 tracked files carry
CRLF in the working tree while committed as LF, including several .go
files; a blanket rule would renormalise all of them the next time anyone
touched them, producing a whole-file diff unrelated to whatever they
were changing. That mixed state is worth fixing, as its own commit.
Two findings, both about assets compiled into the binary with go:embed.
Google Fonts
------------
index.html loaded Inter and JetBrains Mono from fonts.googleapis.com.
Same as aistudio-server finding 9, but worse here, and the irony is
sharper: this UI is embedded in the binary precisely so it needs no
external files — then phoned out for fonts on every page load.
`ai-studio-cli bench-ui` runs ON GPU nodes, which are routinely
air-gapped, and serves the dashboard on localhost. A CDN reference there
does not degrade, it fails.
The privacy half applies too: LG Muenchen I, 3 O 17493/20 (20 Jan 2022)
held that disclosing a visitor's IP to Google via a font request is a
GDPR breach absent consent. Here the disclosed IP is the operator's own
machine.
Chart.js
--------
chart.umd.min.js was a 205 KB copy fetched from jsdelivr carrying only a
banner:
/*! Chart.js v4.4.7 ... Released under the MIT License */
A banner naming a licence is not the licence. MIT requires "the above
copyright notice AND THIS PERMISSION NOTICE" to be included; the
permission notice — the paragraph that actually grants the rights — was
absent, and there was no LICENSE file anywhere near it. Provenance was a
CDN rather than a registry, with jsdelivr's own "Do NOT use SRI with
dynamically generated files" warning still in the header.
Replaced with the npm chart.js@4.4.7 artifact, verified byte-identical to
the vendored copy apart from that banner — so it is provably the same
code from a canonical source — with LICENSE-chartjs.md beside it.
Both handled by scripts/vendor-ui-assets.sh, which also writes
vendor/NOTICE. Latin subset, only the four Inter and two JetBrains
weights index.css actually references.
Unlike aistudio-app, where the equivalent binaries are gitignored and
fetched at setup, these are COMMITTED. `go build` cannot run npm and
go:embed needs the files present at compile time — making a working
build depend on a prior npm run would break `go install` for everyone.
Verified: `//go:embed ui/*` recurses into vendor/fonts/ and vendor/js/,
no vendored filename starts with . or _ (which embed would silently
skip), and spaHandler's fs.Stat + http.FileServer resolves the nested
vendor/ paths.
This is the blocker, and the distribution model makes it the strictest of
the three repos.
`go build` links every module in the build graph into one executable, and
releases publish that executable. The graph includes cobra (Apache-2.0),
viper, pflag, fsnotify, afero, cast, mapstructure, go-toml, locafero,
conc, gotenv, mousetrap, golang.org/x/{term,sys,text} and yaml.v3 — MIT,
BSD-3 and Apache-2.0, all of which condition redistribution on carrying
the copyright notice, with Apache-2.0 s4(d) additionally requiring any
NOTICE file to be reproduced.
Unlike an interpreted project there is no node_modules or site-packages
beside the artifact for those notices to live in. The binary IS the
distribution. If the notices are not inside it they are not accompanying
anything, and they were not.
So they go inside it — the same approach kubectl, docker and gh take:
ai-studio-cli licenses # everything embedded in this binary
ai-studio-cli licenses cobra # filter to one dependency
internal/notices holds the text via go:embed, in its own package so that
`go build` fails loudly if the file is deleted rather than silently
producing an unattributed binary.
scripts/generate-notices.sh runs go-licenses over the REAL build graph —
only what is actually linked, not everything in go.sum, which also lists
test and tooling modules that never reach a user. It appends the embedded
UI's font and Chart.js notices, which go-licenses cannot see because they
are not Go modules but are equally compiled in.
This survives the case that matters: someone copies the binary to an
isolated GPU node with no network and no checkout. The notices go too.
NOT YET GENERATED
-----------------
The committed THIRD-PARTY-NOTICES.txt is an explicit placeholder. The
environment this was written in cannot reach proxy.golang.org, go.dev or
GitHub, so the machinery is complete but has not been run:
make notices && git commit
One command with a Go toolchain and network. I deliberately did not
hand-write the inventory — writing licence facts I could not verify into
a compliance document is the failure mode that makes such documents
worthless.
Guards while the placeholder is present: `make compliance` fails, CI
fails, and the release workflow refuses to publish. A released binary
must never ship without its notices, and `licenses` reports the fault
rather than printing an empty page that reads like "no dependencies".
The installer resolved releases/latest at run time, downloaded a tarball
over curl, and `sudo mv`d the contents into /usr/local/bin. No checksum,
no signature, no version pinning, no check that the tarball even
contained the expected binary. It extracted into /tmp under fixed names,
so concurrent runs clobbered each other and failures left files behind.
This is aistudio-server finding 2, sharper on two counts. That installer
at least pinned a tag; this one installed whatever was newest at that
instant, so it could not be used in a reproducible provisioning flow —
which is what this tool is for. And the payload goes into /usr/local/bin
with root, not into a compose directory.
install.sh now:
- takes a version (`./install.sh v1.2.3`), defaulting to latest but
resolving it once, printing it, and showing how to pin it
- downloads and verifies SHA256SUMS; a mismatch aborts with a report
link. Verifies a detached GPG signature when one is present
- uses a private mktemp -d with a cleanup trap
- confirms the tarball contains the binary before installing
- skips sudo entirely when $INSTALL_DIR is writable, and says so when
it does need it
- uses `set -euo pipefail` rather than bare `set -e`
The missing-checksum path warns rather than aborting only because
existing releases predate SHA256SUMS. There is a comment marking where
to make it fatal once every supported release publishes one.
Verified the verification: with a synthetic SHA256SUMS, a matching
tarball passes, and a filename that grep cannot find makes sha256sum -c
exit 1 rather than silently succeeding on empty input — which was the
failure mode I most suspected in this script.
release.yml produces what the installer expects: refuses to build from a
lightweight tag (no author, date or signature, and silently repointable),
gates on `make compliance` and `make notices-check`, verifies the built
binary prints real notices before publishing, and attaches SHA256SUMS
plus the notices as their own asset so they can be read without running
the binary. LICENSE, NOTICE and the notices also go inside the tarball.
Four jobs. Three verify files in the repository; the fourth builds the
binary and checks the notices are inside the artifact a user receives,
which is what the obligation actually is.
files make compliance, plus a clean regeneration of the vendored UI
assets diffed against the committed copies (they are
committed, so nothing otherwise stops a hand edit), plus a
check that every font fonts.css references exists — a weight
added to the script's list must not ship as a CSS rule with
no file behind it
notices make notices-check — go-licenses over the real build graph
build `ai-studio-cli licenses` prints 10+ notices including the OFL
text and Chart.js; and `strings` on the compiled binary finds
no fonts.googleapis.com, asserted against the artifact rather
than the source
vet go vet
Makefile adds notices, notices-check, vendor-ui and compliance alongside
the build targets, so the local loop matches CI without a push.
Three bugs in my own first draft of the Makefile, found by running it:
`grep -q X && (echo FAIL && exit 1) || echo ok` printed BOTH branches
and did not fail the recipe — the || catches the subshell's own exit.
Replaced with if/then/fi.
The same swallowing pattern with `|| true` meant the LICENSE
placeholder check could never fail.
The CDN check grepped for bare hostnames and so flagged the comment in
index.html that explains why the CDN links were removed. It now matches
only real loading positions (src=/href=/url()/@import followed by an
absolute URL). A check that fires on its own documentation is a check
someone deletes.
README gains a Licensing section. For a change whose purpose is making
the licence position legible, the most-read file was silent on it. It
also documents the installs-but-does-not-distribute boundary.
docs/LICENCE-REVIEW-RESPONSE.md is the finding-by-finding write-up,
including which of the server's ten findings do not apply and why. Two
were already satisfied here: go.sum pins every module by cryptographic
hash — a stronger guarantee than either other repo had — and there are no
container images, so the CUDA EULA question does not arise.
It closes with the pattern across all three repos: same obligation, three
artifacts, three different places the notices had to go. "We have a
LICENSE file" answers a different question from "does the attribution
reach the person receiving the software". In all three the second answer
was no.
This repo has core.fileMode=false, so chmod +x in a working tree never reaches the index and everything commits as 100644. A CI runner then checks the scripts out non-executable and `./scripts/generate-notices.sh` dies with exit 126, "found but not executable". That is exactly how the aistudio-server branch failed its first CI run, so setting the bit up front here rather than discovering it again. The Makefile and both workflows invoke these as `./scripts/...`, and install.sh is documented as `./install.sh`, so all three need it. internal/provision/assets/*.sh are deliberately left 100644: they are payloads copied to remote nodes and invoked as `bash <script>` there, not executed from a checkout.
Fixes the three failing checks on PR #13. All three had one root cause, and the diagnosis was more interesting than "unfinished work" — the design was wrong. files make compliance -> placeholder build make build OK, then `licenses` found the placeholder notices make notices-check exit 2 I had made the generated inventory a COMMITTED artifact with a CI job diffing it against a fresh go-licenses run. Two problems with that: A reviewable branch could not exist without a Go toolchain and network access. The environment this work was done in cannot reach proxy.golang.org, so the notices could not be generated and CI was red by construction. Worse, it put a regenerate-and-commit step on the critical path of every dependency bump, whose only failure mode is a red build with a message that reads like a mistake rather than a routine step. The obligation is narrower than the design assumed. Nothing requires the inventory to live in git. What must be true is that no RELEASED binary ships without its notices. So: make notices generate (needs Go + network) make build-release generate, build, fail if the binary cannot print them — run by CI and by the release workflow make compliance static checks only; reports the placeholder as the normal state rather than failing on it A fresh checkout embeds the placeholder and `ai-studio-cli licenses` says so, distinguishing "you built this yourself, run make build-release" from "this is a released binary, that is a packaging fault, please report it". Dropped go-licenses entirely ---------------------------- The notices job failed with exit 2 after 38s, which may have been go-licenses rather than staleness. It was pulling `go install ...@latest` into every CI run — unpinned, an extra network dependency — and it fails hard on modules whose licence it cannot classify, which turns a licence-notice job into a licence-classifier argument. `go list -deps` already answers the only question that matters: which modules are actually linked into the binary. The module cache already has their licence files. Reading them directly needs no extra tooling and cannot fail on an unrecognised licence. It also stops asserting SPDX identifiers we cannot derive — each entry names the licence FILE as shipped, and the verbatim text below it governs. Modules shipping no licence file are recorded explicitly rather than silently omitted, and the script refuses to overwrite good notices with anything under 50 lines or still containing the placeholder marker. Two bugs found while testing this --------------------------------- `{ ...; [ -n "$MISSING" ] && echo ...; } | sed > out` — under `set -o pipefail` a false test makes the whole pipeline fail, so the script aborted precisely when nothing was wrong. Same class of bug as the Makefile `&&`/`||` swallowing fixed earlier in this branch. Now if/then. `grep -v '^$'` exits 1 when it filters everything out, which pipefail would also treat as failure. Replaced with `sed '/^$/d'`. Also made build-release sequence its steps inside the recipe rather than as prerequisites — those can run in parallel under `make -j`, racing the build against the generator and possibly embedding the placeholder in a binary that then passes the check by luck. Verified locally: make compliance passes, the vendored-asset regeneration diff is byte-identical, all 6 fonts in fonts.css resolve, both pipefail paths exit 0, and the module-walk loop handles a missing licence file correctly.
Fixes the `build` job on PR #13, and the cause was not the licence notices at all. build: cd ai-studio-cli && go build -o ../ai-studio-cli . The module lives in a directory named ai-studio-cli, and the binary is also called ai-studio-cli. So `-o ../ai-studio-cli` from inside the module resolves to the module directory itself, and `go build -o` given an existing directory writes the executable INSIDE it. The binary landed at ai-studio-cli/ai-studio-cli; `./ai-studio-cli` at the repo root was still the directory. Running a directory gives "Is a directory", exit 126, which make reports as exit 2 — the exact symptom. It also retroactively explains the earlier `build` failure I attributed to the placeholder: the annotation said "`ai-studio-cli licenses` failed", which was 126, not a missing notices file. Now builds to bin/, with `test -f` afterwards so a non-regular-file target can never pass silently again. bin/ is gitignored. Found by installing a Go toolchain and actually running it. The Ubuntu archive is reachable from this environment even though proxy.golang.org is not, so I extracted golang-1.24-go and built a synthetic module with local `replace` directives to exercise the whole chain offline. Three rounds of guessing at this from annotations alone had produced two plausible-but-wrong theories; running it took one attempt. Also dropped `go mod download` from the notices script ------------------------------------------------------ With no arguments it resolves the entire build list, which can include modules go.sum has no entry for because nothing needs them to compile. It then fails with "missing go.sum entry" — a step that passes `go build` and `go vet` and fails on its own. `go list -deps` downloads exactly what it needs to resolve the main package's imports, which is the set we want. Hardened while in there: - `go list` runs into a file with an explicit status check rather than as the head of a pipeline, where pipefail turns a Go error into an opaque exit and sed/sort happily emit an empty file from it - a module resolving with an empty .Module.Dir is now a hard error; previously its licence files would have been silently absent, which is the failure this script exists to prevent - release.yml's `grep -q ... && { exit 1; }` became if/then. The AND-list returns grep's status, so the good case evaluates to 1, and under `bash -e` that fails the step precisely when nothing is wrong. Third instance of this trap in this branch. Verified end to end against the synthetic module: make build-release exits 0, produces a real ELF at bin/, and `licenses` prints 28 copyright lines including the OFL text and Chart.js. The compiled binary contains no fonts.googleapis.com string. The "no dependencies" and "no licence file shipped" guards both fire correctly.
Fixes the pre-existing claude-review failure, which predates this branch.
Action failed with error: Command failed:
git fetch origin master --depth=1 --no-recurse-submodules
Both workflows checked out with `fetch-depth: 1`, so the runner had only
the PR head commit and no base branch. The action's own attempt to fetch
master had nothing to graft onto and failed before the review started. A
review action has to diff against the base branch, which means the base
branch has to be present.
Changed to `fetch-depth: 0` in both. claude-code-review.yml is the one
that was failing; claude.yml has the identical checkout and would hit the
same wall whenever it is invoked on a PR, so it is fixed at the same time
rather than left as a trap.
The extra clone cost on a repo this size is negligible next to a job that
cannot run at all.
Written from the committed blobs rather than the working tree: both files
are among the nine tracked files that carry CRLF on disk while committed
as LF, so editing in place would have produced a whole-file diff instead
of a 13-line one.
Not changed, but worth a look separately: both grant `pull-requests: read`.
The review action needs `write` to post review comments, so it may still
be unable to publish its output even once it runs. I have left that alone
rather than widening a permission as a side effect of a build fix.
The `build` job's last step failed with "the compiled binary still
references Google Fonts". It does — in a comment explaining why the Google
Fonts links were removed.
index.html carries that comment, go:embed puts the file into the binary
verbatim, and the check was a bare hostname grep over `strings` output. So
it matched its own documentation.
I had written a comment in the Makefile warning about precisely this trap
for the source-level check:
# A bare hostname grep flags the comment in index.html that explains
# why the CDN links were removed, which is a good way to get the
# check deleted.
and then wrote the naive version for the binary-level check.
Now matches a loading position — `="https://host` — which is what
separates markup from prose. A real regression looks like
`<link href="https://fonts.googleapis.com/...`; documentation naming the
host does not.
Verified against an actual compiled binary embedding the real UI, not just
against the source:
current index.html no match (hostname present in the comment)
reintroduced <link> caught at the embedded byte offset
jsdelivr / unpkg / cdnjs /
fonts.gstatic in src= all caught
prose naming the hosts ignored
Also confirmed there that `//go:embed ui/*` does pull in the nested
vendor/fonts/ and vendor/js/ trees, which the whole approach depends on.
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
2 participants
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.
licensescommand C1 (blocker)