From d442b8cc1b09e8f35c5b755337e2069e85f5a769 Mon Sep 17 00:00:00 2001
From: dan-claude-bot <dan-claude-bot@users.noreply.github.com>
Date: Tue, 21 Jul 2026 13:05:36 +0000
Subject: [PATCH] =?UTF-8?q?fix:=20--port=20is=20argv=20too=20=E2=80=94=20r?=
 =?UTF-8?q?eject=20it=20before=20the=20preflight,=20not=20at=20listen()?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

@claude-bot-andresmgsl's outstanding item from the prior round, which my last
reply passed over in silence. That silence read as an oversight because it was
one.

`--port` on the create path was still bare `Number()`, so `--port abc` became
NaN, reached `server.listen(NaN)` in github-app.ts, and died as an uncaught
ERR_SOCKET_BAD_PORT stack trace — after `detectOwnerType` and the org-admin
preflight had already gone out. It is the same missing argv validation this
round fixed for the two ids, in a command whose stated rule is reject before
any write or network call.

Nothing is destroyed when it fails: no App and no client secret exist at that
point. So this is not about damage, it is about the command honouring its own
rule, and about failing with a sentence rather than a stack trace.

Range-checked as well as digits-only: `--port 99999` passes every test the ids
need and still cannot be listened on.

Scope, stated rather than assumed: `server add --port` (src/cli.ts:2430) has
the identical shape but predates this branch and is not in its diff. It is a
real instance of the same bug and belongs in its own change, not smuggled into
this one.

Four CLI cases — non-numeric, out-of-range, zero, decimal — asserting exit 2,
no stub hits and an unchanged state dir, driven through `create` because that
is the path that reads the flag. Verified by mutation: disabling the check
fails all four.
---
 src/cli.ts                           | 20 ++++++++++++
 test/github-app-register-cli.test.ts | 46 ++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/src/cli.ts b/src/cli.ts
index e7f23b0..446f331 100644
--- a/src/cli.ts
+++ b/src/cli.ts
@@ -1535,6 +1535,26 @@ async function githubAppCommand(rest: string[]): Promise<number> {
       }
     }
   }
+  // `--port` on the create path has the same defect the ids had, and the same
+  // rule applies: `Number("abc")` is NaN, which reaches `server.listen(NaN)` in
+  // github-app.ts and dies as an uncaught ERR_SOCKET_BAD_PORT stack trace —
+  // after `detectOwnerType` and the org-admin preflight have already gone out.
+  // Nothing is lost when it fails (no App and no secret exist yet), so this is
+  // about the command honouring its own stated rule rather than about damage:
+  // reject before any write or network call, and fail with a sentence instead
+  // of a stack trace.
+  //
+  // Range-checked as well as digits-only, because `--port 99999` is accepted by
+  // every check the ids need and still cannot be listened on.
+  if (values.port !== undefined) {
+    const p = Number(values.port);
+    if (!/^\d+$/.test(values.port) || p < 1 || p > 65535) {
+      console.error(
+        `--port must be a port number between 1 and 65535 (got ${JSON.stringify(values.port)})`,
+      );
+      return 2;
+    }
+  }
   const stateDir = stateDirFrom(values.state);
   const bindingsPath = join(stateDir, "environments.yaml");
   const bindings = loadBindings(bindingsPath);
diff --git a/test/github-app-register-cli.test.ts b/test/github-app-register-cli.test.ts
index ccd9f3f..d93fa9b 100644
--- a/test/github-app-register-cli.test.ts
+++ b/test/github-app-register-cli.test.ts
@@ -341,6 +341,52 @@ describe("cast github-app register", () => {
     expect(readdirSync(f.state).sort()).toEqual(before);
   });
 
+  // `--port` belongs to the CREATE path, and had the same defect the ids did:
+  // `Number("abc")` is NaN, which reaches server.listen(NaN) and dies as an
+  // uncaught ERR_SOCKET_BAD_PORT stack trace — after detectOwnerType and the
+  // org-admin preflight have already gone out. Nothing is lost when it fails
+  // (no App and no secret exist yet), so this is about the command honouring
+  // its own rule — reject before any write or network call — and failing with
+  // a sentence rather than a stack trace.
+  //
+  // Driven through `create` because that is the path that reads the flag. The
+  // validation sits in the shared preamble, above openCoolify, so the run ends
+  // before the browser flow this command would otherwise need.
+  for (const [what, port] of [
+    ["a non-numeric --port", "abc"],
+    ["an out-of-range --port", "99999"],
+    ["a zero --port", "0"],
+    ["a decimal --port", "80.5"],
+  ] as const) {
+    it(`refuses ${what} before touching disk or Coolify`, async () => {
+      const stub = await stubCoolify({ repositories: [] });
+      const f = fixture(
+        stub.url,
+        "github_apps:\n  heavy-duty/incubator: hdb-coolify-prod",
+      );
+      const before = readdirSync(f.state).sort();
+
+      const r = await run(
+        [
+          "github-app",
+          "create",
+          "heavy-duty/incubator",
+          "--env",
+          "prod",
+          "--state",
+          f.state,
+          "--port",
+          port,
+        ],
+        null,
+      );
+      expect(r.code).toBe(2);
+      expect(r.output).toContain("--port must be a port number");
+      expect(stub.hits).toEqual([]);
+      expect(readdirSync(f.state).sort()).toEqual(before);
+    });
+  }
+
   it("prints usage for an unknown subcommand", async () => {
     const r = await run(["github-app", "wat"], null);
     expect(r.code).toBe(2);