Skip to content

Conversation

junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Aug 20, 2025

Description

As the title suggests. See #872 (comment) for the details of the "bug".

We previously locked Task to versions < 3.43.0 due to critical issues that caused build failures and hangs. Two specific issues were identified:

  1. task package hangs with task >= 3.43.1 #872 (reported to Dynamic variables in sources cause glob expansion to hang due to unpopulated placeholders and symlink loops go-task/task#2400): Dynamic variables in sources caused glob expansion to hang due to unpopulated placeholders and symlink loops. This was particularly problematic with patterns like {{.DYNAMIC_VARIABLE}}/**/* which would expand to /**/* and cause infinite symlink traversal.
  2. CI failure: go-task modifies rsync commands with path globbing causing file not found errors clp-ffi-js#110 (reported to Brace expansion broken in v3.44.1 due to over-escaping of curly braces go-task/task#2401): Brace expansion was broken in v3.44.1 due to over-escaping of curly braces. The escape function introduced in commit go-task/task@d2bd834 incorrectly escaped { and } characters, breaking legitimate shell features like file.{txt,md} expansion.

This PR makes modification to the taskfile.yaml, upgrades the Task tool to version 3.44.0 and removes the previous version constraints that were in place due to critical bugs introduced in Task v3.43.0. The upgrade to 3.44.0 brings in various benefits such as unblocking use cases the leverage globing in sources files: #1119 (comment)

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. Downloaded latest release (3.44.1) of go-task.
  2. which task and task --version just to be sure the version was to be used.
  3. in the project root: rm -rf build .task
  4. in the project root: task and observed the command to be completed in less than 3 minutes.

Summary by CodeRabbit

  • Chores
    • Standardized build tooling to Task 3.44.0 across Ubuntu, CentOS Stream 9, Manylinux 2.28, and MuslLinux 1.2 install scripts.
    • Simplified task sources configuration by broadening file matching and excluding build artefacts, removing redundant variables.
  • Documentation
    • Updated requirements to specify Task 3.44.0 across README and developer docs.
    • Replaced prior version ranges and related notes with a single fixed version reference.

@junhaoliao junhaoliao requested a review from a team as a code owner August 20, 2025 05:25
Copy link
Contributor

coderabbitai bot commented Aug 20, 2025

Walkthrough

Task tooling is pinned to v3.44.0 across install scripts and documentation. The Taskfile’s python-component task removes the PACKAGE var and switches sources from per-component paths to a repo-wide glob with a dist exclusion. No other control flow or public API changes.

Changes

Cohort / File(s) Summary
Task install scripts updated to v3.44.0
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh, components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh, components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh, components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh
Pin go-task to 3.44.0; update download URLs and comments; logic unchanged.
Task version in docs
docs/README.md, docs/src/dev-docs/building-package.md, docs/src/dev-docs/components-core/index.md, docs/src/dev-docs/contributing-linting.md
Replace prior range/version notes with exact Task 3.44.0 requirement.
Taskfile python-component sources
taskfile.yaml
Remove PACKAGE var; change sources from {{.PACKAGE}}/**/* and pyproject.toml to **/* with exclude: "dist/**/*".

Sequence Diagram(s)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • kirkrodrigues
  • davidlion
  • haiqi96
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

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

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/src/dev-guide/building-package.md (1)

71-71: Remove unused link reference (leftover from removed note).

The reference [clp-issue-872] is no longer used in the page after removing the note; leaving it may confuse readers.

Apply this diff to remove the unused reference:

-[clp-issue-872]: https://github.com/y-scope/clp/issues/872
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d63d8cb and 3fb56ae.

📒 Files selected for processing (2)
  • docs/src/dev-guide/building-package.md (1 hunks)
  • taskfile.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#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.
📚 Learning: 2025-08-04T18:38:33.130Z
Learnt from: haiqi96
PR: y-scope/clp#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.

Applied to files:

  • docs/src/dev-guide/building-package.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: ubuntu-jammy-lint
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: antlr-code-committed (macos-15)
  • GitHub Check: build-macos (macos-15, false)
  • GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (1)
taskfile.yaml (1)

545-547: Map-style exclude support confirmed in Task ≥ 3.32.0 (including v3.40.0) – no action needed

Task has supported map-style exclude entries since v3.32.0 and there are no breaking changes in later 3.x releases. Your current configuration is valid as-is.

Comment on lines +545 to 547
- "**/*"
- exclude: "dist/**/*"
dir: "components/{{.COMPONENT}}"
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Exclude common cache/virtualenv dirs to prevent unnecessary rebuilds.

Broadening sources to "**/*" fixes the dynamic variable bug, but it also makes the task sensitive to transient files (e.g., virtualenvs, caches) within the component directory. Excluding these reduces spurious cache invalidations and speeds up task runs without changing behaviour.

