Skip to content

feat(domain): github alert routing and repo-topic links (1/5) - #12

Merged
TOMOKI977 merged 2 commits into
mainfrom
feat/github-alerts-domain
Sep 24, 2026
Merged

TOMOKI977 merged 2 commits into
mainfrom
feat/github-alerts-domain

Conversation

@TOMOKI977

Copy link
Copy Markdown
Contributor

Summary

First PR of the github-alerts chain (5 PRs). It adds only the domain and the migration. Nothing is wired to HTTP or Telegram yet, so there is no user-visible change.

  • Migration 0002_github_alerts.sql: adds github_org_claims (one org is bound to one team) and repo_topic_links (one topic per repo per team). Both are additive CREATE TABLEs, applied automatically on deploy.
  • GithubEvent + formatGithubAlert: alerts carry only allowlisted fields (repo, action, GitHub login, number, title, link, reviewer) and are capped at Telegram's 4096 characters.
  • Use cases, with no framework imports so a future natural-language layer can call them:
    • linkRepoToTopic: admin-only and limited to the team's claimed org. Re-linking moves the repo and reports the previous topic.
    • unlinkRepo: admin-only.
    • listRepoLinks: open to any registered team member.
    • routeGithubEvent: org claim, then team, then link, then send. Unlinked repos and unclaimed orgs are ignored. A Telegram send failure is reported as an outcome, and infrastructure failures propagate so the route can answer 500.

Size exception: about 1,120 lines. Production code is about 350 of them; the rest is tests, fakes and the apply-progress notes.

Review

Full review with four lenses, since the diff is over 400 lines and includes the authorization and claim gates. There were no blocker findings. Two critical findings were confirmed and fixed. Both were untested behavior:

  • Infrastructure-failure propagation: the 500 contract had no test. The claim and link fakes gained a throws option, and tests now assert that D1-style errors reject instead of being swallowed as "ignored".
  • Alert formats: the reviewer line and the merged and review_requested alerts had no tests.

The warnings were also fixed:

  • a test for the claim+link-without-team data-integrity path;
  • actor-not-found tests for link and unlink;
  • link-repo.ts renamed to link-repo-to-topic.ts so the file matches its export.

A scoped fix-delta validator approved the correction.

Intended and left as is: org_login is the primary key of github_org_claims, so one org belongs to exactly one team.

Disclosed process gap

Part of this slice was not written test-first: the reviewer line and two action variants were implemented without a failing test. The review caught it, and every added test passed on the existing code with no bug found.

Test plan

  • npx vitest run: 229/229 pass, including migration 0002 applied in the D1 test pool
  • npx tsc --noEmit: no errors

After merge

Operator step: claim the org in D1 (tasks.md 6.3), before /linkrepo ships in PR 5.

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.
@TOMOKI977 TOMOKI977 added the size:exception PR over the 400-line review budget, accepted explicitly label Sep 24, 2026
@TOMOKI977
TOMOKI977 merged commit cee64b9 into main Sep 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:exception PR over the 400-line review budget, accepted explicitly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant