CLI: sign-in hint on 401 and a default login server (#1371)

* CLI: explain 401s with a login hint, default login to executor.sh A 401 from a stored profile now renders as a sign-in message naming the profile and the exact login command, instead of the transport-level `Decode error (401 GET .../api/tools)`. `executor login` with no flags and no profiles now targets https://executor.sh instead of erroring, and prints the chosen server before starting the device flow. * Keep profile plumbing out of CLI auth messages The sign-in hint now mirrors how the user addressed the server: plain `executor login` when no flags were typed, echoing --server/--base-url only when they were. Login and logout output mention profiles only when the user works with named profiles. * Add changeset for CLI login hints

Rhys Sullivan committed 4 months ago

Commit 262fc3e1 parent 56b60bf5 files · +359 −24Browse files

Unified · Split

5 files

.changeset/cli-login-hints.md +5 −0
apps/cli/src/main.ts +61 −24
apps/cli/src/server-connection.test.ts +84 −0
apps/cli/src/server-connection.ts +49 −0
e2e/local/cli-call-logged-out.test.ts +160 −0

.changeset/cli-login-hints.md+5−0

@@ -0,0 +1,5 @@
	1	---
	2	"executor": patch
	3	---
	4
	5	Explain 401s from a hosted server as a sign-in problem with the exact `executor login` command to run, instead of surfacing a raw decode error. `executor login` now defaults to https://executor.sh when no server is specified, and profile plumbing stays out of messages unless you address servers by name.

apps/cli/src/main.ts+61−24

 import { Argument as Args, Command, Flag as Options } from "effect/unstable/cli";
 import { BunRuntime, BunServices } from "@effect/platform-bun";
 import { HttpApiClient } from "effect/unstable/httpapi";
-import { FetchHttpClient, HttpClient, HttpClientRequest } from "effect/unstable/http";
+import {
+  FetchHttpClient,
+  HttpClient,
+  HttpClientError,
+  HttpClientRequest,
+} from "effect/unstable/http";
 import { FileSystem, Path as PlatformPath } from "effect";
 import type { PlatformError } from "effect/PlatformError";
 import * as Effect from "effect/Effect";
 import {
   canAutoStartCliServerConnection,
   chooseCliServerConnectionWithActiveLocal,
+  describeUnauthorizedCliServer,
   parseCliExecutorServerConnection,
+  profileNameFromConnectionKey,
   type CliServerConnectionSource,
   withCliServerAuthFallback,
 } from "./server-connection";
 // login. The refreshed tokens are written back to the originating profile.
 const OAUTH_REFRESH_SKEW_SECONDS = 60;
-const profileNameFromKey = (key: string): string | null =>
-  key.startsWith("profile:") ? key.slice("profile:".length) : null;
 const refreshOAuthConnection = (
   connection: ExecutorServerConnection,
 ): Effect.Effect<ExecutorServerConnection, never, FileSystem.FileSystem | PlatformPath.Path> =>
       },
     });
