From 991460eec94e8a4e9459250d13a1746566a58472 Mon Sep 17 00:00:00 2001 From: Peter Schilling Date: Thu, 23 Jul 2026 08:17:38 -0700 Subject: [PATCH] Follow-up to #253: error on invalid configured sort instead of defaulting Add resolveIssueSort() so all issue-listing commands share one sort resolution path: --sort flag > LINEAR_ISSUE_SORT env > issue_sort config > priority default. Unlike the getOption fallback, an explicitly configured but invalid sort value now errors with guidance instead of silently sorting by priority; this also fixes the same latent silent downgrade in issue query. Also cover the gaps around the new default: a genuinely unconfigured subprocess test (the repo's own .linear.toml supplies issue_sort when tests run in-process, so the previous no-config tests were passing via that config file), a test that a configured sort order still wins over the default, and updated skill docs that no longer claim issue list requires a sort order. --- skills/linear-cli/SKILL.md | 2 +- skills/linear-cli/SKILL.template.md | 2 +- src/commands/issue/issue-mine.ts | 11 +- src/commands/issue/issue-query.ts | 6 +- src/config.ts | 30 +++- src/utils/linear.ts | 6 +- test/commands/issue/issue-mine.test.ts | 204 ++++++++++++++++++++++++- test/config.test.ts | 105 ++++++++++++- 8 files changed, 342 insertions(+), 24 deletions(-) diff --git a/skills/linear-cli/SKILL.md b/skills/linear-cli/SKILL.md index ff2a7045..89e43349 100644 --- a/skills/linear-cli/SKILL.md +++ b/skills/linear-cli/SKILL.md @@ -209,7 +209,7 @@ Each command has detailed help output describing all available flags and options Some commands have required flags that aren't obvious. Notable examples: -- `issue list` requires a sort order — provide it via `--sort` (valid values: `manual`, `priority`), the `issue_sort` config option, or the `LINEAR_ISSUE_SORT` env var. Also requires `--team ` unless the team can be inferred from the directory — if unknown, run `linear team list` first. +- `issue list` sorts by priority by default — override via `--sort` (valid values: `manual`, `priority`), the `issue_sort` config option, or the `LINEAR_ISSUE_SORT` env var. Requires `--team ` unless the team can be inferred from the directory — if unknown, run `linear team list` first. - `--no-pager` is only supported on `issue list` — passing it to other commands like `project list` will error. ## Using the Linear GraphQL API Directly diff --git a/skills/linear-cli/SKILL.template.md b/skills/linear-cli/SKILL.template.md index f28dea78..6d7ee9ef 100644 --- a/skills/linear-cli/SKILL.template.md +++ b/skills/linear-cli/SKILL.template.md @@ -92,7 +92,7 @@ Each command has detailed help output describing all available flags and options Some commands have required flags that aren't obvious. Notable examples: -- `issue list` requires a sort order — provide it via `--sort` (valid values: `manual`, `priority`), the `issue_sort` config option, or the `LINEAR_ISSUE_SORT` env var. Also requires `--team ` unless the team can be inferred from the directory — if unknown, run `linear team list` first. +- `issue list` sorts by priority by default — override via `--sort` (valid values: `manual`, `priority`), the `issue_sort` config option, or the `LINEAR_ISSUE_SORT` env var. Requires `--team ` unless the team can be inferred from the directory — if unknown, run `linear team list` first. - `--no-pager` is only supported on `issue list` — passing it to other commands like `project list` will error. ## Using the Linear GraphQL API Directly diff --git a/src/commands/issue/issue-mine.ts b/src/commands/issue/issue-mine.ts index 27f4d459..b8543887 100644 --- a/src/commands/issue/issue-mine.ts +++ b/src/commands/issue/issue-mine.ts @@ -1,7 +1,7 @@ import { Command, EnumType } from "@cliffy/command" import { unicodeWidth } from "@std/cli" import { rgb24 } from "@std/fmt/colors" -import { getOption } from "../../config.ts" +import { resolveIssueSort } from "../../config.ts" import { colorCycleShort, formatCycleShort, @@ -178,14 +178,7 @@ export const mineCommand = new Command() throw new ValidationError("Cannot use --all-states with --state flag") } - const sort = sortFlag || - (getOption("issue_sort") as "manual" | "priority" | undefined) || - "priority" - if (!SortType.values().includes(sort)) { - throw new ValidationError( - `Sort must be one of: ${SortType.values().join(", ")}`, - ) - } + const sort = resolveIssueSort(sortFlag) const teamKey = team || getTeamKey() if (!teamKey) { throw new ValidationError( diff --git a/src/commands/issue/issue-query.ts b/src/commands/issue/issue-query.ts index eb5c6774..69da30d9 100644 --- a/src/commands/issue/issue-query.ts +++ b/src/commands/issue/issue-query.ts @@ -1,7 +1,7 @@ import { Command, EnumType } from "@cliffy/command" import { unicodeWidth } from "@std/cli" import { rgb24 } from "@std/fmt/colors" -import { getOption } from "../../config.ts" +import { resolveIssueSort } from "../../config.ts" import { colorCycleShort, type CycleDisplayInfo, @@ -317,9 +317,7 @@ export const queryCommand = new Command() spinner?.start() // Resolve sort for non-search mode - const sort = search ? undefined : (sortFlag || - (getOption("issue_sort") as "manual" | "priority" | undefined) || - "priority") + const sort = search ? undefined : resolveIssueSort(sortFlag) if (search) { // --- Search mode: use searchIssues() backend --- diff --git a/src/config.ts b/src/config.ts index 3e7da784..d3cb09f1 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2,6 +2,7 @@ import { parse } from "@std/toml" import { join } from "@std/path" import { load } from "@std/dotenv" import * as v from "valibot" +import { ValidationError } from "./utils/errors.ts" let config: Record = {} @@ -131,12 +132,16 @@ function coerceBool(value: unknown): boolean | undefined { // Custom valibot schema for boolean coercion const BooleanLike = v.pipe(v.unknown(), v.transform(coerceBool)) +export const ISSUE_SORT_VALUES = ["manual", "priority"] as const +export type IssueSort = (typeof ISSUE_SORT_VALUES)[number] +export const DEFAULT_ISSUE_SORT: IssueSort = "priority" + // Options schema const OptionsSchema = v.object({ team_id: v.optional(v.string()), api_key: v.optional(v.string()), workspace: v.optional(v.string()), - issue_sort: v.optional(v.picklist(["manual", "priority"])), + issue_sort: v.optional(v.picklist(ISSUE_SORT_VALUES)), issue_create_ask_project: v.optional(BooleanLike), issue_create_assign_self: v.optional(v.picklist(["always", "auto", "never"])), vcs: v.optional(v.picklist(["git", "jj"])), @@ -167,6 +172,29 @@ export function getOption( return undefined as Options[T] } +/** + * Resolve the issue sort order from CLI flag, LINEAR_ISSUE_SORT env var, or + * issue_sort config, defaulting to priority when nothing is set. Unlike + * getOption, an explicitly configured but invalid value errors instead of + * silently falling back to the default. + */ +export function resolveIssueSort(cliValue?: string): IssueSort { + const raw = getRawOption("issue_sort", cliValue) + if (raw == null) return DEFAULT_ISSUE_SORT + const parsed = v.safeParse(v.picklist(ISSUE_SORT_VALUES), raw) + if (!parsed.success) { + throw new ValidationError( + `Invalid issue sort: ${JSON.stringify(raw)}`, + { + suggestion: `Use one of: ${ + ISSUE_SORT_VALUES.join(", ") + } (via --sort, the issue_sort config option, or the LINEAR_ISSUE_SORT environment variable)`, + }, + ) + } + return parsed.output +} + // CLI workspace set via --workspace flag let cliWorkspace: string | undefined diff --git a/src/utils/linear.ts b/src/utils/linear.ts index 686d67c9..564c55cc 100644 --- a/src/utils/linear.ts +++ b/src/utils/linear.ts @@ -15,7 +15,7 @@ import type { SearchIssuesQuery, } from "../__codegen__/graphql.ts" import { Select } from "@cliffy/prompt" -import { getOption } from "../config.ts" +import { getOption, resolveIssueSort } from "../config.ts" import { CliError, NotFoundError, ValidationError } from "./errors.ts" import { getGraphQLClient } from "./graphql.ts" import { normalizeIssueIdentifier } from "./issue-identifier.ts" @@ -593,9 +593,7 @@ export async function fetchIssuesForState( createdAfter?: string, updatedAfter?: string, ) { - const sort: "manual" | "priority" = sortParam ?? - (getOption("issue_sort") as "manual" | "priority" | undefined) ?? - "priority" + const sort = resolveIssueSort(sortParam) const filter: IssueFilter = { team: { key: { eq: teamKey } }, diff --git a/test/commands/issue/issue-mine.test.ts b/test/commands/issue/issue-mine.test.ts index 48fd4067..2bac7edd 100644 --- a/test/commands/issue/issue-mine.test.ts +++ b/test/commands/issue/issue-mine.test.ts @@ -1,6 +1,7 @@ import { snapshotTest as cliffySnapshotTest } from "@cliffy/testing" import { assertEquals } from "@std/assert" import { getColorEnabled, setColorEnabled } from "@std/fmt/colors" +import { fromFileUrl, join } from "@std/path" import { stub } from "@std/testing/mock" import { mineCommand } from "../../../src/commands/issue/issue-mine.ts" import { @@ -120,7 +121,7 @@ Deno.test("Issue Mine Command - Filter By Label", async () => { } }) -Deno.test("Issue Mine Command - Defaults To Priority Sort When None Configured", async () => { +Deno.test("Issue Mine Command - Defaults To Priority Sort Without Flag Or Env Var", async () => { const fixedNow = new Date("2026-03-30T10:00:00.000Z") const RealDate = Date const originalColorEnabled = getColorEnabled() @@ -138,7 +139,9 @@ Deno.test("Issue Mine Command - Defaults To Priority Sort When None Configured", // No LINEAR_ISSUE_SORT and no --sort flag: the request must still go out, // sorted by priority. The mock only matches when sort is the priority - // payload, so a missing/incorrect default would fail to match. + // payload, so a missing/incorrect default would fail to match. Note the + // repo's own .linear.toml also sets issue_sort = "priority", so the truly + // unconfigured default is covered by the subprocess test below. const { cleanup } = await setupMockLinearServer([ { queryName: "GetIssuesForState", @@ -202,6 +205,203 @@ Deno.test("Issue Mine Command - Defaults To Priority Sort When None Configured", } }) +Deno.test("Issue Mine Command - Configured Sort Order Wins Over Default", async () => { + // LINEAR_ISSUE_SORT=manual with no --sort flag: the priority default must + // not override the configured sort. The mock only matches the manual sort + // payload, so if the default won the request would not match. + const { cleanup } = await setupMockLinearServer([ + { + queryName: "GetIssuesForState", + variables: { + sort: [ + { workflowState: { order: "Descending" } }, + { manual: { nulls: "last", order: "Ascending" } }, + ], + }, + response: { + data: { + issues: { + nodes: [ + { + id: "issue-1", + identifier: "ENG-101", + title: "Fix login bug", + priority: 1, + estimate: 3, + assignee: { initials: "MC" }, + state: { + id: "state-1", + name: "In Progress", + color: "#f2c94c", + type: "started", + }, + labels: { nodes: [] }, + cycle: null, + team: { + id: "team-eng-id", + key: "ENG", + cyclesEnabled: false, + activeCycle: null, + }, + inverseRelations: { nodes: [] }, + updatedAt: "2026-03-13T10:00:00.000Z", + }, + ], + pageInfo: { hasNextPage: false, endCursor: null }, + }, + }, + }, + }, + ], { LINEAR_TEAM_ID: "ENG", LINEAR_ISSUE_SORT: "manual", NO_COLOR: "true" }) + + const logs: string[] = [] + const logStub = stub(console, "log", (...args: unknown[]) => { + logs.push(args.map(String).join(" ")) + }) + + try { + await mineCommand.parse(["--team", "ENG"]) + + assertEquals(logs.join("\n").includes("ENG-101"), true) + } finally { + logStub.restore() + await cleanup() + } +}) + +Deno.test("Issue Mine Command - Invalid Configured Sort Errors", async () => { + // An explicitly configured but invalid sort must error with guidance, not + // silently fall back to the priority default. No request should be sent, + // so the mock server has no expectations. + const { cleanup } = await setupMockLinearServer([], { + LINEAR_TEAM_ID: "ENG", + LINEAR_ISSUE_SORT: "banana", + NO_COLOR: "true", + }) + + const errorLogs: string[] = [] + const errorStub = stub(console, "error", (...args: unknown[]) => { + errorLogs.push(args.map(String).join(" ")) + }) + const exitStub = stub(Deno, "exit", (_code?: number): never => { + throw new Error("EXIT") + }) + + try { + await mineCommand.parse(["--team", "ENG"]) + } catch { + // expected: handleError calls the stubbed Deno.exit + } finally { + errorStub.restore() + exitStub.restore() + await cleanup() + } + + const output = errorLogs.join("\n") + assertEquals(output.includes('Invalid issue sort: "banana"'), true) + assertEquals(output.includes("manual, priority"), true) +}) + +Deno.test("Issue Mine Command - Defaults To Priority Sort When Nothing Is Configured", async () => { + // The true out-of-the-box case: run the CLI from a temp cwd with a clean + // HOME and env, so no config file or env var supplies issue_sort. Versions + // that required sort configuration exit with "Sort must be provided" here. + const { server, cleanup } = await setupMockLinearServer([ + { + queryName: "GetIssuesForState", + variables: { + sort: [ + { workflowState: { order: "Descending" } }, + { priority: { nulls: "last", order: "Descending" } }, + { manual: { nulls: "last", order: "Ascending" } }, + ], + }, + response: { + data: { + issues: { + nodes: [ + { + id: "issue-1", + identifier: "ENG-101", + title: "Fix login bug", + priority: 1, + estimate: 3, + assignee: { initials: "MC" }, + state: { + id: "state-1", + name: "In Progress", + color: "#f2c94c", + type: "started", + }, + labels: { nodes: [] }, + cycle: null, + team: { + id: "team-eng-id", + key: "ENG", + cyclesEnabled: false, + activeCycle: null, + }, + inverseRelations: { nodes: [] }, + updatedAt: "2026-03-13T10:00:00.000Z", + }, + ], + pageInfo: { hasNextPage: false, endCursor: null }, + }, + }, + }, + }, + ]) + const tempDir = await Deno.makeTempDir() + + try { + const mainPath = fromFileUrl( + new URL("../../../src/main.ts", import.meta.url), + ) + const denoJsonPath = fromFileUrl( + new URL("../../../deno.json", import.meta.url), + ) + const homeDir = Deno.env.get("HOME") + const denoDir = Deno.env.get("DENO_DIR") ?? + (homeDir == null ? undefined : join(homeDir, ".cache", "deno")) + const command = new Deno.Command(Deno.execPath(), { + args: [ + "run", + "--allow-all", + "--quiet", + `--config=${denoJsonPath}`, + mainPath, + "issue", + "mine", + "--team", + "ENG", + ], + cwd: tempDir, + clearEnv: true, + env: { + PATH: Deno.env.get("PATH") ?? "", + HOME: tempDir, + XDG_CONFIG_HOME: join(tempDir, ".config"), + ...(denoDir == null ? {} : { DENO_DIR: denoDir }), + LINEAR_GRAPHQL_ENDPOINT: server.getEndpoint(), + LINEAR_API_KEY: "Bearer test-token", + NO_COLOR: "true", + }, + stdout: "piped", + stderr: "piped", + }) + + const { code, stdout, stderr } = await command.output() + const output = new TextDecoder().decode(stdout) + const errorOutput = new TextDecoder().decode(stderr) + + assertEquals(code, 0, `expected success, stderr: ${errorOutput}`) + assertEquals(output.includes("ENG-101"), true) + } finally { + await Deno.remove(tempDir, { recursive: true }) + await cleanup() + } +}) + Deno.test("Issue Mine Command - Shows Blocked Indicator", async () => { const fixedNow = new Date("2026-03-30T10:00:00.000Z") const RealDate = Date diff --git a/test/config.test.ts b/test/config.test.ts index 6831ca3a..1e6fc33b 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -1,6 +1,7 @@ -import { assertEquals } from "@std/assert" +import { assertEquals, assertThrows } from "@std/assert" import { fromFileUrl } from "@std/path" -import { getOption } from "../src/config.ts" +import { getOption, resolveIssueSort } from "../src/config.ts" +import { ValidationError } from "../src/utils/errors.ts" // Note: These tests use the cliValue parameter (highest precedence) // to avoid interference from config files that may exist in the repo @@ -622,3 +623,103 @@ Deno.test("getOption - env var takes precedence over home config", async () => { await Deno.remove(tempHome, { recursive: true }) } }) + +// --- resolveIssueSort --- +// Note: the repo's own .linear.toml sets issue_sort = "priority", so it is in +// the loaded config for in-process tests. Env vars are read at call time, so +// setting LINEAR_ISSUE_SORT here exercises precedence over that config value. +// The truly-unconfigured default is tested in a subprocess below. + +Deno.test("resolveIssueSort - cli value takes precedence", () => { + assertEquals(resolveIssueSort("manual"), "manual") + assertEquals(resolveIssueSort("priority"), "priority") +}) + +Deno.test("resolveIssueSort - env var takes precedence over config", () => { + Deno.env.set("LINEAR_ISSUE_SORT", "manual") + try { + assertEquals(resolveIssueSort(), "manual") + } finally { + Deno.env.delete("LINEAR_ISSUE_SORT") + } +}) + +Deno.test("resolveIssueSort - invalid cli value throws", () => { + assertThrows( + () => resolveIssueSort("banana"), + ValidationError, + 'Invalid issue sort: "banana"', + ) +}) + +Deno.test("resolveIssueSort - invalid env value throws instead of defaulting", () => { + Deno.env.set("LINEAR_ISSUE_SORT", "banana") + try { + assertThrows( + () => resolveIssueSort(), + ValidationError, + 'Invalid issue sort: "banana"', + ) + } finally { + Deno.env.delete("LINEAR_ISSUE_SORT") + } +}) + +Deno.test("resolveIssueSort - empty env value throws instead of defaulting", () => { + Deno.env.set("LINEAR_ISSUE_SORT", "") + try { + assertThrows( + () => resolveIssueSort(), + ValidationError, + 'Invalid issue sort: ""', + ) + } finally { + Deno.env.delete("LINEAR_ISSUE_SORT") + } +}) + +Deno.test("resolveIssueSort - defaults to priority when nothing is configured", async () => { + // Subprocess with a cleared env and a temp cwd and HOME so no config file + // or env var (including one set in the test runner's environment) can + // supply issue_sort. + const tempDir = await Deno.makeTempDir() + try { + const configUrl = new URL("../src/config.ts", import.meta.url) + const denoJsonPath = fromFileUrl(new URL("../deno.json", import.meta.url)) + const homeDir = Deno.env.get("HOME") + const denoDir = Deno.env.get("DENO_DIR") ?? + (homeDir == null ? undefined : `${homeDir}/.cache/deno`) + const command = new Deno.Command(Deno.execPath(), { + args: [ + "eval", + `--config=${denoJsonPath}`, + `import { resolveIssueSort } from "${configUrl}"; console.log(resolveIssueSort());`, + ], + cwd: tempDir, + clearEnv: true, + env: { + HOME: tempDir, + XDG_CONFIG_HOME: `${tempDir}/.config`, + PATH: Deno.env.get("PATH") ?? "", + ...(denoDir == null ? {} : { DENO_DIR: denoDir }), + ...(Deno.build.os === "windows" + ? { SystemRoot: Deno.env.get("SystemRoot") ?? "" } + : {}), + }, + stdout: "piped", + stderr: "piped", + }) + + const { stdout, stderr } = await command.output() + const output = new TextDecoder().decode(stdout).trim() + const errorOutput = new TextDecoder().decode(stderr) + + if (errorOutput) { + console.error("Subprocess stderr:", errorOutput) + } + + assertEquals(output, "priority") + } finally { + await Deno.remove(tempDir, { recursive: true }) + } +})