feat(release): add P2 distribution channels - #41
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds multi-platform distribution: Dockerfile + GHCR workflow, Scoop manifest generator + bucket workflow, WinGet submission workflow, Nix flake app export and CI check, monthly flake.lock updater, README/ignore/changelog updates, and Scoop manifest tests. ChangesMulti-Platform Distribution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-pythinker-cli.yml:
- Around line 338-344: Replace the loose presence check for PYTHINKER_MANAGED
with a strict equality assertion: update the CI step that runs apps.default (the
nix run/.#default and build result/bin/pythinker check) to verify that the
wrapper sets PYTHINKER_MANAGED to the string "nix" (for example by matching the
exact exported/assigned value such as PYTHINKER_MANAGED="nix" or an equivalent
exact check) instead of only grepping for the variable name.
In @.github/workflows/docker.yml:
- Around line 54-69: The current loop only checks for HTTP 200 from the pypi
JSON endpoint; update the "Wait for the wheel on PyPI" step to fetch the JSON
body into a variable (using curl with per-call timeouts such as
--connect-timeout and --max-time) and parse it to ensure at least one entry in
the urls array has packagetype == "bdist_wheel" before exiting the loop; replace
the until condition that only checks HTTP code with a check that verifies
presence of a bdist_wheel (use jq or a safe grep/json parse) and handle curl
failures by retrying until the 30-minute deadline, keeping informative log
messages referencing PKG_VERSION and the url variable.
- Around line 14-17: The workflow currently grants packages: write at the
workflow-level which exposes write permissions to all jobs including resolve;
change the workflow-level permissions to only what's needed (e.g., keep
contents: read and set packages: read or remove packages entirely) and then add
packages: write specifically to the publishing jobs by updating the job-level
permissions for the jobs named build-amd64, build-arm64, and merge (move-latest
already has it) so only those jobs have packages: write while resolve retains
read-only permissions.
In @.github/workflows/scoop-bucket.yml:
- Around line 36-44: The version-selection block accepts any non-empty
INPUT_VERSION; change it to validate strict semver and fail fast: replace the
elif [[ -n "${INPUT_VERSION:-}" ]] branch with a test that the INPUT_VERSION
matches the regex ^[0-9]+\.[0-9]+\.[0-9]+$ (e.g. [[ "$INPUT_VERSION" =~
^[0-9]+\.[0-9]+\.[0-9]+$ ]]) and only assign version="$INPUT_VERSION" if it
matches; if it does not match emit an error (echo "::error::Invalid version:
...") and exit 1. This keeps the tag-match branch using GITHUB_REF unchanged and
ensures the version variable is always valid semver.
- Around line 1-25: Add a GitHub Actions concurrency guard to the workflow to
serialize bucket updates and avoid race conditions: inside the update job (job
name "update") add a concurrency block with a stable group name that includes
the bucket/repo and ref or version (for example use group: "scoop-bucket-${{
env.BUCKET_REPO }}-${{ github.ref || github.event.inputs.version }}") and set
cancel-in-progress appropriately (e.g., cancel-in-progress: false to queue new
runs or true to cancel in-flight runs) so only one run may push to
scoop-pythinker:main at a time.
- Around line 26-27: The checkout step "Checkout source repo" uses
actions/checkout@de0fac2e... which leaves persist-credentials true by default;
update that step to explicitly set persist-credentials: false so the checkout
token is not written to local git config (keep the same pinned action version
and step name).
In @.github/workflows/update-flake-lock.yml:
- Line 14: The GitHub Actions job currently declared as "update-lock" lacks a
human-readable display name; add a top-level name: field for the job (the job
keyed as update-lock) in the .github/workflows/update-flake-lock.yml so the
Actions UI shows a descriptive title (e.g., name: "Update flake lock" or
similar) directly under the update-lock job definition.
- Around line 3-7: Add a GitHub Actions concurrency block to the workflow to
prevent duplicate runs/PRs by setting a stable concurrency group name (e.g.,
"update-flake-lock") and enabling cancel-in-progress; modify the top-level
workflow "on:" section (near the existing schedule/cron and workflow_dispatch
entries) to include a concurrency key with a fixed group string and
cancel-in-progress: true so manual triggers during scheduled runs won't spawn
concurrent update jobs.
- Around line 17-18: The checkout step named "Checkout repository" currently
uses actions/checkout@de0fac2e... and leaves the GITHUB_TOKEN in the checked-out
.git/config; update that step to set persist-credentials: false so the token is
not persisted (add the persist-credentials: false key under the uses entry for
the checkout step).
- Around line 9-11: The workflow currently sets workflow-level permissions
("permissions" with "contents: write" and "pull-requests: write"); move these
permissions into each job that requires them to reduce blast radius—remove or
empty the top-level "permissions" block and add a job-level "permissions" map to
only the jobs that need "contents: write" or "pull-requests: write" (reference
the "permissions" key and the "contents" and "pull-requests" entries) so each
job explicitly declares the minimal permissions it requires.
In @.github/workflows/winget.yml:
- Around line 11-20: Add a GitHub Actions concurrency guard to the submit job to
prevent duplicate manual WinGet submissions: inside the job named "submit" add a
concurrency block (e.g. concurrency: group: "winget-${{ github.ref }}-${{
github.event.inputs.version || github.sha }}" cancel-in-progress: false) so runs
for the same ref/version are serialized instead of racing; place the block at
the top level of the submit job in .github/workflows/winget.yml.
- Line 45: The workflow is downloading a moving target via Invoke-WebRequest
-Uri "https://aka.ms/wingetcreate/latest" -OutFile wingetcreate.exe and running
it without integrity checks; change this to download a pinned GitHub release
asset URL for winget-create (use a fixed tag/version in the URL instead of
/latest), also download the release's published SHA256 checksum file (e.g.,
wingetcreate.exe.txt), compute the SHA256 of the downloaded wingetcreate.exe in
the same step, compare it to the published checksum, and fail the job if they do
not match; update the Invoke-WebRequest invocations and add the checksum
verification logic so the pipeline only proceeds when the checksum comparison
succeeds.
In `@Dockerfile`:
- Around line 6-34: The container currently runs as root because there is no
USER instruction; add a non-root runtime user and switch to it before the
ENTRYPOINT/CMD so pythinker doesn't run as root. Create an unprivileged user
(e.g., pythinker or appuser) and group, chown any installed runtime
directories/files if needed, set HOME if required, then add a USER instruction
to switch to that user prior to ENTRYPOINT ["pythinker"] (ensure any files
created during image build are accessible to that user).
In `@packages/scoop-bucket/generate-manifest.py`:
- Around line 35-37: The HTTP fetch helpers _fetch_json and _fetch_text
currently call urllib.request.urlopen without an Authorization header which can
hit GitHub rate limits; update both functions to read an optional GITHUB_TOKEN
from the environment and, when present, add an "Authorization: token
<GITHUB_TOKEN>" header to the urllib.request.Request (preserve the existing
"Accept: application/vnd.github+json" for _fetch_json), then use that Request in
urlopen so authenticated requests are made when a token exists, falling back to
unauthenticated requests if not.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c4755f4-6c34-49c6-b109-2714d0430408
📒 Files selected for processing (13)
.dockerignore.github/workflows/ci-pythinker-cli.yml.github/workflows/docker.yml.github/workflows/scoop-bucket.yml.github/workflows/update-flake-lock.yml.github/workflows/winget.yml.gitignoreDockerfileREADME.mdflake.nixpackages/scoop-bucket/generate-manifest.pypackages/scoop-bucket/pythinker-code.json.tmpltests/test_scoop_manifest.py
Contributor
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 (3)
.github/workflows/ci-pythinker-cli.yml (1)
338-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
"nix"(double-quote) match may not align with how the Nix wrapper actually writes the env var.
makeWrapper/wrapProgramtypically emitexport PYTHINKER_MANAGED='nix'with single quotes, sogrep -q 'PYTHINKER_MANAGED.*"nix"'would fail to match and break this step even whenflake.nixis correctly configured. The exact format depends on how the wrapper is built inflake.nix, which isn't in this diff.If the wrapper uses single quotes, a quote-agnostic match is safer:
🔍 Quote-agnostic assertion
- grep -q 'PYTHINKER_MANAGED.*"nix"' result/bin/pythinker + grep -qE "PYTHINKER_MANAGED[=[:space:]].*['\"]?nix['\"]?" result/bin/pythinkerNote: a
makeBinaryWrapper(compiled) wrapper may not be grep-friendly at all; in that case, prefer running the wrapper and asserting the exported value at runtime.Confirm how
PYTHINKER_MANAGEDis set in the wrapper so the grep pattern actually matches:#!/bin/bash # Inspect flake.nix for the wrapper mechanism and PYTHINKER_MANAGED quoting. fd -t f 'flake.nix' --exec sh -c ' echo "=== {} ===" rg -n -C3 "PYTHINKER_MANAGED|makeWrapper|wrapProgram|makeBinaryWrapper|setEnv|--set" "{}" '🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-pythinker-cli.yml around lines 338 - 344, The grep assertion using grep -q 'PYTHINKER_MANAGED.*"nix"' is brittle because wrappers may use single quotes or produce non-grepable binaries; change the check so it is quote-agnostic or verify the env at runtime: update the grep invocation that references "PYTHINKER_MANAGED.*\"nix\"" to a pattern that matches either single or double quotes (e.g., PYTHINKER_MANAGED.*['\"]nix['\"]) against result/bin/pythinker, or instead run the wrapper (nix run .#default or result/bin/pythinker) and assert the exported PYTHINKER_MANAGED value at runtime by executing the binary and echoing/inspecting $PYTHINKER_MANAGED; ensure you update the step name that currently runs nix run .#default and the grep line accordingly..github/workflows/winget.yml (1)
25-36: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider verifying the installer asset exists.
The step confirms the release is published and non-prerelease but doesn't check whether
PythinkerSetup-${VERSION}.exeexists. If the asset is missing or misnamed, wingetcreate will fail late with a less actionable error.🔍 Suggested enhancement
Extend the verification to confirm the asset exists:
run: | set -euo pipefail is_pre=$(gh release view "v${VERSION}" --repo "${GITHUB_REPOSITORY}" --json isPrerelease -q '.isPrerelease') if [ "$is_pre" != "false" ]; then echo "::error::Release v${VERSION} is not a promoted (non-prerelease) release; refusing to submit to WinGet." >&2 exit 1 fi + + # Verify the installer asset exists + asset_name="PythinkerSetup-${VERSION}.exe" + if ! gh release view "v${VERSION}" --repo "${GITHUB_REPOSITORY}" --json assets -q ".assets[].name" | grep -qx "$asset_name"; then + echo "::error::Installer asset $asset_name not found in release v${VERSION}." >&2 + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/winget.yml around lines 25 - 36, Add an explicit check that the release contains the expected installer asset "PythinkerSetup-${VERSION}.exe" before calling wingetcreate: after using gh release view to get isPrerelease, call gh release view "v${VERSION}" --json assets -q '.assets[].name' (or equivalent) and verify one of the names equals "PythinkerSetup-${VERSION}.exe"; if not, emit a clear ::error:: mentioning the missing asset and exit 1. Ensure you reference the VERSION variable and keep the check in the same step that currently uses is_pre so the workflow fails early with an actionable message when the asset is missing.packages/scoop-bucket/generate-manifest.py (1)
45-57: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider validating URL scheme to satisfy Ruff S310.
Both
_fetch_jsonand_fetch_texttrigger S310 becauseurlopenacceptsfile://or custom schemes. In practice, URLs here come from hardcoded constants or GitHub API responses, so risk is low—but a simple guard would silence the linter and add defense-in-depth.♻️ Optional scheme check
+def _assert_https(url: str) -> None: + if not url.startswith("https://"): + raise ValueError(f"URL must use https scheme: {url}") + + def _fetch_json(url: str) -> dict[str, Any]: + _assert_https(url) request = urllib.request.Request(url, headers=_github_headers(url)) with urllib.request.urlopen(request, timeout=30) as resp: data = json.load(resp) @@ def _fetch_text(url: str) -> str: + _assert_https(url) request = urllib.request.Request(url, headers=_github_headers(url, accept="text/plain")) with urllib.request.urlopen(request, timeout=30) as resp:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/scoop-bucket/generate-manifest.py` around lines 45 - 57, Add a URL-scheme whitelist to both _fetch_json and _fetch_text to satisfy Ruff S310: parse the supplied url with urllib.parse.urlparse and ensure the scheme is "http" or "https" before calling urllib.request.urlopen; if the scheme is not allowed, raise a clear exception (e.g., ValueError or RuntimeError) mentioning the offending URL. Update both functions (_fetch_json and _fetch_text) to perform this check at the top and then proceed to build the Request and call urlopen as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci-pythinker-cli.yml:
- Around line 338-344: The grep assertion using grep -q
'PYTHINKER_MANAGED.*"nix"' is brittle because wrappers may use single quotes or
produce non-grepable binaries; change the check so it is quote-agnostic or
verify the env at runtime: update the grep invocation that references
"PYTHINKER_MANAGED.*\"nix\"" to a pattern that matches either single or double
quotes (e.g., PYTHINKER_MANAGED.*['\"]nix['\"]) against result/bin/pythinker, or
instead run the wrapper (nix run .#default or result/bin/pythinker) and assert
the exported PYTHINKER_MANAGED value at runtime by executing the binary and
echoing/inspecting $PYTHINKER_MANAGED; ensure you update the step name that
currently runs nix run .#default and the grep line accordingly.
In @.github/workflows/winget.yml:
- Around line 25-36: Add an explicit check that the release contains the
expected installer asset "PythinkerSetup-${VERSION}.exe" before calling
wingetcreate: after using gh release view to get isPrerelease, call gh release
view "v${VERSION}" --json assets -q '.assets[].name' (or equivalent) and verify
one of the names equals "PythinkerSetup-${VERSION}.exe"; if not, emit a clear
::error:: mentioning the missing asset and exit 1. Ensure you reference the
VERSION variable and keep the check in the same step that currently uses is_pre
so the workflow fails early with an actionable message when the asset is
missing.
In `@packages/scoop-bucket/generate-manifest.py`:
- Around line 45-57: Add a URL-scheme whitelist to both _fetch_json and
_fetch_text to satisfy Ruff S310: parse the supplied url with
urllib.parse.urlparse and ensure the scheme is "http" or "https" before calling
urllib.request.urlopen; if the scheme is not allowed, raise a clear exception
(e.g., ValueError or RuntimeError) mentioning the offending URL. Update both
functions (_fetch_json and _fetch_text) to perform this check at the top and
then proceed to build the Request and call urlopen as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d44ce1aa-551b-49fb-b2a2-5dcd21dc7da4
📒 Files selected for processing (7)
.github/workflows/ci-pythinker-cli.yml.github/workflows/docker.yml.github/workflows/scoop-bucket.yml.github/workflows/update-flake-lock.yml.github/workflows/winget.ymlDockerfilepackages/scoop-bucket/generate-manifest.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci-pythinker-cli.yml (1)
343-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the assertion exact for
PYTHINKER_MANAGED=nix(not substring).Line 343 still uses a substring match; it can pass on non-exact values. Use an assignment-aware pattern.
Suggested fix
- grep -Eq 'PYTHINKER_MANAGED.*nix' result/bin/pythinker + grep -Eq '(^|[[:space:]])(export[[:space:]]+)?PYTHINKER_MANAGED="?nix"?([[:space:]]|$)' result/bin/pythinker🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-pythinker-cli.yml at line 343, The grep check currently uses a substring pattern 'PYTHINKER_MANAGED.*nix' which can match unintended values; update the grep call in the line containing grep -Eq 'PYTHINKER_MANAGED.*nix' result/bin/pythinker to use an assignment-aware pattern such as grep -Eq '^PYTHINKER_MANAGED=nix$' (or a more permissive assignment-aware variant like grep -Eq '(^|[[:space:]])PYTHINKER_MANAGED=nix($|[[:space:]]|;)' ) so it only matches the exact assignment PYTHINKER_MANAGED=nix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/ci-pythinker-cli.yml:
- Line 343: The grep check currently uses a substring pattern
'PYTHINKER_MANAGED.*nix' which can match unintended values; update the grep call
in the line containing grep -Eq 'PYTHINKER_MANAGED.*nix' result/bin/pythinker to
use an assignment-aware pattern such as grep -Eq '^PYTHINKER_MANAGED=nix$' (or a
more permissive assignment-aware variant like grep -Eq
'(^|[[:space:]])PYTHINKER_MANAGED=nix($|[[:space:]]|;)' ) so it only matches the
exact assignment PYTHINKER_MANAGED=nix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7fa8240a-0cfb-4e9b-80d7-0eed9d169179
📒 Files selected for processing (1)
.github/workflows/ci-pythinker-cli.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
:latestmovement.apps.default,PYTHINKER_MANAGED=nixwrapper env, CI smoke, and monthly flake.lock update PR workflow..understand-anythingcache output.Verification
uv run pytest tests/test_scoop_manifest.py tests/test_version_lockstep.py -vvuv run ruff check packages/scoop-bucket/generate-manifest.py tests/test_scoop_manifest.pyuv run ruff format --check packages/scoop-bucket/generate-manifest.py tests/test_scoop_manifest.pydocker run --rm --security-opt label=disable -v /home/ai/Projects/pythinker-code-main:/repo -w /repo docker.io/rhysd/actionlint:latest -color .github/workflows/docker.yml .github/workflows/scoop-bucket.yml .github/workflows/update-flake-lock.yml .github/workflows/winget.yml .github/workflows/ci-pythinker-cli.ymldocker build --build-arg PYTHINKER_VERSION=0.27.0 -t pythinker-docker-test:local .docker run --rm pythinker-docker-test:local --versiondocker run --rm --entrypoint env pythinker-docker-test:local | grep PYTHINKER_MANAGEDuv run python packages/scoop-bucket/generate-manifest.py --version 0.27.0 --template packages/scoop-bucket/pythinker-code.json.tmpl --output <tmp>/pythinker-code.jsonNotes
nixis not installed locally; CI covers the Nix smoke.Summary by CodeRabbit
New Features
Documentation
Tests
Chores