From 982ba53cf3a206d223710d2a0def8f8f1f0bbf61 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 22 Jun 2026 20:30:26 -0700 Subject: [PATCH 1/2] fix(generate): correct root-file ownership for wildcard configs and mirrored roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up fixes for the #1978 root-file ownership work (#1981): - #1 (mid): `extractConfigFileTargets` collapsed the wildcard array form `targets: ["*"]` to an empty list, so `getConfigFileTargets()` did not fall back and ownership was computed against no targets — reproducing #1894 for the very common `["*"]` config. Expand `*` to the full non-legacy target set (mirroring `Config.getTargets()`). Note the fallback to `getTargets()` is intentionally NOT used because it is CLI `-t` filtered, whereas ownership needs the full config-file target list. - #2 (mid): `computeRootFileOwnership` derived owned roots solely from `getSettablePaths().root`, missing rovodev's generation-time mirror to project-root ./AGENTS.md (mirrorsRootToAgentsMd) and any target's alternativeRoots. Register both so mirrored/aliased root files are attributed to the right target instead of inverting ownership. - #3/#4 (low): document the single-decision-across-output-roots assumption and the load-bearing last-target-in-config-order-wins semantics at the call site. - #5 (low): add a config-resolver regression test for `["*"]` expansion (incl. CLI -t independence) and an E2E test asserting a non-owning target passes check when rovodev owns the mirrored ./AGENTS.md. Closes #1981 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/config/config-resolver.test.ts | 38 ++++++++++++++++++++ src/config/config-resolver.ts | 13 +++++++ src/config/config.ts | 2 +- src/e2e/e2e-rules.spec.ts | 57 ++++++++++++++++++++++++++++++ src/lib/generate.ts | 35 ++++++++++++++++-- 5 files changed, 142 insertions(+), 3 deletions(-) diff --git a/src/config/config-resolver.test.ts b/src/config/config-resolver.test.ts index db39e1f12..47ba148ea 100644 --- a/src/config/config-resolver.test.ts +++ b/src/config/config-resolver.test.ts @@ -158,6 +158,44 @@ describe("config-resolver", () => { }); }); + describe("config file targets (getConfigFileTargets)", () => { + it("expands wildcard targets ['*'] to the full non-legacy target list", async () => { + // Regression for #1981 / #1894: a `targets: ["*"]` config must not collapse + // to an empty config-file target list, otherwise root-file ownership in + // `generate --check` is computed against no targets and the bug reproduces. + const configContent = JSON.stringify({ outputRoots: ["./"], targets: ["*"] }); + await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); + + const config = await ConfigResolver.resolve({ + configPath: join(testDir, "rulesync.jsonc"), + }); + + const configFileTargets = config.getConfigFileTargets(); + expect(configFileTargets.length).toBeGreaterThan(1); + expect(configFileTargets).toContain("claudecode"); + expect(configFileTargets).toContain("codexcli"); + // Legacy targets are excluded from wildcard expansion (must be explicit). + expect(configFileTargets).not.toContain("claudecode-legacy"); + expect(configFileTargets).not.toContain("antigravity"); + }); + + it("keeps the full config-file target list even when CLI -t selects one target", async () => { + const configContent = JSON.stringify({ outputRoots: ["./"], targets: ["*"] }); + await writeFileContent(join(testDir, "rulesync.jsonc"), configContent); + + const config = await ConfigResolver.resolve({ + configPath: join(testDir, "rulesync.jsonc"), + targets: ["codexcli"], + }); + + // CLI -t narrows the generated targets, but config-file ownership must + // still see every target the config lists. + expect(config.getTargets()).toEqual(["codexcli"]); + expect(config.getConfigFileTargets().length).toBeGreaterThan(1); + expect(config.getConfigFileTargets()).toContain("claudecode"); + }); + }); + describe("base directory resolution", () => { it("should load configured outputRoots from file", async () => { const configContent = JSON.stringify({ diff --git a/src/config/config-resolver.ts b/src/config/config-resolver.ts index b9352bd3d..79b924085 100644 --- a/src/config/config-resolver.ts +++ b/src/config/config-resolver.ts @@ -26,6 +26,7 @@ import { ConfigFile, ConfigFileSchema, ConfigParams, + LEGACY_TARGETS, PartialConfigParams, RequiredConfigParams, } from "./config.js"; @@ -359,5 +360,17 @@ function extractConfigFileTargets( if (isRulesyncConfigTargetsObject(targets)) { return Object.keys(targets).filter((key): key is ToolTarget => validTargets.has(key)); } + // The wildcard form `["*"]` lists every (non-legacy) target in the config + // file. Expand it here — mirroring `Config.getTargets()` — so the returned + // list is the full config-file target set rather than an empty array. An + // empty result would make `getConfigFileTargets()` fall back to the + // CLI-filtered `getTargets()`, breaking root-file ownership computation for + // the very common `targets: ["*"]` form (see #1981 / #1894). + if (targets.includes("*")) { + const legacy = new Set(LEGACY_TARGETS); + return ALL_TOOL_TARGETS.filter( + (target): target is ToolTarget => validTargets.has(target) && !legacy.has(target), + ); + } return targets.filter((key): key is ToolTarget => key !== "*" && validTargets.has(key)); } diff --git a/src/config/config.ts b/src/config/config.ts index 694252024..2195c6a7b 100644 --- a/src/config/config.ts +++ b/src/config/config.ts @@ -152,7 +152,7 @@ const CONFLICTING_TARGET_PAIRS: Array<[string, string]> = [ * Legacy targets that should NOT be included in wildcard (*) expansion. * These targets must be explicitly specified. */ -const LEGACY_TARGETS = ["augmentcode-legacy", "claudecode-legacy", "antigravity"] as const; +export const LEGACY_TARGETS = ["augmentcode-legacy", "claudecode-legacy", "antigravity"] as const; /** * Validates that the user-authored config does not double-define the diff --git a/src/e2e/e2e-rules.spec.ts b/src/e2e/e2e-rules.spec.ts index 98f04ef5a..acabd64c9 100644 --- a/src/e2e/e2e-rules.spec.ts +++ b/src/e2e/e2e-rules.spec.ts @@ -436,6 +436,63 @@ globs: ["src/**/*"] expect(stderr).toBe(""); expect(stdout).toContain("All files are up to date."); }); + + it("should attribute rovodev's mirrored ./AGENTS.md so a non-owning target passes check (#1981 #2)", async () => { + const testDir = getTestDir(); + + const rootRuleContent = `--- +root: true +targets: ["*"] +description: "Root rule" +--- + +# Root Rule +`; + await writeFileContent( + join(testDir, RULESYNC_RULES_RELATIVE_DIR_PATH, RULESYNC_OVERVIEW_FILE_NAME), + rootRuleContent, + ); + + await writeFileContent( + join(testDir, RULESYNC_CONFIG_RELATIVE_FILE_PATH), + JSON.stringify( + { + targets: { + codexcli: ["rules"], + rovodev: ["rules"], + }, + }, + null, + 2, + ), + ); + + // rovodev is last in config order and mirrors its root rule to the project + // root ./AGENTS.md, so that mirrored file ends up on disk with rovodev's + // content (an "Additional Conventions" preamble codexcli never emits). + await runGenerate({ + target: "codexcli,rovodev", + features: "rules", + env: { NODE_ENV: "e2e" }, + }); + + const agentsMd = await readFileContent(join(testDir, "AGENTS.md")); + expect(agentsMd).toContain("Additional Conventions"); + + // Check codexcli only. Even though codexcli's own root output is ./AGENTS.md, + // rovodev owns the on-disk mirror, so the file is skipped and the check + // passes. Without crediting the mirror to rovodev, codexcli would be treated + // as the owner and fail on the content mismatch. + const { stdout, stderr } = await runGenerate({ + target: "codexcli", + features: "rules", + check: true, + env: { NODE_ENV: "e2e" }, + }); + + expect(stderr).toBe(""); + expect(stdout).toContain("All files are up to date."); + }); }); describe("E2E: rules (import)", () => { diff --git a/src/lib/generate.ts b/src/lib/generate.ts index d27eecbe7..5e9e2a9ca 100644 --- a/src/lib/generate.ts +++ b/src/lib/generate.ts @@ -3,6 +3,7 @@ import { join } from "node:path"; import { intersection } from "es-toolkit"; import { Config } from "../config/config.js"; +import { AGENTSMD_RULE_FILE_NAME } from "../constants/agentsmd-paths.js"; import { RULESYNC_RELATIVE_DIR_PATH } from "../constants/rulesync-paths.js"; import { CommandsProcessor } from "../features/commands/commands-processor.js"; import { HooksProcessor } from "../features/hooks/hooks-processor.js"; @@ -249,18 +250,48 @@ export async function generate(params: { }; } +// Maps every root-rule file path a target actually emits to that target, so +// `generate --check` can skip root files a (CLI-selected) target does not own. +// +// Ownership is "last target in config order wins": the loop iterates the config +// file's full target list and `Map.set` overwrites, so the final writer in +// config order owns a shared path — consistent with generation write order, +// where the last target's content is what ends up on disk. +// +// Note: a single ownership decision is applied uniformly across all output +// roots (paths are output-root-relative). Multi-output-root `--check` would +// need per-output-root keying; that is out of scope here. function computeRootFileOwnership(params: { targets: ToolTarget[]; global: boolean; }): Map { const ownerByPath = new Map(); + const register = ( + relativeDirPath: string, + relativeFilePath: string, + target: ToolTarget, + ): void => { + ownerByPath.set(toPosixPath(join(relativeDirPath, relativeFilePath)), target); + }; for (const target of params.targets) { const factory = RulesProcessor.getFactory(target); if (!factory) continue; const paths = factory.class.getSettablePaths({ global: params.global }); if ("root" in paths && paths.root) { - const rootPath = toPosixPath(join(paths.root.relativeDirPath, paths.root.relativeFilePath)); - ownerByPath.set(rootPath, target); + register(paths.root.relativeDirPath, paths.root.relativeFilePath, target); + } + // Alternative root files (fallback root locations) are also emitted/owned. + if ("alternativeRoots" in paths && paths.alternativeRoots) { + for (const alt of paths.alternativeRoots) { + register(alt.relativeDirPath, alt.relativeFilePath, target); + } + } + // Some targets (e.g. rovodev) mirror their primary root — which lives in a + // subdirectory — to a project-root `./AGENTS.md` at generation time (project + // scope only). That mirror is exactly the shared-collision path, so it must + // be attributed to the target too, otherwise ownership/skip decisions invert. + if (!params.global && factory.meta.mirrorsRootToAgentsMd) { + register(".", AGENTSMD_RULE_FILE_NAME, target); } } return ownerByPath; From 9f1c56f8f01765dab9555c072d44525e0e4897ab Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 22 Jun 2026 20:36:56 -0700 Subject: [PATCH 2/2] refactor(config): extract shared expandWildcardTargets helper (review follow-up) Address PR review findings on #2008: - Dedupe the wildcard (`*`) -> non-legacy-target expansion into a single exported `expandWildcardTargets()` helper used by both `Config.getTargets()` and `extractConfigFileTargets()`, removing the drift risk (and the redundant `validTargets.has` check). - Reword the `alternativeRoots` ownership comment (they are fallback/secondary root locations, not generation-emitted) and note the rovodev mirror overlaps its alt root today. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/config/config-resolver.ts | 17 +++++++---------- src/config/config.ts | 17 ++++++++++++----- src/lib/generate.ts | 6 +++++- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/config/config-resolver.ts b/src/config/config-resolver.ts index 79b924085..299296d7c 100644 --- a/src/config/config-resolver.ts +++ b/src/config/config-resolver.ts @@ -26,7 +26,7 @@ import { ConfigFile, ConfigFileSchema, ConfigParams, - LEGACY_TARGETS, + expandWildcardTargets, PartialConfigParams, RequiredConfigParams, } from "./config.js"; @@ -361,16 +361,13 @@ function extractConfigFileTargets( return Object.keys(targets).filter((key): key is ToolTarget => validTargets.has(key)); } // The wildcard form `["*"]` lists every (non-legacy) target in the config - // file. Expand it here — mirroring `Config.getTargets()` — so the returned - // list is the full config-file target set rather than an empty array. An - // empty result would make `getConfigFileTargets()` fall back to the - // CLI-filtered `getTargets()`, breaking root-file ownership computation for - // the very common `targets: ["*"]` form (see #1981 / #1894). + // file. Expand it via the shared helper (also used by `Config.getTargets()`) + // so the returned list is the full config-file target set rather than an + // empty array. An empty result would make `getConfigFileTargets()` fall back + // to the CLI-filtered `getTargets()`, breaking root-file ownership + // computation for the very common `targets: ["*"]` form (see #1981 / #1894). if (targets.includes("*")) { - const legacy = new Set(LEGACY_TARGETS); - return ALL_TOOL_TARGETS.filter( - (target): target is ToolTarget => validTargets.has(target) && !legacy.has(target), - ); + return expandWildcardTargets(); } return targets.filter((key): key is ToolTarget => key !== "*" && validTargets.has(key)); } diff --git a/src/config/config.ts b/src/config/config.ts index 2195c6a7b..eaf5c746e 100644 --- a/src/config/config.ts +++ b/src/config/config.ts @@ -154,6 +154,17 @@ const CONFLICTING_TARGET_PAIRS: Array<[string, string]> = [ */ export const LEGACY_TARGETS = ["augmentcode-legacy", "claudecode-legacy", "antigravity"] as const; +/** + * Expand the wildcard target (`*`) to every non-legacy tool target. Legacy + * targets are excluded because they must be requested explicitly. Shared by + * `Config.getTargets()` and `extractConfigFileTargets()` so the two never drift. + */ +export function expandWildcardTargets(): ToolTarget[] { + return ALL_TOOL_TARGETS.filter( + (target) => !LEGACY_TARGETS.includes(target as (typeof LEGACY_TARGETS)[number]), + ); +} + /** * Validates that the user-authored config does not double-define the * target set in both `targets` and `features` object forms. @@ -422,11 +433,7 @@ export class Config { } if (arrayTargets.includes("*")) { - // Exclude legacy targets from wildcard expansion - // Legacy targets must be explicitly specified - return ALL_TOOL_TARGETS.filter( - (target) => !LEGACY_TARGETS.includes(target as (typeof LEGACY_TARGETS)[number]), - ); + return expandWildcardTargets(); } return arrayTargets.filter((target): target is ToolTarget => target !== "*"); diff --git a/src/lib/generate.ts b/src/lib/generate.ts index 5e9e2a9ca..49ae59633 100644 --- a/src/lib/generate.ts +++ b/src/lib/generate.ts @@ -280,7 +280,9 @@ function computeRootFileOwnership(params: { if ("root" in paths && paths.root) { register(paths.root.relativeDirPath, paths.root.relativeFilePath, target); } - // Alternative root files (fallback root locations) are also emitted/owned. + // Secondary/fallback root locations a target recognizes are attributed to + // it as well, so a shared collision at one of those paths is skipped for + // non-owning targets. if ("alternativeRoots" in paths && paths.alternativeRoots) { for (const alt of paths.alternativeRoots) { register(alt.relativeDirPath, alt.relativeFilePath, target); @@ -290,6 +292,8 @@ function computeRootFileOwnership(params: { // subdirectory — to a project-root `./AGENTS.md` at generation time (project // scope only). That mirror is exactly the shared-collision path, so it must // be attributed to the target too, otherwise ownership/skip decisions invert. + // (For rovodev this overlaps its `alternativeRoots` today; the explicit + // block keeps ownership correct even if that alt root is ever removed.) if (!params.global && factory.meta.mirrorsRootToAgentsMd) { register(".", AGENTSMD_RULE_FILE_NAME, target); }