Apply this diff to harden the excludes:

       - "**/*"
-      - exclude: "dist/**/*"
+      - exclude: "dist/**/*"
+      - exclude: ".venv/**/*"
+      - exclude: "**/__pycache__/**/*"
+      - exclude: "**/.pytest_cache/**/*"
+      - exclude: "**/.mypy_cache/**/*"
+      - exclude: "**/.ruff_cache/**/*"
+      - exclude: "**/.tox/**/*"
+      - exclude: "**/node_modules/**/*"
🤖 Prompt for AI Agents
In taskfile.yaml around lines 545 to 547, the source pattern was broadened to
"**/*" but lacks excludes for common transient cache/virtualenv dirs, causing
spurious rebuilds; update the excludes under that block to add patterns such as
"venv/**/*", ".venv/**/*", "__pycache__/**/*", ".cache/**/*",
".mypy_cache/**/*", ".pytest_cache/**/*", "node_modules/**/*" and optionally
"*.pyc" (or other language-specific cache files) so the task ignores virtual
environments and cache directories and prevents unnecessary cache invalidations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing you listed should exist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@junhaoliao I apologize for making assumptions about your directory structure. Let me check what actually exists in the python component directories to better understand the codebase.


🏁 Script executed:

#!/bin/bash
# Check the actual structure of python component directories
echo "=== Python component directories structure ==="
find components -maxdepth 3 -type d | grep -E "(clp-package-utils|clp-py-utils|job-orchestration)" | head -20

echo -e "\n=== Contents of a sample python component directory ==="
ls -la components/clp-package-utils/ 2>/dev/null || echo "Directory not found"

echo -e "\n=== Hidden files and directories in python components ==="
find components -name ".*" -type d | head -10

echo -e "\n=== Check for common cache directories ==="
find components -name "__pycache__" -o -name ".venv" -o -name "venv" -o -name ".mypy_cache" -o -name ".pytest_cache" -o -name ".ruff_cache" -o -name ".tox" -o -name "node_modules" 2>/dev/null | head -10

Length of output: 1707


You're absolutely right! Thank you for the clarification. Looking at the actual component structure, I can see that your python component directories are kept very clean with only:

  • Source code directories
  • pyproject.toml and poetry.lock
  • README.md
  • The dist/ directory (which you're already excluding)

No cache directories, virtual environments, or other transient files exist in the component directories. The current exclude pattern "dist/**/*" is perfectly sufficient for your codebase structure. I apologize for making assumptions based on typical Python project layouts rather than understanding your specific, well-organized directory structure.

Your simplified solution with "**/*" and exclude: "dist/**/*" is exactly what's needed to fix the go-task dynamic variable bug without any unnecessary complexity.


✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.705Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#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.

@junhaoliao junhaoliao requested a review from hoophalab August 20, 2025 05:34
Copy link
Contributor

@hoophalab hoophalab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Validation: Build clp with task version 3.43.3. It no longer hangs.

For the title, how about:
fix(taskfiles): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872).

hoophalab
hoophalab previously approved these changes Aug 21, 2025
@junhaoliao junhaoliao changed the title fix(taskfile): Simplify python-component sources list to avoid dynamic variable bug in go-task (fixes #872). fix(taskfile): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872). Aug 21, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3fb56ae and f1f6303.

📒 Files selected for processing (1)
  • taskfile.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#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.
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.

Applied to files:

  • taskfile.yaml
📚 Learning: 2025-08-14T18:23:39.476Z
Learnt from: junhaoliao
PR: y-scope/clp#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.

Applied to files:

  • taskfile.yaml
🔇 Additional comments (2)
taskfile.yaml (2)

545-547: LGTM: Simplified python-component sources fix the go-task dynamic var pitfall without over-excluding.

Using "/*" with an explicit "dist//*" exclude is a clean, robust way to avoid the dynamic variable bug. Given your repository norms (no caches/venvs inside component dirs per prior clarification), keeping excludes minimal is correct. Also aligns with shopt: ["globstar"] at the top.


545-547: Confirm Task version support for exclude:

The exclude: field on individual entries within the sources: list is supported as of go-task v3.32.0 (released 2023-11-29). Therefore, using exclude: in v3.40.0 is fully valid, and the stated minimum requirement (Task >= 3.40.0) need not be bumped.

@junhaoliao
Copy link
Member Author

junhaoliao commented Aug 22, 2025

we should also update

# Install `task`
# NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872
task_pkg_arch=$(dpkg --print-architecture)
task_pkg_path="$(mktemp -t --suffix ".deb" task-pkg.XXXXXXXXXX)"
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.deb"
dpkg --install "$task_pkg_path"
rm "$task_pkg_path"

@junhaoliao
Copy link
Member Author

we should also update

# Install `task`
# NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872
task_pkg_arch=$(dpkg --print-architecture)
task_pkg_path="$(mktemp -t --suffix ".deb" task-pkg.XXXXXXXXXX)"
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.deb"
dpkg --install "$task_pkg_path"
rm "$task_pkg_path"

addressed in 57e152a

@junhaoliao junhaoliao requested review from hoophalab, kirkrodrigues and anlowee and removed request for kirkrodrigues and anlowee August 23, 2025 01:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
taskfile.yaml (1)

541-548: Remove stale {{.PACKAGE}}/**/* from python-component.sources; it references an undefined var and undermines the fix.

The goal here is to avoid the dynamic-variable bug by globbing **/* under dir: components/{{.COMPONENT}}. Keeping - "{{.PACKAGE}}/**/*" (Line 542) reintroduces a dynamic reference and PACKAGE is no longer defined in this task, which will cause a variable resolution error.

Apply this diff to rely solely on **/* with the dist exclusion:

   sources:
     - "{{.G_BUILD_DIR}}/{{.COMPONENT}}-venv.md5"
-    - "{{.PACKAGE}}/**/*"
     - "{{.ROOT_DIR}}/requirements.txt"
     - "{{.TASKFILE}}"
     - "/etc/os-release"
     - "**/*"
     - exclude: "dist/**/*"

Optional sanity check to ensure no remaining uses of {{.PACKAGE}}:

#!/bin/bash
# Expect no matches or only historical in unrelated areas.
rg -nP '\{\{\s*\.PACKAGE\s*\}\}'
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f1f6303 and ecdb8a8.

📒 Files selected for processing (2)
  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1 hunks)
  • taskfile.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#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.
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.

