Merge remote-tracking branch 'origin/next' into fix/1327-setuppath-idempotency

Co-authored-by: ops-ci-01 <[email protected]>

.woodpecker/ci.yml

@@ -21,11 +21,19 @@ variables:
   - &enable_pnpm 'corepack enable'
 
 when:
-  # PR + manual CI run on any branch — the pull_request pipeline is the merge gate.
+  # PR + manual CI run on any branch: the pull_request pipeline is the merge
-  # push CI is restricted to protected branches (main) so a feature-branch push no
+  # gate (next is protected and the default branch since 2026-08-19).
-  # longer fires a redundant SECOND pipeline alongside its PR pipeline. This ~halves
+  # Push CI runs on main only. next deliberately runs NO push ci: post-merge
-  # CI load on the storage-constrained runner with zero loss of gating (branch
+  # verification on next is carried by publish.yml's `verify` step
-  # protection requires no push/ci status context; main still gets full push CI).
+  # (pnpm verify:release), which mirrors this pipeline's complete mandatory
+  # set step-for-step, enforced by scripts/verify-release.test.mjs. PR CI
+  # tests the PR HEAD tree (refs/pull/N/head, measured 2026-08-19), not a
+  # merge ref, so if next advances before a merge the landed tree differs
+  # from the tested one; publish verify re-runs the full set on the landed
+  # tree (PGlite path). Measured 2026-08-19: the 21 most recent push events
+  # on next each ran exactly one pipeline (publish), zero ci.
+  # Keeping push ci off next also avoids a redundant second full-suite run
+  # per merge on the storage-constrained runner.
   - event: [pull_request, manual]
   - event: push
     branch: main

packages/mosaic/src/stages/finalize-path.spec.ts

