Skip to content

ci: sign sidecar #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

ci: sign sidecar #7

wants to merge 2 commits into from

Conversation

0xbrayo
Copy link
Member

@0xbrayo 0xbrayo commented Jun 20, 2025

Important

Enhances CI/CD workflow to sign macOS sidecar and DMG files, replacing Tauri action with custom build process.

  • CI/CD Workflow:
    • Replaces Tauri action with make build in build.yml for building the app.
    • Adds macOS sidecar signing step in release.yml using codesign.
    • Adds steps to sign macOS app and create DMG in release.yml.
    • Uploads signed DMG to GitHub releases for macOS in release.yml.
  • Makefile:
    • Updates build target to conditionally set NO_STRIP=true on Linux.
    • Adds logic to handle platform-specific build configurations.

This description was created by Ellipsis for 5f4e0c5. You can customize this summary. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 272a67f in 2 minutes and 20 seconds. Click for details.
  • Reviewed 31 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:125
  • Draft comment:
    Consider adding 'set -eo pipefail' at the start of the run block for robust error handling.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The script is fairly simple and already has explicit error handling. While 'set -eo pipefail' is generally good practice, in this case the script already handles the main error cases explicitly. The script intentionally exits 0 when signing identity is missing (skipping step) and exits 1 when binary is missing. Adding strict mode could interfere with this intended behavior. The suggestion follows shell scripting best practices. Not having strict error handling could potentially mask other unexpected errors. The script's error handling is intentionally designed to handle specific cases differently. Adding strict mode could make the script less flexible and might break the intended skip-on-missing-identity behavior. The comment should be deleted as the script already has appropriate error handling for its specific needs, and adding strict mode could interfere with intended behavior.
2. .github/workflows/release.yml:131
  • Draft comment:
    Ensure SIDECAR_PATH matches the actual sidecar binary location; hardcoding this path might need updates if the build output changes.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_LENSw9MU1qSpjf5j

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

fi

echo "Signing sidecar binary at $SIDECAR_PATH"
codesign --force -s "$APPLE_SIGNING_IDENTITY" --keychain build.keychain --deep "$SIDECAR_PATH"
Copy link

Choose a reason for hiding this comment

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

Consider adding the '--timestamp' flag to the codesign command to improve signature longevity.

Suggested change
codesign --force -s "$APPLE_SIGNING_IDENTITY" --keychain build.keychain --deep "$SIDECAR_PATH"
codesign --force -s "$APPLE_SIGNING_IDENTITY" --keychain build.keychain --deep --timestamp "$SIDECAR_PATH"

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 68ec3d5 in 42 seconds. Click for details.
  • Reviewed 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. Makefile:3
  • Draft comment:
    Consider using Makefile’s $(shell uname) to capture the OS in a variable for clarity and reuse.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. Makefile:7
  • Draft comment:
    Verify that non‐Linux environments never require NO_STRIP; adjust the else branch if similar issues arise.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_idDqSQ4ozZcFwb6p

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 7546ac6 in 1 minute and 24 seconds. Click for details.
  • Reviewed 49 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:122
  • Draft comment:
    If the intent is to conditionally enable sidecar signing, consider using a feature flag or YAML condition (like using an input parameter) instead of commenting out the block.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests using a feature flag, but the code already has one via workflow_dispatch input 'sign'. The commented section even uses this flag in its condition. The author likely commented out this section intentionally as part of removing or changing the sidecar signing process. The comment is suggesting something that's already implemented. Maybe there's a good reason the author chose to comment out rather than use the existing flag - perhaps this is a temporary change during development or debugging. Even if it's temporary, suggesting to use a feature flag when one already exists is not helpful. The author clearly knows about feature flags since they're using one. The comment should be deleted because it suggests implementing something that already exists, making it not useful.

Workflow ID: wflow_xgCgkvQ5huKJkjuP

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@@ -118,6 +118,26 @@ jobs:
echo "APPLE_SIGNING_IDENTITY=$SIGNING_IDENTITY" >> $GITHUB_ENV
echo "Certificate imported successfully. Signing identity: $SIGNING_IDENTITY"

# - name: Sign sidecar (macOS only)
Copy link

Choose a reason for hiding this comment

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

