Skip to content

Commit 0c8dd4d

Browse files
committed
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.
1 parent 20f81d2 commit 0c8dd4d

3 files changed

Lines changed: 44 additions & 7 deletions

File tree

‎packages/cli/src/install/install.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,18 @@ export interface ShimTarget {
140140
label: string;
141141
/** Whether this directory receives the `tskl` command stub. */
142142
commands: boolean;
143+
/**
144+
* Marks the catch-all row: the one offered to anyone whose harness this
145+
* catalog does not name. At most one row carries it.
146+
*
147+
* It is a declared field rather than something derived, because every way
148+
* of inferring it is a coincidence. "The last row for the directory" is
149+
* declaration order, and "the object `uniqueShimTargets` happened to keep"
150+
* is reference identity, so reordering the catalog or making the dedupe
151+
* return copies would silently move the hint to a named harness with no
152+
* type error anywhere.
153+
*/
154+
generic?: boolean;
143155
}
144156

145157
/**
@@ -155,7 +167,7 @@ export const SHIM_TARGETS: readonly ShimTarget[] = [
155167
{ dir: ".agents", label: "Codex", commands: false },
156168
{ dir: ".cursor", label: "Cursor", commands: true },
157169
{ dir: ".opencode", label: "OpenCode", commands: false },
158-
{ dir: ".agents", label: "Agent Skills", commands: false },
170+
{ dir: ".agents", label: "Agent Skills", commands: false, generic: true },
159171
];
160172

161173
/**
@@ -164,8 +176,16 @@ export const SHIM_TARGETS: readonly ShimTarget[] = [
164176
* Every consumer that turns rows into directories or into install targets
165177
* goes through here. Without it, a single `.agents/` selection matches both
166178
* `.agents/` rows and the plan writes, reports, and records that directory
167-
* twice. Deduping on `dir` (rather than special-casing Codex) keeps the
168-
* invariant true for any future pair of rows that share a destination.
179+
* twice. Deduping on `dir` (rather than special-casing Codex) keeps that
180+
* one-entry-per-directory invariant true for any future pair of rows sharing
181+
* a destination.
182+
*
183+
* What it does NOT do is reconcile the rows: the surviving entry is one whole
184+
* row, so EVERY field comes from the last row for that directory, not just
185+
* `label`. Today both `.agents/` rows agree on `commands`, so nothing is
186+
* silently chosen. A future pair that disagreed on it would be resolved by
187+
* catalog order rather than by anyone deciding, which is a reason to keep
188+
* rows sharing a directory identical apart from `label` and `generic`.
169189
*
170190
* The LAST row for a directory wins both its position and its value. The
171191
* catalog is ordered named-harness first, generic-fallback last, so `.agents/`

‎packages/cli/src/wizard/steps/locations.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,17 +68,20 @@ export function locationChoices(
6868
// `.agents/` row is that a Codex user sees the word "Codex". Rows sharing a
6969
// directory share a value, so ticking either one selects that directory
7070
// once, and the picker renders both as checked.
71-
const fallbackRow = uniqueShimTargets().find(
72-
(shim) => shim.dir === DEFAULT_SHIM_DIR
73-
);
71+
//
72+
// The catch-all row is identified by its own `generic` flag rather than by
73+
// object identity against the deduped catalog. That comparison was correct
74+
// only while `uniqueShimTargets()` returned the original references AND the
75+
// catalog happened to declare "Agent Skills" after "Codex"; either changing
76+
// would have moved the hint onto a named harness with nothing to catch it.
7477
const options = SHIM_TARGETS.map((shim) => ({
7578
value: shim.dir,
7679
label: `${shim.label} (${shim.dir}/)`,
7780
hint: installedDirectories.has(shim.dir)
7881
? "installed"
7982
: detectedDirectories.has(shim.dir)
8083
? "detected"
81-
: shim === fallbackRow
84+
: shim.generic === true
8285
? "generic agent skills"
8386
: "not detected",
8487
}));

‎packages/cli/test/wizard-steps.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,20 @@ describe("locationChoices", () => {
5555
expect(labels).toContain("Agent Skills (.agents/)");
5656
});
5757

58+
it("puts the generic hint on the catch-all row, not on Codex", async () => {
59+
const { locationChoices } = await import("../src/wizard/steps/locations");
60+
const byLabel = new Map(
61+
locationChoices([]).options.map((o) => [o.label, o.hint])
62+
);
63+
// Both rows write `.agents/`, so nothing about the DIRECTORY separates
64+
// them. The hint has to follow the row's own `generic` flag. Deriving it
65+
// from catalog order or from object identity against the deduped catalog
66+
// put it on whichever row happened to win, which is Codex the moment the
67+
// two rows are reordered.
68+
expect(byLabel.get("Agent Skills (.agents/)")).toBe("generic agent skills");
69+
expect(byLabel.get("Codex (.agents/)")).toBe("not detected");
70+
});
71+
5872
it("pre-checks .agents/ when no tools are detected", async () => {
5973
const { locationChoices } = await import("../src/wizard/steps/locations");
6074
expect(locationChoices([]).initialValues).toEqual([".agents"]);

0 commit comments

Comments
 (0)