Applied to files:

  • taskfile.yaml
📚 Learning: 2025-08-14T18:23:39.476Z
Learnt from: junhaoliao
PR: y-scope/clp#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.

Applied to files:

  • taskfile.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: ubuntu-jammy-deps-image
  • GitHub Check: build-macos (macos-15, false)
  • GitHub Check: lint-check (macos-15)

@@ -29,14 +29,13 @@ DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \
unzip

# Install `task`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Install `task`
# Install `task`
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp/issues/872 and
# https://github.com/y-scope/clp-ffi-js/issues/110

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.44.0 does not avoid #872 (i.e., if we run 3.44.0 with our taskfile.yaml unmodified, the hang is still expected to occur. I'll submit an issue to https://github.com/go-task/task/ once i have the bandwidth.

how about just:

Suggested change
# Install `task`
# Install `task`
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Remind me why we're locking to >= 3.44.0? What's wrong with 3.40+?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.40 does not include this fix go-task/task@a60c2ec , which blocks use cases like #1119 (comment)

I intended to upgrade task to the latest version; however, it was just found that the latest version 3.44.1 causes the another bug in y-scope/clp-ffi-js#110 .

do note that go-task/task@a60c2ec happens to also be the cause that the "dynamic variables do not get populated in sources" issue surfaced, which is why we should modify our taskfile.yaml anyways to mitigate the hang.

