diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 5110ea382..051bf5ccd 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -18,5 +18,5 @@ Fixes # - [ ] Title uses the repo emoji convention, for example `📖 docs: ...`, `🐛 fix: ...`, or `✨ feature: ...`. - [ ] Commits include DCO sign-off (`git commit -s`). - [ ] Docs, examples, and policies are updated when behavior changes. -- [ ] `CHANGELOG.md` has an entry for user-visible changes (features, fixes, new env vars, behavior changes), or the change is not user-facing. +- [ ] A `changelog.d/-.md` fragment carries the changelog entry for user-visible changes (features, fixes, new env vars, behavior changes) — see `changelog.d/README.md`; or the change is not user-facing (`no-changelog` label). Do not append to `CHANGELOG.md`'s `## Unreleased` directly (#5675). - [ ] No secrets, credentials, or local runtime state are committed. diff --git a/.github/release-lines.yml b/.github/release-lines.yml index c88a470f3..ad518db97 100644 --- a/.github/release-lines.yml +++ b/.github/release-lines.yml @@ -42,6 +42,12 @@ release_lines: [v2, v4, v5] # somebody has to justify, and a new release line still has to be added or # explicitly excluded here before the guard goes green again. pinned: + # PR-level guard for the changelog.d/ fragment convention (#5675). v4 only: + # changelog.d/ and src/scripts/compile-changelog.sh exist only on v4, v2 is + # sunset, and a PR targeting v2 runs the v2 merge-ref's workflows anyway, + # where this file does not exist. Same excluded-not-backfilled reasoning as + # docs-link-check.yml below. + changelog-fragment-guard.yml: [-v2] # Static analysis and vulnerability scanning for the Go module. Pinned to the # release lines and nothing else: a feature branch gets the same gate through # `pull_request`, and running it on every pushed branch would burn runner @@ -76,6 +82,11 @@ pinned: # See src/docs/release-line-guard.md, "The infra workflow sync". scorecard.yml: [main, -v2, -v5] suid-contract.yml: [] + # PR-level guard for src/internal/testutil (#5603). Pinned to the release + # lines and nothing else: it is a pull_request-only path-gated check, and a + # testutil regression can only reach a release line through a PR targeting + # one. + testutil-guard.yml: [] v2-ci.yml: [] v2-tests.yml: [] diff --git a/.github/scripts/check-inline-js.js b/.github/scripts/check-inline-js.js index 11525864a..923c9e705 100755 --- a/.github/scripts/check-inline-js.js +++ b/.github/scripts/check-inline-js.js @@ -1,10 +1,27 @@ #!/usr/bin/env node 'use strict'; -// Syntax-checks the inline '.length; -function checkFile(htmlFile, tmpDir) { - const html = fs.readFileSync(htmlFile, 'utf8'); - let match; - let idx = 0; - const blocks = []; +// ECMAScript level the shipped dashboard is written against. In ESLint flat +// config this sets both the parser level and the set of built-in globals +// (Promise, Map, structuredClone, ...) that `no-undef` accepts. +const ECMA_VERSION = 2022; + +// Identifiers the page legitimately gets from somewhere other than its own +// inline scripts. Neither HTML file has an external `` + +func TestApplyBrandingReplacesEveryDefault(t *testing.T) { + out := string(applyBranding([]byte(sampleIndex), Branding{ + ProductName: "REEF", Tagline: "APPLICATIONS", Mark: "🪸", Title: "Reef", + })) + for _, want := range []string{ + `🪸`, + `
REEF
`, + `
APPLICATIONS
`, + `🪸`, + `🪸`, // entity form, not the glyph + `🪸`, // favicon + `Reef`, + } { + if !strings.Contains(out, want) { + t.Errorf("missing %q", want) + } + } + // Substitution is anchored to markup; a loose replace would corrupt this. + if !strings.Contains(out, `"HIVE appears in script too"`) { + t.Error("unanchored replacement corrupted script content") + } +} + +// An empty field leaves the shipped default alone, so a partial branding.json +// is valid and a missing file changes nothing at all. +func TestApplyBrandingPartialAndEmpty(t *testing.T) { + if got := string(applyBranding([]byte(sampleIndex), Branding{})); got != sampleIndex { + t.Error("empty Branding modified the document") + } + out := string(applyBranding([]byte(sampleIndex), Branding{ProductName: "SCHOOL"})) + if !strings.Contains(out, `
SCHOOL
`) { + t.Error("product name not applied") + } + if !strings.Contains(out, `
GATEWAY DASHBOARD
`) { + t.Error("unset tagline should have been left alone") + } +} + +// Through the real document builder and handler, so this fails if the wiring +// is removed even though applyBranding itself still works. +func TestServedIndexCarriesBrandingStrings(t *testing.T) { + doc := webstatic.NewIndexDocument(webstatic.InjectBranding( + applyBranding([]byte(sampleIndex), Branding{ProductName: "SCHOOL"}))) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "identity") + w := httptest.NewRecorder() + doc.ServeHTTP(w, req) + if !strings.Contains(w.Body.String(), `
SCHOOL
`) { + t.Error("served index does not carry the overridden wordmark") + } +} diff --git a/src/pkg/dashboard/budget_floor_save_test.go b/src/pkg/dashboard/budget_floor_save_test.go new file mode 100644 index 000000000..b0873d2f7 --- /dev/null +++ b/src/pkg/dashboard/budget_floor_save_test.go @@ -0,0 +1,149 @@ +package dashboard + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/hivecommons/hive/pkg/config" +) + +// TestBelowFloorBudgetSaveRejected is the SAVE-path half of the #5508 +// asymmetry. Its twin, TestBelowFloorBudgetStillLoads in pkg/config, asserts +// that these exact same values LOAD successfully with only a warning. +// +// The two behaviours are deliberately opposite on the same input: +// - load → warn, accept (nobody is watching; refusing bricks a live spoke) +// - save → reject (a human is at the dashboard to fix the number) +// +// A change that makes these agree in either direction is a regression, so both +// tests must exist and both must be able to fail. +func TestBelowFloorBudgetSaveRejected(t *testing.T) { + for _, tc := range []struct { + name string + limit int64 + hint string + }{ + {"devx-gabriel 5 tokens", 5, "5M"}, + {"z-aiops2 50 tokens", 50, "50M"}, + {"hosted qa-test 1000 tokens", 1000, "1000M"}, + } { + t.Run(tc.name, func(t *testing.T) { + s := govServer(t) + before := s.deps.Config.Governor.Budget.TotalTokens + + rec := doPut(s, "/api/config/governor/budget", map[string]any{ + "totalTokens": tc.limit, "periodDays": 7, "criticalPct": 90, + }) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("PUT budget totalTokens=%d returned %d, want %d — the dashboard "+ + "must REFUSE a below-floor limit, not accept it silently", + tc.limit, rec.Code, http.StatusBadRequest) + } + // The message has to name the mistake, or the operator learns nothing. + body := rec.Body.String() + if !strings.Contains(body, "did you mean") || !strings.Contains(body, tc.hint) { + t.Errorf("rejection message does not name the likely unit mistake %q: %s", tc.hint, body) + } + // A rejected save must not have mutated live state. + if got := s.deps.Config.Governor.Budget.TotalTokens; got != before { + t.Errorf("rejected save still wrote totalTokens=%d (was %d)", got, before) + } + }) + } +} + +// TestSaneBudgetSaveAccepted is the "no behavior change for sane configs" half: +// at or above the floor — and at zero, which disables budget tracking — the +// save path behaves exactly as it did before the floor existed. +func TestSaneBudgetSaveAccepted(t *testing.T) { + for _, tc := range []struct { + name string + limit int64 + }{ + {"exactly at the floor", config.MinUsableBudgetTokens}, + {"a realistic 50M budget", 50_000_000}, + {"zero disables budget tracking", 0}, + } { + t.Run(tc.name, func(t *testing.T) { + s := govServer(t) + rec := doPut(s, "/api/config/governor/budget", map[string]any{ + "totalTokens": tc.limit, "periodDays": 7, "criticalPct": 90, + }) + if rec.Code != http.StatusOK { + t.Fatalf("PUT budget totalTokens=%d returned %d, want 200: %s", + tc.limit, rec.Code, rec.Body.String()) + } + if got := s.deps.Config.Governor.Budget.TotalTokens; got != tc.limit { + t.Errorf("accepted save stored totalTokens=%d, want %d", got, tc.limit) + } + }) + } +} + +// TestBudgetPartialUpdateJudgedOnSuppliedValue pins the rule that the floor +// judges only what the request SUPPLIED, never a value merely stored. +// +// This is what keeps the three live below-floor spokes usable: if the floor +// were applied to the merged effective value, every budget PUT from those +// hives would 400 — the operator could not adjust period_days and could not +// even send an empty payload — locking them out of the screen they need in +// order to fix the limit. +func TestBudgetPartialUpdateJudgedOnSuppliedValue(t *testing.T) { + s := govServer(t) + // Simulate a live spoke that booted with a below-floor limit (load warned + // and accepted it, so this is a reachable state). + s.deps.Config.Governor.Budget.TotalTokens = 50 + s.deps.Config.Governor.Budget.PeriodDays = 7 + s.deps.Config.Governor.Budget.CriticalPct = 90 + + // Editing an unrelated field must SUCCEED despite the stored below-floor + // limit, and must leave that limit alone. + rec := doPut(s, "/api/config/governor/budget", map[string]any{"periodDays": 14}) + if rec.Code != http.StatusOK { + t.Fatalf("editing periodDays on a spoke with a stored below-floor limit returned %d, want 200: %s; "+ + "the floor must judge only SUPPLIED values or affected spokes are locked out", + rec.Code, rec.Body.String()) + } + if got := s.deps.Config.Governor.Budget.PeriodDays; got != 14 { + t.Errorf("PeriodDays = %d, want 14", got) + } + if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50 { + t.Errorf("TotalTokens = %d, want the stored 50 left untouched", got) + } + + // But re-supplying a below-floor limit explicitly is still refused. + rec = doPut(s, "/api/config/governor/budget", map[string]any{"totalTokens": 50}) + if rec.Code != http.StatusBadRequest { + t.Fatalf("explicitly re-supplying 50 returned %d, want 400", rec.Code) + } + + // And the fix itself must go through in one PUT. + rec = doPut(s, "/api/config/governor/budget", map[string]any{"totalTokens": 50_000_000}) + if rec.Code != http.StatusOK { + t.Fatalf("correcting the limit to 50M returned %d, want 200: %s", rec.Code, rec.Body.String()) + } + if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50_000_000 { + t.Errorf("corrected limit stored as %d, want 50000000", got) + } +} + +// TestBudgetRejectionIsJSONError keeps the refusal machine-readable for the UI. +func TestBudgetRejectionIsJSONError(t *testing.T) { + s := govServer(t) + rec := doPut(s, "/api/config/governor/budget", map[string]any{ + "totalTokens": 50, "periodDays": 7, "criticalPct": 90, + }) + if rec.Code != http.StatusBadRequest { + t.Fatalf("PUT budget totalTokens=50 returned %d, want 400", rec.Code) + } + var payload map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatalf("rejection body is not JSON: %v (%s)", err, rec.Body.String()) + } + if _, ok := payload["error"]; !ok { + t.Errorf("rejection JSON has no error field: %v", payload) + } +} diff --git a/src/pkg/dashboard/budget_owner_gate_4134_test.go b/src/pkg/dashboard/budget_owner_gate_4134_test.go index ceb7d8473..a245774ae 100644 --- a/src/pkg/dashboard/budget_owner_gate_4134_test.go +++ b/src/pkg/dashboard/budget_owner_gate_4134_test.go @@ -30,7 +30,7 @@ func seedBudget4134(s *Server) { func putBudget4134(s *Server, decorate func(*http.Request)) *httptest.ResponseRecorder { req := httptest.NewRequest(http.MethodPut, "/api/config/governor/budget", - strings.NewReader(`{"totalTokens":50000}`)) + strings.NewReader(`{"totalTokens":50000000}`)) req.Header.Set("Content-Type", "application/json") if decorate != nil { decorate(req) @@ -53,8 +53,8 @@ func TestBudgetSave4134_OwnerViaBearerToken(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("owner budget save via bearer token = %d, want 200; body=%q", w.Code, w.Body.String()) } - if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50000 { - t.Fatalf("totalTokens = %d, want 50000 (save did not persist)", got) + if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50000000 { + t.Fatalf("totalTokens = %d, want 50000000 (save did not persist)", got) } } @@ -71,8 +71,8 @@ func TestBudgetSave4134_OwnerViaInternalToken(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("owner budget save via internal token = %d, want 200; body=%q", w.Code, w.Body.String()) } - if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50000 { - t.Fatalf("totalTokens = %d, want 50000 (save did not persist)", got) + if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50000000 { + t.Fatalf("totalTokens = %d, want 50000000 (save did not persist)", got) } } diff --git a/src/pkg/dashboard/budget_owner_live_role_4299_test.go b/src/pkg/dashboard/budget_owner_live_role_4299_test.go index 5df01a242..3fc5153fa 100644 --- a/src/pkg/dashboard/budget_owner_live_role_4299_test.go +++ b/src/pkg/dashboard/budget_owner_live_role_4299_test.go @@ -30,7 +30,7 @@ func seedBudget4299(s *Server) { func putBudget4299(s *Server, sid string) *httptest.ResponseRecorder { req := httptest.NewRequest(http.MethodPut, "/api/config/governor/budget", - strings.NewReader(`{"totalTokens":50000}`)) + strings.NewReader(`{"totalTokens":50000000}`)) req.Header.Set("Content-Type", "application/json") req.AddCookie(&http.Cookie{Name: sessionCookieName, Value: sid}) w := httptest.NewRecorder() @@ -53,8 +53,8 @@ func TestBudgetSave4299_GithubGrantedOwnerWithStaleSession(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("granted owner budget save = %d, want 200; body=%q", w.Code, w.Body.String()) } - if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50000 { - t.Fatalf("totalTokens = %d, want 50000 (save did not persist)", got) + if got := s.deps.Config.Governor.Budget.TotalTokens; got != 50000000 { + t.Fatalf("totalTokens = %d, want 50000000 (save did not persist)", got) } } diff --git a/src/pkg/dashboard/config_write_visibility_test.go b/src/pkg/dashboard/config_write_visibility_test.go new file mode 100644 index 000000000..4bebeb098 --- /dev/null +++ b/src/pkg/dashboard/config_write_visibility_test.go @@ -0,0 +1,348 @@ +package dashboard + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "regexp" + "strings" + "testing" +) + +// readSourceFile returns a Go source file from this package by name. The +// response-shape assertions below are about literal handler source, which the +// compiler cannot check for us. +func readSourceFile(t *testing.T, name string) string { + t.Helper() + b, err := os.ReadFile(name) + if err != nil { + t.Fatalf("reading %s: %v", name, err) + } + return string(b) +} + +// #5492: an ACMM level change and an agent cadence change both applied +// server-side but stayed invisible in the dashboard for ~30s. +// +// The mechanism is NOT the family of 30s poll constants in index.html — those +// drive unrelated panels (history, GH auth, nous, KB, audit, leaderboard). Both +// affected panels render from the SSE status payload, and both write paths +// already ask for a status rebuild. The lag is a RACE: +// +// handler mutates state +// -> go RefreshFunc() (asynchronous rebuild, seconds) +// -> returns 200 to the browser +// browser refetches GET /api/status +// -> handleStatus serves the CACHED s.status, which the async rebuild has +// not replaced yet -> the PRE-mutation snapshot, i.e. the OLD value +// +// The browser then sits on that stale render until some later broadcast +// happens to carry the new value. +// +// #4348 already solved exactly this for the restart counter and the budget +// window: the handler returns "minStatusSeq" (the lowest StatusSeq guaranteed +// to reflect the mutation) and the frontend's noteStatusMutation() raises a +// floor so any snapshot built before the mutation is DISCARDED rather than +// rendered. These tests pin that same contract onto the two #5492 paths. +// +// Critically, this fix never renders an unconfirmed value: the floor only +// causes stale snapshots to be dropped, so a failed write leaves the old value +// on screen (and its error toast), it does not paint the requested value. + +// serverSeqFixture builds a Server whose RefreshFunc is deliberately slow, so +// the async-rebuild race the operator hit is reproduced deterministically +// rather than depending on timing luck. +func serverSeqFixture(t *testing.T) *Server { + t.Helper() + s := &Server{} + s.sseClients = make(map[chan []byte]struct{}) + return s +} + +// TestStatusSeqFloorAdvancesPastCachedSnapshot demonstrates the race itself at +// the seq level: a snapshot published from a build that STARTED before the +// mutation carries a seq below the floor the mutation handed out, which is +// precisely the signal the frontend needs to drop it. +func TestStatusSeqFloorAdvancesPastCachedSnapshot(t *testing.T) { + s := serverSeqFixture(t) + + // A snapshot published before any mutation. + s.UpdateStatus(&StatusPayload{}) + s.statusMu.RLock() + preSeq := s.status.StatusSeq + s.statusMu.RUnlock() + + // The operator's write lands. The handler captures the floor. + floor := s.noteStatusMutation() + + if floor <= preSeq { + t.Fatalf("mutation floor %d must exceed the pre-mutation snapshot seq %d — "+ + "otherwise the browser cannot tell a stale snapshot from a fresh one", floor, preSeq) + } + + // The stale in-flight rebuild (started before the mutation) would publish + // a snapshot whose seq is below the floor. The frontend guard drops it. + if preSeq >= floor { + t.Fatalf("pre-mutation snapshot seq %d is not below floor %d", preSeq, floor) + } +} + +// TestCadenceWriteReturnsStatusSeqFloor asserts the cadence handler hands the +// browser a minStatusSeq. Without it the browser has no way to reject the +// pre-mutation /api/status it is about to receive, and renders the OLD cadence. +func TestCadenceWriteReturnsStatusSeqFloor(t *testing.T) { + html := indexHTML(t) + + // The cadence save must raise the floor from the write response. + if !strings.Contains(html, "noteStatusMutation(cadenceAck.minStatusSeq);") { + t.Error("index.html: the agent cadence save does not raise the stale-snapshot " + + "floor from its write response — a pre-mutation /api/status will repaint " + + "the old cadence and the operator waits for a later broadcast (#5492)") + } +} + +// TestACMMLevelWriteReturnsStatusSeqFloor asserts the same for the ACMM level +// path, which is the higher-stakes of the two: an operator who still sees L4 +// after moving to L5 may re-apply, triggering a second fleet-wide reconcile. +// +// NOTE: the bare string "noteStatusMutation(data.minStatusSeq);" already +// appears in the UNRELATED resetRestarts() handler (#4348), so a whole-file +// Contains check here would pass without the fix — vacuous. Both ACMM +// assertions are therefore scoped to the enclosing function body. +func TestACMMLevelWriteReturnsStatusSeqFloor(t *testing.T) { + for _, fn := range []string{"applyACMMPack", "setACMMLevel"} { + body := acmmFuncBody(t, fn) + if !strings.Contains(body, "noteStatusMutation(data.minStatusSeq);") { + t.Errorf("index.html: %s does not raise the stale-snapshot floor from its "+ + "write response — the dashboard can repaint the previous level after a "+ + "successful L4->L5 change (#5492)", fn) + } + } +} + +// acmmFuncBody returns the source of one of the two ACMM level-change +// functions, bounded to that function so assertions cannot be satisfied by +// identical code elsewhere in this 24k-line file. +func acmmFuncBody(t *testing.T, name string) string { + t.Helper() + html := indexHTML(t) + start := strings.Index(html, "async function "+name+"(level) {") + if start < 0 { + t.Fatalf("index.html: %s not found", name) + } + // Both functions end at the next top-level `async function`/`function` + // declaration at the same indentation. + rest := html[start+10:] + end := strings.Index(rest, "\n async function ") + altEnd := strings.Index(rest, "\n function ") + if altEnd >= 0 && (end < 0 || altEnd < end) { + end = altEnd + } + if end < 0 { + t.Fatalf("index.html: could not bound %s", name) + } + return rest[:end] +} + +// TestACMMOverrideRendersServerConfirmedLevel is the anti-lie guard. +// +// The pre-fix code set window._lastStatus.acmmLevel from `level` — the value +// the browser SENT. That is the failure mode the issue explicitly forbids: +// it renders a value the server did not confirm. The handler already returns +// the authoritative level in its body, so the render must come from the +// RESPONSE. +func TestACMMOverrideRendersServerConfirmedLevel(t *testing.T) { + for _, fn := range []string{"applyACMMPack", "setACMMLevel"} { + body := acmmFuncBody(t, fn) + + // The confirmed level must be read out of the response body. + if !strings.Contains(body, "const confirmedLevel = (data && Number.isFinite(Number(data.level))) ? Number(data.level) : level;") { + t.Errorf("index.html: %s does not derive the rendered level from the "+ + "server's response body — rendering the requested level would show a "+ + "value the server never confirmed (#5492)", fn) + } + + // And the override/state writes must use that confirmed value, not the + // requested one. The literal `level` must no longer reach them. + for _, snippet := range []string{ + "window._acmmOverride = { level: confirmedLevel, packAgents: data.packAgents || [] };", + "window._lastStatus.acmmLevel = confirmedLevel;", + } { + if !strings.Contains(body, snippet) { + t.Errorf("index.html %s is missing %q — the ACMM panel still renders "+ + "the requested level rather than the server-confirmed one (#5492)", fn, snippet) + } + } + if strings.Contains(body, "window._acmmOverride = { level: level,") { + t.Errorf("index.html %s still pins the override to the REQUESTED level — "+ + "that renders a value the server never confirmed (#5492)", fn) + } + } +} + +// TestPackSetLevelResponseCarriesMinStatusSeq asserts the SERVER half of the +// ACMM contract: the PUT /api/packs/level body must carry minStatusSeq. +func TestPackSetLevelResponseCarriesMinStatusSeq(t *testing.T) { + src := readSourceFile(t, "api_packs.go") + + // BOTH ACMM write paths must carry the floor. handlePackApply previously + // triggered no status rebuild at all, so scope each assertion to its own + // handler — a whole-file check would let one path satisfy the other. + for _, h := range []struct{ fn, next string }{ + {"handlePackApply", "func (s *Server) handlePackSetLevel("}, + {"handlePackSetLevel", "func (s *Server) syncAgentVisibility("}, + } { + start := strings.Index(src, "func (s *Server) "+h.fn+"(") + if start < 0 { + t.Fatalf("api_packs.go: %s not found", h.fn) + } + end := strings.Index(src[start:], "\n"+h.next) + if end < 0 { + t.Fatalf("api_packs.go: could not bound %s", h.fn) + } + body := src[start : start+end] + + // The handler must take a seq-returning refresh, not the bare async one. + if !regexp.MustCompile(`floor := s\.refresh(AfterMutationSeq|AndPersistSeq)\(\)`).MatchString(body) { + t.Errorf("api_packs.go: %s does not capture the status-seq floor, so its "+ + "response cannot tell the browser which snapshots predate the level "+ + "change (#5492)", h.fn) + } + // gofmt aligns map literal values, so match the key and value + // independently of the run of padding spaces between them. + if !regexp.MustCompile(`"minStatusSeq":\s+floor,`).MatchString(body) { + t.Errorf("api_packs.go: the %s response body does not include "+ + "minStatusSeq (#5492)", h.fn) + } + } +} + +// TestCadenceHandlerResponseCarriesMinStatusSeq asserts the server half of the +// cadence contract. +func TestCadenceHandlerResponseCarriesMinStatusSeq(t *testing.T) { + src := readSourceFile(t, "api.go") + + idx := strings.Index(src, "func (s *Server) handleAgentConfigCadences(") + if idx < 0 { + t.Fatal("api.go: handleAgentConfigCadences not found") + } + // Bound the search to the handler body. + end := strings.Index(src[idx:], "\nfunc (s *Server) handleAgentConfigModels(") + if end < 0 { + t.Fatal("api.go: could not bound handleAgentConfigCadences") + } + body := src[idx : idx+end] + + if !strings.Contains(body, "floor := s.refreshAndPersistSeq()") { + t.Error("handleAgentConfigCadences does not capture the status-seq floor, so a " + + "pre-mutation /api/status can repaint the old cadence (#5492)") + } + if !strings.Contains(body, `"minStatusSeq": floor`) { + t.Error("handleAgentConfigCadences response does not carry minStatusSeq (#5492)") + } + // okResponse is map[string]string and silently cannot carry a numeric + // floor; the handler must use jsonResponse. + if strings.Contains(body, "okResponse(w, map[string]any{") { + t.Error("handleAgentConfigCadences uses okResponse for a payload containing a " + + "numeric floor — that does not compile; use jsonResponse (#5492)") + } +} + +// TestCadenceWriteStillSurfacesFailure is the second anti-lie guard: a failed +// cadence write must not raise the floor and must not repaint. The floor is +// only raised inside the success branch. +func TestCadenceWriteStillSurfacesFailure(t *testing.T) { + html := indexHTML(t) + + // The ack parse and floor bump must sit after the !res.ok throw, so a + // non-2xx response cannot reach them. + ackIdx := strings.Index(html, "noteStatusMutation(cadenceAck.minStatusSeq);") + if ackIdx < 0 { + t.Fatal("index.html: cadence floor bump not present (#5492)") + } + // Find the enclosing generic-section save and confirm the error throw + // precedes the bump. + throwIdx := strings.LastIndex(html[:ackIdx], "if (!res.ok) throw new Error(await saveErrorMessage(res));") + if throwIdx < 0 { + t.Fatal("index.html: the generic section save no longer throws on a non-OK " + + "response — a failed cadence write would be indistinguishable from a slow one") + } + between := html[throwIdx:ackIdx] + if strings.Contains(between, "showToast('Configuration saved'") { + t.Error("index.html: the cadence floor bump happens after the success toast — " + + "it must be gated on the same non-OK throw so a failed write never advances " + + "the floor (#5492)") + } +} + +// TestACMMFailedWriteDoesNotRender asserts the ACMM error branch returns before +// any optimistic render, so a rejected level change leaves the old level on +// screen with an error message rather than painting the requested level. +func TestACMMFailedWriteDoesNotRender(t *testing.T) { + html := indexHTML(t) + + idx := strings.Index(html, "async function setACMMLevel(level) {") + if idx < 0 { + t.Fatal("index.html: setACMMLevel not found") + } + end := strings.Index(html[idx:], "\n // Packs reconcile scheduling") + if end < 0 { + t.Fatal("index.html: could not bound setACMMLevel") + } + body := html[idx : idx+end] + + errIdx := strings.Index(body, "errEl.textContent = data.error || 'Failed to set level';") + renderIdx := strings.Index(body, "window._acmmOverride = { level: confirmedLevel") + if errIdx < 0 { + t.Fatal("setACMMLevel no longer reports a failed level change") + } + if renderIdx < 0 { + t.Fatal("setACMMLevel no longer renders the confirmed level") + } + if errIdx > renderIdx { + t.Error("setACMMLevel renders the level before handling the error response — " + + "a failed write would paint the requested level (#5492)") + } + // The error branch must return, not fall through into the render. + tail := body[errIdx:renderIdx] + if !strings.Contains(tail, "return;") { + t.Error("setACMMLevel's error branch does not return before the render — a " + + "failed level change would still repaint the requested level (#5492)") + } +} + +// TestStatusHandlerServesCachedSnapshot documents the server behaviour that +// makes the floor necessary: GET /api/status serves whatever snapshot is +// cached, with no rebuild. This is why a post-write refetch can legitimately +// return pre-write data, and therefore why the browser must be told to reject +// it rather than the endpoint being made synchronous. +func TestStatusHandlerServesCachedSnapshot(t *testing.T) { + s := serverSeqFixture(t) + s.UpdateStatus(&StatusPayload{}) + s.statusMu.RLock() + cachedSeq := s.status.StatusSeq + s.statusMu.RUnlock() + + // Mutate; the rebuild has NOT run yet (no RefreshFunc wired). + floor := s.noteStatusMutation() + + req := httptest.NewRequest(http.MethodGet, "/api/status", nil) + rec := httptest.NewRecorder() + s.handleStatus(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("GET /api/status = %d, want 200", rec.Code) + } + var got StatusPayload + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decoding status: %v", err) + } + if got.StatusSeq != cachedSeq { + t.Fatalf("status seq = %d, want the cached pre-mutation %d", got.StatusSeq, cachedSeq) + } + if got.StatusSeq >= floor { + t.Fatalf("the post-write refetch returned seq %d which is NOT below the floor %d — "+ + "the stale-snapshot guard would fail to drop it", got.StatusSeq, floor) + } +} diff --git a/src/pkg/dashboard/contribute_lease_persist_test.go b/src/pkg/dashboard/contribute_lease_persist_test.go new file mode 100644 index 000000000..10050ce74 --- /dev/null +++ b/src/pkg/dashboard/contribute_lease_persist_test.go @@ -0,0 +1,221 @@ +package dashboard + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// contribute_lease_persist_test.go pins the crash-safe persist idiom that +// 8c7d6bc ported onto the task-lease registry (the #5625 standard: unique +// os.CreateTemp name, explicit 0600 chmod, fsync before rename, directory +// fsync after). The port shipped with no test delta, leaving saveLeasesLocked +// at 60.8% — the happy path was pinned by contribute_lease_restart_test.go, +// but none of the NEW invariants were: the owner-only file mode, the +// no-stale-temp guarantee on both success and failure, and the warn-and-return +// (never panic, never half-write) posture of every error branch. +// +// These matter because the registry is the C4 authorization record a resume is +// matched against after a hub restart (#5681): a world-readable copy leaks +// which contributor holds which work item, and a stale fixed-name .tmp beside +// the registry is exactly the clobber-in-flight hazard the unique temp name +// exists to remove. + +// persistHub returns a minimal hub wired to persist its lease registry at +// leasesPath, with one live lease so saveLeasesLocked has something to write. +func persistHub(t *testing.T, leasesPath string) *ContributeWSHub { + t.Helper() + h := &ContributeWSHub{ + logger: covBLogger(), + persistTaskLedgers: true, + taskLeasesFile: leasesPath, + leases: map[string]*taskLease{ + "clanker-7": { + identity: "clanker-7", + taskID: "task-5681", + repo: "hivecommons/hive", + number: 5681, + key: "hivecommons/hive#5681", + tier: "C4", + gen: 3, + expiresAt: time.Now().Add(30 * time.Minute), + }, + }, + } + return h +} + +// listStaleTemps returns every CreateTemp-style leftover (".*.tmp") +// sitting beside the registry file. +func listStaleTemps(t *testing.T, leasesPath string) []string { + t.Helper() + matches, err := filepath.Glob(filepath.Join(filepath.Dir(leasesPath), filepath.Base(leasesPath)+".*.tmp")) + if err != nil { + t.Fatalf("globbing temp files: %v", err) + } + return matches +} + +// TestLeasePersist_FileIsOwnerOnly pins the 0600 invariant: the registry is an +// authorization record, and the explicit Chmod exists so the mode is a stated +// contract rather than an accident of CreateTemp's default. +func TestLeasePersist_FileIsOwnerOnly(t *testing.T) { + path := filepath.Join(t.TempDir(), "ws-state", "task-leases.json") + h := persistHub(t, path) + + h.leaseMu.Lock() + h.saveLeasesLocked() + h.leaseMu.Unlock() + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("registry was not written: %v", err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Errorf("lease registry mode = %04o, want 0600 (owner-only authorization record)", got) + } + if temps := listStaleTemps(t, path); len(temps) != 0 { + t.Errorf("successful save left stale temp files beside the registry: %v", temps) + } + + // The bytes that landed must be the bytes loadLeases boots from. + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading registry back: %v", err) + } + var records []persistedLease + if err := json.Unmarshal(data, &records); err != nil { + t.Fatalf("registry is not valid JSON: %v", err) + } + if len(records) != 1 || records[0].Identity != "clanker-7" || records[0].Gen != 3 { + t.Errorf("registry round-trip = %+v, want the single clanker-7 gen-3 lease", records) + } +} + +// TestLeasePersist_ExpiredLeaseNeverWritten pins the skip-at-write half of the +// "a lease that can no longer be re-adopted must not come back from disk" +// contract (loadLeases pins the skip-at-read half). +func TestLeasePersist_ExpiredLeaseNeverWritten(t *testing.T) { + path := filepath.Join(t.TempDir(), "task-leases.json") + h := persistHub(t, path) + h.leases["expired-1"] = &taskLease{ + identity: "expired-1", + taskID: "task-old", + repo: "hivecommons/hive", + number: 1, + gen: 1, + expiresAt: time.Now().Add(-time.Minute), + } + h.leases["zero-expiry"] = &taskLease{ + identity: "zero-expiry", + taskID: "task-zero", + gen: 2, + // expiresAt zero: an unbounded lease is not a thing the hub issues, so + // a record without an expiry must not be persisted as if it were one. + } + + h.leaseMu.Lock() + h.saveLeasesLocked() + h.leaseMu.Unlock() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("registry was not written: %v", err) + } + if s := string(data); strings.Contains(s, "expired-1") || strings.Contains(s, "zero-expiry") { + t.Errorf("expired/unbounded leases reached disk: %s", s) + } + var records []persistedLease + if err := json.Unmarshal(data, &records); err != nil || len(records) != 1 { + t.Fatalf("want exactly the one live lease on disk, got %s (err=%v)", data, err) + } +} + +// TestLeasePersist_RenameFailureRemovesTemp drives the rename error branch: +// the destination is occupied by a non-empty directory, so os.Rename must +// fail. The contract is warn-and-return — the unique temp file is removed +// (keep=false), nothing panics, and the previous registry state (here: the +// blocking directory) is untouched. +func TestLeasePersist_RenameFailureRemovesTemp(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "task-leases.json") + // A non-empty directory at the target path defeats rename on every POSIX + // filesystem without needing permission tricks (which root would ignore). + if err := os.MkdirAll(filepath.Join(path, "occupied"), 0o755); err != nil { + t.Fatalf("building blocking directory: %v", err) + } + h := persistHub(t, path) + + h.leaseMu.Lock() + h.saveLeasesLocked() + h.leaseMu.Unlock() + + info, err := os.Stat(path) + if err != nil || !info.IsDir() { + t.Fatalf("blocking directory should have survived the failed save: %v", err) + } + if temps := listStaleTemps(t, path); len(temps) != 0 { + t.Errorf("failed rename left stale temp files beside the registry: %v", temps) + } +} + +// TestLeasePersist_NilAndDisabledAreNoOps pins the guard clause: a nil hub and +// a hub with persistence off must both return without touching disk. The nil +// receiver matters because saveLeasesLocked is called from paths that can run +// before the hub is fully wired. +func TestLeasePersist_NilAndDisabledAreNoOps(t *testing.T) { + var nilHub *ContributeWSHub + nilHub.saveLeasesLocked() // must not panic + + path := filepath.Join(t.TempDir(), "task-leases.json") + h := persistHub(t, path) + h.persistTaskLedgers = false + + h.leaseMu.Lock() + h.saveLeasesLocked() + h.leaseMu.Unlock() + + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("persistence disabled but registry written anyway (stat err=%v)", err) + } +} + +// TestLeasePersist_SaveThenLoadRestoresLease closes the loop the crash-safety +// work exists for: what saveLeasesLocked commits, a fresh hub's loadLeases +// restores — same identity, same generation, marked restored — and the new +// hub's generation counter is advanced past every restored gen so post-restart +// assignments cannot alias pre-restart ones (#2568). +func TestLeasePersist_SaveThenLoadRestoresLease(t *testing.T) { + path := filepath.Join(t.TempDir(), "task-leases.json") + h := persistHub(t, path) + + h.leaseMu.Lock() + h.saveLeasesLocked() + h.leaseMu.Unlock() + + h2 := &ContributeWSHub{ + logger: covBLogger(), + persistTaskLedgers: true, + taskLeasesFile: path, + } + h2.loadLeases() + + h2.leaseMu.Lock() + lease := h2.leases["clanker-7"] + h2.leaseMu.Unlock() + if lease == nil { + t.Fatal("lease did not survive the save/load round trip") + } + if !lease.restored { + t.Error("round-tripped lease not marked restored") + } + if lease.gen != 3 || lease.taskID != "task-5681" || lease.key != "hivecommons/hive#5681" { + t.Errorf("restored lease = %+v, want gen=3 taskID=task-5681 key=hivecommons/hive#5681", lease) + } + if got := h2.taskGen.Load(); got < 3 { + t.Errorf("taskGen = %d after restore, want >= 3 so new assignments cannot alias restored gens", got) + } +} diff --git a/src/pkg/dashboard/contribute_local_relaunch_posture_test.go b/src/pkg/dashboard/contribute_local_relaunch_posture_test.go new file mode 100644 index 000000000..e65556bb3 --- /dev/null +++ b/src/pkg/dashboard/contribute_local_relaunch_posture_test.go @@ -0,0 +1,99 @@ +package dashboard + +import ( + "strings" + "testing" +) + +// kubestellar/hive#5652: the relay's post-task relaunch dropped local mode's +// sandbox. `just contribute-hive local` resolves a NARROWED launch +// line (claude: --permission-mode dontAsk + native sandbox settings + workspace +// write-allowlist; copilot: --sandbox; opencode: the host-state deny-list) — +// but that resolution lived only in the Justfile, at first launch. The relay +// re-derived its relaunch line from backends.conf's backend_perm_flag, which +// answers with the CONTAINER posture, so the first task exit relaunched a +// sandboxed local agent as `claude --dangerously-skip-permissions +// --permission-mode bypassPermissions` for the rest of the session. +// +// The fix is single-sourcing: the Justfile resolves the launch line once, +// types it into the pane, and exports the SAME value to the relay as +// AGENT_LAUNCH_CMD; buildLaunchCommand() in bin/contributor-relay.sh prefers +// that over its own derivation. The launcher half of that contract is pinned by +// TestContributeHiveExportsLaunchCommandForRelayRelaunch in +// contribute_pane_cwd_test.go; this file pins the CONSUMER half and the +// cross-file agreement. The relay's behaviour itself — a relaunch byte- +// identical to the exported line, sandbox kept when the launch had one, none +// invented when the operator opted out, container fallback intact — is pinned +// behaviorally in bin/contributor-relay.test.js ("#5652 ..."). + +// localModeLaunchBlock returns the local-mode text from the branch head through +// the relay start, covering both the pane launch and the relay's environment. +func localModeLaunchBlock(t *testing.T) string { + t.Helper() + src := justfileSource(t) + start := strings.Index(src, `if [[ "$_MODE" == "local" ]]; then`) + if start < 0 { + t.Fatal("contribute-hive local-mode branch not found in the Justfile") + } + end := strings.Index(src[start:], "cleanup() {") + if end < 0 { + t.Fatal("end of the local-mode relay start block not found in the Justfile") + } + return src[start : start+end] +} + +// TestRelayReadsTheExactVariableTheLauncherExports pins the env-var contract +// across the two files. A rename on either side would fail silently at +// runtime — the variable would simply be absent in the relay's environment, +// and the permissive backends.conf fallback would win again, which is exactly +// the #5652 escape. Neither file's own tests can catch that: each side would +// still be self-consistent. +func TestRelayReadsTheExactVariableTheLauncherExports(t *testing.T) { + const launchCmdVar = "AGENT_LAUNCH_CMD" + + block := localModeLaunchBlock(t) + if !strings.Contains(block, "export "+launchCmdVar+"=") { + t.Fatalf("the Justfile local branch no longer exports %s; "+ + "relaunches would re-derive the CONTAINER posture and drop the sandbox (#5652)", launchCmdVar) + } + + relay := fileSource(t, "bin/contributor-relay.sh") + if !strings.Contains(relay, "process.env."+launchCmdVar) { + t.Fatalf("bin/contributor-relay.sh no longer reads %s; "+ + "local-mode relaunches would fall back to the container posture (#5652)", launchCmdVar) + } +} + +// TestRelayBuildLaunchCommandPrefersTheEntrypointLine pins that the exported +// line is consulted in buildLaunchCommand() itself — the single source every +// relaunch path (task exit, crash restart, stall backstop, revoke) goes +// through — not merely read somewhere in the file. +func TestRelayBuildLaunchCommandPrefersTheEntrypointLine(t *testing.T) { + relay := fileSource(t, "bin/contributor-relay.sh") + fnStart := strings.Index(relay, "function buildLaunchCommand()") + if fnStart < 0 { + t.Fatal("buildLaunchCommand() not found in bin/contributor-relay.sh") + } + buildFn := relay[fnStart:] + if end := strings.Index(buildFn, "\n}"); end > 0 { + buildFn = buildFn[:end] + } + if !strings.Contains(buildFn, "ENTRYPOINT_LAUNCH_CMD") { + t.Error("buildLaunchCommand() no longer prefers the entrypoint-resolved launch line (#5652)") + } +} + +// TestLocalModePaneLaunchNeverBypassesTheExportedLine pins the vulnerable +// shape out of existence: a send-keys line interpolating $CMD $PERM_FLAG +// directly again would relaunch from a DIFFERENT construction than the one +// exported to the relay — two derivations of one launch line, which is how +// #5652 (and #2203 bug 1 before it) happened. +func TestLocalModePaneLaunchNeverBypassesTheExportedLine(t *testing.T) { + block := localModeLaunchBlock(t) + for _, line := range strings.Split(block, "\n") { + if strings.Contains(line, "send-keys") && strings.Contains(line, "$PERM_FLAG") { + t.Errorf("a local-mode pane launch interpolates $PERM_FLAG directly, bypassing AGENT_LAUNCH_CMD: %s", + strings.TrimSpace(line)) + } + } +} diff --git a/src/pkg/dashboard/contribute_pane_cwd_test.go b/src/pkg/dashboard/contribute_pane_cwd_test.go index 2a9ad95bc..c3152e7d5 100644 --- a/src/pkg/dashboard/contribute_pane_cwd_test.go +++ b/src/pkg/dashboard/contribute_pane_cwd_test.go @@ -83,13 +83,13 @@ func requireCdBeforeCmd(t *testing.T, block, label string) { t.Helper() sendKeys := "" for _, line := range strings.Split(block, "\n") { - if strings.Contains(line, "send-keys") && strings.Contains(line, "$CMD") { + if strings.Contains(line, "send-keys") && (strings.Contains(line, "$CMD") || strings.Contains(line, "$AGENT_LAUNCH_CMD")) { sendKeys = line break } } if sendKeys == "" { - t.Fatalf("%s: no send-keys line launching $CMD found in the block", label) + t.Fatalf("%s: no send-keys line launching the CLI found in the block", label) } if !strings.Contains(sendKeys, "cd ") { t.Errorf("%s: the CLI launch must cd into a durable directory first — a tmux server can hand the pane a "+ @@ -148,3 +148,16 @@ func TestContributorAgentPinsPaneWorkingDirectory(t *testing.T) { t.Error("new-session should also pass -c so a healthy server starts the pane in the right directory") } } + +func TestContributeHiveExportsLaunchCommandForRelayRelaunch(t *testing.T) { + block := contributeHiveLaunchBlock(t) + if !strings.Contains(block, "export AGENT_LAUNCH_CMD=") { + t.Fatal("local contribute-hive must export the exact launch command for contributor-relay.sh relaunches") + } + if !strings.Contains(block, "$AGENT_LAUNCH_CMD") { + t.Fatal("the first local launch must use the same AGENT_LAUNCH_CMD value the relay will reuse") + } + if !strings.Contains(block, "PERM_FLAG") || !strings.Contains(block, "LITELLM_ENV") { + t.Fatal("AGENT_LAUNCH_CMD must include the resolved permission flags and backend environment prefix") + } +} diff --git a/src/pkg/dashboard/contribute_protocol.go b/src/pkg/dashboard/contribute_protocol.go index 5e3baaa62..d7971bf65 100644 --- a/src/pkg/dashboard/contribute_protocol.go +++ b/src/pkg/dashboard/contribute_protocol.go @@ -83,6 +83,16 @@ const ( // Self-reported, advisory, and backward-compatible: undeclared clients still // receive work as before. capCapabilityRouting = "capability_routing" + // capTokenRefreshFailed: when a mid-task re-mint FAILS, the hub tells the + // relay so with a token_refresh_failed message instead of only logging it + // hub-side (#5447). Without it the relay's first evidence that its + // credential went stale is a push that starts failing roughly an hour into + // a long task, reported to the agent as a generic auth error — the + // misleading-symptom class of #5343. Purely additive and advisory: the + // existing token stays in place and the hub keeps retrying on the next + // heartbeat exactly as before, so a relay that ignores the message behaves + // precisely as it does today. + capTokenRefreshFailed = "token_refresh_failed" ) // serverCapabilities returns the capability set this hub advertises on auth_ok. @@ -98,6 +108,7 @@ func serverCapabilities() []string { capAgentRoleClaim, capCompletionVerdict, capCapabilityRouting, + capTokenRefreshFailed, } } diff --git a/src/pkg/dashboard/contribute_reconnect_flap_test.go b/src/pkg/dashboard/contribute_reconnect_flap_test.go new file mode 100644 index 000000000..52f089a99 --- /dev/null +++ b/src/pkg/dashboard/contribute_reconnect_flap_test.go @@ -0,0 +1,307 @@ +package dashboard + +import ( + "testing" + "time" +) + +// The tests in this file cover the two halves of the contributor-flap work: +// kubestellar/hive#5151 (a ~1s reconnect must not be booked as a full departure +// plus arrival) and kubestellar/hive#5090 (the heartbeat loop must not report a +// ping failure on a socket somebody else already tore down). + +// flapRows drives the exact three-row sequence one flap writes: +// "released: connection lost" -> "left" -> "joined". +func flapRows(hub *ContributeWSHub, user, task string) { + hub.addActivity(user, "released: connection lost", "contributor", "claude", "m", "", task) + hub.addActivity(user, "left", "contributor", "claude", "m", "", "") + hub.addActivity(user, "joined", "contributor", "claude", "m", "", "") +} + +func actions(entries []ActivityEntry) []string { + out := make([]string, len(entries)) + for i, e := range entries { + out[i] = e.Action + } + return out +} + +// TestReconnectFlap_CollapsesToSingleJoined is the core #5151 assertion: a flap +// leaves ONE row, not three, and the surviving row is the "joined" that proves the +// contributor is present. The prior "picked up" — the row an operator actually +// wants and that this churn was evicting — must survive untouched. +func TestReconnectFlap_CollapsesToSingleJoined(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.addActivity("alice", "picked up", "contributor", "claude", "m", "", "myorg/repo#7") + flapRows(hub, "alice", "myorg/repo#7") + + got := actions(hub.RecentActivity()) + want := []string{"picked up", "joined"} + if len(got) != len(want) { + t.Fatalf("flap should collapse to %v, got %v", want, got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("activity = %v, want %v", got, want) + } + } + + if n := hub.AbsorbedReconnects(); n != 1 { + t.Fatalf("absorbed reconnect counter = %d, want 1 — the flap must stay countable", n) + } +} + +// TestReconnectFlap_ManyFlapsDoNotEvictTheFeed pins the concrete harm #5151 +// reports: at three rows per flap against maxActivityEntries (50), a flapping +// contributor churns the whole retained feed in under 20 minutes, evicting every +// real row. Twenty flaps must not evict the surrounding history. +func TestReconnectFlap_ManyFlapsDoNotEvictTheFeed(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.addActivity("alice", "picked up", "contributor", "claude", "m", "", "myorg/repo#7") + for i := 0; i < 20; i++ { + flapRows(hub, "alice", "myorg/repo#7") + } + + // A contributor that never actually left should occupy exactly one presence + // row, however many times it bounced — not one row per flap, which would be + // the same eviction only quieter. + got := actions(hub.RecentActivity()) + want := []string{"picked up", "joined"} + if len(got) != len(want) { + t.Fatalf("20 flaps should collapse to %v, got %d rows: %v", want, len(got), got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("activity = %v, want %v", got, want) + } + } + if n := hub.AbsorbedReconnects(); n != 20 { + t.Fatalf("absorbed reconnect counter = %d, want 20 — every flap must stay countable", n) + } +} + +// TestReconnectFlap_DoesNotConsumeUnrelatedJoined guards the third row of the +// walk. The superseded "joined" is consumed ONLY after a "left" above it has been +// seen; a "joined" that no departure followed is a live presence and must stand. +func TestReconnectFlap_DoesNotConsumeUnrelatedJoined(t *testing.T) { + hub, _ := covK2Hub(t) + + // Two arrivals with no departure between them: nothing to absorb. (The + // consecutive-action debounce is dodged by using a different user in between.) + hub.addActivity("alice", "joined", "contributor", "claude", "m", "", "") + hub.addActivity("bob", "picked up", "contributor", "claude", "m", "", "t") + hub.addActivity("alice", "joined", "contributor", "claude", "m", "", "") + + got := actions(hub.RecentActivity()) + if len(got) != 3 { + t.Fatalf("a 'joined' with no 'left' after it must not be consumed, got %v", got) + } + if n := hub.AbsorbedReconnects(); n != 0 { + t.Fatalf("nothing was absorbed, counter = %d, want 0", n) + } +} + +// TestGenuineDeparture_KeepsEveryRow is the falling-through case #5151 requires: +// "a grace period that swallows a genuine departure would be worse than the +// churn". With no reconnect, all rows stand exactly as they do today. +func TestGenuineDeparture_KeepsEveryRow(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.addActivity("alice", "released: connection lost", "contributor", "claude", "m", "", "myorg/repo#7") + hub.addActivity("alice", "left", "contributor", "claude", "m", "", "") + + got := actions(hub.RecentActivity()) + if len(got) != 2 || got[0] != "released: connection lost" || got[1] != "left" { + t.Fatalf("a departure with no reconnect must keep every row, got %v", got) + } + if n := hub.AbsorbedReconnects(); n != 0 { + t.Fatalf("nothing was absorbed, counter = %d, want 0", n) + } +} + +// TestReconnectFlap_DoesNotCollapseAcrossUsers guards the narrowness of the walk +// back: bob's departure must not be retracted by alice arriving. +func TestReconnectFlap_DoesNotCollapseAcrossUsers(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.addActivity("bob", "left", "contributor", "claude", "m", "", "") + hub.addActivity("alice", "joined", "contributor", "claude", "m", "", "") + + got := actions(hub.RecentActivity()) + if len(got) != 2 || got[0] != "left" { + t.Fatalf("another user's 'left' must not be absorbed, got %v", got) + } + if hub.RecentActivity()[0].Username != "bob" { + t.Fatalf("bob's departure row was retracted by alice's arrival") + } +} + +// TestReconnectFlap_StaleLeftIsNotAbsorbed checks the window bound: a "left" from +// outside reconnectFlapWindow is a real departure, and a later arrival is a real +// arrival. Both rows stand. +func TestReconnectFlap_StaleLeftIsNotAbsorbed(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.addActivity("alice", "left", "contributor", "claude", "m", "", "") + + // Backdate the departure past the window. + hub.activityMu.Lock() + hub.activity[0].Timestamp = time.Now().Add(-2 * reconnectFlapWindow).UTC().Format(time.RFC3339) + hub.activityMu.Unlock() + + hub.addActivity("alice", "joined", "contributor", "claude", "m", "", "") + + got := actions(hub.RecentActivity()) + if len(got) != 2 || got[0] != "left" || got[1] != "joined" { + t.Fatalf("a departure older than the flap window must not be absorbed, got %v", got) + } +} + +// TestReconnectFlap_BareReleasedRowSurvives ensures a "released: connection lost" +// is only ever retracted as part of a left+joined round trip. On its own it +// describes WORK, not presence, and a subsequent join must not erase it. +func TestReconnectFlap_BareReleasedRowSurvives(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.addActivity("alice", "released: connection lost", "contributor", "claude", "m", "", "myorg/repo#7") + hub.addActivity("alice", "joined", "contributor", "claude", "m", "", "") + + got := actions(hub.RecentActivity()) + if len(got) != 2 || got[0] != "released: connection lost" { + t.Fatalf("a released row with no 'left' must survive, got %v", got) + } +} + +// TestReconnectFlap_PreservesDuplicatePRGuarantee is the #2356 invariant #5151 +// explicitly asks to be pinned by a test. +// +// The duplicate-PR guarantee lives in the release cooldown, NOT in the activity +// feed. The failure mode this guards against is a "fix" that defers or suppresses +// bookReleaseCooldown behind a grace timer, which would reopen the window in which +// selectTask can hand the same issue to a second session while the original relay +// is still working it. The feed collapse must be provably orthogonal: after a full +// flap has been absorbed down to one row, the issue must STILL be in failure +// cooldown. +func TestReconnectFlap_PreservesDuplicatePRGuarantee(t *testing.T) { + hub, _ := covK2Hub(t) + + // A disconnect books the #2356 hedge on the in-flight issue... + hub.bookReleaseCooldown("myorg/repo", 7) + // ...and writes the three feed rows, which the reconnect then absorbs. + flapRows(hub, "alice", "myorg/repo#7") + + if len(hub.RecentActivity()) != 1 { + t.Fatalf("precondition: the flap should have been absorbed, got %v", + actions(hub.RecentActivity())) + } + if !hub.isTaskInFailureCooldown("myorg/repo", 7) { + t.Fatal("#2356 REGRESSION: absorbing the feed rows must not withdraw the " + + "release cooldown — the duplicate-PR window would be reopened") + } +} + +// TestReleaseCooldown_WithdrawnOnlyByLeaseBoundResume documents the one sanctioned +// way the #2356 hedge is withdrawn (#5322): the original owner re-entering +// activeIssues via a lease-bound resume, which is the STRONGER guard the cooldown +// was standing in for. Absorbing feed rows is not that, and must not imitate it. +func TestReleaseCooldown_WithdrawnOnlyByLeaseBoundResume(t *testing.T) { + hub, _ := covK2Hub(t) + + hub.bookReleaseCooldown("myorg/repo", 7) + if !hub.isTaskInFailureCooldown("myorg/repo", 7) { + t.Fatal("precondition: cooldown should be booked") + } + + // The resume path's withdrawal. + hub.clearReleaseCooldown("myorg/repo", 7) + if hub.isTaskInFailureCooldown("myorg/repo", 7) { + t.Fatal("a lease-bound resume should withdraw the speculative hedge") + } + + // And it stays narrow: a cooldown carrying a real consecutive-failure count is + // a genuine failure record and must NOT be launderable by a resume. + hub.recordTaskFailure("myorg/repo", 8, false) + hub.completedMu.Lock() + hub.consecutiveFailures["myorg/repo#8"] = 2 + hub.completedMu.Unlock() + + hub.clearReleaseCooldown("myorg/repo", 8) + if !hub.isTaskInFailureCooldown("myorg/repo", 8) { + t.Fatal("a real failure record must not be withdrawn by a resume") + } +} + +// TestHeartbeatLoop_SilentOnDeregisteredConnection is the #5090 assertion. +// +// The "heartbeat ping failed, closing" line fired ~29-30s after EVERY new +// connection because the heartbeat tick is a fixed offset from registration: the +// read loop's disconnect defer had already deleted the connection and closed the +// socket, and this loop — which has no done channel — slept out its interval and +// then wrote to a corpse. The resulting log line read as a cause and was a +// lagging indicator by up to a full heartbeat interval, which is what sent #5090's +// diagnosis toward a per-direction proxy idle timer. +// +// connectionRegistered is the guard. Asserting it directly is what matters: a +// connection that is not in h.connections must be reported as gone, so the loop +// returns before it can write and mislabel. +func TestHeartbeatLoop_SilentOnDeregisteredConnection(t *testing.T) { + hub, _ := covK2Hub(t) + + c := &ContributorConnection{profile: &ContributorProfile{GitHubUsername: "alice"}} + + if hub.connectionRegistered(c) { + t.Fatal("an unregistered connection must not be reported as registered") + } + + hub.mu.Lock() + hub.connections["conn-1"] = c + hub.mu.Unlock() + if !hub.connectionRegistered(c) { + t.Fatal("a registered connection must be reported as registered") + } + + // What the disconnect defer does on the read goroutine. + hub.mu.Lock() + delete(hub.connections, "conn-1") + hub.mu.Unlock() + if hub.connectionRegistered(c) { + t.Fatal("after deregistration the heartbeat loop must stop rather than " + + "write to a torn-down socket and log a misleading ping failure") + } + + // Identity is by pointer, not by username: a reconnect registering a NEW + // connection for the same contributor must not keep the OLD loop alive. + replacement := &ContributorConnection{profile: &ContributorProfile{GitHubUsername: "alice"}} + hub.mu.Lock() + hub.connections["conn-2"] = replacement + hub.mu.Unlock() + if hub.connectionRegistered(c) { + t.Fatal("the superseded connection must not be kept alive by its replacement") + } + if !hub.connectionRegistered(replacement) { + t.Fatal("the replacement connection should be registered") + } +} + +// TestWriteDeadline_IsBoundedAndUnderHeartbeatInterval pins the relationship the +// #5090 write-deadline fix depends on. A write must not still be parked when the +// next heartbeat tick arrives (which would stack ticker goroutines on writeMu), +// and it must be comfortably longer than the control-frame deadline so an +// ordinarily slow client is never mistaken for a wedged one. +func TestWriteDeadline_IsBoundedAndUnderHeartbeatInterval(t *testing.T) { + if wsWriteDeadline <= 0 { + t.Fatal("an unbounded write deadline is the defect: WriteJSON on a " + + "half-open socket blocks indefinitely while holding writeMu") + } + if wsWriteDeadline >= wsHeartbeatInterval { + t.Fatalf("wsWriteDeadline (%v) must be shorter than wsHeartbeatInterval (%v) "+ + "so a wedged write cannot outlive the tick that started it", + wsWriteDeadline, wsHeartbeatInterval) + } + if wsWriteDeadline <= wsProtocolPingDeadline { + t.Fatalf("wsWriteDeadline (%v) should exceed wsProtocolPingDeadline (%v)", + wsWriteDeadline, wsProtocolPingDeadline) + } +} diff --git a/src/pkg/dashboard/contribute_task_base_branch_test.go b/src/pkg/dashboard/contribute_task_base_branch_test.go new file mode 100644 index 000000000..2d79734a7 --- /dev/null +++ b/src/pkg/dashboard/contribute_task_base_branch_test.go @@ -0,0 +1,154 @@ +package dashboard + +import ( + "strings" + "testing" +) + +// Regression coverage for kubestellar/hive#5729. +// +// A contributor relay works one issue at a time out of a single PERSISTENT +// checkout, and nothing resets that checkout between tasks. The task prompt +// told the agent to fork, clone, commit, push and open a PR but never said +// which branch to base it on, so the base was whatever the previous task left +// checked out. On 2026-09-02 one `[v5]`-titled issue (#5617) put the checkout +// on `v5` and the four PRs after it — #5688, #5700, #5705, #5711, three of them +// fixes for defects live on the deployed `v4` — were opened against `v5` too. +// Branch ancestry confirmed inheritance rather than choice: each was 1–2 +// commits ahead of `v5` and 64–67 ahead of `v4`, with no `base_ref_changed` +// event on any of them. +// +// The invariant these tests hold: every assignment prompt names the branch its +// work belongs on, and a task following a branch-specific one is told this +// hive's branch rather than the previous task's. + +// promptFor renders an assignment prompt for a hive built from hubBranch. +func promptFor(t *testing.T, hubBranch, title string) string { + t.Helper() + withGitBranch(t, hubBranch) + return buildTaskPrompt("hivecommons/hive", 101, title) +} + +// TestBuildTaskPrompt_NamesTheBaseBranch is the core assertion: the prompt has +// to CARRY the base. Fixing only the workspace is measurably not enough — a +// working branch reset from `v5` onto `v4` mid-task, holding zero commits and a +// clean tree, was put back on `v5` by the agent, because the plan it had +// already formed said `v5`. +func TestBuildTaskPrompt_NamesTheBaseBranch(t *testing.T) { + prompt := promptFor(t, "v4", "the dashboard drops a websocket frame") + + if !strings.Contains(prompt, "'v4' branch") { + t.Errorf("prompt does not name the branch to base the work on; got: %q", prompt) + } + if !strings.Contains(prompt, "gh pr create --base v4") { + t.Errorf("prompt does not tell the agent which base to open the PR against; got: %q", prompt) + } + // The head side matters as much as the base side: a branch cut from the + // leftover checkout carries the previous line's commits even when --base is + // right, which is what put 64–67 unrelated commits under those five PRs. + if !strings.Contains(prompt, "upstream/v4") { + t.Errorf("prompt does not tell the agent to start its work branch from the base; got: %q", prompt) + } +} + +// TestBuildTaskPrompt_TaskAfterBranchSpecificOneGetsTheHiveBranch is the +// session-level regression the bug actually is. #5617 was legitimately `[v5]` +// work; #5681 immediately after it was not, and inherited `v5` anyway. Rendering +// both in order pins that the second prompt is not influenced by the first. +func TestBuildTaskPrompt_TaskAfterBranchSpecificOneGetsTheHiveBranch(t *testing.T) { + branchSpecific := promptFor(t, "v4", "[v5] reviewer lane follow-ups from the adjudication run") + if !strings.Contains(branchSpecific, "gh pr create --base v5") { + t.Fatalf("a [v5] issue must be based on v5; got: %q", branchSpecific) + } + + next := promptFor(t, "v4", "contributor task lease is not released on restart") + if !strings.Contains(next, "gh pr create --base v4") { + t.Errorf("the task after a branch-specific one must be based on the hive's own branch; got: %q", next) + } + if strings.Contains(next, "v5") { + t.Errorf("the previous task's branch leaked into the next prompt; got: %q", next) + } +} + +// TestBuildTaskPrompt_BaseFollowsTheHiveBranch pins that the base is derived, +// not re-hardcoded to v4: a hive built from another branch bases its work +// there. This is the same reasoning #3990 recorded for the onboarding page's +// clone command, and the two answers must agree — a contributor who cloned the +// branch that page names should not then be told to base work elsewhere. +func TestBuildTaskPrompt_BaseFollowsTheHiveBranch(t *testing.T) { + prompt := promptFor(t, "v5", "the dashboard drops a websocket frame") + + if !strings.Contains(prompt, "gh pr create --base v5") { + t.Errorf("base did not follow the hive's own build branch; got: %q", prompt) + } + if strings.Contains(prompt, "--base v4") { + t.Error("base is still pinned to v4 rather than derived") + } +} + +// TestBuildTaskPrompt_UnknownBuildBranchFallsBack: a build with no injected +// branch (local `go run`, where versionBranch stays "unknown") must not emit +// "unknown" as a base, and must still forbid inheriting the checkout's branch. +func TestBuildTaskPrompt_UnknownBuildBranchFallsBack(t *testing.T) { + for _, branch := range []string{"", "unknown"} { + t.Run("branch="+branch, func(t *testing.T) { + prompt := promptFor(t, branch, "the dashboard drops a websocket frame") + + if strings.Contains(prompt, "--base unknown") { + t.Fatal("a build with no injected branch emitted an unusable base") + } + if !strings.Contains(prompt, "gh pr create --base "+defaultUpstreamBranch) { + t.Errorf("fallback did not use defaultUpstreamBranch (%q); got: %q", defaultUpstreamBranch, prompt) + } + }) + } +} + +// TestBuildTaskPromptBody_UnresolvedBaseStillForbidsInheritance: an unresolved +// base is the one case where the prompt cannot name a branch. It must still not +// leave the agent to inherit one — a silent wrong base is precisely the failure +// this change exists to remove — so it names the substitute instead. +func TestBuildTaskPromptBody_UnresolvedBaseStillForbidsInheritance(t *testing.T) { + prompt := buildTaskPromptBody("hivecommons/hive", "hivecommons/hive#101", "a title", "", "") + + if !strings.Contains(prompt, "Do not assume the branch the checkout is currently on") { + t.Errorf("prompt with no resolvable base must still forbid inheriting one; got: %q", prompt) + } + if !strings.Contains(prompt, "defaultBranchRef") { + t.Errorf("prompt must name how to resolve the repository's default branch; got: %q", prompt) + } + if strings.Contains(prompt, "--base ''") || strings.Contains(prompt, "'' branch") { + t.Errorf("an empty base leaked into the prompt as a literal; got: %q", prompt) + } +} + +// TestTaskBaseBranch pins the selection rule itself, including the shapes that +// must NOT be read as a branch. The classifier already routes on "[quality]" +// and friends (pkg/classify), so a rule that grabbed any bracketed prefix would +// send every lane-tagged issue to a branch that does not exist. +func TestTaskBaseBranch(t *testing.T) { + cases := []struct { + name string + title string + want string + }{ + {"plain title uses the hive branch", "reviewer lane follow-ups", "v4"}, + {"release-line tag wins", "[v5] reviewer lane follow-ups", "v5"}, + {"tag is case-insensitive", "[V5] reviewer lane follow-ups", "v5"}, + {"multi-digit release lines", "[v12] something", "v12"}, + {"leading whitespace is tolerated", " [v5] something", "v5"}, + {"lane prefixes are not branches", "[quality] tighten the gate", "v4"}, + {"a tag that is not a release line", "[vNext] something", "v4"}, + {"a bare v is not a release line", "[v] something", "v4"}, + {"a tag with trailing words", "[v5 reviewer] something", "v4"}, + {"an unterminated bracket", "[v5 something", "v4"}, + {"a tag that is not leading", "reviewer lane [v5] follow-ups", "v4"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := taskBaseBranch(tc.title, "v4"); got != tc.want { + t.Errorf("taskBaseBranch(%q, \"v4\") = %q, want %q", tc.title, got, tc.want) + } + }) + } +} diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index adaada96e..7a6e5c608 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -76,10 +76,16 @@ var wsUpgrader = websocket.Upgrader{ } type ContributorConnection struct { - ws *websocket.Conn - connID string - profile *ContributorProfile - cliBackend string + ws *websocket.Conn + connID string + profile *ContributorProfile + cliBackend string + // session is the OPTIONAL client-declared session label from auth_response + // (multi-session-per-account). Empty for a single-session contributor, which + // keeps identityOf() at the bare ContributorID. When set, identityOf() + // returns ContributorID#session so concurrent relays under one account get + // independent lease/assignment/failure slots. Sanitized at auth time. + session string model string reasoningEffort string role string // empty = task-driven mode, "scanner"/"reviewer"/etc. = role mode @@ -104,6 +110,12 @@ type ContributorConnection struct { // cleanupLoop auto-releases a task whose lease has not been renewed within // wsTaskTimeout. Zero when no task is active. lastLeaseRenew time.Time + // taskAssignedAt is when currentTask was assigned, kept SEPARATE from + // lastLeaseRenew (which task_progress refreshes) so a terminal report can + // record the task's real wall-clock duration in the run log + // (task_run_log.go). Zero when no task is active or the task was adopted + // via the resume path without a fresh assignment. + taskAssignedAt time.Time lastPong time.Time tmuxOutput []string // tokenMintedAt is when the scoped GitHub token for currentTask was last @@ -185,6 +197,24 @@ type ContributorConnection struct { func (c *ContributorConnection) send(msg WSMessage) error { c.writeMu.Lock() defer c.writeMu.Unlock() + // Bound the write (kubestellar/hive#5090). WriteJSON on a gorilla connection + // with no write deadline blocks INDEFINITELY once the peer's receive window + // closes — a half-open socket (an L7 proxy that dropped the tunnel without + // telling either endpoint) accepts no bytes and sends no RST, so the write + // neither completes nor fails. Every caller of send holds writeMu for the + // duration, so one wedged peer would park the heartbeat ticker, the read + // loop's replies, and the operator revoke/yank/reassign paths for that + // connection behind a lock nothing can break. + // + // wsWriteDeadline turns that unbounded park into a bounded failure the + // existing error paths already handle: the heartbeat's write-failure branch + // closes the socket with a reason, and a reply failure surfaces to its + // caller. The deadline is per-write and generous enough that an ordinary + // slow-but-live client is never cut — it exists to bound the pathological + // case, not to police latency. + if err := c.ws.SetWriteDeadline(time.Now().Add(wsWriteDeadline)); err != nil { + return err + } return c.ws.WriteJSON(msg) } @@ -212,8 +242,21 @@ type WSMessage struct { // Provider is optional, bounded receipt evidence derived by Pi relays from // their canonical provider/model preference. It is never assignment or // routing authority; Model remains the canonical selection transport. - Provider string `json:"provider,omitempty"` - Model string `json:"model,omitempty"` + Provider string `json:"provider,omitempty"` + Model string `json:"model,omitempty"` + // Session is an OPTIONAL client-declared session label (hivecommons/hive: + // multi-session-per-account). One GitHub account has ONE contributor profile + // (one ContributorID, one auth token, one trust tier), but a contributor may + // want to run several relays at once under that account — e.g. one per CLI + // backend (claude, agy, pi, kiro). All identity-keyed hub state (task leases, + // assignment cooldowns, failure streaks, ownership fences) keys on + // identityOf(); without a distinguisher those sessions would collide on a + // single active-task slot. A distinct Session yields a distinct + // session-scoped identity (ContributorID#session) for that state, while auth, + // tier, model admission and rate-limit accounting stay per-account. Additive + // and backward-compatible: omitted → identity is the bare ContributorID, + // exactly the previous single-session behavior. Sanitized/bounded before use. + Session string `json:"session,omitempty"` ReasoningEffort string `json:"reasoning_effort,omitempty"` TaskID string `json:"task_id,omitempty"` // TaskGen is the assignment GENERATION / lease token for this task (kubestellar/ @@ -447,11 +490,15 @@ type ContributeWSHub struct { // mu-guarded to match taskGen's reasoning above: it is touched from the // upgrade path and from deferred cleanup, and must never contend with or // re-enter h.mu. - pendingConns atomic.Int64 - activityMu sync.RWMutex - activity []ActivityEntry - server *Server - completedTasks map[string]time.Time + pendingConns atomic.Int64 + activityMu sync.RWMutex + activity []ActivityEntry + // absorbedReconnects counts flaps collapsed by absorbReconnectFlapLocked + // (kubestellar/hive#5151), so a contributor bouncing stays countable after its + // feed rows stop being written. Guarded by activityMu alongside activity itself. + absorbedReconnects int + server *Server + completedTasks map[string]time.Time // completedTaskCooldown holds a per-task override for how long, from the // completion time in completedTasks, the issue stays in cooldown. It is // populated by markTaskCompleted based on whether a PR was reported. When a @@ -842,17 +889,60 @@ func (h *ContributeWSHub) saveLeasesLocked() { h.logger.Warn("[contribute-ws] task leases directory creation failed", "error", err) return } - tmpPath := path + ".tmp" + // Crash-safe persist per the #5625 idiom: a UNIQUE temp name (a fixed name + // lets a non-cooperating process clobber a commit in flight), fsync of the + // bytes before the rename (the whole point of this file is that the next + // process boots from it, so the record must be durable, not just renamed), + // and an fsync of the directory so the rename itself survives a crash. + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".*.tmp") + if err != nil { + h.logger.Warn("[contribute-ws] task leases temp creation failed", "error", err) + return + } + tmpPath := tmp.Name() + keep := false + defer func() { + _ = tmp.Close() + if !keep { + _ = os.Remove(tmpPath) + } + }() // 0600, unlike the sibling ledgers: this file is the C4 authorization record // that lookupLease matches a resume against, so it is owner-only on both sides // — nothing else on the host has any business reading which contributor holds - // which work item, and nothing else has any business writing it. - if err := os.WriteFile(tmpPath, data, 0o600); err != nil { + // which work item, and nothing else has any business writing it. CreateTemp + // already makes 0600; the explicit chmod pins the invariant rather than + // inheriting it. + if err := tmp.Chmod(0o600); err != nil { + h.logger.Warn("[contribute-ws] task leases chmod failed", "error", err) + return + } + if _, err := tmp.Write(data); err != nil { h.logger.Warn("[contribute-ws] task leases write failed", "error", err) return } + if err := tmp.Sync(); err != nil { + h.logger.Warn("[contribute-ws] task leases sync failed", "error", err) + return + } + if err := tmp.Close(); err != nil { + h.logger.Warn("[contribute-ws] task leases close failed", "error", err) + return + } if err := os.Rename(tmpPath, path); err != nil { h.logger.Warn("[contribute-ws] task leases rename failed", "error", err) + return + } + keep = true + directory, err := os.Open(dir) + if err != nil { + h.logger.Warn("[contribute-ws] task leases directory open failed", "error", err) + return + } + defer func() { _ = directory.Close() }() + if err := directory.Sync(); err != nil { + h.logger.Warn("[contribute-ws] task leases directory sync failed", "error", err) } } @@ -1267,6 +1357,31 @@ func (h *ContributeWSHub) addActivity(username, action, role, cli, model, effort } } } + // #5151: absorb a fast reconnect instead of booking it as a departure plus an + // arrival. A flap emits "released: connection lost" -> "left" -> "joined"; the + // debounce above never fires on it because consecutive entries never repeat an + // action. At three rows per flap against maxActivityEntries (50), one flapping + // contributor evicts the entire retained feed in under 20 minutes, which is what + // #5090 measured as 19 joined / 19 left filling 38 of 50 slots. + // + // Retracting the trailing flap rows on the "joined" that closes the round trip is + // what makes this correct rather than merely quieter: the pair is only collapsed + // once the reconnect has PROVEN the contributor came back, so a genuine departure + // — where no "joined" ever arrives — keeps every row exactly as today. That is the + // property #5151 asks for ("expiry must fall through to exactly today's + // behavior"), and it needs no timer, no deferred work, and no grace period during + // which the hub is holding a decision it has not made. + // + // It touches ONLY the feed. The #2356 duplicate-PR guarantee is untouched and is + // not this function's to weaken: the release cooldown is still booked eagerly by + // the disconnect defer, and is withdrawn only by the lease-bound resume in + // task_progress via clearReleaseCooldown (#5322) — which withdraws it because the + // original owner has re-entered activeIssues, the stronger guard the cooldown was + // standing in for. No window is ever open in which the issue is both out of + // activeIssues and out of cooldown. + if action == "joined" { + h.absorbReconnectFlapLocked(username) + } entry := ActivityEntry{ Timestamp: time.Now().UTC().Format(time.RFC3339), Username: username, @@ -1293,6 +1408,111 @@ func (h *ContributeWSHub) addActivity(username, action, role, cli, model, effort h.broadcastActivity(entry) } +// reconnectFlapWindow is how recently a "left" must have been written for the +// following "joined" to count as the same contributor bouncing rather than a +// genuine departure followed later by a fresh arrival. +// +// It is sized against the relay's reconnect backoff, not against human behaviour: +// BASE_RECONNECT_DELAY_MS is 1s and MAX_RECONNECT_DELAY_MS is 60s, so a relay that +// is coming back does so inside a minute. Matching activityDebounceSecs keeps one +// notion of "the same session, still" in this file rather than two that can drift. +const reconnectFlapWindow = activityDebounceSecs * time.Second + +// absorbReconnectFlapLocked retracts the trailing "left" — and the +// "released: connection lost" that may immediately precede it — written for this +// user by a disconnect that a reconnect has now undone (kubestellar/hive#5151). +// +// Called from addActivity with activityMu already held, immediately before a +// "joined" is appended. It walks back over at most the two rows one flap can +// write, requires them to belong to THIS user and to be inside +// reconnectFlapWindow, and stops at anything else. It therefore cannot reach past +// a flap into unrelated history, cannot collapse two different users' rows +// together, and cannot touch a "picked up" or "completed" — the rows an operator +// actually wants and that this churn was evicting. +// +// A departure with no reconnect behind it is never reached at all: this runs only +// on "joined". A departure whose reconnect arrives later than the window keeps its +// rows, because at that distance it is no longer a flap. +// +// The flap stays COUNTABLE. #5151 is explicit that absorbing must not become +// silence — the hub-side "[contribute-ws] disconnected" log line and the relay's +// describeWsClose output are untouched and unconditional (they are the #5107 +// instrumentation and the real diagnostic surface), and absorbedReconnects +// increments here so "this contributor flapped N times" stays answerable more +// cheaply than by counting feed rows, which is what the issue asked for. +func (h *ContributeWSHub) absorbReconnectFlapLocked(username string) { + if username == "" { + return + } + end := len(h.activity) + i := end + sawLeft := false + // At most three rows, which is everything one flap cycle can leave behind: + // the "left", the "released: connection lost" that may precede it, and the + // "joined" written by the PREVIOUS absorbed flap. + // + // That third row is what makes repeated flapping actually collapse. Each + // absorbed flap leaves its own "joined" as the new trailing row, so on the next + // flap the walk would stop at it and the feed would still grow by one row per + // flap — 20 flaps leaving 20 "joined" rows, which is the same eviction #5151 + // reports, only quieter. Consuming the superseded "joined" makes a contributor + // that flaps N times in a row occupy ONE row rather than N: the arrival that is + // still true is the one about to be appended, and the earlier ones describe a + // presence that never lapsed. + // + // Bounded explicitly rather than by a general scan so this can never chew + // through the feed. + for i > 0 && end-i < 3 { + e := h.activity[i-1] + if e.Username != username { + break + } + t, err := time.Parse(time.RFC3339, e.Timestamp) + if err != nil || time.Since(t) >= reconnectFlapWindow { + break + } + if e.Action == "left" && !sawLeft { + sawLeft = true + i-- + continue + } + if sawLeft && e.Action == "released: connection lost" { + i-- + continue + } + // Only reachable once the left (and any released) above it have been + // consumed, so this can only ever be the arrival that opened the session + // this flap just closed — never an unrelated join. + if sawLeft && e.Action == "joined" { + i-- + continue + } + break + } + // Only collapse when a "left" was actually found. Without it there is no + // departure to undo, and a bare "released: connection lost" must survive — it + // describes work, not presence. + if !sawLeft { + return + } + h.activity = h.activity[:i] + h.absorbedReconnects++ +} + +// AbsorbedReconnects returns how many contributor reconnects have been absorbed +// into the activity feed rather than booked as a departure plus an arrival +// (kubestellar/hive#5151). It is the cheap, non-evicting answer to "is a +// contributor flapping, and how much", which before this was answerable only by +// counting the feed rows the flapping was simultaneously evicting. +func (h *ContributeWSHub) AbsorbedReconnects() int { + if h == nil { + return 0 + } + h.activityMu.RLock() + defer h.activityMu.RUnlock() + return h.absorbedReconnects +} + func (h *ContributeWSHub) RecentActivity() []ActivityEntry { h.activityMu.RLock() defer h.activityMu.RUnlock() @@ -3500,6 +3720,7 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { connID: connID, profile: profile, cliBackend: msg.CLIBackend, + session: sanitizeSessionLabel(msg.Session), model: msg.Model, reasoningEffort: msg.ReasoningEffort, role: requestedRole, @@ -3908,6 +4129,10 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { } hasTask := contributor.currentTask != nil && contributor.currentTask.TaskID == msg.TaskID completedTask := contributor.currentTask + // Captured before the clear below so the run log can record the + // task's wall-clock duration. Zero when the task was adopted + // without a fresh assignment; the record then omits duration. + taskAssignedAt := contributor.taskAssignedAt // SECURITY (audit N9, CWE-862/639): clear ONLY when the reported // task_id actually matches the held assignment. // @@ -3929,6 +4154,7 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { contributor.currentPrompt = "" contributor.currentLabels = nil contributor.tokenMintedAt = time.Time{} + contributor.taskAssignedAt = time.Time{} // #2537: clear any pending/delivered credential state with the task. contributor.pendingToken = "" contributor.credentialDelivered = false @@ -4029,6 +4255,33 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { // inject arbitrary text into the hub's structured logs. "completion_signal", normalizeCompletionSignal(msg.CompletionSignal), ) + // Durable per-run record (task_run_log.go) — the same + // normalized fields the slog line above carries, plus the + // duration nothing recorded before. DECLARE only. + runRec := TaskRunRecord{ + TaskID: msg.TaskID, + TaskGen: msg.TaskGen, + Username: contributor.profile.GitHubUsername, + Backend: contributor.cliBackend, + Provider: provider, + Model: contributor.model, + Effort: contributor.reasoningEffort, + Role: contributor.role, + Outcome: "completed", + CompletionSignal: normalizeCompletionSignal(msg.CompletionSignal), + Verdict: verdict, + VerdictReason: strings.TrimSpace(msg.VerdictReason), + PRURL: verifiedPR, + PRVerified: verifiedPR != "", + } + if completedTask != nil { + runRec.Repo = completedTask.Repo + runRec.Number = completedTask.Number + } + if !taskAssignedAt.IsZero() { + runRec.DurationS = time.Since(taskAssignedAt).Seconds() + } + h.appendTaskRun(runRec) contributor.mu.Lock() contributor.profile.TasksCompleted++ // Trust credit is gated on the VERIFIED PR, not the reported one: @@ -4099,6 +4352,9 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { } hasTask := contributor.currentTask != nil && contributor.currentTask.TaskID == msg.TaskID failedTask := contributor.currentTask + // Duration anchor for the run log, captured before the clear — + // same shape as task_complete above. + taskAssignedAt := contributor.taskAssignedAt // SECURITY (audit N9, CWE-862/639): same hole as task_complete — // clear only on a genuine TaskID match. Unconditionally, a failure // naming any other task released the assignment while revokeLease @@ -4108,6 +4364,7 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { if hasTask { contributor.currentTask = nil contributor.tokenMintedAt = time.Time{} + contributor.taskAssignedAt = time.Time{} // #2537: clear any pending/delivered credential state with the task. contributor.pendingToken = "" contributor.credentialDelivered = false @@ -4171,6 +4428,31 @@ func (h *ContributeWSHub) HandleWS(w http.ResponseWriter, r *http.Request) { "failure_kind", failureKind, "permanent", msg.Permanent, ) + // Durable per-run record (task_run_log.go). The reason is + // the same bounded, fleet-view-displayed text stored on + // lastFailure above; failure_kind is already normalized. + runRec := TaskRunRecord{ + TaskID: msg.TaskID, + TaskGen: msg.TaskGen, + Username: contributor.profile.GitHubUsername, + Backend: contributor.cliBackend, + Provider: provider, + Model: contributor.model, + Effort: contributor.reasoningEffort, + Role: contributor.role, + Outcome: "failed", + FailureKind: failureKind, + Reason: msg.Reason, + Permanent: msg.Permanent, + } + if failedTask != nil { + runRec.Repo = failedTask.Repo + runRec.Number = failedTask.Number + } + if !taskAssignedAt.IsZero() { + runRec.DurationS = time.Since(taskAssignedAt).Seconds() + } + h.appendTaskRun(runRec) contributor.mu.Lock() contributor.profile.TasksFailed++ contributor.mu.Unlock() @@ -4209,6 +4491,35 @@ func (h *ContributeWSHub) heartbeatLoop(c *ContributorConnection) { defer ticker.Stop() for range ticker.C { + // Stop as soon as this socket has been deregistered (kubestellar/hive#5090). + // + // The disconnect defer in HandleWS runs on the READ goroutine the moment + // ReadMessage errors: it deletes the connID from h.connections and closes + // the socket. This loop learns none of that — it has no done channel and no + // reference to the read side — so it slept out the remainder of its 30s tick + // and then wrote a ping to an already-closed connection. That write of + // course failed, and the failure branch logged + // + // [contribute-ws] heartbeat ping failed, closing + // + // which reads as a diagnosis of why the connection died and is nothing of + // the sort: the connection was already dead and buried, by up to a full + // heartbeat interval. That line is what #5090 spent an investigation + // chasing. Because the tick is a fixed offset from REGISTRATION, it landed + // ~29-30s after every "new connection" regardless of what actually killed + // the socket, which is precisely why the flap looked like a clean 30s idle + // timer and sent the diagnosis toward per-direction proxy timeouts. + // + // Checking registration here makes the loop exit silently on a socket + // somebody else already tore down, so the "heartbeat ping failed" line is + // emitted ONLY when the heartbeat write is genuinely the first thing to + // notice the socket is bad. It also stops the goroutine leaking for up to + // one interval per disconnect, which on a flapping session is a goroutine + // per flap. + if !h.connectionRegistered(c) { + return + } + c.mu.Lock() lastPong := c.lastPong c.mu.Unlock() @@ -4265,6 +4576,7 @@ func (h *ContributeWSHub) maybeRefreshToken(c *ContributorConnection) { if err != nil { h.logger.Warn("[contribute-ws] token refresh: mint failed, will retry next heartbeat", "username", c.profile.GitHubUsername, "tier", tier, "error", err) + h.sendTokenRefreshFailed(c, "mint failed, will retry on the next heartbeat") return } if tok == "" { @@ -4313,6 +4625,36 @@ func (h *ContributeWSHub) taskHeldByAnotherConnection(candidate *ContributorConn return false } +// connectionRegistered reports whether this exact connection object is still in +// the hub's live connection map (kubestellar/hive#5090). +// +// h.connections is keyed by a random per-socket connID that the heartbeat loop +// never sees, so the lookup is by VALUE: scan for the pointer. The map is capped +// at maxWSConnections (50), so this is a bounded scan once per 30s tick per +// connection — negligible next to the network write it guards. +// +// Pointer identity is the right test rather than any field comparison: it is +// exactly "is the object I was started for still the registered one", which is +// false both when the socket was deregistered by its disconnect defer and when a +// reconnect replaced it under a new connID. Both mean this loop has no further +// work to do. +// +// Takes only h.mu.RLock and no connection-level lock, so it cannot participate in +// any lock ordering — callers may hold c.mu or c.writeMu or neither. +func (h *ContributeWSHub) connectionRegistered(c *ContributorConnection) bool { + if h == nil || c == nil { + return false + } + h.mu.RLock() + defer h.mu.RUnlock() + for _, conn := range h.connections { + if conn == c { + return true + } + } + return false +} + // taskReadoptedByLiveConnection reports whether some OTHER live connection // belonging to the SAME contributor identity is currently holding the given task // (kubestellar/hive#5322). @@ -4393,6 +4735,7 @@ func (h *ContributeWSHub) resumeTaskToken(c *ContributorConnection, lease *taskL if err != nil { h.logger.Warn("[contribute-ws] resume token refresh: mint failed, refresh will re-arm on next resume/heartbeat", "username", c.profile.GitHubUsername, "tier", tier, "error", err) + h.sendTokenRefreshFailed(c, "mint failed on task resume, refresh will re-arm on the next resume or heartbeat") return } if tok == "" { @@ -4428,6 +4771,47 @@ func tokenRefreshDue(c *ContributorConnection, now time.Time) (tier, repo string return tier, repo, true } +// sendTokenRefreshFailed tells the relay that a mid-task re-mint FAILED, so the +// credential it is holding is the OLD one and will expire at the token_expires_at +// it was last given (#5447). +// +// Before this, a failed mint was recorded only in the hub's log. The relay's first +// evidence was a push that started failing roughly an hour into a long task, which +// the agent saw as a generic auth error — the same misleading-symptom class as +// #5343, where a credential problem was reported as "the branch doesn't exist on +// the remote". +// +// It deliberately carries NO token material: only a type and a human-readable +// reason. The reason is a fixed, caller-supplied string, never the mint error +// itself, because that error can quote GitHub App responses and we do not want +// hub-internal auth detail crossing to a contributor-controlled process. +// +// Advisory only, and it changes NOTHING about the refresh contract: the old token +// stays installed, tokenMintedAt is untouched (so tokenRefreshDue keeps firing), +// and the next heartbeat retries exactly as before. A send failure is swallowed — +// this is a notification about a degraded credential, and failing the refresh path +// because the notification could not be delivered would turn a warning into an +// outage. The heartbeat's own ping remains the authority on whether the socket is +// alive. +// +// Concurrency: goes through c.send, which takes writeMu, and takes no other lock. +// Both callers (maybeRefreshToken, resumeTaskToken) hold neither c.mu nor c.writeMu +// at the call site — tokenRefreshDue releases c.mu before returning — so there is +// no re-entrancy here. +func (h *ContributeWSHub) sendTokenRefreshFailed(c *ContributorConnection, reason string) { + if c == nil { + return + } + if err := c.send(WSMessage{ + Type: "token_refresh_failed", + Seq: h.nextSeq(), + Reason: reason, + }); err != nil { + h.logger.Debug("[contribute-ws] token refresh: could not notify relay of mint failure", + "username", c.profile.GitHubUsername, "error", err) + } +} + // sendTokenRefresh writes a token_refresh message carrying the new token and its // expiry, then records the new mint time. The field names (github_token, // token_expires_at) match exactly what the relay's token_refresh handler @@ -4803,14 +5187,47 @@ const ( // registered ContributorID is preferred; GitHubUsername is the fallback for // connections whose profile predates or lacks an ID. Two WebSocket connections // opened by the same registered contributor therefore share one identity. +// sanitizeSessionLabel bounds and cleans a client-declared session label before +// it becomes part of a map key, log field, and UI string. Multi-session-per- +// account: the label distinguishes concurrent relays under one GitHub account. +// Only [A-Za-z0-9._-] survive (so the ContributorID#session key stays a single +// clean token and cannot smuggle path/format characters); the result is capped +// at 32 bytes. An empty or all-stripped label returns "", which identityOf +// treats as the historical single-session case. +func sanitizeSessionLabel(s string) string { + if s == "" { + return "" + } + var b strings.Builder + for _, r := range s { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', + r == '.', r == '_', r == '-': + b.WriteRune(r) + } + if b.Len() >= 32 { + break + } + } + return b.String() +} + func identityOf(c *ContributorConnection) string { if c == nil || c.profile == nil { return "" } - if c.profile.ContributorID != "" { - return c.profile.ContributorID + base := c.profile.ContributorID + if base == "" { + base = c.profile.GitHubUsername } - return c.profile.GitHubUsername + // Session-scoped identity (multi-session-per-account): a distinct session + // label lets concurrent relays under one account hold independent task + // leases/cooldowns/ownership. Empty session preserves the historical bare + // ContributorID key, so existing single-session contributors are unchanged. + if base != "" && c.session != "" { + return base + "#" + c.session + } + return base } // rateWindowCounts returns how many task assignments the given identity has been @@ -4889,6 +5306,12 @@ func (h *ContributeWSHub) taskUnavailable(reason string) *WSMessage { // buildTaskPrompt is the GitHub-shaped entry point retained for existing call // sites (ops-tab prompt preview, tests). New identity-aware callers use // buildTaskPromptForRef. +// +// One value comes from outside the task's own metadata: the base branch this +// work belongs on (#5729), which taskBaseBranch derives from the issue title +// and the branch this hive was built from. That is process-wide build metadata +// rather than per-request state, so the preview still renders exactly what the +// agent is sent. func buildTaskPrompt(repoFull string, number int, title string) string { return buildTaskPromptForRef(worksource.Ref{Repo: repoFull, Number: number}, title) } @@ -4920,7 +5343,16 @@ func buildTaskPromptForRef(ref worksource.Ref, title string) string { " This work item lives in the %s work source, not in GitHub Issues; read it at %s.", sourceLabel(ref.SourceType), ref.URL) } - return buildTaskPromptBody(repoFull, issueRef, title, sourceHint) + // #5729: the prompt has to CARRY the base branch. The checkout is reused + // across tasks and nothing resets it, so the branch on disk answers the + // previous task, not this one — and an agent follows the instruction it was + // given over the state it finds. That was measured, not assumed: a working + // branch reset from v5 onto v4 mid-task, with zero commits and a clean tree, + // was put back on v5 by the agent, because the plan it had already formed + // said v5. Fixing the workspace alone cannot work; the instruction has to + // carry the answer. + return buildTaskPromptBody(repoFull, issueRef, title, sourceHint, + taskBaseBranch(title, upstreamBranch())) } // taskIDSegment is the per-item component of a task id. For GitHub-backed work @@ -4943,7 +5375,68 @@ func sourceLabel(sourceType string) string { return sourceType } -func buildTaskPromptBody(repoFull, issueRef, title, sourceHint string) string { +// releaseLineFromTitle extracts a leading release-line tag — "[v5] reviewer +// lane follow-ups …" yields "v5" — and returns "" for every other title. +// +// The shape is deliberately narrow: `v` followed by digits and nothing else, +// the same `^v(\d+)$` release-line shape pkg/hub's image_pulls.go matches and +// .github/release-lines.yml's `release_lines` list uses. The lane prefixes the +// classifier already routes on ("[quality]", "[architect]", …) cannot collide +// with it, and a tag naming no real branch fails loudly at `gh pr create` +// rather than silently redirecting the PR — which is the failure mode this +// whole change exists to remove. +func releaseLineFromTitle(title string) string { + t := strings.TrimSpace(title) + if !strings.HasPrefix(t, "[") { + return "" + } + end := strings.Index(t, "]") + if end < 0 { + return "" + } + tag := strings.ToLower(strings.TrimSpace(t[1:end])) + if len(tag) < 2 || tag[0] != 'v' { + return "" + } + for _, r := range tag[1:] { + if r < '0' || r > '9' { + return "" + } + } + return tag +} + +// taskBaseBranch is the branch a task's work must be based on and its PR opened +// against (kubestellar/hive#5729). +// +// A contributor relay works one issue at a time out of a single PERSISTENT +// checkout, and nothing resets that checkout between tasks. The prompt never +// named a base, so the base was whatever the PREVIOUS task happened to leave +// checked out: on 2026-09-02 one `[v5]`-titled issue put the checkout on `v5` +// and the four PRs after it — three of them fixes for defects live on the +// deployed `v4` — were opened against `v5` too, and had to be backported by +// hand. Nobody in the loop can see that going wrong: the agent has nothing to +// check against, the contributor sees PRs opening and merging normally, and a +// maintainer sees correctly-formed PRs on a plausible branch. +// +// hubBranch is the branch this hive itself is built from (upstreamBranch()) — +// the same branch the contribute onboarding page already tells contributors to +// clone (#3990), so "base your work on it" is the answer that was always +// implied and never stated. A branch-specific issue overrides it: an issue +// titled "[v5] …" is work for `v5` whatever branch this hive runs, which is +// also why the inheritance had a plausible-looking first PR to start from. +func taskBaseBranch(title, hubBranch string) string { + if line := releaseLineFromTitle(title); line != "" { + return line + } + return strings.TrimSpace(hubBranch) +} + +// buildTaskPromptBody renders the assignment prompt's text. baseBranch is the +// branch this task's work belongs on; it is empty only when the hive cannot +// resolve one at all, which changes the wording below but never licenses +// inheriting whatever branch the checkout happens to be on. +func buildTaskPromptBody(repoFull, issueRef, title, sourceHint, baseBranch string) string { // The workspace contract (kubestellar/hive#2545): your tmux pane already // starts rooted in $HIVE_WORKSPACE_DIR (contributor-agent.sh creates it and // launches the session with -c pointed there), but nothing had put a repo @@ -4953,6 +5446,27 @@ func buildTaskPromptBody(repoFull, issueRef, title, sourceHint string) string { // its own clone, was left sitting in an empty directory while the // assignment slot stayed held. Spell out an actual clone into that known // directory so there is a concrete first step rather than an implied one. + baseHint := fmt.Sprintf( + // An unresolved base is not a licence to inherit one. Name the only + // trustworthy substitute — the upstream repository's own default + // branch, read from the clone rather than from whatever the last task + // left behind — and keep the "do not use the branch you find" clause, + // which is the load-bearing half in both wordings. + "Do not assume the branch the checkout is currently on is the right base: it may "+ + "be left over from a previous task. Resolve %s's own default branch "+ + "('gh repo view %s --json defaultBranchRef'), start your work branch from it, "+ + "and open the PR against it. ", + repoFull, repoFull) + if b := strings.TrimSpace(baseBranch); b != "" { + baseHint = fmt.Sprintf( + "Base this work on the '%s' branch of %s. The checkout may be left on a "+ + "DIFFERENT branch by a previous task, so do not use whatever branch you "+ + "find there: run 'git fetch upstream' and start your work branch from the "+ + "base with 'git checkout -b upstream/%s'. Open the PR against "+ + "the same branch with 'gh pr create --base %s', and confirm the PR's base "+ + "is '%s' before you report done. ", + b, repoFull, b, b, b) + } return fmt.Sprintf( "You are a contributor to the %s hive. Work on issue %s: \"%s\".%s "+ "You do NOT have push access to the upstream repo. "+ @@ -4962,6 +5476,13 @@ func buildTaskPromptBody(repoFull, issueRef, title, sourceHint string) string { "from a prior task, 'cd' into it and 'git fetch' instead of "+ "re-forking). Then 'cd' into that checkout, read the issue, "+ "understand what's needed, and take action. "+ + // #5729: the base branch. Everything above deliberately REUSES a + // checkout across tasks, which is exactly what makes the branch + // left on disk the previous task's answer rather than this one's. + // Name the branch, name it before the agent forms a plan, and ask + // for the base back at the end — an agent never told a base cannot + // notice it inherited the wrong one. + "%s"+ // DCO is enforced on this repo (CONTRIBUTING.md) and an unsigned // commit blocks the merge, but the prompt used to leave sign-off // entirely to whatever each agent inferred from the repo. That @@ -5021,7 +5542,7 @@ func buildTaskPromptBody(repoFull, issueRef, title, sourceHint string) string { "only when you are actually done, and never before starting work. "+ "If you printed the no_work_needed line above, that already counts "+ "as your completion — do not print both.", - repoFull, issueRef, title, sourceHint, repoFull, repoFull, + repoFull, issueRef, title, sourceHint, repoFull, repoFull, baseHint, ) } @@ -5784,6 +6305,9 @@ func (h *ContributeWSHub) selectTask(c *ContributorConnection) *WSMessage { // #2568: start the hub-owned lease clock. task_progress renews it; cleanupLoop // auto-releases the task if it is not renewed within wsTaskTimeout. c.lastLeaseRenew = time.Now() + // Duration anchor for the run log — lastLeaseRenew moves on every + // progress report, so it cannot serve as the start time. + c.taskAssignedAt = time.Now() // Store the prompt (never the token) so FleetSnapshot can preview it (#2539), // and clear any stale idle reason now that this connection has real work. c.currentPrompt = prompt @@ -5920,6 +6444,22 @@ const wsCloseFrameDeadline = time.Second // reading. const wsProtocolPingDeadline = 10 * time.Second +// wsWriteDeadline bounds every application JSON write to a live contributor +// connection (kubestellar/hive#5090). +// +// gorilla/websocket applies no write deadline by default, so WriteJSON against a +// peer that has stopped reading blocks until the OS gives up on the socket — +// which, on a half-open TCP connection with no RST, can be many minutes of +// retransmission backoff. Because send() holds writeMu across the write, that +// stall is not confined to the writing goroutine: it blocks every other writer +// on the same connection. +// +// It is deliberately shorter than wsHeartbeatInterval so a write cannot still be +// parked when the next heartbeat tick arrives (which would stack ticker +// goroutines on writeMu), and comfortably longer than wsProtocolPingDeadline so +// an ordinary slow client is never mistaken for a wedged one. +const wsWriteDeadline = 15 * time.Second + // writeProtocolPing sends a WebSocket PROTOCOL-level Ping control frame (opcode // 0x9) on the connection. // diff --git a/src/pkg/dashboard/contribute_ws_credential_accept_test.go b/src/pkg/dashboard/contribute_ws_credential_accept_test.go index 057eeb5f9..f2130cd0d 100644 --- a/src/pkg/dashboard/contribute_ws_credential_accept_test.go +++ b/src/pkg/dashboard/contribute_ws_credential_accept_test.go @@ -35,6 +35,7 @@ import ( func wsPipe(t *testing.T) (server *websocket.Conn, client *websocket.Conn) { t.Helper() connReady := make(chan struct{}) + handlerDone := make(chan struct{}) upgrader := websocket.Upgrader{} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c, err := upgrader.Upgrade(w, r, nil) @@ -44,10 +45,12 @@ func wsPipe(t *testing.T) (server *websocket.Conn, client *websocket.Conn) { } server = c close(connReady) - // Keep the handler alive so the conn stays open for the reads below. - time.Sleep(3 * time.Second) + // Keep the handler alive until the test is done with the conn; released + // by the cleanup below (LIFO: runs before srv.Close) rather than a timer. + <-handlerDone })) t.Cleanup(srv.Close) + t.Cleanup(func() { close(handlerDone) }) c, _, err := websocket.DefaultDialer.Dial("ws"+strings.TrimPrefix(srv.URL, "http"), nil) if err != nil { diff --git a/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go b/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go index 56ea15c60..f3d43b5cd 100644 --- a/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go +++ b/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go @@ -41,6 +41,7 @@ func refreshConn(ws *websocket.Conn, mintedAt time.Time) *ContributorConnection func wsPair(t *testing.T) (serverConn, clientConn *websocket.Conn) { t.Helper() connReady := make(chan struct{}) + handlerDone := make(chan struct{}) upgrader := websocket.Upgrader{} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c, err := upgrader.Upgrade(w, r, nil) @@ -50,10 +51,12 @@ func wsPair(t *testing.T) (serverConn, clientConn *websocket.Conn) { } serverConn = c close(connReady) - // Keep the handler alive so the conn stays open for the test body. - time.Sleep(2 * time.Second) + // Keep the handler alive until the test is done with the conn; released + // by the cleanup below (LIFO: runs before srv.Close) rather than a timer. + <-handlerDone })) t.Cleanup(srv.Close) + t.Cleanup(func() { close(handlerDone) }) client, _, err := websocket.DefaultDialer.Dial("ws"+strings.TrimPrefix(srv.URL, "http"), nil) if err != nil { @@ -88,10 +91,16 @@ func TestMaybeRefreshToken_NotDueIsANoOp(t *testing.T) { } // TestMaybeRefreshToken_MintFailureRetriesNextHeartbeat: when the mint fails -// (#2436-style App API error) nothing may be sent — the relay keeps its +// (#2436-style App API error) NO CREDENTIAL may be sent — the relay keeps its // existing token — and tokenMintedAt must NOT advance, so tokenRefreshDue still // reports due on the next heartbeat and the refresh is retried rather than // abandoned for the life of the task. +// +// As of #5447 this path DOES write one frame: an advisory token_refresh_failed +// carrying no token material (asserted below and, for its content, in +// TestMaybeRefreshToken_MintFailureNotifiesRelay). The connection therefore now +// needs a real socket rather than the nil-ws tripwire the other no-send cases +// still use; the retry policy this test guards is unchanged. func TestMaybeRefreshToken_MintFailureRetriesNextHeartbeat(t *testing.T) { hub := &ContributeWSHub{logger: slog.Default()} s := NewServer(0, slog.Default()) @@ -99,10 +108,28 @@ func TestMaybeRefreshToken_MintFailureRetriesNextHeartbeat(t *testing.T) { hub.server = s minted := time.Now().Add(-wsTokenRefreshPeriod - time.Minute) - conn := refreshConn(nil, minted) // nil ws: a send attempt would panic + server, client := wsPair(t) + conn := refreshConn(server, minted) hub.maybeRefreshToken(conn) + // Whatever is written on a failed mint, it must never be a credential. + _ = client.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, data, err := client.ReadMessage() + if err != nil { + t.Fatalf("read after failed mint: %v", err) + } + var wire map[string]any + if err := json.Unmarshal(data, &wire); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if wire["type"] != "token_refresh_failed" { + t.Fatalf("type = %v, want token_refresh_failed", wire["type"]) + } + if _, ok := wire["github_token"]; ok { + t.Fatalf("a failed mint must never put token material on the wire: %s", data) + } + conn.mu.Lock() got := conn.tokenMintedAt conn.mu.Unlock() @@ -213,3 +240,104 @@ func TestMaybeRefreshToken_SendFailureKeepsRetryArmed(t *testing.T) { t.Fatalf("after a failed send, refresh must still be due on the next heartbeat") } } + +// TestMaybeRefreshToken_MintFailureNotifiesRelay is the hub half of #5447. +// +// Before it, a failed re-mint was recorded ONLY in the hub's log. The relay was +// told nothing, so its first evidence that the credential it holds had gone +// stale was a push failing roughly an hour into a long task, which the agent +// reported as a generic auth error — the misleading-symptom class of #5343. +// +// This asserts the OBSERVABLE consequence (#5388): a real frame, of a named +// type, carrying a reason and no token material. Asserting only that the hub +// "handles" a mint failure would pass against the old silent code. +func TestMaybeRefreshToken_MintFailureNotifiesRelay(t *testing.T) { + hub := &ContributeWSHub{logger: slog.Default()} + s := NewServer(0, slog.Default()) + s.deps = &Dependencies{GHAppAuth: newFailingAppAuth(t)} + hub.server = s + + server, client := wsPair(t) + conn := refreshConn(server, time.Now().Add(-wsTokenRefreshPeriod-time.Minute)) + + hub.maybeRefreshToken(conn) + + _ = client.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, data, err := client.ReadMessage() + if err != nil { + t.Fatalf("the relay was never told the re-mint failed: %v", err) + } + var wire map[string]any + if err := json.Unmarshal(data, &wire); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if wire["type"] != "token_refresh_failed" { + t.Fatalf("type = %v, want token_refresh_failed", wire["type"]) + } + reason, _ := wire["reason"].(string) + if reason == "" { + t.Fatalf("token_refresh_failed carried no reason: %s", data) + } + // The reason is a fixed, caller-supplied string. The mint error itself can + // quote GitHub App responses, and that must not cross to a + // contributor-controlled process. + if strings.Contains(strings.ToLower(reason), "ghs_") || strings.Contains(reason, "token ") { + t.Fatalf("reason looks like it leaked auth detail: %q", reason) + } + if _, ok := wire["github_token"]; ok { + t.Fatalf("token material on a failure notice: %s", data) + } +} + +// TestResumeTaskToken_MintFailureNotifiesRelay: the reconnect path must be as +// loud as the heartbeat one. A resume whose mint fails leaves the relay holding +// a credential minted before the disconnect, with refresh un-armed — exactly the +// state in which a later push fails for no visible reason (#5447). +func TestResumeTaskToken_MintFailureNotifiesRelay(t *testing.T) { + hub := &ContributeWSHub{logger: slog.Default()} + s := NewServer(0, slog.Default()) + s.deps = &Dependencies{GHAppAuth: newFailingAppAuth(t)} + hub.server = s + + server, client := wsPair(t) + conn := refreshConn(server, time.Time{}) + + hub.resumeTaskToken(conn, &taskLease{taskID: "t-resume", repo: "o/r", number: 7, tier: "contributor", gen: 1}) + + _ = client.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, data, err := client.ReadMessage() + if err != nil { + t.Fatalf("a failed resume mint told the relay nothing: %v", err) + } + var wire map[string]any + if err := json.Unmarshal(data, &wire); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if wire["type"] != "token_refresh_failed" { + t.Fatalf("type = %v, want token_refresh_failed", wire["type"]) + } + if _, ok := wire["github_token"]; ok { + t.Fatalf("token material on a failure notice: %s", data) + } + + // The lenient retry policy is unchanged: a failed resume mint must leave + // tokenMintedAt zero so a later resume or heartbeat can still arm refresh. + conn.mu.Lock() + minted := conn.tokenMintedAt + conn.mu.Unlock() + if !minted.IsZero() { + t.Fatalf("tokenMintedAt = %s, want zero after a failed resume mint", minted) + } +} + +// TestServerCapabilitiesAdvertisesTokenRefreshFailed: the notice is only useful +// if a client can learn the hub sends it without probing (#2567's contract). +func TestServerCapabilitiesAdvertisesTokenRefreshFailed(t *testing.T) { + caps := serverCapabilities() + for _, c := range caps { + if c == capTokenRefreshFailed { + return + } + } + t.Fatalf("serverCapabilities() = %v, missing %q", caps, capTokenRefreshFailed) +} diff --git a/src/pkg/dashboard/contribute_ws_session_identity_test.go b/src/pkg/dashboard/contribute_ws_session_identity_test.go new file mode 100644 index 000000000..2c97d87b2 --- /dev/null +++ b/src/pkg/dashboard/contribute_ws_session_identity_test.go @@ -0,0 +1,69 @@ +package dashboard + +import "testing" + +// Multi-session-per-account (one GitHub account, several concurrent relays — +// e.g. one per CLI backend). identityOf() is the key for task leases, +// assignment cooldowns, failure streaks and ownership fences, so two sessions +// under one account MUST resolve to distinct identities or they collide on a +// single active-task slot. A contributor that declares no session keeps the +// historical bare-ContributorID identity (backward compatibility). + +func TestIdentityOfSessionScoping(t *testing.T) { + profile := &ContributorProfile{GitHubUsername: "hanthor", ContributorID: "c-abc123", TrustTier: "contributor"} + + bare := &ContributorConnection{profile: profile} + if got := identityOf(bare); got != "c-abc123" { + t.Fatalf("no session: identityOf = %q, want the bare ContributorID %q", got, "c-abc123") + } + + claude := &ContributorConnection{profile: profile, session: "claude"} + agy := &ContributorConnection{profile: profile, session: "agy"} + if identityOf(claude) == identityOf(agy) { + t.Fatalf("two sessions under one account share an identity (%q) — they will collide on one task slot", identityOf(claude)) + } + if got, want := identityOf(claude), "c-abc123#claude"; got != want { + t.Fatalf("session identity = %q, want %q", got, want) + } + if identityOf(bare) == identityOf(claude) { + t.Fatalf("a sessioned relay collides with the bare identity; existing single-session contributors would be displaced") + } +} + +func TestIdentityOfFallsBackToUsername(t *testing.T) { + // No ContributorID (a profile created before IDs, or username-only): the + // session still scopes off the username so the fallback path is not a + // single-slot regression. + profile := &ContributorProfile{GitHubUsername: "hanthor"} + sessioned := &ContributorConnection{profile: profile, session: "pi"} + if got, want := identityOf(sessioned), "hanthor#pi"; got != want { + t.Fatalf("username fallback with session = %q, want %q", got, want) + } + bare := &ContributorConnection{profile: profile} + if got, want := identityOf(bare), "hanthor"; got != want { + t.Fatalf("username fallback no session = %q, want %q", got, want) + } +} + +func TestSanitizeSessionLabel(t *testing.T) { + cases := map[string]string{ + "": "", + "claude": "claude", + "pi-codex": "pi-codex", + "Agy_1.2": "Agy_1.2", + "bad/label": "badlabel", // path chars stripped + "a b\tc": "abc", // whitespace stripped + "../../etc": "....etc", // no traversal survives as separators + "drop#tables": "droptables", // '#' (our separator) stripped + "0123456789012345678901234567890123456789": "01234567890123456789012345678901", // capped at 32 + } + for in, want := range cases { + if got := sanitizeSessionLabel(in); got != want { + t.Errorf("sanitizeSessionLabel(%q) = %q, want %q", in, got, want) + } + } + // The cap must never emit more than 32 bytes. + if got := sanitizeSessionLabel("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); len(got) > 32 { + t.Errorf("sanitizeSessionLabel over-long output len=%d, want <=32", len(got)) + } +} diff --git a/src/pkg/dashboard/contribute_ws_token_refresh_test.go b/src/pkg/dashboard/contribute_ws_token_refresh_test.go index 012a50ff8..68357d6a0 100644 --- a/src/pkg/dashboard/contribute_ws_token_refresh_test.go +++ b/src/pkg/dashboard/contribute_ws_token_refresh_test.go @@ -140,6 +140,7 @@ func TestSendTokenRefreshShape(t *testing.T) { // client can read back and inspect. var serverConn *websocket.Conn connReady := make(chan struct{}) + handlerDone := make(chan struct{}) upgrader := websocket.Upgrader{} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c, err := upgrader.Upgrade(w, r, nil) @@ -149,10 +150,12 @@ func TestSendTokenRefreshShape(t *testing.T) { } serverConn = c close(connReady) - // Keep the handler alive so the conn stays open for the read below. - time.Sleep(2 * time.Second) + // Keep the handler alive until the test body is done with the conn; + // released by the deferred close below rather than a fixed timer. + <-handlerDone })) defer srv.Close() + defer close(handlerDone) client, _, err := websocket.DefaultDialer.Dial("ws"+strings.TrimPrefix(srv.URL, "http"), nil) if err != nil { @@ -226,6 +229,7 @@ func TestResumeTaskTokenReArmsRefresh(t *testing.T) { // Real websocket pair so resumeTaskToken -> sendTokenRefresh writes a frame. var serverConn *websocket.Conn connReady := make(chan struct{}) + handlerDone := make(chan struct{}) upgrader := websocket.Upgrader{} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c, err := upgrader.Upgrade(w, r, nil) @@ -235,9 +239,12 @@ func TestResumeTaskTokenReArmsRefresh(t *testing.T) { } serverConn = c close(connReady) - time.Sleep(2 * time.Second) + // Keep the handler alive until the test body is done with the conn; + // released by the deferred close below rather than a fixed timer. + <-handlerDone })) defer srv.Close() + defer close(handlerDone) client, _, err := websocket.DefaultDialer.Dial("ws"+strings.TrimPrefix(srv.URL, "http"), nil) if err != nil { diff --git a/src/pkg/dashboard/coverage_boost4_test.go b/src/pkg/dashboard/coverage_boost4_test.go index 4ce675689..5d20744c9 100644 --- a/src/pkg/dashboard/coverage_boost4_test.go +++ b/src/pkg/dashboard/coverage_boost4_test.go @@ -376,15 +376,9 @@ func TestHandleKnowledgeCreate_MissingFields_Boost(t *testing.T) { // --- buildHealth with cached health --- func TestBuildHealth_Cached(t *testing.T) { - // Set cached health first - cachedHealthMu.Lock() - cachedHealth = map[string]any{"ci": 95, "nightly": 100} - cachedHealthMu.Unlock() - defer func() { - cachedHealthMu.Lock() - cachedHealth = nil - cachedHealthMu.Unlock() - }() + // Seed cached health first; the shared hook restores the pre-test cache + // state in t.Cleanup (#5570). + setCachedHealth(t, map[string]any{"ci": 95, "nightly": 100}) // Call with nil client to get cached result := buildHealth(nil, nil) @@ -705,7 +699,7 @@ func TestHandleContributorTrust_UserNotFound(t *testing.T) { body := `{"username":"nonexistent","tier":"contributor"}` req := httptest.NewRequest("PUT", "/api/contribute/trust", strings.NewReader(body)) req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-Hive-Role", "owner") // mutation gate fails closed on a missing role (C5) + req.Header.Set("X-Hive-Role", "owner") // mutation gate fails closed on a missing role (C5) req.Header.Set(ownerRoleVerifiedHeader, "true") // requireOwnerRole needs the verified marker too (F14) w := httptest.NewRecorder() srv.handleContributorTrust(w, req) diff --git a/src/pkg/dashboard/csp_base_script_src.go b/src/pkg/dashboard/csp_base_script_src.go index 86fb25e43..d48607802 100644 --- a/src/pkg/dashboard/csp_base_script_src.go +++ b/src/pkg/dashboard/csp_base_script_src.go @@ -16,8 +16,33 @@ import ( var ( baseScriptSrcElemOnce sync.Once baseScriptSrcElemSources string + + brandedIndexMu sync.RWMutex + brandedIndex []byte ) +// setBrandedIndex records the index document AS SERVED, so CSP hashes are +// computed over the same bytes the browser receives. +// +// This matters because branding can rewrite inline SCRIPT content, not just +// markup: the Getting Started flyer builds its DOM from a JavaScript string +// literal that contains `🐝`. Replacing the +// mark there changes a script's bytes, and hashes taken from the embedded +// document would no longer authorise it — CSP would block the flyer on a +// branded hive. Must be called before the first request is served. +func setBrandedIndex(doc []byte) { + brandedIndexMu.Lock() + brandedIndex = append([]byte(nil), doc...) + // Invalidate any previously memoised source list. baseScriptSrcElem uses a + // sync.Once, so without this the hashes depend on whether anything happened + // to ask for them before the document was built — in production Start() + // builds it before serving, but that is an ordering assumption rather than + // a guarantee, and it is exactly the kind of thing that works until it + // silently does not. Safe because this runs at startup, before any request. + baseScriptSrcElemOnce = sync.Once{} + brandedIndexMu.Unlock() +} + // baseScriptSrcElem returns the startup-computed script-src-elem source list // covering the two documents whose bytes are fixed for the life of the // process: the embedded SPA (static/index.html, served verbatim by both @@ -27,7 +52,12 @@ var ( func baseScriptSrcElem() string { baseScriptSrcElemOnce.Do(func() { var docs []byte - if raw, err := fs.ReadFile(staticFS, "static/index.html"); err == nil { + brandedIndexMu.RLock() + branded := brandedIndex + brandedIndexMu.RUnlock() + if len(branded) > 0 { + docs = append(docs, branded...) + } else if raw, err := fs.ReadFile(staticFS, "static/index.html"); err == nil { docs = append(docs, raw...) } docs = append(docs, []byte(loginPage)...) diff --git a/src/pkg/dashboard/csp_script_src_test.go b/src/pkg/dashboard/csp_script_src_test.go index 20ee3144f..dc3050b36 100644 --- a/src/pkg/dashboard/csp_script_src_test.go +++ b/src/pkg/dashboard/csp_script_src_test.go @@ -185,8 +185,18 @@ func TestEmbeddedIndexScriptsSatisfyCSPHashes(t *testing.T) { if err != nil { t.Fatalf("decompressing body: %v", err) } - if string(body) != string(raw) { - t.Fatal("served SPA bytes differ from the embedded document — startup hashes would not match") + // The invariant is that hashes describe the SERVED bytes — not that the + // served bytes equal the embed. Branding may legitimately rewrite the + // document (including inline script content, e.g. the Getting Started + // flyer builds DOM from a string literal containing the bee mark), so the + // CSP layer hashes the served document. Assert the real property: every + // inline script in the served body is authorised by the header. + served := webstatic.ScriptSrcElemSources(body) + for _, sc := range webstatic.ExtractInlineScripts(body) { + if h := webstatic.CSPScriptHash(sc); !strings.Contains(served, h) { + t.Fatalf("served inline script is not covered by a CSP hash (%s) — "+ + "the browser would block it", h) + } } // POSITIVE CONTROL + COUNT FLOOR: the document still contains its scripts. diff --git a/src/pkg/dashboard/f16_owner_gate_test.go b/src/pkg/dashboard/f16_owner_gate_test.go index 24430c07c..e4ad88810 100644 --- a/src/pkg/dashboard/f16_owner_gate_test.go +++ b/src/pkg/dashboard/f16_owner_gate_test.go @@ -90,11 +90,45 @@ func TestF16PrivilegedHandlersAreOwnerGated(t *testing.T) { // explicitly, so the reason this fix exists survives even if someone prunes the // table above. handleGovernorSecurity is the only handler that writes // cfg.AgentSandbox.Enabled from request input. +// +// #5388 item 2: this test previously anchored on the Go identifier +// "AgentSandboxEnabled" and SKIPPED when it was absent. That made the skip +// condition identical to the coupling being guarded, so the guard retired +// itself on the one edit most likely to disturb it. Demonstrated: renaming the +// struct field to SandboxOn — leaving the JSON tag, the config write and the +// whole wire contract untouched, so the toggle is still fully reachable — +// turned this from an owner-gate security assertion into a silent SKIP, and +// nothing else in the file re-asserts that THIS surface writes the sandbox +// toggle. The sibling table test still catches an outright gate removal, but it +// does not know the sandbox toggle exists, so after a rename the highest-impact +// item is guarded only generically and this test never speaks again. +// +// The fix anchors on the two things that are the actual contract rather than a +// private naming choice: the JSON wire field the browser sends, and the config +// field the handler writes. Both must change for the toggle to genuinely move, +// and if they do, this test FAILS and names itself rather than skipping. func TestF16AgentSandboxToggleIsOwnerOnly(t *testing.T) { body := f16HandlerBody(t, f16ReadSource(t, "api_governor_security.go"), "handleGovernorSecurity") - if !strings.Contains(body, "AgentSandboxEnabled") { - t.Skip("handleGovernorSecurity no longer accepts agentSandboxEnabled; the sandbox toggle moved — re-point this test") + + // The wire field and the config write, not the Go identifier that carries + // them between the two. A rename of the local struct field changes neither. + const wireField = `json:"agentSandboxEnabled"` + const configWrite = "cfg.AgentSandbox.Enabled =" + + hasWire := strings.Contains(body, wireField) + hasWrite := strings.Contains(body, configWrite) + + // If the toggle really did move, that is a deliberate change to a security + // surface and must be re-pointed by a human — so fail loudly. It is never + // correct for this assertion to go quiet on its own. + if !hasWire || !hasWrite { + t.Fatalf("handleGovernorSecurity no longer both accepts %s (found=%v) and writes %s (found=%v) — "+ + "the agent-sandbox toggle moved. Re-point this test at whichever handler now writes "+ + "cfg.AgentSandbox.Enabled and confirm THAT handler is owner-gated. Do not delete this case: "+ + "it is the only assertion that names the sandbox toggle specifically (audit F16, #5388)", + wireField, hasWire, configWrite, hasWrite) } + if !strings.Contains(body, "requireOwnerRole(w, r)") { t.Error("handleGovernorSecurity accepts agentSandboxEnabled but is not owner-gated — " + "a read-write member can disable the agent sandbox (audit F16)") diff --git a/src/pkg/dashboard/governor_config_get_contract_test.go b/src/pkg/dashboard/governor_config_get_contract_test.go new file mode 100644 index 000000000..752a400ef --- /dev/null +++ b/src/pkg/dashboard/governor_config_get_contract_test.go @@ -0,0 +1,392 @@ +package dashboard + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "sort" + "strings" + "testing" + + "github.com/hivecommons/hive/pkg/config" + "github.com/hivecommons/hive/pkg/github" +) + +// ============================================================ +// handleGovernorConfigGet — response-contract tests. +// +// This handler (api.go, GET /api/config/governor) is the single source the +// dashboard Settings UI reads to render the Governor tabs (Agents, Labels, +// Notifications, Hub, ...). It has accumulated 16 fix commits over time, each +// patching one section's shape or semantics without a test pinning the +// contract of the other sections. These tests lock down the sections most +// prone to silent regression: the org-qualification logic for repos, the +// idle-exclusion + effective-threshold scaling for thresholds, secret +// masking for notifications, and the label-polarity split between +// "requireLabels" (project.issue_filter) and "labels" (governor.labels.exempt). +// +// Each test asserts both a positive (the right value appears) and a negative +// (the wrong/raw value does NOT appear) so a handler that returns the wrong +// thing, or leaks something it must mask, cannot pass silently. +// ============================================================ + +const ( + testRepoCountThree = 3 // used to exercise threshold scaling with a non-1 repo count +) + +func decodeGovernorConfigGet(t *testing.T, srv *Server) map[string]any { + t.Helper() + req := httptest.NewRequest("GET", "/api/config/governor", nil) + w := httptest.NewRecorder() + srv.handleGovernorConfigGet(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("code = %d, want %d (body: %s)", w.Code, http.StatusOK, w.Body.String()) + } + var result map[string]any + if err := json.NewDecoder(w.Body).Decode(&result); err != nil { + t.Fatalf("decode response: %v (body: %s)", err, w.Body.String()) + } + return result +} + +// --- agents --- + +func TestHandleGovernorConfigGet_AgentsListsConfiguredNamesOnly(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Agents = map[string]config.AgentConfig{ + "scanner": {ID: "scan-001", Role: "scanner", Backend: "claude", Model: "sonnet", Enabled: true}, + "reviewer": {ID: "rev-001", Role: "reviewer", Backend: "claude", Model: "sonnet", Enabled: true}, + "outreach": {ID: "out-001", Role: "outreach", Backend: "claude", Model: "sonnet", Enabled: false}, + } + + result := decodeGovernorConfigGet(t, srv) + + raw, ok := result["agents"].([]any) + if !ok { + t.Fatalf("agents section missing or wrong type: %v", result["agents"]) + } + got := make([]string, 0, len(raw)) + for _, v := range raw { + got = append(got, v.(string)) + } + sort.Strings(got) + want := []string{"outreach", "reviewer", "scanner"} + if len(got) != len(want) { + t.Fatalf("agents = %v, want exactly %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("agents[%d] = %q, want %q", i, got[i], want[i]) + } + } + // Negative: an agent name that was never configured must not appear. + for _, name := range got { + if name == "outreach-ghost" { + t.Errorf("agents contains unconfigured name %q", name) + } + } +} + +// --- thresholds / effectiveThresholds / repoCount --- + +func TestHandleGovernorConfigGet_ThresholdsExcludeIdleAndEffectiveThresholdsMatchConfig(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Project.Repos = []string{"repo-a", "repo-b", "repo-c"} + srv.deps.Config.Governor.Modes = map[string]config.ModeConfig{ + "idle": {Threshold: 0}, + "quiet": {Threshold: 5}, + "busy": {Threshold: 15}, + "surge": {Threshold: 30}, + } + + result := decodeGovernorConfigGet(t, srv) + + thresholds, ok := result["thresholds"].(map[string]any) + if !ok { + t.Fatalf("thresholds section missing or wrong type: %v", result["thresholds"]) + } + // Positive: non-idle modes present with their configured values. + if thresholds["quiet"].(float64) != 5 { + t.Errorf("thresholds[quiet] = %v, want 5", thresholds["quiet"]) + } + if thresholds["busy"].(float64) != 15 { + t.Errorf("thresholds[busy] = %v, want 15", thresholds["busy"]) + } + if thresholds["surge"].(float64) != 30 { + t.Errorf("thresholds[surge] = %v, want 30", thresholds["surge"]) + } + // Negative: idle must never appear, even though cfg.Governor.Modes has an + // idle entry. + if _, present := thresholds["idle"]; present { + t.Errorf("thresholds contains idle entry: %v", thresholds["idle"]) + } + + repoCount := srv.deps.Config.Project.RepoCount() + if repoCount != testRepoCountThree { + t.Fatalf("test setup: RepoCount() = %d, want %d", repoCount, testRepoCountThree) + } + if got, want := result["repoCount"].(float64), float64(repoCount); got != want { + t.Errorf("repoCount = %v, want %v", got, want) + } + + effective, ok := result["effectiveThresholds"].(map[string]any) + if !ok { + t.Fatalf("effectiveThresholds section missing or wrong type: %v", result["effectiveThresholds"]) + } + // Positive: exactly the three modes, each equal to EffectiveThreshold. + wantModes := []string{"quiet", "busy", "surge"} + if len(effective) != len(wantModes) { + t.Fatalf("effectiveThresholds has %d keys (%v), want exactly %v", len(effective), effective, wantModes) + } + for _, mode := range wantModes { + want := float64(srv.deps.Config.Governor.EffectiveThreshold(mode, repoCount)) + got, present := effective[mode] + if !present { + t.Errorf("effectiveThresholds missing key %q", mode) + continue + } + if got.(float64) != want { + t.Errorf("effectiveThresholds[%s] = %v, want %v", mode, got, want) + } + } + // Negative: no extra keys (e.g. idle) leaked into effectiveThresholds. + if _, present := effective["idle"]; present { + t.Errorf("effectiveThresholds contains idle entry: %v", effective["idle"]) + } +} + +// --- repos / primaryRepo org-qualification --- + +func TestHandleGovernorConfigGet_ReposOrgQualifiesBareNamesOnly(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Project.Org = "hivecommons" + srv.deps.Config.Project.Repos = []string{"hive", "otherorg/already-qualified"} + srv.deps.Config.Project.PrimaryRepo = "hive" + + result := decodeGovernorConfigGet(t, srv) + + raw, ok := result["repos"].([]any) + if !ok { + t.Fatalf("repos section missing or wrong type: %v", result["repos"]) + } + got := make([]string, 0, len(raw)) + for _, v := range raw { + got = append(got, v.(string)) + } + want := []string{"hivecommons/hive", "otherorg/already-qualified"} + if len(got) != len(want) { + t.Fatalf("repos = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("repos[%d] = %q, want %q", i, got[i], want[i]) + } + } + // Negative: the already-qualified repo must not get double-prefixed. + for _, r := range got { + if strings.Contains(r, "kubestellar/otherorg") { + t.Errorf("repos double-prefixed an already-qualified name: %q", r) + } + } + + if got, want := result["primaryRepo"].(string), "hivecommons/hive"; got != want { + t.Errorf("primaryRepo = %q, want %q", got, want) + } +} + +func TestHandleGovernorConfigGet_PrimaryRepoAlreadyQualifiedUnchangedAndEmptyStaysEmpty(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Project.Org = "hivecommons" + srv.deps.Config.Project.PrimaryRepo = "otherorg/already-qualified" + + result := decodeGovernorConfigGet(t, srv) + if got, want := result["primaryRepo"].(string), "otherorg/already-qualified"; got != want { + t.Errorf("primaryRepo = %q, want unchanged %q", got, want) + } + + srv2 := newFullServer(t) + srv2.deps.Config.Project.Org = "hivecommons" + srv2.deps.Config.Project.PrimaryRepo = "" + result2 := decodeGovernorConfigGet(t, srv2) + if got := result2["primaryRepo"].(string); got != "" { + t.Errorf("primaryRepo = %q, want empty string to stay empty", got) + } +} + +// --- notifications --- + +func TestHandleGovernorConfigGet_NotificationsEmptyWhenUnconfigured(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Notifications = config.NotificationsConfig{} + + result := decodeGovernorConfigGet(t, srv) + n, ok := result["notifications"].(map[string]any) + if !ok { + t.Fatalf("notifications section missing or wrong type: %v", result["notifications"]) + } + if n["hasNtfy"] != false { + t.Errorf("hasNtfy = %v, want false", n["hasNtfy"]) + } + if n["hasDiscord"] != false { + t.Errorf("hasDiscord = %v, want false", n["hasDiscord"]) + } + for _, field := range []string{"ntfyServer", "ntfyTopic", "discordWebhook"} { + if n[field] != "" { + t.Errorf("%s = %v, want empty string", field, n[field]) + } + } +} + +func TestHandleGovernorConfigGet_DiscordWebhookMaskedNeverRaw(t *testing.T) { + srv := newFullServer(t) + const rawWebhook = "https://discord.com/api/webhooks/1234567890/super-secret-token-value" + srv.deps.Config.Notifications.Discord = &config.DiscordConfig{Webhook: rawWebhook} + + req := httptest.NewRequest("GET", "/api/config/governor", nil) + w := httptest.NewRecorder() + srv.handleGovernorConfigGet(w, req) + if w.Code != http.StatusOK { + t.Fatalf("code = %d", w.Code) + } + body := w.Body.String() + // Negative: the raw webhook must not appear anywhere in the response. + if strings.Contains(body, rawWebhook) { + t.Fatal("response body contains the raw discord webhook") + } + + var result map[string]any + if err := json.NewDecoder(strings.NewReader(body)).Decode(&result); err != nil { + t.Fatal(err) + } + n := result["notifications"].(map[string]any) + if n["hasDiscord"] != true { + t.Errorf("hasDiscord = %v, want true", n["hasDiscord"]) + } + masked, ok := n["discordWebhook"].(string) + if !ok || masked == "" { + t.Fatalf("discordWebhook = %v, want a masked non-empty string", n["discordWebhook"]) + } + if masked == rawWebhook { + t.Errorf("discordWebhook was not masked: %q", masked) + } + // Positive: masking preserves only the last 4 characters (maskSecret). + if want := maskSecret(rawWebhook); masked != want { + t.Errorf("discordWebhook = %q, want %q (maskSecret output)", masked, want) + } +} + +func TestHandleGovernorConfigGet_NtfyConfiguredReflectsServerAndTopic(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Notifications.Ntfy = &config.NtfyConfig{ + Server: "https://ntfy.example.com", + Topic: "hive-alerts", + } + + result := decodeGovernorConfigGet(t, srv) + n := result["notifications"].(map[string]any) + if n["hasNtfy"] != true { + t.Errorf("hasNtfy = %v, want true", n["hasNtfy"]) + } + if got, want := n["ntfyServer"].(string), "https://ntfy.example.com"; got != want { + t.Errorf("ntfyServer = %q, want %q", got, want) + } + if got, want := n["ntfyTopic"].(string), "hive-alerts"; got != want { + t.Errorf("ntfyTopic = %q, want %q", got, want) + } + // Negative: discord must remain unaffected/absent when only ntfy is set. + if n["hasDiscord"] != false { + t.Errorf("hasDiscord = %v, want false when discord unconfigured", n["hasDiscord"]) + } +} + +// --- requireLabels / labels / holdLabels --- + +func TestHandleGovernorConfigGet_LabelPolaritySplit(t *testing.T) { + srv := newFullServer(t) + srv.deps.Config.Project.IssueFilter.RequireLabels = []string{"needs-triage", "ready"} + srv.deps.Config.Governor.Labels.Exempt = []string{"do-not-automate", "manual-only"} + + result := decodeGovernorConfigGet(t, srv) + + requireLabels := toStringSlice(t, result["requireLabels"]) + if !equalStringSlices(requireLabels, []string{"needs-triage", "ready"}) { + t.Errorf("requireLabels = %v, want [needs-triage ready]", requireLabels) + } + + labels := toStringSlice(t, result["labels"]) + if !equalStringSlices(labels, []string{"do-not-automate", "manual-only"}) { + t.Errorf("labels = %v, want [do-not-automate manual-only]", labels) + } + + // Negative: the two lists must not be swapped or merged. + if equalStringSlices(requireLabels, labels) { + t.Fatal("requireLabels and labels must not be equal — they are opposite-polarity lists") + } + + holdLabels := toStringSlice(t, result["holdLabels"]) + if !equalStringSlices(holdLabels, github.HoldLabels) { + t.Errorf("holdLabels = %v, want %v", holdLabels, github.HoldLabels) + } +} + +// --- top-level section presence --- + +func TestHandleGovernorConfigGet_TopLevelSectionsPresent(t *testing.T) { + srv := newFullServer(t) + + result := decodeGovernorConfigGet(t, srv) + + wantKeys := []string{ + "agents", "thresholds", "effectiveThresholds", "repos", "primaryRepo", + "notifications", "budget", "health", "sensing", "logging", "litellm", + "review", "auto_merge", "hub", "attribution", + } + for _, key := range wantKeys { + if _, present := result[key]; !present { + t.Errorf("response missing top-level key %q", key) + } + } + // Negative: a key the UI has never depended on should not be asserted as + // required — guard against a typo'd want list silently always passing by + // checking a deliberately-absent key is in fact absent from wantKeys. + for _, key := range wantKeys { + if key == "does-not-exist-marker" { + t.Fatal("wantKeys sanity check failed") + } + } +} + +// --- helpers --- + +func toStringSlice(t *testing.T, v any) []string { + t.Helper() + if v == nil { + return nil + } + raw, ok := v.([]any) + if !ok { + t.Fatalf("value is not a []any: %v (%T)", v, v) + } + out := make([]string, 0, len(raw)) + for _, item := range raw { + s, ok := item.(string) + if !ok { + t.Fatalf("slice element is not a string: %v (%T)", item, item) + } + out = append(out, s) + } + return out +} + +func equalStringSlices(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/src/pkg/dashboard/health_cache_guard_test.go b/src/pkg/dashboard/health_cache_guard_test.go new file mode 100644 index 000000000..ae7f751b2 --- /dev/null +++ b/src/pkg/dashboard/health_cache_guard_test.go @@ -0,0 +1,51 @@ +package dashboard + +import "testing" + +// restoreHealthCaches is the isolation hook for the package-level health +// caches (#5570). buildHealth's live-client path writes cachedHealth and can +// refresh cachedGreenStreak (status_builder.go), and nothing in non-test code +// ever clears either — so any test that reaches that path, or seeds the caches +// directly as a fixture, leaks state into every later test that exercises the +// nil-client branch and asserts the {"ci": 100} default. Under -shuffle that +// surfaces as an order-dependent failure (e.g. TestBuildHealth_NilClient_Boost +// failing with "ci = 0" whenever TestCovH2_BuildHealthAndRateLimits happens to +// run first; minimal reproduction: -shuffle=11 with just that pair). +// +// Call this FIRST in any test that writes the caches — directly or via a +// non-nil GitHub client — instead of hand-rolling a snapshot/restore at each +// site. It snapshots both caches and registers a t.Cleanup restoring them, so +// the restore runs even when the test later calls t.Fatalf, and a future +// writer test cannot forget half the pair. +func restoreHealthCaches(tb testing.TB) { + tb.Helper() + + cachedHealthMu.Lock() + prevHealth := cachedHealth + cachedHealthMu.Unlock() + + cachedGreenStreakMu.Lock() + prevStreak, prevStreakOK := cachedGreenStreak, cachedGreenStreakOK + cachedGreenStreakMu.Unlock() + + tb.Cleanup(func() { + cachedHealthMu.Lock() + cachedHealth = prevHealth + cachedHealthMu.Unlock() + + cachedGreenStreakMu.Lock() + cachedGreenStreak, cachedGreenStreakOK = prevStreak, prevStreakOK + cachedGreenStreakMu.Unlock() + }) +} + +// setCachedHealth seeds the cachedHealth fixture for tests that assert the +// cached-read branch of buildHealth. It routes through restoreHealthCaches so +// seeding a fixture can never leak past the test that set it. +func setCachedHealth(tb testing.TB, health map[string]any) { + tb.Helper() + restoreHealthCaches(tb) + cachedHealthMu.Lock() + cachedHealth = health + cachedHealthMu.Unlock() +} diff --git a/src/pkg/dashboard/lifecycle_journeys_ui_test.go b/src/pkg/dashboard/lifecycle_journeys_ui_test.go new file mode 100644 index 000000000..fd2393feb --- /dev/null +++ b/src/pkg/dashboard/lifecycle_journeys_ui_test.go @@ -0,0 +1,65 @@ +package dashboard + +import ( + "strings" + "testing" +) + +// TestLifecycleJourneysPanelPinned pins Panel B's journey rendering (#5656). +// Before this, the panel listed raw events and only ever showed the latest +// re-stamped enumeration sweep with "0 merged / 0 blocked" forever. The +// invariants: +// +// 1. The panel renders JOURNEYS (one row per work item) from the DTO's +// `journeys` array, not raw events. +// 2. Each row shows stage chips along the fixed lifecycle axis, with the +// current stage colored via the shared v4KindClass mapping and earlier +// stages dimmed. +// 3. The fleet counters carry an honest coverage label derived from +// fleet.coveredMs — never claiming a 6h window over minutes of history. +// 4. The fetch path and empty state stay wired. +func TestLifecycleJourneysPanelPinned(t *testing.T) { + html := indexHTML(t) + for _, snippet := range []string{ + // Journey rendering, not raw events. + "renderLifecycle", + "dto.journeys", + `
  • `, + `
      `, + // Fixed stage axis + chips through the shared kind→color mapping. + "LC_STAGE_AXIS", + "['enumerated', 'classified', 'kicked', 'pr_opened', 'merged', 'blocked']", + "v4KindClass(k)", + "lc-stage past", + ``, + // Honest window labeling from real coverage. + "lcWindowLabel", + "fleet.coveredMs", + "of recorded history", + // Counters remain, fed by the journeys roll-up. + `
      `, + `
      `, + `
      `, + // Fetch path + calm empty state. + "'/api/lifecycle-timeline?limit=50'", + "No lifecycle journeys yet", + } { + if !strings.Contains(html, snippet) { + t.Fatalf("lifecycle journeys panel missing snippet %q", snippet) + } + } +} + +// TestLifecycleJourneysPanelDropsRawEventList: the flooding raw-event list +// must not come back alongside the journey view. +func TestLifecycleJourneysPanelDropsRawEventList(t *testing.T) { + html := indexHTML(t) + for _, gone := range []string{ + `
        `, + "No lifecycle events yet", + } { + if strings.Contains(html, gone) { + t.Fatalf("raw-event lifecycle markup %q should be gone", gone) + } + } +} diff --git a/src/pkg/dashboard/login_code.go b/src/pkg/dashboard/login_code.go new file mode 100644 index 000000000..4f24039a8 --- /dev/null +++ b/src/pkg/dashboard/login_code.go @@ -0,0 +1,61 @@ +package dashboard + +import ( + "net/http" +) + +// handleAgentLoginCode types an operator-supplied authorization code into an +// agent's pane, completing an OAuth hand-off the dashboard terminal cannot. +// +// It is the write-side twin of handleAgentTerminalURLs. That endpoint exists +// because the terminal cannot deliver a COPY (#5188); this one exists because +// it cannot reliably accept a PASTE either. An operator who has opened the +// sign-in URL is then asked to paste a code back into a ttyd/xterm.js pane over +// tmux, where it arrives mangled — leaving a host shell and `tmux send-keys` as +// the only route, which is no answer for an operator whose interface is a web +// dashboard. +// +// OWNER-ONLY, unlike its read-side twin. handleAgentTerminalURLs is a GET that +// any authenticated role may call because it exposes strictly less than the +// terminal proxy already does. This one WRITES to an interactive pane, so it +// takes the strictest gate the dashboard has rather than inheriting the read +// endpoint's reasoning. It grants no capability the writable ttyd terminal does +// not already give the same operator; the point of the gate is that a narrower +// door should not be easier to open than the wide one beside it. +// +// The code is validated in agent.SubmitLoginCode (printable, whitespace-free, +// length-bounded) so it cannot carry a newline and become a second command. It +// is never logged, and never echoed back in a response — the audit entry +// records that a code was submitted for an agent, not what it was. +func (s *Server) handleAgentLoginCode(w http.ResponseWriter, r *http.Request) { + if !requireOwnerRole(w, r) { + return + } + if s.deps == nil || s.deps.AgentMgr == nil { + jsonError(w, "agent manager unavailable", http.StatusServiceUnavailable) + return + } + + name := s.resolveAgentParam(r.PathValue("name")) + var body struct { + Code string `json:"code"` + } + if err := decodeBody(r, &body); err != nil { + jsonError(w, "invalid request body", http.StatusBadRequest) + return + } + + if err := s.deps.AgentMgr.SubmitLoginCode(name, body.Code); err != nil { + // SubmitLoginCode's errors describe the SHAPE of the problem and never + // quote the value, so they are safe to return to the operator who typed + // it — and they are the only way that operator learns why a paste was + // refused. + jsonError(w, err.Error(), http.StatusBadRequest) + return + } + + if s.deps.Logger != nil { + s.deps.Logger.Info("audit: login code submitted to agent", "agent", name, "trigger", "dashboard-api") + } + jsonResponse(w, map[string]interface{}{"ok": true, "agent": name}) +} diff --git a/src/pkg/dashboard/login_code_handler_test.go b/src/pkg/dashboard/login_code_handler_test.go new file mode 100644 index 000000000..59beb0428 --- /dev/null +++ b/src/pkg/dashboard/login_code_handler_test.go @@ -0,0 +1,105 @@ +package dashboard + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// loginCodeRequest builds an owner-authenticated POST to the login-code +// endpoint. requireOwnerRole demands both the role header and the server-set +// verified marker (F14); without either the handler never reaches the paths +// these tests exercise — that deny path is already pinned by +// TestV4OwnerOnlyHandlerGapsRejectUnverifiedOwners. +func loginCodeRequest(agent, body string) *http.Request { + req := httptest.NewRequest(http.MethodPost, "/api/agents/"+agent+"/login-code", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Hive-Role", "owner") + req.Header.Set(ownerRoleVerifiedHeader, "true") + req.SetPathValue("name", agent) + return req +} + +// Without an agent manager the handler must fail with 503, not panic: the +// endpoint dereferences s.deps.AgentMgr and a dashboard can serve requests +// before its dependencies are wired. +func TestHandleAgentLoginCode_NoAgentManager(t *testing.T) { + srv := &Server{} + w := httptest.NewRecorder() + srv.handleAgentLoginCode(w, loginCodeRequest("scanner", `{"code":"ABCD-1234"}`)) + + if w.Code != http.StatusServiceUnavailable { + t.Errorf("code = %d, want 503 when AgentMgr is nil", w.Code) + } +} + +// A body that is not the expected JSON shape must be a 400, and must be +// rejected before anything is typed toward a pane. +func TestHandleAgentLoginCode_InvalidBody(t *testing.T) { + srv := newFullServer(t) + w := httptest.NewRecorder() + srv.handleAgentLoginCode(w, loginCodeRequest("scanner", "not json")) + + if w.Code != http.StatusBadRequest { + t.Errorf("code = %d, want 400 for a malformed body", w.Code) + } +} + +// The security property of the endpoint: a code carrying a newline would be +// SUBMITTED mid-paste and everything after it typed as a fresh command line. +// agent.SubmitLoginCode must refuse it, the handler must surface that as 400, +// and neither the response nor the error may echo the submitted value. +func TestHandleAgentLoginCode_RejectsInjectableCode(t *testing.T) { + srv := newFullServer(t) + for _, tc := range []struct{ name, code string }{ + {"newline injection", "ABCD\\nrm -rf /"}, + {"embedded space", "ABCD 1234"}, + {"empty", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + w := httptest.NewRecorder() + srv.handleAgentLoginCode(w, loginCodeRequest("scanner", `{"code":"`+tc.code+`"}`)) + + if w.Code != http.StatusBadRequest { + t.Fatalf("code = %d, want 400 — %q must never reach an agent pane", w.Code, tc.code) + } + if strings.Contains(w.Body.String(), "rm -rf") { + t.Error("response echoes the rejected code — errors must describe the shape, never quote the value") + } + }) + } +} + +// A valid code for an agent the manager does not know must be a 400 with the +// agent named, so the operator learns which side of the request was wrong. +func TestHandleAgentLoginCode_UnknownAgent(t *testing.T) { + srv := newFullServer(t) + w := httptest.NewRecorder() + srv.handleAgentLoginCode(w, loginCodeRequest("ghost", `{"code":"ABCD-1234"}`)) + + if w.Code != http.StatusBadRequest { + t.Errorf("code = %d, want 400 for an unknown agent", w.Code) + } + if !strings.Contains(w.Body.String(), "ghost") { + t.Errorf("body = %q, want the unknown agent named", w.Body.String()) + } + if strings.Contains(w.Body.String(), "ABCD-1234") { + t.Error("response echoes the code — it must never be logged or returned") + } +} + +// A known agent with no live terminal session must also be a 400: the code +// has nowhere to go, and the handler must say so rather than pretend success. +func TestHandleAgentLoginCode_KnownAgentWithoutSession(t *testing.T) { + srv := newFullServer(t) + w := httptest.NewRecorder() + srv.handleAgentLoginCode(w, loginCodeRequest("scanner", `{"code":"ABCD-1234"}`)) + + if w.Code != http.StatusBadRequest { + t.Errorf("code = %d, want 400 when the agent has no terminal session", w.Code) + } + if strings.Contains(w.Body.String(), "ABCD-1234") { + t.Error("response echoes the code — it must never be logged or returned") + } +} diff --git a/src/pkg/dashboard/needs_human_ui_test.go b/src/pkg/dashboard/needs_human_ui_test.go new file mode 100644 index 000000000..5afe1117a --- /dev/null +++ b/src/pkg/dashboard/needs_human_ui_test.go @@ -0,0 +1,41 @@ +package dashboard + +import ( + "strings" + "testing" +) + +// TestNeedsHumanEscalationBadgePinned pins the dashboard's needs-human +// escalation surfacing in the Repositories section. The escalation ledger +// (pkg/escalation) labels a PR `needs-human` once automated fix attempts are +// exhausted; before this UI existed, nine escalated PRs sat invisible to the +// operator for a week (kubestellar/console, 2026-09-01). The invariants: +// +// 1. A PR row with the needs-human label renders a "needs human" state chip +// in the action-chip slot, taking precedence over Queue auto-merge (an +// escalated PR is out of the automated lane by definition). +// 2. The PR pill itself takes the needs-human (alert) tint over mergeable +// green. +// 3. The section header shows a "N need human review" counter, hidden at +// zero. +func TestNeedsHumanEscalationBadgePinned(t *testing.T) { + html := indexHTML(t) + for _, snippet := range []string{ + // Label check is the single source of truth (same label the + // escalation ledger writes — pkg/escalation NeedsHumanLabel). + "labels.includes('needs-human')", + // State chip in the action-chip slot. + "pill-needs-human-badge", + "⚠ needs human", + // Pill tint: needs-human wins over mergeable. + ".repo-pr-pill.needs-human", + "needsHuman ? ' needs-human' : mergeClass", + // Section-header counter element + text. + "repos-needs-human", + "' human review'", + } { + if !strings.Contains(html, snippet) { + t.Fatalf("dashboard needs-human escalation UI missing snippet %q", snippet) + } + } +} diff --git a/src/pkg/dashboard/next_kick_eta.go b/src/pkg/dashboard/next_kick_eta.go new file mode 100644 index 000000000..0a52d5142 --- /dev/null +++ b/src/pkg/dashboard/next_kick_eta.go @@ -0,0 +1,54 @@ +package dashboard + +import ( + "fmt" + "time" + + "github.com/hivecommons/hive/pkg/config" +) + +// Next-kick ETA (#5594). The agent card's one-line state summary answers "how +// long until this agent does something". computeNextKickFromCadence returns a +// wall-clock stamp and leaves the subtraction to the reader; this is the same +// instant expressed as a wait. + +// computeNextKickETA is empty exactly when computeNextKickFromCadence is empty, +// so the card can never show an ETA beside a schedule its own fields row calls +// "paused". +func computeNextKickETA(lastKick *time.Time, cadence config.Cadence) string { + if cadence == "" || cadence.IsPaused() { + return "" + } + now := time.Now() + base := now + if lastKick != nil && cadence.Mode() == config.CadenceModeInterval { + base = *lastKick + } + next, ok := cadence.NextAfter(base) + if !ok { + return "" + } + return formatETA(next.Sub(now)) +} + +// formatETA humanises a wait. A time that has already passed reads "due now" +// rather than a negative duration: the governor fires it on the next tick, and +// a leading minus sign read as a bug. +func formatETA(d time.Duration) string { + if d.Seconds() <= 0 { + return "due now" + } + if d.Seconds() < 60 { + return fmt.Sprintf("%ds", int(d.Round(time.Second).Seconds())) + } + if d.Minutes() < 60 { + return fmt.Sprintf("%dm", int(d.Round(time.Minute).Minutes())) + } + d = d.Round(time.Minute) + h := int(d / time.Hour) + m := int((d % time.Hour) / time.Minute) + if m == 0 { + return fmt.Sprintf("%dh", h) + } + return fmt.Sprintf("%dh %dm", h, m) +} diff --git a/src/pkg/dashboard/next_kick_eta_test.go b/src/pkg/dashboard/next_kick_eta_test.go new file mode 100644 index 000000000..83e2c9c00 --- /dev/null +++ b/src/pkg/dashboard/next_kick_eta_test.go @@ -0,0 +1,51 @@ +package dashboard + +import ( + "testing" + "time" + + "github.com/hivecommons/hive/pkg/config" +) + +func TestFormatETA(t *testing.T) { + tests := []struct { + d time.Duration + want string + }{ + {-3 * time.Minute, "due now"}, + {0, "due now"}, + {45 * time.Second, "45s"}, + {90 * time.Second, "2m"}, + {12 * time.Minute, "12m"}, + {2 * time.Hour, "2h"}, + {65 * time.Minute, "1h 5m"}, + } + for _, tc := range tests { + if got := formatETA(tc.d); got != tc.want { + t.Errorf("formatETA(%s) = %q, want %q", tc.d, got, tc.want) + } + } +} + +func TestComputeNextKickETAEmptyWhenNoKickScheduled(t *testing.T) { + now := time.Now() + for _, cad := range []config.Cadence{"", "pause", "off"} { + if got := computeNextKickETA(&now, cad); got != "" { + t.Errorf("computeNextKickETA(cadence=%q) = %q, want empty", cad, got) + } + if stamp := computeNextKickFromCadence(&now, cad); stamp != "" { + t.Errorf("computeNextKickFromCadence(cadence=%q) = %q, want empty", cad, stamp) + } + } +} + +func TestComputeNextKickETAIntervalCountsFromLastKick(t *testing.T) { + last := time.Now().Add(-25 * time.Minute) + if got := computeNextKickETA(&last, config.Cadence("30m")); got != "5m" { + t.Errorf("computeNextKickETA = %q, want %q", got, "5m") + } + overdue := time.Now().Add(-90 * time.Minute) + if got := computeNextKickETA(&overdue, config.Cadence("30m")); got != "due now" { + t.Errorf("overdue ETA = %q, want %q", got, "due now") + } +} diff --git a/src/pkg/dashboard/openapi_emitted_shape_test.go b/src/pkg/dashboard/openapi_emitted_shape_test.go new file mode 100644 index 000000000..0a56a1dcf --- /dev/null +++ b/src/pkg/dashboard/openapi_emitted_shape_test.go @@ -0,0 +1,174 @@ +package dashboard + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" +) + +// TestOpenAPIDeclaredTypeMatchesEmittedJSON is the #5574 guard. +// +// The two guards that already exist both leave this defect class open: +// +// - TestOpenAPISpecCoversEveryRegisteredRoute (#4912) proves every route is +// DOCUMENTED. It never reads a schema, so a route documented entirely in +// placeholders passes it. +// - TestOpenAPISchemaFieldsExistOnGoTypes (#5077) proves no spec property is +// INVENTED, by walking the spec against the Go type the handler marshals. +// It cannot see this one for two independent reasons: handleInferenceModels +// builds an inline map[string]interface{} rather than a named struct, so +// there is no type to pin it to; and its walk returns early on any object +// with no `properties`, which is precisely the placeholder shape at fault. +// +// So `models` was declared `items: {"type": "object"}` while every value the +// handler can assign is a []string — fetchInferenceModelsForBackendDetailed, +// inferenceStaticModelAliases and intersectEntitled all return []string. A +// client generated from the spec decodes into a struct and dies with +// +// json: cannot unmarshal object into Go value of type client.ModelOption +// +// which is the error the #5423 VHS fixture produced before its shape was +// corrected against the code rather than against the spec. +// +// This guard closes the direction those two miss, and does it the only way that +// cannot itself go stale: it runs the REAL handler and compares the JSON kind +// actually emitted against the kind the spec declares. Nothing here restates +// the spec by hand, so the test cannot agree with a wrong spec. +// +// It is deliberately narrow. It asserts the declared type of specific documented +// leaves, not that the whole payload is fully specified — dashboard/openapi.json +// currently carries 125 bare `{"type": "object"}` placeholders (16 under +// /api/status alone), and a guard demanding they all be filled in would fail +// permanently and be skipped, protecting nothing. Extending the table below is +// the cheap way to bring another leaf under the check. +func TestOpenAPIDeclaredTypeMatchesEmittedJSON(t *testing.T) { + // Mock inference endpoint advertising two models, so the handler takes its + // primary discovery path rather than the static-alias fallback. + mockSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": []map[string]any{ + {"id": "openai/gpt-5"}, + {"id": "claude_opus_4.8"}, + }, + }) + })) + defer mockSrv.Close() + + srv := newFullServer(t) + srv.inferenceEndpoints = map[string][]string{"vllm": {mockSrv.URL}} + + req := httptest.NewRequest("GET", "/api/inference/models/vllm", nil) + req.SetPathValue("backend", "vllm") + w := httptest.NewRecorder() + srv.handleInferenceModels(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("handleInferenceModels: code = %d, want 200", w.Code) + } + var emitted map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &emitted); err != nil { + t.Fatalf("decoding handler response: %v", err) + } + + models, ok := emitted["models"].([]any) + if !ok { + t.Fatalf("handler emitted models as %T, want a JSON array", emitted["models"]) + } + if len(models) == 0 { + t.Fatal("handler emitted an empty models array; this guard needs at least one " + + "element to compare against the declared item type") + } + + // The declared item type for /api/inference/models/{backend}.models[]. + declared := declaredItemType(t, "/api/inference/models/{backend}", "models") + + // Compare against every emitted element, so a mixed array cannot slip + // through on the strength of its first entry. + for i, m := range models { + if got := jsonKindOf(m); got != declared { + t.Errorf("dashboard/openapi.json declares "+ + "/api/inference/models/{backend}.models[].type = %q, but handleInferenceModels "+ + "emitted a %s at index %d (%#v).\n"+ + "Every source the handler can assign to `models` is a []string "+ + "(fetchInferenceModelsForBackendDetailed, inferenceStaticModelAliases, "+ + "intersectEntitled), so the SPEC is the wrong side here. A client generated "+ + "from it fails with: json: cannot unmarshal object into Go value of type "+ + "client.ModelOption", + declared, got, i, m) + } + } +} + +// declaredItemType reads the declared `type` of the items of an array-valued +// property in the 200 response of GET path, reading the spec from disk rather +// than from any in-test copy of it. +func declaredItemType(t *testing.T, path, property string) string { + t.Helper() + + raw, err := os.ReadFile(openAPISpecPath) + if err != nil { + t.Fatalf("reading %s: %v", openAPISpecPath, err) + } + var doc struct { + Paths map[string]map[string]struct { + Responses map[string]struct { + Content map[string]struct { + Schema map[string]any `json:"schema"` + } `json:"content"` + } `json:"responses"` + } `json:"paths"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("parsing %s: %v", openAPISpecPath, err) + } + + op, ok := doc.Paths[path]["get"] + if !ok { + t.Fatalf("%s documents no GET %s; this guard has gone stale", openAPISpecPath, path) + } + schema := op.Responses["200"].Content["application/json"].Schema + props, ok := schema["properties"].(map[string]any) + if !ok { + t.Fatalf("GET %s 200 schema has no properties", path) + } + prop, ok := props[property].(map[string]any) + if !ok { + t.Fatalf("GET %s 200 schema does not document %q", path, property) + } + if prop["type"] != "array" { + t.Fatalf("GET %s .%s is declared type %v, want array", path, property, prop["type"]) + } + items, ok := prop["items"].(map[string]any) + if !ok { + t.Fatalf("GET %s .%s declares no items schema", path, property) + } + declared, ok := items["type"].(string) + if !ok { + t.Fatalf("GET %s .%s.items declares no type", path, property) + } + return declared +} + +// jsonKindOf names the OpenAPI primitive type corresponding to a value decoded +// from JSON into any, so a declared `type` can be compared against what a +// handler really sent. +func jsonKindOf(v any) string { + switch v.(type) { + case nil: + return "null" + case bool: + return "boolean" + case float64: + return "number" + case string: + return "string" + case []any: + return "array" + case map[string]any: + return "object" + default: + return "unknown" + } +} diff --git a/src/pkg/dashboard/owner_only_handlers_deny_test.go b/src/pkg/dashboard/owner_only_handlers_deny_test.go index 61e6bc98d..d1bb75b9b 100644 --- a/src/pkg/dashboard/owner_only_handlers_deny_test.go +++ b/src/pkg/dashboard/owner_only_handlers_deny_test.go @@ -52,6 +52,7 @@ func TestV4OwnerOnlyHandlerGapsRejectUnverifiedOwners(t *testing.T) { {"agent config stats", http.MethodPut, "/api/config/agent/scanner/stats", srv.handleAgentConfigStats}, {"agent config tools", http.MethodPut, "/api/config/agent/scanner/tools", srv.handleAgentConfigTools}, {"agent delete", http.MethodDelete, "/api/agents/scanner", srv.handleAgentDelete}, + {"agent login code", http.MethodPost, "/api/agents/scanner/login-code", srv.handleAgentLoginCode}, {"agent prompt save", http.MethodPut, "/api/config/agent/scanner/prompt", srv.handleAgentPromptSave}, {"backup download", http.MethodPost, "/api/backup", srv.handleBackupDownload}, {"backup status", http.MethodGet, "/api/backup/status", srv.handleBackupStatus}, diff --git a/src/pkg/dashboard/pack_pause_ownership_test.go b/src/pkg/dashboard/pack_pause_ownership_test.go new file mode 100644 index 000000000..57ea7771f --- /dev/null +++ b/src/pkg/dashboard/pack_pause_ownership_test.go @@ -0,0 +1,182 @@ +package dashboard + +// Tests for #5706: the ACMM pack visibility sweep must not pause agents whose +// pause/run state the operator owns. Before the fix, syncAgentVisibility — +// which runs inside ApplyPack on EVERY startup ("ACMM pack applied on +// startup") — paused every non-pack agent with reason "agent not in pack +// level N", including reviewer-role agents the operator had explicitly +// created via POST /api/agents and resumed via POST /api/resume/{agent}. The +// operator's run-state silently reverted on every pod roll. Same +// pack-clobbers-operator-config family as #5632; the fix mirrors the +// ModelOwner/BackendOwner (#5558) and cadence-ownership (#5668) markers. + +import ( + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/hivecommons/hive/pkg/config" +) + +// syncLevel is a level whose pack roster contains neither "scanner" nor any +// agent added by these tests (level 1 defines guide + brainstorm only), so +// every test agent is a NON-pack agent from the sweep's point of view. +const syncLevel = 1 + +// TestSyncAgentVisibility_OperatorOwnedNonPackAgentLeftRunning is the +// red-before-fix test for the field report: an operator-created agent +// ("adjudicator", pause_owner: operator) survives the pack sweep running, +// while an unowned non-pack agent is still paused as designed. +func TestSyncAgentVisibility_OperatorOwnedNonPackAgentLeftRunning(t *testing.T) { + s, deps := apiServer(t) + + owned := config.AgentConfig{ + Backend: "claude", + Role: "reviewer", + Enabled: true, + Managed: true, + PauseOwner: config.FieldOwnerOperator, + } + deps.Config.Agents["adjudicator"] = owned + deps.AgentMgr.AddAgent("adjudicator", owned) + + paused, _ := s.syncAgentVisibility(syncLevel) + + for _, name := range paused { + if name == "adjudicator" { + t.Fatalf("pack sweep paused operator-owned agent %q (paused=%v) — the operator's run-state must survive a pack apply", name, paused) + } + } + if deps.AgentMgr.IsPaused("adjudicator") { + t.Fatal("adjudicator is paused after the sweep; an operator-owned non-pack agent must be left running") + } + + // Control: "scanner" carries no ownership marker and is not in the level's + // pack, so the sweep must still pause it — the fix is a targeted skip, not + // a disabling of pack reconciliation. + if !deps.AgentMgr.IsPaused("scanner") { + t.Fatal("scanner (no pause ownership) was not paused; the sweep must still reconcile unowned non-pack agents") + } +} + +// TestSyncAgentVisibility_PackOwnedAgentStillPaused pins the other half of the +// contract: an agent whose pause state is pack-owned (or unowned) still gets +// paused when the level's roster does not include it. +func TestSyncAgentVisibility_PackOwnedAgentStillPaused(t *testing.T) { + s, deps := apiServer(t) + + packMade := config.AgentConfig{ + Backend: "claude", + Enabled: true, + Managed: true, + PauseOwner: config.FieldOwnerPack, + } + deps.Config.Agents["pack-made"] = packMade + deps.AgentMgr.AddAgent("pack-made", packMade) + + _, _ = s.syncAgentVisibility(syncLevel) + + if !deps.AgentMgr.IsPaused("pack-made") { + t.Fatal("pack-owned non-pack agent was not paused; only OPERATOR ownership exempts an agent from the sweep") + } + if !deps.AgentMgr.IsPaused("scanner") { + t.Fatal("unowned non-pack agent was not paused; pack reconciliation must keep working for pack-managed agents") + } +} + +// TestHandleAgentCreate_ClaimsPauseOwnership is the field scenario end to end: +// creating an agent via POST /api/agents stamps operator pause ownership, +// persists it to the agent overlay file, and a subsequent pack sweep (the +// startup path) leaves the agent running instead of pausing it as +// "not in pack level N". +func TestHandleAgentCreate_ClaimsPauseOwnership(t *testing.T) { + s, deps := apiServer(t) + dir := t.TempDir() + deps.Config.Data.AgentsDir = dir + + rec := doPost(s, "/api/agents", map[string]interface{}{ + "name": "adjudicator", + "agent": map[string]interface{}{"backend": "claude", "role": "reviewer"}, + }) + if rec.Code != http.StatusOK { + t.Fatalf("create: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + ac, ok := deps.Config.Agents["adjudicator"] + if !ok { + t.Fatal("adjudicator missing from config after create") + } + if !ac.PauseIsOperatorOwned() { + t.Fatalf("pause_owner = %q after dashboard-API create, want %q", ac.PauseOwner, config.FieldOwnerOperator) + } + + // The marker must be in the overlay FILE, not only in memory: the overlay + // replaces the hive.yaml entry on every config load, so an in-memory-only + // claim would not survive the very restart whose pack apply it guards. + data, err := os.ReadFile(filepath.Join(dir, "adjudicator.yaml")) + if err != nil { + t.Fatalf("reading agent overlay: %v", err) + } + if !strings.Contains(string(data), "pause_owner: "+config.FieldOwnerOperator) { + t.Fatalf("agent overlay does not persist the ownership marker:\n%s", data) + } + + // The startup pack apply must now leave the agent running. + paused, _ := s.syncAgentVisibility(syncLevel) + if deps.AgentMgr.IsPaused("adjudicator") { + t.Fatalf("pack sweep paused the freshly created agent (paused=%v); this is the #5706 boot regression", paused) + } +} + +// TestClaimAgentPauseOwnership_StaysResumedAcrossBoots is the resume-claims- +// ownership round trip: once the operator's resume has claimed ownership, the +// sweep leaves the agent running on this boot AND the next one. +func TestClaimAgentPauseOwnership_StaysResumedAcrossBoots(t *testing.T) { + s, deps := apiServer(t) + + if deps.Config.Agents["scanner"].PauseIsOperatorOwned() { + t.Fatal("precondition: scanner must start with no pause ownership") + } + s.claimAgentPauseOwnership("scanner") + if !deps.Config.Agents["scanner"].PauseIsOperatorOwned() { + t.Fatal("claimAgentPauseOwnership did not mark scanner operator-owned") + } + + // Two sweeps = two boots. Before the fix the FIRST one already re-paused. + for boot := 1; boot <= 2; boot++ { + _, _ = s.syncAgentVisibility(syncLevel) + if deps.AgentMgr.IsPaused("scanner") { + t.Fatalf("boot %d: pack sweep re-paused an agent the operator resumed; the claim must hold across restarts", boot) + } + } + + // The claim is idempotent — a second resume must not lose it. + s.claimAgentPauseOwnership("scanner") + if !deps.Config.Agents["scanner"].PauseIsOperatorOwned() { + t.Fatal("repeat claim dropped the ownership marker") + } +} + +// TestHandleResume_ClaimsPauseOwnership drives the claim through the real +// endpoint: an operator resume of a pack-paused agent stamps operator +// ownership. Resume relaunches the agent session, which can legitimately fail +// without a working tmux (same tolerance as TestHandleResume_RealTransition...), +// so the ownership assertion applies only when the resume succeeded. +func TestHandleResume_ClaimsPauseOwnership(t *testing.T) { + s, deps := apiServer(t) + + if err := deps.AgentMgr.Pause("scanner", "acmm-pack", "agent not in pack level 1"); err != nil { + t.Fatalf("pause: %v", err) + } + rec := doOwnerPost(s, "/api/resume/scanner", nil) + if rec.Code != http.StatusOK && rec.Code != http.StatusBadRequest { + t.Fatalf("resume status = %d, want 200 or 400", rec.Code) + } + if rec.Code == http.StatusOK { + if !deps.Config.Agents["scanner"].PauseIsOperatorOwned() { + t.Fatal("operator resume did not claim pause ownership; the resume would silently last only until the next pod roll") + } + } +} diff --git a/src/pkg/dashboard/server.go b/src/pkg/dashboard/server.go index 08a3acb2c..7bb80b68e 100644 --- a/src/pkg/dashboard/server.go +++ b/src/pkg/dashboard/server.go @@ -188,6 +188,11 @@ type Server struct { deviceFlowMu sync.Mutex deviceFlowState *github.DeviceFlowState + // deviceFlowID binds the in-progress device flow to the caller who started + // it: /start returns it, /poll must present it (both routes are public, and + // the session cookie is minted on the poll response — without this secret + // any anonymous poller could race the operator and steal the session). + deviceFlowID string // userSessions maps a random opaque session id (stored in the client's // hive_session cookie on direct-route spokes) to the authenticated user. @@ -474,6 +479,7 @@ type FrontendAgent struct { PausedTrigger string `json:"pausedTrigger,omitempty"` PausedBy string `json:"pausedBy,omitempty"` OffByCadence bool `json:"offByCadence"` + NoCadence bool `json:"noCadence"` NeedsLogin bool `json:"needsLogin"` AuthAvailable bool `json:"authAvailable"` AuthKnown bool `json:"authKnown"` @@ -487,6 +493,7 @@ type FrontendAgent struct { Pinned bool `json:"pinned"` LastKick string `json:"lastKick,omitempty"` NextKick string `json:"nextKick,omitempty"` + NextKickIn string `json:"nextKickIn,omitempty"` Restarts int `json:"restarts"` LiveSummary string `json:"liveSummary,omitempty"` DetailSummary string `json:"detailSummary,omitempty"` @@ -967,12 +974,34 @@ func (s *Server) Start() error { // on every visit. "/{$}" matches the root path exactly; every other static // path falls through to the plain file server below. if rawIndex, err := fs.ReadFile(staticContent, "index.html"); err == nil { - idx := webstatic.NewIndexDocument(rawIndex) + // Strings are baked in ONCE here, unlike custom.css which is read per + // request: the document carries a precomputed gzip body and a strong + // ETag, so its content cannot vary per request without discarding both. + // Editing branding.json therefore needs a restart; editing the + // stylesheet does not. That asymmetry is documented in branding.md. + branded := webstatic.InjectBranding(applyBranding(rawIndex, s.loadBranding())) + idx := webstatic.NewIndexDocument(branded) + // Hand the FINAL served bytes to the CSP layer explicitly, rather than + // having the document constructor reach out and set global state: + // constructing a document should not silently change the process-wide + // CSP, and a test building a throwaway document must not shrink the + // real allowlist. + setBrandedIndex(branded) s.mux.Handle("GET /{$}", idx) s.mux.Handle("GET /index.html", idx) } else { s.logger.Warn("embedded index.html unavailable; falling back to plain file serving", "error", err) } + // Operator branding override: an optional stylesheet on the data volume, + // served at the path the index document links to. Lets a deployment carry + // its own colours/logo without forking the embedded SPA or rebuilding the + // image — the override is data, not code. + // + // Read per request (not cached at startup) so dropping a file in takes + // effect on reload. It is a single small stylesheet on local disk; the + // index document itself remains startup-precompressed. + s.mux.HandleFunc("GET /branding/custom.css", s.handleBrandingCSS) + s.mux.Handle("GET /", http.FileServer(http.FS(staticContent))) // authenticate is outermost so the identity headers it injects from a @@ -1550,14 +1579,16 @@ async function startFlow(){ document.getElementById('user-code').textContent=d.user_code; document.getElementById('verify-link').href=d.verification_uri; showStep('step-code'); - poll(d.interval||5); + poll(d.interval||5,d.flow_id||''); }catch(e){showError('Network error: '+e.message)} } -async function poll(interval){ +async function poll(interval,flowId){ var ms=interval*1000; async function check(){ try{ - var r=await fetch('/api/gh-user-auth/poll',{method:'POST'}); + // flow_id proves this poll belongs to the flow WE started — the server + // refuses to mint the session for any poll that cannot present it. + var r=await fetch('/api/gh-user-auth/poll',{method:'POST',headers:{'Content-Type':'application/json'},body:JSON.stringify({flow_id:flowId})}); var d=await r.json(); if(d.status==='complete'){showStep('step-done');setTimeout(function(){location.href='/api/gh-user-auth/session'},1000);return} if(d.status==='error'){showError(d.error||'Authorization failed');return} diff --git a/src/pkg/dashboard/static/index.html b/src/pkg/dashboard/static/index.html index 4c5844935..f258d2b22 100644 --- a/src/pkg/dashboard/static/index.html +++ b/src/pkg/dashboard/static/index.html @@ -1047,6 +1047,15 @@ /* Last upstream/launch error — stacked below .agent-state, never under the working-indicator dot, so a retry in progress cannot hide it (#3604). */ .agent-error { font-size: 0.72rem; color: var(--red); margin-top: 4px; text-align: right; word-break: break-word; } + .agent-blockers { display: flex; flex-wrap: wrap; align-items: center; justify-content: flex-end; gap: 2px 6px; font-size: 0.68rem; margin-top: 4px; color: var(--muted); } + .agent-blockers .ab-key { opacity: 0.65; margin-right: 3px; } + .agent-blockers .ab-seg { white-space: nowrap; } + .agent-blockers .ab-sep { opacity: 0.35; } + .agent-blockers .ab-ok { color: var(--green); } + .agent-blockers .ab-bad { color: var(--red); font-weight: 600; } + .oc-detail-blockers { margin-top: 6px; display: flex; flex-wrap: wrap; align-items: center; gap: 6px; } + .never-scheduled-chip { font-size: 0.62rem; font-weight: 700; text-transform: uppercase; letter-spacing: 0.4px; color: var(--yellow); background: none; border: 1px solid var(--yellow); border-radius: 3px; padding: 1px 5px; margin-left: 6px; cursor: pointer; vertical-align: middle; white-space: nowrap; } + .never-scheduled-chip:hover { background: color-mix(in srgb, var(--yellow) 18%, transparent); } .agent-name { font-size: 1.1rem; font-weight: 700; margin-bottom: 8px; } .agent-name .dot { display: inline-block; width: 8px; height: 8px; @@ -1180,7 +1189,7 @@ .reco-advisory-note { margin-top: 12px; font-size: 0.68rem; color: var(--muted); font-style: italic; } .reco-calm { color: var(--muted); font-size: 0.85rem; } - .lc-fleet { display: flex; gap: 10px; flex-wrap: wrap; margin-bottom: 14px; } + .lc-fleet { display: flex; gap: 10px; flex-wrap: wrap; margin-bottom: 6px; } .lc-fleet-stat { flex: 1 1 100px; min-width: 90px; background: var(--bg); border: 1px solid var(--border); border-radius: 6px; padding: 8px 12px; } .lc-fleet-stat .val { font-size: 1.3rem; font-weight: 700; } @@ -1188,9 +1197,10 @@ .lc-fleet-stat .val.merged { color: var(--green); } .lc-fleet-stat .val.blocked { color: var(--red); } .lc-fleet-stat .lbl { font-size: 0.62rem; text-transform: uppercase; letter-spacing: 0.06em; color: var(--muted); margin-top: 2px; } - .lc-events { list-style: none; margin: 0; padding: 0; display: flex; flex-direction: column; gap: 4px; + .lc-window { color: var(--muted); font-size: 0.68rem; margin-bottom: 12px; } + .lc-journeys { list-style: none; margin: 0; padding: 0; display: flex; flex-direction: column; gap: 4px; max-height: 320px; overflow-y: auto; } - .lc-event { display: flex; align-items: center; gap: 8px; font-size: 0.78rem; padding: 5px 8px; + .lc-journey { display: flex; align-items: center; gap: 8px; font-size: 0.78rem; padding: 5px 8px; border-radius: 6px; background: var(--bg); border: 1px solid var(--border); } .lc-kind { flex: 0 0 auto; font-size: 0.6rem; font-weight: 700; letter-spacing: 0.04em; text-transform: uppercase; padding: 2px 7px; border-radius: 8px; } @@ -1198,10 +1208,13 @@ .lc-kind.blue { background: color-mix(in srgb, var(--blue) 18%, transparent); color: var(--blue); } .lc-kind.green { background: var(--green-bg); color: var(--green); } .lc-kind.red { background: var(--red-bg); color: var(--red); } - .lc-event .lc-ref { color: var(--text); font-weight: 600; font-family: var(--font-mono); font-size: 0.72rem; + .lc-journey .lc-ref { color: var(--text); font-weight: 600; font-family: var(--font-mono); font-size: 0.72rem; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } - .lc-event .lc-agent { color: var(--muted); font-size: 0.72rem; } - .lc-event .lc-time { color: var(--muted); font-size: 0.7rem; margin-left: auto; flex: 0 0 auto; } + .lc-stages { display: flex; align-items: center; gap: 3px; flex: 0 1 auto; min-width: 0; flex-wrap: wrap; } + .lc-stage-arrow { color: var(--muted); font-size: 0.6rem; } + .lc-stage.past { opacity: 0.55; } + .lc-journey .lc-agent { color: var(--muted); font-size: 0.72rem; } + .lc-journey .lc-time { color: var(--muted); font-size: 0.7rem; margin-left: auto; flex: 0 0 auto; } .lc-empty { color: var(--muted); font-size: 0.85rem; padding: 6px 0; } /* Repos */ @@ -1241,6 +1254,16 @@ .repo-pr-pill .pill-num { font-weight: 700; white-space: nowrap; } .repo-pr-pill .pill-title { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } .repo-pr-pill .pill-merge-icon { font-size: 0.6rem; margin-left: 1px; } + /* needs-human: the escalation ledger (pkg/escalation) applies the + `needs-human` GitHub label to a PR once automated fix attempts are + exhausted. The pill turns status-alert orange (NOT brand amber) and a + compact companion badge names the state, so parked PRs stand out + instead of blending into the purple PR list — nine escalated PRs were + invisible to the operator for a week without this. */ + .repo-pr-pill.needs-human { --pill-c: var(--orange); } + .pill-needs-human-badge { flex: 0 0 auto; font-weight: 600; white-space: nowrap; } + /* Section-header counter: "· N need human review" next to the host badge. */ + .repos-needs-human-count { display: none; font-size: 0.62em; font-weight: 600; vertical-align: middle; margin-left: 6px; color: var(--orange); } /* Sparklines */ .sparkline { display: inline-block; vertical-align: middle; margin-left: 6px; flex-shrink: 0; } @@ -3219,7 +3242,7 @@
      -

      Repositories

      +

      Repositories

      ` marker, so the escape hatch +# derive-release-version.sh honours can also ride in a fragment. +# - Each fragment is deleted after a successful compile, so `git add -A +# changelog.d` in the release commit records the consumption. +# +# Usage: src/scripts/compile-changelog.sh [changelog-path] [fragments-dir] +# changelog-path default CHANGELOG.md +# fragments-dir default changelog.d +set -euo pipefail + +CHANGELOG="${1:-CHANGELOG.md}" +FRAGDIR="${2:-changelog.d}" + +# Canonical subsection order for newly created subsections (keep-a-changelog +# order; existing subsections keep their position, entries are only appended). +CATEGORIES=(added changed deprecated fixed security) + +category_heading() { + case "$1" in + added) echo "### Added" ;; + changed) echo "### Changed" ;; + deprecated) echo "### Deprecated" ;; + fixed) echo "### Fixed" ;; + security) echo "### Security" ;; + *) return 1 ;; + esac +} + +if [[ ! -f "$CHANGELOG" ]]; then + echo "::error::changelog not found at ${CHANGELOG}" >&2 + exit 1 +fi + +if [[ ! -d "$FRAGDIR" ]]; then + echo "No ${FRAGDIR}/ directory — nothing to compile." + exit 0 +fi + +shopt -s nullglob +fragments=() +for f in "$FRAGDIR"/*.md; do + base="$(basename "$f")" + [[ "$base" == "README.md" ]] && continue + fragments+=("$f") +done +shopt -u nullglob + +if [[ ${#fragments[@]} -eq 0 ]]; then + echo "No fragments in ${FRAGDIR}/ — nothing to compile." + exit 0 +fi + +# The Unreleased heading must exist, or the awk below would pass the file +# through unchanged and the fragments would be deleted without ever landing +# anywhere — silent data loss, the one failure mode a compiler cannot have. +if ! grep -qE '^## Unreleased[[:space:]]*$' "$CHANGELOG"; then + echo "::error::${CHANGELOG} has no '## Unreleased' heading — refusing to compile fragments into nowhere." >&2 + exit 1 +fi + +# --------------------------------------------------------------------------- +# Validate every fragment BEFORE touching anything, so a bad fragment fails +# the whole run with the changelog untouched and every fragment still on disk. +# --------------------------------------------------------------------------- +bad=0 +for f in "${fragments[@]}"; do + base="$(basename "$f")" + if ! [[ "$base" =~ ^(added|changed|deprecated|fixed|security)-[A-Za-z0-9][A-Za-z0-9._-]*\.md$ ]]; then + echo "::error::${f}: fragment name must be -.md with category one of added/changed/deprecated/fixed/security (see ${FRAGDIR}/README.md)" >&2 + bad=1 + continue + fi + first_line="$(grep -m1 '[^[:space:]]' "$f" || true)" + if [[ -z "$first_line" ]]; then + echo "::error::${f}: fragment is empty" >&2 + bad=1 + elif ! [[ "$first_line" == "- "* || "$first_line" == "' marker); it is the entry itself, not a section with its own headings" >&2 + bad=1 + fi +done +if [[ "$bad" -ne 0 ]]; then + exit 1 +fi + +workdir="$(mktemp -d)" +trap 'rm -rf "$workdir"' EXIT + +# --------------------------------------------------------------------------- +# Group fragment bodies by category, trailing blank lines stripped, sorted by +# filename within the category so the compiled order is deterministic. +# --------------------------------------------------------------------------- +for cat in "${CATEGORIES[@]}"; do + catfile="${workdir}/${cat}.entries" + while IFS= read -r f; do + [[ -z "$f" ]] && continue + # Strip trailing blank lines; keep internal continuation lines verbatim. + awk 'NF { last = NR } { lines[NR] = $0 } END { for (i = 1; i <= last; i++) print lines[i] }' "$f" >> "$catfile" + done < <(printf '%s\n' "${fragments[@]}" | grep -E "/${cat}-[^/]+\.md$" | LC_ALL=C sort || true) +done + +# --------------------------------------------------------------------------- +# Insert each category's entries into the Unreleased section: appended to the +# existing `### ` subsection when present, else as a new subsection +# at the end of Unreleased. Blank lines inside the section are buffered so new +# entries land directly after the last existing entry, with the surrounding +# blank-line hygiene (one blank line before the next `##`/`###` heading) +# preserved — the shape tagged-release.yml's move step and +# derive-release-version.sh both parse. +# --------------------------------------------------------------------------- +compiled="${workdir}/CHANGELOG.compiled" +cp "$CHANGELOG" "$compiled" + +for cat in "${CATEGORIES[@]}"; do + catfile="${workdir}/${cat}.entries" + [[ -s "$catfile" ]] || continue + heading="$(category_heading "$cat")" + next="${workdir}/CHANGELOG.next" + awk -v heading="$heading" -v ef="$catfile" ' + function dump( line) { + while ((getline line < ef) > 0) print line + close(ef) + } + function flushblanks( i) { + for (i = 0; i < nb; i++) print "" + nb = 0 + } + BEGIN { in_unrel = 0; in_cat = 0; inserted = 0; nb = 0 } + { + if (!in_unrel) { + print + if ($0 ~ /^## Unreleased[[:space:]]*$/) in_unrel = 1 + next + } + if ($0 ~ /^## /) { + # Unreleased ends here. Land the entries first, then exactly one + # blank line before this next release heading. + if (in_cat && !inserted) { dump(); inserted = 1; in_cat = 0 } + else if (!inserted) { print ""; print heading; print ""; dump(); inserted = 1 } + print "" + nb = 0 + in_unrel = 0 + print + next + } + if ($0 ~ /^[[:space:]]*$/) { nb++; next } + if (in_cat && $0 ~ /^### /) { + # The target subsection ends at the next subsection: append the new + # entries right after its last existing line, then restore the + # buffered blank line(s) that preceded this heading. + dump(); inserted = 1; in_cat = 0 + flushblanks() + print + next + } + flushblanks() + print + if (!inserted && $0 == heading) in_cat = 1 + } + END { + if (in_unrel) { + if (in_cat && !inserted) { dump(); inserted = 1 } + else if (!inserted) { print ""; print heading; print ""; dump(); inserted = 1 } + print "" + } + } + ' "$compiled" > "$next" + mv "$next" "$compiled" +done + +mv "$compiled" "$CHANGELOG" + +for f in "${fragments[@]}"; do + rm "$f" +done + +echo "Compiled ${#fragments[@]} fragment(s) from ${FRAGDIR}/ into ${CHANGELOG}'s Unreleased section and deleted them." diff --git a/src/scripts/test-compile-changelog.sh b/src/scripts/test-compile-changelog.sh new file mode 100755 index 000000000..efdcf81ca --- /dev/null +++ b/src/scripts/test-compile-changelog.sh @@ -0,0 +1,230 @@ +#!/usr/bin/env bash +# test-compile-changelog.sh — exercises src/scripts/compile-changelog.sh +# against fixture fragments and fixture CHANGELOG.md files, asserting the +# PROPERTIES the release pipeline depends on rather than the script's shape +# (#5675): +# +# - fragments land under the right `###` subsection of `## Unreleased`, +# appended AFTER existing entries, with released sections byte-untouched; +# - a category with no existing subsection gets one created inside +# Unreleased, never leaking past the next `## ` release heading — +# tagged-release.yml's move step and derive-release-version.sh both parse +# that boundary; +# - an empty / absent fragments dir is an exact no-op (byte-identical file, +# exit 0) — the idempotency of tagged-release.yml's self-retrigger loop +# rides on this; +# - a second run after a compile is that same no-op (fragments consumed); +# - an unrecognized category prefix fails LOUDLY with the changelog +# untouched and every fragment still on disk (a miscategorized entry +# would change the derived semver bump); +# - the compiled result is exactly what derive-release-version.sh needs: +# an added- fragment flips an empty Unreleased to a minor release. +# +# Usage: src/scripts/test-compile-changelog.sh +set -uo pipefail + +script_dir=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) +compile="$script_dir/compile-changelog.sh" +derive="$script_dir/derive-release-version.sh" +tmp_root=${TMPDIR:-"$script_dir/../.test-tmp"} +mkdir -p "$tmp_root" +tmp=$(mktemp -d "$tmp_root/compile-changelog.XXXXXX") +trap 'rm -rf "$tmp"' EXIT + +fail=0 +note_fail() { echo " FAIL: $*"; fail=1; } +note_ok() { echo " ok: $*"; } + +# A fresh case directory with a fixture CHANGELOG carrying an existing +# Unreleased Fixed entry and one released section — the live file's shape. +new_case() { + local dir="$tmp/case-$1" + mkdir -p "$dir/changelog.d" + cat > "$dir/CHANGELOG.md" <<'EOF' +# Changelog + +Intro prose that must never move. + +## Unreleased + +### Fixed + +- an existing unreleased fix ([#1](https://example.invalid/1)). + +## 2026-01-01 (v1.0.0) + +### Added + +- a released thing that must stay byte-identical. +EOF + echo "$dir" +} + +# --------------------------------------------------------------------------- +# Case 1: fixture fragments => expected CHANGELOG, exactly. +# One fragment appends to the existing Fixed subsection; one creates a new +# Added subsection; the entries stay inside Unreleased. +# --------------------------------------------------------------------------- +dir=$(new_case 1) +printf -- '- squashed a new bug ([#2](https://example.invalid/2)).\n' > "$dir/changelog.d/fixed-2-new-bug.md" +printf -- '- grew a new feature ([#3](https://example.invalid/3)).\n' > "$dir/changelog.d/added-3-feature.md" +printf -- 'this is documentation, not an entry\n' > "$dir/changelog.d/README.md" + +if ( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > "$tmp/case1.out" 2>&1; then + note_ok "compile succeeds on well-formed fragments" +else + note_fail "compile failed on well-formed fragments: $(cat "$tmp/case1.out")" +fi + +cat > "$tmp/case1.expected" <<'EOF' +# Changelog + +Intro prose that must never move. + +## Unreleased + +### Fixed + +- an existing unreleased fix ([#1](https://example.invalid/1)). +- squashed a new bug ([#2](https://example.invalid/2)). + +### Added + +- grew a new feature ([#3](https://example.invalid/3)). + +## 2026-01-01 (v1.0.0) + +### Added + +- a released thing that must stay byte-identical. +EOF +if diff -u "$tmp/case1.expected" "$dir/CHANGELOG.md" > "$tmp/case1.diff"; then + note_ok "fragments compile to exactly the expected CHANGELOG (append to existing Fixed, new Added subsection, released section untouched)" +else + note_fail "compiled CHANGELOG differs from expected:"$'\n'"$(cat "$tmp/case1.diff")" +fi + +if [[ ! -e "$dir/changelog.d/fixed-2-new-bug.md" && ! -e "$dir/changelog.d/added-3-feature.md" ]]; then + note_ok "consumed fragments are deleted" +else + note_fail "fragments should be deleted after a compile" +fi +if [[ -f "$dir/changelog.d/README.md" ]]; then + note_ok "changelog.d/README.md survives the compile" +else + note_fail "README.md must never be consumed as a fragment" +fi + +# --------------------------------------------------------------------------- +# Case 2: idempotency — a second run right after the compile is a no-op. +# --------------------------------------------------------------------------- +cp "$dir/CHANGELOG.md" "$tmp/case2.before" +if ( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > /dev/null 2>&1 \ + && cmp -s "$tmp/case2.before" "$dir/CHANGELOG.md"; then + note_ok "second run after a compile is a byte-identical no-op" +else + note_fail "second run must exit 0 and leave the changelog byte-identical" +fi + +# --------------------------------------------------------------------------- +# Case 3: empty dir and absent dir are exact no-ops. tagged-release.yml's +# self-retrigger loop depends on this: the release commit empties both +# Unreleased and changelog.d/, and the follow-up run must derive nothing. +# --------------------------------------------------------------------------- +dir=$(new_case 3) +cp "$dir/CHANGELOG.md" "$tmp/case3.before" +if ( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > /dev/null 2>&1 \ + && cmp -s "$tmp/case3.before" "$dir/CHANGELOG.md"; then + note_ok "empty changelog.d/ is a byte-identical no-op" +else + note_fail "empty changelog.d/ must exit 0 and leave the changelog byte-identical" +fi +rmdir "$dir/changelog.d" +if ( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > /dev/null 2>&1 \ + && cmp -s "$tmp/case3.before" "$dir/CHANGELOG.md"; then + note_ok "absent changelog.d/ is a byte-identical no-op" +else + note_fail "absent changelog.d/ must exit 0 and leave the changelog byte-identical" +fi + +# --------------------------------------------------------------------------- +# Case 4: unrecognized category prefix fails loudly, changelog untouched, +# every fragment still on disk (including the valid one — all-or-nothing). +# --------------------------------------------------------------------------- +dir=$(new_case 4) +printf -- '- entry under a made-up category.\n' > "$dir/changelog.d/misc-oops.md" +printf -- '- a valid entry riding along.\n' > "$dir/changelog.d/fixed-9-valid.md" +cp "$dir/CHANGELOG.md" "$tmp/case4.before" +if ( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > "$tmp/case4.out" 2>&1; then + note_fail "an unrecognized category prefix must fail, but the script exited 0" +else + note_ok "unrecognized category prefix fails loudly" +fi +if cmp -s "$tmp/case4.before" "$dir/CHANGELOG.md" \ + && [[ -f "$dir/changelog.d/misc-oops.md" && -f "$dir/changelog.d/fixed-9-valid.md" ]]; then + note_ok "a failed compile touches nothing: changelog byte-identical, all fragments still on disk" +else + note_fail "a failed compile must leave the changelog and every fragment untouched" +fi +if grep -q "misc-oops" "$tmp/case4.out"; then + note_ok "the failure names the offending fragment" +else + note_fail "the failure should name the offending fragment, got: $(cat "$tmp/case4.out")" +fi + +# --------------------------------------------------------------------------- +# Case 5: a fragment that is not an entry bullet (its own heading, prose) +# fails — a fragment IS the entry, not a section. +# --------------------------------------------------------------------------- +dir=$(new_case 5) +printf -- '### Fixed\n\n- an entry hiding under its own heading.\n' > "$dir/changelog.d/fixed-10-heading.md" +if ( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > /dev/null 2>&1; then + note_fail "a fragment carrying its own heading must be rejected" +else + note_ok "a fragment that is not a '- ' entry bullet is rejected" +fi + +# --------------------------------------------------------------------------- +# Case 6: end-to-end property the release pipeline depends on — compiling an +# added- fragment into an EMPTY Unreleased makes derive-release-version.sh +# derive a minor release, where before the compile it derived none. This is +# the decide-job ordering (compile before derive) proven as a property. +# --------------------------------------------------------------------------- +dir="$tmp/case-6" +mkdir -p "$dir/changelog.d" +cat > "$dir/CHANGELOG.md" <<'EOF' +# Changelog + +## Unreleased + +## 2026-01-01 (v1.0.0) + +### Added + +- a released thing. +EOF +git init -q "$dir" +git -C "$dir" config user.email test@example.com +git -C "$dir" config user.name "Test" +git -C "$dir" -c commit.gpgsign=false commit -q --allow-empty -m init +git -C "$dir" tag v1.0.0 +before=$( cd "$dir" && GITHUB_OUTPUT="" bash "$derive" CHANGELOG.md ) +printf -- '- a fragment-borne feature.\n' > "$dir/changelog.d/added-11-feature.md" +( cd "$dir" && bash "$compile" CHANGELOG.md changelog.d ) > /dev/null 2>&1 +after=$( cd "$dir" && GITHUB_OUTPUT="" bash "$derive" CHANGELOG.md ) +if printf '%s\n' "$before" | grep -q '^release=false$' \ + && printf '%s\n' "$after" | grep -q '^release=true$' \ + && printf '%s\n' "$after" | grep -q '^bump=minor$' \ + && printf '%s\n' "$after" | grep -q '^version=1.1.0$'; then + note_ok "compiled added- fragment flips derive-release-version.sh from no-release to a 1.1.0 minor release" +else + note_fail "derive before/after compile mismatch. before: [$before] after: [$after]" +fi + +echo "" +if [[ "$fail" -eq 0 ]]; then + echo "test-compile-changelog: all cases passed" +else + echo "test-compile-changelog: FAILURES above" +fi +exit "$fail" diff --git a/systemd/hive-discord.service b/systemd/hive-discord.service index 142d8f7d0..10e9d54bf 100644 --- a/systemd/hive-discord.service +++ b/systemd/hive-discord.service @@ -12,7 +12,40 @@ WorkingDirectory=/tmp/hive/discord EnvironmentFile=/etc/hive/discord.env EnvironmentFile=-/etc/hive/supervisor.env Environment=NODE_ENV=production +# Refuse to start unless the checkout we execute from is owned by this service +# user and is not group/other-writable (#5435). +# +# WHY THIS GUARD EXISTS. WorkingDirectory is under /tmp, which is world-writable +# and cleared on reboot. The sticky bit only stops a user from deleting or +# renaming entries owned by someone ELSE — it does not stop them from CREATING +# /tmp/hive/discord/bot.js in the window after a reboot wipes /tmp and before +# hive-deploy repopulates the checkout. systemd would then run that file as +# `dev` with the Discord bot token from discord.env; Requires=hive.service +# orders startup but validates nothing about who owns the code. +# +# The path stays under /tmp deliberately. HIVE_REPO_DIR's default is /tmp/hive +# (bin/hive-config.sh, bin/hive-deploy.sh) but the literal /tmp/hive is also +# hardcoded in bin/hive.sh, bin/kick-agents.sh and the ExecStart of +# hive-snapshot.service; relocating the checkout is a deploy-layout change +# across all of those, not a fix to this unit. This guard closes the +# code-execution path without moving anything. +# +# The guard is a script, not an inline `test`/`find`: it must assert on BOTH +# the directory and the file, and `find ... -print -quit` exits 0 even when it +# matches nothing, so an inline find would be a no-op guard. +# +# Test with: bash src/deploy/test_hive_discord_unit_contract.sh +ExecStartPre=/usr/local/bin/hive-checkout-guard.sh /tmp/hive/discord bot.js ExecStart=/usr/bin/node bot.js +# Hardening. PrivateTmp is deliberately NOT set: it would give this unit a +# private /tmp namespace, hiding the very checkout WorkingDirectory points at, +# and the service would fail to start. ProtectSystem=full leaves /tmp writable +# (strict would not) while making /usr and /etc read-only. +NoNewPrivileges=yes +ProtectSystem=full +ProtectHome=read-only +PrivateDevices=yes +RestrictSUIDSGID=yes Restart=always RestartSec=10 StandardOutput=journal diff --git a/systemd/hive-snapshot.service b/systemd/hive-snapshot.service index 7f12774bb..fbe6fa992 100644 --- a/systemd/hive-snapshot.service +++ b/systemd/hive-snapshot.service @@ -9,7 +9,47 @@ User=dev Environment=PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin Environment=HIVE_DASHBOARD_URL=http://localhost:3001 Environment=DOCS_REPO_DIR=/tmp/kubestellar-docs-snapshot +# Refuse to start unless the checkout we execute from is owned by this service +# user and is not group/other-writable (#5483, same class as #5435). +# +# WHY THIS GUARD EXISTS. ExecStart resolves out of /tmp/hive, which sits under a +# world-writable parent that is cleared on reboot. /tmp's sticky bit only stops +# a user deleting or renaming entries owned by someone ELSE — it does not stop +# them CREATING /tmp/hive/dashboard/publish-snapshot.sh in the window after a +# reboot wipes /tmp and before hive-deploy repopulates the checkout. systemd +# would then run that file as `dev` the next time hive-snapshot.timer fires. +# +# Lower severity than #5435 but not zero. This unit is Type=oneshot on a timer, +# not Restart=always, so the attacker executes on the timer's 15-minute schedule +# rather than immediately at boot, and the unit carries no EnvironmentFile so no +# token is handed to it by systemd. The script itself, however, reads a GitHub +# App token from /var/run/hive-metrics/gh-app-token.cache and pushes to +# kubestellar/docs, so `dev` execution here still reaches a write credential. +# +# The path stays under /tmp deliberately: the literal /tmp/hive is hardcoded in +# bin/hive.sh, bin/kick-agents.sh and this ExecStart, so relocating the checkout +# is a deploy-layout change across all of them, not a fix to this unit. +# +# The guard is a script, not an inline `test`/`find`: `find ... -print -quit` +# exits 0 even when it matches nothing, so an inline find would be a no-op guard. +# +# Test with: bash src/deploy/test_hive_snapshot_unit_contract.sh +ExecStartPre=/usr/local/bin/hive-checkout-guard.sh /tmp/hive/dashboard publish-snapshot.sh ExecStart=/tmp/hive/dashboard/publish-snapshot.sh +# Hardening. PrivateTmp is deliberately NOT set: it would give this unit a +# private /tmp namespace, hiding both the checkout ExecStart runs from AND +# DOCS_REPO_DIR=/tmp/kubestellar-docs-snapshot, which the script clones into and +# commits from. ProtectSystem=full leaves /tmp writable (strict would not) while +# making /usr and /etc read-only. +# +# ProtectHome is NOT read-only here, unlike hive-discord.service: this script +# drives `git` and `gh`, which write to $HOME (gh refreshes ~/.config/gh state, +# git writes lock files next to ~/.gitconfig). A read-only home would break the +# push/PR path rather than harden it. +NoNewPrivileges=yes +ProtectSystem=full +PrivateDevices=yes +RestrictSUIDSGID=yes TimeoutStartSec=120 StandardOutput=journal StandardError=journal diff --git a/systemd/ttyd-hive.service b/systemd/ttyd-hive.service index cf0ad3b29..eb2b42903 100644 --- a/systemd/ttyd-hive.service +++ b/systemd/ttyd-hive.service @@ -5,7 +5,13 @@ After=network.target [Service] Type=simple User=dev -ExecStart=/usr/bin/ttyd -W -a -p 7681 -t fontSize=14 -t disableLeaveAlert=true /usr/local/bin/ttyd-tmux.sh +# Loopback-only by default: ttyd here is writable (-W) and unauthenticated, so +# binding all interfaces hands a shell as this user to any network peer. This +# mirrors the containerized default (TTYD_BIND=127.0.0.1 in +# src/deploy/entrypoint.sh). To expose it beyond localhost, front it with the +# authenticated dashboard proxy, or change -i and add a credential +# (-c user:pass) via a drop-in override. +ExecStart=/usr/bin/ttyd -W -a -i 127.0.0.1 -p 7681 -t fontSize=14 -t disableLeaveAlert=true /usr/local/bin/ttyd-tmux.sh Restart=always RestartSec=5