From 20d7f91c651fbadb1403525ac5d6c0482984d885 Mon Sep 17 00:00:00 2001 From: Sanaei Date: Tue, 8 Sep 2026 21:29:21 +0200 Subject: [PATCH] refactor(ci): add an adversarial pass and name the analyst briefing REVIEW.md told the reviewer which repository rules to check but never to try breaking the change, so the conditions this panel actually meets went unexamined. "Try to break it" adds six, each tied to a mechanism here rather than to a generic checklist: an upgrade over an operator's existing rows and the rollback that reads them again, a restart that drops in-memory state under the cron jobs, a sub-node racing the master on the same row, an operation applied twice, an inbound or client at the empty and the thousand end, and a dependency that is down. It closes with the gate that running a case is not reporting it - each one still has to clear the verification bar below it, so the section cannot become a licence for hypotheticals. repo-context.md said nothing about which bot reads it. Only the issue analyst does, since the review job's briefing moved inline in acf3603d, so it becomes issue-analyst-context.md and its title names the analyst instead of "the Claude bot". bot_context_test.go pins that path in a constant, so the rename carries through the constant, the four test names and the two comments that named the old file - one of which still said "the bot prompts", plural. Backticks come off mtg-multi in the new section: the same test file reads any hyphenated backticked token in REVIEW.md as a CI job name, and fails on one ci.yml does not define. --- ...po-context.md => issue-analyst-context.md} | 2 +- .github/workflows/claude-issue-analyst.yml | 2 +- REVIEW.md | 32 +++++++++++++ bot_context_test.go | 48 +++++++++---------- 4 files changed, 58 insertions(+), 26 deletions(-) rename .github/claude/{repo-context.md => issue-analyst-context.md} (99%) 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]) }) } }