Avoid leaving large blocks of commented-out code without context. If the sidecar signing step isn't needed, remove it; if it's temporarily disabled, add a comment explaining why.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed f32753b in 36 seconds. Click for details.
  • Reviewed 54 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:125
  • Draft comment:
    Consider adding a '--timestamp' option to the codesign command (e.g. in line ~137) to include a secure timestamp, which enhances the signature’s validity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. .github/workflows/release.yml:144
  • Draft comment:
    Good practice: Adding 'CI: true' ensures the build step correctly detects CI environments.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_S70hiEhPcDAog74G

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 202cbc5 in 30 seconds. Click for details.
  • Reviewed 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build.yml:71
  • Draft comment:
    Directly invoking 'make build' replaces the tauri-action step. Confirm that any build arguments (e.g., matrix.args) or environment variables (GITHUB_TOKEN) previously provided are no longer necessary or are handled elsewhere.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to confirm that certain build arguments or environment variables are no longer necessary or are handled elsewhere. This is a request for confirmation, which violates the rule against asking the PR author to confirm their intention or ensure behavior is intended.

Workflow ID: wflow_jrppjMKhFmShOn6l

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 251e4bc in 1 minute and 19 seconds. Click for details.
  • Reviewed 10 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:159
  • Draft comment:
    The 'args' parameter (matrix.args) is now commented out. If this is intentional to disable custom build arguments, please consider removing it entirely or adding a comment explaining the rationale. If it’s needed for some targets, ensure that the removal doesn’t break the intended behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment asks for clarification about intention and suggests either removing or documenting the change. However, this violates our rules about not asking for explanations or confirmations. The matrix.args are clearly defined in the matrix, so this was likely an intentional change. If it breaks something, the CI would catch it. The args parameter might be critical for cross-compilation targets, and commenting it out could break the build process. The comment raises a valid technical concern. While the concern might be valid, if the change breaks cross-compilation, it would be caught by CI. We shouldn't ask for explanations or confirmations - either the change works or it doesn't. Delete the comment as it violates our rules by asking for confirmation of intention and explanation of rationale. If the change causes issues, the CI system will catch it.

Workflow ID: wflow_Q0dAVxtehhz3CKM7

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 8f1b9a1 in 1 minute and 26 seconds. Click for details.
  • Reviewed 42 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:23
  • Draft comment:
    The matrix config now only uses x86_64 for macOS, replacing the aarch64 entry. If aarch64 is no longer needed, consider removing the commented-out aarch64 config to keep the file clean.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While removing commented-out code is generally good practice, the presence of aarch64 target in the rust toolchain setup suggests this might be temporarily commented for testing or future use. The comment is about code cleanliness rather than a functional issue. It's more of a preference than a critical issue. The comment might be premature - the aarch64 target is still configured in the rust toolchain setup, suggesting it might be needed again soon. Also, commented code can sometimes serve as useful documentation or quick toggles in CI configurations. While the comment is technically correct about code cleanliness, it's not critical enough to warrant immediate action, especially in a CI configuration file where commented alternatives can be useful. The comment should be removed as it's more of a style preference than a critical issue, and the commented code might serve a purpose given the broader context.
2. .github/workflows/release.yml:156
  • Draft comment:
    Uncommenting the 'args' parameter in the build step now passes matrix.args to the tauri action. Also consider updating the Rust toolchain targets (line 51) to reflect that only x86_64 is built on macOS.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. gptme:1
  • Draft comment:
    Submodule pointer updated. Please confirm that updating the gptme submodule commit is intentional.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. gptme-webui:1
  • Draft comment:
    Submodule pointer updated. Please confirm that updating the gptme-webui submodule commit is intentional.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_4EN9zBv7Y6wumVsb

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 8b52197 in 1 minute and 27 seconds. Click for details.
  • Reviewed 28 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:23
  • Draft comment:
    The matrix now switches the macOS build from aarch64 to x86_64. Confirm this change is intentional and, if aarch64 support isn’t needed, consider removing the commented-out aarch64 block.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment violates multiple rules. First, it asks to "confirm this change is intentional" which is explicitly against our rules about not asking for confirmations. Second, the suggestion to remove commented code is a style preference that isn't critical to the functionality. The author may want to keep the commented configuration for future reference. The change from ARM to Intel could potentially be a significant architectural decision that impacts users. Maybe this deserves discussion? While the platform change is significant, our rules clearly state not to ask for confirmations of intent. If this was an incorrect change, it would be caught in testing or deployment. Delete the comment as it violates our rules by asking for confirmation of intent and making non-critical style suggestions about commented code.
