Skip to content

ci: add custom source-ref desktop builds#96

Merged
zouyonghe merged 12 commits intoAstrBotDevs:mainfrom
zouyonghe:main
Mar 18, 2026
Merged

ci: add custom source-ref desktop builds#96
zouyonghe merged 12 commits intoAstrBotDevs:mainfrom
zouyonghe:main

Conversation

@zouyonghe
Copy link
Member

@zouyonghe zouyonghe commented Mar 18, 2026

Summary

  • add a workflow_dispatch custom build mode for explicit source_git_ref desktop builds
  • resolve custom source refs to a pinned commit SHA and publish isolated prerelease tags instead of reusing nightly
  • keep updater manifest generation and prerelease demotion limited to non-custom / nightly flows

Testing

  • node --test scripts/ci/resolve-build-context.test.mjs

Summary by Sourcery

Introduce a custom desktop build mode that pins explicit source refs to commit SHAs and ensure desktop version sync also updates Cargo.lock robustly.

New Features:

  • Add a workflow_dispatch-only desktop build_mode=custom that builds from an explicit source_git_ref and resolves it to a pinned commit SHA.
  • Generate custom build versions, tags, and release names isolated from nightly releases.

Enhancements:

  • Extend desktop version syncing to update the Cargo.lock entry for the desktop Tauri crate while preserving formatting and handling malformed or missing entries gracefully.
  • Update the desktop build workflow to include Cargo.lock in version-sync commits and to skip updater manifest generation and prerelease demotion for custom builds.
  • Enhance the fake git test fixture to support rev-parse for simulating pinned commit SHAs.

Tests:

  • Add comprehensive tests for Cargo.lock version syncing behavior across various edge cases, including extra fields, comments, missing packages, and malformed entries.
  • Add CI resolve-build-context tests for the new custom build mode, event restrictions, error handling, and fake git rev-parse behavior.

@gemini-code-assist
Copy link

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the CI/CD pipeline by introducing a "custom" build mode. This new mode allows developers to trigger desktop builds from any specified Git reference, such as a branch or tag, and ensures these builds are pinned to a precise commit SHA. It also establishes a separate prerelease tagging scheme for these custom builds, preventing them from interfering with or being promoted as standard nightly or release builds.

Highlights

  • New Custom Build Mode: Introduced a new custom build mode for workflow_dispatch events, allowing explicit source_git_ref desktop builds.
  • Source Reference Resolution: Implemented logic to resolve custom source references to specific Git commit SHAs, ensuring builds are pinned to exact states.
  • Isolated Prerelease Tags: Configured custom builds to publish isolated prerelease tags, distinct from standard nightly builds.
  • Build Flow Restrictions: Restricted updater manifest generation and prerelease demotion processes to only non-custom and nightly build flows.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/build-desktop-tauri.yml
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a custom build mode to the CI workflow, allowing builds from an explicit source git ref. The changes are well-structured, adding the necessary logic to handle this new mode, including resolving the ref to a pinned commit SHA and generating unique prerelease tags. The new functionality is also accompanied by corresponding tests, which is great. I have one suggestion to improve code clarity and reduce duplication in the main shell script.

Comment on lines +288 to +291
if [ "${requested_build_mode}" = "custom" ]; then
echo "::error::${GITHUB_EVENT_NAME} runs do not support build_mode=custom." >&2
exit 1
fi

Choose a reason for hiding this comment

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

medium

This check for build_mode=custom is duplicated in the schedule case (lines 249-252). To make the script more DRY, consider consolidating this validation. You could add a single check before the case "${GITHUB_EVENT_NAME}" statement (around line 224) to ensure custom mode is only used with workflow_dispatch events.

For example:

if [ "${requested_build_mode}" = "custom" ] && [ "${GITHUB_EVENT_NAME}" != "workflow_dispatch" ]; then
  echo "::error::build_mode=custom is only supported for workflow_dispatch events, not for '${GITHUB_EVENT_NAME}'." >&2
  exit 1
fi

case "${GITHUB_EVENT_NAME}" in
  # ...