-    const profileName = profileNameFromKey(connection.key);
+    const profileName = profileNameFromConnectionKey(connection.key);
     if (profileName) {
       yield* upsertCliServerConnectionProfile({
         name: profileName,
 }): Effect.Effect<ExecuteCodeResult, Error, FileSystem.FileSystem | PlatformPath.Path> =>
   Effect.gen(function* () {
     const connection = yield* resolveExecutorServerConnection(input.target);
-    const client = yield* makeApiClient(connection);
+    const client = yield* makeApiClient(connection, input.target);
     const response = yield* client.executions.execute({
       payload: {
         code: input.code,
 // Typed API client
 // ---------------------------------------------------------------------------
-const makeApiClient = (connection: ExecutorServerConnection) => {
+const makeApiClient = (connection: ExecutorServerConnection, target: ServerTarget = {}) => {
   const authorization = getExecutorServerAuthorizationHeader(connection);
   return HttpApiClient.make(ExecutorApi, {
     baseUrl: connection.apiBaseUrl,
           ),
         }
       : {}),
+    // A 401 on an endpoint that doesn't model it is a sign-in problem: rewrite
+    // the transport-level error into the login hint. Without this the client
+    // fails decoding the unexpected status and prints the opaque
+    // `Decode error (401 GET .../api/tools)`. Declared 401s (typed API errors)
+    // decode before this catch and pass through untouched.
+    transformResponse: (effect) =>
+      Effect.catchIf(
+        effect,
+        (cause) => HttpClientError.isHttpClientError(cause) && cause.response?.status === 401,
+        () =>
+          Effect.fail(new Error(describeUnauthorizedCliServer({ connection, cliPrefix, target }))),
+      ),
   }).pipe(Effect.provide(FetchHttpClient.layer));
 };
   Effect.gen(function* () {
     if (args.scopeDir) process.env.EXECUTOR_SCOPE_DIR = resolve(args.scopeDir);
-    const connection = yield* resolveExecutorServerConnection({
+    const target: ServerTarget = { baseUrl: args.baseUrl, serverName: args.serverName };
-      baseUrl: args.baseUrl,
+    const connection = yield* resolveExecutorServerConnection(target);
-      serverName: args.serverName,
+    const client = yield* makeApiClient(connection, target);
-    });
-    const client = yield* makeApiClient(connection);
     const tools = yield* client.tools.list({ query: {} });
     const toolPaths = tools.map((tool) => tool.address);
       const contentObj = yield* parseOptionalJsonObject(Option.getOrUndefined(content));
-      const client = yield* makeApiClient(connection);
+      const client = yield* makeApiClient(connection, target);
       const result = yield* client.executions.resume({
         params: { executionId },
         payload: { action, content: contentObj },
   }
 };
+/** Where `executor login` lands with no flags and no stored profiles. */
+const DEFAULT_LOGIN_ORIGIN = "https://executor.sh";
 // Resolve which server a login/logout targets: an existing profile (--server
-// or the default) or a bare origin (--base-url). The profile name is decided
+// or the default), a bare origin (--base-url), or hosted Executor when
-// later, from the authenticated account.
+// nothing is configured yet. The profile name is decided later, from the
+// authenticated account.
 const resolveLoginOrigin = (input: {
   readonly baseUrl: Option.Option<string>;
   readonly server: Option.Option<string>;
     const store = yield* readCliServerConnectionStore();
     const profile = defaultCliServerConnectionProfile(store);
     if (profile) return { origin: profile.connection.origin, profile };
-    return yield* Effect.fail(
+    return { origin: DEFAULT_LOGIN_ORIGIN, profile: null };
-      new Error(
-        "No server to log in to. Pass --base-url <https://your-host> or --server <profile>.",
-      ),
-    );
   });
 const loginNameOption = Options.string("name").pipe(
     Effect.gen(function* () {
       const target = yield* resolveLoginOrigin({ baseUrl, server });
       const explicitName = Option.getOrUndefined(name);
+      // The target may have been picked implicitly (default profile, or the
+      // hosted fallback): say where the login is going before the device flow.
+      console.log(`Signing in to ${target.origin}`);
       const discovery = yield* Effect.tryPromise({
         try: () => discoverCliLogin(target.origin),
         catch: toError,
       });
       console.log("");
-      console.log(`Logged in to ${target.origin} (profile "${profileName}", now the default).`);
+      // Profile bookkeeping stays backstage unless the user works with named
+      // profiles (--name, or a store that already has several).
+      const mentionProfile =
+        explicitName !== undefined || (yield* readCliServerConnectionStore()).profiles.length > 1;
+      console.log(
+        mentionProfile
+          ? `Logged in to ${target.origin} (profile "${profileName}", now the default).`
+          : `Logged in to ${target.origin}.`,
+      );
       if (email) console.log(`Account: ${email}`);
       if (org) console.log(`Organization: ${org}`);
       else if (sub) console.log(`User: ${sub}`);
     }),
-).pipe(Command.withDescription("Sign in to a hosted Executor server in the browser (device flow)"));
+).pipe(
+  Command.withDescription(
+    "Sign in to a hosted Executor server in the browser (device flow). Defaults to https://executor.sh.",
+  ),
+);
 const logoutCommand = Command.make(
   "logout",
         console.log(`No stored login for ${target.origin}.`);
         return;
       }
+      // Profile bookkeeping stays backstage unless the user addressed one.
+      const mentionProfile = Option.isSome(server) || store.profiles.length > 1;
       if (!profile.connection.auth) {
-        console.log(`Profile "${profile.name}" has no stored credentials.`);
+        console.log(
+          mentionProfile
+            ? `Profile "${profile.name}" has no stored credentials.`
+            : `Not signed in to ${target.origin}.`,
+        );
         return;
       }
       yield* upsertCliServerConnectionProfile({
         makeDefault: store.defaultProfile === profile.name,
       });
       console.log(
-        `Logged out of ${profile.connection.origin} (cleared credentials for "${profile.name}").`,
+        mentionProfile
+          ? `Logged out of ${profile.connection.origin} (cleared credentials for "${profile.name}").`
+          : `Logged out of ${profile.connection.origin}.`,
       );
     }),
 ).pipe(Command.withDescription("Clear stored credentials for a server profile"));

apps/cli/src/server-connection.test.ts+84−0

@@ -1,8 +1,11 @@
1	import { describe, expect, it } from "@effect/vitest";	1	import { describe, expect, it } from "@effect/vitest";
2		2
		3	import { normalizeExecutorServerConnection } from "@executor-js/sdk/shared";
		4
3	import {	5	import {
4	canAutoStartCliServerConnection,	6	canAutoStartCliServerConnection,
5	chooseCliServerConnectionWithActiveLocal,	7	chooseCliServerConnectionWithActiveLocal,
		8	describeUnauthorizedCliServer,
6	parseCliExecutorServerConnection,	9	parseCliExecutorServerConnection,
7	withCliServerAuthFallback,	10	withCliServerAuthFallback,
8	} from "./server-connection";	11	} from "./server-connection";
@@ -135,3 +138,84 @@ describe("CLI server connection", () => { · 126 unmodified lines
135	).toBe("conflict");	138	).toBe("conflict");
136	});	139	});
137	});	140	});
		141
		142	describe("describeUnauthorizedCliServer", () => {
		143	it("hints plain `executor login` when the server was picked implicitly", () => {
		144	const connection = normalizeExecutorServerConnection({
		145	key: "profile:rhys-executor.sh",
		146	origin: "https://executor.sh",
		147	auth: { kind: "oauth", accessToken: "stale", expiresAt: 1 },
		148	});
		149
		150	const message = describeUnauthorizedCliServer({
		151	connection,
		152	cliPrefix: "executor",
		153	target: {},
		154	});
		155	expect(message).toContain("You're signed out of https://executor.sh");
		156	expect(message).toContain("Run `executor login` to sign in again.");
		157	// Profile names are plumbing: never surfaced when the user didn't type one.
		158	expect(message).not.toContain("--server");
		159	expect(message).not.toContain("profile");
		160	});
		161
		162	it("echoes --server back when the user targeted a named profile", () => {
		163	const connection = normalizeExecutorServerConnection({
		164	key: "profile:work",
		165	origin: "https://executor.example",
		166	auth: { kind: "oauth", accessToken: "stale", expiresAt: 1 },
		167	});
		168
		169	const message = describeUnauthorizedCliServer({
		170	connection,
		171	cliPrefix: "executor",
		172	target: { serverName: "work" },
		173	});
		174	expect(message).toContain("You're signed out of https://executor.example");
		175	expect(message).toContain("executor login --server work");
		176	});
		177
		178	it("echoes --base-url back when the user targeted an origin", () => {
		179	const connection = normalizeExecutorServerConnection({
		180	origin: "https://executor.example",
		181	});
		182
		183	const message = describeUnauthorizedCliServer({
		184	connection,
		185	cliPrefix: "executor",
		186	target: { baseUrl: "https://executor.example" },
		187	});
		188	expect(message).toContain("no credentials are stored");
		189	expect(message).toContain("executor login --base-url https://executor.example");
		190	});
		191
		192	it("mentions the env vars when a profile-less bearer key is rejected", () => {
		193	const connection = normalizeExecutorServerConnection({
		194	origin: "https://executor.example",
		195	auth: { kind: "bearer", token: "key_bad" },
		196	});
		197
		198	const message = describeUnauthorizedCliServer({
		199	connection,
		200	cliPrefix: "executor",
		201	target: {},
		202	});
		203	expect(message).toContain("rejected the stored bearer credentials");
		204	expect(message).toContain("EXECUTOR_API_KEY");
		205	});
		206
		207	it("uses the dev entrypoint prefix verbatim", () => {
		208	const connection = normalizeExecutorServerConnection({
		209	key: "profile:hosted",
		210	origin: "https://executor.example",
		211	auth: { kind: "oauth", accessToken: "stale" },
		212	});
		213
		214	const message = describeUnauthorizedCliServer({
		215	connection,
		216	cliPrefix: "bun run apps/cli/src/main.ts",
		217	target: {},
		218	});
		219	expect(message).toContain("bun run apps/cli/src/main.ts login");
		220	});
		221	});

