fix(prd-7): M1 architecture review — consolidate duplication, fix bugs, memoize detection
- F1: Consolidated updateForgejoCommentBody into unified updateCommentBody (~150 lines removed) - F2: Fixed wrong run identifier in update-with-branch.ts (context.runId → getForgejoRunIndex()) - F3: Memoized detectPlatform() with _resetPlatformCache() for tests - F4: Added getServerUrl() runtime helper to resolve constant-vs-function inconsistency - F5: Fixed sticky comment bot matching on Forgejo via context.inputs.botName - F6: Documented GHES heuristic misidentification in docs/forgejo.md - Updated PRD M1 checkbox to complete Progress: 17% complete — M1 done, M2 (error handling review) next Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
1 parent
ae326659b1
commit
ac58fb6c14
15 files changed
+151
-266
No files matched your search
@@ -2,6 +2,7 @@
|
||||
|
||||
import { describe, test, expect, beforeEach, afterEach } from "bun:test";
|
||||
import { checkHumanActor } from "../src/github/validation/actor";
|
||||
import { _resetPlatformCache } from "../src/platform/detector";
|
||||
import type { Octokit } from "@octokit/rest";
|
||||
import { createMockContext } from "./mockContext";
|
||||
|
||||
@@ -116,6 +117,7 @@ describe("checkHumanActor", () => {
|
||||
let savedServerUrl: string | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
_resetPlatformCache();
|
||||
savedPlatform = process.env.PLATFORM;
|
||||
savedServerUrl = process.env.GITHUB_SERVER_URL;
|
||||
process.env.PLATFORM = "forgejo";
|
||||
@@ -127,6 +129,7 @@ describe("checkHumanActor", () => {
|
||||
if (savedServerUrl !== undefined)
|
||||
process.env.GITHUB_SERVER_URL = savedServerUrl;
|
||||
else delete process.env.GITHUB_SERVER_URL;
|
||||
_resetPlatformCache();
|
||||
});
|
||||
|
||||
test("should skip actor check entirely on Forgejo", async () => {
|
||||
|
||||
@@ -6,10 +6,12 @@ import {
|
||||
createForgejoBranchLink,
|
||||
createForgejoPrCompareUrl,
|
||||
createForgejoCommentBody,
|
||||
extractOwnerRepoFromUrl,
|
||||
updateForgejoCommentBody,
|
||||
type CommentUpdateInput,
|
||||
} from "../src/forgejo/comment-logic";
|
||||
import {
|
||||
extractOwnerRepoFromUrl,
|
||||
updateCommentBody,
|
||||
type CommentUpdateInput,
|
||||
} from "../src/github/operations/comment-logic";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Test helpers
|
||||
@@ -307,10 +309,10 @@ describe("extractOwnerRepoFromUrl", () => {
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// updateForgejoCommentBody
|
||||
// updateCommentBody
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("updateForgejoCommentBody", () => {
|
||||
describe("updateCommentBody", () => {
|
||||
const forgejoBaseInput: CommentUpdateInput = {
|
||||
currentBody: "Initial comment body",
|
||||
actionFailed: false,
|
||||
@@ -332,7 +334,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "testuser",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("**Claude finished @testuser's task**");
|
||||
expect(result).not.toContain("Claude Code is working");
|
||||
expect(result).not.toContain("<img");
|
||||
@@ -345,7 +347,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "testuser",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("**Claude finished @testuser's task**");
|
||||
expect(result).not.toContain("Claude Code is working");
|
||||
expect(result).not.toContain("⏳");
|
||||
@@ -359,7 +361,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "trigger-user",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(
|
||||
"**Claude finished @trigger-user's task in 1m 14s**",
|
||||
);
|
||||
@@ -373,7 +375,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
executionDetails: { duration_ms: 45000 },
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("**Claude encountered an error after 45s**");
|
||||
});
|
||||
|
||||
@@ -386,7 +388,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
errorDetails: "Failed to fetch issue data",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("**Claude encountered an error after 45s**");
|
||||
expect(result).toContain("[View job]");
|
||||
expect(result).toContain("```\nFailed to fetch issue data\n```");
|
||||
@@ -400,7 +402,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
currentBody: "Some comment",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(`—— [View job](${forgejoBaseInput.jobUrl})`);
|
||||
});
|
||||
});
|
||||
@@ -412,7 +414,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
branchName: "claude/issue-5-20240101-1200",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
// This is the critical fix: GitHub's version uses a hardcoded github.com regex
|
||||
// which fails for Forgejo URLs. Our version uses extractOwnerRepoFromUrl.
|
||||
expect(result).toContain(
|
||||
@@ -427,7 +429,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
branchName: "feature-branch",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("• `feature-branch`");
|
||||
expect(result).not.toContain("[`feature-branch`]");
|
||||
});
|
||||
@@ -440,7 +442,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
"\n[View branch](https://git.wxs.ro/wxs/myrepo/tree/feature-branch)",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(
|
||||
"• [`feature-branch`](https://git.wxs.ro/wxs/myrepo/tree/feature-branch)",
|
||||
);
|
||||
@@ -453,7 +455,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
"\n[View branch](https://git.wxs.ro/wxs/myrepo/tree/auto-detected-branch)",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(
|
||||
"• [`auto-detected-branch`](https://git.wxs.ro/wxs/myrepo/tree/auto-detected-branch)",
|
||||
);
|
||||
@@ -468,7 +470,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
"\n[Create a PR](https://git.wxs.ro/wxs/myrepo/compare/main...feature)",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(
|
||||
"• [Create PR ➔](https://git.wxs.ro/wxs/myrepo/compare/main...feature)",
|
||||
);
|
||||
@@ -481,7 +483,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
"Some comment with [Create a PR](https://git.wxs.ro/wxs/myrepo/compare/main...fix)",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(
|
||||
"• [Create PR ➔](https://git.wxs.ro/wxs/myrepo/compare/main...fix)",
|
||||
);
|
||||
@@ -496,7 +498,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
currentBody: `Done!\n\n[Create a PR](${forgejoUrl})`,
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(`• [Create PR ➔](${forgejoUrl})`);
|
||||
});
|
||||
});
|
||||
@@ -509,7 +511,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "testuser",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain(
|
||||
"**Claude finished @testuser's task in 1m 15s**",
|
||||
);
|
||||
@@ -522,7 +524,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "testuser",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("**Claude finished @testuser's task in 31s**");
|
||||
});
|
||||
|
||||
@@ -533,7 +535,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "testuser",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).toContain("**Claude finished @testuser's task**");
|
||||
expect(result).not.toContain(" in ");
|
||||
});
|
||||
@@ -553,7 +555,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "trigger-user",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
|
||||
// Check header
|
||||
expect(result).toContain(
|
||||
@@ -594,7 +596,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
prLink: "",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
|
||||
expect(result).toContain("Claude finished @claude's task in 1m 30s");
|
||||
expect(result).toContain(`[View job](${forgejoBaseInput.jobUrl})`);
|
||||
@@ -612,7 +614,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "user",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
expect(result).not.toContain("[View job run]");
|
||||
expect(result).toContain("[View job]");
|
||||
});
|
||||
@@ -626,7 +628,7 @@ describe("updateForgejoCommentBody", () => {
|
||||
triggerUsername: "user",
|
||||
};
|
||||
|
||||
const result = updateForgejoCommentBody(input);
|
||||
const result = updateCommentBody(input);
|
||||
// Old link in body removed
|
||||
expect(result).not.toContain("[View branch]");
|
||||
// New branch in header
|
||||
|
||||
@@ -3,6 +3,7 @@ import { prepareMcpConfig } from "../src/mcp/install-mcp-server";
|
||||
import * as core from "@actions/core";
|
||||
import type { ParsedGitHubContext } from "../src/github/context";
|
||||
import { CLAUDE_APP_BOT_ID, CLAUDE_BOT_LOGIN } from "../src/github/constants";
|
||||
import { _resetPlatformCache } from "../src/platform/detector";
|
||||
|
||||
describe("prepareMcpConfig", () => {
|
||||
let consoleInfoSpy: any;
|
||||
@@ -321,6 +322,7 @@ describe("prepareMcpConfig", () => {
|
||||
let savedServerUrl: string | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
_resetPlatformCache();
|
||||
savedPlatform = process.env.PLATFORM;
|
||||
savedForgejo = process.env.FORGEJO;
|
||||
savedServerUrl = process.env.GITHUB_SERVER_URL;
|
||||
@@ -344,6 +346,7 @@ describe("prepareMcpConfig", () => {
|
||||
} else {
|
||||
delete process.env.GITHUB_SERVER_URL;
|
||||
}
|
||||
_resetPlatformCache();
|
||||
});
|
||||
|
||||
test("should skip GitHub public MCP server on Forgejo", async () => {
|
||||
|
||||
@@ -9,7 +9,7 @@
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from "bun:test";
|
||||
import { detectPlatform } from "../src/platform/detector";
|
||||
import { detectPlatform, _resetPlatformCache } from "../src/platform/detector";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Helpers
|
||||
@@ -40,11 +40,13 @@ function restoreEnv() {
|
||||
|
||||
describe("M7: Platform routing for mode handlers", () => {
|
||||
beforeEach(() => {
|
||||
_resetPlatformCache();
|
||||
saveEnv("PLATFORM", "FORGEJO", "GITHUB_SERVER_URL");
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
restoreEnv();
|
||||
_resetPlatformCache();
|
||||
});
|
||||
|
||||
describe("detectPlatform() — used by tag mode and update-comment-link", () => {
|
||||
@@ -156,13 +158,13 @@ describe("M7: Forgejo PR compare URL format", () => {
|
||||
// update-comment-link: platform-aware final comment body
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("M7: updateForgejoCommentBody vs updateCommentBody", () => {
|
||||
it("Forgejo version strips Unicode spinner", async () => {
|
||||
const { updateForgejoCommentBody } = await import(
|
||||
"../src/forgejo/comment-logic"
|
||||
describe("M7: unified updateCommentBody handles both platforms", () => {
|
||||
it("strips Unicode spinner (Forgejo format)", async () => {
|
||||
const { updateCommentBody } = await import(
|
||||
"../src/github/operations/comment-logic"
|
||||
);
|
||||
|
||||
const result = updateForgejoCommentBody({
|
||||
const result = updateCommentBody({
|
||||
currentBody:
|
||||
"Claude Code is working… ⏳\n\nI'll analyze this and get back to you.\n\n[View job run](https://git.wxs.ro/owner/repo/actions/runs/123)",
|
||||
actionFailed: false,
|
||||
|
||||
@@ -3,6 +3,7 @@ import * as core from "@actions/core";
|
||||
import { checkWritePermissions } from "../src/github/validation/permissions";
|
||||
import type { ParsedGitHubContext } from "../src/github/context";
|
||||
import { CLAUDE_APP_BOT_ID, CLAUDE_BOT_LOGIN } from "../src/github/constants";
|
||||
import { _resetPlatformCache } from "../src/platform/detector";
|
||||
|
||||
describe("checkWritePermissions", () => {
|
||||
let coreInfoSpy: any;
|
||||
@@ -310,6 +311,7 @@ describe("checkWritePermissions", () => {
|
||||
let savedServerUrl: string | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
_resetPlatformCache();
|
||||
savedPlatform = process.env.PLATFORM;
|
||||
savedForgejo = process.env.FORGEJO;
|
||||
savedServerUrl = process.env.GITHUB_SERVER_URL;
|
||||
@@ -324,6 +326,7 @@ describe("checkWritePermissions", () => {
|
||||
if (savedServerUrl !== undefined)
|
||||
process.env.GITHUB_SERVER_URL = savedServerUrl;
|
||||
else delete process.env.GITHUB_SERVER_URL;
|
||||
_resetPlatformCache();
|
||||
});
|
||||
|
||||
const createForgejoOctokit = (permissions: {
|
||||
|
||||
@@ -1,9 +1,10 @@
|
||||
import { detectPlatform } from "../src/platform/detector";
|
||||
import { detectPlatform, _resetPlatformCache } from "../src/platform/detector";
|
||||
import { describe, it, expect, beforeEach } from "bun:test";
|
||||
|
||||
describe("detectPlatform", () => {
|
||||
beforeEach(() => {
|
||||
// Reset relevant env vars before each test
|
||||
// Reset memoized result and env vars before each test
|
||||
_resetPlatformCache();
|
||||
delete process.env.PLATFORM;
|
||||
delete process.env.FORGEJO;
|
||||
delete process.env.GITHUB_SERVER_URL;
|
||||
|
||||
Reference in new issue
Block a user