(sorry... i can't find a better way to explain these. let me know if there's any confusion unresolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously i intentionally skipped the y-scope/clp-ffi-js#110 issue from any docs here in CLP because CLP does not depend on clp-ffi-js. though now i think it's good to write why a specific version is used so we don't forget in the future when we are itchy upgrading to new versions

Copy link
Member Author

@junhaoliao junhaoliao Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly off topic: we could change this https://github.com/y-scope/clp-ffi-js/blob/59890ba9a09bb8205281911735eb48cebb5a5a7c/Taskfile.yml#L133-L135

-          "{{.G_CLP_FFI_JS_BUILD_DIR}}/{{.G_CLP_FFI_JS_TARGET_PREFIX}}{{.ITEM}}."{d.ts,js,wasm}
+          {{.G_CLP_FFI_JS_BUILD_DIR}}/{{.G_CLP_FFI_JS_TARGET_PREFIX}}{{.ITEM}}.{d.ts,js,wasm}

then the latest version of task 3.44.1 can also run the clp-ffi-js's package task just fine. apparently go-task/task@d2bd834 , which intrudocued some incorrect string escape handling, is the commit that broke clp-ffi-js's package task.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. These details should really be in the PR description, right? Otherwise reviewers (and developers) don't know why we're upgrading task nor why we're locking to a specific set of versions. Even if we're arbitrarily locking to the latest available version because we don't have time to test other versions, I think we should mention that in the PR description.

I think we should also mention the version bump in the PR title.

Copy link
Member Author

@junhaoliao junhaoliao Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, sorry. the initial intention of the PR was to lift the version locking so developers can install any versions of task (which is sort of important). then the bug in ffi-js was found, and we had to lock the version again. then there were many above recent discoveries that i hadn't document yet

now the issues have been reported to go-task, and i have updated the PR description and title

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i kept the fix keyword in the title but let me know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should replicate these changes in the other container images.

@@ -13,8 +13,7 @@ prebuilt version instead, check out the [releases](https://github.com/y-scope/cl
* Python 3.9 or newer
* python3-dev
* python3-venv (for the version of Python installed)
* [Task] >= 3.40.0 and < 3.43.0
* We constrain the version due to unresolved [issues][clp-issue-872].
* [Task] >= 3.40.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* [Task] >= 3.40.0
* [Task] 3.40.0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per the above installation script, shall we write

Suggested change
* [Task] >= 3.40.0
* [Task] 3.44.0

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

♻️ Duplicate comments (1)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)

32-40: Optional: introduce a TASK_VERSION variable for maintainability.

Keeps the intent explicit and simplifies coordinated upgrades across platform scripts.

Apply this diff to the URL line, and define the variable near the top:

-    "https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.deb"
+    "https://github.com/go-task/task/releases/download/${TASK_VERSION}/task_linux_${task_pkg_arch}.deb"

Add near the top (outside this hunk):

# go-task version (pinned due to clp-ffi-js#110)
TASK_VERSION="v3.44.0"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ecdb8a8 and 68bafb4.

📒 Files selected for processing (9)
  • components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (1 hunks)
  • components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (1 hunks)
  • components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh (1 hunks)
  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1 hunks)
  • docs/README.md (1 hunks)
  • docs/src/dev-guide/building-package.md (1 hunks)
  • docs/src/dev-guide/components-core/index.md (1 hunks)
  • docs/src/dev-guide/contributing-linting.md (1 hunks)
  • taskfile.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#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.
📚 Learning: 2025-08-04T18:38:33.130Z
Learnt from: haiqi96
PR: y-scope/clp#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.

Applied to files:

  • docs/src/dev-guide/contributing-linting.md
  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
  • docs/src/dev-guide/components-core/index.md
  • docs/README.md
  • docs/src/dev-guide/building-package.md
📚 Learning: 2025-08-25T00:45:05.435Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.435Z
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.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
  • components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh
  • docs/src/dev-guide/components-core/index.md
  • docs/README.md
  • docs/src/dev-guide/building-package.md
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
  • components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh
  • components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
📚 Learning: 2025-08-25T00:45:05.435Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.435Z
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.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
  • docs/src/dev-guide/components-core/index.md
  • docs/README.md
  • docs/src/dev-guide/building-package.md
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.

Applied to files:

  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.

Applied to files:

  • taskfile.yaml
📚 Learning: 2025-08-14T18:23:39.476Z
Learnt from: junhaoliao
PR: y-scope/clp#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.

Applied to files:

  • taskfile.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: ubuntu-jammy-deps-image
  • GitHub Check: centos-stream-9-deps-image
  • GitHub Check: antlr-code-committed (macos-15)
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
docs/src/dev-guide/components-core/index.md (1)

12-14: Task version consistency confirmed

All references to the Task version across docs and installer scripts are correctly pinned to 3.44.0. No instances of 3.44.1 or version ranges were found.

components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (2)

42-49: LGTM on pinning Task to 3.44.0 and updating the RPM URL.

This aligns the platform script with the decision to avoid 3.44.1 due to the regression and addresses the dynamic variable issue present in earlier versions.


41-50: Minor: factor Task version into a single variable to ease future bumps.

Keeps scripts simple while reducing mechanical edits if/when we move off 3.44.0.

Apply this diff for the URL line, and add a small var near the top:

-    "https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.rpm"
+    "https://github.com/go-task/task/releases/download/${TASK_VERSION}/task_linux_${task_pkg_arch}.rpm"

Add near the top (outside this hunk):

# go-task version (pinned due to clp-ffi-js#110)
TASK_VERSION="v3.44.0"
⛔ Skipped due to learnings
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.435Z
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
PR: y-scope/clp#1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.435Z
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: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)

32-41: LGTM: correct pin to 3.44.0 and matching .deb URL.

Matches the ecosystem constraint (avoid 3.44.1) and the PR objective to resolve the dynamic variable hang without inheriting the shell-processing regression.

taskfile.yaml (2)

545-547: Good fix: broadening python-component sources removes the dynamic var pitfall.

Switching to "/*" with "dist//*" excluded avoids the go-task dynamic variable bug while keeping rebuilds deterministic in your clean component directories.


541-547: No remaining .PACKAGE references found
The repository was searched for {{.PACKAGE}} usages using the provided ripgrep command, and no matches were found. This confirms that the PACKAGE variable has been fully removed and there are no lingering references to reintroduce the hang.

