ci: migrate from self-hosted to GitHub-hosted runners#318
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI workflows moved to macOS runners and converted to a per‑tag matrix; signing, temporary keychain, and notarization were added; artifact staging made optional; build/release now processes one tag per matrix entry. Several Swift lint/format tidies applied and one SSH function lost its return. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Actions as "GitHub Actions (resolve-tags)"
participant Runner as "macOS Runner (build job)"
participant Keychain as "Temporary Keychain"
participant Notary as "Apple Notary (notarytool)"
participant GitHub as "GitHub Releases"
participant Registry as "Plugin Registry (git)"
Actions->>Runner: start matrix job for single TAG (from matrix)
Runner->>Runner: extract PLUGIN_NAME & VERSION\nbuild arm64 & x86_64\ncompute SHA256\npackage artifacts
Runner->>Keychain: create temp keychain & import signing certs
Runner->>Notary: submit artifacts for notarization
Notary-->>Runner: notarization result
Runner->>GitHub: create per‑tag release & upload artifacts
Runner->>Registry: update plugins.json and push (if deploy key present)
GitHub-->>Actions: release created
Registry-->>Actions: registry updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
135-166:⚠️ Potential issue | 🟠 MajorUse
macos-15-intel(or another explicit Intel label) instead ofmacos-latestfor the x86_64 build job.
macos-latestis an arm64 (Apple Silicon) runner. The job attempts to run x86_64-specific Homebrew commands (arch -x86_64 /usr/local/bin/brew) on an arm64 system, which will fail. If this job needs to build x86_64 binaries or use x86_64 dependencies, pin it tomacos-15-intelor a similar Intel-specific label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 135 - 166, The macOS runner is pinned to arm64 via "runs-on: macos-latest" while the job ("Install x86_64 dependencies" step and commands using "arch -x86_64 /usr/local/bin/brew") requires an Intel runner; change the job's runs-on to an Intel-specific label such as "macos-15-intel" (or another explicit Intel runner) so the x86_64 Homebrew commands succeed, and ensure any documentation/comments reflect the Intel requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-plugin.yml:
- Around line 187-194: The notarization block is unreachable because
NOTARIZE_PLUGINS is never provided to the runner; wire that value into the
workflow (e.g., add a workflow input or job/env entry named NOTARIZE_PLUGINS and
reference it in the job Env) so the shell if [ "${NOTARIZE_PLUGINS:-}" = "true"
] can evaluate correctly; update the workflow to set NOTARIZE_PLUGINS via a
workflow input or job env (or default to "false") and ensure the run step that
invokes xcrun notarytool submit uses the same BUNDLE_NAME and build/Plugins path
as shown.
- Around line 49-51: The matrix strategy runs jobs concurrently and causes race
conditions when each job clones and pushes registry changes; update the workflow
by adding max-parallel: 1 under the existing strategy/matrix block to serialize
execution, or refactor the registry update into a single downstream job that
runs after the matrix completes; if you keep the registry update inside the
matrix, add a pre-commit guard around the commit/push sequence (the place that
updates plugins.json and runs git commit) that checks for staged changes (e.g.,
run a quiet git-diff check before committing) so git commit is only invoked when
there are actual changes to commit.
- Around line 67-80: Replace the incorrect macOS security subcommand
"list-keychain" with the correct plural "list-keychains" in the "Import signing
certificate" step so the final invocation uses security list-keychains -d user
-s "$KEYCHAIN_PATH" login.keychain (the line that references KEYCHAIN_PATH and
login.keychain).
In @.github/workflows/build.yml:
- Around line 83-96: The workflow uses the incorrect macOS security subcommand
"security list-keychain" — replace both occurrences with the correct plural
subcommand "security list-keychains" so the keychain is properly added to the
user keychain list; update the two steps that run the exact string "security
list-keychain -d user -s \"$KEYCHAIN_PATH\" login.keychain" (one in each build
job) to use "security list-keychains" instead and keep the same flags/order.
In `@scripts/ci/package-artifacts.sh`:
- Around line 63-68: The staging block currently swallows all cp failures by
appending "2>/dev/null || true" to the two copy commands; change it so only the
"no match" case is ignored but real copy errors abort. For example, remove the
"|| true" from the cp commands for the dmg and zip and instead handle globs
explicitly (use a loop or enable nullglob) so you only skip when no files exist;
keep the STAGING check and the same cp targets (build/Release/*.dmg and
build/Release/TablePro-${ARCH}.zip) but ensure each cp returns a non-zero exit
on error so the script fails on real copy failures.
---
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 135-166: The macOS runner is pinned to arm64 via "runs-on:
macos-latest" while the job ("Install x86_64 dependencies" step and commands
using "arch -x86_64 /usr/local/bin/brew") requires an Intel runner; change the
job's runs-on to an Intel-specific label such as "macos-15-intel" (or another
explicit Intel runner) so the x86_64 Homebrew commands succeed, and ensure any
documentation/comments reflect the Intel requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e507742-e27a-41af-b469-e58384ae0faa
📒 Files selected for processing (3)
.github/workflows/build-plugin.yml.github/workflows/build.ymlscripts/ci/package-artifacts.sh
Comment on lines
+49
to
+51
| strategy: | ||
| matrix: ${{ fromJson(needs.resolve-tags.outputs.matrix) }} | ||
| fail-fast: false |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
cd .github/workflows && wc -l build-plugin.ymlRepository: datlechin/TablePro
Repository: datlechin/TablePro
Exit code: 0
stdout:
283 build-plugin.yml
Script executed:
cat -n .github/workflows/build-plugin.yml | sed -n '40,60p'Repository: datlechin/TablePro
Repository: datlechin/TablePro
Exit code: 0
stdout:
40 JSON+=']}'
41 echo "matrix=$JSON" >> "$GITHUB_OUTPUT"
42 echo "Matrix: $JSON"
43
44 build-plugin:
45 name: "Build ${{ matrix.tag }}"
46 needs: resolve-tags
47 runs-on: macos-latest
48 timeout-minutes: 30
49 strategy:
50 matrix: ${{ fromJson(needs.resolve-tags.outputs.matrix) }}
51 fail-fast: false
52
53 steps:
54 - name: Checkout code
55 uses: actions/checkout@v4
56 with:
57 lfs: true
58
59 - name: Pull LFS files
60 run: git lfs pull
Script executed:
cat -n .github/workflows/build-plugin.yml | sed -n '210,280p'Repository: datlechin/TablePro
Repository: datlechin/TablePro
Exit code: 0
stdout:
210 gh release create "$TAG" \
211 --title "$DISPLAY_NAME v$VERSION" \
212 --notes "$RELEASE_BODY" \
213 build/Plugins/${BUNDLE_NAME}-arm64.zip \
214 build/Plugins/${BUNDLE_NAME}-x86_64.zip
215
216 # Update plugin registry
217 if [ -n "${REGISTRY_DEPLOY_KEY:-}" ]; then
218 ARM64_URL="https://github.com/${{ github.repository }}/releases/download/${TAG}/${BUNDLE_NAME}-arm64.zip"
219 X86_64_URL="https://github.com/${{ github.repository }}/releases/download/${TAG}/${BUNDLE_NAME}-x86_64.zip"
220
221 WORK=$(mktemp -d)
222 eval "$(ssh-agent -s)"
223 echo "$REGISTRY_DEPLOY_KEY" | ssh-add -
224
225 git clone git@github.com:datlechin/tablepro-plugins.git "$WORK/registry"
226 cd "$WORK/registry"
227 git pull --rebase origin main
228
229 python3 - \
230 "$BUNDLE_ID" "$DISPLAY_NAME" "$VERSION" "$SUMMARY" \
231 "$DB_TYPE_IDS" "$ARM64_URL" "$ARM64_SHA" \
232 "$X86_64_URL" "$X86_SHA" "$MIN_APP_VERSION" \
233 "$ICON" "$HOMEPAGE" "$CATEGORY" \
234 <<'PYTHON_SCRIPT'
235 import json, sys
236
237 bundle_id, name, version, summary = sys.argv[1:5]
238 db_type_ids = json.loads(sys.argv[5])
239 arm64_url, arm64_sha = sys.argv[6], sys.argv[7]
240 x86_64_url, x86_64_sha = sys.argv[8], sys.argv[9]
241 min_app_version, icon, homepage = sys.argv[10], sys.argv[11], sys.argv[12]
242 category = sys.argv[13] if len(sys.argv) > 13 else "database-driver"
243
244 with open("plugins.json", "r") as f:
245 manifest = json.load(f)
246
247 entry = {
248 "id": bundle_id, "name": name, "version": version,
249 "summary": summary,
250 "author": {"name": "TablePro", "url": "https://tablepro.app"},
251 "homepage": homepage, "category": category,
252 "databaseTypeIds": db_type_ids,
253 "downloadURL": arm64_url, "sha256": arm64_sha,
254 "binaries": [
255 {"architecture": "arm64", "downloadURL": arm64_url, "sha256": arm64_sha},
256 {"architecture": "x86_64", "downloadURL": x86_64_url, "sha256": x86_64_sha}
257 ],
258 "minAppVersion": min_app_version,
259 "minPluginKitVersion": 1,
260 "iconName": icon, "isVerified": True
261 }
262
263 manifest["plugins"] = [p for p in manifest["plugins"] if p["id"] != bundle_id]
264 manifest["plugins"].append(entry)
265
266 with open("plugins.json", "w") as f:
267 json.dump(manifest, f, indent=2)
268 f.write("\n")
269 PYTHON_SCRIPT
270
271 git config user.name "github-actions[bot]"
272 git config user.email "github-actions[bot]@users.noreply.github.com"
273 git add plugins.json
274 git commit -m "Update $DISPLAY_NAME to v$VERSION"
275 git push
276
277 ssh-add -D
278 eval "$(ssh-agent -k)"
279 cd -
280 rm -rf "$WORK"
Script executed:
cat -n .github/workflows/build-plugin.yml | sed -n '273,276p'Repository: datlechin/TablePro
Repository: datlechin/TablePro
Exit code: 0
stdout:
273 git add plugins.json
274 git commit -m "Update $DISPLAY_NAME to v$VERSION"
275 git push
276
Script executed:
grep -n "set -e\|set -o pipefail\||| true\|git commit" .github/workflows/build-plugin.ymlRepository: datlechin/TablePro
Repository: datlechin/TablePro
Exit code: 0
stdout:
209: gh release delete "$TAG" --yes 2>/dev/null || true
274: git commit -m "Update $DISPLAY_NAME to v$VERSION"
Script executed:
cat -n .github/workflows/build-plugin.yml | sed -n '145,220p'Repository: datlechin/TablePro
Repository: datlechin/TablePro
Exit code: 0
stdout:
145 DISPLAY_NAME="Redis Driver"; SUMMARY="Redis in-memory data store driver via hiredis"
146 DB_TYPE_IDS='["Redis"]'; ICON="redis-icon"; BUNDLE_NAME="RedisDriver"
147 CATEGORY="database-driver"; HOMEPAGE="https://docs.tablepro.app/databases/redis" ;;
148 xlsx)
149 TARGET="XLSXExport"; BUNDLE_ID="com.TablePro.XLSXExportPlugin"
150 DISPLAY_NAME="XLSX Export"; SUMMARY="Export data to Microsoft Excel XLSX format"
151 DB_TYPE_IDS='null'; ICON="doc.richtext"; BUNDLE_NAME="XLSXExport"
152 CATEGORY="export-format"; HOMEPAGE="https://docs.tablepro.app/features/export" ;;
153 mql)
154 TARGET="MQLExport"; BUNDLE_ID="com.TablePro.MQLExportPlugin"
155 DISPLAY_NAME="MQL Export"; SUMMARY="Export MongoDB data as MQL statements"
156 DB_TYPE_IDS='null'; ICON="doc.text"; BUNDLE_NAME="MQLExport"
157 CATEGORY="export-format"; HOMEPAGE="https://docs.tablepro.app/features/export" ;;
158 sqlimport)
159 TARGET="SQLImport"; BUNDLE_ID="com.TablePro.SQLImportPlugin"
160 DISPLAY_NAME="SQL Import"; SUMMARY="Import data from SQL dump files"
161 DB_TYPE_IDS='null'; ICON="square.and.arrow.down"; BUNDLE_NAME="SQLImport"
162 CATEGORY="import-format"; HOMEPAGE="https://docs.tablepro.app/features/import" ;;
163 *) echo "Unknown plugin: $plugin_name"; return 1 ;;
164 esac
165 }
166
167 PLUGIN_NAME=$(echo "$TAG" | sed -E 's/^plugin-([a-z]+)-v.*$/\1/')
168 VERSION=$(echo "$TAG" | sed -E 's/^plugin-[a-z]+-v(.*)$/\1/')
169
170 resolve_plugin_info "$PLUGIN_NAME"
171
172 echo "Building $TARGET v$VERSION"
173
174 # Build Cassandra dependencies if needed
175 if [ "$PLUGIN_NAME" = "cassandra" ]; then
176 ./scripts/build-cassandra.sh both
177 fi
178
179 # Build both architectures
180 ./scripts/build-plugin.sh "$TARGET" arm64
181 ./scripts/build-plugin.sh "$TARGET" x86_64
182
183 # Capture SHA-256
184 ARM64_SHA=$(cat "build/Plugins/${BUNDLE_NAME}-arm64.zip.sha256")
185 X86_SHA=$(cat "build/Plugins/${BUNDLE_NAME}-x86_64.zip.sha256")
186
187 # Notarize if enabled
188 if [ "${NOTARIZE_PLUGINS:-}" = "true" ]; then
189 for zip in build/Plugins/${BUNDLE_NAME}-*.zip; do
190 xcrun notarytool submit "$zip" \
191 --keychain-profile "TablePro" \
192 --wait
193 done
194 fi
195
196 # Create GitHub Release
197 RELEASE_BODY="## $DISPLAY_NAME v$VERSION
198
199 Plugin release for TablePro.
200
201 ### Installation
202 TablePro will prompt you to install this plugin automatically when you select the database type. You can also install manually via **Settings > Plugins > Browse**.
203
204 ### SHA-256
205 - ARM64: \`$ARM64_SHA\`
206 - x86_64: \`$X86_SHA\`"
207
208 # Delete existing release if any, then create
209 gh release delete "$TAG" --yes 2>/dev/null || true
210 gh release create "$TAG" \
211 --title "$DISPLAY_NAME v$VERSION" \
212 --notes "$RELEASE_BODY" \
213 build/Plugins/${BUNDLE_NAME}-arm64.zip \
214 build/Plugins/${BUNDLE_NAME}-x86_64.zip
215
216 # Update plugin registry
217 if [ -n "${REGISTRY_DEPLOY_KEY:-}" ]; then
218 ARM64_URL="https://github.com/${{ github.repository }}/releases/download/${TAG}/${BUNDLE_NAME}-arm64.zip"
219 X86_64_URL="https://github.com/${{ github.repository }}/releases/download/${TAG}/${BUNDLE_NAME}-x86_64.zip"
220
Prevent concurrent registry pushes—serialize with max-parallel: 1 or move to downstream job.
Matrix jobs execute concurrently by default. Each clones the registry, modifies plugins.json, and pushes to main (lines 216–275). Multi-tag dispatches will race on the remote, and reruns fail at line 274 when plugins.json is unchanged—git commit exits with "nothing to commit" and no error handler is present.
Add max-parallel: 1 to the strategy block to serialize execution, or extract the registry update into a single downstream job triggered after all builds complete. If keeping it in the matrix, add a guard: git diff --cached --quiet || git commit -m "Update..." before the push.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-plugin.yml around lines 49 - 51, The matrix strategy
runs jobs concurrently and causes race conditions when each job clones and
pushes registry changes; update the workflow by adding max-parallel: 1 under the
existing strategy/matrix block to serialize execution, or refactor the registry
update into a single downstream job that runs after the matrix completes; if you
keep the registry update inside the matrix, add a pre-commit guard around the
commit/push sequence (the place that updates plugins.json and runs git commit)
that checks for staged changes (e.g., run a quiet git-diff check before
committing) so git commit is only invoked when there are actual changes to
commit.
Comment on lines
+67
to
+80
| - name: Import signing certificate | ||
| env: | ||
| CERTIFICATES_P12: ${{ secrets.CERTIFICATES_P12 }} | ||
| CERTIFICATES_PASSWORD: ${{ secrets.CERTIFICATES_PASSWORD }} | ||
| run: | | ||
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | ||
| security create-keychain -p "" "$KEYCHAIN_PATH" | ||
| security set-keychain-settings -lut 21600 "$KEYCHAIN_PATH" | ||
| security unlock-keychain -p "" "$KEYCHAIN_PATH" | ||
| echo "$CERTIFICATES_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 | ||
| security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ | ||
| -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" | ||
| security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH" | ||
| security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
macOS security command list-keychains vs list-keychain documentation
💡 Result:
security’s documented subcommand is list-keychains (plural). In Apple’s security(1) man page it’s described as:
list-keychains [-h] [-d user|system|common|dynamic] [-s [keychain...]]— “Display or manipulate the keychain search list.” [1]
There is no documented security list-keychain (singular) subcommand in the security(1) command list; when you see “list-keychain” online, it’s typically a typo/misremembering of list-keychains (e.g., posts discussing it in that mistaken form). [1], [2]
If you want to confirm on your Mac, run:
security help(lists valid subcommands)man security(official local documentation) [1]
Sources: [1] security(1) man page (manpagez mirror) [2] Stack Overflow discussion referencing “security list-keychain” in the singular (as used/mistyped in the question)
Fix the keychain search-list command.
security exposes list-keychains (plural); list-keychain is not a valid subcommand.
Suggested fix
- security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain
+ security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain📝 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
| - name: Import signing certificate | |
| env: | |
| CERTIFICATES_P12: ${{ secrets.CERTIFICATES_P12 }} | |
| CERTIFICATES_PASSWORD: ${{ secrets.CERTIFICATES_PASSWORD }} | |
| run: | | |
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | |
| security create-keychain -p "" "$KEYCHAIN_PATH" | |
| security set-keychain-settings -lut 21600 "$KEYCHAIN_PATH" | |
| security unlock-keychain -p "" "$KEYCHAIN_PATH" | |
| echo "$CERTIFICATES_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 | |
| security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ | |
| -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" | |
| security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH" | |
| security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain | |
| - name: Import signing certificate | |
| env: | |
| CERTIFICATES_P12: ${{ secrets.CERTIFICATES_P12 }} | |
| CERTIFICATES_PASSWORD: ${{ secrets.CERTIFICATES_PASSWORD }} | |
| run: | | |
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | |
| security create-keychain -p "" "$KEYCHAIN_PATH" | |
| security set-keychain-settings -lut 21600 "$KEYCHAIN_PATH" | |
| security unlock-keychain -p "" "$KEYCHAIN_PATH" | |
| echo "$CERTIFICATES_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 | |
| security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ | |
| -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" | |
| security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH" | |
| security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-plugin.yml around lines 67 - 80, Replace the
incorrect macOS security subcommand "list-keychain" with the correct plural
"list-keychains" in the "Import signing certificate" step so the final
invocation uses security list-keychains -d user -s "$KEYCHAIN_PATH"
login.keychain (the line that references KEYCHAIN_PATH and login.keychain).
Comment on lines
+187
to
+194
| # Notarize if enabled | ||
| if [ "${NOTARIZE_PLUGINS:-}" = "true" ]; then | ||
| for zip in build/Plugins/${BUNDLE_NAME}-*.zip; do | ||
| xcrun notarytool submit "$zip" \ | ||
| --keychain-profile "TablePro" \ | ||
| --wait | ||
| done | ||
| fi |
There was a problem hiding this comment.
This notarization block is currently unreachable.
NOTARIZE_PLUGINS is never set in this workflow, so the ZIPs are always released without going through notarytool submit. If notarization is intended here, wire it to a job env/input instead of an unset shell variable.
One possible wiring
- name: Build and release plugin
env:
REGISTRY_DEPLOY_KEY: ${{ secrets.REGISTRY_DEPLOY_KEY }}
GH_TOKEN: ${{ github.token }}
+ NOTARIZE_PLUGINS: "true"
run: |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-plugin.yml around lines 187 - 194, The notarization
block is unreachable because NOTARIZE_PLUGINS is never provided to the runner;
wire that value into the workflow (e.g., add a workflow input or job/env entry
named NOTARIZE_PLUGINS and reference it in the job Env) so the shell if [
"${NOTARIZE_PLUGINS:-}" = "true" ] can evaluate correctly; update the workflow
to set NOTARIZE_PLUGINS via a workflow input or job env (or default to "false")
and ensure the run step that invokes xcrun notarytool submit uses the same
BUNDLE_NAME and build/Plugins path as shown.
Comment on lines
+83
to
+96
| - name: Import signing certificate | ||
| env: | ||
| CERTIFICATES_P12: ${{ secrets.CERTIFICATES_P12 }} | ||
| CERTIFICATES_PASSWORD: ${{ secrets.CERTIFICATES_PASSWORD }} | ||
| run: | | ||
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | ||
| security create-keychain -p "" "$KEYCHAIN_PATH" | ||
| security set-keychain-settings -lut 21600 "$KEYCHAIN_PATH" | ||
| security unlock-keychain -p "" "$KEYCHAIN_PATH" | ||
| echo "$CERTIFICATES_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 | ||
| security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ | ||
| -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" | ||
| security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH" | ||
| security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/build.yml | sed -n '83,96p'Repository: datlechin/TablePro
Length of output: 989
🏁 Script executed:
cat -n .github/workflows/build.yml | sed -n '184,197p'Repository: datlechin/TablePro
Length of output: 989
🌐 Web query:
macOS security command list-keychains syntax official documentation
💡 Result:
The authoritative (“official”) reference for security list-keychains is the security(1) manual page that ships with macOS (view it locally with man security, or get subcommand help with security help list-keychains). The documented syntax is: [1]
security list-keychains [-h] [-d user|system|common|dynamic] [-s [keychain...]]- Without
-s, it displays the current keychain search list. [1] - With
-s, it sets the search list to the provided keychain paths. [1] -d user|system|common|dynamicselects the preference domain that the search list is read from / written to. [1]
Examples
security list-keychains
security list-keychains -d user
security list-keychains -d user -s ~/Library/Keychains/login.keychain-dbFix the keychain command syntax in both build jobs.
Lines 96 and 197 use security list-keychain, but the correct subcommand is security list-keychains (plural). The singular form is not a valid subcommand per the official macOS security(1) manual.
Suggested fix
- security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain
+ security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain📝 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
| - name: Import signing certificate | |
| env: | |
| CERTIFICATES_P12: ${{ secrets.CERTIFICATES_P12 }} | |
| CERTIFICATES_PASSWORD: ${{ secrets.CERTIFICATES_PASSWORD }} | |
| run: | | |
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | |
| security create-keychain -p "" "$KEYCHAIN_PATH" | |
| security set-keychain-settings -lut 21600 "$KEYCHAIN_PATH" | |
| security unlock-keychain -p "" "$KEYCHAIN_PATH" | |
| echo "$CERTIFICATES_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 | |
| security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ | |
| -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" | |
| security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH" | |
| security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain | |
| - name: Import signing certificate | |
| env: | |
| CERTIFICATES_P12: ${{ secrets.CERTIFICATES_P12 }} | |
| CERTIFICATES_PASSWORD: ${{ secrets.CERTIFICATES_PASSWORD }} | |
| run: | | |
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | |
| security create-keychain -p "" "$KEYCHAIN_PATH" | |
| security set-keychain-settings -lut 21600 "$KEYCHAIN_PATH" | |
| security unlock-keychain -p "" "$KEYCHAIN_PATH" | |
| echo "$CERTIFICATES_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 | |
| security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ | |
| -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" | |
| security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH" | |
| security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yml around lines 83 - 96, The workflow uses the
incorrect macOS security subcommand "security list-keychain" — replace both
occurrences with the correct plural subcommand "security list-keychains" so the
keychain is properly added to the user keychain list; update the two steps that
run the exact string "security list-keychain -d user -s \"$KEYCHAIN_PATH\"
login.keychain" (one in each build job) to use "security list-keychains" instead
and keep the same flags/order.
Comment on lines
+63
to
+68
| if [ -n "$STAGING" ]; then | ||
| mkdir -p "$STAGING" | ||
| cp build/Release/*.dmg "$STAGING/" 2>/dev/null || true | ||
| cp "build/Release/TablePro-${ARCH}.zip" "$STAGING/" 2>/dev/null || true | ||
| echo "Artifacts staged to $STAGING" | ||
| ls -lh "$STAGING" |
There was a problem hiding this comment.
Don't swallow staging copy failures.
If a caller passes STAGING and either cp fails, Line 67 still reports success and the script exits cleanly. That makes the local/self-hosted fallback path silently unreliable. Only ignore the no-match case; let real copy errors fail the script.
Suggested fix
if [ -n "$STAGING" ]; then
mkdir -p "$STAGING"
- cp build/Release/*.dmg "$STAGING/" 2>/dev/null || true
- cp "build/Release/TablePro-${ARCH}.zip" "$STAGING/" 2>/dev/null || true
+ compgen -G "build/Release/*.dmg" >/dev/null && cp build/Release/*.dmg "$STAGING/"
+ cp "build/Release/TablePro-${ARCH}.zip" "$STAGING/"
echo "Artifacts staged to $STAGING"
ls -lh "$STAGING"
fi📝 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
| if [ -n "$STAGING" ]; then | |
| mkdir -p "$STAGING" | |
| cp build/Release/*.dmg "$STAGING/" 2>/dev/null || true | |
| cp "build/Release/TablePro-${ARCH}.zip" "$STAGING/" 2>/dev/null || true | |
| echo "Artifacts staged to $STAGING" | |
| ls -lh "$STAGING" | |
| if [ -n "$STAGING" ]; then | |
| mkdir -p "$STAGING" | |
| compgen -G "build/Release/*.dmg" >/dev/null && cp build/Release/*.dmg "$STAGING/" | |
| cp "build/Release/TablePro-${ARCH}.zip" "$STAGING/" | |
| echo "Artifacts staged to $STAGING" | |
| ls -lh "$STAGING" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/package-artifacts.sh` around lines 63 - 68, The staging block
currently swallows all cp failures by appending "2>/dev/null || true" to the two
copy commands; change it so only the "no match" case is ignored but real copy
errors abort. For example, remove the "|| true" from the cp commands for the dmg
and zip and instead handle globs explicitly (use a loop or enable nullglob) so
you only skip when no files exist; keep the STAGING check and the same cp
targets (build/Release/*.dmg and build/Release/TablePro-${ARCH}.zip) but ensure
each cp returns a non-zero exit on error so the script fails on real copy
failures.
…lidation to showBuildSettings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
83-97:⚠️ Potential issue | 🔴 CriticalUse
security list-keychains(plural) in both certificate-import steps.Line 96 and Line 207 currently call
security list-keychain(singular), which is not the valid subcommand and can break keychain setup.Suggested fix
- security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain + security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain ... - security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain + security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain#!/bin/bash # Verify no invalid singular subcommand remains. rg -n 'security list-keychain\b' .github/workflows/build.yml # Expected: no matches # Verify plural form is present. rg -n 'security list-keychains\b' .github/workflows/build.yml # Expected: 2 matches (one per build job)Also applies to: 194-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 83 - 97, In the "Import signing certificate" step (the shell block that creates and configures KEYCHAIN_PATH and runs security import), replace the invalid singular subcommand usage `security list-keychain -d user -s "$KEYCHAIN_PATH" login.keychain` with the correct plural `security list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain`; do the same replacement for the other certificate-import step mentioned (the second occurrence of `security list-keychain`) so both uses call `security list-keychains` for proper keychain setup.
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
93-95: Tighten certificate import permissions (avoid-A).The
-Aflag provides unrestricted access to imported keys. For CI signing, use explicit tool allowlisting via-T /usr/bin/codesign -T /usr/bin/securityto limit exposure.Suggested fix
security import $RUNNER_TEMP/certificate.p12 -P "$CERTIFICATES_PASSWORD" \ - -A -t cert -f pkcs12 -k "$KEYCHAIN_PATH" + -t cert -f pkcs12 -k "$KEYCHAIN_PATH" \ + -T /usr/bin/codesign -T /usr/bin/securityAlso applies to: lines 204-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 93 - 95, Replace the insecure `security import` usage that uses the `-A` (allow all apps) flag with an explicit tool allowlist: in the `security import` command that imports certificate.p12, remove `-A` and add `-T /usr/bin/codesign -T /usr/bin/security` so only codesign and security can access the private key; do the same for the second occurrence noted in the workflow. Keep the subsequent `security set-key-partition-list -S apple-tool:,apple: -k "" "$KEYCHAIN_PATH"` call but ensure it still references the same `"$KEYCHAIN_PATH"` keychain used by the import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Core/Plugins/PluginMetadataRegistry.swift`:
- Around line 128-129: The array of ExplainVariant entries contains a trailing
comma after the last element (the one with pathFieldRole: .database); remove
that trailing comma so the final ExplainVariant entry is the array's last item
without a comma (update the array where ExplainVariant and pathFieldRole:
.database are defined).
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 83-97: In the "Import signing certificate" step (the shell block
that creates and configures KEYCHAIN_PATH and runs security import), replace the
invalid singular subcommand usage `security list-keychain -d user -s
"$KEYCHAIN_PATH" login.keychain` with the correct plural `security
list-keychains -d user -s "$KEYCHAIN_PATH" login.keychain`; do the same
replacement for the other certificate-import step mentioned (the second
occurrence of `security list-keychain`) so both uses call `security
list-keychains` for proper keychain setup.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 93-95: Replace the insecure `security import` usage that uses the
`-A` (allow all apps) flag with an explicit tool allowlist: in the `security
import` command that imports certificate.p12, remove `-A` and add `-T
/usr/bin/codesign -T /usr/bin/security` so only codesign and security can access
the private key; do the same for the second occurrence noted in the workflow.
Keep the subsequent `security set-key-partition-list -S apple-tool:,apple: -k ""
"$KEYCHAIN_PATH"` call but ensure it still references the same
`"$KEYCHAIN_PATH"` keychain used by the import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5020d88b-f5a4-4ba8-a1f5-b227b3aa33a9
📒 Files selected for processing (4)
.github/workflows/build.ymlTablePro/Core/Plugins/PluginMetadataRegistry.swiftTablePro/Views/Connection/ConnectionFormView.swiftscripts/build-release.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- TablePro/Views/Connection/ConnectionFormView.swift
Comment on lines
+128
to
+129
| ], | ||
| pathFieldRole: .database, |
There was a problem hiding this comment.
Remove the trailing comma from the last ExplainVariant.
Line 128 leaves a trailing comma before the closing ], which violates this repo's Swift formatting rules.
Suggested fix
- ExplainVariant(id: "estimate", label: "Estimate", sqlPrefix: "EXPLAIN ESTIMATE"),
+ ExplainVariant(id: "estimate", label: "Estimate", sqlPrefix: "EXPLAIN ESTIMATE")As per coding guidelines, "no trailing commas in Swift code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Core/Plugins/PluginMetadataRegistry.swift` around lines 128 - 129,
The array of ExplainVariant entries contains a trailing comma after the last
element (the one with pathFieldRole: .database); remove that trailing comma so
the final ExplainVariant entry is the array's last item without a comma (update
the array where ExplainVariant and pathFieldRole: .database are defined).
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/build.yml (3)
323-327:⚠️ Potential issue | 🟠 MajorSame conditional issue on appcast commit step.
Apply the same fix: use
secrets.SPARKLE_PRIVATE_KEYin theifcondition.Suggested fix
- name: Commit appcast.xml to repo - if: env.SPARKLE_PRIVATE_KEY != '' + if: ${{ secrets.SPARKLE_PRIVATE_KEY != '' }} continue-on-error: true env: SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 323 - 327, The GitHub Actions step named "Commit appcast.xml to repo" uses env.SPARKLE_PRIVATE_KEY in its if condition, which can be empty; change the conditional to reference the secret directly (use secrets.SPARKLE_PRIVATE_KEY in the if expression) so the step only runs when the secret is present, keeping the existing env mapping of SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }} unchanged.
313-317:⚠️ Potential issue | 🟠 MajorSame conditional issue — use
secrets.SPARKLE_PRIVATE_KEYcheck.Same issue as the previous step. The
env.SPARKLE_PRIVATE_KEYcheck reads an empty value.Suggested fix
- name: Upload appcast artifact - if: env.SPARKLE_PRIVATE_KEY != '' + if: ${{ secrets.SPARKLE_PRIVATE_KEY != '' }} uses: actions/upload-artifact@v4 env: SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 313 - 317, The workflow step "Upload appcast artifact" uses the wrong conditional check; change the step's if: from checking env.SPARKLE_PRIVATE_KEY to checking the secret directly (if: secrets.SPARKLE_PRIVATE_KEY != '') so the condition evaluates correctly at runtime for the step that uses actions/upload-artifact@v4; keep the env assignment (SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }}) for the step's environment.
307-311:⚠️ Potential issue | 🟠 MajorConditional check for
env.SPARKLE_PRIVATE_KEYmay not work as intended.The
if: env.SPARKLE_PRIVATE_KEY != ''condition at line 308 reads the environment before the step'senv:block (line 309-310) is evaluated. This means the condition will always see an empty value and skip the step.Use
secrets.SPARKLE_PRIVATE_KEYin the conditional instead:Suggested fix
- name: Sign update archives with Sparkle - if: env.SPARKLE_PRIVATE_KEY != '' + if: ${{ secrets.SPARKLE_PRIVATE_KEY != '' }} env: SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 307 - 311, The conditional on the "Sign update archives with Sparkle" step uses env.SPARKLE_PRIVATE_KEY which is evaluated before the step env block is applied, so replace the condition with secrets.SPARKLE_PRIVATE_KEY != '' (i.e., change if: env.SPARKLE_PRIVATE_KEY != '' to if: secrets.SPARKLE_PRIVATE_KEY != '') and keep the env: SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }} block so the secret is still injected into the step runtime.
♻️ Duplicate comments (1)
.github/workflows/build-plugin.yml (1)
273-275:⚠️ Potential issue | 🟡 MinorGuard
git commitagainst no-op scenarios.If
plugins.jsonis unchanged (e.g., re-running for the same version),git commitexits with code 1, failing the workflow. Add a check before committing.Suggested fix
git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" git add plugins.json - git commit -m "Update $DISPLAY_NAME to v$VERSION" + git diff --cached --quiet || git commit -m "Update $DISPLAY_NAME to v$VERSION" git push🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-plugin.yml around lines 273 - 275, Guard the git commit step so the workflow doesn't fail when plugins.json is unchanged: after staging plugins.json with git add plugins.json, check whether there are any staged changes for plugins.json (e.g., using a git diff --staged or git status --porcelain check) and only run git commit -m "Update $DISPLAY_NAME to v$VERSION" and git push if that check indicates changes; if no staged changes are present, skip the commit/push steps. This targets the commit sequence (git add plugins.json, git commit, git push) to prevent a failing exit when there is nothing to commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 323-327: The GitHub Actions step named "Commit appcast.xml to
repo" uses env.SPARKLE_PRIVATE_KEY in its if condition, which can be empty;
change the conditional to reference the secret directly (use
secrets.SPARKLE_PRIVATE_KEY in the if expression) so the step only runs when the
secret is present, keeping the existing env mapping of SPARKLE_PRIVATE_KEY: ${{
secrets.SPARKLE_PRIVATE_KEY }} unchanged.
- Around line 313-317: The workflow step "Upload appcast artifact" uses the
wrong conditional check; change the step's if: from checking
env.SPARKLE_PRIVATE_KEY to checking the secret directly (if:
secrets.SPARKLE_PRIVATE_KEY != '') so the condition evaluates correctly at
runtime for the step that uses actions/upload-artifact@v4; keep the env
assignment (SPARKLE_PRIVATE_KEY: ${{ secrets.SPARKLE_PRIVATE_KEY }}) for the
step's environment.
- Around line 307-311: The conditional on the "Sign update archives with
Sparkle" step uses env.SPARKLE_PRIVATE_KEY which is evaluated before the step
env block is applied, so replace the condition with secrets.SPARKLE_PRIVATE_KEY
!= '' (i.e., change if: env.SPARKLE_PRIVATE_KEY != '' to if:
secrets.SPARKLE_PRIVATE_KEY != '') and keep the env: SPARKLE_PRIVATE_KEY: ${{
secrets.SPARKLE_PRIVATE_KEY }} block so the secret is still injected into the
step runtime.
---
Duplicate comments:
In @.github/workflows/build-plugin.yml:
- Around line 273-275: Guard the git commit step so the workflow doesn't fail
when plugins.json is unchanged: after staging plugins.json with git add
plugins.json, check whether there are any staged changes for plugins.json (e.g.,
using a git diff --staged or git status --porcelain check) and only run git
commit -m "Update $DISPLAY_NAME to v$VERSION" and git push if that check
indicates changes; if no staged changes are present, skip the commit/push steps.
This targets the commit sequence (git add plugins.json, git commit, git push) to
prevent a failing exit when there is nothing to commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8ac3100-81d8-4c93-90c1-5262662037f6
📒 Files selected for processing (3)
.github/workflows/build-plugin.yml.github/workflows/build.ymlscripts/ci/verify-build.sh
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
macos-latestGitHub-hosted runners/tmp/shared filesystem artifact passing withupload-artifact/download-artifactbuild-plugin.ymlto use matrix strategy for parallel plugin buildssetup-xcodeactionpackage-artifacts.shfor backward compatibilityNew secrets required
Before merging, add these repository secrets:
CERTIFICATES_P12— base64-encoded Developer ID Application certificate (.p12)CERTIFICATES_PASSWORD— password for the .p12 fileAPPLE_ID— Apple ID email for notarizationAPPLE_TEAM_ID— Apple Developer Team IDNOTARY_PASSWORD— app-specific password for notarizationTest plan
v0.0.0-ci-testto triggerbuild.ymland verify all 4 jobs passworkflow_dispatchonbuild-plugin.ymlwith a test plugin tag to verify matrix buildsSummary by CodeRabbit