diff --git a/.github/claude/repo-context.md b/.github/claude/issue-analyst-context.md similarity index 99% rename from .github/claude/repo-context.md rename to .github/claude/issue-analyst-context.md index b91db5dba..759538d8f 100644 --- a/.github/claude/repo-context.md +++ b/.github/claude/issue-analyst-context.md @@ -1,4 +1,4 @@ -# Repository context for the Claude bot +# Repository context for the issue analyst Briefing for the issue analyst in `.github/workflows/claude-issue-analyst.yml`. It exists so these facts live in ONE place next to the code instead of being diff --git a/.github/workflows/claude-issue-analyst.yml b/.github/workflows/claude-issue-analyst.yml index bbb775f6b..894c6a5e9 100644 --- a/.github/workflows/claude-issue-analyst.yml +++ b/.github/workflows/claude-issue-analyst.yml @@ -79,7 +79,7 @@ jobs: needs, and no deeper. REPOSITORY CONTEXT - Read `.github/claude/repo-context.md` in the checkout before you answer + Read `.github/claude/issue-analyst-context.md` in the checkout before you answer anything. It carries the stack, the repository map, the hard rules, what CI runs, and the support facts reporters most often get wrong - the random generated credentials, the distro-dependent service environment file, the diff --git a/REVIEW.md b/REVIEW.md index 85cb87530..1952a6059 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -79,6 +79,38 @@ surface — still pre-existing, but open the summary with it. (never testify), the panel is Ant Design (never Tailwind or shadcn). Neither golangci-lint nor oxlint forbids the import, so it passes CI clean. +## Try to break it + +The question behind every finding is how this change fails in production, so +read the changed code under the conditions this panel actually meets rather +than the happy path the author had in mind: + +- **An upgrade over an operator's existing database.** Rows written before + this change: a column added with its zero value, a field the old writer + never set, a settings blob in the older shape. And the way back, because + there are no down-migrations — an operator who rolls the binary back reads + the same rows. +- **A restart.** Anything held only in memory is gone when the panel or the + Xray child restarts, and the cron jobs in `internal/web/job/` then fire + against whatever survived. +- **A second actor at the same instant.** Two panel requests, a request racing + a cron job, or a sub-node syncing while the master writes. Read-modify-write + on the same row is where this surfaces. +- **The same operation twice.** A retried request, a re-sent sync, a job that + ran late and then again on schedule. Traffic and quota resets and Xray API + calls have to survive being applied a second time. +- **Absent, empty and extreme input.** An inbound with no clients, a client + with no traffic, an expired or disabled one, a nil settings blob — and the + other end, the operator with thousands of clients whose loop or query this + change sits inside. +- **A dependency that is down.** The Xray gRPC API refusing a call, the + mtg-multi management API unreachable, a sub-node offline, PIA or LDAP + timing out. What the caller sees, and what state is left behind. + +Running a case is not reporting it. Each one still has to clear the +verification bar below — the code path that mishandles it, cited — and a case +the code already handles is not a finding at all. + ## Do not report - Anything CI already enforces: golangci-lint and gofumpt, oxlint, format diff --git a/bot_context_test.go b/bot_context_test.go index 1bd226d4e..f7969dd5e 100644 --- a/bot_context_test.go +++ b/bot_context_test.go @@ -1,7 +1,7 @@ package main -// The bot prompts under .github/workflows/ read .github/claude/repo-context.md -// instead of restating repo facts; a stale claim there is invisible, so pin it. +// The issue analyst prompt reads .github/claude/issue-analyst-context.md instead +// of restating repo facts; a stale claim there is invisible, so pin it. import ( "os" @@ -12,9 +12,9 @@ import ( ) const ( - botContextPath = ".github/claude/repo-context.md" - reviewPath = "REVIEW.md" - ciWorkflowPath = ".github/workflows/ci.yml" + analystContextPath = ".github/claude/issue-analyst-context.md" + reviewPath = "REVIEW.md" + ciWorkflowPath = ".github/workflows/ci.yml" ) func readRepoFile(t *testing.T, path string) string { @@ -32,7 +32,7 @@ func section(t *testing.T, doc, from, to string) string { t.Helper() i := strings.Index(doc, from) if i < 0 { - t.Fatalf("%s no longer contains the heading %q", botContextPath, from) + t.Fatalf("%s no longer contains the heading %q", analystContextPath, from) } rest := doc[i+len(from):] if before, _, ok := strings.Cut(rest, to); ok { @@ -41,18 +41,18 @@ func section(t *testing.T, doc, from, to string) string { return rest } -func TestBotContextLocaleFileCount(t *testing.T) { - doc := readRepoFile(t, botContextPath) +func TestAnalystContextLocaleFileCount(t *testing.T) { + doc := readRepoFile(t, analystContextPath) m := regexp.MustCompile("`internal/web/translation/` \\((\\d+) files\\)").FindStringSubmatch(doc) if m == nil { - t.Fatalf("%s no longer states the locale file count as \"`internal/web/translation/` (N files)\"", botContextPath) + t.Fatalf("%s no longer states the locale file count as \"`internal/web/translation/` (N files)\"", analystContextPath) } files, err := filepath.Glob("internal/web/translation/*.json") if err != nil { t.Fatalf("glob locales: %v", err) } if got := len(files); m[1] != itoa(got) { - t.Errorf("%s claims %s locale files, internal/web/translation/ holds %d; update the claim and every prompt that relies on it", botContextPath, m[1], got) + t.Errorf("%s claims %s locale files, internal/web/translation/ holds %d; update the claim and every prompt that relies on it", analystContextPath, m[1], got) } } @@ -68,26 +68,26 @@ func itoa(n int) string { return string(b) } -func TestBotContextNamesRealCIJobs(t *testing.T) { - doc := readRepoFile(t, botContextPath) +func TestAnalystContextNamesRealCIJobs(t *testing.T) { + doc := readRepoFile(t, analystContextPath) ci := readRepoFile(t, ciWorkflowPath) table := section(t, doc, "## What CI runs", "**What CI does NOT prove.**") rows := regexp.MustCompile("(?m)^\\| `([a-z0-9-]+)` \\|").FindAllStringSubmatch(table, -1) if len(rows) < 5 { - t.Fatalf("expected the CI table in %s to list at least 5 jobs, found %d", botContextPath, len(rows)) + t.Fatalf("expected the CI table in %s to list at least 5 jobs, found %d", analystContextPath, len(rows)) } for _, r := range rows { t.Run(r[1], func(t *testing.T) { if !strings.Contains(ci, "\n "+r[1]+":\n") { - t.Errorf("%s describes a CI job %q that %s does not define", botContextPath, r[1], ciWorkflowPath) + t.Errorf("%s describes a CI job %q that %s does not define", analystContextPath, r[1], ciWorkflowPath) } }) } } -func TestBotContextNamesRealPaths(t *testing.T) { - // REVIEW.md briefs the review job the way repo-context.md briefs the - // issue bot, so both get their paths pinned. +func TestAnalystContextNamesRealPaths(t *testing.T) { + // REVIEW.md briefs the review job the way issue-analyst-context.md briefs + // the analyst, so both get their paths pinned. // internal/web/dist and frontend/node_modules are build output: absent from a // fresh clone, created by `make dist-stub` and `npm ci`. generated := map[string]bool{ @@ -97,7 +97,7 @@ func TestBotContextNamesRealPaths(t *testing.T) { } seen := map[string]bool{} counts := map[string]int{} - for _, src := range []string{botContextPath, reviewPath} { + for _, src := range []string{analystContextPath, reviewPath} { for _, m := range regexp.MustCompile("`([^`]+)`").FindAllStringSubmatch(readRepoFile(t, src), -1) { p := m[1] if !regexp.MustCompile(`^(internal|frontend|docs|tools|\.github)/`).MatchString(p) || @@ -113,19 +113,19 @@ func TestBotContextNamesRealPaths(t *testing.T) { }) } } - if counts[botContextPath] < 20 { - t.Errorf("expected the bot context to name at least 20 repository paths, found %d - has the file been gutted?", counts[botContextPath]) + if counts[analystContextPath] < 20 { + t.Errorf("expected the bot context to name at least 20 repository paths, found %d - has the file been gutted?", counts[analystContextPath]) } } -func TestBotContextSkipGatesExist(t *testing.T) { - doc := readRepoFile(t, botContextPath) +func TestAnalystContextSkipGatesExist(t *testing.T) { + doc := readRepoFile(t, analystContextPath) table := section(t, doc, "**What CI does NOT prove.**", "Mutation testing") // [A-Z0-9_] and not [A-Z_]: XRAY_E2E_BINARY carries a digit, and excluding it // silently dropped that gate from the check instead of failing. gates := regexp.MustCompile("`((?:XUI|XRAY)_[A-Z0-9_]+)`").FindAllStringSubmatch(table, -1) if len(gates) < 5 { - t.Fatalf("expected at least 5 skip-gate variables in %s, found %d", botContextPath, len(gates)) + t.Fatalf("expected at least 5 skip-gate variables in %s, found %d", analystContextPath, len(gates)) } var sources []string err := filepath.WalkDir("internal", func(path string, d os.DirEntry, err error) error { @@ -147,7 +147,7 @@ func TestBotContextSkipGatesExist(t *testing.T) { return } } - t.Errorf("%s lists %s as a test skip gate, but no .go file under internal/ reads it", botContextPath, g[1]) + t.Errorf("%s lists %s as a test skip gate, but no .go file under internal/ reads it", analystContextPath, g[1]) }) } }