From b74af701edc799fc9f78c7d08957ead34fc2330d Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 6 Jul 2026 01:20:44 -0700 Subject: [PATCH 1/3] test(e2e): enforce exact partition in assertGenerateMatrixCoversTargets and correct overstated coverage comments Follow-ups from the PR #2116 review. Finding 1: assertGenerateMatrixCoversTargets documented an "exact partition" of the declared targets into `testedTargets` and `untested`, but only checked `stray` and `uncovered`. A target left in both `testedTargets` and `untested` (or duplicated within `testedTargets`) broke the exclusive-partition invariant while CI stayed green. Add explicit duplicate and overlap checks so those cases fail CI. Finding 2: soften the in-spec comments for the permissions generate list and the hooks project/global standalone lists to state accurately that the completeness check only enforces enumeration == processor-declared set, not that a matching `it` body exists for each name. Closes #2120 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/e2e/e2e-helper.ts | 12 ++++++++++++ src/e2e/e2e-hooks.spec.ts | 7 ++++++- src/e2e/e2e-permissions.spec.ts | 7 +++++-- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/e2e/e2e-helper.ts b/src/e2e/e2e-helper.ts index 19aa87625..b3c79df75 100644 --- a/src/e2e/e2e-helper.ts +++ b/src/e2e/e2e-helper.ts @@ -200,6 +200,18 @@ export function assertGenerateMatrixCoversTargets({ .filter((target) => !target.endsWith("-legacy")); const declaredSet = new Set(declared); + const duplicates = testedTargets.filter((t, i) => testedTargets.indexOf(t) !== i).toSorted(); + expect( + Array.from(new Set(duplicates)), + `These targets appear more than once in \`testedTargets\`: ${duplicates.join(", ")}`, + ).toEqual([]); + + const overlap = testedTargets.filter((t) => untested.includes(t)).toSorted(); + expect( + overlap, + `These targets are listed in both \`testedTargets\` and \`untested\` (a target cannot be both tested and intentionally untested): ${overlap.join(", ")}`, + ).toEqual([]); + const stray = [...testedTargets, ...untested].filter((t) => !declaredSet.has(t)).toSorted(); expect( stray, diff --git a/src/e2e/e2e-hooks.spec.ts b/src/e2e/e2e-hooks.spec.ts index 4b97a396b..442f00fab 100644 --- a/src/e2e/e2e-hooks.spec.ts +++ b/src/e2e/e2e-hooks.spec.ts @@ -29,7 +29,9 @@ function assertHookCommandsPreserved(parsed: { hooks?: unknown }): void { // Tools whose event mapping/serialization needs a // bespoke assertion (vibe, devin, reasonix) live in their own standalone `it`s // below; `hooksProjectStandaloneTargets` lists them so the completeness check -// still accounts for them. +// still accounts for them. The check only enforces that this enumeration matches +// the processor's declared target set — it does NOT verify a matching standalone +// `it` exists for each name, so keep this list in sync with the actual `it`s by hand. const hooksGenerateTargets = [ { target: "claudecode", outputPath: join(".claude", "settings.json") }, { target: "cursor", outputPath: join(".cursor", "hooks.json") }, @@ -576,6 +578,9 @@ const hooksGlobalTargets = [ ] as const; // Global targets exercised by dedicated `it`s (bespoke per-tool serialization). +// As with the project-scope list, the completeness check only enforces that this +// enumeration matches the processor's declared set — not that a matching `it` +// exists for each name; keep it in sync with the actual `it`s by hand. const hooksGlobalStandaloneTargets = ["devin", "vibe", "hermesagent", "reasonix"] as const; describe("E2E: hooks (global mode)", () => { diff --git a/src/e2e/e2e-permissions.spec.ts b/src/e2e/e2e-permissions.spec.ts index ca95a12c2..60b386c94 100644 --- a/src/e2e/e2e-permissions.spec.ts +++ b/src/e2e/e2e-permissions.spec.ts @@ -17,8 +17,11 @@ import { // Permissions targets exercised by the project-scope generate `it`s below. Each // tool has a bespoke serialization, so tests stay hand-written rather than -// table-driven; this explicit list feeds the completeness check so a new -// project-scope permissions tool cannot be added without a matching e2e test. +// table-driven; this explicit list feeds the completeness check. Note the check +// only enforces that this enumeration matches the processor's declared target +// set — it does NOT verify that a dedicated `it` body exists for each name, so a +// tool's `it` could be deleted while its name lingers here and the check stays +// green. Keep this list in sync with the actual `it`s by hand. const permissionsGenerateTargets = [ "opencode", "zed", From d8ac3bcd961d8a9daf9a0a947410e773ea9b9df0 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 6 Jul 2026 01:24:54 -0700 Subject: [PATCH 2/3] test(e2e): also check untested-side duplicates and dedupe error message Address review follow-ups on the exact-partition enforcement: - Symmetrically detect duplicates within `untested`, not just `testedTargets`, since a true exact partition requires each side to be internally unique. - Extract the sorted-unique-duplicates logic into a module-scope helper so the duplicate error messages show each name once. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/e2e/e2e-helper.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/e2e/e2e-helper.ts b/src/e2e/e2e-helper.ts index b3c79df75..261e8e861 100644 --- a/src/e2e/e2e-helper.ts +++ b/src/e2e/e2e-helper.ts @@ -184,6 +184,11 @@ type ProcessorTargets = { * Mirrors the "derive from the implementation, fail on drift" idiom already used * by the TOOL_DISPLAY completeness check and the gitignore derivation. */ +/** Returns the sorted, de-duplicated set of values that appear more than once. */ +function uniqueDuplicates(targets: readonly string[]): string[] { + return Array.from(new Set(targets.filter((t, i) => targets.indexOf(t) !== i))).toSorted(); +} + export function assertGenerateMatrixCoversTargets({ processor, testedTargets, @@ -200,10 +205,18 @@ export function assertGenerateMatrixCoversTargets({ .filter((target) => !target.endsWith("-legacy")); const declaredSet = new Set(declared); - const duplicates = testedTargets.filter((t, i) => testedTargets.indexOf(t) !== i).toSorted(); + // A true "exact partition" requires each side to have no internal duplicates, + // so check both `testedTargets` and `untested` symmetrically. + const testedDuplicates = uniqueDuplicates(testedTargets); + expect( + testedDuplicates, + `These targets appear more than once in \`testedTargets\`: ${testedDuplicates.join(", ")}`, + ).toEqual([]); + + const untestedDuplicates = uniqueDuplicates(untested); expect( - Array.from(new Set(duplicates)), - `These targets appear more than once in \`testedTargets\`: ${duplicates.join(", ")}`, + untestedDuplicates, + `These targets appear more than once in \`untested\`: ${untestedDuplicates.join(", ")}`, ).toEqual([]); const overlap = testedTargets.filter((t) => untested.includes(t)).toSorted(); From a876b872a25207b3c9b01977d9d97c380c7976f7 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 6 Jul 2026 01:44:33 -0700 Subject: [PATCH 3/3] test(e2e): only enforce tested/untested disjointness, allow within-side duplicates The earlier duplicate-within-`testedTargets` check was too strict: a tool can legitimately appear more than once in `testedTargets` when it is exercised by several matrices (e.g. antigravity-ide emits both a root AGENTS.md and a nested .agents/rules tree, so `e2e-rules.spec.ts` lists it in both matrix arrays). That is not a contradiction and must not fail CI. The only real invariant the stray/uncovered checks cannot catch is a tool listed in BOTH `testedTargets` and `untested` (each set covers it, so neither fires). Keep just that disjointness (overlap) check and document that within-side duplicates are allowed. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/e2e/e2e-helper.ts | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/src/e2e/e2e-helper.ts b/src/e2e/e2e-helper.ts index 261e8e861..d42af0f4d 100644 --- a/src/e2e/e2e-helper.ts +++ b/src/e2e/e2e-helper.ts @@ -171,24 +171,25 @@ type ProcessorTargets = { * processor without wiring it into the matrix (or dropping one) fails CI instead * of silently eroding coverage. * - * The declared targets (from `getToolTargets`) must partition exactly into + * The declared targets (from `getToolTargets`) must be covered by the union of * `testedTargets` (tools with an entry in the matrix `it.each` dictionary) and * `untested` (tools intentionally excluded from this matrix — e.g. tools whose * output only exists in another scope, or that merge into a shared file). Every * excluded tool must be listed explicitly with a reason so the omission is a * conscious decision rather than an accidental gap. * + * `testedTargets` and `untested` must be disjoint: a tool cannot be both tested + * and intentionally untested. (A tool may legitimately appear more than once + * *within* `testedTargets` when it is exercised by several matrices — e.g. a + * tool that emits both a root `AGENTS.md` and a nested tree — so duplicates + * within a single side are allowed.) + * * `-legacy` targets are dropped from the comparison: they are duplicate aliases * that the same tables/generators exclude, and are never exercised end-to-end. * * Mirrors the "derive from the implementation, fail on drift" idiom already used * by the TOOL_DISPLAY completeness check and the gitignore derivation. */ -/** Returns the sorted, de-duplicated set of values that appear more than once. */ -function uniqueDuplicates(targets: readonly string[]): string[] { - return Array.from(new Set(targets.filter((t, i) => targets.indexOf(t) !== i))).toSorted(); -} - export function assertGenerateMatrixCoversTargets({ processor, testedTargets, @@ -205,20 +206,9 @@ export function assertGenerateMatrixCoversTargets({ .filter((target) => !target.endsWith("-legacy")); const declaredSet = new Set(declared); - // A true "exact partition" requires each side to have no internal duplicates, - // so check both `testedTargets` and `untested` symmetrically. - const testedDuplicates = uniqueDuplicates(testedTargets); - expect( - testedDuplicates, - `These targets appear more than once in \`testedTargets\`: ${testedDuplicates.join(", ")}`, - ).toEqual([]); - - const untestedDuplicates = uniqueDuplicates(untested); - expect( - untestedDuplicates, - `These targets appear more than once in \`untested\`: ${untestedDuplicates.join(", ")}`, - ).toEqual([]); - + // `testedTargets` and `untested` must be disjoint — a tool listed as both + // tested and intentionally untested is a contradiction the stray/uncovered + // checks below cannot catch (each set covers it, so neither fires). const overlap = testedTargets.filter((t) => untested.includes(t)).toSorted(); expect( overlap,