Dashboard › craft › Session 08PQnOGCZ5RJ
08PQnOGCZ5RJGaJRhYou are an adversarial senior code reviewer for getsentry/craft (a TypeScript release-automation CLI). Review the CURRENT state of branch feat/prefixed-tags-fixes vs origin/master. This is "PR A": the salvageable subset of a parked PR (#844) for monorepo prefixed-tag support, PLUS two review fixes. Do NOT modify files. Do NOT rubber-stamp — find real bugs, regressions, edge cases.
Run:
cd /home/byk/Code/getsentry/craft && git log --oneline origin/master..HEADgit diff origin/master...HEAD -- ':!.lore.md' ':!.craft-issue-842-plan.md' ':!.opencode'
Read full files as needed: src/utils/git.ts, src/commands/changelog.ts, src/config.ts, src/utils/version.ts, src/utils/tests/git.test.ts, src/commands/tests/changelog-versioning-policy.test.ts, src/tests/config.test.ts, docs.getLatestTag(git, tagPrefix='') uses git describe --match '<prefix>*'; threaded through prepare.ts and changelog.ts; getGitTagPrefix() in config.ts warns when multiple github targets declare different tagPrefix and returns the first; version.ts getVersion/parseVersion extract version from prefixed tags (e.g. cli@1.2.3→1.2.3).changelog.ts: getGitTagPrefix() is now wrapped in try/catch (mirroring the versioningPolicy path) so an invalid/unreadable .craft.yml does NOT abort a standalone craft changelog run; on error it falls back to empty prefix (latest tag overall).git.ts findReleaseBranches: the release-branch prefix is now treated as an OPAQUE string. A branch is <prefix>/<version>; the code recovers the branch's prefix by cutting at the LAST / (lastIndexOf('/') + slice), replacing the previous segment-count split/join logic. Fuzzy matching (Levenshtein <= 3) is unchanged.A. findReleaseBranches opaque-prefix rewrite (git.ts ~247-273). This is ...
I'll conduct a thorough adversarial review. Let me start by gathering the diff and reading the relevant files.
412b796 feat: support prefixed tags for monorepo multi-product releases
diff --git a/docs/src/content/docs/configuration.md b/docs/src/content/docs/configuration.md
index d081699..6258a93 100644
--- a/docs/src/content/docs/configuration.md
+++ b/docs/src/content/docs/configuration.md
@@ -176,6 +176,22 @@ releaseBranchPrefix: publish
Full branch name: `{releaseBranchPrefix}/{version}`
+The prefix may contain slashes, which is useful for monorepos that release
+several independently-versioned products from one repository. Pairing a slashed
+`releaseBranchPrefix` with a per-product `github.tagPrefix` keeps each product's
+release branches and tags separate:
+
+```yaml
+releaseBranchPrefix: release/cli
+targets:
+ - name: github
+ tagPrefix: "cli@"
+```
+
+This produces branches like `release/cli/1.2.3` and tags like `cli@1.2.3`. See
+the [GitHub target docs](./targets/github/#monorepo-independently-versioned-products)
+for the full monorepo pattern.
+
## Changelog Policies
Craft supports `simple` and `auto` changelog management modes. ...
The diff shows something unexpected. The task description says Fix 2 rewrites findReleaseBranches to use lastIndexOf('/') + slice, treating prefix as opaque. But the actual diff shows the code still uses prefix.split('/').length segment-count logic — NOT the lastIndexOf approach. This is a critical discrepancy. Let me read the actual current file state.
<path>/home/byk/Code/getsentry/craft/src/utils/git.ts</path>
<type>file</type>
<content>
218: * @param prefix The release branch prefix to search for
219: * @param limit Maximum number of branches to return per category
220: * @returns Object with exactMatches and fuzzyMatches arrays
221: */
222: export async function findReleaseBranches(
223: git: SimpleGit,
224: prefix: string,
225: limit: number = 10,
226: ): Promise<ReleaseBranchSearchResult> {
227: const MAX_EDIT_DISTANCE = 3;
228:
229: try {
230: await git.fetch();
231: } catch (_err) {
232: logger.debug('Failed to fetch from remote, using locally cached refs');
233: }
234:
235: let allBranches: string[];
236: try {
237: const output = await git.raw('branch', '-r');
238: allBranches = parseGitBranchOutput(output);
239: } catch (_err) {
240: logger.debug('Failed to list remote branches');
241: return { exactMatches: [], fuzzyMatches: [] };
242: }
243:
244: const exactMatches: ...
The working tree has the lastIndexOf opaque-prefix version (matching the task description), but the committed diff (git diff origin/master...HEAD) shows the OLD split('/').length segment logic. This means Fix 2 is uncommitted/unstaged. Let me check git status and what's staged vs committed.
On branch feat/prefixed-tags-fixes Your branch is ahead of 'origin/master' by 1 commit. (use "git push" to publish your local commits) 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: .lore.md modified: docs/src/content/docs/targets/github.md modified: src/commands/__tests__/changelog-versioning-policy.test.ts modified: src/commands/changelog.ts modified: src/utils/__tests__/git.test.ts modified: src/utils/git.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") === STAGED DIFF (git.ts) === === UNSTAGED DIFF (git.ts) === diff --git a/src/utils/git.ts b/src/utils/git.ts index 5b2589b..1a1e04d 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -248,22 +248,17 @@ export async function ...
Now the picture is clear. Fix 2 (the opaque-prefix lastIndexOf rewrite) is UNCOMMITTED — it exists only in the working tree, not in the HEAD commit. The single commit 412b796 contains the OLD segment-count logic. Also the git.test.ts, changelog.ts, changelog test, and github.md docs changes are all uncommitted.
Let me check the full working-tree state of all the files to review the actual current state (which is what matters for the review — "CURRENT state of branch"). Let me read the remaining relevant files.
<path>/home/byk/Code/getsentry/craft/src/utils/git.ts</path>
<type>file</type>
<content>
42: );
43: }
44:
45: export async function getLatestTag(
46: git: SimpleGit,
47: tagPrefix = '',
48: ): Promise<string> {
49: try {
50: // This part is courtesy of https://stackoverflow.com/a/7261049/90297
51: const args = ['describe', '--tags', '--abbrev=0'];
52: if (tagPrefix) {
53: // In a monorepo, tags for multiple products (e.g. `cli@1.2.3`,
54: // `mcp@2.0.0`) are interleaved. `--match '<prefix>*'` scopes
55: // `git describe` to a single product's tag namespace so the latest tag
56: // is resolved per-product instead of picking whatever is newest overall.
57: args.push('--match', `${tagPrefix}*`);
58: }
59: return (await git.raw(args)).trim();
60: } catch (err) {
61: // If there are no tags, return an empty string
62: if (
63: err instanceof Error &&
64: (err.message.startsWith('fatal: No names found') ||
65: ...
Now let me look at parseGitBranchOutput (the HEAD filtering) and read the test files, changelog.ts, and config.ts working-tree state.
<path>/home/byk/Code/getsentry/craft/src/utils/git.ts</path>
<type>file</type>
<content>
198: /**
199: * Parses the output of `git branch -r` into an array of trimmed branch names,
200: * filtering out HEAD pointer entries.
201: */
202: function parseGitBranchOutput(output: string): string[] {
203: return output
204: .split('\n')
205: .map(line => line.trim())
206: .filter(line => line && !line.includes('->'));
207: }
208:
209: /**
210: * Searches remote branches for those matching or similar to a given prefix. ...
parseGitBranchOutput filters out any line containing ->, so origin/HEAD -> origin/main is removed BEFORE the loop. Good — the HEAD-entry edge is handled at parse time. Now let me read the git.test.ts findReleaseBranches block in full (working tree) and the changelog.ts, config.ts working tree.
<path>/home/byk/Code/getsentry/craft/src/utils/__tests__/git.test.ts</path>
<type>file</type>
<content>
175:
176: expect(result.exactMatches).toEqual([
177: 'origin/release/2.0.0',
178: 'origin/release/1.0.0',
179: ]);
180: expect(result.fuzzyMatches).toEqual(['origin/releases/1.0.0']);
181: });
182:
183: it('returns empty results when no branches match', async () => {
184: const git = createMockGit(
185: ' origin/main\n origin/develop\n origin/feature/foo\n',
186: );
187:
188: const result = await findReleaseBranches(git, 'release');
189:
190: expect(result.exactMatches).toEqual([]);
191: expect(result.fuzzyMatches).toEqual([]);
192: });
193:
194: it('filters out HEAD pointer entries', async () => {
195: const git = createMockGit(
196: ' origin/HEAD -> origin/main\n origin/release/1.0.0\n',
197: );
198:
199: const result = await findReleaseBranches(git, 'release');
200:
201: ...
The working-tree test file has EXTRA tests (302+) and updated comments (287, 296-297) that reflect the opaque-prefix logic — these are also uncommitted. Note the committed diff's test at line 287 said "release" alone has too few segments but the working tree says "release/1.0.0" has branch-prefix "release" (distance 4) → excluded. Let me read the rest and the earlier part of the describe block.
<path>/home/byk/Code/getsentry/craft/src/utils/__tests__/git.test.ts</path>
<type>file</type>
<content>
300: });
301:
302: it('treats the prefix opaquely: "release" does not claim "release/cli/x" branches', async () => {
303: // With opaque (last-slash) prefix handling, a slashed product branch
304: // belongs to its full prefix ("release/cli"), not the bare "release".
305: const git = createMockGit(
306: ' origin/release/1.0.0\n origin/release/cli/1.2.3\n',
307: );
308:
309: const result = await findReleaseBranches(git, 'release');
310:
311: // "release/1.0.0" → branch-prefix "release" (exact).
312: expect(result.exactMatches).toEqual(['origin/release/1.0.0']);
313: // "release/cli/1.2.3" → branch-prefix "release/cli" (distance 4) → excluded.
314: expect(result.fuzzyMatches).toEqual([]);
315: });
316: });
(End of file - total 316 lines)
</content>
<path>/home/byk/Code/getsentry/craft/src/utils/__tests__/git.test.ts</path>
<type>file</type>
<content>
120: });
121:
122: it('returns true when there are conflicted files', () => {
123: const status = createCleanStatus();
124: status.conflicted = ['conflict.txt'];
125: expect(isRepoDirty(status)).toBe(true);
126: });
127: });
128:
129: describe('findReleaseBranches', () => {
130: function createMockGit(branchOutput: string, fetchError?: Error) {
131: return {
132: fetch: fetchError
133: ? vi.fn().mockRejectedValue(fetchError)
134: : vi.fn().mockResolvedValue(undefined),
135: raw: vi.fn().mockResolvedValue(branchOutput),
136: } as any;
137: }
138:
139: it('returns exact matches for branches with matching prefix', async () => {
140: const git = createMockGit(
141: ' origin/release/1.2.0\n origin/release/1.2.1\n origin/release/1.2.2\n',
142: );
143:
144: const result = await findReleaseBranches(git, ...
Now I have the full test picture. Let me trace one important regression concern. The old committed logic used branchSegments.length <= prefixSegmentCount to skip; the new uses lastSlash <= 0. Consider test at line 154-167: origin/releases/1.0.0 with prefix release.
prefixSegmentCount = 1, branchSegments = ['releases','1.0.0'], branchPrefix = 'releases', length 2 > 1 → considered. levenshtein('releases','release')=1 → fuzzy. ✓withoutRemote='releases/1.0.0', lastSlash at index 8, branchPrefix='releases' → fuzzy. ✓Now the critical divergence — a branch with MORE segments than the prefix under the OLD logic vs the LAST-slash under the NEW. Consider release/cli/1.2.3 with prefix release (line 302 test):
prefixSegmentCount=1, branchSegments=['release','cli','1.2.3'], branchPrefix='release' (first segment), length 3 > 1 → considered → exact match release.branchPrefix='release/cli' → levenshtein('release/cli','release')=4 → excluded.This is a behavior divergence. Let me verify this is intended and run the verification suite. Let me read changelog.ts and config.ts working tree, then run everything.
<path>/home/byk/Code/getsentry/craft/src/commands/changelog.ts</path>
<type>file</type>
<content>
50: default: 'text',
51: });
52:
53: /**
54: * Body of 'changelog' command
55: */
56: export async function changelogMain(argv: ChangelogOptions): Promise<void> {
57: const git = await getGitClient();
58:
59: // Determine base revision for changelog generation
60: let since = argv.since;
61: if (!since) {
62: // Scope the latest-tag lookup to the configured tag prefix (if any) so
63: // monorepos with interleaved product tags (e.g. `cli@`, `mcp@`) resolve
64: // the correct base. Only read the prefix when a config file is present;
65: // the changelog command can run standalone without one. A broken or
66: // unreadable .craft.yml must not abort a standalone changelog run (which
67: // otherwise needs only git history), so fall back to no prefix on error. ...
The working-tree changelog.ts has the FULL try/catch wrapping both findConfigFile() and getGitTagPrefix() (lines 69-78) with a debug log fallback. This differs from the COMMITTED diff which only had const tagPrefix = findConfigFile() ? getGitTagPrefix() : ''; with NO try/catch — the committed version has the Bugbot Medium bug still present. The fix is uncommitted.
So Fix 1 and Fix 2 are BOTH uncommitted working-tree changes. Now let me run the verification suite against the working tree (which is what "current state" means).
=== tsc exit: 0 ===
> @sentry/craft@2.27.0-dev.0 test /home/byk/Code/getsentry/craft
> vitest run src/utils/__tests__/git.test.ts src/commands/__tests__/changelog-versioning-policy.test.ts src/__tests__/config.test.ts
RUN v4.1.8 /home/byk/Code/getsentry/craft
[debug] Failed to fetch from remote, using locally cached refs
[debug] Failed to list remote branches
Test Files 3 passed (3)
Tests 53 passed (53)
Start at 19:37:17
Duration 2.46s (transform 1.10s, setup 0ms, import 4.50s, tests 1.15s, environment 0ms)
Targeted tests pass. Now the full suite and prettier check.
58 | (checkExecutableIsPresent as MockedFunction<
| ^
59 | typeof checkExecutableIsPresent
60 | >) = vi.fn();
Plugin: vite:esbuild
File: /home/byk/Code/getsentry/craft/src/targets/__tests__/symbolCollector.test.ts
7:38:10 PM [vite] (ssr) warning: This assignment will throw because "checkExecutableIsPresent" is an import
66 |
67 | test('symbol collector present and config ok', () => {
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 1067 passed | 1 skipped (1068)
Start at 19:37:47
Duration 44.09s (transform 4.00s, setup 0ms, import 75.70s, tests 32.02s, environment 14ms)
Full suite green: 1067 passed, 1 skipped. The vite warnings are pre-existing (symbolCollector.test.ts, unrelated). Now prettier check on changed files.
Checking formatting... [warn] src/config.ts [warn] Code style issues found in the above file. Run Prettier with --write to fix.
A prettier violation in src/config.ts. Note docs/ is in .prettierignore so those warnings wouldn't matter for CI, but let me confirm the config.ts issue is a real CI failure (CI runs prettier --check ., and config.ts is not ignored). Let me see the exact diff prettier wants.
342,343c342 < const firstPrefix = < (githubTargets[0]?.tagPrefix as string | undefined) || ''; --- > const firstPrefix = (githubTargets[0]?.tagPrefix as string | undefined) || '';
Confirmed: src/config.ts:342-343 fails prettier --check. CI's "Lint fixes" job runs pnpm format:check (prettier --check .) and this will fail. This is exactly the trap noted in the project lore (pnpm lint doesn't catch it). Let me verify config.ts line context and check the docs are indeed prettier-ignored.
coverage/ dist/ node_modules/ pnpm-lock.yaml temp_*/ docs/ # Auto-generated from PR descriptions by the release tooling. # Its contents carry markdown that does not always conform to prettier's # preferences (e.g. literal `_*` sequences from release-note titles that # prettier wants escaped to `\*`). Excluding avoids re-formatting on every # release cut. CHANGELOG.md # Auto-managed by lore (https://github.com/BYK/loreai) — formatting is # controlled by the lore daemon, not prettier. AGENTS.md === format:check script === prettier --check .
Confirmed: docs/ is ignored (so docs formatting is irrelevant to CI), but src/config.ts is NOT ignored and format:check = prettier --check ., so CI will fail on config.ts.
Now let me finalize my regression analysis by enumerating the divergence cases between old committed logic and new working-tree logic. Let me trace the config.ts getGitTagPrefix working tree to confirm the conflict-detection edge (empty-string first prefix + all-empty targets).
<path>/home/byk/Code/getsentry/craft/src/config.ts</path>
<type>file</type>
<content>
336: * multiple `github` targets with *differing* prefixes, the configuration is
337: * ambiguous: the first prefix is returned and a warning is emitted.
338: */
339: export function getGitTagPrefix(): string {
340: const targets = getConfiguration().targets || [];
341: const githubTargets = targets.filter(target => target.name === 'github');
342: const firstPrefix =
343: (githubTargets[0]?.tagPrefix as string | undefined) || '';
344:
345: const hasConflictingPrefix = githubTargets.some(
346: target => ((target.tagPrefix as string | undefined) || '') !== firstPrefix,
347: );
348: if (hasConflictingPrefix) {
349: logger.warn(
350: 'Multiple "github" targets with different "tagPrefix" values found. ' +
351: `Using "${firstPrefix}". For independently-versioned products in a ` +
352: 'monorepo, use a separate .craft.yml per product, each with a single ' ...
Now let me verify the version.ts regex handles the prefixed-tag extraction claims and confirm the getVersion/parseVersion behavior for sentry-cli@10.20.30 (prefix ending in a digit-adjacent). Let me check the regex.
<path>/home/byk/Code/getsentry/craft/src/utils/version.ts</path>
<type>file</type>
<content>
1: import { getGitTagPrefix } from '../config';
2:
3: /**
4: * Regular expression for matching semver versions.
5: *
6: * Modified to match version components
7: * Copyright (c) Sindre Sorhus <sindresorhus@gmail.com> (sindresorhus.com)
8: * @see https://github.com/sindresorhus/semver-regex
9: */
10: const semverRegex = () =>
11: /\bv?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(?:-?([\da-z-]+(?:\.[\da-z-]+)*))?(?:\+([\da-z-]+(?:\.[\da-z-]+)*))?\b/gi;
12:
13: /**
14: * Extracts a version number from the given text.
15: *
16: * In case the version contains a leading "v", it is stripped from the result.
17: * All semantic versions are supported. See {@link http://semver.org/} for
18: * more information. ...
I want to sanity-check one subtle claim in the version test: getVersion('sentry-cli@10.20.30') → 10.20.30. The regex has \b word boundary before v?. In sentry-cli@10.20.30, the @ before 10 — is there a \b between @ and 1? Yes (@ is non-word, 1 is word → boundary). But also cli@10... — could the regex match starting earlier? Let me empirically verify the edge cases rather than reason about it, including a nasty one: a prefix containing a number like foo2@1.2.3.
"cli@1.2.3" => "1.2.3" "mcp@2.0.0-dev.1" => "2.0.0-dev.1" "sentry-cli@10.20.30" => "10.20.30" "cli@v1.2.3" => "1.2.3" "foo2@1.2.3" => "1.2.3" "v2@1.2.3" => "1.2.3" "2.0@1.2.3" => "1.2.3" "abc1.2.3def" => null
Interesting edge: 2.0@1.2.3 → 1.2.3 (correct — the regex requires three dotted components, so 2.0 alone doesn't match, it finds 1.2.3). The version extraction is robust. Note foo2@1.2.3 correctly yields 1.2.3 not 2.1.2 — good, because 2 isn't followed by two more dotted numbers at that position. The version.ts logic is solid.
Now let me confirm the one remaining concern in the divergence analysis empirically — trace old vs new logic on the same input set to enumerate exact differences.
same ["origin/release/1.2.3"] prefix="release"
DIFF ["origin/release/cli/1.2.3"] prefix="release"
OLD: {"ex":["origin/release/cli/1.2.3"],"fz":[]}
NEW: {"ex":[],"fz":[]}
same ["origin/release/cli/1.2.3"] prefix="release/cli"
same ["origin/releases/1.0.0"] prefix="release"
DIFF ["origin/release/1.0.0","origin/release/cli/1.2.3"] prefix="release"
OLD: {"ex":["origin/release/1.0.0","origin/release/cli/1.2.3"],"fz":[]}
NEW: {"ex":["origin/release/1.0.0"],"fz":[]}
same ["origin/foo/"] prefix="foo"
same ["origin/release/mcp/2.0.0"] prefix="release/cli"
same ["origin/prod/staging/1.0.0"] prefix="release"
same ["origin/a/b/c/1.0.0"] prefix="release"
Empirically confirmed. The behavior divergence between old (committed) and new (working-tree) logic is exactly ONE class of case: when a branch has MORE path segments than the prefix (e.g. release/cli/1.2.3 with a bare release prefix).
release/cli/1.2.3 matched bare release exactly.release/cli → distance 4 from release → excluded.This is the intended behavior change (BYK's review comment: prefix is opaque; a bare release release run should NOT claim another product's release/cli/x branches). It's a correctness improvement, not a regression. The foo/ trailing-slash case: lastSlash points at trailing slash, branchPrefix='foo', matches prefix foo exactly — but this is a degenerate branch that shouldn't exist; both old and new treat it identically (old: bs=['foo',''], length 2 > 1, bp='foo' → match). Consistent.
Let me verify the trailing-slash + empty-version concern is truly benign and the lastSlash <= 0 guard on leading-slash. Already traced above: foo/ matches (empty version), but this is harmless because such a branch can't be checked out to a real version and won't exist in practice. Let me now write up findings.
I have everything I need. Here's my review.
feat/prefixed-tags-fixes (PR A)The two review fixes are NOT committed. The branch has exactly one commit (412b796), and git diff origin/master...HEAD shows the OLD segment-count logic for findReleaseBranches and the un-try/catch'd changelog.ts. Both "Fix 1" and "Fix 2" — plus the updated git.test.ts, changelog-versioning-policy.test.ts, and github.md — exist only as unstaged working-tree changes:
Changes not staged for commit:
modified: docs/src/content/docs/targets/github.md
modified: ...
Date: July 21, 2026 * 🟡 (19:41) [tool:result] Prettier format check surfaced a violation: `src/config.ts` fails `pnpm exec prettier --check`, with message "Code style issues found in the above file. Run Prettier with --write to fix." * 🟡 (19:41) Assistant noted `docs/` is listed in `.prettierignore` (so docs warnings don't matter for CI) but flagged that `src/config.ts` is NOT ignored, so need…
Date: July 21, 2026 * 🟡 (19:39) [tool:result] Full `pnpm test` suite run (verification run 3) → 58 test files passed (58), 1067 tests passed, 1 skipped (1068 total), Duration 44.09s (transform 4.00s, import 75.70s, tests 32.02s). Debug logs: "Failed to fetch from remote, using locally cached refs", "Failed to list remote branches" (expected mock behavior). * 🟡 (19:39) [tool:result] Vite/esbuil…
Date: July 21, 2026 * 🟡 (19:30) User asked for an adversarial senior-code-reviewer pass on getsentry/craft branch `feat/prefixed-tags-fixes` vs `origin/master`, called "PR A" — salvageable subset of parked PR #844 (monorepo prefixed-tag support) plus two review fixes. Explicit constraint: do NOT modify files, do NOT rubber-stamp. * 🔴 (19:30) User stated PR A contents: (1) prefix-aware read pat…