2. .github/workflows/release.yml:156
  • Draft comment:
    Uncommenting the 'args: ${{ matrix.args }}' line in the release step is a good update to pass build arguments. Ensure that an empty string for non-macOS builds (e.g., Ubuntu) is handled as intended.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_nb91eAfoAVuyUo9J

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 9ab1ff7 in 34 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. package.json:21
  • Draft comment:
    Updated '@tauri-apps/cli' version to '^2.5.0'. Confirm its compatibility with Tauri and sidecar signing.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_sDWpRzmmyWRO0KFK

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 5c338e4 in 1 minute and 5 seconds. Click for details.
  • Reviewed 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:25
  • Draft comment:
    Updated macOS build target now uses aarch64. Verify that the built sidecar (bins/gptme-server-aarch64-apple-darwin) is correct and that subsequent steps (including code signing) align with this architecture.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify the correctness of the build and subsequent steps, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific code suggestion or ask for a specific test to be written.
2. .github/workflows/release.yml:27
  • Draft comment:
    If x86_64 support is not needed, consider removing the commented-out x86_64 configuration to reduce clutter.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_PUxKrqroCCqvBO7x

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Skipped PR review on 53db5a3 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed e774517 in 21 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. package.json:15
  • Draft comment:
    Consider whether the explicit version '2.3.1' for @tauri-apps/plugin-opener is intentional. It replaces the caret range '^2', limiting updates and patch fixes. If intentional for reproducibility, ensure consistency with other plugin versions.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_qQiy6baoRodoaXEg

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 3e60ecb in 1 minute and 53 seconds. Click for details.
  • Reviewed 172 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:6
  • Draft comment:
    Good use of consistent double quotes; improves overall readability.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. .github/workflows/release.yml:126
  • Draft comment:
    Using 'exit 0' when APPLE_SIGNING_IDENTITY is missing causes the step to succeed even if signing was expected. Ensure this behavior is intentional for release builds.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. .github/workflows/release.yml:190
  • Draft comment:
    The notarization step uses the '--wait' flag which can lead to long build delays if notarization stalls. Consider adding a timeout or improved error handling.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment raises a valid point about potential delays, notarization is an essential step for macOS apps and waiting for completion is typically required to ensure proper app distribution. Adding a timeout could cause more problems by interrupting the notarization process. The current implementation follows standard practices. The comment does identify a real potential issue with build times. Long notarization times are a known issue in macOS app distribution. However, waiting for notarization is a necessary evil - interrupting it could leave the app in an invalid state. The current implementation is the standard approach. The comment should be deleted as it suggests changing a standard and necessary practice without providing a clearly better alternative.
4. .github/workflows/release.yml:215
  • Draft comment:
    The release upload step sets 'draft: true'. Confirm that a draft release is desired; otherwise, remove this to publish automatically.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
5. .github/workflows/release.yml:231
  • Draft comment:
    The non-macOS build uses 'tauri-apps/tauri-action@v0'. Check if an updated version is available to benefit from fixes and improvements.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_AIUfwh9Yh36YiMWS

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.


echo "Creating DMG file"
# Create DMG using create-dmg tool
npm install -g create-dmg
Copy link

Choose a reason for hiding this comment

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

Consider pinning the create-dmg npm package version to ensure build reproducibility and avoid unexpected breaking changes.

Suggested change
npm install -g create-dmg
npm install -g create-dmg@4.0.0

@0xbrayo 0xbrayo marked this pull request as draft June 29, 2025 15:07
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 5181e1d in 1 minute and 9 seconds. Click for details.
  • Reviewed 29 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:88
  • Draft comment:
    Caution: Debug info step prints environment variables (e.g. APPLE_SIGNING_IDENTITY). Ensure no sensitive data is exposed in logs.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The APPLE_SIGNING_IDENTITY is not actually sensitive - it's just the name/identifier used for signing, not a secret key. It's commonly visible in build logs. The actual sensitive data (certificates, passwords) are stored in secrets and only used when needed. The debug info step is just printing basic build information to help with troubleshooting. I could be wrong about the sensitivity of the signing identity. Maybe there are security implications I'm not considering. The signing identity is just a public identifier, like "Developer ID Application: Company Name (TEAMID)". The sensitive parts are the certificates and passwords, which are properly handled as secrets elsewhere in the workflow. The comment should be deleted as it raises a false security concern. The debug info being printed is not sensitive.
