diff --git a/openspec/changes/github-alerts/apply-progress.md b/openspec/changes/github-alerts/apply-progress.md index ae73ac9..397876c 100644 --- a/openspec/changes/github-alerts/apply-progress.md +++ b/openspec/changes/github-alerts/apply-progress.md @@ -122,3 +122,124 @@ Applied on `feat/github-alerts-domain`, still no commit/push, `.codegraph/` unto ### 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. + +## PR2 — D1 Adapters (Phase 2) + +**Mode**: Strict TDD, enforced incrementally — one behavior at a time (not the PR1 batch-RED pattern). For every new query/branch, a test was added and run *before* the code that satisfies it existed or before the code handled that branch, so each RED is either "module not found" (only ever the very first test in a file) or a genuine assertion failure against already-existing code (missing scoping, PK conflict, stale-row leakage). Still on `feat/github-alerts-d1` (from `main` at `cee64b9`, includes PR1). No commit made — working tree only, per instruction. `.codegraph/` untouched. + +### Completed Tasks + +- [x] 2.1 RED: migration test — table/FK/UNIQUE/CHECK enforcement, cross-team isolation on links +- [x] 2.2 GREEN: `src/adapters/d1/github-org-claim-repo.ts`, `repo-topic-link-repo.ts` + +### Files Changed + +| File | Action | What Was Done | +|------|--------|---------------| +| `test/adapters/migrations.test.ts` | Modified | Added a `describe("migrations/0002_github_alerts.sql")` block: CHECK (org_login lowercase, repo_full_name lowercase), FK (claim→teams, link composite FK→claims), UNIQUE/PK (one org one team, one topic per repo per team), and a raw-SQL cross-team isolation check on `repo_topic_links` | +| `src/adapters/d1/github-org-claim-repo.ts` | Created | `createD1GithubOrgClaimRepo(db)` — `findTeamByOrg` (case-insensitive match against the lowercase-stored `org_login`), `isClaimedBy` (team-scoped exact match) | +| `test/adapters/d1/github-org-claim-repo.test.ts` | Created | 6 tests: not-found, found, case-insensitive lookup, claimed-true, tenant isolation (false for a non-claiming team), no-claim-at-all false | +| `src/adapters/d1/repo-topic-link-repo.ts` | Created | `createD1RepoTopicLinkRepo(db)` — `get`/`upsert` (`ON CONFLICT (team_id, repo_full_name) DO UPDATE SET thread_id, updated_at` — re-link moves it)/`remove` (`meta.changes === 1`)/`list`, all team-scoped | +| `test/adapters/d1/repo-topic-link-repo.test.ts` | Created | 10 tests: get not-found/found/tenant-isolated, upsert-insert/upsert-moves-existing, remove-found/remove-not-found, list-empty/list-all/list-tenant-isolated | + +### TDD Cycle Evidence (one behavior at a time; each row is its own run) + +| Behavior | RED (ran before the fix, failed for the stated reason) | GREEN (code added/changed, then passed) | +|---|---|---| +| `findTeamByOrg` not-found | Module missing: `Cannot find module '.../github-org-claim-repo'` | Created file, `findTeamByOrg` hardcoded to `null`, `isClaimedBy` throws `"not implemented"` → 1/1 pass | +| `findTeamByOrg` exact match | Assertion failure: `expected null to be 'team-org-1'` (hardcoded-null stub from the previous step) | Implemented real `SELECT team_id FROM github_org_claims WHERE org_login = ?` → 2/2 pass | +| `findTeamByOrg` case-insensitive | Assertion failure: `expected null to be 'team-org-2'` (exact-match query does not match `"Mixed-Org"` against stored `"mixed-org"`) | Added `.toLowerCase()` on the input → 3/3 pass | +| `isClaimedBy` claimed-true | Threw `Error: not implemented` (stub from step 1) | Implemented `SELECT 1 FROM github_org_claims WHERE org_login = ?` (deliberately not yet team-scoped) → 4/4 pass | +| `isClaimedBy` tenant isolation | Assertion failure: `expected true to be false` — the not-yet-team-scoped query from the previous step matched another team's claim on the same org | Added `AND team_id = ?` to the query → 6/6 pass (isolation + no-claim-at-all both green) | +| `get` not-found | Module missing: `Cannot find module '.../repo-topic-link-repo'` | Created file, `get` hardcoded to `null`, `upsert`/`remove`/`list` throw `"not implemented"` → 1/1 pass | +| `get` found | Assertion failure: `expected {...} to equal null` reversed — actually `expected null` vs the full row (hardcoded-null stub) | Implemented `SELECT * FROM repo_topic_links WHERE repo_full_name = ?` (deliberately not yet team-scoped) → 2/2 pass | +| `get` tenant isolation | Assertion failure: `expected {...} to be null` — the not-yet-team-scoped query returned the owning team's row for a different team's lookup | Added `AND team_id = ?` to the query → 3/3 pass | +| `upsert` insert | Threw `Error: not implemented` | Implemented a plain `INSERT` (no `ON CONFLICT` yet) → 4/4 pass | +| `upsert` move (re-link) | Threw `SQLITE_CONSTRAINT_PRIMARYKEY`: `UNIQUE constraint failed: repo_topic_links.team_id, repo_topic_links.repo_full_name` — the plain `INSERT` from the previous step cannot re-link | Added `ON CONFLICT (team_id, repo_full_name) DO UPDATE SET thread_id = excluded.thread_id, updated_at = excluded.updated_at` → 5/5 pass | +| `remove` found | Threw `Error: not implemented` | Implemented `DELETE ... WHERE team_id = ? AND repo_full_name = ?` returning `meta.changes === 1` → 6/6 pass | +| `remove` not-found (idempotent) | **Passed immediately** — characterization test; the `meta.changes === 1` check from the previous step already returns `false` when nothing matched. No production code changed | N/A | +| `list` empty | Threw `Error: not implemented` | Implemented `SELECT * FROM repo_topic_links` (deliberately no `WHERE` yet) → then failed again in the same run: `expected [] to equal [{...}]` — rows from earlier tests in the same D1-backed file leaked in, a genuine (if incidental) demonstration of the missing tenant scope. Added `WHERE team_id = ?` → 8/8 pass | +| `list` all-for-team, `list` tenant isolation | **Both passed immediately** — characterization tests; the `WHERE team_id = ?` fix from the previous step already covers them. No production code changed | N/A | +| Migration constraint tests (2.1, all 7) | **All passed immediately** — `migrations/0002_github_alerts.sql` was already shipped and applied in PR1 (task 1.9); these tests verify existing SQL against `env.DB` directly, not new production code in this PR. There is no RED→GREEN cycle for schema that already exists; this row is recorded for transparency rather than claimed as TDD-driven new code | N/A | + +Every RED that involved a genuine behavior gap failed either on module resolution (only ever the first test in each file, never reused for a later behavior — per instruction, "a missing module is not an acceptable RED for behavior beyond the first test") or on a real assertion/constraint failure against code that already existed. The rows marked "passed immediately" are disclosed as characterization tests, not RED-driven, consistent with the same honest framing used in the PR1 review-correction section above. + +### Work Unit Evidence (PR2) + +| Evidence | Value | +|---|---| +| Focused test command and exact result | `npx vitest run test/adapters/d1` → 16/16 pass (6 `github-org-claim-repo.test.ts` + 10 `repo-topic-link-repo.test.ts`); `npx vitest run test/adapters/migrations.test.ts` → 15/15 pass (8 pre-existing + 7 new 0002 tests) | +| Runtime harness command/scenario and exact result | vitest-pool-workers D1 (`env.DB` from `cloudflare:test`), same harness as `test/adapters/d1/team-repo.test.ts`. Full suite: `npx vitest run` → 252/252 pass (32 files, up from 229/30 before this PR) | +| Rollback boundary | Delete `src/adapters/d1/{github-org-claim-repo,repo-topic-link-repo}.ts` and `test/adapters/d1/{github-org-claim-repo,repo-topic-link-repo}.test.ts`; revert the additive `describe` block appended to `test/adapters/migrations.test.ts`. Nothing outside these files imports the two new adapters yet — composition wiring is explicitly out of scope for PR2 per tasks.md Phase 2 | + +### Deviations from Design + +- None. `github-org-claim-repo.ts`/`repo-topic-link-repo.ts` match design.md's "Interfaces / Contracts" signatures verbatim; `upsert` uses the exact `ON CONFLICT (team_id, repo_full_name) DO UPDATE SET thread_id = excluded.thread_id, updated_at = excluded.updated_at` clause called for in design.md ("ON CONFLICT DO UPDATE thread_id") and ports.ts. One addition beyond the literal port signature, not a deviation from it: `findTeamByOrg` lowercases its input before querying, since `GithubOrgClaimRepo` is documented as the sole cross-team lookup that receives an org login before any case normalization can have happened upstream (the Phase-4 mapper, which normalizes `GithubEvent.org`, does not exist yet), while claims are stored lowercase by the migration's CHECK constraint. `isClaimedBy` receives its `orgLogin` argument already lowercase from `link-repo-to-topic.ts`'s `orgLoginFromRepo` (derived from the branded, already-lowercase `RepoFullName`), so the same `.toLowerCase()` there is a no-op safety net, not a behavior change. + +### Issues Found / Risks + +- None. Production code is small (112 lines total across both adapters), no D1 error-translation was needed beyond the existing constraint set (the composite FK on `repo_topic_links` is enforced by the migration itself; nothing in this PR's use-case-facing methods needs to catch and re-translate a constraint violation, since `link-repo-to-topic.ts` already checks `isClaimedBy` before calling `upsert`). + +### New Test Count + +- Before this PR: 229 tests passing (30 files) +- After this PR: **252 tests passing** (32 files) — +23 (7 migration constraint tests + 6 `github-org-claim-repo.test.ts` + 10 `repo-topic-link-repo.test.ts`) +- `npx tsc --noEmit`: clean, no errors + +### Line Counts + +| Category | Files | Lines | +|---|---|---| +| Production | `src/adapters/d1/github-org-claim-repo.ts` (32) + `repo-topic-link-repo.ts` (80) | **112** | +| Tests | `test/adapters/d1/github-org-claim-repo.test.ts` (89) + `repo-topic-link-repo.test.ts` (232) + `migrations.test.ts` additions (145, per `git diff --stat`) | **466** | + +Production code is well under the 400-line budget on its own (112 lines vs. the ~250 estimate in tasks.md). The test-heavy total (466 lines) is the `size:exception` the user pre-accepted for test overrun; no code was cut and no split was needed. + +### Workload / PR Boundary + +- Mode: stacked-to-main, chained PR slice (PR2 of 5), stacked on `feat/github-alerts-d1` (from `main` at `cee64b9`, includes PR1) +- Current work unit: Unit 2 — "D1 repos: claims, links, FK/isolation tests" +- Boundary: starts from PR1 (domain layer + migration, unchanged in this PR), ends at the two D1 adapters implementing `GithubOrgClaimRepo`/`RepoTopicLinkRepo`, all covered by passing tests. Not wired into `composition.ts` — that is explicitly Phase 4 (task 4.5) +- Estimated review budget impact: production code is comfortably within budget; test overrun is pre-accepted per instruction + +### Status + +2/2 Phase-2 tasks complete. Full suite: `npx vitest run` → 252/252 pass. `npx tsc --noEmit` → clean, no errors. No composition wiring performed (out of scope). Ready for verify / review. Remaining: Phase 3 (`signature.ts`, route skeleton, env binding), Phase 4 (mapper, alert sender, `buildGithubRouter`, e2e delivery), Phase 5 (`/linkrepo`, `/unlinkrepo`, `/repos` commands), Phase 6 (operator rollout). + +## Correction — PR2 Review Ledger (frozen findings) + +Applied on `feat/github-alerts-d1`, still no commit/push, `.codegraph/` untouched. Fixes the confirmed findings from the frozen PR2 review ledger. Task 6.3 in tasks.md was already checked (operator claimed the org in production) and left unchanged. + +### Findings Addressed + +| Finding | Fix | RED evidence | New test result | +|---|---|---|---| +| RISK-001 (CRITICAL) / REL-002 / READ-001 — `upsert(teamId, link)` ignored the `teamId` argument and wrote under `link.teamId`, a silent cross-tenant write if a caller ever passed mismatched values | `src/adapters/d1/repo-topic-link-repo.ts`: `teamId` argument is now authoritative — bound directly into the SQL instead of `link.teamId`, and a mismatch throws a new `TenantMismatchError` (added to `src/domain/errors.ts`) before touching the database. `test/fakes/index.ts`'s `fakeRepoTopicLinkRepo.upsert` aligned to the identical check | D1 test (`test/adapters/d1/repo-topic-link-repo.test.ts`): ran against the pre-fix code — `promise resolved "undefined" instead of rejecting` (it silently wrote under `link.teamId` instead of rejecting). Fake test (`test/domain/link-repo-to-topic.test.ts`): same failure mode against the pre-fix fake | **Real bug found and fixed.** Both tests failed for the right reason before the fix, passed after. `npx vitest run test/adapters/d1/repo-topic-link-repo.test.ts` → 13/13; `npx vitest run test/domain/link-repo-to-topic.test.ts` → 6/6 | +| REL-003 — no test proved `remove` was tenant-scoped | Added `remove is tenant-scoped: remove(teamB, repo) does not delete teamA's link for the same repo path` to `repo-topic-link-repo.test.ts` | **Passed immediately** — characterization test; `remove`'s existing `WHERE team_id = ? AND repo_full_name = ?` (written in the original PR2 apply, before this correction) already scoped correctly. No production code changed | +| REL-001 — `fakeGithubOrgClaimRepo.isClaimedBy`/`findTeamByOrg` did exact-string org matching, while the real D1 adapter lowercases both lookups; a domain test could pass against the fake on a case assumption the real adapter would reject | `test/fakes/index.ts`: both fake methods now call `orgLogin.toLowerCase()` before matching, mirroring `createD1GithubOrgClaimRepo`. Added a D1 test (`isClaimedBy matches regardless of the input's case, exactly like findTeamByOrg`) to `github-org-claim-repo.test.ts`, and a new `describe("fakeGithubOrgClaimRepo (contract parity with the D1 adapter)")` block to `route-github-event.test.ts` for the fake | D1 test: **passed immediately** — the adapter already lowercased (written in the original PR2 apply). Fake tests: ran against the pre-fix fake — `expected true to be false` / `expected null to be 'team-1'` (exact-match fake rejected the differently-cased input) | D1: no change needed, already correct. **Fake bug found and fixed** — the two new fake-parity tests failed for the right reason before the fix, passed after. `npx vitest run test/adapters/d1/github-org-claim-repo.test.ts` → 8/8; `npx vitest run test/domain/route-github-event.test.ts` → 9/9 | +| RES-001 — no test proved a raw D1 failure (not a domain-level "not found") propagates rather than being swallowed | Added a `failingDb()` stub (implements only `D1Database.prepare()`, returning a statement whose `bind/first/run/all` all reject) to both `test/adapters/d1/github-org-claim-repo.test.ts` and `repo-topic-link-repo.test.ts`, and one test per adapter (`findTeamByOrg` / `get`) asserting the rejection propagates | N/A — see below | **Passed immediately** — characterization tests; neither adapter has a try/catch around its D1 calls, so an `await` on a rejecting D1 call already rejects the caller. No production code changed. This is the cleanest available way to inject a D1-level failure into these adapters without touching the shared `env.DB` used by every other test in the same file — a real `env.DB` failure (e.g. a closed connection) is not something the Workers `cloudflare:test` binding exposes for deliberate breakage, so a structural stub at the `D1Database` interface boundary (only `prepare()` is called by these methods) was used instead of forcing an artificial constraint violation that would conflate "D1 unavailable" with "expected schema rejection" | + +**One real bug found**: RISK-001/REL-002/READ-001 — `upsert` silently wrote a link under `link.teamId` instead of the tenant-scoped `teamId` argument, which a caller bug could have used to write into another team's tenant. Fixed with an explicit rejection (`TenantMismatchError`) rather than a silent correction, so a caller passing mismatched values learns about its bug instead of the write being quietly redirected. One fake bug found (REL-001): the in-memory fakes did exact-case org matching, diverging from the real adapter's lowercase normalization — fixed to match. All other findings were confirmed-correct-but-untested behavior; the new tests pin that down without any production code change. + +### New Test Count + +- Before this correction: 252 tests passing (32 files) +- After this correction: **260 tests passing** (32 files, same file count — no new test files, only additions to existing ones) — +8 (1 D1 mismatch-rejection test + 1 fake mismatch-rejection test + 1 D1 tenant-isolation-for-remove test + 1 D1 case-insensitive-isClaimedBy test + 2 fake case-parity tests + 2 D1-failure-propagation tests) +- `npx tsc --noEmit`: clean, no errors + +### Files Changed (this correction) + +| File | Action | What Was Done | +|------|--------|---------------| +| `src/domain/errors.ts` | Modified | Added `TenantMismatchError` | +| `src/adapters/d1/repo-topic-link-repo.ts` | Modified | `upsert` now binds `teamId` (the argument) instead of `link.teamId`, and rejects with `TenantMismatchError` on a mismatch before any database write | +| `test/fakes/index.ts` | Modified | `fakeRepoTopicLinkRepo.upsert` rejects on the same `teamId`/`link.teamId` mismatch; `fakeGithubOrgClaimRepo.findTeamByOrg`/`isClaimedBy` lowercase their `orgLogin` input | +| `test/adapters/d1/repo-topic-link-repo.test.ts` | Modified | Added the mismatch-rejection test, the `remove` tenant-isolation test, the `failingDb()` stub, and the `get`-propagates-D1-failure test | +| `test/adapters/d1/github-org-claim-repo.test.ts` | Modified | Added the `isClaimedBy` case-insensitivity test, the `failingDb()` stub, and the `findTeamByOrg`-propagates-D1-failure test | +| `test/domain/link-repo-to-topic.test.ts` | Modified | Added the `fakeRepoTopicLinkRepo` mismatch-rejection parity test | +| `test/domain/route-github-event.test.ts` | Modified | Added the `fakeGithubOrgClaimRepo` case-insensitivity parity tests | +| `openspec/changes/github-alerts/apply-progress.md` | Modified | This correction section | + +### Status (after correction) + +All 4 confirmed PR2 review findings addressed. Full suite: `npx vitest run` → 260/260 pass. `npx tsc --noEmit` → clean, no errors. Two genuine bugs found and fixed (the `upsert` tenant-scoping bug in production code, and the case-matching divergence in the fake); the remaining findings were untested-but-correct behavior, now pinned down by tests. No commit/push made; `.codegraph/` untouched; task 6.3 left checked as instructed. diff --git a/openspec/changes/github-alerts/tasks.md b/openspec/changes/github-alerts/tasks.md index 9e61e83..3b9f723 100644 --- a/openspec/changes/github-alerts/tasks.md +++ b/openspec/changes/github-alerts/tasks.md @@ -28,6 +28,8 @@ Chain strategy: stacked-to-main **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. +**PR2 actual size (measured `wc -l`/`git diff --stat`, after apply): production code (`src/adapters/d1/{github-org-claim-repo,repo-topic-link-repo}.ts`) is 112 lines, well under the 400-line budget and the ~250 estimate. Test code (new `test/adapters/d1/{github-org-claim-repo,repo-topic-link-repo}.test.ts` plus 145 added lines in `test/adapters/migrations.test.ts`) totals 466 lines — the size:exception the user pre-accepted for test overrun. No commit was made. + ## Phase 1: Domain Foundation (PR1) - [x] 1.1 RED: `github.ts` — `parseRepoFullName` (lowercase, `owner/repo` shape), `formatGithubAlert` truncation at 4096 (spec: Message Truncated). @@ -42,8 +44,8 @@ Chain strategy: stacked-to-main ## Phase 2: D1 Adapters (PR2) -- [ ] 2.1 RED: migration test — table/FK/UNIQUE/CHECK enforcement, cross-team isolation on links. -- [ ] 2.2 GREEN: `src/adapters/d1/github-org-claim-repo.ts`, `repo-topic-link-repo.ts` (upsert `ON CONFLICT DO UPDATE thread_id`). +- [x] 2.1 RED: migration test — table/FK/UNIQUE/CHECK enforcement, cross-team isolation on links. +- [x] 2.2 GREEN: `src/adapters/d1/github-org-claim-repo.ts`, `repo-topic-link-repo.ts` (upsert `ON CONFLICT DO UPDATE thread_id`). ## Phase 3: Signature and Route Skeleton (PR3) @@ -71,4 +73,4 @@ Chain strategy: stacked-to-main - [ ] 6.1 (after PR3 merges) `npx wrangler secret put GITHUB_WEBHOOK_SECRET`. - [ ] 6.2 (after PR4 merges) Configure org webhook: content type `application/json`, same secret, Pull requests + Issues events; verify ping returns 200. -- [ ] 6.3 (after PR1 merges, before PR5's `/linkrepo` is used) Claim the org via `wrangler d1 execute` insert into `github_org_claims`. +- [x] 6.3 (after PR1 merges, before PR5's `/linkrepo` is used) Claim the org via `wrangler d1 execute` insert into `github_org_claims`. diff --git a/src/adapters/d1/github-org-claim-repo.ts b/src/adapters/d1/github-org-claim-repo.ts new file mode 100644 index 0000000..2650aa9 --- /dev/null +++ b/src/adapters/d1/github-org-claim-repo.ts @@ -0,0 +1,32 @@ +import { asTeamId } from "../../domain/ids"; +import type { TeamId } from "../../domain/ids"; +import type { GithubOrgClaimRepo } from "../../domain/ports"; + +interface ClaimRow { + team_id: string; +} + +export function createD1GithubOrgClaimRepo(db: D1Database): GithubOrgClaimRepo { + return { + async findTeamByOrg(orgLogin: string): Promise { + // Claims are stored lowercase (CHECK constraint, migrations/0002). + // GitHub sends the org's display case in webhook payloads, so the + // lookup normalizes the input to match. + const row = await db + .prepare("SELECT team_id FROM github_org_claims WHERE org_login = ?") + .bind(orgLogin.toLowerCase()) + .first(); + return row ? asTeamId(row.team_id) : null; + }, + + async isClaimedBy(teamId: TeamId, orgLogin: string): Promise { + const row = await db + .prepare( + "SELECT 1 FROM github_org_claims WHERE team_id = ? AND org_login = ?", + ) + .bind(teamId, orgLogin.toLowerCase()) + .first(); + return row !== null; + }, + }; +} diff --git a/src/adapters/d1/repo-topic-link-repo.ts b/src/adapters/d1/repo-topic-link-repo.ts new file mode 100644 index 0000000..ad25102 --- /dev/null +++ b/src/adapters/d1/repo-topic-link-repo.ts @@ -0,0 +1,91 @@ +import type { RepoTopicLink } from "../../domain/entities"; +import { TenantMismatchError } from "../../domain/errors"; +import type { RepoFullName } from "../../domain/github"; +import { asTeamId } from "../../domain/ids"; +import type { TeamId } from "../../domain/ids"; +import type { RepoTopicLinkRepo } from "../../domain/ports"; + +interface LinkRow { + team_id: string; + repo_full_name: string; + org_login: string; + thread_id: number; + created_at: number; + updated_at: number; +} + +function rowToLink(row: LinkRow): RepoTopicLink { + return { + teamId: asTeamId(row.team_id), + repoFullName: row.repo_full_name as RepoFullName, + orgLogin: row.org_login, + threadId: row.thread_id, + createdAt: row.created_at, + updatedAt: row.updated_at, + }; +} + +export function createD1RepoTopicLinkRepo(db: D1Database): RepoTopicLinkRepo { + return { + async get(teamId: TeamId, repo: RepoFullName): Promise { + const row = await db + .prepare( + "SELECT * FROM repo_topic_links WHERE team_id = ? AND repo_full_name = ?", + ) + .bind(teamId, repo) + .first(); + return row ? rowToLink(row) : null; + }, + + async upsert(teamId: TeamId, link: RepoTopicLink): Promise { + // The explicit `teamId` argument is authoritative (ports.ts "Tenancy": + // every tenant-scoped method takes TeamId first) — a caller passing a + // `link.teamId` that disagrees with it is a bug, and MUST NOT silently + // write under the mismatched team (RISK-001/REL-002/READ-001). + if (teamId !== link.teamId) { + throw new TenantMismatchError( + `RepoTopicLinkRepo.upsert: teamId argument ("${teamId}") does not match link.teamId ("${link.teamId}")`, + ); + } + + // Re-linking an already-linked repo moves it instead of erroring on + // the (team_id, repo_full_name) PK conflict (design.md "One Topic Per + // Repo, Re-Link Moves It" — ports.ts RepoTopicLinkRepo.upsert). + await db + .prepare( + `INSERT INTO repo_topic_links + (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT (team_id, repo_full_name) + DO UPDATE SET thread_id = excluded.thread_id, updated_at = excluded.updated_at`, + ) + .bind( + teamId, + link.repoFullName, + link.orgLogin, + link.threadId, + link.createdAt, + link.updatedAt, + ) + .run(); + }, + + async remove(teamId: TeamId, repo: RepoFullName): Promise { + const result = await db + .prepare( + "DELETE FROM repo_topic_links WHERE team_id = ? AND repo_full_name = ?", + ) + .bind(teamId, repo) + .run(); + return result.meta.changes === 1; + }, + + async list(teamId: TeamId): Promise { + const rows = await db + .prepare("SELECT * FROM repo_topic_links WHERE team_id = ?") + .bind(teamId) + .all(); + return rows.results.map(rowToLink); + }, + }; +} diff --git a/src/domain/errors.ts b/src/domain/errors.ts index b0d9fea..2d14f28 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -20,3 +20,9 @@ export class OrgNotClaimedError extends DomainError {} // "send-failed" outcome instead of letting it propagate (design.md // "GitHub route status policy"). export class AlertSendFailedError extends DomainError {} +// Thrown when a tenant-scoped write's explicit `teamId` argument disagrees +// with the `teamId` embedded in the entity being written (e.g. +// RepoTopicLinkRepo.upsert). The explicit argument is always authoritative +// (design.md/ports.ts "Tenancy": every tenant-scoped method takes TeamId +// first) — this is a caller bug, never a silent cross-tenant write. +export class TenantMismatchError extends DomainError {} diff --git a/test/adapters/d1/github-org-claim-repo.test.ts b/test/adapters/d1/github-org-claim-repo.test.ts new file mode 100644 index 0000000..9258be7 --- /dev/null +++ b/test/adapters/d1/github-org-claim-repo.test.ts @@ -0,0 +1,134 @@ +import { env } from "cloudflare:test"; +import { describe, expect, it } from "vitest"; +import { createD1GithubOrgClaimRepo } from "../../../src/adapters/d1/github-org-claim-repo"; +import { asTeamId } from "../../../src/domain/ids"; + +// RES-001: proves a D1 failure propagates (rejects) rather than being +// swallowed. No adapter method here has a try/catch, so this is really +// characterizing "await on a rejecting D1 call rejects the caller" — but it +// is worth pinning down explicitly, since a future refactor adding +// try/catch (e.g. for constraint-error translation, as team-repo.ts does) +// could accidentally swallow this. The stub only implements `prepare`, +// which is all these two methods call. +function failingDb(message = "D1_ERROR: simulated D1 outage"): D1Database { + const err = new Error(message); + const statement = { + bind: () => statement, + first: async () => { + throw err; + }, + run: async () => { + throw err; + }, + all: async () => { + throw err; + }, + }; + return { prepare: () => statement } as unknown as D1Database; +} + +async function seedTeam(teamId: string, chatId: number) { + await env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ) + .bind(teamId, chatId, 0) + .run(); +} + +async function seedClaim(orgLogin: string, teamId: string) { + await env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ) + .bind(orgLogin, teamId, 0) + .run(); +} + +describe("createD1GithubOrgClaimRepo", () => { + it("findTeamByOrg returns null when no claim exists for the org", async () => { + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.findTeamByOrg("no-such-org"); + + expect(result).toBeNull(); + }); + + it("findTeamByOrg returns the claiming team's id when a claim exists", async () => { + await seedTeam("team-org-1", 701); + await seedClaim("acme-corp", "team-org-1"); + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.findTeamByOrg("acme-corp"); + + expect(result).toBe(asTeamId("team-org-1")); + }); + + it("findTeamByOrg matches regardless of the input's case (claims are stored lowercase, GitHub sends the org's display case)", async () => { + await seedTeam("team-org-2", 702); + await seedClaim("mixed-org", "team-org-2"); + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.findTeamByOrg("Mixed-Org"); + + expect(result).toBe(asTeamId("team-org-2")); + }); + + it("isClaimedBy returns true when the team has a claim for the org", async () => { + await seedTeam("team-claimed-1", 703); + await seedClaim("claimed-org-1", "team-claimed-1"); + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.isClaimedBy( + asTeamId("team-claimed-1"), + "claimed-org-1", + ); + + expect(result).toBe(true); + }); + + it("isClaimedBy returns false for a team that did not claim the org, even when another team did (tenant isolation)", async () => { + await seedTeam("team-claimed-owner", 704); + await seedTeam("team-claimed-other", 705); + await seedClaim("claimed-org-2", "team-claimed-owner"); + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.isClaimedBy( + asTeamId("team-claimed-other"), + "claimed-org-2", + ); + + expect(result).toBe(false); + }); + + it("isClaimedBy matches regardless of the input's case, exactly like findTeamByOrg (REL-001)", async () => { + await seedTeam("team-claimed-case", 707); + await seedClaim("case-org", "team-claimed-case"); + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.isClaimedBy( + asTeamId("team-claimed-case"), + "Case-Org", + ); + + expect(result).toBe(true); + }); + + it("isClaimedBy returns false when no claim exists at all", async () => { + await seedTeam("team-claimed-none", 706); + const repo = createD1GithubOrgClaimRepo(env.DB); + + const result = await repo.isClaimedBy( + asTeamId("team-claimed-none"), + "never-claimed-org", + ); + + expect(result).toBe(false); + }); + + it("findTeamByOrg propagates (rejects) when the D1 query fails, instead of swallowing the error (RES-001)", async () => { + const repo = createD1GithubOrgClaimRepo(failingDb()); + + await expect(repo.findTeamByOrg("any-org")).rejects.toThrow( + "D1_ERROR: simulated D1 outage", + ); + }); +}); diff --git a/test/adapters/d1/repo-topic-link-repo.test.ts b/test/adapters/d1/repo-topic-link-repo.test.ts new file mode 100644 index 0000000..29b7627 --- /dev/null +++ b/test/adapters/d1/repo-topic-link-repo.test.ts @@ -0,0 +1,319 @@ +import { env } from "cloudflare:test"; +import { describe, expect, it } from "vitest"; +import { createD1RepoTopicLinkRepo } from "../../../src/adapters/d1/repo-topic-link-repo"; +import { TenantMismatchError } from "../../../src/domain/errors"; +import { parseRepoFullName } from "../../../src/domain/github"; +import { asTeamId } from "../../../src/domain/ids"; + +async function seedTeam(teamId: string, chatId: number) { + await env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ) + .bind(teamId, chatId, 0) + .run(); +} + +async function seedClaim(orgLogin: string, teamId: string) { + await env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ) + .bind(orgLogin, teamId, 0) + .run(); +} + +// RES-001: proves a D1 failure propagates (rejects) rather than being +// swallowed — mirrors the same stub used in github-org-claim-repo.test.ts. +// None of these adapter methods has a try/catch, so this pins down that an +// unavailable D1 rejects the caller rather than being silently absorbed. +function failingDb(message = "D1_ERROR: simulated D1 outage"): D1Database { + const err = new Error(message); + const statement = { + bind: () => statement, + first: async () => { + throw err; + }, + run: async () => { + throw err; + }, + all: async () => { + throw err; + }, + }; + return { prepare: () => statement } as unknown as D1Database; +} + +function repoFullName(raw: string) { + const parsed = parseRepoFullName(raw); + if (!parsed) throw new Error(`invalid test fixture repo: ${raw}`); + return parsed; +} + +describe("createD1RepoTopicLinkRepo", () => { + it("get returns null when no link exists for the repo", async () => { + const repo = createD1RepoTopicLinkRepo(env.DB); + + const result = await repo.get(asTeamId("team-nolink"), repoFullName("acme/none")); + + expect(result).toBeNull(); + }); + + it("get returns the link when one exists for the team and repo", async () => { + await seedTeam("team-get-1", 801); + await seedClaim("get-org", "team-get-1"); + await env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind("team-get-1", "get-org/repo", "get-org", 55, 10, 20) + .run(); + const repo = createD1RepoTopicLinkRepo(env.DB); + + const result = await repo.get(asTeamId("team-get-1"), repoFullName("get-org/repo")); + + expect(result).toEqual({ + teamId: asTeamId("team-get-1"), + repoFullName: repoFullName("get-org/repo"), + orgLogin: "get-org", + threadId: 55, + createdAt: 10, + updatedAt: 20, + }); + }); + + it("get is tenant-scoped: a link with the same repo path under a different team is not returned (RES-002/tenant isolation)", async () => { + await seedTeam("team-get-owner", 802); + await seedTeam("team-get-other", 803); + await seedClaim("shared-get-org", "team-get-owner"); + await env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind("team-get-owner", "shared-get-org/repo", "shared-get-org", 1, 0, 0) + .run(); + const repo = createD1RepoTopicLinkRepo(env.DB); + + const result = await repo.get( + asTeamId("team-get-other"), + repoFullName("shared-get-org/repo"), + ); + + expect(result).toBeNull(); + }); + + it("upsert inserts a new link when none exists for the (team, repo)", async () => { + await seedTeam("team-upsert-1", 810); + await seedClaim("upsert-org", "team-upsert-1"); + const repo = createD1RepoTopicLinkRepo(env.DB); + const link = { + teamId: asTeamId("team-upsert-1"), + repoFullName: repoFullName("upsert-org/repo"), + orgLogin: "upsert-org", + threadId: 111, + createdAt: 1000, + updatedAt: 1000, + }; + + await repo.upsert(asTeamId("team-upsert-1"), link); + + const stored = await repo.get( + asTeamId("team-upsert-1"), + repoFullName("upsert-org/repo"), + ); + expect(stored).toEqual(link); + }); + + it("upsert moves an already-linked repo to a new thread instead of erroring on the PK conflict (re-link semantics)", async () => { + await seedTeam("team-upsert-2", 811); + await seedClaim("upsert-org-2", "team-upsert-2"); + const repo = createD1RepoTopicLinkRepo(env.DB); + await repo.upsert(asTeamId("team-upsert-2"), { + teamId: asTeamId("team-upsert-2"), + repoFullName: repoFullName("upsert-org-2/repo"), + orgLogin: "upsert-org-2", + threadId: 200, + createdAt: 1000, + updatedAt: 1000, + }); + + await repo.upsert(asTeamId("team-upsert-2"), { + teamId: asTeamId("team-upsert-2"), + repoFullName: repoFullName("upsert-org-2/repo"), + orgLogin: "upsert-org-2", + threadId: 300, + createdAt: 1000, + updatedAt: 2000, + }); + + const moved = await repo.get( + asTeamId("team-upsert-2"), + repoFullName("upsert-org-2/repo"), + ); + expect(moved).toEqual({ + teamId: asTeamId("team-upsert-2"), + repoFullName: repoFullName("upsert-org-2/repo"), + orgLogin: "upsert-org-2", + threadId: 300, + createdAt: 1000, + updatedAt: 2000, + }); + }); + + it("upsert rejects (and writes nothing) when the teamId argument disagrees with link.teamId (RISK-001/REL-002/READ-001: teamId argument must be authoritative)", async () => { + await seedTeam("team-mismatch-a", 812); + await seedTeam("team-mismatch-b", 813); + await seedClaim("mismatch-org", "team-mismatch-b"); + const repo = createD1RepoTopicLinkRepo(env.DB); + + await expect( + repo.upsert(asTeamId("team-mismatch-a"), { + teamId: asTeamId("team-mismatch-b"), + repoFullName: repoFullName("mismatch-org/repo"), + orgLogin: "mismatch-org", + threadId: 500, + createdAt: 0, + updatedAt: 0, + }), + ).rejects.toThrow(TenantMismatchError); + + const underArgumentTeam = await repo.get( + asTeamId("team-mismatch-a"), + repoFullName("mismatch-org/repo"), + ); + const underLinkTeam = await repo.get( + asTeamId("team-mismatch-b"), + repoFullName("mismatch-org/repo"), + ); + expect(underArgumentTeam).toBeNull(); + expect(underLinkTeam).toBeNull(); + }); + + it("remove deletes the link and returns true when it existed", async () => { + await seedTeam("team-remove-1", 820); + await seedClaim("remove-org", "team-remove-1"); + const repo = createD1RepoTopicLinkRepo(env.DB); + await repo.upsert(asTeamId("team-remove-1"), { + teamId: asTeamId("team-remove-1"), + repoFullName: repoFullName("remove-org/repo"), + orgLogin: "remove-org", + threadId: 400, + createdAt: 0, + updatedAt: 0, + }); + + const removed = await repo.remove( + asTeamId("team-remove-1"), + repoFullName("remove-org/repo"), + ); + + expect(removed).toBe(true); + const stored = await repo.get( + asTeamId("team-remove-1"), + repoFullName("remove-org/repo"), + ); + expect(stored).toBeNull(); + }); + + it("remove is tenant-scoped: remove(teamB, repo) does not delete teamA's link for the same repo path (REL-003)", async () => { + await seedTeam("team-remove-owner", 822); + await seedTeam("team-remove-other", 823); + await seedClaim("remove-iso-org", "team-remove-owner"); + const repo = createD1RepoTopicLinkRepo(env.DB); + await repo.upsert(asTeamId("team-remove-owner"), { + teamId: asTeamId("team-remove-owner"), + repoFullName: repoFullName("remove-iso-org/repo"), + orgLogin: "remove-iso-org", + threadId: 600, + createdAt: 0, + updatedAt: 0, + }); + + const removed = await repo.remove( + asTeamId("team-remove-other"), + repoFullName("remove-iso-org/repo"), + ); + + expect(removed).toBe(false); + const stillThere = await repo.get( + asTeamId("team-remove-owner"), + repoFullName("remove-iso-org/repo"), + ); + expect(stillThere).not.toBeNull(); + expect(stillThere?.threadId).toBe(600); + }); + + it("remove returns false when no link exists for the (team, repo) (idempotent)", async () => { + await seedTeam("team-remove-2", 821); + const repo = createD1RepoTopicLinkRepo(env.DB); + + const removed = await repo.remove( + asTeamId("team-remove-2"), + repoFullName("remove-org-2/never-linked"), + ); + + expect(removed).toBe(false); + }); + + it("list returns an empty array when the team has no links", async () => { + await seedTeam("team-list-empty", 830); + const repo = createD1RepoTopicLinkRepo(env.DB); + + const result = await repo.list(asTeamId("team-list-empty")); + + expect(result).toEqual([]); + }); + + it("list returns all links for the team", async () => { + await seedTeam("team-list-1", 831); + await seedClaim("list-org-a", "team-list-1"); + await seedClaim("list-org-b", "team-list-1"); + const repo = createD1RepoTopicLinkRepo(env.DB); + await repo.upsert(asTeamId("team-list-1"), { + teamId: asTeamId("team-list-1"), + repoFullName: repoFullName("list-org-a/one"), + orgLogin: "list-org-a", + threadId: 1, + createdAt: 0, + updatedAt: 0, + }); + await repo.upsert(asTeamId("team-list-1"), { + teamId: asTeamId("team-list-1"), + repoFullName: repoFullName("list-org-b/two"), + orgLogin: "list-org-b", + threadId: 2, + createdAt: 0, + updatedAt: 0, + }); + + const result = await repo.list(asTeamId("team-list-1")); + + expect(result.map((l) => l.repoFullName).sort()).toEqual([ + "list-org-a/one", + "list-org-b/two", + ]); + }); + + it("list is tenant-scoped: it never returns another team's links (cross-team isolation)", async () => { + await seedTeam("team-list-owner", 832); + await seedTeam("team-list-other", 833); + await seedClaim("list-iso-org", "team-list-owner"); + const repo = createD1RepoTopicLinkRepo(env.DB); + await repo.upsert(asTeamId("team-list-owner"), { + teamId: asTeamId("team-list-owner"), + repoFullName: repoFullName("list-iso-org/repo"), + orgLogin: "list-iso-org", + threadId: 9, + createdAt: 0, + updatedAt: 0, + }); + + const result = await repo.list(asTeamId("team-list-other")); + + expect(result).toEqual([]); + }); + + it("get propagates (rejects) when the D1 query fails, instead of swallowing the error (RES-001)", async () => { + const repo = createD1RepoTopicLinkRepo(failingDb()); + + await expect( + repo.get(asTeamId("any-team"), repoFullName("any-org/any-repo")), + ).rejects.toThrow("D1_ERROR: simulated D1 outage"); + }); +}); diff --git a/test/adapters/migrations.test.ts b/test/adapters/migrations.test.ts index f5bcf6f..109ca54 100644 --- a/test/adapters/migrations.test.ts +++ b/test/adapters/migrations.test.ts @@ -155,3 +155,148 @@ describe("migrations/0001_init.sql", () => { ).rejects.toThrow(/FOREIGN KEY constraint failed/i); }); }); + +// migrations/0002_github_alerts.sql (design.md "Data Flow" SQL block). +// PR2 (Phase 2 tasks.md 2.1) — raw SQL-level constraint verification against +// the migration shipped in PR1. These tests exercise `env.DB` directly, not +// the adapters (src/adapters/d1/{github-org-claim-repo,repo-topic-link-repo}.ts +// — see repo-scoped tests for that layer). +describe("migrations/0002_github_alerts.sql", () => { + it("rejects a github_org_claims row whose org_login is not already lowercase (CHECK enforced)", async () => { + await env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ) + .bind("team-claim-check", 601, 0) + .run(); + + await expect( + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ) + .bind("Octocat", "team-claim-check", 0) + .run(), + ).rejects.toThrow(/CHECK constraint failed/i); + }); + + it("rejects a github_org_claims row whose team_id has no matching team (FK enforced)", async () => { + await expect( + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ) + .bind("orphan-org", "missing-team-for-claim", 0) + .run(), + ).rejects.toThrow(/FOREIGN KEY constraint failed/i); + }); + + it("rejects a second claim for an already-claimed org, even by a different team (one org -> one team, PK enforced)", async () => { + await env.DB.batch([ + env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ).bind("team-claim-a", 602, 0), + env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ).bind("team-claim-b", 603, 0), + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ).bind("shared-org", "team-claim-a", 0), + ]); + + await expect( + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ) + .bind("shared-org", "team-claim-b", 0) + .run(), + ).rejects.toThrow(/UNIQUE constraint failed/i); + }); + + it("rejects a repo_topic_links row whose repo_full_name is not already lowercase (CHECK enforced)", async () => { + await env.DB.batch([ + env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ).bind("team-link-check", 604, 0), + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ).bind("link-check-org", "team-link-check", 0), + ]); + + await expect( + env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind("team-link-check", "link-check-org/Repo", "link-check-org", 1, 0, 0) + .run(), + ).rejects.toThrow(/CHECK constraint failed/i); + }); + + it("rejects a repo_topic_links row whose (team_id, org_login) has no matching claim (composite FK enforced)", async () => { + await env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ) + .bind("team-link-fk", 605, 0) + .run(); + + await expect( + env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind("team-link-fk", "unclaimed-org/repo", "unclaimed-org", 1, 0, 0) + .run(), + ).rejects.toThrow(/FOREIGN KEY constraint failed/i); + }); + + it("rejects a second row for the same (team_id, repo_full_name) (one topic per repo per team, PK enforced)", async () => { + await env.DB.batch([ + env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ).bind("team-link-dup", 606, 0), + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ).bind("dup-org", "team-link-dup", 0), + env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ).bind("team-link-dup", "dup-org/repo", "dup-org", 10, 0, 0), + ]); + + await expect( + env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind("team-link-dup", "dup-org/repo", "dup-org", 20, 0, 0) + .run(), + ).rejects.toThrow(/UNIQUE constraint failed/i); + }); + + it("isolates repo_topic_links reads by team_id: a query for team A never returns team B's links, even for a same-shaped repo path", async () => { + await env.DB.batch([ + env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ).bind("team-iso-a", 607, 0), + env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ).bind("team-iso-b", 608, 0), + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ).bind("iso-org-a", "team-iso-a", 0), + env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ).bind("iso-org-b", "team-iso-b", 0), + env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ).bind("team-iso-a", "iso-org-a/repo", "iso-org-a", 1, 0, 0), + env.DB.prepare( + "INSERT INTO repo_topic_links (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ).bind("team-iso-b", "iso-org-b/repo", "iso-org-b", 2, 0, 0), + ]); + + const rowsForA = await env.DB.prepare( + "SELECT repo_full_name FROM repo_topic_links WHERE team_id = ?", + ) + .bind("team-iso-a") + .all<{ repo_full_name: string }>(); + + expect(rowsForA.results.map((r) => r.repo_full_name)).toEqual([ + "iso-org-a/repo", + ]); + }); +}); diff --git a/test/domain/link-repo-to-topic.test.ts b/test/domain/link-repo-to-topic.test.ts index 9d4437d..4c1715c 100644 --- a/test/domain/link-repo-to-topic.test.ts +++ b/test/domain/link-repo-to-topic.test.ts @@ -1,6 +1,11 @@ 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 { + NotFoundError, + OrgNotClaimedError, + TenantMismatchError, + UnauthorizedError, +} from "../../src/domain/errors"; import { parseRepoFullName } from "../../src/domain/github"; import { asMemberId, asMembershipId, asTeamId } from "../../src/domain/ids"; import { @@ -123,3 +128,29 @@ describe("linkRepoToTopic", () => { expect(deps.repoTopicLinkRepo.rows[0]?.threadId).toBe(20); }); }); + +// RISK-001/REL-002/READ-001 (PR2 review correction): `fakeRepoTopicLinkRepo` +// must mirror the real D1 adapter's contract — the `teamId` argument is +// authoritative and a mismatching `link.teamId` is rejected, never silently +// written under the wrong team. No use case currently constructs a +// mismatched call (link-repo-to-topic.ts always passes `input.teamId` for +// both), so this exercises the fake directly to keep the two +// implementations honest with each other. +describe("fakeRepoTopicLinkRepo (contract parity with the D1 adapter)", () => { + it("upsert rejects (and writes nothing) when the teamId argument disagrees with link.teamId", async () => { + const deps = makeDeps(); + const otherTeamId = asTeamId("team-2"); + + await expect( + deps.repoTopicLinkRepo.upsert(teamId, { + teamId: otherTeamId, + repoFullName: repo, + orgLogin: "octocat", + threadId: 10, + createdAt: 0, + updatedAt: 0, + }), + ).rejects.toThrow(TenantMismatchError); + expect(deps.repoTopicLinkRepo.rows).toHaveLength(0); + }); +}); diff --git a/test/domain/route-github-event.test.ts b/test/domain/route-github-event.test.ts index 432e3c6..a2dc44f 100644 --- a/test/domain/route-github-event.test.ts +++ b/test/domain/route-github-event.test.ts @@ -132,3 +132,23 @@ describe("routeGithubEvent", () => { expect(deps.alertSender.sent).toHaveLength(0); }); }); + +// REL-001 (PR2 review correction): fakeGithubOrgClaimRepo must normalize +// case exactly like the D1 adapter (createD1GithubOrgClaimRepo lowercases +// both findTeamByOrg and isClaimedBy lookups), so a domain test using the +// fake cannot pass on a case assumption the real adapter would reject. +describe("fakeGithubOrgClaimRepo (contract parity with the D1 adapter)", () => { + it("isClaimedBy matches regardless of the input's case", () => { + const claimRepo = fakeGithubOrgClaimRepo(); + claimRepo.rows.push({ teamId, orgLogin: "case-org" }); + + return expect(claimRepo.isClaimedBy(teamId, "Case-Org")).resolves.toBe(true); + }); + + it("findTeamByOrg matches regardless of the input's case", () => { + const claimRepo = fakeGithubOrgClaimRepo(); + claimRepo.rows.push({ teamId, orgLogin: "case-org-2" }); + + return expect(claimRepo.findTeamByOrg("Case-Org-2")).resolves.toBe(teamId); + }); +}); diff --git a/test/fakes/index.ts b/test/fakes/index.ts index 28761e7..a3214ea 100644 --- a/test/fakes/index.ts +++ b/test/fakes/index.ts @@ -1,4 +1,4 @@ -import { AlertSendFailedError } from "../../src/domain/errors"; +import { AlertSendFailedError, TenantMismatchError } from "../../src/domain/errors"; import type { AuditDraft, Member, @@ -217,13 +217,18 @@ export function fakeGithubOrgClaimRepo( const rows: Array<{ teamId: TeamId; orgLogin: string }> = []; return { rows, + // Mirrors the D1 adapter (REL-001): claims are stored/keyed lowercase, + // so both lookups normalize the input's case exactly like + // createD1GithubOrgClaimRepo does. findTeamByOrg: async (orgLogin: string) => { if (opts.throws) throw new Error("D1 unavailable"); - return rows.find((r) => r.orgLogin === orgLogin)?.teamId ?? null; + const normalized = orgLogin.toLowerCase(); + return rows.find((r) => r.orgLogin === normalized)?.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); + const normalized = orgLogin.toLowerCase(); + return rows.some((r) => r.teamId === teamId && r.orgLogin === normalized); }, }; } @@ -244,6 +249,14 @@ export function fakeRepoTopicLinkRepo( ); }, upsert: async (teamId: TeamId, link: RepoTopicLink) => { + // Mirrors the D1 adapter (RISK-001/REL-002/READ-001): the `teamId` + // argument is authoritative, a mismatching `link.teamId` is rejected + // rather than silently written under the wrong team. + if (teamId !== link.teamId) { + throw new TenantMismatchError( + `RepoTopicLinkRepo.upsert: teamId argument ("${teamId}") does not match link.teamId ("${link.teamId}")`, + ); + } const idx = rows.findIndex( (l) => l.teamId === teamId && l.repoFullName === link.repoFullName, );