Repository navigation
Conversation
|
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 30 minutes. View limit details
Comment |
d886093 to
2cc189f
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of this PR's own diff against test/golang-eql-v4 (5 files). The basic approach is sound: committing the guests on a release-only commit and pushing only the tag does get them into the module zip, and the tag prefix languages/golang/vX.Y.Z is correct for the subdirectory module. What needs work before the first real run: the write token is exposed to the whole build, the workflow can be run from any ref with no approval step, the proxy check does not verify the guests, and the licence for the shipped binaries is unresolved. Every step after the tag push cannot be undone, so these need to be fixed before the workflow runs, not after.
What I verified and found OK: the deterministic TEST builds go to encrypt/testdata/ (mise.toml:213, :276), and the job force-adds only the three embedded paths, so a seeded guest cannot ship. VERSION reaches run: only through env, never through ${{ }}. The action SHAs and the mise version match the rest of the repo. No cache is restored. actionlint is clean, and pnpm run test:scripts passes (72 files).
1. Major: a write token sits in .git/config while the job compiles and runs third-party code
.github/workflows/release-golang.yml:42-44, :57-60
persist-credentials: true together with job-level contents: write stores the token where every later process can read it. Before the push at :130, this job runs:
- the
mise installof the test toolset; - three guest dependency graphs, plus
eql-codegen, with their build scripts and proc macros; go test ./....
Any one of those can read the token, then push tags, move existing tags or create releases. This is the only workflow in the repo that sets persist-credentials: true. The FFI and auth release lines keep building and publishing in separate jobs (_build-ffi-artifacts.yml → publish-ffi) for this reason.
Fix: split the workflow into two jobs.
- A build job with
contents: readandpersist-credentials: false. It builds the guests and the SQL, runs the tests and uploads the artifacts. - A small publish job with
contents: write. It downloads the artifacts, checks them againstguests.sha256, then commits, tags and pushes, passing the token only to that step. It runs no build tooling.
A side benefit: "Re-run failed jobs" can then retry the publish half without rebuilding (see finding 5).
2. Major: no ref guard or approval, and the release gate is weaker than CI
.github/workflows/release-golang.yml:22-28, :40, :92-93
Anyone with write access can dispatch this workflow from any branch. It builds whatever that ref contains and publishes it permanently: once proxy.golang.org has fetched the version, it cannot be withdrawn. The PR body plans to run it from ci/golang-release, which is the unmerged top of the 8-PR stack. The tag message would then say "built from " about a commit that is not on main and may be rebased away.
The job's own checks are also a subset of tests-golang.yml's wasi-check job. It does not run:
wasm:guest:testorwasm:auth-guest:test;- the
go generatedrift check; - golangci-lint;
- the live tests.
It also does not set STACK_ENCRYPT_TESTS_REQUIRE_LOCK or raise the memlock limit, so the memory-lock test skips silently.
Fix:
- Gate the job on
github.ref == 'refs/heads/main', or ongit merge-base --is-ancestor "$GITHUB_SHA" origin/main. - Put it behind a protected
environment:with required reviewers. - Require that
tests-golang.ymlpassed for$GITHUB_SHA, for example by querying the commit's check runs.
If the partner alpha really must ship from the stack branch, make that a documented one-off exception and don't leave it as the workflow's default behaviour.
3. Major: the released module has no licence, and it embeds binaries built from PolyForm Internal Use crates
.github/workflows/release-golang.yml:107-130, SECURITY.md:40-43
The guests link stack-encrypt, stack-kms, stack-auth, stack-profile and stack-guest-abi, which all use license-file = "LICENSE" (PolyForm Internal Use 1.0.0). SECURITY.md says these "may be used for internal business operations only, and not redistributed". This workflow makes them publicly downloadable from the Go proxy, which caches them permanently.
The module zip will also contain no licence file at all:
- For a module in a subdirectory, Go copies the repository-root licence into the zip only if the file is named exactly
LICENSE. Seecmd/go/internal/modfetch/coderepo.goaround lines 1141-1173 in Go 1.26. - This repository's root file is
LICENSE.md. languages/golang/has no licence file of its own.
Consumers would therefore receive binaries with no stated terms. pkg.go.dev also will not show documentation for a module with no detected redistributable licence.
Fix: decide what terms the Go module and its embedded guests are released under. Then commit languages/golang/LICENSE, including a notice covering the embedded guests if their terms differ, before the first tag. Have the workflow fail when that file is missing from the tree it tags.
4. Minor: the "Go resolves the version" check does not look at the guests
.github/workflows/release-golang.yml:163-176
go list -m <module>@<version> downloads only .info and .mod. I confirmed this with an empty GOMODCACHE: after the call the cache held list, v1.12.0.info and v1.12.0.mod, and no zip. The .mod file is identical with or without the guests, so this step would also pass for a tag on a plain main commit, which is exactly the failure the workflow exists to prevent.
Fix:
- Run
go mod download -json <module>@v$VERSIONand read.Zip. - Check that the three
.wasmpaths are in the zip and that their SHA-256 values matchguests.sha256. - Better still, before the push, run the consumer test the PR body describes doing by hand:
git archivethe release commit'slanguages/golang, then build a throwaway module against it with areplacedirective and load the EQL guest. That way a bad tree never reaches the proxy.
5. Minor: nothing after the tag push can be retried, and the existing-tag check misses versions the proxy has already cached
.github/workflows/release-golang.yml:62-68, :130, :132-159, :163-176
Three things go wrong after the tag is pushed:
- If
gh release createor the 2-minute proxy wait fails, the run is red even though the tag is already public. - A re-run then stops at "Refuse a tag that already exists", so the only way to retry is to delete the tag by hand.
- Deleting is unsafe once the proxy has the version.
git ls-remotecannot see a version the proxy still serves. A rebuilt guest with different bytes would then makegoreport a checksum mismatch for anyone who fetched the first copy.
Fix:
- Before tagging, query
https://proxy.golang.org/github.com/cipherstash/stack/languages/golang/@v/v$VERSION.infoand refuse the release if it returns 200. - Make the release step idempotent, the way
release.yml's "Tag and release" step is:gh release view || gh release create, thengh release upload --clobber. - With the job split from finding 1, "Re-run failed jobs" retries only the publish half.
6. Minor: the version check accepts versions Go rejects, and it runs too late to help
.github/workflows/release-golang.yml:52
The regex accepts:
- majors of 2 and above, which need a
/v2(or higher) suffix on the module path, butgo.moddeclaresmodule github.com/cipherstash/stack/languages/golang; - numbers with leading zeros, such as
01.2.3or1.0.0-alpha.01, which are invalid semver.
Go first rejects these at the proxy step, after the tag and the GitHub release already exist.
Fix: use a strict semver regex with no leading zeros. Fail when the major version is above 1 unless the go.mod module path ends in the matching /vN.
7. Minor: the guest builds don't use --locked, so a release can ship dependencies nobody reviewed
mise.toml:157, :234, :324 (called from release-golang.yml:84-90)
The guest builds are plain cargo build. If a guest Cargo.lock is out of date, cargo re-resolves it in memory against crates.io at release time. That builds dependency versions nobody reviewed into the shipped binary. The resolved lock is also not on the release commit, so the shipped guest cannot be rebuilt from its tag.
cargo-lock-freshness.test.mjs:36 documents that it only catches version skew of crates in this tree, not a newly added dependency.
Fix: in this workflow, run cargo metadata --locked --format-version 1 >/dev/null in both guest directories before building, or git diff --exit-code -- languages/golang/*/guest/Cargo.lock after building. Either way the shared tasks stay unchanged.
8. Minor: SECURITY.md is now out of date
SECURITY.md:35-37, :53
SECURITY.md still says the Go module "has no release yet", and its "Note on publishing" lists every publishing workflow except this new one. The AGENTS.md checklist (item 7) asks for meta files to be updated in the same PR.
Fix: update both passages.
9. Nit: release tags have no repository protection
This is repository configuration, outside the diff. The only active ruleset covers the main branch, so anyone with write access can push, move or delete languages/golang/v* tags. The sentence "Never tag a main commit by hand" in AGENTS.md is the only safeguard.
Fix: add a tag ruleset for languages/golang/v* that blocks create, update and delete, with GitHub Actions as the only bypass actor.
10. Nit: the Go READMEs will be wrong for readers of a tagged version
languages/golang/encrypt/README.md:310, languages/golang/auth/README.md:120, and the three */wasm/README.md files tell readers the guests are not committed and that they should run mise run wasm:guest:build. That is wrong for anyone reading a tagged version on pkg.go.dev.
It would also help to say, here or in AGENTS.md, that @main and pseudo-versions resolve to a module with no guests.
11. Nit: the comment on MISE_ENV: test is wrong
.github/workflows/release-golang.yml:47-48
The comment says MISE_ENV: test is there for wasm:guest:test, but this job never runs wasm:guest:test or nextest. The setting makes mise install the six cargo tools in mise.test.toml, uncached, and nothing in the job uses them. They also add to the third-party code that runs alongside the token in finding 1.
Fix: drop the setting if go:test passes without the test environment. Otherwise, set the specific [env] values it needs.
6b9b42c to
3aaeddd
Compare
2cc189f to
9e82e67
Compare
3aaeddd to
8bf6a10
Compare
9e82e67 to
b00e2d6
Compare
|
Rebased onto the updated #1165, and this branch now carries #1167's release workflow rather than a second version of it. While addressing the review above, I found that #1167 already has three newer commits answering it:
I cherry-picked those three onto this branch (now 1f34e01, 14aaab0 and b00e2d6). Findings from the review above that #1167's version still leaves open:
Item 11 ( |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Automated review attestation — /code-review medium plus a cross-check of its findings against head b00e2d64.
Critical / blocking: none.
Non-blocking (3 inline comments):
ls-remote --exit-codefailures (status 128) are read as "tag absent".- The artifact's 7-day retention is shorter than the "Re-run failed jobs" recovery path needs.
gh release createhas no--latest=false.
Reviewed and dropped as low value:
- Tag adoption does not compare the run ID. The commit and tree match is already a strong check.
go.modmodule-line regex strictness. It fails closed.- The
verifyretry window on proxy negative caching.
Generated by Claude Code
| run: | | ||
| set -euo pipefail | ||
| tag="languages/golang/v$VERSION" | ||
| if git ls-remote --exit-code --tags origin "refs/tags/$tag" >/dev/null; then |
There was a problem hiding this comment.
Non-blocking. git ls-remote --exit-code exits 2 when no ref matches but 128 on network/auth errors. Both tests (here and line 86) read any non-zero status as "tag does not exist". The line 86 guard is skipped on a transient failure, and the run then spends a full build before publish. Here, publish pushes against a remote it could not verify. Distinguish status 2: rc=0; git ls-remote ... || rc=$?; [ $rc -eq 2 ].
Generated by Claude Code
| name: go-release | ||
| path: release-out/ | ||
| if-no-files-found: error | ||
| retention-days: 7 |
There was a problem hiding this comment.
Non-blocking. retention-days: 7 limits the documented recovery path ("Re-run failed jobs"). publish and verify both download this artifact. If publish fails after the tag is pushed and nobody re-runs it within 7 days, the artifact is gone. The tag is immutable, so the result is a proxy-fetchable version with no GitHub release and no EQL SQL bundle. Raise the retention, or document the window next to the re-run advice.
Generated by Claude Code
| } > notes.md | ||
| prerelease=() | ||
| if [[ "$VERSION" == *-* ]]; then prerelease=(--prerelease); fi | ||
| gh release create "$tag" --verify-tag "${prerelease[@]}" \ |
There was a problem hiding this comment.
Nit. gh release create has no --latest=false, so a final Go release is marked as the repo's "Latest" release. release.yml publishes the npm packages' releases in this repo, so /releases/latest would then point at a Go module release. Consider --latest=false.
Generated by Claude Code
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
Review attestation. Files reviewed (12): .github/workflows/release-golang.yml, scripts/go-release-version.mjs, scripts/tests/go-release-version.test.mjs, scripts/tests/workflow-publish-permissions.test.mjs, scripts/tests/lint-no-workflow-caching.test.mjs, scripts/tests/eql-suite-ci.test.mjs, scripts/lint-no-workflow-caching.mjs, AGENTS.md, SECURITY.md, languages/golang/LICENSE, languages/golang/auth/README.md, languages/golang/encrypt/README.md. Models: Claude Sonnet 5.5 (claude-sonnet-5-5).
Generated by Claude Code
| - name: Commit the guests | ||
| run: | | ||
| set -euo pipefail | ||
| while read -r _ guest; do |
There was a problem hiding this comment.
publish trusts the guest paths in guests.sha256 from the artifact. build runs third-party code (cargo build scripts, proc macros), so the artifact is untrusted here. install -D ... "$guest" followed by git add -f writes and commits whatever paths the file names. A compromised build could list .github/workflows/x.yml, a ../ path, or any file in languages/golang, and commit it on the tag. Validate the list against the three expected .wasm paths (and reject ..) before installing or adding anything.
Generated by Claude Code
| git commit -m "release(golang): v$VERSION with its WASI guests" \ | ||
| -m "Built from $GITHUB_SHA by $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" | ||
| tree=$(git rev-parse 'HEAD^{tree}') | ||
| if [ "$tree" != "$TREE" ]; then |
There was a problem hiding this comment.
The tree check does not give the independence the header comment and AGENTS.md claim. TREE is a job output of build, the same job that produced the artifact, so a compromised build can emit a matching tree for tampered guests. It only catches accidental drift between the two jobs. Either soften the wording, or compare against something build cannot choose (for example, require exactly the three expected files and their expected locations).
Generated by Claude Code
| timeout-minutes: 15 | ||
| # Required reviewers set on this environment in the repository settings | ||
| # gate every release; with none set, it gates nothing. | ||
| environment: release-golang |
There was a problem hiding this comment.
The only human gate is the release-golang environment's required reviewers, which are set outside the tree (the header admits it gates nothing if unset). A pre-release may be dispatched from any branch, and the dispatched ref's copy of the workflow runs, so a branch that deletes environment: or edits publish can push a languages/golang/v*-alpha tag with no review. Protect the tag pattern with a tag ruleset, or restrict dispatch to protected refs.
Generated by Claude Code
| contents: read | ||
|
|
||
| concurrency: | ||
| group: release-golang |
There was a problem hiding this comment.
cancel-in-progress: false does not queue every run. GitHub keeps one pending run per concurrency group and cancels older pending ones. Two releases dispatched while one is running silently drop the middle one. Fine for manual use, but worth a comment or a per-version group.
Generated by Claude Code
| cache: false | ||
|
|
||
| - name: Add rustfmt and clippy to the pinned toolchain | ||
| run: rustup component add rustfmt clippy |
There was a problem hiding this comment.
rustfmt and clippy are not used by any step in this job, which only builds and tests. This adds a network fetch to the job that builds the published binaries, for no purpose. Drop it unless a mise task needs it.
Generated by Claude Code
|
|
||
| /** The `module` line of a go.mod. */ | ||
| export function modulePathOf(goMod) { | ||
| const match = goMod.match(/^module\s+(\S+)\s*$/m) |
There was a problem hiding this comment.
The regex ^module\s+(\S+)\s*$ fails on a go.mod module line with a trailing comment (module x // c) or a quoted path, and then throws 'go.mod has no module line'. The failure is safe (it stops before anything is tagged) but is a false refusal. Allow an optional // comment and quotes.
Generated by Claude Code
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b00e2d64a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run: | | ||
| set -euo pipefail | ||
| while read -r _ guest; do | ||
| install -D -m 0644 "$guest" "release-out/$guest" |
There was a problem hiding this comment.
Direct Rust cdylib builds for wasm32-wasip1 produce the guest files with mode 0755, so the build job's earlier git add -f records them as 100755 in TREE; this staging step forces the artifact copies to 0644, and the publish job preserves that mode when rebuilding the commit, producing 100644 entries and always failing the tree comparison before the tag can be pushed. Normalize the original guests before calculating TREE, or preserve their modes through the artifact.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
8bf6a10 to
d280ccf
Compare
b00e2d6 to
d280ccf
Compare
GitHub Actions
Failure
This branch had an error being deployed
3 participants
Summary
This adds a release process for the Go module, so a Go user can
go geta version. The module embeds three WebAssembly engine builds ("guests") compiled from Rust. They are gitignored onmain, and Go fetches a version as the files committed at its tag. So tagging amaincommit would give users a module that builds and then fails atNewClientwithErrGuestNotBuilt.Changes
.github/workflows/release-golang.yml, dispatched by hand with a version:eql_v4.version()withgo-<version>;languages/golang/v<version>and pushes only the tag;proxy.golang.orgto resolve the version, so a tag that does not resolve fails the run.lint-no-workflow-cachingand the EQL suite's publish-path exemption.AGENTS.mddocuments the process.Verification
pnpm run test:scripts: 71 files pass, including every workflow lint.git archiveof the tag contains all three guests, and a consumer module built from exactly that tree loads the EQL guest and seesIntegerEqproducible onpublic.eql_v4_integer_eq.go-<version>in the v4 bundle, and the workflow greps for the stamp.Review notes
To cut the partner alpha before the rest of the stack merges, land this workflow on
mainon its own, then rungh workflow run release-golang.yml -f version=0.1.0-alpha.1 --ref ci/golang-release.Related
Refs #1034. Stacked on #1165.
https://claude.ai/code/session_01CA4pFfaN4DPNSTLnJVNxsu