Comment on lines 31 to +39
# Install `task`
# NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110
task_pkg_path=$(mktemp -t --suffix ".rpm" task-pkg.XXXXXXXXXX) || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.rpm"
"https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.rpm"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add curl retries for resilience; keep parity with other platform scripts.

Same reasoning as musllinux. Minimal change shown below.

 curl \
     --fail \
     --location \
     --output "$task_pkg_path" \
-    --show-error \
+    --show-error \
+    --retry 3 \
+    --retry-delay 2 \
     "https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.rpm"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Install `task`
# NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110
task_pkg_path=$(mktemp -t --suffix ".rpm" task-pkg.XXXXXXXXXX) || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.rpm"
"https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.rpm"
# Install `task`
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110
task_pkg_path=$(mktemp -t --suffix ".rpm" task-pkg.XXXXXXXXXX) || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
--retry 3 \
--retry-delay 2 \
"https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.rpm"
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
around lines 31 to 39, the curl invocation downloading the task RPM lacks retry
options; update the curl call to include retry flags (e.g. --retry 5
--retry-delay 2 --retry-connrefused and optionally --max-time or
--retry-max-time) so it behaves like the musllinux scripts and is resilient to
transient network failures; keep all other flags (--fail --location --output
--show-error) and the same URL, and ensure the mktemp exit handling remains
unchanged.

Comment on lines +33 to 41
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110
task_pkg_path=$(mktemp -t task-pkg.XXXXXXXXXX).tar.gz || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.tar.gz"
"https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.tar.gz"
tar -C /usr/local/bin -xzf "$task_pkg_path" task
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Harden the download step with retries (keep scripts consistent across platforms).

A transient network blip will currently fail the install. Add curl retries (and consider mirroring across all platform install scripts in this PR for consistency).

Apply this minimal diff:

 curl \
     --fail \
     --location \
     --output "$task_pkg_path" \
-    --show-error \
+    --show-error \
+    --retry 3 \
+    --retry-delay 2 \
     "https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.tar.gz"

Optional follow-up (separate PR): add SHA256 verification for the Task artifact uniformly across all install-prebuilt-packages scripts.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110
task_pkg_path=$(mktemp -t task-pkg.XXXXXXXXXX).tar.gz || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.tar.gz"
"https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.tar.gz"
tar -C /usr/local/bin -xzf "$task_pkg_path" task
# NOTE: We lock `task` to version 3.44.0 to avoid https://github.com/y-scope/clp-ffi-js/issues/110
task_pkg_path=$(mktemp -t task-pkg.XXXXXXXXXX).tar.gz || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
--retry 3 \
--retry-delay 2 \
"https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.tar.gz"
tar -C /usr/local/bin -xzf "$task_pkg_path" task
🤖 Prompt for AI Agents
components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh
around lines 33 to 41: the curl download is currently a single attempt and will
fail on transient network errors; update the curl invocation to include retry
behavior (for example add --retry 5 --retry-delay 2 --retry-connrefused
--retry-all-errors and optionally --retry-max-time 30) while keeping --fail
--location --show-error and --output so the script will automatically retry
transient failures; apply the same retry flags consistently across the other
platform install-prebuilt-packages scripts in this PR.

