fix(ci): prepare macOS resources before optional signing#119
fix(ci): prepare macOS resources before optional signing#119zouyonghe merged 8 commits intoAstrBotDevs:mainfrom
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The workflow regression test relies on fairly broad regexes over the raw YAML, which makes it brittle to harmless formatting changes or step renames; consider parsing the YAML and asserting on structured fields (step names,
ifconditions, andruncontents) instead. - The assertion that the build step contains the resources comment only checks for the comment text anywhere after the step label; you might want to tighten this check (e.g., anchoring around the specific step block) so that moving or duplicating the comment doesn’t give a false sense of correctness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow regression test relies on fairly broad regexes over the raw YAML, which makes it brittle to harmless formatting changes or step renames; consider parsing the YAML and asserting on structured fields (step names, `if` conditions, and `run` contents) instead.
- The assertion that the build step contains the resources comment only checks for the comment text anywhere after the step label; you might want to tighten this check (e.g., anchoring around the specific step block) so that moving or duplicating the comment doesn’t give a false sense of correctness.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request adds a new test file, build-desktop-tauri-workflow.test.mjs, designed to validate the configuration of the GitHub Actions workflow for macOS desktop builds. The reviewer noted that the current regex-based assertions are fragile and fail to verify the specific order of operations. It is recommended to use step boundaries to ensure commands are executed in the correct sequence and to prevent matches across multiple YAML steps.
| assert.match( | ||
| workflow, | ||
| /- name: Prepare desktop resources \(macOS\)[\s\S]*?run: \|[\s\S]*?pnpm run prepare:resources/, | ||
| ); | ||
| assert.doesNotMatch( | ||
| workflow, | ||
| /- name: Pre-sign backend resources \(macOS\)[\s\S]*?run: \|[\s\S]*?pnpm run prepare:resources/, | ||
| ); | ||
| assert.match( | ||
| workflow, | ||
| /Build desktop app bundle \(macOS\)[\s\S]*?# Resources are already prepared and, when available, pre-signed in earlier steps\./, | ||
| ); |
There was a problem hiding this comment.
The current regex-based assertions are somewhat fragile because [\s\S]*? can match across multiple YAML steps if the expected pattern is missing in the target step but present in a subsequent one. Additionally, the test name implies a specific order of operations ("prepares resources before any conditional pre-signing"), but the current assertions do not verify the relative order of these steps in the workflow file.
Consider using indexOf to identify step boundaries and then asserting on the content within those specific slices. This ensures that the commands are found in the correct steps and that the steps appear in the intended order.
const prepareIdx = workflow.indexOf('- name: Prepare desktop resources (macOS)');
const presignIdx = workflow.indexOf('- name: Pre-sign backend resources (macOS)');
const buildIdx = workflow.indexOf('Build desktop app bundle (macOS)');
assert.ok(prepareIdx !== -1 && presignIdx !== -1 && buildIdx !== -1, 'Required workflow steps not found');
assert.ok(prepareIdx < presignIdx, 'Prepare step should come before Pre-sign step');
assert.ok(presignIdx < buildIdx, 'Pre-sign step should come before Build step');
assert.match(
workflow.slice(prepareIdx, presignIdx),
/run: \|[\s\S]*?pnpm run prepare:resources/,
);
assert.doesNotMatch(
workflow.slice(presignIdx, buildIdx),
/pnpm run prepare:resources/,
);
assert.match(
workflow.slice(buildIdx),
/# Resources are already prepared and, when available, pre-signed in earlier steps\./,
);|
@sourcery-ai review |
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
extractWorkflowJobSteps,finalizeCurrentStepunconditionally overwritescurrentStep.runwithrunLines.join(...), which will erase any previously captured single-linerun:value when there was no blockrun: |; consider only replacingrunwhen a block has actually been collected. - The workflow parser in
build-desktop-tauri-workflow.test.mjsis tightly coupled to the current YAML indentation and exact job/step formatting; to make these tests more resilient to harmless formatting changes, consider parsing the YAML structure (e.g., with a YAML parser) or at least relaxing the reliance on exact prefixes and spacing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `extractWorkflowJobSteps`, `finalizeCurrentStep` unconditionally overwrites `currentStep.run` with `runLines.join(...)`, which will erase any previously captured single-line `run:` value when there was no block `run: |`; consider only replacing `run` when a block has actually been collected.
- The workflow parser in `build-desktop-tauri-workflow.test.mjs` is tightly coupled to the current YAML indentation and exact job/step formatting; to make these tests more resilient to harmless formatting changes, consider parsing the YAML structure (e.g., with a YAML parser) or at least relaxing the reliance on exact prefixes and spacing.
## Individual Comments
### Comment 1
<location path="scripts/ci/build-desktop-tauri-workflow.test.mjs" line_range="18" />
<code_context>
+const STEP_FIELD_PREFIX = ' ';
+const RUN_BLOCK_PREFIX = ' ';
+
+const extractWorkflowJobSteps = (workflow, jobName) => {
+ const lines = workflow.split(/\r?\n/);
+ const jobHeader = ` ${jobName}:`;
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the custom indentation-based workflow parsing with YAML parsing and direct assertions on the jobs/steps structure to simplify the tests and make them more robust to future changes.
You can drop most of the custom parsing by loading the workflow as YAML and asserting directly on `jobs['build-macos'].steps`. That keeps all current behavior (step name/if/run assertions, ordering) while removing the indentation-based state machine.
Example refactor:
```ts
// Add a dependency once (e.g. devDependency):
// pnpm add -D yaml
import { readFile } from 'node:fs/promises';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import assert from 'node:assert/strict';
import { test } from 'node:test';
import { parse } from 'yaml';
const scriptDir = path.dirname(fileURLToPath(import.meta.url));
const projectRoot = path.resolve(scriptDir, '..', '..');
const workflowPath = path.join(projectRoot, '.github', 'workflows', 'build-desktop-tauri.yml');
const readWorkflowObject = async () => {
const content = await readFile(workflowPath, 'utf8');
return parse(content);
};
const extractWorkflowJobSteps = (workflowObj: any, jobName: string) => {
assert.ok(workflowObj.jobs, 'Expected workflow to define jobs.');
const job = workflowObj.jobs[jobName];
assert.ok(job, `Expected workflow job ${jobName} to exist.`);
assert.ok(Array.isArray(job.steps), `Expected workflow job ${jobName} to define steps.`);
return job.steps;
};
const findStepByName = (steps: any[], stepName: string) => {
const step = steps.find((s) => s.name === stepName);
assert.ok(step, `Expected workflow step ${stepName} to exist.`);
return step;
};
```
Your tests can stay almost identical:
```ts
test('macOS workflow prepares resources before optional pre-signing', async () => {
const workflowObj = await readWorkflowObject();
const steps = extractWorkflowJobSteps(workflowObj, 'build-macos');
const prepareStep = findStepByName(steps, 'Prepare desktop resources (macOS)');
const preSignStep = findStepByName(steps, 'Pre-sign backend resources (macOS)');
const buildStep = findStepByName(steps, 'Build desktop app bundle (macOS)');
assert.equal(prepareStep.if, undefined);
assert.match(prepareStep.run, /pnpm run prepare:resources/);
assert.match(prepareStep.run, /resources\/backend not found after prepare:resources/);
assert.equal(preSignStep.if, "${{ steps.import_apple_certificate.outputs.signing_identity != '' }}");
assert.doesNotMatch(preSignStep.run, /pnpm run prepare:resources/);
assert.match(
buildStep.run,
/# Resources are already prepared and, when available, pre-signed in earlier steps\./,
);
});
```
This removes:
- `TOP_LEVEL_JOB_PATTERN`, `MACOS_STEP_PREFIX`, `STEP_FIELD_PREFIX`, `RUN_BLOCK_PREFIX`
- `currentStep`, `collectingRunBlock`, `runLines`, and `finalizeCurrentStep`
while keeping the same assertions and semantics, and it will continue to work even if you add more fields to steps later.
</issue_to_address>Hi @zouyonghe! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
extractWorkflowJobSteps,finalizeCurrentStepalways overwritescurrentStep.runwithrunLines.join(...), which means any single-linerun: fooassignments are lost (becauserunLinesstays empty); consider only replacingrunwhenrunLines.length > 0so both block and single-linerunsyntaxes are handled correctly. - The workflow parser in
build-desktop-tauri-workflow.test.mjsis tightly coupled to current indentation and formatting (e.g.,MACOS_STEP_PREFIX,STEP_FIELD_PREFIX, exact job header lines), which will make the tests brittle to cosmetic YAML changes; consider parsing the workflow as YAML and then asserting on the structuredjobs.build-macos.stepsarray instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `extractWorkflowJobSteps`, `finalizeCurrentStep` always overwrites `currentStep.run` with `runLines.join(...)`, which means any single-line `run: foo` assignments are lost (because `runLines` stays empty); consider only replacing `run` when `runLines.length > 0` so both block and single-line `run` syntaxes are handled correctly.
- The workflow parser in `build-desktop-tauri-workflow.test.mjs` is tightly coupled to current indentation and formatting (e.g., `MACOS_STEP_PREFIX`, `STEP_FIELD_PREFIX`, exact job header lines), which will make the tests brittle to cosmetic YAML changes; consider parsing the workflow as YAML and then asserting on the structured `jobs.build-macos.steps` array instead.
## Individual Comments
### Comment 1
<location path="scripts/ci/build-desktop-tauri-workflow.test.mjs" line_range="18" />
<code_context>
+const STEP_FIELD_PREFIX = ' ';
+const RUN_BLOCK_PREFIX = ' ';
+
+const extractWorkflowJobSteps = (workflow, jobName) => {
+ const lines = workflow.split(/\r?\n/);
+ const jobHeader = ` ${jobName}:`;
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the ad-hoc line/indentation-based workflow parsing with YAML-based parsing or smaller, focused helpers to make the tests simpler and more robust.
You can reduce a lot of the complexity here by leaning on YAML parsing and/or decomposing the parsing into smaller helpers, while keeping the same assertions and behavior.
### 1. Replace ad‑hoc parsing with a YAML parser
Instead of tracking `jobStart`, `stepsStart`, prefixes, and block state, parse the workflow as YAML and navigate the resulting object. This makes the test robust to indentation and formatting changes.
For example, using `js-yaml` as a dev dependency:
```ts
// import at top
import yaml from 'js-yaml';
const extractWorkflowJobSteps = (workflow: string, jobName: string) => {
const doc = yaml.load(workflow) as any;
assert.ok(doc && doc.jobs, 'Expected workflow to define jobs');
const job = doc.jobs[jobName];
assert.ok(job, `Expected workflow job ${jobName} to exist.`);
assert.ok(Array.isArray(job.steps), `Expected workflow job ${jobName} to define steps.`);
// Normalize steps to your current shape
return job.steps.map((s: any) => ({
name: s.name ?? null,
if: s.if ?? null,
run: typeof s.run === 'string' ? s.run : '',
}));
};
```
This preserves the existing test semantics (`name`, `if`, `run` fields, order of steps) without any manual indentation/prefix logic.
### 2. If you want to keep string parsing, split responsibilities
If introducing a YAML dependency isn’t desirable, you can still reduce complexity by decomposing `extractWorkflowJobSteps`:
```ts
const getJobBlockLines = (workflow: string, jobName: string): string[] => {
const lines = workflow.split(/\r?\n/);
const jobHeader = ` ${jobName}:`;
const start = lines.findIndex((line) => line === jobHeader);
assert.notEqual(start, -1, `Expected workflow job ${jobName} to exist.`);
let end = lines.length;
for (let i = start + 1; i < lines.length; i += 1) {
if (TOP_LEVEL_JOB_PATTERN.test(lines[i])) {
end = i;
break;
}
}
return lines.slice(start, end);
};
const getStepsBlockLines = (jobLines: string[]): string[] => {
const stepsIndex = jobLines.findIndex((line) => line.trim() === 'steps:');
assert.notEqual(stepsIndex, -1, 'Expected job to define steps.');
return jobLines.slice(stepsIndex + 1);
};
const parseStepLines = (stepLines: string[]): Array<{ name: string|null; if: string|null; run: string }> => {
// keep a simplified version of your current logic here,
// focused only on interpreting step fields and run blocks
};
```
Then `extractWorkflowJobSteps` becomes a simple composition:
```ts
const extractWorkflowJobSteps = (workflow: string, jobName: string) => {
const jobLines = getJobBlockLines(workflow, jobName);
const stepLines = getStepsBlockLines(jobLines);
return parseStepLines(stepLines);
};
```
This isolates concerns and makes it easier to reason about each part, while leaving behavior intact.
### 3. Simplify `run` block handling
Even within `parseStepLines`, you can remove `runLines`/`collectingRunBlock` juggling by localizing block accumulation:
```ts
const collectRunBlock = (lines: string[], startIndex: number): { run: string; nextIndex: number } => {
const blockLines: string[] = [];
let i = startIndex;
while (i < lines.length && (lines[i].startsWith(RUN_BLOCK_PREFIX) || lines[i].trim() === '')) {
blockLines.push(lines[i].startsWith(RUN_BLOCK_PREFIX)
? lines[i].slice(RUN_BLOCK_PREFIX.length)
: '');
i += 1;
}
return { run: blockLines.join('\n').trimEnd(), nextIndex: i };
};
```
Use it inside your step loop:
```ts
if (trimmed === 'run: |') {
const result = collectRunBlock(stepLines, index + 1);
currentStep.run = result.run;
index = result.nextIndex - 1; // loop will ++
continue;
}
```
This removes the need for `collectingRunBlock` and `runLines` on the step object while preserving multiline behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The workflow structure tests are quite brittle because they depend on exact step names, comments, and the literal
ifexpression string; consider relaxing these assertions (e.g., using partial matches or checking for the presence of key substrings/fields) so normal refactors of labels or comments don’t cause spurious test failures. - In
build-desktop-tauri-workflow.test.mjs,findStepByNamedoes a strict equality match onname, which may break if additional context is added to step names; consider allowing partial or pattern-based matching to make the test more tolerant of minor label changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow structure tests are quite brittle because they depend on exact step names, comments, and the literal `if` expression string; consider relaxing these assertions (e.g., using partial matches or checking for the presence of key substrings/fields) so normal refactors of labels or comments don’t cause spurious test failures.
- In `build-desktop-tauri-workflow.test.mjs`, `findStepByName` does a strict equality match on `name`, which may break if additional context is added to step names; consider allowing partial or pattern-based matching to make the test more tolerant of minor label changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The YAML workflow tests duplicate helper logic (
readWorkflowObject,extractWorkflowJobSteps); consider extracting these into a shared test utility to reduce repetition and keep future workflow assertions consistent. - The workflow tests assert on specific job and step names (e.g.,
'build-macos','Prepare desktop resources'), which makes them brittle to purely cosmetic renames; you might want to key off more stable identifiers (likeuses/runpatterns) or centralize the strings to mitigate this. - In
.github/workflows/check-scripts.yml, theecho/node --checklines inside thewhileloop lost indentation, which slightly hurts readability; aligning them with the surrounding shell block would make the script easier to scan.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The YAML workflow tests duplicate helper logic (`readWorkflowObject`, `extractWorkflowJobSteps`); consider extracting these into a shared test utility to reduce repetition and keep future workflow assertions consistent.
- The workflow tests assert on specific job and step names (e.g., `'build-macos'`, `'Prepare desktop resources'`), which makes them brittle to purely cosmetic renames; you might want to key off more stable identifiers (like `uses`/`run` patterns) or centralize the strings to mitigate this.
- In `.github/workflows/check-scripts.yml`, the `echo`/`node --check` lines inside the `while` loop lost indentation, which slightly hurts readability; aligning them with the surrounding shell block would make the script easier to scan.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/check-scripts.yml" line_range="44-47" />
<code_context>
node --check "${file}"
done < <(find scripts -type f \( -name '*.mjs' -o -name '*.cjs' -o -name '*.js' \) -print0 | sort -z)
+ - name: Install Node test dependencies
+ run: |
+ set -euo pipefail
+ pnpm install --frozen-lockfile
+
- name: Run Node script behavior tests
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider disabling lifecycle scripts during CI installs for a tighter security posture.
Because this step only needs deps to run tests, you can reduce exposure to arbitrary `postinstall`/`preinstall` scripts in CI by adding `--ignore-scripts` to the install command (e.g. `pnpm install --frozen-lockfile --ignore-scripts`).
```suggestion
- name: Install Node test dependencies
run: |
set -euo pipefail
pnpm install --frozen-lockfile --ignore-scripts
```
</issue_to_address>
### Comment 2
<location path=".github/workflows/check-scripts.yml" line_range="27-30" />
<code_context>
node-version: '20'
python-version: '3.12'
+ - name: Setup pnpm
+ uses: pnpm/action-setup@v4.4.0
+ with:
+ version: 10.28.2
+
- name: Check Node scripts syntax
</code_context>
<issue_to_address>
**suggestion (performance):** You can hook pnpm into the existing Node cache to speed up installs.
Since this workflow now uses pnpm, consider integrating it with the existing Node setup step (e.g., `actions/setup-node` with `cache: 'pnpm'`) so the pnpm store is cached between runs and CI installs are faster without changing behavior.
Suggested implementation:
```
node-version: '20'
cache: 'pnpm'
cache-dependency-path: pnpm-lock.yaml
python-version: '3.12'
```
1. Ensure the step containing `node-version: '20'` is indeed an `actions/setup-node@v4` (or compatible) step; if not, adjust so that these `cache` fields are under the `with:` for `actions/setup-node`.
2. If your pnpm lockfile has a different name or location (e.g., `frontend/pnpm-lock.yaml`), update `cache-dependency-path` accordingly, or use a multiline value to include all relevant lockfiles.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The workflow tests currently assert very specific step names and full
runcommand strings (e.g., exactpnpm install --frozen-lockfile --ignore-scripts), which may make future minor refactors noisy; consider relaxing these assertions to focus on key behaviors (e.g., presence of pnpm install with relevant flags and relative ordering) rather than full string matches. - In
build-desktop-tauri-workflow.test.mjs, the step ordering checks usesteps.indexOf(...)repeatedly even thoughfindStepIndexexists and is used elsewhere; consider switching tofindStepIndexhere for consistency and clearer failure messages when steps are not found.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow tests currently assert very specific step names and full `run` command strings (e.g., exact `pnpm install --frozen-lockfile --ignore-scripts`), which may make future minor refactors noisy; consider relaxing these assertions to focus on key behaviors (e.g., presence of pnpm install with relevant flags and relative ordering) rather than full string matches.
- In `build-desktop-tauri-workflow.test.mjs`, the step ordering checks use `steps.indexOf(...)` repeatedly even though `findStepIndex` exists and is used elsewhere; consider switching to `findStepIndex` here for consistency and clearer failure messages when steps are not found.
## Individual Comments
### Comment 1
<location path=".github/workflows/build-desktop-tauri.yml" line_range="318-321" />
<code_context>
- - name: Pre-sign backend resources (macOS)
- if: ${{ steps.import_apple_certificate.outputs.signing_identity != '' }}
+ - name: Prepare desktop resources (macOS)
env:
ASTRBOT_SOURCE_GIT_URL: ${{ needs.resolve_build_context.outputs.source_git_url }}
</code_context>
<issue_to_address>
**suggestion:** Align shell configuration for steps that rely on bash-specific options like `pipefail`
This step uses `set -euo pipefail` but doesn’t declare `shell: bash`, while the following "Pre-sign backend resources" step does for the same pattern. Please also set `shell: bash` here to avoid dependence on the current macOS default and keep the related steps consistent.
Suggested implementation:
```
- name: Prepare desktop resources (macOS)
shell: bash
env:
ASTRBOT_SOURCE_GIT_URL: ${{ needs.resolve_build_context.outputs.source_git_url }}
ASTRBOT_SOURCE_GIT_REF: ${{ needs.resolve_build_context.outputs.source_git_ref }}
```
From the snippet, it looks like the `run: |` block for "Prepare desktop resources (macOS)" is omitted or partially shown. Ensure that this step’s script (the part that uses `set -euo pipefail`) is indeed under this step and remains unchanged, now executing under bash due to the new `shell: bash` line.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - name: Prepare desktop resources (macOS) | ||
| env: | ||
| ASTRBOT_SOURCE_GIT_URL: ${{ needs.resolve_build_context.outputs.source_git_url }} | ||
| ASTRBOT_SOURCE_GIT_REF: ${{ needs.resolve_build_context.outputs.source_git_ref }} |
There was a problem hiding this comment.
suggestion: Align shell configuration for steps that rely on bash-specific options like pipefail
This step uses set -euo pipefail but doesn’t declare shell: bash, while the following "Pre-sign backend resources" step does for the same pattern. Please also set shell: bash here to avoid dependence on the current macOS default and keep the related steps consistent.
Suggested implementation:
- name: Prepare desktop resources (macOS)
shell: bash
env:
ASTRBOT_SOURCE_GIT_URL: ${{ needs.resolve_build_context.outputs.source_git_url }}
ASTRBOT_SOURCE_GIT_REF: ${{ needs.resolve_build_context.outputs.source_git_ref }}
From the snippet, it looks like the run: | block for "Prepare desktop resources (macOS)" is omitted or partially shown. Ensure that this step’s script (the part that uses set -euo pipefail) is indeed under this step and remains unchanged, now executing under bash due to the new shell: bash line.
|
@sourcery-ai review |
…#119) * fix(ci): prepare macOS resources before optional signing * test(ci): parse macOS workflow steps structurally * test(ci): parse workflow YAML structurally * test(ci): relax workflow assertions and install test deps * test(ci): share workflow test helpers * test(ci): harden script workflow dependency setup * fix(ci): setup pnpm before enabling pnpm cache * test(ci): relax workflow step assertions
Summary
prepare:resourcesinto an unconditional macOS workflow step so unsigned builds still materializeresources/backendandresources/webuiTest Plan
Summary by Sourcery
Ensure macOS desktop builds always prepare resources before optional code signing and strengthen CI workflow validation for Node-based scripts and actions.
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: