Distillation

Date: Sep 8, 2026

• 🔴 (17:14) User requested a strict read-only adversarial review in /home/byk/Code/getsentry/craft-workspace-action-propagation of the current uncommitted fix for Craft PR #872 against current HEAD f174ceafa57355e71617f32ba1b0a030953be0a2; instructed not to edit or mutate files. Requested review of src/config.ts, src/__tests__/config.test.ts, and relevant callers; verification that realpath error handling skips only broken/racy glob candidates, cannot hide unsafe entries or bypass lexical/physical containment, preserves determinism, and that the regression genuinely fails before the fix; requested diff-hygiene check, severity-ranked file:line findings, explicit PASS/FAIL, test gaps, and a response ending exactly MERGE or DO-NOT-MERGE.
• 🔴 (17:14) User stated package-management preference: always use pnpm; never use npm or yarn.
• 🟢 (17:14) Repository guidance in AGENTS.md: Node.js is Volta-managed at v22.12.0; dependencies install with pnpm install --frozen-lockfile; development commands are pnpm build, pnpm test, pnpm lint, and pnpm fix; manual testing is pnpm build && ./dist/craft.
• 🟢 (17:14) Repository guidance: TypeScript throughout; Prettier 3.x uses single quotes and no arrow parens via .prettierrc.yml; ESLint 9.x flat config in eslint.config.mjs using typescript-eslint; unused variables prefixed _ are allowed.
• 🟢 (17:14) Repository guidance: tests use Vitest, reside in src/__tests__/, follow *.test.ts, and use vi.fn(), vi.mock(), and vi.spyOn(); CI tests Node.js 20 and 22; main branch is master.
• 🟢 (17:14) Repository guidance: .craft.yml is project configuration at repository root; schemas are in src/schemas/; dry-run destructive operations use getGitClient()/createGitClient(directory), getGitHubClient(), safeFs, safeExec(), or safeExecSync().
• 🟡 (17:15) Current uncommitted diff modifies exactly src/config.ts and src/__tests__/config.test.ts (28 insertions, 1 deletion); HEAD is f174ceafa57355e71617f32ba1b0a030953be0a2.
• 🟡 (17:15) Code change in src/config.ts:249-258: getWorkspaceGlobMatches() changed from unconditional const realMatch = realpathSync(resolvedMatch); to let realMatch: string; try { realMatch = realpathSync(resolvedMatch); } catch { // Glob candidates may disappear or be broken symlinks before resolution. return false; }, then retains isSafeWorkspacePath(match), lexical containment based on resolvedMatch under root, and physical containment based on realMatch under realRoot.
• 🟡 (17:15) Test added in src/__tests__/config.test.ts:504-523, ignores broken symlinks matched by workspace globs: creates temp directory craft-workspaces-*, creates packages/cli, symlinks packages/broken to nonexistent missing-workspace, writes .craft.yml with minVersion: ${WORKSPACES_MIN_VERSION} and workspace glob packages/*: {}, changes cwd to fixture directory, and expects getWorkspaceNames() to equal ['packages/cli'].
• 🟡 (17:15) Existing workspace tests establish that packages/**/release does not expand through symlinked packages/external pointing outside the workspace root, returning only ['packages/internal/release']; unsafe brace alternatives {../outside/*,packages/*} and {/tmp/*,packages/*} must throw Workspace paths must use safe ASCII segments.; overlapping globs packages/* and packages/cli* must report that packages/cli matches multiple workspace patterns.
• 🟡 (17:16) Relevant callers: src/index.ts:92-99 resolves active workspace before yargs parsing via setActiveWorkspace(extractWorkspaceSelection(argv)), because builders can read configuration before middleware; src/commands/prepare.ts:796-815 fetches and loads .craft.yml from trusted --config-from remote branch using loadConfigurationFromString(configContent, repositoryRoot.trim()); getWorkspaceNames() reads config, calls getWorkspaceNamesFromConfig(parsed, path.dirname(configPath)), and checks workspace minVersion when results are nonempty.
• 🟡 (17:16) Focused test command result: Vitest v4.1.8 ran in /home/byk/Code/getsentry/craft-workspace-action-propagation; 1 test file passed, 68 tests passed, duration 1.48s (transform 383ms, import 1.16s, tests 121ms).
• 🟡 (17:17) Assistant adversarial-review finding, MUST-FIX High: src/config.ts:253-258 catches every realpathSync error, not only broken/racy candidates; this silently drops EACCES/EPERM, ELOOP, and unexpected I/O errors. Since lexical and physical containment checks occur only afterward at :260-265, rejected candidates receive neither validation nor diagnostics. Assistant assessed that this cannot admit a candidate past containment checks, but can hide unsafe or ambiguous filesystem state; recommended rethrowing non-disappearance errors.
• 🟡 (17:17) Assistant adversarial-review finding, MUST-FIX Medium: src/__tests__/config.test.ts:504-523 does not prove the broken symlink reaches globSync; it passes whether globSync returns the link and the new filter skips it or whether globSync omits it before filtering, so it does not demonstrate that pre-fix f174ce…:src/config.ts:252 fails. Recommended asserting the fixture is returned by globSync or adding a deterministic mock/spying seam that makes realpathSync throw ENOENT for a glob candidate.
• 🟡 (17:17) Assistant review PASS assessments: lexical containment remains enforced by validated workspace glob syntax and isSafeWorkspacePath() at src/config.ts:235-239 and :260; physical containment remains enforced for successfully resolved candidates at :263-265; ordering remains deterministic through .sort() at :268; selection (:197), listing (:210, :628-635), and remote config loading (src/commands/prepare.ts:805-815) route glob expansion through the reviewed function; only the two requested files changed; git diff --check passed; focused src/__tests__/config.test.ts suite passed with 68 tests.
• 🟡 (17:17) Assistant identified test gaps: unexpected realpathSync errors should fail rather than disappear; candidates removed between globbing and realpathSync should be skipped; regression must prove baseline failure rather than merely fixed behavior.
• 🟡 (17:17) Assistant's final merge recommendation was DO-NOT-MERGE.