Skip to content

ci(#7034): lock CF worker dependencies - #7053

Merged
ifireball merged 1 commit into
mainfrom
codex/7034-fix-cf-worker-lockfile
Sep 7, 2026
Merged

ci(#7034): lock CF worker dependencies#7053
ifireball merged 1 commit into
mainfrom
codex/7034-fix-cf-worker-lockfile

Conversation

@ifireball

Copy link
Copy Markdown
Member

Summary

  • commit a package lock for the Cloudflare worker test dependencies
  • use npm ci so CI installs the locked dependency graph
  • enable setup-node caching for the worker lockfile

Verification

  • mise exec -- make mint-cf-worker-test
  • mise exec -- make lint

Closes #7034

Signed-off-by: Barak Korren <bkorren@redhat.com>
Assisted-by: Codex (gpt-5.6-sol)
@ifireball
ifireball requested a review from a team as a code owner September 6, 2026 06:53
@ifireball ifireball self-assigned this Sep 6, 2026
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Lock Cloudflare Worker test dependencies in CI

⚙️ Configuration changes 🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Commits Cloudflare Worker dependency resolutions for reproducible smoke-test installations.
• Replaces mutable npm installs with lockfile-enforced npm ci.
• Caches npm downloads in CI using the Worker lockfile.
Diagram

graph TD
  CI["CI workflow"] --> Setup["Node setup"] --> Cache["npm cache"]
  CI --> Target["Worker test target"] --> Install["npm ci"] --> Checks["Typecheck and tests"]
  Lock["Worker lockfile"] --> Setup
  Lock --> Install
Loading
High-Level Assessment

The committed package lock plus npm ci is the standard and most reliable approach for reproducible CI installations. Pinning only direct dependencies would leave transitive versions mutable, while using the repository-level lockfile would unnecessarily couple this independently scoped Worker package to unrelated dependencies.

Files changed (3) +2900 / -8

Other (3) +2900 / -8
mint-cf-worker-test.ymlEnable lockfile-based npm caching +2/-3

Enable lockfile-based npm caching

• Configures 'actions/setup-node' to cache npm downloads using the Cloudflare Worker package lock. This removes the previous no-lockfile limitation while preserving the existing Node 22 runtime.

.github/workflows/mint-cf-worker-test.yml

MakefileInstall Worker dependencies with npm ci +2/-5

Install Worker dependencies with npm ci

• Changes the Worker smoke-test target from 'npm install' to 'npm ci', enforcing the committed dependency graph on clean checkouts and in CI. The target documentation now reflects locked installation behavior.

Makefile

package-lock.jsonLock the Worker test dependency graph +2896/-0

Lock the Worker test dependency graph

• Adds an npm lockfile for the Worker package, recording exact direct, transitive, optional, and platform-specific development dependencies. It provides deterministic inputs for clean installs and CI cache invalidation.

internal/dispatch/cf/workersrc/package-lock.json

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:55 AM UTC · Completed 7:12 AM UTC

Commit: 7b57d2c · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.07

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 6, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

A targeted CI stabilization fix whose 2908-line change count is dominated by a generated package-lock.json; actual logic changes are minimal (5 lines in CI workflow and Makefile combined), with no security-sensitive paths, no first-time contributor, and a clear issue scope match — composite 2.24 rounds to moderate.

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Sep 6, 2026
@ifireball
ifireball added this pull request to the merge queue Sep 7, 2026
Merged via the queue into main with commit bccd9e8 Sep 7, 2026
57 checks passed
@ifireball
ifireball deleted the codex/7034-fix-cf-worker-lockfile branch September 7, 2026 13:34
@fullsend-ai-retro

fullsend-ai-retro Bot commented Sep 7, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 1:36 PM UTC · Completed 1:48 PM UTC

Commit: 7b57d2c · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.04

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #7053 — ci(#7034): lock CF worker dependencies

Timeline

Time (UTC) Event
2026-09-05 00:41 Issue #7034 opened — mint-cf-worker-test red on main due to npm install without lockfile
2026-09-05 00:43–00:47 Triage agent completed ($0.64) — correctly recommended lockfile + npm ci + workflow caching
2026-09-05 00:49–00:57 Code agent ran ($2.15) — implemented all three changes, but push was rejected because .github/workflows/mint-cf-worker-test.yml requires workflows permission the coder app lacks. Entire run wasted.
2026-09-06 06:53 Human (ifireball) manually created PR #7053 with the same fix (~30h after agent failure)
2026-09-06 06:55–07:12 Review agent ($4.07) — risk: moderate (2/5), correctly flagged protected-path change, no false positives
2026-09-07 12:50 Human (rh-hemartin) approved
2026-09-07 13:34 PR merged, issue closed

What went well

  • Excellent issue report. The reporter included full diagnosis, timeline, and suggested fix — the triage agent had high-quality input to work with.
  • Triage was fast and accurate — 4 minutes, $0.64, recommendations exactly matched the eventual human fix.
  • Review agent performed well — correctly assessed moderate risk (recognizing that 2900 of 2908 lines were a generated lockfile), flagged the protected-path change without false positives.
  • Clean resolution. The human PR was well-scoped, all CI checks passed, and the fix was exactly what triage recommended.

What could go better

  • Total failure mode wasted the code agent's work. The agent correctly implemented all three changes (Makefile, lockfile, workflow caching), but the entire push was rejected because one file was unpushable. The Makefile + lockfile changes alone would have fixed the core bug. $2.15 was wasted and the fix was delayed ~30 hours.
  • Post-script misclassified the error. The permission rejection was treated as a non-fast-forward error, triggering a futile --force-with-lease retry.

Existing issues providing coverage

Most themes from this workflow are already covered by open issues. New evidence from this retro:

Novel finding

One improvement is not covered by existing issues: the post-code script should attempt a partial push when the full push fails due to protected-path permissions, rather than failing entirely. See the proposal below.

Proposals filed

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

Labels

requires-manual-review Review requires human judgment risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mint-cf-worker-test is red on main: npm install fails with "Cannot read properties of null (reading 'edgesOut')"

2 participants