chore(ci): apply review fixes on top of #3999#4015
Merged
lrsaturnino merged 11 commits intoJun 6, 2026
Merged
Conversation
setup-git-for-yarn previously echoed GIT_CONFIG_GLOBAL=/dev/null (and HOME, XDG_CONFIG_HOME, GIT_CONFIG_NOSYSTEM, GIT_CONFIG_SYSTEM, GIT_CONFIG_COUNT) into $GITHUB_ENV. That propagates to every subsequent step in the job, breaking any downstream "git config --global ..." call: the write lands in /dev/null and the read returns empty. The reusable Solidity docs workflow exercises this path: it calls "git config --global user.email/name" after this action runs, then "git commit -S". Both fail when the global config target is /dev/null. Move the GIT_CONFIG_* sanitisation entirely inside the wrapper script (it already unsets them and exports clean values per invocation, scoped to the git child process). The job env retains only GIT and npm_config_git, which point downstream tooling at the wrapper without poisoning git's config resolution for unrelated steps.
The PR introduced setup-git-for-yarn to centralise git-wrapping for Corepack/Yarn installs, but each workflow still duplicated a ~22-line shell block that exported PATH, scrubbed GIT_CONFIG_*, ran corepack, and invoked yarn install --immutable. That block appeared 13 times across four workflows. Move the install incantation into .github/actions/install-yarn-deps, which takes a working-directory input and also runs setup-git-for-yarn internally so callers do not need a separate step. Each duplicated block collapses to a single `uses:` reference. npm-random-beacon.yml is intentionally left alone in this commit: it runs `yarn upgrade` rather than `yarn install --immutable`, so it does not match the install incantation pattern. Net diff: -351 / +42 across four workflows.
The PR removed setup-node's "cache: yarn" / cache-dependency-path directives across every workflow without a replacement. Yarn 4 stores its content-addressable cache under .yarn/cache (gitignored), so every CI job was re-downloading the full dependency set on every run. Add actions/cache@v4 to the install-yarn-deps composite action, keyed on the workspace's yarn.lock hash. Cache covers .yarn/cache, .yarn/install-state.gz, and node_modules (the workspace uses the node-modules linker). A restore-keys fallback prevents cold-start runs from blocking on a cache miss.
YARN_ENABLE_HARDENED_MODE=0 and YARN_CHECKSUM_BEHAVIOR=ignore turn off Yarn 4's lockfile-integrity verification (poisoned-package detection, checksum mismatch detection). Migrating to Yarn 4 specifically to disable its safety features defeats the supply-chain rationale for the upgrade. The lockfile already carries integrity hashes for the codeload tarballs; ignoring them silences the alarm that would catch real tampering. Drop both env vars from install-yarn-deps and from the previously-dead per-step exports in npm-random-beacon.yml's "Enable Yarn 4" step. If a future "yarn install --immutable" fails with a checksum mismatch, that is a real signal to regenerate the lockfile, not to silence the check.
The reusable Solidity docs workflow used \`hub pull-request\` to open a PR against the destination docs repo. The hub CLI is unmaintained and was removed from ubuntu-22.04 runner images in 2024; it is not present on ubuntu-24.04 images at all. The publish path would fail at runtime the moment GitHub rolls the runner image forward. gh is preinstalled on every hosted runner and is the modern replacement.
The clone step embedded \$GITHUB_TOKEN directly into the HTTPS URL. That token ends up persisted in remote.origin.url inside the cloned repo's .git/config and is visible in any \`git remote -v\` output or process listing during the clone. GitHub Actions redacts known secret strings from job logs, but the token still lands on disk on the runner and in process arguments. Configure gh as a git credential helper for github.com via \`gh auth setup-git\`. gh reads GH_TOKEN from the environment and supplies credentials on demand, so the clone URL stays token-free and the cloned repo's .git/config does not retain credentials.
actions/upload-artifact@master is unpinned: any change pushed to the
action's master branch ships immediately into every CI run, with no
review. Pin to v4 (current major), which also matches GitHub's
recommended supply-chain hygiene for third-party actions.
The two upload-artifact callsites use distinct artifact names
("contracts-after-preprocessing" and "contracts-final-output"), so the
v4 breaking change around per-name uniqueness does not apply.
This file is newly introduced by this PR (previously sourced from keep-network/ci@main). Land it on current major versions of the GitHub-maintained actions rather than the deprecated v3 line. Scope deliberately limited to the new file: pre-existing v3 usage in contracts-*.yml / npm-*.yml is left untouched in this commit to keep the change surgical.
CI surfaced that Yarn 4 auto-enables hardened mode on public PR
contexts ("workflow is executed from a public pull request"), and
ECDSA's lockfile contains a legitimate npm-descriptor -> git-URL remap
that hardened mode rejects:
YN0078: Invalid resolution ethereumjs-abi@npm:0.6.8 ->
https://github.com/ethereumjs/ethereumjs-abi.git#commit=ee...
The remap exists because ethereumjs-abi@0.6.8 was published broken on
npm; the canonical fix is the git pin, and that is what the lockfile
encodes. Hardened mode considers all npm->git remaps a supply-chain
risk, with no per-entry opt-in.
Restore YARN_ENABLE_HARDENED_MODE=0 with a comment explaining the
reason. Keep YARN_CHECKSUM_BEHAVIOR at its default (the previous
\`=ignore\` was unnecessary; per-package integrity hashes are still
enforced).
Verified locally: \`yarn install --immutable\` succeeds in
solidity/ecdsa with this configuration, fails with YN0078 without it.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Both Solidity workspaces resolved @threshold-network/solidity-contracts
via the floating "development" dist-tag, which has advanced to
1.3.0-dev.15+ and dropped a set of legacy TokenStaking methods that the
test and deployment code still depend on:
- test/utils/operators.ts: TokenStaking.stake, .increaseAuthorization
- test/fixtures/index.ts: TokenStaking.pushNotificationReward,
.setNotificationReward
- deploy/*_approve_*.ts: TokenStaking.approveApplication
Symptoms in CI:
TS2551: Property 'stake' does not exist on type 'TokenStaking'
TS2339: Property 'increaseAuthorization' does not exist on type ...
TS2339: Property 'pushNotificationReward' does not exist on type ...
TS2339: Property 'setNotificationReward' does not exist on type ...
Error: No method named "approveApplication" on contract deployed
as "TokenStaking"
1.3.0-dev.14 is the last published version that still ships all five
methods (verified by inspecting TokenStaking.sol in each published
tarball). Pin both workspaces to that exact version so CI is
deterministic regardless of future dist-tag movement.
Lockfiles regenerated. Verified locally: yarn install --immutable
succeeds, hardhat compile + typechain + test compile pass without the
TS2339/TS2551 errors above.
The docs-publish path in reusable-solidity-docs.yml runs its git operations through the Yarn git wrapper that setup-git-for-yarn installs on $PATH and exports as $GIT/$npm_config_git for the whole job. The wrapper forces GIT_CONFIG_GLOBAL=/dev/null with a throwaway HOME on every git invocation, so the docs-sync step fails: gh auth setup-git and the git config --global user.email/name calls cannot write the global config ("could not lock config file /dev/null"), the credential helper is never registered, and the authenticated git push has no credentials. The earlier token-in-URL approach survived the wrapper because the credentials lived in the URL rather than git config; moving to the credential helper made the path secure but non-functional.
The wrapper is only needed so yarn install can clone git dependencies, which has already happened by the time docs are generated. Add a publish-only step that removes the wrapper binary and repoints $GIT/$npm_config_git at the real git for the remaining publishing steps, restoring the intended credential-helper and tokenless-URL design with no changes to the existing steps.
This path is release-gated (publish == true on a solidity/* release tag) and is not exercised by normal PR or preview runs, so it was verified by static analysis and a local reproduction of the /dev/null config-lock failure rather than an end-to-end release run.
lrsaturnino
force-pushed
the
stack/testnet4-01-ci-yarn-review-fixes
branch
from
1208fe2 to
a20e181
Compare
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.
Summary
Follow-up to #3999. Applies the multi-agent / validate-findings review
recommendations that were confirmed valid on top of that PR's Yarn 4
migration. Keeps #3999 surgical and lets each fix stand on its own.
Commits
fix(ci): scope GIT_CONFIG vars to wrapper subshell only(C1)setup-git-for-yarnwas exportingGIT_CONFIG_GLOBAL=/dev/nullto$GITHUB_ENV, breaking downstreamgit config --globalcalls (e.g.docs publish
user.email/name). Scope GIT_CONFIG vars inside thewrapper subshell only.
refactor(ci): extract Yarn install into composite action(S1)~22-line install bash was duplicated 13 times across workflows.
New
.github/actions/install-yarn-depsaction takesworking-directoryand centralises the install incantation. Net diff: -351 / +42.
ci: restore Yarn dependency caching via actions/cache(C4)cache: "yarn"was removed fromsetup-nodewith no replacement,forcing every CI job to re-download deps. Add
actions/cache@v4keyed on yarn.lock for
.yarn/cache,.yarn/install-state.gz,and
node_modules.fix(ci): re-enable Yarn lockfile integrity checks+fix(ci): keep HARDENED_MODE disabled, documented(Sec1)Removed
YARN_CHECKSUM_BEHAVIOR=ignore(lockfile checksums verifiedlocally as consistent). Kept
YARN_ENABLE_HARDENED_MODE=0with adocumented reason: Yarn 4 auto-enables hardened mode in public PR
context, and ECDSA's lockfile contains a legitimate npm-to-git remap
for
ethereumjs-abi(the npm 0.6.8 release was published broken;the git pin is canonical). Hardened mode rejects all such remaps
with
YN0078, regardless of intent.fix(workflows): replace unmaintained hub with gh pr create(C6)hubwas removed fromubuntu-22.04runner images in 2024 and isnot present on
ubuntu-24.04. Replace with the preinstalledgh.fix(workflows): use gh credential helper instead of token-in-URL(Sec3)git clone https://user:$TOKEN@github.com/...persisted the tokenin
remote.origin.url. Configuregh auth setup-gitinstead;token stays in env, out of
.git/config.ci(workflows): pin upload-artifact to v4(Sec4)actions/upload-artifact@masterwas unpinned. Pin to@v4.ci(workflows): bump checkout and setup-node to v4 in new docs workflow(C5)Surgical version bump on the newly-introduced
reusable-solidity-docs.ymlonly; pre-existing v3 usage elsewhereleft alone.
Notes
build-and-test/deployment-dry-runand ECDSAbuild-and-testfailures with errors likeProperty 'pushNotificationReward' does not exist on type 'TokenStaking'and
No method named "approveApplication"are pre-existing inchore(ci): align Yarn 4 workflows and solidity workspace tooling #3999 (verified against its prior commit's run log). They are not
introduced by this PR and are out of scope here.
setup-nodeis left on v3 incontracts-*.ymlandnpm-*.ymltomatch repo convention. Bumping there can be a follow-up.
Test plan
normal PRs unless
publish == true); verify by inspection