From 3bfd4a8b69917a50f634c64d46c6b2c81f88f98a Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 05:41:05 +0200 Subject: [PATCH 01/10] fix(framework): close the last Windows failures (#707) The journal's per-write ACL reset walked (`icacls /T`) into files it does not own - a checked-out `.gitkeep` among them - and left it with no usable ACE, so an ordinary `git add -A` right after got "Permission denied" opening it. Drops `/T`: a file this code writes already gets its own direct icacls pass, and `(OI)(CI)` alone makes the directory's own grant apply to anything created afterward, so nothing needed the recursion. A marker file also stops the directory reset from repeating on every session-start once it has already run. Closes the rest of the set the Windows measurement found: the doc/code-parity test for the figures location now checks the sink's real per-platform answer instead of a POSIX literal, folding the Windows pin into the same three subtests rather than a fourth; eleven CLI integration tests get the direction-correct fix each one actually needed - a native `join` used for a virtual, always-"/" relative path in one production translator (a real small defect, fixed), the same native-vs-virtual-path mismatch inverted across four test files, and three hardcoded POSIX literals; the golden baseline's own normalizer gains a drive-letter/backslash fold so it redacts a Windows path the same way it already redacts a POSIX one (a real gap, not a content difference - the settings.json hash mismatch traced to the same un-redacted path, not different bytes); the other golden file's two timeouts get a per-test bump, not the e2e project's global one. The windows-probe job also gets a step that echoes `icacls` for the runs directory and the written journal file, then runs `git add -A` against the real checkout - proof for both properties on the runner itself, not just inferred from a passing test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VWNxk63AGKkqE8HRqHLjGp --- .github/workflows/cli-ci.yml | 16 +++++- .../built-tree-materialization-translator.ts | 7 ++- ...opencode-hooks-install.integration.test.ts | 13 +++-- ...l-plugin-claude-mode-a.integration.test.ts | 9 +++- ...-plugin-copilot-mode-a.integration.test.ts | 9 +++- ...l-plugin-cursor-mode-b.integration.test.ts | 7 ++- ...t-marketplace-use-case.integration.test.ts | 18 ++++--- .../golden/framework-build-golden.e2e.test.ts | 10 +++- cli/tests/golden/golden-baseline.e2e.test.ts | 7 +++ .../auth/auth-storage.integration.test.ts | 4 +- plugins/aidd-telemetry/hooks/lib/repo.js | 51 +++++++++++++------ .../telemetry-where-things-live.test.js | 30 ++++++++--- 12 files changed, 140 insertions(+), 41 deletions(-) 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/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..b8b9bb71f 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,15 @@ 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. + // resolveSourceForSettings re-resolves the built dir against projectRoot before + // writing it out, so the expectation goes through the same call rather than a + // literal - on win32 a drive-less "/built/claude" resolves against the current + // drive, which a hand-typed literal would not reflect. 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..5f84adeaa 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,15 @@ 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. + // resolveSourceForSettings re-resolves the built dir against projectRoot before + // writing it out, so the expectation goes through the same call rather than a + // literal - on win32 a drive-less "/built/copilot" resolves against the current + // drive, which a hand-typed literal would not reflect. 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..f10ef5580 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") + ); }); }); @@ -324,11 +330,11 @@ describe("outDir invariant for the cache-rebuild build path", () => { const cacheRoot = 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); + 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..ec8c93f1e 100644 --- a/cli/tests/golden/framework-build-golden.e2e.test.ts +++ b/cli/tests/golden/framework-build-golden.e2e.test.ts @@ -113,6 +113,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 +159,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 +196,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..99a9e360a 100644 --- a/cli/tests/golden/golden-baseline.e2e.test.ts +++ b/cli/tests/golden/golden-baseline.e2e.test.ts @@ -46,6 +46,13 @@ interface CommandEntry { function normalize(text: string): string { return ( text + // A Windows capture spells its separator "\" (or "\\" once JSON-escaped inside the + // manifest text below) 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. + .replace(/(?") 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..3bf984a77 100644 --- a/plugins/aidd-telemetry/hooks/lib/repo.js +++ b/plugins/aidd-telemetry/hooks/lib/repo.js @@ -116,12 +116,17 @@ function runsDir(repoRoot) { // from restrictToCurrentUser below, not from this constant. const PRIVATE_DIR_MODE = 0o700; +// Set once this directory's own ACL has been reset, so a repository with many sessions +// does not reset it again on every single one (#707: it collided with git - see below). +// Covered by the `aidd_docs/runs/*` gitignore glob like every other record here. +const ACL_TIGHTENED_MARKER = ".aidd-acl-tightened"; + // `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. function tightenOwnedDir(dir) { if (process.env.AIDD_RUNS_DIR) return; - if (process.platform === "win32") return restrictToCurrentUser(dir, { recursive: true }); + if (process.platform === "win32") return tightenOwnedDirWindows(dir); try { fs.chmodSync(dir, PRIVATE_DIR_MODE); } catch { @@ -129,34 +134,50 @@ function tightenOwnedDir(dir) { } } +// Measured on a real windows-latest runner (#707): resetting this directory's ACL with +// `/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. `/T` bought nothing here anyway: a +// file this code creates gets its own tightenOwnedFile pass below, and `(OI)(CI)` alone +// makes the directory's own grant apply to anything created in it afterward - so this +// only ever touches the directory's own ACL, never a file already sitting inside it. +function tightenOwnedDirWindows(dir) { + const marker = path.join(dir, ACL_TIGHTENED_MARKER); + if (fs.existsSync(marker)) return; + restrictToCurrentUser(dir, { inheritable: true }); + try { + fs.writeFileSync(marker, ""); + } catch { + // Best-effort: a failed write here just means the next write resets it again. + } +} + // 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 - never inherited from the directory alone, +// since `tightenOwnedDirWindows` above deliberately never walks into existing files. 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 +// 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`: seeded +// exclusively to this one target's own ACL entry - see tightenOwnedDirWindows above for +// why walking into existing children is deliberately not done. `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 [ From db1310a3910c86552b9f9479d62c408f85f32cda Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 05:46:42 +0200 Subject: [PATCH 02/10] ci(scratch): trigger cli-ci.yml on push to this branch --- .github/workflows/cli-ci.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cli-ci.yml b/.github/workflows/cli-ci.yml index 4ea0d0671..3148f5ce9 100644 --- a/.github/workflows/cli-ci.yml +++ b/.github/workflows/cli-ci.yml @@ -9,7 +9,10 @@ name: cli CI on: push: - branches: [main, next] + # ci/windows-last: scratch-only addition, dropped before this file leaves that branch - + # the PR route hit a merge conflict against main that silently skipped pull_request + # triggering, so this pushes directly instead. + branches: [main, next, ci/windows-last] paths: - "cli/**" - "kanban/**" From b507d61459dbca83fe5661e20f62bbf5bed13d76 Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 05:59:15 +0200 Subject: [PATCH 03/10] Revert "ci(scratch): trigger cli-ci.yml on push to this branch" This reverts commit db1310a3910c86552b9f9479d62c408f85f32cda. --- .github/workflows/cli-ci.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/cli-ci.yml b/.github/workflows/cli-ci.yml index 3148f5ce9..4ea0d0671 100644 --- a/.github/workflows/cli-ci.yml +++ b/.github/workflows/cli-ci.yml @@ -9,10 +9,7 @@ name: cli CI on: push: - # ci/windows-last: scratch-only addition, dropped before this file leaves that branch - - # the PR route hit a merge conflict against main that silently skipped pull_request - # triggering, so this pushes directly instead. - branches: [main, next, ci/windows-last] + branches: [main, next] paths: - "cli/**" - "kanban/**" From 573290643f347aab4a7aa433cf65f950f9c14736 Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 05:59:29 +0200 Subject: [PATCH 04/10] fix(framework): drop the marker, name the resolve() defect (#707 review) The marker file from the first pass permanently disabled re-tightening the runs directory once written - exactly the "checked-out aidd_docs/ runs/ needs this chmod" case the reset exists for in the first place. Dropping /T alone already closes the git collision (it never touches a file already sitting in the directory), so the reset goes back to running on every write, unconditionally, matching what it did before this task beyond no longer recursing. Also names, rather than only routes around, the real defect the copilot/claude Mode A test fixes surfaced: resolveSourceForSettings re-resolves an already-absolute local source path against projectRoot unconditionally. Harmless for a real builtDir (always drive-qualified on a real Windows machine), 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 process's drive instead of failing loud. Not fixed here; named in both tests instead of left implied by a test-side literal-to-computed edit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VWNxk63AGKkqE8HRqHLjGp --- ...l-plugin-claude-mode-a.integration.test.ts | 13 +++-- ...-plugin-copilot-mode-a.integration.test.ts | 13 +++-- plugins/aidd-telemetry/hooks/lib/repo.js | 47 ++++++------------- 3 files changed, 33 insertions(+), 40 deletions(-) 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 b8b9bb71f..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,10 +80,15 @@ 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. - // resolveSourceForSettings re-resolves the built dir against projectRoot before - // writing it out, so the expectation goes through the same call rather than a - // literal - on win32 a drive-less "/built/claude" resolves against the current - // drive, which a hand-typed literal would not reflect. + // 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", 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 5f84adeaa..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,10 +87,15 @@ 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. - // resolveSourceForSettings re-resolves the built dir against projectRoot before - // writing it out, so the expectation goes through the same call rather than a - // literal - on win32 a drive-less "/built/copilot" resolves against the current - // drive, which a hand-typed literal would not reflect. + // 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", diff --git a/plugins/aidd-telemetry/hooks/lib/repo.js b/plugins/aidd-telemetry/hooks/lib/repo.js index 3bf984a77..6be4d9ca0 100644 --- a/plugins/aidd-telemetry/hooks/lib/repo.js +++ b/plugins/aidd-telemetry/hooks/lib/repo.js @@ -116,17 +116,14 @@ function runsDir(repoRoot) { // from restrictToCurrentUser below, not from this constant. const PRIVATE_DIR_MODE = 0o700; -// Set once this directory's own ACL has been reset, so a repository with many sessions -// does not reset it again on every single one (#707: it collided with git - see below). -// Covered by the `aidd_docs/runs/*` gitignore glob like every other record here. -const ACL_TIGHTENED_MARKER = ".aidd-acl-tightened"; - // `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 tightenOwnedDirWindows(dir); + if (process.platform === "win32") return restrictToCurrentUser(dir, { inheritable: true }); try { fs.chmodSync(dir, PRIVATE_DIR_MODE); } catch { @@ -134,28 +131,9 @@ function tightenOwnedDir(dir) { } } -// Measured on a real windows-latest runner (#707): resetting this directory's ACL with -// `/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. `/T` bought nothing here anyway: a -// file this code creates gets its own tightenOwnedFile pass below, and `(OI)(CI)` alone -// makes the directory's own grant apply to anything created in it afterward - so this -// only ever touches the directory's own ACL, never a file already sitting inside it. -function tightenOwnedDirWindows(dir) { - const marker = path.join(dir, ACL_TIGHTENED_MARKER); - if (fs.existsSync(marker)) return; - restrictToCurrentUser(dir, { inheritable: true }); - try { - fs.writeFileSync(marker, ""); - } catch { - // Best-effort: a failed write here just means the next write resets it again. - } -} - // POSIX needs no second pass: `appendFileSync`'s own `mode` already set 0600 at the // moment it created the file. On Windows this is the direct, non-recursive reset every -// file this code writes gets on its own - never inherited from the directory alone, -// since `tightenOwnedDirWindows` above deliberately never walks into existing files. +// file this code writes gets on its own. function tightenOwnedFile(filePath) { if (process.env.AIDD_RUNS_DIR) return; if (process.platform === "win32") restrictToCurrentUser(filePath); @@ -164,10 +142,15 @@ function tightenOwnedFile(filePath) { // The real mechanism on Windows: reset the target's NTFS ACL to inherit nothing and grant // 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`: seeded -// exclusively to this one target's own ACL entry - see tightenOwnedDirWindows above for -// why walking into existing children is deliberately not done. `icacls` shelled out to -// the same way `git` already is above; `/C` keeps it going past one bad entry instead of +// 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, { inheritable = false } = {}) { From 8c5ec60d20085936d3dde141db804d66120bdb39 Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 06:06:56 +0200 Subject: [PATCH 05/10] fix(framework): un-escape JSON's own doubled backslash, not every one (#707 review) normalize()'s blanket backslash fold ran over the manifest after JSON.stringify - a settings.json path's separator is doubled by JSON's own escaping there, but so is nothing else, and the same fold would have flattened `\"` and `\n` right along with it, corrupting content JSON.parse would then throw on rather than merely miscompare. Walks the parsed manifest and normalizes each string value directly instead (no JSON escaping to fight at that point), and gives the one place that still reads raw JSON bytes off disk - recomputing a real file's hash - its own narrow un-escape: a doubled backslash back to one, scoped to .json files only, and only correct because every settings.json this matrix writes carries path/repo/plugin-name values and nothing else that JSON would need to escape. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VWNxk63AGKkqE8HRqHLjGp --- cli/tests/golden/golden-baseline.e2e.test.ts | 50 ++++++++++++++++---- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/cli/tests/golden/golden-baseline.e2e.test.ts b/cli/tests/golden/golden-baseline.e2e.test.ts index 99a9e360a..b25597603 100644 --- a/cli/tests/golden/golden-baseline.e2e.test.ts +++ b/cli/tests/golden/golden-baseline.e2e.test.ts @@ -46,13 +46,15 @@ interface CommandEntry { function normalize(text: string): string { return ( text - // A Windows capture spells its separator "\" (or "\\" once JSON-escaped inside the - // manifest text below) 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. + // 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(/(?") @@ -65,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), @@ -72,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), }; } @@ -134,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[], From 95ee464d2644b2a5309aaff853746f3b6781a293 Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 06:13:26 +0200 Subject: [PATCH 06/10] fix(framework): mirror resolve() in the third ensure-built-marketplace test (#707) The remaining failure in this file was the same resolve() mismatch as the copilot/claude Mode A tests: FrameworkBuildUseCase.execute() calls resolve(options.outDir) before ever touching the filesystem through it, but this test's own colliding-file path was built from the un-resolved builtMarketplaceDir() output. Harmless for a real projectRoot; this test's drive-less "/proj" is what exposed it. One real, unfixed defect surfaced investigating the file's last failure and is now named at the assertion it breaks, not fixed: EnsureBuiltMarketplaceUseCase.nested() compares sourceDir/builtDir with a hardcoded "/" rather than path.sep, so on Windows it never recognises genuine nesting - the "dogfood" build (a marketplace whose source is the project being built) skips the temp-dir detour meant to keep it out of guardPaths()'s way. Compounded by resolve() only ever being applied to sourceDir (ensure-built-marketplace-use-case.ts:70), never to builtDir, so a separator fix alone would not close it - both sides need to compare normalized, equally-resolved paths. Out of this task's surgical scope; reported rather than patched blind. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VWNxk63AGKkqE8HRqHLjGp --- ...uilt-marketplace-use-case.integration.test.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) 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 f10ef5580..47163af1b 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 @@ -231,7 +231,12 @@ 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"); + // FrameworkBuildUseCase.execute() re-resolves the outDir it is given + // (framework-build-use-case.ts:33) before ever touching the filesystem through it - + // harmless for a real projectRoot (already absolute with a drive letter on Windows), + // but this test's own drive-less "/proj" only matches what the strategy actually + // checks once this seed goes through the same resolve(). + 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"); @@ -335,6 +340,15 @@ describe("outDir invariant for the cache-rebuild build path", () => { expect(underCache || underTmp).toBe(true); } // The dogfood call specifically must have gone through the temp dir, not the cache. + // Red on Windows (#707, not fixed here): ensure-built-marketplace-use-case.ts's own + // nested() compares sourceDir/builtDir with a hardcoded "/" (never path.sep), so on a + // real windows-latest runner it never recognises real nesting - the dogfood call + // routes straight into runBuild() instead of buildViaTemp(), landing this outDir under + // the cache, not tmp. In real use that same source/builtDir pair also differs by + // resolve()'s drive letter (sourceDir goes through it at + // ensure-built-marketplace-use-case.ts:70, builtDir never does), so fixing the + // separator alone would not be enough - both need to compare normalized, equally- + // resolved paths. expect(capturedOutDirs[1]?.startsWith(`${tmpRoot}${sep}`)).toBe(true); }); }); From e7fe9cb61970d186f0a0b16ad20fd3f5aa8851d2 Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 06:26:02 +0200 Subject: [PATCH 07/10] fix(framework): resolve builtDir before it reaches buildFor (#707) builtMarketplaceDir() joins with the native separator, so a drive-less projectRoot yields a drive-less builtDir on Windows. That raw value was captured unresolved into FlatBuildStrategy's write target (absOut) at construction, while FrameworkBuildUseCase.execute() separately resolved its own outDir copy for the preBuild existence check only - letting writes and checks silently diverge on Windows. Resolve builtDir at the same point sourceDir already is. --- .../shared/ensure-built-marketplace-use-case.ts | 11 +++++++---- ...ure-built-marketplace-use-case.integration.test.ts | 9 ++++----- 2 files changed, 11 insertions(+), 9 deletions(-) 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..977ee1763 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, 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 47163af1b..b99c74962 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 @@ -231,11 +231,10 @@ describe("force behavior at the cache-rebuild path", () => { const memFs = new InMemoryFileAdapter(); await seedFromDirectory(memFs, FIXTURE_DIR, { useAbsolutePaths: true }); - // FrameworkBuildUseCase.execute() re-resolves the outDir it is given - // (framework-build-use-case.ts:33) before ever touching the filesystem through it - - // harmless for a real projectRoot (already absolute with a drive letter on Windows), - // but this test's own drive-less "/proj" only matches what the strategy actually - // checks once this seed goes through the same resolve(). + // 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"); From d09e3330e31f23c0b5db14abaa7fc8aaf58defdf Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 06:35:55 +0200 Subject: [PATCH 08/10] fix(framework): normalize nested() and resolve seeded builtDir literals (#707) nested() compared sourceDir/builtDir with a hardcoded "/", never path.sep, so it silently missed real nesting on Windows - mirror guardPaths()'s own "/"-normalized comparison. Also update the four test-side builtDir literals left unresolved after the prior commit started resolving builtDir in production: they now seed/expect the same drive-qualified path the use-case actually reads and returns. --- .../ensure-built-marketplace-use-case.ts | 11 +++++++--- ...t-marketplace-use-case.integration.test.ts | 22 +++++++------------ 2 files changed, 16 insertions(+), 17 deletions(-) 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 977ee1763..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 @@ -115,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/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 b99c74962..2b597cf39 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 @@ -127,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, @@ -146,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, @@ -165,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, @@ -197,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); @@ -338,16 +338,10 @@ describe("outDir invariant for the cache-rebuild build path", () => { 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. - // Red on Windows (#707, not fixed here): ensure-built-marketplace-use-case.ts's own - // nested() compares sourceDir/builtDir with a hardcoded "/" (never path.sep), so on a - // real windows-latest runner it never recognises real nesting - the dogfood call - // routes straight into runBuild() instead of buildViaTemp(), landing this outDir under - // the cache, not tmp. In real use that same source/builtDir pair also differs by - // resolve()'s drive letter (sourceDir goes through it at - // ensure-built-marketplace-use-case.ts:70, builtDir never does), so fixing the - // separator alone would not be enough - both need to compare normalized, equally- - // resolved paths. + // 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); }); }); From aacefc1414f44f4ef3d8e07aff97bb4746ec2638 Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 06:44:15 +0200 Subject: [PATCH 09/10] fix(framework): resolve the outDir-invariant test's cacheRoot literal (#707) Same gap as the prior two commits: execute() now always resolves builtDir before capturedOutDirs sees it, but this test's cacheRoot comparison literal stayed unresolved. --- .../ensure-built-marketplace-use-case.integration.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 2b597cf39..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 @@ -331,7 +331,8 @@ 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}${sep}`); From 716661c856771fc7f1fa8fd46bc9afca68ff311a Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 23 Aug 2026 06:55:07 +0200 Subject: [PATCH 10/10] fix(framework): fold CRLF to LF before hashing the golden build tree (#707) This repo has no .gitattributes, so a Windows checkout's core.autocrlf rewrites every text file's line endings to CRLF - hashDirectory() was hashing those raw bytes, diffing the frozen claude cell against the LF-committed stored baseline on line endings alone, not real content. Corrects the prior diagnosis: AC #1 was not only a timeout (fixed by the earlier 120s bump) but a second, separate content-hash mismatch the timeout fix then exposed. --- .../golden/framework-build-golden.e2e.test.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/cli/tests/golden/framework-build-golden.e2e.test.ts b/cli/tests/golden/framework-build-golden.e2e.test.ts index ec8c93f1e..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 }