@@ -13,8 +13,7 @@ this project:
the size of repo as we add and update images.
* [Node.js] >= 16 to be able to [view the output](#viewing-the-output)
* Python 3.10 or later
* [Task] >= 3.40.0 and < 3.43.0
* We constrain the version due to unresolved [issues][clp-issue-872].
* [Task] 3.44.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Doc pin is good; add brief rationale to prevent accidental upgrades.

Given the known regression in Task v3.44.1, add a short parenthetical note and reference so maintainers don’t “helpfully” bump past 3.44.0 in future edits.

Apply this diff inline:

-* [Task] 3.44.0
+* [Task] 3.44.0 (pinned due to a 3.44.1 regression; see [clp-ffi-js #110])

And append a link reference at the end of the file (or reuse an existing references block):

+[clp-ffi-js #110]: https://github.com/y-scope/clp-ffi-js/issues/110
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [Task] 3.44.0 (pinned due to a 3.44.1 regression; see [clp-ffi-js #110])
[clp-ffi-js #110]: https://github.com/y-scope/clp-ffi-js/issues/110
🤖 Prompt for AI Agents
In docs/README.md around line 16, the pinned Task version "* [Task] 3.44.0"
needs a brief rationale to prevent accidental upgrades: update the line to
include a short parenthetical note about the regression in v3.44.1 and a
citation marker (e.g., "(regression in 3.44.1)[1]"), then append the referenced
link at the end of the file (or add to an existing references block) pointing to
the issue/PR describing the regression so maintainers have context when editing.

@@ -13,8 +13,7 @@ prebuilt version instead, check out the [releases](https://github.com/y-scope/cl
* Python 3.9 or newer
* python3-dev
* python3-venv (for the version of Python installed)
* [Task] >= 3.40.0 and < 3.43.0
* We constrain the version due to unresolved [issues][clp-issue-872].
* [Task] 3.44.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Make the pin’s rationale explicit (align with installer script comments).

Match the installer scripts’ note so readers understand why 3.44.0 is fixed.

-* [Task] 3.44.0
+* [Task] 3.44.0 (pinned to avoid a known 3.44.1 regression; see [clp-ffi-js #110])

Add (or reuse) the link reference at the bottom:

+[clp-ffi-js #110]: https://github.com/y-scope/clp-ffi-js/issues/110
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [Task] 3.44.0 (pinned to avoid a known 3.44.1 regression; see [clp-ffi-js #110])
[clp-ffi-js #110]: https://github.com/y-scope/clp-ffi-js/issues/110
🤖 Prompt for AI Agents
In docs/src/dev-guide/building-package.md around line 16, the pinned version
entry "* [Task] 3.44.0" lacks the installer-script rationale; update that list
item to explicitly state why 3.44.0 is pinned (matching the installer script
comment text) and add or reuse the link reference at the bottom of the file
pointing to the installer note or issue that explains the pin so readers
understand the reason.

💡 Verification agent

🧩 Analysis chain

Verify repository-wide version consistency and Taskfile sources change.

Ensure every doc/script points to 3.44.0 and the python-component Task sources are simplified with the correct exclude.

Run:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check docs for Task version pins =="
rg -nP '\[Task\][^\n]*' docs | sed -E 's/^/DOC: /'

echo "== Check install scripts download URLs =="
rg -nP 'go-task/task/releases/download/v3\.\d+\.\d+' components/core/tools/scripts | sed -E 's/^/SCRIPT: /'

echo "== Expect only v3.44.0 references =="
rg -n 'v3\.44\.0' -n
echo "== Ensure there are no 3.44.1 or older pins lingering =="
! rg -n 'v3\.44\.1|v3\.42\.' || { echo "Found non-3.44.0 Task references above"; exit 1; }

echo "== Locate Taskfile and verify python-component sources simplification =="
# Find Taskfile with common names/cases
fd -HI --strip-cwd-prefix -t f -i '^taskfile\.ya?ml$|^taskfile$|^taskfile\.yml$' || true
TF="$(fd -HI -t f -i '^taskfile\.ya?ml$|^taskfile$|^taskfile\.yml$' | head -n1 || true)"
if [[ -n "${TF:-}" ]]; then
  echo "Using Taskfile: $TF"
  rg -nP -C3 '(?s)python-component:\s.*?sources:\s*\n\s*-\s+\*\*/\*\*\s*(\n.*?exclude:\s*-\s*dist/\*\*/\*\*)?' "$TF" || {
    echo "Could not confirm simplified sources (**/*) and dist/**/* exclude for python-component."
    exit 1
  }
else
  echo "Taskfile not found; verify manually."
fi

Length of output: 3426


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Locate python-component snippet in taskfile.yaml =="
rg -nC5 'python-component' -n taskfile.yaml

Length of output: 2296


Version consistency checks passed; Taskfile update required

All documentation and install scripts correctly reference Task v3.44.0—no action needed there. However, the python-component target in taskfile.yaml is missing the simplified sources and exclude entries that ensure only source files are packaged and built artifacts are omitted. Please update as follows:

• File: taskfile.yaml
• Location: under the python-component: section (around line 533)

Suggested diff:

@@ -533,7 +533,11 @@
   python-component:
     internal: true
     label: "{{.COMPONENT}}"
+    sources:
+      - "**/**"
+    exclude:
+      - "dist/**"
     vars:
       VENV_DIR: "{{.G_BUILD_DIR}}/{{.COMPONENT}}/venv"
     requires:

This ensures that all files (**/**) are included by default, while excluding any dist/** build outputs.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
python-component:
internal: true
label: "{{.COMPONENT}}"
sources:
- "**/**"
exclude:
- "dist/**"
vars:
VENV_DIR: "{{.G_BUILD_DIR}}/{{.COMPONENT}}/venv"
requires:
# (existing requirements)
🤖 Prompt for AI Agents
In docs/src/dev-guide/building-package.md around line 16, update the guidance to
instruct adding simplified sources and exclude entries under the
python-component section of taskfile.yaml (around line 533); specifically, add a
sources entry that includes all files (e.g., '**/**') and an exclude entry that
omits build outputs (e.g., 'dist/**') so only source files are packaged and
built artifacts are excluded. Ensure the instructions mention exactly where
(file and section) to place these entries and that they follow Taskfile YAML
indentation conventions.

@@ -9,8 +9,7 @@ CLP core is the low-level component that performs compression, decompression, an
* A recent compiler that fully supports C++20 features such as
* std::span
* std::source_location
* [Task] >= 3.40.0 and < 3.43.0
* We constrain the version due to unresolved [issues][clp-issue-872].
* [Task] 3.44.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Explain why Task is pinned to exactly 3.44.0 to pre-empt confusion.

Right now the requirement just states 3.44.0. Given we’re avoiding 3.44.1 due to a regression and ≥3.40.0 still has the dynamic var bug, add a short note to capture the rationale and cross-reference the relevant issue IDs.

Apply this diff to clarify the requirement:

-* [Task] 3.44.0
+* [Task] 3.44.0
+  * Note: We pin exactly 3.44.0 because it fixes the dynamic variable hang (see [clp-issue-872]) and 3.44.1 introduces a shell command processing regression (see clp-ffi-js issue #110). We'll revisit once upstream releases a fixed version.

And add a link definition for clp-issue-872 if not already in this file’s link section:

+[clp-issue-872]: https://github.com/y-scope/clp/issues/872
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [Task] 3.44.0
* Note: We pin exactly 3.44.0 because it fixes the dynamic variable hang (see [clp-issue-872]) and 3.44.1 introduces a shell command processing regression (see clp-ffi-js issue #110). We'll revisit once upstream releases a fixed version.
[clp-issue-872]: https://github.com/y-scope/clp/issues/872
🤖 Prompt for AI Agents
In docs/src/dev-guide/components-core/index.md around line 12, the entry "*
[Task] 3.44.0" lacks explanation for why the version is pinned; update that line
to include a short parenthetical note stating that Task is pinned to exactly
3.44.0 because 3.44.1 was avoided due to a regression and versions ≥3.40.0
contain the dynamic var bug (reference related issue IDs), and add a
cross-reference link definition for clp-issue-872 in this file’s link section if
it doesn't already exist.

Comment on lines 18 to 20
* [Task] 3.44.0
* [uv] >= 0.8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Briefly annotate the Task pin; optionally clarify uv usage.

Add a short parenthetical for the Task pin (to mirror installer notes). Consider a one-liner after the list noting uv is used for Python packaging to justify the new dependency.

-* [Task] 3.44.0
+* [Task] 3.44.0 (temporarily pinned; see [clp-ffi-js #110])
 * [uv] >= 0.8

Optionally add under the list:

+> Note: We use [uv] for Python environment and package management during linting.

And add the link reference if not already present in this file:

+[clp-ffi-js #110]: https://github.com/y-scope/clp-ffi-js/issues/110
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [uv] >= 0.8
* [Task] 3.44.0 (temporarily pinned; see [clp-ffi-js #110])
* [uv] >= 0.8
> Note: We use [uv] for Python environment and package management during linting.
[clp-ffi-js #110]: https://github.com/y-scope/clp-ffi-js/issues/110
🤖 Prompt for AI Agents
In docs/src/dev-guide/contributing-linting.md around lines 18 to 20, add a brief
parenthetical next to the Task pin to mirror installer notes (e.g., "Task 3.44.0
(pinned to installer requirement)") and insert a one-line note immediately after
the list clarifying that "uv" is required for Python packaging (e.g., "uv is
used for Python packaging"). Also ensure the link reference for Task (and uv if
used elsewhere) is present in the file; if missing, add the appropriate
reference entry at the bottom of the document.

@junhaoliao junhaoliao changed the title fix(taskfile): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872). fix(taskfile): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872); Upgrade go-task to v3.44.0. Aug 25, 2025
@junhaoliao junhaoliao changed the title fix(taskfile): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872); Upgrade go-task to v3.44.0. fix(taskfile): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872); Upgrade and pin go-task to v3.44.0. Aug 25, 2025
kirkrodrigues
kirkrodrigues previously approved these changes Aug 30, 2025
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the PR title, how about:

fix(taskfile): Simplify `python-component` sources list to avoid go-task/task#2400 (fixes #872); Upgrade and pin `go-task` to v3.44.0 to avoid go-task/task#2401.

@junhaoliao junhaoliao changed the title fix(taskfile): Simplify python-component sources list to avoid the dynamic variable bug in go-task (fixes #872); Upgrade and pin go-task to v3.44.0. fix(taskfile): Simplify python-component sources list to avoid go-task/task#2400 (fixes #872); Upgrade and pin go-task to v3.44.0 to avoid go-task/task#2401. Sep 2, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/src/dev-docs/building-package.md (1)

11-12: Fix grammar: “a some” → “some”.

User-facing docs nit.

-  * It should be possible to build a package for a different environment, it just requires a some
+  * It should be possible to build a package for a different environment; it just requires some
     extra configuration.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 68bafb4 and 1eb19e4.

📒 Files selected for processing (3)
  • docs/src/dev-docs/building-package.md (1 hunks)
  • docs/src/dev-docs/components-core/index.md (1 hunks)
  • docs/src/dev-docs/contributing-linting.md (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#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
PR: y-scope/clp#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.
📚 Learning: 2025-08-04T18:38:33.130Z
Learnt from: haiqi96
PR: y-scope/clp#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.

Applied to files:

  • docs/src/dev-docs/components-core/index.md
  • docs/src/dev-docs/contributing-linting.md
📚 Learning: 2025-08-25T00:45:05.464Z
Learnt from: junhaoliao
PR: y-scope/clp#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.

Applied to files:

  • docs/src/dev-docs/components-core/index.md
  • docs/src/dev-docs/building-package.md
📚 Learning: 2025-08-25T00:45:05.464Z
Learnt from: junhaoliao
PR: y-scope/clp#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.

Applied to files:

  • docs/src/dev-docs/building-package.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: manylinux_2_28-x86_64-deps-image
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: build (macos-15)
🔇 Additional comments (2)
docs/src/dev-docs/components-core/index.md (1)

12-12: All Task version mentions are pinned to v3.44.0; no 3.44.1+ references detected.

docs/src/dev-docs/contributing-linting.md (1)

18-18: Verify all Task invocations in workflows are pinned to 3.44.0
Manually inspect every .github/workflows/*.yml for any task or go-task step and confirm the version is set to 3.44.0 to prevent drift.

@@ -13,8 +13,7 @@ prebuilt version instead, check out the [releases](https://github.com/y-scope/cl
* Python 3.9 or newer
* python3-dev
* python3-venv (for the version of Python installed)
* [Task] >= 3.40.0 and < 3.43.0
* We constrain the version due to unresolved [issues][clp-issue-872].
* [Task] 3.44.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Pin looks correct; consider adding rationale to prevent accidental bump to 3.44.1.

Add a brief note about the 3.44.1 regression (brace expansion) and why we pin to 3.44.0.

-* [Task] 3.44.0
+* [Task] 3.44.0  <!-- Pinned due to 3.44.1 regression breaking brace expansion; see go-task/task#2401 -->
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [Task] 3.44.0 <!-- Pinned due to 3.44.1 regression breaking brace expansion; see go-task/task#2401 -->
🤖 Prompt for AI Agents
In docs/src/dev-docs/building-package.md around line 16, the pin entry "* [Task]
3.44.0" lacks context; update this line to append a short rationale explaining
that 3.44.1 contains a brace-expansion regression and therefore we pin to 3.44.0
to avoid that breakage, include a one-sentence summary of the regression and
optionally a reference (issue/PR number) so future readers know why not to
upgrade automatically.

@@ -9,8 +9,7 @@ CLP core is the low-level component that performs compression, decompression, an
* A recent compiler that fully supports C++20 features such as
* std::span
* std::source_location
* [Task] >= 3.40.0 and < 3.43.0
* We constrain the version due to unresolved [issues][clp-issue-872].
* [Task] 3.44.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Good pin; add a short caution about 3.44.1 regression.

Helps future editors avoid bumping past the safe version.

-* [Task] 3.44.0
+* [Task] 3.44.0  <!-- Pin: 3.44.1 introduces a shell/brace expansion regression -->
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [Task] 3.44.0 <!-- Pin: 3.44.1 introduces a shell/brace expansion regression -->
🤖 Prompt for AI Agents
In docs/src/dev-docs/components-core/index.md around line 12, the pinned version
"* [Task] 3.44.0" needs a short caution about a regression in 3.44.1; update the
line or immediately following sentence to note that 3.44.1 contains a known
regression and that editors should keep the pin at 3.44.0 (or explicitly call
out the safe range), using a concise one-sentence warning so future editors
won’t accidentally bump past the safe version.

* [Task] >= 3.40.0 and < 3.43.0
* Minimum version 3.40.0 is required for [yscope-dev-utils].
* We constrain the version to < 3.43.0 due to unresolved [issues][clp-issue-872].
* [Task] 3.44.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM on pin; mirror the rationale inline for clarity.

-* [Task] 3.44.0
+* [Task] 3.44.0  <!-- Pinned: 3.44.1 regression breaks brace expansion -->
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* [Task] 3.44.0
* [Task] 3.44.0 <!-- Pinned: 3.44.1 regression breaks brace expansion -->
🤖 Prompt for AI Agents
In docs/src/dev-docs/contributing-linting.md around line 18, the release pin "*
[Task] 3.44.0" lacks an inline rationale; update that line to keep the version
pin but append a short explanatory phrase (e.g., "– pinned for lint rule
stability/compatibility with X toolchain") so reviewers see why the pin is
required; keep the rationale concise and specific to the stability/compatibility
reason referenced in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants