Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThree Bash scripts and two GitHub Actions workflows are added to automate plugin CI/CD. A shared ChangesPlugin CI/CD automation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/scripts/check-version-bump.sh (1)
25-30: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConfirm
sort -Vmatches the versioning contract.
sort -Vis only semver-safe for bareX.Y.Zvalues. If plugin manifests can ever carry prereleases or build metadata, this check will compare them incorrectly. Please confirm.tessl-plugin/plugin.jsonis constrained to simple triples, or swap in a real semver comparator.🤖 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/scripts/check-version-bump.sh around lines 25 - 30, The version_gt function relies on sort -V for version comparison, which only correctly handles plain X.Y.Z format and will fail with prerelease versions or build metadata. Either verify that the .tessl-plugin/plugin.json file is constrained to only simple X.Y.Z version triples through documentation or validation, or replace the version comparison logic in the version_gt function with a proper semver comparator that can handle full semantic versioning including prerelease and build metadata components.
🤖 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/scripts/changed-plugins.sh:
- Around line 21-26: The for loop iterating over $changed_dirs will word-split
on whitespace, causing plugin directory names containing spaces or tabs to be
truncated and skipped. Replace the `for dir in $changed_dirs;` loop with a while
read loop that properly handles whitespace-containing directory names, using
`while IFS= read -r dir;` with appropriate input redirection or piping to safely
iterate over each directory without splitting on whitespace.
In @.github/workflows/publish.yml:
- Around line 13-32: In the publish job, replace the mutable version tags for
both actions/checkout and tesslio/setup-tessl with their specific commit SHAs
instead of `@v4` and `@v2` respectively to prevent tag-retargeting attacks. Add an
explicit permissions block at the job level after runs-on to limit access to
only what is required for the workflow. Finally, add persist-credentials: false
to the checkout action step to prevent credential persistence and improve
security when handling sensitive secrets like TESSL_TOKEN.
In @.github/workflows/version-check.yml:
- Around line 8-22: Pin the actions/checkout action to a full-length commit SHA
instead of the `@v4` version tag to prevent supply chain attacks. Replace
actions/checkout@v4 with the full commit SHA format. Additionally, add a
permissions section at the job level (check-version-bump) to restrict the
GITHUB_TOKEN to only contents: read access. Finally, add persist-credentials:
false to the checkout step's with configuration to reduce the attack surface by
preventing credential persistence.
---
Nitpick comments:
In @.github/scripts/check-version-bump.sh:
- Around line 25-30: The version_gt function relies on sort -V for version
comparison, which only correctly handles plain X.Y.Z format and will fail with
prerelease versions or build metadata. Either verify that the
.tessl-plugin/plugin.json file is constrained to only simple X.Y.Z version
triples through documentation or validation, or replace the version comparison
logic in the version_gt function with a proper semver comparator that can handle
full semantic versioning including prerelease and build metadata components.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06892dc2-26d3-44fb-8813-67dabe0da703
📒 Files selected for processing (5)
.github/scripts/changed-plugins.sh.github/scripts/check-version-bump.sh.github/scripts/publish-changed.sh.github/workflows/publish.yml.github/workflows/version-check.yml
…lugin discovery Address CodeRabbit review on #10: SHA-pin actions/checkout and tesslio/setup-tessl, add least-privilege contents:read permissions and persist-credentials:false, and iterate changed dirs with while-read to avoid word-splitting plugin names.
|
@coderabbitai full review |
Gareth added a TESSL_TOKEN repo secret with publish access to the tessl workspace; point the publish workflow at it.
|
Re CodeRabbit's Confirmed — plugin manifest versions are bare (from Claude) |
…e tip changed-plugins.sh used a two-dot `git diff base head`. For a pull_request that compares the head against the base branch's current tip, so once main advances and touches a plugin while a PR is open, that plugin shows as "changed" by the PR even when the PR never touched it - and version-check then demands a bump for it. Switch to a three-dot range so detection is relative to the merge-base: only what the branch actually changed. check-version-bump.sh still reads the base version from the literal base ref, so a head version is still compared against current main and accidental version collisions are still caught. For the publish workflow (before/after on a linear main) the merge-base is the before ref, so three-dot equals the previous two-dot range - no behaviour change there.
… not base tip" This reverts commit 1791501. The two-dot `git diff base head` it replaced was already correct: github.event.pull_request.base.sha is the PR's merge-base, not the base branch's current tip. Verified across all five version-check runs on this PR (2026-06-22 to 2026-06-24, during which main advanced from 35eab39 to df8fb2f): BASE_SHA was 35eab39 - the fork point - in every run, never the moving main tip. So base..head already diffs against the merge-base and the three-dot change was a behavioural no-op.
What & why
product-pluginshas no CI — plugin publishing is manual (tessl plugin publish). That letskill-optimizer0.9.1 get published from an unmerged branch. This adds the two pieces from OPT-877:main—publish.ymlpublishes every plugin whose files changed in the pushed commit.version-check.ymlfails a PR if a changed plugin's publishedversionwasn't bumped forward.Together they close the gap: the PR gate guarantees a version bump rides along with any plugin change, so the publish on
mainalways has a fresh version to publish.Design
A shared script defines "what is a plugin" and "what changed" in one place; both workflows build on it.
.github/scripts/changed-plugins.sh.tessl-plugin/plugin.jsonwhose files changed between two git refs.github/scripts/check-version-bump.sh.tessl-plugin/plugin.jsonversionto be strictly greater than the base ref (new plugins pass).github/scripts/publish-changed.shtessl plugin publish <dir>for each changed plugin; falls back to<sha>^when GitHub sends an all-zerobeforeSHA.github/workflows/version-check.ymlpull_request→main: runs the bump check.github/workflows/publish.ymlpush→main: installs the CLI viatesslio/setup-tessl@v2, then publishes changed plugins;concurrencyguard serializes overlapping mergesNotes
.tessl-plugin/plugin.json), so this coversskill-optimizertoday and any plugin added later — no per-plugin wiring..tessl-plugin/plugin.json, not thetessl.jsonvendored-deps manifest.bash+jq(preinstalled onubuntu-latest); semver comparison viasort -V.Required before this works
TESSL_TOKENrepository secret with publish access to thetesslworkspace —publish.ymlauthenticates with it. (Added by @gareth.)Tested locally
.githubchange → no plugins detectedbash -nFollow-up to watch on first run
skill-optimizer/tessl.jsondeclares atessl-labs/tile-creatorvendored dependency. Notessl installstep precedes publish (it can pull unpublished deps and break). If the first real publish needs deps resolved, that's a one-line addition.Closes OPT-877.