build(deps): Bump go-task to 3.48.0 (fixes #1352); Fix webui dependency hoisting conflicts.#1994
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpgrades the Go Task CLI to >= 3.48.0 in multiple CI workflows and docs, replaces root taskfile references with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai Why can't I link issue #1352 to PR development? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 494 🏁 Script executed: Length of output: 1998
Your PR title includes "fixes Solutions:Option 1: Update the PR description (recommended) This will automatically link the issue and close it when the PR is merged. Option 2: Manual UI linking If you're experiencing issues with the UI's manual linking feature (e.g., the issue doesn't appear in the search or the link doesn't save), it could be due to:
Would you like me to help update the PR body to include the linking keyword? 🧠 Learnings used |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@taskfile.yaml`:
- Around line 354-358: Delete the now-orphaned checksum variables
G_WEBUI_CLIENT_NODE_MODULES_CHECKSUM_FILE and
G_WEBUI_COMMON_NODE_MODULES_CHECKSUM_FILE (they’re no longer produced by
webui-node-modules) and remove those two symbols from the sources list of the
webui task so the webui task no longer references non-existent checksum files;
locate the variable declarations and the webui task's sources block and remove
the two entries.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfiles/lint.yaml (1)
889-889:⚠️ Potential issue | 🟡 MinorInconsistent:
venvtask still references{{.ROOT_DIR}}/taskfile.yamlinstead of{{.ROOT_TASKFILE}}.Every other task across the codebase has been migrated to
{{.ROOT_TASKFILE}}, but thisvenvtask's sources list was missed.Proposed fix
- - "{{.ROOT_DIR}}/taskfile.yaml" + - "{{.ROOT_TASKFILE}}"
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
shall we fix this as well?
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
with the webui lock file fixed, i believe we can further remove the webui server node_modules checksum mechanism. that said, the issue is not strictly related to the go-task upgrade. shall we apply the suggestions and then split the changes into a separate PR?
Checksum used to run on these non-existent node modules directory. Task v3.44 lets it go but v3.48 catches these errors. See the failed workflow: https://github.com/y-scope/clp/actions/runs/22021684927/job/63631740536 |
Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@taskfile.yaml`:
- Around line 350-353: Update the comment that currently states "two different
node_modules directories" to accurately list all three directories used by the
tasks: LOG_VIEWER_OUTPUT_DIR, PACKAGE_OUTPUT_DIR, and SERVER_OUTPUT_DIR; modify
the paragraph to say there are three directories and mention the corresponding
checksum files and validate/compute pairs are generated for each of those three
output dirs so the comment matches the actual logic in the task definitions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
for the title, how about:
build(deps): Bump go-task to 3.48.0 (fixes #1352); Fix webui dependency hoisting conflicts.
| if ((0 == "${package_preinstalled}")); then | ||
| echo "Please uninstall Task and then re-run the install script." | ||
| else | ||
| echo "pipx failed to install the required version of Task." | ||
| echo "To uninstall, run:" | ||
| echo " pipx uninstall go-task-bin" | ||
| fi |
There was a problem hiding this comment.
| if "${package_preinstalled}"; then | |
| echo "Please uninstall Task and then re-run the install script." | |
| else | |
| echo "pipx failed to install the required version of Task." | |
| fi | |
| echo "To uninstall, run:" | |
| echo " pipx uninstall go-task-bin" |
There was a problem hiding this comment.
We will be removing package_preinstalled in #1320
| package_preinstalled=0 | ||
| if ! command -v task >/dev/null 2>&1; then | ||
| package_preinstalled=1 |
There was a problem hiding this comment.
| package_preinstalled=0 | |
| if ! command -v task >/dev/null 2>&1; then | |
| package_preinstalled=1 | |
| package_preinstalled=false | |
| if command -v task >/dev/null 2>&1; then | |
| package_preinstalled=true | |
| else |
There was a problem hiding this comment.
We will be removing package_preinstalled in #1320
| PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm clean-install | ||
| # Test that node_modules have been deduplicated | ||
| - for: ["client", "common", "server"] | ||
| cmd: "test ! -d '{{.G_WEBUI_SRC_DIR}}/{{.ITEM}}/node_modules'" |
There was a problem hiding this comment.
Could we add a note to the PR description for developers who have previously built the webui? They may have stale node_modules directories that will cause the new hoisting assertions to fail. Something like the warning block above with the rm -rf commands should help them resolve it quickly.
> [!WARNING]
> If you previously compiled the webui, stale node_modules directories may remain in `components/webui/client/` , `components/webui/common/`, or components/webui/server/. These will cause the new hoisting assertions to fail with a non-zero exit code.
> To resolve, remove them manually:
>
> ```sh
> rm -rf components/webui/client/node_modules
> rm -rf components/webui/common/node_modules
> rm -rf components/webui/server/node_modules
> ```
>
> Then re-run the build as usual.
if the code block is removed, it should be rendered like
Warning
If you previously compiled the webui, stale node_modules directories may remain in components/webui/client/ , components/webui/common/, or components/webui/server/. These will cause the new hoisting assertions to fail with a non-zero exit code.
To resolve, remove them manually:
rm -rf components/webui/client/node_modules
rm -rf components/webui/common/node_modules
rm -rf components/webui/server/node_modulesThen re-run the build as usual.
There was a problem hiding this comment.
I restored the clean-webui task to clean all 5 possible node_modules directories. I think the long comment above the test ! d will give enough context.
There was a problem hiding this comment.
sounds reasonable. in the future, i feel this check shall be moved into another dep:lock task to be run in a CI workflow, and we won't have to do such assertions at project build time
@coderabbitai create an feature request issue to track
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a feature request issue to track moving webui node_modules hoisting checks to CI
gh issue create \
--title "Move webui node_modules hoisting checks to CI workflow" \
--body "## Context
Currently, the \`webui-node-modules\` task in \`taskfile.yaml\` performs hoisting assertions at build time using \`test ! -d\` checks to verify that npm has fully hoisted all workspace packages to the root node_modules directory (lines 382-383):
\`\`\`yaml
- for: [\"client\", \"common\", \"server\"]
cmd: \"test ! -d '{{.G_WEBUI_SRC_DIR}}/{{.ITEM}}/node_modules'\"
\`\`\`
These assertions ensure that peer dependency conflicts haven't forced npm to create per-workspace node_modules directories.
## Proposed Enhancement
Move these hoisting verification checks into a separate \`dep:lock\` task that runs in a CI workflow, similar to the existing Rust Cargo.lock verification (see \`deps:lock:check-rust\` task and the \`clp-rust-checks\` workflow).
## Benefits
- Separates deterministic dependency validation from project build steps
- Aligns with the existing pattern established for Rust dependency lock verification
- Keeps CI workflows focused on validation tasks
- Avoids unnecessary checks during local development builds
## References
- PR `#1994`: https://github.com/y-scope/clp/pull/1994
- Original discussion: https://github.com/y-scope/clp/pull/1994#discussion_r2815461745
- Requested by: `@junhaoliao`" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created issue #2002 to track moving the webui node_modules hoisting checks into a separate CI workflow task, similar to the existing Rust dependency lock verification pattern. The issue is assigned to you and includes backlinks to this PR and discussion.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1119
File: taskfile.yaml:429-433
Timestamp: 2025-08-14T18:23:39.476Z
Learning: In the webui workspace, the exclude patterns ".tmp" and ".vite-temp" are Vite-specific temporary directories that only apply to the client package. The common and server packages use TypeScript compiler (tsc) directly and don't generate these Vite temporary files, so these exclude patterns should not be applied to their node_modules checksum computations.
Learnt from: jackluo923
Repo: y-scope/clp PR: 1996
File: components/core/tools/packaging/build.sh:86-94
Timestamp: 2026-02-15T04:15:22.260Z
Learning: In CLP build scripts (e.g., components/core/tools/packaging/build.sh), prefer simple grep/sed pipelines over adding dependencies like yq for single-use version extraction from taskfile.yaml. The existing error handling catches extraction failures, and the format is stable enough to not warrant additional tooling.
Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR `#1158`.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.464Z
Learning: Task v3.44.1 has a regression that breaks shell command processing, particularly rsync commands with brace expansion (e.g., `file.{d.ts,js,wasm}`). This causes CI failures in clp-ffi-js project (issue `#110`), so CLP should avoid v3.44.1 and use v3.44.0 instead, which fixes the dynamic variable bug without the shell processing regression.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.464Z
Learning: Task v3.44.1 causes issues in the clp-ffi-js project (issue `#110`), so CLP should avoid upgrading to v3.44.1 and stick with v3.44.0 for now.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR `#1405`) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
Bill-hbrhbr
changed the title
3.44.0 to 3.48.0 (fixes #1352).…l-go-task.sh Co-authored-by: Junhao Liao <junhao@junhao.ca>
…ependency hoisting conflicts. (y-scope#1994) Co-authored-by: Junhao Liao <junhao@junhao.ca>
2 participants
node_modulesreferences from webui-related tasks.{{.ROOT_DIR}}/taskfile.yamlwithROOT_TASKFILE.Description
The upstream issues mentioned in #1352 have been resolved. It's now safe to remove the go-task version pin.
Fixes #1352
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Chores
Documentation