Skip to content

fix: correct two dead bun.lockb references (EXSC-771) - #2199

Open
0xDEnYO wants to merge 4 commits into
mainfrom
chore/exsc-771-ci-cache-bun-install
Open

fix: correct two dead bun.lockb references (EXSC-771)#2199
0xDEnYO wants to merge 4 commits into
mainfrom
chore/exsc-771-ci-cache-bun-install

Conversation

@0xDEnYO

@0xDEnYO 0xDEnYO commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Which Linear task belongs to this PR?

EXSC-771

Why did I implement it this way?

bun.lockb was removed in #1767 (2026-05-07). Two references outlived it; a repo-wide git grep bun.lockb now returns zero.

1. script/deploy/safe/ExecutePendingTimelockDockerfile:6COPY package.json bun.lockb ./

This one is hard-failing, not merely inert: docker build errors on a COPY whose source does not exist, and the next line is RUN bun install --frozen-lockfile --production, which would have no lockfile even if the COPY were tolerant. The file has no in-repo consumers (grep -rn ExecutePendingTimelock across *.yml/*.sh/*.ts/*.json → nothing; there is no docker build, compose file or Makefile target anywhere in the repo), so it is hand-built or driven externally and no CI check would ever surface this.

This fix removes the first of at least two blockers — the image still does not build. I ran docker build on both versions. Pre-fix it dies at step 3/9, "/bun.lockb": not found. Post-fix it gets past the COPY and dies one step later:

#8 [4/9] RUN bun install --frozen-lockfile --production
#8 19.72 gyp ERR! not ok
#8 19.74 error: install script from "usb" exited with 1

usb is a native addon pulled in transitively by @ledgerhq/hw-transport-node-hid (package.json:134) — a runtime dependency, so --production keeps it — and oven/bun:1-alpine carries neither Python nor a C toolchain to compile it. That blocker is orthogonal and pre-existing. Even a green install would not run: the Dockerfile copies four paths while the script imports many more local modules. Tracked in EXSC-773 (delete vs rewrite); out of scope here.

2. .github/workflows/deploy-smoke-test.yml:56 — path filter on bun.lockb

Silently dead — the entry matches no tracked path, so it could never fire. It was added in #1791 eight days after the lockfile was deleted, so it was never correct. validateScripts.yml:64 and ts-unit-tests.yml:55 already spell it bun.lock, so this aligns with both siblings rather than inventing a convention.

The real gap is narrower than "lockfile changes never triggered the smoke test": package.json is in the same filter list (:55), and a lockfile change usually rides along with one. What slipped through were bun.lock-only changes — transitive re-resolution, bunLockfileCheck.yml resyncs. The entry is load-bearing rather than decorative: :31 exports smoke: ${{ steps.filter.outputs.smoke }} and :70 gates the heavy job on it, while branch protection reads the smoke-test-required aggregator (:81+) which treats skipped as green — so a dead filter meant a silent false-green.

Scope note

This PR originally also switched verifyClearSigning.yml and syncLedgerClearSigning.yml from foundry-rs/foundry-toolchain to ./.github/actions/setup-foundry. That work is already done and approved in #2196 (@gvladika), which additionally unfreezes the sync workflow — so those commits were reverted here to avoid a conflicting duplicate. #2196 should land; this PR is deliberately narrowed to the bun.lockb cleanup, which #2196 does not touch.

Supporting evidence gathered for that change, in case it is useful to #2196's security review: foundry-rs/foundry-toolchain's version input defaults to "stable" at the pinned SHA, so both workflows were running a floating foundry against a repo pin of 1.7.1 — and regenerating config/clearSigningProposal.json under 1.7.1 reproduces the committed artifact byte-for-byte (88 entries, empty diff), verified in CI on this branch before the revert (cold cache, full 206-file recompile, verify-clear-signing green, log line Installing Foundry (version: 1.7.1)).

The third instance of that same violation, runPendingTimelockTXs.yml, is not covered by #2196 and is tracked in EXSC-772, along with a better fix for it: one runtime typechain import in a demo-script helper forces bun typechain (38s) on every 10-minute tick, and removing that coupling drops foundry from the funded-key job entirely rather than pinning it.

The leftover Dockerfile (still unbuildable after the lockfile rename, no in-repo consumer) is tracked in EXSC-773 and is not part of this PR.

What CI here does and does not prove

  • Not proven by CI: neither fix. deploy-smoke-test is skipped on drafts, and this PR does not touch bun.lock, so the corrected filter entry is never the matching rule. The Dockerfile has no CI consumer at all. Both are one-token corrections of references to a file that demonstrably does not exist (git ls-files | grep bun.lockbun.lock only), with two in-repo precedents for the spelling.
  • Proven locally, off CI: docker build was run on both the pre-fix and post-fix Dockerfile (results above). It confirms the COPY failure was real and that this change clears it — and also that the image still fails afterwards on the unrelated native-addon build. No .dockerignore exists in the repo, so nothing excludes bun.lock from the build context.

Checklist before requesting a review

  • I have performed a self-review of my code
  • This pull request is as small as possible and only tackles one problem — two references to the same removed lockfile
  • I have added tests that cover the functionality / test the bug — n/a, no test harness for a path filter or a Dockerfile COPY
  • For new facets: I have checked all points from this list: https://www.notion.so/lifi/New-Facet-Contract-Checklist-157f0ff14ac78095a2b8f999d655622e
  • I have updated any required documentation — none required; no header comment references the lockfile

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted — n/a, no contract changes
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted — n/a, no contract changes
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor> — n/a, no new contracts

0xDEnYO and others added 2 commits August 17, 2026 09:44
…ng workflows (EXSC-771)

Both workflows called foundry-rs/foundry-toolchain directly, whose version
input defaults to "stable". They therefore ran a floating foundry release
while the repo pins 1.7.1 in .foundry-version, so a foundry release could
change forge build output and flap the verify-clear-signing diff gate.

Regenerating config/clearSigningProposal.json with the pinned 1.7.1 locally
reproduces the committed artifact byte-for-byte (88 entries, empty diff).

Ref .agents/rules/500-github-actions.md [CONV:FOUNDRY-SETUP].

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The path filter referenced bun.lockb, which no longer exists — the repo
tracks bun.lock. Lockfile changes therefore never triggered the smoke test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f2acb362-0966-4d9f-88ec-b23517764064

📥 Commits

Reviewing files that changed from the base of the PR and between 0e537b5 and 086ee19.

📒 Files selected for processing (1)
  • script/deploy/safe/ExecutePendingTimelockDockerfile

Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review.


Walkthrough

The pull request updates workflow and Dockerfile references from bun.lockb to bun.lock. Two signing workflows now install Foundry through the repository-local setup action.

Changes

Workflow and build configuration updates

Layer / File(s) Summary
Toolchain and lockfile integration
.github/workflows/deploy-smoke-test.yml, .github/workflows/syncLedgerClearSigning.yml, .github/workflows/verifyClearSigning.yml, script/deploy/safe/ExecutePendingTimelockDockerfile
The smoke-test workflow and deployment Dockerfile now use bun.lock. The signing workflows now use the repository-local setup-foundry action.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 086ee

The PR makes localized CI configuration updates, and no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

Suggested labels: AuditNotRequired

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the two obsolete bun.lockb references, which are a substantial part of the changes.
Description check ✅ Passed The description includes all required sections, task details, rationale, scope, validation, and completed checklists.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/exsc-771-ci-cache-bun-install

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…e (EXSC-771)

The COPY referenced bun.lockb, removed in #1767. Docker errors on a COPY
whose source is absent, so the image could not build at all; the file has
no in-repo consumers, so no CI check surfaced it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@0xDEnYO

0xDEnYO commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Review-gate residual findings (not fixed here — need a decision)

Auto-fixed during the gate: completed the bun.lockb cleanup (ExecutePendingTimelockDockerfile:6, was hard-failing docker build), added the measured cache-footprint accounting and the EXSC-772 reference to the description. These remain open:

1. syncLedgerClearSigning.yml:72 — this half of the diff has zero CI evidence. (major)
The workflow is workflow_dispatch-only (schedule/push commented out under the EXSC-738 freeze), so nothing here exercises the edited step. The artifact question is settled (regeneration under pinned 1.7.1 is byte-identical, and verify-clear-signing passed on this PR), so this is about job mechanics only — specifically whether forge lands on PATH for tasks/generateLedgerClearSigning.ts. The workflow has a dry_run input built for exactly this, and the dry-run path stops before commit/push/PR/Slack:

gh workflow run syncLedgerClearSigning.yml --ref chore/exsc-771-ci-cache-bun-install -f dry_run=true

Not run unilaterally because the job checks out the external Ledger registry fork with a sync token. Requesting a maintainer dispatch it, or explicit sign-off to proceed without it.

2. Unsuffixed fork-RPC cache key. (minor, pre-existing pattern)
Both workflows now write foundry-rpc-Linux--<forkPinHash> with no cache-key-suffix and rpc-cache-refresh defaulting false. Today that key is an exact hit so nothing is saved. On the next fork-block-pin change the key goes cold, and if verify-clear-signing (which never forks) wins the race it saves a near-empty entry that can then never be refreshed — the failure mode #2151 addressed. Shared with the 9 other unsuffixed callers, so a consistency question rather than a regression; the fix if wanted is a cache-key-suffix.

3. verifyClearSigning.yml paths: does not include the composite it now depends on. (minor)
A change to .github/actions/setup-foundry/** or .foundry-version — including a foundry bump — will not re-run this clear-signing gate. Covered incidentally on this PR only because the workflow file itself changed. No workflow in the repo does this, so it is a repo-wide gap worth its own ticket rather than a blocker.

4. tasks/buildClearSigningProposal.ts:128 fails open where its sibling fails closed. (minor, pre-existing)
It continues on a missing out/ artifact, while tasks/generateLedgerClearSigning.ts:228 throws on the same condition. In verifyClearSigning under-generation surfaces loudly as a non-empty diff; syncLedgerClearSigning has no diff gate and publishes upstream. Exposure is small (per-workflow cache namespacing, identical build commands), but the two generators reading the same out/ should agree.

Procedural: protectSecurityRelevantCode.yml will require an InformationSecurityManager approval because ^\.github/ files changed. It triggers on pull_request_review: submitted and only when not draft, so it produces no check run until the first review lands on a non-draft PR. smart-contract-core approval is separately required.

…ar-signing workflows (EXSC-771)"

This reverts commit 0efb7a1.
@0xDEnYO 0xDEnYO changed the title ci: pin foundry via setup-foundry in clear-signing workflows + fix dead bun.lockb filter (EXSC-771) fix: correct two dead bun.lockb references (EXSC-771) Aug 17, 2026
@0xDEnYO
0xDEnYO marked this pull request as ready for review August 17, 2026 03:49
@0xDEnYO

0xDEnYO commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Scope narrowed: the foundry-pin commits were reverted here because #2196 (approved) already covers those two workflows. The residual findings in my earlier comment moved with them — raised on #2196 instead, along with the CI verification of the pin. This PR is now only the two dead bun.lockb references.

@0xDEnYO

0xDEnYO commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Review-gate residual findings — round 2 (supersedes the comment above)

The earlier residual comment on this PR is stale: items 1–4 there all concerned verifyClearSigning.yml / syncLedgerClearSigning.yml / buildClearSigningProposal.ts, which were reverted out of this branch (see the Scope note). None of them apply to the current two-line diff. The procedural note at its end still holds: protectSecurityRelevantCode.yml will require an InformationSecurityManager approval because ^\.github/ files changed, and it only produces a check run once the first review lands on a non-draft PR.

This round re-gated the narrowed diff. The code is clean — no changes were made to it. Two claims in the description were corrected (see the edited body), and one finding is escalated:

1. The Dockerfile still cannot build after this fix. (major, pre-existing and orthogonal — not introduced here)
docker build was run for real on both versions. Pre-fix it dies at step 3/9 on "/bun.lockb": not found; post-fix it clears the COPY and dies at step 4/9:

#8 [4/9] RUN bun install --frozen-lockfile --production
#8 19.72 gyp ERR! not ok
#8 19.74 error: install script from "usb" exited with 1

usb is a native addon reached transitively via @ledgerhq/hw-transport-node-hid (package.json:134). It is a runtime dependency, so --production does not drop it, and oven/bun:1-alpine ships neither Python nor a C toolchain to compile it. Three defensible fixes exist — add python3 make g++ to the image, install with --ignore-scripts, or keep the Ledger transport out of this image's dependency set — and they differ in whether the executor can still talk to a Ledger at runtime. That is a behavioural call, so the gate did not pick one.

Because the file has no in-repo consumer (grep -rn ExecutePendingTimelock → nothing; no docker build, compose file or Makefile target anywhere in the repo), this is invisible to CI either way. If an external ops system does build it by path, it is broken today and stays broken after this PR.

Decision needed: merge this as the scoped lockfile cleanup and track the usb/node-gyp blocker in a follow-up ticket, or widen this PR to make the image actually buildable. Recommending the former — the two changes have different blast radii and the second one needs an owner who knows whether that image is still used at all.

Not blocking, for the record: CodeRabbit reported no actionable comments. The revert is complete — git diff --stat origin/main...HEAD is exactly the two one-line hunks, with no residue from the reverted setup-foundry work.

@0xDEnYO

0xDEnYO commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

The usb/node-gyp Dockerfile leftover is now a ticket, not a decision on this PR.

Tracked in EXSC-773: delete the unused image, or rewrite it so it actually runs. Out of scope here — this PR only corrects the dead bun.lockb COPY.

The round-2 residual comment above is closed by that ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants