Dashboard › craft › Session 0HCQB0NNZ4Qp
0HCQB0NNZ4Qpt2k8PYou are an adversarial senior code reviewer for getsentry/craft (a TypeScript release-automation CLI). Review the CURRENT working-tree state of branch feat/workspaces-schema vs origin/master. This is "PR B" of a workspaces redesign: it adds a top-level workspaces: config key + a selector + resolver, and it must be FULLY BACKWARD-COMPATIBLE and INERT when workspaces is absent. Do NOT modify files. Do NOT rubber-stamp — find real bugs, backward-compat breaks, caching bugs, and edge cases.
.craft.yml can define named, independently-versioned release units under a top-level workspaces: map. Each workspace mirrors the release-relevant subset of the top-level config and overrides those fields; unspecified fields inherit the top-level ("base") values.--workspace <name> CLI option (env CRAFT_WORKSPACE), wired through a yargs middleware calling setActiveWorkspace.getConfiguration() returns a RESOLVED VIEW: the base config merged with the selected workspace's overrides. So getGitTagPrefix, getGlobalGitHubConfig, getVersioningPolicy, providers, expandWorkspaceTargets(config.targets) all become workspace-aware automatically.workspaces: true field and expandWorkspaceTargets (npm PACKAGE workspaces). Confirm there's no schema/behavior clash between the two.WORKSPACES_MIN_VERSION (='2.27.0') gate requires the config declare minVersion >= that. Also checkMinimalConfigVersion was changed to relax the CURRENT craft version when it's a pre-release/dev build (e.g. 2.27.0-dev.0 counts as 2.27.0) so a dev build can dogfood a feature before its release is cut.cd /home/byk/Code/getsentry/craft && git diff origin/master...HEAD -- ':!.lore.md' ':!.opencode' ':!.craft-issue-842-plan.md'I'll start by getting the diff and reading the key files.
On branch feat/workspaces-schema Your branch is up to date with 'origin/master'. Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: src/__tests__/config.test.ts modified: src/config.ts modified: src/index.ts modified: src/schemas/project_config.ts Untracked files: (use "git add <file>..." to include in what will be committed) .craft-issue-842-plan.md .opencode/ no changes added to commit (use "git add" and/or "git commit -a") === BRANCH === feat/workspaces-schema === DIFF STAT ===
The changes are uncommitted (working tree). Let me review the working-tree state as the intended PR B.
diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts
index 243835d..0b93a4c 100644
--- a/src/__tests__/config.test.ts
+++ b/src/__tests__/config.test.ts
@@ -8,6 +8,10 @@ import {
getGitTagPrefix,
loadConfigurationFromString,
validateConfiguration,
+ setActiveWorkspace,
+ getActiveWorkspace,
+ getVersioningPolicy,
+ WORKSPACES_MIN_VERSION,
} from '../config';
import { CraftProjectConfigSchema } from '../schemas/project_config';
import { logger } from '../logger';
@@ -122,6 +126,36 @@ describe('noMerge config', () => {
test('fails with invalid noMerge type', () => {
expect(() => validateConfiguration({ noMerge: 'yes' })).toThrow(/noMerge/);
});
+
+ test('parses configuration with workspaces', () => {
+ const data = {
+ minVersion: '2.27.0',
+ github: { owner: 'getsentry', repo: 'toolkit' },
+ workspaces: {
+ cli: {
+ releaseBranchPrefix: 'release/cli',
+ github: { projectPath: 'cli' },
+ ...
Now let me read the full files and the master version of the schema for field-by-field comparison, plus the command builders (publish.ts, prepare.ts) for the parse-time caching concern.
=== MASTER SCHEMA (CraftProjectConfigSchema) ===
z.object({
filePath: z.string().optional(),
policy: z.enum(['auto', 'simple', 'none']).optional(),
scopeGrouping: z.boolean().optional(),
}),
]);
/**
* Craft project-specific configuration
*/
export const CraftProjectConfigSchema = z.object({
github: GitHubGlobalConfigSchema.optional(),
targets: z.array(TargetConfigSchema).optional(),
preReleaseCommand: z.string().optional(),
postReleaseCommand: z.string().optional(),
releaseBranchPrefix: z.string().optional(),
changelog: ChangelogConfigSchema.optional(),
changelogPolicy: z.enum(['auto', 'simple', 'none']).optional(),
minVersion: z
.string()
.regex(/^\d+\.\d+\.\d+.*$/)
.optional(),
requireNames: z.array(z.string()).optional(),
statusProvider: BaseStatusProviderSchema.optional(),
artifactProvider: BaseArtifactProviderSchema.optional(),
versioning: VersioningConfigSchema.optional(),
/**
* Do not merge the release branch after ...
Field-by-field comparison. Master order: github, targets, preReleaseCommand, postReleaseCommand, releaseBranchPrefix, changelog, changelogPolicy, minVersion, requireNames, statusProvider, artifactProvider, versioning, noMerge.
New: releaseUnitFields = github, targets, preReleaseCommand, postReleaseCommand, releaseBranchPrefix, changelog, changelogPolicy, requireNames, statusProvider, artifactProvider, versioning, noMerge. Then CraftProjectConfigSchema spreads those + minVersion + workspaces.
So minVersion moved from position 8 to after noMerge. For Zod .object field order doesn't affect validation. Let me verify github appears exactly once (it's in releaseUnitFields, not re-added at top-level in CraftProjectConfigSchema — correct). Now let me check the parse-time caching concern in the command builders.
src/commands/publish.ts:11: getConfiguration,
src/commands/publish.ts:15: getGlobalGitHubConfig,
src/commands/publish.ts:60: const definedTargets = getConfiguration().targets || [];
src/commands/publish.ts:62: const allowedTargetNames = definedTargets
src/commands/publish.ts:73: choices: allowedTargetNames.concat([
src/commands/publish.ts:243: const config = getConfiguration();
src/commands/publish.ts:265: const githubRepo = await getGlobalGitHubConfig();
src/commands/publish.ts:563: const config = getConfiguration() || {};
src/commands/publish.ts:675: // file by owner/repo. `getGlobalGitHubConfig()` returns cached data on
src/commands/publish.ts:679: publishStateGithubConfig = await getGlobalGitHubConfig();
src/commands/prepare.ts:13: getConfiguration,
src/commands/prepare.ts:14: getGitTagPrefix,
src/commands/prepare.ts:15: getGlobalGitHubConfig,
src/commands/prepare.ts:16: getVersioningPolicy,
src/commands/prepare.ts:704: const config = ...
publish.ts:60 calls getConfiguration().targets — this is the critical parse-time concern. Let me check whether it's in the yargs builder (runs at parse time, BEFORE middleware) or in the handler.
import { captureException } from '@sentry/node';
import { SimpleGit } from 'simple-git';
import {
getGitClient,
getDefaultBranch,
isRepoDirty,
findReleaseBranches,
} from '../utils/git';
import { withTracing } from '../utils/tracing';
import { buildReleaseCommandEnv } from '../utils/releaseCommandEnv';
import { getPublishStatePath } from '../utils/publishState';
/** Default path to post-release script, relative to project root */
const DEFAULT_POST_RELEASE_SCRIPT_PATH = join('scripts', 'post-release.sh');
export const command = ['publish NEW-VERSION'];
export const aliases = ['pp', 'publish'];
export const description = '🛫 Publish artifacts';
export const builder: CommandBuilder = (yargs: Argv) => {
const definedTargets = getConfiguration().targets || [];
const possibleTargetNames = new Set(getAllTargetNames());
const allowedTargetNames = definedTargets
.filter(target => target.name && possibleTargetNames.has(target.name))
.map(BaseTarget.getId);
return ...
This is the critical caching bug candidate. The publish command builder calls getConfiguration() at parse time. In yargs, builders run during parsing to construct the command's options — and crucially, this runs BEFORE middleware. Let me confirm the yargs execution order: builder vs middleware. But there's a more fundamental problem: if the config defines workspaces, getConfiguration() in the builder will THROW ("select one with --workspace") before setActiveWorkspace middleware ever runs.
Let me trace the yargs order precisely and check how the builder handles config-throw today.
* variables with the `--flag=no` kind of notation (using the `=` symbol) but
* not via the positional argument notation (`--flag no`). The only way to do
* this is to define them as string arguments and then _inject_ a truthy string
* if we notice the flag is passed standalone (ie `--flag`).
* @param argv The raw process.argv array
* @returns The processed, injected version of the argv array to pass to yargs
*/
function fixGlobalBooleanFlags(argv: string[]): string[] {
const result = [];
for (const arg of argv) {
result.push(arg);
if (arg.slice(2) in GLOBAL_BOOLEAN_FLAGS) {
result.push('1');
}
}
return result;
}
/**
* Main entrypoint
*/
async function main(): Promise<void> {
// Strip dynamic-linker env vars (LD_PRELOAD, DYLD_*, ...) before anything
// else so they cannot leak into subprocesses Craft spawns.
sanitizeDynamicLinkerEnv();
printVersion();
warnIfCraftEnvFileExists();
const argv = ...
Now I need to definitively establish yargs' builder vs middleware ordering. This is the crux of finding (2). Let me check how the current (master) publish builder behaves — the getConfiguration() call in the builder is pre-existing. The question is whether the builder runs before the workspace middleware. Let me write a minimal test against the actual installed yargs version.
"version": "18.0.0",
done
Rather than fight with module linking, let me just run node from within the craft dir with a small ESM harness that uses its own yargs.
["BUILDER","MIDDLEWARE(workspace=cli)","HANDLER"]
Confirmed: BUILDER runs BEFORE MIDDLEWARE. So publish.ts builder's getConfiguration() at line 60 runs before setActiveWorkspace middleware. This has two consequences for a config that defines workspaces:
getConfiguration() in the builder calls applyWorkspaceSelection with _activeWorkspaceName === undefined → throws "This configuration defines workspaces; select one with --workspace" — even though the user DID pass --workspace cli. The publish command becomes unusable with workspaces.But wait — the builder throwing might be caught, or getConfiguration() might already be cached. Let me check: is getConfiguration() called anywhere before the builder? And does builder throw abort parsing? Let me verify the throw propagation and whether caching from an earlier call masks it.
CAUGHT: BUILDER THREW (workspaces defined, none selected)
The builder throw propagates and aborts before middleware/handler. Now — is getConfiguration() cached before the builder runs? The builder is the first thing to touch config during a publish invocation. On master, the builder call populates _configCache. With workspaces present, that call throws.
Let me verify no earlier getConfiguration() call exists (printVersion, warnIfCraftEnvFileExists, setGlobals) that would populate/throw first.
=== printVersion/warnIfCraftEnvFileExists defs ===
src/utils/env.ts:97:export function warnIfCraftEnvFileExists(): void {
*
* Craft used to load environment variables from these files, but the behavior
* was removed for security reasons: arbitrary values (including credentials)
* could be silently injected into `process.env` based on the current working
* directory. This helper emits a one-time warning per location pointing users
* at their shell / CI environment for credential management.
*/
export function warnIfCraftEnvFileExists(): void {
const candidatePaths: string[] = [];
try {
candidatePaths.push(join(os.homedir(), LEGACY_ENV_FILE_NAME));
} catch {
// os.homedir() can throw in edge cases; skip silently.
}
const configFileDir = getConfigFileDir();
if (configFileDir) {
candidatePaths.push(join(configFileDir, LEGACY_ENV_FILE_NAME));
}
for (const path of candidatePaths) {
if (existsSync(path)) {
logger.warn(
`Found legacy "${LEGACY_ENV_FILE_NAME}" file at "${path}". ` +
`Craft no longer reads this file for security reasons. ...
warnIfCraftEnvFileExists calls getConfigFileDir(), not getConfiguration(), so config isn't parsed/resolved before the builder. Let me confirm getConfigFileDir doesn't call getConfiguration.
/**
* Returns the path to the directory that contains the configuration file
*
* Returns "undefined" if no configuration file can be found.
*/
export function getConfigFileDir(): string | undefined {
const configFilePath = findConfigFile();
if (!configFilePath) {
return undefined;
}
return path.dirname(configFilePath);
}
/**
* Parses and validate passed configuration object
*
* Throw an error is the object cannot be properly parsed as configuration.
*
* @param rawConfig Raw project configuration object
*/
export function validateConfiguration(
rawConfig: Record<string, any>,
): CraftProjectConfig {
logger.debug('Parsing and validating the configuration file...');
try {
return CraftProjectConfigSchema.parse(rawConfig);
} catch (error) {
if (error instanceof ZodError) {
const messages = error.errors
.map(e => `${e.path.join('.')}: ${e.message}`)
.join('\n');
throw new ConfigurationError(
`Cannot parse ...
Confirmed. So for a real publish invocation with a workspaces config:
publish builder → getConfiguration() → applyWorkspaceSelection with _activeWorkspaceName still undefined (middleware hasn't run) → throws "select one with --workspace" even though --workspace cli is present on argv.This is a CRITICAL bug: craft publish is unusable when workspaces is defined. Note targets.ts builder does NOT call getConfiguration at module top — line 10 is inside a function; let me confirm whether it's the builder. Also let me check prepare.ts builder.
=== targets.ts ===
import { getConfiguration, expandWorkspaceTargets } from '../config';
import { formatJson } from '../utils/strings';
import { getAllTargetNames } from '../targets';
import { BaseTarget } from '../targets/base';
export const command = ['targets'];
export const description = 'List defined targets as JSON array';
export async function handler(): Promise<any> {
let definedTargets = getConfiguration().targets || [];
// Expand workspace targets (e.g., npm workspaces)
definedTargets = await expandWorkspaceTargets(definedTargets);
const possibleTargetNames = new Set(getAllTargetNames());
const allowedTargetNames = definedTargets
.filter(target => target.name && possibleTargetNames.has(target.name))
.map(BaseTarget.getId);
console.log(formatJson(allowedTargetNames));
}
=== prepare.ts builder ===
13: getConfiguration,
84:export const builder: CommandBuilder = (yargs: Argv) =>
704: const config = getConfiguration();
819: const config = ...
targets.ts calls getConfiguration in the handler (after middleware) — fine. prepare.ts builder is at line 84; its getConfiguration calls are at 704/819 which are deep in handler logic. Let me confirm prepare's builder (84+) doesn't call getConfiguration.
export const builder: CommandBuilder = (yargs: Argv) =>
yargs
.positional('NEW-VERSION', {
description:
'The new version to release. Can be: a semver string (e.g., "1.2.3"), ' +
'a bump type ("major", "minor", or "patch"), "auto" to determine automatically ' +
'from conventional commits, or "calver" for calendar versioning. ' +
'If omitted, uses the versioning.policy from .craft.yml',
type: 'string',
})
.option('rev', {
alias: 'r',
description:
'Source revision (git SHA or tag) to prepare from (if not branch head)',
type: 'string',
})
.option('no-push', {
default: false,
description: 'Do not push the release branch',
type: 'boolean',
})
.option('no-git-checks', {
default: false,
description: 'Ignore local git changes and unsynchronized remotes',
type: 'boolean',
})
.option('no-changelog', {
default: false,
description: 'Do not ...
prepare builder does NOT call getConfiguration — only publish.ts:60 does at parse/builder time. So the CRITICAL bug is scoped to the publish command.
Now, there IS a subtlety: publish.ts:60 in the builder computes allowedTargetNames from getConfiguration().targets. Even IF the throw weren't there (e.g. non-workspaces config), the builder populates _configCache with the UNRESOLVED (base) config before setActiveWorkspace middleware runs. But setActiveWorkspace clears the cache, so the handler's later getConfiguration() re-resolves correctly. However the builder's allowedTargetNames (the --target choices) would be computed from the BASE targets, not the workspace's targets. With workspaces the builder throws anyway, so that's moot — but worth noting as a secondary issue even after the primary throw is fixed: the --target choices validation would use base targets.
Let me now verify the resolver merge edge cases empirically. Let me build a quick harness using the actual config module. First check versionGreaterOrEqualThan throw behavior for the dev-relaxation question.
51: /** Optional pre-release specifier */
52: pre?: string;
71: pre: matches[4],
81:export function versionGreaterOrEqualThan(v1: SemVer, v2: SemVer): boolean {
88: } else if (!v1.pre && v2.pre) {
90: } else if (v1.pre && !v2.pre) {
93: v1.pre &&
94: v2.pre &&
95: v1.pre !== v2.pre &&
96: /^\d+$/.test(v1.pre) &&
97: /^\d+$/.test(v2.pre)
99: return v1.pre > v2.pre;
100: } else if (v1.build || v2.build || v1.pre || v2.pre) {
101: throw new Error(
111: * A regular expression to detect that a version is a pre-release version.
114: /(?:[^a-z])(preview|pre|rc|dev|alpha|beta|unstable|a|b)(?:[^a-z]|$)/i;
117: * Checks that the provided string is a pre-release version.
120: * @returns True if the string looks like a pre-release version
135:export function versionToTag(version: string, tagPrefix?: string): string {
147: throw new Error('Invalid package.json: the file is empty!');
167: return `${s.major}.${s.minor}.${s.patch}${s.pre ? `-${s.pre}` : ''}${
export function versionGreaterOrEqualThan(v1: SemVer, v2: SemVer): boolean {
if (v1.major !== v2.major) {
return v1.major > v2.major;
} else if (v1.minor !== v2.minor) {
return v1.minor > v2.minor;
} else if (v1.patch !== v2.patch) {
return v1.patch > v2.patch;
} else if (!v1.pre && v2.pre) {
return true;
} else if (v1.pre && !v2.pre) {
return false;
} else if (
v1.pre &&
v2.pre &&
v1.pre !== v2.pre &&
/^\d+$/.test(v1.pre) &&
/^\d+$/.test(v2.pre)
) {
return v1.pre > v2.pre;
} else if (v1.build || v2.build || v1.pre || v2.pre) {
throw new Error(
`Cannot compare the two versions: "${JSON.stringify(
v1,
)}" and "${JSON.stringify(v2)}"`,
);
}
return true;
}
/**
Now I understand the throw path. Let me trace the dev-relaxation logic (finding D):
Current version 2.27.0-dev.0 → parsed: {major:2,minor:27,patch:0,pre:'dev.0'} (actually pre = matches[4]; for 2.27.0-dev.0 the pre group). effectiveCurrentVersion strips pre → {2,27,0, pre:undefined, build:undefined}.
minVersion: 2.27.0 {2,27,0}: all equal, no pre → returns true. ✓ (satisfies, intended)minVersion: 2.28.0 {2,28,0}: minor 27 !== 28 → 27 > 28 = false. ✓ correctly rejects too-newminVersion: 2.14.0: minor 27 > 14 → true. ✓Without relaxation (master): 2.27.0-dev.0 vs 2.27.0: major/minor/patch equal, !v1.pre && v2.pre? v1.pre='dev.0', v2.pre=undefined → v1.pre && !v2.pre → returns false (dev build would REJECT 2.27.0). So the relaxation fixes the dogfooding problem. And it avoids the throw path entirely because it strips pre from the current side, so v1.pre is falsy → the throw branch (v1.build||v2.build||v1.pre||v2.pre) can only fire if v2 (minVersion) has pre/build. minVersion regex /^\d+\.\d+\.\d+.*$/ allows 2.27.0-foo, so a minVersion WITH a pre could still hit the throw — but that's a pre-existing edge, and unchanged by this PR for released current versions. Let me verify the relaxation doesn't weaken released versions: only applied if (currentVersion.pre). A released build (no pre) → effectiveCurrentVersion === currentVersion, unchanged. ✓
Let me now run the verification suite and empirically test the resolver merge behavior.
TSC EXIT: 0
> @sentry/craft@2.27.0-dev.0 test /home/byk/Code/getsentry/craft
> vitest run src/__tests__/config.test.ts
RUN v4.1.8 /home/byk/Code/getsentry/craft
Test Files 1 passed (1)
Tests 30 passed (30)
Start at 10:58:01
Duration 1.32s (transform 365ms, setup 0ms, import 1.09s, tests 32ms, environment 0ms)
Now let me empirically test the resolver's edge cases: the undefined skip behavior (does an absent workspace field appear as undefined in the object?), and the github shallow-merge when base has no github / workspace has no github.
RUN v4.1.8 /home/byk/Code/getsentry/craft No test files found, exiting with code 1 filter: __wstest.test.ts include: src/**/*.test.ts exclude: **/node_modules/**, **/dist/**, **/fixtures/**
Test glob is src/**. Let me put it under src.
RUN v4.1.8 /home/byk/Code/getsentry/craft
Test Files 1 passed (1)
Tests 4 passed (4)
Start at 10:58:23
Duration 1.32s (transform 378ms, setup 0ms, import 1.10s, tests 16ms, environment 0ms)
All resolver edge cases pass. Let me capture the console output for minVersion/workspaces stripping (it printed but got cut). Let me re-run with reporter output.
stdout | src/__tests__/__wstest.test.ts > resolver edge cases > minVersion preserved in resolved config? resolved minVersion = "2.27.0" resolved workspaces = undefined ✓ src/__tests__/__wstest.test.ts > resolver edge cases > absent workspace field: is it undefined-in-object or absent? 10ms ✓ src/__tests__/__wstest.test.ts > resolver edge cases > github merge when BASE has no github 2ms ✓ src/__tests__/__wstest.test.ts > resolver edge cases > github inherited fully when WORKSPACE has no github 1ms ✓ src/__tests__/__wstest.test.ts > resolver edge cases > minVersion preserved in resolved config? 2ms
Confirmed:
Object.entries doesn't iterate them — no nulling). ✓github merge correct when base has no github and when workspace has no github. ✓minVersion preserved (2.27.0), workspaces stripped (undefined) in resolved config. ✓Now the critical caching / parse-time bug. Let me write a test that reproduces the publish-builder-before-middleware scenario at the config level: what happens when getConfiguration() is called with workspaces present but _activeWorkspaceName undefined (simulating the builder), THEN setActiveWorkspace('cli') runs, THEN getConfiguration() again (handler).
✓ src/__tests__/__wsbuilder.test.ts > publish builder-before-middleware simulation > builder call (no selection) throws even though --workspace will be set later 10ms
Confirmed the CRITICAL bug reproduces: with a workspaces config, publish's builder calls getConfiguration() before the --workspace middleware runs, so applyWorkspaceSelection sees _activeWorkspaceName === undefined and throws "select one with --workspace" — aborting craft publish entirely, even when the user passed --workspace cli.
Let me also check the --config-from path: loadConfigurationFromString applies workspace selection too (good for consistency), but is --config-from handled in a way that interacts with the builder? And let me confirm the enumeration of all module-level caches in config.ts.
53:let _configPathCache: string;
58:let _configCache: CraftProjectConfig;
74:let _activeWorkspaceName: string | undefined;
86: _configCache = undefined as unknown as CraftProjectConfig;
87: _globalGitHubConfigCache = undefined;
235: _configPathCache = probePath;
320: _configCache = applyWorkspaceSelection(parsed);
338: _configCache = applyWorkspaceSelection(parsed);
441:let _globalGitHubConfigCache: GitHubGlobalConfig | null | undefined;
446: if (_globalGitHubConfigCache === null) {
482: _globalGitHubConfigCache = Object.freeze(repoGitHubConfig);
Three caches: _configPathCache (line 53), _configCache (58), _globalGitHubConfigCache (441). setActiveWorkspace clears _configCache and _globalGitHubConfigCache but NOT _configPathCache. That's correct — the config file PATH doesn't change with workspace selection (same .craft.yml), so leaving it cached is right, not a bug.
Now let me verify the _globalGitHubConfigCache null-vs-undefined logic at line 441-482.
let _globalGitHubConfigCache: GitHubGlobalConfig | null | undefined;
export async function getGlobalGitHubConfig(
clearCache = false,
): Promise<GitHubGlobalConfig> {
if (!clearCache && _globalGitHubConfigCache !== undefined) {
if (_globalGitHubConfigCache === null) {
throw new ConfigurationError(
'GitHub configuration not found in the config file and cannot be determined from Git',
);
}
return _globalGitHubConfigCache;
}
// We extract global GitHub configuration (owner/repo) from top-level
// configuration
let repoGitHubConfig = getConfiguration(clearCache).github || null;
if (!repoGitHubConfig) {
const configDir = getConfigFileDir() || '.';
const git = createGitClient(configDir);
let remoteUrl;
try {
const remotes = await git.getRemotes(true);
const defaultRemote =
remotes.find(remote => remote.name === 'origin') || remotes[0];
remoteUrl =
defaultRemote &&
...
The null/undefined logic is sound:
undefined = "not yet computed" → falls through to recompute.null = "computed, GitHub config not found" → throws._globalGitHubConfigCache = undefined in setActiveWorkspace correctly forces recompute on next call. The type change from | null to | null | undefined is necessary and correct — previously the initial (uninitialized) value was implicitly undefined anyway (TS just didn't track it), and the !== undefined guard already existed on master. Let me confirm master's guard.283:let _globalGitHubConfigCache: GitHubGlobalConfig | null;
287: if (!clearCache && _globalGitHubConfigCache !== undefined) {
288: if (_globalGitHubConfigCache === null) {
294: return _globalGitHubConfigCache;
324: _globalGitHubConfigCache = Object.freeze(repoGitHubConfig);
Master already used !== undefined (the var was declared | null but was actually undefined before first assignment — a pre-existing minor type imprecision). The PR's type widening to include undefined is correct and honest. No bug here.
Now let me run the full suite and prettier check.
68 | (checkExecutableIsPresent as MockedFunction<
| ^
69 | typeof checkExecutableIsPresent
70 | >) = vi.fn();
Plugin: vite:esbuild
File: /home/byk/Code/getsentry/craft/src/targets/__tests__/symbolCollector.test.ts
[debug] Failed to fetch from remote, using locally cached refs
[debug] Failed to list remote branches
Test Files 58 passed (58)
Tests 1077 passed | 1 skipped (1078)
Start at 10:59:18
Duration 31.11s (transform 2.96s, setup 0ms, import 50.32s, tests 23.98s, environment 13ms)
Full suite: 1077 passed, 1 skipped. Now prettier.
Checking formatting... All matched files use Prettier code style!
Prettier clean. Now let me verify one more edge: the --target choices in publish builder would use BASE targets even after the throw is fixed (secondary issue). And confirm the no-unused-vars eslint-disable is on the right line and lint passes.
> @sentry/craft@2.27.0-dev.0 lint /home/byk/Code/getsentry/craft > eslint --cache --cache-strategy content /home/byk/Code/getsentry/craft/src/commands/publish.ts 441:14 warning '_abortError' is defined but never used @typescript-eslint/no-unused-vars 458:14 warning '_abortError' is defined but never used @typescript-eslint/no-unused-vars 474:16 warning '_statusError' is defined but never used @typescript-eslint/no-unused-vars 480:18 warning '_diffError' is defined but never used @typescript-eslint/no-unused-vars 486:16 warning '_abortError' is defined but never used @typescript-eslint/no-unused-vars /home/byk/Code/getsentry/craft/src/utils/git.ts 231:12 warning '_err' is defined but never used @typescript-eslint/no-unused-vars 239:12 warning '_err' is defined but never used @typescript-eslint/no-unused-vars ✖ 7 problems (0 errors, 7 warnings)
0 errors, 7 pre-existing warnings (all in files not touched by this PR — publish.ts, git.ts). The eslint-disable on the destructure in config.ts is working (no warning there).
Now let me double-check whether the publish builder's getConfiguration() throw is actually reachable in practice — i.e., is the builder wrapped in any error handling that would degrade gracefully? And confirm on master the builder already calls getConfiguration (so this isn't NEW behavior, just newly-throwing). Let me check master's publish builder.
export const builder: CommandBuilder = (yargs: Argv) => {
const definedTargets = getConfiguration().targets || [];
const possibleTargetNames = new Set(getAllTargetNames());
const allowedTargetNames = definedTargets
.filter(target => target.name && possibleTargetNames.has(target.name))
.map(BaseTarget.getId);
return yargs
Confirmed: the builder-time getConfiguration() call is pre-existing on master (it never threw before because there was no workspace gate). PR B introduces the throw condition. So a user running craft publish 1.2.3 --workspace cli against a workspaces config will hit the builder throw before the middleware sets the workspace.
One more check: is there test coverage for this interaction anywhere? And does getConfiguration get called with a config file present in the publish builder path only when a .craft.yml exists? (If no config file, findConfigFile throws — but that's pre-existing too.) Let me verify the builder doesn't guard for missing config, confirming the new throw is the only new failure mode.
The behavior is clear. Let me also verify one final backward-compat concern: the schema releaseUnitFields as const spread — does z.object({...releaseUnitFields, minVersion, workspaces}) produce identical validation? I already compared fields. Let me confirm no .passthrough() was accidentally introduced and that unknown keys still strip.
✓ src/__tests__/__wsstrip.test.ts > schema backward-compat > unknown top-level keys are still stripped (no passthrough) 5ms ✓ src/__tests__/__wsstrip.test.ts > schema backward-compat > all master fields still validate 1ms
Schema is byte-equivalent for backward compat (unknown keys stripped, all fields validate). Let me clean up scratch files.
no stray files
Review complete. Here are my findings.
feat/workspaces-schema)Verdict: DO-NOT-MERGE as-is. One CRITICAL correctness bug makes craft publish unusable with any workspaces config. Everything else is solid — schema is genuinely backward-compatible, the resolver merge is correct, caching is mostly right, and the dev-version relaxation is sound.
craft publish --workspace X throws before the workspace is ever selectedsrc/commands/publish.ts:60 (interaction with src/config.ts:381-407 + src/index.ts:111-125)
The publish yargs builder calls getConfiguration() at parse time. I verified empirically against the installed yargs@18 that builders run before middleware:
["BUILDER","MIDDLEWARE(workspace=cli)","HANDLER"]
So the sequence for craft publish 1.2.3 --workspace cli against a workspaces config is:
getConfiguration() → applyWorkspaceSelection(parsed) with _activeWorkspaceName === undefined (middleware hasn't run yet)hasWorkspaces is true, _activeWorkspaceName is falsy → throws This configuration defines workspaces; select one with --workspace <name>...setActiveWorkspace middleware and handler never run.Reproduced at the config layer:
✓ builder call (no selection) throws even though --workspace will be set later
Result: craft publish is completely broken for any repo that defines workspaces — the exact feature this PR ships. prepare, targets, changelog, config are unaffected (their builders don't call getConfiguration(); only publish.ts:60 does — confirmed pre-existing on master, but it never threw before because there was no gate). ...
Date: Jul 22, 2026 * 🟡 (10:57) [tool:result] versionGreaterOrEqualThan() full implementation shown: compares major/minor/patch numerically first; if !v1.pre && v2.pre → true; if v1.pre && !v2.pre → false; if both pre differ and both purely numeric (/^\d+$/) → numeric compare; else if v1.build || v2.build || v1.pre || v2.pre → throws `Cannot compare the two versions: "${JSON.stringify(v1)}" and "…
Date: Jul 22, 2026 * 🟡 (10:55) [tool:result] Git status for feat/workspaces-schema (getsentry/craft): branch up to date with origin/master; changes are UNSTAGED/uncommitted in working tree (not yet committed) for src/__tests__/config.test.ts, src/config.ts, src/index.ts, src/schemas/project_config.ts; untracked files .craft-issue-842-plan.md and .opencode/. * 🟢 (10:55) Assistant noted the revie…
Date: Jul 22, 2026 * 🟡 (10:53) User requested an adversarial senior-code-reviewer review of branch `feat/workspaces-schema` vs `origin/master` in repo getsentry/craft (TypeScript release-automation CLI), located at /home/byk/Code/getsentry/craft. [requested-review] * 🔴 (10:53) User stated this is "PR B" of a workspaces redesign: adds top-level `workspaces:` config key + a selector + resolver; m…