Conversation
Summary of ChangesHello @hsjoberg, 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 streamlines the client application's version management by introducing a new Highlights
Changelog
Activity
Using Gemini Code AssistThe 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
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 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a convenient just bump command for version management, implemented via a new TypeScript script. While a great addition, the justfile recipe is vulnerable to command injection due to unsafe string interpolation when passing arguments to the shell, which needs to be addressed by using the shell's built-in positional parameter handling. Furthermore, there's a critical issue in how the script modifies package.json that will cause it to fail, and some suggestions have been provided to improve the script's structure and maintainability.
| const packageVersionRegex = /^ "version"\s*:\s*"(\d+\.\d+\.\d+)"/m; | ||
| const appVersionRegex = /config\.expo\.version\s*=\s*"(\d+\.\d+\.\d+)";/; | ||
| const versionCodeRegex = /config\.expo\.android!\.versionCode\s*=\s*(\d+);/; | ||
| const buildNumberRegex = /config\.expo\.ios!\.buildNumber\s*=\s*"(\d+)";/; | ||
|
|
||
| const packageVersionMatch = packageJson.match(packageVersionRegex); | ||
| if (!packageVersionMatch) { | ||
| throw new Error("Could not find a top-level version in client/package.json."); | ||
| } |
There was a problem hiding this comment.
Using regular expressions to modify package.json is fragile and will not work as written.
- The
packageVersionRegexdoes not account for the trailing comma on theversionline inclient/package.json. This will causepackageJson.match(packageVersionRegex)to returnnull, and the script will fail. - Even if the regex were fixed, the replacement logic on line 116 would remove the comma, resulting in an invalid
package.jsonfile.
A more robust approach is to parse the JSON file, modify the version property on the resulting object, and then stringify it back to the file. This would involve using JSON.parse() to read the file, updating the version property, and JSON.stringify() to write it back.
There was a problem hiding this comment.
The packageVersionRegex does not account for the trailing comma on the version line in client/package.json. This will cause packageJson.match(packageVersionRegex) to return null, and the script will fail.
The regex ^ "version"\s*:\s*"(\d+.\d+.\d+)" isn’t anchored to end‑of‑line, so it matches the version substring even with a trailing comma.
|
|
||
| # Bump client version numbers | ||
| bump *args: | ||
| bun scripts/bump_client_version.ts {{ args }} |
There was a problem hiding this comment.
The bump recipe uses string interpolation {{ args }} to pass user-supplied arguments to a shell command. Because just recipes are executed in a shell, an attacker can provide arguments containing shell metacharacters (like ;, &, |, or backticks) to execute arbitrary commands on the system. Using "$@" is the recommended way to safely pass positional arguments to commands in just recipes, ensuring they are correctly quoted and not interpreted by the shell.
bun scripts/bump_client_version.ts "$@"
There was a problem hiding this comment.
The risk is technically real, but the proposed fix is wrong for just.
- {{ args }} is unquoted, so metacharacters are interpreted by the shell. You can see it with just --dry-run bump "foo;bar" which prints bun scripts/
bump_client_version.ts foo;bar. So the injection claim is correct in a strict sense. $@doesn’t apply to just recipe args. just interpolates {{ args }} itself; it doesn’t pass recipe args as shell positional parameters, so $@ would be
empty. That recommendation is incorrect in this context.- Practically, this isn’t a meaningful security issue for a local just command: anyone who can run just already has shell access.
| if (args.length > 0) { | ||
| const first = args[0].toLowerCase(); | ||
| if (["patch", "minor", "major"].includes(first)) { | ||
| if (args.length > 1) { | ||
| throw new Error(`Too many arguments. ${usage}`); | ||
| } | ||
| mode = first as "patch" | "minor" | "major"; | ||
| } else if (isSemver(first)) { | ||
| if (args.length > 1) { | ||
| throw new Error(`Too many arguments. ${usage}`); | ||
| } | ||
| explicitVersion = first; | ||
| } else { | ||
| throw new Error(`Unknown argument "${args[0]}". ${usage}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The argument validation logic can be simplified. The check for too many arguments (args.length > 1) is currently duplicated within each conditional branch. By moving this check to before the branches, you can avoid repetition and make the code easier to read and maintain.
if (args.length > 1) {
throw new Error(`Too many arguments. ${usage}`);
}
if (args.length > 0) {
const first = args[0].toLowerCase();
if (["patch", "minor", "major"].includes(first)) {
mode = first as "patch" | "minor" | "major";
} else if (isSemver(first)) {
explicitVersion = first;
} else {
throw new Error(`Unknown argument "${args[0]}". ${usage}`);
}
}
Simply do
just bumpto bump patch version.You can also do
jump major|minor|patch|<x.x.x>