apps/cli/src/server-connection.ts+49−0

@@ -52,6 +52,55 @@ export const canAutoStartCliServerConnection = (connection: ExecutorServerConnec · 51 unmodified lines
52	return url.protocol === "http:" && canAutoStartLocalDaemonForHost(url.hostname);	52	return url.protocol === "http:" && canAutoStartLocalDaemonForHost(url.hostname);
53	};	53	};
54		54
		55	export const profileNameFromConnectionKey = (key: string): string \| null =>
		56	key.startsWith("profile:") ? key.slice("profile:".length) : null;
		57
		58	// A 401 from the server means the stored credential is stale, revoked, or
		59	// missing: a sign-in problem, not a transport problem. Rendered instead of
		60	// the raw HTTP client error so the user learns the recovery command, not the
		61	// failing endpoint.
		62	//
		63	// The hint mirrors how the user addressed the server. The common case types
		64	// no flags (the default profile, normally executor.sh), and login re-targets
		65	// that same default, so the fix is plain `executor login`: profile names are
		66	// plumbing and stay invisible. Only an explicit --server / --base-url comes
		67	// back in the hint.
		68	export const describeUnauthorizedCliServer = (input: {
		69	readonly connection: ExecutorServerConnection;
		70	readonly cliPrefix: string;
		71	readonly target: { readonly baseUrl?: string; readonly serverName?: string };
		72	}): string => {
		73	const { connection, cliPrefix, target } = input;
		74	const loginCommand = target.serverName
		75	? `${cliPrefix} login --server ${target.serverName}`
		76	: target.baseUrl
		77	? `${cliPrefix} login --base-url ${connection.origin}`
		78	: `${cliPrefix} login`;
		79
		80	if (!connection.auth) {
		81	return [
		82	`${connection.origin} requires authentication, and no credentials are stored for it.`,
		83	`Run \`${loginCommand}\` to sign in.`,
		84	].join("\n");
		85	}
		86
		87	if (connection.auth.kind === "oauth") {
		88	return [
		89	`You're signed out of ${connection.origin}.`,
		90	`Run \`${loginCommand}\` to sign in again.`,
		91	].join("\n");
		92	}
		93
		94	const envHint =
		95	connection.auth.kind === "bearer" && !profileNameFromConnectionKey(connection.key)
		96	? " If the key came from EXECUTOR_API_KEY or EXECUTOR_AUTH_TOKEN, check that value."
		97	: "";
		98	return [
		99	`${connection.origin} rejected the stored ${connection.auth.kind} credentials (401).${envHint}`,
		100	`Run \`${loginCommand}\` to sign in again.`,
		101	].join("\n");
		102	};
		103