2. .github/workflows/release.yml:181
  • Draft comment:
    Good: npm install now pins create-dmg to version 4.0.0 for reproducibility.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_0s0jLZKnEesXl5zs

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@0xbrayo 0xbrayo force-pushed the sidecar-signing branch 2 times, most recently from 58e38f3 to d6f5bbf Compare June 29, 2025 16:21
0xbrayo added 2 commits June 29, 2025 20:20
ci: run make build instead of action

ci: explicit code signing without default tauri action
@0xbrayo 0xbrayo marked this pull request as ready for review June 29, 2025 17:22
@0xbrayo 0xbrayo requested a review from ErikBjare June 29, 2025 17:22
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 5f4e0c5 in 1 minute and 37 seconds. Click for details.
  • Reviewed 221 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build.yml:71
  • Draft comment:
    Replacing the tauri-action with a direct 'make build' call: ensure that all required parameters/environment variables are handled by the Makefile target.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that all required parameters/environment variables are handled by the Makefile target. This is a request for confirmation and not a specific code suggestion or request for a test. It violates the rule against asking the author to ensure behavior is intended or tested.
2. .github/workflows/release.yml:121
  • Draft comment:
    The 'Sign sidecar (macOS only)' step builds the path dynamically using matrix.arch. Verify that the generated filename matches the output from the build step to avoid false negatives.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to verify that the generated filename matches the output from the build step. This falls under the category of asking the author to double-check things, which is against the rules.
3. .github/workflows/release.yml:149
  • Draft comment:
    Consider adding a signature verification (e.g. using 'codesign --verify') after signing the app bundle to ensure the signing process succeeded.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is suggesting something that's already present in the code, just commented out. This indicates the authors are already aware of this best practice. The verification step being commented out might be intentional, and the authors likely have their reasons. Making a comment about something that's already in the code (even if commented) isn't particularly helpful. The verification step being commented out could indicate a known issue with verification, or it might have been temporarily disabled for debugging. Maybe this comment could help ensure it gets re-enabled. While verification is important, the presence of the commented code shows the authors are aware of this. If there was a problem with verification, they would need to solve that first, and this comment doesn't add new information. The comment should be deleted as it suggests something that's already present in the code (though commented out), making it redundant and not actionable.
4. Makefile:28
  • Draft comment:
    In the 'gptme-server-build' rule, parsing the architecture from 'rustc -Vv' output may be brittle. Consider using a more robust method to determine the target platform or explicitly passing the expected architecture.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. .github/workflows/release.yml:126
  • Draft comment:
    Exiting with 0 when APPLE_SIGNING_IDENTITY is missing (in both sidecar and app signing steps) might mask configuration issues. Confirm that silently skipping signing is the desired behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code appears to be handling missing certificates intentionally. The main certificate check happens early (lines 97-100), and subsequent checks are just safeguards. Exiting with 0 is reasonable here since missing certificates is an expected case when signing isn't needed. The workflow can continue without signing for development builds. I might be overlooking potential failure scenarios where certificates are partially present or corrupted. There could be valid security concerns about proceeding without signing. The code has proper error handling - it checks certificates early, logs warnings clearly, and the behavior is intentional. This isn't masking issues, it's handling an expected case. The comment should be deleted. The code's error handling is appropriate and intentional, with proper checks and logging.

Workflow ID: wflow_i3D6ibJUJNOyqdUb

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@0xbrayo
Copy link
Member Author

0xbrayo commented Jun 29, 2025

Got the sidecar signing working but only for the .dmg also for some reason the default tauri action keeps failing. I had to run make build and then sign the dmg.

@0xbrayo
Copy link
Member Author

0xbrayo commented Jun 29, 2025

#5

@0xbrayo 0xbrayo marked this pull request as draft June 29, 2025 18:02
@0xbrayo
Copy link
Member Author

0xbrayo commented Jun 29, 2025

Still doesn't work. I tested the wrong version apparently.

@0xbrayo 0xbrayo removed the request for review from ErikBjare June 29, 2025 18:04
@ErikBjare
Copy link
Member

ErikBjare commented Jul 7, 2025

I see you didn't manage to sign the sidecar.

Did you look into using PyInstaller's built-in codesign support?

I think we should ideally keep using the tauri-apps/tauri-action action to make our build as standard as possible.

@0xbrayo
Copy link
Member Author

0xbrayo commented Jul 7, 2025

Will look into the inbuilt Pyinstaller codesign. Will give it another go tonight. The tauri-apps/tauri-action began failing for some reason, even for commits it previously worked for. Specifying versions didn't help either. Will also look into it later.

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.

2 participants