Repository navigation
Conversation
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews, nothing blocking a merge.
worth doing, does not block
- Review job still has a write token on disk: the action writes it to the remote URL (.github/workflows/claude-code-review.yml:29)
nits
- claude.yml checkout still persists a contents:write token and is not in the follow-ups (.github/workflows/claude.yml:21-24).
claude.ymlkeeps the token from its checkout, even though the job hascontents: write,pull-requests: writeandissues: writeand runs an agent. The description says this PR hardens the workflows that hold tokens. Its follow-up list only covers the remaining test workflows, all of which havecontents: read.claude.ymlis in neither group. Addingpersist-credentials: falsethere may well be safe, because the action sets up its own git auth for pushes, but nobody has checked that yet. It is worth adding to the follow-up list so it is not missed.
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Moves every ${{ }} out of run: scripts into step env:, validates the publish version before use, and sets persist-credentials: false on the checkouts of the publish, bindgen and review workflows. Reviewed 43458b9, full tier (CI workflows).
What I checked, and 4 candidates I ruled out
Read in full: gradle-publish.yml, claude-code-review.yml, android-bindgen.yml, ios-bindgen.yml at the head.
Build: not built. The change is workflow-only, and a Bitkit-track auto review does not run the PR's code here. CI is green: Android Bindgen, iOS Bindgen, validation, USDT, wake and Paykit route.
Checks run
- A scan of every
run:block in.github/workflows/at the head finds no${{ }}. Onmasterit finds 9. - I read the version step, then ran a copy with bash.
v0.8.0-rc15,0.8.0-swaps.11and an empty input (falling back to the ref name) pass. A trailing space, an embedded newline,v1.2.3-..and$(touch PWNED)exit 1 and write no output; nothing was created. All 28 tags in my clone match the pattern. git merge-treeof #210 and #211 merges cleanly.
Ruled out
- Publish needs the persisted token: Gradle publish reads
GITHUB_TOKENandGITHUB_ACTORfrom env (bindings/android/lib/build.gradle.kts:251-252), andgh release uploadgetsGH_TOKEN. The only git fetches in the build are public:cargo install --gitinbuild_android.sh:39, and the git dependenciesswap-commonandjade-client-rsinCargo.toml. - Dropping
server-id/settings-pathbreaks publish: the publish goes through Gradle'smaven { url = "https://maven.pkg.github.com/$repo" }with its own credentials, never through a Mavensettings.xml. - The review agent needs git credentials: that workflow only allows
ghcommands, which readGH_TOKEN, plus read-only git commands, and the action sets up its own git auth. - A non-numeric
pr_number: the input istype: number, and the added regex closes the shell path anyway. The prompt interpolation is not arun:script.
Merge confidence: 4/5. gradle-publish.yml and claude-code-review.yml only run on release or dispatch, so no CI run covers their changes.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 43458b9:
Check: workflow syntax and shell expressions ✅ actionlint and shell syntax checks passed.
Ran actionlint 1.7.12 across all workflows. Parsed all nine workflow files and checked every run: block with bash -n; none contains an Actions expression.
NDK paths remain literal shell data ✅ both export scripts preserved the path.
Executed both workflow scripts with a synthetic NDK path containing spaces and shell metacharacters, then compared the exported variables with the original path. The publish script also rejected a mismatched NDK revision.
Gradle receives the selected version as one argument ✅ build and publish arguments stayed literal.
Executed the workflow's build and publish shell commands with an argument-recording Gradle stub. Each received one literal -Pversion argument; this did not build or publish a package.
Check: publish versions and review numbers are validated ✅ covered by existing evidence at this head.
Relied on the PR's QA notes and the prior review at 43458b9 for accepted versions, tag fallback and rejected malformed inputs. Those cases were not repeated.
Check: Android and iOS bindings build in CI ✅ both exact-head jobs passed.
Verified the commit and successful completion of both runs:
No app build or device run: this changes only workflows and has no device QA case. Release publishing and the Claude manual-dispatch workflow were not triggered; their live execution remains unverified.
Full review of the diff, four files, including the build and publication paths they reach. The resolved token discussion is already reflected in the description and follow-ups.
Reviewed by gpt-6.1-sol-high
Hardens the workflows that hold tokens: the publish job validates its version and stops leaving the token on disk, and no workflow interpolates
${{ }}expressions into shell scripts any more.Description
gradle-publish.yml(runs on release and manual dispatch, withcontents: writeandpackages: write):persist-credentials: falseon checkout, so the write token isn't left in.git/configwhilebuild_android.sh, Cargo build scripts and Gradle run. Nothing later needs it:ghgetsGH_TOKENand Gradle publish getsGITHUB_TOKENthroughenv, and the only git fetch is a publiccargo install --git.${{ }}moved out ofrun:scripts into stepenv:(the version input, the version output, the NDK path), so a value is never parsed as shell.^v?[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$before anything uses it. All 146 existing tags pass. The check now runs right after checkout, so a bad value fails in seconds instead of after the SDK and NDK setup.server-idandsettings-pathfromsetup-java. The build is Gradle-only and never read that Mavensettings.xml; setup-java now writes its default one to~/.m2instead of the workspace.Other workflows:
android-bindgen.yml,ios-bindgen.yml:persist-credentials: falseon checkout, since these run build scripts from pull requests.android-bindgen.ymlalso passes the NDK path throughenv.claude-code-review.yml:persist-credentials: falseon checkout, so the checkout'sGITHUB_TOKENis no longer in.git/config. This does not leave the agent without a token:claude-code-actionsets its own token in theoriginremote URL before the agent runs, and the agent can already callgh api. Also uses$GITHUB_REPOSITORY, and passespr_numberthroughenvwith a check that it is a plain number.Behaviour is unchanged for valid inputs. This merges cleanly with #210, which pins the actions to commit SHAs.
Follow-ups, not in this PR:
packages: write.persist-credentials: falseon the remaining test workflows' checkouts (left out here to avoid conflicts with ci: pin workflow actions to commit SHAs and add Dependabot #210).claude.yml: its checkout still persists acontents: writetoken. Check that the action's own git auth covers its pushes, then addpersist-credentials: false.originremote URL andgh api).Preview
No UI change.
QA Notes
run:script contains a${{ }}expression.v0.8.0-rc15,0.8.0-swaps.11,v0.8.0-rc11.1,v1.0.0-rc-1, an empty input falling back to the tag, and all 146 existing tags pass; a trailing space, an embedded newline,v1.2.3-..and$(touch PWNED)are rejected and run nothing.pr_numbercheck was run locally withghstubbed:207passes,1; touch PWNEDand an empty value are rejected.gradle-publish.ymlandclaude-code-review.ymlonly run on release or manual dispatch, so they were checked statically; the next release exercises the publish job.