Dashboard › craft › Session 012Op1WSIwri
012Op1WSIwriqapweYou are an adversarial senior code reviewer for getsentry/craft (TypeScript release CLI). Final pre-merge review of branch feat/workspaces-schema (PR #848) vs origin/master. Do NOT modify files. Find real bugs only; be concise.
This PR adds a top-level workspaces: config key + --workspace selector + a resolver that makes getConfiguration() return a base+workspace-merged view. Must be fully backward-compatible/inert when workspaces absent.
NOTE: I am concurrently fixing TWO Cursor Bugbot findings, so re-derive them independently but focus your energy elsewhere:
github shallow-merge produces a truthy-but-incomplete github (missing owner/repo) when top-level has no github, causing getGlobalGitHubConfig to skip the git-remote fallback.extractWorkspaceSelection in src/index.ts greedily takes the token after --workspace even if it's another flag or missing, and can suppress CRAFT_WORKSPACE.Get the diff:
cd /home/byk/Code/getsentry/craft && git diff origin/master...HEAD -- ':!.lore.md' ':!.opencode' ':!.craft-issue-842-plan.md'Scrutinize for anything BEYOND the two known bugs:
_configCache or the base such that a later resolve is wrong?setActiveWorkspace clears _configCache and _globalGitHubConfigCache. Is there ANY other cached state (providers, tag prefix, path) that could serve stale data across workspace switches within one process? Enumerate module-level let _* in config.ts.I'll start by getting the diff and reading the key files, then run the verification commands.
4e2d8f4 feat(config): add top-level workspaces schema + --workspace selector
f70d5a8 feat: prefix-aware read paths for monorepo multi-product releases (#847)
83964a8 fix(cloudflare): default to worker, optional account id, infer prod branch (#846)
0f8720e feat(targets): add cloudflare deploy target (#843)
7a65e77 meta: Bump new development version
---BRANCH---
feat/workspaces-schema
---DIFF---
diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts
index 243835d..cd3441f 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', ...
Now let me read the full files and run the verification commands in parallel.
<path>/home/byk/Code/getsentry/craft/src/config.ts</path>
<type>file</type>
<content>
1: import { existsSync, lstatSync, readFileSync } from 'fs';
2: import path from 'path';
3:
4: import { load } from 'js-yaml';
5: import GitUrlParse from 'git-url-parse';
6: import { createGitClient } from './utils/git';
7: import { ZodError } from 'zod';
8:
9: import { logger } from './logger';
10: import {
11: CraftProjectConfig,
12: CraftProjectConfigSchema,
13: GitHubGlobalConfig,
14: ArtifactProviderName,
15: StatusProviderName,
16: TargetConfig,
17: ChangelogPolicy,
18: VersioningPolicy,
19: Workspace,
20: } from './schemas/project_config';
21: import { ConfigurationError } from './utils/errors';
22: import { isCompiledGitHubAction } from './utils/detection';
23: import {
24: getPackageVersion,
25: parseVersion,
26: versionGreaterOrEqualThan,
27: SemVer,
28: } from './utils/version';
29: // Note: We import getTargetByName lazily in expandWorkspaceTargets to avoid
30: ...
The working tree already contains the fixes for the two Bugbot findings (github shallow-merge completeness check at lines 144-154). Let me read index.ts to check the extractWorkspaceSelection fix and run verification.
<path>/home/byk/Code/getsentry/craft/src/index.ts</path>
<type>file</type>
<content>
1: #!/usr/bin/env node
2: // Import Sentry instrumentation first, before any other code
3: import './instrument';
4:
5: import isCI from 'is-ci';
6: import yargs from 'yargs';
7:
8: import { logger, LogLevel } from './logger';
9: import {
10: sanitizeDynamicLinkerEnv,
11: warnIfCraftEnvFileExists,
12: } from './utils/env';
13: import { envToBool, setGlobals } from './utils/helpers';
14: import { getPackageVersion } from './utils/version';
15: import { withTracing } from './utils/tracing';
16: import { setActiveWorkspace } from './config';
17:
18: // Commands
19: import * as prepare from './commands/prepare';
20: import * as publish from './commands/publish';
21: import * as targets from './commands/targets';
22: import * as config from './commands/config';
23: import * as artifacts from './commands/artifacts';
24: import * as changelog from './commands/changelog';
25:
26: function ...
The extractWorkspaceSelection in the working tree is still the naive version — the Medium Bugbot fix is NOT yet applied here (user said they're concurrently fixing it). Let me run verification and read publish.ts + schema.
TSC_EXIT=0
[info] [[target/github]] Release for tag "1.0.0" already exists and is published. Skipping GitHub release creation (likely from a previous run). [info] [[target/github]] Git tag: "1.0.0" [info] [[target/github]] Git tag: "1.0.0" [info] [[target/github]] Git tag: "1.0.0" [warn] [[target/github]] Release body exceeds GitHub limit (180000 > 125000 chars). Truncating and linking to full changelog. [info] [[target/github]] Git tag: "1.0.0" [warn] [[target/github]] Release body exceeds GitHub limit (180000 > 125000 chars). Truncating and linking to full changelog. [info] [[target/github]] Git tag: "1.0.0" [warn] [[target/github]] Release body exceeds GitHub limit (125001 > 125000 chars). Truncating and linking to full changelog. [info] [[target/github]] Git tag: "1.0.0" [warn] [[target/github]] Refusing to delete release "v1.0.0" because it is not a draft [info] [dry-run] Would execute: github.repos.deleteRelease(...) 11:19:39 AM [vite] (ssr) warning: This assignment will throw because ...
tsc clean, 1078 passed / 1 skipped. Now let me read publish.ts and the schema file, and check the partial() behavior of the github schema.
<path>/home/byk/Code/getsentry/craft/src/schemas/project_config.ts</path>
<type>file</type>
<content>
1: import { z } from 'zod';
2:
3: /**
4: * DEPRECATED: Use changelog.policy instead. Different policies for changelog management
5: */
6: export const enum ChangelogPolicy {
7: Auto = 'auto',
8: Simple = 'simple',
9: None = 'none',
10: }
11:
12: /**
13: * Name of the status provider
14: */
15: export const enum StatusProviderName {
16: GitHub = 'github',
17: }
18:
19: /**
20: * Name of the artifact provider
21: */
22: export const enum ArtifactProviderName {
23: GCS = 'gcs',
24: GitHub = 'github',
25: None = 'none',
26: }
27:
28: /**
29: * Default versioning policy when no version argument is provided. ...
Now let me read publish.ts around the builder and check the getAllTargetNames behavior and the strictCommands interaction.
<path>/home/byk/Code/getsentry/craft/src/commands/publish.ts</path>
<type>file</type>
<content>
1: import { Arguments, Argv, CommandBuilder } from 'yargs';
2: import chalk from 'chalk';
3: import { existsSync, mkdirSync, readFileSync } from 'fs';
4:
5: import { safeFs } from '../utils/dryRun';
6: import { dirname, join } from 'path';
7: import * as shellQuote from 'shell-quote';
8: import stringLength from 'string-length';
9:
10: import {
11: getConfiguration,
12: getStatusProviderFromConfig,
13: getArtifactProviderFromConfig,
14: DEFAULT_RELEASE_BRANCH_NAME,
15: getGlobalGitHubConfig,
16: expandWorkspaceTargets,
17: getNoMergeConfig,
18: } from '../config';
19: import { formatTable, logger } from '../logger';
20: import { TargetConfig } from '../schemas/project_config';
21: import { getAllTargetNames, getTargetByName, SpecialTarget } from '../targets';
22: import { BaseTarget } from '../targets/base';
23: import {
24: ConfigurationError,
25: handleGlobalError,
26: ...
I have everything I need. Let me verify one thing about the partial() merge — whether a github key present in the workspace but empty ({}) triggers the fixed branch correctly, and confirm the Object.freeze interaction with the resolver. Let me also check the publish handler re-reads config after middleware.
11: getConfiguration,
15: getGlobalGitHubConfig,
68: const definedTargets = getConfiguration().targets || [];
254: const config = getConfiguration();
276: const githubRepo = await getGlobalGitHubConfig();
574: const config = getConfiguration() || {};
686: // file by owner/repo. `getGlobalGitHubConfig()` returns cached data on
690: publishStateGithubConfig = await getGlobalGitHubConfig();
Now let me check whether the builder's cache-poisoning matters: the builder calls getConfiguration() which sets _configCache. After middleware re-calls setActiveWorkspace, the cache is cleared. Let me confirm the middleware always runs and clears even when the pre-parse extraction already set the same workspace.
The middleware at index.ts:150 calls setActiveWorkspace(argv.workspace) unconditionally, which clears _configCache. So even if the builder poisoned the cache with a wrong/base view, the handler re-resolves. Good. Let me verify one subtle case: does the builder's getConfiguration() succeed and cache a resolved workspace view (since pre-parse extraction ran), and does the middleware's clearing handle it. Let me trace the caching timeline precisely.
A {"github":{"owner":"o","repo":"r","projectPath":"cli"}}
B {}
C {"github":{"owner":"o","repo":"r"}}
D {}
E {"github":{"owner":"o2","repo":"r"}}
F base.github after mutating resolved: {"owner":"o","repo":"r"}
G resolved.targets===workspace.targets ref-shared: npm
The merge logic is correct across all edge cases. Case F confirms no base mutation for github (spread creates a fresh object). Now let me examine the potential shared-reference concern (Case G) more carefully — resolved.targets IS the same array reference as workspace.targets, which lives inside the cached parsed config's workspaces map. Let me assess whether that's a real mutation risk in craft's flow.
src/commands/publish.ts:68: const definedTargets = getConfiguration().targets || []; src/commands/publish.ts:683: let targetConfigList = await expandWorkspaceTargets(config.targets || []); src/commands/prepare.ts:908: targets: config.targets, src/commands/targets.ts:10: let definedTargets = getConfiguration().targets || [];
<path>/home/byk/Code/getsentry/craft/src/commands/publish.ts</path>
<type>file</type>
<content>
683: let targetConfigList = await expandWorkspaceTargets(config.targets || []);
684:
685: // Resolve the GitHub config up front so we can key the publish-state
686: // file by owner/repo. `getGlobalGitHubConfig()` returns cached data on
687: // subsequent calls, so this is effectively free.
688: let publishStateGithubConfig = null;
689: try {
690: publishStateGithubConfig = await getGlobalGitHubConfig();
691: } catch {
692: // Fall through with null — getPublishStatePath() handles this by
693: // falling back to a cwd-hash-only filename, keeping the file in
694: // $XDG_STATE_HOME/craft/ rather than the repo.
695: }
696: const publishStateFile = getPublishStatePath(
697: newVersion,
698: publishStateGithubConfig,
699: );
700:
701: logger.info(`Looking for publish state file for ${newVersion}...`);
702: logger.debug(`Publish state file path: ...
The shared-reference is benign: expandWorkspaceTargets builds a new array, and the config is re-resolved fresh each time (cache is cleared on setActiveWorkspace, and single-process craft runs resolve one workspace). No cross-workspace mutation risk in practice since a resolve always produces a fresh resolved object from a freshly-parsed config. I have enough to write the review.
Let me do one final check: the getAllTargetNames() fallback in the publish builder — confirm it returns bare names (not IDs) and that mixing with BaseTarget.getId output in the choices is consistent.
src/targets/index.ts:63:export function getAllTargetNames(): string[] {
src/targets/base.ts:29: public static getId(target: TargetConfig): string {
src/targets/base.ts:43: this.id = BaseTarget.getId(config);
---
src/targets/index.ts:63:export function getAllTargetNames(): string[] {
<path>/home/byk/Code/getsentry/craft/src/targets/base.ts</path>
<type>file</type>
<content>
27: public readonly githubRepo?: GitHubGlobalConfig;
28:
29: public static getId(target: TargetConfig): string {
30: return target.id
31: ? `${target.name}[${target.id}]`
32: : target.name || '__undefined__';
33: }
34:
35: public constructor(
36: config: TargetConfig,
37: artifactProvider: BaseArtifactProvider,
38: githubRepo?: GitHubGlobalConfig,
39: ) {
40: this.logger = loggerRaw.withScope(`[target/${config.name}]`);
41: this.artifactProvider = artifactProvider;
42: this.config = config;
43: this.id = BaseTarget.getId(config);
44: this.githubRepo = githubRepo;
45: this.filterOptions = {};
46: if (this.config.includeNames) {
(Showing lines 27-46 of 99. Use offset=47 to continue.)
</content>
This is a pre-existing minor inconsistency (fallback yields bare names, happy path yields name[id] IDs), unchanged in spirit from master's behavior and only affects the --target choices list in the rare parse-time-failure fallback. Not a regression. I have everything for the review.
feat/workspaces-schema (PR #848)Verdict: SOLID — MERGE. No CRITICAL or MAJOR issues beyond the two Bugbot findings you're already fixing. tsc clean (exit 0), full suite 1078 passed / 1 skipped.
(High) github shallow-merge → truthy-but-incomplete — ALREADY FIXED in working tree.
src/config.ts:137-154 no longer does a naive spread. It computes mergedGithub, then only adopts it if owner && repo; otherwise it deletes github so getGlobalGitHubConfig() (config.ts:471) still hits the git-remote fallback. I re-derived all edge cases (base-has-github + projectPath override; base-no-github + projectPath-only; empty github:{}; owner-only override) — all correct, and no base mutation (spread makes a fresh object). Confirmed the fix resolves it. Note: the diff you pasted shows the old buggy version; the on-disk file has the corrected version. Make sure the corrected version is what gets committed.
(Medium) extractWorkspaceSelection greedy token + CRAFT_WORKSPACE suppression — CONFIRMED, still present.
src/index.ts:80-91. Two real defects:
--workspace --dry-run returns "--dry-run" as the workspace name (greedy next-token).--workspace (last arg) returns argv[i+1] = undefined, then returns undefined immediately, suppressing a set CRAFT_WORKSPACE. So CRAFT_WORKSPACE=cli craft publish --workspace silently ignores the env var.Concrete fix:
function extractWorkspaceSelection(argv: string[]): string | undefined {
for (let i = 0; i < argv.length; i++) {
const arg = argv[i];
if (arg === '--workspace') {
const next = argv[i + 1];
if (next ...
Date: Jul 22, 2026 * 🔴 (11:17) User requested adversarial senior-code-reviewer final pre-merge review of branch feat/workspaces-schema (PR #848) vs origin/master in getsentry/craft (TypeScript release CLI). PR adds top-level workspaces: config key + --workspace selector + resolver merging base+workspace config via getConfiguration(). Must be fully backward-compatible/inert when workspaces absent…
Date: Jul 22, 2026 * 🟡 (11:18) User posted full contents of craft's config.ts with line numbers, including imports StatusProviderName and BaseArtifactProvider (from './artifact_providers/base'), and several functions/blocks: resolveWorkspaceConfig (~lines 113-227), checkMinimalConfigVersion (line 360), getGlobalGitHubConfig with _globalGitHubConfigCache (line ~458), an artifactProvider switch in…
Date: Jul 22, 2026 * 🟡 (11:18) Tool result returned git log for branch feat/workspaces-schema with 5 recent commits: 4e2d8f4 feat(config): add top-level workspaces schema + --workspace selector; f70d5a8 feat: prefix-aware read paths for monorepo multi-product releases (#847); 83964a8 fix(cloudflare): default to worker, optional account id, infer prod branch (#846); 0f8720e feat(targets): add clo…