@@ -0,0 +1,152 @@
+/**
+ * setupPath profile management (issue #1327, MOSAIC-IMPROVEMENTS 4c / D25).
+ *
+ * The profile append used to be guarded on the binDir value it was about to
+ * write, which is blind to accumulation across different Mosaic homes: every
+ * wizard run against a fresh temp home appended a permanent block to the
+ * operator's real shell profile (1,061 measured appends on sb-it-1-dt).
+ *
+ * Arms below map to the requirements:
+ *   S1 sentinel-managed block, rewritten in place
+ *   S2 a non-default target home never touches the operator profile
+ *   S3 byte-identical profile across repeated runs
+ *   S4 legacy unmarked `# Mosaic` blocks collapse into the managed block
+ *   S5 the Windows ($env:Path) arm shares the same block logic
+ */
+
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs';
+import { join } from 'node:path';
+import { tmpdir, homedir } from 'node:os';
+
+let profilePathMock: string | null = null;
+
+vi.mock('../platform/detect.js', () => ({
+  getShellProfilePath: (): string | null => profilePathMock,
+}));
+
+import { setupPath, managedBlockFor, stripLegacyPathBlocks } from './finalize.js';
+
+// The real resolved default on this host. Tests use it as the comparator a
+// non-default home must fail against, exactly as the wizard would.
+const REAL_DEFAULT_HOME = join(homedir(), '.config', 'mosaic');
+
+function tempHome(prefix: string): string {
+  const dir = join(tmpdir(), prefix);
+  mkdirSync(join(dir, 'bin'), { recursive: true });
+  return dir;
+}
+
+describe('setupPath profile management (#1327)', () => {
+  let workDir: string;
+  let profileFile: string;
+  let defaultLikeHome: string;
+  let otherHome: string;
+  const baseline = '# existing operator content\nexport EDITOR=vim\n';
+
+  beforeEach(() => {
+    workDir = mkdtempSync(join(tmpdir(), 'setuppath-spec-'));
+    profileFile = join(workDir, '.bashrc');
+    writeFileSync(profileFile, baseline, 'utf-8');
+    profilePathMock = profileFile;
+    defaultLikeHome = tempHome(join(workDir, 'home-a', '.config', 'mosaic'));
+    otherHome = tempHome(join(workDir, 'home-b', '.config', 'mosaic'));
+  });
+
+  afterEach(() => {
+    profilePathMock = null;
+    rmSync(workDir, { recursive: true, force: true });
+  });
+
+  // S2 — the arm that MUST fail against the pre-fix code: a home that is not
+  // the resolved default may not modify the operator profile at all.
+  it('does not touch the operator profile when the target home is not the resolved default', () => {
+    const action = setupPath(otherHome, REAL_DEFAULT_HOME);
+    expect(action).toBe('skipped');
+    expect(readFileSync(profileFile, 'utf-8')).toBe(baseline);
+  });
+
+  it('returns skipped when no shell profile can be resolved', () => {
+    profilePathMock = null;
+    const action = setupPath(defaultLikeHome, defaultLikeHome);
+    expect(action).toBe('skipped');
+  });
+
+  // S1 + S3 — two distinct homes (each run as the resolved default in turn,
+  // the shape of two legitimate installs against one operator profile) and
+  // repeated runs against the same home both leave exactly one block.
+  it('leaves exactly one managed block after runs against two distinct homes', () => {
+    const first = setupPath(defaultLikeHome, defaultLikeHome);
+    expect(first).toBe('added');
+
+    const second = setupPath(otherHome, otherHome);
+    expect(second).toBe('added');
+
+    const content = readFileSync(profileFile, 'utf-8');
+    const beginCount = content.split('# >>> mosaic begin >>>').length - 1;
+    const endCount = content.split('# <<< mosaic end <<<').length - 1;
+    expect(beginCount).toBe(1);
+    expect(endCount).toBe(1);
+    expect(content).toContain(join(otherHome, 'bin'));
+    expect(content).toContain(baseline);
+  });
+
+  it('is byte-identical across repeated runs against the same home', () => {
+    setupPath(defaultLikeHome, defaultLikeHome);
+    const afterFirst = readFileSync(profileFile, 'utf-8');
+
+    const again = setupPath(defaultLikeHome, defaultLikeHome);
+    expect(again).toBe('already');
+    expect(readFileSync(profileFile, 'utf-8')).toBe(afterFirst);
+  });
+
+  // S4 — pre-existing unmarked blocks from the old append logic collapse
+  // into the single managed block instead of accumulating beside it.
+  it('collapses legacy unmarked # Mosaic blocks into the managed block', () => {
+    const legacy =
+      '# existing operator content\n' +
+      '# Mosaic\n' +
+      'export PATH="/tmp/mosaic-dead-wizard-1/bin:$PATH"\n' +
+      'export EDITOR=vim\n' +
+      '# Mosaic\n' +
+      'export PATH="/tmp/mosaic-dead-wizard-2/bin:$PATH"\n';
+    writeFileSync(profileFile, legacy, 'utf-8');
+
+    const action = setupPath(defaultLikeHome, defaultLikeHome);
+    expect(action).toBe('added');
+
+    const content = readFileSync(profileFile, 'utf-8');
+    expect(content).not.toContain('/tmp/mosaic-dead-wizard-1/bin');
+    expect(content).not.toContain('/tmp/mosaic-dead-wizard-2/bin');
+    expect(content).toContain('export EDITOR=vim');
+    expect(content.split('# >>> mosaic begin >>>').length - 1).toBe(1);
+    expect(content).toContain(join(defaultLikeHome, 'bin'));
+  });
+});
+
+describe('managed block helpers (#1327)', () => {
+  // S5 — the Windows arm shares markers and shape with the POSIX arm.
+  it('builds the $env:Path variant inside the same markers', () => {
+    const block = managedBlockFor('C:\\Users\\op\\.config\\mosaic\\bin', true);
+    expect(block).toContain('# >>> mosaic begin >>>');
+    expect(block).toContain('# <<< mosaic end <<<');
+    expect(block).toContain('$env:Path = "C:\\Users\\op\\.config\\mosaic\\bin;$env:Path"');
+  });
+
+  it('builds the POSIX export variant inside the same markers', () => {
+    const block = managedBlockFor('/home/op/.config/mosaic/bin', false);
+    expect(block).toContain('# >>> mosaic begin >>>');
+    expect(block).toContain('export PATH="/home/op/.config/mosaic/bin:$PATH"');
+    expect(block).toContain('# <<< mosaic end <<<');
+  });
+
+  it('strips legacy $env:Path pairs on the Windows arm', () => {
+    const legacy =
+      '# Mosaic\n$env:Path = "C:\\tmp\\dead\\bin;$env:Path"\n' +
+      '# Mosaic\n$env:Path = "C:\\tmp\\dead2\\bin;$env:Path"\n' +
+      'Write-Host hi\n';
+    const stripped = stripLegacyPathBlocks(legacy, true);
+    expect(stripped).not.toContain('C:\\tmp\\dead');
+    expect(stripped).toContain('Write-Host hi');
+  });
+});

packages/mosaic/src/stages/finalize.ts

@@ -1,11 +1,12 @@
 import { spawnSync } from 'node:child_process';
-import { existsSync, readFileSync, appendFileSync } from 'node:fs';
+import { existsSync, readFileSync, writeFileSync } from 'node:fs';
 import { join } from 'node:path';
 import { platform } from 'node:os';
 import type { WizardPrompter } from '../prompter/interface.js';
 import type { ConfigService } from '../config/config-service.js';
 import type { WizardState } from '../types.js';
 import { getShellProfilePath } from '../platform/detect.js';
