Prechádzať zdrojové kódy

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.
Sanaei 11 hodín pred
rodič
commit
20d7f91c65

+ 1 - 1
.github/claude/repo-context.md → .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

+ 1 - 1
.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

+ 32 - 0
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

+ 24 - 24
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])
 		})
 	}
 }