@@ -0,0 +1,187 @@ |
| 1 | +# Issues, comments, labels, milestones |
| 2 | + |
| 3 | +S21 ships the issues subsystem: issues + threaded comments, labels, |
| 4 | +milestones, the polymorphic timeline event log, and the cross-reference |
| 5 | +index. The same tables are reused by S22 for pull requests via the |
| 6 | +`issue_kind` discriminator. |
| 7 | + |
| 8 | +## Schema (migration 0022) |
| 9 | + |
| 10 | +``` |
| 11 | +repo_issue_counter — per-repo monotonic numbering (issues + PRs share) |
| 12 | +issues — the row; kind discriminator covers PR rows in S22 |
| 13 | +issue_comments — per-issue threaded comments |
| 14 | +issue_assignees — many-to-many issue ↔ user |
| 15 | +labels — repo-scoped labels |
| 16 | +issue_labels — many-to-many issue ↔ label |
| 17 | +milestones — repo-scoped milestones |
| 18 | +issue_events — generic timeline events (closed, locked, labeled, …) |
| 19 | +issue_references — cross-reference index (comment / issue / commit → issue) |
| 20 | +``` |
| 21 | + |
| 22 | +The `kind` column on `issues` is in from day one (`'issue' | 'pr'`) so |
| 23 | +S22's PR rows are first-class without a schema split. PR-specific |
| 24 | +fields land in a `pull_requests` table keyed on `issues.id`. |
| 25 | + |
| 26 | +### Per-repo numbering |
| 27 | + |
| 28 | +`repo_issue_counter (repo_id, next_number)` is allocated lazily at first |
| 29 | +issue creation and on every repo create (S16's `repos.Create` calls |
| 30 | +`EnsureRepoIssueCounter` inside the create tx). Allocation is one |
| 31 | +`UPDATE … RETURNING` inside the same tx as the issue insert: |
| 32 | + |
| 33 | +```sql |
| 34 | +UPDATE repo_issue_counter |
| 35 | +SET next_number = next_number + 1 |
| 36 | +WHERE repo_id = $1 |
| 37 | +RETURNING (next_number - 1)::bigint AS allocated; |
| 38 | +``` |
| 39 | + |
| 40 | +Postgres serializes concurrent updaters on the row lock, so concurrent |
| 41 | +`issues.Create` calls on the same repo end up with distinct numbers. |
| 42 | +The race test (`TestCreate_ConcurrentRaceForUniqueNumbers`) fans out 8 |
| 43 | +goroutines and asserts no duplicates. |
| 44 | + |
| 45 | +### Default labels |
| 46 | + |
| 47 | +Repo create seeds the GitHub-aligned set: |
| 48 | + |
| 49 | +``` |
| 50 | +bug · d73a4a · Something isn't working |
| 51 | +documentation · 0075ca · Improvements or additions to documentation |
| 52 | +duplicate · cfd3d7 · This issue or pull request already exists |
| 53 | +enhancement · a2eeef · New feature or request |
| 54 | +good first issue · 7057ff · Good for newcomers |
| 55 | +help wanted · 008672 · Extra attention is needed |
| 56 | +invalid · e4e669 · This doesn't seem right |
| 57 | +question · d876e3 · Further information is requested |
| 58 | +wontfix · ffffff · This will not be worked on |
| 59 | +``` |
| 60 | + |
| 61 | +Seeding runs inside the repo-create transaction via |
| 62 | +`issues.SeedDefaultLabels(ctx, tx, repoID)`. It swallows unique-violations |
| 63 | +so re-runs are no-ops; backfill for repos created before S21 is a one-off |
| 64 | +SQL `INSERT … ON CONFLICT DO NOTHING`. |
| 65 | + |
| 66 | +## Routes |
| 67 | + |
| 68 | +| Route | Auth | |
| 69 | +| -------------------------------------------------------------- | ------------- | |
| 70 | +| `GET /{owner}/{repo}/issues?state=open\|closed` | public | |
| 71 | +| `GET /{owner}/{repo}/issues/{number}` | public | |
| 72 | +| `GET /{owner}/{repo}/labels` | public | |
| 73 | +| `GET /{owner}/{repo}/milestones` | public | |
| 74 | +| `GET /{owner}/{repo}/issues/new` | RequireUser | |
| 75 | +| `POST /{owner}/{repo}/issues` | RequireUser | |
| 76 | +| `POST /{owner}/{repo}/issues/{number}/comments` | RequireUser | |
| 77 | +| `POST /{owner}/{repo}/issues/{number}/state` | RequireUser | |
| 78 | +| `POST /{owner}/{repo}/issues/{number}/lock` | RequireUser | |
| 79 | +| `POST /{owner}/{repo}/issues/{number}/labels` | RequireUser | |
| 80 | +| `POST /{owner}/{repo}/issues/{number}/milestone` | RequireUser | |
| 81 | +| `POST /{owner}/{repo}/issues/{number}/assignees` | RequireUser | |
| 82 | +| `POST /{owner}/{repo}/labels` + `/{id}/update` + `/{id}/delete` | RequireUser | |
| 83 | +| `POST /{owner}/{repo}/milestones` + `/{id}/update` | RequireUser | |
| 84 | +| `POST /{owner}/{repo}/milestones/{id}/state` + `/{id}/delete` | RequireUser | |
| 85 | + |
| 86 | +Public reads still pass through `policy.Can(ActionIssueRead)` so |
| 87 | +private repos return the existence-leak 404. RequireUser on writes |
| 88 | +gives anonymous browsers a `/login` redirect instead of the same 404. |
| 89 | + |
| 90 | +## Cross-reference indexing |
| 91 | + |
| 92 | +`internal/issues/references.go::insertReferencesFromBody` parses |
| 93 | +`#N` and `owner/repo#N` from the comment / issue body inside the |
| 94 | +creating transaction. Each parsed reference produces: |
| 95 | + |
| 96 | +1. an `issue_references` row pointing source → target, |
| 97 | +2. a `referenced` event emitted on the *target* issue's timeline. |
| 98 | + |
| 99 | +The cross-repo regex runs first; matched ranges are stripped before |
| 100 | +the same-repo regex sweeps so `alice/proj#3` doesn't also trip the |
| 101 | +`#3` matcher. Self-references skip silently. Unknown owners / repos / |
| 102 | +issue numbers are best-effort dropped — malformed references shouldn't |
| 103 | +fail the create. |
| 104 | + |
| 105 | +Commit-message references (S14's `push:process`) call into the same |
| 106 | +helper with `srcKind = "commit_message"` and `srcObjectID = push_event_id` |
| 107 | +once that worker job ships. |
| 108 | + |
| 109 | +## Markdown + sanitization |
| 110 | + |
| 111 | +Issue bodies and comments use the existing S17 markdown pipeline |
| 112 | +(`internal/repos/markdown.RenderHTML`): Goldmark for parsing, |
| 113 | +bluemonday's `UGCPolicy` for sanitization. Rendered HTML is cached on |
| 114 | +`body_html_cached`; `md_pipeline_version` lets the worker re-render on |
| 115 | +pipeline upgrades (post-MVP). |
| 116 | + |
| 117 | +`<script>` tags are stripped — the sanitization test |
| 118 | +(`TestCreate_RendersHTMLAndSanitizesScripts`) injects one and asserts |
| 119 | +the cached HTML doesn't contain `<script` while still rendering |
| 120 | +`**bold**` as `<strong>bold</strong>`. |
| 121 | + |
| 122 | +## Locked issues |
| 123 | + |
| 124 | +`SetLock` flips `locked` + emits a `locked`/`unlocked` event. Comments |
| 125 | +hit the locked gate inside `AddComment`: non-collaborators get |
| 126 | +`ErrIssueLocked`. The `IsCollab` flag is the caller's responsibility — |
| 127 | +for v1 the web handler treats the repo owner as the only collaborator; |
| 128 | +S15's `repo_collaborators` table is wired but the lookup is deferred. |
| 129 | + |
| 130 | +## Rate limit |
| 131 | + |
| 132 | +Comment creation hits the existing throttle bucket |
| 133 | +(`scope=issue_comment`, `identifier=user:N`) at **20 per hour**. |
| 134 | +Issue creation isn't independently rate-limited; the broader |
| 135 | +`repo_create` bucket and account anti-abuse measures cover the |
| 136 | +spam-floor case for now. |
| 137 | + |
| 138 | +## Timeline events |
| 139 | + |
| 140 | +`issue_events` is intentionally polymorphic: `kind` is a free string, |
| 141 | +`meta` is a `jsonb` blob, and `ref_target_id` is a denormalized |
| 142 | +pointer for events that target another issue (`referenced`, |
| 143 | +`marked_as_duplicate_of`, etc.). Known kinds today: |
| 144 | + |
| 145 | +``` |
| 146 | +closed, reopened, locked, unlocked, |
| 147 | +labeled, unlabeled, |
| 148 | +assigned, unassigned, |
| 149 | +milestoned, demilestoned, |
| 150 | +referenced |
| 151 | +``` |
| 152 | + |
| 153 | +`meta` always carries an empty `'{}'::jsonb` when no metadata is |
| 154 | +applicable — Go's `nil` byte slice would bind as SQL NULL and trip |
| 155 | +the NOT NULL constraint, so the orchestrator passes `[]byte("{}")` |
| 156 | +explicitly. |
| 157 | + |
| 158 | +## Tests |
| 159 | + |
| 160 | +| File | What it covers | |
| 161 | +| ------------------------------------------------- | -------------------------------------- | |
| 162 | +| `internal/issues/references_test.go` | regex behaviour for #N and owner/repo#N | |
| 163 | +| `internal/issues/issues_test.go::Sequential…` | per-repo number allocation | |
| 164 | +| `internal/issues/issues_test.go::ConcurrentRace…` | 8-goroutine fan-out, no duplicates | |
| 165 | +| `internal/issues/issues_test.go::Sanitizes…` | XSS + markdown render | |
| 166 | +| `internal/issues/issues_test.go::LockedRejects…` | locked-issue gate for non-collab | |
| 167 | +| `internal/issues/issues_test.go::EmitsEvent` | close → reopen event ordering | |
| 168 | +| `internal/issues/issues_test.go::CrossRef…` | #N in body emits `referenced` on target | |
| 169 | + |
| 170 | +## Deferred |
| 171 | + |
| 172 | +- **Mention notifications** — `extractMentions` is in place, but the |
| 173 | + notifications surface (S29-area) hasn't shipped, so parsed `@username` |
| 174 | + tokens currently don't emit anything. |
| 175 | +- **Commit-message references** — the helper accepts |
| 176 | + `srcKind = "commit_message"` already; wiring lives with S14's worker |
| 177 | + job (push:process). |
| 178 | +- **Cross-org references** — the cross-repo regex resolves only |
| 179 | + user-owned repos; org owners arrive in S31. |
| 180 | +- **Required signing / status checks on comments** — out of scope. |
| 181 | +- **Reactions, edit history, comment edits across users** — post-MVP UI |
| 182 | + polish (S30+). |
| 183 | +- **Server-side label / assignee / milestone filtering** — current |
| 184 | + list query filters by state + kind only; richer filtering is the |
| 185 | + perf-pass item in S36. |
| 186 | +- **Issue templates / forms** — post-MVP. |
| 187 | +- **Reactions** — post-MVP (own table; doesn't touch the schema here). |