From b9ac41e55db314cf72e7a395e931b6425273ceba Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Fri, 28 Aug 2026 21:42:19 -0700 Subject: [PATCH 1/3] fix(install): name Codex in the install picker The tool-selection multiselect read Claude Code / Cursor / OpenCode / Agent Skills. Codex has always been detected and installed into `.agents/`, but the row that serves it is labelled for people who already know `AGENTS.md` is the file Codex reads, so a Codex user concluded we do not support their harness. Add a `Codex` row on `.agents/`, keeping the generic `Agent Skills` row for harnesses the catalog does not enumerate. Two rows for one directory is the point: people scan the list for the name of the tool they use. That makes the catalog a list of rows rather than a list of directories, which three call sites assumed. `uniqueShimTargets` collapses it on `dir`, so a selection naming `.agents/` is pre-checked once, planned once, written once, and summarised once. The dedupe is on the directory rather than on the Codex row, so a future pair of rows that share a destination inherits it. Fixes #204 --- .changeset/codex-install-picker-row.md | 29 +++++++++ .../proposal.md | 50 ++++++++++++++ .../specs/cli-init/spec.md | 65 +++++++++++++++++++ .../tasks.md | 31 +++++++++ openspec/specs/cli-init/spec.md | 25 ++++++- packages/cli/src/install/install.ts | 49 +++++++++++++- packages/cli/src/wizard/steps/locations.ts | 31 +++++++-- packages/cli/test/install.test.ts | 51 +++++++++++++++ packages/cli/test/wizard-steps.test.ts | 35 +++++++++- 9 files changed, 351 insertions(+), 15 deletions(-) create mode 100644 .changeset/codex-install-picker-row.md create mode 100644 openspec/changes/archive/2026-08-29-codex-install-picker-row/proposal.md create mode 100644 openspec/changes/archive/2026-08-29-codex-install-picker-row/specs/cli-init/spec.md create mode 100644 openspec/changes/archive/2026-08-29-codex-install-picker-row/tasks.md diff --git a/.changeset/codex-install-picker-row.md b/.changeset/codex-install-picker-row.md new file mode 100644 index 00000000..6c02032a --- /dev/null +++ b/.changeset/codex-install-picker-row.md @@ -0,0 +1,29 @@ +--- +"@taskless/cli": patch +--- + +Name Codex in the install picker, so Codex users can see that it supports them. + +Codex has always been detected and installed into `.agents/`. The tool-selection +step just never said so: it read `Claude Code / Cursor / OpenCode / Agent +Skills`, and the entry that serves Codex is the one whose label only makes sense +if you already know `AGENTS.md` is the file Codex reads. A GPT-centric founder +looked at that list and concluded we did not support GPT. He was wrong, and the +list is why he thought it. Somebody who believes their harness is unsupported +does not file a bug, they leave. + +The picker now offers a `Codex` row alongside the generic `Agent Skills` row, +both pointing at `.agents/`. Two rows for one directory is deliberate: people +scan a list for the name of the tool they use, and `Agent Skills` still has to +be there for anyone on a harness the catalog does not enumerate. Neither label +is redundant, so neither one goes. + +That makes the catalog a list of rows rather than a list of directories, which +was an assumption the code held in three places. A single `.agents/` selection +used to match one row; it now matches two, and would have been pre-checked +twice, planned twice, written twice, and reported twice in the install summary. +The pre-checked set, `detectSelectedDirectories`, and the install plan all +collapse the catalog on `dir` first. The dedupe is on the directory rather than +on the Codex row specifically, so the next pair of rows that share a +destination inherits it instead of reintroducing the bug. Ticking either row +selects `.agents/` once, and ticking both is the same install as ticking one. diff --git a/openspec/changes/archive/2026-08-29-codex-install-picker-row/proposal.md b/openspec/changes/archive/2026-08-29-codex-install-picker-row/proposal.md new file mode 100644 index 00000000..c91d17d0 --- /dev/null +++ b/openspec/changes/archive/2026-08-29-codex-install-picker-row/proposal.md @@ -0,0 +1,50 @@ +## Why + +Codex is detected, routed to `.agents/`, and fully supported. The word +"Codex" never appears in the list the user actually reads. + +The install wizard keeps two lists. `TOOLS` drives detection and knows about +Codex. `SHIM_TARGETS` drives the "which tools do you want to enable Taskless +for?" multiselect and offers `Claude Code / Cursor / OpenCode / Agent Skills`. +A Codex user reads that list and concludes we do not support their harness, +because the support arrives under a label that only makes sense once you +already know `AGENTS.md` is the thing Codex reads. Someone who believes their +harness is unsupported does not file a bug, they leave. + +## What Changes + +- **The picker gains a `Codex` row pointing at `.agents/`.** Two rows now name + the same directory. That is the point: people scan the list for the name of + the tool they use. `Agent Skills` stays, because it is the entry that serves + anyone on a harness the catalog does not enumerate. +- **The catalog becomes a list of rows, not a list of directories.** Every + consumer that turns rows into directories or into install targets now goes + through a single deduplicating helper, so a shared directory is pre-checked + once, planned once, written once, and reported once. The dedupe is on `dir`, + not on the Codex row, so a future pair of rows sharing a destination + inherits the invariant. +- **The generic hint follows the generic row.** `generic agent skills` belongs + to the `Agent Skills` row; the `Codex` row hints `detected` or + `not detected` like any other named harness. + +**Delivery is a single PR.** One catalog row, one helper, three call sites, +tests, and a spec delta. No unit of this is meaningful on its own. + +## Capabilities + +### Modified Capabilities + +- `cli-init`: the tool-selection step names Codex, and a directory offered by + more than one row installs exactly once. + +## Impact + +- **Modified**: `packages/cli/src/install/install.ts` (the `Codex` row, + `uniqueShimTargets`, `detectSelectedDirectories`, `buildInstallPlan`), + `packages/cli/src/wizard/steps/locations.ts` (pre-checked set and hints), + `packages/cli/test/install.test.ts`, + `packages/cli/test/wizard-steps.test.ts`. +- **Unchanged**: detection, the canonical store, the stub format, and the + install manifest, which has always been keyed by directory. + +**Tracking:** taskless/cli#204 diff --git a/openspec/changes/archive/2026-08-29-codex-install-picker-row/specs/cli-init/spec.md b/openspec/changes/archive/2026-08-29-codex-install-picker-row/specs/cli-init/spec.md new file mode 100644 index 00000000..ef189a43 --- /dev/null +++ b/openspec/changes/archive/2026-08-29-codex-install-picker-row/specs/cli-init/spec.md @@ -0,0 +1,65 @@ +## MODIFIED Requirements + +### Requirement: Wizard prompts the user to choose install locations + +The wizard's location step SHALL be presented as a tool-selection step: "which tools do you want to enable Taskless for?". It SHALL offer a fixed catalog of rows: `Claude Code` (`.claude/`), `Codex` (`.agents/`), `Cursor` (`.cursor/`), `OpenCode` (`.opencode/`), and `Agent Skills` (`.agents/`). A row names a tool the user scans for, so more than one row MAY offer the same directory: Codex reads `.agents/`, and a user who arrived from Codex SHALL NOT have to know that before recognising their harness in the list. The generic `Agent Skills` row SHALL remain, since it serves harnesses this catalog does not enumerate. + +Every list of directories derived from the catalog SHALL name each directory at most once, however many rows offer it. The pre-checked set SHALL be the union of (a) every directory recorded as a target in the install manifest (`install.targets`) that matches an offered row, and (b) every detected tool's install directory, deduplicated by directory. When the manifest records no targets AND no tools are detected, `.agents/` SHALL be pre-checked as the first-run default. The canonical `.taskless/` store SHALL NOT appear as a selectable entry and SHALL NOT be pre-checked — it is always written and is never a manifest tool-directory target. + +Each offered entry SHALL carry an origin hint: `installed` when the entry's directory is recorded in the install manifest; otherwise `detected` when the entry's tool is detected on the filesystem; otherwise `not detected`. The generic `Agent Skills` row MAY instead carry a hint describing it as the generic agent-skills location; a row naming a specific harness SHALL NOT carry that hint. The `installed` hint SHALL take precedence over `detected` when both apply. + +Unchecking a pre-checked, manifest-recorded entry SHALL cause the resulting install plan to omit that target, so the existing manifest-diff removal path removes Taskless's reference stubs from that directory. The at-least-one-tool selection rule is unchanged: the wizard SHALL require at least one checked entry. + +Each selected directory SHALL produce exactly one `reference` stub target, even when several offered rows name it; the resulting install plan always contains the single `taskless` skill (and, for `.claude/` and `.cursor/`, the `tskl` command). A directory offered by more than one row SHALL be reported in the install summary under a single label. The function that maps detected tools and manifest targets to multiselect choices SHALL be pure — it SHALL receive both the detected tools and the manifest target list as arguments and SHALL perform no filesystem access — so the mapping is unit-testable. + +#### Scenario: Codex is named in the tool list + +- **WHEN** the wizard renders the tool-selection multiselect +- **THEN** a `Codex` entry SHALL be offered for `.agents/` +- **AND** a separate generic `Agent Skills` entry SHALL also be offered for `.agents/` + +#### Scenario: A directory offered by two rows is pre-checked once + +- **WHEN** the wizard reaches the tool-selection step and Codex is detected +- **THEN** `.agents/` SHALL appear exactly once in the pre-checked set + +#### Scenario: A directory offered by two rows installs once + +- **WHEN** the user's selection includes `.agents/`, whichever of its rows was checked +- **THEN** the install plan SHALL contain exactly one `.agents/` target +- **AND** the `.agents/` skill stub SHALL be written once + +#### Scenario: Detected tools are pre-checked + +- **WHEN** the wizard reaches the tool-selection step and `.claude/` is detected +- **THEN** `.claude/` SHALL be pre-checked in the multiselect +- **AND** `.claude/` SHALL carry the `detected` hint when it is not recorded in the install manifest + +#### Scenario: Manifest-recorded locations are pre-checked + +- **WHEN** the wizard reaches the tool-selection step and the install manifest records `.agents/` as a target +- **THEN** `.agents/` SHALL be pre-checked in the multiselect +- **AND** `.agents/` SHALL carry the `installed` hint + +#### Scenario: Installed hint takes precedence over detected + +- **WHEN** the wizard reaches the tool-selection step and `.claude/` is both detected on the filesystem and recorded in the install manifest +- **THEN** `.claude/` SHALL be pre-checked +- **AND** `.claude/` SHALL carry the `installed` hint, not the `detected` hint + +#### Scenario: Unchecking an installed location removes its stubs + +- **WHEN** the install manifest records `.claude/` and `.agents/` as targets and the user unchecks `.claude/` while leaving `.agents/` checked +- **THEN** the resulting install plan SHALL omit the `.claude/` target +- **AND** the wizard summary SHALL list the `.claude/` reference stubs as removals + +#### Scenario: Agents is the default when nothing is detected or installed + +- **WHEN** the wizard reaches the tool-selection step, no tools are detected, and the install manifest records no tool-directory targets +- **THEN** `.agents/` SHALL be pre-checked + +#### Scenario: Canonical store is not a selectable entry + +- **WHEN** the wizard renders the tool-selection multiselect +- **THEN** `.taskless/` SHALL NOT appear as a selectable option +- **AND** `.taskless/` SHALL NOT be pre-checked even though the manifest records it as a target diff --git a/openspec/changes/archive/2026-08-29-codex-install-picker-row/tasks.md b/openspec/changes/archive/2026-08-29-codex-install-picker-row/tasks.md new file mode 100644 index 00000000..ea97ef3c --- /dev/null +++ b/openspec/changes/archive/2026-08-29-codex-install-picker-row/tasks.md @@ -0,0 +1,31 @@ +Delivery shape: **single PR**. One catalog row, one deduplicating helper, its call sites, tests, and a spec delta. Landing the row without the dedupe would install `.agents/` twice, so the units are only correct together. + +## 1. Catalog + +- [x] 1.1 Add the `Codex` row on `.agents/`, ordered so named harnesses come before the generic `Agent Skills` fallback +- [x] 1.2 Add `uniqueShimTargets`, collapsing the catalog to one entry per directory in catalog order +- [x] 1.3 Say in the code why two rows share a directory, so it is not deleted as a duplicate later +- [x] 1.4 Update the `ShimTarget` doc comment, which read as promising one row per directory + +## 2. Call sites + +- [x] 2.1 `detectSelectedDirectories` maps the deduplicated catalog +- [x] 2.2 `buildInstallPlan` iterates the deduplicated catalog +- [x] 2.3 The wizard's pre-checked set is deduplicated; its options stay one per row +- [x] 2.4 The `generic agent skills` hint follows the generic row rather than the directory + +## 3. Tests + +- [x] 3.1 The picker lists Codex and Agent Skills as separate rows +- [x] 3.2 `detectSelectedDirectories` returns `.agents` once when Codex is detected +- [x] 3.3 `locationChoices` pre-checks `.agents` once when Codex is detected +- [x] 3.4 A selection naming `.agents` produces exactly one plan target, labelled `Agent Skills` +- [x] 3.5 An install of that plan writes each `.agents` skill stub once + +## 4. Verification + +- [x] 4.1 Confirm the tests fail with the dedupe removed +- [x] 4.2 Exercise the built CLI against a scratch directory outside the repo +- [x] 4.3 `pnpm build`, `pnpm typecheck`, `pnpm lint`, `pnpm test`, `pnpm cli check` +- [x] 4.4 `pnpm openspec validate --all --strict` +- [x] 4.5 Changeset diff --git a/openspec/specs/cli-init/spec.md b/openspec/specs/cli-init/spec.md index 724cbf4d..dcba767b 100644 --- a/openspec/specs/cli-init/spec.md +++ b/openspec/specs/cli-init/spec.md @@ -380,13 +380,32 @@ The wizard SHALL begin by rendering an ASCII rendition of the Taskless wordmark ### Requirement: Wizard prompts the user to choose install locations -The wizard's location step SHALL be presented as a tool-selection step: "which tools do you want to enable Taskless for?". It SHALL offer a fixed multiselect of `.claude/`, `.cursor/`, `.opencode/`, and `.agents/`. The pre-checked set SHALL be the union of (a) every directory recorded as a target in the install manifest (`install.targets`) that matches one of the four offered entries, and (b) every detected tool's install directory. When the manifest records no targets AND no tools are detected, `.agents/` SHALL be pre-checked as the first-run default. The canonical `.taskless/` store SHALL NOT appear as a selectable entry and SHALL NOT be pre-checked — it is always written and is never a manifest tool-directory target. +The wizard's location step SHALL be presented as a tool-selection step: "which tools do you want to enable Taskless for?". It SHALL offer a fixed catalog of rows: `Claude Code` (`.claude/`), `Codex` (`.agents/`), `Cursor` (`.cursor/`), `OpenCode` (`.opencode/`), and `Agent Skills` (`.agents/`). A row names a tool the user scans for, so more than one row MAY offer the same directory: Codex reads `.agents/`, and a user who arrived from Codex SHALL NOT have to know that before recognising their harness in the list. The generic `Agent Skills` row SHALL remain, since it serves harnesses this catalog does not enumerate. -Each offered entry SHALL carry an origin hint: `installed` when the entry's directory is recorded in the install manifest; otherwise `detected` when the entry's tool is detected on the filesystem; otherwise `not detected` (the `.agents/` first-run default MAY instead carry a hint describing it as the generic agent-skills location). The `installed` hint SHALL take precedence over `detected` when both apply. +Every list of directories derived from the catalog SHALL name each directory at most once, however many rows offer it. The pre-checked set SHALL be the union of (a) every directory recorded as a target in the install manifest (`install.targets`) that matches an offered row, and (b) every detected tool's install directory, deduplicated by directory. When the manifest records no targets AND no tools are detected, `.agents/` SHALL be pre-checked as the first-run default. The canonical `.taskless/` store SHALL NOT appear as a selectable entry and SHALL NOT be pre-checked — it is always written and is never a manifest tool-directory target. + +Each offered entry SHALL carry an origin hint: `installed` when the entry's directory is recorded in the install manifest; otherwise `detected` when the entry's tool is detected on the filesystem; otherwise `not detected`. The generic `Agent Skills` row MAY instead carry a hint describing it as the generic agent-skills location; a row naming a specific harness SHALL NOT carry that hint. The `installed` hint SHALL take precedence over `detected` when both apply. Unchecking a pre-checked, manifest-recorded entry SHALL cause the resulting install plan to omit that target, so the existing manifest-diff removal path removes Taskless's reference stubs from that directory. The at-least-one-tool selection rule is unchanged: the wizard SHALL require at least one checked entry. -Each checked entry SHALL produce one `reference` stub target; the resulting install plan always contains the single `taskless` skill (and, for `.claude/` and `.cursor/`, the `tskl` command). The function that maps detected tools and manifest targets to multiselect choices SHALL be pure — it SHALL receive both the detected tools and the manifest target list as arguments and SHALL perform no filesystem access — so the mapping is unit-testable. +Each selected directory SHALL produce exactly one `reference` stub target, even when several offered rows name it; the resulting install plan always contains the single `taskless` skill (and, for `.claude/` and `.cursor/`, the `tskl` command). A directory offered by more than one row SHALL be reported in the install summary under a single label. The function that maps detected tools and manifest targets to multiselect choices SHALL be pure — it SHALL receive both the detected tools and the manifest target list as arguments and SHALL perform no filesystem access — so the mapping is unit-testable. + +#### Scenario: Codex is named in the tool list + +- **WHEN** the wizard renders the tool-selection multiselect +- **THEN** a `Codex` entry SHALL be offered for `.agents/` +- **AND** a separate generic `Agent Skills` entry SHALL also be offered for `.agents/` + +#### Scenario: A directory offered by two rows is pre-checked once + +- **WHEN** the wizard reaches the tool-selection step and Codex is detected +- **THEN** `.agents/` SHALL appear exactly once in the pre-checked set + +#### Scenario: A directory offered by two rows installs once + +- **WHEN** the user's selection includes `.agents/`, whichever of its rows was checked +- **THEN** the install plan SHALL contain exactly one `.agents/` target +- **AND** the `.agents/` skill stub SHALL be written once #### Scenario: Detected tools are pre-checked diff --git a/packages/cli/src/install/install.ts b/packages/cli/src/install/install.ts index 1eda8aee..c36263f4 100644 --- a/packages/cli/src/install/install.ts +++ b/packages/cli/src/install/install.ts @@ -126,7 +126,12 @@ export const TOOLS: ToolDescriptor[] = [ /** * A selectable stub destination. The wizard offers this fixed catalog as the * "which tools do you want to enable Taskless for?" multiselect; every entry - * is a peer — no directory is special-cased or routed onto another. + * is a peer, no directory is special-cased or routed onto another. + * + * The catalog is a list of *rows the user reads*, not a list of directories: + * more than one row may name the same `dir`. Anything downstream of the + * picker works in directories and must therefore collapse the rows first, + * which is what {@link uniqueShimTargets} is for. */ export interface ShimTarget { /** Directory the stub is written into, relative to the project root. */ @@ -137,13 +142,46 @@ export interface ShimTarget { commands: boolean; } +/** + * Codex and Agent Skills deliberately share `.agents/`. Both rows stay. + * People scan this list for the name of the tool they use, and a Codex user + * who sees only "Agent Skills" concludes Taskless does not support their + * harness, which is how we lost one. "Agent Skills" still serves anyone on a + * harness this catalog does not enumerate. Do not merge the two rows into one + * label: the duplicate directory is the point, not an oversight. + */ export const SHIM_TARGETS: readonly ShimTarget[] = [ { dir: ".claude", label: "Claude Code", commands: true }, + { dir: ".agents", label: "Codex", commands: false }, { dir: ".cursor", label: "Cursor", commands: true }, { dir: ".opencode", label: "OpenCode", commands: false }, { dir: ".agents", label: "Agent Skills", commands: false }, ]; +/** + * The catalog collapsed to one entry per directory, in catalog order. + * + * Every consumer that turns rows into directories or into install targets + * goes through here. Without it, a single `.agents/` selection matches both + * `.agents/` rows and the plan writes, reports, and records that directory + * twice. Deduping on `dir` (rather than special-casing Codex) keeps the + * invariant true for any future pair of rows that share a destination. + * + * A `Map` keyed by `dir` keeps the first occurrence's position while the last + * row wins the value, so a shared directory reports under the catalog's final + * row for it. The catalog is ordered named-harness first, generic-fallback + * last, so `.agents/` is summarised as "Agent Skills": the selection carries + * only a directory, never which row was ticked, and the generic label is the + * one that is accurate either way. + */ +export function uniqueShimTargets(): ShimTarget[] { + const byDirectory = new Map(); + for (const shim of SHIM_TARGETS) { + byDirectory.set(shim.dir, shim); + } + return [...byDirectory.values()]; +} + /** Directory selected by default when no tools are detected. */ export const DEFAULT_SHIM_DIR = ".agents"; @@ -186,7 +224,9 @@ export async function detectSelectedDirectories( const tools = await detectTools(cwd); if (tools.length === 0) return [DEFAULT_SHIM_DIR]; const directories = new Set(tools.map((t) => t.installDir)); - return SHIM_TARGETS.map((s) => s.dir).filter((d) => directories.has(d)); + return uniqueShimTargets() + .map((s) => s.dir) + .filter((d) => directories.has(d)); } // --- Embedded Skills --- @@ -270,7 +310,10 @@ export function buildInstallPlan( }); } - for (const shim of SHIM_TARGETS) { + // Iterate the deduplicated catalog: two rows share `.agents/`, and matching + // both would plan that directory twice, so it would be written twice and + // reported twice in the install summary. + for (const shim of uniqueShimTargets()) { if (!selectedDirectories.includes(shim.dir)) continue; targets.push({ dir: shim.dir, diff --git a/packages/cli/src/wizard/steps/locations.ts b/packages/cli/src/wizard/steps/locations.ts index d04bc45f..475ed62a 100644 --- a/packages/cli/src/wizard/steps/locations.ts +++ b/packages/cli/src/wizard/steps/locations.ts @@ -5,6 +5,7 @@ import { DEFAULT_SHIM_DIR, SHIM_TARGETS, detectTools, + uniqueShimTargets, type ToolDescriptor, } from "../../install/install"; import { readInstallState } from "../../install/state"; @@ -22,8 +23,10 @@ export interface LocationChoice { * the detected tools and the install manifest's recorded targets. Pure — no * prompt, no TTY, no filesystem access — so the mapping is unit-testable. * - * Every shim target is always offered (a peer list); the canonical - * `.taskless/` store is never an entry. The pre-checked set is the union of + * Every shim target row is always offered (a peer list); the canonical + * `.taskless/` store is never an entry. Rows may share a directory (Codex and + * Agent Skills both write `.agents/`), so options are one per row while the + * pre-checked set is one per directory. The pre-checked set is the union of * manifest-recorded shim directories and detected tools' directories, so a * location Taskless already installed into shows checked and can be * unchecked. `.agents/` is pre-checked as the default only when nothing is @@ -48,12 +51,26 @@ export function locationChoices( manifestDirectories.filter((d) => d !== CANONICAL_DIR) ); - const preChecked = SHIM_TARGETS.map((s) => s.dir).filter( - (directory) => - installedDirectories.has(directory) || detectedDirectories.has(directory) - ); + // Pre-checked entries are directories, not rows, so they come from the + // deduplicated catalog. Codex and Agent Skills both offer `.agents/`, and a + // pre-check list naming it twice would hand the multiselect a duplicate + // initial value and carry the duplicate into the install plan. + const preChecked = uniqueShimTargets() + .map((s) => s.dir) + .filter( + (directory) => + installedDirectories.has(directory) || + detectedDirectories.has(directory) + ); const initialValues = preChecked.length > 0 ? preChecked : [DEFAULT_SHIM_DIR]; + // Options, by contrast, are one per row: the whole point of a second + // `.agents/` row is that a Codex user sees the word "Codex". Rows sharing a + // directory share a value, so ticking either one selects that directory + // once, and the picker renders both as checked. + const fallbackRow = uniqueShimTargets().find( + (shim) => shim.dir === DEFAULT_SHIM_DIR + ); const options = SHIM_TARGETS.map((shim) => ({ value: shim.dir, label: `${shim.label} (${shim.dir}/)`, @@ -61,7 +78,7 @@ export function locationChoices( ? "installed" : detectedDirectories.has(shim.dir) ? "detected" - : shim.dir === DEFAULT_SHIM_DIR + : shim === fallbackRow ? "generic agent skills" : "not detected", })); diff --git a/packages/cli/test/install.test.ts b/packages/cli/test/install.test.ts index 932144c5..5b4a0a84 100644 --- a/packages/cli/test/install.test.ts +++ b/packages/cli/test/install.test.ts @@ -127,6 +127,57 @@ describe("detectSelectedDirectories", () => { await mkdir(join(cwd, ".codex"), { recursive: true }); expect(await detectSelectedDirectories(cwd)).toEqual([".agents"]); }); + + it("returns .agents once even though two catalog rows offer it", async () => { + // Codex and Agent Skills are separate rows in the picker pointing at the + // same directory. A duplicate here would flow into the install plan. + await mkdir(join(cwd, ".codex"), { recursive: true }); + await mkdir(join(cwd, ".claude"), { recursive: true }); + const directories = await detectSelectedDirectories(cwd); + expect(directories).toEqual([".claude", ".agents"]); + expect(new Set(directories).size).toBe(directories.length); + }); +}); + +describe("buildInstallPlan and duplicate shim rows", () => { + it("plans .agents exactly once when it is selected", () => { + const plan = buildInstallPlan( + [".agents"], + getEmbeddedSkills(), + getEmbeddedCommands() + ); + const agentsTargets = plan.targets.filter((t) => t.dir === ".agents"); + expect(agentsTargets).toHaveLength(1); + // The generic row supplies the shared directory's label: the selection + // carries a directory, never which of the two rows the user ticked. + expect(agentsTargets[0]!.label).toBe("Agent Skills"); + expect(agentsTargets[0]!.commands).toEqual([]); + }); + + it("plans .agents once when a caller passes it more than once", () => { + const plan = buildInstallPlan( + [".agents", ".agents", ".claude"], + getEmbeddedSkills(), + getEmbeddedCommands() + ); + expect(plan.targets.filter((t) => t.dir === ".agents")).toHaveLength(1); + expect(plan.targets.filter((t) => t.dir === ".claude")).toHaveLength(1); + }); + + it("writes .agents skill stubs once for a both-rows selection", async () => { + // What a user who ticks both `Codex` and `Agent Skills` gets: the + // multiselect collapses them onto one `.agents` value, and the plan must + // not turn that back into two targets writing the same files twice. + const plan = buildInstallPlan([".agents"], getEmbeddedSkills(), []); + const result = await applyInstallPlan(cwd, plan, { cliVersion: "0.7.0" }); + + const agentsWrites = result.writtenSkills.filter( + (entry) => entry.target === ".agents" + ); + expect(new Set(agentsWrites.map((entry) => entry.skill)).size).toBe( + agentsWrites.length + ); + }); }); describe("checkStaleness", () => { diff --git a/packages/cli/test/wizard-steps.test.ts b/packages/cli/test/wizard-steps.test.ts index 6e5b4e56..8d998c93 100644 --- a/packages/cli/test/wizard-steps.test.ts +++ b/packages/cli/test/wizard-steps.test.ts @@ -36,10 +36,25 @@ describe("locationChoices", () => { it("offers every shim target and never the canonical .taskless store", async () => { const { locationChoices } = await import("../src/wizard/steps/locations"); const values = locationChoices([]).options.map((o) => o.value); - expect(values).toEqual([".claude", ".cursor", ".opencode", ".agents"]); + expect(values).toEqual([ + ".claude", + ".agents", + ".cursor", + ".opencode", + ".agents", + ]); expect(values).not.toContain(".taskless"); }); + it("names Codex in the picker, alongside the generic .agents/ entry", async () => { + const { locationChoices } = await import("../src/wizard/steps/locations"); + const labels = locationChoices([]).options.map((o) => o.label); + // Both rows are offered on purpose: a Codex user scans for "Codex", and + // "Agent Skills" still serves harnesses the catalog does not enumerate. + expect(labels).toContain("Codex (.agents/)"); + expect(labels).toContain("Agent Skills (.agents/)"); + }); + it("pre-checks .agents/ when no tools are detected", async () => { const { locationChoices } = await import("../src/wizard/steps/locations"); expect(locationChoices([]).initialValues).toEqual([".agents"]); @@ -103,9 +118,25 @@ describe("locationChoices", () => { // and the first-run .agents/ default applies. expect(initialValues).toEqual([".agents"]); expect(options.map((o) => o.value)).not.toContain(".taskless"); - expect(options.find((o) => o.value === ".agents")?.hint).toBe( + // Two rows share `.agents/`, so the hint is looked up by label: the + // generic hint belongs to the generic row, not to the Codex one. + expect(options.find((o) => o.label.startsWith("Agent Skills"))?.hint).toBe( "generic agent skills" ); + expect(options.find((o) => o.label.startsWith("Codex"))?.hint).toBe( + "not detected" + ); + }); + + it("pre-checks .agents/ exactly once when Codex is detected", async () => { + const { locationChoices } = await import("../src/wizard/steps/locations"); + const { TOOLS } = await import("../src/install/install"); + const codex = TOOLS.find((t) => t.name === "Codex")!; + + // Codex and Agent Skills both offer `.agents/`; the pre-checked set is + // directories, so it must name that directory once. + const { initialValues } = locationChoices([codex]); + expect(initialValues).toEqual([".agents"]); }); it("falls back to .agents/ when neither manifest nor detection has entries", async () => { From 20f81d24cf1a78fd8b95115c144ed6c229bce1f9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Fri, 28 Aug 2026 22:03:35 -0700 Subject: [PATCH 2/3] fix(install): keep .agents last after deduping shared rows `Map.set` overwrites a key's value but keeps its FIRST insertion position, so deduping the catalog left `.agents` holding the `Codex` row's slot (second) while displaying the `Agent Skills` label. That reordered writes and summary lines in every multi-tool install, for a change meant only to add a label, and it contradicted the generic-fallback-last ordering the function's own comment relies on. Deleting the key before setting it lets the last row win position as well as value. The existing dedupe test could not see this: it detects `.claude`, which sorts first under either ordering. The new test detects `.cursor`, declared between the two `.agents` rows, so it lands between them only when the last row wins. Reported by review on #208. --- packages/cli/src/install/install.ts | 20 ++++++++++++++------ packages/cli/test/install.test.ts | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/install/install.ts b/packages/cli/src/install/install.ts index c36263f4..83843d9d 100644 --- a/packages/cli/src/install/install.ts +++ b/packages/cli/src/install/install.ts @@ -167,16 +167,24 @@ export const SHIM_TARGETS: readonly ShimTarget[] = [ * twice. Deduping on `dir` (rather than special-casing Codex) keeps the * invariant true for any future pair of rows that share a destination. * - * A `Map` keyed by `dir` keeps the first occurrence's position while the last - * row wins the value, so a shared directory reports under the catalog's final - * row for it. The catalog is ordered named-harness first, generic-fallback - * last, so `.agents/` is summarised as "Agent Skills": the selection carries - * only a directory, never which row was ticked, and the generic label is the - * one that is accurate either way. + * The LAST row for a directory wins both its position and its value. The + * catalog is ordered named-harness first, generic-fallback last, so `.agents/` + * is summarised as "Agent Skills" and sorts last: the selection carries only a + * directory, never which row was ticked, and the generic label is the one that + * is accurate either way. + * + * The `delete` before the `set` is what buys the position half. `Map.set` + * alone overwrites the value but keeps the FIRST insertion's position, which + * would leave `.agents/` holding the `Codex` row's slot (second) while + * displaying the `Agent Skills` label. That reorders writes and summary lines + * in every multi-tool install, for a change that is only supposed to add a + * label, and it quietly contradicts the generic-fallback-last ordering this + * comment relies on. */ export function uniqueShimTargets(): ShimTarget[] { const byDirectory = new Map(); for (const shim of SHIM_TARGETS) { + byDirectory.delete(shim.dir); byDirectory.set(shim.dir, shim); } return [...byDirectory.values()]; diff --git a/packages/cli/test/install.test.ts b/packages/cli/test/install.test.ts index 5b4a0a84..cb464f90 100644 --- a/packages/cli/test/install.test.ts +++ b/packages/cli/test/install.test.ts @@ -137,6 +137,21 @@ describe("detectSelectedDirectories", () => { expect(directories).toEqual([".claude", ".agents"]); expect(new Set(directories).size).toBe(directories.length); }); + + it("keeps .agents last, after a tool declared later in the catalog", async () => { + // The discriminating case for dedupe ORDER, which the test above cannot + // see: `.claude` sorts first either way. `.cursor` is declared after the + // `Codex` row and before the `Agent Skills` row, so it lands between them + // only if the LAST row for a directory wins its position. Deduping with a + // bare `Map.set` keeps the FIRST occurrence's slot, which would put + // `.agents` second and report `[".agents", ".cursor"]`. + await mkdir(join(cwd, ".codex"), { recursive: true }); + await mkdir(join(cwd, ".cursor"), { recursive: true }); + expect(await detectSelectedDirectories(cwd)).toEqual([ + ".cursor", + ".agents", + ]); + }); }); describe("buildInstallPlan and duplicate shim rows", () => { From 0c8dd4dbade938cc7937fbcf9ef509891c6fc114 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Fri, 28 Aug 2026 22:19:00 -0700 Subject: [PATCH 3/3] fix(install): make the catch-all row explicit data, not a coincidence The generic hint was found with `shim === fallbackRow`, object identity against the deduped catalog. That held only while uniqueShimTargets kept the original references AND the catalog declared Agent Skills after Codex. Making the dedupe return copies, or reordering the two .agents rows, would have moved the hint onto Codex with no type error. ShimTarget now carries a `generic` flag and the hint reads it, so the two rows can be reordered freely. Verified: with the rows swapped the hint stays on Agent Skills. Also corrects the dedupe comment, which claimed the invariant held for any future pair of rows sharing a destination. It holds for one-entry-per- directory; every other field still comes from the last row, so a future pair disagreeing on `commands` would be resolved by catalog order rather than by anyone deciding. uniqueShimTargets is now called once in locationChoices rather than twice. Reported by review on #208. --- packages/cli/src/install/install.ts | 26 +++++++++++++++++++--- packages/cli/src/wizard/steps/locations.ts | 11 +++++---- packages/cli/test/wizard-steps.test.ts | 14 ++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/install/install.ts b/packages/cli/src/install/install.ts index 83843d9d..9c3915fa 100644 --- a/packages/cli/src/install/install.ts +++ b/packages/cli/src/install/install.ts @@ -140,6 +140,18 @@ export interface ShimTarget { label: string; /** Whether this directory receives the `tskl` command stub. */ commands: boolean; + /** + * Marks the catch-all row: the one offered to anyone whose harness this + * catalog does not name. At most one row carries it. + * + * It is a declared field rather than something derived, because every way + * of inferring it is a coincidence. "The last row for the directory" is + * declaration order, and "the object `uniqueShimTargets` happened to keep" + * is reference identity, so reordering the catalog or making the dedupe + * return copies would silently move the hint to a named harness with no + * type error anywhere. + */ + generic?: boolean; } /** @@ -155,7 +167,7 @@ export const SHIM_TARGETS: readonly ShimTarget[] = [ { dir: ".agents", label: "Codex", commands: false }, { dir: ".cursor", label: "Cursor", commands: true }, { dir: ".opencode", label: "OpenCode", commands: false }, - { dir: ".agents", label: "Agent Skills", commands: false }, + { dir: ".agents", label: "Agent Skills", commands: false, generic: true }, ]; /** @@ -164,8 +176,16 @@ export const SHIM_TARGETS: readonly ShimTarget[] = [ * Every consumer that turns rows into directories or into install targets * goes through here. Without it, a single `.agents/` selection matches both * `.agents/` rows and the plan writes, reports, and records that directory - * twice. Deduping on `dir` (rather than special-casing Codex) keeps the - * invariant true for any future pair of rows that share a destination. + * twice. Deduping on `dir` (rather than special-casing Codex) keeps that + * one-entry-per-directory invariant true for any future pair of rows sharing + * a destination. + * + * What it does NOT do is reconcile the rows: the surviving entry is one whole + * row, so EVERY field comes from the last row for that directory, not just + * `label`. Today both `.agents/` rows agree on `commands`, so nothing is + * silently chosen. A future pair that disagreed on it would be resolved by + * catalog order rather than by anyone deciding, which is a reason to keep + * rows sharing a directory identical apart from `label` and `generic`. * * The LAST row for a directory wins both its position and its value. The * catalog is ordered named-harness first, generic-fallback last, so `.agents/` diff --git a/packages/cli/src/wizard/steps/locations.ts b/packages/cli/src/wizard/steps/locations.ts index 475ed62a..b35cc5b4 100644 --- a/packages/cli/src/wizard/steps/locations.ts +++ b/packages/cli/src/wizard/steps/locations.ts @@ -68,9 +68,12 @@ export function locationChoices( // `.agents/` row is that a Codex user sees the word "Codex". Rows sharing a // directory share a value, so ticking either one selects that directory // once, and the picker renders both as checked. - const fallbackRow = uniqueShimTargets().find( - (shim) => shim.dir === DEFAULT_SHIM_DIR - ); + // + // The catch-all row is identified by its own `generic` flag rather than by + // object identity against the deduped catalog. That comparison was correct + // only while `uniqueShimTargets()` returned the original references AND the + // catalog happened to declare "Agent Skills" after "Codex"; either changing + // would have moved the hint onto a named harness with nothing to catch it. const options = SHIM_TARGETS.map((shim) => ({ value: shim.dir, label: `${shim.label} (${shim.dir}/)`, @@ -78,7 +81,7 @@ export function locationChoices( ? "installed" : detectedDirectories.has(shim.dir) ? "detected" - : shim === fallbackRow + : shim.generic === true ? "generic agent skills" : "not detected", })); diff --git a/packages/cli/test/wizard-steps.test.ts b/packages/cli/test/wizard-steps.test.ts index 8d998c93..24f970c9 100644 --- a/packages/cli/test/wizard-steps.test.ts +++ b/packages/cli/test/wizard-steps.test.ts @@ -55,6 +55,20 @@ describe("locationChoices", () => { expect(labels).toContain("Agent Skills (.agents/)"); }); + it("puts the generic hint on the catch-all row, not on Codex", async () => { + const { locationChoices } = await import("../src/wizard/steps/locations"); + const byLabel = new Map( + locationChoices([]).options.map((o) => [o.label, o.hint]) + ); + // Both rows write `.agents/`, so nothing about the DIRECTORY separates + // them. The hint has to follow the row's own `generic` flag. Deriving it + // from catalog order or from object identity against the deduped catalog + // put it on whichever row happened to win, which is Codex the moment the + // two rows are reordered. + expect(byLabel.get("Agent Skills (.agents/)")).toBe("generic agent skills"); + expect(byLabel.get("Codex (.agents/)")).toBe("not detected"); + }); + it("pre-checks .agents/ when no tools are detected", async () => { const { locationChoices } = await import("../src/wizard/steps/locations"); expect(locationChoices([]).initialValues).toEqual([".agents"]);