ronantakizawa/codereview-bench huggingface dataset:
Repository: gchq/CyberChef
File: src/core/lib/Stream.mjs (JavaScript)

Code before:

            // Loop through the elements comparing them to val.
            for (let x = length-1; x > -1; x--) {
                if (this.bytes[this.position-length + x] !== val[x]) {
                    found = false;

                    // If element is not equal to val's element then jump forward by the correct amount.
                    this.position += skiptable[val[x]];
                    break;
                }
            }
            if (found) {
                this.position -= length;
                break;
            }
        }
    }


    /**
     * Consume bytes if it matches the supplied value.
     *
     * @param {Number} val
     */
    consumeWhile(val) {
        while ((++this.position < this.length) && (this.bytes[(this.position)] === val));
    }

    /**
     * Consume the next byte if it matches the supplied value.
     *
     * @param {number} val
     */
    consumeIf(val) {
        if (this.bytes[this.position] === val) {
            this.position++;
            this.bitPos = 0;
        }
    }

    /**
     * Move forwards through the stream by the specified number of bytes.
     *
     * @param {number} numBytes
     */
    moveForwardsBy(numBytes) {
        const pos = this.position + numBytes;
        if (pos < 0 || pos > this.length)
            throw new Error("Cannot move to position " + pos + " in stream. Out of bounds.");
        this.position = pos;
        this.bitPos = 0;

Review comment (suggestion):
This will still move the position forwards by one even if the current byte does not match `val`. In fact it will now not even check the current byte - it moves straight to the next one before the check is made. We need to check the current byte, then increment `this.position` if and only if it matches `val`.

Code after:

            // Loop through the elements comparing them to val.
            for (let x = length-1; x >= 0; x--) {
                if (this.bytes[this.position-length + x] !== val[x]) {
                    found = false;

                    // If element is not equal to val's element then jump forward by the correct amount.
                    this.position += skiptable[val[x]];
                    break;
                }
            }
            if (found) {
                this.position -= length;
                break;
            }
        }
    }


    /**
     * Consume bytes if it matches the supplied value.
     *
     * @param {Number} val
     */
    consumeWhile(val) {
        while (this.position < this.length) {
            if (this.bytes[this.position] !== val) {
                break;
            }
            this.position++;
        }
    }

    /**
     * Consume the next byte if it matches the supplied value.
     *
     * @param {number} val
     */
    consumeIf(val) {
        if (this.bytes[this.position] === val) {
            this.position++;
            this.bitPos = 0;
        }
    }

    /**
     * Move forwards through the stream by the specified number of bytes.
     *
     * @param {number} numBytes
     */
    moveForwardsBy(numBytes) {
Repository: gchq/CyberChef
File: src/core/operations/RailFenceCipherDecode.mjs (JavaScript)

Code before:
        this.args = [
            {
                name: "Key",
                type: "number",
                value: 2
            },
            {
                name: "Offset",
                type: "number",
                value: 0
            }
        ];
    }

    /**
     * @param {string} input
     * @param {Object[]} args
     * @returns {string}
     */
    run(input, args) {
        const [key, offset] = args;

        let cipher = input;

        if (key < 2) {
            return "Key has to be bigger than 2";
        } else if (key > cipher.length) {
            return "Key should be smaller than the cipher's length";
        }

        if (offset < 0) {
            return "Offset has to be a positive integer";
        }

        const cycle = (key - 1) * 2;

        const rest = cipher.length % key;

        if (rest !== 0) {
            cipher = cipher + (" ".repeat(key - rest));
        }

        const plaintext = new Array(cipher.length);

        let j = 0;
        let x, y;

        for (y = 0; y < key; y++) {
            for (x = 0; x < cipher.length; x++) {
                if ((y + x + offset) % cycle === 0 || (y - x - offset) % cycle === 0) {
                    plaintext[x] = cipher[j++];

Review comment (suggestion):
Rather than returning errors as strings, I would recommend using the OperationError object. So this would become 'throw new OperationError("Key has to be bigger than 2");'

To import it you need to insert:
'import OperationError from "../errors/OperationError.mjs";'

Code after:
        this.outputType = "string";
        this.args = [
            {
                name: "Key",
                type: "number",
                value: 2
            },
            {
                name: "Offset",
                type: "number",
                value: 0
            }
        ];
    }

    /**
     * @param {string} input
     * @param {Object[]} args
     * @returns {string}
     */
    run(input, args) {
        const [key, offset] = args;

        let cipher = input;

        if (key < 2) {
            throw new OperationError("Key has to be bigger than 2");
        } else if (key > cipher.length) {
            throw new OperationError("Key should be smaller than the cipher's length");
        }

        if (offset < 0) {
            throw new OperationError("Offset has to be a positive integer");
        }

        const cycle = (key - 1) * 2;

        const rest = cipher.length % key;

        if (rest !== 0) {
            cipher = cipher + (" ".repeat(key - rest));
        }

        const plaintext = new Array(cipher.length);

        let j = 0;
        let x, y;

        for (y = 0; y < key; y++) {
            for (x = 0; x < cipher.length; x++) {
                if ((y + x + offset) % cycle === 0 || (y - x - offset) % cycle === 0) {
Repository: gchq/CyberChef
File: src/core/lib/Modhex.mjs (JavaScript)

Code before:
        output.push(HEX_TO_MODHEX_CONVERSION_MAP[letter]);
    }

    return output.join("");
}


/**
 * Convert a hex string into a byte array.
 *
 * @param {string} data
 * @param {string} [delim]
 * @param {number} [byteLen=2]
 * @returns {byteArray}
 *
 * @example
 * // returns [10,20,30]
 * fromModhex("cl bf bu");
 *
 * // returns [10,20,30]
 * fromModhex("cl:bf:bu", "Colon");
 */
export function fromModhex(data, delim="Auto", byteLen=2) {
    if (delim !== "None") {
        const delimRegex = delim === "Auto" ? /[^cbdefghijklnrtuv]/gi : Utils.regexRep(delim);
        data = data.replace(delimRegex, "");
    }

    let hexconv = "";
    for (const letter of data.split("")) {
        hexconv += MODHEX_TO_HEX_CONVERSION_MAP[letter];
    }

    const output = fromHex(hexconv, "None", byteLen);
    return output;
}

/**
 * Modhex to Hex conversion map.
 */
export const MODHEX_TO_HEX_CONVERSION_MAP = {
    "c": "0",
    "b": "1",
    "d": "2",
    "e": "3",
    "f": "4",
    "g": "5",
    "h": "6",
    "i": "7",
    "j": "8",
    "k": "9",

Review comment (suggestion):
Multiline inputs will break if the `delim` is anything other than `Auto` (new line will be converted into `undefined`).

Code after:
    } else {
        return output;
    }
}


/**
 * Convert a byte array into a modhex string as efficiently as possible with no options.
 *
 * @param {byteArray|Uint8Array|ArrayBuffer} data
 * @returns {string}
 *
 * @example
 * // returns "clbfbu"
 * toModhexFast([10,20,30]);
 */
export function toModhexFast(data) {
    if (!data) return "";
    if (data instanceof ArrayBuffer) data = new Uint8Array(data);

    const output = [];

    for (let i = 0; i < data.length; i++) {
        output.push(MODHEX_ALPHABET_MAP[(data[i] >> 4) & 0xf]);
        output.push(MODHEX_ALPHABET_MAP[data[i] & 0xf]);
    }
    return output.join("");
}


/**
 * Convert a modhex string into a byte array.
 *
 * @param {string} data
 * @param {string} [delim]
 * @param {number} [byteLen=2]
 * @returns {byteArray}
 *
 * @example
 * // returns [10,20,30]
 * fromModhex("cl bf bu");
 *
 * // returns [10,20,30]
 * fromModhex("cl:bf:bu", "Colon");
 */
export function fromModhex(data, delim="Auto", byteLen=2) {
    if (byteLen < 1 || Math.round(byteLen) !== byteLen)
        throw new OperationError("Byte length must be a positive integer");

    // The `.replace(/\s/g, "")` an interesting workaround: Hex "multiline" tests aren't actually
    // multiline. Tests for Modhex fixes that, thus exposing the issue.
Repository: gchq/CyberChef
File: src/core/operations/Jq.mjs (JavaScript)

Code before:
/**
 * @author zhzy0077 [zhzy0077@hotmail.com]
 * @copyright Crown Copyright 2023
 * @license Apache-2.0
 */

import Operation from "../Operation.mjs";
import OperationError from "../errors/OperationError.mjs";
import jq from "jq-web";

/**
 * jq operation
 */
class Jq extends Operation {

    /**
     * Jq constructor
     */
    constructor() {
        super();

        this.name = "jq";
        this.module = "Code";
        this.description = "jq is a lightweight and flexible command-line JSON processor.";
        this.infoURL = "https://github.com/jqlang/jq";
        this.inputType = "JSON";
        this.outputType = "string";
        this.args = [
            {
                name: "Query",
                type: "string",
                value: ""
            }
        ];

Review comment (suggestion):
This import seems to increase the size of the bundle by around 3-4MB, including the size of "Code" modules from 700KB transferred to 4MB transferred.  I suspect there's not a lighter-weight way of doing this, but would it be possible to put it in it's own module?  The "Code" module is used by a bunch of other operators.

Code after:
/**
 * @author zhzy0077 [zhzy0077@hotmail.com]
 * @copyright Crown Copyright 2023
 * @license Apache-2.0
 */

import Operation from "../Operation.mjs";
import OperationError from "../errors/OperationError.mjs";
import jq from "jq-web";

/**
 * jq operation
 */
class Jq extends Operation {

    /**
     * Jq constructor
     */
    constructor() {
        super();

        this.name = "Jq";
        this.module = "Jq";
        this.description = "jq is a lightweight and flexible command-line JSON processor.";
        this.infoURL = "https://github.com/jqlang/jq";
        this.inputType = "JSON";
        this.outputType = "string";
        this.args = [
            {
                name: "Query",
                type: "string",
                value: ""
            }
        ];
Repository: gchq/CyberChef
File: src/core/lib/CipherSaber2.mjs (JavaScript)

Code before:
/**
 * @author n1073645 [n1073645@gmail.com]
 * @copyright Crown Copyright 2020
 * @license Apache-2.0
 */
export function encode(tempIVP, key, rounds, input) {
    const ivp = new Uint8Array(key.concat(tempIVP));
    const state = new Array(256).fill(0);
    let j = 0, i = 0;
    const result = [];

    // Mixing states based off of IV.
    for (let i = 0; i < 256; i++)
        state[i] = i;
    const ivpLength = ivp.length;
    for (let r = 0; r < rounds; r ++) {
        for (let k = 0; k < 256; k++) {
            j = (j + state[k] + ivp[k % ivpLength]) % 256;
            [state[k], state[j]] = [state[j], state[k]];
        }
    }
    j = 0;
    i = 0;

    // XOR cipher with key.
    for (let x = 0; x < input.length; x++) {
        i = (++i) % 256;
        j = (j + state[i]) % 256;
        [state[i], state[j]] = [state[j], state[i]];
        const n = (state[i] + state[j]) % 256;
        result.push(state[n] ^ input[x]);
    }

Review comment (suggestion):
key is a `number[]` and tempIVP is a `Buffer`, so I was initially surprised that they can't be concatenated, but a quick test shows it's true:

```javascript
> const tempIVP = Buffer.from([1, 2])
undefined
> const key = [3, 4]
undefined
> key.concat(tempIVP)
[ 3, 4, <Buffer 01 02> ]
```

An alternative seems to be spreading _just_ the buffer (e.g. `key.concat(...tempIVP)`), but explicitly spreading both seems like a nicer solution anyway.  Nice catch!

Code after:
/**
 * @author n1073645 [n1073645@gmail.com]
 * @copyright Crown Copyright 2020
 * @license Apache-2.0
 */
export function encode(tempIVP, key, rounds, input) {
    const ivp = new Uint8Array([...key, ...tempIVP]);
    const state = new Array(256).fill(0);
    let j = 0, i = 0;
    const result = [];

    // Mixing states based off of IV.
    for (let i = 0; i < 256; i++)
        state[i] = i;
    const ivpLength = ivp.length;
    for (let r = 0; r < rounds; r ++) {
        for (let k = 0; k < 256; k++) {
            j = (j + state[k] + ivp[k % ivpLength]) % 256;
            [state[k], state[j]] = [state[j], state[k]];
        }
    }
    j = 0;
    i = 0;

    // XOR cipher with key.
    for (let x = 0; x < input.length; x++) {
        i = (++i) % 256;
        j = (j + state[i]) % 256;
        [state[i], state[j]] = [state[j], state[i]];
        const n = (state[i] + state[j]) % 256;
        result.push(state[n] ^ input[x]);
    }
Repository: gitbutlerapp/gitbutler
File: apps/desktop/src/lib/forge/gitlab/gitlabPrService.svelte.ts (TypeScript)

Code before:
								await sleep(1000);

								const mr = await api.MergeRequests.show(upstreamProjectId, number, {
									includeRebaseInProgress: true
								});
								// Check if rebase is complete
								if (!mr.rebase_in_progress) {
									rebaseComplete = true;
								}
								attempt++;
							}

							if (!rebaseComplete) {
								throw new Error('Rebase operation timed out. Please try merging again later.');
							}

							// After rebase completes, perform the merge
							await api.MergeRequests.merge(upstreamProjectId, number, {
								shouldRemoveSourceBranch: true
							});
						} else {
							// For 'merge' and 'squash' methods, use the merge API directly
							await api.MergeRequests.merge(upstreamProjectId, number, {
								squash: method === 'squash',
								shouldRemoveSourceBranch: true
							});
						}

						return { data: undefined };
					} catch (e: unknown) {
						return { error: toSerializable(e) };
					}
				},
				invalidatesTags: [invalidatesList(ReduxTag.GitLabPullRequests)]
			}),
			updatePr: build.mutation<
				void,
				{
					number: number;
					update: {
						targetBase?: string;
						description?: string;
						state?: 'open' | 'closed';
					};
				}
			>({
				queryFn: async ({ number, update }, query) => {
					try {
						const { api, upstreamProjectId } = gitlab(query.extra);
						await api.MergeRequests.edit(upstreamProjectId, number, {
							targetBranch: update.targetBase,

Review comment (suggestion):
Based on krlvi's reply in this PR (https://github.com/gitbutlerapp/gitbutler/pull/8222#issuecomment-2828490332), MRs created by GitButler explicitly specify deletion of the source branch, but this isn't necessarily true for MRs created elsewhere.

To ensure the stack branches feature works correctly, I think it's necessary to explicitly pass the `shouldRemoveSourceBranch` parameter when performing the merge.


Code after:
			updatePr: build.mutation<
				void,
				{
					number: number;
					update: {
						targetBase?: string;
						description?: string;
						state?: 'open' | 'closed';
					};
				}
			>({
				queryFn: async ({ number, update }, query) => {
					try {
						const { api, upstreamProjectId } = gitlab(query.extra);
						await api.MergeRequests.edit(upstreamProjectId, number, {
							targetBranch: update.targetBase,
							description: update.description
						});
						return { data: undefined };
					} catch (e: unknown) {
						return { error: toSerializable(e) };
					}
				},
				invalidatesTags: [invalidatesList(ReduxTag.GitLabPullRequests)]
			})
		})
	});
}
Repository: gitbutlerapp/gitbutler
File: apps/desktop/cypress/e2e/support/mock/settings.ts (TypeScript)

Code before:
	singleBranch: false
};

export const MOCK_APP_SETTINGS: AppSettings = {
	onboardingComplete: true,
	telemetry: MOCK_TELEMETRY_SETINGS,
	featureFlags: MOCK_FEATURE_FLAGS,
	fetch: {
		autoFetchIntervalMinutes: 15
	},
	claude: {
		executable: 'claude',
		notifyOnCompletion: false,
		notifyOnPermissionRequest: false,
		dangerouslyAllowAllPermissions: false,
		autoCommitAfterCompletion: true,
		useConfiguredModel: false
	},
	reviews: {
		autoFillPrDescriptionFromCommit: true
	},
	ui: {
		useNativeTitleBar: false,
		cliIsManagedByPackageManager: false,
		checkForUpdatesIntervalInSeconds: 3600
	}
};


Review comment (bug):
I see now that putting most settings into the `ui` section is wrong now that we have a CLI that would basically implement everything the UI can do, and will keep that in mind.

Code after:
	singleBranch: false
};

export const MOCK_APP_SETTINGS: AppSettings = {
	onboardingComplete: true,
	telemetry: MOCK_TELEMETRY_SETINGS,
	featureFlags: MOCK_FEATURE_FLAGS,
	fetch: {
		autoFetchIntervalMinutes: 15
	},
	claude: {
		executable: 'claude',
		notifyOnCompletion: false,
		notifyOnPermissionRequest: false,
		dangerouslyAllowAllPermissions: false,
		autoCommitAfterCompletion: true,
		useConfiguredModel: false
	},
	reviews: {
		autoFillPrDescriptionFromCommit: true
	},
	ui: {
		useNativeTitleBar: false,
		cliIsManagedByPackageManager: false
	},
	checkForUpdatesIntervalInSeconds: 3600
};
Repository: gitbutlerapp/gitbutler
File: crates/but/tests/but/command/rub.rs (Rust)

Code before:
    env.file("a.txt", "arbitrary text\n");
    env.but("zz:a.txt A").assert().success();
    Ok(env)
}

#[test]
fn assign_uncommitted_file() -> anyhow::Result<()> {
    let env = assigned_uncommitted_file_env()?;
    env.but("diff A:a.txt")
        .assert()
        .success()
        .stderr_eq(snapbox::str![])
        .stdout_eq(snapbox::str![[r#"
────────╮
j0 a.txt│
────────╯
     1│+arbitrary text

"#]]);
    Ok(())
}

#[test]
fn uncommitted_file_to_unassigned() -> anyhow::Result<()> {
    let env = assigned_uncommitted_file_env()?;
    env.but("A:a.txt zz")
        .assert()
        .success()
        .stdout_eq(snapbox::str![[r#"
Unassigned the only hunk in a.txt in a stack

"#]])
        .stderr_eq(str![""]);

    env.but("diff zz:a.txt")
        .assert()
        .success()
        .stderr_eq(snapbox::str![])
        .stdout_eq(snapbox::str![[r#"
────────╮
j0 a.txt│
────────╯
     1│+arbitrary text

"#]]);

    Ok(())
}

#[test]
fn uncommitted_file_to_branch() -> anyhow::Result<()> {

Review comment (suggestion):
While `A:a.txt` is great to read in comparison to a CLI-Id, one still has to know that `A` is a branch that exists as part of the fixture.
To my mind, the most maintainable tests will show what's there, do something, assert specifically and if they are cautious, finish like they started with the result state so nothing weird and unrelated sneaks in.

I am happy to backoff from that position though as this is clearly an improvement already, and the tests seem easy enough (right now) to follow.

The underlying problem that led me to thinking about this is still valid though, and I suppose for now we have to live with it (and those who change the output of common commands will have to auto-update a lot of expectations, and do so blindly).

Code after:
    env.file("a.txt", "arbitrary text\n");
    env.but("zz:a.txt A").assert().success();
    Ok(env)
}

#[test]
fn assign_uncommitted_file() -> anyhow::Result<()> {
    let env = assigned_uncommitted_file_env()?;
    env.but("diff A@{stack}:a.txt")
        .assert()
        .success()
        .stderr_eq(snapbox::str![])
        .stdout_eq(snapbox::str![[r#"
────────╮
j0 a.txt│
────────╯
     1│+arbitrary text

"#]]);
    Ok(())
}

#[test]
fn uncommitted_file_to_unassigned() -> anyhow::Result<()> {
    let env = assigned_uncommitted_file_env()?;
    env.but("A@{stack}:a.txt zz")
        .assert()
        .success()
        .stdout_eq(snapbox::str![[r#"
Unassigned the only hunk in a.txt in a stack

"#]])
        .stderr_eq(str![""]);

    env.but("diff zz:a.txt")
        .assert()
        .success()
        .stderr_eq(snapbox::str![])
        .stdout_eq(snapbox::str![[r#"
────────╮
j0 a.txt│
────────╯
     1│+arbitrary text

"#]]);

    Ok(())
}

#[test]
fn uncommitted_file_to_branch() -> anyhow::Result<()> {
Repository: gitbutlerapp/gitbutler
File: ignore-vercel.sh (Shell)

Code before:

# Check each monitored paths for changes
HAS_CHANGES=false

for path in "${MONITORED_PATHS[@]}"; do
  if [ -d "$path" ] || [ -f "$path" ]; then
    if ! git diff --quiet "$PREVIOUS_COMMIT" HEAD -- "$path"; then
      echo "✅ Changes detected in: $path"
      HAS_CHANGES=true
    else
      echo "➖ No changes in: $path"
    fi
  else
    echo "⚠️  No such file or directory: $path"
  fi
done

echo ""

if [ "$HAS_CHANGES" = true ]; then
  echo "🚀 Changes detected in monitored paths. Proceeding with build."
  exit 1  # Build
else
  echo "🛑 No relevant changes detected. Skipping build."
  exit 0  # Skip build
fi



Review comment (suggestion):
Could not find a good reason to separate checking files from directories, it's the exact same command to check regardless.

Also removed `yarn.lock` and `package-lock.json` as they aren't applicable to our project.

Code after:

# Check each monitored path for changes
HAS_CHANGES=false

for path in "${MONITORED_PATHS[@]}"; do
  if [ -d "$path" ] || [ -f "$path" ]; then
    if ! git diff --quiet "$PREVIOUS_COMMIT" HEAD -- "$path"; then
      echo "✅ Changes detected in: $path"
      HAS_CHANGES=true
    else
      echo "➖ No changes in: $path"
    fi
  else
    echo "⚠️  No such file or directory: $path"
  fi
done

echo ""

if [ "$HAS_CHANGES" = true ]; then
  echo "🚀 Changes detected in monitored paths. Proceeding with build."
  exit 1  # Build
else
  echo "🛑 No relevant changes detected. Skipping build."
  exit 0  # Skip build
fi