This would allow you to remove the custom mode checks from both the schedule and * branches of the case statement, centralizing the validation logic.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The Cargo.lock update in syncDesktopVersionFiles relies on a very specific [[package]] block layout (name and version adjacent with only whitespace), which may break if Cargo changes the lockfile formatting or inserts additional fields; consider using a more robust parser or a looser regex that tolerates extra lines between name and version for the target package.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The Cargo.lock update in `syncDesktopVersionFiles` relies on a very specific `[[package]]` block layout (name and version adjacent with only whitespace), which may break if Cargo changes the lockfile formatting or inserts additional fields; consider using a more robust parser or a looser regex that tolerates extra lines between `name` and `version` for the target package.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@SourceryAI SourceryAI left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The updateCargoLockPackageVersion logic relies on fairly strict regex patterns for [[package]] blocks and name/version lines; consider relaxing these (e.g., allow trailing spaces or comments) or using a Cargo.lock/TOML parser to avoid brittle failures if the file format changes slightly.
  • The fake git implementation for rev-parse only supports HEAD; if future changes call rev-parse with other refs, this will start failing tests, so it may be worth generalizing this stub now (e.g., accept arbitrary refs and always return the test SHA).
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `updateCargoLockPackageVersion` logic relies on fairly strict regex patterns for `[[package]]` blocks and `name/version` lines; consider relaxing these (e.g., allow trailing spaces or comments) or using a Cargo.lock/TOML parser to avoid brittle failures if the file format changes slightly.
- The fake git implementation for `rev-parse` only supports `HEAD`; if future changes call `rev-parse` with other refs, this will start failing tests, so it may be worth generalizing this stub now (e.g., accept arbitrary refs and always return the test SHA).

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.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The Cargo.lock update logic hardcodes the package name "astrbot-desktop-tauri" in both updateCargoLockPackageVersion and the tests; consider extracting this into a shared constant to avoid divergence if the crate name ever changes.
  • In fake-git.sh, the new rev-parse handler only supports HEAD and otherwise errors; you may want to either assert the expected ref in the calling tests or broaden the handler to handle arbitrary refs to avoid brittle failures if the script is reused.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The Cargo.lock update logic hardcodes the package name "astrbot-desktop-tauri" in both `updateCargoLockPackageVersion` and the tests; consider extracting this into a shared constant to avoid divergence if the crate name ever changes.
- In `fake-git.sh`, the new `rev-parse` handler only supports `HEAD` and otherwise errors; you may want to either assert the expected ref in the calling tests or broaden the handler to handle arbitrary refs to avoid brittle failures if the script is reused.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • In syncDesktopVersionFiles, updateCargoLockPackageVersion throws if the target crate/package/version lines are not found, which will fail the whole version-sync step whenever Cargo.lock exists but doesn’t contain astrbot-desktop-tauri (e.g., workspace or partially checked-in lockfiles); consider handling this case more leniently (logging and skipping the update) so the job doesn’t hard-fail in those scenarios.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `syncDesktopVersionFiles`, `updateCargoLockPackageVersion` throws if the target crate/package/version lines are not found, which will fail the whole version-sync step whenever `Cargo.lock` exists but doesn’t contain `astrbot-desktop-tauri` (e.g., workspace or partially checked-in lockfiles); consider handling this case more leniently (logging and skipping the update) so the job doesn’t hard-fail in those scenarios.

## Individual Comments

### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="108-113" />
<code_context>
+    blockStartIndex = index;
+  }
+
+  if (!foundTargetPackage || !foundTargetVersion) {
+    throw new Error(`Cannot update Cargo.lock package version for ${packageName}`);
+  }
</code_context>
<issue_to_address>
**suggestion:** Differentiate error causes when updating Cargo.lock to aid debugging.

Since you already track `foundTargetPackage` and `foundTargetVersion`, consider throwing distinct errors for each condition. That way callers can tell whether the package is missing from Cargo.lock or the expected version line/layout is missing, which will make failures easier to diagnose.

```suggestion
    blockStartIndex = index;
  }

  if (!foundTargetPackage) {
    throw new Error(`Cannot update Cargo.lock: package "${packageName}" not found`);
  }

  if (!foundTargetVersion) {
    throw new Error(
      `Cannot update Cargo.lock: version entry for package "${packageName}" not found or has an unexpected layout`
    );
  }
```
</issue_to_address>

### Comment 2
<location path="scripts/prepare-resources/version-sync.mjs" line_range="54" />
<code_context>

