Address PR review feedback (#1461)
- pr-nodes: forward post-increment entity to deps.respond (responseRounds off-by-one); branch-group entity fallback now keys on task.branchContext.groupId, not task.id - core: hoist shared autoMergeGateReason into @fusion/core pr-entity.ts; CLI + dashboard import the single definition (R13) - cli: fn pr create also writes the unified PR entity (ensure → open), keeping legacy prInfo; resolveBranchHeadOid uses argv-safe execFileAsync (injection fix) - changeset: fn-pr-commands bumped minor → major (breaking CLI removal per AGENTS.md) - tests: bin.test.ts daemon fixture off reserved port 4040; lazy-loaded-views title 19 → 20 views Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -805,10 +805,10 @@ describe("bin command routing and fallbacks", () => {
|
||||
});
|
||||
|
||||
it("routes daemon command with all flags", async () => {
|
||||
await runBin(["daemon", "--port", "4040", "--host", "127.0.0.1", "--token", "fn_abc123", "--paused", "--token-only"]);
|
||||
await runBin(["daemon", "--port", "5055", "--host", "127.0.0.1", "--token", "fn_abc123", "--paused", "--token-only"]);
|
||||
|
||||
expect(commandMocks.runDaemon).toHaveBeenCalledWith({
|
||||
port: 4040,
|
||||
port: 5055,
|
||||
paused: true,
|
||||
interactive: false,
|
||||
host: "127.0.0.1",
|
||||
|
||||
@@ -16,15 +16,31 @@ vi.mock("@fusion/engine", () => ({
|
||||
releaseHeldTaskByEvent: (...args: unknown[]) => releaseHeldTaskByEvent(...args),
|
||||
}));
|
||||
|
||||
// @fusion/dashboard is only touched by runPrCreate (not exercised here); stub it
|
||||
// so importing the module never pulls the heavy dashboard graph.
|
||||
// @fusion/dashboard is touched by runPrCreate; stub it so importing the module
|
||||
// never pulls the heavy dashboard graph. `createPr` is controllable so the create
|
||||
// path can be asserted to write the unified PR entity.
|
||||
const createPr = vi.fn();
|
||||
vi.mock("@fusion/dashboard", () => ({
|
||||
GitHubClient: class {},
|
||||
GitHubClient: class {
|
||||
createPr(...args: unknown[]) {
|
||||
return createPr(...args);
|
||||
}
|
||||
},
|
||||
generatePrMetadata: vi.fn(),
|
||||
}));
|
||||
|
||||
// gh-cli helpers used by runPrCreate (repo resolution + auth gating).
|
||||
vi.mock("@fusion/core/gh-cli", () => ({
|
||||
classifyGhError: vi.fn(() => ({ message: "err" })),
|
||||
getGhErrorMessage: vi.fn(() => "err"),
|
||||
getCurrentRepo: vi.fn(() => ({ owner: "owner", repo: "repo" })),
|
||||
isGhAuthenticated: vi.fn(() => true),
|
||||
isGhAvailable: vi.fn(() => true),
|
||||
}));
|
||||
|
||||
const { resolveProject } = await import("../project-context.js");
|
||||
const {
|
||||
runPrCreate,
|
||||
runPrList,
|
||||
runPrShow,
|
||||
runPrApprove,
|
||||
@@ -86,6 +102,54 @@ describe("fn pr commands", () => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
// ── create → legacy prInfo + unified PR entity ──────────────────────────────
|
||||
|
||||
it("runPrCreate writes the unified PR entity (not just legacy prInfo)", async () => {
|
||||
const prInfo = {
|
||||
url: "https://github.com/owner/repo/pull/7",
|
||||
number: 7,
|
||||
status: "open",
|
||||
title: "T",
|
||||
headBranch: "fusion/fn-001",
|
||||
baseBranch: "main",
|
||||
commentCount: 0,
|
||||
};
|
||||
delete process.env.GITHUB_REPOSITORY;
|
||||
createPr.mockResolvedValue(prInfo);
|
||||
const getTask = vi.fn().mockResolvedValue({
|
||||
id: "FN-001",
|
||||
title: "Task one",
|
||||
description: "do a thing",
|
||||
column: "in-review",
|
||||
prInfo: undefined,
|
||||
});
|
||||
const updatePrInfo = vi.fn();
|
||||
const ensurePrEntityForSource = vi.fn().mockReturnValue(makeEntity({ id: "PR-NEW", state: "creating" }));
|
||||
const updatePrEntity = vi.fn().mockReturnValue(makeEntity({ id: "PR-NEW" }));
|
||||
const logEntry = vi.fn();
|
||||
mockStore({ getTask, updatePrInfo, ensurePrEntityForSource, updatePrEntity, logEntry });
|
||||
|
||||
await runPrCreate("FN-001", { ai: false });
|
||||
|
||||
// Legacy field is still written (additive, migration-safe).
|
||||
expect(updatePrInfo).toHaveBeenCalledWith("FN-001", prInfo);
|
||||
// Unified entity is created via the same store path the pr-create node uses.
|
||||
expect(ensurePrEntityForSource).toHaveBeenCalledWith({
|
||||
sourceType: "task",
|
||||
sourceId: "FN-001",
|
||||
repo: "owner/repo",
|
||||
headBranch: "fusion/fn-001",
|
||||
baseBranch: "main",
|
||||
state: "creating",
|
||||
});
|
||||
// …then flipped to open with the persisted PR number/url.
|
||||
expect(updatePrEntity).toHaveBeenCalledWith("PR-NEW", {
|
||||
state: "open",
|
||||
prNumber: 7,
|
||||
prUrl: "https://github.com/owner/repo/pull/7",
|
||||
});
|
||||
});
|
||||
|
||||
// ── read commands ──────────────────────────────────────────────────────────
|
||||
|
||||
it("runPrList reads active entities from the store", async () => {
|
||||
|
||||
@@ -2,7 +2,7 @@ import {
|
||||
TaskStore,
|
||||
isPrEntityActive,
|
||||
isPrEntityActionable,
|
||||
isPrEntityAutoMergeReady,
|
||||
autoMergeGateReason,
|
||||
type PrEntity,
|
||||
type PrThreadState,
|
||||
} from "@fusion/core";
|
||||
@@ -199,8 +199,27 @@ export async function runPrCreate(id: string, options: PrCreateOptions = {}, pro
|
||||
reviewers: options.reviewers,
|
||||
});
|
||||
|
||||
// Store PR info
|
||||
// Store PR info (legacy field, still read by some surfaces during migration).
|
||||
await store.updatePrInfo(task.id, prInfo);
|
||||
|
||||
// Also write the unified PR entity via the SAME store path the pr-create
|
||||
// workflow node uses (mirrors pr-nodes.ts: ensure → flip to open with the
|
||||
// persisted PR number/url). Without this the PR would be invisible to
|
||||
// `fn pr list/show`, the reconciler, and the workflow nodes (R13 parity).
|
||||
const entity = store.ensurePrEntityForSource({
|
||||
sourceType: "task",
|
||||
sourceId: task.id,
|
||||
repo: `${owner}/${repo}`,
|
||||
headBranch: branchName,
|
||||
baseBranch: prInfo.baseBranch,
|
||||
state: "creating",
|
||||
});
|
||||
store.updatePrEntity(entity.id, {
|
||||
state: "open",
|
||||
prNumber: prInfo.number,
|
||||
prUrl: prInfo.url,
|
||||
});
|
||||
|
||||
await store.logEntry(task.id, "Created PR", `PR #${prInfo.number}: ${prInfo.url}`);
|
||||
|
||||
console.log();
|
||||
@@ -235,16 +254,6 @@ function requireEntity(store: TaskStore, id: string): PrEntity {
|
||||
return entity;
|
||||
}
|
||||
|
||||
function autoMergeReason(entity: PrEntity): string {
|
||||
if (!entity.autoMerge) return "Auto-merge off";
|
||||
if (entity.mergeable === "conflicting") return "Blocked: conflict";
|
||||
if (entity.reviewDecision !== "APPROVED") return "Waiting for approval";
|
||||
if (entity.checksRollup !== "success") return "Waiting for checks";
|
||||
if (entity.mergeable !== "clean") return "Waiting for checks";
|
||||
if (isPrEntityAutoMergeReady(entity)) return "Ready to merge";
|
||||
return "Waiting for checks";
|
||||
}
|
||||
|
||||
export async function runPrList(projectName?: string) {
|
||||
const { store } = await getPrContext(projectName);
|
||||
const entities = store.listActivePrEntities();
|
||||
@@ -284,7 +293,7 @@ export async function runPrShow(id: string, projectName?: string) {
|
||||
console.log(` Mergeable: ${entity.mergeable ?? "unknown"}`);
|
||||
console.log(` Review: ${entity.reviewDecision ?? "none"}`);
|
||||
console.log(` Checks: ${entity.checksRollup ?? "none"}`);
|
||||
console.log(` Auto-merge: ${entity.autoMerge ? "on" : "off"} (${autoMergeReason(entity)})`);
|
||||
console.log(` Auto-merge: ${entity.autoMerge ? "on" : "off"} (${autoMergeGateReason(entity)})`);
|
||||
console.log(` Active: ${isPrEntityActive(entity) ? "yes" : "no"}; actionable: ${isPrEntityActionable(entity) ? "yes" : "no"}`);
|
||||
console.log(` Rounds: ${entity.responseRounds}; threads: ${threads.length} (${pending} pending, ${disagreed} disagreed)`);
|
||||
console.log();
|
||||
@@ -363,5 +372,5 @@ export async function runPrAutomerge(id: string, enabled: boolean | undefined, p
|
||||
|
||||
const next = typeof enabled === "boolean" ? enabled : !entity.autoMerge;
|
||||
const updated = store.updatePrEntity(id, { autoMerge: next });
|
||||
console.log(`\n ✓ Auto-merge ${updated.autoMerge ? "enabled" : "disabled"} for ${id} (${autoMergeReason(updated)})\n`);
|
||||
console.log(`\n ✓ Auto-merge ${updated.autoMerge ? "enabled" : "disabled"} for ${id} (${autoMergeGateReason(updated)})\n`);
|
||||
}
|
||||
|
||||
@@ -340,7 +340,7 @@ export function syncGroupPrCallback(
|
||||
* without the stale-head guard, which the reconcile still corroborates. */
|
||||
async function resolveBranchHeadOid(cwd: string, branch: string): Promise<string | undefined> {
|
||||
try {
|
||||
const { stdout } = await execAsync(`git rev-parse "${branch}"`, { cwd, timeout: 30_000 });
|
||||
const { stdout } = await execFileAsync("git", ["rev-parse", branch], { cwd, timeout: 30_000 });
|
||||
const oid = stdout.trim();
|
||||
return oid.length > 0 ? oid : undefined;
|
||||
} catch {
|
||||
|
||||
Reference in New Issue
Block a user