Skip to content

Commit dec94db

Browse files
backnotpropbacknotprop
committed
fix(review): tighten jj defaults and detection
1 parent b166413 commit dec94db

12 files changed

Lines changed: 128 additions & 30 deletions

File tree

‎apps/pi-extension/server.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,7 @@ describe("pi review server", () => {
519519
});
520520
expect(prepared.gitContext.vcsType).toBe("jj");
521521
expect(prepared.diffType).toBe("jj-current");
522-
expect(prepared.base).toBe("trunk()");
522+
expect(prepared.base).toBe("main@git");
523523

524524
const forcedGit = await prepareLocalReviewDiff({
525525
cwd: repoDir,
@@ -578,7 +578,7 @@ describe("pi review server", () => {
578578
gitContext?: { vcsType?: string; diffOptions: Array<{ id: string }> };
579579
};
580580
expect(initial.diffType).toBe("jj-current");
581-
expect(initial.base).toBe("trunk()");
581+
expect(initial.base).toBe("main@git");
582582
expect(initial.gitContext?.vcsType).toBe("jj");
583583
expect(initial.gitContext?.diffOptions.map((option) => option.id)).toEqual([
584584
"jj-current",

‎apps/pi-extension/server/serverReview.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,9 @@ export async function startReviewServer(options: {
225225
// read this (not gitContext.defaultBranch) so they analyze the same diff
226226
// the reviewer is currently looking at. Honors an explicit initialBase from
227227
// the caller — e.g. programmatic Pi callers can request a non-detected base.
228-
let currentBase = options.initialBase || options.gitContext?.defaultBranch || "main";
228+
const detectedCompareTarget = (): string =>
229+
options.gitContext?.defaultBranch || options.gitContext?.compareTarget?.fallback || "main";
230+
let currentBase = options.initialBase || detectedCompareTarget();
229231
let baseEverSwitched = false;
230232

231233
// Fire-and-forget: query the remote for its actual default branch.
@@ -585,7 +587,7 @@ export async function startReviewServer(options: {
585587
if (typeof body.hideWhitespace === "boolean") {
586588
currentHideWhitespace = body.hideWhitespace;
587589
}
588-
const detectedBase = options.gitContext?.defaultBranch || "main";
590+
const detectedBase = detectedCompareTarget();
589591
const base = resolveBaseBranch(
590592
typeof body.base === "string" ? body.base : undefined,
591593
detectedBase,
@@ -921,7 +923,7 @@ export async function startReviewServer(options: {
921923

922924
// Local mode first (matches Bun server priority)
923925
if (hasLocalAccess && !isPRMode) {
924-
const detectedBase = options.gitContext?.defaultBranch || "main";
926+
const detectedBase = detectedCompareTarget();
925927
const base = resolveBaseBranch(
926928
url.searchParams.get("base") ?? undefined,
927929
detectedBase,

‎packages/server/agent-review-message.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ describe("buildClaudeCommand", () => {
8383
expect(allowedTools).toContain("Bash(jj diff:*)");
8484
expect(allowedTools).toContain("Bash(jj log:*)");
8585
expect(allowedTools).toContain("Bash(jj show:*)");
86+
expect(allowedTools).toContain("Bash(jj file show:*)");
87+
expect(allowedTools).toContain("Bash(jj cat:*)");
8688
expect(allowedTools).toContain("Bash(jj bookmark list:*)");
8789
});
8890
});

‎packages/server/claude-review.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,8 @@ export function buildClaudeCommand(prompt: string, model: string = "claude-opus-
208208
"Bash(git show-ref:*)",
209209
// JJ (read-only)
210210
"Bash(jj status:*)", "Bash(jj diff:*)", "Bash(jj log:*)",
211-
"Bash(jj show:*)", "Bash(jj bookmark list:*)",
211+
"Bash(jj show:*)", "Bash(jj file show:*)", "Bash(jj cat:*)",
212+
"Bash(jj bookmark list:*)",
212213
"Bash(wc:*)",
213214
].join(",");
214215

‎packages/server/jj.test.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,18 +41,22 @@ describe("jj diff args", () => {
4141
});
4242

4343
describe("jj compare targets", () => {
44-
test("defaults to jj trunk revset instead of guessing from bookmark names", () => {
44+
test("prefers available default bookmarks before falling back to trunk", () => {
4545
expect(selectDefaultJjCompareTarget({
4646
local: ["main"],
4747
remote: ["main@origin"],
48-
})).toBe("trunk()");
48+
})).toBe("main@origin");
4949
expect(selectDefaultJjCompareTarget({
50-
local: ["develop", "feature"],
51-
remote: ["release@origin"],
52-
})).toBe("trunk()");
50+
local: ["main"],
51+
remote: ["main@git"],
52+
})).toBe("main@git");
53+
expect(selectDefaultJjCompareTarget({
54+
local: ["main"],
55+
remote: [],
56+
})).toBe("main");
5357
expect(selectDefaultJjCompareTarget({
5458
local: ["release"],
55-
remote: ["production@origin"],
59+
remote: ["production@git"],
5660
})).toBe("trunk()");
5761
});
5862

‎packages/server/review.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,10 @@ export async function startReviewServer(
422422
? { display: getDisplayRepo(prMetadata), branch: `${getMRLabel(prMetadata)} ${getMRNumberLabel(prMetadata)}` }
423423
: await getRepoInfo();
424424
if (gitContext?.repository?.displayFallback) {
425-
repoInfo = { display: repoInfo?.display || gitContext.repository.displayFallback };
425+
repoInfo = {
426+
...repoInfo,
427+
display: repoInfo?.display || gitContext.repository.displayFallback,
428+
};
426429
}
427430

428431
// Fetch current platform user (for own-PR/MR detection)

‎packages/server/tour/tour-review.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ describe("buildTourClaudeCommand", () => {
138138
expect(allowedTools).toContain("Bash(jj diff:*)");
139139
expect(allowedTools).toContain("Bash(jj log:*)");
140140
expect(allowedTools).toContain("Bash(jj show:*)");
141+
expect(allowedTools).toContain("Bash(jj file show:*)");
142+
expect(allowedTools).toContain("Bash(jj cat:*)");
141143
expect(allowedTools).toContain("Bash(jj bookmark list:*)");
142144
});
143145
});

‎packages/server/tour/tour-review.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,8 @@ export function buildTourClaudeCommand(prompt: string, model: string = "sonnet",
346346
"Bash(git merge-base:*)", "Bash(git remote:*)", "Bash(git rev-parse:*)",
347347
"Bash(git show-ref:*)",
348348
"Bash(jj status:*)", "Bash(jj diff:*)", "Bash(jj log:*)",
349-
"Bash(jj show:*)", "Bash(jj bookmark list:*)",
349+
"Bash(jj show:*)", "Bash(jj file show:*)", "Bash(jj cat:*)",
350+
"Bash(jj bookmark list:*)",
350351
"Bash(gh pr view:*)", "Bash(gh pr diff:*)", "Bash(gh pr list:*)",
351352
"Bash(gh api repos/*/*/pulls/*)", "Bash(gh api repos/*/*/pulls/*/files*)",
352353
// The tour prompt follows linked issues (`Fixes #123`, `Closes owner/repo#456`),

‎packages/shared/jj-core.test.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,22 @@ describe("jj diff args", () => {
4343
});
4444

4545
describe("jj compare targets", () => {
46-
test("defaults to jj trunk revset instead of guessing from bookmark names", () => {
46+
test("prefers available default bookmarks before falling back to trunk", () => {
4747
expect(selectDefaultJjCompareTarget({
4848
local: ["main"],
4949
remote: ["main@origin"],
50-
})).toBe("trunk()");
50+
})).toBe("main@origin");
5151
expect(selectDefaultJjCompareTarget({
52-
local: ["develop", "feature"],
53-
remote: ["release@origin"],
54-
})).toBe("trunk()");
52+
local: ["main"],
53+
remote: ["main@git"],
54+
})).toBe("main@git");
55+
expect(selectDefaultJjCompareTarget({
56+
local: ["main"],
57+
remote: [],
58+
})).toBe("main");
5559
expect(selectDefaultJjCompareTarget({
5660
local: ["release"],
57-
remote: ["production@origin"],
61+
remote: ["production@git"],
5862
})).toBe("trunk()");
5963
});
6064

‎packages/shared/jj-core.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,22 @@ export function getJjDiffArgs(
159159
}
160160
}
161161

162-
export function selectDefaultJjCompareTarget(_targets: { local: string[]; remote: string[] }): string {
163-
// `trunk()` honors JJ's repository default bookmark/remote and user aliases;
164-
// bookmarks are still listed so users can explicitly pick a different base.
162+
export function selectDefaultJjCompareTarget(targets: { local: string[]; remote: string[] }): string {
163+
const preferredNames = ["main", "develop", "master", "trunk"];
164+
for (const name of preferredNames) {
165+
const preferredRemote = [
166+
`${name}@origin`,
167+
`${name}@upstream`,
168+
`${name}@git`,
169+
...targets.remote.filter((target) => target.startsWith(`${name}@`)).sort(),
170+
].find((target) => targets.remote.includes(target));
171+
if (preferredRemote) return preferredRemote;
172+
173+
if (targets.local.includes(name)) return name;
174+
}
175+
176+
// `trunk()` honors JJ's repository default bookmark/remote and user aliases
177+
// when the repository has no obvious default bookmark to compare against.
165178
return JJ_TRUNK_REVSET;
166179
}
167180

0 commit comments

Comments
 (0)