55	export type CliServerConnectionSource =	104	export type CliServerConnectionSource =
56	\| "explicit"	105	\| "explicit"
57	\| "default-profile"	106	\| "default-profile"

e2e/local/cli-call-logged-out.test.ts+160−0

@@ -0,0 +1,160 @@
	1	// Local CLI: what `executor call` tells a user whose stored hosted-server
	2	// login no longer works. A stub standing in for the hosted server answers
	3	// every request 401 (the wire shape of an expired or revoked session), and
	4	// the default profile carries an expired OAuth token with no way to refresh.
	5	//
	6	// The CLI must treat that 401 as a sign-in problem, not a transport problem:
	7	// say which server the user is signed out of and that `executor login` fixes
	8	// it. The user typed no flags, so the hint is plain `executor login`; profile
	9	// names are internal plumbing and must not leak. And the raw
	10	// `Decode error (401 GET .../api/tools)` from before the 401 mapping must
	11	// never resurface.
	12	//
	13	// The session runs in an interactive shell with an `executor` shim on PATH so
	14	// the terminal.cast reads like the real moment: the user types the command,
	15	// the error is the whole answer.
	16	import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
	17	import { createServer } from "node:http";
	18	import type { AddressInfo } from "node:net";
	19	import { tmpdir } from "node:os";
	20	import { delimiter, join } from "node:path";
	21	import { fileURLToPath } from "node:url";
	22
	23	import { expect } from "@effect/vitest";
	24	import { Effect } from "effect";
	25
	26	import { scenario } from "../src/scenario";
	27	import { Cli, RunDir } from "../src/services";
	28
	29	const repoRoot = fileURLToPath(new URL("../../", import.meta.url));
	30
	31	/** Every route answers like a hosted server rejecting an expired session. */
	32	const listenAsUnauthorizedServer = () =>
	33	Effect.promise(
	34	() =>
	35	new Promise<{ origin: string; close: () => void }>((resolve) => {
	36	const server = createServer((_request, response) => {
	37	response.writeHead(401, { "content-type": "application/json" });
	38	response.end(JSON.stringify({ error: "unauthorized" }));
	39	});
	40	server.listen(0, "127.0.0.1", () => {
	41	const { port } = server.address() as AddressInfo;
	42	resolve({
	43	origin: `http://127.0.0.1:${port}`,
	44	close: () => server.close(),
	45	});
	46	});
	47	}),
	48	);
	49
	50	/** A data dir whose default profile is a hosted server with an expired,
	51	* unrefreshable OAuth token: the state `executor server list` shows as
	52	* `stored-auth` after a login has gone stale. The `profile:` key matches
	53	* what `executor login` persists via upsertCliServerConnectionProfile. */
	54	const writeLoggedOutProfile = (dataDir: string, origin: string) => {
	55	writeFileSync(
	56	join(dataDir, "server-connections.json"),
	57	JSON.stringify(
	58	{
	59	version: 1,
	60	defaultProfile: "hosted",
	61	profiles: [
	62	{
	63	name: "hosted",
	64	connection: {
	65	kind: "http",
	66	key: "profile:hosted",
	67	origin,
	68	displayName: "hosted",
	69	auth: { kind: "oauth", accessToken: "expired-access-token", expiresAt: 1 },
	70	},
	71	},
	72	],
	73	},
	74	null,
	75	2,
	76	),
	77	);
	78	};
	79
	80	/** An `executor` on PATH that runs this checkout's CLI, so the recording
	81	* shows the command a user actually types. */
	82	const writeExecutorShim = (binDir: string) => {
	83	const shim = join(binDir, "executor");
	84	writeFileSync(shim, `#!/bin/sh\nexec bun run "${join(repoRoot, "apps/cli/src/main.ts")}" "$@"\n`);
	85	chmodSync(shim, 0o755);
	86	};
	87
	88	scenario(
	89	"CLI call · a signed-out hosted profile explains how to log back in",
	90	{ timeout: 120_000 },
	91	Effect.gen(function* () {
	92	const cli = yield* Cli;
	93	const runDir = yield* RunDir;
	94
	95	const hosted = yield* listenAsUnauthorizedServer();
	96	const dataDir = mkdtempSync(join(tmpdir(), "executor-cli-logged-out-"));
	97	writeLoggedOutProfile(dataDir, hosted.origin);
	98	writeExecutorShim(dataDir);
	99
	100	yield* cli
	101	.session(
	102	["bash", "--noprofile", "--norc"],
	103	async (term) => {
	104	await term.screen.waitUntil((current) => current.text.includes("$"), {
	105	timeoutMs: 30_000,
	106	});
	107
	108	// The user browses the catalog the way the docs teach: call --help.
	109	await term.keyboard.type("executor call github issues --help", { paceMs: 40 });
	110	await term.keyboard.press("Enter");
	111
	112	// The command exits after printing its sign-in error.
	113	const snapshot = await term.screen.waitUntil(
	114	(current) => current.text.includes("sign in"),
	115	{ timeoutMs: 60_000 },
	116	);
	117	const screen = snapshot.text;
	118
	119	// The moment reads as a sign-in problem naming the server...
	120	expect(screen, "the error says the user is signed out").toContain("You're signed out");
	121	// ...fixed by plain `executor login` (dev mode prefixes the entry
	122	// script, so assert the stable tail of the sentence).
	123	expect(screen, "it hands over the bare login command").toContain(
	124	"login` to sign in again.",
	125	);
	126
	127	// No flags were typed, so no plumbing comes back.
	128	expect(screen, "profile names stay internal").not.toContain("--server");
	129	expect(screen, "profiles are not mentioned at all").not.toContain("profile");
	130
	131	// The pre-fix rendering must not resurface.
	132	expect(screen, "the raw transport error stays internal").not.toContain("Decode error");
	133	expect(screen, "the API endpoint does not leak into the message").not.toContain(
	134	"/api/tools",
	135	);
	136	},
	137	{
	138	cwd: repoRoot,
	139	env: {
	140	EXECUTOR_DATA_DIR: dataDir,
	141	EXECUTOR_SCOPE_DIR: dataDir,
	142	PATH: `${dataDir}${delimiter}${process.env.PATH ?? ""}`,
	143	PS1: "$ ",
	144	// Keep macOS's "default shell is now zsh" banner out of the cast.
	145	BASH_SILENCE_DEPRECATION_WARNING: "1",
	146	},
	147	record: join(runDir, "terminal.cast"),
	148	viewport: { cols: 100, rows: 24 },
	149	},
	150	)
	151	.pipe(
	152	Effect.ensuring(
	153	Effect.sync(() => {
	154	hosted.close();
	155	rmSync(dataDir, { recursive: true, force: true });
	156	}),
	157	),
	158	);
	159	}),
	160	);