chore: address coder-agents-comments

registry/coder/modules/git-clone/README.md

@@ -189,9 +189,14 @@ module "git-clone" {
 
 Pass any additional flags through `extra_args` (one element per argument).
 This lets you enable anything `git clone` supports without the module having
-to expose it explicitly — for example a shallow clone, submodules, parallel
+to expose it explicitly, for example a shallow clone, submodules, parallel
 fetches, or partial clones.
 
+> Do not put secrets in `extra_args`. The resolved `git clone` command
+> (including every element of `extra_args`) is echoed to the workspace
+> startup log, so values like `--config=http.extraHeader=Authorization: Bearer <token>`
+> would appear there in plaintext.
+
 ```tf
 module "git-clone" {
   count    = data.coder_workspace.me.start_count

registry/coder/modules/git-clone/main.test.ts

@@ -29,6 +29,20 @@ const executeScriptInContainer = async (
   };
 };
 
+// Drops a fake `git` onto PATH that prints each argv entry on its own line.
+// Lets tests prove that arguments (including ones with embedded spaces) reach
+// `git clone` as single argv tokens, which the echo line cannot show because
+// it joins with spaces.
+const installFakeGit = [
+  "cat > /usr/local/bin/git <<'SHIM'",
+  "#!/bin/sh",
+  'for arg in "$@"; do',
+  '  printf "argv:%s\\n" "$arg"',
+  "done",
+  "SHIM",
+  "chmod +x /usr/local/bin/git",
+].join("\n");
+
 describe("git-clone", async () => {
   await runTerraformInit(import.meta.dir);
 
@@ -56,7 +70,7 @@ describe("git-clone", async () => {
     expect(output.stdout).toEqual([
       "Creating directory /root/fake-url...",
       "Cloning fake-url to /root/fake-url...",
-      "Running: git clone  fake-url /root/fake-url",
+      "Running: git clone fake-url /root/fake-url",
     ]);
     expect(output.stderr.join(" ")).toContain("fatal");
     expect(output.stderr.join(" ")).toContain("fake-url");
@@ -233,7 +247,7 @@ describe("git-clone", async () => {
     expect(output.stdout).toEqual([
       "Creating directory /root/repo-tests.log...",
       "Cloning https://github.com/michaelbrewer/repo-tests.log to /root/repo-tests.log on branch feat/branch...",
-      "Running: git clone  -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log",
+      "Running: git clone -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log",
     ]);
   });
 
@@ -247,7 +261,7 @@ describe("git-clone", async () => {
     expect(output.stdout).toEqual([
       "Creating directory /root/repo-tests.log...",
       "Cloning https://gitlab.com/mike.brew/repo-tests.log to /root/repo-tests.log on branch feat/branch...",
-      "Running: git clone  -b feat/branch https://gitlab.com/mike.brew/repo-tests.log /root/repo-tests.log",
+      "Running: git clone -b feat/branch https://gitlab.com/mike.brew/repo-tests.log /root/repo-tests.log",
     ]);
   });
 
@@ -269,7 +283,7 @@ describe("git-clone", async () => {
     expect(output.stdout).toEqual([
       "Creating directory /root/repo-tests.log...",
       "Cloning https://github.com/michaelbrewer/repo-tests.log to /root/repo-tests.log on branch feat/branch...",
-      "Running: git clone  -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log",
+      "Running: git clone -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log",
     ]);
   });
 
@@ -322,18 +336,67 @@ describe("git-clone", async () => {
       url: "fake-url",
     });
     const script = findResourceInstance(state, "coder_script").script;
-    expect(script).toContain('EXTRA_ARGS_B64=""');
+    expect(script).toContain('EXTRA_ARGS=""');
   });
 
   it("passes extra_args to git clone", async () => {
     const state = await runTerraformApply(import.meta.dir, {
       agent_id: "foo",
       url: "fake-url",
-      extra_args: '["--recurse-submodules", "--jobs=8"]',
+      extra_args: JSON.stringify([
+        "--recurse-submodules",
+        "--jobs=8",
+        "--config=user.name=Coder User",
+        "-c",
+        "core.sshCommand=ssh -i /tmp/key",
+      ]),
     });
-    const output = await executeScriptInContainer(state, "alpine/git");
+    const output = await executeScriptInContainer(
-    expect(output.stdout).toContain(
+      state,
-      "Running: git clone --recurse-submodules --jobs=8 fake-url /root/fake-url",
+      "alpine/git",
+      installFakeGit,
+    );
+    expect(output.exitCode).toBe(0);
+    expect(output.stdout.join("\n")).toContain(
+      [
+        "argv:clone",
+        "argv:--recurse-submodules",
+        "argv:--jobs=8",
+        "argv:--config=user.name=Coder User",
+        "argv:-c",
+        "argv:core.sshCommand=ssh -i /tmp/key",
+        "argv:fake-url",
+        "argv:/root/fake-url",
+      ].join("\n"),
+    );
+  });
+
+  it("passes extra_args alongside branch_name in the correct order", async () => {
+    const state = await runTerraformApply(import.meta.dir, {
+      agent_id: "foo",
+      url: "fake-url",
+      branch_name: "feat/branch",
+      extra_args: JSON.stringify([
+        "--recurse-submodules",
+        "--config=user.name=Coder User",
+      ]),
+    });
+    const output = await executeScriptInContainer(
+      state,
+      "alpine/git",
+      installFakeGit,
+    );
+    expect(output.exitCode).toBe(0);
+    expect(output.stdout.join("\n")).toContain(
+      [
+        "argv:clone",
+        "argv:--recurse-submodules",
+        "argv:--config=user.name=Coder User",
+        "argv:-b",
+        "argv:feat/branch",
+        "argv:fake-url",
+        "argv:/root/fake-url",
+      ].join("\n"),
     );
   });
 

registry/coder/modules/git-clone/main.tf

@@ -97,8 +97,7 @@ locals {
   encoded_post_clone_script = var.post_clone_script != null ? base64encode(var.post_clone_script) : ""
   # Encode the pre_clone_script for passing to the shell script
   encoded_pre_clone_script = var.pre_clone_script != null ? base64encode(var.pre_clone_script) : ""
-  # Encode extra clone args (newline-separated) so the shell script can split them into an array safely
+  encoded_extra_args       = base64encode(join("\n", var.extra_args))
-  encoded_extra_args = base64encode(join("\n", var.extra_args))
 }
 
 output "repo_dir" {

registry/coder/modules/git-clone/run.sh

@@ -7,7 +7,7 @@ CLONE_PATH="${CLONE_PATH}"
 BRANCH_NAME="${BRANCH_NAME}"
 # Expand home if it's specified!
 CLONE_PATH="$${CLONE_PATH/#\~/$${HOME}}"
-EXTRA_ARGS_B64="${EXTRA_ARGS}"
+EXTRA_ARGS="${EXTRA_ARGS}"
 POST_CLONE_SCRIPT="${POST_CLONE_SCRIPT}"
 PRE_CLONE_SCRIPT="${PRE_CLONE_SCRIPT}"
 
@@ -47,11 +47,11 @@ if [ -n "$PRE_CLONE_SCRIPT" ]; then
 fi
 
 # Build optional git clone flags
-CLONE_FLAGS=()
+extra_args=()
-if [ -n "$EXTRA_ARGS_B64" ]; then
+if [ -n "$EXTRA_ARGS" ]; then
   while IFS= read -r arg || [ -n "$arg" ]; do
-    [ -n "$arg" ] && CLONE_FLAGS+=("$arg")
+    [ -n "$arg" ] && extra_args+=("$arg")
-  done < <(echo "$EXTRA_ARGS_B64" | base64 -d)
+  done < <(echo "$EXTRA_ARGS" | base64 -d)
 fi
 
 # Check if the directory is empty
@@ -59,12 +59,12 @@ fi
 if [ -z "$(ls -A "$CLONE_PATH")" ]; then
   if [ -z "$BRANCH_NAME" ]; then
     echo "Cloning $REPO_URL to $CLONE_PATH..."
-    echo "Running: git clone $${CLONE_FLAGS[*]} $REPO_URL $CLONE_PATH"
+    echo "Running: git clone $${extra_args[@]:+$${extra_args[@]} }$REPO_URL $CLONE_PATH"
-    git clone "$${CLONE_FLAGS[@]}" "$REPO_URL" "$CLONE_PATH"
+    git clone $${extra_args[@]+"$${extra_args[@]}"} "$REPO_URL" "$CLONE_PATH"
   else
     echo "Cloning $REPO_URL to $CLONE_PATH on branch $BRANCH_NAME..."
-    echo "Running: git clone $${CLONE_FLAGS[*]} -b $BRANCH_NAME $REPO_URL $CLONE_PATH"
+    echo "Running: git clone $${extra_args[@]:+$${extra_args[@]} }-b $BRANCH_NAME $REPO_URL $CLONE_PATH"
-    git clone "$${CLONE_FLAGS[@]}" -b "$BRANCH_NAME" "$REPO_URL" "$CLONE_PATH"
+    git clone $${extra_args[@]+"$${extra_args[@]}"} -b "$BRANCH_NAME" "$REPO_URL" "$CLONE_PATH"
   fi
 else
   echo "$CLONE_PATH already exists and isn't empty, skipping clone!"