+const escapeRegExp = (value) => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
+const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
+  const packageHeaderPattern = /^\s*\[\[package\]\]\s*(?:#.*)?$/;
+  const packageNamePattern = new RegExp(
</code_context>
<issue_to_address>
**issue (complexity):** Consider rewriting the Cargo.lock update logic as a single-pass state machine instead of using a nested helper and block indices to track package sections.

The nested `updateBlock` plus `blockStartIndex` scan can be flattened into a single pass that keeps the same behavior (comments preserved, extra fields allowed, only first matching package/version updated, error if name or version missing), while reducing state and indirection.

You can keep the existing regexes and CRLF handling, but replace the block logic with a simple streaming state machine:

```ts
const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
  const packageHeaderPattern = /^\s*\[\[package\]\]\s*(?:#.*)?$/;
  const packageNamePattern = new RegExp(
    `^\\s*name\\s*=\\s*"${escapeRegExp(packageName)}"\\s*(?:#.*)?$`,
  );
  const packageVersionPattern = /^(\s*version\s*=\s*")[^"]+(")(\s*(?:#.*)?)$/;

  const lines = cargoLock.split(/\r?\n/);

  let inPackageBlock = false;
  let inTargetPackage = false;
  let foundTargetPackage = false;
  let foundTargetVersion = false;

  for (let i = 0; i < lines.length; i += 1) {
    const line = lines[i];

    // Detect start of a new [[package]] block
    if (packageHeaderPattern.test(line)) {
      inPackageBlock = true;
      inTargetPackage = false;
      continue;
    }

    if (!inPackageBlock) {
      continue;
    }

    if (!inTargetPackage) {
      if (packageNamePattern.test(line)) {
        inTargetPackage = true;
        foundTargetPackage = true;
      }
      continue;
    }

    // We are inside the target package block
    if (packageHeaderPattern.test(line)) {
      // Safety: new header before version -> stop looking in this block
      inPackageBlock = true;
      inTargetPackage = false;
      continue;
    }

    if (packageVersionPattern.test(line)) {
      lines[i] = line.replace(packageVersionPattern, `$1${version}$2$3`);
      foundTargetVersion = true;
      break;
    }
  }

  if (!foundTargetPackage || !foundTargetVersion) {
    throw new Error(`Cannot update Cargo.lock package version for ${packageName}`);
  }

  return lines.join(cargoLock.includes('\r\n') ? '\r\n' : '\n');
};
```

Key changes:

- Single loop over `lines` instead of `blockStartIndex` + `updateBlock`.
- State reduced to `inPackageBlock` and `inTargetPackage` (plus the two boolean outcome flags).
- Header detection is no longer duplicated in the inner closure.
- No closure mutating outer-scope variables, making control flow easier to follow while preserving all existing behavior.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="62" />
<code_context>
+  return error;
+};
+
+const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
+  const packageHeaderPattern = /^\s*\[\[package\]\]\s*(?:#.*)?$/;
+  const packageNamePattern = new RegExp(
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the Cargo.lock update logic to use block-based parsing, simpler line handling, and minimal error types to make the code easier to follow while preserving behavior.

You can reduce the cognitive load of the new logic without losing any behavior by:

1. **Making `updateCargoLockPackageVersion` block-based instead of flag-based**
2. **Dropping the extra regex complexity for the version line**
3. **Trimming the error plumbing to just what’s needed**

### 1. Refactor to block-based processing

Instead of maintaining `inPackageBlock` / `inTargetPackage` / `foundTargetVersion` flags, you can work in small “package block” slices.

Example sketch (focus on structure, not exact edge cases):

```js
const PACKAGE_HEADER = /^\s*\[\[package\]\]/;

const findPackageBlocks = (lines) => {
  const blocks = [];
  let start = null;

  for (let i = 0; i < lines.length; i++) {
    if (PACKAGE_HEADER.test(lines[i])) {
      if (start !== null) {
        blocks.push({ start, end: i });
      }
      start = i;
    }
  }
  if (start !== null) {
    blocks.push({ start, end: lines.length });
  }
  return blocks;
};

const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
  const lines = cargoLock.split(/\r?\n/);
  const blocks = findPackageBlocks(lines);
  const nameLinePattern = new RegExp(
    `^\\s*name\\s*=\\s*"${escapeRegExp(packageName)}"\\s*(?:#.*)?$`
  );

  let foundPackage = false;
  let updated = false;

  for (const { start, end } of blocks) {
    const blockLines = lines.slice(start, end);
    if (!blockLines.some((line) => nameLinePattern.test(line))) continue;

    foundPackage = true;

    for (let i = start; i < end; i++) {
      const line = lines[i];
      const trimmed = line.trimStart();
      if (!trimmed.startsWith('version')) continue;

      lines[i] = updateVersionLine(line, version);
      updated = true;
      break;
    }

    break; // only one matching package expected
  }

  if (!foundPackage) {
    throw new CargoLockPackageNotFoundError(packageName);
  }
  if (!updated) {
    throw new Error(
      `Cannot update Cargo.lock: version entry for package "${packageName}" not found or has an unexpected layout`
    );
  }

  return lines.join(cargoLock.includes('\r\n') ? '\r\n' : '\n');
};
```

This removes the implicit state machine and makes the control flow “find block → if name matches → update version line”.

### 2. Simplify version-line handling (no regex replacement groups)

You can replace `packageVersionPattern` + `.replace()` with a small helper that preserves spacing and comments:

```js
const updateVersionLine = (line, version) => {
  const [beforeComment, comment = ''] = line.split('#', 2);
  const [left, rightRaw = ''] = beforeComment.split('=', 2);

  if (!rightRaw) return line; // unexpected layout, let caller decide

  const rhsPrefix = rightRaw.match(/^\s*["']?/u)?.[0] ?? '';
  const quote = rhsPrefix.includes('"') || rhsPrefix.includes("'") ? rhsPrefix.trim().slice(0, 1) : '"';

  const newBeforeComment = `${left.trimEnd()} = ${quote}${version}${quote}`;
  return comment ? `${newBeforeComment} #${comment}` : newBeforeComment;
};
```

Then `updateCargoLockPackageVersion` just calls `updateVersionLine(line, version)` instead of the capturing-group regex replacement.

### 3. Simplify error plumbing

You only treat “package not found” specially, so you can:

- Use a small local error subclass, and
- Drop the generic factory and the unused `CARGO_LOCK_VERSION_NOT_FOUND`.

```js
class CargoLockPackageNotFoundError extends Error {
  constructor(packageName) {
    super(`Cannot update Cargo.lock: package "${packageName}" not found`);
    this.code = 'cargo-lock-package-not-found';
  }
}

// usage in updateCargoLockPackageVersion:
if (!foundPackage) {
  throw new CargoLockPackageNotFoundError(packageName);
}
```

Caller stays almost the same:

```js
try {
  updatedCargoLock = updateCargoLockPackageVersion({ ... });
} catch (error) {
  if (error?.code === 'cargo-lock-package-not-found') {
    console.warn(`${cargoLockPath}: ${error.message}. Skipping Cargo.lock version sync.`);
  } else {
    throw error;
  }
}
```

This keeps all current behavior but reduces:

- Boolean flag juggling and control-flow branches,
- Regex complexity,
- Extra abstraction around error creation/codes.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="134-137" />
<code_context>
+      continue;
+    }
+
+    for (let index = packageNameLineIndex + 1; index < end; index += 1) {
+      if (!lines[index].trimStart().startsWith('version')) {
+        continue;
+      }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tighten version-line detection to avoid matching unintended keys.

This condition will also match any key starting with `version` (e.g. `versioned_dep = ...`), which could cause incorrect edits if similar keys are added in `Cargo.lock`. Consider making the check stricter by requiring an `=` and a word boundary, e.g. testing the line against `/^\s*version\s*=/` instead of using `startsWith`.

```suggestion
    for (let index = packageNameLineIndex + 1; index < end; index += 1) {
      if (!/^\s*version\s*=/.test(lines[index])) {
        continue;
      }
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • In updateVersionLine, switching the quote style based on the presence of a single quote in the existing value risks generating single-quoted strings, which are not valid in Cargo.lock/TOML; it would be safer to always use double quotes and escape as needed while still preserving trailing whitespace and comments.
  • The custom build-mode validation logic for unsupported GITHUB_EVENT_NAME values (schedule, others) is spread across multiple case branches in resolve-build-context.sh; consider centralizing the build_mode=custom eligibility checks to avoid divergence if new event types or modes are added.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `updateVersionLine`, switching the quote style based on the presence of a single quote in the existing value risks generating single-quoted strings, which are not valid in Cargo.lock/TOML; it would be safer to always use double quotes and escape as needed while still preserving trailing whitespace and comments.
- The custom build-mode validation logic for unsupported `GITHUB_EVENT_NAME` values (`schedule`, others) is spread across multiple case branches in `resolve-build-context.sh`; consider centralizing the `build_mode=custom` eligibility checks to avoid divergence if new event types or modes are added.

## Individual Comments

### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="63" />
<code_context>
+  }
+}
+
+const findCargoLockPackageBlocks = (lines) => {
+  const blocks = [];
+  let start = null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the Cargo.lock update logic into a single linear pass with a status flag instead of multiple helpers and custom error types.

You can keep the new behavior but reduce complexity by collapsing the “block parsing + line rewriting + custom error” machinery into a single linear pass with simpler error handling.

### 1. Replace block segmentation with a single‑pass state machine

Instead of `findCargoLockPackageBlocks` + nested loops, track state as you stream through the lines once. This removes `blocks`, `start/end`, and the extra helper.

```js
const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
  const lines = cargoLock.split(/\r?\n/);
  const namePattern = new RegExp(
    `^\\s*name\\s*=\\s*"${escapeRegExp(packageName)}"\\s*(?:#.*)?$`,
  );
  const newline = cargoLock.includes('\r\n') ? '\r\n' : '\n';

  let inPackage = false;
  let inTargetPackage = false;
  let updated = false;

  for (let i = 0; i < lines.length; i += 1) {
    const line = lines[i];

    if (CARGO_LOCK_PACKAGE_HEADER.test(line)) {
      inPackage = true;
      inTargetPackage = false;
      continue;
    }

    // Any new [[...]] header ends the current package block
    if (inPackage && /^\s*\[\[/.test(line)) {
      inPackage = false;
      inTargetPackage = false;
    }

    if (!inPackage) continue;

    if (!inTargetPackage && namePattern.test(line)) {
      inTargetPackage = true;
      continue;
    }

    if (inTargetPackage && CARGO_LOCK_VERSION_LINE.test(line)) {
      const updatedLine = updateVersionLine(line, version);
      if (!updatedLine) {
        throw new Error(
          `Cannot update Cargo.lock: version entry for package "${packageName}" has an unexpected layout`,
        );
      }
      lines[i] = updatedLine;
      updated = true;
      break;
    }
  }

  return {
    content: updated ? lines.join(newline) : cargoLock,
    updated,
  };
};
```

This preserves:
- `[[package]]` detection
- name matching
- version line rewriting (via your existing `updateVersionLine`)
- CRLF vs LF handling

…while eliminating `findCargoLockPackageBlocks` and the extra nested loops.

### 2. Simplify error signaling for the caller

Instead of a dedicated `CargoLockPackageNotFoundError`, let the helper return a status flag; the caller can decide whether “not updated” means “not found” or “unchanged”.

```js
if (existsSync(cargoLockPath)) {
  const cargoLock = await readFile(cargoLockPath, 'utf8');

  const { content: updatedCargoLock, updated } = updateCargoLockPackageVersion({
    cargoLock,
    packageName: DESKTOP_TAURI_CRATE_NAME,
    version,
  });

  if (!updated) {
    console.warn(
      `${cargoLockPath}: package "${DESKTOP_TAURI_CRATE_NAME}" not found or version not updated. Skipping Cargo.lock version sync.`,
    );
  } else if (updatedCargoLock !== cargoLock) {
    await writeFile(cargoLockPath, updatedCargoLock, 'utf8');
  }
}
```

This keeps all existing behavior (including the warning) but:
- removes a custom error type and extra `try/catch` branch
- keeps `syncDesktopVersionFiles` more linear and closer in complexity to the other version‑update paths.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

@SourceryAI SourceryAI left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In updateVersionLine, using right.includes("'") to choose the quote character can break valid Cargo.lock lines like version = "1.0.0 with user's patch" by switching to single quotes; consider preserving the original quote character (e.g., by detecting the first quote in the value) instead of inferring from content.
  • The new syncDesktopVersionFiles Cargo.lock tests repeat a lot of boilerplate setup for the temp project structure; factoring that into a small helper (e.g., createTempDesktopProject({ cargoLockContents })) would make future scenarios easier to add and the tests more readable.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `updateVersionLine`, using `right.includes("'")` to choose the quote character can break valid Cargo.lock lines like `version = "1.0.0 with user's patch"` by switching to single quotes; consider preserving the original quote character (e.g., by detecting the first quote in the value) instead of inferring from content.
- The new `syncDesktopVersionFiles` Cargo.lock tests repeat a lot of boilerplate setup for the temp project structure; factoring that into a small helper (e.g., `createTempDesktopProject({ cargoLockContents })`) would make future scenarios easier to add and the tests more readable.

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.

@zouyonghe
Copy link
Member Author

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe zouyonghe merged commit 4c22691 into AstrBotDevs:main Mar 18, 2026
4 checks passed
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