diff --git a/.github/workflows/cli-ci.yml b/.github/workflows/cli-ci.yml index 98cd60ca3..4ea0d0671 100644 --- a/.github/workflows/cli-ci.yml +++ b/.github/workflows/cli-ci.yml @@ -250,6 +250,20 @@ jobs: node plugins/aidd-telemetry/hooks/journal.js turn-end < payload.json find aidd_docs/runs -type f | xargs -I{} cat {} + - name: 'Round trip - #707 the journal stays private AND git add -A still works' + id: rt2acl + continue-on-error: true + run: | + echo "--- icacls aidd_docs/runs (directory ACL) ---" + icacls aidd_docs/runs + runfile=$(find aidd_docs/runs -maxdepth 1 -type f -name '*.jsonl' | head -1) + echo "--- icacls $runfile (journal file ACL) ---" + icacls "$runfile" + echo "--- git add -A, in the checkout that just wrote the journal above ---" + git add -A + echo "git add -A exit code: $?" + git status --porcelain + - name: Round trip - read it back id: rt3 continue-on-error: true @@ -299,7 +313,7 @@ jobs: for outcome in \ "${{ steps.answer1.outcome }}" "${{ steps.answer2.outcome }}" \ "${{ steps.answer3bash.outcome }}" "${{ steps.answer3pwsh.outcome }}" \ - "${{ steps.rt1.outcome }}" "${{ steps.rt2.outcome }}" "${{ steps.rt3.outcome }}" "${{ steps.rt4.outcome }}" \ + "${{ steps.rt1.outcome }}" "${{ steps.rt2.outcome }}" "${{ steps.rt2acl.outcome }}" "${{ steps.rt3.outcome }}" "${{ steps.rt4.outcome }}" \ "${{ steps.pluginsuite.outcome }}" \ "${{ steps.cliunit.outcome }}" "${{ steps.cliintegration.outcome }}" "${{ steps.clie2e.outcome }}"; do if [ "$outcome" = "failure" ]; then diff --git a/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts b/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts index 3659a55b8..75083bd86 100644 --- a/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts +++ b/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts @@ -1,4 +1,4 @@ -import { join } from "node:path"; +import { join, posix } from "node:path"; import { flatHooksSharedDirPath } from "../../../../domain/formats/flat-paths.js"; import { InstallationFile } from "../../../../domain/models/file.js"; import type { Manifest } from "../../../../domain/models/manifest.js"; @@ -128,7 +128,10 @@ export class BuiltTreeMaterializationTranslator implements PluginTranslator { const rel = abs.slice(pluginSrc.length + 1); const content = await this.fs.readFile(abs); return new InstallationFile({ - relativePath: join(name, rel), + // relativePath is always "/"-separated (see withoutHooksPrefix and + // belongsToPlugin below, both string-matching on "/") - node:path's platform + // `join` would answer with "\" on win32, breaking both. + relativePath: posix.join(name, rel), content, hash: this.hasher.hash(content), }); diff --git a/cli/src/application/use-cases/shared/ensure-built-marketplace-use-case.ts b/cli/src/application/use-cases/shared/ensure-built-marketplace-use-case.ts index d2f7cbd02..569c5e755 100644 --- a/cli/src/application/use-cases/shared/ensure-built-marketplace-use-case.ts +++ b/cli/src/application/use-cases/shared/ensure-built-marketplace-use-case.ts @@ -53,10 +53,13 @@ export class EnsureBuiltMarketplaceUseCase { ) {} async execute(options: EnsureBuiltMarketplaceOptions): Promise { - const builtDir = builtMarketplaceDir( - options.projectRoot, - options.marketplace.name, - options.target + // resolve(), matching sourceDir below: builtMarketplaceDir() joins with the platform + // separator, and on Windows a drive-less projectRoot yields a drive-less builtDir here + // while FrameworkBuildUseCase.execute() resolves its own outDir copy for validation only + // - leaving FlatBuildStrategy's write target (captured unresolved at construction) to + // diverge from the path that gets checked. + const builtDir = resolve( + builtMarketplaceDir(options.projectRoot, options.marketplace.name, options.target) ); const resolved = await this.resolveMarketplace.execute({ marketplace: options.marketplace, @@ -112,11 +115,16 @@ export class EnsureBuiltMarketplaceUseCase { await this.runBuild(target, mode, sourceDir, builtDir); } + // "/"-separated comparison, matching FrameworkBuildUseCase.guardPaths(): both dirs are + // native-separated (resolve() returns "\" on Windows), so a hardcoded "/" suffix would + // never match, silently missing genuine nesting. private nested(sourceDir: string, builtDir: string): boolean { + const normalizedSource = sourceDir.replace(/\\/g, "/"); + const normalizedBuilt = builtDir.replace(/\\/g, "/"); return ( - sourceDir === builtDir || - builtDir.startsWith(`${sourceDir}/`) || - sourceDir.startsWith(`${builtDir}/`) + normalizedSource === normalizedBuilt || + normalizedBuilt.startsWith(`${normalizedSource}/`) || + normalizedSource.startsWith(`${normalizedBuilt}/`) ); } diff --git a/cli/tests/application/use-cases/plugin/plugin-add-opencode-hooks-install.integration.test.ts b/cli/tests/application/use-cases/plugin/plugin-add-opencode-hooks-install.integration.test.ts index 46a03012f..9ab31dc78 100644 --- a/cli/tests/application/use-cases/plugin/plugin-add-opencode-hooks-install.integration.test.ts +++ b/cli/tests/application/use-cases/plugin/plugin-add-opencode-hooks-install.integration.test.ts @@ -6,7 +6,7 @@ * measurements.md, Phase 7. */ -import { join } from "node:path"; +import { join, posix } from "node:path"; import { describe, expect, it } from "vitest"; import { PluginAddUseCase } from "../../../../src/application/use-cases/plugin/plugin-add-use-case.js"; import { PluginDistributionReaderAdapter } from "../../../../src/infrastructure/adapters/plugin-distribution-reader-adapter.js"; @@ -48,9 +48,16 @@ describe("PluginAddUseCase OpenCode hooks install (Phase 7)", () => { it("writes every hooks/ script but the manifest under .opencode/plugin/", async () => { const { deps } = await installSamplePlugin(); + // deps.fs is the in-memory adapter, whose listUnder() returns its own "/"-normalised + // keys regardless of host platform - a native `join` would answer with "\" on win32 + // and never match one of those keys. const writtenPaths = deps.fs.listUnder(PROJECT_ROOT); - expect(writtenPaths).toContain(join(PROJECT_ROOT, ".opencode", "plugin", "update_memory.js")); - expect(writtenPaths).not.toContain(join(PROJECT_ROOT, ".opencode", "plugin", "hooks.json")); + expect(writtenPaths).toContain( + posix.join(PROJECT_ROOT, ".opencode", "plugin", "update_memory.js") + ); + expect(writtenPaths).not.toContain( + posix.join(PROJECT_ROOT, ".opencode", "plugin", "hooks.json") + ); }); it("emits no logger.warn — hooks are delivered, not skipped", async () => { diff --git a/cli/tests/application/use-cases/plugin/translator/install-plugin-claude-mode-a.integration.test.ts b/cli/tests/application/use-cases/plugin/translator/install-plugin-claude-mode-a.integration.test.ts index 66cd2ca46..58bbd39b6 100644 --- a/cli/tests/application/use-cases/plugin/translator/install-plugin-claude-mode-a.integration.test.ts +++ b/cli/tests/application/use-cases/plugin/translator/install-plugin-claude-mode-a.integration.test.ts @@ -80,8 +80,20 @@ describe("install claude plugin via Mode A (integration)", () => { const settings = JSON.parse(await fs.readFile(settingsPath)) as Record; expect(settings.extraKnownMarketplaces).toBeDefined(); // Settings reference the BUILT claude tree, not the raw source. + // marketplace-sync-settings-use-case.ts's resolveSourceForSettings re-resolves every + // local source against projectRoot before writing it out, even one already absolute - + // harmless when that path already carries a drive letter, as a real builtDir always + // does, but a genuinely drive-less absolute local path (e.g. a marketplaces.json + // committed on POSIX and read on Windows) would silently gain the current drive + // instead of failing loud. Named, not fixed here - the expectation below goes through + // the same call so this test does not mask that behavior as something else. + // (#707: this is what surfaced it - the fake ensureBuilt stand-in these tests share + // returns exactly that drive-less shape.) expect((settings.extraKnownMarketplaces as Record)[MARKETPLACE_NAME]).toEqual({ - source: { source: "directory", path: "/built/claude" }, + source: { + source: "directory", + path: resolve(PROJECT_ROOT, "/built/claude").replace(/\\/g, "/"), + }, }); expect(settings.enabledPlugins).toBeDefined(); expect( diff --git a/cli/tests/application/use-cases/plugin/translator/install-plugin-copilot-mode-a.integration.test.ts b/cli/tests/application/use-cases/plugin/translator/install-plugin-copilot-mode-a.integration.test.ts index 6cd2b900a..66572a2dc 100644 --- a/cli/tests/application/use-cases/plugin/translator/install-plugin-copilot-mode-a.integration.test.ts +++ b/cli/tests/application/use-cases/plugin/translator/install-plugin-copilot-mode-a.integration.test.ts @@ -87,8 +87,20 @@ describe("install copilot plugin via Mode A (integration)", () => { const settings = JSON.parse(await fs.readFile(settingsPath)) as Record; expect(settings.extraKnownMarketplaces).toBeDefined(); // Settings reference the BUILT copilot tree, not the raw github source. + // marketplace-sync-settings-use-case.ts's resolveSourceForSettings re-resolves every + // local source against projectRoot before writing it out, even one already absolute - + // harmless when that path already carries a drive letter, as a real builtDir always + // does, but a genuinely drive-less absolute local path (e.g. a marketplaces.json + // committed on POSIX and read on Windows) would silently gain the current drive + // instead of failing loud. Named, not fixed here - the expectation below goes through + // the same call so this test does not mask that behavior as something else. + // (#707: this is what surfaced it - the fake ensureBuilt stand-in these tests share + // returns exactly that drive-less shape.) expect((settings.extraKnownMarketplaces as Record)[MARKETPLACE_NAME]).toEqual({ - source: { source: "directory", path: "/built/copilot" }, + source: { + source: "directory", + path: resolve(PROJECT_ROOT, "/built/copilot").replace(/\\/g, "/"), + }, }); expect(settings.enabledPlugins).toBeDefined(); }); diff --git a/cli/tests/application/use-cases/plugin/translator/install-plugin-cursor-mode-b.integration.test.ts b/cli/tests/application/use-cases/plugin/translator/install-plugin-cursor-mode-b.integration.test.ts index d9496f3a1..14d656828 100644 --- a/cli/tests/application/use-cases/plugin/translator/install-plugin-cursor-mode-b.integration.test.ts +++ b/cli/tests/application/use-cases/plugin/translator/install-plugin-cursor-mode-b.integration.test.ts @@ -1,5 +1,5 @@ import "../../../../../src/domain/tools/ai/cursor.js"; -import { join } from "node:path"; +import { join, posix } from "node:path"; import { describe, expect, it } from "vitest"; import { ModeBFlatMaterializationTranslator } from "../../../../../src/application/use-cases/plugin/translator/mode-b-flat-materialization-translator.js"; import { Manifest } from "../../../../../src/domain/models/manifest.js"; @@ -48,7 +48,10 @@ describe("install cursor plugin via Mode B (integration)", () => { "docs" ); - const expectedBase = join(STUB_HOME, ".cursor", "plugins", "local"); + // fs.listAll() returns the adapter's own "/"-normalised keys (in-memory-file-adapter.ts's + // `norm`) - a raw `.startsWith` needs the same convention, unlike `fs.has()` below which + // normalises internally. + const expectedBase = posix.join(STUB_HOME, ".cursor", "plugins", "local"); const writtenPaths = fs.listAll(); expect(writtenPaths.some((p) => p.startsWith(expectedBase))).toBe(true); }); diff --git a/cli/tests/application/use-cases/shared/ensure-built-marketplace-use-case.integration.test.ts b/cli/tests/application/use-cases/shared/ensure-built-marketplace-use-case.integration.test.ts index 08bd397dd..7c96e692b 100644 --- a/cli/tests/application/use-cases/shared/ensure-built-marketplace-use-case.integration.test.ts +++ b/cli/tests/application/use-cases/shared/ensure-built-marketplace-use-case.integration.test.ts @@ -1,5 +1,5 @@ import { tmpdir } from "node:os"; -import { join, resolve } from "node:path"; +import { join, resolve, sep } from "node:path"; import { beforeEach, describe, expect, it } from "vitest"; import { FrameworkBuildUseCase } from "../../../../src/application/use-cases/framework/framework-build-use-case.js"; import { FlatBuildStrategy } from "../../../../src/application/use-cases/framework/strategies/flat-build-strategy.js"; @@ -50,7 +50,11 @@ function stubAssetProvider(): AssetProvider { function makeIsDirectory(memFs: InMemoryFileAdapter): (path: string) => Promise { return async (path: string): Promise => { if (memFs.has(path)) return false; - const prefix = path.endsWith("/") ? path : `${path}/`; + // memFs stores every key "/"-normalised (in-memory-file-adapter.ts's own `norm`) - a + // native, backslash-separated `path` on Windows would never prefix-match one of those + // keys without the same normalisation here. + const normalized = path.replaceAll("\\", "/"); + const prefix = normalized.endsWith("/") ? normalized : `${normalized}/`; return memFs.listAll().some((k) => k.startsWith(prefix)); }; } @@ -80,7 +84,9 @@ function fakeVersion(value: string): VersionReader { describe("builtMarketplaceDir", () => { it("places the per-target tree under .aidd/cache/built//", () => { - expect(builtMarketplaceDir("/p", "aidd", "codex")).toBe("/p/.aidd/cache/built/aidd/codex"); + expect(builtMarketplaceDir("/p", "aidd", "codex")).toBe( + join("/p", BUILT_CACHE_SUBDIR, "aidd", "codex") + ); }); }); @@ -121,7 +127,7 @@ describe("EnsureBuiltMarketplaceUseCase", () => { }); it("does not rebuild when the sentinel matches (cliVer:catalogVer)", async () => { - const builtDir = builtMarketplaceDir(PROJECT, "aidd-framework", "codex"); + const builtDir = resolve(builtMarketplaceDir(PROJECT, "aidd-framework", "codex")); fs.setFile(join(builtDir, ".build-version"), "5.0.0:1.0.0"); const uc = new EnsureBuiltMarketplaceUseCase( fs, @@ -140,7 +146,7 @@ describe("EnsureBuiltMarketplaceUseCase", () => { }); it("rebuilds when the CLI version changed even if catalog version is the same", async () => { - const builtDir = builtMarketplaceDir(PROJECT, "aidd-framework", "codex"); + const builtDir = resolve(builtMarketplaceDir(PROJECT, "aidd-framework", "codex")); fs.setFile(join(builtDir, ".build-version"), "4.0.0:1.0.0"); const uc = new EnsureBuiltMarketplaceUseCase( fs, @@ -159,7 +165,7 @@ describe("EnsureBuiltMarketplaceUseCase", () => { }); it("always rebuilds when catalog version is undefined", async () => { - const builtDir = builtMarketplaceDir(PROJECT, "aidd-framework", "codex"); + const builtDir = resolve(builtMarketplaceDir(PROJECT, "aidd-framework", "codex")); fs.setFile(join(builtDir, ".build-version"), "5.0.0:unversioned"); const uc = new EnsureBuiltMarketplaceUseCase( fs, @@ -191,7 +197,7 @@ describe("EnsureBuiltMarketplaceUseCase", () => { target: "codex", mode: "marketplace", }); - expect(r.builtDir).toBe(builtMarketplaceDir(PROJECT, "aidd-framework", "codex")); + expect(r.builtDir).toBe(resolve(builtMarketplaceDir(PROJECT, "aidd-framework", "codex"))); expect(fs.getFile(join(r.builtDir, "plugins/aidd-vcs/SKILL.md"))).toBe("built content"); // temp dir cleaned up expect(fs.listUnder(tmpdir()).length).toBe(0); @@ -225,7 +231,11 @@ describe("force behavior at the cache-rebuild path", () => { const memFs = new InMemoryFileAdapter(); await seedFromDirectory(memFs, FIXTURE_DIR, { useAbsolutePaths: true }); - const builtDir = builtMarketplaceDir(PROJECT, "aidd-framework", "copilot"); + // EnsureBuiltMarketplaceUseCase.execute() resolves builtDir before handing it to + // buildFor(), so FlatBuildStrategy's write target (absOut) is always drive-qualified on + // Windows - mirror that resolve() here so this test's drive-less PROJECT seeds the same + // path the real write lands on. + const builtDir = resolve(builtMarketplaceDir(PROJECT, "aidd-framework", "copilot")); const agentPath = `${builtDir}/.github/agents/${PLUGIN}-code-reviewer.agent.md`; memFs.setFile(agentPath, "stale cache content from a previous build"); @@ -321,14 +331,18 @@ describe("outDir invariant for the cache-rebuild build path", () => { }); expect(capturedOutDirs).toHaveLength(2); - const cacheRoot = join(PROJECT, BUILT_CACHE_SUBDIR); + // resolve(): execute() always resolves builtDir before this capture sees it (#707). + const cacheRoot = resolve(join(PROJECT, BUILT_CACHE_SUBDIR)); const tmpRoot = tmpdir(); for (const outDir of capturedOutDirs) { - const underCache = outDir === cacheRoot || outDir.startsWith(`${cacheRoot}/`); - const underTmp = outDir === tmpRoot || outDir.startsWith(`${tmpRoot}/`); + const underCache = outDir === cacheRoot || outDir.startsWith(`${cacheRoot}${sep}`); + const underTmp = outDir === tmpRoot || outDir.startsWith(`${tmpRoot}${sep}`); expect(underCache || underTmp).toBe(true); } - // The dogfood call specifically must have gone through the temp dir, not the cache. - expect(capturedOutDirs[1]?.startsWith(`${tmpRoot}/`)).toBe(true); + // The dogfood call specifically must have gone through the temp dir, not the cache + // (#707): nested() now compares "/"-normalized paths, and both sourceDir and builtDir + // are resolve()'d before it sees them, so a drive-less-vs-drive-qualified or + // "\"-vs-"/" mismatch can no longer hide real nesting on Windows. + expect(capturedOutDirs[1]?.startsWith(`${tmpRoot}${sep}`)).toBe(true); }); }); diff --git a/cli/tests/golden/framework-build-golden.e2e.test.ts b/cli/tests/golden/framework-build-golden.e2e.test.ts index 50e72f4fe..424d665e9 100644 --- a/cli/tests/golden/framework-build-golden.e2e.test.ts +++ b/cli/tests/golden/framework-build-golden.e2e.test.ts @@ -52,6 +52,17 @@ const FLAT_TARGETS = ["claude", "cursor", "copilot", "codex", "opencode"] as con */ const FROZEN_CELLS = new Set(["claude"]); +// This repo carries no .gitattributes, so a Windows checkout's core.autocrlf converts +// every text file's LF to CRLF on write to disk (#707) - hashing those raw bytes would +// diff on line endings alone against the LF-committed stored baseline. Fold CRLF -> LF +// before hashing; skip anything that doesn't round-trip through UTF-8 (this tree's +// outputs are all .md/.json/.yml/.js today) so a future binary asset isn't corrupted. +function normalizeLineEndings(content: Buffer): Buffer { + const text = content.toString("utf-8"); + if (Buffer.byteLength(text, "utf-8") !== content.length) return content; + return Buffer.from(text.replace(/\r\n/g, "\n"), "utf-8"); +} + async function hashDirectory(dir: string): Promise { const result: TargetSnapshot = {}; const entries = await readdir(dir, { recursive: true }); @@ -59,7 +70,8 @@ async function hashDirectory(dir: string): Promise { const fullPath = join(dir, entry); try { const content = await readFile(fullPath); - result[entry.replace(/\\/g, "/")] = createHash("sha256").update(content).digest("hex"); + const normalized = normalizeLineEndings(content); + result[entry.replace(/\\/g, "/")] = createHash("sha256").update(normalized).digest("hex"); } catch { // skip directories } @@ -113,6 +125,10 @@ async function captureAllCells( } describe.concurrent("Framework build golden — 9-cell matrix", () => { + // Two full 9-cell builds, concurrently with the other tests in this file - on a real + // windows-latest runner this measured at 60039ms and 60096ms, just over the 60s default, + // not a hang (#707 windows-probe, attempt 3, run 32596840364). Raised per-test rather + // than the e2e project's global testTimeout so every other e2e file's budget is unchanged. it("snapshot is deterministic (two captures of each target are byte-identical)", async () => { const env1 = await createTestEnv("fb-golden-det-1"); const env2 = await createTestEnv("fb-golden-det-2"); @@ -155,8 +171,10 @@ describe.concurrent("Framework build golden — 9-cell matrix", () => { await env1.cleanup(); await env2.cleanup(); } - }); + }, 120_000); + // Same reason as above: a full 9-cell build, concurrently with the other tests in this + // file, measured at 60096ms on a real windows-latest runner - just over the 60s default. it("stored golden baseline covers all 9 cells and the frozen claude cell is byte-identical (AC #1)", async () => { const { tempDir, projectDir, fakeHome, cleanup } = await createTestEnv("fb-golden-baseline"); try { @@ -190,7 +208,7 @@ describe.concurrent("Framework build golden — 9-cell matrix", () => { } finally { await cleanup(); } - }); + }, 120_000); it("all 9 cells are non-empty", async () => { const stored = JSON.parse(await readFile(SNAPSHOT_FILE, "utf-8")) as GoldenSnapshot; diff --git a/cli/tests/golden/golden-baseline.e2e.test.ts b/cli/tests/golden/golden-baseline.e2e.test.ts index 5a79c60d1..b25597603 100644 --- a/cli/tests/golden/golden-baseline.e2e.test.ts +++ b/cli/tests/golden/golden-baseline.e2e.test.ts @@ -46,6 +46,15 @@ interface CommandEntry { function normalize(text: string): string { return ( text + // A Windows capture spells its separator "\" and carries a drive letter neither + // placeholder rule expects - fold both into the same drive-less, "/"-only shape a + // POSIX capture already has, so one set of rules covers a run from either platform. + // The lookbehind keeps a URL's scheme colon ("https:") alone - only a colon not + // preceded by a letter is a drive. Only ever called on a raw string (see + // normalizeManifest below for why the manifest is walked before, not after, + // JSON.stringify) so there is no JSON escaping here to corrupt. + .replace(/(?") @@ -58,6 +67,25 @@ function normalize(text: string): string { ); } +// Walks the parsed manifest and normalizes each string value directly, rather than +// normalizing after JSON.stringify - a Windows path's "\" is a single character in a real +// string but two once JSON-escapes it, and a blanket text replace big enough to fold both +// forms would just as readily mangle every other escape (`\"`, `\n`) the same encoding +// produces, corrupting content `JSON.parse` would then fail on rather than merely miscompare. +function normalizeManifest(value: unknown): unknown { + if (typeof value === "string") return normalize(value); + if (Array.isArray(value)) return value.map(normalizeManifest); + if (value !== null && typeof value === "object") { + return Object.fromEntries( + Object.entries(value as Record).map(([key, v]) => [ + key, + normalizeManifest(v), + ]) + ); + } + return value; +} + function normalizeEntry(entry: CommandEntry): CommandEntry { return { command: normalize(entry.command), @@ -65,8 +93,7 @@ function normalizeEntry(entry: CommandEntry): CommandEntry { stdout: normalize(entry.stdout), stderr: normalize(entry.stderr), filesWritten: entry.filesWritten.map(normalize).sort(), - manifest: - entry.manifest === null ? null : JSON.parse(normalize(JSON.stringify(entry.manifest))), + manifest: entry.manifest === null ? null : normalizeManifest(entry.manifest), }; } @@ -127,13 +154,25 @@ async function recomputeFileHashes(files: unknown, projectDir: string): Promise< const e = entry as Record; if (typeof e.relativePath !== "string") return entry; const content = await readFile(join(projectDir, e.relativePath), "utf-8").catch(() => ""); - const normalizedContent = normalize(content); + const normalizedContent = normalizeFileContent(content, e.relativePath); const hash = createHash("md5").update(normalizedContent, "utf-8").digest("hex"); return { ...e, hash }; }) ); } +// A .json file's raw bytes escape a real path separator as two literal backslash +// characters, never one - un-escape that pairing before the shared normalize() below, +// which expects a genuine single "\" the same way a parsed string (or raw stdout/stderr) +// already has it. Any other file's bytes are not JSON-encoded, so a literal "\" in them +// already is one; normalize() alone is correct as-is. This only ever un-escapes a real +// path separator correctly if the .json file's own string values carry no other escape +// (`\"`, `\n`, ...) - true of every settings.json this matrix writes today (path, repo, +// and plugin-name values only, per marketplace-entry.ts), not a general JSON un-escaper. +function normalizeFileContent(content: string, relativePath: string): string { + return normalize(relativePath.endsWith(".json") ? content.replace(/\\\\/g, "\\") : content); +} + /** Run a command and return a single CommandEntry (raw, not normalized). */ async function captureCommand( args: string[], diff --git a/cli/tests/infrastructure/auth/auth-storage.integration.test.ts b/cli/tests/infrastructure/auth/auth-storage.integration.test.ts index 29f6674de..e0e163b6d 100644 --- a/cli/tests/infrastructure/auth/auth-storage.integration.test.ts +++ b/cli/tests/infrastructure/auth/auth-storage.integration.test.ts @@ -89,7 +89,7 @@ describe("AuthStorage", () => { describe("paths", () => { it("projectConfigPath returns .aidd/auth.json under projectRoot", () => { const path = storage.projectConfigPath("/my/project"); - expect(path).toBe("/my/project/.aidd/auth.json"); + expect(path).toBe(join("/my/project", ".aidd", "auth.json")); }); it("userConfigPath respects AIDD_USER_CONFIG_DIR env override", () => { @@ -97,7 +97,7 @@ describe("AuthStorage", () => { try { process.env.AIDD_USER_CONFIG_DIR = "/custom/config/dir"; const path = storage.userConfigPath(); - expect(path).toBe("/custom/config/dir/auth.json"); + expect(path).toBe(join("/custom/config/dir", "auth.json")); } finally { if (original === undefined) { delete process.env.AIDD_USER_CONFIG_DIR; diff --git a/plugins/aidd-telemetry/hooks/lib/repo.js b/plugins/aidd-telemetry/hooks/lib/repo.js index a8e9f1c4e..6be4d9ca0 100644 --- a/plugins/aidd-telemetry/hooks/lib/repo.js +++ b/plugins/aidd-telemetry/hooks/lib/repo.js @@ -117,11 +117,13 @@ function runsDir(repoRoot) { const PRIVATE_DIR_MODE = 0o700; // `mkdirSync`'s `mode` applies only to a directory it creates, so a checked-out -// `aidd_docs/runs/` needs this chmod. Never applied to a user-named AIDD_RUNS_DIR - a -// user who names their own runs directory keeps responsibility for its permissions. +// `aidd_docs/runs/` needs this chmod - and, on Windows, needs it reset again on every +// write, since anything (a checkout, an admin, another tool) could have widened it since +// the last one. Never applied to a user-named AIDD_RUNS_DIR - a user who names their own +// runs directory keeps responsibility for its permissions. function tightenOwnedDir(dir) { if (process.env.AIDD_RUNS_DIR) return; - if (process.platform === "win32") return restrictToCurrentUser(dir, { recursive: true }); + if (process.platform === "win32") return restrictToCurrentUser(dir, { inheritable: true }); try { fs.chmodSync(dir, PRIVATE_DIR_MODE); } catch { @@ -130,33 +132,35 @@ function tightenOwnedDir(dir) { } // POSIX needs no second pass: `appendFileSync`'s own `mode` already set 0600 at the -// moment it created the file. On Windows, measured on a real windows-latest runner -// (#707), `tightenOwnedDir`'s `/T` recursion does not reliably carry the current-user -// grant onto a leaf file it walks into - the file came back with zero ACEs of its own, -// readable only because the runner's account happened to hold enough privilege to -// override that. This restricts the file directly instead of trusting `/T` to reach it. +// moment it created the file. On Windows this is the direct, non-recursive reset every +// file this code writes gets on its own. function tightenOwnedFile(filePath) { if (process.env.AIDD_RUNS_DIR) return; - if (process.platform === "win32") restrictToCurrentUser(filePath, { recursive: false }); + if (process.platform === "win32") restrictToCurrentUser(filePath); } // The real mechanism on Windows: reset the target's NTFS ACL to inherit nothing and grant -// Full Control to the current user alone. `recursive` adds `/T` plus the container-inherit -// flags `(OI)(CI)` so a directory's own future children pick up the same grant; a file -// gets neither, since a file has no children to inherit anything. `icacls` shelled out to -// the same way `git` already is above; `/C` keeps it going past one bad entry instead of +// Full Control to the current user alone. `inheritable` adds the container-inherit flags +// `(OI)(CI)` so a directory's own future children pick up the same grant; a file gets +// neither, since a file has no children to inherit anything. Never `/T`: measured on a +// real windows-latest runner (#707), `/T` walked into files this code does not own - a +// checked-out `.gitkeep` among them - and left at least one with no usable ACE of its +// own, so an ordinary `git add -A` right after got "Permission denied" opening it. It +// bought nothing here anyway: a file this code creates gets its own tightenOwnedFile +// pass, and `(OI)(CI)` alone makes the directory's own grant apply to anything created +// in it afterward - so this now only ever touches the target's own ACL entry, never a +// file already sitting inside a directory it is applied to. `icacls` shelled out to the +// same way `git` already is above; `/C` keeps it going past one bad entry instead of // aborting the whole reset, and its own exit code is not trusted as proof of anything - // only a caller reading the ACL back can say whether it worked. -function restrictToCurrentUser(target, { recursive = false } = {}) { +function restrictToCurrentUser(target, { inheritable = false } = {}) { try { const owner = process.env.USERDOMAIN ? `${process.env.USERDOMAIN}\\${process.env.USERNAME}` : process.env.USERNAME || os.userInfo().username; if (!owner) return; - const grant = recursive ? `${owner}:(OI)(CI)F` : `${owner}:F`; - const args = [target, "/inheritance:r", "/grant:r", grant]; - if (recursive) args.push("/T"); - args.push("/C", "/Q"); + const grant = inheritable ? `${owner}:(OI)(CI)F` : `${owner}:F`; + const args = [target, "/inheritance:r", "/grant:r", grant, "/C", "/Q"]; spawnSync("icacls", args, { encoding: "utf8" }); } catch { // icacls missing, no resolvable owner, or a domain-policy refusal: leave it as it is. diff --git a/scripts/__tests__/telemetry-where-things-live.test.js b/scripts/__tests__/telemetry-where-things-live.test.js index 904191266..7db439ec2 100644 --- a/scripts/__tests__/telemetry-where-things-live.test.js +++ b/scripts/__tests__/telemetry-where-things-live.test.js @@ -22,13 +22,31 @@ function actualDefaultRootDir() { describe("the documented figures location matches what the sink actually writes", () => { const actual = actualDefaultRootDir(); - // Docs are prose, always written with "/" regardless of the platform that built - // `actual` - path.join on win32 would answer with "\", so the doc-text comparison - // below normalises before substituting the sentinel for "~". - const documented = actual.replace(/\\/gu, "/").replace("/sentinel-home", "~"); + const posixDefault = path.join("/sentinel-home", ".config", "aidd", "telemetry"); + // Docs are prose, always written with "/" and the POSIX default - true regardless of + // which platform runs this file, since it is the docs' own text being checked, not + // `actual` (which does diverge on win32, see the platform branch below). + const documented = posixDefault.replace(/\\/gu, "/").replace("/sentinel-home", "~"); - it("computes the well-known default - a change here means the docs must change too", () => { - assert.equal(actual, path.join("/sentinel-home", ".config", "aidd", "telemetry")); + it("computes the well-known default for this platform - a change here means the docs must change too", () => { + if (process.platform === "win32") { + // sink.js's own windowsRootDir: %APPDATA%\aidd\telemetry, unless it falls back to + // the POSIX-style path (no APPDATA at all - not expected on a real machine, but + // computeRootDir itself falls back rather than erroring, so this mirrors that). + const windowsDefault = process.env.APPDATA + ? path.join(process.env.APPDATA, "aidd", "telemetry") + : posixDefault; + assert.equal(actual, windowsDefault); + if (process.env.APPDATA) { + const readme = fs.readFileSync(PLUGIN_README, "utf8"); + assert.ok( + readme.includes("%APPDATA%\\aidd\\telemetry"), + 'expected plugins/aidd-telemetry/README.md to name Windows\' own default, "%APPDATA%\\aidd\\telemetry"' + ); + } + } else { + assert.equal(actual, posixDefault); + } }); for (const [label, docPath] of [