+import { DEFAULT_MOSAIC_HOME } from '../constants.js';
 import { ManifestError } from '../framework/manifest.js';
 import {
   getDefaultSkillPaths,
@@ -144,32 +145,87 @@ function runDoctor(mosaicHome: string): DoctorResult {
 
 type PathAction = 'already' | 'added' | 'skipped';
 
-function setupPath(mosaicHome: string, _p: WizardPrompter): PathAction {
+const PATH_BLOCK_BEGIN = '# >>> mosaic begin >>>';
-  const binDir = join(mosaicHome, 'bin');
+const PATH_BLOCK_END = '# <<< mosaic end <<<';
-  const currentPath = process.env['PATH'] ?? '';
+const PATH_BLOCK_NOTE = '# Managed by the Mosaic installer; this block is rewritten on install.';
 
-  if (currentPath.includes(binDir)) {
+/**
-    return 'already';
+ * The managed PATH block written into the operator's shell profile.
+ *
+ * The block is delimited by begin/end sentinels so any number of installs,
+ * against any homes, collapse to exactly one block: the writer replaces the
+ * region between the sentinels instead of appending a second copy (#1327).
+ */
+export function managedBlockFor(binDir: string, isWindows: boolean): string {
+  const exportLine = isWindows
+    ? `$env:Path = "${binDir};$env:Path"`
+    : `export PATH="${binDir}:$PATH"`;
+  return `${PATH_BLOCK_BEGIN}\n${PATH_BLOCK_NOTE}\n${exportLine}\n${PATH_BLOCK_END}\n`;
+}
+
+/**
+ * Remove legacy unmarked `# Mosaic` PATH pairs appended by pre-#1327
+ * installs. Only the exact two-line shape this installer used to write is
+ * removed; any other `# Mosaic` comment line is left alone.
+ */
+export function stripLegacyPathBlocks(content: string, isWindows: boolean): string {
+  const legacyExport = isWindows ? /^\$env:Path = ".*;\$env:Path"$/ : /^export PATH=".*:\$PATH"$/;
+  const lines = content.split('\n');
+  const kept: string[] = [];
+  for (let i = 0; i < lines.length; i++) {
+    const line = lines[i] ?? '';
+    const next = i + 1 < lines.length ? lines[i + 1] : undefined;
+    if (line === '# Mosaic' && next !== undefined && legacyExport.test(next)) {
+      i += 1;
+      continue;
+    }
+    kept.push(line);
+  }
+  return kept.join('\n');
+}
+
+/** Drop the region between the managed-block sentinels, first occurrence. */
+function withoutManagedBlock(content: string): string {
+  const beginIdx = content.indexOf(PATH_BLOCK_BEGIN);
+  if (beginIdx < 0) return content;
+  const endIdx = content.indexOf(PATH_BLOCK_END, beginIdx);
+  if (endIdx < 0) return content;
+  return content.slice(0, beginIdx) + content.slice(endIdx + PATH_BLOCK_END.length);
+}
+
+export function setupPath(mosaicHome: string, resolvedDefaultHome: string): PathAction {
+  // Never write outside the home under test (#1327 S2): a wizard run against
+  // a non-default home (test harnesses, throwaway installs) must not mutate
+  // the operator's real shell profile.
+  if (mosaicHome !== resolvedDefaultHome) {
+    return 'skipped';
   }
 
+  const binDir = join(mosaicHome, 'bin');
   const profilePath = getShellProfilePath();
   if (!profilePath) return 'skipped';
 
   const isWindows = platform() === 'win32';
-  const exportLine = isWindows
+  const block = managedBlockFor(binDir, isWindows);
-    ? `\n# Mosaic\n$env:Path = "${binDir};$env:Path"\n`
-    : `\n# Mosaic\nexport PATH="${binDir}:$PATH"\n`;
 
-  // Check if already in profile
+  let content = '';
   if (existsSync(profilePath)) {
-    const content = readFileSync(profilePath, 'utf-8');
+    content = readFileSync(profilePath, 'utf-8');
-    if (content.includes(binDir)) {
+  }
-      return 'already';
+
-    }
+  // Migration (#1327 S4): legacy unmarked blocks collapse into the managed
+  // block, and an existing managed block is rewritten in place rather than
+  // appended beside itself (S1/S3).
+  const base = stripLegacyPathBlocks(withoutManagedBlock(content), isWindows);
+  const trimmed = base.replace(/\n+$/, '');
+  const next = trimmed.length === 0 ? block : `${trimmed}\n${block}`;
+
+  if (next === content) {
+    return 'already';
   }
 
   try {
-    appendFileSync(profilePath, exportLine, 'utf-8');
+    writeFileSync(profilePath, next, 'utf-8');
     return 'added';
   } catch {
     return 'skipped';
@@ -286,7 +342,7 @@ export async function finalizeStage(
   }
 
   // 7. PATH setup
-  const pathAction = setupPath(state.mosaicHome, p);
+  const pathAction = setupPath(state.mosaicHome, DEFAULT_MOSAIC_HOME);
 
   let summaryShown = false;
   const showSummary = () => {