From f98c06b8355c414fe851a6214b7b2912e6e9b486 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Thu, 24 Sep 2026 16:15:07 -0400 Subject: [PATCH 1/2] feat(domain): add github alert routing and repo-topic link use cases Adds migration 0002 (github_org_claims, repo_topic_links), the GithubEvent model and alert formatter, ports for org claims, repo links and alert sending, and the linkRepoToTopic, unlinkRepo, listRepoLinks and routeGithubEvent use cases. Linking is admin-only and limited to the team's claimed org; unlinked repos produce no alerts; infrastructure failures propagate so the route can answer 500. --- migrations/0002_github_alerts.sql | 20 ++++ src/domain/entities.ts | 15 +++ src/domain/errors.ts | 8 ++ src/domain/github.ts | 58 ++++++++++ src/domain/ports.ts | 29 +++++ src/domain/usecases/link-repo-to-topic.ts | 75 ++++++++++++ src/domain/usecases/list-repo-links.ts | 49 ++++++++ src/domain/usecases/route-github-event.ts | 63 ++++++++++ src/domain/usecases/unlink-repo.ts | 35 ++++++ test/adapters/migrations.test.ts | 4 + test/domain/github.test.ts | 109 ++++++++++++++++++ test/domain/link-repo-to-topic.test.ts | 125 ++++++++++++++++++++ test/domain/list-repo-links.test.ts | 80 +++++++++++++ test/domain/route-github-event.test.ts | 134 ++++++++++++++++++++++ test/domain/unlink-repo.test.ts | 106 +++++++++++++++++ test/fakes/index.ts | 76 ++++++++++++ 16 files changed, 986 insertions(+) create mode 100644 migrations/0002_github_alerts.sql create mode 100644 src/domain/github.ts create mode 100644 src/domain/usecases/link-repo-to-topic.ts create mode 100644 src/domain/usecases/list-repo-links.ts create mode 100644 src/domain/usecases/route-github-event.ts create mode 100644 src/domain/usecases/unlink-repo.ts create mode 100644 test/domain/github.test.ts create mode 100644 test/domain/link-repo-to-topic.test.ts create mode 100644 test/domain/list-repo-links.test.ts create mode 100644 test/domain/route-github-event.test.ts create mode 100644 test/domain/unlink-repo.test.ts diff --git a/migrations/0002_github_alerts.sql b/migrations/0002_github_alerts.sql new file mode 100644 index 0000000..e6b1046 --- /dev/null +++ b/migrations/0002_github_alerts.sql @@ -0,0 +1,20 @@ +-- GitHub alerts schema (design.md "Data Flow" schema block). +-- Lowercase org/repo logins, epoch-ms integer timestamps. + +CREATE TABLE github_org_claims ( + org_login TEXT PRIMARY KEY CHECK (org_login = lower(org_login)), -- one org -> one team + team_id TEXT NOT NULL REFERENCES teams(id), + created_at INTEGER NOT NULL, + UNIQUE (team_id, org_login) +); + +CREATE TABLE repo_topic_links ( + team_id TEXT NOT NULL, + repo_full_name TEXT NOT NULL CHECK (repo_full_name = lower(repo_full_name)), + org_login TEXT NOT NULL, + thread_id INTEGER NOT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + PRIMARY KEY (team_id, repo_full_name), -- one topic per repo per team + FOREIGN KEY (team_id, org_login) REFERENCES github_org_claims(team_id, org_login) +); diff --git a/src/domain/entities.ts b/src/domain/entities.ts index 3af6053..5ad0a23 100644 --- a/src/domain/entities.ts +++ b/src/domain/entities.ts @@ -1,3 +1,4 @@ +import type { RepoFullName } from "./github"; import type { MemberId, MembershipId, TeamId } from "./ids"; export interface Team { @@ -75,3 +76,17 @@ export interface DmSelection { teamId: TeamId; expiresAt: number; } + +// One repo maps to at most one forum topic per team (design.md "One Topic +// Per Repo, Re-Link Moves It"). `orgLogin` is redundant with the owner +// segment of `repoFullName` but kept as its own column so the composite FK +// to `github_org_claims(team_id, org_login)` can enforce claim ownership at +// the D1 layer without re-parsing the repo name (see migrations/0002). +export interface RepoTopicLink { + teamId: TeamId; + repoFullName: RepoFullName; + orgLogin: string; + threadId: number; + createdAt: number; + updatedAt: number; +} diff --git a/src/domain/errors.ts b/src/domain/errors.ts index 463db03..b0d9fea 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -12,3 +12,11 @@ export class LastAdminError extends DomainError {} export class ChatAdminCheckFailedError extends DomainError {} export class DmSelectionRequiredError extends DomainError {} export class FieldUnreadableError extends DomainError {} +export class InvalidRepoError extends DomainError {} +export class OrgNotClaimedError extends DomainError {} +// Thrown by an AlertSender implementation when the underlying send fails +// (e.g. Telegram rejects the request because the topic was deleted). +// route-github-event.ts catches this specific error and returns a +// "send-failed" outcome instead of letting it propagate (design.md +// "GitHub route status policy"). +export class AlertSendFailedError extends DomainError {} diff --git a/src/domain/github.ts b/src/domain/github.ts new file mode 100644 index 0000000..7b12f67 --- /dev/null +++ b/src/domain/github.ts @@ -0,0 +1,58 @@ +// Domain-only GitHub event model. The adapter mapper (adapters/github) is +// the only place that reads raw webhook JSON — the domain never sees +// payload shapes (design.md "Architecture Decisions", Event filtering). + +// Branded like TeamId/MemberId (see ids.ts) — always the lowercased +// `owner/repo` shape, never an unchecked string. +export type RepoFullName = string & { readonly __brand: "RepoFullName" }; + +// Owner and repo segments: at least one alphanumeric, dot, hyphen or +// underscore, no slashes or whitespace inside a segment. +const REPO_FULL_NAME_PATTERN = /^[a-z0-9._-]+\/[a-z0-9._-]+$/; + +export function parseRepoFullName(raw: string): RepoFullName | null { + const lower = raw.trim().toLowerCase(); + return REPO_FULL_NAME_PATTERN.test(lower) ? (lower as RepoFullName) : null; +} + +export type GithubEventKind = "pull_request" | "issues"; +export type GithubEventAction = + | "opened" + | "closed" + | "merged" + | "review_requested"; + +// Allowlisted fields only (design.md "Allowlisted Fields Only, No Payload +// Storage or Logging"). `reviewer` and `actor` are logins only. +export interface GithubEvent { + org: string; + repo: RepoFullName; + kind: GithubEventKind; + action: GithubEventAction; + number: number; + title: string; + url: string; + actor: string; + reviewer?: string; +} + +// Telegram's hard limit (design.md "Message"). Title is capped separately +// so one runaway field cannot silently swallow the rest of the message. +const TITLE_MAX = 256; +const MESSAGE_MAX = 4096; + +export function formatGithubAlert(event: GithubEvent): string { + const title = truncate(event.title, TITLE_MAX); + const lines = [ + `${event.repo} — ${event.kind} ${event.action}`, + `#${event.number}: ${title}`, + event.reviewer !== undefined ? `Reviewer: ${event.reviewer}` : null, + `By: ${event.actor}`, + event.url, + ].filter((line): line is string => line !== null); + return truncate(lines.join("\n"), MESSAGE_MAX); +} + +function truncate(text: string, max: number): string { + return text.length > max ? text.slice(0, max) : text; +} diff --git a/src/domain/ports.ts b/src/domain/ports.ts index d5c0057..de7047b 100644 --- a/src/domain/ports.ts +++ b/src/domain/ports.ts @@ -4,8 +4,10 @@ import type { Membership, ProfileField, DmSelection, + RepoTopicLink, Team, } from "./entities"; +import type { RepoFullName } from "./github"; import type { MemberId, MembershipId, TeamId } from "./ids"; import type { Role } from "./entities"; @@ -148,3 +150,30 @@ export interface LogEvent { export interface Logger { log(entry: LogEvent): void; } + +// GitHub alerts (design.md "Tenancy for routing" and "Interfaces / +// Contracts"). `findTeamByOrg` is the sole cross-team lookup here — +// mirrors MembershipRepo.findByUser — because routing an inbound webhook +// starts with only an org login, before any TeamId is known. Everything +// after it takes TeamId first. +export interface GithubOrgClaimRepo { + findTeamByOrg(orgLogin: string): Promise; + isClaimedBy(teamId: TeamId, orgLogin: string): Promise; +} + +export interface RepoTopicLinkRepo { + get(teamId: TeamId, repo: RepoFullName): Promise; + // ON CONFLICT DO UPDATE thread_id (design.md "Interfaces / Contracts") — + // re-linking an already-linked repo moves it instead of erroring. + upsert(teamId: TeamId, link: RepoTopicLink): Promise; + remove(teamId: TeamId, repo: RepoFullName): Promise; + list(teamId: TeamId): Promise; +} + +export interface AlertSender { + // Throws AlertSendFailedError (never resolves false-on-error) so + // routeGithubEvent can distinguish "delivered" from "send failed" and + // return the "send-failed" outcome instead of a 500 (design.md "GitHub + // route status policy"). + send(chatId: number, threadId: number, text: string): Promise; +} diff --git a/src/domain/usecases/link-repo-to-topic.ts b/src/domain/usecases/link-repo-to-topic.ts new file mode 100644 index 0000000..95d82b9 --- /dev/null +++ b/src/domain/usecases/link-repo-to-topic.ts @@ -0,0 +1,75 @@ +import { NotFoundError, OrgNotClaimedError, UnauthorizedError } from "../errors"; +import type { RepoFullName } from "../github"; +import type { MembershipId, TeamId } from "../ids"; +import type { + Clock, + GithubOrgClaimRepo, + MembershipRepo, + RepoTopicLinkRepo, +} from "../ports"; + +export interface LinkRepoToTopicInput { + teamId: TeamId; + actorMembershipId: MembershipId; + repo: RepoFullName; + // Caller (adapter) has already refused a null thread — see design.md + // "Interfaces / Contracts" ("the thread must not be null, the same + // refusal as /datachannel"). + threadId: number; +} + +export interface LinkRepoToTopicDeps { + membershipRepo: MembershipRepo; + githubOrgClaimRepo: GithubOrgClaimRepo; + repoTopicLinkRepo: RepoTopicLinkRepo; + clock: Clock; +} + +export interface LinkRepoToTopicResult { + repo: RepoFullName; + // Null on a first link, so the adapter can tell a fresh link from a move + // (design.md "One Topic Per Repo, Re-Link Moves It"). + previousThreadId: number | null; +} + +export async function linkRepoToTopic( + input: LinkRepoToTopicInput, + deps: LinkRepoToTopicDeps, +): Promise { + const actor = await deps.membershipRepo.get( + input.teamId, + input.actorMembershipId, + ); + if (!actor) { + throw new NotFoundError("Actor membership not found"); + } + if (actor.role !== "admin") { + throw new UnauthorizedError("Only a team admin may link a repo"); + } + + const orgLogin = orgLoginFromRepo(input.repo); + const claimed = await deps.githubOrgClaimRepo.isClaimedBy( + input.teamId, + orgLogin, + ); + if (!claimed) { + throw new OrgNotClaimedError(`Org "${orgLogin}" is not claimed by this team`); + } + + const existing = await deps.repoTopicLinkRepo.get(input.teamId, input.repo); + const now = deps.clock.now(); + await deps.repoTopicLinkRepo.upsert(input.teamId, { + teamId: input.teamId, + repoFullName: input.repo, + orgLogin, + threadId: input.threadId, + createdAt: existing?.createdAt ?? now, + updatedAt: now, + }); + + return { repo: input.repo, previousThreadId: existing?.threadId ?? null }; +} + +function orgLoginFromRepo(repo: RepoFullName): string { + return repo.slice(0, repo.indexOf("/")); +} diff --git a/src/domain/usecases/list-repo-links.ts b/src/domain/usecases/list-repo-links.ts new file mode 100644 index 0000000..968143b --- /dev/null +++ b/src/domain/usecases/list-repo-links.ts @@ -0,0 +1,49 @@ +import type { RepoTopicLink } from "../entities"; +import { NotFoundError } from "../errors"; +import type { MembershipId, TeamId } from "../ids"; +import type { + GithubOrgClaimRepo, + MembershipRepo, + RepoTopicLinkRepo, +} from "../ports"; + +export interface ListRepoLinksInput { + teamId: TeamId; + actorMembershipId: MembershipId; +} + +export interface ListRepoLinksDeps { + membershipRepo: MembershipRepo; + repoTopicLinkRepo: RepoTopicLinkRepo; + githubOrgClaimRepo: GithubOrgClaimRepo; +} + +// Any registered member may list (spec: repo-topic-links "Any Member Lists +// the Team's Claimed-Org Links") — no admin check, unlike link/unlink. +// Read-only: never mutates a stored link. +export async function listRepoLinks( + input: ListRepoLinksInput, + deps: ListRepoLinksDeps, +): Promise { + const actor = await deps.membershipRepo.get( + input.teamId, + input.actorMembershipId, + ); + if (!actor) { + throw new NotFoundError("Actor membership not found"); + } + + const links = await deps.repoTopicLinkRepo.list(input.teamId); + const results: RepoTopicLink[] = []; + for (const link of links) { + // Excludes a link whose org claim was later removed — the link row can + // outlive the claim (spec: "excludes any link that no longer has a + // matching org claim"). + const claimed = await deps.githubOrgClaimRepo.isClaimedBy( + input.teamId, + link.orgLogin, + ); + if (claimed) results.push(link); + } + return results; +} diff --git a/src/domain/usecases/route-github-event.ts b/src/domain/usecases/route-github-event.ts new file mode 100644 index 0000000..ff309ca --- /dev/null +++ b/src/domain/usecases/route-github-event.ts @@ -0,0 +1,63 @@ +import { AlertSendFailedError, NotFoundError } from "../errors"; +import { formatGithubAlert } from "../github"; +import type { GithubEvent } from "../github"; +import type { TeamId } from "../ids"; +import type { + AlertSender, + GithubOrgClaimRepo, + RepoTopicLinkRepo, + TeamRepo, +} from "../ports"; + +export interface RouteGithubEventDeps { + githubOrgClaimRepo: GithubOrgClaimRepo; + repoTopicLinkRepo: RepoTopicLinkRepo; + teamRepo: TeamRepo; + alertSender: AlertSender; +} + +export type RouteGithubEventResult = + | { kind: "delivered"; teamId: TeamId } + | { kind: "send-failed"; teamId: TeamId } + | { kind: "ignored"; reason: "unclaimed-org" | "unlinked-repo" }; + +// design.md "GitHub route status policy": unclaimed org or unlinked repo +// produce no alert and no error (200, ignored). A send failure is caught +// here and reported as "send-failed", not thrown, so the HTTP adapter can +// still return 2xx (GitHub does not need to retry a permanent delivery +// failure like a deleted topic). Any other failure (e.g. D1) propagates so +// the adapter returns 500 and the delivery stays visible for a manual +// redeliver. +export async function routeGithubEvent( + event: GithubEvent, + deps: RouteGithubEventDeps, +): Promise { + const teamId = await deps.githubOrgClaimRepo.findTeamByOrg(event.org); + if (!teamId) { + return { kind: "ignored", reason: "unclaimed-org" }; + } + + const link = await deps.repoTopicLinkRepo.get(teamId, event.repo); + if (!link) { + return { kind: "ignored", reason: "unlinked-repo" }; + } + + const team = await deps.teamRepo.get(teamId); + if (!team) { + // A claim/link exists but the team row is gone — a data-integrity + // problem, not a normal "no alert" case, so this propagates as a 500. + throw new NotFoundError("Team not found for a claimed org"); + } + + const text = formatGithubAlert(event); + try { + await deps.alertSender.send(team.chatId, link.threadId, text); + } catch (err) { + if (err instanceof AlertSendFailedError) { + return { kind: "send-failed", teamId }; + } + throw err; + } + + return { kind: "delivered", teamId }; +} diff --git a/src/domain/usecases/unlink-repo.ts b/src/domain/usecases/unlink-repo.ts new file mode 100644 index 0000000..17973e7 --- /dev/null +++ b/src/domain/usecases/unlink-repo.ts @@ -0,0 +1,35 @@ +import { NotFoundError, UnauthorizedError } from "../errors"; +import type { RepoFullName } from "../github"; +import type { MembershipId, TeamId } from "../ids"; +import type { MembershipRepo, RepoTopicLinkRepo } from "../ports"; + +export interface UnlinkRepoInput { + teamId: TeamId; + actorMembershipId: MembershipId; + repo: RepoFullName; +} + +export interface UnlinkRepoDeps { + membershipRepo: MembershipRepo; + repoTopicLinkRepo: RepoTopicLinkRepo; +} + +// Returns false (not an error) when there was nothing to remove — unlinking +// an already-unlinked repo is idempotent, mirroring RepoTopicLinkRepo.remove. +export async function unlinkRepo( + input: UnlinkRepoInput, + deps: UnlinkRepoDeps, +): Promise { + const actor = await deps.membershipRepo.get( + input.teamId, + input.actorMembershipId, + ); + if (!actor) { + throw new NotFoundError("Actor membership not found"); + } + if (actor.role !== "admin") { + throw new UnauthorizedError("Only a team admin may unlink a repo"); + } + + return deps.repoTopicLinkRepo.remove(input.teamId, input.repo); +} diff --git a/test/adapters/migrations.test.ts b/test/adapters/migrations.test.ts index ab15a98..f5bcf6f 100644 --- a/test/adapters/migrations.test.ts +++ b/test/adapters/migrations.test.ts @@ -15,9 +15,13 @@ describe("migrations/0001_init.sql", () => { expect(names).toEqual([ "audit_log", "dm_selections", + // github_org_claims and repo_topic_links: migrations/0002_github_alerts.sql + // (PR2 covers their own FK/UNIQUE/CHECK migration tests). + "github_org_claims", "members", "memberships", "profile_fields", + "repo_topic_links", "teams", ]); }); diff --git a/test/domain/github.test.ts b/test/domain/github.test.ts new file mode 100644 index 0000000..011f6a9 --- /dev/null +++ b/test/domain/github.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from "vitest"; +import { formatGithubAlert, parseRepoFullName } from "../../src/domain/github"; +import type { GithubEvent } from "../../src/domain/github"; + +describe("parseRepoFullName", () => { + it("accepts a lowercase owner/repo shape", () => { + expect(parseRepoFullName("octocat/hello-world")).toBe("octocat/hello-world"); + }); + + it("lowercases a mixed-case input", () => { + expect(parseRepoFullName("OctoCat/Hello-World")).toBe("octocat/hello-world"); + }); + + it("rejects a string with no slash", () => { + expect(parseRepoFullName("octocat")).toBeNull(); + }); + + it("rejects a string with more than one slash", () => { + expect(parseRepoFullName("octocat/hello/world")).toBeNull(); + }); + + it("rejects an empty owner or repo segment", () => { + expect(parseRepoFullName("/hello-world")).toBeNull(); + expect(parseRepoFullName("octocat/")).toBeNull(); + }); + + it("rejects whitespace inside the name", () => { + expect(parseRepoFullName("octo cat/hello world")).toBeNull(); + }); +}); + +function makeEvent(overrides: Partial = {}): GithubEvent { + return { + org: "octocat", + repo: parseRepoFullName("octocat/hello-world")!, + kind: "pull_request", + action: "opened", + number: 42, + title: "Fix the thing", + url: "https://github.com/octocat/hello-world/pull/42", + actor: "octocat", + ...overrides, + }; +} + +describe("formatGithubAlert", () => { + it("includes only the allowlisted fields", () => { + const text = formatGithubAlert(makeEvent()); + expect(text).toContain("octocat/hello-world"); + expect(text).toContain("#42"); + expect(text).toContain("Fix the thing"); + expect(text).toContain("octocat"); + expect(text).toContain("https://github.com/octocat/hello-world/pull/42"); + }); + + it("truncates a very long title so the message stays at most 4096 characters", () => { + const longTitle = "x".repeat(5000); + const text = formatGithubAlert(makeEvent({ title: longTitle })); + expect(text.length).toBeLessThanOrEqual(4096); + }); + + it("caps the title itself at 256 characters", () => { + const longTitle = "y".repeat(500); + const text = formatGithubAlert(makeEvent({ title: longTitle })); + expect(text).not.toContain("y".repeat(300)); + }); + + it("remains a non-empty string for a normal event", () => { + const text = formatGithubAlert(makeEvent()); + expect(text.length).toBeGreaterThan(0); + expect(text.length).toBeLessThanOrEqual(4096); + }); + + it("includes a Reviewer line with the exact output when reviewer is set", () => { + const text = formatGithubAlert( + makeEvent({ + kind: "pull_request", + action: "review_requested", + reviewer: "hubot", + }), + ); + expect(text).toBe( + [ + "octocat/hello-world — pull_request review_requested", + "#42: Fix the thing", + "Reviewer: hubot", + "By: octocat", + "https://github.com/octocat/hello-world/pull/42", + ].join("\n"), + ); + }); + + it("omits the Reviewer line entirely when reviewer is not set", () => { + const text = formatGithubAlert(makeEvent({ action: "review_requested" })); + expect(text).not.toContain("Reviewer:"); + }); + + it("formats a merged pull_request alert with the exact output", () => { + const text = formatGithubAlert(makeEvent({ kind: "pull_request", action: "merged" })); + expect(text).toBe( + [ + "octocat/hello-world — pull_request merged", + "#42: Fix the thing", + "By: octocat", + "https://github.com/octocat/hello-world/pull/42", + ].join("\n"), + ); + }); +}); diff --git a/test/domain/link-repo-to-topic.test.ts b/test/domain/link-repo-to-topic.test.ts new file mode 100644 index 0000000..9d4437d --- /dev/null +++ b/test/domain/link-repo-to-topic.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from "vitest"; +import { linkRepoToTopic } from "../../src/domain/usecases/link-repo-to-topic"; +import { NotFoundError, OrgNotClaimedError, UnauthorizedError } from "../../src/domain/errors"; +import { parseRepoFullName } from "../../src/domain/github"; +import { asMemberId, asMembershipId, asTeamId } from "../../src/domain/ids"; +import { + fakeClock, + fakeGithubOrgClaimRepo, + fakeMemberRepo, + fakeMembershipRepo, + fakeRepoTopicLinkRepo, +} from "../fakes"; + +const teamId = asTeamId("team-1"); +const repo = parseRepoFullName("octocat/hello-world")!; + +function makeDeps() { + const memberRepo = fakeMemberRepo(); + return { + membershipRepo: fakeMembershipRepo(memberRepo), + githubOrgClaimRepo: fakeGithubOrgClaimRepo(), + repoTopicLinkRepo: fakeRepoTopicLinkRepo(), + clock: fakeClock(), + }; +} + +function pushAdmin(deps: ReturnType) { + const adminId = asMembershipId("m-admin"); + deps.membershipRepo.rows.push({ + id: adminId, + teamId, + memberId: asMemberId("u-admin"), + role: "admin", + joinedAt: 0, + }); + return adminId; +} + +function pushMember(deps: ReturnType) { + const memberId = asMembershipId("m-member"); + deps.membershipRepo.rows.push({ + id: memberId, + teamId, + memberId: asMemberId("u-member"), + role: "member", + joinedAt: 0, + }); + return memberId; +} + +describe("linkRepoToTopic", () => { + it("rejects a caller who is not a registered member", async () => { + const deps = makeDeps(); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + + await expect( + linkRepoToTopic( + { teamId, actorMembershipId: asMembershipId("ghost"), repo, threadId: 10 }, + deps, + ), + ).rejects.toThrow(NotFoundError); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(0); + }); + + it("links a repo whose org is claimed by the team", async () => { + const deps = makeDeps(); + const adminId = pushAdmin(deps); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + + const result = await linkRepoToTopic( + { teamId, actorMembershipId: adminId, repo, threadId: 10 }, + deps, + ); + + expect(result).toEqual({ repo, previousThreadId: null }); + expect(deps.repoTopicLinkRepo.rows).toEqual([ + { + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 10, + createdAt: 1_700_000_000_000, + updatedAt: 1_700_000_000_000, + }, + ]); + }); + + it("rejects linking a repo whose org has no claim, and stores no row", async () => { + const deps = makeDeps(); + const adminId = pushAdmin(deps); + + await expect( + linkRepoToTopic({ teamId, actorMembershipId: adminId, repo, threadId: 10 }, deps), + ).rejects.toThrow(OrgNotClaimedError); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(0); + }); + + it("refuses a non-admin", async () => { + const deps = makeDeps(); + const memberId = pushMember(deps); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + + await expect( + linkRepoToTopic({ teamId, actorMembershipId: memberId, repo, threadId: 10 }, deps), + ).rejects.toThrow(UnauthorizedError); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(0); + }); + + it("re-linking an already-linked repo moves it and reports the previous thread id", async () => { + const deps = makeDeps(); + const adminId = pushAdmin(deps); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + + await linkRepoToTopic({ teamId, actorMembershipId: adminId, repo, threadId: 10 }, deps); + deps.clock.advance(1_000); + const result = await linkRepoToTopic( + { teamId, actorMembershipId: adminId, repo, threadId: 20 }, + deps, + ); + + expect(result).toEqual({ repo, previousThreadId: 10 }); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(1); + expect(deps.repoTopicLinkRepo.rows[0]?.threadId).toBe(20); + }); +}); diff --git a/test/domain/list-repo-links.test.ts b/test/domain/list-repo-links.test.ts new file mode 100644 index 0000000..5c04240 --- /dev/null +++ b/test/domain/list-repo-links.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from "vitest"; +import { listRepoLinks } from "../../src/domain/usecases/list-repo-links"; +import { NotFoundError } from "../../src/domain/errors"; +import { parseRepoFullName } from "../../src/domain/github"; +import { asMemberId, asMembershipId, asTeamId } from "../../src/domain/ids"; +import { + fakeGithubOrgClaimRepo, + fakeMemberRepo, + fakeMembershipRepo, + fakeRepoTopicLinkRepo, +} from "../fakes"; + +const teamId = asTeamId("team-1"); +const repoA = parseRepoFullName("octocat/repo-a")!; +const repoB = parseRepoFullName("octocat/repo-b")!; + +function makeDeps() { + const memberRepo = fakeMemberRepo(); + return { + membershipRepo: fakeMembershipRepo(memberRepo), + repoTopicLinkRepo: fakeRepoTopicLinkRepo(), + githubOrgClaimRepo: fakeGithubOrgClaimRepo(), + }; +} + +describe("listRepoLinks", () => { + it("lets a non-admin member list the team's links", async () => { + const deps = makeDeps(); + const memberId = asMembershipId("m-member"); + deps.membershipRepo.rows.push({ + id: memberId, + teamId, + memberId: asMemberId("u-member"), + role: "member", + joinedAt: 0, + }); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + deps.repoTopicLinkRepo.rows.push( + { teamId, repoFullName: repoA, orgLogin: "octocat", threadId: 1, createdAt: 0, updatedAt: 0 }, + { teamId, repoFullName: repoB, orgLogin: "octocat", threadId: 2, createdAt: 0, updatedAt: 0 }, + ); + + const links = await listRepoLinks({ teamId, actorMembershipId: memberId }, deps); + + expect(links.map((l) => l.repoFullName).sort()).toEqual([repoA, repoB].sort()); + }); + + it("refuses a caller who is not a registered member", async () => { + const deps = makeDeps(); + + await expect( + listRepoLinks({ teamId, actorMembershipId: asMembershipId("ghost") }, deps), + ).rejects.toThrow(NotFoundError); + }); + + it("excludes a link whose org claim no longer exists", async () => { + const deps = makeDeps(); + const memberId = asMembershipId("m-member"); + deps.membershipRepo.rows.push({ + id: memberId, + teamId, + memberId: asMemberId("u-member"), + role: "member", + joinedAt: 0, + }); + // No claim row pushed for "octocat" — the link is stale. + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repoA, + orgLogin: "octocat", + threadId: 1, + createdAt: 0, + updatedAt: 0, + }); + + const links = await listRepoLinks({ teamId, actorMembershipId: memberId }, deps); + + expect(links).toHaveLength(0); + }); +}); diff --git a/test/domain/route-github-event.test.ts b/test/domain/route-github-event.test.ts new file mode 100644 index 0000000..432e3c6 --- /dev/null +++ b/test/domain/route-github-event.test.ts @@ -0,0 +1,134 @@ +import { describe, expect, it } from "vitest"; +import { routeGithubEvent } from "../../src/domain/usecases/route-github-event"; +import { NotFoundError } from "../../src/domain/errors"; +import type { GithubEvent } from "../../src/domain/github"; +import { parseRepoFullName } from "../../src/domain/github"; +import { asTeamId } from "../../src/domain/ids"; +import { + fakeAlertSender, + fakeGithubOrgClaimRepo, + fakeRepoTopicLinkRepo, + fakeTeamRepo, +} from "../fakes"; + +const teamId = asTeamId("team-1"); +const repo = parseRepoFullName("octocat/hello-world")!; + +function makeEvent(overrides: Partial = {}): GithubEvent { + return { + org: "octocat", + repo, + kind: "pull_request", + action: "opened", + number: 42, + title: "Fix the thing", + url: "https://github.com/octocat/hello-world/pull/42", + actor: "octocat", + ...overrides, + }; +} + +function makeDeps() { + return { + githubOrgClaimRepo: fakeGithubOrgClaimRepo(), + repoTopicLinkRepo: fakeRepoTopicLinkRepo(), + teamRepo: fakeTeamRepo(), + alertSender: fakeAlertSender(), + }; +} + +describe("routeGithubEvent", () => { + it("ignores an event whose org is not claimed by any team", async () => { + const deps = makeDeps(); + + const result = await routeGithubEvent(makeEvent(), deps); + + expect(result).toEqual({ kind: "ignored", reason: "unclaimed-org" }); + expect(deps.alertSender.sent).toHaveLength(0); + }); + + it("ignores an event for a repo with no link, even when the org is claimed", async () => { + const deps = makeDeps(); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + + const result = await routeGithubEvent(makeEvent(), deps); + + expect(result).toEqual({ kind: "ignored", reason: "unlinked-repo" }); + expect(deps.alertSender.sent).toHaveLength(0); + }); + + it("delivers an alert to the linked topic for a linked, claimed repo", async () => { + const deps = makeDeps(); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 55, + createdAt: 0, + updatedAt: 0, + }); + deps.teamRepo.rows.push({ id: teamId, chatId: 999, dataTopicThreadId: null, createdAt: 0 }); + + const result = await routeGithubEvent(makeEvent(), deps); + + expect(result).toEqual({ kind: "delivered", teamId }); + expect(deps.alertSender.sent).toHaveLength(1); + expect(deps.alertSender.sent[0]).toMatchObject({ chatId: 999, threadId: 55 }); + }); + + it("returns send-failed (not thrown) when the alert sender fails, without retrying", async () => { + const deps = makeDeps(); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 55, + createdAt: 0, + updatedAt: 0, + }); + deps.teamRepo.rows.push({ id: teamId, chatId: 999, dataTopicThreadId: null, createdAt: 0 }); + const failingDeps = { ...deps, alertSender: fakeAlertSender({ throws: true }) }; + + const result = await routeGithubEvent(makeEvent(), failingDeps); + + expect(result).toEqual({ kind: "send-failed", teamId }); + }); + + it("propagates (rejects) an unexpected error from the org claim lookup, instead of an ignored outcome (RES-001)", async () => { + const deps = makeDeps(); + const failingDeps = { ...deps, githubOrgClaimRepo: fakeGithubOrgClaimRepo({ throws: true }) }; + + await expect(routeGithubEvent(makeEvent(), failingDeps)).rejects.toThrow("D1 unavailable"); + expect(deps.alertSender.sent).toHaveLength(0); + }); + + it("propagates (rejects) an unexpected error from the repo link lookup, instead of an ignored outcome (RES-001)", async () => { + const deps = makeDeps(); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + const failingDeps = { ...deps, repoTopicLinkRepo: fakeRepoTopicLinkRepo({ throws: true }) }; + + await expect(routeGithubEvent(makeEvent(), failingDeps)).rejects.toThrow("D1 unavailable"); + expect(deps.alertSender.sent).toHaveLength(0); + }); + + it("rejects with NotFoundError when the claim and link exist but the team row is missing (REL-002/RES-002)", async () => { + const deps = makeDeps(); + deps.githubOrgClaimRepo.rows.push({ teamId, orgLogin: "octocat" }); + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 55, + createdAt: 0, + updatedAt: 0, + }); + // No row pushed to deps.teamRepo — the team is missing despite the + // claim and link existing (a data-integrity problem, not a normal + // "no alert" outcome). + + await expect(routeGithubEvent(makeEvent(), deps)).rejects.toThrow(NotFoundError); + expect(deps.alertSender.sent).toHaveLength(0); + }); +}); diff --git a/test/domain/unlink-repo.test.ts b/test/domain/unlink-repo.test.ts new file mode 100644 index 0000000..17b85d0 --- /dev/null +++ b/test/domain/unlink-repo.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it } from "vitest"; +import { unlinkRepo } from "../../src/domain/usecases/unlink-repo"; +import { NotFoundError, UnauthorizedError } from "../../src/domain/errors"; +import { parseRepoFullName } from "../../src/domain/github"; +import { asMemberId, asMembershipId, asTeamId } from "../../src/domain/ids"; +import { + fakeMemberRepo, + fakeMembershipRepo, + fakeRepoTopicLinkRepo, +} from "../fakes"; + +const teamId = asTeamId("team-1"); +const repo = parseRepoFullName("octocat/hello-world")!; + +function makeDeps() { + const memberRepo = fakeMemberRepo(); + return { + membershipRepo: fakeMembershipRepo(memberRepo), + repoTopicLinkRepo: fakeRepoTopicLinkRepo(), + }; +} + +describe("unlinkRepo", () => { + it("rejects a caller who is not a registered member", async () => { + const deps = makeDeps(); + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 10, + createdAt: 0, + updatedAt: 0, + }); + + await expect( + unlinkRepo({ teamId, actorMembershipId: asMembershipId("ghost"), repo }, deps), + ).rejects.toThrow(NotFoundError); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(1); + }); + + it("lets a team admin remove an existing link", async () => { + const deps = makeDeps(); + const adminId = asMembershipId("m-admin"); + deps.membershipRepo.rows.push({ + id: adminId, + teamId, + memberId: asMemberId("u-admin"), + role: "admin", + joinedAt: 0, + }); + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 10, + createdAt: 0, + updatedAt: 0, + }); + + const removed = await unlinkRepo({ teamId, actorMembershipId: adminId, repo }, deps); + + expect(removed).toBe(true); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(0); + }); + + it("refuses a non-admin and leaves the link in place", async () => { + const deps = makeDeps(); + const memberId = asMembershipId("m-member"); + deps.membershipRepo.rows.push({ + id: memberId, + teamId, + memberId: asMemberId("u-member"), + role: "member", + joinedAt: 0, + }); + deps.repoTopicLinkRepo.rows.push({ + teamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 10, + createdAt: 0, + updatedAt: 0, + }); + + await expect( + unlinkRepo({ teamId, actorMembershipId: memberId, repo }, deps), + ).rejects.toThrow(UnauthorizedError); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(1); + }); + + it("returns false when there is no link to remove", async () => { + const deps = makeDeps(); + const adminId = asMembershipId("m-admin"); + deps.membershipRepo.rows.push({ + id: adminId, + teamId, + memberId: asMemberId("u-admin"), + role: "admin", + joinedAt: 0, + }); + + const removed = await unlinkRepo({ teamId, actorMembershipId: adminId, repo }, deps); + + expect(removed).toBe(false); + }); +}); diff --git a/test/fakes/index.ts b/test/fakes/index.ts index 13e7e2e..28761e7 100644 --- a/test/fakes/index.ts +++ b/test/fakes/index.ts @@ -1,20 +1,26 @@ +import { AlertSendFailedError } from "../../src/domain/errors"; import type { AuditDraft, Member, Membership, ProfileField, + RepoTopicLink, Role, DmSelection, Team, } from "../../src/domain/entities"; +import type { RepoFullName } from "../../src/domain/github"; import type { + AlertSender, ChatAdminChecker, Clock, DmSelectionRepo, + GithubOrgClaimRepo, IdGen, MemberRepo, MembershipRepo, ProfileRepo, + RepoTopicLinkRepo, TeamRepo, } from "../../src/domain/ports"; import type { MemberId, MembershipId, TeamId } from "../../src/domain/ids"; @@ -201,6 +207,76 @@ export function fakeChatAdminChecker( }; } +// GitHub alerts fakes (design.md "Interfaces / Contracts"). + +export function fakeGithubOrgClaimRepo( + opts: { throws?: boolean } = {}, +): GithubOrgClaimRepo & { + rows: Array<{ teamId: TeamId; orgLogin: string }>; +} { + const rows: Array<{ teamId: TeamId; orgLogin: string }> = []; + return { + rows, + findTeamByOrg: async (orgLogin: string) => { + if (opts.throws) throw new Error("D1 unavailable"); + return rows.find((r) => r.orgLogin === orgLogin)?.teamId ?? null; + }, + isClaimedBy: async (teamId: TeamId, orgLogin: string) => { + if (opts.throws) throw new Error("D1 unavailable"); + return rows.some((r) => r.teamId === teamId && r.orgLogin === orgLogin); + }, + }; +} + +export function fakeRepoTopicLinkRepo( + opts: { throws?: boolean } = {}, +): RepoTopicLinkRepo & { + rows: RepoTopicLink[]; +} { + const rows: RepoTopicLink[] = []; + return { + rows, + get: async (teamId: TeamId, repo: RepoFullName) => { + if (opts.throws) throw new Error("D1 unavailable"); + return ( + rows.find((l) => l.teamId === teamId && l.repoFullName === repo) ?? + null + ); + }, + upsert: async (teamId: TeamId, link: RepoTopicLink) => { + const idx = rows.findIndex( + (l) => l.teamId === teamId && l.repoFullName === link.repoFullName, + ); + if (idx >= 0) rows[idx] = link; + else rows.push(link); + }, + remove: async (teamId: TeamId, repo: RepoFullName) => { + const idx = rows.findIndex( + (l) => l.teamId === teamId && l.repoFullName === repo, + ); + if (idx < 0) return false; + rows.splice(idx, 1); + return true; + }, + list: async (teamId: TeamId) => rows.filter((l) => l.teamId === teamId), + }; +} + +export function fakeAlertSender( + opts: { throws?: boolean } = {}, +): AlertSender & { + sent: Array<{ chatId: number; threadId: number; text: string }>; +} { + const sent: Array<{ chatId: number; threadId: number; text: string }> = []; + return { + sent, + send: async (chatId: number, threadId: number, text: string) => { + if (opts.throws) throw new AlertSendFailedError("sendMessage failed"); + sent.push({ chatId, threadId, text }); + }, + }; +} + export function membership( overrides: Partial & { id: MembershipId; From a71c2187680c309ab8373fc1454f9915ce070c29 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Thu, 24 Sep 2026 16:15:08 -0400 Subject: [PATCH 2/2] docs(openspec): mark github-alerts phase 1 complete --- .../changes/github-alerts/apply-progress.md | 124 ++++++++++++++++++ openspec/changes/github-alerts/tasks.md | 20 +-- 2 files changed, 135 insertions(+), 9 deletions(-) create mode 100644 openspec/changes/github-alerts/apply-progress.md diff --git a/openspec/changes/github-alerts/apply-progress.md b/openspec/changes/github-alerts/apply-progress.md new file mode 100644 index 0000000..ae73ac9 --- /dev/null +++ b/openspec/changes/github-alerts/apply-progress.md @@ -0,0 +1,124 @@ +# Apply Progress: github-alerts + +## PR1 — Domain Foundation (Phase 1) + +**Mode**: Strict TDD (RED → GREEN, no REFACTOR step needed — first-pass implementations matched the fakes/patterns already in the codebase). + +**Branch**: `feat/github-alerts-domain` (from `docs/github-alerts-planning`). No commit made — working tree only, per instruction. + +### Completed Tasks + +- [x] 1.1 RED: `github.ts` tests (`parseRepoFullName`, `formatGithubAlert` truncation) +- [x] 1.2 GREEN: `src/domain/github.ts` +- [x] 1.3 `RepoTopicLink` entity; `GithubOrgClaimRepo`, `RepoTopicLinkRepo`, `AlertSender` ports; `InvalidRepoError`, `OrgNotClaimedError`, `AlertSendFailedError` +- [x] 1.4 RED: `link-repo-to-topic` tests (claim gate, admin gate, move semantics — see Deviation note below) +- [x] 1.5 GREEN: `src/domain/usecases/link-repo-to-topic.ts` +- [x] 1.6 RED/GREEN: `unlink-repo.ts`, `list-repo-links.ts` +- [x] 1.7 RED: `route-github-event` tests (unclaimed-org ignored, unlinked-repo ignored, delivered, send-failed) +- [x] 1.8 GREEN: `src/domain/usecases/route-github-event.ts` +- [x] 1.9 `migrations/0002_github_alerts.sql` + +### Files Changed + +| File | Action | What Was Done | +|------|--------|---------------| +| `migrations/0002_github_alerts.sql` | Created | `github_org_claims`, `repo_topic_links` tables per design.md SQL block, verbatim | +| `src/domain/github.ts` | Created | `RepoFullName` brand + `parseRepoFullName` (lowercases, validates `owner/repo` shape), `GithubEvent`/`GithubEventKind`/`GithubEventAction`, `formatGithubAlert` (allowlisted fields, 256-char title cap, 4096-char message cap) | +| `src/domain/entities.ts` | Modified | Added `RepoTopicLink` interface | +| `src/domain/ports.ts` | Modified | Added `GithubOrgClaimRepo`, `RepoTopicLinkRepo`, `AlertSender` interfaces | +| `src/domain/errors.ts` | Modified | Added `InvalidRepoError`, `OrgNotClaimedError`, `AlertSendFailedError` | +| `src/domain/usecases/link-repo-to-topic.ts` | Created | `linkRepoToTopic` — admin gate, claim gate (`OrgNotClaimedError`), upsert with `previousThreadId` in the result for move detection | +| `src/domain/usecases/unlink-repo.ts` | Created | `unlinkRepo` — admin gate, delegates removal, returns `boolean` (idempotent) | +| `src/domain/usecases/list-repo-links.ts` | Created | `listRepoLinks` — any registered member (no admin check), filters out links whose org claim no longer exists | +| `src/domain/usecases/route-github-event.ts` | Created | `routeGithubEvent` — org→team→link resolution, `ignored`/`delivered`/`send-failed` outcomes, catches `AlertSendFailedError` only (other errors propagate to become a 500 upstream) | +| `test/fakes/index.ts` | Modified | Added `fakeGithubOrgClaimRepo`, `fakeRepoTopicLinkRepo`, `fakeAlertSender` | +| `test/domain/github.test.ts` | Created | RED→GREEN for parsing and formatting/truncation | +| `test/domain/link-repo-to-topic.test.ts` | Created | RED→GREEN for `linkRepoToTopic` | +| `test/domain/unlink-repo.test.ts` | Created | RED→GREEN for `unlinkRepo` | +| `test/domain/list-repo-links.test.ts` | Created | RED→GREEN for `listRepoLinks` | +| `test/domain/route-github-event.test.ts` | Created | RED→GREEN for `routeGithubEvent` | +| `test/adapters/migrations.test.ts` | Modified | Added `github_org_claims`/`repo_topic_links` to the expected table list (this suite runs against the full `migrations/` directory via the shared test setup; adding 0002 without updating it would have broken an unrelated, pre-existing test) | + +### TDD Cycle Evidence + +| Task | RED (failing first, correct reason) | GREEN (implementation, passes) | REFACTOR | +|---|---|---|---| +| 1.1/1.2 `github.ts` | `test/domain/github.test.ts` — ran before `src/domain/github.ts` existed: `Cannot find module '../../src/domain/github'` | Created `github.ts`; `npx vitest run test/domain/github.test.ts` → 10/10 pass | None needed | +| 1.4/1.5 `link-repo-to-topic.ts` | `test/domain/link-repo-to-topic.test.ts` — ran before the module existed: `Cannot find module '.../usecases/link-repo-to-topic'` | Created `link-repo-to-topic.ts`; `npx vitest run test/domain/link-repo-to-topic.test.ts` → 4/4 pass | None needed | +| 1.6 `unlink-repo.ts` / `list-repo-links.ts` | Both test files ran before their modules existed: `Cannot find module` for each | Created both files; `npx vitest run test/domain/unlink-repo.test.ts test/domain/list-repo-links.test.ts` → 6/6 pass | None needed | +| 1.7/1.8 `route-github-event.ts` | `test/domain/route-github-event.test.ts` — ran before the module existed: `Cannot find module '.../usecases/route-github-event'` | Created `route-github-event.ts`; `npx vitest run test/domain/route-github-event.test.ts` → 4/4 pass | None needed | + +Every RED run above failed with a module-resolution error (the right reason — the production file did not exist yet), never a passing or wrongly-failing assertion. + +### Work Unit Evidence (PR1) + +| Evidence | Value | +|---|---| +| Focused test command and exact result | `npx vitest run test/domain` → 24 new/changed domain tests pass (10 `github.test.ts` + 4 `link-repo-to-topic.test.ts` + 3 `unlink-repo.test.ts` + 3 `list-repo-links.test.ts` + 4 `route-github-event.test.ts`, plus pre-existing domain suites unaffected) | +| Runtime harness command/scenario and exact result | N/A — this unit is pure domain logic with in-memory fakes (`test/fakes`), no Workers/D1 runtime boundary. The migration SQL was still exercised through the real vitest-pool-workers D1 setup: `npx vitest run` (full suite, includes `test/adapters/migrations.test.ts` against the actual applied `0001_init.sql` + `0002_github_alerts.sql`) → 221/221 pass | +| Rollback boundary | Delete `src/domain/github.ts`, `src/domain/usecases/{link-repo-to-topic,unlink-repo,list-repo-links,route-github-event}.ts`, `migrations/0002_github_alerts.sql`, and the five new `test/domain/*.test.ts` files; revert the additive edits to `entities.ts`, `ports.ts`, `errors.ts`, `test/fakes/index.ts`, `test/adapters/migrations.test.ts`. No other file references these new symbols yet (confirmed: nothing outside the files above imports them) | + +### Deviations from Design + +- Task 1.4 lists "must be inside a topic" and "re-link reply names old/new topic" as scenarios for the `link-repo-to-topic` RED tests. Per design.md's own contract (`linkRepoToTopic({ teamId, actorMembershipId, repo, threadId }, deps)`) and the precedent in `bindDataChannel`/`/datachannel` (the null-thread refusal happens in `commands.ts`, not the use case), those two concerns are adapter/command-layer responsibilities that belong to Phase 5 (`/linkrepo`), not the domain use case. The domain test suite instead covers: claimed-org link succeeds, unclaimed-org link is rejected (`OrgNotClaimedError`) and stores no row, non-admin is refused, and re-linking moves the thread and reports `previousThreadId` (the data the Phase-5 command needs to build the "moved from topic A to topic B" reply). This matches the design's actual interface contract; nothing in the domain logic differs from design.md. +- No other deviations — `entities.ts`/`ports.ts`/`errors.ts`/`github.ts` additions and the migration SQL are verbatim to design.md's "File Changes", "Interfaces / Contracts", and SQL block. + +### Issues Found / Risks + +- **PR size budget overrun (flagged, not silently exceeded)**: measured via `git add -N` + `git diff --stat`, this PR1 slice is **871 authored lines added, 0 deleted, across 16 files** (update: after the review correction below, the same measurement shows **1121 lines added, 9 deleted, across 18 files** — the correction added required test coverage and a mechanical rename, which further increases the overrun; still no `size:exception` decision or split has been applied) — over the 400-line review budget and over the tasks.md estimate of ~350. Implementation-only code (migration + `entities`/`errors`/`ports` additions + `github.ts` + the 4 use cases + the one-line fix to the pre-existing `migrations.test.ts`) is ~356 lines, under budget on its own. The overrun is entirely the Strict-TDD test suite (5 new `test/domain/*.test.ts` files, 451 lines) plus the 3 new fakes in `test/fakes/index.ts` (64 lines) that Strict TDD requires before each implementation. No code was cut to force a fit, per the instruction to stop and report rather than exceed the budget silently. Recommend one of: (a) accept as `size:exception` given tests are the majority of the overrun and the change is otherwise self-contained and low-risk (pure domain, no I/O), or (b) split into two chained slices before commit — 1a: migration + entities/errors/ports + `github.ts` + `link-repo-to-topic`/`unlink-repo`/`list-repo-links` + tests + fakes (~650 lines, still over 400 but closer), 1b: `route-github-event.ts` + its test (~160 lines). Neither split cleanly fits 400 given the fakes are shared; a maintainer decision is needed before this becomes an actual PR. +- `test/adapters/migrations.test.ts` required a one-line-set edit (added two table names to an existing assertion) because it enumerates every table in the shared D1 test database, which now includes the new migration's tables. This is a pre-existing test, not new test debt, and was not in tasks.md — noted here for visibility. + +## Correction — PR1 Review Ledger (frozen findings) + +Applied on `feat/github-alerts-domain`, still no commit/push, `.codegraph/` untouched. Fixes the CRITICAL/other findings from the frozen PR1 review ledger. + +### Findings Addressed + +| Finding | Fix | New test result | +|---|---|---| +| RES-001 (CRITICAL) — "D1 is unavailable during routing" untested | Added `throws` option to `fakeGithubOrgClaimRepo`/`fakeRepoTopicLinkRepo` (`test/fakes/index.ts`), mirroring `fakeAlertSender`/`fakeChatAdminChecker`. Added two tests in `route-github-event.test.ts` asserting an unexpected error from `findTeamByOrg` and from the link `get` propagates (rejects), not an `ignored` outcome | **Passed immediately** — characterization test; `routeGithubEvent` already had no try/catch around those calls, so the error was already propagating correctly. No production code changed | +| REL-001 (CRITICAL) — `formatGithubAlert` reviewer line and `merged`/`review_requested` untested | Added exact-output tests in `test/domain/github.test.ts`: reviewer line present (`review_requested` + reviewer set), reviewer line omitted when unset, and an exact-output `merged` alert | **Passed immediately** — characterization tests; `formatGithubAlert`'s existing logic (conditional `Reviewer:` line, generic action interpolation) already produced the exact expected strings. No production code changed | +| REL-002/RES-002 — claim+link exist but team row missing (route-github-event.ts:45-50) untested | Added a test asserting `routeGithubEvent` rejects with `NotFoundError` when `teamRepo.get` returns null despite a matching claim and link | **Passed immediately** — characterization test; the `if (!team) throw new NotFoundError(...)` branch was already implemented exactly this way. No production code changed | +| READ-001 — file name must mirror the export | Renamed `src/domain/usecases/link-repo.ts` → `link-repo-to-topic.ts` and `test/domain/link-repo.test.ts` → `link-repo-to-topic.test.ts` (plain `mv`, files were untracked); updated the import in the renamed test file; updated all references in `tasks.md` and this file | N/A — mechanical rename, no behavior change; full suite reverified green after the rename | +| REL-003 — actor-membership-not-found untested for link/unlink | Added a `NotFoundError` test to `link-repo-to-topic.test.ts` and `unlink-repo.test.ts`, mirroring the existing non-member test in `list-repo-links.test.ts` | **Passed immediately** — characterization tests; both use cases already did `const actor = await deps.membershipRepo.get(...); if (!actor) throw new NotFoundError(...)` before any role check. No production code changed | + +**No bugs were exposed.** Every new test in this correction passed on its first run — all five findings were untested gaps in coverage, not incorrect behavior. No production source file changed; only `test/fakes/index.ts` (added a `throws` option, same shape as existing fakes) and the five test files (new tests + one rename) changed, plus `tasks.md`/`apply-progress.md` reference updates. + +### New Test Count + +- Before this correction: 221 tests passing (initial PR1 apply) +- After the rename-only pass (before new tests): 221 passing, same count, different file name +- After all new tests: **229 tests passing** (+8: 1 in `link-repo-to-topic.test.ts`, 1 in `unlink-repo.test.ts`, 3 in `github.test.ts`, 3 in `route-github-event.test.ts`) +- `npx tsc --noEmit`: clean, no errors + +### Files Changed (this correction) + +| File | Action | What Was Done | +|------|--------|---------------| +| `test/fakes/index.ts` | Modified | Added `opts: { throws?: boolean }` to `fakeGithubOrgClaimRepo` and `fakeRepoTopicLinkRepo` | +| `src/domain/usecases/link-repo.ts` → `src/domain/usecases/link-repo-to-topic.ts` | Renamed | No content change beyond the rename | +| `test/domain/link-repo.test.ts` → `test/domain/link-repo-to-topic.test.ts` | Renamed + modified | Updated import path; added actor-not-found test | +| `test/domain/unlink-repo.test.ts` | Modified | Added actor-not-found test | +| `test/domain/github.test.ts` | Modified | Added reviewer-line and `merged` exact-output tests | +| `test/domain/route-github-event.test.ts` | Modified | Added two D1-unavailable propagation tests and one team-missing `NotFoundError` test | +| `openspec/changes/github-alerts/tasks.md` | Modified | Updated `link-repo.ts` references to `link-repo-to-topic.ts` | +| `openspec/changes/github-alerts/apply-progress.md` | Modified | This correction section; updated stale `link-repo.ts` references | + +### Remaining Tasks + +- [ ] Phase 2 (PR2): D1 adapters — `github-org-claim-repo.ts`, `repo-topic-link-repo.ts`, migration FK/UNIQUE/CHECK/isolation tests +- [ ] Phase 3 (PR3): `signature.ts`, route skeleton, env binding +- [ ] Phase 4 (PR4): mapper, alert sender, `buildGithubRouter`, e2e delivery tests +- [ ] Phase 5 (PR5): `/linkrepo`, `/unlinkrepo`, `/repos` commands +- [ ] Phase 6: operator rollout steps + +### Workload / PR Boundary + +- Mode: stacked-to-main, chained PR slice (PR1 of 5) +- Current work unit: Unit 1 — "Migration + domain (types, ports, errors, 4 use cases) with fakes" +- Boundary: starts from zero (first PR on `feat/github-alerts-domain`), ends at the four domain use cases + migration + fakes, all covered by passing tests +- Estimated review budget impact: **over budget** — see Issues Found above; needs an explicit `size:exception` or split decision before this is committed/pushed as an actual PR + +### Status + +9/9 Phase-1 tasks complete. The PR1 review correction above addresses all 5 frozen-ledger findings (RES-001, REL-001, REL-002/RES-002, READ-001, REL-003) — no bugs found, all new tests passed immediately, one mechanical rename applied. Full suite: `npx vitest run` → 229/229 pass. `npx tsc --noEmit` → clean, no errors. Ready for verify, with the PR-size risk called out above for the orchestrator/maintainer to resolve before commit. diff --git a/openspec/changes/github-alerts/tasks.md b/openspec/changes/github-alerts/tasks.md index 09b0f9d..9e61e83 100644 --- a/openspec/changes/github-alerts/tasks.md +++ b/openspec/changes/github-alerts/tasks.md @@ -26,17 +26,19 @@ Chain strategy: stacked-to-main | 4 | Mapper, alert sender, `buildGithubRouter`, end-to-end delivery + 500-on-D1-failure tests | PR4 (~300) | `npm test -- test/http` | `SELF.fetch` + `vi.stubGlobal(fetch)` | delete `src/adapters/github/event-mapper.ts`, `src/adapters/telegram/alert-sender.ts` | | 5 | `/linkrepo`, `/unlinkrepo`, `/repos` commands + tests | PR5 (~250) | `npm test -- test/adapters/telegram/commands.test.ts` | grammY stub, `SELF.fetch` | revert `commands.ts` command registration | +**PR1 actual size (measured `git diff --stat`, intent-to-add, after apply): 871 authored lines (16 files, 0 deletions) — exceeds the 400-line budget and the ~350 estimate above.** Implementation-only lines (migration, `entities.ts`/`errors.ts`/`ports.ts` additions, `github.ts`, 4 use cases, `migrations.test.ts` table-list fix) total ~356, under budget; the overrun comes entirely from the Strict-TDD RED test files (`test/domain/{github,link-repo-to-topic,unlink-repo,list-repo-links,route-github-event}.test.ts`, 451 lines) plus the three new port fakes in `test/fakes/index.ts` (64 lines). All code is written and every test is green (see apply-progress.md). Flagged for the orchestrator/maintainer to decide before this is committed as PR1: accept as `size:exception`, or split into two chained slices (1a: migration + entities/errors/ports + `github.ts` + `link-repo-to-topic`/`unlink-repo`/`list-repo-links` + their tests + fakes; 1b: `route-github-event.ts` + its test). No commit was made. + ## Phase 1: Domain Foundation (PR1) -- [ ] 1.1 RED: `github.ts` — `parseRepoFullName` (lowercase, `owner/repo` shape), `formatGithubAlert` truncation at 4096 (spec: Message Truncated). -- [ ] 1.2 GREEN: `src/domain/github.ts` types, `RepoFullName`, `GithubEvent`, `formatGithubAlert`. -- [ ] 1.3 Add `RepoTopicLink` entity; `GithubOrgClaimRepo`, `RepoTopicLinkRepo`, `AlertSender` ports; `InvalidRepoError`, `OrgNotClaimedError`, `AlertSendFailedError` in `entities.ts`/`ports.ts`/`errors.ts`. -- [ ] 1.4 RED: `link-repo-to-topic` tests — claimed org links, unclaimed org rejected, admin-only, must be inside a topic, re-link moves and reply names old/new topic (spec: repo-topic-links, all "Requirement" scenarios). -- [ ] 1.5 GREEN: `src/domain/usecases/link-repo.ts`. -- [ ] 1.6 RED/GREEN: `unlink-repo.ts`, `list-repo-links.ts` (spec: any member reads, excludes unclaimed-org links). -- [ ] 1.7 RED: `route-github-event` — unclaimed org ignored, unlinked repo ignored (no fallback), linked repo delivers, send failure returns `send-failed` kind not thrown (spec: github-alerts Route/Delivery-Failure). -- [ ] 1.8 GREEN: `src/domain/usecases/route-github-event.ts`. -- [ ] 1.9 `migrations/0002_github_alerts.sql` per design (claims + links, composite FK). +- [x] 1.1 RED: `github.ts` — `parseRepoFullName` (lowercase, `owner/repo` shape), `formatGithubAlert` truncation at 4096 (spec: Message Truncated). +- [x] 1.2 GREEN: `src/domain/github.ts` types, `RepoFullName`, `GithubEvent`, `formatGithubAlert`. +- [x] 1.3 Add `RepoTopicLink` entity; `GithubOrgClaimRepo`, `RepoTopicLinkRepo`, `AlertSender` ports; `InvalidRepoError`, `OrgNotClaimedError`, `AlertSendFailedError` in `entities.ts`/`ports.ts`/`errors.ts`. +- [x] 1.4 RED: `link-repo-to-topic` tests — claimed org links, unclaimed org rejected, admin-only, must be inside a topic, re-link moves and reply names old/new topic (spec: repo-topic-links, all "Requirement" scenarios). Note: the "must be inside a topic" and "reply names old/new topic" scenarios are adapter/command-layer concerns (Phase 5, `/linkrepo`); the domain use case tested here covers the claim gate, admin gate, and move-semantics (`previousThreadId`). +- [x] 1.5 GREEN: `src/domain/usecases/link-repo-to-topic.ts`. +- [x] 1.6 RED/GREEN: `unlink-repo.ts`, `list-repo-links.ts` (spec: any member reads, excludes unclaimed-org links). +- [x] 1.7 RED: `route-github-event` — unclaimed org ignored, unlinked repo ignored (no fallback), linked repo delivers, send failure returns `send-failed` kind not thrown (spec: github-alerts Route/Delivery-Failure). +- [x] 1.8 GREEN: `src/domain/usecases/route-github-event.ts`. +- [x] 1.9 `migrations/0002_github_alerts.sql` per design (claims + links, composite FK). ## Phase 2: D1 Adapters (PR2)